ENG-1118: Review individual commits in PR sidebar#2136
Conversation
Greptile SummaryThis PR adds expandable commit rows to the PR sidebar, showing per-commit file diffs that open in the existing Monaco diff viewer. It generalises the
Confidence Score: 5/5Safe to merge; the ref-generalisation is applied consistently across the diff stack and the new tab-lookup logic correctly scopes commit diffs by value-compared refs. All changed code paths fall back correctly to No files require special attention beyond the minor suggestions already noted.
|
| Filename | Overview |
|---|---|
| src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/commits-list.tsx | Adds expandable commit rows with file lists; replaces virtualizer with flat map and toggle-expand state. useMemo uses the full commit object as a dependency, causing originalRef/modifiedRef to be recreated every time a new page is fetched. |
| src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/use-commit-files.ts | New hook that fetches changed files for a commit; projectId is missing from the query key (previously flagged). |
| src/renderer/features/tasks/tabs/tab-manager-store.ts | Replaces path+group-only tab deduplication with ref-aware lookup (_findDiffEntry), correctly scoping commit diffs by both originalRef and modifiedRef using value-based refsEqual comparisons. |
| src/renderer/features/tasks/diff-view/main-panel/stacked-diff-view.tsx | Propagates modifiedRef for 'git' group slots; contains a redundant ternary inside StackedFileSlot whose false branch is unreachable dead code. |
| src/renderer/features/tasks/diff-view/stores/stacked-diff-panel-store.ts | Makes modifiedUri for 'git' group use this.modifiedRef ?? HEAD_REF instead of hard-coded HEAD_REF, enabling commit-scoped diffs while preserving working-tree behaviour via the fallback. |
| src/renderer/features/tasks/diff-view/changes-panel/hooks/use-prefetch-diff-models.ts | Extends the effectiveModifiedRef shortcut from 'pr'-only to both 'git' and 'pr' groups; logic is consistent with the rest of the PR. |
| src/shared/git.ts | Tightens CommitFile.status from string to GitChangeStatus — a type-safety improvement with no runtime impact. |
| src/shared/view-state.ts | Updates modifiedRef JSDoc to reflect that it now applies to both 'git' and 'pr' groups. |
| src/main/core/git/impl/git-service.ts | Tightens statusMap type from Map<string, string> to Map<string, GitChangeStatus>, consistent with the shared-type change. |
Sequence Diagram
sequenceDiagram
participant Sidebar as PrCommitsList
participant CommitItem
participant FilesList as CommitFilesList
participant Hook as useCommitFiles
participant Prefetch as usePrefetchDiffModels
participant TabMgr as TabManagerStore
participant Renderer as diff-file-renderer
Sidebar->>CommitItem: render commit row
CommitItem->>CommitItem: toggle expandedHashes on click
CommitItem->>FilesList: "mount (isExpanded=true)"
FilesList->>Hook: useCommitFiles(projectId, workspaceId, hash)
Hook-->>FilesList: GitChange[]
FilesList->>Prefetch: usePrefetchDiffModels('git', commitRef(hash^), commitRef(hash))
Note over FilesList: originalRef = commitRef("hash^")<br/>modifiedRef = commitRef("hash")
FilesList->>TabMgr: "openDiffPreview({group:'git', originalRef, modifiedRef})"
TabMgr->>TabMgr: _findDiffEntry — match path + group + refsEqual(originalRef) + optionalRefsEqual(modifiedRef)
TabMgr-->>Renderer: activate DiffTabStore
Renderer->>Renderer: "modifiedUri = toGitUri(uri, tab.modifiedRef ?? HEAD_REF)"
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/renderer/features/tasks/diff-view/main-panel/stacked-diff-view.tsx:183-185
The inner ternary's condition (`diffType === 'git' || diffType === 'pr'`) is always `true` inside the enclosing `else if (diffType === 'git' || diffType === 'pr')` block, so the `: HEAD_REF` branch is dead code. Simplifying to `modifiedRef ?? HEAD_REF` removes the redundancy.
```suggestion
} else if (diffType === 'git' || diffType === 'pr') {
const effectiveModifiedRef = modifiedRef ?? HEAD_REF;
```
### Issue 2 of 2
src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/commits-list.tsx:149-150
`data.pages.flat()` rebuilds the entire `commits` array on every page fetch, giving each element a fresh object identity even when its content is unchanged. Because `useMemo` compares dependencies by reference, `originalRef` and `modifiedRef` are recreated for every expanded `CommitFilesList` on each page load, which in turn invalidates the `useCallback` in `usePrefetchDiffModels`. Since both computations only read `commit.hash`, using that as the sole dependency keeps the memoised values stable across pagination.
```suggestion
const originalRef = useMemo(() => parentRefForCommit(commit), [commit.hash]);
const modifiedRef = useMemo(() => commitRefForCommit(commit), [commit.hash]);
```
Reviews (2): Last reviewed commit: "ENG-1118: Review individual commits in P..." | Re-trigger Greptile
050c6a4 to
721147a
Compare
|
@greptileai @greptile |
|
Tested the updated comment scoping with a PR that has two commits touching the same file: I added one draft line comment in each commit diff, then injected the comments into the agent context. This shows each comment is attached to the specific commit diff where it was written, even though both comments are on the same file path. <user_comments>
<target kind="commit" originalSha="e229a6d960ec82b3f9eb29dbfe50ab2a4b00a509"
modifiedSha="12e6cd36c123ed61268e546c1ba8215584dfe404"
path="src/renderer/features/command-palette/command-palette-modal.tsx">
<comment line="1">test</comment>
</target>
<target kind="commit" originalSha="93ed50e064ce6573b3d946da0af588c1db2bf2c2"
modifiedSha="ef44918580cf4e14d84ddcbbb70ca8636ba62023"
path="src/renderer/features/command-palette/command-palette-modal.tsx">
<comment line="3">test</comment>
</target>
</user_comments> |
|
lfg |
Summary
Validation