Skip to content

fix(chat): route error states through ErrorNotice (batch chat-2) - #8743

Merged
buluoray merged 1 commit into
mainfrom
fix/error-notice-chat-2
Sep 5, 2026
Merged

fix(chat): route error states through ErrorNotice (batch chat-2)#8743
buluoray merged 1 commit into
mainfrom
fix/error-notice-chat-2

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

The dashboard error-state audit (research/error-states-audit.md, workflow run wf_000025, six shard scans + critic pass) found 320 error surfaces bypassing the shared ErrorNotice and 197 mutations/queries failing silently, every one a violation of the blocking AUTOSDE rule errors-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 under website/src/pages/chat/, following each row's recommended_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) that ErrorNotice recovers 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)

askAgent is 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: ChatPage persists the composer draft per slot and an in-chat hand-off opens a fresh slot without navigating away, so a No hand-off: composer draft comment 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 → ErrorNotice

File Before After
SessionSummaryTab.tsx load failure Centered AlertTriangle text-danger + title + body + Retry block notice (title + message) + Retry kept beside it (retry vs hand-off split); askAgent on
SessionSummaryTab.tsx generate failure text-warn div — an error dressed as a warning inline notice, askAgent on (inputs are the already-persisted session)
SideChat.tsx displayError text-danger div inline notice, askAgent OFF + No hand-off comment naming the side-chat composer draft (onError merges the failed question back into it). The localNotice status line beside it is untouched (not an error, per the report)
WorkflowProgressBar.tsx run.error text-danger line inside the toggle <button> inline notice moved out of the button (the hand-off is itself a button; button-in-button is invalid markup); askAgent on
WorkflowProgressBar.tsx snapshotError text-red-500 border-red-500/30 box, label + reason concatenated block notice with the label as title and the backend reason as message (so the reason stays the journal lookup key); literal palette gone
WorkflowSidebarRow.tsx row.error / snapshotError same two shapes as above, text-red-500 same two fixes; the failed-status icon also moves from text-red-500 to the text-danger token
WorkflowRunCard.tsx run.error text-danger line inside the open-panel Btn inline notice moved out of the Btn; askAgent on (the save form lives only inside the Modal)
WorkflowRunCard.tsx save Modal ×2 <p className="text-danger"> for sourceError / saveDefinition.error inline notices, askAgent OFF + No hand-off comment 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.allSettled rejections now counted) share one dismissable actionError notice 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-false is 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 danger X + "Copy failed" label for 1.5 s instead of staying unchanged — same shape chat-1 gave AssistantMessage.
  • share/ShareMessageModal.tsx: handleDownload / handleCopy / openIntent used try/finally with no catch, so a thrown html-to-image import or toBlob became an unhandled rejection. Each now sets exportError → inline notice, askAgent OFF + No hand-off comment (caption textarea + edited card text). openIntent additionally 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.tsx stop_failed_reset: kept as is, with a code comment recording the exemption on the same grounds as ErrorCard'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 to ErrorCard (app-sdk stop_event renderer duplicates StopEventCard's stopping/stopped recipe class-for-class #6229, AppSdkStopEventCardParity.test.tsx).
  • SidePanel.tsx ['session-summary'] probe and SideChat.tsx refreshMutation / SessionSummaryTab post-generate refetch: 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 from KnowledgePicker; deliberately not touched here to avoid a conflict.

