mirror of
https://github.com/AgentSeal/codeburn.git
synced 2026-08-22 23:14:33 +00:00
fix(app): resolve usePolled stale-response race + review nits
Review of T0 (Electron scaffold) approved with one Important finding + cheap
Minors. All fixes scoped to app/.
- usePolled: replace per-call cancel closure with a generation/epoch counter so
an orphaned in-flight fetch (from refresh() or an interval tick, discarded
cancel handle) can't resolve after a newer fetch and clobber fresh data. New
TDD race test (slow deps-A resolves after fast deps-B; keeps B).
- cli: nvm resolution now scans version dirs descending and takes the first
whose bin actually contains codeburn (was lexicographic max, unverified),
matching CodeburnCLI.swift. Export nodeManagerDirs + hermetic nvm test.
- index.html: tighten CSP connect-src ws: -> ws://localhost:5173.
- preload: import type { Envelope } from main instead of redeclaring it.
- main.test: table-driven channel->argv assertion over all 9 codeburn:* channels
plus a keys check and a non-spawning cliStatus case.
This commit is contained in:
parent
6e3729cfa9
commit
b36253d2e2
7 changed files with 156 additions and 21 deletions
|
|
@ -1,10 +1,10 @@
|
|||
// @vitest-environment node
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest'
|
||||
import { mkdtempSync, writeFileSync, rmSync, chmodSync } from 'node:fs'
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, rmSync, chmodSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
|
||||
import { spawnCli, CliError } from './cli'
|
||||
import { spawnCli, CliError, nodeManagerDirs } from './cli'
|
||||
|
||||
let dir: string
|
||||
const originalBin = process.env.CODEBURN_BIN
|
||||
|
|
@ -61,3 +61,35 @@ describe('spawnCli', () => {
|
|||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('nodeManagerDirs (nvm resolution)', () => {
|
||||
const savedNvm = process.env.NVM_DIR
|
||||
afterEach(() => {
|
||||
if (savedNvm === undefined) delete process.env.NVM_DIR
|
||||
else process.env.NVM_DIR = savedNvm
|
||||
})
|
||||
|
||||
it('scans nvm version dirs newest-first and takes the first that holds codeburn', () => {
|
||||
// Two versions; the lexicographically-"newest" (v9.0.0 > v22.0.0 as strings)
|
||||
// has NO codeburn, while the real newer v22.0.0 does. The old `sort().reverse()[0]`
|
||||
// would pick v9.0.0's bin and miss the CLI entirely.
|
||||
const nvm = mkdtempSync(join(tmpdir(), 'codeburn-nvm-'))
|
||||
try {
|
||||
const versions = join(nvm, 'versions', 'node')
|
||||
const v9bin = join(versions, 'v9.0.0', 'bin')
|
||||
const v22bin = join(versions, 'v22.0.0', 'bin')
|
||||
mkdirSync(v9bin, { recursive: true })
|
||||
mkdirSync(v22bin, { recursive: true })
|
||||
const codeburn = join(v22bin, 'codeburn')
|
||||
writeFileSync(codeburn, '#!/bin/sh\n', { mode: 0o755 })
|
||||
chmodSync(codeburn, 0o755)
|
||||
|
||||
process.env.NVM_DIR = nvm
|
||||
const dirs = nodeManagerDirs()
|
||||
expect(dirs).toContain(v22bin)
|
||||
expect(dirs).not.toContain(v9bin)
|
||||
} finally {
|
||||
rmSync(nvm, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@ const DEFAULT_TIMEOUT_MS = 45_000
|
|||
|
||||
// Homebrew + common Node version managers, mirroring mac/CodeburnCLI.swift so a
|
||||
// GUI-launched app (minimal PATH) still finds a globally-installed `codeburn`.
|
||||
function nodeManagerDirs(): string[] {
|
||||
export function nodeManagerDirs(): string[] {
|
||||
const home = homedir()
|
||||
const dirs = [
|
||||
'/opt/homebrew/bin',
|
||||
|
|
@ -34,10 +34,17 @@ function nodeManagerDirs(): string[] {
|
|||
const nvmDir = process.env.NVM_DIR || join(home, '.nvm')
|
||||
const nvmVersions = join(nvmDir, 'versions', 'node')
|
||||
try {
|
||||
// Prefer the newest installed node version's bin dir.
|
||||
const entries = readdirSync(nvmVersions)
|
||||
const newest = entries.sort().reverse()[0]
|
||||
if (newest) dirs.push(join(nvmVersions, newest, 'bin'))
|
||||
// Scan version dirs newest-first and take the first whose bin actually holds
|
||||
// `codeburn`. A lexicographic max ("v9" > "v22") is not a real "newest", and
|
||||
// the top dir may not even contain the CLI — so verify, matching CodeburnCLI.swift.
|
||||
const entries = readdirSync(nvmVersions).sort().reverse()
|
||||
for (const entry of entries) {
|
||||
const bin = join(nvmVersions, entry, 'bin')
|
||||
if (isExecutableFile(join(bin, 'codeburn'))) {
|
||||
dirs.push(bin)
|
||||
break
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// no nvm — ignore
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,6 +20,56 @@ function fakeSpawn(result: unknown = { current: { cost: 12.34 } }) {
|
|||
return { spawnCli, calls }
|
||||
}
|
||||
|
||||
// Every codeburn:* channel with a representative arg tuple → the exact argv it
|
||||
// must spawn. cliStatus is the one channel that resolves without spawning.
|
||||
const CHANNELS = [
|
||||
'codeburn:getOverview',
|
||||
'codeburn:getPlans',
|
||||
'codeburn:getModels',
|
||||
'codeburn:getYield',
|
||||
'codeburn:getSpendFlow',
|
||||
'codeburn:getDevices',
|
||||
'codeburn:getShareStatus',
|
||||
'codeburn:getIdentity',
|
||||
'codeburn:cliStatus',
|
||||
] as const
|
||||
|
||||
const ARGV_CASES: Array<{ channel: string; args: unknown[]; argv: string[] }> = [
|
||||
{ channel: 'codeburn:getOverview', args: ['30days', 'claude'], argv: ['status', '--format', 'menubar-json', '--period', '30days', '--provider', 'claude'] },
|
||||
{ channel: 'codeburn:getOverview', args: ['30days', 'all'], argv: ['status', '--format', 'menubar-json', '--period', '30days'] },
|
||||
{ channel: 'codeburn:getPlans', args: ['week'], argv: ['status', '--format', 'json', '--period', 'week'] },
|
||||
{ channel: 'codeburn:getModels', args: ['week', 'claude', true], argv: ['models', '--format', 'json', '--period', 'week', '--provider', 'claude', '--by-task'] },
|
||||
{ channel: 'codeburn:getModels', args: ['week', 'all', false], argv: ['models', '--format', 'json', '--period', 'week'] },
|
||||
{ channel: 'codeburn:getYield', args: ['today'], argv: ['yield', '--format', 'json', '--period', 'today'] },
|
||||
{ channel: 'codeburn:getSpendFlow', args: ['month', 'openai'], argv: ['spend', '--format', 'flow-json', '--period', 'month', '--provider', 'openai'] },
|
||||
{ channel: 'codeburn:getDevices', args: ['week'], argv: ['devices', '--format', 'json', '--period', 'week'] },
|
||||
{ channel: 'codeburn:getShareStatus', args: [], argv: ['share', 'status', '--format', 'json'] },
|
||||
{ channel: 'codeburn:getIdentity', args: [], argv: ['identity', '--format', 'json'] },
|
||||
]
|
||||
|
||||
describe('createBridgeHandlers (channel → argv for all channels)', () => {
|
||||
it('exposes exactly the nine codeburn:* channels', () => {
|
||||
const handlers = createBridgeHandlers({ spawnCli: vi.fn(), resolveCodeburnPath: () => null })
|
||||
expect(Object.keys(handlers).sort()).toEqual([...CHANNELS].sort())
|
||||
})
|
||||
|
||||
it.each(ARGV_CASES)('$channel with $args spawns the expected argv', async ({ channel, args, argv }) => {
|
||||
const { spawnCli, calls } = fakeSpawn()
|
||||
const handlers = createBridgeHandlers({ spawnCli, resolveCodeburnPath: () => '/bin/codeburn' })
|
||||
const res = await handlers[channel]!(...args)
|
||||
expect(calls[0]).toEqual(argv)
|
||||
expect(res).toMatchObject({ ok: true })
|
||||
})
|
||||
|
||||
it('codeburn:cliStatus resolves from resolveCodeburnPath without spawning', async () => {
|
||||
const spawnCli = vi.fn()
|
||||
const handlers = createBridgeHandlers({ spawnCli, resolveCodeburnPath: () => '/opt/homebrew/bin/codeburn' })
|
||||
const res = await handlers['codeburn:cliStatus']!()
|
||||
expect(spawnCli).not.toHaveBeenCalled()
|
||||
expect(res).toEqual({ ok: true, value: { found: true, path: '/opt/homebrew/bin/codeburn' } })
|
||||
})
|
||||
})
|
||||
|
||||
describe('createBridgeHandlers (IPC wiring)', () => {
|
||||
it('getOverview spawns menubar-json for the period, omitting --provider for "all"', async () => {
|
||||
const { spawnCli, calls } = fakeSpawn()
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
import { contextBridge, ipcRenderer } from 'electron'
|
||||
|
||||
// Envelope mirror of main.ts. Handlers resolve with { ok, value } | { ok, error }
|
||||
// so the structured error `kind` survives the contextBridge boundary.
|
||||
type Envelope<T = unknown> = { ok: true; value: T } | { ok: false; error: { kind: string; message: string } }
|
||||
// Handlers resolve with { ok, value } | { ok, error } so the structured error
|
||||
// `kind` survives the contextBridge boundary. `import type` is erased at build,
|
||||
// so this shares main.ts's declaration without pulling its runtime in.
|
||||
import type { Envelope } from './main'
|
||||
|
||||
async function invoke<T>(channel: string, ...args: unknown[]): Promise<T> {
|
||||
const res = (await ipcRenderer.invoke(channel, ...args)) as Envelope<T>
|
||||
|
|
|
|||
39
app/renderer/hooks/usePolled.test.ts
Normal file
39
app/renderer/hooks/usePolled.test.ts
Normal file
|
|
@ -0,0 +1,39 @@
|
|||
// @vitest-environment jsdom
|
||||
import { describe, it, expect, vi } from 'vitest'
|
||||
import { renderHook, act } from '@testing-library/react'
|
||||
|
||||
import { usePolled } from './usePolled'
|
||||
|
||||
describe('usePolled', () => {
|
||||
it('discards a stale in-flight fetch that resolves after a newer one (epoch guard)', async () => {
|
||||
// A fetcher we resolve by hand, one deferred per call, so we can force a
|
||||
// SLOW deps-A fetch to resolve AFTER a FAST deps-B fetch.
|
||||
const resolvers: Array<(v: string) => void> = []
|
||||
const fetcher = vi.fn(() => new Promise<string>(resolve => { resolvers.push(resolve) }))
|
||||
|
||||
const { result, rerender } = renderHook(
|
||||
({ p }: { p: string }) => usePolled(fetcher, [p]),
|
||||
{ initialProps: { p: 'A' } },
|
||||
)
|
||||
|
||||
// #0 mount fetch (deps A) — resolve it to establish a known baseline.
|
||||
await act(async () => { resolvers[0]!('A0') })
|
||||
expect(fetcher).toHaveBeenCalledTimes(1)
|
||||
expect(result.current.data).toBe('A0')
|
||||
|
||||
// #1 refresh() while deps are still A → an in-flight SLOW fetch whose cancel
|
||||
// handle the hook discards. Leave it unresolved for now.
|
||||
act(() => { result.current.refresh() })
|
||||
expect(fetcher).toHaveBeenCalledTimes(2)
|
||||
|
||||
// #2 deps change A→B → a FAST fetch that resolves first with fresh data.
|
||||
rerender({ p: 'B' })
|
||||
expect(fetcher).toHaveBeenCalledTimes(3)
|
||||
await act(async () => { resolvers[2]!('B-fresh') })
|
||||
expect(result.current.data).toBe('B-fresh')
|
||||
|
||||
// #1 (the slow deps-A fetch) now resolves LATE. It must NOT clobber B.
|
||||
await act(async () => { resolvers[1]!('A-stale') })
|
||||
expect(result.current.data).toBe('B-fresh')
|
||||
})
|
||||
})
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
import { useCallback, useEffect, useState } from 'react'
|
||||
import { useCallback, useEffect, useRef, useState } from 'react'
|
||||
|
||||
import { normalizeCliError } from '../lib/ipc'
|
||||
import type { CliError } from '../lib/types'
|
||||
|
|
@ -20,35 +20,41 @@ export function usePolled<T>(fetcher: () => Promise<T>, deps: unknown[], interva
|
|||
const [data, setData] = useState<T | null>(null)
|
||||
const [error, setError] = useState<CliError | null>(null)
|
||||
const [loading, setLoading] = useState(true)
|
||||
// Generation counter: every load() (mount, deps change, interval, refresh)
|
||||
// claims the next epoch; a fetch applies its result only while its epoch is
|
||||
// still current. This is what keeps a slow fetch from an older deps/period
|
||||
// from clobbering a newer one that already resolved.
|
||||
const epochRef = useRef(0)
|
||||
|
||||
const load = useCallback(() => {
|
||||
let cancelled = false
|
||||
const epoch = ++epochRef.current
|
||||
setLoading(true)
|
||||
fetcher()
|
||||
.then(result => {
|
||||
if (cancelled) return
|
||||
if (epochRef.current !== epoch) return
|
||||
setData(result)
|
||||
setError(null)
|
||||
})
|
||||
.catch(err => {
|
||||
if (!cancelled) setError(normalizeCliError(err))
|
||||
if (epochRef.current !== epoch) return
|
||||
setError(normalizeCliError(err))
|
||||
})
|
||||
.finally(() => {
|
||||
if (!cancelled) setLoading(false)
|
||||
if (epochRef.current !== epoch) return
|
||||
setLoading(false)
|
||||
})
|
||||
return () => {
|
||||
cancelled = true
|
||||
}
|
||||
// deps are intentionally the caller-provided dependency list.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, deps)
|
||||
|
||||
useEffect(() => {
|
||||
const cancel = load()
|
||||
load()
|
||||
const id = setInterval(() => load(), intervalMs)
|
||||
return () => {
|
||||
cancel()
|
||||
clearInterval(id)
|
||||
// Retire this generation so an in-flight fetch can't resolve into state
|
||||
// after unmount or a deps change.
|
||||
epochRef.current++
|
||||
}
|
||||
}, [load, intervalMs])
|
||||
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@
|
|||
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
|
||||
<meta
|
||||
http-equiv="Content-Security-Policy"
|
||||
content="default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; connect-src 'self' ws: http://localhost:5173"
|
||||
content="default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; connect-src 'self' ws://localhost:5173 http://localhost:5173"
|
||||
/>
|
||||
<title>CodeBurn</title>
|
||||
<style>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue