[design-doctor effort xhigh] replacement clone of sourcebot#2: [design-doctor test] sourcebot-1154: Commit diff viewer#25
Conversation
Adds a `commit` pathType to the browse routes (`/browse/<repo>@<branch>/-/commit/<sha>[/<file>]`) that renders a placeholder CommitDiffPanel. Refactors browse path helpers into a discriminated `BrowseProps` union so commitSha is required only for pathType: 'commit'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires up @codemirror/merge (via react-codemirror-merge) inside CommitDiffPanel with a static before/after demo. Adds a CodeDiff component that owns its language extension + view ref so each pane can reconfigure its language compartment independently. Also gates the react-grab dev scripts behind DEBUG_ENABLE_REACT_GRAP so they don't load on every dev page render. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the CodeMirror MergeView-based commit diff rendering with a DOM based split-view that renders directly from git's hunks, inspired by Chrome DevTools' DiffView. Per-file editor instances and the matching getFileSource fetches are gone — a 50-file commit drops from ~100 network requests to 0, and per-row render cost from a full editor mount to a synchronous Lezer highlight + grid emit. - New `LightweightDiffViewer` builds a single 2-column subgrid with hunk headers spanning both sides; each cell uses `subgrid` so line numbers, markers, and content align across all rows. - Pure helpers split out: `hunkParser` (body string → DiffLine[]), `splitPairing` (DiffLine[] → SplitRow[]). - `presentableDiff` from @codemirror/merge supplies character-level intra-line diff highlighting on paired modifications. - Lezer highlight code lifted from `lightweightCodeHighlighter` into `lib/codeHighlight` so both files share the helper. - Drop `react-codemirror-merge` and `commitDiffEditor`. Long lines wrap via `whitespace-pre-wrap break-words` + `minmax(0, 1fr)` on the grid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- File path header now sticks to the top of the scroll viewport while scrolling through that file's diff, using the negative-yOffset trick to compensate for the virtualizer's translateY positioning. Same pattern as searchResultsPanel/fileMatchContainer. - Lightweight diff viewer's grid uses `minmax(<min>, max-content)` for line-number and marker columns so they don't collapse to zero width when one side of the diff is entirely blank (fully-added or fully-deleted files), keeping the right pane aligned across files. - Drop the column gap between left and right panes and instead draw a `border-r` separator on the left cells for a cleaner divider. - Hunk header gets an optional className so the first hunk renders with just `border-b` (the file header above already provides the top border), while subsequent hunks render with `border-y` between them. - Drop the per-row footer padding in the virtualizer; rows now sit flush against each other. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- New `DiffStat` component renders GitHub-style additions/deletions counts with a 5-square indicator scaled log-ish to total change size. Added on the right of each file row header and on the right of the "N files changed" subheader for the commit total. Hidden when there are no line-level changes (pure renames). - Each file row gets a `CopyIconButton` next to the path (copies newPath, falling back to oldPath) and a `CommitActionLink` that opens the file at the commit. Deleted files link to the old path at the parent commit so the user lands on the file's last existing state rather than a 404. - `repoName`, `commitSha`, and `parentSha` are plumbed from the panel through `FileDiffList` to `FileDiffRow` to support the new link. - `computeChangeCounts` is memoized per file in the row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nchoring
History panel rows in both the bottom panel and the commits page are now
clickable — they navigate to the matching commit diff via router.push,
with closest('button, a') short-circuit so inner action buttons keep
their own behavior. Bottom-panel history rows also highlight via
bg-accent when their commit is the one currently being viewed.
Commit diff header now uses AuthorsAvatarGroup + getCommitAuthors +
formatAuthorsText, matching latestCommitInfo and historyRow — co-authors
parsed from the commit body show up correctly.
When the URL trailing path matches one of the commit's files, that file
is moved to the top of the FileDiffList rather than scrolled to. Avoids
estimateSize-based scroll inaccuracy and works regardless of which side
of a rename the URL points to.
Lightweight diff viewer short-circuits with "Diff too large to display"
for files containing lines over 1000 chars, matching the cutoff in
lightweightCodeHighlighter.
PathHeader's breadcrumb measurement reserved 175px for "copy button and
padding"; the actual reservation needed is ~40px. Reduced so breadcrumbs
no longer collapse prematurely on wide layouts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Lift `truncateSha` (was a private helper in getDiffToolComponent) to `lib/utils` so PathHeader can reuse it. The branch/ref display now renders a 40-char SHA as `abc1234`, preserving any `^` / `~N` suffix. - Hide the `·` separator and the path's CopyIconButton when there's no path (repo root). Previously a dangling `·` and copy button rendered with nothing between them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a `path` query/tool parameter to restrict diff output to changes touching a single file via git's `-- <pathspec>` separator. Refactors the route handler to use the shared `getDiffRequestSchema`. Fixes SOU-1154 (sourcebot-dev#1154) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Move single-file commit diffs from `/-/commit/<sha>/<path>` to
`/-/blob/<path>?ref=<sha>&diff=true`, keeping the user's browsing
revision in the URL. `/-/commit/<sha>` still renders the full
multi-file diff.
- New `FocusedCommitDiffPanel` with status row (file status badge +
authors + relative commit date + "View full commit" + DiffStat +
exit-X) and path-filtered `getDiff` so only the single file's diff
is fetched.
- New preview banner in `CodePreviewPanel` when `?ref=` is set, with a
close button that strips the param.
- Make `PathHeader`'s revision clickable, linking to that ref's full
commit view.
- New `HoverPrefetchLink` defers Next.js prefetching until hover; used
in history rows to avoid firing many prefetches on render.
- Hide the bottom panel on `/-/commit/` views.
- Extract `getFileStatus` / `StatusBadge` to a shared `fileStatus.tsx`.
- Workaround Radix Tooltip + RSC re-render bug (drop `asChild` from
`<TooltipTrigger>`, add `key={commitSha}`) so X / Browse-files
buttons survive client navigation between commits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![softlight[bot]](https://avatars.githubusercontent.com/in/1467739?s=60&v=4){kind=small}
| <CommitMessage subject={subject} body={commitResponse.body} /> | ||
| </div> | ||
| <Tooltip key={commitSha}> | ||
| <TooltipTrigger> |
There was a problem hiding this comment.
Browse files button is doubled up inside the commit view
On the full commit page, the Browse files control is wrapped in an extra interactive layer instead of being a single clean button, which can make its hover, focus, and tooltip behavior feel off and adds a redundant tab stop. It should behave as one tidy button like the other actions in this area.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**Browse files button is doubled up inside the commit view**
On the full commit page, the Browse files control is wrapped in an extra interactive layer instead of being a single clean button, which can make its hover, focus, and tooltip behavior feel off and adds a redundant tab stop. It should behave as one tidy button like the other actions in this area.
Issue:
`<TooltipTrigger>` (no asChild) wraps `<Button asChild variant="outline" size="sm">` whose child is a `<Link>`. Because TooltipTrigger is the bare Radix Trigger, it renders its own native <button> around the Button-as-anchor, producing nested interactive elements (<button><a/></button>), a duplicated focus target, and tooltip/aria wiring attached to the wrapper instead of the real control.
Suggested fix:
Add `asChild` to the `<TooltipTrigger>` at fullCommitDiffPanel.tsx:79 so it merges onto the `<Button asChild>`/`<Link>`, matching commitParts.tsx CommitActionLink.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
| <div className="flex flex-row items-center gap-2"> | ||
| <DiffStat {...computeChangeCounts(file)} /> | ||
| <Tooltip key={commitSha}> | ||
| <TooltipTrigger> |
There was a problem hiding this comment.
Exit control on the file diff is wrapped in an extra layer
In the focused file-diff view, the control to leave the diff is wrapped in a redundant interactive layer rather than being a single clean icon button, which can make its hover and focus states feel inconsistent and adds a duplicate tab stop. It should match the tidy icon buttons used elsewhere in this area.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**Exit control on the file diff is wrapped in an extra layer**
In the focused file-diff view, the control to leave the diff is wrapped in a redundant interactive layer rather than being a single clean icon button, which can make its hover and focus states feel inconsistent and adds a duplicate tab stop. It should match the tidy icon buttons used elsewhere in this area.
Issue:
`<TooltipTrigger>` (no asChild) wraps `<Button asChild variant="ghost" size="icon" className="h-6 w-6 text-muted-foreground">` with an X-icon `<Link aria-label="Exit diff view">`. The bare trigger renders an extra native <button> around the Button-as-anchor (<button><a/></button>), duplicating the interactive element and misattaching tooltip/aria to the wrapper.
Suggested fix:
Add `asChild` to the `<TooltipTrigger>` at focusedCommitDiffPanel.tsx:156 so it merges onto the ghost icon `<Button asChild>`/`<Link>`.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
| </Link> | ||
| </span> | ||
| <Tooltip key={previewRef}> | ||
| <TooltipTrigger> |
There was a problem hiding this comment.
Close control on the preview banner is wrapped in an extra layer
On the file preview banner, the close control is wrapped in a redundant interactive layer instead of being a single clean icon button, which can make its hover and focus states feel inconsistent and adds a duplicate tab stop. It should match the tidy icon buttons used elsewhere.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**Close control on the preview banner is wrapped in an extra layer**
On the file preview banner, the close control is wrapped in a redundant interactive layer instead of being a single clean icon button, which can make its hover and focus states feel inconsistent and adds a duplicate tab stop. It should match the tidy icon buttons used elsewhere.
Issue:
`<TooltipTrigger>` (no asChild) wraps `<Button asChild variant="ghost" size="icon" className="h-6 w-6 text-muted-foreground">` with an X-icon `<Link aria-label="Close preview">`, so the bare trigger renders an extra native <button> around the Button-as-anchor.
Suggested fix:
Add `asChild` to the `<TooltipTrigger>` at codePreviewPanel.tsx:105 so it merges onto the ghost icon `<Button asChild>`/`<Link>`.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
| pathType: 'commit', | ||
| commitSha: parent, | ||
| })} | ||
| className="underline hover:text-foreground" |
There was a problem hiding this comment.
Parent-commit links look different from other commit links
The links to parent commits are styled differently from the other commit references shown in the same view, so commit links don't read as one consistent, recognizable affordance. Aligning them would make it clearer at a glance which pieces of text navigate to another commit.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**Parent-commit links look different from other commit links**
The links to parent commits are styled differently from the other commit references shown in the same view, so commit links don't read as one consistent, recognizable affordance. Aligning them would make it clearer at a glance which pieces of text navigate to another commit.
Issue:
commitHashLine.tsx line 42 styles parent-commit links `className="underline hover:text-foreground"` (always underlined muted text that shifts to foreground on hover), with no use of the --link token.
Suggested fix:
Align the parent-commit links in commitHashLine.tsx:42 to the established commit-reference link treatment (`text-link hover:underline`), or match the muted underline-on-hover pattern used by historyRow SHA links if a muted look is intended.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
| <CommitMessage subject={subject} body={commitResponse.body} /> | ||
| </div> | ||
| <Tooltip key={commitSha}> | ||
| <TooltipTrigger> |
There was a problem hiding this comment.
Commit diff action buttons render as doubled controls
On the commit diff and file-preview views, the buttons that browse files or close the diff are each wrapped in an extra, invisible control, so keyboard users hit an empty stop before the real button and the hover hint can feel slightly off. These should behave as a single, clean control.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**Commit diff action buttons render as doubled controls**
On the commit diff and file-preview views, the buttons that browse files or close the diff are each wrapped in an extra, invisible control, so keyboard users hit an empty stop before the real button and the hover hint can feel slightly off. These should behave as a single, clean control.
Issue:
Each tooltip is written as `<TooltipTrigger>` (no asChild) containing `<Button asChild variant=... ><Link/></Button>`. Since `TooltipTrigger = TooltipPrimitive.Trigger` renders its own <button> when asChild is absent, the rendered DOM is `<button><a>...</a></button>` — a button wrapping an anchor, which is invalid nested interactive markup, adds an extra focus/tab stop, and binds the tooltip trigger to a different element than the visible button.
Suggested fix:
Add `asChild` to each of the three TooltipTrigger usages (fullCommitDiffPanel.tsx:79, focusedCommitDiffPanel.tsx:156, codePreviewPanel.tsx:105) so the Button is the trigger, eliminating the wrapping <button>.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
| renamed: 'bg-blue-500/20 text-blue-700 dark:text-blue-400', | ||
| }; | ||
|
|
||
| export const StatusBadge = ({ status }: { status: FileStatus }) => ( |
There was a problem hiding this comment.
File-change labels don't match the app's status badge style
The added/modified/deleted/renamed markers on each file in the commit diff are built as one-off chips rather than the app's standard status badge treatment, so they read as slightly inconsistent with status indicators elsewhere in the product. Aligning them makes file changes feel part of the same visual language.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**File-change labels don't match the app's status badge style**
The added/modified/deleted/renamed markers on each file in the commit diff are built as one-off chips rather than the app's standard status badge treatment, so they read as slightly inconsistent with status indicators elsewhere in the product. Aligning them makes file changes feel part of the same visual language.
Issue:
StatusBadge renders a raw `<span>` with `inline-flex items-center justify-center w-5 h-5 rounded text-xs font-mono font-bold` plus a bespoke STATUS_BADGE_COLORS map (bg-green-500/20 text-green-700 dark:text-green-400, bg-yellow-500/20..., bg-red-500/20..., bg-blue-500/20...).
Suggested fix:
Render StatusBadge via `<Badge className={statusBadgeVariants({ status })}>` (define a small cva mapping added/modified/deleted/renamed to color classes), keeping the compact single-letter sizing through className overrides.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
| <div | ||
| role="link" | ||
| tabIndex={0} | ||
| className="flex flex-row py-3 px-3 items-center justify-between gap-4 min-w-0 border-b cursor-pointer hover:bg-muted" |
There was a problem hiding this comment.
Commit rows give no cue when focused by keyboard
Commit rows in the history list are now clickable and keyboard-reachable, but they only react to mouse hover, so tabbing through the list shows no visible indication of which commit is focused. A clear focus cue makes keyboard navigation easy to follow.
Prompt to fix with AI
This is a comment left during a design review.
Comment:
**Commit rows give no cue when focused by keyboard**
Commit rows in the history list are now clickable and keyboard-reachable, but they only react to mouse hover, so tabbing through the list shows no visible indication of which commit is focused. A clear focus cue makes keyboard navigation easy to follow.
Issue:
The row gains `role="link" tabIndex={0}` with onClick/onKeyDown(Enter) and `className="... cursor-pointer hover:bg-muted"`, but defines no focus-visible treatment, so a keyboard-focused row looks identical to an unfocused one.
Suggested fix:
Add a focus-visible treatment to the row (e.g. focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2, or focus-visible:bg-accent to mirror the selected surface) so keyboard focus is visible.
How can I resolve this? Keep the fix scoped to the PR-touched UI and preserve existing design-system patterns.
Author
|
Closing contaminated Design Doctor effort A/B clone; replacing with a fresh PR for the same xhigh variant. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replacement Design Doctor reasoning-effort A/B clone for #2.
Variant:
xhigh.This branch adds a no-op commit on top of the original PR head so the file contents are equivalent while GitHub checks stay isolated to this test PR.
Replaces contaminated setup PR: #22.