Fix/skills search improvement

[cole-ramer]

Summary
Improves how users discover and inspect skills (search, viewer, and onboarding/harness flows), refreshes harness creation UI (logo and layout), and includes Biome lint / accessibility fixes for sandbox-related UI. Backend support extends Convex skills behavior where needed.

What changed
Skills search — Better search and browsing in the skills browser; Convex skills updates to support the improved flow.
Skill viewer — Dedicated skill viewer dialog and follow-up fixes for markdown / viewer behavior.
Chat & harness — Wiring and UX updates so skills fit cleanly in chat and on the harness edit screen ($harnessId).
Onboarding & recommended skills — Updates to onboarding and the recommended skills grid for consistency with the new skills UX.
Harness creation UI — Visual updates for the harness logo and the “creating harness” experience.
Lint / a11y — Biome-driven fixes in sandbox UI (command input, panel resize handle, results, file viewer, etc.) without changing chat/stream harness config behavior.

[cole-ramer]

@claude review this, focus only on real issues

[cole-ramer]

@claude review this, focus on only real issues

[claude]

Claude finished @cole-ramer's task in 2m 11s —— View job

---

PR Review: Fix/skills search improvement

• Gathering context
• Reading changed files
• Identifying real issues

---

Issues Found

1. Stray 313; at top of harness-types.ts (bug)

apps/web/src/lib/harness-types.ts:1 has a bare expression statement 313; before the JSDoc comment — almost certainly an accidental keypress. TypeScript will emit a warning, and it makes the file look broken.

313; /**
 * Sandbox-related fields on the harness document.

Fix this →

---

2. label → div breaks sandbox checkbox click target (UX regression)

apps/web/src/routes/onboarding.tsx changed the sandbox enable row from <label> to <div>. The <div> has cursor-pointer styling but no onClick, so clicking the row text or the box icon no longer toggles the checkbox — only clicking the actual Radix Checkbox button does. The original <label> wired the full row as a click target automatically.

// Before (line ~908 in original): clicking anywhere in the row toggled the checkbox
<label className="flex cursor-pointer items-center gap-3 ...">
  <Checkbox ... />
  ...
</label>

// After: only the Checkbox itself responds to clicks
<div className="flex cursor-pointer items-center gap-3 ...">
  <Checkbox ... />
  ...
</div>

If this was changed to satisfy a Biome lint rule about nested interactive elements, the fix should be adding an onClick={() => setEnabled(e => !e)} on the <div>, not relying on the <label> association.

Fix this →

---

3. SkillViewerDialog and inline dialog in skills-browser.tsx are duplicated

apps/web/src/components/skill-viewer-dialog.tsx was added as a reusable component, but apps/web/src/components/skills-browser.tsx (lines ~400–448) contains its own nearly-identical inline dialog implementation instead of using SkillViewerDialog. Any future fix to the viewer (error states, frontmatter stripping, etc.) must be applied in two places.

Fix this →

---

4. SkillViewerDialog has no error/empty state — spins forever on missing detail

apps/web/src/components/skill-viewer-dialog.tsx:82:

{detailQuery.isLoading || !detailQuery.data?.detail ? (
  <Loader2 ... />  // shown forever if data loaded but detail is null/undefined
) : (
  <MarkdownMessage ... />
)}

If the query succeeds but the skill has no cached detail yet (or the ensureSkillDetails action fails), the spinner never goes away. There's also no check for detailQuery.isError. A simple fallback message should be shown when !detailQuery.isLoading && !detailQuery.data?.detail.

Fix this →

---

5. toSandboxApiConfig helper is exported but never imported

apps/web/src/lib/harness-types.ts exports toSandboxApiConfig and HarnessSandboxFields, but nothing imports them — the sandbox field mappings in chat/index.tsx are still inlined manually (four separate times). Either use the helper or remove it.

---

Minor notes (not blocking)

• Side effect during render (skill-viewer-dialog.tsx:43–47): calling ensureSkillDetailsFn directly in the render body is technically a React anti-pattern and can double-fire in StrictMode. Moving it into a useEffect with fullId as a dependency would be cleaner, though the ensuredRef guard makes it benign in practice.
• status: "started" as const in onboarding.tsx — the as const casts are correct fixes but suggest the Convex mutation arg types might benefit from tighter inference upstream.

[cole-ramer]

@claude, review this new push, focus on only real issues

[claude]

Claude finished @cole-ramer's task in 3m 33s —— View job

---

PR Review: Fix/skills search improvement

• Gathering context
• Reading changed files
• Identifying real issues

---

Previous review issues are resolved — harness-types.ts is gone (no more stray 313;), SkillViewerDialog now has proper error/empty states, dialog duplication is fixed, and the sandbox <label> is back with correct handlers.

One real issue: nested <button> inside <button>

The Eye/View icon button is rendered as a <button> inside an outer toggle <button> in three places. This is invalid HTML — the spec forbids nesting interactive elements. Browsers typically pull the inner button out of the outer one during parsing, which means e.stopPropagation() won't prevent the outer toggle from firing.

apps/web/src/components/skills-browser.tsx:290-337

<button onClick={() => onToggle(...)}>  {/* outer: toggles skill */}
  ...
  <button onClick={(e) => { e.stopPropagation(); setViewingSkill(skill); }}>
    <Eye />
  </button>
