feat(mobile): report paired device identity#8295
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughMobile clients now resolve platform-specific device names, sanitize them, and include them in encrypted authentication. The runtime sanitizes and records the reported name during authentication, then updates and persists the corresponding device registry entry. Tests cover device identity resolution, handshake payloads, authentication state, registry validation, and persistence. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mobile/src/transport/device-identity.ts (1)
48-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClient sanitization uses
split('')(UTF-16 code units) while the server usesArray.from()(Unicode code points).
raw.split('')splits surrogate pairs into separate code units. Both halves pass thecharCodeAt(0) > 0x1ffilter, so thejoin('')reconstructs them — butslice(0, MAX_DEVICE_NAME_LENGTH)counts code units, not code points. A device name with emoji near the 64-char boundary could be sliced mid-surrogate, producing a lone surrogate that the server'sArray.from()-basedslicewouldn't. The server re-sanitizes, so this won't corrupt persisted data, but the client and server could compute different lengths for the same input.♻️ Use
Array.fromfor code-point-accurate filtering and slicingconst cleaned = raw - .split('') + .split('') // or Array.from(raw) for full code-point alignment with server .filter((character) => { const code = character.charCodeAt(0) return code > 0x1f && code !== 0x7f }) .join('') .replace(/\s+/g, ' ') .trim() - .slice(0, MAX_DEVICE_NAME_LENGTH) + .slice(0, MAX_DEVICE_NAME_LENGTH) // consider Array.from(raw).filter(...).join('') to match serverIf full parity with the server is desired:
- const cleaned = raw - .split('') - .filter((character) => { - const code = character.charCodeAt(0) - return code > 0x1f && code !== 0x7f - }) - .join('') - .replace(/\s+/g, ' ') - .trim() - .slice(0, MAX_DEVICE_NAME_LENGTH) + const cleaned = Array.from(raw) + .filter((character) => { + const codePoint = character.codePointAt(0) + return codePoint !== undefined && codePoint > 0x1f && codePoint !== 0x7f + }) + .join('') + .replace(/\s+/g, ' ') + .trim() + .slice(0, MAX_DEVICE_NAME_LENGTH)mobile/src/transport/device-identity.test.ts (1)
51-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for Android model-only and brand-only fallback paths.
Lines 34-39 of
device-identity.tshandle cases where onlymodelor onlybrandis available on Android, but no test exercises these branches. Consider adding cases for{ Model: 'Pixel 8' }(no Brand) and{ Brand: 'Google' }(no Model).♻️ Suggested additional test cases
it('uses Android model only when brand is absent', () => { platformMock.OS = 'android' platformMock.constants = { Model: 'Pixel 8' } expect(resolveMobileDeviceDisplayName()).toBe('Pixel 8') }) it('uses Android brand only when model is absent', () => { platformMock.OS = 'android' platformMock.constants = { Brand: 'Google' } expect(resolveMobileDeviceDisplayName()).toBe('Google') })src/main/runtime/device-registry-name.test.ts (1)
22-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
createRegistry()in the first test or returningdirfrom it.The first test manually duplicates the temp-dir creation logic (lines 23-24) instead of reusing the
createRegistry()helper. This works because the reload assertion needs thedirpath, but the helper could return both to avoid the duplication.♻️ Optional: return dir from createRegistry
- function createRegistry(): DeviceRegistry { + function createRegistry(): { registry: DeviceRegistry; dir: string } { const dir = mkdtempSync(join(tmpdir(), 'orca-device-registry-')) dirs.push(dir) - return new DeviceRegistry(dir) + return { registry: new DeviceRegistry(dir), dir } } it('renames a device and persists across reload', () => { - const dir = mkdtempSync(join(tmpdir(), 'orca-device-registry-')) - dirs.push(dir) - const registry = new DeviceRegistry(dir) + const { registry, dir } = createRegistry() const device = registry.addDevice('Mobile 7/3/2026', 'mobile')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0343004b-705f-447e-ad3b-029536a28d31
⛔ Files ignored due to path filters (1)
mobile/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
mobile/package.jsonmobile/src/transport/client-context.test.tsmobile/src/transport/client-context.tsxmobile/src/transport/device-identity.test.tsmobile/src/transport/device-identity.tsmobile/src/transport/rpc-client.test.tsmobile/src/transport/rpc-client.tssrc/main/runtime/device-registry-name.test.tssrc/main/runtime/device-registry.tssrc/main/runtime/rpc/e2ee-channel.test.tssrc/main/runtime/rpc/e2ee-channel.tssrc/main/runtime/runtime-rpc.ts
bbingz
force-pushed
the
bbingz/mobile-device-identity
branch
from
8e78fdf to
7c09b48
Compare
|
Addressed the functional Unicode nitpick in I did not add the two optional Android single-field tests: the existing suite already covers brand+model, brand-prefix deduplication, platform fallback, sanitization, and transport injection; the model-only and brand-only branches are direct returns through the same sanitizer, so adding those cases would not protect a distinct contract in this focused PR. I also left the registry test fixture unchanged. The one test that constructs its directory explicitly needs that path for the reload assertion; changing the helper to return a tuple/object would be test-only churn with no behavioral coverage gain. |
bbingz
force-pushed
the
bbingz/mobile-device-identity
branch
from
7c09b48 to
9d93eba
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
e2ee_authpayloadThis is the device-identity replacement extracted from #7949. It intentionally excludes host editing and iOS scene-lifecycle work.
Refs #7949
Screenshots
No visual redesign. The existing paired-device surface displays the additional sanitized name.
Testing
oxlintoxlintoxfmt --checkand max-lines ratchetgit diff --checkorigin/main; retained the latest host-client open registry and connection loggingAI Review Report
Independent review ended spec PASS / quality APPROVED. It verified the latest-main conflict resolution, additive protocol compatibility, encrypted transport, token-to-device binding, secure persistence, and matching client/runtime Unicode sanitizers.
Review also found and fixed a functional boundary bug: UTF-16
String.slice()could leave a lone surrogate when an emoji crossed the 64-character limit. Both sanitizers now truncate anArray.from()code-point array and have regression tests.Security Audit
expo-devicedependency needed for the iOS marketing model; it adds no credential, permission, command, or IPC methodCompatibility
The field is additive and optional, so older mobile clients continue without it and older runtimes ignore it. Desktop, SSH, keyboard, filesystem, and Git-provider paths are unchanged.
Notes
Validation used the repository-declared Node 24 toolchain. Full Electron/E2E and physical mobile-device verification were intentionally not run in this review cycle.