Migrate the diff renderer to @pierre/diffs - #110
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Deploying agent-render with
|
| Latest commit: |
290209a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a1764e7c.agent-render.pages.dev |
| Branch Preview URL: | https://pierre-diffs-migration.agent-render.pages.dev |
|
| Filename | Overview |
|---|---|
| src/components/renderers/diff-renderer.tsx | Migrates patch and content rendering to Pierre components, but the previously reported desktop content-layout defect remains. |
| src/app/globals.css | Replaces legacy diff styling with Pierre custom properties while retaining the two-column shell responsible for the outstanding content-width issue. |
| src/lib/diff/pierre-react.ts | Adds a client-side bridge and registers the custom CSS-variable syntax theme. |
| package.json | Replaces @git-diff-view/react with @pierre/diffs. |
| scripts/check-build-budgets.mjs | Re-keys and tightens the deferred diff-renderer bundle budget. |
Reviews (4): Last reviewed commit: "Drop the diff toolbar's self-narrating l..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const { diffFiles } = renderedDiff; | ||
| const RichDiffView = diffLibrary?.DiffView; | ||
| const richDiffMode = diffLibrary ? getDiffLibraryMode(mode, diffLibrary) : null; | ||
| const diffOptions = getDiffOptions(mode); |
There was a problem hiding this comment.
[WARNING]: getDiffOptions(mode) builds a fresh options object on every render, and it is passed by reference straight into the heaviest deferred renderer (PatchDiff/MultiFileDiff).
DiffRendererContent re-renders on any local state change — setIsReady(true) in the rAF effect, setIsNarrowScreen media-query updates, and activeFileId selection — so every one of those re-renders hands the Shiki/shadow-DOM diff a brand-new options object identity. In the previous implementation the expensive diff work was computed once per artifact (via the warmed DiffFile instances) and was only rebuilt on artifact change; here the heavy component is re-rendered on unrelated state churn whenever the library does not memoize on its own. If @pierre/diffs re-runs diff computation/highlighting on a changed options reference, this is a per-interaction cost regression on exactly the component the PR is trying to keep off the hot path.
Memoize the options on mode (and keep it out of every-render recomputation):
| const diffOptions = getDiffOptions(mode); | |
| const diffOptions = useMemo(() => getDiffOptions(mode), [mode]); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 source files + infra)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 088fbc8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 088fbc8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (11 files, incremental 8428e18..HEAD)
Fix these issues in Kilo Cloud Previous review (commit 8428e18)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files, incremental 3915683..HEAD)
Fix these issues in Kilo Cloud Previous review (commit 3915683)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (22 files)
Reviewed by deepseek-v4-flash · Input: 51.5K · Output: 21.6K · Cached: 967.7K |
![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: 391568316d
ℹ️ 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".
| import { Component, type CSSProperties, type ReactNode, useEffect, useMemo, useRef, useState } from "react"; | ||
| import { Check, Columns2, Copy, Rows3 } from "lucide-react"; | ||
| import { useResolvedTheme, type ResolvedTheme } from "@/components/theme/use-theme-controller"; | ||
| import { PatchDiff, MultiFileDiff, type FileDiffProps } from "@/lib/diff/pierre-react"; |
There was a problem hiding this comment.
When the deferred @pierre/diffs code cannot be fetched or evaluated—for example, during an interrupted load or a deployment with stale cached HTML—this static import fails before DiffRenderer or DiffRendererBoundary can mount, so the artifact stage cannot show the raw patch fallback. The previous implementation caught failure of the inner rich-renderer import and returned getFallbackState; preserve that behavior by loading the library behind a catchable boundary or supplying an error fallback for the dynamic renderer import.
Useful? React with 👍 / 👎.
8428e18 to
088fbc8
Compare
The old @git-diff-view/react output read as a debug dump with pseudo-table headers; the decided direction (cursor session, criticism #15) was to hand rendering to diffs.com's library. @pierre/diffs is Shiki-based, renders into shadow DOM, and computes before/after diffs from raw contents, which the old stack could not do (the oldContent/newContent path rendered an empty diff body). - diff-renderer.tsx: PatchDiff per parsed patch file and MultiFileDiff for before/after contents, imported through a small use-client bridge (src/lib/diff/pierre-react.ts) so the deferred chunk id stays stable across webpack graphs. The parse gate, per-file nav, binary handling, unified/split toggle, fallback, error boundary, and every data attribute are unchanged. - The vendored stylesheet pipeline is gone: diff-view-stylesheet.ts, public/vendor/diff-view-pure.css(.br), the mirror step in assets:compress, its _headers entry, and the sync test. Shadow-DOM styles are themed via --diffs-* custom properties on .diff-renderer-frame, keeping diff bodies dark charcoal in both themes. - Budget re-keyed to the artifact-stage dynamic import and tightened to 160 KiB gzipped (measures 142 KiB; Shiki grammars load as separate on-demand chunks outside this key, unlike the old bundled highlighter at 323.9/340 KiB). - Header tests keep exercising generic *.css.br serving via a neutral fixture name; the e2e stylesheet-lifecycle test now asserts shadow-DOM rendering with no external diff stylesheet. Lint, typecheck, 321 unit tests, build budgets, and the chromium e2e suite (28/28) pass. Visual snapshots are not regenerated here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pinned pierre-dark Shiki theme highlighted diffs in a foreign color
vocabulary. Register a CSS-variables Shiki theme ("agent-render") in the
pierre bridge instead: token colors resolve from --diffs-token-* custom
properties defined on .diff-renderer-frame, which map to the app's --rb-*
rainbow palette so diff syntax matches the code renderer. The concrete
values double as baked-in fallbacks for contexts without the app CSS.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The "review-style diff / syntax highlighted" meta, the fallback's "raw patch fallback / invalid unified diff" pills, and the "Unified is the phone default" note all narrate what the surface already shows; the fallback's explanatory status message below the toolbar stays. Dead .diff-mobile-note CSS removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
088fbc8 to
290209a
Compare
1 participant
Stacked on #109. Implements the diff-renderer decision from the owner's design threads: the old
@git-diff-view/reactoutput "is worthless. just use diffs.com".What
@pierre/diffs(diffs.com's library, Apache-2.0):PatchDiffper parsed patch file inside the existing multi-file shell,MultiFileDifffor before/after contents. Shiki-based highlighting, shadow-DOM styles, split/unified built in.oldContent/newContentpath previously rendered an empty diff body (the old library never computed diffs from raw contents);MultiFileDiffdoes, and a unit test now covers the path.public/vendor/diff-view-pure.css(.br), its loader module,_headersentry,assets:compressmirror step, and the byte-sync test. Net diff is −998 lines.--diffs-*custom properties (names verified against the package) on.diff-renderer-frame, with the pinned pierre-dark Shiki theme.use clientbridge (src/lib/diff/pierre-react.ts); a direct deep import from the deferred chunk produced webpack chunk-id corruption in a prior migration attempt, and the bridge is also the seam unit tests mock.parseGitPatchBundlemetadata, per-file nav and headers, binary short-circuit, unified/split toggle with the phone gating, raw-patch fallback and error boundary, everydata-*attribute, and thenext/dynamiccode-split boundary.Budgets
The deferred diff chunk now measures 142 KiB gzipped vs 323.9 KiB before; the budget is re-keyed to the artifact-stage dynamic import and tightened from 340 to 160 KiB. Shiki grammars load as separate per-language on-demand chunks outside this key.
Verification
style-src 'unsafe-inline', so shadow-DOM styles are unaffected🤖 Generated with Claude Code