Update: use ts and bun
#9
Conversation
WalkthroughThis PR migrates the project to the Bun runtime and introduces TypeScript. It replaces the Docker base image and install flow with Bun, adds Bun/TypeScript tooling (tsconfig, eslint changes, devDependencies), and replaces the server with a Bun-based Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Files/areas to pay extra attention to:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
20-20: Update packageManager field to reference Bun.The
packageManagerfield in package.json should be updated from"pnpm@10.20.0"to"bun@<version>"(or just"bun") to align with the PR's migration to Bun. Bun supports the packageManager field and expects it to be set to "bun" or "bun@", with version pinning being recommended.
🧹 Nitpick comments (10)
src/config/browser.ts (2)
13-18: Consider adding graceful shutdown for browser resources.The browser is launched at module initialization but there's no cleanup mechanism on application shutdown. This could lead to resource leaks or zombie Chromium processes.
Consider registering a cleanup handler in the server startup file:
// In src/server.ts or main entry point process.on("SIGTERM", async () => { await browser.close(); process.exit(0); }); process.on("SIGINT", async () => { await browser.close(); process.exit(0); });
11-18: Consider structured logging instead of console statements.The console.log statements for lifecycle events could be replaced with a structured logger for better observability in production.
src/server.ts (1)
9-35: Server implementation looks functional.The manual route matching approach is straightforward for the current four endpoints. Consider extracting route definitions into a map if the number of routes grows significantly.
If routes expand beyond 5-6 endpoints, consider refactoring to a route map pattern:
const routes = { 'POST /v1/screenshots': handleScreenshotsRequest, 'POST /v1/reports': handleReportsRequest, 'GET /v1/health': handleHealthRequest, 'GET /v1/test': handleTestRequest, } as const; const key = `${req.method} ${path}`; const handler = routes[key]; if (handler) return await handler(req);src/routes/test.ts (1)
10-12: Misleading variable name.The variable
htmlcontains a timestamp string, not HTML. Consider renaming totimestampfor clarity.- const html = await page.evaluate(() => { + const timestamp = await page.evaluate(() => { return new Date().toISOString(); }); await context.close(); - return new Response(generateTestHTML(html), { + return new Response(generateTestHTML(timestamp), { headers: { "Content-Type": "text/html" }, });src/routes/reports.ts (1)
82-88: Consider distinguishing client errors from server errors.All errors return status 400 (Bad Request), including server-side failures like browser crashes or network timeouts. Consider returning 500 for internal errors.
} catch (error) { + // Distinguish validation errors from server errors + const isValidationError = error instanceof Error && + error.name === 'ZodError'; + const status = isValidationError ? 400 : 500; + const message = error instanceof Error ? error.message : "Unknown error"; return new Response(JSON.stringify({ error: message }), { - status: 400, + status, headers: { "Content-Type": "application/json" }, }); }src/routes/screenshots.ts (2)
40-53: Set up route handler only when headers are provided.The route handler is registered unconditionally, even when
body.headersis not provided. This adds unnecessary overhead for the common case where custom headers aren't needed.- // Override headers - await page.route("**/*", async (route, request) => { - const url = request.url(); - if (url.startsWith("http://appwrite/")) { - return await route.continue({ - headers: { - ...request.headers(), - ...(body.headers || {}), - }, - }); - } - - return await route.continue({ headers: request.headers() }); - }); + // Override headers if provided + if (body.headers) { + await page.route("**/*", async (route, request) => { + const url = request.url(); + if (url.startsWith("http://appwrite/")) { + return await route.continue({ + headers: { + ...request.headers(), + ...body.headers, + }, + }); + } + return await route.continue({ headers: request.headers() }); + }); + }
88-94: Consider distinguishing client errors from server errors.All errors return status 400 (Bad Request), including server-side failures. Consider returning 500 for internal errors to help clients distinguish between request issues and server issues.
} catch (error) { + // Distinguish validation errors from server errors + const isValidationError = error instanceof Error && + error.name === 'ZodError'; + const status = isValidationError ? 400 : 500; + const message = error instanceof Error ? error.message : "Unknown error"; return new Response(JSON.stringify({ error: message }), { - status: 400, + status, headers: { "Content-Type": "application/json" }, }); }src/schemas/screenshot.schema.ts (1)
6-6: Tighten header value validation for better type safety.Using
z.any()for header values bypasses Zod's type safety. HTTP header values are typically strings, numbers, or booleans (which are coerced to strings).Apply this diff to improve type safety:
- headers: z.record(z.string(), z.any()).optional(), + headers: z.record(z.string(), z.union([z.string(), z.number(), z.boolean()])).optional(),src/schemas/lighthouse.schema.ts (2)
11-11: Tighten header value validation for better type safety.Using
z.any()for header values bypasses Zod's type safety. HTTP header values are typically strings, numbers, or booleans.Apply this diff to improve type safety:
- headers: z.record(z.string(), z.any()).optional(), + headers: z.record(z.string(), z.union([z.string(), z.number(), z.boolean()])).optional(),
3-73: Consider extracting shared schema fragments to reduce duplication.Both
lighthouseSchemaandscreenshotSchemashare identical definitions forheaders,timezoneId,permissions,waitUntil, andtimeout. Consider extracting these into a shared base schema or reusable Zod objects to maintain consistency and reduce duplication.For example, create a
src/schemas/common.schema.ts:import { z } from "zod"; export const headersSchema = z.record(z.string(), z.union([z.string(), z.number(), z.boolean()])).optional(); export const timezoneIdSchema = z .string() .regex( /^(Africa|America|Antarctica|Arctic|Asia|Atlantic|Australia|Europe|Indian|Pacific|UTC)(\/[A-Za-z_]+)+$/, "Must be a valid IANA timezone identifier" ) .optional(); export const permissionsSchema = z.array(z.enum([ "geolocation", "camera", "microphone", /* ... */ ])).optional(); export const navigationOptionsSchema = z.object({ waitUntil: z.enum(["load", "domcontentloaded", "networkidle", "commit"]).default("domcontentloaded"), timeout: z.number().min(0).max(120000).default(30000), });Then import and use in both schema files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.dockerignore(1 hunks).gitignore(1 hunks)Dockerfile(2 hunks)eslint.config.js(1 hunks)package.json(2 hunks)src/config/browser.ts(1 hunks)src/config/index.ts(1 hunks)src/config/lighthouse.ts(1 hunks)src/routes/health.ts(1 hunks)src/routes/index.ts(1 hunks)src/routes/reports.ts(1 hunks)src/routes/screenshots.ts(1 hunks)src/routes/test.ts(1 hunks)src/schemas/index.ts(1 hunks)src/schemas/lighthouse.schema.ts(1 hunks)src/schemas/screenshot.schema.ts(1 hunks)src/server.ts(1 hunks)src/utils/test-page.ts(6 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/routes/reports.ts (3)
src/schemas/lighthouse.schema.ts (1)
lighthouseSchema(3-73)src/config/browser.ts (2)
defaultContext(4-9)browser(13-16)src/config/lighthouse.ts (1)
lighthouseConfigs(4-7)
src/routes/screenshots.ts (2)
src/schemas/screenshot.schema.ts (1)
screenshotSchema(3-90)src/config/browser.ts (2)
defaultContext(4-9)browser(13-16)
src/routes/test.ts (2)
src/config/browser.ts (2)
browser(13-16)defaultContext(4-9)src/utils/test-page.ts (1)
generateTestHTML(1-326)
src/server.ts (4)
src/routes/screenshots.ts (1)
handleScreenshotsRequest(8-95)src/routes/reports.ts (1)
handleReportsRequest(6-89)src/routes/health.ts (1)
handleHealthRequest(3-12)src/routes/test.ts (1)
handleTestRequest(4-19)
src/routes/health.ts (1)
src/config/browser.ts (1)
browser(13-16)
🔇 Additional comments (12)
eslint.config.js (1)
1-29: LGTM! Solid TypeScript ESLint configuration.The TypeScript integration is well-configured with appropriate rules and the Bun global properly declared as readonly. The underscore ignore pattern for unused variables is a good convention.
src/schemas/index.ts (1)
1-2: LGTM! Standard schema re-export pattern.The use of
.jsextensions in imports follows the Node.js ESM convention for TypeScript and is compatible with Bun's bundler module resolution.src/routes/health.ts (1)
3-12: LGTM! Clean health check implementation.The health check logic is straightforward and correctly uses
browser.isConnected()to report status. The underscore prefix on the unused_reqparameter follows the ESLint configuration.src/config/lighthouse.ts (1)
1-7: LGTM!The Lighthouse configuration aggregator is cleanly structured. The
as constassertion ensures type safety for the viewport keys used in route handlers.src/config/index.ts (1)
1-4: LGTM!The configuration index cleanly aggregates config modules and handles port resolution correctly. The
.jsextensions in imports are correct for TypeScript ESM module resolution.src/routes/index.ts (1)
1-4: LGTM!Clean barrel export pattern for route handlers.
src/utils/test-page.ts (1)
1-326: LGTM!The HTML generator provides comprehensive browser configuration testing. The use of
innerHTMLwith error messages and API data is safe since all values originate from browser APIs rather than user input.src/schemas/screenshot.schema.ts (1)
3-90: Comprehensive schema design with sensible defaults.The schema provides thorough validation coverage for screenshot requests with appropriate:
- Range constraints (viewport 1-3840x1-2160, quality 0-100, sleep 0-60s, timeout 0-120s)
- Sensible defaults (1280x720 viewport, PNG format, 3s sleep, domcontentloaded wait)
- Extensive permission allowlist covering modern browser APIs
- Proper geolocation bounds (±90 lat, ±180 long)
Dockerfile (3)
12-25: Good practice: Chromium installation with size optimization.The Dockerfile appropriately installs Chromium with necessary fonts and removes unnecessary files to reduce image size. The cleanup of crash handlers, resource paks, and sandbox binaries is a solid optimization.
27-34: Proper security and runtime configuration.Good practices implemented:
- Non-root user (
chrome) for improved security- Playwright environment variables correctly configured
tinias PID 1 for proper signal handling
1-1: Checking for known security vulnerabilities in Bun 1.3.2:No action required—Bun version is current and secure.
Bun 1.3.2 is the latest available version, released November 8, 2025. No known critical vulnerabilities specific to this version have been disclosed. The alpine variant minimizes base OS attack surface.
src/schemas/lighthouse.schema.ts (1)
59-67: Well-designed Lighthouse performance thresholds.The thresholds object provides comprehensive coverage of Lighthouse metrics (performance, accessibility, best-practices, SEO, PWA) with appropriate 0-100 ranges and sensible defaults of 0 (no threshold enforcement by default).
Co-authored-by: Matej Bačo <matejbaco2000@gmail.com>
|
|
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Chores
Documentation