feat: web ui polish - #125

internal/api/httpapi/static.go (1)

122-136: Optional optimization: prevent duplicate hashing on concurrent cache misses.

Two simultaneous misses for the same asset can both compute SHA-256 before either writes the cache entry. A second check under write lock avoids redundant work.

♻️ Proposed refinement

 func (h *staticHandler) etagFor(assetPath string, data []byte) string {
 	h.etagMu.RLock()
 	if tag, ok := h.etags[assetPath]; ok {
 		h.etagMu.RUnlock()
 		return tag
 	}
 	h.etagMu.RUnlock()
 
 	sum := sha256.Sum256(data)
 	tag := fmt.Sprintf("%q", hex.EncodeToString(sum[:16]))
 
 	h.etagMu.Lock()
+	if existing, ok := h.etags[assetPath]; ok {
+		h.etagMu.Unlock()
+		return existing
+	}
 	h.etags[assetPath] = tag
 	h.etagMu.Unlock()
 	return tag
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/httpapi/static.go` around lines 122 - 136, In etagFor, avoid
duplicated SHA-256 work on concurrent misses by performing a second existence
check after acquiring the write lock: keep the initial read lock check using
etagMu and etags; on miss, release the read lock and acquire etagMu.Lock(), then
check etags[assetPath] again and return it if present; only if still missing
compute sha256.Sum256(data), format the tag (hex.EncodeToString on sum[:16]),
store it in etags[assetPath], unlock etagMu, and return the tag—this implements
double-checked locking in etagFor to prevent redundant hashing.

packages/ui/src/components/metric.tsx (1)

5-10: ReactNode slots don't match the <p> wrappers.

label, value, and hint accept arbitrary JSX, but these slots are rendered inside <p> tags. Passing block content here will produce invalid nesting. Either narrow the props to text-like content or switch those wrappers to <div>/<span> so the public API matches what the component can safely render.

Also applies to: 30-35

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/metric.tsx` around lines 5 - 10, The MetricProps
declare label, value, and hint as ReactNode but the Metric component renders
them inside <p> elements, which causes invalid HTML if callers pass block-level
JSX; update the component so the public API and rendering match: either restrict
MetricProps.label/value/hint to text-like types (e.g., string | number |
React.ReactText) and validate/convert inputs, or change the JSX wrappers used in
the Metric component from <p> to inline/container-safe elements (e.g., <div> or
<span>) so arbitrary ReactNode content is valid; apply the same change for the
other occurrences referenced (the other label/value/hint usages around the
component render). Ensure you update the MetricProps type or the rendered tags
consistently and adjust any tests/styles relying on <p> behavior.

packages/ui/src/components/status-badge.tsx (1)

68-71: Respect reduced-motion for the pulse affordance.

The dot animation currently runs for everyone. In a shared status primitive, gate it with motion-safe: so users who prefer reduced motion still get the status color without the animation.

Suggested change

         <span
           aria-hidden="true"
           className={cn(
             "size-1.5 rounded-full bg-current",
             pulse &&
-              "animate-pulse shadow-[0_0_0_3px_color-mix(in_srgb,currentColor_18%,transparent)]"
+              "motion-safe:animate-pulse shadow-[0_0_0_3px_color-mix(in_srgb,currentColor_18%,transparent)]"
           )}
         />

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/status-badge.tsx` around lines 68 - 71, The status
dot currently always applies the pulse animation when pulse is true; update the
class composition in the StatusBadge component (the className usage where
"size-1.5 rounded-full bg-current" and the pulse conditional is applied) to gate
the animation with the Tailwind motion-safe prefix (e.g. use
"motion-safe:animate-pulse" and motion-safe for the shadow) so users with
reduced-motion preferences see the static dot while still receiving the color;
adjust the conditional that references pulse to only append the
motion-safe-prefixed animation/shadow classes when pulse is true.

web/src/systems/app-shell/components/workspace-onboarding.tsx (1)

50-51: Use unique IDs for assistive-text bindings

Line 50 and Line 51 use hard-coded IDs, which can collide if this component is rendered multiple times on one page. Prefer useId() so aria-describedby targets stay unique.

♻️ Suggested change

-import { useState, type ReactElement, type FormEvent } from "react";
+import { useId, useState, type ReactElement, type FormEvent } from "react";
@@
-  const inputDescriptionId = "workspace-onboarding-input-help";
-  const inputErrorId = "workspace-onboarding-input-error";
+  const baseId = useId();
+  const inputDescriptionId = `${baseId}-workspace-onboarding-input-help`;
+  const inputErrorId = `${baseId}-workspace-onboarding-input-error`;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/systems/app-shell/components/workspace-onboarding.tsx` around lines
50 - 51, The component defines hard-coded IDs inputDescriptionId and
inputErrorId which can collide when multiple WorkspaceOnboarding instances
render; replace these with React's useId() to generate unique IDs (e.g., const
descriptionId = useId(); const errorId = useId()) and update all usages
(aria-describedby and any label/assistive-text references) to reference
descriptionId and errorId instead of the string literals so each instance gets
unique assistive-text bindings.

