Add pr-screenshots skill for AI-assisted screenshot capture in PRs#1918
Add pr-screenshots skill for AI-assisted screenshot capture in PRs#1918amikofalvy wants to merge 1 commit intomainfrom
Conversation
Adds a new skill that teaches AI coding agents (Cursor, Codex, Claude Code) to capture, redact, annotate, and embed screenshots in pull requests. Includes: - Playwright-based capture script with sensitive data masking - Sharp-based annotation script (labels, borders, side-by-side stitching) - Pre-upload validation script that blocks uploads with leaked secrets - Affected-routes mapping (component files → UI routes) - PR body markdown templates Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
7 Key Findings | Risk: Low
This PR adds a well-structured new skill for AI-assisted screenshot capture. The core implementation is solid, but there are several opportunities to improve error handling and documentation accuracy.
🟠⚠️ Major (3) 🟠⚠️
🟠 1) SKILL.md, capture.ts, .gitignore Path prefix inconsistency (.cursor vs .agents)
Issue: Documentation and comments throughout the skill reference .cursor/skills/pr-screenshots/ instead of the canonical .agents/skills/pr-screenshots/ path. While this works due to symlinks, it creates confusion and fragility.
Why: Users on non-Cursor harnesses (Claude Code, Codex) see Cursor-specific paths. If the symlink structure changes, documented paths break. The canonical source of truth per AGENTS.md is .agents/skills/, not .cursor/skills/.
Fix: Replace all occurrences of .cursor/skills/pr-screenshots/ with .agents/skills/pr-screenshots/ in SKILL.md (lines 50, 56, 62, 89, 92, 111, 150, 154, 158) and capture.ts header comments (lines 8, 14).
Refs:
- AGENTS.md — documents the symlink convention
- SKILL.md:50
// Findings posted as inline comments:
- 🟠 Major:
capture.ts:152Missing viewport input validation - 🟠 Major:
capture.ts:199-204Catch block swallows all errors, not just timeouts - 🟠 Major:
capture.ts:109-115Missing error handling for server startup
🟡 Minor (4) 🟡
// Findings posted as inline comments:
- 🟡 Minor:
annotate.ts:50SVG attribute not escaped (defense-in-depth) - 🟡 Minor:
SKILL.md:120Documentation lists "Email addresses" but script doesn't check - 🟡 Minor:
SKILL.md:203Incorrect dependency source for playwright - 🟡 Minor:
.gitignore:114Comment references non-canonical path
💭 Consider (3) 💭
💭 1) annotate.ts:106 SVG injection in stitchImages (hardcoded colors)
Issue: Same SVG escaping pattern as the inline comment on line 50, but the colors array is hardcoded so not currently exploitable.
Fix: Apply escapeXml(colors[i]) for defense-in-depth if colors are ever parameterized.
💭 2) annotate.ts:40 Add file existence check for input images
Issue: sharp(inputPath).metadata() throws cryptic errors for missing/corrupt files.
Fix: Add if (!fs.existsSync(inputPath)) throw new Error(\Input file not found: ${inputPath}`);`
💭 3) validate-sensitive.ts:67 Wrap main() in try-catch
Issue: Unlike the other scripts, this one calls main() directly without .catch(), so file read errors produce raw stack traces.
Fix: Wrap in try { main(); } catch (err) { console.error('Validation failed:', ...); process.exit(1); }
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-designed skill that follows established patterns. The major findings are all about improving error handling and documentation consistency — they don't block functionality but would improve the developer experience. The path prefix inconsistency is the most widespread issue and should be addressed for maintainability. The error handling improvements in capture.ts would help users debug common failures. Nice work on the sensitive data masking approach! 🎉
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
package.json:94-95 |
New devDependencies (playwright, sharp) | Informational only — both are justified and version pinning follows conventions |
SKILL.md:1-5 |
Frontmatter validation | Positive observation — correctly follows pattern, not an issue |
capture.ts:165-172 |
chromium.connect() error handling | Merged with server startup error handling (similar pattern, lower priority) |
validate-sensitive.ts |
scanFile error handling | Covered by Consider #3 (main() wrapping addresses this) |
annotate.ts:106 |
SVG injection (colors) | Covered by Consider #1 (hardcoded values, not exploitable) |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
6 | 1 | 0 | 0 | 3 | 0 | 2 |
pr-review-standards |
4 | 0 | 1 | 0 | 2 | 0 | 1 |
pr-review-errors |
5 | 0 | 2 | 0 | 2 | 0 | 1 |
| Total | 15 | 1 | 3 | 0 | 7 | 0 | 4 |
Note: pr-review-errors finding #1 (catch all errors) merged with pr-review-standards finding #3 (double navigation) as they describe the same issue.
| } | ||
|
|
||
| const routes = routesStr.split(',').map((r) => r.trim()); | ||
| const [vw, vh] = viewport.split('x').map(Number); |
There was a problem hiding this comment.
🟠 MAJOR: Missing viewport input validation
Issue: The viewport string is split and mapped to Number without validation. If a user passes --viewport invalid or --viewport 1280 (missing height), vw or vh will be NaN, causing a confusing Playwright error downstream.
Why: Input validation at parse time provides clear error messages. Users debugging this would otherwise see cryptic Playwright errors about invalid viewport dimensions.
Fix:
| const [vw, vh] = viewport.split('x').map(Number); | |
| const [vw, vh] = viewport.split('x').map(Number); | |
| if (!vw || !vh || Number.isNaN(vw) || Number.isNaN(vh)) { | |
| console.error('Invalid viewport format. Use WxH (e.g., 1280x800)'); | |
| process.exit(1); | |
| } |
Refs:
| try { | ||
| await page.goto(url, { waitUntil: 'networkidle', timeout: 30000 }); | ||
| } catch { | ||
| console.log(' networkidle timed out, proceeding with domcontentloaded...'); | ||
| await page.goto(url, { waitUntil: 'domcontentloaded', timeout: 30000 }); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Catch block swallows all errors, not just timeouts
Issue: The catch block catches ALL errors from the networkidle navigation but only handles timeouts. Non-timeout failures (DNS errors, connection refused, SSL issues, HTTP 4xx/5xx) are silently discarded. The catch { } syntax with no error parameter means the original error is completely lost.
Why: If the target server is down or returns errors, users see "networkidle timed out" followed by the same failure on retry. The actual cause (e.g., ECONNREFUSED, ERR_CERT_AUTHORITY_INVALID) is lost, making debugging difficult.
Fix: Use waitForLoadState after successful navigation, or check error type before falling back:
await page.goto(url, { waitUntil: 'domcontentloaded', timeout: 30000 });
try {
await page.waitForLoadState('networkidle', { timeout: 10000 });
} catch {
console.log(' networkidle timed out, proceeding with current state...');
}Or catch only timeout errors:
try {
await page.goto(url, { waitUntil: 'networkidle', timeout: 30000 });
} catch (error) {
const isTimeout = error instanceof Error &&
(error.name === 'TimeoutError' || error.message.includes('timeout'));
if (isTimeout) {
console.log(' networkidle timed out, proceeding with domcontentloaded...');
await page.goto(url, { waitUntil: 'domcontentloaded', timeout: 30000 });
} else {
throw error; // Re-throw non-timeout errors
}
}Refs:
| async function startServer(port: number) { | ||
| const server = await chromium.launchServer({ | ||
| port, | ||
| headless: true, | ||
| }); | ||
| console.log(`Playwright server started at: ${server.wsEndpoint()}`); | ||
| console.log('Press Ctrl+C to stop.'); |
There was a problem hiding this comment.
🟠 MAJOR: Missing error handling for server startup
Issue: The startServer function has no error handling for chromium.launchServer(). If the port is in use or Playwright fails to launch, the error bubbles up to main().catch() which outputs "Capture failed:" — misleading because server mode isn't capture mode.
Why: Common failures (port conflict, Chrome not installed, permissions) have no specific guidance. Users see confusing error messages that don't indicate they should try a different port or check their Playwright installation.
Fix: Add specific error handling for server startup:
async function startServer(port: number) {
let server;
try {
server = await chromium.launchServer({
port,
headless: true,
});
} catch (error) {
const message = error instanceof Error ? error.message : String(error);
if (message.includes('EADDRINUSE')) {
console.error(`Port ${port} is already in use. Try a different port with --port <number>`);
} else {
console.error(`Failed to start Playwright server: ${message}`);
}
process.exit(1);
}
console.log(`Playwright server started at: ${server.wsEndpoint()}`);
// ... rest
}Refs:
|
|
||
| const labelSvg = Buffer.from(` | ||
| <svg width="${width}" height="${labelHeight}"> | ||
| <rect x="0" y="0" width="${width}" height="${labelHeight}" fill="${borderColor}" /> |
There was a problem hiding this comment.
🟡 MINOR: SVG attribute not escaped (defense-in-depth)
Issue: The borderColor parameter is interpolated directly into the SVG fill attribute without sanitization. While escapeXml is correctly used for label text, the color value could contain quotes or other XML-special characters.
Why: While low-risk (Sharp renders server-side, not a browser), this violates defense-in-depth principles. A malformed color value could break SVG parsing.
Fix:
| <rect x="0" y="0" width="${width}" height="${labelHeight}" fill="${borderColor}" /> | |
| <rect x="0" y="0" width="${width}" height="${labelHeight}" fill="${escapeXml(borderColor)}" /> |
Refs:
| - Tokens (Bearer, JWT, GitHub PATs) | ||
| - PEM private keys | ||
| - Connection strings with credentials | ||
| - Email addresses |
There was a problem hiding this comment.
🟡 MINOR: Documentation lists "Email addresses" but script doesn't check for them
Issue: The documentation states the validation script checks for "Email addresses" but validate-sensitive.ts has no email pattern in its SENSITIVE_PATTERNS array.
Why: Documentation should accurately reflect what the script does. Users relying on email detection would be surprised to find it's not implemented.
Fix: Either remove "Email addresses" from this list, or add the pattern to validate-sensitive.ts:
{ name: 'Email address', pattern: /[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/g, severity: 'warning' },Refs:
- validate-sensitive.ts:25-61 — SENSITIVE_PATTERNS array
|
|
||
| | Package | Source | Notes | | ||
| |---|---|---| | ||
| | `playwright` | `agents-manage-ui` workspace dep | Browser automation | |
There was a problem hiding this comment.
🟡 MINOR: Incorrect dependency source for playwright
Issue: This table states playwright is an agents-manage-ui workspace dep, but this PR adds it directly to root devDependencies in package.json.
Why: Documentation should accurately reflect where dependencies come from. Users looking here for troubleshooting may be confused.
Fix:
| | `playwright` | `agents-manage-ui` workspace dep | Browser automation | | |
| | `playwright` | Root dev dep | Browser automation | |
Refs:
- package.json:94 — playwright added to root devDependencies
| .pnpm-store/ | ||
| .vercel | ||
|
|
||
| # PR screenshot captures (generated by .cursor/skills/pr-screenshots/) |
There was a problem hiding this comment.
🟡 MINOR: Comment references non-canonical path
Issue: The comment references .cursor/skills/pr-screenshots/ but the canonical source location is .agents/skills/pr-screenshots/. While both work due to symlinks, the canonical path should be used in documentation.
Why: Consistency with the established convention that .agents/skills/ is the source of truth, with .cursor/, .claude/, .codex/ as symlinks per AGENTS.md.
Fix:
| # PR screenshot captures (generated by .cursor/skills/pr-screenshots/) | |
| # PR screenshot captures (generated by .agents/skills/pr-screenshots/) |
Refs:
- AGENTS.md — documents the symlink convention
1 participant
Summary
pr-screenshotsskill that teaches AI coding agents (Cursor, Codex, Claude Code) how to capture, redact, annotate, and embed screenshots in pull requestsWhat's included
Skill:
.agents/skills/pr-screenshots/Key features
capture.ts— Playwright-based screenshot capture--base-url http://localhost:3000)--base-url https://agents-git-branch-inkeep.vercel.app)--serve/--connect ws://...)annotate.ts— Sharp-based image annotationvalidate-sensitive.ts— Pre-upload sensitive data validationExample: capture → validate → annotate pipeline
Dependency changes
sharpandplaywrightto rootdevDependencies/pr-screenshots/to.gitignore(output directory only)Test plan
capture.tscompiles and runs (tested against local dev server)validate-sensitive.tsdetects API keys, JWTs, Bearer tokens (tested with synthetic sensitive data)validate-sensitive.tspasses clean files (tested with normal page content)annotate.tslabel mode produces labeled PNG with colored border (tested with generated image)annotate.tsstitch mode produces side-by-side comparison (tested with two generated images)Made with Cursor