Skip to content

feat: implement rebasing - #534

Merged
jakubbortlik merged 1 commit into
harrisoncramer:developfrom
jakubbortlik:feat/implement-rebasing
May 29, 2026
Merged

jakubbortlik merged 1 commit into
harrisoncramer:developfrom
jakubbortlik:feat/implement-rebasing

Conversation

@jakubbortlik

@jakubbortlik jakubbortlik commented Mar 4, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #497.

This is still in draft because I would like to test it thoroughly, but the main functionality works. The maintained Diffview fork supports updating a diffview, and programmatically modifying selections which enables us to update the reviewer in place without closing and reopening after a rebase.

There will be a trivial conflict with #530 because both PR's add keybindings in the same place in settings.

I've also replaced some synchronous system calls to git with async calls. This prevents blocking the UI (cursor and winbar updates) when checking if the local branch is up-to-date with the remote tracking branch. This makes loading the summary and rebuilding the discussion tree more responsive.

AI use: The Go code is mostly generated by Claude Opus 4.5. The same model has been used for reviewing the lua code.

Comment thread lua/gitlab/reviewer/init.lua Resolved
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch from 27e544a to 6fc6475 Compare March 17, 2026 16:20
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch 2 times, most recently from 3384ee1 to d528de8 Compare March 30, 2026 13:12
@jakubbortlik jakubbortlik changed the title Draft: feat: implement rebasing feat: implement rebasing Apr 10, 2026
@jakubbortlik jakubbortlik changed the title feat: implement rebasing Draft: feat: implement rebasing Apr 10, 2026
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch from dff2d2a to 49d2495 Compare April 11, 2026 11:40
Comment thread doc/gitlab.nvim.txt
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
reload_review = "gl<C-R>", -- Load new MR state from Gitlab and apply new diff refs to the diff view

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm wondering whether it would make sense to connect this to the refresh of the discussion tree instead of making it a separate function and keybinding.

Comment thread doc/gitlab.nvim.txt
Comment on lines +865 to 871
gitlab.close_review() ~

Closes the reviewer tab and discussion tree and cleans up (e.g., removes
winbar timer).
>lua
require("gitlab").close_review()
<

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is an existing API that was just missing documentation.

M.is_open = true
local cur_view = diffview_lib.get_current_view()
M.diffview_layout = cur_view.cur_layout
M.diffview = require("diffview.lib").get_current_view()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Saving this to the module table makes it possible to reduce the reliance on the diffview.lib in other places of the codebase.

Comment thread lua/gitlab/reviewer/init.lua Resolved
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch from a9de870 to 9dd7965 Compare April 19, 2026 06:38
@jakubbortlik jakubbortlik changed the title Draft: feat: implement rebasing feat: implement rebasing Apr 20, 2026
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch 2 times, most recently from 906e633 to 5aad115 Compare April 25, 2026 19:18
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch from 5aad115 to 567f8a9 Compare April 29, 2026 10:09
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch from 567f8a9 to 4bd4100 Compare May 22, 2026 05:13
@jakubbortlik

Copy link
Copy Markdown
Collaborator Author

Hi @harrisoncramer, do you think you could have a look at this PR? I think it is ready to be merged and find the functionality really useful, and it would unblock me for working on other issues. When I'm working on a fix or new feature, I frequently just test it during my normal work and while doing so I really would like to have the functionality from this PR and also from #552 available.

