Feat/final polish

[jon3350]

No description provided.

[jon3350]

@claude review this

[claude]

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

---

PR Review: Feat/final polish

• Gather context and read changed files
• Review changed files
• Post review feedback

---

Summary

This PR adds several "polish" features across the stack: per-user sandbox cap enforcement (frontend + Convex + FastAPI rollback), chat input character counter, harness status editing, duplicate name warnings, and improved empty states. The backend architecture is solid. A few issues worth addressing before merging.

---

Issues

Medium — Dead variable in sandboxes/index.tsx

SandboxCard at line 330 has const isDraft = false; hardcoded and never changes. It controls:

• Card border styling (line 342) — the dashed draft border never applies
• The Start/Stop dropdown item (line 379) — always shows, even for non-applicable states

This is a copy-paste artifact from HarnessCard. Either remove the variable and inline false, or implement actual draft logic. Fix this →

Medium — FastAPI HTTPException detail serialized as raw JSON in toast

In create_sandbox.tsx:66 and onboarding.tsx, errors are read with res.text(), but FastAPI serializes HTTPException as {"detail": "message"}. When the sandbox cap is hit, users see a toast like:

{"detail": "You've reached the limit of 5 sandboxes..."}

instead of the plain message. Parsing the JSON would surface a clean error:

const body = await res.json().catch(() => ({ detail: "" }));
throw new Error(body.detail || `Sandbox API error ${res.status}`);

Fix this →

---

Minor Nits

Commented-out dead code in sandboxes/index.tsx
There are ~10 commented-out lines for duplicateHarness / onDuplicate. If duplication isn't planned soon, remove them to keep the diff clean.

Extra blank line in sandboxes.ts:252
An extra blank line was left before export const updateStatus.

collect() without limit for cap check (sandboxes.ts:86, sandboxes.ts:207)
Both create and createInternal call .collect() on the user's sandboxes just to count them. .take(MAX_SANDBOXES_PER_USER + 1) would be slightly more efficient, though with a cap of 5 it's negligible.

---

Potential Race Condition (low risk, known pattern)

The sandbox cap check in Convex (create and createInternal) reads the count then inserts — not atomic. Two simultaneous requests could both see count = 4 and both succeed, resulting in 6 sandboxes. Given this is a personal sandbox with a low cap, the practical impact is low, but worth noting.

---

Positive Highlights

• Excellent rollback pattern (sandbox.py): If Convex rejects after Daytona creates the sandbox, the orphaned Daytona resource is cleaned up. The two-phase error handling (SandboxRecordError with code) cleanly separates Daytona failures from Convex rejections.
• Defense in depth: Limit enforced at UI (disabled button + banner), Convex mutation, and FastAPI with rollback.
• Mirrored constants with cross-file comments (MAX_SANDBOXES_PER_USER, CHAT_INPUT_MAX_LENGTH) are well-organized and help avoid drift.
• Character counter UX: amber at 80%, red at 100%, replaces the disclaimer rather than adding a second line — clean and unobtrusive.
• Duplicate name warnings: Nice soft guardrail that doesn't block users but surfaces the issue clearly.
• SandboxRecordError.code: Typed error codes enable the status-code mapping in sandbox.py (400 for limit, 502 for other Convex errors) without stringly-typed comparisons.

---

[jon3350]

@claude review this

[claude]

Claude finished @jon3350's task in 2m 29s —— View job

---

PR Review: Feat/final polish

• Gather context
• Read changed files
• Post review feedback

---

Summary

This is a re-review of the same PR. The two medium issues from the previous review are now fixed. The remaining observations are minor. This is clean, shippable code.

---

Previous Issues — Now Fixed ✅

✅ Dead isDraft variable removed (sandboxes/index.tsx)
const isDraft = false and all its guarded branches are gone. The card border is inlined and the Start/Stop item is always rendered.

✅ FastAPI error detail parsed correctly (create_sandbox.tsx:63–66, onboarding.tsx:363–366)
Both callers now do res.json().catch(() => null) and extract body.detail, so users see a clean message instead of raw JSON when the sandbox cap is hit.

✅ All commented-out duplicate/draft dead code removed
duplicateHarness, onDuplicate, and related commented props are gone from SandboxGroup and SandboxCard.

---

New Observations

Minor — atSandboxLimit defaults to false while Convex query loads
In create_sandbox.tsx:42–43:

const { data: sandboxes } = useQuery(convexQuery(api.sandboxes.list, {}));
const sandboxCount = sandboxes?.length ?? 0;
const atSandboxLimit = sandboxCount >= MAX_SANDBOXES_PER_USER;

During the brief window when sandboxes is undefined (still fetching), the Create button is enabled and the limit banner is hidden. If a user clicks fast enough while at the limit, they'll get a server error instead of the in-page banner. Convex and FastAPI will still reject it correctly, so this is never silently wrong — just a UX blip. Adding isLoading to the disabled check would close the gap: disabled={isCreating || atSandboxLimit || isLoading}. Fix this →

Minor — duplicateHarnessNames / duplicateSandboxNames computed inline every render
Both pages compute the duplicate-name sets via an IIFE on every render:

const duplicateSandboxNames = (() => {
  const counts = new Map<string, number>();
  ...
})();

This is fine at today's cap of 5, but it could be wrapped in useMemo for consistency with the rest of the component's derived state and to avoid unnecessary re-computation on unrelated renders. Not worth a blocking comment given the data size.

Minor — status warning banners read persisted state, not local edits
In $harnessId.tsx:460–486, the draft/stopped banners check harness.status (server value), not currentStatus (local draft). So if a user changes the select from "draft" to "started", the amber draft warning keeps showing until they save. Line 515 adds a small note ("Saving will promote this harness out of draft") to compensate, which is a reasonable UX choice. Worth considering changing the outer guard to currentStatus === "draft" to make the banner react immediately to the select change. Fix this →

Nit — no rounded-* class on warning banners
All the new border border-amber-400/40 bg-amber-400/10 px-4 py-3 banners use sharp corners. This appears intentional and consistent across the PR — just confirming it's deliberate rather than overlooked.

---

Positive Highlights

• pendingCreateWorkspace counter pattern (workspaces/index.tsx:226, 1497–1499): Incrementing a counter to force a useEffect re-fire is a clean, idiomatic React trick. The sidebar opens and the create modal triggers correctly without any state resetting needed.
• Onboarding useEffect mode guard (onboarding.tsx:1209–1214): Clamping mode to "existing" when at the limit runs exactly once (no loop risk) and is guarded correctly by enabled.
• FastAPI message length validation (chat.py:355–383): Rejecting before the budget check and before any Anthropic calls is the right fail-fast order. The SSE error event shape is consistent with other errors in the stream.
• Empty workspace state CTA (workspaces/index.tsx:3727–3775): Differentiated copy for zero-vs-existing workspaces plus a direct action button is a big UX improvement over the old static paragraph.
• hasExistingHarness in onboarding title (onboarding.tsx:506–510): Small but high-signal touch — removes the false assumption that every onboarding visit is someone's first harness.

---