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📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsDiffViewer (@astryxdesign/lab) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: createDiffFile throws RangeError: Maximum call stack size exceeded on long texts with only a few distant edits, because ops.push(...middle) in createDiffFile.ts passes one argument per diff op (about 130,000 lines with the first and last line changed reproduces it in V8, while a full rewrite of the same size works). Please append those ops with a loop and add a regression test with a large input changed at both ends; the Lab-only placement and the rest of the change look good. The Files tab currently shows #7536's annotation-key and spaced-path fixes instead of createDiffFile only because this comparison's base predates the split on the base branch.
[Automated review]
The structured model a diff viewer renders (files, hunks, numbered lines) and a tolerant parser for git and diff -u patches: multi-file patches, mode, rename and binary lines, bare hunks, CRLF and no-newline markers. Paths with spaces are read from the git header by its middle space, and git-quoted paths are decoded (octal UTF-8 bytes and C escapes). It never throws. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
Renders one DiffViewerFile: a file header (path, Added/Deleted/Renamed token, +/- counts, a headerEnd slot, an optional disclosure) over its hunks in unified or split view, with old/new line numbers, wrapping, maxHeight and a per-instance syntax theme. Rows paint from the same syntax-theme diff slots as CodeBlock. renderLineAnnotation adds a full-width row under any line, and onLineNumberClick makes each line's anchor-side number a button labelled with the line it reports; each is its own Tab stop (AST-020 FR15). Changed lines carry a visually hidden Added/Removed label, and hidden column headers name every column. Adds tests, docs, a docsite showcase block, Storybook stories, the registry route lock entries, and the @astryx.diffViewer.* en keys. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
…of a text For callers that hold file contents rather than a patch: a line-level Myers diff with configurable context, hunk splitting, and added/deleted/renamed status from null or differing paths. Shared leading and trailing lines are matched without a search, and the search stops at 2,000 edits (a larger change shows as one replaced block), so memory stays bounded for any input. Adds its tests, a Storybook story, and a doc example. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
9407bc6 to
939219d
Compare
3d6d5b6 to
06b36a6
Compare
There was a problem hiding this comment.
Changes requested: createDiffFile still throws RangeError: Maximum call stack size exceeded on long texts with only a few far-apart edits, because ops.push(...middle) in createDiffFile.ts passes one call argument per diff op (126,000 lines with the first and last line changed reproduces it in V8, while a full rewrite of the same size works). Please append those ops with a loop and add a regression test with a large input changed at both ends; the Lab-only placement, the docs and the existing tests otherwise look good. While you are there, a contextLines of NaN currently returns empty hunks and could fall back to the default.
[Automated review]
| } | ||
| const middle = myers(a, b, head, tailA, tailB); | ||
| if (middle != null) { | ||
| ops.push(...middle); |
There was a problem hiding this comment.
This spread passes one call argument per op, so a long middle with few edits throws RangeError: Maximum call stack size exceeded (in V8, 126,000 lines with the first and last line changed). Appending in a loop fixes it:
for (const op of middle) {
ops.push(op);
}A regression test with a text of more than 126,000 lines changed at both ends would keep it fixed.
| }: CreateDiffFileOptions): DiffViewerFile { | ||
| const oldPath = oldPathOption === undefined ? (path ?? null) : oldPathOption; | ||
| const newPath = newPathOption === undefined ? (path ?? null) : newPathOption; | ||
| const context = Math.max(0, Math.floor(contextLines)); |
There was a problem hiding this comment.
Minor: NaN passes through Math.max(0, Math.floor(contextLines)), and createDiffFile({oldValue: 'a\nb\nc', newValue: 'a\nB\nc', contextLines: NaN}) returns two empty @@ -0,0 +0,0 @@ hunks. Falling back to the default 3 for NaN would avoid that.
…lines, and reads copy lines Two different paths with no rename lines (diff -u a.orig a, diff -ruN old new, git diff --no-index, a copy) were reported as renames. They now read as modified with both paths kept. Copy from/copy to lines set the paths, so a copy whose paths contain spaces no longer takes the header's first-space split.
…n returns false `hasThread && <Thread />` returns false for most lines; React renders nothing for it, but the viewer still drew an empty bordered row under each of those lines. Booleans now count as nothing to show, like null.
…ontext falls back to the default ops.push(...middle) passed one call argument per op, so a long text with only a few distant edits threw RangeError: Maximum call stack size exceeded. A contextLines of NaN returned empty hunks; it now uses the default of 3.
There was a problem hiding this comment.
Approved: both earlier requests are addressed. The edit script is now appended in a loop, so long texts with a few far-apart edits return instead of overflowing the stack, and a NaN contextLines falls back to 3; each fix has a regression test that fails without it. This can merge after #7568 and #7536 as planned. Optionally, mention in createDiffFile's JSDoc that past 2,000 changed lines the changed span is shown as one removed-then-added block, so callers can plan for very large diffs.
[Automated review]
…its own file GNU diff -r reports a binary file with one `Binary files … differ` line and no header, so it was attached to the file before it (marking that text file binary) and the binary file was dropped; a binary file listed first absorbed the next file's ---/+++ pair. Such a line now starts its own file unless it belongs to a diff --git header still being read, and a binary entry never takes a ---/+++ pair. The copy test now uses exact copies (no ---/+++ lines) so it depends on the copy lines, including a quoted one.
There was a problem hiding this comment.
Approved: createDiffFile produced minimal, exactly round-tripping diffs with diff -u hunks across thousands of random text pairs, and this merge-only update leaves it unchanged, so it can merge after #7568 and #7536 as planned. Two optional follow-ups:
- Hunk headers recount every line before each hunk, so 1,999 hunks in a 1,000,000-line text take about 4.6 s; carrying running counts across hunks gives identical output about ten times faster.
- The function's JSDoc could say that past 2,000 changed lines the changed span shows as one removed-then-added block, that line-ending and final-newline differences are ignored, and that both paths
nullyields an added file.
[Automated review]
| // Lines of each side that come before this hunk. | ||
| let oldBefore = 0; | ||
| let newBefore = 0; | ||
| for (let i = 0; i < start; i++) { |
There was a problem hiding this comment.
Optional: this recounts every op before each hunk, so numbering costs hunks × lines; 1,999 hunks in a 1,000,000-line text take about 4.6 s. Hunks are disjoint and in order, so running totals give identical output in about 0.46 s there:
// before the hunk loop
let scanned = 0;
let oldBefore = 0;
let newBefore = 0;
// per hunk, instead of recounting from 0
for (; scanned < start; scanned++) {
if (ops[scanned].kind !== 'added') oldBefore++;
if (ops[scanned].kind !== 'removed') newBefore++;
}|
|
||
| /** | ||
| * Compute a {@link DiffViewerFile} from two versions of a text, for callers | ||
| * that hold file contents rather than a patch. |
There was a problem hiding this comment.
Optional: callers can't see three behaviours from here or from DiffViewer.doc.mjs: past 2,000 changed lines the span between the first and last change is shown as removed then re-added; CRLF/LF and final-newline differences are ignored, so a line-ending-only edit shows “No content changes”; and when both oldPath and newPath are null the file is added. A sentence each would let callers plan for them.
…ne regex The regex ran on every line outside a hunk, and on a long line that starts with `Binary files ` but doesn't end in ` differ` (a crafted commit-message line in format-patch output) its two greedy groups backtrack quadratically: about 4 s for a 200 KB line. Checking the prefix and suffix first keeps that line linear; a regression test bounds it.
There was a problem hiding this comment.
Approved: this merge-only update leaves createDiffFile unchanged, all 47 DiffViewer tests pass, and randomised checks found minimal, exactly round-tripping diffs whose hunks GNU patch applies cleanly, so it can merge after #7568 and #7536 as planned. One optional follow-up: the 2,000-edit cap counts removed plus added lines, so editing 1,001 lines in place already shows the whole changed span as one removed-then-added block, and line-ending differences are ignored; a sentence in createDiffFile's JSDoc and the doc example would let callers plan for both.
[Automated review]
| * it keeps grows with the square of this, so the cap bounds memory to a few | ||
| * megabytes; past it the changed middle is shown as one replaced block. | ||
| */ | ||
| const MAX_EDIT_DISTANCE = 2000; |
There was a problem hiding this comment.
Optional: this cap counts removed plus added lines, so editing 1,001 lines in place (2,002 edits) already takes the one-block path. With those edits spread through a 10,000-line file, the viewer gets 9,001 removed then 9,001 added rows instead of 1,001 paired changes. A sentence in createDiffFile's JSDoc and the doc example would let callers plan for it.
The shape guard still let a backtracking case through: a line holding a lone CR, U+2028 or U+2029 before the closing ` differ` (`.` matches none of them), about 2.5 s for a 200 KB crafted line. The regex is replaced with a prefix/suffix check and a split at the last ` and `, which reads any line in one pass with the same result as the greedy pattern. Regression cases cover no closing ` differ`, a CR and a U+2028 before it.
There was a problem hiding this comment.
Approved: createDiffFile is unchanged by this merge-only update, all 49 DiffViewer tests pass, and randomised checks found minimal, exactly round-tripping diffs that GNU patch applies cleanly, so it can merge after #7568 and #7536 as planned. One optional follow-up is worth weighing: past 2,000 changed lines the whole changed span is shown as removed and re-added (two real revisions of this repository's pnpm-lock.yaml, about 2,700 changed lines apart, render as the entire file replaced), and neither that nor the ignored line-ending differences is mentioned in createDiffFile's JSDoc or the doc example.
[Automated review]
# Conflicts: # packages/lab/src/DiffViewer/diffTypes.ts # packages/lab/src/DiffViewer/index.ts # packages/lab/src/DiffViewer/parseUnifiedDiff.ts # packages/lab/src/index.ts
… pages On a right-to-left page the hunk table mirrored, so a line wider than the viewer opened scrolled to the lines' ends: every line's start was cut off and short lines looked blank. The hunk area now reads left-to-right in every page direction, as code editors do (line numbers on the left, old file on the left in split view), while the file header and annotation rows (the caller's comments) follow the page. Adds a RightToLeft story with lines wider than the viewer and an RTL annotation.
# Conflicts: # packages/lab/src/DiffViewer/DiffViewer.doc.mjs
There was a problem hiding this comment.
Approved: this merge-only update leaves createDiffFile unchanged, all 49 DiffViewer tests pass, and randomised checks found minimal, exactly round-tripping diffs that GNU patch applies cleanly, so it can merge after #7536 as planned. One optional follow-up: raising the 2,000-edit cap to 100,000 still passes every test, because the full-rewrite test gets the same block either way, so a case such as 1,001 in-place edits spread through 10,000 lines (one replaced block, where 1,000 edits give 1,000 hunks) would pin the cap.
[Automated review]
| ]); | ||
| }); | ||
|
|
||
| it('shows a rewrite too large to search as one replaced block', () => { |
There was a problem hiding this comment.
Optional: this test also passes with MAX_EDIT_DISTANCE raised to 100,000, because Myers gives a full rewrite the same all-removed-then-all-added block as the fallback, only more slowly (about 1.3 s instead of 0.07 s here). A case whose minimal and capped results differ would pin the cap, for example:
const base = Array.from({length: 10_000}, (_, i) => `line ${i}`);
const edit = (count: number) => {
const next = [...base];
const step = Math.floor(base.length / (count + 1));
for (let e = 1; e <= count; e++) next[e * step] = `changed ${e * step}`;
return createDiffFile({oldValue: base.join('\n'), newValue: next.join('\n')});
};
expect(edit(1000).hunks).toHaveLength(1000); // 2,000 edits: minimal diff
expect(edit(1001).hunks).toHaveLength(1); // 2,002 edits: one replaced block
<!-- Use this template only when adding a new user-facing capability is the primary intent. --> ## User need Builders of code review and change-inspection tools (PR review pages, deploy diffs, config change audits, agent "proposed edits" panels) need to show a file's changes the way reviewers expect to read them: per-file header with path and +/− counts, old/new line numbers, unified or side-by-side layout, and comment threads anchored under specific lines. `CodeBlock` with `isDiff` (#7389) answers the inline case — a diff fence in prose or chat — but by design it is a single-column code surface over the patch text: one line-number column, no file header, no old/new numbering, no split view, and no way to put content between lines. Composing those on top of `CodeBlock` means re-implementing the row model, numbering and pairing in every product, which is the work a design system should own once. Lab is the staging area for this: canary-only, no stability promise, iterate freely before a Core spec. ## Problem and solution fit - **Primary:** `DiffViewer` in `@astryxdesign/lab` — renders one `DiffViewerFile`: - file header: path (`old → new` on rename), Added/Deleted/Renamed `Token` (a category of change, not an alert — so not a status `Badge`), +N −M counts, a `headerEnd` slot, and an optional disclosure (controlled or uncontrolled); - hunks in `viewMode="unified"` or `"split"`; split pairs each run of removed lines with the run of added lines that replaced it, row by row; - old/new line-number gutters (`hasLineNumbers`), wrapping (`isWrapped`), `maxHeight`, per-instance `syntaxTheme`; - `renderLineAnnotation(ref)` — a full-width row under any line (review comments); in split view the row sits under the line's own side; - `onLineNumberClick(ref, event)` — turns line numbers into buttons (e.g. "start a comment here"). Only the line's anchor-side number is a button, and each is its own Tab stop (AST-020 FR15: independent controls get no arrow-key movement). - **Already on main:** the data model and `parseUnifiedDiff` landed in #7568. The two-text builder `createDiffFile` follows in #7565. - **Supporting:** colocated tests, component doc, a runnable docsite showcase block (`packages/lab/blocks/DiffViewerShowcase`), Storybook `Lab/DiffViewer`, the registry route lock entries, `@astryx.diffViewer.*` en catalog keys (+ core changeset for the catalog). Paint reuses CodeBlock's diff work rather than inventing a parallel palette: rows read the five syntax-theme diff slots (`--color-syntax-added`, `-removed`, `-diff-header`, `-added-marker`, `-removed-marker`) and tokens come from the shared tokenizer/`astryx-token-*` classes, so one syntax theme styles both components. ## Current specification None for Lab (by design). `component:CodeBlock` DEC-1 rejects "a separate Diff component" for **diff fences** (a fence is a CodeBlock); this component is a different fact boundary — a file-level review surface with old/new numbering, split layout, file header, and between-line content — and does not change how CodeBlock or Markdown render fences. A Core spec would come with promotion. ## Public delta and caller burden New Lab-only exports; no Core component API changes. Core gains only new `en` catalog keys (no existing string changes). ```tsx import {DiffViewer, parseUnifiedDiff} from '@astryxdesign/lab'; // one file const [file] = parseUnifiedDiff(patch); <DiffViewer file={file} viewMode="split" /> // a whole patch: one viewer per file, caller owns the list layout parseUnifiedDiff(patch).map(file => ( <DiffViewer key={file.newPath ?? file.oldPath} file={file} /> )); // review comments <DiffViewer file={file} onLineNumberClick={ref => startComment(ref.side, ref.lineNumber)} renderLineAnnotation={ref => threads.get(`${ref.side}:${ref.lineNumber}`) ?? null} /> ``` Defaults: `viewMode='unified'`, `hasLineNumbers=true`, `isWrapped=false`, `isCollapsible=true` (needs a path to name the control), `defaultIsCollapsed=false`; language inferred from the file extension, else plain text. Line anchors: every line has exactly one `DiffViewerLineRef` (`side` + `lineNumber`), identical in unified and split view — added → `new`, removed → `old`, context → `new` — so a comment anchor never moves when the reader switches layouts. Theming targets: `astryx-diff-viewer` (reflects `viewMode`), `astryx-diff-viewer-header`, `astryx-diff-viewer-path`. ## Design and alternatives - **One file per component, caller maps the list** — chosen so file-list concerns (file tree, "viewed" state, virtualization, ordering) stay with the product instead of growing props on the viewer. Rejected: a multi-file `DiffViewer` that owns the list. - **One structured input (`file`) plus builders** — rejected: parallel `diff` string / `files` / `oldValue`+`newValue` props (conditional overrides between inputs). - **Table rows** (`<table>` with per-side gutter/marker/code cells) — gives split view, between-line annotation rows (`colSpan`) and wrapped rows that keep gutters aligned. Rejected: extending CodeBlock's per-line grid (it has no second number column or between-line rows). - **Change semantics without colour:** each changed line keeps a visible `+`/`-` glyph (hidden from AT) plus a visually hidden, localized "Added"/"Removed" label; header counts are announced as "N lines added, M lines removed". - **Highlighting per hunk side:** each hunk's old and new text are tokenized as continuous text so multi-line constructs keep their paint within a hunk. - **Not in this PR (follow-ups):** expanding collapsed context between hunks (needs file contents), word-level intra-line highlights, line-range selection/highlight, virtualization for very large files. ## Evidence - Vitest — `packages/lab/src/DiffViewer`: 20 component tests (unified/split row model, gutters off, collapse controlled + uncontrolled with `aria-expanded`/`aria-controls`, annotations in both views, line-number buttons on the anchor side only, each labelled with the line it reports, every line number its own Tab stop, named columns, badges/rename path, binary/no-change messages, `headerEnd`, ref/className/DOM passthrough + `data-view-mode`). - Real-browser screenshots (Chromium, built Storybook) below: unified, split light/dark, wrapped, multi-file patch, review comments. - Right-to-left: the hunk area reads left-to-right in every page direction, as code editors do (line numbers on the left, old file on the left in split view), so a line wider than the viewer opens at its start. In Chromium at 1280 px, the `Split` story under RTL cut off the start of 23 of 44 code lines before this and none after. The file header and comment text follow the page. Comment rows are deliberately simple in this Lab version: they belong to the code layout, take the full width of their side, and scroll with the code, so on a wide right-to-left diff a comment may need a horizontal scroll to bring into view (the `RightToLeft` story shows this). <details><summary>Unified (default)</summary>  </details> <details><summary>Split, light</summary>  </details> <details><summary>Split, dark</summary>  </details> <details><summary>Split, wrapped (dark)</summary>  </details> <details><summary>Multi-file patch: modified, added, renamed, binary</summary>  </details> <details><summary>Review comments in split view (annotation rows on their own side)</summary>  </details> ## Scope - [x] One capability is added. - [x] Unrelated fixes, cleanup, and policy changes were removed or split. - [x] Consumer docs, focused tests, and migration evidence ship with the feature. - [x] Public text and artifacts contain no internal Meta context. ## Testing ``` pnpm vitest run packages/lab/src/DiffViewer # 39 passed (20 viewer + 19 parser from #7568) pnpm -F @astryxdesign/docsite build # ok (incl. ShadCN route lock check) pnpm lint:strict # 0 errors pnpm build # ok pnpm test # 23574 passed; 18 failed in 4 files this PR doesn't touch: # .github/scripts/{release-check,visual-maintenance}-workflow.test.mjs, # apps/storybook/.storybook/story-tree.test.ts (A11y/ and a11y/ story titles), # internal/vibe-tests/setup-test/setup-workspace.test.mjs — local-environment / pre-existing ``` Storybook: `Lab/DiffViewer` — Default, Split, Wrapped, Right To Left, Multi File Patch, Viewed Toggle, Review Comments, Long Comment, Compact.
# Conflicts: # apps/storybook/stories/DiffViewer.stories.tsx # packages/lab/src/DiffViewer/DiffViewer.doc.mjs # packages/lab/src/DiffViewer/DiffViewer.tsx # packages/lab/src/DiffViewer/index.ts # packages/lab/src/index.ts
This branch was successfully deployed
2 participants
User need
Some callers of the Lab
DiffViewerhold two versions of a file, not a patch: an editor's saved and unsaved text, an agent's proposed edit, two config revisions. Today they would have to produce a unified diff themselves beforeparseUnifiedDiffcan read it.Problem and solution fit
createDiffFile({oldValue, newValue, path | oldPath/newPath, contextLines})in@astryxdesign/labreturns the sameDiffViewerFilethatparseUnifiedDiffproduces.diff -udoes.nullor differing path.Lab/DiffViewer→ From Two Texts) and a doc example.Split out of #7536 at review so the viewer and this utility can be reviewed separately. Originally written by @latxh as part of that PR.
Current specification
None for Lab (by design). No Core change.
Public delta and caller burden
New Lab-only export
createDiffFileand typeCreateDiffFileOptions.Design and alternatives
oldValue/newValueprops onDiffViewer(two inputs with override rules). One model type, two builders.Evidence
createDiffFile.test.tscovers identical text, a minimal edit with numbering and header, a round trip of both texts, hunk splitting, added/deleted/renamed status, a minimal diff between long shared runs, the bounded fallback round trip, a 150,000-line text changed at both ends, andNaNcontext.Scope
Testing
#7568 and #7536 are on main; this PR targets main.