Mobile: don't let a different id-less device inherit a saved TV's row (#154) - #155
Conversation
…#154) A lone id-less saved row at an address was trusted on endpoint alone, so a different id-less TV that later answered there silently took over the row's name. Id-less rows now carry a soft fingerprint (model/manufacturer/codename/ user-set name) from the live device's properties; rememberDevice only refreshes a lone match when the fingerprints don't clearly disagree. A disagreement never proves anything either way -- it only rules a match out, recording the connection as a new row and leaving the old one untouched. A missing field on either side is unknown, not a disagreement, so older rows migrate for free. session.identityNote surfaces an honest "different device" note once wherever a connect flow notices the mismatch.
|
@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: a6e53c3280
ℹ️ 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 result = rememberDevice(this.host, this.connectPort, this.connectedDevice); | ||
| if (result.mismatch) this.identityNote = "A different device is now at this address."; |
There was a problem hiding this comment.
When an id-less transport is restored through recoverOrMarkLost, that path calls wirelessConnect and refreshDevices directly and never reaches this rememberDevice check. If the old address now belongs to a different model or manufacturer, the heartbeat recovery marks the new TV live while retaining the sole old saved row, allowing cachedDeviceName to display the old TV's name and subsequent actions to target the wrong device; run the same identity check after a successful silent recovery before marking it live.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 067ac16: recoverOrMarkLost() now calls rememberCurrentDevice() on a successful silent recovery, before setting liveness to "live" -- the same fingerprint check an explicit reconnect gets. On a mismatch it sets identityNote and records the connection as a new row rather than refreshing the old one, so cachedDeviceName becomes ambiguous (not the old name) once that happens. Covered by a new Playwright case in tests/session.test.mjs ("a silent recovery onto a different device does not inherit the old saved row or its name"), verified to fail before the fix.
| if (session.identityNote) { | ||
| showToast(session.identityNote, "info"); | ||
| session.identityNote = ""; |
There was a problem hiding this comment.
When reconnect(d) detects a mismatch, this toast belongs to the Devices component, but the same success block immediately navigates to Dashboard and unmounts it. Clearing identityNote here also prevents Dashboard's onMount consumer from displaying the warning, so reconnecting a saved row from Devices still gives the user no indication that a different device answered; leave the note for Dashboard or use a toast that survives navigation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 067ac16: reconnect(d) in Devices.svelte no longer clears session.identityNote or shows its own toast for the mismatch case -- it leaves the note unread when connecting a saved row so Dashboard's own onMount consumer shows it after the navigate. Covered by a new Playwright case in tests/session.test.mjs ("the Devices screen hands its different-device note on to Dashboard instead of losing it on navigate"), verified to fail before the fix.
…e on navigate Codex review on PR #155 (#154) found two gaps: - recoverOrMarkLost() redialed the same host:port on a failed liveness probe without ever running the fingerprint check, so a different id-less TV that took over mid-session during a silent recovery was trusted unconditionally. It now calls rememberCurrentDevice() on a successful recovery, before marking the device live. - Devices' reconnect(d) cleared session.identityNote and showed its own toast, then immediately navigated to Dashboard -- the toast never survived to be seen. It now leaves the note for Dashboard's onMount to show instead. Both covered by new Playwright cases in tests/session.test.mjs, verified to fail before each fix.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 067ac16a6a
ℹ️ 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".
| // took over this address mid-session would be silently treated as | ||
| // "the" saved TV reconnecting, the same bug #154 fixed for an | ||
| // explicit connect. | ||
| if (live) this.rememberCurrentDevice(); |
There was a problem hiding this comment.
When the heartbeat or connection-loss handler recovers while the user remains on Dashboard (or another screen), this new call can set identityNote, but no mounted component reacts to that state: Dashboard only consumes it in onMount or its explicit switch flow. Thus the follow-up identity check detects the replacement but leaves the warning invisible until the user happens to leave and revisit Dashboard, allowing actions to continue against the unexpected TV; display the note through a reactive/global notification path as part of silent recovery. This is fresh evidence beyond the earlier missing-check finding because the check now runs, but its result has no live consumer.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb42eb4: the display is now global instead of per-screen. App.svelte renders session.identityNote directly in a Toast at the root, reactive to the runes store from wherever it's set, with its own self-clearing 4s $effect -- so it appears the instant rememberCurrentDevice() sets it, regardless of which screen is mounted or whether anything remounts. Dashboard and Devices no longer read or clear identityNote themselves; they only suppress their own "Connected"/"Reconnected" success toast while a note is pending. Covered by two new Playwright cases in tests/session.test.mjs, including one that stays on the Remote screen the entire time (no navigation, no remount) to prove the note still surfaces; both verified to fail before the fix.
…d mount Codex review found the round-2 fix still only displayed session.identityNote from Dashboard's onMount, so a silent recovery landing while the user is already on Dashboard (or any other screen) set the note but nothing showed it until a later remount. App.svelte now renders the note directly at the root in a reactive Toast with its own self-clearing timer, so it appears the moment it's set regardless of which screen is open. Dashboard and Devices no longer read or clear identityNote themselves; they just suppress their own success toast while a note is pending. Covered by new Playwright cases in tests/session.test.mjs, including one that never navigates, verified to fail before the fix.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Closes #154
A lone id-less saved row at an address was trusted on endpoint alone, so a different id-less TV that later answered at the same address silently inherited the old row's name and
localId. Id-less rows now carry an optional soft fingerprint (model, manufacturer, device codename, and the TV's own user-set name) captured from the live device's properties.rememberDeviceonly refreshes a lone id-less match when the saved and live fingerprints don't clearly disagree (different model or manufacturer); a disagreement sets the match aside and records the connection as a new, distinct row instead of overwriting the old one. A missing field on either side is unknown, never a disagreement, so rows saved before this existed migrate for free. The existing#146ambiguity rule already refuses to mark either of two id-less rows sharing an endpoint "connected," so no changes were needed there.session.identityNotesurfaces an honest "A different device is now at this address." note once, wherever a connect flow notices the mismatch (Dashboard, Devices).New tests in
v2/mobile/tests/savedDevices.test.mjscover the mismatch, the match-still-refreshes case, the empty-fingerprint migration case, and the hardware-identified-is-never-second-guessed case; verified to fail before the fix.npm test(127/127) andnpm run check(0 errors / 0 warnings) both pass.🤖 Generated with Claude Code