Fix 6 bugs in the Amicode settings dialog's Developer Tools / Permissions UI

packages/app/src/components/settings-v2/developer-tools-controller.ts

@@ -1,6 +1,8 @@
-import { createSignal, onCleanup, onMount } from "solid-js"
+import { batch, createSignal, onCleanup, onMount } from "solid-js"
 import { useSettings } from "@/context/settings"
 import { inAmicode } from "@/utils/amicode-bridge"
+import { applyRebuildFlagMutation, rebuildFlagMutation } from "./developer-tools-rebuild-flags"
+import { reduceDevToolsRequest } from "./developer-tools-request-state"
 
 export interface DevToolsStatus {
   opencodeValid: boolean
@@ -9,8 +11,6 @@ export interface DevToolsStatus {
   amicodeError?: string
   serverRestarted: boolean
   reloadNeeded: boolean
-  building?: boolean
-  buildError?: string
 }
 
 export type RebuildState = "idle" | "rebuilding" | "rebuilt" | "failed"
@@ -48,7 +48,7 @@ export function createDeveloperToolsController() {
         // Safety timeout: clear after 5 min to avoid permanently stuck state
         setTimeout(() => {
           if (rebuildState() === "rebuilding") {
-            try { localStorage.removeItem("amicode:devtools-rebuilding") } catch {}
+            applyRebuildFlagMutation(rebuildFlagMutation("failed"))
             setRebuildState("failed")
             setRebuildError("Rebuild timed out")
           }
@@ -67,27 +67,29 @@ export function createDeveloperToolsController() {
   const handleMessage = (event: MessageEvent) => {
     const d = event.data
     if (d && d.source === "amicode" && d.kind === "dev-tools-status") {
-      setStatus({
-        opencodeValid: d.opencodeValid ?? true,
-        opencodeError: d.opencodeError,
-        amicodeValid: d.amicodeValid ?? true,
-        amicodeError: d.amicodeError,
-        serverRestarted: d.serverRestarted ?? false,
-        reloadNeeded: d.reloadNeeded ?? false,
-        building: d.building ?? false,
-        buildError: d.buildError,
+      const next = reduceDevToolsRequest(
+        { status: status(), pending: pending() },
+        {
+          type: "status-received",
+          status: {
+            opencodeValid: d.opencodeValid ?? true,
+            opencodeError: d.opencodeError,
+            amicodeValid: d.amicodeValid ?? true,
+            amicodeError: d.amicodeError,
+            serverRestarted: d.serverRestarted ?? false,
+            reloadNeeded: d.reloadNeeded ?? false,
+          },
+        },
+      )
+      batch(() => {
+        setStatus(next.status)
+        setPending(next.pending)
       })
-      setPending(false)
 
       // When a reload is needed (extension was rebuilt), set a flag so the app
       // reopens settings at the developer tools section after the reload.
       if (d.reloadNeeded) {
-        try {
-          localStorage.setItem("amicode:devtools-reopen", "1")
-          localStorage.setItem("amicode:devtools-rebuilt", "1")
-        } catch {
-          // localStorage unavailable — non-critical
-        }
+        applyRebuildFlagMutation({ set: { reopen: "1", rebuilt: "1" }, clear: [] })
       }
     }
 
@@ -97,12 +99,15 @@ export function createDeveloperToolsController() {
         setRebuildState("rebuilding")
         setRebuildError(undefined)
       } else if (d.state === "failed") {
-        try { localStorage.removeItem("amicode:devtools-rebuilding") } catch {}
+        applyRebuildFlagMutation(rebuildFlagMutation("failed"))
         setRebuildState("failed")
         setRebuildError(d.error ?? "Unknown error")
       } else if (d.state === "done") {
-        try { localStorage.removeItem("amicode:devtools-rebuilding") } catch {}
-        // The window reload follows shortly — "rebuilt" flag is read on next mount
+        // The extension host confirmed the build finished — set the
+        // "rebuilt" flag now (not at rebuild-start) so a dialog reopened
+        // after the window reload correctly shows "Rebuilt!" rather than
+        // "Rebuilding..." (#940). The window reload follows shortly.
+        applyRebuildFlagMutation(rebuildFlagMutation("done"))
       }
     }
 
@@ -134,8 +139,12 @@ export function createDeveloperToolsController() {
 
   const sendUpdate = () => {
     if (!inAmicode()) return
-    setPending(true)
-    setStatus(undefined)
+    // Keep the stale status visible (dimmed by the UI via `pending`) instead
+    // of blanking it — clearing it here is what caused the validation
+    // flicker (#940): every path edit made the error/success indicator
+    // vanish and then snap back once the reply arrived.
+    const next = reduceDevToolsRequest({ status: status(), pending: pending() }, { type: "request-sent" })
+    setPending(next.pending)
     window.parent.postMessage(
       {
         source: "amicode",
@@ -153,13 +162,7 @@ export function createDeveloperToolsController() {
     if (rebuildState() === "rebuilding") return // prevent double-clicks
     setRebuildState("rebuilding")
     setRebuildError(undefined)
-    try {
-      localStorage.setItem("amicode:devtools-rebuilding", "1")
-      localStorage.setItem("amicode:devtools-reopen", "1")
-      localStorage.setItem("amicode:devtools-rebuilt", "1")
-    } catch {
-      // non-critical
-    }
+    applyRebuildFlagMutation(rebuildFlagMutation("start"))
     window.parent.postMessage(
       {
         source: "amicode",

packages/app/src/components/settings-v2/developer-tools-rebuild-flags.test.ts

@@ -0,0 +1,84 @@
+import { describe, expect, test } from "bun:test"
+import { rebuildFlagMutation, type RebuildFlagEvent } from "./developer-tools-rebuild-flags"
+
+// ============================================================================
+// The devtools rebuild flag protocol (#940): localStorage flags survive
+// iframe reloads mid-build (git checkout in a watched workspace triggers a
+// reload), so the app's onMount can tell "still rebuilding" apart from
+// "finished while you were away". The bug: rebuild() used to set the
+// "rebuilt" flag at the SAME time as "rebuilding", so reopening the dialog
+// mid-build showed "Rebuilt!" instead of "Rebuilding...". This tests the
+// pure decision of what to set/clear at each lifecycle event, independent
+// of the SolidJS signal wiring around it.
+// ============================================================================
+
+describe("rebuildFlagMutation", () => {
+  test("start sets rebuilding+reopen but never rebuilt", () => {
+    const mutation = rebuildFlagMutation("start")
+    expect(mutation.set).toEqual({ rebuilding: "1", reopen: "1" })
+    expect(mutation.clear).toEqual([])
+    // The exact regression: "rebuilt" must not appear in the start mutation.
+    expect("rebuilt" in mutation.set).toBe(false)
+  })
+
+  test("done clears rebuilding and sets rebuilt — the success signal onMount needs", () => {
+    const mutation = rebuildFlagMutation("done")
+    expect(mutation.clear).toEqual(["rebuilding"])
+    expect(mutation.set).toEqual({ rebuilt: "1" })
+  })
+
+  test("failed clears rebuilding without ever setting rebuilt", () => {
+    const mutation = rebuildFlagMutation("failed")
+    expect(mutation.clear).toEqual(["rebuilding"])
+    expect(mutation.set).toEqual({})
+  })
+
+  test("every event kind produces a defined mutation (exhaustiveness)", () => {
+    const events: RebuildFlagEvent[] = ["start", "done", "failed"]
+    for (const event of events) {
+      expect(rebuildFlagMutation(event)).toBeDefined()
+    }
+  })
+})
+
+describe("rebuild flag lifecycle — localStorage integration", () => {
+  const KEY = (suffix: string) => `amicode:devtools-${suffix}`
+
+  function applyMutation(mutation: ReturnType<typeof rebuildFlagMutation>) {
+    for (const k of mutation.clear) localStorage.removeItem(KEY(k))
+    for (const [k, v] of Object.entries(mutation.set)) localStorage.setItem(KEY(k), v)
+  }
+
+  test("reopening mid-build (before 'done' arrives) shows rebuilding, not rebuilt", () => {
+    localStorage.clear()
+    applyMutation(rebuildFlagMutation("start"))
+
+    // Simulate the dialog reopening mid-build: onMount reads flags directly.
+    const wasRebuilding = localStorage.getItem(KEY("rebuilding")) === "1"
+    const didFinish = localStorage.getItem(KEY("rebuilt")) === "1"
+
+    expect(wasRebuilding).toBe(true)
+    expect(didFinish).toBe(false) // this is the exact bug this fix prevents
+  })
+
+  test("full successful lifecycle: start -> done -> reopen shows rebuilt", () => {
+    localStorage.clear()
+    applyMutation(rebuildFlagMutation("start"))
+    applyMutation(rebuildFlagMutation("done"))
+
+    const wasRebuilding = localStorage.getItem(KEY("rebuilding")) === "1"
+    const didFinish = localStorage.getItem(KEY("rebuilt")) === "1"
+
+    expect(wasRebuilding).toBe(false)
+    expect(didFinish).toBe(true)
+  })
+
+  test("failed lifecycle: start -> failed leaves no success flag behind", () => {
+    localStorage.clear()
+    applyMutation(rebuildFlagMutation("start"))
+    applyMutation(rebuildFlagMutation("failed"))
+
+    expect(localStorage.getItem(KEY("rebuilding"))).toBeNull()
+    expect(localStorage.getItem(KEY("rebuilt"))).toBeNull()
+  })
+})

packages/app/src/components/settings-v2/developer-tools-rebuild-flags.ts

@@ -0,0 +1,58 @@
+/**
+ * The devtools rebuild flag protocol.
+ *
+ * localStorage flags (amicode:devtools-rebuilding / -rebuilt / -reopen)
+ * survive iframe reloads that happen mid-rebuild (e.g. a git checkout
+ * inside a watched workspace folder during a remote rebuild). The
+ * controller's onMount reads them to tell "still rebuilding" apart from
+ * "finished while the dialog was closed".
+ *
+ * This module is the pure decision of what to set/clear at each lifecycle
+ * event, kept separate from the SolidJS signal wiring so the protocol
+ * itself is directly testable. The critical invariant it encodes: "start"
+ * must never set "rebuilt" — only "done" may, and only once the extension
+ * host has actually reported completion.
+ */
+
+export type RebuildFlagEvent = "start" | "done" | "failed"
+
+export type RebuildFlagKey = "rebuilding" | "reopen" | "rebuilt"
+
+export interface RebuildFlagMutation {
+  /** Flags to set to "1". */
+  set: Partial<Record<RebuildFlagKey, "1">>
+  /** Flags to remove. */
+  clear: RebuildFlagKey[]
+}
+
+export function rebuildFlagMutation(event: RebuildFlagEvent): RebuildFlagMutation {
+  switch (event) {
+    case "start":
+      // Rebuild kicks off: mark it in progress and ask the app to reopen
+      // settings at the devtools section after any reload. Do NOT set
+      // "rebuilt" here — that was the bug (#940): it made a mid-build
+      // dialog reopen show "Rebuilt!" instead of "Rebuilding...".
+      return { set: { rebuilding: "1", reopen: "1" }, clear: [] }
+    case "done":
+      // Extension host reported success: clear the in-progress flag and
+      // set the success flag onMount needs to show "Rebuilt!" after reload.
+      return { set: { rebuilt: "1" }, clear: ["rebuilding"] }
+    case "failed":
+      // Extension host reported failure: clear in-progress, no success flag.
+      return { set: {}, clear: ["rebuilding"] }
+  }
+}
+
+const STORAGE_PREFIX = "amicode:devtools-"
+
+/** Apply a mutation to localStorage. Swallows errors (storage may be unavailable). */
+export function applyRebuildFlagMutation(mutation: RebuildFlagMutation): void {
+  try {
+    for (const key of mutation.clear) localStorage.removeItem(STORAGE_PREFIX + key)
+    for (const [key, value] of Object.entries(mutation.set)) {
+      if (value) localStorage.setItem(STORAGE_PREFIX + key, value)
+    }
+  } catch {
+    // non-critical
+  }
+}

packages/app/src/components/settings-v2/developer-tools-request-state.test.ts

@@ -0,0 +1,58 @@
+import { describe, expect, test } from "bun:test"
+import { reduceDevToolsRequest, type DevToolsRequestState } from "./developer-tools-request-state"
+import type { DevToolsStatus } from "./developer-tools-controller"
+
+// ============================================================================
+// The devtools path-validation flicker (#940): sendUpdate() used to clear
+// `status` to undefined the instant a validation round-trip started, so any
+// visible error/success indicator vanished and then snapped back when the
+// reply arrived — a visible flash on every path edit. This models the
+// request lifecycle as a pure reducer so the "don't blank the status while
+// a request is in flight" invariant is directly testable.
+// ============================================================================
+
+const sampleStatus: DevToolsStatus = {
+  opencodeValid: false,
+  opencodeError: "Binary not found at this path",
+  amicodeValid: true,
+  serverRestarted: false,
+  reloadNeeded: false,
+}
+
+const idle: DevToolsRequestState = { status: undefined, pending: false }
+
+describe("reduceDevToolsRequest", () => {
+  test("request-sent while idle marks pending without inventing a status", () => {
+    const next = reduceDevToolsRequest(idle, { type: "request-sent" })
+    expect(next).toEqual({ status: undefined, pending: true })
+  })
+
+  test("status-received clears pending and sets the new status", () => {
+    const sent = reduceDevToolsRequest(idle, { type: "request-sent" })
+    const received = reduceDevToolsRequest(sent, { type: "status-received", status: sampleStatus })
+    expect(received.pending).toBe(false)
+    expect(received.status).toEqual(sampleStatus)
+  })
+
+  test("REGRESSION: a second request-sent must keep the STALE status visible, not blank it", () => {
+    // First round-trip already completed and produced an error.
+    const afterFirst: DevToolsRequestState = { status: sampleStatus, pending: false }
+
+    // User edits the path again — a new request goes out.
+    const midSecondRequest = reduceDevToolsRequest(afterFirst, { type: "request-sent" })
+
+    // This is the exact bug: status must stay visible (not undefined) while
+    // the second round-trip is in flight. Only `pending` should flip.
+    expect(midSecondRequest.status).toEqual(sampleStatus)
+    expect(midSecondRequest.pending).toBe(true)
+  })
+
+  test("the second round-trip's reply replaces the stale status once it lands", () => {
+    const afterFirst: DevToolsRequestState = { status: sampleStatus, pending: false }
+    const midSecondRequest = reduceDevToolsRequest(afterFirst, { type: "request-sent" })
+    const newStatus: DevToolsStatus = { ...sampleStatus, opencodeValid: true, opencodeError: undefined }
+    const afterSecond = reduceDevToolsRequest(midSecondRequest, { type: "status-received", status: newStatus })
+    expect(afterSecond.status).toEqual(newStatus)
+    expect(afterSecond.pending).toBe(false)
+  })
+})

packages/app/src/components/settings-v2/developer-tools-request-state.ts

@@ -0,0 +1,35 @@
+import type { DevToolsStatus } from "./developer-tools-controller"
+
+/**
+ * The devtools path-validation request lifecycle, as a pure reducer.
+ *
+ * Kept separate from the SolidJS signal wiring so the exact invariant that
+ * was wrong (#940) is directly testable: starting a new validation
+ * round-trip must NOT blank the currently-visible status. The old status
+ * (an error or a success indicator) stays on screen — dimmed by the UI via
+ * `pending` — until the new reply actually arrives. Clearing it eagerly is
+ * what produced the flicker: every path edit made the indicator vanish and
+ * then snap back a moment later.
+ */
+
+export interface DevToolsRequestState {
+  status: DevToolsStatus | undefined
+  pending: boolean
+}
+
+export type DevToolsRequestEvent =
+  | { type: "request-sent" }
+  | { type: "status-received"; status: DevToolsStatus }
+
+export function reduceDevToolsRequest(
+  state: DevToolsRequestState,
+  event: DevToolsRequestEvent,
+): DevToolsRequestState {
+  switch (event.type) {
+    case "request-sent":
+      // Keep the stale status visible; only the pending flag changes.
+      return { status: state.status, pending: true }
+    case "status-received":
+      return { status: event.status, pending: false }
+  }
+}