fix(widget): add iframe loading retry logic#7636
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deploying explorer-dev with
|
| Latest commit: |
e988bb3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://271c1d6e.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://feat-fix-widget-retry-logic.explorer-dev-dxz.pages.dev |
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: |
e988bb3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f22e189e.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://feat-fix-widget-retry-logic.swap-dev-5u6.pages.dev |
|
@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. |
✅ Security Review (Codex GPT-5, worked 30m): no verified security findingsReview completed. I found no verified security findings worth posting. Review scope and related context
Generated using the |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesWidget iframe probe loading and preview callback wiring
Sequence Diagram(s)sequenceDiagram
participant Parent
participant MainIframe
participant ProbeIframe
participant ErrorDocument
participant widgetIframeTransport
Parent->>MainIframe: Start primary load timeout
MainIframe->>Parent: Timeout fires
Parent->>ErrorDocument: Render error srcdoc
ErrorDocument->>Parent: Post WIDGET_LOAD_RETRY
Parent->>ProbeIframe: Create hidden probe iframe
ProbeIframe->>widgetIframeTransport: Navigate and emit READY
widgetIframeTransport->>Parent: Deliver READY message
Parent->>MainIframe: Clear srcdoc and reset src
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 |
| * Custom CSS appended to the error document displayed inside the iframe when the widget fails to load. | ||
| * Use it to override the default look (`.errorContent` and `.reloadButton` classes). | ||
| */ | ||
| loadingErrorStyles?: string |
There was a problem hiding this comment.
Since the loading error DOM is on the host side, they can style it using css selector .cow-widget-loading-error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@apps/widget-configurator/src/components/VersionedCowSwapWidget/VersionedCowSwapWidget.tsx`:
- Around line 59-73: The LazyPinnedCowSwapWidget function is missing the
onLoadingError prop in its destructuring and is not forwarding it to the
LazyCowSwapWidget component. Add onLoadingError to the destructuring of
VersionedCowSwapWidgetProps in the LazyPinnedCowSwapWidget function parameters,
and then pass onLoadingError as a prop to the LazyCowSwapWidget component
alongside the existing params, provider, listeners, and onReady props. This
ensures error callbacks are properly propagated for all pinned widget versions,
not just local ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f08f721-6910-4c4a-947a-97a523981983
📒 Files selected for processing (2)
apps/widget-configurator/src/components/VersionedCowSwapWidget/VersionedCowSwapWidget.tsxapps/widget-configurator/src/components/configurator/configurator.component.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@apps/widget-configurator/src/components/VersionedCowSwapWidget/VersionedCowSwapWidget.tsx`:
- Around line 59-73: The LazyPinnedCowSwapWidget function is missing the
onLoadingError prop in its destructuring and is not forwarding it to the
LazyCowSwapWidget component. Add onLoadingError to the destructuring of
VersionedCowSwapWidgetProps in the LazyPinnedCowSwapWidget function parameters,
and then pass onLoadingError as a prop to the LazyCowSwapWidget component
alongside the existing params, provider, listeners, and onReady props. This
ensures error callbacks are properly propagated for all pinned widget versions,
not just local ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f08f721-6910-4c4a-947a-97a523981983
📒 Files selected for processing (2)
apps/widget-configurator/src/components/VersionedCowSwapWidget/VersionedCowSwapWidget.tsxapps/widget-configurator/src/components/configurator/configurator.component.tsx
🛑 Comments failed to post (1)
apps/widget-configurator/src/components/VersionedCowSwapWidget/VersionedCowSwapWidget.tsx (1)
59-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Forward
onLoadingErrorthrough the pinned widget path.
onLoadingErroris part ofwidgetProps(Line 86) but gets dropped inLazyPinnedCowSwapWidget(Lines 65 and 72). That makes the callback work only forsdkVersion === 'local'and breaks preview error callback propagation for pinned versions.Suggested fix
function LazyPinnedCowSwapWidget({ sdkVersion, params, provider, listeners, onReady, + onLoadingError, }: VersionedCowSwapWidgetProps & { sdkVersion: PinnedWidgetSdkVersion }): ReactNode { const LazyCowSwapWidget = LAZY_WIDGETS_BY_VERSION[sdkVersion] const legacyIframeLoadReveal = widgetSdkVersionSupportsReadyEvent(sdkVersion) ? undefined : onReady return ( <Suspense fallback={null}> <LegacyPinnedPreviewReveal onIframeLoad={legacyIframeLoadReveal}> - <LazyCowSwapWidget params={params} provider={provider} listeners={listeners} onReady={onReady} /> + <LazyCowSwapWidget + params={params} + provider={provider} + listeners={listeners} + onReady={onReady} + onLoadingError={onLoadingError} + /> </LegacyPinnedPreviewReveal> </Suspense> ) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function LazyPinnedCowSwapWidget({ sdkVersion, params, provider, listeners, onReady, onLoadingError, }: VersionedCowSwapWidgetProps & { sdkVersion: PinnedWidgetSdkVersion }): ReactNode { const LazyCowSwapWidget = LAZY_WIDGETS_BY_VERSION[sdkVersion] const legacyIframeLoadReveal = widgetSdkVersionSupportsReadyEvent(sdkVersion) ? undefined : onReady return ( <Suspense fallback={null}> <LegacyPinnedPreviewReveal onIframeLoad={legacyIframeLoadReveal}> <LazyCowSwapWidget params={params} provider={provider} listeners={listeners} onReady={onReady} onLoadingError={onLoadingError} /> </LegacyPinnedPreviewReveal> </Suspense> ) }🤖 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 `@apps/widget-configurator/src/components/VersionedCowSwapWidget/VersionedCowSwapWidget.tsx` around lines 59 - 73, The LazyPinnedCowSwapWidget function is missing the onLoadingError prop in its destructuring and is not forwarding it to the LazyCowSwapWidget component. Add onLoadingError to the destructuring of VersionedCowSwapWidgetProps in the LazyPinnedCowSwapWidget function parameters, and then pass onLoadingError as a prop to the LazyCowSwapWidget component alongside the existing params, provider, listeners, and onReady props. This ensures error callbacks are properly propagated for all pinned widget versions, not just local ones.



Summary
The biggest problem was in the fact that we need to rebind all the listeners connected to the iframe window, because after retry the
iframe.contentWindowis a new instance.I didn't add a loading state to the error screen in this PR. I think we should add it separately as a generic solution and display it in case of the default iframe loading (not after retry).
tteee.mov
To Test
Summary by CodeRabbit
New Features
Bug Fixes