Rework close/delete/discard logic and UI, and move out of actions.
Review Request #14654 — Created Oct. 27, 2025 and updated
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 samenavigateTo().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.
ReviewRequestEditorViewnow 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 |
|---|---|
| 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? |
|
|
|
It seems like this will end up showing a second modal dialog before the first one closes. What happens in … |
|
|
|
It seems like this could be either an Error or a BackboneError based on what failed, so it's probably better … |
|
|
|
This should probably also use err.message. |
|
|
|
We should probably bail if we hit this case. |
|
|
|
It seems from your other change like the confirm dialog should be modal. That means that we shouldn't have been … |
|
|
|
The old code used navigateTo to reload the review request after discarding the draft. Do we need to do that … |
|
|
|
This should probably also use err.message. |
|
|
|
This should probably also use err.message. |
|
|
|
Is there a good reason to have the navigateTo() call outside of the confirm handler? I feel like it would … |
|
-
-
/r/14651/ types this as
Promise<void>and doesn't do anything with the result. Is there an update coming for that change? -
It seems like this will end up showing a second modal dialog before the first one closes. What happens in that case?
-
It seems like this could be either an
Erroror aBackboneErrorbased on what failed, so it's probably better to useerr.messagehere. -
-
-
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()andonConfirmgetting called. Can we change this to another assertion? -
The old code used
navigateToto reload the review request after discarding the draft. Do we need to do that here? -
-
-
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'));