Repository navigation
test: comprehensive unit tests for apps/web/src/lib - #788
Closed
2witstudios wants to merge 1 commit into
Closed
2witstudios wants to merge 1 commit into
2witstudios wants to merge 1 commit into
Conversation
Add 88 test files covering all testable source modules in apps/web/src/lib/: - AI core: provider factory, model capabilities, tool filtering, schema introspection, system prompts, message utils, complete request builder, page tree context, MCP tools - AI hooks: useChatStop, useChatTransport, useConversations, useMCPTools, useMessageActions, useProviderSettings, useSendHandoff, useStreamRecovery, useStreamingRegistration - Auth: admin role, CSRF validation, login CSRF utils, platform storage (web/desktop/iOS), auth middleware, cookie config, auth helpers, clear user stores - Editor: CodeBlockShiki extension, PaginationPlus extension, font formatting, sudolang language, Monaco loader, pagination constants/utils - Integrations: Google Calendar (API client, event transform, push service, token refresh, return URL, sync service, webhook auth/token, map attendees) - Repositories: AI settings, auth, chat message, conversation, global conversation - Billing, stripe, subscription: billing visibility, stripe config/customer/client/errors, price config, rate limit middleware, plans, usage service - Analytics: client tracker, device fingerprint - Other: haptics, iOS auth (Apple/Google), keychain plugin, navigation, hotkeys, mentions, monitoring, onboarding/FAQ, canvas CSS sanitizer, logging, tree utils, URL state, theme cookie, websocket security/events, deployment mode, tabs, workflows All tests use vitest with proper mocking (vi.mock, vi.hoisted), mock all external dependencies (database, AI providers, fetch, Capacitor plugins), and follow Arrange-Act-Assert pattern. ~18,600 lines of new test code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughA comprehensive test suite addition covering 131 test files across multiple application modules including AI utilities, authentication, editor extensions, analytics, integrations, repositories, and platform-specific functionality, with extensive external dependency mocking. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
apps/web/src/lib/mentions/__tests__/mentionConfig.test.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorRename this test file to kebab-case.
mentionConfig.test.tsbreaks the TypeScript filename convention in this repo. Please rename it tomention-config.test.ts.As per coding guidelines "Use kebab-case for filenames, except React hooks (camelCase with
useprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/mentions/__tests__/mentionConfig.test.ts` at line 1, Rename the test file mentionConfig.test.ts to kebab-case as mention-config.test.ts and update any references/imports or test-glob patterns that point to mentionConfig.test.ts (search for "mentionConfig.test" or the old filename) so the test runner and any imports continue to resolve; ensure the file export/imports inside the file remain unchanged and run the test suite to confirm the new filename is picked up.apps/web/src/lib/ai/shared/hooks/__tests__/useChatStop.test.ts-49-53 (1)
49-53:⚠️ Potential issue | 🟡 MinorClarify contradictory inline comment about rejection behavior.
The comment says the rejection is both swallowed and propagated. Keep one accurate statement to avoid confusion for future maintainers.
📝 Suggested comment rewrite
- // The hook's try/finally catches the rejection internally, so the returned - // promise should resolve (the error is swallowed by the try block). - // However, the async callback in useCallback has try { await abort() } finally { chatStop() } - // which means the rejection propagates out of the callback. We need to catch it. + // `chatStop` should run even when `abortActiveStream` rejects. + // The rejection still propagates from the callback, so this test catches it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/ai/shared/hooks/__tests__/useChatStop.test.ts` around lines 49 - 53, Update the inline comment to remove the contradictory dual claim and state clearly that although the hook's internal try/finally swallows errors, the async callback used in the test (the callback that does try { await abort() } finally { chatStop() }) will let the rejection propagate out of the callback, so the test must catch that rejection (i.e., explain that the rejection is propagated by the async callback and must be handled here).apps/web/src/lib/utils/__tests__/persist-csrf-token.test.ts-28-39 (1)
28-39:⚠️ Potential issue | 🟡 MinorMissing spy restoration for
Storage.prototype.setItem.The
warnSpyis properly restored, but theStorage.prototype.setItemspy (line 31) is not. This could leak into subsequent tests.🔧 Proposed fix
it('should handle localStorage errors gracefully', () => { vi.mocked(getCookieValue).mockReturnValue('test-token'); const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { + const setItemSpy = vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { throw new Error('Storage full'); }); expect(() => persistCsrfToken()).not.toThrow(); expect(warnSpy).toHaveBeenCalledWith('Failed to persist CSRF token:', expect.any(Error)); warnSpy.mockRestore(); + setItemSpy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/utils/__tests__/persist-csrf-token.test.ts` around lines 28 - 39, The test "should handle localStorage errors gracefully" leaves the spy on Storage.prototype.setItem active which can leak into other tests; after calling persistCsrfToken() restore the setItem mock/spy (the one created with vi.spyOn(Storage.prototype, 'setItem') / vi.spyOn) so it is removed (e.g., call mockRestore or reset the spy) and keep the existing restoration of warnSpy; ensure you reference the same Storage.prototype.setItem spy used in this test and restore it before the test exits.apps/web/src/lib/repositories/__tests__/ai-settings-repository.test.ts-71-81 (1)
71-81:⚠️ Potential issue | 🟡 MinorTest title overstates what is validated
At Line 71, the test says it verifies column selection, but it only checks non-null/id on the result. This will still pass even if extra columns are selected.
Suggested assertion upgrade
it('should select only the relevant AI columns', async () => { @@ const result = await aiSettingsRepository.getUserSettings('user-1'); expect(result).not.toBeNull(); expect(result?.id).toBe('user-1'); + expect(mockSelect).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.anything(), + currentAiProvider: expect.anything(), + currentAiModel: expect.anything(), + subscriptionTier: expect.anything(), + }) + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/repositories/__tests__/ai-settings-repository.test.ts` around lines 71 - 81, The test "should select only the relevant AI columns" currently only checks id/non-null and thus won't fail if extra columns are selected; update the test for aiSettingsRepository.getUserSettings to assert the actual selected columns by either (a) verifying mockSelectWhere was called with the expected projection/column list (check mockSelectWhere call args) or (b) asserting the returned object's keys exactly match the expected set (e.g., ['id','currentAiProvider','currentAiModel','subscriptionTier']); use mockSelectWhere and aiSettingsRepository.getUserSettings to locate where to add the assertion.apps/web/src/lib/repositories/__tests__/ai-settings-repository.test.ts-140-147 (1)
140-147:⚠️ Potential issue | 🟡 Minor
whereverification is too weakAt Line 146,
toHaveBeenCalled()does not prove the update is scoped to the requested user ID. This test can pass even with an incorrect filter.Strengthen filter assertions
-import { db } from '@pagespace/db'; +import { db, eq, users } from '@pagespace/db'; @@ it('should call where with userId eq condition', async () => { @@ - expect(mockUpdateWhere).toHaveBeenCalled(); + expect(eq).toHaveBeenCalledWith(users.id, 'user-5'); + expect(mockUpdateWhere).toHaveBeenCalledWith({ + type: 'eq', + field: users.id, + value: 'user-5', + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/repositories/__tests__/ai-settings-repository.test.ts` around lines 140 - 147, The test for aiSettingsRepository.updateProviderSettings uses mockUpdateWhere.toHaveBeenCalled(), which doesn't assert the update was scoped to the requested user; change the assertion to verify the where/filter argument includes the expected userId (e.g. assert mockUpdateWhere was called with an object containing userId: 'user-5' or use expect.any/Object matching) so the call from updateProviderSettings('user-5', ...) is explicitly checked; locate the mockUpdateWhere reference in the test and replace the weak assertion with a specific call-argument check (using expect(mockUpdateWhere).toHaveBeenCalledWith(expect.objectContaining({ userId: 'user-5' }), ...) or equivalent).apps/web/src/lib/hotkeys/__tests__/registry.test.ts-71-75 (1)
71-75:⚠️ Potential issue | 🟡 MinorTest name doesn't match assertion behavior.
The test is named "should have empty arrays for categories with no hotkeys" but the assertions only verify that
groups.editingandgroups.generalare arrays—they don't verify emptiness. If hotkeys are added to these categories, this test will still pass despite its name.📝 Suggested fix options
Option 1: If the intent is to verify empty arrays:
it('should have empty arrays for categories with no hotkeys', () => { const groups = getHotkeysByCategory(); - expect(Array.isArray(groups.editing)).toBe(true); - expect(Array.isArray(groups.general)).toBe(true); + expect(groups.editing).toEqual([]); + expect(groups.general).toEqual([]); });Option 2: If the intent is to verify arrays exist regardless of content, rename the test:
- it('should have empty arrays for categories with no hotkeys', () => { + it('should return arrays for all categories', () => { const groups = getHotkeysByCategory(); expect(Array.isArray(groups.editing)).toBe(true); expect(Array.isArray(groups.general)).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/hotkeys/__tests__/registry.test.ts` around lines 71 - 75, The test name and assertions disagree: getHotkeysByCategory is being tested but the spec only checks that groups.editing and groups.general are arrays, not that they are empty. Fix by either updating the assertions to assert emptiness (e.g., expect(groups.editing.length).toBe(0) and expect(groups.general.length).toBe(0)) if you intend to ensure no hotkeys exist, or change the test title to something like "should return arrays for known categories" if you only want to verify the values are arrays; update the test around getHotkeysByCategory and the groups.editing/groups.general expectations accordingly.apps/web/src/lib/editor/monaco/__tests__/sudolang-language.test.ts-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorReplace
as anycasts with properly typed test doubles.
as anycasts on lines 25, 36, 37, 45, 46, and 55 bypass TypeScript's type safety in this test file. Use a typed alias based on the function's parameter type instead.Suggested refactor
import { describe, it, expect, vi } from 'vitest'; import { SUDOLANG_LANGUAGE_ID, registerSudolangLanguage } from '../sudolang-language'; +type MonacoArg = Parameters<typeof registerSudolangLanguage>[0]; + describe('sudolang-language', () => { describe('SUDOLANG_LANGUAGE_ID', () => { @@ - function createMockMonaco() { - return { + function createMockMonaco() { + const monaco = { languages: { getLanguages: vi.fn(() => []), register: vi.fn(), setLanguageConfiguration: vi.fn(), setMonarchTokensProvider: vi.fn(), }, }; + return monaco as MonacoArg & typeof monaco; } @@ - registerSudolangLanguage(monaco as any); + registerSudolangLanguage(monaco); @@ - registerSudolangLanguage(monaco as any); - registerSudolangLanguage(monaco as any); + registerSudolangLanguage(monaco); + registerSudolangLanguage(monaco); @@ - registerSudolangLanguage(monaco1 as any); - registerSudolangLanguage(monaco2 as any); + registerSudolangLanguage(monaco1); + registerSudolangLanguage(monaco2); @@ - registerSudolangLanguage(monaco as any); + registerSudolangLanguage(monaco);Guideline:
**/*.{ts,tsx}must never useanytypes—always use proper TypeScript types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/editor/monaco/__tests__/sudolang-language.test.ts` at line 25, The test uses multiple unsafe "as any" casts when calling registerSudolangLanguage; replace those casts with properly typed test doubles by creating a local typed alias matching the function's parameter type (e.g., derive the parameter type of registerSudolangLanguage and declare a TestMonaco type) and build mock objects that conform to that type for each usage instead of using any; update all occurrences that cast to any (the calls around registerSudolangLanguage and other monaco mocks) to use the typed mock instances so TypeScript enforces the expected shape.apps/web/src/lib/tree/__tests__/sortable-tree.test.ts-349-358 (1)
349-358:⚠️ Potential issue | 🟡 MinorThe “no ids match” case does not actually validate descendant preservation.
Both flattened entries are root-level (
parentId: null), so this test can pass even if descendant filtering is broken.🛠️ Suggested fixture correction
it('should return same array if no ids match', () => { const root = item('a'); const child = item('b'); const flatList: FlattenedItem<TreeItem>[] = [ flat(root, null, 0, 0), - flat(child, null, 0, 1), + flat(child, 'a', 1, 0), ]; const result = removeChildrenOf(flatList, ['z']); - expect(result).toHaveLength(2); + expect(result.map(r => r.item.id)).toEqual(['a', 'b']); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/tree/__tests__/sortable-tree.test.ts` around lines 349 - 358, Test setup uses two root-level entries so removeChildrenOf(['z']) can't verify descendant preservation; update the fixture so one entry is a descendant of the other (use item('a') as root and item('b') as a child) by building the flattened list with the child's parentId set to the root's id (i.e. use flat(child, root.id, ...) or otherwise set parentId to root.id) and keep asserting length remains 2 after calling removeChildrenOf(flatList, ['z']).apps/web/src/lib/onboarding/faq/__tests__/seed-template.test.ts-132-137 (1)
132-137:⚠️ Potential issue | 🟡 MinorTest doesn't verify the non-singleton claim.
The test name states "returns a new object on each call (not a singleton)" but only asserts
result1.title === result2.title. To verify non-singleton behavior, you should also assert that the object references are different.🔧 Proposed fix to verify distinct references
it('returns a new object on each call (not a singleton)', () => { const result1 = getReferenceSeedTemplate(); const result2 = getReferenceSeedTemplate(); - // Same structure but different object references - expect(result1.title).toBe(result2.title); + // Same structure but different object references + expect(result1).not.toBe(result2); + expect(result1).toEqual(result2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/onboarding/faq/__tests__/seed-template.test.ts` around lines 132 - 137, The test named "returns a new object on each call (not a singleton)" currently only compares titles; update the assertion to verify distinct object references from getReferenceSeedTemplate by asserting the two returned objects are not the same reference (e.g., use an identity check such as expect(result1).not.toBe(result2)) and optionally mutate a property on one (or check nested object identity) to ensure the second is unaffected; locate the test using the getReferenceSeedTemplate calls in the existing spec to make the change.apps/web/src/lib/onboarding/faq/__tests__/content-page-types.test.ts-53-58 (1)
53-58:⚠️ Potential issue | 🟡 MinorMock state may leak between tests, causing index mismatch.
The test accesses
mock.results[0].value, but since the previous test (line 48-51) also callsbuildBudgetSheetContent(), the mock's results array will have multiple entries. This test should either:
- Clear mocks before each test using
beforeEach(() => vi.clearAllMocks())- Access the correct index (results[1] in this case)
- Use
mock.results.at(-1)to get the most recent result🔧 Proposed fix: Add beforeEach to clear mocks
describe('buildBudgetSheetContent', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + it('should call createEmptySheet with 20 rows and 8 cols', () => {Or alternatively, use the last result:
- const sheet = vi.mocked(createEmptySheet).mock.results[0].value; + const sheet = vi.mocked(createEmptySheet).mock.results.at(-1)?.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/onboarding/faq/__tests__/content-page-types.test.ts` around lines 53 - 58, The test is reading createEmptySheet's mock.results[0].value which can be stale across tests; update the test suite to avoid mock state leakage by either adding a beforeEach(() => vi.clearAllMocks()) in the describe block so each call to buildBudgetSheetContent() produces a fresh mock.results array, or change the assertion to read the most recent mock result (e.g., use vi.mocked(createEmptySheet).mock.results.at(-1).value) when inspecting the created sheet; ensure references to buildBudgetSheetContent and createEmptySheet are updated accordingly.apps/web/src/lib/repositories/__tests__/auth-repository.test.ts-113-118 (1)
113-118:⚠️ Potential issue | 🟡 Minor
incrementUserTokenVersiontest should verify increment semantics, not mere presence.Checking
tokenVersionas “defined” is too permissive; a static assignment would still pass. Assert that the set payload represents an increment expression/derived value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/repositories/__tests__/auth-repository.test.ts` around lines 113 - 118, The test currently only checks presence of tokenVersion; update the assertion in the authRepository.incrementUserTokenVersion test to verify an increment expression/derived value rather than a plain value: retrieve setArg = mockUpdateSet.mock.calls[0][0] and assert that setArg.tokenVersion is not a number (e.g., typeof !== 'number') and that it represents a SQL expression/raw (for example, assert it has methods/properties like toString()/toSQL() or that its string contains an increment pattern such as "token_version" and "+" or " + 1"); ensure you reference incrementUserTokenVersion, mockUpdateSet, and tokenVersion when changing the expectation.apps/web/src/lib/repositories/__tests__/chat-message-repository.test.ts-112-116 (1)
112-116:⚠️ Potential issue | 🟡 MinorStrengthen filter-condition assertions for query/delete safety.
These tests only assert that
wherewas called, not that the expected predicates were built. That can miss regressions that broaden scope unintentionally.Also applies to: 204-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/repositories/__tests__/chat-message-repository.test.ts` around lines 112 - 116, Update the tests to assert the actual predicate used in the where call instead of only checking that where was called: for the getMessagesForPage test referencing chatMessageRepository.getMessagesForPage and mockSelectWhere, verify that mockSelectWhere was called with a filter that includes the conversation id (e.g., predicate or object containing conversationId/ conversation_id: 'conv-1'); likewise update the corresponding delete test(s) (the ones around lines 204-209 that use chatMessageRepository.deleteMessagesForPage and mockDeleteWhere) to assert the delete predicate includes the same conversation id condition so the query/delete scope cannot regress.apps/web/src/lib/repositories/__tests__/auth-repository.test.ts-72-78 (1)
72-78:⚠️ Potential issue | 🟡 MinorTighten predicate assertions to validate actual filter values.
These tests currently confirm that a
whereexists, but not that it contains the intended user/email constraints. They can pass with incorrect predicates.✅ Suggested assertion strengthening
it('should pass the email to the query', async () => { mockFindFirst.mockResolvedValue(null); await authRepository.findUserByEmail('test@test.com'); expect(mockFindFirst).toHaveBeenCalledWith( expect.objectContaining({ where: expect.anything() }) ); + // Also assert predicate builder input(s) here. }); it('should apply where condition for userId and isNull(revokedAt)', async () => { await authRepository.revokeAllUserDeviceTokens('user-1'); - expect(mockUpdateWhere).toHaveBeenCalled(); + expect(mockUpdateWhere).toHaveBeenCalledWith(expect.anything()); + // Also assert userId + non-revoked condition inputs here. });Also applies to: 134-137, 153-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/repositories/__tests__/auth-repository.test.ts` around lines 72 - 78, The test currently only asserts that mockFindFirst was called with an object containing a where key; change it to assert the actual filter value is passed (e.g., for authRepository.findUserByEmail, assert mockFindFirst was called with expect.objectContaining({ where: { email: 'test@test.com' }}) or inspect mockFindFirst.mock.calls[0][0].where and assert equality), and apply the same tightening for the other tests in this file that use mockFindFirst so each verifies the specific predicate field(s) (email/ID/provider, etc.) rather than just the presence of where.apps/web/src/lib/repositories/__tests__/conversation-repository.test.ts-296-303 (1)
296-303:⚠️ Potential issue | 🟡 MinorAssert the soft-delete filter, not just update invocation.
This test currently passes even if the repository accidentally updates all rows. Please also verify the
wherepredicate is applied with conversation scoping.🔍 Suggested assertion hardening
it('should set isActive to false for all messages in conversation', async () => { await conversationRepository.softDeleteConversation('agent-1', 'conv-1'); expect(db.update).toHaveBeenCalled(); expect(mockUpdateSet).toHaveBeenCalledWith( expect.objectContaining({ isActive: false }) ); + expect(mockUpdateWhere).toHaveBeenCalledWith(expect.anything()); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/repositories/__tests__/conversation-repository.test.ts` around lines 296 - 303, Test is only asserting that db.update and mockUpdateSet were called; modify the test for conversationRepository.softDeleteConversation to also assert the WHERE predicate was scoped to the correct conversation and agent so it cannot update all rows. Specifically, add assertions that the mock/db update call received a filter object containing the conversation identifier (e.g., conversationId or id === 'conv-1') and the agent identity (e.g., agentId === 'agent-1') and (optionally) that it targeted only active messages (isActive: true) by checking the predicate argument passed to db.update/mockUpdateSet for those properties.apps/web/src/lib/ai/core/__tests__/tool-utils.test.ts-19-22 (1)
19-22:⚠️ Potential issue | 🟡 MinorAssert the merged tool definitions, not just the keys.
Lines 20-22 still pass if
mergeToolSets()returns the right property names but the wrong payloads. Check the retained and added entries directly so this actually locks down merge behavior.Suggested assertion tightening
const result = mergeToolSets(base as never, additional); - expect(result).toHaveProperty('toolA'); - expect(result).toHaveProperty('toolB'); - expect(result).toHaveProperty('toolC'); + expect(result.toolA).toBe(base.toolA); + expect(result.toolB).toBe(base.toolB); + expect(result.toolC).toBe(additional.toolC);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/ai/core/__tests__/tool-utils.test.ts` around lines 19 - 22, The test currently only checks keys; update assertions to validate merged tool definitions themselves by asserting the retained and added entries match expected payloads: verify that result.toolA deeply equals base.toolA (retained), and that result.toolB and result.toolC deeply equal the corresponding entries from additional (added). Use deep equality/matching assertions (e.g., toEqual or toMatchObject) against base and additional objects rather than only toHaveProperty.apps/web/src/lib/ai/core/__tests__/mcp-tool-converter.test.ts-249-260 (1)
249-260:⚠️ Potential issue | 🟡 MinorThis doesn't verify the unsupported-type fallback yet.
The current assertion only checks "no throw", so silently dropping
weirdor skipping the warning would still pass. Assert the warning and a successful parse of aweirdvalue to cover the fallback branch.Suggested assertion tightening
- expect(() => convertMCPToolSchemaToZod(schema)).not.toThrow(); + const result = convertMCPToolSchemaToZod(schema); + expect(consoleSpy).toHaveBeenCalled(); + expect(result.safeParse({ weird: 'anything' }).success).toBe(true); consoleSpy.mockRestore();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/ai/core/__tests__/mcp-tool-converter.test.ts` around lines 249 - 260, Update the test for convertMCPToolSchemaToZod to actually verify the unsupported-type fallback: keep the console.warn spy (consoleSpy) but assert it was called once (or with an expected message) after calling convertMCPToolSchemaToZod(schema), and then use the returned Zod schema to parse a value containing weird: null (or the expected unsupported value) to assert the parse succeeds without throwing — this ensures the function uses z.unknown() for the unsupported "null" property and that the warning branch executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0a73a44-6fe5-4b41-a641-237d08230682
📒 Files selected for processing (88)
apps/web/src/lib/__tests__/attachment-utils.test.tsapps/web/src/lib/__tests__/deployment-mode.test.tsapps/web/src/lib/__tests__/haptics.test.tsapps/web/src/lib/__tests__/ios-apple-auth.test.tsapps/web/src/lib/__tests__/ios-google-auth.test.tsapps/web/src/lib/__tests__/keychain-plugin.test.tsapps/web/src/lib/__tests__/stripe-config.test.tsapps/web/src/lib/__tests__/stripe-customer.test.tsapps/web/src/lib/__tests__/stripe-errors.test.tsapps/web/src/lib/__tests__/task-status-config.test.tsapps/web/src/lib/__tests__/theme-cookie.test.tsapps/web/src/lib/__tests__/url-state.test.tsapps/web/src/lib/ai/core/__tests__/agent-awareness.test.tsapps/web/src/lib/ai/core/__tests__/ai-utils.test.tsapps/web/src/lib/ai/core/__tests__/client.test.tsapps/web/src/lib/ai/core/__tests__/complete-request-builder.test.tsapps/web/src/lib/ai/core/__tests__/inline-instructions.test.tsapps/web/src/lib/ai/core/__tests__/mcp-tool-converter.test.tsapps/web/src/lib/ai/core/__tests__/model-capabilities.test.tsapps/web/src/lib/ai/core/__tests__/page-tree-context.test.tsapps/web/src/lib/ai/core/__tests__/personalization-utils.test.tsapps/web/src/lib/ai/core/__tests__/schema-introspection.test.tsapps/web/src/lib/ai/core/__tests__/system-prompt.test.tsapps/web/src/lib/ai/core/__tests__/tool-filtering.test.tsapps/web/src/lib/ai/core/__tests__/tool-utils.test.tsapps/web/src/lib/ai/core/__tests__/vision-models.test.tsapps/web/src/lib/ai/shared/__tests__/agent-conversations.test.tsapps/web/src/lib/ai/shared/__tests__/chat-types.test.tsapps/web/src/lib/ai/shared/__tests__/error-messages.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useChatStop.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useChatTransport.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useConversations.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useMCPTools.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useMessageActions.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useProviderSettings.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useSendHandoff.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useStreamRecovery.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useStreamingRegistration.test.tsapps/web/src/lib/analytics/__tests__/client-tracker.test.tsapps/web/src/lib/analytics/__tests__/device-fingerprint.test.tsapps/web/src/lib/auth/__tests__/admin-role.test.tsapps/web/src/lib/auth/__tests__/auth-index.test.tsapps/web/src/lib/auth/__tests__/login-csrf-utils.test.tsapps/web/src/lib/auth/platform-storage/__tests__/desktop-storage.test.tsapps/web/src/lib/auth/platform-storage/__tests__/ios-storage.test.tsapps/web/src/lib/auth/platform-storage/__tests__/web-storage.test.tsapps/web/src/lib/billing/__tests__/billing-visibility.test.tsapps/web/src/lib/canvas/__tests__/css-sanitizer.test.tsapps/web/src/lib/editor/__tests__/font-formatting.test.tsapps/web/src/lib/editor/code-block/__tests__/CodeBlockShikiExtension.test.tsapps/web/src/lib/editor/monaco/__tests__/sudolang-language.test.tsapps/web/src/lib/editor/pagination/__tests__/PaginationExtension.test.tsapps/web/src/lib/editor/pagination/__tests__/constants.test.tsapps/web/src/lib/editor/pagination/__tests__/utils.test.tsapps/web/src/lib/hotkeys/__tests__/registry.test.tsapps/web/src/lib/integrations/google-calendar/__tests__/api-client.test.tsapps/web/src/lib/integrations/google-calendar/__tests__/event-transform.test.tsapps/web/src/lib/integrations/google-calendar/__tests__/push-service.test.tsapps/web/src/lib/integrations/google-calendar/__tests__/return-url.test.tsapps/web/src/lib/integrations/google-calendar/__tests__/token-refresh.test.tsapps/web/src/lib/logging/__tests__/client-logger.test.tsapps/web/src/lib/logging/__tests__/mask.test.tsapps/web/src/lib/mentions/__tests__/mentionConfig.test.tsapps/web/src/lib/monitoring/__tests__/monitoring-queries.test.tsapps/web/src/lib/navigation/__tests__/app-navigation.test.tsapps/web/src/lib/onboarding/__tests__/drive-setup.test.tsapps/web/src/lib/onboarding/__tests__/onboarding-faq.test.tsapps/web/src/lib/onboarding/faq/__tests__/about-agent-system-prompt.test.tsapps/web/src/lib/onboarding/faq/__tests__/content-other.test.tsapps/web/src/lib/onboarding/faq/__tests__/content-page-types.test.tsapps/web/src/lib/onboarding/faq/__tests__/example-agent-prompts.test.tsapps/web/src/lib/onboarding/faq/__tests__/knowledge-base.test.tsapps/web/src/lib/onboarding/faq/__tests__/seed-template.test.tsapps/web/src/lib/repositories/__tests__/ai-settings-repository.test.tsapps/web/src/lib/repositories/__tests__/auth-repository.test.tsapps/web/src/lib/repositories/__tests__/chat-message-repository.test.tsapps/web/src/lib/repositories/__tests__/conversation-repository.test.tsapps/web/src/lib/repositories/__tests__/global-conversation-repository.test.tsapps/web/src/lib/stripe/__tests__/client.test.tsapps/web/src/lib/stripe/__tests__/price-config.test.tsapps/web/src/lib/subscription/__tests__/rate-limit-middleware.test.tsapps/web/src/lib/tree/__tests__/sortable-tree.test.tsapps/web/src/lib/utils/__tests__/formatters.test.tsapps/web/src/lib/utils/__tests__/persist-csrf-token.test.tsapps/web/src/lib/utils/__tests__/query-params.test.tsapps/web/src/lib/utils/__tests__/utils.test.tsapps/web/src/lib/websocket/__tests__/calendar-events.test.tsapps/web/src/lib/websocket/__tests__/ws-security.test.ts
Comment on lines
+220
to
+236
| it('should skip dangerous property names to prevent prototype pollution', () => { | ||
| const schema = { | ||
| type: 'object' as const, | ||
| properties: { | ||
| __proto__: { type: 'string' }, | ||
| constructor: { type: 'string' }, | ||
| prototype: { type: 'string' }, | ||
| safe_field: { type: 'string' }, | ||
| }, | ||
| required: [], | ||
| }; | ||
|
|
||
| expect(() => convertMCPToolSchemaToZod(schema)).not.toThrow(); | ||
| const result = convertMCPToolSchemaToZod(schema); | ||
| const parsed = result.safeParse({ safe_field: 'value' }); | ||
| expect(parsed.success).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const literal = { __proto__: { polluted: true }, safe_field: 1 };
console.log('literal own keys:', Object.keys(literal));
console.log('literal has own "__proto__":', Object.prototype.hasOwnProperty.call(literal, '__proto__'));
console.log('literal prototype polluted:', Object.getPrototypeOf(literal).polluted === true);
const computed = { ['__proto__']: { polluted: true }, safe_field: 1 };
console.log('computed own keys:', Object.keys(computed));
console.log('computed has own "__proto__":', Object.prototype.hasOwnProperty.call(computed, '__proto__'));
console.log('computed prototype polluted:', Object.getPrototypeOf(computed).polluted === true);
NODERepository: 2witstudios/PageSpace
Length of output: 286
🏁 Script executed:
cat -n apps/web/src/lib/ai/core/__tests__/mcp-tool-converter.test.ts | sed -n '220,236p'Repository: 2witstudios/PageSpace
Length of output: 795
Use computed key ['__proto__'] to actually test the guard.
__proto__: in an object literal changes the object's prototype instead of creating an own "__proto__" property. The test as written doesn't exercise the prototype-pollution guard you're naming—it only covers constructor and prototype. Use ['__proto__'] to properly create the property that your conversion function should filter out.
Suggested fix
const schema = {
type: 'object' as const,
properties: {
- __proto__: { type: 'string' },
+ ['__proto__']: { type: 'string' },
constructor: { type: 'string' },
prototype: { type: 'string' },
safe_field: { type: 'string' },
},
required: [],
};
expect(() => convertMCPToolSchemaToZod(schema)).not.toThrow();
const result = convertMCPToolSchemaToZod(schema);
const parsed = result.safeParse({ safe_field: 'value' });
expect(parsed.success).toBe(true);
+ if (parsed.success) {
+ expect(Object.prototype.hasOwnProperty.call(parsed.data, '__proto__')).toBe(false);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/ai/core/__tests__/mcp-tool-converter.test.ts` around lines
220 - 236, The test is not actually creating an own "__proto__" property because
using __proto__: in an object literal sets the prototype instead; update the
test so the schema includes the dangerous key using a computed property (e.g.,
['__proto__']: { type: 'string' }) to ensure convertMCPToolSchemaToZod's
prototype-pollution guard is exercised, then keep the rest of the assertions (no
throw and safeParse of { safe_field: 'value' } succeeding) as-is to verify the
function filters that key.
Comment on lines
+26
to
+33
| function makeMessage(id: string, role: 'user' | 'assistant', text: string): UIMessage { | ||
| return { | ||
| id, | ||
| role, | ||
| content: text, | ||
| parts: [{ type: 'text' as const, text }], | ||
| createdAt: new Date(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Use message parts payloads in edit-path fixtures/assertions.
These tests currently lock in a content-based edit payload, which conflicts with the project’s message-content shape convention and can preserve non-compliant behavior.
💡 Suggested update
function makeMessage(id: string, role: 'user' | 'assistant', text: string): UIMessage {
return {
id,
role,
- content: text,
parts: [{ type: 'text' as const, text }],
createdAt: new Date(),
};
}
@@
expect(mockPatch).toHaveBeenCalledWith(
'/api/ai/page-agents/agent-1/conversations/conv-1/messages/m1',
- { content: 'Updated text' }
+ { parts: [{ type: 'text', text: 'Updated text' }] }
);
@@
expect(mockPatch).toHaveBeenCalledWith(
'/api/ai/global/conv-1/messages/m1',
- { content: 'Updated text' }
+ { parts: [{ type: 'text', text: 'Updated text' }] }
);As per coding guidelines, "Message content should always use the message parts structure with { parts: [{ type: 'text', text: '...' }] }."
Also applies to: 146-149, 169-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/ai/shared/hooks/__tests__/useMessageActions.test.ts` around
lines 26 - 33, Tests in useMessageActions.test.ts are constructing/editing
messages using the deprecated content property instead of the canonical parts
structure; update fixtures and assertions (including the makeMessage helper and
the edit-path test payloads referenced around the makeMessage usage and at the
other locations called out) to use { parts: [{ type: 'text', text: '...' }] }
for message content and edit payloads so message shape matches UIMessage
conventions; locate usages of makeMessage and any assertions or fixtures that
pass or expect content and replace them to assert/produce parts-based payloads
consistently.
Comment on lines
+5
to
+15
| const mockStartStreaming = vi.fn(); | ||
| const mockEndStreaming = vi.fn(); | ||
|
|
||
| vi.mock('@/stores/useEditingStore', () => ({ | ||
| useEditingStore: { | ||
| getState: () => ({ | ||
| startStreaming: mockStartStreaming, | ||
| endStreaming: mockEndStreaming, | ||
| }), | ||
| }, | ||
| })); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Vitest version used by this repository, when a vi.mock()factory references top-levelconstvariables, isvi.hoisted() required to avoid hoisting/TDZ initialization-order issues?
💡 Result:
Yes—in modern Vitest (including current docs for vi.mock) the vi.mock() call is hoisted to the top of the file, so its factory can run before your file’s top-level const/let initializers. If the factory closes over top-level const/let bindings, that can trigger TDZ / initialization-order errors unless those values are made available before the hoisted mock runs. (v3.vitest.dev)
To avoid that, you typically must do one of the following:
- Use
vi.hoisted(() => …)to create the values beforevi.mockexecutes. (main.vitest.dev) - Or use
vi.doMock(), which is not hoisted, specifically so you can reference variables from the surrounding top-level scope. (vitest.dev)
If you paste the repo’s package.json (or the exact Vitest version), I can confirm this against that specific version—but the hoisting/TDZ behavior described above is the intended behavior in the Vitest docs. (v3.vitest.dev)
Citations:
- 1: https://v3.vitest.dev/api/vi?utm_source=openai
- 2: https://main.vitest.dev/api/vi?utm_source=openai
- 3: https://vitest.dev/api/vi?utm_source=openai
- 4: https://v3.vitest.dev/api/vi?utm_source=openai
🏁 Script executed:
find . -name "package.json" -type f | head -5 | xargs grep -l "vitest" 2>/dev/null | head -3Repository: 2witstudios/PageSpace
Length of output: 92
🏁 Script executed:
cat package.json 2>/dev/null | grep -A 2 '"vitest"' || find . -maxdepth 2 -name "package.json" -exec grep -H "vitest" {} \; | head -5Repository: 2witstudios/PageSpace
Length of output: 128
🏁 Script executed:
# Check the exact test file to confirm it matches the review snippet
cat -n "apps/web/src/lib/ai/shared/hooks/__tests__/useStreamingRegistration.test.ts" 2>/dev/null | head -30Repository: 2witstudios/PageSpace
Length of output: 1247
Use vi.hoisted() for mock functions referenced inside vi.mock.
In Vitest v2 (used in this repository), vi.mock() is hoisted to the file's top before your top-level const initializers run. The factory closes over mockStartStreaming and mockEndStreaming, referencing them before they're initialized. Use vi.hoisted() to create these mocks before the hoisted vi.mock() executes:
-const mockStartStreaming = vi.fn();
-const mockEndStreaming = vi.fn();
+const { mockStartStreaming, mockEndStreaming } = vi.hoisted(() => ({
+ mockStartStreaming: vi.fn(),
+ mockEndStreaming: vi.fn(),
+}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mockStartStreaming = vi.fn(); | |
| const mockEndStreaming = vi.fn(); | |
| vi.mock('@/stores/useEditingStore', () => ({ | |
| useEditingStore: { | |
| getState: () => ({ | |
| startStreaming: mockStartStreaming, | |
| endStreaming: mockEndStreaming, | |
| }), | |
| }, | |
| })); | |
| const { mockStartStreaming, mockEndStreaming } = vi.hoisted(() => ({ | |
| mockStartStreaming: vi.fn(), | |
| mockEndStreaming: vi.fn(), | |
| })); | |
| vi.mock('@/stores/useEditingStore', () => ({ | |
| useEditingStore: { | |
| getState: () => ({ | |
| startStreaming: mockStartStreaming, | |
| endStreaming: mockEndStreaming, | |
| }), | |
| }, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/ai/shared/hooks/__tests__/useStreamingRegistration.test.ts`
around lines 5 - 15, The test's vi.mock factory closes over mockStartStreaming
and mockEndStreaming before they're initialized because vi.mock is hoisted;
replace the top-level consts with hoisted mocks using vi.hoisted to ensure
mockStartStreaming and mockEndStreaming are created before the vi.mock factory
runs (reference symbols: mockStartStreaming, mockEndStreaming, vi.hoisted,
vi.mock, and the mocked module '@/stores/useEditingStore'); update the test to
call vi.hoisted for each mock function and keep the vi.mock factory unchanged so
the factory references the already-hoisted mocks.
Comment on lines
+124
to
+131
| it('should return false for an expired token (age > maxAge)', () => { | ||
| // Create a token with a timestamp from 10 minutes ago | ||
| const pastTimestamp = Math.floor(Date.now() / 1000) - 600; | ||
| const { createHmac } = require('crypto'); | ||
| const tokenValue = 'a'.repeat(64); | ||
| const payload = `${tokenValue}.${pastTimestamp}`; | ||
| const signature = createHmac('sha256', validSecret).update(payload).digest('hex'); | ||
| const expiredToken = `${tokenValue}.${pastTimestamp}.${signature}`; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Module type declarations =="
fd 'package.json$' -x sh -c 'echo "--- {} ---"; jq -r ".type // \"(no type field)\"" "{}"'
echo
echo "== CommonJS require() usage in TS/TSX under apps/web/src/lib =="
rg -n --type=ts --type=tsx '\brequire\s*\(' apps/web/src/libRepository: 2witstudios/PageSpace
Length of output: 740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if test file exists =="
if [ -f "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" ]; then
echo "File exists"
wc -l "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts"
else
echo "File not found at expected path"
# Try to locate it
echo "Searching for login-csrf-utils.test.ts..."
find . -name "login-csrf-utils.test.ts" 2>/dev/null || echo "Not found"
fi
echo
echo "== Reading lines 124-131 from the test file =="
if [ -f "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" ]; then
sed -n '124,131p' "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" | cat -n
fi
echo
echo "== Searching for 'require.*crypto' in test file =="
if [ -f "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" ]; then
rg "require.*crypto" "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" -n || echo "No matches found"
fi
echo
echo "== Check imports at top of test file =="
if [ -f "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" ]; then
head -20 "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" | cat -n
fiRepository: 2witstudios/PageSpace
Length of output: 1659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All require() calls in the test file =="
rg "require\s*\(" "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" -n
echo
echo "== Check vitest/tsconfig configuration =="
if [ -f "apps/web/vitest.config.ts" ]; then
echo "--- vitest.config.ts ---"
cat "apps/web/vitest.config.ts"
fi
if [ -f "vitest.config.ts" ]; then
echo "--- Root vitest.config.ts ---"
cat "vitest.config.ts"
fi
if [ -f "apps/web/tsconfig.json" ]; then
echo "--- apps/web/tsconfig.json (first 30 lines) ---"
head -30 "apps/web/tsconfig.json" | cat -n
fi
echo
echo "== Check if crypto is used elsewhere in the lib/auth directory =="
rg "crypto" "apps/web/src/lib/auth" -l --type=tsRepository: 2witstudios/PageSpace
Length of output: 4770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read lines 120-200 to verify context and all require() locations =="
sed -n '120,200p' "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" | cat -n
echo
echo "== Verify the actual implementation file uses crypto =="
head -20 "apps/web/src/lib/auth/login-csrf-utils.ts" | cat -nRepository: 2witstudios/PageSpace
Length of output: 5245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Verify there are NO other require() calls in the test file =="
rg "require\s*\(" "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" -c
echo
echo "== Confirm all three violations in context =="
# Lines 127, 139, 192 should all be the problematic require calls
echo "=== Line 127 ==="
sed -n '125,130p' "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" | cat -n
echo "=== Line 139 ==="
sed -n '137,142p' "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" | cat -n
echo "=== Line 192 ==="
sed -n '190,195p' "apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts" | cat -nRepository: 2witstudios/PageSpace
Length of output: 1391
Replace CommonJS require with ESM import for crypto module.
Lines 127, 139, and 192 use require('crypto'), which violates the repo's ESM-only guideline. The implementation file already uses ESM imports for crypto, and this test file must be consistent.
♻️ Proposed fix
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { createHmac } from 'crypto';
import {
generateLoginCSRFToken,
validateLoginCSRFToken,
LOGIN_CSRF_COOKIE_NAME,
LOGIN_CSRF_MAX_AGE,
} from '../login-csrf-utils';
@@
- const { createHmac } = require('crypto');
const tokenValue = 'a'.repeat(64);
const payload = `${tokenValue}.${pastTimestamp}`;
const signature = createHmac('sha256', validSecret).update(payload).digest('hex');
@@
- const { createHmac } = require('crypto');
const tokenValue = 'a'.repeat(64);
const payload = `${tokenValue}.${futureTimestamp}`;
const signature = createHmac('sha256', validSecret).update(payload).digest('hex');
@@
- const { createHmac } = require('crypto');
const pastTimestamp = Math.floor(Date.now() / 1000) - 5;
const tokenValue = 'c'.repeat(64);
const payload = `${tokenValue}.${pastTimestamp}`;
const signature = createHmac('sha256', validSecret).update(payload).digest('hex');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/auth/__tests__/login-csrf-utils.test.ts` around lines 124 -
131, The test uses CommonJS require('crypto') inside tests (see the "should
return false for an expired token (age > maxAge)" test and other tests
referencing createHmac) which violates the ESM-only guideline; replace those
inline require calls with ESM imports by adding an import like "import {
createHmac } from 'crypto'" at the top of the test file and remove the
require(...) usages (affecting the occurrences around the token creation in this
test and the other spots noted), then update references to use the imported
createHmac to generate signatures.
Comment on lines
+55
to
+64
| it('should fall back to window.open when Capacitor Browser import fails', async () => { | ||
| vi.mocked(isCapacitorApp).mockReturnValue(true); | ||
| const openSpy = vi.spyOn(window, 'open').mockImplementation(() => null); | ||
|
|
||
| await openExternalUrl('https://example.com'); | ||
|
|
||
| expect(openSpy).toHaveBeenCalled(); | ||
| expect(openSpy.mock.calls[0][0]).toBe('https://example.com'); | ||
| expect(openSpy.mock.calls[0][1]).toBe('_blank'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect openExternalUrl implementation (dynamic import + fallback logic):"
fd 'app-navigation\.ts$' --exec sed -n '1,260p' {}
echo
echo "2) Check whether `@capacitor/browser` is declared in workspace package manifests:"
rg -n --glob '**/package.json' '"@capacitor/browser"'
echo
echo "3) Inspect current test mocks for the fallback case:"
fd 'app-navigation\.test\.ts$' --exec sed -n '1,240p' {}Repository: 2witstudios/PageSpace
Length of output: 7424
Mock the @capacitor/browser import failure explicitly to make the test deterministic.
The test title claims the fallback occurs "when Capacitor Browser import fails", but it does not mock the dynamic import to fail. Since @capacitor/browser is in the workspace dependencies, the import will succeed in a normal test environment, leaving the catch block unreached. The test may pass accidentally (the spy catches window.open being called), but not through the intended fallback path.
To test the actual fallback behavior, explicitly mock the @capacitor/browser import to throw an error:
vi.doMock('@capacitor/browser', () => {
return Promise.reject(new Error('Module not available'));
});Or use vi.mock('@capacitor/browser', { default: () => { throw new Error(); } }) depending on the module format. This ensures the catch block is executed and the test is environment-independent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/navigation/__tests__/app-navigation.test.ts` around lines 55
- 64, The test for openExternalUrl claims to exercise the fallback when the
Capacitor Browser import fails but doesn't force the dynamic import to fail;
update the test to explicitly mock the '@capacitor/browser' import to throw
(e.g., using vi.doMock or vi.mock to return a rejected promise/throwing default)
before invoking openExternalUrl while keeping isCapacitorApp mocked true and the
window.open spy; this ensures the catch path in openExternalUrl is executed and
the window.open assertions validate the intended fallback behavior.
Comment on lines
+294
to
+310
| it('sets isTrashed to false on all pages', async () => { | ||
| const customClient = { insert: mockInsert }; | ||
| await populateUserDrive('user-1', 'drive-1', customClient as never); | ||
|
|
||
| const pageInsertCalls = mockInsert.mock.calls.filter((call) => call[0] === pages); | ||
| const valuesWithTrashed = pageInsertCalls.map( | ||
| (_, i) => mockInsertValues.mock.calls[i]?.[0] | ||
| ); | ||
|
|
||
| const allPageValues = mockInsertValues.mock.calls | ||
| .map((call) => call[0]) | ||
| .filter((val) => val && val.isTrashed !== undefined); | ||
|
|
||
| allPageValues.forEach((val) => { | ||
| expect(val.isTrashed).toBe(false); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
isTrashed test can miss missing-field regressions.
At Line 303-305, filtering by val.isTrashed !== undefined means a page without isTrashed is silently excluded instead of failing the test.
💡 Tighten the assertion to enforce presence and value
- const pageInsertCalls = mockInsert.mock.calls.filter((call) => call[0] === pages);
- const valuesWithTrashed = pageInsertCalls.map(
- (_, i) => mockInsertValues.mock.calls[i]?.[0]
- );
-
- const allPageValues = mockInsertValues.mock.calls
- .map((call) => call[0])
- .filter((val) => val && val.isTrashed !== undefined);
-
- allPageValues.forEach((val) => {
- expect(val.isTrashed).toBe(false);
- });
+ const pageValues = mockInsert.mock.calls
+ .map((call, i) => (call[0] === pages ? mockInsertValues.mock.calls[i]?.[0] : undefined))
+ .filter((val): val is { isTrashed?: boolean } => Boolean(val));
+
+ expect(pageValues.length).toBeGreaterThan(0);
+ pageValues.forEach((val) => {
+ expect(val).toHaveProperty('isTrashed');
+ expect(val.isTrashed).toBe(false);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/onboarding/__tests__/drive-setup.test.ts` around lines 294 -
310, The test for "sets isTrashed to false on all pages" is currently filtering
out records that lack isTrashed (val.isTrashed !== undefined) so missing-field
regressions pass; update the assertion logic in this test (around
populateUserDrive, mockInsert, mockInsertValues and pages usage) to iterate the
page insert values directly and assert each value both contains the isTrashed
property (e.g., hasOwnProperty or 'isTrashed' in value) and that value.isTrashed
=== false so the test fails if the field is omitted or set to a non-false value.
Comment on lines
+318
to
+340
| it('calculates due dates correctly for tasks with dueInDays', async () => { | ||
| const customClient = { insert: mockInsert }; | ||
| await populateUserDrive('user-1', 'drive-1', customClient as never); | ||
|
|
||
| const taskItemValuesCalls = mockInsertValues.mock.calls.filter((call) => { | ||
| const val = call[0]; | ||
| return val && val.taskListId !== undefined; | ||
| }); | ||
|
|
||
| // Task with dueInDays: 0 should have dueDate equal to now (same day) | ||
| const taskWithDue0 = taskItemValuesCalls.find( | ||
| (call) => call[0].title === 'Open and edit Example Notes' | ||
| ); | ||
| expect(taskWithDue0).toBeDefined(); | ||
| expect(taskWithDue0![0].dueDate).toBeInstanceOf(Date); | ||
|
|
||
| // Task with dueInDays: 2 should have dueDate 2 days in the future | ||
| const taskWithDue2 = taskItemValuesCalls.find( | ||
| (call) => call[0].title === 'Create your first project folder' | ||
| ); | ||
| expect(taskWithDue2).toBeDefined(); | ||
| expect(taskWithDue2![0].dueDate).toBeInstanceOf(Date); | ||
| }); |
Contributor
There was a problem hiding this comment.
Due-date test does not validate the actual offsets.
At Line 332 and Line 339, checking only Date instances does not prove dueInDays: 0 and dueInDays: 2 are computed correctly.
💡 Assert exact dates with a fixed clock
it('calculates due dates correctly for tasks with dueInDays', async () => {
+ vi.useFakeTimers();
+ const now = new Date('2026-01-01T12:00:00.000Z');
+ vi.setSystemTime(now);
+
const customClient = { insert: mockInsert };
await populateUserDrive('user-1', 'drive-1', customClient as never);
@@
- expect(taskWithDue0![0].dueDate).toBeInstanceOf(Date);
+ expect(taskWithDue0![0].dueDate?.toISOString()).toBe(now.toISOString());
@@
- expect(taskWithDue2![0].dueDate).toBeInstanceOf(Date);
+ const expectedDue2 = new Date(now);
+ expectedDue2.setDate(expectedDue2.getDate() + 2);
+ expect(taskWithDue2![0].dueDate?.toISOString()).toBe(expectedDue2.toISOString());
+
+ vi.useRealTimers();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/onboarding/__tests__/drive-setup.test.ts` around lines 318 -
340, The test only asserts that dueDate is a Date instance; instead freeze the
clock (e.g., set a fixed system time via Jest fake timers or mock Date.now)
before calling populateUserDrive('user-1','drive-1', ...) and then replace the
.toBeInstanceOf(Date) assertions for the entries found via
mockInsertValues.mock.calls (taskWithDue0 and taskWithDue2) with exact equality
checks against the fixed time (for dueInDays: 0) and fixed time + 2 days (for
dueInDays: 2); ensure you reset/restore the clock after the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
apps/web/src/lib/vi.mock,vi.hoisted), mock all external dependencies (database, AI providers, fetch, Capacitor plugins), and follow Arrange-Act-Assert patternCoverage areas
Test plan
pnpm vitest run— 405 test files pass (3 pre-existing DB integration failures unrelated to this PR)🤖 Generated with Claude Code
Summary by CodeRabbit