Skip to content

fix(boards): guard the retry focus restore, and make the throttle and guarded-clear comments true - #2703

Merged
Chris0Jeky merged 4 commits into
mainfrom
issue-2689/followups
Sep 5, 2026
Merged

fix(boards): guard the retry focus restore, and make the throttle and guarded-clear comments true#2703
Chris0Jeky merged 4 commits into
mainfrom
issue-2689/followups

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

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.getBoards callers) 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() in BoardsListView.vue focused 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 on document.activeElement being null or document.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 in boardCrudStore.ts, BoardsListView.vue and boardCrudStore.spec.ts claimed 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 shared state.error without touching the unfiltered stamp, so only force guarantees 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 one boards.error.timeout string 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 a createBoard timeout 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" over expect(state.loading.value).toBe(false), which every settled read satisfies: it names the request count as the load-bearing assertion and points at the BoardsListView.spec.ts case 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). BoardListFetchOptions is re-exported from store/board/index.ts beside BoardFetchOptions, and BoardsListView.vue 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.

Test plan

Verified, all from frontend/taskdeck-web:

  • Red first: npx vitest --run --maxWorkers=2 src/tests/views/BoardsListView.spec.ts with 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 final expect(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.ts at 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 to loadBoards({ forceX: true }) produced TS2561: Object literal may only specify known properties, but 'forceX' does not exist in type 'BoardListFetchOptions', and the probe was reverted before the commit.
  • npx eslint on 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.activeElement after an unmount falls back to document.body exactly as the existing round-2 spec already relied on; a real browser leaves it at body in 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. No MetricsView.vue, no boardStoreHelpers.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.

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
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review record (alpha product-trust lane, review-and-ship round 1 at head 6e9eaa3c6).

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 document.activeElement after nextTick, against the rebuilt DOM, so the #2690 restore still fires when the old button unmounted (the pre-existing spec pins that intermediate state) and no restore fires when the user moved into the create form; document is touched only inside the click handler; the throttle docblock's claims hold against the store (the stamp written only on the success path after the generation check, cleared only by the logout reset, force gating only the throttle line, the filtered actor real in the activity selector); the guarded clear's collision claim holds (all client timeouts collapse to one catalog string, offline failures to axios's "Network Error", the create's toast survives); the re-export is type-only with two consumers and no runtime edge; the new spec is red-first with the rebuilt button bound; scope is the five files.

Findings, triaged once:

  • MEDIUM, fixed in round 2: the guarded clear's "Concretely" example named createBoard timing out, which cannot happen (no client-side timeout on mutations); the example names a reachable actor (the offline path or a bounded sibling read).
  • LOW, fixed in round 2: "only force guarantees a request" is false as written (the in-flight share and demo mode still apply, as the same docblocks say later); "leaving the unfiltered stamp intact" holds for a filtered failure only, since a filtered success writes the stamp.
  • LOW, recorded on [Frontend][Board] Boards list needs a Retry control and error hygiene now that the list read is bounded (follow-ups from the #2688 review) #2689: the body-or-null focus sentinel is proven against a test DOM without the app shell's focusable main element; a Playwright leg on the real shell settles whether an engine could land focus on that ancestor instead.

Round count: 2 after the fix push. Merge gate: CI green at the fix head plus the three-minute age; no second pass is owed (comments only).

…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
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Round-2 record (alpha product-trust lane; fix head a5bbcbc2e, one comment-only commit on top of 6e9eaa3c6; origin/main unmoved at 27b0e9191).

Fixed, comments only (a git diff -U0 filtered to non-comment lines is empty): the guarded clear's "Concretely" example is now the offline path (any failure with no response surfaces axios's "Network Error" for reads and mutations alike), with the timeout half attributed to the two bounded reads that write the shared error (this list read and the detail read, both carrying BOARD_REQUEST_TIMEOUT_MS), and the comment says a mutation cannot supply the timeout half because mutations carry no client timeout; "only force guarantees a request" reads "only force gets past the throttle; the in-flight share and demo mode still apply" in all three copies; "leaving the unfiltered stamp intact" reads that a filtered failure leaves the stamp untouched while a filtered success writes it like any success, in all three copies (the store spec's copy included).

Verified at the fix head: the four named suites 4 files, 131 passed; npm run typecheck clean; eslint on the three changed files clean; git diff --check clean.

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: CI green at a5bbcbc2e and the three-minute age. Round count: 2; no second pass is owed (comments only).

@Chris0Jeky
Chris0Jeky merged commit f74514f into main Sep 5, 2026
35 checks passed
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 5, 2026
Chris0Jeky added a commit that referenced this pull request Sep 5, 2026
…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.
@Chris0Jeky
Chris0Jeky deleted the issue-2689/followups branch September 6, 2026 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant