refactor:open-meteoへ移行 #296
Conversation
Walkthrough天気予報の機能をOpen-Meteo APIに置き換えました。複数ソースのXML/市区町村ベースのパイプラインから、Open-MeteoジオコーディングAPI中心のアプローチへ移行し、新しいデータモデルインターフェースを導入して既存ロジックを簡素化しました。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Module as WeatherModule
participant Geo as Geocoding API
participant WMO as Open-Meteo API
User->>Module: mentionHook (with location/date query)
Module->>Geo: searchLocation(query)
Geo-->>Module: GeocodingResult (lat, lon)
Module->>WMO: fetchForecast(lat, lon)
WMO-->>Module: WeatherForecast (daily data)
Module->>Module: formatWeatherToMfm(info, dateLabel)
Module-->>User: Reply with weather info + emoji reaction
sequenceDiagram
participant System as Scheduler
participant Module as WeatherModule
participant Geo as Geocoding API
participant WMO as Open-Meteo API
System->>Module: scheduleWeatherAutoNote()
Note over Module: Compute next run time
System->>Module: postWeatherNoteForAuto(place)
Module->>Geo: searchLocation(place)
Geo-->>Module: GeocodingResult
Module->>WMO: fetchForecast(lat, lon)
WMO-->>Module: WeatherForecast
Module->>Module: formatWeatherToMfm(info)
Module->>System: Post auto-note
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Summary of ChangesHello @lqvp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、天気情報を提供するモジュールのバックエンドAPIを、従来の日本の天気APIからOpen-Meteo APIに全面的に移行するリファクタリングです。これにより、天気データの取得方法が近代化され、コードの複雑性が軽減されました。また、天気情報の表示方法もOpen-MeteoのWMOコードに基づいて更新され、より汎用的な天気予報の提供が可能になります。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
weatherモジュールをOpen-Meteo APIへ移行するリファクタリング、素晴らしいですね。以前のXMLパース処理などがなくなり、コードが非常にクリーンで構造化されたものになりました。全体的に良い変更ですが、いくつか改善できる点を見つけました。
fetchForecast関数の戻り値に具体的な型を付けることで、より安全なコードになります。getDateLabelという未使用の関数が残っているため、削除するのが望ましいです。formatWeatherToMfm関数で、ユーザーに表示されるメッセージが一部重複してしまっています。
これらの点を修正することで、さらに品質の高いコードになると思います。素晴らしいリファクタリング作業です！
| } | ||
| const serif = serifs.weather.autoNote[wmo.serifKey] || serifs.weather.autoNote.other; | ||
| body += `${serif}\n`; | ||
| body += serifs.weather.forecast(info.place, forecast.dateLabel, wmo.description, '', ''); // Simplified call |
There was a problem hiding this comment.
現在、天気に合わせたセリフ（serif）を追加した後に、serifs.weather.forecastを呼び出しています。しかし、serifs.weather.forecastは天気概要を含んだ文章を生成するため、MFMのbodyにすでに含まれている天気情報と内容が重複してしまいます。
この行を削除し、148行目で生成される天気に合わせたセリフの後に、何か汎用的な締めの一言を加えるのが良いでしょう。例えば、以前の実装で使われていたようなランダムなメッセージや、固定のメッセージ（例：「今日も素敵な一日になりますように♪ ✨」）などが考えられます。
| } | ||
| } | ||
|
|
||
| private async fetchForecast(lat: number, lon: number): Promise<any> { |
There was a problem hiding this comment.
fetchForecast関数の戻り値の型が any になっています。エラー時に null を返す可能性があるため、少なくとも Promise<any | null> とするのがより正確です。さらに、APIレスポンスの型を定義することで、コードの型安全性が向上し、保守しやすくなります。
例えば、以下のようなインターフェースを定義して利用することを推奨します。
interface OpenMeteoForecastResponse {
daily: {
time: string[];
weathercode: number[];
temperature_2m_max: (number | null)[];
temperature_2m_min: (number | null)[];
};
// ... and other fields from the API response
}そして、関数のシグネチャを private async fetchForecast(lat: number, lon: number): Promise<OpenMeteoForecastResponse | null> のように変更します。
| private async fetchForecast(lat: number, lon: number): Promise<any> { | |
| private async fetchForecast(lat: number, lon: number): Promise<any | null> { |
| private getDateLabel(dateStr: string, timezone: string): string { | ||
| const date = new Date(dateStr); | ||
| const now = new Date(); | ||
|
|
||
| // Adjust to the timezone of the location if possible, but for simplicity we compare dates | ||
| // This is a rough approximation. Ideally we should use dayjs with timezone. | ||
| // But since we only need Today/Tomorrow/DayAfterTomorrow relative to "now" (server time), | ||
| // and the API returns daily data... | ||
|
|
||
| // Let's use a simple logic: | ||
| // The API returns dates in YYYY-MM-DD. | ||
| // We can check if it matches today's date in the requested timezone. | ||
|
|
||
| // For now, let's just map index 0, 1, 2 to Today, Tomorrow, DayAfterTomorrow | ||
| // This assumes the API returns data starting from "today" in the requested timezone. | ||
| return '今日'; // Placeholder, will be overwritten by index logic | ||
| } |
There was a problem hiding this comment.
この getDateLabel 関数は、現在のコードではどこからも呼び出されておらず、不要なコード（デッドコード）になっています。関数内のコメントにもあるように、このロジックは不完全なプレースホルダーのようです。コードの可読性を保つために、この関数は削除することを推奨します。
![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 (3)
src/modules/weather/index.ts (3)
12-12: デフォルト地名の言語統一を検討してください以前のデフォルト値が日本語（'東京都'）だったのに対し、'Tokyo'は英語です。geocoding APIは日本語クエリもサポートしているため、一貫性のために'東京'または'Tokyo'のどちらかに統一することをお勧めします。
100-117: 戻り値の型安全性を改善できます
fetchForecastの戻り値がany型になっており、型安全性が低下しています。Open-Meteo APIのレスポンス構造に合わせた専用のインターフェースを定義することをお勧めします。例：
interface OpenMeteoForecastResponse { daily: { time: string[]; weathercode: number[]; temperature_2m_max: number[]; temperature_2m_min: number[]; }; timezone?: string; } private async fetchForecast(lat: number, lon: number): Promise<OpenMeteoForecastResponse | null> { // ... }
252-264: API配列の長さ検証を追加してください
data.daily.time.mapを実行する前に、配列の長さを検証していません。Open-Meteo APIが予期せず短い配列を返した場合、インデックスアクセスで問題が発生する可能性があります。const data = await this.fetchForecast(location.latitude, location.longitude); if (!data || !data.daily) { msg.reply(serifs.weather.fetchError); return { reaction: '❌' }; } + + if (!data.daily.time || data.daily.time.length === 0) { + msg.reply(serifs.weather.fetchError); + return { reaction: '❌' }; + } const forecasts: WeatherForecast[] = data.daily.time.map((date: string, index: number) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/modules/weather/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/weather/index.ts (2)
src/decorators.ts (1)
bindThis(8-45)src/message.ts (2)
text(44-46)Message(12-139)
🔇 Additional comments (6)
src/modules/weather/index.ts (6)
49-69: インターフェース定義が適切です新しいデータモデルは Open-Meteo API のレスポンス構造に合わせて適切に設計されており、null許容型も正しく指定されています。
74-80: 初期化ロジックが簡素化されました以前の複雑なセットアップロジックから、自動投稿スケジューリングのみに簡素化されており、コードの保守性が向上しています。
155-160: 日付ラベル正規化ロジックが適切です入力のバリエーション（'明日', 'あした'等）を適切に処理しており、デフォルト値も妥当です。
225-277: メンションフックの実装が適切です新しいOpen-Meteo APIへの移行に伴い、メンションフックのロジックが適切に更新されています。正規表現パターンも日本語の入力バリエーションを適切に処理しています。
18-47: 元のレビューコメントは根拠がありません検証結果からOpen-Meteo APIが返すWMO天気コードは以下の通りです：
0、1、2、3、45、48、51、53、55、56、57、61、63、65、66、67、71、73、75、77、80、81、82、85、86、95、96、99
WMO_CODE_MAPはこれらのコード全てを網羅しており、Open-Meteoが実際に返す可能性のあるコードに対して完全なカバレッジがあります。理論上のWMO標準に存在するが実際には返されないコード（4-44など）についての懸念は、このAPIの実装には関係ありません。
Likely an incorrect or invalid review comment.
150-150: コードは正しく機能しています - 検証完了関数シグネチャを確認しました。
serifs.weather.forecastの定義は以下の通りです（src/serifs.ts:546）:forecast: (place, dateLabel, telop, tempStr, rainStr) =>line 150の呼び出しは5つの引数をすべて正しく渡しており、空文字列2つの使用は意図的です。理由は以下の通り：
- APIから降水データは取得されていない（line 108で取得: weathercode, temperature_2m_max, temperature_2m_min）
- 気温情報はline 136-141で既に別途フォーマットされており、本文に挿入済み
- この関数呼び出しは最終メッセージのテンプレートであり、気温・降水情報の埋め込みは不要
- コメント「Simplified call」は設計の意図を反映している
| private async searchLocation(query: string): Promise<GeocodingResult | null> { | ||
| try { | ||
| const response = await axios.get<{ results?: GeocodingResult[] }>(OPEN_METEO_GEOCODING_API, { | ||
| params: { | ||
| name: query, | ||
| count: 1, | ||
| language: 'ja', | ||
| format: 'json', | ||
| }, | ||
| timeout: 10000, | ||
| }); | ||
| return response.data.results?.[0] ?? null; | ||
| } catch (e) { | ||
| this.log(`Error searching location ${query}: ${e}`); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
APIレスポンス型の検証を推奨
geocoding APIのレスポンス構造が定義された型と一致するか確認してください。Open-Meteo geocoding APIは追加フィールド（timezone, population等）を返す可能性があります。
🌐 Web query:
What is the response structure of Open-Meteo geocoding API v1/search endpoint?
💡 Result:
The /v1/search endpoint returns a JSON object with a top-level "results" array. Each element is a location object containing (when available) these fields:
- id (integer)
- name (string)
- latitude, longitude (float)
- elevation (float)
- timezone (string)
- feature_code (string)
- country_code (2‑letter string)
- country (string)
- country_id (integer)
- population (integer)
- postcodes (array of strings)
- admin1, admin2, admin3, admin4 (strings)
- admin1_id, admin2_id, admin3_id, admin4_id (integers)
On error the API returns a JSON error object, e.g.:
{ "error": true, "reason": "..." }.
Source: Open‑Meteo Geocoding API docs. [1]
References
[1] Open‑Meteo — Geocoding API /v1/search (response structure and example).
エラーレスポンスの処理をより明確にすることを推奨
Open-Meteo geocoding APIは、エラー時に { "error": true, "reason": "..." } の形式を返します。現在のコードでは results?.[0] が undefined を返し、その後 ?? null で処理されていますが、APIエラーレスポンスと「検索結果なし」を区別していません。
エラーレスポンスを明示的に検証することで、より詳細なエラーログが可能になります：
const errorData = response.data as any;
if (errorData.error) {
this.log(`API error searching location ${query}: ${errorData.reason}`);
return null;
}🤖 Prompt for AI Agents
In src/modules/weather/index.ts around lines 82 to 98, the code currently treats
any missing results the same as an API error; update the try block to detect
Open-Meteo's error-shaped response before returning results: after receiving
response, inspect response.data for an error flag (e.g., response.data.error)
and if present log a specific message including response.data.reason and return
null; otherwise continue to return the first result (or null if no results).
Ensure the new check runs before accessing results and that logs clearly
distinguish API-error vs no-match cases.
| private getDateLabel(dateStr: string, timezone: string): string { | ||
| const date = new Date(dateStr); | ||
| const now = new Date(); | ||
|
|
||
| // Adjust to the timezone of the location if possible, but for simplicity we compare dates | ||
| // This is a rough approximation. Ideally we should use dayjs with timezone. | ||
| // But since we only need Today/Tomorrow/DayAfterTomorrow relative to "now" (server time), | ||
| // and the API returns daily data... | ||
|
|
||
| // Let's use a simple logic: | ||
| // The API returns dates in YYYY-MM-DD. | ||
| // We can check if it matches today's date in the requested timezone. | ||
|
|
||
| // For now, let's just map index 0, 1, 2 to Today, Tomorrow, DayAfterTomorrow | ||
| // This assumes the API returns data starting from "today" in the requested timezone. | ||
| return '今日'; // Placeholder, will be overwritten by index logic | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
未使用のメソッドを削除してください
getDateLabelメソッドはコード内で使用されておらず、プレースホルダーのロジックのみが含まれています。実際の日付ラベル生成はLine 252-255でインデックスベースで実装されているため、この未使用メソッドは削除することをお勧めします。
- private getDateLabel(dateStr: string, timezone: string): string {
- const date = new Date(dateStr);
- const now = new Date();
-
- // Adjust to the timezone of the location if possible, but for simplicity we compare dates
- // This is a rough approximation. Ideally we should use dayjs with timezone.
- // But since we only need Today/Tomorrow/DayAfterTomorrow relative to "now" (server time),
- // and the API returns daily data...
-
- // Let's use a simple logic:
- // The API returns dates in YYYY-MM-DD.
- // We can check if it matches today's date in the requested timezone.
-
- // For now, let's just map index 0, 1, 2 to Today, Tomorrow, DayAfterTomorrow
- // This assumes the API returns data starting from "today" in the requested timezone.
- return '今日'; // Placeholder, will be overwritten by index logic
- }
-📝 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.
| private getDateLabel(dateStr: string, timezone: string): string { | |
| const date = new Date(dateStr); | |
| const now = new Date(); | |
| // Adjust to the timezone of the location if possible, but for simplicity we compare dates | |
| // This is a rough approximation. Ideally we should use dayjs with timezone. | |
| // But since we only need Today/Tomorrow/DayAfterTomorrow relative to "now" (server time), | |
| // and the API returns daily data... | |
| // Let's use a simple logic: | |
| // The API returns dates in YYYY-MM-DD. | |
| // We can check if it matches today's date in the requested timezone. | |
| // For now, let's just map index 0, 1, 2 to Today, Tomorrow, DayAfterTomorrow | |
| // This assumes the API returns data starting from "today" in the requested timezone. | |
| return '今日'; // Placeholder, will be overwritten by index logic | |
| } |
🤖 Prompt for AI Agents
In src/modules/weather/index.ts around lines 162 to 178, remove the unused
getDateLabel method: it contains placeholder logic and is not referenced
anywhere (actual labeling uses index-based logic at lines 252-255). Delete the
entire private getDateLabel(dateStr: string, timezone: string): string { ... }
method and any related dead imports/comments, then run a quick build/TS check to
ensure no references remain.
| private scheduleWeatherAutoNote() { | ||
| const now = new Date(); | ||
| const nextRunTime = new Date(now); | ||
| nextRunTime.setHours(DEFAULT_AUTO_NOTE_HOUR, 0, 0, 0); | ||
| if (now >= nextRunTime) { | ||
| nextRunTime.setDate(nextRunTime.getDate() + 1); | ||
| } | ||
| const msUntilNext = nextRunTime.getTime() - now.getTime(); | ||
|
|
||
| setTimeout(() => { | ||
| this.postWeatherNoteForAuto(DEFAULT_WEATHER_PLACE); | ||
| setInterval( | ||
| () => this.postWeatherNoteForAuto(DEFAULT_WEATHER_PLACE), | ||
| DAILY_INTERVAL_MS | ||
| ); | ||
| }, msUntilNext); | ||
| } |
There was a problem hiding this comment.
タイマーのメモリリーク対策が必要です
setTimeoutとsetIntervalの戻り値が保存されていないため、以下の問題が発生する可能性があります：
- モジュールが複数回初期化されると、複数のインターバルが重複して実行される
- モジュールをアンロードする際にタイマーをクリーンアップできない
- メモリリークの原因となる
タイマーIDをクラスのプロパティに保存し、必要に応じてクリアできるようにすることをお勧めします：
export default class WeatherModule extends Module {
public readonly name = 'weather';
+ private autoNoteTimeout?: NodeJS.Timeout;
+ private autoNoteInterval?: NodeJS.Timeout;
@bindThis
public install() {
this.scheduleWeatherAutoNote();
return {
mentionHook: this.mentionHook,
};
}
+ public uninstall() {
+ if (this.autoNoteTimeout) clearTimeout(this.autoNoteTimeout);
+ if (this.autoNoteInterval) clearInterval(this.autoNoteInterval);
+ }
+
private scheduleWeatherAutoNote() {
+ // Clear existing timers if any
+ if (this.autoNoteTimeout) clearTimeout(this.autoNoteTimeout);
+ if (this.autoNoteInterval) clearInterval(this.autoNoteInterval);
+
const now = new Date();
const nextRunTime = new Date(now);
nextRunTime.setHours(DEFAULT_AUTO_NOTE_HOUR, 0, 0, 0);
if (now >= nextRunTime) {
nextRunTime.setDate(nextRunTime.getDate() + 1);
}
const msUntilNext = nextRunTime.getTime() - now.getTime();
- setTimeout(() => {
+ this.autoNoteTimeout = setTimeout(() => {
this.postWeatherNoteForAuto(DEFAULT_WEATHER_PLACE);
- setInterval(
+ this.autoNoteInterval = setInterval(
() => this.postWeatherNoteForAuto(DEFAULT_WEATHER_PLACE),
DAILY_INTERVAL_MS
);
}, msUntilNext);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/weather/index.ts around lines 180 to 196, the scheduled
setTimeout and setInterval IDs are not stored which can cause duplicate
intervals and prevent cleanup; store both timer return values on the class
(e.g., this.autoNoteTimeoutId and this.autoNoteIntervalId), before scheduling
clear any existing timers (clearTimeout/clearInterval) to avoid duplicates,
assign the timeout ID, and inside the timeout assign the interval ID; also add a
public/ lifecycle method (e.g., dispose/unload) that clears both timers so the
module can be safely unloaded and avoid memory leaks; ensure the stored types
match the runtime (NodeJS.Timeout vs number) and use clearTimeout/clearInterval
when cleaning up.
| }; | ||
|
|
||
| const text = this.formatWeatherToMfm(info); | ||
| this.ai.api('notes/create', { text: text }); |
There was a problem hiding this comment.
API投稿のエラーハンドリングを追加してください
notes/create APIコールにエラーハンドリングがありません。ネットワークエラーや認証エラーが発生した場合、サイレントに失敗してログも残りません。
- const text = this.formatWeatherToMfm(info);
- this.ai.api('notes/create', { text: text });
+ const text = this.formatWeatherToMfm(info);
+ try {
+ await this.ai.api('notes/create', { text: text });
+ } catch (e) {
+ this.log(`Error posting auto weather note: ${e}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/weather/index.ts around line 222, the call
this.ai.api('notes/create', { text: text }) has no error handling; wrap the API
call in an async try/catch (await the promise), catch and log the error with
contextual details (endpoint, payload summary, and error.message), and either
propagate the error or return a failure indicator so callers can react; ensure
any sensitive data is not logged.
resolve: #288
Summary by CodeRabbit
リリースノート
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.