refactor(workspace): rearchitect shell, fix Zero lifecycle, etc#487
Merged
Conversation
… drilling
Workspace shell was getting stuck on the skeleton with intermittent 403s and
"Zero was explicitly closed" errors. Root causes were broader than the symptom:
batched query authorization failed for the whole batch, the Zero client's
lifecycle wasn't tied to userId properly, the StrictMode double-effect was
closing the live client, and three overlapping loading flags produced
contradictory branches in the render path.
Zero correctness
- /api/zero/query: pre-check workspace access per requested workspace and
isolate denied IDs so other queries in the same batch still resolve.
- client: getZero swaps the singleton when userId changes; destroyZero is a
pure teardown.
- provider: tear down only on logout transitions (non-null -> null) via a ref.
StrictMode no longer closes the live client mid-render. reset() bumps a
version so useMemo rebuilds a fresh client.
State machine
- New useWorkspaceView returns a tagged union (loading | unauthenticated |
denied | error | ready). Replaces nested loadingWorkspaces /
loadingCurrentWorkspace / isLoadingWorkspace branches.
- AccessDenied gains optional title/description/onRetry to surface Zero
errors with a "Try again" path that reset()s the client.
Single workspace items subscription
- New WorkspaceItemsProvider centralizes the one Zero subscription for
workspace.items. ChatPanel, MarkdownText, composer-context, ChatProvider,
navigateToItem, and the five chat tool UIs now read items from this
context instead of calling useWorkspaceState themselves. Cuts ~12 leaf
Zero subscriptions to one and decouples those leaves from ZeroProvider.
WorkspaceContext as single source of truth for the active workspace
- Removed the currentWorkspaceId mirror from useWorkspaceStore; it was a
one-render-late copy of context state, kept in sync by an effect that
the React Compiler can't optimize and that introduced cascade renders.
- Context now derives currentWorkspaceId from currentWorkspace?.id locally
and exposes a useCurrentWorkspaceId() shorthand. 14 consumers migrated.
- Persisted UI state (currentThreadIdByWorkspace) stays in Zustand.
Server-side session gating
- /workspace/[slug]/page.tsx is a server component that redirects to
sign-in when no session exists, and to /invite/claim/[token] when an
invite query param is present. Anonymous sessions provisioned upstream
(/home, /share-copy) still pass through.
- AnonymousSessionHandler removed from WorkspaceShell. One fewer client
gate, no more "session loading" lottie phase on /workspace.
Loading affordances
- WorkspaceLoader (full-screen lottie) only for pre-shell phases.
- WorkspaceCardsLoader: small centered spinner inside the workspace
canvas while items load.
- WorkspaceHeaderSkeleton: placeholder for the real header during the
slug->id roundtrip; layout no longer hides the header.
- ChatPanelSkeleton: reserved chat slot during boot; reuses
ThreadLoadingSkeleton so the boot loading state matches the
mid-thread-switch state. WorkspaceLayout no longer gates the chat slot
on currentWorkspaceId, removing the ~25% width snap on first paint.
Shell flattening / dead-code removal
- WorkspaceShell, WorkspaceLayout, WorkspaceSection no longer drill
store-derived UI state through props. Removed key={...} state-reset
hacks. Inlined InviteGuard into the page (now server-side).
- Deleted: WorkspaceSkeleton, InviteGuard, the metadata=true branch and
flag, the unused GET /api/workspaces/[id] handler, the loadingState
shim refetch/version on useWorkspaceState, commented-out Joyride
scaffolding, and one unused useEffect import.
- New useWorkspaceUpload hook replaces duplicated PDF upload logic in
WorkspaceShell and WorkspaceSection.
- Hoisted EMPTY_ITEMS to module scope in WorkspaceSearchDialog so memo
deps stop thrashing when items is null.
Tests / typecheck pass.
Made-with: Cursor
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR performs a major refactoring that replaces Zustand-based workspace state management with a context-driven system, removes the GET workspace metadata endpoint, converts the workspace page to an async server component with session gating, restructures authorization in the Zero query endpoint to use batched access checks, removes the InviteGuard component, and simplifies component props by deriving workspace state from context hooks instead of prop-drilling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant WorkspacePage as WorkspacePage<br/>(async)
participant SessionHandler as SessionHandler/<br/>ZeroProvider
participant WorkspaceContext
participant WorkspaceItemsProvider
participant API as API Routes
User->>Browser: Navigate to /workspace/[slug]
Browser->>WorkspacePage: Render page
Note over WorkspacePage: Awaits searchParams
alt Invite query param present
WorkspacePage->>Browser: Redirect to /invite/claim/[token]
else No session
WorkspacePage->>SessionHandler: Check session via headers
SessionHandler->>API: Fetch session
API-->>SessionHandler: Session response
alt Session not found
WorkspacePage->>Browser: Redirect to /sign-in
else Session found
Note over WorkspacePage: Proceed to render
end
end
WorkspacePage->>WorkspaceContext: Init WorkspaceContext
WorkspaceContext->>API: Fetch workspace by slug
API-->>WorkspaceContext: Workspace data (no metadata)
WorkspaceContext->>WorkspaceItemsProvider: Provide currentWorkspaceId
WorkspaceItemsProvider->>API: Fetch workspace items/state
API-->>WorkspaceItemsProvider: Items array
WorkspaceItemsProvider->>WorkspaceContext: Cache items in context
alt Items loading
WorkspaceContext->>Browser: Render WorkspaceCardsLoader
else Items error/denied
WorkspaceContext->>Browser: Render AccessDenied/error view
else Items ready
WorkspaceContext->>Browser: Render WorkspaceSection + shell
end
sequenceDiagram
participant Client as Client
participant ZeroQuery as /api/zero/query
participant Auth as getAllowedWorkspaceIds
participant DB as Database
participant Transform as handleQueryRequest
participant Response
Client->>ZeroQuery: POST batch of queries
Note over ZeroQuery: Parse request JSON
alt Parse fails
ZeroQuery->>Response: 400 Bad Request
else Parse succeeds
ZeroQuery->>ZeroQuery: Extract all workspace IDs<br/>from batch
ZeroQuery->>Auth: Check access for extracted IDs<br/>(owner + collaborator)
Auth->>DB: Query allowed workspace IDs
DB-->>Auth: Allowed IDs set
Auth-->>ZeroQuery: allowed workspace IDs
ZeroQuery->>Transform: Process each query with allowed IDs
loop Per-query in batch
Transform->>Transform: Verify query workspace<br/>in allowed set
alt Workspace not allowed
Transform->>Transform: Throw WORKSPACE_ACCESS_DENIED_ERROR
else Workspace allowed
Transform->>Transform: Execute query normally
end
end
Transform-->>ZeroQuery: Query results + errors
ZeroQuery->>Response: 200 with batch results<br/>(some may have access_denied)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 5 minutes and 29 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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/slug/[slug]/route.ts (1)
50-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake collaborator workspace resolution deterministic.
Lines 50–63 return an arbitrary collaborator row (
limit(1)without ordering). With duplicate legacy slugs, users can land in different workspaces across requests.Proposed fix
- const [validCollab] = await db + const validCollabs = await db .select({ permissionLevel: workspaceCollaborators.permissionLevel, workspaceId: workspaceCollaborators.workspaceId, }) .from(workspaceCollaborators) .where( and( inArray(workspaceCollaborators.workspaceId, candidateIds), eq(workspaceCollaborators.userId, userId), ), ) - .limit(1); + const collabByWorkspaceId = new Map( + validCollabs.map((c) => [c.workspaceId, c.permissionLevel]), + ); + const foundWorkspace = [...candidateWorkspaces] + .sort((a, b) => a.id.localeCompare(b.id)) + .find((w) => collabByWorkspaceId.has(w.id)); - if (validCollab) { - // Found the specific workspace instance this user has access to - const foundWorkspace = candidateWorkspaces.find( - (w) => w.id === validCollab.workspaceId, - ); - if (foundWorkspace) { - workspace = foundWorkspace; - isShared = true; - permissionLevel = validCollab.permissionLevel; - } + if (foundWorkspace) { + workspace = foundWorkspace; + isShared = true; + permissionLevel = collabByWorkspaceId.get(foundWorkspace.id) ?? null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/workspaces/slug/`[slug]/route.ts around lines 50 - 63, The current collaborator lookup uses .limit(1) without ordering so results are nondeterministic; update the query that selects from workspaceCollaborators (the db.select(...) call using candidateIds and userId) to add a deterministic .orderBy(...) clause so the same collaborator row is chosen consistently (for example order by workspaceCollaborators.workspaceId ascending and then workspaceCollaborators.permissionLevel descending as a tiebreaker) before calling .limit(1).src/hooks/ui/use-navigate-to-item.ts (1)
19-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLoading guard is unreachable after switching to
useWorkspaceItems().At Line 19,
workspaceStateis always truthy ([]fallback), so the "Workspace not loaded" path never runs; users can get a false "Item no longer exists" while data is still loading. Use workspace-items status/loading as the guard instead.Suggested fix
-import { useWorkspaceItems } from "@/hooks/workspace/use-workspace-items"; +import { + useWorkspaceItems, + useWorkspaceItemsStatus, +} from "@/hooks/workspace/use-workspace-items"; export function useNavigateToItem() { - const workspaceState = useWorkspaceItems(); + const workspaceState = useWorkspaceItems(); + const { status } = useWorkspaceItemsStatus(); const setActiveFolderId = useUIStore((state) => state.setActiveFolderId); const navigateToItem = useCallback( (itemId: string, options?: { silent?: boolean }): boolean => { - if (!workspaceState) { + if (status === "loading") { if (!options?.silent) toast.error("Workspace not loaded"); return false; } const item = workspaceState.find((i) => i.id === itemId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/ui/use-navigate-to-item.ts` around lines 19 - 26, The guard in use-navigate-to-item.ts currently checks workspaceState (which is always truthy) so the "Workspace not loaded" branch is unreachable; instead use the loading/status from useWorkspaceItems() to decide whether to show "Workspace not loaded" vs "Item no longer exists". Update the logic in the function that references workspaceState and itemId to check the workspace-items hook's loading or status flag (e.g., isLoading or status) before attempting workspaceState.find(...), return false and toast.error("Workspace not loaded") when loading, and only then perform the item lookup and toast.error("Item no longer exists") if the item is missing; keep respect for options?.silent when showing toasts.
🧹 Nitpick comments (2)
src/components/layout/WorkspaceLayout.tsx (1)
125-134: ⚡ Quick winConsider adding a
keyto forceChatProviderremount on workspace switch.When
currentWorkspaceIdchanges from one workspace to another, theChatProviderreceives the new ID as a prop but doesn't unmount/remount. IfChatProviderholds internal state tied to the workspace (e.g., chat history, pending messages), that state won't reset. Addingkey={currentWorkspaceId}would ensure a fresh provider per workspace.♻️ Proposed fix
if (currentWorkspaceId) { return ( - <ChatProvider workspaceId={currentWorkspaceId}> + <ChatProvider key={currentWorkspaceId} workspaceId={currentWorkspaceId}> <ComposerProvider>{content}</ComposerProvider> </ChatProvider> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/WorkspaceLayout.tsx` around lines 125 - 134, The ChatProvider in WorkspaceLayout doesn't remount when switching workspaces, so add a key prop tied to the workspace id to force remount: update the JSX that renders <ChatProvider workspaceId={currentWorkspaceId}> (within WorkspaceLayout) to include key={currentWorkspaceId} so the ChatProvider (and its children like ComposerProvider) are recreated whenever currentWorkspaceId changes, resetting any workspace-specific internal state.src/components/workspace-canvas/WorkspaceSection.tsx (1)
383-413: ⚡ Quick winUse
view.itemsas the source of truth in thereadybranch.In the
readypath, usingstateinstead ofview.itemsreintroduces dual data sources and risks subtle UI drift during transitions.Suggested patch
<WorkspaceContent - viewState={state} + viewState={view.items} addItem={addItem} updateItem={updateItem} deleteItem={deleteItem} updateAllItems={updateAllItems} openWorkspaceItem={openWorkspaceItem} @@ {!isChatMaximized && view.kind === "ready" && ( <MarqueeSelector scrollContainerRef={scrollAreaRef} - cardIds={state.map((item) => item.id)} + cardIds={view.items.map((item) => item.id)} isGridDragging={isGridDragging} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/workspace-canvas/WorkspaceSection.tsx` around lines 383 - 413, In the ready branch (where view.kind === "ready") replace uses of the local derived state as the canonical item list with view.items: pass view.items instead of state into WorkspaceContent (e.g., the viewState/collection prop or wherever items are provided) and use view.items for MarqueeSelector.cardIds (map item.id from view.items) so the UI uses view.items as the single source of truth and avoids dual data sources causing drift.
🤖 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/zero/query/route.ts`:
- Around line 127-132: Replace the raw-ID console.warn in the denied-workspace
branch (the if (deniedWorkspaceIds.length > 0) block referencing userId,
requestedWorkspaceIds, deniedWorkspaceIds) so it no longer logs identifiable
values; instead log non-identifiable metadata such as deniedWorkspaceIds.length
and requestedWorkspaceIds.length plus a request or correlation ID (or store and
log hashed/redacted versions of the IDs) and update the log call accordingly to
avoid persisting raw user/workspace identifiers.
- Around line 56-68: The current auth logic relies on heuristically inspecting
args via extractWorkspaceId(args) which lets any new workspace-scoped query with
a different arg shape bypass authorization; change to fail-closed by keying
workspace authorization off query identity/metadata instead: introduce a
canonical registry or switch (e.g., map of queryName => requiresWorkspace
boolean) used by the request handler to decide whether to require a workspace
id, update extractWorkspaceId to be a targeted helper (or add a new
getWorkspaceIdForQuery(queryName, args) that knows per-query shapes) and make
the handler reject requests for queries marked as workspace-scoped when no
workspace id can be extracted, and apply the same change where
extractWorkspaceId is used elsewhere (the other usage around lines 137-145) so
unknown queries default to denied.
In `@src/components/chat/ChatPanel.tsx`:
- Around line 39-46: The useEffect currently calls onReady when isLoading is
false and state is truthy, which can run before a workspaceId exists; update the
effect (the useEffect that references useWorkspaceItems,
useWorkspaceItemsLoading and onReady) to also require workspaceId be truthy
before invoking onReady so it won't fire while ChatPanelSkeleton is rendered;
ensure the dependency array includes workspaceId.
In `@src/components/workspace-canvas/WorkspaceSearchDialog.tsx`:
- Around line 36-38: The current check sets isLoadingWorkspace only for
view.kind === "loading" but the empty-state UI also treats a missing
currentWorkspaceId as "Loading...", which can mislabel
denied/error/unauthenticated cases; update WorkspaceSearchDialog to (1) keep
isLoadingWorkspace = view.kind === "loading", (2) change the
empty-state/placeholder logic to inspect view.kind and view.currentWorkspaceId
explicitly and render different messages for "denied", "unauthenticated",
"error" vs true loading or missing workspace id, and (3) reference
useWorkspaceView, view.kind, view.currentWorkspaceId and isLoadingWorkspace when
implementing these conditional branches so each state shows the correct message.
In `@src/components/workspace-canvas/WorkspaceSection.tsx`:
- Around line 419-443: Guard the context menu callbacks so they no-op unless the
workspace view is in a ready state: wrap each callback passed into
renderWorkspaceMenuItems (the functions referencing addItem, handleCreatedItems,
handleUploadMenuItemClick, openAudioDialog, setShowYouTubeDialog,
setShowWebsiteDialog, setShowFlashcardsDialog, setShowQuizDialog) with a check
against the workspace-ready flag (e.g., isWorkspaceReady or workspaceView ===
'ready') and return early if not ready; alternatively disable rendering of
ContextMenuContent/ContextMenuItem when the workspace is not ready so none of
those mutating callbacks can be invoked from
loading/unauthenticated/denied/error states. Ensure the guard lives next to the
ContextMenuContent render logic so the behavior is consistent across all menu
entries.
In `@src/contexts/WorkspaceContext.tsx`:
- Line 103: The fetch URL is built using the raw currentSlug which can contain
reserved URL characters; update the fetch call that uses currentSlug to encode
it (use encodeURIComponent on currentSlug) before interpolation so the request
targets the correct endpoint (replace `/api/workspaces/slug/${currentSlug}` with
a version that interpolates the encoded slug), ensuring any function or block
that calls this fetch (the code referencing currentSlug in WorkspaceContext.tsx)
uses the encoded value.
In `@src/hooks/workspace/use-workspace-upload.ts`:
- Around line 58-61: prepareWorkspaceUploadSelection is still applying the
default max-size filter so oversized files get rejected even when
enforceSizeLimit is false; update the call in useWorkspaceUpload (the block
around prepareWorkspaceUploadSelection(candidates)) to pass the enforceSizeLimit
flag (or extend prepareWorkspaceUploadSelection to accept an options object with
enforceSizeLimit) and update prepareWorkspaceUploadSelection implementation to
skip size filtering when that flag is false, ensuring acceptedFiles retains
oversized files only when enforceSizeLimit === true; reference
prepareWorkspaceUploadSelection and the enforceSizeLimit variable in
use-workspace-upload.ts to locate and change both the call site and the selector
function.
- Around line 42-44: In use-workspace-upload (the callback that checks
operations and currentWorkspaceId), replace the throw new Error("Workspace not
available") with a user-facing toast/error notification and an early return to
avoid an unhandled rejection; specifically, import/use your toast utility, call
something like toast.error("Workspace not available") (or show an equivalent UI
message) and then return (or return Promise.resolve/void) from the handler
instead of throwing, keeping the checks around operations and currentWorkspaceId
intact.
In `@src/lib/zero/client.ts`:
- Around line 42-43: The code calls destroyZero() without awaiting, causing a
race between Zero.close() and creating a new instance; update destroyZero() to
return the Promise from Zero.close() (ensure it propagates the async result) and
change the caller check (the block referencing zeroInstance, zeroUserId and
params.userId) to await destroyZero() when switching users so the previous Zero
session fully closes before creating a new instance.
---
Outside diff comments:
In `@src/app/api/workspaces/slug/`[slug]/route.ts:
- Around line 50-63: The current collaborator lookup uses .limit(1) without
ordering so results are nondeterministic; update the query that selects from
workspaceCollaborators (the db.select(...) call using candidateIds and userId)
to add a deterministic .orderBy(...) clause so the same collaborator row is
chosen consistently (for example order by workspaceCollaborators.workspaceId
ascending and then workspaceCollaborators.permissionLevel descending as a
tiebreaker) before calling .limit(1).
In `@src/hooks/ui/use-navigate-to-item.ts`:
- Around line 19-26: The guard in use-navigate-to-item.ts currently checks
workspaceState (which is always truthy) so the "Workspace not loaded" branch is
unreachable; instead use the loading/status from useWorkspaceItems() to decide
whether to show "Workspace not loaded" vs "Item no longer exists". Update the
logic in the function that references workspaceState and itemId to check the
workspace-items hook's loading or status flag (e.g., isLoading or status) before
attempting workspaceState.find(...), return false and toast.error("Workspace not
loaded") when loading, and only then perform the item lookup and
toast.error("Item no longer exists") if the item is missing; keep respect for
options?.silent when showing toasts.
---
Nitpick comments:
In `@src/components/layout/WorkspaceLayout.tsx`:
- Around line 125-134: The ChatProvider in WorkspaceLayout doesn't remount when
switching workspaces, so add a key prop tied to the workspace id to force
remount: update the JSX that renders <ChatProvider
workspaceId={currentWorkspaceId}> (within WorkspaceLayout) to include
key={currentWorkspaceId} so the ChatProvider (and its children like
ComposerProvider) are recreated whenever currentWorkspaceId changes, resetting
any workspace-specific internal state.
In `@src/components/workspace-canvas/WorkspaceSection.tsx`:
- Around line 383-413: In the ready branch (where view.kind === "ready") replace
uses of the local derived state as the canonical item list with view.items: pass
view.items instead of state into WorkspaceContent (e.g., the
viewState/collection prop or wherever items are provided) and use view.items for
MarqueeSelector.cardIds (map item.id from view.items) so the UI uses view.items
as the single source of truth and avoids dual data sources causing drift.
🪄 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: 4e2c4ed3-eb44-45ff-a2de-9f2147a7f924
📒 Files selected for processing (39)
src/app/api/workspaces/[id]/route.tssrc/app/api/workspaces/slug/[slug]/route.tssrc/app/api/zero/query/route.tssrc/app/workspace/[slug]/page.tsxsrc/components/chat/ChatDropzone.tsxsrc/components/chat/ChatPanel.tsxsrc/components/chat/ChatProvider.tsxsrc/components/chat/TextSelectionManager.tsxsrc/components/chat/composer-context.tsxsrc/components/chat/parts/MarkdownText.tsxsrc/components/chat/tools/CreateDocumentToolUI.tsxsrc/components/chat/tools/CreateFlashcardToolUI.tsxsrc/components/chat/tools/CreateQuizToolUI.tsxsrc/components/chat/tools/EditItemToolUI.tsxsrc/components/chat/tools/YouTubeSearchToolUI.tsxsrc/components/layout/SessionHandler.tsxsrc/components/layout/WorkspaceLayout.tsxsrc/components/workspace-canvas/AudioCardContent.tsxsrc/components/workspace-canvas/WorkspaceCanvasDropzone.tsxsrc/components/workspace-canvas/WorkspaceContent.tsxsrc/components/workspace-canvas/WorkspaceSearchDialog.tsxsrc/components/workspace-canvas/WorkspaceSection.tsxsrc/components/workspace-canvas/WorkspaceSidebar.tsxsrc/components/workspace/AccessDenied.tsxsrc/components/workspace/InviteGuard.tsxsrc/components/workspace/SidebarCardList.tsxsrc/components/workspace/WorkspaceLoader.tsxsrc/components/workspace/WorkspaceShell.tsxsrc/components/workspace/WorkspaceSkeleton.tsxsrc/contexts/WorkspaceContext.tsxsrc/hooks/ui/use-navigate-to-item.tssrc/hooks/workspace/use-workspace-items.tsxsrc/hooks/workspace/use-workspace-state.tssrc/hooks/workspace/use-workspace-upload.tssrc/hooks/workspace/use-workspace-view.tssrc/lib/stores/__tests__/workspace-store.test.tssrc/lib/stores/workspace-store.tssrc/lib/zero/client.tssrc/lib/zero/provider.tsx
💤 Files with no reviewable changes (4)
- src/components/workspace/WorkspaceSkeleton.tsx
- src/lib/stores/tests/workspace-store.test.ts
- src/components/workspace/InviteGuard.tsx
- src/app/api/workspaces/[id]/route.ts
Contributor
Greptile SummaryThis PR addresses several interlocking root causes behind the stuck-skeleton / intermittent-403 / "Zero was explicitly closed" symptoms: the Zero client's lifecycle is now tied to
Confidence Score: 4/5Safe to merge with P2 caveats; no P0/P1 issues found across the core lifecycle, auth, and state-machine changes. All findings are P2: the session-expiry UX edge case, the duplicate useReactiveNavigation instantiation, and the one-frame Zero reset window. The core fixes (batched auth, logout-only teardown, tagged-union view state, single Zero subscription) are well-reasoned and correctly implemented. src/hooks/workspace/use-workspace-view.ts (session-expiry branch), src/components/workspace/WorkspaceShell.tsx and src/components/workspace-canvas/WorkspaceSection.tsx (duplicate useReactiveNavigation) Important Files Changed
Reviews (1): Last reviewed commit: "refactor(workspace): rearchitect shell, ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+38
to
+41
| } | ||
| if (!currentWorkspace) return { kind: "loading" }; | ||
|
|
||
| if (status === "error" || status === "timeout") { |
Contributor
There was a problem hiding this comment.
When a user's session expires mid-session and they navigate to a new workspace slug, session becomes null while sessionPending is false. The guard at line 31 only skips when sessionPending is true, so the code reaches this branch with a null session. session?.user?.isAnonymous evaluates to undefined (falsy), so the hook returns { kind: "denied" } — the user sees "Access Denied" rather than being redirected to sign-in. A null session after load is distinct from an anonymous one and should be handled explicitly.
Suggested change
| } | |
| if (!currentWorkspace) return { kind: "loading" }; | |
| if (status === "error" || status === "timeout") { | |
| return !session | |
| ? { kind: "unauthenticated" } | |
| : session.user?.isAnonymous | |
| ? { kind: "unauthenticated" } | |
| : { kind: "denied" }; |
| const operations = useWorkspaceOperations(currentWorkspaceId, state); | ||
|
|
||
| // Reactive navigation (auto-scroll/select on new items) | ||
| const { handleCreatedItems } = useReactiveNavigation(state); |
Contributor
There was a problem hiding this comment.
useReactiveNavigation call
WorkspaceContent (here) and WorkspaceSection (a direct child) both call useReactiveNavigation(state). Since useReactiveNavigation maintains a module-level activeCleanup closure, two live instances share that mutable slot. Each call to handleCreatedItems from either instance overwrites the other's pending cleanup, so a rapid header-upload followed by a canvas operation (or vice versa) could silently skip the reactive scroll/select for the first batch.
Consider owning the handleCreatedItems callback at the WorkspaceSection level only, and threading the header-upload callback through useWorkspaceUpload's onItemsCreated directly to WorkspaceSection's own handleCreatedItems.
Comment on lines
+64
to
+69
| destroyZero(); | ||
| setResetVersion((v) => v + 1); | ||
| }, | ||
| }), | ||
| [zero], | ||
| ); |
Contributor
There was a problem hiding this comment.
reset() closes Zero synchronously before status context updates
destroyZero() runs synchronously — it calls zeroInstance.close() and nulls the module-level ref. However, zero from useMemo still holds the now-closed instance until the next render triggered by setResetVersion. During that window, status.isReady remains true and any component that calls a Zero hook (e.g. useConnectionState, useQuery) will operate against the closed client. In practice this is a single render frame and the retry path is intentional, but consumers may see transient errors before the loader appears.
Comment on lines
32
to
34
| onOpenChange, | ||
| items, | ||
| currentWorkspaceId, |
Contributor
There was a problem hiding this comment.
useWorkspaceView()
WorkspaceSearchDialog receives items from its parent but independently calls useWorkspaceView() for isLoadingWorkspace. Although both ultimately come from WorkspaceItemsContext and will be consistent within a single render, having two data entry points for logically related state makes the component harder to reason about. Consider passing isLoadingWorkspace as a prop so the dialog's inputs form a single coherent snapshot.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- zero/client: destroyZero returns the close() promise; reset() awaits it. Document user-swap as accepted brief overlap (different sessions, no leak). - zero/query route: fail-closed authorization keyed off query name (registry of workspace-scoped extractors), not best-effort arg shape. New workspace.* queries must be registered to be allowed at all. - zero/query route: redact identifiers in the denied-access log; keep counts only. - ChatPanel: guard onReady() on workspaceId so it can't fire while the panel is showing the skeleton. - WorkspaceContext: encodeURIComponent the slug in the metadata fetch URL. - use-workspace-upload: toast + return instead of throwing when workspace isn't ready; pass enforceSizeLimit through to prepareWorkspaceUploadSelection so disabling the limit actually works. - WorkspaceSection: only render context-menu mutation items when view.kind === "ready"; prevents create/upload actions from firing against a non-ready store. - WorkspaceSearchDialog: surface "Workspace unavailable." when the view is denied/error/unauthenticated instead of mislabelling them as loading. Made-with: Cursor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 39 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/components/layout/SessionHandler.tsx">
<violation number="1" location="src/components/layout/SessionHandler.tsx:22">
P2: `inFlight` is never cleared after a successful anonymous sign-in, so a later `session -> null` transition can skip re-auth and leave the loader stuck.</violation>
</file>
<file name="src/hooks/workspace/use-workspace-upload.ts">
<violation number="1" location="src/hooks/workspace/use-workspace-upload.ts:58">
P2: `enforceSizeLimit: false` is ineffective because `prepareWorkspaceUploadSelection` is still called with its default 50MB filter, so large files are silently dropped.</violation>
</file>
<file name="src/hooks/workspace/use-workspace-view.ts">
<violation number="1" location="src/hooks/workspace/use-workspace-view.ts:35">
P2: Transient/non-404 workspace fetch failures are incorrectly surfaced as `denied` instead of an error state with retry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+22
to
+27
| if (isPending || session || inFlight.current) return; | ||
| inFlight.current = true; | ||
| signIn.anonymous().catch((error) => { | ||
| console.error("Failed to create anonymous session:", error); | ||
| inFlight.current = false; | ||
| }); |
Contributor
There was a problem hiding this comment.
P2: inFlight is never cleared after a successful anonymous sign-in, so a later session -> null transition can skip re-auth and leave the loader stuck.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/layout/SessionHandler.tsx, line 22:
<comment>`inFlight` is never cleared after a successful anonymous sign-in, so a later `session -> null` transition can skip re-auth and leave the loader stuck.</comment>
<file context>
@@ -1,42 +1,32 @@
- console.error("Failed to create anonymous session:", error);
- });
- }
+ if (isPending || session || inFlight.current) return;
+ inFlight.current = true;
+ signIn.anonymous().catch((error) => {
</file context>
Suggested change
| if (isPending || session || inFlight.current) return; | |
| inFlight.current = true; | |
| signIn.anonymous().catch((error) => { | |
| console.error("Failed to create anonymous session:", error); | |
| inFlight.current = false; | |
| }); | |
| if (isPending) return; | |
| if (session) { | |
| inFlight.current = false; | |
| return; | |
| } | |
| if (inFlight.current) return; | |
| inFlight.current = true; | |
| signIn.anonymous().catch((error) => { | |
| console.error("Failed to create anonymous session:", error); | |
| inFlight.current = false; | |
| }); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged the lookup-table dispatch as 'unvalidated dynamic method call' even though the table is a closed object literal. Inline the dispatch as a switch so the analyzer can see it's bounded. Made-with: Cursor
Made-with: Cursor
handleQueryRequest invokes the callback with args=argsArray[0] (already unwrapped). The shared extractor was checking Array.isArray first, which meant the per-query authz check inside the callback always returned null and denied every workspace.items query — including ones the upfront batched access check had just approved. Split into readWorkspaceId (object shape) and unwrap manually upfront. Made-with: Cursor
…denied When a session expires mid-use, session is null but sessionPending is false, so the unresolved-workspace branch fell through to 'denied' instead of prompting sign-in. Treat null session the same as anonymous. Made-with: Cursor
The library's ZeroProvider already handles client lifecycle reactively when
you pass config props directly (userID, schema, mutators, cacheURL, etc.).
We were running in manual mode (`zero={instance}`) and reimplementing all
of that in client.ts — the singleton, user-swap teardown, logout effect,
async destroy, resetVersion bump. The 'Zero was explicitly closed' bug
earlier this session was a direct symptom of doing lifecycle by hand
under StrictMode.
Switching to managed mode:
- client.ts collapses to just the ZeroContext type
- destroyZero/getZero gone
- reset becomes a key bump on ZeroProvider
- StrictMode handling delegated to the library
Also: bento-grid skeleton for WorkspaceCardsLoader (replaces spinner).
Made-with: Cursor
…ing phases Replace the lottie WorkspaceLoader with the same header-bar + bento-grid skeleton used in-shell. Now the user sees the same chrome from the very first frame through every loading phase — no lottie→blank→pop transition between session-resolved and Zero-mounted. Made-with: Cursor
Made-with: Cursor
… dense pack) Made-with: Cursor
Made-with: Cursor
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
Comment on lines
165
to
167
| } catch (error) { | ||
| if ( | ||
| error instanceof Error && | ||
| error.message === WORKSPACE_ACCESS_DENIED_ERROR | ||
| ) { | ||
| return NextResponse.json({ error: "Forbidden" }, { status: 403 }); | ||
| } | ||
|
|
||
| console.error("[zero/query] Unhandled error:", error); | ||
| return NextResponse.json( |
Contributor
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The old code had a dedicated catch for WORKSPACE_ACCESS_DENIED_ERROR that returned a 403 response. The new code removed that branch — the outer catch now converts all errors (including authz denials that handleQueryRequest re-throws) into a generic 500. If handleQueryRequest propagates the per-query throw new Error(WORKSPACE_ACCESS_DENIED_ERROR) instead of swallowing it per-query, the client sees 500 instead of 403, which breaks retry/redirect logic on the client and masks a permissions problem as a server error.
// src/app/api/zero/query/route.ts
} catch (error) {
console.error("[zero/query] Unhandled error:", error);
return NextResponse.json(
{ error: "Internal server error" },
{ status: 500 },
);
}Re-add the WORKSPACE_ACCESS_DENIED_ERROR check before the fallback 500 to preserve the 403 semantics.
Suggested change
| } catch (error) { | |
| if ( | |
| error instanceof Error && | |
| error.message === WORKSPACE_ACCESS_DENIED_ERROR | |
| ) { | |
| return NextResponse.json({ error: "Forbidden" }, { status: 403 }); | |
| } | |
| console.error("[zero/query] Unhandled error:", error); | |
| return NextResponse.json( | |
| } catch (error) { | |
| if ( | |
| error instanceof Error && | |
| error.message === WORKSPACE_ACCESS_DENIED_ERROR | |
| ) { | |
| return NextResponse.json({ error: "Forbidden" }, { status: 403 }); | |
| } | |
| console.error("[zero/query] Unhandled error:", error); | |
| return NextResponse.json( | |
| { error: "Internal server error" }, | |
| { status: 500 }, | |
| ); |
…mless transition Made-with: Cursor
… not just metadata-loaded The header gate was `currentWorkspaceId ? real : skeleton` — but on workspace switch, react-query serves cached metadata instantly so currentWorkspaceId flips to the new workspace immediately while items are still loading. Result: header shows the new workspace's name while the cards are still bento skeletons, which feels disjointed. Tie the header to view.kind === 'ready' so header and cards skeleton together through the entire transition, then swap together when items resolve. Made-with: Cursor
…t on switch) Same edge case as the workspace header: workspaceId flips to the new id instantly on switch (cached metadata), so ChatPanel rendered the new workspace's chat chrome with the old workspace's threads/messages briefly before things caught up. Gate on view.kind === 'ready' so chat skeletons through the entire transition. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Workspace shell was getting stuck on the skeleton with intermittent 403s and "Zero was explicitly closed" errors. Root causes were broader than the symptom: batched query authorization failed for the whole batch, the Zero client's lifecycle wasn't tied to userId properly, the StrictMode double-effect was closing the live client, and three overlapping loading flags produced contradictory branches in the render path.
Zero correctness
State machine
Single workspace items subscription
WorkspaceContext as single source of truth for the active workspace
Server-side session gating
Loading affordances
Shell flattening / dead-code removal
Tests / typecheck pass.
Summary by CodeRabbit
Release Notes
New Features
Improvements