ReviewOS
Reload

also looking at this

stacks/uplink

fix(app): a settings.json this build cannot read no longer loops the app

#46
Merged glennmichael123 wants to merge fix/settings-fallbacks into main
4 files +130 -22

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

app/Desktop/launcher.tsmodified+8-1
@@ -51,7 +51,14 @@ function craftRuntime(): string {
5151 return found
5252}
5353
54const agent = await startDesktopAgent({ version: pkg.version })
54// Anything thrown in here happens before the menubar exists, and launchd's
55// KeepAlive restarts a non-zero exit, so the loop would be silent: the plist
56// sets no StandardErrorPath, and the rebinding above only catches what goes
57// through `console`. Exit the same way, having said why first.
58const agent = await startDesktopAgent({ version: pkg.version }).catch((error: unknown) => {
59 console.error(`[uplink] could not start: ${error instanceof Error ? error.stack ?? error.message : String(error)}`)
60 return process.exit(1)
61})
5562console.log(`[uplink] ${pkg.version} serving the menubar on 127.0.0.1:${agent.port}`)
5663
5764// --tray-popover: Craft hangs the page under the status item on the popover
app/Uplink/desktop-agent.tsmodified+15-12
@@ -8,12 +8,11 @@ import { homedir } from 'node:os'
88import { join } from 'node:path'
99import process from 'node:process'
1010import { loadConfig } from './config'
11import { isEngineId } from './engine'
1211import { allEngines, selectedEngine } from './engines'
1312import { formatDuration, truncate } from './format'
1413import { MessagesAccessError, MessagesDb } from './messages-db'
1514import { AppleScriptSender } from './sender'
16import { cleanToken, DATA_DIR, DATABASE_PATH, readSettings, readToken, settingsEnv, tokenLooksValid, writeSettings, writeToken } from './settings'
15import { cleanToken, coerceSettings, DATA_DIR, DATABASE_PATH, readSettings, readToken, settingsEnv, tokenLooksValid, writeSettings, writeToken } from './settings'
1716import { checkLicenseKey, describeLicense, isLicensed, normalizeKey, portalUrl, PRICING_URL, readLicenseKey, readLicenseState, RECHECK_MS, writeLicenseKey, writeLicenseState } from './license'
1817import { SignIn } from './sign-in'
1918import { SqliteStore } from './sqlite-store'
@@ -614,16 +613,20 @@ export async function startDesktopAgent(options: { version: string }): Promise<D
614613 return json(engineResult())
615614
616615 case '/api/settings': {
617 if (Array.isArray(body.allowed))
618 settings.allowed = body.allowed.map(String).map((s: string) => s.trim()).filter(Boolean)
619 if (typeof body.engine === 'string' && isEngineId(body.engine))
620 settings.engine = body.engine
621 if (typeof body.paused === 'boolean')
622 settings.paused = body.paused
623 if (typeof body.openAtLogin === 'boolean') {
624 settings.openAtLogin = body.openAtLogin
625 setOpenAtLogin(body.openAtLogin)
626 }
616 // The four fields the popover can send, checked by the same tests the
617 // read path uses. Anything absent or unusable keeps what is set now,
618 // so this stays a partial update.
619 settings = coerceSettings({
620 allowed: body.allowed,
621 engine: body.engine,
622 paused: body.paused,
623 openAtLogin: body.openAtLogin,
624 }, settings)
625 // Rewritten whenever the popover sent the field, not only when the
626 // value changed: someone toggling it back and forth is how a login
627 // item left pointing at an old install gets repaired.
628 if (typeof body.openAtLogin === 'boolean')
629 setOpenAtLogin(settings.openAtLogin)
627630 writeSettings(settings)
628631 settings = readSettings()
629632 config = loadConfig(settingsEnv(settings))
app/Uplink/settings.tsmodified+45-8
@@ -47,20 +47,57 @@ export const DEFAULT_SETTINGS: Settings = {
4747 paused: false,
4848}
4949
50/**
51 * One test per field, `undefined` meaning "not a value this build can use".
52 *
53 * Both directions go through these: reading a file written by a hand, an older
54 * build or a half-finished save, and taking a field from the popover. Writing
55 * the checks twice is how the two drift, and it is the read path that cannot
56 * afford to be wrong: it runs before the menubar exists.
57 */
58const FIELDS: { [K in keyof Settings]: (value: unknown) => Settings[K] | undefined } = {
59 allowed: value => Array.isArray(value) ? value.map(String).map(handle => handle.trim()).filter(Boolean) : undefined,
60 engine: value => typeof value === 'string' && isEngineId(value) ? value : undefined,
61 model: value => value === null || (typeof value === 'string' && value !== '') ? (value as string | null) : undefined,
62 workdir: value => typeof value === 'string' && value !== '' ? value : undefined,
63 openAtLogin: value => typeof value === 'boolean' ? value : undefined,
64 paused: value => typeof value === 'boolean' ? value : undefined,
65}
66
67/**
68 * Every field of `input` this build can use, and `base` for every field it
69 * cannot. Never throws, whatever `input` is.
70 *
71 * `base` is what distinguishes the two callers: the defaults when reading a
72 * file, which must always produce usable settings, and the current settings
73 * when taking a partial update from the popover.
74 */
75export function coerceSettings(input: unknown, base: Settings = DEFAULT_SETTINGS): Settings {
76 const fields = (input && typeof input === 'object' ? input : {}) as Record<string, unknown>
77 const settings = { ...base }
78 for (const key of Object.keys(FIELDS) as Array<keyof Settings>) {
79 const value = FIELDS[key](fields[key])
80 if (value !== undefined)
81 Object.assign(settings, { [key]: value })
82 }
83 return settings
84}
85
86/**
87 * Always usable settings, because this is read at the desktop agent's first
88 * statements: a throw here is a launchd restart loop, and the menubar that
89 * would have explained it never starts. Only `engine` used to be checked, so
90 * an `allowed` that was not an array reached `settingsEnv` and took the app
91 * down on every launch, invisibly.
92 */
5093export function readSettings(path: string = SETTINGS_PATH): Settings {
5194 if (!existsSync(path))
5295 return { ...DEFAULT_SETTINGS }
5396 try {
54 const parsed = JSON.parse(readFileSync(path, 'utf8')) as Partial<Settings>
55 const settings = { ...DEFAULT_SETTINGS, ...parsed }
56 // A hand-edited or downgraded file can name an engine this build does not
57 // have. Falling back beats refusing to start.
58 if (!isEngineId(String(settings.engine)))
59 settings.engine = DEFAULT_SETTINGS.engine
60 return settings
97 return coerceSettings(JSON.parse(readFileSync(path, 'utf8')))
6198 }
6299 catch {
63 // A hand-edited file with a typo must not stop the app from starting.
100 // Not JSON at all.
64101 return { ...DEFAULT_SETTINGS }
65102 }
66103}
tests/unit/uplink/sqlite-store.test.tsmodified+62-1
@@ -6,7 +6,7 @@ import { tmpdir } from 'node:os'
66import { join } from 'node:path'
77import { loadConfig } from '../../../app/Uplink/config'
88import { MessagesDb } from '../../../app/Uplink/messages-db'
9import { cleanToken, readSettings, settingsEnv, tokenLooksValid, writeSettings } from '../../../app/Uplink/settings'
9import { cleanToken, coerceSettings, DEFAULT_SETTINGS, readSettings, settingsEnv, tokenLooksValid, writeSettings } from '../../../app/Uplink/settings'
1010import { migrate, RUN_COLUMNS, schemaVersion, SqliteStore, TABLES } from '../../../app/Uplink/sqlite-store'
1111import { Uplink } from '../../../app/Uplink/uplink'
1212import { FakeChatDb, FakeEngine, FakeSender, ME, settle } from './fixtures'
@@ -309,6 +309,67 @@ describe('settings', () => {
309309 expect(readSettings(path).engine).toBe('claude')
310310 })
311311
312 /**
313 * `readSettings` runs at the desktop agent's first statements, before the
314 * menubar exists, and launchd restarts a non-zero exit. So a file it cannot
315 * read is not an error it can report: it is an invisible restart loop. Only
316 * `engine` used to be checked, and an `allowed` that was not an array reached
317 * `settingsEnv` and threw on every launch.
318 */
319 it('falls back per field rather than throwing, whatever the file holds', () => {
320 const path = join(tempDir(), 'settings.json')
321 const broken: Array<[string, unknown]> = [
322 ['allowed', null],
323 ['allowed', 'me@example.com'],
324 ['engine', 'gemini'],
325 ['model', 42],
326 ['workdir', null],
327 ['workdir', ''],
328 ['openAtLogin', 'yes'],
329 ['paused', 1],
330 ]
331 for (const [key, value] of broken) {
332 Bun.write(path, JSON.stringify({ ...DEFAULT_SETTINGS, [key]: value }))
333 const settings = readSettings(path)
334 expect(settings[key as keyof typeof settings]).toEqual(DEFAULT_SETTINGS[key as keyof typeof DEFAULT_SETTINGS])
335 // The throw that used to take the app down, on the field that used to do it.
336 expect(() => settingsEnv(settings, {})).not.toThrow()
337 }
338 })
339
340 it('survives a file that is not an object at all', () => {
341 const path = join(tempDir(), 'settings.json')
342 for (const body of ['null', '[]', '"nope"', '5', '{ not json']) {
343 Bun.write(path, body)
344 expect(readSettings(path)).toEqual(DEFAULT_SETTINGS)
345 }
346 })
347
348 it('reads back exactly what it wrote', () => {
349 const path = join(tempDir(), 'settings.json')
350 const settings = { allowed: ['+15550001111'], engine: 'codex' as const, model: 'gpt-5-codex', workdir: '/tmp', openAtLogin: false, paused: true }
351 writeSettings(settings, path)
352 expect(readSettings(path)).toEqual(settings)
353 // And the default file too, whose `model` is null.
354 writeSettings(DEFAULT_SETTINGS, path)
355 expect(readSettings(path)).toEqual(DEFAULT_SETTINGS)
356 })
357
358 /**
359 * The popover sends the fields it changed. The same checks apply, but an
360 * absent or unusable one keeps what is set rather than reverting to a
361 * default, which is what makes it a partial update.
362 */
363 it('keeps the current value for a field the popover did not send', () => {
364 const current = { ...DEFAULT_SETTINGS, allowed: ['+15550001111'], paused: true }
365 expect(coerceSettings({ paused: false }, current)).toEqual({ ...current, paused: false })
366 expect(coerceSettings({ paused: 'no' }, current)).toEqual(current)
367 expect(coerceSettings({}, current)).toEqual(current)
368 expect(coerceSettings(undefined, current)).toEqual(current)
369 // Handles are normalized the way the popover's own path normalized them.
370 expect(coerceSettings({ allowed: [' me@example.com ', '', 5] }, current).allowed).toEqual(['me@example.com', '5'])
371 })
372
312373 it('cleans a token pasted across a line wrap', () => {
313374 const token = `sk-ant-oat01-${'a'.repeat(60)}`
314375 expect(cleanToken(`${token.slice(0, 40)}\n ${token.slice(40)}\n`)).toBe(token)