feat(ui): add fold toggle for unchanged lines between hunks#303
feat(ui): add fold toggle for unchanged lines between hunks#303adit-chandra wants to merge 3 commits into
Conversation
adit-chandra
changed the title
adit-chandra
changed the title
adit-chandra
changed the title
adit-chandra
force-pushed
the
adit/toggle-expansion
branch
from
0e14617 to
141a8ab
Compare
Attach per-file source fetchers from explicit Git refs, index, worktree, file-pair, and untracked endpoints so expanded context never guesses from the wrong revision.
Model folded context as explicit diff rows before render planning so line gutters, hunk anchors, row windowing, and section geometry all share the same structure.
Let the review controller own gap state and source-load lifecycles so keyboard, mouse, reload invalidation, and visible loading/error rows stay in one review flow.
adit-chandra
force-pushed
the
adit/toggle-expansion
branch
from
2f61098 to
db2b84e
Compare
adit-chandra
changed the title
Greptile SummaryThis PR adds inline expansion of folded unchanged context lines between diff hunks. When a fetcher is available (all git-backed and file-pair inputs), clicking a collapsed gap or pressing
Confidence Score: 3/5The feature works end-to-end for the happy path, but a React updater purity violation in the load-settlement callback would prevent gap expansion from ever resolving if strict mode is active, and the synchronous git spawn in the fetcher will freeze the render loop on every first expansion. The review controller's setSettledStatus mutates two refs inside a state updater; React strict mode double-invokes updaters so the second call finds isCurrentRequest() returning false and leaves the loading status stuck forever. Additionally, readGitObjectSpec uses Bun.spawnSync which holds the JS thread and TUI render loop until the child exits — for a large file on spinning disk this produces a visible freeze on the first gap expand. src/ui/hooks/useReviewController.ts (setSettledStatus updater purity) and src/core/fileSource.ts (synchronous git spawn and missing gitExecutable forwarding) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant App
participant ReviewController
participant DiffPane
participant PierreDiffView
participant expandCollapsedRows
participant FileSourceFetcher
participant GitFS as Git/FS
User->>App: press e or click gap row
App->>ReviewController: toggleSelectedHunkGap or toggleGap
ReviewController->>ReviewController: add gapKey to expandedGapsByFileId
ReviewController->>ReviewController: set sourceStatus loading
ReviewController->>FileSourceFetcher: getFullText(side)
FileSourceFetcher->>GitFS: Bun.spawnSync or Bun.file blocking
GitFS-->>FileSourceFetcher: raw text
FileSourceFetcher-->>ReviewController: text or null
ReviewController->>ReviewController: set sourceStatus loaded
ReviewController-->>DiffPane: expandedGapsByFileId sourceStatusByFileId
DiffPane-->>PierreDiffView: expandedGapKeys sourceStatus
PierreDiffView->>expandCollapsedRows: expandCollapsedRows rows options
expandCollapsedRows-->>PierreDiffView: expanded DiffRow array
PierreDiffView->>PierreDiffView: useHighlightedSource async syntax highlight
PierreDiffView-->>User: renders expanded context rows with syntax highlighting
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
src/ui/hooks/useReviewController.ts:378-392
**Side effect inside state updater breaks in strict mode**
`sourceLoadRequestsRef.current.delete(fileId)` and the `sourceStatusRef.current` mutation are side effects placed inside a `setSourceStatusByFileId` updater function. React calls state updater functions twice in strict mode to detect impurity. On the second invocation, `isCurrentRequest()` returns `false` (the entry was deleted on the first run), so the updater returns `prev` unchanged — meaning the "loading" status never transitions to "loaded" or "error", leaving the gap permanently stuck in the loading state. The fix is to perform the ref mutations outside the updater and only use the updater for the `sourceStatusByFileId` change itself.
### Issue 2 of 4
src/core/fileSource.ts:86-112
**`gitExecutable` not threaded through the source fetcher contract**
`readGitBlobSpec` and `readGitIndexSpec` both accept a `gitExecutable` parameter, but `readSpec` always calls them with the default `"git"`. This means the custom git executable configured via `RunGitTextOptions.gitExecutable` throughout the rest of the codebase is silently ignored for source fetching. In environments where git is at a non-standard path, every gap expansion backed by a git blob spec will fail with a spawn error and show "Could not load N unchanged lines" instead of the actual content.
### Issue 3 of 4
src/core/fileSource.ts:89-112
**Synchronous `Bun.spawnSync` blocks the JS thread inside an async method**
`readGitBlobSpec` and `readGitIndexSpec` call `readGitObjectSpec`, which uses `Bun.spawnSync`. Because `readSpec` is `async` but has no `await` before these calls, the spawn runs synchronously on the microtask that resolves the `getFullText` promise. This holds the JS event loop — and thus the TUI render loop — until the child process exits. For large files or a slow git index on spinning disk this will cause a visible freeze when the user first expands a gap.
### Issue 4 of 4
src/ui/diff/expandCollapsedRows.ts:163-173
**`lineCount` derived from range but source is sliced with zero-based index**
If the fetched source text has fewer lines than `range[1]` implies, the loop silently produces blank expansion rows for out-of-range indices. A guard like `if (sourceLineNumber >= sourceLines.length) break` would make this failure mode more intentional.
Reviews (1): Last reviewed commit: "feat(review): toggle unchanged context i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!isCurrentRequest()) { | ||
| return prev; | ||
| } | ||
| sourceLoadRequestsRef.current.delete(fileId); | ||
| sourceStatusRef.current = { ...sourceStatusRef.current, [fileId]: nextStatus }; | ||
| return { | ||
| ...prev, | ||
| [fileId]: nextStatus, | ||
| }; | ||
| }); | ||
| }; | ||
|
|
||
| void file.sourceFetcher | ||
| .getFullText(side) | ||
| .then((text) => { |
There was a problem hiding this comment.
sourceLoadRequestsRef.current.delete(fileId) and the sourceStatusRef.current mutation are side effects placed inside a setSourceStatusByFileId updater function. React calls state updater functions twice in strict mode to detect impurity. On the second invocation, isCurrentRequest() returns false (the entry was deleted on the first run), so the updater returns prev unchanged — meaning the "loading" status never transitions to "loaded" or "error", leaving the gap permanently stuck in the loading state. The fix is to perform the ref mutations outside the updater and only use the updater for the sourceStatusByFileId change itself.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/hooks/useReviewController.ts
Line: 378-392
Comment:
**Side effect inside state updater breaks in strict mode**
`sourceLoadRequestsRef.current.delete(fileId)` and the `sourceStatusRef.current` mutation are side effects placed inside a `setSourceStatusByFileId` updater function. React calls state updater functions twice in strict mode to detect impurity. On the second invocation, `isCurrentRequest()` returns `false` (the entry was deleted on the first run), so the updater returns `prev` unchanged — meaning the "loading" status never transitions to "loaded" or "error", leaving the gap permanently stuck in the loading state. The fix is to perform the ref mutations outside the updater and only use the updater for the `sourceStatusByFileId` change itself.
How can I resolve this? If you propose a fix, please make it concise.| gitExecutable = "git", | ||
| ): string | null { | ||
| return readGitObjectSpec(spec.repoRoot, `:${spec.path}`, gitExecutable); | ||
| } | ||
|
|
||
| /** Read a blob-like Git object spec such as `HEAD:path` or `:path`. */ | ||
| function readGitObjectSpec( | ||
| repoRoot: string, | ||
| objectName: string, | ||
| gitExecutable = "git", | ||
| ): string | null { | ||
| let proc: ReturnType<typeof Bun.spawnSync>; | ||
|
|
||
| try { | ||
| proc = Bun.spawnSync([gitExecutable, "show", objectName], { | ||
| cwd: repoRoot, | ||
| stdin: "ignore", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| } catch (error) { | ||
| logSourceDiagnostic(`failed to run Git while reading source ${objectName}`, error); | ||
| return null; | ||
| } | ||
|
|
||
| if (proc.exitCode !== 0) { | ||
| const stderr = Buffer.from(proc.stderr ?? []).toString("utf8"); |
There was a problem hiding this comment.
gitExecutable not threaded through the source fetcher contract
readGitBlobSpec and readGitIndexSpec both accept a gitExecutable parameter, but readSpec always calls them with the default "git". This means the custom git executable configured via RunGitTextOptions.gitExecutable throughout the rest of the codebase is silently ignored for source fetching. In environments where git is at a non-standard path, every gap expansion backed by a git blob spec will fail with a spawn error and show "Could not load N unchanged lines" instead of the actual content.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/fileSource.ts
Line: 86-112
Comment:
**`gitExecutable` not threaded through the source fetcher contract**
`readGitBlobSpec` and `readGitIndexSpec` both accept a `gitExecutable` parameter, but `readSpec` always calls them with the default `"git"`. This means the custom git executable configured via `RunGitTextOptions.gitExecutable` throughout the rest of the codebase is silently ignored for source fetching. In environments where git is at a non-standard path, every gap expansion backed by a git blob spec will fail with a spawn error and show "Could not load N unchanged lines" instead of the actual content.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| /** Read a blob-like Git object spec such as `HEAD:path` or `:path`. */ | ||
| function readGitObjectSpec( | ||
| repoRoot: string, | ||
| objectName: string, | ||
| gitExecutable = "git", | ||
| ): string | null { | ||
| let proc: ReturnType<typeof Bun.spawnSync>; | ||
|
|
||
| try { | ||
| proc = Bun.spawnSync([gitExecutable, "show", objectName], { | ||
| cwd: repoRoot, | ||
| stdin: "ignore", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| } catch (error) { | ||
| logSourceDiagnostic(`failed to run Git while reading source ${objectName}`, error); | ||
| return null; | ||
| } | ||
|
|
||
| if (proc.exitCode !== 0) { | ||
| const stderr = Buffer.from(proc.stderr ?? []).toString("utf8"); |
There was a problem hiding this comment.
Bun.spawnSync blocks the JS thread inside an async method
readGitBlobSpec and readGitIndexSpec call readGitObjectSpec, which uses Bun.spawnSync. Because readSpec is async but has no await before these calls, the spawn runs synchronously on the microtask that resolves the getFullText promise. This holds the JS event loop — and thus the TUI render loop — until the child process exits. For large files or a slow git index on spinning disk this will cause a visible freeze when the user first expands a gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/fileSource.ts
Line: 89-112
Comment:
**Synchronous `Bun.spawnSync` blocks the JS thread inside an async method**
`readGitBlobSpec` and `readGitIndexSpec` call `readGitObjectSpec`, which uses `Bun.spawnSync`. Because `readSpec` is `async` but has no `await` before these calls, the spawn runs synchronously on the microtask that resolves the `getFullText` promise. This holds the JS event loop — and thus the TUI render loop — until the child process exits. For large files or a slow git index on spinning disk this will cause a visible freeze when the user first expands a gap.
How can I resolve this? If you propose a fix, please make it concise.| result.push(row); | ||
| continue; | ||
| } | ||
|
|
||
| const key = gapKey(row.position, row.hunkIndex); | ||
| if (!expandedKeys.has(key)) { | ||
| result.push(row); | ||
| continue; | ||
| } | ||
|
|
||
| const range = side === "old" ? row.oldRange : row.newRange; |
There was a problem hiding this comment.
lineCount derived from range but source is sliced with zero-based index
If the fetched source text has fewer lines than range[1] implies, the loop silently produces blank expansion rows for out-of-range indices. A guard like if (sourceLineNumber >= sourceLines.length) break would make this failure mode more intentional.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/expandCollapsedRows.ts
Line: 163-173
Comment:
**`lineCount` derived from range but source is sliced with zero-based index**
If the fetched source text has fewer lines than `range[1]` implies, the loop silently produces blank expansion rows for out-of-range indices. A guard like `if (sourceLineNumber >= sourceLines.length) break` would make this failure mode more intentional.
How can I resolve this? If you propose a fix, please make it concise.1 participant
problem
I often want to inspect surrounding unchanged lines while reviewing diffs to better understand changes in context.
Common editors, review tools etc, including GitHub + Codex, already support expanding folded context inline.
This is the main missing feature preventing me from switching to Hunk as my primary code review tool.
solution + design
test plan
Added or updated coverage for:
Commands run:
bun run typecheckbun run formatbun run format:checkbun run lintgit diff --check origin/main...HEADgit diff --check && git diff --cached --checkbun test src/ui/diff/pierre.test.ts src/ui/diff/expandCollapsedRows.test.ts src/ui/components/ui-components.test.tsxbun test src/ui/hooks/useReviewController.test.tsx src/ui/components/ui-components.test.tsx src/core/git.test.ts src/core/fileSource.test.ts src/ui/diff/codeColumns.test.ts src/ui/diff/expandCollapsedRows.test.ts src/ui/lib/diffSectionGeometry.test.tsbun test src/core/loaders.test.ts -t "loads staged new files before the first commit|staged diffs against an explicit ref fetch old source from that ref|file-pair diffs attach a fetcher|git working-tree diffs read the new side|unstaged working-tree diffs read old source|hunk show <ref>|hunk stash show|untracked files attach a fetcher|deleted files attach a fetcher|renamed files read the old side|raw patch input does not attach"src/core/loaders.test.ts; only the two upstream JJ loader tests failed because this machine does not havejjonPATH.demo:
demo.mov