Feat/workspaces

[jon3350]

Added workspaces feature.
Harnesses have been decoupled from sandboxes.
Added manage sandboxes setting.
In settings, you can select whether or not you want to use workspaces or basic layout.
Harnesses still have default sandbox, but may want to remove this in the future.

[DIodide]

@claude review this

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[DIodide]

@claude review this

[claude]

Claude finished @DIodide's task in 2m 22s —— View job

---

PR Review: Feat/workspaces

• Gather context and read CLAUDE.md
• Review changed files
• Analyze backend changes (Convex + FastAPI)
• Analyze frontend changes (React components + routes)
• Post comprehensive review feedback

---

Overall this is a solid feature addition — the harness/sandbox decoupling is a good architectural direction and the backend (Convex schema, auth patterns, FastAPI simplification) is well done. A few issues worth addressing before merge, with one significant structural concern.

---

Critical: Massive Code Duplication

apps/web/src/routes/workspaces/index.tsx is 4,275 lines and is essentially a copy of chat/index.tsx with workspace-specific state bolted on. Every future change to the chat UI (streaming, MCP errors, message queue, attachments, etc.) will now need to be made in two places. This is the highest-risk part of the PR.

Options:

• Extract shared logic into a shared hook (e.g., useChatSession) and a shared layout component that both routes consume.
• Or keep the duplication short-term with a clear TODO comment, but this will hurt.

Fix this →

---

Backend Issues

1. Inefficient conversations.list in basic mode (packages/convex-backend/convex/conversations.ts:29-31):

.take(100)
.then((conversations) =>
  conversations.filter((conversation) => !conversation.workspaceId).slice(0, 50),
);

This fetches 100 conversations to in-memory filter down to 50 non-workspace ones. A user with many workspace conversations could end up with fewer than 50 non-workspace conversations shown, even when they have more. Consider a dedicated by_user_no_workspace index or accept this limitation is acceptable for now.

2. No remove mutation for workspaces (packages/convex-backend/convex/workspaces.ts):

There's create, update, and touch but no remove. Users can create workspaces but can't delete them from the backend. The UI may not expose deletion yet, but the backend should support it — and deletion should cascade or at minimum detach workspaceId from related conversations.

Fix this →

3. sandbox.py creates its own HTTP client instead of using the injected one (packages/fastapi/app/routes/sandbox.py:70-83):

async with httpx.AsyncClient() as http_client:
    await create_sandbox_record(http_client, ...)

The route handler already receives the shared http_client via Depends(get_http_client) in other routes. This bypasses connection pooling and is inconsistent with the rest of the codebase — though looking at the function signature create_sandbox doesn't use Depends(get_http_client). Consider adding the dependency to stay consistent.

4. searchContentCount fetches up to 1000 docs just to count (packages/convex-backend/convex/conversations.ts:418):

.take(1000)
return results.length;

This is expensive. For a count you only need pagination info (isDone). Consider capping at a lower number and returning "1000+" if it hits the limit, or just remove the count query since the paginated version already exists.

---

Schema / Data Model

5. workspaces.sandboxId is required, not optional (packages/convex-backend/convex/schema.ts:83):

sandboxId: v.id("sandboxes"),

The PR description says "harnesses still have default sandbox, but may want to remove this in the future." The workspace model has the same tight coupling — a workspace without a sandbox can't be created. If sandbox-less workspaces are a future direction, this should be v.optional(v.id("sandboxes")) now.

6. conversations.create validates workspace.harnessId === harnessId (packages/convex-backend/convex/conversations.ts:65-68):

if (workspace.harnessId !== args.harnessId) {
    throw new Error("Workspace harness mismatch");
}

But workspaces.update can change harnessId on an existing workspace. Existing conversations for that workspace will then have a stale lastHarnessId. This is likely fine for now (conversations are historical), but worth documenting as a known edge case.

---

Frontend Issues

7. Possible redirect loop (apps/web/src/routes/workspaces/index.tsx:152-156):

