[#204] Build SVG/HTML lettering editor foundation#217
Conversation
Add overlay data model (Overlay type, coordinate normalization utils) and LetteringEditor component. Editor renders clean image as background, positions overlay elements using normalized coordinates that scale with container size, supports click-to-select with inspector panel showing type/position/text, and click-to-deselect. Extend Cut interface with overlays field (defaults to []). Integrate editor into CutListPanel via "Open editor" button for cuts with clean images. 17 new tests: coordinate normalization (10), editor rendering/selection (6), CutListPanel editor button (1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The PR adds the requested editor surface and selection/inspector foundation, but the coordinate mapping is tied to the editor container instead of the rendered image. That breaks the core #204 requirement that overlay coordinates scale correctly with preview size.
Findings
- [high] Overlay coordinates are computed against the full container, not the displayed image bounds
- File:
app/web/components/LetteringEditor.tsx:127 - Details: The clean image is rendered with
w-full h-full object-contain, so whenever the editor container and image aspect ratios differ, the browser letterboxes the image inside the container. Overlay positions/sizes are then computed fromcontainerSize.width/heightatapp/web/components/LetteringEditor.tsx:135, with no offset for the actual image rectangle. For example, a vertical webtoon image in a wide editor surface will be centered with horizontal padding, butx: 0overlays render at the container's left edge instead of the image's left edge. That means saved normalized coordinates will not line up with the clean image across preview sizes. - Suggestion: Measure the rendered image box (or render the image and overlays inside a shared aspect-ratio wrapper sized to the actual displayed image), then compute overlay left/top/width/height relative to that image box, including any
object-containoffsets. Add a regression test that uses a container/image aspect-ratio mismatch and asserts overlay placement is relative to the image bounds, not the outer editor surface.
- File:
Decision
Requesting changes because correct normalized coordinate scaling is one of #204's explicit acceptance criteria, and the current implementation misplaces overlays in common aspect-ratio cases.
|
@re2 verdict: APPROVE Reviewed: Overlay model (
Cut interface extension (
LetteringEditor component:
CutListPanel integration:
Tests (17):
No issues found. |
Position overlays relative to the actual rendered image area (after object-contain scaling) instead of the full container. Computes image bounds from naturalWidth/naturalHeight and container size, accounting for letterboxing. Tests updated to simulate image load. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@re2 re-review verdict: APPROVE (maintained) Important fix — overlays now position relative to rendered image bounds, not the full container:
Good improvement — without this, overlays would drift on any non-exact-fit image. Approval maintained. |
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The image-bounds implementation now looks directionally correct, but the requested regression coverage is still missing. The updated test setup uses matching 4:3 image/container dimensions, so it would not catch the letterboxing drift that caused the original bug.
Findings
- [medium] Add a true aspect-ratio mismatch regression for overlay positioning.
- File:
app/web/components/LetteringEditor.test.tsx:32 - Suggestion: Use a container/image ratio mismatch, such as a 400x300 container with an 800x400 image, and assert the overlay style includes the expected centered offset from the rendered image bounds. For example, with 400x300 container and 800x400 image, the rendered image is 400x200 with
top: 50; an overlay aty: 0.2should render attop: 90, not60. The currentsimulateImageLoad()uses 800x600 against a 400x300 container, which has no letterboxed padding and would pass even if overlays were still positioned against the full surface.
- File:
Decision
Requesting changes because the prior blocker was specifically an aspect-ratio mismatch bug, and the added tests do not exercise that failure mode. The production logic appears to be on the right track; this should be a focused test update.
Test overlay positioning with 800x200 image in 400x400 container, verifying nonzero y-offset (150px letterbox) and correct overlay dimensions. This would fail with the original container-based positioning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@re2 re-review verdict: APPROVE (maintained) Mismatched aspect ratio regression test is exactly right:
Good regression coverage. Approval maintained. |
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The overlay positioning fix now uses the rendered image bounds after object-contain scaling, and the added regression test covers the mismatched aspect-ratio letterboxing case that previously failed. CI is passing.
Findings
- No blocking findings.
Decision
Approving because the #204 coordinate-scaling acceptance criterion is now covered by implementation and regression test, selection/editor behavior remains scoped, and lint-and-typecheck passes.
Summary
app/lib/overlays.ts):Overlaytype,toPixel/toNormcoordinate normalization,createOverlayfactoryLetteringEditorcomponent: renders clean image as background, positions overlays using normalized coordinates that scale with container size, click-to-select with inspector panel, click-to-deselectCutinterface withoverlays: Overlay[]field (defaults to[], backwards compatible)CutListPanelvia "Open editor" button for cuts with clean imagesTest plan
npm run typecheckpassesnpm run lintpasses (no new errors)npm run testpasses (132 tests)Closes #204
🤖 Generated with Claude Code