feat(examples): Add weather app example with Open-Meteo API integration - #81
Conversation
gabrypavanello
commented
Jan 10, 2026
gabrypavanello
commented
- Create new weather-app example using create-app CLI scaffolding
- Implement three weather tools: getCurrentWeather, getForecast, getWeatherAlerts
- Integrate Open-Meteo API (free, no API key required) with mock fallback
- Add React UI components for displaying weather data, forecast, and alerts
- Include comprehensive test suite (33 tests) using @mcp-apps-kit/testing
- Support mock mode via USE_MOCK_WEATHER env variable for testing
- Create new weather-app example using create-app CLI scaffolding - Implement three weather tools: getCurrentWeather, getForecast, getWeatherAlerts - Integrate Open-Meteo API (free, no API key required) with mock fallback - Add React UI components for displaying weather data, forecast, and alerts - Include comprehensive test suite (33 tests) using @mcp-apps-kit/testing - Support mock mode via USE_MOCK_WEATHER env variable for testing
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a complete Weather App example: an MCP server with three tools (current weather, forecast, alerts), a WeatherService integrating Open‑Meteo with mock fallbacks, a React UI, project configs (Vite/TS/Vitest), and unit/integration tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as React UI
participant App as MCP Server
participant Service as WeatherService
participant API as Open‑Meteo
participant Mock as Mock Data
UI->>App: invoke tool (e.g., getCurrentWeather(location))
App->>Service: getCurrentWeather(location)
Service->>API: geocodeLocation(query)
alt Geocode success
API-->>Service: coordinates
Service->>API: fetchCurrentWeather(lat,lon)
API-->>Service: weather payload
Service-->>App: CurrentWeather (validated)
else Geocode/fetch failure
Service->>Mock: generateMockCurrentWeather()
Mock-->>Service: mock CurrentWeather
Service-->>App: CurrentWeather (mock)
end
App-->>UI: structuredContent (tool output)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
Code Review: Weather App ExampleSummaryThis PR adds a comprehensive weather application example demonstrating integration with the Open-Meteo API. The implementation is well-structured and follows the project's conventions. Overall, this is a high-quality contribution that serves as an excellent example for the framework. ✅ StrengthsArchitecture & Design
Code Quality
MCP Apps Kit Usage
🔧 Issues FoundCritical IssuesNone found. High Priority1. Type duplication in UI component ( Recommendation: Import types from server or create a shared types file: import type {
CurrentWeather,
WeatherForecast,
WeatherAlertsResponse
} from "../../server/services/weatherService.js";2. Mock data always uses same coordinates ( Recommendation: Generate varied mock coordinates based on location name hash or use a small lookup table. Medium Priority3. Missing error handling for invalid location input Recommendation: Add input validation: async getCurrentWeather(locationQuery: string): Promise<CurrentWeather> {
if (!locationQuery?.trim()) {
throw new AppError({
code: ErrorCode.VALIDATION_ERROR,
message: "Location query cannot be empty"
});
}
// ... rest of implementation
}4. Unconstrained random mock data ( Impact: Low - only affects testing/demo scenarios 5. Missing type guards for API responses ( Recommendation: Use Zod to validate API responses: const GeocodingResponseSchema = z.object({
results: z.array(z.object({
name: z.string(),
latitude: z.number(),
// ... rest of schema
})).optional()
});
const data = GeocodingResponseSchema.parse(await response.json());6. Race condition in mock alert IDs ( Recommendation: Use 7. Magic number in isDay calculation ( Impact: Low - acceptable for mock data Low Priority8. Missing JSDoc comments 9. Hard-coded port ( 10. CSS could use more modular approach 🛡️ Security Considerations✅ No API keys required - Good choice using Open-Meteo Minor concern: URL construction uses string interpolation ( const url = new URL("https://geocoding-api.open-meteo.com/v1/search");
url.searchParams.set("name", query);
url.searchParams.set("count", "1");📊 Test CoverageExcellent coverage with 33 tests across:
Minor gap: No tests for UI components. Consider adding React Testing Library tests for the display components. 🎯 Performance Considerations✅ Good: No expensive computations Example caching approach: private cache = new Map<string, { data: CurrentWeather, expires: number }>();
async getCurrentWeather(location: string): Promise<CurrentWeather> {
const cacheKey = `current:${location}`;
const cached = this.cache.get(cacheKey);
if (cached && Date.now() < cached.expires) {
return cached.data;
}
// ... fetch and cache with 5-minute TTL
}📝 Code ConventionsFollows Project Guidelines ✅
Minor Convention Issues
🚀 Suggested Enhancements (Optional)
📋 Checklist Before Merge
VerdictRecommendation: ✅ Approve with minor revisions This is a well-crafted example that demonstrates the framework effectively. The high-priority issues are relatively minor and can be addressed quickly. The code is production-quality for an example application. Estimated effort to address high-priority issues: ~30 minutes Great work! This will be a valuable addition to the examples collection. 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @examples/weather-app/ui/index.html:
- Around line 1-12: The HTML file (contains the DOCTYPE, <div id="root"></div>,
and the <script type="module" src="/src/main.tsx"> entry) has formatting issues
flagged by the pipeline; run the formatter to fix and commit the changes by
running: prettier --write examples/weather-app/ui/index.html, then verify the
file is properly formatted and include the updated file in your commit/pull
request.
In @examples/weather-app/ui/src/App.tsx:
- Around line 18-73: These interfaces (Location, CurrentWeather, DailyForecast,
WeatherForecast, WeatherAlert, WeatherAlertsResponse) are duplicates; remove
their local declarations and import the named type exports from the server
module that defines them instead. Replace the duplicated interface blocks with a
single type-only import for those exact symbols at the top of the file, and
update any local references to use the imported types; ensure the import uses
type-only syntax where supported so you don't pull runtime code.
In @examples/weather-app/ui/src/styles.css:
- Around line 1-411: The CSS file has formatting issues; run the project's
Prettier formatting on examples/weather-app/ui/src/styles.css (e.g. run
"prettier --write examples/weather-app/ui/src/styles.css"), review the resulting
changes across selectors like :root, .weather-card, .forecast-day, .actions,
etc., stage and commit the formatted file, and push the updated commit so the
pipeline passes.
🧹 Nitpick comments (11)
examples/weather-app/README.md (1)
69-85: Add language identifier to fenced code block.The project structure code block should have a language identifier for consistency. Use
textorplaintextfor directory trees.📝 Suggested fix
-``` +```text weather-app/ ├── server/examples/weather-app/ui/index.html (1)
6-6: Consider a more user-friendly title.The current title "weather-app" could be improved to "Weather App" for better user experience.
📝 Suggested improvement
- <title>weather-app</title> + <title>Weather App</title>examples/weather-app/tests/integration/server.test.ts (3)
17-18: Remove hardcoded delay or document its necessity.The 100ms delay after starting the test server may cause flakiness or unnecessary slowdown. If the server needs time to initialize, consider using a readiness check instead.
♻️ Alternative approach
If the delay is truly necessary, document why:
const server = await startTestServer(app, { port: 0 }); + // Wait for server to fully initialize internal state await new Promise((r) => setTimeout(r, 100));Or better yet, check if
startTestServeralready handles initialization:const server = await startTestServer(app, { port: 0 }); - await new Promise((r) => setTimeout(r, 100));
78-84: Consider importing types instead of inline type assertions.Throughout the test file, inline type assertions are used repeatedly. Consider importing the actual types from
weatherService.tsto improve type safety and reduce duplication.This pattern appears in lines 78-84, 101-104, 119-122, 135, 149-153, 169-171, 184-189, 205-209, 225-230, 246-250, and 266-276.
♻️ Refactoring suggestion
Add imports at the top:
import type { CurrentWeather, WeatherForecast, WeatherAlertsResponse } from "../../server/services/weatherService.js";Then simplify assertions:
- const content = result.structuredContent as { - location?: { name?: string }; - temperature?: number; - humidity?: number; - description?: string; - icon?: string; - }; + const content = result.structuredContent as CurrentWeather;
258-294: Test for alerts has low assertion value.The test runs up to 10 iterations to potentially find alerts, but ultimately accepts no alerts as valid (line 293:
expect(true).toBe(true)). This makes the test less valuable.Consider either:
- Using a deterministic mock that always returns alerts for testing
- Testing the no-alerts case explicitly in a separate test
- Removing the loop and accepting that alerts may not always be present
♻️ Suggested improvement
- it("should return valid alert structure when alerts exist", async () => { - // Run multiple times to get alerts (random in mock) - let alertsFound = false; - for (let i = 0; i < 10 && !alertsFound; i++) { - const result = await env.client.callTool("getWeatherAlerts", { - location: "Test City", - }); - - const content = result.structuredContent as { - alerts?: Array<{ - id?: string; - type?: string; - severity?: string; - headline?: string; - description?: string; - startTime?: string; - endTime?: string; - }>; - }; - - if (content.alerts && content.alerts.length > 0) { - alertsFound = true; - const alert = content.alerts[0]; - - expect(alert.id).toBeDefined(); - expect(["warning", "watch", "advisory"]).toContain(alert.type); - expect(["minor", "moderate", "severe", "extreme"]).toContain(alert.severity); - expect(alert.headline).toBeTruthy(); - expect(alert.description).toBeTruthy(); - expect(alert.startTime).toBeDefined(); - expect(alert.endTime).toBeDefined(); - } - } - - // It's ok if no alerts were found in mock mode - expect(true).toBe(true); - }); + it("should return valid alert structure when present", async () => { + const result = await env.client.callTool("getWeatherAlerts", { + location: "Test City", + }); + + expectToolResult(result).toHaveNoError(); + const content = result.structuredContent as WeatherAlertsResponse; + + // Verify structure regardless of whether alerts exist + expect(Array.isArray(content.alerts)).toBe(true); + + // If alerts exist, validate their structure + if (content.alerts.length > 0) { + const alert = content.alerts[0]; + expect(alert.id).toBeDefined(); + expect(["warning", "watch", "advisory"]).toContain(alert.type); + expect(["minor", "moderate", "severe", "extreme"]).toContain(alert.severity); + expect(alert.headline).toBeTruthy(); + expect(alert.description).toBeTruthy(); + expect(alert.startTime).toBeDefined(); + expect(alert.endTime).toBeDefined(); + } + });examples/weather-app/ui/src/App.tsx (2)
168-173: Add explicit typing for severityColors.The
severityColorsobject would benefit from explicit typing for better type safety.♻️ Suggested improvement
- const severityColors: Record<string, string> = { + const severityColors: Record<WeatherAlert["severity"], string> = { minor: "#ffc107", moderate: "#fd7e14", severe: "#dc3545", extreme: "#6f42c1", };
218-238: Type guards could be more robust.The type guard functions use minimal checks that could result in false positives. Consider more thorough validation.
For example,
isCurrentWeatheronly checks fortemperatureandhumidity, but other objects could have these properties. Similar issues exist inisForecastandisAlerts.♻️ More robust type guards
function isCurrentWeather(data: unknown): data is CurrentWeather { - return !!data && typeof data === "object" && "temperature" in data && "humidity" in data; + return ( + !!data && + typeof data === "object" && + "temperature" in data && + "humidity" in data && + "location" in data && + !("daily" in data) && + !("alerts" in data) + ); } function isForecast(data: unknown): data is WeatherForecast { return ( !!data && typeof data === "object" && "daily" in data && - Array.isArray((data as WeatherForecast).daily) + Array.isArray((data as WeatherForecast).daily) && + "location" in data && + !("temperature" in data) && + !("alerts" in data) ); } function isAlerts(data: unknown): data is WeatherAlertsResponse { return ( !!data && typeof data === "object" && "alerts" in data && - Array.isArray((data as WeatherAlertsResponse).alerts) + Array.isArray((data as WeatherAlertsResponse).alerts) && + "location" in data && + !("temperature" in data) && + !("daily" in data) ); }examples/weather-app/tests/unit/weatherService.test.ts (2)
120-144: Test may pass without actually verifying alert structure.If no alerts are generated within 20 iterations (possible with random behavior),
alertFoundremainsfalseand the test passes without validating the alert structure. Consider adding an assertion or increasing iterations to ensure meaningful coverage.♻️ Suggested improvement
} } + // Ensure we actually tested an alert structure + expect(alertFound).toBe(true); });
239-248: Consider usingArray.some()for cleaner emoji check.The manual loop can be simplified using array methods.
♻️ Suggested simplification
// Should have weather-related emojis const weatherEmojis = ["☀️", "🌤️", "⛅", "☁️", "🌧️", "🌦️"]; - let foundWeatherEmoji = false; - for (const icon of icons) { - if (weatherEmojis.includes(icon)) { - foundWeatherEmoji = true; - break; - } - } - expect(foundWeatherEmoji).toBe(true); + const foundWeatherEmoji = [...icons].some((icon) => weatherEmojis.includes(icon)); + expect(foundWeatherEmoji).toBe(true);examples/weather-app/server/index.ts (1)
10-15: Consider separating type-only imports.Per coding guidelines, type-only exports should use
export type. The same principle applies to imports—separating type imports improves tree-shaking and clarity.♻️ Suggested change
-import { - WeatherService, - type CurrentWeather, - type WeatherForecast, - type WeatherAlertsResponse, -} from "./services/weatherService.js"; +import { WeatherService } from "./services/weatherService.js"; +import type { + CurrentWeather, + WeatherForecast, + WeatherAlertsResponse, +} from "./services/weatherService.js";examples/weather-app/server/services/weatherService.ts (1)
93-128: Consider adding a timeout to fetch requests.The
fetchcalls don't have a timeout, which could cause requests to hang indefinitely if the Open-Meteo API is unresponsive. While the fallback to mock data provides resilience, a timeout would improve user experience.♻️ Suggested approach
// Using AbortController for timeout const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 5000); try { const response = await fetch(url, { signal: controller.signal }); // ... } finally { clearTimeout(timeoutId); }This pattern could be applied to all three fetch locations:
geocodeLocation,fetchCurrentWeather, andfetchForecast.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
examples/weather-app/.gitignoreexamples/weather-app/README.mdexamples/weather-app/package.jsonexamples/weather-app/server/index.tsexamples/weather-app/server/services/weatherService.tsexamples/weather-app/tests/integration/server.test.tsexamples/weather-app/tests/setup.tsexamples/weather-app/tests/unit/weatherService.test.tsexamples/weather-app/tsconfig.jsonexamples/weather-app/ui/index.htmlexamples/weather-app/ui/src/App.tsxexamples/weather-app/ui/src/main.tsxexamples/weather-app/ui/src/styles.cssexamples/weather-app/ui/vite.config.tsexamples/weather-app/vitest.config.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use Strict TypeScript mode: noanytypes, useunknownand narrow types instead
Remove all unused variables or prefix them with underscore (_)
Useexport typefor type-only exports
Files:
examples/weather-app/ui/vite.config.tsexamples/weather-app/tests/setup.tsexamples/weather-app/tests/unit/weatherService.test.tsexamples/weather-app/server/index.tsexamples/weather-app/tests/integration/server.test.tsexamples/weather-app/server/services/weatherService.tsexamples/weather-app/vitest.config.ts
**/tests/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files in
tests/directory mirroring source structure with subdirectories for unit/, integration/, and contract/ tests
Files:
examples/weather-app/tests/unit/weatherService.test.tsexamples/weather-app/tests/integration/server.test.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only through
index.tsfiles
Files:
examples/weather-app/server/index.ts
🧠 Learnings (4)
📚 Learning: 2026-01-09T14:18:43.516Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.516Z
Learning: Applies to **/tests/**/*.test.ts : Place test files in `tests/` directory mirroring source structure with subdirectories for unit/, integration/, and contract/ tests
Applied to files:
examples/weather-app/tests/setup.tsexamples/weather-app/tests/unit/weatherService.test.tsexamples/weather-app/tests/integration/server.test.tsexamples/weather-app/vitest.config.ts
📚 Learning: 2026-01-09T14:18:43.516Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.516Z
Learning: Applies to **/*.ts : Use `export type` for type-only exports
Applied to files:
examples/weather-app/tsconfig.json
📚 Learning: 2026-01-09T14:18:43.516Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.516Z
Learning: Applies to **/*.ts : Use Strict TypeScript mode: no `any` types, use `unknown` and narrow types instead
Applied to files:
examples/weather-app/tsconfig.json
📚 Learning: 2026-01-09T14:18:43.516Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.516Z
Learning: Applies to packages/core/**/*.ts : Always use `defineTool` and `defineUI` for type inference when creating tools and UI widgets
Applied to files:
examples/weather-app/tsconfig.json
🧬 Code graph analysis (4)
examples/weather-app/ui/src/main.tsx (1)
examples/weather-app/ui/src/App.tsx (1)
App(240-309)
examples/weather-app/ui/src/App.tsx (2)
examples/weather-app/server/services/weatherService.ts (6)
Location(36-42)CurrentWeather(44-56)DailyForecast(58-69)WeatherForecast(71-75)WeatherAlert(77-85)WeatherAlertsResponse(87-91)examples/weather-app/server/index.ts (1)
AppTools(199-199)
examples/weather-app/tests/setup.ts (1)
packages/testing/src/adapters/vitest.ts (1)
setupVitestMatchers(28-46)
examples/weather-app/tests/unit/weatherService.test.ts (1)
examples/weather-app/server/services/weatherService.ts (2)
WeatherService(344-416)mockWeatherService(422-422)
🪛 GitHub Actions: PR Check
examples/weather-app/ui/index.html
[error] 1-1: Prettier formatting check reported formatting issues. Run 'prettier --write' to fix code style issues in this file.
examples/weather-app/ui/src/styles.css
[error] 1-1: Prettier formatting check reported formatting issues. Run 'prettier --write' to fix code style issues in this file.
🪛 LanguageTool
examples/weather-app/README.md
[grammar] ~61-~61: Ensure spelling is correct
Context: ...ndpoint: http://localhost:3005/mcp Stdio mode (for hosts that support it): ``...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
examples/weather-app/README.md
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (20)
examples/weather-app/.gitignore (1)
1-6: LGTM!Standard and appropriate ignore patterns for a Node.js/TypeScript project with build artifacts and environment files correctly excluded.
examples/weather-app/tsconfig.json (1)
1-15: LGTM!Well-configured TypeScript setup with
strict: trueenabled as per coding guidelines. The include patterns correctly cover both server and UI source directories.examples/weather-app/vitest.config.ts (1)
1-12: LGTM!Clean Vitest configuration with appropriate test patterns and timeouts suitable for integration tests that may involve API calls.
examples/weather-app/tests/setup.ts (1)
1-7: LGTM!Correctly initializes MCP Apps Kit testing matchers for use in unit and integration tests.
examples/weather-app/package.json (1)
1-25: LGTM!Well-structured package configuration with appropriate scripts for development, build, and testing workflows. The workspace dependencies and ESM module type are correctly configured for this monorepo example.
examples/weather-app/README.md (1)
1-66: LGTM!Excellent documentation covering features, usage, development workflow, mock mode, and project structure. Clear and helpful for developers exploring this example.
examples/weather-app/ui/vite.config.ts (1)
1-12: LGTM!Clean Vite configuration with React plugin and
viteSingleFilefor generating a bundled single-file output, which is appropriate for MCP app embedded UIs. Therootpath is correctly set since the config is referenced from the project root via--config ui/vite.config.ts.examples/weather-app/ui/src/main.tsx (1)
1-20: LGTM! Clean React 18 entry point.The implementation correctly validates the root element, uses React 18's
createRoot, and properly wraps the application withStrictModeandAppsProvider.examples/weather-app/ui/src/App.tsx (1)
240-309: LGTM! Well-structured main component.The App component properly integrates MCP client hooks, applies theming, handles multiple data types gracefully with appropriate fallbacks, and provides user actions for follow-up interactions.
examples/weather-app/tests/unit/weatherService.test.ts (2)
1-7: LGTM! Test file structure and imports look correct.The test file is correctly placed in
tests/unit/following the coding guidelines, and imports are properly structured with.jsextension for ESM compatibility.
177-226: LGTM! Fallback behavior tests are well-structured.Good coverage of different failure scenarios: HTTP errors (500), network failures, and empty geocoding results. The mock setup correctly tests that the service gracefully falls back to mock data.
examples/weather-app/server/index.ts (3)
44-110: LGTM! Schema definitions are well-structured.The Zod schemas include helpful descriptions and properly constrain enum values. They align well with the TypeScript interfaces defined in the service.
121-185: LGTM! Tool definitions are comprehensive and well-documented.The tools have clear descriptions, proper input validation with Zod schemas, and appropriate
readOnlyHintannotations. Handler implementations correctly delegate to the WeatherService.
188-208: LGTM! Export structure and startup logic are well-designed.The type exports follow coding guidelines with
export type. The conditional startup based onNODE_ENVensures the server doesn't start during tests, and the namedappexport enables integration testing.examples/weather-app/server/services/weatherService.ts (6)
36-91: LGTM! Interface definitions are clean and well-typed.The interfaces properly use optional fields, union types for constrained values, and are all exported for use by consumers.
130-175: LGTM! Current weather fetch is well-implemented.The function properly handles API failures by returning
null, which the caller converts to mock data. The inline type assertion for the API response is appropriate for external API integration.
177-229: LGTM! Forecast fetch implementation is solid.The function cleanly transforms the Open-Meteo API response into the
DailyForecast[]structure, with proper fallback for unknown weather codes.
242-260: LGTM! Mock weather generator produces valid data.The random ranges for temperature, humidity, wind speed, and direction are realistic. The weather code selection from the hardcoded array ensures valid icons and descriptions.
343-416: LGTM! WeatherService class is well-designed with proper fallbacks.The class implements a clean fallback pattern: mock mode → real API → mock fallback on failure. The
safeDaysclamping for forecasts is good defensive programming, and the comment explaining why alerts always use mock data (Open-Meteo limitation) is helpful.
418-422: LGTM! Exported instances support both production and testing scenarios.Providing both a default instance and an explicit mock instance is a good pattern for the example app, supporting easy testing and demonstration.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5aa2ac70f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| beforeAll(async () => { | ||
| // Set mock mode for predictable tests | ||
| process.env.USE_MOCK_WEATHER = "true"; |
There was a problem hiding this comment.
Setting USE_MOCK_WEATHER in beforeAll happens after app has already been imported, but the server module reads process.env.USE_MOCK_WEATHER at import time when it constructs weatherService. That means these tests will still use the real Open‑Meteo API whenever the env var wasn’t set before module load, making the suite network‑dependent and flaky in offline CI. Consider setting the env var before importing app (e.g., via a dynamic import after the env assignment) or injecting a mock service into the app for tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
examples/weather-app/ui/index.html (1)
1-12: Optional: addcolor-scheme/theme-colormetadata (and a nicer title) for better UA integrationThis app supports light/dark themes in CSS; declaring it in HTML helps browsers pick appropriate default form-control styling and improves theming on some platforms.
Proposed tweak
<head> <meta charset="UTF-8" /> <meta name="viewport" content="width=device-width, initial-scale=1.0" /> - <title>weather-app</title> + <meta name="color-scheme" content="light dark" /> + <meta name="theme-color" content="#f5f7fa" /> + <title>Weather App</title> </head>examples/weather-app/ui/src/styles.css (3)
46-53: Optional: consider100dvhfallback for mobile browser UI chrome
100vhcan be slightly off on mobile when the address bar collapses/expands.Proposed tweak
.container { max-width: 480px; margin: 0 auto; padding: 20px; - min-height: 100vh; + min-height: 100vh; + min-height: 100dvh; display: flex; flex-direction: column; }
307-321: Addprefers-reduced-motionhandling for the loading animationThis avoids forcing motion/animation for users who opt out at the OS level.
Proposed tweak
@keyframes float { 0%, 100% { transform: translateY(0); } 50% { transform: translateY(-10px); } } + +@media (prefers-reduced-motion: reduce) { + .loading-icon { + animation: none; + } +}
357-378: Add explicit.button:focus-visiblestyling (keyboard accessibility)Right now focus indication depends on browser defaults; adding a clear focus ring makes it consistent.
Proposed tweak
.button:active { transform: scale(0.98); } + +.button:focus-visible { + outline: 2px solid var(--accent-hover); + outline-offset: 2px; +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/weather-app/ui/index.htmlexamples/weather-app/ui/src/styles.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: test-and-lint
Code Review: Weather App ExampleSummaryThis PR adds a well-structured weather app example that demonstrates the @mcp-apps-kit framework. The implementation follows best practices with comprehensive testing, proper error handling, and a clean separation of concerns. Overall, this is a high-quality contribution. Strengths1. Architecture & Code Quality
2. API Integration
3. UI Components
4. Documentation
Issues & RecommendationsCritical IssuesNone found Medium Priority1. Type Duplication (weatherService.ts, ui/src/App.tsx:18-73) 2. Mock Alert Generation (weatherService.ts:321) 3. Empty Catch Blocks (weatherService.ts:94-128, 131-175, 178-229) Low Priority4. Magic Numbers (ui/src/App.tsx:85) 5. Mock Location Hardcoded (weatherService.ts:232-240) 6. Redundant Validation (weatherService.ts:378) Security ReviewNo security concerns:
Performance ConsiderationsGenerally good:
Test CoverageExcellent coverage (33 tests):
Minor gap: Real API mode not tested (understandable since it requires network access) Checklist Compliance
Recommendations SummaryBefore Merge:
Post-Merge (Optional): Final VerdictAPPROVE WITH MINOR SUGGESTIONS This is a well-crafted example that demonstrates best practices for building MCP apps. The code is production-quality with excellent test coverage. The suggestions above are minor improvements that do not block merging. Great work! Reviewed using repository CLAUDE.md guidelines |
- Fix type duplication: UI now imports types from server instead of duplicating - Add varied mock coordinates with MOCK_LOCATIONS lookup table and hash-based fallback - Add input validation for empty location queries - Add Zod validation for Open-Meteo API responses - Use crypto.randomUUID() for alert IDs to prevent race conditions - Use URL constructor for cleaner API URL building - Use env variable for port configuration - Use 'export type' for type-only exports - Add JSDoc documentation for public functions - Update tests to verify varied coordinates behavior
Code Review: Weather App ExampleThank you for this comprehensive weather app example! This is a well-structured addition that demonstrates MCP Apps Kit capabilities effectively. Here's my detailed review: ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues Found1. Security: Unvalidated External API Data (Medium Severity)Location: The code uses const parseResult = CurrentWeatherApiSchema.safeParse(rawData);
if (\!parseResult.success) {
return null; // ⚠️ Silently fails on malformed data
}Recommendation: Log validation errors for debugging: if (\!parseResult.success) {
console.warn('Weather API validation failed:', parseResult.error);
return null;
}2. Potential Bug: Inconsistent Hash Algorithm (Low Severity)Location: The hash = hash & hash; // Convert to 32bit integerIssue: This doesn't actually convert to 32-bit. Should be: hash = hash | 0; // Convert to 32bit integer3. Performance: Unnecessary Re-renders (Low Severity)Location: The result extraction logic runs on every render: const rawResult = result?.getCurrentWeather ?? result?.getForecast ?? result?.getWeatherAlerts ?? result;Recommendation: Memoize this with 4. Type Safety: Loose Type Guards (Low Severity)Location: Type guards check for property existence but don't validate structure: function isCurrentWeather(data: unknown): data is CurrentWeather {
return \!\!data && typeof data === 'object' && 'temperature' in data && 'humidity' in data;
}Recommendation: Consider using Zod schemas for runtime validation or more specific checks. 5. Missing Input Validation (Low Severity)Location: Tool handlers don't validate the location string format before processing: handler: async ({ location }): Promise<CurrentWeather> => {
return await weatherService.getCurrentWeather(location);
}Recommendation: Add basic validation (e.g., max length, allowed characters) to prevent abuse. 📋 Minor SuggestionsCode Style
Documentation
Testing
🎯 RecommendationsHigh Priority
Medium Priority
Low Priority
📊 SummaryCode Quality: ⭐⭐⭐⭐⭐ (5/5) Overall: ⭐⭐⭐⭐⭐ (4.6/5) This is an excellent example that demonstrates best practices for MCP Apps Kit. The issues found are minor and don't block merging. The code is production-ready with the suggested improvements being nice-to-haves rather than requirements. Great work! 🎉 Review generated by Claude Code - feedback welcome! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @examples/weather-app/server/index.ts:
- Around line 197-201: The PORT parsing can yield NaN for non-numeric strings;
validate process.env.PORT after parseInt and before calling app.start so you
never pass NaN to app.start({ port }). In the NODE_ENV !== "test" block, check
the parsed port (the local variable port) with Number.isInteger/Number.isFinite
or isNaN, and if invalid either fall back to the default (e.g., 3005) or log an
error and exit; ensure any chosen behavior logs the invalid process.env.PORT
value to aid debugging before calling app.start({ port }).
🧹 Nitpick comments (2)
examples/weather-app/ui/src/App.tsx (1)
158-178: Strengthen type guards with additional field checks.The type guards perform shallow checks (1-2 fields only), which could lead to false positives if the data shape is similar but incomplete. Since the server is the controlled source, the risk is moderate, but adding more discriminating checks would improve runtime safety.
♻️ Suggested improvements to type guards
function isCurrentWeather(data: unknown): data is CurrentWeather { - return !!data && typeof data === "object" && "temperature" in data && "humidity" in data; + return ( + !!data && + typeof data === "object" && + "location" in data && + "temperature" in data && + "humidity" in data && + "timestamp" in data && + "icon" in data + ); } function isForecast(data: unknown): data is WeatherForecast { return ( !!data && typeof data === "object" && + "location" in data && "daily" in data && - Array.isArray((data as WeatherForecast).daily) + Array.isArray((data as WeatherForecast).daily) && + "generatedAt" in data ); } function isAlerts(data: unknown): data is WeatherAlertsResponse { return ( !!data && typeof data === "object" && + "location" in data && "alerts" in data && - Array.isArray((data as WeatherAlertsResponse).alerts) + Array.isArray((data as WeatherAlertsResponse).alerts) && + "lastChecked" in data ); }examples/weather-app/server/services/weatherService.ts (1)
138-280: Consider logging errors in API fetch helpers for debugging.The three API fetch helpers (
geocodeLocation,fetchCurrentWeather,fetchForecast) intentionally returnnullon errors to enable fallback to mock data. However, the empty catch blocks silently swallow all errors, which could make debugging API issues difficult in production.🔍 Suggested error logging
Add error logging in the catch blocks (example for
geocodeLocation, apply similarly to the other two):} catch (error) { + // Log error for debugging, but return null to trigger fallback + if (process.env.NODE_ENV !== 'test') { + console.error('Geocoding API error:', error); + } return null; }This preserves the fallback behavior while improving observability in production.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/weather-app/server/index.tsexamples/weather-app/server/services/weatherService.tsexamples/weather-app/tests/unit/weatherService.test.tsexamples/weather-app/ui/src/App.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/weather-app/tests/unit/weatherService.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use Strict TypeScript mode: noanytypes, useunknownand narrow types instead
Remove all unused variables or prefix them with underscore (_)
Useexport typefor type-only exports
Files:
examples/weather-app/server/index.tsexamples/weather-app/server/services/weatherService.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only through
index.tsfiles
Files:
examples/weather-app/server/index.ts
🧠 Learnings (1)
📚 Learning: 2026-01-09T14:18:43.516Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.516Z
Learning: Applies to packages/core/**/*.ts : Always use `defineTool` and `defineUI` for type inference when creating tools and UI widgets
Applied to files:
examples/weather-app/ui/src/App.tsx
🧬 Code graph analysis (2)
examples/weather-app/server/index.ts (1)
examples/weather-app/server/services/weatherService.ts (5)
weatherService(562-562)WeatherService(456-559)CurrentWeather(86-98)WeatherForecast(113-117)WeatherAlertsResponse(129-133)
examples/weather-app/ui/src/App.tsx (2)
examples/weather-app/server/services/weatherService.ts (3)
CurrentWeather(86-98)WeatherForecast(113-117)WeatherAlertsResponse(129-133)examples/weather-app/server/index.ts (1)
AppTools(194-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-lint
- GitHub Check: claude-review
🔇 Additional comments (15)
examples/weather-app/ui/src/App.tsx (3)
1-20: LGTM! Clean imports with proper type-only imports.The use of
import typefor type-only imports is correct and aligns with strict TypeScript practices. Importing types directly from the server modules ensures type consistency between UI and server.
23-155: LGTM! Well-structured display components.The three display components (CurrentWeatherDisplay, ForecastDisplay, AlertsDisplay) are clean, focused presentational components with appropriate formatting and styling.
180-249: LGTM! Solid App component implementation.The main App component correctly wires MCP Apps Kit hooks, handles both wrapped and unwrapped result formats, provides appropriate waiting/error states, and enables user interactions via follow-up message buttons.
examples/weather-app/server/index.ts (5)
1-20: LGTM! Clean imports and service initialization.The inline
typesyntax for type-only imports is correct, and the WeatherService initialization with environment-based mock mode is appropriate for testing and development scenarios.
22-42: LGTM! UI definitions correctly point to single bundle.All three UI widgets appropriately reference the same HTML bundle, which aligns with the App component's type-based rendering approach.
44-110: LGTM! Comprehensive Zod schemas with proper validation.The output schemas are well-structured, comprehensive, and correctly use Zod 4 features including enums, nested objects, arrays, and descriptions for AI tool usage.
112-186: LGTM! Well-defined MCP tools with proper schemas and handlers.The three tools are correctly structured with comprehensive input/output schemas, appropriate visibility and annotations, and handlers that properly delegate to the WeatherService. The
readOnlyHintannotation correctly indicates these are read-only operations.
189-204: LGTM! Proper type exports and server startup logic.The type exports correctly use
export typefor type-only exports (compliant with coding guidelines), and the conditional server startup appropriately skips in test environments while exporting the app for testing purposes.examples/weather-app/server/services/weatherService.ts (7)
1-37: LGTM! Comprehensive weather code mappings.The use of
crypto.randomUUID()for generating alert IDs is appropriate, and the WEATHER_CODES mapping provides comprehensive coverage of Open-Meteo weather codes with user-friendly descriptions and icons.
39-76: LGTM! Proper Zod validation for external API responses.The Zod schemas appropriately validate Open-Meteo API responses at runtime, providing a safety layer against unexpected API changes or malformed data. This is a best practice for external API integration.
78-133: LGTM! Well-structured TypeScript interfaces.The interfaces are comprehensive and provide clear contracts for the weather data structures, aligning well with the Zod schemas used for validation.
282-340: LGTM! Clever deterministic mock location generation.The lookup table provides realistic coordinates for common cities, and the hash-based fallback generates varied, deterministic coordinates for arbitrary location queries. The
hashCodefunction correctly handles integer conversion with the& hashoperation.
345-445: LGTM! Realistic mock data generators with proper UUID usage.The mock data generators provide realistic, varied test data. The use of
crypto.randomUUID()for alert IDs is appropriate and avoids potential race conditions or collisions that could occur with timestamp-based or sequential IDs.
447-559: LGTM! Robust WeatherService implementation with proper validation and fallbacks.The WeatherService class demonstrates good practices:
- Input validation for empty location queries with clear error messages
- Graceful fallback to mock data when APIs are unavailable
- Proper day clamping for forecast requests (1-16 days)
- Clear JSDoc documentation for all public methods
- Appropriate handling of Open-Meteo's lack of alert data
561-565: LGTM! Convenient default exports for production and testing.Exporting both a default
weatherServiceinstance (real API mode) and amockWeatherServiceinstance (mock-only mode) provides convenient defaults for different use cases.
| if (process.env.NODE_ENV !== "test") { | ||
| const port = parseInt(process.env.PORT || "3005", 10); | ||
| await app.start({ port }); | ||
| console.log(`Weather App MCP server running on http://localhost:${port}`); | ||
| } |
There was a problem hiding this comment.
Validate PORT environment variable to handle invalid values.
The parseInt call can return NaN if PORT contains a non-numeric string. While unlikely in practice, this could cause confusing startup errors.
🛡️ Suggested PORT validation
- const port = parseInt(process.env.PORT || "3005", 10);
+ const portEnv = process.env.PORT || "3005";
+ const port = parseInt(portEnv, 10);
+ if (isNaN(port) || port < 1 || port > 65535) {
+ throw new Error(`Invalid PORT: ${portEnv}. Must be a number between 1 and 65535.`);
+ }
await app.start({ port });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.env.NODE_ENV !== "test") { | |
| const port = parseInt(process.env.PORT || "3005", 10); | |
| await app.start({ port }); | |
| console.log(`Weather App MCP server running on http://localhost:${port}`); | |
| } | |
| if (process.env.NODE_ENV !== "test") { | |
| const portEnv = process.env.PORT || "3005"; | |
| const port = parseInt(portEnv, 10); | |
| if (isNaN(port) || port < 1 || port > 65535) { | |
| throw new Error(`Invalid PORT: ${portEnv}. Must be a number between 1 and 65535.`); | |
| } | |
| await app.start({ port }); | |
| console.log(`Weather App MCP server running on http://localhost:${port}`); | |
| } |
🤖 Prompt for AI Agents
In @examples/weather-app/server/index.ts around lines 197 - 201, The PORT
parsing can yield NaN for non-numeric strings; validate process.env.PORT after
parseInt and before calling app.start so you never pass NaN to app.start({ port
}). In the NODE_ENV !== "test" block, check the parsed port (the local variable
port) with Number.isInteger/Number.isFinite or isNaN, and if invalid either fall
back to the default (e.g., 3005) or log an error and exit; ensure any chosen
behavior logs the invalid process.env.PORT value to aid debugging before calling
app.start({ port }).
PR Review: Weather App ExampleSummaryThis is a well-crafted example that demonstrates the MCP Apps Kit framework effectively. The code is clean, well-documented, and follows the project conventions outlined in CLAUDE.md. Overall, this is a solid contribution with only minor suggestions for improvement. ✅ StrengthsCode Quality
Testing
Design
🔍 Issues & Suggestions1. Potential Runtime Issue in Mock Data Generation (Minor)Location: The mock weather generation uses const weatherCode = codes[Math.floor(Math.random() * codes.length)];Suggestion: Consider using a seeded random function or hash-based selection for more deterministic mock data in tests. 2. Missing Input Validation (Minor)Location: The days: z
.number()
.min(1)
.max(16)
.default(7)Observation: This is actually fine as-is, but worth noting that the service also clamps the value at line 511 of 3. Error Messages Could Be More Specific (Minor)Location: All three methods throw the same generic error: throw new Error("Location query cannot be empty");Suggestion: Consider using the framework's import { AppError, ErrorCode } from "@mcp-apps-kit/core";
throw new AppError({
code: ErrorCode.VALIDATION_ERROR,
message: "Location query cannot be empty"
});4. Silent Failures in API Calls (Low Priority)Location: The geocoding and weather fetch functions catch all errors silently and return } catch {
return null;
}Observation: While the fallback to mock data is a good UX decision, it makes debugging difficult if the API is actually failing. Consider logging these errors in development mode: } catch (error) {
if (process.env.NODE_ENV !== 'production') {
console.error('Geocoding failed:', error);
}
return null;
}5. Type Guard Could Be Stronger (Minor)Location: The type guards use basic checks: function isCurrentWeather(data: unknown): data is CurrentWeather {
return !!data && typeof data === "object" && "temperature" in data && "humidity" in data;
}Suggestion: Consider using Zod's safeParse for runtime validation to match the schema definitions in the server. 6. Protocol Configuration (Question)Location: config: {
protocol: "openai",
},Question: Is this meant to be "openai" or should it support both protocols? The PR description mentions working with both MCP Apps and ChatGPT, but this only specifies OpenAI. According to CLAUDE.md, the framework auto-generates platform-specific metadata, so this might be intentional. 7. Unused Client in UI (Very Minor)Location: const client = useAppsClient<AppTools>();The client is instantiated but only used in button onClick handlers (lines 226-241). This is fine, but worth noting that the client is being initialized on every render. 🔒 Security Considerations✅ No security issues identified
⚡ Performance ConsiderationsGenerally good, with minor notes:
📊 Test Coverage AssessmentStrong coverage with 33 tests:
Potential gap: No explicit tests for:
For an example project, this coverage is excellent. 📝 Documentation QualityExcellent:
✨ RecommendationsMust Fix (None)No blocking issues identified. Should Consider
Nice to Have
🎯 ConclusionThis is ready to merge with only minor suggestions for improvement. The code follows project conventions, has strong test coverage, and provides a clear, working example of the MCP Apps Kit framework. The automatic fallback to mock data and comprehensive testing make it reliable for demonstration purposes. Rating: ⭐⭐⭐⭐½ (4.5/5) Great work! 🎉 |
3 participants