if (settings.workspacesMode !== "workspaces") {
    throw redirect({ to: "/chat" });
}

If /chat does the reverse redirect (→ /workspaces when workspacesMode === "workspaces"), a user mid-navigation who has their settings loaded inconsistently could get stuck in a loop. Verify the /chat route handles this gracefully.

8. navigate in useEffect with missing dep (apps/web/src/routes/workspaces/index.tsx:574-578):

navigate({
    to: "/workspaces",
    search: { workspaceId: activeWorkspace._id },
    replace: true,
});

navigate is called inside a useEffect that lists [activeWorkspace, harnesses, initialHarnessId, navigate]. TanStack Router's navigate is stable, so this is fine in practice — but it's calling navigate on every workspace change even if the URL already matches. Consider checking the current URL first.

---

Minor Nits

• create_sandbox.tsx doesn't use harness_id in the API call body (line 56-62). The backend SandboxCreateRequest likely has harness_id as optional, but it means sandboxes created this way won't be linked to any harness automatically, which is correct for the decoupled model — just make sure it's intentional.
• assertOwnedWorkspace error message (workspaces.ts:14) says "Workspace not found" even when the workspace exists but belongs to another user. This is good for security (avoids leaking existence), but worth noting intentionally.
• The workspaces.list query uses withIndex("by_user_last_used", ...) correctly — good index design.

---

What's Done Well

• Auth checks are consistent and correct across all new Convex functions.
• assertOwnedWorkspace is a clean helper pattern.
• The FastAPI chat.py simplification (removing auto-sandbox creation) is a significant improvement — ~65 lines removed.
• Ownership verification in sandbox routes via _assert_sandbox_owner is properly placed before any destructive operations.
• The SandboxPanelContext handling of sandbox ID changes (incrementing reloadKey) is clean.

[DIodide]

Pushed 5ee3554 with two changes:

Command palette (Cmd+K / Cmd+Shift+P)

• cmdk-powered palette mounted once at the root
• Global commands: nav to chat/workspaces/harnesses/sandboxes, sign out, dynamic Open harness: / Open sandbox: entries
• Chat-scoped: new conversation, cancel stream, toggle sidebar
• Workspace-scoped: create, rename active, quick-switch
• Route-scoped commands register via useRegisterCommands; recent commands persisted in localStorage

Onboarding redirect race fix
New sessions could land on /onboarding despite having harnesses. Root cause: api.harnesses.list returns [] when ctx.auth.getUserIdentity() is null, which the frontend redirect effect couldn't distinguish from "user has zero harnesses." Under any Clerk/Convex handshake latency, the effect fired before the authenticated refetch landed.

Fix gates the redirect in both /chat and /workspaces on useConvexAuth().isAuthenticated, so it only evaluates once Convex has the JWT and the query result reflects the real user.

[jon3350]

@claude review this

[claude]

Claude finished @jon3350's task in 5m 17s —— View job

---

PR Review: Feat/workspaces (updated — latest 4 commits)

• Read CLAUDE.md for repo guidelines
• Review new commits since last review (command palette, skills menu, delete workspace, None harness/sandbox)
• Analyze key changed files in depth
• Post comprehensive review feedback

---

Good progress since the last review. Three of the previous concerns are now addressed. The command palette implementation is well-architected. A few issues worth looking at below.

---

Previously Raised — Now Resolved ✅

• workspaces.remove added (workspaces.ts:149) — cascades to conversations + messages. Good.
• sandboxId/harnessId both optional in schema (schema.ts:91-92) — decoupling complete.
• Onboarding race fixed — both /chat and /workspaces now gate the redirect on convexAuthReady && harnesses, preventing the early-empty-array false trigger.

---

Issues Worth Addressing

1. workspace.remove mutation may hit Convex limits for large workspaces (workspaces.ts:157-178):

The cascade runs one ctx.db.delete per message sequentially, inside a per-conversation loop:

