GPSログベースのリアルタイム路線・行先推定アルゴリズムを実装#5735
Conversation
📝 WalkthroughWalkthroughGPS位置ログをバッファリングして路線を推定する機能が追加され、デバッグUI、Jotai状態、推定ユーティリティ群、カスタムフック、および関連テストが導入されました。 Changes
sequenceDiagram
participant User
participant Screen as SelectLineScreen
participant Modal as RouteEstimationDebugModal
participant Hook as useRouteEstimation
participant State as routeEstimationState
participant Apollo as GraphQL/Apollo
participant Utils as RouteEstimationUtils
User->>Screen: デバッグボタンを押す
Screen->>Modal: modalVisible = true
User->>Modal: 推定開始ボタンを押す
Modal->>Hook: startEstimation()
Hook->>State: isEstimating = true, status = "collecting"
loop 位置更新受信
Hook->>Utils: appendToBuffer(newPoint)
Utils-->>State: locationBuffer 更新
Hook->>Hook: バッファ長チェック
end
Hook->>Utils: preprocessLogs(buffer)
Utils-->>Hook: FilteredLocationLog[]
Hook->>Utils: isTransferStopチェック
alt transfer stop detected
Hook->>State: locationBuffer / candidates をクリア
else
Hook->>Apollo: GET_STATIONS_NEARBY(firstPoint, lastPoint)
Apollo-->>Hook: nearby stations
Hook->>Apollo: GET_LINE_STATIONS(for each candidate line)
Apollo-->>Hook: stations lists
Hook->>Utils: scoreLine / estimateDirection / calcConfidence
Utils-->>Hook: RouteCandidate[]
Hook->>State: candidates = results, status = "ready"
end
User->>Modal: 候補選択
Modal->>Hook: selectCandidate(candidate)
Hook->>State: 選択した路線をlineState/stationStateに設定, status = "idle"
Modal->>Screen: onClose()
Screen->>Screen: modalVisible = false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/components/RouteEstimationDebugModal.tsx (1)
104-106:handleResetのラッパーは簡略化可能です。
handleResetは単にreset()を呼び出すだけなので、useCallbackでラップする必要性が低いです。直接resetを渡すことで、不要なコールバック生成を避けられます。♻️ 提案される修正
- const handleReset = useCallback(() => { - reset(); - }, [reset]);そして、Pressableで直接使用:
- onPress={handleReset} + onPress={reset}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RouteEstimationDebugModal.tsx` around lines 104 - 106, handleReset is an unnecessary wrapper around reset; remove the useCallback wrapper and the handleReset function and instead pass the existing reset function directly where handleReset was used (e.g., the Pressable onPress). Update any references to handleReset to use reset to avoid creating an extra callback and simplify the component.src/utils/routeEstimation/__tests__/scoreLine.test.ts (1)
10-21:mkStationヘルパーが複数ファイルで重複しています。
estimateDirection.test.tsとscoreLine.test.tsで同一のmkStationヘルパーが定義されています。src/utils/test/への共通化を検討できますが、これはオプショナルな改善です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/routeEstimation/__tests__/scoreLine.test.ts` around lines 10 - 21, The mkStation test helper is duplicated across estimateDirection.test.ts and scoreLine.test.ts; extract the mkStation function into a shared test utilities module (e.g., export const mkStation from src/utils/test/index.ts or similar) and update both tests to import mkStation instead of redefining it; ensure the exported symbol name matches (mkStation) and adjust any typing exports so tests compile (preserve the __typename, id, latitude, longitude, name, nameRoman, line, lines, stopCondition shape).src/utils/routeEstimation/__tests__/estimateDirection.test.ts (1)
4-15:mkStationヘルパー関数についてテスト用のファクトリ関数として適切に実装されていますが、
src/utils/test/factories.tsに類似のcreateStationヘルパーが既に存在します。複数のテストファイル(estimateDirection.test.ts、scoreLine.test.ts)で同様のmkStationが定義されているため、共通化を検討してもよいかもしれません。ただし、これはdev版限定機能のテストであり、現状でも問題なく動作するため、オプショナルな改善提案です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/routeEstimation/__tests__/estimateDirection.test.ts` around lines 4 - 15, The tests define a local mkStation factory that duplicates an existing helper createStation in src/utils/test/factories.ts; to deduplicate, remove the local mkStation in estimateDirection.test.ts (and other tests like scoreLine.test.ts) and import/createStation from the shared factories module instead, or move the mkStation implementation into factories and update tests to use that shared createStation symbol so all tests reuse one helper.src/utils/routeEstimation/estimateRoute.ts (1)
43-45: 通常速度域で全候補が新幹線の場合、空配列が返される可能性。Line 44で新幹線を除外していますが、すべての候補路線が
BulletTrainの場合、空配列が返されます。高速域(Line 26-31)や低速域(Line 34-41)ではフォールバックがありますが、通常速度域にはありません。意図した動作かもしれませんが、一貫性のためフォールバックを追加することを検討してください。
♻️ フォールバック追加の提案
// 通常速度域: 新幹線を除外 - return candidates.filter((c) => c.line.lineType !== LineType.BulletTrain); + const nonBullet = candidates.filter((c) => c.line.lineType !== LineType.BulletTrain); + return nonBullet.length > 0 ? nonBullet : candidates;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/routeEstimation/estimateRoute.ts` around lines 43 - 45, The current return in the normal-speed branch filters out BulletTrain via candidates.filter((c) => c.line.lineType !== LineType.BulletTrain) and can yield an empty array if all candidates are BulletTrain; add a fallback so that if the filtered result is empty you return the original candidates (or a defined fallback set) instead. Locate the normal-speed branch in estimateRoute (the line using candidates.filter and LineType.BulletTrain), assign the filtered array to a variable, check its length, and return the filtered array when non-empty or return candidates (or the chosen fallback) when it is empty.src/hooks/useRouteEstimation.ts (2)
57-58: bufferRefの初期化に関する潜在的な問題。
bufferRefはstate.locationBufferで初期化されていますが、これはフックの初回レンダリング時の値のみを取得します。state.locationBufferが外部から変更された場合(例:別のコンポーネントから直接atomを更新)、refとstateが同期しなくなる可能性があります。現在の使用パターンでは問題ないと思われますが、将来の拡張時に注意が必要です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRouteEstimation.ts` around lines 57 - 58, bufferRef is initialized from state.locationBuffer only on first render so it can become out of sync if state.locationBuffer changes externally; update bufferRef whenever state.locationBuffer changes by adding a useEffect that sets bufferRef.current = state.locationBuffer (or merges/appends as your logic requires) and keep existing uses of bufferRef unchanged; reference bufferRef, state.locationBuffer and the useEffect hook to implement this sync so the ref always reflects the latest atom value.
196-197: エラーのサイレント処理を検討。catch ブロックでエラーを握りつぶしていますが、デバッグ目的でエラーをログ出力することを推奨します。開発中の問題特定が容易になります。
♻️ エラーログ追加の提案
- } catch { + } catch (error) { + if (__DEV__) { + console.warn('Route estimation failed:', error); + } setState((prev) => ({ ...prev, status: 'collecting' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRouteEstimation.ts` around lines 196 - 197, The catch block in useRouteEstimation.ts is currently swallowing errors; change the anonymous catch to capture the error (e.g., catch (err)) and log it before restoring state so you retain setState((prev) => ({ ...prev, status: 'collecting' })); use the existing logging mechanism if one exists (or console.error) and include context like "useRouteEstimation error" along with the error object to aid debugging.src/utils/routeEstimation/estimateDirection.ts (1)
94-149: nextStationがcurrentStationと同じになる可能性の確認。Line 125-131のループで、
currentOrderedIdx以降のすべての駅が通過駅の場合、nextStationはcurrentStationのままになります。これは終点到着時には妥当ですが、UIで「次の駅」として表示する際に混乱を招く可能性があります。意図した動作であれば問題ありませんが、ドキュメントにこの挙動を明記することを推奨します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/routeEstimation/estimateDirection.ts` around lines 94 - 149, The loop that sets nextStation in estimateDirection can leave nextStation equal to currentStation when all subsequent stations are pass-through, which confuses UI; update estimateDirection to set nextStation to null (or a distinct sentinel) instead of leaving it as currentStation when no non-pass downstream station is found (use currentOrderedIdx and getIsPass to detect this), and propagate the change by updating the DirectionEstimate type/consumers (UI components that read nextStation) to handle a null/absent nextStation as "no next station / at terminal" rather than treating it as currentStation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/routeEstimation/scoreLine.ts`:
- Around line 196-244: Add unit tests that verify candidates are filtered when
calcConfidence produces values below MIN_CONFIDENCE; specifically, write a test
that constructs a ScoredLine and logs such that calcConfidence yields a low
value (e.g., score=0.6, pointCoverage≈0.8, stationFactor≈0.6,
accuracyFactor≈0.75 → ~0.216) and assert the candidate is rejected by the
filtering branch that checks "if (confidence < MIN_CONFIDENCE) continue;". Also
add a positive control test using the existing ideal conditions (score=0.8, 3
stations, accuracy 30, full pointCoverage) to assert the candidate is kept;
reference calcConfidence, MIN_CONFIDENCE, and the filtering logic around the
confidence check to locate where to hook the tests.
---
Nitpick comments:
In `@src/components/RouteEstimationDebugModal.tsx`:
- Around line 104-106: handleReset is an unnecessary wrapper around reset;
remove the useCallback wrapper and the handleReset function and instead pass the
existing reset function directly where handleReset was used (e.g., the Pressable
onPress). Update any references to handleReset to use reset to avoid creating an
extra callback and simplify the component.
In `@src/hooks/useRouteEstimation.ts`:
- Around line 57-58: bufferRef is initialized from state.locationBuffer only on
first render so it can become out of sync if state.locationBuffer changes
externally; update bufferRef whenever state.locationBuffer changes by adding a
useEffect that sets bufferRef.current = state.locationBuffer (or merges/appends
as your logic requires) and keep existing uses of bufferRef unchanged; reference
bufferRef, state.locationBuffer and the useEffect hook to implement this sync so
the ref always reflects the latest atom value.
- Around line 196-197: The catch block in useRouteEstimation.ts is currently
swallowing errors; change the anonymous catch to capture the error (e.g., catch
(err)) and log it before restoring state so you retain setState((prev) => ({
...prev, status: 'collecting' })); use the existing logging mechanism if one
exists (or console.error) and include context like "useRouteEstimation error"
along with the error object to aid debugging.
In `@src/utils/routeEstimation/__tests__/estimateDirection.test.ts`:
- Around line 4-15: The tests define a local mkStation factory that duplicates
an existing helper createStation in src/utils/test/factories.ts; to deduplicate,
remove the local mkStation in estimateDirection.test.ts (and other tests like
scoreLine.test.ts) and import/createStation from the shared factories module
instead, or move the mkStation implementation into factories and update tests to
use that shared createStation symbol so all tests reuse one helper.
In `@src/utils/routeEstimation/__tests__/scoreLine.test.ts`:
- Around line 10-21: The mkStation test helper is duplicated across
estimateDirection.test.ts and scoreLine.test.ts; extract the mkStation function
into a shared test utilities module (e.g., export const mkStation from
src/utils/test/index.ts or similar) and update both tests to import mkStation
instead of redefining it; ensure the exported symbol name matches (mkStation)
and adjust any typing exports so tests compile (preserve the __typename, id,
latitude, longitude, name, nameRoman, line, lines, stopCondition shape).
In `@src/utils/routeEstimation/estimateDirection.ts`:
- Around line 94-149: The loop that sets nextStation in estimateDirection can
leave nextStation equal to currentStation when all subsequent stations are
pass-through, which confuses UI; update estimateDirection to set nextStation to
null (or a distinct sentinel) instead of leaving it as currentStation when no
non-pass downstream station is found (use currentOrderedIdx and getIsPass to
detect this), and propagate the change by updating the DirectionEstimate
type/consumers (UI components that read nextStation) to handle a null/absent
nextStation as "no next station / at terminal" rather than treating it as
currentStation.
In `@src/utils/routeEstimation/estimateRoute.ts`:
- Around line 43-45: The current return in the normal-speed branch filters out
BulletTrain via candidates.filter((c) => c.line.lineType !==
LineType.BulletTrain) and can yield an empty array if all candidates are
BulletTrain; add a fallback so that if the filtered result is empty you return
the original candidates (or a defined fallback set) instead. Locate the
normal-speed branch in estimateRoute (the line using candidates.filter and
LineType.BulletTrain), assign the filtered array to a variable, check its
length, and return the filtered array when non-empty or return candidates (or
the chosen fallback) when it is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89519a50-4c41-4715-9b61-e79821938f18
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
src/components/RouteEstimationDebugButton.tsxsrc/components/RouteEstimationDebugModal.tsxsrc/hooks/useRouteEstimation.tssrc/screens/SelectLineScreen.render.test.tsxsrc/screens/SelectLineScreen.tsxsrc/store/atoms/routeEstimation.tssrc/utils/routeEstimation/__tests__/estimateDirection.test.tssrc/utils/routeEstimation/__tests__/preprocessLogs.test.tssrc/utils/routeEstimation/__tests__/scoreLine.test.tssrc/utils/routeEstimation/estimateDirection.tssrc/utils/routeEstimation/estimateRoute.tssrc/utils/routeEstimation/index.tssrc/utils/routeEstimation/preprocessLogs.tssrc/utils/routeEstimation/scoreLine.tssrc/utils/routeEstimation/types.ts
- 信頼度フィルタリングのユニットテストを追加(estimateRoute.test.ts) - handleResetの不要なuseCallbackラッパーを削除 - bufferRefをatom値と同期するuseEffectを追加 - catchブロックでエラーをログ出力するように変更 - テストヘルパーを共有createStation/createLineファクトリに統一 - nextStationを全後続駅が通過の場合にnullを返すように修正 - 通常速度域で全候補がBulletTrainの場合のフォールバックを追加 https://claude.ai/code/session_01NGVVDtTcmYSxyGr4Ww6ugr
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/routeEstimation/__tests__/estimateRoute.test.ts (1)
6-20:mkLogヘルパーをsrc/utils/test/factories.tsに抽出する検討
estimateRoute.test.tsとscoreLine.test.tsで完全に同一のmkLog実装が重複しています。src/utils/test/factories.tsにcreateFilteredLocationLogとして統合すれば、コードの重複を削減できます。コーディングガイドラインに「
src/utils/test/からテストヘルパーを再利用してセットアップコードの重複を避ける」と明記されており、この提案と一致しています。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/routeEstimation/__tests__/estimateRoute.test.ts` around lines 6 - 20, The mkLog helper in tests is duplicated; extract it into a shared test factory named createFilteredLocationLog in src/utils/test/factories.ts and replace usages in estimateRoute.test.ts and scoreLine.test.ts to import and call createFilteredLocationLog (keeping the same parameters and return shape FilteredLocationLog); update imports in both test files to import { createFilteredLocationLog } from 'src/utils/test/factories' and remove the local mkLog declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/routeEstimation/__tests__/estimateRoute.test.ts`:
- Around line 6-20: The mkLog helper in tests is duplicated; extract it into a
shared test factory named createFilteredLocationLog in
src/utils/test/factories.ts and replace usages in estimateRoute.test.ts and
scoreLine.test.ts to import and call createFilteredLocationLog (keeping the same
parameters and return shape FilteredLocationLog); update imports in both test
files to import { createFilteredLocationLog } from 'src/utils/test/factories'
and remove the local mkLog declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e0de8be-5862-47fa-8dc9-755ca27a9147
📒 Files selected for processing (8)
src/components/RouteEstimationDebugModal.tsxsrc/hooks/useRouteEstimation.tssrc/utils/routeEstimation/__tests__/estimateDirection.test.tssrc/utils/routeEstimation/__tests__/estimateRoute.test.tssrc/utils/routeEstimation/__tests__/scoreLine.test.tssrc/utils/routeEstimation/estimateDirection.tssrc/utils/routeEstimation/estimateRoute.tssrc/utils/routeEstimation/types.ts
✅ Files skipped from review due to trivial changes (3)
- src/utils/routeEstimation/tests/estimateDirection.test.ts
- src/components/RouteEstimationDebugModal.tsx
- src/utils/routeEstimation/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/routeEstimation/tests/scoreLine.test.ts
- src/utils/routeEstimation/estimateRoute.ts
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai 他になければapproveしてください |
|
Tip For best results, initiate chat on the files or code changes.
[approve] 🧠 Learnings used |
https://claude.ai/code/session_01NGVVDtTcmYSxyGr4Ww6ugr
Summary by CodeRabbit
リリースノート
New Features
Tests