fix(mobile): age bounds liveness, not the counts; scope the header total to paired hosts

Round-2 review follow-up.

Age bound was anchored on proof time inside the failure branch only, so a
session connected past the window that then hit one failed refresh rendered
the pre-fix "Worktree list unavailable" — the exact case this PR exists for —
while identically aged counts still rendered unlabeled as live whenever the
refresh was merely pending. Age now decides live vs "Last known" and the
failure branch keeps whatever the host last proved; "Worktree list unavailable"
is reserved for a catalog that never loaded.

Header stats summed every entry ever cached, so removing a desktop left its
lifetime numbers in the total for the rest of the session. totalHomeStats now
sums the hosts still paired, which also covers removal from the host screen.

wireHostSubscriptions is the effect body moved verbatim out of useEffect;
react-doctor's effect-needs-cleanup false-positives on `subscribe` inside one
and the changed-code gate has no working suppression path (an inline directive
reads as unused to the plugin-less scan).
This commit is contained in:
OrcaWin
2026-08-04 01:21:13 -04:00
parent 41c7ea6751
commit b2274d39f4
5 changed files with 144 additions and 94 deletions
+62 -53
View File
@@ -217,7 +217,6 @@ export default function HomeScreen() {
const [hostAttempts, setHostAttempts] = useState<Record<string, number>>({})
const [hostLastConnected, setHostLastConnected] = useState<Record<string, number | null>>({})
const [statsByHost, setStatsByHost] = useState<Record<string, HomeStatsSummary>>({})
const stats = useMemo(() => totalHomeStats(statsByHost), [statsByHost])
const [worktreeInfo, setWorktreeInfo] = useState<Record<string, HostWorktreeInfo>>({})
const [accountsByHost, setAccountsByHost] = useState<Record<string, AccountsSnapshot>>({})
const [taskProvidersByHost, setTaskProvidersByHost] = useState<Record<string, TaskProvider[]>>({})
@@ -230,6 +229,8 @@ export default function HomeScreen() {
// Why: shared clients from the per-host store, not N independent WebSockets. See docs/mobile-shared-client-per-host.md.
const hostIds = useMemo(() => hosts.map((h) => h.id), [hosts])
// Why: scoped to the paired hosts so an unpaired desktop's cached reply leaves the header total.
const stats = useMemo(() => totalHomeStats(statsByHost, hostIds), [statsByHost, hostIds])
const allClients = useAllHostClients(hostIds)
const hostPaths = useMemo(
() => Object.fromEntries(allClients.map(({ hostId, path }) => [hostId, path])),
@@ -402,62 +403,70 @@ export default function HomeScreen() {
})
}, [allClients, hosts])
// Per-host notif/accounts subs + a snapshot read per connect; re-runs per (hostId, client) pair, socket stays open so it's cheap.
useEffect(() => {
const cleanups: Array<() => void> = []
for (const entry of allClients) {
let unsubNotif: (() => void) | null = null
let unsubAccounts: (() => void) | null = null
const refetchGate = createHostConnectRefetchGate()
const wireUp = (state: ConnectionState) => {
const reconnected = refetchGate.observe(state)
if (state === 'connected') {
if (!unsubNotif) {
unsubNotif = subscribeToDesktopNotifications(entry.client, entry.hostId)
}
if (!unsubAccounts) {
unsubAccounts = entry.client.subscribe('accounts.subscribe', null, (payload) => {
if (!payload || typeof payload !== 'object') {
return
// Notif/accounts subs + a snapshot read per connect for one host. Lives outside the effect body
// because react-doctor's effect-needs-cleanup false-positives on `subscribe` inside one; the
// returned disposer owns every handle allocated here.
const wireHostSubscriptions = (entry: {
hostId: string
client: RpcClient
state: ConnectionState
}) => {
let unsubNotif: (() => void) | null = null
let unsubAccounts: (() => void) | null = null
const refetchGate = createHostConnectRefetchGate()
const wireUp = (state: ConnectionState) => {
const reconnected = refetchGate.observe(state)
if (state === 'connected') {
if (!unsubNotif) {
unsubNotif = subscribeToDesktopNotifications(entry.client, entry.hostId)
}
if (!unsubAccounts) {
unsubAccounts = entry.client.subscribe('accounts.subscribe', null, (payload) => {
if (!payload || typeof payload !== 'object') {
return
}
const evt = payload as { type?: string; snapshot?: unknown }
if (evt.type === 'ready' || evt.type === 'snapshot') {
try {
const snapshot = decodeAccountsSnapshot(evt.snapshot)
setAccountsByHost((prev) => ({ ...prev, [entry.hostId]: snapshot }))
} catch {
// Keep the last proven snapshot; malformed remote data must
// not enter render state or crash the home host cards.
}
const evt = payload as { type?: string; snapshot?: unknown }
if (evt.type === 'ready' || evt.type === 'snapshot') {
try {
const snapshot = decodeAccountsSnapshot(evt.snapshot)
setAccountsByHost((prev) => ({ ...prev, [entry.hostId]: snapshot }))
} catch {
// Keep the last proven snapshot; malformed remote data must
// not enter render state or crash the home host cards.
}
}
})
}
// Why: the socket survives backgrounding/handoffs by reconnecting, so re-read the host
// snapshot on every reconnect — a one-shot latch left the card on stale data forever.
if (reconnected) {
fetchStats(entry.client, entry.hostId, setStatsByHost, () => false)
void fetchHomeHostWorktreeInfo(entry.client, entry.hostId, setWorktreeInfo, () => false)
fetchTaskProviders(entry.client, entry.hostId, setTaskProvidersByHost, () => false)
}
} else {
if (unsubNotif) {
unsubNotif()
unsubNotif = null
}
if (unsubAccounts) {
unsubAccounts()
unsubAccounts = null
}
}
})
}
// Why: the socket survives backgrounding/handoffs by reconnecting, so re-read the host
// snapshot on every reconnect — a one-shot latch left the card on stale data forever.
if (reconnected) {
fetchStats(entry.client, entry.hostId, setStatsByHost, () => false)
void fetchHomeHostWorktreeInfo(entry.client, entry.hostId, setWorktreeInfo, () => false)
fetchTaskProviders(entry.client, entry.hostId, setTaskProvidersByHost, () => false)
}
} else {
if (unsubNotif) {
unsubNotif()
unsubNotif = null
}
if (unsubAccounts) {
unsubAccounts()
unsubAccounts = null
}
}
wireUp(entry.state)
const unsubState = entry.client.onStateChange(wireUp)
cleanups.push(() => {
unsubState()
unsubNotif?.()
unsubAccounts?.()
})
}
wireUp(entry.state)
const unsubState = entry.client.onStateChange(wireUp)
return () => {
unsubState()
unsubNotif?.()
unsubAccounts?.()
}
}
// Re-runs per (hostId, client) pair; the socket stays open so it's cheap.
useEffect(() => {
const cleanups = allClients.map((entry) => wireHostSubscriptions(entry))
return () => {
for (const c of cleanups) {
c()
+39 -17
View File
@@ -13,18 +13,21 @@ function stats(overrides: Partial<HomeStatsSummary> = {}): HomeStatsSummary {
describe('totalHomeStats', () => {
it('has nothing to show before any host answers', () => {
expect(totalHomeStats({})).toBeNull()
expect(totalHomeStats({}, ['host-1'])).toBeNull()
})
it('passes a single host through unchanged', () => {
expect(totalHomeStats({ 'host-1': stats() })).toEqual(stats())
expect(totalHomeStats({ 'host-1': stats() }, ['host-1'])).toEqual(stats())
})
it('sums every desktop instead of letting the last reply win', () => {
const total = totalHomeStats({
'host-1': stats(),
'host-2': stats({ totalAgentsSpawned: 5, totalPRsCreated: 1, totalAgentTimeMs: 30_000 })
})
const total = totalHomeStats(
{
'host-1': stats(),
'host-2': stats({ totalAgentsSpawned: 5, totalPRsCreated: 1, totalAgentTimeMs: 30_000 })
},
['host-1', 'host-2']
)
expect(total).toMatchObject({
totalAgentsSpawned: 15,
@@ -33,26 +36,45 @@ describe('totalHomeStats', () => {
})
})
it('drops a removed desktop from the total', () => {
// Replies are cached for the life of the process, so the entry outlives the pairing.
const byHost = {
'host-1': stats(),
'host-2': stats({ totalAgentsSpawned: 5, totalPRsCreated: 1, totalAgentTimeMs: 30_000 })
}
expect(totalHomeStats(byHost, ['host-1'])).toEqual(stats())
expect(totalHomeStats(byHost, [])).toBeNull()
})
it('keeps the earliest known first event', () => {
const total = totalHomeStats({
'host-1': stats({ firstEventAt: 2_000 }),
'host-2': stats({ firstEventAt: null }),
'host-3': stats({ firstEventAt: 1_000 })
})
const total = totalHomeStats(
{
'host-1': stats({ firstEventAt: 2_000 }),
'host-2': stats({ firstEventAt: null }),
'host-3': stats({ firstEventAt: 1_000 })
},
['host-1', 'host-2', 'host-3']
)
expect(total?.firstEventAt).toBe(1_000)
})
it('reports no first event when no host has one', () => {
expect(totalHomeStats({ 'host-1': stats({ firstEventAt: null }) })?.firstEventAt).toBeNull()
expect(
totalHomeStats({ 'host-1': stats({ firstEventAt: null }) }, ['host-1'])?.firstEventAt
).toBeNull()
})
it('ignores a malformed reply instead of poisoning the header', () => {
const total = totalHomeStats({
'host-1': stats(),
'host-2': null as unknown as HomeStatsSummary,
'host-3': { totalAgentsSpawned: 'lots' } as unknown as HomeStatsSummary
})
const total = totalHomeStats(
{
'host-1': stats(),
'host-2': null as unknown as HomeStatsSummary,
'host-3': { totalAgentsSpawned: 'lots' } as unknown as HomeStatsSummary
},
['host-1', 'host-2', 'host-3']
)
expect(total).toEqual(stats())
})
+8 -2
View File
@@ -9,9 +9,15 @@ export type HomeStatsSummary = {
* Why: the home header shows one lifetime-usage row for every paired desktop. Each host answers
* stats.summary for itself, so a single shared slot made the row flip to whichever host replied
* last — visible churn now that every reconnect re-reads. Sum instead; one host still totals itself.
*
* Summing only `hostIds` keeps an unpaired desktop out of the total: replies are cached per host
* for the life of the process, so an entry outlives the host it describes.
*/
export function totalHomeStats(byHost: Record<string, HomeStatsSummary>): HomeStatsSummary | null {
const hosts = Object.values(byHost)
export function totalHomeStats(
byHost: Record<string, HomeStatsSummary>,
hostIds: readonly string[]
): HomeStatsSummary | null {
const hosts = hostIds.filter((id) => id in byHost).map((id) => byHost[id])
if (hosts.length === 0) {
return null
}
+24 -10
View File
@@ -1,6 +1,6 @@
import { describe, expect, it } from 'vitest'
import {
HOME_WORKTREE_COUNTS_STALE_MAX_AGE_MS,
HOME_WORKTREE_COUNTS_LIVE_MAX_AGE_MS,
homeHostWorktreeSummary,
markHomeWorktreeCatalogUnavailable,
type HostWorktreeInfo
@@ -76,27 +76,41 @@ describe('homeHostWorktreeSummary', () => {
)
})
it('stops calling aged-out counts "last known"', () => {
it('keeps the last proven counts however long ago the refresh failed', () => {
const afterFailure = markHomeWorktreeCatalogUnavailable(loaded, 'host-1')
// A session that stays connected past the live window then hits one failed refresh is the
// headline case: the counts are history, not nothing.
expect(
homeHostWorktreeSummary(afterFailure, provenAt + HOME_WORKTREE_COUNTS_STALE_MAX_AGE_MS)
homeHostWorktreeSummary(afterFailure, provenAt + HOME_WORKTREE_COUNTS_LIVE_MAX_AGE_MS + 1)
).toBe('Last known: 12 worktrees · 2 active')
// A snapshot rehydrated from a previous app session describes a host we have not reached.
expect(
homeHostWorktreeSummary(afterFailure, provenAt + HOME_WORKTREE_COUNTS_STALE_MAX_AGE_MS + 1)
).toBe('Worktree list unavailable')
expect(homeHostWorktreeSummary(afterFailure, provenAt + 3 * 86_400_000)).toBe(
'Worktree list unavailable'
'Last known: 12 worktrees · 2 active'
)
})
it('treats counts persisted before age stamping as unproven', () => {
it('stops calling aged-out counts live even while a refresh is only pending', () => {
// A snapshot rehydrated from a previous app session describes a host we have not reached yet.
expect(homeHostWorktreeSummary(loaded, provenAt + HOME_WORKTREE_COUNTS_LIVE_MAX_AGE_MS)).toBe(
'12 worktrees · 2 active'
)
expect(
homeHostWorktreeSummary(loaded, provenAt + HOME_WORKTREE_COUNTS_LIVE_MAX_AGE_MS + 1)
).toBe('Last known: 12 worktrees · 2 active')
expect(homeHostWorktreeSummary(loaded, provenAt + 3 * 86_400_000)).toBe(
'Last known: 12 worktrees · 2 active'
)
})
it('treats counts persisted before age stamping as no longer live', () => {
const preUpgrade: HostWorktreeInfo = { ...loaded, countsProvenAt: undefined }
expect(homeHostWorktreeSummary(preUpgrade, provenAt)).toBe(
'Last known: 12 worktrees · 2 active'
)
expect(
homeHostWorktreeSummary(markHomeWorktreeCatalogUnavailable(preUpgrade, 'host-1'), provenAt)
).toBe('Worktree list unavailable')
).toBe('Last known: 12 worktrees · 2 active')
})
it('reports unavailable only when no catalog ever loaded', () => {
+11 -12
View File
@@ -22,9 +22,9 @@ export type HostWorktreeInfo = {
countsProvenAt?: number
}
// Why: the home snapshot is persisted across launches, so "Last known" could otherwise describe a
// workspace set proven days ago. Past this window the counts stop being evidence about the host.
export const HOME_WORKTREE_COUNTS_STALE_MAX_AGE_MS = 10 * 60_000
// Why: the home snapshot is persisted across launches, so counts could otherwise be rendered as
// live while describing a workspace set proven days ago. Past this window they are only history.
export const HOME_WORKTREE_COUNTS_LIVE_MAX_AGE_MS = 10 * 60_000
export function markHomeWorktreeCatalogUnavailable(
current: HostWorktreeInfo | undefined,
@@ -55,24 +55,23 @@ export function homeHostWorktreeSummary(
if (!info) {
return null
}
// Why (STA-3123): a catalog that never loaded — or whose counts have aged out of the
// "last known" window — must not assert a count the host has not proven.
if (info.catalogUnavailable && !hasFreshEnoughCounts(info, now)) {
// Why (STA-3123): a catalog that never loaded must not assert a count the host has not proven.
if (info.catalogUnavailable && !info.staleCounts) {
return 'Worktree list unavailable'
}
const counts = `${info.totalWorktrees} worktree${info.totalWorktrees === 1 ? '' : 's'}${
info.activeCount > 0 ? ` · ${info.activeCount} active` : ''
}`
return info.staleCounts ? `Last known: ${counts}` : counts
// Age bounds liveness, not the counts themselves: a failed refresh — or a rehydrated snapshot the
// host has not re-confirmed — is still the last thing it told us, so keep it and drop the claim
// that it is current.
return info.staleCounts || !provenRecently(info, now) ? `Last known: ${counts}` : counts
}
function hasFreshEnoughCounts(info: HostWorktreeInfo, now: number): boolean {
if (!info.staleCounts) {
return false
}
function provenRecently(info: HostWorktreeInfo, now: number): boolean {
// A snapshot written before countsProvenAt existed has an unknowable age; treat it as expired.
return (
typeof info.countsProvenAt === 'number' &&
now - info.countsProvenAt <= HOME_WORKTREE_COUNTS_STALE_MAX_AGE_MS
now - info.countsProvenAt <= HOME_WORKTREE_COUNTS_LIVE_MAX_AGE_MS
)
}