Skip to content

fix(gui): clear a stale install failure once that action succeeded (#1245) #1275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lidge-jun merged 1 commit into from
Aug 8, 2026
Merged

fix(gui): clear a stale install failure once that action succeeded (#1245) #1275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions gui/src/pages/Startup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export default function Startup({ apiBase }: { apiBase: string }) {
const [trayBusy, setTrayBusy] = useState(false);
const [trayError, setTrayError] = useState(false);
const [installBusy, setInstallBusy] = useState<StartupInstallAction | null>(null);
const [installResult, setInstallResult] = useState<{ kind: "success" | "error"; action: StartupInstallAction; repair?: boolean; detail?: string } | null>(null);
const [installResult, setInstallResult] = useState<{ kind: "success" | "error"; action: StartupInstallAction; repair?: boolean; detail?: string; forLocalRouting?: boolean } | null>(null);
const [codexRuntimeWarning, setCodexRuntimeWarning] = useState<string | null>(() => cached?.warning ?? null);
const [codexRuntimeFix, setCodexRuntimeFix] = useState<string | null>(() => cached?.fix ?? null);
/** True while settings (runtime notice) are still in flight — reserves notice slot height. */
Expand All @@ -107,6 +107,29 @@ export default function Startup({ apiBase }: { apiBase: string }) {
const res = await fetch(`${apiBase}/api/startup-health`, { signal });
if (!res.ok) throw new Error("fetch failed");
const next = await res.json() as StartupHealthData;
// #1245: a failed install notice is a claim about ONE attempt, not about the
// current state. Once health independently shows that attempt's goal is met —
// the user may well have reached it another way — the notice contradicts what
// the same page is showing and must yield.
//
// Scoped per action deliberately. `status` is overall restart safety, so a
// machine protected by the service would otherwise erase a failed SHIM
// install, which is still true and still actionable. Each action clears only
// against the health field it was trying to change; restoring native routing
// retires both, since neither install is outstanding then.
setInstallResult(current => {
if (current?.kind !== "error") return current;
Comment on lines +120 to +121

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reconcile failures regardless of response ordering

When a user clicks Refresh while a long-running install request is still pending, the refreshed health can report the action's goal as satisfied before the POST eventually returns an error. This updater then sees installResult === null and does nothing; the later catch stores the failure, leaving the page showing healthy state alongside the stale error until another refresh. The Refresh button is disabled only by loading, not installBusy, so this ordering is possible. Reconcile whenever either health or the failure changes (or compare the failure against the latest health when storing it), and add a test where the health response completes before the failed action response.

Useful? React with 👍 / 👎.

// Native routing retires an install that existed to protect a local
// routing dependency. It does not retire an optional shim a native
// machine can still install — that button is still on the page.
if (next.status === "native" && current.forLocalRouting === true) return null;
const satisfied = current.action === "install-service"
// serviceViable, not installed-and-running: a stale or conflicting
// service can be both while the page still reports it unhealthy.
? next.serviceViable
: next.shimInstalled && next.shimHealthy;
return satisfied ? null : current;
});
paintedRef.current = true;
const prevCache = readSessionListCache<StartupPageCache>(cacheKey);
writeSessionListCache(cacheKey, {
Expand Down Expand Up @@ -247,7 +270,11 @@ export default function Startup({ apiBase }: { apiBase: string }) {
setInstallResult({ kind: "success", action, repair: opts?.repair === true });
refresh();
} catch (error) {
setInstallResult({ kind: "error", action, repair: opts?.repair === true, detail: error instanceof Error ? error.message : String(error) });
// #1245: remember whether this attempt was made while startup depended on
// local routing. A later switch to native retires an install that existed to
// protect that dependency, but says nothing about an optional shim a native
// machine can still choose to install.
setInstallResult({ kind: "error", action, repair: opts?.repair === true, detail: error instanceof Error ? error.message : String(error), forLocalRouting: data?.localRoutingDependency === true });
} finally {
setInstallBusy(null);
}
Expand Down
259 changes: 259 additions & 0 deletions gui/tests/startup-install-result-reconciliation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,259 @@
import { afterEach, beforeEach, expect, test } from "bun:test";
import { Window } from "happy-dom";
import { act } from "react";
import type { Root } from "react-dom/client";
import { LanguageProvider } from "../src/i18n/provider";
import { clearClientResourceStoresForTests } from "../src/client-resource";
import Startup from "../src/pages/Startup";
import { writeSessionListCache } from "../src/session-list-cache";

const globals = ["document", "window", "navigator", "localStorage", "sessionStorage", "IS_REACT_ACT_ENVIRONMENT"] as const;
let previousGlobals: Record<(typeof globals)[number], unknown>;
let testWindow: Window;
const originalFetch = globalThis.fetch;

const API_BASE = "http://localhost";
const CACHE_KEY = `ocx.startup.page.v1:${API_BASE}`;

function atRiskHealth() {
return {
status: "at-risk",
routingKind: "opencodex-local",
routingInjected: true,
localRoutingDependency: true,
autostartEnabled: false,
rebootSafe: false,
protection: "none",
serviceInstalled: false,
serviceViable: false,
serviceEnabled: false,
serviceRunning: false,
serviceStale: false,
serviceConflict: false,
serviceSupported: true,
shimInstalled: false,
shimHealthy: false,
shimCoverage: "none",
platform: "darwin",
recommendedCommand: "ocx service install",
diagnosticStale: false,
commands: { installService: "ocx service install", repairService: "ocx service repair", installShim: "ocx shim install", restoreNative: "ocx restore" },
};
}

function protectedHealth() {
return { ...atRiskHealth(), status: "protected", protection: "service", serviceInstalled: true, serviceViable: true, serviceEnabled: true, serviceRunning: true, rebootSafe: true };
}

beforeEach(() => {
clearClientResourceStoresForTests();
previousGlobals = Object.fromEntries(globals.map(key => [key, Reflect.get(globalThis, key)])) as typeof previousGlobals;
testWindow = new Window({ url: "http://localhost/" });
Object.defineProperties(globalThis, {
document: { configurable: true, value: testWindow.document },
window: { configurable: true, value: testWindow.window },
navigator: { configurable: true, value: testWindow.navigator },
localStorage: { configurable: true, value: testWindow.localStorage },
sessionStorage: { configurable: true, value: testWindow.sessionStorage },
});
(globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
testWindow.sessionStorage.clear();
});

afterEach(() => {
globalThis.fetch = originalFetch;
clearClientResourceStoresForTests();
testWindow.close();
for (const key of globals) {
Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] });
}
});

// #1245: a failed install notice is a claim about one attempt. If the user then
// reaches that attempt's goal another way -- the CLI, the service manager, a retry
// the page did not initiate -- the refresh reports success while the stale failure
// keeps claiming otherwise, with no way to dismiss it.
//
// The clearing is scoped per action on purpose, and the second test pins that:
// `status` is overall restart safety, so a machine protected by the service must
// NOT erase a failed shim install, which is still true and still actionable.

// The two install buttons both read "Install"; only their aria-label
// distinguishes service from shim. Match the accessible name so a copy change
// cannot silently retarget the click.
function accessibleName(el: Element): string {
return el.getAttribute("aria-label") ?? el.textContent ?? "";
}

function clickTarget(pattern: RegExp): HTMLButtonElement {
const button = [...container().querySelectorAll("button")]
.find(b => pattern.test(accessibleName(b)));
if (!button) {
const names = [...container().querySelectorAll("button")].map(accessibleName);
throw new Error(`no button matching ${pattern}; saw: ${JSON.stringify(names)}`);
}
return button as unknown as HTMLButtonElement;
}

let containerEl: HTMLElement | null = null;
function container(): HTMLElement {
if (!containerEl) throw new Error("container not mounted");
return containerEl;
}

async function mount(health: () => Record<string, unknown>, onAction: () => Response) {
const { createRoot } = await import("react-dom/client");
containerEl = document.createElement("div");
document.body.append(containerEl);
globalThis.fetch = (async (input: RequestInfo | URL) => {
const url = String(input);
if (url.includes("/api/settings")) return Response.json({ codexRuntime: {} });
if (url.includes("/api/startup-action")) return onAction();
if (url.includes("/api/startup-health")) return Response.json(health());
return new Response(null, { status: 404 });
}) as typeof fetch;

let root!: Root;
await act(async () => {
root = createRoot(containerEl!);
root.render(<LanguageProvider><Startup apiBase={API_BASE} /></LanguageProvider>);
});
await settle();
return root;
}

// Drain the microtask queue and any zero-delay timers React scheduled, without
// betting on a fixed wall-clock duration.
async function settle() {
for (let i = 0; i < 8; i++) {
await act(async () => { await new Promise<void>(r => testWindow.setTimeout(r, 0)); });
}
}

test("a failed service install clears once the service is independently running (#1245)", async () => {
let health = atRiskHealth();
const root = await mount(() => health, () => Response.json({ error: "service install failed" }, { status: 500 }));

await act(async () => {
clickTarget(/service.*install/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();
expect(container().textContent).toContain("service install failed");

// The service comes up by some other route; the page refreshes.
health = { ...atRiskHealth(), status: "protected", protection: "service", serviceInstalled: true, serviceViable: true, serviceEnabled: true, serviceRunning: true, rebootSafe: true };
await act(async () => {
clickTarget(/refresh/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();

expect(container().textContent).not.toContain("service install failed");

await act(async () => { root.unmount(); });
containerEl!.remove();
containerEl = null;
});

test("a failed shim install survives a refresh that only proves the service is healthy (#1245)", async () => {
let health = atRiskHealth();
const root = await mount(() => health, () => Response.json({ error: "shim install failed" }, { status: 500 }));

await act(async () => {
clickTarget(/shim.*install/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();
expect(container().textContent).toContain("shim install failed");

// Restart safety is now covered BY THE SERVICE. The shim is still not installed,
// so the shim failure is still true — clearing it here would hide a real problem
// behind an unrelated success.
health = { ...atRiskHealth(), status: "protected", protection: "service", serviceInstalled: true, serviceViable: true, serviceEnabled: true, serviceRunning: true, rebootSafe: true };
await act(async () => {
clickTarget(/refresh/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();

expect(container().textContent).toContain("shim install failed");

await act(async () => { root.unmount(); });
containerEl!.remove();
containerEl = null;
});
Comment on lines +157 to +181

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a successful shim-reconciliation test.

gui/src/pages/Startup.tsx Lines 126-131 clear an install-shim failure only when shimInstalled and shimHealthy are both true. This test verifies retention when only the service becomes viable, but it does not verify that the shim failure clears after the shim becomes healthy.

Add a test that fails a shim install, refreshes with both fields true, and asserts that the failure notice is removed. This protects the action-specific shim predicate from regression.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gui/tests/startup-install-result-reconciliation.test.tsx` around lines 157 -
181, Add a test alongside the existing shim-install reconciliation test that
first records a failed shim installation, then refreshes with both shimInstalled
and shimHealthy set to true, and asserts the shim failure notice is removed.
Reuse the existing mount, click, settle, and cleanup helpers and target the
reconciliation behavior in Startup.


test("a failed service repair survives a refresh where the service is still stale (#1245)", async () => {
let health = { ...atRiskHealth(), serviceInstalled: true, serviceRunning: true, serviceStale: true, serviceViable: false };
const root = await mount(() => health, () => Response.json({ error: "service repair failed" }, { status: 500 }));

await act(async () => {
clickTarget(/service.*repair/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();
expect(container().textContent).toContain("service repair failed");

// A stale or conflicting service can be installed AND running while the page
// still reports it unhealthy. Installed-and-running is therefore not proof the
// repair worked; serviceViable is what the UI itself treats as healthy.
health = { ...health, serviceInstalled: true, serviceRunning: true, serviceStale: true, serviceViable: false };
await act(async () => {
clickTarget(/refresh/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();

expect(container().textContent).toContain("service repair failed");

await act(async () => { root.unmount(); });
containerEl!.remove();
containerEl = null;
});

test("a failed shim install on an already-native machine survives a native refresh (#1245)", async () => {
// Native routing means no local routing dependency to protect, but the shim
// Install button is still offered, so a failed optional shim install is still
// true. Clearing on `native` alone would hide it.
const nativeHealth = () => ({ ...atRiskHealth(), status: "native", routingKind: "native", routingInjected: false, localRoutingDependency: false, protection: "none", rebootSafe: true });
const root = await mount(nativeHealth, () => Response.json({ error: "shim install failed" }, { status: 500 }));

await act(async () => {
clickTarget(/shim.*install/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();
expect(container().textContent).toContain("shim install failed");

await act(async () => {
clickTarget(/refresh/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();

expect(container().textContent).toContain("shim install failed");

await act(async () => { root.unmount(); });
containerEl!.remove();
containerEl = null;
});

test("a failed install clears when the user restores native routing instead (#1245)", async () => {
// The positive side of the routing-provenance branch. The attempt was made
// while startup depended on the local proxy; restoring native routing removes
// that dependency, so the install it was protecting is no longer outstanding
// and its failure notice is obsolete.
let health = atRiskHealth();
const root = await mount(() => health, () => Response.json({ error: "service install failed" }, { status: 500 }));

await act(async () => {
clickTarget(/service.*install/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();
expect(container().textContent).toContain("service install failed");

health = { ...atRiskHealth(), status: "native", routingKind: "native", routingInjected: false, localRoutingDependency: false, protection: "none", rebootSafe: true };
await act(async () => {
clickTarget(/refresh/i).dispatchEvent(new testWindow.Event("click", { bubbles: true }));
});
await settle();

expect(container().textContent).not.toContain("service install failed");

await act(async () => { root.unmount(); });
containerEl!.remove();
containerEl = null;
});
Loading