feat(organizations): GET + POST + DELETE /api/organizations/domains#749
Conversation
GET, POST, and DELETE /api/organizations/domains per the contract in recoupable/docs#261. Rows in organization_domains drive email-domain auto-join (assignAccountToOrg / selectOrgByDomain). - GET lists an org's mapped domains (member or Recoup admin only) - POST maps a domain to an org: normalizes (lowercase, trim, strip leading @), idempotent for the same org, 409 when the domain is already mapped to a different org (selectOrgByDomain uses .single(), duplicates would break auto-join) - DELETE unmaps a domain, idempotent - New supabase functions under lib/supabase/organization_domains/ - canManageOrganization helper: org member or Recoup admin Part of recoupable/chat#1841 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds a new ChangesOrganization Domains Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as "domains/route.ts"
participant Handler as "addOrgDomainHandler"
participant Auth as "canManageOrganization"
participant DB as "Supabase organization_domains"
Client->>Route: POST /api/organizations/domains
Route->>Handler: addOrgDomainHandler(request)
Handler->>Handler: validateAuthContext + validateAddOrgDomainBody
Handler->>Auth: canManageOrganization(accountId, organizationId)
Auth-->>Handler: true/false
alt access denied
Handler-->>Client: 403 error
else access allowed
Handler->>DB: selectOrganizationDomain(domain)
DB-->>Handler: existing row or null
alt domain mapped to different org
Handler-->>Client: 409 conflict
else no conflict
Handler->>DB: insertOrganizationDomain(domain, organizationId)
DB-->>Handler: inserted row or null
Handler-->>Client: 200 success / 500 error
end
end
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 25 files
Confidence score: 2/5
- The highest risk is in
lib/organizations/addOrgDomainHandler.ts,lib/organizations/getOrgDomainsHandler.ts, andlib/organizations/removeOrgDomainHandler.ts: 500-path catch blocks return raw exception messages, which can expose internal DB/query/path details to clients if errors occur in production — replace these responses with a hardcoded safe message (for example, "Internal server error") and keep full details only in server logs before merging. lib/organizations/__tests__/addOrgDomainHandler.test.ts,lib/organizations/__tests__/getOrgDomainsHandler.test.ts, andlib/organizations/__tests__/removeOrgDomainHandler.test.tsdo not adequately cover the catch-block failure paths, so this leak can regress unnoticed — add explicit tests that simulate thrown errors and assert sanitized 500 bodies to de-risk future changes.- There is some non-blocking test/maintainability cleanup (
lib/organizations/__tests__/validateAddOrgDomainBody.test.tsmissing a status assertion andlib/supabase/organization_domains/__tests__/insertOrganizationDomain.test.tsnot reliably restoringconsole.error) that could hide failures or cause flaky side effects — tighten these assertions and restore mocks in test hooks as follow-up or in this PR if quick.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/supabase/organization_domains/__tests__/insertOrganizationDomain.test.ts">
<violation number="1" location="lib/supabase/organization_domains/__tests__/insertOrganizationDomain.test.ts:57">
P2: The `console.error` spy in the error-path test won't be restored if an assertion fails before `mockRestore()`, potentially silencing real `console.error` calls in subsequent tests. Switch `beforeEach` from `vi.clearAllMocks()` to `vi.restoreAllMocks()` — it automatically restores all spies after each test and eliminates the need for manual `mockRestore()` calls entirely.</violation>
</file>
<file name="lib/supabase/organization_domains/__tests__/selectOrganizationDomains.test.ts">
<violation number="1" location="lib/supabase/organization_domains/__tests__/selectOrganizationDomains.test.ts:30">
P3: None of the 3 tests assert that `.select()` was called, let alone with `"*"`. If the implementation changed to select a subset of columns, these tests would not catch it. Add `expect(selectFn).toHaveBeenCalledWith("*")` to the first test.</violation>
</file>
<file name="lib/supabase/organization_domains/__tests__/deleteOrganizationDomain.test.ts">
<violation number="1" location="lib/supabase/organization_domains/__tests__/deleteOrganizationDomain.test.ts:36">
P3: `console.error` spy cleanup won't run if an assertion fails. Move `consoleError.mockRestore()` to an `afterEach` block or use a try/finally to prevent spy leaks on test failure.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Route as /api/organizations/domains (route.ts)
participant Handler as Handler (get/add/removeOrgDomainsHandler)
participant Auth as validateAuthContext
participant Access as canManageOrganization
participant Validator as ValidateQueryOrBody
participant Supabase as Supabase Helper (select/insert/delete)
participant DB as organization_domains table
participant AutoJoin as assignAccountToOrg (existing flow)
Note over Client,AutoJoin: NEW: Domain mapping endpoints (GET/POST/DELETE)
Client->>Route: HTTP request (GET/POST/DELETE)
Route->>Handler: Forward to appropriate handler
alt POST /api/organizations/domains
Handler->>Auth: validateAuthContext(request)
alt Unauthenticated
Auth-->>Handler: 401 NextResponse
Handler-->>Route: 401 {status:"error", error:"..."}
Route-->>Client: 401
else Authenticated
Auth-->>Handler: {accountId, orgId, authToken}
Handler->>Validator: validateAddOrgDomainBody(body)
alt Invalid body
Validator-->>Handler: 400 NextResponse
Handler-->>Route: 400 {status:"error", message:"..."}
Route-->>Client: 400
else Valid body
Validator-->>Handler: {organizationId, domain} (domain normalized)
Handler->>Access: canManageOrganization({accountId, organizationId})
alt No access
Access-->>Handler: false
Handler-->>Route: 403 {status:"error", message:"Access denied..."}
Route-->>Client: 403
else Has access
Access-->>Handler: true
Handler->>Supabase: selectOrganizationDomain(normalizedDomain)
Supabase->>DB: SELECT * FROM organization_domains WHERE domain = ?
DB-->>Supabase: row or null
Supabase-->>Handler: row or null
alt Domain found and belongs to different org
Handler-->>Route: 409 {status:"error", message:"already mapped..."}
Route-->>Client: 409
else Domain not found or same org
opt Domain not found
Handler->>Supabase: insertOrganizationDomain({domain, organizationId})
Supabase->>DB: INSERT INTO organization_domains ... RETURNING *
DB-->>Supabase: new row
Supabase-->>Handler: new row
end
Handler-->>Route: 200 {status:"success", id, domain, organization_id}
Route-->>Client: 200
end
end
end
end
else GET /api/organizations/domains
Handler->>Auth: validateAuthContext(request)
alt Unauthenticated
Auth-->>Handler: 401
Handler-->>Route: 401
Route-->>Client: 401
else Authenticated
Auth-->>Handler: auth context
Handler->>Validator: validateGetOrgDomainsQuery(request)
alt Invalid query
Validator-->>Handler: 400
Handler-->>Route: 400
Route-->>Client: 400
else Valid query
Validator-->>Handler: {organization_id}
Handler->>Access: canManageOrganization(...)
alt No access
Access-->>Handler: false
Handler-->>Route: 403
Route-->>Client: 403
else Has access
Access-->>Handler: true
Handler->>Supabase: selectOrganizationDomains(orgId)
Supabase->>DB: SELECT * FROM organization_domains WHERE organization_id = ? ORDER BY created_at
DB-->>Supabase: rows[]
alt DB error
Supabase-->>Handler: null
Handler-->>Route: 500 {status:"error", message:"..."}
Route-->>Client: 500
else Success
Supabase-->>Handler: domains[]
Handler-->>Route: 200 {status:"success", domains}
Route-->>Client: 200
end
end
end
end
else DELETE /api/organizations/domains
Handler->>Auth: validateAuthContext(request)
alt Unauthenticated
Auth-->>Handler: 401
Handler-->>Route: 401
Route-->>Client: 401
else Authenticated
Auth-->>Handler: auth context
Handler->>Validator: validateRemoveOrgDomainQuery(request)
alt Invalid query
Validator-->>Handler: 400
Handler-->>Route: 400
Route-->>Client: 400
else Valid query
Validator-->>Handler: {organization_id, domain} (domain normalized)
Handler->>Access: canManageOrganization(...)
alt No access
Access-->>Handler: false
Handler-->>Route: 403
Route-->>Client: 403
else Has access
Access-->>Handler: true
Handler->>Supabase: deleteOrganizationDomain({domain, organizationId})
Supabase->>DB: DELETE FROM organization_domains WHERE organization_id = ? AND domain = ?
alt DB error
DB-->>Supabase: error
Supabase-->>Handler: false
Handler-->>Route: 500
Route-->>Client: 500
else Success
DB-->>Supabase: success
Supabase-->>Handler: true
Handler-->>Route: 200 {status:"success"}
Route-->>Client: 200
end
end
end
end
end
Note over AutoJoin: EXISTING: Domain auto-join on login/signup
AutoJoin->>Supabase: selectOrgByDomain(domain)
Supabase->>DB: SELECT ... WHERE domain = ? (single row)
DB-->>Supabase: row (or error if multiple found)
Note over AutoJoin,DB: NEW: 409 on POST prevents duplicate rows breaking .single()
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| expect(result).toBeNull(); | ||
| expect(consoleError).toHaveBeenCalled(); | ||
| consoleError.mockRestore(); |
There was a problem hiding this comment.
P2: The console.error spy in the error-path test won't be restored if an assertion fails before mockRestore(), potentially silencing real console.error calls in subsequent tests. Switch beforeEach from vi.clearAllMocks() to vi.restoreAllMocks() — it automatically restores all spies after each test and eliminates the need for manual mockRestore() calls entirely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/organization_domains/__tests__/insertOrganizationDomain.test.ts, line 57:
<comment>The `console.error` spy in the error-path test won't be restored if an assertion fails before `mockRestore()`, potentially silencing real `console.error` calls in subsequent tests. Switch `beforeEach` from `vi.clearAllMocks()` to `vi.restoreAllMocks()` — it automatically restores all spies after each test and eliminates the need for manual `mockRestore()` calls entirely.</comment>
<file context>
@@ -0,0 +1,59 @@
+
+ expect(result).toBeNull();
+ expect(consoleError).toHaveBeenCalled();
+ consoleError.mockRestore();
+ });
+});
</file context>
| const orderFn = vi.fn().mockResolvedValue({ data: rows, error: null }); | ||
| const eqFn = vi.fn().mockReturnValue({ order: orderFn }); | ||
| vi.mocked(supabase.from).mockReturnValue({ | ||
| select: vi.fn().mockReturnValue({ eq: eqFn }), |
There was a problem hiding this comment.
P3: None of the 3 tests assert that .select() was called, let alone with "*". If the implementation changed to select a subset of columns, these tests would not catch it. Add expect(selectFn).toHaveBeenCalledWith("*") to the first test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/organization_domains/__tests__/selectOrganizationDomains.test.ts, line 30:
<comment>None of the 3 tests assert that `.select()` was called, let alone with `"*"`. If the implementation changed to select a subset of columns, these tests would not catch it. Add `expect(selectFn).toHaveBeenCalledWith("*")` to the first test.</comment>
<file context>
@@ -0,0 +1,66 @@
+ const orderFn = vi.fn().mockResolvedValue({ data: rows, error: null });
+ const eqFn = vi.fn().mockReturnValue({ order: orderFn });
+ vi.mocked(supabase.from).mockReturnValue({
+ select: vi.fn().mockReturnValue({ eq: eqFn }),
+ } as never);
+
</file context>
| }); | ||
|
|
||
| it("returns false on delete error", async () => { | ||
| const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); |
There was a problem hiding this comment.
P3: console.error spy cleanup won't run if an assertion fails. Move consoleError.mockRestore() to an afterEach block or use a try/finally to prevent spy leaks on test failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/organization_domains/__tests__/deleteOrganizationDomain.test.ts, line 36:
<comment>`console.error` spy cleanup won't run if an assertion fails. Move `consoleError.mockRestore()` to an `afterEach` block or use a try/finally to prevent spy leaks on test failure.</comment>
<file context>
@@ -0,0 +1,51 @@
+ });
+
+ it("returns false on delete error", async () => {
+ const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
+ const eqDomainFn = vi.fn().mockResolvedValue({ error: { message: "boom" } });
+ const eqOrgFn = vi.fn().mockReturnValue({ eq: eqDomainFn });
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
lib/organizations/validateRemoveOrgDomainQuery.ts (2)
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeprecated Zod v4 constructor params.
{ message: "..." }and the chained.uuid()string method are deprecated in Zod v4 in favor of the unifiederrorparam and top-levelz.uuid(). They still work today, but worth migrating to the recommended v4 API for consistency with the rest of the codebase going forward.[dependency_check]
- Flagging: Zod v4 constructor error param (
messagevserror) and chained.uuid()vs top-levelz.uuid().- Verified: per Zod v4 changelog,
"Replaces message param with error. The old message parameter is still supported but deprecated."♻️ Suggested modernization
export const removeOrgDomainQuerySchema = z.object({ organization_id: z - .string({ message: "organization_id is required" }) - .uuid("organization_id must be a valid UUID"), - domain: z.string({ message: "domain is required" }), + .string({ error: "organization_id is required" }) + .pipe(z.uuid({ error: "organization_id must be a valid UUID" })), + domain: z.string({ error: "domain is required" }), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/validateRemoveOrgDomainQuery.ts` around lines 7 - 12, Update removeOrgDomainQuerySchema to use Zod v4’s unified error handling and top-level UUID schema. Replace the z.string({ message: ... }) usage on organization_id and domain with the error option, and swap the chained .uuid() on organization_id for z.uuid() with the same validation message. Keep the schema shape and field names unchanged while modernizing the constructor and validation API.
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
badRequesthelper is duplicated across validators.The same
badRequestimplementation appears invalidateAddOrgDomainBody.ts(per provided context snippet). Consider extracting this into a shared utility (e.g.lib/organizations/badRequest.tsor a networking helper) to avoid drift between validators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/validateRemoveOrgDomainQuery.ts` around lines 20 - 25, The badRequest helper is duplicated across organization validators and should be centralized to prevent drift. Extract the shared NextResponse JSON error builder used by validateRemoveOrgDomainQuery and validateAddOrgDomainBody into a common utility such as badRequest.ts, then update both validators to import and use that shared helper instead of keeping separate implementations.lib/organizations/getOrgDomainsHandler.ts (1)
35-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDRY: extract the repeated error-response construction.
The
NextResponse.json({ status: "error", message }, { status, headers: getCorsHeaders() })shape is duplicated for the 403, 500, and catch-all paths here and almost certainly across the sibling handlers in this cohort. A small shared helper (e.g.errorJson(message, status)) would keep the CORS/status/shape consistent and reduce copy-paste drift.As per coding guidelines: "Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/getOrgDomainsHandler.ts` around lines 35 - 63, The error-response construction in getOrgDomainsHandler is duplicated across the 403, 500, and catch-all paths, so extract it into a shared helper such as errorJson(message, status) and use that in this handler. Make the helper return the standardized NextResponse.json error payload with getCorsHeaders() so the shape, headers, and status stay consistent. If there are sibling handlers with the same pattern, update them to use the same helper to avoid copy-paste drift.Source: Coding guidelines
lib/organizations/validateGetOrgDomainsQuery.ts (1)
6-10: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefer
z.uuid()here.z.string({ message: ... })still works in Zod 4, so only the UUID validator needs updating.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/validateGetOrgDomainsQuery.ts` around lines 6 - 10, The getOrgDomainsQuerySchema validator still uses z.string(...).uuid(...) for organization_id, but the review asks to switch to z.uuid() directly. Update the organization_id field in getOrgDomainsQuerySchema to use z.uuid() and keep the existing required/error messaging behavior consistent with the schema.lib/organizations/validateAddOrgDomainBody.ts (3)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
badRequesthelper across validators.The same
badRequest(message)shape (400 JSON +getCorsHeaders()) appears to be reimplemented inlib/organizations/validateRemoveOrgDomainQuery.tsper the provided cross-file snippets. Extract this into a shared utility (e.g.lib/networking/badRequest.ts) to avoid drift between validators.As per path instructions, "DRY: Consolidate similar logic into shared utilities."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/validateAddOrgDomainBody.ts` around lines 19 - 24, The badRequest(message) helper is duplicated across the organization domain validators, so extract the shared 400 JSON response with getCorsHeaders() into a reusable utility and have both validateAddOrgDomainBody and validateRemoveOrgDomainQuery call it. Move the common response-building logic into a shared helper such as badRequest in a networking utility, then replace the local helper definitions with that shared function to keep the response shape consistent and avoid drift.Source: Path instructions
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer inferred type over hand-written interface.
AddOrgDomainBodyis manually authored. Path instructions forlib/**/validate*.tsfiles call for exporting the schema's inferred type so the two never drift as the schema evolves.♻️ Suggested fix
-export interface AddOrgDomainBody { - organizationId: string; - /** The normalized bare email domain (lowercase, no "@") */ - domain: string; -} +export type AddOrgDomainBody = z.infer<typeof addOrgDomainBodySchema>;As per path instructions, "Create validate functions ... that export both the schema and inferred TypeScript type."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/validateAddOrgDomainBody.ts` around lines 13 - 17, The AddOrgDomainBody type is hand-written and can drift from the validation schema; update validateAddOrgDomainBody to export the schema’s inferred TypeScript type instead of a manual interface. Use the existing validation schema symbol in this module to derive the type, and keep the exported name AddOrgDomainBody so callers continue to reference the same type while staying aligned with the schema as it evolves.Source: Path instructions
6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUsing deprecated Zod 4 APIs.
.string().uuid(...)and the{ message: ... }constructor param are both deprecated in Zod 4 in favor of the top-levelz.uuid()and the unified{ error: ... }param. They still work today but will be removed in a future major version, andz.uuid()in v4 is also stricter (validates RFC 9562/4122 variant bits) — worth confirming that's the desired behavior for existingorganizationIdvalues.♻️ Suggested modernization
export const addOrgDomainBodySchema = z.object({ - organizationId: z - .string({ message: "organizationId is required" }) - .uuid("organizationId must be a valid UUID"), - domain: z.string({ message: "domain is required" }), + organizationId: z.uuid({ error: "organizationId must be a valid UUID" }), + domain: z.string({ error: "domain is required" }), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/validateAddOrgDomainBody.ts` around lines 6 - 11, The addOrgDomainBodySchema uses deprecated Zod 4 patterns for validation messages and UUID checks. Update the organizationId field to use the top-level z.uuid() with the unified { error: ... } option instead of z.string(...).uuid(...), and switch the domain field constructor to the unified error parameter as well. Keep the changes localized in addOrgDomainBodySchema and confirm the stricter RFC 9562/4122 UUID validation still matches expected organizationId values.Source: Path instructions
lib/organizations/addOrgDomainHandler.ts (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated error-response boilerplate.
The
{status:"error", message}+ status code +getCorsHeaders()shape is repeated four times. Extracting a small helper (similar tobadRequestin the validator file) would reduce duplication and centralize the response contract.As per path instructions, "DRY: Extract common validation and error handling logic."
Also applies to: 50-56, 62-65, 79-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/organizations/addOrgDomainHandler.ts` around lines 41 - 44, The error-response pattern in addOrgDomainHandler is duplicated across multiple branches, so extract a small helper to centralize it. Create a reusable response helper for the common NextResponse.json({ status: "error", message }, { status, headers: getCorsHeaders() }) shape, similar to the validator file’s badRequest helper, and use it in the access-denied and other validation/error paths in addOrgDomainHandler to keep the response contract consistent and DRY.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/organizations/addOrgDomainHandler.ts`:
- Around line 22-87: The addOrgDomainHandler function is too long and should be
refactored to fit the function-length limits. Extract the repeated
NextResponse.json error निर्माण for the 403/409/500 cases into a small helper
such as errorResponse, and move the existing-or-insert domain lookup logic
around selectOrganizationDomain and insertOrganizationDomain into a separate
helper. Keep addOrgDomainHandler focused on auth, validation, and orchestration
while preserving the same behavior and response shapes.
- Around line 77-86: The catch block in addOrgDomainHandler is leaking raw
exception details by returning error.message to the client. Keep the full error
logged server-side with console.error, but change the NextResponse.json payload
to always return a generic message for failures instead of using the caught
Error’s message. Use the addOrgDomainHandler error handling path and the
existing error instanceof Error branch as the place to update this behavior.
- Around line 47-59: The domain mapping check in addOrgDomainHandler is racy
because selectOrganizationDomain and insertOrganizationDomain are separate
operations, so concurrent requests can both pass the pre-check. Move the
one-domain-per-organization guarantee into the database by enforcing uniqueness
on organization_domains.domain, then change the addOrgDomainHandler flow to use
an atomic insert/upsert path and handle duplicate-key conflicts by returning the
existing conflict response. Keep the existing logic around validated.domain and
validated.organizationId, but make the insert path safe under concurrency.
In `@lib/organizations/getOrgDomainsHandler.ts`:
- Around line 55-63: The 500 response in getOrgDomainsHandler is exposing
internal details by returning error.message to callers. Keep the full error only
in the existing server-side console.error inside getOrgDomainsHandler, and
change the NextResponse.json payload to return a generic client-facing message
instead of using error.message when handling the catch block.
In `@lib/supabase/organization_domains/selectOrganizationDomain.ts`:
- Around line 14-23: The selectOrganizationDomain lookup currently returns null
for Supabase query failures, which makes addOrgDomainHandler treat an actual
error as “not mapped” and continue to insert. Update selectOrganizationDomain to
distinguish “no row found” from “query failed” by throwing the Supabase error or
returning an error-aware result instead of null, and then adjust the
addOrgDomainHandler call site to handle that distinct failure path without
falling through to insertOrganizationDomain.
---
Nitpick comments:
In `@lib/organizations/addOrgDomainHandler.ts`:
- Around line 41-44: The error-response pattern in addOrgDomainHandler is
duplicated across multiple branches, so extract a small helper to centralize it.
Create a reusable response helper for the common NextResponse.json({ status:
"error", message }, { status, headers: getCorsHeaders() }) shape, similar to the
validator file’s badRequest helper, and use it in the access-denied and other
validation/error paths in addOrgDomainHandler to keep the response contract
consistent and DRY.
In `@lib/organizations/getOrgDomainsHandler.ts`:
- Around line 35-63: The error-response construction in getOrgDomainsHandler is
duplicated across the 403, 500, and catch-all paths, so extract it into a shared
helper such as errorJson(message, status) and use that in this handler. Make the
helper return the standardized NextResponse.json error payload with
getCorsHeaders() so the shape, headers, and status stay consistent. If there are
sibling handlers with the same pattern, update them to use the same helper to
avoid copy-paste drift.
In `@lib/organizations/validateAddOrgDomainBody.ts`:
- Around line 19-24: The badRequest(message) helper is duplicated across the
organization domain validators, so extract the shared 400 JSON response with
getCorsHeaders() into a reusable utility and have both validateAddOrgDomainBody
and validateRemoveOrgDomainQuery call it. Move the common response-building
logic into a shared helper such as badRequest in a networking utility, then
replace the local helper definitions with that shared function to keep the
response shape consistent and avoid drift.
- Around line 13-17: The AddOrgDomainBody type is hand-written and can drift
from the validation schema; update validateAddOrgDomainBody to export the
schema’s inferred TypeScript type instead of a manual interface. Use the
existing validation schema symbol in this module to derive the type, and keep
the exported name AddOrgDomainBody so callers continue to reference the same
type while staying aligned with the schema as it evolves.
- Around line 6-11: The addOrgDomainBodySchema uses deprecated Zod 4 patterns
for validation messages and UUID checks. Update the organizationId field to use
the top-level z.uuid() with the unified { error: ... } option instead of
z.string(...).uuid(...), and switch the domain field constructor to the unified
error parameter as well. Keep the changes localized in addOrgDomainBodySchema
and confirm the stricter RFC 9562/4122 UUID validation still matches expected
organizationId values.
In `@lib/organizations/validateGetOrgDomainsQuery.ts`:
- Around line 6-10: The getOrgDomainsQuerySchema validator still uses
z.string(...).uuid(...) for organization_id, but the review asks to switch to
z.uuid() directly. Update the organization_id field in getOrgDomainsQuerySchema
to use z.uuid() and keep the existing required/error messaging behavior
consistent with the schema.
In `@lib/organizations/validateRemoveOrgDomainQuery.ts`:
- Around line 7-12: Update removeOrgDomainQuerySchema to use Zod v4’s unified
error handling and top-level UUID schema. Replace the z.string({ message: ... })
usage on organization_id and domain with the error option, and swap the chained
.uuid() on organization_id for z.uuid() with the same validation message. Keep
the schema shape and field names unchanged while modernizing the constructor and
validation API.
- Around line 20-25: The badRequest helper is duplicated across organization
validators and should be centralized to prevent drift. Extract the shared
NextResponse JSON error builder used by validateRemoveOrgDomainQuery and
validateAddOrgDomainBody into a common utility such as badRequest.ts, then
update both validators to import and use that shared helper instead of keeping
separate implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 204cf44f-d863-4008-8204-370ad78f0eb5
⛔ Files ignored due to path filters (12)
lib/organizations/__tests__/addOrgDomainHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/canManageOrganization.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/getOrgDomainsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/normalizeOrgDomain.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/removeOrgDomainHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/validateAddOrgDomainBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/validateGetOrgDomainsQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/organizations/__tests__/validateRemoveOrgDomainQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/organization_domains/__tests__/deleteOrganizationDomain.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/organization_domains/__tests__/insertOrganizationDomain.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/organization_domains/__tests__/selectOrganizationDomain.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/organization_domains/__tests__/selectOrganizationDomains.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (13)
app/api/organizations/domains/route.tslib/organizations/addOrgDomainHandler.tslib/organizations/canManageOrganization.tslib/organizations/getOrgDomainsHandler.tslib/organizations/normalizeOrgDomain.tslib/organizations/removeOrgDomainHandler.tslib/organizations/validateAddOrgDomainBody.tslib/organizations/validateGetOrgDomainsQuery.tslib/organizations/validateRemoveOrgDomainQuery.tslib/supabase/organization_domains/deleteOrganizationDomain.tslib/supabase/organization_domains/insertOrganizationDomain.tslib/supabase/organization_domains/selectOrganizationDomain.tslib/supabase/organization_domains/selectOrganizationDomains.ts
| const existing = await selectOrganizationDomain(validated.domain); | ||
|
|
||
| if (existing && existing.organization_id !== validated.organizationId) { | ||
| return NextResponse.json( | ||
| { | ||
| status: "error", | ||
| message: `Domain "${validated.domain}" is already mapped to a different organization`, | ||
| }, | ||
| { status: 409, headers: getCorsHeaders() }, | ||
| ); | ||
| } | ||
|
|
||
| const row = existing ?? (await insertOrganizationDomain(validated)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File list\n'
git ls-files 'lib/organizations/*' 'lib/supabase/**' | sed -n '1,200p'
printf '\n## Outline: addOrgDomainHandler.ts\n'
ast-grep outline lib/organizations/addOrgDomainHandler.ts --view expanded || true
printf '\n## Search helpers\n'
rg -n "selectOrganizationDomain|insertOrganizationDomain|organization_domains|domain" lib/organizations lib/supabase -g '!**/*.map' || trueRepository: recoupable/api
Length of output: 37593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
ls -laRepository: recoupable/api
Length of output: 2193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '^lib/organizations/|^lib/supabase/' | sed -n '1,200p'Repository: recoupable/api
Length of output: 10889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## addOrgDomainHandler.ts\n'
cat -n lib/organizations/addOrgDomainHandler.ts | sed -n '1,220p'
printf '\n## organization_domains helpers\n'
for f in \
lib/supabase/organization_domains/selectOrganizationDomain.ts \
lib/supabase/organization_domains/insertOrganizationDomain.ts \
lib/supabase/organization_domains/selectOrgByDomain.ts \
lib/supabase/organization_domains/selectOrganizationDomains.ts
do
printf '\n### %s\n' "$f"
cat -n "$f" | sed -n '1,220p'
done
printf '\n## tests for insert/select\n'
for f in \
lib/supabase/organization_domains/__tests__/insertOrganizationDomain.test.ts \
lib/supabase/organization_domains/__tests__/selectOrganizationDomain.test.ts \
lib/organizations/__tests__/addOrgDomainHandler.test.ts
do
printf '\n### %s\n' "$f"
cat -n "$f" | sed -n '1,260p'
doneRepository: recoupable/api
Length of output: 19865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## schema/search for organization_domains\n'
rg -n "organization_domains|unique.*domain|domain.*unique|create table.*organization_domains|index.*organization_domains" . \
-g '!**/node_modules/**' -g '!**/.git/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' || true
printf '\n## likely schema files\n'
git ls-files | rg 'supabase|schema|migration|sql|database' | sed -n '1,250p'Repository: recoupable/api
Length of output: 13956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n types/database.types.ts | sed -n '2168,2215p'Repository: recoupable/api
Length of output: 1774
Make the domain mapping write atomic in lib/organizations/addOrgDomainHandler.ts:47-59. selectOrganizationDomain() and insertOrganizationDomain() are separate calls, so concurrent POSTs can both miss the pre-check and race the insert. Enforce uniqueness on organization_domains.domain and handle the insert with an upsert/duplicate-key path so the one-domain-per-organization invariant is owned by the database.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/organizations/addOrgDomainHandler.ts` around lines 47 - 59, The domain
mapping check in addOrgDomainHandler is racy because selectOrganizationDomain
and insertOrganizationDomain are separate operations, so concurrent requests can
both pass the pre-check. Move the one-domain-per-organization guarantee into the
database by enforcing uniqueness on organization_domains.domain, then change the
addOrgDomainHandler flow to use an atomic insert/upsert path and handle
duplicate-key conflicts by returning the existing conflict response. Keep the
existing logic around validated.domain and validated.organizationId, but make
the insert path safe under concurrency.
There was a problem hiding this comment.
Agreed the invariant belongs in the database — but that's a database submodule migration (unique on organization_domains.domain, plus account_organization_ids(account_id, organization_id) for the same race on the members endpoint), not app-level code in this PR. b10879e closes the adjacent misclassification hole (select error no longer falls through to insert). Filing a follow-up issue for the constraints + upsert conversion.
There was a problem hiding this comment.
@sweetmantech That's a reasonable scoping decision — the uniqueness invariant really does belong at the database schema level (in the database submodule) rather than being simulated with app-level check-then-insert logic here, and consolidating it with the analogous account_organization_ids(account_id, organization_id) race on the members endpoint into one migration makes sense. Good catch on b10879e closing the select-error misclassification gap in the meantime — that at least prevents silently proceeding to insert on a failed lookup.
Happy to open the follow-up issue for you now if you'd like — just confirm and I'll file it with a summary of both races (domains + memberships), the required unique constraints, and the upsert/duplicate-key handling needed in the app code once the constraints land.
✏️ Learnings added
Learnt from: sweetmantech
Repo: recoupable/api PR: 749
File: lib/organizations/addOrgDomainHandler.ts:0-0
Timestamp: 2026-07-03T19:04:48.211Z
Learning: In recoupable/api, the one-domain-per-organization invariant for `organization_domains.domain` and the one-membership-per-account-per-organization invariant for `account_organization_ids(account_id, organization_id)` are intended to be enforced via unique constraints added in the `database` submodule (via migration), not via app-level check-then-insert logic in handlers like `lib/organizations/addOrgDomainHandler.ts`. App-level insert/upsert logic should be converted to rely on these constraints once added.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…omes - Consolidate duplicate access helpers into one canManageOrganization (org member or Recoup admin); members validators now use it and canManageOrgMembers is deleted - Move auth + access checks into per-endpoint request validators (validateGetOrgDomainsRequest, validateAddOrgDomainRequest, validateRemoveOrgDomainRequest), absorbing the old query/body validators; handlers slim to validator -> data call -> response - Harden the three 500 catch blocks: hardcoded "Internal server error" (real error only in console.error) with leak-regression tests - Strengthen error-shape assertions (body.status + body.message) Response contract unchanged per recoupable/docs#261 (including the 409 cross-org conflict and idempotent POST/DELETE). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 27 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
| body: AddOrgDomainBody; | ||
| } | ||
|
|
||
| function badRequest(message: string): NextResponse { |
There was a problem hiding this comment.
DRY - this should already exist somewhere as a shared lib.
There was a problem hiding this comment.
Fixed in b10879e — local badRequest() deleted; all error paths across the members+domains surface now use the shared lib/networking/errorResponse.ts. Note this converges the error envelope on {status:\"error\", error} (the shared helper's and validateAuthContext's shape) — docs follow-up PR updates the contract to match.
| query: RemoveOrgDomainQuery; | ||
| } | ||
|
|
||
| function badRequest(message: string): NextResponse { |
There was a problem hiding this comment.
DRY - use existing shared lib
There was a problem hiding this comment.
Fixed in b10879e — same conversion as the add validator; shared errorResponse everywhere on this surface.
…rorResponse envelope
- Replace hand-rolled { status: "error", message } bodies with the shared
lib/networking/errorResponse helper across the organizations members and
domains validators + handlers, so the whole surface emits one
{ status: "error", error } envelope (matching validateAuthContext 401s).
Error body key changes from "message" to "error" on these endpoints;
status codes and success shapes are unchanged.
- selectOrganizationDomain now throws on Supabase query errors instead of
returning null, so addOrgDomainHandler can no longer mistake a transient
failure for "domain not mapped" and double-map a domain; the handler's
try/catch turns the throw into the hardened 500 (new test covers it).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preview verification — 2026-07-03 (final build)Preview
Not exercised live: 403 (no non-member, non-admin credential available) — covered by unit tests. Every Done-when criterion from recoupable/chat#1841 for this item is verified. Merge order: this PR, then docs#263. |
There was a problem hiding this comment.
1 issue found across 22 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/organizations/addOrgMemberHandler.ts">
<violation number="1" location="lib/organizations/addOrgMemberHandler.ts:3">
P1: The `errorResponse` utility produces `{status: "error", error}` but the contract (per PR description and original target-branch code) specifies `{status: "error", message}`. Replacing inlined `NextResponse.json` calls with `errorResponse` changes the response envelope key from `message` to `error`, which will break any client inspecting `response.message`. Either update `errorResponse` to emit `message` as the key, or update the contract and commit message to reflect the new `error` key.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -1,5 +1,6 @@ | |||
| import { NextRequest, NextResponse } from "next/server"; | |||
| import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; | |||
| import { errorResponse } from "@/lib/networking/errorResponse"; | |||
There was a problem hiding this comment.
P1: The errorResponse utility produces {status: "error", error} but the contract (per PR description and original target-branch code) specifies {status: "error", message}. Replacing inlined NextResponse.json calls with errorResponse changes the response envelope key from message to error, which will break any client inspecting response.message. Either update errorResponse to emit message as the key, or update the contract and commit message to reflect the new error key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/organizations/addOrgMemberHandler.ts, line 3:
<comment>The `errorResponse` utility produces `{status: "error", error}` but the contract (per PR description and original target-branch code) specifies `{status: "error", message}`. Replacing inlined `NextResponse.json` calls with `errorResponse` changes the response envelope key from `message` to `error`, which will break any client inspecting `response.message`. Either update `errorResponse` to emit `message` as the key, or update the contract and commit message to reflect the new `error` key.</comment>
<file context>
@@ -1,5 +1,6 @@
import { NextRequest, NextResponse } from "next/server";
import { getCorsHeaders } from "@/lib/networking/getCorsHeaders";
+import { errorResponse } from "@/lib/networking/errorResponse";
import { validateAddOrgMemberRequest } from "@/lib/organizations/validateAddOrgMemberRequest";
import { getOrCreateAccountByEmail } from "@/lib/accounts/getOrCreateAccountByEmail";
</file context>
…tatus, error} (#263) The api converged these endpoints' error paths on the shared errorResponse helper (recoupable/api#749), which emits {status:"error", error} — the same shape validateAuthContext's 401s always had. New ErrorEnvelope schema; the 16 error responses across the five member/domain operations now reference it. The pre-existing artists endpoint keeps OrganizationsErrorResponse ({status, message}), which is what it actually emits. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Implements the organization domain-mapping endpoints from the tracking issue recoupable/chat#1841, per the OpenAPI contract in recoupable/docs#261.
Merge order: recoupable/docs#261 (contract) merges first, then this PR.
Rows in
organization_domainsdrive email-domain auto-join on login/signup (lib/organizations/assignAccountToOrg.tsviaselectOrgByDomain). These endpoints let org members manage those mappings.Endpoints (
app/api/organizations/domains/route.ts)?organization_id=<uuid>: lists the org's mapped domains. 200{status:"success", domains:[{id, domain, organization_id, created_at}]}{organizationId, domain}: maps a domain to the org. The domain is normalized (lowercase, trimmed, leading@stripped) and must be a plausible bare domain (at least one dot; no spaces, slashes, or@). Idempotent when already mapped to the same org (returns the existing row). Returns 409 when the domain is mapped to a different org, sinceselectOrgByDomainuses.single()and duplicate rows would break auto-join. 200{status:"success", id, domain, organization_id}?organization_id=<uuid>&domain=<domain>: removes the mapping, idempotent. 200{status:"success"}Errors on all: 400 invalid params, 401 unauthenticated (via
validateAuthContext), 403 not a member, shape{status:"error", message}per the contract'sOrganizationsErrorResponse.Access control
Caller must be a member of the organization (or the org account itself) or a Recoup admin. New
lib/organizations/canManageOrganization.tscomposes the existingvalidateOrganizationAccess+isRecoupAdminhelpers (validateAuthContext's built-in org check alone would 403 Recoup admins who are not members).New supabase functions (
lib/supabase/organization_domains/)selectOrganizationDomains(by org),selectOrganizationDomain(by domain, for the 409/idempotency check),insertOrganizationDomain,deleteOrganizationDomain, alongside the existingselectOrgByDomain.Verification
tsc --noEmit: 200 errors before, 200 after (all pre-existing, zero new)pnpm lintandpnpm format:checkcleanContract deviations
validateAuthContexthelper use the pre-existing{status:"error", error}key instead ofmessage. Changing that helper would touch every authenticated endpoint, so it is left as-is; all responses emitted by the new handlers/validators (400/403/409/500) follow the contract's{status:"error", message}shape.Part of recoupable/chat#1841. Contract: recoupable/docs#261.
🤖 Generated with Claude Code
Summary by cubic
Adds GET, POST, and DELETE /api/organizations/domains so org members can manage email‑domain auto‑join mappings, per
recoupable/docs#261. Also standardizes errors to a shared{status:"error", error}shape and hardens conflict handling to avoid double-mapping.New Features
canManageOrganization; CORS preflight supported.Refactors
canManageOrgMemberswithcanManageOrganization; validators now use it.validateGetOrgDomainsRequest,validateAddOrgDomainRequest,validateRemoveOrgDomainRequest.errorResponse({status:"error", error}); status codes and success shapes unchanged.selectOrganizationDomainnow throws on query errors to prevent treating failures as “not mapped”; 500s return a generic "Internal server error" with leak‑regression tests.Written for commit b10879e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Style