fix: resolve buffer name collision and enhance floating window UX #52
Conversation
…andlers Prevents "E95: Buffer with this name already exists" errors when Claude Code is terminated with Ctrl-C and reopened by safely managing existing buffers. - Add handle_buffer_name_collision() helper to check and clean up orphaned buffers - Extract create_terminal_exit_handler() to eliminate code duplication - Improve floating window cleanup on terminal job exit - Add robust error handling in exit callbacks
Improves user experience when using floating window mode by adding intuitive close keymaps with proper error handling. - Add <Esc> and q in normal mode to close floating window - Add <C-q> in terminal mode to close floating window - Wrap keymap setup in pcall for robust error handling
Add documentation explaining the automatic floating window keymaps to help users understand available close shortcuts.
Comprehensive tests verifying buffer collision scenarios are handled correctly without throwing E95 errors.
WalkthroughThe changes introduce robust handling for terminal buffer name collisions and lifecycle management in floating and split windows. New keymaps are added for closing floating terminal windows, and helper functions ensure unique buffer naming and safe cleanup on terminal exit. Comprehensive tests are provided to verify buffer name collision handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Neovim
participant TerminalModule
participant BufferManager
User->>Neovim: Toggle Claude Code terminal
Neovim->>TerminalModule: terminal.toggle()
TerminalModule->>BufferManager: Check for buffer name collision
alt Buffer exists and not displayed
BufferManager->>BufferManager: Delete existing buffer
BufferManager->>TerminalModule: Provide unique buffer name
else Buffer exists and displayed
BufferManager->>BufferManager: Append timestamp to buffer name
BufferManager->>TerminalModule: Provide unique buffer name
end
TerminalModule->>Neovim: Create terminal buffer with unique name
TerminalModule->>Neovim: Set up keymaps for floating window (if applicable)
Neovim->>User: Terminal window appears with correct keymaps
Note over TerminalModule,Neovim: On terminal exit, cleanup and ensure fallback window
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)`lua/**/*.lua`: Check formatting of Lua code using stylua Format Lua code using stylua Run luacheck linter on Lua code
📄 Source: CodeRabbit Inference Engine (CLAUDE.md) List of files the instruction was applied to:
`lua/claude-code*/**/*.lua`: Main plugin code should reside in '/lua/claude-code', with subdirectories for CLI integration, UI components, and context management
📄 Source: CodeRabbit Inference Engine (CLAUDE.md) List of files the instruction was applied to:
🔇 Additional comments (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
lua/claude-code/keymaps.lua (1)
142-144: Consider renaming the error variable for clarity.The variable name
error_msgshadows the parameter name in the pcall error handler.- if not success then - vim.notify('Claude Code: Error setting up floating window keymaps: ' .. tostring(error_msg), vim.log.levels.WARN) - end + if not success then + vim.notify('Claude Code: Error setting up floating window keymaps: ' .. tostring(error_msg), vim.log.levels.WARN) + endlua/claude-code/terminal.lua (2)
124-124:build_command_with_git_rootis called but not defined yet.The function
build_command_with_git_rootis called here but defined later in the file (line 216). While this works in Lua, consider moving the function definition before its first use for better readability.
196-196: Consider usingnewinstead ofenew.Using
enewwill edit a new unnamed buffer in the current window. If you want to ensure a new window is created, usenewinstead.- vim.cmd('enew') + vim.cmd('new')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lua/claude-code/config.lua(1 hunks)lua/claude-code/keymaps.lua(1 hunks)lua/claude-code/terminal.lua(6 hunks)tests/spec/terminal_buffer_name_collision_spec.lua(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`lua/**/*.lua`: Check formatting of Lua code using stylua Format Lua code using stylua Run luacheck linter on Lua code
lua/**/*.lua: Check formatting of Lua code using stylua
Format Lua code using stylua
Run luacheck linter on Lua code
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
lua/claude-code/config.lualua/claude-code/keymaps.lualua/claude-code/terminal.lua
`lua/claude-code*/**/*.lua`: Main plugin code should reside in '/lua/claude-code', with subdirectories for CLI integration, UI components, and context management
lua/claude-code*/**/*.lua: Main plugin code should reside in '/lua/claude-code', with subdirectories for CLI integration, UI components, and context management
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
lua/claude-code/config.lualua/claude-code/keymaps.lualua/claude-code/terminal.lua
`tests/**/*.lua`: Test files for plugin functionality should be placed in the '/tests' directory
tests/**/*.lua: Test files for plugin functionality should be placed in the '/tests' directory
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
tests/spec/terminal_buffer_name_collision_spec.lua
🧠 Learnings (1)
tests/spec/terminal_buffer_name_collision_spec.lua (1)
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T19:40:53.412Z
Learning: Applies to tests/**/*.lua : Test files for plugin functionality should be placed in the '/tests' directory
🪛 GitHub Actions: CI
tests/spec/terminal_buffer_name_collision_spec.lua
[error] 215-215: Test failure: toggle should succeed when handling buffer name collision. Error: Invalid buffer id: 101. Expected true but got false.
[error] 258-258: Test failure: toggle should succeed when buffer is displayed. Error: Invalid buffer id: 101. Expected true but got false.
[error] 351-351: Test failure: file command should be called with timestamped name. Expected true but got false.
[error] 376-376: Test failure: toggle should succeed when existing buffer is invalid. Error: Invalid buffer id: 101. Expected true but got false.
[error] 431-431: Test failure: toggle should succeed when handling multiple collisions. Error: Invalid buffer id: 101. Expected true but got false.
🔇 Additional comments (8)
lua/claude-code/config.lua (1)
136-139: Documentation accurately reflects the implemented floating window keymaps.The comments correctly document the automatic keymaps added for floating windows in the keymaps module.
lua/claude-code/keymaps.lua (1)
111-145: Well-implemented floating window keymaps with proper error handling.The keymaps correctly implement the documented behavior and the error handling ensures robustness.
tests/spec/terminal_buffer_name_collision_spec.lua (1)
1-448: Comprehensive test coverage for buffer name collision scenarios.The test suite thoroughly covers various collision scenarios for both floating and split windows, including edge cases. The mocking structure is well-organized.
lua/claude-code/terminal.lua (5)
73-133: Robust terminal job lifecycle management for floating windows.The implementation correctly checks for running terminal jobs and restarts them with proper exit handlers when reusing buffers.
135-156: Effective buffer name collision handling.The logic correctly handles both cases: deleting orphaned buffers and using timestamped names for displayed buffers.
158-208: Well-structured terminal exit handler with comprehensive error handling.The extracted function successfully eliminates code duplication and provides robust cleanup with proper error handling and fallback window creation.
448-450: Good use of terminal exit handler for proper cleanup.The terminal job creation now includes the exit handler, ensuring proper cleanup when the terminal process exits.
454-455: Consistent buffer name collision handling across window types.Buffer name collision handling is properly applied for both floating and split windows.
… error handling - Fix create_float function calls with consistent parameter passing - Use 'new' instead of 'enew' for better window creation behavior - Add defensive checks for optional parameters in floating window logic - Improve comment clarity for conditional terminal job restart logic Addresses test failures and CodeRabbit review feedback.
Add missing mocks for nvim_get_option_value, vim.b, nvim_set_option_value, and jobwait functions that are required by the buffer collision handling code. Fixes "Invalid buffer id: 101" test failures in CI.
- Add missing vim API mocks (nvim_get_option_value, vim.b, jobwait) - Fix timestamp handling with predictable mocking and proper cleanup - Improve collision handler to handle multiple levels of name conflicts - Update test expectations to match enhanced collision logic All 73 tests now pass locally and should pass in CI.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/spec/terminal_buffer_name_collision_spec.lua (2)
138-142: Fix buffer creation mock to prevent test failures.The buffer creation mock doesn't track newly created buffers, which can cause "Invalid buffer id" errors as noted in past reviews.
Apply the suggested fix from the previous review:
_G.vim.api.nvim_create_buf = function(listed, scratch) buffer_counter = buffer_counter + 1 + existing_buffers[buffer_counter] = nil -- Track the buffer as existing return buffer_counter end
8-502: Add comprehensive os.time mocking as suggested in past review.The test suite uses
os.time()in collision handling but lacks consistent module-level mocking, which can cause test instability.Add the module-level mock as suggested in the previous review:
describe('terminal buffer name collision handling', function() local config local claude_code local git local vim_cmd_calls = {} local existing_buffers = {} local deleted_buffers = {} local nvim_buf_set_name_calls = {} + local original_os_time = os.time + + -- Mock os.time for consistent timestamps + os.time = function() + return 1234567890 + end before_each(function()And add cleanup:
+ after_each(function() + -- Restore original os.time + os.time = original_os_time + end)
🧹 Nitpick comments (2)
lua/claude-code/terminal.lua (2)
73-73: Document the new parameter additions.The function signature has been extended with new parameters that significantly change its behavior. Consider adding documentation for the new parameters to maintain API clarity.
--- Create a floating window for Claude Code --- @param config table Plugin configuration containing window settings --- @param existing_bufnr number|nil Buffer number of existing buffer to show in the float (optional) +--- @param claude_code table|nil Main plugin module for terminal job management (optional) +--- @param git table|nil Git module for building commands (optional) +--- @param instance_id string|nil Instance identifier for cleanup tracking (optional) --- @return number Window ID of the created floating window --- @private
135-168: Enhance collision handling robustness.The collision handling logic is solid but has potential improvements:
- The 10-attempt safety limit could be configurable
- Using
os.time()for timestamps could cause collisions if called rapidly within the same second- Consider using a more granular timestamp or UUID for uniqueness
local function handle_buffer_name_collision(buffer_name, current_bufnr) local existing_bufnr = vim.fn.bufnr(buffer_name) if existing_bufnr ~= -1 and existing_bufnr ~= current_bufnr then -- Only delete if the buffer is valid and not currently displayed if vim.api.nvim_buf_is_valid(existing_bufnr) then local buf_windows = vim.fn.win_findbuf(existing_bufnr) if #buf_windows == 0 then -- Buffer exists but isn't displayed, safe to delete vim.api.nvim_buf_delete(existing_bufnr, {force = true}) else -- Buffer is being displayed, use a different name with timestamp local base_name = buffer_name - local timestamp = os.time() + -- Use more granular timestamp to avoid collisions + local timestamp = tostring(os.time()) .. '-' .. tostring(math.random(1000, 9999)) local attempt = 0 repeat if attempt == 0 then buffer_name = base_name .. '-' .. timestamp else buffer_name = base_name .. '-' .. timestamp .. '-' .. attempt end existing_bufnr = vim.fn.bufnr(buffer_name) attempt = attempt + 1 until existing_bufnr == -1 or attempt > 10 end end end return buffer_name end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lua/claude-code/terminal.lua(8 hunks)tests/spec/terminal_buffer_name_collision_spec.lua(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`lua/**/*.lua`: Check formatting of Lua code using stylua Format Lua code using stylua Run luacheck linter on Lua code
lua/**/*.lua: Check formatting of Lua code using stylua
Format Lua code using stylua
Run luacheck linter on Lua code
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
lua/claude-code/terminal.lua
`lua/claude-code*/**/*.lua`: Main plugin code should reside in '/lua/claude-code', with subdirectories for CLI integration, UI components, and context management
lua/claude-code*/**/*.lua: Main plugin code should reside in '/lua/claude-code', with subdirectories for CLI integration, UI components, and context management
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
lua/claude-code/terminal.lua
`tests/**/*.lua`: Test files for plugin functionality should be placed in the '/tests' directory
tests/**/*.lua: Test files for plugin functionality should be placed in the '/tests' directory
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
tests/spec/terminal_buffer_name_collision_spec.lua
🧠 Learnings (1)
tests/spec/terminal_buffer_name_collision_spec.lua (1)
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-02T19:40:53.412Z
Learning: Applies to tests/**/*.lua : Test files for plugin functionality should be placed in the '/tests' directory
🔇 Additional comments (6)
lua/claude-code/terminal.lua (4)
170-220: Excellent error handling in terminal exit handler.The
create_terminal_exit_handlerfunction demonstrates robust error handling with comprehensive cleanup logic. The fallback window creation and error notification provide good user experience.
289-289: Maintain backward compatibility in create_split.The explicit nil parameters maintain backward compatibility while supporting the new collision handling features.
466-467: Good use of collision handling in buffer creation.The integration of
handle_buffer_name_collisionin both floating and split window creation paths ensures consistent behavior across window types.
395-395: Parameter threading inhandle_existing_instanceis consistent
- lua/claude-code/terminal.lua:531 – the call passes
bufnr, config, claude_code, git, instance_id, matching the updated signatureAll call sites have been updated; no further changes required.
tests/spec/terminal_buffer_name_collision_spec.lua (2)
228-502: Excellent test coverage for collision scenarios.The test suite comprehensively covers:
- Basic collision handling for both floating and split windows
- Collision resolution when buffers are/aren't displayed
- Edge cases with invalid buffers
- Multiple collision scenarios with timestamp suffixes
The test structure is well-organized and the assertions validate both the absence of errors and the correct buffer management behavior.
24-225: Comprehensive API mocking setup.The extensive mocking of Neovim APIs provides a solid foundation for testing the collision handling logic. The mocks correctly simulate buffer states, window management, and job control.
- Fix buffer creation mock to track newly created buffers (prevents 'Invalid buffer id' errors) - Add module-level os.time mock with proper cleanup for consistent test timestamps - Add validation for terminal command before calling vim.fn.termopen - Document new function parameters in create_float function - Enhance timestamp generation with random numbers for better collision avoidance - Update test assertions to match new timestamp-random format All 73 tests now pass successfully.
|
Hey @krishna-bala I am feeling a little better here and can do some work, any thoughts on this one? It's your PR. Whats the status on it? |
Summary
Problem
When users terminated Claude Code with Ctrl-C, orphaned buffers would sometimes remain, causing "E95: Buffer with this name already exists" errors on subsequent launches. Additionally, floating windows lacked intuitive close shortcuts.
Solution
Buffer Collision Handling
handle_buffer_name_collision()helper function to safely manage existing buffersFloating Window UX Improvements
<Esc>andqkeymaps in normal mode to close floating windows<C-q>keymap in terminal mode for convenient closingCode Quality Improvements
create_terminal_exit_handler()to eliminate ~80 lines of code duplicationTest Plan
Files Changed
lua/claude-code/terminal.lua- Core collision handling and refactored exit callbackslua/claude-code/keymaps.lua- Enhanced floating window keymaps with error handlinglua/claude-code/config.lua- Documentation for new keymapstests/spec/terminal_buffer_name_collision_spec.lua- Comprehensive collision testsSummary by CodeRabbit
New Features
<Esc>,q(normal mode), and<C-q>(terminal mode).Bug Fixes
Tests
Documentation