Repository navigation
[3260 by Claude Opus 5.5] The current "Visual Evidence" component fails basic diagnostic ergonomics because it treats functional test artifacts like a Dribbble marketing showcase. - #3261
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…TaskDetails/TaskVisualPreviews.tsx` as the vertical stack the issue describes. The unit tests, the Playwright specs for the task and goal visual previews, lint and typecheck all pass. What changed: - Faux window gone: the dot bar, the rounded window shell and the fixed 224px two-column canvas are removed. The capture now sits on a full-width stage with a 1px border and a light checkerboard background. The stage is 440px tall on desktop and 320px on phones. - Stacked layout: - Header: the section label and capture count, the tab switcher, a Fit / 100% toggle, a lightbox button, and an open-original ↗ link. The ↗ link only appears for public URLs, as before. - Context strip: the title and description, then the viewport, the measured resolution chip (e.g. `1440×900`) and Screenshot/Video. - Stage: the image or video, using the full width. - Fit (default): the whole capture is scaled into the stage, so nothing is cut off. Clicking the image still opens the lightbox. - 100%: the capture is shown at real pixel size and you scroll around it inside a stage of the same height. Switching modes keeps the image that's already loaded, so private media isn't downloaded again. - Tabs: each chip has a device icon (monitor, tablet, phone or film), a number (`01`, `02`…) and a label. The label is Before/After, then the viewport, then the capture's own title, so "Capture 3" no longer appears. A capture whose title mentions a failure or an assertion gets the red tint the issue asks for. Where it differs from the issue mockup: - The resolution chip comes from the loaded image or video itself, because the published preview data only has type, title, description and URL. That means step numbers, run timing and failure flags aren't available, so the "Step 14/18 • 142ms" part of the mockup isn't shown, and the red tint is a guess from words in the title. - The stage is a fixed height rather than a maximum height, so switching captures never shifts the execution trace below it. Tests: I updated the unit tests and rewrote the Playwright layout test, which runs at widths from 320 to 2560px. It checks that the stage spans the section below the context strip, that the whole capture fits in Fit mode, the resolution chip, and that 100% shows the image 1440px wide and scrolls. The 2560px case first failed twice in three repeats when switching to 100% reloaded the image. Keeping the loaded image across modes fixed that, and all 15 repeats then passed. I captured preview screenshots of the new layout at 1440px in Fit and 100%, and at 390px.
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR replaces the constrained two-column preview with a full-width inspection stage and adds Fit/100% controls. It needs a small correctness fix before merge. ✅ Inline inspection — Fit uses ✅ Media handling preserved — Trusted-media filtering, authenticated media loading, video controls, and image-only lightbox navigation remain intact. ✅ Responsive coverage — Updated browser tests cover five viewport widths, capture switching, dimensions, and zoom behavior. The supplied current-head checks report no failures. The issue description is explicitly truncated, so this review cannot assess its unseen layout requirements. Verification was limited to static analysis; no commands were run. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Successful assertions are marked as failures
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise real video sizingThe browser fixture aborts the video request, so checking for S2: 🟢 Show transparency through the checkerboardBoth image zoom modes apply ScoreThe implementation substantially improves inspection ergonomics and has passing current-head checks, but its new failure classification produces a demonstrably incorrect diagnostic signal. The required correction is localized. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/ultrafix |
|
🔄 Ultrafix loop started (goal: 8/10, max cycles: 10) First action:
|
|
✅ Applied the requested follow-up changes in commit 51e67a4
Summary of ChangesI've fixed F1. A capture titled "Checkout assertion passed" no longer gets the red failure styling, and "Assertion failed at 390px" is still flagged. Fix: In Regression test: In Verification:
Other uses checked: I captured a preview of the evidence section showing a neutral chip, a red "Assertion failed" chip, and a neutral "Checkout assertion passed" chip. To do that I ran a temporary Playwright spec based on the existing visual-previews fixture, then deleted it. The Gmail, Google Calendar and Google Drive connectors still need to be authorised in the claude.ai connector settings. This task didn't need them. Visual previewCapture switcher with passed and failed assertions
Chip 02 'Assertion failed at 390px' is tinted red; chip 03 'Checkout assertion passed' keeps the ordinary neutral styling. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
… no longer gets the red failure styling, and "Assertion failed at 390px" is still flagged. I've fixed F1. A capture titled "Checkout assertion passed" no longer gets the red failure styling, and "Assertion failed at 390px" is still flagged. **Fix:** In `propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx:44`, I took the standalone `assertion` word out of `isFailureCapture`. A capture is now flagged only when its title says "fail", "fails", "failed", "failing" or "failure", so mentioning an assertion is no longer enough. "Assertion failed …" is still caught by the word "failed". **Regression test:** In `TaskVisualPreviews.test.tsx`, the switcher test now has a third capture, "Checkout assertion passed". It checks that this chip has none of the red classes and keeps the normal neutral classes, while "Assertion failed at 390px" stays red. **Verification:** - The component's unit tests pass (8 of 8), and eslint with `--max-warnings 0` is clean on both files. - I put the old pattern back temporarily: the updated test failed, so it does catch this bug. Then I restored the fix. - I didn't rerun the existing Playwright specs, because they don't mention assertion or failure titles. **Other uses checked:** `isFailureCapture` is the only failure check from this title pattern in `propr-ui/src`, and `CaptureSwitcher` is its only caller (both the chip tint and the number colour come from it). There were no other instances of the same mistake. I captured a preview of the evidence section showing a neutral chip, a red "Assertion failed" chip, and a neutral "Checkout assertion passed" chip. To do that I ran a temporary Playwright spec based on the existing visual-previews fixture, then deleted it. The Gmail, Google Calendar and Google Drive connectors still need to be authorised in the claude.ai connector settings. This task didn't need them. PR: #3261 Comment by: @integry (ID: 6289814014) Model: Claude Opus 5.5
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR replaces the split faux-window layout with a full-width inspection stage, context strip, and Fit/100% controls. It is merge-ready within the visible scope, conditional on the two pending checks completing successfully. ✅ Improved artifact inspection — Fit uses contained media, while 100% removes image width constraints and enables scrolling within the stage. ✅ Preserved media boundaries — Trusted-media filtering, authenticated media handling, and image-only lightbox navigation remain intact. ✅ Responsive coverage — Updated browser tests check stage geometry, loaded dimensions, zoom behavior, and capture switching across five viewport widths. This review used static analysis only; no commands were run. The supplied issue specification is truncated, so compliance with its unseen requirements cannot be assessed. Current checks report no failures, 15 passes, and two pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Exercise loaded video geometryThe browser fixture in S4: 🟢 Expose transparency against the checkerboardIn ScoreThe implementation addresses the visible layout problems with focused changes and useful regression coverage. No merge-blocking defect was verified; remaining uncertainty concerns unexecuted browser behavior and the truncated issue specification. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3260
Branch:
3260/claude-opus-5-5-the-current-visual-eviden-20261019-1156-npoCommits: ✅ Changes committed (270676e)
AI Processing Completed
Execution Details:
Summary:
I've rebuilt the Visual Evidence section in
propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsxas the vertical stack the issue describes. The unit tests, the Playwright specs for the task and goal visual previews, lint and typecheck all pass.What changed:
1440×900) and Screenshot/Video.01,02…) and a label. The label is Before/After, then the viewport, then the capture's own title, so "Capture 3" no longer appears. A capture whose title mentions a failure or an assertion gets the red tint the issue asks for.Where it differs from the issue mockup:
Tests: I updated the unit tests and rewrote the Playwright layout test, which runs at widths from 320 to 2560px. It checks that the stage spans the section below the context strip, that the whole capture fits in Fit mode, the resolution chip, and that 100% shows the image 1440px wide and scrolls. The 2560px case first failed twice in three repeats when switching to 100% reloaded the image. Keeping the loaded image across modes fixed that, and all 15 repeats then passed.
I captured preview screenshots of the new layout at 1440px in Fit and 100%, and at 390px.
Detailed Logs:
e3709678-a4a2-4660-a679-b6e545c243e1Log files stored at:
/usr/src/app/logs/executions/issue-3260-2026-10-19T12-09-59-677Z-conversation.json/usr/src/app/logs/executions/issue-3260-2026-10-19T12-09-59-677Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3260.
💡 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
Visual evidence at 1440px (Fit)
Faux window and 60/40 split removed: numbered device tabs, Fit/100% toggle, a one-line context strip with the measured 1440×900 resolution, and a full-width checkerboard stage showing the whole capture unclipped.
Original unavailable: managed storage is unavailable or the upload has expired.
Visual evidence at 390px
On a phone the header, tabs and controls wrap above the context strip and stage.
Original unavailable: managed storage is unavailable or the upload has expired.
Visual evidence at 100%
100% mode renders the capture's real pixels in a scrollable stage of the same height, so it can be panned without opening the lightbox.
Original unavailable: managed storage is unavailable or the upload has expired.