test(34): E2E test expansion and staging performance baselines#392
Merged
Conversation
Entire-Checkpoint: 5e6526bb9f68
- Create tests/load/baselines/byo-load-test-plan.md with execution plan - Document 3 BYO load test scenarios (upload, mixed, capacity ceiling) - Include environment variables, execution commands, and expected metrics - Reference Phase 19.2/22 non-BYO baselines from docs/CAPACITY.md - Pinata recommended as provider (PinataProvider already implemented) - Explicit DEFERRED status: awaiting external IPFS provider infrastructure
- Create 34-04-SUMMARY.md (partial: Task 1 complete, Task 2 pending) - Update STATE.md position to Phase 34 Plan 04 checkpoint - Update ROADMAP.md plan progress for Phase 34
…into multi-account utility - Add cleanup-helpers.ts with deleteAccountViaPage that calls /auth/refresh + DELETE /auth/account - Wraps entire function in try/catch with console.warn (never throws in afterAll teardown) - Update closeWalletTestAccounts to delete accounts before closing browser contexts - Each account deletion wrapped individually so one failure doesn't block others
- Single-account specs (full-workflow, search, recycle-bin, mfa, conflict, wallet-login): import + call deleteAccountViaPage before context.close() - Multi-account specs (sharing, writable-shares, invite-link): closeWalletTestAccounts now handles deletion (from Task 1) - Hybrid spec (journey-timing): deleteAccountViaPage for Alice + closeWalletTestAccounts for Bob - wallet-login.spec.ts uses afterEach (fixture-based tests) for TC09 cleanup - conflict-detection: single deletion suffices (both sessions share same wallet identity)
- Create 34-01-SUMMARY.md with execution results - Update STATE.md position to Phase 34 Plan 1/4 - Update ROADMAP.md with plan progress
- Add test-video.mp4 (300KB, >256KB CTR threshold) - Add test-video-small.mp4 (100KB, <256KB GCM fallback) - Add test-audio.mp3 (300KB, >256KB CTR threshold) - Add test-document.pdf (valid PDF with text content) - Add createTestMediaFile() to test-files.ts for fixture copying - Update .gitignore to track committed media fixtures
- streaming-playback.spec.ts: 6 tests covering CTR mode activation, encrypted badge, decrypt progress, and GCM blob URL fallback - media-preview.spec.ts: 5 tests covering PDF canvas viewer, video player modal, audio player modal, and corrupt file error state - Both suites use deleteAccountViaPage cleanup in afterAll
- SUMMARY.md documents 11 new E2E tests across 2 spec files - STATE.md updated to plan 2/4 complete - ROADMAP.md progress updated for phase 34
- playwright.config.ts: support BASE_URL env var, skip webServer for external targets - journey-timing.spec.ts: increase wallet init timeout to 45s for staging - cleanup-helpers.ts: derive API URL from page URL for staging environments - baselines/staging-journey-timing.json: captured staging journey timing baselines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0eb210591a47
- 5 tests covering multi-file selection and batch download via selection action bar - Verifies selection bar count, download event trigger, and batch context menu - Uses deleteAccountViaPage for afterAll cleanup - Confirms batch download fires individual file downloads (not zip)
- SUMMARY.md with task commits and plan metrics - STATE.md updated with plan progress - ROADMAP.md updated via gsd-tools Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 clients x ~70 ops: 305/395 succeeded (0.71 ops/sec). C4/C5 context exhaustion visible at 5 concurrent browser sessions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b0a8b6b36d59
Pinata provider now configured in tests/load/.env. Updated execution instructions and prerequisites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2113055f92e4
200 clients requested, 10 created due to 429 rate limit. Documents throttle bypass gap in SDK test harness for future fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c3955e674528
Journey timing, browser load test, and SDK sustained load baselines all captured. BYO plan upgraded to ACTIVE with Pinata. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b95dfd042bfe
Entire-Checkpoint: 422df895c42c
Documents investigation into why THROTTLE_BYPASS_SECRET doesn't prevent 429s in vitest load tests against staging, despite working with curl and direct Node fetch. CI avoids the issue via NODE_ENV=test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a97d8f0b009f
… bug Root cause was running two load tests simultaneously against staging. Bypass header works correctly when tests run sequentially. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c6e17a344aa0
200 clients x 2 ops/sec x 5min = 11,174 ops, 0.17% error rate. createFolder p50=6.5s, uploadFile p50=8.1s, deleteItem p50=3.2s. p95 latency elevated under full contention but error rate minimal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1cb9ba053541
…ures All 5 tiers completed: 50, 100, 200, 500, 1000 clients. Account creation and API ops succeeded but all Pinata byo-pin operations failed with fetch errors. Pinata endpoint or auth config needs investigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 18e9b7f4db69
403 error confirms JWT auth works but quota is exhausted. Need paid plan or fresh account for successful pin baselines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 5d89428fe42f
200-client sustained load: 11,174 ops, 0.17% error rate. BYO ceiling: all 5 tiers ran, Pinata free tier limits exhausted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 5eff2a8b6367
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2df3efa96a64
- Merge double page.evaluate into single call in cleanup-helpers.ts - Replace inline deleteAccount in load-test.spec.ts with shared helper - Move dynamic fs/path imports to top-level in media-preview.spec.ts - Fix TOCTOU: try/catch unlinkSync instead of existsSync check - Use try/finally for corrupt file cleanup to prevent leaks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 702da977d914
The SDK load test harness (tests/load/) drives 200+ headless clients directly against the API with per-operation metrics. The Playwright browser load test (5 Chromium tabs) measured browser overhead more than API performance and provided no additional signal. Removed: - tests/web-e2e/tests/load-test.spec.ts - tests/web-e2e/playwright.load.config.ts - tests/web-e2e/baselines/staging-load-test.json - testIgnore for load-test.spec.ts in playwright.config.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 665ca031ee1a
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughPhase 34 marked complete; adds an in-page Playwright account-teardown helper wired into many E2E specs, three new serial E2E suites and media fixture utilities, removes the previous Playwright load-test spec/config, and adds BYO‑IPFS load-test plans, workflow inputs, and staging baseline artifacts. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/load/baselines/byo-load-test-plan.md`:
- Around line 3-13: The document incorrectly marks the BYO load test as ACTIVE
and claims BYO config seeding is completed; update
tests/load/baselines/byo-load-test-plan.md to set the status to DEFERRED and
change the prerequisites text to reflect that BYO provider config seeding is not
persisted by the harness (remove or reword the claim that "Test harness creates
accounts with BYO provider config automatically"), referencing the
client-pool.ts behavior and the open TODO around register-cid 400s to clarify
that seeding is not done automatically and that manual setup or future
persistence is required.
In `@tests/web-e2e/tests/batch-download.spec.ts`:
- Around line 142-153: The test title says it should verify the batch context
menu shows a download option, but it only asserts the menu and header text; add
an assertion that the download item is present (e.g. after obtaining headerText
and isBatchMenu, call a contextMenu method to check for the download entry).
Locate the block using fileList.rightClickItem(file2Name),
contextMenu.waitForOpen(), isBatchMenu() and getHeaderText(), and add a line
such as expect(await contextMenu.hasMenuItem('Download')).toBe(true) or
expect((await contextMenu.getMenuItems()).map(t =>
t.toLowerCase())).toContain('download') immediately after the header assertion
so the test actually verifies the download option is shown.
In `@tests/web-e2e/tests/media-preview.spec.ts`:
- Around line 3-6: The test builds a fixture path using process.cwd() (fragile)
instead of the deterministic helper; replace any usage of process.cwd() in the
media-preview.spec.ts test where the fixture path is constructed with
getTestFilePath() from the test-file utilities, update the import list to
include getTestFilePath alongside createTestMediaFile and cleanupTestFiles, and
pass the resulting path into createTestMediaFile (or wherever the path is
consumed) so the fixture resolution is consistent across Playwright run
contexts.
In `@tests/web-e2e/utils/test-files.ts`:
- Around line 146-156: The createTestMediaFile function currently accepts raw
sourceFixture and name allowing ../ to escape FIXTURES_DIR; fix it by
normalizing and validating both fixturePath and filePath after resolving them
(use resolve and then ensure path.relative(FIXTURES_DIR, resolvedPath) does not
start with '..' and is not absolute outside the directory), reject or throw if
either resolves outside FIXTURES_DIR, and only then perform copyFileSync and
push the validated filePath into createdFiles; reference the createTestMediaFile
function and the variables fixturePath, filePath, and createdFiles when making
this change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e673b5a6-923d-4917-a32a-016ae9274c07
⛔ Files ignored due to path filters (4)
tests/web-e2e/fixtures/files/test-audio.mp3is excluded by!**/*.mp3tests/web-e2e/fixtures/files/test-document.pdfis excluded by!**/*.pdftests/web-e2e/fixtures/files/test-video-small.mp4is excluded by!**/*.mp4tests/web-e2e/fixtures/files/test-video.mp4is excluded by!**/*.mp4
📒 Files selected for processing (35)
.learnings/2026-03-29-staging-load-test-throttle-bypass.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-01-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-01-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-02-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-02-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-03-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-03-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-04-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-04-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-RESEARCH.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-VERIFICATION.md.planning/todos/pending/2026-03-29-debug-byo-ipfs-register-cid-400-errors-on-staging.mdtests/load/baselines/byo-load-test-plan.mdtests/load/baselines/staging-byo-capacity-ceiling.jsontests/load/baselines/staging-sustained-load.jsontests/web-e2e/.gitignoretests/web-e2e/baselines/staging-journey-timing.jsontests/web-e2e/playwright.config.tstests/web-e2e/playwright.load.config.tstests/web-e2e/tests/batch-download.spec.tstests/web-e2e/tests/conflict-detection.spec.tstests/web-e2e/tests/full-workflow.spec.tstests/web-e2e/tests/journey-timing.spec.tstests/web-e2e/tests/load-test.spec.tstests/web-e2e/tests/media-preview.spec.tstests/web-e2e/tests/mfa-flows.spec.tstests/web-e2e/tests/recycle-bin.spec.tstests/web-e2e/tests/search-workflow.spec.tstests/web-e2e/tests/streaming-playback.spec.tstests/web-e2e/tests/wallet-login.spec.tstests/web-e2e/utils/cleanup-helpers.tstests/web-e2e/utils/multi-account-wallet.tstests/web-e2e/utils/test-files.ts
💤 Files with no reviewable changes (2)
- tests/web-e2e/playwright.load.config.ts
- tests/web-e2e/tests/load-test.spec.ts
Add byo-upload-throughput, byo-mixed-workload, byo-capacity-ceiling to scenario dropdown. Pass BYO_IPFS_* env vars from GitHub environment vars/secrets when targeting staging. Requires GitHub environment config: - vars: BYO_IPFS_ENDPOINT, BYO_IPFS_PROTOCOL, BYO_IPFS_PROVIDER_NAME - secrets: BYO_IPFS_AUTH_TOKEN Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a18fd3b58711
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/load-test.yml:
- Around line 180-184: Only export the BYO IPFS env vars when the workflow is
running a BYO scenario: wrap the four echo lines that set BYO_IPFS_ENDPOINT,
BYO_IPFS_AUTH_TOKEN, BYO_IPFS_PROTOCOL, and BYO_IPFS_PROVIDER_NAME in a
conditional that checks the selected scenario (e.g., check the SCENARIO/workflow
input/env value starts with "byo-" or a dedicated BYO flag) so these secrets are
not written to GITHUB_ENV for non-BYO runs; update the step containing those
echo commands to perform that conditional check before exporting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39031a8e-c8f8-48fa-8474-20e62e22fa47
📒 Files selected for processing (1)
.github/workflows/load-test.yml
BYO endpoint, protocol, and provider name are now workflow_dispatch inputs with defaults — different providers can be evaluated against each other without changing environment config. Auth token remains a secret: LOAD_TEST_BYO_AUTH_TOKEN Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1e025a001989
- BYO load test plan: status ACTIVE -> PARTIAL, note pending config seeding - batch-download.spec.ts: add download menuitem assertion - media-preview.spec.ts: use getTestFilePath instead of process.cwd - test-files.ts: sanitize paths with basename to prevent directory traversal - load-test.yml: scope BYO env vars to byo-* scenarios only Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: d2aece93b5bb
Contributor
There was a problem hiding this comment.
Pull request overview
Expands the Playwright E2E test suite to cover media streaming/preview and batch download flows, adds shared best-effort account cleanup to prevent orphaned test users, and records staging/load baseline artifacts and related planning docs.
Changes:
- Added shared
deleteAccountViaPageteardown helper and integrated it across E2E specs (including multi-account cleanup). - Added new E2E specs for AES-CTR streaming playback, media preview dialogs, and batch download; committed media fixtures.
- Updated Playwright config to support
BASE_URLoverrides (and skip local webServer for external targets); removed legacy browser-based load-test config/spec; added/updated baseline + planning docs and workflow inputs for BYO load testing.
Reviewed changes
Copilot reviewed 36 out of 40 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/web-e2e/utils/test-files.ts | Adds createTestMediaFile helper to copy committed media fixtures and track cleanup. |
| tests/web-e2e/utils/multi-account-wallet.ts | Ensures multi-account teardown deletes accounts before closing contexts. |
| tests/web-e2e/utils/cleanup-helpers.ts | New helper to delete the logged-in test account via in-page fetch calls. |
| tests/web-e2e/tests/wallet-login.spec.ts | Adds best-effort per-test account cleanup. |
| tests/web-e2e/tests/streaming-playback.spec.ts | New E2E coverage for AES-CTR streaming vs GCM fallback behavior. |
| tests/web-e2e/tests/media-preview.spec.ts | New E2E coverage for PDF/video/audio previews + corrupt media error path. |
| tests/web-e2e/tests/batch-download.spec.ts | New E2E coverage for multi-select and batch download event triggering. |
| tests/web-e2e/tests/search-workflow.spec.ts | Adds account deletion to suite teardown. |
| tests/web-e2e/tests/recycle-bin.spec.ts | Adds account deletion to suite teardown. |
| tests/web-e2e/tests/mfa-flows.spec.ts | Adds account deletion to suite teardown. |
| tests/web-e2e/tests/journey-timing.spec.ts | Adds account deletion for teardown + increases wallet init timeout for staging. |
| tests/web-e2e/tests/full-workflow.spec.ts | Adds account deletion to suite teardown. |
| tests/web-e2e/tests/conflict-detection.spec.ts | Adds account deletion to suite teardown. |
| tests/web-e2e/tests/load-test.spec.ts | Removes legacy browser-driven Playwright load test. |
| tests/web-e2e/playwright.load.config.ts | Removes legacy load-test-specific Playwright config. |
| tests/web-e2e/playwright.config.ts | Supports BASE_URL override and conditionally skips local webServer startup. |
| tests/web-e2e/.gitignore | Allows committed media fixtures while still ignoring generated fixture files. |
| tests/web-e2e/fixtures/files/test-video.mp4 | Adds committed video fixture for CTR streaming threshold testing. |
| tests/web-e2e/fixtures/files/test-video-small.mp4 | Adds committed small video fixture for GCM fallback testing. |
| tests/web-e2e/fixtures/files/test-audio.mp3 | Adds committed audio fixture for preview tests. |
| tests/web-e2e/fixtures/files/test-document.pdf | Adds committed PDF fixture for preview tests. |
| tests/web-e2e/baselines/staging-journey-timing.json | Adds staging journey timing baseline artifact. |
| tests/load/baselines/staging-sustained-load.json | Adds staging sustained SDK load baseline artifact. |
| tests/load/baselines/staging-byo-capacity-ceiling.json | Adds BYO capacity ceiling baseline artifact. |
| tests/load/baselines/byo-load-test-plan.md | Adds BYO load-test plan doc for running/recording baselines. |
| .planning/todos/pending/2026-03-29-debug-byo-ipfs-register-cid-400-errors-on-staging.md | Tracks follow-up investigation for staging BYO register-cid 400s. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-VERIFICATION.md | Adds verification report for Phase 34 deliverables. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-RESEARCH.md | Adds research notes supporting the E2E/baseline work. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-04-SUMMARY.md | Adds summary for staging baselines + BYO plan updates. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-04-PLAN.md | Adds plan for staging baselines + BYO plan documentation. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-03-SUMMARY.md | Adds summary for batch download E2E coverage. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-03-PLAN.md | Adds plan for batch download E2E coverage. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-02-SUMMARY.md | Adds summary for streaming + media preview E2E coverage and fixtures. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-02-PLAN.md | Adds plan for streaming + media preview E2E coverage and fixtures. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-01-SUMMARY.md | Adds summary for shared account cleanup wiring. |
| .planning/phases/34-e2e-test-expansion-staging-baselines/34-01-PLAN.md | Adds plan for shared account cleanup wiring. |
| .planning/STATE.md | Updates milestone/state tracking for Phase 34 completion. |
| .planning/ROADMAP.md | Marks Phase 34 complete and updates success criteria wording. |
| .learnings/2026-03-29-staging-load-test-throttle-bypass.md | Adds operational learning about staging load-test concurrency causing 429s. |
| .github/workflows/load-test.yml | Adds BYO scenario inputs and wires BYO provider env vars for workflow runs. |
- Guard page/context in afterAll hooks across 3 new specs - Wrap context.close() in try/catch in closeWalletTestAccounts - Include 127.0.0.1 in local target detection for playwright config - Fix VERIFICATION.md: remove stale staging-load-test.json ref, update sustained-load description to reflect 200-client run Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 20c3db6e1564
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
+ Coverage 61.15% 61.18% +0.03%
==========================================
Files 132 132
Lines 9750 9750
Branches 976 978 +2
==========================================
+ Hits 5963 5966 +3
+ Misses 3573 3570 -3
Partials 214 214
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Owner
Author
|
@coderabbitai please review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/web-e2e/tests/batch-download.spec.ts`:
- Around line 122-124: Selection is being cleared via await
page.keyboard.press('Escape') but tests don't wait for the UI to reflect that,
causing flaky inherited selection; after each await
page.keyboard.press('Escape') (the two occurrences in this spec), add an
explicit await page.waitForSelector('<selection-indicator-selector>', { state:
'hidden' }) — where <selection-indicator-selector> is the DOM element used by
the app to show selected items (e.g. a selected row class, selection toolbar, or
overlay) so the test only proceeds once the selection UI is gone.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b30ae50-e16f-4c1e-9f0d-ef36a90a204b
📒 Files selected for processing (9)
.github/workflows/load-test.yml.planning/phases/34-e2e-test-expansion-staging-baselines/34-VERIFICATION.mdtests/load/baselines/byo-load-test-plan.mdtests/web-e2e/playwright.config.tstests/web-e2e/tests/batch-download.spec.tstests/web-e2e/tests/media-preview.spec.tstests/web-e2e/tests/streaming-playback.spec.tstests/web-e2e/utils/multi-account-wallet.tstests/web-e2e/utils/test-files.ts
✅ Files skipped from review due to trivial changes (3)
- .planning/phases/34-e2e-test-expansion-staging-baselines/34-VERIFICATION.md
- tests/load/baselines/byo-load-test-plan.md
- tests/web-e2e/tests/streaming-playback.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/web-e2e/utils/multi-account-wallet.ts
- tests/web-e2e/utils/test-files.ts
- tests/web-e2e/tests/media-preview.spec.ts
- tests/web-e2e/playwright.config.ts
- .github/workflows/load-test.yml
Prevents stale selection state leaking between serial tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6b5ed854adec
- byo-load-test-plan.md: clarify timeline of pin success vs ceiling 403s, add pinata to protocol table - todo: add timeline note distinguishing single-client success from capacity ceiling 403 failures - load-test.yml: fail fast if LOAD_TEST_BYO_AUTH_TOKEN secret is missing for byo-* scenarios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 56f2a0194e8b
Owner
Author
|
@coderabbitai please review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Add COMMITTED_FIXTURES set that prevents createTestTextFile, createTestBinaryFile, createTestImageFile, and createTestMediaFile from overwriting committed fixture files. Throws early if the destination name matches any existing fixture. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 721f601d4769
Owner
Author
|
@coderabbitai please review. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/web-e2e/utils/test-files.ts (1)
158-168: Enforce “committed source fixture” contract explicitly.
createTestMediaFilesanitizessourceFixture, but it doesn’t verify that the source is one of the committed fixtures. Adding that check avoids accidental coupling to runtime-generated files.🔧 Suggested hardening
export function createTestMediaFile(sourceFixture: string, name?: string): TestBinaryFile { const timestamp = Date.now(); const safeSource = basename(sourceFixture); + if (!COMMITTED_FIXTURES.has(safeSource)) { + throw new Error(`Unknown committed fixture: ${safeSource}`); + } const ext = extname(safeSource).replace('.', '') || 'bin'; const fileName = basename(name || `test-media-${timestamp}.${ext}`); if (COMMITTED_FIXTURES.has(fileName)) { throw new Error(`Refusing to overwrite committed fixture: ${fileName}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web-e2e/utils/test-files.ts` around lines 158 - 168, The function createTestMediaFile currently sanitizes the provided sourceFixture but doesn't assert that the source is one of the approved committed fixtures; add an explicit check early in createTestMediaFile to ensure the sanitized safeSource (basename(sourceFixture)) is contained in the COMMITTED_FIXTURES set and throw a clear error if not, before resolving fixturePath or reading file stats (references: createTestMediaFile, safeSource, COMMITTED_FIXTURES, fixturePath, statSync).tests/web-e2e/tests/media-preview.spec.ts (1)
110-118: Minor: Redundant visibility assertion after waitFor.Line 112 already waits for the modal to become visible. The subsequent
isVisible()check on line 113 is redundant.♻️ Suggested simplification
// Wait for audio player modal -- audio uses a hidden <audio> element // with canvas visualization, so we check for the modal container await page.locator('.audio-player-modal').waitFor({ state: 'visible', timeout: 30_000 }); - expect(await page.locator('.audio-player-modal').isVisible()).toBe(true); + // Visibility is guaranteed by waitFor above // Close modal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web-e2e/tests/media-preview.spec.ts` around lines 110 - 118, Remove the redundant visibility assertion: after calling page.locator('.audio-player-modal').waitFor({ state: 'visible', ... }) there is no need to call page.locator('.audio-player-modal').isVisible() — delete the isVisible() expect and keep the waitFor check (the same '.audio-player-modal' locator and waitFor call remain); ensure the subsequent modal close logic (page.keyboard.press('Escape') and the waitFor({ state: 'hidden' })) is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/web-e2e/baselines/staging-journey-timing.json`:
- Around line 1-19: Add the missing top-level boolean metadata field
"faro_enabled" to this baseline JSON (alongside "captured" and "environment") so
metadata ingestion passes validation; set "faro_enabled" to true for the staging
baseline and ensure it is present as a sibling to "captured", "environment",
"journeys", and "phases".
In `@tests/web-e2e/utils/cleanup-helpers.ts`:
- Around line 14-16: Add an explicit page lifecycle guard in
deleteAccountViaPage before calling page.evaluate to avoid calling evaluate on a
closed page; check page.isClosed() (or equivalent) and return early (or log and
return) when the page is closed so that the try/catch only covers real runtime
errors from evaluate() and not predictable closed-page skips — update the guard
immediately before the page.evaluate(...) invocation in deleteAccountViaPage.
---
Nitpick comments:
In `@tests/web-e2e/tests/media-preview.spec.ts`:
- Around line 110-118: Remove the redundant visibility assertion: after calling
page.locator('.audio-player-modal').waitFor({ state: 'visible', ... }) there is
no need to call page.locator('.audio-player-modal').isVisible() — delete the
isVisible() expect and keep the waitFor check (the same '.audio-player-modal'
locator and waitFor call remain); ensure the subsequent modal close logic
(page.keyboard.press('Escape') and the waitFor({ state: 'hidden' })) is
unchanged.
In `@tests/web-e2e/utils/test-files.ts`:
- Around line 158-168: The function createTestMediaFile currently sanitizes the
provided sourceFixture but doesn't assert that the source is one of the approved
committed fixtures; add an explicit check early in createTestMediaFile to ensure
the sanitized safeSource (basename(sourceFixture)) is contained in the
COMMITTED_FIXTURES set and throw a clear error if not, before resolving
fixturePath or reading file stats (references: createTestMediaFile, safeSource,
COMMITTED_FIXTURES, fixturePath, statSync).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a688941-8053-4302-81c5-8da06058482e
⛔ Files ignored due to path filters (4)
tests/web-e2e/fixtures/files/test-audio.mp3is excluded by!**/*.mp3tests/web-e2e/fixtures/files/test-document.pdfis excluded by!**/*.pdftests/web-e2e/fixtures/files/test-video-small.mp4is excluded by!**/*.mp4tests/web-e2e/fixtures/files/test-video.mp4is excluded by!**/*.mp4
📒 Files selected for processing (36)
.github/workflows/load-test.yml.learnings/2026-03-29-staging-load-test-throttle-bypass.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-01-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-01-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-02-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-02-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-03-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-03-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-04-PLAN.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-04-SUMMARY.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-RESEARCH.md.planning/phases/34-e2e-test-expansion-staging-baselines/34-VERIFICATION.md.planning/todos/pending/2026-03-29-debug-byo-ipfs-register-cid-400-errors-on-staging.mdtests/load/baselines/byo-load-test-plan.mdtests/load/baselines/staging-byo-capacity-ceiling.jsontests/load/baselines/staging-sustained-load.jsontests/web-e2e/.gitignoretests/web-e2e/baselines/staging-journey-timing.jsontests/web-e2e/playwright.config.tstests/web-e2e/playwright.load.config.tstests/web-e2e/tests/batch-download.spec.tstests/web-e2e/tests/conflict-detection.spec.tstests/web-e2e/tests/full-workflow.spec.tstests/web-e2e/tests/journey-timing.spec.tstests/web-e2e/tests/load-test.spec.tstests/web-e2e/tests/media-preview.spec.tstests/web-e2e/tests/mfa-flows.spec.tstests/web-e2e/tests/recycle-bin.spec.tstests/web-e2e/tests/search-workflow.spec.tstests/web-e2e/tests/streaming-playback.spec.tstests/web-e2e/tests/wallet-login.spec.tstests/web-e2e/utils/cleanup-helpers.tstests/web-e2e/utils/multi-account-wallet.tstests/web-e2e/utils/test-files.ts
💤 Files with no reviewable changes (2)
- tests/web-e2e/playwright.load.config.ts
- tests/web-e2e/tests/load-test.spec.ts
- Add missing faro_enabled field to staging journey timing baseline - Add page.isClosed() guard in deleteAccountViaPage before evaluate Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: d57187304e9f
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
deleteAccountViaPagehelper wired into all 10 E2E specs to prevent orphaned test accounts in the databasestreaming-playback.spec.ts(6 tests) andmedia-preview.spec.ts(5 tests) covering AES-CTR streaming, GCM fallback, PDF/video/audio preview dialogsbatch-download.spec.ts(5 tests) covering multi-select, selection bar, and download event verificationBASE_URLenv var for staging runs, cleanup helper derives API URL from page originload-test.spec.ts) — redundant with SDK load harness that drives 200+ headless clientsStaging Baseline Results
Test plan
cd tests/web-e2e && pnpm exec playwright test --listshows 16 new tests across 3 specsgrep -r deleteAccountViaPage tests/web-e2e/tests/ | wc -lshows cleanup in 10 specsls tests/web-e2e/fixtures/files/test-video.mp4ls tests/web-e2e/baselines/ tests/load/baselines/load-test.spec.tsandplaywright.load.config.tsno longer exist🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Documentation