Skip to content

fix(dashboard): surface steer and plan-export failures via ErrorNotice (#8625) - #8764

Closed
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/onerror-error-notice-8625
Closed

fix(dashboard): surface steer and plan-export failures via ErrorNotice (#8625)#8764
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/onerror-error-notice-8625

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Two user-initiated mutations reported their failures only to console.error, so a rejected press produced no on-screen response at all:

  • ChatPage.tsx: the mid-turn steer mutation's onError only logged. Steer clears the composer before the POST settles, so on a rejection the text was gone, the optimistic "steered" bubble stayed in the transcript asserting delivery, and DevTools held the only trace.
  • ProjectDetailPage.tsx: the plan-export (Export YAML) mutation's onError only logged, so a refused export read as "the click did nothing".

Both are the errors-use-error-notice defect class (website/AUTOSDE.yaml, blocking rule): an error the user cannot see is one they cannot react to, and the ErrorNotice agent hand-off is unavailable when the failure only hits the console.

Why it matters

This is an AI agent app: an error the user cannot fix themselves is usually one the agent can. A silently rejected steer is worse than most - the user's message is already discarded, so they wait on an instruction the agent never received. A silent export failure makes the button look broken and invites repeated clicking.

What changed (motivation -> approach -> change)

Follows the migration pattern PR #8547 established for the sibling ChatSidebar handler: resolve the journal report from the raw error text, render through the shared ErrorNotice, and wire askAgent only where the hand-off cannot destroy unsaved state.

  • ChatPage steer: the rejection now lands on the existing refused-press surface above the composer (the surface's own doc says a new press inherits it by adding a title key and calling showRefusedPress). New action steer with catalog key pages.chatPage.could_not_steer. showRefusedPress now resolves the journal report from the raw error text at capture time, and the surface renders report + askAgent - safe here because the hand-off stages a prompt for a fresh session and per-slot composer drafts persist across the switch.
  • Steer settle correctness (shaped by pre-push review rounds and the live review lanes): steers are independent messages, not retries, so a same-slot rejection always reports. Per-slot monotonic attempt counters order successes against refusals: a success retires only a refusal raised by an earlier-or-same attempt in its own slot. Only a DEFINITIVE refusal (an ApiError below 500) rolls back: the optimistic bubble is spliced (new failed outcome on resolveOptimisticSteer) and the text is restored durably into the origin slot's persisted draft BEFORE the splice, so no window exists where both copies are gone - including a rejection landing while ChatPage unmounts. A transport failure or gateway 5xx leaves delivery UNKNOWN: the bubble stays for WS/history reconciliation, nothing is restored (a restore invites a double-executing resend), and the notice uses a distinct title ("Steer may not have arrived") instead of asserting refusal. NEW RETENTION SEMANTICS, declared explicitly: a slot-scoped steer refusal that lands while the user is on another slot is RETAINED per origin slot (refusedPressBySlotRef) and re-surfaced by the slot-switch effect when they return - the old clear-on-switch behavior applies only to the un-scoped busy-state presses (continue/regenerate/switch-variant), which are also the only ones a turn starting retires. Scoped refusals are retired by dismiss, supersession, or an attempt-ordered steer success.
  • ProjectDetailPage export: the mutation error renders as a block ErrorNotice under the tab bar (data-testid="plan-export-error"), title lead + raw reason, dismiss = mutation.reset(). No askAgent, with a comment saying why: the page holds unsaved plan edits (pendingEdits) and the hand-off unmounts them. The mutation takes taskId as its variable and the render is gated on variables === run.task_id, so a failure can never be attributed to a different run; a reset effect on run switch prevents a stale error resurfacing later.
  • api client: exportPlanYaml now throws via the shared apiFailure chokepoint instead of hand-rolling an ApiError, so the {"error": ...} JSON envelope is unwrapped into prose and the failure is journaled like every other dashboard API error.
  • i18n: two new keys (pages.chatPage.could_not_steer, pages.projectDetailPage.export_failed) in en.manual.json and all 11 locales; en-XA regenerated. Keys are new, so they cannot collide with open PR fix(sidebar): surface folder-create failures inline (#8229) #8547's pages.chatSidebar.* keys.

Deliberately not touched: ChatSidebar.tsx and the folder-create path (owned by open PR #8547), and other console.error sites not named by the issue.

Tests

  • website/src/test/ChatPage.refusedPress.test.tsx: a rejected steer renders the notice with the per-action title, the server reason, and the agent hand-off; the optimistic bubble is spliced; a stale success does not clear a newer steer refusal (deferred concurrent-steer ordering).
  • website/src/test/ProjectDetailPage.test.tsx: a rejected export renders the alert with the action lead and raw reason, offers no hand-off, and dismisses; catalog fallback when the rejection carries no message; the error does not survive a run switch.
  • Full battery run locally: npx tsc -b clean; touched + related suites green (refused-press 6/6, ProjectDetailPage 12, steer receipt, ChatSlice coverage, ApiClient coverage, i18n battery 662/662); eslint clean on touched files.

Manual verification

website/scripts/capture-onerror-error-notice.mjs runs the real built SPA against a stubbed gateway, forces both rejections, asserts 10 properties (notice rendered, titled, alert role, hand-off present/absent as designed, dismiss works, JSON envelope unwrapped), and exits non-zero on failure. All 10 pass at this head.

Screenshots / video

Captured by the harness at this head, under temp-screenshots/onerror-error-notice-8625/:

  • 01-steer-refused-notice-above-composer.png - rejected steer: "Couldn't steer" + server reason + Ask the agent, directly above the composer
  • 02-steer-notice-dismissed.png - dismissed
  • 03-plan-export-error-below-tab-bar.png - rejected export: "Could not export the plan YAML. no plan to export" banner under the tab bar
  • 04-plan-export-notice-dismissed.png - dismissed

Related Issues

Closes #8625

Pattern harvest

Rule candidate: review-prompt
Pattern: mutation onError that only console.errors a user-initiated action - flag any onError whose body is only logging in files that also import ErrorNotice or render user-facing state.

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
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A real silent-failure defect class, fixed at the right surfaces with the delivery-ambiguity (definitive vs unknown) split correctly designed, declared, and tested.

Suggestions

  • The refused-press surface now runs two lifecycle regimes (slot-scoped retained vs unscoped clear-on-switch) distinguished only by scope presence and comments; a small named set or type distinction for scoped actions would keep the next press type from silently inheriting the wrong retirement rule.

[DESIGN-REVIEWED] 87da3bd

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

Silent failures now surface honestly: notices name the action, restore the user's text, and hedge only where delivery is genuinely unknown.

Suggestions

  • could_not_steer ("your message is back in the composer") overpromises for a steer that carried attachments — the diff comment confirms attachments are still discarded on rollback, so a resend silently drops the file; either restore attachments or scope the string to "your text".
  • The steer_may_not_have_arrived alert is retired only by steerMutation.onSuccess or manual dismiss — when the WS steer_push echo later confirms the optimistic bubble, a red "may not have arrived" keeps sitting beside a transcript that answers it; retire the steer_unknown notice for that attempt on echo confirmation.

[UX-REVIEWED] 87da3bd

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for 87da3bde19191907cc91cc6a56bbca777475f078; 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 87da3bde19191907cc91cc6a56bbca777475f078: <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 87da3bde19191907cc91cc6a56bbca777475f078 — 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.

First-Principles-Verdict: PASS

Two silent failure paths now answer the press that caused them, entirely through mechanisms the repo already had: ErrorNotice, the refused-press surface, apiFailure, findReport.

What this change ships

Intent: make a rejected mid-turn steer and a rejected plan export visible on screen instead of dying in DevTools — a FIX (defect class errors-use-error-notice, website/AUTOSDE.yaml:526, blocking).

  1. Rejected steer shows "Couldn't steer" + server reason above the composer — justified (the fix; blocking rule)
  2. Definitive refusal restores the cleared text into the slot draft and removes the optimistic bubble — justified (message was destroyed before)
  3. Transport/5xx failure gets a distinct "may not have arrived" title, bubble kept, no restore — justified (double-send is the counted alternative harm)
  4. Background-slot steer refusal retained and re-shown on return — declared; justified by the rule
  5. A steer success retires only earlier-or-same-attempt refusals in its slot — justified (without it, a resend leaves a now-false notice, or a stale success clears a newer one)
  6. Continue/regenerate/switch-variant notices now also carry askAgent + journal report — rides along on the shared render site; mandated by the rule's askAgent clause, same draft-persistence safety
  7. Failed Export YAML shows a dismissible alert under the tab bar, no hand-off — justified (the fix; pendingEdits loss named)
  8. Export error is prose, journaled via existing apiFailure (client.ts:1480) — justified; deletes a hand-rolled ApiError
  9. Three notice strings across en + 11 locales + en-XA — justified (i18n gate); description says "two new keys" but the third is declared in prose
  10. Capture harness + 4 committed screenshots — convention (100+ sibling capture-*.mjs scripts; temp-screenshots/ holds dozens of committed dirs)

Sibling check for the defect class ("onError whose body only logs"): grepped onError across website/src — after this PR the only remaining silent site is the ChatSidebar folder-create path, which the description defers to open PR #8547; ChatSidebar's other handlers and ChatInput's optimizer already surface state. No unfixed siblings left unclaimed. The pattern-harvest rule proposal even aims at the cause level for the class.

[FIRST-PRINCIPLES-REVIEWED] 87da3bd

@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 87da3bde19191907cc91cc6a56bbca777475f078 — this comment is updated in place on each push.

Review details

Both candidates are mechanically reachable (verified: regenerate/continue/switch_variant call showRefusedPress un-scoped, steer scoped), so I examined whether each produces a clearly-wrong outcome at the 80+ bar.

Candidate 1 (steer success clears a steer_unknown): The retirement condition held.action === 'steer' || held.action === 'steer_unknown' is a deliberate authored inclusion, documented in the onSuccess comment. For a steer_unknown, the optimistic bubble for message M deliberately stays on screen (that's the whole point of the steer_unknown branch), so the maybe-loss is not made invisible — only the accompanying prompt is cleared. Whether that is wrong versus an intended simplification (channel proven working → drop the notice, bubble carries reconciliation) is a UX judgment, not a defect. Confidence < 80.

Candidate 2 (dismissing an un-scoped notice orphans a retained scoped steer refusal that re-surfaces on a slot round-trip): The steer refusal that re-surfaces was never itself dismissed — the user dismissed a different (regenerate/continue) notice that transiently overwrote the display. Re-surfacing a genuinely un-dismissed slot-scoped refusal on return to its slot is precisely the retention feature's documented purpose, and the slotRunning effect intentionally preserves scoped refusals (prev && !prev.slot). This is the design working, not a clear wrong outcome. Confidence < 80.

Neither is a crash, data loss, security hole, or removed guard; the design explicitly preserves the user's text on failure. No grounded new finding survives falsification.

No findings.

[OPUS-REVIEWED] 87da3bd

Verdict parsed from the review's SHA-scoped output markers for commit 87da3bde19191907cc91cc6a56bbca777475f078.

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

@chenmingwei23
chenmingwei23 force-pushed the fix/onerror-error-notice-8625 branch from 84cc5d2 to e35f776 Compare September 5, 2026 15:14
@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
@chenmingwei23
chenmingwei23 force-pushed the fix/onerror-error-notice-8625 branch from e35f776 to 9e00942 Compare September 5, 2026 15:49
@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
@chenmingwei23
chenmingwei23 force-pushed the fix/onerror-error-notice-8625 branch from 9e00942 to 9ec2579 Compare September 5, 2026 16:13
@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
@chenmingwei23
chenmingwei23 force-pushed the fix/onerror-error-notice-8625 branch from 9ec2579 to c3d9d69 Compare September 5, 2026 16:48
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/onerror-error-notice-8625 branch from c3d9d69 to 87da3bd Compare September 5, 2026 17:24
@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: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 5, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt 87da3bd: Zero delta vs base: main discards a rejected steer's text unconditionally; this head keeps it in the composer, in-memory drafts, and persisted drafts, losing it only when localStorage is full/disabled AND the page reloads - a case main also loses. The prescribed remedy (retain the optimistic bubble) is Redux state that dies on the same reload, so it adds no durability. The localStorage swallow is pre-existing chatDrafts behavior pinned by chatDrafts.test.ts; residual filed separately.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 87da3bde19191907cc91cc6a56bbca777475f078.

Zero delta vs base: main discards a rejected steer's text unconditionally; this head keeps it in the composer, in-memory drafts, and persisted drafts, losing it only when localStorage is full/disabled AND the page reloads - a case main also loses. The prescribed remedy (retain the optimistic bubble) is Redux state that dies on the same reload, so it adds no durability. The localStorage swallow is pre-existing chatDrafts behavior pinned by chatDrafts.test.ts; residual filed separately.

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 merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

This PR is a stale fix and reconciling it is moot.

Main has moved 203 commits since this PR's base (92fa934). git merge-tree against current main (f4268fb) reports a real conflict in 16 files: ChatPage.tsx, ProjectDetailPage.tsx, chatSlice.ts, and 13 locale JSONs.

Reconciling that conflict would buy nothing, because both target sites this PR fixes are already covered on main independently:

Forcing this PR through against current main would land a redundant or divergent second implementation of a fix that already exists. Verified at main f4268fb.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Closing as a stale fix. 203 commits of drift from base 92fa934, a real conflict in 16 files, and both target sites are already covered on main by 2b9e5d9 (#8689) and 2e13662 (#8843). Reconciling would land a redundant or divergent implementation. Detail is in the comment above.

@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two onError handlers still route user-initiated mutation failures only to console.error

1 participant