feat(test): Playwright e2e suite + demo mode on MSW#293
Open
jaredglaser wants to merge 6 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 37 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR replaces the legacy ChangesMSW + Playwright E2E Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Bun as Bun CLI
participant StaticServer as serve-static.ts
participant Playwright as Playwright Runner
participant Browser as Chromium
participant SW as mockServiceWorker.js
participant Handlers as MSW Handlers
Dev->>Bun: bun run test:e2e
Bun->>StaticServer: e2e:server:app (port 3101)
Bun->>StaticServer: e2e:server:demo (port 3100)
Bun->>Playwright: playwright test
Playwright->>Browser: navigate to baseURL
Browser->>SW: register mockServiceWorker.js
SW-->>Browser: controllerchange (worker active)
Browser->>Browser: hydrateRoot(StartClient)
Browser->>SW: fetch /_serverFn/<id>
SW->>Handlers: handleServerFn(request)
Handlers-->>SW: serialized JSON Response
SW-->>Browser: MOCK_RESPONSE
Browser-->>Playwright: page assertions pass
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
jaredglaser
force-pushed
the
claude/playwright-test-suite-lfyzhx
branch
from
2ffa0b7 to
9c5a2ae
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 @.github/workflows/ci.yml:
- Line 66: The e2e job is currently running in parallel without gating the
release jobs, allowing artifacts to be published even when browser tests fail.
You need to add the e2e job as a required dependency for all release jobs so
they cannot run until e2e passes. In .github/workflows/ci.yml at lines 225-229
and 335-339, add a needs field to the respective release job definitions that
references the e2e job, ensuring these jobs will wait for and require the e2e
job to complete successfully before proceeding with image publishing and demo
deploy.
- Around line 66-76: The e2e job lacks explicit permissions and has checkout
credentials persisted, which are security risks. Add a permissions block to the
e2e job specifying only the necessary permissions (typically read-only access to
repository contents), and modify the checkout step (actions/checkout@v6) to
include persist-credentials set to false to prevent the GitHub token from being
written to disk and made available to subsequent steps.
In `@docs/playwright-test-plan.md`:
- Around line 7-13: Replace all em dashes (`—`) in the playwright-test-plan.md
file with standard hyphens or other appropriate punctuation as per the repo's
markdown coding guidelines that disallow em dashes, en dashes, and
double-hyphens used as dash substitutes. In the diff section showing the demo
and app build descriptions, replace the em dashes that appear after "**`demo`**"
and "**`app`**" with regular hyphens, and apply the same replacement throughout
the file wherever em dashes are used.
In `@e2e/fixtures.ts`:
- Around line 21-25: The route.fulfill call in the overrideServerFn function is
returning raw JSON without the TanStack server-function envelope expected by
client code. Modify the route.fulfill invocation to wrap the json response in
the proper envelope format containing result, error, and context fields, and add
the x-tss-serialized header to the response headers to indicate the response is
serialized. This will ensure that intercepted /_serverFn/* calls conform to the
expected envelope structure that client code relies on for correct resolution.
In `@scripts/serve-static.ts`:
- Around line 24-27: The filePath construction using raw URL pathname segments
is vulnerable to directory traversal attacks via `..` sequences. Fix this by
resolving the filePath to an absolute canonical path after concatenating it with
dir, then verify that the resolved path is within the intended dir directory
before accessing it with Bun.file(). Only proceed with file operations if the
resolved filePath starts with or is contained within the dir path; otherwise,
reject the request to prevent reads outside the build output directory.
In `@src/lib/constants/mock.ts`:
- Line 1: The import statement at the top of the mock.ts file is using a
relative path to import from the demo module instead of using the `@/` alias for
src modules. Replace the relative path import with the `@/` alias path that points
to the same module location in the src directory, following the project's coding
guidelines that require all src file imports in TypeScript files to use the `@/`
alias.
In `@src/lib/mock/browser.ts`:
- Line 2: The import statement for handlers on line 2 of src/lib/mock/browser.ts
uses a relative path './handlers' instead of the repository-standard `@/` alias.
Replace the relative import path with the `@/` alias format to conform to the
coding guidelines that require all non-test TypeScript modules to use `@/` for src
imports. Change the import statement to use `@/lib/mock/handlers` or the
appropriate `@/` path depending on the actual location of the handlers file.
In `@src/lib/mock/functions/git-tokens.functions.ts`:
- Around line 14-33: Make the MOCK_GIT_TOKENS array mutable (convert from
readonly const to a regular mutable array) so that token operations can persist
changes. Modify the createGitToken function to generate a new MockGitToken
object with an auto-incremented id, a generated name, and current timestamps
(createdAt and lastUsedAt), then add it to the MOCK_GIT_TOKENS array before
returning the token string. Modify the revokeGitToken function to accept a token
id parameter and remove the matching token from the MOCK_GIT_TOKENS array. This
ensures that listGitTokens will reflect the created or revoked tokens on
subsequent calls.
In `@src/lib/mock/functions/hosts.functions.ts`:
- Around line 8-25: The buildHost function in
src/lib/mock/functions/hosts.functions.ts (lines 8-25) unconditionally
increments nextMockId, but updateHost at lines 78-85 calls buildHost only to
overwrite the generated id. This wastes mock IDs on every update, breaking ID
sequencing for subsequent operations. Add an optional id parameter to buildHost
so it can accept an existing ID instead of always generating a new one. When
updateHost calls buildHost, pass the existing host's id as an argument so the ID
is reused rather than consumed and discarded.
In `@src/lib/mock/handlers/function-id.ts`:
- Around line 41-50: The `decodeURIComponent(functionIdSegment)` call in the
`resolveFunctionName` function is outside the try-catch block, which means when
the segment contains malformed percent-encoding (like `%`, `%2`, or `%ZZ`), the
thrown `URIError` is not caught and breaks handler dispatch. Move the
`decodeURIComponent` call inside the try-catch block so that any `URIError` is
caught alongside JSON parsing errors, allowing the function to fall back to the
raw segment as intended. Additionally, add a test case in the test file that
covers malformed percent-encoding input to prevent regression.
In `@src/lib/mock/handlers/index.ts`:
- Around line 1-2: Replace the relative imports on lines 1-2 of the
serverFunctionHandlers and sseHandlers with absolute `@/` path aliases instead.
Change `from './server-functions'` to `from
'`@/lib/mock/handlers/server-functions`'` and `from './sse'` to `from
'`@/lib/mock/handlers/sse`'` to align with the src TypeScript import convention as
specified in the coding guidelines.
In `@src/lib/mock/handlers/server-functions.ts`:
- Around line 140-143: The handler currently allows exceptions from the mock
function call to propagate uncaught, which breaks compatibility with TanStack
server-function response handling that expects a consistent { result, error,
context } envelope. Wrap the await mock(payload) call in a try-catch block to
capture any thrown errors and pass them to serializedResponse alongside the
result, ensuring error serialization maintains parity with the expected response
format. This pattern should be applied consistently wherever the mock function
is invoked in the handler code.
- Around line 4-13: The import statement for the function-id module uses a
relative path (./) while all other imports in this file use the `@/` alias. Change
the import of ./function-id to use the `@/` alias instead to maintain consistency
with the repository's import policy and match the style of the other imports
(dockerFns, zfsFns, proxmoxFns, etc.).
In `@src/lib/mock/handlers/sse.ts`:
- Line 15: The import statement for createSseResponse uses a relative path
'./sse-stream' instead of the `@/` alias as required by coding guidelines for src
TypeScript files. Replace the relative import './sse-stream' with the proper `@/`
alias path that points to the src/lib/mock/handlers/sse-stream module. This will
ensure the file uses consistent alias imports rather than mixing relative and
alias imports in non-test code.
In `@src/lib/mock/start.ts`:
- Line 10: The import statement on line 10 in start.ts uses a relative import
path './browser' instead of the coding-compliant `@/` alias path. Replace the
relative import path './browser' in the dynamic import statement with the `@/`
alias path `@/lib/mock/browser` to comply with the guideline that non-test files
should use `@/` for src file imports.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0c58f9c-e6e8-458e-be63-4b4f95a4351c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.github/workflows/ci.yml.gitignoreCLAUDE.mdbunfig.tomldocs/development.mddocs/playwright-test-plan.mde2e/demo.demo.spec.tse2e/fixtures.tse2e/smoke.spec.tspackage.jsonplaywright.config.tspublic/mockServiceWorker.jsscripts/serve-static.tssrc/client.tsxsrc/components/AppShell.tsxsrc/lib/constants/mock.tssrc/lib/mock/__tests__/mock-event-source.test.tssrc/lib/mock/browser.tssrc/lib/mock/functions/git-tokens.functions.tssrc/lib/mock/functions/hosts.functions.tssrc/lib/mock/handlers/__tests__/function-id.test.tssrc/lib/mock/handlers/__tests__/server-functions.test.tssrc/lib/mock/handlers/__tests__/sse-stream.test.tssrc/lib/mock/handlers/function-id.tssrc/lib/mock/handlers/index.tssrc/lib/mock/handlers/server-functions.tssrc/lib/mock/handlers/sse-stream.tssrc/lib/mock/handlers/sse.tssrc/lib/mock/install-demo.tssrc/lib/mock/mock-event-source.tssrc/lib/mock/start.tssrc/routes/__root.tsxvite.config.ts
💤 Files with no reviewable changes (4)
- src/lib/mock/tests/mock-event-source.test.ts
- src/lib/mock/install-demo.ts
- src/lib/mock/mock-event-source.ts
- src/components/AppShell.tsx
| path: license-report.json | ||
| retention-days: 30 | ||
|
|
||
| e2e: |
Contributor
There was a problem hiding this comment.
Gate release jobs on the new e2e job.
e2e currently runs in parallel and does not block image publishing or demo deploy. A failing browser flow can still ship artifacts.
Suggested patch
publish:
name: Publish Docker Images
runs-on: ubuntu-latest
- needs: [build-test, agent, agent-updater]
+ needs: [build-test, agent, agent-updater, e2e]
@@
deploy-demo:
name: Deploy Demo to GitHub Pages
runs-on: ubuntu-latest
- needs: [build-test, agent, agent-updater]
+ needs: [build-test, agent, agent-updater, e2e]Also applies to: 225-229, 335-339
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 66-94: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/ci.yml at line 66, The e2e job is currently running in
parallel without gating the release jobs, allowing artifacts to be published
even when browser tests fail. You need to add the e2e job as a required
dependency for all release jobs so they cannot run until e2e passes. In
.github/workflows/ci.yml at lines 225-229 and 335-339, add a needs field to the
respective release job definitions that references the e2e job, ensuring these
jobs will wait for and require the e2e job to complete successfully before
proceeding with image publishing and demo deploy.
Comment on lines
+66
to
+76
| e2e: | ||
| name: E2E (Playwright + MSW) | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
|
|
Contributor
There was a problem hiding this comment.
Harden e2e job token and checkout defaults.
The new job inherits default workflow permissions and keeps checkout credentials persisted. Tightening both is a straightforward security hardening step.
Suggested patch
e2e:
name: E2E (Playwright + MSW)
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:
- name: Checkout code
uses: actions/checkout@v6
+ with:
+ persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 71-72: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 66-94: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 72-72: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 75-75: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 75-75: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/ci.yml around lines 66 - 76, The e2e job lacks explicit
permissions and has checkout credentials persisted, which are security risks.
Add a permissions block to the e2e job specifying only the necessary permissions
(typically read-only access to repository contents), and modify the checkout
step (actions/checkout@v6) to include persist-credentials set to false to
prevent the GitHub token from being written to disk and made available to
subsequent steps.
Source: Linters/SAST tools
Comment on lines
+21
to
+25
| if (serverFnMatches(route.request().url(), functionName)) { | ||
| await route.fulfill({ | ||
| contentType: 'application/json', | ||
| body: JSON.stringify(json), | ||
| }); |
Contributor
There was a problem hiding this comment.
Return TanStack server-function envelope in overrideServerFn.
route.fulfill is returning raw JSON, but intercepted /_serverFn/* calls expect the serialized { result, error, context } envelope plus x-tss-serialized. Without that, overrides can resolve incorrectly in client code.
Suggested patch
if (serverFnMatches(route.request().url(), functionName)) {
+ const payload = JSON.stringify({
+ result: json,
+ error: null,
+ context: {},
+ });
await route.fulfill({
contentType: 'application/json',
- body: JSON.stringify(json),
+ headers: {
+ 'x-tss-serialized': '1',
+ },
+ body: payload,
});
return;
}📝 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.
Suggested change
| if (serverFnMatches(route.request().url(), functionName)) { | |
| await route.fulfill({ | |
| contentType: 'application/json', | |
| body: JSON.stringify(json), | |
| }); | |
| if (serverFnMatches(route.request().url(), functionName)) { | |
| const payload = JSON.stringify({ | |
| result: json, | |
| error: null, | |
| context: {}, | |
| }); | |
| await route.fulfill({ | |
| contentType: 'application/json', | |
| headers: { | |
| 'x-tss-serialized': '1', | |
| }, | |
| body: payload, | |
| }); | |
| return; | |
| } |
🤖 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 `@e2e/fixtures.ts` around lines 21 - 25, The route.fulfill call in the
overrideServerFn function is returning raw JSON without the TanStack
server-function envelope expected by client code. Modify the route.fulfill
invocation to wrap the json response in the proper envelope format containing
result, error, and context fields, and add the x-tss-serialized header to the
response headers to indicate the response is serialized. This will ensure that
intercepted /_serverFn/* calls conform to the expected envelope structure that
client code relies on for correct resolution.
Replace the demo-mode Vite alias swap + MockEventSource with an in-browser
MSW mock backend that intercepts the real network calls. The same handlers
back both demo mode and a new Playwright suite, so scenarios are authored
once and the demo stays representative of the real app.
- MSW handlers (src/lib/mock/handlers): server-function dispatch returning the
real seroval {result,error,context} envelope + x-tss-serialized header, and
streaming text/event-stream SSE responses built from the existing generators.
- MSW starts in a custom client entry (src/client.tsx) before hydrateRoot,
the only point early enough to intercept route loaders. Gated by
IS_MOCK_ENABLED: on for demo, or VITE_ENABLE_MSW=true for the e2e app target.
- serverFns.generateFunctionId keeps prod server-fn ids decodable by the handler.
- Playwright runs two static production builds served over MSW: a demo smoke
target and a real-app deep target, with an overrideServerFn fixture for
per-scenario response shaping. One basic smoke test on each.
- docs/playwright-test-plan.md maps the flows for follow-up tests.
https://claude.ai/code/session_01RjhCqbKkgNPEdfCYaQ4SsS
jaredglaser
force-pushed
the
claude/playwright-test-suite-lfyzhx
branch
from
9c5a2ae to
f0ce5ae
Compare
- playwright.config.ts described per-scenario overrides as using page.route, but page.route cannot intercept /_serverFn (the MSW service worker answers first). Point at the overrideServerFn fixture and its window-seeded map instead. - Remove the empty afterEach in the server-functions handler test; the console spy is already restored in the test that creates it. https://claude.ai/code/session_01X7fcidwF7hwVgVZZhxCEPc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/playwright-test-plan.md (1)
32-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix markdown formatting: add blank lines before subsection headings and restart list numbering per section.
The document has two markdown linting violations that will fail CI:
- MD022 (blanks-around-headings): Subsection headings (
### Docker,### Stacks,### ZFS,### Proxmox,### Settings,### Cross-cutting) need a blank line above them.- MD029 (ol-prefix): Each subsection's ordered list should restart at 1, not continue numbering from the previous section (list items 8–11 belong to Stacks, 12 to ZFS, etc.; they should be 1–4, 1–2, etc.).
✅ Proposed fix
## What belongs in Playwright (and what does not) Unit tests (`bun test`) already cover pure logic, hooks in isolation, row converters, schema validation, and component rendering with injected props. They run in Happy-DOM, so they cannot exercise: real layout and overflow, the service worker, EventSource streaming end to end, canvas/WebGL charts, the virtualizer's real measurement, cross-tab broadcast, focus/scroll, or multi-step navigation with live data. Playwright is worth the cost only when a test needs one of those. The flows below are chosen on that basis. Each notes the MSW/`page.route` setup that drives it. +## App target: deep flows + -## App target: deep flows ### Docker + -1. **Live stats stream updates the table.** Stat values and sparklines change over +1. **Live stats stream updates the table.** Stat values and sparklines change overThen for each subsequent subsection (Stacks, ZFS, Proxmox, Settings, Cross-cutting), insert a blank line before the heading and restart the list at 1.
Full corrected structure:
## App target: deep flows ### Docker 1. ... 2. ... 3. ... 4. ... 5. ... 6. ... 7. ... ### Stacks 8. **Deploy lifecycle.** ... 9. **Compose editor ...** ...should become:
## App target: deep flows ### Docker 1. ... 2. ... 3. ... 4. ... 5. ... 6. ... 7. ... ### Stacks 1. **Deploy lifecycle.** ... 2. **Compose editor ...** ...🤖 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 `@docs/playwright-test-plan.md` around lines 32 - 113, Add blank lines before each of the six subsection headings: ### Docker, ### Stacks, ### ZFS, ### Proxmox, ### Settings, and ### Cross-cutting. Then renumber each subsection's ordered list to start at 1: the Docker section items stay 1–7, the Stacks section items 8–11 become 1–4, ZFS item 12 becomes 1, Proxmox item 13 becomes 1, Settings items 14–16 become 1–3, and Cross-cutting items 17–20 become 1–4.
🤖 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 `@src/lib/mock/functions/git-tokens.functions.ts`:
- Around line 1-5: The header comment at the top of git-tokens.functions.ts
(lines 1-5) inaccurately describes the module's behavior as "static,
side-effect-free data." Update this comment to accurately reflect that the
module now intentionally mutates in-memory state to persist session data,
removing the misleading description about side-effect-free behavior while
maintaining the explanation of why this mock exists (for demo/e2e mode without a
database).
- Around line 7-12: The MockGitToken interface diverges from the real server
DTOs in three ways: it uses `name` instead of `label`, represents date fields as
strings instead of Date objects, and does not enforce non-empty label
validation. Update the MockGitToken interface to replace the `name` property
with `label`, change `createdAt` from string to Date, change `lastUsedAt` from
string | null to Date | null, and ensure any code that constructs or validates
MockGitToken instances (around lines 30-42 in the same file) enforces that
`label` is a non-empty string to match the production contract.
In `@src/lib/mock/handlers/__tests__/sse-stream.test.ts`:
- Around line 5-12: The readFrames() function incorrectly assumes each
reader.read() result is a complete SSE frame, but stream chunking does not align
with frame boundaries. This causes flaky tests when frames are split across
multiple chunks or multiple frames arrive in a single chunk. Fix this by
accumulating all read data into a buffer string, then splitting the buffer by
the SSE frame delimiter (typically double newline "\n\n" or single newline "\n"
depending on the SSE format used), and extracting the appropriate number of
parsed frames from the resulting array rather than counting raw read results.
In `@src/lib/mock/handlers/sse.ts`:
- Around line 20-24: The loadDemoSettings() function parses JSON from
localStorage and immediately casts it to Record<string, string> without
validating the actual shape of the parsed data. If the stored JSON contains
non-string values or is not an object structure, the function will return
malformed data that violates the expected Record<string, string> type. Add
validation logic after JSON.parse() to verify that the parsed value is actually
an object and that all of its values are strings before returning it. If the
stored settings do not match the expected shape, handle it appropriately (such
as returning an empty object or falling through to the catch block).
---
Outside diff comments:
In `@docs/playwright-test-plan.md`:
- Around line 32-113: Add blank lines before each of the six subsection
headings: ### Docker, ### Stacks, ### ZFS, ### Proxmox, ### Settings, and ###
Cross-cutting. Then renumber each subsection's ordered list to start at 1: the
Docker section items stay 1–7, the Stacks section items 8–11 become 1–4, ZFS
item 12 becomes 1, Proxmox item 13 becomes 1, Settings items 14–16 become 1–3,
and Cross-cutting items 17–20 become 1–4.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d54224d8-1185-4668-b111-08c0952e0456
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.gitignoreCLAUDE.mdbunfig.tomldocs/development.mddocs/playwright-test-plan.mde2e/demo.demo.spec.tse2e/fixtures.tse2e/smoke.spec.tspackage.jsonplaywright.config.tspublic/mockServiceWorker.jsscripts/serve-static.tssrc/client.tsxsrc/components/AppShell.tsxsrc/lib/constants/mock.tssrc/lib/mock/__tests__/mock-event-source.test.tssrc/lib/mock/browser.tssrc/lib/mock/functions/git-tokens.functions.tssrc/lib/mock/functions/hosts.functions.tssrc/lib/mock/handlers/__tests__/function-id.test.tssrc/lib/mock/handlers/__tests__/server-functions.test.tssrc/lib/mock/handlers/__tests__/sse-stream.test.tssrc/lib/mock/handlers/function-id.tssrc/lib/mock/handlers/index.tssrc/lib/mock/handlers/server-functions.tssrc/lib/mock/handlers/sse-stream.tssrc/lib/mock/handlers/sse.tssrc/lib/mock/install-demo.tssrc/lib/mock/mock-event-source.tssrc/lib/mock/start.tssrc/routes/__root.tsxvite.config.ts
💤 Files with no reviewable changes (4)
- src/lib/mock/mock-event-source.ts
- src/lib/mock/install-demo.ts
- src/components/AppShell.tsx
- src/lib/mock/tests/mock-event-source.test.ts
Comment on lines
+5
to
+12
| async function readFrames(response: Response, count: number): Promise<string[]> { | ||
| const reader = response.body!.pipeThrough(new TextDecoderStream()).getReader(); | ||
| const frames: string[] = []; | ||
| while (frames.length < count) { | ||
| const { value, done } = await reader.read(); | ||
| if (done) break; | ||
| if (value) frames.push(value); | ||
| } |
Contributor
There was a problem hiding this comment.
Parse SSE frames by delimiter instead of per-read chunk.
readFrames() treats each reader.read() result as a full frame, but stream chunking is not frame-aligned. This can make the tests flaky when frames are split or coalesced.
Suggested fix
async function readFrames(response: Response, count: number): Promise<string[]> {
const reader = response.body!.pipeThrough(new TextDecoderStream()).getReader();
const frames: string[] = [];
+ let buffer = '';
while (frames.length < count) {
const { value, done } = await reader.read();
if (done) break;
- if (value) frames.push(value);
+ if (!value) continue;
+ buffer += value;
+ const parts = buffer.split('\n\n');
+ buffer = parts.pop() ?? '';
+ for (const part of parts) {
+ if (!part) continue;
+ frames.push(`${part}\n\n`);
+ if (frames.length >= count) break;
+ }
}
await reader.cancel();
return frames;
}🤖 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 `@src/lib/mock/handlers/__tests__/sse-stream.test.ts` around lines 5 - 12, The
readFrames() function incorrectly assumes each reader.read() result is a
complete SSE frame, but stream chunking does not align with frame boundaries.
This causes flaky tests when frames are split across multiple chunks or multiple
frames arrive in a single chunk. Fix this by accumulating all read data into a
buffer string, then splitting the buffer by the SSE frame delimiter (typically
double newline "\n\n" or single newline "\n" depending on the SSE format used),
and extracting the appropriate number of parsed frames from the resulting array
rather than counting raw read results.
Sessions previously persisted the full OIDC token set (access, refresh,
and ID tokens) JWE-encrypted in the sessions table, but only the ID
token is ever read back (as id_token_hint for RP-initiated logout). The
access token is consumed during the callback for the userinfo lookup
and the refresh token was never used at all.
createSession now takes just the ID token and encrypts { idToken }.
getIdToken keeps parsing legacy three-field payloads, so sessions
created before this change still produce a logout hint.
Closes #246
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The session payload now stores only { idToken }, so the legacy tolerance
for the old { idToken, accessToken, refreshToken } shape is just JSON.parse
ignoring extra keys, not real code. Remove the comment and dedicated test
that documented it, and trim the verbose comments added alongside the
id_token-only change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mo settings The git-token mock returned `name` and string timestamps, so the settings GitTokensTable rendered blank label/user cells in demo and e2e. Match the real GitTokenWithUser DTO (label, Date timestamps, userId/userName/userEmail), reject empty labels like the z.string().min(1) validator, and guard loadDemoSettings against non-string-record JSON in localStorage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Heads up from the #281 review (session cookie: Why this harness can't test it. Both targets are static builds over MSW with no real backend. What "properly testing it" takes (full plan + acceptance criteria in #308):
What this lets #281 shed once the lane exists:
Implementation plan and acceptance criteria: #308. Not blocking this PR; flagging so the auth lane is on the radar as the deep-flow work continues. Generated by Claude Code |
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
2 participants
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.
Part 1 of 2. Sets up Playwright and moves demo mode from the Vite alias swap +
MockEventSourceto an in-browser MSW mock backend, so the same handlers power both the demo site and the e2e tests.What changed
src/lib/mock/handlers/): a service worker intercepts the real network calls instead of swapping modules at build time:/_serverFn/*) are answered with the real seroval{result, error, context}envelope +x-tss-serializedheader. A bare JSON value resolves toundefinedon the client, so matching this format exactly is required./api/*) are answered with streamingtext/event-streambuilt from the existing generators (replacesMockEventSource).src/client.tsx) beforehydrateRoot— the only point early enough to intercept route loaders (a React-level gate is too late). Gated byIS_MOCK_ENABLED: on for demo (VITE_DEMO_MODE), orVITE_ENABLE_MSW=truefor the e2e app target.serverFns.generateFunctionIdinvite.config.tskeeps production ids decodable by the handler.demosmoke target and the real-appappdeep target, with anoverrideServerFnfixture for per-scenario response shaping. One basic smoke test on each.e2ejob;docs/playwright-test-plan.mdmaps the flows for the follow-up.Why production builds (not a dev server)
vite devruns a server-side render pass and re-optimizes deps mid-session, which races MSW and 404s route chunks. A static build has neither, so loaders/auth/SSE are all intercepted before first render.Validation
bun run test:e2egreen (both targets).bun run typecheck:allclean.msw/@playwright/testdeps.Part 2 (deep tests) is stacked on this branch.
https://claude.ai/code/session_01RjhCqbKkgNPEdfCYaQ4SsS
Generated by Claude Code
Summary by CodeRabbit
Tests
Documentation
Chores