Skip to content

[3339 by GPT-6.1 Sol] Visual evidence Fit frame ignores the image's aspect ratio - #3345

Merged
integry merged 3 commits into
mainfrom
3339/gpt-6.1-sol-visual-evidence-fit-frame-20261024-2038-rtd
Oct 24, 2026
Merged

integry merged 3 commits into
mainfrom
3339/gpt-6.1-sol-visual-evidence-fit-frame-20261024-2038-rtd

Conversation

@propr-dev

@propr-dev propr-dev Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

AI Implementation Summary

Closes #3339

Branch: 3339/gpt-6.1-sol-visual-evidence-fit-frame-20261024-2038-rtd
Commits: ✅ Changes committed (009f76b)


AI Processing Completed

Execution Details:

Summary:
Implemented Fit frames that follow each image’s aspect ratio within the existing size limits. Checkerboard now shows through transparent pixels only.

Verified with 8 component tests, 11 browser tests, TypeScript, and lint. Captured focused previews for wide and tall images.

Detailed Logs:

  • Conversation: 44 messages
  • Session: 01a16fa9-c599-7201-8437-ed70c3c44916

Log files stored at:

  • conversation: /usr/src/app/logs/executions/issue-3339-2026-10-24T20-48-59-308Z-conversation.json
  • output: /usr/src/app/logs/executions/issue-3339-2026-10-24T20-48-59-308Z-output.txt
Latest Conversation Messages

This PR was created automatically by ProPR after processing issue #3339.


💡 Need changes?

Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!


Visual preview

Task mobile collapsed header 390

Task mobile collapsed header 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task mobile collapsed header menu 390

Task mobile collapsed header menu 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task mobile collapsed long title 390

Task mobile collapsed long title 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox 1440

Task visual preview lightbox 1440

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox 390

Task visual preview lightbox 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox zoomed 1440

Task visual preview lightbox zoomed 1440

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox zoomed 390

Task visual preview lightbox zoomed 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual previews 1440

Task visual previews 1440

Original unavailable: managed storage is unavailable or the upload has expired.

…image's aspe

Implemented by ProPR AI using gpt-6.1-sol model.

Implementation completed successfully.
@integry

integry commented Oct 24, 2026

Copy link
Copy Markdown
Owner

/ultrafix

@propr-dev propr-dev Bot added the ultrafix label Oct 24, 2026
@propr-dev

propr-dev Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

🔄 Ultrafix loop started (goal: 8/10, max cycles: 10)

First action: /review

💡 Tip: Remove the ultrafix label from this PR to stop further ultrafix cycles.

@propr-dev

propr-dev Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — claude:claude-opus-5-5

Overall Evaluation

The PR resolves issue #3339. In Fit mode the stage now shrink-wraps the image: w-fit max-w-full, with the image set to h-auto w-auto max-h-80 sm:max-h-[440px] max-w-full. The checkerboard therefore frames only the capture's own pixels. Actual-size images and videos keep the fixed 320/440px stage. The change is small, and the Playwright coverage now checks real aspect ratios for a 2560×600 and a 390×1800 capture in the 372px pane from the issue's repro.

One regression needs fixing first. The intrinsic sizing is applied before the image has any size, so the loading and failed states lose their visible frame. Needs minor changes.

✅ Frame follows the capture — TaskVisualPreviews.tsx:197-208 shrink-wraps the stage around the image, which is capped by both max height and pane width. The new e2e cases assert that frame and image boxes match within 1px and that the ratio is within 0.01 of the source.

✅ Fixed stage kept where it matters — 100% mode and videos still use h-80 w-full sm:h-[440px]. The e2e test now checks those heights directly instead of comparing against the Fit box.

