fix(dashboard): surface steer and plan-export failures via ErrorNotice (#8625) - #8764
fix(dashboard): surface steer and plan-export failures via ErrorNotice (#8625)#8764chenmingwei23 wants to merge 1 commit into
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of 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
[DESIGN-REVIEWED] 87da3bd |
UX Review (Fable 5) — ✅ PASSUX-level review of 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
[UX-REVIEWED] 87da3bd |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 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: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of 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 shipsIntent: make a rejected mid-turn steer and a rejected plan export visible on screen instead of dying in DevTools — a FIX (defect class
Sibling check for the defect class ("onError whose body only logs"): grepped [FIRST-PRINCIPLES-REVIEWED] 87da3bd |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates are mechanically reachable (verified: Candidate 1 (steer success clears a 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 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 False positive or not applicable? A repository writer can comment: |
84cc5d2 to
e35f776
Compare
e35f776 to
9e00942
Compare
9e00942 to
9ec2579
Compare
9ec2579 to
c3d9d69
Compare
c3d9d69 to
87da3bd
Compare
|
/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. |
Human judgment recorded@chenmingwei23 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. |
|
This PR is a stale fix and reconciling it is moot. Main has moved 203 commits since this PR's base (92fa934). 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. |
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'sonErroronly 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'sonErroronly logged, so a refused export read as "the click did nothing".Both are the
errors-use-error-noticedefect 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 wireaskAgentonly where the hand-off cannot destroy unsaved state.showRefusedPress). New actionsteerwith catalog keypages.chatPage.could_not_steer.showRefusedPressnow resolves the journal report from the raw error text at capture time, and the surface rendersreport+askAgent- safe here because the hand-off stages a prompt for a fresh session and per-slot composer drafts persist across the switch.failedoutcome onresolveOptimisticSteer) 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.ErrorNoticeunder the tab bar (data-testid="plan-export-error"), title lead + raw reason, dismiss =mutation.reset(). NoaskAgent, with a comment saying why: the page holds unsaved plan edits (pendingEdits) and the hand-off unmounts them. The mutation takestaskIdas its variable and the render is gated onvariables === 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.exportPlanYamlnow throws via the sharedapiFailurechokepoint instead of hand-rolling anApiError, so the{"error": ...}JSON envelope is unwrapped into prose and the failure is journaled like every other dashboard API error.pages.chatPage.could_not_steer,pages.projectDetailPage.export_failed) inen.manual.jsonand all 11 locales;en-XAregenerated. Keys are new, so they cannot collide with open PR fix(sidebar): surface folder-create failures inline (#8229) #8547'spages.chatSidebar.*keys.Deliberately not touched:
ChatSidebar.tsxand the folder-create path (owned by open PR #8547), and otherconsole.errorsites 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.npx tsc -bclean; 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.mjsruns 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 composer02-steer-notice-dismissed.png- dismissed03-plan-export-error-below-tab-bar.png- rejected export: "Could not export the plan YAML. no plan to export" banner under the tab bar04-plan-export-notice-dismissed.png- dismissedRelated Issues
Closes #8625
Pattern harvest
Rule candidate: review-prompt
Pattern: mutation onError that only console.errors a user-initiated action - flag any
onErrorwhose body is only logging in files that also import ErrorNotice or render user-facing state.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement