ReviewOS

also looking at this

craft-native/craft

feat(ios): location recording moves to the manager it always shared — ratchet 23 -> 18

#125
Merged glennmichael123 wants to merge feat/ios-location-recording into main
6 files +1653 -170
10 days ago

Moves the five location-recording lifecycle actions to Zig. Ratchet 23 → 18, the largest single step of this migration, and it closes #124.

The reason had rotted

bridge_mobile_locrecording.zig's module comment spent sixty lines arguing the five could not move. Two of its three blockers had already been removed by other work, and nothing re-checked them:

Stated blockerStatus
enableGeolocation is "a flag with no mirror anywhere in packages/zig/src"Expired. ios_config.zig reads the same craft.config.json Swift decodes, and its own header names this note as one of the two it closes.
The recorder "needs an event channel that does not exist"Expired, wrong enum. It checked capabilities.Channel; the iOS path is ios_events.Event, whose .location_update is "craftLocationUpdate" — already in use for the shipped watchPosition.
getLocationRecordingState answers from memory, so a file-reading Zig divergesCorrect. Ported literally rather than argued with.

The real obstacle was never written down: SwiftUI runs makeCoordinator() before makeUIView, so Coordinator.init — and restoreLocationRecordingState() in it — ran before craft_ios_set_webview had been called even once. Whichever runtime restores first owns the CLLocationManager for the rest of the launch, which is exactly why a Zig-owned stop could not stop it.

What it fixes

bridge_mobile_location.zig already recorded a live defect against itself: while a recording ran, Swift's manager and Zig's manager both fed craftLocationUpdate, so a page that recorded and watched got roughly two events per fix. Its own words were "it disappears when the recording actions migrate". It has.

The fix is the shape, not a patch. In the spec a recording is not a second CLLocationManager — it is a flag on the one delegate plus two files. So the five lifecycle actions live with the manager in bridge_mobile_location.zig, the file format lives in bridge_mobile_locrecording.zig, and didUpdateLocations gains a third consumer that appends before it emits, which is Swift's own order. The emit guard is now Swift's exactly: if isWatchingLocation || isRecordingLocation, satisfied once by one manager.

Two other things fall out of sharing a manager again:

  • Arbitration. clearWatch gains stopWatchingPosition's if !isRecordingLocation, and pause/stop gain its mirror. Neither guard could fail while the two runtimes owned different managers; now one serves both, and whichever finishes second must not end the other's stream.
  • The one-shot's sample returns. Swift appends every fix to the track, including a getCurrentPosition during a recording. That sample had stopped being written when this module got its own manager; it is written again.

Deliberate divergences

  • State reaches disk before the page is told the recording started. Swift sets its four variables and then persists, catching its own write failure and printing — so memory and file can drift from the first sample, with the page told it is recording. Here a persist failure refuses the start.
  • NO_ACTIVE_RECORDING becomes INVALID_PARAMETER. BridgeError carries an enum, not free text — the same trade bridge_mobile_misc.zig and bridge_mobile_watch.zig document. The rejection itself is not lost.
  • readState drops a stopped recording's leftovers, because restoreLocationRecordingState's guard does. A stop-then-read in one launch still reports the id; a relaunch reports null. That is the third blocker, ported.

The seam

One new export, craft_ios_adopt_location_recording. Swift consults it instead of its own restore, inside the same enableGeolocation guard, and falls back to restoreLocationRecordingState() when dlsym finds no runtime — which is exactly the app that still needs it. It returns "Zig owns the recorder", not "a recording was found": a launch with no recording still leaves Zig owning the next start, and a Swift restore under it would be a second owner of one file.

Verification

  • zig build test:ios — 7/7 steps, 1066/1078 passed, 12 skipped, 0 failed.
  • swiftc -typecheck against the iphoneos SDK on the generated app: no errors, and the same four warnings as HEAD, compared set-against-set rather than by count.
  • The conformance guards did the driving: DeferralAlreadyMigrated and the gated-action test both failed until the table, the ratchet and ios_config.gateFor agreed with the code.

Closes #124.

6 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.