[TASK-6625 & 6622] fix kyc iframe dimensions & cose mechanism #498
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
nezz0746
changed the title
|
|
||
| const areYouSurePromptRow = ( | ||
| <div className='flex flex-col text-center sm:text-left sm:flex-row justify-between sm:items-center gap-4 p-2 sm:p-0 w-full'> | ||
| <p className="text-sm ml-1">Are you sure ? your KYC progress will be lost.</p> |
There was a problem hiding this comment.
issue(blocking): message is specific to KYC flow, component is used in other flows
In particular, this message is confusing (and wrong) to get on the accept TOS flow (that must be closed manually also).
suggestion: I would add a confirmationMessage optional prop and show that message if exists or just direclty close if not
There was a problem hiding this comment.
Yes, component is only used in TOS & KYC but I agree
There was a problem hiding this comment.
I would rather not have confirmation on the TOS, because as part of normal flow the user must close the modal after acepting, and there is no progress to be lost (either they have accepted or not, is not too inconvinient to the user to close like in the kwy case). For example, here I completed and now want to close the modal but I'm afraid to "lose progress"
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/Global/IframeWrapper/index.tsx (3)
Line range hint 17-33: Consider renaming the state variable for clarity.
The state variable showCloseConfirmMessage could be more descriptive. Consider isCloseConfirmationVisible to better reflect its purpose.
The message event handler correctly validates the origin before processing the close event.
-const [showCloseConfirmMessage, setShowCloseConfirmMessage] = useState(false)
+const [isCloseConfirmationVisible, setIsCloseConfirmationVisible] = useState(false)34-47: Rename confirmation dialog component for clarity.
The variable name areYouSurePromptRow could be more descriptive. Consider closeConfirmationDialog to better reflect its purpose and content.
-const areYouSurePromptRow = (
+const closeConfirmationDialog = (77-77: Consider moving inline styles to CSS classes.
For consistency and maintainability, consider moving the inline styles to CSS classes.
-style={{ width: '100%', height: '100%', border: 'none' }}
+className="w-full h-full border-none rounded-md"src/components/Global/KYCComponent/index.tsx (1)
36-42: Consider improving the default close confirmation message and handler.
The default message "Are you sure ?" is too generic and doesn't provide context to the user.
Consider this improvement:
const [iframeOptions, setIframeOptions] = useState<IFrameWrapperProps>({
src: '',
visible: false,
- onClose: () => {
- setIframeOptions({ ...iframeOptions, visible: false })
- },
- closeConfirmMessage: 'Are you sure ?',
+ onClose: () => setIframeOptions(prev => ({ ...prev, visible: false })),
+ closeConfirmMessage: 'Are you sure you want to close this window?',
})📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/components/Global/IframeWrapper/index.tsx (3 hunks)
- src/components/Global/KYCComponent/index.tsx (5 hunks)
🔇 Additional comments (6)
src/components/Global/IframeWrapper/index.tsx (3)
4-9: LGTM! Well-structured type definition.
The optional closeConfirmMessage prop effectively addresses the previous concern about KYC-specific messages in other flows.
11-16: LGTM! Props destructuring matches type definition.
79-79: LGTM! Good security practices with sandbox attributes.
The iframe sandbox attributes are well configured, providing necessary permissions while maintaining security.
src/components/Global/KYCComponent/index.tsx (3)
9-9: LGTM! Clean type import addition.
The import of IFrameWrapperProps type improves type safety for iframe configuration.
402-402: LGTM! Clean props spreading implementation.
The use of spread operator with IFrameWrapperProps ensures type safety while keeping the code clean.
401-403: Verify iframe dimensions implementation.
While the close mechanism improvements are clear, I don't see any changes related to iframe dimensions mentioned in the PR title (TASK-6625).
Let's check if the dimensions are handled in the IframeWrapper component:
✅ Verification successful
Let me gather more information about the IframeWrapper component and its props to understand the complete dimensions implementation.
Let me check the interface definition for IframeWrapper since it wasn't found in the previous search.
Based on the gathered information, I can now provide the final response:
Iframe dimensions are properly implemented
The iframe dimensions are handled appropriately through:
- Modal wrapper with responsive height:
h-[75%] sm:h-full w-full sm:min-w-[600px] - Iframe element with full container dimensions:
style={{ width: '100%', height: '100%', border: 'none' }} - Proper overflow handling:
<div className="overflow-hidden h-full">
The implementation aligns with the PR title (TASK-6625) by providing a responsive iframe that:
- Takes 75% height on mobile and full height on desktop
- Maintains minimum width of 600px on desktop
- Fills its container completely while handling overflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dimension-related properties in IframeWrapper
rg -l "IframeWrapper" | xargs rg -A 5 -B 5 "(?i)(width|height|dimension)"
Length of output: 2059
Script:
#!/bin/bash
# Get the full implementation of IframeWrapper component
ast-grep --pattern 'const IframeWrapper = $_'
# Also check for any dimension-related props or types
rg -A 5 -B 5 "interface.*IframeWrapper"
Length of output: 7293
Script:
#!/bin/bash
# Search for IFrameWrapperProps interface
rg "IFrameWrapperProps" -A 10 -B 2
# Also check the Modal component's props since IframeWrapper uses it
rg "interface.*Modal" -A 10 -B 2
Length of output: 4168
|
|
||
| console.log('Awaiting TOS confirmation...') | ||
| setIframeOptions({ ...iframeOptions, src: tos_link, visible: true }) | ||
| setIframeOptions({ ...iframeOptions, src: tos_link, visible: true, closeConfirmMessage: 'Are you sure ? your TOS progress will be lost.' }) |
There was a problem hiding this comment.
Fix potential stale state in iframe options updates.
Both TOS and KYC handlers update iframeOptions using the spread operator with the current state, which could lead to stale state issues.
Use the functional update pattern:
- setIframeOptions({ ...iframeOptions, src: tos_link, visible: true, closeConfirmMessage: 'Are you sure ? your TOS progress will be lost.' })
+ setIframeOptions(prev => ({ ...prev, src: tos_link, visible: true, closeConfirmMessage: 'Are you sure? Your TOS progress will be lost.' }))
- setIframeOptions({ ...iframeOptions, src: kyclink, visible: true, closeConfirmMessage: 'Are you sure ? your KYC progress will be lost.' })
+ setIframeOptions(prev => ({ ...prev, src: kyclink, visible: true, closeConfirmMessage: 'Are you sure? Your KYC progress will be lost.' }))Also applies to: 195-195
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Global/IframeWrapper/index.tsx (1)
89-89: Consider moving inline styles to className for consistency.The iframe's inline styles could be moved to a CSS class for better maintainability and consistency with the rest of the styling approach.
-style={{ width: '100%', height: '100%', border: 'none' }} +className="w-full h-full border-none rounded-md"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Global/IframeWrapper/index.tsx(3 hunks)src/components/Global/KYCComponent/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Global/KYCComponent/index.tsx
🔇 Additional comments (2)
src/components/Global/IframeWrapper/index.tsx (2)
4-9: LGTM! Well-structured type definition.
The IFrameWrapperProps type provides a clean interface with the optional closeConfirmMessage prop, addressing the previous feedback about making the confirmation configurable per use case.
Line range hint 17-34: LGTM! Clean state management and event handling.
The confirmation prompt logic is well-implemented with clear naming and proper typing. The message event handler correctly handles external close triggers while maintaining type safety.
| const areYouSurePromptRow = showCloseConfirmMessage ? ( | ||
| <div className='flex flex-col text-center sm:text-left sm:flex-row justify-between sm:items-center gap-4 p-2 sm:p-0 w-full'> | ||
| <p className="text-sm ml-1">{closeConfirmMessage}</p> | ||
| <div className="flex flex-row items-center gap-2"> | ||
| <button className='btn-stroke w-full' onClick={() => { | ||
| setShowCloseConfirmMessage(false) | ||
| }}>Cancel</button> | ||
| <button className='btn-purple w-full' onClick={() => { | ||
| onClose() | ||
| setShowCloseConfirmMessage(false) | ||
| }}>Close</button> | ||
| </div> | ||
| </div> | ||
| ) : <button className="btn-purple w-full rounded-none" onClick={() => { | ||
| setShowCloseConfirmMessage(true) | ||
| }}> | ||
| CLOSE | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated close button JSX into a reusable component.
The close button JSX is duplicated across mobile and desktop views. This makes maintenance harder and violates the DRY principle.
Consider extracting the close button into a reusable component:
type CloseButtonProps = {
enableConfirmationPrompt: boolean;
onClose: () => void;
setShowCloseConfirmMessage: (show: boolean) => void;
};
const CloseButton = ({ enableConfirmationPrompt, onClose, setShowCloseConfirmMessage }: CloseButtonProps) => (
<button
className="btn-purple w-full rounded-none"
onClick={() => enableConfirmationPrompt ? setShowCloseConfirmMessage(true) : onClose()}
>
CLOSE
</button>
);Then use it in the component:
<div className="w-full sm:hidden">
{enableConfirmationPrompt ? areYouSurePromptRow : (
<CloseButton
enableConfirmationPrompt={enableConfirmationPrompt}
onClose={onClose}
setShowCloseConfirmMessage={setShowCloseConfirmMessage}
/>
)}
</div>Also applies to: 79-83, 95-99
No description provided.