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/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 37fe68fc..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 @@ -155,7 +165,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,13 +194,33 @@ 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 + delete_bufs() winbar.cleanup_timer() end 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) 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) 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)