Feat add more configs #7
Conversation
…nshots and reports, including viewport, geolocation, permissions, and custom thresholds. Update usage examples and improve validation for timezone identifiers.
- Introduced a new GET endpoint `/v1/test` to display current browser configuration values in an HTML format. - Enhanced the README with detailed usage instructions, response format, and various test command examples for different scenarios, including mobile and high-quality desktop tests.
WalkthroughThis pull request expands README documentation (features, quick start, API usage, timezone and permissions examples, contributing) and extends the server payload schemas and handlers. The screenshot and lighthouse schemas now include headers, viewport, format/quality, clip, device flags, browser context options (userAgent, locale, timezoneId, geolocation, permissions), and navigation options (waitUntil, timeout). v1/screenshots and v1/reports are updated to merge and apply these options (context creation, header overrides, navigation, screenshot capture, dynamic thresholds). A new v1/test route renders an HTML page showing detected browser configuration values. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/index.js (5)
141-159: Set correct Content-Type for screenshot responses.You return a raw Buffer but never set the image Content-Type, so clients may treat it as JSON/octet-stream.
Apply headers before returning, mapping type → MIME:
- const screen = await page.screenshot(screenshotOptions); + const screen = await page.screenshot(screenshotOptions); + const mime = { + png: 'image/png', + jpeg: 'image/jpeg', + webp: 'image/webp', + }[screenshotOptions.type] || 'application/octet-stream'; + event.node.res.setHeader('Content-Type', mime); + event.node.res.setHeader('Content-Length', String(screen.length)); // await context.tracing.stop({ path: '/tmp/trace' + Date.now() + '.zip' }); await context.close(); return screen;Also applies to: 176-182
142-151: Headers are only applied to http://appwrite/*; docs imply they should apply to target origin.Users expect headers to affect requests to the page’s origin, not just an internal host.
Scope overrides to body.url origin:
- await page.route("**/*", async (route, request) => { - const url = request.url(); - if (url.startsWith("http://appwrite/")) { + const targetOrigin = new URL(body.url).origin; + await page.route("**/*", async (route, request) => { + const reqOrigin = new URL(request.url()).origin; + if (reqOrigin === targetOrigin) { return await route.continue({ headers: { ...request.headers(), - ...(body.headers || {}), + ...(body.headers ? Object.fromEntries(Object.entries(body.headers).map(([k,v]) => [k, String(v)])) : {}), }, }); } return await route.continue({ headers: request.headers() }); });Apply the same logic in /v1/reports.
295-316: Avoid hardcoding Lighthouse CDP port and improve robustness.Port 9222 is fixed; conflicts or multiple instances will break, and the DevTools port may be reachable from the network.
- Bind DevTools to loopback and allow configurable port.
- Reuse the same variable in launch and playAudit.
-const browser = await chromium.launch({ - args: ["--remote-debugging-port=9222"], -}); +const cdpPort = Number(process.env.CDP_PORT ?? 9222); +const browser = await chromium.launch({ + args: [`--remote-debugging-address=127.0.0.1`,`--remote-debugging-port=${cdpPort}`], +}); ... - port: 9222, + port: cdpPort,Consider
CDP_PORT=0with a discovery step ifplaywright-lighthousesupports it; otherwise document the env.
20-25: Add lifecycle handling and error guard for the global browser.A failed launch or missing graceful shutdown will crash/hang containers on signals/restarts.
Add minimal guards:
-console.log("Chromium starting..."); -const browser = await chromium.launch({ +console.log("Chromium starting..."); +let browser; +try { + browser = await chromium.launch({ args: ["--remote-debugging-port=9222"], -}); + }); +} catch (err) { + console.error("Chromium failed to start:", err); + process.exit(1); +} +for (const sig of ["SIGINT","SIGTERM"]) { + process.on(sig, async () => { + try { if (browser?.isConnected()) await browser.close(); } finally { process.exit(0); } + }); +} console.log("Chromium started!");Adjust with the cdpPort/address change above.
1-16: Runpnpm biome format ./srcto fix formatting throughout the file.Biome check confirms pending formatting changes across src/index.js. The main issues are systematic method-chain formatting in zod schema definitions (breaking chains onto separate lines, restructuring method calls). While the imports at lines 1-16 appear compliant, merging
readValidatedBodyinto the first h3 import would improve consistency:import { createApp, createRouter, defineEventHandler, toNodeListener, readValidatedBody, } from "h3";
🧹 Nitpick comments (5)
src/index.js (3)
36-108: Add schema-level refinement: prevent clip+fullPage and validate viewport clip bounds.Avoid runtime 400s where possible.
Example Zod refinements:
-const screenshotSchema = z.object({ +const screenshotSchema = z.object({ ... -}).optional(), +}).refine((data) => !(data.clip && data.fullPage), { + message: 'clip cannot be used with fullPage', + path: ['clip'], +}).refine((data) => { + if (!data.clip || !data.viewport) return true; + const { x,y,width,height } = data.clip; + const { width: vw, height: vh } = data.viewport; + return x + width <= vw && y + height <= vh; +}, { message: 'clip must fit within viewport', path: ['clip'] });
254-270: Granting unsupported permissions may throw; sanitize against Playwright’s allowed set.The enum includes many names Playwright doesn’t support in grantPermissions. Unknown names can error.
Add a whitelist and filter:
- if (body.permissions && body.permissions.length > 0) { - await context.grantPermissions(body.permissions, { origin: body.url }); - } + const allowed = new Set(['geolocation','midi','clipboard-read','clipboard-write','persistent-storage','push','background-sync','camera','microphone','notifications','sensors']); + if (body.permissions?.length) { + const filtered = body.permissions.filter(p => allowed.has(p)); + if (filtered.length) await context.grantPermissions(filtered, { origin: body.url }); +}Do the same in screenshots. Also reflect supported names in README.
331-691: /v1/test does not reflect caller-provided context; consider query-driven overrides.README positions it for “verify browser state”, but the endpoint always uses defaultContext.
Option: accept query params (viewport, theme, locale, timezoneId, hasTouch, isMobile) and apply to newContext, mirroring screenshots schema minimally.
README.md (2)
520-528: Headers scope: clarify they apply to target origin (not all requests).Align with code change to apply headers to the page origin only; mention that cross-origin requests remain unaffected.
699-709: Dev instructions mix pnpm and npm; prefer one path.Start command uses npm while deps use pnpm; unify or explain both.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)src/index.js(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
src/index.js
[error] 1-1: Biome lint/format check reported formatting changes and a failure. Command 'pnpm run lint' (biome check ./src) exited with code 1.
🪛 LanguageTool
README.md
[uncategorized] ~159-~159: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... fullPage | boolean | false | Capture full page scroll | | clip | object | - | Crop a...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (1)
src/index.js (1)
185-244: Mirror schema fixes: timezone, headers typing, and clip/fullPage constraint.
- Apply the same timezone validation refinement here (Lines 196-199).
- Constrain headers to string values (Line 193).
If you keep regex, update README to remove plain "UTC"/"GMT" or accept them here too.
| | Parameter | Type | Default | Description | | ||
| |-----------|------|---------|-------------| | ||
| | `url` | string | **required** | The URL to screenshot | | ||
| | `viewport` | object | `{width: 1280, height: 720}` | Viewport dimensions | | ||
| | `viewport.width` | number | 1280 | Viewport width (1-3840) | | ||
| | `viewport.height` | number | 720 | Viewport height (1-2160) | | ||
| | `format` | string | "png" | Image format: "png", "jpeg", "webp" | | ||
| | `quality` | number | 90 | Image quality 0-100 (for jpeg/webp) | | ||
| | `fullPage` | boolean | false | Capture full page scroll | | ||
| | `clip` | object | - | Crop area: `{x, y, width, height}` | | ||
| | `theme` | string | "light" | Browser theme: "light", "dark" | | ||
| | `userAgent` | string | - | Custom user agent string | | ||
| | `locale` | string | - | Browser locale (e.g., "en-US") | | ||
| | `timezoneId` | string | - | IANA timezone identifier (see [Timezone](#timezone) section) | | ||
| | `geolocation` | object | - | GPS coordinates | | ||
| | `geolocation.latitude` | number | - | Latitude (-90 to 90) | | ||
| | `geolocation.longitude` | number | - | Longitude (-180 to 180) | | ||
| | `geolocation.accuracy` | number | - | Accuracy in meters | | ||
| | `permissions` | array | - | Browser permissions (see [Permissions](#permissions) section) | | ||
| | `headers` | object | - | Custom HTTP headers | | ||
| | `waitUntil` | string | "domcontentloaded" | Wait condition: "load", "domcontentloaded", "networkidle", "commit" | | ||
| | `timeout` | number | 30000 | Navigation timeout (0-120000ms) | | ||
| | `sleep` | number | 3000 | Wait time after load (0-60000ms) | | ||
| | `deviceScaleFactor` | number | 1 | Device pixel ratio (0.1-3) | | ||
| | `hasTouch` | boolean | false | Touch support | | ||
| | `isMobile` | boolean | false | Mobile device emulation | | ||
|
|
There was a problem hiding this comment.
Document PNG quality constraint and clip/fullPage exclusivity.
Quality applies only to jpeg/webp; clip cannot be used with fullPage.
Proposed edits:
- Note “quality (jpeg/webp only)”.
- In the table: “clip — cannot be combined with fullPage: true”.
- Update examples using format: "png" to omit “quality”.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~159-~159: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... fullPage | boolean | false | Capture full page scroll | | clip | object | - | Crop a...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
| ## Timezone | ||
|
|
||
| The `timezoneId` parameter allows you to set the browser's timezone for accurate time-based testing and content capture. This is particularly useful for testing applications that display time-sensitive content or have timezone-dependent features. | ||
|
|
||
| ### Format | ||
|
|
||
| The `timezoneId` must be a valid **IANA timezone identifier** in the format `Region/City`. This is the same format used by JavaScript's `Intl.DateTimeFormat` and is the standard for timezone identification. | ||
|
|
||
| ### Valid Timezone Examples | ||
|
|
||
| #### Americas | ||
| - `America/New_York` - Eastern Time (US) | ||
| - `America/Chicago` - Central Time (US) | ||
| - `America/Denver` - Mountain Time (US) | ||
| - `America/Los_Angeles` - Pacific Time (US) | ||
| - `America/Toronto` - Eastern Time (Canada) | ||
| - `America/Vancouver` - Pacific Time (Canada) | ||
| - `America/Sao_Paulo` - Brasília Time (Brazil) | ||
| - `America/Mexico_City` - Central Time (Mexico) | ||
| - `America/Argentina/Buenos_Aires` - Argentina Time | ||
|
|
||
| #### Europe | ||
| - `Europe/London` - Greenwich Mean Time / British Summer Time | ||
| - `Europe/Paris` - Central European Time | ||
| - `Europe/Berlin` - Central European Time | ||
| - `Europe/Rome` - Central European Time | ||
| - `Europe/Madrid` - Central European Time | ||
| - `Europe/Amsterdam` - Central European Time | ||
| - `Europe/Moscow` - Moscow Time | ||
| - `Europe/Istanbul` - Turkey Time | ||
|
|
||
| #### Asia | ||
| - `Asia/Tokyo` - Japan Standard Time | ||
| - `Asia/Shanghai` - China Standard Time | ||
| - `Asia/Hong_Kong` - Hong Kong Time | ||
| - `Asia/Singapore` - Singapore Time | ||
| - `Asia/Seoul` - Korea Standard Time | ||
| - `Asia/Dubai` - Gulf Standard Time | ||
| - `Asia/Kolkata` - India Standard Time | ||
| - `Asia/Bangkok` - Indochina Time | ||
|
|
||
| #### Australia & Pacific | ||
| - `Australia/Sydney` - Australian Eastern Time | ||
| - `Australia/Melbourne` - Australian Eastern Time | ||
| - `Australia/Perth` - Australian Western Time | ||
| - `Pacific/Auckland` - New Zealand Time | ||
| - `Pacific/Honolulu` - Hawaii-Aleutian Time | ||
| - `Pacific/Fiji` - Fiji Time | ||
|
|
||
| #### Africa | ||
| - `Africa/Cairo` - Eastern European Time | ||
| - `Africa/Johannesburg` - South Africa Standard Time | ||
| - `Africa/Lagos` - West Africa Time | ||
| - `Africa/Nairobi` - East Africa Time | ||
|
|
||
| #### Special Cases | ||
| - `UTC` - Coordinated Universal Time | ||
| - `GMT` - Greenwich Mean Time | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Timezone section contradicts schema (UTC/GMT).
You list UTC and GMT as valid, but current regex rejects them; adjust docs or schema to accept plain “UTC” (and consider “GMT” if desired).
If adopting Intl-based validation (recommended), keep docs and provide example with "UTC".
I'll help verify this timezone validation issue. Let me search the codebase for the timezone validation schema.
🏁 Script executed:
#!/bin/bash
# Find timezone validation code/schema
rg -n -i "timezone" -C 5 --type=ts --type=js --type=json --type=yaml | head -100Length of output: 4206
🏁 Script executed:
#!/bin/bash
# Search for regex patterns related to timezone or IANA
rg -n "IANA|timezone.*regex|pattern.*timezone" -C 3Length of output: 2966
🏁 Script executed:
#!/bin/bash
# Look for schema files (common patterns)
fd -e json -e ts -e js -e yaml | xargs rg -l "timezone|timezoneId" 2>/dev/null | head -20Length of output: 12
Fix timezone documentation to match validation schema.
The timezone validation regex in src/index.js (lines 59-62 and 196-199) requires the format Region/City with a mandatory forward slash and characters after it. However, the README lists UTC and GMT as valid standalone entries under "Special Cases."
The current regex /^(Africa|America|Antarctica|Arctic|Asia|Atlantic|Australia|Europe|Indian|Pacific|UTC)\/[A-Za-z_]+$/ rejects:
- Plain
"UTC"(requires something like"UTC/Something") "GMT"entirely (not in the prefix list)
Either:
- Update README to remove UTC/GMT from "Special Cases" or clarify they must follow
Region/Cityformat, or - Update schema to accept plain
"UTC"(and optionally"GMT") by modifying the regex to make the slash and suffix optional or adding alternatives
|
|
||
| ```bash | ||
| curl -X POST http://localhost:3000/v1/screenshots \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "url": "https://example.com", | ||
| "viewport": { | ||
| "width": 2560, | ||
| "height": 1440 | ||
| }, | ||
| "format": "jpeg", | ||
| "quality": 100, | ||
| "fullPage": true, | ||
| "theme": "dark", | ||
| "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36", | ||
| "locale": "en-US", | ||
| "timezoneId": "Europe/London", | ||
| "geolocation": { | ||
| "latitude": 51.5074, | ||
| "longitude": -0.1278, | ||
| "accuracy": 20 | ||
| }, | ||
| "permissions": [ | ||
| "geolocation", | ||
| "notifications", | ||
| "camera", | ||
| "microphone", | ||
| "clipboard-read", | ||
| "clipboard-write", | ||
| "payment-handler", | ||
| "usb", | ||
| "bluetooth" | ||
| ], | ||
| "headers": { | ||
| "Authorization": "Bearer premium-token", | ||
| "X-Premium-User": "true", | ||
| "X-Request-Source": "screenshot-api" | ||
| }, | ||
| "waitUntil": "networkidle", | ||
| "timeout": 90000, | ||
| "sleep": 8000, | ||
| "deviceScaleFactor": 2, | ||
| "hasTouch": false, | ||
| "isMobile": false | ||
| }' \ | ||
| --output "high-quality-desktop-screenshot.jpg" | ||
| ``` |
There was a problem hiding this comment.
Example uses quality with PNG; will fail after enforcing constraints.
Remove quality from PNG examples or switch to jpeg/webp.
🤖 Prompt for AI Agents
In README.md around lines 430 to 476, the example POST includes a "quality"
field while using formats that may be PNG in other examples and will fail once
quality constraints are enforced; update the example(s) by either removing the
"quality" property when the format is PNG or change the format to a lossy type
that supports quality (e.g., "jpeg" or "webp"); ensure the JSON payload and
example output filename are consistent with the chosen format.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/index.js (7)
39-39: Restrict header values to strings (Playwright expects string headers).This was flagged in a previous review.
headerscurrently allows any value; non-strings will breakroute.continue.Constrain the schema:
-headers: z.record(z.string(), z.any()).optional(), +headers: z.record(z.string(), z.string()).optional(),
63-69: Timezone validation rejects valid identifiers advertised in README.This was flagged in a previous review. The regex requires
Region/Cityand allowsUTC/City, but README claims plainUTCandGMTare valid.Prefer runtime validation via
Intl:-timezoneId: z.string().regex( - /^(Africa|America|Antarctica|Arctic|Asia|Atlantic|Australia|Europe|Indian|Pacific|UTC)\/[A-Za-z_]+$/, - "Must be a valid IANA timezone identifier (e.g., 'America/New_York', 'Europe/London', 'Asia/Tokyo')" -).optional(), +timezoneId: z.string().refine((tz) => { + try { new Intl.DateTimeFormat('en-US', { timeZone: tz }); return true; } catch { return false; } +}, "Must be a valid IANA timezone (e.g., 'America/New_York', 'Europe/London', 'UTC')").optional(),Mirror the same change in
lighthouseSchema.
180-190: Avoid passing quality for PNG and disallow clip with fullPage.This was flagged in a previous review:
- Playwright rejects
qualityfor PNG.clipandfullPageare mutually exclusive; passing both throws.Update options construction:
- const screenshotOptions = { - type: body.format, - quality: body.quality, - fullPage: body.fullPage, - }; + const screenshotOptions = { + type: body.format, + fullPage: body.fullPage, + }; + if ((body.format === 'jpeg' || body.format === 'webp') && typeof body.quality === 'number') { + screenshotOptions.quality = body.quality; + } - if (body.clip) { + if (body.clip) { + if (body.fullPage) { + throw createError({ statusCode: 400, statusMessage: 'clip cannot be used with fullPage' }); + } screenshotOptions.clip = body.clip; }
208-208: Restrict header values to strings (Playwright expects string headers).Same issue as line 39.
headerscurrently allows any value; non-strings will breakroute.continue.Constrain the schema:
-headers: z.record(z.string(), z.any()).optional(), +headers: z.record(z.string(), z.string()).optional(),
211-217: Timezone validation rejects valid identifiers.Same issue as lines 63-69. The regex requires
Region/Cityformat and rejects plainUTCandGMT.Prefer runtime validation via
Intl:-timezoneId: z.string().regex( - /^(Africa|America|Antarctica|Arctic|Asia|Atlantic|Australia|Europe|Indian|Pacific|UTC)\/[A-Za-z_]+$/, - "Must be a valid IANA timezone identifier (e.g., 'America/New_York', 'Europe/London', 'Asia/Tokyo')" -).optional(), +timezoneId: z.string().refine((tz) => { + try { new Intl.DateTimeFormat('en-US', { timeZone: tz }); return true; } catch { return false; } +}, "Must be a valid IANA timezone (e.g., 'America/New_York', 'Europe/London', 'UTC')").optional(),
52-59: Schema allowsclipandfullPagetogether, but they're mutually exclusive.Playwright throws if both
clipandfullPage: trueare provided. Consider adding schema-level validation to reject this combination early.Add a
.refine()check to the schema:const screenshotSchema = z.object({ // ... existing fields +}).refine( + (data) => !(data.clip && data.fullPage), + { message: "clip and fullPage cannot be used together" } });
50-51: Quality default applies to PNG, but Playwright ignores or rejects it.The
qualityfield defaults to90for all formats, but Playwright'sscreenshotmethod only acceptsqualityfor JPEG and WebP. For PNG, this parameter is either ignored or causes an error.Consider making
qualityconditional or removing the default:-format: z.enum(["png", "jpeg", "webp"]).default("png"), -quality: z.number().min(0).max(100).default(90), +format: z.enum(["png", "jpeg", "webp"]).default("png"), +quality: z.number().min(0).max(100).optional(),Then apply quality only when appropriate in the handler (see separate comment).
🧹 Nitpick comments (3)
src/index.js (3)
280-290: Consider adding viewport and device options to lighthouse context.The screenshot handler (lines 129-137) applies
viewport,deviceScaleFactor,hasTouch, andisMobilefrom the request body, but the lighthouse handler only setscolorScheme. This inconsistency means lighthouse reports can't test mobile viewports or device-specific behavior.Consider extending
lighthouseSchemaand applying these options:const contextOptions = { ...defaultContext, colorScheme: body.theme, + viewport: body.viewport || defaultContext.viewport, + deviceScaleFactor: body.deviceScaleFactor, + hasTouch: body.hasTouch, + isMobile: body.isMobile, };(You would also need to add these fields to
lighthouseSchemaif not already present.)
368-709: Consider extracting HTML template to a separate file.This 340+ line HTML template with embedded CSS and JavaScript reduces maintainability and readability of the main handler file.
Consider moving the template to a separate file:
// In a new file: templates/test.html <!DOCTYPE html> <html lang="en"> ... </html> // In src/index.js: import { readFile } from 'node:fs/promises'; const testTemplate = await readFile('./templates/test.html', 'utf-8'); // In the handler: await context.close(); event.node.res.setHeader("Content-Type", "text/html"); return testTemplate;Alternatively, use a template function that accepts dynamic values if needed.
357-366: Test route uses only default context; cannot verify custom configurations.The test route creates a context with
defaultContextsettings and navigates toabout:blank. This means it cannot test the impact of customuserAgent,locale,timezoneId,viewport,deviceScaleFactor,geolocation, orpermissionsthat the API now supports.Consider accepting query parameters or a POST body to allow testing custom configurations:
router.post("/v1/test", defineEventHandler(async (event) => { const body = await readValidatedBody(event, screenshotSchema.pick({ theme: true, viewport: true, userAgent: true, locale: true, timezoneId: true, geolocation: true, permissions: true, deviceScaleFactor: true, hasTouch: true, isMobile: true, }).parse); const contextOptions = { ...defaultContext, colorScheme: body.theme, viewport: body.viewport || defaultContext.viewport, // ... apply other options }; const context = await browser.newContext(contextOptions); // ... rest of handler }));This would make the test route actually useful for verifying configuration effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.js(7 hunks)
🔇 Additional comments (3)
src/index.js (3)
129-150: LGTM! Context options construction is well-structured.The context options are built with proper fallbacks and conditionally apply optional fields. Permission granting is correctly scoped to the target origin.
256-264: Well-designed threshold defaults.The thresholds object provides sensible defaults (all zeros) when individual properties aren't specified, while keeping the entire object optional. This allows flexible override without requiring all categories.
321-328: LGTM! Threshold fallback logic is correct.The handler correctly uses custom thresholds if provided, otherwise defaults to zeros for all categories. This matches the schema design and allows flexible overrides.
| if (navigator.geolocation) { | ||
| navigator.geolocation.getCurrentPosition( | ||
| (position) => { | ||
| document.getElementById('geolocation-info').innerHTML = | ||
| 'Latitude: ' + position.coords.latitude + '<br>' + | ||
| 'Longitude: ' + position.coords.longitude + '<br>' + | ||
| 'Accuracy: ' + position.coords.accuracy + 'm'; | ||
| }, | ||
| (error) => { | ||
| document.getElementById('geolocation-info').innerHTML = 'Error: ' + error.message; | ||
| }, | ||
| { timeout: 1000 } | ||
| ); | ||
| } else { | ||
| document.getElementById('geolocation-info').textContent = 'Not available'; | ||
| } |
There was a problem hiding this comment.
Geolocation API may fail on about:blank.
The test page tries to call navigator.geolocation.getCurrentPosition(), but this typically requires a secure context with proper permissions. On about:blank, this will likely timeout or error.
Consider either:
- Navigating to a real URL (e.g., a local test page) instead of
about:blank - Pre-granting geolocation permission to the context
- Adjusting the UI to handle the expected failure gracefully (already partially done with error handling)
The current 1-second timeout will cause delays; consider reducing it or making the permission grant explicit.
🤖 Prompt for AI Agents
In src/index.js around lines 603-618, the Geolocation call on about:blank can
fail or timeout because geolocation requires a secure context and permission;
update the code to avoid flaky behavior by either navigating tests to a real
secure URL (or local file:///served page) instead of about:blank, pre-granting
geolocation permission for the test context, and/or reducing the timeout (e.g.,
much less than 1000ms) and improving the UI fallback: ensure getCurrentPosition
errors are treated as expected (show a friendly message and skip waits), and
avoid long blocking timeouts in test flows. Make one concrete change: choose one
approach (prefer navigating to a real test page or programmatically granting
permission) and implement the corresponding adjustment plus a robust
short-timeout error path.
What does this PR do?
Makes the API more flexible, allow passing more configs, keeps reasonable defaults,
Test Plan
Tested manually.
Related PRs and Issues
N/A
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
New Features
Documentation