feat: add @lynx-js/web-mcp-server #1876
Conversation
🦋 Changeset detectedLatest commit: 14f18a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a new package Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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: 10
🧹 Nitpick comments (5)
packages/web-platform/web-mcp-server/package.json (1)
27-29: Reconsider peerDependencies for a private package.Since this package is marked as
"private": true, it won't be published to npm, makingpeerDependencieslargely irrelevant. The peer dependency on Puppeteer is only meaningful for published packages where external consumers might need to provide their own Puppeteer installation.For a private monorepo package, you can:
- Remove the
peerDependenciessection entirely, or- Keep it for documentation purposes but understand it won't be enforced
packages/web-platform/web-mcp-server/test/test.spec.ts (3)
29-41: Remove commented code or convert to tracked issue.Commented-out code should either be removed or tracked as a TODO/issue. If
get_element_hierarchytool functionality is pending, create a proper issue or add a TODO comment.Apply this diff to replace with a TODO:
-// describe('get_element_hierarchy tool', () => { -// it('should has get_element_hierarchy tool', async () => { -// const tools = (await client.listTools()).tools; -// const tool = tools.find((t) => t.name === 'get_element_hierarchy'); -// expect(tool).toBeDefined(); -// }); - -// it('should return element hierarchy', async () => { -// const result = await client.callTool('get_element_hierarchy', { -// url: 'https://lynxjs.github.io/lynx/examples/basic.html', -// }); -// }); -// }); +// TODO: Add tests for get_element_hierarchy toolDo you want me to generate proper test cases for the
get_element_hierarchytool?
43-49: Add functional test for screenshot capture.The current test only checks if the
get_screenshottool exists but doesn't verify it actually works. Consider adding a test that calls the tool and validates the response.Add this test case:
it('should capture screenshot', async () => { const result = await client.callTool('get_screenshot', { url: 'http://localhost:3000', }); expect(result.content).toBeDefined(); expect(result.content[0]).toHaveProperty('type', 'image'); expect(result.content[0]).toHaveProperty('data'); // Optionally: verify base64 format expect(result.content[0].data).toMatch(/^[A-Za-z0-9+/]+=*$/); });
51-54: Use SIGTERM for graceful shutdown.
SIGHUPis less portable and less conventional thanSIGTERMfor graceful process termination. UseSIGTERMor omit the signal argument (defaults toSIGTERM).Apply this diff:
afterAll(() => { client.close(); - rspeedy.kill('SIGHUP'); + rspeedy.kill('SIGTERM'); });packages/web-platform/web-mcp-server/src/createPuppeteer.ts (1)
4-13: Document caller responsibility for page cleanup.The function returns a page object but doesn't document that callers must close it to avoid resource leaks. Add JSDoc to clarify this responsibility.
Add this JSDoc:
+/** + * Creates and configures a Puppeteer page for a Lynx application. + * + * @param url - The URL to navigate to + * @returns A configured Puppeteer page + * @throws {Error} If navigation fails or the lynx-view element is not found + * + * @important Callers MUST call page.close() after use to prevent resource leaks. + * + * @example + * ```typescript + * const page = await createPuppeteerPage('http://localhost:3000'); + * try { + * // Use page + * } finally { + * await page.close(); + * } + * ``` + */ export async function createPuppeteerPage(url: string) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/breezy-drinks-sneeze.md(1 hunks).changeset/config.json(1 hunks)packages/web-platform/web-mcp-server/LICENSE.txt(1 hunks)packages/web-platform/web-mcp-server/README.md(1 hunks)packages/web-platform/web-mcp-server/package.json(1 hunks)packages/web-platform/web-mcp-server/rslib.config.ts(1 hunks)packages/web-platform/web-mcp-server/src/createPuppeteer.ts(1 hunks)packages/web-platform/web-mcp-server/src/index.ts(1 hunks)packages/web-platform/web-mcp-server/test/test.spec.ts(1 hunks)packages/web-platform/web-mcp-server/tsconfig.json(1 hunks)packages/web-platform/web-mcp-server/vitest.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, generate and commit a Changeset describing your changes
Files:
.changeset/breezy-drinks-sneeze.md
🧠 Learnings (2)
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, private packages (marked with "private": true in package.json) like lynx-js/react-transform don't require meaningful changeset entries even when their public APIs change, since they are not published externally and only affect internal development.
Applied to files:
.changeset/config.json
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, empty changeset files (containing only `---\n\n---`) are used for internal changes that modify src/** files but don't require meaningful release notes, such as private package changes or testing-only modifications. This satisfies CI requirements without generating user-facing release notes.
Applied to files:
.changeset/breezy-drinks-sneeze.md
🧬 Code graph analysis (1)
packages/web-platform/web-mcp-server/src/index.ts (1)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (1)
createPuppeteerPage(4-13)
🔇 Additional comments (9)
.changeset/breezy-drinks-sneeze.md (1)
1-5: LGTM! Changeset format is appropriate for private package.The minimal changeset entry is acceptable since
@lynx-js/web-mcp-serveris marked as private in package.json and won't generate user-facing release notes.Based on learnings
packages/web-platform/web-mcp-server/rslib.config.ts (1)
26-28: Verify the ignored 'cjs-resolves-to-esm' rule.The configuration ignores the 'cjs-resolves-to-esm' rule from arethetypeswrong. Since this package is ESM-only (type: "module" in package.json) and targets Node environments, this rule may not apply. However, ensure this doesn't mask any legitimate dual-package hazard warnings if the package might be consumed by CommonJS code.
packages/web-platform/web-mcp-server/tsconfig.json (1)
1-13: LGTM! TypeScript configuration is appropriate.The configuration correctly enables
compositebuilds, includes DOM libraries for Puppeteer types, and uses strict settings likeexactOptionalPropertyTypes. ThenoEmit: truesetting aligns with the rslib-based build process..changeset/config.json (1)
18-19: LGTM! Changeset configuration correctly includes new package.The new
@lynx-js/web-mcp-serverpackage is appropriately added to the first fixed dependency group, ensuring it versions together with related web-platform packages.packages/web-platform/web-mcp-server/vitest.config.ts (1)
1-8: LGTM! Vitest configuration is appropriate.The configuration is minimal and follows best practices. The test include pattern correctly matches the test file location, and the test name provides clear identification in multi-package scenarios.
packages/web-platform/web-mcp-server/package.json (1)
8-11: Bin entry shebang present:src/index.tsbegins with#!/usr/bin/env node.packages/web-platform/web-mcp-server/LICENSE.txt (1)
1-202: LGTM!Standard Apache 2.0 license text with appropriate copyright notice for The Lynx Authors.
packages/web-platform/web-mcp-server/README.md (1)
1-97: LGTM!The documentation is comprehensive, well-structured, and accurately describes the MCP server's capabilities, tools, and usage.
packages/web-platform/web-mcp-server/src/index.ts (1)
87-96: LGTM!The main function properly sets up the stdio transport, connects the server, and includes appropriate error handling with process exit on fatal errors.
PupilTong
force-pushed
the
p/hw/mcp-initial
branch
2 times, most recently
from
ecc0e6e to
3231bf5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (1)
3-5: Avoid top‑levelpuppeteer.launch; add lazy init andcloseBrowser().Top‑level launch creates a long‑lived browser that’s never closed → leaks and brittle tests. Lazily create and export a close function.
-const browser = await puppeteer.launch({ headless: true }); -export async function createPuppeteerPage(url: string) { - const page = await browser.newPage(); +let browser: Awaited<ReturnType<typeof puppeteer.launch>> | null = null; + +export async function getBrowser() { + if (!browser) { + browser = await puppeteer.launch({ headless: true }); + } + return browser; +} + +export async function closeBrowser() { + if (browser) { + await browser.close(); + browser = null; + } +} + +export async function createPuppeteerPage(url: string) { + const browser = await getBrowser(); + const page = await browser.newPage();
🧹 Nitpick comments (3)
packages/web-platform/web-mcp-server/vitest.config.ts (1)
4-7: Broaden test glob and pin environment for stability.Current include misses nested specs; set environment explicitly to avoid defaults changing across Vitest versions.
Based on learnings
export default defineConfig({ test: { - include: ['**/test/*.spec.ts'], - name: 'web-platform/web-mcp-server', + include: ['**/test/**/*.spec.ts'], + name: 'web-platform/web-mcp-server', + environment: 'node', }, });packages/web-platform/web-mcp-server/src/index.ts (2)
5-5: Close the shared browser on shutdown.Add a shutdown hook so the Puppeteer browser is closed when the process exits.
-import { createPuppeteerPage } from './createPuppeteer.js'; +import { createPuppeteerPage, closeBrowser } from './createPuppeteer.js';async function main() { const transport: StdioServerTransport = new StdioServerTransport(); await server.connect(transport); console.error('Lynx Web Platform MCP Server running on stdio'); + // Graceful shutdown + process.once('beforeExit', async () => { + try { await closeBrowser(); } catch {} + }); }Optional: also register SIGINT/SIGTERM handlers to ensure cleanup on signals.
Also applies to: 93-97
51-54: Remove duplicate comment.The “Get the element hierarchy as JSON” comment appears twice.
- // Get the element hierarchy as JSON try { - // Get the element hierarchy as JSON const result = await page.evaluate(() => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/breezy-drinks-sneeze.md(1 hunks).changeset/config.json(1 hunks)packages/web-platform/web-mcp-server/LICENSE.txt(1 hunks)packages/web-platform/web-mcp-server/README.md(1 hunks)packages/web-platform/web-mcp-server/package.json(1 hunks)packages/web-platform/web-mcp-server/rslib.config.ts(1 hunks)packages/web-platform/web-mcp-server/src/createPuppeteer.ts(1 hunks)packages/web-platform/web-mcp-server/src/index.ts(1 hunks)packages/web-platform/web-mcp-server/test/test.spec.ts(1 hunks)packages/web-platform/web-mcp-server/tsconfig.json(1 hunks)packages/web-platform/web-mcp-server/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .changeset/config.json
- packages/web-platform/web-mcp-server/LICENSE.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/web-platform/web-mcp-server/tsconfig.json
- packages/web-platform/web-mcp-server/package.json
- .changeset/breezy-drinks-sneeze.md
- packages/web-platform/web-mcp-server/test/test.spec.ts
- packages/web-platform/web-mcp-server/README.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/web-platform/web-mcp-server/src/index.ts (1)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (1)
createPuppeteerPage(4-20)
| inputSchema: { | ||
| url: z.string().describe('The web preview URL of the current Lynx Page'), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Fix input schema: wrap fields in z.object(...).
inputSchema should be a Zod object, not a plain object containing Zod fields. Current shape likely bypasses validation and may break the SDK.
description: 'run a lynx bundle on web platform and get a screenshot. ',
- inputSchema: {
- url: z.string().describe('The web preview URL of the current Lynx Page'),
- },
+ inputSchema: z.object({
+ url: z.string().describe('The web preview URL of the current Lynx Page'),
+ }), description:
'run a lynx bundle on web platform and get the element hierarchy. Note that the style attributes are not included. ',
- inputSchema: {
- url: z.string().describe('The web preview URL of the Lynx page'),
- },
+ inputSchema: z.object({
+ url: z.string().describe('The web preview URL of the Lynx page'),
+ }),To confirm house style across the repo, search for other usages:
Also applies to: 45-47
🏁 Script executed:
#!/bin/bash
rg -n -C3 --type=ts '\bregisterTool\s*\('Length of output: 952
Wrap inputSchema in z.object for both tools
inputSchema must be a Zod object schema, not a plain object of Zod fields.
- inputSchema: {
- url: z.string().describe('The web preview URL of the current Lynx Page'),
- },
+ inputSchema: z.object({
+ url: z.string().describe('The web preview URL of the current Lynx Page'),
+ }),- inputSchema: {
- url: z.string().describe('The web preview URL of the Lynx page'),
- },
+ inputSchema: z.object({
+ url: z.string().describe('The web preview URL of the Lynx page'),
+ }),📝 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.
| inputSchema: { | |
| url: z.string().describe('The web preview URL of the current Lynx Page'), | |
| }, | |
| }, | |
| // In the JSON‐hierarchy tool (around lines 18–21) | |
| inputSchema: z.object({ | |
| url: z.string().describe('The web preview URL of the current Lynx Page'), | |
| }), | |
| }, | |
| // In the screenshot tool (around lines 45–47) | |
| inputSchema: z.object({ | |
| url: z.string().describe('The web preview URL of the Lynx page'), | |
| }), |
🤖 Prompt for AI Agents
In packages/web-platform/web-mcp-server/src/index.ts around lines 18 to 21, the
inputSchema is currently a plain object of Zod fields; replace it with a Zod
object schema by wrapping the fields in z.object({...}) so inputSchema is a
proper Zod schema (e.g., inputSchema: z.object({ url: z.string().describe('The
web preview URL of the current Lynx Page') })). Ensure both tool definitions use
this z.object wrapper.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (2)
3-3: Browser lifecycle management issue remains unaddressed.The browser is still launched at module initialization with top-level await and never closed, causing resource leaks. This critical issue was flagged in a previous review but has not been addressed.
As noted in the previous review, refactor to lazy initialization with proper cleanup:
-const browser = await puppeteer.launch({ headless: true }); +let browser: Awaited<ReturnType<typeof puppeteer.launch>> | null = null; + +async function getBrowser() { + if (!browser) { + browser = await puppeteer.launch({ headless: true }); + } + return browser; +} + export async function createPuppeteerPage(url: string) { - const page = await browser.newPage(); + const browser = await getBrowser(); + const page = await browser.newPage(); // ... rest of code } + +export async function closeBrowser() { + if (browser) { + await browser.close(); + browser = null; + } +}Then update callers to invoke
closeBrowser()during cleanup.
11-13: Useunknowninstead ofError | anyfor error typing.The catch clause uses
Error | anywhich is unsafe. TypeScript best practice is to useunknownfor caught errors and narrow the type when accessing properties.Apply this diff:
- } catch (error: Error | any) { + } catch (error: unknown) { await page.close(); - throw new Error(`Failed to navigate to ${url}: ${error.message}`); + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to navigate to ${url}: ${message}`); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
🔇 Additional comments (1)
pnpm-workspace.yaml (1)
64-64: LGTM! Correct configuration for Puppeteer.Adding
puppeteertoignoredBuiltDependenciesis the standard approach for handling Puppeteer's native binaries. This prevents pnpm from attempting to build Puppeteer from source, as it manages its own Chromium downloads.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/web-platform/web-mcp-server/rslib.config.ts (1)
23-30: Verify that ESM-only output aligns with package goals.The
cjs-resolves-to-esmrule is ignored, indicating this package publishes ESM-only. Ensure that omitting CJS builds is intentional and documented if consumers might expect dual-format support.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web-platform/web-mcp-server/package.json(1 hunks)packages/web-platform/web-mcp-server/rslib.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web-platform/web-mcp-server/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: zizmor
🔇 Additional comments (1)
packages/web-platform/web-mcp-server/rslib.config.ts (1)
13-13: Declaration files now enabled—previous issue resolved.The
dts: truesetting ensures TypeScript declaration files are generated, addressing the major issue raised in the previous review. This allows downstream consumers to benefit from full type safety.
Web Explorer#5894 Bundle Size — 364.6KiB (0%).14f18a6(current) vs df5ebcf main#5892(baseline) Bundle metrics
|
| Current #5894 |
Baseline #5892 |
|
|---|---|---|
 Initial JS |
144.55KiB |
144.55KiB |
| Initial CSS |
32.11KiB |
32.11KiB |
 Cache Invalidation |
0% |
39.64% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
219(-0.45%) |
220 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.23% |
3.23% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #5894 |
Baseline #5892 |
|
|---|---|---|
| JS |
238.69KiB |
238.69KiB |
| Other |
93.8KiB |
93.8KiB |
| CSS |
32.11KiB |
32.11KiB |
Bundle analysis report Branch PupilTong:p/hw/mcp-initial Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#5898 Bundle Size — 237.56KiB (0%).14f18a6(current) vs df5ebcf main#5896(baseline) Bundle metrics
|
| Current #5898 |
Baseline #5896 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
166 |
166 |
| Duplicate Modules |
68 |
68 |
| Duplicate Code |
46.82% |
46.82% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #5898 |
Baseline #5896 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.8KiB |
91.8KiB |
Bundle analysis report Branch PupilTong:p/hw/mcp-initial Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1876 will degrade performances by 6.92%Comparing Summary
Benchmarks breakdown
Footnotes
|
PupilTong
force-pushed
the
p/hw/mcp-initial
branch
from
9fb4cb1 to
d9d7206
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web-platform/web-mcp-server/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: zizmor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (3)
3-6: Browser lifecycle issue remains unaddressed.The browser is still launched at module initialization with top-level await and never closed, causing resource leaks and making testing difficult. This critical issue was flagged in the previous review but has not been resolved.
Please implement the lazy initialization pattern suggested in the previous review to ensure proper cleanup.
14-16: Unsafe error typing remains unaddressed.The catch block still uses
Error | any, which is unsafe. This issue was flagged in the previous review but has not been fixed.Apply this diff to use safe error typing:
- } catch (error: Error | any) { + } catch (error: unknown) { await page.close(); - throw new Error(`Failed to navigate to ${url}: ${error.message}`); + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to navigate to ${url}: ${message}`); }
23-25: Unsafe error typing remains unaddressed.The catch block still uses
Error | any, which is unsafe. This issue was flagged in the previous review but has not been fixed.Apply this diff to use safe error typing:
- } catch (error: Error | any) { + } catch (error: unknown) { await page.close(); - throw new Error(`Lynx page element not found at ${url}: ${error.message}`); + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Lynx page element not found at ${url}: ${message}`); }
|
|
||
| const browser = await puppeteer.launch({ | ||
| headless: true, | ||
| args: ['--no-sandbox', '--disable-setuid-sandbox'], |
There was a problem hiding this comment.
Security risk: Disabling sandbox protection.
The --no-sandbox and --disable-setuid-sandbox flags bypass critical Chromium security features. While sometimes necessary in Docker/CI environments, this significantly weakens the security posture and exposes the system to potential exploits if the browser processes untrusted content.
Consider:
- Document why these flags are required (e.g., containerized deployment constraints)
- Make sandbox disabling configurable via environment variables so it can be enabled in development/production
- If used in production, ensure additional security controls (network isolation, resource limits, input validation) are in place
Example configuration approach:
const browser = await puppeteer.launch({
headless: true,
args: process.env.DISABLE_SANDBOX === 'true'
? ['--no-sandbox', '--disable-setuid-sandbox']
: [],
});
PupilTong
force-pushed
the
p/hw/mcp-initial
branch
from
13429b8 to
27960f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web-platform/web-mcp-server/test/test.spec.ts (1)
22-27: Add error handling and readiness check for dev server.This issue was already flagged in a previous review. The dev server spawn needs error handling, readiness verification, and proper stdio configuration to prevent test hangs and flaky failures.
🧹 Nitpick comments (3)
packages/web-platform/web-mcp-server/test/test.spec.ts (3)
29-41: Consider removing or enabling commented-out tests.The commented-out tests for
get_element_hierarchysuggest incomplete test coverage or work in progress. Either enable these tests once the feature is ready or remove them to keep the codebase clean.
43-49: Expand test to verify tool functionality.The test only checks that
get_screenshotexists but doesn't validate its functionality. Consider adding a test that actually calls the tool (potentially using the dev server that's already being spawned) to ensure it works correctly.Example test to add:
it('should capture screenshot from dev server', async () => { const result = await client.callTool('get_screenshot', { url: 'http://localhost:3000', }); expect(result.content).toBeDefined(); // Add more specific assertions based on expected response format });
51-54: Improve cleanup robustness.The cleanup doesn't handle potential failures gracefully. If client connection or dev server spawn failed during initialization, the cleanup will throw errors.
Apply this diff for safer cleanup:
afterAll(() => { - client.close(); - rspeedy.kill('SIGHUP'); + try { + client?.close(); + } catch (err) { + console.error('Failed to close client:', err); + } + + try { + rspeedy?.kill('SIGTERM'); + } catch (err) { + console.error('Failed to kill dev server:', err); + } });
PupilTong
force-pushed
the
p/hw/mcp-initial
branch
from
379ca77 to
fb3ea56
Compare
PupilTong
force-pushed
the
p/hw/mcp-initial
branch
from
fb3ea56 to
43a73db
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (2)
36-39: Useunknownin catches and normalize error messageType catches as unknown and safely extract message; current
Error | anyis unsafe.- } catch (error: Error | any) { + } catch (error: unknown) { await page.close(); - throw new Error(`Failed to navigate to ${url}: ${error.message}`); + const msg = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to navigate to ${url}: ${msg}`); } @@ - } catch (error: Error | any) { + } catch (error: unknown) { await page.close(); - throw new Error(`Lynx page element not found at ${url}: ${error.message}`); + const msg = error instanceof Error ? error.message : String(error); + throw new Error(`Lynx page element not found at ${url}: ${msg}`); }Also applies to: 45-47
10-13: Make sandbox disabling configurable, not unconditionalHard-disabling Chromium sandbox weakens security. Gate via env or config; enable sandbox by default.
- browser = await puppeteer.launch({ - headless: true, - args: ['--no-sandbox', '--disable-setuid-sandbox'], - }); + const disableSandbox = process.env.DISABLE_SANDBOX === 'true'; + browser = await puppeteer.launch({ + headless: true, + args: disableSandbox ? ['--no-sandbox', '--disable-setuid-sandbox'] : [], + });packages/web-platform/web-mcp-server/test/test.spec.ts (2)
23-28: Harden dev server handling: errors, readiness, and graceful teardownAdd spawn error handling, wait for readiness, and await async cleanup.
-const rspeedy = spawn('pnpm', ['dev'], { - cwd: reactLynxExampleRoot, - stdio: 'ignore', - shell: true, -}); +const rspeedy = spawn('pnpm', ['dev'], { + cwd: reactLynxExampleRoot, + stdio: 'pipe', + shell: true, +}); +rspeedy.on('error', (err) => { + throw new Error(`Failed to start dev server: ${err.message}`); +}); +// Wait until server responds (adjust URL/timeout as needed) +await new Promise<void>((resolve, reject) => { + const start = Date.now(); + const timeoutMs = 30_000; + const url = process.env.LYNX_EXAMPLE_URL ?? 'http://localhost:3000'; + const poll = async () => { + try { + const res = await fetch(url); + if (res.ok) return resolve(); + } catch {} + if (Date.now() - start > timeoutMs) return reject(new Error('Dev server not ready')); + setTimeout(poll, 500); + }; + poll(); +}); @@ -afterAll(() => { - client.close(); - rspeedy.kill('SIGHUP'); -}); +afterAll(async () => { + try { + await client.close?.(); + } finally { + if (!rspeedy.killed) rspeedy.kill('SIGTERM'); + } +});Also applies to: 52-55
10-21: Initialize client in beforeAll and verify build artifact existsModule-level connect without await can hang and flake. Move to beforeAll, await connect, and fail fast if dist is missing.
-import { describe, it, expect, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; @@ -const client = new Client({ - name: 'test-client', - version: '1.0.0', -}); -client.connect( - new StdioClientTransport({ - command: 'node', - args: [ - path.join(__dirname, '../dist/index.js'), - ], - }), -); +let client: Client; +beforeAll(async () => { + const distPath = path.join(__dirname, '../dist/index.js'); + const { existsSync } = await import('node:fs'); + if (!existsSync(distPath)) { + throw new Error('Build artifact not found. Run `pnpm build` before testing.'); + } + client = new Client({ name: 'test-client', version: '1.0.0' }); + try { + await client.connect( + new StdioClientTransport({ command: 'node', args: [distPath] }), + ); + } catch (err) { + throw new Error( + `Failed to connect MCP client: ${err instanceof Error ? err.message : String(err)}` + ); + } +});packages/web-platform/web-mcp-server/src/index.ts (1)
18-21: Fix inputSchema: must be a Zod objectCurrent shape bypasses validation. Wrap fields in z.object for both tools.
- inputSchema: { - url: z.string().describe('The web preview URL of the current Lynx Page'), - }, + inputSchema: z.object({ + url: z.string().describe('The web preview URL of the current Lynx Page'), + }), @@ - inputSchema: { - url: z.string().describe('The web preview URL of the Lynx page'), - }, + inputSchema: z.object({ + url: z.string().describe('The web preview URL of the Lynx page'), + }),Also applies to: 45-47
🧹 Nitpick comments (1)
packages/web-platform/web-mcp-server/src/index.ts (1)
1-6: Close Puppeteer browser on shutdownAdd signal/exit handlers to call closeBrowser() to avoid orphaned processes.
-import { createPuppeteerPage } from './createPuppeteer.js'; +import { createPuppeteerPage, closeBrowser } from './createPuppeteer.js'; @@ main().catch((error) => { console.error('Fatal error in main():', error); process.exit(1); }); + +// Graceful shutdown +for (const evt of ['SIGINT', 'SIGTERM', 'beforeExit'] as const) { + // eslint-disable-next-line @typescript-eslint/no-misused-promises + process.on(evt, async () => { + try { await closeBrowser(); } finally { + if (evt !== 'beforeExit') process.exit(0); + } + }); +}Also applies to: 93-102
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/breezy-drinks-sneeze.md(1 hunks).changeset/config.json(1 hunks)packages/web-platform/web-mcp-server/LICENSE.txt(1 hunks)packages/web-platform/web-mcp-server/README.md(1 hunks)packages/web-platform/web-mcp-server/package.json(1 hunks)packages/web-platform/web-mcp-server/rslib.config.ts(1 hunks)packages/web-platform/web-mcp-server/src/createPuppeteer.ts(1 hunks)packages/web-platform/web-mcp-server/src/index.ts(1 hunks)packages/web-platform/web-mcp-server/test/test.spec.ts(1 hunks)packages/web-platform/web-mcp-server/tsconfig.json(1 hunks)packages/web-platform/web-mcp-server/turbo.json(1 hunks)packages/web-platform/web-mcp-server/vitest.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/web-platform/web-mcp-server/turbo.json
- packages/web-platform/web-mcp-server/tsconfig.json
- pnpm-workspace.yaml
- .changeset/config.json
- packages/web-platform/web-mcp-server/vitest.config.ts
- packages/web-platform/web-mcp-server/package.json
- packages/web-platform/web-mcp-server/LICENSE.txt
- packages/web-platform/web-mcp-server/README.md
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, generate and commit a Changeset describing your changes
Files:
.changeset/breezy-drinks-sneeze.md
🧠 Learnings (1)
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, empty changeset files (containing only `---\n\n---`) are used for internal changes that modify src/** files but don't require meaningful release notes, such as private package changes or testing-only modifications. This satisfies CI requirements without generating user-facing release notes.
Applied to files:
.changeset/breezy-drinks-sneeze.md
🧬 Code graph analysis (1)
packages/web-platform/web-mcp-server/src/index.ts (1)
packages/web-platform/web-mcp-server/src/createPuppeteer.ts (1)
createPuppeteerPage(28-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: zizmor
🔇 Additional comments (5)
packages/web-platform/web-mcp-server/rslib.config.ts (4)
1-4: LGTM!Standard imports for rslib configuration with appropriate type checking and publishing plugins.
6-14: Previous DTS issue resolved; config follows best practices.The declaration output (
dts: true) is now correctly enabled, addressing the previous review concern. The ESM-only approach withautoExternal: truealigns with modern Node.js library practices and matches the plugin ignore rule forcjs-resolves-to-esm.
15-21: LGTM!Output configuration is appropriate:
target: 'node'with source maps and a standard dist path.
22-32: Type checking runs in CI only; consider local developer workflow.The
pluginAreTheTypesWrongonly executes in CI (process.env['CI']), which improves local build performance but means developers won't catch type resolution issues until CI runs. This is a reasonable trade-off for build speed, but ensure the team is aware..changeset/breezy-drinks-sneeze.md (1)
1-5: Changeset looks fine for a minimal/internal entryLGTM. The minimal body is acceptable in this repo for internal changes.
Based on learnings
| export async function getBrowser(): Promise<Browser> { | ||
| if (browser) { | ||
| return browser; | ||
| } | ||
| try { | ||
| browser = await puppeteer.launch({ | ||
| headless: true, | ||
| args: ['--no-sandbox', '--disable-setuid-sandbox'], | ||
| }); | ||
| return browser; | ||
| } catch (error) { | ||
| console.error('Failed to launch Puppeteer browser:', error); | ||
| throw error; // Re-throw the error to be caught by the caller | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid duplicate browser instances on concurrent calls
getBrowser() can race and launch multiple browsers under load. Latch the launch with a shared Promise.
-import puppeteer, { type Browser, type Page } from 'puppeteer';
+import puppeteer, { type Browser, type Page } from 'puppeteer';
let browser: Browser | null = null;
+let launching: Promise<Browser> | null = null;
export async function getBrowser(): Promise<Browser> {
if (browser) {
return browser;
}
- try {
- browser = await puppeteer.launch({
- headless: true,
- args: ['--no-sandbox', '--disable-setuid-sandbox'],
- });
- return browser;
- } catch (error) {
- console.error('Failed to launch Puppeteer browser:', error);
- throw error; // Re-throw the error to be caught by the caller
- }
+ if (launching) return launching;
+ launching = puppeteer
+ .launch({
+ headless: true,
+ args: ['--no-sandbox', '--disable-setuid-sandbox'],
+ })
+ .then((b) => {
+ browser = b;
+ launching = null;
+ return b;
+ })
+ .catch((e) => {
+ launching = null;
+ console.error('Failed to launch Puppeteer browser:', e);
+ throw e;
+ });
+ return launching;
}
export async function closeBrowser(): Promise<void> {
if (browser) {
await browser.close();
browser = null;
}
}📝 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.
| export async function getBrowser(): Promise<Browser> { | |
| if (browser) { | |
| return browser; | |
| } | |
| try { | |
| browser = await puppeteer.launch({ | |
| headless: true, | |
| args: ['--no-sandbox', '--disable-setuid-sandbox'], | |
| }); | |
| return browser; | |
| } catch (error) { | |
| console.error('Failed to launch Puppeteer browser:', error); | |
| throw error; // Re-throw the error to be caught by the caller | |
| } | |
| } | |
| import puppeteer, { type Browser, type Page } from 'puppeteer'; | |
| let browser: Browser | null = null; | |
| let launching: Promise<Browser> | null = null; | |
| export async function getBrowser(): Promise<Browser> { | |
| if (browser) { | |
| return browser; | |
| } | |
| if (launching) { | |
| return launching; | |
| } | |
| launching = puppeteer | |
| .launch({ | |
| headless: true, | |
| args: ['--no-sandbox', '--disable-setuid-sandbox'], | |
| }) | |
| .then((b) => { | |
| browser = b; | |
| launching = null; | |
| return b; | |
| }) | |
| .catch((e) => { | |
| launching = null; | |
| console.error('Failed to launch Puppeteer browser:', e); | |
| throw e; | |
| }); | |
| return launching; | |
| } | |
| export async function closeBrowser(): Promise<void> { | |
| if (browser) { | |
| await browser.close(); | |
| browser = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/web-platform/web-mcp-server/src/createPuppeteer.ts around lines 5 to
19, getBrowser() can race under concurrent calls and launch multiple browser
instances; fix by introducing a shared Promise (e.g., browserLaunchPromise) that
is set before awaiting puppeteer.launch so all callers await the same promise,
assign the resolved Browser to the module-level browser variable, and
clear/reset the promise (and browser on failure) if launch throws; ensure the
function returns await browserLaunchPromise when a launch is in progress and
that errors are re-thrown after cleaning up the shared state.
| title: 'Get Screenshot', | ||
| description: 'run a lynx bundle on web platform and get a screenshot. ', | ||
| inputSchema: { | ||
| url: z.string().describe('The web preview URL of the current Lynx Page'), | ||
| }, | ||
| }, | ||
| async ({ url }) => { | ||
| const page = await createPuppeteerPage(url); |
There was a problem hiding this comment.
Validate and restrict URL inputs to mitigate SSRF
Constrain to valid URLs and, by default, localhost-only; allow override via env.
- inputSchema: z.object({
- url: z.string().describe('The web preview URL of the current Lynx Page'),
- }),
+ inputSchema: z.object({
+ url: z
+ .string()
+ .url()
+ .describe('The web preview URL of the current Lynx Page')
+ .refine((u) => {
+ try {
+ const { hostname } = new URL(u);
+ const allow = (process.env.ALLOW_REMOTE_URLS === 'true')
+ ? true
+ : ['localhost', '127.0.0.1', '[::1]'].includes(hostname);
+ return allow;
+ } catch { return false; }
+ }, 'URL not allowed by server policy'),
+ }),
@@
- inputSchema: z.object({
- url: z.string().describe('The web preview URL of the Lynx page'),
- }),
+ inputSchema: z.object({
+ url: z
+ .string()
+ .url()
+ .describe('The web preview URL of the Lynx page')
+ .refine((u) => {
+ try {
+ const { hostname } = new URL(u);
+ const allow = (process.env.ALLOW_REMOTE_URLS === 'true')
+ ? true
+ : ['localhost', '127.0.0.1', '[::1]'].includes(hostname);
+ return allow;
+ } catch { return false; }
+ }, 'URL not allowed by server policy'),
+ }),Also applies to: 42-49
1 participant
#1806
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Checklist