Skip to content

fix(pages): route error states through ErrorNotice (batch pages-rest-2) - #8843

Merged
bolichen97 merged 1 commit into
mainfrom
fix/error-notice-pages-rest-2
Sep 6, 2026
Merged

fix(pages): route error states through ErrorNotice (batch pages-rest-2)#8843
bolichen97 merged 1 commit into
mainfrom
fix/error-notice-pages-rest-2

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Batch pages-rest-2 of the dashboard error-state sweep (audit report research/error-states-audit.md, §5.4 "Batch pages-rest-2", 13 top-level src/pages/* files): every hand-written error surface renders through the shared ErrorNotice, per the blocking AUTOSDE rule errors-use-error-notice, and every silent failure the report listed for these pages (a useQuery error nobody read, a useMutation with no onError, a raw .then() with no catch, an if (r.ok) with no else) now renders. Reference implementation: the merged sibling batches (#8729, #8743, #8749, #8760, #8792).

no linked issue: driven by the audit report (research/error-states-audit.md, section 5.4).

Each site passes the SAME string it already rendered, so ErrorNotice recovers the structured context by message match. Every notice carries a testId so tests target it unambiguously (several pages can show two notices at once). The askAgent decision is stated at every site: on where the hand-off loses nothing, off with a No hand-off: <draft> comment next to an unsaved draft.

Sites

askAgent on (reads, lists, actions on persisted items)

  • SchedulePage.tsx — EmptyFolderChip rename/delete failure (inline); batch-move failure; folder-row and job-row action failures (inline, in the cell); batch-delete failure (the typed confirm token is a safety gesture, not a draft — stated in a comment); job-delete failure. Silent fixed: a failed agent roster used to be forwarded only into the job dialog — now a page-level notice with the existing Retry while no dialog is open; cronFolders and defaultAgent query failures (folders silently fell back to a flat list) now render.
  • SchedulePage.tsx JobSecretsPanel — the truncated / unreviewable flags fire on a successful fetch, so they are demoted from ErrorNotice to the surrounding warn note (the rule's inverse violation, per the report's critic); only source.isError stays an ErrorNotice.
  • WebhooksPage.tsx — the 'Failure detail' of a historical run; the load-failure warn Banner (Retry kept beside it, the "reference below still describes the endpoint" hint moved to a muted line so message stays the journal lookup key); the mutation-failure Banner; the failed test-request Banner (success branch unchanged). All four use askAgent={!hasSourceDraft} (aws-control's handOff gating pattern, fix(aws-control): route every error state through the shared notice #8320): the new-source label / destination-agent draft is page state that survives pane switches, so the hand-off is offered only while nothing is typed. Silent fixed: the agents roster query failure now reaches both AgentSelectors as rosterFailure (their built-in failure row + Retry), so a failed load no longer looks like an install with no agents.
  • LogsPage.tsxapi.logLevel().then()useQuery, changeLevel's if (r.ok)useMutation; both failures render inline beside the level selector (a log panel holds no draft).
  • MigrationPage.tsx — the hand-written bg-danger/10 banner → ErrorNotice with onDismiss for the cleanup failure and a labelled Retry Btn beside it for the load failure.
  • McpGatewayCard.tsxsilent fixed: a failed status read used to hide the whole card (enabled defaulted to false); now a notice under the card heading. A failed metrics poll qualifies the stale tiles instead of showing zeros.
  • OverviewPage.tsxsilent fixed: the usage and memory cards left their skeleton up forever on a rejected fetch; both now render the failure.
  • RemoteArtifactDetailPage.tsx — the detailQuery.error || !art Card is split: a real load failure → ErrorNotice with the same title; the not-found branch → a plain empty state (pages.artifactDetailPage.not_found), no longer dressed as an error.
  • SessionArchive.tsx — list and content failures (literal text-red-500).
  • SessionBreakdownTree.tsxsilent fixed: per-node trace read failures render inline in the row (the status chip is the sub-agent's own outcome and stays).
  • TelemetryPanel.tsx — the turns drilldown failure was a text-muted span (no danger class to grep for) → inline notice, Retry kept; silent fixed: a failed startup fetch used to fall through to the "no telemetry recorded yet" body.
  • KiroCrewAgentsPage.tsxsilent fixed: the roster query failure now renders a page-level notice (an empty roster is otherwise indistinguishable from a failed fetch), as do the three editor option-list queries (installed agents / workspaces / config, which fall back to default / kirocrew on error). Both use askAgent={!sheet}.

No hand-off (an unsaved draft shares the screen)

  • SchedulePage.tsx — folder-create failure (folderModalName input); ScriptSourcePanel failure, panel action failure and the job's persisted last_error (all inside the job dialog beside JobForm's unsaved edits; last_error becomes ErrorNotice title='Last Error', whitespace-pre-wrap + font-mono keep the log's shape).
  • ProjectsPage.tsx — plan failure (both panels), refine failure (moved out of the refined-spec block so a refine that failed to start is reported too), the runs-poll failure and the new consolidated action failure: the workspaceDir override and refined textarea edits are unsaved local state (userInput / specText / yamlText are sessionStorage-mirrored and are named as NOT the blocker). Silent fixed: the 3-second taskRunnerStatus poll was awaited without a catch (an unhandled rejection every tick); executePlan / cancel / delete / rename (.catch(() => {})) / planContext / pause / retry / createCron / taskRunToChat dropped both rejections and { ok: false } bodies — all now go through one runAction helper into a dismissable notice; refine() / pollRefine failures surface via refineError.
  • ProjectDetailPage.tsxsilent fixed: approval decisions, DAG approvals, the approval-flag toggle ({ ok: false } included) and the export mutation (was console.error only) now render; the approvals poll failure too. pendingEdits (task title / description / depends_on drafts) and the open TaskDetailPanel form are unsaved.
  • RemoteArtifactDetailPage.tsx — comments read failure and the four comment write mutations (post / reply / mark-review / delete had onError: invalidate only) → one dismissable notice; the comments sidebar's draft is unsaved.
  • KiroCrewAgentsPage.tsx — the workspace-create failure (state split: wsHint keeps the "name is required" validation as plain text, wsError is the request outcome); the crew-sheet footer notice gains its missing No hand-off: dirtyPanes comment; the shared roster-bar error notice (askAgent on) is now rendered only while the sheet is closed, since settleFor writes sheet failures into the same state; the resolved-model read failure in the model pane.

Deviations from the report's recommended_action (author decisions)

  • ProjectsPage / ProjectDetailPage: report suggested askAgent on for the runs list and approval decisions; kept off — the hand-off unmounts the whole page and it always holds unsaved state (workspaceDir, refined, pendingEdits). Flip once those are mirrored like userInput.
  • ProjectsPage runs poll not converted to useQuery: load() carries the loadingRef de-dup (tested), the ?applied pickup and is the onRefresh callback; try/catch + state keeps that intact.
  • WebhooksPage: conditional askAgent={!hasSourceDraft} instead of a fixed on/off, because the label/agent draft is page state visible in every pane.
  • SchedulePage last_error: ErrorNotice has no children slot, so the <pre> body becomes the notice message (block variant preserves line structure); the last_result branch keeps its <pre>.
  • KiroCrewAgentsPage crew-sheet footer still receives the create form's validation hints through the same error state (splitting it touches create()); noted in the comment.
  • TelemetryPanel startup / McpGatewayCard status gate on isError && !data so a transient poll failure keeps the last good figures.

Rule-gap variants seen in this batch (NOT editing website/AUTOSDE.yaml#8729 owns that)

  • error text in text-muted (no danger class) beside a Retry (TelemetryPanel turns)
  • status flags on a successful fetch dressed as ErrorNotice — the inverse violation (SchedulePage secrets truncated/unreviewable)
  • not-found and real failure sharing one condition (detailQuery.error || !art) and one red card (RemoteArtifactDetailPage)
  • a failed read rendered as a different non-error state: card hidden (enabled ?? false, McpGatewayCard), skeleton-forever (OverviewPage), "no telemetry recorded yet" (TelemetryPanel), flat folder list (SchedulePage), empty roster / one-option install (KiroCrewAgentsPage, WebhooksPage)
  • onError: invalidate only — a refetch masquerading as error handling, with a comment claiming errors are "not swallowed" (RemoteArtifactDetailPage)
  • .then(load).catch(() => {}), if (r.ok) load() else-drop, {slot} / {context} truthiness as the only success signal, mutateAsync swallowed to return false, console.error-only onError (ProjectsPage, ProjectDetailPage, LogsPage)
  • one error state shared by two surfaces with different draft exposure (KiroCrewAgentsPage roster bar vs sheet footer); validation hint and request error in one state (WorkspaceForm)
  • an error forwarded as a prop to a child that renders it only conditionally, so the page itself never shows it (SchedulePage rosterFailure)

Out of this batch, noted for the components sweep: src/pages/aidlc/TaskDetailPanel.tsx:92 renders saveError as a hand-written text-danger span.

Pattern harvest

Rule candidate: errors-use-error-notice should name the shapes this batch found the current text misses — a failed read rendered as a different non-error state (card hidden, skeleton-forever, "nothing recorded yet", flat list, empty roster), error text without a danger class beside a Retry, onError: invalidate only, .then(x).catch(() => {}) / if (r.ok) else-drop, and a status flag on a successful fetch dressed as ErrorNotice (the inverse violation). The rule-text edit itself is owned by #8729.

i18n

Four new keys in en.manual.json + all 12 catalogs (hand-translated, glossary terms reused) + en-XA regenerated: pages.kiroCrewAgentsPage.editor_options_load_failed, pages.projectsPage.action_failed, pages.projectDetailPage.failed_to_update_task, pages.logsPage.level_change_failed (fallbacks for { ok: false } bodies with no error). The now-unreferenced pages.migrationPage.dismiss_error is removed from every catalog (dead-key ratchet). Catalog diffs are key-level inserts (+4/−1 per file), validated with a duplicate-key-rejecting parser.

Tests

Aligned to the new shape (no test deleted): SchedulePage.secrets.test.tsx (alert counts → the truncated/unreviewable lines are no longer alerts), cronFoldersSchedulePage.test.tsx (folders-API-down now asserts the notice instead of its absence; batch-move asserts role="alert" via testId), SchedulePage.test.tsx / .scheduleCell / .scriptSource / .sessionOwner (api mock gains defaultAgent, since the page now says when that read fails), ProjectsPageCoverage.test.tsx, ProjectDetailPageCoverage.test.tsx (assert the new notices by testId), LogsPageCov80.test.tsx (render wrapped in a QueryClientProvider).

Verification

  • Local: tsc -b clean, eslint on every touched file clean, I18N_BASE_REF=origin/main npm run i18n:check all gates OK, jscpd 0 clones, no non-inclusive terms in the diff.
  • No test suite run locally by instruction — verification via CI.

Screenshot Evidence

Capture harness: website/capture/error-notice-pages-rest-2.{html,tsx} + website/scripts/capture-error-notice-pages-rest-2.mjs (Vite + Playwright, self-checking: 7 role="alert" notices, 5 hand-offs, Retry beside the Webhooks notice, 0 alerts in the BEFORE column).

Before (origin/main) vs after (this branch), dark:

before/after dark

Light:

before/after light

@CrysisDeu
CrysisDeu requested a review from a team September 5, 2026 23:45
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 5, 2026 23:45
@CrysisDeu
CrysisDeu requested a review from buluoray September 5, 2026 23:45
@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: checking Automated validation is still running labels 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 e607bf148fc98ad531d9f302a645ce2a24eb87d4 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Everything checks out. The diff matches the extremely detailed description at every site I sampled: LogsPage's useQuery/useMutation conversion with the documented chosenLevel semantics, ProjectsPage's runAction helper with actionError kept separate from the poll's loadError, and ErrorNotice's askAgent/testId props exist as used. The capture harness and screenshot commits follow long-standing repo convention (hundreds of prior instances), five sibling batches already merged with this exact shape, askAgent decisions and deviations from the audit report are stated per site, and the rule-text update is correctly deferred to the batch that owns it (#8729).

Design-Verdict: PASS

A mechanical, audit-driven batch matching five merged siblings; every silent-failure fix and askAgent decision is deliberate, documented, and backed by code.

[DESIGN-REVIEWED] e607bf1

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

UX-Verdict: CONCERNS

Cold read holds — every control was read correctly — but several changed surfaces exist only in a synthetic harness, and dead-gateway states stack near-identical red notices.

Watch

  • "Ask the agent" scope is a guess, five times over. The blind reader read it right ("opens a chat with the error already pasted in") but rated it "a guess" and "can't tell whether each one asks about only its own error or opens the same general chat — nothing distinguishes them except position." Every error surface in this batch carries it, persistently; the shared AskAgentButton (not this PR's file) is where a scoping cue would live.
  • One dead gateway stacks sibling notices: SchedulePage can show roster + folders + default-agent notices plus loadError at once, KiroCrewAgentsPage roster + editor-options — each its own red box with its own hand-off link, for one underlying outage. Coalescing when all page reads fail together would cut the alarm wallpaper.

Evidence gaps

  • Secrets truncated/unreviewable demotion (alert → plain status line inside the warn note, schedule-secrets-source-truncated) appears in no screenshot.
  • RemoteArtifactDetailPage's new not-found empty state (pages.artifactDetailPage.not_found card) appears in no screenshot.
  • No notice is shown in real page context — table row, breakdown-tree row, MigrationPage's new labelled Retry Btn, McpGatewayCard's error-only card; both committed images are the isolated capture harness.

Suggestions

  • pages.projectsPage.action_failed ("Action failed"): reuse the sibling Webhooks phrasing shape ("That action did not go through") or name the verb — the bare fallback is the "Something went wrong" pattern the sweep is removing.

[UX-REVIEWED] e607bf1

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of e607bf148fc98ad531d9f302a645ce2a24eb87d4 — 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 evidence is gathered. Here is the review.

First-Principles-Verdict: CONCERNS

Every surface earns its place under the blocking errors-use-error-notice rule, but one description claim (WebhooksPage roster → AgentSelector) is contradicted by the diff.

What this change ships

Intent: make every error on 13 top-level dashboard pages visible and rendered through the shared ErrorNotice — a FIX (rule-compliance sweep + silent-failure defects).

  1. Hand-written red divs/banners on 13 pages become ErrorNotice with per-site hand-off decisions — justified (blocking AUTOSDE.yaml rule, website/AGENTS.md:78).
  2. ~20 formerly silent failures (rosters, folders, polls, {ok:false} bodies, export, traces) now render — justified.
  3. Failed background refetches now banner above retained data (Webhooks, Telemetry, McpGatewayCard) — declared visibility change.
  4. JobSecretsPanel truncated/unreviewable demoted from alert to status text — justified, subtractive.
  5. RemoteArtifactDetailPage not-found split into a plain empty state — justified.
  6. Client validation split from request errors (wsHint/sheetHint) — declared, needed by the split the rule forces.
  7. LogsPage level read/change moved to React Query — justified (documented convention).
  8. ProjectsPage actions consolidated into runAction — justified.
  9. New keys in 13 catalogs; migrationPage.dismiss_error deleted (0 remaining consumers; grepped dismiss_error, survivors are chatPage/appDetailPage keys) — justified.
  10. Capture page/script + 2 committed PNGs — conventional evidence (6 prior error-notice-* capture pairs, ~960 files under temp-screenshots/).

Watch

  • Description says the Webhooks roster failure "reaches both AgentSelectors as rosterFailure (their built-in failure row + Retry)", but grep of WebhooksPage.tsx finds 0 rosterFailure uses — the diff ships a page-level banner instead. The existing mechanism (AgentSelector.rosterFailure, components/AgentSelector.tsx:75, used by SchedulePage) was built for exactly this; either wire it or correct the claim so the next batch doesn't copy the banner as the pattern.
  • WebhooksPage drops the base's deliberate error && !data guard (its deleted comment: a failed refetch "must not claim the page is broken"), so a healthy snapshot now carries a failure banner during any refetch blip. Declared, but it reverses a recorded decision — a human should confirm the flip is wanted.

[FIRST-PRINCIPLES-REVIEWED] e607bf1

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've completed my independent review. Key verifications:

  • ErrorNotice returns null on falsy message (line 77) — every unconditional call site is safe.
  • The blocking rule errors-use-error-notice: every added/touched ErrorNotice carries either askAgent or a {/* No hand-off: … */} comment naming a concrete draft. The retained text-danger divs (wsHint, sheetHint) are client-side validation hints, which the rule explicitly excludes ("name is required"). The secrets_pending_source_truncated/unreviewable conversions to plain status text are verdicts on a fetch that succeeded — status, not error — also excluded. No blocking violation.
  • i18n: the new keys live in en.manual.json (hand-authored English) + all 11 translated locales; en.json is the codemod-regenerated half and correctly does not carry them yet. The removed pages.migrationPage.dismiss_error has no remaining consumer (the surviving dismiss_error refs are the distinct chatPage/appDetailPage keys).
  • Logic paths (LogsPage level derivation + mutation, ProjectsPage.runAction envelope handling, ProjectDetailPage.toggleApprove ok-computation, RemoteArtifactDetailPage isError-vs-empty branching, McpGatewayCard error-vs-disabled) all preserve prior success behavior while surfacing previously-swallowed failures.

