Attempt storybook setup in onlook containers #3042
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
WalkthroughAdds a Storybook preview preload script tag and implements a helper to detect the correct parent window for Onlook messenger initialization across direct, Next.js and nested iframe scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Iframe as App / Iframe
participant Finder as findOnlookParent()
participant Messenger as WindowMessenger
Iframe->>Finder: determine parent window
alt direct iframe (parent === expected)
Finder-->>Iframe: return window.parent
else nested iframe (top !== parent)
Finder-->>Iframe: log diagnostic, return window.top
else not in iframe
Finder-->>Iframe: log diagnostic, return window.parent (fallback)
end
Iframe->>Messenger: initialize with resolved remoteWindow
Messenger-->Iframe: establish message connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
🧹 Nitpick comments (1)
apps/web/preload/script/index.ts (1)
47-48: Consider implementing proper origin validation.The TODO comment correctly identifies that using
allowedOrigins: ['*']is a security concern. In production, this should be restricted to trusted origins to prevent unauthorized cross-origin communication.Consider defining allowed origins based on the environment:
const getAllowedOrigins = (): string[] => { if (process.env.NODE_ENV === 'development') { return ['http://localhost:*', 'http://127.0.0.1:*']; } // Add production domains return ['https://yourdomain.com']; }; const messenger = new WindowMessenger({ remoteWindow: findOnlookParent(), allowedOrigins: getAllowedOrigins(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/.storybook/preview-head.html(1 hunks)apps/web/preload/script/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/preload/script/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/preload/script/index.ts
⏰ 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: Run Chromatic
🔇 Additional comments (2)
apps/web/preload/script/index.ts (1)
46-46: Good approach to use intelligent parent detection.Replacing the hardcoded
window.parentwithfindOnlookParent()is the right approach for handling different iframe embedding scenarios. However, ensure the logic flaw infindOnlookParent()is fixed first (see previous comment).apps/web/client/.storybook/preview-head.html (1)
1-1: No action required—the script path is correctly configured.The file exists at
apps/web/client/public/onlook-preload-script.jsand is properly served by Storybook through the configuredstaticDirs: ["../public"]setting in.storybook/main.ts. The path/onlook-preload-script.jswill resolve correctly in both development and production builds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/preload/script/index.ts (1)
9-35: Logic flaw from previous review has been fixed.The previous critical issue regarding the condition that prevented nested iframe scenarios from working has been correctly resolved. The current implementation properly checks
window.parent === window.top(line 22) for direct iframes before falling back towindow.top(line 28) for nested iframes.Minor observations:
Lines 15-18: When
window === window.top, returningwindow.parentis equivalent to returningwindowitself. Consider updating the message to be more explicit:"Not in an iframe, using window as fallback"and returnwindowdirectly for clarity.Line 28: The check
if (window.top)is always truthy in browsers and appears redundant. This may be present for TypeScript strict null checking, but could be removed if not needed.Optional refactor for clarity:
const findOnlookParent = (): Window => { // If we're not in an iframe, something is wrong if (window === window.top) { - console.warn(`${PENPAL_CHILD_CHANNEL} - Not in an iframe, using window.parent as fallback`); - return window.parent; + console.warn(`${PENPAL_CHILD_CHANNEL} - Not in an iframe, using window as fallback`); + return window; } // Check if we're in a direct iframe (parent is the top window) // This is the Next.js case: Onlook -> Next.js iframe if (window.parent === window.top) { return window.parent; } // We're in a nested iframe (parent is NOT the top window) // This is the Storybook case: Onlook -> CodeSandbox -> Storybook preview iframe - if (window.top) { - console.log(`${PENPAL_CHILD_CHANNEL} - Using window.top for nested iframe scenario`); - return window.top; - } + console.log(`${PENPAL_CHILD_CHANNEL} - Using window.top for nested iframe scenario`); + return window.top!; - // Final fallback - return window.parent; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/preload/script/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/preload/script/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/preload/script/index.ts
⏰ 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: Run Chromatic
🔇 Additional comments (1)
apps/web/preload/script/index.ts (1)
46-46: Correct integration of the parent window detection.The change from hardcoded
window.parenttofindOnlookParent()properly enables dynamic parent window detection for both direct iframe (Next.js) and nested iframe (Storybook) scenarios.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Enhances Storybook integration by adding
findOnlookParent()for iframe handling and preloading Onlook scripts in Storybook.findOnlookParent()inindex.tsto handle both direct and nested iframe scenarios for Onlook connections.createMessageConnection()inindex.tsto usefindOnlookParent()for settingremoteWindow.<script>tag inpreview-head.htmlto preload Onlook scripts in Storybook.This description was created by
for 31daefc. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit