Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Key collisions can make the footer inaccurate, and the documented required fzf version is not released.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable entry inspection without closing fzf, supported by asynchronous IPC and per-run state.
Changes:
- Adds the
show_keybinding and command-specific entry handling. - Supports concurrent IPC channels and callback state.
- Documents the feature and configuration.
File summaries
| File | Description |
|---|---|
README.md |
Documents entry showing and configuration. |
doc/fzf-vim.txt |
Adds Vim help for the feature. |
autoload/fzf/vim/ipc.vim |
Supports concurrent FIFO channels. |
autoload/fzf/vim.vim |
Implements showing, hints, and per-run state. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Buffer-local Show actions can target the wrong buffer after the originating window changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
README.md:111
- This supported-command list omits
:RGand:BMarks, althoughfzf#vim#grep2()and the shared marks implementation both opt into Show. Include them so users of these documented commands do not incorrectly conclude that CTRL-O is unavailable.
`:Rg`, `:Ag`, `:Grep`, `:Lines`, `:BLines`, `:Tags`, `:BTags`, `:Marks`,
`:Changes`). Requires fzf 0.74.4 or later, and is not offered in fullscreen
doc/fzf-vim.txt:164
- This supported-command list omits
:RGand:BMarks, althoughfzf#vim#grep2()and the shared marks implementation both opt into Show. Include them so the help accurately describes availability for those commands.
entries in a single run (Files, GFiles, Buffers, History, Locate, Rg, Ag,
Grep, Lines, BLines, Tags, BTags, Marks, Changes). Requires fzf 0.74.4 or
later, and is not offered in fullscreen mode, with a layout that leaves no
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Key collisions, relative tilde paths, concurrent color previews, and the unpublished dependency version remain unresolved.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
autoload/fzf/vim.vim:642
- Expanding every leading
~as a home path breaks valid relative entries from:Files,:GFiles, and grep. For example, a tracked file named~/notesis emitted relative to the run directory but this resolves it to$HOME/notes, so Show opens the wrong file (if it exists) or does nothing. Preserve directory-relative paths for those commands and apply tilde expansion specifically to:Historyentries.
" filereadable does not expand a leading tilde, which :History reports
if a:path =~# '^\~'
return fnamemodify(a:path, ':p')
autoload/fzf/vim.vim:1049
- Per-run callback arguments prevent the old script-local overwrite, but a canceled older
:Colorsrun still restores its original scheme whenever the global scheme differs—even when that difference is the preview from a newer active run. Overlapping runs can therefore still clobber each other's preview. Track each run's last applied scheme and restore only if that run still owns the current preview, or otherwise coordinate/serialize Colors runs.
function! s:colors_exit(fifo, original, code)
if !empty(a:original) && a:code > 0 && a:original !=# get(g:, 'colors_name', '')
execute 'colo' a:original
endif
autoload/fzf/vim.vim:830
s:split_keys()normalizes configured show keys to fzf's matching rules, buthas_key(actions, key)comparesg:fzf_actionkeys verbatim. A configuration such as{'CTRL-O': 'split'}therefore bypasses this collision check even though fzf treats it as the same key;--expectwins and closes fzf while the footer still advertises Show. Normalize every action key before comparing it.
if has_key(actions, key) || index(paste, key) >= 0 || index(a:claimed, key) >= 0
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Key collisions, newline-containing paths, directory entries, and nested color restoration remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
autoload/fzf/vim.vim:769
- This rejects directories, but
:Locatecan return directory entries and its normal sink can open them. Consequently the documented Show key silently does nothing on those results. Acceptisdirectory(path)as another valid editable target.
if !filereadable(path)
return
autoload/fzf/vim.vim:456
enterandreturnremain distinct after this normalization even though fzf parses both as the same key. For example,show_key = 'return'withpaste_key = 'enter'passess:can_show(), so the footer advertises Show while the existing--expectaction wins. Canonicalize this alias here before all collision checks.
return map(keys, 'strchars(v:val) == 1 ? v:val
\ : v:val =~? "^alt-.$" ? "alt-".v:val[4:] : tolower(v:val)')
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Entry encoding corrupts valid control-byte paths, and buffer-number parsing fails for names containing tabs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 6
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Key aliases and some valid tag and directory entries currently produce incorrect Show behavior.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
autoload/fzf/vim.vim:456
- The collision check compares key spellings rather than fzf key events, so aliases can still make the advertised binding ineffective. For example, the default paste key is
alt-enter; settingshow_keyto its fzf aliasalt-returnpasses this normalization, but fzf maps both to the same event and--expectwins before the Show binding, closing fzf instead. Canonicalize fzf aliases (includingenter/return/ctrl-mandalt-enter/alt-return/ctrl-alt-m) before comparing keys and deciding which footer hints to display.
return map(keys, 'strchars(v:val) == 1 ? v:val
\ : v:val =~? "^alt-.$" ? "alt-".v:val[4:] : tolower(v:val)')
autoload/fzf/vim.vim:723
- This unconditionally re-edits the tag file even when the target window already displays it, unlike the normal tag sink, whose
s:action_for()skips that edit. Re-editing can reload/reset the buffer before applying the tag address; uses:edit_cmds()here so same-file tags only execute their address while other files retain the existing guarded edit behavior.
let cmds = ['keepalt keepjumps hide edit '.s:escape(path),
\ 'keepjumps '.excmd, 'normal! ^zvzz']
autoload/fzf/vim.vim:772
:Locatecan return directories, and its normal sink opens them with:edit, but this regular-file-only guard makes Show silently do nothing for every directory result. Accept directories here as well so Show preserves the command's existing entry behavior.
if !filereadable(path)
return
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
All three from the last pass were real, and all three were small. Taken.
Each was reproduced before the change and re-checked after: |
5c41dcb to
9ef02da
Compare
dedd5ac to
e9015ff
Compare
Files, GFiles, GFiles?, Rg/Ag, Buffers, History, Locate, Lines, BLines, Tags, BTags, Marks, Changes, Commits and BCommits. Writes the entry to an fzf#vim#ipc fifo, so it needs the asynchronous fzf#run from junegunn/fzf@5cb7bab7 (junegunn/fzf#4897, 0.74.4), which the g:loaded_fzf check detects. Commands declare '_show' for the entry kind and '_hint' for footer keys, so the footer and the binding cannot disagree. s:can_show withholds both when the run would block, when the window to open in is not visible, or when g:fzf_action, the paste key or the command itself already claims the key. g:fzf_vim.show_key configures the key, empty to turn it off, comma-separated to bind several ('ctrl-o,double-click'). Named for its 'Show' footer label, since Enter already occupies 'Open' there. Only the first key is labelled, and a clash on any of them drops all of them, so the footer cannot list a key that does nothing. Binding Enter drops its 'Open' hint too, since fzf accepts on Enter rather than through g:fzf_action. s:show_entry parses each entry the way that command's sink does and runs win_execute() in the window fzf started from. That keeps the focus in fzf and works from a popup, which win_gotoid() cannot leave. It opens with 'keepalt keepjumps hide', so stepping through entries leaves the alternate file and the jumplist unchanged. BLines, BTags and a lowercase mark carry a position but no buffer, so the callback carries the buffer the run started on as well as the window. fzf#run is asynchronous, so runs can overlap, and a second run would stop the first's ipc channel and overwrite the script locals its sink reads back afterwards. Channels are now keyed by fifo path and per-run state is bound into the callback instead, which is why Buffers, Colors, Jumps, Maps, Helptags and complete change here.
Files, GFiles, GFiles?, Rg/Ag, Buffers, History, Locate, Lines, BLines, Tags, BTags, Marks and Changes. Writes the entry to an fzf#vim#ipc fifo, so it needs the asynchronous fzf#run from junegunn/fzf@5cb7bab7 (junegunn/fzf#4897, 0.74.4), which the g:loaded_fzf check detects.
Commands declare '_show' for the entry kind and '_hint' for footer keys, so the footer and the binding cannot disagree. s:can_show withholds both when the run would block or the window to open in is not visible.
g:fzf_vim.show_key configures the key, empty to turn it off, comma separated to bind several ('ctrl-o,double-click'). Named for its 'Show' footer label, since Enter already occupies 'Open' there. Only the first key is labelled, and a clash on any of them drops all of them, so the footer cannot list a key that does nothing. Binding Enter drops its 'Open' hint too, since fzf accepts on Enter rather than through g:fzf_action.
s:show_entry parses each entry the way that command's sink does and runs win_execute() in the window fzf started from. That keeps the focus in fzf and works from a popup, which win_gotoid() cannot leave. It opens with 'keepalt keepjumps hide', so stepping through entries leaves the alternate file and the jumplist unchanged.
fzf#run is asynchronous, so runs can overlap, and a second run would stop the first's ipc channel and overwrite the script locals its sink reads back afterwards. Channels are now keyed by fifo path and per-run state is bound into the callback instead, which is why Buffers, Colors, Jumps, Maps, Helptags and complete change here.