fix(frontend): stop four newly-live toast storms from starving the notification surface - #972
Conversation
… surface (#966) PR #965 mounted <Toaster /> for the first time, making ~101 dormant toast.* calls across 25 files go live at once. Four of them storm. - useChat.ts MCP sandbox unreachable: was a per-payload duration:Infinity toast with no id, so three of them permanently occupied every slot under visibleToasts={3} and starved every later toast app-wide. Now routes to the existing RunErrorBanner, which is idempotent by construction. Also wires the event through both parseEvent switches and convertEventToLegacy — it was silently dropped on the unified stream path, so the run stopped with no explanation at all. - useChat.ts recoveryMode error: closes the stream and notifies once per stream behind a latch and a stable id. - useMessageQueue.ts: ~30 toasts per outage collapse into one accumulating summary toast under a fixed id. - ChatContext.tsx: fixes the retry loop under the toast — the save signature stayed dirty on failure, re-firing a failing PATCH every keystroke. Autosave now backs off and re-arms; manual save always bypasses. - schedules stack: "the hook owns the toast" applied across four files, removing 12 duplicate toast statements. - memories/edit.tsx: StrictMode double-fire (not in the issue, same class). Background/effect-driven toasts get stable ids; user-initiated toasts deliberately do not. Refs #966 Signed-off-by: ryaneggz <kre8mymedia@gmail.com>
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR audits frontend toast behavior, adds MCP sandbox stream-error propagation, restricts run-error replay, adds autosave backoff, aggregates queue-drop notifications, centralizes schedule notifications, and prevents duplicate memory-load toasts. ChangesStream and run-error handling
Autosave and queue notifications
Schedule notification ownership
Memory load handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SSEBackend
participant FetchStreamReader
participant StreamSource
participant useChat
participant ChatMessages
SSEBackend->>FetchStreamReader: Send mcp_sandbox_unreachable
FetchStreamReader->>StreamSource: Emit terminal event
StreamSource->>useChat: Deliver event
useChat->>ChatMessages: Set non-recoverable run error
ChatMessages->>ChatMessages: Hide replay control
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
An audit of the diff found one defect worse than the bug it fixed, plus four smaller ones. - ChatContext: the backoff retry is armed from the promise catch, outside the render that scheduled it, so it could not rely on the autosave effect's cleanup — that effect returns early with no cleanup on four branches. A timer armed after one of those could survive unmount and PATCH an unmounted tree forever, holding a stale auth token. Adds a mount-scoped teardown plus a live token re-read, and a test that reproduces the exact escape path (it fails without the teardown). - ChatContext: a failed manual save now re-arms too. It previously left the user in exactly the stranded state the backoff exists to prevent. - useMessageQueue: the drop counter never reset on success, so a degraded backend interleaving drops and successes accumulated indefinitely and eventually reported a mass failure that never happened. - useChat: RunError.title was dead code — no call site set it — with two tests asserting a field the app cannot produce. Removed. - streamSource/useChat: mcp_sandbox_unreachable was missing from both terminal-event sets and worked only because the distributed emitter appends a trailing done; the sync emitter does not. - useSchedules: "Failed to load schedule" was one character from the list-load message on the same screen; now "Failed to open schedule". Refs #966 Signed-off-by: ryaneggz <kre8mymedia@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/pages/memories/edit.test.tsx (1)
23-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the failed request through the shared API mock.
Configure the failed memory request in
src/tests/mocks/. Remove themockGetcall-count assertion. Wait for the toast and redirect instead. This keeps the test focused on observable behavior.As per coding guidelines, “Mock API calls using mocks in
src/tests/mocks/” and “test observable behavior rather than implementation details.”Also applies to: 60-63
🤖 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/src/pages/memories/edit.test.tsx` around lines 23 - 32, Move the failed memory request setup from the local mockGet in the edit test to the shared API mocks under src/tests/mocks/, and remove the mockGet call-count assertion. Update the test to await the observable error toast and redirect behavior instead of inspecting service implementation details.Source: Coding guidelines
Changelog.md (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense these changelog entries.
Keep each entry to the user-visible fix and its effect. Move implementation paths, event sequencing, measurements, and investigation details to the PR description or linked issues.
As per coding guidelines, include “concise change notes.”
🤖 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 `@Changelog.md` around lines 11 - 14, Condense the four fix/966-toast-audit entries in Changelog.md to concise, user-visible change notes describing each fix and its effect. Remove implementation paths, internal symbols, event sequencing, measurements, investigation history, and rejected alternatives; retain only outcomes such as preventing toast storms, surfacing sandbox failures in the run error banner, retrying failed autosaves with backoff, and eliminating duplicate schedule notifications.Source: Coding guidelines
frontend/src/lib/utils/fetchStreamReader.test.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
@/alias for imports fromsrc.These relative imports target modules under
frontend/src. Replace them with the configured alias.
frontend/src/lib/utils/fetchStreamReader.test.ts#L3-L3: import from@/lib/utils/fetchStreamReader.frontend/src/hooks/useChat.test.tsx#L4-L4: import from@/hooks/useChat.frontend/src/components/lists/ChatMessages.test.tsx#L27-L27: import from@/components/lists/ChatMessages.As per coding guidelines, use the
@/import alias for imports fromsrc.🤖 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/src/lib/utils/fetchStreamReader.test.ts` at line 3, Replace the relative imports in frontend/src/lib/utils/fetchStreamReader.test.ts lines 3-3, frontend/src/hooks/useChat.test.tsx lines 4-4, and frontend/src/components/lists/ChatMessages.test.tsx lines 27-27 with the configured `@/` alias imports, preserving each referenced module and test behavior.Source: Coding guidelines
frontend/src/lib/entities/stream.ts (1)
71-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse two-space indentation in the changed frontend code.
The changed code uses tabs. Run Prettier with the repository-required two-space indentation.
frontend/src/lib/entities/stream.ts#L71-L80: reformat the changed interface documentation and declaration.frontend/src/lib/utils/fetchStreamReader.ts#L184-L185: reformat the new parser cases.frontend/src/lib/utils/fetchStreamReader.test.ts#L6-L7: reformat the test helper.frontend/src/lib/utils/streamSource.ts#L221-L228: reformat the terminal-event branch.frontend/src/hooks/useChat.ts#L30-L36: reformat the exported toast identifier block.frontend/src/hooks/useChat.test.tsx#L88-L135: reformat the controllable stream mock.frontend/src/components/lists/ChatMessages.tsx#L272-L275: reformat replay eligibility logic.frontend/src/components/lists/ChatMessages.test.tsx#L29-L32: reformat the render helper.frontend/src/context/ChatContext.tsx#L41-L61: reformat autosave constants.frontend/src/tests/context/ChatContext.test.tsx#L10-L17: reformat the hoisted test mocks.frontend/src/hooks/useMessageQueue.ts#L21-L42: reformat notification constants.frontend/src/tests/hooks/useMessageQueue.test.ts#L10-L15: reformat the toast mock.As per coding guidelines, format frontend TypeScript files with Prettier using two-space indentation.
🤖 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/src/lib/entities/stream.ts` around lines 71 - 80, Reformat the changed frontend TypeScript code with the repository’s Prettier configuration and two-space indentation. Apply this to frontend/src/lib/entities/stream.ts:71-80, frontend/src/lib/utils/fetchStreamReader.ts:184-185, frontend/src/lib/utils/fetchStreamReader.test.ts:6-7, frontend/src/lib/utils/streamSource.ts:221-228, frontend/src/hooks/useChat.ts:30-36, frontend/src/hooks/useChat.test.tsx:88-135, frontend/src/components/lists/ChatMessages.tsx:272-275, frontend/src/components/lists/ChatMessages.test.tsx:29-32, frontend/src/context/ChatContext.tsx:41-61, frontend/src/tests/context/ChatContext.test.tsx:10-17, frontend/src/hooks/useMessageQueue.ts:21-42, and frontend/src/tests/hooks/useMessageQueue.test.ts:10-15; preserve the existing logic and only normalize formatting.Source: Coding guidelines
🤖 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 `@frontend/src/components/panels/AgentSchedulesPanel.test.tsx`:
- Around line 27-35: Replace the inline scheduleService mock in
AgentSchedulesPanel.test.tsx and index.test.tsx with the shared schedule API
mock from src/tests/mocks/, preserving the existing mocked methods and test
behavior in both files.
---
Nitpick comments:
In `@Changelog.md`:
- Around line 11-14: Condense the four fix/966-toast-audit entries in
Changelog.md to concise, user-visible change notes describing each fix and its
effect. Remove implementation paths, internal symbols, event sequencing,
measurements, investigation history, and rejected alternatives; retain only
outcomes such as preventing toast storms, surfacing sandbox failures in the run
error banner, retrying failed autosaves with backoff, and eliminating duplicate
schedule notifications.
In `@frontend/src/lib/entities/stream.ts`:
- Around line 71-80: Reformat the changed frontend TypeScript code with the
repository’s Prettier configuration and two-space indentation. Apply this to
frontend/src/lib/entities/stream.ts:71-80,
frontend/src/lib/utils/fetchStreamReader.ts:184-185,
frontend/src/lib/utils/fetchStreamReader.test.ts:6-7,
frontend/src/lib/utils/streamSource.ts:221-228,
frontend/src/hooks/useChat.ts:30-36, frontend/src/hooks/useChat.test.tsx:88-135,
frontend/src/components/lists/ChatMessages.tsx:272-275,
frontend/src/components/lists/ChatMessages.test.tsx:29-32,
frontend/src/context/ChatContext.tsx:41-61,
frontend/src/tests/context/ChatContext.test.tsx:10-17,
frontend/src/hooks/useMessageQueue.ts:21-42, and
frontend/src/tests/hooks/useMessageQueue.test.ts:10-15; preserve the existing
logic and only normalize formatting.
In `@frontend/src/lib/utils/fetchStreamReader.test.ts`:
- Line 3: Replace the relative imports in
frontend/src/lib/utils/fetchStreamReader.test.ts lines 3-3,
frontend/src/hooks/useChat.test.tsx lines 4-4, and
frontend/src/components/lists/ChatMessages.test.tsx lines 27-27 with the
configured `@/` alias imports, preserving each referenced module and test
behavior.
In `@frontend/src/pages/memories/edit.test.tsx`:
- Around line 23-32: Move the failed memory request setup from the local mockGet
in the edit test to the shared API mocks under src/tests/mocks/, and remove the
mockGet call-count assertion. Update the test to await the observable error
toast and redirect behavior instead of inspecting service implementation
details.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc3c39fb-cac9-42a8-b48c-cd4f7781fe47
📒 Files selected for processing (21)
Changelog.mdfrontend/src/components/lists/ChatMessages.test.tsxfrontend/src/components/lists/ChatMessages.tsxfrontend/src/components/panels/AgentSchedulesPanel.test.tsxfrontend/src/components/panels/AgentSchedulesPanel.tsxfrontend/src/context/ChatContext.tsxfrontend/src/hooks/useAgentSchedules.tsfrontend/src/hooks/useChat.test.tsxfrontend/src/hooks/useChat.tsfrontend/src/hooks/useMessageQueue.tsfrontend/src/hooks/useSchedules.tsfrontend/src/lib/entities/stream.tsfrontend/src/lib/utils/fetchStreamReader.test.tsfrontend/src/lib/utils/fetchStreamReader.tsfrontend/src/lib/utils/streamSource.tsfrontend/src/pages/memories/edit.test.tsxfrontend/src/pages/memories/edit.tsxfrontend/src/pages/schedules/index.test.tsxfrontend/src/pages/schedules/index.tsxfrontend/src/tests/context/ChatContext.test.tsxfrontend/src/tests/hooks/useMessageQueue.test.ts
…audit Signed-off-by: ryaneggz <kre8mymedia@gmail.com> # Conflicts: # Changelog.md # frontend/src/components/lists/ChatMessages.tsx
Closes #966
PR #965 mounted
<Toaster visibleToasts={3} />for the first time, making ~101 previously-dormanttoast.*calls across 25 files go live at once. #965 fixed 8 of them; everything #966 reported has been live indevelopmentsince. This fixes all five reported items plus one of the same class the issue did not name.Status per item
useChat.ts:703wedges the notification surfaceuseMessageQueue.ts~30 toasts per outageChatContext.tsx:777re-toasts on a loopuseChat.ts:319per-event firetoastwrapper (the issue's trailing suggestion)Item 1 — the priority, and what it turned up
toast.error("MCP sandbox unreachable", { duration: Infinity })fired per SSE payload with no id and no guard. Three of those permanently occupied every visible slot and starved every later toast app-wide, meaning a real outage could silence the notification surface during the exact debugging session you need it in.It is no longer a toast. An unreachable sandbox is a terminal failure of a run the transcript is visibly waiting on — the user pressed Send, the spinner vanished, and no assistant message ever appeared — which is precisely what
RunErrorBannerexists for; thestreamMode === "error"branch fifteen lines above already routes there. AsetRunErrorstate write is also idempotent by construction, so N events collapse to one banner with no id bookkeeping for a future change to get wrong.Found while implementing it: the branch was near-unreachable to begin with. The backend emits
mcp_sandbox_unreachable(agents/__init__.py:353), butconvertEventToLegacyhad no case for it and bothparseEventswitches inlib/utils/fetchStreamReader.tsfell through todefault: return null. On the unified stream path the event was silently dropped and the run simply stopped with no explanation — a worse failure than the one reported, and only the legacy fallback path ever reached the storming toast. The event is now typed inlib/entities/stream.tsand wired through both readers and the converter, so the banner is genuinely reachable.Three supporting corrections came with it. The DLQ Replay button rendered unconditionally while
runError.recoverablewas declared and never read (replaying a run that never reached the DLQ dead-ends).metadatais captured by closure atstartManagedStreamrender time whilerun_idarrives in a later event, so the banner's correlation id was never populated. And the event was missing from both terminal-event sets (streamSource.ts'shasTerminalEventandclearDistributedRecovery) — it survived only because the distributed emitter happens to append a trailingdonethat the sync emitter does not; without that accident the reader retried five times and then took the non-recoveryonErrorbranch, firing a blockingalert()and wiping the user's message behind the new banner.Judgment calls
recoverable: falseand hide Replay. The message tells the user to send again instead. Offering a Replay that POSTs/llm/dlq/<runId>/replayfor a run with no DLQ entry is worse than offering nothing.lastSavedPersistentSignatureRefin thecatch. That is the tempting one-liner and it is a data-loss bug — the ref is also the dirty indicator and the autosave-skip guard, so advancing it marks unsaved text as saved. Autosave backs off exponentially instead (2s → 30s cap) and re-arms a real retry at the deadline: the only other autosave trigger is a signature change, so a user who types, sees the failure and stops typing would otherwise be stranded with no pending timer and no signal (hasUnsavedPersistentChangeshas no UI consumer). Manual save always bypasses the backoff.useScheduleExecutions.ts:40as the precedent for a background loop that declines to toast. Rejected here because unsaved user text is at stake; it earns exactly one.useSchedulesconsumers — the page and the sidebar panel — so one outage stacked a toast each, doubled again under<StrictMode>). Create/update/delete deliberately get none: they answer a click and must fire every time, or two identical saves are indistinguishable.connectionToast.ts, rather than hoisted into a shared barrel that adds an import edge and enforces nothing.pages/memories/edit.tsxfixed although the issue does not list it — a mount effect that toasted and navigated with no id and no guard, giving two of each under<StrictMode>. Same class as item 1 and cheap. The audit reviewed all ~101 sites but changes only these; nothing else surfaced as actively harmful.Scope notes for the reviewer
useAgentSchedules.createSchedule/updateScheduleearly-returned on a falsyagentIdbefore any toast and before any API call, so the panel was reporting success for a write that never happened; deleting the duplicate toast would have made it silent, so the hook now reports and throws. And the components'try/catchblocks are kept —deleteSchedulerethrows, so removing them trades a duplicate toast for an unhandled rejection.errorbranch already accepts that tradeoff.Adversarial review round
The diff was audited before this was marked ready, and the audit found a defect I introduced that was worse than the bug it fixed, plus four smaller ones. All are fixed in
dc6e6787; the findings are recorded here because a green suite is what let the first one through.catch, outside the render that scheduled it, so it could not rely on the autosave effect's cleanup — React only runs the cleanup the most recent effect run returned, and that effect returns early with no cleanup on four branches (unauthenticated, skip counter, unchanged signature, streaming). A timer armed after one of those kept PATCHing an unmounted tree with a stale auth token until page reload. Fixed with a mount-scoped teardown and a live token re-read. The new test reproduces the exact escape path rather than a convenient one — the naive version (edit → fail → unmount) passes pre-fix, because there the autosave effect's own cleanup is still the most recent one.RunError.titlewas dead code — no call site set it, yet two tests asserted a field the app cannot produce. Removed rather than wired up.Failed to load schedulevsFailed to load schedulesdiffered by one character on the same screen; the edit-load failure now readsFailed to open schedule.Findings deliberately not acted on:
metadataRef.current = metadatais a write during render (idempotent, and matches existing practice inuseMessageQueue.ts), and the StrictMode double-invoke motivation cited for two of the stable ids is development-only — those fixes stand on their other merits (two concurrentuseSchedulesconsumers in production).Verification
npm run test && npx tsc -b && npm run lint && npm run build— 468 passed, 3 skipped; typecheck and build clean; the 3 lint warnings are pre-existing in files this PR does not touch. Notenpx tsc --noEmitas written in the issue's suggested command is a no-op here —frontend/tsconfig.jsonis solution-style with"files": [], so it compiles zero files and exits 0 regardless;tsc -bis the real gate.Tests pin the anti-storm invariant, not merely that a toast fires — an assertion that
toast.errorwas called passes before and after these fixes. Each new suite was run against the pre-fix code to confirm it actually fails there:mcp_sandbox_unreachablepayloads → zerotoast.errorcalls and exactly one run-error state (pre-fix: 5 toasts, no banner).errorevents → 1 toast; a new stream erroring afterwards still toasts, proving the latch is per-stream rather than global.onErrorfires its informational toast without the recovery id — sonner merges by id, so sharing it would have overwritten the error text with a reassurance.undefined, which is one distinct value, so it passes pre-fix.)patchDefaultsrejecting across 4 successive edits → 1 PATCH and 1 toast; the PATCH count is what pins the real bug. Dirty state still reports dirty, guarding against the forbidden fix.<StrictMode>mount with a rejecting memory fetch → 1 toast and 1 navigate, with the effect confirmed to have run twice.Where a test is a regression guard rather than a falsifier it is marked as such — the schedules page's delete handler and the manual-save bypass both already behaved correctly.
CI verifies the same four commands plus the backend suite. Not verified here: the playwright e2e suite, which CI runs only on manual dispatch. It covers the DLQ replay button this PR now gates, so a vitest test pinning that
recoverable: truewith a truthyrunIdstill renders the button was added specifically to guard that path without playwright.Summary by CodeRabbit
Bug Fixes
Tests