diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index 5e8d80aa5..75e53ee51 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -71,6 +71,7 @@ import { monitorDemos, reportDemoEvent, reportError, reportingEnabled, Sentry } import { isMonitorPayload } from "@handsontable/demo-runtime/monitor"; import { tier1Report } from "./tier1Report.js"; import { isOpaqueNetworkFailure } from "./fetchFailure.js"; +import { describeDependencyFailure } from "./dependencyFailure.js"; import { readFetchDiagnostics, apiBaseOrigin, @@ -182,7 +183,12 @@ function docsPageUrl(framework: string, permalink: string): string { } /** Turn a raw runtime error into a message that explains container prerequisites. */ -function describeRuntimeError(e: unknown, engine: string, version: string): string { +function describeRuntimeError( + e: unknown, + engine: string, + version: string, + packageJson: string | undefined, +): string { // A boot-script failure explains itself: a one-line cause, with the recent boot // output beside it on the error object. Compose the two here — the error card // renders `errorMessage` and nothing else, so this is the only place a user ever @@ -217,12 +223,15 @@ function describeRuntimeError(e: unknown, engine: string, version: string): stri return "This example runs on the container engine, which needs the demo server (Cloudflare Sandbox). It isn't reachable here — run the local API worker (requires Docker) or open this example on the deployed demos.handsontable.com."; } // Sandpack's own bundler message for an unresolved dependency reads like a - // transient hiccup worth retrying ("please try again in a couple - // seconds") — misleading when the actual cause is a pinned Handsontable - // version that was never published, which no amount of retrying fixes. - if (engine === "sandpack" && /could not fetch dependencies/i.test(msg)) { - return `Handsontable ${version} could not be fetched. Check that this exact version is published on npm.`; - } + // transient hiccup worth retrying ("please try again in a couple seconds") — + // misleading when the actual cause is a pinned Handsontable version that was + // never published, which no amount of retrying fixes. But that same wording is + // ALSO what Sandpack says when the authored `/package.json` doesn't parse + // (DEV-2872, Sentry DEMOS-15/DEMOS-85) — see `dependencyFailure.ts` for how the + // two are told apart and why the discriminator is our own file state, not the + // bundler's text. + const dependencyFailure = describeDependencyFailure({ engine, message: msg, version, packageJson }); + if (dependencyFailure) return dependencyFailure; return msg; } @@ -2340,7 +2349,7 @@ function Authoring({ runtime.onError((e) => { if (cancelled) return; setStatus("error"); - setErrorMessage(describeRuntimeError(e, entry.engine, v.value.ref)); + setErrorMessage(describeRuntimeError(e, entry.engine, v.value.ref, filesRef.current["/package.json"])); reportRuntimeError(e, entry.engine, entry.framework); }); runtimeRef.current = runtime; @@ -2358,7 +2367,7 @@ function Authoring({ .catch((e: unknown) => { if (!cancelled) { setStatus("error"); - setErrorMessage(describeRuntimeError(e, entry.engine, v.value.ref)); + setErrorMessage(describeRuntimeError(e, entry.engine, v.value.ref, filesRef.current["/package.json"])); } // Reported even when cancelled: a session the pool refused still failed, // and the unmount that set `cancelled` is often the user giving up on it. diff --git a/runner/apps/authoring/src/dependencyFailure.ts b/runner/apps/authoring/src/dependencyFailure.ts new file mode 100644 index 000000000..645f290dd --- /dev/null +++ b/runner/apps/authoring/src/dependencyFailure.ts @@ -0,0 +1,113 @@ +/** + * Which of two causes put Sandpack's "could not fetch dependencies" message in front + * of a visitor (DEV-2872, Sentry DEMOS-15 / DEMOS-85). + * + * Split out of `App.tsx` for the same reason as `tier1Report.ts`, `fetchFailure.ts` + * and `sessionDiagnostics.ts`: that file pulls `@sentry/react` and reads + * `import.meta.env`, so `node --test` cannot import it, and nothing in it can be + * pinned by a unit test. Keep this module import-free — the discriminator below is + * the whole of what it decides, and `pipeline/dependency-failure.test.mjs` imports it + * as source under `--experimental-strip-types`. Do not let this file grow imports. + * + * WHY NOW. DEV-2855 guarded `buildSetup` so an unparseable `/package.json` no longer + * aborts `mount()`. That fix is correct and stays — before it, the preview died and + * every later keystroke was silently swallowed until "Restart preview". But it changed + * *which* message reaches `describeRuntimeError`: the mount now survives, the bundler + * fails trying to resolve dependencies out of a manifest it cannot parse, and that + * failure carries the exact same "could not fetch dependencies" wording Sandpack uses + * for an unpublished/unresolvable Handsontable version. Both populations now land on + * one branch in `App.tsx` that used to have only one cause. + * + * THE DISCRIMINATOR IS OUR OWN FILE STATE, NOT THIRD-PARTY TEXT. The two causes are + * told apart by whether the authored `/package.json` parses as JSON — not by matching + * on `Cannot read properties of null (reading 'match')`, which is Sandpack's bundler + * choking on a manifest with no resolvable `dependencies` object. That wording is + * undocumented upstream behaviour; keying on it would stop discriminating the moment + * the bundler is bumped and phrases its own internal failure differently. Reading our + * own file's parseability is stable regardless of how the bundler happens to fail on + * it. The branch *entry* — `/could not fetch dependencies/i` — still keys on Sandpack's + * wording, which is fine: that regex only gates onto our decision below and fails open + * (returns the bundler's own message, via `App.tsx`'s existing `return msg`) rather + * than fabricating a wrong answer. + * + * WHY THE PARSE DETAIL IS ASSERTED LOOSELY. `jsonSyntaxError` below hands back + * whatever `JSON.parse` throws for `.message`, verbatim. That wording is + * engine-specific — `fetchFailure.ts`'s header documents the same divergence for + * Chrome/Firefox/Safari network error strings — so a test (or this docblock) must not + * assert a particular V8 sentence; the shape (a non-empty detail string) is the + * contract, not the words. + * + * RESIDUAL, OUT OF SCOPE. A syntactically *valid* `/package.json` pinned to a bogus + * non-Handsontable dependency still fails the same way and still gets the npm + * sentence below — that manifest parses fine, so `jsonSyntaxError` returns `null` and + * this module falls through to the version-not-published wording. Fixing that would + * require keying on the bundler's own error text for "this specific package doesn't + * exist", which is exactly the third-party-text dependency this module exists to + * avoid. Left alone deliberately. + */ + +export interface DependencyFailureFacts { + /** Which preview engine raised the error. Only "sandpack" (Tier 1) is ever this + * module's branch — the container engine (Tier 2) has its own DEV-2538/DEV-2553 + * contract and must never be perturbed here. */ + engine: string; + /** `e.message`, exactly as the shell received it from the runtime — the bounded + * `show-error` text, not a Sentry-rendered cause chain (that tail is `Error`'s own + * `cause` formatting and never reaches this module). */ + message: string; + /** The pinned Handsontable version ref, for the npm sentence. */ + version: string; + /** The authored `/package.json` text at error time, or `undefined` if the file + * doesn't exist in this workspace. */ + packageJson: string | undefined; +} + +/** + * `JSON.parse(raw)` and report why it failed, or `null` if it parses. + * The message is whatever the engine's `JSON.parse` throws — see the docblock above + * for why that wording is never asserted verbatim. + */ +function jsonSyntaxError(raw: string): string | null { + try { + JSON.parse(raw); + return null; + } catch (e) { + return e instanceof Error ? e.message : String(e); + } +} + +/** + * Decide how a Sandpack "could not fetch dependencies" failure should be explained, + * or that this isn't that failure at all. + * + * `null` means "not my branch" — the caller (`describeRuntimeError` in `App.tsx`) + * falls through to its existing `return msg`, the bundler's own message verbatim. + */ +export function describeDependencyFailure(facts: DependencyFailureFacts): string | null { + if (facts.engine !== "sandpack") return null; + if (!/could not fetch dependencies/i.test(facts.message)) return null; + + if (facts.packageJson !== undefined) { + const detail = jsonSyntaxError(facts.packageJson); + if (detail) { + // The sentence deliberately stops at "fix the file" and does NOT promise + // that the preview recovers on its own. Our half of that is verified — + // `sandpack.ts`'s `emitReady()` fires on any bundler `done` without a + // compilation error, and the editor's `writeFile` is not gated on preview + // status, so a corrected manifest does reach the live runtime. What is NOT + // verifiable from this repo is whether Sandpack re-resolves dependencies + // from a changed `/package.json` mid-session or only at mount. Promising + // self-recovery on an unverified third-party behaviour would replace one + // false statement (DEV-2872's npm sentence) with another, which is the + // whole thing this module exists to stop. If someone confirms mid-session + // re-resolution against a live bundler, extend the sentence then. + return `/package.json is not valid JSON, so this demo's dependencies could not be installed: ${detail}. Fix the file to continue.`; + } + } + + // Either the manifest is undefined (deliberate status-quo preservation — DEV-2872 + // does not change this case) or it parses fine, in which case the real cause is + // Sandpack's own: an unresolvable/unpublished version. Byte-identical to the + // sentence `App.tsx` used before this module existed, so no behaviour changes here. + return `Handsontable ${facts.version} could not be fetched. Check that this exact version is published on npm.`; +} diff --git a/runner/pipeline/dependency-failure.test.mjs b/runner/pipeline/dependency-failure.test.mjs new file mode 100644 index 000000000..07bbc3e30 --- /dev/null +++ b/runner/pipeline/dependency-failure.test.mjs @@ -0,0 +1,122 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { describeDependencyFailure } from "../apps/authoring/src/dependencyFailure.ts"; + +// DEV-2872 / Sentry DEMOS-15 / DEMOS-85. DEV-2855 guarded `buildSetup` so an +// unparseable `/package.json` no longer aborts `mount()` — the preview survives, but +// the bundler then fails trying to resolve dependencies out of a manifest it can't +// read, and that failure carries the SAME "could not fetch dependencies" wording +// Sandpack uses for an unpublished/unresolvable Handsontable version. This module is +// what tells the two apart, by checking whether our own `/package.json` actually +// parses — not by matching the bundler's internal failure text. + +// FIXTURE CORRECTION (load-bearing): the ClickUp ticket quotes Sentry's +// `error.value` as +// "...(reading 'match'), Tier-1 compile failed" +// That trailing clause is NOT part of `e.message` as `describeRuntimeError` receives +// it. `App.tsx` reports the Tier-1 case as +// `new Error(report.synthesizeAs.message, { cause: e })`, so Sentry is rendering the +// cause chain — "Tier-1 compile failed" is `COMPILE_TITLE`, the synthesized error's +// own message, not the runtime error's. What actually reaches +// `describeRuntimeError`/`describeDependencyFailure` is the bounded `show-error` +// text below, WITHOUT that tail. The regex this module gates on matches either way, +// which is exactly why using the verbatim, untailed string matters here: a fixture +// with the tail baked in would still pass even if the module accidentally depended +// on text that never reaches it in production. +const MANIFEST_BUNDLER_MESSAGE = + "Could not fetch dependencies, please try again in a couple seconds: Cannot read properties of null (reading 'match')"; + +// The other real production wording for the actual unpublished-version case — pinned +// already by pipeline/sandpack-reload.test.mjs's +// "DEV-2550: the dependency-fetch message reaches describeRuntimeError intact" test. +const VERSION_BUNDLER_MESSAGE = + "Could not fetch dependencies, please try again in a couple seconds: request to https://registry.npmjs.org/handsontable failed"; + +// A mid-keystroke broken manifest: a trailing comma inside `dependencies`, exactly +// the shape a visitor produces while still typing. +const BROKEN_PACKAGE_JSON = `{ + "name": "demo", + "dependencies": { + "handsontable": "14.0.0", + } +} +`; + +const VALID_PACKAGE_JSON = `{ + "name": "demo", + "dependencies": { + "handsontable": "99.0.0" + } +} +`; + +const facts = (over = {}) => ({ + engine: "sandpack", + message: MANIFEST_BUNDLER_MESSAGE, + version: "99.0.0", + packageJson: BROKEN_PACKAGE_JSON, + ...over, +}); + +// Fix-prover: with the manifest branch removed (revert baseline), this fails — +// the module would return the npm sentence for a broken manifest instead of naming +// the JSON problem. +test("a broken manifest gets the JSON sentence, not the npm one (DEV-2872 fix-prover)", () => { + const msg = describeDependencyFailure(facts()); + assert.match(msg, /is not valid JSON/); + assert.doesNotMatch(msg, /published on npm/); +}); + +// Fix-prover: the parse detail must travel into the message. Computed by the TEST's +// own try/catch over the SAME fixture, never a hardcoded V8 string — JSON parse +// wording is engine-specific (see dependencyFailure.ts's docblock, and +// fetchFailure.ts's documented Chrome/Firefox/Safari divergence for the same kind of +// caution). +test("the JSON.parse detail travels into the message (DEV-2872 fix-prover)", () => { + let detail; + try { + JSON.parse(BROKEN_PACKAGE_JSON); + assert.fail("fixture must actually be invalid JSON"); + } catch (e) { + detail = e.message; + } + const msg = describeDependencyFailure(facts()); + assert.ok(detail.length > 0, "the fixture must produce a real parse error"); + assert.ok(msg.includes(detail), `expected the message to include the parse detail: ${detail}`); +}); + +// Guard: a valid manifest plus the unpublished-version wording still gets the npm +// sentence, byte-for-byte — this module must not perturb the case DEV-2872 says to +// preserve. +test("a valid manifest with the unpublished-version wording keeps the npm sentence (guard)", () => { + const msg = describeDependencyFailure( + facts({ message: VERSION_BUNDLER_MESSAGE, packageJson: VALID_PACKAGE_JSON, version: "99.0.0" }), + ); + assert.equal(msg, "Handsontable 99.0.0 could not be fetched. Check that this exact version is published on npm."); +}); + +// Guard: a message that isn't the "could not fetch dependencies" wording at all is +// not this module's branch, regardless of manifest state — the entry gate must still +// key on Sandpack's own wording. +test("a broken manifest with an unrelated (babel code-frame) message is not this branch (guard)", () => { + const msg = describeDependencyFailure( + facts({ message: "/src/main.ts: Unexpected token (3:1)" }), + ); + assert.equal(msg, null); +}); + +// Guard: the container engine (Tier 2, DEV-2538/DEV-2553/DEMOS-9) must never be +// perturbed by this module, even with a matching message and a broken manifest. +// Pinned separately from pipeline/session-start-failure.test.mjs, which owns that +// contract's other end. +test("the container engine is never this branch (guard, DEV-2538 contract)", () => { + const msg = describeDependencyFailure(facts({ engine: "container" })); + assert.equal(msg, null); +}); + +// Guard: an absent /package.json (deliberate status-quo preservation) keeps the npm +// sentence, same as before this module existed. +test("an absent /package.json keeps the npm sentence (guard)", () => { + const msg = describeDependencyFailure(facts({ packageJson: undefined })); + assert.equal(msg, "Handsontable 99.0.0 could not be fetched. Check that this exact version is published on npm."); +}); diff --git a/runner/pipeline/sandpack-reload.test.mjs b/runner/pipeline/sandpack-reload.test.mjs index 013f70413..744a2ac0c 100644 --- a/runner/pipeline/sandpack-reload.test.mjs +++ b/runner/pipeline/sandpack-reload.test.mjs @@ -353,10 +353,10 @@ test("DEV-2550: a short message is reported byte-identical", () => { }); test("DEV-2550: the dependency-fetch message reaches describeRuntimeError intact", () => { - // App.tsx's `/could not fetch dependencies/i` branch rewrites this into the - // "check that this version is published on npm" card. The phrase leads the - // bundler's message, so the cap cannot reach it — pinned here rather than - // left to inspection. + // `describeDependencyFailure` (apps/authoring/src/dependencyFailure.ts) rewrites + // this into the "check that this version is published on npm" card. The phrase + // leads the bundler's message, so the cap cannot reach it — pinned here rather + // than left to inspection. const raw = "Could not fetch dependencies, please try again in a couple seconds: request to https://registry.npmjs.org/handsontable failed"; assert.equal(showError(raw).message, raw); assert.match(showError(raw + " " + "x".repeat(9000)).message, /^could not fetch dependencies/i);