ReviewOS

also looking at this

craft-native/craft

feat(notifications): make the banner answerable — clicks, buttons, and a crash

#74
Merged glennmichael123 wants to merge feat/notification-actions into main
8 files +942 -17
11 days ago

Closes #65. Both stages.

Stacked on #73 (feat/app-delegate) — it needs that PR's install point, because Apple requires the notification delegate to be set before the app finishes launching. Base will retarget to main once #73 lands.

craft posted real UNUserNotificationCenter banners and registered no delegate, so nothing came back. Both stages are here, plus three things found on the way that had to be fixed for either stage to be usable at all.

A crash, first

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException',
    reason: 'bundleProxyForCurrentProcess is nil'
exit: 134   (SIGABRT)

[UNUserNotificationCenter currentNotificationCenter] aborts the process when it has no bundle identifier — which is every unbundled craft run, i.e. the entire dev loop. A page calling craft.notifications.show() took the whole app down.

The existing comment said the call "can crash if called too early". The condition is the bundle, not the timing. Every path that touches the notification centre now checks first; an unbundled run warns and carries on. Confirmed with a twelve-line Objective-C probe rather than assumed — and verified headlessly that craft now survives the same call.

show did not show

craft.notifications.show() routes through the schedule action, whose native delay defaults to 60 seconds. Every show() arrived a minute later — from the page, indistinguishable from one that never arrived. show now sends delay: 0, and a non-positive delay posts with a nil trigger, which is how UserNotifications spells "now".

(This is also why showNotification — where the dead actionId doc lived — is unreachable through the facade.)

Banners were suppressed when the app was focused

willPresentNotification: was missing, so macOS suppressed the banner whenever the app was frontmost. That looks exactly like a broken notification and is the first thing anyone testing this hits.

Stage 1 — click-through

didReceiveNotificationResponse: turns a response into craft:notification:click ({ notificationId }) or craft:notification:action ({ notificationId, actionId }).

A swipe-away arrives as an action with actionId: 'dismiss'. The Approve/Deny case the issue describes needs to hear that there will be no answer, and Apple delivers that through the same callback — so it costs one switch arm rather than a new channel.

Stage 2 — action buttons

Buttons live on a UNNotificationCategory. Because setNotificationCategories: replaces rather than adds, categories cannot be made per notification and discarded — they are keyed by their shape and reused.

That identity is pure and tested. Same buttons in the same order → same category; a relabelled button, a reordered pair, or a moved field boundary → a different one. The last is a real hazard, not a hypothetical: plain concatenation hashes {"ab","c"} and {"a","bc"} identically, and that collision puts one notification's buttons on another's banner. Fields are length-prefixed, with a test for exactly that.

Capped at four buttons, because macOS shows two on a banner and hides the rest behind an "Options" disclosure — registering more would be buttons nobody finds.

Two dead APIs, handled differently

what it waswhat happened
actionId on showdocumented in a comment, never parseddeleted — with real buttons a response names the button, whose id the app already chose
SDK actions?: [{action, title}]published on NotificationOptions, never implementedhonoured — both spellings accepted, {id, label} canonical

The difference is that one was a comment and the other was a published type someone could have compiled against. Deleting the second would break a promise craft made; accepting it makes the promise true.

How it's tested

Driven in a real JS engine through zig-js, against the actual bridge: that show() sends delay: 0, that buttons arrive in the shape the native struct decodes (decoded with that struct, so a rename on either side fails here), that both events reach their handlers, and that the strings the page subscribes to are the ones the capability channels declare.

Control-checked: sabotaging the delay fails the suite, and so does sabotaging a channel name.

The pure category/response logic has 14 tests of its own.

Verified

zig build · zig build test · zig build test-js · x86_64-windows cross-build · zig fmt --check · tsc --noEmit · 498 bun tests · pickier 38 warnings, unchanged from main.

Not covered

Clicking a real banner needs a signed, bundled app and a human hand, so the delivery path is proven by construction (delegate installed, selector and type encodings registered, events emitted through the same Channel the page subscribes to) rather than by a click. Everything up to and including the emitted event string is tested.

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

Sign in to comment on this pull request.