diff --git a/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts b/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts new file mode 100644 index 0000000000..df2484ecbc --- /dev/null +++ b/packages/studio/src/hooks/useAbsentReadRecoveryTelemetry.ts @@ -0,0 +1,40 @@ +import { useEffect, useRef } from "react"; +import { trackStudioEvent } from "../utils/studioTelemetry"; + +/** 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[], + fileTreeLoaded: boolean, +) { + const refreshedPathsRef = useRef>(new Set()); + const pendingRef = useRef>(new Map()); + useEffect(() => { + refreshedPathsRef.current.clear(); + pendingRef.current.clear(); + }, [projectId]); + + 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]); + + /** 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; + 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 e999006ca0..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( @@ -655,6 +662,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,19 +756,307 @@ describe("useSdkSession unavailable telemetry", () => { await act(async () => root.unmount()); }); - it("does not throw when onAbsentRead is not supplied", async () => { + // 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 flushAsyncEffects(); + expect(trackMock).toHaveBeenCalledWith("sdk_absent_read_recovery", { stage: "triggered" }); + trackMock.mockClear(); + + await act(async () => { + 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: "tree_corrected" }), + ); + 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 () => - ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response, - ), + 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, 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 () => 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( + , + ), + ); + await flushAsyncEffects(); + + 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 98efb71b90..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,13 +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]); + // 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 @@ -399,10 +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); - onAbsentRead?.(forPath); + absentReadRecovery.triggerOnce(forPath, onAbsentRead); } useEffect(