recording save dialog: Cancel discards the footage instead of saving the default

The creator reported that clicking Cancel in the post-recording save prompt still saved the file under the default name: "cancel should cancel the save option, discarding the recording" — Enter is what accepts the default. The modal was always correct (Enter -> DialogResult=true, Cancel/Escape/X -> false). The bug was in the caller: FinalizeRecordingAsync only overwrote `stem` when the dialog returned true and then ran File.Move UNCONDITIONALLY, so a falsy result fell straight through into the save. The nullable stem made "I don't want this" look like "I accept the default". Extracted the decision into internal MainViewModel.CompleteRecordingSave so it is testable against real files without a frame pump: creatorSaved == false (Cancel/Escape/X) -> delete the temp file, leave the videos folder empty; a failed delete returns DiscardFailed and the toast names the file + folder (a "discarded" recording still on disk is worse than no report). creatorSaved == true (Save, or Enter on the pre-filled default) -> File.Move. A blank box still means "keep the auto name", but only on an explicit Save. Test: ytLive.Tests/RecordingSaveDialogTests.cs — 5 facts over real temp files. The headline asserts Cancel leaves the directory EMPTY, not merely "the stem is unchanged"; a modal's own test cannot cover the decision it feeds. Full suite 362/362 (the RealMouseDrag flake passed this run).
2026-09-27 09:25:21 -07:00
parent de81fa3c5d
commit e3e69d941d
7 changed files with 324 additions and 19 deletions
@@ -1,10 +1,36 @@
 # HANDOFF — current state

 **Branch:** `main` (pre-1.0, no feature branches). **Last pushed:** `7f14ffb`.
-**This session's work is COMMITTED LOCALLY, NOT PUSHED** — two commits ahead of `origin/main`.
-Push only when the creator says so.
+**This session's work is COMMITTED LOCALLY, NOT PUSHED** — **four** commits ahead of `origin/main`
+(`fae8ec5`, `5aedca7`, `f5a9d46`, `de81fa3`) plus this unit. Push only when the creator says so.

-## Two work units landed this session
+## The 2026-09-26 proof-of-concept round (creator reporting)
+
+Context: **no YouTube private test recordings are being saved**, so the creator is judging
+compositing from **local recordings**. They assumed the live path needed a separate "redirect" —
+**it does not, and that is verified, not assumed:** one `_framePump` is constructed in the
+`MainViewModel` ctor with a single `brandFlash:` callback, and both `Streaming.Operations.cs:68`
+(go-live) and `:199` (record) call that same `StartAsync`. Record+simulcast is one ffmpeg with two
+outputs. Five items, tracked in `TASKS.md` → "Creator-reported batch".
+
+### Done this unit: recording save dialog — Cancel now discards
+
+`RenameRecordingDialog` was always correct (Enter → `DialogResult=true`, Cancel/X/Escape → `false`).
+**The caller was not**: `FinalizeRecordingAsync` only overwrote `stem` when the dialog returned true
+and then ran `File.Move` **unconditionally**, so a cancelled dialog silently saved the recording
+under the default name. Extracted the decision into internal
+`MainViewModel.CompleteRecordingSave(startPath, dir, autoStem, chosenStem, creatorSaved)`:
+
+- `creatorSaved == false` (Cancel/Escape/X) → **delete the temp file**; the videos folder is left
+  empty. A failed delete returns `DiscardFailed` and the toast names the file + folder.
+- `creatorSaved == true` (Save, or Enter on the pre-filled default) → `File.Move` to the final name.
+  Blank box still means "keep the auto name" — but only on an explicit Save.
+
+Test: `ytLive.Tests/RecordingSaveDialogTests.cs` (5 facts, real temp files, no WPF needed) — the
+headline asserts Cancel leaves the directory **empty**, not merely "the stem is unchanged".
+Lesson recorded in `MyMistakes.md` (falsy `ShowDialog()` falling through into a side effect).
+
+## Two earlier work units (committed)

 ### 1. Branding flash is now composited into the OUTPUT (TASK 36 shipped)

@@ -71,8 +97,19 @@ The entire implementation is inside `#if DEBUG`. Release compiles to `DataRoot =

 ## Next

-1. Push the two commits when the creator asks.
-2. Test-console chat UX (queued): don't clear the chat window, 20px right padding on the pull-out
+1. **Ungate the brand flash from `IsLive`** (TASK batch #2) — it must run in every scene and appear
+   in recordings; today it only starts in `UpdateLiveVisuals()`'s live branch, so a recording made
+   without ever going live carries no credit.
+2. **Confine the ticker to the alert box** (#3) — it is still a global 1920×48 top overlay
+   (`BlitOverlay(…, 0, 0)`); needs a rect + scale/clip decision in both preview and output.
+3. **Multi-instance** (#4) — route `FfmpegLocator._toolsDir` through `InstanceProfile`, and confirm
+   the launch method: `dotnet run` while the first app holds `bin/…/ytLive.exe` fails with MSB3027
+   *before any instance starts* (a build-output lock, not a log conflict). `$env:YTLIVE_INSTANCE=2`
+   + `dotnet run --no-build` in a second shell is the known-good path.
+4. **Post-session efficacy report** (#5) — roll up `CurrentHealth` (dropped frames, duration, health
+   message) when a stream or recording ends.
+5. Push the commits when the creator asks.
+6. Test-console chat UX (queued): don't clear the chat window, 20px right padding on the pull-out
   input, Enter inserts a newline.
-3. Decide the alert-gating copy question above.
-4. Ship-checklist items live in `TASKS.md` → "1.0 gates".
+7. Decide the alert-gating copy question above.
+8. Ship-checklist items live in `TASKS.md` → "1.0 gates".
@@ -1047,3 +1047,50 @@ Release binary for it — and it was absent from the *Debug* binary too, because
 at compile time and never reach metadata**. The control (run it against Debug as well) is what
 exposed that. Grep for a **field/type** (`InstanceVariable`) or use reflection; never grep for an
 inlined literal, and always run the check against the build you expect it to FAIL in.
+
+## 2026-09-26 — A falsy `ShowDialog()` fell through into the side effect: "Cancel" that saved
+
+The recording save prompt let the creator name the file. It always had a **Cancel** button, and
+Cancel always *appeared* to work — the dialog closed, the toast was skipped, nothing visibly went
+wrong. The footage was saved anyway, under the default name.
+
+```csharp
+// WRONG — and it type-checked, read fine, and looked correct
+string stem = autoStem;
+var dialog = new RenameRecordingDialog(autoStem);
+if (dialog.ShowDialog() == true && !string.IsNullOrEmpty(dialog.ResultFileName))
+    stem = dialog.ResultFileName;
+
+var finalPath = UniquePath(Path.Combine(dir, stem + ".mp4"));
+File.Move(startPath, finalPath);   // <-- runs even when the creator hit Cancel
+```
+
+**The trap:** `ShowDialog()` returning `false` was collapsed into the *default* branch instead of
+being treated as a distinct outcome. "Not what I typed" and "I don't want this" are different
+answers, and the nullable stem made the first one look like a legitimate value of the second. The
+default value of the accumulator absorbed the cancel.
+
+**The rule, and the grep-able form of it:** when a modal's result gates a side effect, every branch
+is explicit and the side effect sits **inside** the accepting branch:
+
+```csharp
+if (!dialog.ShowDialog() == true) { DoTheOppositeThing(); return; }   // read the FALSE case first
+DoTheThing();                                                          // the effect is not a fall-through
+```
+
+A `DialogResult`/return value that only ever *modifies* a later value (rather than *permitting* the
+effect) is the smell. `null` from a dialog is the same trap wearing a nullable.
+
+**Why it survived so long:** the side effect is in a *different method* from the modal, and the modal
+had its own passing test (`RenameRecordingDialogSizingTests`, which only checks layout). The test
+suite was green because the bug was never in the tested unit. **A modal's own test proves nothing
+about the decision the modal feeds** — cover the commit-or-discard decision itself, against real
+files (`ytLive.Tests/RecordingSaveDialogTests.cs` drives the extracted
+`MainViewModel.CompleteRecordingSave` and asserts Cancel leaves the directory EMPTY, not just "the
+stem is unchanged").
+
+**Creator's actual ruling, worth keeping verbatim:** "if I click cancel, that doesn't mean to accept
+the default file name and save the file — that's what pressing enter should do. Cancel should cancel
+the save option, discarding the recording." A Cancel button on a destructive-and-final dialog means
+*discard*, not *accept default*. When a dialog is the last gate before data is committed, ask what
+Cancel should destroy — don't infer "keep the default" just because the default already exists.
@@ -6,12 +6,16 @@ namespace ytLive;

 /// <summary>Modal that lets the creator rename an auto-named recording before it is
 /// finalized. Returns the chosen file stem (no extension) via <see cref="ResultFileName"/>,
-/// or <c>null</c> on Cancel / keep-auto-name.</summary>
+/// or <c>null</c> when the creator cleared the box and hit Save (keep the auto name).
+/// <para><c>DialogResult == false</c> means DISCARD — the caller deletes the footage
+/// rather than saving it (CREATOR RULING 2026-09-26). Enter on the pre-filled default
+/// is a Save.</para></summary>
 public partial class RenameRecordingDialog : Window
 {
    private readonly string _autoStem;

-    /// <summary>Stem (no extension) the creator chose, or <c>null</c> to keep auto-name.</summary>
+    /// <summary>Stem (no extension) the creator chose, or <c>null</c> when the box was
+    /// cleared on an explicit Save. Never set on Cancel.</summary>
    public string? ResultFileName { get; private set; }

    public RenameRecordingDialog(string autoStem)
@@ -68,6 +68,32 @@

 ## Open items (at a glance)

+### Creator-reported batch (2026-09-26) — proof-of-concept round, recordings used as the reference
+
+The creator is **not** getting YouTube private test recordings saved, so local recordings are the
+proof of concept for compositing; the live path is assumed to share it. **Verified true, not assumed:**
+one `_framePump` is built in the `MainViewModel` ctor with a single `brandFlash:` callback, and BOTH
+`Streaming.Operations.cs:68` (go-live) and `:199` (record) call that same `StartAsync` — there is no
+second encoder path needing a "redirect". Record+simulcast is one ffmpeg with two outputs.
+
+1. ✅ **Recording save dialog: Cancel discards** — was silently saving under the default name. Enter
+   accepts the default; Cancel/Escape/X **deletes** the footage and names the file if the delete fails.
+   `MainViewModel.CompleteRecordingSave` (internal) + `ytLive.Tests/RecordingSaveDialogTests.cs`.
+2. ☐ **Brand flash must run in ALL scenes, not only while live, and must be in recordings.** Currently
+   started/stopped from `UpdateLiveVisuals()`'s `IsLive` branch, so recording-only (never go-live)
+   shows no credit at all. Ungate the presenter from `IsLive`; the pump already carries it to both.
+3. ☐ **Ticker confined to the alert box.** The announcement strip is a **global 1920×48 top overlay**
+   (`AlertOverlayLayer` comment + `BlitOverlay(…, 0, 0)`), but the creator wants it over the *Stream
+   Alerts video* — i.e. inside the `SourceType.AlertBox` rect, in preview AND output. Needs a rect
+   + scale/clip decision (see `ai.md`).
+4. ☐ **Two instances still refuse to run.** `InstanceProfile` isolates layout/auth/log/WebView, but
+   `FfmpegLocator._toolsDir` is still the hardcoded shared `%APPDATA%\ytLlive\tools` and both
+   instances can `Directory.CreateDirectory` + `ExtractBinaries` into it. Also the launch method is
+   unconfirmed: `dotnet run` while the first app holds `bin/…/ytLive.exe` dies with `MSB3027` before
+   any instance starts — that is a build-output lock, not a log conflict.
+5. ☐ **Post-session efficacy report** — on end of stream *or* recording, report what worked and what
+   failed, including the **dropped frames** the creator saw. `CurrentHealth` already tracks
+   `DroppedFrames`/`StreamDuration`; `SessionTeardownTests` is the natural home for the roll-up.
 - **TASK 3** — items 16 (Text source) + 20 (RewardEvent capture → SQLite, Alerts' persistence half) open; **17 (Alerts) is DONE via TASK 43 (2026-09-24) — native six-event alert box**
 - **TASK 9** — item 5 open (webcam identity key reconciliation)
 - **TASK 10** — Velopack update URL pending
@@ -52,18 +52,36 @@ public partial class MainViewModel : ViewModelBase
            var autoStem = RecordingFile.BuildFinalName(_recordStartTime, _recordLength);
            var dir = Path.GetDirectoryName(startPath)!;

-            // Manual rename modal: give the creator a chance to name the recording;
-            // null = keep the auto name (or the default recorded above).
-            string stem = autoStem;
+            // Manual rename modal: give the creator a chance to name the recording.
+            // CREATOR RULING 2026-09-26: Cancel means DISCARD, not "save under the
+            // default name". This used to fall through and File.Move anyway, so a
+            // cancelled dialog silently kept the recording — Cancel was a lie.
            var dialog = new RenameRecordingDialog(autoStem);
-            if (dialog.ShowDialog() == true && !string.IsNullOrEmpty(dialog.ResultFileName))
-                stem = dialog.ResultFileName;
+            var creatorSaved = dialog.ShowDialog() == true;

-            var finalPath = UniquePath(Path.Combine(dir, stem + ".mp4"));
-            File.Move(startPath, finalPath);
-            AppLog.Write($"Recording saved: {finalPath}");
+            var result = CompleteRecordingSave(
+                startPath, dir, autoStem, dialog.ResultFileName, creatorSaved);
+            switch (result.Outcome)
+            {
+                case RecordingSaveOutcome.Saved:
+                    AppLog.Write($"Recording saved: {result.FinalPath}");
                    _notifications.Info("Recording saved",
-                Path.GetFileName(finalPath) + " — you can rename it in your videos folder at any time.");
+                        Path.GetFileName(result.FinalPath!) +
+                        " — you can rename it in your videos folder at any time.");
+                    break;
+                case RecordingSaveOutcome.Discarded:
+                    AppLog.Write($"Recording discarded by creator: {startPath}");
+                    _notifications.Info("Recording discarded",
+                        "The recording was deleted and nothing was saved.");
+                    break;
+                case RecordingSaveOutcome.DiscardFailed:
+                    // A "discarded" recording still sitting on disk is worse than no
+                    // report at all — name the file and the folder so it can be found.
+                    AppLog.Write($"Recording discard failed: {startPath}");
+                    _notifications.Error("Recording discarded?",
+                        $"Could not delete {Path.GetFileName(startPath)} — it is still in {dir}.");
+                    break;
+            }
        }
        catch (Exception ex)
        {
@@ -73,6 +91,48 @@ public partial class MainViewModel : ViewModelBase
        }
    }

+    /// <summary>What happened to the footage once the save dialog was answered, plus
+    /// where it landed (<c>FinalPath</c> is set only for <see cref="RecordingSaveOutcome.Saved"/>).</summary>
+    internal readonly record struct RecordingSaveResult(RecordingSaveOutcome Outcome, string? FinalPath);
+
+    /// <summary>What happened to the footage once the save dialog was answered.</summary>
+    internal enum RecordingSaveOutcome
+    {
+        Saved,
+        Discarded,
+        DiscardFailed,
+    }
+
+    /// <summary>The commit-or-discard decision behind the recording save dialog, split out
+    /// so it can be tested against real files without a frame pump (CREATOR RULING
+    /// 2026-09-26). <paramref name="creatorSaved"/> is the dialog's DialogResult:
+    /// <c>false</c> — Cancel, Escape, or the X — DELETES the footage; only an explicit
+    /// Save (including Enter on the pre-filled default) moves it to its final name.</summary>
+    internal static RecordingSaveResult CompleteRecordingSave(
+        string startPath, string dir, string autoStem, string? chosenStem, bool creatorSaved)
+    {
+        if (!creatorSaved)
+        {
+            if (!File.Exists(startPath)) return new RecordingSaveResult(RecordingSaveOutcome.Discarded, null);
+            try
+            {
+                File.Delete(startPath);
+                return new RecordingSaveResult(RecordingSaveOutcome.Discarded, null);
+            }
+            catch (Exception)
+            {
+                return new RecordingSaveResult(RecordingSaveOutcome.DiscardFailed, startPath);
+            }
+        }
+
+        // A blank name still means "keep the auto name" (the dialog says so), but
+        // only on an explicit Save. Enter with the pre-filled default lands here too.
+        var stem = string.IsNullOrWhiteSpace(chosenStem) ? autoStem : chosenStem!;
+        var finalPath = UniquePath(Path.Combine(dir, stem + ".mp4"));
+        File.Move(startPath, finalPath);
+        return new RecordingSaveResult(RecordingSaveOutcome.Saved, finalPath);
+    }
+
    private static string UniquePath(string path)
    {
        if (!File.Exists(path)) return path;
@@ -705,6 +705,20 @@ lights when a session is actually running.
  **rename-on-stop** to `ty-…-<hh2mm2 actual length>.mp4` (`FinalizeRecordingAsync` after the pump stops &
  the file closes), numeric `-2`/`-3` suffix on collision (`UniquePath`). Length comes from `_liveElapsed`,
  which the session timer walks while `IsLive || IsRecording`.
+- **Save dialog: Cancel DISCARDS (creator ruling 2026-09-26).** The `RenameRecordingDialog` result is a
+  two-outcome decision, and it lives in the extracted `MainViewModel.CompleteRecordingSave(startPath, dir,
+  autoStem, chosenStem, creatorSaved)` (internal, so `ytLive.Tests` can drive it against real files without
+  a frame pump — see `ytLive.Tests/RecordingSaveDialogTests.cs`):
+  - `creatorSaved == false` (**Cancel / Escape / the X**) → **DELETE the temp file.** Nothing is moved,
+    nothing is left in the videos folder. A failed delete returns `DiscardFailed` and the toast NAMES the
+    file + folder, because a "discarded" recording still on disk is worse than no report.
+  - `creatorSaved == true` (Save, or **Enter on the pre-filled default**) → `File.Move` to the final name.
+    A blank/whitespace box still means "keep the auto name" — but only on an explicit Save.
+  - **The bug this fixed:** the caller used to `File.Move` UNCONDITIONALLY and only overwrite `stem` when
+    the dialog returned true, so `ShowDialog() == false` fell straight through into the save — Cancel
+    silently kept the recording under the default name. The modal itself was always correct; the
+    side effect was in the wrong place. **Any modal whose result gates a side effect must have every
+    branch handled explicitly — a falsy result must never fall through into the effect.**
 - **Folder:** default `%APPDATA%\ytLlive\recordings\`, user-overridable via `ChooseRecordFolderCommand`
  (`OpenFolderDialog`), persisted through `LayoutStore.Load/SaveRecordFolder` (`RecordFolder` key).
 - **Explicit sign-out only:** `StopStream` no longer clears the session/token. Sign out via Logout /
@@ -0,0 +1,117 @@
+using System;
+using System.IO;
+using Xunit;
+using ytLive.ViewModels;
+
+namespace ytLive.Tests;
+
+/// <summary>
+/// The post-recording save dialog (CREATOR RULING 2026-09-26).
+/// <para>Reported symptom: clicking Cancel in the save prompt still saved the
+/// recording under the default name — Cancel behaved like "accept the default". The
+/// creator's intent is explicit: <b>Enter accepts the default file name and saves;
+/// Cancel cancels the save and discards the footage.</b></para>
+/// <para>The bug was in the caller, not the modal — <c>FinalizeRecordingAsync</c> ran
+/// <c>File.Move</c> unconditionally, so <c>ShowDialog() == false</c> fell straight
+/// through into the save. These tests drive the extracted commit-or-discard decision
+/// against real files in a temp folder, which is what would have caught it.</para>
+/// </summary>
+public sealed class RecordingSaveDialogTests : IDisposable
+{
+    private readonly string _dir = Path.Combine(
+        Path.GetTempPath(), "ytLive-recsave-" + Guid.NewGuid().ToString("N")[..8]);
+
+    private const string AutoStem = "ty-20260926-1405-0130";
+    private const string StartStem = "ty-20260926-1405-0000";
+    private const string Bytes = "fake-mp4-payload";
+
+    public RecordingSaveDialogTests() => Directory.CreateDirectory(_dir);
+
+    public void Dispose()
+    {
+        try { Directory.Delete(_dir, recursive: true); } catch { /* best-effort */ }
+    }
+
+    /// <summary>The reported bug: Cancel must leave NOTHING in the videos folder — not
+    /// the temp start-name file, and not a renamed copy of it either.</summary>
+    [Fact]
+    public void Cancel_DiscardsTheFootage_AndLeavesNothingOnDisk()
+    {
+        var start = WriteStartFile();
+
+        var result = CompleteRecordingSave(start, _dir, AutoStem, chosenStem: null, creatorSaved: false);
+
+        Assert.Equal(MainViewModel.RecordingSaveOutcome.Discarded, result.Outcome);
+        Assert.False(File.Exists(start), "Cancel must delete the temp recording, not move it");
+        Assert.Empty(Directory.GetFiles(_dir));
+    }
+
+    /// <summary>Enter on the pre-filled default is a Save — the file lands under the
+    /// auto name with its bytes intact (the creator's other half of the ruling).</summary>
+    [Fact]
+    public void EnterOnTheDefault_KeepsTheAutoName_AndSavesTheFootage()
+    {
+        var start = WriteStartFile();
+
+        var result = CompleteRecordingSave(start, _dir, AutoStem, AutoStem, creatorSaved: true);
+
+        Assert.Equal(MainViewModel.RecordingSaveOutcome.Saved, result.Outcome);
+        var expected = Path.Combine(_dir, AutoStem + ".mp4");
+        Assert.Equal(expected, result.FinalPath);
+        Assert.False(File.Exists(start), "Save renames the temp file — it must not be left behind");
+        Assert.Equal(Bytes, File.ReadAllText(expected));
+        Assert.Single(Directory.GetFiles(_dir));
+    }
+
+    /// <summary>A cleared name box on an explicit Save still means "keep the auto
+    /// name" (the dialog promises this) — Cancel is the only way to discard.</summary>
+    [Fact]
+    public void SaveWithABlankName_StillFallsBackToTheAutoName()
+    {
+        var start = WriteStartFile();
+
+        var result = CompleteRecordingSave(start, _dir, AutoStem, "   ", creatorSaved: true);
+
+        Assert.Equal(MainViewModel.RecordingSaveOutcome.Saved, result.Outcome);
+        Assert.True(File.Exists(Path.Combine(_dir, AutoStem + ".mp4")));
+    }
+
+    /// <summary>An explicit Save with a chosen name honours that name.</summary>
+    [Fact]
+    public void SaveWithAChosenName_UsesThatName()
+    {
+        var start = WriteStartFile();
+
+        var result = CompleteRecordingSave(start, _dir, AutoStem, "my-stream", creatorSaved: true);
+
+        Assert.Equal("my-stream.mp4", Path.GetFileName(result.FinalPath!));
+    }
+
+    /// <summary>Never clobber an earlier recording: a second save with the same name
+    /// gets uniquified rather than overwriting the first.</summary>
+    [Fact]
+    public void SaveWithACollidingName_UniquifiesInsteadOfOverwriting()
+    {
+        var first = WriteStartFile();
+        Assert.Equal(
+            MainViewModel.RecordingSaveOutcome.Saved,
+            CompleteRecordingSave(first, _dir, AutoStem, AutoStem, true).Outcome);
+
+        var second = WriteStartFile();
+        var result = CompleteRecordingSave(second, _dir, AutoStem, AutoStem, true);
+
+        Assert.Equal(AutoStem + "-2.mp4", Path.GetFileName(result.FinalPath!));
+        Assert.Equal(2, Directory.GetFiles(_dir).Length);
+    }
+
+    private static MainViewModel.RecordingSaveResult CompleteRecordingSave(
+        string startPath, string dir, string autoStem, string? chosenStem, bool creatorSaved)
+        => MainViewModel.CompleteRecordingSave(startPath, dir, autoStem, chosenStem, creatorSaved);
+
+    private string WriteStartFile()
+    {
+        var start = Path.Combine(_dir, StartStem + ".mp4");
+        File.WriteAllText(start, Bytes);
+        return start;
+    }
+}