packages/ui/tests/primitives.test.tsx (1)

153-158: Tighten the disabled assertion

Line 157 checks a broad "disabled" substring. A more specific button-attribute assertion makes this test less brittle.

♻️ Suggested change

-    expect(html).toContain("disabled");
+    expect(html).toMatch(/<button[^>]*\sdisabled(\s|>|=)/);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/tests/primitives.test.tsx` around lines 153 - 158, The test
"renders loading buttons as busy and disabled" uses renderToStaticMarkup(<Button
loading>...) and currently asserts a broad "disabled" substring; replace that
brittle check with a specific button-attribute check such as asserting the
rendered HTML contains the opening button tag with the disabled attribute (e.g.,
expect(html).toContain('<button disabled')) so the test verifies the disabled
attribute is present on the actual button element rendered by the Button
component.

packages/ui/src/tokens.css (1)

14-15: Prefer WOFF2 for shipped mono fonts

Using .ttf for web delivery is heavier; WOFF2 variants would reduce font payload and improve first render performance.

Also applies to: 22-23

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/tokens.css` around lines 14 - 15, Replace the .ttf font
delivery with WOFF2 in the `@font-face` src declarations: update the src entries
that currently reference "./assets/fonts/Disket-Mono-Regular.ttf" (and the
similar src at lines 22-23) to point to corresponding .woff2 files in
assets/fonts, keep a lightweight fallback chain if desired (e.g., woff2 then
woff), and ensure the new .woff2 files are added to the repo and referenced by
the same `@font-face` name so Disket-Mono loads via the smaller WOFF2 payload.

web/src/systems/workflows/components/workflow-inventory-view.tsx (1)

234-253: Extract duplicated workflow action-link classes into one constant.

Lines 234, 243, and 252 duplicate the same class string. Centralizing it will reduce drift and simplify future style changes.

