Add PR UI screenshot workflow#4
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @kushal238, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust UI screenshot and visual regression testing pipeline within the GitHub Actions workflow. Its primary purpose is to automatically detect unintended visual changes in the user interface by comparing screenshots of key application pages before and after a pull request's changes are applied. This proactive approach helps maintain UI consistency and quality, providing developers and reviewers with immediate visual feedback on any layout or styling discrepancies. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a UI screenshot testing workflow using Playwright and pixelmatch. The changes are well-structured, adding necessary dependencies and a script for capturing and comparing screenshots. I've identified a critical issue where an asynchronous function isn't awaited, which would cause the script to exit prematurely. Additionally, I've provided suggestions to improve performance by parallelizing screenshot capture and to enhance robustness by handling mismatched image dimensions during the diffing process.
| if (process.argv.includes('--diff')) { | ||
| diffAll(); | ||
| } else { | ||
| screenshotAll(); | ||
| } |
There was a problem hiding this comment.
The screenshotAll function is async, but it is not being awaited. This will cause the script to exit before the asynchronous screenshot operations are complete, likely resulting in no screenshots being saved. You should wrap the main execution logic in an async IIFE (Immediately Invoked Function Expression) to properly await the screenshotAll function.
(async () => {
if (process.argv.includes('--diff')) {
diffAll();
} else {
await screenshotAll();
}
})();| const img1 = PNG.sync.read(fs.readFileSync(beforePath)); | ||
| const img2 = PNG.sync.read(fs.readFileSync(afterPath)); | ||
| const { width, height } = img1; | ||
| const diff = new PNG({ width, height }); | ||
| pixelmatch(img1.data, img2.data, diff.data, width, height, { threshold: 0.1 }); | ||
| fs.writeFileSync(path.join(diffDir, name), PNG.sync.write(diff)); |
There was a problem hiding this comment.
If the dimensions of the 'before' and 'after' images do not match, pixelmatch will throw an error, causing the script to crash. This can happen if a UI change affects the page height. It's better to handle this case gracefully by checking the dimensions. If they differ, you could log a warning and, for instance, copy the new image to the diff directory to make the size change apparent.
const img1 = PNG.sync.read(fs.readFileSync(beforePath));
const img2 = PNG.sync.read(fs.readFileSync(afterPath));
const { width, height } = img1;
if (width !== img2.width || height !== img2.height) {
console.warn(`Image dimensions mismatch for ${name}. Copying new image to diff.`);
fs.writeFileSync(path.join(diffDir, name), fs.readFileSync(afterPath));
} else {
const diff = new PNG({ width, height });
pixelmatch(img1.data, img2.data, diff.data, width, height, { threshold: 0.1 });
fs.writeFileSync(path.join(diffDir, name), PNG.sync.write(diff));
}| async function screenshotAll() { | ||
| ensureDir(outDir); | ||
| const browser = await chromium.launch(); | ||
| const page = await browser.newPage({ viewport: { width: 1280, height: 720 } }); | ||
|
|
||
| for (const route of routes) { | ||
| const url = baseUrl + route; | ||
| await page.goto(url, { waitUntil: 'networkidle' }); | ||
| const file = path.join(outDir, `${slug(route)}.png`); | ||
| await page.screenshot({ path: file, fullPage: true }); | ||
| } | ||
|
|
||
| await browser.close(); | ||
| } |
There was a problem hiding this comment.
The screenshotAll function currently takes screenshots sequentially. For a larger number of routes, this could be inefficient. You can significantly improve performance by running these operations in parallel using Promise.all. Additionally, wrapping resource allocation in a try...finally block is a good practice to ensure resources like the browser instance are always cleaned up, even if errors occur.
async function screenshotAll() {
ensureDir(outDir);
const browser = await chromium.launch();
try {
const context = await browser.newContext({ viewport: { width: 1280, height: 720 } });
await Promise.all(
routes.map(async (route) => {
const page = await context.newPage();
try {
const url = baseUrl + route;
await page.goto(url, { waitUntil: 'networkidle' });
const file = path.join(outDir, `${slug(route)}.png`);
await page.screenshot({ path: file, fullPage: true });
} finally {
await page.close();
}
}),
);
} finally {
await browser.close();
}
}
- Supabase client: use placeholder URL during static generation so createClient doesn't throw "supabaseUrl is required" at build time - ui-snapshots: await screenshotAll() via async IIFE so script doesn't exit before screenshots are saved - ui-snapshots: guard against image dimension mismatch in pixelmatch - ui-snapshots: wrap browser in try/finally for cleanup on errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable UI screenshot testing workflow using Playwright. The implementation is clear and well-structured. My main feedback is to refactor the new snapshot script to use asynchronous file system operations consistently, which will improve its robustness and maintainability, especially in a CI environment. The changes to the Supabase client initialization are a good improvement for build-time compatibility.
| function diffAll() { | ||
| ensureDir(diffDir); | ||
| for (const route of routes) { | ||
| const name = `${slug(route)}.png`; | ||
| const beforePath = path.join(beforeDir, name); | ||
| const afterPath = path.join(outDir, name); | ||
| if (!fs.existsSync(beforePath) || !fs.existsSync(afterPath)) continue; | ||
|
|
||
| const img1 = PNG.sync.read(fs.readFileSync(beforePath)); | ||
| const img2 = PNG.sync.read(fs.readFileSync(afterPath)); | ||
| const { width, height } = img1; | ||
|
|
||
| if (width !== img2.width || height !== img2.height) { | ||
| console.warn(`Image dimensions mismatch for ${name}. Copying new image to diff.`); | ||
| fs.writeFileSync(path.join(diffDir, name), fs.readFileSync(afterPath)); | ||
| continue; | ||
| } | ||
|
|
||
| const diff = new PNG({ width, height }); | ||
| pixelmatch(img1.data, img2.data, diff.data, width, height, { threshold: 0.1 }); | ||
| fs.writeFileSync(path.join(diffDir, name), PNG.sync.write(diff)); | ||
| } | ||
| } | ||
|
|
||
| (async () => { | ||
| if (process.argv.includes('--diff')) { | ||
| diffAll(); | ||
| } else { | ||
| await screenshotAll(); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
The diffAll function uses synchronous file system operations, which is inconsistent with the async screenshotAll function. This can be refactored to use async/await with fs.promises for better consistency and to make the script non-blocking. Additionally, the main execution block should await the async diffAll call and include error handling to ensure the script exits correctly on failure, which is important for CI environments.
async function diffAll() {
ensureDir(diffDir);
for (const route of routes) {
const name = `${slug(route)}.png`;
const beforePath = path.join(beforeDir, name);
const afterPath = path.join(outDir, name);
let beforeFile, afterFile;
try {
beforeFile = await fs.promises.readFile(beforePath);
afterFile = await fs.promises.readFile(afterPath);
} catch {
// Skip if either file doesn't exist.
continue;
}
const img1 = PNG.sync.read(beforeFile);
const img2 = PNG.sync.read(afterFile);
const { width, height } = img1;
if (width !== img2.width || height !== img2.height) {
console.warn(`Image dimensions mismatch for ${name}. Copying new image to diff.`);
await fs.promises.writeFile(path.join(diffDir, name), afterFile);
continue;
}
const diff = new PNG({ width, height });
pixelmatch(img1.data, img2.data, diff.data, width, height, { threshold: 0.1 });
await fs.promises.writeFile(path.join(diffDir, name), PNG.sync.write(diff));
}
}
(async () => {
try {
if (process.argv.includes('--diff')) {
await diffAll();
} else {
await screenshotAll();
}
} catch (error) {
console.error('An error occurred:', error);
process.exit(1);
}
})();The base branch (main) doesn't have the Supabase client fallback, so both build steps need dummy env vars to avoid the createClient crash during static generation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Main branch doesn't have ui-snapshots.js or its dependencies (pixelmatch, pngjs, playwright). Restructure workflow to checkout PR first, install deps, save the script, then swap source to base for "before" screenshots. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Details
Notes