feat: using firecrawl to screenshot #2665
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
We don't need to wait for this to finish to close the project anymore because the screenshot is taken async. But we should perhaps take the screenshot before the project stops if there is no screenshot yet. |
packages/ai/src/tools/web.ts
Outdated
| @@ -26,6 +26,14 @@ export const SCRAPE_URL_TOOL_PARAMETERS = z.object({ | |||
| .number() | |||
| .optional() | |||
| .describe('Time in milliseconds to wait for the page to load before scraping.'), | |||
| actions: z | |||
There was a problem hiding this comment.
Fyi this is for the AI tool, which is nice but please ensure the screenshot can be used by the AI properly if it is returned.
So please update the tool handler as well in this case otherwise I would not make this change
There was a problem hiding this comment.
Right now it is returning the image to the AI as a URL. the AI will not be able to see this image and will hallucinate an answer. Please see this vercel/ai#4771
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughServer-side captureScreenshot TRPC route added; client removed local capture/upload flow and now triggers EditorEngine.screenshot.captureScreenshot(); ScreenshotManager added and integrated into EditorEngine; sandbox writes trigger non-blocking captures; DB/schema/types add updatedPreviewImgAt; canvas router removed and user-canvas endpoints adjusted; assorted client/import/UI/test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant EditorEngine
participant TRPC_Server
participant Firecrawl
participant Storage
participant Database
User->>Client: user action triggers handler
Client->>EditorEngine: editorEngine.screenshot.captureScreenshot() (non-blocking)
EditorEngine->>TRPC_Server: api.project.captureScreenshot.mutate({ projectId })
TRPC_Server->>Database: load project by id
TRPC_Server->>Firecrawl: scrape project's sandbox URL (formats: screenshot)
Firecrawl-->>TRPC_Server: returns screenshot URL
TRPC_Server->>TRPC_Server: fetch bytes, infer MIME, compress image
TRPC_Server->>Storage: upload compressed image -> returns path
TRPC_Server->>Database: update project's preview fields (url, path, bucket, updatedPreviewImgAt, updatedAt)
TRPC_Server-->>EditorEngine: return { success, path } or { success: false, error }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
apps/web/client/src/app/project/[id]/_components/top-bar/project-breadcrumb.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/ui/src/components/sonner.tsx (1)
9-16: Consider making the toaster position configurableThe hardcoded
bottom-leftposition might not be suitable for all use cases. Consider accepting it as a prop with a default value.Apply this diff to make the position configurable:
-const Toaster = ({ ...props }: ToasterProps) => { +const Toaster = ({ position = "bottom-left", ...props }: ToasterProps) => { const { theme = 'system' } = useTheme(); return ( <Sonner - position="bottom-left" + position={position} theme={theme as ToasterProps['theme']} className="toaster group" {...props} /> );
🧹 Nitpick comments (5)
apps/web/client/src/server/api/routers/project/project.ts (3)
40-42: Improve error handling for missing API keyThe error message should be more user-friendly and not expose configuration details to the client.
Apply this diff to improve the error message:
if (!env.FIRECRAWL_API_KEY) { - throw new Error('FIRECRAWL_API_KEY is not configured'); + throw new Error('Screenshot service is not configured. Please contact support.'); }
58-67: Consider adding timeout configuration for FirecrawlThe Firecrawl scraping operation could potentially hang or take too long. Consider adding a timeout parameter.
Apply this diff to add timeout configuration:
const result = await app.scrapeUrl(project.sandboxUrl, { formats: ['screenshot'], onlyMainContent: true, + timeout: 30000, // 30 seconds timeout actions: [ { type: 'screenshot', fullPage: true, }, ], });
120-124: Return type inconsistencyThe mutation returns different shapes for success and failure cases. Consider using a consistent return type for better type safety.
Apply this diff to use a consistent return type:
- .mutation(async ({ ctx, input }) => { + .mutation(async ({ ctx, input }): Promise<{ success: boolean; path?: string; error?: string }> => { try { // ... existing code ... return { success: true, path: data.path }; } catch (error) { console.error('Error capturing project screenshot:', error); - return { success: false, error: error instanceof Error ? error.message : 'Unknown error' }; + return { + success: false, + error: error instanceof Error ? error.message : 'Unknown error' + }; } }),apps/web/client/src/app/project/[id]/_components/top-bar/project-breadcrumb.tsx (2)
38-51: Unused parameter in function signatureThe
_routeparameter inhandleNavigateToProjectsis defined but never used.Apply this diff to remove the unused parameter:
-async function handleNavigateToProjects(_route?: 'create' | 'import') { +async function handleNavigateToProjects() {
59-59: Check mutation response handlingThe
captureScreenshotmutation returns a response object withsuccessand potentiallyerrorfields, but this response is not being used.The mutation should properly handle the response as shown in the previous comment to provide user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/client/src/app/project/[id]/_components/top-bar/project-breadcrumb.tsx(7 hunks)apps/web/client/src/components/tools/handlers/web.ts(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(2 hunks)packages/ui/src/components/sonner.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/client/src/server/api/routers/project/project.ts (8)
apps/web/client/src/server/api/trpc.ts (2)
createTRPCRouter(88-88)protectedProcedure(130-149)apps/web/client/src/env.ts (1)
env(4-133)packages/db/src/schema/project/project.ts (1)
projects(12-29)packages/utility/src/urls.ts (1)
getValidUrl(4-9)packages/utility/src/screenshot.ts (1)
getScreenshotPath(1-4)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/db/src/dto/project/project.ts (1)
fromPreviewImg(58-80)packages/models/src/projects/index.ts (1)
Project(1-10)
apps/web/client/src/app/project/[id]/_components/top-bar/project-breadcrumb.tsx (5)
apps/web/preload/script/api/screenshot.ts (1)
captureScreenshot(1-97)apps/web/client/src/trpc/react.tsx (1)
api(23-23)apps/web/server/src/router/trpc.ts (1)
router(12-12)packages/ui/src/components/dropdown-menu.tsx (2)
DropdownMenuItem(216-216)DropdownMenuSeparator(221-221)apps/web/client/src/utils/constants/index.ts (1)
Routes(1-20)
🔇 Additional comments (1)
apps/web/client/src/components/tools/handlers/web.ts (1)
3-7: Type-only imports are appropriate hereThe conversion to type-only imports is correct since these constants are only used as type arguments to
z.infer. This optimization helps reduce bundle size.
apps/web/client/src/app/project/[id]/_components/top-bar/project-breadcrumb.tsx
Outdated
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/top-bar/project-breadcrumb.tsx
Outdated
Show resolved
Hide resolved
| captureScreenshot: protectedProcedure | ||
| .input(z.object({ projectId: z.string() })) | ||
| .mutation(async ({ ctx, input }) => { | ||
| try { | ||
| if (!env.FIRECRAWL_API_KEY) { | ||
| throw new Error('FIRECRAWL_API_KEY is not configured'); | ||
| } | ||
|
|
||
| const project = await ctx.db.query.projects.findFirst({ | ||
| where: eq(projects.id, input.projectId), | ||
| }); | ||
|
|
||
| if (!project) { | ||
| throw new Error('Project not found'); | ||
| } | ||
|
|
||
| if (!project.sandboxUrl) { | ||
| throw new Error('No sandbox URL found'); | ||
| } | ||
|
|
||
| const app = new FirecrawlApp({ apiKey: env.FIRECRAWL_API_KEY }); | ||
|
|
||
| const result = await app.scrapeUrl(project.sandboxUrl, { | ||
| formats: ['screenshot'], | ||
| onlyMainContent: true, | ||
| actions: [ | ||
| { | ||
| type: 'screenshot', | ||
| fullPage: true, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| if (!result.success) { | ||
| throw new Error(`Failed to scrape URL: ${result.error || 'Unknown error'}`); | ||
| } | ||
|
|
||
| const screenshotUrlRaw = result.screenshot; | ||
| const screenshotUrl = screenshotUrlRaw ? getValidUrl(screenshotUrlRaw) : null; | ||
|
|
||
| if (!screenshotUrl) { | ||
| throw new Error('Invalid screenshot URL'); | ||
| } | ||
|
|
||
| const response = await fetch(screenshotUrl); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch screenshot: ${response.status} ${response.statusText}`); | ||
| } | ||
|
|
||
| const arrayBuffer = await response.arrayBuffer(); | ||
| const mimeType = response.headers.get('content-type') ?? 'image/png'; | ||
| const path = getScreenshotPath(project.id, mimeType); | ||
|
|
||
| const { data, error } = await ctx.supabase.storage | ||
| .from(STORAGE_BUCKETS.PREVIEW_IMAGES) | ||
| .upload(path, arrayBuffer, { | ||
| contentType: mimeType, | ||
| }); | ||
|
|
||
| if (error) { | ||
| throw new Error(`Supabase upload error: ${error.message}`); | ||
| } | ||
|
|
||
| if (!data) { | ||
| throw new Error('No data returned from storage upload'); | ||
| } | ||
|
|
||
| const dbPreviewImg = fromPreviewImg({ | ||
| type: 'storage', | ||
| storagePath: { | ||
| bucket: STORAGE_BUCKETS.PREVIEW_IMAGES, | ||
| path: data.path, | ||
| }, | ||
| }); | ||
|
|
||
| await ctx.db.update(projects) | ||
| .set({ | ||
| previewImgUrl: dbPreviewImg.previewImgUrl, | ||
| previewImgPath: dbPreviewImg.previewImgPath, | ||
| previewImgBucket: dbPreviewImg.previewImgBucket, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(projects.id, project.id)); | ||
|
|
||
| return { success: true, path: data.path }; | ||
| } catch (error) { | ||
| console.error('Error capturing project screenshot:', error); | ||
| return { success: false, error: error instanceof Error ? error.message : 'Unknown error' }; | ||
| } | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rate limiting for screenshot capture
The screenshot capture endpoint could be abused since it involves external API calls to Firecrawl and storage operations. Consider adding rate limiting to prevent excessive usage.
Consider implementing rate limiting using a middleware or a rate limiting library like express-rate-limit or maintaining a timestamp in the database to track the last screenshot capture time per project.
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/project/project.ts around lines 36 to
125, the captureScreenshot mutation lacks rate limiting and can be abused; add a
limit by either (A) integrating a request-level rate limiter middleware
compatible with your router (e.g., a token-bucket or express/fastify/trpc
rate-limiter) and apply it to this procedure, or (B) implement a simple
per-project cooldown by adding a lastScreenshotAt timestamp to the projects
table, reading that timestamp at the start of the mutation, returning an error
if the last capture is within the cooldown window (e.g., 1 request per 5
minutes), updating lastScreenshotAt on successful upload, and ensure the DB
check-and-update is transactional to avoid race conditions; choose one approach
and wire the check before calling Firecrawl, returning an explanatory error when
the limit is exceeded.
| const arrayBuffer = await response.arrayBuffer(); | ||
| const mimeType = response.headers.get('content-type') ?? 'image/png'; | ||
| const path = getScreenshotPath(project.id, mimeType); | ||
|
|
||
| const { data, error } = await ctx.supabase.storage | ||
| .from(STORAGE_BUCKETS.PREVIEW_IMAGES) | ||
| .upload(path, arrayBuffer, { | ||
| contentType: mimeType, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file size validation for screenshots
Large screenshots could consume significant storage space. Consider adding size validation before uploading.
Apply this diff to add size validation:
const arrayBuffer = await response.arrayBuffer();
+const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB
+if (arrayBuffer.byteLength > MAX_FILE_SIZE) {
+ throw new Error(`Screenshot too large: ${(arrayBuffer.byteLength / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit`);
+}
const mimeType = response.headers.get('content-type') ?? 'image/png';
const path = getScreenshotPath(project.id, mimeType);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const arrayBuffer = await response.arrayBuffer(); | |
| const mimeType = response.headers.get('content-type') ?? 'image/png'; | |
| const path = getScreenshotPath(project.id, mimeType); | |
| const { data, error } = await ctx.supabase.storage | |
| .from(STORAGE_BUCKETS.PREVIEW_IMAGES) | |
| .upload(path, arrayBuffer, { | |
| contentType: mimeType, | |
| }); | |
| const arrayBuffer = await response.arrayBuffer(); | |
| + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB | |
| + if (arrayBuffer.byteLength > MAX_FILE_SIZE) { | |
| + throw new Error( | |
| + `Screenshot too large: ${(arrayBuffer.byteLength / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit` | |
| + ); | |
| + } | |
| const mimeType = response.headers.get('content-type') ?? 'image/png'; | |
| const path = getScreenshotPath(project.id, mimeType); | |
| const { data, error } = await ctx.supabase.storage | |
| .from(STORAGE_BUCKETS.PREVIEW_IMAGES) | |
| .upload(path, arrayBuffer, { | |
| contentType: mimeType, | |
| }); |
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/project/project.ts around lines 85 to
93, add validation to prevent uploading oversized screenshots: before calling
ctx.supabase.storage.upload, determine the payload size (preferably using
response.headers.get('content-length') if present as a fast pre-check, otherwise
use arrayBuffer.byteLength after awaiting response.arrayBuffer()) and compare it
against a configured MAX_SCREENSHOT_SIZE_BYTES constant; if the size exceeds the
limit, log/return an appropriate error or skip the upload instead of calling
upload, and only proceed to call ctx.supabase.storage.from(...).upload when the
size is within limits.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/web/client/src/server/api/routers/project/project.ts (2)
37-44: Add rate limiting/cooldown to prevent abuseThis endpoint performs external calls and storage writes; it’s susceptible to abuse. Add a per-project cooldown or a rate limiter keyed by userId+projectId.
Options:
- DB-backed cooldown: add a lastScreenshotAt column to projects; on entry, reject if within X minutes; update timestamp on success. Make the read-modify-write transactional.
- External limiter: use a token bucket (e.g., Upstash Redis Ratelimit) keyed by
${ctx.user.id}:${input.projectId}.I can provide a migration and wiring if you choose the DB approach.
81-87: Add screenshot size validation before downloading into memory and before uploadLarge images can blow memory and storage. Pre-check content-length and enforce a hard cap after reading the buffer as a fallback.
Apply this diff:
-const response = await fetch(screenshotUrl); +const response = await fetch(screenshotUrl, { signal: AbortSignal.timeout(30_000) }); if (!response.ok) { throw new Error(`Failed to fetch screenshot: ${response.status} ${response.statusText}`); } -const arrayBuffer = await response.arrayBuffer(); +const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB +const contentLength = response.headers.get('content-length'); +if (contentLength && Number(contentLength) > MAX_FILE_SIZE) { + throw new Error(`Screenshot too large: ${(Number(contentLength) / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit`); +} + +const arrayBuffer = await response.arrayBuffer(); +if (arrayBuffer.byteLength > MAX_FILE_SIZE) { + throw new Error(`Screenshot too large after download: ${(arrayBuffer.byteLength / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit`); +}
🧹 Nitpick comments (2)
apps/web/client/src/server/api/routers/project/project.ts (2)
99-103: Consider setting upsert and cache-control on Supabase uploadTo avoid rare path collisions and improve client caching.
Apply this diff:
- .upload(path, uploadBuffer, { - contentType: finalMimeType, - }); + .upload(path, uploadBuffer, { + contentType: finalMimeType, + upsert: false, // set to true if you expect re-captures to overwrite the same path + cacheControl: 'public, max-age=31536000, immutable', + });
131-134: Avoid returning raw internal error messages to clientsReturning the full error message can leak implementation details or environment state.
Suggested pattern:
- Log the full error server-side.
- Return a terse, user-friendly message (and optionally an error code) client-side.
Example:
-console.error('Error capturing project screenshot:', error); -return { success: false, error: error instanceof Error ? error.message : 'Unknown error' }; +console.error('Error capturing project screenshot:', error); +return { success: false, error: 'Failed to capture project screenshot. Please try again later.' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/server/api/routers/project/project.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/client/src/server/api/routers/project/project.ts (7)
apps/web/client/src/env.ts (1)
env(4-133)packages/db/src/schema/project/project.ts (1)
projects(12-29)packages/utility/src/urls.ts (1)
getValidUrl(4-9)packages/utility/src/screenshot.ts (1)
getScreenshotPath(1-4)packages/image-server/src/compress.ts (1)
compressImageServer(6-124)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/db/src/dto/project/project.ts (1)
fromPreviewImg(58-80)
| const result = await app.scrapeUrl(project.sandboxUrl, { | ||
| formats: ['screenshot'], | ||
| onlyMainContent: true, | ||
| actions: [ | ||
| { | ||
| type: 'screenshot', | ||
| fullPage: true, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Firecrawl API shape and add timeout/failure guards
Ensure the scrapeUrl options and response fields are correct; consider adding a request timeout to avoid hanging requests.
Additionally, consider:
-const result = await app.scrapeUrl(project.sandboxUrl, {
+const result = await app.scrapeUrl(project.sandboxUrl, {
formats: ['screenshot'],
onlyMainContent: true,
actions: [
{
type: 'screenshot',
fullPage: true,
},
],
-});
+ timeout: 30000, // if supported by SDK
+});🌐 Web query:
What is the response shape of FirecrawlApp.scrapeUrl from '@mendable/firecrawl-js' when requesting screenshots? Does the options object support `formats: ['screenshot']` and an `actions` array with `{ type: 'screenshot', fullPage: true }`, and does the result include a top-level `screenshot` URL string?
💡 Result:
Short answer: Yes — on the hosted Firecrawl API / official Node SDK you can request screenshots and the response includes URL(s). Concretely:
- You can request a screenshot by adding formats: ['screenshot'] (or formats: ['screenshot@fullPage'] to ask for a full‑page capture). (docs.firecrawl.dev)
- You can also take screenshots via the actions array: { type: 'screenshot', fullPage: true } (actions support a screenshot action with a fullPage boolean). (docs.firecrawl.dev)
- SDK return shape (hosted/cloud): the SDKs return the data object directly. When you request formats: ['screenshot'] you get a top‑level screenshot key (string URL). When you use actions that include screenshots, the response contains an actions object with screenshots as an array of URL strings (e.g. data.actions.screenshots = [ "https://...png" ]). Example shapes are shown in the docs. (docs.firecrawl.dev)
Caveat (important): screenshots are not fully supported in some self‑hosted deployments (and some engines may return data URIs that the API uploads/rewrites to a hosted URL in production). If you self‑host, check the repository/issues or your deployment’s storage configuration — screenshots may fail or require you to provide S3/MinIO storage for hosting images. (github.com)
If you want, I can:
- paste a minimal Node example using FirecrawlApp.scrapeUrl that requests a fullPage screenshot and show how to read the returned URL(s), or
- inspect the exact version of @mendable/firecrawl-js you’re using (or your self‑hosted setup) and confirm the exact runtime shape for that environment. Which would you prefer?
Citations:
- 1: https://docs.firecrawl.dev/advanced-scraping-guide?utm_source=chatgpt.com
- 2: https://docs.firecrawl.dev/advanced-scraping-guide?utm_source=chatgpt.com
- 3: https://docs.firecrawl.dev/features/scrape?utm_source=chatgpt.com
- 4: https://github.com/mendableai/firecrawl/issues/1028?utm_source=chatgpt.com
🏁 Script executed:
#!/bin/bash
# Print lines around the scrapeUrl call to see how `result` is used.
rg -n "scrapeUrl" -A 10 -B 10 apps/web/client/src/server/api/routers/project/project.tsLength of output: 845
Firecrawl scrapeUrl options and response shape confirmed – add timeout & error handling
- The SDK supports both
formats: ['screenshot']and screenshot actions.- With
formats, the response includes a top-levelresult.screenshotURL (string). - With actions, the response includes
result.actions.screenshots, an array of URL strings.
- With
- To avoid hanging requests, add a
timeout(e.g.30000ms) and wrap the call intry/catch. - If you prefer the actions approach, access the URL via
result.actions.screenshots[0].
Suggested diff:
const app = new FirecrawlApp({ apiKey: env.FIRECRAWL_API_KEY });
-
-const result = await app.scrapeUrl(project.sandboxUrl, {
- formats: ['screenshot'],
- onlyMainContent: true,
- actions: [{ type: 'screenshot', fullPage: true }],
-});
+let result;
+try {
+ result = await app.scrapeUrl(project.sandboxUrl, {
+ formats: ['screenshot'], // yields result.screenshot (string)
+ onlyMainContent: true,
+ // or use:
+ // actions: [{ type: 'screenshot', fullPage: true }],
+ timeout: 30000, // prevent hanging requests
+ });
+} catch (err) {
+ throw new Error(`Screenshot capture failed: ${err.message}`);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await app.scrapeUrl(project.sandboxUrl, { | |
| formats: ['screenshot'], | |
| onlyMainContent: true, | |
| actions: [ | |
| { | |
| type: 'screenshot', | |
| fullPage: true, | |
| }, | |
| ], | |
| }); | |
| const app = new FirecrawlApp({ apiKey: env.FIRECRAWL_API_KEY }); | |
| let result; | |
| try { | |
| result = await app.scrapeUrl(project.sandboxUrl, { | |
| formats: ['screenshot'], // yields result.screenshot (string) | |
| onlyMainContent: true, | |
| // or use: | |
| // actions: [{ type: 'screenshot', fullPage: true }], | |
| timeout: 30000, // prevent hanging requests | |
| }); | |
| } catch (err) { | |
| throw new Error(`Screenshot capture failed: ${err.message}`); | |
| } |
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/project/project.ts around lines 59 to
68, the call to app.scrapeUrl currently neither sets a timeout nor handles
errors and also mixes formats vs actions response shapes; wrap the scrapeUrl
call in a try/catch, pass a timeout (e.g. 30000 ms) in the options, and then
read the screenshot URL from the correct shape (either result.screenshot when
using formats, or result.actions.screenshots[0] when using actions) and handle
the case where the URL is missing by returning/logging a meaningful error.
| conversationId, | ||
| content, | ||
| }); | ||
| const title = "RANDOM TITLE"; |
There was a problem hiding this comment.
Hard-coded title used; replace with dynamic title generation (e.g., via api.chat.conversation.generateTitle.mutate) if intended for production.
| const title = "RANDOM TITLE"; | |
| const title = await api.chat.conversation.generateTitle.mutate({ content }); |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/web/client/src/server/api/routers/project/project.ts (4)
37-44: Add rate limiting/cooldown to prevent abuseThis endpoint triggers external scraping + storage; add rate limiting (per-user or per-project) or a cooldown to avoid abuse and cost spikes.
Options:
- Middleware-based rate limit for this procedure (token bucket/redis).
- Per-project cooldown (e.g., lastScreenshotAt column, transactional check + update).
- Fallback: simple in-memory limiter per node with leaky bucket for quick protection (still add a durable limiter later).
I can draft a minimal redis-backed limiter middleware for tRPC if you share your preferred store.
45-56: Add authorization check (project membership) before screenshot captureAnyone with a valid session can target arbitrary project IDs. Ensure the caller is a member of the project before proceeding.
Apply this diff to enforce membership and use TRPC-native errors:
- if (!project) { - throw new Error('Project not found'); - } - - if (!project.sandboxUrl) { - throw new Error('No sandbox URL found'); - } + if (!project) { + throw new TRPCError({ code: 'NOT_FOUND', message: 'Project not found' }); + } + + // Ensure caller has access to this project + const membership = await ctx.db.query.userProjects.findFirst({ + where: and( + eq(userProjects.userId, ctx.user.id), + eq(userProjects.projectId, input.projectId), + ), + }); + if (!membership) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'You do not have access to this project' }); + } + + if (!project.sandboxUrl) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'No sandbox URL found' }); + }Add this import at the top of the file:
import { TRPCError } from '@trpc/server';
69-74: Do not normalize pre-signed screenshot URLs (risk of breaking signatures)getValidUrl may reorder query params and invalidate signatures. Preserve http(s) URLs as-is; only prepend https when missing, then validate the protocol.
- const screenshotUrlRaw = result.screenshot; - const screenshotUrl = screenshotUrlRaw ? getValidUrl(screenshotUrlRaw) : null; - - if (!screenshotUrl) { - throw new Error('Invalid screenshot URL'); - } + const screenshotUrlRaw = result.screenshot; + // Preserve signed URLs exactly; only prepend scheme when missing + const screenshotUrl = typeof screenshotUrlRaw === 'string' + ? (/^https?:\/\//i.test(screenshotUrlRaw) + ? screenshotUrlRaw + : `https://${screenshotUrlRaw.replace(/^\/\//, '')}`) + : null; + if (!screenshotUrl) { + throw new Error('Invalid screenshot URL'); + } + try { + const parsed = new URL(screenshotUrl); + if (!['http:', 'https:'].includes(parsed.protocol)) { + throw new Error('Unsupported URL protocol'); + } + } catch { + throw new Error('Invalid screenshot URL'); + }
76-97: Add fetch timeout, enforce max file size, and sanitize MIME type before deriving file extension
- Network guard: Abort long-running downloads.
- Size guard: Prevent large uploads from consuming storage/memory.
- MIME correctness: Strip parameters (e.g., "; charset=binary") to avoid bad file extensions in getScreenshotPath.
- Compression: Respect compressImageServer.success and log failures.
- const response = await fetch(screenshotUrl); - if (!response.ok) { - throw new Error(`Failed to fetch screenshot: ${response.status} ${response.statusText}`); - } - - const arrayBuffer = await response.arrayBuffer(); - const mimeType = response.headers.get('content-type') ?? 'image/png'; - - const buffer = Buffer.from(arrayBuffer); - - const compressedImage = await compressImageServer(buffer, undefined, { - quality: 80, - width: 1024, - height: 1024, - format: 'jpeg', - }); - - const useCompressed = !!compressedImage.buffer; - const finalMimeType = useCompressed ? 'image/jpeg' : mimeType; - const finalBuffer = useCompressed ? (compressedImage.buffer ?? buffer) : buffer; - - const path = getScreenshotPath(project.id, finalMimeType); + // Fetch with timeout and file-size guard + const controller = new AbortController(); + const abortId = setTimeout(() => controller.abort(), 15_000); + const response = await fetch(screenshotUrl, { signal: controller.signal }); + clearTimeout(abortId); + if (!response.ok) { + throw new Error(`Failed to fetch screenshot: ${response.status} ${response.statusText}`); + } + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB + const clHeader = response.headers.get('content-length'); + if (clHeader) { + const contentLength = Number(clHeader); + if (!Number.isNaN(contentLength) && contentLength > MAX_FILE_SIZE) { + throw new Error(`Screenshot too large: ${(contentLength / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit`); + } + } + + const arrayBuffer = await response.arrayBuffer(); + if (!clHeader && arrayBuffer.byteLength > MAX_FILE_SIZE) { + throw new Error(`Screenshot too large: ${(arrayBuffer.byteLength / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit`); + } + + const responseContentType = response.headers.get('content-type') ?? 'image/png'; + const originalMimeType = responseContentType.split(';')[0].trim(); + const buffer = Buffer.from(arrayBuffer); + + const compressedImage = await compressImageServer(buffer, undefined, { + quality: 80, + width: 1024, + height: 1024, + format: 'jpeg', + }); + if (!compressedImage.success && compressedImage.error) { + console.warn('Image compression failed:', compressedImage.error); + } + + const compressedOk = !!compressedImage.success && !!compressedImage.buffer; + const finalMimeType = compressedOk ? 'image/jpeg' : originalMimeType; + const finalBuffer = compressedOk ? (compressedImage.buffer as Buffer) : buffer; + + const path = getScreenshotPath(project.id, finalMimeType);
🧹 Nitpick comments (2)
apps/web/client/src/server/api/routers/project/project.ts (2)
130-131: Emit analytics event on successful captureAdd an event to track screenshot capture success and compression usage.
- return { success: true, path: data.path }; + trackEvent({ + distinctId: ctx.user.id, + event: 'project_screenshot_captured', + properties: { + projectId: project.id, + path: data.path, + mime: finalMimeType, + compressed: finalMimeType === 'image/jpeg', + }, + }); + return { success: true, path: data.path };
32-33: Remove getValidUrl import if you adopt the signed-URL safe handlingAfter replacing getValidUrl usage, this import becomes unused.
-import { getScreenshotPath, getValidUrl } from '@onlook/utility'; +import { getScreenshotPath } from '@onlook/utility';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/server/api/routers/project/project.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/client/src/server/api/routers/project/project.ts (9)
apps/web/client/src/server/api/trpc.ts (2)
createTRPCRouter(88-88)protectedProcedure(130-149)apps/web/client/src/server/api/routers/project/createRequest.ts (1)
projectCreateRequestRouter(9-33)apps/web/client/src/env.ts (1)
env(4-133)packages/db/src/schema/project/project.ts (1)
projects(12-29)packages/utility/src/urls.ts (1)
getValidUrl(4-9)packages/image-server/src/compress.ts (1)
compressImageServer(6-124)packages/utility/src/screenshot.ts (1)
getScreenshotPath(1-4)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/db/src/dto/project/project.ts (1)
fromPreviewImg(58-80)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
apps/web/client/src/server/api/routers/project/project.ts (5)
75-80: Avoid normalizing pre-signed URLs and add fallback for actions response shape.Using
getValidUrlcan break pre-signed URLs by reordering query parameters. Also handle the alternative response shape when using actions.- const screenshotUrlRaw = result.screenshot; - const screenshotUrl = screenshotUrlRaw ? getValidUrl(screenshotUrlRaw) : null; + const screenshotUrlRaw = + // When using formats: ['screenshot'] + (result as any).screenshot ?? + // Fallback: when using actions with screenshots + (result as any).actions?.screenshots?.[0] ?? + null; + const screenshotUrl = + typeof screenshotUrlRaw === 'string' && /^https?:\/\//i.test(screenshotUrlRaw) + ? screenshotUrlRaw + : typeof screenshotUrlRaw === 'string' + ? `https://${screenshotUrlRaw.replace(/^\/\//, '')}` + : null;
89-103: Add file size validation before uploading.Large screenshots could consume significant storage space. Consider adding size validation.
const arrayBuffer = await response.arrayBuffer(); +const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB +if (arrayBuffer.byteLength > MAX_FILE_SIZE) { + throw new Error(`Screenshot too large: ${(arrayBuffer.byteLength / 1024 / 1024).toFixed(2)}MB exceeds 10MB limit`); +} const mimeType = response.headers.get('content-type') ?? 'image/png'; const buffer = Buffer.from(arrayBuffer);
37-144: Consider implementing rate limiting.The screenshot capture endpoint could be abused since it involves external API calls and storage operations.
Consider implementing rate limiting at the procedure level or add a simple per-project cooldown check:
// Add at the beginning of the mutation, after fetching the project: if (project.updatedPreviewImgAt) { const COOLDOWN_MS = 5 * 60 * 1000; // 5 minutes const timeSinceLastCapture = Date.now() - new Date(project.updatedPreviewImgAt).getTime(); if (timeSinceLastCapture < COOLDOWN_MS) { throw new TRPCError({ code: 'TOO_MANY_REQUESTS', message: `Please wait ${Math.ceil((COOLDOWN_MS - timeSinceLastCapture) / 1000)} seconds before capturing another screenshot` }); } }
59-69: Add error handling for Firecrawl API calls.The scrapeUrl call should be wrapped in try-catch to handle potential network or API failures gracefully.
- const result = await app.scrapeUrl(project.sandboxUrl, { - formats: ['screenshot'], - onlyMainContent: true, - timeout: 10000, - actions: [ - { - type: 'click', - selector: '#btn-answer-yes', - }, - ], - }); + let result; + try { + result = await app.scrapeUrl(project.sandboxUrl, { + formats: ['screenshot'], + onlyMainContent: true, + timeout: 10000, + actions: [ + { + type: 'click', + selector: '#btn-answer-yes', + }, + ], + }); + } catch (err) { + throw new Error(`Screenshot capture failed: ${err instanceof Error ? err.message : 'Unknown error'}`); + }
45-51: Add authorization check to ensure user has access to the project.Any authenticated user could potentially target arbitrary project IDs. Add a membership check before proceeding.
const project = await ctx.db.query.projects.findFirst({ where: eq(projects.id, input.projectId), }); if (!project) { throw new Error('Project not found'); } + +// Ensure the authenticated user has access to this project +const membership = await ctx.db.query.userProjects.findFirst({ + where: and( + eq(userProjects.userId, ctx.user.id), + eq(userProjects.projectId, input.projectId), + ), +}); +if (!membership) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'You do not have access to this project' }); +}apps/web/client/src/components/store/editor/screenshot/index.tsx (4)
8-13: Exclude editorEngine from MobX observability.Observing the entire editor engine tree is unnecessary and may create heavy recursive observable graphs.
- constructor(private editorEngine: EditorEngine) { - makeAutoObservable(this); - } + constructor(private editorEngine: EditorEngine) { + makeAutoObservable(this, { editorEngine: false }, { autoBind: true }); + }
58-58: Reset isCapturing flag in clear() method.If
clear()is called while a capture is in-flight, future captures could be permanently blocked.- clear() { } + clear() { + this._lastScreenshotTime = null; + this.isCapturing = false; + }
24-31: Fix lostthisbinding when passing method to debounce.Passing a method reference to
debounceloses the class context. Convert to arrow function to preservethis.- captureScreenshot = debounce( - this.debouncedCaptureScreenshot, - 5 * 60 * 1000, - { - leading: true, - trailing: false, - }, - ); + captureScreenshot = debounce( + () => this.debouncedCaptureScreenshot(), + 5 * 60 * 1000, + { + leading: true, + trailing: false, + }, + );
33-44: Move isCapturing assignment after cooldown check.The current implementation sets
isCapturing = truebefore checking the cooldown, which could leave it stuck if the function returns early.private async debouncedCaptureScreenshot() { if (this.isCapturing) { return; } - this.isCapturing = true; - try { - // If the screenshot was captured less than 30 minutes ago, skip capturing - if (this.lastScreenshotAt) { - const thirtyMinutesAgo = subMinutes(new Date(), 30); - if (isAfter(this.lastScreenshotAt, thirtyMinutesAgo)) { - return; - } + // If the screenshot was captured less than 30 minutes ago, skip capturing + if (this.lastScreenshotAt) { + const thirtyMinutesAgo = subMinutes(new Date(), 30); + if (isAfter(this.lastScreenshotAt, thirtyMinutesAgo)) { + return; } + } + this.isCapturing = true; + try {
🧹 Nitpick comments (2)
apps/web/client/src/server/api/routers/project/project.ts (2)
41-43: Use TRPCError for consistency.For consistency with the TRPC error handling pattern in your codebase, replace the plain Error with TRPCError.
- if (!env.FIRECRAWL_API_KEY) { - throw new Error('FIRECRAWL_API_KEY is not configured'); - } + if (!env.FIRECRAWL_API_KEY) { + throw new TRPCError({ code: 'INTERNAL_SERVER_ERROR', message: 'FIRECRAWL_API_KEY is not configured' }); + }Add the import at the top of the file:
import { TRPCError } from '@trpc/server';
129-137: Validate database update succeeded.Check that the update was successful to catch potential RLS or race condition failures.
- await ctx.db.update(projects) - .set({ - previewImgUrl: dbPreviewImg.previewImgUrl, - previewImgPath: dbPreviewImg.previewImgPath, - previewImgBucket: dbPreviewImg.previewImgBucket, - updatedPreviewImgAt: new Date(), - updatedAt: new Date(), - }) - .where(eq(projects.id, project.id)); + const result = await ctx.db.update(projects) + .set({ + previewImgUrl: dbPreviewImg.previewImgUrl, + previewImgPath: dbPreviewImg.previewImgPath, + previewImgBucket: dbPreviewImg.previewImgBucket, + updatedPreviewImgAt: new Date(), + updatedAt: new Date(), + }) + .where(eq(projects.id, project.id)) + .returning(); + if (!result || result.length === 0) { + throw new Error('Failed to update project preview metadata'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(3 hunks)apps/web/client/src/components/store/editor/canvas/index.ts(3 hunks)apps/web/client/src/components/store/editor/screenshot/index.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
- apps/web/client/src/components/store/editor/canvas/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/client/src/components/store/editor/screenshot/index.tsx (2)
apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(31-111)apps/web/client/src/trpc/react.tsx (1)
api(23-23)
apps/web/client/src/server/api/routers/project/project.ts (8)
apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)apps/web/client/src/env.ts (1)
env(4-133)packages/db/src/schema/project/project.ts (1)
projects(12-30)packages/utility/src/urls.ts (1)
getValidUrl(4-9)packages/image-server/src/compress.ts (1)
compressImageServer(6-124)packages/utility/src/screenshot.ts (1)
getScreenshotPath(1-4)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/db/src/dto/project/project.ts (1)
fromPreviewImg(60-82)
🔇 Additional comments (1)
apps/web/client/src/components/store/editor/screenshot/index.tsx (1)
46-50: Check the success field properly.Based on the server implementation, the response has a structure
{ success: boolean, path?: string, error?: string }.The current implementation correctly checks both
resultandresult.successbefore proceeding.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/web/client/src/components/store/editor/screenshot/index.tsx (3)
8-10: Encapsulate fields and promote magic numbers to named constants.Improves readability and prevents accidental external mutation. Constants make the debounce/cooldown intent obvious.
- _lastScreenshotTime: Date | null = null; - isCapturing = false; + private _lastScreenshotTime: Date | null = null; + private isCapturing = false; + readonly cooldownMs = 30 * 60 * 1000; // 30 minutes + readonly debounceMs = 5 * 60 * 1000; // 5 minutes
33-56: Critical: debounced method losesthisbinding; convert to an arrow function and move cooldown check before settingisCapturing.Passing a class method to
debouncewithout binding causesthisto be undefined at call time. Also, set the in-flight flag only after cooldown checks to avoid transiently blocking other triggers. This fixes a runtime bug and tightens the control flow.Apply this diff:
- private async debouncedCaptureScreenshot() { - if (this.isCapturing) { - return; - } - this.isCapturing = true; - try { - // If the screenshot was captured less than 30 minutes ago, skip capturing - if (this.lastScreenshotAt) { - const thirtyMinutesAgo = subMinutes(new Date(), 30); - if (isAfter(this.lastScreenshotAt, thirtyMinutesAgo)) { - return; - } - } - const result = await api.project.captureScreenshot.mutate({ projectId: this.editorEngine.projectId }); - if (!result || !result.success) { - throw new Error('Failed to capture screenshot'); - } - this.lastScreenshotAt = new Date(); - } catch (error) { - console.error('Error capturing screenshot', error); - } finally { - this.isCapturing = false; - } - } + private debouncedCaptureScreenshot = async () => { + // Skip if another capture is in-flight + if (this.isCapturing) return; + + // Respect cooldown window first + if (this.lastScreenshotAt) { + const cutoff = new Date(Date.now() - this.cooldownMs); + if (isAfter(this.lastScreenshotAt, cutoff)) return; + } + + this.isCapturing = true; + try { + const result = await api.project.captureScreenshot.mutate({ + projectId: this.editorEngine.projectId, + }); + if (!result || !result.success) { + const reason = (result as any)?.error ?? 'Failed to capture screenshot'; + throw new Error(String(reason)); + } + this.lastScreenshotAt = new Date(); + } catch (error) { + console.error('Error capturing screenshot', error); + } finally { + this.isCapturing = false; + } + }
11-13: ExcludeeditorEnginefrom MobX observability and enable auto-binding.Avoids creating a large/recursive observable graph and ensures methods are safely bound for callbacks.
- makeAutoObservable(this); + // Avoid observing the entire editor engine; bind methods for safe callbacks + makeAutoObservable(this, { editorEngine: false }, { autoBind: true });
🧹 Nitpick comments (5)
apps/web/client/src/components/store/editor/screenshot/index.tsx (5)
24-31: Use the named debounce constant instead of a magic number.Keeps timing configuration centralized and self-documenting.
- this.debouncedCaptureScreenshot, - 5 * 60 * 1000, + this.debouncedCaptureScreenshot, + this.debounceMs,
58-60: Cancel the debounced timer and reset in-flight state on clear().Prevents stale timers from affecting subsequent projects and avoids a blocked state after reset.
- clear() { - this.lastScreenshotAt = null; - } + clear() { + this.captureScreenshot.cancel?.(); + this.isCapturing = false; + this.lastScreenshotAt = null; + }
3-3: Reduce bundle size: importdebouncevia path import.Named import from
lodashcan pull in more code than necessary depending on bundler settings. Using the direct path helps tree-shaking.-import { debounce } from 'lodash'; +import debounce from 'lodash/debounce';
2-2: Remove unusedsubMinutesimport (after adopting cooldownMs).Keeps imports tidy and avoids dead code.
-import { isAfter, subMinutes } from 'date-fns'; +import { isAfter } from 'date-fns';
15-21: Nit: rename backing field for consistency.Getter/setter are
lastScreenshotAtbut the field is_lastScreenshotTime. Consider renaming to_lastScreenshotAtfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/client/src/components/store/editor/screenshot/index.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/server/api/routers/project/project.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/client/src/components/store/editor/screenshot/index.tsx (3)
apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(31-111)apps/web/client/src/trpc/react.tsx (1)
api(23-23)apps/studio/src/lib/editor/engine/canvas/index.ts (2)
CanvasManager(14-153)undebouncedSaveSettings(141-152)
🔇 Additional comments (1)
apps/web/client/src/components/store/editor/screenshot/index.tsx (1)
46-51: LGTM: success-check before timestamp update is correct.Only updating
lastScreenshotAton a successful server response keeps the cooldown logic accurate.
Kitenite
changed the title
Kitenite
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/web/client/src/components/store/editor/screenshot/index.tsx (2)
29-52: Fix lostthisbinding in debounced method (runtime bug)Passing a class method to debounce without binding will lose
thisand break access to instance state. Define debouncedCaptureScreenshot as an arrow function (lexically bound) to keepthis.Apply this diff:
- private async debouncedCaptureScreenshot() { + private debouncedCaptureScreenshot = async () => { if (this.isCapturing) { return; } this.isCapturing = true; try { // If the screenshot was captured less than 30 minutes ago, skip capturing if (this.lastScreenshotAt) { - const thirtyMinutesAgo = subMinutes(new Date(), 30); + const thirtyMinutesAgo = subMinutes(new Date(), 30); if (isAfter(this.lastScreenshotAt, thirtyMinutesAgo)) { return; } } const result = await api.project.captureScreenshot.mutate({ projectId: this.editorEngine.projectId }); if (!result || !result.success) { - throw new Error('Failed to capture screenshot'); + throw new Error('Failed to capture screenshot'); } this.lastScreenshotAt = new Date(); } catch (error) { console.error('Error capturing screenshot', error); } finally { this.isCapturing = false; } - } + }
23-27: Align debounce to intended UX and make behavior explicitThe PR mentions a 5-minute debounce; code uses 10 seconds and default lodash options (trailing only). If you want immediate capture attempts then suppress further triggers during the window, use leading: true, trailing: false and 5 minutes.
- // 10 second debounce - captureScreenshot = debounce( - this.debouncedCaptureScreenshot, - 10000, - ); + // 5-minute debounce; fire immediately, then ignore subsequent triggers during the window + captureScreenshot = debounce(this.debouncedCaptureScreenshot, 5 * 60 * 1000, { + leading: true, + trailing: false, + });
🧹 Nitpick comments (4)
apps/web/client/src/components/store/editor/screenshot/index.tsx (4)
11-13: Exclude editorEngine from MobX observability and enable autoBindAvoid observing the entire editor engine tree and bind methods for safe callbacks; this also guards against accidental “this” loss for non-arrow methods.
- constructor(private editorEngine: EditorEngine) { - makeAutoObservable(this); - } + constructor(private editorEngine: EditorEngine) { + // Do not observe the engine; auto-bind methods for safe callbacks + makeAutoObservable(this, { editorEngine: false }, { autoBind: true }); + }
35-41: Promote magic numbers to named constants (cooldown/debounce)Replace hard-coded 30-minute and 5-minute values with named properties for clarity and maintainability.
export class ScreenshotManager { _lastScreenshotTime: Date | null = null; isCapturing = false; + readonly debounceMs = 5 * 60 * 1000; // 5 minutes + readonly cooldownMinutes = 30; // 30 minutes constructor(private editorEngine: EditorEngine) { makeAutoObservable(this); } @@ - // 10 second debounce - captureScreenshot = debounce( - this.debouncedCaptureScreenshot, - 10000, - ); + // Debounce configured via class property + captureScreenshot = debounce(this.debouncedCaptureScreenshot, this.debounceMs, { + leading: true, + trailing: false, + }); @@ - if (this.lastScreenshotAt) { - const thirtyMinutesAgo = subMinutes(new Date(), 30); + if (this.lastScreenshotAt) { + const thirtyMinutesAgo = subMinutes(new Date(), this.cooldownMinutes); if (isAfter(this.lastScreenshotAt, thirtyMinutesAgo)) { return; } }
42-47: Include server error details before throwing to aid diagnosisWhen the API returns { success: false, error }, surface the error message for observability.
- const result = await api.project.captureScreenshot.mutate({ projectId: this.editorEngine.projectId }); - if (!result || !result.success) { - throw new Error('Failed to capture screenshot'); - } + const result = await api.project.captureScreenshot.mutate({ projectId: this.editorEngine.projectId }); + if (!result || !result.success) { + const msg = typeof result?.error === 'string' ? result!.error : JSON.stringify(result?.error); + throw new Error(`Failed to capture screenshot${msg ? `: ${msg}` : ''}`); + }
1-3: Consider importing debounce from a path that enables better tree-shakingImporting from 'lodash/debounce' (or 'lodash-es' in ESM-only contexts) can cut bundle size versus the root 'lodash' import.
-import { debounce } from 'lodash'; +import debounce from 'lodash/debounce';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/web/client/src/components/store/editor/screenshot/index.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(4 hunks)packages/db/src/dto/project/project.ts(4 hunks)packages/db/src/schema/project/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/dto/project/project.ts
- apps/web/client/src/server/api/routers/project/project.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/db/src/schema/project/project.ts (1)
packages/models/src/projects/index.ts (1)
Project(1-10)
apps/web/client/src/components/store/editor/screenshot/index.tsx (2)
apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(31-111)apps/web/client/src/trpc/react.tsx (1)
api(23-23)
🔇 Additional comments (1)
packages/db/src/schema/project/project.ts (1)
25-25: LGTM: nullable updatedPreviewImgAt without default prevents false positivesMaking updatedPreviewImgAt nullable and removing any default avoids stamping new projects with “now” when no screenshot exists. This aligns with the cooldown logic and initial capture behavior.
| clear() { | ||
| this.lastScreenshotAt = null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cancel pending debounce and reset state on clear()
Ensure pending debounced calls are cancelled and in-flight state is reset so future captures aren’t blocked.
clear() {
this.lastScreenshotAt = null;
+ // Cancel any pending debounced invocation and reset in-flight state
+ this.captureScreenshot.cancel?.();
+ this.isCapturing = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clear() { | |
| this.lastScreenshotAt = null; | |
| } | |
| clear() { | |
| this.lastScreenshotAt = null; | |
| // Cancel any pending debounced invocation and reset in-flight state | |
| this.captureScreenshot.cancel?.(); | |
| this.isCapturing = false; | |
| } |
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/screenshot/index.tsx around lines
54 to 56, the clear() method only nulls lastScreenshotAt but does not cancel
pending debounced capture calls or reset any in-flight flags; update clear() to
cancel the debounce timer or debounced function (e.g. clearTimeout or call
.cancel() on the debounced function) and reset any capture state flags (e.g.
this.isCapturing = false, this.pendingCapture = null) before setting
this.lastScreenshotAt = null so future captures are not blocked.
| @@ -22,6 +22,7 @@ export const projects = pgTable('projects', { | |||
| previewImgUrl: varchar('preview_img_url'), | |||
| previewImgPath: varchar('preview_img_path'), | |||
| previewImgBucket: varchar('preview_img_bucket'), | |||
| updatedPreviewImgAt: timestamp('updated_preview_img_at', { withTimezone: true }), | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify migration and public model exposure for updatedPreviewImgAt
Confirm there’s a migration adding updated_preview_img_at without a default and that any previous default was dropped and backfilled (NULL where no preview exists). Also ensure all public models/interfaces expose the field consistently. The provided snippet packages/models/src/projects/index.ts appears not to include this field, which could cause typing mismatches.
Run this script to verify:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for migration(s) adding updated_preview_img_at and dropping defaults ==="
rg -n "updated_preview_img_at|ALTER TABLE .* updated_preview_img_at|DROP DEFAULT" -A 3 -S
echo
echo "=== Check for any defaults still defined in schema/migrations ==="
rg -n "updated_preview_img_at.*default" -S
echo
echo "=== Confirm model surfaces expose updatedPreviewImgAt ==="
echo "--- Models referencing updatedPreviewImgAt ---"
rg -n "updatedPreviewImgAt" -S packages
echo
echo "=== Potential older Project interfaces missing the field ==="
fd -t f -a "index.ts" packages/models | xargs rg -n "interface Project|updatedPreviewImgAt" -n -A 5 -SLength of output: 1711
Add migration and expose updatedPreviewImgAt in public models
It looks like there’s no migration in apps/backend/supabase/migrations/ that actually adds the updated_preview_img_at column (and drops any old default/backfills NULL), and our public Project models/interfaces haven’t been updated to expose updatedPreviewImgAt. Please:
- Create a new migration under
apps/backend/supabase/migrations/to:- ALTER TABLE projects ADD COLUMN
updated_preview_img_attimestamptz; - Drop any old default if it existed
- Backfill existing rows with NULL (or appropriate timestamp)
- ALTER TABLE projects ADD COLUMN
- Verify the schema in
packages/db/src/schema/project/project.tsstays in sync. - Update the Project types in your public models (e.g.
packages/models/src/projects/index.ts), addingupdatedPreviewImgAt?: Date | nullwherever the Project interface is defined or mapped. - Run your type-generation or REST/GraphQL codegen to ensure there are no missing-field errors.
This will guarantee the database, internal schema, and public interfaces remain consistent.
🤖 Prompt for AI Agents
In packages/db/src/schema/project/project.ts around line 25 the new column
updatedPreviewImgAt (updated_preview_img_at) is defined in the schema but there
is no corresponding DB migration nor public model exposure; create a new
migration file in apps/backend/supabase/migrations/ that ALTERs TABLE projects
ADD COLUMN updated_preview_img_at timestamptz, removes any old default, and
backfills existing rows with NULL (or an appropriate timestamp), keep
packages/db/src/schema/project/project.ts unchanged if it already matches the
column, then update the public Project types in
packages/models/src/projects/index.ts (and any mapped interfaces) to include
updatedPreviewImgAt?: Date | null, and finally run your type-generation /
REST/GraphQL codegen to ensure no missing-field errors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/db/src/dto/project/project.ts (1)
60-76: Do not set updatedPreviewImgAt when no valid preview was provided.
fromPreviewImgunconditionally sets a timestamp (Line 88) even when neither a valid URL nor valid storage path/bucket is present. This can incorrectly signal a refreshed preview image. Move the timestamp assignment into the branches where a valid preview is established.Apply this diff:
if (!previewImg) { return res; } if (previewImg.type === 'url' && previewImg.url) { res.previewImgUrl = previewImg.url; + res.updatedPreviewImgAt = new Date(); } else if (previewImg.type === 'storage' && previewImg.storagePath && previewImg.storagePath.path && previewImg.storagePath.bucket) { res.previewImgPath = previewImg.storagePath.path; res.previewImgBucket = previewImg.storagePath.bucket; + res.updatedPreviewImgAt = new Date(); } - res.updatedPreviewImgAt = new Date(); return res;Also applies to: 82-89
apps/web/client/src/server/api/routers/project/project.ts (5)
41-43: Use TRPCError for config issues to keep error semantics consistent.Throwing a plain Error here breaks tRPC error typing and handling downstream.
Apply this diff within the mutation:
- if (!env.FIRECRAWL_API_KEY) { - throw new Error('FIRECRAWL_API_KEY is not configured'); - } + if (!env.FIRECRAWL_API_KEY) { + throw new TRPCError({ code: 'INTERNAL_SERVER_ERROR', message: 'FIRECRAWL_API_KEY is not configured' }); + }And add the import at the top of this file (outside this range):
import { TRPCError } from '@trpc/server';
45-56: Missing authorization check: ensure the caller has access to the project.Any authenticated user could target arbitrary project IDs if RLS doesn’t block it. Add an explicit membership check.
Apply this diff:
const project = await ctx.db.query.projects.findFirst({ where: eq(projects.id, input.projectId), }); if (!project) { throw new Error('Project not found'); } + // Ensure the authenticated user has access to this project + const membership = await ctx.db.query.userProjects.findFirst({ + where: and( + eq(userProjects.userId, ctx.user.id), + eq(userProjects.projectId, input.projectId), + ), + }); + if (!membership) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'You do not have access to this project' }); + }
59-71: Consider per-project cooldown to rate-limit screenshot captures.This endpoint can be abused and also flood Firecrawl/storage. Use
updatedPreviewImgAtor a dedicated timestamp to enforce a cooldown (e.g., 5 minutes).Minimal example (place after loading the project and membership check):
+ // Simple per-project cooldown (e.g., 5 minutes) + const COOLDOWN_MS = 5 * 60 * 1000; + if (project.updatedPreviewImgAt && (Date.now() - project.updatedPreviewImgAt.getTime()) < COOLDOWN_MS) { + return { success: false, error: 'Screenshot recently updated. Please try again later.' }; + }If you prefer request-level rate limiting, apply middleware at the router level instead.
89-103: Add screenshot size validation before upload.Large images can bloat storage and memory usage. Enforce a max size.
Apply this diff:
const arrayBuffer = await response.arrayBuffer(); const mimeType = response.headers.get('content-type') ?? 'image/png'; const buffer = Buffer.from(arrayBuffer); + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB + if (buffer.byteLength > MAX_FILE_SIZE) { + throw new Error(`Screenshot too large: ${(buffer.byteLength / (1024 * 1024)).toFixed(2)}MB exceeds 10MB limit`); + }Optional: Pre-check response.headers.get('content-length') to fail fast when present.
133-141: Validate DB update result to catch RLS/race-condition failures.Silently ignoring a failed update hides issues. Capture and validate the update.
Apply this diff:
- await ctx.db.update(projects) - .set({ - previewImgUrl, - previewImgPath, - previewImgBucket, - updatedPreviewImgAt, - updatedAt: new Date(), - }) - .where(eq(projects.id, project.id)); + const [updated] = await ctx.db.update(projects) + .set({ + previewImgUrl, + previewImgPath, + previewImgBucket, + updatedPreviewImgAt, + updatedAt: new Date(), + }) + .where(eq(projects.id, project.id)) + .returning(); + if (!updated) { + throw new Error('Failed to update project preview metadata'); + }
🧹 Nitpick comments (1)
apps/web/client/src/server/api/routers/project/project.ts (1)
82-87: Nit: Add Accept header and slightly longer timeout margin.Helps content negotiation and avoids servers defaulting to HTML responses.
Apply this diff:
- const response = await fetch(screenshotUrl, { - signal: AbortSignal.timeout(10000), - }); + const response = await fetch(screenshotUrl, { + signal: AbortSignal.timeout(20_000), + headers: { accept: 'image/*' }, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/client/src/server/api/routers/project/project.ts(4 hunks)packages/db/src/dto/project/project.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/db/src/dto/project/project.ts (2)
packages/db/src/schema/project/project.ts (1)
Project(57-57)packages/models/src/project/project.ts (2)
Project(1-15)PreviewImg(17-24)
apps/web/client/src/server/api/routers/project/project.ts (7)
apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)apps/web/client/src/env.ts (1)
env(4-133)packages/db/src/schema/project/project.ts (1)
projects(12-30)packages/image-server/src/compress.ts (1)
compressImageServer(6-124)packages/utility/src/screenshot.ts (1)
getScreenshotPath(1-4)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/models/src/projects/index.ts (1)
Project(1-10)
🔇 Additional comments (4)
packages/db/src/dto/project/project.ts (2)
19-20: Mapping looks correct (type alignment).Passing
dbProject.updatedPreviewImgAtthrough to the domain model matches the interface (Date | null) in packages/models. No change needed.
24-38: End-to-end propagation is consistent.
fromProjectnow returnsupdatedPreviewImgAtviafromPreviewImg(...)and writes it onto the DbProject. This is coherent with the schema changes.apps/web/client/src/server/api/routers/project/project.ts (2)
333-336: LGTM: update now bumps updatedAt consistently.This keeps project sorting and freshness accurate after updates.
3-3: FirecrawlApp default import is correct
The@mendable/firecrawl-jspackage exportsFirecrawlAppas its default export (per official docs and source), so the existingimport FirecrawlApp from '@mendable/firecrawl-js';statement is valid. No changes needed.
* 'main' of https://github.com/onlook-dev/onlook: (41 commits) feat: fix delete project (onlook-dev#2683) feat: reduce search latency (onlook-dev#2682) feat: revamp projects ux (onlook-dev#2672) Upload fix (onlook-dev#2677) feat: using firecrawl to screenshot (onlook-dev#2665) fix: file upload build error (onlook-dev#2674) feat: new project frames (onlook-dev#2673) feat: self hosting docs (onlook-dev#2671) fix: docs layout + troubleshoot section (onlook-dev#2670) fix(template): suppress hydration mismatch Feat/editor improvements (onlook-dev#2601) fix: make message content selectable (onlook-dev#2664) bugs: transient stream error (onlook-dev#2663) feat: update migration files (onlook-dev#2659) Start from Blank + Import buttons on the home page (onlook-dev#2653) fix: snapshot type conflict and mastra can't view image (onlook-dev#2656) fix: typecheck snapshot (onlook-dev#2655) feat: clean up mastra branch (onlook-dev#2654) feat: mastra no storage (onlook-dev#2650) feat: update ai packages (onlook-dev#2648) ...
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Screen.Recording.2025-08-13.at.20.17.12.mov
Additional Notes
Important
Introduces server-side project screenshot capture using Firecrawl, updates database schema, and removes client-side screenshot upload flow.
ScreenshotManagerinscreenshot/index.tsxfor capturing project screenshots using Firecrawl.captureScreenshotmutation inprojectRouterinproject.tsto handle server-side screenshot capture.updatedPreviewImgAtfield toprojectstable inproject.tsto store last screenshot update timestamp.canvasRouterand related client-side screenshot upload flow.userCanvasRouterinuser-canvas.tsto includegetWithFramesquery.captureScreenshotinsandbox.test.tsto test screenshot functionality.Toastercomponent insonner.tsxto position notifications at the bottom-left.This description was created by
for e2e1768. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Chores
Tests