fix(ui_select): add native preview for fzf-native/fzf-tmux#2753
Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds Changesui_select native preview support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant UiSelect as lua/fzf-lua/providers/ui_select.lua
participant Config as config.normalize_opts
participant PreviewType as resolve_preview_type
participant Fzf as fzf preview
Caller->>UiSelect: ui_select(items, opts, on_choice)
UiSelect->>Config: normalize_opts(opts, "ui_select")
Config-->>UiSelect: normalized opts
UiSelect->>PreviewType: resolve_preview_type(ui_opts, opts)
PreviewType-->>UiSelect: native or buffer
alt native
UiSelect->>Fzf: preview command writes temp file and renders via bat/type
else buffer
UiSelect->>Fzf: buffer_or_file previewer parses entry and previews item
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Owner
|
Tysm @phanen! |
6 tasks
phanen
force-pushed
the
fix/ui_select
branch
5 times, most recently
from
82c7776 to
4b17264
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/fzf-lua/profiles/fzf-tmux.lua (1)
14-22: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop the hard assert in the preview border callback
ui_select = { preview_type = "native" }only coversui_select; this same border function is still used bymanpages,helptags,undotree, andlsp.code_actionsunderfzf-tmux, where the metadata path ism.type = "nvim". Mirrorfzf-native.luaand remove the assert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/fzf-lua/profiles/fzf-tmux.lua` around lines 14 - 22, The preview border callback in fzf-tmux currently hard-asserts that m.type is "fzf", but this function is also used by manpages, helptags, undotree, and lsp.code_actions where m.type can be "nvim". Update the border function to match the behavior in fzf-native.lua by removing the assert and keeping the border selection logic based on FzfLua.utils.has(m.opts, "fzf", { 0, 63 }), so the callback works for both metadata paths.
🧹 Nitpick comments (1)
tests/ui_select_spec.lua (1)
100-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage, but missing a nil-returning
preview_itemcase.Consider adding a case where
preview_itemreturnsnilfor the non-native (buffer) path — this would have caught theassert()crash risk flagged inproviders/ui_select.lua(Lines 226-249).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui_select_spec.lua` around lines 100 - 149, Add a regression test in ui_select spec for the non-native buffer preview path where preview_item returns nil, since the current async test only covers a non-nil empty table. Use the existing T["ui_select"]["async preview_item opens without crash"] pattern with open_picker/register/reload and target the buffer backend, then assert the picker stays alive and does not crash; this should cover the assert() failure risk in providers/ui_select.lua within the preview_item handling logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/fzf-lua/defaults.lua`:
- Around line 2213-2223: The ui_select default is nesting preview_type under
M.defaults.ui_select.ui_select, but resolve_preview_type() only checks the
top-level opts.preview_type, so the default never takes effect. Move
preview_type to the top level of M.defaults.ui_select in defaults.lua and keep
the existing ui_select table for any nested provider-specific settings, so the
native preview default is actually picked up.
In `@lua/fzf-lua/providers/ui_select.lua`:
- Around line 214-217: Guard the preview-offset setup in ui_select’s item
handling so empty selections do not call ui_opts.preview_item with nil. In
ui_select.lua, before evaluating ui_opts.preview_item(items[1]) and assigning
opts.preview_offset, add an explicit check that items[1] exists (or that items
is non-empty) and only then compute first from preview_item; keep the existing
first.pos[1] logic unchanged for valid items.
- Around line 56-65: The fallback preview-type heuristic in resolve_preview_type
only checks opts[1] for "fzf-tmux", so combined profile tables can miss
native-preview detection and incorrectly fall back to "buffer". Update the
detection logic in resolve_preview_type to also recognize when opts[1] is a
table containing "fzf-tmux" (while preserving the existing opts.profile and
opts.fzf_opts checks), so combined profiles still resolve to "native".
- Around line 226-249: The builtin preview path in ui_select’s previewer
parse_entry currently asserts the result of ui_opts.preview_item, which will
crash on nil/false instead of falling back gracefully. Update the
previewer:_ctor parse_entry logic to handle a missing or invalid preview_item
result without assert, mirroring the native branch’s fallback behavior by
returning the “No preview available” content when preview_item returns nil/false
or lacks buf/pos, and keep the existing reverse_lookup/items flow intact.
- Around line 194-207: The preview command built in the UI select fallback
interpolates tmpfile directly, which breaks paths containing spaces and uses a
Unix-only stderr redirect. Update the command assembly in this preview builder
to properly shell-escape tmpfile for both the bat and fallback cat/type
commands, and switch the redirection target to NUL when utils.__IS_WINDOWS is
true while keeping the existing filetype-based language handling intact.
- Around line 184-224: The native preview path in ui_select.lua reuses a single
tmpfile inside the preview function, which can let an older bat/cat process read
newer content when fzf keeps previews alive. Update the preview handling in
opts.preview.fn to use a per-invocation temp file or stream the buffer content
via stdin instead of the shared tmpfile, and keep the cleanup in
opts.winopts.on_close consistent with the new per-preview lifecycle.
---
Outside diff comments:
In `@lua/fzf-lua/profiles/fzf-tmux.lua`:
- Around line 14-22: The preview border callback in fzf-tmux currently
hard-asserts that m.type is "fzf", but this function is also used by manpages,
helptags, undotree, and lsp.code_actions where m.type can be "nvim". Update the
border function to match the behavior in fzf-native.lua by removing the assert
and keeping the border selection logic based on FzfLua.utils.has(m.opts, "fzf",
{ 0, 63 }), so the callback works for both metadata paths.
---
Nitpick comments:
In `@tests/ui_select_spec.lua`:
- Around line 100-149: Add a regression test in ui_select spec for the
non-native buffer preview path where preview_item returns nil, since the current
async test only covers a non-nil empty table. Use the existing
T["ui_select"]["async preview_item opens without crash"] pattern with
open_picker/register/reload and target the buffer backend, then assert the
picker stays alive and does not crash; this should cover the assert() failure
risk in providers/ui_select.lua within the preview_item handling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdcf4253-924e-4c5c-9084-8858b62f763e
📒 Files selected for processing (5)
lua/fzf-lua/defaults.lualua/fzf-lua/profiles/fzf-native.lualua/fzf-lua/profiles/fzf-tmux.lualua/fzf-lua/providers/ui_select.luatests/ui_select_spec.lua
phanen
force-pushed
the
fix/ui_select
branch
3 times, most recently
from
37621d4 to
d2a8395
Compare
Owner
|
Tysm for this wonderful addition @phanen ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #2743
Summary by CodeRabbit
ui_selectconfiguration for single-select with native preview behavior.ui_select.ui_selectpreview resolution and rendering for both native and buffer modes, including safer handling when preview data is missing.ui_selectcoverage for native/tmux preview, async preview startup, empty preview regression, and abort via escape.