test(stackflow-spa): add browser E2E coverage - #1802
Conversation
|
📝 WalkthroughWalkthroughStackflow SPA에 Playwright 기반 E2E 환경을 추가했습니다. 화면 전환, pop, swipe-back, iframe 동작과 이미지 상태 및 overlay 상호작용을 검증합니다. GitHub Actions에서 Chromium과 WebKit 테스트를 실행합니다. ChangesStackflow SPA E2E 검증
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant StackflowSPA
participant BrowserHistory
Playwright->>StackflowSPA: 화면 push 또는 swipe-back 수행
StackflowSPA->>BrowserHistory: URL 상태 갱신
BrowserHistory-->>StackflowSPA: back 또는 forward 상태 복원
Playwright->>StackflowSPA: settled 상태와 top activity 검증
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Alpha Preview (Stackflow SPA)
|
Alpha Preview (Storybook)
|
c53bf2a to
d8c87ef
Compare
Alpha Preview (Docs)
|
13f89c3 to
8a8f4c3
Compare
Cover the image loading regression in a real browser, the only layer where it is observable: happy-dom has no layout engine, and Chromatic sees identical pixels for a stuck frame and a loading one. - lazy deadlock: assert both sides (deferred before scroll, loaded after) - loading invariant: image keeps its box, fallback sits behind it - error state: image hidden, fallback fills the frame - overlay hit-tests: badge/icon/indicator/reaction stay above the image Tests are named *.e2e.ts so `bun test` never collects them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirrors the existing test workflows: same setup action with prebuilt packages, Chromium installed from the workspace Playwright version, and the Playwright report uploaded when the run fails. The config now spawns a fresh Storybook on CI instead of reusing one, keeps retries off so flakes stay visible, and retains a trace on the first failure since that is the only post-mortem signal a CI run leaves behind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Lowering the fallback behind the image left it covered for hit-testing, so an interactive node passed as `fallback` was unreachable until load. #1791 fixed that with pointer-events on the not-yet-loaded content; this pins it. elementFromPoint is the only way to observe it — the pixels are identical either way, so neither Chromatic nor a unit test can see it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
14a6257 to
87f8256
Compare
The fixtures lived beside the real stories and relied on a per-story chromatic.disableSnapshot to stay out of snapshots. Forgetting it snapshots a 12000px spacer or a deliberately-stalled loading state, so the cost of the mistake is a flaky visual baseline and the only thing preventing it was remembering a parameter on every new fixture. Move them to docs/e2e/*.fixture.tsx next to the tests that use them, and collect that glob only when STORYBOOK_E2E is set, which Playwright's webServer does. Chromatic and the deployed Storybook build through build-storybook without it, so the fixtures are not merely skipped there — they do not exist. No disableSnapshot remains in the repo. Fixture story ids move under an E2E/ title, and storybook:e2e runs a dev server with the fixtures for local debugging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Storybook's CSF indexer only matches files containing `.stories.`, so *.fixture.tsx had no matching indexer and the whole index 500'd — every story disappeared, which is why even the untouched overlay test failed. Measured after the rename: with STORYBOOK_E2E the index has 361 stories including the three e2e- fixtures, without it 358 and none of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous shape leaned on a STORYBOOK_E2E variable threaded through main.ts, playwright.config.ts and a package script, and on filenames that differed by one segment — image-frame.e2e.ts next to image-frame.e2e.stories.tsx. Put the fixtures in e2e/fixtures/ so the directory carries the meaning and the files keep the plain .stories.tsx name the Storybook indexer requires. Collect that glob whenever NODE_ENV is not production: Chromatic and the deploy both run storybook build, so the fixtures cannot reach a snapshot, while a plain `bun run storybook` still shows them for debugging. Measured: storybook build emits 358 stories with no e2e- entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
webpack derives chunk names from the story file's basename, not its path, so e2e/fixtures/avatar.stories.tsx and stories/Avatar.stories.tsx differ only in casing and the dev server refuses to start. Only dev enables both globs, which is why storybook build was fine and the webServer timed out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reasoning lives in the pull request description, including the two filename constraints that are easy to mistake for style: the -e2e suffix avoids a webpack chunk-name collision with stories/Avatar.stories.tsx, and .stories. is what the Storybook indexer matches on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
junghyeonsu
changed the title
junghyeonsu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
examples/stackflow-spa/e2e/stackflow-helpers.ts (1)
5-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftE2E locator를 접근성 role 또는
data-testid로 통일하세요.현재 테스트는
data-part, 상태 속성, CSS selector, 표시 텍스트에 의존합니다. 이 선택자는 Stackflow 내부 DOM 구조와 UI 문구 변경에 취약합니다. 버튼은getByRole("button", { name })로 선택하세요. 전환 상태, edge, iframe처럼 role이 없는 대상은 테스트 전용data-testid를 제공하고getByTestId를 사용하세요.
examples/stackflow-spa/e2e/stackflow-helpers.ts#L5-L39:data-part및 상태 속성 selector를data-testid또는 접근성 role 기반 locator로 교체하세요.examples/stackflow-spa/e2e/stackflow.e2e.ts#L16-L18:getByText및 CSS 기반 locator를 role 또는data-testid기반 locator로 교체하세요.As per coding guidelines, “locator는 접근성 role이나
data-testid를 사용하고 CSS class 및 인라인 style 문자열에 의존하지 않는다” 규칙을 적용해야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/stackflow-spa/e2e/stackflow-helpers.ts` around lines 5 - 39, Update topActivity and expectStackSettled in examples/stackflow-spa/e2e/stackflow-helpers.ts#L5-L39 to replace data-part, state-attribute, and CSS selectors with getByRole or getByTestId locators, adding test IDs where role-based selection is unavailable. Also update examples/stackflow-spa/e2e/stackflow.e2e.ts#L16-L18 to replace getByText and CSS-based locators with accessible role or test-ID locators; apply this consistently without relying on internal DOM structure or styling strings.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/stackflow-spa/e2e/image-behavior.e2e.ts`:
- Around line 35-36: Update the navigation in the image-behavior test after the
HELD_IMAGE_URL route so page.goto uses the domcontentloaded wait condition,
allowing navigation to complete without waiting for the held image and timing
out.
In `@examples/stackflow-spa/src/activities/ActivityE2EImageBehavior.tsx`:
- Around line 27-30: Update the ActivityE2EImageBehavior registration in the
declare module "`@stackflow/config`" block to use {} instead of Record<string,
never> for activities without Stackflow params.
In `@examples/stackflow-spa/src/stackflow/stackflow.config.ts`:
- Line 11: Move the ActivityE2EImageBehavior entry within the activities array
so its /e2e/image-behavior route appears after /detail and before /error-state,
preserving alphabetical route order and the existing kebab-case naming.
---
Nitpick comments:
In `@examples/stackflow-spa/e2e/stackflow-helpers.ts`:
- Around line 5-39: Update topActivity and expectStackSettled in
examples/stackflow-spa/e2e/stackflow-helpers.ts#L5-L39 to replace data-part,
state-attribute, and CSS selectors with getByRole or getByTestId locators,
adding test IDs where role-based selection is unavailable. Also update
examples/stackflow-spa/e2e/stackflow.e2e.ts#L16-L18 to replace getByText and
CSS-based locators with accessible role or test-ID locators; apply this
consistently without relying on internal DOM structure or styling strings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 064fe05d-e1cd-44a1-9792-f7a5030e1476
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockexamples/stackflow-spa/package.jsonis excluded by!**/package.json
📒 Files selected for processing (14)
.github/workflows/e2e-test.yml.gitignoredocs/stories/Avatar.stories.tsxexamples/stackflow-spa/e2e/AGENTS.mdexamples/stackflow-spa/e2e/image-behavior.e2e.tsexamples/stackflow-spa/e2e/stackflow-helpers.tsexamples/stackflow-spa/e2e/stackflow.e2e.tsexamples/stackflow-spa/playwright.config.tsexamples/stackflow-spa/src/activities/ActivityAnimateFalseTest.tsxexamples/stackflow-spa/src/activities/ActivityE2EImageBehavior.tsxexamples/stackflow-spa/src/activities/ActivityPopTest.tsxexamples/stackflow-spa/src/activities/ActivityTransitionStyle.tsxexamples/stackflow-spa/src/stackflow/Stack.tsxexamples/stackflow-spa/src/stackflow/stackflow.config.ts
| await page.route(HELD_IMAGE_URL, () => {}); | ||
| await page.goto("/e2e/image-behavior"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file =="
fd -a 'image-behavior\.e2e\.ts$' . || true
echo "== Inspect relevant file =="
file="$(fd 'image-behavior\.e2e\.ts$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,120p' "$file" | cat -n
fi
echo "== Search related route/page goto patterns =="
rg -n "HELD_IMAGE_URL|page\.route|page\.goto|waitUntil|domcontentloaded|load" examples/stackflow-spa/e2e -S || true
echo "== Package Playwright version =="
if [ -f package.json ]; then
jq '.devDependencies.playwright, .dependencies.playwright' package.json -r || true
fi
if [ -f examples/stackflow-spa/e2e/package.json ]; then
jq '.devDependencies.playwright, .dependencies.playwright' examples/stackflow-spa/e2e/package.json -r || true
fiRepository: daangn/seed-design
Length of output: 7484
🌐 Web query:
Playwright page.route matching requests wait until route fulfilled aborted continued documentation
💡 Result:
In Playwright, page.route() registers a handler for network requests that match a specified URL pattern [1][2]. When a request is intercepted, Playwright pauses the request, and it will remain "stalled" or "pending" until your handler explicitly resolves it using one of the terminal actions: route.fulfill(), route.abort(), or route.continue() [2][3]. Key behaviors regarding request handling: 1. Mandatory Resolution: Once a request is intercepted, you are obligated to perform exactly one terminal action on it [3]. If your handler matches a request but fails to call one of these methods (e.g., due to an unhandled logic branch), the request will hang indefinitely, often leading to a test timeout [3]. 2. Non-blocking/Asynchronous Nature: page.route() itself does not wait for requests to occur; it only registers the handler [4]. Because there is no implicit waiting for requests to hit the route, you must ensure your handlers are registered before the expected network activity occurs to avoid race conditions [4][5]. 3. Terminal Actions: - route.fulfill: Mocks the response entirely, sending a custom response back to the browser without contacting the network [3]. - route.abort: Fails the request as if a network error occurred [1][3]. - route.continue: Sends the request to the network, optionally with modified headers, method, URL, or post data [1][6][3]. 4. Chaining Handlers: If multiple routes match the same request, they are executed in the reverse order of their registration (last registered first) [6][7]. You can use route.fallback() within a handler to invoke the next matching route in the chain rather than immediately resolving the request [6][7]. If you use route.continue() or route.fulfill(), no further (less recently registered) handlers will be invoked for that request [6][7]. If you need to ensure a test waits for a specific request to be processed, you should pair page.route() with synchronization primitives like page.waitForRequest() or page.waitForResponse() where appropriate [4][5].
Citations:
- 1: https://playwright.dev/docs/network
- 2: https://playwright.dev/docs/api/class-page
- 3: https://qaskills.sh/blog/playwright-network-interception-route-guide
- 4: [Question] Page.route not waiting for request? microsoft/playwright#27231
- 5: [BUG] page.route -test is stuck with page.route() just shows the blank screen microsoft/playwright#26689
- 6: https://playwright.dev/docs/api/class-route
- 7: https://github.com/microsoft/playwright/blob/303901d7/docs/src/api/class-route.md
🌐 Web query:
Playwright page.goto waitUntil default load documentation API
💡 Result:
In the Playwright API, the page.goto method includes an optional waitUntil parameter that determines when the navigation operation is considered successful [1][2]. The default value for this parameter is 'load' [1][2]. The available options for the waitUntil parameter are [3][2]: 'load': The operation is considered finished when the 'load' event is fired. This is the default behavior [3][1]. 'domcontentloaded': The operation is considered finished when the 'DOMContentLoaded' event is fired [3][2]. 'networkidle': The operation is considered finished when there are no network connections for at least 500 ms [3][2]. Note that Playwright explicitly discourages the use of 'networkidle' for testing; it is recommended to use web assertions to assess page readiness instead [3][2]. 'commit': The operation is considered finished when the network response is received and the document has started loading [3][1].
Citations:
- 1: https://playwright.dev/docs/next/api/class-page
- 2: https://playwright.dev/docs/api/class-page
- 3: https://github.com/microsoft/playwright/blob/main/docs/src/api/params.md
보류된 이미지 navigation의 대기 상태를 domcontentloaded로 바꾸세요.
Line 36의 page.route(HELD_IMAGE_URL, () => {})는 해당 요소를 계속 유지합니다. 현재는 Page가 load를 기다리는 기본 동작으로 인해 이 navigation이 timeout될 수 있습니다.
수정 예시
- await page.goto("/e2e/image-behavior");
+ await page.goto("/e2e/image-behavior", { waitUntil: "domcontentloaded" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.route(HELD_IMAGE_URL, () => {}); | |
| await page.goto("/e2e/image-behavior"); | |
| await page.route(HELD_IMAGE_URL, () => {}); | |
| await page.goto("/e2e/image-behavior", { waitUntil: "domcontentloaded" }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/stackflow-spa/e2e/image-behavior.e2e.ts` around lines 35 - 36,
Update the navigation in the image-behavior test after the HELD_IMAGE_URL route
so page.goto uses the domcontentloaded wait condition, allowing navigation to
complete without waiting for the held image and timing out.
| declare module "@stackflow/config" { | ||
| interface Register { | ||
| ActivityE2EImageBehavior: Record<string, never>; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stackflow params 타입을 {}로 변경하세요.
이 activity는 Stackflow params를 사용하지 않습니다. Record<string, never> 대신 가이드라인이 요구하는 {}를 선언하세요.
수정 예시
interface Register {
- ActivityE2EImageBehavior: Record<string, never>;
+ ActivityE2EImageBehavior: {};
}As per coding guidelines, Activities without Stackflow params should declare params as \{}` in the `declare module "@stackflow/config"` block`.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declare module "@stackflow/config" { | |
| interface Register { | |
| ActivityE2EImageBehavior: Record<string, never>; | |
| } | |
| declare module "`@stackflow/config`" { | |
| interface Register { | |
| ActivityE2EImageBehavior: {}; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/stackflow-spa/src/activities/ActivityE2EImageBehavior.tsx` around
lines 27 - 30, Update the ActivityE2EImageBehavior registration in the declare
module "`@stackflow/config`" block to use {} instead of Record<string, never> for
activities without Stackflow params.
Source: Coding guidelines
| { route: "/404", name: "ActivityNotFound" }, | ||
| { route: "/pop-test", name: "ActivityPopTest" }, | ||
| { route: "/animate-false-test", name: "ActivityAnimateFalseTest" }, | ||
| { route: "/e2e/image-behavior", name: "ActivityE2EImageBehavior" }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
route를 알파벳순 위치로 이동하세요.
/e2e/image-behavior는 /accordion보다 뒤에 와야 합니다. 이 항목을 /detail 뒤와 /error-state 앞에 배치하세요.
As per coding guidelines, Register activity routes in \src/stackflow/stackflow.config.ts` in the `activities` array using kebab-case route names, maintaining alphabetical order`.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/stackflow-spa/src/stackflow/stackflow.config.ts` at line 11, Move
the ActivityE2EImageBehavior entry within the activities array so its
/e2e/image-behavior route appears after /detail and before /error-state,
preserving alphabetical route order and the existing kebab-case naming.
Source: Coding guidelines
1 participant
요약
주요 변경
테스트 범위
이미지 계약 5개
Stackflow 계약 8개
검증
범위
공개 패키지 API 변경은 없습니다. 테스트 전용 route와 예제 앱의 data-testid만 추가했으며, 기존 수동 QA Activity는 홈에서 계속 접근할 수 있습니다.
Summary by CodeRabbit
새로운 기능
테스트