Skip to content

Commit 76e6979

Browse files
committed
refactor(desktop,cli): review pass on self-hosted desktop support
Two real defects found while auditing the change for hardcoded assumptions. `lastRoute` is a single global setting that carries a workspace id, so it survived an origin change and opened /workspace/<old-id> on the new server. resolveStartRoute cannot rescue that — it discards a route only on a confirmed 403, and a fresh partition draws a 401. Cleared on change, via a named list that is now the documented home for deployment-scoped settings; the agent browser's jar and its known-sites metadata are deliberately kept, together, since changing deployments does not imply the account changed. The offline page's "Check status" sent self-hosters to status.sim.ai, which reports on Sim's deployments and is always green for theirs. Withheld for a non-sim.ai origin, as is the same link in the Help menu, through one isSimCloudOrigin predicate. Hiding it needed `button[hidden]{display:none}`: the page's own `button{display:inline-flex}` is an author rule and outranks the UA `[hidden]`, so the attribute alone left it rendering. The e2e offline test now asserts the whole path, which covers the `server:` local-page IPC gate. Review cleanups: setOrigin no longer rewrites settings when handed the origin it already stores; the picker window installs a permission handler and pre- paints its background like every other window, and its page is theme-aware so that background is not a flash; the CLI reuses httpHealth and the cross-platform openBrowser instead of reimplementing both, skips Compose/Helm discovery when --url makes it dead, and folds two parallel switches into one exhaustive one. Value-flag parsing is now one helper instead of a third copy.
1 parent 0434578 commit 76e6979

18 files changed

Lines changed: 365 additions & 148 deletions

File tree

