Fix preview iframe URL error & create debugging skill#2887
Fix preview iframe URL error & create debugging skill#2887wwwillchen merged 2 commits intodyad-sh:mainfrom
Conversation
|
@BugBot run |
Summary of ChangesHello, 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 significantly enhances the developer experience by introducing a powerful new debugging skill for minified production errors and by fortifying the URL handling within the preview iframe. The debugging skill provides a structured approach to de-minify stack traces, while the iframe improvements prevent common URL-related errors and ensure consistent navigation history, ultimately leading to more reliable development and debugging workflows. 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
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["iframe message received\n(pushState / replaceState)"] --> B["Extract payload.newUrl"]
B --> C{newUrl truthy?}
C -- No --> Z["resolvedUrl = undefined\n(no history update)"]
C -- Yes --> D["Attempt new URL(newUrl, appUrl)"]
D -- Success --> E["resolvedUrl = absolute URL"]
D -- Throws --> F["resolvedUrl = raw newUrl\n(fallback, keep original)"]
E --> G{pushState or replaceState?}
F --> G
G -- pushState --> H["Append resolvedUrl to navigationHistory\nUpdate currentHistoryPosition\nUpdate currentIframeUrlRef"]
G -- replaceState --> I["Replace current entry in navigationHistory\nUpdate currentIframeUrlRef"]
H --> J{selectedAppId && appUrl?}
I --> J
J -- Yes --> K["Attempt new URL(resolvedUrl)\nCompare origins & pathname"]
K -- non-root path --> L["setPreservedUrls[selectedAppId] = resolvedUrl"]
K -- root path --> M["delete preservedUrls[selectedAppId]"]
K -- Throws --> N["Skip preservation"]
J -- No --> N
subgraph AddressBar["Address Bar Display"]
P["Read navigationHistory[currentHistoryPosition]"]
P --> Q["try: new URL(entry).pathname"]
Q -- Success --> R["Show pathname"]
Q -- Throws --> S["Show '/'"]
end
Last reviewed commit: dfac969 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Good bug fix — resolving relative URLs before storing them in navigation history prevents Issues Summary
🟡 MEDIUM: Relative URL stored when 🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (2 items)
Generated by Dyadbot multi-agent code review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let resolvedUrl = payload?.newUrl; | ||
| if (resolvedUrl) { | ||
| try { | ||
| resolvedUrl = new URL(resolvedUrl, appUrl ?? undefined).href; | ||
| } catch { | ||
| // If it can't be resolved at all, keep the raw value | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | state-management
Relative URL silently stored in history when appUrl is null
When appUrl is null/undefined and the iframe sends a relative URL (e.g. /dashboard), new URL(resolvedUrl, undefined) throws, and the catch block preserves the raw relative value. This relative URL then leaks into navigationHistory and currentIframeUrlRef, causing downstream new URL() calls to throw (caught but silently swallowed) and the address bar to always show / instead of the actual path.
💡 Suggestion: When resolution fails, set resolvedUrl to undefined (or null) so the if (type === "pushState" && resolvedUrl) guard prevents storing an unresolvable URL:
| let resolvedUrl = payload?.newUrl; | |
| if (resolvedUrl) { | |
| try { | |
| resolvedUrl = new URL(resolvedUrl, appUrl ?? undefined).href; | |
| } catch { | |
| // If it can't be resolved at all, keep the raw value | |
| } | |
| } | |
| let resolvedUrl = payload?.newUrl; | |
| if (resolvedUrl) { | |
| try { | |
| resolvedUrl = new URL(resolvedUrl, appUrl ?? undefined).href; | |
| } catch { | |
| // If we can't resolve to absolute, don't store the relative URL | |
| // as it will cause downstream URL parsing failures | |
| resolvedUrl = undefined; | |
| } | |
| } |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 3/5
- There is a concrete medium-high risk in
src/components/preview_panel/PreviewIframe.tsx: iframe message-driven navigation is not validating URLs/schemes, so non-http/httpsor non-string inputs could be accepted and lead to unsafe navigation behavior. - The docs issue in
.claude/skills/debug-minified-error/SKILL.mdis lower impact (accuracy/usability), but it can still mislead debugging workflows and should be corrected soon. - This lands at a 3 because the top issue is user-impacting and high-confidence (7/10 severity, 9/10 confidence), while the second issue is non-blocking documentation correctness.
- Pay close attention to
src/components/preview_panel/PreviewIframe.tsx,.claude/skills/debug-minified-error/SKILL.md- enforce strict URL validation for iframe navigation messages and fix thesource-mapversion guidance.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/preview_panel/PreviewIframe.tsx">
<violation number="1" location="src/components/preview_panel/PreviewIframe.tsx:628">
P1: Validate navigation URLs from iframe messages before accepting them. Restrict to string inputs and allow only http/https schemes; reject and log anything else.
(Based on your team's feedback about validating navigation URLs from messages.) [FEEDBACK_USED]</violation>
</file>
<file name=".claude/skills/debug-minified-error/SKILL.md">
<violation number="1" location=".claude/skills/debug-minified-error/SKILL.md:182">
P2: The `source-map` version note is factually incorrect (0.7.x is the async Promise-based API; 0.6.x is synchronous), which can mislead users during debugging.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let resolvedUrl = payload?.newUrl; | ||
| if (resolvedUrl) { | ||
| try { | ||
| resolvedUrl = new URL(resolvedUrl, appUrl ?? undefined).href; |
There was a problem hiding this comment.
P1: Validate navigation URLs from iframe messages before accepting them. Restrict to string inputs and allow only http/https schemes; reject and log anything else.
(Based on your team's feedback about validating navigation URLs from messages.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/PreviewIframe.tsx, line 628:
<comment>Validate navigation URLs from iframe messages before accepting them. Restrict to string inputs and allow only http/https schemes; reject and log anything else.
(Based on your team's feedback about validating navigation URLs from messages.) </comment>
<file context>
@@ -620,33 +620,43 @@ export const PreviewIframe = ({ loading }: { loading: boolean }) => {
+ let resolvedUrl = payload?.newUrl;
+ if (resolvedUrl) {
+ try {
+ resolvedUrl = new URL(resolvedUrl, appUrl ?? undefined).href;
+ } catch {
+ // If it can't be resolved at all, keep the raw value
</file context>
| - React stack frames (reconciler functions like `renderWithHooks`, `beginWork`, `completeWork`, etc.) can be identified by their patterns — they bubble up from the actual throw site. Focus on the topmost non-React frame. | ||
| - If the error is `TypeError: Invalid URL`, look for unguarded `new URL()` calls in render paths. | ||
| - If the error is during React rendering, the topmost frame is the component whose render threw. | ||
| - The `source-map` package version 0.6.x uses `new SourceMapConsumer(rawMap)` which returns a Promise. Version 0.5.x is synchronous. |
There was a problem hiding this comment.
P2: The source-map version note is factually incorrect (0.7.x is the async Promise-based API; 0.6.x is synchronous), which can mislead users during debugging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .claude/skills/debug-minified-error/SKILL.md, line 182:
<comment>The `source-map` version note is factually incorrect (0.7.x is the async Promise-based API; 0.6.x is synchronous), which can mislead users during debugging.</comment>
<file context>
@@ -0,0 +1,193 @@
+- React stack frames (reconciler functions like `renderWithHooks`, `beginWork`, `completeWork`, etc.) can be identified by their patterns — they bubble up from the actual throw site. Focus on the topmost non-React frame.
+- If the error is `TypeError: Invalid URL`, look for unguarded `new URL()` calls in render paths.
+- If the error is during React rendering, the topmost frame is the component whose render threw.
+- The `source-map` package version 0.6.x uses `new SourceMapConsumer(rawMap)` which returns a Promise. Version 0.5.x is synchronous.
+- Source paths in the map often have relative prefixes like `../../../` — strip these mentally or programmatically to get the repo-relative path.
+
</file context>
![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 introduces two main changes. First, it fixes a crash in the preview iframe that occurred when an invalid URL was present in the navigation history. This is addressed by wrapping the URL parsing in a try-catch block. Second, it improves URL handling for pushState and replaceState events by correctly resolving relative URLs against the application's base URL. A new skill for debugging minified errors has also been added.
My feedback focuses on making the new URL resolution logic more robust by handling resolution failures more explicitly.
Note: Security Review did not run due to the size of the PR.
| } catch { | ||
| // If it can't be resolved at all, keep the raw value | ||
| } |
There was a problem hiding this comment.
The current implementation of the catch block keeps the raw value of resolvedUrl if new URL() throws. This means an invalid URL string could be propagated, relying on subsequent try-catch blocks to handle the error again. This could be made more robust.
If URL resolution fails, it's safer to nullify resolvedUrl. This would prevent the if (type === "pushState" && resolvedUrl) and else if (type === "replaceState" && resolvedUrl) blocks from executing with an invalid URL, making the control flow clearer.
} catch (e) {
// If URL resolution fails, we can't reliably use this URL.
// Log the error and nullify resolvedUrl to prevent further processing.
console.error(`Failed to resolve URL '${payload?.newUrl}' with base '${appUrl}':`, e);
resolvedUrl = null;
}
github-actions
bot
added
the
needs-human:review-issue
🎭 Playwright Test Results❌ Some tests failed
Summary: 237 passed, 1 failed, 7 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/refresh.spec.ts
|
1 participant
Uh oh!
There was an error while loading. Please reload this page.