From 0b8bd1c3c73e05dc6c20b75740a704c482536d20 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 17 Sep 2026 02:09:04 -0400 Subject: [PATCH] refactor(mobile): stop requiring the stats row its own reader guards Round-2 findings 1 to 3. `homeHostStatsSchema` required an object that `totalHomeStats` already guards (`if (!host || typeof host !== 'object') continue`), so the requirement bought nothing at the read and cost the row upstream: the refusal reached `fetchMobileHomeStats`'s `.catch`, the per-host slot was never written, `hostIds.filter` found no host and the Home header drew no stats row where main drew `0 / 0s / 0`. It takes `.nullish()`, and `HomeStatsRow` admits the `null | undefined` main always had. The unit pin now says the slot keeps a null summary and the total skips it, and sums one through `totalHomeStats` to show the zeroed row survives. The Home card's `SAFETY:` note claimed the reader proves `worktrees` is an array. It does not; the `?? []` does. That is the same false sentence round 1 removed from the agent-history panel, and a reader who believed it would delete the `??` and reintroduce the white screen. The `catalogError` branch on `RpcIncompatibleReplyError` had nothing holding it: no adapter mounts the host screen, so no golden can reach it. One case in the snapshot client pins the class the `catch` keys on. Mutation-checked by forwarding the catalog schema as `z.unknown()`, which fails that case alone. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../src/home/home-host-reply-schema.test.ts | 34 ++++++++++++++----- mobile/src/home/home-host-reply-schema.ts | 24 +++++++------ mobile/src/stats/home-stats-total.ts | 8 +++-- .../src/worktree/home-host-worktree-fetch.ts | 2 +- .../worktree-catalog-snapshot-client.test.ts | 13 +++++++ 5 files changed, 59 insertions(+), 22 deletions(-) diff --git a/mobile/src/home/home-host-reply-schema.test.ts b/mobile/src/home/home-host-reply-schema.test.ts index d62f3f14146..dc8bcfd7268 100644 --- a/mobile/src/home/home-host-reply-schema.test.ts +++ b/mobile/src/home/home-host-reply-schema.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import type { z } from 'zod' +import { totalHomeStats } from '../stats/home-stats-total' import { homeHostAccountsSchema, homeHostStatsSchema } from './home-host-reply-schema' function reads(schema: z.ZodType, value: unknown): T { @@ -10,6 +11,14 @@ function reads(schema: z.ZodType, value: unknown): T { return parsed.data } +function readsRow(value: unknown): NonNullable> { + const row = reads(homeHostStatsSchema, value) + if (!row) { + throw new Error('expected an object row') + } + return row +} + function refuses(schema: z.ZodType, value: unknown): boolean { return !schema.safeParse(value).success } @@ -22,22 +31,31 @@ describe('a stats row is checked as an object and nothing more', () => { totalAgentTimeMs: 90, firstEventAt: 1700000000000 } - expect(reads(homeHostStatsSchema, row)).toMatchObject(row) + expect(readsRow(row)).toMatchObject(row) }) it('reads a host that answers a shape totalHomeStats still sums', () => { - expect(reads(homeHostStatsSchema, { totalWorktrees: 3 })).toMatchObject({ totalWorktrees: 3 }) - expect(reads(homeHostStatsSchema, {}).totalAgentsSpawned).toBe(undefined) + expect(readsRow({ totalWorktrees: 3 })).toMatchObject({ totalWorktrees: 3 }) + expect(readsRow({}).totalAgentsSpawned).toBe(undefined) }) it('preserves an explicit null firstEventAt, which the total reads as no events yet', () => { - expect(reads(homeHostStatsSchema, { firstEventAt: null }).firstEventAt).toBe(null) - expect(reads(homeHostStatsSchema, { firstEventAt: 'never' }).firstEventAt).toBe(undefined) + expect(readsRow({ firstEventAt: null }).firstEventAt).toBe(null) + expect(readsRow({ firstEventAt: 'never' }).firstEventAt).toBe(undefined) }) - it('keeps a null or absent summary out of the card slot', () => { - expect(refuses(homeHostStatsSchema, null)).toBe(true) - expect(refuses(homeHostStatsSchema, undefined)).toBe(true) + // Main seated a null or absent summary in the per-host slot and `totalHomeStats` skipped it, so + // the header still drew a zeroed row. Refusing here would empty the row instead of zeroing it. + it('seats a null or absent summary in the card slot, which the total skips', () => { + expect(refuses(homeHostStatsSchema, null)).toBe(false) + expect(refuses(homeHostStatsSchema, undefined)).toBe(false) + expect(reads(homeHostStatsSchema, null)).toBe(null) + expect(totalHomeStats({ 'host-1': reads(homeHostStatsSchema, null) }, ['host-1'])).toEqual({ + totalAgentsSpawned: 0, + totalPRsCreated: 0, + totalAgentTimeMs: 0, + firstEventAt: null + }) }) }) diff --git a/mobile/src/home/home-host-reply-schema.ts b/mobile/src/home/home-host-reply-schema.ts index bd179191950..a04e7bf01fa 100644 --- a/mobile/src/home/home-host-reply-schema.ts +++ b/mobile/src/home/home-host-reply-schema.ts @@ -8,21 +8,23 @@ import { salvagedOptional } from '../../../src/shared/zod-salvage' /** * One host's lifetime-usage row. * - * Every member is optional and none is required, because `totalHomeStats` is the reader and it says - * so itself: it skips a non-object row and runs every number through `finiteOrZero` - * (home-stats-total.ts:33-39). What the schema adds is that the stored row is an object at all — - * the card keeps one slot per host for the life of the process, so a null or absent summary used to - * sit in that slot until the host replied again. + * Nothing here is required, not even the object. `totalHomeStats` is the reader and it guards the + * row itself (`if (!host || typeof host !== 'object') continue`, home-stats-total.ts:36), so + * requiring the object would buy nothing at the read and would cost the row upstream: the refusal + * reaches `fetchMobileHomeStats`'s `.catch`, the per-host slot is never written, `hostIds.filter` + * finds no host and the header draws no stats row where main drew a zeroed one. * * `firstEventAt` keeps its explicit `null`: that is the host's "no events yet", and the total * distinguishes it from a number when taking the minimum. */ -export const homeHostStatsSchema = z.looseObject({ - totalAgentsSpawned: salvagedOptional('totalAgentsSpawned', z.number()), - totalPRsCreated: salvagedOptional('totalPRsCreated', z.number()), - totalAgentTimeMs: salvagedOptional('totalAgentTimeMs', z.number()), - firstEventAt: salvagedOptional('firstEventAt', z.number().nullable()) -}) +export const homeHostStatsSchema = z + .looseObject({ + totalAgentsSpawned: salvagedOptional('totalAgentsSpawned', z.number()), + totalPRsCreated: salvagedOptional('totalPRsCreated', z.number()), + totalAgentTimeMs: salvagedOptional('totalAgentTimeMs', z.number()), + firstEventAt: salvagedOptional('firstEventAt', z.number().nullable()) + }) + .nullish() /** * One host's accounts snapshot, forwarded whole. diff --git a/mobile/src/stats/home-stats-total.ts b/mobile/src/stats/home-stats-total.ts index 2df7abd3724..66b46ddc391 100644 --- a/mobile/src/stats/home-stats-total.ts +++ b/mobile/src/stats/home-stats-total.ts @@ -13,8 +13,12 @@ export type HomeStatsSummary = { * 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. */ -/** One host's row as the wire carries it: every field may be missing or the wrong type. */ -export type HomeStatsRow = Partial +/** + * One host's row as the wire carries it: the row itself may be null or absent, and every field may + * be missing or the wrong type. The loop below guards all three, which is why the reader requires + * none of them. + */ +export type HomeStatsRow = Partial | null | undefined export function totalHomeStats( byHost: Record, diff --git a/mobile/src/worktree/home-host-worktree-fetch.ts b/mobile/src/worktree/home-host-worktree-fetch.ts index eb669b622bb..d854e73edb5 100644 --- a/mobile/src/worktree/home-host-worktree-fetch.ts +++ b/mobile/src/worktree/home-host-worktree-fetch.ts @@ -47,7 +47,7 @@ export function fetchHomeHostWorktreeInfo( markUnavailable() return } - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the reader proves `worktrees` is an array and leaves the rows opaque, because three screens project a row differently. This card reads `status` and the resume pick, both through their own guards, and the `worktree-home-catalog` golden records the row it is given as `{worktreeId, displayName, repo, status}`. + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: `worktrees` is a salvaged member, so the `?? []` is what makes it an array; the rows stay opaque because three screens project a row differently. This card reads `status` and the resume pick, both through their own guards, and the `worktree-home-catalog` golden records the row it is given as `{worktreeId, displayName, repo, status}`. const worktrees = (catalog.value.worktrees ?? []) as HomeWorktreeSummary[] setCachedWorktrees(hostId, worktrees, { proven: true }) const active = worktrees.filter((w) => w.status && ACTIVE_STATUSES.has(w.status)) diff --git a/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts b/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts index eb01ad80aec..3260f395d06 100644 --- a/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts +++ b/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' +import { RpcIncompatibleReplyError } from '../transport/rpc-incompatible-reply-error' import { admitWorktreeCatalogResponse, WORKTREE_PS_FULL_LIMIT, @@ -221,6 +222,18 @@ describe('WorktreeCatalogSnapshotClient', () => { }) }) + // Why this belongs here: `use-host-worktree-catalog.ts:125` keys its `invalid_response` state on + // the error *class*, and no adapter mounts that screen, so this is the only place the class is + // pinned. Deleting the reader leaves a host-payload defect reported to the user as a network one. + it('rejects with RpcIncompatibleReplyError when the host answers a result the reader refuses', async () => { + const client = clientWithResults('all') + const snapshots = new WorktreeCatalogSnapshotClient() + + await expect(snapshots.fetch(client, 'host-1')).rejects.toBeInstanceOf( + RpcIncompatibleReplyError + ) + }) + it('falls back to a generic failure code when the error carries none', async () => { const client = { sendRequest: vi.fn(async () => ({ id: 'request', ok: false, error: { message: 'x' } }))