From e1def677685becd19e7b922bf95fa67f3bbd972e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 13 Jun 2025 15:35:19 +0200 Subject: [PATCH] feat: implement bufhidden=hide for native terminal toggle Partially addresses #16 by implementing the core mechanism to preserve terminal processes when hiding windows. The native terminal provider now sets bufhidden=hide before closing windows, preventing Neovim from killing the terminal job when the window is closed. - Add hide_terminal() helper that sets bufhidden=hide before window close - Add show_hidden_terminal() to create windows for existing buffers - Add is_terminal_visible() for buffer visibility detection - Update toggle() logic to use hide/show instead of close/create - Add comprehensive test suite for toggle behavior verification - Optimize logger usage with module-level require This change enables process preservation during window hiding, laying the groundwork for full toggle behavior consistency with snacks provider. Change-Id: I334c00663dc2058eff2c362057e76499700b5e9e Signed-off-by: Thomas Kosiewski --- lua/claudecode/terminal/native.lua | 137 +++++-- tests/unit/native_terminal_toggle_spec.lua | 393 +++++++++++++++++++++ 2 files changed, 505 insertions(+), 25 deletions(-) create mode 100644 tests/unit/native_terminal_toggle_spec.lua diff --git a/lua/claudecode/terminal/native.lua b/lua/claudecode/terminal/native.lua index 803c268..0b26bd5 100644 --- a/lua/claudecode/terminal/native.lua +++ b/lua/claudecode/terminal/native.lua @@ -4,6 +4,8 @@ --- @type TerminalProvider local M = {} +local logger = require("claudecode.logger") + local bufnr = nil local winid = nil local jobid = nil @@ -31,13 +33,12 @@ local function is_valid() if vim.api.nvim_win_get_buf(win) == bufnr then -- Found a window displaying our terminal buffer, update the tracked window ID winid = win - require("claudecode.logger").debug("terminal", "Recovered terminal window ID:", win) + logger.debug("terminal", "Recovered terminal window ID:", win) return true end end - -- Buffer exists but no window displays it - cleanup_state() - return false + -- Buffer exists but no window displays it - this is normal for hidden terminals + return true -- Buffer is valid even though not visible end -- Both buffer and window are valid @@ -82,6 +83,8 @@ local function open_terminal(cmd_string, env_table, effective_config) on_exit = function(job_id, _, _) vim.schedule(function() if job_id == jobid then + logger.debug("terminal", "Terminal process exited, cleaning up") + -- Ensure we are operating on the correct window and buffer before closing local current_winid_for_job = winid local current_bufnr_for_job = bufnr @@ -135,7 +138,7 @@ local function close_terminal() -- If the job already exited, on_exit would have cleaned up. -- This direct close is for user-initiated close. vim.api.nvim_win_close(winid, true) - cleanup_state() -- Ensure cleanup if on_exit doesn't fire (e.g. job already dead) + cleanup_state() -- Cleanup after explicit close end end @@ -146,6 +149,78 @@ local function focus_terminal() end end +local function is_terminal_visible() + -- Check if our terminal buffer exists and is displayed in any window + if not bufnr or not vim.api.nvim_buf_is_valid(bufnr) then + return false + end + + local windows = vim.api.nvim_list_wins() + for _, win in ipairs(windows) do + if vim.api.nvim_win_is_valid(win) and vim.api.nvim_win_get_buf(win) == bufnr then + -- Update our tracked window ID if we find the buffer in a different window + winid = win + return true + end + end + + -- Buffer exists but no window displays it + winid = nil + return false +end + +local function hide_terminal() + -- Hide the terminal window but keep the buffer and job alive + if bufnr and vim.api.nvim_buf_is_valid(bufnr) and winid and vim.api.nvim_win_is_valid(winid) then + -- Set buffer to hide instead of being wiped when window closes + vim.api.nvim_buf_set_option(bufnr, "bufhidden", "hide") + + -- Close the window - this preserves the buffer and job + vim.api.nvim_win_close(winid, false) + winid = nil -- Clear window reference + + logger.debug("terminal", "Terminal window hidden, process preserved") + end +end + +local function show_hidden_terminal(effective_config) + -- Show an existing hidden terminal buffer in a new window + if not bufnr or not vim.api.nvim_buf_is_valid(bufnr) then + return false + end + + -- Check if it's already visible + if is_terminal_visible() then + focus_terminal() + return true + end + + -- Create a new window for the existing buffer + local width = math.floor(vim.o.columns * effective_config.split_width_percentage) + local full_height = vim.o.lines + local placement_modifier + + if effective_config.split_side == "left" then + placement_modifier = "topleft " + else + placement_modifier = "botright " + end + + vim.cmd(placement_modifier .. width .. "vsplit") + local new_winid = vim.api.nvim_get_current_win() + vim.api.nvim_win_set_height(new_winid, full_height) + + -- Set the existing buffer in the new window + vim.api.nvim_win_set_buf(new_winid, bufnr) + winid = new_winid + + vim.api.nvim_set_current_win(winid) + vim.cmd("startinsert") + + logger.debug("terminal", "Showed hidden terminal in new window") + return true +end + local function find_existing_claude_terminal() local buffers = vim.api.nvim_list_bufs() for _, buf in ipairs(buffers) do @@ -158,13 +233,7 @@ local function find_existing_claude_terminal() local windows = vim.api.nvim_list_wins() for _, win in ipairs(windows) do if vim.api.nvim_win_get_buf(win) == buf then - require("claudecode.logger").debug( - "terminal", - "Found existing Claude terminal in buffer", - buf, - "window", - win - ) + logger.debug("terminal", "Found existing Claude terminal in buffer", buf, "window", win) return buf, win end end @@ -193,7 +262,7 @@ function M.open(cmd_string, env_table, effective_config) bufnr = existing_buf winid = existing_win -- Note: We can't recover the job ID easily, but it's less critical - require("claudecode.logger").debug("terminal", "Recovered existing Claude terminal") + logger.debug("terminal", "Recovered existing Claude terminal") focus_terminal() else if not open_terminal(cmd_string, env_table, effective_config) then @@ -211,32 +280,50 @@ end --- @param env_table table --- @param effective_config table function M.toggle(cmd_string, env_table, effective_config) - if is_valid() then - local claude_term_neovim_win_id = winid - local current_neovim_win_id = vim.api.nvim_get_current_win() + -- Check if we have a valid terminal buffer (process running) + local has_buffer = bufnr and vim.api.nvim_buf_is_valid(bufnr) + local is_visible = has_buffer and is_terminal_visible() - if claude_term_neovim_win_id == current_neovim_win_id then - close_terminal() + if has_buffer then + -- Terminal process exists + if is_visible then + -- Terminal is visible - check if we're currently in it + local current_win_id = vim.api.nvim_get_current_win() + if winid == current_win_id then + -- We're in the terminal window, hide it (but keep process running) + hide_terminal() + else + -- Terminal is visible but we're not in it, focus it + focus_terminal() + end else - focus_terminal() -- This already calls startinsert + -- Terminal process exists but is hidden, show it + if show_hidden_terminal(effective_config) then + logger.debug("terminal", "Showing hidden terminal") + else + logger.error("terminal", "Failed to show hidden terminal") + end end else - -- Check if there's an existing Claude terminal we lost track of + -- No terminal process exists, check if there's an existing one we lost track of local existing_buf, existing_win = find_existing_claude_terminal() if existing_buf and existing_win then -- Recover the existing terminal bufnr = existing_buf winid = existing_win - require("claudecode.logger").debug("terminal", "Recovered existing Claude terminal in toggle") + logger.debug("terminal", "Recovered existing Claude terminal") - -- Check if we're currently in this terminal - local current_neovim_win_id = vim.api.nvim_get_current_win() - if existing_win == current_neovim_win_id then - close_terminal() + -- Check if we're currently in this recovered terminal + local current_win_id = vim.api.nvim_get_current_win() + if existing_win == current_win_id then + -- We're in the recovered terminal, hide it + hide_terminal() else + -- Focus the recovered terminal focus_terminal() end else + -- No existing terminal found, create a new one if not open_terminal(cmd_string, env_table, effective_config) then vim.notify("Failed to open Claude terminal using native fallback (toggle).", vim.log.levels.ERROR) end diff --git a/tests/unit/native_terminal_toggle_spec.lua b/tests/unit/native_terminal_toggle_spec.lua new file mode 100644 index 0000000..aacfab3 --- /dev/null +++ b/tests/unit/native_terminal_toggle_spec.lua @@ -0,0 +1,393 @@ +describe("claudecode.terminal.native toggle behavior", function() + local native_provider + local mock_vim + local logger_spy + + before_each(function() + -- Set up the package path for tests + package.path = "./lua/?.lua;" .. package.path + + -- Clean up any loaded modules + package.loaded["claudecode.terminal.native"] = nil + package.loaded["claudecode.logger"] = nil + + -- Mock state for more realistic testing + local mock_state = { + buffers = {}, + windows = {}, + current_win = 1, + next_bufnr = 1, + next_winid = 1000, + next_jobid = 10000, + buffer_options = {}, + } + + -- Mock vim API with stateful behavior + mock_vim = { + api = { + nvim_buf_is_valid = function(bufnr) + return mock_state.buffers[bufnr] ~= nil + end, + nvim_win_is_valid = function(winid) + return mock_state.windows[winid] ~= nil + end, + nvim_list_wins = function() + local wins = {} + for winid, _ in pairs(mock_state.windows) do + table.insert(wins, winid) + end + return wins + end, + nvim_list_bufs = function() + local bufs = {} + for bufnr, _ in pairs(mock_state.buffers) do + table.insert(bufs, bufnr) + end + return bufs + end, + nvim_buf_get_name = function(bufnr) + local buf = mock_state.buffers[bufnr] + return buf and buf.name or "" + end, + nvim_buf_get_option = function(bufnr, option) + local buf = mock_state.buffers[bufnr] + if buf and buf.options and buf.options[option] then + return buf.options[option] + end + return "" + end, + nvim_buf_set_option = function(bufnr, option, value) + local buf = mock_state.buffers[bufnr] + if buf then + buf.options = buf.options or {} + buf.options[option] = value + -- Track calls for verification + mock_state.buffer_options[bufnr] = mock_state.buffer_options[bufnr] or {} + mock_state.buffer_options[bufnr][option] = value + end + end, + nvim_win_get_buf = function(winid) + local win = mock_state.windows[winid] + return win and win.bufnr or 0 + end, + nvim_win_close = function(winid, force) + -- Remove window from state (simulates window closing) + if winid and mock_state.windows[winid] then + mock_state.windows[winid] = nil + end + end, + nvim_get_current_win = function() + return mock_state.current_win + end, + nvim_get_current_buf = function() + local current_win = mock_state.current_win + local win = mock_state.windows[current_win] + return win and win.bufnr or 0 + end, + nvim_set_current_win = function(winid) + if mock_state.windows[winid] then + mock_state.current_win = winid + end + end, + nvim_win_set_buf = function(winid, bufnr) + local win = mock_state.windows[winid] + if win and mock_state.buffers[bufnr] then + win.bufnr = bufnr + end + end, + nvim_win_set_height = function(winid, height) + -- Mock window resizing + end, + nvim_win_set_width = function(winid, width) + -- Mock window resizing + end, + nvim_win_call = function(winid, fn) + -- Mock window-specific function execution + return fn() + end, + }, + cmd = function(command) + -- Handle vsplit and other commands + if command:match("^topleft %d+vsplit") or command:match("^botright %d+vsplit") then + -- Create new window + local winid = mock_state.next_winid + mock_state.next_winid = mock_state.next_winid + 1 + mock_state.windows[winid] = { bufnr = 0 } + mock_state.current_win = winid + elseif command == "enew" then + -- Create new buffer in current window + local bufnr = mock_state.next_bufnr + mock_state.next_bufnr = mock_state.next_bufnr + 1 + mock_state.buffers[bufnr] = { name = "", options = {} } + if mock_state.windows[mock_state.current_win] then + mock_state.windows[mock_state.current_win].bufnr = bufnr + end + end + end, + o = { + columns = 120, + lines = 40, + }, + fn = { + termopen = function(cmd, opts) + local jobid = mock_state.next_jobid + mock_state.next_jobid = mock_state.next_jobid + 1 + + -- Create terminal buffer + local bufnr = mock_state.next_bufnr + mock_state.next_bufnr = mock_state.next_bufnr + 1 + mock_state.buffers[bufnr] = { + name = "term://claude", + options = { buftype = "terminal", bufhidden = "wipe" }, + jobid = jobid, + on_exit = opts.on_exit, + } + + -- Set buffer in current window + if mock_state.windows[mock_state.current_win] then + mock_state.windows[mock_state.current_win].bufnr = bufnr + end + + return jobid + end, + }, + schedule = function(callback) + callback() -- Execute immediately in tests + end, + bo = setmetatable({}, { + __index = function(_, bufnr) + return setmetatable({}, { + __newindex = function(_, option, value) + -- Mock buffer option setting + local buf = mock_state.buffers[bufnr] + if buf then + buf.options = buf.options or {} + buf.options[option] = value + end + end, + __index = function(_, option) + local buf = mock_state.buffers[bufnr] + return buf and buf.options and buf.options[option] or "" + end, + }) + end, + }), + } + _G.vim = mock_vim + + -- Mock logger + logger_spy = { + debug = function(module, message, ...) + -- Track debug calls for verification + end, + error = function(module, message, ...) + -- Track error calls + end, + } + package.loaded["claudecode.logger"] = logger_spy + + -- Load the native provider + native_provider = require("claudecode.terminal.native") + native_provider.setup({}) + + -- Helper function to get mock state for verification + _G.get_mock_state = function() + return mock_state + end + end) + + after_each(function() + _G.vim = nil + package.loaded["claudecode.terminal.native"] = nil + package.loaded["claudecode.logger"] = nil + end) + + describe("toggle with no existing terminal", function() + it("should create a new terminal when none exists", function() + local cmd_string = "claude" + local env_table = { TEST = "value" } + local config = { split_side = "right", split_width_percentage = 0.3 } + + -- Mock termopen to succeed + mock_vim.fn.termopen = function(cmd, opts) + assert.are.equal(cmd_string, cmd[1]) + assert.are.same(env_table, opts.env) + return 12345 -- Valid job ID + end + + native_provider.toggle(cmd_string, env_table, config) + + -- Should have created terminal and have active buffer + assert.is_not_nil(native_provider.get_active_bufnr()) + end) + end) + + describe("toggle with existing hidden terminal", function() + it("should show hidden terminal instead of creating new one", function() + local cmd_string = "claude" + local env_table = { TEST = "value" } + local config = { split_side = "right", split_width_percentage = 0.3 } + + -- First create a terminal + mock_vim.fn.termopen = function(cmd, opts) + return 12345 -- Valid job ID + end + native_provider.open(cmd_string, env_table, config) + + local initial_bufnr = native_provider.get_active_bufnr() + assert.is_not_nil(initial_bufnr) + + -- Simulate hiding the terminal (buffer exists but no window shows it) + mock_vim.api.nvim_list_wins = function() + return { 1, 3 } -- Window 2 (which had our buffer) is gone + end + mock_vim.api.nvim_win_get_buf = function(winid) + return 50 -- Other windows have different buffers + end + + -- Mock window creation for showing hidden terminal + local vsplit_called = false + local original_cmd = mock_vim.cmd + mock_vim.cmd = function(command) + if command:match("vsplit") then + vsplit_called = true + end + original_cmd(command) + end + + mock_vim.api.nvim_get_current_win = function() + return 4 -- New window created + end + + -- Toggle should show the hidden terminal + native_provider.toggle(cmd_string, env_table, config) + + -- Should not have created a new buffer/job, just shown existing one + assert.are.equal(initial_bufnr, native_provider.get_active_bufnr()) + assert.is_true(vsplit_called) + end) + end) + + describe("toggle with visible terminal", function() + it("should hide terminal when toggling from inside it and set bufhidden=hide", function() + local cmd_string = "claude" + local env_table = { TEST = "value" } + local config = { split_side = "right", split_width_percentage = 0.3 } + + -- Create a terminal by opening it + native_provider.open(cmd_string, env_table, config) + local initial_bufnr = native_provider.get_active_bufnr() + assert.is_not_nil(initial_bufnr) + + local mock_state = _G.get_mock_state() + + -- Verify initial state - buffer should exist and have a window + assert.is_not_nil(mock_state.buffers[initial_bufnr]) + assert.are.equal("wipe", mock_state.buffers[initial_bufnr].options.bufhidden) + + -- Find the window that contains our terminal buffer + local terminal_winid = nil + for winid, win in pairs(mock_state.windows) do + if win.bufnr == initial_bufnr then + terminal_winid = winid + break + end + end + assert.is_not_nil(terminal_winid) + + -- Mock that we're currently in the terminal window + mock_state.current_win = terminal_winid + + -- Toggle should hide the terminal + native_provider.toggle(cmd_string, env_table, config) + + -- Verify the critical behavior: + -- 1. Buffer should still exist and be valid + assert.are.equal(initial_bufnr, native_provider.get_active_bufnr()) + assert.is_not_nil(mock_state.buffers[initial_bufnr]) + + -- 2. bufhidden should have been set to "hide" (this is the core fix) + assert.are.equal("hide", mock_state.buffer_options[initial_bufnr].bufhidden) + + -- 3. Window should be closed/invalid + assert.is_nil(mock_state.windows[terminal_winid]) + end) + + it("should focus terminal when toggling from outside it", function() + local cmd_string = "claude" + local env_table = { TEST = "value" } + local config = { split_side = "right", split_width_percentage = 0.3 } + + -- Create a terminal + native_provider.open(cmd_string, env_table, config) + local initial_bufnr = native_provider.get_active_bufnr() + local mock_state = _G.get_mock_state() + + -- Find the terminal window that was created + local terminal_winid = nil + for winid, win in pairs(mock_state.windows) do + if win.bufnr == initial_bufnr then + terminal_winid = winid + break + end + end + assert.is_not_nil(terminal_winid) + + -- Mock that we're NOT in the terminal window (simulate being in a different window) + mock_state.current_win = 1 -- Some other window + + local set_current_win_called = false + local focused_winid = nil + local original_set_current_win = mock_vim.api.nvim_set_current_win + mock_vim.api.nvim_set_current_win = function(winid) + set_current_win_called = true + focused_winid = winid + return original_set_current_win(winid) + end + + -- Toggle should focus the terminal + native_provider.toggle(cmd_string, env_table, config) + + -- Should have focused the terminal window + assert.is_true(set_current_win_called) + assert.are.equal(terminal_winid, focused_winid) + assert.are.equal(initial_bufnr, native_provider.get_active_bufnr()) + end) + end) + + describe("close vs toggle behavior", function() + it("should preserve process on toggle but kill on close", function() + local cmd_string = "claude" + local env_table = { TEST = "value" } + local config = { split_side = "right", split_width_percentage = 0.3 } + + -- Create a terminal + native_provider.open(cmd_string, env_table, config) + local initial_bufnr = native_provider.get_active_bufnr() + assert.is_not_nil(initial_bufnr) + + local mock_state = _G.get_mock_state() + + -- Find the terminal window + local terminal_winid = nil + for winid, win in pairs(mock_state.windows) do + if win.bufnr == initial_bufnr then + terminal_winid = winid + break + end + end + + -- Mock being in terminal window + mock_state.current_win = terminal_winid + + -- Toggle should hide but preserve process + native_provider.toggle(cmd_string, env_table, config) + assert.are.equal(initial_bufnr, native_provider.get_active_bufnr()) + assert.are.equal("hide", mock_state.buffer_options[initial_bufnr].bufhidden) + + -- Close should kill the process (cleanup_state called) + native_provider.close() + assert.is_nil(native_provider.get_active_bufnr()) + end) + end) +end)