Repository navigation
feat(vscode): persist diff viewer unified/split style - #14322
Conversation
Remember the user's unified/split choice across diff viewers. Toggling the style in the full diff viewer or the lightweight diff tab writes it to kilo-code.new.diff.style; viewers open with that choice once set, and source/PR switches no longer force-reset to unified. No setting is written until the first toggle, so per-surface defaults (split for edit-tool diffs, unified for the permission dock) keep working.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (prev === undefined || id === prev) return | ||
| setComments([]) | ||
| setDiffStyle("unified") | ||
| setDiffStyle(savedDiffStyle() ?? "unified") |
There was a problem hiding this comment.
[WARNING]: savedDiffStyle is never updated when the user toggles the style, so a source/PR switch can undo a just-made choice.
The toggle handler (onDiffStyleChange, around line 434) only calls setDiffStyle(style) and posts diffViewer.setDiffStyle — it never calls setSavedDiffStyle(style). The signal is populated only by the host's diffViewer.initialDiffStyle message on webviewReady, which is sent once per panel. So on a fresh panel savedDiffStyle() is still undefined right after the user toggles to split, and switching the source (or PR identity) reaches this line and resets to savedDiffStyle() ?? "unified" — the exact force-reset this PR set out to remove. Conversely, if a style was persisted from a previous session, toggling the opposite way then switching source reverts to the stale persisted value rather than the user's latest choice. Consider also calling setSavedDiffStyle(style) in the toggle handler (the host could echo the value back for consistency).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in a21753b — the toggle handler now also updates savedDiffStyle.
|
|
||
| test("getUserDiffStyle reads the global layer, not workspace-local overrides", () => { | ||
| installConfig({ "diff.style": "split" }) | ||
| expect(getUserDiffStyle()).toBe("split") |
There was a problem hiding this comment.
[SUGGESTION]: This test does not exercise what its name claims. The name says "not workspace-local overrides", but the mock's inspect always returns { globalValue: store[key] } and never exposes a workspaceValue, so no workspace-layer value exists to be ignored. The assertion would pass even if getUserDiffStyle preferred a workspace value, making this effectively a duplicate of the second test. To actually cover the behavior, have the mock return a workspaceValue that differs from globalValue and assert the global one wins.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in a21753b — the test now mocks a differing workspaceValue and asserts the global layer wins
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Prior findings re-verified at Previous Review Summaries (3 snapshots, latest commit cb42e41)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cb42e41)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit a21753b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Previous review (commit 4fb3e2e)Status: 2 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
Track the toggle in savedDiffStyle so a source or PR switch uses the user's latest choice instead of a stale persisted value. Also make the global-layer test exercise a differing workspace value.
| test("getUserDiffStyle reads the global layer, not workspace-local overrides", () => { | ||
| // Mock a workspace-level value differing from the global one; only the | ||
| // global layer may win, matching the inspect().globalValue read. | ||
| ;(vscode.workspace as unknown as { getConfiguration: typeof vscode.workspace.getConfiguration }).getConfiguration = |
There was a problem hiding this comment.
SUGGESTION: The workspace-layer mock added here is overwritten before it can ever run.
The installConfig(...) call on the next line reassigns vscode.workspace.getConfiguration to its own mock, whose inspect only ever returns { globalValue }. So the custom mock installed at lines 51–59 is dead code and workspaceValue is still never observed by getUserDiffStyle. The test passes, but for the wrong reason: it remains a duplicate of the second test and still does not assert that workspace-local overrides are ignored, so the earlier concern is not actually resolved. Move the installConfig(...) call above this mock (or fold a differing workspaceValue into installConfig's own mock) so the workspace value is actually visible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in cb42e41 — installConfig now exposes a workspace layer via a second map, so the differing workspaceValue is actually observed by the test
installConfig now takes a separate workspace-layer map so the global- layer test's differing workspaceValue is actually observed instead of being overwritten by the helper's own mock.
marius-kilocode
left a comment
marius-kilocode
left a comment
There was a problem hiding this comment.
Thanks for the fix, this works well. I verified the persistence in an isolated dev host and adjusted the read to use the effective configuration via get(), so a workspace override applies like the other Kilo settings. Looks good to merge.
feat(vscode): persist diff viewer unified/split style
Issue
Fixes #14321
Context
The unified/split choice in Kilo's diff viewers is not persisted anywhere (#14321):
diffViewer.setDiffStylemessage when toggled (the client side was added in fix: default edit-tool diff to side-by-side (split) view #9576), but the host handler was an empty stub, so nothing was ever stored. It also force-resets to unified whenever the source or PR identity changes.The sibling toggle in the same toolbar (Markdown render) was persisted as
kilo-code.new.diff.renderMarkdownin #9846; this applies the same pattern to the style toggle.Implementation
Adds a
kilo-code.new.diff.stylesetting (unified|split):diffVirtual.setDiffStyle) or the full diff viewer (diffViewer.setDiffStyle, previously a no-op stub) writes the choice to global settings.Design note: the setting intentionally has no
default, so "never toggled" is distinguishable from "unified". The read uses the effective configuration viaget(), so a workspace override applies, matching the other Kilo settings. The per-surface defaults from #9576 (split for edit-tool diffs, unified for the permission dock) keep working until the user makes an explicit choice.Screenshots / Video
N/A — no visual layout change; the toggle already existed. (The only visible difference is
kilo-code.new.diff.styleappearing in Settings after the first toggle.)How to Test
Manual/local verification
packages/kilo-vscode/:bun run typecheck,bun run lint,bun run knip— all pass.bun test tests/unit/diff-style-persistence.test.ts— 4 new tests pass;bun test tests/unit/databridge-shape.test.ts tests/unit/diff-viewer-provider.test.ts tests/unit/extension-arch.test.ts— pass (all run by me as the PR author, on Windows 11).bun run extension:isolated): opened an edit-approval diff, toggled to split, verifiedkilo-code.new.diff.style=splitin Settings; subsequent diff viewers open split; toggling back to unified persists; clearing the setting restores the pre-PR per-surface defaults.Reviewer test steps
bun run extension:isolatedfrompackages/kilo-vscode/kilo-code.new.diff.styleis nowsplitBlocked checks and substitute verification
bun run compile/bun run packagefails at the ffmpeg-helpernpm packstep on my Windows machine — reproduced on cleanmain, unrelated to this change. Substitute: the esbuild bundle (bun run bundle) builds and contains the new handlers; package-level typecheck/lint/tests all pass.kilo-ui-contractruntime-import test failures seen locally are pre-existing on cleanmain(Windows-specific) and reproduce identically without this change.Checklist