Merge pull request #2563 from heygen-com/via/thumbnail-id-escape

fix(studio,runtime): CSS.escape ids so digit-leading selectors don't crash

Author: vanceingalls

.fallowrc.jsonc

@@ -583,6 +583,16 @@
       "packages/studio/src/components/editor/PropertyPanel.test.tsx",
       "packages/studio/src/components/editor/propertyPanelFlatStyleSections.test.tsx",
       "packages/studio/src/components/editor/propertyPanelFlatMotionSection.test.tsx",
+      // Thumbnail id-escape fix (via/thumbnail-id-escape): getElementScreenshotClip's
+      // matches/rect/pad clip-computation body has a pre-existing 19-line clone with
+      // the inline `page.evaluate` block in vite.browser.ts (the dev-server thumbnail
+      // path). Adding a try/catch guard around the querySelectorAll call shifts the
+      // clone's line numbers, re-flagging the inherited duplication. Extracting the
+      // shared body into a common helper would require the browser-side page.evaluate
+      // to import from a Node-side module, which puppeteer's serialization boundary
+      // makes non-trivial.
+      "packages/studio-server/src/helpers/screenshotClip.ts",
+      "packages/studio/vite.browser.ts",
     ],
   },
   "health": {
@@ -798,6 +808,16 @@
       // `waitForCompletion` is untouched. Line-shift fingerprint re-flags
       // the inherited complexity.
       "packages/cli/src/commands/lambda/render.ts",
+      // Thumbnail id-escape fix (via/thumbnail-id-escape): picker.ts has five
+      // pre-existing inherited-complexity findings (isEffectivelyHidden,
+      // isPickableElement, buildElementLabel, getPickCandidatesFromPoint,
+      // pickManyAtPoint — all in this file at the parent SHA). This PR's only
+      // logic edit inside picker.ts is a single-line change to buildElementSelector
+      // (`#${htmlEl.id}` → `#${CSS.escape(htmlEl.id)}`) plus a three-line comment;
+      // the added lines shift every function below buildElementSelector, and Fallow's
+      // file-level fingerprint invalidation re-flags the inherited findings even at
+      // unchanged line numbers.
+      "packages/core/src/runtime/picker.ts",
     ],
   },
 }

packages/core/src/runtime/picker.test.ts

@@ -1,6 +1,30 @@
-import { describe, it, expect, vi, afterEach } from "vitest";
+import { describe, it, expect, vi, afterEach, beforeAll } from "vitest";
 import { createPickerModule } from "./picker";
 
+// jsdom does not implement CSS.escape — polyfill a compact spec-adjacent
+// version. Parallel (simpler) polyfills already live in compositionLoader.test.ts
+// / startResolver.test.ts, but they don't handle the leading-digit case this
+// test needs. Each test file runs in an isolated environment, so we duplicate
+// rather than import.
+beforeAll(() => {
+  const css = globalThis.CSS as { escape?: (input: string) => string } | undefined;
+  if (!css || typeof css.escape !== "function") {
+    (globalThis as { CSS?: { escape: (input: string) => string } }).CSS = {
+      ...(css ?? {}),
+      escape: (value: string) => {
+        // Non-word chars get a leading backslash (spec-adjacent).
+        const escaped = value.replace(/([^\w-])/g, "\\$1");
+        // A leading digit must be encoded as `\<hex> ` (space terminator) per CSS spec.
+        const first = value.charCodeAt(0);
+        if (first >= 48 && first <= 57) {
+          return `\\${first.toString(16)} ${escaped.slice(1)}`;
+        }
+        return escaped;
+      },
+    };
+  }
+});
+
 function createMockPostMessage() {
   return vi.fn();
 }
@@ -181,4 +205,52 @@ describe("createPickerModule", () => {
       expect(document.body.classList.contains("__hf-pick-active")).toBe(true);
     });
   });
+
+  describe("buildElementSelector escapes digit-leading ids", () => {
+    it('produces a CSS-valid selector for id="0" and picks the element back', () => {
+      // Regression: a user's HTML with id="0" (or any digit-leading id) used
+      // to produce the raw selector "#0", which is invalid per the CSS spec —
+      // downstream querySelector calls threw SyntaxError. buildElementSelector
+      // now CSS.escapes the id.
+      const picker = createPickerModule({ postMessage: createMockPostMessage() });
+      picker.installPickerApi();
+      const el = document.createElement("div");
+      el.id = "0";
+      Object.assign(el.style, {
+        position: "absolute",
+        left: "0px",
+        top: "0px",
+        width: "40px",
+        height: "40px",
+      });
+      document.body.appendChild(el);
+
+      // Force elementsFromPoint to hit our div so we exercise the real code
+      // path that calls buildElementSelector via extractElementInfo.
+      const originalElementsFromPoint = document.elementsFromPoint;
+      Object.defineProperty(document, "elementsFromPoint", {
+        configurable: true,
+        value: () => [el],
+      });
+      try {
+        const api = (
+          window as {
+            __HF_PICKER_API?: {
+              pickAtPoint?: (x: number, y: number) => { selector: string } | null;
+            };
+          }
+        ).__HF_PICKER_API;
+        const picked = api?.pickAtPoint?.(10, 10);
+        expect(picked?.selector).toBe("#\\30 ");
+        // And the round trip must find the element back through querySelector.
+        expect(() => document.querySelector(picked?.selector ?? "")).not.toThrow();
+        expect(document.querySelector(picked?.selector ?? "")).toBe(el);
+      } finally {
+        Object.defineProperty(document, "elementsFromPoint", {
+          configurable: true,
+          value: originalElementsFromPoint,
+        });
+      }
+    });
+  });
 });

