feat(macos): persisted settings store via electron-store#32198
Conversation
LUM-1846. Replaces the renderer's `window.vellum.settings.{get,set}`
stubs (Promise.reject) with a real implementation backed by
electron-store. Settings live in the OS-canonical userData path with
atomic file writes; the renderer reads/writes via the same generic
contextBridge API the scaffold already declared.
Schema (validated at write time):
- `hotkeys: Record<string, string>` — key bindings keyed by action name
- `theme: "light" | "dark" | "system"` — color-scheme preference
- `windowState: { x?, y?, width?, height? }` — placeholder for window
geometry restore in a future ticket
- `featureFlags: Record<string, boolean>` — local overrides
The four categories come from the LUM-1846 spec; specific hotkey actions
and window-restore logic land in the feature tickets that consume them.
electron-store rejects writes that don't match the schema (validated by
ajv under the hood), surfacing as a rejected Promise to the renderer.
Wiring:
- New `apps/macos/src/main/settings.ts` owns the store instance + schema.
- Main process registers `vellum:settings:get` / `vellum:settings:set`
IPC handlers under `installSettingsIpc()` during `whenReady`.
- Preload's `settings.{get,set}` now invoke those handlers via
`ipcRenderer.invoke`. The bridge surface in `VellumBridge` is
unchanged — generic `get<T>(key)` / `set<T>(key, value)`.
- `apps/web/src/runtime/is-electron.ts` extends the ambient
`Window.vellum` declaration to include the settings methods so
renderer consumers in `apps/web` get types.
Bundling: electron-store and its `conf` parent are ESM-only. Excluding
them from electron-vite's `externalizeDepsPlugin` bundles their ESM
source into the main-process CJS bundle, where the default-export
interop is handled at bundle time. Without this, `require("electron-store")`
returns the module namespace and `new Store(...)` fails with
"Store is not a constructor" at runtime.
tsconfig: `module`/`moduleResolution` bumped to `Preserve`/`Bundler` so
TypeScript reads conf's `exports` field and resolves the inherited
get/set signatures correctly.
Verified locally with a headless smoke test that exercises the IPC
end-to-end: theme + hotkeys roundtripped through set→get, and a
schema-violating write was correctly rejected by ajv.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58897689c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| const store = (): Store<AppSettings> => { | ||
| if (!instance) { | ||
| instance = new Store<AppSettings>({ schema }); |
There was a problem hiding this comment.
The store is created with only schema, but no rootSchema that disables extra top-level properties, while writeSetting accepts arbitrary string keys via a cast. In conf/electron-store, this means unexpected keys are accepted unless you explicitly close the root object, so renderer typos (for example them instead of theme) get persisted silently instead of failing validation. That breaks the declared four-key settings contract and can cause downstream consumers or migrations to miss bad data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real bug — fixed in 51ed3fa. The Store now passes rootSchema: { additionalProperties: false } so unknown top-level keys (e.g. settings.set("them", "dark") typo'd from "theme") are rejected by ajv at write time instead of silently persisted. Per-key shape validation via the existing schema is unchanged. Verified the option lands in the bundled main process.
Generated by Claude Code
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| "module": "Preserve", | ||
| "moduleResolution": "Bundler", |
There was a problem hiding this comment.
🚩 Module resolution changed from CommonJS/Node to Preserve/Bundler — deviates from apps/ convention
The apps/AGENTS.md convention states "TypeScript imports use .js extensions (NodeNext module resolution)." This PR changes tsconfig.json from "module": "CommonJS" / "moduleResolution": "Node" to "module": "Preserve" / "moduleResolution": "Bundler". Note that the previous state also wasn't NodeNext — it was CommonJS/Node. The change to Bundler is justified: electron-store v11 (and its conf parent) are ESM-only, and Bundler resolution matches how electron-vite/Rollup actually resolves imports during bundling. The .js extension convention IS still followed in all imports (e.g. apps/macos/src/main/index.ts:7). Other apps like apps/web/tsconfig.json use NodeNext. Consider updating apps/AGENTS.md to acknowledge that Electron apps may use Bundler resolution when their bundler handles module interop.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good observation — updated apps/AGENTS.md in 51ed3fa. The convention bullet now reads: "TypeScript imports use .js extensions. Default module resolution is NodeNext; apps that ship with a bundler that handles ESM/CJS interop (currently apps/macos/ via electron-vite) may use moduleResolution: "Bundler" with module: "ESNext" so the bundler's resolution rules match TypeScript's view of the import graph." That matches the @electron-toolkit/tsconfig.node baseline that every electron-vite starter inherits, and the .js-extension convention still applies regardless of resolution mode.
Generated by Claude Code
After a deeper read of the codebase's actual cross-platform precedent
(`native-biometric.ts` — named functions per setting, internal
isNativePlatform() gate, localStorage fallback on web), two grounded
adjustments:
- Remove `windowState` from `AppSettings` and the electron-store schema.
Window geometry is main-process-managed in Electron, system-managed on
iOS, browser-managed on web — the renderer never reads or writes it,
so the bridge shouldn't claim it does. If window-state restore is
wired in a future ticket, it lives in its own keyspace or via a
dedicated library (e.g. `electron-window-state`). Dead-code rule
applied prospectively.
- tsconfig `module: "Preserve"` → `"ESNext"` (moduleResolution stays at
`"Bundler"`). Matches the upstream `@electron-toolkit/tsconfig.node`
baseline every electron-vite starter inherits. `Preserve` worked but
isn't the canonical value for the stack.
Convention note for the next consumer ticket (theme persistence, hotkey
configuration, feature-flag UI): renderer-side wrappers should follow
the `apps/web/src/runtime/native-biometric.ts` precedent — a
per-capability module with named functions (e.g. `getTheme()` /
`setTheme(theme)`), internal platform branch (`isElectron()` → bridge,
`isNativePlatform()` → Capacitor when wired, else → localStorage), and
TypeScript types as the cross-platform contract. The generic
`window.vellum.settings.{get,set}` bridge is the underlying API those
wrappers call; consumers should not import it directly.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
…entions - apps/macos/README.md: replace the stale "Renderer bridge" section that said all methods were stubs. Document what's wired (platform, settings) vs what's stubbed (auth, helper), how to verify the bridge from the renderer, and add a "When to extend the bridge with new methods" section codifying the generic-KV-vs-dedicated-method rule for non-sensitive vs sensitive capabilities. Cites Electron's "one method per IPC message" security guidance. - apps/macos/src/preload/index.ts: refresh the VellumBridge interface comment to reflect the post-LUM-1846 state and point at the README section for anyone adding new methods. - apps/web/src/runtime/is-electron.ts: tell feature code in apps/web/ not to call window.vellum.* directly. The established pattern is a per-capability module under apps/web/src/runtime/ that owns the cross-platform branch internally — matches native-biometric.ts. No code changes; pure docs/comments. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
…s/AGENTS
Two review-feedback follow-ups, kept in the same PR per the
fix-as-you-go convention:
- apps/macos/src/main/settings.ts: pass `rootSchema:
{ additionalProperties: false }` to electron-store. Without it, conf's
validator only enforces the per-key shapes, leaving the root object
open — a renderer typo like `settings.set("them", "dark")` would
silently persist as an unknown top-level key. With the root closed,
unknown keys are rejected at validation time. Per-key shape validation
is unchanged.
- apps/AGENTS.md: the convention bullet previously said "NodeNext module
resolution" full stop. Update it to acknowledge that Electron apps may
use `moduleResolution: "Bundler"` with `module: "ESNext"` when their
bundler (electron-vite, in our case) handles ESM/CJS interop. The
`.js`-extension convention applies regardless. Matches the
`@electron-toolkit/tsconfig.node` baseline that electron-vite starters
inherit.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: Persistent settings store is the foundation layer the rest of the Electron feature work depends on — without it, hotkeys, theme, and feature flags reset on every restart. Landing this cleanly before wiring the real auth/helper methods keeps the bridge surface honest.
Bot reviews were stale at 58897689c8. Verified both findings are resolved at HEAD 51ed3fa6:
Codex P2 — unknown-key rejection: Fixed. settings.ts now passes rootSchema: { additionalProperties: false } alongside schema, so a renderer typo like set("them", "dark") throws a schema validation error instead of silently persisting. ✅
Devin — tsconfig module resolution: "module": "ESNext" + "moduleResolution": "Bundler" is correct for electron-vite (Rollup handles the ESM/CJS interop at bundle time). AGENTS.md now explicitly carves out this exception: apps/macos/ via electron-vite may use Bundler with ESNext; the NodeNext convention still holds everywhere else. ✅
Verified at HEAD:
settings.ts ✅ — Lazy singleton init avoids creating a store file on startup before any setting is written. readSetting returns null (not undefined) for absent keys so the IPC channel marshals cleanly across the bridge. Error path: writeSetting throws synchronously on schema violation; ipcMain.handle catches synchronous throws and rejects the corresponding ipcRenderer.invoke promise — renderer sees a rejected Promise<void> as documented. as never cast is a minor smell (bypasses TS value-type checking), but runtime schema validation is the real guard here.
preload/index.ts ✅ — contextBridge.exposeInMainWorld + contextIsolation: true + sandbox: true is the correct hardened Electron pattern. notImplemented stubs reject with a descriptive error rather than throwing synchronously, which is safer across the bridge boundary.
main/index.ts ✅ — installSettingsIpc() is called inside whenReady().then(), after which ipcMain.handle is safe to register. will-navigate guard restricts top-level navigation to the app origin. setWindowOpenHandler allows OAuth popups as child windows with hardened preferences — no preload key in overrideBrowserWindowOptions, so the OAuth popup doesn't inherit window.vellum exposure. ✅
is-electron.ts ✅ — Ambient declaration intentionally exposes only platform and settings (what's actually wired), not auth/helper stubs. Per-feature modules under runtime/ own the cross-platform branch, which is the right shape and consistent with native-biometric.ts.
electron.vite.config.ts ✅ — ESM_ONLY_DEPS_TO_INLINE: ["electron-store", "conf"] is correct. These are ESM-only packages; excluding them from externalizeDepsPlugin tells Rollup to bundle their ESM source inline into the CJS main output rather than emitting a require() call that would fail at runtime.
Non-blocking — worth tracking before auth.* lands:
- IPC sender frame validation.
ipcMain.handledoesn't validateevent.senderFrame.url.contextIsolation + sandboxprovides strong isolation today, but Electron's security guide recommends explicit sender validation (event.senderFrame.url === APP_ORIGIN) as defense-in-depth — especially onceauth.getToken()is wired and the IPC surface handles real credentials. Low risk now, worth adding then. - Schema unit tests. The
rootSchema: { additionalProperties: false }+ per-key schema combination is hard to reason about without tests. A small spec file asserting (a) valid keys pass, (b) unknown keys reject, (c) wrong types reject, and (d)geton a never-set key returnsnullwould catch regressions as the schema grows.
CI: 10/10 green. ✅
Vellum Constitution — Yours: preferences that survive restarts are a prerequisite for the app to feel like it belongs to the user.
2 participants
Summary
Wires
window.vellum.settings.{get,set}(LUM-1846) so the renderer can persist user preferences across restarts. Storage lives in the OS-canonicaluserDatapath viaelectron-storewith atomic file writes; the renderer accesses it through the generic contextBridge surface declared by the scaffold.What's in
Persisted settings
hotkeysRecord<string, string>{}theme"light" | "dark" | "system""system"featureFlagsRecord<string, boolean>{}Writes are validated against a JSON schema in the main process; a violation surfaces to the renderer as a rejected
Promisefromwindow.vellum.settings.set. Window geometry is intentionally not exposed via the bridge — it's main-process-managed on Electron, system-managed on iOS, browser-managed on web, with no renderer caller. When window-state restore is wired in a future ticket, it lives in its own main-process keyspace or via a dedicated library.Wiring
apps/macos/src/main/settings.tsowns the store instance and JSON schema. Lazy-initialized on first access.apps/macos/src/main/index.tsregistersvellum:settings:get/vellum:settings:setIPC handlers viainstallSettingsIpc()duringwhenReady.apps/macos/src/preload/index.ts—settings.{get,set}invoke those handlers viaipcRenderer.invoke.VellumBridgekeeps a genericget<T>(key)/set<T>(key, value)shape;authandhelperremain typed stubs that reject with "not implemented yet" until their feature tickets land.apps/web/src/runtime/is-electron.tsextends the ambientWindow.vellumdeclaration to expose the settings methods to renderer TypeScript.Conventions documented
apps/macos/README.md— "When to extend the bridge with new methods": genericsettings.{get,set}is appropriate for non-sensitive user preferences; sensitive capabilities (auth tokens, biometric keys, file paths) get dedicated<capability>.<verb>()bridge methods with their own IPC channels. Matches Electron's security tutorial: "one method per IPC message."apps/web/src/runtime/is-electron.ts— feature code inapps/web/should not callwindow.vellum.*directly; wrap each persisted capability in a per-feature module underapps/web/src/runtime/with named functions (getTheme()/setTheme(), etc.). The module owns the cross-platform branch internally —isElectron()calls the bridge,isNativePlatform()calls Capacitor when wired, web falls back tolocalStorage. This matches the establishedapps/web/src/runtime/native-biometric.tsshape.Build-time setup
electron-storeand itsconfparent are ESM-only. They're excluded fromexternalizeDepsPluginso Rollup bundles their ESM source into the CJS main output, where the default-export interop is handled at bundle time. Otherwiserequire("electron-store")returns the module namespace andnew Store(...)fails withStore is not a constructor. This is the documented pattern from electron-vite's Dependency Handling guide.module: "ESNext"+moduleResolution: "Bundler"matches the@electron-toolkit/tsconfig.nodebaseline that every electron-vite starter inherits. Required so TypeScript readsconf'sexportsfield and resolves the inheritedget/setsignatures through theConfbase class.Test plan
bun installclean from a freshnode_modules;bun.lockworkspace name matchespackage.json(@vellumai/macos).bun run typecheckclean.bun run buildclean.apps/webtypecheck clean against the extendedWindow.vellumdeclaration.xvfb-run electron --no-sandbox out/main/index.jsagainst a stub renderer on:5173):set/getroundtrip fortheme: "dark"✅set/getroundtrip forhotkeys: { newConvo: "Cmd+N", focusInput: "Cmd+L" }✅set("theme", "rainbow")rejected withConfig schema violation: 'theme' must be equal to one of the allowed values✅Pending on a real Mac:
cd apps/macos && bun run devrestarts. The config file lands in~/Library/Application Support/<appName>/config.json.Out of scope
apps/web/src/runtime/.@capacitor/preferencesimpl for the same capabilities — pairs with iOS Phase 1 work.electron-store's own README notes its migration system has known bugs; will add when we have a concrete v1 → v2 schema change to ship.https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Generated by Claude Code