feat: add leave workspace for shared collaborators#473
feat: add leave workspace for shared collaborators#473samvitchoudhary wants to merge 2 commits intoThinkEx-OSS:mainfrom
Conversation
|
@samvitchoudhary is attempting to deploy a commit to the ThinkEx Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 Walkthrough🚥 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 |
Greptile SummaryThis PR adds a "Leave Workspace" flow for collaborators (not owners): a new Confidence Score: 4/5Safe to merge; all findings are P2 quality/UX suggestions with no breaking logic errors. No P0 or P1 issues found. Three P2 findings: workspace-ID enumeration via distinct 404 responses, generic server error swallowing in the mutation, and the AlertDialogAction auto-close behaviour masking the loading/error state in the confirmation dialog. The 404 response distinction in
|
| Filename | Overview |
|---|---|
| src/app/api/workspaces/[id]/leave/route.ts | New POST endpoint for collaborator self-removal; auth, owner-guard, and DB delete are all correct, but the two distinct 404 responses allow workspace-ID enumeration by any authenticated user. |
| src/contexts/WorkspaceContext.tsx | Adds leaveWorkspace mutation mirroring deleteWorkspace; cache update and redirect logic are correct; server error body is not surfaced to the caller. |
| src/components/workspace/WorkspaceSettingsModal.tsx | Branches Danger Zone on isShared, adds Leave confirmation dialog; AlertDialogAction auto-close means spinner/error state inside the dialog are not reliably shown. |
Reviews (1): Last reviewed commit: "feat: add leave workspace for shared col..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!deleted) { | ||
| return NextResponse.json( | ||
| { error: "You are not a collaborator of this workspace." }, | ||
| { status: 404 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
The endpoint returns a 404 with "Workspace not found" when the workspace doesn't exist, and a 404 with "You are not a collaborator of this workspace." when the user is just not a member. Any authenticated user can distinguish these two cases and enumerate valid workspace IDs by probing this endpoint — they never need to be a collaborator. Consider returning the same 404 body in both cases ("Workspace not found or you are not a collaborator") to avoid the distinction.
| if (!response.ok) { | ||
| throw new Error("Failed to leave workspace"); | ||
| } |
There was a problem hiding this comment.
When response.ok is false, the mutation throws a generic "Failed to leave workspace" string and ignores the server's JSON body. The API returns useful messages (e.g. "Workspace owners cannot leave their own workspace..." for a 400), but those are silently swallowed. The onError toast also shows the same generic message. Consider reading response.json() and including the server's error field in the thrown Error, so the handleLeaveConfirm catch block (and the toast) can surface the real reason to the user.
| <AlertDialogCancel disabled={isLeaving}>Cancel</AlertDialogCancel> | ||
| <AlertDialogAction | ||
| onClick={handleLeaveConfirm} | ||
| className="bg-destructive text-destructive-foreground hover:bg-destructive/90" | ||
| disabled={isLeaving} | ||
| > | ||
| {isLeaving && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} | ||
| Leave |
There was a problem hiding this comment.
AlertDialogAction closes the dialog before the async handler resolves
Radix UI's AlertDialogAction calls onOpenChange(false) synchronously on click (before onClick resolves), so the confirmation dialog closes immediately. The loading spinner (isLeaving) and the disabled={isLeaving} guard on the Cancel button inside the dialog are both rendered invisible before the fetch completes. This mirrors the existing delete-dialog pattern, but it means any error from handleLeaveConfirm silently surfaces in the outer settings modal's error state rather than inside the confirmation dialog where the user expects it. Consider using a plain Button instead of AlertDialogAction, or call event.preventDefault() and gate dialog close on completion.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/contexts/WorkspaceContext.tsx (1)
158-200: Delete/leave mutation success paths are now duplicated.Both mutations implement nearly identical cache pruning + redirect logic. Extracting one shared helper would reduce risk of behavior drift.
Also applies to: 210-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/WorkspaceContext.tsx` around lines 158 - 200, The delete and leave mutation handlers duplicate the same cache-pruning and redirect logic; extract a shared helper (e.g., pruneWorkspacesAfterRemoval or handleWorkspaceRemoval) that accepts the removedWorkspaceId and performs the queryClient.setQueryData update, checks currentWorkspaceId/currentSlug, calls switchWorkspace or router.push("/home") as needed, and shows the toast; then replace the inline onSuccess blocks in deleteWorkspaceMutation and the corresponding leave mutation (lines ~210-258) to call this helper so behavior is centralized and de-duplicated.src/components/workspace/WorkspaceSettingsModal.tsx (1)
163-184: Consider extracting a reusable destructive-action section/dialog.The delete/leave flows now duplicate very similar handler + card + dialog structures. A small reusable component/helper would reduce drift and future maintenance overhead.
Also applies to: 289-328, 344-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/workspace/WorkspaceSettingsModal.tsx` around lines 163 - 184, Extract the repeated delete/leave UI and logic into a reusable DestructiveAction component that encapsulates the dialog/card, confirm/cancel buttons and handles loading/error state; replace the duplicated code in WorkspaceSettingsModal by having handleLeaveClick call setShowLeaveDialog on the DestructiveAction and move the async confirm logic (currently in handleLeaveConfirm) into a prop callback (e.g., onConfirm) that invokes leaveWorkspace(workspace.id), toggles isLeaving via a passed setIsLoading or internal state, closes the modal via onOpenChange(false), calls onUpdate() if provided, and surfaces errors via a passed onError or returned promise; reuse the new component for the other similar flows (the handlers and state setters setShowLeaveDialog, setIsLeaving, setError, and functions leaveWorkspace, onOpenChange, onUpdate are the key symbols to wire into the reusable component).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/workspaces/`[id]/leave/route.ts:
- Around line 25-32: The handler currently uses params' workspaceId directly in
the query (const { id: workspaceId } = await params) which can throw on
malformed UUIDs; add an early validation step to check that workspaceId is a
valid UUID (before calling db.select / eq(workspaces.id, workspaceId)) and
return a 400 response when invalid. Locate the route function that reads params
and uses db.select({ userId: workspaces.userId
}).from(workspaces).where(eq(workspaces.id, workspaceId)) and insert a
validation using your project's UUID utility or a simple regex/validator,
returning a 400 error if validation fails so the DB query is never executed with
a bad ID.
In `@src/contexts/WorkspaceContext.tsx`:
- Around line 226-240: The early return on null cache prevents the redirect when
the workspaces list isn't loaded; update the updater to compute
remainingWorkspaces using const existing = old || []; (i.e. don't return early),
then perform the redirect logic that checks leftWorkspaceId ===
currentWorkspaceId and currentSlug and calls
switchWorkspace(remainingWorkspaces[0].slug || remainingWorkspaces[0].id) or
router.push("/home") using remainingWorkspaces, and finally return
remainingWorkspaces; reference the updater closure variables leftWorkspaceId,
currentWorkspaceId, currentSlug, switchWorkspace, router.push and ensure the
function returns the updated array instead of returning early when old is
undefined.
- Around line 223-247: The onSuccess handler updates the ["workspaces"] list but
doesn't evict per-workspace queries so stale ["workspace-by-slug", ...] data can
persist; inside the same onSuccess (where queryClient.setQueryData is used) find
the removed workspace from the old array by matching leftWorkspaceId (e.g.,
const removed = old.find(w => w.id === leftWorkspaceId)), then call
queryClient.removeQueries(["workspace-by-slug", removed.slug]) and also remove
any id-based key you use (e.g., queryClient.removeQueries(["workspace-by-id",
leftWorkspaceId])) before calling switchWorkspace/router.push and toast.success
to ensure per-workspace cache is cleared.
---
Nitpick comments:
In `@src/components/workspace/WorkspaceSettingsModal.tsx`:
- Around line 163-184: Extract the repeated delete/leave UI and logic into a
reusable DestructiveAction component that encapsulates the dialog/card,
confirm/cancel buttons and handles loading/error state; replace the duplicated
code in WorkspaceSettingsModal by having handleLeaveClick call
setShowLeaveDialog on the DestructiveAction and move the async confirm logic
(currently in handleLeaveConfirm) into a prop callback (e.g., onConfirm) that
invokes leaveWorkspace(workspace.id), toggles isLeaving via a passed
setIsLoading or internal state, closes the modal via onOpenChange(false), calls
onUpdate() if provided, and surfaces errors via a passed onError or returned
promise; reuse the new component for the other similar flows (the handlers and
state setters setShowLeaveDialog, setIsLeaving, setError, and functions
leaveWorkspace, onOpenChange, onUpdate are the key symbols to wire into the
reusable component).
In `@src/contexts/WorkspaceContext.tsx`:
- Around line 158-200: The delete and leave mutation handlers duplicate the same
cache-pruning and redirect logic; extract a shared helper (e.g.,
pruneWorkspacesAfterRemoval or handleWorkspaceRemoval) that accepts the
removedWorkspaceId and performs the queryClient.setQueryData update, checks
currentWorkspaceId/currentSlug, calls switchWorkspace or router.push("/home") as
needed, and shows the toast; then replace the inline onSuccess blocks in
deleteWorkspaceMutation and the corresponding leave mutation (lines ~210-258) to
call this helper so behavior is centralized and de-duplicated.
🪄 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: 41807883-2ada-48dd-b089-56829f3e0623
📒 Files selected for processing (3)
src/app/api/workspaces/[id]/leave/route.tssrc/components/workspace/WorkspaceSettingsModal.tsxsrc/contexts/WorkspaceContext.tsx
| onSuccess: (leftWorkspaceId) => { | ||
| queryClient.setQueryData( | ||
| ["workspaces"], | ||
| (old: WorkspaceWithState[] | undefined) => { | ||
| if (!old) return []; | ||
| const remainingWorkspaces = old.filter( | ||
| (w) => w.id !== leftWorkspaceId, | ||
| ); | ||
|
|
||
| if (leftWorkspaceId === currentWorkspaceId && currentSlug) { | ||
| if (remainingWorkspaces.length > 0) { | ||
| switchWorkspace( | ||
| remainingWorkspaces[0].slug || remainingWorkspaces[0].id, | ||
| ); | ||
| } else { | ||
| router.push("/home"); | ||
| } | ||
| } | ||
|
|
||
| return remainingWorkspaces; | ||
| }, | ||
| ); | ||
|
|
||
| toast.success("Left workspace successfully"); | ||
| }, |
There was a problem hiding this comment.
Evict left-workspace metadata cache after successful leave.
Only the ["workspaces"] list is updated. The per-workspace query (["workspace-by-slug", ...]) can remain fresh and serve stale data after leave (especially on back-nav).
Proposed fix
onSuccess: (leftWorkspaceId) => {
+ const previous = queryClient.getQueryData<WorkspaceWithState[]>(["workspaces"]) ?? [];
+ const leftWorkspace = previous.find((w) => w.id === leftWorkspaceId);
+
queryClient.setQueryData(
["workspaces"],
(old: WorkspaceWithState[] | undefined) => {
if (!old) return [];
const remainingWorkspaces = old.filter(
(w) => w.id !== leftWorkspaceId,
);
@@
return remainingWorkspaces;
},
);
+
+ if (leftWorkspace?.slug) {
+ queryClient.removeQueries({
+ queryKey: ["workspace-by-slug", leftWorkspace.slug],
+ exact: true,
+ });
+ }
toast.success("Left workspace successfully");
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contexts/WorkspaceContext.tsx` around lines 223 - 247, The onSuccess
handler updates the ["workspaces"] list but doesn't evict per-workspace queries
so stale ["workspace-by-slug", ...] data can persist; inside the same onSuccess
(where queryClient.setQueryData is used) find the removed workspace from the old
array by matching leftWorkspaceId (e.g., const removed = old.find(w => w.id ===
leftWorkspaceId)), then call queryClient.removeQueries(["workspace-by-slug",
removed.slug]) and also remove any id-based key you use (e.g.,
queryClient.removeQueries(["workspace-by-id", leftWorkspaceId])) before calling
switchWorkspace/router.push and toast.success to ensure per-workspace cache is
cleared.
| (old: WorkspaceWithState[] | undefined) => { | ||
| if (!old) return []; | ||
| const remainingWorkspaces = old.filter( | ||
| (w) => w.id !== leftWorkspaceId, | ||
| ); | ||
|
|
||
| if (leftWorkspaceId === currentWorkspaceId && currentSlug) { | ||
| if (remainingWorkspaces.length > 0) { | ||
| switchWorkspace( | ||
| remainingWorkspaces[0].slug || remainingWorkspaces[0].id, | ||
| ); | ||
| } else { | ||
| router.push("/home"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Redirect logic is skipped when ["workspaces"] cache is empty.
At Line 227, if (!old) return [] exits before the current-workspace redirect check. If the list cache isn’t ready, leaving the active workspace can succeed without navigation fallback.
Proposed fix
queryClient.setQueryData(
["workspaces"],
(old: WorkspaceWithState[] | undefined) => {
- if (!old) return [];
- const remainingWorkspaces = old.filter(
+ const remainingWorkspaces = (old ?? []).filter(
(w) => w.id !== leftWorkspaceId,
);
if (leftWorkspaceId === currentWorkspaceId && currentSlug) {
if (remainingWorkspaces.length > 0) {
switchWorkspace(
remainingWorkspaces[0].slug || remainingWorkspaces[0].id,
);
} else {
router.push("/home");
}
}
return remainingWorkspaces;
},
);📝 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.
| (old: WorkspaceWithState[] | undefined) => { | |
| if (!old) return []; | |
| const remainingWorkspaces = old.filter( | |
| (w) => w.id !== leftWorkspaceId, | |
| ); | |
| if (leftWorkspaceId === currentWorkspaceId && currentSlug) { | |
| if (remainingWorkspaces.length > 0) { | |
| switchWorkspace( | |
| remainingWorkspaces[0].slug || remainingWorkspaces[0].id, | |
| ); | |
| } else { | |
| router.push("/home"); | |
| } | |
| } | |
| (old: WorkspaceWithState[] | undefined) => { | |
| const remainingWorkspaces = (old ?? []).filter( | |
| (w) => w.id !== leftWorkspaceId, | |
| ); | |
| if (leftWorkspaceId === currentWorkspaceId && currentSlug) { | |
| if (remainingWorkspaces.length > 0) { | |
| switchWorkspace( | |
| remainingWorkspaces[0].slug || remainingWorkspaces[0].id, | |
| ); | |
| } else { | |
| router.push("/home"); | |
| } | |
| } | |
| return remainingWorkspaces; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contexts/WorkspaceContext.tsx` around lines 226 - 240, The early return
on null cache prevents the redirect when the workspaces list isn't loaded;
update the updater to compute remainingWorkspaces using const existing = old ||
[]; (i.e. don't return early), then perform the redirect logic that checks
leftWorkspaceId === currentWorkspaceId and currentSlug and calls
switchWorkspace(remainingWorkspaces[0].slug || remainingWorkspaces[0].id) or
router.push("/home") using remainingWorkspaces, and finally return
remainingWorkspaces; reference the updater closure variables leftWorkspaceId,
currentWorkspaceId, currentSlug, switchWorkspace, router.push and ensure the
function returns the updated array instead of returning early when old is
undefined.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/contexts/WorkspaceContext.tsx">
<violation number="1" location="src/contexts/WorkspaceContext.tsx:227">
P2: Early return `if (!old) return []` skips the redirect-if-current-workspace check. If the `["workspaces"]` cache is not yet populated when the leave mutation succeeds, this exits before evaluating whether the user needs to be navigated away. Use `(old ?? []).filter(...)` instead so the redirect logic always executes.</violation>
</file>
<file name="src/app/api/workspaces/[id]/leave/route.ts">
<violation number="1" location="src/app/api/workspaces/[id]/leave/route.ts:35">
P2: The two 404 responses have different error messages (`"Workspace not found"` vs `"You are not a collaborator of this workspace."`), allowing any authenticated user to enumerate valid workspace IDs by probing this endpoint. Use the same message for both cases, e.g. `"Workspace not found or you are not a collaborator"`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Pushed fixes for the three concerns I considered most actionable. Triaged the rest as either pre-existing patterns I'd rather not diverge from in this PR, or scope creep. Fixed:
Skipped, with reasoning:
|
1 participant
What
Adds a "Leave Workspace" affordance for users on workspaces shared with them. Replaces the Danger Zone "Delete" button in the workspace settings modal with "Leave" when the workspace is shared (not owned). Owners see the existing Delete flow unchanged.
Why
Previously there was no way for a collaborator to remove themselves from a workspace shared with them — only the owner could kick someone. The settings modal also rendered a "Delete" button on shared workspaces that silently 403'd, which was a latent UX bug.
Approach
3 files, no schema changes, no Zero changes (workspace_collaborators is REST-served, not Zero-synced):
POST /api/workspaces/[id]/leave— auth check, rejects owner self-leave with 400, otherwise deletes the user'sworkspace_collaboratorsrow. Returns 404 if the user wasn't a collaborator.leaveWorkspace(workspaceId)mirroring the existingdeleteWorkspacemutation: optimistic cache update, redirect-if-current-workspace, success/error toasts.workspace.isShared. Shared → "Leave Workspace" withLogOuticon and a parallel AlertDialog. Owned → existing Delete flow untouched.Verification
Tested locally with two Google accounts:
/homeif user was inside the workspace when they leftPOST /api/workspaces/<owned-id>/leavereturns 400 with clear error (owner-leave guard)pnpm lintclean,pnpm tc:tsccleanOut of scope (known follow-ups)
WorkspaceGrid.tsxstill calls owner-onlyDELETE /api/workspaces/[id]for shared workspaces, silently 403'ing. Should either filter shared out of bulk delete or split into "Delete (owned) / Leave (shared)". Worth its own PR.permissionLevel.hasWorkspaceReadAccess+assertWorkspaceWriteAccessboth check the collaborator row, so post-leave requests 403). Locally cachedworkspace_itemsrows persist in IndexedDB until Zero's GC runs, but the UI never reads them since navigation paths are also gated. Not a leak, but worth flagging.Summary by CodeRabbit