Production hardening: auth, org scoping, colleague invites, and CI - #21
Conversation
Keep local advisor plans and MCP tool descriptors out of version control.
Block role from Better Auth user input and force client role via database hook so anonymous signups cannot escalate to admin.
Remove seedIfEmpty from live GET routes and guard the seeder so it only runs against local file databases.
Exclude vendored evilcharts from linting and fix first-party Biome violations without changing runtime behavior.
Add eight read-path indexes and sync migration 0011 for schema drift (status_change_requests table and invite colleague fields).
Pin canAccessProject boundaries and invite token consumption invariants with isolated migration-backed tests.
Pull patched versions within existing semver ranges. Audit residuals: 47 advisories (1 critical, 9 high, 33 moderate, 4 low), mostly dev-tooling.
Runs blocking gates on PRs and main pushes. bun audit is advisory only.
… dueDate Fail fast when the session secret is missing in production. Serialize null milestone due dates as empty strings to match the PublicProjectMilestone type.
Inventory org-aware vs org-blind paths and recommend freeze/remove for single-operator launch.
Document wiring, dead-link bug, and options to complete or remove the feature.
Move invite lifecycle functions verbatim and re-export from records.ts so callers stay unchanged.
Pin CI to Bun 1.3.12 for reproducible builds, document advisory-only audit step, add ON DELETE SET NULL to initiated_by_client_id FK, and reject linkUserToClient when the target client does not exist.
Implements ADR 0002 Option A: portal colleague invites no longer send email on creation, resend is blocked until approval, and tests cover the approval-gated token lifecycle.
Close cross-org leaks in canAccessProject, portal summary, portal project listing, and client PATCH/DELETE. Add duplicate colleague-invite guard and tests for org-boundary enforcement.
Add adminOwnsProject and verify org ownership before project updates and deletes, matching the client mutation guard.
Add org-ownership guards for project creation, invites, nested project resources, user management, status-change requests, and pending-invite reads. Filter pending status-change requests by organization.
Strip invite tokens from portal team responses, block re-approval with 409, refresh before resend email, and return 200 for duplicate colleague invites.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds organization-scoped admin ownership guards ( ChangesOrg-scoped Authorization & Invite Lifecycle
CI, Tooling, Package Updates, and Documentation
Sequence Diagram(s)Portal Colleague Invite Creation (Fixed Flow) sequenceDiagram
participant PortalUser
participant PortalTeamRoute
participant createPortalColleagueInvite
participant AdminApproveRoute
participant sendInviteEmail
PortalUser->>PortalTeamRoute: POST /api/portal/team (email)
PortalTeamRoute->>createPortalColleagueInvite: dedup check (clientId, email)
alt existing active invite
createPortalColleagueInvite-->>PortalTeamRoute: existing inviteId
PortalTeamRoute-->>PortalUser: 200 (serialized invite, no token)
else new invite
createPortalColleagueInvite-->>PortalTeamRoute: new invite
PortalTeamRoute-->>PortalUser: 201 (serialized invite, no token)
end
Note over PortalTeamRoute,sendInviteEmail: No email sent at creation
AdminApproveRoute->>sendInviteEmail: POST /api/invites/:id/approve triggers email
sendInviteEmail-->>PortalUser: invite email with working link
Admin Org-scoped Mutation Gate sequenceDiagram
participant AdminClient
participant RouteHandler
participant adminOwnsX
participant DBQuery
participant RecordMutation
AdminClient->>RouteHandler: PATCH/DELETE mutation (resourceId)
RouteHandler->>adminOwnsX: adminOwnsX(user, resourceId)
adminOwnsX->>DBQuery: join resource → clients → org filter by activeOrganizationId
DBQuery-->>adminOwnsX: row found / not found
alt ownership fails
adminOwnsX-->>RouteHandler: false
RouteHandler-->>AdminClient: 404 notFoundError
else ownership passes
adminOwnsX-->>RouteHandler: true
RouteHandler->>RecordMutation: perform update/delete
RecordMutation-->>RouteHandler: result
RouteHandler-->>AdminClient: 200 success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
React Doctor found 5 issues in 2 files · 5 warnings · score 89 / 100 (Great) · vs 5 warnings
Reviewed by React Doctor for commit |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/routes/api/invites.ts (1)
34-42: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider removing redundant client fetch guard.
The
adminOwnsClientcheck at line 34 already verifies the client exists and belongs to the admin's organization. The subsequentif (!client)guard at lines 40-42 is now unreachable (unless the client is deleted between the two queries, which would be a rare race). ThegetClientByIdcall itself is still needed for email data, but the null check could be simplified or removed.This is a minor optimization—the current code is safe, just slightly redundant.
🤖 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 `@src/routes/api/invites.ts` around lines 34 - 42, The if (!client) null check guard that returns notFoundError is redundant because the adminOwnsClient check at the start of the block already verifies the client exists. Remove the entire if (!client) guard block while keeping the getClientById call itself, since that call is still needed to retrieve the email data from the client record. This eliminates the unreachable code path while maintaining all necessary functionality.src/routes/api/admin/status-change-requests.ts (1)
16-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
ROLES.ADMINconstant instead of hardcoded string.Per coding guidelines, role checks should use the constants from
src/auth/roles.ts.♻️ Suggested fix
Add import:
import { createFileRoute } from "`@tanstack/react-router`"; import { listAllPendingStatusChangeRequests } from "`@/db/records`"; +import { ROLES } from "`@/auth/roles`"; import {Then update the check:
- if (auth.user.role !== "admin") { + if (auth.user.role !== ROLES.ADMIN) { return forbiddenError("Admin only."); }🤖 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 `@src/routes/api/admin/status-change-requests.ts` around lines 16 - 17, The role check in the authorization guard is using a hardcoded string "admin" instead of a constant, which violates coding guidelines. Import the ROLES constant from src/auth/roles.ts at the top of the file, then replace the hardcoded string "admin" in the condition auth.user.role !== "admin" with ROLES.ADMIN to use the proper constant reference.Source: Coding guidelines
src/routes/api/invites/$id/resend.ts (1)
73-79: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueInconsistent date serialization compared to sibling routes.
The response returns
createdAtandexpiresAtas raw Date objects, but the/approveroute serializes them withtoISOString(). This may cause inconsistent API behavior for clients.♻️ Suggested fix for consistency
return Response.json({ clientId: refreshedInvite.clientId, - createdAt: refreshedInvite.createdAt, + createdAt: refreshedInvite.createdAt.toISOString(), email: refreshedInvite.email, - expiresAt: refreshedInvite.expiresAt, + expiresAt: refreshedInvite.expiresAt.toISOString(), id: refreshedInvite.id, });🤖 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 `@src/routes/api/invites/`$id/resend.ts around lines 73 - 79, The Response.json call in the resend endpoint is returning createdAt and expiresAt as raw Date objects, whereas the approve route serializes these dates using toISOString(). To ensure consistent API behavior, apply toISOString() to both the createdAt and expiresAt fields from the refreshedInvite object within the Response.json response body, matching the serialization approach used in the sibling approve route.src/routes/api/portal/team.ts (1)
35-36: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
ROLES.CLIENTconstant instead of hardcoded string.Per coding guidelines, role checks should use the constants from
src/auth/roles.ts. This applies to line 35 and line 47.♻️ Suggested fix
Add import:
import { createFileRoute } from "`@tanstack/react-router`"; import { portalInviteSchema } from "`@/api/validation`"; import { getSessionUserFromHeaders } from "`@/auth/session.server`"; +import { ROLES } from "`@/auth/roles`"; import { createPortalColleagueInvite, listPortalTeam } from "`@/db/records`";Then update both checks:
- if (user.role !== "client") { + if (user.role !== ROLES.CLIENT) { return forbiddenError("Client portal only."); }- if (auth.user.role !== "client") { + if (auth.user.role !== ROLES.CLIENT) { return forbiddenError("Client portal only."); }🤖 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 `@src/routes/api/portal/team.ts` around lines 35 - 36, Import the ROLES constant from src/auth/roles.ts at the top of the file, then replace both hardcoded "client" string comparisons in the role checks (where user.role is compared to "client") with ROLES.CLIENT to follow the coding guidelines for role validation. This ensures consistency with the centralized role constants and makes future role value changes easier to maintain.Source: Coding guidelines
src/db/schema.ts (1)
139-141: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a composite index for invite dedupe lookups.
Line 139-141 only indexes
clientId, but pending-invite checks also filter byconsumedAt,revokedAt, andexpiresAt. This can degrade into per-client scans on the portal invite path as invite history grows.🤖 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 `@src/db/schema.ts` around lines 139 - 141, The invitesClientIdIdx index in the schema currently only indexes the clientId column, but pending-invite queries also filter by email, consumedAt, revokedAt, and expiresAt. Modify the index definition to be a composite index that includes all these columns (clientId, email, consumedAt, revokedAt, and expiresAt) to prevent full table scans as invite history grows. Update the index call on table.clientId to include these additional columns in the index definition..github/workflows/quality.yml (1)
17-30: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a job timeout to prevent stuck CI runs.
A hung test/build step can block the required Quality check indefinitely. Adding a job-level timeout improves CI reliability.
Proposed change
jobs: quality: runs-on: ubuntu-latest + timeout-minutes: 25 steps:🤖 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 @.github/workflows/quality.yml around lines 17 - 30, The quality job lacks a timeout configuration, which can cause the CI workflow to hang indefinitely if any step (lint, typecheck, test, or build) gets stuck. Add a timeout-minutes property to the quality job definition to set a reasonable maximum execution time for the entire job. This will ensure the workflow fails fast if any step hangs, improving CI reliability.
🤖 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 `@docs/decisions/0001-multi-tenancy.md`:
- Around line 1-114: The ADR document status on line 3 and the recommendation
section (lines 107-114) are outdated and do not reflect the actual
implementation. Update the status header from "Proposed — awaiting maintainer
decision" to "Accepted — Option B implemented" to indicate the decision has been
made. Replace the recommendation section to reflect that Option B (finish
multi-tenancy) has been implemented through org-scoped admin ownership guards
like adminOwnsClient and adminOwnsProject added to mutation routes, org-stamping
on creation, and org-aware tests in access-control.test.ts. Document Option A as
a future alternative consideration if scope changes post-launch.
In `@src/db/invites.ts`:
- Around line 226-271: The createPortalColleagueInvite function performs a
separate read via findActivePendingInviteForClientEmail followed by an insert
operation, creating a race condition where concurrent requests for the same
clientId and email can both pass the existence check and insert duplicate active
invites. Wrap both the select query in findActivePendingInviteForClientEmail and
the subsequent insert operation in db.insert within a single database
transaction to ensure the read-check-then-insert operation is atomic and
prevents concurrent duplicate invites from being created.
In `@src/routes/api/portal/summary.ts`:
- Around line 16-18: The role comparison in the condition checking user.role !==
"client" uses a hardcoded string instead of the constant from the roles module.
Replace the hardcoded string "client" with the ROLES.CLIENT constant, ensuring
that ROLES is imported from src/auth/roles.ts at the top of the file if it is
not already imported. This applies to the role check before the forbiddenError
call in the summary.ts file.
In `@src/routes/portal/projects/`$id.tsx:
- Line 164: Replace the hardcoded string "admin" in the admins filter assignment
with the centralized ROLES.ADMIN constant. First, import ROLES from
src/auth/roles.ts at the top of the file if not already imported, then update
the filter condition in the admins variable assignment to use ROLES.ADMIN
instead of the literal string "admin" to maintain consistency with the shared
auth contract.
---
Nitpick comments:
In @.github/workflows/quality.yml:
- Around line 17-30: The quality job lacks a timeout configuration, which can
cause the CI workflow to hang indefinitely if any step (lint, typecheck, test,
or build) gets stuck. Add a timeout-minutes property to the quality job
definition to set a reasonable maximum execution time for the entire job. This
will ensure the workflow fails fast if any step hangs, improving CI reliability.
In `@src/db/schema.ts`:
- Around line 139-141: The invitesClientIdIdx index in the schema currently only
indexes the clientId column, but pending-invite queries also filter by email,
consumedAt, revokedAt, and expiresAt. Modify the index definition to be a
composite index that includes all these columns (clientId, email, consumedAt,
revokedAt, and expiresAt) to prevent full table scans as invite history grows.
Update the index call on table.clientId to include these additional columns in
the index definition.
In `@src/routes/api/admin/status-change-requests.ts`:
- Around line 16-17: The role check in the authorization guard is using a
hardcoded string "admin" instead of a constant, which violates coding
guidelines. Import the ROLES constant from src/auth/roles.ts at the top of the
file, then replace the hardcoded string "admin" in the condition auth.user.role
!== "admin" with ROLES.ADMIN to use the proper constant reference.
In `@src/routes/api/invites.ts`:
- Around line 34-42: The if (!client) null check guard that returns
notFoundError is redundant because the adminOwnsClient check at the start of the
block already verifies the client exists. Remove the entire if (!client) guard
block while keeping the getClientById call itself, since that call is still
needed to retrieve the email data from the client record. This eliminates the
unreachable code path while maintaining all necessary functionality.
In `@src/routes/api/invites/`$id/resend.ts:
- Around line 73-79: The Response.json call in the resend endpoint is returning
createdAt and expiresAt as raw Date objects, whereas the approve route
serializes these dates using toISOString(). To ensure consistent API behavior,
apply toISOString() to both the createdAt and expiresAt fields from the
refreshedInvite object within the Response.json response body, matching the
serialization approach used in the sibling approve route.
In `@src/routes/api/portal/team.ts`:
- Around line 35-36: Import the ROLES constant from src/auth/roles.ts at the top
of the file, then replace both hardcoded "client" string comparisons in the role
checks (where user.role is compared to "client") with ROLES.CLIENT to follow the
coding guidelines for role validation. This ensures consistency with the
centralized role constants and makes future role value changes easier to
maintain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e57ff244-ecef-4f85-ab3d-50ba08ac8f08
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
.github/workflows/quality.yml.gitignoreCLAUDE.mdbiome.jsoncdocs/decisions/0001-multi-tenancy.mddocs/decisions/0002-colleague-invites.mddrizzle/0011_medical_scarlet_witch.sqldrizzle/meta/0011_snapshot.jsondrizzle/meta/_journal.jsonpackage.jsonsrc/__tests__/access-control.test.tssrc/__tests__/api-admin-crud.test.tssrc/__tests__/api-collaboration.test.tssrc/__tests__/api-invite-management.test.tssrc/__tests__/api-pending-invites.test.tssrc/__tests__/api-portal-summary.test.tssrc/__tests__/api-portal-team.test.tssrc/__tests__/api-project-milestones.test.tssrc/__tests__/api-project-updates.test.tssrc/__tests__/auth-role-hardening.test.tssrc/__tests__/invite-lifecycle.test.tssrc/__tests__/records-collaboration.test.tssrc/auth/better-auth.tssrc/auth/guards.tssrc/components/auth/worker-invite-form.tsxsrc/components/common/product-charts.tsxsrc/db/client.tssrc/db/invites.tssrc/db/records.tssrc/db/schema.tssrc/routes/api/admin/status-change-requests.tssrc/routes/api/admin/status-change-requests/$id.tssrc/routes/api/clients.tssrc/routes/api/clients/$id.tssrc/routes/api/clients/$id/invites.tssrc/routes/api/files/$id.tssrc/routes/api/invites.tssrc/routes/api/invites/$id/approve.tssrc/routes/api/invites/$id/resend.tssrc/routes/api/invites/$id/revoke.tssrc/routes/api/portal/summary.tssrc/routes/api/portal/team.tssrc/routes/api/project-milestones/$id.tssrc/routes/api/project-updates/$id.tssrc/routes/api/projects.tssrc/routes/api/projects/$id.tssrc/routes/api/projects/$id/collaboration.tssrc/routes/api/projects/$id/milestones.tssrc/routes/api/projects/$id/updates.tssrc/routes/api/search.tssrc/routes/api/users/$id.tssrc/routes/portal/activity.tsxsrc/routes/portal/files.tsxsrc/routes/portal/projects/$id.tsxsrc/routes/projects/$id.tsx
💤 Files with no reviewable changes (1)
- src/tests/api-collaboration.test.ts
Greptile SummaryThis PR hardens Clientra's auth, multi-tenancy, and invite workflows for production: signup role is now forced to
Confidence Score: 4/5Safe to merge; the auth and org-scoping changes are well-structured with defense-in-depth at both the route and database layers. The core security work — forced client role on signup, org-scoped admin mutations, deferred colleague invite emails, and token stripping from portal responses — is solid and backed by new integration tests. Three non-blocking issues were found: a TOCTOU window in the duplicate-invite dedup check that could produce multiple pending invites for the same address under concurrent requests, a dueDate null-to-empty-string change that shifts the API contract and could affect frontend date-parsing logic, and an inconsistent Date serialization in the resend response. None of these affect auth or data integrity, but the first two touch the invite and milestone flows and warrant attention before the next feature builds on them. src/db/invites.ts (createPortalColleagueInvite dedup race), src/db/records.ts (dueDate null to empty string serialization change), src/routes/api/invites/$id/resend.ts (raw Date objects in response) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant PortalClient as Portal Client
participant PortalAPI as POST /api/portal/team
participant DB as Database
participant AdminAPI as POST /api/invites/:id/approve
participant Email as Email Service
participant Invitee as Invitee
PortalClient->>PortalAPI: Invite colleague (email)
PortalAPI->>DB: listPortalTeam(user) → clientId
PortalAPI->>DB: "createPortalColleagueInvite(clientId, email, token) initiatedByClientId set, adminApprovedAt = null"
DB-->>PortalAPI: invite record (no email sent)
PortalAPI-->>PortalClient: 201 invite (token stripped from response)
Note over DB: Invite sits pending admin approval
AdminAPI->>DB: getInviteRecordById(id)
AdminAPI->>DB: adminOwnsClient(user, clientId) org scope check
AdminAPI->>DB: approveInviteRecord(id) WHERE adminApprovedAt IS NULL
DB-->>AdminAPI: approved invite
AdminAPI->>Email: sendInviteEmail(token, inviteUrl)
Email-->>Invitee: Invite email delivered
AdminAPI-->>AdminAPI: returns emailSent true/false
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant PortalClient as Portal Client
participant PortalAPI as POST /api/portal/team
participant DB as Database
participant AdminAPI as POST /api/invites/:id/approve
participant Email as Email Service
participant Invitee as Invitee
PortalClient->>PortalAPI: Invite colleague (email)
PortalAPI->>DB: listPortalTeam(user) → clientId
PortalAPI->>DB: "createPortalColleagueInvite(clientId, email, token) initiatedByClientId set, adminApprovedAt = null"
DB-->>PortalAPI: invite record (no email sent)
PortalAPI-->>PortalClient: 201 invite (token stripped from response)
Note over DB: Invite sits pending admin approval
AdminAPI->>DB: getInviteRecordById(id)
AdminAPI->>DB: adminOwnsClient(user, clientId) org scope check
AdminAPI->>DB: approveInviteRecord(id) WHERE adminApprovedAt IS NULL
DB-->>AdminAPI: approved invite
AdminAPI->>Email: sendInviteEmail(token, inviteUrl)
Email-->>Invitee: Invite email delivered
AdminAPI-->>AdminAPI: returns emailSent true/false
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| createdAt: milestone.createdAt.toISOString(), | ||
| description: milestone.description, | ||
| dueDate: milestone.dueDate, | ||
| dueDate: milestone.dueDate ?? "", |
There was a problem hiding this comment.
milestone.dueDate is a nullable text column; before this change the API returned null for milestones with no due date. Changing that to "" can break any frontend code that passes dueDate directly to a date-parser (e.g., a <DatePicker> or new Date(dueDate)): new Date(null) gives the epoch, new Date("") gives Invalid Date. If the intent is to always return a string, the API contract should be updated explicitly and the frontend audited for this change.
| dueDate: milestone.dueDate ?? "", | |
| dueDate: milestone.dueDate ?? null, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/db/records.ts
Line: 454
Comment:
**null due-date silently becomes an empty string**
`milestone.dueDate` is a nullable text column; before this change the API returned `null` for milestones with no due date. Changing that to `""` can break any frontend code that passes `dueDate` directly to a date-parser (e.g., a `<DatePicker>` or `new Date(dueDate)`): `new Date(null)` gives the epoch, `new Date("")` gives `Invalid Date`. If the intent is to always return a string, the API contract should be updated explicitly and the frontend audited for this change.
```suggestion
dueDate: milestone.dueDate ?? null,
```
How can I resolve this? If you propose a fix, please make it concise.
- Use ROLES.ADMIN/ROLES.CLIENT constants instead of hardcoded strings (portal summary/team, admin status-change-requests, portal project page) - Make portal colleague-invite dedupe atomic via db.transaction to close a read-check-then-insert race that could create duplicate active invites - Add composite invites(client_id, email) index for dedupe lookups (+migration) - Serialize resend invite createdAt/expiresAt with toISOString() for parity with the approve route - Add timeout-minutes to the Quality CI job - Update ADR 0001 status to Accepted — Option B (multi-tenancy) implemented - Fix doctor script: react-doctor@2 -> @latest (npm has no @2 tag) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| </div> | ||
| </div> | ||
|
|
||
| {renderSessionActions(invitation)} |
There was a problem hiding this comment.
React Doctor · react-doctor/no-render-in-render (warning)
Your users lose state because "renderSessionActions()" builds UI from an inline call that React remounts, so pull it into its own component instead.
Fix → Make it a named component so React preserves its identity and does not remount its state.
| </div> | ||
| ) : null} | ||
| </CardContent> | ||
| <CardContent>{renderCardContent()}</CardContent> |
There was a problem hiding this comment.
React Doctor · react-doctor/no-render-in-render (warning)
Your users lose state because "renderCardContent()" builds UI from an inline call that React remounts, so pull it into its own component instead.
Fix → Make it a named component so React preserves its identity and does not remount its state.
| }; | ||
|
|
||
| if (/progress/.test(statusKey)) { | ||
| if (statusKey.includes("progress")) { |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| dark: ["#2dd4bf"], | ||
| }; | ||
| } else if (/completed/.test(statusKey)) { | ||
| } else if (statusKey.includes("completed")) { |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| dark: ["#22c55e"], | ||
| }; | ||
| } else if (/planning/.test(statusKey)) { | ||
| } else if (statusKey.includes("planning")) { |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@drizzle/meta/0012_snapshot.json`:
- Around line 1204-1210: The `requested_status` column and `approval_state`
column in the migration snapshot are defined as unconstrained TEXT fields
without database-level CHECK constraints, which allows invalid states to be
stored and diverges from the constraint definitions in
scripts/migrate-portal.ts. Add CHECK constraint definitions to both the
`requested_status` field (around line 1204) and the `approval_state` field
(around line 1218) in the snapshot JSON to restrict these columns to valid enum
values, matching the explicit CHECK clauses used in scripts/migrate-portal.ts
for the status_change_requests table. Update the constraint section at line 1286
to include these checks for both status fields.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e23da80-219d-4565-97cf-b59cb2c72fcf
📒 Files selected for processing (13)
.github/workflows/quality.ymldocs/decisions/0001-multi-tenancy.mddrizzle/0012_pale_mastermind.sqldrizzle/meta/0012_snapshot.jsondrizzle/meta/_journal.jsonpackage.jsonsrc/db/invites.tssrc/db/schema.tssrc/routes/api/admin/status-change-requests.tssrc/routes/api/invites/$id/resend.tssrc/routes/api/portal/summary.tssrc/routes/api/portal/team.tssrc/routes/portal/projects/$id.tsx
✅ Files skipped from review due to trivial changes (2)
- drizzle/0012_pale_mastermind.sql
- docs/decisions/0001-multi-tenancy.md
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/quality.yml
- drizzle/meta/_journal.json
- src/routes/api/admin/status-change-requests.ts
- src/routes/api/portal/team.ts
- src/routes/portal/projects/$id.tsx
- src/routes/api/invites/$id/resend.ts
- package.json
- src/routes/api/portal/summary.ts
- src/db/schema.ts
- src/db/invites.ts
| "requested_status": { | ||
| "name": "requested_status", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "autoincrement": false | ||
| }, |
There was a problem hiding this comment.
Add DB-level constraints for status enums to prevent invalid workflow states.
Line 1286 shows no table check constraints, so requested_status/approval_state are unconstrained TEXT in this migration path. That allows invalid states to persist and diverges from scripts/migrate-portal.ts (which creates status_change_requests with explicit CHECK clauses).
Proposed migration sketch
+-- drizzle/0013_enforce_status_change_request_checks.sql
+CREATE TABLE status_change_requests__new (
+ id TEXT PRIMARY KEY NOT NULL,
+ project_id TEXT NOT NULL REFERENCES projects(id) ON DELETE CASCADE,
+ requested_by TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE,
+ requested_status TEXT NOT NULL
+ CHECK (requested_status IN ('planning','in_progress','completed')),
+ reason TEXT NOT NULL,
+ approval_state TEXT NOT NULL DEFAULT 'pending'
+ CHECK (approval_state IN ('pending','approved','rejected')),
+ reviewed_by TEXT REFERENCES users(id) ON DELETE SET NULL,
+ reviewed_at INTEGER,
+ created_at INTEGER NOT NULL
+);
+INSERT INTO status_change_requests__new (
+ id, project_id, requested_by, requested_status, reason,
+ approval_state, reviewed_by, reviewed_at, created_at
+)
+SELECT
+ id, project_id, requested_by, requested_status, reason,
+ approval_state, reviewed_by, reviewed_at, created_at
+FROM status_change_requests
+WHERE requested_status IN ('planning','in_progress','completed')
+ AND approval_state IN ('pending','approved','rejected');
+DROP TABLE status_change_requests;
+ALTER TABLE status_change_requests__new RENAME TO status_change_requests;
+CREATE INDEX status_change_requests_project_id_idx
+ ON status_change_requests(project_id);Also applies to: 1218-1225, 1286-1286
🤖 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 `@drizzle/meta/0012_snapshot.json` around lines 1204 - 1210, The
`requested_status` column and `approval_state` column in the migration snapshot
are defined as unconstrained TEXT fields without database-level CHECK
constraints, which allows invalid states to be stored and diverges from the
constraint definitions in scripts/migrate-portal.ts. Add CHECK constraint
definitions to both the `requested_status` field (around line 1204) and the
`approval_state` field (around line 1218) in the snapshot JSON to restrict these
columns to valid enum values, matching the explicit CHECK clauses used in
scripts/migrate-portal.ts for the status_change_requests table. Update the
constraint section at line 1286 to include these checks for both status fields.
1 participant
Summary
This PR hardens Clientra for production launch: auth signup safety, org-scoped admin access, colleague-invite workflow (admin approval before email), database indexes/migrations, CI quality gate, and targeted security fixes from Greptile review.
Highlights
Auth and data safety
role: clientat signup; block self-assigned admin (input: false+ database hooks)BETTER_AUTH_SECRETin productionseedIfEmptyguardMulti-tenancy / org boundaries
canAccessProject, portal summary, and all admin mutations toactiveOrganizationIdadminOwnsClient,adminOwnsProject,adminOwnsProjectUpdate,adminOwnsProjectMilestone,adminManagesUserColleague invites (ADR 0002 Option A)
src/db/invites.tsDatabase and CI
0011: indexes,status_change_requests, colleague-invite columns, FKON DELETE SET NULLDocs
CLAUDE.md, ADR 0001 (multi-tenancy), ADR 0002 (colleague invites)Test plan
bun run lintbun run typecheckbun run test(120 tests)bun run buildKnown follow-up
workspace_settingsremains a global singleton (no per-org column yet); needs schema work to scope settings by organization.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores