fix: discussion window teardown and a recursion guard

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

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
 

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)

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)

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)