From 0602e363e25e3ab963900424955559823c592250 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 27 Apr 2026 13:20:01 +0200 Subject: [PATCH 1/4] fix(selection): close debounce timers safely Change-Id: I4caa6d010f8f824aff1c38a7a73d08d47b400cce Signed-off-by: Thomas Kosiewski --- lua/claudecode/selection.lua | 67 +++++++++++++++++--- tests/selection_test.lua | 116 ++++++++++++++++++++++++++++++++++- 2 files changed, 172 insertions(+), 11 deletions(-) diff --git a/lua/claudecode/selection.lua b/lua/claudecode/selection.lua index 9bbfed9b..9eb5e3cc 100644 --- a/lua/claudecode/selection.lua +++ b/lua/claudecode/selection.lua @@ -5,6 +5,8 @@ local M = {} local logger = require("claudecode.logger") local terminal = require("claudecode.terminal") +local uv = vim.uv or vim.loop + M.state = { latest_selection = nil, tracking_enabled = false, @@ -45,10 +47,33 @@ function M.disable() M.state.latest_selection = nil M.server = nil - if M.state.debounce_timer then - vim.loop.timer_stop(M.state.debounce_timer) - M.state.debounce_timer = nil + M._cancel_debounce_timer() + + if M.state.demotion_timer then + local demotion_timer = M.state.demotion_timer + M.state.demotion_timer = nil + + demotion_timer:stop() + demotion_timer:close() + end +end + +---Cancels and closes the current debounce timer, if any. +---@local +function M._cancel_debounce_timer() + local timer = M.state.debounce_timer + if not timer then + return end + + -- Clear state before stopping/closing so any already-scheduled callback is a no-op. + M.state.debounce_timer = nil + + assert(timer.stop, "Expected debounce timer to have :stop()") + assert(timer.close, "Expected debounce timer to have :close()") + + timer:stop() + timer:close() end ---Creates autocommands for tracking selections. @@ -107,14 +132,36 @@ end ---Ensures that `update_selection` is not called too frequently by deferring ---its execution. function M.debounce_update() - if M.state.debounce_timer then - vim.loop.timer_stop(M.state.debounce_timer) - end + M._cancel_debounce_timer() + + assert(type(M.state.debounce_ms) == "number", "Expected debounce_ms to be a number") + + local timer = uv.new_timer() + assert(timer, "Expected uv.new_timer() to return a timer handle") + assert(timer.start, "Expected debounce timer to have :start()") + assert(timer.stop, "Expected debounce timer to have :stop()") + assert(timer.close, "Expected debounce timer to have :close()") + + M.state.debounce_timer = timer - M.state.debounce_timer = vim.defer_fn(function() - M.update_selection() - M.state.debounce_timer = nil - end, M.state.debounce_ms) + timer:start( + M.state.debounce_ms, + 0, -- 0 repeat = one-shot + vim.schedule_wrap(function() + -- Ignore stale timers (e.g., cancelled and replaced before callback runs) + if M.state.debounce_timer ~= timer then + return + end + + -- Clear state before stopping/closing so cancellation is idempotent. + M.state.debounce_timer = nil + + timer:stop() + timer:close() + + M.update_selection() + end) + ) end ---Updates the current selection state. diff --git a/tests/selection_test.lua b/tests/selection_test.lua index ee59383e..98a58931 100644 --- a/tests/selection_test.lua +++ b/tests/selection_test.lua @@ -1,4 +1,6 @@ if not _G.vim then + local next_timer_id = 0 + _G.vim = { ---@type vim_global_api schedule_wrap = function(fn) return fn @@ -192,7 +194,49 @@ if not _G.vim then end, loop = { - timer_stop = function(_timer) -- Prefix unused param with underscore + now = function() + return 0 + end, + new_timer = function() + next_timer_id = next_timer_id + 1 + + local timer = { + _id = next_timer_id, + _start_calls = 0, + _stop_calls = 0, + _close_calls = 0, + _callback = nil, + } + + function timer:start(timeout, repeat_interval, callback) + self._start_calls = self._start_calls + 1 + self._timeout = timeout + self._repeat_interval = repeat_interval + self._callback = callback + return true + end + + function timer:stop() + self._stop_calls = self._stop_calls + 1 + return true + end + + function timer:close() + self._close_calls = self._close_calls + 1 + return true + end + + function timer:fire() + assert(self._callback, "Timer has no callback; did you call :start()?") + return self._callback() + end + + return timer + end, + timer_stop = function(timer) + if timer and timer.stop then + timer:stop() + end return true end, }, @@ -362,6 +406,76 @@ describe("Selection module", function() assert(selection.state.latest_selection == nil) end) + describe("debounce_update", function() + it("should cancel and close previous debounce timer when re-debouncing", function() + local update_calls = 0 + local old_update_selection = selection.update_selection + + selection.update_selection = function() + update_calls = update_calls + 1 + end + + selection.debounce_update() + local timer1 = selection.state.debounce_timer + assert(timer1 ~= nil) + + selection.debounce_update() + local timer2 = selection.state.debounce_timer + assert(timer2 ~= nil) + assert.are_not.equal(timer1, timer2) + + assert.are.equal(1, timer1._stop_calls) + assert.are.equal(1, timer1._close_calls) + + -- Clean up the active timer + timer2:fire() + assert.are.equal(1, update_calls) + + selection.update_selection = old_update_selection + end) + + it("should ignore stale debounce timer callbacks", function() + local update_calls = 0 + local old_update_selection = selection.update_selection + + selection.update_selection = function() + update_calls = update_calls + 1 + end + + selection.debounce_update() + local timer1 = selection.state.debounce_timer + assert(timer1 ~= nil) + + selection.debounce_update() + local timer2 = selection.state.debounce_timer + assert(timer2 ~= nil) + + -- A callback from a cancelled timer should be ignored. + timer1:fire() + assert.are.equal(0, update_calls) + + timer2:fire() + assert.are.equal(1, update_calls) + assert(selection.state.debounce_timer == nil) + assert.are.equal(1, timer2._stop_calls) + assert.are.equal(1, timer2._close_calls) + + selection.update_selection = old_update_selection + end) + + it("disable() should cancel an active debounce timer", function() + selection.enable(mock_server) + selection.debounce_update() + local timer = selection.state.debounce_timer + assert(timer ~= nil) + + selection.disable() + assert(selection.state.debounce_timer == nil) + assert.are.equal(1, timer._stop_calls) + assert.are.equal(1, timer._close_calls) + end) + end) + it("should get cursor position in normal mode", function() local old_win_get_cursor = _G.vim.api.nvim_win_get_cursor _G.vim.api.nvim_win_get_cursor = function() From bb2e5a46f844f7743cf397b9fd3666b859723024 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 27 Apr 2026 17:06:23 +0200 Subject: [PATCH 2/4] fix(selection): apply identity-check pattern to demotion timer Extracts _cancel_demotion_timer() mirroring _cancel_debounce_timer() and applies the same identity-check guard to the demotion timer callback so late-firing libuv callbacks cannot mutate state after the timer was cancelled (e.g. by disable() or by being replaced). Also adds defense-in-depth by gating handle_selection_demotion() on tracking_enabled, switches the demotion timer to the uv alias, and adds a regression test that exercises a stale demotion callback firing after disable(). Change-Id: Ifce08a736d550605ce99a697a187deecf62768a1 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Thomas Kosiewski --- lua/claudecode/selection.lua | 76 ++++++++++++++++++------------------ tests/selection_test.lua | 69 +++++++++++++++++++++++++++++--- 2 files changed, 102 insertions(+), 43 deletions(-) diff --git a/lua/claudecode/selection.lua b/lua/claudecode/selection.lua index 9eb5e3cc..d1e8e1b4 100644 --- a/lua/claudecode/selection.lua +++ b/lua/claudecode/selection.lua @@ -34,7 +34,8 @@ function M.enable(server, visual_demotion_delay_ms) end ---Disables selection tracking. ----Clears autocommands, resets internal state, and stops any active debounce timers. +---Clears autocommands, resets internal state, and stops any active debounce or +---demotion timers. function M.disable() if not M.state.tracking_enabled then return @@ -48,14 +49,7 @@ function M.disable() M.server = nil M._cancel_debounce_timer() - - if M.state.demotion_timer then - local demotion_timer = M.state.demotion_timer - M.state.demotion_timer = nil - - demotion_timer:stop() - demotion_timer:close() - end + M._cancel_demotion_timer() end ---Cancels and closes the current debounce timer, if any. @@ -69,8 +63,20 @@ function M._cancel_debounce_timer() -- Clear state before stopping/closing so any already-scheduled callback is a no-op. M.state.debounce_timer = nil - assert(timer.stop, "Expected debounce timer to have :stop()") - assert(timer.close, "Expected debounce timer to have :close()") + timer:stop() + timer:close() +end + +---Cancels and closes the current demotion timer, if any. +---@local +function M._cancel_demotion_timer() + local timer = M.state.demotion_timer + if not timer then + return + end + + -- Clear state before stopping/closing so any already-scheduled callback is a no-op. + M.state.demotion_timer = nil timer:stop() timer:close() @@ -153,7 +159,7 @@ function M.debounce_update() return end - -- Clear state before stopping/closing so cancellation is idempotent. + -- Clear state so _cancel_debounce_timer() is a no-op if called after firing. M.state.debounce_timer = nil timer:stop() @@ -178,11 +184,7 @@ function M.update_selection() -- If the buffer name starts with "term://" and contains "claude", do not update selection if buf_name and buf_name:match("^term://") and buf_name:lower():find("claude", 1, true) then -- Optionally, cancel demotion timer like for the terminal - if M.state.demotion_timer then - M.state.demotion_timer:stop() - M.state.demotion_timer:close() - M.state.demotion_timer = nil - end + M._cancel_demotion_timer() return end @@ -191,11 +193,7 @@ function M.update_selection() local claude_term_bufnr = terminal.get_active_terminal_bufnr() if claude_term_bufnr and current_buf == claude_term_bufnr then -- Cancel any pending demotion if we switch to the Claude terminal - if M.state.demotion_timer then - M.state.demotion_timer:stop() - M.state.demotion_timer:close() - M.state.demotion_timer = nil - end + M._cancel_demotion_timer() return end end @@ -206,11 +204,7 @@ function M.update_selection() if current_mode == "v" or current_mode == "V" or current_mode == "\022" then -- If a new visual selection is made, cancel any pending demotion - if M.state.demotion_timer then - M.state.demotion_timer:stop() - M.state.demotion_timer:close() - M.state.demotion_timer = nil - end + M._cancel_demotion_timer() current_selection = M.get_visual_selection() @@ -246,21 +240,25 @@ function M.update_selection() -- The 'current_selection' for comparison should also be this visual one. current_selection = M.state.latest_selection - if M.state.demotion_timer then -- Should not happen due to elseif, but as safeguard - M.state.demotion_timer:stop() - M.state.demotion_timer:close() - end + local timer = uv.new_timer() + assert(timer, "Expected uv.new_timer() to return a timer handle") - M.state.demotion_timer = vim.loop.new_timer() - M.state.demotion_timer:start( + M.state.demotion_timer = timer + timer:start( M.state.visual_demotion_delay_ms, 0, -- 0 repeat = one-shot vim.schedule_wrap(function() - if M.state.demotion_timer then -- Check if it wasn't cancelled right before firing - M.state.demotion_timer:stop() - M.state.demotion_timer:close() - M.state.demotion_timer = nil + -- Ignore stale timers (e.g., cancelled and replaced before callback runs) + if M.state.demotion_timer ~= timer then + return end + + -- Clear state so _cancel_demotion_timer() is a no-op if called after firing. + M.state.demotion_timer = nil + + timer:stop() + timer:close() + M.handle_selection_demotion(current_buf) -- Pass buffer at time of scheduling end) ) @@ -296,6 +294,10 @@ function M.handle_selection_demotion(original_bufnr_when_scheduled) -- Timer object is already stopped and cleared by its own callback wrapper or cancellation points. -- M.state.demotion_timer should be nil here if it fired normally or was cancelled. + if not M.state.tracking_enabled then + return + end + local current_buf = vim.api.nvim_get_current_buf() local claude_term_bufnr = terminal.get_active_terminal_bufnr() diff --git a/tests/selection_test.lua b/tests/selection_test.lua index 98a58931..e8acb30f 100644 --- a/tests/selection_test.lua +++ b/tests/selection_test.lua @@ -233,12 +233,6 @@ if not _G.vim then return timer end, - timer_stop = function(timer) - if timer and timer.stop then - timer:stop() - end - return true - end, }, test = { ---@type vim_test_utils @@ -453,6 +447,9 @@ describe("Selection module", function() -- A callback from a cancelled timer should be ignored. timer1:fire() assert.are.equal(0, update_calls) + -- Stale callback must not double-stop or double-close the already-cancelled timer. + assert.are.equal(1, timer1._stop_calls) + assert.are.equal(1, timer1._close_calls) timer2:fire() assert.are.equal(1, update_calls) @@ -476,6 +473,66 @@ describe("Selection module", function() end) end) + describe("demotion_timer", function() + local function install_terminal_stub() + local terminal_module = package.loaded["claudecode.terminal"] + local original_get = terminal_module and terminal_module.get_active_terminal_bufnr or nil + if not terminal_module then + terminal_module = {} + package.loaded["claudecode.terminal"] = terminal_module + end + terminal_module.get_active_terminal_bufnr = function() + return nil + end + return original_get, terminal_module + end + + it("disable() should cancel an active demotion timer and ignore stale callbacks", function() + local original_get, terminal_module = install_terminal_stub() + + selection.enable(mock_server) + + -- Seed a non-empty visual selection so the demotion path triggers on normal-mode entry. + selection.state.last_active_visual_selection = { + bufnr = 1, + selection_data = { + text = "x", + filePath = "/path/to/test.lua", + fileUrl = "file:///path/to/test.lua", + selection = { + start = { line = 0, character = 0 }, + ["end"] = { line = 0, character = 1 }, + isEmpty = false, + }, + }, + timestamp = 0, + } + selection.state.latest_selection = selection.state.last_active_visual_selection.selection_data + + _G.vim.test.set_mode("n") + selection.update_selection() + + local timer = selection.state.demotion_timer + assert(timer ~= nil) + + selection.disable() + + assert(selection.state.demotion_timer == nil) + assert(selection.state.latest_selection == nil) + assert.are.equal(1, timer._stop_calls) + assert.are.equal(1, timer._close_calls) + + -- A late-firing callback from the cancelled timer must not mutate state after teardown. + timer:fire() + assert(selection.state.latest_selection == nil) + assert(selection.state.demotion_timer == nil) + assert.are.equal(1, timer._stop_calls) + assert.are.equal(1, timer._close_calls) + + terminal_module.get_active_terminal_bufnr = original_get + end) + end) + it("should get cursor position in normal mode", function() local old_win_get_cursor = _G.vim.api.nvim_win_get_cursor _G.vim.api.nvim_win_get_cursor = function() From 40a1a611f36dd53214833411c410fab50dbd52a6 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 27 Apr 2026 18:41:03 +0200 Subject: [PATCH 3/4] fix(selection): clear visual cache on disable; cover demotion fire path Address review findings (P3, P3) on PR #245: - DEREM-18: disable() now resets last_active_visual_selection so a disable/enable cycle does not leave a stale entry that causes a phantom demotion timer on the first normal-mode cursor move. - DEREM-17: Add a demotion_timer test that fires the timer while it is still the active one, exercising the identity-guard, stop/close, and the handle_selection_demotion success path with full assertions. Change-Id: Id84ee382503d236a5dfd18109ff9ee0554f6993b Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Thomas Kosiewski --- lua/claudecode/selection.lua | 1 + tests/selection_test.lua | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/lua/claudecode/selection.lua b/lua/claudecode/selection.lua index d1e8e1b4..c06b32ce 100644 --- a/lua/claudecode/selection.lua +++ b/lua/claudecode/selection.lua @@ -46,6 +46,7 @@ function M.disable() M._clear_autocommands() M.state.latest_selection = nil + M.state.last_active_visual_selection = nil M.server = nil M._cancel_debounce_timer() diff --git a/tests/selection_test.lua b/tests/selection_test.lua index e8acb30f..a39a9e31 100644 --- a/tests/selection_test.lua +++ b/tests/selection_test.lua @@ -531,6 +531,57 @@ describe("Selection module", function() terminal_module.get_active_terminal_bufnr = original_get end) + + it("should demote to cursor position when timer fires normally", function() + local original_get, terminal_module = install_terminal_stub() + + selection.enable(mock_server) + + local visual_selection = { + text = "x", + filePath = "/path/to/test.lua", + fileUrl = "file:///path/to/test.lua", + selection = { + start = { line = 0, character = 0 }, + ["end"] = { line = 0, character = 1 }, + isEmpty = false, + }, + } + selection.state.last_active_visual_selection = { + bufnr = 1, + selection_data = visual_selection, + timestamp = 0, + } + selection.state.latest_selection = visual_selection + + _G.vim.test.set_mode("n") + _G.vim.test.set_cursor(0, 2, 3) + mock_server.last_broadcast = nil + + selection.update_selection() + + local timer = selection.state.demotion_timer + assert(timer ~= nil) + + timer:fire() + + assert(selection.state.demotion_timer == nil) + assert.are.equal(1, timer._stop_calls) + assert.are.equal(1, timer._close_calls) + + local demoted = selection.state.latest_selection + assert(demoted ~= nil) + assert.are.equal("", demoted.text) + assert.are.equal(true, demoted.selection.isEmpty) + assert.are.equal(1, demoted.selection.start.line) + assert.are.equal(3, demoted.selection.start.character) + assert(selection.state.last_active_visual_selection == nil) + assert(mock_server.last_broadcast ~= nil) + assert.are.equal("selection_changed", mock_server.last_broadcast.event) + + selection.disable() + terminal_module.get_active_terminal_bufnr = original_get + end) end) it("should get cursor position in normal mode", function() From 07ac16a5a1b66630b15759ba10d5da79f6ff2d21 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 27 Apr 2026 19:12:45 +0200 Subject: [PATCH 4/4] test(selection): assert last_active_visual_selection cleared on disable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: DEREM-19 — without this assertion, the disable() clear at selection.lua:49 could be silently reverted without test failure. Change-Id: Icefe72a3fb867a541fca10077636673d12f47a04 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Thomas Kosiewski --- tests/selection_test.lua | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/selection_test.lua b/tests/selection_test.lua index a39a9e31..1ba9d6e6 100644 --- a/tests/selection_test.lua +++ b/tests/selection_test.lua @@ -519,6 +519,7 @@ describe("Selection module", function() assert(selection.state.demotion_timer == nil) assert(selection.state.latest_selection == nil) + assert(selection.state.last_active_visual_selection == nil) assert.are.equal(1, timer._stop_calls) assert.are.equal(1, timer._close_calls) @@ -526,6 +527,7 @@ describe("Selection module", function() timer:fire() assert(selection.state.latest_selection == nil) assert(selection.state.demotion_timer == nil) + assert(selection.state.last_active_visual_selection == nil) assert.are.equal(1, timer._stop_calls) assert.are.equal(1, timer._close_calls)