From 64d90da33e598e84004a5f93ac22261edaecb70e 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/9] 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..c8885c2d 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 wipes the discussion buffers, and a buffer wiped from inside this callback + -- fires no BufWipeout, so the autocmds above would never reset the bufnr fields. + 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 c4c0951158fabdb01a094296668d08308917a820 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/9] 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 c8885c2d..42e8fac8 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 06bb8d3b9fb056be0ac4d1cc9baab5779b611221 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/9] 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) From cf9c233395c921a461cfb48fbfb336e4de30d185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:07:37 +0200 Subject: [PATCH 4/9] feat: browse MR's commit history --- README.md | 1 + doc/gitlab.nvim.txt | 11 +++++++ lua/gitlab/init.lua | 3 ++ lua/gitlab/reviewer/init.lua | 33 +++++++++++++++++++ lua/gitlab/state.lua | 7 ++++ tests/spec/history_browse_commits_spec.lua | 21 ++++++++++++ tests/spec/reviewer_autocommands_spec.lua | 38 ++++++++++++++++++++++ 7 files changed, 114 insertions(+) create mode 100644 tests/spec/history_browse_commits_spec.lua create mode 100644 tests/spec/reviewer_autocommands_spec.lua diff --git a/README.md b/README.md index ca64337f..b8e7b260 100644 --- a/README.md +++ b/README.md @@ -178,6 +178,7 @@ These keymaps are available globally (i.e., in any buffer). | `glC` | Create a new MR for currently checked-out feature branch | | `glc` | Chose MR for review | | `glS` | Start review for the currently checked-out branch | +| `glh` | Browse the MR's commit history, one commit at a time (read-only) | | `gl` | Load new MR state from Gitlab and apply new diff refs to the diff view | | `gls` | Show the editable summary of the MR | | `glu` | Copy the URL of the MR to the system clipboard | diff --git a/doc/gitlab.nvim.txt b/doc/gitlab.nvim.txt index 1868e5df..979e88e8 100644 --- a/doc/gitlab.nvim.txt +++ b/doc/gitlab.nvim.txt @@ -249,6 +249,7 @@ you call this function with no values the defaults will be used: create_mr = "glC", -- Create a new MR for currently checked-out feature branch choose_merge_request = "glc", -- Chose MR for review (if necessary check out the feature branch) start_review = "glS", -- Start review for the currently checked-out branch + browse_commits = "glh", -- Browse the MR's commit history, one commit at a time (read-only) reload_review = "gl", -- Load new MR state from Gitlab and apply new diff refs to the diff view summary = "gls", -- Show the editable summary of the MR copy_mr_url = "glu", -- Copy the URL of the MR to the system clipboard @@ -915,6 +916,16 @@ Opens the reviewer pane. Can be used from anywhere within Neovim after the plugin is loaded. If run twice, will open a second reviewer pane. >lua require("gitlab").review() +< + *gitlab.nvim.browse_commits* +gitlab.browse_commits() ~ + +Opens a read-only view of the MR's commit history (powered by Diffview's +file-history view), letting you step through it one commit at a time. Each entry +shows a single commit's isolated diff, to understand how the MR was built up. +Commenting is not available here; use `gitlab.review()` for that. +>lua + require("gitlab").browse_commits() < *gitlab.nvim.reload_review* gitlab.reload_review() ~ diff --git a/lua/gitlab/init.lua b/lua/gitlab/init.lua index a6412a21..4ca0f009 100644 --- a/lua/gitlab/init.lua +++ b/lua/gitlab/init.lua @@ -78,6 +78,9 @@ return { close_review = function() reviewer.close() end, + browse_commits = async.sequence({ info }, function() + reviewer.browse_commits() + end), pipeline = async.sequence({ latest_pipeline }, pipeline.open), merge = async.sequence({ u.merge(info, { refresh = true }) }, merge.merge), rebase = async.sequence({ u.merge(mergeability, { refresh = true }), info }, rebase.rebase), diff --git a/lua/gitlab/reviewer/init.lua b/lua/gitlab/reviewer/init.lua index 0336bf4b..9f7f022f 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -90,6 +90,32 @@ M.open = function() git.check_mr_in_good_condition() end +-- Opens a read-only, commit-by-commit browser for the MR range using Diffview's +-- FileHistory. Each entry shows a single commit's isolated diff, for understanding +-- how the MR was built up; commenting is not supported here (that stays in M.open). +M.browse_commits = function() + -- Diffview does not deduplicate views: DiffviewFileHistory always opens a new tabpage. + -- Focus the existing browser instead of stacking a second, orphaning the first (whose + -- keymaps would then reject every action, since only the newest tab passes the gate). + if M.history_tabid ~= nil and vim.api.nvim_tabpage_is_valid(M.history_tabid) then + vim.api.nvim_set_current_tabpage(M.history_tabid) + return + end + + local diff_refs = state.INFO.diff_refs + if diff_refs == nil then + u.notify("Gitlab did not provide diff refs required to browse this MR", vim.log.levels.ERROR) + return + end + + if diff_refs.base_sha == "" or diff_refs.head_sha == "" then + u.notify("Merge request contains no changes", vim.log.levels.ERROR) + return + end + + vim.api.nvim_command(string.format("DiffviewFileHistory --range=%s..%s", diff_refs.base_sha, diff_refs.head_sha)) +end + ---Close the reviewer and clean up. M.close = function() if M.tabid ~= nil and vim.api.nvim_tabpage_is_valid(M.tabid) then @@ -494,6 +520,13 @@ M.set_reviewer_autocommands = function(bufnr) group = group, buffer = bufnr, callback = function() + -- These autocommands manage the reviewer's own two windows, but they are + -- buffer-local and Diffview shares a revision's buffer across views, so the same + -- buffer shows up in the commit browser's tab. Acting there would strip the browse + -- keymaps and make the blob writable. Gate matches set_callback_for_buf_read. + if not (vim.api.nvim_get_current_tabpage() == M.tabid or (M.is_open and M.tabid == nil)) then + return + end if vim.api.nvim_get_current_win() == M.buf_winids[bufnr] then M.stored_win = vim.api.nvim_get_current_win() u.switch_can_edit_buf(bufnr, false) diff --git a/lua/gitlab/state.lua b/lua/gitlab/state.lua index f264abb9..9c14e7e6 100644 --- a/lua/gitlab/state.lua +++ b/lua/gitlab/state.lua @@ -94,6 +94,7 @@ M.settings = { create_mr = "glC", choose_merge_request = "glc", start_review = "glS", + browse_commits = "glh", reload_review = "gl", summary = "gls", copy_mr_url = "glu", @@ -344,6 +345,12 @@ M.set_global_keymaps = function() end, { desc = "Start Gitlab review", nowait = keymaps.global.start_review_nowait }) end + if keymaps.global.browse_commits then + vim.keymap.set("n", keymaps.global.browse_commits, function() + require("gitlab").browse_commits() + end, { desc = "Browse MR commit history", nowait = keymaps.global.browse_commits_nowait }) + end + if keymaps.global.reload_review then vim.keymap.set("n", keymaps.global.reload_review, function() require("gitlab").reload_review() diff --git a/tests/spec/history_browse_commits_spec.lua b/tests/spec/history_browse_commits_spec.lua new file mode 100644 index 00000000..983e0266 --- /dev/null +++ b/tests/spec/history_browse_commits_spec.lua @@ -0,0 +1,21 @@ +-- Verifies M.browse_commits focuses an already-open commit browser instead of opening a +-- second one: Diffview does not dedupe FileHistory views, so a second DiffviewFileHistory +-- call would orphan the first tab (see reviewer.clear_history_tab). + +local reviewer = require("gitlab.reviewer") + +describe("reviewer.browse_commits", function() + it("Switches to the existing tab instead of opening a new browser", function() + vim.cmd("tabnew") + local existing_tabid = vim.api.nvim_get_current_tabpage() + vim.cmd("tabnew") + + reviewer.history_tabid = existing_tabid + reviewer.browse_commits() + + assert.are.equal(existing_tabid, vim.api.nvim_get_current_tabpage()) + + vim.cmd("silent! tabonly") + reviewer.history_tabid = nil + end) +end) diff --git a/tests/spec/reviewer_autocommands_spec.lua b/tests/spec/reviewer_autocommands_spec.lua new file mode 100644 index 00000000..64275449 --- /dev/null +++ b/tests/spec/reviewer_autocommands_spec.lua @@ -0,0 +1,38 @@ +-- The reviewer's window autocommands are buffer-local, and Diffview shares a revision's +-- buffer between the reviewer and the commit browser, so they also fire in the browser's +-- tab. There they must do nothing: the else branch would delete the browse keymaps and +-- make the blob writable. + +describe("reviewer.set_reviewer_autocommands", function() + local reviewer = require("gitlab.reviewer") + local state = require("gitlab.state") + + it("Leaves the buffer alone outside the reviewer tab", function() + local reviewer_tabid = vim.api.nvim_get_current_tabpage() + vim.cmd("tabnew") + local bufnr = vim.api.nvim_create_buf(false, true) + vim.api.nvim_win_set_buf(0, bufnr) + vim.keymap.set("n", state.settings.keymaps.reviewer.create_comment, function() end, { buffer = bufnr }) + vim.api.nvim_set_option_value("modifiable", false, { buf = bufnr }) + + reviewer.tabid = reviewer_tabid + reviewer.is_open = true + -- Matching ids drive the else branch into its modifiable-and-unmap path, which the + -- tab gate has to prevent from being reached at all. + reviewer.diffview_layout = { b = { id = -1 } } + reviewer.buf_winids[bufnr] = -1 + reviewer.set_reviewer_autocommands(bufnr) + + vim.api.nvim_exec_autocmds("WinEnter", { buffer = bufnr }) + + assert.is_false(vim.api.nvim_get_option_value("modifiable", { buf = bufnr })) + assert.are.equal(1, #vim.api.nvim_buf_get_keymap(bufnr, "n")) + + vim.cmd("tabclose") + reviewer.tabid = nil + reviewer.is_open = false + reviewer.diffview_layout = nil + reviewer.buf_winids[bufnr] = nil + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) +end) From fb00ded8893e2e3f9d26573ac21a7636b8211ae3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Sun, 2 Aug 2026 08:57:08 +0200 Subject: [PATCH 5/9] feat: comment while browsing MR commits c comments on the line under the cursor while browsing a MR commit by commit. The comment is anchored to the commit being viewed: the position's head_sha is that commit and a top-level commit_id is sent alongside it, which binds the note to that commit's diff while keeping it MR-scoped. Every line of the commit's new side is commentable. GitLab overwrites base_sha and start_sha with the MR base whatever is sent, so a line the commit deletes cannot be positioned from here: its old_line would be numbered against the MR base, while the browser shows the file at the commit's parent. Commenting from the old side is refused rather than guessed at. Such a note is marked in the browser and nowhere else. Its lines are relative to that commit's own diff, so in the MR diff the marker would sit on an unrelated line. a jumps from the marker into the discussion tree, as it does in the reviewer. ]v and [v follow the line under the cursor to the next or previous commit that changes it, via git log -L. That is navigation only, and independent of commenting. refresh_diagnostics() no longer errors when no regular reviewer view is open, which commenting from the browser would otherwise trigger on an otherwise successful comment. It still errors when a reviewer is open, where a missing view is a broken state. --- README.md | 19 +- after/syntax/gitlab.vim | 8 +- cmd/app/comment.go | 4 + cmd/app/comment_helpers.go | 1 + cmd/app/comment_test.go | 61 +- cmd/app/draft_notes.go | 4 + cmd/app/draft_notes_test.go | 48 ++ doc/gitlab.nvim.txt | 19 +- lua/gitlab/actions/comment.lua | 43 +- lua/gitlab/actions/common.lua | 14 +- lua/gitlab/actions/discussions/init.lua | 1 + lua/gitlab/actions/discussions/tree.lua | 25 +- lua/gitlab/actions/draft_notes/init.lua | 5 +- lua/gitlab/annotations.lua | 1 + lua/gitlab/colors.lua | 1 + lua/gitlab/indicators/common.lua | 43 +- lua/gitlab/indicators/diagnostics.lua | 39 +- lua/gitlab/init.lua | 8 + lua/gitlab/reviewer/history.lua | 527 ++++++++++++++++++ lua/gitlab/reviewer/history_diff.lua | 61 ++ lua/gitlab/reviewer/history_log.lua | 75 +++ lua/gitlab/reviewer/init.lua | 24 +- lua/gitlab/state.lua | 3 + tests/spec/comment_spec.lua | 50 ++ tests/spec/commit_comment_jump_spec.lua | 163 ++++++ tests/spec/discussions_commit_marker_spec.lua | 98 ++++ tests/spec/history_comment_spec.lua | 172 ++++++ tests/spec/history_diagnostics_spec.lua | 220 ++++++++ tests/spec/history_diff_spec.lua | 102 ++++ tests/spec/history_keymaps_spec.lua | 99 ++++ tests/spec/history_log_spec.lua | 96 ++++ tests/spec/history_select_commit_spec.lua | 116 ++++ tests/spec/history_tab_spec.lua | 18 + tests/spec/indicators_common_filter_spec.lua | 73 +++ 34 files changed, 2198 insertions(+), 43 deletions(-) create mode 100644 lua/gitlab/reviewer/history.lua create mode 100644 lua/gitlab/reviewer/history_diff.lua create mode 100644 lua/gitlab/reviewer/history_log.lua create mode 100644 tests/spec/comment_spec.lua create mode 100644 tests/spec/commit_comment_jump_spec.lua create mode 100644 tests/spec/discussions_commit_marker_spec.lua create mode 100644 tests/spec/history_comment_spec.lua create mode 100644 tests/spec/history_diagnostics_spec.lua create mode 100644 tests/spec/history_diff_spec.lua create mode 100644 tests/spec/history_keymaps_spec.lua create mode 100644 tests/spec/history_log_spec.lua create mode 100644 tests/spec/history_select_commit_spec.lua create mode 100644 tests/spec/history_tab_spec.lua create mode 100644 tests/spec/indicators_common_filter_spec.lua diff --git a/README.md b/README.md index b8e7b260..ff16feba 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,7 @@ These keymaps are available globally (i.e., in any buffer). | `glC` | Create a new MR for currently checked-out feature branch | | `glc` | Chose MR for review | | `glS` | Start review for the currently checked-out branch | -| `glh` | Browse the MR's commit history, one commit at a time (read-only) | +| `glh` | Browse the MR's commit history, one commit at a time | | `gl` | Load new MR state from Gitlab and apply new diff refs to the diff view | | `gls` | Show the editable summary of the MR | | `glu` | Copy the URL of the MR to the system clipboard | @@ -240,6 +240,23 @@ These `keymaps` are active in the reviewer window (the diff view). | `s` | Create a suggestion for the lines that the following {motion} moves over | | `a` | Jump to the comment in the discussion tree | +#### Commit Browser Keymaps + +These `keymaps` are active in the commit browser (`glh`), which steps through the +MR one commit at a time. + +| Keys | Action | +| ---- | -------------------------------------------------------------------------------- | +| `c` | Comment on the current line, anchored to the commit being viewed | +| `a` | Jump to the comment in the discussion tree | +| `]v` | Follow this line to the next newer commit that touches it (toward the MR head) | +| `[v` | Follow this line to the next older commit that touches it (toward the MR base) | +| `g?` | Show these keymaps | + +`c` only works on the new (right) side; commenting on a line the commit deletes +(left side) is not supported. `]v` / `[v` follow the line to the next commit +that changes it, then fall back to the next commit that merely touches the file. + ## Contributing Contributions to the plugin are welcome. Please read [.github/CONTRIBUTING.md](.github/CONTRIBUTING.md) before you start working on a pull request. diff --git a/after/syntax/gitlab.vim b/after/syntax/gitlab.vim index 5922f296..5e4ccd57 100644 --- a/after/syntax/gitlab.vim +++ b/after/syntax/gitlab.vim @@ -11,10 +11,13 @@ let formatted_date = '\w\+ \{1,2}\d\{1,2}, \d\{4}' let absolute_time = '\d\{2}/\d\{2}/\d\{4} at \d\{2}:\d\{2}' let date = '\%(' . time_ago . '\|' . formatted_date . '\|' . absolute_time . '\|just now\)' -let published = date . ' \%(' . g:gitlab_discussion_tree_resolved . '\|' . g:gitlab_discussion_tree_unresolved . '\|' . g:gitlab_discussion_tree_unlinked . '\)\?' +" Commits are referenced by the first 7 characters of their SHA, e.g. '1a2b3c4' +let commit_ref = '[0-9a-f]\{7}' + +let published = date . '\%( ' . commit_ref . '\)\?' . ' \%(' . g:gitlab_discussion_tree_resolved . '\|' . g:gitlab_discussion_tree_unresolved . '\|' . g:gitlab_discussion_tree_unlinked . '\)\?' let state = ' \%(' . published . '\|' . g:gitlab_discussion_tree_draft . '\)' -execute 'syntax match GitlabNoteHeader "' . expanders . username . state . '" contains=GitlabDate,GitlabUnresolved,GitlabUnlinked,GitlabResolved,GitlabExpander,GitlabDraft,GitlabUsername' +execute 'syntax match GitlabNoteHeader "' . expanders . username . state . '" contains=GitlabDate,GitlabUnresolved,GitlabUnlinked,GitlabResolved,GitlabExpander,GitlabDraft,GitlabUsername,GitlabCommit' execute 'syntax match GitlabDate "' . date . '" contained' execute 'syntax match GitlabUnresolved "' . g:gitlab_discussion_tree_unresolved . '" contained' @@ -24,5 +27,6 @@ execute 'syntax match GitlabExpander "' . expanders . '" contained' execute 'syntax match GitlabDraft "' . g:gitlab_discussion_tree_draft . '" contained' execute 'syntax match GitlabUsername "' . username . '" contained' execute 'syntax match GitlabMention "' . username . '"' +execute 'syntax match GitlabCommit "' . commit_ref . '" contained' let b:current_syntax = 'gitlab' diff --git a/cmd/app/comment.go b/cmd/app/comment.go index cdd88074..e81f511a 100644 --- a/cmd/app/comment.go +++ b/cmd/app/comment.go @@ -97,6 +97,10 @@ func (a commentService) postComment(w http.ResponseWriter, r *http.Request) { opt.Position = buildCommentPosition(commentWithPositionData) } + if payload.CommitID != "" { + opt.CommitID = &payload.CommitID + } + discussion, res, err := a.client.CreateMergeRequestDiscussion(a.projectInfo.ProjectId, a.projectInfo.MergeId, &opt) if err != nil { diff --git a/cmd/app/comment_helpers.go b/cmd/app/comment_helpers.go index 05881da2..4d6552ac 100644 --- a/cmd/app/comment_helpers.go +++ b/cmd/app/comment_helpers.go @@ -31,6 +31,7 @@ type PositionData struct { StartCommitSHA string `json:"start_commit_sha"` Type string `json:"type"` LineRange *LineRange `json:"line_range,omitempty"` + CommitID string `json:"commit_id,omitempty"` } /* RequestWithPosition is an interface that abstracts the handling of position data for a comment or a draft comment */ diff --git a/cmd/app/comment_test.go b/cmd/app/comment_test.go index a10ef5fc..de76480f 100644 --- a/cmd/app/comment_test.go +++ b/cmd/app/comment_test.go @@ -9,6 +9,7 @@ import ( type fakeCommentClient struct { testBase + capturedOpt **gitlab.CreateMergeRequestDiscussionOptions } func (f fakeCommentClient) CreateMergeRequestDiscussion(pid interface{}, mergeRequest int64, opt *gitlab.CreateMergeRequestDiscussionOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Discussion, *gitlab.Response, error) { @@ -17,6 +18,10 @@ func (f fakeCommentClient) CreateMergeRequestDiscussion(pid interface{}, mergeRe return nil, nil, err } + if f.capturedOpt != nil { + *f.capturedOpt = opt + } + return &gitlab.Discussion{Notes: []*gitlab.Note{{}}}, resp, err } func (f fakeCommentClient) UpdateMergeRequestDiscussionNote(pid interface{}, mergeRequest int64, discussion string, note int64, opt *gitlab.UpdateMergeRequestDiscussionNoteOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Note, *gitlab.Response, error) { @@ -76,10 +81,62 @@ func TestPostComment(t *testing.T) { assert(t, data.Message, "Comment created successfully") }) + t.Run("Passes commit_id through to the Gitlab client when provided", func(t *testing.T) { + testCommentCreationData := PostCommentRequest{ + Comment: "Some comment", + PositionData: PositionData{ + FileName: "file.txt", + CommitID: "abc123", + }, + } + request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData) + var capturedOpt *gitlab.CreateMergeRequestDiscussionOptions + svc := middleware( + commentService{testProjectData, fakeCommentClient{capturedOpt: &capturedOpt}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{ + http.MethodPost: newPayload[PostCommentRequest], + http.MethodDelete: newPayload[DeleteCommentRequest], + http.MethodPatch: newPayload[EditCommentRequest], + }), + withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch), + ) + getSuccessData(t, svc, request) + if capturedOpt.CommitID == nil { + t.Fatal("expected CommitID to be set") + } + assert(t, *capturedOpt.CommitID, "abc123") + }) + + t.Run("Leaves commit_id unset when not provided", func(t *testing.T) { + testCommentCreationData := PostCommentRequest{ + Comment: "Some comment", + PositionData: PositionData{ + FileName: "file.txt", + }, + } + request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData) + var capturedOpt *gitlab.CreateMergeRequestDiscussionOptions + svc := middleware( + commentService{testProjectData, fakeCommentClient{capturedOpt: &capturedOpt}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{ + http.MethodPost: newPayload[PostCommentRequest], + http.MethodDelete: newPayload[DeleteCommentRequest], + http.MethodPatch: newPayload[EditCommentRequest], + }), + withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch), + ) + getSuccessData(t, svc, request) + if capturedOpt.CommitID != nil { + t.Fatalf("expected CommitID to be nil, got %q", *capturedOpt.CommitID) + } + }) + t.Run("Handles errors from Gitlab client", func(t *testing.T) { request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData) svc := middleware( - commentService{testProjectData, fakeCommentClient{testBase{errFromGitlab: true}}}, + commentService{testProjectData, fakeCommentClient{testBase: testBase{errFromGitlab: true}}}, withMr(testProjectData, fakeMergeRequestLister{}), withPayloadValidation(methodToPayload{ http.MethodPost: newPayload[PostCommentRequest], @@ -95,7 +152,7 @@ func TestPostComment(t *testing.T) { t.Run("Handles non-200s from Gitlab client", func(t *testing.T) { request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData) svc := middleware( - commentService{testProjectData, fakeCommentClient{testBase{status: http.StatusSeeOther}}}, + commentService{testProjectData, fakeCommentClient{testBase: testBase{status: http.StatusSeeOther}}}, withMr(testProjectData, fakeMergeRequestLister{}), withPayloadValidation(methodToPayload{ http.MethodPost: newPayload[PostCommentRequest], diff --git a/cmd/app/draft_notes.go b/cmd/app/draft_notes.go index 102dcba7..a036e2a2 100644 --- a/cmd/app/draft_notes.go +++ b/cmd/app/draft_notes.go @@ -112,6 +112,10 @@ func (a draftNoteService) postDraftNote(w http.ResponseWriter, r *http.Request) opt.Position = buildCommentPosition(draftNoteWithPosition) } + if payload.CommitID != "" { + opt.CommitID = &payload.CommitID + } + draftNote, res, err := a.client.CreateDraftNote(a.projectInfo.ProjectId, a.projectInfo.MergeId, &opt) if err != nil { diff --git a/cmd/app/draft_notes_test.go b/cmd/app/draft_notes_test.go index f92f1570..f8c3ee1e 100644 --- a/cmd/app/draft_notes_test.go +++ b/cmd/app/draft_notes_test.go @@ -9,6 +9,7 @@ import ( type fakeDraftNoteManager struct { testBase + capturedOpt **gitlab.CreateDraftNoteOptions } func (f fakeDraftNoteManager) ListDraftNotes(pid interface{}, mergeRequest int64, opt *gitlab.ListDraftNotesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.DraftNote, *gitlab.Response, error) { @@ -24,6 +25,9 @@ func (f fakeDraftNoteManager) CreateDraftNote(pid interface{}, mergeRequest int6 if err != nil { return nil, nil, err } + if f.capturedOpt != nil { + *f.capturedOpt = opt + } return &gitlab.DraftNote{}, resp, err } @@ -104,6 +108,50 @@ func TestPostDraftNote(t *testing.T) { data := getSuccessData(t, svc, request) assert(t, data.Message, "Draft note created successfully") }) + + t.Run("Passes commit_id through to the Gitlab client when provided", func(t *testing.T) { + testData := PostDraftNoteRequest{ + Comment: "Some comment", + PositionData: PositionData{ + FileName: "file.txt", + CommitID: "abc123", + }, + } + request := makeRequest(t, http.MethodPost, "/mr/draft_notes/", testData) + var capturedOpt *gitlab.CreateDraftNoteOptions + svc := middleware( + draftNoteService{testProjectData, fakeDraftNoteManager{capturedOpt: &capturedOpt}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{ + http.MethodPost: newPayload[PostDraftNoteRequest], + http.MethodPatch: newPayload[UpdateDraftNoteRequest], + }), + withMethodCheck(http.MethodGet, http.MethodPost, http.MethodPatch, http.MethodDelete), + ) + getSuccessData(t, svc, request) + if capturedOpt.CommitID == nil { + t.Fatal("expected CommitID to be set") + } + assert(t, *capturedOpt.CommitID, "abc123") + }) + + t.Run("Leaves commit_id unset when not provided", func(t *testing.T) { + request := makeRequest(t, http.MethodPost, "/mr/draft_notes/", testPostDraftNoteRequestData) + var capturedOpt *gitlab.CreateDraftNoteOptions + svc := middleware( + draftNoteService{testProjectData, fakeDraftNoteManager{capturedOpt: &capturedOpt}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{ + http.MethodPost: newPayload[PostDraftNoteRequest], + http.MethodPatch: newPayload[UpdateDraftNoteRequest], + }), + withMethodCheck(http.MethodGet, http.MethodPost, http.MethodPatch, http.MethodDelete), + ) + getSuccessData(t, svc, request) + if capturedOpt.CommitID != nil { + t.Fatalf("expected CommitID to be nil, got %q", *capturedOpt.CommitID) + } + }) } func TestDeleteDraftNote(t *testing.T) { diff --git a/doc/gitlab.nvim.txt b/doc/gitlab.nvim.txt index 979e88e8..5bbb4f1a 100644 --- a/doc/gitlab.nvim.txt +++ b/doc/gitlab.nvim.txt @@ -249,7 +249,7 @@ you call this function with no values the defaults will be used: create_mr = "glC", -- Create a new MR for currently checked-out feature branch choose_merge_request = "glc", -- Chose MR for review (if necessary check out the feature branch) start_review = "glS", -- Start review for the currently checked-out branch - browse_commits = "glh", -- Browse the MR's commit history, one commit at a time (read-only) + browse_commits = "glh", -- Browse the MR's commit history, one commit at a time reload_review = "gl", -- Load new MR state from Gitlab and apply new diff refs to the diff view summary = "gls", -- Show the editable summary of the MR copy_mr_url = "glu", -- Copy the URL of the MR to the system clipboard @@ -298,6 +298,8 @@ you call this function with no values the defaults will be used: create_comment = "c", -- Create a comment for the lines that the following {motion} moves over. Repeat the key(s) for creating comment for the current line create_suggestion = "s", -- Create a suggestion for the lines that the following {motion} moves over. Repeat the key(s) for creating comment for the current line move_to_discussion_tree = "a", -- Jump to the comment in the discussion tree + history_next_version = "]v", -- In the commit browser, follow this line to the next newer commit that touches it + history_prev_version = "[v", -- In the commit browser, follow this line to the next older commit that touches it }, }, popup = { -- The popup for comment creation, editing, and replying @@ -477,6 +479,7 @@ you call this function with no values the defaults will be used: file_name = "Normal", resolved = "DiagnosticSignOk", unresolved = "DiagnosticSignWarn", + commit = "DiagnosticSignInfo", draft = "DiffviewNonText", draft_mode = "DiagnosticWarn", live_mode = "DiagnosticOk", @@ -920,10 +923,16 @@ plugin is loaded. If run twice, will open a second reviewer pane. *gitlab.nvim.browse_commits* gitlab.browse_commits() ~ -Opens a read-only view of the MR's commit history (powered by Diffview's -file-history view), letting you step through it one commit at a time. Each entry -shows a single commit's isolated diff, to understand how the MR was built up. -Commenting is not available here; use `gitlab.review()` for that. +Opens a view of the MR's commit history, letting you step through it one commit +at a time. Each entry shows a single commit's isolated diff, to understand how +the MR was built up. + +Press `c` to comment on a line, anchored to the commit being viewed. This only +works on the new (right) side; commenting on a line the commit deletes (left +side) is not supported. Use `]v` / `[v` to follow the line to the next commit +that changes it, then to the next commit that merely touches the file. Press +`a` on a commented line to jump to the comment in the discussion tree. `g?` +lists these keymaps. >lua require("gitlab").browse_commits() < diff --git a/lua/gitlab/actions/comment.lua b/lua/gitlab/actions/comment.lua index 862521af..601d1235 100644 --- a/lua/gitlab/actions/comment.lua +++ b/lua/gitlab/actions/comment.lua @@ -21,6 +21,26 @@ local M = { comment_popup = nil, } +---Build the position_data payload for a positioned comment, anchored to the MR's current +---revision or, with `M.location.commit_override` set, to that single commit. Gitlab +---rejects a commit_id whose position refs do not describe the commit's own diff. +---@return table +M.build_position_data = function() + local revision = state.MR_REVISIONS[1] + local override = M.location.commit_override + return { + file_name = M.location.reviewer_data.file_name, + old_file_name = M.location.reviewer_data.old_file_name, + base_commit_sha = override and override.base_sha or revision.base_commit_sha, + start_commit_sha = override and override.start_sha or revision.start_commit_sha, + head_commit_sha = override and override.head_sha or revision.head_commit_sha, + old_line = M.location.location_data.old_line, + new_line = M.location.location_data.new_line, + line_range = M.location.location_data.line_range, + commit_id = override and override.commit_id, + } +end + ---Fire the API to send the comment data to the Go server. ---@param text string comment text ---@param unlinked boolean if true, the comment is not linked to a line @@ -72,17 +92,7 @@ local confirm_create_comment = function(text, unlinked, discussion_id) return end - local revision = state.MR_REVISIONS[1] - local position_data = { - file_name = M.location.reviewer_data.file_name, - old_file_name = M.location.reviewer_data.old_file_name, - base_commit_sha = revision.base_commit_sha, - start_commit_sha = revision.start_commit_sha, - head_commit_sha = revision.head_commit_sha, - old_line = M.location.location_data.old_line, - new_line = M.location.location_data.new_line, - line_range = M.location.location_data.line_range, - } + local position_data = M.build_position_data() -- Creating a new comment (linked to specific changes) local body = u.merge({ type = "text", comment = text }, position_data) @@ -236,6 +246,17 @@ M.create_note = function() layout:mount() end +---Open a comment popup for a `location` the caller resolved itself, instead of reading the +---live reviewer. +---@param location table reviewer_data (file_name, old_file_name, new_sha_focused), +---location_data (old_line, new_line, line_range), visual_range (start_line, end_line), and +---optionally commit_override (base_sha, start_sha, head_sha, commit_id) +M.create_comment_for_location = function(location) + M.location = location + local layout = M.create_comment_layout({ unlinked = false }) + layout:mount() +end + ---Given the current visually selected area of text, builds text to fill in the ---comment popup with a suggested change ---@return LineRange? diff --git a/lua/gitlab/actions/common.lua b/lua/gitlab/actions/common.lua index ff346332..567b9c1a 100644 --- a/lua/gitlab/actions/common.lua +++ b/lua/gitlab/actions/common.lua @@ -307,13 +307,25 @@ M.jump_to_reviewer = function(tree) u.notify("Could not get line number", vim.log.levels.ERROR) return end + -- A commit-anchored comment has no position in the MR's changeset. Its line numbers + -- only mean anything in that commit's own diff, which is what the commit browser shows. + if root_node.commit_id ~= nil then + -- An old-side line is numbered against the MR base, while the browser shows + -- `parent..commit`. Only new-side lines carry over unchanged. + if not is_new_sha then + u.notify("Cannot jump to a commit comment left on a deleted line", vim.log.levels.WARN) + return + end + require("gitlab.reviewer.history").jump_to_commit(root_node.commit_id, root_node.file_name, line_number) + return + end reviewer.jump(root_node.file_name, root_node.old_file_name, line_number, is_new_sha) end ---Jump to the file in a new tab. ---@param tree NuiTree M.jump_to_file = function(tree) - local node = tree:get_node() + local node = M.get_current_node(tree) local root_node = M.get_root_node(tree, node) if root_node == nil then u.notify("Could not get discussion node", vim.log.levels.ERROR) diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 42e8fac8..0e95610b 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -116,6 +116,7 @@ end M.refresh_diagnostics = function() if state.settings.discussion_signs.enabled then diagnostics.refresh_diagnostics() + require("gitlab.reviewer.history").refresh_diagnostics() end common.add_empty_titles() end diff --git a/lua/gitlab/actions/discussions/tree.lua b/lua/gitlab/actions/discussions/tree.lua index 4672b3ac..5cd3b1cc 100644 --- a/lua/gitlab/actions/discussions/tree.lua +++ b/lua/gitlab/actions/discussions/tree.lua @@ -41,10 +41,13 @@ M.add_discussions_to_table = function(items, unlinked) local root_new_line = nil local root_old_line = nil local root_url + ---@type string? + local root_commit_id for j, note in ipairs(discussion.notes) do if j == 1 then - _, root_text, root_text_nodes = M.build_note(note, { resolved = note.resolved, resolvable = note.resolvable }) + _, root_text, root_text_nodes = + M.build_note(note, { resolved = note.resolved, resolvable = note.resolvable }, true) root_file_name = (type(note.position) == "table" and note.position.new_path or nil) root_old_file_name = (type(note.position) == "table" and note.position.old_path or nil) root_new_line = (type(note.position) == "table" and note.position.new_line or nil) @@ -55,6 +58,9 @@ M.add_discussions_to_table = function(items, unlinked) resolved = note.resolved root_url = state.INFO.web_url .. "#note_" .. note.id range = (type(note.position) == "table" and note.position.line_range or nil) + -- go-gitlab types commit_id as a plain string, so a note without a commit arrives + -- as "", never nil + root_commit_id = (note.commit_id ~= nil and note.commit_id ~= "") and note.commit_id or nil else -- Otherwise insert it as a child node... local note_node = M.build_note(note) table.insert(discussion_children, note_node) @@ -89,6 +95,7 @@ M.add_discussions_to_table = function(items, unlinked) resolvable = resolvable, resolved = resolved, url = root_url, + commit_id = root_commit_id, }, body) table.insert(t, root_node) @@ -272,9 +279,10 @@ end ---Build note node body. ---@param note Note|DraftNote ---@param resolve_info? ResolveInfo Nil if the note is a child node +---@param is_root? boolean True if the note is the root of a discussion or draft note ---@return string ---@return NuiTree.Node[] -local function build_note_body(note, resolve_info) +local function build_note_body(note, resolve_info, is_root) local text_nodes = {} local i = 0 for body_line in u.split_by_new_lines(note.body or note.note) do @@ -300,7 +308,13 @@ local function build_note_body(note, resolve_info) symbol = state.settings.discussion_tree.unlinked end - local noteHeader = common.build_note_header(note) .. " " .. symbol + -- The marker belongs to the discussion, so it goes on the root and not on every reply + local commit_marker = "" + if is_root and note.commit_id and note.commit_id ~= "" then + commit_marker = " " .. note.commit_id:sub(1, 7) + end + + local noteHeader = common.build_note_header(note) .. commit_marker .. " " .. symbol return noteHeader, text_nodes end @@ -308,11 +322,12 @@ end ---Build note node. ---@param note Note|DraftNote ---@param resolve_info? ResolveInfo Nil if the note is a child node +---@param is_root? boolean True if the note is the root of a discussion or draft note ---@return NuiTree.Node ---@return string ---@return NuiTree.Node[] -M.build_note = function(note, resolve_info) - local text, text_nodes = build_note_body(note, resolve_info) +M.build_note = function(note, resolve_info, is_root) + local text, text_nodes = build_note_body(note, resolve_info, is_root) local note_node = NuiTree.Node({ text = text, is_draft = note.note ~= nil, diff --git a/lua/gitlab/actions/draft_notes/init.lua b/lua/gitlab/actions/draft_notes/init.lua index ec2b2aec..fd8100fa 100755 --- a/lua/gitlab/actions/draft_notes/init.lua +++ b/lua/gitlab/actions/draft_notes/init.lua @@ -150,7 +150,7 @@ end ---@param note DraftNote ---@return NuiTree.Node M.build_root_draft_note = function(note) - local _, root_text, root_text_nodes = discussion_tree.build_note(note) + local _, root_text, root_text_nodes = discussion_tree.build_note(note, nil, true) return NuiTree.Node({ range = (type(note.position) == "table" and note.position.line_range or nil), text = root_text, @@ -166,6 +166,9 @@ M.build_root_draft_note = function(note) resolvable = false, resolved = false, url = state.INFO.web_url .. "#note_" .. note.id, + -- go-gitlab types commit_id as a plain string, so a note without a commit arrives as + -- "", never nil + commit_id = (note.commit_id ~= nil and note.commit_id ~= "") and note.commit_id or nil, }, root_text_nodes) end diff --git a/lua/gitlab/annotations.lua b/lua/gitlab/annotations.lua index 7aee76cf..294dc05c 100644 --- a/lua/gitlab/annotations.lua +++ b/lua/gitlab/annotations.lua @@ -197,6 +197,7 @@ ---@field file_name? string ---@field resolved? string ---@field unresolved? string +---@field commit? string ---@field draft? string ---@field draft_mode? string ---@field live_mode? string diff --git a/lua/gitlab/colors.lua b/lua/gitlab/colors.lua index bf29dec3..66676fd7 100644 --- a/lua/gitlab/colors.lua +++ b/lua/gitlab/colors.lua @@ -30,6 +30,7 @@ vim.api.nvim_create_autocmd({ "VimEnter", "ColorScheme" }, { vim.api.nvim_set_hl(0, "GitlabFileName", get_colors_for_group(discussion_colors.file_name)) vim.api.nvim_set_hl(0, "GitlabResolved", get_colors_for_group(discussion_colors.resolved)) vim.api.nvim_set_hl(0, "GitlabUnresolved", get_colors_for_group(discussion_colors.unresolved)) + vim.api.nvim_set_hl(0, "GitlabCommit", get_colors_for_group(discussion_colors.commit)) vim.api.nvim_set_hl(0, "GitlabUnlinked", get_colors_for_group(discussion_colors.unlinked)) vim.api.nvim_set_hl(0, "GitlabDraft", get_colors_for_group(discussion_colors.draft)) vim.api.nvim_set_hl(0, "GitlabDraftMode", get_colors_for_group(discussion_colors.draft_mode)) diff --git a/lua/gitlab/indicators/common.lua b/lua/gitlab/indicators/common.lua index 649b13d1..b68dbff6 100644 --- a/lua/gitlab/indicators/common.lua +++ b/lua/gitlab/indicators/common.lua @@ -9,15 +9,28 @@ local M = {} ---@field resolvable? boolean ---@field resolved? boolean ---@field created_at? string +---@field commit_id? string + +---@param note NoteWithValues +---@return boolean +local function is_skipped_as_resolved(note) + return state.settings.discussion_signs.skip_resolved_discussion and note.resolvable and note.resolved +end ---Return true if discussion has a placeable diagnostic, false otherwise. ---@param note NoteWithValues ---@return boolean local filter_discussions_and_notes = function(note) + -- A note anchored to a single commit has line numbers relative to that commit's isolated + -- diff, not to the MR diff shown here. go-gitlab types commit_id as a plain string, so a + -- note without a commit arrives as "", never nil. + if note.commit_id ~= nil and note.commit_id ~= "" then + return false + end ---Do not include unlinked notes return note.position ~= nil ---Skip resolved discussions if user wants to - and not (state.settings.discussion_signs.skip_resolved_discussion and note.resolvable and note.resolved) + and not is_skipped_as_resolved(note) ---Skip discussions from old revisions and not ( state.settings.discussion_signs.skip_old_revision_discussion @@ -27,9 +40,10 @@ local filter_discussions_and_notes = function(note) ) end ----Filter all discussions and drafts which have placeable signs and diagnostics. +---Apply `predicate` to the first note of each discussion, and to each draft note itself. +---@param predicate fun(note: NoteWithValues): boolean ---@return (Discussion|DraftNote)[] -M.filter_placeable_discussions = function() +local function filter_notes(predicate) local discussions = u.ensure_table(state.DISCUSSION_DATA and state.DISCUSSION_DATA.discussions or {}) if type(discussions) ~= "table" then discussions = {} @@ -42,16 +56,31 @@ M.filter_placeable_discussions = function() local filtered_discussions = List.new(discussions):filter(function(discussion) local first_note = discussion.notes[1] - return type(first_note.position) == "table" and filter_discussions_and_notes(first_note) + return type(first_note.position) == "table" and predicate(first_note) end) - local filtered_draft_notes = List.new(draft_notes):filter(function(note) - return filter_discussions_and_notes(note) - end) + local filtered_draft_notes = List.new(draft_notes):filter(predicate) return u.join(filtered_discussions, filtered_draft_notes) end +---Filter all discussions and drafts which have placeable signs and diagnostics. +---@return (Discussion|DraftNote)[] +M.filter_placeable_discussions = function() + return filter_notes(filter_discussions_and_notes) +end + +---Filter the discussions and drafts anchored to the commit `sha`. +---skip_old_revision_discussion is deliberately not applied: browsing an older commit is +---what the browser is for. +---@param sha string +---@return (Discussion|DraftNote)[] +M.filter_commit_discussions = function(sha) + return filter_notes(function(note) + return note.commit_id == sha and note.position ~= nil and not is_skipped_as_resolved(note) + end) +end + ---Parse old and new line from a line code like "3f454a98e586d1aa0d322e19afd5e67e08f2d3c8_10_44". ---@param line_code string A SHA hash of the file name and line numbers before and after change ---@return integer The line number before the change diff --git a/lua/gitlab/indicators/diagnostics.lua b/lua/gitlab/indicators/diagnostics.lua index d4d6b810..afbda499 100644 --- a/lua/gitlab/indicators/diagnostics.lua +++ b/lua/gitlab/indicators/diagnostics.lua @@ -105,9 +105,14 @@ M.refresh_diagnostics = function() M.clear_diagnostics() M.placeable_discussions = indicators_common.filter_placeable_discussions() - local view = require("gitlab.reviewer").diffview + local reviewer = require("gitlab.reviewer") + local view = reviewer.diffview if view == nil then - u.notify("Could not find Diffview view", vim.log.levels.ERROR) + -- A nil view means either no reviewer was opened (commenting from the commit browser + -- lands here) or an open one lost its view. Only the second is an error. + if reviewer.is_open then + u.notify("Could not find Diffview view", vim.log.levels.ERROR) + end return end M.place_diagnostics(view.cur_layout.a.file.bufnr) @@ -158,6 +163,36 @@ M.place_diagnostics = function(bufnr) end end +---Place the diagnostics for the comments anchored to the commit the browser shows. Diffview +---reuses a revision's buffer across views, so the buffer is written even when the commit +---has no comments, to drop what another commit or the reviewer put there. +---@param bufnr integer Buffer holding the commit's version of the file (the new side) +---@param sha string The commit currently browsed +---@param file_path string Path of the file shown +M.place_commit_diagnostics = function(bufnr, sha, file_path) + if not bufnr or not vim.api.nvim_buf_is_valid(bufnr) then + return + end + if not state.settings.discussion_signs.enabled then + return + end + + -- An old-side line is numbered against the MR base, while the old side shown here is the + -- commit's parent, so only new-side notes can be placed. + local commit_discussions = List.new(indicators_common.filter_commit_discussions(sha)):filter(function(d_or_n) + local note = indicators_common.get_first_note(d_or_n) + return note.position.new_path == file_path and indicators_common.is_new_sha(d_or_n) + end) + + local ok, err = pcall(function() + set_diagnostics(M.diagnostics_namespace, bufnr, M.parse_diagnostics(commit_discussions), create_display_opts()) + end) + + if not ok then + u.notify(string.format("Error setting diagnostics: %s", err), vim.log.levels.ERROR) + end +end + ---Return a list of diagnostics definitions parsed from discussions. ---@param discussions List ---@return vim.Diagnostic.Set[] diff --git a/lua/gitlab/init.lua b/lua/gitlab/init.lua index 4ca0f009..4fd8378b 100644 --- a/lua/gitlab/init.lua +++ b/lua/gitlab/init.lua @@ -4,6 +4,7 @@ local async = require("gitlab.async") local server = require("gitlab.server") local state = require("gitlab.state") local reviewer = require("gitlab.reviewer") +local history = require("gitlab.reviewer.history") local discussions = require("gitlab.actions.discussions") local merge_requests = require("gitlab.actions.merge_requests") local merge = require("gitlab.actions.merge") @@ -41,6 +42,7 @@ local function setup(args) state.set_global_keymaps() require("gitlab.colors") -- Sets colors discussions.initialize_discussions() + history.setup() local is_healthy = health.check(true) if not is_healthy then @@ -81,6 +83,12 @@ return { browse_commits = async.sequence({ info }, function() reviewer.browse_commits() end), + history_create_comment = async.sequence({ info, revisions }, function() + history.create_comment() + end), + history_create_multiline_comment = async.sequence({ info, revisions }, function() + history.create_multiline_comment() + end), pipeline = async.sequence({ latest_pipeline }, pipeline.open), merge = async.sequence({ u.merge(info, { refresh = true }) }, merge.merge), rebase = async.sequence({ u.merge(mergeability, { refresh = true }), info }, rebase.rebase), diff --git a/lua/gitlab/reviewer/history.lua b/lua/gitlab/reviewer/history.lua new file mode 100644 index 00000000..29e340b9 --- /dev/null +++ b/lua/gitlab/reviewer/history.lua @@ -0,0 +1,527 @@ +-- Commenting and line-history navigation while browsing a MR commit-by-commit. +-- +-- browse_commits opens Diffview's FileHistory (base..head), which shows each commit's +-- isolated commit^..commit diff. GitLab's comment endpoint honors a position anchored to +-- any commit in the MR and keeps the note MR-scoped, so a new-side (right window) line is +-- commented directly against the browsed commit: no base..head translation, every line is +-- commentable. The position has to be the commit's own diff refs, base_sha and start_sha +-- on its first parent and head_sha on the commit, plus a top-level commit_id; anything +-- else is rejected with "commit_id does not match the diff refs". +-- +-- GitLab stores base_sha/start_sha as the MR base whatever we send, so an old-side (left +-- window) line, one the commit deletes, ends up numbered against a diff other than the one +-- the browser shows and has no verified anchor; commenting there is refused (see +-- create_comment). +-- +-- Those commit-anchored notes are marked in the browser and nowhere else, since their +-- lines only mean anything in the commit's own diff (see indicators/common.lua). +-- +-- git does the cross-commit tracking (git diff A B, git log -L) for line-history +-- navigation; the pure parsing lives in history_diff.lua and history_log.lua (unit-tested). +-- This module orchestrates git, the live view, and the existing comment path. + +local List = require("gitlab.utils.list") +local u = require("gitlab.utils") +local state = require("gitlab.state") +local reviewer = require("gitlab.reviewer") +local async = require("diffview.async") +local history_diff = require("gitlab.reviewer.history_diff") +local history_log = require("gitlab.reviewer.history_log") + +local M = {} + +---Run a `git diff A B -- ` with zero context and return its output as lines. +---@param a_sha string +---@param b_sha string +---@param new_path string +---@param old_path string +---@return string[]? +local function git_diff_lines(a_sha, b_sha, new_path, old_path) + local out = vim.fn.systemlist({ + "git", + "diff", + "--minimal", + "--unified=0", + "--no-color", + a_sha, + b_sha, + "--", + old_path, + new_path, + }) + if vim.v.shell_error ~= 0 then + return nil + end + return out +end + +---Return the first parent of `sha`, or nil when git cannot resolve it (a root commit). +---@param sha string +---@return string? +local function first_parent(sha) + local out = vim.fn.systemlist({ "git", "rev-parse", "--verify", sha .. "^1" }) + if vim.v.shell_error ~= 0 then + return nil + end + return out[1] +end + +---@class BrowseContext +---@field view table The live FileHistory view +---@field commit_sha string SHA of the commit currently shown +---@field parent_sha string SHA of that commit's first parent, the old side of the shown diff +---@field file string Path of the current file (new version) +---@field old_file string Path of the current file (old version; equals file unless renamed) +---@field new_side boolean True if the cursor is in the new (commit) window, false for old (commit^) +---@field line integer Cursor line number in the focused window + +---Gather everything needed from the live FileHistory view and cursor. Notifies and +---returns nil when the view, commit, file, or side cannot be determined. +---@return BrowseContext? +M.get_context = function() + if reviewer.history_tabid == nil or vim.api.nvim_get_current_tabpage() ~= reviewer.history_tabid then + u.notify("Not in the commit browser", vim.log.levels.ERROR) + return nil + end + + local view = require("diffview.lib").get_current_view() + if view == nil or view.panel == nil or view.panel.cur_item == nil then + u.notify("No commit browser view", vim.log.levels.ERROR) + return nil + end + + local log_entry = view.panel.cur_item[1] + local commit_sha = log_entry and log_entry.commit and log_entry.commit.hash + local cur_file = view.panel.cur_item[2] + if commit_sha == nil or cur_file == nil then + u.notify("Could not read commit or file", vim.log.levels.ERROR) + return nil + end + + local layout = view.cur_layout + if layout == nil or layout.a == nil or layout.b == nil then + u.notify("No diff layout", vim.log.levels.ERROR) + return nil + end + + local current_bufnr = vim.api.nvim_win_get_buf(vim.api.nvim_get_current_win()) + if current_bufnr ~= layout.a.file.bufnr and current_bufnr ~= layout.b.file.bufnr then + u.notify("Put the cursor in a diff window", vim.log.levels.ERROR) + return nil + end + + local parent_sha = first_parent(commit_sha) + if parent_sha == nil then + u.notify("Could not resolve the parent of the browsed commit", vim.log.levels.ERROR) + return nil + end + + return { + view = view, + commit_sha = commit_sha, + parent_sha = parent_sha, + file = cur_file.path, + old_file = cur_file.oldpath or cur_file.path, + new_side = current_bufnr == layout.b.file.bufnr, + line = vim.api.nvim_win_get_cursor(0)[1], + } +end + +---Build a Location-shaped object for the existing comment path, covering +---[start_line, end_line]. M.create_comment and M.create_multiline_comment refuse the old +---side first, so a range always types as "new". +---@param ctx BrowseContext +---@param start_line integer +---@param end_line integer +---@return table +local function build_location(ctx, start_line, end_line) + local location_data + if end_line > start_line then + location_data = { + old_line = nil, + new_line = end_line, + line_range = { + start = { new_line = start_line, type = "new" }, + ["end"] = { new_line = end_line, type = "new" }, + }, + } + else + location_data = { old_line = nil, new_line = end_line, line_range = nil } + end + + return { + location_data = location_data, + reviewer_data = { + file_name = ctx.file, + old_file_name = ctx.old_file ~= ctx.file and ctx.old_file or "", + new_sha_focused = ctx.new_side, + }, + visual_range = { start_line = start_line, end_line = end_line }, + } +end + +---Attach the commit_override anchoring the location to ctx's browsed commit (see module +---header) and hand it to the existing comment path. +---@param ctx BrowseContext +---@param location table +local function submit_comment(ctx, location) + location.commit_override = { + base_sha = ctx.parent_sha, + start_sha = ctx.parent_sha, + head_sha = ctx.commit_sha, + commit_id = ctx.commit_sha, + } + require("gitlab.actions.comment").create_comment_for_location(location) +end + +---Comment on the current line while browsing, anchored to the browsed commit. +---The old (commit^) side has no verified anchor and is refused, see the module header. +M.create_comment = function() + local ctx = M.get_context() + if ctx == nil then + return + end + + if not ctx.new_side then + u.notify("Comments can only be placed from the new side (right window) while browsing commits", vim.log.levels.WARN) + return + end + + submit_comment(ctx, build_location(ctx, ctx.line, ctx.line)) +end + +---Comment on the range covered by the operator motion or visual selection while browsing. +---Same new-side restriction as M.create_comment. +M.create_multiline_comment = function() + if not u.check_visual_mode() then + return + end + + local ctx = M.get_context() + if ctx == nil then + u.press_escape() + return + end + + if not ctx.new_side then + u.press_escape() + u.notify("Comments can only be placed from the new side (right window) while browsing commits", vim.log.levels.WARN) + return + end + + local start_line, end_line = u.get_visual_selection_boundaries() + submit_comment(ctx, build_location(ctx, start_line, end_line)) +end + +---Return the commit SHAs of the browsed range in panel order (newest first). +---@param view table +---@return string[] +local function ordered_shas(view) + local shas = {} + for _, entry in ipairs(view.panel.entries or {}) do + if entry.commit and entry.commit.hash then + table.insert(shas, entry.commit.hash) + end + end + return shas +end + +---Return true if the commit's log entry touches the given file (as new or old path). +---@param entry table A LogEntry +---@param path string +---@return boolean +local function entry_touches_file(entry, path) + return List.new(entry.files or {}):includes(function(f) + return f.path == path or f.oldpath == path + end) +end + +---Follow the current line to an adjacent commit, on the new side only. +---Two tiers: the next commit that changes this exact line (`git log -L`), or, once the +---line's history ends in that direction, the next commit that merely touches the file. +---@param direction 1|-1 +1 steps toward head, -1 toward base +M.goto_version = function(direction) + local ctx = M.get_context() + if ctx == nil then + return + end + if not ctx.new_side then + u.notify("Navigate from the new side (right window)", vim.log.levels.INFO) + return + end + + local diff_refs = state.INFO.diff_refs + if diff_refs == nil then + u.notify("Gitlab did not provide diff refs required to browse this MR", vim.log.levels.ERROR) + return + end + + local diff_to_head = git_diff_lines(ctx.commit_sha, diff_refs.head_sha, ctx.file, ctx.old_file) + if diff_to_head == nil then + u.notify("Could not diff this commit against head", vim.log.levels.ERROR) + return + end + -- The cursor line is typically itself the change being traced, so it maps to the start of + -- the hunk that changes it rather than to a single corresponding head line. + local head_line = history_diff.map_old_to_new(diff_to_head, ctx.line) + local shas = ordered_shas(ctx.view) + -- The panel is newest-first, so a newer commit is a *lower* index: chronological + -- direction maps to the inverse list-index direction used by pick_next_changing. + local list_dir = -direction + + -- Tier 1: the next commit that changes this exact line. A rename earlier in the range + -- truncates what `git log -L` can trace, and we fall through to the file tier below. + local out = vim.fn.systemlist({ + "git", + "log", + "--no-color", + string.format("-L%d,%d:%s", head_line, head_line, ctx.file), + string.format("%s..%s", diff_refs.base_sha, diff_refs.head_sha), + }) + if vim.v.shell_error == 0 then + local line_changes = history_log.parse_log_l(out) + local target = history_log.pick_next_changing(shas, line_changes, ctx.commit_sha, list_dir) + if target ~= nil then + M.select_commit(ctx.view, target.sha, ctx.file, target.new_line) + return + end + end + + -- Tier 2: no more line changes that way, so step to the next commit touching this file. + local file_commits = {} + for _, entry in ipairs(ctx.view.panel.entries or {}) do + if entry.commit and entry.commit.hash and entry_touches_file(entry, ctx.file) then + table.insert(file_commits, { sha = entry.commit.hash }) + end + end + local target = history_log.pick_next_changing(shas, file_commits, ctx.commit_sha, list_dir) + if target == nil then + u.notify(direction > 0 and "No newer commit for this file" or "No older commit for this file", vim.log.levels.INFO) + return + end + + u.notify( + direction > 0 and "No newer line change; moved to next file commit" + or "No older line change; moved to previous file commit", + vim.log.levels.INFO + ) + -- Land on the line's position in the target commit (context there, or near where it + -- will change), by mapping the head-anchored line back into that commit. + local target_diff = git_diff_lines(target.sha, diff_refs.head_sha, ctx.file, ctx.old_file) + local cursor_line = target_diff and history_diff.map_new_to_old(target_diff, head_line) or head_line + M.select_commit(ctx.view, target.sha, ctx.file, cursor_line) +end + +---Show `file_path` at commit `sha` in the FileHistory panel and move the cursor to +---`cursor_line` (clamped to the buffer). cursor_line is the changing hunk's start for a +---line-change jump, or the line's mapped position for a file-only jump; inside a hunk it +---may land a few lines from the exact line, which is fine for navigation. +---@param view table The live FileHistory view +---@param sha string Target commit SHA +---@param file_path string Path of the file to show +---@param cursor_line integer Line to place the cursor on +M.select_commit = function(view, sha, file_path, cursor_line) + local log_entry = List.new(view.panel.entries or {}):find(function(entry) + return entry.commit ~= nil and entry.commit.hash == sha + end) + if log_entry == nil then + u.notify("Commit not found in the browser", vim.log.levels.ERROR) + return + end + + -- Only the file entry matching the file we were on is a valid jump target; a commit + -- that doesn't touch it (e.g. after a rename) must warn rather than silently opening + -- an unrelated file with a line number computed for the wrong file. + local file_entry = List.new(log_entry.files or {}):find(function(f) + return f.path == file_path or f.oldpath == file_path + end) + if file_entry == nil then + u.notify("Commit does not touch this file", vim.log.levels.WARN) + return + end + + async.await(view:set_file(file_entry)) + view.cur_layout.b:focus() + M.refresh_diagnostics() + + local new_win = u.get_window_id_by_buffer_id(view.cur_layout.b.file.bufnr) + if new_win ~= nil then + local line_count = vim.api.nvim_buf_line_count(view.cur_layout.b.file.bufnr) + vim.api.nvim_win_set_cursor(new_win, { math.max(1, math.min(cursor_line, line_count)), 0 }) + end +end + +---Mark the browsed commit's own comments in its diff. The reviewer clears the whole +---diagnostic namespace on every refresh, so this has to run on each entry into the browser, +---not only when the shown commit changes. +M.refresh_diagnostics = function() + if reviewer.history_tabid == nil or vim.api.nvim_get_current_tabpage() ~= reviewer.history_tabid then + return + end + + local view = require("diffview.lib").get_current_view() + local cur_item = view ~= nil and view.panel ~= nil and view.panel.cur_item or nil + if cur_item == nil then + return + end + + local log_entry, file = cur_item[1], cur_item[2] + local sha = log_entry ~= nil and log_entry.commit ~= nil and log_entry.commit.hash or nil + local layout = view.cur_layout + local bufnr = layout ~= nil and layout.b ~= nil and layout.b.file ~= nil and layout.b.file.bufnr or nil + if sha == nil or file == nil or bufnr == nil then + return + end + + require("gitlab.indicators.diagnostics").place_commit_diagnostics(bufnr, sha, file.path) +end + +---Show the commit that a commit-anchored comment was left on, in the commit browser. +---Opens the browser when it is not up yet; Diffview fills its panel from `git log` +---asynchronously, so there is nothing to select from for a moment after that. +---@param sha string The commit the comment is anchored to +---@param file_path string Path of the commented file +---@param line integer Line of the comment, in that commit's version of the file +M.jump_to_commit = function(sha, file_path, line) + reviewer.browse_commits() + if reviewer.history_tabid == nil or vim.api.nvim_get_current_tabpage() ~= reviewer.history_tabid then + u.notify("Could not open the commit browser", vim.log.levels.ERROR) + return + end + + local view = require("diffview.lib").get_current_view() + if view == nil or view.panel == nil then + u.notify("No commit browser view", vim.log.levels.ERROR) + return + end + + -- Diffview's post_open picks an initial file asynchronously, guarded on nothing being + -- selected yet. Waiting for that pick to land keeps ours last; picking first only wins + -- the race when their callback happens to run before we set the cursor. + local loaded = vim.wait(2000, function() + return view.panel:cur_file() ~= nil + end, 50) + if not loaded then + u.notify("The commit browser is still loading, try again", vim.log.levels.WARN) + return + end + + M.select_commit(view, sha, file_path, line) +end + +---Attach the browse-mode keymaps (comment + line navigation) to a diff buffer. +---@param bufnr integer +M.set_keymaps = function(bufnr) + if bufnr == nil or not vim.api.nvim_buf_is_loaded(bufnr) then + return + end + local keymaps = state.settings.keymaps + if keymaps.disable_all or keymaps.reviewer.disable_all then + return + end + + if keymaps.reviewer.create_comment ~= false then + vim.keymap.set("o", keymaps.reviewer.create_comment, function() + -- The "V" in "V%d$" forces linewise motion, see `:h o_V` + vim.api.nvim_cmd({ cmd = "normal", bang = true, args = { string.format("V%d$", vim.v.count1) } }, {}) + end, { + buffer = bufnr, + desc = "Create comment for [count] lines (commit browser)", + nowait = keymaps.reviewer.create_comment_nowait, + }) + + vim.keymap.set("n", keymaps.reviewer.create_comment, function() + reviewer.operator_count = vim.v.count + reviewer.execute_operatorfunc("history_create_multiline_comment") + end, { + buffer = bufnr, + desc = "Create comment for range of motion (commit browser)", + nowait = keymaps.reviewer.create_comment_nowait, + }) + + vim.keymap.set("v", keymaps.reviewer.create_comment, function() + require("gitlab").history_create_multiline_comment() + end, { + buffer = bufnr, + desc = "Create comment for selected text (commit browser)", + nowait = keymaps.reviewer.create_comment_nowait, + }) + end + + if keymaps.reviewer.move_to_discussion_tree ~= false then + vim.keymap.set("n", keymaps.reviewer.move_to_discussion_tree, function() + require("gitlab").move_to_discussion_tree_from_diagnostic() + end, { + buffer = bufnr, + desc = "Move to discussion (commit browser)", + nowait = keymaps.reviewer.move_to_discussion_tree_nowait, + }) + end + + if keymaps.reviewer.history_next_version ~= false then + vim.keymap.set("n", keymaps.reviewer.history_next_version, function() + M.goto_version(1) + end, { buffer = bufnr, desc = "Follow this line to the next newer commit that touches it" }) + end + + if keymaps.reviewer.history_prev_version ~= false then + vim.keymap.set("n", keymaps.reviewer.history_prev_version, function() + M.goto_version(-1) + end, { buffer = bufnr, desc = "Follow this line to the next older commit that touches it" }) + end + + if keymaps.help then + vim.keymap.set("n", keymaps.help, function() + require("gitlab.actions.help").open() + end, { buffer = bufnr, desc = "Open help popup", nowait = keymaps.help_nowait }) + end +end + +---Register the FileHistory hooks, once at plugin setup: attach browse keymaps and the +---commit's comment markers to diff buffers (the browse tab gate keeps these out of the +---regular reviewer), and forget the history tab when its view closes. +--- +---Both buffer events are needed. DiffviewDiffBufRead fires once per buffer, and +---FileHistory caches blob buffers by revision and path, so a commit's new side is the +---same buffer as the next commit's old side and is read only once. DiffviewDiffBufWinEnter +---fires every time a diff buffer is displayed, which covers the reused ones; setting the +---keymaps again on an already mapped buffer just overwrites them. +M.setup = function() + local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.history_keymaps", {}) + vim.api.nvim_create_autocmd("User", { + pattern = { "DiffviewDiffBufRead", "DiffviewDiffBufWinEnter" }, + group = group, + callback = function(args) + if reviewer.history_tabid ~= nil and vim.api.nvim_get_current_tabpage() == reviewer.history_tabid then + M.set_keymaps(args.buf) + M.refresh_diagnostics() + end + end, + }) + + -- The reviewer's right side and the browser's newest commit are the same cached blob + -- buffer, and the reviewer's WinEnter autocmd maps it for the changeset. Switching back + -- here loads no file, so no Diffview buffer event fires to restore the browse keymaps. + vim.api.nvim_create_autocmd("TabEnter", { + group = group, + callback = function() + if reviewer.history_tabid == nil or vim.api.nvim_get_current_tabpage() ~= reviewer.history_tabid then + return + end + local view = require("diffview.lib").get_current_view() + local layout = view ~= nil and view.cur_layout or nil + if layout == nil or layout.a == nil or layout.b == nil then + return + end + M.set_keymaps(layout.a.file and layout.a.file.bufnr) + M.set_keymaps(layout.b.file and layout.b.file.bufnr) + M.refresh_diagnostics() + end, + }) + + require("diffview.config").user_emitter:on("view_closed", function(_, args) + reviewer.clear_history_tab(args.tabpage) + end) +end + +return M diff --git a/lua/gitlab/reviewer/history_diff.lua b/lua/gitlab/reviewer/history_diff.lua new file mode 100644 index 00000000..bbd606dc --- /dev/null +++ b/lua/gitlab/reviewer/history_diff.lua @@ -0,0 +1,61 @@ +-- Pure diff-analysis helpers for commit-by-commit commenting (browse mode). +-- Both functions take the line array of a `git diff A B -- file --unified=0` output. +-- With `--unified=0` a hunk's old-range holds exactly the removed/changed old lines and +-- its new-range exactly the added/changed new lines, so range membership alone decides +-- whether a line took part in a change. + +local hunks = require("gitlab.hunks") + +local M = {} + +---@param diff_lines string[] +---@param cb fun(hunk: Hunk) +local function each_hunk(diff_lines, cb) + for _, line in ipairs(diff_lines) do + local hunk = hunks.parse_possible_hunk_headers(line) + if hunk ~= nil then + cb(hunk) + end + end +end + +---Map an old-side line to its number in the new version, by summing the size change of +---every hunk that lies entirely before it. A line that is itself part of a change has no +---single corresponding new-side line, so it maps to the start of the hunk that changed it +---(a pure-deletion hunk's "start" is its insertion point, since its new range is empty). +---An empty diff maps identically. +---@param diff_lines string[] Output lines of `git diff A B -- file --unified=0` +---@param old_linenr integer +---@return integer +M.map_old_to_new = function(diff_lines, old_linenr) + local offset = 0 + local hunk_start = nil + each_hunk(diff_lines, function(hunk) + -- last_old is the last old line the hunk covers (the insertion point for a pure add). + local last_old = hunk.old_range > 0 and hunk.old_line + hunk.old_range - 1 or hunk.old_line + if hunk.old_range > 0 and old_linenr >= hunk.old_line and old_linenr <= last_old then + hunk_start = hunk.new_line + elseif old_linenr > last_old then + offset = offset + (hunk.new_range - hunk.old_range) + end + end) + return hunk_start or (old_linenr + offset) +end + +---Map an unchanged new-side line back to its number in the old version. Mirror of +---map_old_to_new's offset summing. Only valid for a line that is not itself an added line. +---@param diff_lines string[] Output lines of `git diff A B -- file --unified=0` +---@param new_linenr integer +---@return integer +M.map_new_to_old = function(diff_lines, new_linenr) + local offset = 0 + each_hunk(diff_lines, function(hunk) + local last_new = hunk.new_range > 0 and hunk.new_line + hunk.new_range - 1 or hunk.new_line + if new_linenr > last_new then + offset = offset + (hunk.new_range - hunk.old_range) + end + end) + return new_linenr - offset +end + +return M diff --git a/lua/gitlab/reviewer/history_log.lua b/lua/gitlab/reviewer/history_log.lua new file mode 100644 index 00000000..96ac012b --- /dev/null +++ b/lua/gitlab/reviewer/history_log.lua @@ -0,0 +1,75 @@ +-- Pure parsing of `git log -L ,: base..head` output for line-history +-- navigation in browse mode. `git log -L` traces a single line region through history +-- and emits the commits that changed it, newest first, each followed by its diff hunk. + +local hunks = require("gitlab.hunks") + +local M = {} + +---@class LineHistoryEntry +---@field sha string Full commit SHA +---@field new_line integer Line number of the region in that commit's new version +---@field old_line integer Line number of the region in that commit's parent version + +---Parse `git log -L` output into the ordered list of commits that touch the line. +---Commits without a hunk header (e.g. a trailing/empty block) are skipped. +---@param output_lines string[] +---@return LineHistoryEntry[] +M.parse_log_l = function(output_lines) + local entries = {} + local pending_sha = nil + local pending_open = false + + for _, line in ipairs(output_lines) do + local sha = line:match("^commit%s+(%x+)") + if sha ~= nil then + pending_sha = sha + pending_open = true + elseif pending_open then + local hunk = hunks.parse_possible_hunk_headers(line) + if hunk ~= nil then + table.insert(entries, { sha = pending_sha, new_line = hunk.new_line, old_line = hunk.old_line }) + pending_open = false + end + end + end + + return entries +end + +---Find the next commit that changes the line, relative to `current_sha`, walking the full +---panel timeline (newest first). direction 1 steps toward older commits, -1 toward newer. +---The current commit is skipped whether or not it changes the line. Returns nil at either +---end of the chain or when `current_sha` is not on the timeline. +---@param ordered_shas string[] All commits in the browsed range, newest first +---@param entries LineHistoryEntry[] The subset of commits that change the line +---@param current_sha string +---@param direction 1|-1 +---@return LineHistoryEntry? +M.pick_next_changing = function(ordered_shas, entries, current_sha, direction) + local by_sha = {} + for _, entry in ipairs(entries) do + by_sha[entry.sha] = entry + end + + local current_index = nil + for i, sha in ipairs(ordered_shas) do + if sha == current_sha then + current_index = i + break + end + end + if current_index == nil then + return nil + end + + for i = current_index + direction, direction > 0 and #ordered_shas or 1, direction do + local entry = by_sha[ordered_shas[i]] + if entry ~= nil then + return entry + end + end + return nil +end + +return M diff --git a/lua/gitlab/reviewer/init.lua b/lua/gitlab/reviewer/init.lua index 9f7f022f..66d19d33 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -13,6 +13,7 @@ local M = { is_open = false, bufnr = nil, tabid = nil, + history_tabid = nil, stored_win = nil, buf_winids = {}, } @@ -90,9 +91,9 @@ M.open = function() git.check_mr_in_good_condition() end --- Opens a read-only, commit-by-commit browser for the MR range using Diffview's --- FileHistory. Each entry shows a single commit's isolated diff, for understanding --- how the MR was built up; commenting is not supported here (that stays in M.open). +-- Opens a commit-by-commit browser for the MR range using Diffview's FileHistory. Each +-- entry shows a single commit's isolated diff, for understanding how the MR was built up +-- and commenting against the browsed commit directly (see reviewer/history.lua). M.browse_commits = function() -- Diffview does not deduplicate views: DiffviewFileHistory always opens a new tabpage. -- Focus the existing browser instead of stacking a second, orphaning the first (whose @@ -114,6 +115,17 @@ M.browse_commits = function() end vim.api.nvim_command(string.format("DiffviewFileHistory --range=%s..%s", diff_refs.base_sha, diff_refs.head_sha)) + M.history_tabid = vim.api.nvim_get_current_tabpage() +end + +---Forget the commit-history tab once its Diffview view closes. history_tabid gates the +---browse keymaps and comment path; left pointing at a closed tab it would be a dangling +---handle. Mirrors the tabid cleanup in M.open. +---@param tabpage integer Tabpage of the closed Diffview view +M.clear_history_tab = function(tabpage) + if M.history_tabid == tabpage then + M.history_tabid = nil + end end ---Close the reviewer and clean up. @@ -392,7 +404,7 @@ end ---Set the operatorfunc that will work on the lines defined by the motion that follows ---after the operator mapping, and enter the operator-pending mode. ---@param cb string Name of the gitlab.nvim API function to call, e.g., "create_multiline_comment" -local function execute_operatorfunc(cb) +M.execute_operatorfunc = function(cb) M.old_opfunc = vim.opt.operatorfunc M.old_winnr = vim.api.nvim_get_current_win() M.old_cursor_position = vim.api.nvim_win_get_cursor(M.old_winnr) @@ -435,7 +447,7 @@ M.set_keymaps = function(bufnr) keymaps.reviewer.create_comment, function() M.operator_count = vim.v.count - execute_operatorfunc("create_multiline_comment") + M.execute_operatorfunc("create_multiline_comment") end, { buffer = bufnr, desc = "Create comment for range of motion", nowait = keymaps.reviewer.create_comment_nowait } ) @@ -465,7 +477,7 @@ M.set_keymaps = function(bufnr) vim.keymap.set("n", keymaps.reviewer.create_suggestion, function() M.operator_count = vim.v.count M.operator = keymaps.reviewer.create_suggestion - execute_operatorfunc("create_comment_suggestion") + M.execute_operatorfunc("create_comment_suggestion") end, { buffer = bufnr, desc = "Create suggestion for range of motion", diff --git a/lua/gitlab/state.lua b/lua/gitlab/state.lua index 9c14e7e6..d00bc8aa 100644 --- a/lua/gitlab/state.lua +++ b/lua/gitlab/state.lua @@ -143,6 +143,8 @@ M.settings = { create_comment = "c", create_suggestion = "s", move_to_discussion_tree = "a", + history_next_version = "]v", + history_prev_version = "[v", }, }, popup = { @@ -306,6 +308,7 @@ M.settings = { file_name = "Normal", resolved = "DiagnosticSignOk", unresolved = "DiagnosticSignWarn", + commit = "DiagnosticSignInfo", draft = "DiffviewReference", draft_mode = "DiagnosticWarn", live_mode = "DiagnosticOk", diff --git a/tests/spec/comment_spec.lua b/tests/spec/comment_spec.lua new file mode 100644 index 00000000..fd2bd581 --- /dev/null +++ b/tests/spec/comment_spec.lua @@ -0,0 +1,50 @@ +-- Tests for the positioned-comment payload builder in actions/comment.lua. A browse-mode +-- caller anchors via M.location.commit_override, which has to carry the browsed commit's +-- whole diff refs because Gitlab validates commit_id against them; the regular reviewer +-- path leaves it unset and gets the MR revision with no commit_id in the payload. + +local comment = require("gitlab.actions.comment") +local state = require("gitlab.state") + +describe("actions/comment.lua build_position_data", function() + before_each(function() + state.MR_REVISIONS = { + { + base_commit_sha = "base123", + start_commit_sha = "start123", + head_commit_sha = "head123", + }, + } + end) + + local function make_location(commit_override) + return { + reviewer_data = { file_name = "f.lua", old_file_name = "" }, + location_data = { old_line = nil, new_line = 10, line_range = nil }, + commit_override = commit_override, + } + end + + it("Anchors a browse-mode comment to the browsed commit", function() + comment.location = make_location({ + base_sha = "parentXYZ", + start_sha = "parentXYZ", + head_sha = "commitABC", + commit_id = "commitABC", + }) + local position_data = comment.build_position_data() + assert.are.equal("commitABC", position_data.head_commit_sha) + assert.are.equal("commitABC", position_data.commit_id) + -- Sending the MR base here is what Gitlab rejects with "commit_id does not match the + -- diff refs"; the position has to describe the commit's own parent..commit diff. + assert.are.equal("parentXYZ", position_data.base_commit_sha) + assert.are.equal("parentXYZ", position_data.start_commit_sha) + end) + + it("Leaves a regular reviewer comment's payload unchanged, with no commit_id", function() + comment.location = make_location(nil) + local position_data = comment.build_position_data() + assert.are.equal("head123", position_data.head_commit_sha) + assert.is_nil(position_data.commit_id) + end) +end) diff --git a/tests/spec/commit_comment_jump_spec.lua b/tests/spec/commit_comment_jump_spec.lua new file mode 100644 index 00000000..43cd59d6 --- /dev/null +++ b/tests/spec/commit_comment_jump_spec.lua @@ -0,0 +1,163 @@ +-- Comments anchored to a single commit carry `commit_id` and are positioned in that +-- commit's own diff. They must reach the discussion tree with that anchor, and jumping +-- from the tree must land in the commit browser instead of the MR's changeset. + +local tree_utils = require("gitlab.actions.discussions.tree") +local draft_notes = require("gitlab.actions.draft_notes") +local common = require("gitlab.actions.common") +local reviewer = require("gitlab.reviewer") +local history = require("gitlab.reviewer.history") +local state = require("gitlab.state") + +---A discussion as Gitlab returns it, reduced to what the tree builder reads. +---@param commit_id string +local function discussion_with_commit_id(commit_id) + return { + id = "d1", + individual_note = false, + notes = { + { + id = 1, + author = { username = "gitlab.username" }, + body = "Commented while browsing", + commit_id = commit_id, + created_at = "2026-08-01T10:00:00.000Z", + resolvable = true, + resolved = false, + position = { + new_path = "file.lua", + old_path = "file.lua", + new_line = 11, + base_sha = "base", + start_sha = "base", + head_sha = commit_id, + }, + }, + }, + } +end + +describe("actions/discussions/tree commit anchor", function() + before_each(function() + state.INFO = { web_url = "https://gitlab.example/-/merge_requests/1" } + state.settings.discussion_tree.tree_type = "simple" + end) + after_each(function() + state.INFO = nil + end) + + it("Carries commit_id onto the root node", function() + local nodes = tree_utils.add_discussions_to_table({ discussion_with_commit_id("abc123") }) + assert.are.equal("abc123", nodes[1].commit_id) + end) + + it("Leaves commit_id nil for the empty string Gitlab sends on plain comments", function() + local nodes = tree_utils.add_discussions_to_table({ discussion_with_commit_id("") }) + assert.is_nil(nodes[1].commit_id) + end) +end) + +describe("actions/draft_notes.build_root_draft_note commit anchor", function() + before_each(function() + state.INFO = { web_url = "https://gitlab.example/-/merge_requests/1" } + state.USER = { username = "gitlab.username" } + end) + after_each(function() + state.INFO = nil + state.USER = nil + end) + + ---A draft note as Gitlab returns it, reduced to what the tree builder reads. + ---@param commit_id string + local function draft_note_with_commit_id(commit_id) + return { + id = 1, + note = "Commented while browsing", + commit_id = commit_id, + position = vim.NIL, + discussion_id = "", + } + end + + it("Carries commit_id onto the root node", function() + local node = draft_notes.build_root_draft_note(draft_note_with_commit_id("abc123")) + assert.are.equal("abc123", node.commit_id) + end) + + it("Leaves commit_id nil for the empty string Gitlab sends on plain draft comments", function() + local node = draft_notes.build_root_draft_note(draft_note_with_commit_id("")) + assert.is_nil(node.commit_id) + end) +end) + +describe("actions/common.jump_to_reviewer", function() + local originals = {} + + before_each(function() + originals.get_line_number_from_node = common.get_line_number_from_node + originals.reviewer_jump = reviewer.jump + originals.jump_to_commit = history.jump_to_commit + originals.notify = require("gitlab.utils").notify + end) + + after_each(function() + common.get_line_number_from_node = originals.get_line_number_from_node + reviewer.jump = originals.reviewer_jump + history.jump_to_commit = originals.jump_to_commit + require("gitlab.utils").notify = originals.notify + end) + + ---@param node table The node the cursor is on + ---@param is_new_sha boolean + ---@return table calls, table tree A tree holding `node`, to pass to jump_to_reviewer + local function arrange(node, is_new_sha) + local calls = { reviewer = {}, history = {}, notified = {} } + local tree = { + get_node = function() + return node + end, + } + common.get_line_number_from_node = function() + return 11, is_new_sha + end + reviewer.jump = function(...) + table.insert(calls.reviewer, { ... }) + end + history.jump_to_commit = function(...) + table.insert(calls.history, { ... }) + end + require("gitlab.utils").notify = function(msg) + table.insert(calls.notified, msg) + end + return calls, tree + end + + it("Sends a commit-anchored comment to the commit browser", function() + local calls, tree = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, true) + + common.jump_to_reviewer(tree) + + assert.are.same({}, calls.reviewer) + assert.are.same({ { "abc123", "file.lua", 11 } }, calls.history) + end) + + it("Sends a plain comment to the reviewer", function() + local calls, tree = + arrange({ is_root = true, type = "note", file_name = "file.lua", old_file_name = "file.lua" }, true) + + common.jump_to_reviewer(tree) + + assert.are.same({}, calls.history) + assert.are.equal(1, #calls.reviewer) + end) + + it("Refuses an old-side commit comment rather than jumping to a wrong line", function() + local calls, tree = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, false) + + common.jump_to_reviewer(tree) + + assert.are.same({}, calls.history) + assert.are.same({}, calls.reviewer) + assert.are.equal(1, #calls.notified) + end) +end) diff --git a/tests/spec/discussions_commit_marker_spec.lua b/tests/spec/discussions_commit_marker_spec.lua new file mode 100644 index 00000000..4175f179 --- /dev/null +++ b/tests/spec/discussions_commit_marker_spec.lua @@ -0,0 +1,98 @@ +describe("gitlab/actions/discussions/tree.lua commit marker", function() + local tree = require("gitlab.actions.discussions.tree") + local state = require("gitlab.state") + local utils = require("gitlab.utils") + local original_time_since = utils.time_since + + local author = { username = "gitlab.username" } + + before_each(function() + state.INFO = { web_url = "https://gitlab.com/some-org/-/merge_requests/4963" } + state.USER = author + state.settings.discussion_tree.tree_type = "simple" + utils.time_since = function() + return "5 days ago" + end + end) + + after_each(function() + utils.time_since = original_time_since + state.INFO = nil + state.USER = nil + end) + + ---@param id integer + ---@param body string + ---@param commit_id string + ---@return Note + local function make_note(id, body, commit_id) + return { + author = author, + body = body, + commit_id = commit_id, + created_at = "2023-10-28T18:27:34.082Z", + id = id, + position = vim.NIL, + resolvable = true, + resolved = false, + } + end + + it("Shows the commit marker on the discussion root when commit_id is set", function() + local discussion = { + id = "disc-1", + individual_note = false, + notes = { make_note(1, "root comment", "1a2b3c4d5e6f7890") }, + } + local nodes = tree.add_discussions_to_table({ discussion }) + assert.are.equal("@gitlab.username 5 days ago 1a2b3c4 -", nodes[1].text) + end) + + it("Does not show the commit marker when Gitlab sends an empty commit_id", function() + local discussion = { + id = "disc-2", + individual_note = false, + notes = { make_note(2, "root comment", "") }, + } + local nodes = tree.add_discussions_to_table({ discussion }) + assert.are.equal("@gitlab.username 5 days ago -", nodes[1].text) + end) + + it("Does not repeat the commit marker on replies within the same discussion", function() + local discussion = { + id = "disc-3", + individual_note = false, + notes = { + make_note(3, "root comment", "1a2b3c4d5e6f7890"), + make_note(4, "reply", "1a2b3c4d5e6f7890"), + }, + } + local nodes = tree.add_discussions_to_table({ discussion }) + assert.are.equal("@gitlab.username 5 days ago 1a2b3c4 -", nodes[1].text) + assert.are.equal("@gitlab.username 5 days ago ", nodes[1].__children[2].text) + end) + + it("Shows the commit marker on a draft root when commit_id is set", function() + local draft_notes = require("gitlab.actions.draft_notes") + local node = draft_notes.build_root_draft_note({ + id = 5, + note = "draft comment", + commit_id = "1a2b3c4d5e6f7890", + position = vim.NIL, + discussion_id = "", + }) + assert.are.equal("@gitlab.username ✎ 1a2b3c4 ", node.text) + end) + + it("Does not show the commit marker on a draft root without a commit_id", function() + local draft_notes = require("gitlab.actions.draft_notes") + local node = draft_notes.build_root_draft_note({ + id = 6, + note = "draft comment", + commit_id = "", + position = vim.NIL, + discussion_id = "", + }) + assert.are.equal("@gitlab.username ✎ ", node.text) + end) +end) diff --git a/tests/spec/history_comment_spec.lua b/tests/spec/history_comment_spec.lua new file mode 100644 index 00000000..04c95dbd --- /dev/null +++ b/tests/spec/history_comment_spec.lua @@ -0,0 +1,172 @@ +-- Tests for reviewer/history.lua create_comment: what the new side anchors to, and that +-- the old side refuses. M.get_context and the comment module are stubbed so these run +-- without a live Diffview/FileHistory view. + +local history = require("gitlab.reviewer.history") +local comment = require("gitlab.actions.comment") + +describe("reviewer/history.lua create_comment", function() + local original_get_context = history.get_context + local original_create_comment_for_location = comment.create_comment_for_location + + after_each(function() + history.get_context = original_get_context + comment.create_comment_for_location = original_create_comment_for_location + end) + + it("Anchors a new-side comment to the browsed commit", function() + history.get_context = function() + return { + commit_sha = "commitABC", + parent_sha = "parentXYZ", + file = "f.lua", + old_file = "f.lua", + new_side = true, + line = 42, + } + end + + local captured + comment.create_comment_for_location = function(location) + captured = location + end + + history.create_comment() + + assert.is_not_nil(captured) + assert.are.equal(42, captured.location_data.new_line) + assert.is_nil(captured.location_data.old_line) + assert.are.equal("commitABC", captured.commit_override.head_sha) + assert.are.equal("commitABC", captured.commit_override.commit_id) + -- Gitlab rejects commit_id unless the position is the commit's own diff refs. + assert.are.equal("parentXYZ", captured.commit_override.base_sha) + assert.are.equal("parentXYZ", captured.commit_override.start_sha) + end) + + it("Refuses to comment on the old side and creates no comment", function() + history.get_context = function() + return { + commit_sha = "commitABC", + file = "f.lua", + old_file = "f.lua", + new_side = false, + line = 5, + } + end + + local called = false + comment.create_comment_for_location = function() + called = true + end + + local u = require("gitlab.utils") + local original_notify = u.notify + local notified + u.notify = function(msg, lvl) + notified = { msg = msg, lvl = lvl } + end + + history.create_comment() + + u.notify = original_notify + + assert.is_false(called) + assert.is_not_nil(notified) + assert.are.equal( + "Comments can only be placed from the new side (right window) while browsing commits", + notified.msg + ) + end) +end) + +describe("reviewer/history.lua create_multiline_comment", function() + local original_get_context = history.get_context + local original_create_comment_for_location = comment.create_comment_for_location + + local bufnr + + before_each(function() + bufnr = vim.api.nvim_create_buf(false, true) + vim.api.nvim_buf_set_lines(bufnr, 0, -1, false, { "1", "2", "3", "4", "5" }) + vim.api.nvim_set_current_buf(bufnr) + end) + + after_each(function() + history.get_context = original_get_context + comment.create_comment_for_location = original_create_comment_for_location + -- Leave any visual mode the test left behind before tearing down the buffer. + vim.api.nvim_feedkeys(vim.api.nvim_replace_termcodes("", false, true, true), "nx", false) + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) + + it("Anchors a new-side range comment to the browsed commit", function() + vim.api.nvim_win_set_cursor(0, { 2, 0 }) + vim.cmd("normal! V2j") + + history.get_context = function() + return { + commit_sha = "commitABC", + parent_sha = "parentXYZ", + file = "f.lua", + old_file = "f.lua", + new_side = true, + line = 2, + } + end + + local captured + comment.create_comment_for_location = function(location) + captured = location + end + + history.create_multiline_comment() + + assert.is_not_nil(captured) + assert.are.equal(2, captured.location_data.line_range.start.new_line) + assert.are.equal(4, captured.location_data.line_range["end"].new_line) + assert.are.equal(4, captured.location_data.new_line) + assert.is_nil(captured.location_data.old_line) + assert.are.equal("commitABC", captured.commit_override.head_sha) + assert.are.equal("commitABC", captured.commit_override.commit_id) + assert.are.equal("parentXYZ", captured.commit_override.base_sha) + assert.are.equal("parentXYZ", captured.commit_override.start_sha) + end) + + it("Refuses to comment on the old side for a range and creates no comment", function() + vim.api.nvim_win_set_cursor(0, { 2, 0 }) + vim.cmd("normal! V2j") + + history.get_context = function() + return { + commit_sha = "commitABC", + file = "f.lua", + old_file = "f.lua", + new_side = false, + line = 2, + } + end + + local called = false + comment.create_comment_for_location = function() + called = true + end + + local u = require("gitlab.utils") + local original_notify = u.notify + local notified + u.notify = function(msg, lvl) + notified = { msg = msg, lvl = lvl } + end + + history.create_multiline_comment() + + u.notify = original_notify + + assert.is_false(called) + assert.is_not_nil(notified) + assert.are.equal( + "Comments can only be placed from the new side (right window) while browsing commits", + notified.msg + ) + end) +end) diff --git a/tests/spec/history_diagnostics_spec.lua b/tests/spec/history_diagnostics_spec.lua new file mode 100644 index 00000000..eaeb18a6 --- /dev/null +++ b/tests/spec/history_diagnostics_spec.lua @@ -0,0 +1,220 @@ +-- A commit-anchored comment is positioned in that commit's own diff, so the reviewer does +-- not show it (see indicators/common.lua). The browser draws it on the commit's new side. + +local diagnostics = require("gitlab.indicators.diagnostics") +local common = require("gitlab.indicators.common") +local history = require("gitlab.reviewer.history") +local reviewer = require("gitlab.reviewer") +local diffview_lib = require("diffview.lib") +local signs = require("gitlab.indicators.signs") +local state = require("gitlab.state") + +---@param id string +---@param commit_id string +---@param new_line integer +---@param path string +local function make_discussion(id, commit_id, new_line, path) + return { + id = id, + notes = { + { + id = 1, + author = { username = "author" }, + body = "Commented while browsing", + commit_id = commit_id, + created_at = "2026-08-01T10:00:00.000Z", + resolvable = true, + resolved = false, + position = { new_path = path, old_path = path, new_line = new_line }, + }, + }, + } +end + +---A note on a deleted line: old side, no new_line. +---@param id string +---@param commit_id string +local function make_old_side_discussion(id, commit_id, old_line, path) + local discussion = make_discussion(id, commit_id, nil, path) + discussion.notes[1].position.old_line = old_line + return discussion +end + +---@return integer bufnr +local function make_buffer() + local bufnr = vim.api.nvim_create_buf(false, true) + vim.api.nvim_buf_set_lines(bufnr, 0, -1, false, { "one", "two", "three", "four", "five" }) + return bufnr +end + +---@param bufnr integer +---@return vim.Diagnostic[] +local function placed(bufnr) + return vim.diagnostic.get(bufnr, { namespace = diagnostics.diagnostics_namespace }) +end + +describe("indicators/common.filter_commit_discussions", function() + after_each(function() + state.DISCUSSION_DATA = nil + state.DRAFT_NOTES = nil + end) + + it("Keeps only the notes anchored to the given commit", function() + state.DISCUSSION_DATA = { + discussions = { make_discussion("d1", "sha1", 2, "f.lua"), make_discussion("d2", "sha2", 3, "f.lua") }, + } + state.DRAFT_NOTES = {} + + local result = common.filter_commit_discussions("sha1") + + assert.are.equal(1, #result) + assert.are.equal("d1", result[1].id) + end) + + it("Keeps a draft note anchored to the commit", function() + state.DISCUSSION_DATA = { discussions = {} } + state.DRAFT_NOTES = { { id = 7, commit_id = "sha1", position = { new_path = "f.lua", new_line = 2 } } } + + local result = common.filter_commit_discussions("sha1") + + assert.are.equal(1, #result) + assert.are.equal(7, result[1].id) + end) + + it("Leaves out the notes that belong to the changeset instead of a commit", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "", 2, "f.lua") } } + state.DRAFT_NOTES = {} + + assert.are.equal(0, #common.filter_commit_discussions("sha1")) + end) +end) + +describe("indicators/diagnostics.place_commit_diagnostics", function() + local bufnr + + before_each(function() + bufnr = make_buffer() + state.settings.discussion_signs.enabled = true + signs.setup_signs() + end) + + after_each(function() + vim.diagnostic.reset(diagnostics.diagnostics_namespace) + vim.api.nvim_buf_delete(bufnr, { force = true }) + state.DISCUSSION_DATA = nil + state.DRAFT_NOTES = nil + end) + + it("Marks the commented line of the browsed commit", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "sha1", 3, "f.lua") } } + state.DRAFT_NOTES = {} + + diagnostics.place_commit_diagnostics(bufnr, "sha1", "f.lua") + + local result = placed(bufnr) + assert.are.equal(1, #result) + assert.are.equal(2, result[1].lnum) + assert.are.equal("d1", result[1].user_data.discussion_id) + end) + + it("Leaves out the comments of other commits", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "sha2", 3, "f.lua") } } + state.DRAFT_NOTES = {} + + diagnostics.place_commit_diagnostics(bufnr, "sha1", "f.lua") + + assert.are.equal(0, #placed(bufnr)) + end) + + it("Leaves out the comments on another file of the same commit", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "sha1", 3, "other.lua") } } + state.DRAFT_NOTES = {} + + diagnostics.place_commit_diagnostics(bufnr, "sha1", "f.lua") + + assert.are.equal(0, #placed(bufnr)) + end) + + it("Leaves out a comment on a line the commit deletes", function() + -- Old-side lines are numbered against the MR base, not the parent the browser shows. + state.DISCUSSION_DATA = { discussions = { make_old_side_discussion("d1", "sha1", 3, "f.lua") } } + state.DRAFT_NOTES = {} + + diagnostics.place_commit_diagnostics(bufnr, "sha1", "f.lua") + + assert.are.equal(0, #placed(bufnr)) + end) + + it("Replaces what another commit left on the buffer", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "sha1", 3, "f.lua") } } + state.DRAFT_NOTES = {} + diagnostics.place_commit_diagnostics(bufnr, "sha1", "f.lua") + + diagnostics.place_commit_diagnostics(bufnr, "sha2", "f.lua") + + assert.are.equal(0, #placed(bufnr)) + end) +end) + +describe("reviewer/history.refresh_diagnostics", function() + local bufnr, original_get_current_view, original_history_tabid + + before_each(function() + bufnr = make_buffer() + original_get_current_view = diffview_lib.get_current_view + original_history_tabid = reviewer.history_tabid + state.settings.discussion_signs.enabled = true + signs.setup_signs() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "sha1", 4, "f.lua") } } + state.DRAFT_NOTES = {} + diffview_lib.get_current_view = function() + return { + panel = { cur_item = { { commit = { hash = "sha1" } }, { path = "f.lua" } } }, + cur_layout = { b = { file = { bufnr = bufnr } } }, + } + end + end) + + after_each(function() + diffview_lib.get_current_view = original_get_current_view + reviewer.history_tabid = original_history_tabid + vim.diagnostic.reset(diagnostics.diagnostics_namespace) + vim.api.nvim_buf_delete(bufnr, { force = true }) + state.DISCUSSION_DATA = nil + state.DRAFT_NOTES = nil + end) + + it("Draws the shown commit's comments on the new side", function() + reviewer.history_tabid = vim.api.nvim_get_current_tabpage() + + history.refresh_diagnostics() + + local result = placed(bufnr) + assert.are.equal(1, #result) + assert.are.equal(3, result[1].lnum) + end) + + it("Does nothing outside the commit browser", function() + reviewer.history_tabid = nil + + history.refresh_diagnostics() + + assert.are.equal(0, #placed(bufnr)) + end) +end) + +describe("reviewer/history.set_keymaps", function() + it("Registers the jump to the discussion tree", function() + local bufnr = vim.api.nvim_create_buf(false, true) + + history.set_keymaps(bufnr) + + local lhs = {} + for _, map in ipairs(vim.api.nvim_buf_get_keymap(bufnr, "n")) do + lhs[map.lhs] = true + end + assert.is_true(lhs[state.settings.keymaps.reviewer.move_to_discussion_tree] == true) + + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) +end) diff --git a/tests/spec/history_diff_spec.lua b/tests/spec/history_diff_spec.lua new file mode 100644 index 00000000..f9ed9c95 --- /dev/null +++ b/tests/spec/history_diff_spec.lua @@ -0,0 +1,102 @@ +-- Tests for the diff-analysis helpers, driven by hand-written `git diff --unified=0` +-- output: no git, no diffview, no live state. + +describe("reviewer/history_diff.lua", function() + it("Loads package", function() + local ok, _ = pcall(require, "gitlab.reviewer.history_diff") + assert._is_true(ok) + end) + + local _, bd = pcall(require, "gitlab.reviewer.history_diff") + + describe("map_old_to_new", function() + it("Maps identically across an empty diff", function() + assert.are.equal(10, bd.map_old_to_new({}, 10)) + end) + + it("Shifts old lines after an addition", function() + local diff = { "@@ -5,0 +6,2 @@", "+a", "+b" } + assert.are.equal(4, bd.map_old_to_new(diff, 4)) + assert.are.equal(5, bd.map_old_to_new(diff, 5)) + assert.are.equal(8, bd.map_old_to_new(diff, 6)) + end) + + it("Shifts old lines after a deletion", function() + local diff = { "@@ -20,3 +19,0 @@", "-x", "-y", "-z" } + assert.are.equal(19, bd.map_old_to_new(diff, 19)) + assert.are.equal(20, bd.map_old_to_new(diff, 23)) + end) + + it("Shifts old lines before and after a modification", function() + local diff = { "@@ -10,2 +10,3 @@", "-old1", "-old2", "+new1", "+new2", "+new3" } + assert.are.equal(9, bd.map_old_to_new(diff, 9)) + assert.are.equal(13, bd.map_old_to_new(diff, 12)) + end) + + it("Maps a line inside a modified hunk to the hunk's new-side start", function() + local diff = { "@@ -10,2 +10,3 @@", "-old1", "-old2", "+new1", "+new2", "+new3" } + assert.are.equal(10, bd.map_old_to_new(diff, 10)) + assert.are.equal(10, bd.map_old_to_new(diff, 11)) + end) + + it("Maps a line inside a single-old-line modified hunk (omitted count) to the hunk's new-side start", function() + local diff = { "@@ -23 +23,2 @@", "-old", "+new1", "+new2" } + assert.are.equal(23, bd.map_old_to_new(diff, 23)) + end) + + it("Shifts old lines after a single-old-line modified hunk (omitted count)", function() + local diff = { "@@ -23 +23,2 @@", "-old", "+new1", "+new2" } + assert.are.equal(25, bd.map_old_to_new(diff, 24)) + end) + + it("Does not snap the context line at a pure-insertion hunk's boundary", function() + local diff = { "@@ -5,0 +6,2 @@", "+a", "+b" } + assert.are.equal(5, bd.map_old_to_new(diff, 5)) + assert.are.equal(8, bd.map_old_to_new(diff, 6)) + end) + + it("Maps a line inside a pure-deletion hunk (omitted old count) to the hunk's new-side start", function() + local diff = { "@@ -20 +19,0 @@", "-x" } + assert.are.equal(19, bd.map_old_to_new(diff, 20)) + end) + + it("Shifts old lines after a single-line deletion (omitted count)", function() + local diff = { "@@ -20 +19,0 @@", "-x" } + assert.are.equal(20, bd.map_old_to_new(diff, 21)) + end) + + it("Maps a line inside a multi-line pure-deletion hunk to the hunk's new-side start", function() + local diff = { "@@ -20,3 +19,0 @@", "-x", "-y", "-z" } + assert.are.equal(19, bd.map_old_to_new(diff, 20)) + assert.are.equal(19, bd.map_old_to_new(diff, 22)) + end) + end) + + describe("map_new_to_old", function() + it("Maps identically across an empty diff", function() + assert.are.equal(10, bd.map_new_to_old({}, 10)) + end) + + it("Shifts new lines back across an addition", function() + local diff = { "@@ -5,0 +6,2 @@", "+a", "+b" } + assert.are.equal(5, bd.map_new_to_old(diff, 5)) + assert.are.equal(6, bd.map_new_to_old(diff, 8)) + end) + + it("Shifts new lines back across a deletion", function() + local diff = { "@@ -20,3 +19,0 @@", "-x", "-y", "-z" } + assert.are.equal(19, bd.map_new_to_old(diff, 19)) + assert.are.equal(23, bd.map_new_to_old(diff, 20)) + end) + + it("Shifts new lines back across a single-old-line modified hunk (omitted count)", function() + local diff = { "@@ -23 +23,2 @@", "-old", "+new1", "+new2" } + assert.are.equal(24, bd.map_new_to_old(diff, 25)) + end) + + it("Shifts new lines back across a single-line deletion (omitted count)", function() + local diff = { "@@ -20 +19,0 @@", "-x" } + assert.are.equal(21, bd.map_new_to_old(diff, 20)) + end) + end) +end) diff --git a/tests/spec/history_keymaps_spec.lua b/tests/spec/history_keymaps_spec.lua new file mode 100644 index 00000000..c7c527af --- /dev/null +++ b/tests/spec/history_keymaps_spec.lua @@ -0,0 +1,99 @@ +-- set_keymaps must register the commit-browser keymaps from the default config without +-- erroring (a key name that no longer exists in the defaults would be nil and throw), and +-- the hooks must reach a buffer that Diffview reuses across commits, which fires only +-- DiffviewDiffBufWinEnter and no second DiffviewDiffBufRead. + +local history = require("gitlab.reviewer.history") +local reviewer = require("gitlab.reviewer") +local state = require("gitlab.state") + +describe("reviewer/history.lua set_keymaps", function() + it("Registers the browser keymaps from the default config without error", function() + local bufnr = vim.api.nvim_create_buf(false, true) + + local ok, err = pcall(history.set_keymaps, bufnr) + assert.is_true(ok, err) + + local lhs = {} + for _, map in ipairs(vim.api.nvim_buf_get_keymap(bufnr, "n")) do + lhs[map.lhs] = true + end + + local reviewer_keymaps = state.settings.keymaps.reviewer + assert.is_true(lhs[reviewer_keymaps.create_comment] == true) + assert.is_true(lhs[reviewer_keymaps.history_next_version] == true) + assert.is_true(lhs[reviewer_keymaps.history_prev_version] == true) + assert.is_true(lhs[state.settings.keymaps.help] == true) + + -- create_comment must also act as an operator (o-pending self-motion) and a visual-mode + -- action, the same as in the live reviewer, so `cc` and a visual selection both work. + local o_lhs = {} + for _, map in ipairs(vim.api.nvim_buf_get_keymap(bufnr, "o")) do + o_lhs[map.lhs] = true + end + assert.is_true(o_lhs[reviewer_keymaps.create_comment] == true) + + local v_lhs = {} + for _, map in ipairs(vim.api.nvim_buf_get_keymap(bufnr, "v")) do + v_lhs[map.lhs] = true + end + assert.is_true(v_lhs[reviewer_keymaps.create_comment] == true) + + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) + + it("Attaches the keymaps to a buffer that is only displayed, never read", function() + history.setup() + vim.cmd("tabnew") + reviewer.history_tabid = vim.api.nvim_get_current_tabpage() + local bufnr = vim.api.nvim_create_buf(false, true) + vim.api.nvim_win_set_buf(0, bufnr) + + vim.api.nvim_exec_autocmds("User", { pattern = "DiffviewDiffBufWinEnter" }) + + local lhs = {} + for _, map in ipairs(vim.api.nvim_buf_get_keymap(bufnr, "n")) do + lhs[map.lhs] = true + end + assert.is_true(lhs[state.settings.keymaps.reviewer.create_comment] == true) + + vim.cmd("tabclose") + reviewer.history_tabid = nil + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) + + it("Restores the browser keymaps on a buffer the reviewer tab has remapped", function() + history.setup() + local bufnr = vim.api.nvim_create_buf(false, true) + -- The reviewer and the browser share Diffview's blob buffer for the MR head commit, so + -- the reviewer's own mapping is what sits here after a visit to its tab. + vim.keymap.set("n", state.settings.keymaps.reviewer.create_comment, function() end, { + buffer = bufnr, + desc = "Reviewer comment", + }) + + local diffview_lib = package.loaded["diffview.lib"] + package.loaded["diffview.lib"] = { + get_current_view = function() + return { cur_layout = { a = { file = { bufnr = bufnr } }, b = { file = { bufnr = bufnr } } } } + end, + } + vim.cmd("tabnew") + reviewer.history_tabid = vim.api.nvim_get_current_tabpage() + + vim.api.nvim_exec_autocmds("TabEnter", {}) + + local desc + for _, map in ipairs(vim.api.nvim_buf_get_keymap(bufnr, "n")) do + if map.lhs == state.settings.keymaps.reviewer.create_comment then + desc = map.desc + end + end + assert.are.equal("Create comment for range of motion (commit browser)", desc) + + package.loaded["diffview.lib"] = diffview_lib + vim.cmd("tabclose") + reviewer.history_tabid = nil + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) +end) diff --git a/tests/spec/history_log_spec.lua b/tests/spec/history_log_spec.lua new file mode 100644 index 00000000..00d4ef39 --- /dev/null +++ b/tests/spec/history_log_spec.lua @@ -0,0 +1,96 @@ +-- Tests for the line-history parsing, driven by hand-written `git log -L` output: +-- no git, no diffview, no live state. + +describe("reviewer/history_log.lua", function() + it("Loads package", function() + local ok, _ = pcall(require, "gitlab.reviewer.history_log") + assert._is_true(ok) + end) + + local _, bl = pcall(require, "gitlab.reviewer.history_log") + + describe("parse_log_l", function() + it("Extracts ordered commits with their new-side line number", function() + local out = { + "commit aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "Author: A ", + "Date: Mon Jan 1 00:00:00 2024 +0000", + "", + " change two", + "", + "diff --git a/f.lua b/f.lua", + "--- a/f.lua", + "+++ b/f.lua", + "@@ -10,3 +10,4 @@ context", + " keep", + "-old", + "+new1", + "+new2", + "commit bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "Author: B ", + "Date: Sun Dec 31 00:00:00 2023 +0000", + "", + " change one", + "", + "diff --git a/f.lua b/f.lua", + "--- a/f.lua", + "+++ b/f.lua", + "@@ -8,2 +8,3 @@ context", + " keep", + "+added", + } + local got = bl.parse_log_l(out) + assert.are.same({ + { sha = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", new_line = 10, old_line = 10 }, + { sha = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", new_line = 8, old_line = 8 }, + }, got) + end) + + it("Returns an empty list when the line has no history", function() + assert.are.same({}, bl.parse_log_l({})) + end) + + it("Skips a commit that has no hunk header", function() + local out = { + "commit cccccccccccccccccccccccccccccccccccccccc", + "Author: C ", + "Date: Mon Jan 1 00:00:00 2024 +0000", + "", + " no diff body", + "", + } + assert.are.same({}, bl.parse_log_l(out)) + end) + end) + + describe("pick_next_changing", function() + -- Full panel timeline, newest first. Only f and d actually change the line. + local ordered = { "h", "g", "f", "e", "d" } + local entries = { + { sha = "f", new_line = 8 }, + { sha = "d", new_line = 5 }, + } + + it("Finds the next changing commit toward older history (direction 1)", function() + assert.are.same({ sha = "f", new_line = 8 }, bl.pick_next_changing(ordered, entries, "h", 1)) + end) + + it("Skips over the current commit even when it changes the line", function() + assert.are.same({ sha = "d", new_line = 5 }, bl.pick_next_changing(ordered, entries, "f", 1)) + end) + + it("Finds the next changing commit toward newer history (direction -1)", function() + -- From e (between f and d), the first newer changing commit is f. + assert.are.same({ sha = "f", new_line = 8 }, bl.pick_next_changing(ordered, entries, "e", -1)) + end) + + it("Returns nil at the end of the chain", function() + assert.is_nil(bl.pick_next_changing(ordered, entries, "d", 1)) + assert.is_nil(bl.pick_next_changing(ordered, entries, "f", -1)) + end) + + it("Returns nil when the current sha is not on the timeline", function() + assert.is_nil(bl.pick_next_changing(ordered, entries, "zzzz", 1)) + end) + end) +end) diff --git a/tests/spec/history_select_commit_spec.lua b/tests/spec/history_select_commit_spec.lua new file mode 100644 index 00000000..66cb9b33 --- /dev/null +++ b/tests/spec/history_select_commit_spec.lua @@ -0,0 +1,116 @@ +-- Tests for reviewer/history.lua select_commit: jumping to a target commit must not +-- silently fall back to an unrelated file when the target commit's file list has no +-- match for the file being followed (e.g. after a rename earlier in the history), and must +-- focus the new-side window so the cursor jump doesn't leave the caller's window focused. + +local history = require("gitlab.reviewer.history") + +describe("reviewer/history.lua select_commit", function() + it("Does not jump and warns when the target commit does not touch the followed file", function() + local view = { + panel = { + entries = { + { commit = { hash = "sha1" }, files = { { path = "unrelated.lua" } } }, + }, + }, + set_file = function() + error("select_commit must not open a file when there is no match") + end, + } + + local u = require("gitlab.utils") + local original_notify = u.notify + local notified + u.notify = function(msg, lvl) + notified = { msg = msg, lvl = lvl } + end + + history.select_commit(view, "sha1", "original.lua", 10) + + u.notify = original_notify + + assert.is_not_nil(notified) + assert.are.equal("Commit does not touch this file", notified.msg) + end) + + it("Focuses the new-side window, so the cursor jump actually lands there", function() + local focused = false + local bufnr = vim.api.nvim_create_buf(false, true) + + local view = { + panel = { + entries = { + { commit = { hash = "sha1" }, files = { { path = "f.lua" } } }, + }, + }, + set_file = function() + return { await = function() end } + end, + cur_layout = { + b = { + focus = function() + focused = true + end, + file = { bufnr = bufnr }, + }, + }, + } + + history.select_commit(view, "sha1", "f.lua", 3) + + assert.is_true(focused) + + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) +end) + +describe("reviewer/history.lua jump_to_commit", function() + it("Waits for Diffview's own initial file selection before picking a target commit", function() + local reviewer = require("gitlab.reviewer") + local diffview_lib = require("diffview.lib") + + local original_browse_commits = reviewer.browse_commits + local original_get_current_view = diffview_lib.get_current_view + local original_select_commit = history.select_commit + local original_history_tabid = reviewer.history_tabid + + local tabid = vim.api.nvim_get_current_tabpage() + reviewer.history_tabid = tabid + reviewer.browse_commits = function() end + + -- Diffview's panel already has entries from the start; only `cur_file()` lags, + -- mirroring the initial-selection race M.jump_to_commit waits out. + local view = { + panel = { + entries = { { commit = { hash = "sha1" }, files = { { path = "f.lua" } } } }, + cur_item = nil, + }, + } + function view.panel:cur_file() + return self.cur_item + end + + diffview_lib.get_current_view = function() + return view + end + + local cur_item_was_set_before_select + history.select_commit = function() + cur_item_was_set_before_select = view.panel.cur_item ~= nil + end + + -- Simulate Diffview's own initial selection landing shortly after the wait starts. + vim.defer_fn(function() + view.panel.cur_item = {} + end, 100) + + history.jump_to_commit("sha1", "f.lua", 3) + + reviewer.browse_commits = original_browse_commits + diffview_lib.get_current_view = original_get_current_view + history.select_commit = original_select_commit + reviewer.history_tabid = original_history_tabid + + assert.is_true(cur_item_was_set_before_select) + end) +end) diff --git a/tests/spec/history_tab_spec.lua b/tests/spec/history_tab_spec.lua new file mode 100644 index 00000000..1cc1a0c9 --- /dev/null +++ b/tests/spec/history_tab_spec.lua @@ -0,0 +1,18 @@ +-- Verifies the commit-history tab id is forgotten when its Diffview view closes, so the +-- browse gate never keeps pointing at a dead tabpage. + +describe("reviewer.clear_history_tab", function() + local reviewer = require("gitlab.reviewer") + + it("Clears history_tabid when the closed view's tab matches", function() + reviewer.history_tabid = 42 + reviewer.clear_history_tab(42) + assert.is_nil(reviewer.history_tabid) + end) + + it("Keeps history_tabid when a different tab closes", function() + reviewer.history_tabid = 42 + reviewer.clear_history_tab(7) + assert.are.equal(42, reviewer.history_tabid) + end) +end) diff --git a/tests/spec/indicators_common_filter_spec.lua b/tests/spec/indicators_common_filter_spec.lua new file mode 100644 index 00000000..8d3d8d3c --- /dev/null +++ b/tests/spec/indicators_common_filter_spec.lua @@ -0,0 +1,73 @@ +-- A discussion or draft note anchored to a single commit has position line numbers +-- relative to that commit's isolated parent..commit diff, not the MR diff the regular +-- reviewer shows, so it must not be placed there. go-gitlab types commit_id as a plain +-- string, so a note without a commit arrives as "", never nil. + +local common = require("gitlab.indicators.common") +local state = require("gitlab.state") + +describe("indicators/common.filter_placeable_discussions", function() + local original_discussion_data = state.DISCUSSION_DATA + local original_draft_notes = state.DRAFT_NOTES + + after_each(function() + state.DISCUSSION_DATA = original_discussion_data + state.DRAFT_NOTES = original_draft_notes + end) + + local function make_discussion(id, commit_id) + return { + id = id, + notes = { { position = { new_line = 1 }, commit_id = commit_id } }, + } + end + + local function make_draft_note(id, commit_id) + return { id = id, position = { new_line = 1 }, commit_id = commit_id } + end + + it("Filters out a discussion anchored to a commit (non-empty commit_id)", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "abc123") } } + state.DRAFT_NOTES = {} + + local result = common.filter_placeable_discussions() + + assert.are.equal(0, #result) + end) + + it("Keeps a discussion with commit_id as an empty string", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", "") } } + state.DRAFT_NOTES = {} + + local result = common.filter_placeable_discussions() + + assert.are.equal(1, #result) + end) + + it("Keeps a discussion with commit_id as nil", function() + state.DISCUSSION_DATA = { discussions = { make_discussion("d1", nil) } } + state.DRAFT_NOTES = {} + + local result = common.filter_placeable_discussions() + + assert.are.equal(1, #result) + end) + + it("Filters out a draft note anchored to a commit (non-empty commit_id)", function() + state.DISCUSSION_DATA = { discussions = {} } + state.DRAFT_NOTES = { make_draft_note("dn1", "abc123") } + + local result = common.filter_placeable_discussions() + + assert.are.equal(0, #result) + end) + + it("Keeps a draft note with commit_id as an empty string", function() + state.DISCUSSION_DATA = { discussions = {} } + state.DRAFT_NOTES = { make_draft_note("dn1", "") } + + local result = common.filter_placeable_discussions() + + assert.are.equal(1, #result) + end) +end) From ede468903892de938205549e8a0f73deb2abebfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:36:58 +0200 Subject: [PATCH 6/9] feat: add a per-tab discussion window registry --- lua/gitlab/actions/discussions/windows.lua | 89 ++++++++++++++++++++++ tests/spec/discussions_windows_spec.lua | 89 ++++++++++++++++++++++ 2 files changed, 178 insertions(+) create mode 100644 lua/gitlab/actions/discussions/windows.lua create mode 100644 tests/spec/discussions_windows_spec.lua diff --git a/lua/gitlab/actions/discussions/windows.lua b/lua/gitlab/actions/discussions/windows.lua new file mode 100644 index 00000000..9f640bcc --- /dev/null +++ b/lua/gitlab/actions/discussions/windows.lua @@ -0,0 +1,89 @@ +-- Registry of the discussion window per tabpage. Several tabs (e.g. the MR diff and the +-- commit browser) can each show their own discussion tree, and the window in a given tab +-- may hold different buffers over time (linked/unlinked discussions, notes), so the split +-- state can't live in a single module-level handle. + +local M = {} + +---@class DiscussionWindowEntry +---@field split NuiSplit +---@field winid integer +---@field bufnr integer The buffer currently shown in `split` +---@field view_type "discussions"|"notes" +---@field last_row integer? +---@field last_column integer? +---@field last_node_at_cursor NuiTree.Node? + +---@type table +local entries = {} + +---@param tabid integer +---@param entry DiscussionWindowEntry? +---@return boolean +local function is_valid(tabid, entry) + return entry ~= nil + and vim.api.nvim_tabpage_is_valid(tabid) + and vim.api.nvim_win_is_valid(entry.winid) + -- A live window is not necessarily still this tab's window: `T` moves it into a + -- new tabpage, after which the entry would send `tabid` to a window somewhere else. + and vim.api.nvim_win_get_tabpage(entry.winid) == tabid +end + +---@param tabid integer +---@param entry DiscussionWindowEntry +M.set = function(tabid, entry) + entries[tabid] = entry +end + +---Get the entry for `tabid` (default: the current tabpage), or nil if there is none or it +---no longer points at a live tab/window. +---@param tabid integer? +---@return DiscussionWindowEntry? +M.get = function(tabid) + tabid = tabid or vim.api.nvim_get_current_tabpage() + local entry = entries[tabid] + if not is_valid(tabid, entry) then + entries[tabid] = nil + return nil + end + return entry +end + +---@param tabid integer +M.remove = function(tabid) + entries[tabid] = nil +end + +---@param winid integer +M.remove_by_winid = function(winid) + for tabid, entry in pairs(entries) do + if entry.winid == winid then + entries[tabid] = nil + return + end + end +end + +---Call `fn(entry, tabid)` for every entry whose tab and window are still live, pruning the +---rest. +---@param fn fun(entry: DiscussionWindowEntry, tabid: integer) +M.each = function(fn) + for tabid, entry in pairs(entries) do + if is_valid(tabid, entry) then + fn(entry, tabid) + else + entries[tabid] = nil + end + end +end + +---@return boolean +M.any = function() + local found = false + M.each(function() + found = true + end) + return found +end + +return M diff --git a/tests/spec/discussions_windows_spec.lua b/tests/spec/discussions_windows_spec.lua new file mode 100644 index 00000000..c9633ac5 --- /dev/null +++ b/tests/spec/discussions_windows_spec.lua @@ -0,0 +1,89 @@ +-- Verifies the tabpage-keyed registry: entries stay independent across tabs, and get pruned +-- once their window or tabpage is no longer live. + +local windows = require("gitlab.actions.discussions.windows") + +describe("actions/discussions/windows", function() + after_each(function() + vim.cmd("silent! tabonly") + end) + + it("Returns the entry set for the current tabpage", function() + local winid = vim.api.nvim_get_current_win() + local tabid = vim.api.nvim_get_current_tabpage() + local entry = { winid = winid, bufnr = vim.api.nvim_get_current_buf(), view_type = "discussions" } + + windows.set(tabid, entry) + + assert.are.equal(entry, windows.get()) + end) + + it("Keeps two tabpages' entries independent", function() + local first_tabid = vim.api.nvim_get_current_tabpage() + local first_entry = { winid = vim.api.nvim_get_current_win(), bufnr = vim.api.nvim_get_current_buf() } + windows.set(first_tabid, first_entry) + + vim.cmd("tabnew") + local second_tabid = vim.api.nvim_get_current_tabpage() + local second_entry = { winid = vim.api.nvim_get_current_win(), bufnr = vim.api.nvim_get_current_buf() } + windows.set(second_tabid, second_entry) + + assert.are.equal(second_entry, windows.get()) + vim.api.nvim_set_current_tabpage(first_tabid) + assert.are.equal(first_entry, windows.get()) + end) + + it("Prunes an entry whose window closed, on the next each(), and any() goes false", function() + vim.cmd("split") + local winid = vim.api.nvim_get_current_win() + local tabid = vim.api.nvim_get_current_tabpage() + windows.set(tabid, { winid = winid, bufnr = vim.api.nvim_get_current_buf() }) + assert.is_true(windows.any()) + + vim.api.nvim_win_close(winid, true) + + local seen = {} + windows.each(function(_, id) + table.insert(seen, id) + end) + assert.are.same({}, seen) + assert.is_false(windows.any()) + end) + + it("get() returns nil once the entry's window was moved to another tabpage", function() + vim.cmd("tabnew") + vim.cmd("split") + local tabid = vim.api.nvim_get_current_tabpage() + windows.set(tabid, { winid = vim.api.nvim_get_current_win(), bufnr = vim.api.nvim_get_current_buf() }) + + vim.cmd("wincmd T") + + assert.is_nil(windows.get(tabid)) + end) + + it("remove_by_winid removes the entry owning that window and leaves others", function() + local first_tabid = vim.api.nvim_get_current_tabpage() + local first_winid = vim.api.nvim_get_current_win() + windows.set(first_tabid, { winid = first_winid, bufnr = vim.api.nvim_get_current_buf() }) + + vim.cmd("tabnew") + local second_tabid = vim.api.nvim_get_current_tabpage() + local second_winid = vim.api.nvim_get_current_win() + windows.set(second_tabid, { winid = second_winid, bufnr = vim.api.nvim_get_current_buf() }) + + windows.remove_by_winid(first_winid) + + assert.is_nil(windows.get(first_tabid)) + assert.is_not_nil(windows.get(second_tabid)) + end) + + it("get() returns nil once the entry's tabpage itself is closed", function() + vim.cmd("tabnew") + local tabid = vim.api.nvim_get_current_tabpage() + windows.set(tabid, { winid = vim.api.nvim_get_current_win(), bufnr = vim.api.nvim_get_current_buf() }) + + vim.cmd("tabclose") + + assert.is_nil(windows.get(tabid)) + end) +end) From 3f75b99b8df9378e28beb77a32191b611500427d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:37:07 +0200 Subject: [PATCH 7/9] feat: open the discussion tree in several tabs Buffers and trees are shared, so the tabs show the same content; only the window, its view type and its cursor position are per tab. Closing the reviewer takes down every discussion window, since one can now sit in a tab the reviewer does not own. --- .github/workflows/lua.yaml | 1 + lua/gitlab/actions/common.lua | 43 ++- lua/gitlab/actions/discussions/init.lua | 312 +++++++++++++----- lua/gitlab/actions/discussions/tree.lua | 19 +- lua/gitlab/actions/discussions/winbar.lua | 31 +- lua/gitlab/actions/draft_notes/init.lua | 2 +- lua/gitlab/actions/merge.lua | 2 +- lua/gitlab/actions/merge_requests.lua | 4 +- lua/gitlab/emoji.lua | 2 +- lua/gitlab/init.lua | 5 +- lua/gitlab/reviewer/init.lua | 75 +++-- tests/spec/commit_comment_jump_spec.lua | 26 +- tests/spec/discussions_current_node_spec.lua | 121 +++++++ tests/spec/discussions_orphan_window_spec.lua | 56 ++-- tests/spec/discussions_shared_bufs_spec.lua | 37 ++- tests/spec/discussions_spec.lua | 215 ++++++++++++ tests/spec/reviewer_close_session_spec.lua | 78 +++++ tests/spec/reviewer_close_spec.lua | 67 ++++ tests/spec/reviewer_diffview_closed_spec.lua | 40 +++ .../spec/reviewer_reset_discussions_spec.lua | 39 +++ 20 files changed, 978 insertions(+), 197 deletions(-) create mode 100644 tests/spec/discussions_current_node_spec.lua create mode 100644 tests/spec/reviewer_close_session_spec.lua create mode 100644 tests/spec/reviewer_close_spec.lua create mode 100644 tests/spec/reviewer_diffview_closed_spec.lua create mode 100644 tests/spec/reviewer_reset_discussions_spec.lua diff --git a/.github/workflows/lua.yaml b/.github/workflows/lua.yaml index 9ecfa484..0469a81a 100644 --- a/.github/workflows/lua.yaml +++ b/.github/workflows/lua.yaml @@ -6,6 +6,7 @@ on: - develop paths: - 'lua/**' # Ignore changes to the Go code + - 'tests/**' jobs: lua_lint: name: Lint Lua 💅 diff --git a/lua/gitlab/actions/common.lua b/lua/gitlab/actions/common.lua index 567b9c1a..25373d68 100644 --- a/lua/gitlab/actions/common.lua +++ b/lua/gitlab/actions/common.lua @@ -7,8 +7,45 @@ local u = require("gitlab.utils") local reviewer = require("gitlab.reviewer") local indicators_common = require("gitlab.indicators.common") local state = require("gitlab.state") +local windows = require("gitlab.actions.discussions.windows") local M = {} +---Return the window showing the tree in the current tabpage. +---@param tree NuiTree +---@return integer? +local function get_tree_winid(tree) + local current_winid = vim.api.nvim_get_current_win() + if vim.api.nvim_win_get_buf(current_winid) == tree.bufnr then + return current_winid + end + local entry = windows.get() + if entry ~= nil and entry.bufnr == tree.bufnr then + return entry.winid + end +end + +---Return the node under `winid`'s cursor, plus that cursor's column. +---`tree:get_node()` resolves the window as `win_findbuf(tree.bufnr)[1]`, which with the +---discussion buffers shared between tabpages is an arbitrary tab's window. +---@param tree NuiTree +---@param winid integer +---@return NuiTree.Node?, integer column +M.get_node_at = function(tree, winid) + local row, column = unpack(vim.api.nvim_win_get_cursor(winid)) + return tree:get_node(row), column +end + +---Return the node under the cursor of the current tabpage's tree window. +---@param tree NuiTree +---@return NuiTree.Node? +M.get_current_node = function(tree) + local winid = get_tree_winid(tree) + if winid == nil then + return nil + end + return (M.get_node_at(tree, winid)) +end + ---Build note header from note. ---@param note Note|DraftNote ---@return string @@ -99,7 +136,7 @@ end ---@param tree NuiTree ---@return string? M.get_url = function(tree) - local current_node = tree:get_node() + local current_node = M.get_current_node(tree) local note_node = M.get_note_node(tree, current_node) if note_node == nil then return @@ -134,7 +171,7 @@ end ---For developers! ---@param tree NuiTree M.print_node = function(tree) - local current_node = tree:get_node() + local current_node = M.get_current_node(tree) vim.print(current_node) end @@ -296,7 +333,7 @@ end ---Move the cursor to the reviewer's location associated with the note. ---@param tree NuiTree M.jump_to_reviewer = function(tree) - local node = tree:get_node() + local node = M.get_current_node(tree) local root_node = M.get_root_node(tree, node) if root_node == nil then u.notify("Could not get discussion node", vim.log.levels.ERROR) diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 0e95610b..f902ad70 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -21,10 +21,9 @@ local diagnostics = require("gitlab.indicators.diagnostics") local winbar = require("gitlab.actions.discussions.winbar") local help = require("gitlab.actions.help") local emoji = require("gitlab.emoji") +local windows = require("gitlab.actions.discussions.windows") local M = { - split_visible = false, - split = nil, ---@type number linked_bufnr = nil, ---@type number @@ -35,7 +34,8 @@ local M = { unlinked_discussion_tree = nil, } ----Delete discussion buffers to prevent two leaked buffers on each M.open/M.close cycle. +---Delete the discussion buffers that all windows share, to prevent two leaked buffers on +---each open/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 }) @@ -45,6 +45,19 @@ local function delete_bufs() end end +---Find the registry entry owning `winid`, across all tabpages. +---@param winid integer +---@return DiscussionWindowEntry? +local function entry_for_winid(winid) + local found + windows.each(function(entry) + if entry.winid == winid then + found = entry + end + end) + return found +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 @@ -123,66 +136,99 @@ end ---Open the discussion and unlinked note trees and set the keybindings. ---@param callback? function ----@param view_type "discussions"|"notes" Defines the view type to select (useful for overriding the default view type when jumping to discussion tree when it's closed) +---@param view_type? "discussions"|"notes" Defines the view type to select (useful for overriding the default view type when jumping to discussion tree when it's closed) M.open = function(callback, view_type) local original_window = vim.api.nvim_get_current_win() -- The window from which ther user called M.open + local tabid = vim.api.nvim_get_current_tabpage() - M.current_view_type = view_type and view_type or state.settings.discussion_tree.default_view + local requested_view_type = view_type and view_type or state.settings.discussion_tree.default_view state.DISCUSSION_DATA = u.ensure_table(state.DISCUSSION_DATA) state.DISCUSSION_DATA.discussions = u.ensure_table(state.DISCUSSION_DATA.discussions) state.DISCUSSION_DATA.unlinked_discussions = u.ensure_table(state.DISCUSSION_DATA.unlinked_discussions) state.DRAFT_NOTES = u.ensure_table(state.DRAFT_NOTES) - -- Make discussion split window and buffers, store buffer numbers - local split, linked_bufnr, unlinked_bufnr = M.create_split_and_bufs() - M.split = split - M.linked_bufnr = linked_bufnr - M.unlinked_bufnr = unlinked_bufnr - M.split_visible = true + -- The current tabpage already has a discussion window; focus it instead of mounting a + -- second one. + local existing = windows.get(tabid) + if existing then + vim.api.nvim_set_current_win(existing.winid) + if type(callback) == "function" then + callback() + end + return + end + + local is_first_window = not windows.any() + + -- Make discussion split window, creating the shared buffers only for the first window + -- (later tabs reuse them, so they show the same tree). + local split = M.create_split() + if is_first_window then + M.linked_bufnr, M.unlinked_bufnr = M.create_bufs() + end split:mount() + windows.set(tabid, { split = split, winid = split.winid, bufnr = M.linked_bufnr, view_type = requested_view_type }) + -- Set window and buffer local options to discussion tree split after mounting the split for opt, val in pairs(state.settings.discussion_tree.winopts) do - vim.api.nvim_set_option_value(opt, val, { win = M.split.winid }) + vim.api.nvim_set_option_value(opt, val, { win = split.winid }) end - vim.api.nvim_set_option_value("filetype", "gitlab", { buf = M.linked_bufnr }) - vim.api.nvim_set_option_value("filetype", "gitlab", { buf = M.unlinked_bufnr }) + if is_first_window then + vim.api.nvim_set_option_value("filetype", "gitlab", { buf = M.linked_bufnr }) + vim.api.nvim_set_option_value("filetype", "gitlab", { buf = M.unlinked_bufnr }) - -- Set autocmds to clean up state when discussions buffers are deleted manually - vim.api.nvim_create_autocmd("BufWipeout", { - buffer = M.linked_bufnr, - callback = function() - M.linked_bufnr = nil - end, - }) - vim.api.nvim_create_autocmd("BufWipeout", { - buffer = M.unlinked_bufnr, - callback = function() - M.unlinked_bufnr = nil - end, - }) + -- Set autocmds to clean up state when discussions buffers are deleted manually + vim.api.nvim_create_autocmd("BufWipeout", { + buffer = M.linked_bufnr, + callback = function() + M.linked_bufnr = nil + end, + }) + vim.api.nvim_create_autocmd("BufWipeout", { + buffer = M.unlinked_bufnr, + callback = function() + M.unlinked_bufnr = nil + end, + }) + end - -- Set autocmd to clean up state when discussions split is closed manually + -- Set autocmd to clean up state when this tab's discussion split is closed manually vim.api.nvim_create_autocmd("WinClosed", { - pattern = tostring(M.split.winid), - -- M.close wipes the discussion buffers, and a buffer wiped from inside this callback - -- fires no BufWipeout, so the autocmds above would never reset the bufnr fields. + pattern = tostring(split.winid), callback = function() - vim.schedule(M.close) + windows.remove_by_winid(split.winid) + if not windows.any() then + winbar.cleanup_timer() + end + -- Unmount, or the split keeps its buffer and augroups for the rest of the session, + -- one set per window the user closes by hand. Defer it: delete_bufs wipes the + -- discussion buffers, and a buffer wiped from inside this callback fires no + -- BufWipeout, so the autocmds above would never reset the bufnr fields. + vim.schedule(function() + pcall(function() + split:unmount() + end) + if not windows.any() then + delete_bufs() + end + end) end, }) -- Initialize winbar - winbar.start_timer() + if is_first_window then + winbar.start_timer() + end -- Rebuild trees in order to set keymaps and make buffers protected - M.switch_view_type(M.current_view_type) + M.switch_view_type(requested_view_type) M.rebuild_unlinked_discussion_tree() M.rebuild_discussion_tree() -- Focus the correct window - local win_to_enter = not state.settings.discussion_tree.focus_on_open and original_window or M.split.winid + local win_to_enter = not state.settings.discussion_tree.focus_on_open and original_window or split.winid if vim.api.nvim_win_is_valid(win_to_enter) then vim.api.nvim_set_current_win(win_to_enter) end @@ -195,19 +241,20 @@ M.open = function(callback, view_type) end end ----Clear the discussion state and unmount the split. -M.close = function() - if M.split == nil then +---Unmount the discussion split of `tabid` (default: the current tabpage). +---@param tabid integer? +M.close = function(tabid) + tabid = tabid or vim.api.nvim_get_current_tabpage() + local entry = windows.get(tabid) + if entry == 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 vim.api.nvim_win_is_valid(entry.winid) then + local ok, err = pcall(vim.api.nvim_win_close, entry.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) + ok = pcall(vim.api.nvim_win_close, entry.winid, true) end if not ok then u.notify("Could not close the discussion window", vim.log.levels.WARN) @@ -217,16 +264,34 @@ M.close = function() -- 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() + entry.split:unmount() end) - M.split_visible = false - M.discussion_tree = nil - delete_bufs() + windows.remove(tabid) + if not windows.any() then + winbar.cleanup_timer() + delete_bufs() + end +end + +---Unmount every registered discussion window, across all tabpages. A window left in +---another tab would otherwise keep showing the outgoing MR's discussions and hold its +---NuiSplit buffer and augroups. +M.close_all = function() + local tabids = {} + windows.each(function(_, tabid) + table.insert(tabids, tabid) + end) + -- Close after collecting: it fires WinClosed, which mutates the registry we'd otherwise + -- still be iterating. + for _, tabid in ipairs(tabids) do + M.close(tabid) + end winbar.cleanup_timer() end ---Move to the discussion tree at the discussion from diagnostic on current line. M.move_to_discussion_tree = function() + local tabid = vim.api.nvim_get_current_tabpage() local current_line = vim.api.nvim_win_get_cursor(0)[1] local d = vim.diagnostic.get(0, { namespace = diagnostics.diagnostics_namespace, lnum = current_line - 1 }) @@ -248,12 +313,17 @@ M.move_to_discussion_tree = function() discussion_node:expand() end M.discussion_tree:render() - vim.api.nvim_set_current_win(M.split.winid) - M.switch_view_type("discussions") - vim.api.nvim_win_set_cursor(M.split.winid, { line_number, 0 }) + local entry = windows.get(tabid) + if entry then + vim.api.nvim_set_current_win(entry.winid) + M.switch_view_type("discussions") + vim.api.nvim_win_set_cursor(entry.winid, { line_number, 0 }) + else + u.notify("Discussion tree window not found", vim.log.levels.WARN) + end end - if not M.split_visible then + if windows.get(tabid) == nil then M.open(jump_after_tree_opened, "discussions") else jump_after_tree_opened() @@ -261,9 +331,10 @@ M.move_to_discussion_tree = function() end if #d == 0 then - if state.settings.reviewer_settings.jump_with_no_diagnostics then - vim.api.nvim_win_set_cursor(M.split.winid, { M.last_row, M.last_column }) - vim.api.nvim_set_current_win(M.split.winid) + local entry = windows.get(tabid) + if state.settings.reviewer_settings.jump_with_no_diagnostics and entry then + vim.api.nvim_win_set_cursor(entry.winid, { entry.last_row, entry.last_column }) + vim.api.nvim_set_current_win(entry.winid) else u.notify("No diagnostics for this line.", vim.log.levels.WARN) end @@ -293,7 +364,7 @@ M.reply = function(tree) return end - local node = tree:get_node() + local node = common.get_current_node(tree) local discussion_node = common.get_root_node(tree, node) if discussion_node == nil then @@ -322,7 +393,7 @@ M.delete_comment = function(tree, unlinked) prompt = "Delete comment?", }, function(choice) if choice == "Confirm" then - local current_node = tree:get_node() + local current_node = common.get_current_node(tree) local note_node = common.get_note_node(tree, current_node) local root_node = common.get_root_node(tree, current_node) if note_node == nil or root_node == nil then @@ -346,7 +417,7 @@ end ---@param tree NuiTree ---@param unlinked boolean M.edit_comment = function(tree, unlinked) - local current_node = tree:get_node() + local current_node = common.get_current_node(tree) local note_node = common.get_note_node(tree, current_node) local root_node = common.get_root_node(tree, current_node) if note_node == nil or root_node == nil then @@ -396,7 +467,7 @@ end ---Toggle the resolved status of the current discussion and send the change to the Go server. ---@param tree NuiTree M.toggle_discussion_resolved = function(tree) - local note = tree:get_node() + local note = common.get_current_node(tree) if note == nil then return end @@ -425,7 +496,7 @@ end ---@param tree any ---@param unlinked boolean M.add_emoji_to_note = function(tree, unlinked) - local node = tree:get_node() + local node = common.get_current_node(tree) local note_node = common.get_note_node(tree, node) if note_node == nil then @@ -448,7 +519,7 @@ end ---@param tree any ---@param unlinked boolean M.delete_emoji_from_note = function(tree, unlinked) - local node = tree:get_node() + local node = common.get_current_node(tree) local note_node = common.get_note_node(tree, node) if note_node == nil then @@ -509,8 +580,18 @@ M.rebuild_discussion_tree = function() return end - local current_node = discussions_tree.get_node_at_cursor(M.discussion_tree, M.last_node_at_cursor) - local current_cursor_column = vim.api.nvim_win_get_cursor(0)[2] + -- The buffer is shared between windows, and the rebuild clears and re-adds its lines. + -- Capture the cursor per window, or only one window's position survives. + local restore_targets = {} + windows.each(function(entry) + if entry.bufnr == M.linked_bufnr then + table.insert(restore_targets, { + winid = entry.winid, + node = discussions_tree.get_node_at_cursor(M.discussion_tree, entry.winid, entry.last_node_at_cursor), + column = vim.api.nvim_win_get_cursor(entry.winid)[2], + }) + end + end) local expanded_node_ids = M.gather_expanded_node_ids(M.discussion_tree) common.switch_can_edit_bufs(true, M.linked_bufnr, M.unlinked_bufnr) @@ -533,7 +614,9 @@ M.rebuild_discussion_tree = function() tree_utils.open_node_by_id(discussion_tree, id) end discussion_tree:render() - discussions_tree.restore_cursor_position(M.split.winid, discussion_tree, current_cursor_column, current_node, nil) + for _, target in ipairs(restore_targets) do + discussions_tree.restore_cursor_position(target.winid, discussion_tree, target.column, target.node, nil) + end M.set_tree_keymaps(discussion_tree, M.linked_bufnr, false) M.discussion_tree = discussion_tree @@ -548,8 +631,17 @@ M.rebuild_unlinked_discussion_tree = function() return end - local current_node = discussions_tree.get_node_at_cursor(M.unlinked_discussion_tree, M.last_node_at_cursor) - local current_cursor_column = vim.api.nvim_win_get_cursor(0)[2] + -- Capture cursor state per registered window, see M.rebuild_discussion_tree. + local restore_targets = {} + windows.each(function(entry) + if entry.bufnr == M.unlinked_bufnr then + table.insert(restore_targets, { + winid = entry.winid, + node = discussions_tree.get_node_at_cursor(M.unlinked_discussion_tree, entry.winid, entry.last_node_at_cursor), + column = vim.api.nvim_win_get_cursor(entry.winid)[2], + }) + end + end) local expanded_node_ids = M.gather_expanded_node_ids(M.unlinked_discussion_tree) common.switch_can_edit_bufs(true, M.linked_bufnr, M.unlinked_bufnr) @@ -572,7 +664,9 @@ M.rebuild_unlinked_discussion_tree = function() tree_utils.open_node_by_id(unlinked_discussion_tree, id) end unlinked_discussion_tree:render() - discussions_tree.restore_cursor_position(M.split.winid, unlinked_discussion_tree, current_cursor_column, current_node) + for _, target in ipairs(restore_targets) do + discussions_tree.restore_cursor_position(target.winid, unlinked_discussion_tree, target.column, target.node) + end M.set_tree_keymaps(unlinked_discussion_tree, M.unlinked_bufnr, true) M.unlinked_discussion_tree = unlinked_discussion_tree @@ -581,47 +675,60 @@ M.rebuild_unlinked_discussion_tree = function() state.unlinked_discussion_tree.unresolved_expanded = false end ----Create the split for the discussion tree and returns it, with both buffer numbers. +---Create the split window for the discussion tree in the current tabpage. ---@return NuiSplit ----@return integer ----@return integer -M.create_split_and_bufs = function() +M.create_split = function() local position = state.settings.discussion_tree.position local size = state.settings.discussion_tree.size local relative = state.settings.discussion_tree.relative - local split = Split({ + return Split({ relative = relative, position = position, size = size, }) +end +---Create the linked/unlinked discussion buffers, shared by every discussion window, and +---their cursor-tracking autocmds. +---@return integer linked_bufnr +---@return integer unlinked_bufnr +M.create_bufs = function() local linked_bufnr = vim.api.nvim_create_buf(true, false) local unlinked_bufnr = vim.api.nvim_create_buf(true, false) vim.api.nvim_create_autocmd("WinLeave", { buffer = linked_bufnr, callback = function() - M.last_row, M.last_column = unpack(vim.api.nvim_win_get_cursor(0)) - M.last_node_at_cursor = M.discussion_tree and M.discussion_tree:get_node() or nil + local entry = entry_for_winid(vim.api.nvim_get_current_win()) + if entry == nil then + return + end + entry.last_row, entry.last_column = unpack(vim.api.nvim_win_get_cursor(0)) + entry.last_node_at_cursor = M.discussion_tree and M.discussion_tree:get_node(entry.last_row) or nil end, }) vim.api.nvim_create_autocmd("WinLeave", { buffer = unlinked_bufnr, callback = function() - M.last_node_at_cursor = M.unlinked_discussion_tree and M.unlinked_discussion_tree:get_node() or nil + local entry = entry_for_winid(vim.api.nvim_get_current_win()) + if entry == nil then + return + end + local cursor_row = vim.api.nvim_win_get_cursor(0)[1] + entry.last_node_at_cursor = M.unlinked_discussion_tree and M.unlinked_discussion_tree:get_node(cursor_row) or nil end, }) - return split, linked_bufnr, unlinked_bufnr + return linked_bufnr, unlinked_bufnr end ---Check if type of current node is note or note body. ---@param tree NuiTree ---@return boolean M.is_current_node_note = function(tree) - return common.is_node_note(tree:get_node()) + return common.is_node_note(common.get_current_node(tree)) end ---Set the discussion tree keymaps. @@ -738,13 +845,21 @@ M.set_tree_keymaps = function(tree, bufnr, unlinked) if keymaps.discussion_tree.toggle_node then vim.keymap.set("n", keymaps.discussion_tree.toggle_node, function() - tree_utils.toggle_node(M.split.winid, tree) + local entry = windows.get() + if entry == nil then + return + end + tree_utils.toggle_node(entry.winid, tree) end, { buffer = bufnr, desc = "Toggle node", nowait = keymaps.discussion_tree.toggle_node_nowait }) end if keymaps.discussion_tree.toggle_all_discussions then vim.keymap.set("n", keymaps.discussion_tree.toggle_all_discussions, function() - tree_utils.toggle_nodes(M.split.winid, tree, unlinked, { + local entry = windows.get() + if entry == nil then + return + end + tree_utils.toggle_nodes(entry.winid, tree, unlinked, { toggle_resolved = true, toggle_unresolved = true, keep_current_open = state.settings.discussion_tree.keep_current_open, @@ -758,7 +873,11 @@ M.set_tree_keymaps = function(tree, bufnr, unlinked) if keymaps.discussion_tree.toggle_resolved_discussions then vim.keymap.set("n", keymaps.discussion_tree.toggle_resolved_discussions, function() - tree_utils.toggle_nodes(M.split.winid, tree, unlinked, { + local entry = windows.get() + if entry == nil then + return + end + tree_utils.toggle_nodes(entry.winid, tree, unlinked, { toggle_resolved = true, toggle_unresolved = false, keep_current_open = state.settings.discussion_tree.keep_current_open, @@ -772,7 +891,11 @@ M.set_tree_keymaps = function(tree, bufnr, unlinked) if keymaps.discussion_tree.toggle_unresolved_discussions then vim.keymap.set("n", keymaps.discussion_tree.toggle_unresolved_discussions, function() - tree_utils.toggle_nodes(M.split.winid, tree, unlinked, { + local entry = windows.get() + if entry == nil then + return + end + tree_utils.toggle_nodes(entry.winid, tree, unlinked, { toggle_resolved = false, toggle_unresolved = true, keep_current_open = state.settings.discussion_tree.keep_current_open, @@ -861,18 +984,25 @@ M.set_tree_keymaps = function(tree, bufnr, unlinked) emoji.init_popup(tree, bufnr) end ----Toggle the current view type (or sets it to `override`) and update the view. +---Toggle the view type of the current tabpage's discussion window, or set it to +---`override`. ---@param override? "discussions"|"notes" The view type to select M.switch_view_type = function(override) - vim.api.nvim_set_option_value("winfixbuf", false, { win = M.split.winid }) - if override == "discussions" or M.current_view_type == "notes" then - M.current_view_type = "discussions" - vim.api.nvim_set_current_buf(M.linked_bufnr) - elseif override == "notes" or M.current_view_type == "discussions" then - M.current_view_type = "notes" - vim.api.nvim_set_current_buf(M.unlinked_bufnr) - end - vim.api.nvim_set_option_value("winfixbuf", true, { win = M.split.winid }) + local entry = windows.get() + if entry == nil then + return + end + vim.api.nvim_set_option_value("winfixbuf", false, { win = entry.winid }) + if override == "discussions" or entry.view_type == "notes" then + entry.view_type = "discussions" + entry.bufnr = M.linked_bufnr + vim.api.nvim_win_set_buf(entry.winid, entry.bufnr) + elseif override == "notes" or entry.view_type == "discussions" then + entry.view_type = "notes" + entry.bufnr = M.unlinked_bufnr + vim.api.nvim_win_set_buf(entry.winid, entry.bufnr) + end + vim.api.nvim_set_option_value("winfixbuf", true, { win = entry.winid }) winbar.update_winbar() end @@ -913,7 +1043,7 @@ end ---@param tree NuiTree ---@return boolean M.is_draft_note = function(tree) - local current_node = tree:get_node() + local current_node = common.get_current_node(tree) local note_node = common.get_note_node(tree, current_node) if note_node and note_node.is_draft then return true diff --git a/lua/gitlab/actions/discussions/tree.lua b/lua/gitlab/actions/discussions/tree.lua index 5cd3b1cc..55276f37 100644 --- a/lua/gitlab/actions/discussions/tree.lua +++ b/lua/gitlab/actions/discussions/tree.lua @@ -410,12 +410,11 @@ end ---@param unlinked boolean ---@param opts ToggleNodesOptions M.toggle_nodes = function(winid, tree, unlinked, opts) - local current_node = tree:get_node() + local current_node, current_cursor_column = common.get_node_at(tree, winid) if current_node == nil then return end local root_node = common.get_root_node(tree, current_node) - local current_cursor_column = vim.api.nvim_win_get_cursor(winid)[2] for _, node in ipairs(tree:get_nodes()) do if opts.toggle_resolved then if @@ -460,13 +459,14 @@ end ---Get current node for restoring cursor position. ---@param tree NuiTree The inline discussion tree or the unlinked discussion tree ----@param last_node? NuiTree.Node The last active discussion tree node in case we are not in any of the discussion trees -M.get_node_at_cursor = function(tree, last_node) +---@param winid integer The window whose cursor position to check +---@param last_node? NuiTree.Node The last active discussion tree node in case `winid` isn't the current window +M.get_node_at_cursor = function(tree, winid, last_node) if tree == nil then return end - if vim.api.nvim_get_current_win() == vim.fn.win_findbuf(tree.bufnr)[1] then - return tree:get_node() + if winid == vim.api.nvim_get_current_win() then + return (common.get_node_at(tree, winid)) else return last_node end @@ -491,7 +491,9 @@ M.restore_cursor_position = function(winid, tree, cursor_column, original_node, end end if line_number ~= nil and winid and vim.api.nvim_win_is_valid(winid) then - local last_line = vim.fn.line("$") + -- The rebuild restores every registered window, so `winid` is usually not the current + -- one; the clamp has to be against its buffer, not against whatever is focused. + local last_line = vim.api.nvim_buf_line_count(vim.api.nvim_win_get_buf(winid)) vim.api.nvim_win_set_cursor(winid, { math.min(line_number, last_line), cursor_column or 0 }) end end @@ -550,8 +552,7 @@ end ---@param winid integer The id if the tree split ---@param tree NuiTree The current discussion tree M.toggle_node = function(winid, tree) - local node = tree:get_node() - local current_cursor_column = vim.api.nvim_win_get_cursor(winid)[2] + local node, current_cursor_column = common.get_node_at(tree, winid) -- Switch to the "note" node from "note_body" nodes to enable toggling discussions inside comments if node ~= nil and node.type == "note_body" then diff --git a/lua/gitlab/actions/discussions/winbar.lua b/lua/gitlab/actions/discussions/winbar.lua index f59dbee6..dbeda6b1 100644 --- a/lua/gitlab/actions/discussions/winbar.lua +++ b/lua/gitlab/actions/discussions/winbar.lua @@ -1,6 +1,7 @@ local u = require("gitlab.utils") local List = require("gitlab.utils.list") local state = require("gitlab.state") +local windows = require("gitlab.actions.discussions.windows") local M = {} @@ -47,9 +48,10 @@ end local spinner_index = 0 state.discussion_tree.last_updated = nil ----Return the raw content of the winbar. +---Return the raw content of the winbar for a window showing `view_type`. +---@param view_type "discussions"|"notes" ---@return string -local function content() +local function content(view_type) local updated if state.discussion_tree.last_updated then local last_update = tostring(os.date("!%Y-%m-%dT%H:%M:%S", state.discussion_tree.last_updated)) @@ -77,6 +79,7 @@ local function content() end) local t = { + view_type = view_type, resolvable_discussions = resolvable_discussions, resolved_discussions = resolved_discussions, non_resolvable_discussions = non_resolvable_discussions, @@ -94,24 +97,12 @@ local function content() return state.settings.discussion_tree.winbar and state.settings.discussion_tree.winbar(t) or M.make_winbar(t) end ----Update the winbar. +---Update the winbar in every registered discussion window, from each window's own +---view_type. M.update_winbar = function() - local d = require("gitlab.actions.discussions") - if d.split == nil then - return - end - - local win_id = d.split.winid - if win_id == nil then - return - end - - if not vim.api.nvim_win_is_valid(win_id) then - return - end - - local c = content() - vim.api.nvim_set_option_value("winbar", c, { scope = "local", win = win_id }) + windows.each(function(entry) + vim.api.nvim_set_option_value("winbar", content(entry.view_type), { scope = "local", win = entry.winid }) + end) end ---TODO: remove this function and hardcode " " where called @@ -152,7 +143,7 @@ end ---@param t WinbarTable ---@return string winbar The raw content of the winbar M.make_winbar = function(t) - local discussions_focused = require("gitlab.actions.discussions").current_view_type == "discussions" + local discussions_focused = t.view_type == "discussions" local discussion_text = add_drafts_and_resolvable( "Comments:", t.resolvable_discussions, diff --git a/lua/gitlab/actions/draft_notes/init.lua b/lua/gitlab/actions/draft_notes/init.lua index fd8100fa..475bb298 100755 --- a/lua/gitlab/actions/draft_notes/init.lua +++ b/lua/gitlab/actions/draft_notes/init.lua @@ -112,7 +112,7 @@ end ---API call to refresh the relevant data for that tree and re-render it. ---@param tree NuiTree M.confirm_publish_draft = function(tree) - local current_node = tree:get_node() + local current_node = common.get_current_node(tree) local note_node = common.get_note_node(tree, current_node) local root_node = common.get_root_node(tree, current_node) diff --git a/lua/gitlab/actions/merge.lua b/lua/gitlab/actions/merge.lua index 55e8a4cb..a5282278 100644 --- a/lua/gitlab/actions/merge.lua +++ b/lua/gitlab/actions/merge.lua @@ -60,7 +60,7 @@ M.confirm_merge = function(merge_body, squash_message) end job.run_job("/mr/merge", "POST", merge_body, function(data) - reviewer.close() + reviewer.close_session() u.notify(data.message, vim.log.levels.INFO) end) end diff --git a/lua/gitlab/actions/merge_requests.lua b/lua/gitlab/actions/merge_requests.lua index 154b5b5b..1c69b863 100644 --- a/lua/gitlab/actions/merge_requests.lua +++ b/lua/gitlab/actions/merge_requests.lua @@ -27,9 +27,7 @@ M.choose_merge_request = function(opts) return end - if reviewer.is_open then - reviewer.close() - end + reviewer.close_session() if choice.source_branch ~= git.get_current_branch() then local has_clean_tree, clean_tree_err = git.has_clean_tree() diff --git a/lua/gitlab/emoji.lua b/lua/gitlab/emoji.lua index 9d8e5daa..14f2d707 100644 --- a/lua/gitlab/emoji.lua +++ b/lua/gitlab/emoji.lua @@ -92,7 +92,7 @@ end M.init_popup = function(tree, bufnr) vim.api.nvim_create_autocmd({ "CursorHold" }, { callback = function() - local node = tree:get_node() + local node = common.get_current_node(tree) if node == nil or not common.is_node_note(node) then return end diff --git a/lua/gitlab/init.lua b/lua/gitlab/init.lua index 4fd8378b..c763073c 100644 --- a/lua/gitlab/init.lua +++ b/lua/gitlab/init.lua @@ -6,6 +6,7 @@ local state = require("gitlab.state") local reviewer = require("gitlab.reviewer") local history = require("gitlab.reviewer.history") local discussions = require("gitlab.actions.discussions") +local discussion_windows = require("gitlab.actions.discussions.windows") local merge_requests = require("gitlab.actions.merge_requests") local merge = require("gitlab.actions.merge") local rebase = require("gitlab.actions.rebase") @@ -78,7 +79,7 @@ return { reviewer.reload() end, close_review = function() - reviewer.close() + reviewer.close_session() end, browse_commits = async.sequence({ info }, function() reviewer.browse_commits() @@ -94,7 +95,7 @@ return { rebase = async.sequence({ u.merge(mergeability, { refresh = true }), info }, rebase.rebase), -- Discussion Tree Actions 🌴 toggle_discussions = function() - if discussions.split_visible then + if discussion_windows.get() then discussions.close() else async.sequence({ diff --git a/lua/gitlab/reviewer/init.lua b/lua/gitlab/reviewer/init.lua index 66d19d33..8ff366b0 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -18,6 +18,25 @@ local M = { buf_winids = {}, } +---Handle Diffview's "view_closed" event for the reviewer's own tabpage: forget it, and stop +---the winbar timer only if no discussion window is registered in any tab anymore (one may +---still be open elsewhere, e.g. the commit browser). +---@param view { tabpage: integer } +M.on_diffview_closed = function(view) + if view.tabpage == M.tabid then + M.tabid = nil + if not require("gitlab.actions.discussions.windows").any() then + require("gitlab.actions.discussions.winbar").cleanup_timer() + end + end +end + +---Tear down discussion windows before auto-opening fresh ones for a (re-)opened review. +M.reset_discussions_for_auto_open = function() + require("gitlab.actions.discussions").close_all() + require("gitlab").toggle_discussions() -- Fetches data and opens discussions +end + -- Open the reviewer windows. M.open = function() require("gitlab.emoji").init() -- Read in emojis for lookup purposes @@ -68,24 +87,15 @@ M.open = function() ) end - -- Register Diffview hook for close event to set tab page # to nil - local on_diffview_closed = function(view) - if view.tabpage == M.tabid then - M.tabid = nil - require("gitlab.actions.discussions.winbar").cleanup_timer() - end - end require("diffview.config").user_emitter:on("view_closed", function(_, args) if M.tabid == args.tabpage then M.is_open = false - on_diffview_closed(args) + M.on_diffview_closed(args) end end) if state.settings.discussion_tree.auto_open then - local discussions = require("gitlab.actions.discussions") - discussions.close() - require("gitlab").toggle_discussions() -- Fetches data and opens discussions + M.reset_discussions_for_auto_open() end git.check_mr_in_good_condition() @@ -116,6 +126,12 @@ M.browse_commits = function() vim.api.nvim_command(string.format("DiffviewFileHistory --range=%s..%s", diff_refs.base_sha, diff_refs.head_sha)) M.history_tabid = vim.api.nvim_get_current_tabpage() + + if state.settings.discussion_tree.auto_open then + -- Not reset_discussions_for_auto_open: its close_all() would take the reviewer's own + -- discussion window down with it, and that one lives in a different tabpage. + require("gitlab").toggle_discussions() + end end ---Forget the commit-history tab once its Diffview view closes. history_tabid gates the @@ -128,16 +144,35 @@ M.clear_history_tab = function(tabpage) end end ----Close the reviewer and clean up. +---Close the reviewer's own tabpage, together with the discussion window registered there. +---Windows in other tabs (e.g. the commit browser) are left standing -- for tearing those +---down too, see M.close_session. M.close = function() - if M.tabid ~= nil and vim.api.nvim_tabpage_is_valid(M.tabid) then - -- FIXME: This fails if there is only one tabpage. Find a way to use DiffviewClose - -- that was originally here, but use it for the correct tabpage when there are - -- multiple Diffviews open. - vim.cmd.tabclose(vim.api.nvim_tabpage_get_number(M.tabid)) - end - local discussions = require("gitlab.actions.discussions") - discussions.close() + if M.tabid == nil or not vim.api.nvim_tabpage_is_valid(M.tabid) then + return + end + -- NuiSplit releases its buffer and augroups only in Split:unmount, which tabclose does + -- not trigger, so unmount explicitly first. + require("gitlab.actions.discussions").close(M.tabid) + -- FIXME: This fails if there is only one tabpage. Find a way to use DiffviewClose + -- that was originally here, but use it for the correct tabpage when there are + -- multiple Diffviews open. pcall'd so M.close_session still runs its remaining steps + -- when this is the only tabpage left. + pcall(vim.cmd.tabclose, vim.api.nvim_tabpage_get_number(M.tabid)) +end + +---Tear down the whole review session: the reviewer tab (see M.close), the commit-browser +---tab with its history_tabid handle, and every discussion window left in another tab. +---Use this when the MR under review is being left behind, M.close alone when it is not. +M.close_session = function() + M.close() + if M.history_tabid ~= nil and vim.api.nvim_tabpage_is_valid(M.history_tabid) then + local closed = pcall(vim.cmd.tabclose, vim.api.nvim_tabpage_get_number(M.history_tabid)) + if closed then + M.history_tabid = nil + end + end + require("gitlab.actions.discussions").close_all() end ---Load new INFO state from Gitlab. Then, if diffview.api is available, apply the new diff --git a/tests/spec/commit_comment_jump_spec.lua b/tests/spec/commit_comment_jump_spec.lua index 43cd59d6..251a871e 100644 --- a/tests/spec/commit_comment_jump_spec.lua +++ b/tests/spec/commit_comment_jump_spec.lua @@ -94,6 +94,7 @@ describe("actions/common.jump_to_reviewer", function() local originals = {} before_each(function() + originals.get_current_node = common.get_current_node originals.get_line_number_from_node = common.get_line_number_from_node originals.reviewer_jump = reviewer.jump originals.jump_to_commit = history.jump_to_commit @@ -101,6 +102,7 @@ describe("actions/common.jump_to_reviewer", function() end) after_each(function() + common.get_current_node = originals.get_current_node common.get_line_number_from_node = originals.get_line_number_from_node reviewer.jump = originals.reviewer_jump history.jump_to_commit = originals.jump_to_commit @@ -109,14 +111,11 @@ describe("actions/common.jump_to_reviewer", function() ---@param node table The node the cursor is on ---@param is_new_sha boolean - ---@return table calls, table tree A tree holding `node`, to pass to jump_to_reviewer local function arrange(node, is_new_sha) local calls = { reviewer = {}, history = {}, notified = {} } - local tree = { - get_node = function() - return node - end, - } + common.get_current_node = function() + return node + end common.get_line_number_from_node = function() return 11, is_new_sha end @@ -129,32 +128,31 @@ describe("actions/common.jump_to_reviewer", function() require("gitlab.utils").notify = function(msg) table.insert(calls.notified, msg) end - return calls, tree + return calls end it("Sends a commit-anchored comment to the commit browser", function() - local calls, tree = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, true) + local calls = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, true) - common.jump_to_reviewer(tree) + common.jump_to_reviewer({}) assert.are.same({}, calls.reviewer) assert.are.same({ { "abc123", "file.lua", 11 } }, calls.history) end) it("Sends a plain comment to the reviewer", function() - local calls, tree = - arrange({ is_root = true, type = "note", file_name = "file.lua", old_file_name = "file.lua" }, true) + local calls = arrange({ is_root = true, type = "note", file_name = "file.lua", old_file_name = "file.lua" }, true) - common.jump_to_reviewer(tree) + common.jump_to_reviewer({}) assert.are.same({}, calls.history) assert.are.equal(1, #calls.reviewer) end) it("Refuses an old-side commit comment rather than jumping to a wrong line", function() - local calls, tree = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, false) + local calls = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, false) - common.jump_to_reviewer(tree) + common.jump_to_reviewer({}) assert.are.same({}, calls.history) assert.are.same({}, calls.reviewer) diff --git a/tests/spec/discussions_current_node_spec.lua b/tests/spec/discussions_current_node_spec.lua new file mode 100644 index 00000000..3a0ca0e8 --- /dev/null +++ b/tests/spec/discussions_current_node_spec.lua @@ -0,0 +1,121 @@ +-- The discussion buffers are shared between tabpages, so a tree can be displayed in more +-- than one window. Verifies that the node under the cursor is resolved from the window the +-- user is in, not from the first window that happens to show the buffer. + +local NuiTree = require("nui.tree") +local common = require("gitlab.actions.common") +local tree_utils = require("gitlab.actions.discussions.tree") +local windows = require("gitlab.actions.discussions.windows") + +---Render a two-note tree into a fresh buffer. Both notes are collapsed, so line 1 holds +---note "a" and line 2 note "b". +---@return NuiTree, integer bufnr +local function make_tree() + local bufnr = vim.api.nvim_create_buf(false, true) + local tree = NuiTree({ + bufnr = bufnr, + nodes = { + NuiTree.Node( + { id = "a", text = "a", type = "note", is_root = true }, + { NuiTree.Node({ id = "a1", text = "a1", type = "note_body" }) } + ), + NuiTree.Node( + { id = "b", text = "b", type = "note", is_root = true }, + { NuiTree.Node({ id = "b1", text = "b1", type = "note_body" }) } + ), + }, + }) + tree:render() + return tree, bufnr +end + +---Open a new tabpage showing `bufnr` with the cursor on `row`. +---@param bufnr integer +---@param row integer +---@return integer tabid, integer winid +local function open_in_new_tab(bufnr, row) + vim.cmd("tabnew") + local winid = vim.api.nvim_get_current_win() + vim.api.nvim_win_set_buf(winid, bufnr) + vim.api.nvim_win_set_cursor(winid, { row, 0 }) + return vim.api.nvim_get_current_tabpage(), winid +end + +describe("actions/common.get_current_node", function() + after_each(function() + vim.cmd("silent! tabonly") + end) + + it("Reads the cursor of the current window, not of the first window showing the buffer", function() + local tree, bufnr = make_tree() + local first_tabid, _ = open_in_new_tab(bufnr, 1) + local second_tabid, _ = open_in_new_tab(bufnr, 2) + + assert.are.equal("b", common.get_current_node(tree).text) + + vim.api.nvim_set_current_tabpage(first_tabid) + assert.are.equal("a", common.get_current_node(tree).text) + + vim.api.nvim_set_current_tabpage(second_tabid) + assert.are.equal("b", common.get_current_node(tree).text) + end) + + it("Falls back to the registered discussion window of the current tabpage", function() + local tree, bufnr = make_tree() + open_in_new_tab(bufnr, 1) + + local tabid, winid = open_in_new_tab(bufnr, 2) + windows.set(tabid, { winid = winid, bufnr = bufnr, view_type = "discussions" }) + -- Leave the tree window while staying in the same tabpage + vim.cmd("split") + vim.api.nvim_win_set_buf(0, vim.api.nvim_create_buf(false, true)) + + assert.are.equal("b", common.get_current_node(tree).text) + windows.remove(tabid) + end) + + it("Returns nil when no window of the current tabpage shows the tree", function() + local tree, bufnr = make_tree() + open_in_new_tab(bufnr, 1) + vim.cmd("tabnew") + + assert.is_nil(common.get_current_node(tree)) + end) +end) + +describe("actions/discussions/tree.restore_cursor_position", function() + after_each(function() + vim.cmd("silent! tabonly") + end) + + it("Clamps against the target window's buffer, not against the focused one", function() + local tree, bufnr = make_tree() + local _, winid = open_in_new_tab(bufnr, 1) + + -- Focus a window whose buffer is shorter than the tree. The rebuild restores cursors + -- for every registered window, so this is the normal case, not an exotic one. + vim.cmd("tabnew") + vim.api.nvim_buf_set_lines(vim.api.nvim_get_current_buf(), 0, -1, false, { "one line" }) + + tree_utils.restore_cursor_position(winid, tree, 0, tree:get_node("-b"), nil) + + assert.are.same({ 2, 0 }, vim.api.nvim_win_get_cursor(winid)) + end) +end) + +describe("actions/discussions/tree.toggle_node", function() + after_each(function() + vim.cmd("silent! tabonly") + end) + + it("Toggles the node under the given window's cursor while another tab shows the tree", function() + local tree, bufnr = make_tree() + open_in_new_tab(bufnr, 1) + local _, winid = open_in_new_tab(bufnr, 2) + + tree_utils.toggle_node(winid, tree) + + assert.is_false(tree:get_node("-a"):is_expanded()) + assert.is_true(tree:get_node("-b"):is_expanded()) + end) +end) diff --git a/tests/spec/discussions_orphan_window_spec.lua b/tests/spec/discussions_orphan_window_spec.lua index dd5bbb58..19c8533d 100644 --- a/tests/spec/discussions_orphan_window_spec.lua +++ b/tests/spec/discussions_orphan_window_spec.lua @@ -1,75 +1,72 @@ -- 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. +-- tests check that the window is gone afterwards and that the registry 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") +local windows = require("gitlab.actions.discussions.windows") ----Register a split with the given unmount behaviour, in a window of its own. +---Register a split with the given unmount behaviour, in a tabpage of its own. ---@param unmount fun(split: table) ---@return integer winid +---@return integer tabid 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 + local tabid = vim.api.nvim_get_current_tabpage() + windows.set(tabid, { + split = { winid = winid, unmount = unmount }, + winid = winid, + bufnr = vim.api.nvim_get_current_buf(), + view_type = "discussions", + }) + return winid, tabid 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) + local winid, tabid = 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) + assert.is_nil(windows.get(tabid)) end) it("Closes the window itself when unmounting raises", function() - local winid = arrange(function() + local winid, tabid = 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) + assert.is_nil(windows.get(tabid)) end) it("Closes the window when it is the last one in the session", function() - vim.cmd("silent! tabonly") - vim.cmd("silent! only") + local winid, tabid = arrange(function() end) -- 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 + vim.cmd("silent! tabonly") + vim.cmd("silent! only") 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) + assert.is_nil(windows.get(tabid)) end) it("Tears the split down when the user closes the window by hand", function() @@ -79,15 +76,18 @@ describe("actions/discussions.close", function() draft_notes.rebuild_view = function() end vim.cmd("tabnew") discussions.open() - local winid = discussions.split.winid + local winid = windows.get().winid + assert.is_not_nil(discussions.linked_bufnr, "M.open left no discussion buffer to release") vim.api.nvim_win_close(winid, true) - local torn_down = vim.wait(200, function() - return discussions.split_visible == false + -- The registry drops a dead window on its own, so the deferred teardown shows up + -- elsewhere: the last window to close releases the discussion buffers. + local released = vim.wait(200, function() + return discussions.linked_bufnr == nil end, 10) - - assert.is_true(torn_down, "split_visible is still set 200ms after the window closed") draft_notes.rebuild_view = original_rebuild_view + + assert.is_true(released, "the discussion buffers are still listed 200ms after the window closed") end) end) diff --git a/tests/spec/discussions_shared_bufs_spec.lua b/tests/spec/discussions_shared_bufs_spec.lua index 36e332a1..13fb2251 100644 --- a/tests/spec/discussions_shared_bufs_spec.lua +++ b/tests/spec/discussions_shared_bufs_spec.lua @@ -1,8 +1,9 @@ --- The linked and unlinked buffers belong to one open, not to the session, so close() owns --- their release. +-- The linked and unlinked buffers are shared by every discussion window but not by the +-- session: the first open creates them, the last close owns their release. local discussions = require("gitlab.actions.discussions") local draft_notes = require("gitlab.actions.draft_notes") +local windows = require("gitlab.actions.discussions.windows") local winbar = require("gitlab.actions.discussions.winbar") local state = require("gitlab.state") @@ -25,8 +26,6 @@ describe("actions/discussions buffers", function() 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 @@ -69,4 +68,34 @@ describe("actions/discussions buffers", function() ) discussions.close() end) + + it("Keeps the buffers while another tab still shows them", function() + vim.cmd("tabnew") + discussions.open() + local linked, unlinked = discussions.linked_bufnr, discussions.unlinked_bufnr + vim.cmd("tabnew") + discussions.open() + + discussions.close() + + assert.is_true(vim.api.nvim_buf_is_valid(linked), ("linked buffer %d was pulled from the other tab"):format(linked)) + assert.is_true( + vim.api.nvim_buf_is_valid(unlinked), + ("unlinked buffer %d was pulled from the other tab"):format(unlinked) + ) + end) + + it("Deletes both buffers when the user closes the last window by hand", function() + vim.cmd("tabnew") + discussions.open() + local linked = discussions.linked_bufnr + + vim.api.nvim_win_close(windows.get().winid, true) + + local released = vim.wait(200, function() + return not vim.api.nvim_buf_is_valid(linked) + end, 10) + + assert.is_true(released, ("linked buffer %d is still alive 200ms after the window closed"):format(linked)) + end) end) diff --git a/tests/spec/discussions_spec.lua b/tests/spec/discussions_spec.lua index 8f2c0416..bc5b6d08 100644 --- a/tests/spec/discussions_spec.lua +++ b/tests/spec/discussions_spec.lua @@ -3,4 +3,219 @@ describe("gitlab/actions/discussions/init.lua", function() local utils_ok, _ = pcall(require, "gitlab.actions.discussions") assert._is_true(utils_ok) end) + + describe("multi-tab window handling", function() + local discussions = require("gitlab.actions.discussions") + local windows = require("gitlab.actions.discussions.windows") + local draft_notes = require("gitlab.actions.draft_notes") + local winbar = require("gitlab.actions.discussions.winbar") + local state = require("gitlab.state") + local original_rebuild_view + + before_each(function() + -- M.open's tail calls into draft_notes.rebuild_view to fetch fresh data from the Go + -- server, which these tests aren't exercising and have no server to talk 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 + -- `tabonly` never closes the *current* tab, so hop to a fresh one first to make sure + -- every discussion window opened by the test (incl. the one in the tab we ended on) + -- actually closes and prunes its registry entry via the real WinClosed path. + vim.cmd("tabnew") + vim.cmd("silent! tabonly") + winbar.cleanup_timer() + discussions.linked_bufnr = nil + discussions.unlinked_bufnr = nil + discussions.discussion_tree = nil + discussions.unlinked_discussion_tree = nil + state.DISCUSSION_DATA = nil + end) + + it("Open() in a second tab adds a second window entry without creating new buffers", function() + vim.cmd("tabnew") + local tab_a = vim.api.nvim_get_current_tabpage() + discussions.open() + local linked_bufnr, unlinked_bufnr = discussions.linked_bufnr, discussions.unlinked_bufnr + + vim.cmd("tabnew") + local tab_b = vim.api.nvim_get_current_tabpage() + discussions.open() + + local entry_a = windows.get(tab_a) + local entry_b = windows.get(tab_b) + assert.is_not_nil(entry_a) + assert.is_not_nil(entry_b) + assert.is_true(entry_a.winid ~= entry_b.winid) + assert.are.equal(linked_bufnr, discussions.linked_bufnr) + assert.are.equal(unlinked_bufnr, discussions.unlinked_bufnr) + end) + + it("Switch_view_type only changes the current tab's window", function() + vim.cmd("tabnew") + local tab_a = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_a = windows.get(tab_a) + + vim.cmd("tabnew") + local tab_b = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_b = windows.get(tab_b) + + vim.api.nvim_set_current_tabpage(tab_a) + discussions.switch_view_type("notes") + + assert.are.equal(discussions.unlinked_bufnr, vim.api.nvim_win_get_buf(entry_a.winid)) + assert.are.equal("notes", entry_a.view_type) + assert.are.equal(discussions.linked_bufnr, vim.api.nvim_win_get_buf(entry_b.winid)) + assert.are.equal("discussions", entry_b.view_type) + end) + + it("Toggling the view type (no override) in one tab doesn't skip the toggle in another", function() + -- Regression: the decision used to hang off a module-global "current view type", so + -- toggling tab A to "notes" made tab B's own (unrelated) toggle press a no-op. + vim.cmd("tabnew") + local tab_a = vim.api.nvim_get_current_tabpage() + discussions.open() + + vim.cmd("tabnew") + local tab_b = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_b = windows.get(tab_b) + + vim.api.nvim_set_current_tabpage(tab_a) + discussions.switch_view_type() -- tab A: discussions -> notes + + vim.api.nvim_set_current_tabpage(tab_b) + discussions.switch_view_type() -- tab B: discussions -> notes, independent of tab A + + assert.are.equal("notes", entry_b.view_type) + assert.are.equal(discussions.unlinked_bufnr, vim.api.nvim_win_get_buf(entry_b.winid)) + end) + + it("Close(tabid) unmounts that tab's window even when it isn't the current tab", function() + vim.cmd("tabnew") + local target_tab = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry = windows.get(target_tab) + + vim.cmd("tabnew") -- move away from target_tab before closing it + + discussions.close(target_tab) + + assert.is_nil(windows.get(target_tab)) + assert.is_false(vim.api.nvim_win_is_valid(entry.winid)) + end) + + it("Close_all() unmounts every registered window across all tabs", function() + vim.cmd("tabnew") + local tab_a = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_a = windows.get(tab_a) + + vim.cmd("tabnew") + local tab_b = vim.api.nvim_get_current_tabpage() + discussions.open() + + discussions.close_all() + + assert.is_nil(windows.get(tab_a)) + assert.is_nil(windows.get(tab_b)) + assert.is_false(vim.api.nvim_win_is_valid(entry_a.winid)) + assert.is_nil(winbar.timer) + end) + + it("Closing the window by hand releases the split's own buffer", function() + vim.cmd("tabnew") + discussions.open() + local entry = windows.get() + local split_bufnr = entry.split.bufnr + assert.is_true(vim.api.nvim_buf_is_valid(split_bufnr)) + + vim.api.nvim_win_close(entry.winid, true) + + assert.is_true(vim.wait(200, function() + return not vim.api.nvim_buf_is_valid(split_bufnr) + end, 10)) + end) + + it("Closing one tab's discussion window (WinClosed) removes only that tab's entry", function() + vim.cmd("tabnew") + local tab_a = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_a = windows.get(tab_a) + + vim.cmd("tabnew") + local tab_b = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_b = windows.get(tab_b) + + vim.api.nvim_win_close(entry_b.winid, true) + + assert.is_nil(windows.get(tab_b)) + assert.is_not_nil(windows.get(tab_a)) + assert.is_not_nil(winbar.timer) + + vim.api.nvim_win_close(entry_a.winid, true) + + assert.is_nil(windows.get(tab_a)) + assert.is_nil(winbar.timer) + end) + + it("Rebuild_discussion_tree restores each tab's own cursor node, not another tab's", function() + state.INFO = { web_url = "https://gitlab.com/some-org/-/merge_requests/1" } + state.settings.discussion_tree.tree_type = "simple" + local function make_discussion(id, note_id, body) + return { + id = id, + individual_note = false, + notes = { + { + id = note_id, + author = { username = "author" }, + body = body, + created_at = "2023-10-28T18:27:34.082Z", + position = vim.NIL, + resolvable = false, + resolved = false, + }, + }, + } + end + state.DISCUSSION_DATA = { + discussions = { make_discussion("disc-a", 101, "Discussion A"), make_discussion("disc-b", 102, "Discussion B") }, + unlinked_discussions = {}, + emojis = {}, + } + + vim.cmd("tabnew") + local tab_a = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_a = windows.get(tab_a) + + vim.cmd("tabnew") + local tab_b = vim.api.nvim_get_current_tabpage() + discussions.open() + local entry_b = windows.get(tab_b) + + local _, line_a = discussions.discussion_tree:get_node("-disc-a") + local _, line_b = discussions.discussion_tree:get_node("-disc-b") + + vim.api.nvim_set_current_win(entry_a.winid) + vim.api.nvim_win_set_cursor(entry_a.winid, { line_a, 0 }) + vim.api.nvim_set_current_win(entry_b.winid) -- WinLeave on entry_a's window captures its own node + vim.api.nvim_win_set_cursor(entry_b.winid, { line_b, 0 }) + vim.api.nvim_set_current_win(entry_a.winid) -- WinLeave on entry_b's window captures its own node + + discussions.rebuild_discussion_tree() + + local _, restored_line_a = discussions.discussion_tree:get_node("-disc-a") + local _, restored_line_b = discussions.discussion_tree:get_node("-disc-b") + + assert.are.equal(restored_line_a, vim.api.nvim_win_get_cursor(entry_a.winid)[1]) + assert.are.equal(restored_line_b, vim.api.nvim_win_get_cursor(entry_b.winid)[1]) + end) + end) end) diff --git a/tests/spec/reviewer_close_session_spec.lua b/tests/spec/reviewer_close_session_spec.lua new file mode 100644 index 00000000..0c31b4fc --- /dev/null +++ b/tests/spec/reviewer_close_session_spec.lua @@ -0,0 +1,78 @@ +-- reviewer.close_session is the teardown variant: on top of the narrow reviewer.close, it +-- must also close the commit-browser tab and forget history_tabid, and unmount any +-- discussion window left registered elsewhere. + +describe("reviewer.close_session", function() + local reviewer = require("gitlab.reviewer") + local windows = require("gitlab.actions.discussions.windows") + local Split = require("nui.split") + + after_each(function() + reviewer.tabid = nil + reviewer.history_tabid = nil + vim.cmd("tabnew") + vim.cmd("silent! tabonly") + end) + + it( + "Closes the reviewer tab and the commit-browser tab, clears history_tabid, and unmounts a stray discussion window", + function() + vim.cmd("tabnew") + local stray_tab = vim.api.nvim_get_current_tabpage() + local stray_split = Split({ relative = "editor", position = "right", size = "20%" }) + stray_split:mount() + windows.set(stray_tab, { split = stray_split, winid = stray_split.winid, bufnr = vim.api.nvim_get_current_buf() }) + + vim.cmd("tabnew") + reviewer.history_tabid = vim.api.nvim_get_current_tabpage() + local browser_tab = reviewer.history_tabid + + vim.cmd("tabnew") + reviewer.tabid = vim.api.nvim_get_current_tabpage() + local reviewer_split = Split({ relative = "editor", position = "right", size = "20%" }) + reviewer_split:mount() + windows.set( + reviewer.tabid, + { split = reviewer_split, winid = reviewer_split.winid, bufnr = vim.api.nvim_get_current_buf() } + ) + local reviewer_tab = reviewer.tabid + + reviewer.close_session() + + assert.is_false(vim.api.nvim_tabpage_is_valid(reviewer_tab)) + assert.is_false(vim.api.nvim_tabpage_is_valid(browser_tab)) + assert.is_nil(reviewer.history_tabid) + assert.is_nil(windows.get(stray_tab)) + -- the stray tab itself is not a browser/reviewer tab, so close_all only unmounts its + -- window, it doesn't close the tab. + assert.is_true(vim.api.nvim_tabpage_is_valid(stray_tab)) + end + ) + + it("Leaves history_tabid untouched when there is no commit-browser tab", function() + reviewer.history_tabid = nil + reviewer.tabid = nil + + reviewer.close_session() + + assert.is_nil(reviewer.history_tabid) + end) + + it( + "Still clears history_tabid and unmounts stray windows when the reviewer tab fails to close (last tabpage)", + function() + vim.cmd("silent! tabonly") + reviewer.tabid = vim.api.nvim_get_current_tabpage() + reviewer.history_tabid = nil + local split = Split({ relative = "editor", position = "right", size = "20%" }) + split:mount() + windows.set(reviewer.tabid, { split = split, winid = split.winid, bufnr = vim.api.nvim_get_current_buf() }) + + assert.has_no.errors(function() + reviewer.close_session() + end) + + assert.is_nil(windows.get(reviewer.tabid)) + end + ) +end) diff --git a/tests/spec/reviewer_close_spec.lua b/tests/spec/reviewer_close_spec.lua new file mode 100644 index 00000000..013d9850 --- /dev/null +++ b/tests/spec/reviewer_close_spec.lua @@ -0,0 +1,67 @@ +-- reviewer.close is the narrow variant: it must close only the reviewer's own tabpage +-- (and the discussion window registered there), leaving any other tab (e.g. the commit +-- browser) and its discussion window untouched. M.reload relies on that to reopen the +-- same MR without tearing down state that belongs to it. + +describe("reviewer.close", function() + local reviewer = require("gitlab.reviewer") + local windows = require("gitlab.actions.discussions.windows") + local Split = require("nui.split") + + after_each(function() + reviewer.tabid = nil + vim.cmd("tabnew") + vim.cmd("silent! tabonly") + end) + + it("Closes the reviewer tab and unmounts its discussion window, leaving another tab's window standing", function() + vim.cmd("tabnew") + local other_tab = vim.api.nvim_get_current_tabpage() + local other_split = Split({ relative = "editor", position = "right", size = "20%" }) + other_split:mount() + windows.set(other_tab, { split = other_split, winid = other_split.winid, bufnr = vim.api.nvim_get_current_buf() }) + + vim.cmd("tabnew") + reviewer.tabid = vim.api.nvim_get_current_tabpage() + local reviewer_split = Split({ relative = "editor", position = "right", size = "20%" }) + reviewer_split:mount() + windows.set( + reviewer.tabid, + { split = reviewer_split, winid = reviewer_split.winid, bufnr = vim.api.nvim_get_current_buf() } + ) + local reviewer_tab = reviewer.tabid + + reviewer.close() + + assert.is_false(vim.api.nvim_tabpage_is_valid(reviewer_tab)) + assert.is_nil(windows.get(reviewer_tab)) + assert.is_not_nil(windows.get(other_tab)) + + windows.remove(other_tab) + end) + + it("Does nothing when there is no reviewer tabpage", function() + reviewer.tabid = nil + assert.has_no.errors(function() + reviewer.close() + end) + end) + + it("Does not error when the reviewer tab is the only tabpage (tabclose can't close it)", function() + vim.cmd("silent! tabonly") + reviewer.tabid = vim.api.nvim_get_current_tabpage() + local split = Split({ relative = "editor", position = "right", size = "20%" }) + split:mount() + windows.set(reviewer.tabid, { split = split, winid = split.winid, bufnr = vim.api.nvim_get_current_buf() }) + local reviewer_tab = reviewer.tabid + + assert.has_no.errors(function() + reviewer.close() + end) + + -- tabclose failed (FIXME, pre-existing), so the tab is still there, but its discussion + -- window was unmounted regardless. + assert.is_true(vim.api.nvim_tabpage_is_valid(reviewer_tab)) + assert.is_nil(windows.get(reviewer_tab)) + end) +end) diff --git a/tests/spec/reviewer_diffview_closed_spec.lua b/tests/spec/reviewer_diffview_closed_spec.lua new file mode 100644 index 00000000..627997d4 --- /dev/null +++ b/tests/spec/reviewer_diffview_closed_spec.lua @@ -0,0 +1,40 @@ +-- A discussion window can be registered in a tab other than the reviewer's own (e.g. the +-- commit browser), so the winbar timer must only stop once none are left anywhere. + +describe("reviewer.on_diffview_closed", function() + local reviewer = require("gitlab.reviewer") + local windows = require("gitlab.actions.discussions.windows") + local winbar = require("gitlab.actions.discussions.winbar") + + after_each(function() + winbar.cleanup_timer() + reviewer.tabid = nil + vim.cmd("silent! tabonly") + end) + + it("Stops the winbar timer when no discussion window is registered anywhere", function() + reviewer.tabid = 99 + winbar.start_timer() + + reviewer.on_diffview_closed({ tabpage = 99 }) + + assert.is_nil(reviewer.tabid) + assert.is_nil(winbar.timer) + end) + + it("Keeps the winbar timer running while a discussion window is still registered in another tab", function() + vim.cmd("tabnew") + local other_tab = vim.api.nvim_get_current_tabpage() + windows.set(other_tab, { winid = vim.api.nvim_get_current_win(), bufnr = vim.api.nvim_get_current_buf() }) + + reviewer.tabid = 99 + winbar.start_timer() + + reviewer.on_diffview_closed({ tabpage = 99 }) + + assert.is_nil(reviewer.tabid) + assert.is_not_nil(winbar.timer) + + windows.remove(other_tab) + end) +end) diff --git a/tests/spec/reviewer_reset_discussions_spec.lua b/tests/spec/reviewer_reset_discussions_spec.lua new file mode 100644 index 00000000..96d0618a --- /dev/null +++ b/tests/spec/reviewer_reset_discussions_spec.lua @@ -0,0 +1,39 @@ +-- Discussion windows can be registered in tabs other than the one being freshly reviewed +-- (e.g. a commit browser left open from a previous MR), so auto-opening discussions for a +-- (re-)opened review must tear all of them down, not just the current tab's. + +describe("reviewer.reset_discussions_for_auto_open", function() + local reviewer = require("gitlab.reviewer") + local windows = require("gitlab.actions.discussions.windows") + local gitlab = require("gitlab") + local original_toggle_discussions + + before_each(function() + original_toggle_discussions = gitlab.toggle_discussions + -- Only the window teardown is under test here; toggle_discussions fetches from the Go + -- server to reopen, which these tests aren't exercising and have no server to talk to. + gitlab.toggle_discussions = function() end + end) + + after_each(function() + gitlab.toggle_discussions = original_toggle_discussions + vim.cmd("silent! tabonly") + end) + + it("Closes a discussion window left registered in another tab", function() + vim.cmd("tabnew") + local other_tab = vim.api.nvim_get_current_tabpage() + local split = require("nui.split")({ relative = "editor", position = "right", size = "20%" }) + split:mount() + windows.set( + other_tab, + { split = split, winid = split.winid, bufnr = vim.api.nvim_get_current_buf(), view_type = "discussions" } + ) + + vim.cmd("tabnew") -- the reviewer's own, freshly-opened tab + + reviewer.reset_discussions_for_auto_open() + + assert.is_nil(windows.get(other_tab)) + end) +end) From 1bfe43da34b54ddcfca171bbac86674559358c8f 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 19:37:31 +0200 Subject: [PATCH 8/9] refactor: flatten the jump to the discussion tree --- lua/gitlab/actions/discussions/init.lua | 97 +++++----- tests/spec/discussions_move_to_tree_spec.lua | 183 +++++++++++++++++++ 2 files changed, 239 insertions(+), 41 deletions(-) create mode 100644 tests/spec/discussions_move_to_tree_spec.lua diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index f902ad70..3f5749ff 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -289,70 +289,85 @@ M.close_all = function() winbar.cleanup_timer() end +---Expand the discussion in the tree and put the cursor on it, in the discussion window of +---`tabid`. +---@param tabid integer +---@param discussion_id string +local function reveal_discussion(tabid, discussion_id) + local discussion_node, line_number = M.discussion_tree:get_node("-" .. discussion_id) + if discussion_node == nil or next(discussion_node) == nil then + u.notify("Discussion not found", vim.log.levels.WARN) + return + end + if not discussion_node:is_expanded() then + for _, child in ipairs(discussion_node:get_child_ids()) do + M.discussion_tree:get_node(child):expand() + end + discussion_node:expand() + end + M.discussion_tree:render() + + local entry = windows.get(tabid) + if entry == nil then + u.notify("Discussion tree window not found", vim.log.levels.WARN) + return + end + vim.api.nvim_set_current_win(entry.winid) + M.switch_view_type("discussions") + vim.api.nvim_win_set_cursor(entry.winid, { line_number, 0 }) +end + +---Move the cursor to the position the discussion window of `tabid` was last left at, +---without selecting a discussion. +---@param tabid integer +---@return boolean True if the tab has a discussion window to jump to +local function jump_to_last_position(tabid) + local entry = windows.get(tabid) + if entry == nil then + return false + end + vim.api.nvim_win_set_cursor(entry.winid, { entry.last_row, entry.last_column }) + vim.api.nvim_set_current_win(entry.winid) + return true +end + ---Move to the discussion tree at the discussion from diagnostic on current line. M.move_to_discussion_tree = function() local tabid = vim.api.nvim_get_current_tabpage() local current_line = vim.api.nvim_win_get_cursor(0)[1] local d = vim.diagnostic.get(0, { namespace = diagnostics.diagnostics_namespace, lnum = current_line - 1 }) - ---Function used to jump to the discussion tree after the menu selection. - local jump_after_menu_selection = function(diagnostic) - ---Function used to jump to the discussion tree after the discussion tree is opened. - local jump_after_tree_opened = function() - -- All diagnostics in `diagnotics_namespace` have diagnostic_id - local discussion_id = diagnostic.user_data.discussion_id - local discussion_node, line_number = M.discussion_tree:get_node("-" .. discussion_id) - if discussion_node == nil or next(discussion_node) == nil then - u.notify("Discussion not found", vim.log.levels.WARN) - return - end - if not discussion_node:is_expanded() then - for _, child in ipairs(discussion_node:get_child_ids()) do - M.discussion_tree:get_node(child):expand() - end - discussion_node:expand() - end - M.discussion_tree:render() - local entry = windows.get(tabid) - if entry then - vim.api.nvim_set_current_win(entry.winid) - M.switch_view_type("discussions") - vim.api.nvim_win_set_cursor(entry.winid, { line_number, 0 }) - else - u.notify("Discussion tree window not found", vim.log.levels.WARN) - end + ---Jump to the discussion the diagnostic was created for, opening the tree if needed. + ---@param diagnostic vim.Diagnostic + local jump_to = function(diagnostic) + -- All diagnostics in `diagnostics_namespace` have a discussion_id + local reveal = function() + reveal_discussion(tabid, diagnostic.user_data.discussion_id) end - if windows.get(tabid) == nil then - M.open(jump_after_tree_opened, "discussions") + M.open(reveal, "discussions") else - jump_after_tree_opened() + reveal() end end if #d == 0 then - local entry = windows.get(tabid) - if state.settings.reviewer_settings.jump_with_no_diagnostics and entry then - vim.api.nvim_win_set_cursor(entry.winid, { entry.last_row, entry.last_column }) - vim.api.nvim_set_current_win(entry.winid) - else + if not (state.settings.reviewer_settings.jump_with_no_diagnostics and jump_to_last_position(tabid)) then u.notify("No diagnostics for this line.", vim.log.levels.WARN) end - return - elseif #d > 1 then + elseif #d == 1 then + jump_to(d[1]) + else vim.ui.select(d, { prompt = "Choose discussion to jump to", format_item = function(diagnostic) return diagnostic.message end, }, function(diagnostic) - if not diagnostic then - return + if diagnostic ~= nil then + jump_to(diagnostic) end - jump_after_menu_selection(diagnostic) end) - else - jump_after_menu_selection(d[1]) end end diff --git a/tests/spec/discussions_move_to_tree_spec.lua b/tests/spec/discussions_move_to_tree_spec.lua new file mode 100644 index 00000000..4397dd7d --- /dev/null +++ b/tests/spec/discussions_move_to_tree_spec.lua @@ -0,0 +1,183 @@ +local discussions = require("gitlab.actions.discussions") +local windows = require("gitlab.actions.discussions.windows") +local draft_notes = require("gitlab.actions.draft_notes") +local winbar = require("gitlab.actions.discussions.winbar") +local diagnostics = require("gitlab.indicators.diagnostics") +local state = require("gitlab.state") +local u = require("gitlab.utils") + +---@param id string +---@param note_id integer +local function make_discussion(id, note_id) + return { + id = id, + individual_note = false, + notes = { + { + id = note_id, + author = { username = "author" }, + body = "Body of " .. id, + created_at = "2023-10-28T18:27:34.082Z", + position = vim.NIL, + resolvable = false, + resolved = false, + }, + }, + } +end + +---Open a tab with a diff buffer and the discussion tree, cursor in the diff buffer. +---@return integer tabid, integer diff_winid, integer diff_bufnr +local function open_tab_with_tree() + vim.cmd("tabnew") + local tabid = vim.api.nvim_get_current_tabpage() + local diff_winid = vim.api.nvim_get_current_win() + local diff_bufnr = vim.api.nvim_create_buf(false, true) + vim.api.nvim_buf_set_lines(diff_bufnr, 0, -1, false, { "one", "two", "three" }) + vim.api.nvim_win_set_buf(diff_winid, diff_bufnr) + discussions.open() + vim.api.nvim_set_current_win(diff_winid) + return tabid, diff_winid, diff_bufnr +end + +---Place one diagnostic per discussion id on `lnum` (1-based). +---@param bufnr integer +---@param lnum integer +---@param ids string[] +local function set_diagnostics(bufnr, lnum, ids) + local ds = {} + for _, id in ipairs(ids) do + table.insert(ds, { lnum = lnum - 1, col = 0, message = "Note on " .. id, user_data = { discussion_id = id } }) + end + vim.diagnostic.set(diagnostics.diagnostics_namespace, bufnr, ds) +end + +describe("actions/discussions.move_to_discussion_tree", function() + local original_rebuild_view, original_notify, original_select + local notifications + + before_each(function() + original_rebuild_view = draft_notes.rebuild_view + draft_notes.rebuild_view = function() end + original_notify = u.notify + notifications = {} + u.notify = function(msg) + table.insert(notifications, msg) + end + original_select = vim.ui.select + + state.INFO = { web_url = "https://gitlab.example/-/merge_requests/1" } + state.settings.discussion_tree.tree_type = "simple" + state.DISCUSSION_DATA = { + discussions = { make_discussion("disc-a", 101), make_discussion("disc-b", 102) }, + unlinked_discussions = {}, + emojis = {}, + } + end) + + after_each(function() + draft_notes.rebuild_view = original_rebuild_view + u.notify = original_notify + vim.ui.select = original_select + vim.diagnostic.reset(diagnostics.diagnostics_namespace) + vim.cmd("tabnew") + vim.cmd("silent! tabonly") + winbar.cleanup_timer() + discussions.linked_bufnr = nil + discussions.unlinked_bufnr = nil + discussions.discussion_tree = nil + discussions.unlinked_discussion_tree = nil + state.DISCUSSION_DATA = nil + state.INFO = nil + end) + + it("Puts the cursor on the discussion of the only diagnostic on the line", function() + local tabid, diff_winid, diff_bufnr = open_tab_with_tree() + set_diagnostics(diff_bufnr, 2, { "disc-b" }) + vim.api.nvim_win_set_cursor(diff_winid, { 2, 0 }) + + discussions.move_to_discussion_tree() + + local entry = windows.get(tabid) + local _, line = discussions.discussion_tree:get_node("-disc-b") + assert.are.equal(entry.winid, vim.api.nvim_get_current_win()) + assert.are.equal(line, vim.api.nvim_win_get_cursor(entry.winid)[1]) + end) + + it("Opens the discussion tree first when the tab has none", function() + vim.cmd("tabnew") + local tabid = vim.api.nvim_get_current_tabpage() + local diff_bufnr = vim.api.nvim_create_buf(false, true) + vim.api.nvim_buf_set_lines(diff_bufnr, 0, -1, false, { "one", "two", "three" }) + vim.api.nvim_win_set_buf(0, diff_bufnr) + set_diagnostics(diff_bufnr, 1, { "disc-a" }) + vim.api.nvim_win_set_cursor(0, { 1, 0 }) + + discussions.move_to_discussion_tree() + + local entry = windows.get(tabid) + assert.is_not_nil(entry) + local _, line = discussions.discussion_tree:get_node("-disc-a") + assert.are.equal(entry.winid, vim.api.nvim_get_current_win()) + assert.are.equal(line, vim.api.nvim_win_get_cursor(entry.winid)[1]) + end) + + it("Lets the user choose when the line carries several diagnostics", function() + local tabid, diff_winid, diff_bufnr = open_tab_with_tree() + set_diagnostics(diff_bufnr, 3, { "disc-a", "disc-b" }) + vim.api.nvim_win_set_cursor(diff_winid, { 3, 0 }) + local offered = {} + vim.ui.select = function(items, _, on_choice) + for _, item in ipairs(items) do + table.insert(offered, item.user_data.discussion_id) + end + on_choice(items[2]) + end + + discussions.move_to_discussion_tree() + + assert.are.same({ "disc-a", "disc-b" }, offered) + local entry = windows.get(tabid) + local _, line = discussions.discussion_tree:get_node("-disc-b") + assert.are.equal(line, vim.api.nvim_win_get_cursor(entry.winid)[1]) + end) + + it("Stays put when the user aborts the choice", function() + local _, diff_winid, diff_bufnr = open_tab_with_tree() + set_diagnostics(diff_bufnr, 3, { "disc-a", "disc-b" }) + vim.api.nvim_win_set_cursor(diff_winid, { 3, 0 }) + vim.ui.select = function(_, _, on_choice) + on_choice(nil) + end + + discussions.move_to_discussion_tree() + + assert.are.equal(diff_winid, vim.api.nvim_get_current_win()) + end) + + it("Warns on a line without a diagnostic", function() + state.settings.reviewer_settings.jump_with_no_diagnostics = false + local _, diff_winid, diff_bufnr = open_tab_with_tree() + set_diagnostics(diff_bufnr, 1, { "disc-a" }) + vim.api.nvim_win_set_cursor(diff_winid, { 2, 0 }) + + discussions.move_to_discussion_tree() + + assert.are.equal(diff_winid, vim.api.nvim_get_current_win()) + assert.are.same({ "No diagnostics for this line." }, notifications) + end) + + it("Jumps to the tree window's last position on a line without a diagnostic when enabled", function() + state.settings.reviewer_settings.jump_with_no_diagnostics = true + local tabid, diff_winid = open_tab_with_tree() + local entry = windows.get(tabid) + entry.last_row, entry.last_column = 2, 0 + + vim.api.nvim_win_set_cursor(diff_winid, { 1, 0 }) + discussions.move_to_discussion_tree() + + assert.are.equal(entry.winid, vim.api.nvim_get_current_win()) + assert.are.equal(2, vim.api.nvim_win_get_cursor(entry.winid)[1]) + state.settings.reviewer_settings.jump_with_no_diagnostics = false + end) +end) From 080055483f5f53a05587c9d3c6fcf164d32ada40 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 19:49:04 +0200 Subject: [PATCH 9/9] fix: word the jump warning in domain terms --- lua/gitlab/actions/discussions/init.lua | 2 +- tests/spec/discussions_move_to_tree_spec.lua | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 3f5749ff..9ad4e6df 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -353,7 +353,7 @@ M.move_to_discussion_tree = function() if #d == 0 then if not (state.settings.reviewer_settings.jump_with_no_diagnostics and jump_to_last_position(tabid)) then - u.notify("No diagnostics for this line.", vim.log.levels.WARN) + u.notify("No comment on this line.", vim.log.levels.WARN) end elseif #d == 1 then jump_to(d[1]) diff --git a/tests/spec/discussions_move_to_tree_spec.lua b/tests/spec/discussions_move_to_tree_spec.lua index 4397dd7d..c0ac0473 100644 --- a/tests/spec/discussions_move_to_tree_spec.lua +++ b/tests/spec/discussions_move_to_tree_spec.lua @@ -164,7 +164,7 @@ describe("actions/discussions.move_to_discussion_tree", function() discussions.move_to_discussion_tree() assert.are.equal(diff_winid, vim.api.nvim_get_current_win()) - assert.are.same({ "No diagnostics for this line." }, notifications) + assert.are.same({ "No comment on this line." }, notifications) end) it("Jumps to the tree window's last position on a line without a diagnostic when enabled", function()