Skip to content

Conversation

jakubbortlik
Copy link
Collaborator

@jakubbortlik jakubbortlik commented Feb 27, 2025

This PR attempts to fix #425.

By default the gitlab client attempts 5 retries which corresponds to the fact that it bulk publishing drafts "fails" with a server error, it still creates 1+5 copies of the comments.

Apart from finally removing the retry functionality, this MR also introduces the following improves:

  • Check that the local repository is up-to-date with remote before publishing drafts (bulk or single), as I've found that draft publishing can fail when commits have been added to the MR between creating drafts in gitlab.nvim and publishing them (probably just when there is a comment on some code that has changed).
  • Show explanation what to do when publishing drafts fails (but a draft actually has been published)
  • Fetch the remote branch before checking local branch is up-to-date with remote
  • Some refactoring

@jakubbortlik

This comment was marked as outdated.

@jakubbortlik jakubbortlik force-pushed the fix-retries-in-gitlab-client branch from df98a1a to c4ef505 Compare February 28, 2025 23:12
@jakubbortlik jakubbortlik force-pushed the fix-retries-in-gitlab-client branch from 95ba45f to d523ea2 Compare February 28, 2025 23:42
@jakubbortlik jakubbortlik changed the title Fix: Disable retries in gitlab client Fix: Make publishing drafts more robust Feb 28, 2025
@jakubbortlik
Copy link
Collaborator Author

Possible followup: I think it would be useful to do a check_current_branch_up_to_data_on_remote before placing any linked comments/suggestions - this would mean that a git fetch {REMOTE} {BRANCH} would be performed each time, but it would prevent situations when a reviewer is adding comments locally on code that has already changed on Gitlab. This could possibly be configurable (i.e., it would be off by default and it could be enabled in the user configuration).

@harrisoncramer
Copy link
Owner

I've merged this separately from the other PRs as this feels higher risk. I'm going to close the related issue but if there are issues that you notice feel free to drop comments here.

@harrisoncramer harrisoncramer merged commit 6137e58 into harrisoncramer:develop Mar 1, 2025
6 checks passed
harrisoncramer added a commit that referenced this pull request Apr 14, 2025
Fix: Jumping to renamed files (#484)
Fix: Store reviewer data before creating comment popup (#476)
Fix: Make publishing drafts more robust (#483)
Fix: Swap file_name and old_file_name in reviewer data (#485)

---------

Co-authored-by: Jakub F. Bortlík <[email protected]>
@jakubbortlik jakubbortlik deleted the fix-retries-in-gitlab-client branch April 15, 2025 04:07
harrisoncramer added a commit that referenced this pull request Jun 25, 2025
* Fix: Jumping to renamed files (#484)

* fix: prevent "cursor position outside buffer" error

* fix: swap file_name and old_file_name in reviewer data

`old_file_name` is not set to the empty string for un-renamed files anymore, because then we can
remove the empty-line check in `comment_helpers.go` which was used to replace the empty string with
the current file name anyway.

* fix: add old_file_name to discussion root node data

* fix: also consider old_file_name when jumping to the reviewer

This fixes jumping to renamed files, however, may not work for comments that
were created on renamed files with the previous version of `gitlab.nvim` as
that version assigned the `file_name` and `old_file_name` incorrectly.

* refactor: don't shadow variable

* fix: check file_name or old_file_name based on which SHA comment belongs to

* Fix: Store reviewer data before creating comment popup (#476)

* Fix: Make publishing drafts more robust (#483)

* Fix: Swap file_name and old_file_name in reviewer data (#485)

* Feat: Enable toggling date format between relative and absolute (#491)

* Fix: Add opts to help popup (#492)

* Fix: Force start_line for jumping to diagnostic to be inside buffer (#494)

* fix: redefine colors after reloading colorscheme (#500)

* Fix: Use path instead of oldpath as fallback for unrenamed files (#496)

* Fix: Use file_name when old_file_name is not set (#495)

* fix(ci): fix lua tests (#501)

* Proxy Support (#499)

This is a #MINOR release.

---------

Co-authored-by: Jakub F. Bortlík <[email protected]>
Co-authored-by: Jonathan Duck <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants