TST-44: Frontend store integration tests#816
Conversation
…ancelItem, and triage polling lifecycle
…d, and column CRUD
… board-scoped filtering
…and error handling
Self-review findingsReviewed all 6 changed files against the adversarial checklist: Mock levelAll tests mock at the HTTP layer ( State transition verificationEvery test verifies both:
No test just calls an action without assertions. Error path coverageEach store action tested has both success and failure paths:
Test isolation
Boundary testing accuracy
Verdict: No issues found. All tests are clean. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive integration tests for several stores and API modules, including archiveApi, boardStore, captureStore, notificationStore, queueStore, and sessionStore. The review feedback suggests using more idiomatic Vitest assertions, such as toHaveBeenCalledWith with expect.stringContaining instead of manual mock call extraction, and recommends verifying the exact number of API calls in polling tests to ensure logic correctness.
| const calledUrl = vi.mocked(http.get).mock.calls[0][0] as string | ||
| expect(calledUrl).toContain('entityType=card') |
There was a problem hiding this comment.
Instead of manually extracting the URL from the mock calls, use expect(http.get).toHaveBeenCalledWith(expect.stringContaining(...)). This is more idiomatic in Vitest and avoids brittle index-based access to mock calls.
| const calledUrl = vi.mocked(http.get).mock.calls[0][0] as string | |
| expect(calledUrl).toContain('entityType=card') | |
| expect(http.get).toHaveBeenCalledWith(expect.stringContaining('entityType=card')) |
| const calledUrl = vi.mocked(http.get).mock.calls[0][0] as string | ||
| expect(calledUrl).toContain('entityType=card') | ||
| expect(calledUrl).toContain('boardId=board-1') | ||
| expect(calledUrl).toContain('limit=50') |
There was a problem hiding this comment.
When testing multiple query parameters, you can use multiple toHaveBeenCalledWith assertions to verify the presence of all required parameters in the URL. This is cleaner than manual string extraction.
| const calledUrl = vi.mocked(http.get).mock.calls[0][0] as string | |
| expect(calledUrl).toContain('entityType=card') | |
| expect(calledUrl).toContain('boardId=board-1') | |
| expect(calledUrl).toContain('limit=50') | |
| expect(http.get).toHaveBeenCalledWith(expect.stringContaining('entityType=card')) | |
| expect(http.get).toHaveBeenCalledWith(expect.stringContaining('boardId=board-1')) | |
| expect(http.get).toHaveBeenCalledWith(expect.stringContaining('limit=50')) |
| // After reaching 'Triaged' (terminal), polling should have stopped | ||
| expect(store.triagePollingItemId).toBeNull() | ||
| expect(store.detailById['c-poll']?.status).toBe('Triaged') |
There was a problem hiding this comment.
It is recommended to verify the number of API calls made during polling. This confirms that the polling logic correctly executed the expected number of attempts before reaching the terminal status.
| // After reaching 'Triaged' (terminal), polling should have stopped | |
| expect(store.triagePollingItemId).toBeNull() | |
| expect(store.detailById['c-poll']?.status).toBe('Triaged') | |
| // After reaching 'Triaged' (terminal), polling should have stopped | |
| expect(http.get).toHaveBeenCalledTimes(3) | |
| expect(store.triagePollingItemId).toBeNull() | |
| expect(store.detailById['c-poll']?.status).toBe('Triaged') |
There was a problem hiding this comment.
Pull request overview
Adds/extends Vitest + Pinia integration tests that exercise the real store→API-module→mocked-HTTP boundary to improve coverage for common success/error paths and regressions.
Changes:
- Extend store integration suites (capture/board/notification/session/queue) with additional actions and error/edge-case scenarios.
- Add a new
archiveApiintegration test suite validating query building, URL encoding, and error propagation. - Add coverage for lifecycle behaviors like triage polling and loading-state transitions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/tests/store/sessionStore.integration.spec.ts | Adds integration coverage for changePassword and isAuthenticated behavior with expired vs valid tokens. |
| frontend/taskdeck-web/src/tests/store/queueStore.integration.spec.ts | Adds tests for loading-state transitions and cancel-request isolation behavior. |
| frontend/taskdeck-web/src/tests/store/notificationStore.integration.spec.ts | Adds unread-count behavior checks and board-scoped mark-all-read expectations. |
| frontend/taskdeck-web/src/tests/store/captureStore.integration.spec.ts | Adds server-error coverage for create, query param forwarding, cancel action, and triage polling lifecycle tests. |
| frontend/taskdeck-web/src/tests/store/boardStore.integration.spec.ts | Adds tests for 409 handling on move, deleteCard behavior, and column create/delete flows. |
| frontend/taskdeck-web/src/tests/store/archiveApi.integration.spec.ts | New integration suite for archiveApi.getItems and archiveApi.restoreItem including encoding + error propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('retains original column and position when move API rejects with 409 (snap-back)', async () => { | ||
| const store = useBoardStore() | ||
| const card = makeCardPayload({ id: 'card-snap', columnId: 'col-1', position: 0 }) | ||
| store.currentBoardCards = [card] | ||
|
|
||
| vi.mocked(http.post).mockRejectedValue({ response: { status: 409, data: { message: 'Stale position' } } }) | ||
|
|
||
| await expect(store.moveCard('board-1', 'card-snap', 'col-2', 3)).rejects.toBeDefined() | ||
|
|
||
| // The card should still be in the store (not removed) and in the original column | ||
| const storedCard = store.currentBoardCards.find(c => c.id === 'card-snap') | ||
| expect(storedCard).toBeDefined() | ||
| expect(storedCard?.columnId).toBe('col-1') | ||
| expect(storedCard?.position).toBe(0) | ||
| }) |
There was a problem hiding this comment.
This test is labeled as a 409 “snap-back” regression, but moveCard in the store does not perform an optimistic local move before awaiting the API response (it only mutates state after a successful cardsApi.moveCard). As written, the assertion will pass even if no rollback/snap-back behavior exists. Either rename the test to reflect the actual contract (no local mutation on failure) or simulate the optimistic move in state first and then assert it is reverted on 409 (if that’s the intended behavior).
| expect(store.items).toHaveLength(0) | ||
| // getErrorDisplay mock returns the fallback when error lacks .message | ||
| expect(store.actionError).toBe('Failed to capture item') |
There was a problem hiding this comment.
The inline comment about the 500 case is misleading: this rejection payload includes a message (response.data.message), and in this suite getErrorDisplay is mocked to always return the fallback regardless. Consider updating/removing the comment so it accurately reflects why the fallback is expected here.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Post-bot review and fixes
After reading all 6 test files and cross-referencing against the actual store/API implementations, here are the issues I found and am fixing:
Bot comment fixes
-
archiveApi: manual URL extraction pattern (Gemini) — Lines 67-68, 76-77, 85-86, 94-97, and similar patterns in captureStore. Replaced
mock.calls[0][0]extraction with idiomaticexpect(http.get).toHaveBeenCalledWith(expect.stringContaining(...)). -
archiveApi restoreItem URL encoding test — Same pattern for
http.postmock call extraction on line 144-146. -
captureStore pollTriageCompletion: missing call count assertion (Gemini) — Added assertion that
http.getwas called the expected number of times (3) to verify polling actually executed the right number of attempts. -
boardStore 409 snap-back test (Copilot) — The test IS valid. After reading
cardStore.ts,moveCardcalls the API first and only updates local state after success. On 409, the card genuinely stays in its original column. Renamed the comment from "snap-back" to clarify there's no optimistic mutation to roll back — state was never changed. -
captureStore line 282 misleading comment (Copilot) — The
getErrorDisplaymock always returns the fallback, unconditionally. Fixed the comment to reflect reality.
Additional issues found in adversarial review
-
captureStore query parameter tests partly duplicate existing coverage — The
fetchItemsdescribe block already tests status forwarding (line 97-105). The new query parameter tests addboardIdand combined params, which are valuable, but noted overlap. -
notificationStore unread count tests test Array.filter, not a store getter — These document expected behavior but the store doesn't expose an
unreadCountcomputed. The "adding a new unread notification locally" test exercises pure array manipulation, not a store action. Added a comment clarifying the intent. -
captureStore fetchItems query parameter tests — Applied the same
toHaveBeenCalledWith(expect.stringContaining(...))idiom.
Replace manual mock.calls[0][0] URL extraction with expect(http.get).toHaveBeenCalledWith(expect.stringContaining(...)) for query param and URL encoding assertions. More idiomatic Vitest and less brittle.
…c matchers - Fix misleading comment on 500 error test: getErrorDisplay mock always returns fallback, not conditionally - Add callCount assertion to pollTriageCompletion to verify expected number of poll attempts - Replace manual mock.calls URL extraction with toHaveBeenCalledWith(expect.stringContaining(...)) for query param tests
…back The store calls the API before updating local state, so on rejection the card is never modified. Updated test name and comment to reflect this — there is no snap-back because there was no optimistic mutation.
The store does not expose an unreadCount computed, so these tests verify the expected derivation from the notifications array after store actions rather than testing a store getter.
Summary
Closes #711
Test plan