Mobile: keep id-less saved TVs distinct and honest about identity (#146) - #152
Conversation
Id-less saved rows (no hardware id) were keyed everywhere by their 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 discarded -- and buildDiscoveryRows could mark more than one of them "connected" at a shared live endpoint. Give every id-less row a stable random localId the first time it is persisted; key it on that instead of the endpoint (identity.ts, savedDevices.ts). rememberDevice only refreshes an existing id-less row when exactly one saved row matches the connecting endpoint -- with two or more, which one reconnected is unknowable, so a new row is saved rather than guessing. discoveryRows.ts never attributes a shared, id-less live endpoint to a specific saved row when more than one claims it: the discovery row falls back to a generic name and the saved rows report "saved-address" instead of claiming the connection. Existing stored rows migrate to the new key on first read without losing any. Approach (a) from #146, chosen over honesty-messaging-only merging because it keeps the two TVs' names and reconnect targets instead of discarding one.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59487e32d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // id-less TVs have shared this exact endpoint over time. Which one just | ||
| // answered is not knowable from the endpoint alone, so none of them is | ||
| // claimed -- a new row is saved rather than overwriting a guess. | ||
| const existing = matches.length === 1 ? matches[0] : undefined; |
There was a problem hiding this comment.
When two id-less rows already share this endpoint, every successful reconnect leaves existing undefined, generates another localId below, and prepends another saved row. Reconnecting repeatedly therefore fills the list with representations of the same unidentified connection; once MAX is reached, write() silently evicts the oldest genuine saved TVs. Preserve the ambiguity without persisting a fresh identity on every connection—for example, skip persistence here or reuse a session-bound unidentified row rather than guessing one of the address matches.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
| return device.localId | ||
| ? `local:${device.localId}` | ||
| : `idless:${device.host}:${device.connectPort}`; |
There was a problem hiding this comment.
Once this key allows multiple id-less rows at one endpoint to survive, Devices.svelte lines 74–85 still filters current-device membership by calling the endpoint-based savedDeviceMatchesConnection independently for every row. If the live TV reports no serial, all distinct rows at that endpoint return true and disappear from “Other TVs,” including their Forget controls; the additional row created by rememberDevice disappears too. That consumer needs the same uniqueness/ambiguity handling added to discovery rows so an address alone does not classify every saved identity as current.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed: otherTvs in Devices.svelte switched to the ambiguity-aware savedDeviceIsLiveConnection helper in d129c7b, so an id-less row at a shared live endpoint stays visible (with its Forget control) instead of being filtered out. This looks like a stale re-listing of the round-1 finding against the original commit.
…sibility) - rememberDevice no longer saves a new row on every ambiguous id-less reconnect: with two or more existing matches it writes nothing, since claiming one would be a guess and a fresh row per reconnect would eventually evict a genuine saved TV once MAX is reached. - Add shared savedDeviceIsLiveConnection (identity.ts): a hardware-id match is always unambiguous, an id-less match only counts when it is the single id-less row at that endpoint. discoveryRows.ts now uses this one helper instead of its own duplicated ambiguity bookkeeping. - Devices.svelte's "Other TVs" filter used the plain endpoint-based savedDeviceMatchesConnection, which hid every ambiguous id-less row (including their Forget controls) once the live TV reported no serial. Switched to savedDeviceIsLiveConnection so an ambiguous row stays visible.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d129c7b0f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `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 a new row is saved rather than overwriting a guess. `discoveryRows.ts` never marks |
There was a problem hiding this comment.
When two id-less saved rows share the connecting endpoint, this claims that a new row is saved, but rememberDevice actually returns immediately when matches.length > 1, leaving storage unchanged. Because this handoff is the authoritative description of mobile behavior—and BACKLOG.md repeats the same claim—future identity work and release validation will be based on behavior that does not exist; update both documents to state that ambiguous reconnects are deliberately not persisted.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82078994aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 a new row is saved rather than overwriting a guess. `discoveryRows.ts` never marks |
There was a problem hiding this comment.
This says an ambiguous reconnect saves a new row, but rememberDevice now returns immediately when multiple id-less rows match (savedDevices.ts:201-207), writing nothing. Because this is the authoritative mobile handoff and the same incorrect claim also appears in BACKLOG.md, a future maintainer could restore the storage-proliferation behavior that the follow-up deliberately removed; describe the connection as intentionally not persisted instead.
AGENTS.md reference: AGENTS.md:L152-L154
Useful? React with 👍 / 👎.
HANDOFF.md and BACKLOG.md still described the pre-fix rememberDevice
behavior ("a new row is saved" on an ambiguous reconnect). Update both to
say the connection is deliberately not persisted at all in that case, and
note the shared savedDeviceIsLiveConnection check used by both
discoveryRows.ts and the Devices screen.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff495c34b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existing = current.find((d) => | ||
| sameTv(d, host, connectPort, hardwareId), | ||
| ); | ||
| const matches = current.filter((d) => sameTv(d, host, connectPort, hardwareId)); |
There was a problem hiding this comment.
When a different id-less TV is the first device to reuse an existing row's host:port—common with legacy port 5555—sameTv matches that sole row using only the endpoint, so matches.length === 1 and the code replaces the old TV's name/type while retaining its localId. Thus the new local key still cannot preserve both TVs in the ordinary address-reuse path; the second row only survives if multiple ambiguous rows somehow already exist. Pass the selected/session identity into this update or otherwise avoid treating a lone address match as verified identity.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and real, but it predates this PR: the lone-match branch of sameTv already treated a bare endpoint match as identity before #146 (same heuristic savedDeviceMatchesConnection/cachedDeviceName/discovery-row attribution use everywhere, covered by the pre-existing "matches id-less rows only at the exact endpoint" test). Fixing it requires deciding whether to stop trusting a lone id-less endpoint match at all, which risks reintroducing the round-1 proliferation problem if done without a secondary signal. That's a product decision beyond this PR's scope -- filed as #154.
1 participant
Closes #146
Problem
Id-less saved rows (no hardware id) were keyed everywhere -- storage, the saved-TV list, and the scan rows -- by their bare
host:port. Two genuinely different TVs that had each saved to the same endpoint at different times collapsed into one storage slot on read/migration, with the older one silently discarded. The same bare-endpoint matching also letbuildDiscoveryRowsmark more than one id-less saved row "connected" at a shared live endpoint, picking one arbitrarily.Approach
Chose (a): give id-less saved rows a stable local key.
SavedDevicegets a randomlocalIdthe first time it is persisted (savedDevices.ts).savedDeviceKey(identity.ts) keys on that instead of the endpoint, so two id-less TVs sharing an address stay two distinct rows with their own names and reconnect targets -- never merged, never silently dropped.read(), losing nothing (the old code's "collapse to the newest" migration path, which did lose the older row, is gone).rememberDeviceonly 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 isn't knowable from the address alone, so a new row is saved rather than overwriting a guess.buildDiscoveryRowsnever attributes a shared, id-less live endpoint to a specific saved row when more than one id-less row claims it: the discovery row shows a generic name (not one of the ambiguous TVs' names) and both saved rows reportsaved-address, neverconnected.(b) was rejected: it would keep the data-loss-causing merge and only add messaging, whereas (a) is both safe (no row ever silently overwrites another) and keeps full information (both TVs' names/reconnect targets survive).
Forget continues to act on the exact row the user picked (
savedDeviceKey-keyed), which now works correctly for id-less rows too since their keys are unique.Tests
New coverage in
v2/mobile/tests/savedDevices.test.mjsandv2/mobile/tests/discoveryRows.test.mjs, each verified to fail before the fix:rememberDevicesaves a new row instead of guessing when two id-less rows already share an endpoint.buildDiscoveryRowsnever marks two id-less saved rows "connected" at once, with and without an mDNS advert present.v2/mobile/HANDOFF.mdandBACKLOG.mdupdated (the old backlog note described the now-fixed collapsing behavior as intentional).Gates
From
v2/mobile:npm test(119/119) andnpm run check(0 errors / 0 warnings) both pass.🤖 Generated with Claude Code