Refactor diff viewer onto Pierre React primitives#5308
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a typed DiffViewerStatus and document sync, seeds App reducer with bootstrap-derived initialStatus, converts controller→host status publishing, repositions LoadingLayer, adds streaming patch parsing, actions/utilities, theme/options helpers, icons, and tests. ChangesDiff Viewer Status Reactive Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR refactors the diff viewer away from direct DOM manipulation via a monolithic controller toward React-managed state (
Confidence Score: 5/5Safe to merge — the React state migration is well-structured, streaming logic is tested, and the executable-bound server reuse check correctly validates the running PID via proc_pidpath before reusing a cached server. The controller removal and React state migration are complete: every status transition goes through dispatch, the loading layer is a pure React component, and document.body.dataset is kept in sync by usePageDataAttributes. The streaming patch loader handles repeated-path IDs, renames, and git-boundary splitting correctly. The Swift proc_pidpath check uses the correct PROC_PIDPATHINFO_MAXSIZE buffer and falls back gracefully for old state files that lack the executablePath field. Localization for the new copyFailedGitApplyCommand label covers all existing catalog locales. No correctness, data, or security issues were identified. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App mounts] --> B[initialDiffViewerStatus]
B --> C{payload type?}
C -->|pendingReplacement| D[usePendingReplacement]
C -->|statusMessage| E[set-status: terminal message]
C -->|normal diff| F[useRenderDiff]
F --> I[streamPatch fetch patchURL]
I --> J[createStreamingPatchFileSplitter]
J --> K[appendFileDiffToModel]
K --> L{batch ready?}
L -->|yes| M[onBatch dispatch append-items]
L -->|no| N[yieldToNextFrame rAF]
N --> K
M --> O[onTreeSource dispatch set-tree-source]
M --> Q[React renders CodeView + LoadingLayer]
Q --> R[usePageDataAttributes syncs document.body.dataset]
Reviews (21): Last reviewed commit: "Normalize diff separator backgrounds" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (firstRender) { | ||
| firstRender = false; | ||
| document.body.dataset.loading = "false"; | ||
| status.remove(); | ||
| } |
There was a problem hiding this comment.
host.setStatus
On the normal rendering path, flushPendingItems clears the loading state with a direct DOM write (document.body.dataset.loading = "false") without calling host.setStatus(). As a result, the React status state is permanently stale (loading: true, original message) after the first batch renders. The #loading-layer is hidden by CSS correctly, so there's no visible bug, but the React state never reflects the completed loading transition. If setStatus is later called for a secondary status update (e.g., an error that makes #loading-layer visible again), React will briefly see the old stale loading status before applying the new one in the same commit — an incomplete move to the React-state ownership the PR intends.
| test("initialDiffViewerStatus uses pending replacement status", () => { | ||
| const config: DiffViewerConfig = { | ||
| payload: { | ||
| pendingReplacement: true, | ||
| statusMessage: "Rendering diff", | ||
| }, | ||
| }; | ||
|
|
||
| expect(initialDiffViewerStatus(config, label)).toEqual({ | ||
| error: false, | ||
| loading: true, | ||
| message: "Rendering diff", | ||
| pending: true, | ||
| statusOnly: false, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
pendingReplacement without a statusMessage
initialDiffViewerStatus contains a fallback path (payload.statusMessage ?? label("loadingDiff")) when pendingReplacement: true but statusMessage is absent, yet only the case with statusMessage present is tested. A test confirming the fallback produces message: "Loading diff" with loading: true, pending: true would close the coverage gap.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Covered earlier in this PR. diff-viewer/test/status.test.ts now has a pendingReplacement-without-statusMessage case that asserts the fallback loading status.
— Claude Code
There was a problem hiding this comment.
Covered in current head. diff-viewer/test/status.test.ts includes the pendingReplacement-without-statusMessage fallback case.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@diff-viewer/src/viewer-controller.ts`:
- Around line 291-295: The code clears only document.body.dataset.loading when
loading finishes, leaving the React host state stale; instead update the React
status via the existing helper: replace the direct DOM-only clearing call with a
call to showStatusMessage or host.setStatus using a success/idle
DiffViewerStatus (e.g. createDiffViewerStatus('', { loading: false, pending:
false }) or similar) so that applyDiffViewerStatusToDocument still updates the
DOM and host.setStatus keeps React state in sync; locate the direct manipulation
and swap it to call showStatusMessage(...) or host.setStatus(...) with the
non-loading status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf512748-e44a-4452-8519-cf6a8381d3f8
📒 Files selected for processing (7)
Resources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/src/App.tsxdiff-viewer/src/main.tsxdiff-viewer/src/status.tsdiff-viewer/src/styles.cssdiff-viewer/src/viewer-controller.tsdiff-viewer/test/status.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 7 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
lawrencecchen
changed the title
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e32358b498
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onCopyGitApply={async () => { | ||
| try { | ||
| const message = await copyGitApplyCommand(payload.patchURL, label); | ||
| setStatus(createDiffViewerStatus(message, { loading: false, statusOnly: true })); |
There was a problem hiding this comment.
When a user chooses "Copy git apply command" after a diff has rendered, this sets statusOnly: true; the existing CSS for body[data-status-only="true"] hides the files sidebar and centers #loading-layer as an empty-state screen, so a successful copy replaces the usable diff with a permanent confirmation page until reload. The previous implementation only updated the options button title/aria label, so this should be a transient/non-status-only notification instead of entering the terminal status-only state.
Useful? React with 👍 / 👎.
| if (!navigator.clipboard?.writeText) { | ||
| throw new Error("Clipboard API unavailable"); |
There was a problem hiding this comment.
In viewer contexts where navigator.clipboard.writeText is unavailable, this now always throws and the Copy git apply command cannot work. The deleted controller had an execCommand("copy") fallback for exactly this case, so users on those embedded/custom-scheme WebKit contexts lose the copy action even though a textarea fallback can still succeed.
Useful? React with 👍 / 👎.
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@diff-viewer/src/actions.ts`:
- Around line 25-28: The function copyGitApplyCommand currently returns
label("renderFailed") when patchURL is missing which causes callers like
Toolbar.onCopyGitApply to treat the action as a success; change
copyGitApplyCommand to throw an Error (or reject) when patchURL is undefined
instead of returning a string so the failure path is taken. Locate
copyGitApplyCommand and replace the early return that uses label("renderFailed")
with a thrown Error (include a descriptive message that mentions patchURL) so
callers can catch and surface an actual failure.
In `@diff-viewer/src/App.tsx`:
- Around line 600-608: The call to preparePresortedFileTreeInput(source.paths)
is being recomputed on every PierreFileTree render; memoize this preprocessed
value and pass the memoized result into useFileTree as preparedInput to avoid
repeated full scans. Create a memo (e.g., via useMemo) keyed on source.paths (or
a stable derived key) where preparePresortedFileTreeInput is invoked, update the
preparedInput prop to use that memo value, and ensure resetPaths (and any other
callers) reuse the same memoized prepared input so
preparePresortedFileTreeInput, useFileTree, and resetPaths no longer perform
redundant work on unrelated status/selection toggles.
In `@diff-viewer/test/app.test.tsx`:
- Around line 10-15: The afterEach cleanup currently only unmounts React via the
root variable; update the afterEach block to also restore any globals the test
mocks (e.g., window, document, navigator, DOM constructors like
Element/Node/HTMLElement, and fetch) to their original values by assigning back
previously-saved originals (e.g., originalWindow, originalDocument,
originalNavigator, originalElementConstructor, originalFetch) before setting
root = null; ensure these restore assignments occur inside the same afterEach
that references root so later tests don't inherit the mocked globals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2aab744-674b-43df-af01-f4a122823c3c
⛔ Files ignored due to path filters (1)
diff-viewer/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Resources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/package.jsondiff-viewer/src/App.tsxdiff-viewer/src/actions.tsdiff-viewer/src/diff-stream.tsdiff-viewer/src/icons.tsxdiff-viewer/src/pierre-options.tsdiff-viewer/src/viewer-controller.tsdiff-viewer/test/app.test.tsx
💤 Files with no reviewable changes (1)
- diff-viewer/src/viewer-controller.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2264854f37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| const started = useRef(false); | ||
| useEffect(() => { | ||
| if (started.current || config.payload?.pendingReplacement === true || typeof config.payload?.statusMessage === "string") { |
There was a problem hiding this comment.
When the host includes statusMessage: "" (or another empty string), initialDiffViewerStatus treats it as a normal diff render because it checks statusMessage.length > 0, but this guard skips streamPatch for any string value. In that case the patch URL is never fetched and the viewer remains on the initial loading state indefinitely; keep this condition consistent with the non-empty check used in status.ts.
Useful? React with 👍 / 👎.
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d0c0fe17d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (navigator.clipboard?.writeText) { | ||
| await navigator.clipboard.writeText(command); |
There was a problem hiding this comment.
When navigator.clipboard.writeText exists but rejects (for example, after the async patch fetch loses the user-activation window or in WebKit contexts that expose the API but deny permission), this throws into the App catch and reports a render failure even though the textarea fallback below can still copy. The previous implementation fell back to execCommand("copy") on writeText failure, so wrap this call in try/catch and use the fallback before surfacing an error.
Useful? React with 👍 / 👎.
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
diff-viewer/src/App.tsx (1)
593-618:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
useSyncedRefto write synchronously (not viauseEffect) sorenderRowDecorationcan’t read stalesource/label.
diff-viewer/src/App.tsx’suseSyncedRefupdatesref.currentinuseEffect, butrenderRowDecorationreadslatest.currentduring the same render pass where the callback is created, so the first paint aftersource/labelchanges can show stale row stats/titles.Suggested fix
function useSyncedRef<T>(value: T): React.MutableRefObject<T> { const ref = useRef(value); - useEffect(() => { - ref.current = value; - }, [value]); + ref.current = value; return ref; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@diff-viewer/src/App.tsx` around lines 593 - 618, The hook useSyncedRef is updating ref.current inside useEffect which is async, causing latest.current to be stale for renderRowDecoration; change useSyncedRef so it writes the incoming value synchronously (e.g., useRef inside the hook and assign ref.current = value during the hook body or via useLayoutEffect instead of useEffect) so callers like latest = useSyncedRef({ label, onSelectItem, source }) always see the latest label/source during the same render and renderRowDecoration reads up-to-date stats/titles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@diff-viewer/src/App.tsx`:
- Around line 593-618: The hook useSyncedRef is updating ref.current inside
useEffect which is async, causing latest.current to be stale for
renderRowDecoration; change useSyncedRef so it writes the incoming value
synchronously (e.g., useRef inside the hook and assign ref.current = value
during the hook body or via useLayoutEffect instead of useEffect) so callers
like latest = useSyncedRef({ label, onSelectItem, source }) always see the
latest label/source during the same render and renderRowDecoration reads
up-to-date stats/titles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60f9914c-7813-4681-a4f4-3eef221cecf0
📒 Files selected for processing (3)
Resources/markdown-viewer/diff-viewer-app/main.mjsdiff-viewer/src/App.tsxdiff-viewer/src/icons.tsx
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a1148de. Configure here.
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d7c6bedb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const source = action.source.preparedInput == null | ||
| ? { ...action.source, preparedInput: preparePresortedFileTreeInput(action.source.paths) } |
There was a problem hiding this comment.
When a large diff is streaming, every onTreeSource dispatch reaches this reducer and calls preparePresortedFileTreeInput over the full source.paths array before React can append just the new paths, so sidebar work grows with the whole diff on each batch and can make large diffs janky. Fresh evidence since the earlier thread: the current reducer now recreates preparedInput here before storing the source, so the append-only file-tree hook no longer avoids the full rebuild.
Useful? React with 👍 / 👎.
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
|
This PR is large and would use a significant portion of your monthly review quota. Comment |
1 participant
Summary
#viewerby rendering the loading layer as a sibling overlay.Testing
cd diff-viewer && bun testcd diff-viewer && bun run typecheckcd diff-viewer && bun run lint:cicd diff-viewer && bun run react-doctor:cicd diff-viewer && bun run build./scripts/build-diff-viewer-app.sh --checkNotes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Protocol and executable-matching change how the local diff viewer server is reused (fewer stale-server bugs, more restarts after rebuilds); asset path/MIME changes must stay aligned with the bundled React app.
Overview
Adds a CI gate that runs
./scripts/check-diff-viewer-react-compiler.mjsin thereact-apps-checkjob so bundled diff-viewer output must pass the React Compiler guard.Diff viewer HTTP server (Swift): bumps the protocol to
react-app-v2 executable-bound, recordsexecutablePathin server state, and only reuses an existing listener when the live process path matches the currentcmuxbinary (viaproc_pidpath). Stale servers from another install or rebuild are no longer reused silently.Bundled Pierre assets: upgrades the shipped bundle to
pierre-diffs-1.2.7, switches the worker entry fromworker-portable.mjstoworker-portable.js, and treats.jsas valid JS alongside.mjsfor MIME checks and asset enumeration.Localization: adds
diffViewer.copyFailedGitApplyCommandacross locales for when copying the git apply command fails.Reviewed by Cursor Bugbot for commit 9b65c3c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Refactors the diff viewer onto React
@pierre/diffs/@pierre/treeswith streamed rendering, an external worker pool, and a transparent code surface. Adds a React Compiler CI guard, a stress‑sample opener, executable‑bound server reuse, and localized copy‑failure feedback.Refactors
CodeView+FileTree; move lifecycle into hooks;Appowns status overlays.WorkerPoolContextProviderwith external Pierre worker assets; sync worker render options; bump to@pierre/diffs@1.2.7/@pierre/trees@1.0.0-beta.4.react-app-v2 executable-bound; allow.jsassets, stageworker-portable.js, and enumerate.js/.mjs.actions.tsfor URL rewriting and clipboard‑safe copy; add stress‑sample opener; CI enforces React Compiler output.Bug Fixes
CodeViewsizing/scrolling; avoid redundant worker option syncs.Written for commit 9b65c3c. Summary will update on new commits.
Summary by CodeRabbit