Nothing survived falsification at 80+, and I found no new grounded defect to add.

No findings.

[OPUS-REVIEWED] e607bf1

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

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

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e607bf1

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

@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 6, 2026
Batch pages-rest-2 of the dashboard error-state sweep (audit report §5.4):
every hand-written error surface in the 13 top-level pages renders through
the shared ErrorNotice, with the askAgent decision stated at each site, and
the silent query/mutation/raw-promise failures the report listed now render.

Pages: KiroCrewAgentsPage, LogsPage, McpGatewayCard, MigrationPage,
OverviewPage, ProjectDetailPage, ProjectsPage, RemoteArtifactDetailPage,
SchedulePage, SessionArchive, SessionBreakdownTree, TelemetryPanel,
WebhooksPage. Tests aligned to the new shape (testId / role=alert).

Four new keys (13 catalogs + en-XA); the orphaned
pages.migrationPage.dismiss_error key is removed.

Review rounds folded in: the crew sheet's create-form validation hints are
kept out of its ErrorNotice (sheetHint); McpGatewayCard / TelemetryPanel /
WebhooksPage say a failed refetch over cached data instead of gating the
notice on !data; the webhooks agents-roster failure is a page-level
ErrorNotice with Retry rather than AgentSelector's hand-written row.
@CrysisDeu
CrysisDeu force-pushed the fix/error-notice-pages-rest-2 branch from 2bf03f6 to e607bf1 Compare September 6, 2026 00:46
@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 6, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tech Lead review — approved.

