diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index c8f007835..c330b3e89 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -23,6 +23,7 @@ import { isNextPrereleaseVersion, pinHandsontableFiles as pinToVersionRef, resolveStarterBucket, + selectedReleaseMajor, validateHandsontableVersion, type CatalogEntry, type DemoRuntime, @@ -54,7 +55,7 @@ import { AdminPanel } from "./Admin.js"; import { applyDroppedFiles } from "./addFiles.js"; import { AskAiButton, ChatPanel } from "./Chat.js"; import { StyleButton, StylePanel } from "./StylePanel.js"; -import { THEME_MODULE_BASENAME } from "./theme/codegen.js"; +import { buildResetChanges, hasWiredTheme, THEME_MODULE_BASENAME } from "./theme/codegen.js"; import { ShareLinks } from "./ShareLinks.js"; import { EditInfoDialog } from "./EditInfoDialog.js"; import { GuidePage } from "./Guide.js"; @@ -90,14 +91,6 @@ const FW_DOCS: Record = { angular: "angular-data-grid", }; -/** Numeric major of a plain release version (e.g. "17.1.0" -> 17), or null for - * next-dist-tag / pkg.pr.new / non-release refs, which are never floor-checked. */ -function releaseMajor(version: string): number | null { - if (isNextPrereleaseVersion(version)) return null; - const m = /^(\d+)\./.exec(version.trim()); - return m ? Number(m[1]) : null; -} - /** The Style panel writes `import … from "handsontable/themes"` plus three * `themes/static/variables/*` imports into the demo, and none of those paths * exist before 17.0.0 — below that the generated module cannot resolve its own @@ -108,13 +101,21 @@ function releaseMajor(version: string): number | null { * `pipeline/blank-starters.mjs` calls `isLegacyBucket`. */ const THEME_API_MIN_MAJOR = 17; +/** Said when a version change takes a generated theme module back out (DEV-2571). + * Names the removal, because it is an edit to the visitor's files that they did + * not make — and says where the theme went, since it is still in localStorage + * and reopening the panel on a supported core writes it back. */ +const THEME_REMOVED_NOTICE = + `Theming needs Handsontable ${THEME_API_MIN_MAJOR} or newer, so the custom theme was removed from this demo` + + ` — reopen Style on ${THEME_API_MIN_MAJOR} or newer to put it back.`; + /** A starter may declare a minimum core major (e.g. the UI-library starters need * the themes API added in Handsontable 17); hide lower published majors from its * version picker. next/custom refs (major null) always pass through. */ function versionsForEntry(options: string[], minCoreMajor: number | null): string[] { if (minCoreMajor == null) return options; return options.filter((v) => { - const major = releaseMajor(v); + const major = selectedReleaseMajor(v); return major == null || major >= minCoreMajor; }); } @@ -950,6 +951,12 @@ function Authoring({ // "sign in again", which is an action, not a sentence. const [sessionExpired, setSessionExpired] = useState(false); const [versionWarning, setVersionWarning] = useState(null); + /** Did the floor below just cost this demo its theme module? Its own state, not + * a `versionWarning` string: the dirty-switch branches set that one *after* + * this runs, in the same commit, and applying a theme is what makes a + * workspace dirty — so the message would always be the one the user did not + * need (DEV-2571). */ + const [themeRemoved, setThemeRemoved] = useState(false); // Cost-guardrail notice (DEV-2030): non-null once spend crosses the warn // threshold, so a user learns live sessions are about to get restricted // *before* one is refused. Null whenever the guardrail is observe-only. @@ -1054,19 +1061,104 @@ function Authoring({ // example. Mutually exclusive with the chat panel: they occupy the same edge // of the screen, and both are secondary to the code. const [styleOpen, setStyleOpen] = useState(false); - /** Can this demo's core be themed at all? `releaseMajor` answers null for the - * `next` dist-tag and for pkg.pr.new refs, which are post-18 builds — those - * pass, since refusing them would block exactly the people testing them. */ + /** Can this demo's core be themed at all? `selectedReleaseMajor` answers null + * for the `next` dist-tag and for pkg.pr.new refs, which are post-18 builds — + * those pass, since refusing them would block exactly the people testing them. + * + * It validates before reading the major, which is load-bearing: a bare `16` or + * `16.2` is a version both the pencil and `?v=` accept, and the raw-string + * reading this replaced found no `\d+\.` in either, answered null, and so + * handed a v16 core the prerelease pass-through (DEV-2571). */ const themingSupported = (() => { - const major = releaseMajor(version); + // A ref the validator refuses is not themeable either. `selectedReleaseMajor` + // answers null for one — the same null that waves prereleases through — so + // without this a `?v=14.0.0` deep link gets a live Style button over a preview + // the mount guard refuses to boot. + if (!validateHandsontableVersion(version).ok) return false; + const major = selectedReleaseMajor(version); return major === null || major >= THEME_API_MIN_MAJOR; })(); + + /** Is this demo on a core we *know* cannot resolve the theme module's imports — + * a real release major below the floor? + * + * A narrower question than `themingSupported`, and the two must not be + * conflated (review, PR #241). A ref the validator refuses is not themeable + * either, but it is not a pre-17 core: a half-typed version in the pencil, a + * legacy `latest` sentinel on a saved row (DEV-2565), a `?v=14.0.0` typo. The + * preview refuses to boot on all three, and taking a theme out of the + * workspace over any of them is destroying files to fix nothing. + * + * `selectedReleaseMajor` already answers null for everything that carries no + * comparable major — prereleases, pkg.pr.new refs, refused refs — so the + * positive test is the whole guard. */ + const belowThemeApi = (() => { + const major = selectedReleaseMajor(version); + return major !== null && major < THEME_API_MIN_MAJOR; + })(); + // Switching the version *down* has to close an open panel, not just hide it: // `styleOpen` would stay latched true and the toolbar button would keep // reading as pressed with nothing on screen. useEffect(() => { if (!themingSupported) setStyleOpen(false); }, [themingSupported]); + + // And on a core below the floor the panel is not the only thing that has to go + // (DEV-2571, Sentry DEMOS-1P). The generated theme module is a real workspace + // file, and a version switch on a dirty workspace deliberately *keeps* the files + // it finds (ADR-0021 §6) — applying a theme is what dirtied it, so a themed demo takes + // exactly that branch on the way down and arrives on a core where + // `handsontable/themes` does not exist. The preview then fails to resolve the + // module's own imports. Reset is already the operation that takes a theme back + // out, restores the `themeName` and container class it displaced, and leaves + // the module inert, so reuse it rather than inventing a second unwire. + // + // Nothing of the visitor's is lost: the theme *state* lives in localStorage + // (`StylePanel`), and reopening the panel on a supported core reconciles it + // straight back into the demo. + // + // `files` in the deps, not just the floor: a saved, shared or imported + // workspace can *arrive* already themed on a sub-17 pin, with no version change + // anywhere — a fix hanging off the version handler alone would ship green and + // leave that path reporting. + // + // Declared above the runtime-mount effect on purpose. Effects run in + // declaration order, so this write to `filesRef.current` lands before the + // remount that a version change triggers reads it; below the mount effect the + // broken module gets compiled once and only then repaired, which is the very + // event this fixes. + useEffect(() => { + if (!belowThemeApi) return; + if (!hasWiredTheme(filesRef.current)) return; + let next = filesRef.current; + for (const change of buildResetChanges(next)) { + // Skip a write that changes nothing: Reset emits the inert module + // unconditionally, and re-writing byte-identical contents would recompile + // the preview for no reason. + if (next[change.path] === change.contents) continue; + next = { ...next, [change.path]: change.contents }; + // Covers a `files` change that moves no mount dependency. Safe to do + // unconditionally: in both runtimes a non-quiet write supersedes any + // quiet write still pending for the same path (`sandpack.ts` keeps one + // authoritative `files` map; `container.ts` deletes the path from both + // queues first), so the Style panel's unmount flush cannot push the + // themed module back over this. + runtimeRef.current?.writeFile(change.path, change.contents); + } + // Nothing moved. Bail before `setFiles` rather than handing React a fresh + // object with identical contents: `files` is in this effect's own deps, so an + // unchanged-but-new map re-runs it forever. Which is what any disagreement + // between `hasWiredTheme` and `buildResetChanges` would degrade into, so the + // guard stays even though the two are written to agree. + if (next === filesRef.current) return; + filesRef.current = next; + setFiles(next); + // Deliberately not `markDirty`: this is a repair of a workspace that cannot + // run, not an edit the visitor made. Dirtying it would light up `Save •` on + // a shared demo nobody has touched. + setThemeRemoved(true); + }, [belowThemeApi, files]); /** Edit info (`114:24410`), opened from the BOX INFO pencil. Replaces the two * bare inputs T2 had to park in the authed action bar for want of a frame. * @@ -1139,7 +1231,16 @@ function Authoring({ setDirtyPaths((prev) => (prev.size ? new Set() : prev)); }, []); - /** Replace the whole workspace (entry + files + lineage) and remount. */ + /** The single `Notice` slot in the preview bar (DEV-2173 owns its placement). + * Two facts can be true at once after a downgrade — the theme was removed, + * and the edits kept may not match the new version's API — and the theme + * leads because it reports a change to the visitor's files rather than a + * caveat about them (DEV-2571). */ + const versionNotice = useMemo( + () => [themeRemoved ? THEME_REMOVED_NOTICE : null, versionWarning].filter(Boolean).join(" ") || null, + [themeRemoved, versionWarning], + ); + /** One line naming what an import refused, or null when it took everything. * Built here rather than in the Worker so the wording lives with the UI. */ const importNotice = useMemo(() => { @@ -1149,6 +1250,7 @@ function Authoring({ return `Not imported: ${shown.join(", ")}${rest > 0 ? ` and ${rest} more` : ""}.`; }, [importSkipped]); + /** Replace the whole workspace (entry + files + lineage) and remount. */ const loadWorkspace = useCallback( (nextEntry: CatalogEntry, nextFiles: FilesMap, lineage: string) => { // Whatever workspace replaces an ad-hoc one is no longer its, so its title @@ -1162,6 +1264,13 @@ function Authoring({ setImportSkipped([]); } filesRef.current = nextFiles; // ensure the mount effect reads the new files + // A new workspace has not had anything taken out of it. Cleared here rather + // than in each picker because every install lands here — starter, docs, + // import, payload, saved demo — and the notice is about the files that just + // went away with the old one (DEV-2571). A workspace that genuinely arrives + // themed below the floor sets it again in the same pass: this batches with + // `setFiles`, and the strip effect runs after both. + setThemeRemoved(false); setEntry(nextEntry); setFramework(nextEntry.framework); setFiles(nextFiles); @@ -1680,7 +1789,7 @@ function Authoring({ } const indexEntry = getEntry(framework); - const requestedMajor = releaseMajor(v.value.ref); + const requestedMajor = selectedReleaseMajor(v.value.ref); // pkg.pr.new refs are current-dev builds — they read the next bucket. const bucket = v.value.pkgPrNew ? "next" @@ -1861,6 +1970,7 @@ function Authoring({ const changeVersion = useCallback((next: string) => { docsRequestSeqRef.current += 1; setVersionWarning(null); + setThemeRemoved(false); if (docsPathRef.current) { setDocsItems([]); setActiveDocsBucket(null); @@ -1907,9 +2017,9 @@ function Authoring({ } // Per-starter floor: these starters were authored against a core API that // older majors lack, so booting them there produces a broken (or blank) - // grid. Refuse rather than boot. `releaseMajor` (shared with the version - // picker) returns null for next/pkg.pr.new refs, which bypass the check. - const requestedMajor = releaseMajor(v.value.ref); + // grid. Refuse rather than boot. `selectedReleaseMajor` (shared with the + // version picker) returns null for next/pkg.pr.new refs, which bypass it. + const requestedMajor = selectedReleaseMajor(v.value.ref); if ( !docsPath && entry.minCoreMajor != null && @@ -2101,6 +2211,13 @@ function Authoring({ */ const shellSchemeMode: SchemeMode = useMemo(() => { if (docsPath) return "auto"; + // Path presence, which latches this on `auto` forever after a Reset: the + // module stays behind as an inert `customTheme = undefined`. `hasWiredTheme` + // is the predicate that answers this correctly, and swapping it in is *not* + // enough on its own — the shell then sends its mode again and the override + // still does not come back, so there is a second cause in the bridge. Left + // as found rather than half-fixed; needs its own ticket and its own test + // (measured under DEV-2571, e2e/preview-scheme.spec.ts). const wired = Object.keys(files).some((path) => path.includes(THEME_MODULE_BASENAME)); return wired ? "auto" : themeMode; }, [docsPath, files, themeMode]); @@ -2593,7 +2710,7 @@ function Authoring({ // `dirtyPaths` is which files carry the per-tab dot (T12). dirty={dirty} dirtyPaths={dirtyPaths} - versionWarning={versionWarning} + versionWarning={versionNotice} budgetNotice={budgetNotice} importNotice={importNotice} // Ask AI and Style, both from DEV-2047. Available on every route — the diff --git a/runner/apps/authoring/src/theme/codegen.ts b/runner/apps/authoring/src/theme/codegen.ts index 57ea2de88..11b215578 100644 --- a/runner/apps/authoring/src/theme/codegen.ts +++ b/runner/apps/authoring/src/theme/codegen.ts @@ -800,6 +800,32 @@ export function themeModulePath(files: Record): string { return `/${THEME_MODULE_BASENAME}.${isTypescript(files) ? "ts" : "js"}`; } +/** Every path the generated module can occupy. `themeModulePath` picks one of the + * two by `isTypescript`, and that answer moves after the fact — adding a single + * `.ts` file to a JS demo is enough — so anything that has to *find* an existing + * module, or clear one, has to consider both (DEV-2571). */ +const THEME_MODULE_PATHS = [`/${THEME_MODULE_BASENAME}.ts`, `/${THEME_MODULE_BASENAME}.js`]; + +/** + * Is a *live* theme module wired into these files? + * + * Contents, not filename (DEV-2571 / Sentry DEMOS-1P). `buildResetChanges` does + * not delete the module — it leaves `export const customTheme = undefined` + * behind, and it writes that file even on a workspace that never had a theme — + * so a path-presence check reports a theme forever after the first Reset. What + * makes a module a theme is the import that a pre-17 core cannot resolve. + * + * Exact paths, and exactly the ones `buildResetChanges` clears. The two must + * agree: a caller that strips on a downgrade asks this, repairs, and asks again, + * so a module this finds and that does not clear is an effect that never reaches + * a fixed point. It also means a *copy* of the module somewhere else in the tree + * is not "a wired theme" — we cannot unwire what we did not write, and claiming + * otherwise is the same non-convergence by another route. + */ +export function hasWiredTheme(files: Record): boolean { + return THEME_MODULE_PATHS.some((path) => (files[path] ?? "").includes("handsontable/themes")); +} + /** * The file edits that apply `state` to a demo: the theme module, plus the one * line that hands it to the grid when we can see where that happens. @@ -874,10 +900,17 @@ export function buildResetChanges(files: Record): ThemeFileChang changes.push({ path, contents }); } - changes.push({ - path: themeModulePath(files), - contents: "// Theme cleared.\nexport const customTheme = undefined;\n", - }); + // Every extension the module exists at, not only the one `isTypescript` names + // today: a JS demo that has since gained a `.ts` file keeps its live module at + // `.js` while `themeModulePath` now answers `.ts`, and clearing only the latter + // leaves the unresolvable import exactly where it was (DEV-2571). The canonical + // path is always written, which is what makes Reset produce the inert module + // even on a workspace that never had a theme. + const cleared = "// Theme cleared.\nexport const customTheme = undefined;\n"; + const stale = THEME_MODULE_PATHS.filter((path) => path in files); + for (const path of new Set([themeModulePath(files), ...stale])) { + changes.push({ path, contents: cleared }); + } return changes; } diff --git a/runner/docs/style-panel.md b/runner/docs/style-panel.md index f695dada4..8b916e6b2 100644 --- a/runner/docs/style-panel.md +++ b/runner/docs/style-panel.md @@ -193,6 +193,41 @@ to compile. So `App.tsx` gates the toolbar button on `THEME_API_MIN_MAJOR = 17`, disabling it with the reason in its tooltip rather than hiding it, and closing an open panel when the version drops. +Closing the panel is not enough, because **the module is a file** (DEV-2571, +Sentry DEMOS-1P). It outlives the version that wrote it: a switch on a dirty +workspace deliberately keeps the files it finds and only re-pins them (ADR-0021 +§6), and applying a theme is what dirtied the workspace — so a themed demo takes +exactly that branch down to a core where `handsontable/themes` does not exist, +and the preview then cannot resolve the module's own imports. The same state also +*arrives* ready-made: a saved, shared or imported workspace can open already +themed on a sub-17 pin with no version change anywhere. + +So below the floor the app runs `buildResetChanges` over the workspace — the same +unwire the footer Reset uses, restoring the displaced `themeName` and container +class — and says so in the preview bar. Nothing of the visitor's is lost: the +theme *state* is in `localStorage`, and reopening the panel on a supported core +reconciles it straight back in. The effect is declared above the runtime-mount +effect on purpose; below it, the broken module gets compiled once and only then +repaired, which is the reported event. + +Two details that made this reachable at all: + +* The floor reads `selectedReleaseMajor` (`packages/runtime`), which validates + before taking the major. Reading it off the raw string found no `\d+\.` in a + bare `16` or `16.2` — versions the pencil and `?v=` both pass through verbatim + and `validateHandsontableVersion` accepts — answered `null`, and `null` is the + pass-through meant for prereleases. `?v=16` therefore opened the panel on a + v16 core, which is the one configuration where the generated module is the + *only* thing importing `handsontable/themes` (a 16 starter wires `themeName`), + and so exactly what the Sentry message named. +* "Is there a theme here" is `hasWiredTheme`, which reads the module's contents. + Reset does not delete the file — it leaves `customTheme = undefined` behind, + and writes that file even on a demo that never had a theme — so a filename + test answers yes forever after the first Reset. `shellSchemeMode` still asks + the filename question and so keeps standing the shell down after a Reset; + swapping the predicate there is measurably *not* sufficient (the override does + not come back), so it is left alone here and wants its own ticket. + Deliberately *not* the runner's `DEFAULT_MIN_MAJOR` (15, `packages/runtime/src/version.ts`): that floor is "cores we boot", this one is "cores with a theme API" — the same cut line `pipeline/blank-starters.mjs` calls `isLegacyBucket`. `next` and pkg.pr.new diff --git a/runner/e2e/style-apply.spec.ts b/runner/e2e/style-apply.spec.ts index 7c2325a38..9a10d9b39 100644 --- a/runner/e2e/style-apply.spec.ts +++ b/runner/e2e/style-apply.spec.ts @@ -1,4 +1,5 @@ import { test, expect, type APIRequestContext, type FrameLocator, type Page } from "@playwright/test"; +import { workspaceFiles } from "./helpers"; // Does a generated theme module actually reach the grid? (DEV-2197) // @@ -126,7 +127,14 @@ async function cellHeight(page: Page): Promise { const RUNNER_PREVIEW_PAGE = /Reconnecting to the demo|The demo stopped responding/; async function openExample(page: Page, example: string) { - await page.goto(`/?example=${example}`); + return openAt(page, `/?example=${example}`, example); +} + +/** The waits `openExample` is made of, over any URL — a payload route needs the + * same "did the preview really come up, and is the frame holding the demo" + * reading, and it is the reading that costs 40 lines. */ +async function openAt(page: Page, url: string, label: string) { + await page.goto(url); const pane = page.locator('[aria-label="Preview"]'); // Not `toHaveAttribute("ready")`: a preview that fails outright sits on `error` // for the full 180s and reports as a timeout. Poll off `booting` first, then say @@ -136,7 +144,7 @@ async function openExample(page: Page, example: string) { .not.toEqual("booting"); if ((await pane.getAttribute("data-preview-status")) === "error") { const detail = await pane.locator("pre").first().innerText().catch(() => "(no detail)"); - throw new Error(`the ${example} preview failed to start: ${detail}`); + throw new Error(`the ${label} preview failed to start: ${detail}`); } await expect(pane).toHaveAttribute("data-preview-status", "ready"); @@ -156,7 +164,7 @@ async function openExample(page: Page, example: string) { ]); if (outcome === "runner-page") { throw new Error( - `the ${example} preview frame is holding the runner's own page, not the demo: ${await apology.innerText()}`, + `the ${label} preview frame is holding the runner's own page, not the demo: ${await apology.innerText()}`, ); } } @@ -528,3 +536,72 @@ test("a recolour reaches the header the grid is painting right now", async ({ pa ).toEqual("rgb(230, 244, 234)"); // #e6f4ea, the ramp's lightest step }).toPass({ timeout: 60_000 }); }); + +// DEV-2571 / Sentry DEMOS-1P, and the one assertion no amount of generated text +// can make: after the module is taken back out, does the demo actually compile +// and render on a core that has no theme API? +// +// The fixture is a workspace that *arrives* themed on a 16 pin — a payload, +// which is also the only shape where the generated module is the sole importer +// of `handsontable/themes`. A 16 starter wires `themeName`, so nothing else in +// such a demo asks for that path, which is precisely why the reported event +// named `/handsontable-theme.js`. (A dirty 18 -> 16 switch is a different +// story: those starters import `handsontable/themes` themselves and ADR-0021 §6 +// keeps the files it finds, which is what the version warning is for.) +const THEMED_AT_16_PAYLOAD = { + framework: "javascript", + title: "Themed on 16", + files: { + // A real Vite entry: the module script is what loads `/index.js` at all, and + // 16 has no CSS auto-injection, so the stylesheet is imported by hand. + "/index.html": + '\n\n\n' + + '\n
\n' + + ' \n\n\n', + "/index.js": + "import { customTheme } from './handsontable-theme'; // handsontable-theme\n" + + "import Handsontable from 'handsontable';\n" + + "import 'handsontable/dist/handsontable.full.min.css';\n\n" + + "new Handsontable(document.getElementById('example'), {\n" + + " data: [['Tesla', 'Model 3'], ['Nissan', 'Leaf']],\n" + + " theme: customTheme,\n" + + " rowHeaders: true,\n" + + " colHeaders: true,\n" + + " licenseKey: 'non-commercial-and-evaluation',\n" + + "});\n", + "/handsontable-theme.js": + "import { getTheme, hasTheme, registerTheme, reinitTheme } from 'handsontable/themes';\n" + + "import tokensPreset from 'handsontable/themes/static/variables/tokens/main';\n\n" + + "const THEME_NAME = 'custom-theme';\n" + + "if (hasTheme(THEME_NAME)) reinitTheme(THEME_NAME, { tokens: tokensPreset });\n" + + "else registerTheme(THEME_NAME, { tokens: tokensPreset });\n" + + "export const customTheme = getTheme(THEME_NAME);\n", + "/package.json": JSON.stringify( + { dependencies: { handsontable: "16.2.0" } }, + null, + 2, + ), + }, +}; + +test("a demo that arrives themed on a pre-theme-API core still renders", async ({ page }) => { + test.skip(process.env.E2E_LIVE !== "1", "set E2E_LIVE=1 to run live-render checks"); + test.setTimeout(240_000); + + await page.route("**/api/payload/thm16live1", (route) => + route.fulfill({ json: THEMED_AT_16_PAYLOAD })); + + // Unstripped, `handsontable/themes` does not exist in handsontable@16.2.0 and + // the bundler answers `Could not find module in path: 'handsontable/themes' + // relative to '/handsontable-theme.js'` — `openAt` turns that into the + // preview-failed-to-start error with the message attached. + await openAt(page, "/?payload=thm16live1&v=16.2.0", "themed-on-16 payload"); + + await expect(page.locator(".hot-file-row", { hasText: "handsontable-theme" })).toBeVisible(); + const files = await workspaceFiles(page); + expect(files["/handsontable-theme.js"]).toContain("Theme cleared."); + expect(files["/index.js"]).not.toContain("customTheme"); + // The grid rendered above; it must be the demo's own, unthemed — no theme + // class, because the module that would have registered one is inert. + expect(await themeClass(page)).toBe(""); +}); diff --git a/runner/e2e/style-panel.spec.ts b/runner/e2e/style-panel.spec.ts index 32f716814..3b0ed5d60 100644 --- a/runner/e2e/style-panel.spec.ts +++ b/runner/e2e/style-panel.spec.ts @@ -1,5 +1,5 @@ import { test, expect, type Page } from "@playwright/test"; -import { stubShell, workspaceFiles } from "./helpers"; +import { pickFromMenu, stubShell, workspaceFiles } from "./helpers"; // The Style panel itself (DEV-2203) — the highest-value spec on the task, // because this seam is where seven defects hid, four of them silently. @@ -436,3 +436,218 @@ test("a Google Font pick injects the stylesheet link and says so", async ({ page expect(module).toContain("document.head.appendChild(fontLink)"); }).toPass(); }); + +// DEV-2571 (Sentry DEMOS-1P): `DemoError: Could not find module in path: +// 'handsontable/themes' relative to '/handsontable-theme.js'`. +// +// The generated module is a real workspace file, and a version switch on a +// *dirty* workspace deliberately keeps the files it finds (ADR-0021 §6) — +// applying a theme is what dirties it, so a themed demo takes exactly that +// branch down to a core that has no `handsontable/themes` at all. + +/** `stubShell` advertises 18.0.0 and 17.1.0 only, and the picker cannot offer a + * version the list does not hold — so a downgrade case has to widen it. The + * override is registered *after* `stubShell` on purpose: Playwright matches the + * most recently added handler first. */ +async function openPanelAtVersions(page: Page, url: string, versions: string[]) { + await stubShell(page); + await page.route("**/api/versions", (route) => + route.fulfill({ json: { latest: "18.0.0", next: "19.0.0-next.1", versions } })); + await page.route("**/api/versions/exists**", (route) => route.fulfill({ json: { exists: true } })); + await page.goto(url); + await page.getByRole("button", { name: "Style", exact: true }).click(); + await expect(page.locator(STYLE)).toBeVisible(); + return page.locator(STYLE); +} + +const themeModule = (files: Record) => + Object.entries(files).find(([path]) => path.includes("handsontable-theme")); + +test("a downgrade below the theme API takes the theme module back out", async ({ page }) => { + const drawer = await openPanelAtVersions(page, "/?example=react&v=18.0.0", ["18.0.0", "17.1.0", "16.2.0"]); + + await drawer.getByLabel("Colour scheme").selectOption("dark"); + await expectApplied(page); + + // Precondition: the module really does import the path a pre-17 core lacks, + // and the entry really is wired to it. Without this the test could pass on a + // demo that was never themed. + let entry = ""; + await expect(async () => { + const files = await workspaceFiles(page); + const [, module] = themeModule(files) ?? []; + expect(module, "fixture precondition: a theme module was written").toContain("handsontable/themes"); + entry = Object.keys(files).find((p) => files[p].includes("theme={customTheme}")) ?? ""; + expect(entry, "fixture precondition: the theme was wired into the grid").not.toBe(""); + }).toPass(); + + // The drawer overlays the preview bar, so the version pill is only clickable + // with the panel closed — which is also the honest flow: nobody switches core + // versions from inside the Style panel. Closing it flushes the panel's pending + // quiet writes, so the theme is fully landed before the downgrade. + await page.getByRole("button", { name: "Style", exact: true }).click(); + await expect(page.locator(STYLE)).toBeHidden(); + // The drawer hands focus back to its trigger as it unmounts, and the trigger + // shows its tooltip on focus — a wide one, which then covers the version pill. + // Escape is what that button offers to dismiss it (`StyleButton`, onKeyDown). + await page.keyboard.press("Escape"); + await expect(page.getByRole("tooltip")).toBeHidden(); + + await pickFromMenu(page, "Handsontable version", "16.2.0"); + + await expect(async () => { + const files = await workspaceFiles(page); + // The switch itself happened — otherwise everything below is vacuous. + expect(files["/package.json"]).toContain('"handsontable": "16.2.0"'); + // The module is inert, so nothing it used to import is asked for any more. + const [, module] = themeModule(files) ?? []; + expect(module).toContain("Theme cleared."); + expect(module).not.toContain("handsontable/themes"); + // Unwired in place, not line-deleted: the grid element survives. + expect(files[entry]).not.toContain("customTheme"); + expect(files[entry]).toContain("=17 + // starters import `handsontable/themes` themselves (`isLegacyBucket` in + // pipeline/blank-starters.mjs emits `theme:` above 16 and `themeName:` + // below), and a dirty cross-bucket switch keeps the files it finds by + // design (ADR-0021 §6). So this workspace can still hold a 17+ API on a + // 16 core — that is what the second half of the composed notice is for, + // and it is not DEV-2571's to remove. + }).toPass(); + + // The notice has to name the theme. The dirty-switch branch sets its own + // "unsaved edits may not match the selected version API" string in the same + // commit and into the same single slot, and left to win it would tell the user + // nothing about the file that just changed under them. + await expect(page.locator('span[title*="the custom theme was removed from this demo"]')).toBeVisible(); + + // And the button says why it can no longer be opened. + await expect(page.getByRole("button", { name: "Style", exact: true })) + .toHaveAttribute("aria-disabled", "true"); +}); + +test("a bare major deep link cannot open the Style panel", async ({ page }) => { + // `16` is a version `validateHandsontableVersion` accepts (coerced to 16.0.0) + // and both `?v=` and the version pencil pass through verbatim. Reading the + // major off the raw string found no `\d+\.`, answered null, and null is the + // pass-through meant for `next`/pkg.pr.new builds — so this booted a v16 core + // with theming enabled, which is how a themed 16 workspace gets made at all. + await stubShell(page); + await page.goto("/?example=react&v=16"); + + // `aria-disabled` rather than `disabled`: the button stays focusable so its + // tooltip is reachable by keyboard. Playwright reads it as not enabled, which + // is the assertion — a click cannot be dispatched at all. + const style = page.getByRole("button", { name: "Style", exact: true }); + await expect(style).toHaveAttribute("aria-disabled", "true"); + await expect(style).toBeDisabled(); + await expect(page.locator(STYLE)).toBeHidden(); +}); + +test("a version the validator refuses cannot open the Style panel either", async ({ page }) => { + // `selectedReleaseMajor` answers null for a ref the validator rejects, and null + // is the pass-through meant for `next`/pkg.pr.new builds — so a sub-floor deep + // link used to get a live Style button over a preview the mount guard refuses + // to boot, and theming it would have written a module into a workspace that + // cannot run at all. + await stubShell(page); + await page.goto("/?example=react&v=14.0.0"); + + const style = page.getByRole("button", { name: "Style", exact: true }); + await expect(style).toHaveAttribute("aria-disabled", "true"); + await expect(style).toBeDisabled(); + await expect(page.locator(STYLE)).toBeHidden(); +}); + +// The hydration half of DEV-2571, and the shape the reported event most likely +// had: a workspace that *arrives* themed on a sub-17 pin, with no version change +// anywhere. Authored as a payload so the only `handsontable/themes` import in it +// is the generated module's — a 16 starter wires `themeName`, so nothing else in +// a themed-at-16 demo asks for that path, which is exactly why Sandpack blamed +// `/handsontable-theme.js` by name. +const THEMED_AT_16 = { + framework: "javascript", + title: "Themed on 16", + files: { + "/index.html": '
', + "/index.js": + "import { customTheme } from './handsontable-theme'; // handsontable-theme\n" + + "import Handsontable from 'handsontable';\n" + + "import { data } from './data.js';\n\n" + + "new Handsontable(document.getElementById('example'), {\n" + + " data: data,\n" + + " theme: customTheme,\n" + + " rowHeaders: true,\n" + + "});\n", + "/data.js": "export const data = [['a']];\n", + "/handsontable-theme.js": + "import { getTheme, hasTheme, registerTheme, reinitTheme } from 'handsontable/themes';\n" + + "import tokensPreset from 'handsontable/themes/static/variables/tokens/main';\n\n" + + "const THEME_NAME = 'custom-theme';\n" + + "if (hasTheme(THEME_NAME)) reinitTheme(THEME_NAME, { tokens: tokensPreset });\n" + + "else registerTheme(THEME_NAME, { tokens: tokensPreset });\n" + + "export const customTheme = getTheme(THEME_NAME);\n", + "/package.json": JSON.stringify({ dependencies: { handsontable: "16.2.0" } }, null, 2), + }, +}; + +test("an unusable version leaves a wired theme alone", async ({ page }) => { + // "Not themeable" and "on a core below the theme API" are two different + // questions, and the strip may only key on the second (review, PR #241). A ref + // the validator refuses — a half-typed version in the pencil, a legacy `latest` + // sentinel on a saved row, a `?v=` typo — is not a pre-17 core. The preview + // refuses to boot on all of them, and taking a theme out of the workspace over + // any of them destroys files to fix nothing. + await stubShell(page); + await page.route("**/api/payload/thm16at0002", (route) => route.fulfill({ json: THEMED_AT_16 })); + await page.goto("/?payload=thm16at0002&v=latest"); + + // The Style button is still refused — that half is right. + await expect(page.getByRole("button", { name: "Style", exact: true })) + .toHaveAttribute("aria-disabled", "true"); + + // The files are untouched. Polled, so this cannot pass by reading the workspace + // before the strip effect would have had its chance. + await expect(async () => { + const files = await workspaceFiles(page); + expect(Object.keys(files), "the payload opened").toContain("/handsontable-theme.js"); + expect(files["/handsontable-theme.js"]).toContain("handsontable/themes"); + expect(files["/index.js"]).toContain("customTheme"); + }).toPass(); + await expect(page.locator('span[title*="the custom theme was removed"]')).toBeHidden(); +}); + +test("a workspace that arrives themed on a sub-17 pin is repaired on load", async ({ page }) => { + await stubShell(page); + await page.route("**/api/payload/thm16at0001", (route) => route.fulfill({ json: THEMED_AT_16 })); + await page.goto("/?payload=thm16at0001&v=16.2.0"); + + await expect(async () => { + const files = await workspaceFiles(page); + expect(Object.keys(files), "the payload opened").toContain("/handsontable-theme.js"); + // The import a 16 core cannot resolve — the DEMOS-1P message names this file + // and this specifier — is gone, and so is the wiring that reached for it. + expect(files["/handsontable-theme.js"]).not.toContain("handsontable/themes"); + expect(files["/handsontable-theme.js"]).toContain("Theme cleared."); + expect(files["/index.js"]).not.toContain("customTheme"); + // Unwired in place: the settings object and the grid survive. + expect(files["/index.js"]).toContain("rowHeaders: true,"); + expect(files["/index.js"]).toContain("new Handsontable("); + }).toPass(); + + await expect(page.locator('span[title*="the custom theme was removed from this demo"]')).toBeVisible(); + await expect(page.getByRole("button", { name: "Style", exact: true })) + .toHaveAttribute("aria-disabled", "true"); + + // The notice is about the workspace that just lost its theme, so it must not + // outlive it: the next workspace never had one. (Nothing sets the flag back to + // false on its own — the strip effect early-returns when there is no theme.) + await page.getByRole("button", { name: /JavaScript/ }).first().click(); + await page.getByText("Starter templates", { exact: true }).click(); + await page.getByRole("treeitem", { name: "React (Vite, TS)" }).click(); + await expect(async () => { + const files = await workspaceFiles(page); + expect(Object.keys(files), "the starter replaced the payload").not.toContain("/handsontable-theme.js"); + }).toPass(); + await expect(page.locator('span[title*="the custom theme was removed"]')).toBeHidden(); +}); diff --git a/runner/packages/runtime/src/index.ts b/runner/packages/runtime/src/index.ts index f4a077a58..aa9231bd6 100644 --- a/runner/packages/runtime/src/index.ts +++ b/runner/packages/runtime/src/index.ts @@ -17,6 +17,7 @@ export { handsontableDependencyRef, pinHandsontableFiles, validateHandsontableVersion, + selectedReleaseMajor, isHandsontablePackage, isNextPrereleaseVersion, pickLatestNextVersion, diff --git a/runner/packages/runtime/src/version.ts b/runner/packages/runtime/src/version.ts index 42e5f3a06..d6de0ad69 100644 --- a/runner/packages/runtime/src/version.ts +++ b/runner/packages/runtime/src/version.ts @@ -159,6 +159,37 @@ export function validateHandsontableVersion( return err ? { ok: false, message: err } : { ok: true, value: { ref: normalized, pkgPrNew: false } }; } +/** + * The major of the version a picker, a `?v=` deep link or the version pencil + * actually selected — validated first, so npm-style partials read as their real + * major. + * + * DEV-2571 (Sentry DEMOS-1P): the authoring app used to take the major off the + * raw string with `/^(\d+)\./`, which has no match in a bare `16` or `16.2`. + * Both are accepted by `validateHandsontableVersion` (coerced to `16.0.0`) and + * both reach version state verbatim, so a floor check on the raw string read + * them as "no major" — and "no major" is the pass-through a theme/API floor + * grants a prerelease. `?v=16` therefore opened the Style panel on a v16 core. + * + * `null` means only what it says: this ref carries no comparable release major. + * That is the `next` nightlies (a post-18 build parsing as major 0), dotted + * `-next` prereleases, pkg.pr.new build refs, and anything the validator + * refuses outright. Callers must not read it as 0. + * + * Deliberately built on the module-private `releaseMajor` below rather than + * re-deriving: that one answers a *different* question (which CSS link a ref + * wants) and this is the only other place a release major is compared, so a + * third implementation is a third thing to drift. + */ +export function selectedReleaseMajor(version: string): number | null { + const validated = validateHandsontableVersion(version); + if (!validated.ok) return null; + // A pkg.pr.new ref is a build id, not a version — `releaseMajor` would read + // a bare `7940` as major 7940 rather than "not a release". + if (validated.value.pkgPrNew) return null; + return releaseMajor(validated.value.ref); +} + /** True if `name` is a Handsontable package that should be version-pinned. */ export function isHandsontablePackage(name: string): boolean { return name.includes("handsontable") && !NEVER_REWRITE.has(name); @@ -283,7 +314,12 @@ export function applyHandsontableVersion( * as major 0 under plain semver while actually being a post-18 nightly, so any * `major <= N` comparison silently classifies the newest core as the oldest. * Null means "not a release we can compare" — callers must not treat it as 0. - * Mirrors the App-side `releaseMajor` helper. + * + * Answers a narrower question than `selectedReleaseMajor` above and is not + * interchangeable with it: this one takes an already-validated ref, and it reads + * every `-next` build as null because a nightly wants the >=17 CSS treatment + * whatever its printed major says. It used to claim to mirror an App-side helper + * that disagreed with it on both `"16"` and `"19.0.0-next.1"` (DEV-2571). */ function releaseMajor(ref: string): number | null { const trimmed = ref.trim(); diff --git a/runner/pipeline/theme-wiring.test.mjs b/runner/pipeline/theme-wiring.test.mjs index cf8d8aa52..8462b27b5 100644 --- a/runner/pipeline/theme-wiring.test.mjs +++ b/runner/pipeline/theme-wiring.test.mjs @@ -167,7 +167,7 @@ const CASES = { }; const SCRIPT = ` -const { buildThemeChanges, buildResetChanges, buildThemeModule } = await import("./theme/codegen.ts"); +const { buildThemeChanges, buildResetChanges, buildThemeModule, hasWiredTheme } = await import("./theme/codegen.ts"); const { DEFAULT_THEME } = await import("./theme/vocabulary.ts"); const cases = ${JSON.stringify(CASES)}; const out = {}; @@ -177,10 +177,14 @@ for (const [name, { path, source: original, keep, unwired }] of Object.entries(c for (const c of changes) files[c.path] = c.contents; const applied = files[path]; const code = applied.split("\\n").filter((l) => !l.includes("handsontable-theme")).join("\\n"); + const detectedApplied = hasWiredTheme(files); for (const c of buildResetChanges(files)) files[c.path] = c.contents; out[name] = { linked, applied, + detectedApplied, + detectedReset: hasWiredTheme(files), + themesImportAfterReset: Object.values(files).some((s) => s.includes("handsontable/themes")), unwired: Boolean(unwired), wired: code.includes("theme={customTheme}") || code.includes("theme: customTheme") @@ -213,6 +217,52 @@ out.__density = buildThemeModule( }, true, ); +// DEV-2571: what a sub-17 core does to a themed workspace. The strip reuses +// Reset, so the predicate that decides *whether* to strip is the part that has +// to be right — path presence is not it, because Reset leaves the module behind. +out.__detect = { + none: hasWiredTheme({ "/src/index.jsx": "const a = 1;\\n" }), + clearedOnly: hasWiredTheme({ + "/src/index.jsx": "const a = 1;\\n", + "/handsontable-theme.js": "// Theme cleared.\\nexport const customTheme = undefined;\\n", + }), + moduleNamedButUnrelated: hasWiredTheme({ "/handsontable-theme.js": "export const customTheme = 1;\\n" }), + typescript: (() => { + const files = { "/src/index.tsx": '' }; + for (const c of buildThemeChanges(files, DEFAULT_THEME).changes) files[c.path] = c.contents; + return { wired: hasWiredTheme(files), paths: Object.keys(files) }; + })(), + // A copy of the module elsewhere in the tree. Not ours to unwire — and saying + // otherwise is a predicate that finds what the repair cannot clear. + copyElsewhere: hasWiredTheme({ + "/src/handsontable-theme-copy.js": "import { getTheme } from 'handsontable/themes';\\n", + }), + // The extension themeModulePath names can move *after* the module is written: + // one .ts file added to a JS demo is enough. Repair has to reach a fixed point + // anyway, or the effect that strips on a downgrade re-runs forever and the + // unresolvable import stays put. (No backticks in here: SCRIPT is a template + // literal.) + mixedExtension: (() => { + const files = { "/index.js": "new Handsontable(el, {\\n data: data,\\n});\\n" }; + for (const c of buildThemeChanges(files, DEFAULT_THEME).changes) files[c.path] = c.contents; + const wroteTo = Object.keys(files).filter((p) => p.includes("handsontable-theme")); + files["/util.ts"] = "export const x = 1;\\n"; + let repaired = files; + for (const c of buildResetChanges(repaired)) repaired = { ...repaired, [c.path]: c.contents }; + // A second pass must ask for nothing new — that is the fixed point. + const secondPass = buildResetChanges(repaired) + .filter((c) => repaired[c.path] !== c.contents) + .map((c) => c.path); + return { + wroteTo, + wiredAfterRepair: hasWiredTheme(repaired), + themesLeft: Object.entries(repaired) + .filter(([, src]) => typeof src === "string" && src.includes("handsontable/themes")) + .map(([path]) => path), + secondPass, + }; + })(), +}; console.log(JSON.stringify(out)); `; @@ -275,6 +325,54 @@ test("an inline theme object is swapped whole — no dangling brace, no bail", { } }); +// DEV-2571 (Sentry DEMOS-1P). The generated module is a real workspace file, so +// it outlives a downgrade to a core with no `handsontable/themes` at all and the +// preview then fails to resolve its imports. The app strips it below the floor, +// and it decides *whether* there is anything to strip with this predicate. +test("hasWiredTheme reads the module's contents, not its filename", { skip }, () => { + for (const [name, r] of wiringCases()) { + if (r.unwired) continue; + assert.equal(r.detectedApplied, true, `${name}: a wired theme went undetected, so a downgrade would keep it`); + } + + const d = results.__detect; + assert.equal(d.none, false, "no module at all is not a theme"); + assert.equal(d.copyElsewhere, false, "a copy of the module elsewhere is not the module we wrote"); + // Reset does not delete the file — it leaves `customTheme = undefined` behind + // (and creates that file even on an unthemed workspace). Keying on the path + // would report a theme forever after the first Reset. + assert.equal(d.clearedOnly, false, "a cleared module is not a theme"); + assert.equal(d.moduleNamedButUnrelated, false, "a module that imports nothing themeable is not a theme"); + assert.equal(d.typescript.wired, true, `the .ts module must be found too, got ${d.typescript.paths.join(", ")}`); +}); + +// The apply-time extension and the repair-time extension are two different +// answers, and they diverge as soon as a JS demo gains a `.ts` file. Before this +// was pinned, Reset wrote a *new* cleared `.ts` module and left the live `.js` +// one importing `handsontable/themes` — so `hasWiredTheme` stayed true, the +// downgrade strip re-ran on every render (`files` is in its deps), and DEMOS-1P +// went on firing behind a preview being recompiled in a loop. +test("repair clears the module at whichever extension it was written to", { skip }, () => { + const m = results.__detect.mixedExtension; + assert.deepEqual(m.wroteTo, ["/handsontable-theme.js"], "fixture precondition: a JS demo writes the JS module"); + assert.deepEqual(m.themesLeft, [], "a handsontable/themes import survived the repair"); + assert.equal(m.wiredAfterRepair, false, "still reads as themed, so the strip would run again"); + assert.deepEqual(m.secondPass, [], "repair is not a fixed point: a second pass still wants to write"); +}); + +test("Reset leaves no handsontable/themes import anywhere", { skip }, () => { + // The invariant the downgrade strip depends on: whatever Reset returns has to + // be safe to compile against a pre-17 core, in every wiring shape. + for (const [name, r] of wiringCases()) { + assert.equal(r.detectedReset, false, `${name}: still reads as themed after Reset`); + assert.equal( + r.themesImportAfterReset, + false, + `${name}: a handsontable/themes import survived Reset — below 17 that is the DEMOS-1P resolve failure`, + ); + } +}); + test("a Unicode line separator cannot break out of the marker comment", { skip }, () => { // U+2028/U+2029 are line terminators to JavaScript but are left untouched by // JSON.stringify. The displaced `themeName` rides in a `//` comment, so one diff --git a/runner/pipeline/version.test.mjs b/runner/pipeline/version.test.mjs index 3c510584b..0d08b44aa 100644 --- a/runner/pipeline/version.test.mjs +++ b/runner/pipeline/version.test.mjs @@ -1,6 +1,11 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { applyHandsontableCss, pickLatestNextVersion, validateHandsontableVersion } from "../packages/runtime/dist/version.js"; +import { + applyHandsontableCss, + pickLatestNextVersion, + selectedReleaseMajor, + validateHandsontableVersion, +} from "../packages/runtime/dist/version.js"; // DEV-2207: `dist/handsontable.full.min.css` was removed from the package at // 17.0.0, so every pre-DEV-2207 artifact 404s at >=17 — where core injects its @@ -149,3 +154,33 @@ test("pickLatestNextVersion returns null when no -next versions exist", () => { assert.equal(pickLatestNextVersion({}), null); assert.equal(pickLatestNextVersion(undefined), null); }); + +// DEV-2571 (Sentry DEMOS-1P): the authoring app used to read the major straight +// off the raw version string with /^(\d+)\./, so a bare npm-style partial — "16", +// "16.2", both of which validateHandsontableVersion accepts and both reachable +// through the version pencil and a hand-typed ?v= — answered null. null is the +// pass-through the theming gate grants `next`/pkg.pr.new refs, so `?v=16` opened +// the Style panel on a core with no theme API at all. Validate first: the major +// comes off the *normalized* ref, and null now means only "no semver here". +test("selectedReleaseMajor reads the major off the validated ref, partials included", () => { + assert.equal(selectedReleaseMajor("17.1.0"), 17); + assert.equal(selectedReleaseMajor("16"), 16); + assert.equal(selectedReleaseMajor("16.2"), 16); + assert.equal(selectedReleaseMajor(" 18.0.0 "), 18); +}); + +test("selectedReleaseMajor answers null only for refs carrying no comparable semver", () => { + // The npm `next` nightly parses as major 0 and is really a post-18 build. + assert.equal(selectedReleaseMajor("0.0.0-next-64139ae-20260219"), null); + // A dotted prerelease is a -next build too. + assert.equal(selectedReleaseMajor("19.0.0-next.1"), null); + // pkg.pr.new build ids, bare and as a URL. + assert.equal(selectedReleaseMajor("7940"), null); + assert.equal(selectedReleaseMajor("https://pkg.pr.new/handsontable@7940"), null); + // Anything the validator refuses has no major to report — a range, a dist-tag, + // a major under the floor, junk. + assert.equal(selectedReleaseMajor("^17.0.0"), null); + assert.equal(selectedReleaseMajor("latest"), null); + assert.equal(selectedReleaseMajor("14.0.0"), null); + assert.equal(selectedReleaseMajor(""), null); +});