ディープリンク再実装とMaestro E2Eリグレッションテストの追加 #5439
Merged
TinyKitten merged 12 commits intodevfrom Feb 23, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough既存の useOpenRouteFromLink フックとそのテストを削除し、ディープリンク処理を拡張した useDeepLink に統合。DeepLinkProvider の表示条件を更新し、グローバルな navigationRef を導入。Maestro の山手線 inbound/outbound E2E 設定を追加。 Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / DeepLinkProvider
participant Hook as useDeepLink
participant GraphQL as Apollo (line / lineGroup queries)
participant Store as Jotai Atoms
participant Nav as Navigation (navigationRef)
rect rgba(135,206,250,0.5)
App->>Hook: 初期URL取得またはイベントで handleUrl(url)
end
rect rgba(144,238,144,0.5)
Hook->>GraphQL: useLazyQuery(fetch by lineGroupId OR lineId)
GraphQL-->>Hook: stations data / error
end
rect rgba(255,228,181,0.5)
Hook->>Store: applyRoute -> setLine / setStation / setNavigation
Store-->>Hook: state 更新完了
end
rect rgba(221,160,221,0.5)
Hook->>Nav: navigationRef.isReady? -> navigate('Main') (retry logic)
Hook-->>App: initialUrlProcessed / isLoading / error（集約）
App->>App: 表示制御（ロード待ち or children 表示）
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/__fixtures__/yamanote.ts`:
- Around line 194-200: nameChinese フィールドが置換文字「�」を含む壊れた文字列になっていますので、fixtures
の該当エントリ（id: 1130210、name: '駒込' のオブジェクト）の nameChinese
プロパティの値を正しい中国語表記に修正してください（例: "駒込"
または適切な簡体字/繁体字表記に置き換え）。修正後、src/__fixtures__/yamanote.ts の該当オブジェクト（name, id
が目印）を更新してスナップショットやZH表示のテストを再実行してください。
In `@src/test/e2eHelpers.ts`:
- Around line 111-128: buildHeaderProps computes nextStation by using
currentIndex +/-1 which becomes undefined at line endpoints; detect loop lines
(e.g., config.selectedLine.isLoopLine or add an isLoopLine flag) and when
isLoopLine is true wrap the index arithmetic instead of returning undefined:
compute nextIndex = (currentIndex + (config.selectedDirection === 'INBOUND' ? 1
: -1) ± stations.length) % stations.length and use stations[nextIndex] for
nextStation so circular routes correctly wrap; ensure fallback for non-loop
lines still returns undefined when out of bounds.
In `@src/test/mainScreen.e2e.test.tsx`:
- Around line 338-341: The tests currently call jest.clearAllMocks() inside a
beforeEach; move that call into an afterEach block instead (for the describe
'メイン画面 E2E: Header (HeaderE235)' and the other describe flagged around lines
514-516) so mocks are cleared after each test; locate the existing beforeEach
that contains jest.clearAllMocks() and replace it with an afterEach containing
jest.clearAllMocks() to satisfy the guideline.
- Around line 1-16: This E2E test is standalone but must extend the shared
integration harness in src/test/e2e.ts instead of calling render directly;
replace direct use of render from '@testing-library/react-native' in
mainScreen.e2e.test.tsx by importing and using the shared setup/render helper
exported by src/test/e2e.ts (e.g., setupTestApp or renderApp from that module)
and move any Jotai state injection into that harness extension so the test uses
the common e2e flow and helpers.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useDeepLink.test.tsx (1)
352-370: リスナーのクリーンアップテストの追加を検討してください。イベントリスナーの登録は検証されていますが、コンポーネントのアンマウント時に
remove関数が呼ばれることを検証するテストがあると、メモリリーク防止の保証が強化されます。♻️ クリーンアップテストの例
it('アンマウント時にURLイベントリスナーを解除する', () => { mockGetInitialURL.mockResolvedValue(null); setupAtoms(); setupQueries(); const mockRemove = jest.fn(); mockAddEventListener.mockReturnValue({ remove: mockRemove }); const { unmount } = render( <HookBridge onReady={() => { /* noop */ }} /> ); unmount(); expect(mockRemove).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDeepLink.test.tsx` around lines 352 - 370, Add a test that ensures the URL event listener is cleaned up on unmount: in the test for HookBridge, set mockGetInitialURL.mockResolvedValue(null), create const mockRemove = jest.fn(), have mockAddEventListener.mockReturnValue({ remove: mockRemove }), render HookBridge, call unmount(), and assert mockRemove was called; this verifies the remove function returned by mockAddEventListener is invoked during component teardown.
🤖 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/useDeepLink.test.tsx`:
- Around line 352-370: Add a test that ensures the URL event listener is cleaned
up on unmount: in the test for HookBridge, set
mockGetInitialURL.mockResolvedValue(null), create const mockRemove = jest.fn(),
have mockAddEventListener.mockReturnValue({ remove: mockRemove }), render
HookBridge, call unmount(), and assert mockRemove was called; this verifies the
remove function returned by mockAddEventListener is invoked during component
teardown.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/hooks/useDeepLink.ts`:
- Around line 166-183: Wrap the async calls to handleUrl in try/catch/finally so
that setInitialUrlProcessed is always invoked even if handleUrl throws: modify
checkInitialUrl to try { await handleUrl(initialUrl) } catch (err) { /* log */ }
finally { setInitialUrlProcessed(true) }; likewise in the Linking event listener
call handleUrl(e.url).catch(...) to swallow/log errors (prevent unhandled
promise rejections) and ensure the cleanup returns a function that calls
listener.remove() to unregister the listener; reference functions/vars:
handleUrl, setInitialUrlProcessed, checkInitialUrl, listener.remove.
- Around line 88-131: The openLink/handleUrl flow assumes lineId is required but
also has a branch that should work with only lineGroupId; update the types and
validation to allow that: make lineId optional (number | undefined) in
openLink's parameter type and any related type defs, change the handleUrl
validation from if (!stationGroupId || !lineId) to if (!stationGroupId ||
(!lineId && !lineGroupId)), and before calling fetchStationsByLineId (and using
lineId) add a guard that lineId is defined so you only follow the lineId path
when present; keep the existing lineGroupId branch as-is.
TinyKitten
changed the title
Member
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.maestro/yamanote-inbound.yaml (1)
44-46:assertTrueのcondition: trueは常に成功するため、実質的なアサーションになっていません。この assertTrue は
condition: trueが固定値のため、テストの成否に関係なく常にパスします。実際の検証はextendedWaitUntilで行われているので、このアサーションは削除するか、実際の状態を検証するように修正することを検討してください。♻️ 修正案: 不要なアサーションを削除
- extendedWaitUntil: visible: text: ".*代々木.*" timeout: 120000 label: "シミュレーション進行: 代々木駅が表示される" - -- assertTrue: - condition: true - label: "内回り: 新宿から代々木への駅進行を確認"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/yamanote-inbound.yaml around lines 44 - 46, The assertTrue step currently uses a fixed condition (assertTrue with condition: true) which always passes and is redundant because the real check is performed by extendedWaitUntil; remove the assertTrue block (or replace it with a meaningful assertion that checks the actual station/state) so the YAML only relies on the existing extendedWaitUntil verification or an assert that references the real condition, and ensure you update or remove the label "内回り: 新宿から代々木への駅進行を確認" accordingly to reflect the retained verification.
🤖 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/hooks/useDeepLink.ts`:
- Around line 117-124: The current useDeepLink flow calls
navigationRef.dispatch(CommonActions.navigate(...)) only when
navigationRef.isReady() is true and lacks a fallback; add a retry/fallback path
so the app doesn't get stuck on the Privacy screen when isReady() is false:
implement a short exponential/backoff retry loop that re-checks
navigationRef.isReady() for a bounded number of attempts (or timeout) and then
dispatches CommonActions.navigate to 'MainStack' -> 'Main' when ready, and if
still not ready, perform a safe fallback such as queuing the intended route in a
pendingNavigation variable or logging and calling a replacement navigation
method (or show a user-friendly error) so the behavior in the block around
navigationRef.isReady() and the similar block at the second occurrence (the code
around the CommonActions.navigate at the other location) both handle the false
case gracefully.
---
Duplicate comments:
In `@src/hooks/useDeepLink.ts`:
- Around line 184-193: The effect's async function checkInitialUrl can skip
calling setInitialUrlProcessed(true) if handleUrl throws; update checkInitialUrl
to wrap the await Linking.getInitialURL() and await handleUrl(initialUrl) in
try/catch and use a finally block to always call setInitialUrlProcessed(true).
Also log or forward the caught error inside the catch so failures in handleUrl
(or Linking.getInitialURL) are visible; this change touches the checkInitialUrl
function within the useEffect that calls Linking.getInitialURL(), handleUrl, and
setInitialUrlProcessed.
- Around line 163-165: The guard currently returns when either stationGroupId or
lineId is missing, preventing URLs that contain only lineGroupId from reaching
the existing lineGroupId handling; change the condition in useDeepLink so it
only returns when both are missing (i.e., require at least one of stationGroupId
or lineId). Update the check that references stationGroupId and lineId to use a
logical AND (both absent) rather than OR, so the later lineGroupId branch can
run.
- Around line 195-201: The event callback passed to Linking.addEventListener
calls handleUrl(e.url) but does not handle its returned Promise, risking
Unhandled Promise Rejection; update the listener to explicitly handle the
Promise (e.g., call void handleUrl(e.url).catch(err => /* log or handle error
*/) or Promise.resolve(handleUrl(e.url)).catch(err => /* log or handle error
*/)) so any rejection is caught, keeping the useEffect cleanup (return
listener.remove) unchanged and referencing the same handleUrl and
listener.remove symbols.
---
Nitpick comments:
In @.maestro/yamanote-inbound.yaml:
- Around line 44-46: The assertTrue step currently uses a fixed condition
(assertTrue with condition: true) which always passes and is redundant because
the real check is performed by extendedWaitUntil; remove the assertTrue block
(or replace it with a meaningful assertion that checks the actual station/state)
so the YAML only relies on the existing extendedWaitUntil verification or an
assert that references the real condition, and ensure you update or remove the
label "内回り: 新宿から代々木への駅進行を確認" accordingly to reflect the retained verification.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useDeepLink.test.tsx (1)
101-117:setupAtomsのモック実装がコール順序に依存しすぎている。
atomCallIdx % 3でセッターを順番に返す実装は、フックの内部実装（useSetAtomの呼び出し順序）に強く依存しています。フックのリファクタリングでアトムの追加や順序変更があるとテストが壊れやすくなります。アトムを明示的に識別してモックするか、少なくとも現状の順序依存を認識できるようコメントを追加することを推奨します。
♻️ 順序依存を明示するコメント追加案
const setupAtoms = () => { const mockSetStationState = jest.fn(); const mockSetNavigationState = jest.fn(); const mockSetLineState = jest.fn(); + // NOTE: フック内のuseSetAtom呼び出し順序に依存 + // 1: stationState, 2: navigationState, 3: lineState const setters = [ mockSetStationState, mockSetNavigationState, mockSetLineState, ]; let atomCallIdx = 0; mockUseSetAtom.mockImplementation(() => { const result = setters[atomCallIdx % 3]; atomCallIdx++; return result; }); return { mockSetStationState, mockSetNavigationState, mockSetLineState }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDeepLink.test.tsx` around lines 101 - 117, The current setupAtoms mock uses an order-dependent implementation of mockUseSetAtom (cycling setters via atomCallIdx % 3), which breaks if useSetAtom call order changes; update the mock so mockUseSetAtom inspects its input atom argument and returns the corresponding setter (map specific atoms to mockSetStationState, mockSetNavigationState, mockSetLineState) instead of relying on call index, or if you prefer to keep the index approach add a clear comment in setupAtoms explaining the order dependency and why the cycle is used; ensure you reference mockUseSetAtom, setupAtoms, and the three setter mocks (mockSetStationState, mockSetNavigationState, mockSetLineState) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/hooks/useDeepLink.ts`:
- Around line 216-233: Wrap calls to handleUrl in try/catch/finally so
setInitialUrlProcessed(true) always runs: inside the checkInitialUrl async
function, await handleUrl(initialUrl) inside a try block, catch and
log/error-handle any thrown errors, and put setInitialUrlProcessed(true) in a
finally block; similarly, update the Linking.addEventListener callback to call
handleUrl(e.url) and handle any returned Promise rejections (e.g., invoke
handleUrl(e.url).catch(...)) to avoid unhandled promise rejections. Target the
checkInitialUrl function, the useEffect that registers Linking.addEventListener,
handleUrl, and setInitialUrlProcessed so DeepLinkProvider no longer blocks on
exceptions.
- Around line 131-141: Confirm whether openLink's parameter contract requires
lineId to always be present or if lineGroupId (lgid) should be usable alone; if
lineId must remain mandatory, add a clarifying comment on openLink and keep the
signature as ({ stationGroupId, direction, lineGroupId, lineId }: {
stationGroupId: number; direction: 0 | 1; lineGroupId?: number; lineId: number
}) and ensure validation/assertion logic uses lineId; if lgid should be a valid
alternative, change the type to make lineId optional (lineId?: number) and
update the validation logic inside openLink to handle cases where only
lineGroupId is provided (and update any callers/tests that assume lid is always
present); reference the openLink function and its parameter object when making
the change and update related tests that construct URLs with lid/lgid.
---
Nitpick comments:
In `@src/hooks/useDeepLink.test.tsx`:
- Around line 101-117: The current setupAtoms mock uses an order-dependent
implementation of mockUseSetAtom (cycling setters via atomCallIdx % 3), which
breaks if useSetAtom call order changes; update the mock so mockUseSetAtom
inspects its input atom argument and returns the corresponding setter (map
specific atoms to mockSetStationState, mockSetNavigationState, mockSetLineState)
instead of relying on call index, or if you prefer to keep the index approach
add a clear comment in setupAtoms explaining the order dependency and why the
cycle is used; ensure you reference mockUseSetAtom, setupAtoms, and the three
setter mocks (mockSetStationState, mockSetNavigationState, mockSetLineState)
when making the change.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useDeepLink.test.tsx (1)
549-597: リトライ待ちのテストは fake timers で短縮すると安定します。
実時間待ち（約1.5秒）が入るため、タイマーを進めて検証するとテスト時間とフレークを抑えられます。♻️ 例: fake timers で待機を短縮
it('ナビゲーターが未準備の場合はリトライ後にナビゲートしない', async () => { + jest.useFakeTimers(); const mockDispatch = navigationRef.dispatch as jest.Mock; const mockIsReady = navigationRef.isReady as jest.Mock; mockIsReady.mockReturnValue(false); @@ - await waitFor( - () => { - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('not ready after retries') - ); - }, - { timeout: 5000 } - ); + await act(async () => { + jest.advanceTimersByTime(2000); + }); + await waitFor(() => { + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('not ready after retries') + ); + }); + jest.useRealTimers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDeepLink.test.tsx` around lines 549 - 597, Use fake timers to avoid real 1.5s backoff in this test: call jest.useFakeTimers() before rendering HookBridge, trigger the code path (render HookBridge), then advance timers by the total retry backoff (about 1500ms) inside an act (e.g., await act(async () => { jest.advanceTimersByTime(1500) })) so the retry loop driven by navigationRef.isReady (mockIsReady) runs immediately; afterward assert warnSpy was called. Ensure to restore timers (jest.useRealTimers()) after the assertion. Reference: mockIsReady, navigationRef, HookBridge, warnSpy, and the waitFor assertion.
🤖 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/useDeepLink.test.tsx`:
- Around line 549-597: Use fake timers to avoid real 1.5s backoff in this test:
call jest.useFakeTimers() before rendering HookBridge, trigger the code path
(render HookBridge), then advance timers by the total retry backoff (about
1500ms) inside an act (e.g., await act(async () => {
jest.advanceTimersByTime(1500) })) so the retry loop driven by
navigationRef.isReady (mockIsReady) runs immediately; afterward assert warnSpy
was called. Ensure to restore timers (jest.useRealTimers()) after the assertion.
Reference: mockIsReady, navigationRef, HookBridge, warnSpy, and the waitFor
assertion.
Member
Author
|
@coderabbitai ほかになければapproveしてください |
Contributor
|
🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useDeepLink/useOpenRouteFromLink）を削除し、SelectBoundModal.handleBoundSelectedパターンに準拠して再実装navigationRefを導入し、ランタイムディープリンクでもMain画面へ明示的にナビゲートDeepLinkProviderにinitialUrlProcessedフラグを追加し、初回URL処理完了までレンダリングをブロックするレースコンディション修正stopApp+openLinkによるコールドスタートディープリンク、permissions: { all: allow }でPrivacy画面バイパス）Key Changes
src/hooks/useDeepLink.ts:lid必須化、lgidオプション、CommonActions.navigateによるランタイムナビゲーションsrc/stacks/rootNavigation.ts:createNavigationContainerRefでnavigationRefをエクスポートsrc/index.tsx:NavigationContainerにref={navigationRef}を設定src/providers/DeepLinkProvider.tsx:initialUrlProcessedでレンダリングゲート.maestro/yamanote-inbound.yaml/yamanote-outbound.yaml: 新宿駅起点の駅進行シミュレーションE2Eテストsrc/hooks/useOpenRouteFromLink.ts/.test.tsx: 削除Test plan
pnpm test全134スイート、1241テスト合格pnpm lintパスpnpm typecheckパスmaestro test .maestro/yamanote-inbound.yaml）https://claude.ai/code/session_01XAuZBUUyZ8AbCLWSZyHtsh
Summary by CodeRabbit
テスト
バグ修正
リファクタ
雑作業