fix(boards): Retry on the boards list error state, a caught Metrics load, translated timeout copy and an error clear on success - #2690
Conversation
The list read has been bounded with skipRetry since #2685, so a one-off 503 or an API restart during mount no longer heals itself in the retry layer. The alert stood until the user navigated away and back, and because the read is shared, every unfiltered caller on the page failed with it. The error block now renders the message in a role="alert" paragraph with a real button beside it, carrying the same data-action="retry-board-load" shape PaperTriageTable uses. role="alert" moved from the wrapper to the paragraph so the announcement is the sentence alone and the button is a focusable sibling outside the live region, described by the alert through aria-describedby. Both the mount read and the retry go through one loadBoards function, so a retry produces the same sequence a mount does: skeleton in flight, then the grid or the alert again. A retry is never swallowed by the store throttle, which stamps only after a success. This commit also lands the two board-store boundary keys (boards.error .timeout and boards.error.cancelled) in en, es and it, so the catalogs stay structurally parallel in one step; the store starts using them in a later commit of this branch. Refs #2689 (item 1), #2685, PR #2688.
loadBoards is awaited inside an async onMounted callback and had a try with a finally but no catch, so the store rethrow escaped the hook. Vue routes a lifecycle-hook rejection to app.config.errorHandler, and with no handler installed it rethrows from its own catch, which reaches the window as an unhandledrejection and is forwarded to Sentry by installWindowErrorListeners. The failure was pre-existing; the #2685 bound made it routine. The store already set its error surface and toasted through handleApiError, so the catch reports nothing further. boards keeps its previous value on failure: a failed read learned nothing about the list and must not be reported as empty. The finally is unchanged. The spec asserts through a mount-level app error handler, which is the first thing Vue reaches on that route, so "never called" is precisely "the rejection did not escape the callback". A process-level listener would not work here because Vue intercepts the rejection before it can reach the window. Refs #2689 (item 2), #2685, PR #2688.
getErrorMessage prefers response.data.message and then err.message, which is right for an API error and wrong for a transport one: a transport failure carries no server message, so err.message is the axios string "timeout of 10000ms exceeded", rendered verbatim inside a localized alert and a localized toast. Since #2685 bounded every board read, that is a routine 10 s outcome rather than an exotic one. handleApiError now answers the two transport shapes from the catalog before falling through to getErrorMessage, so every other error keeps exactly the behaviour it had. Timeouts are detected by the axios codes ECONNABORTED and ETIMEDOUT, with a message check behind them for an adapter that reports a timeout without a code; that arm is gated on isAxiosError and on the absence of a response, so a server error whose own wording mentions a timeout keeps the server wording. A cancel is mapped rather than dropped. It should not reach here at all (both read paths return on axios.isCancel first, and the only aborter today bumps the read generation before aborting), but if one ever does, silence would leave error null with boards still empty, and the BoardsListView v-else-if chain renders that as the EMPTY state: an unconfirmed claim that the account has no boards, which is the #1961 class of lie the shared read exists to prevent. The copy is resolved through i18n.global.t rather than useI18n for the reason recorded in locales/en/review.ts section 1: this factory is called from stores and from specs that never mount a component, and i18n.global.t still reads the live locale, so the copy follows a language switch. utils/errorMessage.ts is deliberately untouched: it is cross-surface, and this is the board store's own boundary. Refs #2689 (item 3), #2685, PR #2688.
…ead succeeds fetchBoards cleared error on ENTRY to a read but never on the success path. Two list reads genuinely overlap, because a filtered (includeArchived) read never joins the share and never populates it: the activity selector's can still be on the wire when the boards list mounts its unfiltered one. When the failing half writes its alert after the surviving half has already cleared on entry, error was left set beside a populated boards, and the BoardsListView chain of v-if loading, v-else-if error, then the grid showed the alert instead of the boards it already held. The #2685 bound made the failing half deterministic at 10 s, so this stopped being a race nobody reaches. The clear sits after the generation check, so a superseded read still writes nothing. The detail read has the same gap and is deliberately left alone: a successful BACKGROUND detail refresh must not clear an alert it does not own, so the fix there needs an intent gate rather than the one line this slice authorizes, and it has no coverage in this branch. Refs #2689 (item 4), #2685, PR #2688.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head Reviewer: one fresh-context independent reviewer subagent (read-only; Codex credits exhausted, SC-9), given the worktree at the head and the merge-base diff. Verdict: SHIP, no CRITICAL or HIGH; three MEDIUMs on the new behaviour, all fixed in round 2 because each is a defect in the control this PR introduces. Lenses that found nothing: the success-path clear sits after the generation check, so a superseded post-logout read cannot clear the next session's error or write the throttle stamp; Findings, triaged once:
Round count: 2 after the fix push. Merge gate: |
…ndow
Round-2 review finding 1. The Retry control this PR added was a silent
no-op inside the store 5 s throttle window, and the BoardsListView
docblock asserted the opposite.
The premise the docblock rested on is true: the throttle stamp is
written only after a success, so a retry that follows a FAILED list read
was never blocked. But the stamp from an EARLIER success survives, and
state.error is shared by every board action. A list read succeeds at T0,
createBoard (or any board, column, card or label mutation) fails at
T0+2s and sets error, the v-else-if error branch renders the alert with
a live Retry button, and the click hit the throttle check and returned
before loading was set or any request was made: no skeleton, no request,
a dead button until the window passed.
fetchBoards takes an optional third argument, options: { force?: boolean
}, which skips the throttle check only. BoardsListView routes the mount
read without it and the Retry click with it. Every existing caller keeps
its behaviour: the parameter is optional and defaults to no force. The
boardStore facade passes fetchBoards straight through, so it needs no
change.
force deliberately does NOT bypass the in-flight share, which is checked
first and unconditionally: joining a read already on the wire is the
correct answer to a second caller, and forcing a parallel one would
reintroduce the #1961 fan-out the share removed.
The docblock now states what actually holds instead of the half of it
that did.
Refs #2689, PR #2690.
Round-2 review finding 2. The success-path clear added earlier in this PR was unconditional, so it erased alerts the list read never raised. state.error is one ref shared by every board action: the mount read is slow, the user submits the create form at T+2s, createBoard fails and sets its message, the mount read commits at T+5s and wipes it. The user was told about a failure and then silently was not. At the merge base that alert survived, so this was a regression introduced by the fix for item 4. The guard follows the repo precedent in BoardView.vue, which clears only when the error is still the one it observed. handleApiError now returns the message it wrote (callers that ignore the return are unaffected), the crud store keeps lastListReadError in its closure and sets it from that return in the list read's catch, and a current-generation success clears state.error only when it is still strictly equal to that message. The marker is dropped on ANY current-generation success, matched or not, so a message from an older failed read can never authorise a clear later on. resetForLogout drops it too, since the alert it would authorise clearing is cleared there anyway. The item 4 case stays green: a failing overlapping read followed by a successful one still clears. Not addressed here, and unchanged from the merge base: the clear at the START of a read is still unconditional, so beginning a list read drops another surface's alert. That is pre-existing behaviour every current spec depends on, and it is at least the user asking for fresh data rather than a background commit overwriting them. Refs #2689, PR #2690.
Round-2 review finding 3. Activating Retry unmounts the button that was just activated, because the loading branch replaces the whole error block, and a failed retry rebuilds that block with a brand new button. Focus fell back to body, so a keyboard or screen-reader user had to tab from the top of the page to reach the control they had just used, on every attempt. A template ref on the button plus an await nextTick() after the load settles puts focus back. It runs only in retryLoad, never after the mount read: this follows a user action, so moving focus finishes what the user started rather than stealing it. The ref is null when the retry SUCCEEDED, since the error block is gone by then, so the optional call is also the only-on-failure guard. The alert paragraph is a new node on each failure, so the re-announcement holds independently of the focus move. The spec attaches the wrapper to document.body, because focus and document.activeElement are meaningless for a detached tree, and unmounts it at the end. It asserts the loss as well as the restore: the button is absent and activeElement is body while the read is in flight, then the rebuilt button (a different element from the original) has focus. Refs #2689, PR #2690.
…mocks Three pre-existing handleApiError mockImplementationOnce overrides on the detail-read tests returned void, which vue-tsc rejected once the shared mock took the real helper's string return type. They now mirror the real helper: resolve the message, write it to state, return it. Behaviour of those tests is unchanged; they assert the call arguments and state.error, never the return. Refs #2689, PR #2690.
|
Round-2 record (alpha product-trust lane; fix head Fixed, one commit per finding:
Verified at the fix head: the seven named specs 7 files, 194 passed; Not verified: no browser run; the focus restore is asserted in happy-dom; the forced path is proven with a mocked store and no real 5 s window. Unchanged and recorded in the commit body: the clear at the start of a read is still unconditional (pre-existing; every current spec depends on it). Base: |
|
Round-2 verification record (scoped to the fix diff Verdict: SHIP for the fix diff, no CRITICAL or HIGH. What it checked: Findings, triaged once at the two-round ceiling, all recorded on #2689 as items 6 to 10 rather than a third round:
Round count: 2. Merge gate remaining: |
…s line, SC-10 scope, item-5 count, carry-forward) Answers the docs review of PR #2693: the header points forward to the coordination-lane subsection the coordinator adds after the block lands instead of at one that does not exist yet; a corrections line retires the thirteenth block's present-tense claim that #2629 and #2654 were unmerged; the SC-10 sentence scopes the platform lane's five to its own checkpoint and leaves the ten-PR measurement to the coordinator's sweep record; item 5's caller count is the measured five; the not-shipped line carries the fourteenth block's non-ruling residuals forward; items 5 to 10 named on the #2690 bullet; one bare issue number backticked.
1 participant
Summary
PR #2688 bounded the shared board-list read with
timeout: BOARD_REQUEST_TIMEOUT_MSandskipRetry: true. That bound is now load-bearing on the boards list, and its review named four costs. This PR pays all four. The boards list gets a Retry control, so a one-off 503 or an API restart during mount no longer strands the alert until the user navigates away and back.MetricsView.loadBoardscatches, so a bounded failure is no longer an unhandled rejection out ofonMounted. The board store's own boundary maps an axios timeout and an axios cancel to translated copy, so "timeout of 10000ms exceeded" stops appearing verbatim inside a localized alert. And a list read that succeeds clearsstate.error, so a concurrent fail-then-succeed pair cannot leave the alert standing over a populated list.Frontend only. No API, no shared
utils/errorMessage.ts, no Paper Inbox files.Refs #2689 (items 1 to 4; item 5, the four direct
boardsApi.getBoardscallers, stays open on the issue and is not in this slice). Refs #2685, PR #2688.Changes
1. Retry control on the boards list error state (
src/views/BoardsListView.vue,src/locales/{en,es,it}/boards.ts)The error block now renders the message in a
role="alert"paragraph with a real<button data-action="retry-board-load">beside it, the same shapePaperTriageTableuses.role="alert"moved from the wrapper to the paragraph so the announcement is the sentence alone and the button is a focusable sibling outside the live region, tied to it byaria-describedby. The mount read and the retry share oneloadBoardsfunction, so a retry produces the sequence a mount does: skeleton in flight, then the grid or the alert again. A retry is never swallowed by the store's 5 s throttle, which stamps only after a success and releases the shared in-flight promise on rejection.New keys:
boards.error.retryin all three catalogs, wording mirrored frominbox.triage.boardPick.retry. The same commit landsboards.error.timeoutandboards.error.cancelledso the three catalogs stay structurally parallel in one step; the store starts using them in change 3.2. A caught board-list read in Metrics (
src/views/MetricsView.vue)loadBoardshad atrywith afinallyand nocatch, and is awaited inside an asynconMountedcallback, so the store's rethrow escaped the hook. Vue routes a lifecycle-hook rejection toapp.config.errorHandler; with none installed it rethrows from its own.catch, which reaches the window as anunhandledrejectionand is forwarded to Sentry byinstallWindowErrorListeners. The store already seterrorand toasted, so the catch reports nothing further.boardskeeps its previous value on failure: a failed read learned nothing about the list and must not be reported as empty. Thefinallyis unchanged.3. Translated transport copy at the board store boundary (
src/store/board/boardStoreHelpers.ts)getErrorMessageprefersresponse.data.messagethenerr.message, which is right for an API error and wrong for a transport one: a transport failure carries no server message, soerr.messageis axios' own English.handleApiErrornow answers the two transport shapes from the catalog before falling through togetErrorMessage, so every other error keeps exactly the behaviour it had. Timeouts are detected by the axios codesECONNABORTEDandETIMEDOUT, with a message check behind them for an adapter that reports a timeout without a code; that arm is gated onisAxiosErrorand on the absence of a response, so a server error whose own wording mentions a timeout keeps the server's wording.A cancel is mapped rather than dropped. It should not reach here at all: both read paths return on
axios.isCancelfirst, and the only aborter today (the logout reset) bumps the read generation before aborting. If one ever did, silence would leaveerrornull withboardsstill empty, andBoardsListView'sv-else-ifchain renders that as the EMPTY state, an unconfirmed claim that the account has no boards. That is the #1961 class of lie the shared read exists to prevent, so a short honest line plus the Retry control is the smaller failure.The copy resolves through
i18n.global.trather thanuseI18n(), the pattern recorded inlocales/en/review.tssection 1 and used byuseReviewActions/useReviewProposals/usePaperReviewSelectors: this is a plain factory called from stores and from specs that never mount a component, andi18n.global.tstill reads the live locale, so the copy follows a language switch.src/utils/errorMessage.tsis deliberately untouched, being cross-surface.4. An error clear on the list read's success path (
src/store/board/boardCrudStore.ts)fetchBoardsclearederroron entry to a read but never on success. Two list reads genuinely overlap, because a filtered (includeArchived) read never joins the share and never populates it: the activity selector's can still be on the wire when the boards list mounts its unfiltered one. When the failing half writes its alert after the surviving half has already cleared on entry,errorwas left set beside a populatedboards, and thev-if loading / v-else-if error / ... / v-else gridchain showed the alert instead of the boards it already held. The clear sits after the generation check, so a superseded read still writes nothing.Test plan
Verified, all from
frontend/taskdeck-web:npx vitest --run --maxWorkers=2 src/tests/views/BoardsListView.spec.ts src/tests/views/MetricsView.spec.ts src/tests/views/MetricsView.coverage.spec.ts src/tests/store/board/boardCrudStore.spec.ts src/tests/store/board/boardStoreHelpers.spec.ts src/tests/store/boardStore.spec.ts src/tests/i18n/catalogs.spec.tsgives Test Files 7 passed (7), Tests 187 passed (187). Seven paths named, seven files run.npx vitest --run --maxWorkers=2 src/tests/store src/tests/i18ngives Test Files 51 passed (51), Tests 906 passed (906). The whole store tree, because thehandleApiErrorchange sits on every board-store failure path.npx vitest --run --maxWorkers=2 src/tests/viewsgives Test Files 78 passed (78), Tests 1422 passed (1422).MetricsView.coverage.spec.ts(held by PR test: wait for MetricsView error alert #2653) is inside that run and is green and untouched.npm run typecheckclean.npx eslintover all eleven changed files: no output, no warnings.git diff --check origin/main..HEADclean.Red-first. Every new assertion was run against the unmodified source (the four source files and the three catalogs restored to
origin/main, the specs kept). Result: Test Files 4 failed (4), Tests 9 failed | 110 passed (119).BoardsListView.spec.ts, three failures: "renders a focusable Retry button beside the alert" (no[data-action="retry-board-load"]element exists); "re-issues the read, shows the skeleton in flight, and leaves the error state on success"; "shows the alert again, still retryable, when the retry also fails".MetricsView.spec.ts, one failure: "mounts through a rejected board-list read without escaping the mounted hook" fails withexpected "vi.fn()" to not be called at all, but actually been called 1 times, the app error handler receiving the escaped rejection.boardStoreHelpers.spec.ts, four failures: the timeout mapping, theitlocale check, theETIMEDOUTcode, and the cancel mapping, eachexpected 'Failed to fetch boards' to bethe catalog copy.boardCrudStore.spec.ts, one failure: "clears an overlapping failure's alert when a current-generation read succeeds" fails withexpected 'Failed to fetch boards' to be null.One new test is a guard rather than a red-first assertion and passes both before and after: "leaves a server error message alone even when it mentions a timeout", which pins the response gate on the message arm of the timeout check.
How the Metrics assertion works, since it is not obvious: it installs an
app.config.errorHandlerspy through the mount options and asserts it is never called. Vue intercepts a lifecycle-hook rejection before it can reach the window, so a process-levelunhandledrejectionlistener would not observe anything; the app error handler is the first thing Vue reaches on that route, which makes "never called" precisely "the rejection did not escape the callback".NOT verified:
.paper-triage__retry.npm run build.vue-tsc -band the mounted-component specs cover the changed templates, but the Vite production build was not run.role="alert"move. The DOM relationship (alert text alone,aria-describedbyfrom the button to the alert's id) is asserted; the announcement itself is not.MACHINE_TRANSLATED_LOCALESand Seed an i18n translation layer (vue-i18n) with Italian and Spanish locales #1770. Structural parity is proven bycatalogs.spec.ts; tone and correctness are not.ETIMEDOUTarm is covered by a syntheticAxiosError, not by a real axios request withtransitional.clarifyTimeoutErroron.Boundaries and risks
Changed files, all frontend and all inside the claimed set:
src/views/BoardsListView.vue,src/views/MetricsView.vue,src/store/board/boardStoreHelpers.ts,src/store/board/boardCrudStore.ts,src/locales/{en,es,it}/boards.ts, and the four matching specs. Not touched:src/utils/errorMessage.ts(cross-surface, out of lane),src/api/**, the Paper Inbox files (PR #2654),src/tests/views/MetricsView.coverage.spec.ts(PR #2653).Two decisions a reviewer should weigh.
The cancel is mapped rather than skipped, for the reason in change 3. The alternative reading, that a cancel is the app's own decision and deserves silence, is defensible for the detail surface but not for the list one, where silence renders as a false empty state.
The detail read has the same missing success-path clear and is deliberately left alone. Fixing it is not the one line the list needed: a successful BACKGROUND detail refresh must not clear an alert it does not own, so the change needs an intent gate, and the concurrent pair item 4 describes is far less reachable there because the detail path supersedes itself. It is called out here rather than fixed silently.
Risk on change 4:
erroris a single shared ref across the list and detail surfaces, so a succeeding list read now also clears an alert a detail failure set. That is last-writer-wins either way, and the issue accepts it for the list; a per-surface error ref would be a larger refactor than this slice.Risk on change 3:
boards.error.timeoutandboards.error.cancelledlive in theboardsnamespace but can surface on any screen the board store backs, sincehandleApiErroris shared. There is no common namespace insrc/locales, andboardDetail.tsis a different owner's surface, so a duplicate key there would be worse. The catalog comments in all three locales say so explicitly for the next translator.Docs: no canonical doc changed. This restores intended behaviour on an existing surface rather than shipping new capability, so
docs/STATUS.md,docs/IMPLEMENTATION_MASTERPLAN.mdanddocs/decisions/were left alone; flag it if the lane reads it otherwise.Worktree: built in
.worktrees/codex-2689-boards-retry. The only gitignored content isfrontend/taskdeck-web/node_modulesfromnpm ci, which is reproducible and nothing was copied out. Ready for a plaingit worktree removeonce this lands.