fix(pager): render static diffs in captured pagers#271
Conversation
Greptile SummaryThis PR adds a static ANSI diff renderer for
Confidence Score: 4/5Safe to merge. The new rendering path cannot corrupt or lose data — it falls back to raw text on any failure — and the TTY detection logic is guarded by tests covering all three new branches. The core change is well-tested and the fallback design means a rendering bug produces at worst uncolored diff output. The two findings are non-blocking: the silent catch makes render failures invisible to developers, and the TERM=dumb heuristic could emit ANSI codes on genuine dumb terminals, though the previous behavior (launching the full TUI) was already broken in that scenario. src/ui/staticDiffPager.ts (error observability) and src/core/startup.ts (TERM=dumb breadth). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[hunk pager called] --> B{stdin looks like a patch?}
B -- No --> C[plain-text-pager\nTUI plain text viewer]
B -- Yes --> D{stdoutIsTTY?}
D -- No --> E[passthrough\nwrite stdin to stdout]
D -- Yes --> F{TERM=dumb?}
F -- Yes --> G[static-diff-pager\nANSI colored diff]
F -- No --> H{controllingTerminal\navailable?}
H -- No --> G
H -- Yes --> I[app\nlaunch interactive TUI]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/ui/staticDiffPager.ts:161-182
**Silent catch swallows all render errors**
The bare `catch` at the bottom discards every possible error — including unexpected programming bugs, type mismatches, or broken theme lookups — and returns the original `text` with no diagnostic output. A user whose render silently fails sees the raw diff instead of a colored one and has no indication that something went wrong; a developer can't tell whether the fallback was triggered by an expected parse failure or an unintended regression.
Consider emitting at least a one-line warning to `process.stderr` (or re-exporting the caught value for test inspection) so the fallback path is observable without changing the user-visible behavior.
### Issue 2 of 2
src/core/startup.ts:117-125
**`TERM=dumb` heuristic can misfire on genuine dumb terminals**
The comment says this targets captured pager hosts like LazyGit, but `TERM=dumb` is a standard signal that the terminal does not support ANSI escape codes at all (e.g., a serial console, a minimal SSH session, or certain CI runners). When hunk is configured as a git pager on such a system with `stdoutIsTTY=true`, this branch emits ANSI truecolor codes to a terminal that explicitly declared it cannot render them, producing visible escape sequence garbage. Before this PR the same scenario would have attempted a full TUI, so this is still an improvement — but a narrower condition (e.g., also checking for a `LAZYGIT=1` env var or a well-known LazyGit variable) would be safer.
Reviews (1): Last reviewed commit: "fix(pager): render static diffs in captu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Render diff-like pager stdin as colored static output, falling back to the original patch on failure. */ | ||
| export async function renderStaticDiffPager(text: string, options: CommonOptions = {}) { | ||
| try { | ||
| const bootstrap = await loadAppBootstrap({ | ||
| kind: "patch", | ||
| file: "-", | ||
| text, | ||
| options: { | ||
| ...options, | ||
| pager: true, | ||
| }, | ||
| }); | ||
| const theme = resolveTheme(options.theme, null); | ||
| const rendered = await Promise.all( | ||
| bootstrap.changeset.files.map((file) => renderStaticFile(file, theme)), | ||
| ); | ||
|
|
||
| return rendered.length > 0 ? `${rendered.join("\n\n")}\n` : text; | ||
| } catch { | ||
| return text; | ||
| } | ||
| } |
There was a problem hiding this comment.
The bare catch at the bottom discards every possible error — including unexpected programming bugs, type mismatches, or broken theme lookups — and returns the original text with no diagnostic output. A user whose render silently fails sees the raw diff instead of a colored one and has no indication that something went wrong; a developer can't tell whether the fallback was triggered by an expected parse failure or an unintended regression.
Consider emitting at least a one-line warning to process.stderr (or re-exporting the caught value for test inspection) so the fallback path is observable without changing the user-visible behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/staticDiffPager.ts
Line: 161-182
Comment:
**Silent catch swallows all render errors**
The bare `catch` at the bottom discards every possible error — including unexpected programming bugs, type mismatches, or broken theme lookups — and returns the original `text` with no diagnostic output. A user whose render silently fails sees the raw diff instead of a colored one and has no indication that something went wrong; a developer can't tell whether the fallback was triggered by an expected parse failure or an unintended regression.
Consider emitting at least a one-line warning to `process.stderr` (or re-exporting the caught value for test inspection) so the fallback path is observable without changing the user-visible behavior.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Added an observable fallback diagnostic. renderStaticDiffPager now writes a one-line warning before returning the raw diff when static rendering fails or renders no files, and the fallback test asserts that diagnostic.
This comment was generated by Pi using OpenAI GPT-5
| // Captured pager hosts like LazyGit can provide a PTY while advertising TERM=dumb. | ||
| // In that mode, emit static colored diff output instead of launching the TUI. | ||
| if (env.TERM === "dumb") { | ||
| return { | ||
| kind: "static-diff-pager", | ||
| text: stdinText, | ||
| options: parsedCliInput.options, | ||
| }; | ||
| } |
There was a problem hiding this comment.
TERM=dumb heuristic can misfire on genuine dumb terminals
The comment says this targets captured pager hosts like LazyGit, but TERM=dumb is a standard signal that the terminal does not support ANSI escape codes at all (e.g., a serial console, a minimal SSH session, or certain CI runners). When hunk is configured as a git pager on such a system with stdoutIsTTY=true, this branch emits ANSI truecolor codes to a terminal that explicitly declared it cannot render them, producing visible escape sequence garbage. Before this PR the same scenario would have attempted a full TUI, so this is still an improvement — but a narrower condition (e.g., also checking for a LAZYGIT=1 env var or a well-known LazyGit variable) would be safer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/startup.ts
Line: 117-125
Comment:
**`TERM=dumb` heuristic can misfire on genuine dumb terminals**
The comment says this targets captured pager hosts like LazyGit, but `TERM=dumb` is a standard signal that the terminal does not support ANSI escape codes at all (e.g., a serial console, a minimal SSH session, or certain CI runners). When hunk is configured as a git pager on such a system with `stdoutIsTTY=true`, this branch emits ANSI truecolor codes to a terminal that explicitly declared it cannot render them, producing visible escape sequence garbage. Before this PR the same scenario would have attempted a full TUI, so this is still an improvement — but a narrower condition (e.g., also checking for a `LAZYGIT=1` env var or a well-known LazyGit variable) would be safer.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Narrowed this so a plain TERM=dumb terminal now gets raw passthrough rather than ANSI static output. The static path is limited to likely captured pager hosts (for example LazyGit-style pager env) or the separate no-controlling-terminal fallback, with a regression test for the plain dumb terminal case.
This comment was generated by Pi using OpenAI GPT-5
1 participant
Summary
hunk pagerwhen it is launched by captured pager hosts such as LazyGit.TERM=dumbterminals.Verification
bun test src/ui/staticDiffPager.test.ts src/core/startup.test.ts test/cli/entrypoint.test.tsbun run typecheckgit.pagers[].pager = "bun /home/bentlegen/Projects/hunk-b/src/main.tsx pager"This PR description was generated by Pi using OpenAI GPT-5