backend migration to organizations completed#200
Conversation
|
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 📝 WalkthroughWalkthroughAdds multi-tenant organization support: database tables, org-scoped API routes and middleware, frontend org context/hooks/pages/navigation, org-aware PDF/Google Drive APIs and stores, durable-object and DO-auth changes, and extensive test/migration/documentation updates enabling /api/orgs/:orgId/... routing. Changes
Sequence Diagram(s)%% Org redirect and project listing (frontend + auth + org API + DB)
sequenceDiagram
actor User
participant Client as Web Client
participant OrgRedirect as OrgRedirect
participant Auth as Auth Service
participant OrgAPI as Org API
participant DB as Database
User->>Client: Open dashboard
Client->>OrgRedirect: render
OrgRedirect->>Auth: isLoggedIn()
Auth-->>OrgRedirect: authenticated
OrgRedirect->>OrgAPI: GET /api/orgs (user's orgs)
OrgAPI->>DB: SELECT member/orgs WHERE userId
DB-->>OrgAPI: org list
OrgAPI-->>OrgRedirect: orgs
alt has lastOrgSlug
OrgRedirect->>Client: navigate to /orgs/{lastOrg}
else
OrgRedirect->>Client: navigate to /orgs/{firstOrg}
end
Client->>OrgAPI: GET /api/orgs/{orgId}/projects
OrgAPI->>DB: SELECT projects WHERE orgId
DB-->>OrgAPI: projects list
OrgAPI-->>Client: projects
%% Org-scoped request middleware flow (API request -> middleware -> DB -> handler)
sequenceDiagram
actor Client
participant API as Org API
participant Middleware as requireOrgMembership/requireProjectAccess
participant DB as Database
participant Handler as Route Handler
Client->>API: GET /api/orgs/{orgId}/projects/{projectId}
API->>Middleware: run auth middleware
Middleware->>DB: resolve session/user
DB-->>Middleware: user
rect rgb(220,235,255)
Note over Middleware,DB: requireOrgMembership
Middleware->>DB: SELECT member WHERE userId & orgId
DB-->>Middleware: member row or null
end
rect rgb(220,235,255)
Note over Middleware,DB: requireProjectAccess
Middleware->>DB: SELECT project WHERE id=projectId
DB-->>Middleware: project (with orgId)
Middleware->>DB: SELECT projectMembers WHERE projectId & userId
DB-->>Middleware: project member
end
alt allowed
Middleware->>Handler: continue
Handler->>DB: read project details
DB-->>Handler: project data
Handler-->>Client: 200 + data
else denied
Middleware-->>Client: 403 ORGANIZATION/PROJECT error
end
%% Invitation accept flow (magic link -> org + project membership creation)
sequenceDiagram
actor Invitee
participant Client as Web Client
participant OrgAPI as Org API
participant DB as Database
participant AuthEmail as Better Auth Email
Invitee->>Client: Click magic link -> POST /accept?token
Client->>OrgAPI: POST /api/orgs/{orgId}/projects/{projectId}/invitations/accept
OrgAPI->>DB: SELECT invitation WHERE token
DB-->>OrgAPI: invitation (valid)
rect rgb(200,255,200)
Note over OrgAPI,DB: Atomic accept flow
OrgAPI->>DB: ENSURE org membership (insert if missing)
DB-->>OrgAPI: org membership ensured
OrgAPI->>DB: INSERT project member
DB-->>OrgAPI: project member inserted
OrgAPI->>DB: UPDATE invitation acceptedAt
DB-->>OrgAPI: batch committed
end
OrgAPI->>AuthEmail: notify user / sync DO (best-effort)
AuthEmail-->>OrgAPI: queued
OrgAPI-->>Client: 200 + { orgId, orgSlug, projectId, role }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 | Updated (UTC) |
|---|---|---|---|
| 🔵 In progress View logs |
corates | 6629488 | Dec 30 2025, 05:22 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 63b57e8 | Commit Preview URL | Dec 30 2025, 07:03 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (23)
packages/web/src/components/Navbar.jsx (1)
19-22: Consider extracting dropdown logic into a reusable primitive.The dropdown pattern (show signal, ref, click-outside handler) is now duplicated for both user and org menus. If more dropdowns are anticipated, consider extracting a
useDropdownprimitive inpackages/web/src/primitives/that encapsulates:
- Show/hide signal
- Ref management
- Click-outside handling
This is a low-priority refactor that can be deferred. Based on learnings, reusable logic should be created in primitives.
packages/web/src/components/project/CreateProjectForm.jsx (1)
99-99: Terminology consistency: "organization" vs "workspace".The error message uses "workspace" while the code uses "organization"/"org" throughout (prop name, API path). Verify this terminology is consistent with other user-facing messages in the app. If "workspace" is the preferred user-facing term, consider documenting this convention.
packages/workers/migrations/meta/0001_snapshot.json (1)
368-373: Nullable slug field with unique constraint.The
organization.slugfield is nullable but has a unique index. This allows multiple organizations withnullslugs, which may be unexpected if slugs are intended as unique identifiers. Consider makingslugrequired (notNull: true) if every organization should have a unique slug.packages/workers/src/routes/invitations.js (1)
210-219: Consider handling missing organization gracefully.If
invitation.orgIdis set but the organization doesn't exist (e.g., deleted between invitation creation and acceptance),orgwill beundefinedandorgSlugwill benull. This is handled safely, but consider whether a warning log would help debugging orphaned invitations.packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx (1)
375-382: Consider extracting path builder to a shared utility.This
getProjectPathpattern appears to be duplicated across components. Per coding guidelines, consider moving this to a shared primitive (e.g.,useOrgProjectContext.jsmentioned in the summary) to ensure consistency and reduce duplication.packages/workers/migrations/0001_nice_blonde_phantom.sql (1)
14-22: Consider adding unique constraint on member table.As noted in the schema review, the
membertable allows duplicate (userId, organizationId) combinations. Consider adding:CREATE UNIQUE INDEX `member_user_org_unique` ON `member` (`userId`, `organizationId`);packages/workers/src/db/schema.js (1)
36-47: Add a unique constraint on (userId, organizationId) to the member table for data integrity.While the code currently checks for existing membership before inserting (in
ensureOrgMembershipand similar places), the database lacks a unique constraint to enforce this at the schema level. This leaves room for duplicates if application logic is ever bypassed. Adding a unique index in the migration would provide a safety net:CREATE UNIQUE INDEX member_user_org_unique ON member(userId, organizationId);packages/workers/src/middleware/requireOrg.js (1)
109-109: Import projectMembers at the top level for consistency.The
projectMembersschema is dynamically imported here, which is inconsistent with how other schema entities (member,organization,projects) are imported at the top of the file. Dynamic imports add unnecessary complexity and can make the code harder to follow.Proposed refactor
At the top of the file, update line 7:
-import { member, organization, projects } from '../db/schema.js'; +import { member, organization, projects, projectMembers } from '../db/schema.js';Then remove the dynamic import at line 109:
const db = createDb(c.env.DB); - const { projectMembers } = await import('../db/schema.js');packages/web/src/stores/projectActionsStore/members.js (1)
28-50: Consider usinghandleFetchErrorfor consistent error handling.Per coding guidelines, frontend fetch calls should use
handleFetchErrorfrom@/lib/error-utils.js. The current implementation manually parses the error response, which works but diverges from the established pattern used elsewhere.Proposed refactor using handleFetchError
+import { handleFetchError } from '@/lib/error-utils.js'; // ... inside remove function: try { const response = await fetch( `${API_BASE}/api/orgs/${orgId}/projects/${projectId}/members/${memberId}`, { method: 'DELETE', credentials: 'include', }, ); if (!response.ok) { - const data = await response.json(); - throw new Error(data.error || 'Failed to remove member'); + await handleFetchError(response, { showToast: false }); }packages/web/src/primitives/useOrgProjectList.js (1)
21-24: Content-Type header is unnecessary for GET requests.GET requests don't have a body, so the
Content-Type: application/jsonheader is superfluous. While it doesn't cause issues, removing it keeps the code clean.Proposed fix
const response = await fetch(`${API_BASE}/api/orgs/${orgId}/projects`, { credentials: 'include', - headers: { 'Content-Type': 'application/json' }, });packages/web/src/components/org/CreateOrgPage.jsx (1)
29-36: Slug sanitization differs between auto-generation and manual input.The auto-generated slug (Lines 30-35) removes leading/trailing hyphens with
.replace(/^-|-$/g, ''), but the manual input handler (Line 140) only applies.toLowerCase().replace(/[^a-z0-9-]/g, ''). A user could manually enter--my-org--which passes validation but may look odd in URLs.Consider applying the same sanitization to manual input:
Proposed fix
<input id='org-slug' type='text' value={slug()} - onInput={e => setSlug(e.target.value.toLowerCase().replace(/[^a-z0-9-]/g, ''))} + onInput={e => { + const sanitized = e.target.value + .toLowerCase() + .replace(/[^a-z0-9-]/g, '') + .replace(/-+/g, '-') + .replace(/^-|-$/g, ''); + setSlug(sanitized); + }} placeholder='my-research-lab'Also applies to: 140-140
packages/web/src/stores/projectActionsStore/project.js (1)
58-76: Org-scoped deletion with proper cache invalidation.The
deleteByIdfunction correctly:
- Falls back to
getActiveOrgId()when no explicit org is provided- Uses the org-scoped API endpoint
/api/orgs/${orgId}/projects/${projectId}- Invalidates both
projects.allandprojects.byOrg(orgId)to ensure UI consistencyHowever, if
orgIdisnull/undefined, the API call will fail with an invalid URL. Consider adding a guard:Proposed fix
async function deleteById(targetProjectId, targetOrgId) { const orgId = targetOrgId || getActiveOrgId(); + if (!orgId) { + throw new Error('Organization ID is required to delete a project'); + } try { const response = await fetch(`${API_BASE}/api/orgs/${orgId}/projects/${targetProjectId}`, {packages/web/src/components/org/OrgProjectsPage.jsx (2)
93-119: Delete logic duplicates projectActionsStore.deleteById.The delete handler (Lines 103-118) duplicates the fetch logic that exists in
projectActionsStore/project.js. Consider reusing the store action to maintain a single source of truth:Proposed fix
+import { projectActionsStore } from '@/stores/projectActionsStore/index.js'; + // Delete a project const handleDeleteProject = async targetProjectId => { const confirmed = await confirmDialog.open({ title: 'Delete Project', description: 'Are you sure you want to delete this entire project? This action cannot be undone.', confirmText: 'Delete Project', variant: 'danger', }); if (!confirmed) return; try { - const response = await fetch(`${API_BASE}/api/orgs/${orgId()}/projects/${targetProjectId}`, { - method: 'DELETE', - credentials: 'include', - }); - if (!response.ok) { - const data = await response.json().catch(() => ({})); - throw new Error(data.error || 'Failed to delete project'); - } - - queryClient.invalidateQueries({ queryKey: queryKeys.projects.byOrg(orgId()) }); + await projectActionsStore.project.deleteById(targetProjectId, orgId()); showToast.success('Project Deleted', 'The project has been deleted successfully'); } catch (err) { const { handleError } = await import('@/lib/error-utils.js'); await handleError(err, { toastTitle: 'Delete Failed' }); } };
116-117: Dynamic import in catch block adds latency to error display.The dynamic
import('@/lib/error-utils.js')in the catch block delays error handling. IfhandleErroris lightweight, consider importing it statically at the top of the file.Proposed fix
+import { handleError } from '@/lib/error-utils.js'; ... } catch (err) { - const { handleError } = await import('@/lib/error-utils.js'); await handleError(err, { toastTitle: 'Delete Failed' }); }packages/workers/src/auth/config.js (1)
346-361: Consider using db.batch() for atomic org bootstrap.The org and member inserts (Lines 347-361) are related operations that should succeed or fail together. Per coding guidelines, use
db.batch()for multiple related database operations to ensure atomicity:Proposed fix
- // Insert org and membership - await db.insert(schema.organization).values({ - id: orgId, - name: `${userName}'s Workspace`, - slug, - metadata: JSON.stringify({ type: 'personal' }), - createdAt: now, - }); - - await db.insert(schema.member).values({ - id: memberId, - userId, - organizationId: orgId, - role: 'owner', - createdAt: now, - }); + // Insert org and membership atomically + await db.batch([ + db.insert(schema.organization).values({ + id: orgId, + name: `${userName}'s Workspace`, + slug, + metadata: JSON.stringify({ type: 'personal' }), + createdAt: now, + }), + db.insert(schema.member).values({ + id: memberId, + userId, + organizationId: orgId, + role: 'owner', + createdAt: now, + }), + ]);Based on learnings, database operations should use
db.batch()for atomicity.packages/workers/src/routes/orgs/members.js (1)
567-569: Consider removing sensitive URL logging in non-production.While guarded by environment check, logging magic link URLs even in development could be a security concern if logs are inadvertently exposed. Consider using a more secure mechanism for development testing.
packages/workers/src/routes/orgs/projects.js (1)
325-331: Consider parallel deletion with concurrency limit.While
Promise.allworks, for projects with many PDFs, a large batch could create significant concurrent requests. Consider using a concurrency limiter for very large projects.Optional: Add concurrency control for large deletions
For very large projects, consider batching deletions:
// Example with batch size limit const BATCH_SIZE = 50; for (let i = 0; i < keysToDelete.length; i += BATCH_SIZE) { const batch = keysToDelete.slice(i, i + BATCH_SIZE); await Promise.all(batch.map(key => c.env.PDF_BUCKET.delete(key))); }packages/workers/src/routes/orgs/index.js (1)
47-56: Consider using validateRequest middleware for consistency.The POST
/route performs inline validation for thenamefield. For consistency with other routes and per coding guidelines, consider using thevalidateRequestmiddleware with a schema.Use validateRequest for org creation
+import { orgSchemas } from '../../config/validation.js'; + orgRoutes.post( '/', + validateRequest(orgSchemas.create), async c => { - try { - const body = await c.req.json(); - - if (!body.name?.trim()) { - const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { - reason: 'name_required', - }); - return c.json(error, error.statusCode); - } + const { name, slug, logo, metadata } = c.get('validatedBody');packages/workers/src/routes/orgs/pdfs.js (1)
46-53: Consider adding check for path traversal sequences.The validation blocks slashes and control characters but doesn't explicitly check for
..sequences. While the key construction prevents traversal since it's prefixed, adding this check provides defense in depth.Proposed enhancement
function isValidFileName(fileName) { if (!fileName) return false; if (fileName.length > 200) return false; if (/[\\/]/.test(fileName)) return false; if (/\p{C}/u.test(fileName)) return false; if (fileName.includes('"')) return false; + if (fileName.includes('..')) return false; return true; }packages/web/src/primitives/useOrgProjectContext.js (1)
96-112: Consider potential reactivity concern with path builder closures.The path builder functions (
getStudyPath,getChecklistPath,getReconcilePath) capture the current values oforgContext.orgSlug()andprojectId()when called, which is the intended behavior for imperative navigation. However, if these are used in reactive contexts (e.g., passed tocreateMemo), the outer function reference won't trigger updates.This is likely fine given the navigation use case, but worth documenting if consumers might use them reactively.
packages/workers/src/routes/orgs/invitations.js (3)
176-210: Consider extracting magic link generation to avoid per-request auth instantiation.Creating a new
betterAuthinstance with full adapter setup for each invitation request is heavyweight. Consider extracting this to a shared utility or caching the auth instance.
310-316: Consider using a more semantically appropriate error for not-found invitation.
VALIDATION_ERRORS.FIELD_INVALID_FORMATsuggests a format issue, but the actual problem is that the invitation doesn't exist. A 404-style error would be clearer.Proposed fix
Consider adding an
INVITATION_NOT_FOUNDerror toPROJECT_ERRORSor reusing an existing not-found pattern:if (!invitation) { - const error = createDomainError(VALIDATION_ERRORS.FIELD_INVALID_FORMAT, { - field: 'invitationId', - value: invitationId, + const error = createDomainError(PROJECT_ERRORS.NOT_FOUND, { + resource: 'invitation', + id: invitationId, }); return c.json(error, error.statusCode); }
367-384: Consider using more specific error types for invalid/expired tokens.Using
VALIDATION_ERRORS.FIELD_INVALID_FORMATfor both "not found" (line 368) and "expired" (line 379) scenarios conflates different error conditions. Distinct error types would improve debugging and client-side error handling.
| createEffect(() => { | ||
| const pid = params.projectId; | ||
| const oid = orgId(); | ||
| if (pid && oid) { | ||
| projectActionsStore._setActiveProject(pid, oid); | ||
| connect(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the useProject hook's connect/disconnect logic
rg -n "connect|disconnect" packages/web/src/primitives/useProject/ -A 3 -B 1 | head -100Repository: InfinityBowman/corates
Length of output: 8497
🏁 Script executed:
# Check the full useProject implementation structure
ls -la packages/web/src/primitives/useProject/Repository: InfinityBowman/corates
Length of output: 611
🏁 Script executed:
# Examine the specific effect in ChecklistYjsWrapper.jsx with context
cat -n packages/web/src/components/checklist/ChecklistYjsWrapper.jsx | sed -n '40,65p'Repository: InfinityBowman/corates
Length of output: 1074
🏁 Script executed:
# Get the full connect() function implementation
sed -n '63,145p' packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 2707
🏁 Script executed:
# Check how useProject manages the connect function and connection lifecycle
sed -n '1,100p' packages/web/src/primitives/useProject/index.js | grep -A 30 "function useProject"Repository: InfinityBowman/corates
Length of output: 48
🏁 Script executed:
# Look for disconnect() implementation
rg -n "function disconnect|const disconnect" packages/web/src/primitives/useProject/connection.js -A 10Repository: InfinityBowman/corates
Length of output: 351
🏁 Script executed:
# Check how connect() is exported and managed from useProject
sed -n '1,50p' packages/web/src/primitives/useProject/index.jsRepository: InfinityBowman/corates
Length of output: 1845
🏁 Script executed:
# Look for the return statement and how connect is exposed in useProject
rg -n "return {" packages/web/src/primitives/useProject/index.js -A 30 | head -60Repository: InfinityBowman/corates
Length of output: 145
🏁 Script executed:
# Check if there's reference counting or cleanup in useProject
rg -n "refCount|cleanup|onCleanup" packages/web/src/primitives/useProject/index.js -B 2 -A 2Repository: InfinityBowman/corates
Length of output: 1636
🏁 Script executed:
# Check if connect() checks for existing provider before creating a new one
sed -n '63,80p' packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 617
🏁 Script executed:
# Look at how the connection is used in ChecklistYjsWrapper - check if there are multiple calls
rg -n "connect\(\)" packages/web/src/components/checklist/ChecklistYjsWrapper.jsxRepository: InfinityBowman/corates
Length of output: 85
🏁 Script executed:
# Check the broader context of useProject hook's onCleanup
sed -n '300,320p' packages/web/src/primitives/useProject/index.jsRepository: InfinityBowman/corates
Length of output: 328
🏁 Script executed:
# Check what connect() returns and if it's safe to call multiple times
sed -n '63,150p' packages/web/src/primitives/useProject/connection.js | grep -A 5 "provider ="Repository: InfinityBowman/corates
Length of output: 336
🏁 Script executed:
# Verify if connect() checks for existing provider before creating new one
sed -n '63,75p' packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 390
🏁 Script executed:
# Check the full disconnect function and what it does
sed -n '208,220p' packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 331
🏁 Script executed:
# Check if there's a guard in connect() that prevents creating provider if one exists
sed -n '63,105p' packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 1279
🏁 Script executed:
# Verify the entire ChecklistYjsWrapper effect structure and what happens on remount
sed -n '45,70p' packages/web/src/components/checklist/ChecklistYjsWrapper.jsxRepository: InfinityBowman/corates
Length of output: 967
🏁 Script executed:
# Check if connect is called elsewhere in ChecklistYjsWrapper
rg -n "connect" packages/web/src/components/checklist/ChecklistYjsWrapper.jsxRepository: InfinityBowman/corates
Length of output: 299
🏁 Script executed:
# Examine the full logic - check if provider is checked before creating new one
sed -n '63,78p' packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 608
🏁 Script executed:
# Verify the reference counting behavior in useProject - check how connections are managed
sed -n '40,65p' packages/web/src/primitives/useProject/index.jsRepository: InfinityBowman/corates
Length of output: 594
Add onCleanup() to prevent duplicate connections when org context changes.
The effect at lines 46-53 calls connect() but lacks onCleanup(). When orgId() changes, the effect re-runs and creates a new connection without destroying the previous one. Per the coding guidelines, effects creating subscriptions must use onCleanup() to call disconnect() on unmount or dependency changes.
createEffect(() => {
const pid = params.projectId;
const oid = orgId();
if (pid && oid) {
projectActionsStore._setActiveProject(pid, oid);
connect();
onCleanup(() => {
disconnect();
});
}
});🤖 Prompt for AI Agents
In packages/web/src/components/checklist/ChecklistYjsWrapper.jsx around lines
46-53, the createEffect calls connect() when pid and orgId are present but does
not register an onCleanup, causing duplicate connections when org context
changes; fix by adding onCleanup() inside the same conditional to call
disconnect() (or otherwise tear down the created connection) so every connect()
is paired with a disconnect() on re-run/unmount, ensuring cleanup only runs if
connect() was invoked.
| // Note: Project list (dashboard) functionality is now handled by TanStack Query | ||
| // via useProjectList and useOrgProjectList hooks. Those hooks are tested separately. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the projectStore.test.js file
fd projectStore.test.jsRepository: InfinityBowman/corates
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Check the specific lines mentioned (360-361)
if [ -f "packages/web/src/primitives/__tests__/projectStore.test.js" ]; then
echo "=== Lines 360-361 context ==="
sed -n '355,365p' packages/web/src/primitives/__tests__/projectStore.test.js
echo ""
echo "=== Total lines in file ==="
wc -l packages/web/src/primitives/__tests__/projectStore.test.js
fiRepository: InfinityBowman/corates
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Search for useProjectList and useOrgProjectList tests
echo "=== Searching for useProjectList tests ==="
fd -e test.js -e test.jsx -e test.ts -e test.tsx . --exec grep -l 'useProjectList' {} \;
echo ""
echo "=== Searching for useOrgProjectList tests ==="
fd -e test.js -e test.jsx -e test.ts -e test.tsx . --exec grep -l 'useOrgProjectList' {} \;
echo ""
echo "=== Searching for hook implementations ==="
fd 'useProjectList|useOrgProjectList' --extension js --extension jsx --extension ts --extension tsxRepository: InfinityBowman/corates
Length of output: 416
🏁 Script executed:
#!/bin/bash
# Check if these hooks are exported and where they're defined
echo "=== Searching for useProjectList hook definition ==="
rg 'export.*useProjectList|function useProjectList|const useProjectList' --type js --type jsx --type ts --type tsx
echo ""
echo "=== Searching for useOrgProjectList hook definition ==="
rg 'export.*useOrgProjectList|function useOrgProjectList|const useOrgProjectList' --type js --type jsx --type ts --type tsxRepository: InfinityBowman/corates
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Check if hooks have dedicated test files
echo "=== Searching for dedicated hook test files ==="
fd 'useProjectList.*test|useOrgProjectList.*test' --extension js --extension jsx
echo ""
echo "=== Check if hooks are tested in other files ==="
rg 'useProjectList|useOrgProjectList' packages/web/src/primitives/__tests__/ -l
echo ""
echo "=== Examine useProjectList hook ==="
head -20 packages/web/src/primitives/useProjectList.js
echo ""
echo "=== Examine useOrgProjectList hook ==="
head -20 packages/web/src/primitives/useOrgProjectList.jsRepository: InfinityBowman/corates
Length of output: 1508
🏁 Script executed:
#!/bin/bash
# Search for test files that might test these hooks
echo "=== All test files in primitives ==="
fd -e test.js packages/web/src/primitives/__tests__/
echo ""
echo "=== Check if hooks are tested elsewhere in the codebase ==="
rg 'describe.*useProjectList|test.*useProjectList|describe.*useOrgProjectList|test.*useOrgProjectList' --type jsRepository: InfinityBowman/corates
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Corrected search for test files in primitives
echo "=== All test files in primitives __tests__ directory ==="
fd . 'packages/web/src/primitives/__tests__/' -type f
echo ""
echo "=== Search for any mention of useProjectList in test files ==="
rg 'useProjectList' packages/web/src/primitives/__tests__/ -A 2 -B 2
echo ""
echo "=== Search for any mention of useOrgProjectList in test files ==="
rg 'useOrgProjectList' packages/web/src/primitives/__tests__/ -A 2 -B 2Repository: InfinityBowman/corates
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Check if hooks are tested in hooks-specific test directory
echo "=== Looking for hooks test directory ==="
fd --full-path '.*hooks.*test' packages/web/src/primitives/
echo ""
echo "=== Check for useProject tests (parent hook) ==="
find packages/web/src -type f -name '*test*' | grep -i 'useproject' | head -20
echo ""
echo "=== Check directory structure of primitives ==="
ls -la packages/web/src/primitives/ | head -30Repository: InfinityBowman/corates
Length of output: 1470
🏁 Script executed:
#!/bin/bash
# Search entire web package for any tests of these hooks
echo "=== Search entire web package for useProjectList tests ==="
rg 'describe.*useProjectList|test.*useProjectList' packages/web/src/ -type js
echo ""
echo "=== Search entire web package for useOrgProjectList tests ==="
rg 'describe.*useOrgProjectList|test.*useOrgProjectList' packages/web/src/ -type js
echo ""
echo "=== Count test files in web package ==="
find packages/web/src -name '*test.js' -o -name '*test.jsx' | wc -lRepository: InfinityBowman/corates
Length of output: 304
Tests for useProjectList and useOrgProjectList hooks do not exist. The comment states these hooks are "tested separately," but no test files exist for either hook. Dedicated tests are required to ensure the migrated project list functionality is properly covered.
| ALTER TABLE `project_invitations` ADD `orgId` text NOT NULL REFERENCES organization(id);--> statement-breakpoint | ||
| ALTER TABLE `project_invitations` ADD `orgRole` text DEFAULT 'member';--> statement-breakpoint | ||
| ALTER TABLE `projects` ADD `orgId` text NOT NULL REFERENCES organization(id);--> statement-breakpoint |
There was a problem hiding this comment.
Migration will fail on databases with existing data.
SQLite's ALTER TABLE ADD COLUMN with NOT NULL and no DEFAULT will fail if the table contains any rows. Lines 34 and 36 add orgId as NOT NULL to project_invitations and projects respectively.
Options to fix:
- Add
DEFAULT ''(or a placeholder) temporarily, then update and remove default - Use a multi-step migration: add nullable column, backfill data, then use a new migration to add constraint
- Document that this migration only works on fresh databases
Example multi-step approach
-- Step 1: Add nullable column
ALTER TABLE `projects` ADD `orgId` text REFERENCES organization(id);
-- Step 2: Backfill existing projects (requires creating default org first)
-- UPDATE projects SET orgId = 'default-org-id' WHERE orgId IS NULL;
-- Step 3: In a subsequent migration, add NOT NULL constraint
-- (SQLite doesn't support ALTER COLUMN, so this requires table recreation)🤖 Prompt for AI Agents
packages/workers/migrations/0001_nice_blonde_phantom.sql around lines 34-36: the
migration adds orgId as NOT NULL to project_invitations and projects which will
fail on databases with existing rows; change to a safe multi-step migration: 1)
add orgId as a nullable column with the same REFERENCES clause (no NOT NULL), 2)
backfill existing rows with a valid org id or a temporary placeholder (perform
UPDATEs after creating/choosing the default org), and 3) in a following
migration enforce NOT NULL (and recreate the table in SQLite if necessary to add
the constraint) — alternatively, if you need a single-step change, add a
temporary DEFAULT value when adding the column, backfill and then remove the
default in a later migration; implement one of these approaches so the migration
won’t error on non-empty databases.
| "invitation": { | ||
| "name": "invitation", | ||
| "columns": { | ||
| "id": { | ||
| "name": "id", | ||
| "type": "text", | ||
| "primaryKey": true, | ||
| "notNull": true, | ||
| "autoincrement": false | ||
| }, | ||
| "email": { | ||
| "name": "email", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false | ||
| }, | ||
| "inviterId": { | ||
| "name": "inviterId", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false | ||
| }, | ||
| "organizationId": { | ||
| "name": "organizationId", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false | ||
| }, | ||
| "role": { | ||
| "name": "role", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false, | ||
| "default": "'member'" | ||
| }, | ||
| "status": { | ||
| "name": "status", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false, | ||
| "default": "'pending'" | ||
| }, | ||
| "expiresAt": { | ||
| "name": "expiresAt", | ||
| "type": "integer", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false | ||
| }, | ||
| "createdAt": { | ||
| "name": "createdAt", | ||
| "type": "integer", | ||
| "primaryKey": false, | ||
| "notNull": false, | ||
| "autoincrement": false, | ||
| "default": "(unixepoch())" | ||
| } | ||
| }, | ||
| "indexes": {}, | ||
| "foreignKeys": { | ||
| "invitation_inviterId_user_id_fk": { | ||
| "name": "invitation_inviterId_user_id_fk", | ||
| "tableFrom": "invitation", | ||
| "tableTo": "user", | ||
| "columnsFrom": ["inviterId"], | ||
| "columnsTo": ["id"], | ||
| "onDelete": "cascade", | ||
| "onUpdate": "no action" | ||
| }, | ||
| "invitation_organizationId_organization_id_fk": { | ||
| "name": "invitation_organizationId_organization_id_fk", | ||
| "tableFrom": "invitation", | ||
| "tableTo": "organization", | ||
| "columnsFrom": ["organizationId"], | ||
| "columnsTo": ["id"], | ||
| "onDelete": "cascade", | ||
| "onUpdate": "no action" | ||
| } | ||
| }, | ||
| "compositePrimaryKeys": {}, | ||
| "uniqueConstraints": {}, | ||
| "checkConstraints": {} | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for invitation schema definition
rg -A 20 -B 5 'invitation.*=.*(?:pgTable|sqliteTable|mysqlTable)' --type tsRepository: InfinityBowman/corates
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Find files that contain "invitation" and likely have schema definitions
fd -e ts -e js | head -20
echo "---"
# Search for invitation in schema-related files
rg "invitation" --type ts -l | grep -E "(schema|migration|db)" | head -10
echo "---"
# Broader search for invitation table definition
rg "invitation" --type ts -A 5 -B 2 | head -100Repository: InfinityBowman/corates
Length of output: 739
🏁 Script executed:
#!/bin/bash
# Check the structure of packages/workers
ls -la packages/workers/
echo "---"
# Find all TypeScript schema files
fd -e ts -p "*schema*" packages/
echo "---"
# Search specifically for "invitation" in the codebase with context
rg "invitation" packages/ -A 10 | head -150Repository: InfinityBowman/corates
Length of output: 11958
🏁 Script executed:
#!/bin/bash
# Look for schema definition files in packages/workers/src
find packages/workers/src -type f -name "*.ts" -o -name "*.js" | head -30
echo "---"
# Search for projectInvitations definition
rg "projectInvitations" packages/workers/src --type ts --type js -B 5 -A 5 | head -80Repository: InfinityBowman/corates
Length of output: 6705
🏁 Script executed:
#!/bin/bash
# Read the schema file to find invitation table definition
cat -n packages/workers/src/db/schema.js | head -200Repository: InfinityBowman/corates
Length of output: 10225
🏁 Script executed:
#!/bin/bash
# Continue reading the schema file to see the complete projectInvitations table definition
cat -n packages/workers/src/db/schema.js | sed -n '200,220p'Repository: InfinityBowman/corates
Length of output: 724
🏁 Script executed:
#!/bin/bash
# Check the migrations directory for unique constraint definitions
ls -la packages/workers/migrations/
echo "---"
# Search for unique constraints in migration files
rg "unique" packages/workers/migrations/ -A 2 -B 2 | head -100Repository: InfinityBowman/corates
Length of output: 7406
🏁 Script executed:
#!/bin/bash
# Double-check the invitation table section in the snapshot file to be thorough
sed -n '120,207p' packages/workers/migrations/meta/0001_snapshot.json | tail -20Repository: InfinityBowman/corates
Length of output: 704
Add unique constraint to prevent duplicate invitations per organization.
The invitation table allows the same email address to receive multiple pending invitations to the same organization. Add a unique constraint on (email, organizationId) to the schema definition in packages/workers/src/db/schema.js.
🤖 Prompt for AI Agents
In packages/workers/migrations/meta/0001_snapshot.json around lines 120-207, the
invitation table lacks a unique constraint for (email, organizationId); add a
unique constraint in the authoritative schema source
(packages/workers/src/db/schema.js) by adding a uniqueConstraints entry for
invitation with columns ["email","organizationId"], then regenerate or update
the migration snapshot so the JSON includes that unique constraint under the
invitation object's "uniqueConstraints" key (or add it manually), and ensure the
migration that runs against existing DB either creates a UNIQUE index or handles
existing duplicate rows before applying the constraint.
| console.error( | ||
| `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`, | ||
| ); | ||
| const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { | ||
| reason: 'email_mismatch', | ||
| userEmail: currentUser.email, | ||
| invitationEmail: invitation.email, | ||
| }); | ||
| return c.json(error, error.statusCode); | ||
| } |
There was a problem hiding this comment.
Avoid logging PII (email addresses) in error messages.
Logging user emails constitutes PII exposure, which raises compliance concerns (GDPR/CCPA). Consider logging only non-identifying information or hashed/masked values.
Proposed fix
if (normalizedUserEmail !== normalizedInvitationEmail) {
console.error(
- `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`,
+ `[Invitation] Email mismatch for user=${authUser.id}, invitation=${invitation.id}`,
);
const error = createDomainError(AUTH_ERRORS.FORBIDDEN, {
reason: 'email_mismatch',
- userEmail: currentUser.email,
- invitationEmail: invitation.email,
});
return c.json(error, error.statusCode);
}📝 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.
| console.error( | |
| `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`, | |
| ); | |
| const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { | |
| reason: 'email_mismatch', | |
| userEmail: currentUser.email, | |
| invitationEmail: invitation.email, | |
| }); | |
| return c.json(error, error.statusCode); | |
| } | |
| console.error( | |
| `[Invitation] Email mismatch for user=${currentUser.id}, invitation=${invitation.id}`, | |
| ); | |
| const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { | |
| reason: 'email_mismatch', | |
| }); | |
| return c.json(error, error.statusCode); |
🤖 Prompt for AI Agents
In packages/workers/src/routes/orgs/invitations.js around lines 418 to 427, the
code currently logs and returns raw user and invitation email addresses which
exposes PII; remove raw emails from console.error and from the createDomainError
payload, replacing them with non-identifying info such as a reason and either a
hashed/masked version of the emails (e.g., mask local part or store a short
hash) or omit them entirely; update the console message to a generic "Email
mismatch" with contextual IDs only, and ensure the error object passed to c.json
excludes raw email fields and uses only non-PII fields (reason, relevant IDs, or
masked/hash values).
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
packages/web/src/stores/adminStore.js (1)
250-262: Cache invalidation target appears incorrect.Same issue as in
grantAccess- when revoking subscription access fromuserId, the code invalidatesqueryKeys.subscription.current(the admin's subscription cache) instead of the target user's cache.Apply the same fix pattern: invalidate user-specific caches that actually display the modified subscription data.
packages/workers/src/__tests__/seed-schemas.js (1)
81-81: Schema mismatch:orgIdmust match database definition.The
orgIdfield isnullable().optional().default(null)in the seed schema, but inpackages/workers/src/db/schema.jstheprojects.orgIdcolumn is.notNull()with a required foreign key reference. Update the seed schema to requireorgIdby removingnullable(),optional(), anddefault(null).Proposed fix
export const seedProjectSchema = z.object({ id: z.string().min(1, 'Project ID is required'), name: z.string().min(1, 'Project name is required'), description: z.string().nullable().optional().default(null), - orgId: z.string().nullable().optional().default(null), + orgId: z.string().min(1, 'Organization ID is required'), createdBy: z.string().min(1, 'Created by user ID is required'), createdAt: dateOrTimestampToNumber, updatedAt: dateOrTimestampToNumber, });packages/workers/src/middleware/requireOrg.js (1)
35-56: Wrap database operation in try-catch block.The organization membership query (lines 38-48) is not wrapped in a try-catch block, while the database queries in
requireProjectAccess(lines 113-132, 151-168) correctly use try-catch. Database operations can fail due to connection issues or timeouts and should be handled gracefully.As per coding guidelines:
Wrap database operations in try-catch blocks and return domain errors using
createDomainError(SYSTEM_ERRORS.DB_ERROR, ...)with operation metadataProposed fix
const db = createDb(c.env.DB); - // Check if user is a member of the organization - const membership = await db - .select({ - id: member.id, - role: member.role, - orgName: organization.name, - orgSlug: organization.slug, - }) - .from(member) - .innerJoin(organization, eq(member.organizationId, organization.id)) - .where(and(eq(member.organizationId, orgId), eq(member.userId, user.id))) - .get(); + let membership; + try { + // Check if user is a member of the organization + membership = await db + .select({ + id: member.id, + role: member.role, + orgName: organization.name, + orgSlug: organization.slug, + }) + .from(member) + .innerJoin(organization, eq(member.organizationId, organization.id)) + .where(and(eq(member.organizationId, orgId), eq(member.userId, user.id))) + .get(); + } catch (error) { + const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { + operation: 'fetch_org_membership', + orgId, + userId: user.id, + originalError: error.message, + }); + return c.json(dbError, dbError.statusCode); + } if (!membership) {packages/workers/src/db/schema.js (1)
120-123: Migration concern already flagged in previous review.The
orgIdcolumn is defined asnotNull(), which will cause the migration to fail on databases with existing projects. This was already identified in a previous review comment.packages/workers/src/routes/orgs/invitations.js (1)
415-425: Avoid logging PII (email addresses) in error messages.This issue was flagged in a previous review but appears unaddressed. Logging raw email addresses constitutes PII exposure, which raises compliance concerns (GDPR/CCPA). The error response also exposes both emails to the client.
Proposed fix
if (normalizedUserEmail !== normalizedInvitationEmail) { console.error( - `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`, + `[Invitation] Email mismatch for user=${authUser.id}, invitation=${invitation.id}`, ); const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { reason: 'email_mismatch', - userEmail: currentUser.email, - invitationEmail: invitation.email, }); return c.json(error, error.statusCode); }
🧹 Nitpick comments (12)
.cursor/rules/api-routes.mdc (1)
214-232: Organization-scoped routes section is clear but examples lack error handling.The new section (lines 214–232) properly documents the route structure and middleware usage for org-scoped routes. However, the examples show only the happy path. Consider adding a note about error handling for org membership and project access failures, or reference the error handling section for completeness.
For example, you might add context that org/project access failures should return appropriate
AUTHORIZATION_ERRORSor similar domain errors (as per the error handling guide on line 120–142).packages/web/src/components/checklist/LocalAppraisalsPanel.jsx (3)
34-45: Consider adding error handling for the delete operation.If
deleteChecklistfails, the user receives no feedback. While the hook may handle errors internally, explicit handling here would improve UX.Proposed improvement
const handleDelete = async (e, checklistId) => { e.stopPropagation(); const confirmed = await confirmDialog.open({ title: 'Delete Appraisal', description: 'Are you sure you want to delete this appraisal? This cannot be undone.', confirmText: 'Delete', variant: 'danger', }); if (confirmed) { - await deleteChecklist(checklistId); + try { + await deleteChecklist(checklistId); + } catch (err) { + console.error('Failed to delete checklist:', err); + // Consider showing a toast notification here + } } };
102-118: **Loading state shows empty grid.**Whenloading()is true andchecklists()is empty, the grid renders nothing. Consider showing a loading skeleton or spinner for better perceived performance during initial data fetch.
165-171: Addaria-labelfor screen reader accessibility.The delete button has a
titleattribute, but screen readers may not reliably announce it. Adding an explicitaria-labelimproves accessibility.Proposed fix
<button onClick={e => handleDelete(e, checklist.id)} class='rounded-lg p-1.5 text-gray-400 transition-colors hover:bg-red-50 hover:text-red-600 focus:ring-2 focus:ring-blue-500 focus:outline-none' title='Delete appraisal' + aria-label='Delete appraisal' > <FiTrash2 class={compact() ? 'h-4 w-4' : 'h-5 w-5'} /> </button>packages/web/src/primitives/useRecentsNav.js (1)
30-49: Consider preventing duplicate tracking on initial render.The
createEffectwill track the initiallocation.pathnameon mount, which may add the current page to recents before the user intentionally navigates. This could be intentional behavior, but if you only want to track actual navigation events, consider comparing against a previous path.Optional: Skip initial tracking
+ let isFirstRun = true; + // Track navigation and add to recents createEffect(() => { const path = location.pathname; + if (isFirstRun) { + isFirstRun = false; + return; + } const item = parsePathToRecentItem(path); if (!item) return;packages/web/src/components/sidebar/Sidebar.jsx (2)
170-178: Route change detection uses mutable variable outside reactive scope.The
lastPathnamevariable is declared outside the effect but mutated inside it. While this works, it's a pattern that can be confusing. Consider using a signal or ref for clarity.Optional: Use a ref-like pattern for previous pathname
- // Close mobile sidebar when route changes - let lastPathname = location.pathname; - createEffect(() => { - const currentPathname = location.pathname; - if (props.mobileOpen && currentPathname !== lastPathname) { - props.onCloseMobile?.(); - } - lastPathname = currentPathname; - }); + // Close mobile sidebar when route changes + const lastPathnameRef = { current: location.pathname }; + createEffect(() => { + const currentPathname = location.pathname; + if (props.mobileOpen && currentPathname !== lastPathnameRef.current) { + props.onCloseMobile?.(); + } + lastPathnameRef.current = currentPathname; + });
248-391: Consider extracting shared sidebar content to reduce duplication.The desktop expanded content (lines 248-391) and mobile panel content (lines 494-636) are nearly identical. This duplication increases maintenance burden and risk of divergence.
Consider extracting the shared content into a
SidebarContentcomponent or a render function that both views can use:const SidebarContent = () => ( <div class='sidebar-scrollbar flex-1 overflow-x-hidden overflow-y-auto'> {/* Dashboard/Workspace Link */} {/* Recents Section */} {/* Cloud Projects Section */} {/* Local Checklists Section */} <div class='h-8' /> </div> );Then use
<SidebarContent />in both the desktop and mobile views.Also applies to: 494-636
packages/web/src/stores/adminStore.js (1)
20-319: Consider applying consistent error handling across all fetch calls.While
grantAccessandrevokeAccessnow usehandleFetchError, the remaining 14 async functions still use manual error handling patterns. For consistency and to align with coding guidelines, consider refactoring the other functions to usehandleFetchErroras well.This would provide uniform error handling and make future maintenance easier.
As per coding guidelines,
handleFetchErrorshould be used for all frontend fetch calls in this file.packages/workers/migrations/0002_equal_iron_man.sql (1)
2-12: Consider adding an index on the foreign key column.SQLite doesn't automatically create indexes on foreign key columns. If queries frequently join
mediaFilestouserviauploadedBy, consider adding an index for better performance:CREATE INDEX idx_mediaFiles_uploadedBy ON mediaFiles(uploadedBy);This can be added in a separate migration if needed. Based on coding guidelines, use DrizzleKit to generate the index migration.
packages/workers/src/config/validation.js (1)
82-90: Consider extracting org roles to a constant for consistency.The
orgRolevalidation uses a hardcoded enum['member', 'admin', 'owner'], whileroleuses thePROJECT_ROLESconstant. For consistency and maintainability, consider extracting org roles to a constant inconfig/constants.js(similar toPROJECT_ROLES), especially since the same hierarchy is defined inrequireOrg.jsasORG_ROLE_HIERARCHY.Proposed refactor
In
packages/workers/src/config/constants.js, add:export const ORG_ROLES = ['member', 'admin', 'owner'];Then update this schema:
+import { PROJECT_ROLES, ORG_ROLES } from './constants.js'; export const invitationSchemas = { create: z.object({ email: z.string().email('Invalid email address'), role: z.enum(PROJECT_ROLES, { error: `Role must be one of: ${PROJECT_ROLES.join(', ')}`, }), - orgRole: z.enum(['member', 'admin', 'owner'], { + orgRole: z.enum(ORG_ROLES, { - error: "Org role must be one of: 'member', 'admin', 'owner'", + error: `Org role must be one of: ${ORG_ROLES.join(', ')}`, }), }),Also update
packages/workers/src/middleware/requireOrg.js:+import { ORG_ROLES } from '../config/constants.js'; -const ORG_ROLE_HIERARCHY = ['member', 'admin', 'owner']; +const ORG_ROLE_HIERARCHY = ORG_ROLES;packages/docs/guides/organizations.md (1)
20-34: Add language specifier to fenced code blocks.Per static analysis, this diagram block should have a language specifier. Use
textorplaintextfor ASCII diagrams:Suggested fix
-``` +```text Organization | +-- Projects (owned by org)The same applies to blocks at lines 150, 274, and 406.
packages/workers/src/routes/orgs/invitations.js (1)
163-264: Consider extracting email sending logic to a separate utility.The magic link generation and email queueing logic (lines 163-264) is complex and spans ~100 lines within the route handler. This could be extracted to a utility function for better testability and reusability.
Suggested approach
Consider creating a utility like
sendProjectInvitationEmail(c.env, { email, token, project, inviter, role })insrc/lib/invitation-email.jsthat encapsulates:
- Magic link URL generation via Better Auth
- Email template generation
- Queue submission
This would make the route handler more focused on business logic and make the email logic reusable/testable.
| ``` | ||
| /api/orgs/:orgId/projects/:projectId/... | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block for route structure should have a language specifier for proper syntax highlighting and consistency.
Proposed fix
-```
+```text
/api/orgs/:orgId/projects/:projectId/...</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In packages/docs/guides/api-development.md around lines 86 to 88, the fenced
code block for the route structure is missing a language specifier; update the
opening fence to include a language (e.g., change ``` to ```text) so the block
becomes ```text /api/orgs/:orgId/projects/:projectId/... ``` to enable proper
syntax highlighting and consistency.
| const response = await handleFetchError( | ||
| fetch(`${API_BASE}/api/admin/users/${userId}/subscription`, { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }), | ||
| { showToast: false }, | ||
| ); | ||
| const result = await response.json(); | ||
|
|
||
| // Invalidate subscription cache so frontend immediately reflects the change | ||
| queryClient.invalidateQueries({ queryKey: queryKeys.subscription.current }); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Cache invalidation target appears incorrect.
When granting subscription access to userId, the code invalidates queryKeys.subscription.current, which likely refers to the currently logged-in admin's subscription cache, not the target user's subscription. This means the UI won't reflect the subscription changes for the user being modified.
Consider invalidating a user-specific cache or the user details/list cache instead. For example:
queryKeys.users.detail(userId)if viewing user detailsqueryKeys.users.listif on the users list page- Or pass the appropriate query key as a parameter
Proposed fix example
const result = await response.json();
- // Invalidate subscription cache so frontend immediately reflects the change
- queryClient.invalidateQueries({ queryKey: queryKeys.subscription.current });
+ // Invalidate user cache so frontend immediately reflects the change
+ queryClient.invalidateQueries({ queryKey: queryKeys.users.detail(userId) });
+ queryClient.invalidateQueries({ queryKey: queryKeys.users.list });
return result;| PRAGMA foreign_keys=OFF;--> statement-breakpoint | ||
| CREATE TABLE `__new_mediaFiles` ( | ||
| `id` text PRIMARY KEY NOT NULL, | ||
| `filename` text NOT NULL, | ||
| `originalName` text, | ||
| `fileType` text, | ||
| `fileSize` integer, | ||
| `uploadedBy` text, | ||
| `bucketKey` text NOT NULL, | ||
| `createdAt` integer DEFAULT (unixepoch()), | ||
| FOREIGN KEY (`uploadedBy`) REFERENCES `user`(`id`) ON UPDATE no action ON DELETE set null | ||
| ); | ||
| --> statement-breakpoint | ||
| INSERT INTO `__new_mediaFiles`("id", "filename", "originalName", "fileType", "fileSize", "uploadedBy", "bucketKey", "createdAt") SELECT "id", "filename", "originalName", "fileType", "fileSize", "uploadedBy", "bucketKey", "createdAt" FROM `mediaFiles`;--> statement-breakpoint | ||
| DROP TABLE `mediaFiles`;--> statement-breakpoint | ||
| ALTER TABLE `__new_mediaFiles` RENAME TO `mediaFiles`;--> statement-breakpoint | ||
| PRAGMA foreign_keys=ON; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the drizzle configuration and schema definitions
fd -t f "drizzle" --max-one-result
fd -t f "schema" packages/workers/src
find packages/workers -name "*.config.*" -o -name "drizzle.config.*"Repository: InfinityBowman/corates
Length of output: 466
🏁 Script executed:
# Look for the mediaFiles and user table schema definitions
rg -t ts "export const mediaFiles" packages/workers/src
rg -t ts "export const user" packages/workers/srcRepository: InfinityBowman/corates
Length of output: 48
🏁 Script executed:
# Check the git history of the migration file to understand its origin
git log --oneline -n 20 -- packages/workers/migrations/0002_equal_iron_man.sqlRepository: InfinityBowman/corates
Length of output: 182
🏁 Script executed:
# Look for drizzle migration patterns and configuration
find packages/workers -type f -name "*.config.*"
cat packages/workers/drizzle.config.ts 2>/dev/null || echo "Config not found"Repository: InfinityBowman/corates
Length of output: 289
🏁 Script executed:
# Read the schema file to find table definitions
cat packages/workers/src/db/schema.jsRepository: InfinityBowman/corates
Length of output: 9374
🏁 Script executed:
# Check if there are any other migration-related files or patterns
ls -la packages/workers/migrations/ | head -20Repository: InfinityBowman/corates
Length of output: 460
🏁 Script executed:
# Look for any data migration or validation scripts
find packages/workers -type f -name "*seed*" -o -name "*migration*" -o -name "*validate*"Repository: InfinityBowman/corates
Length of output: 140
Verify data consistency before running this migration: ensure no orphaned uploadedBy references exist.
This migration adds a foreign key constraint from mediaFiles.uploadedBy to user(id) with ON DELETE set null. If any existing mediaFiles rows reference non-existent user IDs, the INSERT operation on line 14 will fail with a foreign key constraint violation.
Before deploying, verify that all current uploadedBy values correspond to valid users in the user table. If orphaned references exist, they must be cleaned up first (set to NULL or delete the rows).
🤖 Prompt for AI Agents
In packages/workers/migrations/0002_equal_iron_man.sql around lines 1-17, the
migration adds a foreign key on mediaFiles.uploadedBy but does not handle
existing orphaned uploadedBy values, which will cause the INSERT to fail; before
creating __new_mediaFiles and copying data, add a pre-migration cleanup step
that finds mediaFiles rows whose uploadedBy is not present in user and either
sets uploadedBy = NULL for those rows (recommended) or deletes them, e.g. run a
SELECT to identify orphans and then an UPDATE to NULL them, or include those
cleanup SQL statements in this migration before the CREATE/INSERT so the data
copy succeeds.
| await env.DB.prepare( | ||
| 'INSERT INTO mediaFiles (id, filename, bucketKey, uploadedBy, createdAt) VALUES (?1, ?2, ?3, ?4, ?5)', | ||
| ) | ||
| .bind('media-1', 'test.pdf', 'bucket-key-1', 'u1', nowSec) | ||
| .run(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the resetSchema function to see all tables it creates
head -n 250 packages/workers/src/__tests__/admin.test.js | tail -n +49 | grep -E "CREATE TABLE|mediaFiles"Repository: InfinityBowman/corates
Length of output: 300
🏁 Script executed:
# Let's search for any mediaFiles table creation in the entire test file
rg -n "mediaFiles" packages/workers/src/__tests__/admin.test.js | head -20Repository: InfinityBowman/corates
Length of output: 366
🏁 Script executed:
# Let's check the full resetSchema function more carefully
sed -n '49,225p' packages/workers/src/__tests__/admin.test.jsRepository: InfinityBowman/corates
Length of output: 5263
🏁 Script executed:
# Search for mediaFiles table creation anywhere in the workers directory
rg -n "CREATE TABLE.*mediaFiles" packages/workers/Repository: InfinityBowman/corates
Length of output: 241
🏁 Script executed:
# Check if there are migration files that might define mediaFiles
fd -t f "\.sql$|migration" packages/workers/Repository: InfinityBowman/corates
Length of output: 231
🏁 Script executed:
# Search for any mediaFiles schema definition in the entire codebase
rg -n "mediaFiles" packages/workers/src/ -A 2 | head -40Repository: InfinityBowman/corates
Length of output: 3155
🏁 Script executed:
# Check if there's a database schema file or migrations directory
find packages/workers -type f -name "*schema*" -o -name "*migration*" | head -20Repository: InfinityBowman/corates
Length of output: 174
🏁 Script executed:
# Check the beginning of admin.test.js to see test setup
head -n 50 packages/workers/src/__tests__/admin.test.jsRepository: InfinityBowman/corates
Length of output: 1624
🏁 Script executed:
# Look for beforeEach or beforeAll hooks in admin.test.js
rg -n "beforeEach|beforeAll|env\.DB|resetSchema" packages/workers/src/__tests__/admin.test.js | head -20Repository: InfinityBowman/corates
Length of output: 1102
🏁 Script executed:
# Check what seed-schemas.js does
cat packages/workers/src/__tests__/seed-schemas.jsRepository: InfinityBowman/corates
Length of output: 5755
🏁 Script executed:
# Verify the migration file has mediaFiles table definition
head -n 30 packages/workers/migrations/0000_acoustic_power_pack.sqlRepository: InfinityBowman/corates
Length of output: 965
Test will fail: mediaFiles table not created in resetSchema().
The test inserts directly into the mediaFiles table at line 683, but resetSchema() does not create this table. The function only creates: user, account, projects, project_members, session, verification, twoFactor, and subscriptions. The mediaFiles table is defined in the production migrations but excluded from the test schema.
Either add mediaFiles table creation to resetSchema(), or use the full migration SQL to initialize the test database.
🤖 Prompt for AI Agents
packages/workers/src/__tests__/admin.test.js lines 682-686: the test inserts
into the mediaFiles table but resetSchema() used for tests does not create that
table, causing the test to fail; fix by either adding the mediaFiles table DDL
to resetSchema() (create table mediaFiles with columns id, filename, bucketKey,
uploadedBy, createdAt and any required types/indexes matching production) or
change the test setup to run the full migration SQL before tests so the
mediaFiles table is present.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.