From 1740e9c123e44f8d01598086e34745550cebbf0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Thu, 20 Aug 2026 11:29:45 +0200 Subject: [PATCH 1/3] fix(runner): take the theme module out below the theme API (DEV-2571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Style panel's generated `/handsontable-theme.{ts,js}` is a real workspace file, and `THEME_API_MIN_MAJOR = 17` only ever disabled the toolbar button. So the module outlived the version that wrote it and the preview could not resolve its own imports: DemoError: Could not find module in path: 'handsontable/themes' relative to '/handsontable-theme.js' Two paths reached that state. The reported one is the theming gate reading the major off the *raw* version string. A bare `16` or `16.2` has no `\d+\.` in it, so the regex answered null — and null is the pass-through that lets `next` and pkg.pr.new refs be themed. Both are versions `validateHandsontableVersion` accepts (coerced to `16.0.0`) and both reach version state verbatim, from the version pencil and from `?v=`. `?v=16` therefore opened the panel on a v16 core, and that is the one shape where the generated module is the *only* importer of `handsontable/themes`: a 16 starter wires `themeName`. Which is exactly the file Sandpack named. Fixed by `selectedReleaseMajor` in packages/runtime, which validates before taking the major, so null now means only "no comparable release major here". The second is the downgrade. A version 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 dirties the workspace — so a themed demo takes that branch down to a core with no theme API. 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. Below the floor the app now runs `buildResetChanges` over the workspace — the same unwire the panel's own Reset uses, restoring the `themeName` and container class it displaced — 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. Four details worth keeping: * The effect is declared above the runtime-mount effect. Effects run in declaration order, so its write to `filesRef.current` lands before the remount reads it; below the mount effect the broken module is compiled once and only then repaired, which is the reported event. * The notice is its own state, not a `versionWarning` string. The dirty-switch branches set that one *after* this runs, in the same commit, and a theme is what dirtied the workspace — so the message would always have been the one the user did not need. It is cleared in `loadWorkspace`, which every workspace install goes through, so it cannot outlive the files it describes. * "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. * Not `markDirty`: this repairs a workspace that cannot run, it is not an edit the visitor made, and dirtying it would light up `Save •` on a shared demo nobody has touched. Two things deliberately left alone. The >=17 starters import `handsontable/themes` themselves, so a dirty cross-bucket downgrade can still hold a 17+ API on a 16 core — ADR-0021's documented trade-off, and what the second half of the composed notice is for. And `shellSchemeMode` asks the filename question that `hasWiredTheme` replaces, so it keeps standing the shell down after a Reset; swapping the predicate there is measurably not sufficient (the override does not come back), so it stays as found and wants its own ticket. Verified by reverting each source file to master in place: the two pipeline files and all three deterministic specs go red, and the live case reproduces the Sentry message verbatim before the fix and renders a grid after it. Fixes DEMOS-1P Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 123 +++++++++++--- runner/apps/authoring/src/theme/codegen.ts | 20 +++ runner/docs/style-panel.md | 35 ++++ runner/e2e/style-apply.spec.ts | 83 +++++++++- runner/e2e/style-panel.spec.ts | 176 ++++++++++++++++++++- runner/packages/runtime/src/index.ts | 1 + runner/packages/runtime/src/version.ts | 38 ++++- runner/pipeline/theme-wiring.test.mjs | 55 ++++++- runner/pipeline/version.test.mjs | 37 ++++- 9 files changed, 540 insertions(+), 28 deletions(-) diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index c8f007835..4bc8f5827 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,68 @@ 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); + 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. + // + // And 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 `themingSupported`: 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 (!themingSupported) setStyleOpen(false); - }, [themingSupported]); + if (themingSupported) return; + setStyleOpen(false); + if (!hasWiredTheme(filesRef.current)) return; + let next = filesRef.current; + for (const change of buildResetChanges(next)) { + 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); + } + 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); + }, [themingSupported, 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. * @@ -1142,6 +1198,16 @@ function Authoring({ /** Replace the whole workspace (entry + files + lineage) and remount. */ /** 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. */ + /** 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], + ); + const importNotice = useMemo(() => { if (!importSkipped.length) return null; const shown = importSkipped.slice(0, 2).map((s) => `${s.path} (${s.reason})`); @@ -1162,6 +1228,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 +1753,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 +1934,7 @@ function Authoring({ const changeVersion = useCallback((next: string) => { docsRequestSeqRef.current += 1; setVersionWarning(null); + setThemeRemoved(false); if (docsPathRef.current) { setDocsItems([]); setActiveDocsBucket(null); @@ -1907,9 +1981,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 +2175,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 +2674,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..52a329ab4 100644 --- a/runner/apps/authoring/src/theme/codegen.ts +++ b/runner/apps/authoring/src/theme/codegen.ts @@ -800,6 +800,26 @@ export function themeModulePath(files: Record): string { return `/${THEME_MODULE_BASENAME}.${isTypescript(files) ? "ts" : "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. + * + * Both extensions are scanned rather than just `themeModulePath(files)`: adding + * a `.ts` file to a JS demo flips that answer, and the stale module would then + * be invisible to the very check meant to find it. + */ +export function hasWiredTheme(files: Record): boolean { + return Object.entries(files).some(([path, source]) => + path.includes(THEME_MODULE_BASENAME) + && typeof source === "string" + && source.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. 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..ccc8f06dc 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,177 @@ 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(); +}); + +// 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("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..4d5b9a7e6 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,22 @@ 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) }; + })(), +}; console.log(JSON.stringify(out)); `; @@ -275,6 +295,39 @@ 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"); + // 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(", ")}`); +}); + +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); +}); From a70c4c321b4bd95daea3b07ab4414d9c76fc0145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Thu, 20 Aug 2026 12:33:50 +0200 Subject: [PATCH 2/3] fix(runner): make the theme repair reach a fixed point (review, PR #241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the review of the parent commit, two of them the same defect found independently by Bugbot. **The repair and the predicate disagreed on which file to fix.** `hasWiredTheme` matched any path containing `handsontable-theme` at either extension, while `buildResetChanges` cleared only `themeModulePath(files)` — which flips between `.ts` and `.js` on `isTypescript(files)`. That answer moves *after* the module is written: one `.ts` file added to a JS demo is enough. Reproduced by running the real codegen over a JS demo themed at `/handsontable-theme.js` that then gains a `.ts` file — the repair wrote a *new* cleared `.ts` module and left the live `.js` one importing `handsontable/themes`: iteration 0: changed ["/index.js", "/handsontable-theme.ts"] stillWired: true iteration 1: changed ["/handsontable-theme.ts"] stillWired: true (…forever) So DEMOS-1P went on firing, and because `files` is in the strip effect's own dep list the effect re-ran on every render — an unbounded loop with a non-quiet `writeFile` on each pass. Reachable without any file editing at all: JS starter at 18, apply a theme, add a `.ts` file (New file, drag-drop, an imported repo), switch to 16. Both sides now name the same two paths. `THEME_MODULE_PATHS` is the pair the module can occupy; `hasWiredTheme` reads exactly those, and `buildResetChanges` clears every one of them that exists plus the canonical path (which is what keeps Reset producing the inert module on a workspace that never had a theme). Exact paths also retire the substring match, so a *copy* of the module elsewhere in the tree is no longer reported as a wired theme — we cannot unwire what we did not write, and claiming otherwise is the same non-convergence by another route. **The strip effect had no fixed-point guard**, so any such disagreement became a render loop rather than a harmless no-op. It now skips a write whose contents already match and returns before `setFiles` when nothing moved. Kept even though the two sides are written to agree. **A ref the validator refuses is not themeable.** `selectedReleaseMajor` answers null for `14.0.0` — the same null that waves prereleases through — so `?v=14.0.0` got a live Style button over a preview the mount guard refuses to boot, where `master`'s helper correctly said no. `themingSupported` now requires the version to validate first. **And three doc comments had stacked up** where the `versionNotice` memo was inserted, leaving `importNotice` and `loadWorkspace` documented by each other's JSDoc. Put back on their own definitions. Tests: the mixed-extension case is pinned in `pipeline/theme-wiring.test.mjs` (apply on JS, add a `.ts` file, repair once — nothing still imports `handsontable/themes`, and a second pass wants to write nothing), together with the copy-elsewhere case; `?v=14.0.0` leaving the Style button disabled is a new deterministic spec. Both new pipeline assertions go red against the parent commit's codegen; verified by checking that file out in place. 757 pipeline tests 0 fail 0 skipped, 199 deterministic e2e, 20 live, four typechecks clean. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 21 ++++++++-- runner/apps/authoring/src/theme/codegen.ts | 35 +++++++++++------ runner/e2e/style-panel.spec.ts | 15 ++++++++ runner/pipeline/theme-wiring.test.mjs | 45 ++++++++++++++++++++++ 4 files changed, 102 insertions(+), 14 deletions(-) diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index 4bc8f5827..bd999a83d 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -1070,6 +1070,11 @@ function Authoring({ * reading this replaced found no `\d+\.` in either, answered null, and so * handed a v16 core the prerelease pass-through (DEV-2571). */ const themingSupported = (() => { + // 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; })(); @@ -1107,6 +1112,10 @@ function Authoring({ 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 @@ -1116,6 +1125,12 @@ function Authoring({ // 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 @@ -1195,9 +1210,6 @@ function Authoring({ setDirtyPaths((prev) => (prev.size ? new Set() : prev)); }, []); - /** Replace the whole workspace (entry + files + lineage) and remount. */ - /** 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. */ /** 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 @@ -1208,6 +1220,8 @@ function Authoring({ [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(() => { if (!importSkipped.length) return null; const shown = importSkipped.slice(0, 2).map((s) => `${s.path} (${s.reason})`); @@ -1215,6 +1229,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 diff --git a/runner/apps/authoring/src/theme/codegen.ts b/runner/apps/authoring/src/theme/codegen.ts index 52a329ab4..11b215578 100644 --- a/runner/apps/authoring/src/theme/codegen.ts +++ b/runner/apps/authoring/src/theme/codegen.ts @@ -800,6 +800,12 @@ 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? * @@ -809,15 +815,15 @@ export function themeModulePath(files: Record): string { * 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. * - * Both extensions are scanned rather than just `themeModulePath(files)`: adding - * a `.ts` file to a JS demo flips that answer, and the stale module would then - * be invisible to the very check meant to find it. + * 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 Object.entries(files).some(([path, source]) => - path.includes(THEME_MODULE_BASENAME) - && typeof source === "string" - && source.includes("handsontable/themes")); + return THEME_MODULE_PATHS.some((path) => (files[path] ?? "").includes("handsontable/themes")); } /** @@ -894,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/e2e/style-panel.spec.ts b/runner/e2e/style-panel.spec.ts index ccc8f06dc..d85cf9082 100644 --- a/runner/e2e/style-panel.spec.ts +++ b/runner/e2e/style-panel.spec.ts @@ -544,6 +544,21 @@ test("a bare major deep link cannot open the Style panel", async ({ page }) => { 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 diff --git a/runner/pipeline/theme-wiring.test.mjs b/runner/pipeline/theme-wiring.test.mjs index 4d5b9a7e6..8462b27b5 100644 --- a/runner/pipeline/theme-wiring.test.mjs +++ b/runner/pipeline/theme-wiring.test.mjs @@ -232,6 +232,36 @@ out.__detect = { 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)); `; @@ -307,6 +337,7 @@ test("hasWiredTheme reads the module's contents, not its filename", { skip }, () 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. @@ -315,6 +346,20 @@ test("hasWiredTheme reads the module's contents, not its filename", { skip }, () 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. From 39165b2f5e9d8c646fb61a714f7747382b604ae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Thu, 20 Aug 2026 12:50:42 +0200 Subject: [PATCH 3/3] fix(runner): only strip a theme on a core we know is below the API (Bugbot #241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `themingSupported` gated two different questions, and the strip effect keyed on the wrong one. "Not themeable" now included *any* ref the validator refuses — which was right for the Style button and wrong for taking files out of the workspace. 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, none of them is a pre-17 core, and unwiring the visitor's theme over any of them destroys files to fix nothing — silently, and without `markDirty`, so nothing even marks the workspace as changed. Split into two predicates. `themingSupported` keeps its validity check and keeps gating the button and the panel. The strip now keys on `belowThemeApi` — a real release major, below the floor — which is the only state where the module's imports are known to be unresolvable. `selectedReleaseMajor` already answers null for everything carrying no comparable major (prereleases, pkg.pr.new refs, refused refs), so the positive test is the whole guard. Closing an open panel goes back to its own effect on `themingSupported`, since that half was never about the floor. `e2e/style-panel.spec.ts` pins it: a themed payload opened at `?v=latest` keeps its module, its wiring and its silence, while the Style button stays refused. Red against a70c4c32 — verified by checking that App.tsx out in place. 757 pipeline tests 0 fail 0 skipped, 200 deterministic e2e, 20 live, typechecks clean. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 45 ++++++++++++++++++++++--------- runner/e2e/style-panel.spec.ts | 26 ++++++++++++++++++ 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index bd999a83d..c330b3e89 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -1078,14 +1078,36 @@ function Authoring({ 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. - // - // And 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 + 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 @@ -1096,10 +1118,10 @@ function Authoring({ // (`StylePanel`), and reopening the panel on a supported core reconciles it // straight back into the demo. // - // `files` in the deps, not just `themingSupported`: 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. + // `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 @@ -1107,8 +1129,7 @@ function Authoring({ // broken module gets compiled once and only then repaired, which is the very // event this fixes. useEffect(() => { - if (themingSupported) return; - setStyleOpen(false); + if (!belowThemeApi) return; if (!hasWiredTheme(filesRef.current)) return; let next = filesRef.current; for (const change of buildResetChanges(next)) { @@ -1137,7 +1158,7 @@ function Authoring({ // run, not an edit the visitor made. Dirtying it would light up `Save •` on // a shared demo nobody has touched. setThemeRemoved(true); - }, [themingSupported, files]); + }, [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. * diff --git a/runner/e2e/style-panel.spec.ts b/runner/e2e/style-panel.spec.ts index d85cf9082..3b0ed5d60 100644 --- a/runner/e2e/style-panel.spec.ts +++ b/runner/e2e/style-panel.spec.ts @@ -591,6 +591,32 @@ const THEMED_AT_16 = { }, }; +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 }));