feat(qr-modal): add preventIframe option + improve fallback behaviour #62
Conversation
Added a new option 'preventScreenshot' to ModalOptions to control screenshot prevention behavior.
Added optional preventIframe option to QR modal settings.
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Modal as QRCodeModal
participant DOM
participant QR as QRGenerator
Caller->>Modal: show(requestUrl)
Modal->>DOM: Close existing modal (if any)
alt preventIframe enabled
Modal->>Modal: Check "in iframe" (may throw SecurityError)
alt inside iframe or error
Modal->>Modal: Log info
Modal->>Caller: invoke onClose (optional)
Modal-->>Caller: return early
end
end
Modal->>DOM: Insert modal HTML
Modal->>QR: Generate QR into modal
QR-->>DOM: Render QR
Modal->>DOM: Add event listeners
Modal->>Modal: Start auto-close timer
Modal-->>Caller: resolve show()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/types.ts (1)
62-72: ModalOptions is missingpreventIframeand exposes unusedpreventScreenshotThe runtime code (
src/utils/modalUtils.ts) and README both rely on apreventIframemodal option, butModalOptionsonly declarespreventScreenshot?: boolean. Accessingthis.options.preventIframeonModalOptionsis a TypeScript error, and integrators copying the README snippet will see a type error.You should at minimum add
preventIframe?: booleantoModalOptions, and consider whetherpreventScreenshotshould stay (it’s currently unused and not documented here).For example:
export type ModalOptions = { title?: string; description?: string; extensionUrl?: string; darkTheme?: boolean; modalPopupTimer?: number; showExtensionInstallButton?: boolean; onClose?: () => void; - // NEW OPTION - preventScreenshot?: boolean; // when true: add blur + disable copy/screenshot hints + // Prevent opening the QR modal when running inside an iframe + preventIframe?: boolean; + + // (Optional / future) when true: add blur + disable copy/screenshot hints + preventScreenshot?: boolean; };
♻️ Duplicate comments (1)
README.md (1)
258-267: Docs usepreventIframethat isn’t currently inModalOptionsThis example correctly shows the intended
preventIframeusage, butModalOptionsinsrc/utils/types.tsdoesn’t yet declarepreventIframe(onlypreventScreenshot). Until the type is updated, TypeScript consumers will get errors when they copy this snippet.Once you add
preventIframe?: booleantoModalOptions(see comment insrc/utils/types.ts), the README and types will be in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/utils/modalUtils.ts(2 hunks)src/utils/types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/modalUtils.ts (1)
src/utils/logger.ts (1)
error(46-48)
Added options to prevent screenshots and iframes in modal.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/modalUtils.ts (1)
49-49: Potential duplicateonCloseinvocation when reopening modal.Calling
this.close()here triggers theonClosecallback (lines 88-90) for any existing modal before opening a new one. Ifshow()is called multiple times in succession,onClosewill fire for each previous modal removal, which may not match user expectations—they might expectonCloseonly for user-initiated closes.Consider one of these approaches:
Add an optional parameter to
close()to skip the callback during re-opening:close(skipCallback: boolean = false): void { // ... timer cleanup ... const modal = document.getElementById(this.modalId); if (modal) { modal.remove(); } if (!skipCallback && this.options.onClose) { this.options.onClose(); } }Then use
this.close(true)at line 49.Extract DOM-only cleanup into a separate private method and use that instead of
this.close().
🧹 Nitpick comments (3)
src/utils/types.ts (1)
70-73: Add inline documentation forpreventIframeand remove temporary comment.The addition of
preventIframeresolves the type concern from the previous review. However, there's a documentation inconsistency:
preventScreenshothas an inline comment explaining its behaviorpreventIframelacks any documentationFor consistency and maintainability, please add an inline comment for
preventIframeexplaining when/why the modal is blocked, and remove the "// NEW OPTION" scaffolding comment.- // NEW OPTION - preventScreenshot?: boolean; // when true: add blur + disable copy/screenshot hints - preventIframe?: boolean; + preventScreenshot?: boolean; // when true: add blur + disable copy/screenshot hints + preventIframe?: boolean; // when true: block modal rendering inside iframessrc/utils/modalUtils.ts (2)
30-46: Iframe prevention logic is solid; consider log level adjustment.The iframe detection correctly handles both the standard check (
window.self !== window.top) and cross-origin security errors. The addition ofpreventIframetoModalOptionsresolves the type concern from the previous review.However, for better log level semantics, consider using a warning-level method instead of
logger.infofor these blocking scenarios, since they represent prevented operations rather than normal informational events:if (window.self !== window.top) { - logger.info( + logger.warn?.( "Reclaim Modal blocked: preventIframe = true and page is inside an iframe." - ); + ) ?? logger.info("Reclaim Modal blocked: preventIframe = true and page is inside an iframe."); if (this.options.onClose) this.options.onClose(); return; } } catch { - logger.info( + logger.warn?.( "Reclaim Modal blocked: preventIframe = true and iframe check threw a security error." - ); + ) ?? logger.info("Reclaim Modal blocked: preventIframe = true and iframe check threw a security error."); if (this.options.onClose) this.options.onClose(); return; }(Only apply if
logger.warnis available in your logger module.)
66-69: Use error-level logging for exceptions.The try-catch wrapper is good for robustness, but
logger.infois not the appropriate log level for error scenarios. If your logger module supports it, use an error-level method:} catch (error) { - logger.info('Error showing QR code modal:', error); + logger.error?.('Error showing QR code modal:', error) ?? logger.info('Error showing QR code modal:', error); throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/modalUtils.ts(2 hunks)src/utils/types.ts(2 hunks)
⏰ 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: Cursor Bugbot
Removed the preventScreenshot option from the types definition.
| ); | ||
| if (this.options.onClose) this.options.onClose(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: onClose callback invoked when modal never opened
When preventIframe blocks the modal from opening, the onClose callback is invoked even though the modal was never actually opened. The onClose callback is documented as "Callback when modal is closed" and should only fire when an opened modal is being closed, not when the modal is prevented from opening. This creates semantic inconsistency and will cause false positives for developers tracking modal lifecycle events for analytics, cleanup, or state management purposes.
There was a problem hiding this comment.
Fixed the type definition issues and modal cleanup. All checks are passing. Ready for review!
Removed redundant modal close call before creating a new modal.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/modalUtils.ts (1)
32-48: RemoveonCloseinvocations when modal is blocked from opening.The
onClosecallback at lines 38 and 45 should not be invoked when the modal is prevented from opening due to iframe detection. The callback is documented as "Callback when modal is closed" and should only fire when an opened modal is being closed, not when the modal is blocked from opening in the first place.This creates semantic inconsistency and false positives for consumers tracking modal lifecycle events for analytics, cleanup, or state management. Additionally, since
this.close()is called at line 29, if a modal was previously open,onClosewill have already been invoked—resulting in a double invocation when the iframe check subsequently blocks.Apply this diff to remove the incorrect
onClosecalls:try { if (window.self !== window.top) { logger.info( "Reclaim Modal blocked: preventIframe = true and page is inside an iframe." ); - if (this.options.onClose) this.options.onClose(); return; } } catch { logger.info( "Reclaim Modal blocked: preventIframe = true and iframe check threw a security error." ); - if (this.options.onClose) this.options.onClose(); return; }If you need to notify consumers when the modal is blocked (as opposed to closed), consider introducing a separate callback like
onBlockedinModalOptions.
🧹 Nitpick comments (1)
src/utils/modalUtils.ts (1)
26-69: Well-structured modal lifecycle flow.The restructuring of the
show()method is solid:
- Closing any existing modal first (line 29) prevents stale DOM and timer leaks
- The iframe prevention check (lines 32-48) gates expensive operations before they occur
- The modal creation sequence (lines 50-63) follows a logical order: create HTML → insert DOM → generate QR → register listeners → start timer
- The try-catch wrapper provides appropriate error handling
Optional optimization: Consider moving the iframe check before
this.close()at line 29. This avoids unnecessarily closing an existing modal when the new one will be blocked anyway:async show(requestUrl: string): Promise<void> { try { - // Always close previous modal FIRST - this.close(); - // IFRAME CHECK if (this.options.preventIframe) { try { if (window.self !== window.top) { logger.info( "Reclaim Modal blocked: preventIframe = true and page is inside an iframe." ); return; } } catch { logger.info( "Reclaim Modal blocked: preventIframe = true and iframe check threw a security error." ); return; } } + + // Always close previous modal before opening new one + this.close(); // Create modal HTML const modalHTML = this.createModalHTML();This ensures that if the modal is blocked, the existing modal remains open rather than being closed and not replaced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/modalUtils.ts(2 hunks)
⏰ 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: Cursor Bugbot
2 participants
Description
Adds support for a new
preventIframemodal option that disables opening the QR modal when the SDK is running inside an iframe.This improves SDK behavior for embedded apps, dashboards, widgets, and hosted integrations.
Changes included:
preventIframetoModalOptionstypemodalUtils.tsTesting (ignore for documentation update)
preventIframe: true, the modal no longer rendersType of change
Checklist:
Additional Notes:
This enhancement ensures developers embedding the SDK inside external platforms have better control over modal behavior in restricted iframe environments.
Note
Adds a
preventIframemodal option and iframe detection to block the QR modal inside iframes, plus safer modal lifecycle; updates README with usage.preventIframetoModalOptionsinsrc/utils/types.ts.QRCodeModal.show()(src/utils/modalUtils.ts):preventIframeis true, log, invokeonClose, and skip rendering.README.mdwith a newpreventIframesection and usage example.Written by Cursor Bugbot for commit 74902a0. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.