test: cover the desktop UI with end-to-end flows - #60
Merged
Conversation
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.
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.
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-<view> 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.
The add form inferred success from any repository row matching the submitted URL, which a pre-existing duplicate satisfies just as well as a genuine new row -- so the backend's "duplicate" error (the most reachable real failure) closed the modal silently instead of showing repo-add-error. Require the matched row's id to be new since the submit started; add a regression spec and its duplicateFails scenario.
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.
Add a desktop-e2e job that installs Chromium and runs the Playwright suite, add a prettier check to the existing ci job (never run in CI before), and widen test:e2e to chain the CLI and desktop suites. Document both suites under the end-to-end section.
The e2e job ran the widened `pnpm test:e2e`, which now also drives Playwright, but that job never installs Chromium -- only the new desktop-e2e job does. Run `pnpm test:e2e:cli` there instead, so the desktop half runs exactly once, in the job that sets it up. Also fix a comment on the Playwright browser cache key: it says the key is scoped to the Playwright version, but it hashes the whole lockfile on purpose, to avoid a version-parsing gap silently reusing a stale browser binary.
check-fixture-repo/SKILL.md still told the pre-release gate to run the widened `pnpm test:e2e`, which now also drives Playwright -- not what a fixture-repo check needs. Point it at `pnpm test:e2e:cli`, fix its stale "51 tests" expectation to the current 67, and add the two mcp-oauth/mcp-parameters specs missing from its per-spec table. AGENTS.md and the CLI harness's build-check error carried the same stale command; repoint those too, and note the desktop suite's existence in AGENTS.md where the CLI suite is documented.
Reset submitting on cancel so dismissing an in-flight add no longer leaves the Add button disabled forever, and key the post-submit error lookup by the last notification's id instead of the log's length so a full, capped log no longer swallows a genuine failure.
Toasts is generic cross-cutting UI, so its testid should name the kind
("toast"), not one caller's identity -- rename data-mcp-update-error
to data-testid=toast and let assertions scope by text instead. Give
skill-tree leaves and groups their own unique node id as the identity
attribute value, matching mcpTree.tsx's rule, instead of a bare name
or repo-relative path that two repositories (or Global vs. a project)
can legitimately share.
Assert mcp_apply's payload carries the chosen "Europe" option instead of only counting the call, so a Select that always submitted its first option would fail. Assert the agent-count badge's actual text so agentCount: 2 is read back, not just its visibility. Rewrite a comment that claimed proof of the instance surviving mcp_update: the scenario's fixed mcp_installs answer cannot falsify that; the row being visible only proves the skip message and call count, and the survival claim is covered by update_inner's own Rust tests instead.
Post-split, "only pnpm test:e2e needs it" misleads: test:e2e:desktop does not touch the fixture submodule at all.
Ten fixture and spec headers cited paths under `.superpowers/`, which is git-ignored. For anyone but the author those references resolve to nothing, so they were a dead end sitting at the top of the files a reader opens first. The prose around them was already self-contained: which flows the file covers, why a command carries no default answer, how row identity works, which sidebar shape the page has. Only the pointers are gone; nothing that explained a decision was removed. Where a reference carried real content -- why flow 12 is not a preflight refusal -- that content is now stated inline instead of cited. The remaining `.superpowers` mentions are deliberate and predate this branch: the entries in .gitignore, .dockerignore and .prettierignore are load-bearing, and AGENTS.md documents the workflow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a Playwright suite that drives the desktop renderer through twelve documented user flows, splits
e2e/into a CLI half and a desktop half, and gates the result in CI.Why not drive the app itself
Playwright cannot drive a Tauri window -- the webview is WKWebView on macOS and WebKitGTK on Linux, and neither speaks the DevTools Protocol Playwright attaches to. Tauri's own answer is WebdriverIO with
@wdio/tauri-service, but its documentation states that withtauri-driver"only Windows and Linux are supported on desktop, as macOS has no WKWebView driver tool available", and the service was published three days before this work began. That is a days-old dependency in the path of a blocking gate, on a platform matrix needing a second mechanism for one of its three members.What makes the alternative work is that the renderer's Tauri surface is four files. Ninety-eight files reach the backend and every one goes through
bridgeClient; below it the app is an ordinary React SPA.The seam is one layer below
bridgeClient, not at it. The scripted backend replacesinvoke/listenthroughmockIPC, soservices/bridge/client.ts-- where the 69 command names and their argument shapes are written down -- stays under test. SwappingbridgeClientfor a fake would have dropped that file out of coverage entirely and let a renderer that had drifted from the Rust contract still pass.Isolation is structural, not configured. With
mockIPCthere is no filesystem, no git, no network. The suite cannot damage a developer's configuration because it has no access to it.What it covers
Twelve flows from
docs/getting-started.md,docs/usage/mcp.md,docs/usage/projects.mdanddocs/usage/skills-and-hooks.md: adding a repository and a repository that will not clone; browsing, installing, and skill dependencies; tracking a project and a project whose folder is gone; Settings sections and an offered update; MCP install with parameters, a parameter update, and an update the agent cannot express.Determinism
retries: 0, locally and in CI. A retry converts a flake into a slow pass and throws away the signal.Full gate green:
pnpm lint,typecheck,test:cov(1080/1080),format:check,cargo fmt --check,cargo clippy -D warnings,cargo test --workspace(1440/1440),pnpm test:e2e(CLI 67/67 + desktop 20/20),frontend:build.Defects found and fixed along the way
The work surfaced six real problems in shipped code, none of them in the tests:
RepoAddButtoninferred success by URL membershipduplicateerror rendered as success -- modal closed, error never shownsubmitting.sk-page__body's flex layout.sk-list-footer'smargin-top: autostopped pinning; only visible while filteringskillTree.tsxused bare skill names as row identityterminal_startandapp_update_checkswallow their rejections at bootterminal_startalso removed a ~1.5s background retry chain on every launchThe last one was found by the harness itself: an assertion that no backend command goes unmocked, which fires after every test.
Three assertions that could not fail
Caught during review and fixed or documented: a Settings validity check that passed for every spec in the suite because the harness hardcoded validity; the MCP regression test built on a preflight refusal that cannot occur (
preflight_inneronly checks missing parameters -- an inexpressible transport returns askippedentry frommcp_update); and flow 12's "the row survives" assertion, which the harness answers unconditionally. The first two are fixed. The third now states plainly what it proves -- the skip message and the singlemcp_updatecall -- and what it does not; instance survival is covered byupdate_inner's own Rust tests. Closing it needs a sequenced-response mechanism in the harness, which is a follow-up.CI
A fourth job,
Desktop UI (Playwright), blocking but deliberately not added to branch protection yet -- that is a separate step once the suite has some history. Also adds a prettier gate to the existingcijob:pnpm format:checkexisted as a script but no workflow ran it, which is why eight files had drifted unnoticed.The pre-existing end-to-end job now runs
pnpm test:e2e:clirather than the widenedpnpm test:e2e, which would otherwise have invoked Playwright in a job with no Chromium.Notes
CHANGES.mdentry: a test suite is not user-visible perCONTRIBUTING.md. Thedata-testidattributes are inert in the shipped bundle.