feat: fix sharing#169
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 22f8285 in 9 seconds. Click for details.
- Reviewed
718lines of code in8files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_xhQobriXfA5iOfoQ
You can customize 
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a workspace invite system: DB schema for invites, API endpoints to view/claim/revoke invites, UI components and guards to handle invite flows, auth redirect adjustments, and workspace/collaborator updates to support pending invites and claiming. Changes
Sequence DiagramsequenceDiagram
participant User as User (Guest / Auth)
participant Browser as Client
participant Auth as Auth Service
participant API as API Server
participant DB as Database
participant Email as Email Service
Note over User,Browser: User clicks token link from email
Browser->>API: GET /api/invites/[token]
API->>DB: select invite by token
DB-->>API: invite details
API-->>Browser: invite info (workspace, inviter, email, expiresAt)
alt User not signed in
Browser->>Auth: redirect /auth/sign-up?invite=token&redirect=currentURL
Auth-->>Browser: show sign-up (invite context)
User->>Auth: submit signup
Auth-->>Browser: create session, redirect back
end
Browser->>API: POST /api/invites/claim { token } (with session)
API->>Auth: verify session
Auth-->>API: user info
API->>DB: validate invite (token, expiry, email match)
DB-->>API: invite valid
API->>DB: insert collaborator (if not exists)
DB-->>API: collaborator added
API->>DB: delete workspace_invite
DB-->>API: deleted
API-->>Browser: { success, workspaceId, workspaceSlug }
Browser->>Browser: navigate to workspace
Browser-->>User: joined workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { db } from "@/lib/db/client"; | ||
| import { workspaceInvites, workspaceCollaborators, workspaces } from "@/lib/db/schema"; | ||
| import { eq, and } from "drizzle-orm"; | ||
| import { withErrorHandling, requireAuth } from "@/lib/api/workspace-helpers"; |
Greptile OverviewGreptile SummaryThis PR introduces a workspace invitation system: API routes to create/revoke/claim email-based invites, a landing page + client-side guard for invite links, and UI updates in the share dialog and account management. Key flow: Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant UI as Workspace route (/workspace/[slug])
participant IG as InviteGuard
participant ILP as InviteLandingPage
participant API1 as GET /api/invites/{token}
participant API2 as POST /api/invites/claim
participant WC as WorkspaceContext
participant WS as Workspaces API (/api/workspaces/slug/{slug})
participant DB as DB (workspace_invites/collaborators)
U->>UI: Open /workspace/{slug}?invite={token}
UI->>IG: Render InviteGuard
IG->>IG: Read inviteToken from search params
alt session loading
IG-->>U: Show spinner
else inviteToken present AND anonymous/not logged in
IG->>ILP: Render landing page
ILP->>API1: Fetch invite details
API1->>DB: Select invite, workspace, inviter
DB-->>API1: Invite details
API1-->>ILP: JSON details
ILP-->>U: Show invite + Sign in/Sign up
U->>ILP: Click Sign in/Sign up
ILP->>UI: Navigate to /auth/... with redirect_url back
else inviteToken present AND logged in
IG->>API2: Claim invite token
API2->>DB: Validate invite + email match
API2->>DB: Insert workspace_collaborators (if missing)
API2->>DB: Delete workspace_invites row
API2-->>IG: success
IG->>UI: router.replace(remove invite param)
UI->>WC: workspace-by-slug query enabled
WC->>WS: Fetch workspace metadata
WS-->>WC: Workspace metadata
WC-->>UI: Render workspace dashboard
else no inviteToken
IG-->>UI: Render children (dashboard)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Verify email matches current user | ||
| if (invite.email.toLowerCase() !== session.user.email.toLowerCase()) { |
There was a problem hiding this comment.
Missing null check - session.user.email could be null or undefined, which would cause a runtime error when calling .toLowerCase().
| if (invite.email.toLowerCase() !== session.user.email.toLowerCase()) { | |
| if (!session.user.email || invite.email.toLowerCase() !== session.user.email.toLowerCase()) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/invites/claim/route.ts
Line: 43:43
Comment:
Missing null check - `session.user.email` could be null or undefined, which would cause a runtime error when calling `.toLowerCase()`.
```suggestion
if (!session.user.email || invite.email.toLowerCase() !== session.user.email.toLowerCase()) {
```
How can I resolve this? If you propose a fix, please make it concise.| // Remove query param | ||
| const newParams = new URLSearchParams(searchParams.toString()); | ||
| newParams.delete('invite'); | ||
| router.replace(`/workspace/${data.workspaceSlug || ''}?${newParams.toString()}`); |
There was a problem hiding this comment.
Malformed URL - workspaceSlug could be empty string, resulting in URL like /workspace/?..., and query string isn't properly appended (would result in ?invite=...?other=...).
| router.replace(`/workspace/${data.workspaceSlug || ''}?${newParams.toString()}`); | |
| router.replace(`/workspace/${data.workspaceSlug || data.workspaceId}`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/dashboard/page.tsx
Line: 516:516
Comment:
Malformed URL - `workspaceSlug` could be empty string, resulting in URL like `/workspace/?...`, and query string isn't properly appended (would result in `?invite=...?other=...`).
```suggestion
router.replace(`/workspace/${data.workspaceSlug || data.workspaceId}`);
```
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| async function claimInvite() { | ||
| if (!inviteToken || !session?.user || isSessionLoading) return; | ||
|
|
||
| try { | ||
| const res = await fetch('/api/invites/claim', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ token: inviteToken }) | ||
| }); | ||
|
|
||
| const data = await res.json(); | ||
|
|
||
| if (res.ok) { | ||
| toast.success('Invitation accepted!'); | ||
| // Remove query param | ||
| const newParams = new URLSearchParams(searchParams.toString()); | ||
| newParams.delete('invite'); | ||
| router.replace(`/workspace/${data.workspaceSlug || ''}?${newParams.toString()}`); | ||
| // Force reload to get permission updates | ||
| window.location.reload(); | ||
| } else { | ||
| toast.error(data.message || data.error || 'Failed to accept invitation'); | ||
| } | ||
| } catch (e) { | ||
| console.error(e); | ||
| toast.error('Failed to accept invitation'); | ||
| } | ||
| } | ||
|
|
||
| claimInvite(); | ||
| }, [inviteToken, session, isSessionLoading, router, searchParams]); |
There was a problem hiding this comment.
Missing cleanup or error handling - if claiming the invite fails, the invite query param persists in the URL, causing the claim attempt to repeat on every re-render or navigation. This creates an infinite retry loop for failed invites.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/dashboard/page.tsx
Line: 498:529
Comment:
Missing cleanup or error handling - if claiming the invite fails, the `invite` query param persists in the URL, causing the claim attempt to repeat on every re-render or navigation. This creates an infinite retry loop for failed invites.
How can I resolve this? If you propose a fix, please make it concise.| const generateToken = () => { | ||
| const array = new Uint8Array(24); | ||
| crypto.getRandomValues(array); | ||
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); | ||
| }; |
There was a problem hiding this comment.
Using crypto.getRandomValues() in Node.js requires importing from node:crypto or using the global crypto object which may not be available in all environments. Verify this works in your deployment environment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/workspaces/[id]/collaborators/route.ts
Line: 210:214
Comment:
Using `crypto.getRandomValues()` in Node.js requires importing from `node:crypto` or using the global `crypto` object which may not be available in all environments. Verify this works in your deployment environment.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/api/workspaces/[id]/collaborators/route.ts
Line: 119:124
Comment:
Dead code path - this check now happens after user existence check, but this early return prevents pending invite flow for non-existent users. The logic at line 119-124 will never execute for new users since `invitedUser` is checked at line 157, making this block unreachable for the intended pending invite flow.
```suggestion
// Find the user by email
const [invitedUser] = await db
.select({ id: user.id })
.from(user)
.where(eq(user.email, email.trim().toLowerCase()))
.limit(1);
```
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/workspaces/[id]/collaborators/route.ts (2)
119-124:⚠️ Potential issue | 🔴 CriticalDead code: Pending invite logic is unreachable.
This early return when the user doesn't exist prevents the pending invite flow (lines 206-270) from ever executing. According to the PR objectives, non-existing users should receive a pending invite with a token, but this code path makes that impossible.
Remove this early return to allow the pending invite logic to execute for non-existing users.
🐛 Proposed fix
- if (!invitedUser) { - return NextResponse.json( - { message: "User not found. They need to sign up first." }, - { status: 404 } - ); - }
243-257:⚠️ Potential issue | 🟡 MinorApply consistent null check for
ws.Same issue as line 186 - add optional chaining for consistency.
🛡️ Proposed fix
try { - const identifier = ws.slug || workspaceId; + const identifier = ws?.slug || workspaceId;
🤖 Fix all issues with AI agents
In `@src/app/api/invites/claim/route.ts`:
- Around line 50-73: Wrap the existence check, potential insert into
workspaceCollaborators, and delete from workspaceInvites in a single
db.transaction() so the claim is atomic; inside the transaction attempt the
insert of {workspaceId: invite.workspaceId, userId: session.user.id,
permissionLevel: invite.permissionLevel} but use onConflictDoNothing()
(targeting the composite key workspaceId+userId / the
workspace_collaborators_workspace_user_unique constraint) to avoid
unique-constraint errors from concurrent claims, then delete the invite
(workspaceInvites where id = invite.id) within the same transaction; keep the
prior existence check if desired but ensure the insert+delete run inside the
transaction obtained from db.transaction().
In `@src/app/api/workspaces/`[id]/collaborators/route.ts:
- Around line 183-201: The code accesses ws.slug and ws.name without the
null-check used earlier, which can cause a runtime error if ws is null; update
the invitation-email block (the identifier/workspaceUrl construction and the
InviteEmailTemplate props passed to resend.emails.send) to safely use optional
chaining or the same guard as before (e.g., use ws?.slug ?? workspaceId and
ws?.name ?? 'Workspace' and ws?.name ?? 'a workspace'), and ensure all uses of
ws in that try block are null-safe (including building workspaceUrl and subject)
so the send call never dereferences a potentially null ws.
In `@src/app/dashboard/page.tsx`:
- Around line 497-529: The current claimInvite flow races router.replace() with
an immediate window.location.reload(), which can reload the old URL (with the
invite param) before the SPA navigation commits; replace that sequence by
building the target URL (use the same logic that deletes the 'invite' key from
searchParams and uses data.workspaceSlug || '') and call
window.location.assign(targetUrl) instead of router.replace(...) followed by
window.location.reload(); update the claimInvite function to remove
router.replace(...) and window.location.reload() and use
window.location.assign(targetUrl) to force a full-page navigation to the correct
URL.
In `@src/lib/db/schema.ts`:
- Around line 233-234: Remove the permissive public SELECT policy named "Public
can view invite by token" on workspace_invites (the pgPolicy call with using:
sql`true` and to: ["public"]) because it exposes emails and tokens; instead
restrict SELECT to authenticated users (e.g., change to to: ["authenticated"] or
add a using condition that checks auth.uid matches workspace membership) and
ensure any token validation/lookup is performed server-side (RPC or Edge
Function) using the service role key rather than allowing public reads.
🧹 Nitpick comments (1)
src/app/api/workspaces/[id]/collaborators/route.ts (1)
1-14: Remove duplicated documentation block.The file header comment (lines 1-6) is duplicated again at lines 8-13.
🧹 Proposed fix
/** * Collaborators API - List and invite collaborators * * GET /api/workspaces/[id]/collaborators - List collaborators * POST /api/workspaces/[id]/collaborators - Invite a new collaborator */ -/** - * Collaborators API - List and invite collaborators - * - * GET /api/workspaces/[id]/collaborators - List collaborators - * POST /api/workspaces/[id]/collaborators - Invite a new collaborator - */ - import { NextRequest, NextResponse } from "next/server";
| // Check if already a collaborator - if so, just consume the invite | ||
| const [existing] = await db | ||
| .select() | ||
| .from(workspaceCollaborators) | ||
| .where(and( | ||
| eq(workspaceCollaborators.workspaceId, invite.workspaceId), | ||
| eq(workspaceCollaborators.userId, session.user.id) | ||
| )) | ||
| .limit(1); | ||
|
|
||
| if (!existing) { | ||
| // Add to collaborators | ||
| await db.insert(workspaceCollaborators).values({ | ||
| workspaceId: invite.workspaceId, | ||
| userId: session.user.id, | ||
| permissionLevel: invite.permissionLevel, | ||
| }); | ||
| } | ||
|
|
||
| // Clean up used invite | ||
| await db | ||
| .delete(workspaceInvites) | ||
| .where(eq(workspaceInvites.id, invite.id)); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and verify its existence
find . -name "route.ts" -path "*/api/invites/claim*" | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 99
🏁 Script executed:
# Search for the file in the repository
git ls-files | grep -i "invite" | grep -i "route"Repository: ThinkEx-OSS/thinkex
Length of output: 134
🏁 Script executed:
# Get the repository structure to understand the project
ls -la | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 1235
🏁 Script executed:
# Read the file to see the code at lines 50-73
cat -n src/app/api/invites/claim/route.ts | sed -n '45,80p'Repository: ThinkEx-OSS/thinkex
Length of output: 1523
🏁 Script executed:
# Find the database schema files to understand constraints
find . -type f -name "*.ts" -o -name "*.sql" | xargs grep -l "workspaceCollaborators\|workspaceInvites" | head -10Repository: ThinkEx-OSS/thinkex
Length of output: 513
🏁 Script executed:
# Look for schema definitions
fd -t f "schema\|db\|types" --path "*db*" --path "*schema*" | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 296
🏁 Script executed:
# Search for schema/database definitions
find . -type f \( -name "*.ts" -o -name "*.sql" \) | xargs grep -l "workspaceCollaborators" | head -10Repository: ThinkEx-OSS/thinkex
Length of output: 497
🏁 Script executed:
# Look for Drizzle schema files
find . -path "*/schema*" -name "*.ts" | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 962
🏁 Script executed:
# Search more broadly for db schema
rg "workspaceCollaborators" -t ts | grep -E "(table|primaryKey|unique)" | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
# Read the database schema file
cat -n src/lib/db/schema.ts | head -200Repository: ThinkEx-OSS/thinkex
Length of output: 12031
🏁 Script executed:
# Search specifically for workspaceCollaborators definition
rg "workspaceCollaborators" src/lib/db/schema.ts -A 20Repository: ThinkEx-OSS/thinkex
Length of output: 1485
🏁 Script executed:
# Check if the codebase uses db.transaction anywhere
rg "db\.transaction|\.transaction\(" src/ -A 5 | head -50Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
# Look for onConflictDoNothing usage in the codebase
rg "onConflictDoNothing|onConflict" src/ -A 3 -B 1Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
# Check package.json for Drizzle version
cat package.json | grep -A 5 -B 5 "drizzle"Repository: ThinkEx-OSS/thinkex
Length of output: 1069
🌐 Web query:
Drizzle ORM 0.44.7 transaction support PostgreSQL
💡 Result:
Drizzle ORM v0.44.7 already has full PostgreSQL transaction support via db.transaction(...) (commit/rollback handled automatically). You can also pass PostgreSQL-specific transaction options like isolationLevel, accessMode, and deferrable. [1]
What changed in 0.44.7? Nothing PostgreSQL-specific—v0.44.7 is a small patch release that fixes a durable-sqlite transaction return value issue, not Postgres transactions. [2]
PostgreSQL transaction usage (v0.44.7):
await db.transaction(
async (tx) => {
await tx.update(accounts).set({ /* ... */ });
await tx.insert(logs).values({ /* ... */ });
},
{
isolationLevel: "read committed",
accessMode: "read write",
deferrable: true,
}
);[1]
Nested transactions: tx.transaction(...) works on PostgreSQL by using SAVEPOINTs under the hood (so nested blocks can roll back independently). [1]
Sources
[1] Drizzle ORM docs – Transactions https://orm.drizzle.team/docs/transactions
[2] Drizzle ORM site – v0.44.7 release note (durable sqlite fix) https://orm.drizzle.team/
🌐 Web query:
Drizzle ORM onConflictDoNothing composite unique key
💡 Result:
To use onConflictDoNothing with a composite unique key in Drizzle, pass an array of columns as the target, and make sure your schema actually creates a UNIQUE constraint (or composite PRIMARY KEY) on the same columns.
1) Define a composite UNIQUE constraint in the schema
import { pgTable, text, unique } from "drizzle-orm/pg-core";
export const users = pgTable("users", {
firstName: text("first_name").notNull(),
lastName: text("last_name").notNull(),
}, (t) => [
unique().on(t.firstName, t.lastName),
]);Drizzle supports composite unique constraints via unique().on(col1, col2). [1]
2) Insert with onConflictDoNothing targeting that composite key
await db.insert(users).values({ firstName: "John", lastName: "Doe" })
.onConflictDoNothing({ target: [users.firstName, users.lastName] });onConflictDoNothing supports an explicit target, and conflict targets are columns (or an array of columns for composite). [2]
SQLite note (common pitfall)
If you get ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint, it usually means your table has a unique index but not a UNIQUE constraint / PK matching the target (this has come up with SQLite in Drizzle). In that case either:
- ensure the migration produces a real
UNIQUE(col1, col2)table constraint (like theunique().on(...)example), or - use
.onConflictDoNothing()withouttargetto ignore conflicts on any unique constraint. [2][3]
Sources: Drizzle docs on constraints [1] and insert/onConflictDoNothing [2], plus SQLite-related conflict-target issue discussion [3].
Wrap collaborator insert and invite delete in a transaction to prevent race conditions.
The current code is vulnerable to concurrent requests: two simultaneous claims can both pass the existence check and attempt to insert, causing one to fail on the unique constraint workspace_collaborators_workspace_user_unique on (workspaceId, userId). Use db.transaction() to make both the insert and delete atomic, and handle conflicts with onConflictDoNothing() targeting the composite key.
Proposed transactional fix
- if (!existing) {
- // Add to collaborators
- await db.insert(workspaceCollaborators).values({
- workspaceId: invite.workspaceId,
- userId: session.user.id,
- permissionLevel: invite.permissionLevel,
- });
- }
-
- // Clean up used invite
- await db
- .delete(workspaceInvites)
- .where(eq(workspaceInvites.id, invite.id));
+ await db.transaction(async (tx) => {
+ if (!existing) {
+ await tx
+ .insert(workspaceCollaborators)
+ .values({
+ workspaceId: invite.workspaceId,
+ userId: session.user.id,
+ permissionLevel: invite.permissionLevel,
+ })
+ .onConflictDoNothing({
+ target: [
+ workspaceCollaborators.workspaceId,
+ workspaceCollaborators.userId,
+ ],
+ });
+ }
+
+ await tx
+ .delete(workspaceInvites)
+ .where(eq(workspaceInvites.id, invite.id));
+ });📝 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.
| // Check if already a collaborator - if so, just consume the invite | |
| const [existing] = await db | |
| .select() | |
| .from(workspaceCollaborators) | |
| .where(and( | |
| eq(workspaceCollaborators.workspaceId, invite.workspaceId), | |
| eq(workspaceCollaborators.userId, session.user.id) | |
| )) | |
| .limit(1); | |
| if (!existing) { | |
| // Add to collaborators | |
| await db.insert(workspaceCollaborators).values({ | |
| workspaceId: invite.workspaceId, | |
| userId: session.user.id, | |
| permissionLevel: invite.permissionLevel, | |
| }); | |
| } | |
| // Clean up used invite | |
| await db | |
| .delete(workspaceInvites) | |
| .where(eq(workspaceInvites.id, invite.id)); | |
| // Check if already a collaborator - if so, just consume the invite | |
| const [existing] = await db | |
| .select() | |
| .from(workspaceCollaborators) | |
| .where(and( | |
| eq(workspaceCollaborators.workspaceId, invite.workspaceId), | |
| eq(workspaceCollaborators.userId, session.user.id) | |
| )) | |
| .limit(1); | |
| await db.transaction(async (tx) => { | |
| if (!existing) { | |
| await tx | |
| .insert(workspaceCollaborators) | |
| .values({ | |
| workspaceId: invite.workspaceId, | |
| userId: session.user.id, | |
| permissionLevel: invite.permissionLevel, | |
| }) | |
| .onConflictDoNothing({ | |
| target: [ | |
| workspaceCollaborators.workspaceId, | |
| workspaceCollaborators.userId, | |
| ], | |
| }); | |
| } | |
| await tx | |
| .delete(workspaceInvites) | |
| .where(eq(workspaceInvites.id, invite.id)); | |
| }); |
🤖 Prompt for AI Agents
In `@src/app/api/invites/claim/route.ts` around lines 50 - 73, Wrap the existence
check, potential insert into workspaceCollaborators, and delete from
workspaceInvites in a single db.transaction() so the claim is atomic; inside the
transaction attempt the insert of {workspaceId: invite.workspaceId, userId:
session.user.id, permissionLevel: invite.permissionLevel} but use
onConflictDoNothing() (targeting the composite key workspaceId+userId / the
workspace_collaborators_workspace_user_unique constraint) to avoid
unique-constraint errors from concurrent claims, then delete the invite
(workspaceInvites where id = invite.id) within the same transaction; keep the
prior existence check if desired but ensure the insert+delete run inside the
transaction obtained from db.transaction().
| // Send standard invitation email | ||
| try { | ||
| // Use slug if available, otherwise fallback to id | ||
| const identifier = ws.slug || workspaceId; | ||
| const workspaceUrl = `https://thinkex.app/workspace/${identifier}`; | ||
| const { data, error } = await resend.emails.send({ | ||
| from: 'ThinkEx <hello@thinkex.app>', | ||
| to: [email], | ||
| subject: `You've been invited to collaborate on ${ws.name || 'a workspace'}`, | ||
| react: InviteEmailTemplate({ | ||
| inviterName: currentUser.name || 'A user', | ||
| workspaceName: ws.name || 'Workspace', | ||
| workspaceUrl, | ||
| }), | ||
| }); | ||
| if (error) console.error("Failed to send invitation email:", error); | ||
| } catch (emailError) { | ||
| console.error("Error sending invitation email:", emailError); | ||
| } |
There was a problem hiding this comment.
Inconsistent null check for ws.
Line 166 checks if (ws && ...) before accessing ws.userId, but line 186 accesses ws.slug without a null check. While verifyWorkspaceAccess should ensure the workspace exists, it's safer to be consistent.
🛡️ Proposed fix
// Send standard invitation email
try {
// Use slug if available, otherwise fallback to id
- const identifier = ws.slug || workspaceId;
+ const identifier = ws?.slug || workspaceId;📝 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.
| // Send standard invitation email | |
| try { | |
| // Use slug if available, otherwise fallback to id | |
| const identifier = ws.slug || workspaceId; | |
| const workspaceUrl = `https://thinkex.app/workspace/${identifier}`; | |
| const { data, error } = await resend.emails.send({ | |
| from: 'ThinkEx <hello@thinkex.app>', | |
| to: [email], | |
| subject: `You've been invited to collaborate on ${ws.name || 'a workspace'}`, | |
| react: InviteEmailTemplate({ | |
| inviterName: currentUser.name || 'A user', | |
| workspaceName: ws.name || 'Workspace', | |
| workspaceUrl, | |
| }), | |
| }); | |
| if (error) console.error("Failed to send invitation email:", error); | |
| } catch (emailError) { | |
| console.error("Error sending invitation email:", emailError); | |
| } | |
| // Send standard invitation email | |
| try { | |
| // Use slug if available, otherwise fallback to id | |
| const identifier = ws?.slug || workspaceId; | |
| const workspaceUrl = `https://thinkex.app/workspace/${identifier}`; | |
| const { data, error } = await resend.emails.send({ | |
| from: 'ThinkEx <hello@thinkex.app>', | |
| to: [email], | |
| subject: `You've been invited to collaborate on ${ws.name || 'a workspace'}`, | |
| react: InviteEmailTemplate({ | |
| inviterName: currentUser.name || 'A user', | |
| workspaceName: ws.name || 'Workspace', | |
| workspaceUrl, | |
| }), | |
| }); | |
| if (error) console.error("Failed to send invitation email:", error); | |
| } catch (emailError) { | |
| console.error("Error sending invitation email:", emailError); | |
| } |
🤖 Prompt for AI Agents
In `@src/app/api/workspaces/`[id]/collaborators/route.ts around lines 183 - 201,
The code accesses ws.slug and ws.name without the null-check used earlier, which
can cause a runtime error if ws is null; update the invitation-email block (the
identifier/workspaceUrl construction and the InviteEmailTemplate props passed to
resend.emails.send) to safely use optional chaining or the same guard as before
(e.g., use ws?.slug ?? workspaceId and ws?.name ?? 'Workspace' and ws?.name ??
'a workspace'), and ensure all uses of ws in that try block are null-safe
(including building workspaceUrl and subject) so the send call never
dereferences a potentially null ws.
| pgPolicy("Public can view invite by token", { as: "permissive", for: "select", to: ["public"], using: sql`true` }), | ||
| pgPolicy("Users can insert invites for workspaces they own/edit", { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Postgres RLS policy "using true" "to public" anonymous client all rows security
💡 Result:
In PostgreSQL RLS, a policy like:
CREATE POLICY p ON my_table
FOR ALL
TO PUBLIC
USING (true);means:
TO PUBLICapplies to every role (it’s a special “all roles” pseudo-role), so it includes anonymous / unauthenticated DB roles too. (postgresql.org)USING (true)allows every existing row for commands that use theUSINGclause (notablySELECT, and the row-targeting part ofUPDATE/DELETE). Since the predicate is always true, RLS won’t filter anything—so the client can see “all rows” as long as they also have normal SQL privileges (e.g.,GRANT SELECT) on the table. (postgresql.org)- RLS policies only take effect if RLS is enabled on the table (
ALTER TABLE ... ENABLE ROW LEVEL SECURITY). If enabled but no applicable policies exist, Postgres is default-deny. (postgresql.org) - For
INSERT/UPDATEof new row values, enforcement is viaWITH CHECK(orUSINGis reused ifWITH CHECKisn’t specified for policies where that applies). So “allow all writes” typically needsWITH CHECK (true)too. (postgresql.org)
If your goal is “anonymous can’t read anything”, don’t use USING (true) for PUBLIC. Use USING (false), or target only authenticated roles (not PUBLIC), and ensure you haven’t granted broad table privileges to the anonymous role. (postgresql.org)
Citations:
- 1: https://www.postgresql.org/docs/current/sql-createpolicy.html?utm_source=openai
- 2: https://www.postgresql.org/docs/current/sql-createpolicy.html?utm_source=openai
- 3: https://www.postgresql.org/docs/current/sql-createpolicy.html?utm_source=openai
- 4: https://www.postgresql.org/docs/current/sql-createpolicy.html?utm_source=openai
- 5: https://www.postgresql.org/docs/current/sql-createpolicy.html?utm_source=openai
🌐 Web query:
Supabase RLS public SELECT policy security best practices token lookup
💡 Result:
Public SELECT policy (e.g., USING (true)) — when it’s OK, and how to do it safely
- Assume anything in
publicis reachable via the Data API; if RLS is off, the table is accessible using theanonrole. Enable RLS on every exposed table/view/function first. [1], [2] - RLS-on + no policies = no access (default deny). Only add a “public read” policy if you truly want public reads. [2]
- If you intentionally allow public reads, prefer least data:
- Put only non-sensitive fields in that table, or expose a view that contains only safe columns/rows, and put the public
SELECTpolicy on the view/table that’s safe to publish. (Supabase emphasizes RLS as the control point for what rows are accessible.) [2]
- Put only non-sensitive fields in that table, or expose a view that contains only safe columns/rows, and put the public
- Also lock down table privileges (RLS and grants work together): for public-read tables, grant
SELECTonly; revoke write privileges fromanon. [3] - Never expose the service role key in any client (it bypasses RLS). Use the anon key in the browser. [4]
- In policies that require a logged-in user, explicitly check auth is present (because
auth.uid()isNULLwhen unauthenticated). [2]
Token lookup best practices (API keys, invite tokens, magic links, “shared secret” rows)
- Do not make the token table publicly selectable. A “public
SELECT” policy on a table that stores tokens (or token hashes) enables enumeration/harvesting and turns your DB into an oracle. Keep token material out of anything you expose broadly. (General rule derived from Supabase’s “public schema is reachable” + “policies act like a WHERE clause” model.) [1], [2] - Prefer server-side validation paths:
- Validate tokens in a backend/Edge Function using the service role key (server-only) and return only the minimal result to the client. [4]
- Or use a database function (RPC) to validate the token and return a minimal payload—this lets you keep the underlying token table non-readable while still supporting the workflow. (Supabase’s guidance: secure data via RLS; don’t rely on clients having broad table access.) [1], [2]
- Store tokens as hashes, not plaintext, and compare hashes inside the trusted path (RPC/backend). (This isn’t Supabase-specific, but it’s the standard mitigation once you accept tokens may leak via logs/backups.)
- If you’re using JWT claims for authorization logic, be careful about what claims you trust; Supabase JWTs carry claims and role used for RLS evaluation. [5]
Minimal “public read” pattern (safe-only table)
- RLS enabled +
SELECTpolicyUSING (true)on a sanitized table/view. [2] REVOKE ALLthenGRANT SELECTtoanonfor that object. [3]
If you share your exact “token lookup” flow (what the token represents, who presents it, and what data you need to return), I can suggest a concrete table + RLS/RPC pattern that avoids public token reads.
Sources: Supabase docs on securing the API/public schema exposure and RLS basics [1][2], Data API hardening and grants [3], and service-role key/RLS bypass guidance [4], plus JWT/claims overview [5].
🌐 Web query:
Postgres Row Level Security using: true select permission public role behavior
💡 Result:
-
CREATE POLICY ... FOR SELECT TO PUBLIC USING (true)means: for any role, the RLS predicate forSELECTis always true, so RLS will not filter out any rows (all rows are visible as far as RLS is concerned). RLS rows are shown when theUSINGexpression evaluates to true; false/null rows are silently hidden. [1] -
This does not grant
SELECTprivilege. PostgreSQL checks normal table privileges first; you still mustGRANT SELECT ON table ...(directly or via a role the user inherits). RLS only further restricts which rows are visible once the command itself is allowed. [2] -
PUBLICinTO PUBLICis the special “all roles” target (not thepublicschema). So the policy applies to everyone unless overridden by other policies. [1][2] -
If you have multiple permissive
SELECTpolicies, they are OR’ed together; a singleUSING (true)permissive policy effectively makes the permissive portion always pass. Restrictive policies, if any, are AND’ed and can still block rows. [1] -
Table owners (and superusers / roles with
BYPASSRLS) can bypass RLS unless youALTER TABLE ... FORCE ROW LEVEL SECURITY. [2]
🏁 Script executed:
# Find the invites table definition and any related RLS policies
cd src/lib/db && grep -n "invites" schema.ts | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 920
🏁 Script executed:
# Look at the invites table schema and all its RLS policies
cd src/lib/db && sed -n '200,300p' schema.tsRepository: ThinkEx-OSS/thinkex
Length of output: 2492
🏁 Script executed:
# Check for GRANT statements related to workspace_invites
rg "GRANT.*workspace_invites|workspace_invites.*GRANT" src/Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
# Search for where the public SELECT policy might be used/called in the codebase
rg "workspace_invites" src/ --type ts --type tsx | grep -E "(select|query|from)" | head -20Repository: ThinkEx-OSS/thinkex
Length of output: 90
🏁 Script executed:
# Check if there's any RLS configuration or anon role setup
rg "anon|role|GRANT|permission" src/lib/db/schema.ts | head -30Repository: ThinkEx-OSS/thinkex
Length of output: 706
Remove public SELECT policy on workspace_invites — exposes email addresses and invite tokens.
The using: sql\true`withto: ["public"]` allows any anonymous client to read all rows, including email addresses (PII) and unique invite tokens. This enables enumeration attacks and token harvesting.
Move token validation to a backend RPC or Edge Function using the service role key, or restrict the SELECT policy to authenticated users only. Supabase guidance explicitly recommends against exposing token tables publicly.
🤖 Prompt for AI Agents
In `@src/lib/db/schema.ts` around lines 233 - 234, Remove the permissive public
SELECT policy named "Public can view invite by token" on workspace_invites (the
pgPolicy call with using: sql`true` and to: ["public"]) because it exposes
emails and tokens; instead restrict SELECT to authenticated users (e.g., change
to to: ["authenticated"] or add a using condition that checks auth.uid matches
workspace membership) and ensure any token validation/lookup is performed
server-side (RPC or Edge Function) using the service role key rather than
allowing public reads.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/app/api/workspaces/[id]/collaborators/route.ts">
<violation number="1" location="src/app/api/workspaces/[id]/collaborators/route.ts:156">
P0: Dead code: The pending invite feature will never execute. There's an early return at lines 118-122 that returns 404 when `invitedUser` is not found, but this `if (invitedUser)` check assumes the user might not exist. The entire else branch (pending invite logic) is unreachable. The early return needs to be removed for this feature to work.</violation>
</file>
<file name="src/components/workspace/InviteLandingPage.tsx">
<violation number="1" location="src/components/workspace/InviteLandingPage.tsx:58">
P1: The authentication page (`src/app/auth/[path]/page.tsx`) expects a `redirect_url` query parameter to handle post-login redirection, but this component sends `callbackUrl`. This mismatch will cause the user to be redirected to the default page (e.g., `/home`) instead of returning to this invite page, breaking the auto-claim workflow.</violation>
</file>
<file name="src/lib/db/schema.ts">
<violation number="1" location="src/lib/db/schema.ts:234">
P2: The \`INSERT\` policy checks workspace permissions but does not enforce that the \`inviter_id\` matches the authenticated user. This allows a user to create an invite while impersonating another user as the inviter.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }).onDelete("cascade"), | ||
| unique("workspace_invites_token_key").on(table.token), | ||
| pgPolicy("Public can view invite by token", { as: "permissive", for: "select", to: ["public"], using: sql`true` }), | ||
| pgPolicy("Users can insert invites for workspaces they own/edit", { |
There was a problem hiding this comment.
P2: The `INSERT` policy checks workspace permissions but does not enforce that the `inviter_id` matches the authenticated user. This allows a user to create an invite while impersonating another user as the inviter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/db/schema.ts, line 234:
<comment>The \`INSERT\` policy checks workspace permissions but does not enforce that the \`inviter_id\` matches the authenticated user. This allows a user to create an invite while impersonating another user as the inviter.</comment>
<file context>
@@ -210,3 +210,31 @@ export const workspaceCollaborators = pgTable("workspace_collaborators", {
+ }).onDelete("cascade"),
+ unique("workspace_invites_token_key").on(table.token),
+ pgPolicy("Public can view invite by token", { as: "permissive", for: "select", to: ["public"], using: sql`true` }),
+ pgPolicy("Users can insert invites for workspaces they own/edit", {
+ as: "permissive", for: "insert", to: ["authenticated"], withCheck: sql`(EXISTS ( SELECT 1
+ FROM workspaces w
</file context>
| const [confirmPassword, setConfirmPassword] = useState(""); | ||
| const [showDeleteAlert, setShowDeleteAlert] = useState(false); | ||
| const [isDeleting, setIsDeleting] = useState(false); | ||
| const router = useRouter(); |
| } from "@/components/ui/dropdown-menu"; | ||
| import { AccountModal } from "@/components/auth/AccountModal"; | ||
|
|
||
| import { toast } from "sonner"; |
There was a problem hiding this comment.
3 issues found across 10 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/app/auth/[path]/page.tsx">
<violation number="1" location="src/app/auth/[path]/page.tsx:25">
P1: Potential Open Redirect vulnerability. The `redirect_url` and `callbackUrl` parameters are taken directly from user input without validation. If an attacker provides an external URL (e.g., `https://evil.com`), this value is passed to the authentication flow.
Specifically, in the `sign-up` path, this URL is encoded and passed as a query parameter to `/onboarding`. The `OnboardingPage` (verified via file read) redirects to this URL if it doesn't match a specific internal blocklist, which inadvertently allows external domains.
To prevent this, ensure `redirectTo` is a relative path (starts with `/` and not `//`).</violation>
</file>
<file name="src/app/api/workspaces/[id]/collaborators/route.ts">
<violation number="1" location="src/app/api/workspaces/[id]/collaborators/route.ts:207">
P1: The fallback to `Math.random()` creates predictable tokens, which is a critical security vulnerability for invitation systems.
In Next.js 16 (Node.js 18+ or Edge Runtime), the Web Crypto API (`crypto`) is globally available. If `crypto` is missing for some reason, the application should fail securely rather than silently degrading to insecure token generation.
Remove the fallback and reliance on `Math.random`.</violation>
</file>
<file name="src/components/workspace-canvas/WorkspaceSection.tsx">
<violation number="1" location="src/components/workspace-canvas/WorkspaceSection.tsx:436">
P1: `useSearchParams` is a hook and must be called at the top level of the component. Calling it inside this conditional render branch violates the Rules of Hooks and can cause hook ordering bugs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!redirectTo) { | ||
| if (path === "sign-in") { | ||
| redirectTo = "/home"; | ||
| let redirectTo = redirect_url || callbackUrl; |
There was a problem hiding this comment.
P1: Potential Open Redirect vulnerability. The redirect_url and callbackUrl parameters are taken directly from user input without validation. If an attacker provides an external URL (e.g., https://evil.com), this value is passed to the authentication flow.
Specifically, in the sign-up path, this URL is encoded and passed as a query parameter to /onboarding. The OnboardingPage (verified via file read) redirects to this URL if it doesn't match a specific internal blocklist, which inadvertently allows external domains.
To prevent this, ensure redirectTo is a relative path (starts with / and not //).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/auth/[path]/page.tsx, line 25:
<comment>Potential Open Redirect vulnerability. The `redirect_url` and `callbackUrl` parameters are taken directly from user input without validation. If an attacker provides an external URL (e.g., `https://evil.com`), this value is passed to the authentication flow.
Specifically, in the `sign-up` path, this URL is encoded and passed as a query parameter to `/onboarding`. The `OnboardingPage` (verified via file read) redirects to this URL if it doesn't match a specific internal blocklist, which inadvertently allows external domains.
To prevent this, ensure `redirectTo` is a relative path (starts with `/` and not `//`).</comment>
<file context>
@@ -17,18 +17,23 @@ export default async function AuthPage({
- if (!redirectTo) {
- if (path === "sign-in") {
- redirectTo = "/home";
+ let redirectTo = redirect_url || callbackUrl;
+
+ // For sign-up, always route through onboarding, but preserve the final destination
</file context>
| let redirectTo = redirect_url || callbackUrl; | |
| let redirectTo = redirect_url || callbackUrl; | |
| // Prevent open redirects by ensuring the path is relative and not protocol-relative (//) | |
| if (redirectTo && (!redirectTo.startsWith("/") || redirectTo.startsWith("//"))) { | |
| redirectTo = undefined; | |
| } |
| // Use Web Crypto API which is available in Node.js global scope in newer versions | ||
| // or fall back to Math.random for older environments if critical | ||
| if (typeof crypto !== 'undefined') { | ||
| const array = new Uint8Array(24); | ||
| crypto.getRandomValues(array); | ||
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); | ||
| } else { | ||
| // Fallback for environments without crypto global | ||
| return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); | ||
| } |
There was a problem hiding this comment.
P1: The fallback to Math.random() creates predictable tokens, which is a critical security vulnerability for invitation systems.
In Next.js 16 (Node.js 18+ or Edge Runtime), the Web Crypto API (crypto) is globally available. If crypto is missing for some reason, the application should fail securely rather than silently degrading to insecure token generation.
Remove the fallback and reliance on Math.random.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/api/workspaces/[id]/collaborators/route.ts, line 207:
<comment>The fallback to `Math.random()` creates predictable tokens, which is a critical security vulnerability for invitation systems.
In Next.js 16 (Node.js 18+ or Edge Runtime), the Web Crypto API (`crypto`) is globally available. If `crypto` is missing for some reason, the application should fail securely rather than silently degrading to insecure token generation.
Remove the fallback and reliance on `Math.random`.</comment>
<file context>
@@ -208,9 +204,16 @@ async function handlePOST(
- const array = new Uint8Array(24);
- crypto.getRandomValues(array);
- return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join('');
+ // Use Web Crypto API which is available in Node.js global scope in newer versions
+ // or fall back to Math.random for older environments if critical
+ if (typeof crypto !== 'undefined') {
</file context>
| // Use Web Crypto API which is available in Node.js global scope in newer versions | |
| // or fall back to Math.random for older environments if critical | |
| if (typeof crypto !== 'undefined') { | |
| const array = new Uint8Array(24); | |
| crypto.getRandomValues(array); | |
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); | |
| } else { | |
| // Fallback for environments without crypto global | |
| return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); | |
| } | |
| const array = new Uint8Array(24); | |
| crypto.getRandomValues(array); | |
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/workspace-canvas/WorkspaceSection.tsx (1)
430-440:⚠️ Potential issue | 🟠 MajorMove
useSearchParams()to the top level.
useSearchParams()is a hook and must be called unconditionally at the component's top level, not inside conditional render paths. Calling it inside a ternary expression violates React's hook rules and can break render ordering. Hoist it to the top of the component and derive a boolean variable from it.Proposed fix
export function WorkspaceSection({ // props... }: WorkspaceSectionProps) { + const searchParams = useSearchParams(); + const hasInviteParam = searchParams.get("invite"); const selectedCardIdsArray = useUIStore( useShallow(selectSelectedCardIdsArray) ); // ... other hooks return ( // ... - ) : useSearchParams().get('invite') ? ( + ) : hasInviteParam ? (
🤖 Fix all issues with AI agents
In `@src/app/api/workspaces/`[id]/collaborators/route.ts:
- Around line 204-216: The generateToken function currently falls back to
Math.random which is insecure; replace its implementation to use Node's
crypto.randomBytes unconditionally (e.g., call
crypto.randomBytes(24).toString('hex') or import randomBytes from 'crypto') for
invite token generation in the API route; remove the Web Crypto + Math.random
branch and ensure the code uses the Node crypto API only so tokens are
cryptographically secure.
In `@src/app/auth/`[path]/page.tsx:
- Around line 25-36: The redirect target logic currently assigns redirect_url or
callbackUrl directly to redirectTo (variables: redirectTo, redirect_url,
callbackUrl, path) which allows external or protocol-relative open redirects;
add a validation step (e.g., an isSafeRedirect helper) that ensures the chosen
redirect is an internal path only — require it to start with a single slash
("/") and explicitly reject values that start with "//" or include a protocol
("://"); use this validator before encoding/embedding into
`/onboarding?redirect_url=...` and if validation fails fall back to a safe
default like "/home" or "/" so that only validated internal routes are used for
redirectTo.
In `@src/lib/auth.ts`:
- Around line 66-75: The afterDelete hook for deleteUser currently swallows
failures when running db.delete(workspaces).where(eq(workspaces.userId,
user.id)), which can leave orphaned user data; update deleteUser.afterDelete to
make workspace cleanup actionable: either ensure DB cascade constraints on
workspaces.userId and remove this manual delete, or if keeping manual cleanup,
propagate the error (rethrow) so account deletion aborts on failure, and/or
enqueue a retry + emit an alert/metric when the delete fails; also audit and add
similar cleanup (or cascades) for other user-linked tables such as
collaborators, invites, and events to avoid orphaned records.
| // Import nanoid dynamically or use a simple random string generator since nanoid might be ESM only | ||
| // Simple secure random token generator | ||
| const generateToken = () => { | ||
| // Use Web Crypto API which is available in Node.js global scope in newer versions | ||
| // or fall back to Math.random for older environments if critical | ||
| if (typeof crypto !== 'undefined') { | ||
| const array = new Uint8Array(24); | ||
| crypto.getRandomValues(array); | ||
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); | ||
| } else { | ||
| // Fallback for environments without crypto global | ||
| return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the runtime configuration and examine the file context
rg -n "export const runtime" src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
# Also check the full structure of the file around the problematic code
head -n 220 src/app/api/workspaces/[id]/collaborators/route.ts | tail -n 30Repository: ThinkEx-OSS/thinkex
Length of output: 1325
🏁 Script executed:
# Check if there are any existing imports from crypto
rg -n "import.*crypto" src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
wc -l src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 117
🏁 Script executed:
# Read the beginning of the file to see imports and understand the context
head -n 50 src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 1628
🏁 Script executed:
# Check if next.config file has any edge runtime configuration that might affect this route
find . -name "next.config.*" -type f | head -5Repository: ThinkEx-OSS/thinkex
Length of output: 79
🏁 Script executed:
# Read the full route file to check for runtime configuration
cat -n src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 11627
🏁 Script executed:
# Check next.config for any edge runtime settings
cat next.config.tsRepository: ThinkEx-OSS/thinkex
Length of output: 1208
Remove the insecure Math.random fallback from invite token generation.
The fallback to Math.random() is cryptographically insecure and should never be used for generating security-sensitive tokens like invitations. Since this is a Node.js API route (no edge runtime is configured), use crypto.randomBytes() unconditionally instead of the defensive Web Crypto API check with unsafe fallback.
🔐 Suggested fix
+import { randomBytes } from "crypto";
...
- const generateToken = () => {
- // Use Web Crypto API which is available in Node.js global scope in newer versions
- // or fall back to Math.random for older environments if critical
- if (typeof crypto !== 'undefined') {
- const array = new Uint8Array(24);
- crypto.getRandomValues(array);
- return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join('');
- } else {
- // Fallback for environments without crypto global
- return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15);
- }
- };
+ const generateToken = () => randomBytes(24).toString("hex");📝 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.
| // Import nanoid dynamically or use a simple random string generator since nanoid might be ESM only | |
| // Simple secure random token generator | |
| const generateToken = () => { | |
| // Use Web Crypto API which is available in Node.js global scope in newer versions | |
| // or fall back to Math.random for older environments if critical | |
| if (typeof crypto !== 'undefined') { | |
| const array = new Uint8Array(24); | |
| crypto.getRandomValues(array); | |
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); | |
| } else { | |
| // Fallback for environments without crypto global | |
| return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); | |
| } | |
| import { randomBytes } from "crypto"; | |
| // Import nanoid dynamically or use a simple random string generator since nanoid might be ESM only | |
| // Simple secure random token generator | |
| const generateToken = () => randomBytes(24).toString("hex"); |
🤖 Prompt for AI Agents
In `@src/app/api/workspaces/`[id]/collaborators/route.ts around lines 204 - 216,
The generateToken function currently falls back to Math.random which is
insecure; replace its implementation to use Node's crypto.randomBytes
unconditionally (e.g., call crypto.randomBytes(24).toString('hex') or import
randomBytes from 'crypto') for invite token generation in the API route; remove
the Web Crypto + Math.random branch and ensure the code uses the Node crypto API
only so tokens are cryptographically secure.
| let redirectTo = redirect_url || callbackUrl; | ||
|
|
||
| // For sign-up, always route through onboarding, but preserve the final destination | ||
| if (path === "sign-up") { | ||
| if (redirectTo) { | ||
| redirectTo = `/onboarding?redirect_url=${encodeURIComponent(redirectTo)}`; | ||
| } else { | ||
| redirectTo = "/onboarding"; | ||
| } | ||
| } else if (!redirectTo) { | ||
| // For sign-in without a specific target, go to home | ||
| redirectTo = "/home"; |
There was a problem hiding this comment.
Sanitize redirect targets to prevent open redirects.
Line 25 now accepts callbackUrl as a redirect target without validation. This allows external/protocol-relative redirects. Please restrict redirects to safe internal paths.
🔒 Suggested fix
- let redirectTo = redirect_url || callbackUrl;
+ const sanitizeRedirect = (value?: string) => {
+ if (!value) return undefined;
+ // allow only internal, absolute paths
+ if (value.startsWith("/") && !value.startsWith("//")) return value;
+ return undefined;
+ };
+
+ let redirectTo = sanitizeRedirect(redirect_url || callbackUrl);📝 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.
| let redirectTo = redirect_url || callbackUrl; | |
| // For sign-up, always route through onboarding, but preserve the final destination | |
| if (path === "sign-up") { | |
| if (redirectTo) { | |
| redirectTo = `/onboarding?redirect_url=${encodeURIComponent(redirectTo)}`; | |
| } else { | |
| redirectTo = "/onboarding"; | |
| } | |
| } else if (!redirectTo) { | |
| // For sign-in without a specific target, go to home | |
| redirectTo = "/home"; | |
| const sanitizeRedirect = (value?: string) => { | |
| if (!value) return undefined; | |
| // allow only internal, absolute paths | |
| if (value.startsWith("/") && !value.startsWith("//")) return value; | |
| return undefined; | |
| }; | |
| let redirectTo = sanitizeRedirect(redirect_url || callbackUrl); | |
| // For sign-up, always route through onboarding, but preserve the final destination | |
| if (path === "sign-up") { | |
| if (redirectTo) { | |
| redirectTo = `/onboarding?redirect_url=${encodeURIComponent(redirectTo)}`; | |
| } else { | |
| redirectTo = "/onboarding"; | |
| } | |
| } else if (!redirectTo) { | |
| // For sign-in without a specific target, go to home | |
| redirectTo = "/home"; |
🤖 Prompt for AI Agents
In `@src/app/auth/`[path]/page.tsx around lines 25 - 36, The redirect target logic
currently assigns redirect_url or callbackUrl directly to redirectTo (variables:
redirectTo, redirect_url, callbackUrl, path) which allows external or
protocol-relative open redirects; add a validation step (e.g., an isSafeRedirect
helper) that ensures the chosen redirect is an internal path only — require it
to start with a single slash ("/") and explicitly reject values that start with
"//" or include a protocol ("://"); use this validator before encoding/embedding
into `/onboarding?redirect_url=...` and if validation fails fall back to a safe
default like "/home" or "/" so that only validated internal routes are used for
redirectTo.
| user: { | ||
| deleteUser: { | ||
| enabled: true, | ||
| afterDelete: async (user) => { | ||
| // Cleanup workspaces since there is no cascade constraint in the DB | ||
| try { | ||
| await db.delete(workspaces).where(eq(workspaces.userId, user.id)); | ||
| } catch (error) { | ||
| console.error("Failed to cleanup user workspaces:", error); | ||
| } |
There was a problem hiding this comment.
Account deletion cleanup shouldn’t silently fail.
Line 71–75 logs and continues if workspace deletion fails, which can leave user-owned content behind after account deletion (data retention/compliance risk). Consider enforcing DB-level cascades or making cleanup failures actionable (e.g., throw to abort if possible, or enqueue a guaranteed retry + alert). Also verify whether other user-linked tables (collaborators/invites/events) need explicit cleanup to avoid orphaned records.
🤖 Prompt for AI Agents
In `@src/lib/auth.ts` around lines 66 - 75, The afterDelete hook for deleteUser
currently swallows failures when running
db.delete(workspaces).where(eq(workspaces.userId, user.id)), which can leave
orphaned user data; update deleteUser.afterDelete to make workspace cleanup
actionable: either ensure DB cascade constraints on workspaces.userId and remove
this manual delete, or if keeping manual cleanup, propagate the error (rethrow)
so account deletion aborts on failure, and/or enqueue a retry + emit an
alert/metric when the delete fails; also audit and add similar cleanup (or
cascades) for other user-linked tables such as collaborators, invites, and
events to avoid orphaned records.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1f7dbec in 17 seconds. Click for details.
- Reviewed
562lines of code in7files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_3Owm8U7mTWoc0uaN
You can customize
| import WorkspaceSettingsModal from "@/components/workspace/WorkspaceSettingsModal"; | ||
| import ShareWorkspaceDialog from "@/components/workspace/ShareWorkspaceDialog"; | ||
| import { RealtimeProvider } from "@/contexts/RealtimeContext"; | ||
| import { toast } from "sonner"; |
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/app/onboarding/page.tsx">
<violation number="1" location="src/app/onboarding/page.tsx:61">
P1: The `redirectUrl` is used in `router.replace` without validation, potentially allowing Open Redirects to external sites (e.g. `https://evil.com` which passes the negative checks).
While enabling workspace redirects is the goal, it is recommended to validate that `redirectUrl` is a relative path (starts with `/` and not `//`) to prevent open redirects. This replaces the removed line with a safety check that still allows `/workspace` paths.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if ( | ||
| redirectUrl && | ||
| !redirectUrl.startsWith("/workspace") && | ||
| !redirectUrl.startsWith("/dashboard") && |
There was a problem hiding this comment.
P1: The redirectUrl is used in router.replace without validation, potentially allowing Open Redirects to external sites (e.g. https://evil.com which passes the negative checks).
While enabling workspace redirects is the goal, it is recommended to validate that redirectUrl is a relative path (starts with / and not //) to prevent open redirects. This replaces the removed line with a safety check that still allows /workspace paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/onboarding/page.tsx, line 61:
<comment>The `redirectUrl` is used in `router.replace` without validation, potentially allowing Open Redirects to external sites (e.g. `https://evil.com` which passes the negative checks).
While enabling workspace redirects is the goal, it is recommended to validate that `redirectUrl` is a relative path (starts with `/` and not `//`) to prevent open redirects. This replaces the removed line with a safety check that still allows `/workspace` paths.</comment>
<file context>
@@ -58,7 +58,6 @@ export default function OnboardingPage() {
if (
redirectUrl &&
- !redirectUrl.startsWith("/workspace") &&
!redirectUrl.startsWith("/dashboard") &&
!redirectUrl.startsWith("/onboarding") &&
!redirectUrl.startsWith("/home")
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/api/workspaces/`[id]/collaborators/route.ts:
- Around line 72-83: The current code always queries and returns
workspaceInvites (the invites variable), which exposes invitee emails to
viewers; update the route handler in route.ts to first determine the requester's
permission for the workspace (e.g., call or implement a
getWorkspacePermission(userId, workspaceId) or reuse existing permission check)
and only run the workspaceInvites query (the select from workspaceInvites) when
the permissionLevel is owner/editor; for viewer-level users return an empty
invites list or omit invites from the response. Ensure you reference
workspaceId, workspaceInvites, and the invites variable when gating the database
query so emails are never returned to viewers.
In `@src/components/workspace/ShareWorkspaceDialog.tsx`:
- Around line 84-87: The current isRevoking state only holds a single id causing
races; change its type to a collection (e.g., useState<Set<string>> or
useState<string[]>) and update all usages (isRevoking, setIsRevoking) so that
starting a revoke adds the invite id to the set and finishing/removing the
revoke deletes that id (preserve immutability: create a new Set/array when
updating). Also update the UI checks (e.g., button disabled or spinner
visibility) from isRevoking === id to collection.has(id) or
collection.includes(id), and ensure the error/cleanup paths remove the id on
both success and failure; apply the same pattern wherever isRevoking is
referenced (including the other revoke locations noted).
🧹 Nitpick comments (1)
src/components/workspace/ShareWorkspaceDialog.tsx (1)
33-40: NarrowPendingInvite.permissionLevelto known values.Using
stringweakens type safety; align it with the collaborator permission union.♻️ Suggested change
interface PendingInvite { id: string; email: string; - permissionLevel: string; + permissionLevel: "viewer" | "editor"; createdAt: string; expiresAt: string; inviterId: string; }
| // Get pending invites | ||
| const invites = await db | ||
| .select({ | ||
| id: workspaceInvites.id, | ||
| email: workspaceInvites.email, | ||
| permissionLevel: workspaceInvites.permissionLevel, | ||
| createdAt: workspaceInvites.createdAt, | ||
| expiresAt: workspaceInvites.expiresAt, | ||
| inviterId: workspaceInvites.inviterId, | ||
| }) | ||
| .from(workspaceInvites) | ||
| .where(eq(workspaceInvites.workspaceId, workspaceId)); |
There was a problem hiding this comment.
Don’t expose pending invite emails to viewers.
This endpoint allows “viewer” access, but returning invites leaks invitee emails to users who cannot manage access. Consider only returning invites to owners/editors.
🔐 Suggested change (gate invite visibility)
- const invites = await db
- .select({
- id: workspaceInvites.id,
- email: workspaceInvites.email,
- permissionLevel: workspaceInvites.permissionLevel,
- createdAt: workspaceInvites.createdAt,
- expiresAt: workspaceInvites.expiresAt,
- inviterId: workspaceInvites.inviterId,
- })
- .from(workspaceInvites)
- .where(eq(workspaceInvites.workspaceId, workspaceId));
+ let invites: Array<{
+ id: string;
+ email: string;
+ permissionLevel: string;
+ createdAt: string;
+ expiresAt: string;
+ inviterId: string;
+ }> = [];
+ try {
+ await verifyWorkspaceAccess(workspaceId, userId, "editor");
+ invites = await db
+ .select({
+ id: workspaceInvites.id,
+ email: workspaceInvites.email,
+ permissionLevel: workspaceInvites.permissionLevel,
+ createdAt: workspaceInvites.createdAt,
+ expiresAt: workspaceInvites.expiresAt,
+ inviterId: workspaceInvites.inviterId,
+ })
+ .from(workspaceInvites)
+ .where(eq(workspaceInvites.workspaceId, workspaceId));
+ } catch {
+ invites = [];
+ }Also applies to: 99-99
🤖 Prompt for AI Agents
In `@src/app/api/workspaces/`[id]/collaborators/route.ts around lines 72 - 83, The
current code always queries and returns workspaceInvites (the invites variable),
which exposes invitee emails to viewers; update the route handler in route.ts to
first determine the requester's permission for the workspace (e.g., call or
implement a getWorkspacePermission(userId, workspaceId) or reuse existing
permission check) and only run the workspaceInvites query (the select from
workspaceInvites) when the permissionLevel is owner/editor; for viewer-level
users return an empty invites list or omit invites from the response. Ensure you
reference workspaceId, workspaceInvites, and the invites variable when gating
the database query so emails are never returned to viewers.
| // Pending Invites State | ||
| const [invites, setInvites] = useState<PendingInvite[]>([]); | ||
| const [isRevoking, setIsRevoking] = useState<string | null>(null); | ||
|
|
There was a problem hiding this comment.
Track revocations per invite to avoid spinner/enablement races.
isRevoking holds only one id, so a second revoke can clear the first one’s loading state while it’s still in-flight.
🛠️ Suggested fix
- const [isRevoking, setIsRevoking] = useState<string | null>(null);
+ const [revokingIds, setRevokingIds] = useState<Set<string>>(new Set());
...
- setIsRevoking(inviteId);
+ setRevokingIds(prev => new Set(prev).add(inviteId));
...
- setIsRevoking(null);
+ setRevokingIds(prev => {
+ const next = new Set(prev);
+ next.delete(inviteId);
+ return next;
+ });
...
- disabled={isRevoking === invite.id}
+ disabled={revokingIds.has(invite.id)}
...
- {isRevoking === invite.id ? (
+ {revokingIds.has(invite.id) ? (Also applies to: 285-303, 525-535
🤖 Prompt for AI Agents
In `@src/components/workspace/ShareWorkspaceDialog.tsx` around lines 84 - 87, The
current isRevoking state only holds a single id causing races; change its type
to a collection (e.g., useState<Set<string>> or useState<string[]>) and update
all usages (isRevoking, setIsRevoking) so that starting a revoke adds the invite
id to the set and finishing/removing the revoke deletes that id (preserve
immutability: create a new Set/array when updating). Also update the UI checks
(e.g., button disabled or spinner visibility) from isRevoking === id to
collection.has(id) or collection.includes(id), and ensure the error/cleanup
paths remove the id on both success and failure; apply the same pattern wherever
isRevoking is referenced (including the other revoke locations noted).
|
@CodeRabbit check this pr |
|
I'll analyze the implementation across backend, database, and frontend changes to ensure everything is working correctly and identify any potential issues or improvements. ✅ Actions performedReview triggered.
|
| !isLoadingWorkspace && !loadingWorkspaces && !currentWorkspaceId ? ( | ||
| session?.user?.isAnonymous ? ( | ||
| <LoginGate /> | ||
| ) : useSearchParams().get('invite') ? ( | ||
| // If we have an invite query param, show skeleton instead of Access Denied | ||
| // This handles the race condition where workspace fetch 404s before claim completes |
There was a problem hiding this comment.
Invalid hook call path
useSearchParams() is being called inside a conditional render branch (... ? useSearchParams().get('invite') ? ...), which violates React’s Rules of Hooks and will throw at runtime when that branch is reached. This needs to be moved to the top-level of the component (call once, then reference the value in conditionals) so hook order is consistent across renders.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/workspace-canvas/WorkspaceSection.tsx
Line: 433:438
Comment:
**Invalid hook call path**
`useSearchParams()` is being called inside a conditional render branch (`... ? useSearchParams().get('invite') ? ...`), which violates React’s Rules of Hooks and will throw at runtime when that branch is reached. This needs to be moved to the top-level of the component (call once, then reference the value in conditionals) so hook order is consistent across renders.
How can I resolve this? If you propose a fix, please make it concise.| const generateToken = () => { | ||
| // Use Web Crypto API which is available in Node.js global scope in newer versions | ||
| // or fall back to Math.random for older environments if critical | ||
| if (typeof crypto !== 'undefined') { | ||
| const array = new Uint8Array(24); | ||
| crypto.getRandomValues(array); | ||
| return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); | ||
| } else { | ||
| // Fallback for environments without crypto global | ||
| return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); | ||
| } |
There was a problem hiding this comment.
Insecure invite token fallback
generateToken() falls back to Math.random() when crypto is undefined (lines in the 219-229 range). In any runtime where the global Web Crypto API isn’t present (or is blocked), this produces guessable invite tokens, which would allow unauthorized users to brute-force invite links. Since these tokens gate workspace access, the fallback should be removed and token generation should fail hard (or use Node’s crypto module) rather than degrading to non-cryptographic randomness.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/workspaces/[id]/collaborators/route.ts
Line: 219:229
Comment:
**Insecure invite token fallback**
`generateToken()` falls back to `Math.random()` when `crypto` is undefined (lines in the 219-229 range). In any runtime where the global Web Crypto API isn’t present (or is blocked), this produces guessable invite tokens, which would allow unauthorized users to brute-force invite links. Since these tokens gate workspace access, the fallback should be removed and token generation should fail hard (or use Node’s `crypto` module) rather than degrading to non-cryptographic randomness.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 125373c in 18 seconds. Click for details.
- Reviewed
549lines of code in6files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_dZjYq9xPr63BwWxp
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/workspace/use-workspace-mutation.ts (2)
181-195:⚠️ Potential issue | 🟡 MinorPermission toast may be skipped due to early return.
The early return at line 181 (
if (!workspaceId || !context?.previous) return;) executes before the permission error toast is shown. IfworkspaceIdis null orcontext.previousis undefined, users won't see the permission denied notification.Move the toast logic before the guard, or restructure to ensure the toast always fires for permission errors:
🐛 Proposed fix
onError: (err, event, context) => { logger.error("❌ [MUTATION] Event mutation failed:", err); logger.error("❌ [MUTATION] Failed event:", event); logger.error("❌ [MUTATION] Error details:", { message: err instanceof Error ? err.message : String(err), workspaceId, }); - if (!workspaceId || !context?.previous) return; - // Show toast for permission errors if (err.message === "PERMISSION_DENIED") { toast.error("You don't have permission to edit this workspace"); - } else { - // Only show generic error for non-permission issues to avoid noise - // toast.error("Failed to save changes"); } + if (!workspaceId || !context?.previous) return; + // Rollback to previous state queryClient.setQueryData( ["workspace", workspaceId, "events"], context.previous ); },
326-351:⚠️ Potential issue | 🟡 MinorRetry error handler doesn't show permission toast.
When the retry at line 268 fails with a permission error, the
.catch()handler logs and rolls back but doesn't show a toast to the user. This creates inconsistent UX where initial permission failures show a toast but retry failures don't.🐛 Proposed fix to add permission toast in retry error handler
}).catch((err) => { logger.error("❌ [RETRY] Retry failed with error:", err); + // Show toast for permission errors + if (err.message === "PERMISSION_DENIED") { + toast.error("You don't have permission to edit this workspace"); + } + // Remove optimistic event queryClient.setQueryData<EventResponse>(
🤖 Fix all issues with AI agents
In `@src/app/auth/`[path]/page.tsx:
- Around line 59-60: The query that fetches invites using workspaceInvites.token
and inviteToken should also filter out expired invites by adding a condition on
workspaceInvites.expiresAt (e.g., require expiresAt to be greater than the
current time); update the query chain that currently calls
.where(eq(workspaceInvites.token, inviteToken)).limit(1) to include an
additional where/compare (using expiresAt and the appropriate GT/greater-than
comparator or timestamp check) so only non-expired invites are returned.
🧹 Nitpick comments (6)
src/hooks/workspace/use-workspace-mutation.ts (1)
44-46: Consider using a custom error class for type-safe error handling.Using a magic string
"PERMISSION_DENIED"as an error message and later comparing against it (line 184) is fragile. A custom error class would provide type safety and better extensibility.♻️ Optional refactor using custom error class
// At top of file or in a shared errors module class PermissionDeniedError extends Error { constructor() { super("You don't have permission to edit this workspace"); this.name = "PermissionDeniedError"; } }Then in
appendWorkspaceEvent:if (response.status === 403) { - throw new Error("PERMISSION_DENIED"); + throw new PermissionDeniedError(); }And in
onError:- if (err.message === "PERMISSION_DENIED") { + if (err instanceof PermissionDeniedError) { toast.error("You don't have permission to edit this workspace"); }src/components/workspace/InviteGuard.tsx (1)
21-64: Consider adding an abort controller to prevent state updates after unmount.The
claimInviteasync function lacks cleanup handling. If the component unmounts while the fetch is in-flight (e.g., user navigates away),router.replacecould be called on an unmounted component. Additionally, ifsessionchanges rapidly, multiple claim requests could fire.♻️ Suggested fix with AbortController
useEffect(() => { + const controller = new AbortController(); + let isMounted = true; + async function claimInvite() { if (!inviteToken || !session?.user || session.user.isAnonymous || isSessionLoading) return; try { const res = await fetch('/api/invites/claim', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ token: inviteToken }) + body: JSON.stringify({ token: inviteToken }), + signal: controller.signal }); + if (!isMounted) return; + const data = await res.json(); // ... rest of the handler } catch (e) { + if (e instanceof DOMException && e.name === 'AbortError') return; console.error(e); // ... } } claimInvite(); + + return () => { + isMounted = false; + controller.abort(); + }; }, [inviteToken, session, isSessionLoading, router]);src/components/workspace/ShareWorkspaceDialog.tsx (3)
40-47: Consider using a stricter type forpermissionLevel.
PendingInvite.permissionLevelis typed asstring, whileCollaborator.permissionLeveluses"viewer" | "editor" | "owner". For consistency and type safety, consider using the same union type.♻️ Suggested fix
interface PendingInvite { id: string; email: string; - permissionLevel: string; + permissionLevel: "viewer" | "editor"; createdAt: string; expiresAt: string; inviterId: string; }
308-309: Add error logging in the catch block.The caught error
eis not logged, making it harder to debug failures in production.♻️ Suggested fix
} catch (e) { + console.error("Failed to revoke invite:", e); toast.error("Failed to revoke invite"); } finally {
112-123: Missing function dependencies in useEffect.The
useEffectcallsloadCollaborators()andloadFrequentCollaborators()but these functions aren't included in the dependency array. While this works in practice (the functions are stable), it may trigger ESLint warnings. Consider wrapping them withuseCallbackor disabling the lint rule with an explanation.src/app/auth/[path]/page.tsx (1)
53-73: Consider combining sequential queries with a JOIN.Three sequential database queries add SSR latency. These could be combined into a single query with JOINs.
♻️ Suggested optimization
- const [invite] = await db - .select({ - workspaceId: workspaceInvites.workspaceId, - inviterId: workspaceInvites.inviterId, - }) - .from(workspaceInvites) - .where(eq(workspaceInvites.token, inviteToken)) - .limit(1); - - if (invite) { - const [workspace] = await db - .select({ name: workspaces.name }) - .from(workspaces) - .where(eq(workspaces.id, invite.workspaceId)) - .limit(1); - - const [inviter] = await db - .select({ name: user.name, email: user.email }) - .from(user) - .where(eq(user.id, invite.inviterId)) - .limit(1); + const [result] = await db + .select({ + workspaceName: workspaces.name, + inviterName: user.name, + inviterEmail: user.email, + }) + .from(workspaceInvites) + .innerJoin(workspaces, eq(workspaces.id, workspaceInvites.workspaceId)) + .innerJoin(user, eq(user.id, workspaceInvites.inviterId)) + .where(eq(workspaceInvites.token, inviteToken)) + .limit(1); + + if (result) { + const workspaceName = result.workspaceName || "a workspace"; + const inviterName = result.inviterName || result.inviterEmail || "someone";
| .where(eq(workspaceInvites.token, inviteToken)) | ||
| .limit(1); |
There was a problem hiding this comment.
Consider filtering out expired invites.
The query doesn't check expiresAt, so expired invites will still display personalized invite messaging. This could confuse users who see "You've been invited!" but then fail to claim.
🛡️ Suggested fix
+import { eq, and, gt } from "drizzle-orm";
...
.from(workspaceInvites)
- .where(eq(workspaceInvites.token, inviteToken))
+ .where(and(
+ eq(workspaceInvites.token, inviteToken),
+ gt(workspaceInvites.expiresAt, new Date())
+ ))
.limit(1);📝 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.
| .where(eq(workspaceInvites.token, inviteToken)) | |
| .limit(1); | |
| .where(and( | |
| eq(workspaceInvites.token, inviteToken), | |
| gt(workspaceInvites.expiresAt, new Date()) | |
| )) | |
| .limit(1); |
🤖 Prompt for AI Agents
In `@src/app/auth/`[path]/page.tsx around lines 59 - 60, The query that fetches
invites using workspaceInvites.token and inviteToken should also filter out
expired invites by adding a condition on workspaceInvites.expiresAt (e.g.,
require expiresAt to be greater than the current time); update the query chain
that currently calls .where(eq(workspaceInvites.token, inviteToken)).limit(1) to
include an additional where/compare (using expiresAt and the appropriate
GT/greater-than comparator or timestamp check) so only non-expired invites are
returned.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7655c58 in 17 seconds. Click for details.
- Reviewed
54lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_xx8caBZJ0xvo8Eni
You can customize
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/workspace/InviteGuard.tsx">
<violation number="1" location="src/components/workspace/InviteGuard.tsx:83">
P2: Triggering a route change (`router.replace`) directly in the component's render body is a side effect, which violates React's purity rules and can lead to errors (e.g., "Cannot update a component while rendering a different component").
**Recommended Fix:**
Move the redirection logic into a `useEffect` hook at the top level of the component:
```typescript
useEffect(() => {
if (!isSessionLoading && inviteToken && (!session || session.user?.isAnonymous)) {
const currentUrl = window.location.href;
router.replace(`/auth/sign-up?invite=${inviteToken}&redirect_url=${encodeURIComponent(currentUrl)}`);
}
}, [isSessionLoading, inviteToken, session, router]);
Then, update the render logic to show a loading state instead of returning null:
if (!session || session.user?.isAnonymous) {
return (
<div className="flex h-screen w-full items-center justify-center bg-black/95">
<Loader2 className="h-8 w-8 animate-spin text-white/50" />
</div>
);
}
```</violation>
</file>
<file name="src/app/auth/[path]/page.tsx">
<violation number="1" location="src/app/auth/[path]/page.tsx:59">
P2: The invite query should filter out expired invites to prevent showing "You've been invited!" messaging for invites that will fail when claimed. Add a check for `expiresAt > new Date()` to the where clause.</violation>
</file>Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Determine if we should send to sign-in or sign-up (default to sign-up for new invites) | ||
| // We pass the invite token so the auth page can show the custom header | ||
| // We pass redirect_url so they come back here to claim the invite after auth | ||
| router.replace(`/auth/sign-up?invite=${inviteToken}&redirect_url=${encodeURIComponent(currentUrl)}`); |
There was a problem hiding this comment.
P2: Triggering a route change (router.replace) directly in the component's render body is a side effect, which violates React's purity rules and can lead to errors (e.g., "Cannot update a component while rendering a different component").
Recommended Fix:
Move the redirection logic into a useEffect hook at the top level of the component:
useEffect(() => {
if (!isSessionLoading && inviteToken && (!session || session.user?.isAnonymous)) {
const currentUrl = window.location.href;
router.replace(`/auth/sign-up?invite=${inviteToken}&redirect_url=${encodeURIComponent(currentUrl)}`);
}
}, [isSessionLoading, inviteToken, session, router]);Then, update the render logic to show a loading state instead of returning null:
if (!session || session.user?.isAnonymous) {
return (
<div className="flex h-screen w-full items-center justify-center bg-black/95">
<Loader2 className="h-8 w-8 animate-spin text-white/50" />
</div>
);
}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/workspace/InviteGuard.tsx, line 83:
<comment>Triggering a route change (`router.replace`) directly in the component's render body is a side effect, which violates React's purity rules and can lead to errors (e.g., "Cannot update a component while rendering a different component").
**Recommended Fix:**
Move the redirection logic into a `useEffect` hook at the top level of the component:
```typescript
useEffect(() => {
if (!isSessionLoading && inviteToken && (!session || session.user?.isAnonymous)) {
const currentUrl = window.location.href;
router.replace(`/auth/sign-up?invite=${inviteToken}&redirect_url=${encodeURIComponent(currentUrl)}`);
}
}, [isSessionLoading, inviteToken, session, router]);
Then, update the render logic to show a loading state instead of returning null:
if (!session || session.user?.isAnonymous) {
return (
<div className="flex h-screen w-full items-center justify-center bg-black/95">
<Loader2 className="h-8 w-8 animate-spin text-white/50" />
</div>
);
}
```</comment>
<file context>
@@ -75,9 +74,14 @@ export function InviteGuard({ children }: InviteGuardProps) {
+ // Determine if we should send to sign-in or sign-up (default to sign-up for new invites)
+ // We pass the invite token so the auth page can show the custom header
+ // We pass redirect_url so they come back here to claim the invite after auth
+ router.replace(`/auth/sign-up?invite=${inviteToken}&redirect_url=${encodeURIComponent(currentUrl)}`);
+ return null; // Don't render anything while redirecting
}
</file context>
| inviterId: workspaceInvites.inviterId, | ||
| }) | ||
| .from(workspaceInvites) | ||
| .where(eq(workspaceInvites.token, inviteToken)) |
There was a problem hiding this comment.
P2: The invite query should filter out expired invites to prevent showing "You've been invited!" messaging for invites that will fail when claimed. Add a check for expiresAt > new Date() to the where clause.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/auth/[path]/page.tsx, line 59:
<comment>The invite query should filter out expired invites to prevent showing "You've been invited!" messaging for invites that will fail when claimed. Add a check for `expiresAt > new Date()` to the where clause.</comment>
<file context>
@@ -36,9 +39,49 @@ export default async function AuthPage({
+ inviterId: workspaceInvites.inviterId,
+ })
+ .from(workspaceInvites)
+ .where(eq(workspaceInvites.token, inviteToken))
+ .limit(1);
+
</file context>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d23ad57 in 18 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_gb9uPlSdLMakIIvF
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d8cedd2 in 18 seconds. Click for details.
- Reviewed
122lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_l5HHLsDaKk1PD1Xw
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3161d49 in 14 seconds. Click for details.
- Reviewed
588lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_Hpfu2yWT8kQ1wKsU
You can customize
|
1 participant
Important
This pull request adds comprehensive workspace invitation management, including sending, claiming, and revoking invites, with integrated UI and API support.
route.ts,ShareWorkspaceDialog.tsx).route.ts,InviteGuard.tsx).AccountModal.tsx).GET /api/invites/[token]: Retrieve invite details.POST /api/invites/claim: Claim an invite.DELETE /api/workspaces/[id]/invites/[inviteId]: Revoke an invite.GET /api/workspaces/[id]/collaborators: List collaborators and pending invites.POST /api/workspaces/[id]/collaborators: Invite a new collaborator.InviteGuard: Handles invite token processing and redirects.ShareWorkspaceDialog: Manages inviting collaborators and viewing pending invites.AuthForms: Updated to handle invite context during sign-up.workspaceInvitestable to manage invite tokens and expiration.invite-email.tsx).page.tsx).This description was created by
for 3161d49. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Refactor
Style