Skip to content

Conversation

@wonderwhy-er
Copy link
Owner

@wonderwhy-er wonderwhy-er commented Nov 12, 2025

CodeAnt-AI Description

Emit mcp_client_initialized event after client info is set

What Changed

  • The server now emits an mcp_client_initialized analytics event immediately after a connecting client's name and version are recorded during initialization
  • Startup analytics and telemetry will receive the actual client name/version instead of an "uninitialized" placeholder, fixing incorrect client labels in recorded events

Impact

✅ Accurate client name/version in startup analytics
✅ Fewer analytics events labeled "uninitialized"
✅ Clearer client-version data for telemetry and diagnostics

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

  • Chores
    • Added telemetry instrumentation to track initialization events for monitoring purposes.

…e event immediately after currentClient is populated with actual values\n- Event now includes correct client_name and client_version in analytics payload\n- Added client_info_available property to track edge cases where clientInfo is missing\n- Resolves timing issue where run_server_start captured 'uninitialized' client name
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Nov 12, 2025

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

A telemetry capture event (mcp_client_initialized) is added to the InitializeRequest handler in src/server.ts, emitted after the currentClient is updated from the request payload.

Changes

Cohort / File(s) Summary
Telemetry capture
src/server.ts
Added capture('mcp_client_initialized') call in InitializeRequest handler following currentClient update

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single, straightforward telemetry addition with no logic changes
  • Verify the telemetry event name aligns with tracking conventions
  • Confirm placement after currentClient update is the intended sequence

Possibly related PRs

  • PR #176: Established the client initialization tracking infrastructure and currentClient storage mechanism in the InitializeRequest handler, providing the foundation that this PR extends with telemetry capture.

Poem

🐰 A ping across the wire so bright,
Tracking clients through the night,
MCP awakes with metrics clear,
Every init we hold most dear! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Feature/mcp client initialized event' directly corresponds to the main change: adding a telemetry capture event ('mcp_client_initialized') in the InitializeRequest handler.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/mcp-client-initialized-event

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codeant-ai codeant-ai bot added the size:XS This PR changes 0-9 lines, ignoring generated files label Nov 12, 2025
deferLog('info', `Client connected: ${currentClient.name} v${currentClient.version}`);
}

capture('mcp_client_initialized');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Include the current client name/version when emitting the initialization analytics event. [enhancement]

Severity Level: Minor ⚠️

Suggested change
capture('mcp_client_initialized');
capture('mcp_client_initialized', {
client: {
name: currentClient?.name || 'unknown',
version: currentClient?.version || 'unknown'
}
});
Why it matters? ⭐

The suggestion is valid and low-risk: adding client name/version to the initialization event gives richer analytics and is straightforward to implement.
Note: src/utils/capture.ts already already enriches events with client context (it reads and injects currentClient -> client_name/client_version), so this change will duplicate that data in the event payload rather than fix a bug. Duplication isn't harmful because captureBase sanitizes and filters properties before sending, but it means the improvement is mainly cosmetic/analytics-related (better explicitness) rather than required to make telemetry work.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/server.ts
**Line:** 134:134
**Comment:**
	*Enhancement: Include the current client name/version when emitting the initialization analytics event.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

@codeant-ai
Copy link
Contributor

codeant-ai bot commented Nov 12, 2025

CodeAnt AI finished reviewing your PR.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/server.ts (1)

134-135: LGTM! Telemetry addition is well-placed.

The mcp_client_initialized event is correctly placed after client initialization setup and within error handling. The fire-and-forget pattern (not awaiting) is acceptable.

Optional improvements for consistency:

  1. Consider awaiting the capture call to ensure telemetry is sent before the response returns, matching the pattern used for other successful operations (e.g., lines 1066, 1087, 1106):
-        capture('mcp_client_initialized');
+        await capture('mcp_client_initialized');
  1. Consider including client metadata as properties for richer analytics (ensuring no PII is included):
-        await capture('mcp_client_initialized');
+        await capture('mcp_client_initialized', {
+            client_name: currentClient.name,
+            has_version: !!currentClient.version
+        });

These are minor consistency improvements rather than issues—the current implementation works correctly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a82cf16 and fe44727.

📒 Files selected for processing (1)
  • src/server.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.
📚 Learning: 2025-05-25T16:02:52.184Z
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.

Applied to files:

  • src/server.ts
🧬 Code graph analysis (1)
src/server.ts (1)
src/utils/capture.ts (1)
  • capture (277-284)

@wonderwhy-er wonderwhy-er merged commit 3b4c089 into main Nov 13, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants