diff --git a/frontend/taskdeck-web/src/store/board/boardCrudStore.ts b/frontend/taskdeck-web/src/store/board/boardCrudStore.ts index 45f5e391f3..120616c30a 100644 --- a/frontend/taskdeck-web/src/store/board/boardCrudStore.ts +++ b/frontend/taskdeck-web/src/store/board/boardCrudStore.ts @@ -28,14 +28,24 @@ export interface BoardListFetchOptions { * Skip the throttle window, and NOTHING else — an explicit user request for * fresh data, which the 5 s gap between mounts was never meant to answer. * - * The stamp is written only after a success, so a retry that follows a FAILED - * list read was never blocked by it. What this exists for is the stamp an - * EARLIER success left behind: `state.error` is shared by every board action, - * so a create/rename/archive failure two seconds after a good list read puts - * BoardsListView on its error branch with a Retry control, and without this - * the click returned here before touching `loading` or issuing a request — - * no skeleton, no request, a dead button until the window passed (#2689 - * round-2 finding 1). + * The stamp is written only after a success, but that is not the same as + * "a failure reopens the window": a stamp an EARLIER success left behind + * outlives every later failure, so the window is open for the whole 5 s + * regardless of what happened in between. Two ways in: `state.error` is + * shared by every board action, so a create/rename/archive failure two + * seconds after a good list read puts BoardsListView on its error branch + * with a Retry control; and a FILTERED list read (the activity selector's + * `includeArchived` one) writes the same shared `error` when it fails, and + * a filtered FAILURE leaves the stamp untouched — though a filtered success + * writes it like any other success, since `lastFetchBoardsAt` below is not + * gated on `isFilteredRequest`. + * + * So only `force` gets past the THROTTLE; the in-flight share and demo mode + * still apply, which is why this is a skipped window rather than a + * guaranteed request. That is enough for the retry path, which always passes + * it. Without it the click returned here before touching `loading` or + * issuing a request: no skeleton, no request, a dead button until the window + * passed (#2689 round-2 finding 1, docblock corrected in #2689 item 7). * * Deliberately NOT a bypass of the in-flight share: joining a read that is * already on the wire is the correct answer to a second caller, and forcing @@ -174,6 +184,32 @@ export function createBoardCrudActions(state: BoardState, helpers: BoardHelpers) // error still identical to the one this path observed. The marker is // dropped on any current-generation success, matched or not, so a stale // message can never authorise a later clear. + // + // The documented limitation, at the same granularity as that + // precedent: the guard compares MESSAGES, and the copy collapses. Every + // client-side timeout maps to the one `boards.error.timeout` string and + // every offline failure to axios's "Network Error" (no response, so + // `getErrorMessage` falls through to `err.message`), for reads and + // mutations alike, so two different surfaces routinely produce + // byte-identical text and this comparison cannot tell them apart. + // + // Concretely, offline: this list read fails with "Network Error", the + // user submits the create form, `createBoard` fails with the same two + // words during the forced retry, and the retry's success then clears an + // alert the list read did not raise. The timeout string collides the + // same way between the two BOUNDED reads — this one and the detail read + // in `startBoardFetch`, both carrying `BOARD_REQUEST_TIMEOUT_MS` — since + // both write `boards.error.timeout` into the one shared `error`. Note + // which actor is NOT available for that half: `boardsApi.createBoard` + // is a bare `http.post` and the axios instance sets no default timeout + // (see the bound's own comment above), so a mutation can never produce + // the timeout string — only the offline string. + // + // It is not exotic; offline is the ordinary case. Distinguishing them + // needs an owner tag on the error surface rather than a string compare, + // which is a wider change than this seam (#2689 item 8); the losing + // surface's toast survives either way, so the failure is still + // reported. const listReadErrorToClear = lastListReadError lastListReadError = null if (listReadErrorToClear !== null && state.error.value === listReadErrorToClear) { diff --git a/frontend/taskdeck-web/src/store/board/index.ts b/frontend/taskdeck-web/src/store/board/index.ts index 0009e4975b..e7b03a0137 100644 --- a/frontend/taskdeck-web/src/store/board/index.ts +++ b/frontend/taskdeck-web/src/store/board/index.ts @@ -3,7 +3,11 @@ export type { CardFilters, BoardState } from './boardState' export { createBoardHelpers } from './boardStoreHelpers' export type { BoardHelpers } from './boardStoreHelpers' export { createBoardCrudActions } from './boardCrudStore' -export type { BoardFetchIntent, BoardFetchOptions } from './boardCrudStore' +export type { + BoardFetchIntent, + BoardFetchOptions, + BoardListFetchOptions, +} from './boardCrudStore' export { createColumnActions } from './columnStore' export { createCardActions } from './cardStore' export { createCardCommentActions } from './cardCommentStore' diff --git a/frontend/taskdeck-web/src/tests/store/board/boardCrudStore.spec.ts b/frontend/taskdeck-web/src/tests/store/board/boardCrudStore.spec.ts index 0abfc48e48..272ce2c0bd 100644 --- a/frontend/taskdeck-web/src/tests/store/board/boardCrudStore.spec.ts +++ b/frontend/taskdeck-web/src/tests/store/board/boardCrudStore.spec.ts @@ -187,12 +187,16 @@ describe('boardCrudStore', () => { }) // #2689 round-2 finding 1. The throttle stamp is written only after a - // SUCCESS, so a retry following a failed read was never blocked by it — but - // the stamp an EARLIER success left behind is a different matter. `error` - // is shared by every board action, so the boards list can be sitting on its - // error branch with a live Retry control (a create/rename/archive failure) - // while this window is still open. An explicit retry has to get through; - // an ordinary mount still must not. + // SUCCESS, but a later failure does not reopen the window: the stamp an + // EARLIER success left behind survives it, including a filtered read's + // failure, which writes the shared `error` while leaving the stamp + // untouched (a filtered SUCCESS writes it like any other success). Only + // `force` gets past the throttle — the in-flight share and demo mode still + // apply. `error` is shared by every board action, so the boards list can be + // sitting on its error branch with a live Retry control (a + // create/rename/archive failure) while this window is still open. An + // explicit retry has to get through; an ordinary mount still must not. + // (Comment corrected in #2689 item 7.) it('lets a forced read through the throttle window while an unforced one is still skipped', async () => { vi.useFakeTimers() mockBoardsApi.getBoards.mockResolvedValue([{ id: 'board-1', name: 'My Board' }]) @@ -206,7 +210,12 @@ describe('boardCrudStore', () => { await fetchBoards() expect(mockBoardsApi.getBoards).toHaveBeenCalledTimes(1) - // Same window, forced: a real request, and the skeleton the view needs. + // Same window, forced: a real request goes out. The request count is the + // load-bearing assertion here; `loading` back at false only says the read + // settled, which every settled read satisfies (#2689 item 10). That the + // view actually SHOWS a skeleton while a forced retry is in flight is + // proven in BoardsListView.spec.ts, "forces past the throttle window when + // the alert came from another action after a good read". await fetchBoards(undefined, false, { force: true }) expect(mockBoardsApi.getBoards).toHaveBeenCalledTimes(2) expect(state.loading.value).toBe(false) diff --git a/frontend/taskdeck-web/src/tests/views/BoardsListView.spec.ts b/frontend/taskdeck-web/src/tests/views/BoardsListView.spec.ts index 0de062d91e..224c221b4a 100644 --- a/frontend/taskdeck-web/src/tests/views/BoardsListView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/BoardsListView.spec.ts @@ -262,6 +262,62 @@ describe('BoardsListView', () => { wrapper.unmount() }) + // #2689 item 6, the other half of that restore. The retry read is bounded + // at 10 s and the create panel sits ABOVE the loading chain, so it stays + // interactive while the retry is in flight: the user can open "+ New Board" + // and start typing during those ten seconds. Restoring unconditionally then + // pulled the caret out of the name input the moment the read failed, and + // the next Space or Enter re-fired Retry instead of typing. The restore + // exists for focus that was LOST, so it is guarded on + // `document.activeElement` being null or . + it('leaves the caret alone when the user moved into the create form during the retry', async () => { + mockBoardStore.error = 'Failed to load boards' + + const wrapper = mount(BoardsListView, { attachTo: document.body }) + await waitForUi() + + const firstButton = wrapper.find('[data-action="retry-board-load"]') + .element as HTMLButtonElement + firstButton.focus() + expect(document.activeElement).toBe(firstButton) + + let failRead!: () => void + mockBoardStore.fetchBoards.mockImplementation(() => { + mockBoardStore.loading = true + return new Promise((_resolve, reject) => { + failRead = () => { + mockBoardStore.error = 'Failed to load boards' + mockBoardStore.loading = false + reject(new Error('still failing')) + } + }) + }) + + await wrapper.find('[data-action="retry-board-load"]').trigger('click') + + // The read is on the wire and the error block is gone with the button + // that was focused. The user opens the create panel and puts the caret in + // the name input. + const newBoardBtn = wrapper.findAll('button').find((b) => b.text().includes('+ New Board')) + expect(newBoardBtn).toBeDefined() + await newBoardBtn!.trigger('click') + await waitForUi() + const nameInput = wrapper.find('#new-board-name').element as HTMLInputElement + nameInput.focus() + expect(document.activeElement).toBe(nameInput) + + failRead() + await flushPromises() + + // The retry failed and the error block was rebuilt with a new Retry + // button, but focus was never lost — so it stays where the user put it. + const rebuiltButton = wrapper.find('[data-action="retry-board-load"]') + expect(rebuiltButton.exists()).toBe(true) + expect(document.activeElement).toBe(nameInput) + + wrapper.unmount() + }) + it('shows the alert again, still retryable, when the retry also fails', async () => { mockBoardStore.error = 'Failed to load boards' diff --git a/frontend/taskdeck-web/src/views/BoardsListView.vue b/frontend/taskdeck-web/src/views/BoardsListView.vue index 9288bbc175..fcae9d5f5c 100644 --- a/frontend/taskdeck-web/src/views/BoardsListView.vue +++ b/frontend/taskdeck-web/src/views/BoardsListView.vue @@ -3,6 +3,11 @@ import { computed, nextTick, onMounted, ref } from 'vue' import { useRouter } from 'vue-router' import { useI18n } from 'vue-i18n' import { useBoardStore } from '../store/boardStore' +// Type-only, so the barrel adds no runtime import here: the view's load +// signature is the store's own list-option type rather than a hand-copied +// `{ force?: boolean }` that a second option would silently leave behind +// (#2689 item 9). +import type { BoardListFetchOptions } from '../store/board' import { logError } from '../utils/errorReporting' import { TdSkeleton } from '../components/ui' import PaperHLBtn from '../components/paper/PaperHLBtn.vue' @@ -58,7 +63,7 @@ function formatCreatedAt(createdAt: string): string { * retry layer, and without a control the alert stayed until the user navigated * away and back (#2689 item 1). */ -async function loadBoards(options: { force?: boolean } = {}) { +async function loadBoards(options: BoardListFetchOptions = {}) { // Catch the rethrown error — boardStore.error is already set by handleApiError // so the template can display it. Without this catch, Vue treats the unhandled // rejection as a lifecycle-hook error and may tear down the component. @@ -72,16 +77,22 @@ onMounted(() => { /** * The Retry click. `force` skips the store's throttle window and nothing else. * - * The stamp is written only after a success, so a retry that follows a FAILED - * list read was never blocked by it — the earlier docblock here stated that as - * if it settled the question, and it does not. `state.error` is shared by every - * board action, so a create/rename/archive failure two seconds after a good - * list read puts this view on its error branch with a live Retry button while - * the throttle window from THAT success is still open. Unforced, the click - * returned inside the store before `loading` was touched or any request was - * made: no skeleton, no request, a dead button until the window passed (#2689 - * round-2 finding 1). The in-flight share is still respected — `force` does not - * bypass it, so a click during a read already on the wire joins that read. + * The stamp is written only after a success, but a failure does not reopen the + * window: a stamp an earlier success left behind survives every later failure, + * including a filtered read's (the activity selector's `includeArchived` read + * writes the same shared `state.error` when it FAILS without touching the + * stamp; a filtered success writes the stamp like any other success). + * So only `force` gets past the THROTTLE — the in-flight share and demo mode + * still apply, so it is a skipped window rather than a guaranteed request — + * and this path always forces. `state.error` is shared by every board action, + * so a create/rename/archive failure two seconds after a good list read puts + * this view on its error branch with a live Retry button while the throttle + * window from THAT success is still open. Unforced, the click returned inside + * the store before `loading` was touched or any request was made: no skeleton, + * no request, a dead button until the window passed (#2689 round-2 finding 1; + * this docblock corrected in #2689 item 7). The in-flight share is still + * respected — `force` does not bypass it, so a click during a read already on + * the wire joins that read. */ async function retryLoad() { await loadBoards({ force: true }) @@ -98,8 +109,20 @@ async function retryLoad() { // block is gone — so the optional call is also the "only on failure" guard. // The alert paragraph is a new node on each failure, so it is announced // again independently of this. + // + // The second guard is what keeps "restore" from meaning "steal". The read is + // bounded at 10 s and the create panel is rendered ABOVE the loading chain, + // so it stays interactive for the whole wait: a user who opens "+ New Board" + // and starts typing during a hung retry had the caret yanked back to the + // rebuilt button when the read finally failed, and their next Space or Enter + // re-fired Retry instead of typing (#2689 item 6). Focus is only put back + // when it was actually LOST — `document.activeElement` null or , which + // is where the browser leaves it after the activated button unmounts. await nextTick() - retryButton.value?.focus() + const focused = document.activeElement + if (focused === null || focused === document.body) { + retryButton.value?.focus() + } } async function createBoard() {