for (const conversation of conversations) {
  const messages = await ctx.db.query("messages")...collect();
  for (const message of messages) {
    await ctx.db.delete(message._id);   // sequential awaits
  }
  await ctx.db.delete(conversation._id);
}

Convex mutations have complexity limits. A workspace with 50 conversations × 100 messages each = 5,050 awaited ops in one mutation — this can timeout. The inner message loop should use Promise.all at minimum:

await Promise.all(messages.map((m) => ctx.db.delete(m._id)));

Fix this →

2. Duplicate Escape handler in command palette (use-command-palette-hotkey.ts:45-52):

useEffect(() => {
  if (!open) return;
  const onEsc = (e: KeyboardEvent) => {
    if (e.key === "Escape") setOpen(false);
  };
  document.addEventListener("keydown", onEsc);
  ...
}, [open, setOpen]);

Cmdk.Dialog already calls onOpenChange(false) natively on Escape. This listener means setOpen(false) fires twice. Harmless (idempotent), but the second listener can also intercept Escape before cmdk's own handler in some browser event orderings. You can remove this entire second useEffect — cmdk handles Escape correctly.

3. "None" harness quietly reassigns a harness at runtime (workspaces/index.tsx:582-593):

When a workspace has no harnessId, the effect falls through to pick "the first started harness":

if (activeWorkspace.harnessId) {
  setActiveHarnessId(activeWorkspace.harnessId);
} else if (...harnesses?.some(...)) {
  setActiveHarnessId(activeHarnessId);  // keep existing
} else if (harnesses?.length) {
  const started = harnesses.find((h) => h.status === "started");
  setActiveHarnessId(started?._id ?? harnesses[0]._id);   // silent fallback
} else {
  setActiveHarnessId(null);
}

So selecting "None" in the workspace editor doesn't mean "no harness" — it means "pick whatever." The "None" label in the sidebar (harness?.name ?? "None") will then show the real harness name, not "None", as soon as the workspace is opened. This is confusing — a workspace showing "None / Sandbox A" in the list will immediately flip to "My Harness / Sandbox A" on open.

If the intent is that harness-less workspaces still use whatever harness is selected, the sidebar label should reflect the runtime harness instead of "None." If the intent is truly "no harness," the fallback chain needs to be gated on activeWorkspace.harnessId !== undefined.

4. deleteWorkspace confirmation state not reset on error (workspaces/index.tsx:1478-1487):

const deleteWorkspace = useMutation({
  onSuccess: () => { resetRenameWorkspaceDialog(); ... },
  onError: () => { toast.error("Failed to delete workspace"); },
});

If deletion fails, the dialog stays open with confirmDeleteWorkspace: true. Clicking "Delete Workspace" again immediately fires the mutation again (no re-confirmation prompt). Add setConfirmDeleteWorkspace(false) in onError.

Fix this →

---

Minor Nits

5. React.ComponentType used without React import (lib/command-palette/types.ts:33):

export type CommandIcon =
  | LucideIcon
  | React.ComponentType<{ className?: string }>;

React is not imported in this file. It works today if global React types are injected elsewhere, but prefer import type { ComponentType } from "react" and use ComponentType<...> directly — more explicit and won't silently break if the global setup changes.

6. PackagePlus icon for "Open sandbox" commands (global-commands.tsx:107):

PackagePlus is the "add/create" icon — the "Open sandbox: X" entries should use Package or Box for semantic consistency with how harness "open" commands use Package.

7. HeaderSkillsMenu stays open behind the browse dialog (header-skills-menu.tsx:111-113):

When "Browse skills…" is clicked, setBrowseOpen(true) fires but setOpen(false) doesn't. The dropdown is still rendered (but hidden behind the portal dialog). On dialog close, the dropdown is still visible. Adding setOpen(false) when opening the browse dialog would clean this up.

---

What's Well Done in These Commits

