Add AI-generated E2E testing feature - #3681
Conversation
|
@BugBot run |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental end-to-end testing feature, allowing users to record user flows in the preview iframe, generate Playwright tests via chat, and run them within a new 'Tests' panel. For database-connected Neon apps, test runs are isolated using ephemeral copy-on-write branches. Feedback focuses on improving the robustness of this system: propagating and checking the abort signal during database setup and server polling, only clearing the test branch ID in the database upon successful deletion to avoid orphaned branches, re-activating the test recorder upon iframe navigation/reload, and safely handling empty paths in test path normalization to prevent directory-write crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a well-structured, large feature PR (~5900 lines across 47 files) that adds AI-generated E2E testing with a Tests panel, Playwright integration, record-a-test flow, and Neon branch isolation for database-connected apps. What's done well:
Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments were addressed with code changes or a reasoned engineering decision, lint/typecheck pass, and changes are pushed — held at 4 only because the cancellation and isolation paths warrant a manual smoke-test before merge. Unresolved Threads
No unresolved threads Resolved Threads
Product Principle Suggestions
🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Large, well-structured feature PR that adds AI-generated E2E testing with a thoughtful UX (result taxonomy, database isolation, lazy Playwright bootstrap, record-a-test). The security posture for screenshot reading (symlink resolution + containment check), command injection prevention (array args, no shell on Unix), and path normalization is solid. Two mechanical bugs need fixing before merge. Issues Summary
HIGH #1 — HIGH #2 — Extension mismatch loses the test card for non- 🟢 Low Priority Notes (2 items)
What's done well:
Generated by Dyadbot persona-based code review |
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a well-architected, security-conscious feature PR that adds AI-generated E2E testing with appropriate defense-in-depth across all layers. The core security decisions are sound:
The DB isolation design (Neon branches + Supabase test users with crash-safe reconciliation) is thoughtful, and the test coverage across 8 test files is solid. Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a well-designed, large feature PR that adds AI-generated E2E testing with good security practices (path traversal protection, CLI flag injection prevention, symlink resolution, SQL injection guards), thorough test coverage (unit tests for all critical utilities and the isolation service), and careful UX design (infra-vs-assertion taxonomy, isolated test data, crash reconciliation). The architecture follows established Dyad patterns for IPC contracts, jotai atoms, and panel components. Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
- tighten test isolation warnings and setup safety - fix Playwright bootstrap/run/report edge cases - prevent stale test output and clean up runtime state
`playwright test` has no `--base-url` option, so the run aborted with "unknown option '--base-url'". The base URL is already supplied via the DYAD_TEST_BASE_URL env var read by the generated playwright.config.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover the three highest-value flows of the Tests panel + <dyad-generate-test> feature, driven with the fake LLM (no real Playwright run): - Generate a test: renders a chat card, writes the spec under tests/, and auto-discovers it in the Tests panel when the turn ends. - Per-app opt-in gate: off by default, persists across a panel remount, and can be disabled. - Data-safety warning scales to the backend: strong warning with no isolation, calm warning when tests run against an isolated Neon branch copy. Also teach the PreviewPanel page object about the overflow-hosted `tests` mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔍 Dyadbot Code Review SummaryRecommendation: human-review PR Review: #3681 — Add AI-generated E2E testing featureAuthor: azizmejri1 Verdict✅ YES — Ready to merge Recommendation: This is a well-engineered feature PR with impressive attention to security, data safety, and edge cases. The isolation strategy (Neon branch copy-on-write + Supabase ephemeral test users with RLS) ensures production data is never at risk. Security boundaries — path traversal defense, SQL injection prevention, command injection hardening, and IPC contract validation via Zod — are thorough and layered. No issues warrant blocking merge. Review Personas Applied1. Correctness & Debugging ExpertRun serialization ( Crash reconciliation ( Process tree-killing ( 2. Code Health ExpertIPC contracts: All test-related IPC channels use Zod schemas ( State management: Jotai atoms for per-app test state and TanStack Query for IPC-backed fetching follow established Dyad patterns. The Migration: 3. UX WizardOpt-in gate: Output streaming: The Deep-linking: IssuesNo HIGH-severity issues found. LOW notes (non-blocking)
Security Assessment
Reviewed with three personas: Correctness & Debugging Expert, Code Health Expert, UX Wizard. |
|
@BugBot run |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🔍 Dyadbot Code Review SummaryPR Review: #3681 — Add AI-generated E2E testing featureVerdict: ✅ YES — Ready to merge Recommendation: SummaryThis PR adds a full-stack AI-generated E2E testing feature (+9232/−20 across 65 files): Playwright-based test execution with a Tests panel UI, copy-on-write Neon branch isolation, throwaway Supabase test-user isolation with RLS checks, crash-recovery reconciliation, prompt guidance for test generation in both build and local-agent modes, and comprehensive unit/E2E test coverage. The code quality is high overall. Security-sensitive surfaces (screenshot handler, SQL construction, Issues
M1 —
|
🎭 Playwright Test Results❌ Some tests failed
Summary: 229 passed, 22 failed, 6 skipped Failed Tests🍎 macOSShow all 22 failures
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/capacitor.spec.ts \
e2e-tests/cloud_sandbox.spec.ts \
e2e-tests/context_window.spec.ts \
e2e-tests/dump_messages.spec.ts \
e2e-tests/engine.spec.ts \
e2e-tests/local_agent_auto.spec.ts \
e2e-tests/mention_app.spec.ts \
e2e-tests/mention_files.spec.ts \
e2e-tests/prompt_library.spec.ts \
e2e-tests/security_review.spec.ts \
e2e-tests/smart_context_balanced.spec.ts \
e2e-tests/smart_context_deep.spec.ts \
e2e-tests/thinking_budget.spec.ts \
e2e-tests/turbo_edits_v2.spec.ts |
- Gate AGENT_TEST_WRITING_GUIDANCE in local-agent prompts on testingEnabled, mirroring the build-mode gating so non-testing apps don't carry the guidance in every prompt. - Enforce app.testingEnabled in the tests:run IPC handler before preparing isolation or bootstrapping Playwright, so a stale/buggy renderer caller can't run tests against real data. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scaffold .gitignore now includes Playwright test artifacts (/test-results/, /playwright-report/, /playwright/.cache/, .last-run.json) for the AI-generated E2E testing feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@BugBot run |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 31 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e-tests/snapshots/context_window.spec.ts_context-window-2.txt">
<violation number="1" location="e2e-tests/snapshots/context_window.spec.ts_context-window-2.txt:36">
P3: The `.gitignore` snapshot includes `/playwright/.cache/`, but this is not a standard Playwright artifact path. Playwright stores browser binaries at the OS level (`~/.cache/ms-playwright`), not in a project-level `playwright/.cache/` directory. The official Playwright .gitignore only recommends `/test-results/` and `playwright-report`. This entry in the AI context window snapshot could lead the AI to generate non-standard `.gitignore` patterns. Consider removing `/playwright/.cache/` to align with Playwright conventions.</violation>
</file>
<file name="e2e-tests/snapshots/context_window.spec.ts_context-window-1.txt">
<violation number="1" location="e2e-tests/snapshots/context_window.spec.ts_context-window-1.txt:37">
P3: The snapshot includes `.last-run.json` as a Playwright test artifact to ignore, but `.last-run.json` is not a standard Playwright output file — Playwright uses `.last-run.json` internally via the `@playwright/test` runner configuration, but it's stored under the `test-results/` directory by default, not at the repo root. Including it as a standalone entry in the AI prompt's gitignore could mislead the LLM into thinking it's a root-level Playwright artifact.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # Playwright test artifacts | ||
| /test-results/ | ||
| /playwright-report/ | ||
| /playwright/.cache/ |
There was a problem hiding this comment.
P3: The .gitignore snapshot includes /playwright/.cache/, but this is not a standard Playwright artifact path. Playwright stores browser binaries at the OS level (~/.cache/ms-playwright), not in a project-level playwright/.cache/ directory. The official Playwright .gitignore only recommends /test-results/ and playwright-report. This entry in the AI context window snapshot could lead the AI to generate non-standard .gitignore patterns. Consider removing /playwright/.cache/ to align with Playwright conventions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At e2e-tests/snapshots/context_window.spec.ts_context-window-2.txt, line 36:
<comment>The `.gitignore` snapshot includes `/playwright/.cache/`, but this is not a standard Playwright artifact path. Playwright stores browser binaries at the OS level (`~/.cache/ms-playwright`), not in a project-level `playwright/.cache/` directory. The official Playwright .gitignore only recommends `/test-results/` and `playwright-report`. This entry in the AI context window snapshot could lead the AI to generate non-standard `.gitignore` patterns. Consider removing `/playwright/.cache/` to align with Playwright conventions.</comment>
<file context>
@@ -29,6 +29,12 @@ dist-ssr
+# Playwright test artifacts
+/test-results/
+/playwright-report/
+/playwright/.cache/
+.last-run.json
.dyad/
</file context>
| /test-results/ | ||
| /playwright-report/ | ||
| /playwright/.cache/ | ||
| .last-run.json |
There was a problem hiding this comment.
P3: The snapshot includes .last-run.json as a Playwright test artifact to ignore, but .last-run.json is not a standard Playwright output file — Playwright uses .last-run.json internally via the @playwright/test runner configuration, but it's stored under the test-results/ directory by default, not at the repo root. Including it as a standalone entry in the AI prompt's gitignore could mislead the LLM into thinking it's a root-level Playwright artifact.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At e2e-tests/snapshots/context_window.spec.ts_context-window-1.txt, line 37:
<comment>The snapshot includes `.last-run.json` as a Playwright test artifact to ignore, but `.last-run.json` is not a standard Playwright output file — Playwright uses `.last-run.json` internally via the `@playwright/test` runner configuration, but it's stored under the `test-results/` directory by default, not at the repo root. Including it as a standalone entry in the AI prompt's gitignore could mislead the LLM into thinking it's a root-level Playwright artifact.</comment>
<file context>
@@ -29,6 +29,12 @@ dist-ssr
+/test-results/
+/playwright-report/
+/playwright/.cache/
+.last-run.json
.dyad/
</file context>
| let run; | ||
| try { | ||
| run = await spawnStreaming({ | ||
| command: "npx", |
There was a problem hiding this comment.
🟡 MEDIUM
npx resolves Playwright via remote fallback instead of local binary
The test runner invokes npx playwright test which, if the local node_modules/.bin resolution fails for any reason, falls back to downloading the playwright package from the npm registry. While ensurePlaywrightBootstrap installs @playwright/test beforehand, and the user's app is already trusted (it runs a dev server executing arbitrary code), resolving the binary directly from node_modules/.bin would eliminate the remote-fetch fallback entirely as a defense-in-depth measure.
💡 Suggestion: Consider resolving the Playwright CLI path directly via require.resolve('@playwright/test/cli', { paths: [appPath] }) or path.join(appPath, 'node_modules', '.bin', 'playwright') instead of going through npx.
| } | ||
|
|
||
| function projectUrlFor(ref: string): string { | ||
| return `https://${ref}.supabase.co`; |
There was a problem hiding this comment.
🟡 MEDIUM
projectUrlFor constructs URL from DB value without format validation
The function projectUrlFor(ref) constructs https://${ref}.supabase.co using the supabaseProjectId column value directly. While this column is populated from the Supabase integration flow, there is no validation that the value is a safe URL hostname component. A corrupted or unexpected value (containing slashes, colons, or special characters) could produce a malformed URL pointing to an unintended host. The deleteUserBestEffort function validates userId with UUID_RE, but the host portion has no equivalent guard.
💡 Suggestion: Add a simple alphanumeric regex check (e.g. /^[a-z]{20}$/ or a more permissive Supabase ref pattern) on the ref parameter before constructing the URL.
| // later try to re-delete a branch that's already gone. If the delete | ||
| // failed, leave the column set so startup reconciliation retries it. | ||
| if (deleted) { | ||
| await db |
There was a problem hiding this comment.
🟡 MEDIUM
db.update failure after branch deletion leaves stale neonTestBranchId column
When Neon Auth provisioning fails (line 222), the code deletes the branch (line 223) then tries to clear neonTestBranchId (lines 228-230). If db.update fails (e.g. transient SQLite lock), the branch is deleted on Neon but the column still points to it. On next startup, reconciliation self-heals by trying to delete the non-existent branch (404 is treated as success) then clearing the column, but the error thrown at line 233 does not indicate that the branch was already cleaned up, which could confuse debugging.
💡 Suggestion: Wrap the db.update in a try/catch that logs a warning rather than letting it throw, since the branch is already deleted and the stale column self-heals at startup.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a large, well-architected PR (+8992/-38 across 92 files) that adds a complete AI-generated E2E testing feature: a Tests panel with opt-in gate, Security highlights: The screenshot IPC handler ( Architecture highlights: Test run lifecycle management correctly chains runs via Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 250 passed, 1 failed, 1 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/edit_code.spec.ts
|
The "Fix" button handed a failing test to the AI as text only. The failure screenshot was captured and shown in the panel but never sent to the model, so the agent couldn't see the real UI state at the point of failure. (Pointing the agent at the on-disk PNG doesn't help either: its read_file tool decodes files as UTF-8.) askAiToFix now fetches the screenshot via getTestScreenshot and passes it as a chat-context image attachment, which is converted to a model image part server-side. Falls back to text-only if the screenshot is missing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@BugBot run |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| */ | ||
| async function cleanUpRowsOwnedBy({ | ||
| projectId, | ||
| organizationSlug, |
There was a problem hiding this comment.
🟡 MEDIUM
SQL cleanup uses string interpolation inside DO block despite format()
In cleanUpRowsOwnedBy, the SQL DO block passes table, column, and userId as JS template-literal interpolations inside format() arguments: format('DELETE FROM public.%I WHERE %I = %L', '${table}', '${column}', '${userId}'). While SAFE_IDENT_RE and UUID_RE regex guards are correct and load-bearing, this pattern is fragile to future regex relaxations. A future maintainer relaxing SAFE_IDENT_RE to allow quotes or dollar signs would silently open a SQL injection through the dollar-quote boundary or the format() arguments. The inline comments document this, but a parameterized approach would be more robust.
💡 Suggestion: Consider a two-step approach: discover columns with the current query, then delete via a parameterized SQL function or at minimum add a runtime assertion that rejects single-quotes and dollar signs in the values before interpolation, separate from the regex.
| try { | ||
| // checkRls and createTempTestUser each make network requests that can take | ||
| // several seconds; honor a Stop pressed in between them so cancellation | ||
| // takes effect promptly instead of only after they complete. |
There was a problem hiding this comment.
🟡 MEDIUM
restartAppInPlace does not verify app restarted with correct branch
During teardown, after restoreEnvFile + restartAppInPlace, the code does not call waitForServerReady or otherwise verify the restarted dev server is reading the restored real env. In setup, waitForServerReady is called after the restart. If the teardown restart partially fails (process starts but reads cached env), the user could unknowingly interact with the test branch database after the run. This is low-probability since env changes are picked up on restart, but the asymmetry between setup (which waits) and teardown (which does not) is worth noting.
💡 Suggestion: Consider calling waitForServerReady after the teardown restart, or at minimum logging a warning if the proxy URL doesn't come back within a short timeout.
| // under `tests/`, ending in a spec extension, with no traversal or leading | ||
| // dash. This stops a compromised renderer from passing a flag-like value | ||
| // (e.g. `--config=…`) that Playwright would interpret as a CLI option. The | ||
| // allowed characters must cover everything the listing glob can surface |
There was a problem hiding this comment.
🟡 MEDIUM
TEST_FILE_PATTERN allows NUL bytes in path via regex gap
The TEST_FILE_PATTERN regex character class [^\\:\x00-\x1f] correctly blocks ASCII control characters including NUL, but does not restrict non-ASCII characters that could be problematic in certain filesystem encodings or Unicode normalization scenarios. While path.posix.normalize handles most cases and Playwright ultimately opens the file (not a shell), the pattern is more permissive than the glob that discovers test files. This is defense-in-depth rather than an active vulnerability.
💡 Suggestion: Consider whether restricting to printable ASCII and a curated set of Unicode categories would be appropriate, or add a comment documenting the intentional permissiveness for non-ASCII filenames.
| // All test activity lives behind the opt-in gate; don't register the IPC | ||
| // listener (or accumulate output) for apps that haven't enabled testing. | ||
| if (!testingEnabled) return; | ||
| const unsubscribe = ipc.events.tests.onOutput((payload) => { |
There was a problem hiding this comment.
🟡 MEDIUM
IPC output listener registered without appId filter
The tests:output IPC event listener accumulates output chunks from ALL apps into the pendingOutputRef buffer, not just the currently selected app. While per-appId atom writes and MAX_OUTPUT_LENGTH bound the impact, this means output from background test runs on other apps accumulates in renderer memory even when that app is not selected and will never be viewed. For single-app usage this is a non-issue, but in multi-app workflows it represents unnecessary memory churn.
💡 Suggestion: Consider filtering the listener callback to only buffer chunks for the currently selected appId, or documenting that cross-app output accumulation is intentional for preserving output when switching apps.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a well-engineered, large feature PR that adds a complete AI-generated E2E testing pipeline to Dyad. The architecture is sound: database isolation for Neon (copy-on-write branches) and Supabase (throwaway RLS-scoped users), crash recovery via startup reconciliation, thorough input validation, and a carefully gated opt-in UX. The code demonstrates strong security awareness throughout -- path traversal protections, symlink resolution, command injection guards, SQL injection defenses, and proper IPC boundary enforcement. Issues Summary
DetailsSQL cleanup uses string interpolation inside DO block despite format() (MEDIUM) In restartAppInPlace does not verify app restarted with correct branch (MEDIUM) In TEST_FILE_PATTERN allows NUL bytes in path via regex gap (MEDIUM) The IPC output listener registered without appId filter (MEDIUM) In 🟢 Low Priority Notes (5 items)
Security Review SummaryThe security posture of this PR is strong:
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results✅ All tests passed!
Total: 251 tests passed (3 flaky) (6 skipped)
|
| - Otherwise, define a shared test user and create it by driving the app's OWN signup flow (so the user can really authenticate). If the flow needs a login and the app has no signup flow yet, build one (or an equivalent way to create a user) first. Say so clearly if you add it. | ||
| - Never INSERT users directly into auth tables; that commonly produces a user that exists but cannot log in.`; | ||
|
|
||
| /** Build-mode test-writing guidance: emit the spec via a `<dyad-generate-test>` tag. */ |
There was a problem hiding this comment.
IMO, I think we can just have build mode not support generating tests, but I'm OK with deferring this in a future PR.
I think the core loop of automatically writing + running tests + fixing tests is really the main benefit of tests and you lose that with build mode.
If the cost of keeping test mode in build mode is cheap, then I think it's OK
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
awesome job @azizmejri1 - I think there's some UX improvements we can make (e.g. making it clear why Generate test button is disabled, or by sending it to the prompt queue), but this is a great starting point. merging
- left a comment around build mode
|
and thanks for the screenshots! |
|
@wwwillchen Awesome. Thanks for reviewing this so fast! |
🎭 Playwright Test Results❌ Some tests failed
Summary: 774 passed, 4 failed, 6 flaky, 260 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/package_manager.spec.ts
|
2 participants
HIghlight of the changes made after the last time you saw this :
Interface of the testing panel before enabling testing (In this case we're using a Neon database) :
When fixing a test we attach the screenshot now + giving the agent a nudge to read error-context.md file :
