feat(testing): Drive the real app with agent-browser#2660
Merged
Conversation
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
.claude/skills/screenshot-dev-app/SKILL.md:3
**`allowed-tools` pattern doesn't match `pnpm app:cdp`**
The pattern `Bash(pnpm app:cdp:*)` requires the command to begin with the literal string `pnpm app:cdp:` (colon-then-wildcard), so it covers subcommands like `pnpm app:cdp:something` but not the bare `pnpm app:cdp` invocation shown throughout the skill body. An agent following this skill will be blocked from running the preflight step. The same pattern also appears in `test-electron-app/SKILL.md`. Dropping the trailing colon — `Bash(pnpm app:cdp*)` — matches the bare command and any future variants.
### Issue 2 of 2
scripts/electron-cdp.mjs:25-30
**Spawn error other than ENOENT silently falls through**
The ENOENT guard only fires when the `agent-browser` binary can't be found. Any other `spawnSync` error (e.g. permission denied, `EACCES`) leaves `version.error` set but non-null with a different `.code`, so the check passes and execution continues to `version.stdout.trim()`. With `encoding: "utf8"` set, `stdout` will be an empty string rather than null, so it won't crash — but the success banner will read `✓ agent-browser ` with no version, and the script will proceed to the CDP check against an unusable binary. Consider broadening the guard to `version.error` (any error) to give a clear failure message.
Reviews (1): Last reviewed commit: "restore screenshot skill via agent-brows..." | Re-trigger Greptile |
759b017 to
195e1c5
Compare
adamleithp
approved these changes
Jun 13, 2026
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.
Problem
We want to verify and screenshot changes in the real running Electron app (live tRPC, workspace-server, real data), not just in unit or E2E specs.
Changes
:9222: snapshot the accessibility tree, click/type, and screenshot the live UItest-electron-appskill documenting the connect → snapshot → screenshot loop plus repo specifics (port, auth/data, dev-alongside-prod)pnpm app:cdppreflight (scripts/electron-cdp.mjs): validate the port, verify the app is reachable, then connect--remote-debugging-port=9222--color-scheme dark: that global flag forces a 1280x720 viewport that blanks the Electron window and sticks in the agent-browser daemon until it restartsHow did you test this?
Manually by using itself
Automatic notifications