289 factory based testing pattern#290
Conversation
…m/InfinityBowman/corates into 289-factory-based-testing-pattern
…m/InfinityBowman/corates into 289-factory-based-testing-pattern
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces comprehensive test factory infrastructure to standardize test data creation, adds extensive planning documentation for architectural patterns (local-first, command pattern, authorization), and refactors multiple test files to replace seed-based setup with factory-based data generation, alongside documentation enhancements for Claude plugin skills and agents. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The review requires assessing: (1) correctness and completeness of new factory abstractions across six modules with interdependencies; (2) consistency and semantic equivalence of extensive test rewiring across 11+ test files with context-specific assertions and authentication handling; (3) alignment of dynamic ID generation and header-based user context across heterogeneous test scenarios; (4) documentation accuracy for planning documents with architectural and pattern details. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 781426a | Commit Preview URL | Jan 14 2026, 07:42 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/docs/audits/command-pattern-implementation-plan.md (1)
186-197: Document org membership prerequisite for project invitations.Based on learnings, the project invitation flow must create org membership before project membership, including both orgRole and projectRole in invitation data. The responsibilities should explicitly mention this requirement.
Suggested addition
**Responsibilities**: +- Ensure org membership exists or will be created during acceptance - Create/update invitation record - Generate magic link - Queue invitation emailAdditionally, update the interface documentation to clarify that invitations must include orgRole:
-export async function inviteMember(env, actor, { projectId, email, role }) { - // Returns: { invitation: true, email } +export async function inviteMember(env, actor, { projectId, email, projectRole, orgRole }) { + // Returns: { invitation: true, email } + // Note: Creates org membership during acceptance if user doesn't exist }Based on learnings, project invitation flow must create org membership before project membership.
packages/workers/src/routes/__tests__/database.test.js (1)
24-37: Default mock user ID may not exist in factory-based tests.The mock defaults to
'user-1'when nox-test-user-idheader is provided (line 27), but factory-generated users have IDs likeusr_.... Tests that don't explicitly set the header (e.g., "should require authentication" on line 83) will authenticate as a non-existent user.Consider updating the mock to derive its default from a factory-created user or ensure all tests explicitly provide the header.
Suggested approach
Either ensure all tests provide the header explicitly (current approach in line 72), or refactor the mock to require the header without a fallback:
return { requireAuth: async (c, next) => { - const userId = c.req.raw.headers.get('x-test-user-id') || 'user-1'; + const userId = c.req.raw.headers.get('x-test-user-id'); + if (!userId) { + return c.json({ error: 'Unauthorized' }, 401); + } c.set('user', { id: userId,packages/workers/src/routes/__tests__/org-auth.test.js (1)
66-72: Remove unnecessaryclearProjectDOscall — tests don't create ProjectDoc DOs.The
clearProjectDOs(['project-1', 'project-2'])call uses hardcoded IDs that won't match the dynamically generated project IDs created bybuildProject()(e.g.,proj-<uuid>). Since this test only exercises HTTP authorization routes and doesn't create ProjectDoc Durable Objects, this cleanup is unnecessary and can be removed. If future changes add actual ProjectDoc operations, add a separate helper to track and clear created project IDs dynamically.
🤖 Fix all issues with AI agents
In `@packages/docs/audits/local-first-comparison-outline-corates.md`:
- Around line 100-117: The Dexie schema in CoratesDB.constructor uses invalid
type annotations and inline comments inside the stores() strings; update the
version(1).stores call so each table schema string contains only comma-separated
indexed field names (e.g., change "projects: 'id, orgId, updatedAt, ydoc:
Y.Doc'" to "projects: 'id, orgId, updatedAt, ydoc'") and move any type info or
comments outside the schema strings as regular code comments; apply the same fix
for any other entries that include annotations or comments inside the quotes
(e.g., ensure pdfs, avatars, formStates, localChecklists, queryCache strings
contain only field names).
In `@packages/workers/src/routes/__tests__/avatars.test.js`:
- Around line 267-268: The test overrides fetch on the result of
mockProjectDO.get() but get() returns a new object each call, so the route
handler never sees the override; fix by calling mockProjectDO.get(...) once into
a variable (e.g., const projectDO = mockProjectDO.get({ toString: () =>
`do-${project.id}` })), store originalFetch from projectDO.fetch, override
projectDO.fetch with your async test implementation, then stub mockProjectDO.get
to return that same projectDO (e.g., mockProjectDO.get = jest.fn(() =>
projectDO)) so subsequent calls in the route use the overridden fetch, and
restore originalFetch and mockProjectDO.get after the test.
🧹 Nitpick comments (17)
.claude/plugins/codebase-analysis/skills/local-first-patterns/SKILL.md (1)
55-63: Consider formatting sync triggers as a markdown list for consistency.The sync trigger patterns are currently in a code block with comment syntax. For consistency with other list sections in the document, you might consider formatting this as a standard markdown list:
**Sync trigger patterns:** - On reconnect - Periodic background sync - On user action - Real-time via WebSocket - Push notification triggeredThis is purely stylistic and doesn't affect functionality.
.claude/plugins/codebase-analysis/skills/security-patterns/SKILL.md (1)
29-43: LGTM! Consider enhancing the path traversal safe pattern.The code examples effectively demonstrate dangerous vs safe patterns for common vulnerabilities. The SQL injection, XSS, and command injection examples are accurate and clear.
For the path traversal mitigation (line 42), while
path.basename()prevents directory traversal attacks, a more complete safe pattern would also verify the resolved path remains within the base directory:📁 Enhanced path traversal mitigation
-readFile(path.join(basePath, path.basename(userInput))) +// Verify resolved path stays within basePath +const safePath = path.join(basePath, path.basename(userInput)); +if (!path.resolve(safePath).startsWith(path.resolve(basePath))) { + throw new Error('Invalid path'); +} +readFile(safePath)This additional check guards against edge cases where basename alone might not be sufficient.
packages/docs/audits/test-factory-pattern-plan.md (1)
1085-1085: Consider splitting the long export statement.The export statement spans many items and may impact readability. Consider splitting it across multiple lines for the actual implementation.
♻️ Suggested formatting for implementation
-import { buildProject, buildProjectWithOwner, buildProjectMember, buildProjectWithMembers } from '@/__tests__/factories'; +import { + buildProject, + buildProjectWithOwner, + buildProjectMember, + buildProjectWithMembers, +} from '@/__tests__/factories';packages/docs/audits/command-pattern-implementation-plan.md (3)
87-92: Consider explicitly mentioning atomic transaction requirement.Based on learnings, project creation should occur in a single atomic batch operation (project + owner membership). While
insertWithQuotaCheckis mentioned, consider explicitly documenting that the command must maintain atomicity.Suggested clarification
Add a note about atomicity:
**Responsibilities**: - Validate quota (via insertWithQuotaCheck) -- Create project record -- Create owner membership +- Create project record and owner membership in atomic batch operation - Sync to Durable Object
244-252: Document Durable Object internal endpoints and Y.Doc structure.The doSync utilities should clarify that they interact with ProjectDoc's internal sync endpoints (
/sync,/sync-member,/disconnect-all). Additionally, consider documenting the Y.Doc structure being updated: meta (Map), members (Map), and reviews (Map with checklists, pdfs, reconciliation).Suggested additions
**File**: `packages/workers/src/commands/lib/doSync.js` Consolidate DO sync logic used across commands: +/** + * Syncs to ProjectDoc Durable Object internal endpoints + * Y.Doc structure: meta (Map), members (Map), reviews (Map) + */ export async function syncProjectMeta(env, projectId, meta) { ... } export async function syncProjectMember(env, projectId, action, member) { ... } -export async function disconnectAllFromProject(env, projectId) { ... } +export async function disconnectAllFromProject(env, projectId) { + // Closes WebSocket connections with code 1008 for access denial + ... +}Based on learnings, ProjectDoc implements internal sync endpoints and maintains specific Y.Doc structure.
284-320: Add ProjectDoc Durable Object cleanup to test examples.The test examples should demonstrate clearing ProjectDoc Durable Objects between tests to prevent invalidation/reset edge cases. Consider adding
clearProjectDOs()helper usage.Suggested test setup pattern
describe('createProject', () => { + afterEach(async () => { + await clearProjectDOs(); + }); + test('creates project with owner membership', async () => { const env = await setupTestEnv(); const user = await buildUser(env.DB);Based on learnings, clearing ProjectDoc DOs between tests prevents invalidation/reset edge cases.
packages/workers/src/__tests__/factories/user.js (1)
67-75: Override spread order causesbanReasonandbanExpiresdefaults to be ignored when explicitly passed.When
overridesincludesbanReasonorbanExpires, the current spread order sets them first (lines 71-72), then spreads...overrideswhich overwrites them again with the same values. This works but is redundant. More critically, the fallback defaults ('Test ban'andts + 86400) are only used when these properties are falsy inoverrides, but then...overridesspreads the original (potentiallyundefined) values back.Consider simplifying by letting
buildUserhandle the merge:♻️ Suggested simplification
export async function buildBannedUser(overrides = {}) { const ts = nowSec(); return buildUser({ banned: 1, - banReason: overrides.banReason || 'Test ban', - banExpires: overrides.banExpires || ts + 86400, // 1 day from now + banReason: 'Test ban', + banExpires: ts + 86400, // 1 day from now ...overrides, }); }This way, explicit overrides naturally take precedence via the spread, and there's no redundant property access.
packages/workers/src/__tests__/factories/org.js (1)
108-124: Consider documenting thatmembersarray includes the owner.The returned
membersarray includes the owner as the first element (line 113), which might be unexpected since the JSDoc says "additional members (besides owner)". The owner is also returned separately, so consumers need to be aware of this to avoid double-counting.📝 Suggested documentation update
/** * Build an organization with multiple members * * `@param` {Object} [options] * `@param` {number} [options.memberCount=2] - Number of additional members (besides owner) * `@param` {Object} [options.org] - Org field overrides - * `@returns` {Promise<{org: Object, owner: Object, members: Array}>} + * `@returns` {Promise<{org: Object, owner: Object, members: Array}>} members array includes owner as first element */packages/workers/src/routes/__tests__/google-drive.test.js (1)
111-124: Default headers infetchGoogleDrivemay cause confusion.The helper function has hardcoded defaults (
user-1,user1@example.com) that won't match any factory-created user. While individual tests override these headers correctly, the defaults could lead to subtle bugs if a test forgets to pass headers.Consider removing the defaults or documenting that headers must always be provided:
♻️ Option 1: Remove misleading defaults
async function fetchGoogleDrive(path, init = {}) { const ctx = createExecutionContext(); const req = new Request(`http://localhost${path}`, { ...init, headers: { - 'x-test-user-id': 'user-1', - 'x-test-user-email': 'user1@example.com', ...init.headers, }, }); const res = await app.fetch(req, env, ctx); await waitOnExecutionContext(ctx); return res; }This ensures tests explicitly provide user context, making the test setup clearer and avoiding accidental mismatches with factory-generated users.
packages/workers/src/routes/__tests__/avatars.test.js (1)
149-162: Same concern: default headers infetchAvatarmay be misleading.As with
fetchGoogleDrive, the hardcoded defaults (user-1,user1@example.com) don't correspond to any factory-created user. Consider removing these defaults for consistency with the factory-based approach.♻️ Remove misleading defaults
async function fetchAvatar(path, init = {}) { const ctx = createExecutionContext(); const req = new Request(`http://localhost${path}`, { ...init, headers: { - 'x-test-user-id': 'user-1', - 'x-test-user-email': 'user1@example.com', ...init.headers, }, }); const res = await app.fetch(req, env, ctx); await waitOnExecutionContext(ctx); return res; }packages/workers/src/routes/__tests__/users.test.js (1)
54-59: Same stale cleanup issue with hardcoded project IDs.Similar to
org-auth.test.js, thisclearProjectDOs(['project-1'])uses a hardcoded ID that won't match factory-generated project IDs.packages/workers/src/routes/__tests__/projects.test.js (2)
62-67: Same staleclearProjectDOspattern.Hardcoded project IDs in cleanup won't match factory-generated IDs. This is a repeated pattern across multiple test files that should be addressed.
273-284: Consider more explicit member access pattern.Accessing
members[1].userrelies on implicit array ordering (owner at index 0, members at index 1+). Consider using a more explicit pattern for clarity:const regularMember = members.find(m => m.role === 'member')?.user;This makes the test intent clearer and is less fragile if the factory's return order changes.
packages/workers/src/__tests__/factories/project.js (1)
60-71: Minor: Duplicate field application inwithDefaults.
projectDefaultsalready includes values fromoptions.project(id, name, description at lines 61-63), thenwithDefaultsmergesoptions.projectagain. This works but appliesoptions.projectvalues twice—once explicitly and once via merge.Consider simplifying by not pre-extracting from
options.project:Suggested simplification
const projectDefaults = { - id: projectId, - name: projectName, - description: options.project?.description || `Description for ${projectName}`, + id: generateId('proj'), + name: `Test Project ${n}`, + description: `Description for Test Project ${n}`, orgId: org.id, createdBy: owner.id, createdAt: ts, updatedAt: ts, }; const projectData = withDefaults(projectDefaults, options.project);packages/workers/src/routes/__tests__/members.test.js (1)
79-84: Stale hardcoded project ID inclearProjectDOs.
clearProjectDOs(['project-1'])uses a hardcoded ID, but factory-created projects have dynamic IDs likeproj-<uuid>. This call won't clear DOs for factory-created projects.Since project IDs are now unique per test, this may not cause issues in practice, but the dead code is misleading. Consider either removing it or making it dynamic if DO cleanup is actually needed.
Option 1: Remove if not needed
beforeEach(async () => { await resetTestDatabase(); - // Clear ProjectDoc DOs to prevent invalidation errors between tests - await clearProjectDOs(['project-1']); vi.clearAllMocks(); resetCounter();Option 2: Document why it's retained
beforeEach(async () => { await resetTestDatabase(); - // Clear ProjectDoc DOs to prevent invalidation errors between tests - await clearProjectDOs(['project-1']); + // Note: clearProjectDOs not needed since factory creates unique project IDs per test vi.clearAllMocks(); resetCounter();packages/workers/src/routes/__tests__/account-merge.test.js (1)
92-107: Consider extractingseedAccountto factory module.
seedAccountuses raw SQL while other entities use factories. For consistency, consider moving this to abuildAccountfactory in the factories module, especially if account seeding is needed in other test files.packages/workers/src/routes/__tests__/project-invitations.test.js (1)
87-91: Same staleclearProjectDOsissue as members.test.js.
clearProjectDOs(['project-1'])won't affect factory-created projects with dynamic IDs.beforeEach(async () => { await resetTestDatabase(); resetCounter(); - await clearProjectDOs(['project-1']); vi.clearAllMocks();
| ```javascript | ||
| // packages/web/src/primitives/db.js | ||
| class CoratesDB extends Dexie { | ||
| constructor() { | ||
| super('corates', { addons: [yDexie] }); | ||
|
|
||
| this.version(1).stores({ | ||
| projects: 'id, orgId, updatedAt, ydoc: Y.Doc', // Y.Doc via y-dexie | ||
| pdfs: 'id, projectId, studyId, cachedAt', // PDF cache | ||
| ops: '++id, idempotencyKey, status', // Operation queue | ||
| avatars: 'userId, cachedAt', // Avatar cache | ||
| formStates: 'key, type', // Form persistence | ||
| localChecklists: 'id, type', // Offline checklists | ||
| queryCache: 'key', // TanStack Query cache | ||
| }); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix invalid Dexie schema syntax.
Line 109 contains invalid Dexie schema syntax. The stores() method accepts only indexed field names as comma-separated strings, not type annotations.
📝 Proposed fix for the schema syntax
this.version(1).stores({
- projects: 'id, orgId, updatedAt, ydoc: Y.Doc', // Y.Doc via y-dexie
+ projects: 'id, orgId, updatedAt', // Y.Doc stored separately via y-dexie addon
pdfs: 'id, projectId, studyId, cachedAt', // PDF cache
ops: '++id, idempotencyKey, status', // Operation queue
avatars: 'userId, cachedAt', // Avatar cache🤖 Prompt for AI Agents
In `@packages/docs/audits/local-first-comparison-outline-corates.md` around lines
100 - 117, The Dexie schema in CoratesDB.constructor uses invalid type
annotations and inline comments inside the stores() strings; update the
version(1).stores call so each table schema string contains only comma-separated
indexed field names (e.g., change "projects: 'id, orgId, updatedAt, ydoc:
Y.Doc'" to "projects: 'id, orgId, updatedAt, ydoc'") and move any type info or
comments outside the schema strings as regular code comments; apply the same fix
for any other entries that include annotations or comments inside the quotes
(e.g., ensure pdfs, avatars, formStates, localChecklists, queryCache strings
contain only field names).
| if (options.org) { | ||
| org = options.org; | ||
| owner = options.owner || (await buildUser()); | ||
| // Ensure owner is org member if not already | ||
| if (options.owner && !options.skipOrgMembership) { | ||
| try { | ||
| await buildOrgMember({ orgId: org.id, user: owner, role: 'owner' }); | ||
| } catch (e) { | ||
| // Ignore if already a member (UNIQUE constraint) | ||
| if (!e.message?.includes('UNIQUE constraint')) throw e; | ||
| } | ||
| } | ||
| } else { | ||
| const orgResult = await buildOrg({ owner: options.owner }); | ||
| org = orgResult.org; | ||
| owner = orgResult.owner; | ||
| } |
There was a problem hiding this comment.
Bug: New user created without org membership when options.org provided without options.owner.
When options.org is provided but options.owner is not:
- Line 40 creates a new user via
buildUser() - Line 42 condition
if (options.owner && !options.skipOrgMembership)is false sinceoptions.owneris falsy - No org membership is created for the new user
- The project is still created with this user as
createdBy(line 65)
This leaves the database in an inconsistent state where a user owns a project but isn't an org member.
Proposed fix
if (options.org) {
org = options.org;
owner = options.owner || (await buildUser());
- // Ensure owner is org member if not already
- if (options.owner && !options.skipOrgMembership) {
+ // Ensure owner is org member if not already (skip only if explicitly requested)
+ if (!options.skipOrgMembership) {
try {
await buildOrgMember({ orgId: org.id, user: owner, role: 'owner' });
} catch (e) {
// Ignore if already a member (UNIQUE constraint)
if (!e.message?.includes('UNIQUE constraint')) throw e;
}
}| const originalFetch = mockProjectDO.get({ toString: () => `do-${project.id}` }).fetch; | ||
| mockProjectDO.get({ toString: () => `do-${project.id}` }).fetch = async request => { |
There was a problem hiding this comment.
Mock override has no effect - mockProjectDO.get() returns a new object each call.
The mock override assigns to a property on the result of mockProjectDO.get(), but get() returns a new object each time it's called (line 133). The route handler will call get() again, receiving a fresh object with the original fetch, so the overridden assertions won't execute.
🐛 Proposed fix
it('should sync avatar to project memberships', async () => {
const { project, owner } = await buildProject();
- const originalFetch = mockProjectDO.get({ toString: () => `do-${project.id}` }).fetch;
- mockProjectDO.get({ toString: () => `do-${project.id}` }).fetch = async request => {
+ const originalGet = mockProjectDO.get;
+ mockProjectDO.get = _id => ({
+ fetch: async request => {
const url = new URL(request.url);
if (url.pathname === '/sync-member') {
const body = await request.json();
expect(body.action).toBe('update');
expect(body.member.userId).toBe(owner.id);
expect(body.member.image).toMatch(new RegExp(`^/api/users/avatar/${owner.id}\\?t=\\d+$`));
+ return new Response(JSON.stringify({ ok: true }), { status: 200 });
}
- return originalFetch(request);
- };
+ return new Response(JSON.stringify({ ok: false }), { status: 404 });
+ },
+ });
const imageData = new Uint8Array([0xff, 0xd8, 0xff, 0xe0]);
// ... rest of test
+
+ // Restore original mock
+ mockProjectDO.get = originalGet;
});🤖 Prompt for AI Agents
In `@packages/workers/src/routes/__tests__/avatars.test.js` around lines 267 -
268, The test overrides fetch on the result of mockProjectDO.get() but get()
returns a new object each call, so the route handler never sees the override;
fix by calling mockProjectDO.get(...) once into a variable (e.g., const
projectDO = mockProjectDO.get({ toString: () => `do-${project.id}` })), store
originalFetch from projectDO.fetch, override projectDO.fetch with your async
test implementation, then stub mockProjectDO.get to return that same projectDO
(e.g., mockProjectDO.get = jest.fn(() => projectDO)) so subsequent calls in the
route use the overridden fetch, and restore originalFetch and mockProjectDO.get
after the test.
2 participants
Summary by CodeRabbit
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.