ReviewOS

also looking at this

craft-native/craft

feat(prefs): a Settings… item and a preference store that keeps its types

#58
Merged glennmichael123 wants to merge feat/settings-and-prefs into main
14 files +2289 -10
11 days ago

Closes #51. Two halves of the same convention, both missing.

Authored from a 10-agent survey/design workflow; the two decisions that actually shaped the result came out of running code rather than reading docs, and both cut against the obvious implementation. They're in §"Two things I got wrong first" below.

Settings…

The default App menu had About/Hide/Quit and no Settings…, which every Mac app has at ⌘,. Verified in the real menu bar, not just in the table:

[0] About …        key=    enabled=true  target=false
[1] (separator)
[2] Settings…      key=,   enabled=true  target=true
[3] (separator)
[4] Hide …
...
--- performActionForItemAtIndex:2 ---
[Menu] Settings chosen but no webview to notify: error.NoWebView

enabled=true is the assertion that matters — AppKit greys out an item whose action nobody implements, which is how craft shipped a bar of grey text once before (#29). performActionForItemAtIndex: is what a click does, so that last line is the callback actually firing.

It gets its own craft:settings:open event, not a reserved craft:menu:action id. Menu ids are passed through verbatim with no interception point anywhere in the bridge, so "reserved" would be a convention nothing enforces and an app-declared item could shadow it; and an app that never calls craft.menu.set() shouldn't have to subscribe to the menu API to catch ⌘,. craft.settings.open('toolbar') reaches the same handler from a gear button without crossing the bridge.

craft.menu.set() still replaces the whole bar, as documented — silently injecting into a bar craft didn't design would contradict that. Apps declare { id: 'settings', role: 'settings', … } to keep it, and the same target answers either way.

craft.prefs

The issue points at storage.zig's commented-out NSUserDefaults code. I did not build on it: its load()/save() bodies are Objective-C pseudocode in comments, it's unreachable from the binary, it stores caller slices verbatim and frees nothing, and its load() clears the dirty flag that save() early-outs on — so a set-then-load would discard the write the moment save() became real. Reviving it would have meant two sources of truth, an in-memory map and the preferences database, with nothing keeping them agreeing. It's still there, still dead; removing it is its own PR.

Strings, numbers and booleans, as native plist types under a craft.prefs. prefix. End-to-end through the real bridge:

craft.prefs.theme      Type is string
craft.prefs.darkMode   Type is boolean
craft.prefs.fontSize   Type is integer
craft.prefs.scale      Type is float

Refusing containers isn't a corner cut — it's what makes a crash unreachable. The preferences API raises an Objective-C exception for a non-plist value, Zig can't catch one, and the value comes from page JavaScript. With three scalar constructors there is no code path that can build a CFArray out of page input.

Two things I got wrong first

The convenience APIs read argv as a preferences domain. Measured:

$ ./probe -zinjected 99
CopyAppValue(zinjected): CFString     ← "99"
CopyValue   (zinjected): null

Same for NSUserDefaults, including a suite made with initWithSuiteName: — my first sketch used exactly that and would have shipped the bug. craft's entire interface is flags, so craft --title Hello would have made prefs.get('title') return "Hello", permanently: an argument-domain value can't be removed at runtime. The exact-domain CFPreferences quad is immune.

objCType can't tell a stored boolean from the number 1. The usual idiom compares it to @encode(BOOL), which is "B" on arm64 while [@YES objCType] is "c" — it matches nothing, on any Mac. CFGetTypeID(v) == CFBooleanGetTypeID() is correct in-process and after a round trip through the daemon. The native test writes and reads in the same process on purpose: a test that relaunches passes even with a wrong implementation, because the plist round trip normalises number types in a fresh process.

Wire actions are namespaced

prefs:get, never a bare get. The pending-reply queue is keyed by action name alone: a bare get would be drained by keychain.get and tags.get, and a bare set/delete/clear would be resolved by keychain, crash-reporter and tags, all of which emit results under those bare names even though their own facades never wait for one. Qualifying the action keeps _req's 30-second reaper, unlike the one existing dodge in the tree which hand-rolls its promise and loses it.

An unrecognised action is filed under a constant rather than echoed back — sendErrorToJS interpolates the action into a single-quoted JS literal with no escaping, and craft.invoke lets a page choose the bytes.

Tests

45 new, across three layers, all control-checked:

  • prefs.zig — the rules, against an in-memory backend, on any host. Includes that clear() leaves NSWindow Frame … alone, that it syncs once rather than once per key, and that a returned string is allocator-owned (the whole suite runs under the testing allocator, so the borrowed-slice bug storage.zig shipped would show up).
  • prefs_macos.zig — the real preferences daemon, against a throwaway domain that's removed in teardown. defaults read dev.craft.prefs.selftest → does not exist, including after a deliberately failing run. Control: flipping the read dispatch to the wrong order fails the boolean test.
  • injected_js_test.zig — the real craft-bridge.js under zig-js, decoded by the real native decoder. Control: un-namespacing the JS actions fails 4; deleting the settings dispatch fails 1.

zig build test clean, bun test 471 pass, Windows cross-build clean (that's the check CI caught me on in #56), tsc --noEmit clean, pickier clean with no new warnings in craft-bridge.js.

Not done, deliberately

  • No onChange/watch. evalJS resolves to the last window created, so a broadcast would reach one window and silently miss the others. Shipping a subscription on that transport is shipping a bug.
  • No has() (it's get(k) !== undefined) and no getAll() (it's keys() plus N round trips, and immediately raises a partial-failure question).
  • No nested values. The refusal message names the two-line JSON.stringify fix at the call site.

One thing worth knowing: an unbundled dev binary writes to a domain named after the executable, so every dev-mode app through the shared craft binary shares one, and a packaged .app uses its bundle id. prefs.info() reports the live domain and a copy-pasteable defaults read command rather than leaving that to be discovered.

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

Sign in to comment on this pull request.