Skip to content

feat(ui): add fold toggle for unchanged lines between hunks#303

Open
adit-chandra wants to merge 4 commits into
modem-dev:mainfrom
adit-chandra:adit/toggle-expansion
Open

feat(ui): add fold toggle for unchanged lines between hunks#303
adit-chandra wants to merge 4 commits into
modem-dev:mainfrom
adit-chandra:adit/toggle-expansion

Conversation

@adit-chandra
Copy link
Copy Markdown

@adit-chandra adit-chandra commented May 13, 2026

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

  • Expand folded context in place, not in a separate source view, so reviewers stay in the same file/hunk flow with agent notes, sticky headers, and scroll behavior intact.
  • Treat full source text as a lazy per-file capability. Loaders attach exact old/new source endpoints only when they can identify the correct Git ref, index, worktree, file-pair, difftool, or untracked source.
  • Avoid guessing. Raw patches and unsupported Git range shapes get no expander rather than risking context from the wrong revision.
  • Convert expanded context into normal diff rows before render planning, so rendering, row windowing, gutter width, geometry, and mouse/keyboard interaction all use the same row model.
  • Reuse Pierre/Shiki highlighting for loaded source lines, so expanded unchanged rows match the surrounding diff instead of rendering as plain text.
  • Let the review controller own expansion state and source request lifecycles, which gives one place to invalidate soft reloads, ignore stale async completions, and show explicit error rows.
  • Suggested review order: core source endpoints, then row synthesis/geometry, then controller/UI wiring.

test plan

Added or updated coverage for:

  • Source resolution: Git refs, index/worktree sides, file-pair diffs, difftool, show/stash, untracked files, deleted/renamed files, and staged additions before the first commit.
  • Source failure handling: missing source, unexpected Git/read failures, caching, rejected fetchers, and visible error state.
  • Row expansion: split/stack synthesized rows, trailing and leading gaps, deleted-file old-side expansion, CRLF/tab handling, syntax-aware spans, and keyboard gap selection.
  • Layout/rendering: expanded rows affecting section height, later file placement, line-number gutter width, row geometry, clickable gap toggles, and syntax highlighting for expanded unchanged lines.
  • Controller behavior: lazy load status, no-op without fetcher, reload invalidation, and stale pending source loads after soft reload.
  • Shared fixtures: source fetcher and deferred-promise helpers to keep controller/component tests shorter and easier to scan.

Commands run:

  • Passed: bun run typecheck
  • Passed: bun run format
  • Passed: bun run format:check
  • Passed: bun run lint
  • Passed: git diff --check origin/main...HEAD
  • Passed: git diff --check && git diff --cached --check
  • Passed: bun test src/ui/diff/pierre.test.ts src/ui/diff/expandCollapsedRows.test.ts src/ui/components/ui-components.test.tsx
  • Passed: bun 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.ts
  • Passed: bun 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"
  • Attempted: broader affected run including full src/core/loaders.test.ts; only the two upstream JJ loader tests failed because this machine does not have jj on PATH.

demo:

demo.mov

@adit-chandra adit-chandra changed the title feat(review): expand unchanged context between hunks add expand toggle for folded unchanged lines between hunks May 13, 2026
@adit-chandra adit-chandra changed the title add expand toggle for folded unchanged lines between hunks feat (ui): add expand toggle for folded unchanged lines between hunks May 13, 2026
@adit-chandra adit-chandra changed the title feat (ui): add expand toggle for folded unchanged lines between hunks feat(ui): add expand toggle for folded unchanged lines between hunks May 13, 2026
@adit-chandra adit-chandra force-pushed the adit/toggle-expansion branch 2 times, most recently from 2f61098 to db2b84e Compare May 13, 2026 06:17
@adit-chandra adit-chandra marked this pull request as ready for review May 13, 2026 21:13
@adit-chandra adit-chandra changed the title feat(ui): add expand toggle for folded unchanged lines between hunks feat(ui): add fold toggle for unchanged lines between hunks May 13, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 13, 2026

Greptile Summary

This 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 e loads the full file source, slices the covered line range, and inserts synthesized context rows into the normal diff row stream so all existing rendering, geometry, windowing, and keyboard behaviour continues to work unmodified.

  • Core plumbing (fileSource.ts, git.ts, loaders.ts): a new FileSourceFetcher contract is attached to each DiffFile; loaders resolve the correct old/new git endpoints and build fetchers for every supported input mode, with raw patches deliberately left without a fetcher.
  • Row synthesis (expandCollapsedRows.ts, pierre.ts): collapsed rows now carry 1-based line ranges and a position field; expanded gaps are materialised as ordinary context rows with isExpansionRow so navigation anchoring and hunk-bounds measurement skip them.
  • Controller & UI wiring (useReviewController.ts, PierreDiffView.tsx): expansion state and load status are owned by the review controller; syntax highlighting reuses Pierre/Shiki via the new useHighlightedSource hook.

Confidence Score: 3/5

The 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

Filename Overview
src/ui/hooks/useReviewController.ts Adds gap expansion state, source load request lifecycle, and toggleGap/toggleSelectedHunkGap callbacks; contains a latent strict-mode correctness issue where ref mutations inside a state updater can prevent load-settled transitions.
src/core/fileSource.ts New module providing FileSourceFetcher and spec-based resolution; git specs use Bun.spawnSync (blocking) and do not forward the configurable gitExecutable from the rest of the codebase.
src/core/git.ts Adds resolveGitDiffEndpoints and resolveGitCommitRef; edge cases (empty repo, symmetric ranges, octopus merges) are handled correctly.
src/core/loaders.ts Wires source fetcher builders into all load paths; raw patch inputs correctly receive no fetcher.
src/ui/diff/expandCollapsedRows.ts New module synthesizing context rows from loaded source text; handles loading/error/loaded states and CRLF normalization. Out-of-bounds source lines silently produce blank rows.
src/ui/diff/pierre.ts Adds position/range fields to collapsed rows, generalizes queueHighlightedWork, and adds loadHighlightedSourceLines. Clean and backward-compatible.
src/ui/diff/reviewRenderPlan.ts Simplifies diffRowStableKeys using the new position field and adds isExpansionRow guard to rowCanAnchorHunk.
src/ui/diff/useHighlightedSource.ts New hook for lazily loading and caching syntax highlights for expanded source lines.
src/ui/lib/diffSectionGeometry.ts Threads expansion state through geometry measurement and extends the cache key with an expansion suffix.
src/ui/diff/PierreDiffView.tsx Composes expandCollapsedRows into the render pipeline; gapToggleHandler correctly suppresses the toggle when no source fetcher exists.

Sequence Diagram

sequenceDiagram
    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
Loading
Prompt To Fix All With AI
Fix 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

Comment thread src/ui/hooks/useReviewController.ts Outdated
Comment thread src/core/fileSource.ts Outdated
Comment thread src/core/fileSource.ts Outdated
Comment thread src/ui/diff/expandCollapsedRows.ts
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 adit-chandra force-pushed the adit/toggle-expansion branch from b0b933a to 952f1d8 Compare May 14, 2026 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant