feat: follow the controller's Bluetooth-then-Wi-Fi sequence - #32
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe setup flow opens Bluetooth after pairing-window confirmation. After pairing, it can watch a controller’s held network, recover a Wi-Fi connection after Bluetooth disconnects, and confirm eligible lost network-write responses by reading back settings. The app and SetupBench display updated network, pairing, and connection status. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to A narrow Wi-Fi status message can remain stale in some direct-search flows; the main pairing and recovery races are resolved, so merge is reasonable with bounded follow-up. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new recovery path can treat a network write as successful after reading a newer configuration with the requested network name. That does not necessarily prove this phone’s write took effect if another authorized writer changed the controller meanwhile. The potential impact is bounded to the controller being configured, and the flow retains identity and retry controls. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
apps/ios/SetupBench/Sources/SetupBench/main.swift-379-380 (1)
379-380: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
watchJoincan stop on a brief.connectionLostbefore the Wi-Fi search starts.In
SetupFlow.runWatch, the catch path setsjoin = .connectionLost. It then awaitsclose()and setsjoin = .waitingonly after that. It setsisSwitchingToWiFi = truebefore the close. This predicate returnstruefor.connectionLostwithout checkingisSwitchingToWiFi. A 50 ms poll can land during the Bluetooth close. If it does,pairandwritecallflow.reset()and cancel the Wi-Fi search. Thejust bench pairpath added by this PR then never shows the Wi-Fi handover.Proposed fix
- case .failed, .noAnswer, .connectionLost: return true + case .failed, .noAnswer: return true + // A search on Wi-Fi follows a lost Bluetooth link. + case .connectionLost: return !flow.isSwitchingToWiFiapps/ios/SetupKit/Sources/SetupKit/SetupFlow.swift-753-756 (1)
753-756: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack a completed Wi-Fi search separately from
wifiUnavailable.When
reachWiFireaches its 30-second deadline without a DNS-SD or stored-address candidate, it returnsnilandcontinueOverWiFisetsjointo.connectionLost. It does not updatewifiUnavailable, soSetupViewshows only the Bluetooth message.
wifiUnavailablealso persists across searches. A previous address attempt can therefore make a later search appear to have failed after 30 seconds. The directwatchJoinAgain→connect()path does not itself set.connectionLostwhen its single search fails, but the same stale-value problem occurs whencontinueOverWiFicompletes a search.Add search-specific state. Set it only when
reachWiFiactually reaches its deadline, clear it when a new search starts or succeeds, and use it instead ofwifiUnavailable != nilfor the 30-second text. KeepwifiUnavailableonly for the optional failure reason.Suggested fix
+ public private(set) var wifiSearchFailed = false public private(set) var wifiUnavailable: WiFiUnavailable? private func reachWiFi(deviceID: String, _ operation: Int) async -> WiFiSession? { transportActive = true isSwitchingToWiFi = true + wifiSearchFailed = false defer { isSwitchingToWiFi = false } let deadline = clock.now + Self.wifiLimit ... if generation == operation { SetupLog.flow.notice("the controller was not found on Wi-Fi") transportActive = false + wifiSearchFailed = !Task.isCancelled && clock.now >= deadline } return nil }Use
flow.wifiSearchFailedfor the connection-lost 30-second message, and showwifiUnavailable.reasononly when that value is non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: b849ad7b-8eab-44a8-8895-affba16036f8
📒 Files selected for processing (7)
apps/ios/Origin89/SetupView.swiftapps/ios/SetupBench/Sources/SetupBench/main.swiftapps/ios/SetupKit/Sources/SetupKit/SetupFlow.swiftapps/ios/SetupKit/Tests/SetupKitTests/ControllerMismatchTests.swiftapps/ios/SetupKit/Tests/SetupKitTests/SetupFlowTests.swiftapps/ios/SetupKit/Tests/SetupKitTests/WiFiFlowTests.swiftapps/ios/SetupKit/Tests/SetupKitTests/WiFiHandoverTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
|
Bench on board A, controller flashed from firmware main (2ecada9), comms
Not yet run: the unit-without-network case (needs the network cleared and a write, where the write's answer may now be lost as the station starts) and an iPhone run of either case. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Preserve held-network detection across a reconnect. · SetupFlow.swift:521
apps/ios/SetupKit/Sources/SetupKit/SetupFlow.swift:521
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve held-network detection across a reconnect.
If Bluetooth drops after Pair and Hello but before
readNetwork()returns,failleavesresumeas.readNetwork. A successful retry callsproceed, which callsreadNetwork()withafterPair == false. The controller can then report its held SSID, but the flow enters.editingNetworkinstead of watching the join. Keep the post-pair detection pending until a network read succeeds, including after a reconnect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: a65a70fe-0d10-4d2b-bc87-c43761078a18
📒 Files selected for processing (3)
apps/ios/SetupBench/Sources/SetupBench/main.swiftapps/ios/SetupKit/Sources/SetupKit/SetupFlow.swiftapps/ios/SetupKit/Tests/SetupKitTests/WiFiHandoverTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Check the operation before entering .openWindow. · SetupFlow.swift:339-340
apps/ios/SetupKit/Sources/SetupKit/SetupFlow.swift:339-340
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck the operation before entering
.openWindow.If
suspend()runs whileconnect()awaitsclearExcludedPeers(), suspension advancesgenerationand sets.suspended. When the await returns, this branch can replace.suspendedwith.openWindow. Checkgeneration == operationafter the awaited cleanup and before this branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: 0f542418-2ca2-4555-9987-e2d846e59264
📒 Files selected for processing (3)
apps/ios/SetupBench/Sources/SetupBench/main.swiftapps/ios/SetupKit/Sources/SetupKit/SetupFlow.swiftapps/ios/SetupKit/Tests/SetupKitTests/WiFiFlowTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
The app did not follow the order the controller firmware supports since origin89hq/firmware#161. It connected over Bluetooth before the pairing window was open, but a unit whose station has joined advertises only while the window is open, so re-pairing such a unit failed before the window instructions appeared. After
Pair, a unit that already holds a network was sent to the network form even though its station starts on its own. A Bluetooth drop while the station started ended with "connection lost" instead of looking for the unit on Wi-Fi.Now:
Pair, a unit that holds a network and passphrase goes straight to watching that join and then moves to Wi-Fi, with nothing written. "Choose another network" stays available.pairfollows the join of a held network (--watch SECONDS) and, with--ssid, writes a network on the pairing connection;clearwrites no network.For #28. Its acceptance names an iPhone run, which is still to do; both cases passed on board A from the macOS bench (below).
Validation
just checkpasses: format check, Rust tests, 155 SetupKit tests, the unsigned simulator build and the bench build. New tests cover the window-first open and its failures, a held network afterPair(also when the first read comes after a reconnect) and the cases that still go to the form, the Wi-Fi search after a drop (found, not found within the limit, no candidate, suspended, reset before it starts), and a lost write answer (confirmed, not held, unit not found, refusal not searched).Board A, controller from firmware main (2ecada9), comms
0.0.0+g56488aca(#161), macOS bench over the Mac's Bluetooth:wasabi:bench pair --window-openpaired in 2.3 s, went towritten(1)with nothing written, lost Bluetooth 4.2 s after Pair as the station started, found the unit over DNS-SD 11.5 s later and read the join over the WebSocket at 192.168.0.180.bench clear, epoch raised so the power-on window opens):bench pair --window-open --ssid wasabipaired, wrote version 3 with its answer 110 ms later, lost Bluetooth 4 s after the write and continued over Wi-Fi 10.6 s after the drop, joined.The radio reported on the section version in both runs, so the held-network watch reads the right version. The lost-write path did not trigger: the
SetConfiganswer arrived before the station started. Not run: the same two cases on an iPhone. Found on the way: board A advertises nothing over BLE with no network and the window closed (origin89hq/firmware#166).