Repository navigation
feat(log): review contiguous commit ranges - #1031
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds contiguous multi-commit selection to interactive Git and Jujutsu history, introduces provider-owned range planning in extension API version 21, and routes selected ranges into embedded reviews.
Confidence Score: 3/5The PR is not yet safe to merge because opening ranges can freeze the interactive UI during uncancellable provider queries, and filtered histories can open a broader commit span than the selection reports. Two concrete range-review failures remain: synchronous Git/JJ planning blocks the event loop and cancellation, while displayed-row counting does not account for commits omitted by active history filters. Files Needing Attention: packages/hunk-git/src/history.ts, packages/hunk-jj/src/history.ts, packages/hunk/src/ui/log/controller.ts Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant Log as LogApp / LogController
participant Host as HunkSessionHost
participant Provider as Git or JJ range planner
participant Review as Embedded review
User->>Log: Extend selection
Log->>Log: Derive displayed endpoints and count
User->>Log: Open selection
Log->>Host: selection, count, optional parent
Host->>Provider: planRangeReview(endpoints)
Provider-->>Host: revision-range action
Host->>Review: Prepare parent-to-newest diff
Review-->>User: Render cumulative changes
Prompt To Fix All With AI### Issue 1
packages/hunk-git/src/history.ts:76
**Range planning blocks cancellation**
Opening a range runs `git merge-base` synchronously on the renderer thread; the JJ planner also uses `Bun.spawnSync`. The preparation signal is checked only after planning returns, so a slow or stuck VCS query freezes the history UI and prevents quit or cancellation from taking effect. These queries need to use the existing asynchronous, signal-aware provider command path.
### Issue 2
packages/hunk/src/ui/log/controller.ts:392-401
**Filtered ranges hide commits**
The selection count comes from adjacent displayed rows, but filtered histories can omit intermediate commits. Git and JJ only verify that the endpoints share ancestry, so selecting two adjacent filtered results can show “2 commits selected” while the resulting diff includes changes from hidden commits between them. Range selection should either reject filtered gaps or clearly represent the full ancestry span being reviewed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(log): review contiguous commit rang..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ): ExtensionVcsHistoryRangeReviewAction { | ||
| const newest = requireRevision(selection.newestCommit.revisionId); | ||
| const oldest = requireRevision(selection.oldestCommit.revisionId); | ||
| const ancestry = runGitResult(["merge-base", "--is-ancestor", oldest, newest], options, [0, 1]); |
There was a problem hiding this comment.
Opening a range runs git merge-base synchronously on the renderer thread; the JJ planner also uses Bun.spawnSync. The preparation signal is checked only after planning returns, so a slow or stuck VCS query freezes the history UI and prevents quit or cancellation from taking effect. These queries need to use the existing asynchronous, signal-aware provider command path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk-git/src/history.ts
Line: 76
Comment:
**Range planning blocks cancellation**
Opening a range runs `git merge-base` synchronously on the renderer thread; the JJ planner also uses `Bun.spawnSync`. The preparation signal is checked only after planning returns, so a slow or stuck VCS query freezes the history UI and prevents quit or cancellation from taking effect. These queries need to use the existing asynchronous, signal-aware provider command path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed. Git and Jujutsu range planning now use the shared asynchronous, signal-aware provider command runner. The history host forwards its preparation signal into provider planning, cancellation terminates the owned process tree, and provider plus host regression tests cover the path.
This comment was generated by Pi using gpt-5.6-sol
| const anchor = this.snapshot.selectionAnchor ?? this.snapshot.selected; | ||
| const newestIndex = Math.min(anchor, this.snapshot.selected); | ||
| const oldestIndex = Math.max(anchor, this.snapshot.selected); | ||
| return { | ||
| focus, | ||
| newest: this.snapshot.rows[newestIndex]!, | ||
| oldest: this.snapshot.rows[oldestIndex]!, | ||
| newestIndex, | ||
| oldestIndex, | ||
| count: oldestIndex - newestIndex + 1, |
There was a problem hiding this comment.
The selection count comes from adjacent displayed rows, but filtered histories can omit intermediate commits. Git and JJ only verify that the endpoints share ancestry, so selecting two adjacent filtered results can show “2 commits selected” while the resulting diff includes changes from hidden commits between them. Range selection should either reject filtered gaps or clearly represent the full ancestry span being reviewed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/ui/log/controller.ts
Line: 392-401
Comment:
**Filtered ranges hide commits**
The selection count comes from adjacent displayed rows, but filtered histories can omit intermediate commits. Git and JJ only verify that the endpoints share ancestry, so selecting two adjacent filtered results can show “2 commits selected” while the resulting diff includes changes from hidden commits between them. Range selection should either reject filtered gaps or clearly represent the full ancestry span being reviewed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed conservatively. Multi-commit selection is now refused whenever author, message, date, or path traversal filters can omit intermediate commits, with a clear in-app notice. Unfiltered ranges retain their existing behavior, and controller coverage verifies that filtered ranges cannot be formed.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
Follow-up from the independent review: --all is now covered by the same refusal because multiple-head topology can interleave unrelated commits between selected endpoints. Documentation and regression coverage were updated accordingly.
This comment was generated by Pi using gpt-5.6-sol
ff97b44 to
347d14d
Compare
347d14d to
89c037d
Compare
This branch was successfully deployed
1 participant
Summary
hunk logSemantics
The selected range compares the chosen parent (or empty/root baseline) of the oldest commit directly with the newest commit. Git never uses triple-dot semantics. Non-ancestral endpoint selections are rejected clearly.
Stack
Merge this PR before #1032.
Validation
bun run typecheckbun run deps:checkbun run test(2243 passed, 9 skipped on the complete stack)bun run test:integration(155 passed, 1 skipped)Not tested on Windows or in a physical SSH/Mosh session.
This PR description was generated by Pi using gpt-5.6-sol