fix(connectivity): harden WCSession delegate registration and reply handling#84
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.0.0-alpha.3 #84 +/- ##
=================================================
Coverage ? 65.47%
=================================================
Files ? 32
Lines ? 533
Branches ? 0
=================================================
Hits ? 349
Misses ? 184
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review: Adding TransferUserInfo (#84)OverviewThis PR adds queued-transport support to the
The approach is solid and well-documented. A few things worth addressing before merge: IssuesMedium1. Silent failure in
2. In 3. All other mutating methods on 4. Comments reference "Phase 2" — violates CLAUDE.md guidance
5.
Low6. Orphaned temp file on crash If the process is killed after 7. Discarded transfer handles
Positive Notes
SummaryThe core design is sound. The main asks before merge are: (1) decide on a failure-propagation strategy for |
|
review text |
Code Review: Adding TransferUserInfo (PR 84)OverviewThis PR adds queued-transport support to SundialKit's WatchConnectivity layer via Bugs / Correctness1. Temp file leak on app crash between write and
2. Silent error discard in The 3. MockSession
Design4. Breaking protocol change with no default implementations
5. The flag is defined and documented but nothing in this PR reads it. A Thread Safety6.
Test Coverage7. No test for The 8.
CI / Workflow9. Tag trigger removed from The version-tag trigger was removed. Must be restored before merging to Nits
Summary
Highest-priority fixes before merging: silent error discard in |
Code Review: Adding TransferUserInfo (#84)OverviewThis PR adds the queued background-transport layer to SundialKit's WatchConnectivity abstraction: The design is largely clean and follows existing project patterns. A few issues worth addressing before merge: Issues1. MockSession asymmetry —
|
Review: Adding TransferUserInfo (PR #84)Good foundation for the queued transport layer. The documentation throughout is excellent — the A few issues worth addressing before merge: Bug —
|
| Core protocol additions | ✅ Clean, well-documented |
WatchConnectivitySession implementation |
✅ Correct (sync file read, temp-file lifecycle) |
NeverConnectivitySession stubs |
✅ |
SendOptions.useApplicationContext design |
✅ |
MockSession.transferUserInfo loopback |
❌ Incorrect simulation |
| Write/transfer error surfaces | |
CapturingConnectivityDelegate @unchecked Sendable |
|
ConnectivitySendContext.queued discoverability |
ℹ️ Suggest comment |
The loopback bug in MockSession is the one I'd call a blocking issue; the rest are improvements that can land in follow-ups or alongside this PR.
Constructing a WatchConnectivitySession no longer sets WCSession.default's delegate as a side effect. A throwaway instance (e.g. created and discarded by a SwiftUI @State autoclosure re-evaluation) could otherwise hijack the delegate from the active instance and silently drop all incoming messages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
didReceiveMessage now calls replyHandler([:]) after forwarding, mirroring the binary didReceiveMessageData path. Without it, every reply-expecting sendMessage times out with WCErrorDomain 7012. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code ReviewThis PR contains two independent bug fixes — both address real problems, but one has a latent correctness issue worth addressing before merge. Fix 1: Delegate registration deferred to
|
Delegate deferred to activate() |
✅ Correct, well-commented — add a regression test |
Auto-ack replyHandler([:]) |
|
| Test coverage | Missing for both new behaviors |
| PR description | Empty — please add context |
transferUserInfo |
Not present — is it coming in a follow-up commit? |
Guard the WCSession reply handler in both didReceiveMessage and didReceiveMessageData so a delegate that sends a real reply (via ConnectivityReceiveContext.replyWith) cannot double-invoke the handler alongside the unconditional auto-acknowledgment. The binary path was equally exposed since its delegate bridge forwards the handler to consumers. Also document that activate() is safe to call twice and that only the activated instance owns WCSession.default's delegate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR Review: Adding TransferUser Info / Three WatchConnectivity FixesThis PR bundles three related bug fixes for 1. Delegate registration moved to
|
| Change | Assessment |
|---|---|
Delegate in activate() |
✅ Correct fix, well-motivated |
| Auto-ack dict messages | ✅ Correct mirror of binary path |
| Once-guard reply handlers | ✅ Correct intent, replied flag is not thread-safe for async reply patterns |
| Tests |
The delegate-registration change is the highest-value fix here. The once-guard is logically sound for the synchronous-delegate case that exists today; hardening the replied flag against async callers and adding tests would make this production-ready.
Summary
Hardens
WatchConnectivitySessionso WatchConnectivity delivery is reliable and reply handlers behave correctly. NotransferUserInfois involved — all messaging stays on the existing dictionary / application-context paths.Changes
Register the
WCSessiondelegate inactivate(), notinit(). Constructing aWatchConnectivitySessionno longer has the global side effect of claimingWCSession.default's delegate. This prevents a throwaway instance — e.g. one created then discarded by a SwiftUI@Stateautoclosure re-evaluation — from hijacking delivery from the real, activated instance. Callingactivate()more than once is safe (re-assigning the delegate is a no-op and WCSession tolerates a repeatedactivate()).Auto-acknowledge received messages so reply-expecting sends don't time out. When a sender uses the reply-expecting
sendMessage, the receiver now auto-acks (empty reply) if the delegate didn't reply itself, avoidingWCErrorDomain 7012timeouts. The dictionary path now mirrors the existing binarydidReceiveMessageDatapath.Once-guard the auto-ack reply handlers. The same reply closure is handed to the delegate and auto-acknowledged. A delegate that actually replies (e.g. via
ConnectivityReceiveContext.replyWith) would otherwise invokereplyHandlertwice — undefined behavior on the sender side. Both the dictionary and binary paths now wrap the handler so it fires at most once.Files
WatchConnectivitySession.swift— drop delegate assignment frominitWatchConnectivitySession+ConnectivitySession.swift— assign delegate inactivate()WatchConnectivitySession+WCSessionDelegate.swift— once-guarded reply handlers + auto-ack on both message paths