test: frontend store integration tests (#711)#821
Conversation
Tests the full chatApi → http chain including session creation, message accumulation, proposal references, degraded messages, tool call metadata, health checks, URL encoding, and error propagation.
Covers column reorder with card association preservation, card update 409 conflict handling, expectedUpdatedAt stale edit detection, moveCard column card count tracking, and editingCardId preservation during board operations.
…ion tests Covers Pending → Processing → Completed transitions via re-fetch, server-side deletion handling, stale state reconciliation, cancel isolation (409 failure keeps item), submit appends to existing items, and fetchStats independence from request list.
Covers OIDC code exchange via /auth/oidc/exchange, localStorage persistence, error mapping, comprehensive logout state clearing, sequential login/logout cycles with different users, sessionState computed, invalid JWT structure validation, and loading state transitions.
Covers simulated real-time notification arrival with unread count derivation, stale state reconciliation on re-fetch, loading state transitions for all async operations, error recovery on retry, board-scoped filtering with combined query params, and markAsRead idempotency.
Covers localStorage mode persistence and fallback, concurrent overlapping preference requests with version guard, clearHomeSummary/clearTodaySummary badge count reset, homeLoading/todayLoading transitions, fetchHomeSummary mode sync from server, resetForLogout behavior, and onboarding sync across home and today summaries.
Self-Review: Adversarial FindingsGenuine Issues Found
Not Issues (Verified OK)
VerdictItems 1-4 are real but low severity. Items 1 and 2 are weak assertions that could theoretically pass with incorrect behavior but don't currently mask bugs. Items 3-4 are style/accuracy nits. None require blocking fixes. |
…view - boardStore: assert column order is preserved (not just count) on reorder failure - sessionStore: rename misleading test re: error clearing, add auth artifact assertions - notificationStore: rename and fix error recovery test to accurately document that markAsRead does not clear errors from prior failures
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48974ae219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // After logout, mode should read from localStorage which has 'workbench' | ||
| // (resetForLogout calls applyMode(getLocalWorkspaceMode()) which re-persists) | ||
| expect(store.homeSummary).toBeNull() | ||
| expect(store.todaySummary).toBeNull() | ||
| expect(store.preferencesHydrated).toBe(false) |
There was a problem hiding this comment.
This test claims to verify that resetForLogout restores the mode from localStorage, but it never asserts store.mode (or the storage value) after calling resetForLogout. As written, it will still pass even if the fallback mode behavior regresses, so it does not protect the contract described in the test name/comment.
Useful? React with 👍 / 👎.
| // After logout, error should be cleared (logout calls clearSession) | ||
| store.logout() | ||
| // error is not explicitly cleared by clearSession, but the session state should be clean | ||
| expect(store.isAuthenticated).toBe(false) |
There was a problem hiding this comment.
The test is named and documented as validating that logout clears error state, but it only checks isAuthenticated after store.logout(). If store.error remains populated, this test still passes, so it does not actually catch regressions in the behavior it advertises.
Useful? React with 👍 / 👎.
![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 introduces a comprehensive suite of integration tests for several Pinia stores and API modules, including boardStore, chatApi, notificationStore, queueStore, sessionStore, and workspaceStore. These tests cover critical paths such as column reordering, OIDC authentication flows, real-time notification handling, and state reconciliation. Feedback is provided to improve the robustness of assertions in error-handling scenarios and to ensure test descriptions accurately reflect the underlying store implementations, particularly regarding how error states are managed during logout and failed operations.
|
|
||
| // On failure, columns must not be mutated to the new order | ||
| // (the API call is atomic — either it succeeds and we update, or it fails and we keep original) | ||
| expect(store.currentBoard?.columns).toHaveLength(2) |
There was a problem hiding this comment.
The test asserts that the columns length remains 2, but it doesn't verify that the order of the columns was not mutated. To ensure the state is truly not "corrupted" or left in a partially updated state (especially if optimistic updates are used), the test should verify the IDs and positions of the columns match the original state.
expect(store.currentBoard?.columns).toHaveLength(2)
expect(store.currentBoard?.columns[0].id).toBe('col-a')
expect(store.currentBoard?.columns[1].id).toBe('col-b')| expect(store.notifications).toHaveLength(1) | ||
| }) | ||
|
|
||
| it('updates notification state on successful retry even when error from prior failure persists', async () => { |
There was a problem hiding this comment.
The test description states that it "clears error on successful markAsRead", but the implementation of markAsRead in notificationStore.ts does not actually clear the error ref on success (it only sets it on failure). Furthermore, the test does not assert that store.error is null. The test description should be updated to reflect the actual behavior, or the store implementation should be updated to clear the error.
| it('updates notification state on successful retry even when error from prior failure persists', async () => { | |
| it('updates notification state on successful markAsRead after a previous error', async () => { |
| expect(localStorage.getItem('taskdeck_session')).toBeNull() | ||
| }) | ||
|
|
||
| it('does not leave authentication artifacts after logout following a failed login', async () => { |
There was a problem hiding this comment.
The test title "clears error state on logout" is misleading because clearSession (called by logout) does not reset the error ref. The test also lacks an assertion for store.error. If the intention is that logout should reset the error state, the store implementation needs to be updated. Otherwise, the test description should be corrected to avoid implying behavior that isn't implemented or tested.
| it('does not leave authentication artifacts after logout following a failed login', async () => { | |
| it('resets authentication state on logout', async () => { |
|
|
||
| const store = useSessionStore() | ||
| await store.login({ usernameOrEmail: 'user', password: 'pass' }) | ||
|
|
There was a problem hiding this comment.
While checking localStorage is important, the test should also verify the in-memory store state. Specifically, store.isAuthenticated should be false if the token structure was invalid, as setSession returns early and does not update the store's reactive state.
expect(store.isAuthenticated).toBe(false)
expect(localStorage.getItem('taskdeck_token')).toBeNull()There was a problem hiding this comment.
Pull request overview
Adds a new set of frontend store + real API module integration tests (mocking only the HTTP layer) to cover cross-boundary behavior that unit tests with mocked API modules don’t exercise.
Changes:
- Introduces integration test suites for
chatApiplus key Pinia stores (boardStore,queueStore,sessionStore,notificationStore,workspaceStore). - Validates important scenarios like optimistic updates, conflict handling, loading/error transitions, stale reconciliation, and localStorage persistence.
- Ensures API boundary details are exercised (URL encoding, query-string composition, and response normalization).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/tests/store/chatApi.integration.spec.ts | Exercises chat API module behavior against mocked HTTP for key endpoints (sessions, messages, health). |
| frontend/taskdeck-web/src/tests/store/boardStore.columnReorder.spec.ts | Covers column reorder + card preservation, 409 conflict on card update, move-card count tracking, and editing state preservation. |
| frontend/taskdeck-web/src/tests/store/queueStore.polling.spec.ts | Tests queue request lifecycle via repeated fetches, deletion reconciliation, cancel isolation, and stats independence. |
| frontend/taskdeck-web/src/tests/store/sessionStore.oidc.spec.ts | Adds OIDC exchange + session lifecycle tests, persistence, token structure guard coverage, and loading transitions. |
| frontend/taskdeck-web/src/tests/store/notificationStore.realtime.spec.ts | Simulates realtime arrivals + reconciliation, loading transitions, error recovery, filtering, and idempotency. |
| frontend/taskdeck-web/src/tests/store/workspaceStore.modePersistence.spec.ts | Tests workspace mode persistence, concurrent preference request guarding, summary clearing, mode sync, and loading transitions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| store.resetForLogout() | ||
|
|
||
| // After logout, mode should read from localStorage which has 'workbench' | ||
| // (resetForLogout calls applyMode(getLocalWorkspaceMode()) which re-persists) |
| it('does not leave authentication artifacts after logout following a failed login', async () => { | ||
| vi.mocked(http.post).mockRejectedValue({ | ||
| response: { data: { message: 'Login failed' } }, | ||
| }) | ||
|
|
||
| const store = useSessionStore() | ||
| await expect(store.login({ usernameOrEmail: 'bad', password: 'bad' })).rejects.toBeDefined() | ||
| expect(store.error).toBe('Login failed') | ||
|
|
||
| store.logout() | ||
|
|
||
| // clearSession does not explicitly null out error, but all auth artifacts must be gone | ||
| expect(store.isAuthenticated).toBe(false) | ||
| expect(store.token).toBeNull() |
| // With an invalid JWT structure, setSession should guard against persistence | ||
| // The token is still set in-memory but may not validate as authenticated | ||
| // depending on isTokenExpired behavior with malformed tokens |
| it('updates notification state on successful retry even when error from prior failure persists', async () => { | ||
| const store = useNotificationStore() | ||
| store.notifications = [makeNotification({ id: 'n-err', isRead: false })] | ||
|
|
||
| // First markAsRead fails — error is set | ||
| vi.mocked(http.post).mockRejectedValueOnce(new Error('server error')) | ||
| await expect(store.markAsRead('n-err')).rejects.toBeInstanceOf(Error) | ||
| expect(store.error).toBe('Failed to mark notification as read') | ||
|
|
||
| // Retry succeeds — notification is updated even though error persists | ||
| // (markAsRead only sets error on failure; it does not clear it on success) | ||
| const readNotification = makeNotification({ id: 'n-err', isRead: true, readAt: '2026-02-01T00:00:00Z' }) | ||
| vi.mocked(http.post).mockResolvedValueOnce({ data: readNotification }) | ||
| await store.markAsRead('n-err') | ||
|
|
||
| expect(store.notifications[0].isRead).toBe(true) | ||
| // Error from the prior failure is still set — not a bug, just markAsRead's current behavior | ||
| expect(store.error).toBe('Failed to mark notification as read') |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b92092a874
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // With an invalid JWT structure, setSession should guard against persistence | ||
| // The token is still set in-memory but may not validate as authenticated | ||
| // depending on isTokenExpired behavior with malformed tokens | ||
| expect(localStorage.getItem('taskdeck_token')).toBeNull() |
There was a problem hiding this comment.
This test is described as proving that an invalid JWT does not establish a session, but it only checks that taskdeck_token was not written to localStorage. If a regression sets store.token/store.userId in memory before skipping persistence, this test would still pass while the app could treat the user as authenticated for the current tab. Add assertions on in-memory auth state (for example isAuthenticated === false and null session fields) so the contract is actually enforced.
Useful? React with 👍 / 👎.
Round 2 Adversarial ReviewI have read all 6 new test files, cross-referenced them against the production store/API code, verified mock isolation chains, and checked CI status. Here are my findings. Important (should fix before merge)1. The test "does not persist session when token has invalid JWT structure" only asserts that expect(store.isAuthenticated).toBe(false)
expect(store.token).toBeNull()
expect(store.userId).toBeNull()Without these assertions, a regression where 2. The test "reads the localStorage fallback mode after reset" sets localStorage to 'workbench', fetches a home summary that sets mode to 'agent', then calls expect(store.mode).toBe('workbench')This was flagged by two prior reviewers and has not been addressed. 3. Across the test files, The cases where this matters most are the error path tests in Minor (acceptable, but worth noting)4. The test accurately documents that 5. The comment says "The token is still set in-memory but may not validate as authenticated" -- but reading Verified OK (no issues)
VerdictItems 1 and 2 are genuine assertion gaps that reduce confidence in the test suite -- they test names promise behavior verification but the assertions don't actually enforce it. Item 3 is a consistency issue that weakens error path coverage. These should be fixed before merge. The remaining items are informational. |
- sessionStore: assert in-memory auth state (isAuthenticated, token, userId) for invalid JWT test, not just localStorage; fix misleading comment - workspaceStore: add store.mode assertion to resetForLogout test and fix incorrect comment (fetchHomeSummary overwrites localStorage before reset); add second test for localStorage fallback without server sync - Replace 12 weak rejects.toBeDefined() with type-specific assertions: toMatchObject for HTTP error shapes, toThrow for Error instances - All 2334 tests pass
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2255eca69a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| it('sets loading=true during register and clears after failure', async () => { | ||
| vi.mocked(http.post).mockRejectedValue({ | ||
| response: { data: { message: 'Email already taken' } }, | ||
| }) | ||
|
|
There was a problem hiding this comment.
This test is labeled as validating loading=true during register, but it only checks loading after the promise rejects. If register() regresses to never set loading at request start (so the UI loses its in-flight state), this test would still pass. Capture store state inside the mocked request (as done in the login test) to assert the mid-request transition.
Useful? React with 👍 / 👎.
| it('sets loading=true during updatePreferences and clears after', async () => { | ||
| vi.mocked(http.put).mockResolvedValue({ | ||
| data: { userId: 'u-1', mentionImmediateEnabled: false }, | ||
| }) | ||
|
|
There was a problem hiding this comment.
The test name says updatePreferences sets loading=true and then clears it, but it only asserts the final false state. That means a regression where updatePreferences() never raises loading during the API call would go undetected, even though users would lose loading feedback. Add an in-flight assertion by inspecting store state from within the mocked HTTP call.
Useful? React with 👍 / 👎.
Update STATUS.md, IMPLEMENTATION_MASTERPLAN.md, TESTING_GUIDE.md, and MANUAL_TEST_CHECKLIST.md for the supplementary test depth wave. - Add delivery entry 134 to masterplan for 6 parallel worktree PRs - Update test counts: backend ~4,479+, frontend ~2,454+, combined ~6,950+ - Add new TESTING_GUIDE section with run commands and test breakdowns - Add Z22 manual validation checklist for new test categories - Update date stamps to 2026-04-13
Summary
New test files
Closes #711
Test plan