apps/desktop/e2e/smoke.spec.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,15 @@ test.describe('desktop shell smoke', () => {
113113
await expect(window.locator('.wordmark')).toBeVisible()
114114
await expect(window.locator('.wordmark')).toHaveAttribute('aria-label', 'Sim')
115115
await expect(window.locator('#title')).toHaveText('Can’t connect to Sim')
116-
await expect(window.locator('#status')).toHaveText('Check status')
116+
// The recovery path for a self-hosted shell pointed at a server it cannot
117+
// reach. Exercised end to end here because it is the only coverage of the
118+
// `server:` local-page IPC gate: the bundled page reads the configuration
119+
// over the real preload bridge, and status.sim.ai is withheld because this
120+
// origin is not one of Sim's own. `toBeHidden` is load-bearing — the page's
121+
// own `button { display: inline-flex }` outranks the UA `[hidden]` rule, so
122+
// the attribute alone does not hide it.
123+
await expect(window.locator('#server')).toBeVisible()
124+
await expect(window.locator('#status')).toBeHidden()
117125
await expect
118126
.poll(() => window.evaluate(() => document.fonts.check('16px "Season Sans"')))
119127
.toBe(true)

apps/desktop/src/main/config.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
createConfigStore,
1010
DEFAULT_ORIGIN,
1111
isSafeInternalPath,
12+
isSimCloudOrigin,
1213
partitionForOrigin,
1314
validateOriginInput,
1415
} from '@/main/config'
@@ -145,6 +146,26 @@ describe('createConfigStore', () => {
145146
expect(reloaded.getOrigin()).toBe('https://self-hosted.example')
146147
})
147148

149+
// setOrigin writes the whole settings file synchronously on the main thread,
150+
// and re-confirming the URL already in the field is the common case in the
151+
// server picker.
152+
it('does not rewrite settings when setOrigin is given the stored origin', () => {
153+
const filePath = tempSettingsPath()
154+
const store = createConfigStore(filePath, {})
155+
store.setOrigin('https://self-hosted.example')
156+
// A sentinel only this test could have written. A rewrite serializes the
157+
// in-memory settings over it, so its survival proves no write happened —
158+
// unlike an mtime comparison, which two writes a fraction of a millisecond
159+
// apart can pass by accident.
160+
writeFileSync(filePath, `${readFileSync(filePath, 'utf8')}\n// sentinel\n`)
161+
162+
expect(store.setOrigin('https://self-hosted.example')).toEqual({
163+
ok: true,
164+
origin: 'https://self-hosted.example',
165+
})
166+
expect(readFileSync(filePath, 'utf8')).toContain('// sentinel')
167+
})
168+
148169
it('canonicalizes the apex production origin on setOrigin, not just on load', () => {
149170
// Entering https://sim.ai mid-session must not persist the apex: the
150171
// running session would use the wrong cookie partition and misclassify
@@ -223,6 +244,25 @@ describe('createConfigStore', () => {
223244
})
224245
})
225246

247+
describe('isSimCloudOrigin', () => {
248+
it('recognizes Sim-operated origins and nothing else', () => {
249+
for (const origin of ['https://sim.ai', 'https://www.sim.ai', 'https://www.staging.sim.ai']) {
250+
expect(isSimCloudOrigin(origin)).toBe(true)
251+
}
252+
// A lookalike host must not pass — the suffix check is on the parsed
253+
// hostname, never a prefix or substring of the raw string.
254+
for (const origin of [
255+
'https://sim.example.com',
256+
'https://sim.ai.evil.example',
257+
'https://notsim.ai',
258+
'http://localhost:3000',
259+
'not a url',
260+
]) {
261+
expect(isSimCloudOrigin(origin)).toBe(false)
262+
}
263+
})
264+
})
265+
226266
describe('channelForOrigin', () => {
227267
it('maps each environment origin to its channel', () => {
228268
expect(channelForOrigin('https://sim.ai')).toBe('prod')

apps/desktop/src/main/config.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,22 @@ export function canonicalOrigin(origin: string): string {
179179
return ORIGIN_REWRITES[origin] ?? origin
180180
}
181181

182+
/**
183+
* Whether an origin is one of Sim's own deployments rather than a self-hosted
184+
* one. Sim-operated resources — the public status page above all — describe
185+
* only these, so a shell pointed elsewhere must not be offered them: telling a
186+
* self-hoster whose server is down to consult a page that is always green
187+
* sends the person who most needs an answer to the one place that has none.
188+
*/
189+
export function isSimCloudOrigin(origin: string): boolean {
190+
try {
191+
const host = new URL(origin).hostname.toLowerCase()
192+
return host === 'sim.ai' || host.endsWith('.sim.ai')
193+
} catch {
194+
return false
195+
}
196+
}
197+
182198
/**
183199
* Maps a server origin to its cookie/storage partition. Each origin gets an
184200
* isolated persistent partition so sessions never leak across instances.
@@ -339,6 +355,12 @@ export function createConfigStore(
339355
// only repairs it on the next launch. The canonical origin is also
340356
// returned so the caller sees what was actually stored.
341357
const origin = canonicalOrigin(validated.origin)
358+
// Re-confirming the origin already stored is the common case in the
359+
// server picker, and setOrigin's write is a synchronous mkdir + whole-file
360+
// write + rename on the main thread. There is nothing to persist.
361+
if (origin === settings.origin) {
362+
return { ok: true, origin }
363+
}
342364
settings.origin = origin
343365
// Not debounced: changing the origin tears the session down and
344366
// reloads, so a pending write could be lost on the way out — and this

apps/desktop/src/main/index.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ function reportHandoffFailure(error: unknown): void {
7979
}
8080

8181
const OFFLINE_PAGE = 'static/offline.html'
82+
const SERVER_PAGE = 'static/server.html'
8283
const DOCK_ICON_FOR_CHANNEL = {
8384
prod: 'dock-icon.png',
8485
staging: 'dock-icon-staging.png',
@@ -474,14 +475,10 @@ function main(): void {
474475
},
475476
})
476477

477-
/**
478-
* The native server picker. Self-hosted operators install the same signed
479-
* build as everyone else and repoint it here — the bundle bakes only a
480-
* DEFAULT origin, and every runtime guard reads the configured one.
481-
*/
482478
const serverWindow = createServerWindow({
483479
config,
484480
defaultOrigin: DEFAULT_ORIGIN,
481+
pagePath: SERVER_PAGE,
485482
preloadPath,
486483
isPackaged: app.isPackaged,
487484
getParentWindow: getMainWindow,

apps/desktop/src/main/ipc.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ describe('registerIpcHandlers', () => {
318318
},
319319
server: {
320320
open: vi.fn(),
321-
getConfiguration: vi.fn(() => ({ origin: APP, defaultOrigin: APP })),
321+
getConfiguration: vi.fn(() => ({ origin: APP, defaultOrigin: APP, isSimCloud: true })),
322322
setOrigin: vi.fn(() => ({ ok: true as const, origin: APP, unchanged: true })),
323323
},
324324
}

apps/desktop/src/main/menu.test.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ import { BrowserWindow, type MenuItemConstructorOptions } from 'electron'
66
import type { ConfigStore } from '@/main/config'
77
import { buildMenuTemplate, type MenuDeps } from '@/main/menu'
88

9-
function makeDeps(): MenuDeps {
9+
function makeDeps(origin = 'https://sim.ai'): MenuDeps {
1010
return {
1111
config: {
1212
filePath: '/tmp/settings.json',
13-
getOrigin: vi.fn(() => 'https://sim.ai'),
13+
getOrigin: vi.fn(() => origin),
1414
setOrigin: vi.fn(),
1515
get: vi.fn(() => undefined),
1616
set: vi.fn(),
@@ -107,6 +107,13 @@ describe('buildMenuTemplate', () => {
107107
expect(help.map((item) => item.label)).toEqual(['Sim Documentation', 'Sim Status'])
108108
})
109109

110+
// status.sim.ai reports on Sim's deployments only, so it is worse than
111+
// useless to an operator whose own server is the one that is down.
112+
it('drops Sim status for a self-hosted server', () => {
113+
const help = submenu(buildMenuTemplate(makeDeps('https://sim.example.com')), 'Help')
114+
expect(help.map((item) => item.label)).toEqual(['Sim Documentation'])
115+
})
116+
110117
it('never exposes developer tools in the application menu', () => {
111118
const view = submenu(buildMenuTemplate(makeDeps()), 'View')
112119
expect(view.some((item) => item.role === 'toggleDevTools')).toBe(false)

apps/desktop/src/main/menu.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { MenuItemConstructorOptions } from 'electron'
22
import { app, BrowserWindow, Menu } from 'electron'
3-
import type { ConfigStore } from '@/main/config'
3+
import { type ConfigStore, isSimCloudOrigin } from '@/main/config'
44
import { DOCS_URL, STATUS_URL } from '@/main/external-links'
55
import { openExternalSafe } from '@/main/navigation'
66
import type {
@@ -253,10 +253,16 @@ export function buildMenuTemplate(deps: MenuDeps): MenuItemConstructorOptions[]
253253
label: 'Sim Documentation',
254254
click: () => void openExternalSafe(DOCS_URL, deps.allowHttpLocalhost()),
255255
},
256-
{
257-
label: 'Sim Status',
258-
click: () => void openExternalSafe(STATUS_URL, deps.allowHttpLocalhost()),
259-
},
256+
// Omitted for a self-hosted shell, like the offline page's status
257+
// button — see isSimCloudOrigin.
258+
...(isSimCloudOrigin(deps.config.getOrigin())
259+
? [
260+
{
261+
label: 'Sim Status',
262+
click: () => void openExternalSafe(STATUS_URL, deps.allowHttpLocalhost()),
263+
},
264+
]
265+
: []),
260266
],
261267
},
262268
]

apps/desktop/src/main/server-window.test.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ function makeDeps(overrides: Partial<ServerWindowDeps> = {}): ServerWindowDeps {
3030
raw.startsWith('https://') ? { ok: true, origin: raw } : { ok: false, error: 'bad origin' }
3131
),
3232
defaultOrigin: DEFAULT,
33+
pagePath: 'static/server.html',
3334
preloadPath: '/tmp/preload.cjs',
3435
isPackaged: false,
3536
getParentWindow: () => null,
@@ -49,9 +50,20 @@ describe('server window', () => {
4950
expect(createServerWindow(deps).getConfiguration()).toEqual({
5051
origin: CURRENT,
5152
defaultOrigin: DEFAULT,
53+
isSimCloud: false,
5254
})
5355
})
5456

57+
// Drives whether the offline page offers Sim's status page, which describes
58+
// only Sim's own deployments.
59+
it('marks a sim.ai origin as Sim cloud', () => {
60+
const cloud = makeDeps({
61+
config: makeConfig('https://www.sim.ai', (raw) => ({ ok: true, origin: raw })),
62+
})
63+
64+
expect(createServerWindow(cloud).getConfiguration().isSimCloud).toBe(true)
65+
})
66+
5567
it('relaunches after storing a different origin', () => {
5668
const result = createServerWindow(deps).setOrigin('https://sim.other.example')
5769

@@ -60,8 +72,22 @@ describe('server window', () => {
6072
expect(deps.relaunch).toHaveBeenCalledTimes(1)
6173
})
6274

63-
// Re-confirming the URL already in the field is the most likely thing a user
64-
// does in this window; restarting the app for it would be pure disruption.
75+
// The saved route carries the previous deployment's workspace id, and
76+
// resolveStartRoute only discards a route on a confirmed 403 — a fresh
77+
// partition answers 401, so a kept route would survive onto the new server.
78+
it('drops the saved route when the origin changes', () => {
79+
createServerWindow(deps).setOrigin('https://sim.other.example')
80+
81+
expect(deps.config.set).toHaveBeenCalledWith('lastRoute', undefined)
82+
})
83+
84+
it('keeps the saved route when the origin is unchanged', () => {
85+
createServerWindow(deps).setOrigin(CURRENT)
86+
87+
expect(deps.config.set).not.toHaveBeenCalled()
88+
})
89+
90+
// Re-confirming the pre-filled URL is the common case here.
6591
it('does not relaunch when the origin is unchanged', () => {
6692
const result = createServerWindow(deps).setOrigin(CURRENT)
6793

0 commit comments

Comments
 (0)