Skip to content

Commit 8d0984c

Browse files
suraj-markupclaude
andcommitted
fix(release-train): Windows spawn + dismiss-while-blocked review feedback
- (P1) `ao update` silently never ran on Windows because `spawn("ao", ...)` doesn't consult PATHEXT, so npm's `ao.cmd` shim wasn't found and the async ENOENT was swallowed by the error handler. Add `shell: isWindows()` + `windowsHide: true` per the cross-platform guide. - (P1) Dismiss button was inert when the banner was in the `blocked` (409) or `error` phase — `setDismissedFor` set the localStorage flag but the hide condition required `phase === "idle"`, so the banner stayed pinned until reload. `handleDismiss` now resets phase to idle (and clears the error message) so the existing condition fires. Added a regression test covering dismiss from the 409 path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a5a52b9 commit 8d0984c

3 files changed

Lines changed: 49 additions & 6 deletions

File tree

packages/web/src/app/api/update/route.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
import { spawn } from "node:child_process";
1515
import { NextResponse, type NextRequest } from "next/server";
16+
import { isWindows } from "@aoagents/ao-core";
1617
import { getServices } from "@/lib/services";
1718

1819
export const dynamic = "force-dynamic";
@@ -64,16 +65,22 @@ export async function POST(_req: NextRequest) {
6465
// tears down the dashboard process. We rely on PATH resolution because the
6566
// user installed `ao` themselves — there's no canonical install location.
6667
//
67-
// ENOENT (binary missing from PATH) fires asynchronously as an `error`
68-
// event on the child, NOT as a sync throw — without an explicit handler
69-
// it would propagate as an unhandled error and crash the test runner. So
70-
// we attach a noop handler here; in production, the worst-case is the
71-
// banner shows "Update started" but nothing actually installs, which the
72-
// user will notice on next page load.
68+
// `shell: isWindows()` is required so PATHEXT gets consulted on Windows —
69+
// npm's `ao` shim is `ao.cmd`, and Node.js does not look at PATHEXT for
70+
// non-shell spawns, so the bare `ao` lookup would silently ENOENT on every
71+
// Windows install. `windowsHide: true` keeps the shell window from flashing.
72+
//
73+
// ENOENT still fires asynchronously as an `error` event on the child (POSIX
74+
// case where `ao` isn't on PATH), NOT as a sync throw — without an explicit
75+
// handler it would propagate as an unhandled error and crash the test
76+
// runner. The handler is a noop in production; the user will see "no
77+
// version change" on next page load if the install never ran.
7378
try {
7479
const child = spawn("ao", ["update"], {
7580
detached: true,
7681
stdio: "ignore",
82+
shell: isWindows(),
83+
windowsHide: true,
7784
});
7885
child.on("error", () => {
7986
// Swallow async spawn errors (ENOENT etc.) so they don't become

packages/web/src/components/UpdateBanner.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,12 @@ export function UpdateBanner() {
6868
// ignore
6969
}
7070
setDismissedFor(info.latest);
71+
// Reset to idle so the hide condition (`dismissedFor === info.latest &&
72+
// phase === "idle"`) fires even when the user dismisses while we're
73+
// showing a 409 / error message. Without this the banner would stay
74+
// pinned on screen until the user reloads.
75+
setPhase("idle");
76+
setErrorMessage(null);
7177
}, [info]);
7278

7379
const handleUpdate = useCallback(async () => {

packages/web/src/components/__tests__/UpdateBanner.test.tsx

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,4 +155,34 @@ describe("UpdateBanner", () => {
155155

156156
await screen.findByText(/3 sessions active/);
157157
});
158+
159+
it("dismiss button hides the banner even from the 'blocked' (409) error state", async () => {
160+
fetchMock
161+
.mockResolvedValueOnce(
162+
mockVersionResponse({
163+
current: "0.5.0",
164+
latest: "0.5.1",
165+
channel: "stable",
166+
isOutdated: true,
167+
}),
168+
)
169+
.mockResolvedValueOnce({
170+
ok: false,
171+
status: 409,
172+
json: async () => ({
173+
ok: false,
174+
message: "1 session active. Run `ao stop` first.",
175+
activeSessions: 1,
176+
}),
177+
} as Response);
178+
179+
const { container } = render(<UpdateBanner />);
180+
const update = await screen.findByRole("button", { name: "Update" });
181+
fireEvent.click(update);
182+
// Wait for the 409 to surface so we're definitely in the blocked phase.
183+
await screen.findByText(/1 session active/);
184+
185+
fireEvent.click(screen.getByRole("button", { name: "Dismiss" }));
186+
await waitFor(() => expect(container.firstChild).toBeNull());
187+
});
158188
});

0 commit comments

Comments
 (0)