diff --git a/v2/mobile/BACKLOG.md b/v2/mobile/BACKLOG.md index e477c03..c5c824d 100644 --- a/v2/mobile/BACKLOG.md +++ b/v2/mobile/BACKLOG.md @@ -75,9 +75,10 @@ Fixed GitHub #115–#118, all found by a code-read audit. Each repro is now a te **none has run on a phone or TV.** - One hardware-id rule (`lib/identity.ts`, matching desktop `idKey`): trim, and empty or any - casing of `unknown` is no id. Stored placeholder ids migrate to id-less rows, and rows that then - share one identity collapse to the newest, so saved-TV list keys are unique and Forget removes - exactly one TV (#116). A placeholder is no longer a wildcard for `adb-unknown-*` adverts. + casing of `unknown` is no id. Stored placeholder ids migrate to id-less rows, each keyed by its + own stable random local id rather than its bare endpoint (see #146 below), so saved-TV list keys + are unique and Forget removes exactly one TV (#116). A placeholder is no longer a wildcard for + `adb-unknown-*` adverts. - `cachedDeviceName` matches on verified id, or an id-less row at the exact endpoint; an identified row never names a TV that reports no id. Advertised serials match only exactly or with adbd's six-character suffix, so `shield` no longer verifies `adb-shield-a` (#117). @@ -85,6 +86,32 @@ Fixed GitHub #115–#118, all found by a code-read audit. Each repro is now a te - `loadHealth` cannot wedge after its device vanishes mid-load, Cancel no longer shows a reconnect failure, and Diagnostics keys its safety lookup on the sorted package set (#118). +## Id-less saved TVs stay distinct (2026-10-01) + +Fixed GitHub #146, raised by Codex on PR #144 and declined there as an edge case at the time. +Two id-less saved TVs (no hardware id) that had shared one `host:port` used to collapse into a +single storage row on migration/read -- the older one silently discarded -- because every id-less +row was keyed by its bare endpoint. Fixed in `savedDevices.ts`/`identity.ts`/`discoveryRows.ts`: + +- Every id-less row gets a stable random `localId` the first time it is persisted; `savedDeviceKey` + keys on that instead of the endpoint, so two id-less TVs stay two rows no matter what address + they shared. Existing stored rows are migrated on first read without losing any. +- `rememberDevice` only refreshes an existing id-less row when exactly one saved row matches the + connecting endpoint. When two or more already share it, which one just reconnected is unknowable, + so the connection is deliberately not persisted against any of them -- it writes nothing, rather + than either overwriting one of them with a guess or saving a new row on every repeat reconnect + (the latter would eventually evict a genuine saved TV once `MAX` is reached). +- A shared `savedDeviceIsLiveConnection` check (`identity.ts`) is the one place that decides + whether a saved row is unambiguously the live connection: always true for a verified hardware id, + true for an id-less match only when it is the single id-less row at that endpoint. Both + `buildDiscoveryRows` and the Devices screen's "Other TVs" filter use it, so neither marks more + than one id-less saved row "connected" at a shared live endpoint, and neither hides an ambiguous + row (and its Forget control) from the list -- the discovery row falls back to a generic name and + both saved rows report "saved-address" instead of one of them claiming the connection. + +Covered by new tests in `tests/savedDevices.test.mjs` and `tests/discoveryRows.test.mjs`; none has +run on a device. + ## Stability reset (2026-09-04) A four-track audit (feature parity vs v1/desktop, connection lifecycle, screen UX, Rust backend) diff --git a/v2/mobile/HANDOFF.md b/v2/mobile/HANDOFF.md index 6c4f3d3..cb6e16e 100644 --- a/v2/mobile/HANDOFF.md +++ b/v2/mobile/HANDOFF.md @@ -37,6 +37,23 @@ exactly or with adbd's 6-character suffix. `loadHealth` can no longer wedge, Can reports a reconnect failure, and Diagnostics no longer blanks its safety badges when two apps swap rank. Every fix is covered by a test reproducing the issue; none has run on a device. +**Id-less saved rows stay distinct (2026-10-01, GitHub #146).** An id-less row (no hardware id) +used to be keyed everywhere by its bare `host:port`, so two genuinely different TVs that had +shared an endpoint at different times collapsed into one storage slot — the older one silently +lost. Every id-less row now gets its own random `localId` the first time it is persisted +(`savedDevices.ts`), and `savedDeviceKey`/`identity.ts` key id-less rows on that instead of the +endpoint; existing stored rows are migrated in place on first read, losing nothing. +`rememberDevice` only refreshes an existing id-less row when exactly one saved row matches the +connecting endpoint — when two or more already share it, which one just reconnected is not +knowable, so the connection is deliberately not persisted against any of them (writing nothing, +not a new row every time, is what stops repeat reconnects from eventually evicting a genuine saved +TV once `MAX` is reached). `discoveryRows.ts` and the Devices screen's "Other TVs" filter both go +through one shared `savedDeviceIsLiveConnection` check (`identity.ts`) so neither ever marks more +than one id-less saved row "connected" at a shared live endpoint, or hides an ambiguous row from +the list; the discovery row shows a generic name and both saved rows report "saved-address" +instead. Covered by new tests in `savedDevices.test.mjs` and `discoveryRows.test.mjs`; unverified +on a device. + --- ## 1. What this is diff --git a/v2/mobile/src/lib/discoveryRows.ts b/v2/mobile/src/lib/discoveryRows.ts index 0f57c3b..4c3ea6a 100644 --- a/v2/mobile/src/lib/discoveryRows.ts +++ b/v2/mobile/src/lib/discoveryRows.ts @@ -1,6 +1,7 @@ import type { Discovery, SavedDevice } from "./types"; import { normalizeHardwareId, + savedDeviceIsLiveConnection, savedDeviceKey, savedDeviceMatchesConnection, } from "./identity"; @@ -114,9 +115,6 @@ export function buildDiscoveryRows( // other host is stale. Only that advert is dropped, never the other // devices answering from the same address. const liveId = live.connected ? normalizeHardwareId(live.hardwareId) : undefined; - const liveSaved = liveId - ? savedDevices.find((saved) => normalizeHardwareId(saved.hardwareId) === liveId) - : undefined; const hosts = new Map(); for (const discovery of discoveries) { const host = discovery.host.trim(); @@ -173,15 +171,12 @@ export function buildDiscoveryRows( ); // On the live row only the live TV's own identity counts. When it reports // no id, an advert there may be stale, so the row is named only after an - // id-less saved TV at this exact endpoint, or after nothing. + // id-less saved TV at this exact endpoint -- and only when that endpoint + // has exactly one such TV saved, never a guess between several. if (isLive) { - savedMatch = liveId - ? liveSaved - : savedDevices.find( - (saved) => - normalizeHardwareId(saved.hardwareId) === undefined && - savedDeviceMatchesConnection(saved, live.host, live.connectPort), - ); + savedMatch = savedDevices.find((saved) => + savedDeviceIsLiveConnection(saved, savedDevices, live.host, live.connectPort, live.hardwareId), + ); } if (savedMatch) verified.add(savedMatch); rows.push({ @@ -248,7 +243,7 @@ export function buildDiscoveryRows( pairingPorts: [], legacyConnectPorts: [], status: atLiveEndpoint - ? savedDeviceMatchesConnection(saved, live.host, live.connectPort, live.hardwareId) + ? savedDeviceIsLiveConnection(saved, savedDevices, live.host, live.connectPort, live.hardwareId) ? "connected" // Something answers at this saved address, but nothing shows it is // this TV, so the row claims the address and not the connection. diff --git a/v2/mobile/src/lib/identity.ts b/v2/mobile/src/lib/identity.ts index 82fa427..7a9fcaa 100644 --- a/v2/mobile/src/lib/identity.ts +++ b/v2/mobile/src/lib/identity.ts @@ -31,9 +31,39 @@ export function savedDeviceMatchesConnection( ); } +/// Whether a saved row can be told apart as *the specific TV* live on this +/// connection, not just a row that matches the bare address. A verified +/// hardware id is always unambiguous -- ids are unique by construction. An +/// id-less match is only unambiguous when it is the single id-less row saved +/// at that exact endpoint; when another id-less TV shares it, the address +/// alone cannot say which of them answered, so neither counts as the live +/// row here. +export function savedDeviceIsLiveConnection( + device: SavedDevice, + allSaved: SavedDevice[], + host: string, + connectPort: number, + hardwareId?: string | null, +): boolean { + if (!savedDeviceMatchesConnection(device, host, connectPort, hardwareId)) return false; + if (normalizeHardwareId(hardwareId)) return true; + const idlessAtEndpoint = allSaved.filter( + (other) => + normalizeHardwareId(other.hardwareId) === undefined && + other.host === host && + other.connectPort === connectPort, + ).length; + return idlessAtEndpoint <= 1; +} + export function savedDeviceKey(device: SavedDevice): string { const hardwareId = normalizeHardwareId(device.hardwareId); - return hardwareId - ? `hardware:${hardwareId}` + if (hardwareId) return `hardware:${hardwareId}`; + // An id-less row is keyed by its own stable local id once it has one + // (assigned by savedDevices.ts the first time it is persisted), so two + // id-less TVs that share an endpoint stay distinct rows. The bare endpoint + // is only a fallback for a row that has not gone through that migration. + return device.localId + ? `local:${device.localId}` : `idless:${device.host}:${device.connectPort}`; } diff --git a/v2/mobile/src/lib/savedDevices.ts b/v2/mobile/src/lib/savedDevices.ts index 85430c5..1ab8bf1 100644 --- a/v2/mobile/src/lib/savedDevices.ts +++ b/v2/mobile/src/lib/savedDevices.ts @@ -11,17 +11,24 @@ import type { Device, SavedDevice } from "./types"; import { deviceLabelOf } from "./types"; import { normalizeHardwareId, + savedDeviceIsLiveConnection, savedDeviceKey, savedDeviceMatchesConnection, } from "./identity"; -export { savedDeviceKey, savedDeviceMatchesConnection }; +export { savedDeviceIsLiveConnection, savedDeviceKey, savedDeviceMatchesConnection }; const KEY = "atv.savedDevices.v1"; const AUTO_KEY = "atv.autoConnect.v1"; const MAX = 16; const EPOCH = new Date(0).toISOString(); +/// Not a security identifier -- just random enough that two rows saved in the +/// same process tick never collide. +function randomLocalId(): string { + return `${Date.now().toString(36)}${Math.random().toString(36).slice(2, 10)}`; +} + function normalizeSavedDevice(value: unknown): SavedDevice | null { if (!value || typeof value !== "object") return null; const d = value as Record; @@ -48,10 +55,20 @@ function normalizeSavedDevice(value: unknown): SavedDevice | null { const hardwareId = normalizeHardwareId( typeof d.hardwareId === "string" ? d.hardwareId : undefined, ); + // Every id-less row needs a stable key of its own (see identity.ts); carry + // an existing one over, or mint one now if this row has never had one (an + // older record, or one whose only id was a placeholder that just migrated + // away above). + const localId = hardwareId + ? undefined + : typeof d.localId === "string" && d.localId.trim() !== "" + ? d.localId.trim() + : randomLocalId(); return { host: d.host.trim(), connectPort: d.connectPort, ...(hardwareId ? { hardwareId } : {}), + ...(localId ? { localId } : {}), name: typeof d.name === "string" && d.name.trim() !== "" ? d.name.trim() @@ -63,6 +80,19 @@ function normalizeSavedDevice(value: unknown): SavedDevice | null { }; } +/// Whether a raw stored record will come out of `normalizeSavedDevice` as +/// id-less and without an existing local key -- i.e. it is about to be +/// assigned a fresh one, which must be persisted so it stays stable. +function rawNeedsLocalId(raw: unknown): boolean { + if (!raw || typeof raw !== "object") return false; + const d = raw as Record; + const hardwareId = normalizeHardwareId( + typeof d.hardwareId === "string" ? d.hardwareId : undefined, + ); + if (hardwareId) return false; + return typeof d.localId !== "string" || d.localId.trim() === ""; +} + function read(): SavedDevice[] { try { const raw = localStorage.getItem(KEY); @@ -90,7 +120,10 @@ function read(): SavedDevice[] { const stored = d.hardwareId; return normalizeHardwareId(typeof stored === "string" ? stored : undefined) !== stored; }); - if (unnormalizedId || unique.length !== normalized.length) write(unique); + const needsLocalIdMigration = parsed.some(rawNeedsLocalId); + if (unnormalizedId || needsLocalIdMigration || unique.length !== normalized.length) { + write(unique); + } return unique; } catch { return []; @@ -165,9 +198,15 @@ export function rememberDevice( ): void { const current = read(); const hardwareId = hardwareIdOf(device); - const existing = current.find((d) => - sameTv(d, host, connectPort, hardwareId), - ); + const matches = current.filter((d) => sameTv(d, host, connectPort, hardwareId)); + // An id-less connection can match more than one saved row only when several + // id-less TVs have shared this exact endpoint over time. Which one just + // answered is not knowable from the endpoint alone, so nothing is written: + // claiming one would be a guess, and saving a fresh row on every repeat + // reconnect would eventually evict a genuine saved TV once MAX is reached. + if (matches.length > 1) return; + const existing = matches[0]; + const combinedHardwareId = hardwareId ?? existing?.hardwareId; const reportedFriendlyName = device?.properties?.friendly_name?.trim(); const name = reportedFriendlyName || existing?.name || deviceLabelOf(device); const list = current.filter((d) => d !== existing); @@ -176,9 +215,8 @@ export function rememberDevice( connectPort, name, deviceType: device?.device_type ?? existing?.deviceType ?? "unknown", - ...(hardwareId || existing?.hardwareId - ? { hardwareId: hardwareId ?? existing?.hardwareId } - : {}), + ...(combinedHardwareId ? { hardwareId: combinedHardwareId } : {}), + ...(combinedHardwareId ? {} : { localId: existing?.localId ?? randomLocalId() }), lastUsed: new Date().toISOString(), }); write(list); diff --git a/v2/mobile/src/lib/types.ts b/v2/mobile/src/lib/types.ts index edf3407..50e1128 100644 --- a/v2/mobile/src/lib/types.ts +++ b/v2/mobile/src/lib/types.ts @@ -391,6 +391,11 @@ export interface SavedDevice { /// Hardware serial when the TV reported one. Lets a TV keep its row and /// name across an IP change, and stops a reused IP from inheriting a name. hardwareId?: string; + /// Stable random id assigned the first time a row with no hardware id is + /// saved. Two TVs that never reported a serial and happened to share a + /// host:port are still distinct rows -- the address alone cannot tell them + /// apart, so each gets its own key instead of collapsing into one. + localId?: string; /// ISO timestamp of the last successful connect, for "last used" copy. lastUsed: string; } diff --git a/v2/mobile/src/screens/Devices.svelte b/v2/mobile/src/screens/Devices.svelte index 59f3044..d34b249 100644 --- a/v2/mobile/src/screens/Devices.svelte +++ b/v2/mobile/src/screens/Devices.svelte @@ -6,8 +6,8 @@ forgetSavedDevice, lastUsedLabel, listSavedDevices, + savedDeviceIsLiveConnection, savedDeviceKey, - savedDeviceMatchesConnection, savedHostHasMultipleIdentities, } from "../lib/savedDevices"; import { deviceTypeLabel } from "../lib/types"; @@ -75,8 +75,13 @@ saved.filter( (device) => !session.connectedDevice || - !savedDeviceMatchesConnection( + // An id-less row at the live endpoint is only "the current TV" when + // it is the only id-less row saved there -- otherwise which one is + // actually connected cannot be told, so none of them is hidden from + // this list (and its Forget control stays reachable). + !savedDeviceIsLiveConnection( device, + saved, session.host, session.connectPort, currentHardwareId, diff --git a/v2/mobile/tests/discoveryRows.test.mjs b/v2/mobile/tests/discoveryRows.test.mjs index 91f6c75..8daba19 100644 --- a/v2/mobile/tests/discoveryRows.test.mjs +++ b/v2/mobile/tests/discoveryRows.test.mjs @@ -156,6 +156,51 @@ test("only the saved TV whose id is live claims a shared live endpoint (#115)", assert.deepEqual(idless.map((row) => [row.name, row.status]), [["No id", "connected"]]); }); +test("two id-less saved TVs sharing the live endpoint never both claim connected (#146)", () => { + const first = saved({ + hardwareId: undefined, + localId: "guest-a", + name: "Guest A", + lastUsed: "2026-09-01T00:00:00.000Z", + }); + const second = saved({ + hardwareId: undefined, + localId: "guest-b", + name: "Guest B", + lastUsed: "2026-09-02T00:00:00.000Z", + }); + + // With an advert, the live address still gets a generic connected row, but + // neither saved TV's name is attached to it -- the address cannot say which + // of them answered. + const withAdvert = buildDiscoveryRows( + [advert("192.168.1.10", 5555, LEGACY, "Android TV")], + [first, second], + liveAt("192.168.1.10", 5555), + ); + assert.deepEqual( + withAdvert.map((row) => [row.key, row.name, row.status]), + [ + ["discovery:192.168.1.10", "Android TV", "connected"], + ["saved:local:guest-b", "Guest B", "saved-address"], + ["saved:local:guest-a", "Guest A", "saved-address"], + ], + ); + assert.equal(withAdvert[0].savedTarget, undefined); + + // With no advert at all, the live endpoint has no discovery row to stand + // in as an honest "connected, but unidentified" placeholder, so neither + // saved row may claim it -- both report the address as previously used. + const silent = buildDiscoveryRows([], [first, second], liveAt("192.168.1.10", 5555)); + assert.deepEqual( + silent.map((row) => [row.key, row.status]), + [ + ["saved:local:guest-b", "saved-address"], + ["saved:local:guest-a", "saved-address"], + ], + ); +}); + test("a stale advert never names the live row after a different saved TV (#115)", () => { const shieldA = saved({ hardwareId: "shield-a", name: "Shield A" }); const googleB = saved({ hardwareId: "google-b", name: "Google B", lastUsed: "2026-08-01T00:00:00.000Z" }); diff --git a/v2/mobile/tests/savedDevices.test.mjs b/v2/mobile/tests/savedDevices.test.mjs index 8540efe..3d790fc 100644 --- a/v2/mobile/tests/savedDevices.test.mjs +++ b/v2/mobile/tests/savedDevices.test.mjs @@ -340,9 +340,11 @@ test("a reused address never lends an identified TV's name to a TV that reports assert.equal(savedDevices.cachedDeviceName("192.168.1.10", 41234), null); }); -test("a stored placeholder id becomes an id-less row with one key (#116)", () => { +test("a stored placeholder id becomes an id-less row with its own stable key, never merged with another (#116, #146)", () => { seed([ saved({ hardwareId: undefined, name: "Real", lastUsed: "2026-09-02T00:00:00.000Z" }), + // Shares "Real"'s exact host:port once its placeholder id normalizes + // away -- a genuinely different TV, not a duplicate of "Real". saved({ hardwareId: "unknown", name: "Ghost", lastUsed: "2026-09-01T00:00:00.000Z" }), saved({ host: "192.168.1.20", hardwareId: " UNKNOWN ", name: "Other", lastUsed: "2026-08-01T00:00:00.000Z" }), ]); @@ -350,15 +352,24 @@ test("a stored placeholder id becomes an id-less row with one key (#116)", () => const rows = savedDevices.listSavedDevices(); const keys = rows.map(savedDevices.savedDeviceKey); assert.equal(new Set(keys).size, keys.length); + // All three survive -- the placeholder migration must not discard a TV + // just because it now shares an address with another id-less row. assert.deepEqual( - rows.map((row) => [row.name, row.hardwareId]), - [["Real", undefined], ["Other", undefined]], + new Set(rows.map((row) => `${row.name}:${row.hardwareId}`)), + new Set(["Real:undefined", "Ghost:undefined", "Other:undefined"]), ); // The placeholder is migrated out of storage, not just hidden on read. assert.equal(rawRows().some((row) => "hardwareId" in row), false); + // Each row's freshly-minted local key is itself persisted, so it stays + // stable across reads instead of being re-rolled every time. + assert.deepEqual(savedDevices.listSavedDevices().map(savedDevices.savedDeviceKey), keys); - savedDevices.forgetSavedDevice(rows[0]); - assert.deepEqual(savedDevices.listSavedDevices().map((row) => row.name), ["Other"]); + const ghost = rows.find((row) => row.name === "Ghost"); + savedDevices.forgetSavedDevice(ghost); + assert.deepEqual( + new Set(savedDevices.listSavedDevices().map((row) => row.name)), + new Set(["Real", "Other"]), + ); }); test("reconnecting a TV that reports a placeholder id refreshes its row instead of appending (#116)", () => { @@ -373,6 +384,43 @@ test("reconnecting a TV that reports a placeholder id refreshes its row instead assert.equal(rows[0].name, "Ghost"); }); +test("never guesses which of two id-less TVs at one endpoint just reconnected (#146)", () => { + seed([ + saved({ hardwareId: undefined, localId: "first-tv", name: "First TV", lastUsed: "2026-09-01T00:00:00.000Z" }), + saved({ hardwareId: undefined, localId: "second-tv", name: "Second TV", lastUsed: "2026-09-02T00:00:00.000Z" }), + ]); + + // An id-less connection lands on the one shared endpoint of two already + // distinct saved TVs. Which one it is cannot be told from the address + // alone, so neither existing row is claimed (and so overwritten with a + // possibly-wrong identity) -- and nothing new is persisted either, since a + // fresh unidentified row on every repeat reconnect would eventually evict a + // genuine saved TV once MAX is reached. The connection is simply not + // recorded against any saved identity. + savedDevices.rememberDevice("192.168.1.10", 5555, device(undefined, { + name: "Just reported", + properties: { friendly_name: null, serial_number: undefined }, + })); + + // Repeating the ambiguous reconnect many times still does not grow storage. + for (let i = 0; i < 20; i++) { + savedDevices.rememberDevice("192.168.1.10", 5555, device(undefined, { + name: "Just reported", + properties: { friendly_name: null, serial_number: undefined }, + })); + } + + const rows = savedDevices.listSavedDevices(); + assert.equal(rows.length, 2); + assert.deepEqual( + new Set(rows.map((row) => row.name)), + new Set(["First TV", "Second TV"]), + ); + // Both original rows are untouched -- neither lost its name nor its key. + assert.equal(rows.some((row) => row.name === "First TV" && savedDevices.savedDeviceKey(row) === "local:first-tv"), true); + assert.equal(rows.some((row) => row.name === "Second TV" && savedDevices.savedDeviceKey(row) === "local:second-tv"), true); +}); + test("sorts by lastUsed and truncates the oldest saved rows", () => { seed(Array.from({ length: 18 }, (_, index) => saved({ host: `192.168.1.${index + 1}`, @@ -528,6 +576,39 @@ test("matches the current TV by verified id or an exact id-less endpoint", () => ); }); +test("only an unambiguous match counts as the live connection (#146)", () => { + const shieldA = saved({ hardwareId: "shield-a" }); + const lone = saved({ hardwareId: undefined, localId: "lone-tv" }); + const first = saved({ hardwareId: undefined, localId: "first-tv" }); + const second = saved({ hardwareId: undefined, localId: "second-tv", name: "Second TV" }); + + // A verified hardware id is unambiguous regardless of what else is saved. + assert.equal( + savedDevices.savedDeviceIsLiveConnection(shieldA, [shieldA, first, second], "192.168.1.10", 5555, "shield-a"), + true, + ); + // A single id-less row at the endpoint is as good as this app's identity + // story gets, so it counts. + assert.equal( + savedDevices.savedDeviceIsLiveConnection(lone, [lone], "192.168.1.10", 5555, undefined), + true, + ); + // Two id-less rows sharing the endpoint: neither may claim the connection. + assert.equal( + savedDevices.savedDeviceIsLiveConnection(first, [first, second], "192.168.1.10", 5555, undefined), + false, + ); + assert.equal( + savedDevices.savedDeviceIsLiveConnection(second, [first, second], "192.168.1.10", 5555, undefined), + false, + ); + // A row that does not even match the endpoint is never live, ambiguous or not. + assert.equal( + savedDevices.savedDeviceIsLiveConnection(first, [first, second], "192.168.1.99", 5555, undefined), + false, + ); +}); + test("forgets the exact selected identity at an identical endpoint", () => { seed([ saved({ hardwareId: "shield-a", lastUsed: "2026-09-03T00:00:00.000Z" }),