Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
|
abb33e6 to
bdf03d7
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the page is well scoped (Sandbox-only, deterministic workloads, the route exports cleanly and the tests pass), but three readings can mislead the tuning work it exists for. Please address these before merging:
- “First file on screen” is read in a frame callback before the paint, while “Full render” is read after it, so with the current
DiffViewerboth describe the same paint; take the first-file reading after the paint too, and record a late detection instead of falling back to the render time. - The Run metrics badges follow the live selection, so picking another workload after a run relabels its numbers; store the label, line total, view and syntax with the run, and keep Run benchmark and the other controls disabled during the scroll test.
- Unavailable readings show as zero: long tasks read “0 / 0.0 ms” in Firefox, which has no Long Tasks API, and heap growth reads “0.0 MB” where the heap reading is coarse (headless Chromium); show “—” in both cases.
[Automated review]
| observer = new PerformanceObserver(list => { | ||
| entries.push(...list.getEntries()); | ||
| }); | ||
| observer.observe({type: 'longtask', buffered: false}); |
There was a problem hiding this comment.
Observing an unsupported entry type does not throw (Firefox only logs “Ignoring unsupported entryTypes: longtask.”), so the catch never runs and the card reports “0 / 0.0 ms” for a multi-second synchronous render. Check PerformanceObserver.supportedEntryTypes?.includes('longtask') and return null so the card shows “—”.
| } | ||
| requestAnimationFrame(watchFirstFile); | ||
| }; | ||
| requestAnimationFrame(watchFirstFile); |
There was a problem hiding this comment.
This frame callback reads the first file before that frame’s style, layout and paint, while Full render (L236) reads after the same paint; because the whole list commits in one flushSync, both describe one paint. Take this reading at the after-paint point too (a setTimeout(0) from the callback that finds the table), and update run.firstFileMs when detection lands late instead of falling back to renderMs (L241).
| heapBytes: | ||
| heapBefore == null || heapAfter == null | ||
| ? null | ||
| : heapAfter - heapBefore, |
There was a problem hiding this comment.
Where the heap reading is coarse (headless Chromium gave identical readings before and after a ~200 MB allocation), this subtraction yields 0 and the card shows “0.0 MB”; treat an unchanged reading as unavailable and say so in the caption.
| const changeSyntax = (next: string) => | ||
| timeInteraction(`Syntax ${next}`, () => setSyntax(next as Syntax)); | ||
|
|
||
| const runScrollTest = () => { |
There was a problem hiding this comment.
The scroll test never sets isBusy, so Run benchmark, the selectors and Collapse all / Expand all stay enabled for its four seconds, and a run started meanwhile receives this loop’s stale result from setScroll (L321). Set isBusy for the duration of the test.
| <VStack gap={3}> | ||
| <Heading level={3}>Run metrics</Heading> | ||
| <HStack gap={2} wrap="wrap"> | ||
| <Badge label={workload.label} /> |
There was a problem hiding this comment.
These badges follow the live selection while the metrics below belong to the last run: choosing “1,000” after a 200-file run labels that run “1,000 files / 152,827 file lines”. Store the label, line total, view and syntax with the run (or reset run when the selection changes).
Renders a whole change set, one Lab DiffViewer per file, from deterministic workloads (50/200/500/1,000 files at 10k-150k lines, mostly small files plus a few very large ones; single files of 100-20k lines, sparse or rewritten) and measures the diff model, first file on screen, full render, long tasks, DOM size, heap growth, scroll frames, and collapse/expand/view switches.
3d6d5b6 to
06b36a6
Compare
bdf03d7 to
ccc71ce
Compare
There was a problem hiding this comment.
Requesting changes: this revision only restacks the branch, so the three measurement problems raised on the previous revision still apply; otherwise the page is well scoped, and its route and tests are in good shape.
- Take "First file on screen" after the paint, as "Full render" does, and keep a late detection instead of falling back to the render time.
- Store the workload label, line total, view and syntax with each run, and keep the controls disabled while the scroll test runs.
- Show "—" instead of zero when the browser has no Long Tasks API (Firefox) or the heap reading does not change (for example, headless Chromium).
[Automated review]
…ch run's labels, and shows no reading as a dash - First file on screen is now read after the paint that shows it, like full render, and the run waits for it rather than falling back to the render time when it lands late. - Each run stores its workload label, line total, view and syntax, so changing a control afterwards no longer relabels its numbers; every control stays disabled while the scroll test runs. - Long tasks show a dash where the browser has no Long Tasks API, and heap growth shows a dash when the heap reading does not move.
There was a problem hiding this comment.
Requesting changes: this revision fixes all three measurement problems raised earlier (first file read after the paint, per-run labels with the controls locked during the scroll test, and dashes for unavailable readings), and the route, build and tests are in good shape. One labeling issue remains: the badges keep the run's view and syntax, but switching either re-renders the list without a new run, so a later scroll test or collapse/expand timing appears under the old labels (a split scroll result shows as "unified"). Store the view and syntax with each scroll reading and timed interaction, or clear the scroll reading on a switch, so every number names what it measured.
[Automated review]
| return; | ||
| } | ||
| const elapsed = now - start; | ||
| setScroll({ |
There was a problem hiding this comment.
This reading measures whatever view and syntax are on screen, but the card labels it with the run's (shown, L393–398). After a unified run, switching to split and running this test showed 17 / 53 / 28 (unified: 40 / 17 / 1) under the "unified" badge, and Collapse all / Expand all timings after the switch have the same problem. Capture viewMode and syntax when the test (or a timed interaction) starts and show them with the reading, or clear scroll when the view or syntax changes.
… reading with the view it measured A view or syntax switch re-renders the list without a new run, so a later scroll test or collapse/expand timing appeared under the run's view. Each scroll reading and timed interaction now stores the view and syntax it was taken in and shows them in its label (a switch is labelled with the view it switched to).
There was a problem hiding this comment.
Requesting changes: this revision fixes the earlier labeling issue (every scroll and interaction reading now names the view and syntax it measured), and the route, build and tests are in good shape. One small labeling problem is left: the "diff rows rendered" badge sums diff lines, but split view puts a removed line and its added line in one row, so a split run of the 5,000-line full rewrite shows 10,000 rows while 5,002 are rendered. Label the badge with what it counts (for example "diff lines") or count rows for the run's view.
[Automated review]
| label={`${shown.totalLines.toLocaleString()} file lines`} | ||
| /> | ||
| <Badge | ||
| label={`${diffLines.toLocaleString()} diff rows rendered`} |
There was a problem hiding this comment.
This sums hunk lines (renderedLineCount), but DiffViewer adds a header row per hunk and, in split view, puts a removed line and its added line in one row: a split run of the one-file 5,000-line full rewrite shows "10,000 diff rows rendered" while 5,002 rows are rendered. Label it with what it counts, or count rows for the run's view.
<Badge label={`${diffLines.toLocaleString()} diff lines`} />
There was a problem hiding this comment.
Requesting changes: this revision only merges the updated base, so the one labeling problem raised on the previous revision is still open; the route, static export, tests and lint are otherwise in good shape. The "diff rows rendered" badge sums diff lines, but split view puts a removed line and its added line in one row, so a split run of the 5,000-line full rewrite still shows "10,000 diff rows rendered" while its table renders 5,000 line rows. Label the badge with what it counts (for example "diff lines") or count rows for the run's view.
[Automated review]
The badge summed hunk lines but called them rendered rows; split view puts a removed line and its added line in one row and adds hunk header rows, so a split full rewrite of 5,000 lines read "10,000 diff rows rendered". It now says "diff lines", which is what it counts.
There was a problem hiding this comment.
Recommending approval: the badge now reads "diff lines", which is what it counts, and the route, static export, workload tests and lint are in good shape on this revision. Optionally, refresh the description's first-file / full-render row (4.9 s / 5.7 s), which predates reading first file after the paint; both readings now come from one paint. This can merge once #7568, #7536 and #7565 have landed.
[Automated review]
There was a problem hiding this comment.
Recommending approval: this revision only merges the updated base, and the page, its route and static export, the workload tests and lint all check out on it. Optionally, refresh the description's evidence table, which still shows the earlier first-file / full-render split (4.9 s / 5.7 s) and "diff rows" where the page now reports "diff lines". This can merge once #7536 and #7565 have landed.
[Automated review]
The page had a route but no entry in the sandbox page registry, so it did not appear on the tools page. Adds it next to CodeBlock Perf and Markdown Perf.
This branch was successfully deployed
1 participant
Maintainer impact
The Lab DiffViewer has no way to measure how it behaves on the change sets people actually review: many files and a lot of code. Anyone tuning it (virtualization, lazy file bodies, highlighting) needs one runnable page that reproduces those workloads and reports the same numbers every time.
Intended invariant
Sandbox-only. No package API, component behavior or visual change. Modeled on the existing CodeBlock and Markdown performance pages.
Change
apps/sandbox/.../pages/diffviewer-perf: renders a whole change set, oneDiffViewerper file, inside a fixed-height scrollable surface.benchmark.ts, seeded, byte-identical every run):createDiffFile) time, first file on screen, full render to the next painted frame, long tasks (count and total), DOM nodes, heap growth (where the browser reports it);Stacked on #7565 (createDiffFile); #7568 (the parser) and #7536 (the viewer) are on main. Merge order: #7565, then this.
Evidence
Production Sandbox build of this branch (
pnpm build, static export verified), headless Chrome 155, 200 files (50,827 file lines, 16,465 diff rows), unified, highlighted:createDiffFile)Every control and button was driven in the browser; no console errors. In a bare page with no app shell, the same Lab build renders 50 / 200 / 500 / 1,000 files in 1.3 / 3.1 / 5.2 / 8.3 s, each as a single long task. Profiling the 200-file case puts most of that time in per-file scroll-area measurement (forced style and layout) inside the one commit. The diff model takes about 80 ms and tokenizing about 70 ms. That is the baseline this page exists to move.
Scope
Testing
vitest run apps/sandbox/.../diffviewer-perf: 7 passed (deterministic workloads, size targets within 5%, small/large mix, added/deleted/rewritten files)scripts/next-export-routes.json)