Stop debounced entry resets surfacing "Cancelled" error toast#2415
Stop debounced entry resets surfacing "Cancelled" error toast#2415myieye wants to merge 3 commits into
Conversation
runed's useDebounce rejects pending promises with the string "Cancelled" when cancel() supersedes them. In EntryLoaderService these rejections went unhandled (notably the fire-and-forget reset in the watch), so the global error handler surfaced a red "Cancelled" toast during rapid filter typing or entry switching. Swallow only that specific rejection at the debounced call sites, rethrowing anything else so real errors still surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA helper function ChangesDebounce cancellation handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/viewer/src/lib/services/entry-loader-service.svelte.ts (1)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for the rethrow branch.
The existing test around debounce collapsing (
entry-loader-service.svelte.test.ts:184-204) confirms the suppression path works but doesn't assert that a non-"Cancelled" rejection from the debounced reset still propagates as an error. Worth adding given this is the crux of the fix (only suppress the specific cancellation, not genuine failures).🤖 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 `@frontend/viewer/src/lib/services/entry-loader-service.svelte.ts` around lines 30 - 37, Add a test in entry-loader-service.svelte.test.ts for the rethrow path of ignoreDebounceCancelled: verify that when the debounced reset rejects with something other than "Cancelled", the error is not swallowed and still propagates. Extend the existing debounce collapsing coverage around the entry loader service/reset flow so it asserts the suppression only applies to the specific cancellation case, while genuine failures from useDebounce remain observable.
🤖 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 `@frontend/viewer/src/lib/services/entry-loader-service.svelte.ts`:
- Around line 30-37: Add a test in entry-loader-service.svelte.test.ts for the
rethrow path of ignoreDebounceCancelled: verify that when the debounced reset
rejects with something other than "Cancelled", the error is not swallowed and
still propagates. Extend the existing debounce collapsing coverage around the
entry loader service/reset flow so it asserts the suppression only applies to
the specific cancellation case, while genuine failures from useDebounce remain
observable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e3ac1cd-d027-4bb6-989d-8877c25dcfcf
📒 Files selected for processing (1)
frontend/viewer/src/lib/services/entry-loader-service.svelte.ts
Export the helper and assert "Cancelled" is swallowed while any other rejection still propagates, per CodeRabbit review on #2415. The service-level reset path cannot exercise the rethrow branch since #executeReset swallows its own fetch errors, so this covers the helper directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the nitpick: exported 🤖 Addressed by Claude Code |
The .catch(ignoreDebounceCancelled) wrapper makes quietReset() return a fresh promise per call, so the old expect(p1).toBe(p2) identity checks no longer hold. Collapse behavior is unchanged and already covered by the single-API-call assertion; assert that directly instead of promise identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rapid filter typing / entry switching in the viewer could pop a red "Cancelled" error toast.
runed'suseDebouncerejects pending promises with the string"Cancelled"whencancel()supersedes them; inEntryLoaderServicethose rejections went unhandled (notably the fire-and-forgetvoid this.#scheduleFilterReset()in thewatch), so the global error handler surfaced them.Fix swallows only the
"Cancelled"string rejection at the two debounced call sites (#scheduleFilterReset,#scheduleEventReset), rethrowing anything else so genuine load errors still surface. Scoped to the service rather than the global handler to keep the suppression narrow.Reproduced on
/testing/project-viewby typing a filter and immediately clicking an entry.svelte-checkclean.🤖 Generated with Claude Code