[#205] Implement speech bubble, narration, and SFX overlay tools#218
Conversation
Add full overlay CRUD to LetteringEditor: toolbar buttons to create speech/narration/SFX overlays, drag to move, corner handle to resize, editable text/speaker fields in inspector, delete with confirmation, and bubble tail anchor adjustment for speech overlays. Extend Overlay model with tailAnchor field for speech bubble tails. Overlays persist via existing onSave → PUT cuts.json flow. Tests: add overlay via toolbar, edit text via inspector, delete with double-click confirmation, save callback verification, tailAnchor defaults and JSON roundtrip. 138 total tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@re2 verdict: APPROVE Reviewed: Overlay model (
CRUD operations:
Drag/resize:
Inspector:
Tests (6 new): Add overlay, edit text, delete with confirmation, save callback, tailAnchor defaults, JSON roundtrip. Existing tests updated for new No issues found. |
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The overlay CRUD foundation is mostly in place and CI is passing, but speech overlays that were saved before tailAnchor existed cannot use the new tail-anchor adjustment UI. That leaves the tail feature unavailable for valid existing speech overlay data.
Findings
- [medium] Existing speech overlays without
tailAnchordo not get tail-anchor controls.- File:
app/web/components/LetteringEditor.tsx:295 - Details: The inspector renders tail controls only when
selectedOverlay.tailAnchoris already present. SincetailAnchoris optional in the model and older/minimal speech overlays can havetype: "speech"without this field, selecting those overlays will not show any tail controls and saving will not add the default tail anchor. This undercuts the #205 acceptance around speech bubble tail adjustment/persistence for existing overlay data. - Suggestion: Gate the controls on
selectedOverlay.type === "speech", derive a default like{ x: 0.5, y: 1.2 }when the field is missing, and have edits persist that value. Please add a regression test with a preexisting speech overlay that lackstailAnchor, verifies the tail controls appear with defaults, edits one coordinate, and saves the resultingtailAnchor.
- File:
Decision
Requesting changes because the new tail-anchor feature should work for both newly created speech overlays and existing persisted speech overlays. The rest of the implementation is directionally solid, and lint-and-typecheck is passing.
Gate tail anchor controls on type === "speech" instead of
tailAnchor existence. Uses default {x: 0.5, y: 1.2} when the
field is missing, so existing speech overlays without tailAnchor
can still adjust and persist the tail. Add regression test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@re2 re-review verdict: APPROVE (maintained) Fix correctly addresses @re1's finding:
Approval maintained. |
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The tail-anchor compatibility issue is fixed: all speech overlays now show tail controls, including existing persisted overlays without a tailAnchor, and edits persist through the normal save flow. CI is passing.
Findings
- No blocking findings.
Decision
Approving because PR #218 now satisfies #205's overlay CRUD and tail-anchor requirements, keeps changes scoped to the editor model/UI, and lint-and-typecheck passes.
Summary
tailAnchorfor speech bubble tails (default{x: 0.5, y: 1.2})Test plan
npm run typecheckpassesnpm run lintpasses (no new errors)npm run testpasses (138 tests)Closes #205
🤖 Generated with Claude Code