Fix preview navigation black screen when selecting routes from dropdown#2610
Conversation
…wn (dyad-sh#2428) Changed navigateToRoute() to use postMessage + location.replace() pattern instead of direct location.href assignment. This matches the approach used by back/forward navigation buttons and provides smooth navigation without the black screen flicker and sidebar state reset reported by users. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @wwwillchen-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses and resolves a significant user experience issue where navigating between routes in the preview panel via the dropdown would cause a disruptive black screen flicker and reset the sidebar state. By refactoring the navigation mechanism to use a more robust Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
Greptile OverviewGreptile SummaryChanged route dropdown navigation from direct Key changes:
The postMessage target origin issue flagged in the previous thread (using Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant PreviewIframe as PreviewIframe.tsx
participant Iframe as Preview Iframe
participant Shim as dyad-shim.js
Note over User,Shim: Route Dropdown Navigation Flow
User->>PreviewIframe: Select route from dropdown
PreviewIframe->>PreviewIframe: navigateToRoute(path)
PreviewIframe->>PreviewIframe: Build full URL from appUrl + path
PreviewIframe->>Iframe: postMessage({type: "navigate", url})
Iframe->>Shim: Receive navigate message
Shim->>Shim: Validate URL (http/https only)
Shim->>Iframe: location.replace(url)
Iframe-->>Shim: Navigation complete
Shim->>PreviewIframe: postMessage({type: "pushState/replaceState"})
PreviewIframe->>PreviewIframe: Update navigation history
PreviewIframe->>PreviewIframe: Update currentIframeUrlRef
PreviewIframe->>PreviewIframe: Update preservedUrls for HMR
Note over PreviewIframe: useMemo keeps iframe src stable<br/>during React re-renders
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a UI flicker during preview navigation by replacing "location.href" with a "postMessage"-based approach, which is a great improvement for the user experience and correctly handles preserving the URL state for HMR consistency. However, this implementation introduces a medium-severity security vulnerability by not specifying a target origin for "postMessage". This should be remediated by replacing the wildcard "*" with the specific origin of the application URL to prevent potential information disclosure.
🔍 Dyadbot Code Review SummaryFound 2 new issue(s) flagged by 3 independent reviewers (consensus voting: 2+ agents must agree, at least one rating MEDIUM+). Summary
Issues to Address
🟢 Low Priority Issues (3 items, below consensus threshold)
See inline comments for details. Generated by Dyadbot code review (3 independent agents with consensus voting) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/preview_panel/PreviewIframe.tsx">
<violation number="1" location="src/components/preview_panel/PreviewIframe.tsx:946">
P2: Using `"*"` as the `postMessage` target origin is a security risk - it allows any page currently loaded in the iframe to receive the navigation command and URL. Since `baseUrl` is already calculated from `appUrl.origin` on line 936, use it as the target origin to restrict message delivery to the intended recipient only.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
✅ Claude Code completed successfullySummary
Review Comments Addressed
CI Status
DetailsReview threads resolved:
Changes made in
|
github-actions
Bot
added
cc:done
needs-human:review-issue
|
@BugBot run |
|
@BugBot run |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const iframeSrc = useMemo(() => { | ||
| if (!appUrl) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const currentUrl = currentIframeUrlRef.current; | ||
| if (!currentUrl) { | ||
| return appUrl; | ||
| } | ||
|
|
||
| try { | ||
| const currentOrigin = new URL(currentUrl).origin; | ||
| const appOrigin = new URL(appUrl).origin; | ||
| return currentOrigin === appOrigin ? currentUrl : appUrl; | ||
| } catch { | ||
| return appUrl; | ||
| } | ||
| }, [appUrl, reloadKey, selectedAppId]); |
There was a problem hiding this comment.
🟡 useMemo reads stale ref on app switch when origins match, producing wrong iframe src
The iframeSrc useMemo reads currentIframeUrlRef.current during render, but the ref is only reset to the new app's base URL in a useEffect (which runs after render). When selectedAppId changes, the useMemo recomputes with the old ref value. If the old and new apps share the same origin (e.g., both on localhost:3000), the origin check passes and the memo returns the old app's route URL instead of the new app's base URL. Subsequent renders don't fix this because the useMemo deps (appUrl, reloadKey, selectedAppId) don't change again.
Root Cause and Impact
Sequence when switching apps with the same origin:
selectedAppIdandappUrlchange →useMemorecomputes.currentIframeUrlRef.currentstill holds the previous app's URL (e.g.http://localhost:3000/about).- Origin check at line 994-996 sees matching origins → returns the stale URL.
- The
useEffectatPreviewIframe.tsx:725-735runs after render and setscurrentIframeUrlRef.current = appUrl, but this doesn't trigger auseMemorecomputation because none of its deps changed. - The iframe
srcis set to the old app's route URL for the lifetime of the component (until a reload or anotherappUrlchange).
Impact: When two apps happen to share the same origin (e.g., one app stops and another starts on the same port), the iframe would load the wrong route from the previous app. In practice this is an uncommon edge case because Dyad typically assigns different ports to different apps.
Prompt for agents
The useMemo for iframeSrc reads currentIframeUrlRef.current, but this ref is only updated in a useEffect that runs after render. When selectedAppId changes and the old/new apps share the same origin, the memo returns a stale URL. To fix this, either: (1) Reset currentIframeUrlRef.current synchronously during render (before the useMemo) when selectedAppId or appUrl changes, using a pattern like checking prevAppUrlRef inline. For example, add a synchronous check before the useMemo: if (appUrl && appUrl !== prevAppUrlRef.current) { currentIframeUrlRef.current = appUrl; prevAppUrlRef.current = appUrl; } Or (2) Add a separate state variable (e.g. iframeSrcVersion) that gets incremented in the useEffect alongside the ref reset, and include it in the useMemo dependency array to force recomputation.
Was this helpful? React with 👍 or 👎 to provide feedback.
🔍 Dyadbot Code Review SummaryFound 3 consensus issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
Already Commented (1 issue)
🟢 Low Priority Issues (4 items, no consensus)
See inline comments for details on MEDIUM issues. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
| const baseUrl = new URL(appUrl).origin; | ||
| const newUrl = `${baseUrl}${path}`; |
There was a problem hiding this comment.
🟡 MEDIUM — Unguarded new URL(appUrl) can throw
3/3 reviewers flagged this (2 MEDIUM, 1 LOW)
new URL(appUrl).origin on line 936 will throw a TypeError if appUrl is malformed. Every other navigation handler in this component (handleNavigateBack, handleNavigateForward, handleReload, pushState/replaceState handlers) wraps URL parsing in a try/catch. This is the only handler that doesn't, making it an inconsistency and a potential crash site during edge cases like app startup transitions.
| const baseUrl = new URL(appUrl).origin; | |
| const newUrl = `${baseUrl}${path}`; | |
| try { | |
| const baseUrl = new URL(appUrl).origin; | |
| const newUrl = `${baseUrl}${path}`; |
The closing } catch { console.error('Invalid appUrl during route navigation'); return; } should wrap the rest of the function body before the final } of the if block.
| const iframeSrc = useMemo(() => { | ||
| if (!appUrl) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const currentUrl = currentIframeUrlRef.current; | ||
| if (!currentUrl) { | ||
| return appUrl; | ||
| } | ||
|
|
||
| try { | ||
| const currentOrigin = new URL(currentUrl).origin; | ||
| const appOrigin = new URL(appUrl).origin; | ||
| return currentOrigin === appOrigin ? currentUrl : appUrl; | ||
| } catch { | ||
| return appUrl; | ||
| } | ||
| }, [appUrl, reloadKey, selectedAppId]); |
There was a problem hiding this comment.
🟡 MEDIUM — useMemo reads a mutable ref but deps cannot track ref mutations
3/3 reviewers flagged this (3 MEDIUM)
This useMemo reads currentIframeUrlRef.current (line 988), but refs are not reactive — changes to the ref do NOT trigger re-computation of the memo. The dependency array [appUrl, reloadKey, selectedAppId] relies on the assumption that one of these values always changes whenever the ref needs to be re-read.
After navigateToRoute or back/forward handlers update the ref, none of these deps change, so the memo returns a stale value. This appears to be intentional (the comment says "freeze iframe src"), and it's the actual mechanism that prevents the black screen flicker. However, the coupling is fragile and undocumented — a future maintainer could easily break this by adding a code path that updates the ref expecting the memo to recompute, or by using iframeSrc where they actually need the current URL.
Suggestion: Add a comment directly on the dependency array documenting this intentional design:
| const iframeSrc = useMemo(() => { | |
| if (!appUrl) { | |
| return undefined; | |
| } | |
| const currentUrl = currentIframeUrlRef.current; | |
| if (!currentUrl) { | |
| return appUrl; | |
| } | |
| try { | |
| const currentOrigin = new URL(currentUrl).origin; | |
| const appOrigin = new URL(appUrl).origin; | |
| return currentOrigin === appOrigin ? currentUrl : appUrl; | |
| } catch { | |
| return appUrl; | |
| } | |
| }, [appUrl, reloadKey, selectedAppId]); | |
| // Freeze iframe src between remounts so in-iframe SPA navigation (pushState/replaceState) | |
| // doesn't cause React to set a new src and trigger a second full navigation flicker. | |
| const iframeSrc = useMemo(() => { | |
| if (!appUrl) { | |
| return undefined; | |
| } | |
| const currentUrl = currentIframeUrlRef.current; | |
| if (!currentUrl) { | |
| return appUrl; | |
| } | |
| try { | |
| const currentOrigin = new URL(currentUrl).origin; | |
| const appOrigin = new URL(appUrl).origin; | |
| return currentOrigin === appOrigin ? currentUrl : appUrl; | |
| } catch { | |
| return appUrl; | |
| } | |
| // NOTE: currentIframeUrlRef.current is intentionally read as a side-channel | |
| // and is NOT in the dependency array. This memo only re-evaluates when appUrl, | |
| // reloadKey, or selectedAppId change. Navigation handlers (navigateToRoute, | |
| // handleNavigateBack, etc.) update the ref synchronously but do NOT trigger | |
| // memo recomputation — this is what prevents the iframe src from being reset | |
| // during SPA navigation, which would cause a visible black screen flicker. | |
| // Do NOT use iframeSrc to read the current iframe URL; use currentIframeUrlRef.current instead. | |
| }, [appUrl, reloadKey, selectedAppId]); |
🎭 Playwright Test Results
|
| OS | Passed | Failed | Flaky | Skipped |
|---|---|---|---|---|
| 🍎 macOS | 224 | 3 | 9 | 6 |
Summary: 224 passed, 3 failed, 9 flaky, 6 skipped
Failed Tests
🍎 macOS
security_review.spec.ts > security review - edit and use knowledge- Error: expect(string).toMatchSnapshot(expected) failed
select_component.spec.ts > select component next.js- Error: expect(locator).toBeVisible() failed
template-create-nextjs.spec.ts > create next.js app- Error: expect(locator).toBeVisible() failed
📋 Re-run Failing Tests (macOS)
Copy and paste to re-run all failing spec files locally:
npm run e2e \
e2e-tests/security_review.spec.ts \
e2e-tests/select_component.spec.ts \
e2e-tests/template-create-nextjs.spec.ts⚠️ Flaky Tests
🍎 macOS
chat_history.spec.ts > should open, navigate, and select from history menu(passed after 1 retry)debugging_logs.spec.ts > console logs should appear in the console(passed after 1 retry)local_agent_ask.spec.ts > local-agent ask mode(passed after 1 retry)local_agent_basic.spec.ts > local-agent - dump request(passed after 1 retry)new_chat.spec.ts > new chat (second button)(passed after 1 retry)refresh.spec.ts > preview navigation - forward and back buttons work(passed after 1 retry)setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed(passed after 1 retry)setup.spec.ts > setup ai provider(passed after 1 retry)thinking_budget.spec.ts > thinking budget(passed after 1 retry)
…wn (dyad-sh#2610) ## Summary - Changed `navigateToRoute()` to use `postMessage` + `location.replace()` pattern instead of direct `location.href` assignment - This matches the approach used by back/forward navigation buttons and provides smooth navigation without the black screen flicker and sidebar state reset - Also added proper `currentIframeUrlRef` and `preservedUrls` updates for HMR remount consistency Fixes dyad-sh#2428 ## Test plan - [x] Build succeeds - [x] Unit tests pass (784 tests) - [x] E2E tests pass for preview navigation (`npm run e2e -- --grep "preview navigation"`) - Manual testing: Open a multi-page app, use the route dropdown to navigate between routes - should no longer see black screen flicker or sidebar collapse 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/2610" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes dyad-sh#2428: removes black screen flicker and sidebar collapse when selecting routes from the preview dropdown. Navigation now uses postMessage + location.replace and keeps the iframe src stable across SPA navigation and HMR. - **Bug Fixes** - Use postMessage + location.replace to navigate; prevents flicker and preserves sidebar state. - Freeze iframe src with useMemo and same-origin check so SPA nav/HMR don’t reset it; added e2e test to verify src remains unchanged. - Sync navigation state: history, canGoBack/canGoForward, currentIframeUrlRef, and per-app preservedUrls (clears on root). <sup>Written for commit 1bdd763. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> --------- Co-authored-by: Will Chen <willchen90@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Summary
navigateToRoute()to usepostMessage+location.replace()pattern instead of directlocation.hrefassignmentcurrentIframeUrlRefandpreservedUrlsupdates for HMR remount consistencyFixes #2428
Test plan
npm run e2e -- --grep "preview navigation")🤖 Generated with Claude Code
Summary by cubic
Fixes #2428: removes black screen flicker and sidebar collapse when selecting routes from the preview dropdown. Navigation now uses postMessage + location.replace and keeps the iframe src stable across SPA navigation and HMR.
Written for commit 1bdd763. Summary will update on new commits.