packages/slides/src/view/canvas/shapes/stars/handles.ts (1)

4-5: 💤 Low value

Consider parameterizing STAR_MIN/STAR_MAX in future.

The hardcoded constants duplicate values from each star's ADJUSTMENT_SPEC. As noted in the lessons document, this is acceptable for the pilot since all 6 stars share the same range, but P3-A.2 should consider passing (min, max) to the factory or reading from ADJUSTMENT_SPECS.get(kind) to avoid silent desync if a future star shape has different bounds.

🤖 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/shapes/stars/handles.ts` around lines 4 - 5,
STAR_MIN and STAR_MAX are hardcoded duplicates of ranges defined on each star's
ADJUSTMENT_SPEC; update the star handle creation so the min/max are taken from
the star's ADJUSTMENT_SPEC (or from ADJUSTMENT_SPECS.get(kind)) or passed into
the factory instead of using the constants STAR_MIN/STAR_MAX to avoid silent
desync—locate usages in the handle factory and replace references to
STAR_MIN/STAR_MAX with the adjustmentSpec.min/adjustmentSpec.max (or injected
min/max parameters) for the functions that build star handles.

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

216-246: ⚡ Quick win

Extract localToWorld to avoid duplicating the rotation math.

The closure at L231-235 is byte-for-byte identical to the one in renderRotatedHandles at L119-123. They both transform (lx, ly) from frame-local coords (origin = top-left, scale = frame.w × frame.h) into world coords via a rotation around the frame center. Future tweaks (e.g. a different anchor, or scaling for the rotated path) now have to be made in two places in lockstep.

Pulling it into a file-local helper keeps both call sites in sync and shrinks the new function meaningfully. This is per coding guidelines ("Keep functions small and focused on a single responsibility").

♻️ Proposed extraction

+function frameLocalToWorld(frame: Frame) {
+  const cx = frame.x + frame.w / 2;
+  const cy = frame.y + frame.h / 2;
+  const cos = Math.cos(frame.rotation);
+  const sin = Math.sin(frame.rotation);
+  return (lx: number, ly: number) => {
+    const dx = lx - frame.w / 2;
+    const dy = ly - frame.h / 2;
+    return { x: cx + dx * cos - dy * sin, y: cy + dx * sin + dy * cos };
+  };
+}

 function renderAdjustmentHandles(
   overlay: HTMLDivElement,
   el: Element,
   options: OverlayOptions,
 ): void {
   if (el.type !== 'shape') return;
   const handles = ADJUSTMENT_HANDLES.get(el.data.kind);
   if (!handles || handles.length === 0) return;

   const { scale } = options;
   const { frame } = el;
-  const cx = frame.x + frame.w / 2;
-  const cy = frame.y + frame.h / 2;
-  const cos = Math.cos(frame.rotation);
-  const sin = Math.sin(frame.rotation);
-  const localToWorld = (lx: number, ly: number) => {
-    const dx = lx - frame.w / 2;
-    const dy = ly - frame.h / 2;
-    return { x: cx + dx * cos - dy * sin, y: cy + dx * sin + dy * cos };
-  };
+  const localToWorld = frameLocalToWorld(frame);

And the same swap inside renderRotatedHandles at L119-123.

🤖 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/overlay.ts` around lines 216 - 246, Extract
the duplicated rotation/translation closure into a file-local helper (e.g.,
transformLocalToWorld or localToWorldForFrame) and replace the inline
localToWorld closures in renderAdjustmentHandles and renderRotatedHandles with
calls to that helper; the helper should accept the Element frame (or frame.x,
frame.y, frame.w, frame.h, frame.rotation) and return either a function (lx, ly)
=> {x,y} or accept (frame, lx, ly) and compute cx, cy, cos, sin and the same
dx/dy math so behavior remains identical to the existing localToWorld used when
computing world coordinates for handles (used alongside
ADJUSTMENT_HANDLES/defaultAdjustmentsFor and makeAdjustmentHandle).

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

957-1012: ⚡ Quick win

Local↔world transform is computed three different ways inside one function.

Within startAdjustmentDrag you build cx/cy/cos/sin (L957-960), the worldToLocal closure (L962-968), and then again the inverse (local→world) inline for the tooltip anchor at L996-1005 — all from the same captured startEl.frame. The forward transform also matches the closures in overlay.ts (renderRotatedHandles / renderAdjustmentHandles). Three copies of the rotation math means a bugfix has to land in three sites to stay correct.

A small frameTransform(frame) helper returning { localToWorld, worldToLocal } makes the inline tooltip math at L996-1005 a one-liner and aligns this file with the overlay refactor suggested in overlay.ts. Per coding guidelines, this keeps startAdjustmentDrag focused on the interaction loop rather than the geometry kernel.

♻️ Sketch

-    const handleLocal = handle.position(
-      { w: startEl.frame.w, h: startEl.frame.h },
-      live,
-    );
-    const handleWorld = {
-      x:
-        cx +
-        (handleLocal.x - startEl.frame.w / 2) * cos -
-        (handleLocal.y - startEl.frame.h / 2) * sin,
-      y:
-        cy +
-        (handleLocal.x - startEl.frame.w / 2) * sin +
-        (handleLocal.y - startEl.frame.h / 2) * cos,
-    };
+    const handleLocal = handle.position(
+      { w: startEl.frame.w, h: startEl.frame.h },
+      live,
+    );
+    const handleWorld = localToWorld(handleLocal.x, handleLocal.y);

…where localToWorld is built alongside worldToLocal at the top of the function (or imported from a shared frameTransform helper used by overlay.ts too).

🤖 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 957 - 1012, The
interaction computes rotation transforms in three places; extract a helper
frameTransform(frame) that returns { worldToLocal(wx,wy), localToWorld(lx,ly) }
based on startEl.frame (using cx/cy/cos/sin) and replace the inline worldToLocal
closure and the tooltip local→world math in startAdjustmentDrag with calls to
frameTransform.worldToLocal and frameTransform.localToWorld respectively (update
the tooltip call that feeds showAdjustmentTooltip to use
localToWorld(handleLocal)). Keep symbol references: startAdjustmentDrag,
startEl.frame, worldToLocal, localToWorld, handle.position,
paintLiveAdjustments, showAdjustmentTooltip; optionally reuse/import the same
helper in overlay.ts to keep geometry consistent.

---

1129-1149: ⚡ Quick win

lastWord will collapse labels for future multi-word specs that share a trailing token.

For the 9 pilot shapes the spec names produce unambiguous suffixes (e.g. x / y for wedgeRectCallout), so this is fine in P3-A.1. But the function is the canonical label formatter for every future adjustment registered in P3-A.2 (24 more shapes per PR objectives). The moment a shape ships specs like "Inner radius" + "Outer radius" or "First inset" + "Last inset", both legs render as radius: … / inset: … in the tooltip, and the user can't tell which value is which.

Two options that scale cleanly:

• Disambiguate by taking the last two words when the trailing token isn't unique within specs, or
• Let each spec declare a short label field and use that directly instead of slicing name.

The lighter-touch fix:

♻️ Disambiguate at format time

 function formatAdjustments(
   specs: readonly { name: string; format?: (v: number) => string }[],
   values: number[],
 ): string {
   if (specs.length === 1) {
     const v = values[0];
     return specs[0].format ? specs[0].format(v) : String(v);
   }
+  const labels = specs.map((s) => lastWord(s.name));
+  const hasCollision = new Set(labels).size !== labels.length;
   return specs
     .map((s, i) => {
-      const label = lastWord(s.name);
+      const label = hasCollision ? s.name.toLowerCase() : lastWord(s.name);
       const value = s.format ? s.format(values[i]) : String(values[i]);
       return `${label}: ${value}`;
     })
     .join(' / ');
 }

🤖 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 1129 - 1149,
formatAdjustments currently calls lastWord(name) which collapses different spec
names that share a trailing token; change formatting so labels are
disambiguated: in formatAdjustments, for each spec use an explicit short label
if provided (e.g. spec.label) otherwise compute the label by inspecting all
specs' name values — if the last word is unique use that, but when duplicates
exist use the last two words (or more as needed) for that group; update or
replace lastWord to return either one- or two-word suffixes based on uniqueness
and ensure you still apply the existing s.format when present (refer to
formatAdjustments, lastWord, specs[i].name, spec.label and s.format to locate
the logic).