fix: wrong mode after :term then quickly stopi->close->new picker->starti#2688
fix: wrong mode after :term then quickly stopi->close->new picker->starti#2688
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a deferred stop-insert helper and a global one-shot hook for invoking builtin actions: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FzfLua as actions.lua
participant Win as win.lua
participant Neovim
User->>FzfLua: select builtin entry
FzfLua->>FzfLua: resolve method via vim.F.nil_wrap(require ...)
alt global hook present
FzfLua->>Win: pass resolved fn to _G.fzf_lua_stopinsert_hack
Win->>Neovim: maybe execute :stopinsert (if ctx.mode == "nt" and mode differs)
Win->>Neovim: register one-shot User & ModeChanged autocmds
Neovim->>Win: ModeChanged to nt fires
Win->>Neovim: schedule execution of stored User autocmds
Neovim->>FzfLua: invoke resolved builtin (deferred)
else no hook
FzfLua->>FzfLua: call resolved fn() directly
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
There was a problem hiding this comment.
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/win.lua (1)
1169-1201:⚠️ Potential issue | 🟡 MinorVerify the ModeChanged→User-autocmd event ordering and add test coverage for rapid
:FzfLuainvocations from terminal mode.The mechanism depends on this synchronization:
stopinsert(ctx)registers theModeChangedlistener at line 1178, having just issuedstopinsertat line 1172.run_builtin(line 519–520) registers theUserlistener before the queuedModeChangedevent is processed.If Neovim processes the
ModeChangedevent before theUserautocmd is registered (e.g., if both are in the same event-loop cycle or if action dispatch is deferred),nvim_exec_autocmds("User", …)runs with no listener, and the selected builtin entry is dropped. Theschedule_wrapon line 1175 schedules the callback once the event fires, but does not defer the event firing itself. A race is possible if the two registration events are not strictly ordered.Additionally, the change from unconditional to conditional
stopinsert(line 1171 guard) should be tested for any non-terminal-mode workflows that previously depended on unconditional restoration (see the comment at line 1197 about restoring normal mode).🤖 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 1169 - 1201, The ModeChanged→User autocmd registration can race and drop the selected entry; in stopinsert ensure the User autocmd (the callback stored into _G.fzf_lua_stopinsert_hack) is created/registered before you register the ModeChanged autocmd or otherwise ensure ordering (e.g. assign and register the User autocmd first, or defer ModeChanged registration with vim.schedule) so nvim_exec_autocmds("User", ...) always finds the listener; update stopinsert (and the places that call it such as FzfWin:close and run_builtin) to guarantee the User handler exists before ModeChanged can fire and add tests simulating rapid :FzfLua invocations from terminal mode and a test for the conditional stopinsert guard to cover non-terminal-mode workflows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lua/fzf-lua/win.lua`:
- Around line 1169-1201: The ModeChanged→User autocmd registration can race and
drop the selected entry; in stopinsert ensure the User autocmd (the callback
stored into _G.fzf_lua_stopinsert_hack) is created/registered before you
register the ModeChanged autocmd or otherwise ensure ordering (e.g. assign and
register the User autocmd first, or defer ModeChanged registration with
vim.schedule) so nvim_exec_autocmds("User", ...) always finds the listener;
update stopinsert (and the places that call it such as FzfWin:close and
run_builtin) to guarantee the User handler exists before ModeChanged can fire
and add tests simulating rapid :FzfLua invocations from terminal mode and a test
for the conditional stopinsert guard to cover non-terminal-mode workflows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35325ec5-3f5f-420f-8603-2446f6ed2ac7
📒 Files selected for processing (2)
lua/fzf-lua/actions.lualua/fzf-lua/win.lua
Fix #2685.
reuse=trueshould also work, but it break previewer for some reason.Summary by CodeRabbit