refactor(binds): consolidate all binds into a ...#2634
Draft
refactor(binds): consolidate all binds into a ...#2634
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:
📝 WalkthroughWalkthroughIntroduces a unified binds subsystem: new design doc and a Neovim-side bind layer that normalizes legacy keymaps/actions into a single Changes
Sequence Diagram(s)sequenceDiagram
participant NV as Neovim (fzf-lua)
participant FZF as fzf process
participant PIPE as Shell Transform (pipe_wrap_fn)
participant HAND as fzf-lua dispatch handler
NV->>FZF: spawn fzf with consolidated --bind=...:transform:PIPE_CMD
Note right of FZF: key presses or events\nemit triggers (keys or __evt__...)
FZF->>PIPE: invoke PIPE_CMD with trigger + {q}/{n}
PIPE->>HAND: call dispatch handler (stdin / env)
HAND->>HAND: normalize trigger, strip {q}/{n}, update resume query
HAND->>NV: invoke resolved Neovim handler or builtin executor
NV-->>HAND: return fzf action string (e.g., "reload:+...", "print()")
HAND-->>PIPE: write action string back to fzf
PIPE-->>FZF: deliver action string to fzf process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/specs/2026-03-27-unified-binds-design.md (1)
140-149: Add language identifiers to fenced code blocks.The fences at Line 140, Line 145, and Line 366 are missing a language tag, which triggers markdownlint
MD040.
Usetext(or a more specific language where applicable) to keep lint clean and rendering consistent.Proposed markdown diff
- ``` + ```text --bind=ctrl-r,ctrl-g,f4,f1:transform:BASE_CMD {+} {q} {n}
--bind=load:transform:BASE_CMD __evt__load {+} {q} {n} --bind=resize:transform:BASE_CMD __evt__resize {+} {q} {n} --bind=start:transform:BASE_CMD __evt__start {+} {q} {n}-
+text
User Config normalize_binds() Build Phase
─────────── ───────────────── ───────────
...
-+</details> Also applies to: 366-382 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/specs/2026-03-27-unified-binds-design.mdaround lines 140 - 149, Add
the missing fenced-code language identifiers by changing the three code fences
that contain the examples to use "text" (or a specific language) — specifically
the fence around the line "--bind=ctrl-r,ctrl-g,f4,f1:transform:BASE_CMD {+} {q}
{n}", the fence around the event examples beginning
"--bind=load:transform:BASE_CMD __evt__load {+} {q} {n}" (and the resize/start
lines), and the fence that contains the table starting with "User Config
normalize_binds() Build Phase"; update each opening ``` to
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/2026-03-27-unified-binds-design.md`:
- Around line 412-414: Clarify deterministic conflict resolution for normalized
duplicate keys by updating the spec to state that normalization is applied
deterministically and tie-breaking must be explicit: either (a) preserve
declaration order by requiring binds/actions to be declared as an ordered array
(and document that normalization uses that array order for last-write-wins
within the same table), or (b) treat same-table normalized collisions as an
error and require the parser/validator to emit a clear warning/error; also
mention that provider merges still follow the documented precedence across
tables. Ensure you reference the existing terms "last-write-wins", "normalize",
and "binds/actions" when making this change so readers know exactly where the
rule applies.
- Around line 15-17: Replace the user-facing occurrences of the word "builtin"
with the hyphenated form "built-in" in the spec text (not changing actual code
identifiers), specifically update the description that currently reads
"`keymap.builtin` — neovim terminal-mode keymaps for builtin actions" to use
"built-in", and fix the other nearby prose occurrences that describe "actions"
processed by conversion functions to use "built-in" consistently; search for the
token `keymap.builtin` in the document and update its surrounding descriptive
text and the other two referenced prose instances so all user-facing mentions
read "built-in".
---
Nitpick comments:
In `@docs/specs/2026-03-27-unified-binds-design.md`:
- Around line 140-149: Add the missing fenced-code language identifiers by
changing the three code fences that contain the examples to use "text" (or a
specific language) — specifically the fence around the line
"--bind=ctrl-r,ctrl-g,f4,f1:transform:BASE_CMD {+} {q} {n}", the fence around
the event examples beginning "--bind=load:transform:BASE_CMD __evt__load {+} {q}
{n}" (and the resize/start lines), and the fence that contains the table
starting with "User Config normalize_binds()
Build Phase"; update each opening ``` to ```text to satisfy MD040.
🪄 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: a900dbfa-e6df-4be8-a1d6-5731cad873bd
📒 Files selected for processing (1)
docs/specs/2026-03-27-unified-binds-design.md
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
from
709d099 to
211c08e
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/2026-03-27-unified-binds-design.md`:
- Around line 140-149: Add explicit language identifiers to the fenced code
blocks that show the --bind examples (the blocks containing lines like
"--bind=ctrl-r,ctrl-g,f4,f1:transform:BASE_CMD {+} {q} {n}" and the event
examples "__evt__load/__evt__resize/__evt__start"), e.g. use backticks with
```bash or ```text so the fences are annotated and MD040 warnings are resolved;
update the other similar fenced block around the event examples (noted also at
lines ~366-382) the same way.
In `@lua/fzf-lua/binds.lua`:
- Around line 97-109: normalize_value currently converts non-event function
values into {fn = value, accept = true}, but legacy keymap.fzf = function
bindings relied on create_fzf_binds() treating bare functions as execute-silent;
update normalize_value to preserve that legacy behavior by returning a
normalized table that encodes the execute-silent semantics (e.g., { fn = value,
execute = true, silent = true }) for plain functions when is_event is false, and
apply the same fix to the duplicate logic around lines referenced (the other
normalize block at 178-188) so legacy function-valued keymap.fzf entries
continue to run in-place instead of exiting fzf.
- Around line 208-219: When processing opts.binds in the
normalize_key/normalize_value loop you currently set merged[fzf_key]=nil for
explicit unbinds, which prevents the later cleanup from seeing the override and
allows legacy tables (opts.actions and opts.keymap.fzf) to resurrect the bind;
modify the loop in the binds handling (the
normalize_key/normalize_value/FZF_EVENTS section) so that when a bind overrides
or value == false you either set a persistent marker in merged (e.g.,
merged[fzf_key] = { _source = "binds", _nvim_key = key, _ignore = true }) or
explicitly clear the legacy entries (set opts.actions[key] = nil and if
opts.keymap and opts.keymap.fzf then opts.keymap.fzf[key] = nil) so
actions.expect()/create_fzf_binds() cannot resurrect the binding; apply the same
change to the other similar block handling binds (the block around the 471-509
region).
- Around line 321-343: The else branch currently collapses plain accept entries
into bare functions losing metadata; instead detect when entry is a table and
preserve its metadata by building action_entry as a table that sets fn =
entry.fn (or uses entry.fn when present) and copies prefix, postfix, desc,
header, field_index, reuse, chain (or any other relevant fields) into
action_entry, while leaving true bare-function entries as-is; update the code
that sets opts.actions[key] (referencing accept, action_entry, and opts.actions)
so actions.expect() and header rendering still see the original metadata.
- Around line 69-88: normalize_key currently returns early for raw fzf-style
keys (the if-not-angle-brackets branch) and therefore never checks
special_neovim_only; change normalize_key so it does not return immediately for
non-angled keys but instead sets fzf_key = key when the input is already
fzf-style (and otherwise calls utils.neovim_bind_to_fzf for angle-bracket
inputs), then consult the special_neovim_only table and return fzf_key plus the
boolean special flag accordingly (so raw fzf-style unsupported combos like
"ctrl-enter" are treated the same as "<C-Enter>" and routed via the SIGWINCH
bridge).
In `@lua/fzf-lua/core.lua`:
- Around line 266-274: The unified bind normalization is currently only run when
convert_actions is true AND opts.actions is a table, which skips
binds.build_transform_binds() for pickers that set opts.binds or opts.keymap.*
but not opts.actions; change the conditional so that when convert_actions is
truthy you call binds.build_transform_binds(opts) whenever
binds.can_unified(opts) is true (i.e., use if convert_actions and
binds.can_unified(opts) then ... without also requiring type(opts.actions) ==
"table"), and keep the existing else branch (M.convert_reload_actions /
M.convert_exec_silent_actions) for the non-unified path so that opts.binds and
opts.keymap.fzf/builtin get materialized and win.setup_keybinds() early-return
path works correctly.
In `@lua/fzf-lua/win.lua`:
- Around line 240-259: The unified-transform preview handlers call
win.toggle_preview()/win.toggle_preview_cw() which can re-enter the SIGWINCH
callbacks and double-apply the fzf-side preview action; to prevent this, add a
temporary guard flag (e.g. self._fzf_skip_sigwinch) that you set true
immediately before invoking win.toggle_preview or win.toggle_preview_cw in the
unified transform handlers and unset after the call, and update the on_SIGWINCH
callbacks (the anonymous functions registered in on_SIGWINCH for
"toggle-preview" and "toggle-preview-cw") to return early when
self._fzf_skip_sigwinch is true so the fzf-side action is not applied a second
time while Neovim-side toggle is running; leave the existing
_fzf_toggle_prev_bind behavior unchanged.
🪄 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: d8a57119-60eb-4c6b-a6f9-74692d0649cb
📒 Files selected for processing (4)
docs/specs/2026-03-27-unified-binds-design.mdlua/fzf-lua/binds.lualua/fzf-lua/core.lualua/fzf-lua/win.lua
Comment on lines
+240
to
+259
| -- Register SIGWINCH handler for toggle-preview so that direct calls | ||
| -- to win.toggle_preview() (e.g. from API/tests) can sync fzf state. | ||
| -- Only needed for split layouts or fzf-native previewers (not builtin | ||
| -- previewer without split, where fzf's preview pane is size 0). | ||
| if not utils.__IS_WINDOWS | ||
| and (self.winopts.split or not self.previewer_is_builtin) then | ||
| self.on_SIGWINCH(self._o, "toggle-preview", function(args) | ||
| if tonumber(args[1]) then | ||
| return "toggle-preview" | ||
| else | ||
| return string.format("change-preview-window(%s)", self:normalize_preview_layout().str) | ||
| end | ||
| end) | ||
| self.on_SIGWINCH(self._o, "toggle-preview-cw", function(args) | ||
| if not tonumber(args[1]) then return end | ||
| return string.format("change-preview-window(%s)", self:normalize_preview_layout().str) | ||
| end) | ||
| -- Set _fzf_toggle_prev_bind so toggle_preview() can use SIGWINCH | ||
| self._fzf_toggle_prev_bind = true | ||
| end |
There was a problem hiding this comment.
Avoid re-entering preview sync from unified transform handlers.
In unified mode this block sets _fzf_toggle_prev_bind = true, but the builtin transform handlers already return toggle-preview / change-preview-window(...) after calling win.toggle_preview() or win.toggle_preview_cw(). That means a single transform-dispatched preview key can also fire a SIGWINCH from inside the Neovim-side toggle, so the fzf-side preview action gets applied twice and the state can end up flipped back out of sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/win.lua` around lines 240 - 259, The unified-transform preview
handlers call win.toggle_preview()/win.toggle_preview_cw() which can re-enter
the SIGWINCH callbacks and double-apply the fzf-side preview action; to prevent
this, add a temporary guard flag (e.g. self._fzf_skip_sigwinch) that you set
true immediately before invoking win.toggle_preview or win.toggle_preview_cw in
the unified transform handlers and unset after the call, and update the
on_SIGWINCH callbacks (the anonymous functions registered in on_SIGWINCH for
"toggle-preview" and "toggle-preview-cw") to return early when
self._fzf_skip_sigwinch is true so the fzf-side action is not applied a second
time while Neovim-side toggle is running; leave the existing
_fzf_toggle_prev_bind behavior unchanged.
Owner
Author
There was a problem hiding this comment.
Not a real bug. The SIGWINCH handler and toggle_preview() run in different processes — the handler returns a string to fzf (external process), it does not call toggle_preview(). And toggle_preview() calls self:SIGWINCH(...) asynchronously (sends POSIX signal 28), which triggers fzf to fire its resize event later. The signal does not synchronously invoke the handler. There is no re-entry path: neovim side runs toggle_preview() → sends signal → fzf receives signal → fires resize → handler returns string → fzf processes it. These are separate async phases.
There was a problem hiding this comment.
@ibhagwan, you're absolutely right — I misread the execution model. The SIGWINCH callback only returns a string back to the fzf process; it never synchronously calls toggle_preview(). And toggle_preview() only sends POSIX signal 28 asynchronously, which fzf receives and processes in a later resize event cycle. There is no synchronous re-entry path whatsoever. Apologies for the noise.
✏️ Learnings added
Learnt from: ibhagwan
Repo: ibhagwan/fzf-lua PR: 2634
File: lua/fzf-lua/win.lua:240-277
Timestamp: 2026-03-27T17:15:09.265Z
Learning: In fzf-lua (lua/fzf-lua/win.lua), the SIGWINCH handler registered via `FzfWin.on_SIGWINCH` and `toggle_preview()` operate in completely separate async phases across separate processes (Neovim and fzf). The SIGWINCH callback only returns a fzf action string to the external fzf process — it never calls `toggle_preview()`. Conversely, `toggle_preview()` only sends POSIX signal 28 asynchronously via `self:SIGWINCH(...)`; fzf receives the signal later, fires its `resize` event, and then the handler runs. There is no synchronous re-entry path between these two sides. Do not flag double-apply / re-entry bugs in this flow.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
2 times, most recently
from
b4e56e1 to
68b86e2
Compare
Owner
Author
|
@phanen - what do you think? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
lua/fzf-lua/binds.lua (5)
215-227:⚠️ Potential issue | 🟠 MajorKeep the legacy tables synchronized with the winning canonical bind.
opts.binds[key] = falseremoves the merged record before cleanup, and the later rewrite/_ignore logic only touches the normalizedkey, not the originalentry._nvim_key. That leaves stale entries inopts.actions/opts.keymap.fzf, soactions.expect()orcreate_fzf_binds()can still resurrect an overridden bind or emit both<C-x>andctrl-x.Also applies to: 329-352, 481-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 215 - 227, When a bind is explicitly removed (value == false) the code currently nils merged[fzf_key] but leaves stale entries in legacy tables (opts.actions and opts.keymap.fzf) keyed by the original nvim key, so functions like actions.expect() and create_fzf_binds() can resurrect or duplicate binds; update the removal branch in the block that handles opts.binds (and the other similar bind-processing blocks) to also delete any legacy entries using both the normalized key (fzf_key) and the original nvim key (key / entry._nvim_key) — ensure you call the same cleanup logic used when a canonical entry wins (remove entries from opts.actions and opts.keymap.fzf and clear any other legacy references) so legacy tables stay synchronized with merged.
408-415:⚠️ Potential issue | 🟠 MajorPer-key
field_indexis still flattened to the default payload.All transform keys share one consolidated
--bindwith hardcoded{+} {q} {n}, and only event entries setopts.__transform_custom_fi. Any key bind with a customfield_indexreceives the wrong argv while the dispatch handler still strips query/match-count as if the default payload was used.Also applies to: 421-427, 711-726
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 408 - 415, The current implementation flattens per-key field_index into one consolidated bind entry (built from transform_keys, base_cmd and hardcoded "{+} {q} {n}"), causing keys with custom field_index to receive the wrong argv and the dispatch handler to mis-parse; fix by emitting a separate bind entry for each transform key (instead of one joined bind) that injects that key's specific payload including its field_index metadata (the same identifier used in opts.__transform_custom_fi) so the dispatch handler sees the correct argv for each key; apply the same change pattern where similar consolidation occurs (the other transform/grouped bind sites you noted around lines 421-427 and 711-726) and ensure the dispatch logic continues to reference opts.__transform_custom_fi keyed by the exact bind payload you generate.
338-350:⚠️ Potential issue | 🟠 MajorKeep accept-table metadata intact.
The plain accept branch collapses a table back to
entry.fn, which dropsprefix,postfix,desc,header, andfield_index.actions.expect()and the help/header code then see a different action shape than the equivalent legacy table.Preserve the action table instead of collapsing it to a bare function
- else - action_entry = entry.fn or entry + else + action_entry = type(entry.fn) == "function" and { + fn = entry.fn, + prefix = entry.prefix, + postfix = entry.postfix, + desc = entry.desc, + header = entry.header, + field_index = entry.field_index, + } or entry end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 338 - 350, The code currently collapses an action table to a bare function by setting action_entry = entry.fn which drops metadata; instead preserve the full table shape so metadata (prefix, postfix, desc, header, field_index, reuse, fn) remains available for actions.expect() and help/header logic — replace the plain-function branch to keep the table (e.g., set action_entry = entry or copy entry while ensuring entry.fn is still callable) rather than assigning entry.fn, referencing the same action_entry and entry fields used elsewhere (fn, reuse, prefix, postfix, desc, header, field_index).
69-96:⚠️ Potential issue | 🟠 MajorDon't return early for raw fzf-style special keys.
The non-
<...>branch skipsspecial_neovim_only, so raw spellings likectrl-enter,ctrl-bs, orshift-enterare treated as normal fzf binds and never reach the SIGWINCH bridge. That breaks the “both styles accepted” normalization promise for the unsupported modifier+special combos.One way to normalize both spellings through the same path
local function normalize_key(key) - -- Already in fzf format (no angle brackets) - if not key:match("^<.*>$") then - return key, false - end - local fzf_key = utils.neovim_bind_to_fzf(key) + local fzf_key = key + if key:match("^<.*>$") then + fzf_key = utils.neovim_bind_to_fzf(key) + endThen run the existing shift-letter and
special_neovim_onlylogic againstfzf_keybefore returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 69 - 96, The normalize_key function currently returns early for keys that are already in fzf format (not matching "^<.*>$"), which bypasses the shift-letter collapse and the special_neovim_only checks; change the function so both branches produce an fzf_key and then run the same normalization logic: call utils.neovim_bind_to_fzf when needed (or treat the raw key as fzf_key), apply the gsub that collapses "shift-(%a)" to uppercase, and finally test fzf_key against the special_neovim_only table before returning fzf_key and the boolean flag; this ensures raw fzf-style special keys like "ctrl-enter"/"ctrl-bs"/"shift-enter" are handled the same way as "<...>" inputs.
110-116:⚠️ Potential issue | 🟠 MajorPreserve bare
keymap.fzffunctions as execute-silent.This branch normalizes every non-event function to
{ accept = true }. Legacyopts.keymap.fzf[key] = function(...) ... endcallbacks used to stay on the execute-silent path, so unified mode now closes fzf instead of running them in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 110 - 116, If a bare function comes from the fzf keymap (opts.keymap.fzf) we must preserve the legacy execute-silent behavior instead of converting it to an accept callback; so inside the branch where type(value) == "function" (and when is_event is false) detect that the function originates from the fzf keymap (opts.keymap.fzf) and return a descriptor that marks it for execute-silent (e.g. return { fn = value, exec = true, silent = true } or the project’s equivalent flags) rather than { fn = value, accept = true }; keep the existing event branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/binds.lua`:
- Around line 243-250: The SIGWINCH routing currently returns M.SIGWINCH for
Neovim-only keys (when is_neovim_only and not is_event), but that bridge invokes
handlers as handler({}, {}) which loses selection/query context for
function-based special keys; update the logic in the is_neovim_only branch (the
code using is_neovim_only, is_event, opts._is_fzf_tmux, _G.fzf_jobstart,
utils.__IS_WINDOWS and returning M.DROPPED / M.SIGWINCH) to preserve context:
detect when a bind's action is a function or needs {+}/{q} (the cases where
handlers rely on context) and either route those to M.DROPPED instead of
M.SIGWINCH or ensure the SIGWINCH bridge carries and forwards the current
selection/query to the registered callback (so handlers are invoked with the
proper args instead of handler({}, {})); make the change wherever the same
pattern appears (noted at lines around 359-397, 459-468, 543-580, 594-601).
- Around line 138-140: In classify_bind() (lua/fzf-lua/binds.lua) the branch
that handles table binds with a string (the code checking type(value) == "table"
and type(value[1]) == "string") currently returns { fzf_action = value[1], desc
= value.desc } which treats builtin Neovim-only actions as direct fzf actions;
change this to run the same builtin-to-fzf lookup used by the plain-string
branch so that if value[1] is a known builtin it gets converted to the
corresponding fzf action (or otherwise preserved), and then return the
normalized table keeping desc (i.e. produce fzf_action or vim_action consistent
with the string-only normalization logic instead of unconditionally assigning
value[1]).
---
Duplicate comments:
In `@lua/fzf-lua/binds.lua`:
- Around line 215-227: When a bind is explicitly removed (value == false) the
code currently nils merged[fzf_key] but leaves stale entries in legacy tables
(opts.actions and opts.keymap.fzf) keyed by the original nvim key, so functions
like actions.expect() and create_fzf_binds() can resurrect or duplicate binds;
update the removal branch in the block that handles opts.binds (and the other
similar bind-processing blocks) to also delete any legacy entries using both the
normalized key (fzf_key) and the original nvim key (key / entry._nvim_key) —
ensure you call the same cleanup logic used when a canonical entry wins (remove
entries from opts.actions and opts.keymap.fzf and clear any other legacy
references) so legacy tables stay synchronized with merged.
- Around line 408-415: The current implementation flattens per-key field_index
into one consolidated bind entry (built from transform_keys, base_cmd and
hardcoded "{+} {q} {n}"), causing keys with custom field_index to receive the
wrong argv and the dispatch handler to mis-parse; fix by emitting a separate
bind entry for each transform key (instead of one joined bind) that injects that
key's specific payload including its field_index metadata (the same identifier
used in opts.__transform_custom_fi) so the dispatch handler sees the correct
argv for each key; apply the same change pattern where similar consolidation
occurs (the other transform/grouped bind sites you noted around lines 421-427
and 711-726) and ensure the dispatch logic continues to reference
opts.__transform_custom_fi keyed by the exact bind payload you generate.
- Around line 338-350: The code currently collapses an action table to a bare
function by setting action_entry = entry.fn which drops metadata; instead
preserve the full table shape so metadata (prefix, postfix, desc, header,
field_index, reuse, fn) remains available for actions.expect() and help/header
logic — replace the plain-function branch to keep the table (e.g., set
action_entry = entry or copy entry while ensuring entry.fn is still callable)
rather than assigning entry.fn, referencing the same action_entry and entry
fields used elsewhere (fn, reuse, prefix, postfix, desc, header, field_index).
- Around line 69-96: The normalize_key function currently returns early for keys
that are already in fzf format (not matching "^<.*>$"), which bypasses the
shift-letter collapse and the special_neovim_only checks; change the function so
both branches produce an fzf_key and then run the same normalization logic: call
utils.neovim_bind_to_fzf when needed (or treat the raw key as fzf_key), apply
the gsub that collapses "shift-(%a)" to uppercase, and finally test fzf_key
against the special_neovim_only table before returning fzf_key and the boolean
flag; this ensures raw fzf-style special keys like
"ctrl-enter"/"ctrl-bs"/"shift-enter" are handled the same way as "<...>" inputs.
- Around line 110-116: If a bare function comes from the fzf keymap
(opts.keymap.fzf) we must preserve the legacy execute-silent behavior instead of
converting it to an accept callback; so inside the branch where type(value) ==
"function" (and when is_event is false) detect that the function originates from
the fzf keymap (opts.keymap.fzf) and return a descriptor that marks it for
execute-silent (e.g. return { fn = value, exec = true, silent = true } or the
project’s equivalent flags) rather than { fn = value, accept = true }; keep the
existing event branch unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
lua/fzf-lua/binds.lua (7)
69-96:⚠️ Potential issue | 🟠 MajorDon't short-circuit raw fzf-style special keys.
Line 71 returns
(key, false)before the unsupported-key lookup, soctrl-enter/shift-enter/alt-escstay on the normal--bindpath instead of the SIGWINCH bridge. That breaks the “both spellings accepted” normalization for exactly the keys fzf cannot bind directly.According to the official fzf documentation or man page, are `ctrl-enter`, `ctrl-bs`, `shift-enter`, `shift-cr`, and `alt-esc` valid `--bind` key names?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 69 - 96, The function normalize_key currently returns early for keys not wrapped in angle brackets, bypassing the special_neovim_only lookup and leaving unsupported fzf-style special keys (e.g., ctrl-enter, shift-enter, alt-esc, ctrl-bs, shift-cr) on the normal --bind path; modify normalize_key so that raw fzf-style special keys are still checked against the special_neovim_only table (move or duplicate the special-key check before the early return or adjust the early-return branch to consult special_neovim_only) so those keys are routed to the SIGWINCH bridge; reference normalize_key and the special_neovim_only table to locate where to change the control flow.
216-227:⚠️ Potential issue | 🟠 MajorDon't drop explicit unbinds out of
merged.Removing the entry here means the later cleanup never sees the override, so an older
opts.actions[...]oropts.keymap.fzf[...]can survive and resurrect a bind thatopts.bindsexplicitly disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 216 - 227, The code currently removes binds by setting merged[fzf_key] = nil when opts.binds supplies false, which loses the override and allows older binds from opts.actions or opts.keymap.fzf to be resurrected; instead preserve a tombstone in merged for that key (e.g., assign a marker object rather than nil) so the later cleanup/merge logic can detect an explicit unbind. Update the branch that handles value == false to set merged[fzf_key] to a sentinel table with fields like _source = "binds", _nvim_key = key, and a flag such as _disabled = true (or similar) so normalize_key/normalize_value consumers and the later cleanup know this was intentionally disabled. Ensure the rest of the merge/cleanup logic treats this sentinel as an explicit override.
329-350:⚠️ Potential issue | 🟠 MajorPreserve accept-action metadata when rebuilding
opts.actions.The
elsebranch collapses plain accept tables to a bare function, which dropsprefix,postfix,desc,header, andfield_indexbeforeactions.expect()and header rendering see them.Suggested fix
- else - action_entry = entry.fn or entry + else + action_entry = type(entry.fn) == "function" and { + fn = entry.fn, + prefix = entry.prefix, + postfix = entry.postfix, + desc = entry.desc, + header = entry.header, + field_index = entry.field_index, + reuse = entry.reuse, + } or entry end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 329 - 350, The current rebuild of accept entries in binds.lua drops metadata in the else branch by assigning action_entry = entry.fn or entry; instead, when entry is a table (not chain/reuse) preserve its metadata by constructing action_entry as a table with fn = entry.fn or entry, and copy prefix, postfix, desc, header, and field_index into it (similar to the reuse branch) so opts.actions passed to actions.expect and header rendering retain those fields; update the else branch in the for key, entry in pairs(accept) loop to build this metadata-preserving table.
456-468:⚠️ Potential issue | 🟠 MajorSIGWINCH handlers currently lose
{+}/{q}context.The bridge always invokes
handler({}, {}), so Neovim-onlyreload/exec_silent/ function binds run without selection or query data. If one of those is the only reload-style handler,__transform_reload_cmdis never initialized either, so the returned action can collapse to a no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 456 - 468, The SIGWINCH bridge is calling handler({}, {}) which strips selection/query context and breaks reload-style binds; update the win.on_SIGWINCH callback (registered with handler_key via M._make_handler) to supply the real items and query instead of empty tables — e.g. read the last stored context from opts.__sigwinch_triggers[handler_key] (or another per-handler slot on opts) and call handler(stored_items_or_empty, {q = stored_query_or_empty}) so __transform_reload_cmd gets initialized and reload/exec_silent/function binds receive {+}/{q} as expected.
138-140:⚠️ Potential issue | 🟠 MajorHandle builtin actions in the
{"action", desc = ...}form.
{"toggle-help", desc = ...}is currently normalized as a direct fzf action, so the Neovim-side handler never runs once a description is attached.Suggested fix
if type(value) == "table" and type(value[1]) == "string" then - return { fzf_action = value[1], desc = value.desc } + if BUILTIN_ACTIONS[value[1]] then + return { builtin = value[1], desc = value.desc } + end + return { fzf_action = value[1], desc = value.desc } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 138 - 140, The current normalization in binds.lua treats any table like {"toggle-help", desc=...} as a direct fzf action (returning {fzf_action = ...}) which bypasses Neovim-side builtin handlers; update the table-to-action normalization so that when value[1] is a builtin action name (e.g., "toggle-help", "toggle-preview" — check against your builtin_actions list or table) you return a descriptor that preserves it as a builtin action (for example { action = value[1], desc = value.desc }) instead of { fzf_action = value[1], desc = ... }, so the Neovim handler for builtins in the code that consumes this normalized object will run.
408-415:⚠️ Potential issue | 🟠 MajorPer-key
field_indexcan't share the hardcoded consolidated bind.This bind always appends
{+} {q} {n}, but transform keys can still declare their ownfield_index. Events handle that case below; key transforms need the same grouping or splitting, otherwise those handlers receive the wrong argv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 408 - 415, The current transform bind creation (using transform_keys, bind_entries, base_cmd) always appends the consolidated "{+} {q} {n}" placeholders which breaks per-key field_index handling; update the logic to either group transform_keys by their declared field_index/placeholder pattern or emit separate bind entries per key so each transform key carries its own correct placeholders (respect any ":field_index" or similar suffix on individual transform keys) instead of a single hardcoded "{+} {q} {n}" for all keys; locate the code assembling transform_keys and bind_entries and change it to build per-key or per-group format strings that preserve each key's field_index before inserting into bind_entries.
105-116:⚠️ Potential issue | 🟠 MajorKeep source-specific function semantics during normalization.
This helper maps every non-event function to
{ accept = true }, but legacykeymap.fzf = functionentries used the execute-silent path whileactions = functionused accept. Unified mode now turns those keymap callbacks into accept-and-close bindings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/binds.lua` around lines 105 - 116, The normalization currently forces every bare function to become an accept action, which loses source-specific semantics; update normalize_value to accept an additional source flag (e.g., is_action or is_keymap) alongside the existing is_event and, when type(value) == "function", only set { fn = value, accept = true } if the callsite indicates an action (is_action), set { fn = value, event = true } if is_event, and otherwise return { fn = value } (preserving the execute-silent/keymap behavior); update all callers of normalize_value to pass the appropriate source flag so keymap.fzf callbacks remain execute-silent while actions/functions remain accept-and-close.lua/fzf-lua/win.lua (1)
246-258:⚠️ Potential issue | 🟠 MajorGuard unified preview toggles from re-entering the SIGWINCH handlers.
With
_fzf_toggle_prev_bind = true, the unified builtin handlers callwin.toggle_preview()/win.toggle_preview_cw(), which already queue a SIGWINCH action, and then the transform handler returns the fzf preview action again. One keypress can therefore double-apply the preview change and leave the fzf-side layout out of sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 246 - 258, The SIGWINCH transform handlers ("toggle-preview" and "toggle-preview-cw" registered via self.on_SIGWINCH) can re-enter the same handlers by calling win.toggle_preview()/win.toggle_preview_cw() while _fzf_toggle_prev_bind is true, causing double-applies; fix by short-circuiting the transform path when the bind flag is set: inside the two anonymous handlers (the one returning "toggle-preview" or change-preview-window(...) and the one for "toggle-preview-cw") check self._fzf_toggle_prev_bind and return nil/skip returning an fzf action when it's true so that calling toggle_preview()/toggle_preview_cw() (which already queues SIGWINCH) does not additionally emit the fzf-side action; reference symbols: self.on_SIGWINCH, "toggle-preview", "toggle-preview-cw", self._fzf_toggle_prev_bind, toggle_preview(), toggle_preview_cw(), normalize_preview_layout().str.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/binds.lua`:
- Around line 572-580: The exec_silent branch currently calls entry.fn and
returns an empty string, which drops any wrapper actions; update the handler for
entry.exec_silent so it still emits the wrapper actions (entry.prefix and
entry.postfix) while keeping the core fzf action empty: after invoking
entry.fn(items, opts) (if it's a function), build and return the final action
string by concatenating entry.prefix (if present), the empty core action, and
entry.postfix (if present) so wrappers like select-all+... are preserved;
reference the symbols entry.exec_silent, entry.fn, entry.prefix, and
entry.postfix in your change.
In `@lua/fzf-lua/types.lua`:
- Around line 239-241: The exported type for __sigwinch_triggers is incorrect:
it currently declares values as function but binds.build_transform_binds()
stores handler-key strings and win.lua treats them as opaque handler names.
Update the annotation for __sigwinch_triggers (the field named
__sigwinch_triggers in the types file) from table<string, function> to
table<string, string> so downstream tooling sees strings instead of functions;
no code changes beyond updating that type annotation are needed.
---
Duplicate comments:
In `@lua/fzf-lua/binds.lua`:
- Around line 69-96: The function normalize_key currently returns early for keys
not wrapped in angle brackets, bypassing the special_neovim_only lookup and
leaving unsupported fzf-style special keys (e.g., ctrl-enter, shift-enter,
alt-esc, ctrl-bs, shift-cr) on the normal --bind path; modify normalize_key so
that raw fzf-style special keys are still checked against the
special_neovim_only table (move or duplicate the special-key check before the
early return or adjust the early-return branch to consult special_neovim_only)
so those keys are routed to the SIGWINCH bridge; reference normalize_key and the
special_neovim_only table to locate where to change the control flow.
- Around line 216-227: The code currently removes binds by setting
merged[fzf_key] = nil when opts.binds supplies false, which loses the override
and allows older binds from opts.actions or opts.keymap.fzf to be resurrected;
instead preserve a tombstone in merged for that key (e.g., assign a marker
object rather than nil) so the later cleanup/merge logic can detect an explicit
unbind. Update the branch that handles value == false to set merged[fzf_key] to
a sentinel table with fields like _source = "binds", _nvim_key = key, and a flag
such as _disabled = true (or similar) so normalize_key/normalize_value consumers
and the later cleanup know this was intentionally disabled. Ensure the rest of
the merge/cleanup logic treats this sentinel as an explicit override.
- Around line 329-350: The current rebuild of accept entries in binds.lua drops
metadata in the else branch by assigning action_entry = entry.fn or entry;
instead, when entry is a table (not chain/reuse) preserve its metadata by
constructing action_entry as a table with fn = entry.fn or entry, and copy
prefix, postfix, desc, header, and field_index into it (similar to the reuse
branch) so opts.actions passed to actions.expect and header rendering retain
those fields; update the else branch in the for key, entry in pairs(accept) loop
to build this metadata-preserving table.
- Around line 456-468: The SIGWINCH bridge is calling handler({}, {}) which
strips selection/query context and breaks reload-style binds; update the
win.on_SIGWINCH callback (registered with handler_key via M._make_handler) to
supply the real items and query instead of empty tables — e.g. read the last
stored context from opts.__sigwinch_triggers[handler_key] (or another
per-handler slot on opts) and call handler(stored_items_or_empty, {q =
stored_query_or_empty}) so __transform_reload_cmd gets initialized and
reload/exec_silent/function binds receive {+}/{q} as expected.
- Around line 138-140: The current normalization in binds.lua treats any table
like {"toggle-help", desc=...} as a direct fzf action (returning {fzf_action =
...}) which bypasses Neovim-side builtin handlers; update the table-to-action
normalization so that when value[1] is a builtin action name (e.g.,
"toggle-help", "toggle-preview" — check against your builtin_actions list or
table) you return a descriptor that preserves it as a builtin action (for
example { action = value[1], desc = value.desc }) instead of { fzf_action =
value[1], desc = ... }, so the Neovim handler for builtins in the code that
consumes this normalized object will run.
- Around line 408-415: The current transform bind creation (using
transform_keys, bind_entries, base_cmd) always appends the consolidated "{+} {q}
{n}" placeholders which breaks per-key field_index handling; update the logic to
either group transform_keys by their declared field_index/placeholder pattern or
emit separate bind entries per key so each transform key carries its own correct
placeholders (respect any ":field_index" or similar suffix on individual
transform keys) instead of a single hardcoded "{+} {q} {n}" for all keys; locate
the code assembling transform_keys and bind_entries and change it to build
per-key or per-group format strings that preserve each key's field_index before
inserting into bind_entries.
- Around line 105-116: The normalization currently forces every bare function to
become an accept action, which loses source-specific semantics; update
normalize_value to accept an additional source flag (e.g., is_action or
is_keymap) alongside the existing is_event and, when type(value) == "function",
only set { fn = value, accept = true } if the callsite indicates an action
(is_action), set { fn = value, event = true } if is_event, and otherwise return
{ fn = value } (preserving the execute-silent/keymap behavior); update all
callers of normalize_value to pass the appropriate source flag so keymap.fzf
callbacks remain execute-silent while actions/functions remain accept-and-close.
In `@lua/fzf-lua/win.lua`:
- Around line 246-258: The SIGWINCH transform handlers ("toggle-preview" and
"toggle-preview-cw" registered via self.on_SIGWINCH) can re-enter the same
handlers by calling win.toggle_preview()/win.toggle_preview_cw() while
_fzf_toggle_prev_bind is true, causing double-applies; fix by short-circuiting
the transform path when the bind flag is set: inside the two anonymous handlers
(the one returning "toggle-preview" or change-preview-window(...) and the one
for "toggle-preview-cw") check self._fzf_toggle_prev_bind and return nil/skip
returning an fzf action when it's true so that calling
toggle_preview()/toggle_preview_cw() (which already queues SIGWINCH) does not
additionally emit the fzf-side action; reference symbols: self.on_SIGWINCH,
"toggle-preview", "toggle-preview-cw", self._fzf_toggle_prev_bind,
toggle_preview(), toggle_preview_cw(), normalize_preview_layout().str.
🪄 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: a843b962-fff1-4657-8611-1eb0e318ad42
📒 Files selected for processing (4)
lua/fzf-lua/binds.lualua/fzf-lua/core.lualua/fzf-lua/types.lualua/fzf-lua/win.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- lua/fzf-lua/core.lua
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
2 times, most recently
from
33309de to
ef85035
Compare
... single table routed into a single fzf "transform".
Simplifies the config significantly (no more actions /
keymap.{fzf|builtin}), use a single "binds" table instead.
All keybinds/events which require complex handling have a single
transform RPC endpoint, this also enables to bind neovim keys using
the SIGWINCH method.
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
from
ef85035 to
440ef9b
Compare
Add lua/fzf-lua/binds.lua (~740 lines) implementing the consolidated
bind transform system described in docs/specs/2026-03-27-unified-binds-design.md.
WHAT: All complex binds (lua functions, reload, exec_silent, builtin
actions, events) are routed through a single --bind=key1,key2,...:transform:CMD
entry point using shell.pipe_wrap_fn. Simple fzf-native actions (abort,
half-page-down, etc.) stay as direct --bind=key:action. Accept actions
use print(key)+accept so fzf can exit. Neovim-only keys (e.g. <C-Enter>)
use SIGWINCH bridge terminal keymaps.
WHY: Eliminates the need for separate convert_reload_actions and
convert_exec_silent_actions pipelines, and removes neovim terminal
keymaps for builtin actions (toggle-preview, toggle-help, etc.) when
fzf >= 0.59. All key dispatch goes through fzf's own bind system,
making binds work consistently regardless of focus.
KEY DESIGN DECISIONS:
- Gated to fzf >= 0.59 (can_unified check); legacy path unchanged
- Accept both old (keymap.fzf/keymap.builtin/actions) and new (binds)
formats indefinitely, no deprecation
- 5 bind categories: Direct, Accept, Transform, SIGWINCH, Dropped
- Events use __evt__<name> prefix since $FZF_KEY is empty for events
- For builtin previewer without split, toggle-preview does NOT send
fzf actions (fzf's preview pane is zero-width)
- { fn, actions.resume } backward compat converted to { fn, reload=true }
- Precedence: binds > actions > keymap.fzf > keymap.builtin
FILES CHANGED:
- lua/fzf-lua/binds.lua (new): normalize_binds(), build_transform_binds(),
classify_bind(), _make_handler(), _execute_builtin(), _create_dispatch_handler(),
can_unified()
- lua/fzf-lua/core.lua: import binds module, gate fzf_wrap to call
binds.build_transform_binds when can_unified
- lua/fzf-lua/win.lua: unified path in setup_keybinds (SIGWINCH terminal
keymaps, toggle-preview SIGWINCH handlers, Esc fix, early return);
nil guard in on_SIGWINCH resize handler for window reuse safety
fix(binds): convert shift+letter to uppercase for fzf key format
neovim_bind_to_fzf converts <M-S-j> to 'alt-shift-j' but fzf expects
'alt-J' (uppercase letter). Collapse 'shift-<letter>' to the uppercase
letter at the end of the key string. Only applies to single ASCII
letters, not special keys (shift-down, shift-tab stay as-is).
ci: fix lint
fix(binds): pcall dispatch handler to prevent fzf hang on errors
Wrap the transform handler call in the dispatch function with pcall
so that errors in user-defined actions (e.g. the builtin "error" fn)
do not crash the headless nvim process and leave fzf hanging. On
error, log a warning via utils.warn and return empty string instead.
This mirrors the resilience of the legacy execute-silent path where
errors in the pipe-wrapped nvim instance are non-fatal.
fix(win): toggle-preview feedkey fallback on Windows in unified path
On Windows, SIGWINCH is not available so the unified path cannot use
the signal-based mechanism to tell fzf to toggle its preview pane.
The unified path was also skipping the legacy keymap scanning that
sets _fzf_toggle_prev_bind, leaving it nil and breaking all
toggle-preview functionality.
Fix by adding an elseif branch for Windows that scans keymap.builtin
for toggle-preview, writes the fzf key back to keymap.fzf, and sets
_fzf_toggle_prev_bind — mirroring what the legacy path does at
lines 321-344. This lets toggle_preview() feed the key directly to
fzf without needing SIGWINCH.
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
from
cccd570 to
dfb7149
Compare
…re.lua condition
Fix six bugs found during code review of the unified bind system:
1. normalize_key: Check SPECIAL_NEOVIM_ONLY for raw fzf-style keys.
Previously only neovim-format keys (<C-Enter>) were checked, so
raw fzf-style unsupported keys like "ctrl-enter" bypassed the
SIGWINCH bridge and hit fzf as invalid binds.
2. normalize_value: Add source parameter. Bare functions from
keymap.fzf now return {fn, exec_silent=true} to preserve legacy
execute-silent behavior (core.lua create_fzf_binds converts bare
fns to execute-silent:...). Other sources keep accept=true.
3. normalize_value: Check BUILTIN_ACTIONS for table-with-string[1]
values (e.g. {"toggle-help", desc=...}). Previously these were
always treated as direct fzf actions, missing the builtin handler.
4. unbind (value==false): Clear opts.actions and opts.keymap.fzf
entries for the removed key so legacy tables cannot resurrect the
binding via actions.expect()/create_fzf_binds().
5. ACCEPT branch metadata: Preserve desc, prefix, postfix, header,
field_index in the else branch (non-chain, non-reuse entries)
instead of collapsing to bare function.
6. exec_silent handler: Preserve prefix/postfix wrapper actions in
the returned fzf action string instead of always returning "".
7. core.lua fzf_wrap: Change condition from
`convert_actions and type(opts.actions)=="table"` to
`convert_actions and (binds.can_unified(opts) or type(opts.actions)=="table")`
so pickers using opts.binds (without opts.actions) still enter
the unified path.
fix(win): disable unified bind transform on Windows
The consolidated transform shell command (BASE_CMD generated by
shell.pipe_wrap_fn) does not work with cmd.exe on Windows. When fzf
executes the transform via `$SHELL -c BASE_CMD`, the command fails
silently, causing fzf to show no items at all.
Disable the unified path on Windows by adding `not utils.__IS_WINDOWS`
to can_unified(). Windows always uses the legacy path (execute-silent
binds via convert_exec_silent_actions) which works correctly.
This also makes the setup_keybinds Windows toggle-preview fix dead
code (reached only when can_unified returns true), but it is kept
for a potential future re-enable after verifying the transform
command works on Windows.
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
from
dfb7149 to
839fa3f
Compare
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
from
839fa3f to
5f6ea90
Compare
Preview scroll actions (preview-half-page-down, preview-page-down, etc.) in keymap.builtin go through the transform handler which calls win.lua functions operating on the neovim preview buffer. With fzf previewers (bat, git status/commits), the preview is in fzf pane so these functions are no-ops and scrolling does nothing. Fix: in classify_bind, when a preview action matches a whitelist of valid fzf preview action names and the previewer is fzf-native, set entry.fzf_action and return DIRECT so fzf processes it natively. Detect fzf previewer via opts.previewer (set for explicit previewer like "bat") or opts.preview (set by providers like git_commits when the preview command is a fzf --preview string). Only when BOTH are nil is the previewer the neovim builtin overlay. Whitelist prevents invalid fzf action names (preview-reset, preview-ts-ctx-*, etc.) from being routed as DIRECT which would cause fzf to error.
ibhagwan
force-pushed
the
feat/consolidated_bind_transform
branch
from
ed6994d to
a9d147a
Compare
Covers all normalize_binds scenarios per the unified-binds-design spec:
- key normalization: neovim format, fzf format, shift+letter uppercase,
neovim-only keys (ctrl-enter, alt-esc), function keys, alt keys
- value normalization: strings (fzf action, builtin), functions
(actions/exec_silent accept), tables with fn (metadata preservation),
string [1] (help format), function arrays (chain), backward compat
{fn, actions.resume} reload, false/nil/true removal
- merge precedence: binds > actions > keymap.fzf > keymap.builtin,
across all combinations
- unbind: removes from merged, clears opts.actions and opts.keymap.fzf
- source tracking: _source and _nvim_key for all 4 sources
- empty/missing tables: graceful handling
- internal actions: underscore prefix excluded
- fzf events: event=true for load/start/resize/change/focus/result
- complex scenarios: mixed sources, cross-layer unbind, direct/accept/transform
Collaborator
Not sure why I miss the message. So now all defined keys are wrap inside transform? "comma-separated key/event lists" this is smart, it can make thing shorten. |
Collaborator
|
Related: #2450 (comment) |
phanen
reviewed
Mar 30, 2026
| |------|-----------|-----------|----------|-----------------| | ||
| | **Full** | fzf >= 0.59, neovim terminal | Yes | Yes | Yes (via SIGWINCH bridge) | | ||
| | **Partial** | fzf >= 0.59, fzf-tmux or CLI profile | Yes | No | Dropped (no terminal buffer) | | ||
| | **Legacy** | fzf < 0.59 | No (current paths) | Partial (>= 0.46) | Only via `keymap.builtin` | |
Collaborator
There was a problem hiding this comment.
We have one more way, both latest skim/fzf support socket rpc.
Collaborator
There was a problem hiding this comment.
In latest fzf, "win.lua" tnoremap seems also replaced by sigwinch? it leading to one more RTT.
Also maybe this related to this: #2634 (comment).
Owner
Author
There was a problem hiding this comment.
We have one more way, both latest skim/fzf support socket rpc.
You think we can make this work in skim even without support for transform?
phanen
reviewed
Mar 30, 2026
| -- Build the fzf action string: unbind+reload(cmd)+postfix | ||
| local parts = {} | ||
| if type(entry.prefix) == "string" then | ||
| table.insert(parts, entry.prefix:gsub("%+$", "")) |
Collaborator
There was a problem hiding this comment.
gsub return 2 args
[Fzf-lua] transform handler error [enter]: /home/phan/b/fzf-lua/lua/fzf-lua/binds.lua:648: bad argument #2 to 'insert' (number expected, got string)
Owner
Author
There was a problem hiding this comment.
The code needs more work for sure, this is model generated.
phanen
reviewed
Mar 30, 2026
| table.insert(parts, string.format("reload(%s)", reload_cmd)) | ||
| end | ||
| if type(entry.postfix) == "string" then | ||
| table.insert(parts, entry.postfix:gsub("^%+", "")) |
phanen
reviewed
Mar 30, 2026
| -- Preserve prefix/postfix wrappers (e.g. "select-all+...") | ||
| local parts = {} | ||
| if type(entry.prefix) == "string" then | ||
| table.insert(parts, entry.prefix:gsub("%+$", "")) |
phanen
reviewed
Mar 30, 2026
| table.insert(parts, entry.prefix:gsub("%+$", "")) | ||
| end | ||
| if type(entry.postfix) == "string" then | ||
| table.insert(parts, entry.postfix:gsub("^%+", "")) |
Collaborator
|
|
Collaborator
diff --git a/lua/fzf-lua/config.lua b/lua/fzf-lua/config.lua
index 7159e63..eb1b7f4 100644
--- a/lua/fzf-lua/config.lua
+++ b/lua/fzf-lua/config.lua
@@ -375,7 +375,7 @@ function M.normalize_opts(opts, globals, __resume_key) ---@diagnostic disable
-- Merge values from globals
for _, k in ipairs({
- "winopts", "keymap", "fzf_opts", "fzf_colors", "fzf_tmux_opts", "hls"
+ "winopts", "keymap", "fzf_opts", "fzf_colors", "fzf_tmux_opts", "hls", "binds"
}) do
extend_opts(globals, k)
extend_opts(M.globals, k)
|
Owner
Author
Not sure why I didn’t draft this, this is far from being ready to merge, this is just an idea yet which compiles 2 ideas I had:
I’m still working on this, this was just “my” (Claude/MimoV2) first attempt at it lol |
Owner
Author
Forgot we even had this discussion :) |
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.
... single table routed into a single fzf "transform".
Simplifies the config significantly (no more actions / keymap.{fzf|builtin}), use a single "binds" table instead.
All keybinds/events which require complex handling have a single transform RPC endpoint, this also enables to bind neovim keys using the SIGWINCH method.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes