Add Playwright e2e tests for upload interception - #68
Conversation
Cover image block upload, media library drop, editor canvas drop, featured image drop, and Media Library admin upload against wp-env. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change adds a Playwright end-to-end test setup for WordPress upload interception. It includes local and CI configuration, authenticated request utilities, shared media helpers, and JPG-to-WebP coverage across multiple upload flows. ChangesE2E upload interception
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@e2e/playwright.config.ts`:
- Around line 21-26: Remove ignoreHTTPSErrors from the Playwright use
configuration in e2e/playwright.config.ts and the authenticated context setup in
e2e/config/global-setup.ts, preserving TLS validation for WordPress credentials,
WP_BASE_URL, and storage-state restoration; configure trust only for the local
test CA used by HTTPS endpoints.
In `@e2e/readme.md`:
- Around line 12-18: Update the environment-variable code fence in the README
example to declare the dotenv language while preserving its existing contents.
In `@e2e/test-utils/test.ts`:
- Around line 11-18: Update the requestUtils fixture to read WP_BASE_URL,
WP_USERNAME, and WP_PASSWORD once before calling ExtendedRequestUtils.setup,
validate that all required values are present, and throw a clear configuration
error identifying the missing E2E configuration before fixture setup. Pass the
validated values into the existing setup call.
In `@e2e/tests/upload-interception.spec.ts`:
- Around line 16-25: Guard destructive setup and cleanup in
e2e/tests/upload-interception.spec.ts and e2e/test-utils/test.ts with an
explicit destructive-test opt-in before creating authenticated fixtures or
making destructive API calls. Replace requestUtils.deleteAllMedia() in the
upload-interception beforeEach/afterEach flow with deletion limited to media
created by the current test, while preserving post cleanup through deletePost
and clearing pageId.
- Around line 90-102: Update the modal selection flow after expectNewMediaIsWebp
to wait for the .attachment[data-id="${media.id}"] element to appear before
attempting selection, rather than relying on count(). Also wait for the
Select/Insert button to become enabled before clicking it, while preserving the
existing modal visibility guard and selection behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7f70a65-ac1a-46b6-a515-cfa16f5af2d4
⛔ Files ignored due to path filters (2)
e2e/fixtures/sample.jpgis excluded by!**/*.jpgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.env.example.eslintignore.gitignore.wp-env.jsone2e/config/global-setup.tse2e/playwright.config.tse2e/readme.mde2e/test-utils/index.tse2e/test-utils/media.tse2e/test-utils/requestUtils.tse2e/test-utils/test.tse2e/tests/upload-interception.spec.tspackage.jsontsconfig.json
| use: { | ||
| baseURL: process.env.WP_BASE_URL, | ||
| ignoreHTTPSErrors: true, | ||
| screenshot: 'only-on-failure', | ||
| trace: 'retain-on-failure', | ||
| video: 'retain-on-failure', |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files:\n'
git ls-files | sed -n '1,120p'
printf '\nTarget files:\n'
for f in e2e/playwright.config.ts e2e/config/global-setup.ts; do
if [ -f "$f" ]; then
echo "== $f =="
nl -ba "$f" | sed -n '1,120p'
else
echo "missing $f"
fi
done
printf '\nSearch WP auth / store / restore / baseURL settings:\n'
rg -n "WP_BASE_URL|WP_AUTH_STORAGE|WP_USERNAME|WP_PASSWORD|ignoreHTTPSErrors|storageState|useStorageState|setupRest|global-setup|testDir" e2e || trueRepository: gambitph/Cimo
Length of output: 2407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect package files to understand Playwright version/options semantics references.
printf 'Package files:\n'
git ls-files | rg '(^|/)(package.json|pnpm-lock.yaml|package-lock.json|yarn.lock)$' || true
printf '\nDependency versions for `@playwright/test`:\n'
node - <<'JS'
const fs = require('fs');
for (const name of ['package.json','pnpm-lock.yaml','package-lock.json','yarn.lock']) {
if (!fs.existsSync(name)) continue;
const text = fs.readFileSync(name, 'utf8');
const m = text.match(/@playwright\/test(?:[^#\n]*|[^@]*@[^#\n]*|[^"]*"[^"]*")/);
if (m) console.log(`${name}: ${m[0].split('\n')[0]}`);
}
JSRepository: gambitph/Cimo
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json relevant deps/scripts =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path('package.json')
data=json.loads(p.read_text())
print("scripts:", data.get("scripts"))
print("dependencies:", {k:v for k,v in data.get("dependencies",{}).items() if "playwright" in k.lower()})
print("devDependencies:", {k:v for k,v in data.get("devDependencies",{}).items() if "playwright" in k.lower()})
PY
echo "== target file contents =="
for f in e2e/playwright.config.ts e2e/config/global-setup.ts; do
echo "== $f =="
awk '{printf "%6d %s\n", NR, $0}' "$f"
done
echo "== relevant test utils =="
for f in e2e/test-utils/requestUtils.ts e2e/test-utils/test.ts e2e/tests/upload-interception.spec.ts; do
if [ -f "$f" ]; then
echo "== $f =="
awk '{printf "%6d %s\n", NR, $0}' "$f"
fi
done
echo "== search all relevant identifiers =="
grep -RInE "WP_BASE_URL|WP_AUTH_STORAGE|WP_USERNAME|WP_PASSWORD|ignoreHTTPSErrors|storageState|useStorageState|setupRest|newContext\\(" e2eRepository: gambitph/Cimo
Length of output: 17408
🌐 Web query:
Playwright 1.61 use ignoreHTTPSErrors browser context request context TLS certificate validation
💡 Result:
In Playwright, the ignoreHTTPSErrors option is used to disable TLS certificate validation for network requests [1][2]. This setting applies to both browser navigation and requests initiated via API request contexts [1][3][2]. Browser Contexts and Test Configuration When configuring Playwright, you can set ignoreHTTPSErrors: true in your playwright.config.ts file under the use object [1][4]. This ensures that all browser contexts created during your tests will ignore HTTPS errors (such as those caused by self-signed certificates) during navigation [1][4]. // playwright.config.ts import { defineConfig } from '@playwright/test'; export default defineConfig({ use: { ignoreHTTPSErrors: true, }, }); Request Contexts APIRequestContext objects, which are used for standalone API testing or via page.request/context.request, also support this option [3][2]. When creating an isolated request context using apiRequest.newContext, you can pass the option directly [2][5]: const context = await playwright.request.newContext({ ignoreHTTPSErrors: true }); If you are using a browser context's built-in request object (context.request), the context will inherit the ignoreHTTPSErrors setting from the configuration or the browserContext.newContext call used to create that context [6][7][4]. If you encounter TLS certificate errors in API requests that are not resolved by the global config, explicitly setting this option when creating the request context is the recommended approach [6].
Citations:
- 1: https://playwright.dev/docs/api/class-testoptions
- 2: https://playwright.dev/docs/api/class-apirequestcontext
- 3: https://playwright.dev/docs/api/class-apirequest
- 4: https://playwright.dev/docs/test-use-options
- 5: https://playwright.dev/docs/next/api/class-apirequest
- 6: [Question]: Playwright: apiRequestContext.post: self-signed certificate in certificate chain microsoft/playwright#14440
- 7: [Bug]: Error not thrown when using self-signed certificate with clientCertificates passed to Browser.newContext() and making requests with BrowserContext.request microsoft/playwright#32973
Security Misconfiguration (CWE-295): Improper Certificate Validation
Reachability: Internal
Enable TLS certificate validation for authenticated contexts.
ignoreHTTPSErrors: true disables TLS validation for the Playwright browser context and the authenticated request contexts that send WordPress credentials, accept WP_BASE_URL, and restore authenticated storage state. Remove these flags; trust a local test CA only for HTTPS test endpoints.
📍 Affects 2 files
e2e/playwright.config.ts#L21-L26(this comment)e2e/config/global-setup.ts#L6-L18
🤖 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/playwright.config.ts` around lines 21 - 26, Remove ignoreHTTPSErrors from
the Playwright use configuration in e2e/playwright.config.ts and the
authenticated context setup in e2e/config/global-setup.ts, preserving TLS
validation for WordPress credentials, WP_BASE_URL, and storage-state
restoration; configure trust only for the local test CA used by HTTPS endpoints.
| ``` | ||
| WP_BASE_URL=http://localhost:8889 | ||
| WP_AUTH_STORAGE=wp-auth.json | ||
| WP_USERNAME=admin | ||
| WP_PASSWORD=password | ||
| CIMO_SLUG=Cimo/cimo | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the environment file fence.
The Markdown linter reports MD040. Use dotenv for this environment-variable example.
Proposed fix
-```
+```dotenv
WP_BASE_URL=http://localhost:8889
WP_AUTH_STORAGE=wp-auth.json
WP_USERNAME=admin
WP_PASSWORD=password
CIMO_SLUG=Cimo/cimo</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/readme.md` around lines 12 - 18, Update the environment-variable code
fence in the README example to declare the dotenv language while preserving its
existing contents.
Source: Linters/SAST tools
| requestUtils: async ( {}, use ) => { | ||
| const requestUtils = await ExtendedRequestUtils.setup( { | ||
| baseURL: process.env.WP_BASE_URL, | ||
| user: { | ||
| username: process.env.WP_USERNAME, | ||
| password: process.env.WP_PASSWORD, | ||
| }, | ||
| } ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject incomplete E2E configuration before fixture setup.
These environment variables can be undefined. The fixture then fails during authentication or a later REST request. Read the required values once and throw a clear configuration error.
Proposed fix
+const getRequiredEnv = ( name: string ) => {
+ const value = process.env[ name ]
+ if ( ! value ) {
+ throw new Error( `Missing required E2E environment variable: ${ name }` )
+ }
+ return value
+}
+
const test = base.extend<{
requestUtils: ExtendedRequestUtils;
}>( {
requestUtils: async ( {}, use ) => {
const requestUtils = await ExtendedRequestUtils.setup( {
- baseURL: process.env.WP_BASE_URL,
+ baseURL: getRequiredEnv( 'WP_BASE_URL' ),
user: {
- username: process.env.WP_USERNAME,
- password: process.env.WP_PASSWORD,
+ username: getRequiredEnv( 'WP_USERNAME' ),
+ password: getRequiredEnv( 'WP_PASSWORD' ),
},
} )📝 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.
| requestUtils: async ( {}, use ) => { | |
| const requestUtils = await ExtendedRequestUtils.setup( { | |
| baseURL: process.env.WP_BASE_URL, | |
| user: { | |
| username: process.env.WP_USERNAME, | |
| password: process.env.WP_PASSWORD, | |
| }, | |
| } ) | |
| const getRequiredEnv = ( name: string ) => { | |
| const value = process.env[ name ] | |
| if ( ! value ) { | |
| throw new Error( `Missing required E2E environment variable: ${ name }` ) | |
| } | |
| return value | |
| } | |
| requestUtils: async ( {}, use ) => { | |
| const requestUtils = await ExtendedRequestUtils.setup( { | |
| baseURL: getRequiredEnv( 'WP_BASE_URL' ), | |
| user: { | |
| username: getRequiredEnv( 'WP_USERNAME' ), | |
| password: getRequiredEnv( 'WP_PASSWORD' ), | |
| }, | |
| } ) |
🤖 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/test-utils/test.ts` around lines 11 - 18, Update the requestUtils fixture
to read WP_BASE_URL, WP_USERNAME, and WP_PASSWORD once before calling
ExtendedRequestUtils.setup, validate that all required values are present, and
throw a clear configuration error identifying the missing E2E configuration
before fixture setup. Pass the validated values into the existing setup call.
| test.beforeEach( async ( { requestUtils } ) => { | ||
| await requestUtils.deleteAllMedia() | ||
| } ) | ||
|
|
||
| test.afterEach( async ( { requestUtils } ) => { | ||
| if ( pageId ) { | ||
| await requestUtils.deletePost( pageId, 'pages' ) | ||
| pageId = null | ||
| } | ||
| await requestUtils.deleteAllMedia() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 '\bdeleteAllMedia\s*\(|\bdeletePost\s*\(|WP_BASE_URL|CIMO_E2E' e2e
fd -a -I 'playwright.config.ts' e2e --exec sed -n '1,260p' {}
fd -a -I 'global-setup.ts' e2e --exec sed -n '1,260p' {}Repository: gambitph/Cimo
Length of output: 5911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== request-utils outline =="
ast-grep outline e2e/test-utils/requestUtils.ts || true
echo "== requestUtils relevant sections =="
sed -n '1,260p' e2e/test-utils/requestUtils.ts
echo "== test fixture section =="
sed -n '1,80p' e2e/test-utils/test.ts
echo "== upload interception relevant sections =="
sed -n '1,120p' e2e/tests/upload-interception.spec.ts
echo "== all WP_BASE_URL references =="
rg -n 'WP_BASE_URL|CIMO_E2E|WP_AUTH' .Repository: gambitph/Cimo
Length of output: 8045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in ["e2e/test-utils/requestUtils.ts", "e2e/tests/upload-interception.spec.ts"]:
text = Path(p).read_text()
print(f"\n== {p}: deleteAllMedia definitions/usages ==")
lines = text.splitlines()
for i,l in enumerate(lines, start=1):
if "deleteAllMedia" in l or "deletePost" in l or "requestUtils.setup" in l:
start=max(1,i-3); end=min(len(lines), i+8)
print(f"-- around line {i} --")
for n in range(start,end+1):
print(f"{n:4}: {lines[n-1]}")
PYRepository: gambitph/Cimo
Length of output: 1769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = ["e2e/test-utils/requestUtils.ts", "e2e/tests/upload-interception.spec.ts"]
for p in files:
text = Path(p).read_text()
print(f"== {p} ==")
print("contains deleteAllMedia:", "deleteAllMedia" in text)
print("contains listMedia:", "listMedia" in text)
print("contains deleteMedia:", "deleteMedia" in text or re.search(r'\bdelete\s*\([^)]*\bmedia\b', text, re.I) is not None)
for name in ["deleteAllMedia", "deleteMedia", "deletePost"]:
print(f"count {name}: {text.count(name)}")
config = Path("e2e/playwright.config.ts").read_text()
print("== e2e/playwright.config.ts ==")
print("contains CI env config references:", "CI" in config or "workers" in config or "timeout" in config)
print("contains destructive/prod env guard:", re.search(r'(CI|unsafe|destructive|prod|production|WP_BASE_URL\s*=\s*"https?://(?!localhost|127\.0\.0\.1))', config, re.I) is not None)
PYRepository: gambitph/Cimo
Length of output: 643
Guard destructive cleanup from arbitrary WordPress targets.
upload-interception.spec.ts calls requestUtils.deleteAllMedia() before and after every case, while WP_BASE_URL is used directly by the fixture and global setup without an unsafe-target guard. A mistaken remote endpoint can delete unrelated media on each run.
- Replace broad media deletion with cleanup of media created by the current test where safe.
- Require an explicit destructive-test opt-in before creating authenticated fixtures or performing destructive API calls.
📍 Affects 2 files
e2e/tests/upload-interception.spec.ts#L16-L25(this comment)e2e/test-utils/test.ts#L12-L18
🤖 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/tests/upload-interception.spec.ts` around lines 16 - 25, Guard
destructive setup and cleanup in e2e/tests/upload-interception.spec.ts and
e2e/test-utils/test.ts with an explicit destructive-test opt-in before creating
authenticated fixtures or making destructive API calls. Replace
requestUtils.deleteAllMedia() in the upload-interception beforeEach/afterEach
flow with deletion limited to media created by the current test, while
preserving post cleanup through deletePost and clearing pageId.
The PR check only built the plugin but never executed the e2e suite added in e2e/. This spins up wp-env, builds the plugin, and runs Playwright against the test WordPress instance on every push/PR to master and develop. Co-authored-by: Cursor <cursoragent@cursor.com>
wp-env logs streams and never exits unless --no-watch is passed, which caused the failure-diagnostics step to hang the CI job indefinitely instead of completing. Co-authored-by: Cursor <cursoragent@cursor.com>
- dropFile(): build the File from decoded bytes instead of fetch()-ing a data: URL, since that fetch fails inside the block editor iframe's CSP. - Featured image test: getByLabel(/Settings/i) matched 3 elements (toggle button, sidebar region, close button); target the exact "Settings" toggle button instead. - Media Library modal test: the upload drop zone no longer has the legacy .media-frame-uploader/.uploader-inline/.upload-ui classes visible in this WP version; target the "Upload files" tabpanel by role instead. Co-authored-by: Cursor <cursoragent@cursor.com>
The payload was always undefined because the callback treated the resolved element as the data argument. Also switched to passing a plain byte array instead of a base64 string/data-URL fetch, avoiding both the iframe CSP fetch failure and any string-serialization edge cases. Co-authored-by: Cursor <cursoragent@cursor.com>
…drop target - Media Library modal test: the uploaded attachment only appears as a selectable grid item under the "Media Library" tab, not "Upload files". Switch tabs before selecting, and wait for visibility/enabled state instead of racing with count()/isEnabled() checks. - Featured image test: Cimo's interceptor re-dispatches the converted file onto Gutenberg's actual DropZone element (.components-drop-zone), nested inside .editor-post-featured-image. Dropping on the outer container missed that inner target; drop on the inner zone when present. - Bump the describe-level timeout to 120s since some flows legitimately take over a minute under CI load. Co-authored-by: Cursor <cursoragent@cursor.com>
A single click plus waiting for the Select button to enable was flaky (button stayed disabled). Double-clicking the attachment selects and inserts it in one step, matching the standard WP media modal UX pattern; falls back to the click + Select button flow if the modal is still open afterwards. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…king Diagnostics showed the grid item's <li> renders (and passes visibility checks) before its thumbnail/content is filled in; clicking that early landed on an inert placeholder that never toggled Backbone's selection state, leaving the Select button permanently disabled. Waiting for .attachment-preview to render first fixes it. Also removes the temporary debug logging and restores the full e2e test run in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… button locator This grid's Attachment view only toggles selection via its .check checkbox button — clicking elsewhere on the item doesn't register. Separately, the loose /Select|Insert/i search wasn't scoped to the toolbar, so it ambiguously matched the attachment's own checkbox once its accessible name toggled to "Deselect". Scoping to .media-toolbar fixes that. Also restores the full e2e test run in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Playwright clicks never update wp.media's Backbone selection, so Select stays disabled after a modal drop. Select the uploaded attachment via the media frame API instead. Also skip already-active sidebar tabs so the featured-image test does not hang waiting for a stable click. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
@wordpress/enve2e scaffolding (mirrors Interactions): config, global setup, fixtures, and helpers.build:e2e/test/test:debugscripts,.wp-env.json, and.env.example.Test plan
cp .env.example .env(confirmCIMO_SLUGmatches checkout folder)npx @wordpress/env start(Docker required)npm run build:e2enpm run test— all 5 upload interception specs passnpm run test:debugand spot-check one failing surface if any flakeMade with Cursor
Summary by CodeRabbit
New Features
Tests