Skip to content

feat(lab): DiffViewer — unified/split file diff for code review - #7536

Open
latxh wants to merge 18 commits into
mainfrom
feat/lab-diff-viewer
Open

latxh wants to merge 18 commits into
mainfrom
feat/lab-diff-viewer

Conversation

@latxh

@latxh latxh commented Oct 14, 2026 •

Copy link
Copy Markdown

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 feat(lab): parseUnifiedDiff reads a patch into the DiffViewer file model #7568. The two-text builder createDiffFile follows in feat(lab): createDiffFile builds a DiffViewer file from two versions of a text #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).

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); the file header and annotation rows follow the page. 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 RightToLeft story (lines wider than the viewer, an RTL annotation) opens with every visible line at its start.
Unified (default)

Unified (default)

Split, light

Split, light

Split, dark

Split, dark

Split, wrapped (dark)

Split, wrapped (dark)

Multi-file patch: modified, added, renamed, binary

Multi-file patch: modified, added, renamed, binary

Review comments in split view (annotation rows on their own side)

Review comments in split view (annotation rows on their own side)

Scope

  • One capability is added.
  • Unrelated fixes, cleanup, and policy changes were removed or split.
  • Consumer docs, focused tests, and migration evidence ship with the feature.
  • 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, Compact.

@vercel

vercel Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Oct 15, 2026 4:01pm UTC

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 14, 2026
@astracat-bot

astracat-bot Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

AI review status for this pull request.

Review status Updated
🔄 In progress (for maintainers only) Oct 15, 2026, 3:55 PM UTC

@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Oct 14, 2026
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR

🧪 Sandbox Preview

View Sandbox for this PR

Modified Components

DiffViewer (@astryxdesign/lab) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 1385 -
Complexity N/A Very High (135) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/lab N/A 3.9KB 1.6KB

Accessibility Audit

Status: No accessibility violations detected.

Visual Regression

38 of 474 shot(s) changed.

A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.

component story theme mode pixels
Selector Indicator Space Bottom Sheet Narrow End RTL neutral light 754,077
Selector Indicator Space Bottom Sheet Wide Start neutral light 740,188
Chat Full AI Chat probe light 238,087
Chat Full AI Chat probe dark 237,060
Collapsible Dividers probe dark 66,445
Collapsible Dividers probe light 66,445
Chat Default neutral dark 33,205
Chat Default neutral light 32,876
Chat Empty State probe light 23,531
Chat Empty State probe dark 22,622
ChatAutoScroll Scroll Behavior Comparison neutral dark 15,378
ChatAutoScroll Scroll Behavior Comparison neutral light 14,107
Chat With Attachments probe light 13,406
Chat With Attachments probe dark 11,232
ProgressBar With Target Mark probe dark 7,895
ProgressBar With Target Mark probe light 7,895
TableRowExpansion Detail Panel neutral dark 5,150
TableRowExpansion Detail Panel neutral light 4,068
SideNav Resizable in AppShell probe light 3,846
TableGroupedRows Default neutral light 2,133

and 18 more.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough Lab DiffViewer; changes are requested before it can merge: two behavior fixes and a scope split.

  • Line-number keyboard handling is attached to the whole table, so ArrowUp/ArrowDown/Home/End pressed inside an annotation row (for example a comment textarea) move focus to a line number; please handle those keys only when a line-number button has focus, and add a test.
  • parseUnifiedDiff splits the diff --git header at the first space, so a binary or mode-only change to a path containing a space shows the wrong path and a false Renamed badge.
  • At about 3,100 authored lines, please move the standalone createDiffFile utility (with its tests and story) to a follow-up and split further, or explain why the remaining pieces must land together.

[Automated review]

Comment on lines +1020 to +1021
onKeyDown:
onLineNumberClick != null ? handleLineButtonsKeyDown : undefined,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This key handler is on the wrapper around the whole table, annotation rows included, so ArrowUp/ArrowDown/Home/End pressed inside renderLineAnnotation content (a comment textarea, a Resolve button) move focus to a line-number button and cancel the caret move. Consider delegating only when the event comes from a line-number button, for example if ((event.target as Element).closest('[data-diff-line-button]')) handleLineButtonsKeyDown(event);, with a test that types in an annotation textarea.

} from './diffTypes';