Mechanical, audit-driven batch matching five merged siblings (#8729, #8743, #8749, #8760, #8792). Verified independently rather than taking the description on trust:

  • Rule compliance: every added ErrorNotice carries either askAgent or a {/* No hand-off: … */} comment naming a concrete draft. The retained text-danger spans (wsHint, sheetHint) are client-side validation hints the errors-use-error-notice rule explicitly excludes, and the secrets_pending_source_truncated/unreviewable demotion to plain status text is correct — those flags fire on a successful fetch, so they were the rule's inverse violation.
  • Logic conversions preserve prior semantics: LogsPage's chosenLevel ?? levelQuery.data?.level ?? 'INFO' keeps the selector on the backend's level until a change is accepted, and the useMutation now rejects on a { ok: false } body that the old if (r.ok) dropped silently. ProjectsPage.runAction routes both a rejection and a resolved { ok: false } envelope into actionError, keeps it separate from the 3s poll's loadError, and runs onOk only when the action took. WebhooksPage's loadError = error (was error && !data) is a declared visibility change: a failed background refetch is now said above the retained snapshot instead of passing for a live one.
  • i18n is consistent: identical 4-key addition (action_failed, editor_options_load_failed, failed_to_update_task, level_change_failed) across all 12 translated locales + en.manual.json, with en.json correctly left to the codemod. Every newly referenced key resolves (pages.artifactDetailPage.not_found, components.agentSelector.roster_load_failed, pages.kiroCrewAgentsPage.editor_options_load_failed), and the removed pages.migrationPage.dismiss_error has no remaining consumer — its only surviving mentions on the branch are in a code comment.
  • Tests strengthen rather than relax: the flipped assertions are exactly where behaviour deliberately changed (cronFoldersSchedulePage now asserts the folders failure is said while the job list still renders; the secrets test asserts one alert plus a schedule-secrets-source-truncated status line instead of three alerts), and the new defaultAgent mocks exist because the page now surfaces that read's failure.

All checks green including the five AI lanes; the six cancelled runs are superseded duplicates. Two advisory notes, neither blocking: the description's claim that the Webhooks roster failure reaches both AgentSelectors as rosterFailure is stale — the diff renders it as a page-level notice with its own Retry, which fixes the same silent failure at a different surface; and the UX lane's notice-stacking / askAgent scoping concerns live in the shared AskAgentButton, outside this PR's files.

@bolichen97
bolichen97 merged commit 2e13662 into main Sep 6, 2026
67 of 73 checks passed
@bolichen97
bolichen97 deleted the fix/error-notice-pages-rest-2 branch September 6, 2026 02:03
@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants