packages/docs/test/model/color.test.ts (1)

16-20: ⚡ Quick win

Coverage gap: wrapLegacyColor only exercises the passthrough branch.

The single test only asserts string passthrough. Given the function is named wrapLegacy, the more interesting paths — what it produces for legacy-shaped inputs and idempotency on already-wrapped ThemeColor values — are not covered. Since legacy migration is one of the headline behaviors of this PR, a thin extra test or two here would meaningfully harden the contract.

🧪 Suggested additions

it('passes through an already-wrapped srgb value (idempotent)', () => {
  const wrapped = { kind: 'srgb' as const, value: '#abc' };
  expect(wrapLegacyColor(wrapped)).toEqual(wrapped);
});

it('passes through a role-bound color unchanged', () => {
  const role = { kind: 'role' as const, role: 'accent1' as const };
  expect(wrapLegacyColor(role)).toEqual(role);
});

Adjust to match the actual signature/return contract of wrapLegacyColor.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/docs/test/model/color.test.ts` around lines 16 - 20, Add tests to
cover non-passthrough branches of wrapLegacyColor: add one test that passes an
already-wrapped ThemeColor object (e.g., { kind: 'srgb', value: '#abc' }) and
asserts wrapLegacyColor(...) returns the same object (idempotency), and another
test that passes a role-shaped color (e.g., { kind: 'role', role: 'accent1' })
and asserts it is returned unchanged; place these in the same
describe('wrapLegacyColor', ...) block in packages/docs/test/model/color.test.ts
and ensure expectations use toEqual to compare objects and match the function's
actual return shape.

packages/slides/package.json (2)

23-24: 💤 Low value

Optional: tighten the visual.test selector.

vitest run visual.test matches any file whose path contains the substring visual.test (e.g. a hypothetical visual.test.helpers.ts). If you intend to run only the two visual suites, a more specific glob like '**/*.visual.test.ts' is less prone to drift as the test tree grows.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/package.json` around lines 23 - 24, The package.json visual
test scripts ("test:visual" and "test:visual:update") use the loose selector
"visual.test" which matches any path containing that substring; tighten both
script commands to use a more specific glob (for example
"'**/*.visual.test.ts'") so only intended visual suites run and avoid accidental
matches as the test tree grows.

---

34-34: ⚡ Quick win

Consider canvas@^3 instead of ^2.11.2 for the visual-test dependency.

canvas 3.x is the current major (latest is 3.2.3) and brings the N-API migration, fixed Node 24 crash via external memory API change, and better long-term compatibility. v2.11.2 is the last 2.x and is increasingly fragile on newer Node versions.

Since this is a devDependency for INCLUDE_VISUAL_TESTS=1, the upgrade is low-risk; the API surface used by node-canvas in tests (createCanvas, 2D context) is compatible across 2.x → 3.x. The codebase requires Node.js >=18.12.0 for canvas 3.x, which is not constrained elsewhere in the project.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/package.json` at line 34, The package.json currently pins
"canvas": "^2.11.2" for visual tests; update that dependency to a 3.x range
(e.g. "canvas": "^3") in packages/slides/package.json, then reinstall dev deps
(npm/yarn install or pnpm install) and run the visual tests with
INCLUDE_VISUAL_TESTS=1 to ensure createCanvas/2D context usage remains
compatible; also update any lockfile (package-lock.json / pnpm-lock.yaml /
yarn.lock) and CI dev matrix if it pins older Node versions to ensure Node
>=18.12.0 is used for canvas@^3.

packages/slides/src/store/memory.test.ts (1)

132-143: 💤 Low value

LGTM — solid immutability check for the new ThemeColor fill shape.

Mutating bg.fill.value after the write and asserting via the discriminated kind narrowing is exactly the right test for the deep-clone contract. One small enhancement worth considering: also reassign bg.fill = { kind: 'srgb', value: '#00ff00' } (or pass through JSON.parse(JSON.stringify(...)) round-trip) to catch a future regression where only top-level fields are cloned. Optional — the current assertion already covers the realistic caller-mutation scenario.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/src/store/memory.test.ts` around lines 132 - 143, Add a
stronger immutability check to the test "updateSlideBackground stores a clone,
not a reference": after calling store.updateSlideBackground(id, bg) keep the
existing mutation of bg.fill.value and also reassign bg.fill = { kind: 'srgb',
value: '#00ff00' } (or equivalent JSON round-trip) to ensure only a deep clone
of the entire fill object was stored; then read the store via
store.read().slides[0].background.fill and assert the stored fill still has kind
'srgb' and value '#ff0000'. Reference MemSlidesStore, updateSlideBackground,
read(), slides[0].background.fill and the local bg variable when making the
change.

packages/slides/src/themes/streamline.ts (1)

11-16: 💤 Low value

Consider whether the all-blue accent palette gives enough perceptual differentiation.

accent1–accent6 are all sequential shades of the same blue (#1976D2 → #E3F2FD). For chart series, callouts, or any UI that relies on accent index to convey distinct categories, neighboring accents (e.g. accent1/accent3, accent4/accent5) will be hard to tell apart. The default-dark theme by contrast uses six visibly distinct hues.

Additionally, accent6 (#E3F2FD) on the background (#FAFAFA) has very low contrast and will read as a near-invisible fill on light surfaces — fine for tints, but worth confirming this is the intended role.

If "Streamline" is deliberately monochromatic, consider a brief code comment noting that, so future authors don't "fix" it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/src/themes/streamline.ts` around lines 11 - 16, The current
Streamline theme uses a sequential monochromatic blue for accent1..accent6 which
reduces perceptual differentiation and makes accent6 (`#E3F2FD`) low-contrast
against background (`#FAFAFA`); update the accent1–accent6 values in the theme
object to provide more distinct hues (or at least increase contrast for accent6)
so series/callouts are visually separable, or if the monochrome palette is
intentional add a code comment next to the theme object (e.g., near
accent1..accent6) stating that Streamline is deliberately monochromatic and
noting accent6 is a tint for subtle UI elements only. Ensure any changed colors
preserve overall theme intent and that accent names (accent1..accent6) and
background are the targeted symbols.

packages/slides/vite.config.ts (1)

26-29: ⚡ Quick win

Use configDefaults.exclude to avoid silently dropping Vitest's built-in exclusions.

Setting exclude in config replaces the default value entirely. To extend the default exclude patterns, configDefaults from vitest/config should be used. The current code drops Vitest's built-in exclusions for hidden directories (**/.{idea,git,cache,output,temp}/**) and tool config files (**/{vitest,vite,...}.config.*). While harmless for this bounded package today, it's a latent footgun and the duplication of node_modules/dist across both branches can be eliminated for free.

♻️ Proposed refactor using `configDefaults`

-import { defineConfig } from 'vitest/config';
+import { configDefaults, defineConfig } from 'vitest/config';

 ...

     exclude:
       process.env.INCLUDE_VISUAL_TESTS === '1'
-        ? ['**/node_modules/**', '**/dist/**']
-        : ['**/node_modules/**', '**/dist/**', '**/*.visual.test.ts'],
+        ? [...configDefaults.exclude]
+        : [...configDefaults.exclude, '**/*.visual.test.ts'],

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/vite.config.ts` around lines 26 - 29, The Vitest config
currently sets exclude directly (based on process.env.INCLUDE_VISUAL_TESTS)
which replaces Vitest's built-in exclusions; instead import configDefaults from
'vitest/config' and set exclude = [...configDefaults.exclude,
'**/node_modules/**', '**/dist/**', ...(process.env.INCLUDE_VISUAL_TESTS === '1'
? [] : ['**/*.visual.test.ts'])] so you preserve default exclusions, remove
duplicate '**/node_modules/**' and '**/dist/**' across branches, and only add
the visual test pattern when visual tests are disabled; update the config export
where "exclude" is defined to use this merged array.

docs/tasks/active/20260507-slides-themes-layouts-import-todo.md (1)

54-58: 💤 Low value

Create the paired lessons file alongside the todo.

The coding guidelines require a YYYYMMDD-<slug>-lessons.md alongside every *-todo.md. Line 57 tracks this as a future task, but the file is expected to exist now (even if empty). Consider adding docs/tasks/active/20260507-slides-themes-layouts-import-lessons.md in this PR so the pair is complete from the start.

Based on coding guidelines: "Non-trivial tasks use paired files in docs/tasks/active/: YYYYMMDD-<slug>-todo.md and YYYYMMDD-<slug>-lessons.md."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/tasks/active/20260507-slides-themes-layouts-import-todo.md` around lines
54 - 58, Add the paired lessons file for the task by creating
docs/tasks/active/20260507-slides-themes-layouts-import-lessons.md (matching the
todo slug) even if empty; ensure the new file follows the
YYYYMMDD-<slug>-lessons.md naming convention and is committed in this PR so the
todo and lessons pair exist together as required by the coding guidelines.

packages/slides/src/view/editor/editor.ts (1)

733-755: ⚖️ Poor tradeoff

paintLive and currentSlide() each call store.read() independently from render()'s snapshot.

paintLive (line 745) and currentSlide() (line 754) each issue a fresh store.read(), while render() already has a snapshot in scope. On every mousemove during drag/resize/rotate, this results in two store reads: one for the live doc (theme colors in forceRender) and one inside currentSlide() called by repaintOverlay. Passing the already-read doc into these helpers would halve store reads on hot paths.

♻️ Sketch: thread `doc` through `paintLive` to avoid the second read

-  private paintLive(live: Map<string, Frame>): void {
-    const slide = this.currentSlide();
+  private paintLive(live: Map<string, Frame>, doc?: SlidesDocument): void {
+    const d = doc ?? this.options.store.read();
+    const id = this.currentId;
+    const slide = id ? d.slides.find((s) => s.id === id) : undefined;
     if (!slide) return;
     const synthetic = { ... };
-    this.renderer.forceRender(synthetic, this.options.store.read());
+    this.renderer.forceRender(synthetic, d);
     ...
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/src/view/editor/editor.ts` around lines 733 - 755, The
paintLive and currentSlide helpers each call options.store.read() causing two
reads per mousemove; change currentSlide to accept a doc parameter (e.g.,
currentSlide(doc)) and change paintLive to accept the already-read doc (e.g.,
paintLive(live, doc)), then update render to pass its snapshot/doc into
paintLive instead of letting those methods call options.store.read(); inside
paintLive use the passed doc when constructing synthetic and when selecting
elements for renderOverlay and call this.renderer.forceRender(synthetic, doc) so
forceRender and renderOverlay use the single snapshot rather than performing a
second store.read().

packages/slides/src/view/canvas/element-renderer.ts (1)

14-22: 💤 Low value

doc is on the public surface but unused; either wire it or drop it.

The JSDoc on lines 14–17 says doc is threaded in so shape/text painters can resolve theme bindings, but only theme is forwarded to drawShape/drawText; doc is silenced via void doc;. Every caller (and now every test) has to construct a SlidesDocument for an argument the implementation discards. Since theme resolution is already covered by the theme argument, consider dropping doc until the planned colorResolver wiring actually consumes it, or at least correct the JSDoc so it doesn't promise behavior the painters don't currently rely on.

Also applies to: 26-28, 59-62

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/src/view/canvas/element-renderer.ts` around lines 14 - 22,
The public parameter doc (SlidesDocument) is declared but unused (silenced via
`void doc;`) while only `theme` is forwarded to drawShape/drawText; either
remove `doc` from the public API and update the JSDoc, or actually thread it
through to the painters: stop silencing `doc`, pass it into `drawShape` and
`drawText` (and any other painter calls that currently receive `theme`), and
ensure the JSDoc comment reflects that painters can resolve ThemeColor/ThemeFont
via the provided `doc` and `theme`; also apply the same change pattern where
`doc` is similarly ignored (the other occurrences noted near the
drawShape/drawText usages and the onAssetLoad handling).

packages/docs/test/view/themed-color.test.ts (1)

86-92: 💤 Low value

Strengthen the fallback assertion.

expect(fillStyles.some((s) => s.startsWith('#'))).toBe(true) is satisfied as long as any hex value reached fillStyle at any point during paint — including an unrelated background or stroke color, or a default that the test setup itself wrote. If the text run silently no-ops on undefined, this check still passes. Consider asserting the specific theme default the renderer is expected to fall back to (e.g. '#000000' from defaultLight.colors.text) so a regression in the fallback path is actually caught.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/docs/test/view/themed-color.test.ts` around lines 86 - 92, The test
currently only checks that some fillStyle starts with '#' which can pass for
unrelated colors; update the assertion to verify the renderer fell back to the
theme default by asserting that fillStyles includes the expected theme text
color (use the known value like defaultLight.colors.text or its literal
'#000000') instead of the generic startsWith check; locate the test around
paintLayout(ctx, layout, 0, 0) and replace or add an expectation referencing
fillStyles and defaultLight.colors.text (or the literal) so the fallback from
defaultColorResolver is explicitly asserted.

packages/slides/src/view/canvas/text-renderer.ts (1)

43-55: 💤 Low value

Consider naming the '#000000' sentinel constant.

The magic literal ties this function to DEFAULT_INLINE_STYLE.color in the docs package. If that default ever changes, the remapping silently breaks. The comment is thorough, but a named constant (const DOCS_DEFAULT_TEXT_COLOR = '#000000') would make the coupling explicit and searchable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/src/view/canvas/text-renderer.ts` around lines 43 - 55, The
function makeColorResolver uses the magic literal '#000000' to detect the docs
package default text color; extract that string into a named constant (e.g.,
DOCS_DEFAULT_TEXT_COLOR = '#000000') and replace the inline literal in the
conditional with this constant so the coupling to DEFAULT_INLINE_STYLE.color is
explicit and searchable; update the comment to reference the new constant and
keep the behavior in makeColorResolver, which handles StoredColor/ThemeColor
resolution, unchanged otherwise.

packages/docs/src/view/table-renderer.ts (1)

171-180: 💤 Low value

Cell-level backgroundColor bypasses defaultColorResolver while inline run backgrounds do not.

Line 173 sets ctx.fillStyle = cell.style.backgroundColor directly (raw string), while lines 220–222 route the inline run's style.backgroundColor through defaultColorResolver. If the model ever evolves to allow ThemeColor-shaped objects at the cell level, this path would silently render the object's toString() representation instead of the resolved color.

This is currently safe since cell.style.backgroundColor is always a plain string, but flagging it for awareness if the model is extended.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/docs/src/view/table-renderer.ts` around lines 171 - 180, The cell
background path in table-renderer.ts assigns ctx.fillStyle directly from
cell.style.backgroundColor which bypasses defaultColorResolver; update the cell
background branch in the renderer (the code that currently sets ctx.fillStyle =
cell.style.backgroundColor and calls ctx.fillRect) to pass
cell.style.backgroundColor through defaultColorResolver (same resolver used for
inline runs) and use the resolved value for ctx.fillStyle, guarding for
undefined/null so you only set fillStyle and call ctx.fillRect when a resolved
color string is returned.

packages/frontend/tests/app/slides/themed-color-picker.test.ts (1)

23-37: 💤 Low value

Test name claims OOXML ordering but assertions only check membership.

THEME_ROLES.includes(...) does not verify the slot order, so the "in OOXML order" part of the description is unverified. Either rename to "has all 12 ColorScheme slots" or add an order-sensitive assertion (e.g., assert.deepEqual(THEME_ROLES, ['text','background', ...])).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/frontend/tests/app/slides/themed-color-picker.test.ts` around lines
23 - 37, The test name claims OOXML ordering but currently only checks
membership; update the test for THEME_ROLES to either rename the test to "has
all 12 ColorScheme slots" or perform an order-sensitive assertion: replace the
individual includes checks with a single equality check asserting THEME_ROLES
exactly equals the expected OOXML-ordered array (e.g.,
assert.deepEqual(THEME_ROLES,
['text','background','textSecondary','backgroundAlt','accent1','accent2','accent3','accent4','accent5','accent6','hyperlink','visitedHyperlink']));
keep the length check if desired and remove the redundant includes assertions
when using deepEqual.

packages/frontend/src/app/slides/slides-formatting-toolbar.tsx (1)

256-266: 💤 Low value

Font picker value is hardcoded to undefined, so the active font is never highlighted.

The shape-fill picker reads the live value via readShapeFill and reflects the current selection in its swatch state, but the font picker passes value={undefined} regardless — so even when a text element has a uniform font (the common case after onTextFontChange runs), the picker can't show which row is "active". Cheap symmetry win with the fill picker.

A reasonable v0.5 cut is to sample the first run of the first block (matching the write path which sets every run uniformly) and pass that as value.

♻️ Sketch — derive font from first run

+  const textFontValue: ThemeFont | undefined = (() => {
+    if (!isText || !theme || !store) return undefined;
+    const first = store
+      .read()
+      .slides.find((s) => s.id === selected!.slideId)
+      ?.elements.find((e) => e.id === selected!.element.id);
+    // Read fontFamily off the first run of the first block; matches the
+    // uniform-write path in onTextFontChange.
+    const family = first && first.type === "text"
+      ? first.data.blocks[0]?.inlines[0]?.style.fontFamily
+      : undefined;
+    return family ? { kind: "family", family } : undefined;
+  })();
   ...
-          <ThemedFontPicker
-            value={undefined}
+          <ThemedFontPicker
+            value={textFontValue}
             theme={theme}
             onChange={onTextFontChange}
             hint={fontHint}
           />

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/frontend/src/app/slides/slides-formatting-toolbar.tsx` around lines
256 - 266, The ThemedFontPicker is always passed value={undefined}, so the
active font never highlights; update the DropdownMenuContent usage to compute
the current font from the selected text element (same approach as your write
path): read the first block's first run font (e.g. sample
selection.blocks[0].runs[0].font or via the existing helper used when applying
fonts) and pass that string as the value prop to ThemedFontPicker (keep theme,
onTextFontChange, and hint as-is) so the picker reflects the current selection.

packages/slides/src/model/theme.ts (1)

54-57: ⚡ Quick win

parseHex silently produces NaN for 3-char shorthand hex.

parseHex('#fff') returns [255, 15, NaN] because the slices assume a 6-digit form. Any tint/shade applied to such a color cascades through Math.max(0, Math.min(255, NaN)) = NaN and emits a '#NaNNaNNaN'-shaped string, silently corrupting the paint.

Built-in theme files most likely use 6-char hex so production is unaffected today, but the type signature accepts any string and authored themes (theme builder is PR3 of this rollout) commonly use shorthand. Cheap to harden now.

♻️ Proposed handling for shorthand + invalid hex

-function parseHex(hex: string): [number, number, number] {
-  const h = hex.startsWith('#') ? hex.slice(1) : hex;
-  return [parseInt(h.slice(0, 2), 16), parseInt(h.slice(2, 4), 16), parseInt(h.slice(4, 6), 16)];
-}
+function parseHex(hex: string): [number, number, number] {
+  const raw = hex.startsWith('#') ? hex.slice(1) : hex;
+  // Expand shorthand `abc` → `aabbcc` so theme authors can use either form.
+  const h = raw.length === 3
+    ? raw.split('').map((c) => c + c).join('')
+    : raw;
+  if (h.length !== 6 || !/^[0-9a-fA-F]{6}$/.test(h)) {
+    throw new Error(`Invalid hex color: ${hex}`);
+  }
+  return [
+    parseInt(h.slice(0, 2), 16),
+    parseInt(h.slice(2, 4), 16),
+    parseInt(h.slice(4, 6), 16),
+  ];
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/slides/src/model/theme.ts` around lines 54 - 57, parseHex currently
assumes a 6-digit hex and yields NaN for 3-char shorthand; update parseHex to
first strip a leading '#', detect 3-character shorthand (e.g. 'fff') and expand
it to 6 characters ('ffffff'), then validate the resulting string with a hex
regex /^[0-9A-Fa-f]{6}$/; if validation fails, throw a descriptive Error
including the original input so callers can surface bad theme values. Ensure the
function still returns a [number, number, number] tuple after parsing the
validated 6-char string.

packages/frontend/src/app/slides/yorkie-slides-store.ts (1)

2-17: ⚡ Quick win

Reuse the shared DEFAULT_BACKGROUND here.

This now duplicates the model default from packages/slides/src/model/presentation.ts. If the shared default changes again, Yorkie-backed new slides and memory/migrated slides will silently diverge.

♻️ Suggested change

 import {
+  DEFAULT_BACKGROUND as MODEL_DEFAULT_BACKGROUND,
   DEFAULT_MASTER,
   type Background,
   type ElementInit,
   type Frame,
   type Layout,
   type Master,
   type SlidesDocument,
   type SlidesStore,
   type Theme,
   BUILT_IN_LAYOUTS,
   defaultLight,
   generateId,
   getLayout,
   migrateDocument,
 } from '@wafflebase/slides';
@@
-const DEFAULT_BACKGROUND: YorkieSlide['background'] = {
-  fill: { kind: 'role', role: 'background' },
-};

Then use the shared constant at slide creation time:

background: clone(MODEL_DEFAULT_BACKGROUND) as YorkieSlide['background'],

Also applies to: 27-29

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/frontend/src/app/slides/yorkie-slides-store.ts` around lines 2 - 17,
The slide default background is duplicated — import and reuse the shared
DEFAULT_BACKGROUND (or MODEL_DEFAULT_BACKGROUND from the slides model) instead
of re-defining it, and when constructing Yorkie-backed slides assign a cloned
copy (e.g. clone(DEFAULT_BACKGROUND) cast to YorkieSlide['background']) wherever
the current hardcoded/default background is used (also update the other
occurrences mentioned around the slide creation code that set background).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/frontend/src/app/harness/visual/slides-scenarios.tsx (1)

305-319: ⚡ Quick win

data-visual-scenario-ready="true" is set before the canvas is painted

The data-visual-scenario-ready attribute is hardcoded unconditionally on the Card element, so it is present in the DOM from the very first React commit — before the SlideCanvas useEffect has run and before SlideRenderer.render() has drawn anything to the canvas.

captureScreenshots in the script resolves per-target via locator.waitFor({ state: "visible" }) (script line 221), which unblocks as soon as the card element is visible — not after the canvas effect fires. On a slow CI runner or a loaded headless browser process, load states only reflect document and network activity; they do not guarantee that dynamic components or client-side rendering are complete, making this a real flakiness risk for the five slides-canvas-* scenarios.

A robust fix is to have SlideCanvas set a data-canvas-ready attribute after the effect completes, and have the harness script wait for it:

💡 Proposed fix

In SlideCanvas:

 function SlideCanvas({ doc, width = 480, height = 270 }: ...) {
   const ref = useRef<HTMLCanvasElement | null>(null);
+  const [ready, setReady] = useState(false);

   useEffect(() => {
     const canvas = ref.current;
     if (!canvas) return;
     canvas.width = width;
     canvas.height = height;
     const ctx = canvas.getContext("2d");
     if (!ctx) return;
     const renderer = new SlideRenderer(ctx, { hostWidth: width, hostHeight: height, dpr: 1 });
     renderer.render(doc.slides[0], doc);
+    setReady(true);
   }, [doc, width, height]);

   return (
     <canvas
       ref={ref}
+      data-canvas-ready={ready ? "true" : undefined}
       className="rounded-md border bg-background"
       style={{ width, height }}
     />
   );
 }

In SlidesScenarioCard, replace the hardcoded attribute with one derived from the scenario type:

 <Card
   data-visual-scenario-id={scenario.id}
-  data-visual-scenario-ready="true"
-  data-visual-scenario-state="ready"
+  data-visual-scenario-ready={scenario.id.startsWith("slides-canvas-") ? undefined : "true"}

And in verify-visual-browser.mjs, add a canvas-specific wait after the section wait:

// After the section is visible, wait for each canvas scenario to be painted:
for (const canvasId of scenarioIds.filter((id) => id.startsWith("slides-canvas-"))) {
  await page
    .locator(`[data-visual-scenario-id='${canvasId}'] canvas[data-canvas-ready='true']`)
    .waitFor({ state: "attached", timeout: 10000 });
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/frontend/src/app/harness/visual/slides-scenarios.tsx` around lines
305 - 319, The Card currently hardcodes data-visual-scenario-ready="true" which
causes the harness to believe canvas scenarios are ready before SlideCanvas's
effect finishes; remove the unconditional attribute from SlidesScenarioCard and
instead have SlideCanvas set a data-canvas-ready="true" attribute on its
rendered <canvas> after SlideRenderer.render() completes (e.g., in the useEffect
callback). Keep any non-canvas scenarios using a readiness attribute if needed
(use scenario.type or scenario.id to discriminate) and update the visual test
harness (verify-visual-browser.mjs) to, after the section becomes visible, wait
specifically for canvas scenarios by awaiting
page.locator(`[data-visual-scenario-id='${id}']
canvas[data-canvas-ready='true']`). Ensure you reference SlidesScenarioCard and
SlideCanvas when making these changes.

packages/frontend/scripts/verify-visual-browser.mjs (1)

195-226: ⚡ Quick win

Missing section-level readiness wait for slides — inconsistent with other sections

The harness explicitly waits for sheet, format, and chart sections with data-visual-*-ready guards (lines 204–217) before starting per-target captures. There is no corresponding wait for the slides section. While the static data-visual-slides-ready="true" on the section element would resolve immediately and isn't a useful synchronization point on its own, the gap in the pattern means the canvas scenarios rely entirely on networkidle + document.fonts.ready for timing — which, as noted above in slides-scenarios.tsx, does not guarantee useEffect completion on the canvas.

The concrete fix for canvas readiness belongs in slides-scenarios.tsx (adding a data-canvas-ready signal), but this script should also add a wait to consume it before the per-target capture loop starts:

💡 Suggested addition after line 217

const slidesSection = page.locator(
  "[data-testid='visual-harness-slides-section'][data-visual-slides-ready='true']",
);
await slidesSection.waitFor({ state: "visible", timeout: 20000 });

// Wait for all canvas scenarios to finish rendering before capture.
for (const id of scenarioIds.filter((id) => id.startsWith("slides-canvas-"))) {
  await page
    .locator(`[data-visual-scenario-id='${id}'] canvas[data-canvas-ready='true']`)
    .waitFor({ state: "attached", timeout: 10000 });
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/frontend/scripts/verify-visual-browser.mjs` around lines 195 - 226,
Add a slides-section readiness wait and consume per-canvas readiness before the
per-target capture loop: insert a wait for the locator
"[data-testid='visual-harness-slides-section'][data-visual-slides-ready='true']"
(e.g. as slidesSection.waitFor(...)) and then, for each id in scenarioIds that
startsWith "slides-canvas-", wait on the page locator
`[data-visual-scenario-id='${id}'] canvas[data-canvas-ready='true']` to be
attached/visible with a reasonable timeout; place this logic immediately before
the for (const target of visualTargets) loop so captures
(captures.set(captureKey(profile.id, target.id), ...)) only run after canvas
scenarios are signalled ready.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes