feat(ui): route detail-pane review actions through OperationStatus
Approve, Submit, Reject, and Park each get their own OperationStatus so the button locks and shows an OperationIndicator for the duration of the call; Approve additionally scopes a MergeProgressEvent subscription to its own task id to sharpen the label from "merging" to "verifying" mid-flight. MergeSectionViewModel's preview refresh gets the same treatment for display only, since it has no command button to gate.
This commit is contained in:
@@ -299,7 +299,12 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
[NotifyCanExecuteChangedFor(nameof(RejectReviewCommand))]
|
||||
private string _reviewFeedback = "";
|
||||
|
||||
public bool HasReviewFeedback => !string.IsNullOrWhiteSpace(ReviewFeedback);
|
||||
public bool HasReviewFeedback => !string.IsNullOrWhiteSpace(ReviewFeedback) && !RejectOp.IsRunning;
|
||||
|
||||
public OperationStatus ApproveOp { get; } = new();
|
||||
public OperationStatus SubmitOp { get; } = new();
|
||||
public OperationStatus RejectOp { get; } = new();
|
||||
public OperationStatus ParkOp { get; } = new();
|
||||
|
||||
public DetailsIslandViewModel(
|
||||
IDbContextFactory<ClaudeDoDbContext> dbFactory,
|
||||
@@ -330,6 +335,30 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
};
|
||||
Prep = new PrepPanelViewModel(worker);
|
||||
|
||||
// OperationStatus.IsRunning is a nested ObservableObject property, so
|
||||
// [NotifyCanExecuteChangedFor] on the command can't see it -- without this, the
|
||||
// button stays clickable for the whole duration of the call.
|
||||
ApproveOp.PropertyChanged += (_, e) =>
|
||||
{
|
||||
if (e.PropertyName == nameof(OperationStatus.IsRunning)) ApproveReviewCommand.NotifyCanExecuteChanged();
|
||||
};
|
||||
SubmitOp.PropertyChanged += (_, e) =>
|
||||
{
|
||||
if (e.PropertyName == nameof(OperationStatus.IsRunning)) SubmitForReviewCommand.NotifyCanExecuteChanged();
|
||||
};
|
||||
RejectOp.PropertyChanged += (_, e) =>
|
||||
{
|
||||
if (e.PropertyName == nameof(OperationStatus.IsRunning))
|
||||
{
|
||||
OnPropertyChanged(nameof(HasReviewFeedback));
|
||||
RejectReviewCommand.NotifyCanExecuteChanged();
|
||||
}
|
||||
};
|
||||
ParkOp.PropertyChanged += (_, e) =>
|
||||
{
|
||||
if (e.PropertyName == nameof(OperationStatus.IsRunning)) ParkReviewCommand.NotifyCanExecuteChanged();
|
||||
};
|
||||
|
||||
Notes = new NotesEditorViewModel(_notesApi);
|
||||
Notes.ErrorReported += msg => { if (ShowErrorAsync is not null) _ = ShowErrorAsync(msg); };
|
||||
Subtasks.CollectionChanged += (_, _) => NotifyStepsChanged();
|
||||
@@ -1218,12 +1247,22 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
private async System.Threading.Tasks.Task ApproveReviewAsync()
|
||||
{
|
||||
if (Task is null || !_worker.IsConnected) return;
|
||||
var taskId = Task.Id;
|
||||
using var op = ApproveOp.Begin(Loc.T("ops.merge.merging"));
|
||||
// Subscribed only for the duration of the call: the worker broadcasts to every
|
||||
// client, and a VM left on this event would keep reacting to merges for other tasks.
|
||||
void OnMergeProgress(string progressTaskId, string phase, int elapsedSeconds)
|
||||
{
|
||||
if (progressTaskId != taskId || phase != MergePhaseVerifying) return;
|
||||
ApproveOp.Report(Loc.T("ops.merge.verifying", FormatElapsed(elapsedSeconds)));
|
||||
}
|
||||
_worker.MergeProgressEvent += OnMergeProgress;
|
||||
try
|
||||
{
|
||||
var hasChildren = Subtasks.Count > 0 || ChildOutcomes.Count > 0;
|
||||
var result = await _worker.ApproveReviewAsync(Task.Id, Merge.SelectedMergeTarget ?? "");
|
||||
var result = await _worker.ApproveReviewAsync(taskId, Merge.SelectedMergeTarget ?? "");
|
||||
if (!hasChildren && result?.Status == "conflict")
|
||||
await _merge.ResolveConflictAsync(Task.Id, Merge.SelectedMergeTarget ?? "");
|
||||
await _merge.ResolveConflictAsync(taskId, Merge.SelectedMergeTarget ?? "");
|
||||
// The merge itself already landed; the verify command failed, so the task stayed
|
||||
// out of Done. Surface that instead of silently looking like nothing happened.
|
||||
else if (!hasChildren && result?.Status == "verify_failed" && ShowErrorAsync != null)
|
||||
@@ -1238,25 +1277,38 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
if (ShowErrorAsync != null)
|
||||
await ShowErrorAsync(ex.Message);
|
||||
}
|
||||
finally
|
||||
{
|
||||
_worker.MergeProgressEvent -= OnMergeProgress;
|
||||
}
|
||||
}
|
||||
|
||||
/// Mirrors TaskMergeService.PhaseVerifying — a hub payload token, not a display string.
|
||||
private const string MergePhaseVerifying = "verifying";
|
||||
|
||||
private static string FormatElapsed(int seconds) =>
|
||||
TimeSpan.FromSeconds(Math.Max(0, seconds)).ToString(@"mm\:ss");
|
||||
|
||||
// Force the diff to have been opened before a merge can happen — but only when
|
||||
// there is actually something to inspect (a childless sandbox run with no worktree
|
||||
// has no diff, so it approves straight through).
|
||||
private bool CanApproveReview() =>
|
||||
Task != null && _worker.IsConnected && IsWaitingForReview
|
||||
&& (!Merge.HasReviewableDiff || ReviewDiffViewed);
|
||||
&& (!Merge.HasReviewableDiff || ReviewDiffViewed)
|
||||
&& !ApproveOp.IsRunning;
|
||||
|
||||
// An interactive (ConPTY) session leaves its worktree changed but never flips the task
|
||||
// status. Offer "Submit for review" for an Idle/Failed task that still has a worktree, so
|
||||
// its hand-driven work can enter the normal review/merge flow (commits first, server-side).
|
||||
public bool CanSubmitForReview =>
|
||||
Task != null && _worker.IsConnected && (IsIdle || IsFailed) && !string.IsNullOrEmpty(WorktreePath);
|
||||
Task != null && _worker.IsConnected && (IsIdle || IsFailed) && !string.IsNullOrEmpty(WorktreePath)
|
||||
&& !SubmitOp.IsRunning;
|
||||
|
||||
[RelayCommand(CanExecute = nameof(CanSubmitForReview))]
|
||||
private async System.Threading.Tasks.Task SubmitForReviewAsync()
|
||||
{
|
||||
if (Task is null || !_worker.IsConnected) return;
|
||||
using var op = SubmitOp.Begin(Loc.T("ops.review.submitting"));
|
||||
try { await _worker.SubmitTaskForReviewAsync(Task.Id); }
|
||||
catch (Exception ex)
|
||||
{
|
||||
@@ -1270,20 +1322,24 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
if (Task is null || !_worker.IsConnected) return;
|
||||
var feedback = ReviewFeedback;
|
||||
if (string.IsNullOrWhiteSpace(feedback)) return;
|
||||
using var op = RejectOp.Begin(Loc.T("ops.review.rejecting"));
|
||||
try { await _worker.RejectReviewToQueueAsync(Task.Id, feedback); }
|
||||
catch { /* stale review action; broadcast reconciles */ return; }
|
||||
ReviewFeedback = "";
|
||||
}
|
||||
|
||||
// Park: set the task aside (back to Idle), keeping its worktree intact.
|
||||
[RelayCommand]
|
||||
[RelayCommand(CanExecute = nameof(CanParkReview))]
|
||||
private async System.Threading.Tasks.Task ParkReviewAsync()
|
||||
{
|
||||
if (Task is null || !_worker.IsConnected) return;
|
||||
using var op = ParkOp.Begin(Loc.T("ops.review.parking"));
|
||||
try { await _worker.RejectReviewToIdleAsync(Task.Id); }
|
||||
catch { /* stale review action; broadcast reconciles */ }
|
||||
}
|
||||
|
||||
private bool CanParkReview() => Task != null && _worker.IsConnected && !ParkOp.IsRunning;
|
||||
|
||||
[RelayCommand]
|
||||
private async System.Threading.Tasks.Task ResetReviewAsync()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user