feat(tools): add screenshot tool for website capture#253
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 significantly enhances the asset management system by introducing a new capability to capture and display website screenshots. This feature provides valuable visual context for assets directly within the console, improving the overall user experience and information richness. The changes span across the frontend for display, the backend API for data management, and the worker service for execution, ensuring a cohesive and integrated solution. 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 a new screenshot tool, which is a great addition. The implementation spans the backend, worker, and frontend. The worker-side screenshot handler using Puppeteer is well-implemented with good anti-detection measures. The backend changes correctly integrate the new tool into the job system. On the frontend, new components are added to display the screenshots.
I've found a few issues: a critical bug in an image component that could break existing image loading, a high-severity bug in the backend that could lead to broken screenshot URLs, and a medium-severity issue regarding resource management in the worker. My review comments provide details and suggestions for fixing these.
| /> | ||
| ); | ||
| // If not a complete URL, prepend with API storage path | ||
| const imageUrl = isHttpUrl ? url : `api${url}`; |
There was a problem hiding this comment.
The removal of the leading slash in the imageUrl construction changes it from an absolute path to a relative path. This will likely break image loading for any non-HTTP URLs, as the browser will try to resolve the path from the current page's directory.
| const imageUrl = isHttpUrl ? url : `api${url}`; | |
| const imageUrl = isHttpUrl ? url : `/api${url}`; |
| asset.dnsRecords = item.asset?.dnsRecords; | ||
| asset.isEnabled = item.asset?.isEnabled; | ||
| asset.port = item.port; | ||
| asset.screenshotPath = `${STORAGE_BASE_PATH}/${item.screenshotPath}`; |
There was a problem hiding this comment.
The screenshotPath is constructed without checking if item.screenshotPath exists. If item.screenshotPath is null or undefined, this will result in asset.screenshotPath being set to a string like '/api/storage/undefined', leading to a broken image link. You should conditionally construct this path, similar to how it's done in getManyAsssetServices.
| asset.screenshotPath = `${STORAGE_BASE_PATH}/${item.screenshotPath}`; | |
| asset.screenshotPath = item.screenshotPath && `${STORAGE_BASE_PATH}/${item.screenshotPath}`; |
| import type { Job } from '../../services/core-api/api'; | ||
|
|
||
| // Shared browser instance to reuse across requests | ||
| let browser: Browser | null = null; |
There was a problem hiding this comment.
The shared Puppeteer browser instance is created but never explicitly closed. While it will be terminated when the worker process exits, it's a good practice to handle resources gracefully. Consider exporting a closeBrowser function that can be called during the worker's shutdown sequence to ensure the browser is closed cleanly. This prevents potential resource leaks, especially in scenarios where the worker might be part of a more complex lifecycle management system.
You could add a function like this:
export async function closeBrowser(): Promise<void> {
if (browser) {
await browser.close();
browser = null;
}
}...and then call it from your application's shutdown logic.
No description provided.