From a3fc59e8182cdea54a5ef915e6d0ca3499732793 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Tue, 22 Sep 2026 22:06:24 +0200 Subject: [PATCH] Keep a project in the sidebar when its folder name ends in a space (#5205) * fix(app): keep sidebar order keys that end in whitespace A project view key embeds the project's path, so a directory whose name ends in a space produces a key that ends in a space. The sidebar order store trimmed every key on write, so the stored key could never match the key the sidebar looks up. computeSidebarOrderUpdates saw the project as missing from the order on every pass, appended it, and the store trimmed it away again, setting a fresh array each time. The effect that applies those updates re-ran on every render until React gave up with error #185, and the app failed to load until the folder was renamed. Keep keys verbatim on the runtime setters, dropping only blank keys and duplicates. Migration of persisted state still trims, where a stray space is an artifact of an older format rather than part of the key. Fixes #4880 * test(app): give AsyncStorage a real in-memory stub instead of mocking it `docs/testing.md` bans `vi.mock`, and the test added here mocked `@react-native-async-storage/async-storage` only because the module has no Node implementation: its default export reaches for the native module, so every write from the persist middleware rejects and a test that touches a persisted store drowns in unhandled rejections. That is exactly the case `test-stubs/` exists for, reached through the resolve.alias in `vitest.config.ts` the same way react-native-unistyles, react-native-svg and expo-linking are. The stub is a real in-memory key-value store with the same contract, so a test can read back what a store wrote rather than inspect a recording double. `sidebar-view-store.test.ts` carried the same mock for the same reason and no longer needs it either. * test(app): give the AsyncStorage stub the multi-key calls the app uses `provider-snapshot-cache` declares its own `ProviderSnapshotStorage` port and is handed the real AsyncStorage, so it calls `multiGet` and `multiRemove` during reconciliation and cleanup. The stub stopped at the single-key calls, which is fine until a test reaches the provider cache and then fails with `storage.multiGet is not a function`. `multiSet` comes along with them so the three multi-key calls behave as a set rather than leaving the next caller to notice the gap the same way. * test(app): cover a project folder name ending in a space end to end Adds a browser regression for #4880: the daemon accepts a directory whose name ends in a space, and the sidebar has to render its project row instead of tearing the app down with "Maximum update depth exceeded" on every load. * test(app): move the trailing-space spec setup behind a seeded-project helper docs/testing.md asks for E2E bodies that read like plain English with the setup in helpers, and for helpers that own the project record until cleanup. The spec did its directory creation, daemon seeding, and removal inline. seedProjectNamedWithTrailingSpace owns the directory and the daemon project and returns a cleanup, the same shape seedWorkspace returns, which cannot be used here because mkdtemp appends the random suffix and so cannot produce a name that ends in a space. The body is now goto, assert the row, assert no error screen. The row is still located by its sidebar-project-row test id, which is how every other sidebar spec and the shared sidebar helpers address a project row. --------- Co-authored-by: L4XB Co-authored-by: paseo-bot[bot] --- .../sidebar-project-name-whitespace.spec.ts | 77 +++++++++++++++++++ .../src/stores/sidebar-order-store.test.ts | 76 +++++++++++++++++- .../app/src/stores/sidebar-order-store.ts | 41 +++++++--- .../app/src/stores/sidebar-view-store.test.ts | 10 +-- packages/app/test-stubs/async-storage.ts | 45 +++++++++++ packages/app/vitest.config.ts | 6 ++ 6 files changed, 233 insertions(+), 22 deletions(-) create mode 100644 packages/app/e2e/browser/sidebar-project-name-whitespace.spec.ts create mode 100644 packages/app/test-stubs/async-storage.ts diff --git a/packages/app/e2e/browser/sidebar-project-name-whitespace.spec.ts b/packages/app/e2e/browser/sidebar-project-name-whitespace.spec.ts new file mode 100644 index 0000000000..3197cd060a --- /dev/null +++ b/packages/app/e2e/browser/sidebar-project-name-whitespace.spec.ts @@ -0,0 +1,77 @@ +import { randomUUID } from "node:crypto"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import type { Page } from "@playwright/test"; +import { expect, test } from "../support/fixtures"; +import { gotoAppShell } from "../support/helpers/app"; +import { projectEquivalenceViewKey } from "../support/helpers/project-view-key"; +import { connectSeedClient } from "../support/helpers/seed-client"; + +interface SeededProject { + projectKey: string; + cleanup(): Promise; +} + +/** + * Adds a project whose directory name ends in a space, which `seedWorkspace` + * cannot produce because `mkdtemp` appends the random suffix. The project is + * owned until `cleanup`, which removes the daemon record before the directory. + */ +async function seedProjectNamedWithTrailingSpace(): Promise { + const parentDirectory = await mkdtemp(path.join(tmpdir(), "paseo-e2e-trailing-space-")); + const directoryPath = path.join(parentDirectory, `Reklamation-${randomUUID().slice(0, 8)} `); + await mkdir(directoryPath); + await writeFile(path.join(directoryPath, "README.md"), "# Trailing space\n"); + + const client = await connectSeedClient(); + const removeParentDirectory = () => rm(parentDirectory, { recursive: true, force: true }); + try { + const added = await client.addProject(directoryPath); + expect(added.error).toBeNull(); + expect(added.project?.projectRootPath).toBe(directoryPath); + const project = added.project; + if (!project?.projectKey) { + throw new Error(`The daemon added ${directoryPath} without a project key`); + } + return { + projectKey: project.projectKey, + cleanup: async () => { + await client.removeProject(project.projectId).catch(() => undefined); + await client.close().catch(() => undefined); + await removeParentDirectory().catch(() => undefined); + }, + }; + } catch (error) { + await client.close().catch(() => undefined); + await removeParentDirectory().catch(() => undefined); + throw error; + } +} + +async function expectProjectRow(page: Page, projectKey: string): Promise { + const row = page.getByTestId(`sidebar-project-row-${projectEquivalenceViewKey(projectKey)}`); + await expect(row).toBeVisible({ timeout: 60_000 }); +} + +async function expectAppShellRendered(page: Page): Promise { + await expect(page.getByText("Paseo ran into a problem.")).toHaveCount(0); +} + +// A project view key carries the project's path, so a directory whose name ends +// in a space produces a key that ends in a space. The sidebar order store used +// to trim it on write, which left the reconcile effect writing a new order on +// every render until React tore the app down with "Maximum update depth +// exceeded" on every load (#4880). +test("a project whose folder name ends in a space renders in the sidebar", async ({ page }) => { + const project = await seedProjectNamedWithTrailingSpace(); + + try { + await gotoAppShell(page); + + await expectProjectRow(page, project.projectKey); + await expectAppShellRendered(page); + } finally { + await project.cleanup(); + } +}); diff --git a/packages/app/src/stores/sidebar-order-store.test.ts b/packages/app/src/stores/sidebar-order-store.test.ts index c58931ca69..a68ca206f9 100644 --- a/packages/app/src/stores/sidebar-order-store.test.ts +++ b/packages/app/src/stores/sidebar-order-store.test.ts @@ -1,5 +1,9 @@ -import { describe, expect, it } from "vitest"; -import { migrateSidebarOrderState } from "./sidebar-order-store"; +import { beforeEach, describe, expect, it } from "vitest"; +import { + computeSidebarOrderUpdates, + type SidebarProjectEntry, +} from "@/hooks/sidebar-workspaces-view-model"; +import { migrateSidebarOrderState, useSidebarOrderStore } from "./sidebar-order-store"; describe("migrateSidebarOrderState", () => { it("prefixes legacy per-server workspace order with the source server id", () => { @@ -31,3 +35,71 @@ describe("migrateSidebarOrderState", () => { expect(migrated.pinnedWorkspaceOrder).toEqual(["host-a:one", "host-b:two"]); }); }); + +// A directory whose name ends in a space produces a view key that ends in a +// space. Trimming it on write used to make the sidebar's reconcile effect loop +// forever, crashing the app with React error #185 (see #4880). +describe("sidebar order keys that end in whitespace", () => { + const PROJECT_KEY = "host:srv-1:/home/u/Reklamation "; + + beforeEach(() => { + useSidebarOrderStore.setState({ + projectOrder: [], + pinnedWorkspaceOrder: [], + workspaceOrderByProject: {}, + }); + }); + + it("stores a project key exactly as given", () => { + useSidebarOrderStore.getState().setProjectOrder([PROJECT_KEY]); + + expect(useSidebarOrderStore.getState().projectOrder).toEqual([PROJECT_KEY]); + }); + + it("scopes a workspace order under the project key the sidebar indexes by", () => { + useSidebarOrderStore.getState().setWorkspaceOrder(PROJECT_KEY, ["srv-1:main"]); + + // use-sidebar-workspaces-list reads workspaceOrderByProject by view key + // directly, so the scope has to be stored under that same key. + expect(useSidebarOrderStore.getState().workspaceOrderByProject[PROJECT_KEY]).toEqual([ + "srv-1:main", + ]); + expect(useSidebarOrderStore.getState().getWorkspaceOrder(PROJECT_KEY)).toEqual(["srv-1:main"]); + }); + + it("settles the sidebar order reconcile instead of rewriting it forever", () => { + const projects: SidebarProjectEntry[] = [ + { + viewKey: PROJECT_KEY, + projectName: "Reklamation ", + projectKind: "non_git", + iconWorkingDir: "/home/u/Reklamation ", + hosts: [], + workspaces: [], + }, + ]; + const runPass = () => + computeSidebarOrderUpdates({ + projects, + persistedProjectOrder: useSidebarOrderStore.getState().projectOrder, + getWorkspaceOrder: (key) => useSidebarOrderStore.getState().getWorkspaceOrder(key), + }); + + // First pass adds the newly visible project. + const first = runPass(); + expect(first.projectOrder).toEqual([PROJECT_KEY]); + useSidebarOrderStore.getState().setProjectOrder(first.projectOrder ?? []); + + // Second pass must find nothing left to do; otherwise the effect that + // applies these updates re-runs on every render. + expect(runPass().projectOrder).toBeNull(); + }); + + it("still trims keys when migrating persisted state", () => { + const migrated = migrateSidebarOrderState({ + projectOrder: [" host-a:one ", "host-a:one"], + }); + + expect(migrated.projectOrder).toEqual(["host-a:one"]); + }); +}); diff --git a/packages/app/src/stores/sidebar-order-store.ts b/packages/app/src/stores/sidebar-order-store.ts index 27e7a190c8..10831480f8 100644 --- a/packages/app/src/stores/sidebar-order-store.ts +++ b/packages/app/src/stores/sidebar-order-store.ts @@ -38,6 +38,8 @@ interface SidebarWorkspaceOrderScope { projectViewKey: string; } +// Trims each key. Only for persisted state read at migration time, where a +// stray space is an artifact of an older format rather than part of the key. function normalizeKeys(keys: string[]): string[] { const seen = new Set(); const normalized: string[] = []; @@ -54,6 +56,28 @@ function normalizeKeys(keys: string[]): string[] { return normalized; } +/** + * Drops blank keys and duplicates but keeps each key exactly as given. View + * keys embed a project's path, so a directory whose name ends in a space + * produces a key that ends in a space. Trimming it stores a key that can never + * match the one the sidebar looks up, so the caller sees its key as missing, + * writes it again, and the effect that reconciles the order never settles. + */ +function dedupeKeys(keys: string[]): string[] { + const seen = new Set(); + const deduped: string[] = []; + + for (const key of keys) { + if (!key.trim() || seen.has(key)) { + continue; + } + seen.add(key); + deduped.push(key); + } + + return deduped; +} + function normalizeWorkspaceOrderByProject( workspaceOrderByProject: Record | undefined, ): Record { @@ -134,27 +158,22 @@ export const useSidebarOrderStore = create()( workspaceOrderByProject: {}, getProjectOrder: () => get().projectOrder, setProjectOrder: (keys) => { - const normalized = normalizeKeys(keys); - set({ projectOrder: normalized }); + set({ projectOrder: dedupeKeys(keys) }); }, getPinnedWorkspaceOrder: () => get().pinnedWorkspaceOrder, setPinnedWorkspaceOrder: (keys) => { - const normalized = normalizeKeys(keys); - set({ pinnedWorkspaceOrder: normalized }); + set({ pinnedWorkspaceOrder: dedupeKeys(keys) }); }, getWorkspaceOrder: (projectViewKey) => { - const scope = projectViewKey.trim(); - if (!scope) return []; - return get().workspaceOrderByProject[scope] ?? []; + if (!projectViewKey.trim()) return []; + return get().workspaceOrderByProject[projectViewKey] ?? []; }, setWorkspaceOrder: (projectViewKey, keys) => { - const scope = projectViewKey.trim(); - if (!scope) return; - const normalized = normalizeKeys(keys); + if (!projectViewKey.trim()) return; set((state) => ({ workspaceOrderByProject: { ...state.workspaceOrderByProject, - [scope]: normalized, + [projectViewKey]: dedupeKeys(keys), }, })); }, diff --git a/packages/app/src/stores/sidebar-view-store.test.ts b/packages/app/src/stores/sidebar-view-store.test.ts index b9df715fa2..902294dd2d 100644 --- a/packages/app/src/stores/sidebar-view-store.test.ts +++ b/packages/app/src/stores/sidebar-view-store.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it } from "vitest"; import type { StateStorage } from "zustand/middleware"; import { createSidebarViewStorage, @@ -8,14 +8,6 @@ import { useSidebarViewStore, } from "./sidebar-view-store"; -vi.mock("@react-native-async-storage/async-storage", () => ({ - default: { - getItem: vi.fn().mockResolvedValue(null), - setItem: vi.fn().mockResolvedValue(undefined), - removeItem: vi.fn().mockResolvedValue(undefined), - }, -})); - interface MemoryStorage extends StateStorage> { reads: string[]; } diff --git a/packages/app/test-stubs/async-storage.ts b/packages/app/test-stubs/async-storage.ts new file mode 100644 index 0000000000..68cc5dd114 --- /dev/null +++ b/packages/app/test-stubs/async-storage.ts @@ -0,0 +1,45 @@ +/** + * AsyncStorage has no implementation under Node: its default export reaches for + * the native module and every call rejects, so any test that touches a + * persisted store emits unhandled rejections from the persist middleware. + * + * This is a real in-memory key-value store with the same contract, not a + * recording double, so a test can read back what a store wrote. + */ +const entries = new Map(); + +export function clearAsyncStorageStub(): void { + entries.clear(); +} + +const AsyncStorageStub = { + getItem: (key: string): Promise => Promise.resolve(entries.get(key) ?? null), + setItem: (key: string, value: string): Promise => { + entries.set(key, value); + return Promise.resolve(); + }, + removeItem: (key: string): Promise => { + entries.delete(key); + return Promise.resolve(); + }, + getAllKeys: (): Promise => Promise.resolve([...entries.keys()]), + // `provider-snapshot-cache` declares its own `ProviderSnapshotStorage` port and + // hands it the real AsyncStorage, so a stub that stops at the single-key calls + // fails any test that reaches the provider cache with `multiGet is not a function`. + multiGet: (keys: readonly string[]): Promise> => + Promise.resolve(keys.map((key) => [key, entries.get(key) ?? null] as [string, string | null])), + multiSet: (pairs: ReadonlyArray): Promise => { + for (const [key, value] of pairs) entries.set(key, value); + return Promise.resolve(); + }, + multiRemove: (keys: readonly string[]): Promise => { + for (const key of keys) entries.delete(key); + return Promise.resolve(); + }, + clear: (): Promise => { + entries.clear(); + return Promise.resolve(); + }, +}; + +export default AsyncStorageStub; diff --git a/packages/app/vitest.config.ts b/packages/app/vitest.config.ts index 5e560be880..521fb6dbe0 100644 --- a/packages/app/vitest.config.ts +++ b/packages/app/vitest.config.ts @@ -185,6 +185,12 @@ export default defineConfig({ find: /^expo-linking$/, replacement: path.resolve(__dirname, "test-stubs/expo-linking.ts"), }, + // No Node implementation: every call on the real module rejects, so a test + // touching a persisted store drowns in unhandled rejections. + { + find: /^@react-native-async-storage\/async-storage$/, + replacement: path.resolve(__dirname, "test-stubs/async-storage.ts"), + }, { find: /^lucide-react-native$/, replacement: path.resolve(__dirname, "test-stubs/lucide-react-native.ts"),