</button>

apps/web/src/components/recommended-skills-grid.tsx:37-84 — same pattern.

apps/web/src/routes/harnesses/$harnessId.tsx:743-769 — same pattern.

The fix is to change the outer <button> to a <div> with role="button" + tabIndex={0} + keyboard handler, or restructure so the toggle onClick is on the wrapper div and the Eye button sits beside it (not inside it). The simplest approach:

// Outer: div acting as the card container, inner buttons are peers not children
<div className="..." role="button" tabIndex={0}
  onClick={() => onToggle(...)}
  onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') onToggle(...); }}
>
  <Checkbox ... />
  <div ...>...</div>
  <button type="button" onClick={(e) => { e.stopPropagation(); ... }}>
    <Eye />
  </button>
</div>

Fix skills-browser.tsx → | Fix recommended-skills-grid.tsx → | Fix $harnessId.tsx →

[cole-ramer]

@claude review this, only focus on real issues

[claude]

Claude finished @cole-ramer's task in 3m 40s —— View job

---

PR Review: Fix/skills search improvement

• Gathering context
• Reading changed files
• Identifying real issues

---

All issues from prior reviews are resolved — nested <button> nesting is fixed with div[role="button"] + keyboard handlers in skills-browser.tsx, recommended-skills-grid.tsx, and $harnessId.tsx. SkillViewerDialog error/empty states are correct. Dialog duplication is gone.

One real issue found

discoverSkillsFromSearch accepts unauthenticated writes

packages/convex-backend/convex/skills.ts:487–527 — the discoverSkillsFromSearch action has no auth check. Any caller (including unauthenticated browser sessions) can insert arbitrary rows into the global skillsIndex table.

Compare with ensureSkillDetails at line 395–397, which correctly gates:

const identity = await ctx.auth.getUserIdentity();
if (!identity) throw new Error("Unauthenticated");

discoverSkillsFromSearch has no equivalent. The fix is to add the same two lines at the top of its handler:

handler: async (ctx, args): Promise<number> => {
+   const identity = await ctx.auth.getUserIdentity();
+   if (!identity) throw new Error("Unauthenticated");
    const fullIds = args.skills.map((s) => s.fullId);
    ...

Fix this →

---

Minor notes (not blocking)

• command-input.tsx:51–55: useEffect with no dependency array runs on every render and refocuses the input after 50ms. This prevents users from selecting/copying text from terminal output since focus is stolen back almost immediately. Intentional for terminal UX but worth noting.
• command-input.tsx:58–60: useEffect(..., []) comment says "scroll to bottom when entries change" but the empty dep array means it only runs on mount — auto-scroll never fires for new command output. Pre-existing but worth fixing.