From f8a1f3d5af9f700da1d3d7477067d9637d2634cf Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Fri, 17 Jul 2026 09:53:35 -0700 Subject: [PATCH 1/5] fix(ui): retry the prerequisite probe after a transient failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review (PR #19) caught that a transient prerequisite-probe failure wedged the shared singleton: probe()'s catch reset `check` to null but left `loading` holding the resolved promise, so load() no-op'd for the rest of the session — canEditHistory stayed true (history editing wrongly enabled) and the banner stayed hidden, with no retry path. - prerequisites.svelte.ts: reset `loading = null` in the catch so a later load()/refresh() actually re-probes. - PrereqBanner.svelte: re-probe on window focus when the probe hasn't succeeded (check === null), not only when a prerequisite is known-missing. Co-Authored-By: Claude Opus 4.8 --- src/lib/components/PrereqBanner.svelte | 9 +++++---- src/lib/prerequisites.svelte.ts | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/lib/components/PrereqBanner.svelte b/src/lib/components/PrereqBanner.svelte index 6fe8a31..efc5228 100644 --- a/src/lib/components/PrereqBanner.svelte +++ b/src/lib/components/PrereqBanner.svelte @@ -8,11 +8,12 @@ onMount(() => { void prerequisites.load(); - // Re-probe when the user returns to the app while a prerequisite is still missing — - // e.g. after finishing the async Command Line Tools installer — so the banner clears - // and the editing controls re-enable without a restart. Stops once everything's met. + // Re-probe when the user returns to the app and prerequisites aren't confirmed met — + // after finishing the async Command Line Tools installer, OR to retry a probe that + // transiently failed (check === null) — so the banner clears and the editing controls + // re-enable without a restart. Stops once everything's met (check set + editable). const onReturn = () => { - if (prerequisites.check && !prerequisites.canEditHistory) void prerequisites.refresh(); + if (!prerequisites.check || !prerequisites.canEditHistory) void prerequisites.refresh(); }; window.addEventListener("focus", onReturn); return () => window.removeEventListener("focus", onReturn); diff --git a/src/lib/prerequisites.svelte.ts b/src/lib/prerequisites.svelte.ts index c42c375..e537a56 100644 --- a/src/lib/prerequisites.svelte.ts +++ b/src/lib/prerequisites.svelte.ts @@ -15,6 +15,11 @@ function makePrerequisites() { check = await api.checkPrerequisites(); } catch { check = null; + // Clear the cached promise so a later load()/refresh() actually re-probes. Without + // this, a transient failure leaves `loading` holding this (resolved) promise, so + // load() no-ops forever and canEditHistory stays true — history editing wrongly + // enabled, and the banner hidden, for the rest of the session. + loading = null; } } From 09d26f95bdc3f866cc02d637f64da2e09e91ac4b Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Fri, 17 Jul 2026 10:18:46 -0700 Subject: [PATCH 2/5] fix(ui): keep history editing disabled after a failed prerequisite probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex (PR #20 review) noted the first fix still left the destructive rewrite reachable: canEditHistory returned true whenever `check` was null, which includes a FAILED probe — so between a transient failure and a successful retry, "Rewrite history" ran on unverified prerequisites with no warning. Track the failure separately: a new `probeFailed` flag distinguishes "probe rejected" from "not probed yet". canEditHistory now allows only when a probe succeeded or hasn't run yet (optimistic pre-first-probe / non-Tauri), and stays DISABLED once a probe has failed until a later probe succeeds. Guard the probe with isTauri so the browser preview stays "unknown", not "failed". Co-Authored-By: Claude Opus 4.8 --- src/lib/prerequisites.svelte.ts | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/lib/prerequisites.svelte.ts b/src/lib/prerequisites.svelte.ts index e537a56..885d345 100644 --- a/src/lib/prerequisites.svelte.ts +++ b/src/lib/prerequisites.svelte.ts @@ -6,19 +6,29 @@ import { api } from "./api"; import type { PrerequisiteCheck } from "./types"; +function isTauri(): boolean { + return typeof window !== "undefined" && "__TAURI_INTERNALS__" in window; +} + function makePrerequisites() { let check = $state(null); + // True once a probe has REJECTED without producing a result — distinct from "not probed + // yet" (check null, probeFailed false). Keeps history editing disabled while set. + let probeFailed = $state(false); let loading: Promise | null = null; async function probe() { + // Non-Tauri (browser preview) has no backend to probe — leave the state "unknown" + // (not a failure) so canEditHistory stays optimistic there. + if (!isTauri()) return; try { check = await api.checkPrerequisites(); + probeFailed = false; } catch { check = null; - // Clear the cached promise so a later load()/refresh() actually re-probes. Without - // this, a transient failure leaves `loading` holding this (resolved) promise, so - // load() no-ops forever and canEditHistory stays true — history editing wrongly - // enabled, and the banner hidden, for the rest of the session. + probeFailed = true; + // Clear the cached promise so a later load()/refresh() re-probes a transient failure + // instead of no-op'ing on this (resolved) promise for the rest of the session. loading = null; } } @@ -27,11 +37,14 @@ function makePrerequisites() { get check() { return check; }, - // Commit-time editing needs git + python3 + a working bundled filter-repo. Unknown - // (not yet probed / non-Tauri) → treated as available so we never block prematurely; - // a genuinely-missing tool still fails loudly at run time. + // True only when a SUCCESSFUL probe confirms git + python3 + a working bundled + // filter-repo. Before the first probe resolves — and in non-Tauri — we stay optimistic + // (check null, probeFailed false) so the control doesn't flash disabled. But once a + // probe has FAILED we keep editing DISABLED: a destructive history rewrite must never + // run on unverified prerequisites. A later successful probe re-enables it. get canEditHistory() { - return check ? check.git && check.python3 && check.filterRepo : true; + if (check) return check.git && check.python3 && check.filterRepo; + return !probeFailed; }, // Probe once, then share the result — deduped so the banner and ApplyPanel don't // double-invoke. From d39fb48f512432cd4c415ab4efad830e56258187 Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Fri, 17 Jul 2026 10:18:46 -0700 Subject: [PATCH 3/5] fix(github): don't retry explicit rate limits in commit-activity fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex (PR #20 review) flagged that isRetryableActivityError retried a RateLimited error, so fetchActivityWithRetry hit the same endpoint five times over ~6s — a rate limit won't clear inside that fixed backoff, and re-hitting it prolongs the throttle. Narrow the predicate to the transient cold-cache case only: retry `Other`/parse errors and non-GithubError blips; do NOT retry RateLimited, Forbidden, or the permanent kinds. Update the unit tests accordingly. Co-Authored-By: Claude Opus 4.8 --- src/lib/github/activity.test.ts | 26 +++++++++++++------------- src/lib/github/activity.ts | 23 +++++++++++------------ 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/lib/github/activity.test.ts b/src/lib/github/activity.test.ts index 2893dc7..65426a6 100644 --- a/src/lib/github/activity.test.ts +++ b/src/lib/github/activity.test.ts @@ -15,14 +15,12 @@ describe("sparklinePoints", () => { }); describe("isRetryableActivityError", () => { - it("retries cold-stats / transient failures", () => { - for (const kind of ["Other", "RateLimited", "Forbidden"]) { - expect(isRetryableActivityError({ kind })).toBe(true); - } + it("retries only transient cold-cache failures", () => { + expect(isRetryableActivityError({ kind: "Other", message: "parse activity" })).toBe(true); expect(isRetryableActivityError("network blip")).toBe(true); // non-GithubError → retry }); - it("does not retry permanent failures", () => { - for (const kind of ["NotFound", "NotAuthed", "NoRemote", "NotInstalled"]) { + it("does not retry explicit rate limits or permanent failures", () => { + for (const kind of ["RateLimited", "Forbidden", "NotFound", "NotAuthed", "NoRemote", "NotInstalled"]) { expect(isRetryableActivityError({ kind })).toBe(false); } }); @@ -70,15 +68,17 @@ describe("fetchActivityWithRetry", () => { expect(fetchOnce).toHaveBeenCalledTimes(3); }); - it("aborts immediately on a PERMANENT error without burning retries", async () => { - const fetchOnce = vi.fn().mockRejectedValue({ kind: "NotFound" }); - await expect(fetchActivityWithRetry(fetchOnce, noSleep, 5)).rejects.toEqual({ kind: "NotFound" }); - expect(fetchOnce).toHaveBeenCalledTimes(1); + it("aborts immediately on a non-retryable error (permanent OR explicit rate limit)", async () => { + for (const kind of ["NotFound", "RateLimited"]) { + const fetchOnce = vi.fn().mockRejectedValue({ kind }); + await expect(fetchActivityWithRetry(fetchOnce, noSleep, 5)).rejects.toEqual({ kind }); + expect(fetchOnce).toHaveBeenCalledTimes(1); + } }); - it("propagates the last error when a transient failure never clears", async () => { - const fetchOnce = vi.fn().mockRejectedValue({ kind: "RateLimited" }); - await expect(fetchActivityWithRetry(fetchOnce, noSleep, 3)).rejects.toEqual({ kind: "RateLimited" }); + it("propagates the last error when a transient (Other) failure never clears", async () => { + const fetchOnce = vi.fn().mockRejectedValue({ kind: "Other", message: "parse activity" }); + await expect(fetchActivityWithRetry(fetchOnce, noSleep, 3)).rejects.toMatchObject({ kind: "Other" }); expect(fetchOnce).toHaveBeenCalledTimes(3); }); }); diff --git a/src/lib/github/activity.ts b/src/lib/github/activity.ts index cf4b41a..1531e5d 100644 --- a/src/lib/github/activity.ts +++ b/src/lib/github/activity.ts @@ -11,19 +11,18 @@ export function sparklinePoints(values: number[], w: number, h: number): string .join(" "); } -/** A cold-stats failure is worth retrying; a missing repo / auth / remote problem is not. - * GitHub computes /stats/commit_activity lazily (HTTP 202 the first time), so the initial - * request can fail transiently — a rate limit, a 202 surfaced as an error, an unparseable - * placeholder body. Those clear on their own; NotFound/NotAuthed/NoRemote/NotInstalled won't. */ +/** Only a transient cold-cache failure is worth the short fixed backoff. GitHub computes + * /stats/commit_activity lazily (HTTP 202 the first time), which can surface as an + * unparseable placeholder body — an `Other`/parse error — or a non-GithubError network + * blip. Explicit terminal states are NOT retried: `RateLimited` won't clear inside the + * backoff window (and re-hitting it prolongs the throttle), and Forbidden / NotFound / + * NotAuthed / NoRemote / NotInstalled are permanent. Reserve the retries for the + * computing/parse case; a real rate limit should surface (and wait for its reset) instead. */ export function isRetryableActivityError(e: unknown): boolean { - const kind = - e && typeof e === "object" && "kind" in e ? (e as GithubError).kind : "Other"; - return ( - kind !== "NotFound" && - kind !== "NotAuthed" && - kind !== "NoRemote" && - kind !== "NotInstalled" - ); + if (e && typeof e === "object" && "kind" in e) { + return (e as GithubError).kind === "Other"; + } + return true; // non-GithubError throw → treat as a transient blip } /** Fetch commit activity, retrying through GitHub's lazy-computation window. From dc01bb6b1e03a64a3e7a617984cee44d63b482c5 Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Fri, 17 Jul 2026 10:27:21 -0700 Subject: [PATCH 4/5] fix(ui): serialize prerequisite probes so a stale one can't re-disable editing Codex (PR #20 re-review) flagged a race: refresh() (window-focus) can start a probe while another is still in flight, and both write check/probeFailed on completion. A newer success followed by an older failure completing late would clobber the good result and wrongly re-disable history editing until the next focus cycle. Add a monotonic probeSeq: each probe captures its id and commits its result only if it is still the latest, so superseded/out-of-order completions are dropped. Co-Authored-By: Claude Opus 4.8 --- src/lib/prerequisites.svelte.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/lib/prerequisites.svelte.ts b/src/lib/prerequisites.svelte.ts index 885d345..4130faa 100644 --- a/src/lib/prerequisites.svelte.ts +++ b/src/lib/prerequisites.svelte.ts @@ -16,15 +16,24 @@ function makePrerequisites() { // yet" (check null, probeFailed false). Keeps history editing disabled while set. let probeFailed = $state(false); let loading: Promise | null = null; + // Monotonic probe id. refresh() can start a probe while another is still in flight, so + // only the LATEST probe may commit its result — otherwise a stale/out-of-order completion + // (e.g. an older failure resolving after a newer success) would clobber the current state + // and wrongly re-disable editing. + let probeSeq = 0; async function probe() { // Non-Tauri (browser preview) has no backend to probe — leave the state "unknown" // (not a failure) so canEditHistory stays optimistic there. if (!isTauri()) return; + const seq = ++probeSeq; try { - check = await api.checkPrerequisites(); + const result = await api.checkPrerequisites(); + if (seq !== probeSeq) return; // superseded by a newer probe — drop this result + check = result; probeFailed = false; } catch { + if (seq !== probeSeq) return; // superseded by a newer probe — drop this stale failure check = null; probeFailed = true; // Clear the cached promise so a later load()/refresh() re-probes a transient failure From 55cea093602ad7188a0edadaf6057e25d6437088 Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Fri, 17 Jul 2026 10:59:50 -0700 Subject: [PATCH 5/5] fix(ui): disable history editing until a successful desktop probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex (PR #20 re-review) flagged that a still-pending initial probe left canEditHistory true (check null), so a slow probe let "Rewrite history" run before git/python3/filter-repo were verified — the probeFailed flag only covered the FAILED case, not the pending one. Make canEditHistory optimistic only in non-Tauri (browser preview): in the desktop app any non-success check (pending, failed, or unprobed) keeps editing DISABLED until a successful probe confirms availability. This subsumes the probeFailed flag, which is removed. Co-Authored-By: Claude Opus 4.8 --- src/lib/prerequisites.svelte.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/lib/prerequisites.svelte.ts b/src/lib/prerequisites.svelte.ts index 4130faa..522e224 100644 --- a/src/lib/prerequisites.svelte.ts +++ b/src/lib/prerequisites.svelte.ts @@ -12,9 +12,6 @@ function isTauri(): boolean { function makePrerequisites() { let check = $state(null); - // True once a probe has REJECTED without producing a result — distinct from "not probed - // yet" (check null, probeFailed false). Keeps history editing disabled while set. - let probeFailed = $state(false); let loading: Promise | null = null; // Monotonic probe id. refresh() can start a probe while another is still in flight, so // only the LATEST probe may commit its result — otherwise a stale/out-of-order completion @@ -31,11 +28,9 @@ function makePrerequisites() { const result = await api.checkPrerequisites(); if (seq !== probeSeq) return; // superseded by a newer probe — drop this result check = result; - probeFailed = false; } catch { if (seq !== probeSeq) return; // superseded by a newer probe — drop this stale failure check = null; - probeFailed = true; // Clear the cached promise so a later load()/refresh() re-probes a transient failure // instead of no-op'ing on this (resolved) promise for the rest of the session. loading = null; @@ -46,14 +41,14 @@ function makePrerequisites() { get check() { return check; }, - // True only when a SUCCESSFUL probe confirms git + python3 + a working bundled - // filter-repo. Before the first probe resolves — and in non-Tauri — we stay optimistic - // (check null, probeFailed false) so the control doesn't flash disabled. But once a - // probe has FAILED we keep editing DISABLED: a destructive history rewrite must never - // run on unverified prerequisites. A later successful probe re-enables it. + // Commit-time editing needs a SUCCESSFUL probe confirming git + python3 + a working + // bundled filter-repo. Until one arrives, the DESKTOP app keeps the destructive "Rewrite + // history" action DISABLED — a slow OR failed probe must never leave it invokable on + // unverified prerequisites. A non-Tauri preview has nothing to verify (editing is inert + // there anyway), so it stays optimistic. A later successful probe enables it. get canEditHistory() { if (check) return check.git && check.python3 && check.filterRepo; - return !probeFailed; + return !isTauri(); }, // Probe once, then share the result — deduped so the banner and ApplyPanel don't // double-invoke.