From ef1ffaba2cdda44367904572f67241319fc460bd Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 12:07:17 +0100 Subject: [PATCH 01/10] fix(auth): treat transient code-access checks as indeterminate, not denied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `updateCodeAccessFromSession` previously collapsed every non-success path into `hasCodeAccess: false`: a transient throw (network blip, timeout) and a non-`true` body (e.g. `{}` from a 401) both denied access. Because this runs on every token refresh, resume, and reconnect via `syncAuthenticatedSession`, a single transient failure overwrote a previously-valid `true` and stranded a valid user on the invite screen until restart. Introduce a discriminated `CodeAccessOutcome` (`resolved` vs `indeterminate`) and a `checkCodeAccess` helper: - Only a clean 2xx response carrying an explicit boolean `has_access` is authoritative and may set `hasCodeAccess`. A genuine `{ has_access: false }` still correctly shows the invite screen. - Offline status, network errors, timeouts, non-2xx responses (including 401/403), and unparseable/incomplete bodies are indeterminate and preserve the prior value — a confirmed `true` stays `true`, a pending `null` stays `null` — letting the next sync re-check. - Transient failures self-heal within the sync via a bounded retry (CODE_ACCESS_MAX_ATTEMPTS = 3) using the existing sleepWithBackoff + REFRESH_BACKOFF, with a fast exit when offline. The freshly synced token is used directly rather than routing through authenticatedFetch, because this runs inside the bootstrap/refresh flow where re-entering the token-refresh machinery would deadlock. Adds a `code access resilience` test block (5 cases) covering explicit denial, transient network/401 failures preserving `true`, and inconclusive restores staying `null`. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/auth/auth.test.ts | 128 ++++++++++++++++++++++++++++ packages/core/src/auth/auth.ts | 104 ++++++++++++++++++---- 2 files changed, 217 insertions(+), 15 deletions(-) diff --git a/packages/core/src/auth/auth.test.ts b/packages/core/src/auth/auth.test.ts index 1b9448e04..d0909741f 100644 --- a/packages/core/src/auth/auth.test.ts +++ b/packages/core/src/auth/auth.test.ts @@ -1342,4 +1342,132 @@ describe("AuthService", () => { expect(redeemCallCount).toBe(2); }); }); + + describe("code access resilience", () => { + // A fetch stub whose user/org endpoints always succeed, so only the + // check-access endpoint's behaviour varies between tests. + const authFetchWithCheck = (checkAccess: () => Response) => + vi.fn(async (input: string | Request) => { + const url = typeof input === "string" ? input : input.url; + + if (url.includes("/api/users/@me/")) { + return { + ok: true, + json: vi + .fn() + .mockResolvedValue({ uuid: "user-1", organization: { id: "org-1" } }), + } as unknown as Response; + } + + if (/\/api\/organizations\/([^/]+)\/$/.test(url)) { + return { + ok: true, + json: vi + .fn() + .mockResolvedValue({ name: "Org 1", teams: [{ id: 42, name: "Project 42" }] }), + } as unknown as Response; + } + + return checkAccess(); + }) as unknown as typeof fetch; + + const okBody = (body: unknown): Response => + ({ ok: true, json: vi.fn().mockResolvedValue(body) }) as unknown as Response; + + beforeEach(() => { + seedStoredSession(); + oauthFlow.refreshToken.mockResolvedValue( + mockTokenResponse({ + accessToken: "access-token", + refreshToken: "rotated-refresh-token", + }), + ); + }); + + it("denies access when the server explicitly reports no access", async () => { + vi.stubGlobal( + "fetch", + authFetchWithCheck(() => okBody({ has_access: false })), + ); + + await service.initialize(); + + expect(service.getState()).toMatchObject({ + status: "authenticated", + hasCodeAccess: false, + }); + }); + + it("keeps a confirmed grant when a later check fails with a network error", async () => { + let failCheck = false; + vi.stubGlobal( + "fetch", + authFetchWithCheck(() => { + if (failCheck) throw new Error("network down"); + return okBody({ has_access: true }); + }), + ); + + await service.initialize(); + expect(service.getState().hasCodeAccess).toBe(true); + + failCheck = true; + await service.refreshAccessToken(); + + // A transient blip must not be mistaken for a revoked invite. + expect(service.getState().hasCodeAccess).toBe(true); + }); + + it("keeps a confirmed grant when a later check returns 401", async () => { + let unauthorized = false; + vi.stubGlobal( + "fetch", + authFetchWithCheck(() => + unauthorized + ? ({ + ok: false, + status: 401, + json: vi.fn().mockResolvedValue({}), + } as unknown as Response) + : okBody({ has_access: true }), + ), + ); + + await service.initialize(); + expect(service.getState().hasCodeAccess).toBe(true); + + unauthorized = true; + await service.refreshAccessToken(); + + expect(service.getState().hasCodeAccess).toBe(true); + }); + + it("stays indeterminate (never auto-denies) when the check is inconclusive", async () => { + vi.stubGlobal( + "fetch", + authFetchWithCheck(() => { + throw new Error("network down"); + }), + ); + + await service.initialize(); + + const state = service.getState(); + expect(state.status).toBe("authenticated"); + // null, not false: the UI shows "Checking access…" rather than the + // invite screen, and the next sync re-checks. + expect(state.hasCodeAccess).toBeNull(); + }); + + it("ignores a 2xx response without an explicit has_access flag", async () => { + vi.stubGlobal( + "fetch", + authFetchWithCheck(() => okBody({})), + ); + + await service.initialize(); + + expect(service.getState().hasCodeAccess).toBeNull(); + }); + }); }); diff --git a/packages/core/src/auth/auth.ts b/packages/core/src/auth/auth.ts index b7b47b424..0985f99df 100644 --- a/packages/core/src/auth/auth.ts +++ b/packages/core/src/auth/auth.ts @@ -71,6 +71,13 @@ interface TokenResponseOptions { selectedProjectId: number | null; } +// A Code invite access check is only authoritative when the server gives a +// clear yes/no. Anything else (offline, network error, timeout, non-2xx, or an +// unparseable body) is "indeterminate" and must not be mistaken for a denial. +type CodeAccessOutcome = + | { kind: "resolved"; hasAccess: boolean } + | { kind: "indeterminate" }; + @injectable() export class AuthService extends TypedEventEmitter { private state: AuthState = { @@ -910,26 +917,93 @@ export class AuthService extends TypedEventEmitter { return; } - try { - const apiHost = getCloudUrlFromRegion(this.session.cloudRegion); - const response = await this.executeAuthenticatedFetch( - fetch, - `${apiHost}/api/code/invites/check-access/`, - {}, - this.session.accessToken, - ); - const data = (await response.json().catch(() => ({}))) as { - has_access?: boolean; - }; + const outcome = await this.checkCodeAccess(this.session); - this.updateState({ hasCodeAccess: data.has_access === true }); - } catch (error) { - this.logger.warn("Failed to update code access state", { error }); - this.updateState({ hasCodeAccess: false }); + if (outcome.kind === "resolved") { + this.updateState({ hasCodeAccess: outcome.hasAccess }); + return; + } + + // Indeterminate: a network blip, timeout, or transient/unauthorized + // response is not proof that the invite was revoked, so it must not clobber + // a known value and strand the user on the invite screen. Keep whatever we + // already know — a confirmed `true` stays `true`; a still-unknown `null` + // stays `null` (the UI keeps showing "Checking access…") — and let the next + // session sync re-check. + this.logger.warn( + "Code access check was inconclusive; keeping previous value", + { hasCodeAccess: this.state.hasCodeAccess }, + ); + } + /** + * Resolves Code invite access for the given session. + * + * Only a successful (2xx) response carrying an explicit boolean `has_access` + * is authoritative. Offline status, network errors, timeouts, non-2xx + * responses (including a 401/403 from an unexpectedly rejected token), and + * unparseable or incomplete bodies are all treated as indeterminate and + * retried with backoff, so a transient failure never masquerades as a denied + * invite. The freshly synced access token is used directly rather than + * routing through `authenticatedFetch`, because this runs inside the + * bootstrap/refresh flow where re-entering the token-refresh machinery would + * deadlock. + */ + private async checkCodeAccess( + session: InMemorySession, + ): Promise { + const url = `${getCloudUrlFromRegion(session.cloudRegion)}/api/code/invites/check-access/`; + + for ( + let attempt = 0; + attempt < AuthService.CODE_ACCESS_MAX_ATTEMPTS; + attempt++ + ) { + if (!this.connectivity.getStatus().isOnline) { + return { kind: "indeterminate" }; + } + + try { + const response = await this.executeAuthenticatedFetch( + fetch, + url, + {}, + session.accessToken, + ); + + if (response.ok) { + const data = (await response.json().catch(() => null)) as { + has_access?: unknown; + } | null; + if (data && typeof data.has_access === "boolean") { + return { kind: "resolved", hasAccess: data.has_access }; + } + this.logger.warn("Code access response missing has_access flag", { + status: response.status, + }); + } else { + this.logger.warn("Code access check returned non-OK status", { + status: response.status, + }); + } + } catch (error) { + this.logger.warn("Code access check request failed", { + error, + attempt, + }); + } + + const isLastAttempt = + attempt === AuthService.CODE_ACCESS_MAX_ATTEMPTS - 1; + if (!isLastAttempt) { + await sleepWithBackoff(attempt, AuthService.REFRESH_BACKOFF); + } } + + return { kind: "indeterminate" }; } private static readonly REFRESH_MAX_ATTEMPTS = 3; private static readonly ORG_FETCH_MAX_ATTEMPTS = 3; + private static readonly CODE_ACCESS_MAX_ATTEMPTS = 3; private static readonly ORG_RECOVERY_MAX_ATTEMPTS = 5; private static readonly REFRESH_BACKOFF: BackoffOptions = { initialDelayMs: 1_000, From a0e9e8978596dee1239d0caa3364da21290aa59a Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 12:31:17 +0100 Subject: [PATCH 02/10] =?UTF-8?q?feat(connectivity):=20graceful=20offline?= =?UTF-8?q?=20UX=20=E2=80=94=20banner,=20action=20gating,=20auto-recovery?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on the code-access resilience fix to make intermittent connectivity graceful across the app instead of letting individual network actions fail opaquely. Three coordinated pieces, leaning on the existing server-side connectivity detection (which already debounces with a 2-failure hysteresis): 1. Connectivity banner (`ConnectivityBanner`, mounted in the shell `__root` for the main, channels, and settings layouts). While offline it explains that network actions are paused and offers a manual Retry that forces an immediate reachability probe via a new `ConnectivityClient.checkNow()` (wired to the existing `connectivity.checkNow` route); on recovery it briefly confirms "Back online" then collapses. Modeled on `UpdateBanner`. 2. Consistent gating of network-requiring git actions, reusing the existing "No internet connection" idiom: - `computeGitInteractionState` gains an `isOnline` input and returns the offline reason for push/sync/publish and create-PR, which propagates to the primary button and both dropdown variants automatically. Local actions (commit, new branch) stay enabled offline. - `usePrActions` (PR lifecycle: merge/close/draft/ready) and `useInboxCloudTaskRunner` (inbox Create PR / Discuss, scout Discuss) gain an imperative offline guard that toasts instead of firing a doomed call. 3. Auto-recovery on reconnect (`NetworkReconnectContribution` + `SessionService.recoverAfterReconnect()`). On an offline→online transition it retries errored cloud streams and flushes stranded cloud message queues — the same recovery the window-focus and auth-restored paths already perform, for the network-reconnect event that previously had no trigger. Local sessions already recover via `reconcileLocalConnection` re-firing on the `isOnline` flip, so they need no extra wiring. Tests: offline cases in `gitInteractionLogic.test.ts` (push/create-pr gated, commit/branch still allowed) and a new `network-reconnect.contribution.test.ts` (recovers only on offline→online, once per cycle). Core + UI + apps/code typecheck, biome, and the touched test suites all pass. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- apps/code/src/renderer/di/container.ts | 1 + .../gitInteractionLogic.test.ts | 54 +++++++ .../git-interaction/gitInteractionLogic.ts | 7 + packages/core/src/sessions/sessionService.ts | 13 ++ .../connectivity/ConnectivityBanner.tsx | 133 ++++++++++++++++++ .../connectivity/connectivity.module.ts | 2 + .../connectivity/connectivityClient.ts | 3 + .../network-reconnect.contribution.test.ts | 58 ++++++++ .../network-reconnect.contribution.ts | 39 +++++ .../git-interaction/useGitInteraction.ts | 4 + .../features/git-interaction/usePrActions.ts | 7 + .../inbox/hooks/useInboxCloudTaskRunner.ts | 9 ++ packages/ui/src/router/routes/__root.tsx | 4 + 13 files changed, 334 insertions(+) create mode 100644 packages/ui/src/features/connectivity/ConnectivityBanner.tsx create mode 100644 packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts create mode 100644 packages/ui/src/features/connectivity/network-reconnect.contribution.ts diff --git a/apps/code/src/renderer/di/container.ts b/apps/code/src/renderer/di/container.ts index 705ada1eb..8d7854cbe 100644 --- a/apps/code/src/renderer/di/container.ts +++ b/apps/code/src/renderer/di/container.ts @@ -160,6 +160,7 @@ container.bind(UPDATES_CLIENT).toConstantValue(updatesClient); // connectivity client — passthrough over the renderer host client const connectivityClient: ConnectivityClient = { getStatus: () => trpcClient.connectivity.getStatus.query(), + checkNow: () => trpcClient.connectivity.checkNow.mutate(), onStatusChange: (sub) => trpcClient.connectivity.onStatusChange.subscribe(undefined, sub), }; diff --git a/packages/core/src/git-interaction/gitInteractionLogic.test.ts b/packages/core/src/git-interaction/gitInteractionLogic.test.ts index 9e6db75a0..426002209 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.test.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.test.ts @@ -19,6 +19,7 @@ function makeState(overrides: Partial = {}): GitState { ghStatus: { installed: true, authenticated: true }, repoInfo: { owner: "test", repo: "test" }, prStatus: null, + isOnline: true, ...overrides, }; } @@ -249,4 +250,57 @@ describe("computeGitInteractionState", () => { expect(result.primaryAction.enabled).toBe(false); }); }); + + describe("offline", () => { + it("disables push with a no-internet reason on a feature branch", () => { + const result = computeGitInteractionState( + makeState({ + currentBranch: "feature/test", + hasChanges: false, + aheadOfRemote: 2, + isOnline: false, + }), + ); + const push = result.actions.find((a) => a.id === "push"); + expect(push?.enabled).toBe(false); + expect(push?.disabledReason).toBe("No internet connection"); + expect(result.pushDisabledReason).toBe("No internet connection"); + }); + + it("disables create-pr with a no-internet reason", () => { + const result = computeGitInteractionState( + makeState({ + currentBranch: "feature/test", + hasChanges: true, + isOnline: false, + }), + ); + const createPr = result.actions.find((a) => a.id === "create-pr"); + // create-pr is dropped from the action list once disabled, so assert via + // the primary fallback instead: offline pushes the primary off create-pr. + expect(createPr).toBeUndefined(); + expect(result.primaryAction.id).not.toBe("create-pr"); + }); + + it("still allows local commit while offline", () => { + const result = computeGitInteractionState( + makeState({ + currentBranch: "feature/test", + hasChanges: true, + isOnline: false, + }), + ); + const commit = result.actions.find((a) => a.id === "commit"); + expect(commit?.enabled).toBe(true); + expect(commit?.disabledReason).toBeNull(); + }); + + it("still allows creating a branch while offline", () => { + const result = computeGitInteractionState( + makeState({ currentBranch: null, isOnline: false }), + ); + expect(result.primaryAction.id).toBe("branch-here"); + expect(result.primaryAction.enabled).toBe(true); + }); + }); }); diff --git a/packages/core/src/git-interaction/gitInteractionLogic.ts b/packages/core/src/git-interaction/gitInteractionLogic.ts index 725ae2a7c..944b2bcad 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.ts @@ -20,8 +20,13 @@ interface GitState { headBranch: string | null; prUrl: string | null; } | null; + /** Network reachability. Remote actions (push/sync/publish, create PR) are + * gated on this; local actions (commit, new branch) are not. */ + isOnline: boolean; } +const OFFLINE_REASON = "No internet connection"; + interface GitComputed { actions: GitMenuAction[]; primaryAction: GitMenuAction; @@ -74,6 +79,7 @@ function getPushDisabledReason( opts?: { assumeWillHaveCommits?: boolean }, ): string | null { if (repoReason) return repoReason; + if (!s.isOnline) return OFFLINE_REASON; if (s.behind > 0) { return "Sync branch with remote first."; @@ -96,6 +102,7 @@ function getCreatePrDisabledReason( repoReason: string | null, ): string | null { if (repoReason) return repoReason; + if (!s.isOnline) return OFFLINE_REASON; if (!s.ghStatus) return "Checking GitHub CLI status..."; if (!s.ghStatus.installed) return "Install GitHub CLI: `brew install gh`"; diff --git a/packages/core/src/sessions/sessionService.ts b/packages/core/src/sessions/sessionService.ts index 28993a584..c95f0ca3f 100644 --- a/packages/core/src/sessions/sessionService.ts +++ b/packages/core/src/sessions/sessionService.ts @@ -3872,6 +3872,19 @@ export class SessionService { } } + /** + * Recovers sessions after the network comes back online. Retries cloud + * streams that exhausted their SSE reconnect budget and flushes cloud message + * queues that stranded while offline — the same two recovery steps the + * window-focus and auth-restored paths already perform, now also driven by the + * connectivity transition. Local sessions need no action here: they reconnect + * on their own when `reconcileLocalConnection` re-fires as `isOnline` flips. + */ + public recoverAfterReconnect(): void { + this.retryUnhealthyCloudSessions(); + this.flushQueuedCloudMessagesAfterAuthRestored(); + } + public flushQueuedCloudMessagesAfterAuthRestored(): void { const sessions = this.d.store.getSessions(); for (const session of Object.values(sessions)) { diff --git a/packages/ui/src/features/connectivity/ConnectivityBanner.tsx b/packages/ui/src/features/connectivity/ConnectivityBanner.tsx new file mode 100644 index 000000000..46b975a78 --- /dev/null +++ b/packages/ui/src/features/connectivity/ConnectivityBanner.tsx @@ -0,0 +1,133 @@ +import { ArrowsClockwise, WifiHigh, WifiSlash } from "@phosphor-icons/react"; +import { useService } from "@posthog/di/react"; +import { useConnectivity } from "@posthog/ui/hooks/useConnectivity"; +import { Box } from "@radix-ui/themes"; +import { AnimatePresence, motion } from "framer-motion"; +import { useEffect, useRef, useState } from "react"; +import { + CONNECTIVITY_CLIENT, + type ConnectivityClient, +} from "./connectivityClient"; + +/** How long the green "Back online" confirmation lingers before collapsing. */ +const BACK_ONLINE_VISIBLE_MS = 2_500; + +/** + * Slim shell banner that surfaces the global connectivity state. While offline + * it explains that network actions are paused and offers a manual Retry (which + * forces an immediate reachability probe); on recovery it briefly confirms + * "Back online" then collapses. The underlying detection is server-side and + * already debounced (it only reports offline after repeated probe failures), so + * this renders the settled state without re-debouncing. + */ +export function ConnectivityBanner() { + const { isOnline } = useConnectivity(); + const client = useService(CONNECTIVITY_CLIENT); + const [isChecking, setIsChecking] = useState(false); + const [showBackOnline, setShowBackOnline] = useState(false); + const wasOnlineRef = useRef(isOnline); + + useEffect(() => { + const wasOnline = wasOnlineRef.current; + wasOnlineRef.current = isOnline; + + if (!wasOnline && isOnline) { + setIsChecking(false); + setShowBackOnline(true); + const timer = setTimeout( + () => setShowBackOnline(false), + BACK_ONLINE_VISIBLE_MS, + ); + return () => clearTimeout(timer); + } + + if (!isOnline) { + // A fresh outage supersedes any lingering "back online" confirmation. + setShowBackOnline(false); + } + + return undefined; + }, [isOnline]); + + const handleRetry = () => { + if (isChecking) return; + setIsChecking(true); + void client + .checkNow() + .catch(() => undefined) + .finally(() => setIsChecking(false)); + }; + + const isVisible = !isOnline || showBackOnline; + + return ( + + {isVisible && ( + + + {isOnline ? ( + + ) : ( + + )} + + + )} + + ); +} + +function OfflineRow({ + isChecking, + onRetry, +}: { + isChecking: boolean; + onRetry: () => void; +}) { + return ( +
+ +
+ You're offline + + {isChecking + ? "Checking connection…" + : "Network actions are paused — reconnecting automatically."} + +
+ +
+ ); +} + +function BackOnlineRow() { + return ( + + + Back online + + ); +} diff --git a/packages/ui/src/features/connectivity/connectivity.module.ts b/packages/ui/src/features/connectivity/connectivity.module.ts index 7be5f45c9..0b8fa40b7 100644 --- a/packages/ui/src/features/connectivity/connectivity.module.ts +++ b/packages/ui/src/features/connectivity/connectivity.module.ts @@ -1,7 +1,9 @@ import { CONTRIBUTION } from "@posthog/di/contribution"; import { ContainerModule } from "inversify"; import { ConnectivityEventsContribution } from "./connectivity-events.contribution"; +import { NetworkReconnectContribution } from "./network-reconnect.contribution"; export const connectivityUiModule = new ContainerModule(({ bind }) => { bind(CONTRIBUTION).to(ConnectivityEventsContribution).inSingletonScope(); + bind(CONTRIBUTION).to(NetworkReconnectContribution).inSingletonScope(); }); diff --git a/packages/ui/src/features/connectivity/connectivityClient.ts b/packages/ui/src/features/connectivity/connectivityClient.ts index 239d513d5..dd97ace57 100644 --- a/packages/ui/src/features/connectivity/connectivityClient.ts +++ b/packages/ui/src/features/connectivity/connectivityClient.ts @@ -9,6 +9,9 @@ export interface ConnectivityStatusPayload { export interface ConnectivityClient { getStatus(): Promise; + /** Forces an immediate reachability probe, bypassing the poll interval, and + * resolves with the freshly measured status. Backs the banner's Retry. */ + checkNow(): Promise; onStatusChange(sub: Subscriber): { unsubscribe: () => void; }; diff --git a/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts b/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts new file mode 100644 index 000000000..13782b735 --- /dev/null +++ b/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts @@ -0,0 +1,58 @@ +import { connectivityStore } from "@posthog/core/connectivity/connectivityStore"; +import type { SessionService } from "@posthog/core/sessions/sessionService"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { NetworkReconnectContribution } from "./network-reconnect.contribution"; + +function makeSessionService() { + return { recoverAfterReconnect: vi.fn() } as unknown as SessionService; +} + +function setOnline(isOnline: boolean) { + connectivityStore.getState().setOnline(isOnline); +} + +describe("NetworkReconnectContribution", () => { + beforeEach(() => { + // The store is a process-wide singleton shared with the contribution; reset + // it to the default online state before each case. + setOnline(true); + }); + + it("recovers sessions on an offline -> online transition", () => { + const sessionService = makeSessionService(); + new NetworkReconnectContribution(sessionService).start(); + + setOnline(false); + expect(sessionService.recoverAfterReconnect).not.toHaveBeenCalled(); + + setOnline(true); + expect(sessionService.recoverAfterReconnect).toHaveBeenCalledTimes(1); + }); + + it("does not recover when going online -> offline", () => { + const sessionService = makeSessionService(); + new NetworkReconnectContribution(sessionService).start(); + + setOnline(false); + expect(sessionService.recoverAfterReconnect).not.toHaveBeenCalled(); + }); + + it("does not recover on a redundant online update", () => { + const sessionService = makeSessionService(); + new NetworkReconnectContribution(sessionService).start(); + + setOnline(true); + expect(sessionService.recoverAfterReconnect).not.toHaveBeenCalled(); + }); + + it("recovers again on each offline -> online cycle", () => { + const sessionService = makeSessionService(); + new NetworkReconnectContribution(sessionService).start(); + + setOnline(false); + setOnline(true); + setOnline(false); + setOnline(true); + expect(sessionService.recoverAfterReconnect).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/ui/src/features/connectivity/network-reconnect.contribution.ts b/packages/ui/src/features/connectivity/network-reconnect.contribution.ts new file mode 100644 index 000000000..499b8c3ae --- /dev/null +++ b/packages/ui/src/features/connectivity/network-reconnect.contribution.ts @@ -0,0 +1,39 @@ +import { connectivityStore } from "@posthog/core/connectivity/connectivityStore"; +import { + SESSION_SERVICE, + type SessionService, +} from "@posthog/core/sessions/sessionService"; +import type { Contribution } from "@posthog/di/contribution"; +import { inject, injectable } from "inversify"; + +/** + * Drives session recovery off the network-reconnect event. Subscribes to the + * connectivity store and, on an offline→online transition, asks the session + * service to retry errored cloud streams and flush cloud message queues that + * stranded while offline. + * + * This closes the one recovery gap the existing triggers left open: window + * focus (`GlobalEventHandlers`) and auth-restored (`AuthContribution`) already + * call into the same recovery, but a network blip that resolves while the app + * stays focused and authenticated fired neither. Local sessions are unaffected + * — they reconnect on their own once `reconcileLocalConnection` re-runs as + * `isOnline` flips back. + */ +@injectable() +export class NetworkReconnectContribution implements Contribution { + constructor( + @inject(SESSION_SERVICE) + private readonly sessionService: SessionService, + ) {} + + start(): void { + let wasOnline = connectivityStore.getState().isOnline; + connectivityStore.subscribe((state) => { + const justCameOnline = !wasOnline && state.isOnline; + wasOnline = state.isOnline; + if (justCameOnline) { + this.sessionService.recoverAfterReconnect(); + } + }); + } +} diff --git a/packages/ui/src/features/git-interaction/useGitInteraction.ts b/packages/ui/src/features/git-interaction/useGitInteraction.ts index 4bae9fb75..d54c5b3a9 100644 --- a/packages/ui/src/features/git-interaction/useGitInteraction.ts +++ b/packages/ui/src/features/git-interaction/useGitInteraction.ts @@ -11,6 +11,7 @@ import { import { useService } from "@posthog/di/react"; import { useHostTRPC } from "@posthog/host-router/react"; import type { ChangedFile } from "@posthog/shared/domain-types"; +import { useConnectivity } from "@posthog/ui/hooks/useConnectivity"; import { useQueryClient } from "@tanstack/react-query"; import { useMemo, useRef } from "react"; import { WORKSPACE_QUERY_KEY } from "../workspace/identifiers"; @@ -94,6 +95,7 @@ export function useGitInteraction( const store = useGitInteractionStore(); const { actions: modal } = store; const pushAbortRef = useRef(null); + const { isOnline } = useConnectivity(); const git = useGitQueries(repoPath); @@ -114,6 +116,7 @@ export function useGitInteraction( ghStatus: git.ghStatus ?? null, repoInfo: git.repoInfo ?? null, prStatus: git.prStatus ?? null, + isOnline, }), [ repoPath, @@ -130,6 +133,7 @@ export function useGitInteraction( git.ghStatus, git.repoInfo, git.prStatus, + isOnline, ], ); diff --git a/packages/ui/src/features/git-interaction/usePrActions.ts b/packages/ui/src/features/git-interaction/usePrActions.ts index bcb9fbab6..4d288e8a2 100644 --- a/packages/ui/src/features/git-interaction/usePrActions.ts +++ b/packages/ui/src/features/git-interaction/usePrActions.ts @@ -4,12 +4,15 @@ import { } from "@posthog/core/git-interaction/prStatus"; import { useHostTRPC } from "@posthog/host-router/react"; import type { PrActionType } from "@posthog/shared"; +import { showOfflineToast } from "@posthog/ui/features/connectivity/connectivityToast"; +import { useConnectivity } from "@posthog/ui/hooks/useConnectivity"; import { useMutation, useQueryClient } from "@tanstack/react-query"; import { toast } from "../../primitives/toast"; export function usePrActions(prUrl: string | null) { const trpc = useHostTRPC(); const queryClient = useQueryClient(); + const { isOnline } = useConnectivity(); const mutation = useMutation({ ...trpc.git.updatePrByUrl.mutationOptions(), @@ -41,6 +44,10 @@ export function usePrActions(prUrl: string | null) { return { execute: (action: PrActionType) => { if (!prUrl) return; + if (!isOnline) { + showOfflineToast(); + return; + } mutation.mutate({ prUrl, action }); }, isPending: mutation.isPending, diff --git a/packages/ui/src/features/inbox/hooks/useInboxCloudTaskRunner.ts b/packages/ui/src/features/inbox/hooks/useInboxCloudTaskRunner.ts index 4e4a766fe..ce2cb2ee4 100644 --- a/packages/ui/src/features/inbox/hooks/useInboxCloudTaskRunner.ts +++ b/packages/ui/src/features/inbox/hooks/useInboxCloudTaskRunner.ts @@ -11,10 +11,12 @@ import { import { useService } from "@posthog/di/react"; import { ANALYTICS_EVENTS, getCloudUrlFromRegion } from "@posthog/shared"; import { useAuthStateValue } from "@posthog/ui/features/auth/store"; +import { showOfflineToast } from "@posthog/ui/features/connectivity/connectivityToast"; import { resolveDefaultModel } from "@posthog/ui/features/inbox/hooks/resolveDefaultModel"; import { useUserRepositoryIntegration } from "@posthog/ui/features/integrations/useIntegrations"; import { useSettingsStore } from "@posthog/ui/features/settings/settingsStore"; import { useCreateTask } from "@posthog/ui/features/tasks/useTaskCrudMutations"; +import { useConnectivity } from "@posthog/ui/hooks/useConnectivity"; import { toast } from "@posthog/ui/primitives/toast"; import { openTask } from "@posthog/ui/router/useOpenTask"; import { track } from "@posthog/ui/shell/analytics"; @@ -106,11 +108,17 @@ export function useInboxCloudTaskRunner({ const modelResolver = useService(REPORT_MODEL_RESOLVER); const cloudRegion = useAuthStateValue((state) => state.cloudRegion); const queryClient = useQueryClient(); + const { isOnline } = useConnectivity(); const run = useCallback(async () => { if (isRunning) return; const log = logger.scope(loggerScope); + if (!isOnline) { + showOfflineToast(); + return; + } + if (!cloudRepository) { toast.error(copy.errorTitle, { description: copy.missingRepository }); return; @@ -244,6 +252,7 @@ export function useInboxCloudTaskRunner({ } }, [ isRunning, + isOnline, loggerScope, cloudRepository, cloudRegion, diff --git a/packages/ui/src/router/routes/__root.tsx b/packages/ui/src/router/routes/__root.tsx index 1a3dcb555..e50faf37f 100644 --- a/packages/ui/src/router/routes/__root.tsx +++ b/packages/ui/src/router/routes/__root.tsx @@ -23,6 +23,7 @@ import { } from "@posthog/ui/features/canvas/components/FeedbackModal"; import { CommandMenu } from "@posthog/ui/features/command/CommandMenu"; import { KeyboardShortcutsSheet } from "@posthog/ui/features/command/KeyboardShortcutsSheet"; +import { ConnectivityBanner } from "@posthog/ui/features/connectivity/ConnectivityBanner"; import { useNewTaskDeepLink } from "@posthog/ui/features/deep-links/useNewTaskDeepLink"; import { useTaskDeepLink } from "@posthog/ui/features/deep-links/useTaskDeepLink"; import { useFeatureFlag } from "@posthog/ui/features/feature-flags/useFeatureFlag"; @@ -347,6 +348,7 @@ function RootLayout() { + {/* Content sits in a bordered, rounded card inset from the window @@ -385,6 +387,7 @@ function RootLayout() { if (isSettingsRoute) { return ( + + From fe728a0b86a5143bc4a3fd9e49368598d88afdf7 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 12:51:06 +0100 Subject: [PATCH 03/10] style(auth): apply Biome formatting to code-access test helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `quality` CI check (`biome ci .`) flagged the `code access resilience` test helpers (`authFetchWithCheck` mock bodies and the `okBody` helper) as unformatted — they were committed after a `biome lint` (which skips formatting) rather than a full `biome check`. Whitespace only; no behavior change. All 41 auth tests still pass. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/auth/auth.test.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/core/src/auth/auth.test.ts b/packages/core/src/auth/auth.test.ts index d0909741f..13fd8fed5 100644 --- a/packages/core/src/auth/auth.test.ts +++ b/packages/core/src/auth/auth.test.ts @@ -1353,18 +1353,20 @@ describe("AuthService", () => { if (url.includes("/api/users/@me/")) { return { ok: true, - json: vi - .fn() - .mockResolvedValue({ uuid: "user-1", organization: { id: "org-1" } }), + json: vi.fn().mockResolvedValue({ + uuid: "user-1", + organization: { id: "org-1" }, + }), } as unknown as Response; } if (/\/api\/organizations\/([^/]+)\/$/.test(url)) { return { ok: true, - json: vi - .fn() - .mockResolvedValue({ name: "Org 1", teams: [{ id: 42, name: "Project 42" }] }), + json: vi.fn().mockResolvedValue({ + name: "Org 1", + teams: [{ id: 42, name: "Project 42" }], + }), } as unknown as Response; } @@ -1372,7 +1374,10 @@ describe("AuthService", () => { }) as unknown as typeof fetch; const okBody = (body: unknown): Response => - ({ ok: true, json: vi.fn().mockResolvedValue(body) }) as unknown as Response; + ({ + ok: true, + json: vi.fn().mockResolvedValue(body), + }) as unknown as Response; beforeEach(() => { seedStoredSession(); From b2684d0b026b6935ad3f5283de9015eda0d4ef6d Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 12:59:45 +0100 Subject: [PATCH 04/10] refactor(git-interaction): surface createPrDisabledReason; parameterize offline tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Greptile review feedback on the offline-gating tests: - Add `createPrDisabledReason` to `GitComputed`, mirroring `pushDisabledReason`. A disabled create-pr action is dropped from `actions`, so its reason was previously unreadable by consumers and tests; the offline create-pr test could only assert the action's absence (which any disabled reason would satisfy). The named field now lets callers and tests read the actual reason. - Parameterize the four offline cases into two `it.each` tables — "remote actions gated offline" (push, create-pr) asserting the no-internet reason via the named field, and "local actions still allowed offline" (commit, new branch) — per the team's preference for parameterized tests over duplicated bodies. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- .../gitInteractionLogic.test.ts | 89 ++++++++++--------- .../git-interaction/gitInteractionLogic.ts | 7 ++ 2 files changed, 54 insertions(+), 42 deletions(-) diff --git a/packages/core/src/git-interaction/gitInteractionLogic.test.ts b/packages/core/src/git-interaction/gitInteractionLogic.test.ts index 426002209..0d5d34d83 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.test.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.test.ts @@ -252,55 +252,60 @@ describe("computeGitInteractionState", () => { }); describe("offline", () => { - it("disables push with a no-internet reason on a feature branch", () => { - const result = computeGitInteractionState( - makeState({ + // Remote actions can't run offline: each surfaces the no-internet reason + // via its named GitComputed field (a disabled create-pr is dropped from + // `actions`, so its reason is only readable here). + it.each([ + { + action: "push", + field: "pushDisabledReason" as const, + overrides: { currentBranch: "feature/test", hasChanges: false, aheadOfRemote: 2, - isOnline: false, - }), - ); - const push = result.actions.find((a) => a.id === "push"); - expect(push?.enabled).toBe(false); - expect(push?.disabledReason).toBe("No internet connection"); - expect(result.pushDisabledReason).toBe("No internet connection"); - }); - - it("disables create-pr with a no-internet reason", () => { - const result = computeGitInteractionState( - makeState({ + } satisfies Partial, + }, + { + action: "create-pr", + field: "createPrDisabledReason" as const, + overrides: { currentBranch: "feature/test", hasChanges: true, - isOnline: false, - }), - ); - const createPr = result.actions.find((a) => a.id === "create-pr"); - // create-pr is dropped from the action list once disabled, so assert via - // the primary fallback instead: offline pushes the primary off create-pr. - expect(createPr).toBeUndefined(); - expect(result.primaryAction.id).not.toBe("create-pr"); - }); + } satisfies Partial, + }, + ])( + "gates $action with a no-internet reason while offline", + ({ field, overrides }) => { + const result = computeGitInteractionState( + makeState({ ...overrides, isOnline: false }), + ); + expect(result[field]).toBe("No internet connection"); + }, + ); - it("still allows local commit while offline", () => { - const result = computeGitInteractionState( - makeState({ + // Local actions don't touch the network, so they stay enabled offline. + it.each([ + { + action: "commit", + overrides: { currentBranch: "feature/test", hasChanges: true, - isOnline: false, - }), - ); - const commit = result.actions.find((a) => a.id === "commit"); - expect(commit?.enabled).toBe(true); - expect(commit?.disabledReason).toBeNull(); - }); - - it("still allows creating a branch while offline", () => { - const result = computeGitInteractionState( - makeState({ currentBranch: null, isOnline: false }), - ); - expect(result.primaryAction.id).toBe("branch-here"); - expect(result.primaryAction.enabled).toBe(true); - }); + } satisfies Partial, + }, + { + action: "branch-here", + overrides: { currentBranch: null } satisfies Partial, + }, + ])( + "still allows the local $action action while offline", + ({ action, overrides }) => { + const result = computeGitInteractionState( + makeState({ ...overrides, isOnline: false }), + ); + const found = result.actions.find((a) => a.id === action); + expect(found?.enabled).toBe(true); + expect(found?.disabledReason).toBeNull(); + }, + ); }); }); diff --git a/packages/core/src/git-interaction/gitInteractionLogic.ts b/packages/core/src/git-interaction/gitInteractionLogic.ts index 944b2bcad..5f51aee47 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.ts @@ -31,6 +31,10 @@ interface GitComputed { actions: GitMenuAction[]; primaryAction: GitMenuAction; pushDisabledReason: string | null; + // Surfaced as a named field (like pushDisabledReason) because a disabled + // create-pr action is dropped from `actions`, so its reason would otherwise + // be unreadable by consumers and tests. + createPrDisabledReason: string | null; prBaseBranch: string | null; prHeadBranch: string | null; prUrl: string | null; @@ -179,6 +183,7 @@ export function computeGitInteractionState(input: GitState): GitComputed { actions: [branchAction], primaryAction: branchAction, pushDisabledReason: "Create a branch first.", + createPrDisabledReason: "Create a branch first.", prBaseBranch: input.defaultBranch, prHeadBranch: null, prUrl: null, @@ -207,6 +212,7 @@ export function computeGitInteractionState(input: GitState): GitComputed { actions, primaryAction, pushDisabledReason: "Create a feature branch first.", + createPrDisabledReason, prBaseBranch: input.defaultBranch, prHeadBranch: input.currentBranch, prUrl: input.prStatus?.prUrl ?? null, @@ -238,6 +244,7 @@ export function computeGitInteractionState(input: GitState): GitComputed { pushDisabledReason: getPushDisabledReason(input, repoReason, { assumeWillHaveCommits: true, }), + createPrDisabledReason, prBaseBranch: input.prStatus?.baseBranch ?? input.defaultBranch, prHeadBranch: input.prStatus?.headBranch ?? input.currentBranch, prUrl: input.prStatus?.prUrl ?? null, From 2fc2cd4cb68030344e604871f31f198310a18439 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 13:17:07 +0100 Subject: [PATCH 05/10] docs(connectivity): trim verbose comments Condense the over-long comments/docstrings added with the connectivity feature (banner, reconnect contribution, recoverAfterReconnect, createPrDisabledReason, offline tests) to keep the "why" without the prose. No behavior change. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- .../git-interaction/gitInteractionLogic.test.ts | 6 ++---- .../src/git-interaction/gitInteractionLogic.ts | 9 ++++----- packages/core/src/sessions/sessionService.ts | 9 +++------ .../features/connectivity/ConnectivityBanner.tsx | 10 ++-------- .../features/connectivity/connectivityClient.ts | 3 +-- .../network-reconnect.contribution.ts | 15 ++++----------- 6 files changed, 16 insertions(+), 36 deletions(-) diff --git a/packages/core/src/git-interaction/gitInteractionLogic.test.ts b/packages/core/src/git-interaction/gitInteractionLogic.test.ts index 0d5d34d83..b0232e8d8 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.test.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.test.ts @@ -252,9 +252,8 @@ describe("computeGitInteractionState", () => { }); describe("offline", () => { - // Remote actions can't run offline: each surfaces the no-internet reason - // via its named GitComputed field (a disabled create-pr is dropped from - // `actions`, so its reason is only readable here). + // create-pr is dropped from `actions` when disabled, so its reason is read + // from the named field. it.each([ { action: "push", @@ -283,7 +282,6 @@ describe("computeGitInteractionState", () => { }, ); - // Local actions don't touch the network, so they stay enabled offline. it.each([ { action: "commit", diff --git a/packages/core/src/git-interaction/gitInteractionLogic.ts b/packages/core/src/git-interaction/gitInteractionLogic.ts index 5f51aee47..fa7f2d48b 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.ts @@ -20,8 +20,8 @@ interface GitState { headBranch: string | null; prUrl: string | null; } | null; - /** Network reachability. Remote actions (push/sync/publish, create PR) are - * gated on this; local actions (commit, new branch) are not. */ + /** Remote actions (push/sync/publish, create PR) gate on this; local + * actions (commit, new branch) don't. */ isOnline: boolean; } @@ -31,9 +31,8 @@ interface GitComputed { actions: GitMenuAction[]; primaryAction: GitMenuAction; pushDisabledReason: string | null; - // Surfaced as a named field (like pushDisabledReason) because a disabled - // create-pr action is dropped from `actions`, so its reason would otherwise - // be unreadable by consumers and tests. + // Named like pushDisabledReason: a disabled create-pr is dropped from + // `actions`, so its reason is only readable here. createPrDisabledReason: string | null; prBaseBranch: string | null; prHeadBranch: string | null; diff --git a/packages/core/src/sessions/sessionService.ts b/packages/core/src/sessions/sessionService.ts index c95f0ca3f..c1ab6c1e9 100644 --- a/packages/core/src/sessions/sessionService.ts +++ b/packages/core/src/sessions/sessionService.ts @@ -3873,12 +3873,9 @@ export class SessionService { } /** - * Recovers sessions after the network comes back online. Retries cloud - * streams that exhausted their SSE reconnect budget and flushes cloud message - * queues that stranded while offline — the same two recovery steps the - * window-focus and auth-restored paths already perform, now also driven by the - * connectivity transition. Local sessions need no action here: they reconnect - * on their own when `reconcileLocalConnection` re-fires as `isOnline` flips. + * Recovers cloud sessions after reconnect: retries errored streams and + * flushes stranded queues (same steps as the window-focus and auth-restored + * paths). Local sessions recover on their own via `reconcileLocalConnection`. */ public recoverAfterReconnect(): void { this.retryUnhealthyCloudSessions(); diff --git a/packages/ui/src/features/connectivity/ConnectivityBanner.tsx b/packages/ui/src/features/connectivity/ConnectivityBanner.tsx index 46b975a78..9d3020bed 100644 --- a/packages/ui/src/features/connectivity/ConnectivityBanner.tsx +++ b/packages/ui/src/features/connectivity/ConnectivityBanner.tsx @@ -9,16 +9,11 @@ import { type ConnectivityClient, } from "./connectivityClient"; -/** How long the green "Back online" confirmation lingers before collapsing. */ const BACK_ONLINE_VISIBLE_MS = 2_500; /** - * Slim shell banner that surfaces the global connectivity state. While offline - * it explains that network actions are paused and offers a manual Retry (which - * forces an immediate reachability probe); on recovery it briefly confirms - * "Back online" then collapses. The underlying detection is server-side and - * already debounced (it only reports offline after repeated probe failures), so - * this renders the settled state without re-debouncing. + * Shell banner for the global connectivity state: while offline, offers a Retry + * that forces an immediate probe; briefly shows "Back online" on recovery. */ export function ConnectivityBanner() { const { isOnline } = useConnectivity(); @@ -42,7 +37,6 @@ export function ConnectivityBanner() { } if (!isOnline) { - // A fresh outage supersedes any lingering "back online" confirmation. setShowBackOnline(false); } diff --git a/packages/ui/src/features/connectivity/connectivityClient.ts b/packages/ui/src/features/connectivity/connectivityClient.ts index dd97ace57..fe16749b6 100644 --- a/packages/ui/src/features/connectivity/connectivityClient.ts +++ b/packages/ui/src/features/connectivity/connectivityClient.ts @@ -9,8 +9,7 @@ export interface ConnectivityStatusPayload { export interface ConnectivityClient { getStatus(): Promise; - /** Forces an immediate reachability probe, bypassing the poll interval, and - * resolves with the freshly measured status. Backs the banner's Retry. */ + /** Forces an immediate reachability probe; backs the banner's Retry. */ checkNow(): Promise; onStatusChange(sub: Subscriber): { unsubscribe: () => void; diff --git a/packages/ui/src/features/connectivity/network-reconnect.contribution.ts b/packages/ui/src/features/connectivity/network-reconnect.contribution.ts index 499b8c3ae..c7d1b0e7e 100644 --- a/packages/ui/src/features/connectivity/network-reconnect.contribution.ts +++ b/packages/ui/src/features/connectivity/network-reconnect.contribution.ts @@ -7,17 +7,10 @@ import type { Contribution } from "@posthog/di/contribution"; import { inject, injectable } from "inversify"; /** - * Drives session recovery off the network-reconnect event. Subscribes to the - * connectivity store and, on an offline→online transition, asks the session - * service to retry errored cloud streams and flush cloud message queues that - * stranded while offline. - * - * This closes the one recovery gap the existing triggers left open: window - * focus (`GlobalEventHandlers`) and auth-restored (`AuthContribution`) already - * call into the same recovery, but a network blip that resolves while the app - * stays focused and authenticated fired neither. Local sessions are unaffected - * — they reconnect on their own once `reconcileLocalConnection` re-runs as - * `isOnline` flips back. + * On an offline→online transition, asks the session service to recover cloud + * sessions. Window-focus and auth-restored already trigger the same recovery; + * this covers the reconnect event, which fired neither. Local sessions recover + * on their own via `reconcileLocalConnection`. */ @injectable() export class NetworkReconnectContribution implements Contribution { From 5189b371b6baa5dbde3be505e981abdf75defe93 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 13:18:07 +0100 Subject: [PATCH 06/10] refactor(auth): simplify code-access outcome and trim comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the single-use `CodeAccessOutcome` discriminated union with a `boolean | null` return from `checkCodeAccess` — `null` already means "indeterminate" in the store's `hasCodeAccess: boolean | null`, so the union was redundant structure. Align the retry loop with the file's other loops (`if (isLastAttempt) break;`). Condense the verbose docstrings/comments to keep the rationale (the deadlock-avoidance note, the indeterminate-preserves- prior-value note) without the prose. No behavior change; all 41 auth tests pass. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/auth/auth.ts | 51 ++++++++++++---------------------- 1 file changed, 17 insertions(+), 34 deletions(-) diff --git a/packages/core/src/auth/auth.ts b/packages/core/src/auth/auth.ts index 0985f99df..d73b0ed2e 100644 --- a/packages/core/src/auth/auth.ts +++ b/packages/core/src/auth/auth.ts @@ -71,13 +71,6 @@ interface TokenResponseOptions { selectedProjectId: number | null; } -// A Code invite access check is only authoritative when the server gives a -// clear yes/no. Anything else (offline, network error, timeout, non-2xx, or an -// unparseable body) is "indeterminate" and must not be mistaken for a denial. -type CodeAccessOutcome = - | { kind: "resolved"; hasAccess: boolean } - | { kind: "indeterminate" }; - @injectable() export class AuthService extends TypedEventEmitter { private state: AuthState = { @@ -917,40 +910,31 @@ export class AuthService extends TypedEventEmitter { return; } - const outcome = await this.checkCodeAccess(this.session); + const hasAccess = await this.checkCodeAccess(this.session); - if (outcome.kind === "resolved") { - this.updateState({ hasCodeAccess: outcome.hasAccess }); + if (hasAccess !== null) { + this.updateState({ hasCodeAccess: hasAccess }); return; } - // Indeterminate: a network blip, timeout, or transient/unauthorized - // response is not proof that the invite was revoked, so it must not clobber - // a known value and strand the user on the invite screen. Keep whatever we - // already know — a confirmed `true` stays `true`; a still-unknown `null` - // stays `null` (the UI keeps showing "Checking access…") — and let the next - // session sync re-check. + // Indeterminate: a transient/unauthorized failure isn't proof the invite + // was revoked, so keep the prior value and let the next sync re-check. this.logger.warn( "Code access check was inconclusive; keeping previous value", { hasCodeAccess: this.state.hasCodeAccess }, ); } /** - * Resolves Code invite access for the given session. - * - * Only a successful (2xx) response carrying an explicit boolean `has_access` - * is authoritative. Offline status, network errors, timeouts, non-2xx - * responses (including a 401/403 from an unexpectedly rejected token), and - * unparseable or incomplete bodies are all treated as indeterminate and - * retried with backoff, so a transient failure never masquerades as a denied - * invite. The freshly synced access token is used directly rather than - * routing through `authenticatedFetch`, because this runs inside the - * bootstrap/refresh flow where re-entering the token-refresh machinery would - * deadlock. + * Resolves Code invite access. Only a 2xx response with an explicit boolean + * `has_access` is authoritative; everything else (offline, network error, + * non-2xx, malformed body) is indeterminate, retried with backoff, then + * returned as `null` so the caller keeps the prior value. Uses the synced + * token directly rather than `authenticatedFetch`, which would re-enter the + * refresh flow this runs inside and deadlock. */ private async checkCodeAccess( session: InMemorySession, - ): Promise { + ): Promise { const url = `${getCloudUrlFromRegion(session.cloudRegion)}/api/code/invites/check-access/`; for ( @@ -959,7 +943,7 @@ export class AuthService extends TypedEventEmitter { attempt++ ) { if (!this.connectivity.getStatus().isOnline) { - return { kind: "indeterminate" }; + return null; } try { @@ -975,7 +959,7 @@ export class AuthService extends TypedEventEmitter { has_access?: unknown; } | null; if (data && typeof data.has_access === "boolean") { - return { kind: "resolved", hasAccess: data.has_access }; + return data.has_access; } this.logger.warn("Code access response missing has_access flag", { status: response.status, @@ -994,12 +978,11 @@ export class AuthService extends TypedEventEmitter { const isLastAttempt = attempt === AuthService.CODE_ACCESS_MAX_ATTEMPTS - 1; - if (!isLastAttempt) { - await sleepWithBackoff(attempt, AuthService.REFRESH_BACKOFF); - } + if (isLastAttempt) break; + await sleepWithBackoff(attempt, AuthService.REFRESH_BACKOFF); } - return { kind: "indeterminate" }; + return null; } private static readonly REFRESH_MAX_ATTEMPTS = 3; private static readonly ORG_FETCH_MAX_ATTEMPTS = 3; From 9719250d8ceb3c76191dad896e918e489ad1723d Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 13:38:53 +0100 Subject: [PATCH 07/10] test(auth): parameterize code-access cases, cover retry recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Greptile review feedback on the code-access tests: - Collapse the three single-phase cases (explicit deny, throw → indeterminate, 2xx without has_access → indeterminate) into one `it.each`, and add the previously-uncovered positive grant (has_access: true → true) as a fourth row. - Add a test for the retry loop's recovery path: attempt 1 throws, attempt 2 returns has_access: true, hasCodeAccess becomes true — guarding against a regression that short-circuits after the first failure. - Add the missing blank line between `updateCodeAccessFromSession` and the `checkCodeAccess` JSDoc. No production behavior change; 43 auth tests pass. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/auth/auth.test.ts | 64 +++++++++++++++++------------ packages/core/src/auth/auth.ts | 1 + 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/packages/core/src/auth/auth.test.ts b/packages/core/src/auth/auth.test.ts index 13fd8fed5..af2835dd2 100644 --- a/packages/core/src/auth/auth.test.ts +++ b/packages/core/src/auth/auth.test.ts @@ -1389,18 +1389,39 @@ describe("AuthService", () => { ); }); - it("denies access when the server explicitly reports no access", async () => { - vi.stubGlobal( - "fetch", - authFetchWithCheck(() => okBody({ has_access: false })), - ); + // null (not false) for an inconclusive check: the UI shows "Checking + // access…" rather than the invite screen, and the next sync re-checks. + it.each([ + { + name: "grants access when the server reports has_access true", + checkAccess: () => okBody({ has_access: true }), + expected: true, + }, + { + name: "denies access when the server explicitly reports no access", + checkAccess: () => okBody({ has_access: false }), + expected: false, + }, + { + name: "stays indeterminate when the check throws", + checkAccess: () => { + throw new Error("network down"); + }, + expected: null, + }, + { + name: "stays indeterminate on a 2xx response without a has_access flag", + checkAccess: () => okBody({}), + expected: null, + }, + ])("$name", async ({ checkAccess, expected }) => { + vi.stubGlobal("fetch", authFetchWithCheck(checkAccess)); await service.initialize(); - expect(service.getState()).toMatchObject({ - status: "authenticated", - hasCodeAccess: false, - }); + const state = service.getState(); + expect(state.status).toBe("authenticated"); + expect(state.hasCodeAccess).toBe(expected); }); it("keeps a confirmed grant when a later check fails with a network error", async () => { @@ -1447,32 +1468,21 @@ describe("AuthService", () => { expect(service.getState().hasCodeAccess).toBe(true); }); - it("stays indeterminate (never auto-denies) when the check is inconclusive", async () => { + it("recovers within the retry loop when a later attempt succeeds", async () => { + let attempts = 0; vi.stubGlobal( "fetch", authFetchWithCheck(() => { - throw new Error("network down"); + attempts += 1; + if (attempts === 1) throw new Error("transient"); + return okBody({ has_access: true }); }), ); await service.initialize(); - const state = service.getState(); - expect(state.status).toBe("authenticated"); - // null, not false: the UI shows "Checking access…" rather than the - // invite screen, and the next sync re-checks. - expect(state.hasCodeAccess).toBeNull(); - }); - - it("ignores a 2xx response without an explicit has_access flag", async () => { - vi.stubGlobal( - "fetch", - authFetchWithCheck(() => okBody({})), - ); - - await service.initialize(); - - expect(service.getState().hasCodeAccess).toBeNull(); + expect(service.getState().hasCodeAccess).toBe(true); + expect(attempts).toBeGreaterThanOrEqual(2); }); }); }); diff --git a/packages/core/src/auth/auth.ts b/packages/core/src/auth/auth.ts index d73b0ed2e..872bb06e6 100644 --- a/packages/core/src/auth/auth.ts +++ b/packages/core/src/auth/auth.ts @@ -924,6 +924,7 @@ export class AuthService extends TypedEventEmitter { { hasCodeAccess: this.state.hasCodeAccess }, ); } + /** * Resolves Code invite access. Only a 2xx response with an explicit boolean * `has_access` is authoritative; everything else (offline, network error, From ef6d83f4db146c590868e7f3e33830b2e7a5af76 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 13:55:35 +0100 Subject: [PATCH 08/10] test(auth): dedupe fetch stub and parameterize grant-preservation cases Fold the code-access tests' bespoke `authFetchWithCheck` helper into the existing `stubAuthFetch` via an optional `checkAccess` override, removing a duplicated copy of the user/org URL-routing stub. Parameterize the two "keeps a confirmed grant on a later transient failure" cases (network error, 401) into a single `it.each`. Test-only; 43 auth tests pass. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/auth/auth.test.ts | 119 ++++++++++------------------ 1 file changed, 44 insertions(+), 75 deletions(-) diff --git a/packages/core/src/auth/auth.test.ts b/packages/core/src/auth/auth.test.ts index af2835dd2..870ca218b 100644 --- a/packages/core/src/auth/auth.test.ts +++ b/packages/core/src/auth/auth.test.ts @@ -151,6 +151,9 @@ describe("AuthService", () => { string, { name: string; projects: { id: number; name: string }[] } >; + // Overrides the /api/code/invites/check-access/ response (defaults to + // granting access). May throw to simulate a network error. + checkAccess?: () => Response; } = {}, ) => { const accountKey = options.accountKey ?? "user-1"; @@ -186,6 +189,9 @@ describe("AuthService", () => { } as unknown as Response; } + if (options.checkAccess) { + return options.checkAccess(); + } return { ok: true, json: vi.fn().mockResolvedValue({ has_access: true }), @@ -1344,35 +1350,6 @@ describe("AuthService", () => { }); describe("code access resilience", () => { - // A fetch stub whose user/org endpoints always succeed, so only the - // check-access endpoint's behaviour varies between tests. - const authFetchWithCheck = (checkAccess: () => Response) => - vi.fn(async (input: string | Request) => { - const url = typeof input === "string" ? input : input.url; - - if (url.includes("/api/users/@me/")) { - return { - ok: true, - json: vi.fn().mockResolvedValue({ - uuid: "user-1", - organization: { id: "org-1" }, - }), - } as unknown as Response; - } - - if (/\/api\/organizations\/([^/]+)\/$/.test(url)) { - return { - ok: true, - json: vi.fn().mockResolvedValue({ - name: "Org 1", - teams: [{ id: 42, name: "Project 42" }], - }), - } as unknown as Response; - } - - return checkAccess(); - }) as unknown as typeof fetch; - const okBody = (body: unknown): Response => ({ ok: true, @@ -1415,7 +1392,7 @@ describe("AuthService", () => { expected: null, }, ])("$name", async ({ checkAccess, expected }) => { - vi.stubGlobal("fetch", authFetchWithCheck(checkAccess)); + stubAuthFetch({ checkAccess }); await service.initialize(); @@ -1424,60 +1401,52 @@ describe("AuthService", () => { expect(state.hasCodeAccess).toBe(expected); }); - it("keeps a confirmed grant when a later check fails with a network error", async () => { - let failCheck = false; - vi.stubGlobal( - "fetch", - authFetchWithCheck(() => { - if (failCheck) throw new Error("network down"); - return okBody({ has_access: true }); - }), - ); - - await service.initialize(); - expect(service.getState().hasCodeAccess).toBe(true); - - failCheck = true; - await service.refreshAccessToken(); - - // A transient blip must not be mistaken for a revoked invite. - expect(service.getState().hasCodeAccess).toBe(true); - }); - - it("keeps a confirmed grant when a later check returns 401", async () => { - let unauthorized = false; - vi.stubGlobal( - "fetch", - authFetchWithCheck(() => - unauthorized - ? ({ - ok: false, - status: 401, - json: vi.fn().mockResolvedValue({}), - } as unknown as Response) - : okBody({ has_access: true }), - ), - ); + // A transient blip on a later check must not be mistaken for a revoked + // invite — a previously confirmed grant stays granted. + it.each([ + { + name: "a network error", + fail: (): Response => { + throw new Error("network down"); + }, + }, + { + name: "a 401", + fail: (): Response => + ({ + ok: false, + status: 401, + json: vi.fn().mockResolvedValue({}), + }) as unknown as Response, + }, + ])( + "keeps a confirmed grant when a later check hits $name", + async ({ fail }) => { + let shouldFail = false; + stubAuthFetch({ + checkAccess: () => + shouldFail ? fail() : okBody({ has_access: true }), + }); - await service.initialize(); - expect(service.getState().hasCodeAccess).toBe(true); + await service.initialize(); + expect(service.getState().hasCodeAccess).toBe(true); - unauthorized = true; - await service.refreshAccessToken(); + shouldFail = true; + await service.refreshAccessToken(); - expect(service.getState().hasCodeAccess).toBe(true); - }); + expect(service.getState().hasCodeAccess).toBe(true); + }, + ); it("recovers within the retry loop when a later attempt succeeds", async () => { let attempts = 0; - vi.stubGlobal( - "fetch", - authFetchWithCheck(() => { + stubAuthFetch({ + checkAccess: () => { attempts += 1; if (attempts === 1) throw new Error("transient"); return okBody({ has_access: true }); - }), - ); + }, + }); await service.initialize(); From 028388ecb3da7f602a44151216b3b3dceded8e95 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 14:07:09 +0100 Subject: [PATCH 09/10] docs(connectivity): remove self-evident comments Drop comments the code already makes clear: the `isOnline` field note (the gating is visible in the disabled-reason helpers), the offline-test note (redundant with the source-side comment), the `checkNow` doc (the name says it), and trim the reconnect test's singleton note to one line. No behavior change. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/git-interaction/gitInteractionLogic.test.ts | 2 -- packages/core/src/git-interaction/gitInteractionLogic.ts | 2 -- packages/ui/src/features/connectivity/connectivityClient.ts | 1 - .../connectivity/network-reconnect.contribution.test.ts | 3 +-- 4 files changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/core/src/git-interaction/gitInteractionLogic.test.ts b/packages/core/src/git-interaction/gitInteractionLogic.test.ts index b0232e8d8..1f6fdf510 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.test.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.test.ts @@ -252,8 +252,6 @@ describe("computeGitInteractionState", () => { }); describe("offline", () => { - // create-pr is dropped from `actions` when disabled, so its reason is read - // from the named field. it.each([ { action: "push", diff --git a/packages/core/src/git-interaction/gitInteractionLogic.ts b/packages/core/src/git-interaction/gitInteractionLogic.ts index fa7f2d48b..8ac016848 100644 --- a/packages/core/src/git-interaction/gitInteractionLogic.ts +++ b/packages/core/src/git-interaction/gitInteractionLogic.ts @@ -20,8 +20,6 @@ interface GitState { headBranch: string | null; prUrl: string | null; } | null; - /** Remote actions (push/sync/publish, create PR) gate on this; local - * actions (commit, new branch) don't. */ isOnline: boolean; } diff --git a/packages/ui/src/features/connectivity/connectivityClient.ts b/packages/ui/src/features/connectivity/connectivityClient.ts index fe16749b6..ce98f4770 100644 --- a/packages/ui/src/features/connectivity/connectivityClient.ts +++ b/packages/ui/src/features/connectivity/connectivityClient.ts @@ -9,7 +9,6 @@ export interface ConnectivityStatusPayload { export interface ConnectivityClient { getStatus(): Promise; - /** Forces an immediate reachability probe; backs the banner's Retry. */ checkNow(): Promise; onStatusChange(sub: Subscriber): { unsubscribe: () => void; diff --git a/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts b/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts index 13782b735..6e523c86d 100644 --- a/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts +++ b/packages/ui/src/features/connectivity/network-reconnect.contribution.test.ts @@ -13,8 +13,7 @@ function setOnline(isOnline: boolean) { describe("NetworkReconnectContribution", () => { beforeEach(() => { - // The store is a process-wide singleton shared with the contribution; reset - // it to the default online state before each case. + // Reset the process-wide singleton store between cases. setOnline(true); }); From 3e141c0bd420b5020347ce64aaff02e1f93b2820 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 25 Jun 2026 14:07:45 +0100 Subject: [PATCH 10/10] docs(auth): remove self-evident test comments Drop two comments the test names and rows already convey: the "null (not false)" note above the parameterized code-access cases, and the "transient blip" note above the grant-preservation cases. Test-only; no behavior change. Generated-By: PostHog Code Task-Id: 8a950c40-02d5-444a-b653-0485cd4f1756 --- packages/core/src/auth/auth.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/core/src/auth/auth.test.ts b/packages/core/src/auth/auth.test.ts index 870ca218b..328923f8f 100644 --- a/packages/core/src/auth/auth.test.ts +++ b/packages/core/src/auth/auth.test.ts @@ -1366,8 +1366,6 @@ describe("AuthService", () => { ); }); - // null (not false) for an inconclusive check: the UI shows "Checking - // access…" rather than the invite screen, and the next sync re-checks. it.each([ { name: "grants access when the server reports has_access true", @@ -1401,8 +1399,6 @@ describe("AuthService", () => { expect(state.hasCodeAccess).toBe(expected); }); - // A transient blip on a later check must not be mistaken for a revoked - // invite — a previously confirmed grant stays granted. it.each([ { name: "a network error",