♻️ Proposed refactor

 function WorkflowRow({
   workflow,
   onSync,
   onStartRun,
   onArchive,
   pendingSync,
   pendingStart,
   pendingArchive,
 }: {
@@
 }): ReactElement {
+  const workflowActionLinkClassName =
+    "inline-flex items-center justify-center gap-2 rounded-[var(--radius-md)] border border-border bg-[color:var(--surface-inset)] px-3 py-1.5 text-sm text-foreground transition-colors hover:border-border-strong hover:bg-surface-hover";
+
   return (
     <li>
       <SurfaceCard data-interactive="true" data-testid={`workflow-row-${workflow.slug}`}>
@@
           <Link
-            className="inline-flex items-center justify-center gap-2 rounded-[var(--radius-md)] border border-border bg-[color:var(--surface-inset)] px-3 py-1.5 text-sm text-foreground transition-colors hover:border-border-strong hover:bg-surface-hover"
+            className={workflowActionLinkClassName}
             data-testid={`workflow-view-board-${workflow.slug}`}
@@
           <Link
-            className="inline-flex items-center justify-center gap-2 rounded-[var(--radius-md)] border border-border bg-[color:var(--surface-inset)] px-3 py-1.5 text-sm text-foreground transition-colors hover:border-border-strong hover:bg-surface-hover"
+            className={workflowActionLinkClassName}
             data-testid={`workflow-view-spec-${workflow.slug}`}
@@
           <Link
-            className="inline-flex items-center justify-center gap-2 rounded-[var(--radius-md)] border border-border bg-[color:var(--surface-inset)] px-3 py-1.5 text-sm text-foreground transition-colors hover:border-border-strong hover:bg-surface-hover"
+            className={workflowActionLinkClassName}
             data-testid={`workflow-view-memory-${workflow.slug}`}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/systems/workflows/components/workflow-inventory-view.tsx` around
lines 234 - 253, Extract the repeated class string used on the three Link
components into a single constant (e.g., WORKFLOW_ACTION_LINK_CLASS) declared at
the top of the module and replace the className prop on the Link elements that
render the task board, spec, and memory buttons (the Links with data-testid
values workflow-view-board-{workflow.slug}, workflow-view-spec-{workflow.slug},
and workflow-view-memory-{workflow.slug}) to reference that constant; keep the
constant local to the file and ensure it preserves the exact class value so
behavior and tests remain unchanged.

web/src/systems/spec/components/workflow-spec-view.test.tsx (1)

162-182: Add a test for the “active tab disappears” fallback too.

This covers the workflow.slug reset path, but the new validity guard in workflow-spec-view.tsx is a separate branch. Please also rerender the same workflow after removing the active document and assert that the view falls back to the first present tab; that is the easier regression to miss here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/systems/spec/components/workflow-spec-view.test.tsx` around lines 162
- 182, Add a second assertion to cover the "active tab disappears" branch in
workflow-spec-view.tsx by rerendering the same workflow (keep workflow.id/slug
unchanged) but remove the currently active document(s) (e.g., set the previously
active collection to undefined/empty in the call to view.rerender after clicking
the ADR tab) and then assert the UI falls back to the first present tab (for
example expect the PRD or Techspec tab content to be displayed). Locate the test
using renderSpec in workflow-spec-view.test.tsx and update the test flow after
userEvent.click(screen.getByTestId("workflow-spec-tab-adrs")) to perform the
rerender-without-active-documents and the fallback assertion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/daemon/query_service.go (1)

739-739: Use constant for retrying in lane ordering too.

Line 739 still hard-codes "retrying" while neighboring statuses use constants. Swapping to runStatusRetrying keeps a single source of truth.

♻️ Suggested diff

-	for _, status := range []string{runStatusPending, "running", "retrying", runStatusCompleted, "failed", "canceled"} {
+	for _, status := range []string{runStatusPending, "running", runStatusRetrying, runStatusCompleted, "failed", "canceled"} {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/daemon/query_service.go` at line 739, The loop building the status
list uses a string literal "retrying" instead of the constant; update the slice
in the for loop that iterates over []string{runStatusPending, "running",
"retrying", runStatusCompleted, "failed", "canceled"} to use runStatusRetrying
so all statuses come from the same constants (e.g., replace the "retrying"
literal with runStatusRetrying in the status-slice used for lane ordering).

internal/api/httpapi/security_headers_test.go (1)

27-33: Make CSP assertion directive-aware instead of exact-substring based.

The current check can miss unsafe-inline if token order changes (or if extra sources are inserted). Assert against the parsed script-src directive itself.

🔍 Suggested hardening

 	csp := recorder.Header().Get("Content-Security-Policy")
-	if !strings.Contains(csp, "script-src 'self';") {
-		t.Fatalf("Content-Security-Policy = %q, want script-src 'self';", csp)
-	}
-	if strings.Contains(csp, "script-src 'self' 'unsafe-inline';") {
-		t.Fatalf("Content-Security-Policy = %q, want no unsafe-inline script directive", csp)
-	}
+	var scriptSrc string
+	for _, d := range strings.Split(csp, ";") {
+		d = strings.TrimSpace(d)
+		if strings.HasPrefix(d, "script-src ") {
+			scriptSrc = d
+			break
+		}
+	}
+	if scriptSrc == "" || !strings.Contains(scriptSrc, "'self'") {
+		t.Fatalf("Content-Security-Policy = %q, want script-src to include 'self'", csp)
+	}
+	if strings.Contains(scriptSrc, "'unsafe-inline'") {
+		t.Fatalf("Content-Security-Policy = %q, want script-src without unsafe-inline", csp)
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/httpapi/security_headers_test.go` around lines 27 - 33, The test
currently checks the CSP header via substring matching; instead parse the
Content-Security-Policy header from
recorder.Header().Get("Content-Security-Policy"), split it into directives by
';', locate the "script-src" directive, split that directive into source tokens,
and then assert that the token list contains "'self'" and does NOT contain
"'unsafe-inline'". Update the test around the csp variable to extract the
script-src directive, normalize/trimming whitespace, and perform membership
checks on the parsed tokens rather than substring matches.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes