Guard Playwright web security override and stub Draco decoders#166
Conversation
WalkthroughThe PR establishes infrastructure for locally serving Draco decoder assets in E2E tests through Playwright route interception. It introduces a fixtures module that registers Draco routes before test execution, updates existing test files to import from fixtures, creates a Draco utility for asset routing, enhances playwright configuration with GPU/WebGL flags and conditional security controls, and documents the testing setup in the README. Changes
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant Fixtures as fixtures.ts
participant Draco as draco.ts
participant Browser as Browser Context
participant Assets as Local draco3d<br/>Package
Test->>Fixtures: Load test fixture
Fixtures->>Draco: Call registerDracoRoutes(context)
Draco->>Assets: Locate draco3d package files
Draco->>Browser: Register route handlers<br/>(WASM, JS, Binary)
Browser-->>Draco: Routes installed
Draco-->>Fixtures: Complete
Fixtures-->>Test: Test ready
Test->>Browser: Navigate & execute
Browser->>Browser: Intercept Draco asset requests
Browser->>Assets: Load from local package
Assets-->>Browser: Return assets
Browser-->>Test: Assets loaded successfully
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve new logic for route interception and fixture setup, distributed import updates across multiple test files, and configuration enhancements. The Draco utility introduces file reading and route registration logic with WeakSet tracking, while fixtures add context/page hooks. These are moderately complex but follow clear patterns without intricate business logic. Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fulfill = (route: Route, body: string | Buffer, contentType: string) => | ||
| route.fulfill({ status: 200, body, contentType, headers: { 'cache-control': 'public, max-age=86400' } }); | ||
|
|
||
| export async function registerDracoRoutes(context: BrowserContext): Promise<void> { | ||
| if (patchedContexts.has(context)) { | ||
| return; | ||
| } | ||
|
|
||
| patchedContexts.add(context); | ||
|
|
||
| await Promise.all([ | ||
| context.route(`${decoderBaseUrl}draco_wasm_wrapper.js`, (route) => | ||
| fulfill(route, wasmWrapperSource, 'application/javascript'), | ||
| ), | ||
| context.route(`${decoderBaseUrl}draco_decoder.js`, (route) => | ||
| fulfill(route, decoderSource, 'application/javascript'), | ||
| ), | ||
| context.route(`${decoderBaseUrl}draco_decoder.wasm`, (route) => | ||
| fulfill(route, wasmBinary, 'application/wasm'), |
There was a problem hiding this comment.
The new fixture fulfills https://www.gstatic.com/draco/v1/decoders/* routes with static assets but the helper only sets cache-control. After removing --disable-web-security, those responses are subject to normal CORS and draco_wasm_wrapper.js’s internal fetch for draco_decoder.wasm will fail because the stubbed response lacks Access-Control-Allow-Origin. The real gstatic endpoints include permissive CORS headers. Without mirroring them, decoder initialization will still break under secure browser settings. Consider adding access-control-allow-origin: * (and any other required headers) in the fulfilled responses.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
frontend/README.md(1 hunks)frontend/tests/e2e/cache.spec.ts(1 hunks)frontend/tests/e2e/fixtures.ts(1 hunks)frontend/tests/e2e/perf.p90.spec.ts(1 hunks)frontend/tests/e2e/perf/perf-budget.spec.ts(2 hunks)frontend/tests/e2e/utils/draco.ts(1 hunks)playwright.config.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/tests/e2e/perf/perf-budget.spec.ts (2)
frontend/tests/e2e/fixtures.ts (1)
registerDracoRoutes(16-16)frontend/tests/e2e/utils/draco.ts (1)
registerDracoRoutes(31-49)
frontend/tests/e2e/fixtures.ts (1)
frontend/tests/e2e/utils/draco.ts (1)
registerDracoRoutes(31-49)
frontend/tests/e2e/utils/draco.ts (2)
frontend/tools/perf/run-perf-budget.js (2)
frontendRoot(10-10)path(3-3)frontend/tests/e2e/fixtures.ts (1)
registerDracoRoutes(16-16)
🔍 Remote MCP
The Linear search results are returning general Linear product documentation rather than project-specific documentation. Let me try a different approach with web search to find more specific information about the PR and related context:Based on my research, I now have sufficient context to provide a comprehensive summary of the PR. Let me compile the findings:
Summary of Additional Context for PR #166 Review
Key Technical Context
Draco 3D Decoder Overview:
Draco is a library for compressing and decompressing 3D geometric meshes and point clouds, intended to improve the storage and transmission of 3D graphics. The decoder consists of draco_decoder.wasm (WebAssembly decoder) and draco_wasm_wrapper.js (JavaScript wrapper for the WASM decoder).
Web Security Implications:
The --disable-web-security flag is commonly used in Playwright to disable CORS restrictions during testing. However, disabling web security can have potential security implications, and should be used with caution, always re-enabled when running tests or accessing websites that require secure browsing.
PR Strategy for Addressing Web Security:
Rather than permanently disabling web security, this PR implements a more secure approach by:
-
Conditional Security Override: The
--disable-web-securityflag is only enabled when explicitly requested via environment variable configuration. This guards the flag behind thePLAYWRIGHT_ALLOW_INSECUREenvironment variable, ensuring it's off by default. -
Local Decoder Service Alternative: The PR introduces Playwright fixtures that intercept and serve Draco decoder assets locally through the test context's route handlers, avoiding the need for external CORS-restricted WASM fetches.
Implementation Review Points
Fixture Architecture:
- New
fixtures.tsestablishes a reusable fixture extending Playwright's base test - Uses
beforeEachhooks to register Draco routes on both test context and page context - Ensures routes are registered before any tests run, following Playwright best practices
Route Interception Mechanism:
- The
registerDracoRoutes()utility uses Playwright'sbrowserContext.route()API to intercept three key assets:draco_wasm_wrapper.js(served asapplication/javascript)draco_decoder.js(served asapplication/javascript)draco_decoder.wasm(served asapplication/wasm)
- WeakSet tracks patched contexts to prevent duplicate routing setup
Configuration Management:
- Playwright config consolidates launch arguments into a dedicated
launchArgsarray - GPU/WebGL flags and conditional web-security override are centralized for maintainability
[::web_search::], [::DeepWiki_read_wiki_structure::]
🔇 Additional comments (8)
frontend/tests/e2e/perf.p90.spec.ts (1)
1-1: LGTM: Clean migration to fixtures module.The import change correctly switches to the new fixtures module while preserving all test behavior.
frontend/tests/e2e/cache.spec.ts (1)
1-1: LGTM: Import successfully migrated to fixtures.The import change correctly adopts the new fixtures module while preserving test logic.
playwright.config.ts (1)
6-15: LGTM: Well-guarded security override with appropriate GPU flags.The conditional logic correctly restricts
--disable-web-securityto explicit opt-in viaPLAYWRIGHT_ALLOW_INSECURE=1. The GPU/WebGL flags are appropriate for headless 3D rendering scenarios. This implementation follows security best practices by defaulting to secure mode.frontend/README.md (1)
28-35: LGTM: Clear and accurate testing documentation.The documentation effectively explains the E2E test setup, Draco asset interception strategy, and the
PLAYWRIGHT_ALLOW_INSECUREoverride. The emphasis on "off by default" reinforces the security-conscious design.frontend/tests/e2e/perf/perf-budget.spec.ts (1)
114-115: LGTM: Correct manual registration for programmatically-created context.The explicit
registerDracoRoutes(context)call is necessary here becausecollectNavigationMetricscreates a new context viabrowser.newContext()that falls outside the fixtures' automatic registration. This ensures Draco decoder assets are intercepted for performance measurement runs.frontend/tests/e2e/utils/draco.ts (3)
9-15: LGTM: Clear error handling for missing dependencies.The try-catch with an actionable error message provides fast feedback if
draco3dis not installed. Failing at module load time (rather than during test execution) helps developers quickly identify missing dependencies.
26-49: LGTM: Elegant route interception with proper safeguards.The implementation correctly:
- Uses a WeakSet to prevent duplicate registrations while allowing automatic cleanup
- Registers routes in parallel for efficiency
- Sets appropriate content-type and cache-control headers
- Returns early if context is already patched
The fulfill helper provides consistent response handling across all three decoder assets.
17-17: No action required; original review comment contains incorrect premise.The hardcoded URL in the test file matches the official Draco decoder documentation. The test file (
frontend/tests/e2e/utils/draco.ts) is E2E test infrastructure that intercepts requests tohttps://www.gstatic.com/draco/v1/decoders/, serving localdraco3dpackage files instead. This URL is documented indocs/GLB_ASSET_STANDARD.mdas the standard decoder path.Draco decoders are not used in application code itself—only in test infrastructure. The route interception is correctly configured and will function as intended if any E2E test loads Draco-compressed models.
Likely an incorrect or invalid review comment.
| const test = base.extend({ | ||
| context: async ({ context }, use) => { | ||
| await registerDracoRoutes(context); | ||
| await use(context); | ||
| }, | ||
| page: async ({ page }, use) => { | ||
| await registerDracoRoutes(page.context()); | ||
| await use(page); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying by removing the page fixture.
The page fixture registers Draco routes on page.context(), but the context fixture already handles this. Since all pages derive from a context and route registration is context-scoped, the page fixture is redundant. The WeakSet in draco.ts makes duplicate registration safe, but removing the page fixture would simplify the code.
Apply this diff to simplify:
const test = base.extend({
context: async ({ context }, use) => {
await registerDracoRoutes(context);
await use(context);
},
- page: async ({ page }, use) => {
- await registerDracoRoutes(page.context());
- await use(page);
- },
});📝 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.
| const test = base.extend({ | |
| context: async ({ context }, use) => { | |
| await registerDracoRoutes(context); | |
| await use(context); | |
| }, | |
| page: async ({ page }, use) => { | |
| await registerDracoRoutes(page.context()); | |
| await use(page); | |
| }, | |
| }); | |
| const test = base.extend({ | |
| context: async ({ context }, use) => { | |
| await registerDracoRoutes(context); | |
| await use(context); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In frontend/tests/e2e/fixtures.ts around lines 5 to 14, remove the redundant
page fixture that registers Draco routes on page.context() because the context
fixture already registers those routes; delete the entire "page: async ({ page
}, use) => { await registerDracoRoutes(page.context()); await use(page); }"
block from the base.extend call and keep only the context fixture, leaving the
rest of the file and exports untouched so tests continue to import the same test
fixture.
|
Note Docstrings generation - SUCCESS |
…playwright` Docstrings generation was requested by @shayancoin. * #166 (comment) The following files were modified: * `frontend/tests/e2e/utils/draco.ts`
…playwright` (#298) Docstrings generation was requested by @shayancoin. * #166 (comment) The following files were modified: * `frontend/tests/e2e/utils/draco.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Guard Playwright security override * 📝 Add docstrings to `codex/remove-disable-web-security-argument-from-playwright` (#298) Docstrings generation was requested by @shayancoin. * #166 (comment) The following files were modified: * `frontend/tests/e2e/utils/draco.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
--disable-web-securityis only enabled when explicitly requestedPLAYWRIGHT_ALLOW_INSECUREoverride and E2E testing workflowTesting
https://chatgpt.com/codex/tasks/task_e_68f28d961c5c833088eb9fe86e11976a
Summary by CodeRabbit
Documentation
Chores