diff --git a/.github/workflows/lua.yaml b/.github/workflows/lua.yaml index 9ecfa4844..0469a81a2 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/README.md b/README.md index ca64337f1..ff16febae 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 | | `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 | @@ -239,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 5922f296b..5e4ccd575 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 cdd880745..e81f511a9 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 05881da25..4d6552acc 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 a10ef5fc0..de76480f0 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 102dcba7d..a036e2a26 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 f92f1570f..f8c3ee1e4 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 1868e5dfc..5bbb4f1a1 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 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 @@ -297,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 @@ -476,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", @@ -915,6 +919,22 @@ 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 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() < *gitlab.nvim.reload_review* gitlab.reload_review() ~ diff --git a/lua/gitlab/actions/comment.lua b/lua/gitlab/actions/comment.lua index 862521afa..601d1235d 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 d328440ad..25373d688 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 @@ -159,6 +196,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 @@ -291,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) @@ -302,13 +344,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 37fe68fc2..9ad4e6dff 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,6 +34,30 @@ local M = { unlinked_discussion_tree = nil, } +---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 }) + 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 + +---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 @@ -106,68 +129,106 @@ 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 ---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), - callback = M.close, + pattern = tostring(split.winid), + callback = function() + 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 @@ -180,73 +241,133 @@ M.open = function(callback, view_type) end end ----Clear the discussion state and unmounts the split. -M.close = function() - if M.split then - M.split:unmount() +---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 + 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, entry.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() + entry.split:unmount() + end) + 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 - M.split_visible = false - M.discussion_tree = nil 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() - 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 }) + ---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 not M.split_visible then - M.open(jump_after_tree_opened, "discussions") + if windows.get(tabid) == nil then + M.open(reveal, "discussions") else - jump_after_tree_opened() + reveal() end 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) - else - u.notify("No diagnostics for this line.", vim.log.levels.WARN) + if not (state.settings.reviewer_settings.jump_with_no_diagnostics and jump_to_last_position(tabid)) then + u.notify("No comment on 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 @@ -258,7 +379,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 @@ -287,7 +408,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 @@ -311,7 +432,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 @@ -361,7 +482,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 @@ -390,7 +511,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 @@ -413,7 +534,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 @@ -474,8 +595,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) @@ -498,7 +629,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 @@ -513,8 +646,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) @@ -537,7 +679,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 @@ -546,47 +690,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. @@ -703,13 +860,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, @@ -723,7 +888,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, @@ -737,7 +906,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, @@ -826,18 +999,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 @@ -878,7 +1058,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 4672b3aca..55276f37d 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, @@ -395,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 @@ -445,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 @@ -476,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 @@ -535,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 f59dbee6a..dbeda6b18 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/discussions/windows.lua b/lua/gitlab/actions/discussions/windows.lua new file mode 100644 index 000000000..9f640bccd --- /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/lua/gitlab/actions/draft_notes/init.lua b/lua/gitlab/actions/draft_notes/init.lua index ec2b2aeca..475bb2987 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) @@ -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/actions/merge.lua b/lua/gitlab/actions/merge.lua index 55e8a4cb4..a5282278c 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 154b5b5bc..1c69b8639 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/annotations.lua b/lua/gitlab/annotations.lua index 7aee76cf5..294dc05c1 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 bf29dec36..66676fd7b 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/emoji.lua b/lua/gitlab/emoji.lua index 9d8e5daa8..14f2d707a 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/indicators/common.lua b/lua/gitlab/indicators/common.lua index 649b13d18..b68dbff6a 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 d4d6b8105..afbda4990 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 a6412a210..c763073c2 100644 --- a/lua/gitlab/init.lua +++ b/lua/gitlab/init.lua @@ -4,7 +4,9 @@ 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 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") @@ -41,6 +43,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 @@ -76,14 +79,23 @@ return { reviewer.reload() end, close_review = function() - reviewer.close() + reviewer.close_session() end, + 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), -- 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/history.lua b/lua/gitlab/reviewer/history.lua new file mode 100644 index 000000000..29e340b93 --- /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 000000000..bbd606dc6 --- /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 000000000..96ac012b7 --- /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 0336bf4bf..8ff366b0b 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -13,10 +13,30 @@ local M = { is_open = false, bufnr = nil, tabid = nil, + history_tabid = nil, stored_win = nil, 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 @@ -67,39 +87,92 @@ 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() end ----Close the reviewer and clean up. +-- 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 + -- 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)) + 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 +---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'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 @@ -366,7 +439,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) @@ -409,7 +482,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 } ) @@ -439,7 +512,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", @@ -494,6 +567,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 f264abb94..d00bc8aad 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", @@ -142,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 = { @@ -305,6 +308,7 @@ M.settings = { file_name = "Normal", resolved = "DiagnosticSignOk", unresolved = "DiagnosticSignWarn", + commit = "DiagnosticSignInfo", draft = "DiffviewReference", draft_mode = "DiagnosticWarn", live_mode = "DiagnosticOk", @@ -344,6 +348,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/comment_spec.lua b/tests/spec/comment_spec.lua new file mode 100644 index 000000000..fd2bd5816 --- /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 000000000..251a871ea --- /dev/null +++ b/tests/spec/commit_comment_jump_spec.lua @@ -0,0 +1,161 @@ +-- 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_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 + originals.notify = require("gitlab.utils").notify + 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 + require("gitlab.utils").notify = originals.notify + end) + + ---@param node table The node the cursor is on + ---@param is_new_sha boolean + local function arrange(node, is_new_sha) + local calls = { reviewer = {}, history = {}, notified = {} } + common.get_current_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 + end + + it("Sends a commit-anchored comment to the commit browser", function() + local calls = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, true) + + 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 = arrange({ is_root = true, type = "note", file_name = "file.lua", old_file_name = "file.lua" }, true) + + 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 = arrange({ is_root = true, type = "note", file_name = "file.lua", commit_id = "abc123" }, false) + + common.jump_to_reviewer({}) + + assert.are.same({}, calls.history) + assert.are.same({}, calls.reviewer) + assert.are.equal(1, #calls.notified) + end) +end) diff --git a/tests/spec/common_root_node_spec.lua b/tests/spec/common_root_node_spec.lua new file mode 100644 index 000000000..6d7c69ae0 --- /dev/null +++ b/tests/spec/common_root_node_spec.lua @@ -0,0 +1,25 @@ +-- Without the nil-parent guard this test does not fail, it hangs, and the suite stops here. + +local NuiTree = require("nui.tree") +local common = require("gitlab.actions.common") + +describe("actions/common.get_root_node", function() + it("Gives up on a top level node that is not marked as a root", function() + local bufnr = vim.api.nvim_create_buf(false, true) + local tree = NuiTree({ + bufnr = bufnr, + nodes = { NuiTree.Node({ id = "a", text = "a", type = "note" }) }, + }) + tree:render() + -- The loop only forms if NuiTree can answer `get_node(nil)`, which it does from the + -- cursor of a window showing the buffer. + vim.api.nvim_win_set_buf(0, bufnr) + + assert.is_nil( + common.get_root_node(tree, tree:get_node("-a")), + "get_root_node claimed a root for a note node that has no parent" + ) + + vim.api.nvim_buf_delete(bufnr, { force = true }) + end) +end) diff --git a/tests/spec/discussions_commit_marker_spec.lua b/tests/spec/discussions_commit_marker_spec.lua new file mode 100644 index 000000000..4175f179e --- /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/discussions_current_node_spec.lua b/tests/spec/discussions_current_node_spec.lua new file mode 100644 index 000000000..3a0ca0e8f --- /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_move_to_tree_spec.lua b/tests/spec/discussions_move_to_tree_spec.lua new file mode 100644 index 000000000..c0ac04737 --- /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 comment on 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) diff --git a/tests/spec/discussions_orphan_window_spec.lua b/tests/spec/discussions_orphan_window_spec.lua new file mode 100644 index 000000000..19c8533de --- /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 the registry says so. + +local discussions = require("gitlab.actions.discussions") +local draft_notes = require("gitlab.actions.draft_notes") +local windows = require("gitlab.actions.discussions.windows") + +---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() + 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.linked_bufnr = 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, tabid = arrange(function() end) + + discussions.close() + + assert.is_false(vim.api.nvim_win_is_valid(winid), ("window %d survived close()"):format(winid)) + assert.is_nil(windows.get(tabid)) + end) + + it("Closes the window itself when unmounting raises", 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_nil(windows.get(tabid)) + end) + + it("Closes the window when it is the last one in the session", function() + 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 bufnr = vim.api.nvim_create_buf(true, false) + vim.api.nvim_win_set_buf(winid, bufnr) + 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_nil(windows.get(tabid)) + 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 = 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) + + -- 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) + 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 new file mode 100644 index 000000000..13fb22514 --- /dev/null +++ b/tests/spec/discussions_shared_bufs_spec.lua @@ -0,0 +1,101 @@ +-- 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") + +-- 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.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) + + 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 8f2c04166..bc5b6d082 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/discussions_windows_spec.lua b/tests/spec/discussions_windows_spec.lua new file mode 100644 index 000000000..c9633ac57 --- /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) diff --git a/tests/spec/history_browse_commits_spec.lua b/tests/spec/history_browse_commits_spec.lua new file mode 100644 index 000000000..983e0266b --- /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/history_comment_spec.lua b/tests/spec/history_comment_spec.lua new file mode 100644 index 000000000..04c95dbd5 --- /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 000000000..eaeb18a6e --- /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 000000000..f9ed9c950 --- /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 000000000..c7c527af9 --- /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 000000000..00d4ef392 --- /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 000000000..66cb9b338 --- /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 000000000..1cc1a0c99 --- /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 000000000..8d3d8d3c2 --- /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) diff --git a/tests/spec/reviewer_autocommands_spec.lua b/tests/spec/reviewer_autocommands_spec.lua new file mode 100644 index 000000000..64275449e --- /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) diff --git a/tests/spec/reviewer_close_session_spec.lua b/tests/spec/reviewer_close_session_spec.lua new file mode 100644 index 000000000..0c31b4fc0 --- /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 000000000..013d98500 --- /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 000000000..627997d4a --- /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 000000000..96d0618a8 --- /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)