From 270676ece3fd3a26f79ae80dfa84ddcc76744656 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 12:09:54 +0000 Subject: [PATCH 1/2] I've rebuilt the Visual Evidence section in `propr-ui/src/components/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. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../e2e/task-details-visual-previews.pw.ts | 47 ++-- .../TaskDetails/TaskVisualPreviews.test.tsx | 67 ++++-- .../TaskDetails/TaskVisualPreviews.tsx | 211 ++++++++++++------ 3 files changed, 227 insertions(+), 98 deletions(-) diff --git a/propr-ui/e2e/task-details-visual-previews.pw.ts b/propr-ui/e2e/task-details-visual-previews.pw.ts index a878ae5c31..5386934b89 100644 --- a/propr-ui/e2e/task-details-visual-previews.pw.ts +++ b/propr-ui/e2e/task-details-visual-previews.pw.ts @@ -58,7 +58,7 @@ async function capture(page: Page, name: string) { const noHorizontalOverflow = (page: Page) => page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth); for (const width of [320, 390, 1024, 1440, 2560]) { - test(`frames one capture at a time in a fixed-height canvas at ${width}px`, async ({ page }) => { + test(`stacks the context strip over a full-width imaging stage at ${width}px`, async ({ page }) => { await fixture(page); await page.setViewportSize({ width, height: 900 }); await page.goto(`/tasks/${taskId}`); @@ -66,28 +66,45 @@ for (const width of [320, 390, 1024, 1440, 2560]) { await expect(section.getByRole('heading', { name: 'Visual evidence (3 captures)' })).toBeVisible(); const image = section.getByAltText('Task queue at desktop width'); await expect(image).toBeVisible(); - const canvas = section.getByTestId('visual-evidence-canvas'); - const canvasBox = (await canvas.boundingBox())!; - if (width >= 640) { - // Side by side: the framed capture takes three fifths, its description the rest, 224px tall. - expect(Math.abs(canvasBox.height - 224)).toBeLessThanOrEqual(1); - const imageBox = (await image.boundingBox())!; - expect(imageBox.width / canvasBox.width).toBeGreaterThan(0.55); - expect(imageBox.width / canvasBox.width).toBeLessThan(0.62); - } else { - expect(canvasBox.height).toBeLessThanOrEqual(420); - } + await expect.poll(() => image.evaluate(node => (node as HTMLImageElement).complete && (node as HTMLImageElement).naturalWidth)).toBeGreaterThan(0); + const stage = section.getByTestId('visual-evidence-canvas'); + const stageBox = (await stage.boundingBox())!; + const sectionContentWidth = await section.evaluate(node => { + const style = getComputedStyle(node); + return node.clientWidth - parseFloat(style.paddingLeft) - parseFloat(style.paddingRight); + }); + // The stage spans the section and sits below the context strip, not beside it. + expect(Math.abs(stageBox.width - sectionContentWidth)).toBeLessThanOrEqual(2); + const contextBox = (await section.getByTestId('visual-evidence-context').boundingBox())!; + expect(contextBox.y + contextBox.height).toBeLessThanOrEqual(stageBox.y + 1); + expect(Math.abs(stageBox.height - (width >= 640 ? 440 : 320))).toBeLessThanOrEqual(2); + // Fit shows the whole capture: nothing is clipped by the stage. + const imageBox = (await image.boundingBox())!; + expect(imageBox.x).toBeGreaterThanOrEqual(stageBox.x); + expect(imageBox.y).toBeGreaterThanOrEqual(stageBox.y); + expect(imageBox.x + imageBox.width).toBeLessThanOrEqual(stageBox.x + stageBox.width + 0.5); + expect(imageBox.y + imageBox.height).toBeLessThanOrEqual(stageBox.y + stageBox.height + 0.5); + await expect(section.getByTestId('visual-evidence-dimensions')).toHaveText(`1440×900`); await expect(section.getByText('Private PR evidence served through the authenticated application media path.')).toBeVisible(); await expect(section.locator('img, video')).toHaveCount(1); + if (width === 390 || width === 1440) await capture(page, `task-visual-previews-${width}`); + + // 100% renders the capture's own pixels and scrolls inside the stage. + await section.getByRole('button', { name: '100%' }).click(); + await expect(stage).toHaveAttribute('data-zoom', 'actual'); + expect(Math.round((await image.boundingBox())!.width)).toBe(1440 + 2); + expect(await stage.evaluate(node => node.scrollWidth > node.clientWidth || node.scrollHeight > node.clientHeight)).toBe(true); + expect(Math.abs((await stage.boundingBox())!.height - stageBox.height)).toBeLessThanOrEqual(1); + if (width === 1440) await capture(page, `task-visual-previews-actual-size-${width}`); + await section.getByRole('button', { name: 'Fit' }).click(); const switcher = section.getByRole('group', { name: 'Captures' }); - await switcher.getByRole('button', { name: 'Capture 2' }).click(); + await switcher.getByRole('button', { name: 'Checkout walkthrough' }).click(); await expect(section.locator('video[controls]')).toHaveCount(1); - expect(Math.abs((await canvas.boundingBox())!.height - canvasBox.height)).toBeLessThanOrEqual(1); + expect(Math.abs((await stage.boundingBox())!.height - stageBox.height)).toBeLessThanOrEqual(1); await switcher.getByRole('button', { name: 'Desktop' }).click(); await expect(image).toBeVisible(); expect(await noHorizontalOverflow(page)).toBe(true); - if (width === 390 || width === 1440) await capture(page, `task-visual-previews-${width}`); }); } diff --git a/propr-ui/src/components/TaskDetails/TaskVisualPreviews.test.tsx b/propr-ui/src/components/TaskDetails/TaskVisualPreviews.test.tsx index c0b43685ff..da6a01724c 100644 --- a/propr-ui/src/components/TaskDetails/TaskVisualPreviews.test.tsx +++ b/propr-ui/src/components/TaskDetails/TaskVisualPreviews.test.tsx @@ -5,26 +5,28 @@ import TaskVisualPreviews from './TaskVisualPreviews'; const asset = (id: string) => `https://github.com/user-attachments/assets/${id}`; describe('TaskVisualPreviews', () => { - it('frames one capture at a time beside its title, description and viewport', () => { + it('stacks the context strip over a full-width stage that fits the whole capture', () => { const { container } = render(); expect(screen.getByRole('heading', { name: 'Visual evidence (2 captures)' })).toBeInTheDocument(); - const canvas = screen.getByTestId('visual-evidence-canvas'); - expect(canvas).toHaveClass('sm:h-56', 'sm:flex-row'); + const stage = screen.getByTestId('visual-evidence-canvas'); + expect(stage).toHaveClass('w-full', 'sm:h-[440px]', 'items-center', 'justify-center'); + expect(stage).toHaveAttribute('data-zoom', 'fit'); const image = screen.getByAltText('Task list at Desktop 1920px'); expect(image).toHaveAttribute('src', asset('image')); - expect(image).toHaveClass('h-full', 'w-full', 'object-cover', 'object-left-top'); - expect(within(canvas).getByText('Row keeps its full title')).toBeInTheDocument(); - expect(within(canvas).getByText('Desktop 1920px')).toBeInTheDocument(); - // A light browser window frames the capture, sized like one, with no black bezel around it. - const frame = within(canvas).getByTestId('visual-evidence-window'); - expect(frame).toHaveClass('border-slate-200', 'bg-white'); - expect(within(frame).getByTestId('visual-evidence-window-size')).toHaveTextContent(/^1920px$/); - expect(canvas.innerHTML).not.toMatch(/bg-slate-950 p-|bg-slate-900 /); - expect(screen.getByText('Click to inspect full resolution')).toBeInTheDocument(); + // Fit never clips: the whole capture is scaled into the stage. + expect(image).toHaveClass('max-h-full', 'max-w-full', 'object-contain'); + expect(image).not.toHaveClass('object-cover'); + const context = screen.getByTestId('visual-evidence-context'); + expect(within(context).getByText('Row keeps its full title')).toBeInTheDocument(); + expect(within(context).getByText('Desktop 1920px')).toBeInTheDocument(); + expect(within(context).getByText('Screenshot')).toBeInTheDocument(); + // No faux browser window around the capture. + expect(screen.queryByTestId('visual-evidence-window')).toBeNull(); + expect(container.querySelectorAll('.rounded-full')).toHaveLength(0); expect(screen.getByRole('link', { name: 'Open original: Task list at Desktop 1920px' })).toHaveAttribute('href', asset('image')); // Only the selected capture is on screen: the video waits behind the switcher. expect(container.querySelector('video')).toBeNull(); @@ -32,6 +34,38 @@ describe('TaskVisualPreviews', () => { expect(container.querySelector('img[src="https://evil.test/image.png"]')).toBeNull(); }); + it('shows the capture at its real pixel size in a scrollable stage at 100%', () => { + render(); + const zoom = screen.getByRole('group', { name: 'Zoom' }); + expect(within(zoom).getByRole('button', { name: 'Fit' })).toHaveAttribute('aria-pressed', 'true'); + fireEvent.click(within(zoom).getByRole('button', { name: '100%' })); + expect(within(zoom).getByRole('button', { name: '100%' })).toHaveAttribute('aria-pressed', 'true'); + const stage = screen.getByTestId('visual-evidence-canvas'); + expect(stage).toHaveAttribute('data-zoom', 'actual'); + expect(stage).toHaveClass('overflow-auto'); + expect(screen.getByAltText('Tall mobile page')).toHaveClass('max-w-none', 'h-auto', 'w-auto'); + }); + + it('reports the captured resolution once the image loads', () => { + render(); + const image = screen.getByAltText('Dialog'); + Object.defineProperty(image, 'naturalWidth', { value: 1440 }); + Object.defineProperty(image, 'naturalHeight', { value: 900 }); + fireEvent.load(image); + expect(screen.getByTestId('visual-evidence-dimensions')).toHaveTextContent('1440×900'); + }); + + it('numbers captures in the switcher, marks their device and flags failed ones', () => { + render(); + const switcher = screen.getByRole('group', { name: 'Captures' }); + expect(within(switcher).getAllByRole('button').map(button => button.textContent)).toEqual(['01Desktop 1440px', '02390px']); + expect(within(switcher).getByRole('button', { name: '390px' })).toHaveClass('border-red-300', 'text-red-700', 'bg-red-50'); + expect(within(switcher).getByRole('button', { name: 'Desktop 1440px' })).not.toHaveClass('border-red-300'); + }); + it('switches captures in place and plays videos with their controls', () => { const { container } = render( { { type: 'video', title: 'Walkthrough', url: asset('video') }, ]} />); const switcher = screen.getByRole('group', { name: 'Captures' }); - expect(within(switcher).getAllByRole('button').map(button => button.textContent)).toEqual(['Before', 'After', 'Capture 3']); + expect(within(switcher).getAllByRole('button').map(button => button.textContent)).toEqual(['01Before', '02After', '03Walkthrough']); expect(within(switcher).getByRole('button', { name: 'Before' })).toHaveAttribute('aria-pressed', 'true'); fireEvent.click(within(switcher).getByRole('button', { name: 'After' })); @@ -47,11 +81,12 @@ describe('TaskVisualPreviews', () => { expect(screen.queryByAltText('Before the fix')).toBeNull(); expect(screen.getByText('Mobile 390px')).toBeInTheDocument(); - fireEvent.click(within(switcher).getByRole('button', { name: 'Capture 3' })); + fireEvent.click(within(switcher).getByRole('button', { name: 'Walkthrough' })); const video = container.querySelector('video'); expect(video).toHaveAttribute('src', asset('video')); expect(video).toHaveAttribute('controls'); expect(screen.queryByRole('button', { name: 'Lightbox' })).toBeNull(); + expect(screen.queryByRole('group', { name: 'Zoom' })).toBeNull(); }); it('opens images in an in-app lightbox, skips videos, and restores focus on close', () => { @@ -60,7 +95,7 @@ describe('TaskVisualPreviews', () => { { type: 'video', title: 'Walkthrough', url: asset('video') }, { type: 'image', title: 'Second', url: asset('second') }, ]} />); - fireEvent.click(screen.getByRole('button', { name: 'Capture 3' })); + fireEvent.click(screen.getByRole('button', { name: 'Second' })); const trigger = screen.getByRole('button', { name: 'Open full-size preview: Second' }); expect(trigger).toHaveAttribute('aria-haspopup', 'dialog'); trigger.focus(); @@ -76,7 +111,7 @@ describe('TaskVisualPreviews', () => { expect(screen.queryByRole('dialog')).toBeNull(); expect(trigger).toHaveFocus(); // The canvas follows the capture the lightbox was left on. - expect(screen.getByRole('button', { name: 'Capture 1' })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: 'First' })).toHaveAttribute('aria-pressed', 'true'); }); it('opens the lightbox from the header button too', () => { diff --git a/propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx b/propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx index 4e0b1d5810..c2048fd162 100644 --- a/propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx +++ b/propr-ui/src/components/TaskDetails/TaskVisualPreviews.tsx @@ -1,15 +1,28 @@ -import { useEffect, useRef, useState } from 'react'; -import { ExternalLink, Maximize2, ZoomIn } from 'lucide-react'; +import { useEffect, useLayoutEffect, useRef, useState, type CSSProperties, type RefObject } from 'react'; +import { ArrowUpRight, Film, Image as ImageIcon, Maximize2, Monitor, Smartphone, Tablet } from 'lucide-react'; import { trustedApplicationPreviewMediaUrl, trustedPreviewMedia, type PublishedVisualPreview } from '@propr/shared'; import { PreviewImage, PreviewVideo } from '../PreviewMedia'; import PreviewLightbox from '../PreviewLightbox'; +type DeviceKind = 'desktop' | 'tablet' | 'mobile'; +type ZoomMode = 'fit' | 'actual'; + /** The width a capture was taken at, when its title or description says, e.g. `1920`. */ function captureWidth(preview: PublishedVisualPreview): string | null { const text = `${preview.title} ${preview.description ?? ''}`; return text.match(/\b(\d{3,4})\s*(?:px\b|[x×]\s*\d{3,4})/i)?.[1] ?? null; } +/** The device class a capture was taken on: named in its title or description, or implied by its width. */ +function captureDevice(preview: PublishedVisualPreview): DeviceKind | null { + const text = `${preview.title} ${preview.description ?? ''}`; + const named = text.match(/\b(desktop|tablet|mobile)\b/i)?.[1]?.toLowerCase() as DeviceKind | undefined; + if (named) return named; + const width = Number(captureWidth(preview)); + if (!width) return null; + return width < 640 ? 'mobile' : width < 1024 ? 'tablet' : 'desktop'; +} + /** The viewport a capture was taken at, when its title or description says, e.g. `Desktop 1920px`. */ function captureViewport(preview: PublishedVisualPreview): string | null { const text = `${preview.title} ${preview.description ?? ''}`; @@ -20,18 +33,110 @@ function captureViewport(preview: PublishedVisualPreview): string | null { return label ?? (width ? `${width}px` : null); } -/** A short name for a capture in the switcher: its viewport, a before/after side, or its position. */ -function captureLabel(preview: PublishedVisualPreview, index: number): string { +/** A short name for a capture in the switcher: a before/after side, its viewport, or its own title. */ +function captureLabel(preview: PublishedVisualPreview): string { const side = preview.title.match(/\b(before|after)\b/i)?.[1]; if (side) return side[0].toUpperCase() + side.slice(1).toLowerCase(); - return captureViewport(preview) ?? `Capture ${index + 1}`; + return captureViewport(preview) ?? preview.title; +} + +/** Captures whose title reports a failed run or assertion are flagged in the switcher. */ +const isFailureCapture = (preview: PublishedVisualPreview) => /\b(fail(?:s|ed|ing|ure)?|assertion)\b/i.test(preview.title); + +function CaptureIcon({ preview }: { preview: PublishedVisualPreview }) { + const device = captureDevice(preview); + const Icon = preview.type === 'video' ? Film : device ? { desktop: Monitor, tablet: Tablet, mobile: Smartphone }[device] : ImageIcon; + return