Skip to content

fix(dashboard): surface refused regenerate/switch-variant via ErrorNotice (#4202) - #4238

Merged
bolichen97 merged 1 commit into
mainfrom
fix/refused-press-error-notice-4202
Aug 18, 2026
Merged

fix(dashboard): surface refused regenerate/switch-variant via ErrorNotice (#4202)#4238
bolichen97 merged 1 commit into
mainfrom
fix/refused-press-error-notice-4202

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Two presses in the dashboard chat swallow a server refusal into console.warn, so the button flicks to disabled and straight back with nothing on screen:

Both endpoints re-check under the slot lock and refuse for ordinary, user-actionable reasons: a turn already running, a stop in progress, a pending approval. The user's only theory is "the click did nothing", and the natural next move is pressing the button again.

The third such press -- Continue -- is being fixed by #4194 with a titled ErrorNotice above the composer. This PR does not touch the continue path; #4194 owns that surface.

Why it matters

A control that promises recovery (regenerate a failed answer, flip to another variant) and then says nothing at all teaches the user the dashboard is broken. The server already names the reason in its error body; throwing that prose away in the console is pure information loss at the exact moment the user needs it.

What changed (motivation -> approach -> change)

Symptom: refusals die in console.warn. Root cause: neither press has any rendered error surface. Change: generalize the surface rather than adding a second one-off --

  • One refused-press ErrorNotice rendered above the composer, fed by a single showRefusedPress(action, error) helper. Any refusable press added later inherits the behavior by adding one entry to the REFUSED_PRESS_TITLE_KEYS map instead of re-discovering console.warn.
  • Per-action titles via 2 new catalog keys -- pages.chatPage.could_not_regenerate ("Couldn't regenerate") and pages.chatPage.could_not_switch_variant ("Couldn't switch version", matching the variant switcher's own "Next/Previous version" labels) -- across the 12 shipped locales, with en-XA regenerated by scripts/gen-pseudolocale.mjs. The key map is as const so the catalog-key gate statically resolves every member.
  • Both call sites wired: the regenerate catch keeps its snapshot-restore + spinner reset; the switch-variant catch surfaces the reason instead of logging it.
  • The notice retires when a turn starts (the refusal now describes a state that passed), when the session changes, or on dismiss. The message is the server's own prose, unwrapped by the api client's friendlyErrText chokepoint, so ErrorNotice's journal lookup recovers the structured context (endpoint, status, backend code).

Tests

website/src/test/ChatPage.refusedPress.test.tsx (mounted ChatPage, mutation-verified -- all 3 fail with the ChatPage change reverted):

  1. A refused regenerate renders the notice with the per-action title and the server's reason.
  2. A refused switch-variant renders the notice; dismiss clears it; the handler passes the pressed variant index.
  3. A turn taking over retires the refusal.

Full local battery on the rebased head: npx tsc -b clean, website vitest 1354 files / 21240 tests green, npm run i18n:check fully green, eslint 0 errors (no new warnings), brand gate clean.

Manual verification

Capture harness (website/capture/refused-press.{html,tsx} + website/scripts/capture-refused-press.mjs, same pattern as the repo's other capture/ entries) mounts the REAL ErrorNotice + ChatInput against the real stylesheet and live i18n catalog; each scene asserts its rendered text before writing a frame, so a frame cannot contradict the diff.

Screenshots / video

Before (dark) -- a refused press shows nothing above the composer:

before dark

After -- refused regenerate (dark):

regenerate dark

After -- refused switch-variant (dark):

switch variant dark

After -- refused regenerate (light)

regenerate light

Related Issues

Closes #4202

Checklist

  • Single commit 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) -- N/A: no config schema or documented behavior change
  • No secrets, credentials, or internal references in the diff

@chenmingwei23
chenmingwei23 requested a review from a team August 18, 2026 01:53
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 18, 2026 01:53
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 18, 2026
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 196c516a531a2e907a0625859166cc9fba956d02 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Real information-loss bug, fixed at its root (no rendered surface) with a small generalized helper rather than a third one-off — proportionate and well-evidenced.

Watch

  • The PR's own rationale — "generalize the surface rather than adding a second one-off" — is undercut by leaving the third refusable press (Continue, fix(chat): stop a slow kiro-cli probe from disabling Continue #4194) on a separate ErrorNotice. Once both land, the composer has two adjacent refusal surfaces with independent retirement rules; whichever PR merges second should fold Continue into REFUSED_PRESS_TITLE_KEYS/showRefusedPress so the map is actually the single registry it claims to be.

Suggestions

  • slotRunning clearing only fires on the false→true transition; a switch-variant refusal issued while a turn is already running (that press has no !slotRunning guard) persists through the whole turn — clear on refusal-supersession or gate the press the same way regenerate is gated.

[DESIGN-REVIEWED] 196c516

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 196c516a531a2e907a0625859166cc9fba956d02 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 196c516

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 196c516a531a2e907a0625859166cc9fba956d02: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Advisory premise-level review of 196c516a531a2e907a0625859166cc9fba956d02 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

Contract read; evidence gathered. I verified the key mechanical claims: ErrorNotice is the existing shared error component being reused (not duplicated); the two remaining console.warn swallows in ChatPage.tsx after this change are continue (line 4903, explicitly deferred to #4194) and background title generation (line 6350, not a user press); the capture harness follows a documented repo convention (152 sibling capture-*.mjs scripts, temp-screenshots/<feature>/ mandated by the PR template with its own cleanup workflow); and the new map/helper each have exactly 2 real consumers.

First-Principles-Verdict: PASS

A reported silent-failure defect (#4202) fixed at the cause the author names — no rendered surface — by reusing the existing ErrorNotice, not inventing one.

What this change ships

Intent: make a server-refused regenerate or switch-variant press tell the user why, instead of silently un-disabling the button. This is a FIX.

  1. Refused regenerate shows a titled notice above the composer — justified (the fix; defect Regenerate and switch-variant swallow server refusals into console.warn #4202).
  2. Refused switch-variant shows the same notice — justified (the fix).
  3. Notice auto-retires on turn start / session change, dismissible — justified (stale refusal would mislead).
  4. Two new titles across 12 locales + pseudolocale — mandated by the i18n gate.
  5. showRefusedPress helper + REFUSED_PRESS_TITLE_KEYS map — 2 consumers each (both catches), not premature.
  6. Capture harness + 4 committed PNGs — declared; repo convention (152 sibling capture-*.mjs scripts, temp-screenshots/ in the PR template).
  7. Mounted-ChatPage regression test — justified (pins the pixel-level behavior).

Watch

  • The map's stated payoff — "any refusable press added later inherits the behavior" — is contradicted for the one known sibling: the description says "fix(chat): stop a slow kiro-cli probe from disabling Continue #4194 owns that surface" for continue (ChatPage.tsx:4903 still console.warns). If fix(chat): stop a slow kiro-cli probe from disabling Continue #4194 ships its own one-off notice, ChatPage carries two refusal surfaces above the composer; when it lands, fold continue into REFUSED_PRESS_TITLE_KEYS and delete the one-off.
  • addNotification (critical priority, used at ChatPage.tsx:3910 for a failed session start) is an existing refusal surface; the inline notice is meaningfully different (adjacent to the pressed control, retires on turn start), so not a second spelling — noted only so a human confirms that split stays deliberate.

[FIRST-PRINCIPLES-REVIEWED] 196c516

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

Advisory UX-level review of 196c516a531a2e907a0625859166cc9fba956d02 — updated in place on each push; does not block merge.

UX-Verdict: CONCERNS

The refusal finally reaches the screen, but its body is raw backend prose — "slot is running", "no variants" — internal jargon the title's own vocabulary contradicts.

Watch

  • The notice body is the server's verbatim refusal text (message: e.message), and the committed screenshots show what ships: "Couldn't switch version — no variants" clashes with the UI's own "Previous version / Next version" vocabulary, and "slot is running" names an internal concept ("slot") instead of what the user sees (a reply already in progress) or what to do (wait, then retry). Every refusal renders this way (frequency: always for this feature; impact: comprehension friction, likely re-press despite the notice existing to prevent that). Smallest fix: map the known refusal reasons to catalog strings and fall back to server text only for unknown ones.
  • The String(e) fallback means a plain network failure renders raw exception text ("TypeError: Failed to fetch") as the banner body — exactly the class of copy the shared ErrorNotice exists to avoid. Route unknown errors through friendlyErrText/the error journal instead of stringifying.

Suggestions

  • "Couldn't regenerate" / "Couldn't switch version" sit beside the existing sibling key could_not_set_the_agent_default_model ("Could not…"); pick one contraction style for the pages.chatPage error titles.

[UX-REVIEWED] 196c516

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 196c516a531a2e907a0625859166cc9fba956d02 — this comment is updated in place on each push.

Review details

Both ErrorNotice (line 94) and i18nT (line 205) resolve. The two new keys sit in the same catalog object as could_not_set_the_agent_default_model, which is referenced as pages.chatPage.could_not_set_the_agent_default_model — so REFUSED_PRESS_TITLE_KEYS resolves correctly. The switch-variant path performs no optimistic index change in the diff (index is server-driven via variant_idx), so no rollback is missing on refusal. The clearing effects (activeSlot, slotRunning) are benign. Nothing survives falsification.

No findings.

[OPUS-REVIEWED] 196c516

Verdict parsed from the review's SHA-scoped output markers for commit 196c516a531a2e907a0625859166cc9fba956d02.

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

@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 Aug 18, 2026
…tice (#4202)

A regenerate or switch-variant press the server refuses under the slot
lock (a turn already running, a stop in progress, a pending approval, a
readiness probe that timed out) died in console.warn: the control
flicked to disabled and straight back with nothing on screen.

Generalize the refusal surface: one refused-press ErrorNotice above the
composer, fed by any refusable press through showRefusedPress with a
per-action title (pages.chatPage.could_not_regenerate /
could_not_switch_variant across the 12 shipped locales + generated
en-XA), showing the server's own message. The notice retires when a
turn starts, the session changes, or it is dismissed. The continue
press keeps its own path (open PR #4194 owns that surface).

Tests lock in: refused regenerate renders title + server reason;
refused switch-variant renders and dismiss clears; a starting turn
retires the notice. Capture harness + temp-screenshots included.
@chenmingwei23
chenmingwei23 force-pushed the fix/refused-press-error-notice-4202 branch from ea17989 to 196c516 Compare August 18, 2026 02:17
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Aug 18, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for UX Review CONCERNS (head ea17989, addressed at 196c516):

Screenshots showed invented refusal prose -- fixed at 196c516. The finding is verified legitimate: the wired endpoints return {"error": "slot is running"} (409) and "no variants" (400), and the capture harness hardcoded polished prose that ships nowhere. The harness REFUSALS map and the driver's text assertions now carry the literal backend strings, and all four frames were recaptured -- the PR body now documents the real render, run-on and all ("Couldn't regenerate slot is running").

Friendly-copy mapping in showRefusedPress -- rebutted as the wrong mechanism for this PR. A client-side map keyed on refusal prose is fragile: any backend rewording silently misses the map and falls back anyway, and nothing detects the drift. The repo's sanctioned direction (AGENTS.md: "a new non-2xx JSON body MUST carry a machine-readable code field") is a backend code on the refusal + a code-keyed catalog string on the client -- these pre-existing endpoints simply do not carry code yet, and adding it is a backend change outside this frontend PR. Surfacing the server's own words verbatim is also the pattern the sibling continue-refusal surface ships (its evidence frame shows raw "sub-agents are running"), so this PR matches the established behavior rather than forking it. The copy-quality improvement is real but belongs to a systemic pass (add code to the slot-refusal bodies, map once for every ErrorNotice consumer), noted for the maintainer rather than half-built here per-prose.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for First Principles Review CONCERNS (head ea17989, applies unchanged to 196c516):

"Fold Continue into the map here / retire one of the two surfaces" -- rebutted for THIS PR, with the convergence path stated. The deferral is deliberate collision avoidance, not scope-dodging: #4194 is another author's open, in-review branch that already carries the continue wiring, its own i18n key, tests, and evidence frames. Landing a competing continue implementation here would guarantee whichever PR merges second rebases through a semantic conflict in the same ChatPage region and re-arms every reviewer on both PRs -- strictly worse than sequencing.

The generalized map is exactly what makes convergence cheap: whichever PR lands second, migrating continue onto showRefusedPress is one REFUSED_PRESS_TITLE_KEYS entry plus swapping one setContinueError(...) call, deleting the one-off state. That final state -- exactly one refused-press mechanism -- is the agreed end shape; the only question is sequencing across two live branches, and that is decided by merge order, not by widening this diff. This PR deliberately does not touch the continue catch (console.warn('continue failed', ...) stays) precisely so #4194's diff remains valid.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for UX Review CONCERNS on 196c516 (round 2):

Raw backend prose as the notice body -- rebutted for this PR (previously dispositioned; restating with the round-2 specifics). The finding is legitimate as a copy-quality observation, and the screenshots now honestly show it. The prescribed prose-keyed map remains the wrong mechanism: matching on server sentence strings drifts silently the day the backend rewords, and per-surface maps fork the copy problem instead of solving it. The repo's own contract (AGENTS.md: non-2xx bodies MUST carry a machine-readable code) points at the durable fix -- add code to the slot-refusal bodies and map code -> catalog string once, at the ErrorNotice/journal layer, for every consumer including the sibling continue surface. Recorded in the operator's tracked backlog as a systemic follow-up; building a prose-map here would ship throwaway plumbing the code-keyed layer immediately deletes.

String(e) renders raw exception text on network failure -- rebutted as pre-existing sibling-wide behavior, not a defect this PR introduces. Verified: api.regenerateSlot/switchVariant are bare fetch wrappers, so a network failure rejects with the browser's TypeError before apiFailure/friendlyErrText can run (those take a Response, which does not exist when fetch itself rejects). The e instanceof Error ? e.message : String(e) shape here is byte-for-byte the pattern of the sibling continue-refusal surface and the other ErrorNotice call sites; the journal lookup by message is ErrorNotice's own documented recovery path. Wrapping transport failures with friendly copy is the same systemic layer as the code-keyed map above -- one chokepoint fix in the api client, not a per-press special case -- and is folded into the same tracked follow-up.

Title contraction style ("Couldn't" vs sibling "Could not") -- rebutted with precedent. The chosen style matches the newest key in this exact surface family: the continue-refusal title shipped by the sibling PR uses "Couldn't continue", and these two titles will sit in the same notice. Consistency within the refused-press surface outranks consistency with an older, unrelated settings-error key; a catalog-wide style pass is copy-editing outside this PR's scope.

@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 Aug 18, 2026
@bolichen97
bolichen97 merged commit 1128b92 into main Aug 18, 2026
65 of 67 checks passed
@bolichen97
bolichen97 deleted the fix/refused-press-error-notice-4202 branch August 18, 2026 06:32
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 18, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…tice (kirodotdev#4202) (kirodotdev#4238)

A regenerate or switch-variant press the server refuses under the slot
lock (a turn already running, a stop in progress, a pending approval, a
readiness probe that timed out) died in console.warn: the control
flicked to disabled and straight back with nothing on screen.

Generalize the refusal surface: one refused-press ErrorNotice above the
composer, fed by any refusable press through showRefusedPress with a
per-action title (pages.chatPage.could_not_regenerate /
could_not_switch_variant across the 12 shipped locales + generated
en-XA), showing the server's own message. The notice retires when a
turn starts, the session changes, or it is dismissed. The continue
press keeps its own path (open PR kirodotdev#4194 owns that surface).

Tests lock in: refused regenerate renders title + server reason;
refused switch-variant renders and dismiss clears; a starting turn
retires the notice. Capture harness + temp-screenshots included.
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.

Regenerate and switch-variant swallow server refusals into console.warn

2 participants