ReviewOS
Reload

also looking at this

craft-native/craft

fix(location): unify watch handles across mobile surfaces

#268
Merged glennmichael123 wants to merge codex/location-watch-parity into main
4 files +168 -13

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/ios/src/page-script.test.tsmodified+80-5
@@ -85,6 +85,7 @@ function loadPage(beforeInject?: (page: Record<string, any>) => void, seed = SEE
8585 return {
8686 craft,
8787 last,
88 count: (action: string) => posts.filter(message => message.action === action).length,
8889 // What Swift's resolveCallback and rejectCallback evaluate.
8990 answer: (action: string, value: unknown) => craft._resolveCallback(last(action).callbackId, value),
9091 refuse: (action: string, message: string, code: string) => craft._rejectCallback(last(action).callbackId, message, code),
@@ -96,6 +97,8 @@ function loadPage(beforeInject?: (page: Record<string, any>) => void, seed = SEE
9697 page.dispatchEvent(new CustomEvent('craftNotificationResponse', { detail })),
9798 receive: (detail: unknown) =>
9899 page.dispatchEvent(new CustomEvent('craftNotificationReceived', { detail })),
100 position: (detail: unknown) =>
101 page.dispatchEvent(new CustomEvent('craftLocationUpdate', { detail })),
99102 }
100103}
101104
@@ -108,7 +111,6 @@ describe('the injected iOS page script', () => {
108111 ['startListening', craft => craft.startListening()],
109112 ['stopListening', craft => craft.stopListening()],
110113 ['watchPosition', craft => craft.geolocation.watchPosition(() => {})],
111 ['clearWatch', craft => craft.geolocation.clearWatch()],
112114 ]
113115
114116 for (const [action, call] of calls) {
@@ -123,6 +125,12 @@ describe('the injected iOS page script', () => {
123125 })
124126 }
125127
128 it('settles a legacy clear with no active watch without posting a redundant stop', async () => {
129 const page = loadPage()
130 expect(await page.craft.geolocation.clearWatch()).toBe(true)
131 expect(page.count('clearWatch')).toBe(0)
132 })
133
126134 // #226: the counter restarted at 0 on every injection, so the seventh call
127135 // of a reloaded page drew `cb_7` again — and an answer still owed to the
128136 // previous page's `cb_7` settled it, with that call's result or its TIMEOUT.
@@ -165,19 +173,86 @@ describe('the injected iOS page script', () => {
165173 await expect(haptic).rejects.toMatchObject({ code: 'CAPABILITY_DISABLED' })
166174 })
167175
168 it('sends the v1 location clearWatch with an id once the last watch leaves', async () => {
176 it('makes the flat location pair use numeric v1 handles and one native stream', async () => {
169177 const page = loadPage()
170 const first = page.craft.location.watchPosition(() => {})
178 const first = page.craft.watchPosition(() => {})
171179 const second = page.craft.location.watchPosition(() => {})
180 expect(typeof first).toBe('number')
181 expect(typeof second).toBe('number')
182 expect(second).not.toBe(first)
183 expect(page.count('watchPosition')).toBe(1)
172184 page.answer('watchPosition', true)
173185
174 page.craft.location.clearWatch(first)
186 page.craft.clearWatch(first)
175187 expect(() => page.last('clearWatch')).toThrow()
176188 page.craft.location.clearWatch(second)
177 expect(page.last('clearWatch').callbackId).toMatch(/^cb_\d+$/)
189 expect(page.count('clearWatch')).toBe(1)
178190 page.answer('clearWatch', true)
179191 })
180192
193 it('keeps no-argument clearWatch compatible by clearing every active handle once', () => {
194 const page = loadPage()
195 let calls = 0
196 page.craft.watchPosition(() => calls++)
197 page.craft.location.watchPosition(() => calls++)
198 page.answer('watchPosition', true)
199
200 page.craft.clearWatch()
201 expect(page.count('clearWatch')).toBe(1)
202 page.position({ latitude: 1 })
203 expect(calls).toBe(0)
204
205 page.craft.clearWatch()
206 expect(page.count('clearWatch')).toBe(1)
207 })
208
209 it('drops a refused v1 start so the next subscriber asks native again', async () => {
210 const page = loadPage()
211 let calls = 0
212 page.craft.watchPosition(() => calls++)
213 page.refuse('watchPosition', 'Location is disabled', 'CAPABILITY_DISABLED')
214 await Promise.resolve()
215
216 page.position({ latitude: 1 })
217 expect(calls).toBe(0)
218 page.craft.watchPosition(() => calls++)
219 expect(page.count('watchPosition')).toBe(2)
220 page.refuse('watchPosition', 'Location is disabled', 'CAPABILITY_DISABLED')
221 await Promise.resolve()
222 })
223
224 it('removes a refused legacy listener instead of leaving a dead watch behind', async () => {
225 const page = loadPage()
226 let calls = 0
227 const started = page.craft.geolocation.watchPosition(() => calls++)
228 page.refuse('watchPosition', 'Location is disabled', 'CAPABILITY_DISABLED')
229
230 await expect(started).rejects.toMatchObject({ code: 'CAPABILITY_DISABLED' })
231 page.position({ latitude: 1 })
232 expect(calls).toBe(0)
233 })
234
235 it('does not let a legacy clear stop v1 subscribers sharing the native stream', async () => {
236 const page = loadPage()
237 let v1Calls = 0
238 let legacyCalls = 0
239 const id = page.craft.location.watchPosition(() => v1Calls++)
240 page.answer('watchPosition', true)
241 await Promise.resolve()
242
243 const legacyStarted = page.craft.geolocation.watchPosition(() => legacyCalls++)
244 expect(page.count('watchPosition')).toBe(1)
245 expect(await legacyStarted).toBe(true)
246 expect(await page.craft.geolocation.clearWatch()).toBe(true)
247 expect(page.count('clearWatch')).toBe(0)
248
249 page.position({ latitude: 1 })
250 expect(v1Calls).toBe(1)
251 expect(legacyCalls).toBe(0)
252 page.craft.location.clearWatch(id)
253 expect(page.count('clearWatch')).toBe(1)
254 })
255
181256 // Haptics are feedback. An app that left enableHaptics off must not have
182257 // `await haptics.selection()` stop the flow it sits in, which is how the
183258 // SDK documents it and how Android and the web fallback behave.
packages/ios/templates/CraftApp.swiftmodified+64-5
@@ -2125,10 +2125,10 @@ struct CraftWebView: UIViewRepresentable {
21252125 return this.geolocation.getCurrentPosition({});
21262126 },
21272127 watchPosition: function(callback) {
2128 return this.geolocation.watchPosition(callback);
2128 return this.location.watchPosition(callback);
21292129 },
21302130 clearWatch: function(watchId) {
2131 return this.geolocation.clearWatch(watchId);
2131 return this.location.clearWatch(watchId);
21322132 },
21332133 getContacts: function() {
21342134 return this._invoke('getContacts');
@@ -3253,21 +3253,80 @@ struct CraftWebView: UIViewRepresentable {
32533253 };
32543254
32553255 var locationWatchCallbacks = new Map();
3256 var legacyLocationWatchCallbacks = [];
32563257 var nextLocationWatchId = 0;
3258 var locationWatchStarted = false;
3259 var locationWatchStart = null;
3260 var locationWatchGeneration = 0;
3261
3262 function hasLocationWatchers() {
3263 return locationWatchCallbacks.size > 0 || legacyLocationWatchCallbacks.length > 0;
3264 }
3265
3266 function startLocationWatch() {
3267 if (locationWatchStarted) return Promise.resolve(true);
3268 if (locationWatchStart) return locationWatchStart;
3269
3270 var generation = locationWatchGeneration;
3271 var pending = craft._invoke('watchPosition');
3272 var managed = pending.then(function(result) {
3273 if (generation === locationWatchGeneration) {
3274 locationWatchStarted = hasLocationWatchers();
3275 }
3276 if (locationWatchStart === managed) locationWatchStart = null;
3277 return result;
3278 }, function(error) {
3279 if (generation === locationWatchGeneration) {
3280 locationWatchCallbacks.clear();
3281 legacyLocationWatchCallbacks.length = 0;
3282 locationWatchStarted = false;
3283 }
3284 if (locationWatchStart === managed) locationWatchStart = null;
3285 throw error;
3286 });
3287 locationWatchStart = managed;
3288 return managed;
3289 }
3290
3291 function stopLocationWatchIfUnused() {
3292 if (hasLocationWatchers()) return Promise.resolve(true);
3293 if (!locationWatchStarted && !locationWatchStart) return Promise.resolve(true);
3294
3295 locationWatchGeneration += 1;
3296 locationWatchStarted = false;
3297 locationWatchStart = null;
3298 return craft._invoke('clearWatch');
3299 }
3300
32573301 window.addEventListener('craftLocationUpdate', function(event) {
32583302 locationWatchCallbacks.forEach(function(callback) { callback(event.detail); });
3303 legacyLocationWatchCallbacks.slice().forEach(function(callback) { callback(event.detail); });
32593304 });
3305 legacyGeolocation.watchPosition = function(callback) {
3306 legacyLocationWatchCallbacks.push(callback);
3307 return startLocationWatch();
3308 };
3309 legacyGeolocation.clearWatch = function() {
3310 legacyLocationWatchCallbacks.length = 0;
3311 return stopLocationWatchIfUnused();
3312 };
32603313 craft.location = {
32613314 getCurrentPosition: function(options) { return legacyGeolocation.getCurrentPosition(options || {}); },
32623315 watchPosition: function(callback) {
32633316 var id = ++nextLocationWatchId;
32643317 locationWatchCallbacks.set(id, callback);
3265 if (locationWatchCallbacks.size === 1) void craft._invoke('watchPosition');
3318 void startLocationWatch().catch(function() {});
32663319 return id;
32673320 },
32683321 clearWatch: function(id) {
3269 locationWatchCallbacks.delete(id);
3270 if (locationWatchCallbacks.size === 0) void craft._invoke('clearWatch');
3322 if (typeof id === 'undefined') {
3323 locationWatchCallbacks.clear();
3324 legacyLocationWatchCallbacks.length = 0;
3325 void stopLocationWatchIfUnused().catch(function() {});
3326 return;
3327 }
3328 if (!locationWatchCallbacks.delete(id)) return;
3329 void stopLocationWatchIfUnused().catch(function() {});
32713330 },
32723331 startRecording: function() { return craft._invoke('startLocationRecording'); },
32733332 pauseRecording: function() { return craft._invoke('pauseLocationRecording'); },
packages/typescript/types/craft.d.tsmodified+3-3
@@ -249,13 +249,13 @@ export interface CraftBridge {
249249 * @param callback - Function called with new position
250250 * @returns Watch ID
251251 */
252 watchPosition(callback: (position: Position) => void): string;
252 watchPosition(callback: (position: Position) => void): number;
253253
254254 /**
255255 * Clear position watch
256 * @param watchId - Watch ID from watchPosition
256 * @param watchId - Watch ID from watchPosition; omit to clear every active watch
257257 */
258 clearWatch(watchId: string): void;
258 clearWatch(watchId?: number): void;
259259
260260 // ==================== Sensors ====================
261261
packages/zig/test/bridge_surface_test.zigmodified+21-0
@@ -175,3 +175,24 @@ test "every direct method craft.d.ts declares exists directly on both bridges" {
175175 return error.DeclaredMethodMissingFromBridge;
176176 }
177177}
178
179test "the flat location pair uses numeric handles and delegates to the v1 iOS surface" {
180 try testing.expect(std.mem.indexOf(
181 u8,
182 sdk_types,
183 "watchPosition(callback: (position: Position) => void): number;",
184 ) != null);
185 try testing.expect(std.mem.indexOf(u8, sdk_types, "clearWatch(watchId?: number): void;") != null);
186
187 const ios = craftObject(ios_spec) orelse return error.IosCraftObjectNotFound;
188 try testing.expect(std.mem.indexOf(
189 u8,
190 ios,
191 "watchPosition: function(callback) {\n return this.location.watchPosition(callback);",
192 ) != null);
193 try testing.expect(std.mem.indexOf(
194 u8,
195 ios,
196 "clearWatch: function(watchId) {\n return this.location.clearWatch(watchId);",
197 ) != null);
198}