Rework close/delete/discard logic and UI, and move out of actions.

Review Request #14654 — Created Oct. 27, 2025 and updated

Information

Review Board
release-7.1.x

Reviewers

Formerly, actions were handling the UI logic for closing and deleting
review requests, and unified banner was handling all discard logic.
There was some overlap in that both were implementing review request
discard logic. And then some of that logic ended up running two
instances of the same navigateTo().

Some of those actions provided confirmation of operations using the old
modalBox, which is a couple generations out of date.

This change reworks all of this to better centralize logic and update
the UI.

ReviewRequestEditorView now has new methods that handle some of this
common logic:

  • closeCompleted()
  • closeDiscarded()
  • deleteReviewRequest().
  • discardDraft()

The actions can call this directly, and no longer need to reimplement
any of the logic. This simplifies them considerably.

UnifiedBannerView also calls out to two of these: closeDiscarded()
and discardDraft(). It no longer has its own confirmation dialog logic
for these.

All operations requiring confirmation now use Ink's new
showConfirmDialog(), and use reworked confirm button labels that
better convey the action being invoked. These new versions keep the
dialog open while the operation is being performed, managing the busy
state on the confirm button.

The post-operation page refreshes/navigation no longer live in any of
these functions. We had code already that handled this as a result of
various signals, and were just missing one for deleting review requests,
which have been added. We also no longer need the closeError signal,
handling error handling instead in the central functions.

There's also a fix for error reporting for closing review requests. When
closing was converted to actions, the triggering of closeError was
performed on the wrong object, meaning we weren't handling it correctly.
This is no longer a problem with the new implementation.

Unit tests pass.

Tested each of the actions and buttons on the Close menu and Unified
Banner, both canceling and performing the action. Checked the message
handling as well.

Summary ID
Rework close/delete/discard logic and UI, and move out of actions.
Formerly, actions were handling the UI logic for closing and deleting review requests, and unified banner was handling all discard logic. There was some overlap in that both were implementing review request discard logic. And then some of that logic ended up running two instances of the same `navigateTo()`. Some of those actions provided confirmation of operations using the old `modalBox`, which is a couple generations out of date. This change reworks all of this to better centralize logic and update the UI. `ReviewRequestEditorView` now has new methods that handle some of this common logic: * `closeCompleted()` * `closeDiscarded()` * `deleteReviewRequest()`. * `discardDraft()` The actions can call this directly, and no longer need to reimplement any of the logic. This simplifies them considerably. `UnifiedBannerView` also calls out to two of these: `closeDiscarded()` and `discardDraft()`. It no longer has its own confirmation dialog logic for these. All operations requiring confirmation now use Ink's new `showConfirmDialog()`, and use reworked confirm button labels that better convey the action being invoked. These new versions keep the dialog open while the operation is being performed, managing the busy state on the confirm button. The post-operation page refreshes/navigation no longer live in any of these functions. We had code already that handled this as a result of various signals, and were just missing one for deleting review requests, which have been added. We also no longer need the `closeError` signal, handling error handling instead in the central functions. There's also a fix for error reporting for closing review requests. When closing was converted to actions, the triggering of `closeError` was performed on the wrong object, meaning we weren't handling it correctly. This is no longer a problem with the new implementation.
5907e27c2c7a7f57a33625465eb38b618e0ac4a7

Description From Last Updated

/r/14651/ types this as Promise<void> and doesn't do anything with the result. Is there an update coming for that change?

daviddavid

It seems like this will end up showing a second modal dialog before the first one closes. What happens in …

daviddavid

It seems like this could be either an Error or a BackboneError based on what failed, so it's probably better …

daviddavid

This should probably also use err.message.

daviddavid

We should probably bail if we hit this case.

daviddavid

It seems from your other change like the confirm dialog should be modal. That means that we shouldn't have been …

daviddavid

The old code used navigateTo to reload the review request after discarding the draft. Do we need to do that …

daviddavid

This should probably also use err.message.

daviddavid

This should probably also use err.message.

daviddavid

Is there a good reason to have the navigateTo() call outside of the confirm handler? I feel like it would …

daviddavid
chipx86
Review request changed
Added Files:
david
  1. 
      
  2. Show all issues

    /r/14651/ types this as Promise<void> and doesn't do anything with the result. Is there an update coming for that change?

  3. Show all issues

    It seems like this will end up showing a second modal dialog before the first one closes. What happens in that case?

  4. Show all issues

    It seems like this could be either an Error or a BackboneError based on what failed, so it's probably better to use err.message here.

  5. Show all issues

    This should probably also use err.message.

  6. Show all issues

    We should probably bail if we hit this case.

  7. Show all issues

    It seems from your other change like the confirm dialog should be modal. That means that we shouldn't have been able to have the draft disappear in between showConfirmDialog() and onConfirm getting called. Can we change this to another assertion?

  8. Show all issues

    The old code used navigateTo to reload the review request after discarding the draft. Do we need to do that here?

  9. Show all issues

    This should probably also use err.message.

  10. Show all issues

    This should probably also use err.message.

  11. reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Is there a good reason to have the navigateTo() call outside of the confirm handler? I feel like it would be nice to just do:

    await pendingReview.destroy();
    RB.navigateTo(reviewRequest.get('reviewURL'));
    
  12.