packages/core/src/runtime/picker.ts

@@ -95,7 +95,10 @@ export function createPickerModule(deps: PickerModuleDeps): PickerModule {
 
   function buildElementSelector(el: Element): string {
     const htmlEl = el as HTMLElement;
-    if (htmlEl.id) return `#${htmlEl.id}`;
+    // Escape the ID so digit-leading or otherwise CSS-illegal ids (e.g. `#0`,
+    // `#1`) produce valid selectors — `document.querySelector("#0")` throws
+    // SyntaxError per the CSS spec. Sibling branches below already escape.
+    if (htmlEl.id) return `#${CSS.escape(htmlEl.id)}`;
     const compositionId = el.getAttribute("data-composition-id");
     if (compositionId) return `[data-composition-id="${CSS.escape(compositionId)}"]`;
     const compositionSrc = el.getAttribute("data-composition-src");

packages/studio-server/src/helpers/screenshotClip.test.ts

@@ -0,0 +1,54 @@
+import { afterEach, describe, expect, it } from "vitest";
+import { getElementScreenshotClip } from "./screenshotClip";
+
+afterEach(() => {
+  document.body.innerHTML = "";
+});
+
+describe("getElementScreenshotClip", () => {
+  it("returns undefined (not throws) when the selector is CSS-invalid", () => {
+    // Regression: an HTML element with `id="0"` produces the selector `#0`,
+    // which is invalid per the CSS spec — `document.querySelectorAll('#0')`
+    // throws SyntaxError. Puppeteer surfaces that as a page.evaluate error,
+    // which used to bubble up and fail the whole thumbnail. The clip helper
+    // now swallows the SyntaxError so callers fall back to a full-page shot.
+    const el = document.createElement("div");
+    el.id = "0";
+    Object.assign(el.style, {
+      width: "100px",
+      height: "80px",
+    });
+    document.body.appendChild(el);
+
+    expect(() => getElementScreenshotClip("#0")).not.toThrow();
+    expect(getElementScreenshotClip("#0")).toBeUndefined();
+  });
+
+  it("returns undefined (not throws) for garbage selectors", () => {
+    expect(() => getElementScreenshotClip("::: garbage :::")).not.toThrow();
+    expect(getElementScreenshotClip("::: garbage :::")).toBeUndefined();
+  });
+
+  it("returns a clip for a well-formed selector matching a visible element", () => {
+    const el = document.createElement("div");
+    el.id = "hero";
+    el.getBoundingClientRect = () =>
+      ({
+        left: 10,
+        top: 20,
+        width: 100,
+        height: 80,
+        right: 110,
+        bottom: 100,
+        x: 10,
+        y: 20,
+        toJSON: () => ({}),
+      }) as DOMRect;
+    document.body.appendChild(el);
+
+    const clip = getElementScreenshotClip("#hero");
+    expect(clip).toBeDefined();
+    expect(clip?.width).toBeGreaterThan(0);
+    expect(clip?.height).toBeGreaterThan(0);
+  });
+});

packages/studio-server/src/helpers/screenshotClip.ts

@@ -9,9 +9,19 @@ export function getElementScreenshotClip(
   selector: string,
   selectorIndex?: number,
 ): ScreenshotClip | undefined {
-  const matches = Array.from(document.querySelectorAll(selector)).filter(
-    (el): el is HTMLElement => el instanceof HTMLElement,
-  );
+  // Guard against invalid CSS selectors (e.g. `#0` — a digit-leading id from
+  // user HTML that upstream producers forgot to CSS.escape). querySelectorAll
+  // throws SyntaxError on those, which bubbles out of page.evaluate and fails
+  // the whole thumbnail. Returning undefined here falls back to a full-page
+  // screenshot, so the user still sees a thumbnail instead of a broken image.
+  let matches: HTMLElement[];
+  try {
+    matches = Array.from(document.querySelectorAll(selector)).filter(
+      (el): el is HTMLElement => el instanceof HTMLElement,
+    );
+  } catch {
+    return undefined;
+  }
   const safeIndex = Math.max(0, Math.min(matches.length - 1, Math.floor(selectorIndex ?? 0)));
   const el = matches[safeIndex] ?? null;
   if (!(el instanceof HTMLElement)) return undefined;