Conversation
📝 WalkthroughWalkthrough5つの新規カスタムフック(useInitialNearbyStation、useLineSelection、usePresetCarouselData、useSelectLineWalkthrough、useStationsCache)をsrc/hooks/に追加し、これらをホークスのインデックスにエクスポートしました。SelectLineScreenをリファクタリングして、インラインの状態管理とGraphQLロジックをこれらのフックに移行しました。 Changes
Sequence Diagram(s)sequenceDiagram
participant SelectLineScreen
participant useInitialNearbyStation
participant useFetchNearbyStation
participant LocationAPI
participant GraphQL
participant AsyncStorage
participant AlertAPI
SelectLineScreen->>useInitialNearbyStation: マウント
useInitialNearbyStation->>LocationAPI: hasStartedLocationUpdatesAsync()
LocationAPI-->>useInitialNearbyStation: true
useInitialNearbyStation->>LocationAPI: stopLocationUpdatesAsync()
useInitialNearbyStation->>useFetchNearbyStation: 座標で駅を取得
useFetchNearbyStation->>GraphQL: fetchNearby(座標、limit=1)
GraphQL-->>useFetchNearbyStation: 駅データ
useFetchNearbyStation-->>useInitialNearbyStation: 駅
useInitialNearbyStation->>AsyncStorage: getItem('firstAlertShown')
AsyncStorage-->>useInitialNearbyStation: null
useInitialNearbyStation->>AlertAPI: Alert.alert('notice')
AlertAPI-->>useInitialNearbyStation: ユーザー応答
useInitialNearbyStation-->>SelectLineScreen: station, nearbyStationLoading
sequenceDiagram
participant SelectLineScreen
participant useLineSelection
participant GraphQL
participant JotaiState
participant GeoLib
SelectLineScreen->>useLineSelection: handleLineSelected(line)
useLineSelection->>GraphQL: GET_LINE_STATIONS(lineId)
GraphQL-->>useLineSelection: 駅リスト
useLineSelection->>GeoLib: findNearest(駅, 現在位置)
GeoLib-->>useLineSelection: 最寄駅
useLineSelection->>JotaiState: pendingStation, pendingStations, pendingLine更新
SelectLineScreen->>useLineSelection: handleTrainTypeSelect(trainType)
useLineSelection->>GraphQL: GET_LINE_GROUP_STATIONS(lineGroupId)
GraphQL-->>useLineSelection: 駅グループデータ
useLineSelection->>JotaiState: pendingStations, pendingTrainType更新
sequenceDiagram
participant SelectLineScreen
participant usePresetCarouselData
participant useSavedRoutes
participant GraphQL
participant JotaiState
SelectLineScreen->>usePresetCarouselData: マウント
usePresetCarouselData->>useSavedRoutes: ルート一覧取得
useSavedRoutes-->>usePresetCarouselData: SavedRoute[]
usePresetCarouselData->>GraphQL: GET_LINE_LIST_STATIONS_PRESET(lineIds)
GraphQL-->>usePresetCarouselData: 駅マップ
usePresetCarouselData->>GraphQL: GET_LINE_GROUP_LIST_STATIONS_PRESET(lineGroupIds)
GraphQL-->>usePresetCarouselData: グループ駅マップ
usePresetCarouselData->>JotaiState: carouselData構築・更新
usePresetCarouselData-->>SelectLineScreen: carouselData, routes, isRoutesDBInitialized
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (11)
src/hooks/useSelectLineWalkthrough.ts (2)
64-73: マジックナンバーの抽出を検討してください。
-16の補正値は意図がコメントで説明されていますが、定数として抽出するとより明確になります。♻️ 提案
+const PRESETS_MARGIN_TOP_OFFSET = 16; + // プリセットエリアをハイライト(marginTop: -16 を補正) useEffect(() => { if (currentStepId === 'savedRoutes' && presetsLayout) { setSpotlightArea({ x: presetsLayout.x, - y: presetsLayout.y - 16, + y: presetsLayout.y - PRESETS_MARGIN_TOP_OFFSET, width: presetsLayout.width, height: presetsLayout.height, borderRadius: 12, }); } }, [currentStepId, presetsLayout, setSpotlightArea]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useSelectLineWalkthrough.ts` around lines 64 - 73, Extract the magic number -16 used when computing the spotlight Y position into a well-named constant (e.g. SPOTLIGHT_VERTICAL_OFFSET or SPOTLIGHT_Y_ADJUSTMENT) and replace the literal in useSelectLineWalkthrough's effect where currentStepId === 'savedRoutes' sets the spotlight (referencing presetsLayout and setSpotlightArea). Define the constant near the top of the hook (or with other related constants) and include a short comment explaining why the offset is needed so the intent is clear.
7-12:Layout型の重複を検討してください。
ButtonLayout(FooterTabBar.tsx)やHeaderLayout(NowHeader.tsx)と同一の構造です。共通の型を再利用するか、共通の場所に定義することで重複を削減できます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useSelectLineWalkthrough.ts` around lines 7 - 12, The Layout type is duplicated across components; extract a single shared type (e.g., export type Rect = { x: number; y: number; width: number; height: number } or export type LayoutRect = ...) into a common types module and replace the local Layout, ButtonLayout and HeaderLayout aliases with this shared type so all usages (Layout, ButtonLayout, HeaderLayout) import the common type; update the declarations in useSelectLineWalkthrough.ts, FooterTabBar.tsx, and NowHeader.tsx to import and use the new shared type and remove the redundant local definitions.src/hooks/useInitialNearbyStation.test.tsx (1)
56-61: HookBridge パターンについて
HookBridgeコンポーネントパターンはフックの同期的な結果取得に有効ですが、@testing-library/react-nativeのrenderHookを使用すると、より標準的なテストパターンになります(他のテストファイルで使用されているように)。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useInitialNearbyStation.test.tsx` around lines 56 - 61, Replace the custom HookBridge component pattern with the standard renderHook usage from the testing library: remove the HookBridge component and instead call renderHook(() => useInitialNearbyStation(), { wrapper: ... }) (or with appropriate provider wrapper used in other tests), then access result.current (or await waitForNextUpdate/waitFor) to assert hook output; refer to the HookBridge component and the useInitialNearbyStation hook in the test to find where to substitute renderHook so the test follows the same pattern as other files.src/hooks/usePresetCarouselData.ts (1)
33-104: レースコンディションの可能性があります。
routesが急速に変更された場合、複数のfetchAsyncが並行して実行される可能性があります。prevRoutesKeyRef.currentはsetCarouselDataの後に更新されるため、古いフェッチの結果が新しいデータを上書きする可能性があります。♻️ 提案: フェッチ開始時に routesKey を保存
useEffect(() => { const routesKey = routes .map((r) => `${r.id}:${r.lineId}:${r.trainTypeId}:${r.hasTrainType}`) .join(','); if (routesKey === prevRoutesKeyRef.current) return; + prevRoutesKeyRef.current = routesKey; const fetchAsync = async () => { try { // ... fetch logic ... setCarouselData( routes.map((r, i) => ({ ...r, __k: `${r.id}-${i}`, stations: r.hasTrainType ? (trainTypeStationsMap.get(r.trainTypeId) ?? []) : (lineStationsMap.get(r.lineId) ?? []), })) ); - prevRoutesKeyRef.current = routesKey; } catch (err) { console.error(err); } }; fetchAsync(); }, [routes]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/usePresetCarouselData.ts` around lines 33 - 104, The effect can suffer a race where an earlier fetchAsync overwrites newer results; fix by capturing this run's routesKey into a local constant (e.g., const myKey = routesKey) and use a ref to mark the latest fetch (e.g., latestFetchKeyRef.current = myKey) before awaiting; then, right before calling setCarouselData and updating prevRoutesKeyRef.current, confirm latestFetchKeyRef.current === myKey so only the most recent fetch writes state, and update prevRoutesKeyRef.current = myKey when you setCarouselData; reference useEffect, fetchAsync, prevRoutesKeyRef, setCarouselData and add latestFetchKeyRef to implement this guard.src/screens/SelectLineScreen.render.test.tsx (2)
269-272:afterEachでのjest.clearAllMocks()呼び出しを検討してくださいコーディングガイドラインでは
afterEachフックでjest.clearAllMocks()を呼び出すことが推奨されています。beforeEachで呼び出すと、テスト失敗時のデバッグ時にモックの状態が既にクリアされている場合があります。♻️ 修正案
describe('SelectLineScreen', () => { - beforeEach(() => { + afterEach(() => { jest.clearAllMocks(); });As per coding guidelines: "Call
jest.clearAllMocks()inafterEachhook"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/SelectLineScreen.render.test.tsx` around lines 269 - 272, In the describe('SelectLineScreen') test suite, move the jest.clearAllMocks() call out of the beforeEach hook and into an afterEach hook so mocks are cleared after each test rather than before; update the suite to remove jest.clearAllMocks() from beforeEach and add afterEach(() => jest.clearAllMocks()) to ensure mock state is preserved during test failure debugging.
258-264: atom の呼び出し順序に依存するモック実装
atomCallCountを使った実装は、フック内でのuseAtomValueの呼び出し順序に暗黙的に依存しています。SelectLineScreen の実装変更で順序が変わると、テストが失敗する可能性があります。♻️ 修正案: atom を個別にモックする
- // stationState を1回目、isLEDThemeAtom を2回目に返す - let atomCallCount = 0; - (useAtomValue as jest.Mock).mockImplementation(() => { - atomCallCount++; - if (atomCallCount % 2 === 1) return { stationsCache }; - return false; // isLEDTheme - }); + (useAtomValue as jest.Mock).mockImplementation((atom) => { + // atom の参照で判定する方がより堅牢 + // 実際の atom をインポートして比較するか、 + // 呼び出し引数のパターンで判定 + return { stationsCache }; // 簡易版 + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/SelectLineScreen.render.test.tsx` around lines 258 - 264, The test currently mocks useAtomValue with a call-count based implementation (atomCallCount) which makes the test fragile to call-order changes; instead update the mock of useAtomValue to inspect its atom argument and return per-atom values (e.g. return { stationsCache } when the atom equals stationState or the specific station atom, and return false when the atom equals isLEDThemeAtom), so locate the mock of useAtomValue in the test and replace the call-count logic with an argument-based branching on the atom parameter (referencing useAtomValue, isLEDThemeAtom and the station/state symbol that provides stationsCache).src/hooks/useLineSelection.ts (3)
108-111: 不要な null 合体演算子
lineはパラメータとして渡されているため、常に定義されています。line ?? nullは冗長です。♻️ 修正案
setLineState((prev) => ({ ...prev, - pendingLine: line ?? null, + pendingLine: line, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useLineSelection.ts` around lines 108 - 111, 不要な null 合体演算子が使われています: setLineState の更新で受け取ったパラメータ line は必ず定義されるため `pendingLine: line ?? null` は冗長です。useLineSelection.ts 内の該当箇所(setLineState の更新)を修正して `pendingLine: line` に置き換え、余分な null 合体演算子を削除してください(関数名: setLineState、プロパティ: pendingLine)。
291-295:station.idの型安全性
station.id as numberでキャストしていますが、station.idがnullの可能性がある場合、ランタイムエラーの原因になります。♻️ null チェックの追加
+ if (station.id == null) return; + const fetchedTrainTypesData = await fetchTrainTypes({ variables: { - stationId: station.id as number, + stationId: station.id, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useLineSelection.ts` around lines 291 - 295, The code casts station.id with "as number" before calling fetchTrainTypes which can cause runtime errors if station.id is null; in useLineSelection.ts guard against null/undefined by checking station?.id (or station.id != null) before calling fetchTrainTypes (e.g., early return, throw, or skip fetch), and pass the validated number to fetchTrainTypes.variables.stationId so you never rely on a forced cast; update the fetch call site and any control flow in the function to handle the case where station.id is missing.
247-259: geolib の座標フォーマットの統一を検討してください
orderByDistanceで{ lat, lon }形式を使用していますが、同じファイル内のfindNearest(192-197 行) では{ latitude, longitude }形式を使用しています。geolib は両方のフォーマットを受け入れるため機能上の問題はありませんが、コード内で座標フォーマットが一貫していないと可読性に影響します。♻️ 座標フォーマットの統一
const sortedStationCoords = latitude && longitude ? (orderByDistance( - { lat: latitude, lon: longitude }, + { latitude, longitude }, stations.map((sta) => ({ latitude: sta.latitude as number, longitude: sta.longitude as number, })) ) as { latitude: number; longitude: number }[])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useLineSelection.ts` around lines 247 - 259, The code uses two different geolib coordinate shapes; make them consistent by changing the orderByDistance call in sortedStationCoords to accept the same { latitude, longitude } format used by findNearest: update the reference point and the stations.map transformation inside the orderByDistance invocation to produce { latitude, longitude } keys (and adjust the expected cast/type if needed) so all coordinate handling in useLineSelection.ts uses the same shape (refer to sortedStationCoords and findNearest to align formats).src/hooks/useLineSelection.test.tsx (1)
74-287:handlePresetPressのテストケースが不足しています
handleLineSelected、handleTrainTypeSelect、handleCloseSelectBoundModalはテストされていますが、handlePresetPressのテストがありません。
handlePresetPressのテストケースを追加しますか?この関数はroute.hasTrainTypeに基づいてopenModalByTrainTypeIdまたはopenModalByLineIdを呼び出す分岐があります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useLineSelection.test.tsx` around lines 74 - 287, Add tests for handlePresetPress to cover both branches: when route.hasTrainType is true it should call openModalByTrainTypeId, and when false it should call openModalByLineId. In each test use setupMolecules()/setupQueries(), render HookBridge to get hookRef, pass a route object with hasTrainType true/false and appropriate trainTypeId/lineId, mock or spy the openModalByTrainTypeId and openModalByLineId functions (and ensure other modal function is not called), invoke hookRef.current?.handlePresetPress(route) inside act, and assert the correct modal function was called with the expected id while the other was not.src/hooks/useInitialNearbyStation.ts (1)
46-56: バックグラウンド位置更新の停止処理
stopLocationUpdatesの非同期処理でエラーが発生した場合のハンドリングがありません。♻️ エラーハンドリングの追加
useEffect(() => { const stopLocationUpdates = async () => { - const hasStartedLocationUpdates = - await Location.hasStartedLocationUpdatesAsync(LOCATION_TASK_NAME); - if (hasStartedLocationUpdates) { - await Location.stopLocationUpdatesAsync(LOCATION_TASK_NAME); - } + try { + const hasStartedLocationUpdates = + await Location.hasStartedLocationUpdatesAsync(LOCATION_TASK_NAME); + if (hasStartedLocationUpdates) { + await Location.stopLocationUpdatesAsync(LOCATION_TASK_NAME); + } + } catch (error) { + console.error('Failed to stop location updates:', error); + } }; stopLocationUpdates(); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useInitialNearbyStation.ts` around lines 46 - 56, The async stopLocationUpdates function inside the useEffect lacks error handling; wrap the body of stopLocationUpdates in a try/catch so failures from Location.hasStartedLocationUpdatesAsync or Location.stopLocationUpdatesAsync are caught and handled (e.g., log the error or surface it to a state/error reporter). Keep the existing flow using LOCATION_TASK_NAME and ensure the catch logs which call failed and the error object so debugging is possible while preserving the current behavior when no updates are started.
🤖 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/hooks/useInitialNearbyStation.test.tsx`:
- Around line 56-61: Replace the custom HookBridge component pattern with the
standard renderHook usage from the testing library: remove the HookBridge
component and instead call renderHook(() => useInitialNearbyStation(), {
wrapper: ... }) (or with appropriate provider wrapper used in other tests), then
access result.current (or await waitForNextUpdate/waitFor) to assert hook
output; refer to the HookBridge component and the useInitialNearbyStation hook
in the test to find where to substitute renderHook so the test follows the same
pattern as other files.
In `@src/hooks/useInitialNearbyStation.ts`:
- Around line 46-56: The async stopLocationUpdates function inside the useEffect
lacks error handling; wrap the body of stopLocationUpdates in a try/catch so
failures from Location.hasStartedLocationUpdatesAsync or
Location.stopLocationUpdatesAsync are caught and handled (e.g., log the error or
surface it to a state/error reporter). Keep the existing flow using
LOCATION_TASK_NAME and ensure the catch logs which call failed and the error
object so debugging is possible while preserving the current behavior when no
updates are started.
In `@src/hooks/useLineSelection.test.tsx`:
- Around line 74-287: Add tests for handlePresetPress to cover both branches:
when route.hasTrainType is true it should call openModalByTrainTypeId, and when
false it should call openModalByLineId. In each test use
setupMolecules()/setupQueries(), render HookBridge to get hookRef, pass a route
object with hasTrainType true/false and appropriate trainTypeId/lineId, mock or
spy the openModalByTrainTypeId and openModalByLineId functions (and ensure other
modal function is not called), invoke hookRef.current?.handlePresetPress(route)
inside act, and assert the correct modal function was called with the expected
id while the other was not.
In `@src/hooks/useLineSelection.ts`:
- Around line 108-111: 不要な null 合体演算子が使われています: setLineState の更新で受け取ったパラメータ line
は必ず定義されるため `pendingLine: line ?? null` は冗長です。useLineSelection.ts
内の該当箇所(setLineState の更新)を修正して `pendingLine: line` に置き換え、余分な null
合体演算子を削除してください(関数名: setLineState、プロパティ: pendingLine)。
- Around line 291-295: The code casts station.id with "as number" before calling
fetchTrainTypes which can cause runtime errors if station.id is null; in
useLineSelection.ts guard against null/undefined by checking station?.id (or
station.id != null) before calling fetchTrainTypes (e.g., early return, throw,
or skip fetch), and pass the validated number to
fetchTrainTypes.variables.stationId so you never rely on a forced cast; update
the fetch call site and any control flow in the function to handle the case
where station.id is missing.
- Around line 247-259: The code uses two different geolib coordinate shapes;
make them consistent by changing the orderByDistance call in sortedStationCoords
to accept the same { latitude, longitude } format used by findNearest: update
the reference point and the stations.map transformation inside the
orderByDistance invocation to produce { latitude, longitude } keys (and adjust
the expected cast/type if needed) so all coordinate handling in
useLineSelection.ts uses the same shape (refer to sortedStationCoords and
findNearest to align formats).
In `@src/hooks/usePresetCarouselData.ts`:
- Around line 33-104: The effect can suffer a race where an earlier fetchAsync
overwrites newer results; fix by capturing this run's routesKey into a local
constant (e.g., const myKey = routesKey) and use a ref to mark the latest fetch
(e.g., latestFetchKeyRef.current = myKey) before awaiting; then, right before
calling setCarouselData and updating prevRoutesKeyRef.current, confirm
latestFetchKeyRef.current === myKey so only the most recent fetch writes state,
and update prevRoutesKeyRef.current = myKey when you setCarouselData; reference
useEffect, fetchAsync, prevRoutesKeyRef, setCarouselData and add
latestFetchKeyRef to implement this guard.
In `@src/hooks/useSelectLineWalkthrough.ts`:
- Around line 64-73: Extract the magic number -16 used when computing the
spotlight Y position into a well-named constant (e.g. SPOTLIGHT_VERTICAL_OFFSET
or SPOTLIGHT_Y_ADJUSTMENT) and replace the literal in useSelectLineWalkthrough's
effect where currentStepId === 'savedRoutes' sets the spotlight (referencing
presetsLayout and setSpotlightArea). Define the constant near the top of the
hook (or with other related constants) and include a short comment explaining
why the offset is needed so the intent is clear.
- Around line 7-12: The Layout type is duplicated across components; extract a
single shared type (e.g., export type Rect = { x: number; y: number; width:
number; height: number } or export type LayoutRect = ...) into a common types
module and replace the local Layout, ButtonLayout and HeaderLayout aliases with
this shared type so all usages (Layout, ButtonLayout, HeaderLayout) import the
common type; update the declarations in useSelectLineWalkthrough.ts,
FooterTabBar.tsx, and NowHeader.tsx to import and use the new shared type and
remove the redundant local definitions.
In `@src/screens/SelectLineScreen.render.test.tsx`:
- Around line 269-272: In the describe('SelectLineScreen') test suite, move the
jest.clearAllMocks() call out of the beforeEach hook and into an afterEach hook
so mocks are cleared after each test rather than before; update the suite to
remove jest.clearAllMocks() from beforeEach and add afterEach(() =>
jest.clearAllMocks()) to ensure mock state is preserved during test failure
debugging.
- Around line 258-264: The test currently mocks useAtomValue with a call-count
based implementation (atomCallCount) which makes the test fragile to call-order
changes; instead update the mock of useAtomValue to inspect its atom argument
and return per-atom values (e.g. return { stationsCache } when the atom equals
stationState or the specific station atom, and return false when the atom equals
isLEDThemeAtom), so locate the mock of useAtomValue in the test and replace the
call-count logic with an argument-based branching on the atom parameter
(referencing useAtomValue, isLEDThemeAtom and the station/state symbol that
provides stationsCache).
|
@coderabbitai nitpickコメントをまとめて起票してください |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 46001 🏁 Script executed: Length of output: 24884
#!/bin/bash Create GitHub issue with consolidated nitpick commentsgh issue create 関連PR: #5368 対応項目src/hooks/useSelectLineWalkthrough.ts (2件)
src/hooks/useInitialNearbyStation.test.tsx (1件)
src/hooks/usePresetCarouselData.ts (1件)
src/screens/SelectLineScreen.render.test.tsx (2件)
src/hooks/useLineSelection.ts (3件)
src/hooks/useLineSelection.test.tsx (1件)
src/hooks/useInitialNearbyStation.ts (1件)
優先度このissueはコードの品質向上を目的としており、緊急性は低いです。時間のある時に対応してください。" Learnt from: TinyKitten Learnt from: TinyKitten Learnt from: CR Learnt from: CR Learnt from: CR |
Summary by CodeRabbit
テスト: 駅の初期化、路線選択、プリセットカルーセルデータ取得、駅キャッシング機能、ウォークスルーガイドに関する包括的なテストを追加しました。
リファクター: 駅と路線の選択フローを改善し、プリセットカルーセルのデータ管理を最適化し、駅データのキャッシング処理を強化しました。