✅ One image tree across zoom modes — Only class names change between Fit and 100%, so toggling zoom still restyles the loaded image instead of fetching it again.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F1: 🔴 Loading and unavailable states collapse in Fit mode

  • Required behavior: Fit mode must still show a visible frame for an image that is loading or failed to load. Before this PR the loading pulse and the "image unavailable" placeholder were clearly visible; making the frame intrinsic must not remove them.

  • Evidence: propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx:198 (stage class) and :207 (Fit image class)

    Static trace:

    1. A task publishes private evidence under /api/preview-media/..., as in the e2e fixture. usePreviewMediaSource returns {status: 'loading'} until the blob fetch resolves.
    2. PreviewImage (PreviewMedia.tsx:57) renders <span className="block h-auto w-auto max-h-80 max-w-full object-contain sm:max-h-[440px] block animate-pulse bg-slate-100">.
      • The span has no content, so it is 0×0.
      • The wrapping button is a flex item and the stage is w-fit, with no padding and no height.
      • So the whole checkerboard stage collapses to 0×0.
    3. The user sees only the context strip with no skeleton. The previous min-h-16 min-w-32 classes, which gave the pulse a 64×128 box inside a 320/440px stage, were removed.
    4. If the fetch fails (403/404/network) or <img> fires onError, PreviewMedia.tsx:56 renders the unavailable span with the same classes plus flex. It is sized only by its 20px ImageOff icon, so the frame shrinks to a ~20×20 checkerboard square.

    Nothing else protects these states: dimensions is null, the stage has no fallback height, and the unit test now asserts that min-h-16/min-w-32 are absent. All e2e cases wait for a successful load, so neither state is covered.

    Proposed regression: render a protected-URL preview whose fetch is pending or rejected, then assert the canvas has a non-trivial height.

  • Minimum fix: Use the intrinsic frame only once the image has measured dimensions, and keep the fixed stage otherwise. For example, change the stage class condition to isImage && !actual && dimensions. Alternatively, give the Fit image class a minimum size that applies only to the placeholder states. Add a unit or e2e assertion for the loading and failed cases.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 Reduce layout shift below the frame

The removed comment documented an invariant: "one fixed height so switching captures never moves the trace below". The Fit frame now changes height per capture and on every load, so the trace and log header below move. This trade-off follows from the issue, but reserving space would soften it. Once dimensions are known, an aspect-ratio style on the frame would hold it; a per-session cached size would help when switching back to a capture. Optional, because the objective explicitly asks for an intrinsically sized frame.

S2: 🟢 Small captures stay small

Captures smaller than the pane (for example a 200×120 dialog crop) now render as a tiny frame at natural size, with no upscaling and no minimum. That is arguably correct for evidence fidelity, but a short note or a deliberate minimum frame width would make the behaviour explicit. Optional design choice.

S3: 🟢 Cover the Inbox caller

TriageVerdictCard.tsx:288 reuses the component with className="" in a differently padded scroll container. A quick visual check or e2e assertion there would confirm that the mx-auto w-fit frame behaves as expected outside the task pane. Optional, because the change is container-agnostic.

Score

The core fix is correct and well tested for loaded images. However, the intrinsic Fit frame collapses the loading skeleton and the unavailable placeholder, which is a reachable regression for private media; a localized fix is needed.

Score: 6/10


🤖 Review Details

  • Model: Claude Opus 5.5
  • Time: 49s
  • Tokens: 44,221 (40,030 in / 4,191 out)
  • Cost: $0.28

View Task


💡 Next step: Comment /fix to address every F# merge blocker, /fix all for every pending blocker and suggestion, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them or request /fix all, and they never relax a merge blocker.

Copy and edit to choose records from this review:

/fix F1 S1 S2 S3

🤖 Review by ProPR

@propr-dev

propr-dev Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit 82a7611

Addressed 1 AI review comment (IDs: 6390405788)

Addressed finding F1

Summary of Changes

Addressed F1 only. Fit loading and unavailable placeholders now retain a visible 128×64 minimum frame; loaded images keep their intrinsic aspect ratio.

Validation passed: 25 unit tests, 19 browser tests, and focused lint checks. Added regressions for pending fetches, 403/404 responses, network failures, and image-decoding failures. Captured mobile and desktop preview evidence.

Visual preview

Task mobile collapsed header 390

Task mobile collapsed header 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task mobile collapsed header menu 390

Task mobile collapsed header menu 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task mobile collapsed long title 390

Task mobile collapsed long title 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox 1440

Task visual preview lightbox 1440

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox 390

Task visual preview lightbox 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox zoomed 1440

Task visual preview lightbox zoomed 1440

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual preview lightbox zoomed 390

Task visual preview lightbox zoomed 390

Original unavailable: managed storage is unavailable or the upload has expired.

Task visual previews 1440

Task visual previews 1440

Original unavailable: managed storage is unavailable or the upload has expired.


🤖 Implementation Details

  • Model: GPT-6.1 Sol (medium)
  • Requested By: @propr-ultrafix
  • Time: 4m 9s
  • Tokens: 521,176 (517,629 in / 3,547 out)
  • Cost: $0.15

View Task Execution


💡 ProPR Slash Commands
Command Description Example
/merge Merge target branch into this PR and resolve conflicts /merge
/review Request an AI code review /review or /review claude-sonnet
/fix Address review blockers, or blockers and suggestions with /fix all /fix, /fix all, or /fix F3 S5
/switch Change the AI model for this PR /switch claude-opus
/use Override the model for a single follow-up run /use claude-sonnet
/ultrafix Loop review→fix cycles until score goal is met /ultrafix or /ultrafix goal=8 max=10

🤖 Generated by ProPR

… now retain a visible 128×64 minimum frame; loaded images keep their intrinsic aspect ratio.

Addressed F1 only. Fit loading and unavailable placeholders now retain a visible 128×64 minimum frame; loaded images keep their intrinsic aspect ratio.

Validation passed: 25 unit tests, 19 browser tests, and focused lint checks. Added regressions for pending fetches, 403/404 responses, network failures, and image-decoding failures. Captured mobile and desktop preview evidence.

PR: #3345
Comment by: @propr-ultrafix (ID: 0)
Model: GPT-6.1 Sol
@propr-dev

propr-dev Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — claude:claude-opus-5-5

Overall Evaluation

The PR fixes issue #3339. In Fit mode the Visual Evidence stage now sizes to the image instead of using a fixed h-80 w-full sm:h-[440px] box:

  • The stage is w-fit max-w-full.
  • The image is h-auto w-auto max-h-80 sm:max-h-[440px] max-w-full.
  • The checkerboard now shows only through transparent pixels.
  • The fixed stage is kept for 100% mode and for videos.
  • Loading and unavailable placeholders keep a minimum size through [&>span]:min-h-16 [&>span]:min-w-32.

The e2e coverage is strong. However, the new layout makes the image fully cover its trigger button, which hides the button's inset keyboard focus ring in Fit mode. That needs a small fix before merge.

✅ Aspect-ratio frame matches the objective — The stage follows the image, and the new e2e cases check frame and image boxes to within 1px. They also check the aspect ratio for 2560×600 and 390×1800 captures in a 372px pane, plus the height and width caps.

✅ Placeholders don't collapse — Spans get minimum sizes from the button rather than the image class, so a loaded image is not forced to 128×64. The 403/404/network/decode × 390/1440 matrix checks the pending and failed states.

✅ 100%/video stage preserved — The fixed-height stage stays for actual-size images and videos, and the e2e now asserts the 440/320 height directly rather than comparing it to the Fit box.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F2: 🔴 Fit-mode trigger loses its visible focus indicator

  • Required behavior: Changed behavior must keep a visible keyboard focus indicator on the interactive "Open full-size preview" control (WCAG 2.4.7). The base code kept part of it visible in Fit mode.

  • Evidence: propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx:204-209 — the button keeps focus:outline-none focus-visible:ring-2 focus-visible:ring-inset, but in Fit mode it now shrinks to exactly the image's size (flex min-w-0 max-w-full …, with the image block h-auto w-auto …).

    Static trace:

    1. A keyboard user tabs to "Open full-size preview: …" while Fit mode is showing a loaded, opaque screenshot.
    2. ring-inset renders as an inset box-shadow on the button. That shadow paints with the button's background, beneath its in-flow children.
    3. The <img> now covers the button's whole box, because stage, button and image are all sized to the image. The image paints over the entire inset ring.
    4. focus:outline-none has removed the default outline, so nothing shows focus.

    Before this PR, the button was h-full w-full in a fixed-height stage and the image was letterboxed inside it. The top/bottom (or left/right) bands of the ring stayed visible. Existing unit and e2e tests never focus the frame and check for a visible indicator, so they don't catch this.

    Proposed regression: focus the trigger in Fit mode and assert a visible outline or an outset ring.

  • Minimum fix: In Fit mode, draw focus outside or above the image rather than with an inset shadow. Either:

    • drop ring-inset for the Fit branch, so the ring is outset (the Fit stage has no overflow clip), or
    • use focus-visible:outline focus-visible:outline-2 focus-visible:-outline-offset-2 focus-visible:outline-sky-400, since outlines paint above content.

Suggestions

These are optional follow-ups and are not sent to /fix.

S4: 🟢 Negated multi-class assertions are weak

In TaskVisualPreviews.test.tsx, expect(stage).not.toHaveClass('h-80', 'sm:h-[440px]', 'p-2') and expect(image).not.toHaveClass('min-h-16', 'min-w-32', 'bg-white') only fail when all the listed classes are present. That is how jest-dom treats a negated multi-class check. A regression that reintroduces just h-80 or bg-white would pass. Split these into one negative assertion per class.

S5: 🟢 Switching captures now shifts the trace below

The old fixed stage height was deliberately chosen so that switching captures never moved the content below. Now a loading placeholder (64px tall) grows to as much as 440px when the image loads. The stage also resizes between a wide image, a tall image and a fixed-height video. This follows from the objective, but it may be worth reserving height while an image is loading to reduce layout shift.

S6: 🟢 Very small or lazy images before decode

Before load, a Fit image with h-auto w-auto and no minimum size has no reserved box. Captures much smaller than the pane also get a correspondingly tiny frame. Neither is demonstrated to break anything, but it may be worth a quick check or test with a small (e.g. 64×32) capture.

Score

The objective is implemented well and the new layout tests are thorough. One localized accessibility regression (the hidden Fit-mode focus ring) should be fixed, and Shard 4 and Validate Changes are still pending.

Score: 6/10


🤖 Review Details

  • Model: Claude Opus 5.5
  • Time: 54s
  • Tokens: 49,453 (44,579 in / 4,874 out)
  • Cost: $0.32

View Task


💡 Next step: Comment /fix to address every F# merge blocker, /fix all for every pending blocker and suggestion, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them or request /fix all, and they never relax a merge blocker.

Copy and edit to choose records from this review:

/fix F2 S4 S5 S6

🤖 Review by ProPR

@propr-dev

propr-dev Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit 23a06d4

Addressed 1 AI review comment (IDs: 6390605692)

Addressed finding F2

Summary of Changes

Addressed F2 only: Fit mode now uses an outset focus ring that stays visible around opaque images.

Added keyboard-focus regressions for wide and tall captures; confirmed they failed before the fix and pass afterward.

Validation: 19 browser tests and 8 component tests passed. Captured and inspected both focused-state previews.

Visual preview

Fit keyboard focus: wide capture

Fit keyboard focus: wide capture

Keyboard focus draws a visible blue outset ring around an opaque 2560×600 capture in a narrow task pane.

Original unavailable: managed storage is unavailable or the upload has expired.

Fit keyboard focus: tall capture

Fit keyboard focus: tall capture

Keyboard focus remains visible around a tall 390×1800 capture in Fit mode.

Original unavailable: managed storage is unavailable or the upload has expired.


🤖 Implementation Details

  • Model: GPT-6.1 Sol (medium)
  • Requested By: @propr-ultrafix
  • Time: 3m 54s
  • Tokens: 759,577 (756,677 in / 2,900 out)
  • Cost: $0.19

View Task Execution


💡 ProPR Slash Commands
Command Description Example
/merge Merge target branch into this PR and resolve conflicts /merge
/review Request an AI code review /review or /review claude-sonnet
/fix Address review blockers, or blockers and suggestions with /fix all /fix, /fix all, or /fix F3 S5
/switch Change the AI model for this PR /switch claude-opus
/use Override the model for a single follow-up run /use claude-sonnet
/ultrafix Loop review→fix cycles until score goal is met /ultrafix or /ultrafix goal=8 max=10

🤖 Generated by ProPR

…hat stays visible around opaque images.

Addressed F2 only: Fit mode now uses an outset focus ring that stays visible around opaque images.

Added keyboard-focus regressions for wide and tall captures; confirmed they failed before the fix and pass afterward.

Validation: 19 browser tests and 8 component tests passed. Captured and inspected both focused-state previews.

PR: #3345
Comment by: @propr-ultrafix (ID: 0)
Model: GPT-6.1 Sol
@propr-dev

propr-dev Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — claude:claude-opus-5-5

Overall Evaluation

