feat: client portal redesign and feature expansion - #18
Conversation
…tivity, team, SCR)
…tion dates, and ui feedback
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 46 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends the client portal with invite approval, project status-change requests, and three new hub pages (Team, Files, Activity). It adds DB schema and migration, DB record functions, server API routes with validation, React Query hooks/mutations, client UI, admin review flows, generated route wiring, and tests. ChangesClient Portal Features: Invites, Status Changes, Files & Team
Sequence DiagramsequenceDiagram
participant Admin
participant ApproveAPI
participant DB
participant Email
Admin->>ApproveAPI: POST /api/invites/:id/approve
ApproveAPI->>DB: approveInviteRecord(id)
DB->>DB: UPDATE invites.adminApprovedAt
DB-->>ApproveAPI: updated invite
ApproveAPI->>Email: sendInviteEmail(inviteUrl)
Email-->>ApproveAPI: success/failure
ApproveAPI->>Admin: JSON { approved invite, emailSent }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6961c1f712
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| await sendInviteEmail({ | ||
| clientCompany: client.company, | ||
| clientName: client.name, | ||
| email: invite.email, | ||
| inviteId: invite.id, | ||
| inviteUrl: inviteUrl.toString(), | ||
| requestUrl: request.url, | ||
| }); |
There was a problem hiding this comment.
When a client requests a colleague invite, this immediately sends the /invite/<token> link even though createPortalColleagueInvite leaves adminApprovedAt null, and the new invite lookup/redeem path rejects client-initiated invites until that field is set. In the normal portal-team flow, recipients receive a link that appears invalid/expired before an admin approves it (the UI also says it will be sent once approved), so the email should be deferred to the approval endpoint instead.
Useful? React with 👍 / 👎.
| const created = await createStatusChangeRequestRecord({ | ||
| id: crypto.randomUUID(), | ||
| projectId: parsed.data.projectId, | ||
| reason: parsed.data.reason, | ||
| requestedBy: auth.user.id, | ||
| requestedStatus: parsed.data.requestedStatus, | ||
| }); |
There was a problem hiding this comment.
The portal form hides after the current query sees a pending request, but this POST path inserts unconditionally. With two portal users or tabs submitting before either cache sees the other's request, the same project can have multiple pending status changes; admins can then approve a stale second request after the first changes the project status. Please check for an existing pending request for this projectId (or enforce it in the database) before creating another one.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR delivers a comprehensive client portal redesign: five new sidebar pages (Overview, Projects, Files, Activity, Team), a full admin approval workflow for client-initiated status change requests and colleague invites, and the database schema/migrations to support them.
Confidence Score: 3/5The core approval flows work correctly, but the invite approval route can fire duplicate emails because the underlying DB function has no guard against re-approving an already-approved invite. The transactional status-change approval is well-constructed and the new portal pages are clean. The invite approval path lacks an idempotency check in src/db/records.ts (approveInviteRecord idempotency) and src/routes/api/portal/status-change-requests.ts (duplicate pending request guard) Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Client Portal
participant PA as /api/portal/…
participant DB as Database
participant AA as /api/admin/…
participant AD as Admin UI
Note over C,DB: Colleague invite flow
C->>PA: POST /api/portal/team (email)
PA->>DB: "createPortalColleagueInvite (adminApprovedAt=null)"
DB-->>PA: invite row
PA-->>C: 201 invite (awaiting approval)
AD->>AA: POST /api/invites/:id/approve
AA->>DB: "UPDATE invites SET adminApprovedAt=now WHERE id AND consumed=null AND revoked=null AND expires>now"
DB-->>AA: updated invite
AA->>AA: sendInviteEmail (best-effort)
AA-->>AD: "200 {emailSent}"
Note over C,DB: Status change request flow
C->>PA: POST /api/portal/status-change-requests
PA->>DB: "INSERT status_change_requests (approvalState=pending)"
DB-->>PA: created request
PA-->>C: 201 request
AD->>AA: PATCH /api/admin/status-change-requests/:id
AA->>DB: getStatusChangeRequestById (pre-check 404/409)
DB-->>AA: request row
AA->>DB: BEGIN TRANSACTION
DB->>DB: "UPDATE request SET approvalState=approved"
DB->>DB: SELECT project (validate transition)
DB->>DB: "UPDATE project SET status=requestedStatus"
DB->>DB: COMMIT
DB-->>AA: updated request
AA-->>AD: 200 updated request
Prompt To Fix All With AIFix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
src/db/records.ts:1754-1775
**Missing idempotency guard on invite approval**
`approveInviteRecord` has no `isNull(invitesTable.adminApprovedAt)` condition in its WHERE clause. A direct POST to `/api/invites/:id/approve` for an already-approved invite will overwrite `adminApprovedAt` with a new timestamp and, because the route unconditionally calls `sendInviteEmail`, trigger a second invite email to the recipient. The admin UI hides the Approve button once `adminApprovedAt` is set, but the API itself is unguarded.
Adding `isNull(invitesTable.adminApprovedAt)` to the WHERE clause would make the function return `null` for any already-approved invite, and the route already turns that into a 404 before the email is sent.
### Issue 2 of 5
src/routes/api/portal/activity.ts:12-13
**Wrong HTTP status for authenticated non-client users**
When a user is authenticated but holds a non-client role (e.g., an admin), the handler returns `unauthorizedError()` (HTTP 401). A 401 tells clients "you are not authenticated — please send credentials", which is incorrect here: the user is authenticated, just not authorized. This should be `forbiddenError()` (HTTP 403) to match the semantics and to be consistent with the parallel role-check in `team.ts` which already uses `forbiddenError`. The same applies to the identical check in `files.ts`.
### Issue 3 of 5
src/routes/api/portal/status-change-requests.ts:40-57
**No server-side guard against duplicate pending requests**
The POST handler creates a new status change request without verifying whether the project already has a pending one. The portal UI does a client-side check (`hasPendingRequest`), but a client making direct API calls can bypass this and create multiple simultaneous pending requests for the same project. An admin could then approve them in sequence — for example, approve "planning → in_progress" and later approve a stale "planning → completed", silently jumping the project two stages in one go. The `isValidTransition` check inside `reviewStatusChangeRequestRecord` only verifies `current status ≠ requested status`, so it would allow the second approval.
Adding a check here (or a unique partial index in the schema) that rejects a new request when `approvalState = 'pending'` already exists for the same `projectId` would close this gap.
### Issue 4 of 5
src/db/records.ts:1756-1768
Add `isNull(invitesTable.adminApprovedAt)` to the WHERE clause so that re-approving an already-approved invite returns `null` instead of overwriting `adminApprovedAt` and re-sending the email.
```suggestion
export async function approveInviteRecord(inviteId: string) {
const [invite] = await db
.update(invitesTable)
.set({ adminApprovedAt: new Date() })
.where(
and(
eq(invitesTable.id, inviteId),
isNull(invitesTable.consumedAt),
isNull(invitesTable.revokedAt),
isNull(invitesTable.adminApprovedAt),
gt(invitesTable.expiresAt, new Date())
)
)
.returning();
```
### Issue 5 of 5
src/routes/api/portal/files.ts:11-12
Return `forbiddenError` (403) when the user is authenticated but not a client, matching the semantics used in `team.ts`. Returning `unauthorizedError` (401) here signals "re-authenticate", which is incorrect for a valid authenticated session with the wrong role.
```suggestion
if (!user) return unauthorizedError();
if (user.role !== "client") return forbiddenError("Client portal only.");
```
Reviews (1): Last reviewed commit: "fix: db transactions, migration error ex..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| export async function approveInviteRecord(inviteId: string) { | ||
| const [invite] = await db | ||
| .update(invitesTable) | ||
| .set({ adminApprovedAt: new Date() }) | ||
| .where( | ||
| and( | ||
| eq(invitesTable.id, inviteId), | ||
| isNull(invitesTable.consumedAt), | ||
| isNull(invitesTable.revokedAt), | ||
| gt(invitesTable.expiresAt, new Date()) | ||
| ) | ||
| ) | ||
| .returning(); | ||
|
|
||
| return invite ? mapInvite(invite) : null; | ||
| } | ||
|
|
||
| export async function revokeInviteRecord(inviteId: string) { | ||
| const [invite] = await db | ||
| .update(invitesTable) |
There was a problem hiding this comment.
approveInviteRecord has no isNull(invitesTable.adminApprovedAt) condition in its WHERE clause. A direct POST to /api/invites/:id/approve for an already-approved invite will overwrite adminApprovedAt with a new timestamp and, because the route unconditionally calls sendInviteEmail, trigger a second invite email to the recipient. The admin UI hides the Approve button once adminApprovedAt is set, but the API itself is unguarded.
Adding isNull(invitesTable.adminApprovedAt) to the WHERE clause would make the function return null for any already-approved invite, and the route already turns that into a 404 before the email is sent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/db/records.ts
Line: 1754-1775
Comment:
**Missing idempotency guard on invite approval**
`approveInviteRecord` has no `isNull(invitesTable.adminApprovedAt)` condition in its WHERE clause. A direct POST to `/api/invites/:id/approve` for an already-approved invite will overwrite `adminApprovedAt` with a new timestamp and, because the route unconditionally calls `sendInviteEmail`, trigger a second invite email to the recipient. The admin UI hides the Approve button once `adminApprovedAt` is set, but the API itself is unguarded.
Adding `isNull(invitesTable.adminApprovedAt)` to the WHERE clause would make the function return `null` for any already-approved invite, and the route already turns that into a 404 before the email is sent.
How can I resolve this? If you propose a fix, please make it concise.| if (user.role !== "client") return unauthorizedError(); | ||
| const activity = await listPortalActivityForUser(user); |
There was a problem hiding this comment.
When a user is authenticated but holds a non-client role (e.g., an admin), the handler returns unauthorizedError() (HTTP 401). A 401 tells clients "you are not authenticated — please send credentials", which is incorrect here: the user is authenticated, just not authorized. This should be forbiddenError() (HTTP 403) to match the semantics and to be consistent with the parallel role-check in team.ts which already uses forbiddenError. The same applies to the identical check in files.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/api/portal/activity.ts
Line: 12-13
Comment:
**Wrong HTTP status for authenticated non-client users**
When a user is authenticated but holds a non-client role (e.g., an admin), the handler returns `unauthorizedError()` (HTTP 401). A 401 tells clients "you are not authenticated — please send credentials", which is incorrect here: the user is authenticated, just not authorized. This should be `forbiddenError()` (HTTP 403) to match the semantics and to be consistent with the parallel role-check in `team.ts` which already uses `forbiddenError`. The same applies to the identical check in `files.ts`.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const parsed = await parseJsonBody(request, statusChangeRequestSchema); | ||
| if (!parsed.ok) return parsed.error; | ||
|
|
||
| const hasAccess = await canAccessProject(auth.user, parsed.data.projectId); | ||
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | ||
|
|
||
| const created = await createStatusChangeRequestRecord({ | ||
| id: crypto.randomUUID(), | ||
| projectId: parsed.data.projectId, | ||
| reason: parsed.data.reason, | ||
| requestedBy: auth.user.id, | ||
| requestedStatus: parsed.data.requestedStatus, | ||
| }); | ||
|
|
||
| if (!created) return internalServerError("Request could not be created."); | ||
|
|
||
| return Response.json(created, { status: 201 }); |
There was a problem hiding this comment.
The POST handler creates a new status change request without verifying whether the project already has a pending one. The portal UI does a client-side check (hasPendingRequest), but a client making direct API calls can bypass this and create multiple simultaneous pending requests for the same project. An admin could then approve them in sequence — for example, approve "planning → in_progress" and later approve a stale "planning → completed", silently jumping the project two stages in one go. The isValidTransition check inside reviewStatusChangeRequestRecord only verifies current status ≠ requested status, so it would allow the second approval.
Adding a check here (or a unique partial index in the schema) that rejects a new request when approvalState = 'pending' already exists for the same projectId would close this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/api/portal/status-change-requests.ts
Line: 40-57
Comment:
**No server-side guard against duplicate pending requests**
The POST handler creates a new status change request without verifying whether the project already has a pending one. The portal UI does a client-side check (`hasPendingRequest`), but a client making direct API calls can bypass this and create multiple simultaneous pending requests for the same project. An admin could then approve them in sequence — for example, approve "planning → in_progress" and later approve a stale "planning → completed", silently jumping the project two stages in one go. The `isValidTransition` check inside `reviewStatusChangeRequestRecord` only verifies `current status ≠ requested status`, so it would allow the second approval.
Adding a check here (or a unique partial index in the schema) that rejects a new request when `approvalState = 'pending'` already exists for the same `projectId` would close this gap.
How can I resolve this? If you propose a fix, please make it concise.| export async function approveInviteRecord(inviteId: string) { | ||
| const [invite] = await db | ||
| .update(invitesTable) | ||
| .set({ adminApprovedAt: new Date() }) | ||
| .where( | ||
| and( | ||
| eq(invitesTable.id, inviteId), | ||
| isNull(invitesTable.consumedAt), | ||
| isNull(invitesTable.revokedAt), | ||
| gt(invitesTable.expiresAt, new Date()) | ||
| ) | ||
| ) | ||
| .returning(); |
There was a problem hiding this comment.
isNull(invitesTable.adminApprovedAt) to the WHERE clause so that re-approving an already-approved invite returns null instead of overwriting adminApprovedAt and re-sending the email.
| export async function approveInviteRecord(inviteId: string) { | |
| const [invite] = await db | |
| .update(invitesTable) | |
| .set({ adminApprovedAt: new Date() }) | |
| .where( | |
| and( | |
| eq(invitesTable.id, inviteId), | |
| isNull(invitesTable.consumedAt), | |
| isNull(invitesTable.revokedAt), | |
| gt(invitesTable.expiresAt, new Date()) | |
| ) | |
| ) | |
| .returning(); | |
| export async function approveInviteRecord(inviteId: string) { | |
| const [invite] = await db | |
| .update(invitesTable) | |
| .set({ adminApprovedAt: new Date() }) | |
| .where( | |
| and( | |
| eq(invitesTable.id, inviteId), | |
| isNull(invitesTable.consumedAt), | |
| isNull(invitesTable.revokedAt), | |
| isNull(invitesTable.adminApprovedAt), | |
| gt(invitesTable.expiresAt, new Date()) | |
| ) | |
| ) | |
| .returning(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/db/records.ts
Line: 1756-1768
Comment:
Add `isNull(invitesTable.adminApprovedAt)` to the WHERE clause so that re-approving an already-approved invite returns `null` instead of overwriting `adminApprovedAt` and re-sending the email.
```suggestion
export async function approveInviteRecord(inviteId: string) {
const [invite] = await db
.update(invitesTable)
.set({ adminApprovedAt: new Date() })
.where(
and(
eq(invitesTable.id, inviteId),
isNull(invitesTable.consumedAt),
isNull(invitesTable.revokedAt),
isNull(invitesTable.adminApprovedAt),
gt(invitesTable.expiresAt, new Date())
)
)
.returning();
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (!user) return unauthorizedError(); | ||
| if (user.role !== "client") return unauthorizedError(); |
There was a problem hiding this comment.
forbiddenError (403) when the user is authenticated but not a client, matching the semantics used in team.ts. Returning unauthorizedError (401) here signals "re-authenticate", which is incorrect for a valid authenticated session with the wrong role.
| if (!user) return unauthorizedError(); | |
| if (user.role !== "client") return unauthorizedError(); | |
| if (!user) return unauthorizedError(); | |
| if (user.role !== "client") return forbiddenError("Client portal only."); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/api/portal/files.ts
Line: 11-12
Comment:
Return `forbiddenError` (403) when the user is authenticated but not a client, matching the semantics used in `team.ts`. Returning `unauthorizedError` (401) here signals "re-authenticate", which is incorrect for a valid authenticated session with the wrong role.
```suggestion
if (!user) return unauthorizedError();
if (user.role !== "client") return forbiddenError("Client portal only.");
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
src/routes/api/invites/$id/approve.ts (1)
37-50: ⚡ Quick winConsider adding observability for failed invite emails.
The endpoint gracefully handles email failures by returning
emailSent: false, which is good for resilience. However, failed emails are only logged to console. Consider adding structured logging or metrics to track email delivery failures for operational monitoring.🤖 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/approve.ts around lines 37 - 50, The catch block for sendInviteEmail currently only console.error's and flips emailSent to false; add structured observability by logging the failure with the app logger and including contextual fields (invite id/email/client/company/requestUrl) and/or incrementing a metric/telemetry counter so failures are tracked; update the catch to call your structured logger (e.g., processLogger.error or telemetry.record) with message like "invite approval email failed" plus { inviteId: approvedInvite.id, email: approvedInvite.email, clientCompany: client.company, clientName: client.name, requestUrl: request.url } and/or call a metric function (e.g., metrics.increment('invite_email_failure')) so operators can monitor sendInviteEmail failures.src/lib/api.ts (1)
1591-1637: 💤 Low valueConsider invalidating
portalSummaryafter SCR approval.When a status change request is approved, the project status changes. The
portalSummaryquery includesactiveProjectsfiltered by status, so approving an SCR that marks a project "completed" could leave stale data in the portal home page until the user navigates away.♻️ Optional enhancement
export function useReviewStatusChangeRequestMutation() { const queryClient = useQueryClient(); return useMutation({ mutationFn: reviewStatusChangeRequestFn, onSuccess: (updated) => { // Remove from admin pending list queryClient.invalidateQueries({ queryKey: queryKeys.adminStatusChangeRequests }); // Update per-project SCR list queryClient.setQueryData<StatusChangeRequest[]>( queryKeys.portalStatusChangeRequests(updated.projectId), (current) => (current ?? []).map((r) => (r.id === updated.id ? updated : r)) ); // Refresh projects so updated status reflects queryClient.invalidateQueries({ queryKey: queryKeys.projects }); + // Refresh portal summary in case active projects changed + queryClient.invalidateQueries({ queryKey: queryKeys.portalSummary }); }, }); }🤖 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/lib/api.ts` around lines 1591 - 1637, When approving/rejecting an SCR the portal home summary can become stale; update useReviewStatusChangeRequestMutation's onSuccess (the function using reviewStatusChangeRequestFn) to also invalidate the portal summary query by calling queryClient.invalidateQueries with queryKeys.portalSummary so the portalSummary (which includes activeProjects filtered by status) is refreshed after an SCR is processed.src/routes/api/admin/status-change-requests.ts (1)
10-18: ⚡ Quick winUse
forbiddenErrorhelper for consistency.Line 14 manually constructs the 403 response. The codebase provides
forbiddenError(message)for this purpose (imported on line 4 from route-utils), which ensures consistent error shape and status codes across endpoints.♻️ Proposed refactor
const auth = await requireSessionRequest(request); if (auth.error) return auth.error; -if (auth.user.role !== "admin") { - return Response.json({ error: "Admin only." }, { status: 403 }); -} +if (auth.user.role !== "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 10 - 18, The GET handler currently returns a manually constructed 403 Response.json when the user is not an admin; replace that manual response with the project's helper by calling forbiddenError("Admin only.") instead of Response.json(...) inside the GET async function (where requireSessionRequest, listAllPendingStatusChangeRequests are used) to ensure consistent error shape and status code; confirm the existing import of forbiddenError (from route-utils) is used and remove the manual Response.json-only 403 branch.src/components/layout/portal-shell.tsx (1)
61-69: ⚖️ Poor tradeoffConsider selective cache invalidation instead of clearing all queries.
queryClient.clear()removes all cached data and cancels all active queries, including potentially in-flight mutations. If a user triggers sign-out while another request is pending, this could cause unexpected behavior or race conditions.Consider using
queryClient.removeQueries()with a predicate to clear only user-specific data, or rely on the server session invalidation and natural cache expiration.♻️ Alternative approach using selective invalidation
const handleSignOut = async () => { try { await authClient.signOut(); - queryClient.clear(); + // Clear user-specific queries but preserve non-sensitive cached data + queryClient.removeQueries({ + predicate: (query) => { + const key = query.queryKey[0]; + return typeof key === 'string' && + (key.startsWith('portal') || key.startsWith('user') || key === 'session'); + } + }); await router.navigate({ to: "/login" });🤖 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/components/layout/portal-shell.tsx` around lines 61 - 69, The sign-out handler (handleSignOut) uses queryClient.clear() which wipes all cached queries and cancels in-flight requests; replace this with selective invalidation/removal of only user/session-scoped data (e.g., use queryClient.removeQueries or queryClient.invalidateQueries with a predicate or specific query keys for user/profile/auth data) so global caches and unrelated in-flight mutations are preserved, then proceed with authClient.signOut() and router.navigate as before.src/routes/projects/$id.tsx (2)
463-483: ⚡ Quick winRemove duplicate cache invalidations already handled by mutation.
The
useReviewStatusChangeRequestMutationhook'sonSuccesscallback already invalidates bothqueryKeys.projectsand updatesqueryKeys.portalStatusChangeRequests(projectId)optimistically. Lines 474–475 duplicate this work, discarding the optimistic update and causing an unnecessary refetch.♻️ Recommended: Remove redundant invalidations
type: "success", message: `Request ${approvalState === "approved" ? "approved" : "rejected"} successfully.`, }); - queryClient.invalidateQueries({ queryKey: queryKeys.projects }); - queryClient.invalidateQueries({ queryKey: queryKeys.portalStatusChangeRequests(project.id) }); } catch (err) {The mutation's
onSuccess(defined insrc/lib/api.ts) already handles all necessary cache updates.🤖 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/projects/`$id.tsx around lines 463 - 483, The handleReviewRequest function is redundantly invalidating caches after calling reviewMutation.mutateAsync—remove the two queryClient.invalidateQueries calls that reference queryKeys.projects and queryKeys.portalStatusChangeRequests(project.id) so the mutation's onSuccess in useReviewStatusChangeRequestMutation can perform the intended optimistic update and cache updates (leave reviewMutation.mutateAsync, setNotification and error handling intact).
637-638: ⚡ Quick winPrefer
formatStatusLabelfor consistency and robustness.Lines 637–638 use
.replace("_", " ")to format status values, but the file already imports and usesformatStatusLabel(line 238) for the same purpose. Using the utility function ensures consistent formatting and handles edge cases correctly.♻️ Recommended: Use the existing utility
<p className="text-muted-foreground text-xs leading-relaxed"> - Client requested to change status from <span className="font-bold capitalize">{project.status.replace("_", " ")}</span> to{" "} - <span className="font-bold capitalize">{pendingStatusRequest.requestedStatus.replace("_", " ")}</span>. + Client requested to change status from <span className="font-bold">{formatStatusLabel(project.status)}</span> to{" "} + <span className="font-bold">{formatStatusLabel(pendingStatusRequest.requestedStatus)}</span>. </p>Note: Remove the
capitalizeclass sinceformatStatusLabelalready handles capitalization.🤖 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/projects/`$id.tsx around lines 637 - 638, The displayed status strings currently call .replace("_", " ") on project.status and pendingStatusRequest.requestedStatus; replace those usages with the existing utility formatStatusLabel(project.status) and formatStatusLabel(pendingStatusRequest.requestedStatus) to ensure consistent formatting, and remove the now-redundant "capitalize" class on those span elements; update the spans that reference project.status and pendingStatusRequest.requestedStatus so they call formatStatusLabel instead of .replace.
🤖 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 `@src/components/layout/portal-shell.tsx`:
- Around line 66-68: In PortalShell, the catch block that currently just
console.error(e) should set component state to surface the failure to the user:
add a state variable signOutError (and setter setSignOutError) and in the catch
assign signOutError to e?.message || String(e) (while still logging); then
update the account dropdown render path to display signOutError as a visible
error message or banner and offer a retry action so users get immediate feedback
if sign-out or navigation fails.
In `@src/routes/api/portal/activity.ts`:
- Around line 10-12: The role check in activity route uses unauthorizedError()
for an authenticated user with the wrong role; replace the second return
unauthorizedError() with forbiddenError("Client portal only.") so role-based
failures return HTTP 403; locate the user retrieval and role check around
getSessionUserFromHeaders(...) and change the return for user.role !== "client"
to call forbiddenError with the same message used in /api/portal/team.
In `@src/routes/api/portal/files.ts`:
- Around line 10-12: The role check in the handler uses unauthorizedError() for
authenticated-but-unauthorized users; update the authorization branch to return
forbiddenError(...) instead of unauthorizedError() so role mismatches produce
HTTP 403. Locate the user check around getSessionUserFromHeaders(...) and
replace the second unauthorizedError() call with forbiddenError("Client portal
only.") (matching the existing pattern used in /api/portal/team) to ensure
consistent role-based authorization handling.
In `@src/routes/api/portal/status-change-requests.ts`:
- Around line 19-35: The GET handler currently only checks project access but
not that the requester is a portal client, so add a role check after
requireSessionRequest succeeds: verify auth.user.role === "client" and if not
return forbiddenError("You do not have access to this project.") (or a similar
client-only message) before calling canAccessProject; keep the existing
requireSessionRequest, canAccessProject and listStatusChangeRequestsForProject
calls and only proceed to listStatusChangeRequestsForProject(projectId) when the
role check and canAccessProject both pass.
- Around line 37-58: The POST handler currently checks project access but not
the caller's role, allowing admins to call a portal-only endpoint; update the
POST handler in this file to enforce that auth.user.role === "client" (similar
to the checks in /api/portal/team), e.g., after requireMutationSessionRequest
and before creating the record validate the role and return forbiddenError("You
do not have access to this resource.") if not a client; keep the existing
canAccessProject check and then proceed with createStatusChangeRequestRecord
only when both role and project access pass.
In `@src/routes/portal/activity.tsx`:
- Around line 60-68: The ActivityCard component uses unsafe casts of item.data
to ProjectComment/ProjectUpdate/ProjectFile which can crash at runtime; add
runtime type guard functions (e.g., isProjectComment, isProjectUpdate, reuse
isProjectFile from src/routes/portal/files.tsx) that validate the shape of
item.data and then use those guards before assigning comment/update/file
(replace lines that cast item.data with guarded checks using the new
isProjectComment/isProjectUpdate/isProjectFile functions), so downstream
accesses like comment.content and update.status are only performed when the
guard returns true.
In `@src/routes/portal/files.tsx`:
- Around line 91-108: In the onClientUploadComplete callback of useUploadThing
(inside the onClientUploadComplete handler), detect when
isProjectFile(entry.serverData) returns false and surface that to the user
instead of silently skipping: collect invalid entries while iterating uploaded,
log a clear warning (e.g., console.warn with entry identifiers) and call the
existing setUploadError or trigger a toast/notification indicating which
uploaded items failed validation (include file name or id from the upload
entry), and ensure queryClient.setQueryData only receives the validated
newFiles; make these changes in the onClientUploadComplete block where newFiles
is built and queryClient is updated.
In `@src/routes/portal/projects/`$id.tsx:
- Around line 699-815: Change the requestedStatus state to initialize as an
empty string (useState<ProjectStatusValue | "">("")) and update the select
(id="requested-status") to include a placeholder option like <option
value="">Select new status…</option>; keep PROJECT_STATUSES rendering but leave
each option disabled when s.value === currentStatus. Update the onChange to cast
to ProjectStatusValue | "" via setRequestedStatus(e.target.value as
ProjectStatusValue | ""). In handleSubmit, adjust the validation to reject when
requestedStatus === "" or requestedStatus === currentStatus so the form requires
the user to pick a different non-empty status before submitting.
In `@src/routes/portal/team.tsx`:
- Around line 80-83: Update the DialogDescription text in the invite dialog
(components DialogTitle / DialogDescription) to state that the invite email is
sent only after admin approval so users aren’t misled into thinking delivery is
immediate; locate the invite flow where the invite submission UI (the invite
form and send-invite action that references the approval step) is handled and
replace the current sentence "The invite will be delivered via email." with a
short clarification such as "The invite will be sent by email once an admin
approves the request," ensuring the new copy appears alongside the existing
DialogTitle "Invite a colleague."
In `@src/routes/projects/`$id.tsx:
- Line 428: The code assumes a single pending status change request by using
statusRequests.find; change this to collect all pending requests (e.g., const
pendingStatusRequests = statusRequests.filter(r => r.approvalState ===
"pending")) and derive hasPendingRequest as pendingStatusRequests.length > 0;
update any usage of pendingStatusRequest (rendering, toggles, forms) to handle
multiple items (iterate or show a summary) so the UI no longer hides/adds the
form incorrectly when more than one pending SCR exists and ensure variable names
(pendingStatusRequests, hasPendingRequest) replace the old identifier where
used.
---
Nitpick comments:
In `@src/components/layout/portal-shell.tsx`:
- Around line 61-69: The sign-out handler (handleSignOut) uses
queryClient.clear() which wipes all cached queries and cancels in-flight
requests; replace this with selective invalidation/removal of only
user/session-scoped data (e.g., use queryClient.removeQueries or
queryClient.invalidateQueries with a predicate or specific query keys for
user/profile/auth data) so global caches and unrelated in-flight mutations are
preserved, then proceed with authClient.signOut() and router.navigate as before.
In `@src/lib/api.ts`:
- Around line 1591-1637: When approving/rejecting an SCR the portal home summary
can become stale; update useReviewStatusChangeRequestMutation's onSuccess (the
function using reviewStatusChangeRequestFn) to also invalidate the portal
summary query by calling queryClient.invalidateQueries with
queryKeys.portalSummary so the portalSummary (which includes activeProjects
filtered by status) is refreshed after an SCR is processed.
In `@src/routes/api/admin/status-change-requests.ts`:
- Around line 10-18: The GET handler currently returns a manually constructed
403 Response.json when the user is not an admin; replace that manual response
with the project's helper by calling forbiddenError("Admin only.") instead of
Response.json(...) inside the GET async function (where requireSessionRequest,
listAllPendingStatusChangeRequests are used) to ensure consistent error shape
and status code; confirm the existing import of forbiddenError (from
route-utils) is used and remove the manual Response.json-only 403 branch.
In `@src/routes/api/invites/`$id/approve.ts:
- Around line 37-50: The catch block for sendInviteEmail currently only
console.error's and flips emailSent to false; add structured observability by
logging the failure with the app logger and including contextual fields (invite
id/email/client/company/requestUrl) and/or incrementing a metric/telemetry
counter so failures are tracked; update the catch to call your structured logger
(e.g., processLogger.error or telemetry.record) with message like "invite
approval email failed" plus { inviteId: approvedInvite.id, email:
approvedInvite.email, clientCompany: client.company, clientName: client.name,
requestUrl: request.url } and/or call a metric function (e.g.,
metrics.increment('invite_email_failure')) so operators can monitor
sendInviteEmail failures.
In `@src/routes/projects/`$id.tsx:
- Around line 463-483: The handleReviewRequest function is redundantly
invalidating caches after calling reviewMutation.mutateAsync—remove the two
queryClient.invalidateQueries calls that reference queryKeys.projects and
queryKeys.portalStatusChangeRequests(project.id) so the mutation's onSuccess in
useReviewStatusChangeRequestMutation can perform the intended optimistic update
and cache updates (leave reviewMutation.mutateAsync, setNotification and error
handling intact).
- Around line 637-638: The displayed status strings currently call .replace("_",
" ") on project.status and pendingStatusRequest.requestedStatus; replace those
usages with the existing utility formatStatusLabel(project.status) and
formatStatusLabel(pendingStatusRequest.requestedStatus) to ensure consistent
formatting, and remove the now-redundant "capitalize" class on those span
elements; update the spans that reference project.status and
pendingStatusRequest.requestedStatus so they call formatStatusLabel instead of
.replace.
🪄 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: 93852541-07b0-40f7-81bb-414a5b009362
📒 Files selected for processing (25)
scripts/migrate-portal.tssrc/__tests__/api-invite-management.test.tssrc/__tests__/api-pending-invites.test.tssrc/__tests__/client-pending-invites.test.tsxsrc/api/validation.tssrc/components/layout/portal-shell.tsxsrc/db/records.tssrc/db/schema.tssrc/lib/api.tssrc/routeTree.gen.tssrc/routes/api/admin/status-change-requests.tssrc/routes/api/admin/status-change-requests/$id.tssrc/routes/api/clients/$id/invites.tssrc/routes/api/invites/$id/approve.tssrc/routes/api/portal/activity.tssrc/routes/api/portal/files.tssrc/routes/api/portal/status-change-requests.tssrc/routes/api/portal/team.tssrc/routes/clients/$id.tsxsrc/routes/portal/activity.tsxsrc/routes/portal/files.tsxsrc/routes/portal/index.tsxsrc/routes/portal/projects/$id.tsxsrc/routes/portal/team.tsxsrc/routes/projects/$id.tsx
| } catch (e) { | ||
| console.error(e); | ||
| } |
There was a problem hiding this comment.
Provide user feedback when sign-out fails.
Errors during sign-out are caught and logged to the console, but the user receives no visual feedback. If the sign-out request fails or navigation is blocked, the user may remain on the portal page without realizing the operation failed.
🛡️ Suggested improvement
+ const [signOutError, setSignOutError] = useState<string | null>(null);
+
const handleSignOut = async () => {
+ setSignOutError(null);
try {
await authClient.signOut();
queryClient.clear();
await router.navigate({ to: "/login" });
} catch (e) {
console.error(e);
+ setSignOutError(e instanceof Error ? e.message : "Failed to sign out");
}
};Then render the error in the dropdown:
<DropdownMenuSeparator />
+ {signOutError && (
+ <div className="px-2 py-1.5 text-rose-600 text-xs">
+ {signOutError}
+ </div>
+ )}
<DropdownMenuItem className="cursor-pointer" onClick={handleSignOut}>🤖 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/components/layout/portal-shell.tsx` around lines 66 - 68, In PortalShell,
the catch block that currently just console.error(e) should set component state
to surface the failure to the user: add a state variable signOutError (and
setter setSignOutError) and in the catch assign signOutError to e?.message ||
String(e) (while still logging); then update the account dropdown render path to
display signOutError as a visible error message or banner and offer a retry
action so users get immediate feedback if sign-out or navigation fails.
| const user = await getSessionUserFromHeaders(request.headers); | ||
| if (!user) return unauthorizedError(); | ||
| if (user.role !== "client") return unauthorizedError(); |
There was a problem hiding this comment.
Use forbiddenError for role-based authorization, not unauthorizedError.
Line 12 returns HTTP 401 for a role mismatch, but the user is authenticated—they simply lack the "client" role. HTTP 403 (forbidden) is the correct status for authorization failures. /api/portal/team (line 25) uses forbiddenError("Client portal only.") for the identical check.
🔧 Proposed fix
const user = await getSessionUserFromHeaders(request.headers);
if (!user) return unauthorizedError();
-if (user.role !== "client") return unauthorizedError();
+if (user.role !== "client") return forbiddenError("Client portal only.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const user = await getSessionUserFromHeaders(request.headers); | |
| if (!user) return unauthorizedError(); | |
| if (user.role !== "client") return unauthorizedError(); | |
| const user = await getSessionUserFromHeaders(request.headers); | |
| if (!user) return unauthorizedError(); | |
| if (user.role !== "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/activity.ts` around lines 10 - 12, The role check in
activity route uses unauthorizedError() for an authenticated user with the wrong
role; replace the second return unauthorizedError() with forbiddenError("Client
portal only.") so role-based failures return HTTP 403; locate the user retrieval
and role check around getSessionUserFromHeaders(...) and change the return for
user.role !== "client" to call forbiddenError with the same message used in
/api/portal/team.
| const user = await getSessionUserFromHeaders(request.headers); | ||
| if (!user) return unauthorizedError(); | ||
| if (user.role !== "client") return unauthorizedError(); |
There was a problem hiding this comment.
Use forbiddenError for role-based authorization, not unauthorizedError.
Line 12 returns HTTP 401 for a role mismatch, but the user is authenticated—they simply lack the "client" role. HTTP 403 (forbidden) is the correct status for authorization failures. /api/portal/team (line 25) uses forbiddenError("Client portal only.") for the identical check.
🔧 Proposed fix
const user = await getSessionUserFromHeaders(request.headers);
if (!user) return unauthorizedError();
-if (user.role !== "client") return unauthorizedError();
+if (user.role !== "client") return forbiddenError("Client portal only.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const user = await getSessionUserFromHeaders(request.headers); | |
| if (!user) return unauthorizedError(); | |
| if (user.role !== "client") return unauthorizedError(); | |
| const user = await getSessionUserFromHeaders(request.headers); | |
| if (!user) return unauthorizedError(); | |
| if (user.role !== "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/files.ts` around lines 10 - 12, The role check in the
handler uses unauthorizedError() for authenticated-but-unauthorized users;
update the authorization branch to return forbiddenError(...) instead of
unauthorizedError() so role mismatches produce HTTP 403. Locate the user check
around getSessionUserFromHeaders(...) and replace the second unauthorizedError()
call with forbiddenError("Client portal only.") (matching the existing pattern
used in /api/portal/team) to ensure consistent role-based authorization
handling.
| GET: async ({ request }) => { | ||
| const auth = await requireSessionRequest(request); | ||
| if (auth.error) return auth.error; | ||
|
|
||
| const url = new URL(request.url); | ||
| const projectId = url.searchParams.get("projectId"); | ||
|
|
||
| if (!projectId) { | ||
| return Response.json({ error: "projectId is required." }, { status: 400 }); | ||
| } | ||
|
|
||
| const hasAccess = await canAccessProject(auth.user, projectId); | ||
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | ||
|
|
||
| const requests = await listStatusChangeRequestsForProject(projectId); | ||
| return Response.json(requests); | ||
| }, |
There was a problem hiding this comment.
Add client-role authorization check.
The GET handler verifies project access but never checks that auth.user.role === "client". canAccessProject returns true for admins, so an admin user could call this portal-specific endpoint. Other portal routes (e.g., /api/portal/team lines 25, 33) enforce role === "client".
🔒 Proposed fix
GET: async ({ request }) => {
const auth = await requireSessionRequest(request);
if (auth.error) return auth.error;
+ if (auth.user.role !== "client") return forbiddenError("Client portal only.");
const url = new URL(request.url);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GET: async ({ request }) => { | |
| const auth = await requireSessionRequest(request); | |
| if (auth.error) return auth.error; | |
| const url = new URL(request.url); | |
| const projectId = url.searchParams.get("projectId"); | |
| if (!projectId) { | |
| return Response.json({ error: "projectId is required." }, { status: 400 }); | |
| } | |
| const hasAccess = await canAccessProject(auth.user, projectId); | |
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | |
| const requests = await listStatusChangeRequestsForProject(projectId); | |
| return Response.json(requests); | |
| }, | |
| GET: async ({ request }) => { | |
| const auth = await requireSessionRequest(request); | |
| if (auth.error) return auth.error; | |
| if (auth.user.role !== "client") return forbiddenError("Client portal only."); | |
| const url = new URL(request.url); | |
| const projectId = url.searchParams.get("projectId"); | |
| if (!projectId) { | |
| return Response.json({ error: "projectId is required." }, { status: 400 }); | |
| } | |
| const hasAccess = await canAccessProject(auth.user, projectId); | |
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | |
| const requests = await listStatusChangeRequestsForProject(projectId); | |
| return Response.json(requests); | |
| }, |
🤖 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/status-change-requests.ts` around lines 19 - 35, The
GET handler currently only checks project access but not that the requester is a
portal client, so add a role check after requireSessionRequest succeeds: verify
auth.user.role === "client" and if not return forbiddenError("You do not have
access to this project.") (or a similar client-only message) before calling
canAccessProject; keep the existing requireSessionRequest, canAccessProject and
listStatusChangeRequestsForProject calls and only proceed to
listStatusChangeRequestsForProject(projectId) when the role check and
canAccessProject both pass.
| POST: async ({ request }) => { | ||
| const auth = await requireMutationSessionRequest(request); | ||
| if (auth.error) return auth.error; | ||
|
|
||
| const parsed = await parseJsonBody(request, statusChangeRequestSchema); | ||
| if (!parsed.ok) return parsed.error; | ||
|
|
||
| const hasAccess = await canAccessProject(auth.user, parsed.data.projectId); | ||
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | ||
|
|
||
| const created = await createStatusChangeRequestRecord({ | ||
| id: crypto.randomUUID(), | ||
| projectId: parsed.data.projectId, | ||
| reason: parsed.data.reason, | ||
| requestedBy: auth.user.id, | ||
| requestedStatus: parsed.data.requestedStatus, | ||
| }); | ||
|
|
||
| if (!created) return internalServerError("Request could not be created."); | ||
|
|
||
| return Response.json(created, { status: 201 }); | ||
| }, |
There was a problem hiding this comment.
Add client-role authorization check.
The POST handler verifies project access but never checks that auth.user.role === "client". canAccessProject returns true for admins, so an admin user could call this portal-specific endpoint. Other portal routes (e.g., /api/portal/team lines 25, 33) enforce role === "client".
🔒 Proposed fix
POST: async ({ request }) => {
const auth = await requireMutationSessionRequest(request);
if (auth.error) return auth.error;
+ if (auth.user.role !== "client") return forbiddenError("Client portal only.");
const parsed = await parseJsonBody(request, statusChangeRequestSchema);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| POST: async ({ request }) => { | |
| const auth = await requireMutationSessionRequest(request); | |
| if (auth.error) return auth.error; | |
| const parsed = await parseJsonBody(request, statusChangeRequestSchema); | |
| if (!parsed.ok) return parsed.error; | |
| const hasAccess = await canAccessProject(auth.user, parsed.data.projectId); | |
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | |
| const created = await createStatusChangeRequestRecord({ | |
| id: crypto.randomUUID(), | |
| projectId: parsed.data.projectId, | |
| reason: parsed.data.reason, | |
| requestedBy: auth.user.id, | |
| requestedStatus: parsed.data.requestedStatus, | |
| }); | |
| if (!created) return internalServerError("Request could not be created."); | |
| return Response.json(created, { status: 201 }); | |
| }, | |
| POST: async ({ request }) => { | |
| const auth = await requireMutationSessionRequest(request); | |
| if (auth.error) return auth.error; | |
| if (auth.user.role !== "client") return forbiddenError("Client portal only."); | |
| const parsed = await parseJsonBody(request, statusChangeRequestSchema); | |
| if (!parsed.ok) return parsed.error; | |
| const hasAccess = await canAccessProject(auth.user, parsed.data.projectId); | |
| if (!hasAccess) return forbiddenError("You do not have access to this project."); | |
| const created = await createStatusChangeRequestRecord({ | |
| id: crypto.randomUUID(), | |
| projectId: parsed.data.projectId, | |
| reason: parsed.data.reason, | |
| requestedBy: auth.user.id, | |
| requestedStatus: parsed.data.requestedStatus, | |
| }); | |
| if (!created) return internalServerError("Request could not be created."); | |
| return Response.json(created, { status: 201 }); | |
| }, |
🤖 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/status-change-requests.ts` around lines 37 - 58, The
POST handler currently checks project access but not the caller's role, allowing
admins to call a portal-only endpoint; update the POST handler in this file to
enforce that auth.user.role === "client" (similar to the checks in
/api/portal/team), e.g., after requireMutationSessionRequest and before creating
the record validate the role and return forbiddenError("You do not have access
to this resource.") if not a client; keep the existing canAccessProject check
and then proceed with createStatusChangeRequestRecord only when both role and
project access pass.
| function ActivityCard({ item }: { item: PortalActivityItem }) { | ||
| const isComment = item.type === "comment"; | ||
| const isUpdate = item.type === "update"; | ||
| const isFile = item.type === "file"; | ||
|
|
||
| const comment = isComment ? (item.data as ProjectComment) : null; | ||
| const update = isUpdate ? (item.data as ProjectUpdate) : null; | ||
| const file = isFile ? (item.data as ProjectFile) : null; | ||
|
|
There was a problem hiding this comment.
Replace unsafe type assertions with runtime type guards.
Lines 65-67 use as type assertions to cast item.data without validation. If the API returns data that doesn't match ProjectComment, ProjectUpdate, or ProjectFile, this will cause runtime errors when accessing properties like comment.content (line 131) or update.status (line 105).
Add runtime type guards similar to isProjectFile in src/routes/portal/files.tsx (lines 68-80) to safely validate the data shape before casting.
🔒 Recommended fix with type guards
Add type guards above the ActivityCard component:
+function isProjectComment(value: unknown): value is ProjectComment {
+ if (!value || typeof value !== "object") return false;
+ const c = value as Record<string, unknown>;
+ return (
+ typeof c.id === "string" &&
+ typeof c.content === "string" &&
+ typeof c.authorName === "string"
+ );
+}
+
+function isProjectUpdate(value: unknown): value is ProjectUpdate {
+ if (!value || typeof value !== "object") return false;
+ const u = value as Record<string, unknown>;
+ return (
+ typeof u.id === "string" &&
+ typeof u.title === "string" &&
+ typeof u.status === "string"
+ );
+}
+
+function isProjectFile(value: unknown): value is ProjectFile {
+ if (!value || typeof value !== "object") return false;
+ const f = value as Record<string, unknown>;
+ return (
+ typeof f.id === "string" &&
+ typeof f.fileName === "string" &&
+ typeof f.fileUrl === "string"
+ );
+}
+
function ActivityCard({ item }: { item: PortalActivityItem }) {
const isComment = item.type === "comment";
const isUpdate = item.type === "update";
const isFile = item.type === "file";
- const comment = isComment ? (item.data as ProjectComment) : null;
- const update = isUpdate ? (item.data as ProjectUpdate) : null;
- const file = isFile ? (item.data as ProjectFile) : null;
+ const comment = isComment && isProjectComment(item.data) ? item.data : null;
+ const update = isUpdate && isProjectUpdate(item.data) ? item.data : null;
+ const file = isFile && isProjectFile(item.data) ? item.data : null;
+
+ // Skip rendering if data doesn't match expected shape
+ if ((isComment && !comment) || (isUpdate && !update) || (isFile && !file)) {
+ console.warn("Activity item has unexpected data shape:", item);
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function ActivityCard({ item }: { item: PortalActivityItem }) { | |
| const isComment = item.type === "comment"; | |
| const isUpdate = item.type === "update"; | |
| const isFile = item.type === "file"; | |
| const comment = isComment ? (item.data as ProjectComment) : null; | |
| const update = isUpdate ? (item.data as ProjectUpdate) : null; | |
| const file = isFile ? (item.data as ProjectFile) : null; | |
| function isProjectComment(value: unknown): value is ProjectComment { | |
| if (!value || typeof value !== "object") return false; | |
| const c = value as Record<string, unknown>; | |
| return ( | |
| typeof c.id === "string" && | |
| typeof c.content === "string" && | |
| typeof c.authorName === "string" | |
| ); | |
| } | |
| function isProjectUpdate(value: unknown): value is ProjectUpdate { | |
| if (!value || typeof value !== "object") return false; | |
| const u = value as Record<string, unknown>; | |
| return ( | |
| typeof u.id === "string" && | |
| typeof u.title === "string" && | |
| typeof u.status === "string" | |
| ); | |
| } | |
| function isProjectFile(value: unknown): value is ProjectFile { | |
| if (!value || typeof value !== "object") return false; | |
| const f = value as Record<string, unknown>; | |
| return ( | |
| typeof f.id === "string" && | |
| typeof f.fileName === "string" && | |
| typeof f.fileUrl === "string" | |
| ); | |
| } | |
| function ActivityCard({ item }: { item: PortalActivityItem }) { | |
| const isComment = item.type === "comment"; | |
| const isUpdate = item.type === "update"; | |
| const isFile = item.type === "file"; | |
| const comment = isComment && isProjectComment(item.data) ? item.data : null; | |
| const update = isUpdate && isProjectUpdate(item.data) ? item.data : null; | |
| const file = isFile && isProjectFile(item.data) ? item.data : null; | |
| // Skip rendering if data doesn't match expected shape | |
| if ((isComment && !comment) || (isUpdate && !update) || (isFile && !file)) { | |
| console.warn("Activity item has unexpected data shape:", item); | |
| return null; | |
| } |
🤖 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/portal/activity.tsx` around lines 60 - 68, The ActivityCard
component uses unsafe casts of item.data to
ProjectComment/ProjectUpdate/ProjectFile which can crash at runtime; add runtime
type guard functions (e.g., isProjectComment, isProjectUpdate, reuse
isProjectFile from src/routes/portal/files.tsx) that validate the shape of
item.data and then use those guards before assigning comment/update/file
(replace lines that cast item.data with guarded checks using the new
isProjectComment/isProjectUpdate/isProjectFile functions), so downstream
accesses like comment.content and update.status are only performed when the
guard returns true.
…r, signout cache cleanup, and multiple pending requests UI
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 `@src/components/layout/portal-shell.tsx`:
- Around line 64-69: The allowlist used in the queryClient.removeQueries call in
portal-shell.tsx only clears some query prefixes and leaves other authenticated
keys (e.g., projects, project-files, project-updates, pending-invites,
dashboard-activity, admin-status-change-requests) intact; replace the
removeQueries block with a full cache clear (call queryClient.clear()) or
extract a shared auth-cache predicate used by both portal-shell.tsx and
app-shell.tsx so signing out fully removes all authenticated queries instead of
relying on the current predicate that checks query.queryKey[0].
🪄 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: 27677f74-54ab-47d9-af70-9ddd3f623460
📒 Files selected for processing (11)
src/components/layout/app-shell.tsxsrc/components/layout/portal-shell.tsxsrc/lib/api.tssrc/routes/api/admin/status-change-requests.tssrc/routes/api/admin/status-change-requests/$id.tssrc/routes/api/invites/$id/approve.tssrc/routes/api/portal/status-change-requests.tssrc/routes/portal/files.tsxsrc/routes/portal/projects/$id.tsxsrc/routes/portal/team.tsxsrc/routes/projects/$id.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/routes/api/admin/status-change-requests/$id.ts
- src/routes/api/invites/$id/approve.ts
- src/routes/api/portal/status-change-requests.ts
- src/routes/api/admin/status-change-requests.ts
- src/routes/portal/projects/$id.tsx
- src/routes/portal/team.tsx
- src/routes/portal/files.tsx
- src/lib/api.ts
…logout in layouts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Client Portal Redesign & Feature Expansion Walkthrough
This document walks through the completed implementation of the client portal redesign and its feature expansions, including database schema updates, UI styling alignment with the admin dashboard, new routes, and admin approval workflows.
What Was Accomplished
We successfully implemented all planned features across the portal and admin sections, including recent database transactional logic, robust migration error handling, and UI alert states:
1. Database Schema Extensions & Constraints
status_change_requeststable to allow clients to request project status changes with a detailed reason/comment. Added CHECK constraints onrequested_status(allowing only'planning','in_progress', or'completed') andapproval_state(allowing only'pending','approved', or'rejected'). AddedON DELETE SET NULLto thereviewed_byforeign key.invitestable withinitiatedByClientId(identifies client-invited colleagues, now withON DELETE SET NULLrule) andadminApprovedAt(records admin approval timestamp).2. Client Portal Visual Overhaul
bg-sidebar,bg-background,text-foreground, etc.).3. New Portal Features & Pages
/portal/files): Grouped all client project files in one central location with UploadThing file-upload capabilities./portal/activity): A global read-only activity feed listing comments, updates, and file uploads./portal/team): Lists current colleagues and pending colleague invites, offering a modal to invite new members. Clarified in the invite modal text that invites require administrator review and approval before being sent.4. Admin Portal Approvals & Guards
border-rose-200class.emailSent: falsein a 200 response rather than failing with a 500 error, ensuring database update authority)./api/portal/activityand/api/portal/filesendpoints to restrict accesses to clients.404 Not Foundif it doesn't exist, and a409 Conflict(already reviewed) if the status is not pending.createdAt,expiresAt,adminApprovedAt) to ISO string format in the Response JSON returned by the/api/invites/$id/approveendpoint.src/routes/projects/$id.tsxto give administrators visual feedback upon approving or rejecting status change requests.scripts/migrate-portal.tsto identify the benign duplicate column sqlite error while throwing/exiting with code1on other unexpected migration failures.Verification & Testing
Automated Tests
We executed the unit tests and TypeScript compiler. All checks pass perfectly:
bun run typecheckpasses cleanly with no compiler warnings or errors.bun run testruns all 22 test files (94 tests total, including the new guard assertions for invite approvals) successfully.Test Files 22 passed (22) Tests 94 passed (94) Start at 11:33:04 Duration 93.51sManual Verification
/api/portal/activityand/api/portal/filesreturn 401/403 as expected./api/invites/$id/approvereturn 403 as expected.Summary by CodeRabbit
New Features
Improvements