feat(diff-view): unified changes view (split/unified toggle)#2473
feat(diff-view): unified changes view (split/unified toggle)#2473snirye wants to merge 11 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private async _fetchChanges(): Promise<GitChange[]> { | ||
| const base = this.baseRef; | ||
| if (!base) return []; | ||
| const mbResult = await rpc.workspace.git.getMergeBase(this.projectId, this.workspaceId, base); | ||
| if (!mbResult.success || !mbResult.data.sha) return []; | ||
| const result = await rpc.workspace.git.getChangedFiles( | ||
| this.projectId, | ||
| this.workspaceId, | ||
| commitRef(mbResult.data.sha) | ||
| ); | ||
| if (!result.success) return []; | ||
| return result.data.changes; | ||
| } |
There was a problem hiding this comment.
mergeBase and changes use independent, unsynchronised SHA fetches
_fetchChanges calls getMergeBase via RPC on its own (line 100), while UnifiedSection.open() reads from unified.mergeBase.data (the separate mergeBase Resource). Because the two resources run on independent refresh cycles with different debounce timers (e.g. 100 ms for HEAD vs 300 ms for index), their cached SHAs can momentarily diverge. If a user clicks a file while mergeBase.data still holds the previous SHA but changes was already rebuilt with a newer one, the diff viewer opens with an originalRef that does not match the comparison used to produce the file list — the rendered diff can show the wrong additions and deletions.
Consider returning { sha, changes } from a single fetch function and exposing one combined Resource, or making _fetchChanges read from this.mergeBase so that both consumers always use the same SHA.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/renderer/features/tasks/diff-view/stores/unified-changes-store.ts
Line: 97-109
Comment:
**`mergeBase` and `changes` use independent, unsynchronised SHA fetches**
`_fetchChanges` calls `getMergeBase` via RPC on its own (line 100), while `UnifiedSection.open()` reads from `unified.mergeBase.data` (the separate `mergeBase` Resource). Because the two resources run on independent refresh cycles with different debounce timers (e.g. 100 ms for HEAD vs 300 ms for index), their cached SHAs can momentarily diverge. If a user clicks a file while `mergeBase.data` still holds the previous SHA but `changes` was already rebuilt with a newer one, the diff viewer opens with an `originalRef` that does not match the comparison used to produce the file list — the rendered diff can show the wrong additions and deletions.
Consider returning `{ sha, changes }` from a single fetch function and exposing one combined `Resource`, or making `_fetchChanges` read from `this.mergeBase` so that both consumers always use the same SHA.
How can I resolve this? If you propose a fix, please make it concise.| this.mergeBase = new Resource<string | null>( | ||
| () => this._fetchMergeBase(), | ||
| this._statusEventStrategies('merge-base') | ||
| ); | ||
|
|
||
| this.changes = new Resource<GitChange[]>( | ||
| () => this._fetchChanges(), | ||
| this._statusEventStrategies('unified-changes') |
There was a problem hiding this comment.
mergeBase resource
Both mergeBase and changes call _statusEventStrategies(watchTag), which registers a filesystem watcher (path '', watch-tag unified-changes-merge-base and unified-changes-unified-changes respectively). The merge-base SHA can only change when HEAD or a remote ref moves — never when an ordinary working-tree file is saved. Yet the mergeBase resource is triggered and issues a getMergeBase RPC call on every non-.git file modification, doubling unnecessary round-trips on every keystroke. The mergeBase resource only needs the HEAD, index, and ref-changed event strategies; the filesystem strategy can be dropped from it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/renderer/features/tasks/diff-view/stores/unified-changes-store.ts
Line: 40-47
Comment:
**Redundant filesystem watcher registered for `mergeBase` resource**
Both `mergeBase` and `changes` call `_statusEventStrategies(watchTag)`, which registers a filesystem watcher (path `''`, watch-tag `unified-changes-merge-base` and `unified-changes-unified-changes` respectively). The merge-base SHA can only change when HEAD or a remote ref moves — never when an ordinary working-tree file is saved. Yet the `mergeBase` resource is triggered and issues a `getMergeBase` RPC call on every non-`.git` file modification, doubling unnecessary round-trips on every keystroke. The `mergeBase` resource only needs the HEAD, index, and ref-changed event strategies; the filesystem strategy can be dropped from it.
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryAdds a Unified view to the Changes panel that lists every file differing between the working tree and the merge-base of the PR base (or default branch), with a split/unified toggle persisted in app settings. The existing three-section Split view is preserved intact.
Confidence Score: 3/5The feature is largely well-structured, but The
|
| Filename | Overview |
|---|---|
| apps/emdash-desktop/src/renderer/features/tasks/diff-view/stores/unified-changes-store.ts | New store for unified changes view; has a P1 race condition where _fetchChanges re-fetches the merge base independently from the mergeBase Resource, and a P2 redundant filesystem watcher on the mergeBase resource. |
| apps/emdash-desktop/src/renderer/features/tasks/diff-view/changes-panel/unified-section.tsx | New unified section component; reads mergeBase.data for originalRef in open() while the file list is built from a separate fetch inside _fetchChanges — coherent with the P1 in the store. |
| apps/emdash-desktop/src/renderer/features/tasks/diff-view/stores/diff-view-store.ts | Wires up UnifiedChangesStore and adds 'unified' guard to activeFile override and reaction; disposal is handled correctly. |
| apps/emdash-desktop/src/renderer/features/tasks/diff-view/main-panel/diff-file-renderer.tsx | Extends disk-branch logic to also cover unified for model registration, URI resolution, and cleanup; pattern is consistent. |
| apps/emdash-desktop/src/renderer/features/tasks/diff-view/changes-panel/changes-panel.tsx | Adds split/unified toggle header and conditionally renders split or unified layout; structurally clean refactor. |
| apps/emdash-desktop/src/renderer/features/tasks/diff-view/changes-panel/hooks/use-panel-mode.ts | Thin hook persisting panel mode to app settings; uses as never cast to work around a type gap in the settings update callback. |
| apps/emdash-desktop/src/main/core/git/impl/git-service.ts | Adds getMergeBase to GitService; correctly runs git merge-base <base> HEAD and returns null on failure. |
| apps/emdash-desktop/src/main/core/git/controller.ts | New getMergeBase RPC handler; follows existing error-handling pattern with Result type and structured logging. |
| apps/emdash-desktop/src/main/core/settings/schema.ts | Adds changesPanelModeSchema ('split' |
Sequence Diagram
sequenceDiagram
participant UI as UnifiedSection
participant UCStore as UnifiedChangesStore
participant MB as mergeBase Resource
participant CH as changes Resource
participant RPC as rpc.git
Note over UI,RPC: Initial load / refresh
UCStore->>MB: start()
UCStore->>CH: start()
MB->>RPC: getMergeBase(base)
RPC-->>MB: "sha="abc123""
CH->>RPC: getMergeBase(base)
RPC-->>CH: "sha="abc123""
CH->>RPC: getChangedFiles(commitRef("abc123"))
RPC-->>CH: [file1, file2]
Note over UI,RPC: HEAD advances → both resources invalidated
MB->>RPC: getMergeBase(base)
RPC-->>MB: "sha="def456""
Note over UI: mergeBase.data = "def456"
CH->>RPC: getMergeBase(base)
Note over UI: changes still showing old list
RPC-->>CH: "sha="def456""
CH->>RPC: getChangedFiles(commitRef("def456"))
RPC-->>CH: [file1, file2, file3]
Note over UI,RPC: Race window — user clicks file3
UI->>UI: open(file3) uses mergeBase.data as originalRef
Note over UI: If mergeBase.data ≠ SHA in _fetchChanges, wrong diff shown
Comments Outside Diff (1)
-
apps/emdash-desktop/src/renderer/features/tasks/diff-view/changes-panel/hooks/use-panel-mode.ts, line 7 (link)as neversuppresses type safety on settings updateupdate(next as never)works around a typing gap inuseAppSettingsKey'supdatecallback. Casting toneverdefeats TypeScript's ability to catch mismatched setting types at call sites. The underlying issue is that the inferred type ofupdateis too narrow, sonextshould be explicitly typed to match, or the hook wrapper should be widened to accept the schema type directly.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/emdash-desktop/src/renderer/features/tasks/diff-view/changes-panel/hooks/use-panel-mode.ts Line: 7 Comment: **`as never` suppresses type safety on settings update** `update(next as never)` works around a typing gap in `useAppSettingsKey`'s `update` callback. Casting to `never` defeats TypeScript's ability to catch mismatched setting types at call sites. The underlying issue is that the inferred type of `update` is too narrow, so `next` should be explicitly typed to match, or the hook wrapper should be widened to accept the schema type directly. How can I resolve this? If you propose a fix, please make it concise.
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!
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/emdash-desktop/src/renderer/features/tasks/diff-view/stores/unified-changes-store.ts:97-109
**`mergeBase` and `changes` use independent, unsynchronised SHA fetches**
`_fetchChanges` calls `getMergeBase` via RPC on its own (line 100), while `UnifiedSection.open()` reads from `unified.mergeBase.data` (the separate `mergeBase` Resource). Because the two resources run on independent refresh cycles with different debounce timers (e.g. 100 ms for HEAD vs 300 ms for index), their cached SHAs can momentarily diverge. If a user clicks a file while `mergeBase.data` still holds the previous SHA but `changes` was already rebuilt with a newer one, the diff viewer opens with an `originalRef` that does not match the comparison used to produce the file list — the rendered diff can show the wrong additions and deletions.
Consider returning `{ sha, changes }` from a single fetch function and exposing one combined `Resource`, or making `_fetchChanges` read from `this.mergeBase` so that both consumers always use the same SHA.
### Issue 2 of 3
apps/emdash-desktop/src/renderer/features/tasks/diff-view/stores/unified-changes-store.ts:40-47
**Redundant filesystem watcher registered for `mergeBase` resource**
Both `mergeBase` and `changes` call `_statusEventStrategies(watchTag)`, which registers a filesystem watcher (path `''`, watch-tag `unified-changes-merge-base` and `unified-changes-unified-changes` respectively). The merge-base SHA can only change when HEAD or a remote ref moves — never when an ordinary working-tree file is saved. Yet the `mergeBase` resource is triggered and issues a `getMergeBase` RPC call on every non-`.git` file modification, doubling unnecessary round-trips on every keystroke. The `mergeBase` resource only needs the HEAD, index, and ref-changed event strategies; the filesystem strategy can be dropped from it.
### Issue 3 of 3
apps/emdash-desktop/src/renderer/features/tasks/diff-view/changes-panel/hooks/use-panel-mode.ts:7
**`as never` suppresses type safety on settings update**
`update(next as never)` works around a typing gap in `useAppSettingsKey`'s `update` callback. Casting to `never` defeats TypeScript's ability to catch mismatched setting types at call sites. The underlying issue is that the inferred type of `update` is too narrow, so `next` should be explicitly typed to match, or the hook wrapper should be widened to accept the schema type directly.
```suggestion
const setMode = (next: ChangesPanelMode) => update(next);
```
Reviews (1): Last reviewed commit: "chore: format unified changes view files" | Re-trigger Greptile
1 participant
Summary
Adds a Unified view to the Changes panel that lists every file differing between the working tree and the PR base (or default branch when no PR exists), under a single read-only list. The existing 3-section Split view is preserved and the user toggles between them via a new icon in the panel header. Persisted per-task via app settings.
getMergeBaseRPC + newunifieddiff group plumbed through tab manager / lifecycle / file renderer (orig = merge-base commit, modified = working-tree buffer)UnifiedChangesStorederives base ref (PR base ?? default branch), fetchesmerge-base(base, HEAD), and produces one row per file via existinggetChangedFilesAll changes (read-only)Test plan
Spec:
docs/superpowers/specs/2026-06-10-unified-changes-view-design.mdPlan:
docs/superpowers/plans/2026-06-10-unified-changes-view.md