Snapshot detail v2: category is a mask preset, checkmark off image, full-width images - #83
Merged
Merged
Conversation
…e, full-width images Follow-up on the first redesign (PR #82), driven by two threads: a conceptual correction about what "Category" actually is, and concrete layout complaints (checkmark overlapping the image, image still small, Baseline tab clickable with nothing to show, a category's masks each getting their own chip). Category was never structurally different from "a named preset of masks" -- it's one string column on a snapshot that scopes both a Settings label and category-mask matching. The standalone free-text Category input made it look like an independent, general-purpose label decoupled from masks, which is what was actually causing the "feels weird" complaint. This removes that input outright: category membership is now visible and settable only through the existing mask-assignment flow (pick/create a category while saving a mask). Author-confirmed tradeoff: there's now no way to untag a snapshot short of deleting the category globally -- accepted deliberately, same shape as the earlier approve-on-pass tradeoff. - Status pill, Dual/Single, Show diff, and (Single mode) Baseline/ Candidate tabs collapse onto one top-bar line, with Approve pushed to the far right via justify-between -- no longer absolutely positioned over the image at all, so it can't overlap the pixels by construction. - Baseline tab disabled when snapshot.baselineUrl is null. - Images render at className="w-full h-auto" instead of a fixed Small/ Medium/Large pixel width, filling whatever width their pane gives them. imageDisplaySize.ts and its Settings UI (ImageSizeInput) are deleted outright, not left as dead code -- this change was their only remaining consumer. - A category's masks collapse to one chip -- "{category} (N)", no #, no delete control -- instead of one identical-looking #chip per mask. Global/per-image masks are unaffected. Two architect SOLID-gate rounds (round 1 CONCERNS, both findings verified empirically rather than argued abstractly): - The "images fill available width" claim was false for the candidate/ diff pane specifically: InteractiveImagePane's wrapper was inline-block, which shrink-wraps to intrinsic content size, so a percentage-width child inside it resolves against an indeterminate containing block and silently renders at natural pixel size instead of filling anything. Confirmed with an isolated Playwright repro before the fix, and again visually after (Docker screenshots below). - The test-impact list missed examples/demo-app/e2e.test.ts's category- management test, which drove the exact UI being deleted and whose "tag with zero masks" scenario becomes unreachable once tagging only happens via mask assignment -- rewritten to tag via drawing a mask + "+ New category" instead, asserting the new masks-section chip. Round 2: SOUND. Verification: both fixes (Baseline-disabled, category-chip collapse) mutation-tested against the real suite; 2 new tests + rewrites to 2 existing tests reflecting the removed Category input and collapsed chip; full lint/test/e2e/backend suite green (55 viewer tests, 6/6 real e2e including the rewritten category flow, 141 backend); real Docker build + Playwright screenshots at two viewport widths confirmed the image-fill fix actually works, the checkmark never touches the image, and the disabled Baseline tab renders correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrQeh9CXrigN1cvpJuyzy4
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up on the first redesign (PR #82), driven by a conceptual correction
plus concrete layout feedback.
Category was never structurally different from "a mask preset." It's
one string column on a snapshot that scopes both a Settings label and
category-mask matching. The standalone free-text Category input made it
look like an independent label decoupled from masks -- that mismatch was
the actual source of the "feels weird" complaint. This PR removes the input
outright: category membership is now visible/settable only through the
existing mask-assignment flow. Accepted tradeoff: there's no longer a
way to untag a snapshot short of deleting the category globally -- same
shape as the earlier approve-on-
passtradeoff, confirmed with the author.onto one top-bar line, Approve pushed to the far right via
justify-between-- no longer absolutely positioned over the image, soit can't overlap the pixels by construction.
w-full h-autoinstead of a fixed Small/Medium/Largepixel width.
imageDisplaySize.ts+ its Settings UI deleted outright(this was their only remaining consumer, not left as dead code).
{category} (N), no#, nodelete -- instead of one identical #chip per mask. Global/per-image
masks unaffected.
Process
Two architect SOLID-gate rounds, both round-1 findings verified
empirically:
pane: its wrapper was
inline-block, which shrink-wraps to intrinsiccontent size, so a percentage-width child silently rendered at natural
pixel size instead of filling anything. Confirmed with an isolated
Playwright repro before the fix, and again visually after.
examples/demo-app's live-browsercategory-management e2e test, which drove the exact UI being deleted --
rewritten to tag via mask-assignment instead of the removed field.
Round 2: SOUND.
Test plan
tested against the real suite
npm run lint --workspacescleannpm test --workspaces-- 55 viewer tests (2 new, 2 rewritten, 3deleted for removed UI)
npm run test:e2e -w examples/demo-app-- 6/6, including therewritten category-management flow against this exact markup
ruff check+pytest-- 141/141 (no backend changes)confirmed the image-fill fix actually works, checkmark never
touches the image, disabled Baseline tab renders correctly
viewer/CODEMAP.md+examples/demo-app/CODEMAP.mdupdatedCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01NrQeh9CXrigN1cvpJuyzy4