From f0f2864062af254bcbbeedf37c26c69659da6d3d Mon Sep 17 00:00:00 2001 From: Obada Qawwas Date: Mon, 21 Sep 2026 03:56:17 +0300 Subject: [PATCH] [FIX] Make Preferences win over .subzillarc in the Mac app - Rule: once the Preferences window has been saved, Preferences is the only source of settings. Until then a .subzillarc file seeds the initial values over the built-in defaults, and the window shows them, so saving locks them in. Restore Defaults hands control back. - The old "RC < stored" merge could not express any rule: electron-store materialises every default, so .subzillarc was ignored for almost every key (lineEndings, strip.html, ...) yet leaked through for the few keys without a default (output.directory, output.format, batch.maxDepth), even after the user had saved their preferences. - A userSavedConfig marker records the save. Installs that predate it count as saved when their stored values differ from the defaults, so an existing setup is never overridden. - getConfig() now awaits the RC load; previously the first conversion after launch could run before the file had been read. - Removed a second, hidden lookup through ConfigManager.loadConfig(): it searched process.cwd() on its own, returned schema-filled defaults, and never applied the environment variables its comment promised. - An RC file that is not a map or fails schema validation is ignored entirely instead of being half-applied. - The Preferences window and README now state the rule. Tests: 14 new cases against real .subzillarc files and a store that materialises defaults like electron-store does; 4 of them fail on the previous logic. Also verified in a real Electron process against the real electron-store (seed, save, relaunch, reset). Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 2 +- README.md | 2 + .../main/preferences-precedence.test.ts | 231 ++++++++++++++++++ packages/mac/src/main/preferences.ts | 153 ++++++++---- packages/mac/src/renderer/preferences.html | 3 + 5 files changed, 341 insertions(+), 50 deletions(-) create mode 100644 packages/mac/__tests__/main/preferences-precedence.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 3db2803..6045401 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,7 +48,7 @@ yarn workspace @subzilla/mac build # package the a ## Gotchas - Adding a strip option touches ~10 places: `IStripOptions`, Zod schema, `ConfigManager` (`KNOWN_PROPERTIES` + defaults), CLI `options.ts` + `strip-options.ts` + `IStripCommandOptions`, mac `preferences.ts` (schema, defaults, presets), `preferences.html`, `preferences.js` (element, listener list, load, save, presets ×2), READMEs. `git grep -n bidiControl` lists them all. -- In the mac app, electron-store defaults override `.subzillarc` values (open product question — do not "fix" silently). +- Mac app config rule: once Preferences has been saved (`userSavedConfig` marker, or stored values differing from defaults), Preferences is the ONLY source; before that `.subzillarc` seeds values over the built-in defaults. electron-store materialises every default, so a plain "stored overrides RC" merge can never express this. Logic + rationale: `getConfig()` in `main/preferences.ts`. - `packages/mac/electron-builder.yml` is the only builder config. Never re-add a `"build"` field to `packages/mac/package.json`: it silently shadows the yml. - The app is ad-hoc signed by `scripts/adhoc-sign.js`. An invalid signature makes macOS report "contains malware" and delete the bundle. Before launching any build: `codesign --verify --deep --strict `. - macOS has no `timeout` command. When checking that a launched process is alive, use its PID (`kill -0 $PID`); `pgrep -f ` matches your own shell command. diff --git a/README.md b/README.md index c9b5526..04a07e1 100644 --- a/README.md +++ b/README.md @@ -361,6 +361,8 @@ SubZilla looks for configuration files in the following order: 3. `.subzilla.yml` or `.subzilla.yaml` 4. `subzilla.config.yml` or `subzilla.config.yaml` +The **Mac app** treats these files differently from the CLI: once you save the Preferences window, Preferences is the only source of settings and `.subzillarc` files are ignored. Before the first save (and again after _Reset to Defaults_) a `.subzillarc` in your home directory seeds the initial values over the built-in defaults, and the Preferences window shows them. Environment variables are not read by the app. + ### Example Configurations Several example configurations are provided in the `examples/config` directory: diff --git a/packages/mac/__tests__/main/preferences-precedence.test.ts b/packages/mac/__tests__/main/preferences-precedence.test.ts new file mode 100644 index 0000000..9441234 --- /dev/null +++ b/packages/mac/__tests__/main/preferences-precedence.test.ts @@ -0,0 +1,231 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; + +import { describe, it, expect, beforeEach, afterEach, jest } from '@jest/globals'; + +import { IConfig } from '@subzilla/types'; + +/** + * Precedence between the Preferences window and .subzillarc files. + * + * The rule: once Preferences has been saved, Preferences is the ONLY source of + * truth. Until then, .subzillarc seeds the initial values over the built-in + * defaults. + * + * Runs against real .subzillarc files on disk and a fake store that behaves + * like electron-store where it matters: defaults are materialised into the + * store on construction (which is exactly what made the old "RC < stored" + * merge meaningless) and clear() brings them back. + */ + +type TRecord = Record; + +class FakeElectronStore { + public store: TRecord; + public path = '/fake/preferences.json'; + private readonly defaults: TRecord; + + constructor(options: { defaults: TRecord }) { + this.defaults = JSON.parse(JSON.stringify(options.defaults)); + this.store = { ...JSON.parse(JSON.stringify(this.defaults)), ...persisted }; + } + + public get(key: string, fallback?: unknown): unknown { + return key in this.store ? this.store[key] : fallback; + } + + public set(keyOrObject: string | TRecord, value?: unknown): void { + if (typeof keyOrObject === 'string') { + this.store[keyOrObject] = value; + } else { + this.store = { ...this.store, ...keyOrObject }; + } + + persisted = JSON.parse(JSON.stringify(this.store)); + } + + public clear(): void { + this.store = JSON.parse(JSON.stringify(this.defaults)); + persisted = {}; + } +} + +// What is "on disk" for the store; survives constructing a new ConfigMapper (= relaunching the app) +let persisted: TRecord = {}; + +jest.mock('electron-store', () => jest.fn().mockImplementation((options) => new FakeElectronStore(options as never))); + +interface IConfigMapper { + getConfig: () => Promise; + saveConfig: (config: IConfig) => Promise; + resetConfig: () => Promise; +} + +describe('Mac app: Preferences vs .subzillarc', () => { + let rcDir: string; + let launchApp: () => IConfigMapper; + + const writeRc = (content: string, name = '.subzillarc'): void => fs.writeFileSync(path.join(rcDir, name), content); + + beforeEach(async () => { + persisted = {}; + rcDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'subzilla-rc-')); + + const { ConfigMapper } = await import('../../src/main/preferences'); + + // Point the RC search at our temp dir only, so neither the developer's + // home directory nor this repository's own .subzillarc leaks into the test + class TestConfigMapper extends ConfigMapper { + protected async getRcSearchDirs(): Promise { + return [rcDir]; + } + } + + launchApp = (): IConfigMapper => new TestConfigMapper() as unknown as IConfigMapper; + }); + + afterEach(async () => { + await fs.promises.rm(rcDir, { recursive: true, force: true }); + }); + + describe('before Preferences has ever been saved', () => { + it('.subzillarc overrides the built-in defaults, including keys that HAVE a default', async () => { + writeRc('output:\n lineEndings: crlf\n bom: false\nstrip:\n html: true\n markdown: true\n'); + + const config = await launchApp().getConfig(); + + // These are the values the old code silently ignored + expect(config.output?.lineEndings).toBe('crlf'); + expect(config.output?.bom).toBe(false); + expect(config.strip?.html).toBe(true); + expect(config.strip?.markdown).toBe(true); + }); + + it('keys the file does not mention keep their built-in defaults', async () => { + writeRc('strip:\n html: true\n'); + + const config = await launchApp().getConfig(); + + expect(config.strip?.bidiControl).toBe(true); + expect(config.strip?.colors).toBe(false); + expect(config.output?.bom).toBe(true); + expect(config.output?.overwriteExisting).toBe(true); + expect(config.batch?.chunkSize).toBe(5); + }); + + it('is already applied on the very first getConfig() call (no race with the async file load)', async () => { + writeRc('output:\n lineEndings: crlf\n'); + + const app = launchApp(); + + expect((await app.getConfig()).output?.lineEndings).toBe('crlf'); + }); + + it('with no .subzillarc at all, the built-in defaults apply', async () => { + const config = await launchApp().getConfig(); + + expect(config.output?.lineEndings).toBe('auto'); + expect(config.strip?.html).toBe(false); + }); + + it('never exposes app-only or bookkeeping keys as conversion options', async () => { + writeRc('strip:\n html: true\n'); + + expect(Object.keys(await launchApp().getConfig()).sort()).toEqual(['batch', 'input', 'output', 'strip']); + }); + }); + + describe('after Preferences has been saved', () => { + it('Preferences wins on every key, including one explicitly set back to its default value', async () => { + writeRc('output:\n lineEndings: crlf\nstrip:\n html: true\n urls: true\n'); + + const app = launchApp(); + const shown = await app.getConfig(); + + // The user unticks HTML (its built-in default!) and picks LF, then saves + await app.saveConfig({ + ...shown, + output: { ...shown.output, lineEndings: 'lf' }, + strip: { ...shown.strip, html: false }, + }); + + const config = await app.getConfig(); + + expect(config.strip?.html).toBe(false); // not resurrected by the file + expect(config.output?.lineEndings).toBe('lf'); + expect(config.strip?.urls).toBe(true); // what the window showed was saved + }); + + it('.subzillarc no longer leaks keys that have no built-in default', async () => { + const app = launchApp(); + + await app.saveConfig(await app.getConfig()); + + // The file appears (or changes) AFTER the user saved their preferences + writeRc('output:\n directory: /tmp/somewhere-else\n format: ass\nbatch:\n maxDepth: 2\n'); + + const config = await launchApp().getConfig(); + + expect(config.output?.directory).toBeUndefined(); + expect(config.output?.format).toBeUndefined(); + expect(config.batch?.maxDepth).toBeUndefined(); + }); + + it('still wins after the app is relaunched', async () => { + writeRc('strip:\n html: true\n'); + + const first = launchApp(); + const shown = await first.getConfig(); + + await first.saveConfig({ ...shown, strip: { ...shown.strip, html: false } }); + + expect((await launchApp().getConfig()).strip?.html).toBe(false); + }); + + it('Restore Defaults hands control back: built-in defaults, seeded by .subzillarc again', async () => { + writeRc('strip:\n html: true\n'); + + const app = launchApp(); + const shown = await app.getConfig(); + + await app.saveConfig({ ...shown, strip: { ...shown.strip, html: false, emojis: true } }); + await app.resetConfig(); + + const config = await app.getConfig(); + + expect(config.strip?.html).toBe(true); // from the file again + expect(config.strip?.emojis).toBe(false); // built-in default again + }); + }); + + describe('installs that predate this rule', () => { + it('stored values that differ from the defaults count as saved Preferences and win', async () => { + // An existing preferences.json with a customised value, but no "saved" marker + persisted = { strip: { html: false, colors: true, bidiControl: true } }; + writeRc('strip:\n html: true\n colors: false\n'); + + const config = await launchApp().getConfig(); + + expect(config.strip?.colors).toBe(true); + expect(config.strip?.html).toBe(false); + }); + }); + + describe('a broken .subzillarc cannot break conversion', () => { + it.each([ + ['not YAML at all', '{{{{ : ::: \n\t- ]['], + ['wrong types', 'strip: yes-please\noutput:\n lineEndings: sideways\n'], + ['a list instead of a map', '- a\n- b\n'], + ['empty file', ''], + ])('%s is ignored and the built-in defaults apply', async (_name, content) => { + writeRc(content); + + const config = await launchApp().getConfig(); + + expect(config.output?.lineEndings).toBe('auto'); + expect(config.strip).toEqual(expect.objectContaining({ html: false, bidiControl: true })); + expect(typeof config.strip).toBe('object'); + }); + }); +}); diff --git a/packages/mac/src/main/preferences.ts b/packages/mac/src/main/preferences.ts index 28e1a2d..dd23d56 100644 --- a/packages/mac/src/main/preferences.ts +++ b/packages/mac/src/main/preferences.ts @@ -3,8 +3,7 @@ import path from 'path'; import Store from 'electron-store'; -import { ConfigManager } from '@subzilla/core'; -import { IConfig, IStripOptions } from '@subzilla/types'; +import { IConfig, IStripOptions, configSchema } from '@subzilla/types'; export interface IMacAppPreferences { // Application-specific preferences @@ -24,14 +23,21 @@ export interface IMacAppPreferences { }; } +// userSavedConfig: set once the Preferences window has been saved. From then on +// Preferences is the only source of truth and .subzillarc files are ignored. +type TStoredConfig = IConfig & { app: IMacAppPreferences; userSavedConfig?: boolean }; + export class ConfigMapper { - private store: Store; + private store: Store; private rcConfig: IConfig | null = null; + // getConfig() awaits this, so the first conversion after launch can never run + // before the .subzillarc file has been read + private rcLoaded: Promise; constructor() { console.log('⚙️ Initializing configuration store...'); - this.store = new Store({ + this.store = new Store({ name: 'preferences', defaults: this.getDefaultConfig(), schema: { @@ -86,6 +92,7 @@ export class ConfigMapper { failFast: { type: 'boolean' }, }, }, + userSavedConfig: { type: 'boolean' }, app: { type: 'object', properties: { @@ -102,33 +109,19 @@ export class ConfigMapper { console.log('✅ Configuration store initialized'); - // Load RC config asynchronously (will be merged when getConfig() is called) - // We don't await this to avoid blocking the constructor - this.loadRcConfig().catch((err) => { + // Load RC config asynchronously; a constructor cannot await, getConfig() does + this.rcLoaded = this.loadRcConfig().catch((err) => { console.warn('⚠️ Failed to load RC config:', err); }); } /** - * Load configuration from .subzillarc files - * Search order: cwd < app resources directory < home directory - * This follows the same precedence as the CLI: defaults < file config < env vars < app preferences + * Directories searched for RC files, in order of precedence (later overrides earlier) */ - private async loadRcConfig(): Promise { - console.log('🔍 Loading RC configuration...'); - + protected async getRcSearchDirs(): Promise { const fs = await import('fs/promises'); - const yaml = await import('yaml'); const { app } = await import('electron'); - const rcFiles = [ - '.subzillarc', - '.subzilla.yml', - '.subzilla.yaml', - 'subzilla.config.yml', - 'subzilla.config.yaml', - ]; - // Directories to search for RC files (in order of precedence - later overrides earlier) const searchDirs = [ os.homedir(), // Global user config @@ -158,6 +151,30 @@ export class ConfigMapper { } } + return searchDirs; + } + + /** + * Load configuration from .subzillarc files + * Later search directories override earlier ones (see getRcSearchDirs). + * How the result combines with stored Preferences is decided in getConfig(). + */ + private async loadRcConfig(): Promise { + console.log('🔍 Loading RC configuration...'); + + const fs = await import('fs/promises'); + const yaml = await import('yaml'); + + const rcFiles = [ + '.subzillarc', + '.subzilla.yml', + '.subzilla.yaml', + 'subzilla.config.yml', + 'subzilla.config.yaml', + ]; + + const searchDirs = await this.getRcSearchDirs(); + let foundConfig: IConfig | null = null; let foundPath: string | null = null; @@ -172,6 +189,18 @@ export class ConfigMapper { const content = await fs.readFile(rcPath, 'utf8'); const config = yaml.parse(content); + // Validate, but keep the RAW object: the schema fills in its own + // defaults, which would override the app's defaults for keys the + // file never mentioned. A file that fails validation is ignored + // entirely rather than half-applied. + if (!config || typeof config !== 'object' || Array.isArray(config)) continue; + + if (!configSchema.safeParse(config).success) { + console.warn(`⚠️ Ignoring invalid RC config: ${rcPath}`); + + continue; + } + foundConfig = config; foundPath = rcPath; console.log(`✅ Loaded RC config from ${rcPath}`); @@ -182,23 +211,6 @@ export class ConfigMapper { } } - // Also try ConfigManager for env vars support - try { - const coreConfigResult = await ConfigManager.loadConfig(); - - if (coreConfigResult.source === 'file' && coreConfigResult.filePath) { - console.log(`✅ ConfigManager found config at: ${coreConfigResult.filePath}`); - - // If ConfigManager found a file we didn't find, use it - if (!foundConfig) { - foundConfig = coreConfigResult.config; - foundPath = coreConfigResult.filePath; - } - } - } catch { - // Continue without ConfigManager config - } - if (foundConfig) { this.rcConfig = foundConfig; console.log(`✅ Using RC config from: ${foundPath}`); @@ -307,20 +319,62 @@ export class ConfigMapper { }; } + /** + * The effective conversion settings. + * + * Rule: once the Preferences window has been saved, Preferences is the only + * source of truth. Until then, a .subzillarc file seeds the initial values over + * the built-in defaults (and is what the Preferences window shows, so saving + * locks those values in). + * + * "stored < RC" could never express this: electron-store materialises every + * default into the store, so stored values always "won" - except for the few + * keys without a default (output.directory, batch.maxDepth, ...), which leaked + * through from the file even after the user had saved their preferences. + */ public async getConfig(): Promise { - const fullConfig = this.store.store; + await this.rcLoaded; - // Return only the IConfig part (without app preferences) + // Only the IConfig part: no app preferences, no bookkeeping // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { app, ...storedConfig } = fullConfig; + const { app, userSavedConfig, ...storedConfig } = this.store.store; - // Merge RC config (base) with stored preferences (override) - // Precedence: defaults < RC file config < stored app preferences - if (this.rcConfig) { - return this.mergeConfigs(this.rcConfig, storedConfig); + if (this.hasUserSavedConfig() || !this.rcConfig) { + return storedConfig; } - return storedConfig; + return this.mergeConfigs(storedConfig, this.rcConfig); + } + + /** + * True once the user has saved Preferences. Installs that predate the marker + * count as saved when their stored values differ from the defaults, so an + * existing customised setup is never overridden by a .subzillarc file. + */ + private hasUserSavedConfig(): boolean { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { app, userSavedConfig, ...storedConfig } = this.store.store; + + if (userSavedConfig === true) return true; + + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { app: defaultApp, ...defaultConfig } = this.getDefaultConfig(); + + return !this.isSameConfig(storedConfig, defaultConfig); + } + + private isSameConfig(a: unknown, b: unknown): boolean { + const normalise = (value: unknown): unknown => + value && typeof value === 'object' && !Array.isArray(value) + ? Object.fromEntries( + Object.entries(value as Record) + .filter(([, entry]) => entry !== undefined) + .sort(([x], [y]) => x.localeCompare(y)) + .map(([key, entry]) => [key, normalise(entry)]), + ) + : value; + + return JSON.stringify(normalise(a)) === JSON.stringify(normalise(b)); } /** @@ -358,7 +412,8 @@ export class ConfigMapper { // Preserve app preferences while updating core config const currentApp = await this.getAppPreferences(); - this.store.set({ ...config, app: currentApp }); + // From here on Preferences wins over any .subzillarc file (see getConfig) + this.store.set({ ...config, app: currentApp, userSavedConfig: true }); console.log('✅ Configuration saved'); } @@ -379,7 +434,7 @@ export class ConfigMapper { return this.store.path; } - public getStore(): Store { + public getStore(): Store { return this.store; } diff --git a/packages/mac/src/renderer/preferences.html b/packages/mac/src/renderer/preferences.html index 60c2b34..7843f63 100644 --- a/packages/mac/src/renderer/preferences.html +++ b/packages/mac/src/renderer/preferences.html @@ -273,6 +273,9 @@

Application Data

Loading...
+
+ Once saved, these preferences are the only settings the app uses. A .subzillarc file only provides the initial values shown here before your first save, and again after Reset to Defaults. +