From e33e194dc33db001ee35b4654501d7660d67349e Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 1 Oct 2026 01:17:31 -0700 Subject: [PATCH 1/3] feat(studio): emit sdk_absent_read_recovery around the stale-tree fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The acceptance metric chosen for #4370 (absent-reads-per-tab, p50/max) never moved — re-checked 4 times over a week against 25+ versions and ~1200 tabs, p50 sat at 1.0 on every single version, fix included. That metric can't prove or disprove the fix: it's dominated by agent-driven sessions (100% of absent-read events in the last 7 days carry PostHog's bot flag — expected, this product is built for agents) where more than one absent read per tab was already the norm before the fix existed, so there was never a "loop" baseline to show improvement against. This adds the metric that can answer it directly. `useSdkSession` already has the one state transition that matters: `onAbsentRead` fires once per (projectId, path) when a read hits `reason: "absent"` (the trigger), and the open effect's next successful read for that identity is the only possible "it healed" signal (the resolution). Both sides were silent. `sdk_absent_read_recovery` now fires twice around that window: - `{ stage: "triggered" }` from `handleReadFailure`, alongside the existing `onAbsentRead` call. - `{ stage: "recovered", elapsed_ms }` from the open effect's success branch, via `reportAbsentRecoveryIfPending`, keyed off a `Map` recording when the trigger fired. A path that never recovers just never emits the second event — visible directly as a gap between triggered and recovered counts, no inference required. 3 new tests: triggered fires alongside onAbsentRead, recovered fires with a numeric elapsed_ms on the next successful read, and no recovery event for a read that never went absent. 51/51 across the three touched test files, oxlint/oxfmt/tsc clean. Co-Authored-By: Claude Sonnet 5 --- .../hooks/useSdkSession.lifecycle.test.tsx | 44 +++++++++++++++++++ packages/studio/src/hooks/useSdkSession.ts | 24 ++++++++++ 2 files changed, 68 insertions(+) diff --git a/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx b/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx index e999006ca0..cd66a5ad30 100644 --- a/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx +++ b/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx @@ -655,6 +655,9 @@ describe("useSdkSession unavailable telemetry", () => { expect(captured.handle?.compositionMissing).toBe(true); expect(onAbsentRead).toHaveBeenCalledOnce(); expect(onAbsentRead).toHaveBeenCalledWith("index.html"); + expect(trackMock).toHaveBeenCalledWith("sdk_absent_read_recovery", { + stage: "triggered", + }); // A second absent read for the SAME path must not refresh again — the // refresh already ran and didn't fix it (the file really is gone). @@ -746,6 +749,47 @@ describe("useSdkSession unavailable telemetry", () => { await act(async () => root.unmount()); }); + it("emits sdk_absent_read_recovery 'recovered' with elapsed_ms once a later read succeeds", async () => { + const fetchMock = vi.fn( + async () => ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response, + ); + vi.stubGlobal("fetch", fetchMock); + openComposition.mockResolvedValue(fakeSession()); + const root = createRoot(document.createElement("div")); + await act(async () => root.render()); + await flushAsyncEffects(); + expect(trackMock).toHaveBeenCalledWith("sdk_absent_read_recovery", { + stage: "triggered", + }); + trackMock.mockClear(); + + fetchMock.mockImplementation(async () => response("PROJECT_A")); + await act(async () => { + captured.handle?.forceReload(); + }); + await flushAsyncEffects(); + + expect(trackMock).toHaveBeenCalledWith( + "sdk_absent_read_recovery", + expect.objectContaining({ stage: "recovered", elapsed_ms: expect.any(Number) }), + ); + await act(async () => root.unmount()); + }); + + it("does not report a recovery for a read that never went absent", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => response("PROJECT_A")), + ); + openComposition.mockResolvedValue(fakeSession()); + const root = createRoot(document.createElement("div")); + await act(async () => root.render()); + await flushAsyncEffects(); + + expect(trackMock).not.toHaveBeenCalledWith("sdk_absent_read_recovery", expect.anything()); + await act(async () => root.unmount()); + }); + it("does not throw when onAbsentRead is not supplied", async () => { vi.stubGlobal( "fetch", diff --git a/packages/studio/src/hooks/useSdkSession.ts b/packages/studio/src/hooks/useSdkSession.ts index 98efb71b90..699a9d106a 100644 --- a/packages/studio/src/hooks/useSdkSession.ts +++ b/packages/studio/src/hooks/useSdkSession.ts @@ -383,6 +383,15 @@ export function useSdkSession( useEffect(() => { refreshedAbsentPathsRef.current.clear(); }, [projectId]); + // Start time of each outstanding tree-refresh-on-absent fallback, keyed the + // same way. Telemetry-only: proves whether `onAbsentRead` actually resolves + // the stale tree (the prior acceptance metric, absent-reads-per-tab, turned + // out to never move with or without the fix — it was dominated by agent- + // driven sessions where >1 absent read per tab was already the norm, not a + // sign of the loop this fallback targets). This measures the one thing that + // metric couldn't: did a given absent read get followed by a successful one + // for the same path, and how long did that take. + const pendingAbsentRecoveryRef = useRef>(new Map()); /** * Update `unreachableProject`/`compositionMissing` for one failed read, and @@ -402,9 +411,23 @@ export function useSdkSession( const key = `${forProjectId}:${forPath}`; if (refreshedAbsentPathsRef.current.has(key)) return; refreshedAbsentPathsRef.current.add(key); + pendingAbsentRecoveryRef.current.set(key, Date.now()); + trackStudioEvent("sdk_absent_read_recovery", { stage: "triggered" }); onAbsentRead?.(forPath); } + /** The other half of `sdk_absent_read_recovery`: a read that just succeeded. */ + function reportAbsentRecoveryIfPending(forProjectId: string, forPath: string): void { + const key = `${forProjectId}:${forPath}`; + const startedAt = pendingAbsentRecoveryRef.current.get(key); + if (startedAt === undefined) return; + pendingAbsentRecoveryRef.current.delete(key); + trackStudioEvent("sdk_absent_read_recovery", { + stage: "recovered", + elapsed_ms: Date.now() - startedAt, + }); + } + useEffect( () => addExternalFileReloadListener((changedPath) => { @@ -454,6 +477,7 @@ export function useSdkSession( } setUnreachableProject(null); setCompositionMissing(false); + reportAbsentRecoveryIfPending(projectId, activeCompPath); const content = read.content; // No persist queue: Studio's writeProjectFile (via sdkCutover's // persistSdkSerialize) is the SINGLE writer. Wiring the SDK persist From 5acd65d304e2072636efe95f16603799e3654821 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 1 Oct 2026 01:58:09 -0700 Subject: [PATCH 2/3] fix(studio): measure sdk_absent_read_recovery on the tree, not a same-path read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #4843 (Somu, two independent passes including Codex) found the original design would most likely report a WORKING fix as "rarely recovers" — both findings verified against source before this fix: B1: `onAbsentRead` → `refreshFileTree` starts no read (confirmed in useFileTree.ts — it only updates the fetched file/composition lists), and the open effect deliberately excludes `fileTree` from its deps. So the fallback's actual target scenario (an agent deletes the file, the SSE is missed) never produces a same-path read success: the tree just stops listing the path and CompositionMissingBanner tells the user to pick another composition. The old `recovered` stage would have scored that as unrecovered. On the master view it's worse — a refresh rotates `masterCompPath` to the next composition (App.tsx), orphaning the original trigger's key entirely. B2: DesignPanelPromoteProvider opens a second, callback-less `useSdkSession` targeting the same path as the primary session whenever nothing is selected (confirmed at its call site) — `triggered` fired from both, with no field to tell PostHog's two identical events apart. Both fixed, and extracted into `useAbsentReadRecoveryTelemetry` (also keeps useSdkSession.ts under the 600-line filesize gate): - `triggerOnce` now only does the once-per-path bookkeeping and emits `triggered` when `onAbsentRead` is actually provided — the callback-less secondary session no longer fires at all. - The success half is now `stage: "tree_corrected"`, fired from an effect over `fileTree`/`fileTreeLoaded` that checks every still-pending path against the CURRENT tree, independent of which path `useSdkSession` happens to be reading right now. That's the one thing the fallback actually controls. - `elapsed_ms` now comes from `performance.now()` (monotonic), not `Date.now()` (wall-clock, skews under tab suspension or clock changes). - Pending keys are path-only, not `${projectId}:${path}` — the composite key could stringify two different (projectId, path) pairs identically once either contains `:` (a real near-term risk: #4829 opens colon- containing project names). Safe without the prefix because the map is fully cleared on every `projectId` change, so only one project's entries are ever live. That clear now covers both the dedup set and the pending map — only the former was cleared before, so a pending entry could survive a project switch and either inflate its own `elapsed_ms` with idle time spent on the other project, or collide with an unrelated path reused there. 9 new/rewritten tests, 2 manually mutation-checked against this head (removing the `onAbsentRead` guard, removing the delete-after-emit) — both caught by the existing assertions before being reverted. 56/56 across the three touched test files. oxlint/oxfmt/tsc clean. Co-Authored-By: Claude Sonnet 5 --- .../hooks/useAbsentReadRecoveryTelemetry.ts | 76 +++++ .../hooks/useSdkSession.lifecycle.test.tsx | 296 ++++++++++++++++-- packages/studio/src/hooks/useSdkSession.ts | 39 +-- 3 files changed, 355 insertions(+), 56 deletions(-) create mode 100644 packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts diff --git a/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts b/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts new file mode 100644 index 0000000000..5a00ce91d3 --- /dev/null +++ b/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts @@ -0,0 +1,76 @@ +import { useEffect, useRef } from "react"; +import { trackStudioEvent } from "../utils/studioTelemetry"; + +/** + * Tracks whether `useSdkSession`'s tree-refresh-on-absent fallback + * (`onAbsentRead`) actually resolves a stale tree, and reports + * `sdk_absent_read_recovery` telemetry for it. + * + * The prior acceptance metric for that fallback — absent-reads-per-tab — + * turned out to never move with or without the fix: it was dominated by + * agent-driven sessions where more than one absent read per tab was already + * the norm, not a sign of the loop the fallback targets. + * + * This measures the one thing that metric couldn't, and does so on the + * TREE, not on a same-path read succeeding: `onAbsentRead` only calls + * `refreshFileTree`, which starts no read, and in the fallback's own target + * scenario — an agent deletes the file and the SSE-driven refresh is missed + * — no same-path read ever happens again. The tree just stops listing the + * path, and `CompositionMissingBanner` tells the user to pick another + * composition. Measuring "read succeeded" would have scored that as + * unrecovered. What the fallback actually controls is the tree itself, so + * recovery here is "this path is no longer in `fileTree`". + * + * Keyed by path alone, not `${projectId}:${path}` — the composite key could + * stringify two different (projectId, path) pairs identically once either + * part contains `:`. Safe without the prefix only because every entry is + * cleared on `projectId` change, so just one project's worth is ever live. + */ +export function useAbsentReadRecoveryTelemetry( + projectId: string | null, + fileTree: readonly string[], + fileTreeLoaded: boolean, +) { + const refreshedPathsRef = useRef>(new Set()); + const pendingRef = useRef>(new Map()); + useEffect(() => { + refreshedPathsRef.current.clear(); + pendingRef.current.clear(); + }, [projectId]); + + // Runs on every tree update rather than tied to this hook's own open/read + // cycle — on the master view, a refresh rotates `masterCompPath` to the + // next composition, so the path `useSdkSession` is currently reading is + // NOT the one whose absence triggered the fallback; only the tree itself + // says whether that one resolved. + useEffect(() => { + if (!fileTreeLoaded || pendingRef.current.size === 0) return; + for (const [path, startedAt] of pendingRef.current) { + if (fileTree.includes(path)) continue; + pendingRef.current.delete(path); + trackStudioEvent("sdk_absent_read_recovery", { + stage: "tree_corrected", + elapsed_ms: performance.now() - startedAt, + }); + } + }, [fileTree, fileTreeLoaded]); + + /** + * Fires the once-per-path fallback for an absent read. No-ops without a + * collaborator: `onAbsentRead` is absent on the secondary SDK sessions + * DesignPanelPromoteProvider opens, which default to the SAME path as the + * primary session when nothing is selected — without this guard, one + * absent file fires `triggered` twice, once per hook instance, with no + * field to tell PostHog's two identical events apart. + */ + function triggerOnce(path: string, onAbsentRead: ((path: string) => void) | undefined): void { + if (!onAbsentRead) return; + if (refreshedPathsRef.current.has(path)) return; + refreshedPathsRef.current.add(path); + pendingRef.current.set(path, performance.now()); + trackStudioEvent("sdk_absent_read_recovery", { stage: "triggered" }); + onAbsentRead(path); + } + + return { triggerOnce }; +} diff --git a/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx b/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx index cd66a5ad30..d120d9f49f 100644 --- a/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx +++ b/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx @@ -625,15 +625,22 @@ describe("useSdkSession unavailable telemetry", () => { projectId, path, onAbsentRead, + fileTree = [], + fileTreeLoaded = false, }: { projectId: string; path: string; onAbsentRead?: (path: string) => void; + fileTree?: readonly string[]; + fileTreeLoaded?: boolean; }) { - captured.handle = useSdkSession(projectId, path, [], false, onAbsentRead); + captured.handle = useSdkSession(projectId, path, fileTree, fileTreeLoaded, onAbsentRead); return null; } const captured: { handle: SdkSessionHandle | null } = { handle: null }; + const absentRead = vi.fn( + async () => ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response, + ); it("sets compositionMissing and calls onAbsentRead once for an absent read", async () => { vi.stubGlobal( @@ -749,29 +756,204 @@ describe("useSdkSession unavailable telemetry", () => { await act(async () => root.unmount()); }); - it("emits sdk_absent_read_recovery 'recovered' with elapsed_ms once a later read succeeds", async () => { - const fetchMock = vi.fn( - async () => ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response, - ); - vi.stubGlobal("fetch", fetchMock); - openComposition.mockResolvedValue(fakeSession()); + // Recovery is measured on the TREE, not on a same-path read succeeding — + // see the comment on `pendingTreeCorrectionRef` in useSdkSession.ts. + // Reviewed on #4843 (Somu): the original design measured "a later read of + // the same path succeeded", which the fallback does not control (it only + // calls `refreshFileTree`, which starts no read) and which the fallback's + // OWN target scenario never satisfies (the file stays gone; the tree just + // stops listing it). That would have scored a working fix as "never + // recovers". These tests exercise the corrected design. + it("emits tree_corrected with a positive elapsed_ms once the path drops out of the tree", async () => { + // Not mocking `performance.now()`: other code (React, jsdom) calls it + // too, so a queued mock value can be consumed by one of those instead + // of by this effect. A real (small) delay between trigger and + // resolution is what actually defeats a hardcoded `elapsed_ms: 0`. + vi.stubGlobal("fetch", absentRead); const root = createRoot(document.createElement("div")); - await act(async () => root.render()); + await act(async () => + root.render( + , + ), + ); await flushAsyncEffects(); - expect(trackMock).toHaveBeenCalledWith("sdk_absent_read_recovery", { - stage: "triggered", - }); + expect(trackMock).toHaveBeenCalledWith("sdk_absent_read_recovery", { stage: "triggered" }); trackMock.mockClear(); - fetchMock.mockImplementation(async () => response("PROJECT_A")); await act(async () => { - captured.handle?.forceReload(); + await new Promise((resolve) => setTimeout(resolve, 5)); }); + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + + expect(trackMock).toHaveBeenCalledWith( + "sdk_absent_read_recovery", + expect.objectContaining({ stage: "tree_corrected", elapsed_ms: expect.any(Number) }), + ); + const call = trackMock.mock.calls.find( + ([event, props]) => + event === "sdk_absent_read_recovery" && props?.stage === "tree_corrected", + ); + expect(call?.[1]?.elapsed_ms).toBeGreaterThan(0); + await act(async () => root.unmount()); + }); + + it("does not emit tree_corrected while the path is still listed", async () => { + vi.stubGlobal("fetch", absentRead); + const root = createRoot(document.createElement("div")); + await act(async () => + root.render(), + ); + await flushAsyncEffects(); + trackMock.mockClear(); + + // fileTreeLoaded flips true, but the path is STILL in the tree — must stay silent. + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + + expect(trackMock).not.toHaveBeenCalledWith( + "sdk_absent_read_recovery", + expect.objectContaining({ stage: "tree_corrected" }), + ); + await act(async () => root.unmount()); + }); + + it("does not re-emit tree_corrected on a later unrelated tree update", async () => { + vi.stubGlobal("fetch", absentRead); + const root = createRoot(document.createElement("div")); + // First render lists the path (the realistic stale-tree shape — the + // tree update that resolves it always happens on a LATER render); this + // populates the pending map before the tree ever says it's gone. + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + trackMock.mockClear(); + + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + expect(trackMock).toHaveBeenCalledWith( + "sdk_absent_read_recovery", + expect.objectContaining({ stage: "tree_corrected" }), + ); + trackMock.mockClear(); + + // A new fileTree reference, still not containing the path: the pending + // entry must already be gone, not re-matched. + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + + expect(trackMock).not.toHaveBeenCalledWith( + "sdk_absent_read_recovery", + expect.objectContaining({ stage: "tree_corrected" }), + ); + await act(async () => root.unmount()); + }); + + it("resolves only the path that actually left the tree, not any pending path", async () => { + // One hook instance, two paths over time — the master-view-rotation + // shape: `masterCompPath` moves to the next composition after a + // refresh, so the hook can be actively reading path B while path A's + // pending entry (from before the rotation) is still unresolved. + vi.stubGlobal("fetch", absentRead); + const onAbsentRead = vi.fn(); + const root = createRoot(document.createElement("div")); + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + await act(async () => + root.render( + , + ), + ); await flushAsyncEffects(); + expect(onAbsentRead).toHaveBeenCalledTimes(2); + trackMock.mockClear(); + // Only b.html leaves the tree — a.html's pending entry must stay open. + await act(async () => + root.render( + , + ), + ); + await flushAsyncEffects(); + + expect(trackMock).toHaveBeenCalledTimes(1); expect(trackMock).toHaveBeenCalledWith( "sdk_absent_read_recovery", - expect.objectContaining({ stage: "recovered", elapsed_ms: expect.any(Number) }), + expect.objectContaining({ stage: "tree_corrected" }), ); await act(async () => root.unmount()); }); @@ -790,19 +972,91 @@ describe("useSdkSession unavailable telemetry", () => { await act(async () => root.unmount()); }); - it("does not throw when onAbsentRead is not supplied", async () => { + it("does not throw, and does not emit triggered, when onAbsentRead is not supplied", async () => { + vi.stubGlobal("fetch", absentRead); + const root = createRoot(document.createElement("div")); + await act(async () => root.render()); + await flushAsyncEffects(); + + expect(captured.handle?.compositionMissing).toBe(true); + expect(trackMock).not.toHaveBeenCalledWith("sdk_absent_read_recovery", expect.anything()); + await act(async () => root.unmount()); + }); + + // Reviewed on #4843 (Somu): DesignPanelPromoteProvider opens a second, + // callback-less `useSdkSession` targeting the SAME path as the primary + // session whenever nothing is selected — without the `onAbsentRead` guard, + // one absent file fired `triggered` twice, with no way for PostHog to + // tell the two hook instances apart. + it("does not double-count triggered when a second, callback-less session reads the same absent path", async () => { + vi.stubGlobal("fetch", absentRead); + const onAbsentRead = vi.fn(); + function TwoSessions() { + useSdkSession("project-a", "index.html", [], false, onAbsentRead); + useSdkSession("project-a", "index.html"); + return null; + } + const root = createRoot(document.createElement("div")); + await act(async () => root.render()); + await flushAsyncEffects(); + + expect(onAbsentRead).toHaveBeenCalledOnce(); + expect( + trackMock.mock.calls.filter(([event]) => event === "sdk_absent_read_recovery"), + ).toHaveLength(1); + await act(async () => root.unmount()); + }); + + // Reviewed on #4843 (Somu): `pendingTreeCorrectionRef` wasn't cleared on + // project change — a stale entry from a prior project could (a) have its + // elapsed time inflated by however long was spent on the other project, + // and (b) collide if a different project later used the same path. + it("clears the pending entry on project change, so a later project can't wrongly resolve it", async () => { + vi.stubGlobal("fetch", absentRead); + const root = createRoot(document.createElement("div")); + await act(async () => + root.render(), + ); + await flushAsyncEffects(); + expect(trackMock).toHaveBeenCalledWith("sdk_absent_read_recovery", { stage: "triggered" }); + + // Switch to a DIFFERENT project using the SAME path, with a read that + // succeeds (no trigger of its own) — isolates whatever project-b's tree + // update does from project-a's now-orphaned pending entry for the same + // key. Keys are path-only (see the comment on `refreshedAbsentPathsRef`), + // so without the clear, this is exactly the collision the key shape + // depends on the clear to avoid. vi.stubGlobal( "fetch", - vi.fn( - async () => - ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response, + vi.fn(async () => response("PROJECT_B")), + ); + openComposition.mockResolvedValue(fakeSession()); + await act(async () => { + usePlayerStore.getState().beginTimelineSession("project-b"); + usePlayerStore.getState().markPreviewBooted(); + root.render(); + }); + await flushAsyncEffects(); + trackMock.mockClear(); + + // project-b's tree not listing "index.html" must not resolve project-a's + // orphaned entry for that same path. + await act(async () => + root.render( + , ), ); - const root = createRoot(document.createElement("div")); - await act(async () => root.render()); await flushAsyncEffects(); - expect(captured.handle?.compositionMissing).toBe(true); + expect(trackMock).not.toHaveBeenCalledWith( + "sdk_absent_read_recovery", + expect.objectContaining({ stage: "tree_corrected" }), + ); await act(async () => root.unmount()); }); }); diff --git a/packages/studio/src/hooks/useSdkSession.ts b/packages/studio/src/hooks/useSdkSession.ts index 699a9d106a..b2aa53f97f 100644 --- a/packages/studio/src/hooks/useSdkSession.ts +++ b/packages/studio/src/hooks/useSdkSession.ts @@ -6,6 +6,7 @@ import { trackStudioEvent } from "../utils/studioTelemetry"; import type { PublishSdkSession } from "../utils/sdkCutover"; import { addExternalFileReloadListener } from "./externalFileReloadBus"; import { whenPreviewBooted } from "../player/store/playerStore"; +import { useAbsentReadRecoveryTelemetry } from "./useAbsentReadRecoveryTelemetry"; /** * Why an optional project-file read produced no usable content. `stage: "read"` @@ -376,22 +377,8 @@ export function useSdkSession( reloadTokenRef.current = reloadToken; const [unreachableProject, setUnreachableProject] = useState(null); const [compositionMissing, setCompositionMissing] = useState(false); - // Keyed `${projectId}:${path}` so a refresh that doesn't fix it (the file - // really is gone) can't loop, and so it fires again for a genuinely - // different path or project. - const refreshedAbsentPathsRef = useRef>(new Set()); - useEffect(() => { - refreshedAbsentPathsRef.current.clear(); - }, [projectId]); - // Start time of each outstanding tree-refresh-on-absent fallback, keyed the - // same way. Telemetry-only: proves whether `onAbsentRead` actually resolves - // the stale tree (the prior acceptance metric, absent-reads-per-tab, turned - // out to never move with or without the fix — it was dominated by agent- - // driven sessions where >1 absent read per tab was already the norm, not a - // sign of the loop this fallback targets). This measures the one thing that - // metric couldn't: did a given absent read get followed by a successful one - // for the same path, and how long did that take. - const pendingAbsentRecoveryRef = useRef>(new Map()); + // Keyed by path alone: cleared below on every `projectId` change, so only + const absentReadRecovery = useAbsentReadRecoveryTelemetry(projectId, fileTree, fileTreeLoaded); /** * Update `unreachableProject`/`compositionMissing` for one failed read, and @@ -408,24 +395,7 @@ export function useSdkSession( setUnreachableProject(reportReadFailure(read, forProjectId, pathInTree)); setCompositionMissing(read.reason === "absent"); if (read.reason !== "absent") return; - const key = `${forProjectId}:${forPath}`; - if (refreshedAbsentPathsRef.current.has(key)) return; - refreshedAbsentPathsRef.current.add(key); - pendingAbsentRecoveryRef.current.set(key, Date.now()); - trackStudioEvent("sdk_absent_read_recovery", { stage: "triggered" }); - onAbsentRead?.(forPath); - } - - /** The other half of `sdk_absent_read_recovery`: a read that just succeeded. */ - function reportAbsentRecoveryIfPending(forProjectId: string, forPath: string): void { - const key = `${forProjectId}:${forPath}`; - const startedAt = pendingAbsentRecoveryRef.current.get(key); - if (startedAt === undefined) return; - pendingAbsentRecoveryRef.current.delete(key); - trackStudioEvent("sdk_absent_read_recovery", { - stage: "recovered", - elapsed_ms: Date.now() - startedAt, - }); + absentReadRecovery.triggerOnce(forPath, onAbsentRead); } useEffect( @@ -477,7 +447,6 @@ export function useSdkSession( } setUnreachableProject(null); setCompositionMissing(false); - reportAbsentRecoveryIfPending(projectId, activeCompPath); const content = read.content; // No persist queue: Studio's writeProjectFile (via sdkCutover's // persistSdkSerialize) is the SINGLE writer. Wiring the SDK persist From e359a8eb28009b24ac210cbbef6aaca06b1144af Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 1 Oct 2026 02:08:29 -0700 Subject: [PATCH 3/3] docs(studio): trim useAbsentReadRecoveryTelemetry.ts under its package's comment share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's comment-ratchet gate flagged it at 18.0% against the package's 10.8% — cut the per-function rationale down to one line each; the invariant is in the commit history and the PR thread, not owed to every reader of the file. Co-Authored-By: Claude Sonnet 5 --- .../hooks/useAbsentReadRecoveryTelemetry.ts | 40 +------------------ 1 file changed, 2 insertions(+), 38 deletions(-) diff --git a/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts b/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts index 5a00ce91d3..df2484ecbc 100644 --- a/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts +++ b/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts @@ -1,31 +1,7 @@ import { useEffect, useRef } from "react"; import { trackStudioEvent } from "../utils/studioTelemetry"; -/** - * Tracks whether `useSdkSession`'s tree-refresh-on-absent fallback - * (`onAbsentRead`) actually resolves a stale tree, and reports - * `sdk_absent_read_recovery` telemetry for it. - * - * The prior acceptance metric for that fallback — absent-reads-per-tab — - * turned out to never move with or without the fix: it was dominated by - * agent-driven sessions where more than one absent read per tab was already - * the norm, not a sign of the loop the fallback targets. - * - * This measures the one thing that metric couldn't, and does so on the - * TREE, not on a same-path read succeeding: `onAbsentRead` only calls - * `refreshFileTree`, which starts no read, and in the fallback's own target - * scenario — an agent deletes the file and the SSE-driven refresh is missed - * — no same-path read ever happens again. The tree just stops listing the - * path, and `CompositionMissingBanner` tells the user to pick another - * composition. Measuring "read succeeded" would have scored that as - * unrecovered. What the fallback actually controls is the tree itself, so - * recovery here is "this path is no longer in `fileTree`". - * - * Keyed by path alone, not `${projectId}:${path}` — the composite key could - * stringify two different (projectId, path) pairs identically once either - * part contains `:`. Safe without the prefix only because every entry is - * cleared on `projectId` change, so just one project's worth is ever live. - */ +/** Reports `sdk_absent_read_recovery`: recovery is the path leaving `fileTree`, not a later read of it succeeding. */ export function useAbsentReadRecoveryTelemetry( projectId: string | null, fileTree: readonly string[], @@ -38,11 +14,6 @@ export function useAbsentReadRecoveryTelemetry( pendingRef.current.clear(); }, [projectId]); - // Runs on every tree update rather than tied to this hook's own open/read - // cycle — on the master view, a refresh rotates `masterCompPath` to the - // next composition, so the path `useSdkSession` is currently reading is - // NOT the one whose absence triggered the fallback; only the tree itself - // says whether that one resolved. useEffect(() => { if (!fileTreeLoaded || pendingRef.current.size === 0) return; for (const [path, startedAt] of pendingRef.current) { @@ -55,14 +26,7 @@ export function useAbsentReadRecoveryTelemetry( } }, [fileTree, fileTreeLoaded]); - /** - * Fires the once-per-path fallback for an absent read. No-ops without a - * collaborator: `onAbsentRead` is absent on the secondary SDK sessions - * DesignPanelPromoteProvider opens, which default to the SAME path as the - * primary session when nothing is selected — without this guard, one - * absent file fires `triggered` twice, once per hook instance, with no - * field to tell PostHog's two identical events apart. - */ + /** No-ops without `onAbsentRead`, so a collaborator-less caller can't double-count. */ function triggerOnce(path: string, onAbsentRead: ((path: string) => void) | undefined): void { if (!onAbsentRead) return; if (refreshedPathsRef.current.has(path)) return;