test(dashboard): playwright smoke harness + PR check#981
Conversation
|
🤖 Claude Code Review Status: Complete No issues found. This PR adds comprehensive Playwright smoke tests for all dashboard routes with proper CI integration. Review Summary: The implementation is solid:
Previously Addressed:
No action required. |
|
@oskarszoon Thanks for the clarification on the WebSocket mock (in reply to inline comment on ws.ts:8)! You're correct — Playwright's routeWebSocket automatically completes the handshake in mock-only mode without requiring connectToServer(). The empirical evidence (all smokes passing with no WS errors) confirms this is working as intended. Issue resolved. |
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-29 08:54 UTC |
…cripts, local playwright binary)
ReviewNice harness — the fork-safe CI in particular is carefully done. One confirmed blocker before merge, plus some cleanup. 🔴 Blocker: the new wrapper
|
| Content | Before (card is direct child) | After (bare wrapper) |
|---|---|---|
Sparse (e.g. peers returns [], like the smoke fixtures) |
1031px — fills | 105px — collapses to a centered sliver |
| Wide table (prod, 64-char hashes) | 1031px — constrained, scrolls internally | 1390px — overflows the 1213px container → horizontal scrollbar |
So it breaks in both directions depending on content width. Critically, the smoke harness this PR adds cannot detect it — page-root is still "visible" and there are no console errors, so the tests stay green on a visibly broken page (which is also why "12 passed locally" didn't surface it).
Fix (verified): give each new wrapper width: 100%:
<div data-testid="page-root" style="width: 100%">With that, sparse and wide both render at 1031px — identical to pre-PR. (display: contents is not an option: Playwright treats a box-less element as not visible, so toBeVisible() would fail.) The seven pages that reused an existing styled div (admin, home, forks, p2p, settings, login, wstest) are unaffected — no change needed there.
🟡 Convention
The codebase already uses data-test-id (hyphenated) 20×, and PageWithMenu exposes a testId prop that emits it onto a full-width .content-container. This PR introduces data-testid (no hyphen, 0 prior uses). Worth aligning the spelling — and for the seven PageWithMenu pages, <PageWithMenu testId="…"> would have provided the hook on an already-full-width element, sidestepping the wrapper issue entirely (login/wstest don't use PageWithMenu, so they'd still need their own).
🟡 Test quality
src/routes/+page.sveltejust renders<Home />, so the/test and the standalonesmoke: /homedescribe exercise the same component, and/homeduplicates theROUTESloop body — fold it intoROUTES.- The click-through's final assertion reuses
[data-testid="page-root"], which also exists on the list page it navigated from — weak proof the detail page rendered, plus a strict-mode multi-match risk if both roots are briefly mounted during transition. Prefer a detail-unique selector (e.g. the block-hash heading).
⚪ Nits
- Allowlist has a dead entry (
Download the React DevTools— that's React, not Svelte) and an over-broad one (bareechartssubstring would also swallow a genuine ECharts error). waitForTimeout(1000)×13 is a hard sleep (~13s of pure wait); fine for a late-console window but inherently soft.- Playwright browser cache has no
restore-keys, so any lockfile change forces a full re-download. - The
npm ci --ignore-scriptspath is only exercised in CI, not in the localtest:integrationverification — it'll first be proven on this PR's own run.
What's solid
- Fork-safe CI:
pull_request(notpull_request_target),permissions: contents: read, all actions SHA-pinned,--ignore-scripts, direct./node_modules/.bin/playwrightinvocation. Model example. - Catch-all-first mock ordering correctly matches Playwright's LIFO resolution.
- The
blocksregex correctly excludesblockstatsvia the(?:\?|$)anchor. - Lockfile is consistent — base already resolves
@playwright/test@1.57.0, which satisfies the new^1.55.0floor, sonpm ciis fine. - Checked one concern and cleared it: vitest won't try to run the Playwright
.spec.tsfiles —vite.config.tsscopestest.includetosrc/**, and the specs live intests/.
Verdict
One confirmed blocker — the wrapper divs regress layout on 5 production pages, undetectable by the harness being added. It's a two-line-per-file fix (width: 100% on each new wrapper), verified to restore original rendering. Everything else is convention/cleanup. Fix the wrappers and this is a strong addition.
…test-id Addresses review on bsv-blockchain#981: - Drop the new wrapper <div>s on 5 pages that regressed layout (the bare wrappers became shrink-to-fit flex items, collapsing or overflowing the cards they wrapped). PageWithMenu already exposes a testId prop that emits data-test-id onto its full-width .content-container — use that. - Migrate all 10 PageWithMenu consumers to the testId prop. login/wstest (no PageWithMenu) keep their existing styled element; rename to the repo's data-test-id convention. - Fold /home into the ROUTES loop (it shared a component with /). - Click-through assertion now waits for the block hash text on the detail page — a detail-page-unique signal instead of the reused page-root. - Trim console allowlist: drop "Download the React DevTools" (not Svelte) and the bare "echarts" substring (too broad). - Add restore-keys to the Playwright browser cache so unrelated lockfile bumps don't force a full re-download.
|
Thanks for the layout catch — that was a real bug the harness couldn't see itself. Pushed Blocker — wrapper layout regression: Migrated all 10 Convention: All Test quality:
Nits:
|
…llowlist Addresses review findings on bsv-blockchain#981: - Add subtree/tx/utxo detail smokes (direct nav with fixtures). The click-through only covered /viewer/block; the other three /viewer/[type] components use the same Svelte 4 patterns the runes migration targets, so the net now guards all four detail types. - Add subtree/tx/txmeta mock routes + fixtures (disjoint regexes; raw bodies the api client wraps in {ok, data}). - Fix block-detail.json merkleroot: was 66 hex chars, now 64. - Scope the auth-401 console allowlist to `authenticated: false` tests only, so a genuine 401 on an authenticated route still fails the smoke.
|
Follow-up review surfaced a few more items beyond @ordishs's. Pushed Coverage — Fixture bug. Allowlist scope. The auth- Documented limitation. Pre-existing dashboard issues found during review (dup Still deferring |
ordishs
left a comment
There was a problem hiding this comment.
Verified locally on the PR head (8a9ab22ff): the full Playwright smoke suite is green — 16/16 passed across two consecutive runs (retries disabled, exit 0). Covers all 12 navigable routes, the /viewer block-detail click-through, and the subtree/tx/utxo detail pages.
Strong points:
- Fork-safe CI —
pull_request(notpull_request_target),contents: readonly, all third-party actions SHA-pinned,npm ci --ignore-scripts, and the locally-resolved Playwright binary. No secrets exposed. - Hermetic harness — REST + WebSocket fully mocked, so no backend dependency and no reconnect-loop console noise.
- Mock route precedence (LIFO, catch-all first) and the dual prod/dev URL patterns are correct.
Non-blocking follow-ups (fine to fold into the runes PR):
.npmrcsetsengine-strict=true, so the newengines: node >=20.19.0is now a hard gate onnpm installinui/dashboard(includingmake build-dashboard). Defensible given Vite 7, but worth pinning the CI Node minor or adding an.nvmrcso the workflow can't drift below the floor the package now requires.- Smoke can't distinguish an auth redirect:
/loginalso carriesdata-test-id="page-root"and the SPA fallback makes every response 200, so an authed route that silently redirected would still pass the visibility check. Consider asserting the final URL or a route-unique selector.
Approving.
|
Summary
Adds a Playwright smoke test per dashboard route plus a fork-safe GitHub Actions workflow that runs on every PR touching
ui/dashboard/**. Lands the regression net before the Svelte 5 runes migration in a follow-up PR (closes #977).Coverage
One test per route: navigate, assert visible selector (
[data-testid="page-root"]added to pages that lacked a stable hook), assert zero console errors and zero Svelte deprecation messages during nav + 1s idle. All network calls (REST + WebSocket) mocked viapage.route()/routeWebSocket()so the harness is hermetic — no real backend needed.12 routes covered:
/,/home,/network,/p2p,/peers,/forks,/ancestors,/viewer,/settings,/admin,/wstest,/login. The/apigroup is server-only (just+server.tshandlers, no navigable page) so it's excluded.What this PR doesn't do
npm run checkis intentionally not a CI step. The legacy-mode baseline carries ~33 svelte-check errors that pre-date this PR; the runes migration (follow-up) clears them and re-introduces the check as a gate.CI safety (fork-friendly)
pull_requesttrigger withpaths:filter so the job only runs on UI changes.contents: readpermission only. No secrets, no Docker Hub login, no PR comment writes.Verification
npm run test:integration --prefix ui/dashboard— 12 passed locally (two consecutive runs).console.errorin/home, confirmed smoke failed with the captured error; restored, confirmed green.Follow-up
npm run checkas a CI gate once the baseline is clean and fattens fixtures further (e.g. block-graph data) so smoke catches more data-shape regressions.