feat: added iframe detection and auto resolving for exit action #2867
Conversation
|
WalkthroughA new utility function Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@chesterkmr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 3
🧹 Outside diff range and nitpick comments (4)
apps/kyb-app/src/common/utils/is-iframe.ts (1)
1-17: Consider caching the result for better performance.Since the iframe status won't change during runtime, consider memoizing the result to avoid redundant checks.
+let isIframeCache: boolean | null = null; + export const isIframe = (): boolean => { + if (isIframeCache !== null) { + return isIframeCache; + } + try { // Check if window.self and window.top are the same reference const isFramed = window.self !== window.top; // Additional security check - try to access parent // This will throw an error if cross-origin if (isFramed) { window.parent.location.origin; } - return isFramed; + isIframeCache = isFramed; + return isIframeCache; } catch (e: unknown) { // If we get a security error, we're definitely in a cross-origin iframe - return true; + isIframeCache = true; + return isIframeCache; } };apps/kyb-app/src/hooks/useAppExit/useAppExit.ts (3)
Line range hint
18-29: Add error handling for trackEvent callWhile the logic for handling iframe and exit actions is correct, consider adding error handling for the tracking event to ensure robustness.
if (kybOnExitAction === 'send-event' || isIframe()) { - trackEvent(CollectionFlowEvents.USER_EXITED); + try { + trackEvent(CollectionFlowEvents.USER_EXITED); + } catch (error) { + console.error('Failed to track exit event:', error); + } return; }
33-33: Improve readability of isExitAvailable logicThe current ternary expression could be made more readable by splitting it into separate conditions.
- isExitAvailable: kybOnExitAction === 'send-event' || isIframe() ? true : !!customer?.websiteUrl, + isExitAvailable: (() => { + if (kybOnExitAction === 'send-event' || isIframe()) { + return true; + } + return !!customer?.websiteUrl; + })(),
Line range hint
9-36: Consider splitting the hook for better separation of concernsThe
useAppExithook currently handles multiple responsibilities: iframe detection, exit action handling, and customer portal redirection. Consider splitting this into smaller, more focused hooks:
useIframeDetectionuseExitActionuseCustomerRedirectThis would improve maintainability and make the code more testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/kyb-app/src/common/utils/is-iframe.ts(1 hunks)apps/kyb-app/src/hooks/useAppExit/useAppExit.ts(3 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (3)
apps/kyb-app/src/common/utils/is-iframe.ts (2)
1-17: LGTM! The implementation correctly handles iframe detection and cross-origin security.
The function properly checks for iframes using window references and includes appropriate error handling for cross-origin scenarios.
1-17: Verify browser compatibility of the window APIs.
The implementation uses standard window APIs. Let's verify their browser support to ensure compatibility across different browsers and versions.
✅ Verification successful
Browser compatibility is verified and supported
The implementation uses well-supported window APIs (window.self, window.top, window.parent) that are compatible with modern browsers. This is confirmed by:
- The project's browserslist configuration which targets modern browsers (
>0.2%,not dead,not IE 11) - The consistent usage of these APIs across the codebase without any special polyfills or fallbacks
- The implementation follows standard cross-origin security patterns with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any browser compatibility issues or workarounds in the codebase
# Look for similar window.self, window.top, and window.parent usage patterns
# Search for similar iframe detection patterns
rg -A 2 "window\.(self|top|parent)" --type ts --type js
# Search for any browser compatibility comments or issues
rg -i "browser.*(compatibility|support).*iframe" --type ts --type js
Length of output: 2092
Script:
#!/bin/bash
# Let's check for any polyfills, fallbacks, or browser-specific code related to window APIs
rg -i "(polyfill|fallback|browser|compatibility).*(window|iframe)" --type ts --type js
# Check package.json for any browser compatibility configurations or polyfills
fd "package.json" --exec cat {} \; | jq -r '.browserslist, .targets, .engines'
# Look for any browser-specific tests or test cases
rg -i "test.*(window|iframe)" --type ts --type js
Length of output: 798
apps/kyb-app/src/hooks/useAppExit/useAppExit.ts (1)
1-1: Verify the implementation of the imported iframe detection utility
Let's ensure the imported isIframe utility is implemented correctly and handles edge cases.
✅ Verification successful
Implementation of isIframe utility is robust and secure
The isIframe utility is well-implemented with proper security considerations:
- Correctly checks window references (
window.self !== window.top) - Handles cross-origin scenarios with try-catch block
- Includes additional security verification by attempting to access parent origin
- Returns appropriate boolean values for both same-origin and cross-origin cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of isIframe utility
# Expected: The utility should handle cross-origin access and various window contexts
# Search for the implementation
ast-grep --pattern 'export const isIframe = $_'
Length of output: 17370
chesterkmr
force-pushed
the
illiar/feat/kyb-iframe-detection
branch
from
7553719 to
c7368b0
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Chores