From ab3ff9516401e0fb54d186090aafb835ed5c9fed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:33:49 +0200 Subject: [PATCH 1/3] fix: close an orphaned discussion window `NuiSplit:unmount` sets an internal loading flag before destroying buffer and window and clears it only at the end, so an error in between leaves the flag set and every later unmount returns early without closing anything. `close` marked the split as gone regardless, so the window can stay on screen while `split_visible` says otherwise and the next toggle opens a second one beside it. No such failure was observed, that part is hardening. Close the window directly when the split does not, and keep `split_visible` set for as long as the window is alive. The WinClosed handler defers the teardown to the next tick. A buffer wiped from inside a WinClosed callback fires no BufWipeout, and the autocmds that reset `linked_bufnr` and `unlinked_bufnr` hang off that event, so a synchronous teardown leaves both fields holding the number of a wiped buffer. --- lua/gitlab/actions/discussions/init.lua | 35 ++++++- tests/spec/discussions_orphan_window_spec.lua | 98 +++++++++++++++++++ 2 files changed, 130 insertions(+), 3 deletions(-) create mode 100644 tests/spec/discussions_orphan_window_spec.lua diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 37fe68fc..835253af 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -155,7 +155,11 @@ M.open = function(callback, view_type) -- Set autocmd to clean up state when discussions split is closed manually vim.api.nvim_create_autocmd("WinClosed", { pattern = tostring(M.split.winid), - callback = M.close, + -- M.close deletes the tree buffers. Autocmds do not nest, so only outside this callback + -- (hence vim.schedule) does that fire BufWipeout and run the resets above. + callback = function() + vim.schedule(M.close) + end, }) -- Initialize winbar @@ -180,10 +184,35 @@ M.open = function(callback, view_type) end end ----Clear the discussion state and unmounts the split. +---Clear the discussion state and unmount the split. M.close = function() - if M.split then + if M.split == nil then + return + end + -- nui nils `split.winid` and `split.bufnr` as it tears them down, so read both while they + -- are still set. + local winid = M.split.winid + local split_bufnr = M.split.bufnr + if winid ~= nil and vim.api.nvim_win_is_valid(winid) then + local ok, err = pcall(vim.api.nvim_win_close, winid, true) + if not ok and tostring(err):find("E444") then + -- Last window in the session, so it needs a sibling before it can be closed. + vim.cmd("silent! vsplit") + ok = pcall(vim.api.nvim_win_close, winid, true) + end + if not ok then + u.notify("Could not close the discussion window", vim.log.levels.WARN) + return + end + end + -- Release nui's own buffer and augroups, which nothing else frees. Guarded so a failure + -- in there cannot skip the state cleanup below. + pcall(function() M.split:unmount() + end) + -- Reached only when unmount did not get that far, and nothing else would free that buffer. + if split_bufnr ~= nil and vim.api.nvim_buf_is_valid(split_bufnr) then + vim.api.nvim_buf_delete(split_bufnr, { force = true }) end M.split_visible = false M.discussion_tree = nil diff --git a/tests/spec/discussions_orphan_window_spec.lua b/tests/spec/discussions_orphan_window_spec.lua new file mode 100644 index 00000000..38895194 --- /dev/null +++ b/tests/spec/discussions_orphan_window_spec.lua @@ -0,0 +1,98 @@ +-- close() closes the window itself instead of leaving that to NuiSplit, which gives up on +-- the last window of a session and ignores every later unmount once one has failed. These +-- tests check that the window and nui's own buffer are gone afterwards and that +-- `split_visible` says so. + +local discussions = require("gitlab.actions.discussions") +local draft_notes = require("gitlab.actions.draft_notes") +local winbar = require("gitlab.actions.discussions.winbar") +local state = require("gitlab.state") + +---Register a split with the given unmount behaviour, in a window of its own. +---@param unmount fun(split: table) +---@return integer winid +---@return integer bufnr The scratch buffer NuiSplit allocates on mount +local function arrange(unmount) + vim.cmd("tabnew") + vim.cmd("split") + local winid = vim.api.nvim_get_current_win() + local bufnr = vim.api.nvim_create_buf(false, true) + discussions.split = { winid = winid, bufnr = bufnr, unmount = unmount } + discussions.split_visible = true + return winid, bufnr +end + +describe("actions/discussions.close", function() + after_each(function() + discussions.split = nil + discussions.split_visible = false + discussions.discussion_tree = nil + discussions.linked_bufnr = nil + discussions.unlinked_bufnr = nil + winbar.cleanup_timer() + state.DISCUSSION_DATA = nil + vim.cmd("tabnew") + vim.cmd("silent! tabonly") + vim.cmd("silent! only") + end) + + it("Closes the window itself when a poisoned split ignores unmount", function() + local winid, bufnr = arrange(function() end) + + discussions.close() + + assert.is_false(vim.api.nvim_win_is_valid(winid), ("window %d survived close()"):format(winid)) + assert.is_false(vim.api.nvim_buf_is_valid(bufnr), ("nui buffer %d survived close()"):format(bufnr)) + assert.is_false(discussions.split_visible) + end) + + it("Closes the window itself when unmounting raises", function() + local winid, bufnr = arrange(function() + error("nui teardown failed") + end) + + discussions.close() + + assert.is_false(vim.api.nvim_win_is_valid(winid), ("window %d survived close()"):format(winid)) + assert.is_false(vim.api.nvim_buf_is_valid(bufnr), ("nui buffer %d survived close()"):format(bufnr)) + assert.is_false(discussions.split_visible) + end) + + it("Closes the window when it is the last one in the session", function() + vim.cmd("silent! tabonly") + vim.cmd("silent! only") + -- Neovim refuses to close the last window, so close() has to open a sibling first. That + -- sibling shows the tree buffer, which is wiped a moment later. + local winid = vim.api.nvim_get_current_win() + local bufnr = vim.api.nvim_create_buf(true, false) + vim.api.nvim_win_set_buf(winid, bufnr) + discussions.split = { winid = winid, unmount = function() end } + discussions.split_visible = true + discussions.linked_bufnr = bufnr + + discussions.close() + + assert.is_false(vim.api.nvim_win_is_valid(winid), ("window %d survived close()"):format(winid)) + assert.is_false(vim.api.nvim_buf_is_valid(bufnr), ("buffer %d survived close()"):format(bufnr)) + assert.is_false(discussions.split_visible) + end) + + it("Tears the split down when the user closes the window by hand", function() + -- M.open calls draft_notes.rebuild_view, which talks to the Go server these tests + -- cannot connect to. + local original_rebuild_view = draft_notes.rebuild_view + draft_notes.rebuild_view = function() end + vim.cmd("tabnew") + discussions.open() + local winid = discussions.split.winid + + vim.api.nvim_win_close(winid, true) + + local torn_down = vim.wait(200, function() + return discussions.split_visible == false + end, 10) + + assert.is_true(torn_down, "split_visible is still set 200ms after the window closed") + draft_notes.rebuild_view = original_rebuild_view + end) +end) From 40e3a232203f5e09b0af3fc294aeec14ce4e9c3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:34:29 +0200 Subject: [PATCH 2/3] fix: release the discussion buffers on close The linked and unlinked buffers are created per open, not per session, so the pair the closing window leaves behind stays listed forever while the next open allocates a fresh one: two leaked buffers per open/close cycle. --- lua/gitlab/actions/discussions/init.lua | 16 +++-- tests/spec/discussions_shared_bufs_spec.lua | 72 +++++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 tests/spec/discussions_shared_bufs_spec.lua diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 835253af..c3600f7d 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -35,6 +35,17 @@ local M = { unlinked_discussion_tree = nil, } +---Delete discussion buffers to prevent leaked buffers on each M.open/M.close cycle. +---@param split_bufnr number? Passed in because `unmount` has already nil'd `M.split.bufnr`. +local function delete_bufs(split_bufnr) + -- pairs, because any of these might be nil + for _, bufnr in pairs({ split_bufnr, M.linked_bufnr, M.unlinked_bufnr }) do + if vim.api.nvim_buf_is_valid(bufnr) then + vim.api.nvim_buf_delete(bufnr, { force = true }) + end + end +end + ---Re-fetch all discussions and re-render the relevant view. ---TODO: simplify the function signature - "unlinked" and "all" should not be two booleans ---@param unlinked boolean @@ -210,12 +221,9 @@ M.close = function() pcall(function() M.split:unmount() end) - -- Reached only when unmount did not get that far, and nothing else would free that buffer. - if split_bufnr ~= nil and vim.api.nvim_buf_is_valid(split_bufnr) then - vim.api.nvim_buf_delete(split_bufnr, { force = true }) - end M.split_visible = false M.discussion_tree = nil + delete_bufs(split_bufnr) winbar.cleanup_timer() end diff --git a/tests/spec/discussions_shared_bufs_spec.lua b/tests/spec/discussions_shared_bufs_spec.lua new file mode 100644 index 00000000..36e332a1 --- /dev/null +++ b/tests/spec/discussions_shared_bufs_spec.lua @@ -0,0 +1,72 @@ +-- The linked and unlinked buffers belong to one open, not to the session, so close() owns +-- their release. + +local discussions = require("gitlab.actions.discussions") +local draft_notes = require("gitlab.actions.draft_notes") +local winbar = require("gitlab.actions.discussions.winbar") +local state = require("gitlab.state") + +-- Without this precondition the deletion asserts below would also pass if open() never +-- created the buffers in the first place. +local function assert_buffers_created(linked, unlinked) + assert.is_true(linked ~= nil and vim.api.nvim_buf_is_valid(linked), "open() created no linked buffer") + assert.is_true(unlinked ~= nil and vim.api.nvim_buf_is_valid(unlinked), "open() created no unlinked buffer") +end + +describe("actions/discussions buffers", function() + local original_rebuild_view + + before_each(function() + -- M.open tails into draft_notes.rebuild_view, which talks to the Go server these tests + -- have no connection to. + original_rebuild_view = draft_notes.rebuild_view + draft_notes.rebuild_view = function() end + end) + + after_each(function() + draft_notes.rebuild_view = original_rebuild_view + discussions.split = nil + discussions.split_visible = false + discussions.discussion_tree = nil + discussions.linked_bufnr = nil + discussions.unlinked_bufnr = nil + winbar.cleanup_timer() + state.DISCUSSION_DATA = nil + vim.cmd("tabnew") + vim.cmd("silent! tabonly") + vim.cmd("silent! only") + end) + + it("Deletes both buffers when the window is closed", function() + vim.cmd("tabnew") + discussions.open() + local linked, unlinked = discussions.linked_bufnr, discussions.unlinked_bufnr + assert_buffers_created(linked, unlinked) + + discussions.close() + + assert.is_false(vim.api.nvim_buf_is_valid(linked), ("linked buffer %d was not deleted"):format(linked)) + assert.is_false(vim.api.nvim_buf_is_valid(unlinked), ("unlinked buffer %d was not deleted"):format(unlinked)) + end) + + it("Leaves no buffer behind over an open/close cycle", function() + vim.cmd("tabnew") + discussions.open() + local first_linked, first_unlinked = discussions.linked_bufnr, discussions.unlinked_bufnr + assert_buffers_created(first_linked, first_unlinked) + discussions.close() + + discussions.open() + + assert.are_not.equal(first_linked, discussions.linked_bufnr) + assert.is_false( + vim.api.nvim_buf_is_valid(first_linked), + ("linked buffer %d of the first open leaked"):format(first_linked) + ) + assert.is_false( + vim.api.nvim_buf_is_valid(first_unlinked), + ("unlinked buffer %d of the first open leaked"):format(first_unlinked) + ) + discussions.close() + end) +end) From e567f527778678b031aaac0af22b3c813b9ead05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:11:55 +0200 Subject: [PATCH 3/3] fix: guard get_root_node against a nil parent A node of type "note" without `is_root` recurses into `tree:get_node(nil)`, which resolves a node from a window's cursor and can hand back the very same node. The call is in tail position, so the recursion never overflows the stack, it freezes Neovim. Give up on a nil parent, the way get_note_node already does. --- lua/gitlab/actions/common.lua | 5 +++++ tests/spec/common_root_node_spec.lua | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/spec/common_root_node_spec.lua diff --git a/lua/gitlab/actions/common.lua b/lua/gitlab/actions/common.lua index d328440a..ff346332 100644 --- a/lua/gitlab/actions/common.lua +++ b/lua/gitlab/actions/common.lua @@ -159,6 +159,11 @@ M.get_root_node = function(tree, node) end if node.type == "note_body" or node.type == "note" and not node.is_root then local parent_id = node:get_parent_id() + -- `tree:get_node(nil)` falls back to the node under the cursor, which can be this very + -- node again. Because this is a tail call recursion, it would loop forever instead of overflowing. + if parent_id == nil then + return nil + end return M.get_root_node(tree, tree:get_node(parent_id)) elseif node.is_root then return node diff --git a/tests/spec/common_root_node_spec.lua b/tests/spec/common_root_node_spec.lua new file mode 100644 index 00000000..6d7c69ae --- /dev/null +++ b/tests/spec/common_root_node_spec.lua @@ -0,0 +1,25 @@ +-- Without the nil-parent guard this test does not fail, it hangs, and the suite stops here. + +local NuiTree = require("nui.tree") +local common = require("gitlab.actions.common") + +describe("actions/common.get_root_node", function() + it("Gives up on a top level node that is not marked as a root", function() + local bufnr = vim.api.nvim_create_buf(false, true) + local tree = NuiTree({ + bufnr = bufnr, + nodes = { NuiTree.Node({ id = "a", text = "a", type = "note" }) }, + }) + tree:render() + -- The loop only forms if NuiTree can answer `get_node(nil)`, which it does from the + -- cursor of a window showing the buffer. + vim.api.nvim_win_set_buf(0, bufnr) + + assert.is_nil( + common.get_root_node(tree, tree:get_node("-a")), + "get_root_node claimed a root for a note node that has no parent" + ) + + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) +end)