feat(screenshot): add OS-specific browser path detection#259
Conversation
Summary of ChangesHello @l1ttps, 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 enhances the asset selection process within asset groups by improving the management of selected assets and introducing filtering capabilities. Additionally, it improves the screenshot tool by adding OS-specific browser path detection, simplifying its setup and configuration. 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces two main features: OS-specific browser path detection for the screenshot tool and a search filter for selecting assets to add to an asset group. The changes also include a refactoring to use a Set for managing selected assets, which is a good performance improvement.
My review focuses on improving code clarity and maintainability. I've suggested refactoring the new path detection logic in screenshot.ts to make it more concise and easier to manage. I also pointed out an empty useEffect hook in select-assets-dialog.tsx that should either be implemented or removed to avoid confusion.
Overall, the changes are good and add valuable functionality.
| function getExecutablePath(): string | undefined { | ||
| const envPath = process.env.PUPPETEER_EXECUTABLE_PATH; | ||
| if (envPath) { | ||
| return envPath; | ||
| } | ||
|
|
||
| const platform = os.platform(); | ||
|
|
||
| if (platform === 'win32') { | ||
| // Try common Windows Chromium/Chrome locations | ||
| const windowsPaths = [ | ||
| 'C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe', | ||
| 'C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe', | ||
| 'C:\\Program Files\\Chromium\\Application\\chrome.exe', | ||
| 'C:\\Program Files (x86)\\Chromium\\Application\\chrome.exe', | ||
| ]; | ||
|
|
||
| for (const path of windowsPaths) { | ||
| if (require('fs').existsSync(path)) { | ||
| return path; | ||
| } | ||
| } | ||
|
|
||
| // If no specific path found, let puppeteer use its default (will auto-download if needed) | ||
| return undefined; | ||
| } else if (platform === 'linux') { | ||
| // Common Linux paths | ||
| const linuxPaths = [ | ||
| '/usr/bin/chromium-browser', | ||
| '/usr/bin/chromium', | ||
| '/usr/bin/google-chrome', | ||
| '/usr/bin/google-chrome-stable', | ||
| ]; | ||
|
|
||
| for (const path of linuxPaths) { | ||
| if (require('fs').existsSync(path)) { | ||
| return path; | ||
| } | ||
| } | ||
|
|
||
| return '/usr/bin/chromium-browser'; // fallback to original | ||
| } else if (platform === 'darwin') { | ||
| // macOS paths | ||
| const macPaths = [ | ||
| '/Applications/Google Chrome.app/Contents/MacOS/Google Chrome', | ||
| '/Applications/Chromium.app/Contents/MacOS/Chromium', | ||
| ]; | ||
|
|
||
| for (const path of macPaths) { | ||
| if (require('fs').existsSync(path)) { | ||
| return path; | ||
| } | ||
| } | ||
|
|
||
| return undefined; // let puppeteer use default | ||
| } | ||
|
|
||
| return undefined; // let puppeteer use default | ||
| } |
There was a problem hiding this comment.
This function has a few areas for improvement:
- Module Imports: It uses
require('fs')inline, which is not standard for ES modules. It's better toimport fs from 'fs';at the top of the file for consistency and clarity. - Repetitive Logic: The structure for checking paths is repeated for each OS. This can be simplified.
- Readability: The function is quite long.
Consider refactoring it by defining the paths in a separate configuration object and using a single loop. This would make the code cleaner and easier to maintain. You would need to add import fs from 'fs'; at the top of the file for this to work.
Here's an example of how it could look:
const BROWSER_PATHS: Partial<Record<NodeJS.Platform, string[]>> = {
win32: [
'C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe',
'C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe',
'C:\\Program Files\\Chromium\\Application\\chrome.exe',
'C:\\Program Files (x86)\\Chromium\\Application\\chrome.exe',
],
linux: [
'/usr/bin/chromium-browser',
'/usr/bin/chromium',
'/usr/bin/google-chrome',
'/usr/bin/google-chrome-stable',
],
darwin: [
'/Applications/Google Chrome.app/Contents/MacOS/Google Chrome',
'/Applications/Chromium.app/Contents/MacOS/Chromium',
],
};
function getExecutablePath(): string | undefined {
const envPath = process.env.PUPPETEER_EXECUTABLE_PATH;
if (envPath) {
return envPath;
}
const platform = os.platform();
const paths = BROWSER_PATHS[platform];
if (paths) {
for (const path of paths) {
if (fs.existsSync(path)) {
return path;
}
}
}
if (platform === 'linux') {
return '/usr/bin/chromium-browser'; // fallback to original
}
return undefined; // let puppeteer use default
}
No description provided.