@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch 4 times, most recently from 291636a to a231360 Compare May 29, 2026 05:44
- Rebases the MR on the server.
- Pulls the remote branch after rebasing.
- Aborts rebase if it is not necessary, but enables forcing a rebase.
- Aborts rebase when there are conflicts.
- Adds keymaps for common rebase variants, and for reloading reviewer.
- Checks if branch is up-to-date asynchronously.
@jakubbortlik
jakubbortlik force-pushed the feat/implement-rebasing branch from a231360 to b5368b0 Compare May 29, 2026 06:04
@jakubbortlik
jakubbortlik merged commit 76a3abd into harrisoncramer:develop May 29, 2026
6 checks passed
@jakubbortlik
jakubbortlik deleted the feat/implement-rebasing branch May 29, 2026 06:10
harrisoncramer added a commit that referenced this pull request Sep 14, 2026
* feat: Fetch all merge request beyond the default 20 (#549)

* feat(choose_merge_request): add per_page option for merge request fetching

Allow users to configure the number of merge requests fetched via the
`per_page` setting in `choose_merge_request`. Defaults to 100.

* Remove the per_page option and now fetch all merge request internally

---------

Co-authored-by: Clement Hue <clement.hue@thetrainline.com>

* fix: encode empty merge_requests body as JSON object instead of array (#554)

When choose_merge_request() is called without arguments we must pass an empty JSON *object* to the Go server instead of an empty *array*.

* fix(git): handle detached HEAD silently (#557)

Allow reviewing a merge request from detached HEAD.

Co-authored-by: Sebastian Flügge <952313+seflue@users.noreply.github.com>

* Feature merge request beyond 20 (#551)

* feat(choose_merge_request): add per_page option for merge request fetching

Allow users to configure the number of merge requests fetched via the
`per_page` setting in `choose_merge_request`. Defaults to 100.

* Remove the per_page option and now fetch all merge request internally

* Implement merge request pagination using gitlab api

---------

Co-authored-by: Clement Hue <clement.hue@thetrainline.com>

* fix: use correct type annotation for optional parameters (#562)

* fix(server): use vim.system instead of jobstart to bypass shell (#560)

Switch the server launch from vim.fn.jobstart to vim.system, passing the binary and JSON settings as a list. Neovim spawns the process directly instead of going through the user's shell, removing the Windows-specific "-escaping hack that only worked for POSIX-style shells, otherwise it prevented the Go server from starting.

* fix: use light background color for dark labels (#563)

Dark MR labels are hard to distinguish from a dark background. Select a light colorscheme-aware background color for such labels, if the vim.o.background is set to dark.

* feat: implement rebasing (#534)

- Rebases the MR on the server.
- Pulls the remote branch after rebasing.
- Aborts rebase if it is not necessary, but enables forcing a rebase.
- Aborts rebase when there are conflicts.
- Adds keymaps for common rebase variants, and for reloading reviewer.
- Checks if branch is up-to-date asynchronously.

* feat: open discussions immediately load data afterwards (#552)

Main features:
- opens discussion panel immediately and loads data asynchronously
- improves restoring cursor position after rebuilding trees
- uses winfixbuf to disable switching to another buffer in discussion panel
- discussion panel window options do not leak to reviewer
- autocommands are added to clean up state when discussion panel is closed manually

Minor improvements:
- moves view type switching from winbar to discussions
- adds protection from invalid line number
- replaces deprecated vim.api function

* docs: update references to diffview-plus.nvim (#566)

Document that dlyongemallo's fork has been renamed.

Also, add a health check that verifies if the recommended (but not required) diffview.api package is present.

* docs: add missing dependencies and unify quick start (#567)

* refactor: remove dead code related to emojis

* feat: pull emoji data from Gitlab

Instead of using an incomplete local emojis.json list, the plugin now
pulls the emoji data from Gitlab. This makes sure the data is complete
and doesn't contain invalid items, which would cause a Go server error
(e.g., cmd/config/emojis.json contained ⛵ for ⛵).

* fix: only initialize emojis when starting a review

* feat: replace plenary Job with vim.system (#556)

Plenary is not actively maintained and will be archived soon.
Replace plenary.Job with builtin vim.system calls.

* refactor: remove unused utils functions

* refactor: remove some more unused functions (#571)

* fix: only check Go version when building the server (#572)

The go version is now only checked when building the server. When the user provides a server binary via configuration, Go version is not checked.

The Go version and server binary availability checks are moved to the health module to run when starting the plugin.

Co-authored by: jakub.bortlik@proton.me

* fix: set diffview hook for added files (#573)

The hook for setting up keymaps and autocommands didn't fire correctly
if the first file in the review was a new file, so it was diffed against
diffview://null, which caused some autocommands to not fire
appropriately or some state (reviewer.tabid) not to be set correctly.

This PR also removes an unnecessary diagnostics refresh.

* docs: improve type annotations and user documentation (#574)

Adds function annotations throughout the whole codebase to increase maintainability and ease of contribution.

The README.md and doc/gitlab.nvim.txt are updated to correspond to the current recommended installation process.

Removes several unused functions and one whole unused module (hover.lua).

Also adds several TODOs and FIXMEs for issues discovered during the codebase review.

* docs: fix typos (#575)

* fix: use pagination to list project members (#579)

Use `gitlab.Scan` to paginate through all project members.

Co-authored-by: Farsan <farsan.rashid@agoda.com>

* fix: hunk header counts and the deleted-line check (#580)

Commenting on the unchanged line directly above a deletion of exactly one line sent it as an added line, anchored to the new side, and without the warning that comments on unmodified lines are placed in
the old file. It now goes to the old file with both line numbers, the way it already did above a deletion of two or more lines.

The check for deleted lines implemented the general algorithm for arbitrary diffs, including context lines, and its range carried an off-by-one that no caller could reach. The only diff it ever sees comes from
git.diff_files, which runs with --unified=0 and has no context lines, so a line is removed exactly when it falls in the hunk's old range. Simplify to that rather than fix the bound.

* fix: close an orphaned discussion window

`NuiSplit:unmount` sets an internal loading flag before destroying
buffer and window and clears it only at the end, so an error in between
leaves the flag set and every later unmount returns early without
closing anything. `close` marked the split as gone regardless, so the
window can stay on screen while `split_visible` says otherwise and the
next toggle opens a second one beside it. No such failure was observed,
that part is hardening.

Close the window directly when the split does not, and keep
`split_visible` set for as long as the window is alive.

The WinClosed handler defers the teardown to the next tick. A buffer
wiped from inside a WinClosed callback fires no BufWipeout, and the
autocmds that reset `linked_bufnr` and `unlinked_bufnr` hang off that
event, so a synchronous teardown leaves both fields holding the number
of a wiped buffer.

* fix: release the discussion buffers on close

The linked and unlinked buffers are created per open, not per session,
so the pair the closing window leaves behind stays listed forever while
the next open allocates a fresh one: two leaked buffers per open/close
cycle.

* fix: guard get_root_node against a nil parent

A node of type "note" without `is_root` recurses into
`tree:get_node(nil)`, which resolves a node from a window's cursor and
can hand back the very same node. The call is in tail position, so the
recursion never overflows the stack, it freezes Neovim. Give up on a nil
parent, the way get_note_node already does.

* fix: get correct location data for comments efficiently

* docs: replace number with integer in type annotations

* feat: execute a callback on all run_job paths

Previously a transport failure, an empty response body, or unparseable
JSON returned without running any callback, leaving callers unable to
tell that the request had finished. Now exactly one callback runs for
every outcome, including curl failing to spawn.

* refactor: extract anonymous on_exit from run_job

The handler was a nested anonymous function, which obscured the outcome
branches and could not be exercised directly. Move it to M._make_on_exit
and cover the branches with specs.

* refactor: rename job.lua to client.lua

The module sends requests to the Go server rather than managing jobs, so name
it for what it does: run_job becomes send_request, and the spec follows.

* fix: redact request body from notification

Redact the request body from the notifications that echo the curl
command, since it can carry comment and MR text.

* fix: reject response bodies that are not objects

* refactor: rename details to error in ErrorResponse

"error" states what the field holds, which reads better on the Lua side where
it gates the on_error callback. This changes the wire format, so a response
carrying the old key is still accepted, with a warning to rebuild, for users
who supply their own binary via server.binary_provided.

* feat: show time since last update in winbar even when updating

last_updated doubled as the in-flight marker, so the timestamp
disappeared while a refresh ran. Track the two states separately, and
drive the spinner phase from vim.uv.hrtime so it advances with
wall-clock time rather than with the number of winbar renders.

* feat: use counter for updates

A single Boolean `updating` flag stopped the spinner as soon as the
first of two overlapping refreshes finished, suggesting all work was
done. An integer counter updates for each refresh individually, so the
spinner runs until the last one completes.

---------

Co-authored-by: Clement-Hue <47351376+Clement-Hue@users.noreply.github.com>
Co-authored-by: Clement Hue <clement.hue@thetrainline.com>
Co-authored-by: Mathias Eek <51080320+Cliffback@users.noreply.github.com>
Co-authored-by: Sebastian Flügge <seflue@web.de>
Co-authored-by: Sebastian Flügge <952313+seflue@users.noreply.github.com>
Co-authored-by: Jakub F. Bortlík <jakub.bortlik@proton.me>
Co-authored-by: JonLD <73548328+JonLD@users.noreply.github.com>
Co-authored-by: Artem Starikov <contact@snejugal.ru>
Co-authored-by: Farsan Rashid <farsan.rashid@gmail.com>
Co-authored-by: Farsan <farsan.rashid@agoda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve rebasing experience

1 participant