fix(gui): don't show a multi-pid device twice in the carousel#252
fix(gui): don't show a multi-pid device twice in the carousel#252buliwyf42 wants to merge 1 commit into
Conversation
A device with more than one model id resolves to a different `config_key` per transport — the MX Master 3S is pid b034 over BTLE but b043 via a Bolt receiver (AprilNEA#137). `config_key` is `{ext_model_id}{model_ids[0]}`, so the same physical mouse keys differently across connections, and `append_offline_known` (the AprilNEA#224 "keep asleep devices" union) deduped persisted identities against the live list only by `config_key`. So a 3S seen under both pids — e.g. after Easy-Switching it to another host — showed as TWO cards: the live/cached record under one pid plus a stale persisted-identity placeholder under the other. Dedup the offline-placeholder union by (display name, kind) as well as `config_key` — both against live records and among the placeholders themselves (a device persisted under both pids must still surface once). The resolved display name is per-model, so it collapses the pid variants without a schema change or the asset resolver in this hot path. Reported live on 0.6.12: two "MX Master 3S" appeared after switching the mouse to another host. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryFixes a phantom-duplicate card in the device carousel that appeared when an MX Master 3S (or any multi-pid device) was seen under both BTLE and Bolt pids — each producing a different
Confidence Score: 4/5The change is tightly scoped to one private function and its tests; the rest of build_device_list is untouched. The dedup logic is correct and the three unit tests cover the main scenario (live + stale pid), the all-offline scenario, and the pre-existing baseline. The seen.insert() side-effect pattern is subtle but sound. The one behavioural uncertainty — which config_key a collapsed offline placeholder gets when both pids are persisted and neither is live — has no user-visible impact while the device remains offline, and disappears the moment the device comes back online. No files require special attention; the single changed file has good test coverage for the new paths. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[append_offline_known called] --> B[Build present_keys from live list config_keys]
B --> C[Build seen from live list display_name+kind pairs]
C --> D[Iterate over known persisted identities]
D --> E{config_key in present_keys?}
E -- Yes --> F[Skip: live record already covers this key]
E -- No --> G{seen.insert display_name+kind returns true?}
G -- No: already seen --> H[Skip: same-model phantom suppressed]
G -- Yes: newly seen --> I[Create offline_record placeholder]
I --> J[Add to phantoms]
F --> D
H --> D
J --> D
D --> K[drop present_keys borrow]
K --> L[list.extend phantoms]
Reviews (1): Last reviewed commit: "fix(gui): don't show a multi-pid device ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let phantoms: Vec<DeviceRecord> = known | ||
| .filter(|(key, _)| !present.contains(key)) | ||
| .filter(|(key, identity)| { | ||
| !present_keys.contains(key) | ||
| && seen.insert((identity.display_name.clone(), identity.kind)) | ||
| }) | ||
| .map(|(key, identity)| offline_record(key, identity)) | ||
| .collect(); |
There was a problem hiding this comment.
.filter() predicate calls seen.insert() for its side effect, relying on lazy, in-order iterator evaluation to grow seen as placeholders are accepted. This is correct Rust, but the statefulness is invisible at the call site. A brief inline note would make the intent explicit and signal to future refactors that the predicate must not be reordered or parallelised.
| let phantoms: Vec<DeviceRecord> = known | |
| .filter(|(key, _)| !present.contains(key)) | |
| .filter(|(key, identity)| { | |
| !present_keys.contains(key) | |
| && seen.insert((identity.display_name.clone(), identity.kind)) | |
| }) | |
| .map(|(key, identity)| offline_record(key, identity)) | |
| .collect(); | |
| // `seen.insert` doubles as a check-and-mark: it returns `false` (filter | |
| // out) if the (name, kind) tuple is already present, and `true` (keep) | |
| // while adding it so subsequent duplicates are also rejected. The side | |
| // effect is intentional and relies on sequential, in-order iteration. | |
| let phantoms: Vec<DeviceRecord> = known | |
| .filter(|(key, identity)| { | |
| !present_keys.contains(key) | |
| && seen.insert((identity.display_name.clone(), identity.kind)) | |
| }) | |
| .map(|(key, identity)| offline_record(key, identity)) | |
| .collect(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let phantoms: Vec<DeviceRecord> = known | ||
| .filter(|(key, _)| !present.contains(key)) | ||
| .filter(|(key, identity)| { | ||
| !present_keys.contains(key) | ||
| && seen.insert((identity.display_name.clone(), identity.kind)) | ||
| }) | ||
| .map(|(key, identity)| offline_record(key, identity)) | ||
| .collect(); |
There was a problem hiding this comment.
When both pids of the same device are persisted and neither is live (the "offline on another host" path), seen.insert() keeps whichever entry the known iterator yields first. If known_identities() is backed by a HashMap, iteration order can vary between runs, so the placeholder's config_key is non-deterministic. The offline card is unconfigurable (route: None), but the key it carries is the one used to look up saved DPI/button settings for the read-only display. The PR description explicitly acknowledges this as an acceptable trade-off — worth noting for anyone who later tries to determinise the placeholder selection.
|
Verified live on the rig (macOS 27 beta, MX Master 3S Easy-Switched to another host, on a Bolt receiver). A note on the exact trigger I reproduced — it's slightly broader than the b034/b043 framing, and this fix covers both. With the 3S offline (on another host) it returns no HID++ 2.0 model info, so Running a build of this branch against that exact state, the dedupe converges to one card: The |
|
Good flag, but it doesn't apply here: |
1 participant
Problem
On 0.6.12, after Easy-Switching an MX Master 3S to another host, the device carousel showed two "MX Master 3S" cards.
Root cause
config_keyisformat!("{:x}{:04x}", extended_model_id, model_ids[0])— it keys on the device's primary reported pid. A device with more than one model id reports a different primary pid per transport: the MX Master 3S is b034 over BTLE but b043 via a Bolt receiver (exactly #137). So the same physical mouse resolves to a differentconfig_keyacross connections.#224's "keep asleep devices" union (
append_offline_known) re-adds a persisted-identity placeholder for every known device absent from the live list, matched only byconfig_key. So when the 3S was seen under both pids — a live/cached record under one, a stale persisted identity under the other — neither matched the other's key, and both showed:Switching the mouse to another host is what surfaced it: the live record went offline/cached while the placeholder under the other pid stayed.
Fix
Dedup the offline-placeholder union by (display name, kind) as well as
config_key— both against the live records and among the placeholders themselves (a device persisted under both pids must still surface once). The asset-resolved display name is per-model, so it collapses the pid variants. No schema change, no asset resolver needed in this hot path (the function stays testable in isolation, as its doc comment requires).The edge case — two physically distinct devices of the same model, one online and one offline-elsewhere — collapses to one card while the second is away (it carries
route: None, so it's unconfigurable until it returns anyway, at which point it gets its own live record). That's a far rarer and milder outcome than a phantom duplicate of one device.Tests (
cargo test -p openlogi-gui state::devices, 9 pass)one_model_under_two_pids_is_not_duplicated— live record under one pid + stale identity under the other → one card; a genuinely different known device is still added.one_offline_model_persisted_under_two_pids_collapses_to_one_card— both pids persisted, neither live → one card (placeholder-vs-placeholder dedup).known_devices_are_appended_only_when_absent_from_livestill passes.cargo fmt --all -- --checkclean,cargo clippy -p openlogi-gui --all-targets -- -D warningsclean.Verification
Found and reproduced live on 0.6.12 (macOS 27 beta, MX Master 3S on a Bolt receiver). Refs #137, #224.