const HUNK_HEADER = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/;
const GIT_HEADER = /^diff --git (?:"?a\/)?(.+?)"? (?:"?b\/)?(.+?)"?$/;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lazy groups split the header at the first space: diff --git a/assets/my logo.png b/assets/my logo.png gives assets/my and logo.png b/assets/my logo.png. For binary or mode-only changes nothing overrides these paths, so finalize reports the file as renamed. Splitting an unquoted header at the midpoint when both halves match (or reading the Binary files ... differ line) would fix it, and a test with a spaced path would cover it.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes: annotation-row keys and paths with spaces now behave, but changes are still requested before this Lab DiffViewer can merge.

  • The clickable line numbers share one roving Tab stop with ArrowUp/ArrowDown/Home/End, while the accessibility spec (AST-020 FR15) keeps independent controls like these in the page Tab order unless the component's record names an exception, so please give each line number its own Tab stop or have a maintainer record that exception.
  • At about 2,750 authored lines, please stack parseUnifiedDiff (with its tests, stories and showcase) as the next layer, or explain concretely why it must land with the viewer.
  • parseUnifiedDiff keeps git's escapes in quoted non-ASCII paths (for example caf\303\251.md), so please decode quoted paths and add a test.

[Automated review]

itemSelector: '[data-diff-line-button]',
orientation: 'vertical',
wrap: false,
hasRovingTabIndex: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives the clickable line numbers one roving Tab stop with ArrowUp/ArrowDown/Home/End. AST-020 FR15 keeps a set of independently focusable controls in the page Tab order, without arrow keys, unless the component's record names an exception, and these buttons are independent: each starts its own line's action and nothing around them is a composite widget. Either drop the roving stop so each line number keeps its own Tab stop, or have a maintainer record the gutter as a named FR15 exception in a DiffViewer record.

function cleanPath(raw: string, prefix: 'a/' | 'b/'): string | null {
let path = raw.split('\t')[0].trim();
if (path.startsWith('"') && path.endsWith('"') && path.length >= 2) {
path = path.slice(1, -1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Git quotes non-ASCII paths by default ("a/docs/caf\303\251.md"), and only the quotes are removed here, so the header shows docs/caf\303\251.md. Decoding the C-style escapes (octal byte runs as UTF-8, plus \\, \", \t, \n) here and in the quoted diff --git fallback, with a test, would show docs/café.md.

latxh and others added 2 commits October 15, 2026 01:48
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>

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Lab DiffViewer looks ready to merge into its parser branch: each clickable line number is now its own Tab stop, the parser, createDiffFile and the performance page live in their own stacked pull requests, and the viewer's tests, lint, catalog, registry-lock and accessibility checks pass on this revision, with its files type-checking cleanly. No further changes are needed here; the stack reaches main once #7568 lands. One optional follow-up: renderLineAnnotation draws an empty row when the callback returns false (for example cond && <Thread />), so treating booleans like null would make that idiom safe.

[Automated review]

{renderMarker(row.cell, 'm')}
{renderCode(row.cell, 'c')}
</tr>
{note != null && (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional, not blocking: false is a valid ReactNode (for example from ref => hasThread(ref) && <Thread />), but it passes this != null test and draws an empty, padded annotation row under every such line; the split branch (hasNote, line 893) has the same test. The JSDoc asks for null, so documented usage is fine, but treating booleans and '' like null here, with a test, would make the common JSX idiom safe.

…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.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Lab DiffViewer looks ready to merge into its parser branch: false annotation results now draw no row, with a regression test, and the viewer's tests, lint, catalog, registry-lock, keyboard and automated accessibility checks pass on this revision. Nothing else is needed here, and two follow-ups are worth tracking before promotion:

  • The clickable line numbers are 8.4–16.9 by 20 px pointer targets, which WCAG 2.2's target-size check (SC 2.5.8) flags, so either document why the inline exception applies or enlarge the targets.
  • The new 'use client' in packages/lab/src/DiffViewer/index.ts makes parseUnifiedDiff uncallable from React Server Components, and DiffViewer.tsx already carries its own directive, so the folder-level one can go.

[Automated review]

gutterRemoved: {
color: 'var(--color-syntax-removed-marker)',
},
gutterButton: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, for promotion: these inline buttons measure 8.4 by 20 px (16.9 by 20 px with two digits) at a 20 px row pitch, and axe's target-size rule (WCAG 2.2 SC 2.5.8, off by default, so the PR audit skips it) flags 31 of the 33 line-number buttons in the Review Comments story. Either record why the inline exception covers the gutter, or meet the size or spacing rule, for example with a 24 px row pitch while onLineNumberClick is set.

@@ -1,13 +1,18 @@
// Copyright (c) Meta Platforms, Inc. and affiliates.

'use client';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: this directive makes the whole folder entry a client module, so a React Server Component that imports parseUnifiedDiff from @astryxdesign/lab gets a client reference it cannot call. DiffViewer.tsx already starts with 'use client', so dropping this one keeps the viewer a client component without turning the parser into a client reference.

return null;
}
const note = renderLineAnnotation(cell.ref);
return typeof note === 'boolean' ? null : note;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: an empty string (for example label && <Thread /> with an empty label) still draws a padded empty row here. The JSDoc asks for null or false, so this is not a contract break, but treating '' like null would cover the last common falsy idiom.

…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.
…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.
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.
cixzhang added a commit that referenced this pull request Oct 15, 2026
…del (#7568)

<!-- Use this template only when adding a new user-facing capability is the primary intent. -->

## User need

Builders showing code changes (review pages, deploy diffs, proposed-edit panels) usually start from a patch: `git diff` output, a `diff -u` file, or a hunk from an API. To render it they need it as structured files, hunks and numbered lines, without writing their own parser.

## Problem and solution fit

- **Primary:** `parseUnifiedDiff(patch)` in `@astryxdesign/lab` returns a `DiffViewerFile[]`, one per file in the patch. It handles:
  - git patches with mode, rename and binary lines;
  - classic `diff -u` output and multi-file patches;
  - bare hunks, CRLF line endings and no-newline markers.
  It never throws.
- **Paths:**
  - An unquoted git header path with spaces (`a/my logo.png b/my logo.png`) is read by its middle space when both sides match.
  - Git-quoted paths are decoded, so `"a/docs/caf\303\251.md"` reads `docs/café.md`. Decoding covers octal UTF-8 bytes and C escapes, in headers, `---`/`+++` lines and rename lines.
- **Supporting:** the model types (`DiffViewerFile`, `DiffViewerHunk`, `DiffViewerLine`, …) and 13 tests.

This is the bottom layer of the Lab DiffViewer stack, split out at review. #7536 (the viewer) stacks on it; its tests, stories and showcase render parsed patches.

## Current specification

None for Lab (by design). No Core change.

## Public delta and caller burden

New Lab-only exports: `parseUnifiedDiff` and the `DiffViewer*` model types.

```ts
const files = parseUnifiedDiff(patch);
```

## Design and alternatives

- **Tolerant, not strict:** unknown header lines are skipped, and a hunk whose counts run short ends where its lines end. Patches pasted from tools and chat are often trimmed.
- **Line anchors:** each line has old/new numbers, so a comment anchor survives a change of layout.

## Evidence

Vitest, `parseUnifiedDiff.test.ts`, 13 tests:
- patch shapes: git, multi-file, rename, binary, deleted, `diff -u`, bare hunk;
- line edge cases: `--`-prefixed removed lines, CRLF, no-newline markers, non-diff input;
- paths: spaced paths in headers and renames, and quoted non-ASCII and escaped paths.

## 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   # 13 passed
```

Originally written by @latxh in #7536.
@cixzhang
cixzhang changed the base branch from feat/lab-diff-parse to main October 15, 2026 13:30
# Conflicts:
#	packages/lab/src/DiffViewer/diffTypes.ts
#	packages/lab/src/DiffViewer/index.ts
#	packages/lab/src/DiffViewer/parseUnifiedDiff.ts
#	packages/lab/src/index.ts

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are requested on this Lab DiffViewer for one right-to-left fix; everything else checks out on this revision. On a right-to-left page, when any line is wider than the viewer (the default unwrapped mode), it opens scrolled to the lines' ends, so each code line's start is cut off and short lines look blank; for example, the Split story in a right-to-left frame at 1280 px hides the start of 23 of its 44 non-empty code cells, while left-to-right hides none. Please keep line starts visible at the initial scroll position (for example a left-to-right viewport like a code editor, or sticky mirrored gutters) and add a right-to-left story or check with a line wider than the viewer. The small line-number targets and the folder-level 'use client' noted earlier remain optional follow-ups.

[Automated review]

Comment on lines +532 to +537
// Code, hunk headers, paths, and counts read left-to-right in every page
// direction; only the table's column order (gutters) mirrors under RTL.
ltr: {
direction: 'ltr',
unicodeBidi: 'isolate',
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a right-to-left page these left-to-right code cells sit inside a max-content table whose viewport inherits the page direction, so when any line is wider than the viewer it opens at its right edge and every line's start is cut off (short lines look blank). In Chromium, the Split story at 1280 by 800 hides the start of 23 of its 44 code cells right-to-left and none left-to-right. Keeping the hunk viewport left-to-right, or making the mirrored gutters sticky and opening at the code's start, would keep line starts and numbers visible together; a right-to-left story with a long line would guard it.

… 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.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are requested on this Lab DiffViewer for one right-to-left follow-up: code lines now open at their start on right-to-left pages, but annotation rows were switched back to right-to-left inside that left-anchored hunk area. Whenever a line is wider than the viewer, a right-to-left page's comments now open past the right edge (in the Review Comments split view the comment text is out of view while its Resolve button shows, and the new Right To Left story's note opens off-screen), and the split divider in annotation rows moves to the outer left edge. Please keep annotation content in view at the initial scroll, for example by leaving the cells left-to-right and giving an inner wrapper the page direction and the viewer's visible width, and extend the right-to-left story or a check so its note is visible on load. The small line-number targets and the folder-level 'use client' remain optional follow-ups.

[Automated review]

// Annotation rows hold the caller's content (review comments), which reads
// in the page's direction even inside the left-to-right hunk area.
annotation: {
direction: {default: 'ltr', ':is([dir="rtl"] *)': 'rtl'},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a right-to-left page this switches the annotation cell back to right-to-left inside the left-to-right viewport, so caller content starts at the cell's right end, which is the far end of the table. Whenever any line is wider than the viewer, that end is out of view at the initial scroll: in the Review Comments split view the comment text sits past the right edge while its Resolve button shows, and the Right To Left story's note opens more than 2,000 px off-screen. It also moves codeOldSide's borderInlineEnd on the old-side annotation cell (line 924) to the outer left edge. Leaving the cell left-to-right and giving an inner wrapper the page direction and the viewer's visible width would keep comments in view and the divider in place.

…t pages

Annotation cells were switched to right-to-left inside the left-to-right
hunk area, so on a wide table a right-to-left page's comments started at
the table's far right end, out of view, and the split divider moved to the
outer edge. The cells now stay left-to-right; an inner wrapper takes the
page direction and is capped at the viewport's visible width, which the
shared ResizeObserver writes to a custom property on the viewport (no
re-render). The RightToLeft story's note now sits on the old side, where it
opens in view.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are requested on this Lab DiffViewer for one remaining right-to-left case: the new annotation wrapper keeps comments in view in unified view and on the old side of split view, but a new-side comment in split view still opens with its start past the right edge whenever that side starts inside the viewer. In the Review Comments story's split view on a right-to-left page, the comment text sits just past the edge of the 1,046 px viewer while its Resolve button is visible, as on the previous revision. Please cap the new-side wrapper to the part of the viewer that side occupies (for example by subtracting how far into the viewer the new side starts) and add a right-to-left split-view story or check with a new-side note. Optionally, overflowWrap: 'anywhere' on the wrapper would stop unbreakable right-to-left text wider than the cap from being clipped at the table's left edge.

[Automated review]

Comment on lines +965 to +969
{newNote != null && (
<div {...stylex.props(annotationStyles.content)}>
{newNote}
</div>
)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a right-to-left page this new-side wrapper gets the same cap as the old side, the viewer's whole visible width, but it starts where the new-side cell starts, mid-viewer, so it still runs past the right edge and right-to-left content starts out of view. In the Review Comments split view the comment text sits just past the edge of the 1,046 px viewer while its Resolve button shows, as on the previous revision. Capping this wrapper to the part of the viewer the new side occupies (for example, the observer also writing how far into the viewer the new side starts, or 0 when it starts past the edge) would keep its start in view, and a right-to-left split-view story or check with a new-side note would guard it.

const annotationStyles = stylex.create({
content: {
direction: {default: null, ':is([dir="rtl"] *)': 'rtl'},
maxInlineSize: `calc(var(${VISIBLE_INLINE_SIZE}, 100%) - 2 * ${spacingVars['--spacing-3']})`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: with the page-direction wrapper narrower than the table, an unbreakable right-to-left run wider than this cap overflows toward the table's left edge, which the left-to-right viewer cannot scroll past (a 1,178 px token in the Right To Left note starts 232 px left of the viewer while the scroll is already at its start). Adding overflowWrap: 'anywhere' here would let such runs wrap within the cap.

…art of the viewer that side occupies

The new-side annotation got the whole visible width as its cap but starts
mid-viewer, so on a right-to-left page its start ran past the right edge.
The ResizeObserver (now also watching the content box, which resizes with
the columns) writes how much of the visible width the new side occupies,
measured from the new side's first column header; a slice too narrow to
hold a word counts as past the edge. Annotation content also wraps long
unbreakable runs (overflow-wrap: anywhere). Adds a RightToLeftSplitComments
story with a comment on each side.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are requested on this Lab DiffViewer for one follow-up to the new-side cap: right-to-left comments in split view now open in view, but when only a narrow part of the new side is visible the cap crushes the comment on both left-to-right and right-to-left pages. In the Review Comments story's split view at a 768 px page, the comment card is 70 px wide, its text stacks one letter per line and the Resolve button is a 28 px sliver with its label clipped, while the previous revision kept that card intact on left-to-right pages. Please treat a slice narrower than a usable comment width as past the edge (for example by raising MIN_NEW_SIDE_VISIBLE_PX to around 200–240 px, so narrower slices scroll as they already do under 48 px) and add a story or check with a narrow new-side slice. Optionally, measuring where the new side starts in layout pixels (such as the header cell's offsetLeft) instead of getBoundingClientRect() would keep the cap right inside scaled containers, such as a dialog during its opening animation.

[Automated review]

* the new side counts as starting past the edge (its note then needs a scroll,
* like its code).
*/
const MIN_NEW_SIDE_VISIBLE_PX = 48;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: with a 48 px floor, a new-side slice only somewhat wider than that still caps the comment at the slice, whatever the content needs. In the Review Comments story's split view at a 768 px page the cap is 70 px: the comment text stacks one letter per line and Resolve shrinks to a 28 px sliver with its label clipped, on left-to-right pages too. Raising this floor to a usable comment width (around 200–240 px) would send narrower slices to the existing fallback (the whole visible width, scrolled to like that side's code).

Comment on lines +780 to +782
const start =
newSide.getBoundingClientRect().left -
table.getBoundingClientRect().left;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: getBoundingClientRect() includes ancestor transforms while viewport.clientWidth does not, so inside a scaled container the cap comes out too wide; with the story scaled to 50% and resized, it read 710 px instead of 422 px and the right-to-left note started out of view. A dialog's scale(0.95) opening animation could do the same if the first measurement lands during it. Measuring in layout pixels, for example the header cell's offsetLeft within the table, avoids this.

…ead of crushing its comment

With a 48 px floor, a new-side slice only a little wider capped the comment
at the slice: at a 768 px page the Review Comments card was 70 px wide, its
text one letter per line and its Resolve button clipped, on left-to-right
pages too. The floor is now 240 px (a usable comment with its actions);
narrower slices keep the whole visible width and are reached by scrolling,
like that side's code. Where the new side starts is now read in layout
pixels (the header cell's offsetLeft) so a scaled ancestor, such as a
dialog's opening animation, no longer skews the cap.

This branch was successfully deployed

1 active deployment
Preview — 58be66dd Deployed Oct 15, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants