Skip to content

fix(pager): avoid less in captured text pagers#362

Merged
benvinegar merged 2 commits into
mainfrom
fix/lazygit-non-diff-pager-passthrough
May 24, 2026
Merged

fix(pager): avoid less in captured text pagers#362
benvinegar merged 2 commits into
mainfrom
fix/lazygit-non-diff-pager-passthrough

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • pass non-diff hunk pager input through when running under captured pager hosts like LazyGit
  • also avoid launching a text pager for non-diff input when TERM=dumb
  • add startup-plan regression tests and changelog entry

Fixes #337

Testing

  • bun run typecheck
  • bun test src/core/startup.test.ts src/core/pager.test.ts test/cli/entrypoint.test.ts
  • Manual LazyGit PTY smoke test with HUNK_TEXT_PAGER pointed at a wrapper around real less -R; LazyGit invoked hunk pager while advertising TERM=dumb, the less wrapper was invoked 0 times, and no live less processes remained.

This PR description was generated by Pi using OpenAI GPT-5.1 Codex

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 24, 2026

Greptile Summary

This PR fixes hunk pager so that non-diff text (e.g. branch lists from LazyGit) is written directly to stdout instead of being handed to an interactive less session when the host advertises TERM=dumb or is a recognized captured-pager environment. Two regression tests are added alongside a fix to an existing test that was missing explicit env and stdoutIsTTY deps.

  • src/core/startup.ts: A new guard short-circuits the non-diff pager path with a passthrough plan when isCapturedPagerHost(env) or env.TERM === "dumb", preventing less from being spawned in contexts that cannot host an interactive pager.
  • src/core/startup.test.ts: Two new tests cover the captured-pager-host and plain-dumb-terminal passthrough cases; the pre-existing plain-text-pager test is hardened with explicit env and stdoutIsTTY values.

Confidence Score: 4/5

Safe to merge; the change is a small, well-tested guard that avoids launching an interactive pager in dumb-terminal contexts.

The logic change is narrow and the new tests cover both target cases. The one notable point is that isCapturedPagerHost(env) || env.TERM === "dumb" collapses to just env.TERM === "dumb" because the helper function itself requires TERM=dumb, making the captured-host branch unreachable in this expression — a minor clarity issue rather than a behavioral defect.

src/core/startup.ts — specifically the redundant OR condition on the new guard at line 147

Important Files Changed

Filename Overview
src/core/startup.ts Adds a passthrough guard for non-diff pager input when TERM=dumb or a captured pager host is detected; the guard condition contains a redundant isCapturedPagerHost branch since that function already requires TERM=dumb
src/core/startup.test.ts Adds two regression tests for the new passthrough paths (captured-pager-host and plain dumb-terminal); also correctly backfills stdoutIsTTY and env deps on the pre-existing plain-text-pager test to avoid relying on ambient process.env
CHANGELOG.md Adds a user-visible changelog entry for the pager passthrough fix; no issues

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[hunk pager invoked] --> B[read stdin text]
    B --> C{looksLikePatchInput?}
    C -- No --> D{isCapturedPagerHost OR TERM=dumb?}
    D -- Yes --> E[passthrough: write text to stdout]
    D -- No --> F[plain-text-pager: launch less]
    C -- Yes --> G{stdoutIsTTY?}
    G -- No --> E
    G -- Yes --> H{TERM=dumb AND NOT capturedHost?}
    H -- Yes --> E
    H -- No --> I{isCapturedPagerHost?}
    I -- Yes --> J[static-diff-pager: colored diff output]
    I -- No --> K[openControllingTerminal]
    K -- null --> J
    K -- terminal --> L[app: interactive TUI]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/core/startup.ts:146-147
The condition `isCapturedPagerHost(env) || env.TERM === "dumb"` is logically equivalent to just `env.TERM === "dumb"`, because `isCapturedPagerHost` itself requires `env.TERM === "dumb"` as its first operand — the captured-host check can never be true when the dumb-terminal check is false. The `isCapturedPagerHost` arm is therefore dead code here. This contrasts with the diff path (lines 167–178) where the two conditions produce different outcomes (`passthrough` vs `static-diff-pager`), making the distinction meaningful — but for non-diff input both arms return `passthrough`, collapsing the distinction entirely. The comment "Captured pager and dumb-terminal hosts" implies these are separate categories, which may mislead future readers.

```suggestion
      // Dumb-terminal and captured pager hosts (which also advertise TERM=dumb) cannot safely own an interactive text pager.
      if (env.TERM === "dumb") {
```

Reviews (1): Last reviewed commit: "fix(pager): avoid less in captured text ..." | Re-trigger Greptile

Comment thread src/core/startup.ts Outdated
Comment on lines +146 to +147
// Captured pager and dumb-terminal hosts cannot safely own an interactive text pager.
if (isCapturedPagerHost(env) || env.TERM === "dumb") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The condition isCapturedPagerHost(env) || env.TERM === "dumb" is logically equivalent to just env.TERM === "dumb", because isCapturedPagerHost itself requires env.TERM === "dumb" as its first operand — the captured-host check can never be true when the dumb-terminal check is false. The isCapturedPagerHost arm is therefore dead code here. This contrasts with the diff path (lines 167–178) where the two conditions produce different outcomes (passthrough vs static-diff-pager), making the distinction meaningful — but for non-diff input both arms return passthrough, collapsing the distinction entirely. The comment "Captured pager and dumb-terminal hosts" implies these are separate categories, which may mislead future readers.

Suggested change
// Captured pager and dumb-terminal hosts cannot safely own an interactive text pager.
if (isCapturedPagerHost(env) || env.TERM === "dumb") {
// Dumb-terminal and captured pager hosts (which also advertise TERM=dumb) cannot safely own an interactive text pager.
if (env.TERM === "dumb") {
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/startup.ts
Line: 146-147

Comment:
The condition `isCapturedPagerHost(env) || env.TERM === "dumb"` is logically equivalent to just `env.TERM === "dumb"`, because `isCapturedPagerHost` itself requires `env.TERM === "dumb"` as its first operand — the captured-host check can never be true when the dumb-terminal check is false. The `isCapturedPagerHost` arm is therefore dead code here. This contrasts with the diff path (lines 167–178) where the two conditions produce different outcomes (`passthrough` vs `static-diff-pager`), making the distinction meaningful — but for non-diff input both arms return `passthrough`, collapsing the distinction entirely. The comment "Captured pager and dumb-terminal hosts" implies these are separate categories, which may mislead future readers.

```suggestion
      // Dumb-terminal and captured pager hosts (which also advertise TERM=dumb) cannot safely own an interactive text pager.
      if (env.TERM === "dumb") {
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — simplified this to check TERM=dumb directly and updated the comment so it does not imply a separate captured-host branch in the non-diff path.

This comment was generated by Pi using OpenAI GPT-5.1 Codex

@benvinegar benvinegar merged commit 8c2331d into main May 24, 2026
5 checks passed
@benvinegar benvinegar deleted the fix/lazygit-non-diff-pager-passthrough branch May 24, 2026 14:21
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.

100% CPU usage by less under lazygit

1 participant