From de470aa019db79d79ec9dfdb0a78eca1c54864dd Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 00:21:52 +0200 Subject: [PATCH 01/25] test: move the cli end-to-end suite into e2e/cli --- e2e/{ => cli}/package.json | 0 e2e/{ => cli}/src/cli.ts | 4 ++-- e2e/{ => cli}/tests/fixture.spec.ts | 0 e2e/{ => cli}/tests/mcp-oauth.spec.ts | 0 e2e/{ => cli}/tests/mcp-parameters.spec.ts | 0 e2e/{ => cli}/tests/mcp.spec.ts | 0 e2e/{ => cli}/tests/repair.spec.ts | 0 e2e/{ => cli}/tests/requires.spec.ts | 0 e2e/{ => cli}/tests/skills.spec.ts | 0 e2e/{ => cli}/tsconfig.json | 0 jest.config.cjs | 6 +++--- package.json | 3 ++- 12 files changed, 7 insertions(+), 6 deletions(-) rename e2e/{ => cli}/package.json (100%) rename e2e/{ => cli}/src/cli.ts (97%) rename e2e/{ => cli}/tests/fixture.spec.ts (100%) rename e2e/{ => cli}/tests/mcp-oauth.spec.ts (100%) rename e2e/{ => cli}/tests/mcp-parameters.spec.ts (100%) rename e2e/{ => cli}/tests/mcp.spec.ts (100%) rename e2e/{ => cli}/tests/repair.spec.ts (100%) rename e2e/{ => cli}/tests/requires.spec.ts (100%) rename e2e/{ => cli}/tests/skills.spec.ts (100%) rename e2e/{ => cli}/tsconfig.json (100%) diff --git a/e2e/package.json b/e2e/cli/package.json similarity index 100% rename from e2e/package.json rename to e2e/cli/package.json diff --git a/e2e/src/cli.ts b/e2e/cli/src/cli.ts similarity index 97% rename from e2e/src/cli.ts rename to e2e/cli/src/cli.ts index ab7723c2..812ea93f 100644 --- a/e2e/src/cli.ts +++ b/e2e/cli/src/cli.ts @@ -20,8 +20,8 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from 'node:f import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; -/** Repository root, from this file's location (e2e/src -> ../..). */ -export const REPO_ROOT = resolve(__dirname, '..', '..'); +/** Repository root, from this file's location (e2e/cli/src -> ../../..). */ +export const REPO_ROOT = resolve(__dirname, '..', '..', '..'); /** The fixture submodule's working tree. */ export const FIXTURE_DIR = join(REPO_ROOT, 'examples', 'test-repo'); diff --git a/e2e/tests/fixture.spec.ts b/e2e/cli/tests/fixture.spec.ts similarity index 100% rename from e2e/tests/fixture.spec.ts rename to e2e/cli/tests/fixture.spec.ts diff --git a/e2e/tests/mcp-oauth.spec.ts b/e2e/cli/tests/mcp-oauth.spec.ts similarity index 100% rename from e2e/tests/mcp-oauth.spec.ts rename to e2e/cli/tests/mcp-oauth.spec.ts diff --git a/e2e/tests/mcp-parameters.spec.ts b/e2e/cli/tests/mcp-parameters.spec.ts similarity index 100% rename from e2e/tests/mcp-parameters.spec.ts rename to e2e/cli/tests/mcp-parameters.spec.ts diff --git a/e2e/tests/mcp.spec.ts b/e2e/cli/tests/mcp.spec.ts similarity index 100% rename from e2e/tests/mcp.spec.ts rename to e2e/cli/tests/mcp.spec.ts diff --git a/e2e/tests/repair.spec.ts b/e2e/cli/tests/repair.spec.ts similarity index 100% rename from e2e/tests/repair.spec.ts rename to e2e/cli/tests/repair.spec.ts diff --git a/e2e/tests/requires.spec.ts b/e2e/cli/tests/requires.spec.ts similarity index 100% rename from e2e/tests/requires.spec.ts rename to e2e/cli/tests/requires.spec.ts diff --git a/e2e/tests/skills.spec.ts b/e2e/cli/tests/skills.spec.ts similarity index 100% rename from e2e/tests/skills.spec.ts rename to e2e/cli/tests/skills.spec.ts diff --git a/e2e/tsconfig.json b/e2e/cli/tsconfig.json similarity index 100% rename from e2e/tsconfig.json rename to e2e/cli/tsconfig.json diff --git a/jest.config.cjs b/jest.config.cjs index d76edd6f..42392be6 100644 --- a/jest.config.cjs +++ b/jest.config.cjs @@ -7,12 +7,12 @@ // // This file is `.cjs` on purpose: the root package.json sets `"type": "module"`, // so a `.js` config would be ESM and Jest's config loader plus ts-jest's -// CommonJS transform are simplest kept out of ESM entirely. See e2e/tsconfig.json. +// CommonJS transform are simplest kept out of ESM entirely. See e2e/cli/tsconfig.json. /** @type {import('jest').Config} */ module.exports = { rootDir: __dirname, testEnvironment: 'node', - testMatch: ['/e2e/tests/**/*.spec.ts'], + testMatch: ['/e2e/cli/tests/**/*.spec.ts'], // Jest's module map otherwise walks the whole tree and trips over duplicate // package.json names: `.claude/worktrees/*` holds full checkouts of this same // repository (git worktrees for parallel branches), and the fixture submodule @@ -21,7 +21,7 @@ module.exports = { haste: { retainAllFiles: false }, watchPathIgnorePatterns: ['/.claude/', '/target/'], transform: { - '^.+\\.ts$': ['ts-jest', { tsconfig: '/e2e/tsconfig.json' }], + '^.+\\.ts$': ['ts-jest', { tsconfig: '/e2e/cli/tsconfig.json' }], }, // Each spec adds a repository, installs skills, and shells out to git; the // default 5s is far too tight for real process work on a cold cache. diff --git a/package.json b/package.json index 4048f95d..505d3f03 100644 --- a/package.json +++ b/package.json @@ -27,7 +27,8 @@ "test": "vitest run", "test:watch": "vitest", "test:cov": "vitest run --coverage", - "test:e2e": "node scripts/e2e-prepare.mjs && jest --config jest.config.cjs", + "test:e2e": "pnpm test:e2e:cli", + "test:e2e:cli": "node scripts/e2e-prepare.mjs && jest --config jest.config.cjs", "lint": "eslint .", "lint:fix": "eslint . --fix", "format": "prettier --write .", From 46d6160b7de16ca64e5669555ea49397a377b5af Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 00:26:52 +0200 Subject: [PATCH 02/25] docs: fix stale e2e paths left by the cli suite move --- .agents/skills/check-fixture-repo/SKILL.md | 16 ++++++++-------- AGENTS.md | 2 +- docs/development/development.md | 2 +- e2e/cli/tests/mcp-parameters.spec.ts | 2 +- e2e/cli/tsconfig.json | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.agents/skills/check-fixture-repo/SKILL.md b/.agents/skills/check-fixture-repo/SKILL.md index 5a88ea49..bd2da987 100644 --- a/.agents/skills/check-fixture-repo/SKILL.md +++ b/.agents/skills/check-fixture-repo/SKILL.md @@ -24,7 +24,7 @@ its own README explains what each fixture drives. ## Isolation -The suite never touches the developer's machine state. `e2e/src/cli.ts` is the +The suite never touches the developer's machine state. `e2e/cli/src/cli.ts` is the only way a spec can invoke the CLI, and it always sets **both**: - `XDG_CONFIG_HOME`, which relocates `state.json` and `config.yaml` @@ -48,7 +48,7 @@ pnpm test:e2e That is the whole check. The script behind it (`scripts/e2e-prepare.mjs`) initializes the fixture submodule, force-pulls it to the tip of its branch, and builds `target/debug/skillkeeper`; Jest then runs the -specs in `e2e/tests/`. +specs in `e2e/cli/tests/`. Set `SKILLKEEPER_E2E_PIN_FIXTURE=1` to run against the fixture commit this repository pins instead of pulling. CI does that for reproducibility; locally @@ -61,11 +61,11 @@ to look: | spec | covers | a failure means | |---|---|---| -| `e2e/tests/fixture.spec.ts` | the submodule is checked out, ASCII-only, and still has the manifests and file modes the rest of the suite assumes | the **fixture** drifted | -| `e2e/tests/skills.spec.ts` | resolution schemes, `.skid.yml` identity, nested body paths, selective `+x`, guidance precedence, hook merge and consent, the delimited-text region, and both silent-failure modes of the resolver | the **product** changed | -| `e2e/tests/mcp.spec.ts` | preset discovery including the group-scoped file, parameter substitution, both ledger files, the `.gitignore` guard for the secrets file, rules rendering, instance-name allocation, the Codex stdio-only skip, and removal | the **product** changed | -| `e2e/tests/repair.spec.ts` | `verify` -> `repair` -> `verify`, directory pruning, the bounds that keep repair inside the repaired skill, and uninstall reversing hooks and guidance | the **product** changed | -| `e2e/tests/requires.spec.ts` | skill dependencies: every `repo lint` code the `requires` group triggers, the single-document `--json` form, both target-misuse exits, the transitive install closure, and the uninstall breakage report | the **product** changed | +| `e2e/cli/tests/fixture.spec.ts` | the submodule is checked out, ASCII-only, and still has the manifests and file modes the rest of the suite assumes | the **fixture** drifted | +| `e2e/cli/tests/skills.spec.ts` | resolution schemes, `.skid.yml` identity, nested body paths, selective `+x`, guidance precedence, hook merge and consent, the delimited-text region, and both silent-failure modes of the resolver | the **product** changed | +| `e2e/cli/tests/mcp.spec.ts` | preset discovery including the group-scoped file, parameter substitution, both ledger files, the `.gitignore` guard for the secrets file, rules rendering, instance-name allocation, the Codex stdio-only skip, and removal | the **product** changed | +| `e2e/cli/tests/repair.spec.ts` | `verify` -> `repair` -> `verify`, directory pruning, the bounds that keep repair inside the repaired skill, and uninstall reversing hooks and guidance | the **product** changed | +| `e2e/cli/tests/requires.spec.ts` | skill dependencies: every `repo lint` code the `requires` group triggers, the single-document `--json` form, both target-misuse exits, the transitive install closure, and the uninstall breakage report | the **product** changed | If `fixture.spec.ts` fails, fix or re-pin the fixture. If it passes and another spec fails, the CLI's behaviour moved and the fixture is telling you so. @@ -90,7 +90,7 @@ git -C examples/test-repo status --porcelain # expect clean ``` Every spec runs the CLI with throwaway `HOME` and `XDG_CONFIG_HOME` -(`e2e/src/cli.ts`), so a dirty tree here means a test wrote somewhere it should +(`e2e/cli/src/cli.ts`), so a dirty tree here means a test wrote somewhere it should not have -- a blocker, and a harness defect rather than a product one. Note that a force-pull may legitimately leave the submodule pointer moved; that diff --git a/AGENTS.md b/AGENTS.md index 031b0e3e..9e935df9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -94,7 +94,7 @@ pnpm test:e2e # Jest, drives the built CLI against examples/test-repo Separate from the gate above because it needs the fixture submodule and a `cargo build`. It covers what the in-memory fakes cannot: the real binary against a real working tree. Run it after touching resolution, install, hooks, guidance, -or MCP. Specs live in `e2e/tests/`, the harness in `e2e/src/cli.ts`; the runner is +or MCP. Specs live in `e2e/cli/tests/`, the harness in `e2e/cli/src/cli.ts`; the runner is Jest (not Vitest) and the suite is scoped to CommonJS -- see [docs/development/development.md](./docs/development/development.md#end-to-end-tests). diff --git a/docs/development/development.md b/docs/development/development.md index 570e5bf2..7a766b66 100644 --- a/docs/development/development.md +++ b/docs/development/development.md @@ -158,7 +158,7 @@ Two things about the design are worth knowing before adding a spec: overlap: Vitest runs pure logic in-process under the coverage gate, Jest drives a subprocess against the filesystem. Jest's config is `jest.config.cjs`, and the suite is deliberately CommonJS so no `--experimental-vm-modules` is needed. -- **Isolation belongs to the harness.** `Sandbox` (in `e2e/src/cli.ts`) always +- **Isolation belongs to the harness.** `Sandbox` (in `e2e/cli/src/cli.ts`) always sets throwaway `HOME` *and* `XDG_CONFIG_HOME`. The first relocates the agents' global roots (a global-scope Codex MCP install writes to `~/.codex/config.toml`, a project-scoped one to `/.codex/config.toml`, and diff --git a/e2e/cli/tests/mcp-parameters.spec.ts b/e2e/cli/tests/mcp-parameters.spec.ts index 1254c0d6..f4445b7e 100644 --- a/e2e/cli/tests/mcp-parameters.spec.ts +++ b/e2e/cli/tests/mcp-parameters.spec.ts @@ -1,7 +1,7 @@ /** * MCP descriptions and options end to end: link rendering, description * truncation, option-value validation on install, and every mcp lint - * warning. Model of `e2e/tests/mcp.spec.ts`; reuses the same fixture and + * warning. Model of `e2e/cli/tests/mcp.spec.ts`; reuses the same fixture and * harness rather than building a second one. * * The fixtures this exercises are `docs-linked` (a linked description plus a diff --git a/e2e/cli/tsconfig.json b/e2e/cli/tsconfig.json index d0b05cc0..ed89f81e 100644 --- a/e2e/cli/tsconfig.json +++ b/e2e/cli/tsconfig.json @@ -10,7 +10,7 @@ "compilerOptions": { "target": "ES2023", "lib": ["ES2023"], - // node16 rather than the deprecated node10: e2e/package.json declares this + // node16 rather than the deprecated node10: e2e/cli/package.json declares this // directory CommonJS, so node16 emits require() while staying a current, // non-deprecated setting under TypeScript 6. "module": "node16", From 4dc5b1c3cbcb838a709f55cb35266a0303b6092c Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 00:48:32 +0200 Subject: [PATCH 03/25] test: boot the renderer under a scripted backend --- .gitignore | 5 ++ apps/desktop/src/renderer/app/App.tsx | 1 + e2e/desktop/harness/commands.ts | 123 ++++++++++++++++++++++++++ e2e/desktop/harness/installHarness.ts | 99 +++++++++++++++++++++ e2e/desktop/harness/scenario.ts | 85 ++++++++++++++++++ e2e/desktop/playwright.config.ts | 34 +++++++ e2e/desktop/tests/boot.spec.ts | 14 +++ e2e/desktop/tsconfig.json | 36 ++++++++ jest.config.cjs | 8 +- package.json | 1 + pnpm-lock.yaml | 28 ++++++ 11 files changed, 431 insertions(+), 3 deletions(-) create mode 100644 e2e/desktop/harness/commands.ts create mode 100644 e2e/desktop/harness/installHarness.ts create mode 100644 e2e/desktop/harness/scenario.ts create mode 100644 e2e/desktop/playwright.config.ts create mode 100644 e2e/desktop/tests/boot.spec.ts create mode 100644 e2e/desktop/tsconfig.json diff --git a/.gitignore b/.gitignore index 12d327b4..7b6eceb0 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,11 @@ Thumbs.db # Storybook apps/desktop/storybook-static/ +# Playwright +/test-results/ +/playwright-report/ +/blob-report/ + # Documentation site (mkdocs + uv): uv caches the tools globally, but keep any # local Python virtualenv and the rendered static site out of git. .venv/ diff --git a/apps/desktop/src/renderer/app/App.tsx b/apps/desktop/src/renderer/app/App.tsx index c9134725..fba3c605 100644 --- a/apps/desktop/src/renderer/app/App.tsx +++ b/apps/desktop/src/renderer/app/App.tsx @@ -292,6 +292,7 @@ export function App() {
diff --git a/e2e/desktop/harness/commands.ts b/e2e/desktop/harness/commands.ts new file mode 100644 index 00000000..6eb8a627 --- /dev/null +++ b/e2e/desktop/harness/commands.ts @@ -0,0 +1,123 @@ +import type { Scenario } from './scenario.js'; + +/** + * Prefix on the error thrown by the harness's mocked `invoke` handler for a + * command with no entry in the table below. Task 9 asserts a rejection + * message starts with this string, so it stays a distinct, greppable + * constant rather than an inline literal. + * + * Throwing (rather than resolving `undefined`, or logging and moving on) is + * the point: a renamed or newly added backend command must surface here, as + * a named failure, not three steps later as a confusing assertion mismatch. + */ +export const UNKNOWN_COMMAND_PREFIX = 'e2e-harness: unmocked command '; + +/** + * Default answers keyed by the backend command name as written in + * `apps/desktop/src/renderer/services/bridge/client.ts`. A scenario's + * `responses` are merged over this table (see `Scenario` in `scenario.ts`); + * a later task may make one of those override values a function of the + * invoke arguments instead of plain data. + * + * STARTUP COMMAND LIST (Task 2, Step 6): discovered by running + * `boot.spec.ts` against an empty table and `defaultScenario()` (macOS, + * onboarding already completed, empty repositories/projects/installs, + * update mode 'manual'), reading each `e2e-harness: unmocked command ` + * page error, adding a minimal valid answer, and repeating. + * + * Only three commands turned out to be load-bearing for the assertions in + * `boot.spec.ts` (`[data-testid="app-shell"]` visible, zero page errors) -- + * everything else `loadAll` awaits is wrapped in try/catch, so an unmocked + * response there is a caught rejection (`store.error` gets set, a background + * task's status flips to 'error'), never an uncaught page error: + * + * 1. platform -- main.tsx's `bridgeClient.init()`, awaited + * before the app mounts at all; index.html's + * preloader stays up until this settles. Left + * unmocked, `bridgeClient.platform` stays '' + * (its default), `hostPlatform('')` resolves to + * 'linux' rather than 'mac', and WindowChrome + * then calls `window_is_maximized` too -- + * chasing that command is a trap; fix `platform` + * and it disappears on its own. + * 2. onboarding_menu_sync -- App's "keep the native menu in sync" effect + * (`bridgeClient.onboardingMenuSync`) calls + * `invoke` with a bare `void`, no .catch -- an + * unmocked rejection is a genuine unhandled + * promise rejection, unconditionally, on every + * boot. + * 3. terminal_resize -- likewise a bare `void invoke(...)` with no + * .catch, fired by the always-mounted terminal + * view's initial fit/resize, independent of + * whether the terminal panel is open. + * + * `boot.spec.ts` would pass with only those three mocked -- `loadAll`'s + * Promise.all would simply reject and `store.error` would be set. This table + * mocks the rest of `loadAll`'s round trip anyway, because a boot test that + * tolerates the ENTIRE startup data fetch failing is not meaningfully + * proving "a scripted backend answers" (this suite's stated purpose), and + * every later task built on `defaultScenario()` wants a working store, not + * one parked in its error state: + * + * - config_get -- store.loadAll. + * - onboarding_get -- store.loadAll -> loadOnboarding. + * - repositories_list -- store.loadAll. + * - skills_reconcile -- store.loadAll (reconciles the install ledger + * against disk; NOT `skills_list`, which no + * startup path calls). + * - skills_available -- store.loadAll. + * - projects_list -- store.loadAll. + * - mcp_reconcile -- store.loadAll (NOT `mcp_installs`, same + * reason as `skills_reconcile` above). + * - get_app_version -- StatusBar's `useAppVersion`, mounted + * unconditionally alongside the shell (its own + * `.then(ok, () => undefined)` already tolerates + * a rejection, so this one was never required + * either -- included for the same reason as the + * `loadAll` set above). + * + * Commands that need no entry at all, and why: + * - `window_is_maximized` / `window:maximizeChanged` (WindowChrome): only + * called when `hostPlatform(bridgeClient.platform) !== 'mac'`; + * `defaultScenario` reports `platform: 'darwin'`, so WindowChrome renders + * nothing and never calls either. + * - `app_update_check` (useAppUpdateSchedule's one-time startup check): + * DOES fire once loading settles, but `store.runAppUpdateCheck` wraps the + * call in try/catch, same as the `loadAll` set above. + * - Every repository/project/MCP mutation and every `plugin:event|*` + * listener registration: `mockIPC` is called with + * `{ shouldMockEvents: true }` (see `installHarness.ts`), so `listen()` + * calls (onConfigChanged, onSshUnlockResolved, the app-update + * subscriptions, ...) are answered by `@tauri-apps/api/mocks`'s own + * internal event registry and never reach this table at all. + */ +export function defaultResponses(scenario: Scenario): Record { + return { + platform: scenario.platform, + onboarding_menu_sync: null, + terminal_resize: null, + config_get: { + config: scenario.config, + validity: { + general: 'valid', + updates: 'valid', + agents: 'valid', + executables: 'valid', + security: 'valid', + notifications: 'valid', + repositories: 'valid', + projects: 'valid', + mcp: 'valid', + }, + warnings: [], + }, + onboarding_get: scenario.onboarding, + repositories_list: scenario.repositories, + skills_reconcile: scenario.installs, + skills_available: { skills: [], warnings: [] }, + projects_list: scenario.projects, + mcp_reconcile: scenario.mcpInstalls, + get_app_version: '0.0.0-e2e', + ...scenario.responses, + }; +} diff --git a/e2e/desktop/harness/installHarness.ts b/e2e/desktop/harness/installHarness.ts new file mode 100644 index 00000000..bea15278 --- /dev/null +++ b/e2e/desktop/harness/installHarness.ts @@ -0,0 +1,99 @@ +import { createRequire } from 'node:module'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import type { Page } from '@playwright/test'; +import type { Scenario } from './scenario.js'; +import { UNKNOWN_COMMAND_PREFIX, defaultResponses } from './commands.js'; + +/** + * `@tauri-apps/api/mocks` (`mockIPC`, `mockWindows`) cannot be `import`ed from + * a `page.addInitScript` payload: browsers only resolve bare specifiers + * (`import('@tauri-apps/api/mocks')`) through an import map, and `vite + * preview` serves the built production bundle as plain static files with none + * configured -- that module was never part of the app's own dependency graph + * in the first place, since the app itself never imports its own test mocks. + * Bundling the harness with Vite (the brief's other suggested option) would + * need its own build step just for this. + * + * Instead, read the installed package's CommonJS build (`require.resolve` + * follows the package's "require" export condition to `mocks.cjs`, a script + * of plain function declarations ending in `exports.mockIPC = mockIPC;` etc.) + * once, here in Node, and hand its text to the page as init-script *data* + * (not code) via `installHarness`'s `addInitScript` argument. The page-side + * function runs that text through `new Function('exports', source)`, + * supplying the `exports` object the script assigns onto, entirely at + * page-runtime, with no module resolution involved at all. + * + * `@tauri-apps/api` is a dependency of `@skillkeeper/desktop`, not of the + * repository root (pnpm keeps this suite, like `e2e/cli`, outside the + * workspace), so it is resolved through a `require` rooted at the desktop + * package rather than at this file. + */ +const requireFromDesktop = createRequire(fileURLToPath(new URL('../../../apps/desktop/package.json', import.meta.url))); +const TAURI_MOCKS_SOURCE = readFileSync(requireFromDesktop.resolve('@tauri-apps/api/mocks'), 'utf8'); + +/** Data passed into the page; must stay JSON-serializable end to end (see + * `Scenario`'s doc comment on why `responses` cannot carry a function yet). */ +interface HarnessInit { + readonly scenario: Scenario; + readonly responses: Record; + readonly unknownCommandPrefix: string; + readonly mocksSource: string; +} + +/** + * Installs the scripted backend before any application module runs. + * + * `mockIPC` writes `window.__TAURI_INTERNALS__`, and the renderer's `invoke` + * reads `window.__TAURI_INTERNALS__.invoke(...)` at call time, not at module + * load -- so this beats every lazily imported route exactly as well as it + * beats an eagerly imported one. Call this before `page.goto`. + */ +export async function installHarness(page: Page, scenario: Scenario): Promise { + const init: HarnessInit = { + scenario, + responses: defaultResponses(scenario), + unknownCommandPrefix: UNKNOWN_COMMAND_PREFIX, + mocksSource: TAURI_MOCKS_SOURCE, + }; + + await page.addInitScript((arg: HarnessInit) => { + // See the module doc comment: this is the only way to get `mocks.cjs`'s + // exports out of its source text without a real module system, in a page + // served as static files with no import map. + const modFactory = new Function('exports', `${arg.mocksSource}\nreturn exports;`) as ( + exportsObj: Record, + ) => { + mockIPC: (cb: (cmd: string, args: unknown) => unknown, options?: { shouldMockEvents?: boolean }) => void; + mockWindows: (current: string, ...rest: string[]) => void; + }; + const { mockIPC, mockWindows } = modFactory({}); + + (window as unknown as Record).__SKK_E2E_CALLS__ = []; + (window as unknown as Record).__SKK_E2E_CLIPBOARD__ = []; + const label = arg.scenario.windowLabel || 'main'; + (window as unknown as Record).__SKK_E2E_WINDOW_LABEL__ = label; + + // main.tsx renders SshUnlockApp instead of App when the current window's + // label is 'ssh-unlock' -- see Scenario's doc comment on windowLabel. + mockWindows(label); + + // `shouldMockEvents: true` routes every `plugin:event|listen` / + // `plugin:event|emit` / `plugin:event|unlisten` invoke to mockIPC's own + // in-page listener registry instead of to the callback below. Without it, + // the app's first `listen()` call (useConfigWatch's onConfigChanged fires + // during App's very first effect pass) would throw "unmocked command + // plugin:event|listen" before the command table below ever got a look in. + mockIPC( + (cmd, args) => { + const calls = (window as unknown as Record).__SKK_E2E_CALLS__ as unknown[]; + calls.push({ cmd, args }); + if (!Object.prototype.hasOwnProperty.call(arg.responses, cmd)) { + throw new Error(`${arg.unknownCommandPrefix}${cmd}`); + } + return arg.responses[cmd]; + }, + { shouldMockEvents: true }, + ); + }, init); +} diff --git a/e2e/desktop/harness/scenario.ts b/e2e/desktop/harness/scenario.ts new file mode 100644 index 00000000..5ee5f5da --- /dev/null +++ b/e2e/desktop/harness/scenario.ts @@ -0,0 +1,85 @@ +/** + * The scripted backend's fixture data for one Playwright run. + * + * A `Scenario` is plain, JSON-serializable data: `installHarness` carries it + * across into the page via `page.addInitScript`'s argument, which uses + * Playwright's structured-clone-style serialization -- functions and other + * non-serializable values do not survive that trip. `responses` is the one + * deliberate escape hatch: `commands.ts`'s `defaultResponses` merges it over + * the default command table, so a scenario built entirely from plain data (as + * `defaultScenario` is) never runs into the restriction; a later task that + * needs a per-call computed answer resolves it a different way (see + * `installHarness.ts`'s notes on that boundary). + * + * Field shapes loosely mirror the real backend's without importing from + * `apps/desktop` -- this suite stays a self-contained package, the same way + * `e2e/cli` does not depend on the CLI crate's Rust types either: + * config -> LoadConfigResult['config'] (SkillKeeperConfig) + * onboarding -> OnboardingState { version, completed, step } + * repositories -> Repository[] + * projects -> Project[] + * installs -> InstallManifest[] + * mcpInstalls -> McpInstall[] + * + * This is the minimal shape Task 2's command table needs (see + * `commands.ts`'s `defaultResponses`). A later task extends this file to add + * whatever a flow test needs; it will not recreate it. + */ +export interface Scenario { + /** `process.platform` as the Rust backend reports it: 'darwin' | 'win32' | 'linux'. */ + readonly platform: string; + /** The mocked Tauri window label. `main.tsx` mounts an entirely different + * app (`SshUnlockApp`) for the 'ssh-unlock' label, so this is load-bearing, + * not decoration -- default 'main'. */ + readonly windowLabel: string; + readonly config: Record; + readonly onboarding: Record; + readonly repositories: readonly unknown[]; + readonly projects: readonly unknown[]; + readonly installs: readonly unknown[]; + readonly mcpInstalls: readonly unknown[]; + /** + * Per-command overrides, merged over `commands.ts`'s defaults. A value here + * is plain data for now (see the file-level note on why a function value + * cannot cross into the page yet). + */ + readonly responses: Record; + /** Named canned event payloads a later task's harness can emit on demand + * (e.g. to drive `onConfigChanged`/`onTerminalData` subscribers). Unused + * until that task wires an emit path through the mocked event plugin. */ + readonly events: Record; +} + +/** + * A scenario with a clean, empty-but-valid backend: no repositories, no + * projects, no installs, onboarding already completed (so the guided tour + * never opens unprompted), update checks on 'manual' (so nothing schedules a + * startup sweep), and `platform: 'darwin'` -- which resolves to the 'mac' + * chrome variant (see `hostPlatform.ts`), so `WindowChrome` renders nothing + * and the `window_is_maximized`/`window:maximizeChanged` round trip it would + * otherwise drive never comes into play. + */ +export function defaultScenario(): Scenario { + return { + platform: 'darwin', + windowLabel: 'main', + config: { + general: { language: 'en', theme: 'system', animations: 'normal' }, + updates: { mode: 'manual', intervalMinutes: 720, checkOnStartup: false }, + agents: { enabled: ['claude', 'codex', 'copilot', 'cursor', 'opencode'], overrides: {} }, + executables: { globs: [] }, + security: { hookConsentPolicy: 'always-ask' }, + notifications: { enabled: true }, + repositories: { gitPath: 'git' }, + projects: { checkIntervalMinutes: 1 }, + mcp: { servers: [] }, + }, + onboarding: { version: 1, completed: true, step: 'done' }, + repositories: [], + projects: [], + installs: [], + mcpInstalls: [], + responses: {}, + events: {}, + }; +} diff --git a/e2e/desktop/playwright.config.ts b/e2e/desktop/playwright.config.ts new file mode 100644 index 00000000..d9f96672 --- /dev/null +++ b/e2e/desktop/playwright.config.ts @@ -0,0 +1,34 @@ +import { defineConfig, devices } from '@playwright/test'; + +// The suite drives the PRODUCTION bundle, not the dev server: `vite preview` +// serves what `vite build` wrote to dist-tauri. That removes module +// transformation, the hot-module transport, and the dev/prod split from the +// failure surface, and makes every run prove the bundle boots. Run +// `pnpm --filter @skillkeeper/desktop run frontend:build` at least once +// before this config's webServer has anything to serve. +export default defineConfig({ + testDir: './tests', + // A retry turns a flake into a slow pass and throws away the signal. If a + // spec is unstable, the spec or the harness is wrong. + retries: 0, + fullyParallel: true, + reporter: process.env.CI ? [['github'], ['html', { open: 'never' }]] : [['list']], + use: { + baseURL: 'http://localhost:4173', + // Layout-dependent visibility must not depend on the runner's default. + viewport: { width: 1440, height: 900 }, + // The renderer animates with `motion`; a moving target is the classic + // reason a click lands nowhere. Task 3 also zeroes durations in CSS. + reducedMotion: 'reduce', + locale: 'en-US', + trace: 'on-first-retry', + screenshot: 'only-on-failure', + }, + projects: [{ name: 'chromium', use: { ...devices['Desktop Chrome'] } }], + webServer: { + command: 'pnpm --filter @skillkeeper/desktop exec vite preview --port 4173 --strictPort', + url: 'http://localhost:4173', + reuseExistingServer: !process.env.CI, + timeout: 60_000, + }, +}); diff --git a/e2e/desktop/tests/boot.spec.ts b/e2e/desktop/tests/boot.spec.ts new file mode 100644 index 00000000..7cae9276 --- /dev/null +++ b/e2e/desktop/tests/boot.spec.ts @@ -0,0 +1,14 @@ +import { test, expect } from '@playwright/test'; +import { installHarness } from '../harness/installHarness.js'; +import { defaultScenario } from '../harness/scenario.js'; + +test('the application mounts against the scripted backend', async ({ page }) => { + const failures: string[] = []; + page.on('pageerror', (e) => failures.push(e.message)); + await installHarness(page, defaultScenario()); + await page.goto('/'); + // The preloader in index.html stays up until bridgeClient.init() settles, so + // seeing the shell means the harness answered the startup round-trip. + await expect(page.getByTestId('app-shell')).toBeVisible(); + expect(failures, failures.join('\n')).toEqual([]); +}); diff --git a/e2e/desktop/tsconfig.json b/e2e/desktop/tsconfig.json new file mode 100644 index 00000000..d3a83161 --- /dev/null +++ b/e2e/desktop/tsconfig.json @@ -0,0 +1,36 @@ +{ + // Like e2e/cli/tsconfig.json, this suite is deliberately its own TypeScript + // scope: it is not a pnpm workspace package (pnpm-workspace.yaml globs only + // packages/* and apps/*), so it does not extend tsconfig.base.json and is + // not part of `pnpm typecheck` (which is per-workspace-package). Playwright + // Test transpiles these files with esbuild at run time and does not type-check + // them; this file exists for editor support and for anyone who runs `tsc + // --noEmit -p e2e/desktop/tsconfig.json` by hand. + // + // Unlike e2e/cli (Node-only, CommonJS), this suite needs DOM types for the + // `page.addInitScript` payloads in harness/installHarness.ts, which run in + // the browser, and stays ESM (the repository root is "type": "module" and + // this directory has no package.json of its own to override that), which is + // what lets installHarness.ts use `import.meta.url`. + "compilerOptions": { + "target": "ES2023", + "lib": ["ES2023", "DOM"], + "module": "NodeNext", + "moduleResolution": "NodeNext", + "types": ["node"], + "strict": true, + "noUncheckedIndexedAccess": true, + "noImplicitOverride": true, + "noImplicitReturns": true, + "noFallthroughCasesInSwitch": true, + "noUnusedLocals": true, + "noUnusedParameters": true, + "esModuleInterop": true, + "skipLibCheck": true, + "forceConsistentCasingInFileNames": true, + "isolatedModules": true, + "verbatimModuleSyntax": false, + "noEmit": true + }, + "include": ["**/*.ts"] +} diff --git a/jest.config.cjs b/jest.config.cjs index 42392be6..ec9429a5 100644 --- a/jest.config.cjs +++ b/jest.config.cjs @@ -1,6 +1,8 @@ -// Jest drives the end-to-end suite in `e2e/` only. Unit tests stay on Vitest -// (`vitest.config.ts`, `pnpm test:cov`) -- the two runners cover different -// layers and never overlap: +// Jest drives the end-to-end suite in `e2e/cli/` only; `e2e/desktop/` is +// Playwright's (see e2e/desktop/playwright.config.ts), driving the renderer in +// Chromium against a scripted backend rather than a real Git working tree. +// Unit tests stay on Vitest (`vitest.config.ts`, `pnpm test:cov`) -- these +// three runners cover different layers and never overlap: // // Vitest pure logic, in-process, coverage-gated at 90% // Jest the built `skillkeeper` binary against a real Git working tree diff --git a/package.json b/package.json index 505d3f03..76fcfb07 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ }, "devDependencies": { "@eslint/js": "10.0.1", + "@playwright/test": "1.63.0", "@resvg/resvg-js": "^2.6.2", "@types/jest": "30.0.0", "@types/node": "24.13.3", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 292c1444..3233a62e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -11,6 +11,9 @@ importers: '@eslint/js': specifier: 10.0.1 version: 10.0.1(eslint@10.10.0(jiti@2.7.0)) + '@playwright/test': + specifier: 1.63.0 + version: 1.63.0 '@resvg/resvg-js': specifier: ^2.6.2 version: 2.6.2 @@ -1159,6 +1162,11 @@ packages: resolution: {integrity: sha512-SEeaJLb3qBNF/OaXnaR1NmmBbFYk1zC0ZH/52fATcRPLFg/p791YrcyFFy44Bo9sLaGuSuLp5Q6axbb/O+v/RA==} engines: {node: ^14.18.0 || >=16.0.0} + '@playwright/test@1.63.0': + resolution: {integrity: sha512-oxMK4vllB9RK5NQ2l1pq1IfOf2AvnEuj/vYGDj0H2nMtmtZpKtCwt/l00GEO6xjGfpBNAvjovvYdCm50dRQkpQ==} + engines: {node: '>=20'} + hasBin: true + '@resvg/resvg-js-android-arm-eabi@2.6.2': resolution: {integrity: sha512-FrJibrAk6v29eabIPgcTUMPXiEz8ssrAk7TXxsiZzww9UTQ1Z5KAbFJs+Z0Ez+VZTYgnE5IQJqBcoSiMebtPHA==} engines: {node: '>= 10'} @@ -3446,6 +3454,16 @@ packages: resolution: {integrity: sha512-HRDzbaKjC+AOWVXxAU/x54COGeIv9eb+6CkDSQoNTt4XyWoIJvuPsXizxu/Fr23EiekbtZwmh1IcIG/l/a10GQ==} engines: {node: '>=8'} + playwright-core@1.63.0: + resolution: {integrity: sha512-rYCsBF/M5HjUch52bbtVONEFjv6Xu8sm8h72dNlR5bzIE1fvC/bxgspzkjSfU+MweEMmPM8KJebG6nnyxo5mCg==} + engines: {node: '>=20'} + hasBin: true + + playwright@1.63.0: + resolution: {integrity: sha512-+7ziBLidS4NaNCdt57SUDT+wYmmd5fmiQejUic/kb+YsYSCPyOOE9sebzMjNmQrsnNpDJqd4WHvV/8lfKfUDUg==} + engines: {node: '>=20'} + hasBin: true + pngjs@6.0.0: resolution: {integrity: sha512-TRzzuFRRmEoSW/p1KVAmiOgPco2Irlah+bGFCeNfJXxxYGwSw7YwAOAcd7X28K/m5bjBWKsC29KyoMfHbypayg==} engines: {node: '>=12.13.0'} @@ -5272,6 +5290,10 @@ snapshots: '@pkgr/core@0.3.6': {} + '@playwright/test@1.63.0': + dependencies: + playwright: 1.63.0 + '@resvg/resvg-js-android-arm-eabi@2.6.2': optional: true @@ -7853,6 +7875,12 @@ snapshots: dependencies: find-up: 4.1.0 + playwright-core@1.63.0: {} + + playwright@1.63.0: + dependencies: + playwright-core: 1.63.0 + pngjs@6.0.0: {} pngjs@7.0.0: {} From 21fe82d06180408428ada4fdd1936d649b679d70 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 01:03:18 +0200 Subject: [PATCH 04/25] fix: address boot-smoke-test review findings --- .gitignore | 14 ++++++++++---- e2e/desktop/harness/commands.ts | 2 +- e2e/desktop/harness/installHarness.ts | 9 +++++++++ e2e/desktop/harness/scenario.ts | 7 +++++++ e2e/desktop/playwright.config.ts | 21 ++++++++++++++++----- e2e/desktop/tests/boot.spec.ts | 16 ++++++++++++++-- jest.config.cjs | 5 +++-- 7 files changed, 60 insertions(+), 14 deletions(-) diff --git a/.gitignore b/.gitignore index 7b6eceb0..a96206ce 100644 --- a/.gitignore +++ b/.gitignore @@ -31,10 +31,16 @@ Thumbs.db # Storybook apps/desktop/storybook-static/ -# Playwright -/test-results/ -/playwright-report/ -/blob-report/ +# Playwright: `outputDir`'s default walks up from the config file's directory +# (e2e/desktop/) to the nearest package.json and appends test-results/ to +# THAT directory -- today that lands at the repository root (e2e/desktop/ has +# no package.json of its own), but that is one `e2e/desktop/package.json` away +# from moving to e2e/desktop/test-results/ (see e2e/cli/package.json for a +# sibling suite that already has one). No leading slash, so these match +# wherever it actually lands rather than assuming which. +test-results/ +playwright-report/ +blob-report/ # Documentation site (mkdocs + uv): uv caches the tools globally, but keep any # local Python virtualenv and the rendered static site out of git. diff --git a/e2e/desktop/harness/commands.ts b/e2e/desktop/harness/commands.ts index 6eb8a627..7d872e53 100644 --- a/e2e/desktop/harness/commands.ts +++ b/e2e/desktop/harness/commands.ts @@ -114,7 +114,7 @@ export function defaultResponses(scenario: Scenario): Record { onboarding_get: scenario.onboarding, repositories_list: scenario.repositories, skills_reconcile: scenario.installs, - skills_available: { skills: [], warnings: [] }, + skills_available: { skills: scenario.skills, warnings: [] }, projects_list: scenario.projects, mcp_reconcile: scenario.mcpInstalls, get_app_version: '0.0.0-e2e', diff --git a/e2e/desktop/harness/installHarness.ts b/e2e/desktop/harness/installHarness.ts index bea15278..fe611b60 100644 --- a/e2e/desktop/harness/installHarness.ts +++ b/e2e/desktop/harness/installHarness.ts @@ -71,6 +71,13 @@ export async function installHarness(page: Page, scenario: Scenario): Promise).__SKK_E2E_CALLS__ = []; (window as unknown as Record).__SKK_E2E_CLIPBOARD__ = []; + // Every command name the callback below could not answer, in call order. + // `store.loadAll` and several call sites swallow a rejection (into + // `store.error`, a caught background-task status, or a `.then(ok, () => + // undefined)`), so an unmocked command does not reliably surface anywhere + // a test's DOM assertions can see it -- a test that needs to know reads + // this array directly instead. + (window as unknown as Record).__SKK_E2E_UNMOCKED__ = []; const label = arg.scenario.windowLabel || 'main'; (window as unknown as Record).__SKK_E2E_WINDOW_LABEL__ = label; @@ -89,6 +96,8 @@ export async function installHarness(page: Page, scenario: Scenario): Promise).__SKK_E2E_CALLS__ as unknown[]; calls.push({ cmd, args }); if (!Object.prototype.hasOwnProperty.call(arg.responses, cmd)) { + const unmocked = (window as unknown as Record).__SKK_E2E_UNMOCKED__ as string[]; + unmocked.push(cmd); throw new Error(`${arg.unknownCommandPrefix}${cmd}`); } return arg.responses[cmd]; diff --git a/e2e/desktop/harness/scenario.ts b/e2e/desktop/harness/scenario.ts index 5ee5f5da..ee20c4d3 100644 --- a/e2e/desktop/harness/scenario.ts +++ b/e2e/desktop/harness/scenario.ts @@ -20,6 +20,9 @@ * projects -> Project[] * installs -> InstallManifest[] * mcpInstalls -> McpInstall[] + * skills -> AvailableSkill[] (the `skills_available` catalog; its + * `warnings` half stays a fixed empty array in + * `commands.ts` -- nothing here needed one yet) * * This is the minimal shape Task 2's command table needs (see * `commands.ts`'s `defaultResponses`). A later task extends this file to add @@ -38,6 +41,9 @@ export interface Scenario { readonly projects: readonly unknown[]; readonly installs: readonly unknown[]; readonly mcpInstalls: readonly unknown[]; + /** The `skills_available` catalog (installable skills across all tracked + * repositories), independent of `installs` (what is already installed). */ + readonly skills: readonly unknown[]; /** * Per-command overrides, merged over `commands.ts`'s defaults. A value here * is plain data for now (see the file-level note on why a function value @@ -79,6 +85,7 @@ export function defaultScenario(): Scenario { projects: [], installs: [], mcpInstalls: [], + skills: [], responses: {}, events: {}, }; diff --git a/e2e/desktop/playwright.config.ts b/e2e/desktop/playwright.config.ts index d9f96672..f1630af8 100644 --- a/e2e/desktop/playwright.config.ts +++ b/e2e/desktop/playwright.config.ts @@ -3,9 +3,15 @@ import { defineConfig, devices } from '@playwright/test'; // The suite drives the PRODUCTION bundle, not the dev server: `vite preview` // serves what `vite build` wrote to dist-tauri. That removes module // transformation, the hot-module transport, and the dev/prod split from the -// failure surface, and makes every run prove the bundle boots. Run -// `pnpm --filter @skillkeeper/desktop run frontend:build` at least once -// before this config's webServer has anything to serve. +// failure surface, and makes every run prove the bundle boots. The webServer +// command below chains the build in: `vite preview` serves a stale +// `dist-tauri` without complaint (only a MISSING one exits loudly), so +// building every time a fresh server actually launches is what stops a +// developer from testing yesterday's bundle and believing today's passed. +// `reuseExistingServer: !process.env.CI` still means a server already +// listening on :4173 from an earlier run is reused with no rebuild at all -- +// stop it between real changes to the renderer if you rely on this config +// picking them up locally. export default defineConfig({ testDir: './tests', // A retry turns a flake into a slow pass and throws away the signal. If a @@ -21,12 +27,17 @@ export default defineConfig({ // reason a click lands nowhere. Task 3 also zeroes durations in CSS. reducedMotion: 'reduce', locale: 'en-US', - trace: 'on-first-retry', + // `retries: 0` above means a trace on-first-retry never gets a retry to + // fire on -- the two settings would otherwise cancel out and every + // failure would ship with no trace at all. + trace: 'retain-on-failure', screenshot: 'only-on-failure', }, projects: [{ name: 'chromium', use: { ...devices['Desktop Chrome'] } }], webServer: { - command: 'pnpm --filter @skillkeeper/desktop exec vite preview --port 4173 --strictPort', + command: + 'pnpm --filter @skillkeeper/desktop run frontend:build && ' + + 'pnpm --filter @skillkeeper/desktop exec vite preview --port 4173 --strictPort', url: 'http://localhost:4173', reuseExistingServer: !process.env.CI, timeout: 60_000, diff --git a/e2e/desktop/tests/boot.spec.ts b/e2e/desktop/tests/boot.spec.ts index 7cae9276..38c32882 100644 --- a/e2e/desktop/tests/boot.spec.ts +++ b/e2e/desktop/tests/boot.spec.ts @@ -7,8 +7,20 @@ test('the application mounts against the scripted backend', async ({ page }) => page.on('pageerror', (e) => failures.push(e.message)); await installHarness(page, defaultScenario()); await page.goto('/'); - // The preloader in index.html stays up until bridgeClient.init() settles, so - // seeing the shell means the harness answered the startup round-trip. + // `` mounts inside main.tsx's `bridgeClient.init().finally(...)`, + // which runs its callback on rejection too -- so `app-shell` appearing does + // NOT by itself prove the harness answered anything. `toBeVisible()` also + // checks bounding box and CSS visibility, not occlusion, so it would still + // pass with `#sk-preloader` (a `position: fixed; inset: 0; z-index: + // 2147483647` overlay) sitting on top of everything. `#sk-preloader` is + // only removed by App.tsx's own effect, once `loading` has cycled true then + // false -- proof the mount-to-first-effects lifecycle ran to completion, not + // proof any one command was answered correctly. What actually catches a + // broken command (like an unmocked `platform`, which several call sites + // never await/catch) is the `failures` array below, from `pageerror`. All + // three assertions together are what the "scripted backend answers" + // round-trip needs; any one alone would pass on a harness with real gaps. await expect(page.getByTestId('app-shell')).toBeVisible(); + await expect(page.locator('#sk-preloader')).toHaveCount(0); expect(failures, failures.join('\n')).toEqual([]); }); diff --git a/jest.config.cjs b/jest.config.cjs index ec9429a5..c25759c7 100644 --- a/jest.config.cjs +++ b/jest.config.cjs @@ -4,8 +4,9 @@ // Unit tests stay on Vitest (`vitest.config.ts`, `pnpm test:cov`) -- these // three runners cover different layers and never overlap: // -// Vitest pure logic, in-process, coverage-gated at 90% -// Jest the built `skillkeeper` binary against a real Git working tree +// Vitest pure logic, in-process, coverage-gated at 90% +// Jest the built `skillkeeper` binary against a real Git working tree +// Playwright the built renderer bundle in Chromium, against a scripted backend // // This file is `.cjs` on purpose: the root package.json sets `"type": "module"`, // so a `.js` config would be ESM and Jest's config loader plus ts-jest's From 3a7593e0e93e10e6e2aeca1b7620ef4effa11a70 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 01:19:22 +0200 Subject: [PATCH 05/25] test: give the desktop suite a scenario-driven fixture Scenario now carries every field the renderer's bridge types define (imported straight from the generated/hand-written sources instead of hand-rolled approximations), and withScenario/the app fixture let a spec declare one and drive it through a scripted backend. defaultScenario answers terminal_start and app_update_check so a boot is complete, and the fixture fails loudly if any command ever reaches the backend unmocked again. --- e2e/desktop/fixtures/base.ts | 128 ++++++++++++++++++++++++++ e2e/desktop/harness/commands.ts | 44 ++++++++- e2e/desktop/harness/fixture.ts | 12 +++ e2e/desktop/harness/installHarness.ts | 18 ++++ e2e/desktop/harness/scenario.ts | 111 ++++++++++++++-------- e2e/desktop/tests/boot.spec.ts | 9 +- e2e/desktop/tests/harness.spec.ts | 36 ++++++++ e2e/desktop/tsconfig.json | 25 ++++- 8 files changed, 332 insertions(+), 51 deletions(-) create mode 100644 e2e/desktop/fixtures/base.ts create mode 100644 e2e/desktop/harness/fixture.ts create mode 100644 e2e/desktop/tests/harness.spec.ts diff --git a/e2e/desktop/fixtures/base.ts b/e2e/desktop/fixtures/base.ts new file mode 100644 index 00000000..5dcc26bf --- /dev/null +++ b/e2e/desktop/fixtures/base.ts @@ -0,0 +1,128 @@ +/** + * The Playwright fixture every desktop spec is built on: a `scenario` test + * option, and an `app` object wired to whatever that scenario says. + * + * `harness/fixture.ts` re-exports `test`/`expect` from here under the path + * every spec actually imports (`'../harness/fixture'`). This file is the + * implementation, kept apart from `harness/` on purpose: `harness/` is the + * backend-scripting mechanism (`Scenario`, `installHarness`, the command + * table) and knows nothing about Playwright's `test.extend`; this file is + * testing-framework plumbing layered on top of that mechanism, not part of + * it. + */ +import { test as base, expect } from '@playwright/test'; +import type { Page } from '@playwright/test'; +import { installHarness } from '../harness/installHarness.js'; +import { defaultScenario } from '../harness/scenario.js'; +import type { Scenario } from '../harness/scenario.js'; + +/** One entry of `window.__SKK_E2E_CALLS__` -- see `installHarness.ts`. */ +interface RecordedCall { + readonly cmd: string; + readonly args: unknown; +} + +/** The spec-facing handle onto one test's scripted backend. */ +export interface App { + /** + * Zeroes animation/transition durations, then navigates to the app's root. + * Call once per test, before any assertion or interaction. + * + * `reducedMotion: 'reduce'` (see `playwright.config.ts`) is a media-query + * hint that `motion` only honours where a component asks for it -- this is + * belt-and-braces on top of that, so a click can never land mid-transition + * regardless of whether the component checked. The stylesheet is added + * right after the document exists (immediately post-navigation, before + * control returns to the spec), not literally before `page.goto` -- + * `addStyleTag` writes into the CURRENT document, and a full navigation + * replaces that document, so anything added beforehand would not survive + * the trip anyway. + */ + goto(): Promise; + /** + * Dispatches `payload` to every listener currently registered for `name`, + * through the same mocked event plugin `@tauri-apps/api/event`'s + * `listen()`/`emit()` use internally (see `installHarness.ts`'s + * `shouldMockEvents` note) -- e.g. to drive `skills:progress` at a moment + * the spec chooses, not whenever a real backend operation would have. + */ + emit(name: string, payload: unknown): Promise; + /** Every recorded invocation of `command`, as its `args`, in call order. + * Empty when the command was never called. */ + calls(command: string): Promise; + /** Every clipboard write recorded so far (see `installHarness.ts`'s + * clipboard stub), in write order. */ + clipboard(): Promise; +} + +function buildApp(page: Page): App { + return { + async goto() { + await page.goto('/'); + await page.addStyleTag({ + content: `*, *::before, *::after { + animation-duration: 0s !important; + animation-delay: 0s !important; + transition-duration: 0s !important; + transition-delay: 0s !important; + }`, + }); + }, + async emit(name, payload) { + await page.evaluate( + ({ name, payload }) => { + const internals = ( + window as unknown as { + __TAURI_INTERNALS__: { invoke: (cmd: string, args: unknown) => Promise }; + } + ).__TAURI_INTERNALS__; + return internals.invoke('plugin:event|emit', { event: name, payload }); + }, + { name, payload }, + ); + }, + async calls(command) { + return page.evaluate((command) => { + const calls = ((window as unknown as Record).__SKK_E2E_CALLS__ ?? []) as RecordedCall[]; + return calls.filter((call) => call.cmd === command).map((call) => call.args); + }, command); + }, + async clipboard() { + return page.evaluate( + () => ((window as unknown as Record).__SKK_E2E_CLIPBOARD__ ?? []) as string[], + ); + }, + }; +} + +interface Fixtures { + /** The backend data for this test. Override per spec (or per describe + * block) with `test.use({ scenario: withScenario({ ... }) })`. */ + scenario: Scenario; + app: App; +} + +export const test = base.extend({ + scenario: [defaultScenario(), { option: true }], + app: async ({ page, scenario }, use) => { + await installHarness(page, scenario); + await use(buildApp(page)); + // Fail loudly rather than let an unmocked command hide behind a caught + // rejection (see `installHarness.ts`'s `__SKK_E2E_UNMOCKED__` note). + // `defaultScenario()` is a complete startup (see `commands.ts`'s Task 3 + // additions for `terminal_start`), so a non-empty list here means either + // this spec's scenario left a command it actually exercises unanswered, + // or the renderer started calling a new one this harness has not caught + // up with -- either way, a passing test that hit this is the wrong + // outcome, not a flake to retry away (see `playwright.config.ts`'s + // `retries: 0`). + const unmocked = await page.evaluate( + () => (window as unknown as Record).__SKK_E2E_UNMOCKED__ as string[] | undefined, + ); + if (unmocked && unmocked.length > 0) { + throw new Error(`e2e harness: unmocked command(s) reached the backend: ${unmocked.join(', ')}`); + } + }, +}); + +export { expect }; diff --git a/e2e/desktop/harness/commands.ts b/e2e/desktop/harness/commands.ts index 7d872e53..25e62539 100644 --- a/e2e/desktop/harness/commands.ts +++ b/e2e/desktop/harness/commands.ts @@ -51,6 +51,24 @@ export const UNKNOWN_COMMAND_PREFIX = 'e2e-harness: unmocked command '; * view's initial fit/resize, independent of * whether the terminal panel is open. * + * TASK 3 ADDITION -- terminal_start: `TerminalView`'s mount effect awaits + * `startWithRetry(() => bridgeClient.startTerminal(...))` + * (`systems/terminal/startShell.ts`), which retries an unmocked/rejecting + * call up to `START_ATTEMPTS` (3) times with a real `setTimeout` + * (`START_RETRY_MS`, 750ms) between attempts before settling into + * `setTerminalError`. Left unmocked, every boot -- not just this suite's, any + * spec's -- pays that ~1.5s of real timers running in the background before + * the rejection is swallowed into store state, which is exactly the kind of + * nondeterministic, timer-driven tail this suite's "no timers, no randomness" + * rule exists to rule out; it also used to be invisible to `boot.spec.ts` + * (caught locally, no page error, no assertion touches `terminalOpen`), which + * is why Task 2's `__SKK_E2E_UNMOCKED__` array was the only thing that caught + * it. Mocked here so `defaultScenario()` is a genuinely complete startup and + * the fixture's post-test "nothing went unmocked" assertion (`fixture.ts`) + * never has to carry an exemption list for it. The value is the retained + * scrollback the renderer replays into the terminal on start; empty string is + * a valid "freshly started, nothing buffered yet" answer. + * * `boot.spec.ts` would pass with only those three mocked -- `loadAll`'s * Promise.all would simply reject and `store.error` would be set. This table * mocks the rest of `loadAll`'s round trip anyway, because a boot test that @@ -76,14 +94,31 @@ export const UNKNOWN_COMMAND_PREFIX = 'e2e-harness: unmocked command '; * either -- included for the same reason as the * `loadAll` set above). * + * TASK 3 ADDITION -- mcp_list_available: NOT part of startup (`loadAll` never + * calls it -- only `refreshMcpPresets`, run after a repository + * add/update/sync or when the MCP page reads its catalog). Mocked here anyway + * so a scenario's `mcpAvailable` catalog (the MCP counterpart of `skills` + * above) is answered the moment a later task's flow reaches for it, the same + * way `skills_available` is answered though nothing in `boot.spec.ts` needs + * it either. + * + * TASK 3 ADDITION -- app_update_check: fires once, unconditionally, every + * boot (`useAppUpdateSchedule`'s startup check), independent of the + * scenario's `updates.mode`. `store.runAppUpdateCheck` wraps the call in + * try/catch, so `boot.spec.ts`'s page-error-based assertions never needed + * this mocked -- but that same try/catch is exactly what let it slip past + * unnoticed into `__SKK_E2E_UNMOCKED__` until Task 3's fixture started + * asserting that array empty after every test (see `fixtures/base.ts`). + * Answered here for the same "complete startup" reason as `terminal_start` + * above, with the least eventful `CheckOutcome`: no offer, and `suppressed: + * true` so a spec never has to reason about `checkAppUpdate`'s network-gate + * semantics by accident. + * * Commands that need no entry at all, and why: * - `window_is_maximized` / `window:maximizeChanged` (WindowChrome): only * called when `hostPlatform(bridgeClient.platform) !== 'mac'`; * `defaultScenario` reports `platform: 'darwin'`, so WindowChrome renders * nothing and never calls either. - * - `app_update_check` (useAppUpdateSchedule's one-time startup check): - * DOES fire once loading settles, but `store.runAppUpdateCheck` wraps the - * call in try/catch, same as the `loadAll` set above. * - Every repository/project/MCP mutation and every `plugin:event|*` * listener registration: `mockIPC` is called with * `{ shouldMockEvents: true }` (see `installHarness.ts`), so `listen()` @@ -96,6 +131,7 @@ export function defaultResponses(scenario: Scenario): Record { platform: scenario.platform, onboarding_menu_sync: null, terminal_resize: null, + terminal_start: '', config_get: { config: scenario.config, validity: { @@ -117,6 +153,8 @@ export function defaultResponses(scenario: Scenario): Record { skills_available: { skills: scenario.skills, warnings: [] }, projects_list: scenario.projects, mcp_reconcile: scenario.mcpInstalls, + mcp_list_available: { mcp: scenario.mcpAvailable, warnings: [] }, + app_update_check: { offer: null, suppressed: true }, get_app_version: '0.0.0-e2e', ...scenario.responses, }; diff --git a/e2e/desktop/harness/fixture.ts b/e2e/desktop/harness/fixture.ts new file mode 100644 index 00000000..b1217ff6 --- /dev/null +++ b/e2e/desktop/harness/fixture.ts @@ -0,0 +1,12 @@ +/** + * The name every spec imports the scenario-driven `test`/`expect` from + * (`import { test, expect } from '../harness/fixture'`). + * + * The implementation lives in `../fixtures/base.ts` -- see that file's doc + * comment for why the two are separate. This module exists so a spec reaches + * for it alongside the rest of the harness (`scenario.ts`, `installHarness.ts` + * are both in this directory) without needing to know about the `fixtures/` + * split. + */ +export { test, expect } from '../fixtures/base.js'; +export type { App } from '../fixtures/base.js'; diff --git a/e2e/desktop/harness/installHarness.ts b/e2e/desktop/harness/installHarness.ts index fe611b60..c60969a4 100644 --- a/e2e/desktop/harness/installHarness.ts +++ b/e2e/desktop/harness/installHarness.ts @@ -70,6 +70,10 @@ export async function installHarness(page: Page, scenario: Scenario): Promise).__SKK_E2E_CALLS__ = []; + // Writes recorded by the clipboard stub below, in write order. A spec + // reads this through `fixture.ts`'s `app.clipboard()` to assert a copy + // action without a real OS clipboard (there is none in a headless + // Chromium run, and the Clipboard API needs a user gesture besides). (window as unknown as Record).__SKK_E2E_CLIPBOARD__ = []; // Every command name the callback below could not answer, in call order. // `store.loadAll` and several call sites swallow a rejection (into @@ -85,6 +89,14 @@ export async function installHarness(page: Page, scenario: Scenario): Promise { const calls = (window as unknown as Record).__SKK_E2E_CALLS__ as unknown[]; calls.push({ cmd, args }); + if (cmd === CLIPBOARD_WRITE_TEXT) { + const clipboard = (window as unknown as Record).__SKK_E2E_CLIPBOARD__ as string[]; + const text = (args as { text?: unknown } | undefined)?.text; + clipboard.push(typeof text === 'string' ? text : ''); + return null; + } if (!Object.prototype.hasOwnProperty.call(arg.responses, cmd)) { const unmocked = (window as unknown as Record).__SKK_E2E_UNMOCKED__ as string[]; unmocked.push(cmd); diff --git a/e2e/desktop/harness/scenario.ts b/e2e/desktop/harness/scenario.ts index ee20c4d3..1fdd27da 100644 --- a/e2e/desktop/harness/scenario.ts +++ b/e2e/desktop/harness/scenario.ts @@ -11,23 +11,18 @@ * needs a per-call computed answer resolves it a different way (see * `installHarness.ts`'s notes on that boundary). * - * Field shapes loosely mirror the real backend's without importing from - * `apps/desktop` -- this suite stays a self-contained package, the same way - * `e2e/cli` does not depend on the CLI crate's Rust types either: - * config -> LoadConfigResult['config'] (SkillKeeperConfig) - * onboarding -> OnboardingState { version, completed, step } - * repositories -> Repository[] - * projects -> Project[] - * installs -> InstallManifest[] - * mcpInstalls -> McpInstall[] - * skills -> AvailableSkill[] (the `skills_available` catalog; its - * `warnings` half stays a fixed empty array in - * `commands.ts` -- nothing here needed one yet) - * - * This is the minimal shape Task 2's command table needs (see - * `commands.ts`'s `defaultResponses`). A later task extends this file to add - * whatever a flow test needs; it will not recreate it. + * Every field type below is imported from the same generated/hand-written + * sources the renderer itself uses (`apps/desktop/src/renderer/services/ + * bridge/generated/**` for the ts-rs output, `.../bridge/contracts.ts` for the + * hand-written result wrappers) -- never a hand-rolled approximation. A + * scenario that drifts from those shapes is a defect here, not a "close + * enough" fixture; the renderer would reject the same payload from the real + * backend. */ +import type { SkillKeeperConfig, OnboardingState } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/index.js'; +import type { Repository, Project, InstallManifest } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/index.js'; +import type { AvailableSkill, AvailableMcp, McpInstall } from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; + export interface Scenario { /** `process.platform` as the Rust backend reports it: 'darwin' | 'win32' | 'linux'. */ readonly platform: string; @@ -35,27 +30,65 @@ export interface Scenario { * app (`SshUnlockApp`) for the 'ssh-unlock' label, so this is load-bearing, * not decoration -- default 'main'. */ readonly windowLabel: string; - readonly config: Record; - readonly onboarding: Record; - readonly repositories: readonly unknown[]; - readonly projects: readonly unknown[]; - readonly installs: readonly unknown[]; - readonly mcpInstalls: readonly unknown[]; + readonly config: SkillKeeperConfig; + readonly onboarding: OnboardingState; + readonly repositories: readonly Repository[]; + readonly projects: readonly Project[]; + readonly installs: readonly InstallManifest[]; /** The `skills_available` catalog (installable skills across all tracked - * repositories), independent of `installs` (what is already installed). */ - readonly skills: readonly unknown[]; + * repositories), independent of `installs` (what is already installed). + * `skills_available`'s `warnings` half stays a fixed empty array in + * `commands.ts` -- no flow in this plan needs one yet. */ + readonly skills: readonly AvailableSkill[]; + /** The `mcp_list_available` catalog (installable MCP presets discovered from + * tracked repositories), independent of `mcpInstalls` (what is already + * installed) -- same split as `skills`/`installs` above. Its `warnings` + * half is likewise fixed to an empty array in `commands.ts`. Not part of + * startup (`loadAll` never calls it; only `refreshMcpPresets`, run after a + * repository add/update/sync or when the MCP page asks), so an empty + * default costs `defaultScenario` nothing. */ + readonly mcpAvailable: readonly AvailableMcp[]; + readonly mcpInstalls: readonly McpInstall[]; /** * Per-command overrides, merged over `commands.ts`'s defaults. A value here * is plain data for now (see the file-level note on why a function value * cannot cross into the page yet). */ readonly responses: Record; - /** Named canned event payloads a later task's harness can emit on demand - * (e.g. to drive `onConfigChanged`/`onTerminalData` subscribers). Unused - * until that task wires an emit path through the mocked event plugin. */ + /** + * Reserved for a scenario that wants to declare named event payloads + * up front (e.g. to seed a subscriber before the spec's first assertion). + * Unused today: `fixture.ts`'s `app.emit(name, payload)` is the imperative + * escape hatch a spec uses instead, dispatched on demand through the mocked + * event plugin rather than replayed from scenario data. Kept as part of the + * `Scenario` shape so a later task can add that replay without another + * interface change. + */ readonly events: Record; } +/** A `SkillKeeperConfig` with every section present and valid, matching what + * a fresh install's `config.yaml` defaults resolve to. */ +function emptyConfig(): SkillKeeperConfig { + return { + general: { language: 'en', theme: 'system', animations: 'normal' }, + updates: { mode: 'manual', intervalMinutes: 720, checkOnStartup: false }, + agents: { enabled: ['claude', 'codex', 'copilot', 'cursor', 'opencode'], overrides: {} }, + executables: { globs: [] }, + security: { hookConsentPolicy: 'always-ask' }, + notifications: { enabled: true }, + repositories: { gitPath: 'git' }, + projects: { checkIntervalMinutes: 1 }, + mcp: { servers: [] }, + }; +} + +/** No skills installed anywhere -- the ledger `skills_reconcile` returns for a + * fresh install. */ +function emptyManifest(): InstallManifest[] { + return []; +} + /** * A scenario with a clean, empty-but-valid backend: no repositories, no * projects, no installs, onboarding already completed (so the guided tour @@ -69,24 +102,22 @@ export function defaultScenario(): Scenario { return { platform: 'darwin', windowLabel: 'main', - config: { - general: { language: 'en', theme: 'system', animations: 'normal' }, - updates: { mode: 'manual', intervalMinutes: 720, checkOnStartup: false }, - agents: { enabled: ['claude', 'codex', 'copilot', 'cursor', 'opencode'], overrides: {} }, - executables: { globs: [] }, - security: { hookConsentPolicy: 'always-ask' }, - notifications: { enabled: true }, - repositories: { gitPath: 'git' }, - projects: { checkIntervalMinutes: 1 }, - mcp: { servers: [] }, - }, + config: emptyConfig(), onboarding: { version: 1, completed: true, step: 'done' }, repositories: [], projects: [], - installs: [], - mcpInstalls: [], + installs: emptyManifest(), skills: [], + mcpAvailable: [], + mcpInstalls: [], responses: {}, events: {}, }; } + +/** `defaultScenario()` with `patch` shallow-merged over it. A field in `patch` + * replaces the default wholesale (e.g. `repositories: [...]` replaces the + * empty array, it does not append to it). */ +export function withScenario(patch: Partial): Scenario { + return { ...defaultScenario(), ...patch }; +} diff --git a/e2e/desktop/tests/boot.spec.ts b/e2e/desktop/tests/boot.spec.ts index 38c32882..4798ff40 100644 --- a/e2e/desktop/tests/boot.spec.ts +++ b/e2e/desktop/tests/boot.spec.ts @@ -1,12 +1,9 @@ -import { test, expect } from '@playwright/test'; -import { installHarness } from '../harness/installHarness.js'; -import { defaultScenario } from '../harness/scenario.js'; +import { test, expect } from '../harness/fixture'; -test('the application mounts against the scripted backend', async ({ page }) => { +test('the application mounts against the scripted backend', async ({ app, page }) => { const failures: string[] = []; page.on('pageerror', (e) => failures.push(e.message)); - await installHarness(page, defaultScenario()); - await page.goto('/'); + await app.goto(); // `` mounts inside main.tsx's `bridgeClient.init().finally(...)`, // which runs its callback on rejection too -- so `app-shell` appearing does // NOT by itself prove the harness answered anything. `toBeVisible()` also diff --git a/e2e/desktop/tests/harness.spec.ts b/e2e/desktop/tests/harness.spec.ts new file mode 100644 index 00000000..a3aff7b9 --- /dev/null +++ b/e2e/desktop/tests/harness.spec.ts @@ -0,0 +1,36 @@ +/** + * Proves the scenario-driven fixture actually threads a spec's data through + * to the scripted backend: overriding `repositories` on the scenario reaches + * `repositories_list`, the same call `store.loadAll` makes at boot. + * + * This does NOT assert on a rendered repository row -- `repo-row` is a + * later task's testid, not this one's. Once it exists, that task tightens + * this same scenario onto it; until then, "the backend answered with the + * scenario's data" is proven at the bridge boundary instead of the DOM. + */ +import { test, expect } from '../harness/fixture'; +import { withScenario } from '../harness/scenario'; + +test.use({ + scenario: withScenario({ + repositories: [ + { + id: 'demo', + name: 'demo', + url: 'https://example.invalid/demo.git', + kind: 'generic', + transport: 'https', + lfs: false, + localPath: '/repos/demo', + branch: 'main', + }, + ], + }), +}); + +test('a scenario decides what the backend returns', async ({ app, page }) => { + await app.goto(); + await expect(page.getByTestId('app-shell')).toBeVisible(); + const calls = await app.calls('repositories_list'); + expect(calls.length).toBeGreaterThan(0); +}); diff --git a/e2e/desktop/tsconfig.json b/e2e/desktop/tsconfig.json index d3a83161..5825b34d 100644 --- a/e2e/desktop/tsconfig.json +++ b/e2e/desktop/tsconfig.json @@ -12,11 +12,32 @@ // the browser, and stays ESM (the repository root is "type": "module" and // this directory has no package.json of its own to override that), which is // what lets installHarness.ts use `import.meta.url`. + // + // "moduleResolution": "bundler" (Task 3), not "NodeNext": `scenario.ts` + // imports its field types straight from the ts-rs-generated sources under + // `apps/desktop/src/renderer/services/bridge/generated/**` rather than + // hand-rolling approximations, and those generated files -- authored for + // Vite's bundler resolution, same as the rest of `apps/desktop` -- import + // each other with no file extension. `tsc` type-checks a whole program, not + // file-by-file, so once anything in this directory reaches into that tree, + // EVERY file on the way is checked under this tsconfig's resolution mode; + // under "NodeNext" that turns every one of those extensionless sibling + // imports into an error in a generated file this suite does not own and + // must not edit. "bundler" accepts both a bare specifier and an explicit + // `.js` extension pointing at a `.ts` source, so it resolves that tree + // cleanly without changing how this directory's OWN files are written -- + // they keep the explicit `.js` extension throughout, which also still + // matches how Node actually resolves them at runtime (the "type": "module" + // requirement `.js`-on-`.ts` served under `NodeNext`), this option just + // stops REQUIRING it. Playwright's runtime never consults this setting at + // all (see the note above: esbuild transpiles per file and does not + // validate module resolution the way `tsc` does), so this is purely a + // type-checking/editor concern. "compilerOptions": { "target": "ES2023", "lib": ["ES2023", "DOM"], - "module": "NodeNext", - "moduleResolution": "NodeNext", + "module": "ESNext", + "moduleResolution": "bundler", "types": ["node"], "strict": true, "noUncheckedIndexedAccess": true, From b235175e8fa0acde6e380895dce211177d5bf74f Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 01:28:52 +0200 Subject: [PATCH 06/25] fix(e2e): zero animations earlier and prove the harness wiring Move the animation-zeroing stylesheet into a page.addInitScript so it lands before the app's own scripts on every navigation instead of after page.goto resolves, closing a window where an entrance animation could start and finish before the zeroing style arrived. Add self-tests that drive app.emit and app.clipboard through the same wire shapes the real event and clipboard-manager plugins use, so their wiring is proven rather than only reasoned about. --- e2e/desktop/fixtures/base.ts | 68 ++++++++++++++++++-------- e2e/desktop/tests/harness.spec.ts | 79 ++++++++++++++++++++++++++++--- 2 files changed, 122 insertions(+), 25 deletions(-) diff --git a/e2e/desktop/fixtures/base.ts b/e2e/desktop/fixtures/base.ts index 5dcc26bf..c6094fc1 100644 --- a/e2e/desktop/fixtures/base.ts +++ b/e2e/desktop/fixtures/base.ts @@ -25,18 +25,13 @@ interface RecordedCall { /** The spec-facing handle onto one test's scripted backend. */ export interface App { /** - * Zeroes animation/transition durations, then navigates to the app's root. - * Call once per test, before any assertion or interaction. + * Navigates to the app's root. Call once per test, before any assertion or + * interaction. * - * `reducedMotion: 'reduce'` (see `playwright.config.ts`) is a media-query - * hint that `motion` only honours where a component asks for it -- this is - * belt-and-braces on top of that, so a click can never land mid-transition - * regardless of whether the component checked. The stylesheet is added - * right after the document exists (immediately post-navigation, before - * control returns to the spec), not literally before `page.goto` -- - * `addStyleTag` writes into the CURRENT document, and a full navigation - * replaces that document, so anything added beforehand would not survive - * the trip anyway. + * The animation/transition-zeroing stylesheet is NOT applied here -- it is + * installed once, per test, via `installAnimationZeroing`'s + * `page.addInitScript` (see that function's doc comment for why `goto` + * itself is too late for it). */ goto(): Promise; /** @@ -55,18 +50,52 @@ export interface App { clipboard(): Promise; } +/** + * Zeroes animation/transition durations for every document this `page` ever + * navigates to, from the very first paint. + * + * `reducedMotion: 'reduce'` (see `playwright.config.ts`) is a media-query + * hint that `motion` only honours where a component asks for it -- this is + * belt-and-braces on top of that, so a click can never land mid-transition + * regardless of whether the component checked. + * + * This MUST be a `page.addInitScript`, not a post-navigation + * `page.addStyleTag`: `addInitScript` re-runs on every navigation, before any + * of the page's own scripts -- exactly the "beats every lazily imported + * route" guarantee `installHarness.ts` relies on for the backend mocks, and + * the same guarantee an entrance animation needs here. A style added AFTER + * `page.goto()` resolves (`load`, by default) arrives long after the + * document exists and after the app's own scripts started -- an entrance + * animation triggered on initial mount can start, and finish, before that + * style ever lands. Since `document.documentElement` may not exist yet at + * the moment an init script first runs, this falls back to `DOMContentLoaded` + * when it does not. + */ +async function installAnimationZeroing(page: Page): Promise { + await page.addInitScript(() => { + const css = `*, *::before, *::after { + animation-duration: 0s !important; + animation-delay: 0s !important; + transition-duration: 0s !important; + transition-delay: 0s !important; + }`; + function insert(): void { + const style = document.createElement('style'); + style.textContent = css; + document.documentElement.appendChild(style); + } + if (document.documentElement) { + insert(); + } else { + document.addEventListener('DOMContentLoaded', insert, { once: true }); + } + }); +} + function buildApp(page: Page): App { return { async goto() { await page.goto('/'); - await page.addStyleTag({ - content: `*, *::before, *::after { - animation-duration: 0s !important; - animation-delay: 0s !important; - transition-duration: 0s !important; - transition-delay: 0s !important; - }`, - }); }, async emit(name, payload) { await page.evaluate( @@ -106,6 +135,7 @@ export const test = base.extend({ scenario: [defaultScenario(), { option: true }], app: async ({ page, scenario }, use) => { await installHarness(page, scenario); + await installAnimationZeroing(page); await use(buildApp(page)); // Fail loudly rather than let an unmocked command hide behind a caught // rejection (see `installHarness.ts`'s `__SKK_E2E_UNMOCKED__` note). diff --git a/e2e/desktop/tests/harness.spec.ts b/e2e/desktop/tests/harness.spec.ts index a3aff7b9..8db5bdd2 100644 --- a/e2e/desktop/tests/harness.spec.ts +++ b/e2e/desktop/tests/harness.spec.ts @@ -1,12 +1,25 @@ /** * Proves the scenario-driven fixture actually threads a spec's data through - * to the scripted backend: overriding `repositories` on the scenario reaches - * `repositories_list`, the same call `store.loadAll` makes at boot. + * to the scripted backend, and that its two imperative escape hatches + * (`app.emit`, `app.clipboard`) actually work end to end -- not just by + * construction. * - * This does NOT assert on a rendered repository row -- `repo-row` is a - * later task's testid, not this one's. Once it exists, that task tightens - * this same scenario onto it; until then, "the backend answered with the - * scenario's data" is proven at the bridge boundary instead of the DOM. + * The first test does NOT assert on a rendered repository row -- `repo-row` + * is a later task's testid, not this one's. Once it exists, that task + * tightens this same scenario onto it; until then, "the backend answered + * with the scenario's data" is proven at the bridge boundary instead of the + * DOM. + * + * The other two are self-tests of the fixture itself, not of the app: five + * later tasks compose specs against `app.emit`/`app.clipboard` (one + * specifically needs `app.emit` to drive `skills:progress`), and neither has + * any application UI to exercise it through yet. Each drives the exact wire + * shape a real caller would -- `plugin:event|listen`/`|emit` the way + * `@tauri-apps/api/event`'s `listen`/`emit` build them (see `core.js`'s + * `invoke` and `event.js`'s `listen`), and `plugin:clipboard-manager|write_text` + * the way `@tauri-apps/plugin-clipboard-manager`'s `writeText` builds it -- + * from inside the page, entirely independent of `fixture.ts`'s own + * implementation, so a wrong shape on either side surfaces here. */ import { test, expect } from '../harness/fixture'; import { withScenario } from '../harness/scenario'; @@ -34,3 +47,57 @@ test('a scenario decides what the backend returns', async ({ app, page }) => { const calls = await app.calls('repositories_list'); expect(calls.length).toBeGreaterThan(0); }); + +/** Shape of `window.__TAURI_INTERNALS__` this file's self-tests reach into + * directly, matching `fixture.ts`'s own narrowing. */ +interface TauriInternals { + readonly invoke: (cmd: string, args: unknown) => Promise; + readonly transformCallback: (callback: (data: unknown) => void) => number; +} + +test('app.emit dispatches to a page-registered listener', async ({ app, page }) => { + await app.goto(); + + // Register a listener the same way `@tauri-apps/api/event`'s `listen()` + // registers one under the hood: mint a callback id via `transformCallback`, + // then invoke `plugin:event|listen` with it. This is the harness's own + // mocked event plugin (`installHarness.ts`'s `shouldMockEvents`), reached + // directly rather than through the app -- no application code needs to + // exist for this to prove `app.emit` actually dispatches. + await page.evaluate(() => { + const internals = (window as unknown as { __TAURI_INTERNALS__: TauriInternals }).__TAURI_INTERNALS__; + const received = window as unknown as Record; + received.__E2E_SELF_TEST_RECEIVED__ = undefined; + const handler = internals.transformCallback((data) => { + received.__E2E_SELF_TEST_RECEIVED__ = data; + }); + return internals.invoke('plugin:event|listen', { + event: 'e2e-self-test', + target: { kind: 'Any' }, + handler, + }); + }); + + await app.emit('e2e-self-test', { hello: 'world' }); + + const received = await page.evaluate( + () => (window as unknown as Record).__E2E_SELF_TEST_RECEIVED__, + ); + expect(received).toEqual({ event: 'e2e-self-test', payload: { hello: 'world' } }); +}); + +test('app.clipboard records a write made through the clipboard plugin', async ({ app, page }) => { + await app.goto(); + + // The exact command name and argument shape + // `@tauri-apps/plugin-clipboard-manager`'s `writeText()` sends (see its + // `dist-js/index.js`), reached directly rather than through the app -- + // there is no copy button to click yet. + await page.evaluate(() => { + const internals = (window as unknown as { __TAURI_INTERNALS__: TauriInternals }).__TAURI_INTERNALS__; + return internals.invoke('plugin:clipboard-manager|write_text', { text: 'copied-by-self-test' }); + }); + + const clipboard = await app.clipboard(); + expect(clipboard).toEqual(['copied-by-self-test']); +}); From 6637fa44446ae48e8276f2ef44e2db3e0ae9863a Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 01:53:48 +0200 Subject: [PATCH 07/25] test: cover adding a repository and a failing clone Adds the data-testid convention later tasks reuse (repositories-page, repo-add-*, repo-row/data-repo-name, repo-row-branch), the two flow-1/ flow-9 specs, and their scenario fixtures. RepoAddButton now awaits the add-then-clone chain to tell an add-level failure from success and surfaces it inline instead of closing blind. Repositories is not the default view, so a nav- testid was added to the sidebar (App.tsx, SidebarItem.tsx) for the spec to reach it; the row's name/branch testids landed on RepositoryCard, the only place that content actually renders. harness.spec.ts's placeholder assertion is tightened onto the now-real repo-row. --- apps/desktop/src/renderer/app/App.tsx | 8 +- .../entities/repository/ui/RepositoryCard.tsx | 5 +- .../features/repoAdd/ui/RepoAddButton.tsx | 75 ++++++++++++++++-- .../pages/Repositories/RepositoriesPage.tsx | 7 +- .../shared/ui/Sidebar/SidebarItem.tsx | 12 ++- e2e/desktop/fixtures/repositories.ts | 77 +++++++++++++++++++ e2e/desktop/tests/harness.spec.ts | 20 +++-- e2e/desktop/tests/repositories.spec.ts | 44 +++++++++++ 8 files changed, 230 insertions(+), 18 deletions(-) create mode 100644 e2e/desktop/fixtures/repositories.ts create mode 100644 e2e/desktop/tests/repositories.spec.ts diff --git a/apps/desktop/src/renderer/app/App.tsx b/apps/desktop/src/renderer/app/App.tsx index fba3c605..6a148558 100644 --- a/apps/desktop/src/renderer/app/App.tsx +++ b/apps/desktop/src/renderer/app/App.tsx @@ -303,7 +303,13 @@ export function App() { drag/traffic-light zone, so it renders a draggable panel there. */} {NAV_ITEMS.map(({ id, key }) => ( - } active={activeView === id} onClick={() => goTo(id)}> + } + active={activeView === id} + onClick={() => goTo(id)} + data-testid={`nav-${id}`} + > {t(key)} ))} diff --git a/apps/desktop/src/renderer/entities/repository/ui/RepositoryCard.tsx b/apps/desktop/src/renderer/entities/repository/ui/RepositoryCard.tsx index 3b202d58..1077c01a 100644 --- a/apps/desktop/src/renderer/entities/repository/ui/RepositoryCard.tsx +++ b/apps/desktop/src/renderer/entities/repository/ui/RepositoryCard.tsx @@ -119,7 +119,9 @@ export function RepositoryCard({
- {repository.name} + + {repository.name} + {indicatorKey !== null && ( diff --git a/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx b/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx index ee21a33c..9627cfde 100644 --- a/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx +++ b/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx @@ -1,7 +1,8 @@ -import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; import { useSkillkeeperStore } from '@/app/store'; import { useTranslator } from '@/systems/i18n'; import { deriveRepoName, MAX_REPO_NAME_LENGTH } from '@/entities/repository'; +import { resolveNotification } from '@/systems/notifications'; import { Button, Modal, TextField } from '@/shared/ui'; import { asSchemeUrl, scpPortMistake } from '../lib/remoteHint'; import './RepoAddButton.scss'; @@ -25,11 +26,23 @@ export function RepoAddButton() { const [url, setUrl] = useState(''); const [name, setName] = useState(''); const [nameEdited, setNameEdited] = useState(false); + const [submitting, setSubmitting] = useState(false); + // Set only when `addRepository` itself failed (the `repositories_add` + // command, before any row could exist) -- a clone failure that happens + // AFTER the repository record was created leaves the row in place with its + // own error indicator instead (see RepositoryCard's error dot), and this + // form has already closed by then. + const [submitError, setSubmitError] = useState(null); + // Bumped on cancel/reopen so a submit's `.then`/`.catch` -- resolving after + // the user has already dismissed or restarted the form -- never applies its + // (now stale) outcome to a different attempt's state. + const submitToken = useRef(0); const reset = (): void => { setUrl(''); setName(''); setNameEdited(false); + setSubmitError(null); }; // Open prefilled when another page requests adding a repo (e.g. an unlinked @@ -44,12 +57,14 @@ export function RepoAddButton() { }, [addRepoRequest, clearAddRepoRequest]); const cancel = (): void => { + submitToken.current += 1; setOpen(false); reset(); }; const onUrlChange = (value: string): void => { setUrl(value); + setSubmitError(null); if (!nameEdited) setName(deriveRepoName(value)); }; @@ -60,20 +75,54 @@ export function RepoAddButton() { const showError = url.trim() !== '' && !valid; const submit = (): void => { - if (!valid) return; - void addRepository(url.trim(), name.trim()); - setOpen(false); - reset(); + if (!valid || submitting) return; + const trimmedUrl = url.trim(); + const trimmedName = name.trim(); + const token = (submitToken.current += 1); + setSubmitting(true); + setSubmitError(null); + // `addRepository` (app/store/store.ts) chains add -> clone -> describe and + // never rejects on a backend failure -- it calls `notify` and resolves. + // The only way to tell success from an add-level failure back here is to + // check, once it settles, whether the row actually landed: `notify`'s + // `set()` calls happen synchronously inside that same async chain, so by + // the time this await resolves the store already reflects the outcome. + const notificationsBefore = useSkillkeeperStore.getState().notifications.length; + void addRepository(trimmedUrl, trimmedName) + .then(() => { + // The user cancelled or restarted the form before this settled -- + // applying its outcome now would stomp a different attempt's state. + if (submitToken.current !== token) return; + const state = useSkillkeeperStore.getState(); + const wasAdded = state.repositories.some((r) => r.url === trimmedUrl); + setSubmitting(false); + if (wasAdded) { + setOpen(false); + reset(); + return; + } + // Not added: the failure is the newest error `notify`d since this + // submit started (the add call notifies with the raw backend error, + // never a rejection -- see the comment above). + const failure = state.notifications.slice(notificationsBefore).find((n) => n.level === 'error'); + setSubmitError(failure !== undefined ? resolveNotification(failure, t) : ''); + }) + .catch((err: unknown) => { + if (submitToken.current !== token) return; + setSubmitting(false); + setSubmitError(err instanceof Error ? err.message : String(err)); + }); }; return ( <> - -
+
onUrlChange(e.target.value)} @@ -91,11 +140,21 @@ export function RepoAddButton() { setName(e.target.value); }} /> + {submitError !== null && ( +

+ {submitError} +

+ )}
-
diff --git a/apps/desktop/src/renderer/pages/Repositories/RepositoriesPage.tsx b/apps/desktop/src/renderer/pages/Repositories/RepositoriesPage.tsx index ca2fe070..e420c9f8 100644 --- a/apps/desktop/src/renderer/pages/Repositories/RepositoriesPage.tsx +++ b/apps/desktop/src/renderer/pages/Repositories/RepositoriesPage.tsx @@ -148,10 +148,12 @@ export function RepositoriesPage() { } > {repositories.length === 0 ? ( -

{t('repositories.empty')}

+

+ {t('repositories.empty')} +

) : ( <> -
+
{filtered.map((r, i) => ( void; readonly className?: string; + /** Test id for driving navigation from an E2E spec (e.g. `nav-repositories`). */ + readonly 'data-testid'?: string; } -export function SidebarItem({ icon, children, active, onClick, className }: SidebarItemProps) { +export function SidebarItem({ + icon, + children, + active, + onClick, + className, + 'data-testid': testId, +}: SidebarItemProps) { return ( -
diff --git a/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx b/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx index 3c20381b..01b24413 100644 --- a/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx +++ b/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx @@ -209,7 +209,7 @@ export function SkillsComponentsPage() { , - , ] @@ -253,52 +253,58 @@ export function SkillsComponentsPage() { } dock={dock} > - {baseTree.length === 0 ? ( -

{t('skills.emptyRepositories')}

- ) : ( - <> - - setRepoChecked([ - ...applyCheckChange( - { explicit: repoChecked, restored: NO_RESTORED }, - NO_BASELINE, - graph, - selection.shown, - next, - ).explicit, - ]) - } - defaultExpandedIds={expandedIds} - onExpandedChange={(ids) => setSkillsUi({ expandedIds: ids })} - ariaLabel={t('skills.componentsTitle')} - /> - {(searching || filtering) && ( -
- {searching && ( - setQuery('')} - /> - )} - {filtering && ( -
- -
- )} -
- )} - - )} + {/* e2e (flows 3/11, `skills.spec.ts`): a stable anchor for "the Skills + Components page is showing" -- see `ManagementPage.tsx`'s matching + `skills-page` wrapper for why this is one wrapper rather than one + testid per branch. */} +
+ {baseTree.length === 0 ? ( +

{t('skills.emptyRepositories')}

+ ) : ( + <> + + setRepoChecked([ + ...applyCheckChange( + { explicit: repoChecked, restored: NO_RESTORED }, + NO_BASELINE, + graph, + selection.shown, + next, + ).explicit, + ]) + } + defaultExpandedIds={expandedIds} + onExpandedChange={(ids) => setSkillsUi({ expandedIds: ids })} + ariaLabel={t('skills.componentsTitle')} + /> + {(searching || filtering) && ( +
+ {searching && ( + setQuery('')} + /> + )} + {filtering && ( +
+ +
+ )} +
+ )} + + )} +
setInstallOpen(false)} skillKeys={repoChecked} /> ); diff --git a/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx b/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx index dde83b46..51883924 100644 --- a/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx +++ b/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx @@ -716,57 +716,64 @@ export function SkillsManagementPage() { } dock={dock} > - {/* An empty tree has two causes now that the Global root can be filtered - out too (before this it was always present, so `baseTree` was never - empty): nothing is tracked at all, or the filters excluded everything - that is. Only the first is "no projects tracked yet"; the second must - say so and carry its own reset, since the footer that normally holds - one is inside the non-empty branch. */} - {baseTree.length === 0 ? ( - filtering ? ( -
-

{t('skills.emptyFiltered')}

- -
- ) : ( -

{t('skills.emptyProjects')}

- ) - ) : ( - <> - setSkillsUi({ expandedIds: ids })} - ariaLabel={t('skills.managementTitle')} - /> - {(searching || filtering) && ( -
- {searching && ( - setQuery('')} - /> - )} - {filtering && ( -
- -
- )} + {/* e2e (flow 2, `skills.spec.ts`): a stable anchor for "the Skills + Management page is showing", mirroring `repositories-page` -- one + wrapper around both branches below rather than one testid per + branch, since (unlike Repositories) there is no single element common + to both that would otherwise need it. */} +
+ {/* An empty tree has two causes now that the Global root can be filtered + out too (before this it was always present, so `baseTree` was never + empty): nothing is tracked at all, or the filters excluded everything + that is. Only the first is "no projects tracked yet"; the second must + say so and carry its own reset, since the footer that normally holds + one is inside the non-empty branch. */} + {baseTree.length === 0 ? ( + filtering ? ( +
+

{t('skills.emptyFiltered')}

+
- )} - - )} + ) : ( +

{t('skills.emptyProjects')}

+ ) + ) : ( + <> + setSkillsUi({ expandedIds: ids })} + ariaLabel={t('skills.managementTitle')} + /> + {(searching || filtering) && ( +
+ {searching && ( + setQuery('')} + /> + )} + {filtering && ( +
+ +
+ )} +
+ )} + + )} +
= { ), }; -export function ChangeBadge({ kind, label, onClick, tabIndex, className }: ChangeBadgeProps) { +export function ChangeBadge({ kind, label, onClick, tabIndex, className, 'data-testid': testId }: ChangeBadgeProps) { // Unique per instance so multiple badges never collide on the mask id. const maskId = `sk-change-badge-${useId().replace(/[^a-zA-Z0-9]/g, '')}`; const glyph = ( @@ -94,7 +98,7 @@ export function ChangeBadge({ kind, label, onClick, tabIndex, className }: Chang return ( {onClick === undefined ? ( - + {glyph} ) : ( @@ -103,6 +107,7 @@ export function ChangeBadge({ kind, label, onClick, tabIndex, className }: Chang tabIndex={tabIndex} className={cx(classes, 'sk-change-badge--button')} aria-label={label} + data-testid={testId} onClick={(e) => { // The badge owns this click; the row behind it must not also act on // it (e.g. a TreeView leaf row toggles its checkbox on click). diff --git a/apps/desktop/src/renderer/shared/ui/Modal/Modal.tsx b/apps/desktop/src/renderer/shared/ui/Modal/Modal.tsx index 6065facd..f66688f6 100644 --- a/apps/desktop/src/renderer/shared/ui/Modal/Modal.tsx +++ b/apps/desktop/src/renderer/shared/ui/Modal/Modal.tsx @@ -31,6 +31,9 @@ export interface ModalProps { readonly title?: ReactNode; readonly children?: ReactNode; readonly className?: string; + /** Test id for the dialog element. Generic passthrough -- Modal has no + * product knowledge of it, a caller sets it for the flows that need it. */ + readonly 'data-testid'?: string; } // Height of a top/bottom fade block when that edge has hidden content. @@ -55,7 +58,7 @@ function updateFades(viewport: HTMLDivElement, scrim: HTMLDivElement): void { scrim.style.setProperty('--sk-modal-fade-bottom', hasHiddenBottom ? `${FADE_PX}px` : '0px'); } -export function Modal({ open, onClose, title, children, className }: ModalProps) { +export function Modal({ open, onClose, title, children, className, 'data-testid': testId }: ModalProps) { const scrimRef = useRef(null); const viewportRef = useRef(null); const dialogRef = useRef(null); @@ -119,6 +122,7 @@ export function Modal({ open, onClose, title, children, className }: ModalProps) animate="animate" exit="exit" onClick={(e) => e.stopPropagation()} + data-testid={testId} > {title !== undefined &&
{title}
}
{children}
diff --git a/apps/desktop/src/renderer/shared/ui/ProgressBar/ProgressBar.tsx b/apps/desktop/src/renderer/shared/ui/ProgressBar/ProgressBar.tsx index 1b68b5b8..f5b5cd71 100644 --- a/apps/desktop/src/renderer/shared/ui/ProgressBar/ProgressBar.tsx +++ b/apps/desktop/src/renderer/shared/ui/ProgressBar/ProgressBar.tsx @@ -11,9 +11,13 @@ export interface ProgressBarProps { /** Accessible label. */ readonly label?: string; readonly className?: string; + /** Test id for the progressbar element. Generic passthrough -- ProgressBar + * has no product knowledge of it, a caller sets it for the flows that need + * it (e.g. to read `aria-valuenow` once a scripted progress event lands). */ + readonly 'data-testid'?: string; } -export function ProgressBar({ value, label, className }: ProgressBarProps) { +export function ProgressBar({ value, label, className, 'data-testid': testId }: ProgressBarProps) { const indeterminate = value === undefined; const pct = indeterminate ? 0 : Math.max(0, Math.min(1, value)) * 100; return ( @@ -24,6 +28,7 @@ export function ProgressBar({ value, label, className }: ProgressBarProps) { aria-valuenow={indeterminate ? undefined : Math.round(pct)} aria-valuemin={indeterminate ? undefined : 0} aria-valuemax={indeterminate ? undefined : 100} + data-testid={testId} >
diff --git a/apps/desktop/src/renderer/shared/ui/TreeView/TreeView.tsx b/apps/desktop/src/renderer/shared/ui/TreeView/TreeView.tsx index ea034ddb..b07b57b1 100644 --- a/apps/desktop/src/renderer/shared/ui/TreeView/TreeView.tsx +++ b/apps/desktop/src/renderer/shared/ui/TreeView/TreeView.tsx @@ -54,6 +54,22 @@ export interface TreeNode { readonly selectable?: boolean; /** Dim the row (e.g. an orphaned skill whose source is gone). */ readonly muted?: boolean; + /** + * Test id for this row (a KIND, e.g. `skill-row` -- never a per-instance + * value). Generic passthrough: TreeView has no product knowledge of what it + * means, a caller (e.g. `entities/skill`'s tree builders) sets it only for + * the nodes an e2e flow needs to reach. Rendered on the `treeitem` element, + * which already carries `aria-checked` in checkbox mode, so a specific row + * found this way can also be asserted checked/unchecked directly. + */ + readonly rowTestId?: string; + /** + * An identity data-attribute for this row (e.g. `{ attr: 'skill-id', value: + * 'lint-basic' }` renders `data-skill-id="lint-basic"`), placed on the row's + * label -- a CHILD of the row, never the row itself, per the e2e + * identity-is-a-separate-attribute convention. Generic, like `rowTestId`. + */ + readonly identity?: { readonly attr: string; readonly value: string }; } export interface TreeViewProps { @@ -422,6 +438,10 @@ export function TreeView({ const labelText = typeof node.label === 'string' ? node.label : undefined; if (isOpen) everOpened.current.add(node.id); const mountChildren = isOpen || everOpened.current.has(node.id); + // See the TreeNode doc comment: both are markup-only passthroughs with no + // meaning to TreeView itself. + const identityAttrs: Record = + node.identity !== undefined ? { [`data-${node.identity.attr}`]: node.identity.value } : {}; return (
  • { @@ -478,7 +499,7 @@ export function TreeView({
  • ` sits INSIDE + * its parent group's `
  • `), so `.filter({ has: ... })` also matches the + * parent -- it contains a descendant with that attribute too. Walking up from + * the identity attribute is unambiguous regardless of nesting depth. + */ +function groupRow(page: Page, groupId: string) { + return page.locator(`[data-group-id="${groupId}"]`).locator('xpath=ancestor::*[@data-testid="skill-group"][1]'); +} + +test.describe('browsing skills', () => { + test.use({ scenario: flatAndGrouped() }); + + test('the tree lists a flat skill, a group, and a nested group', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-skills').click(); + await page.getByTestId('nav-skills-management').click(); + await expect(page.getByTestId('skills-page')).toBeVisible(); + + // The repository root has no dedicated test id (no flow needs to select it + // by identity); it is the one branch labeled with the repository's own + // name, and expanding it is what reveals the flat skill and the group. + await page.getByText('skills-repo', { exact: true }).click(); + + const flatSkill = page.getByTestId('skill-row').filter({ has: page.locator('[data-skill-id="flat-skill"]') }); + await expect(flatSkill).toBeVisible(); + + const group = groupRow(page, 'platform'); + await expect(group).toBeVisible(); + + // Expanding the group reveals the nested group underneath it. + await group.click(); + const nestedGroup = groupRow(page, 'platform/lint'); + await expect(nestedGroup).toBeVisible(); + }); +}); + +test.describe('installing a skill', () => { + test.use({ scenario: installable() }); + + test('checking a skill and applying it drives progress to completion', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-skills').click(); + await page.getByTestId('nav-skills-components').click(); + await expect(page.getByTestId('skills-page')).toBeVisible(); + + // Checking the skill row (a leaf click toggles its checkbox -- see + // `TreeView`'s `activateRow`) reveals the dock's "Install" button. + const row = page.getByTestId('skill-row').filter({ has: page.locator('[data-skill-id="installable-skill"]') }); + await row.click(); + + await page.getByTestId('skill-install-open').click(); + await expect(page.getByTestId('skill-install-modal')).toBeVisible(); + + // Step 1: pick the project. `projects_detect_agents` (mocked by the + // scenario) auto-fills the agent selection, so there is nothing else to + // drive before "Next" is enabled. + await page.getByRole('combobox', { name: 'Project' }).click(); + await page.getByRole('option', { name: 'Demo' }).click(); + await page.getByRole('button', { name: 'Next' }).click(); + + // Step 2: the picked skill is already checked -- seeded from the page's + // hand pick (`seedInstallSelection`). + const checkbox = page + .getByTestId('skill-install-checkbox') + .filter({ has: page.locator('[data-skill-id="installable-skill"]') }); + await expect(checkbox).toHaveAttribute('aria-checked', 'true'); + + // Save is a double-confirm: the first click only arms it. + await page.getByTestId('skill-install-submit').click(); + + // The confirming click, the `skills:progress` emit, and the read of its + // effect all happen inside ONE `page.evaluate` rather than as separate + // `app`-fixture calls. This is not stylistic: `applySkills` (`app/store/ + // store.ts`) awaits the scripted `skills_apply` then `skills_list`, both + // of which the harness resolves within a couple of microtask ticks (see + // `harness/installHarness.ts`'s synchronous mock callback) -- an order of + // magnitude faster than a second, separate Playwright round trip can + // land. Confirmed empirically: a standalone `app.emit` call issued after + // an already-awaited confirming click always arrives once the apply has + // already resolved and the modal has already closed, so there is no + // listener left to receive it. Sequencing by microtask ticks + // (`await Promise.resolve()`) inside one script is deterministic JS + // ordering, not a timing guess -- unlike a real wait, it cannot be + // "almost long enough". The emitted event is dispatched through the exact + // same `plugin:event|emit` invoke call `harness/fixture.ts`'s `app.emit` + // uses, so this exercises the identical wire path. + const resultValueNow = await page.evaluate(async () => { + const submit = document.querySelector('[data-testid="skill-install-submit"]') as HTMLButtonElement; + submit.click(); + // One tick for `applySkills`'s synchronous `set({ skillApply: ... })` + // (made before its first await) to reach a committed render. + await Promise.resolve(); + const internals = ( + window as unknown as { + __TAURI_INTERNALS__: { invoke: (cmd: string, args: unknown) => Promise }; + } + ).__TAURI_INTERNALS__; + await internals.invoke('plugin:event|emit', { + event: 'skills:progress', + payload: { done: 1, total: 1 }, + }); + // One more tick for the emitted update to commit before the apply's own + // (already in-flight) resolution clears it again. + await Promise.resolve(); + return document.querySelector('[data-testid="skill-install-result"]')?.getAttribute('aria-valuenow') ?? null; + }); + + expect(resultValueNow).toBe('100'); + }); +}); + +test.describe('a skill with dependencies', () => { + test.use({ scenario: withDependency() }); + + test('checking a skill also selects its dependency, marked required', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-skills').click(); + await page.getByTestId('nav-skills-components').click(); + await expect(page.getByTestId('skills-page')).toBeVisible(); + + const dependent = page + .getByTestId('skill-row') + .filter({ has: page.locator('[data-skill-id="needs-dependency"]') }); + await dependent.click(); + + await page.getByTestId('skill-install-open').click(); + await page.getByRole('combobox', { name: 'Project' }).click(); + await page.getByRole('option', { name: 'Demo' }).click(); + await page.getByRole('button', { name: 'Next' }).click(); + + const dependencyRow = page + .getByTestId('skill-install-checkbox') + .filter({ has: page.locator('[data-skill-id="depended-on-skill"]') }); + await expect(dependencyRow).toHaveAttribute('aria-checked', 'true'); + await expect(dependencyRow.getByTestId('skill-install-required-badge')).toBeVisible(); + }); +}); From e1ca53297a483ebf5d026d94cc8dabb527b32f83 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 03:05:11 +0200 Subject: [PATCH 10/25] fix(desktop): keep skills-page layout inert, tighten flows 3/11 --- .../skillInstall/ui/SkillInstallModal.tsx | 13 ++++-- .../renderer/pages/Skills/ComponentsPage.tsx | 6 ++- .../renderer/pages/Skills/ManagementPage.tsx | 8 +++- .../src/renderer/pages/Skills/SkillsPage.scss | 18 ++++++++ e2e/desktop/fixtures/base.ts | 29 ++++++++++++ e2e/desktop/tests/skills.spec.ts | 46 +++++++++++++++++-- 6 files changed, 107 insertions(+), 13 deletions(-) diff --git a/apps/desktop/src/renderer/features/skillInstall/ui/SkillInstallModal.tsx b/apps/desktop/src/renderer/features/skillInstall/ui/SkillInstallModal.tsx index fe106adc..f6e4cab3 100644 --- a/apps/desktop/src/renderer/features/skillInstall/ui/SkillInstallModal.tsx +++ b/apps/desktop/src/renderer/features/skillInstall/ui/SkillInstallModal.tsx @@ -266,14 +266,19 @@ export function SkillInstallModal({ open, onClose, skillKeys }: SkillInstallModa />
    • {busy && progress !== null && ( + // e2e (flow 3, `skills.spec.ts`): the section a `skills:progress` + // event driven through `app.emit` lands in -- the spec reads the + // nested `ProgressBar`'s `aria-valuenow` (via its `role`, not a + // second test id: one id naming this whole section is enough, and + // a separate `skill-install-result` id here would just name the + // same element twice under two ids for two halves of one check). + // The FLOW's actual result -- did the install succeed -- is + // `skill-install-modal` becoming hidden once `save()` resolves; + // this section only proves the emitted event was received.
    0 ? progress.done / progress.total : undefined} label={t('skills.install.installing')} - // e2e (flow 3, `skills.spec.ts`): asserts the result of a - // `skills:progress` event driven through `app.emit` via - // `aria-valuenow`, once the spec's {done, total} lands here. - data-testid="skill-install-result" /> {progress.label}
    diff --git a/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx b/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx index 01b24413..9bcb07f0 100644 --- a/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx +++ b/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx @@ -256,8 +256,10 @@ export function SkillsComponentsPage() { {/* e2e (flows 3/11, `skills.spec.ts`): a stable anchor for "the Skills Components page is showing" -- see `ManagementPage.tsx`'s matching `skills-page` wrapper for why this is one wrapper rather than one - testid per branch. */} -
    + testid per branch. `sk-skills-page-body` (SkillsPage.scss) replicates + `Page`'s own `.sk-page__body` flex layout so this wrapper is + transparent to rendering -- see that class's own doc comment. */} +
    {baseTree.length === 0 ? (

    {t('skills.emptyRepositories')}

    ) : ( diff --git a/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx b/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx index 51883924..194e74ea 100644 --- a/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx +++ b/apps/desktop/src/renderer/pages/Skills/ManagementPage.tsx @@ -720,8 +720,12 @@ export function SkillsManagementPage() { Management page is showing", mirroring `repositories-page` -- one wrapper around both branches below rather than one testid per branch, since (unlike Repositories) there is no single element common - to both that would otherwise need it. */} -
    + to both that would otherwise need it. `sk-skills-page-body` + (SkillsPage.scss) replicates `Page`'s own `.sk-page__body` flex + layout so this wrapper is transparent to rendering -- a test id must + never change what renders; see that class's own doc comment (it also + explains why `.sk-list-footer`'s bottom-pinning depended on this). */} +
    {/* An empty tree has two causes now that the Global root can be filtered out too (before this it was always present, so `baseTree` was never empty): nothing is tracked at all, or the filters excluded everything diff --git a/apps/desktop/src/renderer/pages/Skills/SkillsPage.scss b/apps/desktop/src/renderer/pages/Skills/SkillsPage.scss index 1c037ec3..2da5ceef 100644 --- a/apps/desktop/src/renderer/pages/Skills/SkillsPage.scss +++ b/apps/desktop/src/renderer/pages/Skills/SkillsPage.scss @@ -33,6 +33,24 @@ min-width: 0; } +// The e2e `skills-page` test id (ManagementPage.tsx, ComponentsPage.tsx) sits +// on a wrapper `
    ` around the page body's content (empty state, or tree + +// footer) -- introduced only to give both branches one common anchor. That +// wrapper is a new element in what was `Page`'s own `.sk-page__body` flex +// context (`Page.scss`): without this class it becomes a plain block box, and +// `.sk-list-footer`'s `margin-top: auto` (below) stops pinning the footer to +// the bottom of a short tree, since `margin: auto` only resolves against a +// flex/grid container. Replicates `.sk-page__body`'s own flex properties +// exactly (not its padding, which stays on the real `.sk-page__body` parent) +// so the wrapper is transparent to layout -- a test id must never change what +// renders. +.sk-skills-page-body { + display: flex; + flex-direction: column; + gap: var(--sk-space-5); + flex: 1 0 auto; +} + // Spacer between the (sticky) header and the tree. .sk-skills-tree { margin-top: var(--sk-space-4); diff --git a/e2e/desktop/fixtures/base.ts b/e2e/desktop/fixtures/base.ts index c6094fc1..f10a8740 100644 --- a/e2e/desktop/fixtures/base.ts +++ b/e2e/desktop/fixtures/base.ts @@ -40,6 +40,35 @@ export interface App { * `listen()`/`emit()` use internally (see `installHarness.ts`'s * `shouldMockEvents` note) -- e.g. to drive `skills:progress` at a moment * the spec chooses, not whenever a real backend operation would have. + * + * LIMITATION, found by Task 5: this cannot land inside a window bounded by a + * SYNCHRONOUSLY-resolving mocked command. `installHarness.ts`'s mocked + * `invoke` (the real `@tauri-apps/api/mocks`, read from its own `.cjs` + * source) is a plain synchronous callback wrapped in an `async` function + * with no internal `await` -- so a store action chaining two or three such + * calls (e.g. `applySkills`'s `skills_apply` then `skills_list`) resolves + * end to end within a single microtask drain, faster than a second, + * separate Playwright round trip (this method included) can ever arrive: by + * the time its own `page.evaluate` call reaches the page, the listener the + * spec meant to reach has usually already been unregistered. Confirmed with + * a four-point diagnostic (`page.evaluate` sampling DOM state after zero, + * one, and several microtask/macrotask ticks) before concluding this, not + * assumed. There is no way to widen that window from a scenario today -- + * `Scenario.responses` values are plain, already-resolved data (see this + * file's own doc comment on why), not a deferred/gated response a spec + * could release on demand. + * + * Worked example / the only known way around it today: + * `e2e/desktop/tests/skills.spec.ts`'s "installing a skill" test inlines the + * exact same `plugin:event|emit` invoke call this method makes, sequenced + * against the triggering click by microtask ticks (`await + * Promise.resolve()`) inside ONE `page.evaluate` -- deterministic JS + * ordering, not a timing guess. Read that spec before reaching for a + * standalone `app.emit` call anywhere a store action might resolve this + * fast (the MCP install flow's `applyMcp`/`updateMcp` are likely candidates: + * same synchronous-mock shape). A proper fix -- a scenario-level + * gated/deferred response a spec can release on demand -- is intentionally + * NOT built here; it is scoped as its own task. */ emit(name: string, payload: unknown): Promise; /** Every recorded invocation of `command`, as its `args`, in call order. diff --git a/e2e/desktop/tests/skills.spec.ts b/e2e/desktop/tests/skills.spec.ts index ca3d5e75..15ac2b56 100644 --- a/e2e/desktop/tests/skills.spec.ts +++ b/e2e/desktop/tests/skills.spec.ts @@ -102,8 +102,12 @@ test.describe('installing a skill', () => { // ordering, not a timing guess -- unlike a real wait, it cannot be // "almost long enough". The emitted event is dispatched through the exact // same `plugin:event|emit` invoke call `harness/fixture.ts`'s `app.emit` - // uses, so this exercises the identical wire path. - const resultValueNow = await page.evaluate(async () => { + // uses (see that file's doc comment, which now records this limit), so + // this exercises the identical wire path. The payload matches the + // authoritative `ApplyProgress` shape (`contracts.ts`) in full, including + // `label` -- the modal renders it, so an approximated payload would be a + // silent product-code path this spec never actually exercises. + const progressValueNow = await page.evaluate(async () => { const submit = document.querySelector('[data-testid="skill-install-submit"]') as HTMLButtonElement; submit.click(); // One tick for `applySkills`'s synchronous `set({ skillApply: ... })` @@ -116,15 +120,29 @@ test.describe('installing a skill', () => { ).__TAURI_INTERNALS__; await internals.invoke('plugin:event|emit', { event: 'skills:progress', - payload: { done: 1, total: 1 }, + payload: { done: 1, total: 1, label: 'Installing installable-skill' }, }); // One more tick for the emitted update to commit before the apply's own // (already in-flight) resolution clears it again. await Promise.resolve(); - return document.querySelector('[data-testid="skill-install-result"]')?.getAttribute('aria-valuenow') ?? null; + return ( + document + .querySelector('[data-testid="skill-install-progress"] [role="progressbar"]') + ?.getAttribute('aria-valuenow') ?? null + ); }); - expect(resultValueNow).toBe('100'); + // Proves the emitted event was received: the progress section reflects + // the {done: 1, total: 1} the spec pushed, not whatever `skills_apply`'s + // own (much faster) resolution would have shown on its own. + expect(progressValueNow).toBe('100'); + + // The flow's actual result -- a successful apply -- is the modal closing + // (`SkillInstallModal.save()` calls `onClose()` only once every op's + // `applySkills` call resolves `ok: true`). Web-first: by now the apply has + // long since settled (a real round trip past the evaluate above), so this + // is asserting a stable end state, not racing a transient one. + await expect(page.getByTestId('skill-install-modal')).toBeHidden(); }); }); @@ -152,5 +170,23 @@ test.describe('a skill with dependencies', () => { .filter({ has: page.locator('[data-skill-id="depended-on-skill"]') }); await expect(dependencyRow).toHaveAttribute('aria-checked', 'true'); await expect(dependencyRow.getByTestId('skill-install-required-badge')).toBeVisible(); + + // The design doc's rule for this flow (`installSelection.ts`'s own header + // comment) is that the apply plan is built from the DERIVED checked set, + // never the user's hand pick alone -- the mistake that would draw the + // dependency correctly and then install none of it. `needs-dependency` + // alone is the only hand pick (`skillKeys`); if the plan were built from + // that instead of `derived.shown`, `depended-on-skill` would be silently + // missing from `skills_apply`'s `install` list below. Submitting and + // reading the actually-recorded call is what catches that regression -- + // asserting the checkbox/badge above only proves the tree DRAWS the + // dependency, not that applying it INSTALLS the dependency too. + await page.getByTestId('skill-install-submit').click(); + await page.getByTestId('skill-install-submit').click(); + + const calls = await app.calls('skills_apply'); + expect(calls).toHaveLength(1); + const { install } = (calls[0] as { args: { install: readonly { name: string }[] } }).args; + expect(install.map((ref) => ref.name).sort()).toEqual(['depended-on-skill', 'needs-dependency']); }); }); From df26bbb6b63f27cb448e18858cf2114af590836c Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 03:15:24 +0200 Subject: [PATCH 11/25] test: cover tracking a project and a missing folder --- .../entities/project/ui/ProjectCard.tsx | 16 +++- .../projectAdd/ui/ProjectAddButton.tsx | 2 +- .../renderer/pages/Projects/ProjectsPage.tsx | 2 +- .../src/renderer/shared/ui/Badge/Badge.tsx | 16 +++- .../src/renderer/shared/ui/Card/Card.tsx | 7 +- e2e/desktop/fixtures/projects.ts | 74 +++++++++++++++++++ e2e/desktop/tests/projects.spec.ts | 39 ++++++++++ 7 files changed, 146 insertions(+), 10 deletions(-) create mode 100644 e2e/desktop/fixtures/projects.ts create mode 100644 e2e/desktop/tests/projects.spec.ts diff --git a/apps/desktop/src/renderer/entities/project/ui/ProjectCard.tsx b/apps/desktop/src/renderer/entities/project/ui/ProjectCard.tsx index 434b449c..486b3b92 100644 --- a/apps/desktop/src/renderer/entities/project/ui/ProjectCard.tsx +++ b/apps/desktop/src/renderer/entities/project/ui/ProjectCard.tsx @@ -101,7 +101,7 @@ export function ProjectCard({ }: ProjectCardProps) { const washHue = hueFromName(project.name); return ( - + {/* Decorative left wash: a blurred, scaled copy of the project icon when there is one, else a soft colour field keyed to the project name. It fades to transparent toward the centre. The name-keyed gradient is @@ -135,7 +135,9 @@ export function ProjectCard({
    - {truncateEnd(project.name, NAME_MAX)} + + {truncateEnd(project.name, NAME_MAX)} + {missing === true && ( - + )} @@ -191,7 +197,9 @@ export function ProjectCard({ )} {agentsLabel !== undefined && ( - {agentsLabel} + + {agentsLabel} + )} diff --git a/apps/desktop/src/renderer/features/projectAdd/ui/ProjectAddButton.tsx b/apps/desktop/src/renderer/features/projectAdd/ui/ProjectAddButton.tsx index 6c39e9eb..14229df9 100644 --- a/apps/desktop/src/renderer/features/projectAdd/ui/ProjectAddButton.tsx +++ b/apps/desktop/src/renderer/features/projectAdd/ui/ProjectAddButton.tsx @@ -31,7 +31,7 @@ export function ProjectAddButton() { } return ( - ); diff --git a/apps/desktop/src/renderer/pages/Projects/ProjectsPage.tsx b/apps/desktop/src/renderer/pages/Projects/ProjectsPage.tsx index 86f04b04..6ea9f7cb 100644 --- a/apps/desktop/src/renderer/pages/Projects/ProjectsPage.tsx +++ b/apps/desktop/src/renderer/pages/Projects/ProjectsPage.tsx @@ -134,7 +134,7 @@ export function ProjectsPage() { } > -
    +
    + {children} ); diff --git a/apps/desktop/src/renderer/shared/ui/Card/Card.tsx b/apps/desktop/src/renderer/shared/ui/Card/Card.tsx index 845f11fd..7de7a632 100644 --- a/apps/desktop/src/renderer/shared/ui/Card/Card.tsx +++ b/apps/desktop/src/renderer/shared/ui/Card/Card.tsx @@ -12,14 +12,17 @@ export interface CardProps { /** Use a translucent glass surface instead of the solid one. */ readonly glass?: boolean; readonly className?: string; + /** Test id for the card element. Generic passthrough -- Card has no product + * knowledge of it, a caller sets it for the flows that need it. */ + readonly 'data-testid'?: string; } -export function Card({ children, glass, className }: CardProps) { +export function Card({ children, glass, className, 'data-testid': testId }: CardProps) { const ref = useRef(null); // Refract the backdrop when the glass variant is on; no-op otherwise. useGlassRefraction(ref, { enabled: glass === true }); return ( -
    +
    {children}
    ); diff --git a/e2e/desktop/fixtures/projects.ts b/e2e/desktop/fixtures/projects.ts new file mode 100644 index 00000000..3ca4de9a --- /dev/null +++ b/e2e/desktop/fixtures/projects.ts @@ -0,0 +1,74 @@ +/** + * Scenarios for the Projects page (flows 4 and 10 -- see + * `.superpowers/specs/2026-09-09-desktop-ui-e2e-design.md`). + * + * `App`'s `activeView` starts at `'projects'` (see `App.tsx`), so `ProjectsPage` + * mounts on every `app.goto()` with no navigation click needed, unlike the + * Repositories and Skills pages. + */ +import { withScenario } from '../harness/scenario.js'; +import type { Scenario } from '../harness/scenario.js'; +import type { Project } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/index.js'; +import type { ProjectResult, ProjectInfo } from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; + +/** + * Flow 4: tracking a project. Neither `projects_add`, `projects_describe`, nor + * `editors_list` has a default answer in `harness/commands.ts` (none is part + * of `store.loadAll`'s startup round trip), so this scenario supplies all + * three, plus `dialog_select_folder` -- the native folder picker + * `ProjectAddButton` awaits before it ever calls `addProject` + * (`bridgeClient.selectFolder`). `editors_list` is needed because the added + * project's card is not `missing`, so `OpenProjectButton` mounts and calls it + * (`ProjectCard.tsx`'s actions column renders `openControl` only when the + * folder is not missing). `agentCount: 2` is set so the added card's agent + * badge (`project-card-agents`) actually renders -- `ProjectCard` only shows + * it when `agentCount > 0`. + */ +export function trackable(): Scenario { + const project: Project = { + id: 'tracked-project-id', + path: '/projects/demo-project', + name: 'Demo Project', + addedAt: '2024-01-01T00:00:00Z', + }; + const added: ProjectResult = { ok: true, project }; + const info: ProjectInfo = { skillCount: 0, fromReposCount: 0, agentCount: 2 }; + return withScenario({ + responses: { + dialog_select_folder: project.path, + projects_add: added, + projects_describe: info, + editors_list: [], + }, + }); +} + +/** + * Flow 10: a tracked project whose folder no longer exists. The project is + * seeded up front (`scenario.projects`), exactly as a normal page load would + * show it; `projects_folder_state` answers `missing` for it, matching what + * `useProjectCheckSchedule`'s startup sweep (`store.checkProjects`) polls for + * every tracked project. `projects_describe` still needs an answer -- + * `ProjectsPage`'s mount effect (`refreshProjectInfo`) describes every project + * regardless of folder state -- but `editors_list` is deliberately NOT + * mocked: `ProjectCard` never renders `OpenProjectButton` while a project is + * `missing` (see `ProjectCard.tsx`'s actions column, which shows only the + * remove button in that case), so the app never calls it for this scenario, + * and mocking it anyway would hide that fact. + */ +export function folderMissing(): Scenario { + const project: Project = { + id: 'missing-project-id', + path: '/projects/gone', + name: 'Gone Project', + addedAt: '2024-01-01T00:00:00Z', + }; + const info: ProjectInfo = { skillCount: 0, fromReposCount: 0, agentCount: 0 }; + return withScenario({ + projects: [project], + responses: { + projects_describe: info, + projects_folder_state: 'missing', + }, + }); +} diff --git a/e2e/desktop/tests/projects.spec.ts b/e2e/desktop/tests/projects.spec.ts new file mode 100644 index 00000000..fa60845b --- /dev/null +++ b/e2e/desktop/tests/projects.spec.ts @@ -0,0 +1,39 @@ +/** + * Flows 4 and 10 (Projects page): tracking a project, and a project whose + * folder has gone missing. See `.superpowers/specs/2026-09-09-desktop-ui-e2e- + * design.md`'s "The flows" section. + * + * Projects is the default view (`App.tsx`'s `activeView` starts at + * 'projects'), so unlike the Repositories and Skills specs, neither test + * navigates before asserting. + */ +import { test, expect } from '../harness/fixture'; +import { trackable, folderMissing } from '../fixtures/projects'; + +test.describe('tracking a project', () => { + test.use({ scenario: trackable() }); + + test('adding a project shows its card with agent badges', async ({ app, page }) => { + await app.goto(); + await expect(page.getByTestId('projects-page')).toBeVisible(); + await page.getByTestId('project-add-button').click(); + const card = page + .getByTestId('project-card') + .filter({ has: page.locator('[data-project-id="tracked-project-id"]') }); + await expect(card).toBeVisible(); + await expect(card.getByTestId('project-card-agents')).toBeVisible(); + }); +}); + +test.describe('a project whose folder is missing', () => { + test.use({ scenario: folderMissing() }); + + test('a missing folder shows the folder-missing affordance, not a generic error', async ({ app, page }) => { + await app.goto(); + const card = page + .getByTestId('project-card') + .filter({ has: page.locator('[data-project-id="missing-project-id"]') }); + await expect(card).toBeVisible(); + await expect(card.getByTestId('project-card-folder-missing')).toBeVisible(); + }); +}); From 34b303b1398a81caec56f21a1b5dbb4ea91f93a3 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 03:31:06 +0200 Subject: [PATCH 12/25] test: cover the settings page and an offered update --- apps/desktop/src/renderer/app/App.tsx | 1 + .../renderer/pages/Settings/SettingsPage.tsx | 15 ++-- .../renderer/shared/ui/Form/FormSection.tsx | 27 +++++- .../appUpdate/ui/AppUpdateCheckButton.tsx | 7 +- .../appUpdate/ui/UpdateReadyDialog.tsx | 4 +- e2e/desktop/fixtures/settings.ts | 84 +++++++++++++++++++ e2e/desktop/tests/settings.spec.ts | 69 +++++++++++++++ 7 files changed, 197 insertions(+), 10 deletions(-) create mode 100644 e2e/desktop/fixtures/settings.ts create mode 100644 e2e/desktop/tests/settings.spec.ts diff --git a/apps/desktop/src/renderer/app/App.tsx b/apps/desktop/src/renderer/app/App.tsx index 4daf09cc..62fda2e9 100644 --- a/apps/desktop/src/renderer/app/App.tsx +++ b/apps/desktop/src/renderer/app/App.tsx @@ -402,6 +402,7 @@ export function App() { icon={} active={activeView === 'settings'} onClick={() => goTo('settings')} + data-testid="nav-settings" > {t('nav.settings')} diff --git a/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx b/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx index f8a6609a..70868334 100644 --- a/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx +++ b/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx @@ -109,11 +109,12 @@ export function SettingsPage() { > - + - + - + - + {showUpdateNow && ( diff --git a/apps/desktop/src/renderer/systems/appUpdate/ui/UpdateReadyDialog.tsx b/apps/desktop/src/renderer/systems/appUpdate/ui/UpdateReadyDialog.tsx index 20d87d07..5ef28736 100644 --- a/apps/desktop/src/renderer/systems/appUpdate/ui/UpdateReadyDialog.tsx +++ b/apps/desktop/src/renderer/systems/appUpdate/ui/UpdateReadyDialog.tsx @@ -96,7 +96,9 @@ export function UpdateReadyDialog({ platform = bridgeClient.platform }: UpdateRe return ( -

    {t('appUpdate.readyBody', { version })}

    +

    + {t('appUpdate.readyBody', { version })} +

    {path !== null &&

    {t('appUpdate.readyPath', { path })}

    }

    {t('appUpdate.readyHint')}

    {showMacFallback && ( diff --git a/e2e/desktop/fixtures/settings.ts b/e2e/desktop/fixtures/settings.ts new file mode 100644 index 00000000..29009559 --- /dev/null +++ b/e2e/desktop/fixtures/settings.ts @@ -0,0 +1,84 @@ +/** + * Scenarios for the Settings page and the self-update surface (flows 5 and 6 + * -- see `.superpowers/specs/2026-09-09-desktop-ui-e2e-design.md`). + */ +import { withScenario } from '../harness/scenario.js'; +import type { Scenario } from '../harness/scenario.js'; +import type { SkillKeeperConfig } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/index.js'; +import type { AppUpdateOffer } from '../../../apps/desktop/src/renderer/services/bridge/generated/AppUpdateOffer.js'; +import type { SshKeyDto } from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; + +/** + * `SettingsPage`'s mount round trip: `OpenConfigButton` (the toolbar's editor + * picker, same lazy-loaded-once pattern as `OpenProjectButton`) reads + * `editors_list`, and the repositories section's `SshKeyField` reads + * `ssh_key_state` -- neither has a default in `harness/commands.ts` (not part + * of `loadAll`'s startup round trip), so any scenario that opens Settings + * must answer both, exactly like `fixtures/skills.ts`'s + * `projectsPageStartupResponses`. + */ +function settingsPageStartupResponses(): Record { + const sshKeyState: SshKeyDto = { state: 'notConfigured' }; + return { + editors_list: [], + ssh_key_state: sshKeyState, + }; +} + +/** + * A config with every section populated by values distinct from + * `harness/scenario.ts`'s own `emptyConfig()` defaults, so a rendered field + * can be told apart from an incidental default. `config_get`'s + * `SectionValidity` (`harness/commands.ts`'s `defaultResponses`) marks every + * section 'valid' regardless of the values here -- that default IS flow 5's + * "all-valid" precondition, kept implicit rather than duplicated in a + * `responses` override. + */ +function config(): SkillKeeperConfig { + return { + general: { language: 'en', theme: 'system', animations: 'fast' }, + updates: { mode: 'scheduled', intervalMinutes: 180, checkOnStartup: false }, + agents: { enabled: ['claude', 'codex', 'copilot', 'cursor', 'opencode'], overrides: {} }, + executables: { globs: [] }, + security: { hookConsentPolicy: 'always-ask' }, + notifications: { enabled: true }, + repositories: { gitPath: '/usr/bin/git' }, + projects: { checkIntervalMinutes: 5 }, + mcp: { servers: [] }, + }; +} + +/** Flow 5: opening Settings renders every section, backed by a config whose + * `SectionValidity` is all-valid (see `config()`'s doc comment). */ +export function settingsPage(): Scenario { + return withScenario({ config: config(), responses: settingsPageStartupResponses() }); +} + +/** + * The offer `app_update_check_now` resolves with once the spec presses + * "Check now" on the Settings page. `showDialog: false` keeps the "update + * available" dialog from auto-opening (`noteAppUpdateOffer` in + * `app/store/store.ts`), so the only overlay this flow drives is the "ready + * to install" dialog, via a plain `appUpdate:ready` emit afterward. + */ +function offer(): AppUpdateOffer { + return { + version: '1.5.0', + bump: 'minor', + notes: '', + truncatedHistory: false, + installable: true, + showDialog: false, + }; +} + +/** Flow 6: `app_update_check_now` returns an offer; the spec then emits + * `appUpdate:ready` to complete it -- see `settings.spec.ts`. */ +export function offeredUpdate(): Scenario { + return withScenario({ + responses: { + ...settingsPageStartupResponses(), + app_update_check_now: offer(), + }, + }); +} diff --git a/e2e/desktop/tests/settings.spec.ts b/e2e/desktop/tests/settings.spec.ts new file mode 100644 index 00000000..2fac5227 --- /dev/null +++ b/e2e/desktop/tests/settings.spec.ts @@ -0,0 +1,69 @@ +/** + * Flows 5 and 6 (Settings and self-update): opening Settings renders every + * section against an all-valid config, and an offered update completes into + * the "ready to install" status once `appUpdate:ready` arrives. See + * `.superpowers/specs/2026-09-09-desktop-ui-e2e-design.md`'s "The flows" + * section. + * + * Settings is a FLAT sidebar item (`nav-settings`), not a group -- unlike + * Skills/MCP it has no sub-items to expand first. + */ +import { test, expect } from '../harness/fixture'; +import { settingsPage, offeredUpdate } from '../fixtures/settings'; + +test.describe('the settings page', () => { + test.use({ scenario: settingsPage() }); + + test('every section renders and reports valid', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-settings').click(); + await expect(page.getByTestId('settings-page')).toBeVisible(); + + // Each section's identity sits on its title (a CHILD of the + // `settings-section` container), never on the container itself -- see + // `FormSection`'s `sectionId` doc comment. + const sectionIds = ['general', 'repositories', 'projects', 'onboarding', 'app-updates']; + for (const id of sectionIds) { + const section = page.getByTestId('settings-section').filter({ has: page.locator(`[data-section-id="${id}"]`) }); + await expect(section).toBeVisible(); + } + + // The scenario's `config_get` reports every section 'valid' (see + // `fixtures/settings.ts`'s `settingsPage`); the user-visible sign of that + // is that the invalid-config banner (`ConfigBanner`, `role="alert"`) + // never appears. + await expect(page.getByRole('alert')).toHaveCount(0); + }); +}); + +test.describe('an offered update', () => { + test.use({ scenario: offeredUpdate() }); + + test('checking now then receiving a ready event surfaces the ready status', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-settings').click(); + await expect(page.getByTestId('settings-page')).toBeVisible(); + + // No status is showing yet -- the "ready to install" dialog only mounts + // its content once `appUpdateReadyOpen` is true (`Modal` renders nothing + // while closed). + await expect(page.getByTestId('app-update-status')).toBeHidden(); + + await page.getByTestId('app-update-check-button').click(); + const calls = await app.calls('app_update_check_now'); + expect(calls).toHaveLength(1); + + // `useAppUpdateSchedule`'s `onAppUpdateReady` subscription is set up once + // for the App's lifetime (an empty-deps effect), not inside the check's + // own resolving chain, so this plain `app.emit` lands normally -- unlike + // `skills.spec.ts`'s "installing a skill" test, there is no transient + // listener window to race here. + await app.emit('appUpdate:ready', { version: '1.5.0', path: '/tmp/SkillKeeper-1.5.0.pkg' }); + + // The offer noted from `app_update_check_now` (not the emitted event's + // own `version`, which the store deliberately ignores -- see + // `useAppUpdateSchedule.ts`) is what the ready dialog renders. + await expect(page.getByTestId('app-update-status')).toBeVisible(); + await expect(page.getByTestId('app-update-status')).toContainText('1.5.0'); + }); +}); From d0c4dc23d3e629de23752b48a9e710aa3639975e Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 03:39:59 +0200 Subject: [PATCH 13/25] fix(e2e): drive config validity from the scenario --- e2e/desktop/fixtures/settings.ts | 34 +++++++++++++++++++++++++++++- e2e/desktop/harness/commands.ts | 14 ++---------- e2e/desktop/harness/scenario.ts | 32 +++++++++++++++++++++++++++- e2e/desktop/tests/settings.spec.ts | 22 +++++++++++++++++-- 4 files changed, 86 insertions(+), 16 deletions(-) diff --git a/e2e/desktop/fixtures/settings.ts b/e2e/desktop/fixtures/settings.ts index 29009559..ac6a3796 100644 --- a/e2e/desktop/fixtures/settings.ts +++ b/e2e/desktop/fixtures/settings.ts @@ -4,10 +4,18 @@ */ import { withScenario } from '../harness/scenario.js'; import type { Scenario } from '../harness/scenario.js'; -import type { SkillKeeperConfig } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/index.js'; +import type { + SkillKeeperConfig, + SectionValidity, +} from '../../../apps/desktop/src/renderer/services/bridge/generated/config/index.js'; import type { AppUpdateOffer } from '../../../apps/desktop/src/renderer/services/bridge/generated/AppUpdateOffer.js'; import type { SshKeyDto } from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; +/** The warning `config_get` reports alongside `invalidSection()`'s + * `repositories: 'invalid'`. Exported so the spec can assert its exact text + * reaches `ConfigBanner` rather than merely asserting the banner exists. */ +export const REPOSITORIES_INVALID_WARNING = 'repositories: gitPath must not be empty'; + /** * `SettingsPage`'s mount round trip: `OpenConfigButton` (the toolbar's editor * picker, same lazy-loaded-once pattern as `OpenProjectButton`) reads @@ -54,6 +62,30 @@ export function settingsPage(): Scenario { return withScenario({ config: config(), responses: settingsPageStartupResponses() }); } +/** The counterpart to `settingsPage()`: same config, but `repositories` is + * reported invalid with one warning -- so the invalid-config banner + * (`ConfigBanner`) has something to actually show, giving the valid case + * above a contrasting negative to be meaningful against. */ +export function invalidSection(): Scenario { + const validity: SectionValidity = { + general: 'valid', + updates: 'valid', + agents: 'valid', + executables: 'valid', + security: 'valid', + notifications: 'valid', + repositories: 'invalid', + projects: 'valid', + mcp: 'valid', + }; + return withScenario({ + config: config(), + validity, + configWarnings: [REPOSITORIES_INVALID_WARNING], + responses: settingsPageStartupResponses(), + }); +} + /** * The offer `app_update_check_now` resolves with once the spec presses * "Check now" on the Settings page. `showDialog: false` keeps the "update diff --git a/e2e/desktop/harness/commands.ts b/e2e/desktop/harness/commands.ts index 25e62539..4887d85f 100644 --- a/e2e/desktop/harness/commands.ts +++ b/e2e/desktop/harness/commands.ts @@ -134,18 +134,8 @@ export function defaultResponses(scenario: Scenario): Record { terminal_start: '', config_get: { config: scenario.config, - validity: { - general: 'valid', - updates: 'valid', - agents: 'valid', - executables: 'valid', - security: 'valid', - notifications: 'valid', - repositories: 'valid', - projects: 'valid', - mcp: 'valid', - }, - warnings: [], + validity: scenario.validity, + warnings: scenario.configWarnings, }, onboarding_get: scenario.onboarding, repositories_list: scenario.repositories, diff --git a/e2e/desktop/harness/scenario.ts b/e2e/desktop/harness/scenario.ts index 1fdd27da..d5d1a69c 100644 --- a/e2e/desktop/harness/scenario.ts +++ b/e2e/desktop/harness/scenario.ts @@ -19,7 +19,7 @@ * enough" fixture; the renderer would reject the same payload from the real * backend. */ -import type { SkillKeeperConfig, OnboardingState } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/index.js'; +import type { SkillKeeperConfig, OnboardingState, SectionValidity } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/index.js'; import type { Repository, Project, InstallManifest } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/index.js'; import type { AvailableSkill, AvailableMcp, McpInstall } from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; @@ -31,6 +31,18 @@ export interface Scenario { * not decoration -- default 'main'. */ readonly windowLabel: string; readonly config: SkillKeeperConfig; + /** Per-section validity `config_get` reports alongside `config` (see + * `harness/commands.ts`'s `defaultResponses`). `store.setConfig` reads + * this straight off the result into `configValidity`, and `ConfigBanner` + * reads it back -- a scenario that wants the invalid-config banner visible + * sets one section here to `'invalid'` (and usually pairs it with a + * `configWarnings` entry, since `ConfigBanner` lists `configWarnings` + * underneath the banner text). */ + readonly validity: SectionValidity; + /** Warnings `config_get` reports alongside `validity` -- one human-readable + * line per invalid section, by convention (see `LoadConfigResult`'s own + * doc comment), though nothing enforces that count here. */ + readonly configWarnings: readonly string[]; readonly onboarding: OnboardingState; readonly repositories: readonly Repository[]; readonly projects: readonly Project[]; @@ -89,6 +101,22 @@ function emptyManifest(): InstallManifest[] { return []; } +/** Every config section reported valid -- the default `config_get` validity, + * matching what a config that parsed cleanly resolves to. */ +function allValidValidity(): SectionValidity { + return { + general: 'valid', + updates: 'valid', + agents: 'valid', + executables: 'valid', + security: 'valid', + notifications: 'valid', + repositories: 'valid', + projects: 'valid', + mcp: 'valid', + }; +} + /** * A scenario with a clean, empty-but-valid backend: no repositories, no * projects, no installs, onboarding already completed (so the guided tour @@ -103,6 +131,8 @@ export function defaultScenario(): Scenario { platform: 'darwin', windowLabel: 'main', config: emptyConfig(), + validity: allValidValidity(), + configWarnings: [], onboarding: { version: 1, completed: true, step: 'done' }, repositories: [], projects: [], diff --git a/e2e/desktop/tests/settings.spec.ts b/e2e/desktop/tests/settings.spec.ts index 2fac5227..af544fde 100644 --- a/e2e/desktop/tests/settings.spec.ts +++ b/e2e/desktop/tests/settings.spec.ts @@ -9,7 +9,7 @@ * Skills/MCP it has no sub-items to expand first. */ import { test, expect } from '../harness/fixture'; -import { settingsPage, offeredUpdate } from '../fixtures/settings'; +import { settingsPage, invalidSection, offeredUpdate, REPOSITORIES_INVALID_WARNING } from '../fixtures/settings'; test.describe('the settings page', () => { test.use({ scenario: settingsPage() }); @@ -31,11 +31,29 @@ test.describe('the settings page', () => { // The scenario's `config_get` reports every section 'valid' (see // `fixtures/settings.ts`'s `settingsPage`); the user-visible sign of that // is that the invalid-config banner (`ConfigBanner`, `role="alert"`) - // never appears. + // never appears. This assertion only has power to fail paired with the + // "an invalid section" test below, which drives the same banner from the + // opposite scenario -- on its own it would pass even if `config_get`'s + // validity never reached the UI at all. await expect(page.getByRole('alert')).toHaveCount(0); }); }); +test.describe('an invalid section', () => { + test.use({ scenario: invalidSection() }); + + test('the config banner reports the invalid section and its warning', async ({ app, page }) => { + await app.goto(); + + // `ConfigBanner` is mounted app-wide (`App.tsx`, alongside `WindowChrome`), + // not scoped to the Settings page, so it is already visible on the + // default Projects view -- no navigation needed for this assertion. + const banner = page.getByRole('alert'); + await expect(banner).toBeVisible(); + await expect(banner).toContainText(REPOSITORIES_INVALID_WARNING); + }); +}); + test.describe('an offered update', () => { test.use({ scenario: offeredUpdate() }); From d56512c9561585d24528ab5c48140b7b37738b2a Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 04:13:05 +0200 Subject: [PATCH 14/25] test: cover installing, updating, and a refused mcp update --- apps/desktop/src/renderer/app/App.tsx | 3 + .../mcpInstall/ui/McpInstallModal.scss | 10 + .../mcpInstall/ui/McpInstallModal.tsx | 30 ++- .../ui/McpUpdateParamsModal.stories.tsx | 29 ++- .../mcpInstall/ui/McpUpdateParamsModal.tsx | 203 ++++++++++++------ .../renderer/pages/Mcp/ManagementPage.scss | 19 ++ .../src/renderer/pages/Mcp/ManagementPage.tsx | 97 +++++---- .../src/renderer/pages/Mcp/lib/mcpTree.tsx | 32 ++- .../src/renderer/pages/Mcp/useMcpActions.tsx | 94 ++++---- .../ui/DescriptionText/DescriptionText.tsx | 8 +- e2e/desktop/fixtures/mcp.ts | 195 +++++++++++++++++ e2e/desktop/tests/mcp.spec.ts | 143 ++++++++++++ 12 files changed, 703 insertions(+), 160 deletions(-) create mode 100644 e2e/desktop/fixtures/mcp.ts create mode 100644 e2e/desktop/tests/mcp.spec.ts diff --git a/apps/desktop/src/renderer/app/App.tsx b/apps/desktop/src/renderer/app/App.tsx index 62fda2e9..98750c34 100644 --- a/apps/desktop/src/renderer/app/App.tsx +++ b/apps/desktop/src/renderer/app/App.tsx @@ -366,6 +366,7 @@ export function App() { icon={} className={cx('sk-sidebar-item--group', mcpOpen && 'sk-sidebar-item--group--open')} onClick={() => setMcpOpen((open) => !open)} + data-testid="nav-group-mcp" > {t('nav.mcp')} @@ -384,6 +385,7 @@ export function App() { className="sk-sidebar-item--sub" active={activeView === 'mcp-components'} onClick={() => goTo('mcp-components')} + data-testid="nav-mcp-components" > {t('mcp.componentsTitle')} @@ -391,6 +393,7 @@ export function App() { className="sk-sidebar-item--sub" active={activeView === 'mcp-management'} onClick={() => goTo('mcp-management')} + data-testid="nav-mcp-management" > {t('mcp.managementTitle')} diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss b/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss index 92569679..a5a7f1c3 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss @@ -36,6 +36,16 @@ color: var(--sk-color-label-2); } +// The preflight-refusal message shown inside `McpUpdateParamsModal` (the +// 0.7.0 regression case: the source changed to something the agent's config +// cannot express). Tinted like the other destructive/error surfaces +// (`app/App.scss`'s `.sk-state--error`), not the neutral `__param-help` +// hint above -- this is a real failure, not a disabled-button explanation. +.sk-mcp-install__error { + font-size: 13px; + color: var(--sk-red); +} + .sk-mcp-install__agents { display: flex; flex-wrap: wrap; diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.tsx b/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.tsx index a6c4ee33..f2b806b6 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.tsx +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.tsx @@ -175,10 +175,16 @@ export function McpInstallModal({ onClose={busy ? () => {} : onClose} title={t('mcp.installTitle', { name: preset.name })} className="sk-mcp-install" + data-testid="mcp-install-modal" >
    {serverSpans !== undefined && ( - + )}
    diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx index 0b8566bc..56ed9f5f 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx @@ -4,12 +4,25 @@ * parameter is a `Select`, not a text field, so a value outside the option set * cannot be submitted from here -- and the description the author wrote for * that parameter is shown above it. + * + * The preflight now runs from INSIDE the modal (its own Confirm button), not + * before it opens -- see the component's own doc comment. Each story's + * `onPreflight` resolves as if the backend had already found the params named + * below missing, so press Confirm once in the Storybook canvas to reveal the + * fields these stories are named for. */ import type { Meta, StoryObj } from '@storybook/react'; import type { McpPreset } from '@/app/store'; import type { DescriptionSpan } from '@/services/bridge'; import { McpUpdateParamsModal } from './McpUpdateParamsModal'; +/** Resolves as an accepted preflight reporting `missing` as the params still + * needed -- what a real `onPreflight` reports once the backend has checked + * every affected instance's stored values against the new source def. */ +function acceptedPreflight(missing: string[]): () => Promise<{ ok: true; missingParams: string[] }> { + return async () => ({ ok: true, missingParams: missing }); +} + const meta = { title: 'features/McpUpdateParamsModal', component: McpUpdateParamsModal, @@ -68,16 +81,26 @@ const spans = fakeSpans({ // The newly required parameter carries `options`, so it is a Select with its // description above it. Update stays disabled until a value is picked. export const OptionConstrainedParameter: Story = { - args: { preset, missingParams: ['access'], getDescriptionSpans: spans }, + args: { preset, onPreflight: acceptedPreflight(['access']), getDescriptionSpans: spans }, }; // A described parameter with no options stays a text field, exactly as before // options existed. export const DescribedFreeTextParameter: Story = { - args: { preset, missingParams: ['workspace'], getDescriptionSpans: spans }, + args: { preset, onPreflight: acceptedPreflight(['workspace']), getDescriptionSpans: spans }, }; // Both at once, which is what a def introducing two placeholders produces. export const BothKinds: Story = { - args: { preset, missingParams: ['access', 'workspace'], getDescriptionSpans: spans }, + args: { preset, onPreflight: acceptedPreflight(['access', 'workspace']), getDescriptionSpans: spans }, +}; + +// The 0.7.0 regression case: the source changed to something the agent's +// native config cannot express. Confirm to see the refusal render inline. +export const PreflightRefused: Story = { + args: { + preset, + onPreflight: async () => ({ ok: false, error: 'codex cannot express the http transport' }), + getDescriptionSpans: spans, + }, }; diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx index fa1d844a..e6846886 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx @@ -1,11 +1,30 @@ /** - * Minimal prompt shown before updating one or more installed MCP instances, - * when the new source def introduces `{param}` placeholders that are absent - * from every affected instance's OWN stored `.skmcp.params.yml` values (see - * the design doc "MCP support" section 5, "Update"). Only the MISSING param - * names ever reach the renderer -- never any stored value -- so this modal - * asks for exactly those names and nothing else (no project/agent pickers: - * those are already fixed by the instances being updated). + * Confirmation prompt shown before updating one or more installed MCP + * instances to their preset's current source def. + * + * The preflight (`onPreflight`) does NOT run when this modal opens -- it runs + * only when Confirm is first pressed (see `handleSubmit`'s `'confirm'` branch + * below). This is what the 0.7.0 fix ("Updating an MCP server no longer + * deletes it when the new definition cannot be installed") depends on being + * testable end to end: an update whose new def the agent's native config + * cannot express (an inexpressible transport, or a placeholder with no value) + * must show its refusal HERE, before `onConfirm` -- and therefore before the + * mutating `updateMcp` call -- ever runs, rather than as a toast that could + * fire after a partial removal. Confirm doubles as that trigger and, once the + * preflight has resolved, as the actual confirm button: + * + * - refused (`ok: false`): shows the refusal inline (`error`, phase + * `'error'`) and stops -- `onConfirm` is never called, so the caller's + * `runMcpUpdate` (which is what calls `updateMcp`) never runs either. + * - accepted, nothing missing (`missingParams` empty): calls `onConfirm({})` + * immediately -- no fields to ask for. + * - accepted, something missing: shows exactly those fields (phase + * `'params'`) and waits for a second Confirm press. + * + * Only the MISSING param names ever reach the renderer -- never any stored + * value -- so the `'params'` phase asks for exactly those names and nothing + * else (no project/agent pickers: those are already fixed by the instances + * being updated). * * The controls are the install modal's, for the same reason they are there: a * parameter with `options` renders as a `Select`, its description renders @@ -14,28 +33,38 @@ * set here, have the backend refuse it, and read an error about their own * input as if it were about something stored. * - * Closing without every missing param filled in ABORTS the update: `onClose` - * never receives the partially-filled values, only `onConfirm` does, and - * Confirm stays disabled until every field holds an acceptable value. + * Closing without confirming ABORTS the update at any phase: `onClose` never + * receives a value and `onConfirm` is never called unless Confirm itself + * calls it. */ import { useEffect, useState } from 'react'; import type { McpPreset } from '@/app/store'; import { bridgeClient } from '@/services/bridge'; -import type { DescriptionSpan } from '@/services/bridge'; +import type { DescriptionSpan, McpUpdatePreflightResult } from '@/services/bridge'; import { useTranslator } from '@/systems/i18n'; import { Modal, Button, TextField, Select, DescriptionText } from '@/shared/ui'; import { descriptionQueries, spansForParam } from '../lib/descriptionSpanQueries'; import { paramValueValid } from '../lib/paramValueValid'; import './McpInstallModal.scss'; +/** Where the modal is in its confirm -> preflight -> (params ->) confirm + * sequence. Reset to `'confirm'` every time the modal opens. */ +type Phase = 'confirm' | 'checking' | 'params' | 'error'; + export interface McpUpdateParamsModalProps { readonly open: boolean; /** The preset being updated to, whose `def.parameters` carries each * parameter's description and its accepted `options`. */ readonly preset: McpPreset; - /** Sorted, de-duplicated param names the update needs that are not yet stored. */ - readonly missingParams: readonly string[]; - /** Receives the filled-in values, keyed by param name, when Confirm is pressed. */ + /** + * Runs the preflight for every instance this update affects. Called once, + * the first time Confirm is pressed -- never on open, so a modal that is + * opened and immediately closed never reaches the backend. + */ + readonly onPreflight: () => Promise; + /** Receives the filled-in values (keyed by param name; empty when nothing + * was missing) once the preflight has accepted the update and every + * required field holds an acceptable value. */ readonly onConfirm: (values: Record) => void; readonly onClose: () => void; /** @@ -50,24 +79,30 @@ export interface McpUpdateParamsModalProps { export function McpUpdateParamsModal({ open, preset, - missingParams, + onPreflight, onConfirm, onClose, getDescriptionSpans = bridgeClient.mcpDescriptionSpans, }: McpUpdateParamsModalProps) { const t = useTranslator(); + const [phase, setPhase] = useState('confirm'); + const [missingParams, setMissingParams] = useState([]); + const [error, setError] = useState(''); const [values, setValues] = useState>({}); // Populated once per open by a single `mcp_description_spans` call, exactly // as in `McpInstallModal`; empty until it resolves, which renders as "no // description" the same way "none authored" does. const [descriptionSpans, setDescriptionSpans] = useState([]); - // Reset the draft every time the modal opens, mirroring McpInstallModal. + // Reset every time the modal opens, mirroring McpInstallModal -- including + // the phase, so reopening after an earlier refusal or a filled-in form + // starts clean rather than replaying stale state. useEffect(() => { if (!open) return undefined; - const seeded: Record = {}; - for (const param of missingParams) seeded[param] = ''; - setValues(seeded); + setPhase('confirm'); + setMissingParams([]); + setError(''); + setValues({}); setDescriptionSpans([]); // Alive-flag guard, as in `McpInstallModal`: open A, close, open B before // A's spans resolve must not land A's descriptions on B's parameters. @@ -94,58 +129,104 @@ export function McpUpdateParamsModal({ const allFilled = missingParams.every((param) => paramValueValid(preset.def.parameters[param], values[param] ?? '')); - function confirm(): void { - if (!allFilled) return; - onConfirm(values); + function handleSubmit(): void { + if (phase === 'params') { + if (!allFilled) return; + onConfirm(values); + return; + } + if (phase !== 'confirm') return; + setPhase('checking'); + void onPreflight().then((result) => { + if (!result.ok) { + setError(result.error); + setPhase('error'); + return; + } + if (result.missingParams.length === 0) { + onConfirm({}); + return; + } + const seeded: Record = {}; + for (const param of result.missingParams) seeded[param] = ''; + setValues(seeded); + setMissingParams(result.missingParams); + setPhase('params'); + }); } + const submitDisabled = phase === 'checking' || phase === 'error' || (phase === 'params' && !allFilled); + return ( - +
    -
    - {t('mcp.field.parameters')} - {missingParams.map((param) => { - const meta = preset.def.parameters[param]; - const options = meta?.options ?? []; - const paramSpans = spansForParam(preset, descriptionSpans, param); - const value = values[param] ?? ''; - return ( - - ); - })} -
    + )} + + ); + })} +
    + )} + {phase === 'error' && ( +

    + {error} +

    + )}
    -
    diff --git a/apps/desktop/src/renderer/pages/Mcp/ManagementPage.scss b/apps/desktop/src/renderer/pages/Mcp/ManagementPage.scss index 4f1c1653..8d633022 100644 --- a/apps/desktop/src/renderer/pages/Mcp/ManagementPage.scss +++ b/apps/desktop/src/renderer/pages/Mcp/ManagementPage.scss @@ -25,6 +25,25 @@ margin: 0 var(--sk-space-3); } +// The e2e `mcp-page` test id (ManagementPage.tsx) sits on a wrapper `
    ` +// around the page body's content (empty state, or tree + footer) -- +// introduced only to give both branches one common anchor. That wrapper is a +// new element in what was `Page`'s own `.sk-page__body` flex context +// (`shared/ui/Page/Page.scss`): without this class it becomes a plain block +// box, and `.sk-list-footer`'s `margin-top: auto` below stops pinning the +// footer to the bottom of a short tree, since `margin: auto` only resolves +// against a flex/grid container. Replicates `.sk-page__body`'s own flex +// properties exactly (not its padding, which stays on the real +// `.sk-page__body` parent) so the wrapper is transparent to layout -- a test +// id must never change what renders. Mirrors +// `pages/Skills/SkillsPage.scss`'s `.sk-skills-page-body`. +.sk-mcp-page-body { + display: flex; + flex-direction: column; + gap: var(--sk-space-5); + flex: 1 0 auto; +} + // Spacer between the (sticky) toolbar and the tree. Mirrors McpPage.scss's/ // ComponentsPage.scss's own tree spacer. .sk-mcp-management-tree { diff --git a/apps/desktop/src/renderer/pages/Mcp/ManagementPage.tsx b/apps/desktop/src/renderer/pages/Mcp/ManagementPage.tsx index a14a3a6b..26a42ae8 100644 --- a/apps/desktop/src/renderer/pages/Mcp/ManagementPage.tsx +++ b/apps/desktop/src/renderer/pages/Mcp/ManagementPage.tsx @@ -126,11 +126,11 @@ export function ManagementPage() { }, [projects]); const decorated = useMemo(() => { - function renderBadge(label: string, tone: 'accent' | 'neutral', onClick: () => void): ReactNode { + function renderBadge(label: string, tone: 'accent' | 'neutral', onClick: () => void, testId?: string): ReactNode { return ( e.stopPropagation()}> - @@ -148,13 +148,14 @@ export function ManagementPage() { case 'repo-preset': return ( - {renderBadge(t('mcp.installMcp'), 'accent', () => openInstall(item.preset))} + {renderBadge(t('mcp.installMcp'), 'accent', () => openInstall(item.preset), 'mcp-install-open')} ); case 'installed': return ( - {item.updatable && renderBadge(t('mcp.update'), 'accent', () => startMcpUpdate(item.installs))} + {item.updatable && + renderBadge(t('mcp.update'), 'accent', () => startMcpUpdate(item.installs), 'mcp-update-open')} {renderBadge(t('mcp.delete'), 'neutral', () => requestDeleteInstalls(name, item.installs))} ); @@ -311,45 +312,57 @@ export function ManagementPage() {
    } > - {/* An empty tree has two causes now that the Global root can be filtered - out too (before this it was always present, so `baseTree` was never - empty): there is nothing installed at all, or the filters excluded - everything there is. Only the first is "no MCP servers yet"; the second - must say so and carry a reset, since this page has no in-tree footer - reset to fall back on at all. */} - {baseTree.length === 0 ? ( - filtering ? ( -
    -

    {t('mcp.emptyFiltered')}

    - -
    - ) : ( -

    {t('mcp.empty')}

    - ) - ) : ( - <> - setMcpUi({ expandedIds: ids })} - ariaLabel={t('mcp.managementTitle')} - /> - {searching && ( -
    - setQuery('')} - /> + {/* e2e (flows 7, 8, 12, `mcp.spec.ts`): a stable anchor for "the MCP + Management page is showing", mirroring `pages/Skills/ManagementPage.tsx`'s + `skills-page` -- one wrapper around both branches below rather than + one testid per branch, since there is no single element common to + both that would otherwise need it. `.sk-mcp-page-body` + (ManagementPage.scss) replicates `Page`'s own `.sk-page__body` flex + layout so this wrapper is transparent to rendering -- a test id must + never change what renders; see that class's own doc comment (it + also explains why `.sk-list-footer`'s bottom-pinning depended on + this). */} +
    + {/* An empty tree has two causes now that the Global root can be filtered + out too (before this it was always present, so `baseTree` was never + empty): there is nothing installed at all, or the filters excluded + everything there is. Only the first is "no MCP servers yet"; the second + must say so and carry a reset, since this page has no in-tree footer + reset to fall back on at all. */} + {baseTree.length === 0 ? ( + filtering ? ( +
    +

    {t('mcp.emptyFiltered')}

    +
    - )} - - )} + ) : ( +

    {t('mcp.empty')}

    + ) + ) : ( + <> + setMcpUi({ expandedIds: ids })} + ariaLabel={t('mcp.managementTitle')} + /> + {searching && ( +
    + setQuery('')} + /> +
    + )} + + )} +
    {modals} diff --git a/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx b/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx index 46fbad9e..793c3d6f 100644 --- a/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx +++ b/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx @@ -334,7 +334,23 @@ export function buildMcpProjectTree( const rowsFor = (p: RepoPreset): TreeNode[] => { const presetLeafId = mcpProjectPresetLeafId(scope.id, p.id); items.set(presetLeafId, { kind: 'repo-preset', preset: p }); - const presetLeaf: TreeNode = { id: presetLeafId, label: p.name, icon: mcpIcon }; + const presetLeaf: TreeNode = { + id: presetLeafId, + label: p.name, + icon: mcpIcon, + // e2e (flow 7, `mcp.spec.ts`): the Management page's per-scope "install + // this preset" row. Tagged only here, NOT on the matched "installed" + // row rendered beside it a few lines down and NOT on the top-level + // catalog leaf above -- a preset with an existing install renders + // BOTH this row and that one side by side under the same repo node, + // so tagging every occurrence of a preset's name in this tree with + // the same `data-mcp-name` would make `.filter({ has: ... })` match + // more than one row wherever that overlap exists. The flow that reads + // this tag only ever targets a preset with no install yet, so the + // ambiguity never arises for it. + rowTestId: 'mcp-server-row', + identity: { attr: 'mcp-name', value: p.name }, + }; const matches = projectInstalls.filter((inst) => identityMatchesRepoPreset(inst.identity, p)); const byInstance = new Map(); @@ -411,7 +427,19 @@ export function buildMcpProjectTree( const id = mcpInstalledLeafId(scope.id, key); const updatable = mcpInstallHasUpdate(first, presets); items.set(id, { kind: 'installed', installs: group, updatable }); - return { id, label: instanceDisplayName(first.identity.source, first.instanceName), icon: mcpIconInstalled }; + return { + id, + label: instanceDisplayName(first.identity.source, first.instanceName), + icon: mcpIconInstalled, + // e2e (flows 8, 12, `mcp.spec.ts`): the Management page's installed + // row for a manual preset's instance -- the Update badge's target. + // A manual preset has no per-project "install row" duplicate (see + // this function's own doc comment), so this is the only row this + // instance's name ever renders as, unlike the repo-preset case + // above. + rowTestId: 'mcp-server-row', + identity: { attr: 'mcp-name', value: first.identity.source }, + }; }); // Unlinked: installs matching no current preset, bucketed by source/remote. diff --git a/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx b/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx index a20e475f..465e4f72 100644 --- a/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx +++ b/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx @@ -14,7 +14,7 @@ import { useTranslator } from '@/systems/i18n'; import { applyScope } from '@/domain'; import type { ApplyScope } from '@/domain'; import { bridgeClient } from '@/services/bridge'; -import type { McpInstall, McpUpdateReq } from '@/services/bridge'; +import type { McpInstall, McpUpdatePreflightResult, McpUpdateReq } from '@/services/bridge'; import { Button, Modal } from '@/shared/ui'; import { McpCard } from '@/entities/mcp'; import { McpEditModal } from '@/features/mcpEdit'; @@ -84,14 +84,16 @@ export function useMcpActions(): McpActions { const [editOpen, setEditOpen] = useState(false); const [editingPreset, setEditingPreset] = useState(undefined); const [installTarget, setInstallTarget] = useState<{ preset: McpPreset; projectId?: string } | null>(null); - // The pending update's target, once the preflight has determined which - // params are missing (prompt open); null means closed. Closing WITHOUT - // confirming aborts the update -- no `McpUpdateParamsModal` `onConfirm` call - // means `runMcpUpdate` never runs. + // The pending update's target; null means the confirm modal is closed. + // Unlike before, this opens as soon as the Update badge is clicked -- + // BEFORE any preflight call -- so `McpUpdateParamsModal` itself decides + // when to preflight (its own Confirm button) and can show a refusal inline + // instead of a toast that could fire with no modal open at all. Closing + // WITHOUT confirming aborts the update -- no `McpUpdateParamsModal` + // `onConfirm` call means `runMcpUpdate` never runs. const [updateTarget, setUpdateTarget] = useState<{ scope: ApplyScope; installs: readonly McpInstall[]; - missingParams: string[]; // The preset being updated TO, carried so the prompt can render each // parameter's description and its accepted options rather than a bare // text field -- see `McpUpdateParamsModal`. @@ -160,54 +162,54 @@ export function useMcpActions(): McpActions { [projects, mcpPresets, updateMcp, notify, t], ); - // Update entry point: preflight every affected agent's instance (one per - // `toUpdate` entry) against the preset's current def, then either update - // directly (nothing missing) or open the params modal for the UNION of - // missing names across all of them. Closing that modal without confirming - // aborts -- `updateTarget` is simply cleared, `runMcpUpdate` never runs. - const startMcpUpdateAsync = useCallback( - async (toUpdate: readonly McpInstall[]): Promise => { + // Update entry point: opens the confirm modal immediately -- resolving the + // scope/preset is synchronous, so there is nothing to await before the + // modal can appear. The preflight itself runs from inside + // `McpUpdateParamsModal`, via `preflightUpdate` below (passed as its + // `onPreflight` prop), the first time its Confirm button is pressed. + const startMcpUpdate = useCallback( + (toUpdate: readonly McpInstall[]): void => { const first = toUpdate[0]; if (first === undefined) return; const scope = applyScope(first.projectId, projects); if (scope === null) return; const preset = matchMcpPreset(first, mcpPresets); if (preset === undefined) return; - const results = await Promise.all( - toUpdate.map((inst) => - bridgeClient.mcpUpdatePreflight({ - projectId: scope.projectId, - projectPath: scope.projectPath, - agent: inst.agent, - instanceName: inst.instanceName, - def: preset.def, - scope: scope.scope, - }), - ), - ); - const missing = new Set(); - for (const r of results) { - if (!r.ok) { - notify(r.error, 'error'); - return; - } - for (const p of r.missingParams) missing.add(p); - } - if (missing.size === 0) { - await runMcpUpdate(toUpdate, {}); - return; - } - setUpdateTarget({ scope, installs: toUpdate, missingParams: [...missing].sort(), preset }); + setUpdateTarget({ scope, installs: toUpdate, preset }); }, - [projects, mcpPresets, notify, runMcpUpdate], + [projects, mcpPresets], ); - const startMcpUpdate = useCallback( - (toUpdate: readonly McpInstall[]): void => { - void startMcpUpdateAsync(toUpdate); - }, - [startMcpUpdateAsync], - ); + // Preflights every instance `updateTarget` affects (one call per agent + // target) against the preset's current def: the UNION of missing param + // names across all of them if every one accepts the update, or the first + // refusal encountered -- mirrors the old `startMcpUpdateAsync`'s + // aggregation, just run from inside the modal instead of before it opens. + // `updateTarget` is read fresh on every call (closed over via the + // dependency array), so a stale target from an already-closed modal can + // never be preflighted. + const preflightUpdate = useCallback(async (): Promise => { + const target = updateTarget; + if (target === null) return { ok: false, error: '' }; + const results = await Promise.all( + target.installs.map((inst) => + bridgeClient.mcpUpdatePreflight({ + projectId: target.scope.projectId, + projectPath: target.scope.projectPath, + agent: inst.agent, + instanceName: inst.instanceName, + def: target.preset.def, + scope: target.scope.scope, + }), + ), + ); + const missing = new Set(); + for (const r of results) { + if (!r.ok) return r; + for (const p of r.missingParams) missing.add(p); + } + return { ok: true, missingParams: [...missing].sort() }; + }, [updateTarget]); // Removes one leaf's installed instances (installed or unlinked): all share // the same project (the tree groups installs by project node), so the first @@ -313,7 +315,7 @@ export function useMcpActions(): McpActions { { const target = updateTarget; setUpdateTarget(null); diff --git a/apps/desktop/src/renderer/shared/ui/DescriptionText/DescriptionText.tsx b/apps/desktop/src/renderer/shared/ui/DescriptionText/DescriptionText.tsx index 1c29f606..7944f100 100644 --- a/apps/desktop/src/renderer/shared/ui/DescriptionText/DescriptionText.tsx +++ b/apps/desktop/src/renderer/shared/ui/DescriptionText/DescriptionText.tsx @@ -34,6 +34,10 @@ export interface DescriptionTextProps { * value. */ readonly onOpenLink: (url: string) => void; readonly className?: string; + /** Test id for the rendered span. Generic passthrough -- DescriptionText + * has no product knowledge of it, a caller sets it only for the flows + * that need it. */ + readonly 'data-testid'?: string; } /** One span plus a stable React key. Keyed by position: spans never reorder @@ -45,9 +49,9 @@ export function spansToKeyedParts(spans: readonly DescriptionSpan[]): KeyedDescr return spans.map((span, index) => ({ ...span, key: String(index) })); } -export function DescriptionText({ spans, onOpenLink, className }: DescriptionTextProps) { +export function DescriptionText({ spans, onOpenLink, className, 'data-testid': testId }: DescriptionTextProps) { return ( - + {spansToKeyedParts(spans).map((part) => part.kind === 'text' ? ( {part.text} diff --git a/e2e/desktop/fixtures/mcp.ts b/e2e/desktop/fixtures/mcp.ts new file mode 100644 index 00000000..7fee67fa --- /dev/null +++ b/e2e/desktop/fixtures/mcp.ts @@ -0,0 +1,195 @@ +/** + * Scenarios for the MCP pages (flows 7, 8 and 12 -- see + * `.superpowers/sdd/2026-09-09-desktop-ui-e2e/task-8-brief.md`). All three + * run on `nav-mcp-management` (`App.tsx`'s two-level MCP sidebar group, and + * the only MCP page `useMcpActions`'s Install/Update badges are wired into + * with the testids this task adds): flow 7 installs a not-yet-installed + * repo-discovered preset; flows 8 and 12 update an already-installed + * instance. + * + * Every scenario below targets the GLOBAL install scope rather than a + * tracked project -- `applyScope('global', [])` resolves fine with zero + * tracked projects, so no scenario here needs the Projects page's own + * startup trio (`editors_list`/`projects_describe`/`projects_folder_state`) + * that a scenario carrying a tracked project would (see `fixtures/skills.ts`'s + * `projectsPageStartupResponses` for that case -- `App`'s `activeView` starts + * at `'projects'` regardless of which page a spec navigates to afterward, so + * a non-empty `projects` array would still need answering). Staying + * Global-only also sidesteps a real ambiguity in `buildMcpProjectTree`: its + * per-project repo-preset "install row" renders once per SCOPE ROOT shown + * (Global AND every tracked project), so a tracked project alongside Global + * would render the SAME preset name twice, each carrying the same + * `data-mcp-name` -- `mcp-server-row`'s `.filter({ has: ... })` locator would + * then match more than one row. See `pages/Mcp/lib/mcpTree.tsx`'s own comment + * on the tagged leaves for the other half of that avoidance (which leaf kinds + * carry the tag at all). + * + * `mcp_installs` carries no default answer in `harness/commands.ts` (like + * `skills_list`, it is not part of `store.loadAll`'s startup round trip -- + * see that file's doc comment): `ManagementPage`'s own mount effect calls it + * unconditionally, so every scenario here answers it explicitly. + */ +import { withScenario, defaultScenario } from '../harness/scenario.js'; +import type { Scenario } from '../harness/scenario.js'; +import type { Repository } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/index.js'; +import type { DescriptionSpan } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/DescriptionSpan.js'; +import type { McpPreset as ConfigMcpPreset } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/McpPreset.js'; +import type { + AvailableMcp, + RawMcpServerDef, + McpInstall, + ApplyMcpResult, + UpdateMcpResult, +} from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; + +/** The one repository flow 7's repo-discovered preset resolves from. */ +function repo(): Repository { + return { + id: 'mcp-repo-id', + name: 'mcp-repo', + url: 'https://example.invalid/mcp-repo.git', + kind: 'generic', + transport: 'https', + lfs: false, + localPath: '/repos/mcp-repo-id', + }; +} + +const SERVER_DESCRIPTION_SPANS: DescriptionSpan[] = [ + { kind: 'text', text: 'Connect to ' }, + { kind: 'link', text: 'GitHub', url: 'https://github.com' }, + { kind: 'text', text: ' for issues.' }, +]; +const REGION_DESCRIPTION_SPANS: DescriptionSpan[] = [{ kind: 'text', text: 'Pick a region.' }]; + +/** + * Flow 7: a repo-discovered preset with a server description (parsed into + * spans, never raw `[text](url)` markup -- see + * `features/mcpInstall/lib/descriptionRenderSites.test.ts` for the rule this + * exercises end to end) and one option-constrained parameter. Not yet + * installed anywhere, so it renders as exactly one `mcp-server-row` (the + * "install this preset" row) -- no matching "installed" row exists yet to + * collide with it. + */ +export function withParameters(): Scenario { + const def: RawMcpServerDef = { + name: 'github', + type: 'http', + url: 'https://api.example.invalid/{region}', + description: 'Connect to [GitHub](https://github.com) for issues.', + parameters: { + region: { + description: 'Pick a region.', + options: [ + { value: 'us', label: 'United States' }, + { value: 'eu', label: 'Europe' }, + ], + }, + }, + }; + const mcpAvailable: AvailableMcp[] = [{ repoId: repo().id, remote: repo().url, def, hash: 'hash-github' }]; + const applied: ApplyMcpResult = { + ok: true, + installed: [{ agent: 'claude', instanceName: 'github', notes: [] }], + removed: 0, + skipped: [], + }; + return withScenario({ + repositories: [repo()], + mcpAvailable, + responses: { + mcp_installs: [], + mcp_description_spans: [SERVER_DESCRIPTION_SPANS, REGION_DESCRIPTION_SPANS], + mcp_apply: applied, + }, + }); +} + +const MANUAL_PRESET_ID = 'preset-github'; + +/** + * The one manual preset flows 8 and 12 update an installed instance of. Its + * `url` carries a `{token}` placeholder; a manually-authored preset carries + * no per-parameter metadata at all (see the generated config `McpPreset`'s + * own doc comment: "the desktop editor does not author `parameters` or + * `options`"), so `token` always renders as a plain text field, never a + * `Select` -- `withParameters` above is what covers the option-select case. + */ +function manualPreset(): ConfigMcpPreset { + return { id: MANUAL_PRESET_ID, name: 'github', type: 'http', url: 'https://api.example.invalid/{token}' }; +} + +/** `defaultScenario()`'s config with `manualPreset()` as its one manual MCP + * server, everything else left at its default. */ +function configWithManualPreset(): Scenario['config'] { + return { ...defaultScenario().config, mcp: { servers: [manualPreset()] } }; +} + +/** + * The one already-installed instance flows 8 and 12 target, at the GLOBAL + * scope (see this file's own doc comment for why). `hash` deliberately never + * matches the live `hashMcpDefInRenderer(manualPreset().def)` output (an + * unfakeable SHA-256 digest computed client-side from the CURRENT def) -- + * that mismatch is exactly what `mcpInstallHasUpdate` reads as "an update is + * available", which is what makes the Update badge (`mcp-update-open`) + * render at all. + */ +function installedInstance(): McpInstall { + return { + projectId: 'global', + agent: 'codex', + instanceName: 'github', + identity: { local: MANUAL_PRESET_ID, source: 'github' }, + hash: 'sha256:does-not-match-the-live-def', + hasParams: true, + }; +} + +/** + * Flow 8: the preflight accepts the update but reports `token` (the source's + * placeholder) missing from this instance's stored params -- the counterpart + * to `preflightRefuses` below, exercising the SAME `McpUpdateParamsModal` + * confirm-then-preflight-then-params sequence with an acceptance instead of a + * refusal. + */ +export function updatable(): Scenario { + const updated: UpdateMcpResult = { + ok: true, + updated: [{ agent: 'codex', instanceName: 'github', notes: [] }], + skipped: [], + }; + return withScenario({ + config: configWithManualPreset(), + mcpInstalls: [installedInstance()], + responses: { + mcp_installs: [installedInstance()], + // `McpUpdateParamsModal` fetches description spans unconditionally on + // open (mirroring `McpInstallModal`); `manualPreset()` authors no + // description at all, so nothing ever reads this response back -- + // it only needs to exist so the call is answered. + mcp_description_spans: [], + mcp_update_preflight: { ok: true, missingParams: ['token'] }, + mcp_update: updated, + }, + }); +} + +/** + * Flow 12: the preflight refuses -- the regression test for the 0.7.0 fix + * "Updating an MCP server no longer deletes it when the new definition + * cannot be installed". `message` is asserted verbatim by the spec, so it is + * supplied by the caller rather than fixed here. + */ +export function preflightRefuses(message: string): Scenario { + return withScenario({ + config: configWithManualPreset(), + mcpInstalls: [installedInstance()], + responses: { + mcp_installs: [installedInstance()], + // See `updatable()`'s identical override just above for why this is + // never actually read. + mcp_description_spans: [], + mcp_update_preflight: { ok: false, error: message }, + }, + }); +} diff --git a/e2e/desktop/tests/mcp.spec.ts b/e2e/desktop/tests/mcp.spec.ts new file mode 100644 index 00000000..472ec76e --- /dev/null +++ b/e2e/desktop/tests/mcp.spec.ts @@ -0,0 +1,143 @@ +/** + * Flows 7, 8 and 12 (MCP pages): installing a preset with a description and + * an option-constrained parameter, updating an installed instance whose + * preflight accepts it, and updating one whose preflight refuses it -- the + * regression test for the 0.7.0 fix "Updating an MCP server no longer + * deletes it when the new definition cannot be installed" (see + * `.superpowers/sdd/2026-09-09-desktop-ui-e2e/task-8-brief.md`, and + * `../fixtures/mcp.ts`'s own doc comment for why every scenario here targets + * the Global scope rather than a tracked project). + * + * MCP is a two-level sidebar group exactly like Skills (see `App.tsx`'s + * `NAV_ITEMS` comment): `nav-group-mcp` must be expanded before either of its + * two sub-items (`nav-mcp-components`, `nav-mcp-management`) is clickable. + * All three flows below use `nav-mcp-management` -- the page whose Install + * and Update badges (`useMcpActions`) carry this task's testids. + */ +import { test, expect } from '../harness/fixture'; +import { withParameters, updatable, preflightRefuses } from '../fixtures/mcp'; + +test.describe('an mcp update the agent cannot express', () => { + test.use({ scenario: preflightRefuses('codex cannot express the http transport') }); + + test('leaves the instance alone', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-mcp').click(); + await page.getByTestId('nav-mcp-management').click(); + await expect(page.getByTestId('mcp-page')).toBeVisible(); + + const row = page.getByTestId('mcp-server-row').filter({ + has: page.locator('[data-mcp-name="github"]'), + }); + await expect(row).toBeVisible(); + + await row.getByTestId('mcp-update-open').click(); + await page.getByTestId('mcp-update-submit').click(); + + await expect(page.getByTestId('mcp-update-error')).toContainText('cannot express'); + // The point of the 0.7.0 fix: the removal must not have happened. + await expect(row).toBeVisible(); + expect(await app.calls('mcp_update')).toHaveLength(0); + }); +}); + +test.describe('installing an mcp server with a description and an option parameter', () => { + test.use({ scenario: withParameters() }); + + test('the description renders as spans and the option select offers its labels', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-mcp').click(); + await page.getByTestId('nav-mcp-management').click(); + await expect(page.getByTestId('mcp-page')).toBeVisible(); + + // The Global scope root is already expanded (it is a tree root, per + // `ManagementPage.tsx`'s `rootIds(baseTree)` seed); the repo node nested + // under it is not -- mirroring `skills.spec.ts`'s "browsing skills" flow, + // expanding it reveals the preset's install row. + await page.getByText('mcp-repo', { exact: true }).click(); + + const row = page.getByTestId('mcp-server-row').filter({ has: page.locator('[data-mcp-name="github"]') }); + await row.getByTestId('mcp-install-open').click(); + + const modal = page.getByTestId('mcp-install-modal'); + await expect(modal).toBeVisible(); + + // The backend has already parsed the description into spans by the time + // it reaches the renderer (`mcp_description_spans`, mocked by the + // scenario) -- this asserts it rendered as those spans (a link button + // reading "GitHub" alongside its surrounding text), never as the raw + // `[GitHub](https://github.com)` markup string. See + // `features/mcpInstall/lib/descriptionRenderSites.test.ts` for the + // source-level rule this is the end-to-end counterpart of. + const description = page.getByTestId('mcp-install-description'); + await expect(description).toContainText('Connect to'); + await expect(description.getByRole('button', { name: 'GitHub' })).toBeVisible(); + await expect(description).toContainText('for issues.'); + const descriptionText = await description.textContent(); + expect(descriptionText).not.toContain('['); + expect(descriptionText).not.toContain(']'); + expect(descriptionText).not.toContain('(https://github.com)'); + + // `exact: true` and scoped to the modal: the toolbar's own "Projects" + // filter combobox is also on screen, and a substring match on "Project" + // would otherwise resolve to both. + await modal.getByRole('combobox', { name: 'Project', exact: true }).click(); + await page.getByRole('option', { name: 'Global' }).click(); + // The native checkbox input is visually hidden (`Checkbox.scss`'s + // `.sk-checkbox__input`, zero-size + opacity 0 -- the styled box is a + // sibling), so it never becomes Playwright-"visible" itself; clicking its + // label text toggles it exactly as a real click anywhere on the row would. + await modal.getByText('Claude', { exact: true }).click(); + + // The option-constrained parameter renders as a Select, not a text + // field; opening it must offer both authored option labels. + const regionField = page + .getByTestId('mcp-param-select') + .filter({ has: page.locator('[data-param-name="region"]') }); + await regionField.getByRole('button').click(); + await expect(page.getByRole('option', { name: 'United States' })).toBeVisible(); + await page.getByRole('option', { name: 'Europe' }).click(); + + await page.getByTestId('mcp-install-submit').click(); + await expect(modal).toBeHidden(); + + expect(await app.calls('mcp_apply')).toHaveLength(1); + }); +}); + +test.describe('updating an mcp server with a passing preflight', () => { + test.use({ scenario: updatable() }); + + test('a missing parameter is asked for and the update proceeds', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-mcp').click(); + await page.getByTestId('nav-mcp-management').click(); + await expect(page.getByTestId('mcp-page')).toBeVisible(); + + const row = page.getByTestId('mcp-server-row').filter({ has: page.locator('[data-mcp-name="github"]') }); + await expect(row).toBeVisible(); + + await row.getByTestId('mcp-update-open').click(); + await expect(page.getByTestId('mcp-update-modal')).toBeVisible(); + + // The first Confirm press is what runs the preflight (see + // `McpUpdateParamsModal`'s own doc comment); it reports the source's new + // `{token}` placeholder missing from this instance's stored params. + await page.getByTestId('mcp-update-submit').click(); + + const tokenField = page.getByTestId('mcp-param-input').filter({ has: page.locator('[data-param-name="token"]') }); + await expect(tokenField).toBeVisible(); + await tokenField.locator('input').fill('secret-token'); + + // The second press confirms with the filled-in value. + await page.getByTestId('mcp-update-submit').click(); + + await expect(page.getByTestId('mcp-update-modal')).toBeHidden(); + await expect(page.getByTestId('mcp-update-error')).toHaveCount(0); + + const calls = await app.calls('mcp_update'); + expect(calls).toHaveLength(1); + const { updates } = (calls[0] as { args: { updates: readonly { values: Record }[] } }).args; + expect(updates[0]?.values).toEqual({ token: 'secret-token' }); + }); +}); From af5ba4e2f37c1ea28d4096c1c9dd18bb73b82532 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 04:36:04 +0200 Subject: [PATCH 15/25] test: rebuild mcp update regression test on the real path --- .../mcpInstall/ui/McpInstallModal.scss | 10 - .../ui/McpUpdateParamsModal.stories.tsx | 29 +-- .../mcpInstall/ui/McpUpdateParamsModal.tsx | 206 ++++++------------ .../src/renderer/pages/Mcp/lib/mcpTree.tsx | 89 ++++++-- .../src/renderer/pages/Mcp/useMcpActions.tsx | 94 ++++---- .../systems/notifications/ui/Toasts.tsx | 10 + e2e/desktop/fixtures/mcp.ts | 197 +++++++++++++---- e2e/desktop/tests/mcp.spec.ts | 140 +++++++----- 8 files changed, 436 insertions(+), 339 deletions(-) diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss b/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss index a5a7f1c3..92569679 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpInstallModal.scss @@ -36,16 +36,6 @@ color: var(--sk-color-label-2); } -// The preflight-refusal message shown inside `McpUpdateParamsModal` (the -// 0.7.0 regression case: the source changed to something the agent's config -// cannot express). Tinted like the other destructive/error surfaces -// (`app/App.scss`'s `.sk-state--error`), not the neutral `__param-help` -// hint above -- this is a real failure, not a disabled-button explanation. -.sk-mcp-install__error { - font-size: 13px; - color: var(--sk-red); -} - .sk-mcp-install__agents { display: flex; flex-wrap: wrap; diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx index 56ed9f5f..0b8566bc 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.stories.tsx @@ -4,25 +4,12 @@ * parameter is a `Select`, not a text field, so a value outside the option set * cannot be submitted from here -- and the description the author wrote for * that parameter is shown above it. - * - * The preflight now runs from INSIDE the modal (its own Confirm button), not - * before it opens -- see the component's own doc comment. Each story's - * `onPreflight` resolves as if the backend had already found the params named - * below missing, so press Confirm once in the Storybook canvas to reveal the - * fields these stories are named for. */ import type { Meta, StoryObj } from '@storybook/react'; import type { McpPreset } from '@/app/store'; import type { DescriptionSpan } from '@/services/bridge'; import { McpUpdateParamsModal } from './McpUpdateParamsModal'; -/** Resolves as an accepted preflight reporting `missing` as the params still - * needed -- what a real `onPreflight` reports once the backend has checked - * every affected instance's stored values against the new source def. */ -function acceptedPreflight(missing: string[]): () => Promise<{ ok: true; missingParams: string[] }> { - return async () => ({ ok: true, missingParams: missing }); -} - const meta = { title: 'features/McpUpdateParamsModal', component: McpUpdateParamsModal, @@ -81,26 +68,16 @@ const spans = fakeSpans({ // The newly required parameter carries `options`, so it is a Select with its // description above it. Update stays disabled until a value is picked. export const OptionConstrainedParameter: Story = { - args: { preset, onPreflight: acceptedPreflight(['access']), getDescriptionSpans: spans }, + args: { preset, missingParams: ['access'], getDescriptionSpans: spans }, }; // A described parameter with no options stays a text field, exactly as before // options existed. export const DescribedFreeTextParameter: Story = { - args: { preset, onPreflight: acceptedPreflight(['workspace']), getDescriptionSpans: spans }, + args: { preset, missingParams: ['workspace'], getDescriptionSpans: spans }, }; // Both at once, which is what a def introducing two placeholders produces. export const BothKinds: Story = { - args: { preset, onPreflight: acceptedPreflight(['access', 'workspace']), getDescriptionSpans: spans }, -}; - -// The 0.7.0 regression case: the source changed to something the agent's -// native config cannot express. Confirm to see the refusal render inline. -export const PreflightRefused: Story = { - args: { - preset, - onPreflight: async () => ({ ok: false, error: 'codex cannot express the http transport' }), - getDescriptionSpans: spans, - }, + args: { preset, missingParams: ['access', 'workspace'], getDescriptionSpans: spans }, }; diff --git a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx index e6846886..358d64da 100644 --- a/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx +++ b/apps/desktop/src/renderer/features/mcpInstall/ui/McpUpdateParamsModal.tsx @@ -1,30 +1,11 @@ /** - * Confirmation prompt shown before updating one or more installed MCP - * instances to their preset's current source def. - * - * The preflight (`onPreflight`) does NOT run when this modal opens -- it runs - * only when Confirm is first pressed (see `handleSubmit`'s `'confirm'` branch - * below). This is what the 0.7.0 fix ("Updating an MCP server no longer - * deletes it when the new definition cannot be installed") depends on being - * testable end to end: an update whose new def the agent's native config - * cannot express (an inexpressible transport, or a placeholder with no value) - * must show its refusal HERE, before `onConfirm` -- and therefore before the - * mutating `updateMcp` call -- ever runs, rather than as a toast that could - * fire after a partial removal. Confirm doubles as that trigger and, once the - * preflight has resolved, as the actual confirm button: - * - * - refused (`ok: false`): shows the refusal inline (`error`, phase - * `'error'`) and stops -- `onConfirm` is never called, so the caller's - * `runMcpUpdate` (which is what calls `updateMcp`) never runs either. - * - accepted, nothing missing (`missingParams` empty): calls `onConfirm({})` - * immediately -- no fields to ask for. - * - accepted, something missing: shows exactly those fields (phase - * `'params'`) and waits for a second Confirm press. - * - * Only the MISSING param names ever reach the renderer -- never any stored - * value -- so the `'params'` phase asks for exactly those names and nothing - * else (no project/agent pickers: those are already fixed by the instances - * being updated). + * Minimal prompt shown before updating one or more installed MCP instances, + * when the new source def introduces `{param}` placeholders that are absent + * from every affected instance's OWN stored `.skmcp.params.yml` values (see + * the design doc "MCP support" section 5, "Update"). Only the MISSING param + * names ever reach the renderer -- never any stored value -- so this modal + * asks for exactly those names and nothing else (no project/agent pickers: + * those are already fixed by the instances being updated). * * The controls are the install modal's, for the same reason they are there: a * parameter with `options` renders as a `Select`, its description renders @@ -33,38 +14,28 @@ * set here, have the backend refuse it, and read an error about their own * input as if it were about something stored. * - * Closing without confirming ABORTS the update at any phase: `onClose` never - * receives a value and `onConfirm` is never called unless Confirm itself - * calls it. + * Closing without every missing param filled in ABORTS the update: `onClose` + * never receives the partially-filled values, only `onConfirm` does, and + * Confirm stays disabled until every field holds an acceptable value. */ import { useEffect, useState } from 'react'; import type { McpPreset } from '@/app/store'; import { bridgeClient } from '@/services/bridge'; -import type { DescriptionSpan, McpUpdatePreflightResult } from '@/services/bridge'; +import type { DescriptionSpan } from '@/services/bridge'; import { useTranslator } from '@/systems/i18n'; import { Modal, Button, TextField, Select, DescriptionText } from '@/shared/ui'; import { descriptionQueries, spansForParam } from '../lib/descriptionSpanQueries'; import { paramValueValid } from '../lib/paramValueValid'; import './McpInstallModal.scss'; -/** Where the modal is in its confirm -> preflight -> (params ->) confirm - * sequence. Reset to `'confirm'` every time the modal opens. */ -type Phase = 'confirm' | 'checking' | 'params' | 'error'; - export interface McpUpdateParamsModalProps { readonly open: boolean; /** The preset being updated to, whose `def.parameters` carries each * parameter's description and its accepted `options`. */ readonly preset: McpPreset; - /** - * Runs the preflight for every instance this update affects. Called once, - * the first time Confirm is pressed -- never on open, so a modal that is - * opened and immediately closed never reaches the backend. - */ - readonly onPreflight: () => Promise; - /** Receives the filled-in values (keyed by param name; empty when nothing - * was missing) once the preflight has accepted the update and every - * required field holds an acceptable value. */ + /** Sorted, de-duplicated param names the update needs that are not yet stored. */ + readonly missingParams: readonly string[]; + /** Receives the filled-in values, keyed by param name, when Confirm is pressed. */ readonly onConfirm: (values: Record) => void; readonly onClose: () => void; /** @@ -79,30 +50,24 @@ export interface McpUpdateParamsModalProps { export function McpUpdateParamsModal({ open, preset, - onPreflight, + missingParams, onConfirm, onClose, getDescriptionSpans = bridgeClient.mcpDescriptionSpans, }: McpUpdateParamsModalProps) { const t = useTranslator(); - const [phase, setPhase] = useState('confirm'); - const [missingParams, setMissingParams] = useState([]); - const [error, setError] = useState(''); const [values, setValues] = useState>({}); // Populated once per open by a single `mcp_description_spans` call, exactly // as in `McpInstallModal`; empty until it resolves, which renders as "no // description" the same way "none authored" does. const [descriptionSpans, setDescriptionSpans] = useState([]); - // Reset every time the modal opens, mirroring McpInstallModal -- including - // the phase, so reopening after an earlier refusal or a filled-in form - // starts clean rather than replaying stale state. + // Reset the draft every time the modal opens, mirroring McpInstallModal. useEffect(() => { if (!open) return undefined; - setPhase('confirm'); - setMissingParams([]); - setError(''); - setValues({}); + const seeded: Record = {}; + for (const param of missingParams) seeded[param] = ''; + setValues(seeded); setDescriptionSpans([]); // Alive-flag guard, as in `McpInstallModal`: open A, close, open B before // A's spans resolve must not land A's descriptions on B's parameters. @@ -129,34 +94,11 @@ export function McpUpdateParamsModal({ const allFilled = missingParams.every((param) => paramValueValid(preset.def.parameters[param], values[param] ?? '')); - function handleSubmit(): void { - if (phase === 'params') { - if (!allFilled) return; - onConfirm(values); - return; - } - if (phase !== 'confirm') return; - setPhase('checking'); - void onPreflight().then((result) => { - if (!result.ok) { - setError(result.error); - setPhase('error'); - return; - } - if (result.missingParams.length === 0) { - onConfirm({}); - return; - } - const seeded: Record = {}; - for (const param of result.missingParams) seeded[param] = ''; - setValues(seeded); - setMissingParams(result.missingParams); - setPhase('params'); - }); + function confirm(): void { + if (!allFilled) return; + onConfirm(values); } - const submitDisabled = phase === 'checking' || phase === 'error' || (phase === 'params' && !allFilled); - return (
    - {phase === 'params' && ( -
    - {t('mcp.field.parameters')} - {missingParams.map((param) => { - const meta = preset.def.parameters[param]; - const options = meta?.options ?? []; - const paramSpans = spansForParam(preset, descriptionSpans, param); - const value = values[param] ?? ''; - return ( - // e2e (flow 8, `mcp.spec.ts`): mirrors `McpInstallModal`'s - // per-parameter row -- the field's kind (input or select) is - // this row's own testid; `data-param-name` (on the child - // label span, never this row itself) names which parameter. - - ); - })} -
    - )} - {phase === 'error' && ( -

    - {error} -

    - )} + {!paramValueValid(meta, value) && ( + {t('mcp.error.invalidOption')} + )} + + ) : ( + { + const next = e.target.value; + setValues((v) => ({ ...v, [param]: next })); + }} + /> + )} + + ); + })} +
    -
    diff --git a/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx b/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx index 793c3d6f..1e0121c0 100644 --- a/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx +++ b/apps/desktop/src/renderer/pages/Mcp/lib/mcpTree.tsx @@ -50,6 +50,25 @@ * `` is the full `/`-joined group prefix up to that level (e.g. * `platform` then `platform/lint`), not just the last segment, so a branch * at each nesting level gets a distinct id. + * + * ROW IDENTITY (e2e): every leaf `buildMcpProjectTree` emits carries + * `rowTestId: 'mcp-server-row'` and `identity: { attr: 'mcp-name', value: + * }` -- the id, never a bare + * display name (a preset's `.name`, or an install's `identity.source`). + * Two different leaves routinely share the same name in this tree: a repo + * preset's per-project "install this again" row (`rowsFor`'s `presetLeaf`) + * renders beside its own matched "installed" row the moment one instance + * exists, and the SAME preset can render once per scope root shown (Global + * and every tracked project). A name-keyed identity would make + * `.filter({ has: page.locator('[data-mcp-name="..."]') })` match more than + * one row in exactly those (common) cases; the leaf's own id is already + * guaranteed unique by construction (see the disjoint id-family list above), + * so reusing it costs nothing and removes the ambiguity outright rather than + * relying on a scenario/flow never triggering it. A spec/fixture computing an + * expected value calls the same id-builder (`mcpProjectPresetLeafId`, + * `mcpInstalledLeafId` + `instanceKey`, `mcpUnlinkedLeafId`, or + * `mcpManualLeafId`) this module exports for exactly that purpose, rather + * than guessing the format. */ import { Icon } from '@/shared/ui'; import type { TreeNode } from '@/shared/ui'; @@ -166,8 +185,12 @@ function identityKey(identity: McpInstall['identity']): string { /** A stable grouping key for one logical installed instance: the same * (identity, instance-config name) pair across every agent it is installed - * for collapses into one row. */ -function instanceKey(identity: McpInstall['identity'], instanceName: string): string { + * for collapses into one row. Exported (mirrors `repoMcpPresetId`'s own + * precedent) so an e2e fixture/spec can compute the exact + * `mcpInstalledLeafId`/`mcpUnlinkedLeafId` a scenario's install will render + * as -- e.g. `mcpInstalledLeafId(scope.id, instanceKey(install.identity, + * install.instanceName))` -- instead of guessing the format. */ +export function instanceKey(identity: McpInstall['identity'], instanceName: string): string { return `${identityKey(identity)}|${instanceName}`; } @@ -309,7 +332,17 @@ export function buildMcpProjectTree( .map((p) => { const id = mcpManualLeafId(p.id); items.set(id, { kind: 'manual-preset', preset: p }); - return { id, label: p.name, icon: mcpIcon }; + // e2e: the top-level catalog leaf for a manual preset, shown once + // regardless of scope. `id` (not `p.name`) is the identity value -- + // see this function's own doc comment on row identity for why every + // leaf below uses its own id rather than a bare display name. + return { + id, + label: p.name, + icon: mcpIcon, + rowTestId: 'mcp-server-row', + identity: { attr: 'mcp-name', value: id }, + }; }); const byRepo = new Map(); @@ -338,18 +371,16 @@ export function buildMcpProjectTree( id: presetLeafId, label: p.name, icon: mcpIcon, - // e2e (flow 7, `mcp.spec.ts`): the Management page's per-scope "install - // this preset" row. Tagged only here, NOT on the matched "installed" - // row rendered beside it a few lines down and NOT on the top-level - // catalog leaf above -- a preset with an existing install renders - // BOTH this row and that one side by side under the same repo node, - // so tagging every occurrence of a preset's name in this tree with - // the same `data-mcp-name` would make `.filter({ has: ... })` match - // more than one row wherever that overlap exists. The flow that reads - // this tag only ever targets a preset with no install yet, so the - // ambiguity never arises for it. + // e2e (flow 7, `mcp.spec.ts`): the Management page's per-scope + // "install this preset" row. `presetLeafId` (`scope.id` + + // `p.id`) is already the row's own unique tree-node id -- using it + // as the identity value, rather than the bare (non-unique) preset + // name, is what lets this row and the matched "installed" row + // rendered beside it (same preset, same scope, once an instance + // exists) both carry `mcp-server-row` without either becoming + // ambiguous to `.filter({ has: ... })`. rowTestId: 'mcp-server-row', - identity: { attr: 'mcp-name', value: p.name }, + identity: { attr: 'mcp-name', value: presetLeafId }, }; const matches = projectInstalls.filter((inst) => identityMatchesRepoPreset(inst.identity, p)); @@ -370,7 +401,16 @@ export function buildMcpProjectTree( const id = mcpInstalledLeafId(scope.id, key); const updatable = mcpInstallHasUpdate(first, presets); items.set(id, { kind: 'installed', installs: group, updatable }); - return { id, label: instanceDisplayName(first.identity.source, first.instanceName), icon: mcpIconInstalled }; + return { + id, + label: instanceDisplayName(first.identity.source, first.instanceName), + icon: mcpIconInstalled, + // e2e: the installed row for a repo preset's instance, distinct + // from `presetLeaf` above by its own unique id (see this + // function's doc comment). + rowTestId: 'mcp-server-row', + identity: { attr: 'mcp-name', value: id }, + }; }); return [presetLeaf, ...instanceLeaves]; @@ -433,12 +473,10 @@ export function buildMcpProjectTree( icon: mcpIconInstalled, // e2e (flows 8, 12, `mcp.spec.ts`): the Management page's installed // row for a manual preset's instance -- the Update badge's target. - // A manual preset has no per-project "install row" duplicate (see - // this function's own doc comment), so this is the only row this - // instance's name ever renders as, unlike the repo-preset case - // above. + // `id` (not `first.identity.source`) is the identity value -- see + // this function's own doc comment on row identity. rowTestId: 'mcp-server-row', - identity: { attr: 'mcp-name', value: first.identity.source }, + identity: { attr: 'mcp-name', value: id }, }; }); @@ -464,7 +502,16 @@ export function buildMcpProjectTree( const id = mcpUnlinkedLeafId(scope.id, key); items.set(id, { kind: 'unlinked', installs: group }); const label = instanceDisplayName(first.identity.source, first.instanceName); - const leaf: TreeNode = { id, label, icon: mcpIcon, muted: true }; + // e2e: the unlinked-instance row -- `id` (not the display label) is + // the identity value, see this function's doc comment. + const leaf: TreeNode = { + id, + label, + icon: mcpIcon, + muted: true, + rowTestId: 'mcp-server-row', + identity: { attr: 'mcp-name', value: id }, + }; const bucket = byGroupKey.get(groupKey); if (bucket !== undefined) bucket.rows.push({ leaf, sortLabel: label }); else byGroupKey.set(groupKey, { label: unlinkedGroupLabel(first.identity), rows: [{ leaf, sortLabel: label }] }); diff --git a/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx b/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx index 465e4f72..a20e475f 100644 --- a/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx +++ b/apps/desktop/src/renderer/pages/Mcp/useMcpActions.tsx @@ -14,7 +14,7 @@ import { useTranslator } from '@/systems/i18n'; import { applyScope } from '@/domain'; import type { ApplyScope } from '@/domain'; import { bridgeClient } from '@/services/bridge'; -import type { McpInstall, McpUpdatePreflightResult, McpUpdateReq } from '@/services/bridge'; +import type { McpInstall, McpUpdateReq } from '@/services/bridge'; import { Button, Modal } from '@/shared/ui'; import { McpCard } from '@/entities/mcp'; import { McpEditModal } from '@/features/mcpEdit'; @@ -84,16 +84,14 @@ export function useMcpActions(): McpActions { const [editOpen, setEditOpen] = useState(false); const [editingPreset, setEditingPreset] = useState(undefined); const [installTarget, setInstallTarget] = useState<{ preset: McpPreset; projectId?: string } | null>(null); - // The pending update's target; null means the confirm modal is closed. - // Unlike before, this opens as soon as the Update badge is clicked -- - // BEFORE any preflight call -- so `McpUpdateParamsModal` itself decides - // when to preflight (its own Confirm button) and can show a refusal inline - // instead of a toast that could fire with no modal open at all. Closing - // WITHOUT confirming aborts the update -- no `McpUpdateParamsModal` - // `onConfirm` call means `runMcpUpdate` never runs. + // The pending update's target, once the preflight has determined which + // params are missing (prompt open); null means closed. Closing WITHOUT + // confirming aborts the update -- no `McpUpdateParamsModal` `onConfirm` call + // means `runMcpUpdate` never runs. const [updateTarget, setUpdateTarget] = useState<{ scope: ApplyScope; installs: readonly McpInstall[]; + missingParams: string[]; // The preset being updated TO, carried so the prompt can render each // parameter's description and its accepted options rather than a bare // text field -- see `McpUpdateParamsModal`. @@ -162,54 +160,54 @@ export function useMcpActions(): McpActions { [projects, mcpPresets, updateMcp, notify, t], ); - // Update entry point: opens the confirm modal immediately -- resolving the - // scope/preset is synchronous, so there is nothing to await before the - // modal can appear. The preflight itself runs from inside - // `McpUpdateParamsModal`, via `preflightUpdate` below (passed as its - // `onPreflight` prop), the first time its Confirm button is pressed. - const startMcpUpdate = useCallback( - (toUpdate: readonly McpInstall[]): void => { + // Update entry point: preflight every affected agent's instance (one per + // `toUpdate` entry) against the preset's current def, then either update + // directly (nothing missing) or open the params modal for the UNION of + // missing names across all of them. Closing that modal without confirming + // aborts -- `updateTarget` is simply cleared, `runMcpUpdate` never runs. + const startMcpUpdateAsync = useCallback( + async (toUpdate: readonly McpInstall[]): Promise => { const first = toUpdate[0]; if (first === undefined) return; const scope = applyScope(first.projectId, projects); if (scope === null) return; const preset = matchMcpPreset(first, mcpPresets); if (preset === undefined) return; - setUpdateTarget({ scope, installs: toUpdate, preset }); + const results = await Promise.all( + toUpdate.map((inst) => + bridgeClient.mcpUpdatePreflight({ + projectId: scope.projectId, + projectPath: scope.projectPath, + agent: inst.agent, + instanceName: inst.instanceName, + def: preset.def, + scope: scope.scope, + }), + ), + ); + const missing = new Set(); + for (const r of results) { + if (!r.ok) { + notify(r.error, 'error'); + return; + } + for (const p of r.missingParams) missing.add(p); + } + if (missing.size === 0) { + await runMcpUpdate(toUpdate, {}); + return; + } + setUpdateTarget({ scope, installs: toUpdate, missingParams: [...missing].sort(), preset }); }, - [projects, mcpPresets], + [projects, mcpPresets, notify, runMcpUpdate], ); - // Preflights every instance `updateTarget` affects (one call per agent - // target) against the preset's current def: the UNION of missing param - // names across all of them if every one accepts the update, or the first - // refusal encountered -- mirrors the old `startMcpUpdateAsync`'s - // aggregation, just run from inside the modal instead of before it opens. - // `updateTarget` is read fresh on every call (closed over via the - // dependency array), so a stale target from an already-closed modal can - // never be preflighted. - const preflightUpdate = useCallback(async (): Promise => { - const target = updateTarget; - if (target === null) return { ok: false, error: '' }; - const results = await Promise.all( - target.installs.map((inst) => - bridgeClient.mcpUpdatePreflight({ - projectId: target.scope.projectId, - projectPath: target.scope.projectPath, - agent: inst.agent, - instanceName: inst.instanceName, - def: target.preset.def, - scope: target.scope.scope, - }), - ), - ); - const missing = new Set(); - for (const r of results) { - if (!r.ok) return r; - for (const p of r.missingParams) missing.add(p); - } - return { ok: true, missingParams: [...missing].sort() }; - }, [updateTarget]); + const startMcpUpdate = useCallback( + (toUpdate: readonly McpInstall[]): void => { + void startMcpUpdateAsync(toUpdate); + }, + [startMcpUpdateAsync], + ); // Removes one leaf's installed instances (installed or unlinked): all share // the same project (the tree groups installs by project node), so the first @@ -315,7 +313,7 @@ export function useMcpActions(): McpActions { { const target = updateTarget; setUpdateTarget(null); diff --git a/apps/desktop/src/renderer/systems/notifications/ui/Toasts.tsx b/apps/desktop/src/renderer/systems/notifications/ui/Toasts.tsx index fd348ba2..0cec7acd 100644 --- a/apps/desktop/src/renderer/systems/notifications/ui/Toasts.tsx +++ b/apps/desktop/src/renderer/systems/notifications/ui/Toasts.tsx @@ -75,6 +75,16 @@ export function Toasts() { key={toast.id} type="button" className="sk-toasts__item" + // e2e (flow 12, `mcp.spec.ts`): the mcp update preflight refusal + // this task guards against surfaces as a toast, not a modal (see + // `pages/Mcp/useMcpActions.tsx`'s `startMcpUpdateAsync`) -- this + // is the one addressable DOM surface it reaches. Deliberately not + // conditioned on `toast` content: `Toasts` is generic, cross- + // cutting UI with no MCP knowledge, and every spec that reaches + // this asserts against exactly one active toast, so a KIND-only + // testid (every toast gets it) is sufficient without teaching + // this component about any one caller's message. + data-testid="mcp-update-error" // A toast that carries documentation opens it, since the toast is // gone in five seconds and the log entry behind it is easy to miss. // Without one, clicking just dismisses, as before. diff --git a/e2e/desktop/fixtures/mcp.ts b/e2e/desktop/fixtures/mcp.ts index 7fee67fa..c8da00f8 100644 --- a/e2e/desktop/fixtures/mcp.ts +++ b/e2e/desktop/fixtures/mcp.ts @@ -1,28 +1,29 @@ /** * Scenarios for the MCP pages (flows 7, 8 and 12 -- see - * `.superpowers/sdd/2026-09-09-desktop-ui-e2e/task-8-brief.md`). All three - * run on `nav-mcp-management` (`App.tsx`'s two-level MCP sidebar group, and - * the only MCP page `useMcpActions`'s Install/Update badges are wired into - * with the testids this task adds): flow 7 installs a not-yet-installed - * repo-discovered preset; flows 8 and 12 update an already-installed - * instance. + * `.superpowers/sdd/2026-09-09-desktop-ui-e2e/task-8-brief.md`, and the + * fix-round notes appended to `task-8-report.md` for how flow 12's mechanism + * and the row-identity scheme below changed after review). All three run on + * `nav-mcp-management` (`App.tsx`'s two-level MCP sidebar group, and the + * only MCP page `useMcpActions`'s Install/Update badges are wired into with + * the testids this task adds). * - * Every scenario below targets the GLOBAL install scope rather than a - * tracked project -- `applyScope('global', [])` resolves fine with zero - * tracked projects, so no scenario here needs the Projects page's own - * startup trio (`editors_list`/`projects_describe`/`projects_folder_state`) - * that a scenario carrying a tracked project would (see `fixtures/skills.ts`'s - * `projectsPageStartupResponses` for that case -- `App`'s `activeView` starts - * at `'projects'` regardless of which page a spec navigates to afterward, so - * a non-empty `projects` array would still need answering). Staying - * Global-only also sidesteps a real ambiguity in `buildMcpProjectTree`: its - * per-project repo-preset "install row" renders once per SCOPE ROOT shown - * (Global AND every tracked project), so a tracked project alongside Global - * would render the SAME preset name twice, each carrying the same - * `data-mcp-name` -- `mcp-server-row`'s `.filter({ has: ... })` locator would - * then match more than one row. See `pages/Mcp/lib/mcpTree.tsx`'s own comment - * on the tagged leaves for the other half of that avoidance (which leaf kinds - * carry the tag at all). + * Row identity: `pages/Mcp/lib/mcpTree.tsx`'s `mcp-server-row`/ + * `data-mcp-name` now carries each leaf's OWN unique tree-node id, never a + * bare preset/instance name (see that file's "ROW IDENTITY" doc comment for + * why a bare name is not unique). The functions below compute that same id + * by mirroring the same builders the tree itself uses + * (`repoMcpPresetId`, `mcpProjectPresetLeafId`, `mcpInstalledLeafId`, + * `instanceKey`, `identityKey`) rather than guessing the format -- NOT by + * importing them: those live in `app/store/store.ts` and + * `pages/Mcp/lib/mcpTree.tsx`, both `@/`-aliased and (the latter) JSX + * source, neither of which this directory's own, deliberately narrower + * `tsconfig.json` can resolve (see that file's own doc comment on why it + * has no path aliases and no `"jsx"` option -- it only ever reaches into the + * ts-rs-generated `services/bridge` tree, which needs neither). A drift + * between the mirror below and the real builders would only surface as a + * spec failure (the computed id would stop matching any row), not a type + * error -- kept intentionally close to the source's exact string + * concatenation for that reason. * * `mcp_installs` carries no default answer in `harness/commands.ts` (like * `skills_list`, it is not part of `store.loadAll`'s startup round trip -- @@ -31,7 +32,7 @@ */ import { withScenario, defaultScenario } from '../harness/scenario.js'; import type { Scenario } from '../harness/scenario.js'; -import type { Repository } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/index.js'; +import type { Repository, Project } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/index.js'; import type { DescriptionSpan } from '../../../apps/desktop/src/renderer/services/bridge/generated/core/DescriptionSpan.js'; import type { McpPreset as ConfigMcpPreset } from '../../../apps/desktop/src/renderer/services/bridge/generated/config/McpPreset.js'; import type { @@ -42,6 +43,39 @@ import type { UpdateMcpResult, } from '../../../apps/desktop/src/renderer/services/bridge/contracts.js'; +// -- row-identity mirrors (see this file's own doc comment for why these are +// -- copies of `app/store/store.ts`'s/`pages/Mcp/lib/mcpTree.tsx`'s builders +// -- rather than imports of them) --------------------------------------- + +/** Mirrors `app/store/store.ts`'s `repoMcpPresetId`. */ +function repoMcpPresetId(repoId: string, group: string | undefined, name: string): string { + return `repo:${repoId}:${group ?? ''}:${name}`; +} + +/** Mirrors `pages/Mcp/lib/mcpTree.tsx`'s `mcpProjectPresetLeafId`. */ +function mcpProjectPresetLeafId(projectId: string, presetId: string): string { + return ['mcp-repo', 'leaf', projectId, presetId].join('::'); +} + +/** Mirrors `pages/Mcp/lib/mcpTree.tsx`'s `mcpInstalledLeafId`. */ +function mcpInstalledLeafId(projectId: string, key: string): string { + return ['mcp-inst', projectId, key].join('::'); +} + +/** Mirrors `pages/Mcp/lib/mcpTree.tsx`'s `identityKey`, narrowed to the one + * identity shape this file's fixtures ever build (a manual preset's + * `local` id) -- the remote-based branch is not needed here. */ +function localIdentityKey(presetId: string): string { + return `local:${presetId}`; +} + +/** Mirrors `pages/Mcp/lib/mcpTree.tsx`'s `instanceKey`. */ +function instanceKey(identityKey: string, instanceName: string): string { + return `${identityKey}|${instanceName}`; +} + +// -- flow 7: installing a repo-discovered preset into a tracked project ----- + /** The one repository flow 7's repo-discovered preset resolves from. */ function repo(): Repository { return { @@ -55,6 +89,34 @@ function repo(): Repository { }; } +/** The one tracked project flow 7 installs into -- a tracked-project scope + * is the common case the original (Global-only) version of this fixture + * did not cover at all; see `mcpTree.tsx`'s "ROW IDENTITY" comment for why + * moving here no longer risks row-identity ambiguity against Global's own + * copy of the same preset. */ +function project(): Project { + return { id: 'mcp-project-id', path: '/projects/demo', name: 'Demo', addedAt: '2024-01-01T00:00:00Z' }; +} + +/** + * `App`'s `activeView` starts at `'projects'` (see `App.tsx`), so + * `ProjectsPage` mounts first on every `app.goto()` regardless of which page + * the spec navigates to afterward -- and with a non-empty `projects` array, + * its own mount effect (`refreshProjectInfo` -> `projects_describe`) and its + * cards' `OpenProjectButton` (`editors_list`) fire immediately, alongside the + * app-wide `useProjectCheckSchedule` sweep (`projects_folder_state`). None of + * the three has a default in `harness/commands.ts` (not part of `loadAll`'s + * startup round trip), so any scenario carrying a tracked project must answer + * them -- mirrors `fixtures/skills.ts`'s identical helper. + */ +function projectsPageStartupResponses(): Record { + return { + editors_list: [], + projects_describe: { skillCount: 0, fromReposCount: 0, agentCount: 0 }, + projects_folder_state: 'present', + }; +} + const SERVER_DESCRIPTION_SPANS: DescriptionSpan[] = [ { kind: 'text', text: 'Connect to ' }, { kind: 'link', text: 'GitHub', url: 'https://github.com' }, @@ -62,14 +124,29 @@ const SERVER_DESCRIPTION_SPANS: DescriptionSpan[] = [ ]; const REGION_DESCRIPTION_SPANS: DescriptionSpan[] = [{ kind: 'text', text: 'Pick a region.' }]; +/** The synthesized preset id `refreshMcpPresets` gives flow 7's + * repo-discovered preset -- same builder the store itself uses. */ +function githubPresetId(): string { + return repoMcpPresetId(repo().id, undefined, 'github'); +} + +/** + * The exact row identity flow 7's spec must filter by: the preset's install + * row nested under the TRACKED PROJECT's own branch. Global's copy of the + * same preset renders alongside it (every repo preset gets an install row + * per scope root shown) with a DIFFERENT id, so this must be the project- + * scoped one specifically, not the bare preset name. + */ +export function withParametersInstallRowId(): string { + return mcpProjectPresetLeafId(project().id, githubPresetId()); +} + /** * Flow 7: a repo-discovered preset with a server description (parsed into * spans, never raw `[text](url)` markup -- see * `features/mcpInstall/lib/descriptionRenderSites.test.ts` for the rule this - * exercises end to end) and one option-constrained parameter. Not yet - * installed anywhere, so it renders as exactly one `mcp-server-row` (the - * "install this preset" row) -- no matching "installed" row exists yet to - * collide with it. + * exercises end to end) and one option-constrained parameter, installed into + * a tracked project. */ export function withParameters(): Scenario { const def: RawMcpServerDef = { @@ -96,8 +173,10 @@ export function withParameters(): Scenario { }; return withScenario({ repositories: [repo()], + projects: [project()], mcpAvailable, responses: { + ...projectsPageStartupResponses(), mcp_installs: [], mcp_description_spans: [SERVER_DESCRIPTION_SPANS, REGION_DESCRIPTION_SPANS], mcp_apply: applied, @@ -105,6 +184,8 @@ export function withParameters(): Scenario { }); } +// -- flows 8, 12: updating an already-installed manual preset's instance ---- + const MANUAL_PRESET_ID = 'preset-github'; /** @@ -127,12 +208,15 @@ function configWithManualPreset(): Scenario['config'] { /** * The one already-installed instance flows 8 and 12 target, at the GLOBAL - * scope (see this file's own doc comment for why). `hash` deliberately never - * matches the live `hashMcpDefInRenderer(manualPreset().def)` output (an - * unfakeable SHA-256 digest computed client-side from the CURRENT def) -- - * that mismatch is exactly what `mcpInstallHasUpdate` reads as "an update is - * available", which is what makes the Update badge (`mcp-update-open`) - * render at all. + * scope (unlike flow 7, no tracked project is needed here: `applyScope` + * resolves the global scope with zero tracked projects, and a manual + * preset's installed-instance row has no per-project "install row" + * duplicate to disambiguate against -- see `mcpTree.tsx`'s doc comment). + * `hash` deliberately never matches the live + * `hashMcpDefInRenderer(manualPreset().def)` output (an unfakeable SHA-256 + * digest computed client-side from the CURRENT def) -- that mismatch is + * exactly what `mcpInstallHasUpdate` reads as "an update is available", + * which is what makes the Update badge (`mcp-update-open`) render at all. */ function installedInstance(): McpInstall { return { @@ -145,12 +229,20 @@ function installedInstance(): McpInstall { }; } +/** The exact row identity flows 8 and 12 must filter by -- computed the same + * way `mcpTree.tsx`'s `manualInstanceLeaves` builds it. */ +export function installedInstanceRowId(): string { + const install = installedInstance(); + return mcpInstalledLeafId('global', instanceKey(localIdentityKey(MANUAL_PRESET_ID), install.instanceName)); +} + /** * Flow 8: the preflight accepts the update but reports `token` (the source's - * placeholder) missing from this instance's stored params -- the counterpart - * to `preflightRefuses` below, exercising the SAME `McpUpdateParamsModal` - * confirm-then-preflight-then-params sequence with an acceptance instead of a - * refusal. + * placeholder) missing from this instance's stored params, opening + * `McpUpdateParamsModal` for exactly that field. Untouched by the fix round: + * this path was already correct (the params modal only ever opens when + * something is missing, so Confirm is pressed once to run it and once more + * to finalize). */ export function updatable(): Scenario { const updated: UpdateMcpResult = { @@ -175,21 +267,36 @@ export function updatable(): Scenario { } /** - * Flow 12: the preflight refuses -- the regression test for the 0.7.0 fix - * "Updating an MCP server no longer deletes it when the new definition - * cannot be installed". `message` is asserted verbatim by the spec, so it is - * supplied by the caller rather than fixed here. + * Flow 12: the 0.7.0 regression case -- codex cannot express an http + * transport. This is NOT a preflight refusal: `mcp_update_preflight` + * (`preflight_inner`, `src-tauri/src/commands/mcp/update.rs`) only ever + * checks for MISSING PARAMS, never transport/oauth support, so it reports + * `ok: true` with nothing missing and `startMcpUpdateAsync` proceeds + * straight to `updateMcp` -- no params modal opens at all. The actual + * transport check happens inside `mcp_update` itself (`update_inner`): + * codex's write for this instance is SKIPPED (`reason: 'transport'`), and + * `update_inner` `continue`s BEFORE `remove_mcp_instance` for that agent -- + * which is the literal mechanism the 0.7.0 fix guarantees (the instance is + * never removed for an agent whose write it already knows will fail). + * `updateMcp`'s own top-level result is still `ok: true` (the call as a + * whole succeeded; only codex's entry is reported skipped) -- see + * `task-8-report.md`'s fix-round notes for why the original version of this + * fixture (an `ok: false` preflight) was unreachable for this cause. */ -export function preflightRefuses(message: string): Scenario { +export function transportSkipped(): Scenario { + const updated: UpdateMcpResult = { + ok: true, + updated: [], + skipped: [{ agent: 'codex', source: 'github', reason: 'transport', transport: 'http' }], + }; return withScenario({ config: configWithManualPreset(), mcpInstalls: [installedInstance()], responses: { mcp_installs: [installedInstance()], - // See `updatable()`'s identical override just above for why this is - // never actually read. mcp_description_spans: [], - mcp_update_preflight: { ok: false, error: message }, + mcp_update_preflight: { ok: true, missingParams: [] }, + mcp_update: updated, }, }); } diff --git a/e2e/desktop/tests/mcp.spec.ts b/e2e/desktop/tests/mcp.spec.ts index 472ec76e..328088e0 100644 --- a/e2e/desktop/tests/mcp.spec.ts +++ b/e2e/desktop/tests/mcp.spec.ts @@ -1,45 +1,40 @@ /** * Flows 7, 8 and 12 (MCP pages): installing a preset with a description and - * an option-constrained parameter, updating an installed instance whose - * preflight accepts it, and updating one whose preflight refuses it -- the - * regression test for the 0.7.0 fix "Updating an MCP server no longer - * deletes it when the new definition cannot be installed" (see - * `.superpowers/sdd/2026-09-09-desktop-ui-e2e/task-8-brief.md`, and - * `../fixtures/mcp.ts`'s own doc comment for why every scenario here targets - * the Global scope rather than a tracked project). + * an option-constrained parameter into a tracked project, updating an + * installed instance whose preflight reports a missing parameter, and + * updating one whose update SKIPS an agent that cannot express its + * transport -- the regression test for the 0.7.0 fix "Updating an MCP + * server no longer deletes it when the new definition cannot be installed" + * (see `.superpowers/sdd/2026-09-09-desktop-ui-e2e/task-8-brief.md`, and + * `task-8-report.md`'s fix-round notes for why flow 12 is NOT a preflight + * refusal -- that mechanism cannot occur for this cause). * * MCP is a two-level sidebar group exactly like Skills (see `App.tsx`'s - * `NAV_ITEMS` comment): `nav-group-mcp` must be expanded before either of its - * two sub-items (`nav-mcp-components`, `nav-mcp-management`) is clickable. - * All three flows below use `nav-mcp-management` -- the page whose Install - * and Update badges (`useMcpActions`) carry this task's testids. + * `NAV_ITEMS` comment): `nav-group-mcp` must be expanded before either of + * its two sub-items (`nav-mcp-components`, `nav-mcp-management`) is + * clickable. All three flows below use `nav-mcp-management` -- the page + * whose Install and Update badges (`useMcpActions`) carry this task's + * testids. */ import { test, expect } from '../harness/fixture'; -import { withParameters, updatable, preflightRefuses } from '../fixtures/mcp'; +import type { Page } from '@playwright/test'; +import { + withParameters, + withParametersInstallRowId, + updatable, + transportSkipped, + installedInstanceRowId, +} from '../fixtures/mcp'; -test.describe('an mcp update the agent cannot express', () => { - test.use({ scenario: preflightRefuses('codex cannot express the http transport') }); - - test('leaves the instance alone', async ({ app, page }) => { - await app.goto(); - await page.getByTestId('nav-group-mcp').click(); - await page.getByTestId('nav-mcp-management').click(); - await expect(page.getByTestId('mcp-page')).toBeVisible(); - - const row = page.getByTestId('mcp-server-row').filter({ - has: page.locator('[data-mcp-name="github"]'), - }); - await expect(row).toBeVisible(); - - await row.getByTestId('mcp-update-open').click(); - await page.getByTestId('mcp-update-submit').click(); - - await expect(page.getByTestId('mcp-update-error')).toContainText('cannot express'); - // The point of the 0.7.0 fix: the removal must not have happened. - await expect(row).toBeVisible(); - expect(await app.calls('mcp_update')).toHaveLength(0); - }); -}); +/** + * A `mcp-server-row` identified by its own (now-unique) `data-mcp-name` + * tree-node id -- see `pages/Mcp/lib/mcpTree.tsx`'s "ROW IDENTITY" comment + * and `fixtures/mcp.ts`'s `withParametersInstallRowId`/ + * `installedInstanceRowId` for why this is an id, not a bare preset name. + */ +function mcpRow(page: Page, rowId: string) { + return page.getByTestId('mcp-server-row').filter({ has: page.locator(`[data-mcp-name="${rowId}"]`) }); +} test.describe('installing an mcp server with a description and an option parameter', () => { test.use({ scenario: withParameters() }); @@ -50,13 +45,18 @@ test.describe('installing an mcp server with a description and an option paramet await page.getByTestId('nav-mcp-management').click(); await expect(page.getByTestId('mcp-page')).toBeVisible(); - // The Global scope root is already expanded (it is a tree root, per - // `ManagementPage.tsx`'s `rootIds(baseTree)` seed); the repo node nested - // under it is not -- mirroring `skills.spec.ts`'s "browsing skills" flow, - // expanding it reveals the preset's install row. - await page.getByText('mcp-repo', { exact: true }).click(); - - const row = page.getByTestId('mcp-server-row').filter({ has: page.locator('[data-mcp-name="github"]') }); + // The tracked project's own root is already expanded (it is a tree + // root); the repo node nested under it is not -- mirroring + // `skills.spec.ts`'s "browsing skills" flow, expanding it reveals the + // preset's install row. Global's own root ALSO shows a copy of the same + // repo/preset (every repo preset gets an install row per scope shown), + // so the click is scoped to the "Demo" project's own branch -- its + // `[role="treeitem"]` is the only one whose (accumulated, nested) text + // contains "Demo" at all. + const demoRoot = page.locator('[role="treeitem"]').filter({ hasText: 'Demo' }); + await demoRoot.getByText('mcp-repo', { exact: true }).click(); + + const row = mcpRow(page, withParametersInstallRowId()); await row.getByTestId('mcp-install-open').click(); const modal = page.getByTestId('mcp-install-modal'); @@ -82,7 +82,7 @@ test.describe('installing an mcp server with a description and an option paramet // filter combobox is also on screen, and a substring match on "Project" // would otherwise resolve to both. await modal.getByRole('combobox', { name: 'Project', exact: true }).click(); - await page.getByRole('option', { name: 'Global' }).click(); + await page.getByRole('option', { name: 'Demo' }).click(); // The native checkbox input is visually hidden (`Checkbox.scss`'s // `.sk-checkbox__input`, zero-size + opacity 0 -- the styled box is a // sibling), so it never becomes Playwright-"visible" itself; clicking its @@ -105,7 +105,7 @@ test.describe('installing an mcp server with a description and an option paramet }); }); -test.describe('updating an mcp server with a passing preflight', () => { +test.describe('updating an mcp server with a missing parameter', () => { test.use({ scenario: updatable() }); test('a missing parameter is asked for and the update proceeds', async ({ app, page }) => { @@ -114,26 +114,24 @@ test.describe('updating an mcp server with a passing preflight', () => { await page.getByTestId('nav-mcp-management').click(); await expect(page.getByTestId('mcp-page')).toBeVisible(); - const row = page.getByTestId('mcp-server-row').filter({ has: page.locator('[data-mcp-name="github"]') }); + const row = mcpRow(page, installedInstanceRowId()); await expect(row).toBeVisible(); + // The preflight runs eagerly, before any modal opens (see + // `useMcpActions.tsx`'s `startMcpUpdateAsync`); it reports the source's + // new `{token}` placeholder missing from this instance's stored params, + // which is what opens `McpUpdateParamsModal` with exactly that field. await row.getByTestId('mcp-update-open').click(); - await expect(page.getByTestId('mcp-update-modal')).toBeVisible(); - // The first Confirm press is what runs the preflight (see - // `McpUpdateParamsModal`'s own doc comment); it reports the source's new - // `{token}` placeholder missing from this instance's stored params. - await page.getByTestId('mcp-update-submit').click(); + const modal = page.getByTestId('mcp-update-modal'); + await expect(modal).toBeVisible(); const tokenField = page.getByTestId('mcp-param-input').filter({ has: page.locator('[data-param-name="token"]') }); await expect(tokenField).toBeVisible(); await tokenField.locator('input').fill('secret-token'); - // The second press confirms with the filled-in value. await page.getByTestId('mcp-update-submit').click(); - - await expect(page.getByTestId('mcp-update-modal')).toBeHidden(); - await expect(page.getByTestId('mcp-update-error')).toHaveCount(0); + await expect(modal).toBeHidden(); const calls = await app.calls('mcp_update'); expect(calls).toHaveLength(1); @@ -141,3 +139,37 @@ test.describe('updating an mcp server with a passing preflight', () => { expect(updates[0]?.values).toEqual({ token: 'secret-token' }); }); }); + +test.describe('an mcp update the agent cannot express', () => { + test.use({ scenario: transportSkipped() }); + + test('leaves the instance alone and reports why', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-group-mcp').click(); + await page.getByTestId('nav-mcp-management').click(); + await expect(page.getByTestId('mcp-page')).toBeVisible(); + + const row = mcpRow(page, installedInstanceRowId()); + await expect(row).toBeVisible(); + + // The preflight accepts the update outright (nothing is missing -- + // `preflight_inner` never checks transport support), so no params modal + // opens at all: the click runs the update straight through. + await row.getByTestId('mcp-update-open').click(); + + // `updateMcp` succeeds overall but skips codex specifically (`reason: + // 'transport'`); `runMcpUpdate` (`useMcpActions.tsx`) turns that into an + // info-level `notify`, which surfaces as a toast -- there is no modal in + // this path at all, so there is nothing to press Confirm on. + const toast = page.getByTestId('mcp-update-error'); + await expect(toast).toContainText('Codex'); + await expect(toast).toContainText('http'); + + // The point of the 0.7.0 fix: `update_inner` skips codex's write and + // `continue`s BEFORE `remove_mcp_instance` for it, so the instance is + // never removed -- the row must still be there afterward. + await expect(row).toBeVisible(); + + expect(await app.calls('mcp_update')).toHaveLength(1); + }); +}); From 09b10d3a824d0c1097a958b56d56eaee5f33f92e Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 04:52:04 +0200 Subject: [PATCH 16/25] test: prove the unmocked-command guard can actually fail Add an expectUnmocked fixture option so one spec can name exactly which commands the harness's "nothing went unmocked" guard should record instead of failing the test outright, then write the spec that uses it: an add-repository flow with repositories_add left genuinely unmocked, asserting both the on-screen error and the recorded message carry UNKNOWN_COMMAND_PREFIX plus the command name. Every other spec keeps asserting emptiness unchanged, and the hatch requires an exact match (wrong count or an empty recording both still fail), so it cannot become a place future silence hides. --- e2e/desktop/fixtures/base.ts | 92 +++++++++++++++++++++++++-- e2e/desktop/harness/fixture.ts | 2 +- e2e/desktop/harness/installHarness.ts | 28 +++++--- e2e/desktop/tests/harness.spec.ts | 50 +++++++++++++++ 4 files changed, 155 insertions(+), 17 deletions(-) diff --git a/e2e/desktop/fixtures/base.ts b/e2e/desktop/fixtures/base.ts index f10a8740..681beb58 100644 --- a/e2e/desktop/fixtures/base.ts +++ b/e2e/desktop/fixtures/base.ts @@ -22,6 +22,12 @@ interface RecordedCall { readonly args: unknown; } +/** One entry of `window.__SKK_E2E_UNMOCKED__` -- see `installHarness.ts`. */ +export interface UnmockedCommand { + readonly cmd: string; + readonly message: string; +} + /** The spec-facing handle onto one test's scripted backend. */ export interface App { /** @@ -77,6 +83,24 @@ export interface App { /** Every clipboard write recorded so far (see `installHarness.ts`'s * clipboard stub), in write order. */ clipboard(): Promise; + /** + * Every command the scripted backend could not answer so far, in call + * order, as recorded by `installHarness.ts`'s mocked `invoke` (see + * `window.__SKK_E2E_UNMOCKED__`). Reading this directly -- rather than only + * relying on the fixture's own post-test guard below -- is how a spec + * proves what the guard actually captured: the `cmd` name AND the exact + * `message` the thrown `Error` carried, so an assertion against + * `UNKNOWN_COMMAND_PREFIX` (`harness/commands.ts`) is checking the real + * runtime string, not a value re-derived to match it. + * + * Every other spec in this suite leaves this empty for its whole run -- + * the fixture's own teardown (below) fails the test the instant anything + * goes unmocked, so there is nothing left here to read afterwards. Calling + * this is only meaningful in a spec that also sets `expectUnmocked`, the + * one escape hatch that tells that teardown to expect specific commands + * here instead of failing on them. + */ + unmocked(): Promise; } /** @@ -150,19 +174,64 @@ function buildApp(page: Page): App { () => ((window as unknown as Record).__SKK_E2E_CLIPBOARD__ ?? []) as string[], ); }, + async unmocked() { + return page.evaluate( + () => ((window as unknown as Record).__SKK_E2E_UNMOCKED__ ?? []) as UnmockedCommand[], + ); + }, }; } +/** True when `actual` and `expected` name exactly the same commands, ignoring + * order (repeated calls to the same never-mocked command all belong to one + * name) but NOT count of distinct names or membership -- unlike a subset or + * "at least" check, `['a']` against a recorded `['a', 'b']` is a mismatch, + * and so is `['a']` against a recorded `[]`. Used only by `expectUnmocked`'s + * teardown check below; see that option's doc comment for why an exact match + * is the point. */ +function sameCommandNames(actual: readonly string[], expected: readonly string[]): boolean { + const a = [...new Set(actual)].sort(); + const b = [...new Set(expected)].sort(); + return a.length === b.length && a.every((name, i) => name === b[i]); +} + interface Fixtures { /** The backend data for this test. Override per spec (or per describe * block) with `test.use({ scenario: withScenario({ ... }) })`. */ scenario: Scenario; + /** + * The exact set of command names this test expects `__SKK_E2E_UNMOCKED__` + * to hold once the test body finishes -- default `[]`, i.e. every ordinary + * spec still asserts the array is empty, unchanged from before this option + * existed. + * + * This is the ONLY way to stop the `app` fixture's teardown (below) from + * failing a test over an unmocked command; it is deliberately not a + * permissive "allow list" -- the teardown requires the recorded command + * names to match `expectUnmocked` EXACTLY (same names, same count; see + * `sameCommandNames`), so `expectUnmocked: ['x']` still fails the test if + * the harness recorded `['x', 'y']` (a second, unrelated command also went + * unmocked -- the hatch does not launder that away) or recorded nothing at + * all (the guard silently stopped firing -- the exact failure mode this + * option exists to make demonstrable, see `harness.spec.ts`'s "a command + * with no scripted answer" describe block). A hatch that only suppressed + * failure, rather than requiring an exact match, would itself become a + * place a future regression could hide unnoticed -- which is precisely the + * silence this suite's other assertions exist to rule out. + * + * Override per spec (or per describe block) with + * `test.use({ expectUnmocked: ['command_name'] })`, exactly like `scenario` + * above. Read what was actually recorded (both the command name and the + * exact message text) via `app.unmocked()`. + */ + expectUnmocked: readonly string[]; app: App; } export const test = base.extend({ scenario: [defaultScenario(), { option: true }], - app: async ({ page, scenario }, use) => { + expectUnmocked: [[], { option: true }], + app: async ({ page, scenario, expectUnmocked }, use) => { await installHarness(page, scenario); await installAnimationZeroing(page); await use(buildApp(page)); @@ -174,12 +243,21 @@ export const test = base.extend({ // or the renderer started calling a new one this harness has not caught // up with -- either way, a passing test that hit this is the wrong // outcome, not a flake to retry away (see `playwright.config.ts`'s - // `retries: 0`). - const unmocked = await page.evaluate( - () => (window as unknown as Record).__SKK_E2E_UNMOCKED__ as string[] | undefined, - ); - if (unmocked && unmocked.length > 0) { - throw new Error(`e2e harness: unmocked command(s) reached the backend: ${unmocked.join(', ')}`); + // `retries: 0`). `expectUnmocked` (default `[]`) is the one escape hatch, + // and it is checked for an EXACT match, not merely "at most these" -- see + // that option's own doc comment above. + const unmocked = ((await page.evaluate( + () => (window as unknown as Record).__SKK_E2E_UNMOCKED__ as UnmockedCommand[] | undefined, + )) ?? []) as UnmockedCommand[]; + const commandNames = unmocked.map((entry) => entry.cmd); + if (!sameCommandNames(commandNames, expectUnmocked)) { + if (expectUnmocked.length === 0) { + throw new Error(`e2e harness: unmocked command(s) reached the backend: ${commandNames.join(', ')}`); + } + throw new Error( + `e2e harness: expected exactly [${expectUnmocked.join(', ')}] to go unmocked, ` + + `but recorded [${commandNames.join(', ')}]`, + ); } }, }); diff --git a/e2e/desktop/harness/fixture.ts b/e2e/desktop/harness/fixture.ts index b1217ff6..4e39afb8 100644 --- a/e2e/desktop/harness/fixture.ts +++ b/e2e/desktop/harness/fixture.ts @@ -9,4 +9,4 @@ * split. */ export { test, expect } from '../fixtures/base.js'; -export type { App } from '../fixtures/base.js'; +export type { App, UnmockedCommand } from '../fixtures/base.js'; diff --git a/e2e/desktop/harness/installHarness.ts b/e2e/desktop/harness/installHarness.ts index c60969a4..8b92d76e 100644 --- a/e2e/desktop/harness/installHarness.ts +++ b/e2e/desktop/harness/installHarness.ts @@ -75,12 +75,18 @@ export async function installHarness(page: Page, scenario: Scenario): Promise).__SKK_E2E_CLIPBOARD__ = []; - // Every command name the callback below could not answer, in call order. - // `store.loadAll` and several call sites swallow a rejection (into - // `store.error`, a caught background-task status, or a `.then(ok, () => - // undefined)`), so an unmocked command does not reliably surface anywhere - // a test's DOM assertions can see it -- a test that needs to know reads - // this array directly instead. + // Every unmocked command the callback below could not answer, in call + // order, as `{ cmd, message }` -- `message` is the exact string the + // thrown `Error` carried (`${unknownCommandPrefix}${cmd}`), kept + // alongside the bare name rather than reconstructed by a reader, so a + // spec asserting on it (Task 9's `harness.spec.ts` demonstration) is + // checking what the harness actually produced, not restating the same + // template a second time. `store.loadAll` and several call sites swallow + // a rejection (into `store.error`, a caught background-task status, or a + // `.then(ok, () => undefined)`), so an unmocked command does not reliably + // surface anywhere a test's DOM assertions can see it -- a test that + // needs to know reads this array directly (via `fixture.ts`'s + // `app.unmocked()`) instead. (window as unknown as Record).__SKK_E2E_UNMOCKED__ = []; const label = arg.scenario.windowLabel || 'main'; (window as unknown as Record).__SKK_E2E_WINDOW_LABEL__ = label; @@ -114,9 +120,13 @@ export async function installHarness(page: Page, scenario: Scenario): Promise).__SKK_E2E_UNMOCKED__ as string[]; - unmocked.push(cmd); - throw new Error(`${arg.unknownCommandPrefix}${cmd}`); + const unmocked = (window as unknown as Record).__SKK_E2E_UNMOCKED__ as { + cmd: string; + message: string; + }[]; + const message = `${arg.unknownCommandPrefix}${cmd}`; + unmocked.push({ cmd, message }); + throw new Error(message); } return arg.responses[cmd]; }, diff --git a/e2e/desktop/tests/harness.spec.ts b/e2e/desktop/tests/harness.spec.ts index 8b684328..19d5dd40 100644 --- a/e2e/desktop/tests/harness.spec.ts +++ b/e2e/desktop/tests/harness.spec.ts @@ -24,9 +24,19 @@ * the way `@tauri-apps/plugin-clipboard-manager`'s `writeText` builds it -- * from inside the page, entirely independent of `fixture.ts`'s own * implementation, so a wrong shape on either side surfaces here. + * + * Task 9 adds one more self-test, in the final `describe` block below: proof + * that `fixtures/base.ts`'s "nothing went unmocked" guard can actually fail. + * Every other spec in this suite only ever exercises the guard's PASSING + * path (no scenario has ever left a command it actually exercises + * unanswered on purpose), so without this the guard's failing path would be + * verified by hand only -- exactly the kind of silent regression this + * suite's own stated purpose (a scripted backend, not a decorative one) + * exists to rule out. */ import { test, expect } from '../harness/fixture'; import { withScenario } from '../harness/scenario'; +import { UNKNOWN_COMMAND_PREFIX } from '../harness/commands'; test.use({ scenario: withScenario({ @@ -111,3 +121,43 @@ test('app.clipboard records a write made through the clipboard plugin', async ({ const clipboard = await app.clipboard(); expect(clipboard).toEqual(['copied-by-self-test']); }); + +test.describe('a command with no scripted answer', () => { + // The default scenario (unchanged here) never gives `repositories_add` a + // response -- see `fixtures/repositories.ts`'s doc comment: it is not part + // of `store.loadAll`'s startup round trip, so `defaultScenario()` leaves it + // genuinely unmocked on purpose, exactly like every other add/update/apply + // command a spec must supply its own answer for. Driving the add form + // without doing that is this test's whole point, so `expectUnmocked` names + // it instead of a scenario override supplying one. + test.use({ expectUnmocked: ['repositories_add'] }); + + test('is recorded by name and message, not silently accepted', async ({ app, page }) => { + await app.goto(); + await page.getByTestId('nav-repositories').click(); + await page.getByTestId('repo-add-button').click(); + await page.getByTestId('repo-add-url').fill('https://example.invalid/unmocked.git'); + await page.getByTestId('repo-add-submit').click(); + + // `RepoAddButton`'s submit handler (`features/repoAdd/ui/RepoAddButton. + // tsx`) catches `addRepository`'s rejection and renders `err.message` + // verbatim -- so if the harness ever stopped throwing a named error here + // (e.g. resolved `undefined` instead), this banner would either never + // appear or would stop naming the command, and this assertion would fail + // for that reason rather than passing on a coincidence. + await expect(page.getByTestId('repo-add-error')).toContainText(UNKNOWN_COMMAND_PREFIX + 'repositories_add'); + + // The authoritative check: exactly one command went unmocked, and its + // recorded message is the real `UNKNOWN_COMMAND_PREFIX` constant (not a + // hardcoded copy of today's string) followed by the offending command's + // own name -- so renaming or reformatting that constant without updating + // every place it is produced breaks this assertion instead of passing + // quietly. `expectUnmocked: ['repositories_add']` above only stops the + // fixture's teardown from failing the test over this EXACT name; a + // second, unrelated command going unmocked at the same time (or none at + // all) would still fail it (see that option's doc comment in + // `fixtures/base.ts`). + const unmocked = await app.unmocked(); + expect(unmocked).toEqual([{ cmd: 'repositories_add', message: UNKNOWN_COMMAND_PREFIX + 'repositories_add' }]); + }); +}); From 3be4defa3d3d82368f6dcbb54f341b918222677c Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Wed, 9 Sep 2026 04:58:09 +0200 Subject: [PATCH 17/25] style: run prettier over drifted renderer and e2e files --- .../features/repoAdd/ui/RepoAddButton.tsx | 7 +------ .../renderer/pages/Settings/SettingsPage.tsx | 6 +----- .../renderer/pages/Skills/ComponentsPage.tsx | 8 +++++++- .../renderer/shared/ui/Sidebar/SidebarItem.tsx | 9 +-------- e2e/desktop/fixtures/skills.ts | 6 +++++- e2e/desktop/harness/scenario.ts | 18 +++++++++++++++--- e2e/desktop/tests/harness.spec.ts | 4 +--- e2e/desktop/tests/skills.spec.ts | 4 +--- 8 files changed, 32 insertions(+), 30 deletions(-) diff --git a/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx b/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx index bc43b5cf..f1e66c6b 100644 --- a/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx +++ b/apps/desktop/src/renderer/features/repoAdd/ui/RepoAddButton.tsx @@ -157,12 +157,7 @@ export function RepoAddButton() { -
    diff --git a/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx b/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx index 70868334..d3ad1fba 100644 --- a/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx +++ b/apps/desktop/src/renderer/pages/Settings/SettingsPage.tsx @@ -184,11 +184,7 @@ export function SettingsPage() {
    - + {/* The cadence belongs on the row, not in a section footer: the row would otherwise be an empty expanse with a button pinned to its right edge, and the explanation would float loose underneath diff --git a/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx b/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx index 9bcb07f0..bd9384d3 100644 --- a/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx +++ b/apps/desktop/src/renderer/pages/Skills/ComponentsPage.tsx @@ -209,7 +209,13 @@ export function SkillsComponentsPage() { , - , ] diff --git a/apps/desktop/src/renderer/shared/ui/Sidebar/SidebarItem.tsx b/apps/desktop/src/renderer/shared/ui/Sidebar/SidebarItem.tsx index d7650a94..38955d88 100644 --- a/apps/desktop/src/renderer/shared/ui/Sidebar/SidebarItem.tsx +++ b/apps/desktop/src/renderer/shared/ui/Sidebar/SidebarItem.tsx @@ -17,14 +17,7 @@ export interface SidebarItemProps { readonly 'data-testid'?: string; } -export function SidebarItem({ - icon, - children, - active, - onClick, - className, - 'data-testid': testId, -}: SidebarItemProps) { +export function SidebarItem({ icon, children, active, onClick, className, 'data-testid': testId }: SidebarItemProps) { return (