feat: add floating window support #25
Conversation
Adds comprehensive floating window support with configurable dimensions, position, and border styles. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
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 <[email protected]>
Co-authored-by: Copilot <[email protected]>
WalkthroughThe changes introduce comprehensive support for floating windows in the Claude Code Neovim plugin. Documentation, configuration schema, window creation logic, and test suites are all updated to support and validate a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClaudeCodePlugin
participant Neovim
User->>ClaudeCodePlugin: Toggle Claude Code terminal
ClaudeCodePlugin->>ClaudeCodePlugin: Parse config (check window.position)
alt window.position == "float"
ClaudeCodePlugin->>Neovim: Create scratch buffer (if needed)
ClaudeCodePlugin->>Neovim: Open floating window with config (size, position, border)
ClaudeCodePlugin->>Neovim: Set buffer/window options
ClaudeCodePlugin->>Neovim: Run terminal command in buffer
else window.position == "left"/"right"/"top"/"bottom"
ClaudeCodePlugin->>Neovim: Open split window
ClaudeCodePlugin->>Neovim: Set buffer/window options
ClaudeCodePlugin->>Neovim: Run terminal command in buffer
end
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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: 4
🧹 Nitpick comments (9)
doc/claude-code.txt (1)
94-108: Clarify the documentedpositionvalues
position = "vertical"is accepted by the implementation, but many Neovim users are accustomed to"vsplit"(which is also supported bycreate_split). It may be worth mentioning both aliases to avoid confusion when users copy the example verbatim.Additionally, consider noting that
"botright vsplit"/"topleft split"may be provided verbatim when finer-grained control is desired, becausecreate_split()passes through any string containing “split”.tests/spec/config_validation_spec.lua (3)
35-43: Test only the reverted field, not the full fallbackThe assertion verifies that
window.positionreverts to its default when thefloattable is invalid – good.
To protect against future regressions, also assert thatwindow.floathas been reset (or at least that it is a table). Otherwise, an implementation could return{ position = default, float = 'invalid' }and still pass.
45-56: Duplicate logic across similar testsThese three tests differ only by the field under test (
width,relative,border) but each rewrites the entire test body. Consider using a parameterised helper such as:local function expect_invalid_float(field, value) local cfg = vim.deepcopy(config.default_config) cfg.window.position = 'float' cfg.window.float[field] = value local result = config.parse_config(cfg, true) assert.are.equal(config.default_config.window.position, result.window.position) endThis reduces duplication and makes it trivial to add more invalid-field checks later.
71-83: Border validation could also assert revert of borderSame remark as earlier: ensure the invalid border does not “bleed through” into the parsed config.
tests/spec/terminal_spec.lua (2)
350-372: Assertion misses window style guaranteesYou already assert
relativeandborder. To fully vet the float config, also assert calculatedwidth,height,row,colfor the numeric case so that later refactors can’t silently change the layout.
391-393: Hard-coded expected sizes risk driftIf default editor size constants change (e.g., you decide to mock 160×50 later), these magic numbers will break. Consider:
assert.are.equal(math.floor(120 * 0.8), nvim_open_win_config.width) assert.are.equal(math.floor(40 * 0.5), nvim_open_win_config.height)This makes intent explicit and independent of the stubbed values.
lua/claude-code/terminal.lua (3)
38-45: Clamp computed positions inside the screen
row/colvalues can currently exceed the screen bounds (e.g.,110%).
Considermath.max(0, math.min(pos, max_value - window_size))to keep the float visible.
90-96:create_splitmutates state before size calculation
is_verticallooks for substrings'vsplit'or'vertical'. If the user passes'botright vsplit', the subsequentposition .. ' split'duplication produces'botright vsplit split'.
Simplify: detect ifpositionalready contains'split'(any flavour) and otherwise prepend'split'/vsplit.
215-242: Duplicated git-root command constructionBoth floating & split paths build nearly identical
pushd … && … && popdstrings. Extract into a small helper to avoid divergence and ease unit testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 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(2 hunks)tests/spec/config_spec.lua(1 hunks)tests/spec/config_validation_spec.lua(1 hunks)tests/spec/terminal_spec.lua(1 hunks)
🔇 Additional comments (14)
lua/claude-code/config.lua (5)
12-12: LGTM! Clear documentation update.The documentation for the window position has been correctly updated to include the new "float" option, which aligns with the implementation.
17-23: Comprehensive documentation for float parameters.All the floating window configuration options are well-documented with clear parameter descriptions. This provides good guidance for users who want to customize their floating windows.
77-85: Well-structured default floating window configuration.The default configuration for floating windows is comprehensive and sensible:
- 80% width and height provides good visibility without taking the entire screen
- Center positioning is intuitive for most users
- The rounded border style is aesthetically pleasing and consistent with modern UI practices
162-225: Thorough validation for floating window configuration.The validation logic is comprehensive and robust:
- Conditionally validating float configuration only when position is "float"
- Proper handling of both numeric and percentage dimensions
- Validation of relative positioning options
- Comprehensive border style validation
- Flexible row/col validation supporting numbers, "center", and percentage strings
342-345: Good fallback to default float configuration.This logic ensures that when a user sets
position = "float"but doesn't provide float-specific configuration, the plugin will use sensible defaults rather than failing. This improves the user experience by making the feature work out-of-the-box with minimal configuration.CHANGELOG.md (1)
12-13: Clear and concise changelog entries.The changelog entries effectively communicate the new floating window feature and its configuration options. This is placed correctly in the "Unreleased" section, following the Keep a Changelog format.
README.md (3)
36-36: Updated features list to include floating windows.The features list now correctly mentions the support for floating windows, which is consistent with the implementation.
100-114: Comprehensive configuration documentation.The default configuration example has been updated to include the floating window options with clear comments explaining each setting. This provides users with a complete reference of all available options.
212-230: Excellent floating window example.The dedicated example for floating window configuration is clear and illustrative. It shows users exactly how to customize the floating window with practical values and explains what each setting does. This will help users quickly adopt this new feature.
tests/spec/config_spec.lua (4)
57-77: Good test coverage for basic float configuration.This test thoroughly verifies that the config parser correctly handles the floating window configuration with numeric dimensions. It checks all essential properties including width, height, relative positioning, and border style.
79-95: Proper testing of percentage-based dimensions.This test verifies that percentage strings (e.g., "80%") are correctly parsed and preserved in the configuration. This is important since percentage-based sizing is a key feature for responsive floating windows.
97-115: Validates center positioning functionality.The test confirms that 'center' values for row and column positioning are correctly handled by the configuration parser, which is essential for the centered floating window use case.
117-135: Verifies default float configuration behavior.This test confirms that when a user specifies
position = "float"without providing explicit float configuration, the system correctly applies the default floating window settings. This ensures the feature works out-of-the-box with minimal configuration.tests/spec/terminal_spec.lua (1)
418-438:nvim_create_buf_calledflag may mis-report
nvim_create_buf_calledis set only when the mocknvim_create_bufis executed.
Ifterminal.togglere-enters the early-return path before reaching the mock (e.g., because buffer validation changes), the test would pass even though no window opened. To ensure correctness:
- Assert that
nvim_open_win_called == true(already done); and- Assert that
nvim_create_buf_called == falseafter first ensuring the code path should indeed reuse the buffer.This guards against future logic regressions.
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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lua/claude-code/terminal.lua (2)
257-258: Consider applying the same command validation in the split window pathSimilar to the floating window path, consider validating the terminal command here as well.
53-95: 🛠️ Refactor suggestionEnhancement needed for terminal job verification in buffer reuse
While the function validates that an existing buffer is still a terminal by checking its buftype, it doesn't verify if the terminal job is still running. This could lead to displaying a terminated terminal.
-- Validate existing buffer is still a terminal local buftype = vim.api.nvim_buf_get_option(bufnr, 'buftype') - if buftype ~= 'terminal' then + local terminal_job_id = nil + pcall(function() + terminal_job_id = vim.api.nvim_buf_get_var(bufnr, 'terminal_job_id') + end) + local is_valid_terminal = buftype == 'terminal' and terminal_job_id and vim.fn.jobwait({terminal_job_id}, 0)[1] == -1 + if not is_valid_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
🧹 Nitpick comments (3)
lua/claude-code/terminal.lua (3)
216-251: Consider extracting common window configuration logic to reduce duplicationThe floating window and split window paths have duplicated logic for setting window options like hiding numbers and signcolumn. Consider extracting this into a helper function.
+ --- Configure common window options + --- @param win_id number Window ID to configure + --- @param config table Plugin configuration + --- @private + 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) + end + + if config.window.hide_signcolumn then + vim.api.nvim_win_set_option(win_id, 'signcolumn', 'no') + end + end -- In the floating window path: - 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) - end - - if config.window.hide_signcolumn then - vim.api.nvim_win_set_option(win_id, 'signcolumn', 'no') - end + configure_window_options(win_id, config) -- And similarly in the split window path
252-269: Consider using API calls consistently instead of mixing vim.cmd and API functionsThe split window path uses
vim.cmdwhile the floating window path uses direct API calls. Consider using API calls consistently for better maintainability.- vim.cmd 'setlocal nonumber norelativenumber' + vim.api.nvim_win_set_option(0, 'number', false) + vim.api.nvim_win_set_option(0, 'relativenumber', false) - vim.cmd 'setlocal signcolumn=no' + vim.api.nvim_win_set_option(0, 'signcolumn', 'no')
229-231: Consider validating that the terminal command is not emptyThe code runs
termopendirectly with the command without validating it. Consider adding a check to ensure the command is not empty to avoid potential errors.- vim.fn.termopen(cmd) + if cmd and cmd ~= "" then + vim.fn.termopen(cmd) + else + vim.api.nvim_buf_set_lines(bufnr, 0, -1, false, {"Error: No command specified"}) + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lua/claude-code/terminal.lua(3 hunks)tests/spec/terminal_spec.lua(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/spec/terminal_spec.lua
🔇 Additional comments (5)
lua/claude-code/terminal.lua (5)
18-31: Well-implemented dimension calculation with safe defaultsThe calculation functions provide a robust implementation to handle different input formats (numbers, percentages) with appropriate defaults. The nil-safe default is especially helpful.
33-51: Good position calculation with window edge handlingThe position calculation correctly handles "center" alignment and percentage-based positions, with proper clamping to keep windows within editor boundaries. This prevents windows from being positioned offscreen.
97-112: Good implementation of git root directory handling with proper path escapingThe function properly quotes git paths with
shellescapeto handle spaces in paths, fixing the issue mentioned in previous reviews.
120-136: Clean implementation for handling both floating and split windowsThe function has a clear separation of concerns between floating and split windows, and correctly preserves the existing split behavior while adding float support.
180-194: Proper validation of terminal buffer statusThe code correctly validates if the buffer is still a valid terminal and resets it if necessary, which prevents issues with stale or invalid terminal buffers.
|
Hi @krishna-bala, thanks for the awesome PR. Any chance you have time to do those last few suggestions from coderabbit? i would actually just accept this and do them myself later, but I won't have time for a few days. |
|
Sure! Might take a few days as well but should be able to get to it soon. |
|
Closing this PR as it's a bit stale - I'll take another stab at it soon if still relevant. |
Summary
Test plan
:ClaudeCode(with position = "float")🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests