feat(widget): show loading indication on retry#7782
Conversation
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughUpdates the widget iframe retry flow to deduplicate error panels, show a disabled “Loading…” state, recreate the iframe through a reload callback, restore visibility on readiness, and expose the current iframe reference. Adds Jest coverage for these behaviors. ChangesIframe retry lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ErrorPanel
participant widgetIframeLoading
participant cowSwapWidget
participant Iframe
participant Widget
User->>ErrorPanel: click Reload
ErrorPanel->>widgetIframeLoading: disable button and show Loading...
widgetIframeLoading->>cowSwapWidget: invoke reloadIframe
cowSwapWidget->>Iframe: destroy and replace iframe
cowSwapWidget->>Widget: reattach iframe and run setup
Widget->>widgetIframeLoading: report ready
widgetIframeLoading->>Iframe: restore visibility
widgetIframeLoading->>ErrorPanel: remove error panel
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/widget-lib/src/widgetIframeLoading.ts (1)
27-54: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry loading state can get stuck forever — timeout isn't re-armed on reload.
The initial
window.setTimeout(..., LOADING_TIMEOUT)(Lines 74-78) is set up once and only cleared inonWidgetReady. The reload click handler (Lines 41-54) never re-arms an equivalent timeout for the retry attempt. If the retried load silently hangs (never fireserror, never becomes ready — exactly the scenarioLOADING_TIMEOUTexists to catch), the button remains disabled with "Loading…" indefinitely with no recovery path. This directly undermines this PR's goal of giving users a visible, resolvable retry indication.🐛 Suggested fix: re-arm the loading timeout on retry
reloadBtn.addEventListener('click', () => { reloadBtn.disabled = true errorText.innerText = 'Loading…' destroy(true) iframe.src = 'about:blank' setTimeout(() => { iframe.src = originalSrc setup() + + timeout = window.setTimeout(() => { + if (cancelled) return + onLoadingError() + }, LOADING_TIMEOUT) }, 100) })Also applies to: 74-78
🤖 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 27 - 54, The retry path in onLoadingError does not re-arm the loading timeout, so a hung reload can leave the panel stuck in the loading state. Update the reload button handler in widgetIframeLoading.ts to start a fresh LOADING_TIMEOUT for each retry attempt, and make sure onWidgetReady still clears whichever timeout is active. Use the existing onLoadingError, onWidgetReady, and reloadBtn flow to keep the retry behavior consistent.
🧹 Nitpick comments (2)
libs/widget-lib/src/widgetIframeLoading.ts (1)
40-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo screen-reader announcement for the loading-state change.
The button/text toggle (
disabled = true,errorText.innerText = 'Loading…') is a purely visual/DOM change; withoutaria-liveonerrorTextoraria-busyon the panel, assistive tech won't announce the retry progress. Consider addingaria-live="polite"toerrorText(or the container) for accessible progress feedback.🤖 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 40 - 47, The reload/loading state update in widgetIframeLoading’s click handler is only visual, so assistive tech won’t announce the retry progress. Update the logic around reloadBtn and errorText to expose the loading transition to screen readers, preferably by adding aria-live="polite" to errorText (or setting aria-busy on the surrounding panel) when switching to “Loading…”. Keep the change localized to the reload button state management in widgetIframeLoading.libs/widget-lib/src/widgetIframeLoading.test.ts (1)
72-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the retry's actual reload sequence.
Tests verify the disabled/loading UI state and panel dedupe, but none exercise the
setTimeout(..., 100)callback that resetsiframe.srcand callssetup()after a reload click — nor a scenario where the retry itself times out (relevant to the missing timeout re-arm flagged inwidgetIframeLoading.ts). Consider adding a fake-timers test to assertsetup()is invoked andiframe.srcis restored after the retry delay.🤖 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.test.ts` around lines 72 - 91, The retry path in widgetIframeLoading lacks coverage for the actual delayed reload flow, including the setTimeout callback that restores iframe.src and re-invokes setup after a retry click. Add a fake-timers test in widgetIframeLoading.test that exercises the retry sequence on the widget iframe loading behavior, then assert the delayed callback runs, setup() is called again, and iframe.src is restored; also cover the retry timeout being re-armed in widgetIframeLoading.ts using the relevant retry handler/setup symbols.
🤖 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.
Outside diff comments:
In `@libs/widget-lib/src/widgetIframeLoading.ts`:
- Around line 27-54: The retry path in onLoadingError does not re-arm the
loading timeout, so a hung reload can leave the panel stuck in the loading
state. Update the reload button handler in widgetIframeLoading.ts to start a
fresh LOADING_TIMEOUT for each retry attempt, and make sure onWidgetReady still
clears whichever timeout is active. Use the existing onLoadingError,
onWidgetReady, and reloadBtn flow to keep the retry behavior consistent.
---
Nitpick comments:
In `@libs/widget-lib/src/widgetIframeLoading.test.ts`:
- Around line 72-91: The retry path in widgetIframeLoading lacks coverage for
the actual delayed reload flow, including the setTimeout callback that restores
iframe.src and re-invokes setup after a retry click. Add a fake-timers test in
widgetIframeLoading.test that exercises the retry sequence on the widget iframe
loading behavior, then assert the delayed callback runs, setup() is called
again, and iframe.src is restored; also cover the retry timeout being re-armed
in widgetIframeLoading.ts using the relevant retry handler/setup symbols.
In `@libs/widget-lib/src/widgetIframeLoading.ts`:
- Around line 40-47: The reload/loading state update in widgetIframeLoading’s
click handler is only visual, so assistive tech won’t announce the retry
progress. Update the logic around reloadBtn and errorText to expose the loading
transition to screen readers, preferably by adding aria-live="polite" to
errorText (or setting aria-busy on the surrounding panel) when switching to
“Loading…”. Keep the change localized to the reload button state management in
widgetIframeLoading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15248336-733f-4170-b3cb-175abbd291b6
📒 Files selected for processing (2)
libs/widget-lib/src/widgetIframeLoading.test.tslibs/widget-lib/src/widgetIframeLoading.ts
elena-zh
added
the
trigger-preview
|
Cloudflare Pages preview mirror Preview branch URL: https://github.com/cowprotocol/cowswap/tree/cf-preview/pr-7782 Source fork branch:
|
github-actions
Bot
removed
the
trigger-preview
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve , thank you for the fix.
From a UI perspective, changes look good to me.
But from the functional perspective, it does not work as expected:
- I press on Retry
- nothing is blocking requests to pre-load the widget
- but is cannot be loaded, I still see an error with 'Retry' button
Expected: 'retry' button should load the trading interface if the URL is unblocked. Please compare the behavior with https://dev.widget.cow.fi/
Clicking "Reload" on the widget loading-error panel tore the panel down immediately and showed a blank iframe, so there was no indication the retry was running until the widget finished loading (or the 30s timeout re-fired). Keep the panel mounted and put it into a loading state instead: disable the button (the existing :disabled style already sets cursor: progress) and swap the copy to "Loading…". The panel is cleared by onWidgetReady on success — which now also re-reveals the iframe — or replaced by onLoadingError on another failure. onLoadingError also removes any existing panel first so repeated failures don't stack panels.
Swapping the sandboxed iframe's src in place did not reliably re-fetch the widget, so a failed load never recovered when the URL became reachable again. Retry now tears the frame down and builds a fresh one, which re-emits READY on load and clears the error panel. Expose handler.iframe via a getter so it keeps pointing at the live element after a rebuild. Addresses @elena-zh's review feedback.
tenderdeve
force-pushed
the
fix/7734-widget-retry-loading
branch
from
19f3f37 to
8b16e69
Compare
|
@elena-zh good catch — the retry only swapped the sandboxed iframe's |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Thank you, looks good now
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 5m): retry drops runtime-updated widget listeners
Finding: [BLOCKING] Preserve updated event listeners across iframe retry
- Location:
libs/widget-lib/src/cowSwapWidget.ts:148 - This one is important:
setup()always constructsIframeCowEventEmitterwith the initialprops.listeners. updateListeners()updates only the current emitter. AfterreloadIframe()rebuilds the iframe and callssetup(), listeners added through the public API disappear, while old mount-time listeners may return.- The React wrapper uses
updateListeners()when itslistenersprop changes, so this affects the normal integration path.
Suggested fix
- Track the current listeners alongside
providerandcurrentParams; update that value inupdateListeners()and pass it to every new emitter. - Add a widget-level regression test: update listeners, trigger iframe error and Reload, emit an event from the rebuilt
handler.iframe, then verify only the updated listener runs.
Review scope and related context
- The existing reviewer verified that rebuilding the iframe now recovers when the URL becomes available.
- CodeRabbit’s timeout concern is fixed at the current head: retry calls
setup(), which creates a fresh loading context and timeout. - CodeRabbit already raised the accessibility announcement concern, so it is not repeated here.
🤖 Prompt for AI agents
Verify this finding against the current PR head and keep the fix scoped.
Context:
- createCowSwapWidget.setup() initializes IframeCowEventEmitter with the original props.listeners.
- handler.updateListeners() changes only the active emitter.
- reloadIframe() destroys that emitter and calls setup(), restoring the original listeners.
Persist the latest listeners across iframe rebuilds and add a widget-level test covering updateListeners -> loading error -> Reload -> event from the rebuilt iframe.
Generated using the pr-review skill from the CoW Protocol skills repo.
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 `@libs/widget-lib/src/cowSwapWidget.ts`:
- Around line 163-180: The inner zero-argument reloadIframe function shadows the
module-level reloadIframe(iframe, params), causing updateParams to rebuild the
iframe instead of performing the about:blank sandbox transition when
disableWindowOpen changes. Rename or otherwise disambiguate the local helper and
update updateParams to call the intended module-level function; adjust the
affected test to read handler.iframe after updateParams or assert the rebuilt
iframe behavior.
🪄 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: 539b02ac-a22a-4031-b2ae-c58172823049
📒 Files selected for processing (3)
libs/widget-lib/src/cowSwapWidget.tslibs/widget-lib/src/widgetIframeLoading.test.tslibs/widget-lib/src/widgetIframeLoading.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/widget-lib/src/widgetIframeLoading.test.ts
| const loadingContext = widgetIframeLoading(container, iframe, reloadIframe, props.onLoadingError) | ||
|
|
||
| cancelWidgetLoading = loadingContext.cancelWidgetLoading | ||
| const onWidgetReady = loadingContext.onWidgetReady | ||
| } | ||
|
|
||
| // Rebuild the iframe from scratch and re-run setup. Reusing the same sandboxed frame and only | ||
| // swapping its `src` did not reliably re-fetch the widget on retry, so a failed load could never | ||
| // recover. A brand new iframe gives a clean document that re-emits READY once it loads. | ||
| function reloadIframe(): void { | ||
| destroy() | ||
|
|
||
| iframe = createIframe(currentParams) | ||
| container.appendChild(iframe) | ||
|
|
||
| setup() | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the about:blank test still exists in the current branch
rg -n 'about:blank' libs/widget-lib/src/cowSwapWidget.test.ts
# Also check if the test captures handler.iframe before updateParams
rg -n -A5 'reloads the iframe with no popup' libs/widget-lib/src/cowSwapWidget.test.tsRepository: cowprotocol/cowswap
Length of output: 477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the widget implementation around the reported lines and the module-level reload helper.
sed -n '140,240p' libs/widget-lib/src/cowSwapWidget.ts
printf '\n---\n'
sed -n '300,360p' libs/widget-lib/src/cowSwapWidget.ts
printf '\n=== TEST CONTEXT ===\n'
sed -n '300,350p' libs/widget-lib/src/cowSwapWidget.test.tsRepository: cowprotocol/cowswap
Length of output: 7842
updateParams is calling the local reloadIframe() helper, not the module-level sandbox swap.
The zero-arg inner function shadows reloadIframe(iframe, params), so the disableWindowOpen path now destroys and recreates the iframe instead of performing the about:blank transition. The existing test that captures handler.iframe before updateParams should either re-read the live iframe after the call or be updated to reflect the rebuild flow.
🤖 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/cowSwapWidget.ts` around lines 163 - 180, The inner
zero-argument reloadIframe function shadows the module-level
reloadIframe(iframe, params), causing updateParams to rebuild the iframe instead
of performing the about:blank sandbox transition when disableWindowOpen changes.
Rename or otherwise disambiguate the local helper and update updateParams to
call the intended module-level function; adjust the affected test to read
handler.iframe after updateParams or assert the rebuilt iframe behavior.
3 participants
Fixes #7734
Clicking Reload on the widget's loading-error panel removed the panel right away and showed a blank iframe, so nothing indicated the retry was in progress until the widget loaded (or the 30s timeout re-fired).
Now the panel stays mounted in a loading state: the button is disabled (the
:disabledrule already shipscursor: progress) and the text switches to "Loading…". It's cleared byonWidgetReadyon success — which now also re-reveals the iframe — or replaced byonLoadingErroron another failure.onLoadingErroralso clears any existing panel first so repeated failures don't stack.Tests added to
widgetIframeLoading.test.ts: reload shows the loading state, ready removes the panel and reveals the iframe, and repeated failures keep a single panel.Summary by CodeRabbit