fix(ui): finish the 404-vs-unreachable sweep across the saves surfaces

Review caught three consumers that still spoke of an unreachable server on
an answered 404, and three smaller defects:

- computeSyncSummary (the Saves-tab slot header, #1560's own surface) printed
  a literal "Server unreachable" off the bare server_query_failed flag. Gate
  it on server_query_reason: only server_unreachable says so, otherwise "Save
  status unavailable".
- CustomPlayButton's resolve-conflict toast body was left ungated inside the
  branch whose store feed was already gated, so it asserted reachability on a
  404. Gate the copy on the same slug.
- VersionHistoryPanel's load and restore copy claimed the game's saves are
  gone. The 404 can be the device registration, not the ROM (the backend says
  so in its own comment), so it now says RomM couldn't find the save data —
  matching the wizard's wording.

Each of these three strings now carries a test asserting it never claims
reachability nor that the saves are absent; the two that had unpinned copy
were how the drift got in.

Also: the 404 gate's nested-try handling was the reverse of its docstring —
a nested peel leaked into the outer handler's scan and would false-positive.
Scope the dict walk to skip nested try statements (each is scanned as its own
statement) and pin both directions with tests. Move a misplaced union comment
onto the member it describes, and drop a dead duplicate mock setup.

Author: danielcopper

scripts/check_404_not_unreachable.py

@@ -62,9 +62,10 @@
 The AST heuristic is intentionally conservative, and a guardrail rather than a
 prover: it reads one ``try`` statement at a time and does not follow a verdict
 returned by a helper the handler calls, nor one assembled across statements
-(``resp = {...}; resp["reason"] = ...``). A ``try`` nested *inside* a catch-all
-handler contributes its own dict literals to that handler's scan, so a nested
-``except RommNotFoundError`` peel can clear the outer handler too.
+(``resp = {...}; resp["reason"] = ...``). A ``try`` nested *inside* a handler is
+scanned as its own statement, with its own sibling-peel check, and its dict
+literals are NOT attributed to the enclosing handler — otherwise a nested peel
+would read as an unpeeled verdict on the outer one and false-positive.
 """
 
 from __future__ import annotations
@@ -73,6 +74,10 @@
 import sys
 from dataclasses import dataclass
 from pathlib import Path
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from collections.abc import Iterator
 
 REPO_ROOT = Path(__file__).resolve().parent.parent
 SERVICES_DIR = REPO_ROOT / "py_modules" / "services"
@@ -154,14 +159,30 @@ def _hardcoded_verdict_spelling(value: ast.expr) -> str | None:
     return None
 
 
+def _walk_outside_nested_try(node: ast.AST) -> Iterator[ast.AST]:
+    """Yield *node*'s descendants, never descending into a nested ``ast.Try``.
+
+    The caller's top-level walk visits every ``ast.Try`` in the module, so a
+    nested one is scanned on its own terms — with its own sibling-peel check.
+    Attributing its dict literals to the enclosing handler as well would make a
+    nested ``except RommNotFoundError`` peel look like an unpeeled verdict on
+    the outer handler, which is a false positive.
+    """
+    for child in ast.iter_child_nodes(node):
+        if isinstance(child, ast.Try):
+            continue
+        yield child
+        yield from _walk_outside_nested_try(child)
+
+
 def _unreachable_spelling(handler: ast.ExceptHandler) -> str | None:
     """Return how *handler* hardcodes the unreachable verdict, or None.
 
     Only a verdict *key* counts (see :data:`VERDICT_KEYS`) — mentioning the slug
     anywhere else in the handler (a log line, a comparison against a classified
     reason) is not a hardcoded verdict.
     """
-    for node in ast.walk(handler):
+    for node in _walk_outside_nested_try(handler):
         if not isinstance(node, ast.Dict):
             continue
         for key, value in zip(node.keys, node.values, strict=True):

src/components/CustomPlayButton.test.tsx

@@ -1245,7 +1245,9 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276)
   });
 
   it("server_query_failed keeps the button in conflict and toasts (does NOT silently proceed)", async () => {
-    vi.mocked(backend.getSaveStatus).mockResolvedValue(saveStatus({ server_query_failed: true, conflicts: [] }));
+    vi.mocked(backend.getSaveStatus).mockResolvedValue(
+      saveStatus({ server_query_failed: true, server_query_reason: "server_unreachable", conflicts: [] }),
+    );
     const dataChanged = vi.fn();
     globalThis.addEventListener("romm_data_changed", dataChanged);
 
@@ -1274,12 +1276,16 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276)
     const utils = await renderInConflict();
     await clickResolve(utils);
 
-    // Post-catch state: the global store actually flipped.
+    // Post-catch state: the global store actually flipped, and the copy still
+    // names the connection — that IS the cause on this branch.
     expect(getRommConnectionState()).toBe("offline");
+    expect(vi.mocked(toaster.toast)).toHaveBeenCalledWith(
+      expect.objectContaining({ body: expect.stringContaining("Couldn't reach server") }),
+    );
     await utils.findByText("Resolve Conflict");
   });
 
-  it("server_query_failed with a not_found reason leaves the store ALONE", async () => {
+  it("server_query_failed with a not_found reason leaves the store ALONE and does not blame the connection", async () => {
     setRommConnectionState("connected");
     vi.mocked(backend.getSaveStatus).mockResolvedValue(
       saveStatus({ server_query_failed: true, server_query_reason: "not_found", conflicts: [] }),
@@ -1289,9 +1295,15 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276)
     await clickResolve(utils);
 
     // The #1570 defect: a definitive 404 is the server ANSWERING, so feeding
-    // the global store off the bare flag blacked out the whole UI. The button
-    // must still refuse to claim the conflict was resolved.
+    // the global store off the bare flag blacked out the whole UI. The toast
+    // must not assert reachability either, nor claim the saves are gone — the
+    // 404 can be the device registration rather than the ROM.
     expect(getRommConnectionState()).toBe("connected");
+    const toastCalls = vi.mocked(toaster.toast).mock.calls;
+    const body = toastCalls[toastCalls.length - 1]?.[0].body ?? "";
+    expect(body).not.toMatch(/couldn't reach|unreachable|not reachable|offline/i);
+    expect(body).not.toMatch(/no saves|has no save|without saves/i);
+    // Still refuses to claim the conflict was resolved.
     expect(vi.mocked(handleConflicts)).not.toHaveBeenCalled();
     await utils.findByText("Resolve Conflict");
   });

src/components/CustomPlayButton.tsx

@@ -735,14 +735,20 @@ export const CustomPlayButton: FC<CustomPlayButtonProps> = ({ appId }) => { // N
       // believing the conflict was cleared. Surface it and stay in conflict,
       // exactly like the network-throw catch below (#1276).
       if (result.server_query_failed) {
-        // Only an explicit unreachable verdict is a connectivity signal:
-        // feeding the store off the bare flag blacked out the whole UI over a
-        // ROM the server merely no longer has (#1570).
-        if (result.server_query_reason === "server_unreachable") {
+        // Only an explicit unreachable verdict is a connectivity signal — for
+        // the store AND the copy. Off the bare flag both blamed the connection
+        // for a ROM the server merely no longer has (#1570).
+        const unreachable = result.server_query_reason === "server_unreachable";
+        if (unreachable) {
           reportServerReachable(false);
         }
         detach(debugLog(`CustomPlayButton: resolve conflict — server query failed for rom ${romId}`));
-        toaster.toast({ title: "RomM Sync", body: "Couldn't reach server to resolve conflict" });
+        toaster.toast({
+          title: "RomM Sync",
+          body: unreachable
+            ? "Couldn't reach server to resolve conflict"
+            : "RomM couldn't find this game's save data — conflict left unresolved",
+        });
         setState("conflict");
         return;
       }

src/components/SlotSetupWizard.test.tsx

@@ -1120,9 +1120,6 @@ describe("SlotSetupWizard", () => {
       // #1560's path: the 404 means the server ANSWERED. Reporting offline
       // here blacked out the whole UI while RomM was working fine (#1570).
       setRommConnectionState("checking");
-      vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(
-        makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }),
-      );
       const result = makeSetupInfo({ recommended_action: "not_found", server_query_failed: true });
       vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(result);
       render(<SlotSetupWizard {...defaultProps()} />);

src/components/saves/VersionHistoryPanel.test.tsx

@@ -155,9 +155,12 @@ describe("VersionHistoryPanel", () => {
     const { getByText, container } = render(<VersionHistoryPanel {...defaultProps()} />);
     fireEvent.click(getByText("Previous Versions"));
     await flushAsync();
-    expect(container.textContent).toContain("RomM no longer has this game's saves.");
-    // The server answered — blaming the connection is the #1570 defect.
-    expect(container.textContent).not.toContain("Couldn't reach RomM");
+    expect(container.textContent).toContain("RomM couldn't find this game's save data.");
+    // The server answered, so the copy must not blame the connection — and it
+    // must not claim the saves are gone either: the 404 can be the device
+    // registration rather than the ROM (#1570).
+    expect(container.textContent).not.toMatch(/couldn't reach|unreachable|not reachable|offline/i);
+    expect(container.textContent).not.toMatch(/no saves|has no save|without saves/i);
   });
 
   it("Retry button retriggers loadVersions", async () => {
@@ -390,9 +393,13 @@ describe("VersionHistoryPanel", () => {
       fireEvent.click(getByText("Restore"));
       await flushAsync();
       await flushAsync();
-      expect(vi.mocked(toaster.toast)).toHaveBeenCalledWith(
-        expect.objectContaining({ body: "RomM no longer has this game's saves — nothing was restored." }),
-      );
+      const toastCalls = vi.mocked(toaster.toast).mock.calls;
+      const body = toastCalls[toastCalls.length - 1]?.[0].body ?? "";
+      expect(body).toBe("RomM couldn't find this game's save data — nothing was restored.");
+      // Same rule as the wizard copy: no reachability claim, no claim that the
+      // game's saves are gone (the 404 can be the device id) — see #1570.
+      expect(body).not.toMatch(/couldn't reach|unreachable|not reachable|offline/i);
+      expect(body).not.toMatch(/no saves|has no save|without saves/i);
       expect(onRestored).not.toHaveBeenCalled();
     });
 

src/components/saves/VersionHistoryPanel.tsx

@@ -54,7 +54,7 @@ export const VersionHistoryPanel: FC<VersionHistoryPanelProps> = ({
         // The server answered — blaming the connection here would be a lie (#1570).
         detach(debugLog(`VersionHistoryPanel: server has no such entry for ${filename}: ${result.message}`));
         setVersions(null);
-        setLoadError("RomM no longer has this game's saves.");
+        setLoadError("RomM couldn't find this game's save data.");
       } else {
         detach(debugLog(`VersionHistoryPanel: server unreachable for ${filename}: ${result.message}`));
         setVersions(null);
@@ -154,9 +154,8 @@ export const VersionHistoryPanel: FC<VersionHistoryPanelProps> = ({
         // instead of telling the user the version is gone.
         toaster.toast({ title: "RomM Sync", body: "Couldn't reach RomM. Check your connection and try again." });
       } else if (result.status === "not_found") {
-        // Mirror of the branch above: RomM answered, so a retry cannot help
-        // and the toast must not send the user to check their connection.
-        toaster.toast({ title: "RomM Sync", body: "RomM no longer has this game's saves — nothing was restored." });
+        // Mirror of the branch above: RomM answered, so a retry cannot help.
+        toaster.toast({ title: "RomM Sync", body: "RomM couldn't find this game's save data — nothing was restored." });
         // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- exhaustive final branch of the 9-member RollbackStatus union; an explicit check (vs. plain `else`) keeps the per-status symmetry and leaves any future-added status unhandled instead of silently routing it to the "unsupported" toast
       } else if (result.status === "unsupported") {
         toaster.toast({ title: "RomM Sync", body: "Version history requires RomM 4.7+" });

src/components/saves/helpers.test.ts

@@ -291,9 +291,10 @@ describe("computeSyncSummary", () => {
     });
   });
 
-  it("short-circuits to 'Server unreachable' when server_query_failed is true", () => {
+  it("short-circuits to 'Server unreachable' when the reason is server_unreachable", () => {
     const status = makeStatus({
       server_query_failed: true,
+      server_query_reason: "server_unreachable",
       files: [
         {
           filename: "save.srm",
@@ -317,8 +318,31 @@ describe("computeSyncSummary", () => {
     });
   });
 
+  it("does NOT claim the server is unreachable on a definitive 404", () => {
+    // The Saves tab is #1560's own surface: saying "Server unreachable" for a
+    // ROM the server merely answered 404 for is the exact lie #1570 removes.
+    // It still must not run the matrix against the empty server list.
+    const status = makeStatus({
+      server_query_failed: true,
+      server_query_reason: "not_found",
+      files: [],
+    });
+    const { syncSummaryText } = computeSyncSummary(true, status, []);
+    expect(syncSummaryText).not.toMatch(/unreachable|not reachable|offline/i);
+    expect(syncSummaryText).not.toMatch(/no saves|has no save|without saves/i);
+    expect(syncSummaryText).toBe("Save status unavailable");
+  });
+
+  it("does not claim unreachable when the query failed without a reason slug", () => {
+    // Defensive: an older backend (or an unclassified failure) sends the flag
+    // with no reason. Silence about the cause beats asserting the wrong one.
+    const status = makeStatus({ server_query_failed: true, files: [] });
+    const { syncSummaryText } = computeSyncSummary(true, status, []);
+    expect(syncSummaryText).not.toMatch(/unreachable|not reachable|offline/i);
+  });
+
   it("server_query_failed wins over conflicts", () => {
-    const status = makeStatus({ server_query_failed: true });
+    const status = makeStatus({ server_query_failed: true, server_query_reason: "server_unreachable" });
     const conflicts: SyncConflict[] = [
       {
         type: "sync_conflict",

src/components/saves/helpers.ts

@@ -123,9 +123,11 @@ export function statusLabel(status: string, lastSyncAt: string | null): { color:
  * Build the active slot's sync-summary header line.
  *
  * Returns the empty (null) state for inactive slots or missing status. When
- * the backend signals `server_query_failed`, short-circuits with a neutral
- * "Server unreachable" instead of running the matrix-derived classification
- * (which would otherwise read an empty server list as "ready to upload").
+ * the backend signals `server_query_failed`, short-circuits instead of running
+ * the matrix-derived classification (which would read an empty server list as
+ * "ready to upload"). Only an explicit `server_query_reason` of
+ * `server_unreachable` may say so in the text — on an answered 404 that would
+ * be false (#1570).
  *
  * The summary is derived from the per-file matrix statuses so it can never
  * disagree with the per-file badges (`statusLabel`): any file pending upload
@@ -142,7 +144,9 @@ export function computeSyncSummary(
   if (!isActive || !saveStatus) return { syncSummaryText: null, syncSummaryColor: MUTED_COLOR };
 
   if (saveStatus.server_query_failed) {
-    return { syncSummaryText: "Server unreachable", syncSummaryColor: MUTED_COLOR };
+    const text =
+      saveStatus.server_query_reason === "server_unreachable" ? "Server unreachable" : "Save status unavailable";
+    return { syncSummaryText: text, syncSummaryColor: MUTED_COLOR };
   }
 
   const hasConflict = conflicts.length > 0;

src/types/saves.ts

@@ -244,8 +244,8 @@ export type RollbackStatus =
 export type ListFileVersionsResult =
   | { status: "ok"; versions: SaveVersionEntry[] }
   | { status: "multi_file_unsupported"; versions: SaveVersionEntry[] }
-  // See RollbackStatus — the server answered, it just has no such entry.
   | { status: "server_unreachable"; message: string }
+  // See RollbackStatus — the server answered, it just has no such entry.
   | { status: "not_found"; message: string };
 
 /** Discriminated-status result of `copySaveToSlot` — copies one server save into

tests/scripts/test_check_404_not_unreachable.py

@@ -248,6 +248,42 @@ def f():
         """)
         assert findings == []
 
+    def test_nested_try_peeling_the_404_does_not_flag_the_outer_handler(self):
+        """A nested ``try`` is scanned on its own terms, not the enclosing one.
+
+        Attributing the inner dict literals to the outer handler made a nested
+        peel read as an unpeeled verdict — a false positive, which would
+        wrongly block a future PR.
+        """
+        findings = _scan("""
+            def f():
+                try:
+                    outer()
+                except Exception:
+                    try:
+                        fetch()
+                    except RommNotFoundError:
+                        return {"recommended_action": "not_found", "server_query_failed": True}
+                    except Exception:
+                        return {"recommended_action": "server_unreachable", "server_query_failed": True}
+        """)
+        assert findings == []
+
+    def test_nested_try_does_not_mask_the_outer_handlers_own_verdict(self):
+        """The exclusion is scoped: a verdict in the OUTER body is still found."""
+        findings = _scan("""
+            def f():
+                try:
+                    outer()
+                except Exception:
+                    try:
+                        cleanup()
+                    except Exception:
+                        pass
+                    return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": "m"}
+        """)
+        assert len(findings) == 1
+
     def test_handler_with_no_return_dict(self):
         findings = _scan("""
             def f():