[TASK-14494] fix: minor UI issues #1208
Conversation
Zishan-7
commented
Sep 12, 2025
Zishan-7
commented
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdjusts layout/styling in the IframeWrapper (height tweaks) and TransactionDetails drawer (padding changes). Adds an optional className prop to TransactionDetailsReceipt to support horizontal padding. No logic or behavior changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Global/IframeWrapper/index.tsx (1)
91-106: Validate postMessage origin and source to avoid spoofed close eventsThe message handler trusts any window and any origin. A malicious page can postMessage to your app and force-close the modal. Check both
event.originagainstnew URL(src).originandevent.sourceagainst the iframe’scontentWindow.Apply this diff:
@@ - useEffect(() => { - const handleMessage = (event: MessageEvent) => { - const data = event.data - if (data?.name === 'complete' && data?.metadata?.status === 'completed') { - onClose('completed') - } - // @dev note: kinda hacky, but tos modal takes too long to close using websocket, so we use the signedAgreementId to close it - // persona fires this event when the user clicks the "accept" button within the iframe - if (data?.signedAgreementId) { - onClose('tos_accepted') - } - } - - window.addEventListener('message', handleMessage) - return () => window.removeEventListener('message', handleMessage) - }, [onClose]) + useEffect(() => { + const allowedOrigin = (() => { + try { + return new URL(src, window.location.href).origin + } catch { + return null + } + })() + + const handleMessage = (event: MessageEvent) => { + if (!allowedOrigin || event.origin !== allowedOrigin) return + if (event.source !== iframeRef.current?.contentWindow) return + const data = event.data + if (data?.name === 'complete' && data?.metadata?.status === 'completed') { + onClose('completed') + } + if (data?.signedAgreementId) { + onClose('tos_accepted') + } + } + + window.addEventListener('message', handleMessage) + return () => window.removeEventListener('message', handleMessage) + }, [onClose, src])And add the iframe ref:
@@ - const [isVerificationStarted, setIsVerificationStarted] = useState(false) + const [isVerificationStarted, setIsVerificationStarted] = useState(false) + const iframeRef = useRef<HTMLIFrameElement>(null) @@ - <iframe + <iframe + ref={iframeRef}
🧹 Nitpick comments (3)
src/components/Global/IframeWrapper/index.tsx (3)
141-146: Height re-balance looks fine; consider flex-based layout to avoid % height edge cases85%/15% sums cleanly, but percent heights inside scrolling containers can produce nested scroll/overflow on iOS. A flex-based approach is more resilient; also prefer Tailwind over inline styles for consistency.
Minimal diff to switch to flex and Tailwind:
- <div className="h-full w-full flex-grow overflow-scroll"> - <iframe + <div className="flex h-full w-full flex-col overflow-auto"> + <iframe key={src} src={src} allow="camera;" - style={{ width: '100%', height: '85%', border: 'none' }} - className="rounded-md" + className="rounded-md w-full flex-1 border-0" sandbox="allow-same-origin allow-scripts allow-forms allow-popups allow-modals allow-top-navigation-by-user-activation" /> - <div className="flex h-[15%] w-full flex-col items-center justify-center gap-4 px-5"> + <div className="flex w-full flex-col items-center justify-center gap-4 px-5 py-4 shrink-0">
24-29: Clipboard: add a small catch for rejected writes
navigator.clipboard.writeTextcan fail (permissions/HTTP). Add acatchto surface a helpful message or fallback.- navigator.clipboard.writeText(textToCopy).then(() => { + navigator.clipboard.writeText(textToCopy).then(() => { setCopied(true) setTimeout(() => setCopied(false), 2000) - }) + }).catch(() => { + // Optional: toast/error UI + setCopied(false) + })
116-120: String contains check for TOS is brittle
src.includes('tos')can misfire. Prefer URL parsing and explicit pathname check.- if (src.includes('tos')) { + const pathname = (() => { try { return new URL(src, location.href).pathname } catch { return '' } })() + if (pathname.toLowerCase().includes('/tos')) { onClose('manual') return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Global/IframeWrapper/index.tsx(1 hunks)src/components/TransactionDetails/TransactionDetailsDrawer.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/TransactionDetails/TransactionDetailsDrawer.tsx (1)
src/components/Global/Drawer/index.tsx (1)
DrawerContent(86-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (3)
src/components/Global/IframeWrapper/index.tsx (1)
143-144: Reconfirm sandbox permissions are the minimum required
allow-same-origin,allow-popups, andallow-top-navigation-by-user-activationbroaden capabilities. If Persona flow doesn’t require all of them, tighten the list.Quick checks to run in QA:
- Does verification still work if you drop
allow-popups?- Is
allow-same-originactually needed for the KYC vendor embed?- Is top navigation required, or does the flow use in-iframe redirects only?
src/components/TransactionDetails/TransactionDetailsDrawer.tsx (2)
46-46: LGTM on DrawerContent padding tweakSwitching to
py-5here and delegating horizontal padding to the child makes spacing ownership clearer.
56-56: TransactionDetailsReceipt already forwards className; nested px found — decide whether to remove inner padding
- TransactionDetailsReceipt accepts className?: HTMLDivElement['className'] and forwards it to the outermost wrapper: src/components/TransactionDetails/TransactionDetailsReceipt.tsx:280 — <div ref={contentRef} className={twMerge('space-y-4', className)}>
- Drawer passes the padding prop: src/components/TransactionDetails/TransactionDetailsDrawer.tsx:56 — className="px-5".
- There is a nested Card with its own horizontal padding: src/components/TransactionDetails/TransactionDetailsReceipt.tsx:301 — className="px-4 py-0". Remove or align that inner px-4 if you want the Drawer’s px-5 to be the single source of horizontal padding.
Likely an incorrect or invalid review comment.
2 participants
