Turisanapo/heb 305 mcp add world time tool#172
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR bumps the MCP package to 0.2.0, refactors server initialization to create an MCP per request via a factory, adds a world-time tool for timezone-aware time data and business-hours checks, and updates the Calculator with a new method, signature change, and a renamed global variable. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant HTTP as HTTP Handler
participant Factory as createMcpServer()
participant MCP as McpServer
participant Tool as worldTimeTool
Client->>HTTP: POST /api/...
HTTP->>Factory: createMcpServer()
Factory->>MCP: initialize (version 0.2.0) & register tools
HTTP->>MCP: invoke handler for request
MCP->>Tool: call worldTimeTool.handler(input)
Tool-->>MCP: return per-timezone results (JSON)
MCP-->>HTTP: respond with tool output
HTTP-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mcp/src/index.ts (1)
36-42:⚠️ Potential issue | 🟠 MajorCreate
McpServeronce at app initialization, not per request.The MCP SDK's recommended pattern for stateless HTTP servers is to instantiate the
McpServeronce and register tools once, then create a newWebStandardStreamableHTTPServerTransportper incoming request. The current code creates both the server and transport on every request, which unnecessarily re-instantiates the server and re-registers tools (countLetterToolandcurrentTimestampTool) for each HTTP call.Move
createMcpServer()outside the request handler and reuse the single instance across requests:Suggested fix
const server = createMcpServer(); const createMcp = () => new Elysia() .use(opentelemetry(getOtelConfig("hebo-mcp"))) .use(logger({ level: logLevel })) .get("/", () => hello) .group("/aikit", (app) => app.mount("/", async (request) => { const transport = new WebStandardStreamableHTTPServerTransport(); await server.connect(transport); return transport.handleRequest(request); }), );
🤖 Fix all issues with AI agents
In `@apps/mcp/src/aikit/current-timestamp.ts`:
- Around line 14-20: The handler function uses new Date().toLocaleString with
the caller-supplied timezone which will throw a RangeError for invalid timezone
strings; wrap the toLocaleString call in a try/catch inside the handler (around
where formatted is created) to catch RangeError and return or throw a friendly
error response/message indicating the timezone is invalid (include the provided
timezone in the message), otherwise return the formatted value as before;
reference the handler function, the timezone parameter, and the formatted
variable when making this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/mcp/src/aikit/world-time.ts`:
- Around line 75-76: businessHours time parsing is brittle: update the Zod
schema that defines businessHours (the start and end fields) to validate the
HH:MM format (e.g. a regex like /^\d{2}:\d{2}$/ or a more permissive
/^\d{1,2}:\d{2}$/) so malformed values are rejected at validation, and then
harden the parsing in isBusinessHours (the code that currently uses
(businessHours?.start || "09:00").split(":").map(Number) and similar for end) to
first assert the string matches the expected pattern before splitting and to
handle invalid parse results (avoid relying on NaN yielding false); keep
defaults only when validation fails and return a sensible fallback or treat the
businessHours as invalid.
🧹 Nitpick comments (2)
apps/mcp/src/aikit/world-time.ts (2)
5-12: Ambiguous partial matches may surprise callers.
resolve("us")orresolve("east")will silently return the first alphabetical IANA match, which may not be the user's intent. Consider documenting this behavior or returning multiple candidates when the partial match is ambiguous.
64-72: Fragile offset calculation via locale-string round-trip.Constructing a
DatefromtoLocaleStringoutput (line 65, 68-69) is implementation-defined — the string format varies across runtimes and can silently produceInvalid Date. A safer approach is to extract the offset fromIntl.DateTimeFormatparts or use a well-known library.♻️ Suggested safer offset extraction
One alternative: use
formatToPartsto extract hour/minute in the target TZ vs UTC and compute the delta, avoiding any string→Date round-trip. For example:function getOffsetMinutes(date: Date, iana: string): number { const utcParts = new Intl.DateTimeFormat("en-US", { timeZone: "UTC", hour: "numeric", minute: "numeric", hour12: false, year: "numeric", month: "numeric", day: "numeric", }).formatToParts(date); const tzParts = new Intl.DateTimeFormat("en-US", { timeZone: iana, hour: "numeric", minute: "numeric", hour12: false, year: "numeric", month: "numeric", day: "numeric", }).formatToParts(date); const toMin = (parts: Intl.DateTimeFormatPart[]) => { const get = (t: string) => Number(parts.find(p => p.type === t)!.value); return get("day") * 1440 + get("hour") * 60 + get("minute"); }; let diff = toMin(tzParts) - toMin(utcParts); if (diff > 720) diff -= 1440; if (diff < -720) diff += 1440; return diff; }
turisanapo
changed the title
| config: { | ||
| description: | ||
| "Get current time for one or more locations. Supports IANA timezones (e.g., 'America/New_York') or partial city names (e.g., 'tokyo' → 'Asia/Tokyo').", | ||
| inputSchema: { |
There was a problem hiding this comment.
I liked the suggestion from ChatGPT to allow queries, at and hints which makes it much more powerful:
There was a problem hiding this comment.
Added at for querying specific points in time.
I would skip hints because the tool uses Intl.supportedValuesOf("timeZone"), which only provides IANA identifiers. There's no geodata, country mapping, or city database to resolve coordinates or disambiguate locations.
That intelligence should come from the AI layer above the tool. The LLM can interpret "Paris" in context and pass the correct IANA zone (Europe/Paris vs America/Chicago) to the tool. I would keep the tool simple and let the AI handle disambiguation, which I find a better approach.
There was a problem hiding this comment.
The hints are super important for the API, otherwise it becomes quite weak. LLMs are inherently bad at mapping data, they will hallucinate. There are libraries that provide the mappings. Just let your Cursor implement it.
|
@claude can you help review this PR? |
2 participants
Summary by CodeRabbit
New Features
Chores