fix(boards): guard the retry focus restore, and make the throttle and guarded-clear comments true - #2703
Conversation
The retry read is bounded at 10 s and the create panel renders above the loading chain, so it stays interactive while the retry is in flight. The unconditional restore pulled the caret out of the new-board name input the moment the read failed, and the next Space or Enter re-fired Retry instead of typing. The restore is now guarded on document.activeElement being null or the body, which is where the browser leaves focus after the activated button unmounts. A red-first spec opens the create panel and focuses the name input while the forced retry is pending, then rejects the read and asserts the input keeps focus; the existing spec that asserts the restore when focus was lost is unchanged. Refs #2689
…he code does The throttle docblocks in boardCrudStore.ts, BoardsListView.vue and boardCrudStore.spec.ts claimed a retry after a failed read was never blocked by the window. A stamp left by an earlier success survives every later failure, including a filtered read's failure that writes the shared error, so only force guarantees a request and the retry path always forces. The comment beside the guarded clear now names the collapsed-copy collision: every client timeout maps to one catalog string and every offline failure to axios's Network Error, so two surfaces can write byte-identical messages and a message compare cannot separate them. The store spec's forced-read comment no longer promises a skeleton over an assertion every settled read satisfies; it names the request count as the load-bearing assertion and points at the view spec that proves the skeleton. No behaviour change. Refs #2689
BoardListFetchOptions is now re-exported from store/board/index.ts beside BoardFetchOptions, and BoardsListView imports it type-only instead of declaring an inline { force?: boolean }. A second list option now propagates to the view's load signature instead of being silently dropped.
Refs #2689
|
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 merge-blocking items; one MEDIUM and two LOWs are wording in the comments this PR exists to make true and are fixed in round 2 (comments only). Lenses that found nothing: the focus guard reads Findings, triaged once:
Round count: 2 after the fix push. Merge gate: |
…on comments Review round 2 on PR #2703, three findings, all in the comments this PR exists to make true. No code line changes. MEDIUM: the collision example named createBoard timing out, which cannot happen. boardsApi.createBoard is a bare http.post and the axios instance sets no default timeout, so a mutation never produces boards.error.timeout. The example is now the offline path, where any failure without a response yields axios's Network Error for reads and mutations alike, and the timeout half is attributed to the two bounded reads that can actually write that string. The comment now says outright which actor is not available. LOW: only force guarantees a request was false, since the in-flight share check runs before the throttle and is deliberately not skipped by force, and demo mode returns without a request. All three copies now say only force gets past the throttle, the in-flight share and demo mode still apply. LOW: leaving the unfiltered stamp intact held only for a filtered failure. A filtered success writes lastFetchBoardsAt like any other success, since that assignment is not gated on isFilteredRequest. All three copies now separate the two cases. Refs #2689
|
Round-2 record (alpha product-trust lane; fix head Fixed, comments only (a Verified at the fix head: the four named suites 4 files, 131 passed; Not verified: the documented collisions are not driven by a spec; the focus sentinel's real-shell check is recorded on #2689. Merge gate remaining: |
…pointers, #2701 record, #2630 correction clause) Answers the docs review of PR #2704: the #2703 bullet no longer says only force guarantees a request (it gets past the throttle only; the in-flight share and demo mode still apply, the very overclaim round 2 removed from the code); the #2700 and #2703 bullets name every open residual the not-shipped line names; the #2701 pointer says the sixteenth block's subsection is its record; the #2700 bullet says its round 2 touched copy and specs only; the corrections line quotes the thirteenth block's clause that the pinned warning itself sticks.
Summary
The round-2 verification residuals of PR #2690 on the boards list and the board store, items 6 to 10 of #2689. One is a behaviour fix: the focus restore after a failed retry no longer steals the caret from the create form the user moved into while the retry was on the wire. The other four make comments and a type say what the code actually does. Item 5 of #2689 (the five direct
boardsApi.getBoardscallers) is a separate per-caller decision and stays open.Refs #2689 (items 6 to 10; item 5 stays open)
Refs PR #2690
Changes
fix(boards): restore focus after a failed retry only when focus was lost(item 6).retryLoad()inBoardsListView.vuefocused the rebuilt Retry button after every failed retry. The read is bounded at 10 s and the create panel renders above the loading chain, so it stays interactive for that whole wait: a user who opened "+ New Board" and started typing had the caret pulled back to the button when the read failed, and the next Space or Enter re-fired Retry instead of typing. The restore is now guarded ondocument.activeElementbeingnullordocument.body, which is where the browser leaves focus after the activated button unmounts. A red-first spec opens the create panel and focuses the name input while the forced retry is pending, rejects the read, and asserts the input keeps focus; the existing spec asserting the restore when focus was lost is unchanged and still passes.docs(boards): make the throttle and guarded-clear comments say what the code does(items 7, 8, 10). The throttle docblocks inboardCrudStore.ts,BoardsListView.vueandboardCrudStore.spec.tsclaimed that a retry following a failed list read was never blocked by the window. A stamp written by an earlier success survives every later failure, including a filtered read's failure, which writes the sharedstate.errorwithout touching the unfiltered stamp, so onlyforceguarantees that a request goes out and the retry path always forces. The comment beside the guarded clear now names the collapsed-copy collision: every client timeout maps to the oneboards.error.timeoutstring and every offline failure to axios's "Network Error", so two surfaces routinely write byte-identical messages and a message compare cannot separate them; the concrete case is a list-read timeout followed by acreateBoardtimeout during the forced retry, whose alert the retry's success can clear (the create's toast survives). The store spec's forced-read comment no longer promises "the skeleton the view needs" overexpect(state.loading.value).toBe(false), which every settled read satisfies: it names the request count as the load-bearing assertion and points at theBoardsListView.spec.tscase that proves the skeleton. No behaviour change in this commit.refactor(boards): give the boards list the store's own list-option type(item 9).BoardListFetchOptionsis re-exported fromstore/board/index.tsbesideBoardFetchOptions, andBoardsListView.vueimports it type-only instead of declaring an inline{ force?: boolean }. A second list option now propagates to the view's load signature instead of being silently dropped.Test plan
Verified, all from
frontend/taskdeck-web:npx vitest --run --maxWorkers=2 src/tests/views/BoardsListView.spec.tswith the new spec added and the guard not yet written. Test Files 1 failed, Tests 1 failed | 19 passed (20). The failure is exactly the reported defect:expected <button data-action="retry-board-load"> to be <input id="new-board-name">at the finalexpect(document.activeElement).toBe(nameInput), that is, the unconditional restore moved the caret off the create input onto the rebuilt Retry button. After the guard: Test Files 1 passed, Tests 20 passed (20).npx vitest --run --maxWorkers=2 src/tests/views/BoardsListView.spec.ts src/tests/store/board/boardCrudStore.spec.ts src/tests/store/boardStore.spec.ts src/tests/views/paper/boardMutationCapabilityParity.spec.tsat the final tree: Test Files 4 passed (4), Tests 131 passed (131). Four spec paths named, four files run.npm run typecheck(vue-tsc -b): clean, exit 0. It proves item 9's shape rather than merely compiling: a deliberate probe changing the single call site toloadBoards({ forceX: true })producedTS2561: Object literal may only specify known properties, but 'forceX' does not exist in type 'BoardListFetchOptions', and the probe was reverted before the commit.npx eslinton the five changed files: exit 0, no output.git diff --check: clean.NOT verified: no browser or e2e run, so the guard is proven in jsdom only, where
document.activeElementafter an unmount falls back todocument.bodyexactly as the existing round-2 spec already relied on; a real browser leaves it atbodyin the same situation but that was not exercised here. No backend test run and no full frontend suite run: the change is five frontend files and nothing outside the boards list and the board store's list path is reachable from it. The collapsed-copy collision described in the item 8 comment is documented, not tested and not fixed; separating those messages needs an owner tag on the error surface rather than a string compare, which is a wider change than this seam.Boundaries and risks
Files changed:
src/views/BoardsListView.vue,src/store/board/boardCrudStore.ts(one docblock, one comment, no logic),src/store/board/index.ts(one re-export),src/tests/views/BoardsListView.spec.ts,src/tests/store/board/boardCrudStore.spec.ts. NoMetricsView.vue, noboardStoreHelpers.ts, no locale change, no API change, no doc change.The only behaviour change is the focus guard, and it is a narrowing: focus is now moved in a strict subset of the cases where it was moved before, so no path that previously kept the caret can lose it. The case it gives up is the one where something else legitimately took focus during the retry and the user would still have preferred to be returned to the button; the alert paragraph is a new node on each failure and announces independently of focus, so the failure is still reported to a screen-reader user.
One observation for the record, not a change:
BoardView.vue's guarded clear at lines 88 to 101, cited as the precedent for item 8's granularity, carries no comment of its own. The precedent followed here is its code shape.