feat(composer): replace Apply view diff with @pierre/diffs#2394
feat(composer): replace Apply view diff with @pierre/diffs#2394caezium wants to merge 5 commits into
Conversation
|
This is so amazing. Huge upgrade from our bespoke UI. Using the design tokens is a huge win! The code is well organized. I will consider using it to help with the v4 diff view as well. @logancyang can we assign agent review and consider merging it when the review looks good? I manually went through the change and it LGTM. Fantastic work @caezium! 🥳 |
|
@caezium Thanks for the PR! Great stuff! I've tried a bit and seems the pure additions are not highlighted green, only the edits are red/green. Is this something we can customize? For example, these line additions in the first image are hard to see since only the line number is green but not the text. 
|
|
@caezium Thank you! LGTM. @logancyang I can see the highlights for pure additions, but I agree they should be more visible. @caezium can you make them stand out more? 
|
Pure-add and pure-delete rows in the Apply view were too subtle on most themes — the line wash was sourced from --background-modifier-success/ error-rgb at 22% alpha, which is the theme's tuned subtle-affordance color and washed out almost entirely on dark themes. Reviewer feedback on PR logancyang#2394 flagged that pure-addition rows showed only a colored gutter bar while the line body looked uncolored. Changes: - Reroute the line wash to the vivid --color-{green,red}-rgb palette tokens and bump alpha 0.22 → 0.40. Gives an unambiguous colored band across the whole row at a glance, comparable to GitHub's intensity. - Override --diffs-{addition,deletion}-color-override to --color-green / --color-red so the text in pure-changed rows reads in color too. The --diffs-modified-color-override stays at --text-normal so paired-edit rows (which already have inline-emphasis highlights) keep neutral text underneath their token-level wash. - Inline emphasis stays at 0.55 alpha — still ~1.4x the new line wash, so changed tokens punch through the surrounding row on paired edits.
caezium
force-pushed
the
feat/diff-improvements
branch
from
299c761 to
9ea603e
Compare
|
https://diffs.com/ has more info on this I pinned the Accept/Reject bar so it stays put when you scroll, tidy up the header, and changed pure additions and deletions colors. To tweak the diff look later: all the colors live in src/styles/tailwind.css under .copilot-pierre-view as CSS variable overrides (--diffs-bg-addition-override, --diffs-bg-deletion-override, text colors, gutter colors, etc.). Renders across light/dark mode and a few different themes: idk why in light mode copilot looks like this, maybe custom css on my setup |
There was a problem hiding this comment.
LGTM! @caezium Can you resolve the merge conflicts?
|
@caezium looks great! BTW I've updated the merge requirements to enforce signed commits for security reasons. Could you please follow the CONTRIBUTING.md instructions and set it up if you haven't already? Also please resolve the merge conflicts and I will merge. Brilliant work! |
|
Would like to chime in to point out that due to transitively pulling in Shiki’s full bundled language registry plus inlined Oniguruma WASM as a dependency, this change causes the plugin to be massively bloated by ~10 MB - a 3x increase over the whole functionality of Copilot for only a better edit preview experience. Would definitely recommend considering swapping out the rendering backend to avoid such unnecessary bloat. |
caezium
force-pushed
the
feat/diff-improvements
branch
from
608cc64 to
b8cb69f
Compare
|
@logancyang Rebased on master and re-signed all 4 commits with SSH. |
|
@EL-File4138 @pierre/diffs/dist/highlighter/languages/resolveLanguage.js doing a static import { bundledLanguages } from "shiki", which makes esbuild pull in the entire @shikijs/langs tree (~9.8MB on its own). Fix is pre-registering markdown via Pierre's registerCustomLanguage. Should drop main.js from ~15MB to ~3–5MB |
|
and also: to anyone wanting to push the diff UI further: https://diffs.com/ has live demos of features this PR is not using yet, the package exposes a lot more than what's wired up here. Per-line accept/reject (hover-revealed), hunk-level controls, custom gutter buttons, scroll-sync between split panes - are all available, you can choose how you want it to look like |
caezium
force-pushed
the
feat/diff-improvements
branch
from
b8cb69f to
5285171
Compare
Swap the bespoke blocks-based diff in ApplyView for @pierre/diffs'
PatchDiff component, themed to render in Obsidian's CSS variables.
Renderer:
- New PierreRenderer wraps PatchDiff with createPatch + a custom Shiki
theme ("obsidian") that maps every syntax token to Obsidian text
variables, so markdown headings, bold, links, and code render in the
user's editor colors instead of Pierre's stock red/orange/cyan palette.
- disableWorkerPool — Obsidian's renderer process already runs in a
single Electron window; spawning Shiki workers there is wasted.
- overflow: "wrap" so long prose lines don't horizontally truncate in
side-by-side view.
CSS theming (`.copilot-pierre-view` in tailwind.css):
- Override --diffs-{addition,deletion,modified}-color-override to
--text-normal so changed-line TEXT reads as normal Obsidian text;
diff signal comes from background wash + inline emphasis only.
- Comprehensive --diffs-* overrides for bg, gutter, font, line-height,
hover, selection, line numbers, decoration bar, Shiki token colors.
- Container border + header strip themed to --background-secondary.
ApplyView:
- Drops ~600 lines of SideBySideBlock / SplitBlock / per-block
accept-reject state — Pierre doesn't model per-block decisions, so
accept writes newText verbatim.
- Existing viewMode toggle (split / side-by-side) drives Pierre's
diffStyle prop and persists via the diffViewMode setting.
- Floating Accept/Reject pill is centered within the pane via a
full-width absolute row + flex-justify-center (instead of viewport-
anchored tw-fixed + tw-left-1/2, which put the pill's left edge —
not its center — at the 50% line and drifted off-center with
sidebars open). Pointer-events split so empty space alongside the
pill doesn't swallow clicks on the diff below.
Misc cleanup:
- Drop dead `ApplyViewState.simple` prop — was set by editFile but
never read by any consumer.
- Jest moduleNameMapper + __mocks__/pierre-diffs-react.js stub so
ts-jest can resolve Pierre's ESM subpath exports in tests (tests
don't exercise Pierre rendering, just need the import to compile).
The floating bar scrolled offscreen because the React root div had no height — set it to fill the leaf via inline style. Header gets a soft secondary band, mono path, and +N/−M line counts.
Wash sourced from --background-modifier-{success,error}-rgb at 0.22 was
a tuned-subtle theme color that washed out on most dark themes. Switch
to the vivid --color-{green,red} palette at 0.40 alpha, and override
--diffs-{addition,deletion}-color-override to green/red so the text in
pure-changed rows reads in color too.
Collapsible per-line decisions panel beneath the Pierre diff — click the header to override individual added/removed lines before applying. The common case (accept everything) stays one click via the floating bar.
caezium
force-pushed
the
feat/diff-improvements
branch
from
5285171 to
189a14f
Compare
Upstream added stricter ESLint rules between this PR's base and current master: - `no-restricted-syntax` now bans bare `createRoot` from `react-dom/client`; standalone React roots must use `createPluginRoot` so `useApp()` is available in descendants (PR logancyang#2466). - `obsidianmd/no-static-styles-assignment` bans `element.style.cssText`; use Tailwind classes instead. - `@typescript-eslint/no-unsafe-member-access` now flags `.message` on `unknown`/`any` caught errors — narrow with `instanceof Error` first. - `@typescript-eslint/no-unnecessary-type-assertion` flags `as Decision` in spots where TS already infers the literal correctly. Net effect on the diff renderer: no behavior change, just imports and small reshuffles.
|
@caezium thanks for following up on this. We are heads down on V4 preview right now and the Copilot plugin is going to be revamped. We will revisit this PR once v4 is ready. Thanks again for the contribution! |
5 participants
Summary
Swap the bespoke blocks-based diff in the Apply view for
@pierre/diffs(Shiki-backed), themed against Obsidian's CSS variables so it inherits the user's editor look.The previous renderer rebuilt its own line/word diff UI from scratch (~600 lines of
SideBySideBlock/SplitBlock/ per-block accept-reject state). Pierre handles all of that — including markdown-aware syntax highlighting via Shiki — and the result reads as native Obsidian text rather than a separate pierre-dark pane.Before / After
What changed
New renderer (
src/components/composer/PierreRenderer.tsx):PatchDiffwithcreatePatch+ a custom Shiki theme ("obsidian") registered once at module load.token-keyword,token-string,token-link, …) to an Obsidian text variable (--text-normal,--text-accent,--text-muted), so code blocks, headings, links, and bold inside the diff render in the user's editor colors — not Pierre's stock red/orange/cyan.disableWorkerPool(Obsidian's renderer process is single-Electron-window, no need to spawn Shiki workers).overflow: "wrap"so long prose lines wrap instead of horizontally truncating in side-by-side.CSS theming (
src/styles/tailwind.css → .copilot-pierre-view):--diffs-{addition,deletion,modified}-color-overrideto--text-normal— changed-line text stays the normal Obsidian color; the diff signal comes from background wash + inline emphasis spans only.--diffs-*overrides for backgrounds (--background-modifier-success-rgb/error-rgbat 0.22 alpha for line wash, 0.55 for inline emphasis), gutters, line numbers, hover, selection, font family, line-height, and decoration bar.--background-secondary.ApplyView.tsxsimplification:~600 linesofSideBySideBlock/SplitBlock/ per-block accept-reject. Pierre doesn't model per-block decisions, so accept writesnewTextverbatim — same UX as the previous "accept all" path.viewModetoggle (split / side-by-side) now drives Pierre'sdiffStyleprop and persists via thediffViewModesetting.tw-flex tw-justify-center, pointer-events split so empty space doesn't swallow clicks on the diff). The previoustw-fixed + tw-left-1/2anchored to the viewport and put the pill's left edge — not its center — at the 50% line.Misc:
ApplyViewState.simpleprop (set byeditFilebut never read by any consumer).__mocks__/pierre-diffs-react.jsstub + twomoduleNameMapperentries injest.config.js— Pierre uses ESM subpath exports thatts-jest's resolver doesn't follow, and tests touching theApplyView → PierreRendererimport chain (e.g.ComposerTools.test.ts) would otherwise crash at module load. Tests don't assert on Pierre rendering, so the stub returns an empty<div>.Tradeoffs