This PR fixes issue #3339. In Fit mode for images, the viewer now sizes the frame from the image's own dimensions instead of a fixed h-80 sm:h-[440px] box:

  • Frame: the stage becomes mx-auto flex w-fit max-w-full.
  • Image: h-auto w-auto max-h-80 sm:max-h-[440px] max-w-full, so it drives the frame size within the height and width caps.
  • Unchanged modes: 100% mode and videos keep the fixed, scrollable stage.
  • Placeholders: loading and failure placeholders keep a minimum size through [&>span]:min-h-16 [&>span]:min-w-32 on the trigger.
  • Focus ring: it is no longer inset in Fit mode, because an opaque image covering the trigger would hide it.

The change is small and stays in scope. The browser tests check the issue's exact repro (2560×600 and a tall 390×1800 capture in a 372px pane) as well as the earlier width sweep. All current-head checks pass. I found no blockers; the PR is ready to merge, with a few optional follow-ups below.

✅ Issue repro is asserted geometrically — The new e2e tests in propr-ui/e2e/task-details-visual-previews.pw.ts check three things for both very wide and very tall captures in a narrow pane:

  • the frame and the image match to within 1px;
  • the frame's ratio matches the capture's ratio to within 0.01;
  • the frame stays within the 320px height and 372px width caps.

✅ Placeholder regressions were anticipated — Once the image's own min-size classes were removed, the [&>span] min sizes keep the loading and unavailable spans visible. A 2 widths × 4 failure-mode matrix (403/404/network/decode) asserts a box of at least 128×64 for both the pending and failed states.

✅ Zoom toggling preserves one tree — The stage div, the trigger button and the keyed PreviewImage persist across Fit and 100%. Only class strings change, so toggling zoom still restyles the image rather than fetching it again. The 100% test also checks that the fixed stage height returns.

Merge blockers

No merge blockers.

Suggestions

These are optional follow-ups and are not sent to /fix.

S7: 🟢 Frame collapses while direct-URL images load

GitHub attachment URLs resolve to status: 'ready' immediately in usePreviewMediaSource, so they render an <img> rather than the loading span. Before this PR, that <img> carried min-h-16 min-w-32 and kept a visible box during download. In Fit mode it is now h-auto w-auto with no minimum size, so on a slow network the frame is roughly 0×0 until the image decodes, then jumps to full size. The new placeholder tests only cover protected /api/preview-media URLs, which render a span. Consider a min size on the frame (or on the img) until load, and add a delayed-route test for an asset(...) image.

S8: 🟢 Negated multi-class assertions are weak

In TaskVisualPreviews.test.tsx, expect(stage).not.toHaveClass('h-80', 'sm:h-[440px]', 'p-2') and expect(image).not.toHaveClass('min-h-16', 'min-w-32', 'bg-white') pass whenever any one of the listed classes is missing. jest-dom's positive form requires all classes, so the negation only fails if all of them are present. The assertions therefore would not catch a single class reappearing. Splitting them into one not.toHaveClass per class would make the regression guard real.

S9: 🟢 Verify intrinsic sizing in WebKit

The w-fit frame matching the image depends on the browser carrying max-h-80 through the aspect ratio into the image's width. It also depends on how a percentage max-w-full resolves inside a fit-content parent. Both are verified here only in Chromium, the only browser Playwright runs in CI. Running the new narrow-pane tests once under WebKit would confirm that a tall capture does not leave checkerboard beside the image in Safari.

S10: 🟢 Trace now shifts when switching captures

The old comment promised that switching captures "never moves the trace below." Now each image (and every image↔video switch) changes the section's height. That follows directly from the issue's request, so it is acceptable. If the jump feels disruptive in the task pane, a fixed outer container with the ratio-fitted frame centred inside it would keep both behaviours.

Score

The change is focused and correctly implements an aspect-ratio-following Fit frame. It has strong geometric e2e coverage of the reported repro and of the placeholder states, and all current-head checks pass. Remaining items are optional polish.

Score: 8/10


🤖 Review Details

  • Model: Claude Opus 5.5
  • Time: 46s
  • Tokens: 47,185 (42,900 in / 4,285 out)
  • Cost: $0.30

View Task


💡 Next step: Comment /fix to address every F# merge blocker, /fix all for every pending blocker and suggestion, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them or request /fix all, and they never relax a merge blocker.

Copy and edit to choose records from this review:

/fix S7 S8 S9 S10

🤖 Review by ProPR

@propr-dev propr-dev Bot removed the ultrafix label Oct 24, 2026
@integry
integry merged commit c3c17c7 into main Oct 24, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Visual evidence Fit frame ignores the image's aspect ratio

1 participant