feat: organizations schema, membership, and drive ownership (#578-580)#741
Open
2witstudios wants to merge 10 commits intomasterfrom
Open
feat: organizations schema, membership, and drive ownership (#578-580)#7412witstudios wants to merge 10 commits intomasterfrom
2witstudios wants to merge 10 commits intomasterfrom
Conversation
Organizations foundation schema with CUID2 IDs, role enum (OWNER/ADMIN/MEMBER), invitation tokens with expiry, proper indexes and unique constraints. Wired into schema aggregator and explicit re-exports in db index. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- org-repository.ts: CRUD, access checks, member listing - org-membership.ts: invitation create/accept/revoke flows - API routes: POST/GET /, GET/PUT/DELETE /[orgId], GET/POST members, DELETE members/[userId], POST invitations/[token]/accept - All routes use Next.js 15 async params pattern and session-only auth Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add nullable orgId FK on drives table with index - Org members automatically see org-owned drives in their drive list - Update organizations relations to include drives - Drive listing queries org membership for auto-visibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d import 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
📝 WalkthroughWalkthroughAdds organization support across the stack: DB schema and migration, repository and membership/invitation logic, API routes for orgs/members/invitations, and org-aware drive access with test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Handler"
participant Auth as "Auth Service"
participant OrgSvc as "Org Service"
participant DB as "Database"
Client->>API: POST /api/organizations/.../invitations/{token}
API->>Auth: authenticate (session + CSRF)
Auth-->>API: userId
API->>OrgSvc: acceptInvitation(token, userId)
OrgSvc->>DB: SELECT invitation by token
DB-->>OrgSvc: invitation
OrgSvc->>DB: SELECT user by id / INSERT org_member / UPDATE invitation
DB-->>OrgSvc: success
OrgSvc-->>API: { orgId }
API-->>Client: 200 OK { orgId }
sequenceDiagram
participant Client
participant API as "API Handler"
participant Auth as "Auth Service"
participant Repo as "Org Repo"
participant DB as "Database"
Client->>API: GET /api/organizations/{orgId}
API->>Auth: authenticate (read)
Auth-->>API: userId
API->>Repo: checkOrgAccess(orgId, userId)
Repo->>DB: SELECT organization, SELECT org_members
DB-->>Repo: org + role
alt org missing
API-->>Client: 404
else not member
API-->>Client: 403
else member
API-->>Client: 200 { org, currentUserRole }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Replace `catch (error)` with `catch` in API route handlers where the error variable is not referenced, fixing @typescript-eslint/no-unused-vars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Aligns TypeScript type with Zod schema that accepts string | null | undefined for avatarUrl when updating an organization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 5
🧹 Nitpick comments (3)
packages/lib/src/organizations/org-membership.ts (1)
145-151: Push pending filters into the DB query.Line 145 currently fetches all org invitations and filters in memory at Line 151. Apply
acceptedAt IS NULLandexpiresAt > nowin SQL to reduce payload and avoid unnecessary row scans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/organizations/org-membership.ts` around lines 145 - 151, The code currently fetches all rows via db.query.orgInvitations.findMany and then filters in memory; instead add the predicates to the DB query so acceptedAt IS NULL and expiresAt > now are evaluated in SQL. Update the db.query.orgInvitations.findMany call (the orgInvitations query) to include where conditions for eq(orgInvitations.orgId, orgId), isNull(orgInvitations.acceptedAt) (or eq(..., null) depending on query API) and gt(orgInvitations.expiresAt, new Date()/db.now()) so the returned invitations already satisfy acceptedAt unset and expiresAt > now, then remove the in-memory filter.packages/db/src/schema/organizations.ts (1)
14-23: Remove redundant indexes on already-unique columns.Line 14 (
slug) and Line 46 (token) already create indexes via unique constraints; adding Line 22 and Line 54 duplicates index maintenance cost without improving lookups.♻️ Proposed fix
export const organizations = pgTable('organizations', { @@ }, (table) => ({ ownerIdx: index('organizations_owner_id_idx').on(table.ownerId), - slugIdx: index('organizations_slug_idx').on(table.slug), })); @@ export const orgInvitations = pgTable('org_invitations', { @@ }, (table) => ({ orgIdx: index('org_invitations_org_id_idx').on(table.orgId), emailIdx: index('org_invitations_email_idx').on(table.email), - tokenIdx: index('org_invitations_token_idx').on(table.token), orgEmailKey: unique('org_invitations_org_email_key').on(table.orgId, table.email), }));Also applies to: 46-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/organizations.ts` around lines 14 - 23, Remove the duplicate index definitions on columns that are already unique: delete the slug index entry (the slugIdx/index('organizations_slug_idx').on(table.slug) in the organizations table) and the corresponding token index entry for the tokens table (the index entry that targets the token column which duplicates the unique constraint). Leave the unique() column declarations (slug.unique(), token.unique()) intact and only remove the extra index(...) lines and their exported index identifiers (e.g., slugIdx, tokenIdx) to avoid redundant index maintenance.apps/web/src/app/api/organizations/[orgId]/invitations/[token]/route.ts (1)
7-26: UnusedorgIdparameter — consider validating token belongs to org.The
orgIdfrom params is extracted but not used. While the token alone identifies the invitation, consider validating that the invitation's org matches the URL'sorgIdto prevent URL manipulation where a valid token is used with an incorrect orgId.try { - const result = await acceptInvitation(token, auth.userId); + const result = await acceptInvitation(token, auth.userId, orgId); return NextResponse.json(result);This would require updating
acceptInvitationto accept and validate the orgId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/organizations/`[orgId]/invitations/[token]/route.ts around lines 7 - 26, The route POST currently extracts orgId from context.params but never uses it; update the handler to pass the orgId into acceptInvitation and ensure acceptInvitation (and its callers) validate that the invitation's organization matches the provided orgId, returning an appropriate error (e.g., 404 or 400) when it does not; specifically, modify the POST function to await context.params for both token and orgId, call acceptInvitation(token, auth.userId, orgId), and update acceptInvitation to check the invitation.orgId === orgId (and throw a clear Error when mismatched) so the route's catch block can map that to the correct status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/src/organizations/org-membership.ts`:
- Around line 128-139: The two separate DB writes (the insert into orgMembers
using db.insert(...).values(...) and the update of orgInvitations via
db.update(...).set(...).where(...)) must be executed within a single transaction
so they either both succeed or both roll back; wrap both operations in a
transaction using your DB client's transaction API (e.g., db.transaction or
db.$transaction), perform the insert of orgMembers and the update of
orgInvitations inside that transaction block, and surface/throw any error so the
transaction rolls back if the second write fails (ensuring no orphaned
membership records remain if marking the invitation fails).
- Around line 55-79: The current logic only checks acceptedAt and blocks all
existing invites even if expired, while accepted invites fall through causing
unique constraint violations; update the check around existingInvitation (from
db.query.orgInvitations.findFirst) to: if existingInvitation exists and has
acceptedAt -> throw (user already accepted/in member), else if
existingInvitation exists and not accepted and expiresAt is in the future ->
throw (active pending invite), but if existingInvitation exists and expiresAt is
past allow creating a new invitation (optionally remove or overwrite the old
invite before inserting). Use the existing symbols existingInvitation,
existingInvitation.acceptedAt, existingInvitation.expiresAt, orgInvitations, and
the insert block that creates token/createId to implement this conditional flow.
In `@packages/lib/src/organizations/org-repository.ts`:
- Around line 73-87: Wrap the organization insert and the owner membership
insert in a single database transaction so they either both commit or both roll
back; specifically, execute the db.insert(...).values({...}).returning() that
creates the org and the subsequent db.insert(...orgMembers...).values({...})
that creates the OWNER row inside the same transactional block (e.g.,
db.transaction or the repository's transactional API), using orgId/org.id and
ownerId as currently used, and ensure the transaction returns the created org
only after both inserts succeed or throws/rolls back on failure.
- Around line 196-203: The current code builds countResults by calling
db.query.orgMembers.findMany per orgId causing N+1 queries; replace that with a
single grouped/count query over all orgIds (use db.query.orgMembers.aggregate or
a groupBy/count approach on orgMembers where orgId in orgIds, selecting orgId
and count) and then map the aggregated rows to the orgIds array (produce {
orgId, count } entries, defaulting to 0 for orgIds with no rows). Update the
code that references countResults to use this aggregated result instead of
per-org queries.
In `@packages/lib/src/services/drive-service.ts`:
- Around line 100-128: getDriveAccess (and thus getDriveWithAccess) currently
only checks drive ownership and driveMembers and returns null for users who
access a drive via org membership; update getDriveAccess to also query
orgMembers for membership in the drive's org and, if the user is an org member
of the drive's org, return access with role 'MEMBER' (unless a higher role from
driveMembers or owner overrides it) and isMember=true; use the existing drives,
orgMembers, and driveMembers symbols to locate the drive's orgId and membership,
ensure includeTrash/trashed checks mirror other checks, and preserve precedence
so explicit drive roles (OWNER/ADMIN/MEMBER) override org-derived MEMBER access.
---
Nitpick comments:
In `@apps/web/src/app/api/organizations/`[orgId]/invitations/[token]/route.ts:
- Around line 7-26: The route POST currently extracts orgId from context.params
but never uses it; update the handler to pass the orgId into acceptInvitation
and ensure acceptInvitation (and its callers) validate that the invitation's
organization matches the provided orgId, returning an appropriate error (e.g.,
404 or 400) when it does not; specifically, modify the POST function to await
context.params for both token and orgId, call acceptInvitation(token,
auth.userId, orgId), and update acceptInvitation to check the invitation.orgId
=== orgId (and throw a clear Error when mismatched) so the route's catch block
can map that to the correct status.
In `@packages/db/src/schema/organizations.ts`:
- Around line 14-23: Remove the duplicate index definitions on columns that are
already unique: delete the slug index entry (the
slugIdx/index('organizations_slug_idx').on(table.slug) in the organizations
table) and the corresponding token index entry for the tokens table (the index
entry that targets the token column which duplicates the unique constraint).
Leave the unique() column declarations (slug.unique(), token.unique()) intact
and only remove the extra index(...) lines and their exported index identifiers
(e.g., slugIdx, tokenIdx) to avoid redundant index maintenance.
In `@packages/lib/src/organizations/org-membership.ts`:
- Around line 145-151: The code currently fetches all rows via
db.query.orgInvitations.findMany and then filters in memory; instead add the
predicates to the DB query so acceptedAt IS NULL and expiresAt > now are
evaluated in SQL. Update the db.query.orgInvitations.findMany call (the
orgInvitations query) to include where conditions for eq(orgInvitations.orgId,
orgId), isNull(orgInvitations.acceptedAt) (or eq(..., null) depending on query
API) and gt(orgInvitations.expiresAt, new Date()/db.now()) so the returned
invitations already satisfy acceptedAt unset and expiresAt > now, then remove
the in-memory filter.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/web/src/app/api/organizations/[orgId]/invitations/[token]/route.tsapps/web/src/app/api/organizations/[orgId]/members/[userId]/route.tsapps/web/src/app/api/organizations/[orgId]/members/route.tsapps/web/src/app/api/organizations/[orgId]/route.tsapps/web/src/app/api/organizations/route.tspackages/db/drizzle/0090_early_black_cat.sqlpackages/db/drizzle/meta/0090_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/schema.tspackages/db/src/schema/core.tspackages/db/src/schema/organizations.tspackages/lib/src/organizations/index.tspackages/lib/src/organizations/org-membership.tspackages/lib/src/organizations/org-repository.tspackages/lib/src/server.tspackages/lib/src/services/drive-service.ts
…queries - Add orgId: null to createMockDrive helper in drive-service tests - Add orgMembers to db mock - Update selectDistinct mock to handle 4 sequential calls (member drives, permission drives, org memberships, org drives) - Fix UpdateOrgInput avatarUrl type to accept null Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add orgId: string | null to DriveWithAccess interface in drive-service.ts - Add orgId: null to mock drive objects in 8 test files that create drive fixtures, fixing TypeScript errors from the new drives.orgId column Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/lib/src/services/drive-service.ts (1)
229-262:⚠️ Potential issue | 🟠 Major
getDriveAccessdoesn't recognize org membership — org drive users will get 403.This function only checks drive ownership and
driveMemberstable membership. Users who access a drive solely via org membership (from the newlistAccessibleDriveslogic at lines 101-129) will receive{ isMember: false, role: null }, causing API endpoints that gate on this result to return 403.Add an org membership check before returning the null-access result:
🛠️ Proposed fix
if (membership.length > 0) { const role = membership[0].role as 'ADMIN' | 'MEMBER'; return { isOwner: false, isAdmin: role === 'ADMIN', isMember: true, role, }; } + // Check org membership if drive belongs to an org + if (drive.orgId) { + const orgMembership = await db + .select({ orgId: orgMembers.orgId }) + .from(orgMembers) + .where(and(eq(orgMembers.orgId, drive.orgId), eq(orgMembers.userId, userId))) + .limit(1); + + if (orgMembership.length > 0) { + return { + isOwner: false, + isAdmin: false, + isMember: true, + role: 'MEMBER', + }; + } + } + return { isOwner: false, isAdmin: false, isMember: false, role: null };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/services/drive-service.ts` around lines 229 - 262, getDriveAccess currently only checks drive owner and driveMembers and misses users who gain access via org membership; update getDriveAccess to, after fetching the drive (getDriveById) and before returning no-access, check whether the user belongs to the drive's organization (e.g. query org membership using drive.orgId against your orgMembers/orgUsers table or call the existing listAccessibleDrives helper) and if so return isMember: true and role set appropriately (e.g., 'MEMBER' or map org role to 'ADMIN' if applicable); keep the existing owner and driveMembers checks (drive.ownerId, driveMembers query) and only return the null-access result when neither direct drive membership nor org membership applies.
🧹 Nitpick comments (1)
packages/lib/src/services/__tests__/drive-service.test.ts (1)
86-120: Well-documented mock sequence for the 4-step data gathering.The refactored
selectDistinctmock correctly handles the expanded query sequence. The comments clearly document which call corresponds to which query, making the test maintainable.Consider adding test cases for org membership scenarios (e.g., user is org member but not direct drive member) in a follow-up to fully exercise the new org-aware logic in
listAccessibleDrives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/services/__tests__/drive-service.test.ts` around lines 86 - 120, Add a new test exercising org-membership-only access so listAccessibleDrives returns drives granted via organization membership: extend the existing drive-service.test.ts to mock db.selectDistinct to simulate call `#3` returning an org membership for the user and call `#4` returning drives for that org, then call listAccessibleDrives and assert org-only drives are included; reference the mocked db.selectDistinct behavior and the listAccessibleDrives function when adding the new test so the 4-step mock sequence (driveMembers, pagePermissions, orgMembers, drives by orgId) is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/lib/src/services/drive-service.ts`:
- Around line 229-262: getDriveAccess currently only checks drive owner and
driveMembers and misses users who gain access via org membership; update
getDriveAccess to, after fetching the drive (getDriveById) and before returning
no-access, check whether the user belongs to the drive's organization (e.g.
query org membership using drive.orgId against your orgMembers/orgUsers table or
call the existing listAccessibleDrives helper) and if so return isMember: true
and role set appropriately (e.g., 'MEMBER' or map org role to 'ADMIN' if
applicable); keep the existing owner and driveMembers checks (drive.ownerId,
driveMembers query) and only return the null-access result when neither direct
drive membership nor org membership applies.
---
Nitpick comments:
In `@packages/lib/src/services/__tests__/drive-service.test.ts`:
- Around line 86-120: Add a new test exercising org-membership-only access so
listAccessibleDrives returns drives granted via organization membership: extend
the existing drive-service.test.ts to mock db.selectDistinct to simulate call `#3`
returning an org membership for the user and call `#4` returning drives for that
org, then call listAccessibleDrives and assert org-only drives are included;
reference the mocked db.selectDistinct behavior and the listAccessibleDrives
function when adding the new test so the 4-step mock sequence (driveMembers,
pagePermissions, orgMembers, drives by orgId) is exercised.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/src/app/api/account/drives-status/__tests__/route.test.tsapps/web/src/app/api/account/handle-drive/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.tsapps/web/src/app/api/workflows/__tests__/route.test.tspackages/lib/src/organizations/org-repository.tspackages/lib/src/services/__tests__/drive-service.test.tspackages/lib/src/services/drive-service.ts
✅ Files skipped from review due to trivial changes (2)
- apps/web/src/app/api/account/drives-status/tests/route.test.ts
- apps/web/src/app/api/drives/[driveId]/members/tests/route.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lib/src/organizations/org-repository.ts
Add missing orgId field to createDriveFixture in drives route tests, matching the updated DriveWithAccess interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/app/api/drives/__tests__/route.test.ts (1)
194-216: Consider verifyingorgIdin the response contract test.The "should include all drive fields in response" test verifies presence of various drive properties but doesn't check for the new
orgIdfield. IforgIdis part of the API response contract, consider adding it here for completeness.📝 Optional: Add orgId to response contract verification
expect(body[0]).toHaveProperty('drivePrompt'); + expect(body[0]).toHaveProperty('orgId'); expect(body[0]).toHaveProperty('isOwned'); expect(body[0]).toHaveProperty('role');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/__tests__/route.test.ts` around lines 194 - 216, The test validating the GET route response misses the new orgId field; update the spec in apps/web/src/app/api/drives/__tests__/route.test.ts to assert the response includes orgId (e.g., add expect(body[0]).toHaveProperty('orgId')). Ensure the drive fixture created by createDriveFixture (used with id 'drive_full') supplies an orgId (or relies on its default) and that the mocked listAccessibleDrives still returns that fixture so GET(request) returns the orgId in body[0]; reference createDriveFixture, listAccessibleDrives and GET in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/api/drives/__tests__/route.test.ts`:
- Around line 194-216: The test validating the GET route response misses the new
orgId field; update the spec in
apps/web/src/app/api/drives/__tests__/route.test.ts to assert the response
includes orgId (e.g., add expect(body[0]).toHaveProperty('orgId')). Ensure the
drive fixture created by createDriveFixture (used with id 'drive_full') supplies
an orgId (or relies on its default) and that the mocked listAccessibleDrives
still returns that fixture so GET(request) returns the orgId in body[0];
reference createDriveFixture, listAccessibleDrives and GET in your changes.
- Fix re-invite flow: allow re-inviting after expired/accepted invitations by updating existing row instead of violating unique constraint - Make invitation acceptance atomic with db.transaction - Make org creation atomic with db.transaction (org + owner membership) - Fix N+1 member count queries with single grouped count query - Add org membership check to getDriveAccess and getDriveAccessWithDrive so users accessing drives via org membership get proper MEMBER role Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
organizations,org_members,org_invitationstables with CUID2 IDs,OrgMemberRoleenum (OWNER/ADMIN/MEMBER), proper indexes and unique constraintsorg-repository.ts(CRUD, access checks, member listing) andorg-membership.ts(invitation create/accept/revoke with 7-day expiry)/organizations, GET/PUT/DELETE/organizations/[orgId], GET/POST members, DELETEmembers/[userId], POSTinvitations/[token]/acceptorgIdFK on drives table; org members automatically see org-owned drives in their drive listArchitecture
driveMembers/driveRolespatterns for consistencylistAccessibleDrivesservice functiongetDriveAccessandgetDriveAccessWithDrivecheck org membership so org-drive users get proper MEMBER access (not 403)packages/lib/src/organizations/, exported viaserver.tsTest plan
pnpm db:generate)pnpm db:migrate)orgIdfield🤖 Generated with Claude Code