From bea85a9ee584581e0752d659709cc3e3fba59116 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Wed, 12 Aug 2026 09:42:15 +0200 Subject: [PATCH] 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. --- .../Islands/DetailsIslandViewModel.cs | 68 ++++++- .../Islands/MergeSectionViewModel.cs | 6 + .../Views/Islands/Detail/WorkConsole.axaml | 36 ++-- .../DetailsIslandReviewActionsTests.cs | 185 +++++++++++++++++- 4 files changed, 277 insertions(+), 18 deletions(-) diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 05a18f8d..b8a202d2 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -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 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() { diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs index 6dae9e71..b3953a81 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs @@ -1,6 +1,7 @@ using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using System.Collections.ObjectModel; +using ClaudeDo.Ui.Localization; using ClaudeDo.Ui.Services; using ClaudeDo.Ui.ViewModels.Modals; using Microsoft.Extensions.DependencyInjection; @@ -27,6 +28,10 @@ public sealed partial class MergeSectionViewModel : ViewModelBase [ObservableProperty] private ObservableCollection _mergeTargetBranches = new(); [ObservableProperty] private string? _selectedMergeTarget; + // No CanExecute to gate here — RefreshMergePreviewAsync is triggered from the Bind path, + // not a [RelayCommand], so PreviewOp only drives the indicator, never locks a button. + public OperationStatus PreviewOp { get; } = new(); + [ObservableProperty] [NotifyPropertyChangedFor(nameof(ShowMergePreviewMuted))] private string _mergePreviewText = ""; @@ -118,6 +123,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase } var capturedTaskId = TaskId; var capturedTarget = SelectedMergeTarget; + using var op = PreviewOp.Begin(Loc.T("ops.review.previewing")); var dto = await _worker.PreviewMergeAsync(capturedTaskId, capturedTarget ?? ""); if (TaskId != capturedTaskId || SelectedMergeTarget != capturedTarget) return; var (text, clean, conflict) = MergePreviewPresenter.Describe(dto); diff --git a/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml b/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml index 17811483..8f31a34e 100644 --- a/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml +++ b/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml @@ -2,6 +2,7 @@ xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:vm="using:ClaudeDo.Ui.ViewModels.Islands" xmlns:loc="using:ClaudeDo.Ui.Localization" + xmlns:controls="using:ClaudeDo.Ui.Views.Controls" x:DataType="vm:DetailsIslandViewModel" x:Class="ClaudeDo.Ui.Views.Islands.Detail.WorkConsole"> @@ -263,10 +264,13 @@ ScrollViewer.VerticalScrollBarVisibility="Auto" FontFamily="{StaticResource MonoFont}" FontSize="{StaticResource FontSizeMono}" /> -