relocate icons when out of boundary

[erinz2020]

replace icons when out of frame

[codecov-commenter]

Codecov Report

❌ Patch coverage is 12.82051% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.37%. Comparing base (d722707) to head (441a614).

Files with missing lines	Patch %	Lines
frontend/src/components/ImageModal.jsx	10.00%	18 Missing ⚠️
frontend/src/pages/Encounter/ImageCard.jsx	15.78%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
- Coverage   51.50%   51.37%   -0.13%     
==========================================
  Files         308      308              
  Lines       12100    12136      +36     
  Branches     3897     3826      -71     
==========================================
+ Hits         6232     6235       +3     
- Misses       5578     5618      +40     
+ Partials      290      283       -7     

Flag	Coverage Δ
backend	51.37% <12.82%> (-0.13%)	⬇️
frontend	51.37% <12.82%> (-0.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JasonWildMe]

Focus: "relocate icons when out of boundary"

Reviewers: Claude (Opus 4.8) + Codex, independently — same findings.

Verdict: Request changes. The goal is good and the edge-clamping instinct is right, but as written it doesn't actually achieve "keep icons in frame" for two common cases, and it changes the default icon position for every annotation.

All findings are fixable.

🔴 Should fix before merge

1. Rotated annotations break the whole point. iconTop/iconLeft are computed in unrotated container coordinates, but
   they're applied as top/left inside the annotation box div — which has transform: rotate(theta). So for any rotated box
   the clamped position is itself rotated, and the icons land somewhere other than the intended in-frame spot. Cleanest
   fix: render the icon cluster as a sibling of the box in container space instead of a child of the rotated div — then
   top: iconTop, left: iconLeft directly (no - newRect.x/y), and the rotation problem disappears.

2. The horizontal icon cluster can still overflow. ImageCard.jsx feeds the same iconSize = 20 overflow check to both
   overlays, but the second one (d-flex, the other-encounter case at the right: -2 hunk) is ~2 icons wide (~40px).
   iconLeft + iconSize > containerWidth only reserves 20px, so a 40px cluster still spills ~one icon past the edge. (It
   also reserves totalIconHeight = 40 for that horizontal cluster, which is only ~20 tall — over-clamps it upward near
   the bottom.) Use the actual cluster width/height per overlay (a ref + measured size, or distinct constants).

3. It changes the default position for all annotations, not just edge ones. Old: top:0, right:0 → icons at the
   top-right inside the box. New default: iconLeft = newRect.x + newRect.width → left = newRect.width → icons sit just
   outside the box's right edge. That's a visible UX change on every mid-image annotation, not only the overflow case. If
   that's intentional (icons beside the box rather than over it), great — but it should be a deliberate design call. If
   not, make the normal case annotationRight - clusterWidth (top-right inside) and only relocate on overflow.

🟠 Worth addressing

4. boxRef null/0 on first paint. containerWidth/Height fall back to 0, which forces the overflow branch and yields
   offsets like top: -newRect.y on the first render. A ref populating doesn't trigger a re-render, so it can stick until
   some other state change. Fall back to the prior simple positioning until dimensions are measured (or use a
   ResizeObserver/layout state).

5. Hardcoded iconCount = 2. Fine today, but it drifts the moment an overlay renders 1 or 3 buttons
   (permissions/state-dependent) — and the horizontal overlay already shows the shared constants don't describe every
   call site. Prefer measuring the actual cluster.

🟢 Minor / quality

• The positioning block is duplicated across the two files and the two overlays in ImageCard. A small shared helper
  (computeIconPosition(rect, container, clusterSize)) would make it consistent and testable — and is the natural place
  to fix #1–#2 once.
• The Prettier reflow of hasNonTrivialAnnotations is harmless.

On our earlier question

Confirmed again here: no conflict with our work — GitHub reports MERGEABLE, and these are presentation-only regions
distinct from the status/gating and match-results code we touched.

---

Net: encourage the author — the approach is on the right track — but ask for: confirm/scope the default-position
change (#3), handle rotated boxes (#1, ideally by rendering the cluster as a container-space sibling), and size the
horizontal cluster correctly (#2). #1 and #2 are what make Codex (and me) land on "don't merge yet."

Want me to post this as a PR review on #1564? I can format it as GitHub review comments (constructively, given it's a
contributor fix) — say the word and I'll push it up.

[erinz2020]

🔴 Should fix before merge

Rotated annotations break the whole point. iconTop/iconLeft are computed in unrotated container coordinates, but
they're applied as top/left inside the annotation box div — which has transform: rotate(theta). So for any rotated box
the clamped position is itself rotated, and the icons land somewhere other than the intended in-frame spot. Cleanest
fix: render the icon cluster as a sibling of the box in container space instead of a child of the rotated div — then
top: iconTop, left: iconLeft directly (no - newRect.x/y), and the rotation problem disappears.

does this review mean that the icons should not rotate with the boxes? in this way the box is rotated but icons are away from the box, it might look weird: in following image, the icons belong to the rotated box

the current solution is as following: icons rotate with the box, which we have been doing since we have this page.

i can move it back to the inside of the box if needed(this is what issue #3 is about), but i still think the icons should rotate with the box

@JasonWildMe what do you think?