feat: add floating window support#37
feat: add floating window support#37greggh merged 18 commits intogreggh:mainfrom krishna-bala:krishna.add-floating-window-2
Conversation
WalkthroughThe changes introduce comprehensive support for floating terminal windows in the Claude Code Neovim plugin. Documentation, configuration schema, validation logic, and terminal window creation are all updated to enable and document floating window options. Extensive tests are added to ensure correct parsing, validation, and functionality of floating window configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Neovim
participant ClaudeCodePlugin
participant TerminalWindow
User->>Neovim: Issues Claude Code toggle command
Neovim->>ClaudeCodePlugin: Calls M.toggle(config, git)
ClaudeCodePlugin->>ClaudeCodePlugin: Parses config
alt window.position == "float"
ClaudeCodePlugin->>TerminalWindow: create_float(config)
TerminalWindow-->>ClaudeCodePlugin: Returns win_id and buf
else window.position is split
ClaudeCodePlugin->>TerminalWindow: create_split(position, config)
TerminalWindow-->>ClaudeCodePlugin: Returns win_id and buf
end
ClaudeCodePlugin->>TerminalWindow: Run terminal command in buffer
TerminalWindow-->>ClaudeCodePlugin: Terminal running in window
ClaudeCodePlugin-->>Neovim: Window displayed to user
Poem
✨ 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 (4)
README.md (1)
8-8: Update test count badge to reflect current test coverage.The badge shows "44 passing" tests, but according to the PR summary, there are now 66 tests. Please update this to reflect the current test count.
-[](https://github.com/greggh/claude-code.nvim/actions/workflows/ci.yml) +[](https://github.com/greggh/claude-code.nvim/actions/workflows/ci.yml)tests/spec/config_validation_spec.lua (1)
35-89: LGTM! Comprehensive validation tests for floating window configuration.The tests thoroughly validate the float configuration structure and constraints. Good practice checking that invalid configurations don't bleed through.
Consider adding tests for edge cases:
- Empty float table
{}- Float configuration when position is not 'float'
- Valid percentage strings like "0%" or "100%"
lua/claude-code/config.lua (1)
164-227: Thorough validation logic for float configuration.The validation comprehensively checks all float properties. Consider extracting the duplicate row/col validation logic into a helper function to reduce code duplication.
+local function validate_position_value(value, field_name) + if type(value) == 'string' and value ~= 'center' then + if not value:match('^%d+%%$') then + return false, 'window.float.' .. field_name .. ' must be a number, "center", or percentage string' + end + elseif type(value) ~= 'number' and value ~= 'center' then + return false, 'window.float.' .. field_name .. ' must be a number, "center", or percentage string' + end + return true, nil +end -- Validate row and col if they exist if config.window.float.row ~= nil then - if type(config.window.float.row) == 'string' and config.window.float.row ~= 'center' then - if not config.window.float.row:match('^%d+%%$') then - return false, 'window.float.row must be a number, "center", or percentage string' - end - elseif type(config.window.float.row) ~= 'number' and config.window.float.row ~= 'center' then - return false, 'window.float.row must be a number, "center", or percentage string' - end + local valid, err = validate_position_value(config.window.float.row, 'row') + if not valid then return false, err end end if config.window.float.col ~= nil then - if type(config.window.float.col) == 'string' and config.window.float.col ~= 'center' then - if not config.window.float.col:match('^%d+%%$') then - return false, 'window.float.col must be a number, "center", or percentage string' - end - elseif type(config.window.float.col) ~= 'number' and config.window.float.col ~= 'center' then - return false, 'window.float.col must be a number, "center", or percentage string' - end + local valid, err = validate_position_value(config.window.float.col, 'col') + if not valid then return false, err end endlua/claude-code/terminal.lua (1)
137-145: Update to newer Neovim window option API.Replace deprecated
nvim_win_set_optionwith the newer API:local function configure_window_options(win_id, config) if config.window.hide_numbers then - vim.api.nvim_win_set_option(win_id, 'number', false) - vim.api.nvim_win_set_option(win_id, 'relativenumber', false) + vim.api.nvim_set_option_value('number', false, {win = win_id}) + vim.api.nvim_set_option_value('relativenumber', false, {win = win_id}) end if config.window.hide_signcolumn then - vim.api.nvim_win_set_option(win_id, 'signcolumn', 'no') + vim.api.nvim_set_option_value('signcolumn', 'no', {win = win_id}) end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)README.md(3 hunks)doc/claude-code.txt(1 hunks)lua/claude-code/config.lua(4 hunks)lua/claude-code/terminal.lua(4 hunks)tests/spec/config_spec.lua(1 hunks)tests/spec/config_validation_spec.lua(1 hunks)tests/spec/terminal_spec.lua(6 hunks)
🔇 Additional comments (23)
CHANGELOG.md (1)
13-14: Well-documented changelog entries for floating window support.The changelog entries clearly document the new floating window feature with appropriate level of detail for a changelog format.
tests/spec/config_spec.lua (4)
57-77: Comprehensive test coverage for basic floating window configuration.The test properly validates that float configuration is correctly parsed and preserved when position is set to 'float'.
79-95: Good test coverage for percentage-based dimensions.This test ensures that percentage strings for width and height are properly handled, which is important for responsive floating window sizing.
97-115: Excellent test for center positioning functionality.This test validates the 'center' positioning option, which is likely a commonly used configuration for floating windows.
117-135: Important test for default configuration fallback.This test ensures that when no float configuration is provided, sensible defaults are applied. The default values (80% width/height, center positioning, rounded border) are reasonable choices.
README.md (4)
32-32: Good addition to features list.Including floating windows in the features list appropriately highlights this new capability.
96-96: Complete position documentation.The updated position description now includes "float" as a valid option, maintaining consistency with the implementation.
101-109: Comprehensive floating window configuration documentation.The float configuration block provides complete documentation of all available options with clear descriptions and sensible default values.
208-226: Excellent practical example for floating window configuration.This example section provides clear, practical guidance for users who want to configure floating windows. The configuration shown (90% width/height, center positioning, double border) is a good demonstration of the available options.
doc/claude-code.txt (2)
94-94: Consistent position documentation in Vim help.The position field documentation now includes "float" option, maintaining consistency with README and other documentation.
100-108: Complete floating window configuration in Vim help format.The float configuration documentation follows proper Vim help format and provides comprehensive coverage of all floating window options with clear descriptions.
lua/claude-code/config.lua (2)
78-86: Good default configuration for floating windows.The default float configuration with 80% dimensions and centered positioning provides a good user experience out of the box.
348-351: Good handling of default float configuration.The deep copy ensures that users get the default float configuration when position is 'float' but no custom float config is provided.
tests/spec/terminal_spec.lua (4)
35-57: Excellent enhancement of buffer validation mocking.The improved mocking now properly simulates terminal buffer validation including buffer type and job ID checks, making tests more realistic.
172-185: Good fix for shell escaping verification.The test now properly verifies that git root paths are shell-escaped in terminal commands, preventing issues with paths containing spaces.
268-272: Improved buffer name sanitization check.The enhanced assertion now properly extracts and validates the buffer name to ensure it doesn't contain special characters.
455-644: Comprehensive floating window test suite!Excellent test coverage for the new floating window functionality:
- Fixed and percentage-based dimensions
- Centering behavior
- Buffer reuse scenarios
- Edge cases with out-of-bounds dimensions
- Proper mock setup for all Neovim APIs
The tests ensure robust floating window support.
lua/claude-code/terminal.lua (6)
33-46: Well-implemented dimension calculation logic.The function correctly handles all input types (nil, percentage strings, and numeric values) with appropriate defaults and conversions.
48-66: Excellent position calculation with viewport boundary protection.The position clamping logic ensures floating windows remain fully visible within the editor viewport, preventing partial off-screen windows. This is a thoughtful addition that improves user experience.
116-131: Consider shell compatibility for directory navigation.The
pushd/popdcommands are bash/zsh-specific and may not work in other shells (e.g., fish, sh). Consider using a more portable approach:if git_root then local quoted_root = vim.fn.shellescape(git_root) - return 'pushd ' .. quoted_root .. ' && ' .. base_cmd .. ' && popd' + return '(cd ' .. quoted_root .. ' && ' .. base_cmd .. ')' endThe subshell approach
(cd dir && cmd)works across all POSIX-compliant shells.
148-159: Good buffer name sanitization.The pattern correctly sanitizes the instance identifier to ensure valid buffer names across different file systems and git repositories.
167-182: Clean delegation pattern for floating windows.The refactoring properly delegates floating window creation while maintaining backward compatibility for split windows. The split command construction logic is correctly fixed.
298-355: Excellent refactoring with helper function usage.The code properly separates floating and split window creation paths, reuses helper functions effectively, and maintains consistent behavior across both window types. The buffer management and window configuration are well-structured.
|
@krishna-bala there is a nwe merge conflict because I merged in PR #36 Can you take a look at that and get the conflict fixed in a way you're happy with? I'm ready to merge this when that is fixed. |
Adds comprehensive floating window support with configurable dimensions, position, and border styles. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Adds documentation for floating window support with configurable dimensions, position, and border styles. Includes percentage-based sizing and automatic centering options. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Preserve existing vim.o fields when setting columns and lines in tests instead of replacing the entire table. This prevents potential issues where tests might fail if code under test accesses other vim.o fields. Addresses CodeRabbitAI feedback on floating window PR
- Add explicit assertions for window dimensions and position - Use dynamic calculations instead of hardcoded expected values - Add test case for out-of-bounds dimensions to ensure graceful handling - Makes tests more maintainable and comprehensive Addresses CodeRabbitAI feedback on floating window PR
- Extract git root command building into helper function - Add proper shell escaping using vim.fn.shellescape() - Prevents errors when git root contains spaces - Eliminates code duplication between float and split paths Addresses CodeRabbitAI feedback on floating window PR
- Add nil check for dimensions with 80% default fallback - Clamp calculated positions to ensure window stays visible on screen - Prevents floating windows from appearing partially off-screen - Makes configuration more forgiving with sensible defaults Addresses CodeRabbitAI feedback on floating window PR
- Use appropriate split command (vsplit/split) based on direction - Avoid appending 'split' to commands that already contain it - Update comment to accurately reflect the logic - Prevents 'vsplit split' duplication issue Addresses CodeRabbitAI feedback on floating window PR
- Validate buftype is 'terminal' before reusing buffer in create_float - Check terminal job is still running in toggle function - Reset stored buffer reference when buffer is no longer valid - Prevents errors from reusing invalid or non-terminal buffers - Uses pcall for safe access to terminal_job_id variable Addresses CodeRabbitAI feedback on floating window PR
- Add proper mocks for nvim_buf_get_option and nvim_buf_get_var - Mock jobwait to simulate running terminal jobs - Update git root test to expect shell-escaped paths - Make nvim_buf_is_valid check for positive buffer IDs
- Extract common window configuration logic into helper functions - Add better test coverage with edge case validation - Improve test assertions to prevent invalid data bleed-through - Update documentation to clarify position value options - Fix test expectations to match actual behavior - Reduce code duplication between floating and split window paths All 66 tests now pass successfully.
- Account for command line and status line in editor height calculation - Use editor_height = vim.o.lines - vim.o.cmdheight - 1 for more accurate centering - Update tests to reflect corrected dimension calculations - Ensures floating windows are truly centered in the available editor space This fixes the issue where floating windows appeared slightly below center.
- Resolved merge conflicts in terminal.lua - Combined floating window support with configurable shell commands - Fixed test pattern to account for shell escaping - Applied code formatting Refactored to reduce cyclomatic complexity: - validate_config: reduced from 72 to ~10 (was 44 on main) - M.toggle: reduced from 21 to ~8 (new issue from float additions) Extracted helper functions for better maintainability: - Validation helpers for each config section - Terminal operation helpers for cleaner logic All 67 tests passing, 0 lint warnings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lua/claude-code/terminal.lua (2)
104-110: Update deprecated API usage in buffer validation.The buffer validation logic is excellent, but should use the newer API as mentioned in previous reviews.
Apply this diff to update the deprecated API:
- local buftype = vim.api.nvim_buf_get_option(bufnr, 'buftype') + local buftype = vim.api.nvim_get_option_value('buftype', {buf = bufnr})
270-278: Update deprecated buffer API usage.The function uses deprecated APIs for accessing buffer options and variables.
Apply this diff to update to newer APIs:
- local buftype = vim.api.nvim_buf_get_option(bufnr, 'buftype') + local buftype = vim.api.nvim_get_option_value('buftype', {buf = bufnr}) local terminal_job_id = nil pcall(function() - terminal_job_id = vim.api.nvim_buf_get_var(bufnr, 'terminal_job_id') + terminal_job_id = vim.b[bufnr].terminal_job_id end)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)README.md(3 hunks)doc/claude-code.txt(1 hunks)lua/claude-code/config.lua(4 hunks)lua/claude-code/terminal.lua(4 hunks)tests/spec/config_spec.lua(1 hunks)tests/spec/config_validation_spec.lua(1 hunks)tests/spec/terminal_spec.lua(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- tests/spec/config_spec.lua
- doc/claude-code.txt
- README.md
- tests/spec/config_validation_spec.lua
- tests/spec/terminal_spec.lua
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
lua/claude-code/terminal.lua (4)
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Project structure should be organized with main plugin code under lua/claude-code/, plugin setup in after/plugin/, tests in tests/, and documentation in doc/ for clarity and maintainability.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Buffer names in the Claude Code Plugin include the git root path to help users easily identify which project or repository a buffer belongs to.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
🔇 Additional comments (8)
lua/claude-code/terminal.lua (3)
33-46: LGTM! Clean dimension calculation logic.The function correctly handles percentage strings, numbers, and nil values with appropriate defaults. The regex pattern
^%d+%%$properly validates percentage format.
48-66: LGTM! Robust position calculation with proper clamping.The function handles all positioning scenarios (center, percentage, numeric) and includes essential position clamping to ensure windows remain visible on screen.
375-405: Excellent modularization of the toggle function.The refactored toggle function is much cleaner and more maintainable with proper separation of concerns. The logic flow is clear and handles all edge cases appropriately.
lua/claude-code/config.lua (5)
11-23: Comprehensive documentation for floating window configuration.The documentation clearly explains all floating window configuration options with appropriate type information and examples.
85-93: Sensible default floating window configuration.The default values (80% width/height, centered positioning, rounded border) provide a good user experience out of the box.
176-245: Thorough floating window validation logic.The validation function comprehensively covers all floating window configuration options with proper type checking, format validation for percentages, and support for both string and numeric values.
439-447: Excellent cross-validation between keymaps and command variants.This validation ensures that keymap variants have corresponding command variants, preventing configuration errors at runtime.
467-473: Smart default float configuration handling.The logic correctly provides default floating window configuration when the position is set to 'float' but no float config is provided by the user.
Replace deprecated vim.api functions with modern equivalents: - Replace nvim_buf_get_option with nvim_get_option_value - Replace nvim_win_set_option with nvim_set_option_value - Replace nvim_buf_get_var with vim.b[] access - Add buffer validation in create_float function
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lua/claude-code/terminal.lua (3)
100-114: Good buffer validation, but consider consolidating logic.The buffer validation logic is comprehensive and uses the updated APIs correctly. However, this validation pattern is duplicated in the
is_valid_terminal_bufferfunction.Consider using the dedicated validation function:
-- Create buffer if we don't have an existing one local bufnr = existing_bufnr if not bufnr then bufnr = vim.api.nvim_create_buf(false, true) -- unlisted, scratch else - -- Validate existing buffer is still valid and a terminal - if not vim.api.nvim_buf_is_valid(bufnr) then - bufnr = vim.api.nvim_create_buf(false, true) -- unlisted, scratch - else - local buftype = vim.api.nvim_get_option_value('buftype', {buf = bufnr}) - if buftype ~= 'terminal' then - -- Buffer exists but is no longer a terminal, create a new one - bufnr = vim.api.nvim_create_buf(false, true) -- unlisted, scratch - end - end + -- Validate existing buffer is still a valid terminal + if not is_valid_terminal_buffer(bufnr) then + bufnr = vim.api.nvim_create_buf(false, true) -- unlisted, scratch + end end
328-328: Remove redundant buffer assignment.The buffer is already associated with the window when
nvim_open_winis called on line 117, making this assignment redundant.- -- Set current buffer to run terminal command - vim.api.nvim_win_set_buf(win_id, new_bufnr)
392-409: Simplify redundant buffer validation logic.The buffer validation and pruning logic contains redundant checks that can be streamlined for better readability and efficiency.
-- Validate existing buffer if bufnr and not is_valid_terminal_buffer(bufnr) then -- Buffer is no longer a valid terminal, reset claude_code.claude_code.instances[instance_id] = nil bufnr = nil end - if bufnr and vim.api.nvim_buf_is_valid(bufnr) then + if bufnr then -- Handle existing instance (toggle visibility) handle_existing_instance(bufnr, config) else - -- Prune invalid buffer entries - if bufnr and not vim.api.nvim_buf_is_valid(bufnr) then - claude_code.claude_code.instances[instance_id] = nil - end -- Create new instance create_new_instance(claude_code, config, git, instance_id) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lua/claude-code/terminal.lua(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
lua/claude-code/terminal.lua (4)
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Project structure should be organized with main plugin code under lua/claude-code/, plugin setup in after/plugin/, tests in tests/, and documentation in doc/ for clarity and maintainability.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Buffer names in the Claude Code Plugin include the git root path to help users easily identify which project or repository a buffer belongs to.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
🪛 GitHub Actions: CI
lua/claude-code/terminal.lua
[error] 274-274: Invalid buffer id: 42 error occurred during terminal module toggle with multi-instance enabled tests.
[error] 322-322: Invalid buffer id: 43 error occurred during terminal module floating window tests.
🔇 Additional comments (4)
lua/claude-code/terminal.lua (4)
33-46: LGTM! Clean implementation of dimension calculations.The helper function correctly handles percentage strings, numeric values, and provides sensible defaults. The logic is clear and handles edge cases well.
54-66: Excellent position clamping implementation.The position calculation correctly handles "center", percentage strings, and numeric values. The clamping logic ensures floating windows remain visible within the editor viewport, which is crucial for usability.
126-149: Robust command building with proper shell escaping.The function correctly handles git root integration and uses
vim.fn.shellescape()to prevent issues with paths containing spaces. The configurable shell commands add good flexibility.
269-283: Comprehensive terminal buffer validation.The validation logic correctly checks buffer validity, buffer type, terminal job ID existence, and job status. The use of
pcallfor safe variable access is appropriate.
lua/claude-code/terminal.lua
Outdated
| local buftype = vim.api.nvim_get_option_value('buftype', {buf = bufnr}) | ||
| local terminal_job_id = nil | ||
| pcall(function() | ||
| terminal_job_id = vim.b[bufnr].terminal_job_id | ||
| end) | ||
|
|
||
| return buftype == 'terminal' | ||
| and terminal_job_id | ||
| and vim.fn.jobwait({ terminal_job_id }, 0)[1] == -1 | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Investigate pipeline failures related to buffer validation.
The buffer validation logic appears sound, but pipeline failures indicate "Invalid buffer id" errors during tests. This suggests potential issues with test mocking or buffer lifecycle management.
🏁 Script executed:
#!/bin/bash
# Check for buffer validation patterns and test setup
echo "=== Searching for buffer validation patterns ==="
rg -A 5 -B 5 "Invalid buffer id"
echo "=== Checking test buffer setup patterns ==="
rg -A 10 "nvim_create_buf.*false.*true" tests/
echo "=== Looking for terminal buffer test mocks ==="
rg -A 5 -B 5 "terminal_job_id" tests/Length of output: 1036
Guard against invalid buffers in terminal buffer detection
The “Invalid buffer id” errors occur because vim.api.nvim_get_option_value('buftype', {buf = bufnr}) is called unguarded when bufnr is invalid (tests only mock vim.api.nvim_buf_get_var). To prevent pipeline failures, add an early buffer-validity check or wrap the option lookup in a pcall. For example:
File: lua/claude-code/terminal.lua
Location: start of the terminal-check function (around line 274)
Suggested patch:
- local buftype = vim.api.nvim_get_option_value('buftype', {buf = bufnr})
+ -- avoid errors on invalid or unloaded buffers
+ if not vim.api.nvim_buf_is_valid(bufnr) then
+ return false
+ end
+
+ local ok, buftype = pcall(vim.api.nvim_get_option_value, 'buftype', { buf = bufnr })
+ if not ok then
+ return false
+ endThis ensures we never call Neovim APIs on an invalid buffer, preventing “Invalid buffer id” during tests.
🧰 Tools
🪛 GitHub Actions: CI
[error] 274-274: Invalid buffer id: 42 error occurred during terminal module toggle with multi-instance enabled tests.
🤖 Prompt for AI Agents
In lua/claude-code/terminal.lua around lines 274 to 283, the call to
vim.api.nvim_get_option_value('buftype', {buf = bufnr}) is unguarded and causes
"Invalid buffer id" errors when bufnr is invalid. Fix this by adding an early
check to verify if bufnr is a valid buffer using vim.api.nvim_buf_is_valid or by
wrapping the option lookup in a pcall to safely handle invalid buffers. This
prevents calling Neovim APIs on invalid buffers and avoids errors during tests.
Add mocks for nvim_get_option_value and nvim_set_option_value APIs: - Mock nvim_get_option_value to handle new buffer option API - Mock vim.b table for buffer variable access - Mock nvim_set_option_value for both buffer and window options - Maintain backward compatibility with existing deprecated API mocks
Protect vim.o.lines and vim.o.columns from being corrupted as strings: - Use metatable to ensure numeric values stay as numbers - Prevent test interference with global vim.o state - Fixes arithmetic errors in split/float calculations Reduces test errors from 18 to 2 (remaining are test logic issues)
Fix window close mock to properly remove windows from array. API mock improvements: - Remove opts.buf requirement from nvim_get_option_value mock - Improve vim.o metatable for better numeric preservation Still debugging buffer reuse test - appears is_valid_terminal_buffer is failing for test buffers, causing new instance creation instead of buffer reuse
Fixed buffer close test: - Improve nvim_win_close mock to properly track window removal - Return copy from win_findbuf to avoid iterator issues - Add test-specific buffer validation for edge cases Fixed metatable stack overflow: - Use rawget/rawset in vim.b metatable to prevent infinite recursion - Ensure proper buffer variable access in test environment All 67 tests now pass (100% success rate)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/spec/terminal_spec.lua (3)
350-355: Buffer name sanitization test could be more explicit.The regex pattern
[^%w%-_]correctly identifies invalid characters, but the test could be more explicit about what constitutes valid characters for better maintainability.- assert.is_nil(buffer_name:match('[^%w%-_]'), 'Buffer name should not contain special characters') + -- Buffer names should only contain alphanumeric, hyphen, and underscore + assert.is_nil(buffer_name:match('[^%w%-_]'), 'Buffer name should only contain [a-zA-Z0-9_-]')
672-673: Hardcoded expected values make tests brittle.The test hardcodes expected values (96, 19) which could break if mock screen dimensions change. Consider calculating these values dynamically or making the screen dimensions explicit constants.
- -- Verify percentage calculations are independent of hardcoded values - assert.are.equal(96, expected_width) - assert.are.equal(19, expected_height) -- floor(38 * 0.5) = 19 + -- Verify percentage calculations match expected formula + local mock_columns = 120 + local mock_editor_height = 38 + assert.are.equal(math.floor(mock_columns * 0.8), expected_width) + assert.are.equal(math.floor(mock_editor_height * 0.5), expected_height)
700-724: Buffer reuse test could be more specific about validation logic.The test for buffer reuse is somewhat vague about what it's actually validating. Consider making the assertions more specific about the expected behavior when reusing existing buffers.
-- Should open floating window with existing buffer assert.is_true(nvim_open_win_called, 'nvim_open_win should be called') - -- Validate the window was created successfully - assert.is_not_nil(nvim_open_win_config) - -- In the reuse case, the buffer validation happens inside create_float - -- This test primarily ensures the floating window path is taken correctly + -- Verify floating window configuration is correct + assert.is_not_nil(nvim_open_win_config, 'Window config should be provided') + assert.are.equal('editor', nvim_open_win_config.relative) + assert.are.equal('none', nvim_open_win_config.border) + + -- Verify existing buffer is reused (buffer 42 from instances) + -- The specific buffer reuse logic is tested implicitly through the toggle function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/spec/terminal_spec.lua(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
tests/spec/terminal_spec.lua (3)
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Buffer names in the Claude Code Plugin include the git root path to help users easily identify which project or repository a buffer belongs to.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Project structure should be organized with main plugin code under lua/claude-code/, plugin setup in after/plugin/, tests in tests/, and documentation in doc/ for clarity and maintainability.
🔇 Additional comments (3)
tests/spec/terminal_spec.lua (3)
25-39: Excellent metatable approach for protecting test data integrity.The metatable implementation ensures that critical vim options (
lines,columns,cmdheight) remain numeric and prevents test corruption from string assignments. This is a robust solution for maintaining test reliability.
52-92: Comprehensive API mocking with good compatibility coverage.The mocks handle both deprecated (
nvim_buf_get_option) and new APIs (nvim_get_option_value,vim.b) which ensures compatibility across different Neovim versions. The buffer variable mocking with metatable approach is particularly well-designed.
562-750: Comprehensive floating window test coverage with excellent organization.The floating window test suite provides excellent coverage of the new functionality including:
- Basic floating window creation
- Percentage-based dimensions
- Centering behavior
- Buffer reuse scenarios
- Edge case handling
The tests are well-organized, use appropriate mocks, and verify the correct API calls and configurations.
| it('should handle out-of-bounds dimensions gracefully', function() | ||
| -- Claude Code is not running | ||
| claude_code.claude_code.bufnr = nil | ||
|
|
||
| -- Configure floating window with large dimensions | ||
| config.window.position = 'float' | ||
| config.window.float = { | ||
| width = '150%', | ||
| height = '110%', | ||
| row = '90%', | ||
| col = '95%', | ||
| relative = 'editor', | ||
| border = 'rounded' | ||
| } | ||
|
|
||
| -- Call toggle | ||
| terminal.toggle(claude_code, config, git) | ||
|
|
||
| -- Check that window is created (even if dims are out of bounds) | ||
| assert.is_true(nvim_open_win_called, 'nvim_open_win should be called') | ||
| local editor_height = 40 - 1 - 1 -- lines - cmdheight - status line = 38 | ||
| assert.are.equal(math.floor(120 * 1.5), nvim_open_win_config.width) | ||
| assert.are.equal(math.floor(editor_height * 1.1), nvim_open_win_config.height) | ||
| end) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Out-of-bounds test doesn't verify proper handling.
The test calculates out-of-bounds dimensions but doesn't verify that the terminal module properly clamps or handles them. This could miss bugs in dimension validation.
-- Check that window is created (even if dims are out of bounds)
assert.is_true(nvim_open_win_called, 'nvim_open_win should be called')
local editor_height = 40 - 1 - 1 -- lines - cmdheight - status line = 38
- assert.are.equal(math.floor(120 * 1.5), nvim_open_win_config.width)
- assert.are.equal(math.floor(editor_height * 1.1), nvim_open_win_config.height)
+ -- Verify that out-of-bounds dimensions are handled appropriately
+ local expected_width = math.floor(120 * 1.5) -- 180
+ local expected_height = math.floor(editor_height * 1.1) -- 41
+
+ -- Test should verify actual behavior: are dimensions clamped or passed through?
+ assert.are.equal(expected_width, nvim_open_win_config.width)
+ assert.are.equal(expected_height, nvim_open_win_config.height)
+
+ -- Add verification that position is clamped to keep window visible
+ assert.is_true(nvim_open_win_config.row >= 0)
+ assert.is_true(nvim_open_win_config.col >= 0)🤖 Prompt for AI Agents
In tests/spec/terminal_spec.lua around lines 726 to 749, the test sets
out-of-bounds floating window dimensions but does not verify that these
dimensions are properly clamped or handled by the terminal module. Update the
test to check that the actual window dimensions used are within valid bounds,
ensuring the terminal module correctly clamps or adjusts the dimensions instead
of blindly using the oversized values.
| if cmd:match("terminal pushd '/test/git/root' && " .. config.command .. " && popd") then | ||
| git_root_cmd_found = true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify shell escaping pattern in test assertion.
The test expects the git root path to be shell-escaped, but the pattern matching may be too specific. Consider if this test will be robust across different shell escaping implementations.
🏁 Script executed:
#!/bin/bash
# Check how shell escaping is implemented in the terminal module
ast-grep --pattern 'function $_(path)
$$$
return $$$
end'Length of output: 63
🏁 Script executed:
#!/bin/bash
# Search for usage of pushd in codebase
rg -n "pushd" .
# Search for shellescape function in codebase
rg -n "shellescape" .Length of output: 1405
🏁 Script executed:
#!/bin/bash
sed -n '110,160p' lua/claude-code/terminal.luaLength of output: 1620
Make tests use dynamic shell-escaped paths
Hard‐coding the single‐quoted path in your regex makes the test brittle if shellescape ever changes its quoting style or if paths contain special characters. Instead, generate the expected quote at runtime with vim.fn.shellescape.
• tests/spec/terminal_spec.lua line 228
• tests/spec/terminal_spec.lua line 423
Suggested diff:
- if cmd:match("terminal pushd '/test/git/root' && " .. config.command .. " && popd") then
+ local quoted_root = vim.fn.shellescape('/test/git/root')
+ if cmd:match("terminal pushd " .. quoted_root .. " && " .. config.command .. " && popd") then
…
- if cmd:match('terminal pushd .*/test/git/root.* && ' .. config.command .. ' && popd') then
+ local quoted_root_pattern = vim.fn.shellescape('/test/git/root'):gsub("([^%w])","%%%1")
+ if cmd:match('terminal pushd ' .. quoted_root_pattern .. ' && ' .. config.command .. ' && popd') thenThis ensures the test always aligns with whatever vim.fn.shellescape produces.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cmd:match("terminal pushd '/test/git/root' && " .. config.command .. " && popd") then | |
| git_root_cmd_found = true | |
| local quoted_root = vim.fn.shellescape('/test/git/root') | |
| if cmd:match("terminal pushd " .. quoted_root .. " && " .. config.command .. " && popd") then | |
| git_root_cmd_found = true |
| if cmd:match("terminal pushd '/test/git/root' && " .. config.command .. " && popd") then | |
| git_root_cmd_found = true | |
| local quoted_root_pattern = vim.fn.shellescape('/test/git/root'):gsub("([^%w])","%%%1") | |
| if cmd:match('terminal pushd ' .. quoted_root_pattern .. ' && ' .. config.command .. ' && popd') then |
🤖 Prompt for AI Agents
In tests/spec/terminal_spec.lua around lines 228 to 229, the test uses a
hard-coded single-quoted path in the regex, which can break if shellescape
changes its quoting style or if paths have special characters. Replace the
hard-coded path with a dynamically generated one using vim.fn.shellescape to
quote the path at runtime, ensuring the test matches the actual shell-escaped
string. Apply the same fix around line 423 as well.
Addresses CodeRabbit feedback by adding pcall protection around buffer option API calls and enhancing test quality with explicit assertions. - Add pcall wrapper for nvim_get_option_value to prevent invalid buffer errors - Replace hardcoded test values with dynamic calculations - Improve test assertions with explicit configuration checks - Add clearer comments for buffer name validation patterns
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lua/claude-code/terminal.lua (1)
33-46: Good implementation with minor input validation concern.The dimension calculation logic is sound and handles percentage strings correctly. However, consider adding validation for the input value parameter to handle unexpected types gracefully.
local function calculate_float_dimension(value, max_value) if value == nil then return math.floor(max_value * 0.8) -- Default to 80% if not specified elseif type(value) == 'string' and value:match('^%d+%%$') then local percentage = tonumber(value:match('^(%d+)%%$')) return math.floor(max_value * percentage / 100) + elseif type(value) == 'number' then + return value + else + -- Fallback to default for unexpected types + return math.floor(max_value * 0.8) end - return value end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lua/claude-code/terminal.lua(4 hunks)tests/spec/terminal_spec.lua(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/spec/terminal_spec.lua
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
lua/claude-code/terminal.lua (4)
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The plugin integrates with the Claude Code CLI for direct communication and context-aware interactions from within Neovim.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: The Claude Code Plugin for Neovim supports running multiple AI assistant instances, one per git repository root, allowing parallel work across multiple projects and tabs. This is controlled by the git.multi_instance option in the plugin setup (default: true).
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Buffer names in the Claude Code Plugin include the git root path to help users easily identify which project or repository a buffer belongs to.
Learnt from: CR
PR: greggh/claude-code.nvim#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T17:25:51.389Z
Learning: Project structure should be organized with main plugin code under lua/claude-code/, plugin setup in after/plugin/, tests in tests/, and documentation in doc/ for clarity and maintainability.
🔇 Additional comments (7)
lua/claude-code/terminal.lua (7)
48-66: Excellent position calculation with proper bounds checking.The position calculation correctly handles centering, percentages, and numeric values. The position clamping ensures floating windows remain visible within the editor viewport.
68-118: Well-implemented floating window creation with proper buffer validation.The function correctly addresses previous review feedback by using the newer
nvim_get_option_valueAPI and implementing comprehensive buffer validation with proper error handling.
120-149: Secure command construction with proper shell escaping.The function correctly uses
vim.fn.shellescape()to prevent shell injection vulnerabilities when handling git root paths that may contain special characters or spaces.
265-287: Robust buffer validation addresses previous pipeline failures.The comprehensive validation using
pcallfor both buffer options and terminal job access effectively handles the "Invalid buffer id" errors mentioned in previous reviews. The multi-layer validation ensures buffer integrity.
289-314: Clean separation of toggle logic with proper mode handling.The modularization improves code maintainability while correctly handling both floating and split window scenarios. The aggressive insert mode handling ensures consistent terminal behavior across window types.
316-381: Comprehensive instance creation handling both window types.The function properly handles the complexity of creating terminals in both floating and split windows, with consistent buffer naming, option configuration, and mode management.
383-413: Excellent refactoring of toggle function with improved modularity.The refactored toggle function effectively uses helper functions to handle instance management, buffer validation, and window creation. The buffer cleanup logic ensures no stale references remain in the instances map.
|
Is there anything missing in order to merge and release this? |
|
@caioariede I think I need maintainer approval to merge - @greggh let me know if you need any help reviewing this PR, happy to answer any questions. (also sorry I thought "close" might prompt me to "merge", hence the quick "close -> reopen") |
|
This is a great feature, thanks @krishna-bala! Hopefully it gets merged soon. |
|
I had an accident and multiple fractures with many hospital visits that kept me away. I am starting to catch up now, but I'm pretty slow with only one hand available. Got this merged now. Thanks for the awesome PR. |
|
That's terrible! I'm glad you're catching up now. Good recovery @greggh |
|
@greggh best wishes on a speedy recovery! Let me know if there's anything we can do to help maintain the repo, I'm happy to do some reviews on other PRs if that would be helpful. |
I would love the support, I could really use a co-maintainer for that. I'll be pretty slow and on pain killers for a few weeks, not the best state of mind to review code. If you're up for it I'd be happy to add you. |
|
I am happy to be a co-maintainer for however long you need! |
Invite sent |
|
@krishna-bala @greggh I've created this plugin especially to work with this feature: https://github.com/caioariede/nvim-persist-window |
This commit completes the Phase 4 AI integration implementation with production-ready features and comprehensive user documentation. ## Changes ### README.md (Complete Rewrite: 300 → 966 lines) - **Feature Overview**: Core, AI, Security, and Team capabilities - **Installation Guide**: Support for 4 package managers (Lazy.nvim, Packer, vim-plug, Manual) - **30+ Commands Documented**: Core, Variants, Multi-Instance, AI-powered - **Configuration Examples**: Default, floating windows, custom variants, multi-instance - **Quick Start Guide**: 3-step setup for immediate use - **AI Features Section**: Deployment, workflows, security best practices - **Multi-Instance Guide**: Polyrepo support with isolated sessions per repository - **Team Profiles**: Pre-built configs for backend, frontend, DevOps roles - **Documentation Links**: 8 comprehensive guides (deployment, workflows, security, etc.) - **Troubleshooting**: Common issues and solutions ### tests/spec/ai_integration_spec.lua (Critical Fix) - **Fix**: API unavailability test now properly mocks `config.enabled` state - **Issue**: Test was failing with "AI integration is disabled" instead of "API unavailable" - **Solution**: Added proper setup/teardown for enabled state in error handling tests - **Result**: All 22 AI integration tests now passing (100%) ## Test Results - **Total**: 114/115 tests passing (99.1% coverage) - **AI Integration**: 22/22 tests passing - **Critical Bugs Fixed**: JSON encoding circular reference handling - **Zero Regressions**: All existing functionality maintained ## Documentation Delivered (8 Comprehensive Guides) - AI_DEPLOYMENT_GUIDE.md (16KB, 750 lines) - AI_WORKFLOWS.md (16KB, 700 lines) - AI_SECURITY.md (14KB, 650 lines) - MULTI_INSTANCE_GUIDE.md (600 lines) - TROUBLESHOOTING.md (500 lines) - TEAM_ONBOARDING.md (450 lines) - config/README.md (350 lines) - README.md (966 lines) ## Security Features (7 Total) - Automatic secret redaction (5 pattern types) - Rate limiting (30 req/min + burst control) - Code size validation (100KB limit) - Local-first processing - Enhanced error handling - Input sanitization - Zero telemetry ## Project Status - Production-ready with enterprise-grade security - Complete team collaboration infrastructure - Multi-instance support for polyrepo workflows - AI-powered development capabilities (optional) Related: greggh#37, Phase 1-4 implementation complete 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
Summary
This is a rebased and improved version of the previously closed PR #25, addressing all CodeRabbit feedback and additional improvements discovered during testing.
Key Features
Changes Since Original PR #25
This implementation addresses all feedback from CodeRabbit on the original PR and includes additional improvements:
CodeRabbit Feedback Addressed
Additional Improvements
Test Coverage
Configuration Example
Test Plan
:ClaudeCode(with position = "float")Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests