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

18-21: ⚡ Quick win

Consider expanding the comment to explain why explicit preservation is needed.

The current comment states what the code does, but the lessons doc reveals an important why: migrateDocument reconstructs meta from scratch (lines 13-17), so any new optional field will be dropped unless explicitly preserved here. This is a recurring pattern that future contributors adding Meta fields should understand.

📚 Suggested enhancement

- // Preserve the optional unit field if present and valid.
+ // migrateDocument reconstructs meta from a fixed literal (lines 13-17),
+ // so optional fields like unit are dropped unless explicitly preserved.
+ // Every new Meta field needs a similar preservation block.
  if (raw?.meta?.unit === 'in' || raw?.meta?.unit === 'cm') {
    meta.unit = raw.meta.unit;
  }

As per coding guidelines, "Write clear comments explaining the 'why' behind complex logic, not just the 'what'". The lesson doc at docs/tasks/archive/2026/05/20260529-slides-format-options-panel-lessons.md lines 5-16 identifies this as a recurring pattern that surprised the implementer.

🤖 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/migrate.ts` around lines 18 - 21, The comment above
the unit-preservation branch is too terse; update it to explain why explicit
preservation is required by noting that migrateDocument reconstructs meta from
scratch (see migrateDocument) so any optional fields like meta.unit would be
dropped unless explicitly carried over; modify the comment near the if
(raw?.meta?.unit === 'in' || raw?.meta?.unit === 'cm') block to state that this
preserves optional Meta fields across migration because migrateDocument rebuilds
meta and would otherwise omit newly added optional properties.

packages/slides/test/store/mem-set-unit.test.ts (2)

4-28: ⚖️ Poor tradeoff

Consider adding test coverage for YorkieSlidesStore.setUnit.

The test suite only covers MemSlidesStore, but YorkieSlidesStore is the production implementation used in the collaborative editor. Test coverage for both implementations helps ensure consistency and catch implementation-specific bugs. As per coding guidelines, critical business logic should have test coverage.

🤖 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/test/store/mem-set-unit.test.ts` around lines 4 - 28, Add
equivalent tests for YorkieSlidesStore.setUnit alongside the existing
MemSlidesStore cases: create a YorkieSlidesStore instance and mirror the four
tests (default meta.unit undefined, setUnit('cm') writes 'cm', setUnit('in')
writes 'in', and setUnit rejects invalid values) in the same test file (or a new
parallel test file) to ensure parity; reference the YorkieSlidesStore class and
its setUnit method, use its batch API like the MemSlidesStore tests, and assert
via yorkieStore.read().meta.unit and expect(...).toThrow(/invalid unit/i) for
the invalid case.

---

4-28: ⚡ Quick win

Add test coverage for batch requirement enforcement.

The test suite should verify that calling setUnit outside of a batch() throws an error, as this is a critical contract of the store interface. As per coding guidelines, tests should cover critical business logic and edge cases.

🧪 Suggested test case

it('setUnit throws when called outside batch', () => {
  const store = new MemSlidesStore();
  expect(() => store.setUnit('cm')).toThrow(/batch/i);
});

🤖 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/test/store/mem-set-unit.test.ts` around lines 4 - 28, Add a
test to enforce the batch requirement for MemSlidesStore.setUnit: in the
MemSlidesStore.setUnit test suite, add a test that instantiates MemSlidesStore
and asserts that calling store.setUnit('cm') outside of store.batch(...) throws
an error matching /batch/i; reference the MemSlidesStore class and setUnit
method to locate where to add the new test.

packages/frontend/tests/app/slides/format-panel/image-adjustments-section.test.tsx (1)

50-62: ⚡ Quick win

Add coverage for the keyboard commit path.

This suite only verifies pointerUp. Once the keyboard commit gap in image-adjustments-section.tsx is addressed, add a case that fires keyUp after a value change and asserts onCommit is invoked, to lock in the accessibility fix.

🤖 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/format-panel/image-adjustments-section.test.tsx`
around lines 50 - 62, Add a new test in image-adjustments-section.test.tsx that
mirrors the existing pointerUp case but exercises the keyboard commit path:
render ImageAdjustmentsSection with elements like img('a', 1) and img('b', 1)
and a mock onCommit, change the transparency slider
(screen.getByLabelText(/transparency/i)) to a new value, then fire a keyUp event
on the slider and assert onCommit was called with ['a','b'] and the expected
opacity (e.g., 0.6); this ensures ImageAdjustmentsSection's keyboard commit path
invokes the same onCommit handler as pointerUp.

packages/frontend/src/app/slides/format-panel/size-position-section.tsx (1)

69-73: ⚡ Quick win

Lock reset depends on elements array identity, which may be fragile.

The effect resets locked whenever the elements reference changes. If the parent (FormatPanel) derives elements by recreating the array each render (e.g. selection.map(...)/filter(...)), any unrelated re-render that preserves the same selection will produce a new array reference and silently flip locked back to false mid-edit. Keying on a stable selection identity avoids this.

♻️ Key the reset on selection ids

   const [locked, setLocked] = useState(false);
   // Reset lock state when the selection changes.
-  useEffect(() => {
-    setLocked(false);
-  }, [elements]);
+  const selectionKey = ids.join('\u0000');
+  useEffect(() => {
+    setLocked(false);
+  }, [selectionKey]);

Confirm whether FormatPanel passes a referentially stable elements array:

#!/bin/bash
# Inspect how `elements` is built/passed to SizePositionSection in the panel shell.
fd -e tsx -e ts . --full-path 'format-panel/index' --exec cat {}
rg -nP -C4 '<SizePositionSection' --type=tsx

🤖 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/format-panel/size-position-section.tsx`
around lines 69 - 73, The effect resetting locked currently depends on the
referential identity of elements (useEffect(() => setLocked(false),
[elements])), which is fragile; change it to depend on a stable selection
identity such as the set of element ids instead: derive a stable array or string
of ids (e.g. selectionIds = elements.map(e => e.id).join(','), or useMemo(() =>
elements.map(e => e.id), [elements]) and then useEffect(() => setLocked(false),
[selectionIds]); alternatively, make FormatPanel pass a referentially stable
elements prop to SizePositionSection; update the dependency array and the
derivation of selectionIds near the locked/setLocked and useEffect code
accordingly (referencing locked, setLocked, useEffect, elements,
SizePositionSection, FormatPanel).

Fix all unresolved CodeRabbit comments on this PR:

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