Skip to content

fix: discussion window teardown and a recursion guard - #581

Open
seflue wants to merge 3 commits into
harrisoncramer:developfrom
seflue:fix/discussion-window-teardown
Open

fix: discussion window teardown and a recursion guard#581
seflue wants to merge 3 commits into
harrisoncramer:developfrom
seflue:fix/discussion-window-teardown

Conversation

@seflue

@seflue seflue commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Three unrelated bugs, all found while working on #577.

A discussion window that can't be closed

Every so often gld leaves a discussion window on screen. It has to go by hand, and toggling again opens a new one beside it.

M.close cleared split_visible regardless of whether anything had been closed. From then on the plugin considered the tree gone while its window was still there, and the next toggle mounted a second one beside it.

NuiSplit:unmount() raises an internal loading flag before _buf_destroy() and _close_window() and clears it only once both return. An error in between leaves the object loading for good, and every later unmount() returns early without closing anything. One trigger is E444: Cannot close last window, raised inside _close_window when the split is the last window in the session. After that nui cannot close that tree at all.

M.close now closes the window itself when the split does not, and keeps split_visible set for as long as the window is alive; the WinClosed handler defers to the next tick, so the buffer wipe still fires BufWipeout. Whether that is the path behind the issues I observed I can't say; any error in between does it. #308 shows the same nui failure from the popup side.

A second window can also appear without any of this, when something splits the tree's window and the split keeps its buffer. That one is not addressed here, though the buffer release below does clear it up on the next toggle.

get_root_node can hang Neovim

Every reply in the tree is a node with type = "note" and no is_root, and get_root_node walks those up to their root. On a node without a parent, get_parent_id() returns nil and the walk calls tree:get_node(nil). NuiTree reads a missing id as "the node under the cursor" and hands back the node we started from. The recursion sits in tail position, so the stack never grows and nothing is raised: Neovim spins until you kill it.

get_note_node already returns nil on a missing parent, get_root_node does not. Every non-root note node currently sits under a root, but nothing enforces that. Both callers already report a nil root as an error, so returning nil is enough to make the failure visible. I ran into this while writing a test, which hung instead of failing.

Two leaked buffers per open/close cycle

:ls fills with stale gitlab buffers over a session. create_split_and_bufs allocates a fresh linked/unlinked pair on every open and nothing ever released the previous one. They are listed buffers, so until now you could reach a closed discussion tree again with :bnext, but after this you can't.

Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread tests/spec/discussions_shared_bufs_spec.lua
@seflue
seflue force-pushed the fix/discussion-window-teardown branch 2 times, most recently from 76ee693 to 9f8991e Compare August 3, 2026 17:08
@seflue
seflue requested a review from jakubbortlik August 3, 2026 17:21

@jakubbortlik jakubbortlik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've suggested some modifications to the docstrings/comments and expressed some remaining uncertainty. I don't mean to be nitpicky, trying to understand and hopefully make the comments more useful for other contributors as well :)

Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
@seflue
seflue force-pushed the fix/discussion-window-teardown branch from 9f8991e to 4345963 Compare August 3, 2026 23:23
@seflue
seflue requested a review from jakubbortlik August 4, 2026 00:13
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
@seflue
seflue force-pushed the fix/discussion-window-teardown branch from 4345963 to 868476f Compare August 4, 2026 13:19
@seflue
seflue requested a review from jakubbortlik August 4, 2026 14:25
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
Comment thread lua/gitlab/actions/discussions/init.lua Outdated
@seflue
seflue force-pushed the fix/discussion-window-teardown branch from 868476f to 06bb8d3 Compare August 6, 2026 18:59
@seflue
seflue requested a review from jakubbortlik August 6, 2026 19:01
seflue added 3 commits August 6, 2026 22:08
`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.
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.
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.
@seflue
seflue force-pushed the fix/discussion-window-teardown branch from 06bb8d3 to 91f0773 Compare August 6, 2026 20:08
@jakubbortlik

Copy link
Copy Markdown
Collaborator

One last thing (promise 🤞) . Last time I used tour branch the gld gld cycle of closing and reopening the split still seemed to leave some orphaned buffers behind (not sure though if that was gitlab.nvim's fault or some other plugin, I didn't try with a clean install and only the necessary dependencies).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants