Merge pull request 'Colour the window's frame, inset Hosts like its neighbours, drop Pins' (#9) from claude/title-bar-color-windows-6d386c into main

Reviewed-on: #9

Directory.Build.targets

@@ -0,0 +1,68 @@
+<Project>
+
+  <!--
+    ◆ THE TRIMMER'S VERSION IS PINNED HERE BECAUSE OTHERWISE THE LOCK FILES ARE NOT LOCKED.
+
+    Microsoft.NET.ILLink.Tasks is not referenced by anything in this repository. The SDK adds it
+    on its own to any project that sets IsTrimmable or IsAotCompatible — DodoSSH.Contracts and
+    DodoSSH.Crypto do, and the Android head gets it from trimming being on by default there — and
+    the version it asks for is whatever the running SDK happens to bundle. That version lives in
+    the SDK's own Microsoft.NETCoreSdk.BundledVersions.props, as a KnownILLinkPack item.
+
+    Which makes it a dependency whose version is a property of the toolchain rather than of this
+    repository, and that is the whole problem: packages.lock.json records it as a Direct reference
+    with a requested range, so the lock file silently means "whichever SDK last ran a restore".
+    global.json says rollForward: latestMinor, so CI's setup-dotnet installs the newest 10.x SDK
+    that exists on the day it runs. The moment .NET ships a servicing release, CI's SDK asks for a
+    version the committed lock files do not have, and the locked-mode restore in ci.yml fails with
+    NU1004 before a single file is compiled.
+
+    That is not hypothetical. It closed the whole pipeline: main's run 125 and every open pull
+    request went red together, on
+
+        error NU1004: The package reference Microsoft.NET.ILLink.Tasks version has changed
+        from [10.0.10, ) to [10.0.11, ).
+
+    with nothing in any of those commits touching a package. .NET had shipped SDK 10.0.400, which
+    bundles ILLink 10.0.11 where 10.0.302 bundled 10.0.10, and setup-dotnet installed it the next
+    time anything ran.
+
+    Worse than the outage is the shape of the repair without this pin. Regenerating the lock files
+    holds only until the next servicing release, and it cannot be done from a machine whose newest
+    SDK is older than the runner's: a restore on 10.0.302 writes 10.0.10 straight back and re-breaks
+    CI, so the recorded version becomes a fact about whoever ran restore last rather than about this
+    repository. That is exactly the state locking exists to prevent, and it is not a hypothetical
+    either — every SDK installed on the machine this pin was written on tops out at 10.0.302.
+
+    Pinning it makes the recorded version a decision this repository made, reviewable in a diff
+    like every other version in Directory.Packages.props, and identical on every machine whatever
+    SDK it has. Moving it is then a deliberate edit here plus a regenerated lock file, which is the
+    same ceremony any other dependency bump gets.
+
+    It is an Update on the SDK's item rather than a PackageVersion in Directory.Packages.props, and
+    it has to be: the reference is implicit, so the SDK supplies the version itself and central
+    package management never gets asked. ProcessFrameworkReferences reads @(KnownILLinkPack) when
+    it runs, which is why this lives in Directory.Build.targets — the item does not exist yet while
+    Directory.Build.props is being evaluated.
+
+    Keep this within a patch or two of the runtime the SDK ships. It is the trimming analyzer and
+    the ILLink task, so a small skew is harmless, but a version far behind the framework being
+    analysed is a real way to miss a trim warning.
+  -->
+  <Target Name="PinTheILLinkPackVersion" BeforeTargets="ProcessFrameworkReferences">
+    <!--
+      Inside a target, and not for tidiness. The SDK ships one KnownILLinkPack per target framework
+      and they all share the identity "Microsoft.NET.ILLink.Tasks", so the TargetFramework metadata
+      is the only thing telling net10.0's entry from net8.0's. A condition on %(...) is item
+      batching, which MSBuild permits in a target and rejects during evaluation with MSB4191 — so
+      an ItemGroup at the top of this file cannot express "only the net10.0 one" at all, and the
+      unconditioned Update it would have to become rewrites every framework's entry.
+    -->
+    <ItemGroup>
+      <KnownILLinkPack Update="Microsoft.NET.ILLink.Tasks"
+                       Condition="'%(TargetFramework)' == 'net10.0'"
+                       ILLinkPackVersion="10.0.11" />
+    </ItemGroup>
+  </Target>
+
+</Project>

docs/manual-checks.md

@@ -2424,7 +2424,9 @@ script warns rather than failing when that is legitimate, which is the first rel
 ### 16.7 The update arrives, and the restart lands in it · **the whole point of the work**
 
 With v0.1.0 installed and running, a vault unlocked, a host change made, and **a terminal open**, publish
-v0.1.1 (`-Upload`). Then press CHECK NOW on Settings → General rather than waiting six hours.
+v0.1.1 (`-Upload`). Then press CHECK NOW on Settings → General rather than waiting six hours. Closing and
+reopening the application does the same thing without the button: the first pass of the loop runs at launch,
+so a client started after a release finds it without anybody asking.
 
 **Pass:** the progress bar moves, the banner appears above the status bar, and — the part to actually watch
 — the terminal **reflows cleanly rather than being sliced**, with the remote seeing the smaller row count.

src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs

@@ -74,16 +74,6 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
     /// </remarks>
     private static readonly TimeSpan CheckInterval = TimeSpan.FromHours(6);
 
-    /// <summary>How long to wait before the first pass.</summary>
-    /// <remarks>
-    /// A delay, where <c>VaultViewModel</c>'s sync loop runs a pass immediately. The difference is what the
-    /// user is waiting for: a vault edited on another machine should be current by the time they have
-    /// finished reading the list, whereas nothing anybody does in their first two minutes depends on an
-    /// update. Launch is already contending for the network and the CPU with a schema migration, a resumed
-    /// sign-in and a first sync, at the one moment somebody is watching the window.
-    /// </remarks>
-    private static readonly TimeSpan FirstCheckDelay = TimeSpan.FromMinutes(2);
-
     private readonly IUpdateChannel updates;
     private readonly ClientSettingsStore settings;
     private readonly TimeProvider clock;
@@ -242,16 +232,40 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
         loop = RunCheckLoopAsync(lifetime.Token);
     }
 
+    /// <remarks>
+    /// <para>
+    /// <b>The first pass runs at launch, with no delay in front of it.</b> It used to wait two minutes, on
+    /// the argument that nothing anybody does in their first two minutes depends on an update and launch is
+    /// already contending for the network with a schema migration, a resumed sign-in and a first sync. What
+    /// that argument leaves out is the run that is over before the two minutes are: a client opened to reach
+    /// one host and closed again never checks at all, and a machine used that way is exactly the one ADR
+    /// 0011 warns about — quietly a year behind, with the mechanism to fix it switched on and never reached.
+    /// Every start now asks.
+    /// </para>
+    /// <para>
+    /// <b>The yield is what keeps that off the launch path.</b> <see cref="Start"/> is called from
+    /// <c>MainWindowViewModel.StartAsync</c> before the migration, so running the pass inline would put
+    /// whatever the channel does before its own first await — Velopack reads the install layout from disk —
+    /// between the user and their window. Yielding hands the rest of the launch back and lets the check run
+    /// in a later turn, which is the same moment in every sense that matters and none of the cost.
+    /// </para>
+    /// </remarks>
     private async Task RunCheckLoopAsync(CancellationToken cancellationToken)
     {
         try
         {
-            await Task.Delay(FirstCheckDelay, clock, cancellationToken).ConfigureAwait(true);
+            await Task.Yield();
 
             using var timer = new PeriodicTimer(CheckInterval, clock);
 
             do
             {
+                // Task.Yield takes no token, unlike the delay it replaced, so a shutdown that lands while
+                // the loop is waiting to be handed back the thread has to be observed here rather than
+                // only at the next tick. Otherwise an application closed during launch spends its last
+                // moment asking a release channel about a build it is not going to run.
+                cancellationToken.ThrowIfCancellationRequested();
+
                 await CheckOnceAsync(cancellationToken).ConfigureAwait(true);
             }
             while (await timer.WaitForNextTickAsync(cancellationToken).ConfigureAwait(true));

src/DodoSSH.Client.Shell/WebAssets/terminal.js

@@ -84,14 +84,57 @@ const RELEASE_FOCUS_MESSAGE = 'dodossh.release-focus';
 const root = document.getElementById('root');
 const statusBanner = document.getElementById('status');
 
-/** @type {Map<number, {term: object, fit: object, pane: HTMLElement}>} */
+/** @type {Map<number, {term: object, fit: object, pane: HTMLElement, notice: string}>} */
 const sessions = new Map();
 
 /** @type {WebSocket | null} */
 let socket = null;
 
-function setStatus(text) {
-  statusBanner.textContent = text ?? '';
+/** Whose pane is showing, or null before there is one — see activate(). */
+let activeSessionId = null;
+
+/*
+  ── THE BANNER BELONGS TO ONE PANE AT A TIME ─────────────────────────────────────────────────────────
+  There is one #status element for the whole page, because there is one page for every terminal: the
+  panes are stacked in the same box and all but the active one are hidden. What goes in it comes from
+  two sources that are not the same size, and the difference is the whole of this.
+
+  The socket's troubles are the page's. There is a single socket behind every pane, so "the view is
+  reconnecting" is true of whatever is on screen and true of the panes behind it.
+
+  A session's last words are not. "The remote closed the session." is a fact about one terminal and says
+  nothing whatever about the others — so it is held on the session and drawn only while that session's
+  pane is the one showing. Written straight into the shared element, which is what this used to do, it
+  outlived the tab it described: switching to a live terminal left the dead one's epitaph sitting under
+  it, and opening or closing any other tab wiped the message whether or not it belonged to that tab.
+
+  The socket's half wins when both have something to say: a page whose socket is down is not showing
+  live output on any pane, which makes what became of one session the less urgent of the two.
+*/
+let transportStatus = statusBanner.textContent ?? '';
+
+function renderStatus() {
+  const notice = activeSessionId === null ? '' : sessions.get(activeSessionId)?.notice ?? '';
+
+  statusBanner.textContent = transportStatus || notice;
+}
+
+/** Says something about the socket, which every pane shares. */
+function setTransportStatus(text) {
+  transportStatus = text ?? '';
+  renderStatus();
+}
+
+/** Records what became of one session, to be drawn only while that session's pane is showing. */
+function setSessionNotice(sessionId, text) {
+  const session = sessions.get(sessionId);
+
+  if (!session) {
+    return;
+  }
+
+  session.notice = text ?? '';
+  renderStatus();
 }
 
 /** Builds a frame: opcode, big-endian session id, then payload. */
@@ -292,7 +335,7 @@ function createSession(sessionId) {
 
   term.onResize(() => sendResize(sessionId, term, pane));
 
-  const session = { term, fit, pane };
+  const session = { term, fit, pane, notice: '' };
   sessions.set(sessionId, session);
 
   activate(sessionId);
@@ -306,6 +349,11 @@ function activate(sessionId) {
     session.pane.dataset.active = String(id === sessionId);
   }
 
+  // The banner follows the pane. Whatever this session has to say for itself replaces whatever the
+  // session that was showing had to say for its own, which is the point of holding it per session.
+  activeSessionId = sessionId;
+  renderStatus();
+
   const active = sessions.get(sessionId);
   if (active) {
     active.term.focus();
@@ -373,7 +421,10 @@ function handleFrame(buffer) {
         session.term.write(REPLAY_BANNER);
       }
 
-      setStatus('');
+      // This session's own line, and only this one's: a session that is open has nothing to say about
+      // how it ended. The page's own "Connecting…" is cleared by the socket opening, which happens
+      // before any frame can arrive.
+      setSessionNotice(sessionId, '');
       break;
     }
 
@@ -426,7 +477,14 @@ function handleFrame(buffer) {
       session.pane.remove();
       sessions.delete(sessionId);
 
-      setStatus('');
+      // The notice went with the session record it was held on, but the page can still be pointing at
+      // the pane that is now gone. Cleared rather than left dangling, so the banner stops describing a
+      // closed tab while the host decides which pane to show next.
+      if (activeSessionId === sessionId) {
+        activeSessionId = null;
+      }
+
+      renderStatus();
       break;
     }
 
@@ -503,14 +561,19 @@ function handleFrame(buffer) {
       const session = sessions.get(sessionId);
       const reason = new TextDecoder().decode(payload);
 
-      if (session) {
-        // The pane and its scrollback stay. The user was probably reading the last thing the
-        // remote said, and that is usually why the session ended.
-        session.term.write(`\r\n\x1b[38;5;244m── ${reason} ──\x1b[0m\r\n`);
-        session.term.options.cursorBlink = false;
+      if (!session) {
+        // No pane, so there is nothing this page can honestly hang the reason on. It used to go into
+        // the banner anyway, which printed one session's ending underneath whichever pane happened to
+        // be showing at the time.
+        break;
       }
 
-      setStatus(reason);
+      // The pane and its scrollback stay. The user was probably reading the last thing the
+      // remote said, and that is usually why the session ended.
+      session.term.write(`\r\n\x1b[38;5;244m── ${reason} ──\x1b[0m\r\n`);
+      session.term.options.cursorBlink = false;
+
+      setSessionNotice(sessionId, reason);
       break;
     }
 
@@ -535,7 +598,7 @@ function scheduleReconnect() {
     return;
   }
 
-  setStatus('Reconnecting the terminal view…');
+  setTransportStatus('Reconnecting the terminal view…');
 
   reconnectTimer = setTimeout(() => {
     reconnectTimer = null;
@@ -555,7 +618,7 @@ function connect() {
   socket.binaryType = 'arraybuffer';
 
   socket.addEventListener('open', () => {
-    setStatus('');
+    setTransportStatus('');
 
     // Back to the quick attempt for whatever the next failure turns out to be. Kept slow between
     // attempts within one outage, reset once the outage is actually over.

tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs

@@ -403,6 +403,46 @@ public sealed class UpdateFlowTests : IDisposable
         channel.Checks.ShouldBe(1);
     }
 
+    /// <remarks>
+    /// <para>
+    /// The loop rather than <c>CheckOnceAsync</c>, which is the one thing the rest of this file avoids
+    /// driving — and here it is the whole point, because the claim is about when the first pass happens
+    /// rather than about what it does. The first pass used to wait two minutes, which meant a client opened
+    /// to reach one host and closed again never asked at all.
+    /// </para>
+    /// <para>
+    /// It waits on the pass and not on a clock, so there is nothing here to be flaky about: a regression
+    /// that puts a delay back in front of the loop does not fail on a margin, it spins until the suite's own
+    /// cancellation ends it.
+    /// </para>
+    /// </remarks>
+    [Fact]
+    public async Task TheFirstPassRunsAtStart_RatherThanOnADelay()
+    {
+        channel.Available = new AvailableUpdate("1.3.0");
+
+        var updates = Build();
+        await using var _ = updates.ConfigureAwait(false);
+
+        updates.Start();
+
+        while (updates.State is not UpdateState.Ready)
+        {
+            Token.ThrowIfCancellationRequested();
+
+            await Task.Yield();
+        }
+
+        channel.Checks.ShouldBe(1);
+        updates.ReadyVersion.ShouldBe("1.3.0");
+    }
+
+    /// <remarks>
+    /// Started and disposed with nothing in between, which since the first pass stopped waiting two minutes
+    /// is a race rather than a formality: the loop may be anywhere between its yield and a finished check
+    /// when the cancellation lands. What is asserted is what matters either way — that disposing returns,
+    /// rather than waiting on a pass that will never be allowed to finish.
+    /// </remarks>
     [Fact]
     public async Task DisposingStopsTheLoop()
     {