diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index 459dccb27..5e8d80aa5 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -71,10 +71,62 @@ import { monitorDemos, reportDemoEvent, reportError, reportingEnabled, Sentry } import { isMonitorPayload } from "@handsontable/demo-runtime/monitor"; import { tier1Report } from "./tier1Report.js"; import { isOpaqueNetworkFailure } from "./fetchFailure.js"; +import { + readFetchDiagnostics, + apiBaseOrigin, + diagnosticTags, + diagnosticExtras, + netEffectiveType, +} from "./fetchDiagnostics.js"; +import { recordEditorEvent } from "./editorTrail.js"; const SANDPACK_BUNDLER_URL = import.meta.env.VITE_SANDPACK_BUNDLER_URL || undefined; const API_BASE = import.meta.env.VITE_API_BASE || "http://localhost:8787"; +// DEV-2859: one synthesized "versions fetch unreachable" event per page load, +// however many times the effect below re-fires (StrictMode's double-invoke in +// dev, or a genuine remount). Module-scope, not component state, because a +// remount must not reset an allowance that already told the operator "this +// visitor's session tried, and both attempts failed". +let versionsFetchEventSent = false; + +/** + * DEMOS-7D (DEV-2859 ruling): instrument, don't suppress. `App.tsx`'s two + * `fetchDocsJson` catches already report *every* failure with no + * `isOpaqueNetworkFailure` gate, so this only adds tags to the event that was + * always going to be sent — no re-promotion, no fingerprint change, no new + * issue. `Sentry.captureException` inside `run()` picks up whatever tags this + * scope carries, which is what makes wrapping the existing call enough. + * + * `docs-catalog.ts`'s `fetchDocsJson` has no retry of its own to report: it + * cannot import `fetchDiagnostics.ts` and stay importable by + * `pipeline/docs-catalog.test.mjs` under `--experimental-strip-types`, which + * cannot resolve a sibling `./x.js` specifier (verified empirically against a + * throwaway probe file — the same constraint `fetchDiagnostics.ts`'s header + * documents). So this diagnostics bundle is deliberately thinner than the + * versions-fetch one: online state and the API-base classification, no + * attempt count or outcome (`diagnosticTags`/`diagnosticExtras` both treat + * those as optional for exactly this caller). + */ +function withDocsFetchDiagnostics(context: string, run: () => void): void { + Sentry.withScope((scope) => { + const { context: _population, ...tags } = diagnosticTags({ + context, + onlineAtStart: typeof navigator !== "undefined" ? navigator.onLine : undefined, + apiBaseOrigin: apiBaseOrigin(API_BASE, location.origin), + }); + // `context` is dropped on purpose. `run()` is a `reportError` call, and that + // captures with `{ tags: { context } }` of its own — an event-level tag beats + // a scope one, so setting `context` here would be silently overwritten and + // this wrapper would look like it tagged something it did not. The population + // is still identifiable two ways that do NOT collide: `reportError`'s own + // `docs-example-load:fetch` / `docs-bucket-resolve:fetch` value, and the + // `docs_fetch_*` tag names `prefixFor` derives from the context passed above. + scope.setTags(tags); + run(); + }); +} + // Framework preference (used to auto-pick a variant when an example is chosen and // the current one isn't available) + short labels for the framework picker. const FW_PREF = ["react", "typescript", "javascript", "vue", "angular"]; @@ -1330,6 +1382,20 @@ function Authoring({ /** Replace the whole workspace (entry + files + lineage) and remount. */ const loadWorkspace = useCallback( (nextEntry: CatalogEntry, nextFiles: FilesMap, lineage: string) => { + // DEV-2859: the lineage *prefix only* — the segment before the first + // `:` — never the full string. `import:` and `docs::` + // both carry visitor- or docs-authored data after that first colon; a + // saved-demo id (no colon at all) collapses to the constant "saved" + // rather than the id itself, which is the shape this redaction is aimed + // at (an id is not a URL, but it is still not this trail's business to + // carry verbatim). + recordEditorEvent({ + kind: "workspace", + source: "load", + path: lineage.includes(":") ? lineage.slice(0, lineage.indexOf(":")) : "saved", + quiet: false, + size: 0, + }); // Whatever workspace replaces an ad-hoc one is no longer its, so its title // and its skipped-files notice are cleared here — at the moment the new // files are installed, which a failed starter or docs load never reaches. @@ -1652,8 +1718,19 @@ function Authoring({ useEffect(() => { let cancelled = false; fetchVersions(API_BASE) - .then(({ latest, next, versions }) => { + .then(({ latest, next, versions, diagnostics }) => { if (cancelled) return; + // A retry that recovered is a blip, not an issue (DEV-2859) — the same + // "worth knowing, not worth an issue" treatment the catch branch below + // gives the visitor-network population, on the success side of it. + if ((diagnostics.attempts ?? 1) > 1) { + Sentry.addBreadcrumb({ + category: "fetch", + level: "info", + message: "versions-fetch recovered on retry", + data: diagnosticTags({ ...diagnostics, context: "versions-fetch" }), + }); + } setNextVersion(next ?? ""); const opts = [...new Set([latest, ...versions, next].filter((v): v is string => !!v))]; if (opts.length) setVersionOptions(opts); @@ -1665,9 +1742,37 @@ function Authoring({ .catch((error) => { // Fails open onto the hardcoded VERSION_OPTIONS, so the picker silently // goes stale rather than breaking — worth knowing about, unless it is the - // visitor's own network dropping mid-request (Sentry DEMOS-2X): that shape - // carries nothing about our host, so it is a breadcrumb, not an issue. - if (isOpaqueNetworkFailure(error)) { + // visitor's own network dropping mid-request (Sentry DEMOS-2X). + // + // `fetchDiagnostics` is read FIRST, ahead of `isOpaqueNetworkFailure` + // (DEV-2859): once the sibling fetchFailure.ts fix lands, that check + // starts matching the real production wording, and if it ran first + // every exhausted two-attempt failure would fall back to a silent + // breadcrumb — exactly the regression item 2 exists to prevent. A + // `!res.ok` throw and a JSON-parse failure never carry + // `fetchDiagnostics` (both are thrown by catalog.ts itself, not by + // fetchWithDiagnostics), so they fall through to the branch below + // unchanged from today. + const diag = readFetchDiagnostics(error); + if (diag) { + if (!versionsFetchEventSent) { + versionsFetchEventSent = true; + const full = { + ...diag, + context: "versions-fetch" as const, + apiBaseOrigin: apiBaseOrigin(API_BASE, location.origin), + netEffectiveType: netEffectiveType(), + }; + Sentry.withScope((scope) => { + scope.setLevel("warning"); + scope.setTags(diagnosticTags(full)); + scope.setExtras(diagnosticExtras(full)); + Sentry.captureMessage("versions fetch unreachable", { + fingerprint: ["versions-fetch-unreachable"], + }); + }); + } + } else if (isOpaqueNetworkFailure(error)) { Sentry.addBreadcrumb({ category: "fetch", level: "info", @@ -1821,13 +1926,25 @@ function Authoring({ // open. Tagged by which step failed so a missing artifact (docs linking // an example that was never imported) is distinguishable from a // transient fetch. - reportError(error, `docs-example-load:${isMissingDocsResource(error) ? "path" : "fetch"}`); - failOpenDocs(isMissingDocsResource(error) ? "path" : "fetch"); + // Only a transport failure gets the fetch diagnostics: `onlineAtStart` + // and `apiBaseOrigin` answer "was the visitor offline / is this build + // pointing at localhost", and neither means anything for a missing + // artifact, which is a 404 from a server we plainly reached. Tagging + // both would put two different faults under one `context` and defeat + // the filtering these tags exist for. + const missing = isMissingDocsResource(error); + if (missing) reportError(error, "docs-example-load:path"); + else withDocsFetchDiagnostics("docs-fetch", () => reportError(error, "docs-example-load:fetch")); + failOpenDocs(missing ? "path" : "fetch"); } }) .catch((error) => { - reportError(error, `docs-bucket-resolve:${isMissingDocsResource(error) ? "bucket" : "fetch"}`); - failOpenDocs(isMissingDocsResource(error) ? "bucket" : "fetch"); + // Same split as the example-load catch above: diagnostics only on the + // transport sub-case. + const missing = isMissingDocsResource(error); + if (missing) reportError(error, "docs-bucket-resolve:bucket"); + else withDocsFetchDiagnostics("docs-fetch", () => reportError(error, "docs-bucket-resolve:fetch")); + failOpenDocs(missing ? "bucket" : "fetch"); }); return () => { cancelled = true; }; }, [initialDocs, loadWorkspace, nextVersion, route.mode, version, versionsResolved]); @@ -2095,6 +2212,11 @@ function Authoring({ ); const changeVersion = useCallback((next: string) => { + // DEV-2859: a version switch never reaches `onEdit` — it re-pins the file + // set (elsewhere, via `pinHandsontableFiles` + `setFiles`) rather than + // editing it — so it needs its own trail entry, tagged by the requested + // version rather than a file path. + recordEditorEvent({ kind: "version", source: "repin", path: next, quiet: false, size: 0 }); docsRequestSeqRef.current += 1; setVersionWarning(null); setThemeRemoved(false); @@ -2294,7 +2416,12 @@ function Authoring({ // workspace itself is updated the same way regardless: `filesRef` is what Save, // Download, Share and the next example switch read, so nothing may ever sit // between an edit and this assignment. - const onEdit = useCallback( + // + // Split from `onEdit`/`onEditFromStyle`/`onEditFromChat` below (DEV-2859): the + // trail entry has to be recorded once, tagged with which surface actually + // triggered the write, not by this shared writer — which is why the + // recording lives at each App-owned callsite and this one stays undecorated. + const writeFile = useCallback( (path: string, contents: string, opts?: WriteFileOptions) => { const next = { ...filesRef.current, [path]: contents }; filesRef.current = next; @@ -2313,6 +2440,62 @@ function Authoring({ [markDirty, showSyncing], ); + /** Bound straight to `EditorShell`'s `onEdit` — a keystroke in the code + * editor itself. DEV-2859 (Sentry DEMOS-1D): recorded into the bounded + * editor trail (`editorTrail.ts`) so a render-loop crash's event carries + * what was happening just before it, without the rejected + * one-breadcrumb-per-edit design (see that module's header). */ + const onEdit = useCallback( + (path: string, contents: string, opts?: WriteFileOptions) => { + recordEditorEvent({ + kind: "edit", + source: "editor", + path, + quiet: Boolean(opts?.quiet), + size: contents.length, + }); + writeFile(path, contents, opts); + }, + [writeFile], + ); + + /** Passed to `StylePanel` as `applyEdit` instead of the bare `onEdit` + * (DEV-2859). `opts?.quiet` is *already* how StylePanel distinguishes its + * two callsites — `patchLive` writes quietly (`{ quiet: true }`), `reset` + * does not — so this wrapper reuses that existing signal rather than + * requiring a StylePanel.tsx change to carry a new one. */ + const onEditFromStyle = useCallback( + (path: string, contents: string, opts?: WriteFileOptions) => { + recordEditorEvent({ + kind: "edit", + source: opts?.quiet ? "style" : "style-reset", + path, + quiet: Boolean(opts?.quiet), + size: contents.length, + }); + writeFile(path, contents, opts); + }, + [writeFile], + ); + + /** Passed to `ChatPanel` as `applyEdit` instead of the bare `onEdit` + * (DEV-2859). Unlike StylePanel, Chat's `apply()`/`undo()` have no existing + * signal to tell them apart — `ChatPanelProps.applyEdit`'s third parameter + * is new, added for exactly this. */ + const onEditFromChat = useCallback( + (path: string, contents: string, isUndo?: boolean) => { + recordEditorEvent({ + kind: "edit", + source: isUndo ? "ai-undo" : "ai", + path, + quiet: false, + size: contents.length, + }); + writeFile(path, contents); + }, + [writeFile], + ); + /** Send a message into the running preview (DEV-2496: the Style panel's live theme * patch). Cross-origin on both tiers — the bundler's origin for Tier 1, the * container's for Tier 2 — which postMessage is fine with. */ @@ -2378,6 +2561,11 @@ function Authoring({ * that rebuild is a container round trip of several seconds, and it used to say so * when theme edits went out as ordinary writes. */ const flushQuietEdits = useCallback(() => { + // DEV-2859: the only caller today is StylePanel (a colour-drag's quiet + // writes, flushed once the drag ends), hence `source: "style"` — there is + // no generic "flush" source in the trail's vocabulary because nothing + // else calls this yet. + recordEditorEvent({ kind: "flush-quiet", source: "style", path: "", quiet: false, size: 0 }); try { runtimeRef.current?.flushQuiet?.(); showSyncing(); @@ -3045,7 +3233,7 @@ function Authoring({ token={getToken()} htVersion={version} getFiles={() => filesRef.current} - applyEdit={onEdit} + applyEdit={onEditFromStyle} postToPreview={postToPreview} onPreviewMessage={onPreviewMessage} flushQuietEdits={flushQuietEdits} @@ -3060,7 +3248,7 @@ function Authoring({ htVersion={version} docsPath={docsPath} getFiles={() => filesRef.current} - applyEdit={onEdit} + applyEdit={onEditFromChat} onClose={() => setChatOpen(false)} /> )} diff --git a/runner/apps/authoring/src/Chat.tsx b/runner/apps/authoring/src/Chat.tsx index 0249aa7d9..3024ab99f 100644 --- a/runner/apps/authoring/src/Chat.tsx +++ b/runner/apps/authoring/src/Chat.tsx @@ -51,8 +51,12 @@ export interface ChatPanelProps { docsPath: string | null; /** Read the editor's live files at send time, not at mount time. */ getFiles: () => FilesMap; - /** Write a file back into the editor + running preview. */ - applyEdit: (path: string, contents: string) => void; + /** Write a file back into the editor + running preview. `isUndo` is a pure + * discriminator for the caller's own instrumentation (DEV-2859) — an Apply + * and an Undo call this with the same two-argument shape otherwise, and the + * App-owned wrapper needs a way to tell them apart without Chat.tsx + * importing anything Sentry-related itself. */ + applyEdit: (path: string, contents: string, isUndo?: boolean) => void; onClose: () => void; } @@ -213,7 +217,7 @@ export function ChatPanel({ function undo(index: number) { const turn = turnsRef.current[index]; if (!turn?.undo) return; - for (const [path, contents] of Object.entries(turn.undo)) applyEdit(path, contents); + for (const [path, contents] of Object.entries(turn.undo)) applyEdit(path, contents, true); setTurns((current) => current.map((t, i) => (i === index ? { ...t, undo: undefined } : t))); reportChatEvent(apiBase, "edit_undone", framework); } diff --git a/runner/apps/authoring/src/auth.ts b/runner/apps/authoring/src/auth.ts index 809e72dd8..b024bcb4b 100644 --- a/runner/apps/authoring/src/auth.ts +++ b/runner/apps/authoring/src/auth.ts @@ -10,6 +10,7 @@ // and the admin writes on the server side. import { reportError } from "./sentry.js"; +import { applyUserContext, resetUserContext } from "./userScope.js"; const BROKER = import.meta.env.VITE_LOGIN_BROKER_URL || "https://mcp-auth-proxy-j0tb.onrender.com"; const API_BASE = import.meta.env.VITE_API_BASE || "http://localhost:8787"; @@ -47,7 +48,10 @@ export function isTokenSession(): boolean { export async function currentUser(): Promise { // Local dev bypass — set VITE_DEV_USER in .env.local; never set in production. const devUser = import.meta.env.VITE_DEV_USER as string | undefined; - if (devUser) return { email: devUser }; + if (devUser) { + void applyUserContext({ email: devUser }, { devUser, token: getToken() }); + return { email: devUser }; + } const hash = new URLSearchParams(location.hash.slice(1)); if (hash.get("error")) { @@ -76,9 +80,12 @@ export async function currentUser(): Promise { }); if (!res.ok) { sessionStorage.removeItem(TOKEN_KEY); + resetUserContext(); return null; } - return (await res.json()) as User; + const resolved = (await res.json()) as User; + void applyUserContext(resolved, { token }); + return resolved; } catch (error) { // The broker being unreachable presents as "signed out" with no explanation, // and every write endpoint then rejects. @@ -110,6 +117,7 @@ export function clearSession(): void { // network corrects it. Removed by name rather than `sessionStorage.clear()`: // this key is ours, the rest of the origin's storage is not. sessionStorage.removeItem(PROFILE_CACHE_KEY); + resetUserContext(); } /** diff --git a/runner/apps/authoring/src/catalog.ts b/runner/apps/authoring/src/catalog.ts index 04ab922c5..b7f7a593c 100644 --- a/runner/apps/authoring/src/catalog.ts +++ b/runner/apps/authoring/src/catalog.ts @@ -2,6 +2,7 @@ import { stableBucketVersions } from "@handsontable/demo-runtime"; import type { Catalog, CatalogIndexEntry } from "@handsontable/demo-runtime"; import catalogJson from "../../../catalog.json"; import docsBucketsJson from "../../../docs-buckets.json"; +import { fetchWithDiagnostics, type FetchDiagnostics } from "./fetchDiagnostics.js"; // The index only (~15 KB): framework rows without files. Full starter // artifacts are lazy-fetched per version bucket — see starter-catalog.ts. @@ -40,13 +41,24 @@ export const VERSION_OPTIONS = stableBucketVersions(catalog.bucketVersions); * "the visitor has not chosen" before swapping in npm `latest`. */ export const DEFAULT_VERSION = VERSION_OPTIONS[0]; -/** Fetch real published versions from the API (npm-backed). */ +/** + * Fetch real published versions from the API (npm-backed). + * + * A thin caller over `fetchWithDiagnostics` (DEV-2859): the retry + timeout + * policy and the attempt bookkeeping live there, import-free and unit-tested; + * this function stays exactly what it was for the one line that a Sentry + * population (DEMOS-2X) already groups on — `if (!res.ok) throw new Error(...)` + * is byte-identical, so that grouping does not move. The `diagnostics` on the + * success path let the caller (App.tsx) tell a retried-then-recovered blip + * from a clean first try, without this module importing Sentry. + */ export async function fetchVersions( apiBase: string, -): Promise<{ latest: string | null; next: string | null; versions: string[] }> { - const res = await fetch(`${apiBase}/api/versions`); +): Promise<{ latest: string | null; next: string | null; versions: string[]; diagnostics: FetchDiagnostics }> { + const { res, diagnostics } = await fetchWithDiagnostics(`${apiBase}/api/versions`); if (!res.ok) throw new Error(`versions ${res.status}`); - return (await res.json()) as { latest: string | null; next: string | null; versions: string[] }; + const body = (await res.json()) as { latest: string | null; next: string | null; versions: string[] }; + return { ...body, diagnostics }; } /** Is `version` an exact published Handsontable version on npm? Used to detect diff --git a/runner/apps/authoring/src/editorTrail.ts b/runner/apps/authoring/src/editorTrail.ts new file mode 100644 index 000000000..6978dde0e Binary files /dev/null and b/runner/apps/authoring/src/editorTrail.ts differ diff --git a/runner/apps/authoring/src/fetchDiagnostics.ts b/runner/apps/authoring/src/fetchDiagnostics.ts new file mode 100644 index 000000000..23db9f0fe --- /dev/null +++ b/runner/apps/authoring/src/fetchDiagnostics.ts @@ -0,0 +1,277 @@ +// Measurement + retry policy for `fetchVersions` / `fetchDocsJson` (DEV-2859, +// Sentry DEMOS-2X / DEMOS-7D). +// +// `fetchFailure.ts` (DEV-2858 / the sibling fix in +// fix/DEV-2859-opaque-fetch-host-suffix) only classifies a failure shape after +// the fact — it has no measurement and no remedy. This module adds both: a +// bounded retry so a transient blip doesn't become an issue, and a diagnostics +// bundle (attempts, outcome, elapsed, online) so a real dip is distinguishable +// from one visitor's dropped tab without either population being silenced. +// +// Import-free by construction, same reason and same constraint as +// `reportingGate.ts` / `fetchFailure.ts` / `sessionDiagnostics.ts` / +// `identity.ts`: `pipeline/fetch-diagnostics.test.mjs` imports this directly +// under `--experimental-strip-types`, which cannot resolve a sibling `./x.js` +// specifier (verified empirically). Every ambient dependency (fetch, the +// clock, `navigator.onLine`, the retry delay) is an injected parameter with a +// browser-real default, so node can drive every branch without a DOM. +// +// RETRY IS THE DISCRIMINATOR, not the classifier. A visitor's own network +// dropping mid-request (DEMOS-2X) and our host having a real dip both start +// the same way — a fetch that never completes — but only the first one +// recovers 300ms later. `versions_fetch_attempts` (attached to the thrown +// error, read at the callsite) is the tag that tells them apart: a dip fails +// twice, a blip does not. +// +// `!res.ok` (our host answering, e.g. a 503) is deliberately NOT retried here +// at all — retrying a real outage amplifies it, and the caller already reports +// that population as it always has (`catalog.ts`'s +// `if (!res.ok) throw new Error(...)` stays byte-identical). This module's +// retry only ever fires on a *thrown* fetch failure — a request that never +// produced a response. +// +// A timeout (the per-attempt AbortController, 5s, copied from +// `checkVersionExists` in catalog.ts) does NOT retry either: retrying a stall +// would either double a 5s wait into a 10s one on the version picker, or turn +// one outage into a retry storm across every open tab. It is its own outcome +// value (`"timeout"`) rather than being folded into `"transport"`. + +export interface FetchDiagnosticsDeps { + fetchFn?: typeof fetch; + /** A monotonic clock. `performance.now` in the browser; injectable so a test + * can control elapsed time without real timers. */ + now?: () => number; + isOnline?: () => boolean | undefined; + sleep?: (ms: number) => Promise; + /** Per-attempt abort budget. Defaults to 5000ms — the same figure + * `checkVersionExists` uses, for the same reason (see its own comment). */ + timeoutMs?: number; + /** Delay before the single retry. Defaults to 300ms. */ + retryDelayMs?: number; +} + +export type FetchOutcome = "ok" | "transport" | "timeout"; + +export interface FetchDiagnostics { + /** 1 (no retry attempted, or a timeout — which never retries) or 2 (a + * transport failure was retried once). The decisive tag: a dip fails at + * both attempts, a blip only fails the first. Always populated by + * `fetchWithDiagnostics`; optional only because `docs-catalog.ts`'s + * `fetchDocsJson` has no retry of its own to report (it cannot import this + * module and stay node-importable — see this file's header) and still + * reuses `diagnosticTags`/`diagnosticExtras` with a thinner bundle + * (DEV-2859's DEMOS-7D ruling). */ + attempts?: number; + outcome?: FetchOutcome; + /** `navigator.onLine` at the start of the call. `undefined` outside a + * browser. `true` proves nothing (an extension or captive portal still + * refuses the request); only an explicit `false` is evidence either way — + * same caution `tier1Report.ts` documents for the same signal. */ + onlineAtStart: boolean | undefined; + elapsedMs?: number; + /** Set by the caller (`context: "versions-fetch"` or `"docs-fetch"`) before + * tagging — this module has no opinion on which population it is instrumenting. */ + context?: string; + apiBaseOrigin?: "same" | "cross" | "localhost"; + /** Chromium-only (`navigator.connection.effectiveType`); omitted elsewhere. */ + netEffectiveType?: string; +} + +const DEFAULT_TIMEOUT_MS = 5000; +const DEFAULT_RETRY_DELAY_MS = 300; + +function defaultIsOnline(): boolean | undefined { + return typeof navigator !== "undefined" ? navigator.onLine : undefined; +} + +function defaultSleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** Attach diagnostics to a thrown error as a non-enumerable property, leaving + * `name`/`message` untouched — `isOpaqueNetworkFailure` (fetchFailure.ts) + * must still classify the underlying error by its own wording, and a + * non-enumerable property does not show up in `JSON.stringify`, a `for...in`, + * or Sentry's own error serialisation, so no grouping shifts. */ +function attachDiagnostics(error: unknown, diagnostics: FetchDiagnostics): unknown { + if (typeof error === "object" && error !== null) { + Object.defineProperty(error, "fetchDiagnostics", { + value: diagnostics, + enumerable: false, + configurable: true, + }); + } + return error; +} + +export function readFetchDiagnostics(error: unknown): FetchDiagnostics | undefined { + if (typeof error !== "object" || error === null) return undefined; + return (error as { fetchDiagnostics?: FetchDiagnostics }).fetchDiagnostics; +} + +interface AttemptResult { + res?: Response; + error?: unknown; + timedOut: boolean; +} + +async function attemptOnce( + fetchFn: typeof fetch, + url: string, + init: RequestInit | undefined, + timeoutMs: number, +): Promise { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { + const res = await fetchFn(url, { ...init, signal: controller.signal }); + return { res, timedOut: false }; + } catch (error) { + return { error, timedOut: controller.signal.aborted }; + } finally { + clearTimeout(timer); + } +} + +/** + * Fetch with the DEV-2859 retry policy. Resolves with `{ res, diagnostics }` + * whenever a response was produced (`res.ok` may still be false — this + * function has no opinion on HTTP status, only on whether the request + * completed at all). Throws the terminal error, decorated with diagnostics, + * when it never does. + */ +export async function fetchWithDiagnostics( + url: string, + init?: RequestInit, + deps: FetchDiagnosticsDeps = {}, +): Promise<{ res: Response; diagnostics: FetchDiagnostics }> { + const fetchFn = deps.fetchFn ?? fetch; + const now = deps.now ?? (() => performance.now()); + const isOnline = deps.isOnline ?? defaultIsOnline; + const sleep = deps.sleep ?? defaultSleep; + const timeoutMs = deps.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const retryDelayMs = deps.retryDelayMs ?? DEFAULT_RETRY_DELAY_MS; + + const onlineAtStart = isOnline(); + const start = now(); + + const first = await attemptOnce(fetchFn, url, init, timeoutMs); + if (first.res) { + return { + res: first.res, + diagnostics: { attempts: 1, outcome: "ok", onlineAtStart, elapsedMs: now() - start }, + }; + } + if (first.timedOut) { + // A timeout never retries — see this file's header. + throw attachDiagnostics(first.error, { + attempts: 1, + outcome: "timeout", + onlineAtStart, + elapsedMs: now() - start, + }); + } + + // A transport failure (the request never completed, and it wasn't our own + // abort) gets exactly one retry. + await sleep(retryDelayMs); + const second = await attemptOnce(fetchFn, url, init, timeoutMs); + if (second.res) { + return { + res: second.res, + diagnostics: { attempts: 2, outcome: "ok", onlineAtStart, elapsedMs: now() - start }, + }; + } + throw attachDiagnostics(second.error, { + attempts: 2, + outcome: second.timedOut ? "timeout" : "transport", + onlineAtStart, + elapsedMs: now() - start, + }); +} + +// --- Buckets, origin classification, tags/extras --------------------------- + +/** Fetch-scale elapsed buckets — distinct boundaries from + * `sessionDiagnostics.ts`'s `elapsedBucket`, which is scaled for a Tier-2 + * container boot (seconds to ~120s), not a single HTTP round trip. */ +const FETCH_BOUNDARIES_MS = [100, 500, 1000, 3000, 5000]; + +export function elapsedBucket(ms: number): string { + for (const boundary of FETCH_BOUNDARIES_MS) { + if (ms < boundary) { + return boundary >= 1000 ? `<${boundary / 1000}s` : `<${boundary}ms`; + } + } + return ">=5s"; +} + +/** Is the API base same-origin, cross-origin, or the committed localhost + * fallback (`catalog.ts` / `auth.ts`'s `API_BASE`)? A `localhost` result in + * production means `VITE_API_BASE` never made it into the build — a single + * candidate cause that would fail the request for every visitor of that + * deploy, and would settle DEMOS-2X outright. */ +export function apiBaseOrigin(apiBase: string, pageOrigin: string): "same" | "cross" | "localhost" { + let apiHost: string; + try { + apiHost = new URL(apiBase, pageOrigin).host; + } catch { + return "cross"; + } + if (/^localhost(:\d+)?$/.test(apiHost) || /^127\.0\.0\.1(:\d+)?$/.test(apiHost)) return "localhost"; + let pageHost: string; + try { + pageHost = new URL(pageOrigin).host; + } catch { + return "cross"; + } + return apiHost === pageHost ? "same" : "cross"; +} + +/** `context` is required and drives the tag-name prefix, so the same + * diagnostics shape can tag both the `versions-fetch` (DEMOS-2X) and + * `docs-fetch` (DEMOS-7D) populations without one borrowing the other's tag + * names in the Sentry UI. */ +function prefixFor(diag: FetchDiagnostics): string { + return (diag.context ?? "fetch").replace(/-/g, "_"); +} + +export function diagnosticTags(diag: FetchDiagnostics): Record { + const prefix = prefixFor(diag); + const tags: Record = { + context: diag.context ?? "fetch", + }; + if (diag.attempts !== undefined) tags[`${prefix}_attempts`] = String(diag.attempts); + if (diag.outcome !== undefined) tags[`${prefix}_outcome`] = diag.outcome; + if (diag.elapsedMs !== undefined) tags[`${prefix}_elapsed_bucket`] = elapsedBucket(diag.elapsedMs); + if (diag.onlineAtStart !== undefined) tags[`${prefix}_online`] = String(diag.onlineAtStart); + if (diag.apiBaseOrigin) tags.api_base_origin = diag.apiBaseOrigin; + if (diag.netEffectiveType) tags.net_effective_type = diag.netEffectiveType; + return tags; +} + +export function diagnosticExtras(diag: FetchDiagnostics): Record { + const extras: Record = {}; + if (diag.elapsedMs !== undefined) extras.elapsedMs = String(Math.round(diag.elapsedMs)); + if (diag.attempts !== undefined && diag.outcome !== undefined) { + extras.attemptSummary = `${diag.attempts} attempt${diag.attempts === 1 ? "" : "s"}, ${diag.outcome}`; + } + if (diag.apiBaseOrigin) { + // Host only — never a full URL, never a query string. `apiBaseOrigin`'s + // caller passes the classification, not the raw base, so there is nothing + // more specific to redact here; this extra exists so the classification + // shows up in the event body, not just as a filterable tag. + extras.apiBaseClass = diag.apiBaseOrigin; + } + return extras; +} + +/** Chromium-only `navigator.connection.effectiveType` (e.g. "4g", "3g"). + * `undefined` on every other engine, and the caller omits the tag entirely + * rather than sending `"undefined"`. */ +export function netEffectiveType(): string | undefined { + const nav = typeof navigator !== "undefined" ? (navigator as Navigator & { + connection?: { effectiveType?: string }; + }) : undefined; + return nav?.connection?.effectiveType; +} diff --git a/runner/apps/authoring/src/identity.ts b/runner/apps/authoring/src/identity.ts new file mode 100644 index 000000000..c673c2bb6 --- /dev/null +++ b/runner/apps/authoring/src/identity.ts @@ -0,0 +1,112 @@ +// Visitor / user identity decisions for Sentry context (DEV-2859). +// +// Import-free by construction, same reason and same constraint as +// `reportingGate.ts` / `fetchFailure.ts` / `sessionDiagnostics.ts`: this is the +// one piece of `pipeline/*.test.mjs` can import directly under +// `--experimental-strip-types`, because the Sentry-touching wiring +// (`userScope.ts`, `main.tsx`, `auth.ts`) imports `@sentry/react` and/or reads +// `import.meta.env`, neither of which node resolves. Do not let this file grow +// imports — a sibling `./x.js` specifier does not resolve under strip-types +// either (verified empirically against a throwaway probe file). +// +// WHY THIS EXISTS AT ALL: every `setUser` call in `apps/**` turns out to be +// React local state — nothing ever calls `Sentry.setUser`, `Sentry.setTag`, or +// sets `sendDefaultPii`. That is why every Sentry issue in this project reports +// `users: 0`, and why a "0 users" reading misled an earlier triage into +// suppressing a population that was, in fact, affecting real visitors. + +/** Storage the visitor id is minted into and read from. A narrow structural + * type (not `Storage` itself) so a test can hand in a throwing fake without + * implementing the full Web Storage interface. */ +export interface KeyValueStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; +} + +/** Same key `auth.ts` uses for the broker/API token — sessionStorage, not + * localStorage. Deliberately: localStorage would mint a new persistent + * pseudonymous identifier on a public site, which is a bigger privacy + * commitment than this instrumentation is trying to make. The cost of that + * choice is stated once, here, because it is easy to forget while reading a + * Sentry issue: a reload mints a new id, so `users` on an anonymous issue + * counts *sessions*, not people. It is a floor, not a headcount. */ +const VISITOR_ID_KEY = "hot_sid"; + +function randomId(): string { + // `crypto.randomUUID` is available in every browser this app supports and + // in Node 22 (used by the test only to prove the mint path is exercised). + return `s_${crypto.randomUUID().replace(/-/g, "").slice(0, 20)}`; +} + +/** + * Read the existing visitor id, or mint and persist one. + * + * Must tolerate a throwing storage without throwing itself — Safari private + * mode (and any browser with storage disabled by policy) throws on + * `sessionStorage.getItem`/`setItem`, and an identity helper crashing the app + * it exists to observe would be exactly backwards. A storage that throws + * degrades to "a fresh id every call", which is honest: nothing persisted, so + * nothing to read back next time either. + */ +export function visitorId(storage: KeyValueStorage): string { + try { + const existing = storage.getItem(VISITOR_ID_KEY); + if (existing) return existing; + const minted = randomId(); + storage.setItem(VISITOR_ID_KEY, minted); + return minted; + } catch { + return randomId(); + } +} + +/** + * `"u_" + sha256(lowercased, trimmed email).slice(0, 16)`, via `crypto.subtle` + * (available in every browser this app supports, and in Node >= 19 including + * the Node 22 this test suite runs under). + * + * Privacy, stated honestly rather than oversold: over a known internal + * address list (this is a Google-login-gated internal tool — see auth.ts) a + * truncated hash is reversible *by us*, e.g. by hashing every + * @handsontable.com address and comparing. The goal is "never ship a raw + * address to a third party" (Sentry), not anonymisation against someone who + * already has the address list. No `email`, no `username`, no raw address is + * ever set anywhere in this module or its callers. + */ +export async function hashedUserId(email: string): Promise { + const normalized = email.trim().toLowerCase(); + const bytes = new TextEncoder().encode(normalized); + const digest = await crypto.subtle.digest("SHA-256", bytes); + const hex = Array.from(new Uint8Array(digest)) + .map((b) => b.toString(16).padStart(2, "0")) + .join(""); + return `u_${hex.slice(0, 16)}`; +} + +export type AuthMode = "anonymous" | "google" | "api-token" | "dev-bypass"; + +export interface AuthModeInputs { + /** `import.meta.env.VITE_DEV_USER` — truthy means the local dev bypass in + * `auth.ts`'s `currentUser()` short-circuited. Passed in as a value, never + * read here, so this module stays import-free of `import.meta.env`. */ + devUser?: string; + /** The stored broker/API token (`getToken()` in auth.ts), or null/undefined + * when signed out. */ + token?: string | null; + /** The resolved user, when `currentUser()` succeeded. */ + user?: { email: string } | null; +} + +/** Mirrors `PAT_PREFIX` in auth.ts — passed in rather than imported, same + * reason every other input here is passed in. */ +export function authMode({ devUser, token, user }: AuthModeInputs, patPrefix: string): AuthMode { + // Checked first and independent of `user`: the dev bypass in auth.ts returns + // a `User` too (`{ email: devUser }`), so `auth_mode` would otherwise read + // "google" for a local dev session. This value doubles as a tripwire for the + // exact leak the `dist` grep in AGENTS.md exists to catch — a build that + // reports `dev-bypass` traffic in production means `VITE_DEV_USER` shipped. + if (devUser) return "dev-bypass"; + if (!user) return "anonymous"; + if (token?.startsWith(patPrefix)) return "api-token"; + return "google"; +} diff --git a/runner/apps/authoring/src/main.tsx b/runner/apps/authoring/src/main.tsx index 65bdb46e7..dd619d57b 100644 --- a/runner/apps/authoring/src/main.tsx +++ b/runner/apps/authoring/src/main.tsx @@ -1,6 +1,30 @@ // Must stay first: initialises error reporting before any other module runs, so a // throw during module evaluation is still captured. import { Sentry } from "./sentry.js"; +// Seeds an anonymous visitor id + `auth_mode` tag synchronously, right after +// reporting is initialised and before `createRoot` — so an early +// module-evaluation crash is already attributable to *a* visitor, not one of +// the `users: 0` issues that misled an earlier triage (DEV-2859). Deliberately +// not `currentUser()`: that round-trips the Render-hosted broker and would add +// its latency to every route, including ones that need no identity at all. +import { seedAnonymousContext } from "./userScope.js"; +seedAnonymousContext(); +// DEMOS-1D (DEV-2859): attach the bounded editor trail to every event this +// client sends, plus the tags derived from its most recent entry. Registered +// unconditionally — `Sentry.addEventProcessor` is a no-op when reporting is +// gated off (nothing is ever sent for the processor to run against) — so this +// import stays free of a second `reportingEnabled` check. +// +// `surface === "demo-runtime"` events are skipped: those are relayed preview +// failures (`reportDemoEvent` in sentry.ts), and this app's own editor +// activity says nothing about what a demo's own code just did. +import { snapshotEditorTrail, editorTrailTags } from "./editorTrail.js"; +Sentry.addEventProcessor((event) => { + if (event.tags?.surface === "demo-runtime") return event; + event.tags = { ...event.tags, ...editorTrailTags() }; + event.extra = { ...event.extra, editorTrail: snapshotEditorTrail() }; + return event; +}); // The code face the design specifies (Figma 48:6719 / 31:6597) — bundled, not a // CDN link, so the editor never renders a fallback face first. Loaded here, not // in the shell: editor-shell stays a side-effect-free source package, and the diff --git a/runner/apps/authoring/src/userScope.ts b/runner/apps/authoring/src/userScope.ts new file mode 100644 index 000000000..78a0861c9 --- /dev/null +++ b/runner/apps/authoring/src/userScope.ts @@ -0,0 +1,99 @@ +// Sentry user-context wiring (DEV-2859). Deliberately trivial and +// decision-free — the decisions (what id to mint, what auth_mode is, how to +// hash an email) live in `identity.ts`, which is import-free and unit-tested. +// This module cannot be: it imports `@sentry/react` via `./sentry.js`, which +// `node --test` cannot resolve (same reason as `sentry.ts` itself). +// +// Wrapped in `reportingEnabled` throughout: off-host (local dev, CI, a +// Playwright run against production — see `reportingGate.ts`) this must write +// no storage key and call no Sentry API, the same guarantee the rest of the +// reporting surface makes. + +import { Sentry, reportingEnabled } from "./sentry.js"; +import { authMode, hashedUserId, visitorId, type AuthModeInputs } from "./identity.js"; + +/** Mirrors `PAT_PREFIX` in auth.ts (see identity.ts's `authMode`). Duplicated + * rather than imported: auth.ts already imports this module's sibling + * `sentry.ts`, and a cycle back into auth.ts is not worth avoiding a second + * literal. */ +const PAT_PREFIX = "hot_pat_"; + +/** + * `identity.ts`'s `visitorId` catches a throwing storage's *method calls* + * (`getItem`/`setItem`) — but in a locked-down environment (Safari with all + * site storage denied by policy) merely reading the global `sessionStorage` + * accessor can itself throw, at the argument-evaluation site, before + * `visitorId` ever gets a value to call a method on. This is the one path in + * this whole feature that could white-screen the app it exists to observe, so + * the access is guarded here rather than assumed safe. */ +function safeSessionStorage(): Storage | { getItem(): null; setItem(): void } { + try { + return sessionStorage; + } catch { + return { getItem: () => null, setItem: () => {} }; + } +} + +/** + * Seed anonymous context. Called synchronously from `main.tsx`, before + * `createRoot`, so a crash while the module graph is still evaluating is + * already attributable to a visitor id. Never calls `currentUser()` — that + * would round-trip the Render-hosted broker and add its latency to every + * route, including the ones that need no identity at all. + */ +export function seedAnonymousContext(): void { + if (!reportingEnabled) return; + const id = visitorId(safeSessionStorage()); + Sentry.setUser({ id }); + Sentry.setTag("auth_mode", "anonymous"); +} + +export interface ApplyUserContextInputs { + /** `import.meta.env.VITE_DEV_USER`, forwarded from the caller so this module + * stays free of `import.meta.env` reads (it already imports the SDK, so + * that constraint is about keeping the *set of things* that changes small, + * not about testability — this file was never going to be node-importable). */ + devUser?: string; + token?: string | null; +} + +/** + * Upgrade (or re-seed) the user context once identity is known: the + * dev-bypass early return and the resolved-user return in `auth.ts`'s + * `currentUser()`. + * + * Never sets `email`, `username`, or any raw address — only the hash from + * `identity.ts`. `sendDefaultPii` stays unset in `sentry.ts`; this function is + * the only place a `User`'s identity reaches Sentry at all, and it never + * reaches for the field that would leak it. + */ +export async function applyUserContext( + user: { email: string } | null, + { devUser, token }: ApplyUserContextInputs, +): Promise { + if (!reportingEnabled) return; + const mode = authMode({ devUser, token, user } satisfies AuthModeInputs, PAT_PREFIX); + if (!user) { + resetUserContext(); + return; + } + // The dev-bypass email is hashed like any other — see `authMode`'s note on + // why `auth_mode` itself is what makes a `dev-bypass` leak visible, not a + // special case here. + const id = await hashedUserId(user.email); + Sentry.setUser({ id }); + Sentry.setTag("auth_mode", mode); +} + +/** + * Drop back to an anonymous identity: the 401 path in `currentUser()` and + * `clearSession()`. Re-mints from `visitorId` rather than clearing the user + * entirely, so a signed-out session still carries *some* identity instead of + * reverting to the pre-DEV-2859 "no user context at all" state. + */ +export function resetUserContext(): void { + if (!reportingEnabled) return; + const id = visitorId(safeSessionStorage()); + Sentry.setUser({ id }); + Sentry.setTag("auth_mode", "anonymous"); +} diff --git a/runner/pipeline/docs-catalog.test.mjs b/runner/pipeline/docs-catalog.test.mjs index b519203df..a3795d84c 100644 --- a/runner/pipeline/docs-catalog.test.mjs +++ b/runner/pipeline/docs-catalog.test.mjs @@ -102,6 +102,29 @@ test("manifest: a 500 rejects but is NOT missing — a transient failure stays t assert.match(error.message, /500/); }); +// DEV-2859: `fetchDocsJson` was deliberately left untouched by that task — it +// cannot import `fetchDiagnostics.ts` (a sibling `./x.js` specifier does not +// resolve under `--experimental-strip-types`, which is exactly what keeps +// this file able to import `docs-catalog.ts` at all) — so there is no retry +// and no diagnostics bundle here, only the DEMOS-7D tagging added at the +// App.tsx callsite. This guard pins that the one thing that *could* have +// silently changed — how a raw transport failure (the fetch call itself +// rejecting, never producing a Response) is classified — did not: it must +// stay a plain, non-missing rejection, distinct from the SPA-fallback case +// above. Fails if `fetchDocsJson` ever grows a `catch` that folds a thrown +// fetch error into `DocsResourceMissingError`. +test("manifest: a transport failure (fetch itself rejects) is NOT a missing bucket", async (t) => { + const original = globalThis.fetch; + globalThis.fetch = async () => { throw new TypeError("Failed to fetch"); }; + t.after(() => { globalThis.fetch = original; }); + + const error = await rejection(fetchDocsManifest("transport-failure-bucket")); + + assert.equal(isDocsResourceMissing(error), false); + assert.equal(error.name, "TypeError"); + assert.equal(error.message, "Failed to fetch"); +}); + /** The JSON fast path must consume the body with `res.json()`, never `res.text()` * + `JSON.parse`: the release manifest is ~800 KB and the whole point of testing * content-type before sniffing for `<` is to avoid that extra JS-side string copy. diff --git a/runner/pipeline/editor-trail.test.mjs b/runner/pipeline/editor-trail.test.mjs new file mode 100644 index 000000000..444fb4de0 --- /dev/null +++ b/runner/pipeline/editor-trail.test.mjs @@ -0,0 +1,195 @@ +// The DEMOS-1D editor trail (DEV-2859): a bounded ring buffer with +// consecutive-identical coalescing, replacing a rejected one-breadcrumb-per- +// `onEdit` design that would have let a "Maximum update depth exceeded" loop +// evict the very context it exists to preserve. +// +// Imported straight from the .ts, the way `pipeline/session-diagnostics.test.mjs` +// imports `sessionDiagnostics.ts` — `editorTrail.ts` must stay import-free for +// the same reason (a sibling `./x.js` specifier does not resolve under +// `--experimental-strip-types`, verified empirically against a throwaway probe +// file). + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + recordEditorEvent, + snapshotEditorTrail, + editorTrailTags, + resetEditorTrail, + __setClockForTest, +} from "../apps/authoring/src/editorTrail.ts"; + +function useFakeClock(t, step = 1) { + let now = 0; + __setClockForTest(() => { now += step; return now; }); + t.after(() => __setClockForTest(null)); +} + +test.beforeEach(() => resetEditorTrail()); + +test("a plain edit is recorded with an incrementing seq", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "editor", path: "/index.js", quiet: false, size: 10 }); + recordEditorEvent({ kind: "edit", source: "editor", path: "/other.js", quiet: false, size: 12 }); + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 2); + assert.ok(snap[1].seq > snap[0].seq); +}); + +test("same path, same signature: two writes coalesce (size does not break coalescing)", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "editor", path: "/index.js", quiet: false, size: 10 }); + recordEditorEvent({ kind: "edit", source: "editor", path: "/index.js", quiet: false, size: 12 }); + const snap = snapshotEditorTrail(); + // `size` is deliberately excluded from the coalescing signature (see + // editorTrail.ts's `signature`): a tight loop over a shrinking/growing + // buffer must still collapse into one slot, or the whole design would be + // defeated by the exact shape it exists to handle. + assert.equal(snap.length, 1); + assert.equal(snap[0].n, 2); + assert.equal(snap[0].size, 12); // the latest size wins +}); + +test("capacity + 5 distinct entries: the oldest is evicted", (t) => { + useFakeClock(t); + for (let i = 0; i < 29; i++) { + // A distinct signature every time (differing path), so nothing coalesces + // and this exercises the eviction path, not the coalescing one. + recordEditorEvent({ kind: "edit", source: "editor", path: `/file-${i}.js`, quiet: false, size: i }); + } + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 24); // CAPACITY + // The oldest 5 (file-0..file-4) are gone; the newest (file-28) survived. + assert.equal(snap.some((e) => e.path === "/file-0.js"), false); + assert.equal(snap.some((e) => e.path === "/file-4.js"), false); + assert.equal(snap[snap.length - 1].path, "/file-28.js"); + assert.equal(snap[0].path, "/file-5.js"); +}); + +test("3000 identical pushes coalesce into ONE entry with n === 3000, and the entry pushed before the loop survives", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "workspace", source: "load", path: "docs", quiet: false, size: 0 }); + for (let i = 0; i < 3000; i++) { + recordEditorEvent({ kind: "edit", source: "editor", path: "/App.jsx", quiet: false, size: 42 }); + } + const snap = snapshotEditorTrail(); + // This is the whole design claim: a 3000-iteration loop must occupy exactly + // one slot, and the entry from before the loop must still be readable — + // with the rejected per-edit-breadcrumb design (maxBreadcrumbs: 200), both + // of these would be false: the loop alone would have overflowed 200 distinct + // slots and evicted the "load" entry long before this assertion. + assert.equal(snap.length, 2); + assert.equal(snap[0].kind, "workspace"); + assert.equal(snap[0].source, "load"); + const loopEntry = snap[1]; + assert.equal(loopEntry.kind, "edit"); + assert.equal(loopEntry.source, "editor"); + assert.equal(loopEntry.n, 3000); +}); + +test("a signature change breaks coalescing into two entries", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "editor", path: "/a.js", quiet: false, size: 1 }); + recordEditorEvent({ kind: "edit", source: "editor", path: "/a.js", quiet: false, size: 2 }); + recordEditorEvent({ kind: "edit", source: "style", path: "/a.js", quiet: false, size: 3 }); // source differs + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 2); + assert.equal(snap[0].n, 2); + assert.equal(snap[0].source, "editor"); + assert.equal(snap[1].n, 1); + assert.equal(snap[1].source, "style"); +}); + +test("quiet is part of the coalescing signature (a style patch vs. a style reset)", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "style", path: "/theme.js", quiet: true, size: 5 }); + recordEditorEvent({ kind: "edit", source: "style", path: "/theme.js", quiet: false, size: 5 }); + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 2); +}); + +test("size is length, not contents — the real privacy assertion", (t) => { + useFakeClock(t); + const sentinel = "SECRET_VISITOR_SOURCE_MARKER_do_not_leak_this"; + // A caller that (incorrectly) still passes `contents` alongside `size` must + // not be able to get it into the trail — this module never reads or spreads + // an unknown property from its input. + recordEditorEvent({ + kind: "edit", + source: "editor", + path: "/index.js", + quiet: false, + size: sentinel.length, + contents: sentinel, + }); + const snap = snapshotEditorTrail(); + assert.equal(JSON.stringify(snap).includes(sentinel), false); + assert.equal(snap[snap.length - 1].size, sentinel.length); +}); + +test("path is capped", (t) => { + useFakeClock(t); + const longPath = "/" + "a".repeat(500) + ".js"; + recordEditorEvent({ kind: "edit", source: "editor", path: longPath, quiet: false, size: 1 }); + const snap = snapshotEditorTrail(); + assert.ok(snap[0].path.length <= 121); // 120 + the "…" marker +}); + +// --- editorTrailTags ------------------------------------------------------- + +test("editorTrailTags reads the most recent entry", (t) => { + useFakeClock(t); + assert.deepEqual(editorTrailTags(), {}); + recordEditorEvent({ kind: "edit", source: "ai", path: "/a.js", quiet: false, size: 1 }); + for (let i = 0; i < 50; i++) { + recordEditorEvent({ kind: "edit", source: "ai", path: "/a.js", quiet: false, size: 1 }); + } + const tags = editorTrailTags(); + assert.equal(tags.editor_loop_source, "ai"); + assert.equal(tags.editor_last_kind, "edit"); + assert.equal(tags.editor_loop_n_bucket, "<100"); +}); + +test("editorTrailTags skips a trailing flush-quiet entry (a Style-panel drag ends with one)", (t) => { + useFakeClock(t); + // A colour drag: ~200 coalesced quiet writes, then the flush that always + // follows one (App.tsx's `flushQuietEdits`). Without the fix, reading the + // bare last entry would report the flush's own source ("style", n=1) + // instead of the coalesced drag underneath it — hiding the exact signal + // DEMOS-1D exists to read. + for (let i = 0; i < 200; i++) { + recordEditorEvent({ kind: "edit", source: "style", path: "/theme.js", quiet: true, size: 5 }); + } + recordEditorEvent({ kind: "flush-quiet", source: "style", path: "", quiet: false, size: 0 }); + const tags = editorTrailTags(); + assert.equal(tags.editor_loop_source, "style"); + assert.equal(tags.editor_last_kind, "edit"); // NOT "flush-quiet" + assert.equal(tags.editor_loop_n_bucket, "<1000"); // the 200-run's bucket, not the flush's n=1 +}); + +test("editorTrailTags falls back to the last entry when the trail is nothing but flushes", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "flush-quiet", source: "style", path: "", quiet: false, size: 0 }); + const tags = editorTrailTags(); + assert.equal(tags.editor_last_kind, "flush-quiet"); +}); + +test("editorTrailTags n_bucket boundaries", (t) => { + const cases = [ + [1, "<10"], + [9, "<10"], + [10, "<100"], + [99, "<100"], + [100, "<1000"], + [999, "<1000"], + [1000, ">=1000"], + ]; + for (const [n, expected] of cases) { + resetEditorTrail(); + useFakeClock(t); + for (let i = 0; i < n; i++) { + recordEditorEvent({ kind: "edit", source: "editor", path: "/a.js", quiet: false, size: 1 }); + } + assert.equal(editorTrailTags().editor_loop_n_bucket, expected, `n=${n}`); + } +}); diff --git a/runner/pipeline/fetch-diagnostics.test.mjs b/runner/pipeline/fetch-diagnostics.test.mjs new file mode 100644 index 000000000..2aff3eca3 --- /dev/null +++ b/runner/pipeline/fetch-diagnostics.test.mjs @@ -0,0 +1,265 @@ +// Retry policy, buckets, and tag/extra shaping for `fetchVersions` / +// `fetchDocsJson` (DEV-2859, Sentry DEMOS-2X / DEMOS-7D). +// +// The central claim under test: retry is the discriminator between a +// visitor's own network dropping mid-request (a blip, recovers on the second +// attempt) and a real host dip (fails at both attempts). `!res.ok` is +// deliberately never retried — the "exactly one call" tests below are the +// retry-storm guard for that. +// +// Every ambient dependency (fetch, the clock, `navigator.onLine`, the retry +// delay/timeout) is injected, so this drives every branch without a DOM and +// without real timers. + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + fetchWithDiagnostics, + readFetchDiagnostics, + elapsedBucket, + apiBaseOrigin, + diagnosticTags, + diagnosticExtras, +} from "../apps/authoring/src/fetchDiagnostics.ts"; + +function fakeClock(step = 10) { + let t = 0; + return () => { t += step; return t; }; +} + +const noSleep = () => Promise.resolve(); + +function okResponse(extra = {}) { + return { ok: true, status: 200, ...extra }; +} + +function notOkResponse(status) { + return { ok: false, status }; +} + +// --- retry policy ------------------------------------------------------ + +test("success on the first attempt: exactly one fetch call, outcome ok", async () => { + let calls = 0; + const fetchFn = async () => { calls += 1; return okResponse(); }; + const { res, diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }); + assert.equal(calls, 1); + assert.equal(res.ok, true); + assert.equal(diagnostics.attempts, 1); + assert.equal(diagnostics.outcome, "ok"); +}); + +test("a transport failure then a 200: two calls, outcome ok, the good response is returned", async () => { + let calls = 0; + const fetchFn = async () => { + calls += 1; + if (calls === 1) throw new TypeError("Failed to fetch"); + return okResponse({ marker: "second" }); + }; + const { res, diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }); + assert.equal(calls, 2); + assert.equal(res.marker, "second"); + assert.equal(diagnostics.attempts, 2); + assert.equal(diagnostics.outcome, "ok"); +}); + +test("both attempts fail (transport): throws with attempts 2, diagnostics attached", async () => { + let calls = 0; + const fetchFn = async () => { calls += 1; throw new TypeError("Failed to fetch"); }; + await assert.rejects( + fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }), + (error) => { + assert.equal(calls, 2); + const diag = readFetchDiagnostics(error); + assert.equal(diag.attempts, 2); + assert.equal(diag.outcome, "transport"); + // name/message untouched, so isOpaqueNetworkFailure (fetchFailure.ts) + // still classifies it — the whole point of a non-enumerable property. + assert.equal(error.name, "TypeError"); + assert.equal(error.message, "Failed to fetch"); + return true; + }, + ); +}); + +test("!res.ok (our host answering, e.g. 503): retry-storm guard — exactly one fetch call", async () => { + let calls = 0; + const fetchFn = async () => { calls += 1; return notOkResponse(503); }; + const { res, diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }); + // This function has no opinion on HTTP status: a 503 is a completed request, + // so outcome is "ok" and the caller (catalog.ts) decides what a non-2xx + // status means. What matters here is the call count: retrying an outage + // would amplify it. + assert.equal(calls, 1); + assert.equal(res.ok, false); + assert.equal(diagnostics.attempts, 1); +}); + +test("aborting the first attempt: outcome timeout, one call, no raw AbortError escapes undiagnosed", async () => { + let calls = 0; + const fetchFn = async (_url, init) => { + calls += 1; + return new Promise((_resolve, reject) => { + init.signal.addEventListener("abort", () => { + reject(new DOMException("The operation was aborted.", "AbortError")); + }); + }); + }; + await assert.rejects( + fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, timeoutMs: 1, + }), + (error) => { + assert.equal(calls, 1); // a timeout never retries + const diag = readFetchDiagnostics(error); + // The caller (App.tsx) is instructed to branch on diagnostics.outcome, + // never on re-classifying the error itself — this is what makes that + // possible: the outcome is readable without inspecting error.name. + assert.equal(diag.outcome, "timeout"); + assert.equal(diag.attempts, 1); + return true; + }, + ); +}); + +test("onlineAtStart is read once, at the start of the call", async () => { + const fetchFn = async () => okResponse(); + const { diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => false, sleep: noSleep, + }); + assert.equal(diagnostics.onlineAtStart, false); +}); + +// --- apiBaseOrigin ----------------------------------------------------- + +test("apiBaseOrigin: the committed localhost fallback reads as localhost", () => { + assert.equal(apiBaseOrigin("http://localhost:8787", "https://demos.handsontable.com"), "localhost"); +}); + +test("apiBaseOrigin: same host as the page is same", () => { + assert.equal( + apiBaseOrigin("https://demos.handsontable.com/api", "https://demos.handsontable.com"), + "same", + ); +}); + +test("apiBaseOrigin: a different host is cross", () => { + assert.equal(apiBaseOrigin("https://api.example.com", "https://demos.handsontable.com"), "cross"); +}); + +// --- elapsedBucket ------------------------------------------------------- + +test("elapsedBucket: every boundary is pinned from both sides", () => { + const pairs = [ + [0, "<100ms"], + [99, "<100ms"], + [100, "<500ms"], + [499, "<500ms"], + [500, "<1s"], + [999, "<1s"], + [1000, "<3s"], + [2999, "<3s"], + [3000, "<5s"], + [4999, "<5s"], + [5000, ">=5s"], + [50_000, ">=5s"], + ]; + for (const [ms, expected] of pairs) { + assert.equal(elapsedBucket(ms), expected, `elapsedBucket(${ms})`); + } +}); + +// --- diagnosticTags / diagnosticExtras ----------------------------------- + +test("diagnosticTags: prefixes tag names from context, versions-fetch", () => { + const tags = diagnosticTags({ + context: "versions-fetch", + attempts: 2, + outcome: "transport", + onlineAtStart: true, + elapsedMs: 6000, + apiBaseOrigin: "localhost", + }); + assert.equal(tags.context, "versions-fetch"); + assert.equal(tags.versions_fetch_attempts, "2"); + assert.equal(tags.versions_fetch_outcome, "transport"); + assert.equal(tags.versions_fetch_elapsed_bucket, ">=5s"); + assert.equal(tags.versions_fetch_online, "true"); + assert.equal(tags.api_base_origin, "localhost"); +}); + +test("diagnosticTags: prefixes tag names from context, docs-fetch (DEMOS-7D)", () => { + const tags = diagnosticTags({ + context: "docs-fetch", + attempts: 1, + outcome: "ok", + onlineAtStart: undefined, + elapsedMs: 50, + }); + assert.equal(tags.docs_fetch_attempts, "1"); + assert.equal(tags.docs_fetch_outcome, "ok"); + assert.equal("docs_fetch_online" in tags, false); // omitted when unknown +}); + +test("diagnosticExtras: host-class only, never a raw URL", () => { + const extras = diagnosticExtras({ + context: "versions-fetch", + attempts: 2, + outcome: "transport", + onlineAtStart: true, + elapsedMs: 1234, + apiBaseOrigin: "same", + }); + assert.equal(extras.elapsedMs, "1234"); + assert.match(extras.attemptSummary, /2 attempts, transport/); + assert.equal(extras.apiBaseClass, "same"); + for (const value of Object.values(extras)) { + assert.equal(String(value).includes("http"), false); + assert.equal(String(value).includes("?"), false); + } +}); + +test("the attached fetchDiagnostics property is non-enumerable", async () => { + // Load-bearing, and previously only inferred from `name`/`message` staying + // intact. If this property ever became enumerable it would surface in + // `JSON.stringify(error)` and in any spread of the error, changing what + // downstream reporting sees — and an enumerable own property is the kind of + // thing a refactor flips without noticing. Pinned directly. + let caught; + let fetchCalls = 0; + try { + // Mocks go in the THIRD argument (`deps`), and the keys are `fetchFn` / + // `isOnline`. Passed as the second argument (`init`) with the wrong names, + // this ran the REAL fetch against example.test with a real 300ms sleep and a + // real 5s abort — and still passed, because a genuine network failure also + // throws with diagnostics attached. It asserted the invariant for the wrong + // reason and could stall or flake. Caught by Bugbot on PR #329. + await fetchWithDiagnostics("https://x/api/versions", undefined, { + fetchFn: () => { fetchCalls += 1; return Promise.reject(new TypeError("Failed to fetch")); }, + now: fakeClock(), + isOnline: () => true, + sleep: noSleep, + }); + } catch (error) { + caught = error; + } + // Proves the injected mock was actually used rather than the real `fetch`: + // two attempts means the retry ran through `fetchFn`. Without this the test + // could pass off a genuine network failure, which is how it shipped broken. + assert.equal(fetchCalls, 2); + assert.ok(caught, "expected the exhausted retry to throw"); + const descriptor = Object.getOwnPropertyDescriptor(caught, "fetchDiagnostics"); + assert.ok(descriptor, "fetchDiagnostics should be an own property"); + assert.equal(descriptor.enumerable, false); + assert.equal("fetchDiagnostics" in JSON.parse(JSON.stringify({ ...caught })), false); + // And the accessor still reads it, so non-enumerable does not mean unreachable. + assert.ok(readFetchDiagnostics(caught)); +}); diff --git a/runner/pipeline/identity.test.mjs b/runner/pipeline/identity.test.mjs new file mode 100644 index 000000000..0a1c7d312 --- /dev/null +++ b/runner/pipeline/identity.test.mjs @@ -0,0 +1,114 @@ +// Sentry identity decisions (DEV-2859): visitor ids, the signed-in id hash, and +// `auth_mode`. Every Sentry issue in this project reports `users: 0` because +// nothing ever calls `Sentry.setUser` — see `identity.ts`'s header. These tests +// pin the two things that made "0 users" a misleading reading: the id is a +// session-scoped floor, not a headcount, and the hash never leaks a raw address. +// +// Imported straight from the .ts, the way `pipeline/sentry-gating.test.mjs` +// imports `reportingGate.ts` — the root `test` script runs node with +// `--experimental-strip-types`, which cannot resolve a sibling `./x.js` +// specifier (verified empirically against a throwaway probe file), so +// `identity.ts` must stay import-free. + +import test from "node:test"; +import assert from "node:assert/strict"; +import { authMode, hashedUserId, visitorId } from "../apps/authoring/src/identity.ts"; + +const PAT_PREFIX = "hot_pat_"; + +// --- visitorId --------------------------------------------------------- + +/** A minimal in-memory stand-in for sessionStorage. */ +function memoryStorage(initial = {}) { + const store = new Map(Object.entries(initial)); + return { + getItem: (key) => (store.has(key) ? store.get(key) : null), + setItem: (key, value) => store.set(key, value), + _store: store, + }; +} + +test("visitorId mints once and returns the same id on a second call", () => { + const storage = memoryStorage(); + const first = visitorId(storage); + const second = visitorId(storage); + assert.equal(first, second); + assert.match(first, /^s_/); +}); + +test("visitorId survives a throwing storage (Safari private mode) without throwing", () => { + const throwing = { + getItem() { throw new DOMException("denied"); }, + setItem() { throw new DOMException("denied"); }, + }; + // Fails with the change reverted only if a real implementation ever forgets + // the try/catch — this is a guard against regressing that, not a behavioural + // claim about what id comes out (a throwing storage can't persist one). + assert.doesNotThrow(() => visitorId(throwing)); + assert.match(visitorId(throwing), /^s_/); +}); + +test("visitorId reads back what a prior mint wrote (same storage instance)", () => { + const storage = memoryStorage(); + const minted = visitorId(storage); + assert.equal(storage.getItem("hot_sid"), minted); +}); + +// --- hashedUserId -------------------------------------------------------- + +test("hashedUserId matches a known SHA-256 vector", async () => { + // sha256("foo@bar.com") = 0c7e6a405862e402... — pins the exact algorithm and + // truncation, so a swap to a different hash or a different slice length + // fails loudly instead of merely "still looking hash-shaped". + assert.equal(await hashedUserId("foo@bar.com"), "u_0c7e6a405862e402"); +}); + +test("hashedUserId output never contains the raw address or its local part", async () => { + // A real, non-hex-heavy local part: "artur.medrygal" contains letters + // (r, t, u, l, g, ...) that are not valid hex digits, so it cannot appear as + // a substring of a lowercase-hex string by chance — this is a real privacy + // assertion, not a coincidence-prone one. + const email = "artur.medrygal@handsontable.com"; + const id = await hashedUserId(email); + assert.equal(id.includes("@"), false); + assert.equal(id.toLowerCase().includes("artur.medrygal"), false); + assert.match(id, /^u_[0-9a-f]{16}$/); +}); + +test("hashedUserId normalises case and trailing whitespace", async () => { + const a = await hashedUserId("Foo@Bar.com"); + const b = await hashedUserId(" foo@bar.com "); + const c = await hashedUserId("foo@bar.com"); + assert.equal(a, c); + assert.equal(b, c); +}); + +// --- authMode -------------------------------------------------------------- + +test("authMode: dev bypass wins even when a resolved user is also present", () => { + // The dev-bypass early return in auth.ts's currentUser() returns a User too + // ({ email: devUser }) — this pins that devUser is checked first, or a local + // dev session would misreport as "google". + assert.equal( + authMode({ devUser: "dev@handsontable.com", token: null, user: { email: "dev@handsontable.com" } }, PAT_PREFIX), + "dev-bypass", + ); +}); + +test("authMode: no user at all is anonymous", () => { + assert.equal(authMode({ token: null, user: null }, PAT_PREFIX), "anonymous"); +}); + +test("authMode: a resolved user with a PAT-prefixed token is api-token", () => { + assert.equal( + authMode({ token: "hot_pat_abc123", user: { email: "a@b.com" } }, PAT_PREFIX), + "api-token", + ); +}); + +test("authMode: a resolved user with a non-PAT token is google", () => { + assert.equal( + authMode({ token: "some-broker-jwt", user: { email: "a@b.com" } }, PAT_PREFIX), + "google", + ); +});