Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a preview-aware pager flow and a preview wrapper: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant FZF as "fzf (preview)"
participant Forgit as "git-forgit"
participant GitConfig as "git config / env"
participant Pager as "pager program"
FZF->>Forgit: invoke preview command (preview subprocess)
Forgit->>Forgit: set FORGIT_IN_PREVIEW via _forgit_preview
Forgit->>GitConfig: check FORGIT_PREVIEW_PAGER / other pager settings
alt FORGIT_PREVIEW_PAGER set and IN_PREVIEW
Forgit->>Pager: launch FORGIT_PREVIEW_PAGER (non-interactive)
else fallback
Forgit->>Pager: resolve and launch normal pager
end
Pager-->>Forgit: stream output
Forgit-->>FZF: deliver preview content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wfxr
force-pushed
the
tui-pager
branch
2 times, most recently
from
1265734 to
672394f
Compare
|
I might very well be missing something here, but when testing this I do not get the interactive pager when pressing |
|
@sandr01d You were right to call this out. I re-checked #491 and the scope here is only fixing blank fzf previews for TUI pagers. I did try to make TUI pagers work in the Enter/fullscreen path as well, but that turned out to be a separate problem and introduced extra complexity and compatibility concerns around the existing fullscreen pager flow. So I narrowed this PR back to the minimal fix. What changed now is:
|
|
Even with the new approach, I still get the preview pager when |
Fixed. This turned out to be trickier than I expected. I first looked at using inferred signals to detect preview context:
But both run into the same problem here: the So instead of trying to infer preview context, I switched to making it explicit:
That keeps preview panes on the non-interactive pager, while leaving Enter/fullscreen behavior unchanged. I also added regression tests for both the pager-selection logic and the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/git-forgit (1)
146-153: ScopeFORGIT_IN_PREVIEWto the delegated call to avoid leaking state when sourced.Line 151 exports globally in the current shell process. When
bin/git-forgitis sourced (tests/helpers), this can affect subsequent helper calls unexpectedly.♻️ Proposed refactor
_forgit_preview() { local cmd=$1 shift - # Funnel all fzf --preview commands through a single wrapper so preview-only - # pager behavior is controlled by an explicit marker instead of ambient env. - export FORGIT_IN_PREVIEW=1 - _forgit_"${cmd}" "$@" + local fn="_forgit_${cmd}" + # Funnel all fzf --preview commands through a single wrapper so preview-only + # pager behavior is controlled by an explicit marker instead of ambient env. + if ! declare -F "$fn" >/dev/null; then + _forgit_warn "preview command not found: $cmd" + return 1 + fi + ( + export FORGIT_IN_PREVIEW=1 + "$fn" "$@" + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-forgit` around lines 146 - 153, The code exports FORGIT_IN_PREVIEW globally in _forgit_preview which leaks state when the script is sourced; change it to set the variable only for the delegated call instead of exporting. Replace the export+separate call with a single scoped invocation such as invoking _forgit_"${cmd}" with FORGIT_IN_PREVIEW=1 in the same command (or run the call in a subshell that exports the var) so the flag is not exported to the calling shell; update the _forgit_preview function accordingly.tests/preview-context.test.sh (1)
50-66: Consider adding one assertion for Enter/fullscreen binding preservation.You already assert preview wiring; adding an Enter binding check would lock in the “preview-only override” guarantee.
🧪 Suggested assertion add-on
function test_forgit_diff_preview_command_uses_preview_wrapper() { bashunit::mock "fzf" 'printf "%s" "$FZF_DEFAULT_OPTS"' local output output=$(_forgit_diff) assert_contains "--preview=\"$FORGIT preview diff_view {}" "$output" + assert_contains "--bind=\"enter:execute($FORGIT diff_enter {}" "$output" + assert_contains "| $FORGIT pager enter)\"" "$output" }As per coding guidelines: Add or update tests for behavior changes, especially parsing, selection, and cross-shell integration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview-context.test.sh` around lines 50 - 66, Add one assertion in both test_forgit_diff_preview_command_uses_preview_wrapper and test_forgit_show_preview_command_uses_preview_wrapper to verify the Enter/fullscreen binding is preserved: check that the generated fzf command output from _forgit_diff and _forgit_show includes a --bind entry that maps enter to executing the FORGIT preview wrapper (i.e., a --bind mapping where enter triggers execution of the FORGIT preview command, such as enter:execute:...preview), so the tests assert both preview wiring and that Enter opens the preview wrapper rather than performing a normal selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/git-forgit`:
- Around line 146-153: The code exports FORGIT_IN_PREVIEW globally in
_forgit_preview which leaks state when the script is sourced; change it to set
the variable only for the delegated call instead of exporting. Replace the
export+separate call with a single scoped invocation such as invoking
_forgit_"${cmd}" with FORGIT_IN_PREVIEW=1 in the same command (or run the call
in a subshell that exports the var) so the flag is not exported to the calling
shell; update the _forgit_preview function accordingly.
In `@tests/preview-context.test.sh`:
- Around line 50-66: Add one assertion in both
test_forgit_diff_preview_command_uses_preview_wrapper and
test_forgit_show_preview_command_uses_preview_wrapper to verify the
Enter/fullscreen binding is preserved: check that the generated fzf command
output from _forgit_diff and _forgit_show includes a --bind entry that maps
enter to executing the FORGIT preview wrapper (i.e., a --bind mapping where
enter triggers execution of the FORGIT preview command, such as
enter:execute:...preview), so the tests assert both preview wiring and that
Enter opens the preview wrapper rather than performing a normal selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5c33ed2-e6d8-4035-b26c-0b9ba94f82e6
📒 Files selected for processing (2)
bin/git-forgittests/preview-context.test.sh
When users configure a TUI/interactive diff pager (e.g., diffnav, tig) via `git config pager.diff`, fzf preview panes show blank output because TUI pagers require a TTY that fzf previews don't provide. Add FORGIT_PREVIEW_PAGER env var that overrides the diff pager only in fzf preview context. Preview detection relies on a FORGIT_IN_PREVIEW marker set by a new `_forgit_preview` wrapper, so the override applies only to preview commands and leaves fullscreen behavior unchanged. Fixes #491
2 participants
Check list
Description
Fixes #491.
When users configure a TUI/interactive diff pager (e.g.,
diffnav,tig) viagit config pager.diff, all fzf preview panes show blank output. This is because fzf preview commands do not run in a terminal, and TUI pagers require a TTY to render.This PR adds a
FORGIT_PREVIEW_PAGERenv var that is used only in actual fzf preview context. Preview detection now relies on fzf'sFZF_PREVIEW_COLUMNSenvironment variable, so the override applies only to preview commands and leaves existing Enter/fullscreen behavior unchanged.Users can set
FORGIT_PREVIEW_PAGERto a non-interactive pager likedeltato get working previews while keeping the rest of the pager flow unchanged.Type of change
Test environment
Summary by CodeRabbit
New Features
Documentation