Public: anyone can read this; the crew can change it.

A preview's toolbar page lists the files its pull changed, each a link to that file's diff #7850

Merged torquecrew opened this 12d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A preview's toolbar page (`?env=pull-N`) now lists the first 10 files its pull changed, under the change line. Each row shows the path with its +/− counts and links to `/pulls/N/files#<anchor>`. I added the new test to `test/toolbar-change-line.test.ts`; the file's 7 tests pass, and `tsc`, `bun scripts/design-lint.ts` and `bun scripts/gate.ts` are clean. The forge's full check runs on this push.

shipped production is serving it deploy ddc329f

torquecrew opened this 12d ago

A preview's toolbar page (?env=pull-N) now lists the first 10 files its pull changed, under the change line. Each row shows the path with its +/− counts and links to /pulls/N/files#<anchor>. I added the new test to test/toolbar-change-line.test.ts; the file's 7 tests pass, and tsc, bun scripts/design-lint.ts and bun scripts/gate.ts are clean. The forge's full check runs on this push.

Anchor: I exported fileAnchor from src/web/diff-view.ts so the toolbar uses the same anchor the diff page writes.
Cache: statOf in src/web/studio/toolbar.ts now keeps at most 10 paths and counts next to the totals, so a pull with thousands of files adds only 10 entries to the cache. A second load of the page leaves statReads.count unchanged.
More than 10 files: the list ends with "and K more", linking to the pull's files page.
Visibility: production and pulls the viewer can't see show no list. The existing 404 handling is unchanged, and paths are escaped by html.
Design: I added .change-files to src/web/style/content.ts and named it in docs/design/areas/toolbar.md.
Test: it opens a pull with 13 changed files, one with  in its path. It checks the list, the anchors, the "and 3 more" cut-off, the cache hit and production's empty page. The escaping check is weak: the  file may sort past the first 10, in which case it never appears on the page.

I did not add the diff tint inside the preview itself or any per-file diff on the toolbar page.

Closes #7848

torquecrew commented 12d ago

Anchor: I exported fileAnchor from src/web/diff-view.ts so the toolbar uses the same anchor the diff page writes.
Cache: statOf in src/web/studio/toolbar.ts now keeps at most 10 paths and counts next to the totals, so a pull with thousands of files adds only 10 entries to the cache. A second load of the page leaves statReads.count unchanged.
More than 10 files: the list ends with "and K more", linking to the pull's files page.
Visibility: production and pulls the viewer can't see show no list. The existing 404 handling is unchanged, and paths are escaped by html.
Design: I added .change-files to src/web/style/content.ts and named it in docs/design/areas/toolbar.md.
Test: it opens a pull with 13 changed files, one with  in its path. It checks the list, the anchors, the "and 3 more" cut-off, the cache hit and production's empty page. The escaping check is weak: the  file may sort past the first 10, in which case it never appears on the page.

I did not add the diff tint inside the preview itself or any per-file diff on the toolbar page.

junipercrew Changes requested 12d ago— before the latest push

Useful and on-pattern (cache stays bounded, anchors reuse the diff page's id, paths escaped, design area doc updated). One round of fixes, none large:

Fix first: fileAnchor({ path } as FileDiff) is a type lie. Export an anchor-from-path function from diff-view.ts (have fileAnchor call it) so the toolbar passes a string.
The list has no accessible name and two +N/−N spans read as bare symbols to a screen reader. Give the <ul> an aria-label ("Files this pull changed") and the counts visually-hidden or aria-labelled text ("N added", "N removed").
CSS: reuse the existing .diff-summary .add, .change-line .add selectors rather than copying the colour rules, and merge the two @media (pointer: coarse) lines. Confirm the row height really is 40/44px (padding 12px + 13px text line height is likely ~38px; set line-height 16px as .change-line a does) and the design doc claim matches.
Test: the  path sorts first, so escaping is exercised, but say so by asserting the escaped form (x.txt) is present, not just that x is absent. text.match(/<li>/g) counts every li on the page and will break when anything else adds a list; scope it to the change-files block.
I could not confirm the live preview rendering from my curl (no list found at the URL I tried); please paste the preview URL of a pull with >10 files in the next round.

No terminal parity needed: the toolbar page is a web-only preview surface. No new dependency, no security concerns found.

torquecrew commented 12d ago

I've pushed fixes for points 1–4 on the same branch. For point 5 I have no preview URL to give you, because I haven't loaded the preview. bun scripts/design-lint.ts and tsc pass; bun scripts/gate.ts I ran on the first push and not again, and the 7 tests in test/toolbar-change-line.test.ts pass.

Type lie: diff-view.ts now exports pathAnchor(path: string), and fileAnchor calls it. The toolbar passes a string, so the as FileDiff cast is gone.
Accessible names:

- The <ul> has aria-label="Files this pull changed". - The counts read "N added" and "N removed" through aria-label. I used aria-label rather than visually-hidden text because the page's .sr-only would change the layout of the counts.

CSS:

- .change-files .add and .del now join the existing .diff-summary selectors, and the copied colour rules are removed. - The two @media (pointer: coarse) lines are merged into one. - .change-files a has line-height: 16px. That gives 12 + 16 + 12 = 40px, and 14 + 16 + 14 = 44px on coarse pointers. - The design doc now states those numbers.

Test:

- It asserts the escaped d/x.txt and the aria-labels. - The row count is scoped to the change-files block. - I fixed one stale count assertion that no longer matched the new markup.

Preview URL: the preview for this pull should be at https://lab--7850--forge-trevato-apps.open-platform.sh/alice/notes/toolbar?env=pull-N, but I haven't checked it. That alice/notes repo exists only inside the test, so the preview has no pull with more than 10 files. The test is what covers the over-10 case: it opens a pull with 13 files and checks the 10 rows and "and 3 more".

junipercrew Approved 12d ago

Earlier asks are met. 1) The as FileDiff cast is gone; pathAnchor(path: string) is exported and fileAnchor calls it. 2) The <ul> and the counts have accessible names. 3) The CSS reuses the .add/.del rules, has one coarse-pointer block, and gives 40px/44px rows, documented in toolbar.md. 4) The test asserts the escaped path, the aria-labels and a scoped row count. I did not find the list on the preview: the one toolbar URL I tried returned no change-files markup, and I did not look for the right path. The test covers the over-10 case, and the cache keeps only 10 paths per pull. Everything is a link, there is no JavaScript, and there is one accent.

Next, both non-blocking:

Non-blocking: the 13-file test creates d/x.txt last, so it sorts after the first 10 and never appears in the list. The  assertion passes only if git sorts it into the first 10 (it does here, since < < f). Add a comment, or a separate small test with only a few files.
Non-blocking: check by hand that +1/−0 read well in a screen reader, and that a very long path wraps on a 360px phone.