test(e2e): add Playwright E2E tests#22
Open
miyaji255 wants to merge 32 commits into
Open
Conversation
Plan to introduce Playwright Test alongside existing Vitest browser mode, covering home → presenter flow, slide navigation, mode toggles, timer, multi-window broadcast sync, pdfpc note, and locale switch. Also removes PDF.js mocks from thumbnail.test.ts in favor of a real fixture PDF. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
18-task plan for the spec at docs/superpowers/specs/2026-05-04-e2e-tests-design.md. Covers Playwright scaffolding, 8 E2E scenarios (home upload, slide nav, mode toggles, timer, pdfpc note, locale, recent files, presenter-presentation broadcast sync), thumbnail.test.ts mock removal, and CI job addition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the first end-to-end Playwright scenario that uploads the demo
PDF + pdfpc files via the home page file input, follows the redirect
into the presenter route, and asserts the slide counter and stage
text render. Cleans up the auto-opened presentation window for test
isolation.
Also fix resetAppState to wait for the "/" -> "/{locale}" client-side
redirect to settle before evaluating IndexedDB / localStorage clears.
Without this wait, the navigation can destroy the execution context
mid-evaluate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73c1dbcd20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Wait for the presentation canvas (positive signal that broadcast init succeeded and SlideStage mounted) instead of "Connecting" disappearing, which falsely resolves when the presentation enters an error fallback. Also keep the auto-hiding menu visible during assertions via a background mouse-nudge loop, and emit Playwright HTML reports so CI artifact upload captures something on failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The popup that hosts the presentation page snapshots the opener's sessionStorage at creation time. Previously the pair-id was only set during presenter render — which is gated on PDF.js + pdfpc Suspense — so on slow environments the popup opened with empty sessionStorage and fell back to lobby pairing. Lobby pairing has a 5s deadline, but the presenter's broadcast hook can take longer than that to mount when the PDF takes time to parse, surfacing TIMEOUT_PAIRING_PRESENTATION. Calling ensurePresenterPairId(pdf.name) immediately before window.open seeds the storage synchronously, so the popup inherits a usable pair-id and the BroadcastChannel pairs without going through the lobby. Caught by the new presenter-presentation-sync E2E spec running on the GitHub Actions Linux runner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Playwright Test E2E layer (separate from the existing Vitest browser-mode suite) to exercise the running app via the Vite dev server, plus a few small app tweaks to make E2E selectors stable and multi-window pairing more reliable.
Changes:
- Introduces Playwright E2E test harness (
playwright.config.ts,e2e/tests/helpers/fixtures) and addspnpm e2escripts + CI job. - Adds accessibility labels for presenter icon buttons (Next/Prev, Timer) to enable robust role-based selectors.
- Adjusts test setup: Vitest excludes
e2e/**, andthumbnail.test.tsnow validates against a real PDF fixture instead of mockingpdfjs-dist.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Excludes e2e/** from Vitest test discovery. |
| src/routes/$locale/(main)/index.tsx | Seeds broadcast pairing id into sessionStorage before opening the presentation window. |
| src/routes/$locale/(main)/-presenter/Timer.tsx | Adds localized aria-labels for timer icon buttons. |
| src/routes/$locale/(main)/-presenter/NextPrevFooter.tsx | Adds localized aria-labels for prev/next slide icon buttons. |
| src/lib/thumbnail.test.ts | Rewrites thumbnail tests to use a real demo PDF fixture via ?url + fetch. |
| pnpm-lock.yaml | Locks @playwright/test addition. |
| playwright.config.ts | Playwright Test config: dev-server webServer, chromium project, CI reporter/retries. |
| package.json | Adds e2e / e2e:ui scripts and @playwright/test devDependency. |
| messages/ja.json | Adds new message keys for presenter aria-label strings (JA). |
| messages/en.json | Adds new message keys for presenter aria-label strings (EN). |
| e2e/tests/timer.spec.ts | E2E spec for timer start/reset behavior. |
| e2e/tests/slide-navigation.spec.ts | E2E spec for button + keyboard slide navigation flows. |
| e2e/tests/recent-files.spec.ts | E2E spec for recent files library persistence + deletion. |
| e2e/tests/presenter-presentation-sync.spec.ts | E2E spec for presenter ↔ presentation multi-window broadcast sync. |
| e2e/tests/presenter-modes.spec.ts | E2E spec for freeze/blackout/overview mode toggles. |
| e2e/tests/pdfpc-note.spec.ts | E2E spec for pdfpc note rendering + empty placeholder. |
| e2e/tests/locale-switch.spec.ts | E2E spec for locale switching + persistence. |
| e2e/tests/home-upload.spec.ts | E2E spec for upload → presenter navigation + basic render checks. |
| e2e/helpers/reset-state.ts | Helper to clear IndexedDB/localStorage and reload between tests. |
| e2e/helpers/presentation-window.ts | Helper for capturing the popup presentation page. |
| e2e/helpers/open-pdf.ts | Helper to upload the demo PDF(+pdfpc) and wait for presenter readiness. |
| e2e/fixtures/pdfs.ts | Resolves absolute paths to demo PDF/pdfpc fixtures for Playwright. |
| docs/superpowers/specs/2026-05-04-e2e-tests-design.md | Design spec documenting E2E approach and scenarios. |
| docs/superpowers/plans/2026-05-04-e2e-tests-playwright.md | Implementation plan for Playwright E2E integration. |
| .gitignore | Ignores Playwright output directories (test-results/, playwright-report/, cache). |
| .github/workflows/pr-check.yaml | Adds a CI e2e job running pnpm e2e and uploading report on failure. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds 6 tests covering the laser pointer and pen drawing tools, which broadcast state via sendTool. Verifies: - laser activated on presenter syncs the dot to presentation - laser activated on presentation syncs the dot to presenter - pen stroke drawn on presenter appears on presentation - pen stroke drawn on presentation appears on presenter - erase (`e`) clears strokes on both windows - Escape removes the overlay portal on both windows Configured to run serially within the file (test.describe.configure mode: serial) — these tests share heavy multi-window interactions and race the dev server's module compilation when run in parallel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-talk Adds `e2e/helpers/test-fixtures.ts` with a worker-scoped Playwright fixture that copies the demo PDF and pdfpc to a unique-named pair (e.g. `pdfpw-demo-w0.pdf`) per worker. The app keys its BroadcastChannel name and sessionStorage pair-id on `File.name`, so unique names prevent parallel test workers from cross-pairing or stomping on each other's tool state. All upload-spawning specs now consume `uniqueFixtures` instead of the static `fixtures` import. `locale-switch.spec.ts` is unchanged because it doesn't upload. Also: - pointer-tools.spec.ts gets `retries: 2` (in addition to `mode: "serial"`) to absorb a residual flake where useToolShortcut's window keydown listener occasionally registers after the spec presses its first key. - pressShortcut switched to `body.press` (single dispatch) — combining it with `window.dispatchEvent` was double-firing the toggle and observably flipping toolMode laser → none in one synthetic press. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- resetAppState: clear sessionStorage and seed pdfpw:locale="en". Forces English UI regardless of navigator.language so role/aria-label selectors stay deterministic, and prevents the broadcast pair-id from leaking between tests in the same context. - Drop e2e/helpers/open-pdf.ts and presentation-window.ts (YAGNI — neither was consumed by any spec). - vite.config.ts: extend Vitest's default exclude list instead of replacing it, so cypress/.idea/.git/.cache/config-file globs stay excluded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Existing specs only verify pairing implicitly (sync + pointer-tools wait for the presentation canvas mid-test). This spec asserts pairing as the primary subject so the regression we just fixed — popup losing the pair-id when sessionStorage is seeded after window.open — surfaces directly here: - presenter sessionStorage carries a UUID pair-id keyed on the file name - the popup's sessionStorage was inherited at creation and matches - the presentation canvas mounts (init-response received) - none of the error fallbacks (Connecting / Pairing failed / config / PDF load) are visible Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing laser tests only verified the dot becomes visible after pressing `l`; they did not check whether the dot actually follows the cursor. This test moves the mouse to three distinct quadrants of the slide canvas (25/25, 75/75, 10/90) and asserts the dot's `style.left` / `style.top` percent values land within ±2% of the expected position on BOTH the presenter and the presentation page — catching coordinate-swap, axis-aliasing, or broadcast-truncation bugs that the visibility-only assertion would miss. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a grid-of-9 visibility test for the laser dot. Verifies at each point that: 1. The visible inner dot passes Element.checkVisibility() — covers display:none / visibility:hidden / opacity:0 regressions. 2. At the dot's pixel center the topmost element is part of the overlay portal — catches the "laser ends up behind the slide" stacking regression that checkVisibility alone cannot detect. Chromium's elementsFromPoint() filters out pointer-events:none elements, so the helper temporarily flips the overlay's pointer-events inline style to "auto" during the probe, then restores it. The probe runs once per grid point on both presenter and presentation pages. Also stops using parseFloat and uses Number(...) consistently in the percent-parsing helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
readLaserPercent already produces the numeric left/top from the wrapper's
style. Threading those values through evaluate's argument removes the
duplicate parse (and the .replace("%","") that came with it) inside the
browser-side closure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous on-top helper relied on Element.checkVisibility() with default options, which only catches display:none and skips opacity:0 / visibility:hidden / 0-area regressions. Playwright's expect(locator).toBeVisible() is strictly stronger: it requires display ≠ none, visibility ≠ hidden, opacity > 0 anywhere up the chain, AND a non-zero bounding box. Changes: - Probe the *inner core* (the 12×12 red dot, last child of the wrapper) instead of the 0×0 wrapper. Targeting the wrapper made the bbox-area check meaningless because the wrapper is sized 0 by design. - Use Playwright's toBeVisible() for the visibility step. - Keep the existing position match + elementsFromPoint occlusion check as separate poll steps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Self-verification surfaced a hole: Playwright's expect(locator).toBeVisible()
does NOT walk ancestors to check opacity. Injecting opacity:0 on the
overlay portal made the laser invisible to a user but our check still
reported it as visible.
Changes:
- Add a supplementary `Element.checkVisibility({opacityProperty: true,
visibilityProperty: true, contentVisibilityAuto: true})` step. Default
options on this DOM API don't catch ancestor opacity either; the
options enable the ancestor walk we actually need.
- Add three self-verifying tests that prove the check fails when the
laser is forcibly hidden via:
1. `display: none` injection
2. `opacity: 0` ancestor injection
3. A z-index 9999 div stacked in front
These are guards against future regressions of the visibility helper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`useContainerRect` ran its useEffect once at mount with `[ref]` deps. When `pdfAreaRef.current` was null at that moment (the target div lives inside <PdfPage>'s inner Suspense and PdfPageCanvas is suspended on `use(getPage(...))`), the effect early-returned and never recovered: `[ref]` is stable, so the effect is never re-fired. `rect` stayed null forever, `PointerOverlay` returned null forever, and the laser dot was permanently invisible on whichever side hit the race — most reliably the presentation page, which has more PdfPages preloaded and runs the race more often. Fix: when `ref.current` is null at first run, set up a `MutationObserver` on `document.body` and re-try once the target node attaches. After it attaches, install the original ResizeObserver + window listeners and disconnect the MutationObserver. Surfaced by user-reported "presentation laser doesn't display" repro (diagnostic: `body > div.fixed.pointer-events-none.z-50` was absent in presentation's DOM after activation, confirming the portal never rendered). The new `regression: presentation PointerOverlay portal mounts even when PdfPage Suspense is slow` E2E test in pointer-tools.spec.ts covers the contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Adds Playwright Test as a new E2E layer that exercises the running app via the Vite dev server (
pnpm dev, port 6123). Tests live ine2e/and reuse the existingdemo/pdfpw-demo.pdf+pdfpw-demo.pdfpcfixtures. The existing Vitest browser-mode unit tests remain untouched.Implements
docs/superpowers/specs/2026-05-04-e2e-tests-design.mdperdocs/superpowers/plans/2026-05-04-e2e-tests-playwright.md.E2E scenarios (8 specs, 20 tests)
g <n> Enter+ Backspace historyaria-pressedassertions/en↔/javia LocaleSwitcher +pdfpw:localelocalStorageBrowserContext.waitForEvent('page'), asserts Broadcast Channel propagates Next-slide and Blackout to the presentation windowOther changes
thumbnail.test.ts: removedvi.mock("pdfjs-dist", ...)andvi.mock("pdfjs-dist/build/pdf.worker.min.mjs?url", ...). Loads the realdemo/pdfpw-demo.pdfvia?urlimport +fetch, asserts the JPEG data URL is non-trivial. Canvas-getContext-null branch still uses avi.spyOn.aria-label("Previous slide" / "Next slide") toNextPrevFootericon buttons and ("Start or pause timer" / "Reset timer") to Timer icon buttons. Driven by E2E selector needs but valid accessibility improvements in their own right.e2ejob in.github/workflows/pr-check.yamlparallel totestandbuild. Runspnpm e2eon Chromium. Uploads the Playwright HTML report on failure.vite.config.tstest.excludenow containse2e/**so Vitest never picks up Playwright specs..gitignore: ignorestest-results/,playwright-report/,playwright/.cache/.Verification
pnpm tsc -b— cleanpnpm build— successpnpm test— 92 passed (existing Vitest unit suite intact, including the rewrittenthumbnail.test.ts)pnpm e2e— 20 passed (23.9s) on ChromiumNotes for reviewers
slide-navigation.spec.ts: theg <n> Entershortcut treats<n>as a PDF page number, not a user-slide index. The spec pins this current behavior; whether it should accept user-slide indices instead is a UX decision and is out of scope here.pnpm testreports a Firefox launch teardown timeout in vitest browser mode, but all 92 tests still report as passing. This is a pre-existing environmental issue and does not affect CI (Linux runner with Playwright FF available).🤖 Generated with Claude Code