fix(chat): route error states through ErrorNotice (batch chat-2) - #8743
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of The diff matches the description faithfully across all twelve files. Design assessment complete. Design-Verdict: PASS A rule-driven sweep executed batch-by-batch, with every hand-off/no-hand-off and exemption decision justified per surface and rule-gap findings routed to the PR that owns the rule. Suggestions
[DESIGN-REVIEWED] 49b2a20 |
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Previously swallowed failures now surface at the point of action with rollback, dismissal, and correctly reasoned hand-off decisions; remaining issues are copy-polish only. Suggestions
[UX-REVIEWED] 49b2a20 |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @CrysisDeu overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates target the generation-counter logic in For Candidate 2, I confirmed For Candidate 1, the reset-to-0 ABA is a genuine logic quirk, but its trigger requires a very specific interleaving: pick A and pick B while the popover is open, B's request succeeds first (closing the popover and resetting Neither survives Step 1. Scanning the rest of the diff (ErrorNotice migrations, No findings. [OPUS-REVIEWED] 49b2a20 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified. The First-Principles-Verdict: PASS Every surface migrated here is mandated by the blocking What this change shipsIntent: make chat-page failures visible and recoverable through the shared error surface, per the audit and the blocking AUTOSDE rule. This is a FIX (of rule violations and swallowed rejections).
Watch
[FIRST-PRINCIPLES-REVIEWED] 49b2a20 |
|
/ai-review override gpt 9973201: The real half (copyToClipboard resolving false shown as Copied) is fixed at this head — both handlers now read the boolean. The remaining half asks for an ErrorNotice on a clipboard refusal; the audit this PR implements (research/error-states-audit.md §3, rows AssistantMessage:457 / UserMessage:289,299 / PinnedMessagesPanel:123,131) explicitly classifies it "not an ErrorNotice candidate": a browser permission has no journal context to recover and no hand-off can fix it, so the rule's purpose does not apply. The 1.5s icon flash is the same idiom as the Copied checkmark, and identical to what chat-1 (#8730) shipped in AssistantMessage. |
5dd20da to
9973201
Compare
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
|
First Principles — disposition (Watch item, advisory) The three swallowed user-press failures named as unowned are owned by the sibling batch chat-1 (#8730), which is open in parallel and was grepped against
Both are listed in #8730's body ("Silent failures surfaced" and the |
|
/ai-review override gpt 4317933: Same head as 9973201 rebased onto the fixed main (c791f0f, #8712) — no code change. The real half of F1 (copyToClipboard resolving false shown as Copied) is fixed: both handlers read the boolean. The remaining half asks for an ErrorNotice on a clipboard refusal; the audit this PR implements (research/error-states-audit.md §3, rows AssistantMessage:457 / UserMessage:289,299 / PinnedMessagesPanel:123,131) classifies it "not an ErrorNotice candidate": a browser permission has no journal context and no hand-off can fix it. The 1.5s icon flash is the same idiom as the Copied checkmark and matches chat-1 (#8730). |
9973201 to
4317933
Compare
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
|
/ai-review override gpt 49b2a20: Both blocking findings on 4317933 are FIXED at this head — exportBlob now throws on a null toBlob result (every caller's catch fires; openIntent closes the pre-opened tab and does not navigate), and SessionColorPicker gates close/rollback/report on a write generation so a stale failed save cannot overwrite a newer persisted colour. This override covers only the carried-over UserMessage clipboard-icon finding from 5dd20da: the audit (research/error-states-audit.md §3, rows AssistantMessage:457 / UserMessage:289,299) classifies a clipboard refusal as not an ErrorNotice candidate — no journal context, no hand-off can fix a browser permission; the icon flash matches chat-1 (#8730). |
4317933 to
49b2a20
Compare
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
|
/ai-review override gpt 49b2a20: Clipboard refusal is a browser permission, not a request failure: the error journal holds nothing for ErrorNotice to recover and no agent hand-off can grant clipboard access. The audit this PR implements (research/error-states-audit.md §3, rows AssistantMessage:457 / UserMessage:289,299) classifies these as not-ErrorNotice candidates; the 1.5s icon flash is the same idiom chat-1 (#8730) shipped in AssistantMessage. The false-as-Copied half was fixed at 9973201. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Problem / Motivation
The dashboard error-state audit (
research/error-states-audit.md, workflow runwf_000025, six shard scans + critic pass) found 320 error surfaces bypassing the sharedErrorNoticeand 197 mutations/queries failing silently, every one a violation of the blocking AUTOSDE ruleerrors-use-error-notice(website/AUTOSDE.yaml). The report recommends fixing by surface in batches of ≤25 files. This PR is §5.2, batch chat-2: 12 files underwebsite/src/pages/chat/, following each row'srecommended_action.No linked issue: driven by the audit report.
Why it matters
This is an AI agent app: an error the user cannot fix is usually one the agent can, so a hand-written red line is a dead end — it throws away the structured context (route, endpoint, HTTP status, backend
code) thatErrorNoticerecovers from the error journal and offers to the agent. A swallowed rejection is worse: a refused Stop on a subagent left the row spinning as if the press never happened, a failed session-colour save kept a colour the server never stored until the next reload silently reverted it, and a thrown share-card export un-busied the button and did nothing else.What changed (motivation → approach → change)
askAgentis on wherever the surface holds no unsaved draft (status rows, side-panel reads, the wave chip). Where the critic's finding was "the composer draft is at risk", it was verified in code before adopting:ChatPagepersists the composer draft per slot and an in-chat hand-off opens a fresh slot without navigating away, so aNo hand-off: composer draftcomment would name a draft that is not at risk (the rule forbids that). Each notice carries the reasoning as a code comment.Hand-written surfaces migrated →
ErrorNoticeSessionSummaryTab.tsxload failureAlertTriangle text-danger+ title + body + Retrytitle+message) + Retry kept beside it (retry vs hand-off split); askAgent onSessionSummaryTab.tsxgenerate failuretext-warndiv — an error dressed as a warningSideChat.tsxdisplayErrortext-dangerdivNo hand-offcomment naming the side-chat composer draft (onError merges the failed question back into it). ThelocalNoticestatus line beside it is untouched (not an error, per the report)WorkflowProgressBar.tsxrun.errortext-dangerline inside the toggle<button>WorkflowProgressBar.tsxsnapshotErrortext-red-500 border-red-500/30box, label + reason concatenatedtitleand the backend reason asmessage(so the reason stays the journal lookup key); literal palette goneWorkflowSidebarRow.tsxrow.error/snapshotErrortext-red-500text-red-500to thetext-dangertokenWorkflowRunCard.tsxrun.errortext-dangerline inside the open-panelBtnBtn; askAgent on (the save form lives only inside the Modal)WorkflowRunCard.tsxsave Modal ×2<p className="text-danger">forsourceError/saveDefinition.errorNo hand-offcomment naming the save-definition form (name, slug, description)Silent failures surfaced
SessionColorPicker.tsx:api.setSlotColor(...).catch(() => {})→ the optimistic store colour is rolled back to what the server kept, the popover stays open with an inline notice (askAgent on), and closes only once a save lands.SubagentProgressBar.tsx×3: per-row Stop (spawnDelete), Stop all (spawnStopAll) and Retry-failed (Promise.allSettledrejections now counted) share one dismissableactionErrornotice under the chip header, askAgent on. The 30 s reconcile poll's.catch(() => {})is kept and documented as intentional (a background poll that only ever removes phantom cards).ToolCallLine.tsx: a refused End-wait now renders an inline notice beside the button instead of only re-enabling it; the HEAD-probe's collapse-to-falseis documented as a deliberate affordance gate (not a missed error state).UserMessage.tsx×2: per the report not ErrorNotice candidates (clipboard refusal has no journal context). Copy / Copy-link now flip to a dangerX+ "Copy failed" label for 1.5 s instead of staying unchanged — same shape chat-1 gaveAssistantMessage.share/ShareMessageModal.tsx:handleDownload/handleCopy/openIntentusedtry/finallywith nocatch, so a thrownhtml-to-imageimport ortoBlobbecame an unhandled rejection. Each now setsexportError→ inline notice, askAgent OFF +No hand-offcomment (caption textarea + edited card text).openIntentadditionally closes the pre-opened tab and does not navigate on a failed export — the card never reached the clipboard, so opening the composer would be exactly the caption-only post the auto-copy exists to prevent.Decisions recorded, no code change
StopEventCard.tsxstop_failed_reset: kept as is, with a code comment recording the exemption on the same grounds asErrorCard's in the rule — a transcript row (hand-off would be circular) rendering a fixed localized status label (nothing for the journal to recover), whose class recipe is already pinned byte-for-byte toErrorCard(app-sdk stop_event renderer duplicates StopEventCard's stopping/stopped recipe class-for-class #6229,AppSdkStopEventCardParity.test.tsx).SidePanel.tsx['session-summary']probe andSideChat.tsxrefreshMutation/SessionSummaryTabpost-generaterefetch: documented-intentional in the report; left alone.useKnowledgeFetch.ts(listed in this batch): already fixed in chat-1 (fix(chat): route error states through ErrorNotice (batch chat-1) #8730) because it was inseparable fromKnowledgePicker; deliberately not touched here to avoid a conflict.Deliberately not done
website/AUTOSDE.yamluntouched (settings-1 owns the rule-text augmentation). Rule-gap variants found here, for that PR: (4.1) error rendered inside a<button>— the rule's "hand-written red div" ban reads as a styling rule, but the structural blocker is that a compliant notice cannot live inside an interactive parent (WorkflowProgressBar / WorkflowSidebarRow / WorkflowRunCard all had this); (4.6)try { … } finally { … }with nocatchon an async handler — a throw becomes an unhandled rejection and the button just un-busies (ShareMessageModal ×3); (4.6) optimistic dispatch +.catch(() => {})with no rollback (SessionColorPicker); (4.3) an error colouredtext-warn(SessionSummaryTab generate).WorkflowRunTree.tsx:202): the expanded tree renders the samerun.errora fourth time viai18nT('…failed_with_error'). That file is in the apps shard and is left for that batch.pages/ChatPage.tsxuntouched (pages-rest batch).i18n: 8 new keys in
en.manual.json+ 11 translations (zh-CN, ja, ko, de, fr, es, it, pt, ru, hi, bn — curly apostrophes for fr/it, formal Hindi),en-XAregenerated. Wherever the site already had a string it is passed unchanged (failed_title/failed,queue_*_failed,source_unavailable,request_failed,could_not_load_*) so the journal message-match keeps working. The retry message is deliberately count-free so it needs no plural forms.Tests
sessionSummaryTab.test.tsx("reports a load failure with a way to recover" — title text, body regex, Retry that refetches; "already being written" generate refusal),SessionColorPickerCov80.test.tsx(persists then closes the popover — still true, close now follows the resolved save),SubagentProgressBar*.test.tsx(Stop / Stop all wiring, localized retry label),AppSdkStopEventCardParity.test.tsx+StopEvent.integration.test.tsx(unchanged card recipe and testids),SideChat.steerQueue.test.tsx(failed edit merges back into the composer).tsc -b,eslinton every touched file, the fulli18n-check.mjschain (added-lines 0, source-strings 8 new / 0 badly shaped, key-refs 0 dangling, pseudolocale in sync),check-phantom-classes.mjs,check-theme-colors.mjs.Manual verification
Capture harness
website/capture/error-notice-chat-2.{html,tsx}+website/scripts/capture-error-notice-chat-2.mjsmounts the realWorkflowSidebarRow,SessionSummaryTabandSubagentProgressBarwithfetchstubbed to reject, and presses the chip's per-row Stop so the refusedspawnDeleteactually lands. The runner asserts the after frames render one "Ask the agent" per scene, the stop-refused notice is present, and notext-red-500remains; the before frame (same harness against the base tree viagit stash) asserts zero hand-offs and no notice after the press — a frame cannot photograph the wrong tree.Screenshots / video
Before (base) → after (this PR), dark. Note the third scene: on base the refused Stop leaves the chip unchanged.
After, light theme
Related Issues
Batch chat-2 of the error-state sweep (
research/error-states-audit.md§5.2). Sibling batches: settings-1 (owns the AUTOSDE rule text), chat-1 (#8730), pages-rest.Pattern harvest
Rule candidate: semgrep
Pattern: an async event handler with
try { … } finally { … }and nocatch(the throw becomes an unhandled rejection and the UI only un-busies), and an error string rendered as a descendant of<button>(a compliant notice cannot be mounted there). Both recorded for the settings-1 rule-text PR.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)