ReviewOS

also looking at this

craft-native/craft

fix(ios): answer a malformed argument instead of leaving the page waiting

#246
Merged glennmichael123 wants to merge fix/ios-invalid-argument into main
2 files +136 -0
9 days ago

Closes #221.

What was wrong

case "setBadge":
    if let count = body["count"] as? Int {
        setBadgeCount(count, callbackId: callbackId)
    }

No else. The page posted a callbackId and got a promise, and craft.setBadge('3') settled nothing at all — it waited out its own 30s timeout with a message naming no framework and no code.

Zig serves all of these first, so the Swift path is what runs in an app built without the Zig runtime, and what a Zig build falls back to when CraftZigRuntime.offer returns false because the payload will not serialize.

Thirteen cases, not three

The issue named openURL, setBadge and setFlashlight; its comment added the Siri pair. The scan found eight more, which is the argument for writing the scan rather than fixing the five by hand:

setSharedItem, getSharedItem, removeSharedItem, setShortcuts, setBiometricPersistence, updateWidget, sendToWatch, updateWatchContext.

All thirteen now reject with INVALID_ARGUMENT — the code share already uses for nothing to share — and a message naming what the call needs.

The ratchet

ios_conformance_test.zig reads each case the page waits on and fails any whose if let closes without an else:

  • Only if lets at the case's own indentation are read. One nested inside a closure belongs to that closure's contract, not to this dispatch.
  • The else does not have to reject. caseUsesCallback is already what checks a case answers at all; this checks there is a branch to answer from.
  • Against a recorded exception list that may only shrink, as the issue asked. It is empty as of this change.
  • It reports every gap rather than the first. That mattered immediately: the first run named only setSharedItem, and collecting them all is what turned five known cases into thirteen.

caseBody shares its boundary rule with caseUsesCallback — from the label to the next line at or above its indentation that starts another case, a default: or the closing brace — so nested switches stay inside the case they belong to.

Tests that fail without the fix

Removing setBadge's else again fails the scan, and the failure names three cases rather than one, because the if let it reopens swallows the two that follow it in the same switch. That is the scan working: a missing branch changes where the case is judged to end.

Verification

zig build test and zig build test:ios, Pickier (0 errors), the ios/android package tests (154), and packages/ios/scripts/typecheck-template.sh — the generated Swift typechecks with no new warnings.

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

Sign in to comment on this pull request.