• Command palette architecture — commandsRef (a Map) + snapshot() on open is the right pattern: avoids stale command closures, stable callback identity, zero re-renders while the palette is closed.
• useRegisterCommands — clean composable hook with proper cleanup on unmount.
• useWorkspaceShortcuts — ref-based workspacesRef/onSelectRef avoids stale closures while keeping the keydown handler from rebinding on every render. The isModalOpen() guard correctly blocks workspace shortcuts while the command palette is open (cmdk dialog has aria-modal="true").
• pushRecentCommand — correctly deduplicates before prepending, with graceful localStorage fallback.
• getIsMac — SSR-safe with typeof navigator === "undefined" guard.
• canCreateWorkspace prop on useWorkspaceActionCommands is forward-looking — when a workspace cap is added, the hookup point already exists.
• Spinners — both use useEffect for canvas/animation setup, so SSR rendering is safe.

---

[jon3350]

@claude review this

[claude]

Claude finished @jon3350's task in 13m 20s —— View job

---

PR Review: Feat/workspaces (commit 470d63d)

• Read CLAUDE.md / gather context
• Check git log and diff since last review
• Review changed files in depth
• Post comprehensive review feedback

---

Good work — the latest commit cleanly addresses all 7 issues from the previous review. Here's the accounting:

Resolved from Last Review ✅

Issue	Fix
workspace.remove sequential message deletes could timeout	Promise.all(messages.map(m => ctx.db.delete(m._id)))
Duplicate Escape handler in command palette	Second useEffect with onEsc removed entirely
"None" harness silently picking a random harness at runtime	Fallback chain removed; now correctly sets null when no harnessId
Delete workspace confirm state not reset on error	setConfirmDeleteWorkspace(false) added to onError
React.ComponentType without import in types.ts	Replaced with import type { ComponentType } from "react"
PackagePlus icon on "Open sandbox" commands	Changed to Package
HeaderSkillsMenu browse dialog opened with dropdown still mounted	setOpen(false) added before setBrowseOpen(true)

---

Still Open from First Review

These were flagged in the initial review but haven't been addressed across any of the commits. Flagging them again for tracking:

1. Code duplication (apps/web/src/routes/workspaces/index.tsx at 4,487 lines vs chat/index.tsx at 4,068 lines):

The two files still share ~90% of logic. Any change to the chat flow (streaming, MCP errors, message queue, attachments, etc.) needs to be made in both files. This is the highest-maintenance debt in the PR.

Fix this →

2. conversations.list in-memory filter (conversations.ts:28-31):

.take(100)
.then((conversations) =>
  conversations.filter((conversation) => !conversation.workspaceId).slice(0, 50),
);

Still fetches up to 100 to filter down to 50 non-workspace conversations. A user with lots of workspace conversations may see fewer than 50 basic-mode conversations even when more exist.

3. searchContentCount fetches 1,000 docs to count (conversations.ts:418):

.take(1000);
return results.length;

This is still an expensive count-by-read. A cheaper approach would be to cap at a reasonable limit and return "1000+" if it hits the ceiling, or drop the count query in favor of the paginated one that already exists.

4. sandbox.py creates its own HTTP client (sandbox.py:70-83):

async with httpx.AsyncClient() as http_client:
    await create_sandbox_record(http_client, ...)

The create_sandbox route doesn't declare http_client: httpx.AsyncClient = Depends(get_http_client) unlike the rest of the codebase. Minor inconsistency that bypasses connection pooling.

---

New Observation

The handleSend guard (if (!content || !activeHarness || budgetExceeded) return; at line 3979) correctly blocks sending when a workspace has no harness. But there's no visible UI indicator to tell the user why the send button is disabled when activeHarness is null. A workspace with "None" harness will just silently not send. Worth adding a tooltip or disabled-state label ("Select a harness to chat") to make this actionable for users.

Fix this →

---

The critical Promise.all fix and the harness-null fix are the most important changes — both were correctness/reliability issues. Everything else in this commit is clean-up. The PR is in good shape; the main outstanding risk before merge is the duplication concern.