feat(review): gate Approve & Merge behind opening the diff

Approve & Merge is disabled until the pending changes have been inspected: DetailsIslandViewModel tracks ReviewDiffViewed (reset on task switch and on every state change), MergeSectionViewModel raises DiffViewed when a diff or combined diff is opened and exposes HasReviewableDiff, and a hint sits next to the button. A review with nothing to inspect (sandbox run, no worktree) approves straight through. ClaudeDo-Task: f9809a93
2026-07-24 12:52:57 +02:00
parent 3e9ea3ad58
commit 2aaaa23912
7 changed files with 108 additions and 3 deletions
@@ -36,7 +36,7 @@ Design/     — Tokens.axaml (design tokens; merged before styles) + IslandStyle
 - **IslandsShellViewModel** — root coordinator; owns the three island VMs and the `WorkerClient`, wires cross-island events (selection, notes/prep mode, conflict resolution), owns connection state, the update banner, the inline worker-log strip (clickable → Log Visualizer overlay via `OpenLogVisualizerCommand`; `FlashFooterError` surfaces UI-action failures + the worker's Serilog Warn/Error there), responsive-layout flags (`ShowLists`/`ShowDetails` by window width), `PrimeStatus` flash, and the modal openers (About, RepoImport, WeeklyReport, WorktreesOverview, WorkerConnection help, LogVisualizer) plus `RestartWorkerAsync`/`CheckForUpdatesAsync`. Hosts `UpdateCheckService`.
 - **ListsIslandViewModel** — smart lists (My Day, Important, Planned, virtual queued/running/review), user lists, selection, list CRUD, drag-reorder, badge counts, opens list settings / repo import / worktrees overview, `OpenInExplorer`/`OpenInTerminal`.
 - **TasksIslandViewModel** — open/overdue/completed groups for the selected list with hierarchy-aware regrouping; task CRUD, drag-reorder, toggle done/star, schedule, enqueue/dequeue, cancel; review actions (approve, reject-rerun, reject-park, cancel); planning session lifecycle (open/resume/discard/finalize, `QueuePlanningSubtasksAsync`); `RefineTask`, `OpenConPtySessionRequested` (embedded ConPTY terminal), `PickUpInTerminalAsync`; MyDay extras (`IsMyDayList`, `ClearDayCommand`, `ShowPrepLogCommand`) and the pinned Notes pseudo-row (`ShowNotesRow`, `OpenNotesCommand`). Raises `NotesRequested`/`PrepRequested` events consumed by the shell.
- **DetailsIslandViewModel** — the detail pane for a bound `TaskRowViewModel`. Owns live-log streaming (`Log` via `StreamLineFormatter`), debounced title/description editing, subtasks, session-outcome/roadblock split (splits `Result` at the roadblock marker into two cards), the three-tab work console (`output`/`git`/`session`), child surfacing (`ChildOutcomes` rows plus `ChildrenNeedingAttention`/`HasChildrenNeedingAttention` — children that failed, were cancelled, await review, or reported roadblocks — drive an attention band on the Session tab, which is only visible when `HasChildOutcomes`), and the modes: `IsNotesMode` (hosts `NotesEditorViewModel`), `IsPrepMode`, computed `IsTaskDetailVisible = !IsNotesMode && !IsPrepMode`. Three concerns are extracted into section VMs exposed as properties: **AgentConfigEditorViewModel** (scope=Task; per-task Model/MaxTurns/AgentPath overrides with `InheritedBadge` + `InheritanceResolver`, additive SystemPrompt, debounced auto-save; exposed as `AgentSettings`), **MergeSectionViewModel** (merge-target selection, mergeability indicator via `MergePreviewPresenter` over `PreviewMergeAsync`, `OpenDiffAsync` and `ReviewCombinedDiffCommand` — both build a `DiffViewerViewModel` and call `ShowDiffViewer`), **PrepPanelViewModel** (daily-prep panel: `PrepLog`, `PlanDayCommand` → `RunDailyPrepNowAsync`, persisted last run via `GetLastPrepLogAsync`). Attachments: `Attachments` (`ObservableCollection<AttachmentRowViewModel>`), `IsDragOver`, `DropStatus`, `CanAcceptDrop`, `AddFilesAsync`, `RemoveAttachmentCommand`; loads on task change; `ComposedPreview` includes attachment paths. Writes directly via `new AttachmentStore()` + `new TaskAttachmentRepository(ctx)`. Helper rows (`ChildOutcomeRowViewModel`, `SubtaskRowViewModel`, `LogLineViewModel`, `AttachmentRowViewModel`) live in the same file.
+- **DetailsIslandViewModel** — the detail pane for a bound `TaskRowViewModel`. Owns live-log streaming (`Log` via `StreamLineFormatter`), debounced title/description editing, subtasks, session-outcome/roadblock split (splits `Result` at the roadblock marker into two cards), the three-tab work console (`output`/`git`/`session`), child surfacing (`ChildOutcomes` rows plus `ChildrenNeedingAttention`/`HasChildrenNeedingAttention` — children that failed, were cancelled, await review, or reported roadblocks — drive an attention band on the Session tab, which is only visible when `HasChildOutcomes`), and the modes: `IsNotesMode` (hosts `NotesEditorViewModel`), `IsPrepMode`, computed `IsTaskDetailVisible = !IsNotesMode && !IsPrepMode`. Three concerns are extracted into section VMs exposed as properties: **AgentConfigEditorViewModel** (scope=Task; per-task Model/MaxTurns/AgentPath overrides with `InheritedBadge` + `InheritanceResolver`, additive SystemPrompt, debounced auto-save; exposed as `AgentSettings`), **MergeSectionViewModel** (merge-target selection, mergeability indicator via `MergePreviewPresenter` over `PreviewMergeAsync`, `OpenDiffAsync` and `ReviewCombinedDiffCommand` — both build a `DiffViewerViewModel`, call `ShowDiffViewer`, and fire the `DiffViewed` callback; `HasReviewableDiff` reports whether anything is inspectable, feeding the review gate), **PrepPanelViewModel** (daily-prep panel: `PrepLog`, `PlanDayCommand` → `RunDailyPrepNowAsync`, persisted last run via `GetLastPrepLogAsync`). Attachments: `Attachments` (`ObservableCollection<AttachmentRowViewModel>`), `IsDragOver`, `DropStatus`, `CanAcceptDrop`, `AddFilesAsync`, `RemoveAttachmentCommand`; loads on task change; `ComposedPreview` includes attachment paths. Writes directly via `new AttachmentStore()` + `new TaskAttachmentRepository(ctx)`. Helper rows (`ChildOutcomeRowViewModel`, `SubtaskRowViewModel`, `LogLineViewModel`, `AttachmentRowViewModel`) live in the same file.
 - **TaskRowViewModel** / **ListNavItemViewModel** — lightweight display VMs (task row: status, planning phase, parent/blocked links, roadblock count, computed `IsDraft`/`IsPlanned`/`IsChild`/`IsPlanningParent`/`CanRefine`; list row: kind Smart/Virtual/User, count, icon/dot keys, drop hints).
 - **NotesEditorViewModel** — day navigator + bullet CRUD for daily notes via `INotesApi`.
 - **Modal VMs** — `SettingsModalViewModel` (four tabs: General, Worktrees, Files prompt-paths, Prime Claude incl. `DailyPrepMaxTasks` + prime-schedule rows), `ListSettingsModalViewModel` (name, working dir, commit type, delete list; hosts shared `AgentConfigEditorViewModel` as `Agent` property (scope=List) — save delegates to `Agent.SaveAsync()`), `RepoImportModalViewModel` (bulk-create lists from git repos found under chosen parents; already-wired repos disabled), `WeeklyReportModalViewModel` (range pickers default "since last standup weekday → today", cached per range, markdown via MarkdownView), `MergeModalViewModel` (single-task merge form, called from the diff modal), `WorktreesOverviewModalViewModel` (global/per-list worktree rows, batch merge + state ops), `UnfinishedPlanningModalViewModel` (Resume/FinalizeNow/Discard for a draft planning session), `WorkerConnectionModalViewModel` (offline help), `AboutModalViewModel`, `LogVisualizerViewModel` (worker logs, last 30 min, all levels + a warn/error-only filter; loads via `GetRecentLogsAsync`).
@@ -1162,6 +1162,13 @@
    <Setter Property="BorderBrush" Value="{StaticResource AccentBrush}" />
    <Setter Property="TextElement.Foreground" Value="{StaticResource TextBrush}" />
  </Style>
+  <!-- Disabled: drop the accent so a gated action (e.g. Approve before the diff is
+       opened) reads clearly as unavailable instead of looking clickable. -->
+  <Style Selector="Button.accent:disabled /template/ ContentPresenter">
+    <Setter Property="Background"  Value="{StaticResource Surface2Brush}" />
+    <Setter Property="BorderBrush" Value="{StaticResource LineBrush}" />
+    <Setter Property="TextElement.Foreground" Value="{StaticResource TextMuteBrush}" />
+  </Style>

  <!-- ============================================================ -->
  <!-- DAY TOGGLE                                                    -->
@@ -296,6 +296,15 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable

        AgentSettings = new AgentConfigEditorViewModel(worker, AgentConfigScope.Task);
        Merge = new MergeSectionViewModel(worker, services);
+        Merge.DiffViewed = () => ReviewDiffViewed = true;
+        Merge.PropertyChanged += (_, e) =>
+        {
+            if (e.PropertyName == nameof(MergeSectionViewModel.HasReviewableDiff))
+            {
+                ApproveReviewCommand.NotifyCanExecuteChanged();
+                OnPropertyChanged(nameof(ShowReviewDiffHint));
+            }
+        };
        Prep = new PrepPanelViewModel(worker);

        Notes = new NotesEditorViewModel(_notesApi);
@@ -399,6 +408,11 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
                DequeueCommand.NotifyCanExecuteChanged();
                ResetAndRetryCommand.NotifyCanExecuteChanged();
                ContinueCommand.NotifyCanExecuteChanged();
+                // A state change means a new run/review cycle: the diff must be
+                // re-inspected before merge can be approved again.
+                ReviewDiffViewed = false;
+                ApproveReviewCommand.NotifyCanExecuteChanged();
+                OnPropertyChanged(nameof(ShowReviewDiffHint));
                AgentSettings.IsRunning = IsRunning;
                NotifySessionSections();
                OnPropertyChanged(nameof(CanAcceptDrop));
@@ -752,6 +766,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable

    partial void OnTaskChanged(TaskRowViewModel? value)
    {
+        ReviewDiffViewed = false;
        Merge.SyncTaskContext(Task?.Id, Task?.Title, Task?.IsPlanningParent == true);
        NotifySessionSections();
        OnPropertyChanged(nameof(CanAcceptDrop));
@@ -981,7 +996,20 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
    private bool CanResetAndRetry() =>
        Task != null && _worker.IsConnected && ShowResetAndRetry;

-    [RelayCommand]
+    // Set once the user opens the diff/combined-diff for the current review. Reset on
+    // task switch and on every state change (a new run means a new diff to read), so
+    // Approve & Merge stays blocked until the pending changes have been inspected.
+    [ObservableProperty]
+    [NotifyCanExecuteChangedFor(nameof(ApproveReviewCommand))]
+    [NotifyPropertyChangedFor(nameof(ShowReviewDiffHint))]
+    private bool _reviewDiffViewed;
+
+    // True while a review is pending, there is a diff to inspect, and it has not been
+    // opened yet — drives the "open the diff first" hint next to Approve & Merge.
+    public bool ShowReviewDiffHint =>
+        IsWaitingForReview && Merge.HasReviewableDiff && !ReviewDiffViewed;
+
+    [RelayCommand(CanExecute = nameof(CanApproveReview))]
    private async System.Threading.Tasks.Task ApproveReviewAsync()
    {
        if (Task is null || !_worker.IsConnected) return;
@@ -999,6 +1027,13 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
        }
    }

+    // 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);
+
    [RelayCommand(CanExecute = nameof(HasReviewFeedback))]
    private async System.Threading.Tasks.Task RejectReviewAsync()
    {
@@ -48,6 +48,15 @@ public sealed partial class MergeSectionViewModel : ViewModelBase
    public Func<DiffViewerViewModel, System.Threading.Tasks.Task>? ShowDiffViewer { get; set; }
    public Func<MergeModalViewModel, System.Threading.Tasks.Task>? ShowMergeModal { get; set; }

+    // Raised when the user opens a diff/combined-diff for the current task, so the
+    // review gate can record that the changes were inspected before merging.
+    public Action? DiffViewed { get; set; }
+
+    // True when there is something to inspect before merging (a live worktree diff,
+    // a merged commit range, or a planning/children combined diff). When false there
+    // is nothing to read, so the review gate must not block approve.
+    public bool HasReviewableDiff => CanOpenDiff() || CanReviewDiff();
+
    public MergeSectionViewModel(IWorkerClient worker, IServiceProvider services)
    {
        _worker = worker;
@@ -69,6 +78,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase
        _worktreeStateLabel = worktreeState;
        _listWorkingDir = listWorkDir;
        OnPropertyChanged(nameof(ShowMergeSection));
+        OnPropertyChanged(nameof(HasReviewableDiff));
        OpenDiffCommand.NotifyCanExecuteChanged();
        OpenWorktreeCommand.NotifyCanExecuteChanged();
    }
@@ -79,6 +89,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase
        TaskTitle = taskTitle;
        _isPlanningParent = isPlanningParent;
        OnPropertyChanged(nameof(ShowMergeSection));
+        OnPropertyChanged(nameof(HasReviewableDiff));
    }

    internal void SyncChildOutcomes(bool hasChildOutcomes, int subtaskCount)
@@ -86,6 +97,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase
        _hasChildOutcomes = hasChildOutcomes;
        _subtaskCount = subtaskCount;
        OnPropertyChanged(nameof(ShowMergeSection));
+        OnPropertyChanged(nameof(HasReviewableDiff));
        ReviewCombinedDiffCommand.NotifyCanExecuteChanged();
    }

@@ -128,6 +140,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase
        var vm = _services.GetRequiredService<DiffViewerViewModel>();
        vm.ConfigurePlanning(TaskId, SelectedMergeTarget ?? "main");
        await vm.LoadAsync();
+        DiffViewed?.Invoke();
        await ShowDiffViewer(vm);
    }

@@ -157,6 +170,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase
        else return;

        await vm.LoadAsync();
+        DiffViewed?.Invoke();
        await ShowDiffViewer(vm);
    }

@@ -348,6 +348,16 @@
              <Border Height="1" Background="{DynamicResource LineBrush}"
                      IsVisible="{Binding Merge.ShowMergeSection}" />

+              <!-- Gate: you must open the diff before Approve & Merge unlocks. -->
+              <StackPanel Orientation="Horizontal" Spacing="6"
+                          IsVisible="{Binding ShowReviewDiffHint}">
+                <PathIcon Data="{StaticResource Icon.ArrowOut}" Width="11" Height="11"
+                          Foreground="{DynamicResource AmberBrush}" VerticalAlignment="Center" />
+                <TextBlock Classes="meta" VerticalAlignment="Center"
+                           Foreground="{DynamicResource AmberBrush}"
+                           Text="Open the diff to enable merge" />
+              </StackPanel>
+
              <WrapPanel Orientation="Horizontal">
                <Button Classes="btn accent" Content="Approve &amp; Merge" Margin="0,0,8,8"
                        Command="{Binding ApproveReviewCommand}" />