Improve setup wizard onboarding

[akemmanuel]

Summary\n- Improve setup wizard onboarding flow and state handling\n- Clean up obsolete setup wizard locale strings\n- Resolve sidebar merge cleanup\n\n## Checks\n- vp check (passes with existing pi-bridge await-thenable warnings)

[github-actions]

Confidence Score: 3/5

Issues Detected

1. Duplicate state updates in useEffect — SetupWizard.tsx:91-107

refreshHarnessStatus() (line 75) already calls setInventories() and setHarnessState() internally. The useEffect then calls both again with the same values after the function returns. This causes an unnecessary re-render every time the component mounts.

// refreshHarnessStatus already sets:
//   setInventories(result);
//   setHarnessState(hasModelReadyHarness(result) || hasInstalledHarness(result) ? "ready" : "none");

useEffect(() => {
  let cancelled = false;
  void (async () => {
    const result = await refreshHarnessStatus();  // <-- already sets state
    if (cancelled) return;
    setInventories(result);                        // <-- redundant
    setHarnessState(/* same logic */);              // <-- redundant
  })();
  return () => { cancelled = true; };
}, []);

Fix: Remove the duplicate state-sets in the useEffect; it only needs the cancellation guard:

useEffect(() => {
  let cancelled = false;
  void (async () => {
    await refreshHarnessStatus();
    if (cancelled) return;
  })();
  return () => { cancelled = true; };
}, []);

2. Duplicate harness-state logic — SetupWizard.tsx:80-82 and SetupWizard.tsx:98-100

The same ternary expression appears in two places. If the readiness criteria ever changes (e.g., adding "warning" status), both sites must be updated in lockstep.

3. SidebarRail removal — AppSidebar.tsx

SidebarRail is removed from imports and JSX. The component is still exported from src/components/ui/sidebar.tsx:692 but no longer referenced anywhere in the app. Good cleanup, but the dead export could be removed in a follow-up.

What's Working Well

• Properly removes the obsolete "appearance" step from the wizard (reducing from 4 to 3 steps)
• Correctly updated progress dots, step counter, and navigation references
• HarnessInventorySummary now uses i18n keys instead of hardcoded strings
• canUseAnyHarness now includes CLI-only harnesses (hasInstalledHarness), improving the UX for users who install a harness manually but haven't configured models yet
• Obsolete locale keys cleaned from all three language files consistently
• Locale keys deleted match exactly what was removed from the code (no dangling references)

opencode session  |  github run

[github-actions]

Confidence Score: 4/5

Issues Detected

1. SidebarRail removal may break responsive sidebar toggling — src/components/AppSidebar.tsx

SidebarRail in shadcn/ui is not decorative; it renders the clickable rail that lets users collapse/expand the sidebar on larger screens. Removing it means the sidebar can no longer be toggled via the rail edge, only via the programmatic sidebar trigger button. If that trigger isn't present on every view, users on desktop could get stuck with a permanently open sidebar.

// REMOVED — this was the only way to collapse the sidebar by dragging/clicking the edge
<SidebarRail />

Fix: Unless you've verified the sidebar trigger button is always accessible, keep the SidebarRail or swap to a different collapse mechanism.

---

2. Inconsistent return value from refreshHarnessStatus when cancelled — src/components/SetupWizard.tsx:76

When shouldUpdate() returns false in the try block, the function returns the real result array. When it returns false in the catch block, it returns []. The contract is inconsistent. Currently harmless (caller uses void and discards the value), but if a future caller relies on the return type they'll get surprising empty arrays for cancelled errors.

async function refreshHarnessStatus(shouldUpdate = () => true) {
  setHarnessState("detecting");
  try {
    const result = await client.runtime.getHarnessInventories().catch(() => []);
    if (!shouldUpdate()) return result;       // returns real data
    // ...
  } catch {
    if (!shouldUpdate()) return [];            // returns empty
    // ...
  }
}

Fix: Return the same shape in both early-exit paths. Either both return [] or both return result.

---

3. setHarnessState("detecting") fires before cancellation check — src/components/SetupWizard.tsx:74

refreshHarnessStatus synchronously calls setHarnessState("detecting") before any await. If the component unmounts before the async call resolves, the "detecting" state update has already been queued on an unmounted component. In React 18+ this is a no-op warning-wise, but it's a stale state update that could theoretically race if a re-mount happens quickly.

async function refreshHarnessStatus(shouldUpdate = () => true) {
  setHarnessState("detecting");  // fires even if shouldUpdate returns false at next check

Fix: Guard the initial state set or move it after the cancellation check:

async function refreshHarnessStatus(shouldUpdate = () => true) {
  if (!shouldUpdate()) return [];
  setHarnessState("detecting");
  // ...

---

4. Button layout justify-end → justify-between is a notable visual change — src/components/SetupWizard.tsx:209

The "Check again" button moves from the right edge (grouped with other actions) to the far left, creating visual separation. This is a deliberate UX choice but should be confirmed with design.

---

What's Working Well

• Fix of duplicate state updates: The old useEffect called refreshHarnessStatus() (which already sets state internally) and then redundantly called setInventories + setHarnessState again. The PR correctly eliminates this double-render by passing a cancellation callback directly into refreshHarnessStatus.
• Dead empty-if removed: The old code had if (result.some(...)) { } (empty body) — now gone.
• stepNumber refactored from fragile array-index lookup to explicit Record<Step, number> — much clearer.
• Locale cleanup is complete: All removed code references have corresponding key deletions in all three languages (en, de, es). No dangling keys.
• canUseAnyHarness now includes hasInstalledHarness: Previously only model-ready harnesses counted, meaning CLI-only installs wouldn't advance the flow. Fixed.
• HarnessInventorySummary internationalized: Now uses t() calls instead of hardcoded English strings.

opencode session  |  github run