test(frontend): add comprehensive unit test suite (69 cases) - #46
test(frontend): add comprehensive unit test suite (69 cases)#46parthdude07 wants to merge 1 commit into
Conversation
- migrateLibrary: 9 tests (schema migration v1→v2, invalid input, future versions) - bookmarkStore: 6 tests (localStorage persistence, corrupted JSON recovery, fallback) - api service: 10 tests (auth header injection, 401 token clearing, network errors, timeout) - AuthContext: 6 tests (mount validation, login/logout, token persistence, modal state) - ProtectedRoute: 4 tests (loading spinner, auth rendering, modal triggering, redirect) - BookmarkButton: 9 tests (toggle state, click behavior, label display, event propagation) - useBookmarks: 11 tests (guest mode CRUD, idempotency, highlight truncation, filtering) - useNotes: 7 tests (fetch, sort/filter, add/update/delete, togglePin) - useBookmarkReconciliation: 6 tests (stale refresh, deleted flagging, cache cross-reference)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
10 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/test/lib/bookmarkStore.test.ts">
<violation number="1" location="src/test/lib/bookmarkStore.test.ts:23">
P3: The comment describing the dynamic imports is inaccurate. The import returns the already-cached module singleton every test (`bookmarkStore` is created once at first module evaluation), so re-importing does not recreate the store or re-run module-level setup against fresh mocks. This is harmless for the current tests because `LocalBookmarkStore.load/save` read and write localStorage on each call, but the comment's stated intent (that module-level code runs with mocks in place) is not what happens.</violation>
<violation number="2" location="src/test/lib/bookmarkStore.test.ts:67">
P2: The quota-exceeded pruning path is untested despite the header and PR description claiming coverage. The lone save-error test deliberately throws a plain `Error`, which can't match the `DOMException`/`QuotaExceededError` check in `save()`, so it only exercises the generic `console.error` branch. Add a test that stubs `setItem` to throw a `DOMException` with `name: 'QuotaExceededError'` (or code 22) and verifies highlights are pruned and the retry is attempted.</violation>
<violation number="3" location="src/test/lib/bookmarkStore.test.ts:81">
P2: The 'InMemoryStore (fallback)' test does not test InMemoryStore at all. It imports the shared `bookmarkStore` singleton, which in the test environment is the `LocalBookmarkStore` (created at module load when `isLocalStorageAvailable()` returns true), and merely roundtrips save/load through localStorage. The fallback (private/incognito) path is a distinct class (`InMemoryStore`) that is never exercised, so the test is mislabeled and provides false coverage of the fallback path described in the PR. The InMemoryStore class is not exported, and the `bookmarkStore` module singleton is cached, so there is no way this test reaches the fallback implementation. Export `InMemoryStore` (or run the module with localStorage disabled) and instantiate it directly so the fallback path is genuinely covered.</violation>
</file>
<file name="src/test/services/api.test.ts">
<violation number="1" location="src/test/services/api.test.ts:35">
P3: The api.test.ts suite never exercises the timeout path. The PR description claims "timeout" coverage for the api service, and this file sets up `vi.useFakeTimers({ shouldAdvanceTime: true })`, presumably for that purpose, but no test triggers the `AbortError` → 408 `TIMEOUT` branch in `createTimeoutController`/`request`. Add a test that makes fetch honor the AbortSignal and assert the resulting APIError has `statusCode: 408` and `code: 'TIMEOUT'`, otherwise the timeout handling (and the fake-timer setup) is unverified.</violation>
</file>
<file name="src/test/components/ProtectedRoute.test.tsx">
<violation number="1" location="src/test/components/ProtectedRoute.test.tsx:125">
P2: The redirect test can never fail and doesn't test the scenario it claims. In `ProtectedRoute`, both `useEffect` hooks run on mount in declaration order: effect 1 sets `modalWasOpened.current = true`, then effect 2 sees it and calls `navigate` on the very first render. So `mockNavigate` is already called with `/home` before any modal opens/closes. Wrapping the assertion in `if (mockNavigate.mock.calls.length > 0)` inside `waitFor` also means the test passes even if navigation never happens, since `waitFor` resolves when the callback doesn't throw. Drop the guard and assert directly, and actually simulate the modal opening then closing (e.g. re-render with `isLoginModalOpen: true` then `false`) so the test validates the intended open→close→redirect flow.</violation>
</file>
<file name="src/test/lib/migrateLibrary.test.ts">
<violation number="1" location="src/test/lib/migrateLibrary.test.ts:80">
P3: The 'returns v2 data as-is when shape is valid' test asserts `expect(result).toBe(v2Data)`, tying the test to the implementation detail that `migrateLibrary` returns the stored reference without copying. If the migration is later changed to defensively copy/normalize valid v2 data (a common safety practice), this test breaks even though behavior is unchanged. Assert on the result's shape/content instead (e.g. `toEqual` with the expected library state), which reflects the contract the tests should protect.</violation>
</file>
<file name="src/test/hooks/useBookmarkReconciliation.test.tsx">
<violation number="1" location="src/test/hooks/useBookmarkReconciliation.test.tsx:41">
P3: The 'cache not loaded' case seeds no data, so the hook's queryFn (`queryClient.getQueryData(['transcripts'])`) resolves to `undefined`. React Query v5 rejects a queryFn resolving to undefined with its 'Query data cannot be undefined' error, leaving the query in an error state and emitting console noise. The test still passes because the hook treats `allTranscripts` as falsy and returns bookmarks as-is, but the test does not exercise the intended no-cache branch cleanly and the assertion would pass whether or not the fallback logic worked. Return a defined value (e.g. `?? null`) from the hook's queryFn so the no-cache path is a clean 'success with no data' rather than an error.</violation>
</file>
<file name="src/test/contexts/AuthContext.test.tsx">
<violation number="1" location="src/test/contexts/AuthContext.test.tsx:186">
P3: If the `expect(...).toThrow(...)` assertion in this test fails, `spy.mockRestore()` is never reached and `console.error` stays suppressed for the rest of the test run, hiding the cause of later failures. This `describe` block has no `afterEach` cleanup (the `vi.restoreAllMocks()` is scoped to the `AuthContext` describe), so there is no safety net. Wrap the assertion in `try/finally` so the spy is always restored.</violation>
</file>
<file name="src/test/hooks/useNotes.test.tsx">
<violation number="1" location="src/test/hooks/useNotes.test.tsx:53">
P3: `vi.stubGlobal('crypto', { randomUUID: ... })` replaces the entire global `crypto` object, dropping `getRandomValues`, `subtle`, and every other method, and it is never restored. The hook only needs `randomUUID`, so preserve the rest of the browser crypto (and restore after) instead of shadowing the whole API, which could silently break unrelated code that runs during these tests.</violation>
</file>
<file name="src/test/hooks/useBookmarks.test.tsx">
<violation number="1" location="src/test/hooks/useBookmarks.test.tsx:53">
P3: This stub replaces the entire global crypto object and is never restored (no afterEach/vi.unstubAllGlobals). Replacing the whole object discards other crypto APIs such as getRandomValues/subtle that other code in the same test worker may rely on, and it silently overrides the real implementation rather than just the one API under test. Scope the override to randomUUID only and restore it in afterEach to keep the test isolated and non-polluting.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }) | ||
| }) | ||
|
|
||
| describe('InMemoryStore (fallback)', () => { |
There was a problem hiding this comment.
P2: The 'InMemoryStore (fallback)' test does not test InMemoryStore at all. It imports the shared bookmarkStore singleton, which in the test environment is the LocalBookmarkStore (created at module load when isLocalStorageAvailable() returns true), and merely roundtrips save/load through localStorage. The fallback (private/incognito) path is a distinct class (InMemoryStore) that is never exercised, so the test is mislabeled and provides false coverage of the fallback path described in the PR. The InMemoryStore class is not exported, and the bookmarkStore module singleton is cached, so there is no way this test reaches the fallback implementation. Export InMemoryStore (or run the module with localStorage disabled) and instantiate it directly so the fallback path is genuinely covered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/lib/bookmarkStore.test.ts, line 81:
<comment>The 'InMemoryStore (fallback)' test does not test InMemoryStore at all. It imports the shared `bookmarkStore` singleton, which in the test environment is the `LocalBookmarkStore` (created at module load when `isLocalStorageAvailable()` returns true), and merely roundtrips save/load through localStorage. The fallback (private/incognito) path is a distinct class (`InMemoryStore`) that is never exercised, so the test is mislabeled and provides false coverage of the fallback path described in the PR. The InMemoryStore class is not exported, and the `bookmarkStore` module singleton is cached, so there is no way this test reaches the fallback implementation. Export `InMemoryStore` (or run the module with localStorage disabled) and instantiate it directly so the fallback path is genuinely covered.</comment>
<file context>
@@ -0,0 +1,100 @@
+ })
+})
+
+describe('InMemoryStore (fallback)', () => {
+ it('load() returns empty state and save() persists in memory', async () => {
+ // Simulate no localStorage available by importing InMemoryStore path.
</file context>
| // After the useEffects run, navigate should be called with redirectTo | ||
| await waitFor(() => { | ||
| // The redirect may happen after two renders (once for modal open, once for close) | ||
| if (mockNavigate.mock.calls.length > 0) { |
There was a problem hiding this comment.
P2: The redirect test can never fail and doesn't test the scenario it claims. In ProtectedRoute, both useEffect hooks run on mount in declaration order: effect 1 sets modalWasOpened.current = true, then effect 2 sees it and calls navigate on the very first render. So mockNavigate is already called with /home before any modal opens/closes. Wrapping the assertion in if (mockNavigate.mock.calls.length > 0) inside waitFor also means the test passes even if navigation never happens, since waitFor resolves when the callback doesn't throw. Drop the guard and assert directly, and actually simulate the modal opening then closing (e.g. re-render with isLoginModalOpen: true then false) so the test validates the intended open→close→redirect flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/components/ProtectedRoute.test.tsx, line 125:
<comment>The redirect test can never fail and doesn't test the scenario it claims. In `ProtectedRoute`, both `useEffect` hooks run on mount in declaration order: effect 1 sets `modalWasOpened.current = true`, then effect 2 sees it and calls `navigate` on the very first render. So `mockNavigate` is already called with `/home` before any modal opens/closes. Wrapping the assertion in `if (mockNavigate.mock.calls.length > 0)` inside `waitFor` also means the test passes even if navigation never happens, since `waitFor` resolves when the callback doesn't throw. Drop the guard and assert directly, and actually simulate the modal opening then closing (e.g. re-render with `isLoginModalOpen: true` then `false`) so the test validates the intended open→close→redirect flow.</comment>
<file context>
@@ -0,0 +1,130 @@
+ // After the useEffects run, navigate should be called with redirectTo
+ await waitFor(() => {
+ // The redirect may happen after two renders (once for modal open, once for close)
+ if (mockNavigate.mock.calls.length > 0) {
+ expect(mockNavigate).toHaveBeenCalledWith('/home', { replace: true })
+ }
</file context>
| expect(stored.bookmarks).toHaveLength(1) | ||
| }) | ||
|
|
||
| it('save() returns state even on non-quota error', async () => { |
There was a problem hiding this comment.
P2: The quota-exceeded pruning path is untested despite the header and PR description claiming coverage. The lone save-error test deliberately throws a plain Error, which can't match the DOMException/QuotaExceededError check in save(), so it only exercises the generic console.error branch. Add a test that stubs setItem to throw a DOMException with name: 'QuotaExceededError' (or code 22) and verifies highlights are pruned and the retry is attempted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/lib/bookmarkStore.test.ts, line 67:
<comment>The quota-exceeded pruning path is untested despite the header and PR description claiming coverage. The lone save-error test deliberately throws a plain `Error`, which can't match the `DOMException`/`QuotaExceededError` check in `save()`, so it only exercises the generic `console.error` branch. Add a test that stubs `setItem` to throw a `DOMException` with `name: 'QuotaExceededError'` (or code 22) and verifies highlights are pruned and the retry is attempted.</comment>
<file context>
@@ -0,0 +1,100 @@
+ expect(stored.bookmarks).toHaveLength(1)
+ })
+
+ it('save() returns state even on non-quota error', async () => {
+ const spy = vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
+ throw new Error('Generic storage error')
</file context>
| }) | ||
|
|
||
| it('load() returns DEFAULT_LIBRARY when localStorage is empty', async () => { | ||
| // Dynamically import so module-level code runs with our mocks in place |
There was a problem hiding this comment.
P3: The comment describing the dynamic imports is inaccurate. The import returns the already-cached module singleton every test (bookmarkStore is created once at first module evaluation), so re-importing does not recreate the store or re-run module-level setup against fresh mocks. This is harmless for the current tests because LocalBookmarkStore.load/save read and write localStorage on each call, but the comment's stated intent (that module-level code runs with mocks in place) is not what happens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/lib/bookmarkStore.test.ts, line 23:
<comment>The comment describing the dynamic imports is inaccurate. The import returns the already-cached module singleton every test (`bookmarkStore` is created once at first module evaluation), so re-importing does not recreate the store or re-run module-level setup against fresh mocks. This is harmless for the current tests because `LocalBookmarkStore.load/save` read and write localStorage on each call, but the comment's stated intent (that module-level code runs with mocks in place) is not what happens.</comment>
<file context>
@@ -0,0 +1,100 @@
+ })
+
+ it('load() returns DEFAULT_LIBRARY when localStorage is empty', async () => {
+ // Dynamically import so module-level code runs with our mocks in place
+ const { bookmarkStore } = await import('@/lib/bookmarkStore')
+ const result = bookmarkStore.load()
</file context>
| @@ -0,0 +1,170 @@ | |||
| /** | |||
There was a problem hiding this comment.
P3: The api.test.ts suite never exercises the timeout path. The PR description claims "timeout" coverage for the api service, and this file sets up vi.useFakeTimers({ shouldAdvanceTime: true }), presumably for that purpose, but no test triggers the AbortError → 408 TIMEOUT branch in createTimeoutController/request. Add a test that makes fetch honor the AbortSignal and assert the resulting APIError has statusCode: 408 and code: 'TIMEOUT', otherwise the timeout handling (and the fake-timer setup) is unverified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/services/api.test.ts, line 35:
<comment>The api.test.ts suite never exercises the timeout path. The PR description claims "timeout" coverage for the api service, and this file sets up `vi.useFakeTimers({ shouldAdvanceTime: true })`, presumably for that purpose, but no test triggers the `AbortError` → 408 `TIMEOUT` branch in `createTimeoutController`/`request`. Add a test that makes fetch honor the AbortSignal and assert the resulting APIError has `statusCode: 408` and `code: 'TIMEOUT'`, otherwise the timeout handling (and the fake-timer setup) is unverified.</comment>
<file context>
@@ -0,0 +1,170 @@
+ })
+})
+
+describe('api.request (via api.get/post)', () => {
+ const originalFetch = global.fetch
+
</file context>
| } | ||
|
|
||
| const result = migrateLibrary(v2Data) | ||
| expect(result).toBe(v2Data) // same reference, no copy |
There was a problem hiding this comment.
P3: The 'returns v2 data as-is when shape is valid' test asserts expect(result).toBe(v2Data), tying the test to the implementation detail that migrateLibrary returns the stored reference without copying. If the migration is later changed to defensively copy/normalize valid v2 data (a common safety practice), this test breaks even though behavior is unchanged. Assert on the result's shape/content instead (e.g. toEqual with the expected library state), which reflects the contract the tests should protect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/lib/migrateLibrary.test.ts, line 80:
<comment>The 'returns v2 data as-is when shape is valid' test asserts `expect(result).toBe(v2Data)`, tying the test to the implementation detail that `migrateLibrary` returns the stored reference without copying. If the migration is later changed to defensively copy/normalize valid v2 data (a common safety practice), this test breaks even though behavior is unchanged. Assert on the result's shape/content instead (e.g. `toEqual` with the expected library state), which reflects the contract the tests should protect.</comment>
<file context>
@@ -0,0 +1,107 @@
+ }
+
+ const result = migrateLibrary(v2Data)
+ expect(result).toBe(v2Data) // same reference, no copy
+ })
+
</file context>
| ] | ||
|
|
||
| it('returns bookmarks as-is when transcript cache is not loaded', async () => { | ||
| // Don't pre-seed cache |
There was a problem hiding this comment.
P3: The 'cache not loaded' case seeds no data, so the hook's queryFn (queryClient.getQueryData(['transcripts'])) resolves to undefined. React Query v5 rejects a queryFn resolving to undefined with its 'Query data cannot be undefined' error, leaving the query in an error state and emitting console noise. The test still passes because the hook treats allTranscripts as falsy and returns bookmarks as-is, but the test does not exercise the intended no-cache branch cleanly and the assertion would pass whether or not the fallback logic worked. Return a defined value (e.g. ?? null) from the hook's queryFn so the no-cache path is a clean 'success with no data' rather than an error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/hooks/useBookmarkReconciliation.test.tsx, line 41:
<comment>The 'cache not loaded' case seeds no data, so the hook's queryFn (`queryClient.getQueryData(['transcripts'])`) resolves to `undefined`. React Query v5 rejects a queryFn resolving to undefined with its 'Query data cannot be undefined' error, leaving the query in an error state and emitting console noise. The test still passes because the hook treats `allTranscripts` as falsy and returns bookmarks as-is, but the test does not exercise the intended no-cache branch cleanly and the assertion would pass whether or not the fallback logic worked. Return a defined value (e.g. `?? null`) from the hook's queryFn so the no-cache path is a clean 'success with no data' rather than an error.</comment>
<file context>
@@ -0,0 +1,149 @@
+ ]
+
+ it('returns bookmarks as-is when transcript cache is not loaded', async () => {
+ // Don't pre-seed cache
+ const { result } = renderHook(
+ () => useBookmarkReconciliation(baseBookmarks, baseHighlights),
</file context>
| const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) | ||
|
|
||
| function BadComponent() { | ||
| useAuth() | ||
| return null | ||
| } | ||
|
|
||
| expect(() => render(<BadComponent />)).toThrow('useAuth must be used within an AuthProvider') | ||
|
|
||
| spy.mockRestore() |
There was a problem hiding this comment.
P3: If the expect(...).toThrow(...) assertion in this test fails, spy.mockRestore() is never reached and console.error stays suppressed for the rest of the test run, hiding the cause of later failures. This describe block has no afterEach cleanup (the vi.restoreAllMocks() is scoped to the AuthContext describe), so there is no safety net. Wrap the assertion in try/finally so the spy is always restored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/contexts/AuthContext.test.tsx, line 186:
<comment>If the `expect(...).toThrow(...)` assertion in this test fails, `spy.mockRestore()` is never reached and `console.error` stays suppressed for the rest of the test run, hiding the cause of later failures. This `describe` block has no `afterEach` cleanup (the `vi.restoreAllMocks()` is scoped to the `AuthContext` describe), so there is no safety net. Wrap the assertion in `try/finally` so the spy is always restored.</comment>
<file context>
@@ -0,0 +1,197 @@
+describe('useAuth (outside provider)', () => {
+ it('throws when used outside AuthProvider', () => {
+ // Suppress React error boundary noise
+ const spy = vi.spyOn(console, 'error').mockImplementation(() => {})
+
+ function BadComponent() {
</file context>
| const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) | |
| function BadComponent() { | |
| useAuth() | |
| return null | |
| } | |
| expect(() => render(<BadComponent />)).toThrow('useAuth must be used within an AuthProvider') | |
| spy.mockRestore() | |
| it('throws when used outside AuthProvider', () => { | |
| const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) | |
| function BadComponent() { | |
| useAuth() | |
| return null | |
| } | |
| try { | |
| expect(() => render(<BadComponent />)).toThrow('useAuth must be used within an AuthProvider') | |
| } finally { | |
| spy.mockRestore() | |
| } | |
| }) |
| })) | ||
|
|
||
| // Mock crypto.randomUUID | ||
| vi.stubGlobal('crypto', { |
There was a problem hiding this comment.
P3: vi.stubGlobal('crypto', { randomUUID: ... }) replaces the entire global crypto object, dropping getRandomValues, subtle, and every other method, and it is never restored. The hook only needs randomUUID, so preserve the rest of the browser crypto (and restore after) instead of shadowing the whole API, which could silently break unrelated code that runs during these tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/hooks/useNotes.test.tsx, line 53:
<comment>`vi.stubGlobal('crypto', { randomUUID: ... })` replaces the entire global `crypto` object, dropping `getRandomValues`, `subtle`, and every other method, and it is never restored. The hook only needs `randomUUID`, so preserve the rest of the browser crypto (and restore after) instead of shadowing the whole API, which could silently break unrelated code that runs during these tests.</comment>
<file context>
@@ -0,0 +1,224 @@
+}))
+
+// Mock crypto.randomUUID
+vi.stubGlobal('crypto', {
+ randomUUID: () => 'temp-uuid-' + Math.random().toString(36).slice(2, 8),
+})
</file context>
| })) | ||
|
|
||
| // Mock crypto.randomUUID | ||
| vi.stubGlobal('crypto', { |
There was a problem hiding this comment.
P3: This stub replaces the entire global crypto object and is never restored (no afterEach/vi.unstubAllGlobals). Replacing the whole object discards other crypto APIs such as getRandomValues/subtle that other code in the same test worker may rely on, and it silently overrides the real implementation rather than just the one API under test. Scope the override to randomUUID only and restore it in afterEach to keep the test isolated and non-polluting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/test/hooks/useBookmarks.test.tsx, line 53:
<comment>This stub replaces the entire global crypto object and is never restored (no afterEach/vi.unstubAllGlobals). Replacing the whole object discards other crypto APIs such as getRandomValues/subtle that other code in the same test worker may rely on, and it silently overrides the real implementation rather than just the one API under test. Scope the override to randomUUID only and restore it in afterEach to keep the test isolated and non-polluting.</comment>
<file context>
@@ -0,0 +1,233 @@
+}))
+
+// Mock crypto.randomUUID
+vi.stubGlobal('crypto', {
+ randomUUID: () => 'mock-uuid-' + Math.random().toString(36).slice(2, 8),
+})
</file context>
Summary by cubic
Adds a comprehensive frontend unit test suite to lock in auth, bookmarks, notes, and API client behavior. Adds
@testing-library/user-eventfor interaction tests; no runtime behavior changes.Coverage highlights
BookmarkButton,ProtectedRoute,AuthContext.useBookmarks(guest/localStorage),useNotes(React Query),useBookmarkReconciliation.bookmarkStore,migrateLibrary,apiandAPIError.Contract assumptions
btc-auth-tokenandbtc-library; 401 responses clear the token.{ success, data }or{ success: false, error: { code, message } }; 500s surfaceerror.codeandmessage.ProtectedRouteshows a spinner while loading, opens the login modal when unauthenticated, and redirects toredirectToif the modal closes without login.['transcripts']to refresh snapshots and flag deleted items.Written for commit ab0994a. Summary will update on new commits.