ReviewOS

also looking at this

craft-native/craft

fix(ios): no gated dispatcher arm can hang the page any more (58 → 0)

#120
Merged glennmichael123 wants to merge fix/ios-gated-arms-reject into main
1 file +298 -106
10 days ago

Closes #116. 58 gated dispatcher arms could leave the page's promise unsettled. Zero can now.

An arm shaped like this settles nothing when its gate is off:

case "openCamera":
    if config.enableCamera {
        pendingCallbackId = callbackId
        openCamera()
    }

Not a slow rejection — these wrappers do not all carry the 30-second timeout, so the promise never settles at all. enableCamera and friends default to false, so this is the default path for any capability the app has not opted into.

The count was wrong twice before it was right

inherited estimate"~60 of 65"
first measurement57 hang / 8 correct
after brace-tracking58 hang / 7 correct

The first pass counted an inner else (a payload check) as handling the gate. share has both and is genuinely correct; others have only the inner one and still hang.

Two commits

30 gate-only arms get the else the seven correct arms already use — secureClear and share are the models, and CAPABILITY_DISABLED is the code Zig's own capability gate answers, so both arms agree for an action served either way.

28 arms that bind payload inside the gate need the gate split out first, because one else cannot distinguish "turn this on in craft.config.json" from "you sent the wrong payload":

if config.enableSecureStorage {
    if let key = …, let value = … {
        …
    } else {
        rejectCallback(callbackId, error: "…", code: "INVALID_ARGUMENT")
    }
} else {
    rejectCallback(callbackId, error: "Secure storage is disabled", code: "CAPABILITY_DISABLED")
}

Four were done by hand: startListening (body on the brace line), clipboardWrite (gate and binding on one line), and downloadFile/saveFile (an enableFileDownload flag the message table missed).

Two mistakes the sweep made, caught before commit

  • Deriving messages from flag names produced "Secure storage is disabled is disabled" — 17 of them — because the phrase already carried the suffix. It also turned enableAR into "A r" and enableNFC into "N f c", so all 30 messages are written out rather than generated.
  • The arm-range walk swallowed six // MARK: section comments. They are duplicated elsewhere in the file, so a naive presence check said they were fine; only comparing occurrence counts against HEAD found it. All 96 are restored.

Verification

  • git diff -w removes nothing but the if config.enable…, and let … { lines being restructured — no behaviour line lost.
  • The generated app typechecks against the iphonesimulator SDK: 0 errors, and exactly the same 4 warnings as HEAD (I compared by stashing and regenerating, because my first "8 → 4" reading was a raw line count, not distinct warnings).
  • zig build test green — the conformance test reads this file.

Related: #115 is a different silent-failure shape in the same file.

1 changed file on the files tab, with 0 review threads.

Sign in to comment on this pull request.