fix(runner): take the theme module out below the theme API (DEV-2571)

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<string, string> = {
   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<string | null>(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

runner/apps/authoring/src/theme/codegen.ts

@@ -800,6 +800,32 @@ export function themeModulePath(files: Record<string, string>): 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<string, string>): 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<string, string>): 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;
 }