Skip to content

Commit cdb52e8

Browse files
authored
Merge pull request #3261 from integry/3260/claude-opus-5-5-the-current-visual-eviden-20261019-1156-npo
[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.
2 parents 42931b1 + 51e67a4 commit cdb52e8

3 files changed

Lines changed: 231 additions & 98 deletions

File tree

‎propr-ui/e2e/task-details-visual-previews.pw.ts‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,36 +58,53 @@ async function capture(page: Page, name: string) {
5858
const noHorizontalOverflow = (page: Page) => page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth);
5959

6060
for (const width of [320, 390, 1024, 1440, 2560]) {
61-
test(`frames one capture at a time in a fixed-height canvas at ${width}px`, async ({ page }) => {
61+
test(`stacks the context strip over a full-width imaging stage at ${width}px`, async ({ page }) => {
6262
await fixture(page);
6363
await page.setViewportSize({ width, height: 900 });
6464
await page.goto(`/tasks/${taskId}`);
6565
const section = page.getByRole('region', { name: /Visual evidence/ });
6666
await expect(section.getByRole('heading', { name: 'Visual evidence (3 captures)' })).toBeVisible();
6767
const image = section.getByAltText('Task queue at desktop width');
6868
await expect(image).toBeVisible();
69-
const canvas = section.getByTestId('visual-evidence-canvas');
70-
const canvasBox = (await canvas.boundingBox())!;
71-
if (width >= 640) {
72-
// Side by side: the framed capture takes three fifths, its description the rest, 224px tall.
73-
expect(Math.abs(canvasBox.height - 224)).toBeLessThanOrEqual(1);
74-
const imageBox = (await image.boundingBox())!;
75-
expect(imageBox.width / canvasBox.width).toBeGreaterThan(0.55);
76-
expect(imageBox.width / canvasBox.width).toBeLessThan(0.62);
77-
} else {
78-
expect(canvasBox.height).toBeLessThanOrEqual(420);
79-
}
69+
await expect.poll(() => image.evaluate(node => (node as HTMLImageElement).complete && (node as HTMLImageElement).naturalWidth)).toBeGreaterThan(0);
70+
const stage = section.getByTestId('visual-evidence-canvas');
71+
const stageBox = (await stage.boundingBox())!;
72+
const sectionContentWidth = await section.evaluate(node => {
73+
const style = getComputedStyle(node);
74+
return node.clientWidth - parseFloat(style.paddingLeft) - parseFloat(style.paddingRight);
75+
});
76+
// The stage spans the section and sits below the context strip, not beside it.
77+
expect(Math.abs(stageBox.width - sectionContentWidth)).toBeLessThanOrEqual(2);
78+
const contextBox = (await section.getByTestId('visual-evidence-context').boundingBox())!;
79+
expect(contextBox.y + contextBox.height).toBeLessThanOrEqual(stageBox.y + 1);
80+
expect(Math.abs(stageBox.height - (width >= 640 ? 440 : 320))).toBeLessThanOrEqual(2);
81+
// Fit shows the whole capture: nothing is clipped by the stage.
82+
const imageBox = (await image.boundingBox())!;
83+
expect(imageBox.x).toBeGreaterThanOrEqual(stageBox.x);
84+
expect(imageBox.y).toBeGreaterThanOrEqual(stageBox.y);
85+
expect(imageBox.x + imageBox.width).toBeLessThanOrEqual(stageBox.x + stageBox.width + 0.5);
86+
expect(imageBox.y + imageBox.height).toBeLessThanOrEqual(stageBox.y + stageBox.height + 0.5);
87+
await expect(section.getByTestId('visual-evidence-dimensions')).toHaveText(`1440×900`);
8088
await expect(section.getByText('Private PR evidence served through the authenticated application media path.')).toBeVisible();
8189
await expect(section.locator('img, video')).toHaveCount(1);
90+
if (width === 390 || width === 1440) await capture(page, `task-visual-previews-${width}`);
91+
92+
// 100% renders the capture's own pixels and scrolls inside the stage.
93+
await section.getByRole('button', { name: '100%' }).click();
94+
await expect(stage).toHaveAttribute('data-zoom', 'actual');
95+
expect(Math.round((await image.boundingBox())!.width)).toBe(1440 + 2);
96+
expect(await stage.evaluate(node => node.scrollWidth > node.clientWidth || node.scrollHeight > node.clientHeight)).toBe(true);
97+
expect(Math.abs((await stage.boundingBox())!.height - stageBox.height)).toBeLessThanOrEqual(1);
98+
if (width === 1440) await capture(page, `task-visual-previews-actual-size-${width}`);
99+
await section.getByRole('button', { name: 'Fit' }).click();
82100

83101
const switcher = section.getByRole('group', { name: 'Captures' });
84-
await switcher.getByRole('button', { name: 'Capture 2' }).click();
102+
await switcher.getByRole('button', { name: 'Checkout walkthrough' }).click();
85103
await expect(section.locator('video[controls]')).toHaveCount(1);
86-
expect(Math.abs((await canvas.boundingBox())!.height - canvasBox.height)).toBeLessThanOrEqual(1);
104+
expect(Math.abs((await stage.boundingBox())!.height - stageBox.height)).toBeLessThanOrEqual(1);
87105
await switcher.getByRole('button', { name: 'Desktop' }).click();
88106
await expect(image).toBeVisible();
89107
expect(await noHorizontalOverflow(page)).toBe(true);
90-
if (width === 390 || width === 1440) await capture(page, `task-visual-previews-${width}`);
91108
});
92109
}
93110

‎propr-ui/src/components/TaskDetails/TaskVisualPreviews.test.tsx‎

Lines changed: 55 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,53 +5,92 @@ import TaskVisualPreviews from './TaskVisualPreviews';
55
const asset = (id: string) => `https://github.com/user-attachments/assets/${id}`;
66

77
describe('TaskVisualPreviews', () => {
8-
it('frames one capture at a time beside its title, description and viewport', () => {
8+
it('stacks the context strip over a full-width stage that fits the whole capture', () => {
99
const { container } = render(<TaskVisualPreviews previews={[
1010
{ type: 'image', title: 'Task list at Desktop 1920px', description: 'Row keeps its full title', url: asset('image') },
1111
{ type: 'video', title: 'Checkout flow', url: asset('video') },
1212
{ type: 'image', title: 'Untrusted', url: 'https://evil.test/image.png' },
1313
]} />);
1414
expect(screen.getByRole('heading', { name: 'Visual evidence (2 captures)' })).toBeInTheDocument();
15-
const canvas = screen.getByTestId('visual-evidence-canvas');
16-
expect(canvas).toHaveClass('sm:h-56', 'sm:flex-row');
15+
const stage = screen.getByTestId('visual-evidence-canvas');
16+
expect(stage).toHaveClass('w-full', 'sm:h-[440px]', 'items-center', 'justify-center');
17+
expect(stage).toHaveAttribute('data-zoom', 'fit');
1718
const image = screen.getByAltText('Task list at Desktop 1920px');
1819
expect(image).toHaveAttribute('src', asset('image'));
19-
expect(image).toHaveClass('h-full', 'w-full', 'object-cover', 'object-left-top');
20-
expect(within(canvas).getByText('Row keeps its full title')).toBeInTheDocument();
21-
expect(within(canvas).getByText('Desktop 1920px')).toBeInTheDocument();
22-
// A light browser window frames the capture, sized like one, with no black bezel around it.
23-
const frame = within(canvas).getByTestId('visual-evidence-window');
24-
expect(frame).toHaveClass('border-slate-200', 'bg-white');
25-
expect(within(frame).getByTestId('visual-evidence-window-size')).toHaveTextContent(/^1920px$/);
26-
expect(canvas.innerHTML).not.toMatch(/bg-slate-950 p-|bg-slate-900 /);
27-
expect(screen.getByText('Click to inspect full resolution')).toBeInTheDocument();
20+
// Fit never clips: the whole capture is scaled into the stage.
21+
expect(image).toHaveClass('max-h-full', 'max-w-full', 'object-contain');
22+
expect(image).not.toHaveClass('object-cover');
23+
const context = screen.getByTestId('visual-evidence-context');
24+
expect(within(context).getByText('Row keeps its full title')).toBeInTheDocument();
25+
expect(within(context).getByText('Desktop 1920px')).toBeInTheDocument();
26+
expect(within(context).getByText('Screenshot')).toBeInTheDocument();
27+
// No faux browser window around the capture.
28+
expect(screen.queryByTestId('visual-evidence-window')).toBeNull();
29+
expect(container.querySelectorAll('.rounded-full')).toHaveLength(0);
2830
expect(screen.getByRole('link', { name: 'Open original: Task list at Desktop 1920px' })).toHaveAttribute('href', asset('image'));
2931
// Only the selected capture is on screen: the video waits behind the switcher.
3032
expect(container.querySelector('video')).toBeNull();
3133
expect(screen.queryByText('Untrusted')).toBeNull();
3234
expect(container.querySelector('img[src="https://evil.test/image.png"]')).toBeNull();
3335
});
3436

37+
it('shows the capture at its real pixel size in a scrollable stage at 100%', () => {
38+
render(<TaskVisualPreviews previews={[{ type: 'image', title: 'Tall mobile page', url: asset('tall') }]} />);
39+
const zoom = screen.getByRole('group', { name: 'Zoom' });
40+
expect(within(zoom).getByRole('button', { name: 'Fit' })).toHaveAttribute('aria-pressed', 'true');
41+
fireEvent.click(within(zoom).getByRole('button', { name: '100%' }));
42+
expect(within(zoom).getByRole('button', { name: '100%' })).toHaveAttribute('aria-pressed', 'true');
43+
const stage = screen.getByTestId('visual-evidence-canvas');
44+
expect(stage).toHaveAttribute('data-zoom', 'actual');
45+
expect(stage).toHaveClass('overflow-auto');
46+
expect(screen.getByAltText('Tall mobile page')).toHaveClass('max-w-none', 'h-auto', 'w-auto');
47+
});
48+
49+
it('reports the captured resolution once the image loads', () => {
50+
render(<TaskVisualPreviews previews={[{ type: 'image', title: 'Dialog', url: asset('dialog') }]} />);
51+
const image = screen.getByAltText('Dialog');
52+
Object.defineProperty(image, 'naturalWidth', { value: 1440 });
53+
Object.defineProperty(image, 'naturalHeight', { value: 900 });
54+
fireEvent.load(image);
55+
expect(screen.getByTestId('visual-evidence-dimensions')).toHaveTextContent('1440×900');
56+
});
57+
58+
it('numbers captures in the switcher, marks their device and flags failed ones', () => {
59+
render(<TaskVisualPreviews previews={[
60+
{ type: 'image', title: 'Chart at desktop 1440px', url: asset('a') },
61+
{ type: 'image', title: 'Assertion failed at 390px', url: asset('b') },
62+
{ type: 'image', title: 'Checkout assertion passed', url: asset('c') },
63+
]} />);
64+
const switcher = screen.getByRole('group', { name: 'Captures' });
65+
expect(within(switcher).getAllByRole('button').map(button => button.textContent)).toEqual(['01Desktop 1440px', '02390px', '03Checkout assertion passed']);
66+
expect(within(switcher).getByRole('button', { name: '390px' })).toHaveClass('border-red-300', 'text-red-700', 'bg-red-50');
67+
expect(within(switcher).getByRole('button', { name: 'Desktop 1440px' })).not.toHaveClass('border-red-300');
68+
const passed = within(switcher).getByRole('button', { name: 'Checkout assertion passed' });
69+
expect(passed).not.toHaveClass('border-red-300', 'text-red-700', 'bg-red-50');
70+
expect(passed).toHaveClass('border-transparent', 'text-slate-600');
71+
});
72+
3573
it('switches captures in place and plays videos with their controls', () => {
3674
const { container } = render(<TaskVisualPreviews previews={[
3775
{ type: 'image', title: 'Before the fix', url: asset('before') },
3876
{ type: 'image', title: 'After the fix on mobile 390px', url: asset('after') },
3977
{ type: 'video', title: 'Walkthrough', url: asset('video') },
4078
]} />);
4179
const switcher = screen.getByRole('group', { name: 'Captures' });
42-
expect(within(switcher).getAllByRole('button').map(button => button.textContent)).toEqual(['Before', 'After', 'Capture 3']);
80+
expect(within(switcher).getAllByRole('button').map(button => button.textContent)).toEqual(['01Before', '02After', '03Walkthrough']);
4381
expect(within(switcher).getByRole('button', { name: 'Before' })).toHaveAttribute('aria-pressed', 'true');
4482

4583
fireEvent.click(within(switcher).getByRole('button', { name: 'After' }));
4684
expect(screen.getByAltText('After the fix on mobile 390px')).toBeInTheDocument();
4785
expect(screen.queryByAltText('Before the fix')).toBeNull();
4886
expect(screen.getByText('Mobile 390px')).toBeInTheDocument();
4987

50-
fireEvent.click(within(switcher).getByRole('button', { name: 'Capture 3' }));
88+
fireEvent.click(within(switcher).getByRole('button', { name: 'Walkthrough' }));
5189
const video = container.querySelector('video');
5290
expect(video).toHaveAttribute('src', asset('video'));
5391
expect(video).toHaveAttribute('controls');
5492
expect(screen.queryByRole('button', { name: 'Lightbox' })).toBeNull();
93+
expect(screen.queryByRole('group', { name: 'Zoom' })).toBeNull();
5594
});
5695

5796
it('opens images in an in-app lightbox, skips videos, and restores focus on close', () => {
@@ -60,7 +99,7 @@ describe('TaskVisualPreviews', () => {
6099
{ type: 'video', title: 'Walkthrough', url: asset('video') },
61100
{ type: 'image', title: 'Second', url: asset('second') },
62101
]} />);
63-
fireEvent.click(screen.getByRole('button', { name: 'Capture 3' }));
102+
fireEvent.click(screen.getByRole('button', { name: 'Second' }));
64103
const trigger = screen.getByRole('button', { name: 'Open full-size preview: Second' });
65104
expect(trigger).toHaveAttribute('aria-haspopup', 'dialog');
66105
trigger.focus();
@@ -76,7 +115,7 @@ describe('TaskVisualPreviews', () => {
76115
expect(screen.queryByRole('dialog')).toBeNull();
77116
expect(trigger).toHaveFocus();
78117
// The canvas follows the capture the lightbox was left on.
79-
expect(screen.getByRole('button', { name: 'Capture 1' })).toHaveAttribute('aria-pressed', 'true');
118+
expect(screen.getByRole('button', { name: 'First' })).toHaveAttribute('aria-pressed', 'true');
80119
});
81120

82121
it('opens the lightbox from the header button too', () => {

0 commit comments

Comments
 (0)