fix: fix widget retry logic#7636
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughThe ChangesWidget iframe probe loading and retry flow
Sequence DiagramsequenceDiagram
participant Parent
participant MainIframe
participant ProbeIframe
participant ErrorDocument
Parent->>MainIframe: Start load timeout
MainIframe->>MainIframe: Timeout fires
Parent->>ErrorDocument: Replace iframe srcdoc with error UI
Parent->>Parent: User clicks retry button
ErrorDocument->>Parent: Post WIDGET_LOAD_RETRY message
Parent->>ProbeIframe: Create hidden probe iframe
ProbeIframe->>ProbeIframe: Navigate to widget URL
ProbeIframe->>Parent: Send READY message via widgetIframeTransport
Parent->>MainIframe: Clear srcdoc, reset src
Parent->>MainIframe: Restart load timeout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying explorer-dev with
|
| Latest commit: |
158a172
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2b4e1fe5.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://feat-fix-widget-retry-logic.explorer-dev-dxz.pages.dev |
| background: #fff5f5; | ||
| color: #d32f2f; | ||
|
|
||
| border: 1px solid #f5c2c7; |
There was a problem hiding this comment.
Whether the border and border radius work in the integrators page depends on how the iframe is being rendered/placed in the page, so I've removed them just to be safe.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/widget-lib/src/widgetIframeLoading.ts (1)
163-174: 💤 Low valueConsider removing the message listener in
onWidgetReady.The
onRetryMessagelistener (added on line 159) is only removed incancelWidgetLoading, not inonWidgetReady. After the widget loads successfully, this listener remains attached and fires on everywindowmessage event, returning early due to theisLoadedcheck. While functionally harmless, it adds unnecessary overhead.♻️ Suggested fix
onWidgetReady() { isLoaded = true clearTimeout(loadingTimeoutID) + window.removeEventListener('message', onRetryMessage) },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/widget-lib/src/widgetIframeLoading.ts` around lines 163 - 174, The onRetryMessage listener is only removed in cancelWidgetLoading, so after a successful load the window 'message' listener remains attached; update the onWidgetReady handler to also remove the listener (call window.removeEventListener('message', onRetryMessage)) and perform the same cleanup you do on cancel (e.g., clearTimeout(loadingTimeoutID) and optionally call cleanUpLoadCheck()) so the listener and timers are cleared when the widget loads successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/widget-lib/src/widgetIframeLoading.ts`:
- Around line 163-174: The onRetryMessage listener is only removed in
cancelWidgetLoading, so after a successful load the window 'message' listener
remains attached; update the onWidgetReady handler to also remove the listener
(call window.removeEventListener('message', onRetryMessage)) and perform the
same cleanup you do on cancel (e.g., clearTimeout(loadingTimeoutID) and
optionally call cleanUpLoadCheck()) so the listener and timers are cleared when
the widget loads successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a60e98c5-e0a9-4e70-8f00-e57db5161294
📒 Files selected for processing (1)
libs/widget-lib/src/widgetIframeLoading.ts
Deploying swap-dev with
|
| Latest commit: |
158a172
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://609475b4.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://feat-fix-widget-retry-logic.swap-dev-5u6.pages.dev |
kernelwhisperer
requested review from
kernelwhisperer
and removed request for
a team
|
@Danziger the button doesn't work to me. kkklk.mov |
|
@shoom3301 The widget configurator preview sets a strict CSP (script-src 'self'), and srcdoc iframes inherit the parent page's policy. I could fix it adding I'd park this until the widget configurator revamp PR is merged, as I have some changes there that show a loader while the iframe loads (added from the app (widget configurator app in this case) context). I'll try this change in there later and re-assess the fix and/or whether we need/want to fix it. |
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 2m): retry probe can be swallowed in local widget origins
Finding: Hidden retry probe can mark the real widget as ready before reload
- Location:
libs/widget-lib/src/widgetIframeLoading.ts:129 - The retry check creates a hidden iframe that emits the normal
cowSwapWidgetREADYmessage. In local widget origins,IframeTransport.listenToMessageFromWindow()intentionally accepts source mismatches when the origin is local, so the existinglistenToReady(iframeWindow, ...)listener for the real iframe can consume the probe iframe'sREADYfirst. - That calls
onWidgetReady(), setsisLoaded = true, and then the probe callback returns early incompleteCleanUpLoadCheck(true). The visible iframe keeps the errorsrcdocand the button can stay onLoading...instead of retrying the real widget. - This is separate from the existing open thread, which only discusses iframe error-page border styling.
Suggested fix
- Do not let the probe reuse the same parent-level
READYchannel in a way that existing widget listeners can consume, or make the probe completion path reload the visible iframe before/independent ofonWidgetReady()being set by the probe message. - Add focused coverage for the local-origin source-mismatch case: dispatch a
READYmessage from a non-visible/probe iframe window while the visible iframe is still showingsrcdoc, and assert retry removessrcdocand restoresoriginalSrc.
Review scope and related context
Checked current PR head 167870f17075866e709b440e0432d67f6dfda071, the PR description, the single existing review thread, and the changed widget loading flow. I did not find other non-duplicate actionable comments.
🤖 Prompt for AI agents
Verify this finding against current PR head before changing code.
Context:
- `widgetIframeLoading.ts` creates a hidden probe iframe and listens for `WidgetMethodsEmit.READY`.
- `cowSwapWidget.ts` already has a parent `READY` listener for the real iframe.
- `IframeTransport.listenToMessageFromWindow()` allows source mismatches for local origins.
- If the probe READY reaches the older real-iframe READY listener first, `onWidgetReady()` can set `isLoaded = true` before `completeCleanUpLoadCheck(true)` reloads the visible iframe.
Expected fix:
- Ensure probe READY cannot be consumed as real-widget readiness, or ensure successful probe completion still reloads the visible iframe.
- Add targeted widget-lib test coverage for local-origin source mismatch during retry.
❌ Automated QA failed: advertised widget retry flow still does not show loading stateOutcome
Run details
How to retest
Expected per PR description:
Observed:
Artifacts Full MP4 recording: https://github.com/user-attachments/assets/c4ed31b4-2f35-424b-a178-5b988ae87bd5 1s after clicking Retry: After waiting post-retry: Commands + setup
|
4 participants
Summary
Clicking "Retry" will now show "Loading..." in the button. If the retry fails, the same error page is displayed. Before this fix, the default browser error page will be shown instead in this scenario.
To Test
baseURLto an incorrect (non-existent) page.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes