Conversation
SettingsPolicy.defaults existed in the schema but nothing wrote or read it. Wire it up end to end: - lib/admin/policy-settings.ts: one shared catalogue of governable settings (type + allowed values) used by the admin UI, the policy sanitizer and the client. Adds signaturePosition and signatureSeparatorEnabled to the list. - Admin Policy tab gets a Default control per setting (Built-in / value); policy.json defaults are sanitized on load and save like the sidebar apps, and the PUT route rejects a non-object defaults section. - Settings store tracks which keys the user set themselves (explicitSettings, exported and synced with the rest). Policy defaults fill in only the others, are applied on policy load, re-applied over an imported/synced blob that predates them, and are what Reset to defaults lands on. - Composing tab honours lock/hide for the two new keys. Lets an operator make e.g. signature position 'above quoted text' the default for everyone without changing the build default.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
It introduces a runtime safety issue in updateSetting (can overwrite store actions) and currently allows unconstrained numeric policy defaults that can permit invalid/pathological values.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR wires up policy.defaults end-to-end so operators can define per-setting default values that apply only to users who have never explicitly changed those settings, without changing build defaults or overriding user choices.
Changes:
- Adds a shared governable-settings catalog plus server-side sanitization for
policy.defaults. - Extends the Admin → Policy tab UI to configure per-setting defaults (Built-in vs explicit value).
- Introduces
explicitSettingstracking and applies policy defaults in the settings store (on policy load, after imports, and on reset).
| File | Description |
|---|---|
stores/settings-store.ts |
Tracks user-explicit settings and applies operator defaults without overriding explicit user choices. |
stores/__tests__/settings-store-policy-defaults.test.ts |
Adds tests for default-application semantics and explicit-choice preservation. |
lib/admin/policy-settings.ts |
New central catalog + validator/sanitizer for governable settings defaults. |
lib/admin/config-manager.ts |
Sanitizes policy.defaults on policy load/save. |
lib/admin/__tests__/policy-settings.test.ts |
Adds tests for defaults sanitization behavior. |
FEATURES.md |
Documents the new admin policy defaults capability. |
components/settings/composing-settings.tsx |
Applies lock/hide policy to newly governable composing settings. |
app/api/admin/policy/route.ts |
Validates that defaults is an object on PUT. |
app/(main)/admin/_tabs/policy.tsx |
Admin UI: adds “Default” control per setting and uses shared catalog/validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { key: 'markAsReadDelay', label: 'Mark as Read Delay', category: 'Email', type: 'number' }, | ||
| { key: 'deleteAction', label: 'Delete Action', category: 'Email', type: 'enum', allowedValues: ['trash', 'trash-and-read', 'permanent'] }, | ||
| { key: 'showPreview', label: 'Show Preview', category: 'Email', type: 'boolean' }, | ||
| { key: 'mailLayout', label: 'Mail Layout', category: 'Email', type: 'enum', allowedValues: ['split', 'focus', 'horizontal'] }, | ||
| { key: 'emailsPerPage', label: 'Emails Per Page', category: 'Email', type: 'number' }, | ||
| { key: 'externalContentPolicy', label: 'External Content Policy', category: 'Email', type: 'enum', allowedValues: ['allow', 'block', 'ask'] }, |
| updateSetting: (key, value) => { | ||
| set({ [key]: value }); | ||
| const { explicitSettings } = get(); | ||
| const trackAsExplicit = key !== 'explicitSettings' && !explicitSettings.includes(key); | ||
| set({ | ||
| [key]: value, | ||
| ...(trackAsExplicit ? { explicitSettings: [...explicitSettings, key] } : {}), | ||
| }); |
Review follow-ups: - emailsPerPage and markAsReadDelay are numeric enums now (the settings UI only ever produces 10/25/50/100 and 0/3000/5000/-1), so policy.json and the admin UI can no longer hand users a 0 or negative page size. Free numbers (sessionTimeout) must be integers within an optional min/max; the admin input carries the same bounds. - updateSetting refuses keys that are not in DEFAULT_SETTINGS: the type admits every key of the state, including the store actions, and a bad call would otherwise overwrite one and record it as explicit.
|
Thanks for this PR — it's exactly what we need. We run Bulwark for small businesses in Brazil and would love two more settings covered by operator defaults:
Related, maybe a separate issue: the policy's default/forced theme is only fetched after the first login, so on a first visit /login renders the stock theme. Loading the default theme on the login page would make branding consistent from the first visit. Happy to test a build. |
What
Lets an operator choose the value users start with for governable settings, e.g. make signature position → above quoted text the default for everyone, without changing the build default or forcing it on people who chose otherwise.
SettingsPolicy.defaultshas been in the policy schema (andusePolicyStore.getEffectiveDefault) for a while, but nothing wrote to it and nothing read it. This wires it up end to end.How
lib/admin/policy-settings.ts– one shared catalogue of governable settings (key, type, allowed values). Replaces theRESTRICTABLE_SETTINGSlist that lived only in the admin tab, and is now used by the admin UI, the server-side sanitizer and the client. AddssignaturePositionandsignatureSeparatorEnabled.policy.defaultsis sanitized on load and save (same place asdefaultSidebarApps), so a hand-editedpolicy.jsoncan't inject unknown keys or wrong-shaped values;PUT /api/admin/policyrejects a non-objectdefaults.explicitSettings: string[](persisted, exported and synced with the rest) records which keys the user set themselves viaupdateSetting.applyPolicyDefaults()fills in only the other keys. It runs when the policy loads, is re-applied at the end ofimportSettings()(so a synced blob written before the operator set the default still picks it up), and is what "Reset to defaults" lands on.Semantics: an operator default applies to anyone who never touched that setting – including existing users – and a user's own choice is never overridden, even if the operator changes the default later.
Tests
lib/admin/__tests__/policy-settings.test.ts– sanitizer keeps valid defaults, drops unknown keys / wrong shapes / malformed sections.stores/__tests__/settings-store-policy-defaults.test.ts– applied on policy load; explicit choice preserved; non-setting keys ignored; re-applied over an older synced blob; explicit choice inside an imported blob honoured; reset lands on the operator default;explicitSettingsexported.npm run typecheck,npm run lint,npx vitest runall green (the one failure locally is the pre-existing timezone-dependentbirthday-calendartest, unrelated).Notes for review
sendConfirmation,plainTextMode, …) before this PR either; I only wired the two keys I added to keep the diff focused. Happy to do the rest in a follow-up.