From 500219b8ef2bee47f40e191232f48c19f021cfa3 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 27 Aug 2026 09:02:30 -0700 Subject: [PATCH] fix(catalog): make ProjectHostSetup field types true at the ingest boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Crash 3bcc5be3: a setup row whose repoId arrived null reached Settings' projectByRepoId memo and threw on .trim(). The type said `string`; persisted JSON and remote hosts on other versions can disagree. Normalize project/setup rows where untrusted data enters typed code — the persisted-state load (marking dirty, which is the migration), profile transfer reads, the repo-derived projection, and the renderer's IPC/RPC ingest and adoption steps — instead of re-guarding each consumer. Also covers `setup.path`, whose identical crash is on the sidebar render path. Coercion only: never drops a row, never adds or removes an optional key, and returns input references when a row already conforms, so selector and useMemo identity is unchanged. --- .../normalize-loaded-project-catalog.test.ts | 51 +++++ .../project-catalog-null-field-ingest.test.ts | 175 ++++++++++++++++++ 2 files changed, 226 insertions(+) create mode 100644 src/main/persistence/loading-store/normalize-loaded-project-catalog.test.ts create mode 100644 src/renderer/src/store/projects/project-catalog-null-field-ingest.test.ts diff --git a/src/main/persistence/loading-store/normalize-loaded-project-catalog.test.ts b/src/main/persistence/loading-store/normalize-loaded-project-catalog.test.ts new file mode 100644 index 00000000000..22d83ad4e3c --- /dev/null +++ b/src/main/persistence/loading-store/normalize-loaded-project-catalog.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it, vi } from 'vitest' +import type { PersistedState } from '../../../shared/persisted-state-types' +import { normalizeLoadedProjectCatalog } from './normalize-loaded-state-collections' + +function makeParsed(setups: unknown[], projects: unknown[] = []): PersistedState { + return { projects, projectHostSetups: setups } as unknown as PersistedState +} + +const badSetup = { + id: 'setup-1', + projectId: 'project-1', + hostId: 'local', + repoId: null, + path: null, + displayName: 'orca', + setupState: 'ready', + setupMethod: 'legacy-repo', + createdAt: 1, + updatedAt: 1 +} + +describe('normalizeLoadedProjectCatalog', () => { + it('repairs stored rows whose field types do not match the declared ones', () => { + const result = normalizeLoadedProjectCatalog(makeParsed([{ ...badSetup }]), vi.fn()) + expect(result.projectHostSetups[0]?.repoId).toBe('') + expect(result.projectHostSetups[0]?.path).toBe('') + }) + + // Why: without a save the bad rows stay on disk, get repaired again every launch, and this + // host keeps publishing them to paired clients. Marking dirty is the migration. + it('marks the profile dirty so the repair is persisted', () => { + const markNeedsSave = vi.fn() + normalizeLoadedProjectCatalog(makeParsed([{ ...badSetup }]), markNeedsSave) + expect(markNeedsSave).toHaveBeenCalled() + }) + + it('leaves a conforming catalog untouched and does not schedule a save', () => { + const markNeedsSave = vi.fn() + const setups = [{ ...badSetup, repoId: 'repo-1', path: '/repo' }] + const parsed = makeParsed(setups) + const result = normalizeLoadedProjectCatalog(parsed, markNeedsSave) + expect(result.projectHostSetups).toBe(setups) + expect(markNeedsSave).not.toHaveBeenCalled() + }) + + it('tolerates missing or non-array collections', () => { + const result = normalizeLoadedProjectCatalog({} as PersistedState, vi.fn()) + expect(result.projects).toEqual([]) + expect(result.projectHostSetups).toEqual([]) + }) +}) diff --git a/src/renderer/src/store/projects/project-catalog-null-field-ingest.test.ts b/src/renderer/src/store/projects/project-catalog-null-field-ingest.test.ts new file mode 100644 index 00000000000..b810e49529e --- /dev/null +++ b/src/renderer/src/store/projects/project-catalog-null-field-ingest.test.ts @@ -0,0 +1,175 @@ +import { describe, expect, it, vi } from 'vitest' +import type { Project, ProjectHostSetup } from '../../../../shared/project-types' +import type { Repo } from '../../../../shared/repo-types' +import { getProjectHostSetupProjectionFromState } from '../project-host-setup-selector' +import { fetchProjectHostSetupCompatibility, setupWithFetchedOwner } from './project-host-routing' + +// Crash 3bcc5be3 (v1.4.188, Linux, page.settings boundary): a setup row whose repoId arrived +// null reached Settings' projectByRepoId useMemo and threw "Cannot read properties of null +// (reading 'trim')". Normalizing at ingest is what must make that unreachable — by the time a +// row is in the store, its declared field types hold. +const repos = [ + { id: 'repo-1', path: '/Users/alice/orca', displayName: 'orca', badgeColor: '#000', addedAt: 1 } +] as unknown as Repo[] + +const projects = [ + { + id: 'repo:repo-1', + displayName: 'orca', + badgeColor: '#000', + sourceRepoIds: ['repo-1'], + createdAt: 1, + updatedAt: 1 + } +] as unknown as Project[] + +// Mirrors the Settings.tsx projectByRepoId useMemo that crashed. +function buildProjectByRepoIdLikeSettings( + setups: readonly ProjectHostSetup[], + projectList: readonly Project[] +): Map { + const projectById = new Map(projectList.map((project) => [project.id, project])) + const byRepoId = new Map() + for (const setup of setups) { + const project = projectById.get(setup.projectId) + if (project && setup.repoId.trim()) { + byRepoId.set(setup.repoId, project) + } + } + return byRepoId +} + +// Mirrors project-grouping.ts's checkout identity, which runs on the sidebar render path. +function checkoutIdentityLikeSidebar(setup: ProjectHostSetup): string { + return setup.path.trim() || setup.repoId || setup.id +} + +function badSetups(): unknown[] { + return [ + { + id: 'repo:repo-1::local', + projectId: 'repo:repo-1', + hostId: 'local', + repoId: 'repo-1', + path: '/Users/alice/orca', + displayName: 'orca', + setupState: 'ready', + setupMethod: 'legacy-repo', + createdAt: 1, + updatedAt: 1 + }, + { + id: 'repo:repo-1::local::2', + projectId: 'repo:repo-1', + hostId: 'local', + repoId: null, + path: null, + displayName: 'orca-2', + setupState: 'ready', + setupMethod: 'legacy-repo', + createdAt: 1, + updatedAt: 1 + } + ] +} + +function stubProjectsApi(setups: unknown[], projectRows: unknown[] = projects): void { + vi.stubGlobal('window', { + api: { + projects: { + list: vi.fn().mockResolvedValue(projectRows), + listHostSetups: vi.fn().mockResolvedValue(setups) + } + } + }) +} + +describe('project catalog ingest with non-string row fields', () => { + it('coerces a null repoId and path on the local IPC boundary', async () => { + stubProjectsApi(badSetups()) + const projection = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + const row = projection.setups.find((setup) => setup.id === 'repo:repo-1::local::2') + expect(row?.repoId).toBe('') + expect(row?.path).toBe('') + }) + + // Why: a remote host on a different Orca version is a first-class source of these rows, and + // decoders hand them over verbatim — the client cannot assume the host already repaired them. + it('coerces on the remote adoption boundary too', () => { + const adopted = setupWithFetchedOwner(badSetups()[1] as ProjectHostSetup, { + kind: 'environment', + environmentId: 'env-1' + }) + expect(adopted.repoId).toBe('') + expect(adopted.path).toBe('') + expect(adopted.hostId).toBe('runtime:env-1') + }) + + it('lets the Settings projectByRepoId memo run instead of throwing on .trim()', async () => { + stubProjectsApi(badSetups()) + const ingested = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + const projection = getProjectHostSetupProjectionFromState({ + repos, + projects: [...ingested.projects], + projectHostSetups: [...ingested.setups] + }) + expect(() => + buildProjectByRepoIdLikeSettings(projection.setups, projection.projects) + ).not.toThrow() + // Why: an empty repoId is not an openable repo row, so it must not be indexed. + const byRepoId = buildProjectByRepoIdLikeSettings(projection.setups, projection.projects) + expect(byRepoId.has('')).toBe(false) + expect([...byRepoId.keys()]).toEqual(['repo-1']) + }) + + // Why: a null path takes down the whole main window rather than one error boundary, because + // the sidebar grouping index is built on every render. + it('lets the sidebar checkout identity run instead of throwing on a null path', async () => { + stubProjectsApi(badSetups()) + const ingested = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + expect(() => ingested.setups.map(checkoutIdentityLikeSidebar)).not.toThrow() + }) + + // Why: a coerced field must change one value, never which rows exist. + it('does not add or drop rows because a field was repaired', async () => { + stubProjectsApi(badSetups()) + const repaired = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + const clean = badSetups() + ;(clean[1] as { repoId: string; path: string }).repoId = '' + ;(clean[1] as { repoId: string; path: string }).path = '' + stubProjectsApi(clean) + const untouched = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + expect(repaired.setups.map((setup) => setup.id)).toEqual( + untouched.setups.map((setup) => setup.id) + ) + expect(repaired.projects.map((project) => project.id)).toEqual( + untouched.projects.map((project) => project.id) + ) + }) + + // Why: this is a zustand selector compared with Object.is, so instability is a re-render storm. + it('keeps the projection reference-stable per (repos, projects, setups) input', async () => { + stubProjectsApi(badSetups()) + const ingested = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + const args = { + repos, + projects: [...ingested.projects], + projectHostSetups: [...ingested.setups] + } + expect(getProjectHostSetupProjectionFromState(args)).toBe( + getProjectHostSetupProjectionFromState(args) + ) + }) + + // Why: nothing else in the ingest path allocates, so a clean catalog must hand back the very + // rows it was given — TerminalPane and TabBarQuickCommandsButton use them as useMemo deps. + it('preserves row identity when the catalog is already clean', async () => { + const clean = badSetups() + ;(clean[1] as { repoId: string; path: string }).repoId = '' + ;(clean[1] as { repoId: string; path: string }).path = '' + stubProjectsApi(clean) + const projection = await fetchProjectHostSetupCompatibility({ kind: 'local' }, repos) + expect(projection.setups[0]).toBe(clean[0]) + expect(projection.setups[1]).toBe(clean[1]) + }) +})