chore: Add tracing start/stop tools and enhance global switch management#2059
chore: Add tracing start/stop tools and enhance global switch management#2059KananSu wants to merge 6 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d120290 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 StartTracing and EndTracing tools and registers them; updates connector types and EventMap for tracing events; changes GetGlobalSwitch response shape to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-06T01:19:23.670ZApplied to files:
📚 Learning: 2025-11-06T01:17:11.892ZApplied to files:
🧬 Code graph analysis (1)packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (2)
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
🧹 Nitpick comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (2)
25-41: Consider simplifying the debug mode check.The
ensureLynxConnectedfunction already enablesenable_debug_modefor connected clients. This additional check handles the edge case where debug mode was disabled after connection, but could be simplified. The current approach is defensive and provides good user feedback.
63-68: Preferincludes()overindexOf() >= 0for readability.Using
includes()is more idiomatic in modern JavaScript/TypeScript.🔎 Suggested fix
if ( - msg.indexOf('Failed to get trace controller') >= 0 - || msg.indexOf('Not implemented:') >= 0 - || msg.indexOf('Tracing not enabled') >= 0 - || msg.indexOf('Failed to start tracing') >= 0 + msg.includes('Failed to get trace controller') + || msg.includes('Not implemented:') + || msg.includes('Tracing not enabled') + || msg.includes('Failed to start tracing') ) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/angry-insects-care.mdpackages/mcp-servers/devtool-mcp-server/src/connector.tspackages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
📚 Learning: 2025-12-26T05:10:01.595Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T05:10:01.595Z
Learning: Use changeset tool with `pnpm changeset` to document changes before contribution
Applied to files:
.changeset/angry-insects-care.md
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
Repo: lynx-family/lynx-stack PR: 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/angry-insects-care.md
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/angry-insects-care.md
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
.changeset/angry-insects-care.mdpackages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
.changeset/angry-insects-care.mdpackages/mcp-servers/devtool-mcp-server/src/index.ts
🔇 Additional comments (13)
.changeset/angry-insects-care.md (1)
1-5: LGTM!The changeset correctly documents the new tracing tools as a minor version bump, which is appropriate for a feature addition.
packages/mcp-servers/devtool-mcp-server/src/connector.ts (5)
87-93: LGTM!The
TracingCompletetype and updatedEventMapentries correctly support the new tracing events and maintain backward compatibility with the existing global switch response format.
113-119: LGTM!The
Tracing.tracingCompleteevent entry follows the established pattern for CDP events and correctly uses theTracingCompletetype.
149-156: LGTM!The resolution logic correctly accesses
result?.messageto match the updated response payload structure.
467-477: LGTM!The
offCDPEventmethod correctly mirrors theonCDPEventsignature and enables proper cleanup of event listeners, which is essential for the tracing workflow.
511-522: LGTM!The
enable_debug_modehandling follows the established pattern forenable_devtool, ensuring debug mode is enabled automatically when clients connect.packages/mcp-servers/devtool-mcp-server/src/index.ts (2)
41-42: LGTM!The imports for the new tracing tools follow the established pattern.
88-92: LGTM!The new tracing tools are properly registered in the
TOOLSarray with a clear section comment, following the existing organizational pattern.packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (1)
13-21: LGTM!Tool definition follows the established pattern with appropriate naming, description, and annotations.
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (4)
1-15: LGTM!Imports and type definition are appropriate for the file I/O operations required by this tool.
17-35: LGTM!Tool definition and the
sendIOReadMessagehelper are well-structured with an appropriate chunk size.
61-84: LGTM!The
writeDataToFilefunction properly handles stream events and ensures the write stream is closed correctly.
91-133: LGTM on the async flow logic.The event handling flow is well-structured: register listener → send CDP message → wait for completion. The
finallyblock ensures proper cleanup, and client ID filtering prevents cross-client interference.
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
a01ea11 to
3517d4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (1)
62-67: Consider usingincludes()instead ofindexOf()for readability.Modern JavaScript/TypeScript style prefers
includes()for substring checks as it's more readable and returns a boolean directly.🔎 Suggested refactor
if ( - msg.indexOf('Failed to get trace controller') >= 0 - || msg.indexOf('Not implemented:') >= 0 - || msg.indexOf('Tracing not enabled') >= 0 - || msg.indexOf('Failed to start tracing') >= 0 + msg.includes('Failed to get trace controller') + || msg.includes('Not implemented:') + || msg.includes('Tracing not enabled') + || msg.includes('Failed to start tracing') ) {packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
55-78: Consider handling write stream backpressure.
writeStream.write()returnsfalsewhen the internal buffer is full, indicating the caller should wait for the'drain'event before writing more data. For large trace files, ignoring this could lead to increased memory usage.🔎 Suggested approach using pipeline
For a simpler and more robust solution, consider using
stream.Readable.from()withpipeline():import { pipeline } from 'node:stream/promises'; import { Readable } from 'node:stream'; const writeDataToFile = async ( dataChunks: Buffer[], filePath: string, ): Promise<void> => { const readable = Readable.from(dataChunks); const writeStream = fs.createWriteStream(filePath); await pipeline(readable, writeStream); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/angry-insects-care.mdpackages/mcp-servers/devtool-mcp-server/src/connector.tspackages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/angry-insects-care.md
- packages/mcp-servers/devtool-mcp-server/src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
🧬 Code graph analysis (3)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (2)
packages/mcp-servers/devtool-mcp-server/src/index.ts (1)
defineTool(135-135)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (4)
packages/mcp-servers/devtool-mcp-server/src/index.ts (1)
defineTool(135-135)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)packages/mcp-servers/devtool-mcp-server/src/connector.ts (1)
TracingComplete(87-89)packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (1)
clientId(6-10)
packages/mcp-servers/devtool-mcp-server/src/connector.ts (2)
packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (2)
clientId(6-10)sessionId(22-26)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)
🔇 Additional comments (16)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (4)
1-11: LGTM!The imports and type definition are appropriate. The
TracingStartResponsetype correctly models the CDP error response structure.
22-41: LGTM!The Android-specific debug mode handling logic is well-structured. The flow correctly:
- Checks if the client is Android
- Gets the current
enable_debug_modestate- Enables it if not already enabled
- Returns early with a restart prompt (since the mode change requires an app restart)
This aligns with the learning that the devtool-mcp-server intentionally provides clear feedback for corrective action.
42-49: Previous concern addressed - trace config is now valid.The
excludedCategories: ['*']has been removed, resolving the contradictory configuration. The config now correctly uses onlyincludedCategories: ['*']to capture all trace categories.
62-70: Previous concern addressed - URL is now platform-agnostic.The documentation URL no longer includes the
?platform=iosquery parameter, making it appropriate for all platforms.packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (5)
1-15: LGTM!The imports are appropriate for the file operations and CDP integration. The
IOReadResponsetype correctly models the expected CDP response forIO.read.
36-53: LGTM!The
readStreamDatafunction correctly implements chunked reading with EOF detection. The error handling for missing data is appropriate.
79-92: Previous concerns addressed - ordering is now correct.The
Promise.withResolvers()call is now correctly placed before thesetTimeout, soresolveis in scope when the timer fires. The timeout handler also now removes the event listener before resolving.However, note that the timeout handler calls
offCDPEventdirectly (line 85) while thecleanupfunction also clears the timer (line 91). For consistency and to ensureclearTimeout(timer)is called in the timeout path (though technically unnecessary since the timer already fired), you could callcleanup()instead:const timer = setTimeout(() => { response.appendLines( 'Loading trace data timeout, please try again later!', ); - connector.offCDPEvent('Tracing.tracingComplete', handleTraceComplete); + cleanup(); resolve(); }, 8000);This is a minor consistency improvement.
93-125: LGTM!The
handleTraceCompleteevent handler correctly:
- Filters by
clientIdto handle only relevant events- Reads the stream data and writes to a temp file
- Provides informative success/error messages
- Calls
cleanup()andresolve()in thefinallyblock to ensure proper resource cleanup
127-135: LGTM!The event registration and CDP message sending are correctly ordered - the listener is registered before sending
Tracing.endto ensure the completion event is not missed.packages/mcp-servers/devtool-mcp-server/src/connector.ts (7)
87-89: LGTM!The
TracingCompletetype is correctly defined and exported, matching the expected CDPTracing.tracingCompleteevent payload structure with a stream handle.
113-119: LGTM!The
CDP-event-Tracing.tracingCompleteevent is correctly added to theEventMapwith the appropriate tuple type signature, consistent with other CDP events in the map.
121-121: LGTM!The
GlobalKeytype is correctly extended to include'enable_debug_mode', which aligns with the new tracing functionality requirements.
146-157: LGTM!The
getGlobalSwitchmethod correctly handles the updated response format by checking forresult?.messagewhen the result is an object. The boolean resolution logic handles both string'true'and booleantruevalues appropriately.
467-477: LGTM!The
offCDPEventmethod correctly mirrors theonCDPEventmethod signature and implementation, providing proper event listener cleanup capability needed by theEndTracingtool.
511-522: LGTM!The
enable_debug_modehandling follows the same pattern asenable_devtool, maintaining consistency. The sequential get/set approach is appropriate for ensuring each switch is properly configured before proceeding.
52-56: Type change fromglobal_valuetomessageinGetGlobalSwitchResponseis correct.The
GetGlobalSwitchResponsetype has been updated to usemessageinstead ofglobal_value. This change is consistent throughout the codebase—line 152 correctly readsresult?.message, and the remainingglobal_valuereferences are for the distinctSetGlobalSwitchResponsetype, which appropriately usesglobal_value. No inconsistencies found.
KananSu
changed the title
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
6a96033 to
9d98767
Compare
…ent**
- Introduced `StartTracing` and `EndTracing` tools to the devtool-mcp-server, enabling trace event collection control via MCP server.
- `StartTracing`: Starts trace event collection, checks and enables debug mode for Android, and provides user feedback based on result.
- `EndTracing`: Stops trace event collection, reads trace data from stream, saves it to a temporary file, and returns the file path or error.
- Registered the new tracing tools in the MCP server tool list.
- Extended global switch management:
- Added support for `enable_debug_mode` global key.
- Ensured both `enable_devtool` and `enable_debug_mode` are enabled when connecting.
- Updated response types and internal logic to use `{ message: ... }` instead of `{ global_value: ... }` for global switch responses.
- Added `offCDPEvent` method for event listener cleanup.
- Updated event map to support `Tracing.tracingComplete`.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
9d98767 to
f16f391
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
72-76: Consider handling write stream backpressure.The current implementation writes all chunks synchronously without checking the return value of
writeStream.write(). If the write buffer fills up, this could cause increased memory usage. For trace files that may be large, consider handling backpressure by waiting for the'drain'event whenwrite()returnsfalse.🔎 Optional fix with backpressure handling
- for (const chunk of dataChunks) { - writeStream.write(chunk); - } - - writeStream.end(); + const writeNext = (index: number) => { + while (index < dataChunks.length) { + const ok = writeStream.write(dataChunks[index]); + index++; + if (!ok) { + writeStream.once('drain', () => writeNext(index)); + return; + } + } + writeStream.end(); + }; + writeNext(0);
102-104: Consider validatingdata.streambefore use.The
data.streamis accessed directly without checking if it's defined. If the CDP event delivers aTracingCompletepayload without a validstreamproperty, this will cause a runtime error that may produce an unclear message.🔎 Suggested validation
try { const stream = data.stream; + if (stream === undefined || stream === null) { + throw new Error('TracingComplete event missing stream handle'); + } const dataChunks = await readStreamData(stream);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/angry-insects-care.mdpackages/mcp-servers/devtool-mcp-server/src/connector.tspackages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/angry-insects-care.md
- packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/index.tspackages/mcp-servers/devtool-mcp-server/src/connector.ts
🧬 Code graph analysis (1)
packages/mcp-servers/devtool-mcp-server/src/connector.ts (2)
packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (2)
clientId(6-10)sessionId(22-26)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)
🔇 Additional comments (13)
packages/mcp-servers/devtool-mcp-server/src/index.ts (2)
41-42: LGTM!The imports for
StartTracingandEndTracingfollow the existing import conventions and path structure.
88-91: LGTM!The new Tracing section is well-organized and follows the existing pattern of grouping tools by domain (CSS, Debugger, Device, DOM, etc.). The registration order is consistent with the import order.
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (4)
1-15: LGTM!License header, imports, and type definition are appropriate. The
IOReadResponsetype correctly models the CDP IO.read response structure.
36-53: LGTM!The
readStreamDatafunction correctly handles streaming with EOF detection and throws a descriptive error when data is unavailable, aligning with the pattern of providing clear feedback to automation tools. Based on learnings, this intentional error throwing is the expected behavior.
79-92: LGTM!The
Promise.withResolversis correctly placed before the timer creation (addressing the previous review concern). The timeout handler properly removes the event listener before resolving, and the cleanup function is correctly structured for use in the event handler's finally block.
127-135: LGTM!The implementation correctly registers the event listener before sending the
Tracing.endCDP message, ensuring the completion event won't be missed. The promise-based flow properly coordinates the async event handling.packages/mcp-servers/devtool-mcp-server/src/connector.ts (7)
52-56: LGTM!The
GetGlobalSwitchResponsetype correctly accommodates the new response format wheredatacan be a primitive (string | boolean) or an object with amessagefield. This aligns with the updated handling logic ingetGlobalSwitch.
87-89: LGTM!The
TracingCompletetype is appropriately minimal, exporting just thestreamhandle needed by theEndTracingtool.
113-118: LGTM!The
CDP-event-Tracing.tracingCompleteevent entry follows the established pattern of other CDP events in theEventMap, with appropriate typing for error, payload, clientId, and sessionId.
121-121: LGTM!The
GlobalKeyunion type is appropriately expanded to include'enable_debug_mode', enabling type-safe usage of the new global switch throughout the codebase.
149-156: LGTM!The updated handler correctly accesses
result?.messageinstead of the previousglobal_value, and properly handles both string ('true') and boolean (true) representations of the value.
467-477: LGTM!The
offCDPEventmethod is a well-designed mirror ofonCDPEvent, providing proper event listener cleanup capability. This is essential for the timeout handling inEndTracingto prevent memory leaks.
511-522: LGTM!The
enable_debug_modehandling follows the same established pattern asenable_devtooland is correctly placed in the client connection flow. The optional chaining onconnectorprovides safety in edge cases.
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
53-78: Consider handling write stream backpressure for large traces.
writeStream.write(chunk)at line 68 doesn't check the return value. When the internal buffer is full,write()returnsfalse, and you should wait for the'drain'event before writing more data. For very large traces, this could cause memory buildup.🔎 Suggested improvement with backpressure handling
const readAndWrite = async () => { let hasEnd = false; while (!hasEnd) { try { const message = await sendIOReadMessage(stream); if (!message.data) { reject( new Error('Failed to read trace data: no data available'), ); return; } const chunk = Buffer.from(message.data, 'base64'); - writeStream.write(chunk); + const canContinue = writeStream.write(chunk); + if (!canContinue) { + await new Promise<void>((r) => writeStream.once('drain', r)); + } hasEnd = message.eof; } catch (error) { writeStream.destroy(); reject(error); return; } } writeStream.end(); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🔇 Additional comments (5)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (5)
1-15: LGTM!Imports are well-organized using the
node:prefix for built-in modules, and the localIOReadResponsetype is appropriately scoped.
17-25: LGTM!Tool definition follows the established pattern with appropriate metadata and annotations.
83-96: LGTM!The
Promise.withResolverspattern is correctly used, and the timeout handler properly cleans up the CDP event listener before resolving. The cleanup function provides comprehensive resource cleanup for the success path.
97-128: LGTM!The handler correctly filters by
clientId, uses atry/catch/finallypattern for robust error handling, and ensures cleanup always occurs. The temp file naming with timestamp is appropriate.
130-137: LGTM!The event registration before sending
Tracing.endensures no race condition. The returned promise correctly awaits either the completion event or timeout.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
41df9bd to
1362602
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
28-35: Critical: Stream handle type should bestring, notnumber.The Chrome DevTools Protocol specifies
IO.StreamHandleas a string type. Usingnumberhere creates a type mismatch with the CDP specification. This issue affects both the function parameter and the upstreamTracingComplete.streamtype definition inconnector.ts(lines 86-88).🔎 Proposed fix
Update the parameter type:
- const sendIOReadMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOReadMessage = (stream: string): Promise<IOReadResponse> => { return connector.sendCDPMessage<IOReadResponse>( params.clientId, -1, 'IO.read', { handle: stream, size: 1024 * 1024 }, ); };Also update
TracingCompleteinpackages/mcp-servers/devtool-mcp-server/src/connector.ts:export type TracingComplete = { stream: string; };Run this script to verify all usages of TracingComplete.stream:
#!/bin/bash # Find all references to TracingComplete and stream usage rg -n "TracingComplete|\.stream" packages/mcp-servers/devtool-mcp-server/src/ -C 2
🧹 Nitpick comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
36-82: Consider using streaming API instead of buffering.The current implementation reads trace data in chunks and buffers them through memory before writing to disk. As suggested in previous review, using the Web Streams API (
ReadableStreamwithpipeTo) would be more memory-efficient for potentially large trace files.🔎 Proposed streaming implementation (based on previous feedback)
Replace the current buffering approach with:
const readStreamDataToFile = async ( stream: string, // Note: also fix type to string filePath: string, ): Promise<void> => { const sourceStream = new ReadableStream<Uint8Array>({ async pull(controller) { try { const message = await sendIOReadMessage(stream); if (message.data) { controller.enqueue(Buffer.from(message.data, 'base64')); } if (message.eof) { controller.close(); } } catch (err) { controller.error(err); } }, }); const fileWritable = Writable.toWeb(fs.createWriteStream(filePath)); await sourceStream.pipeTo(fileWritable); };You'll need to import
Writablefromnode:stream:import { Writable } from 'node:stream';Based on learnings, this approach was previously suggested by colinaaa.
107-107: Add validation for stream handle.While the type issue needs to be fixed separately, consider adding validation to ensure the stream handle exists before attempting to read from it.
🔎 Suggested validation
try { const stream = data.stream; + + if (!stream) { + throw new Error('No stream handle provided in tracing complete event'); + } const tempDir = os.tmpdir();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧬 Code graph analysis (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (3)
packages/mcp-servers/devtool-mcp-server/src/index.ts (1)
defineTool(135-135)packages/mcp-servers/devtool-mcp-server/src/connector.ts (1)
TracingComplete(87-89)packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (1)
clientId(6-10)
🔇 Additional comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
83-96: LGTM: Promise ordering and cleanup logic are correct.The previously reported issues have been properly addressed:
Promise.withResolversis now called before the timeout (line 83 before line 85)- The timeout handler correctly calls cleanup before resolving (line 89)
- The cleanup function properly removes the event listener and clears the timer
130-137: LGTM: Event handling flow is correct.The event listener is properly registered before sending the
Tracing.endcommand, ensuring no race condition. The promise-based return allows proper async handling of the trace completion.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
1362602 to
c1b943c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (1)
72-80: URL hardcoded to iOS platform despite Android check.The handler explicitly checks for Android clients (line 35), but the error message at line 79 always links to the iOS documentation (
?platform=ios). This is inconsistent and misleading for Android users.🔎 Suggested fix
if ( msg.includes('Failed to get trace controller') || msg.includes('Not implemented:') || msg.includes('Tracing not enabled') || msg.includes('Failed to start tracing') ) { + const platform = client?.info.query.os === 'Android' ? 'android' : 'ios'; throw new Error( - 'Tracing functionality is not supported in the current version. Please integrate the Lynx development version (with -dev suffix) to enable tracing. For more information, visit: https://lynxjs.org/en/guide/start/integrate-lynx-dev-version.html?platform=ios', + `Tracing functionality is not supported in the current version. Please integrate the Lynx development version (with -dev suffix) to enable tracing. For more information, visit: https://lynxjs.org/en/guide/start/integrate-lynx-dev-version.html?platform=${platform}`, ); } else {packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
29-29: Critical: Stream handle type mismatch with CDP protocol specification.According to the Chrome DevTools Protocol specification,
IO.StreamHandleis astringtype, notnumber. BothsendIOReadMessage(line 29) andreadStreamDataToFile(line 38) incorrectly declare thestreamparameter asnumber. Additionally, theTracingCompletetype imported fromconnector.ts(line 7) definesstream: number, which should be updated tostream: stringto match the CDP specification.🔎 Suggested fix
Update the type signatures in this file:
- const sendIOReadMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOReadMessage = (stream: string): Promise<IOReadResponse> => { return connector.sendCDPMessage<IOReadResponse>(const readStreamDataToFile = async ( - stream: number, + stream: string, filePath: string, ): Promise<void> => {Also update
TracingCompleteinpackages/mcp-servers/devtool-mcp-server/src/connector.ts:export type TracingComplete = { - stream: number; + stream: string; };Also applies to: 38-38
🧹 Nitpick comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
66-72: Use cleanup() function for consistency.The timeout handler manually calls
connector.offCDPEvent(line 70) andresolve()(line 71), but doesn't use thecleanup()function defined at lines 74-77. For better maintainability and consistency with the success path (which callscleanup()at line 106), consider refactoring to usecleanup()here as well.🔎 Suggested fix
const timer = setTimeout(() => { response.appendLines( 'Loading trace data timeout, please try again later!', ); - connector.offCDPEvent('Tracing.tracingComplete', handleTraceComplete); + cleanup(); resolve(); }, 8000);Note:
clearTimeout(timer)insidecleanup()will be a no-op when called from within the timeout callback, but this improves code consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧬 Code graph analysis (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (3)
packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)packages/mcp-servers/devtool-mcp-server/src/connector.ts (1)
TracingComplete(87-89)packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (1)
clientId(6-10)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (2)
packages/mcp-servers/devtool-mcp-server/src/index.ts (1)
defineTool(135-135)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)
🔇 Additional comments (7)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (3)
14-28: LGTM! Well-designed tool schema with clear parameter descriptions.The tool definition includes sensible defaults for JSProfileType ('quickjs'), enableSystrace (true), and JSProfileInterval (100ms), with helpful descriptions for each parameter.
32-50: LGTM! Proper handling of Android debug mode prerequisite.The code correctly checks and enables
enable_debug_modefor Android clients before starting tracing, with a clear error message prompting the user to restart the app.
51-59: LGTM! Trace configuration is now correct.The contradictory configuration issue (having both
includedCategories: ['*']andexcludedCategories: ['*']) has been resolved. The current configuration correctly traces all categories with sensible defaults for buffer size (200KB) and compression enabled.packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (4)
18-26: LGTM! Clean tool definition.The EndTracing tool is properly defined with a minimal schema (only clientId required) and appropriate annotations.
37-63: LGTM! Excellent streaming implementation.The code correctly implements streaming using
ReadableStreamwith a pull controller and pipes data directly to the file without caching in memory. This addresses the past review concern about memory efficiency and follows modern Node.js streaming patterns.
78-109: LGTM! Robust event handler with proper cleanup.The
handleTraceCompletecallback correctly filters events by clientId, handles both success and error cases, and ensures cleanup is performed in the finally block. The try-catch-finally pattern provides comprehensive error handling.
111-119: LGTM! Clean async coordination.The main handler logic properly registers the event listener before sending the
Tracing.endcommand and returns a promise that resolves when either the trace completes or the timeout fires. The async flow is well-structured.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
c1b943c to
b1bf6ed
Compare
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
Outdated
Show resolved
Hide resolved
| response.appendLines( | ||
| 'Loading trace data timeout, please try again later!', | ||
| ); |
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
29-45: Fix stream handle type: should bestring, notnumber.According to the Chrome DevTools Protocol specification,
IO.StreamHandleis a string type. The current implementation incorrectly usesnumberfor thestreamparameter in bothsendIOReadMessage(line 29) andsendIOCloseMessage(line 38). This type mismatch could cause runtime issues when the CDP returns a string handle.🔎 Fix the stream parameter type
- const sendIOReadMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOReadMessage = (stream: string): Promise<IOReadResponse> => { return connector.sendCDPMessage<IOReadResponse>( params.clientId, -1, 'IO.read', { handle: stream, size: 1024 * 1024 }, ); }; - const sendIOCloseMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOCloseMessage = (stream: string): Promise<IOReadResponse> => { return connector.sendCDPMessage<IOReadResponse>( params.clientId, -1, 'IO.close', { handle: stream }, ); };Note: You'll also need to update the
readStreamDataToFileparameter type at line 47 and theTracingComplete.streamtype inconnector.ts(line 88 based on past comments) to maintain type consistency across the codebase.
🧹 Nitpick comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
81-96: Consider adding a comment to clarify the unhandled rejection suppression.The
promise.catch(() => { void 0; })pattern at lines 82-83 is intentionally used to prevent Node's unhandledRejection warnings (as noted in commit messages). While functionally correct, a brief comment explaining this intent would improve code clarity for future maintainers.🔎 Suggested comment
const { promise, resolve, reject } = Promise.withResolvers<void>(); + // Attach a no-op catch handler to mark the promise as handled and prevent + // Node's unhandledRejection warnings. Actual errors are handled via settle(). promise.catch(() => { void 0; });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🔇 Additional comments (9)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (4)
1-12: LGTM!Imports and type definitions are properly structured.
14-31: LGTM!Tool schema is well-defined with appropriate defaults and helpful descriptions for each parameter.
32-50: LGTM!Android debug mode check is correctly implemented. The tool properly enables the flag and provides clear feedback that the app needs to be restarted.
51-87: LGTM!Trace configuration and error handling are well-implemented. The config correctly includes all categories without contradictory exclusions, and error messages provide helpful guidance with documentation links.
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (5)
1-16: LGTM!Imports and type definitions are properly structured for streaming I/O operations.
18-26: LGTM!Tool definition is appropriately minimal for stopping trace collection.
46-80: LGTM!Streaming implementation is well-designed. The code properly reads trace data in chunks, decodes base64, streams to a file, and ensures the IO stream is closed in the
finallyblock even if errors occur.
98-106: LGTM!Timeout and cleanup logic is correctly implemented. The event listener is properly removed before settling the promise on timeout.
107-147: LGTM!Event handler and execution flow are well-structured. The code properly handles both success and error cases, ensures cleanup in all paths, and correctly filters events by clientId.
- Add sendIOCloseMessage helper to send IO.close for trace streams
- Wrap sourceStream.pipeTo in try/finally to always close the CDP IO handle
- Replace resolve-only promise with settle helper using resolve / reject and isSettled
- Change trace loading timeout to reject with a friendly error instead of resolving
- Route Tracing.tracingComplete success/failure paths through settle to avoid double-settle and surface errors
- Attach a no-op promise.catch(() => {}) to mark the internal promise as handled for Node’s unhandledRejection detection, without affecting the await /catch flow in index.ts
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
a2f29ab to
563395d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
29-47: Stream handle type should bestring, notnumber.Per CDP specification,
IO.StreamHandleis a string type. Thestreamparameter insendIOReadMessage,sendIOCloseMessage, andreadStreamDataToFileshould be typed asstringto match the protocol and theTracingComplete.streamtype (which should also bestringin connector.ts).🔎 Suggested fix
- const sendIOReadMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOReadMessage = (stream: string): Promise<IOReadResponse> => { return connector.sendCDPMessage<IOReadResponse>( params.clientId, -1, 'IO.read', { handle: stream, size: 1024 * 1024 }, ); }; - const sendIOCloseMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOCloseMessage = (stream: string): Promise<void> => { return connector.sendCDPMessage<IOReadResponse>( params.clientId, -1, 'IO.close', { handle: stream }, ); }; const readStreamDataToFile = async ( - stream: number, + stream: string, filePath: string, ): Promise<void> => {Also update
TracingComplete.streamtype inconnector.tsfromnumbertostring.#!/bin/bash # Check TracingComplete type definition in connector.ts rg -n "TracingComplete" packages/mcp-servers/devtool-mcp-server/src/connector.ts -A 5packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (1)
51-60: Contradictory trace configuration: both include and exclude all categories.The config specifies both
includedCategories: ['*']andexcludedCategories: ['*']. Per CDP semantics, exclusions typically take precedence, which could result in no categories being traced.If the intent is to trace all categories, remove
excludedCategories. If specific categories should be traced, replace the wildcards with explicit category lists.🔎 Suggested fix to trace all categories
const config = { recordMode: 'recordContinuously', includedCategories: ['*'], - excludedCategories: ['*'], enableSystrace: params.enableSystrace, bufferSize: 200 * 1024, JSProfileInterval: params.JSProfileInterval, JSProfileType: params.JSProfileType, enableCompress: true, };Chrome DevTools Protocol Tracing.start includedCategories excludedCategories wildcard precedence
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧬 Code graph analysis (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (2)
packages/mcp-servers/devtool-mcp-server/src/index.ts (1)
defineTool(135-135)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)
🔇 Additional comments (7)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (4)
81-96: Well-structured settle pattern to prevent double-resolution.The use of
Promise.withResolverswith anisSettledguard and the no-opcatchhandler to prevent unhandled rejection warnings is a clean approach. The settle function correctly routes to reject or resolve based on error presence.
98-106: Timeout and cleanup logic is correct.The timeout handler properly removes the event listener before settling with an error, and the cleanup function correctly clears both the listener and the timer. This addresses previous review concerns about potential memory leaks.
107-138: Handler logic is well-structured with proper error propagation.The
handleTraceCompletehandler correctly:
- Filters by clientId to handle only relevant events
- Uses try/catch/finally for proper error handling
- Calls cleanup in finally block to ensure resources are released
- Routes errors through the settle helper for proper rejection
71-79: Good use of try/finally for IO cleanup.The
sendIOCloseMessagecall in the finally block ensures the CDP IO handle is closed even ifpipeTofails. Silently catching the close error is acceptable since it's cleanup code.packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (3)
14-28: Schema definition is well-structured with sensible defaults.The tool schema exposes useful configuration options (JSProfileType, enableSystrace, JSProfileInterval) with appropriate defaults and clear descriptions. The use of Zod for schema validation is consistent with other tools.
35-49: Android debug mode handling is appropriate.The logic correctly checks and enables
enable_debug_modefor Android clients before tracing, and throws a clear error message prompting app restart when the mode needs to be enabled.
70-87: Error handling properly maps CDP errors to user-friendly messages.The error handling:
- Uses
String.prototype.includesfor message matching (as suggested in past reviews)- Throws errors instead of appending to response (as suggested)
- Maps specific CDP error patterns to helpful guidance with a documentation link
- Falls back to including the original error message for unexpected errors
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
Outdated
Show resolved
Hide resolved
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
Outdated
Show resolved
Hide resolved
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
2 times, most recently
from
a024c37 to
74047ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (1)
54-63: Contradictory trace configuration still present despite previous fix.Lines 56-57 still have both
includedCategories: ['*']andexcludedCategories: ['*'], which is contradictory. According to Chrome DevTools Protocol semantics, exclusions take precedence, so this configuration will trace no categories and render tracing non-functional.A previous review comment indicated this was "Addressed in commit 3517d4e", but the issue persists in the current code.
🔎 Proposed fix
To trace all categories, remove the
excludedCategoriesline:const config = { recordMode: 'recordContinuously', includedCategories: ['*'], - excludedCategories: ['*'], enableSystrace: params.enableSystrace, bufferSize: 200 * 1024, JSProfileInterval: params.JSProfileInterval, JSProfileType: params.JSProfileType, enableCompress: true, };Alternatively, if you need to trace only specific categories, replace
includedCategorieswith an explicit list and keepexcludedCategories: ['*'].
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:17:11.892Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/src/connector.ts:347-368
Timestamp: 2025-11-06T01:17:11.892Z
Learning: In the lynx-family/lynx-stack repository, for the devtool-mcp-server package, the getSource and getConsole methods intentionally throw errors when collectors don't exist (rather than returning empty arrays) to provide clear feedback to code agents and automation tools that something is wrong (e.g., debugger not enabled, collectors not initialized), enabling them to take corrective action.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧬 Code graph analysis (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts (2)
packages/mcp-servers/devtool-mcp-server/src/index.ts (1)
defineTool(135-135)packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
29-36: Fix stream handle type: should bestring, notnumber.The parameter type
stream: numbercontradicts the Chrome DevTools Protocol specification, which definesIO.StreamHandleas astringtype. Update this parameter and all related usages (includingsendIOCloseMessage,readStreamDataToFile, and theTracingComplete.streamtype in connector.ts) tostring.
38-45: Return type should bePromise<void>, notPromise<IOReadResponse>.The
IO.closeCDP method returns no payload. The function signature should returnPromise<void>instead ofPromise<IOReadResponse>.🔎 Suggested fix
- const sendIOCloseMessage = (stream: number): Promise<IOReadResponse> => { - return connector.sendCDPMessage<IOReadResponse>( + const sendIOCloseMessage = (stream: string): Promise<void> => { + return connector.sendCDPMessage<void>( params.clientId, -1, 'IO.close', { handle: stream }, ); };
🧹 Nitpick comments (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (1)
83-86: Consider callingcleanup()in timeout handler for consistency.The timeout handler removes the event listener but doesn't call
clearTimeout(timer). While not strictly necessary (since the timeout has already fired), callingcleanup()would be more consistent and make the cleanup logic centralized.🔎 Suggested refactor
const timer = setTimeout(() => { - connector.offCDPEvent('Tracing.tracingComplete', handleTraceComplete); + cleanup(); reject(new Error('Loading trace data timeout, please try again later!')); }, 8000);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧬 Code graph analysis (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (3)
packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)packages/mcp-servers/devtool-mcp-server/src/connector.ts (1)
TracingComplete(87-89)packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (1)
clientId(6-10)
🔇 Additional comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
46-80: Good use of streaming API.The implementation correctly uses
ReadableStreamandpipeToto stream trace data directly to the file system, avoiding memory accumulation. Thefinallyblock ensures the IO stream is closed even if an error occurs during the write.
92-148: Well-structured error handling and resource cleanup.The event handler correctly validates the clientId, manages the streaming trace file write, and ensures cleanup happens in all code paths. The specific error message rewrapping for unsupported versions and unstarted tracing provides helpful user feedback.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
74047ac to
89be0ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
29-36: Fix stream handle type: should bestring, notnumber.The Chrome DevTools Protocol specifies
IO.StreamHandleas a string type. The current implementation incorrectly usesnumberfor the stream parameter. This type mismatch affects this function,sendIOCloseMessage,readStreamDataToFile, and theTracingComplete.streamtype definition in connector.ts (line 87).🔎 Proposed fix
- const sendIOReadMessage = (stream: number): Promise<IOReadResponse> => { + const sendIOReadMessage = (stream: string): Promise<IOReadResponse> => { return connector.sendCDPMessage<IOReadResponse>( params.clientId, -1, 'IO.read', { handle: stream, size: 1024 * 1024 }, ); };Also update:
sendIOCloseMessageparameter (line 38)readStreamDataToFileparameter (line 47)TracingComplete.streamtype in connector.ts (line 87)Based on past review comments and CDP protocol specification.
38-45: Fix return type:IO.closereturnsPromise<void>, notPromise<IOReadResponse>.The CDP
IO.closemethod doesn't return a response payload withdataoreoffields. The return type should bePromise<void>.🔎 Proposed fix
- const sendIOCloseMessage = (stream: number): Promise<IOReadResponse> => { - return connector.sendCDPMessage<IOReadResponse>( + const sendIOCloseMessage = (stream: string): Promise<void> => { + return connector.sendCDPMessage<void>( params.clientId, -1, 'IO.close', { handle: stream }, ); };Based on past review comments.
🧹 Nitpick comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
74-78: Consider logging IO.close errors for observability.The
IO.closeerror is silently swallowed in the finally block. While this is acceptable for cleanup, consider logging the error for debugging purposes.🔎 Suggested improvement
} finally { try { await sendIOCloseMessage(stream); - } catch { - void 0; + } catch (err) { + // Ignore close errors during cleanup, but could log for debugging + // console.error('Failed to close IO stream:', err); } }
83-86: Consider callingcleanup()in the timeout handler for consistency.The timeout handler removes the event listener and rejects, but doesn't call
cleanup()which would also clear the timer. While clearing an already-fired timer is redundant, callingcleanup()would make the error path consistent with the success path.🔎 Suggested improvement
const timer = setTimeout(() => { - connector.offCDPEvent('Tracing.tracingComplete', handleTraceComplete); + cleanup(); reject(new Error('Loading trace data timeout, please try again later!')); }, 8000);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.tspackages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/StartTracing.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts
🧬 Code graph analysis (1)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (3)
packages/mcp-servers/devtool-mcp-server/src/McpContext.ts (1)
connector(20-22)packages/mcp-servers/devtool-mcp-server/src/connector.ts (1)
TracingComplete(87-89)packages/mcp-servers/devtool-mcp-server/src/schema/index.ts (1)
clientId(6-10)
🔇 Additional comments (2)
packages/mcp-servers/devtool-mcp-server/src/tools/Tracing/EndTracing.ts (2)
92-119: LGTM: Clean async event handler with proper cleanup.The handler correctly filters by clientId, streams trace data to a file, provides user feedback, and ensures cleanup in the finally block. Error handling properly rejects the promise as suggested in past reviews.
121-150: LGTM: Robust error handling with helpful user feedback.The CDP message handling properly cleans up on error and maps specific CDP error messages to user-friendly guidance. The promise-based control flow correctly rejects on failures and resolves on success.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
2 times, most recently
from
46e9fab to
ffd4fef
Compare
…ls to prevent hanging on device disconnection Add timeout mechanisms to StartTracing and EndTracing to handle cases where sendCDPMessage doesn't return due to device disconnection or USB communication blockage. This ensures tool handlers return within 8 seconds instead of hanging indefinitely, improving system stability and user experience.
KananSu
force-pushed
the
p/kanan/trace_mcp_tools
branch
from
ffd4fef to
c4b5232
Compare
2 participants
StartTracingandEndTracingtools to the devtool-mcp-server, enabling trace event collection control via MCP server.StartTracing: Starts trace event collection, checks and enables debug mode for Android, and provides user feedback based on result.EndTracing: Stops trace event collection, reads trace data from stream, saves it to a temporary file, and returns the file path or error.enable_debug_modeglobal key.enable_devtoolandenable_debug_modeare enabled when connecting.{ message: ... }instead of{ global_value: ... }for global switch responses.offCDPEventmethod for event listener cleanup.Tracing.tracingComplete.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist