ReviewOS

also looking at this

craft-native/craft

fix(ios): let share() settle the promise its native side answers

#208
Merged glennmichael123 wants to merge fix/share-settles-promise into main
7 files +302 -22
.github/workflows/README.mdmodified+2-2
Changes to .github/workflows/README.md
@@ -30,8 +30,8 @@ bun run test:mobile-e2e:ios
3030```
3131
3232```
33ok ios-shim — 4/4 cases, no zig
34ok ios-runtime — 4/4 cases, zig served clipboardRead, clipboardWrite, getCurrentPosition, getDeviceInfo, log
33ok ios-shim — 5/5 cases, no zig
34ok ios-runtime — 5/5 cases, zig served clipboardRead, clipboardWrite, getCurrentPosition, getDeviceInfo, log, share
3535```
3636
3737Android needs JDK 17, Gradle 8.11.1, an Android SDK with `platforms;android-36`
packages/ios/templates/CraftApp.swiftmodified+23-2
Changes to packages/ios/templates/CraftApp.swift
@@ -2008,7 +2008,28 @@ struct CraftWebView: UIViewRepresentable {
20082008 },
20092009
20102010 share: function(text) {
2011 window.webkit.messageHandlers.craft.postMessage({action: 'share', text: text});
2011 return this._share({text: text});
2012 },
2013
2014 // Both share entry points come through here, because the
2015 // native side answers them the same way: `true` when the
2016 // person finished an activity, `false` when they dismissed
2017 // the sheet, a rejection when sharing is disabled or there is
2018 // nothing to share. The flat `share` used to post with no
2019 // callbackId, so every one of those answers stopped at the nil
2020 // guard in resolveCallback and the page got `undefined`.
2021 //
2022 // No timeout, unlike `_invoke`'s thirty seconds. The sheet
2023 // waits on a person, and someone slower than that to pick an
2024 // app would be told the share failed while it was still on
2025 // screen, with the real answer then dropped.
2026 _share: function(payload) {
2027 var self = this;
2028 var id = 'cb_' + (++this._callbackId);
2029 window.webkit.messageHandlers.craft.postMessage(Object.assign({}, payload, {action: 'share', callbackId: id}));
2030 return new Promise(function(resolve, reject) {
2031 self._callbacks[id] = {resolve: resolve, reject: reject};
2032 });
20122033 },
20132034
20142035 openCamera: function() {
@@ -2919,7 +2940,7 @@ struct CraftWebView: UIViewRepresentable {
29192940 }
29202941 };
29212942 var shareApi = function(text) { return legacyShare(text); };
2922 shareApi.share = function(options) { return craft._invoke('share', {options: options || {}}); };
2943 shareApi.share = function(options) { return craft._share({options: options || {}}); };
29232944 craft.share = shareApi;
29242945 craft.lifecycle = {
29252946 getState: function() { return document.visibilityState === 'visible' ? 'active' : 'background'; },
packages/zig/test/ios_conformance_test.zigmodified+229-0
Changes to packages/zig/test/ios_conformance_test.zig
@@ -1643,3 +1643,232 @@ test "the SDK's event map declares everything that does fire" {
16431643 }
16441644 }
16451645}
1646
1647// ## Answers the page never receives
1648//
1649// The other half of the `ota*` bug that opens this file. That was a promise
1650// with nothing to answer it; this is an answer with no promise to take it.
1651// `resolveCallback` and `rejectCallback` both begin
1652// `guard let id = callbackId else { return }`, so a page method that posts
1653// without a callbackId throws away whatever the dispatcher says, and the page
1654// gets `undefined`.
1655//
1656// `share` was one (#203). Its case resolves whether the person shared and
1657// rejects `CAPABILITY_DISABLED` and `INVALID_ARGUMENT`, and `craft.share(text)`
1658// posted `{action: 'share', text}`, so an app with sharing switched off called
1659// it, heard nothing and carried on as if it had worked.
1660
1661/// Actions the page still posts without a callbackId even though the spec
1662/// answers them. Tracked in #207. This list may only shrink: the second test
1663/// below fails when one of them is fixed and left here.
1664const unanswered_posts = [_][]const u8{
1665 "haptic",
1666 "startListening",
1667 "watchPosition",
1668 "clearWatch",
1669 "vibrate",
1670};
1671
1672/// One `postMessage(...)` call in the page script.
1673const Post = struct {
1674 /// The literal action it names, or null when the message is built
1675 /// elsewhere — `_invoke` posts a variable, and always adds a callbackId.
1676 action: ?[]const u8,
1677 carries_callback: bool,
1678};
1679
1680/// The `postMessage(` call whose opening parenthesis is at `open`, with its
1681/// argument read to the matching parenthesis.
1682///
1683/// Balanced rather than cut at the first `}` or `)`. A cut is what the first
1684/// version of this scan did, and it misread
1685/// `{action: 'startAR', options: options || {}, callbackId: id}` as a post
1686/// with no callbackId, because the `{}` ended the object early.
1687fn postAt(source: []const u8, open: usize) ?Post {
1688 var depth: usize = 0;
1689 var i = open;
1690 while (i < source.len) : (i += 1) {
1691 switch (source[i]) {
1692 '(' => depth += 1,
1693 ')' => {
1694 depth -= 1;
1695 if (depth == 0) {
1696 const argument = source[open + 1 .. i];
1697 return .{
1698 .action = literalAction(argument),
1699 .carries_callback = std.mem.indexOf(u8, argument, "callbackId") != null,
1700 };
1701 }
1702 },
1703 else => {},
1704 }
1705 }
1706 return null;
1707}
1708
1709fn literalAction(argument: []const u8) ?[]const u8 {
1710 const needle = "action: '";
1711 const at = std.mem.indexOf(u8, argument, needle) orelse return null;
1712 const start = at + needle.len;
1713 const end = std.mem.indexOfScalarPos(u8, argument, start, '\'') orelse return null;
1714 return argument[start..end];
1715}
1716
1717/// Iterates every `postMessage(` call in `source`.
1718const PostIterator = struct {
1719 source: []const u8,
1720 search: usize = 0,
1721
1722 const needle = "postMessage(";
1723
1724 fn next(self: *PostIterator) ?Post {
1725 while (std.mem.indexOfPos(u8, self.source, self.search, needle)) |at| {
1726 self.search = at + needle.len;
1727 if (postAt(self.source, at + needle.len - 1)) |post| return post;
1728 }
1729 return null;
1730 }
1731};
1732
1733/// Whether the dispatcher's `case` for `action` answers through the callback.
1734///
1735/// The case runs from its label to the next line at the same indentation that
1736/// starts another case, a `default:` or the closing brace. Nested switches sit
1737/// deeper and stay inside it. Comment lines are skipped so a remark that
1738/// mentions callbackId cannot pass for code that uses it.
1739fn specAnswers(region: []const u8, action: []const u8) bool {
1740 var lines = std.mem.splitScalar(u8, region, '\n');
1741 var case_indent: ?usize = null;
1742 while (lines.next()) |line| {
1743 const body = std.mem.trimStart(u8, line, " \t");
1744 if (body.len == 0) continue;
1745 const indent = line.len - body.len;
1746
1747 if (case_indent) |at| {
1748 const closes = indent <= at and (std.mem.startsWith(u8, body, "case ") or
1749 std.mem.startsWith(u8, body, "default") or body[0] == '}');
1750 if (closes) return false;
1751 if (std.mem.startsWith(u8, body, "//")) continue;
1752 if (std.mem.indexOf(u8, body, "callbackId") != null) return true;
1753 continue;
1754 }
1755
1756 if (std.mem.startsWith(u8, body, "case ") and labels(body, action)) case_indent = indent;
1757 }
1758 return false;
1759}
1760
1761/// Whether a `case` line lists `action` among its labels.
1762fn labels(case_line: []const u8, action: []const u8) bool {
1763 var search: usize = 0;
1764 while (std.mem.indexOfPos(u8, case_line, search, action)) |at| {
1765 search = at + action.len;
1766 if (at == 0 or case_line[at - 1] != '"') continue;
1767 if (search < case_line.len and case_line[search] == '"') return true;
1768 }
1769 return false;
1770}
1771
1772fn isRecordedUnanswered(action: []const u8) bool {
1773 for (unanswered_posts) |recorded| {
1774 if (std.mem.eql(u8, recorded, action)) return true;
1775 }
1776 return false;
1777}
1778
1779test "the post scan tells a call that waits from one that does not" {
1780 // Shown both answers before it is trusted with the real file. A scanner
1781 // that finds nothing proves nothing.
1782 const cases = [_]struct { line: []const u8, action: ?[]const u8, waits: bool }{
1783 .{ .line = "postMessage({action: 'share', text: text});", .action = "share", .waits = false },
1784 .{ .line = "postMessage(Object.assign({}, payload, {action: 'share', callbackId: id}));", .action = "share", .waits = true },
1785 .{ .line = "postMessage({action: 'startAR', options: options || {}, callbackId: id});", .action = "startAR", .waits = true },
1786 .{ .line = "postMessage(message);", .action = null, .waits = false },
1787 };
1788 for (cases) |expected| {
1789 var it = PostIterator{ .source = expected.line };
1790 const post = it.next() orelse return error.ScanMissedAPost;
1791 try testing.expect(it.next() == null);
1792 if (expected.action) |name| {
1793 try testing.expectEqualStrings(name, post.action orelse return error.ScanMissedTheAction);
1794 } else {
1795 try testing.expect(post.action == null);
1796 }
1797 try testing.expectEqual(expected.waits, post.carries_callback);
1798 }
1799
1800 const dispatcher =
1801 \\ case "stopListening":
1802 \\ stopSpeechRecognition()
1803 \\ case "haptic", "buzz":
1804 \\ switch style {
1805 \\ case "heavy":
1806 \\ break
1807 \\ default:
1808 \\ // callbackId is not used here
1809 \\ break
1810 \\ }
1811 \\ rejectCallback(callbackId, error: "Haptics is disabled")
1812 \\ case "log":
1813 \\ print(message)
1814 \\ }
1815 ;
1816 try testing.expect(specAnswers(dispatcher, "haptic"));
1817 try testing.expect(specAnswers(dispatcher, "buzz"));
1818 try testing.expect(!specAnswers(dispatcher, "stopListening"));
1819 try testing.expect(!specAnswers(dispatcher, "log"));
1820 try testing.expect(!specAnswers(dispatcher, "hap"));
1821}
1822
1823test "every call the spec answers carries a callbackId to answer it on" {
1824 const region = dispatcherRegion();
1825 var it = PostIterator{ .source = swift_spec };
1826 var literal_posts: usize = 0;
1827 while (it.next()) |post| {
1828 const action = post.action orelse continue;
1829 literal_posts += 1;
1830 if (post.carries_callback) continue;
1831 if (!specAnswers(region, action)) continue;
1832 if (isRecordedUnanswered(action)) continue;
1833
1834 std.debug.print(
1835 "the page posts `{s}` with no callbackId, and the spec answers it.\n" ++
1836 " resolveCallback and rejectCallback return early on a nil id, so the\n" ++
1837 " answer is dropped and the page gets undefined. Post a callbackId and\n" ++
1838 " return the promise, the way openCamera does.\n",
1839 .{action},
1840 );
1841 return error.PageDropsTheSpecsAnswer;
1842 }
1843 // Non-vacuity: a renamed handler or reshaped post would find nothing.
1844 try testing.expect(literal_posts >= 40);
1845}
1846
1847test "every recorded unanswered post is real, and still unanswered" {
1848 const region = dispatcherRegion();
1849 for (unanswered_posts) |action| {
1850 if (!specAnswers(region, action)) {
1851 std.debug.print(
1852 "`{s}` is recorded as unanswered, but the spec no longer answers it.\n" ++
1853 " Take it off unanswered_posts.\n",
1854 .{action},
1855 );
1856 return error.RecordedPostIsNotAnswered;
1857 }
1858
1859 var it = PostIterator{ .source = swift_spec };
1860 const still_dropped = while (it.next()) |post| {
1861 const name = post.action orelse continue;
1862 if (std.mem.eql(u8, name, action) and !post.carries_callback) break true;
1863 } else false;
1864
1865 if (!still_dropped) {
1866 std.debug.print(
1867 "every post of `{s}` carries a callbackId now.\n" ++
1868 " Take it off unanswered_posts so it cannot regress unnoticed.\n",
1869 .{action},
1870 );
1871 return error.RecordedPostIsFixed;
1872 }
1873 }
1874}
scripts/mobile-e2e/driver.htmlmodified+17-0
Changes to scripts/mobile-e2e/driver.html
@@ -181,6 +181,23 @@
181181 })
182182 }
183183 },
184 {
185 // Sharing is off too, and this is the call that used to answer
186 // nothing: `craft.share(text)` posted no callbackId, so Swift's
187 // CAPABILITY_DISABLED stopped at a nil guard and the page got
188 // `undefined`, which a caller awaiting it reads as success (#203).
189 // The refusal settles long before any sheet could appear, so the case
190 // needs nobody at the simulator.
191 name: 'share.disabled.rejects',
192 platforms: ['ios'],
193 run: function () {
194 return withTimeout(function () { return window.craft.share('craft e2e ' + NONCE) }).then(function (outcome) {
195 if (outcome.state === 'timeout') return 'share never settled'
196 if (outcome.state === 'resolved') return 'share resolved with ' + JSON.stringify(outcome.value) + ' while sharing is disabled'
197 if (outcome.error.code !== 'CAPABILITY_DISABLED') return 'code is ' + JSON.stringify(outcome.error.code)
198 })
199 }
200 },
184201 {
185202 // Android's equivalent gate. With push disabled the generator emits a
186203 // literal reject call rather than a Firebase block, so the rejection
scripts/mobile-e2e/ios.tsmodified+13-9
Changes to scripts/mobile-e2e/ios.ts
@@ -2,22 +2,26 @@ import type { LegOutcome, RunnerOptions } from './types'
22import { closeSync, copyFileSync, existsSync, mkdirSync, openSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
33import { join } from 'node:path'
44import { bootSimulator, init, pickSimulator } from '../../packages/ios/src/index'
5import { evaluateRun, hasTerminated, ZIG_REFUSED_ACTION, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigRefusals } from './protocol'
5import { evaluateRun, hasTerminated, ZIG_REFUSED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigRefusals } from './protocol'
66import { command, driverPage, waitForFile } from './support'
77
88/**
99 * The app the suite runs against.
1010 *
11 * Clipboard on, geolocation off, and that pairing is the whole point: the
12 * success case needs a capability that is enabled and reachable without a
13 * permission prompt, and the rejection case needs one that is switched off so
14 * the refusal is a property of the configuration rather than of the machine.
15 * Reading a pasteboard the app itself just wrote raises no iOS paste prompt,
16 * which is what makes the round trip scriptable.
11 * Clipboard on, geolocation and sharing off, and that pairing is the whole
12 * point: the success case needs a capability that is enabled and reachable
13 * without a permission prompt, and the rejection cases need ones that are
14 * switched off so the refusal is a property of the configuration rather than
15 * of the machine. Reading a pasteboard the app itself just wrote raises no iOS
16 * paste prompt, which is what makes the round trip scriptable.
17 *
18 * `enableShare` is spelled out although false is the default, because the
19 * share case depends on it and a default is not something this file controls.
1720 */
1821const CONFIG = {
1922 enableClipboard: true,
2023 enableGeolocation: false,
24 enableShare: false,
2125}
2226
2327const BUNDLE_ID = 'dev.craft.e2e.probe'
@@ -172,8 +176,8 @@ async function runLeg(leg: Leg, options: RunnerOptions): Promise<LegOutcome> {
172176 // the action, so it alone does not prove Zig answered. The refusal line is
173177 // written by Zig's own capability gate, which means the rejection the page
174178 // saw came off Zig's error route.
175 if (!refused.includes(ZIG_REFUSED_ACTION))
176 failures.push(`Zig never refused ${ZIG_REFUSED_ACTION}; the rejection the page saw came from Swift, not from the Zig gate`)
179 for (const action of ZIG_REFUSED_ACTIONS.filter(action => !refused.includes(action)))
180 failures.push(`Zig never refused ${action}; the rejection the page saw came from Swift, not from the Zig gate`)
177181 }
178182 else {
179183 if (dispatched.length)
scripts/mobile-e2e/protocol.test.tsmodified+11-7
Changes to scripts/mobile-e2e/protocol.test.ts
@@ -1,7 +1,7 @@
11import { describe, expect, it } from 'bun:test'
22import { readFileSync } from 'node:fs'
33import { join } from 'node:path'
4import { ANDROID_DECLINE_PHRASES, androidDeclines, evaluateRun, hasTerminated, parseDriverOutput, REQUIRED_CASES, requiredCaseProblems, ZIG_REFUSED_ACTION, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigRefusals } from './protocol'
4import { ANDROID_DECLINE_PHRASES, androidDeclines, evaluateRun, hasTerminated, parseDriverOutput, REQUIRED_CASES, requiredCaseProblems, ZIG_REFUSED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigRefusals } from './protocol'
55
66const ESC = String.fromCharCode(27)
77
@@ -151,14 +151,14 @@ describe('evaluateRun', () => {
151151
152152 it('fails when the tally disagrees with the transcript', () => {
153153 const text = [
154 'CRAFT-E2E {"event":"plan","platform":"ios","cases":["bridge.ready","deviceInfo.isSimulator","clipboard.roundTrip","geolocation.disabled.rejects"]}',
154 `CRAFT-E2E ${JSON.stringify({ event: 'plan', platform: 'ios', cases: REQUIRED_CASES.ios })}`,
155155 ...REQUIRED_CASES.ios.map(name => `CRAFT-E2E {"event":"case","name":"${name}","status":"pass","detail":""}`),
156156 'CRAFT-E2E {"event":"done","passed":99,"failed":0}',
157157 ].join('\n')
158158
159159 const verdict = evaluateRun('ios', text)
160160 expect(verdict.ok).toBe(false)
161 expect(verdict.failures).toContain('done says 99 passed / 0 failed, transcript shows 4 / 0')
161 expect(verdict.failures).toEqual([`done says 99 passed / 0 failed, transcript shows ${REQUIRED_CASES.ios.length} / 0`])
162162 })
163163
164164 it('fails when the app is not the platform the leg was built for', () => {
@@ -270,12 +270,16 @@ describe('zig attribution', () => {
270270 })
271271
272272 it('reads the refusal Zig writes through its own capability gate', () => {
273 const text = 'info: ios: refusing getCurrentPosition; enableGeolocation is not enabled in craft.config.json'
274 expect(zigRefusals(text)).toEqual([ZIG_REFUSED_ACTION])
273 const text = [
274 'info: ios: refusing share; enableShare is not enabled in craft.config.json',
275 'info: ios: refusing getCurrentPosition; enableGeolocation is not enabled in craft.config.json',
276 ].join('\n')
277 expect(zigRefusals(text)).toEqual(ZIG_REFUSED_ACTIONS)
275278 })
276279
277 it('expects a refusal for an action the suite actually exercises', () => {
278 expect(ZIG_TESTED_ACTIONS).toContain(ZIG_REFUSED_ACTION)
280 it('expects refusals only for actions the suite actually exercises', () => {
281 for (const action of ZIG_REFUSED_ACTIONS)
282 expect(ZIG_TESTED_ACTIONS).toContain(action)
279283 })
280284})
281285
scripts/mobile-e2e/protocol.tsmodified+7-2
Changes to scripts/mobile-e2e/protocol.ts
@@ -67,6 +67,7 @@ export const REQUIRED_CASES: Record<MobilePlatform, string[]> = {
6767 'deviceInfo.isSimulator',
6868 'clipboard.roundTrip',
6969 'geolocation.disabled.rejects',
70 'share.disabled.rejects',
7071 ],
7172 android: [
7273 'bridge.ready',
@@ -353,10 +354,14 @@ export const ZIG_TESTED_ACTIONS = [
353354 'clipboardWrite',
354355 'getCurrentPosition',
355356 'getDeviceInfo',
357 'share',
356358]
357359
358/** The one action the suite configures off, so Zig must refuse it by name. */
359export const ZIG_REFUSED_ACTION = 'getCurrentPosition'
360/** The actions the suite configures off, so Zig must refuse each by name. */
361export const ZIG_REFUSED_ACTIONS = [
362 'getCurrentPosition',
363 'share',
364]
360365
361366/**
362367 * The three ways an Android native says it gave up, as `android_dispatch.zig`