Deliberately not done

  • website/AUTOSDE.yaml untouched (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 no catch on 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 coloured text-warn (SessionSummaryTab generate).
  • Cross-shard pointer (report row WorkflowRunTree.tsx:202): the expanded tree renders the same run.error a fourth time via i18nT('…failed_with_error'). That file is in the apps shard and is left for that batch.
  • pages/ChatPage.tsx untouched (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-XA regenerated. 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

  • Per the owner's standing instruction no test suite was run locally; verification is on CI. Existing coverage that pins the new shape without edits: 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).
  • Local gates run clean: tsc -b, eslint on every touched file, the full i18n-check.mjs chain (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.mjs mounts the real WorkflowSidebarRow, SessionSummaryTab and SubagentProgressBar with fetch stubbed to reject, and presses the chip's per-row Stop so the refused spawnDelete actually lands. The runner asserts the after frames render one "Ask the agent" per scene, the stop-refused notice is present, and no text-red-500 remains; the before frame (same harness against the base tree via git 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.

before — hand-written surfaces, refused stop invisible

after — ErrorNotice with hand-off, refused stop reported

After, light theme

after — light

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 no catch (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

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality (CI; existing tests pin the new shape — see Tests)
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no user docs describe these surfaces
  • No secrets, credentials, or internal references in the diff

@CrysisDeu
CrysisDeu requested a review from a team September 5, 2026 12:33
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 5, 2026 12:33
@CrysisDeu
CrysisDeu requested a review from patrigao September 5, 2026 12:33
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 49b2a20994023678231ccbe38056129864970299 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • The SessionColorPicker generation-counter + confirmed-ref machinery is the most intricate piece here; if a second surface ever needs optimistic-save-with-rollback, extract it as a hook rather than copying the refs.

[DESIGN-REVIEWED] 49b2a20

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 49b2a20994023678231ccbe38056129864970299 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • retry_failed_error "Couldn't retry some of the failed subagents": the allSettled results hold the exact rejection count (and the trigger button already says "Retry failed ({{count}})"), yet the notice says "some" even when all of them refused — count the rejections and interpolate: "Couldn't retry {{count}} subagents".
  • SessionSummaryTab failure (visible in after-dark.png): the block notice runs title into message as "Could not load the summary Try again, or reload the page if it keeps failing" while a "Try again" button sits directly beneath — the message half now restates the adjacent button; trim sessionSummary.failed to the reload fallback only (or drop it) so the box carries the fact and the button carries the action.
  • UserMessage copy buttons: on failure the title tooltip still reads "Copy" while the icon shows a red X and the aria-label says "Copy failed" — route title through the same copyOutcomeLabel as aria-label so hover text matches the state.

[UX-REVIEWED] 49b2a20

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @CrysisDeu overrides the GPT 5.6 finding for 49b2a20994023678231ccbe38056129864970299; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt 49b2a20994023678231ccbe38056129864970299: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 49b2a20994023678231ccbe38056129864970299 — this comment is updated in place on each push.

Review details

Both candidates target the generation-counter logic in SessionColorPicker.tsx. Let me verify my falsification of each against the actual behavior.

For Candidate 2, I confirmed sseSlotColor is dispatched only by client code (the picker and SessionColorSwatches), never from a server-push handler. The colorIndex prop is fed from store.slots, which the server refreshes via sseSlots snapshots (useWebSocket.ts:973). So a "concurrent server-pushed colour" is only reachable if a different client changes the colour while this client's write is in flight AND this client's write then fails — and the resulting wrong colour is transient, corrected by the very next sseSlots snapshot (not "next reload"). This requires a multi-client concurrent-edit race with a coincident failure of the local write, and the wrong outcome self-heals. That is an "if a caller were to" condition, not one that occurs in practice; it does not clear the bar.

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 writeGen to 0), the user reopens and picks C (reusing gen=1) while A's request is still in flight, and then A settles as a rejection while C succeeds. Each step is individually possible but the compound ordering (a later-issued request completing before an earlier one that then fails, straddling a popover reopen) is a "might occur" race, and its worst outcome is a spurious error notice plus a transiently wrong dot that self-corrects on the next snapshot/reload. No data loss (the server holds C), no crash, no security boundary. Below the 80 bar and below "occurs in practice."

Neither survives Step 1. Scanning the rest of the diff (ErrorNotice migrations, ShareMessageModal export-error handling, UserMessage copy-outcome, i18n key additions), I find nothing new that grounds all of (a)/(b)/(c) at 80+; the documented askAgent off/on choices and the StopEventCard/file-exists-query exemptions match the errors-use-error-notice rule's own carve-outs.

No findings.

[OPUS-REVIEWED] 49b2a20

Verdict parsed from the review's SHA-scoped output markers for commit 49b2a20994023678231ccbe38056129864970299.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 49b2a20994023678231ccbe38056129864970299: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 49b2a20994023678231ccbe38056129864970299 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. The errors-use-error-notice rule is real and blocking: true (website/AUTOSDE.yaml:526); ErrorNotice's contract matches every use (falsy message renders nothing, opt-in askAgent documented as the safety default); the capture harness + committed screenshots are an established repo convention (195 files in website/capture/, 923 under temp-screenshots/); chat-1 (#8730) is not in this base, which explains — rather than contradicts — the description's cross-references. Remaining swallowed rejections in the same directory are counted below.

First-Principles-Verdict: PASS

Every surface migrated here is mandated by the blocking errors-use-error-notice rule, and each silent failure fixed names a real observed dead end.

What this change ships

Intent: 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).

  1. Session-summary load failure becomes a notice with agent hand-off, Retry kept — justified
  2. Session-summary generate failure stops dressing as a warning — justified
  3. Side-chat send error becomes a notice, hand-off off (draft at risk) — justified
  4. Workflow failure lines move out of their toggle buttons into notices — justified (button-in-button is a platform rule)
  5. Snapshot-load failures become titled notices, literal red palette dropped — justified
  6. Refused session-colour save now rolls back and reports instead of silently reverting on reload — justified
  7. Refused subagent Stop / Stop-all / Retry now surface on the chip — justified
  8. Refused End-wait shows a notice instead of a button that "did nothing" — justified
  9. Copy / Copy-link flash a failure icon for 1.5s — justified (per-rule exemption recorded)
  10. Failed share-card export reports, closes the pre-opened tab, skips the caption-only post — justified
    More than 10 items exist (capture harness, i18n keys ×12 locales, exemption comments); these are the repo's standard evidence and catalog conventions.

Watch

  • Same root cause, 4 unfixed siblings in this directory (grepped .catch(() => {}) under src/pages/chat/): ActivityViewer.tsx:158,837 swallow the identical spawnDelete refusal fixed in SubagentProgressBar, and PinnedMessagesPanel.tsx:123,131 swallow the copy refusal fixed in UserMessage. The description's leftover list names only WorkflowRunTree and ChatPage — confirm these four sit in another batch's rows, else they fall through the audit.

[FIRST-PRINCIPLES-REVIEWED] 49b2a20

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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.

@CrysisDeu
CrysisDeu force-pushed the fix/error-notice-chat-2 branch from 5dd20da to 9973201 Compare September 5, 2026 12:47
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

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 main rather than that branch:

  • ActivityViewer.tsx:158 — refused spawnDelete on the Subagents-tab Cancel → chat-1 replaces the .catch(() => {}) with setActionError(cancel_failed) rendered through an inline ErrorNotice askAgent (and does the same for its Dismiss-done allSettled).
  • PinnedMessagesPanel.tsx:123,131 — swallowed copy / copy-link → chat-1 flips the icon to a danger X with a pins.copy_failed label, the same shape this PR gives UserMessage.

Both are listed in #8730's body ("Silent failures surfaced" and the AssistantMessage / PinnedMessagesPanel bullet). Once both PRs land, grep '\.catch(() => {})' website/src/pages/chat/ leaves only background polls and documented-intentional swallows (the 30 s reconcile poll here, SideChat's refreshMutation, SessionSummaryTab's post-generate refetch), each with a comment saying why.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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).

@CrysisDeu
CrysisDeu force-pushed the fix/error-notice-chat-2 branch from 9973201 to 4317933 Compare September 5, 2026 13:05
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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).

@CrysisDeu
CrysisDeu force-pushed the fix/error-notice-chat-2 branch from 4317933 to 49b2a20 Compare September 5, 2026 13:22
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 49b2a20994023678231ccbe38056129864970299.

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.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants