fix(pages): route error states through ErrorNotice (batch pages-rest-2) - #8843
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Everything checks out. The diff matches the extremely detailed description at every site I sampled: LogsPage's 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 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of 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
Evidence gaps
Suggestions
[UX-REVIEWED] e607bf1 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence is gathered. Here is the review. First-Principles-Verdict: CONCERNS Every surface earns its place under the blocking What this change shipsIntent: make every error on 13 top-level dashboard pages visible and rendered through the shared
Watch
[FIRST-PRINCIPLES-REVIEWED] e607bf1 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've completed my independent review. Key verifications:
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 False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
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.
2bf03f6 to
e607bf1
Compare
bolichen97
left a comment
There was a problem hiding this comment.
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
ErrorNoticecarries eitheraskAgentor a{/* No hand-off: … */}comment naming a concrete draft. The retainedtext-dangerspans (wsHint,sheetHint) are client-side validation hints theerrors-use-error-noticerule explicitly excludes, and thesecrets_pending_source_truncated/unreviewabledemotion 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'schosenLevel ?? levelQuery.data?.level ?? 'INFO'keeps the selector on the backend's level until a change is accepted, and theuseMutationnow rejects on a{ ok: false }body that the oldif (r.ok)dropped silently.ProjectsPage.runActionroutes both a rejection and a resolved{ ok: false }envelope intoactionError, keeps it separate from the 3s poll'sloadError, and runsonOkonly when the action took.WebhooksPage'sloadError = error(waserror && !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, withen.jsoncorrectly 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 removedpages.migrationPage.dismiss_errorhas 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 (
cronFoldersSchedulePagenow asserts the folders failure is said while the job list still renders; the secrets test asserts one alert plus aschedule-secrets-source-truncatedstatus line instead of three alerts), and the newdefaultAgentmocks 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.
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-levelsrc/pages/*files): every hand-written error surface renders through the sharedErrorNotice, per the blocking AUTOSDE ruleerrors-use-error-notice, and every silent failure the report listed for these pages (auseQueryerror nobody read, auseMutationwith noonError, a raw.then()with no catch, anif (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
ErrorNoticerecovers the structured context by message match. Every notice carries atestIdso tests target it unambiguously (several pages can show two notices at once). TheaskAgentdecision is stated at every site: on where the hand-off loses nothing, off with aNo 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;cronFoldersanddefaultAgentquery failures (folders silently fell back to a flat list) now render.SchedulePage.tsxJobSecretsPanel — thetruncated/unreviewableflags fire on a successful fetch, so they are demoted fromErrorNoticeto the surrounding warn note (the rule's inverse violation, per the report's critic); onlysource.isErrorstays anErrorNotice.WebhooksPage.tsx— the 'Failure detail' of a historical run; the load-failure warnBanner(Retry kept beside it, the "reference below still describes the endpoint" hint moved to a muted line somessagestays the journal lookup key); the mutation-failureBanner; the failed test-requestBanner(success branch unchanged). All four useaskAgent={!hasSourceDraft}(aws-control'shandOffgating 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 bothAgentSelectors asrosterFailure(their built-in failure row + Retry), so a failed load no longer looks like an install with no agents.LogsPage.tsx—api.logLevel().then()→useQuery,changeLevel'sif (r.ok)→useMutation; both failures render inline beside the level selector (a log panel holds no draft).MigrationPage.tsx— the hand-writtenbg-danger/10banner →ErrorNoticewithonDismissfor the cleanup failure and a labelled RetryBtnbeside it for the load failure.McpGatewayCard.tsx— silent fixed: a failed status read used to hide the whole card (enableddefaulted to false); now a notice under the card heading. A failed metrics poll qualifies the stale tiles instead of showing zeros.OverviewPage.tsx— silent fixed: the usage and memory cards left their skeleton up forever on a rejected fetch; both now render the failure.RemoteArtifactDetailPage.tsx— thedetailQuery.error || !artCard is split: a real load failure →ErrorNoticewith 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 (literaltext-red-500).SessionBreakdownTree.tsx— silent 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 atext-mutedspan (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.tsx— silent 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 todefault/kirocrewon error). Both useaskAgent={!sheet}.No hand-off (an unsaved draft shares the screen)
SchedulePage.tsx— folder-create failure (folderModalNameinput); ScriptSourcePanel failure, panel action failure and the job's persistedlast_error(all inside the job dialog beside JobForm's unsaved edits;last_errorbecomesErrorNotice title='Last Error',whitespace-pre-wrap+font-monokeep 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: theworkspaceDiroverride andrefinedtextarea edits are unsaved local state (userInput/specText/yamlTextare sessionStorage-mirrored and are named as NOT the blocker). Silent fixed: the 3-secondtaskRunnerStatuspoll was awaited without a catch (an unhandled rejection every tick);executePlan/ cancel / delete / rename (.catch(() => {})) /planContext/ pause / retry /createCron/taskRunToChatdropped both rejections and{ ok: false }bodies — all now go through onerunActionhelper into a dismissable notice;refine()/pollRefinefailures surface viarefineError.ProjectDetailPage.tsx— silent fixed: approval decisions, DAG approvals, the approval-flag toggle ({ ok: false }included) and the export mutation (wasconsole.erroronly) 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 hadonError: invalidateonly) → one dismissable notice; the comments sidebar's draft is unsaved.KiroCrewAgentsPage.tsx— the workspace-create failure (state split:wsHintkeeps the "name is required" validation as plain text,wsErroris the request outcome); the crew-sheet footer notice gains its missingNo hand-off: dirtyPanescomment; the shared roster-barerrornotice (askAgent on) is now rendered only while the sheet is closed, sincesettleForwrites 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 likeuserInput.ProjectsPageruns poll not converted touseQuery:load()carries theloadingRefde-dup (tested), the?appliedpickup and is theonRefreshcallback; try/catch + state keeps that intact.WebhooksPage: conditionalaskAgent={!hasSourceDraft}instead of a fixed on/off, because the label/agent draft is page state visible in every pane.SchedulePagelast_error:ErrorNoticehas no children slot, so the<pre>body becomes the noticemessage(block variant preserves line structure); thelast_resultbranch keeps its<pre>.KiroCrewAgentsPagecrew-sheet footer still receives the create form's validation hints through the sameerrorstate (splitting it touchescreate()); noted in the comment.TelemetryPanelstartup /McpGatewayCardstatus gate onisError && !dataso a transient poll failure keeps the last good figures.Rule-gap variants seen in this batch (NOT editing
website/AUTOSDE.yaml— #8729 owns that)text-muted(no danger class) beside a Retry (TelemetryPanel turns)ErrorNotice— the inverse violation (SchedulePage secrets truncated/unreviewable)detailQuery.error || !art) and one red card (RemoteArtifactDetailPage)enabled ?? false, McpGatewayCard), skeleton-forever (OverviewPage), "no telemetry recorded yet" (TelemetryPanel), flat folder list (SchedulePage), empty roster / one-option install (KiroCrewAgentsPage, WebhooksPage)onError: invalidateonly — 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,mutateAsyncswallowed toreturn false,console.error-onlyonError(ProjectsPage, ProjectDetailPage, LogsPage)errorstate shared by two surfaces with different draft exposure (KiroCrewAgentsPage roster bar vs sheet footer); validation hint and request error in one state (WorkspaceForm)rosterFailure)Out of this batch, noted for the components sweep:
src/pages/aidlc/TaskDetailPanel.tsx:92renderssaveErroras a hand-writtentext-dangerspan.Pattern harvest
Rule candidate:
errors-use-error-noticeshould 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: invalidateonly,.then(x).catch(() => {})/if (r.ok)else-drop, and a status flag on a successful fetch dressed asErrorNotice(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-XAregenerated: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 noerror). The now-unreferencedpages.migrationPage.dismiss_erroris 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 assertsrole="alert"via testId),SchedulePage.test.tsx/.scheduleCell/.scriptSource/.sessionOwner(api mock gainsdefaultAgent, 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 aQueryClientProvider).Verification
tsc -bclean,eslinton every touched file clean,I18N_BASE_REF=origin/main npm run i18n:checkall gates OK,jscpd0 clones, no non-inclusive terms in the diff.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: 7role="alert"notices, 5 hand-offs, Retry beside the Webhooks notice, 0 alerts in the BEFORE column).Before (origin/main) vs after (this branch), dark:
Light: