From 0311874d67c3ff59373fa063a869483a04ea53bb 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 | 31 ++++++- tests/spec/discussions_orphan_window_spec.lua | 93 +++++++++++++++++++ 2 files changed, 120 insertions(+), 4 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..a6014236 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,11 +184,30 @@ 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 - M.split:unmount() + if M.split == nil then + return end + -- nui nils `split.winid` as soon as the window closes, so read it while it is still set. + local winid = M.split.winid + 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) M.split_visible = false M.discussion_tree = nil winbar.cleanup_timer() diff --git a/tests/spec/discussions_orphan_window_spec.lua b/tests/spec/discussions_orphan_window_spec.lua new file mode 100644 index 00000000..dd5bbb58 --- /dev/null +++ b/tests/spec/discussions_orphan_window_spec.lua @@ -0,0 +1,93 @@ +-- 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 is 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 +local function arrange(unmount) + vim.cmd("tabnew") + vim.cmd("split") + local winid = vim.api.nvim_get_current_win() + discussions.split = { winid = winid, unmount = unmount } + discussions.split_visible = true + return winid +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 = arrange(function() end) + + discussions.close() + + assert.is_false(vim.api.nvim_win_is_valid(winid), ("window %d survived close()"):format(winid)) + assert.is_false(discussions.split_visible) + end) + + it("Closes the window itself when unmounting raises", function() + local winid = 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(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 20852e3cf01b4c4924d0b3b354d7fafa3fba464f 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 | 11 ++++ tests/spec/discussions_shared_bufs_spec.lua | 72 +++++++++++++++++++++ 2 files changed, 83 insertions(+) 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 a6014236..a50262a2 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -35,6 +35,16 @@ local M = { unlinked_discussion_tree = nil, } +---Delete discussion buffers to prevent two leaked buffers on each M.open/M.close cycle. +local function delete_bufs() + if M.linked_bufnr ~= nil and vim.api.nvim_buf_is_valid(M.linked_bufnr) then + vim.api.nvim_buf_delete(M.linked_bufnr, { force = true }) + end + if M.unlinked_bufnr ~= nil and vim.api.nvim_buf_is_valid(M.unlinked_bufnr) then + vim.api.nvim_buf_delete(M.unlinked_bufnr, { force = true }) + 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,6 +220,7 @@ M.close = function() end) M.split_visible = false M.discussion_tree = nil + delete_bufs() 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 91f07733b9477f0acdabde4a17db2830bbda709e 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)