fix: prevent closing adjacent window when writing rename buffer with :wq #536
Conversation
WalkthroughBufWriteCmd in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BufWriteCmd
participant Timer
participant post_write
participant UI
User->>BufWriteCmd: Trigger write
alt Error
BufWriteCmd->>BufWriteCmd: Log error
BufWriteCmd-->>User: Return (early)
else Success
BufWriteCmd->>BufWriteCmd: Compute result, capture winid/bufnr
BufWriteCmd->>Timer: Schedule s:post_write(resolve, result, winid, bufnr)
BufWriteCmd-->>User: Return (async pending)
Timer->>post_write: Fire callback
post_write->>UI: Close window if winid contains bufnr
post_write->>post_write: Call Resolve(result)
post_write-->>User: Complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (1)
autoload/fern/internal/replacer.vim (1)
107-111: Async resolution approach correctly defers window closure.The timer-based deferral allows
:wqto complete before custom window management occurs, solving the issue of closing adjacent windows. Settingnomodifiedensures no unsaved changes prompt.Consider adding a comment explaining why the timer is necessary:
endtry set nomodified let l:Resolve = b:fern_replacer_resolve let winnr = winnr() let bufnr = bufnr() + " Defer window closure via timer to prevent :wq from closing adjacent windows call timer_start(0, { -> s:post_write(l:Resolve, result, winnr, bufnr) }) endfunction
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autoload/fern/internal/replacer.vim(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest, stable)
🔇 Additional comments (3)
autoload/fern/internal/replacer.vim (3)
105-105: Good addition of explicit return after error handling.This ensures the function exits properly after logging the error, preventing any subsequent code from executing.
116-116: Based on my verification, I found that the syntaxexecute winnr.'wincmd c'is a documented pattern for closing a window by number, but it has important caveats. This pattern won't work if the window isn't in the current tab page.Looking at the code context on line 116, the function
s:post_writealready validates that the buffer exists in the window viawinbufnr(a:winnr) == a:bufnrbefore attempting to close it. This suggests the developer is aware of window validation, though it doesn't explicitly check tab page constraints.The syntax used in line 116 appears to be a valid (if quirky) VimScript pattern that is documented in community discussions, but developers working with this code should be aware of its tab page limitation.
Syntax is valid but has tab-page constraints.
The
execute winnr.'wincmd c'pattern is a recognized approach for closing a specific window, but it won't work if the window isn't in the current tab page. The existing buffer check on line 115 provides partial validation, but ensure this code path only operates on windows in the active tab.
114-120: No changes needed—the code is correctly designed for the replacer use case.The concerns raised are based on generic window manipulation patterns, but don't apply here:
Multiple windows showing the buffer: The replacer is a temporary, single-purpose buffer used only for this replacement operation. It's never intentionally displayed in multiple windows, so closing only the specified window is the intended behavior.
Error handling on window close: The
winbufnrguard on line 115 guarantees the window exists and contains the expected buffer. If the check passes,wincmd cwill succeed. This pattern is consistent throughout the codebase and sufficient for the use case.
autoload/fern/internal/replacer.vim
Outdated
| if winbufnr(a:winnr) == a:bufnr | ||
| execute a:winnr . 'wincmd c' | ||
| endif |
There was a problem hiding this comment.
How about using win_execute instead?
There was a problem hiding this comment.
Use win_execute and window-ID instead winnr.
autoload/fern/internal/replacer.vim
Outdated
| if winbufnr(a:winnr) == a:bufnr | ||
| execute a:winnr . 'wincmd c' | ||
| endif | ||
| let Resolve = a:resolve |
There was a problem hiding this comment.
Well, I'm not sure, but I think we need l: prefix here.
The rename buffer's write command handler now uses a timer to defer window closing, ensuring only the rename buffer window is closed and not any adjacent windows when using :wq command.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
autoload/fern/internal/replacer.vim (1)
107-107: Consider usingsetlocalinstead ofset.While
set nomodifiedworks in this buffer context,setlocal nomodifiedwould be more precise and avoid any potential side effects on the global setting.Apply this diff:
- set nomodified + setlocal nomodified
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autoload/fern/internal/replacer.vim(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (macos-latest, v8.2.5136)
🔇 Additional comments (3)
autoload/fern/internal/replacer.vim (3)
100-105: LGTM! Error handling correctly prevents resolution.The early return ensures that the resolve callback is not invoked when modifiers fail, which is the correct behavior. The user must either fix the error or explicitly cancel with
:q!.
108-111: Excellent fix using timer deferral.The timer-based deferral correctly addresses the issue where
:wqwould close adjacent windows. By deferring the window close and resolution to after the write command completes, only the rename buffer window is closed as intended.The captured
winidandbufnrenable safe validation ins:post_write(), and thel:prefix onResolveaddresses the past review comment.
114-120: Well-implemented post-write handler addressing past review comments.The implementation correctly:
- Validates the window still contains the expected buffer before closing (line 115), gracefully handling cases where the user manually closed the window
- Uses
win_execute()as suggested in the past review comment (line 116)- Applies the
l:prefix pattern as suggested in the past review comment (line 118)- Ensures the resolve callback is invoked after window management completes
The order of operations is correct: the window is closed first (triggering buffer wipe due to
bufhidden=wipe), then the resolve callback is invoked with the results.
|
Thanks 🎉 |
2 participants
The rename buffer's write command handler now uses a timer to defer window closing, ensuring only the rename buffer window is closed and not any adjacent windows when using
:wqcommand.Summary by CodeRabbit