feat(photos): show photo preview in "Add photo details" modal (#1722)#1724
Conversation
Renders a preview of the selected photo inside the PhotoMetadataModal above the description field. The preview image is created from a local object URL of the selected File object, created when the file changes and revoked on unmount (and on file change) to avoid memory leaks. Out of scope: PhotoMetadataSidepanel (unchanged by this story). Fixes #1722 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com> Co-Authored-By: Claude frontend-developer (Haiku 4.5) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com> Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) <noreply@anthropic.com>
|
[product-owner] Product Owner review of #1722 acceptance criteria. Verdict: APPROVED — all 6 acceptance criteria met.
Scope discipline: Frontend-only; Notes (non-blocking):
All acceptance criteria are satisfied. Recommend approval pending the standard architecture/security/QA reviews and green CI. |
steilerDev
left a comment
There was a problem hiding this comment.
[product-architect]
Architecture Review — APPROVE (posted as comment; cannot self-approve)
Small, well-scoped frontend enhancement. Adds a pre-upload photo preview to PhotoMetadataModal. Reviewed for object-URL lifecycle, React patterns, scope, and test coverage.
Object-URL lifecycle — correct
useEffectkeyed[file]creates the URL; the cleanup closure revokes it. Critically, cleanup captures the localurlconst, not theobjectUrlstate — this avoids the classic stale-state revocation bug and guarantees the exact URL created by that effect run is the one revoked.- Lifecycle fully covered: create on mount, revoke on unmount, revoke-old + create-new on
filechange. No memory leak.
React patterns — clean
- Standard create/revoke effect; conditional render
{objectUrl && ...}is the right guard for the brief pre-effect frame. Existing focus-trap effect untouched;handleSavepayload unchanged.alt={file.name}gives an accessible label.
Scope — tight
- Only the preview is added;
PhotoMetadataSidepanelcorrectly left out of scope per the PR description. No contract/schema/shared-type changes — appropriately frontend-only.
CSS — compliant
- Uses design tokens (
--color-bg-tertiary,--color-border,--radius-md).object-fit: contain+max-heightwith a mobile breakpoint is sound. No hardcoded values.
Tests — adequate
- 7 unit cases cover create-once, render (src/alt), DOM position, falsy-URL no-render, revoke-on-unmount, revoke+recreate on file change, Save-payload-unchanged. jsdom URL API stubbing is properly saved/restored per test.
- E2E Scenario 12 (smoke) + 12b (mobile) assert visibility,
alt, andblob:src across viewports.
Non-blocking notes (Low)
- The "first element child of formBody" unit test couples to DOM nesting via
parentElementwalks — correct but brittle to future layout wrapping. Fine to leave.
Verdict: approve. No blocking findings.
steilerDev
left a comment
There was a problem hiding this comment.
[ux-designer]
Design review for PR #1724 — PhotoMetadataModal photo preview (Story #1722).
Verdict: APPROVE
All checks pass. The implementation matches the visual spec exactly.
Token Adherence
.photoPreview:background: var(--color-bg-tertiary),border: 1px solid var(--color-border),border-radius: var(--radius-md)— all semantic tokens with confirmed dark mode overrides intokens.css. No hardcoded color or radius values..photoPreviewImage: no color properties; layout-only values (max-width: 100%,max-height: 14rem,object-fit: contain,object-position: center). The14rem/10remmax-height values are plain layout constraints — acceptable as documented in the style guide notes.border-radius: var(--radius-md)on the image element is correct (prevents image corners bleeding through the container'soverflow: hidden).
Dark Mode
Automatic via semantic tokens. --color-bg-tertiary resolves to var(--color-slate-600) in dark mode and --color-border resolves to var(--color-slate-500). No special dark mode treatment needed.
Responsive Behavior
- Desktop:
max-height: 14rem— matches spec. - Mobile breakpoint:
@media (max-width: 767px)reduces tomax-height: 10remon both container and image — matches spec. (Note: the spec used767pxas the upper bound for the tablet/mobile breakpoint, consistent with the1023pxconvention for desktop upper bounds.) - Both container and image
max-heightare kept in sync across the media query, which is correct — the image'smax-heightmust match the container's or theobject-fit: containcalculation can overflow.
Accessibility
alt={file.name}is informative (not decorativealt=""), appropriate for a pre-upload preview that represents a specific file the user selected.- Conditional render
{objectUrl && ...}means the image is absent from DOM until the object URL is created — no emptysrcor broken-image state exposed to screen readers. - No interactive element added; no focus management changes required.
Visual Consistency
display: flex; align-items: center; justify-content: centeron the container correctly centers landscape and portrait photos within the fixed-height box.overflow: hiddenon the container clips any edge cases where the image might exceed bounds.display: blockon the image eliminates the inline baseline gap.- Placement above the description textarea matches the spec's "above the description field" requirement.
Lifecycle Correctness
useEffectwith[file]dependency creates a new object URL on mount and when the file changes, and revokes the previous URL on cleanup — no memory leaks.- The unit tests cover the full lifecycle (create, revoke on unmount, revoke-and-recreate on file change), the falsy-URL conditional branch, and the save behavior regression. This is thorough.
No findings. Implementation is spec-compliant.
|
🎉 This PR is included in version 2.9.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Document the two photo enhancements shipped in v2.9.0: - Photo preview in the "Add photo details" modal (#1722, #1724) - Hierarchy-aware area picker (indentation + ancestor path) and description-aware orientation search (#1723, #1725) Updates guides/diary/photo-capture.md and guides/work-items/areas-and-trades.md, and rewrites RELEASE_SUMMARY.md for the v2.9.0 release. Co-authored-by: Frank Steiler <frank@steiler.de> Co-authored-by: Claude docs-writer (Opus 4.6) <noreply@anthropic.com>
Summary
PhotoMetadataModal, displayed above the description fieldFileon file change and revoked on unmount/file change to prevent memory leaksPhotoMetadataSidepanelis intentionally out of scope for this storyFixes #1722
Test plan
PhotoMetadataModal.test.tsxcovers preview rendering, object URL lifecycle (create/revoke), and absence when no file is selectedphoto-capture-flow.spec.tsverifies the preview image appears in the modal after file selection🤖 Generated with Claude Code