Merged
Conversation
Contributor
📝 WalkthroughWalkthrough開発者オーバーレイがドラッグ可能な対話型パネルに差し替えられ、表示/非表示トグル用の翻訳キーと長押しメニュー経由のトグルおよび AsyncStorage 永続化が追加されました。 変更内容
シーケンス図sequenceDiagram
participant User as User
participant Permitted as Permitted\n(menu)
participant Tuning as TuningState
participant Storage as AsyncStorage
participant DevOverlay as DevOverlay
User->>Permitted: 長押し → Show/Hide Dev Overlay を選択
Permitted->>Tuning: devOverlayEnabled を更新
Permitted->>Storage: setItem('@TrainLCD:devOverlayEnabled', value)
Storage-->>Permitted: 保存完了
Tuning-->>DevOverlay: devOverlayEnabled の変化を通知
User->>DevOverlay: パネルをドラッグ
DevOverlay->>DevOverlay: 位置を計算・クランプして保持
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~60 minutes 関連する可能性のある PR
ポエム
🚥 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: 2
🧹 Nitpick comments (1)
src/components/DevOverlay.test.tsx (1)
84-95: 未対応の atom は即失敗させた方が安全です。未知の
atomをundefinedで返すと、DevOverlayが新しいuseAtomValue依存を増やしてもセットアップ漏れを見逃しやすいです。ここはthrowで落としておく方が原因を追いやすいです。💡 修正例
if (atom === backgroundLocationTrackingAtom) { return backgroundLocationTracking as never; } - return undefined as never; + throw new Error('Unhandled atom in DevOverlay test'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.test.tsx` around lines 84 - 95, The mockUseAtomValue implementation in the test should fail fast for unknown atoms: update the mock inside DevOverlay.test.tsx (the mockUseAtomValue function) so that after checking locationAtom, accuracyHistoryAtom, and backgroundLocationTrackingAtom it throws an error (including the atom value in the message) instead of returning undefined; this ensures any new useAtomValue dependency in DevOverlay will cause the test to fail and force adding the required mock.
🤖 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/components/DevOverlay.tsx`:
- Around line 511-528: The accuracy placeholder '---' is being rendered twice
when accuracyChartBlocks.length === 0 (once in the first Typography and again in
the second with testID "dev-overlay-accuracy-history"); update DevOverlay.tsx so
only one Typography renders the placeholder: remove the first conditional
placeholder (keep the first Typography for spacing/styling but render null when
empty) and let the second Typography handle either the mapped blocks or the
single '---' placeholder (also apply the same change for the other occurrence
around the 583-600 block). Ensure you reference accuracyChartBlocks,
chartValueStyle, styles.chartValue and the testID "dev-overlay-accuracy-history"
when making the change.
In `@src/components/Permitted.tsx`:
- Around line 241-250: handler 内で AsyncStorage.setItem
の失敗をハンドルするため、現在の楽観的更新を保持する前の値を退避し、setTuning で UI を更新した後に try/catch で await
AsyncStorage.setItem(ASYNC_STORAGE_KEYS.DEV_OVERLAY_ENABLED, String(nextValue))
を実行してください。catch ブロックでは保存に失敗した場合に元の値へロールバックするために setTuning(prev => ({ ...prev,
devOverlayEnabled: prevValue })) を呼び出し（必要に応じてエラーログ／通知も出す）、handler は async
のまま内部で例外を消化して未処理の rejection が残らないようにしてください（参照する識別子: devOverlayEnabled,
setTuning, ASYNC_STORAGE_KEYS.DEV_OVERLAY_ENABLED, handler）。
---
Nitpick comments:
In `@src/components/DevOverlay.test.tsx`:
- Around line 84-95: The mockUseAtomValue implementation in the test should fail
fast for unknown atoms: update the mock inside DevOverlay.test.tsx (the
mockUseAtomValue function) so that after checking locationAtom,
accuracyHistoryAtom, and backgroundLocationTrackingAtom it throws an error
(including the atom value in the message) instead of returning undefined; this
ensures any new useAtomValue dependency in DevOverlay will cause the test to
fail and force adding the required mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2f05197-5b32-44be-bfa7-0552d121698f
📒 Files selected for processing (6)
assets/translations/en.jsonassets/translations/ja.jsonsrc/components/DevOverlay.test.tsxsrc/components/DevOverlay.tsxsrc/components/Permitted.tsxsrc/constants/asyncStorage.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/DevOverlay.tsx (1)
519-540:⚠️ Potential issue | 🟡 Minor空の精度履歴プレースホルダが二重描画されています。
accuracyChartBlocks.length === 0のとき、上段のTypographyとtestID="dev-overlay-accuracy-history"側の両方が---を出しているので、空状態だけ 2 行になります。プレースホルダは 1 箇所に寄せた方が自然です。修正例
- <Typography style={[styles.chartValue, chartValueStyle]}> - {accuracyChartBlocks.length === 0 ? '---' : null} - </Typography> <Typography style={[styles.chartValue, chartValueStyle]} testID="dev-overlay-accuracy-history" > @@ - <Typography style={[styles.chartValue, chartValueStyle]}> - {accuracyChartBlocks.length === 0 ? '---' : null} - </Typography> <Typography style={[styles.chartValue, chartValueStyle]} testID="dev-overlay-accuracy-history" >Also applies to: 591-612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.tsx` around lines 519 - 540, The empty accuracy-history placeholder is rendered twice: once in the first Typography (style={styles.chartValue, chartValueStyle}) and again inside the Typography with testID="dev-overlay-accuracy-history"; remove the duplicate by rendering the '---' placeholder only in the testID="dev-overlay-accuracy-history" block and make the earlier Typography return null when accuracyChartBlocks.length === 0 (or simply omit the placeholder there); apply the same change to the parallel accuracy/history block handled later in DevOverlay (the other accuracy chart rendering).
🧹 Nitpick comments (1)
src/components/DevOverlay.test.tsx (1)
84-95: 未知の atom を黙ってundefinedにしない方がいいです。ここがフォールバックで通ってしまうと、
DevOverlayの atom 依存が増えた時にテストが誤って緑のまま残ります。unexpected atom は throw して fail-fast にしておく方が安全です。修正例
if (atom === backgroundLocationTrackingAtom) { return backgroundLocationTracking as never; } - return undefined as never; + throw new Error('Unexpected atom passed to useAtomValue mock'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.test.tsx` around lines 84 - 95, The mockUseAtomValue implementation used in DevOverlay.test.tsx currently falls back to returning undefined for unknown atoms, which can hide missing dependencies; update the mockImplementation for mockUseAtomValue so it throws an error for any atom that is not one of the expected atoms (locationAtom, accuracyHistoryAtom, backgroundLocationTrackingAtom) — e.g., detect the unexpected atom in the final branch and throw a descriptive Error including the atom identity to fail fast and surface new atom requirements.
🤖 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/components/DevOverlay.tsx`:
- Around line 36-51: The root style currently mixes shadow props and overflow:
'hidden' (root) which on iOS clips shadows; modify the DevOverlay component to
split this into two Views: an outer wrapper that keeps the shadow-related styles
(shadowColor, shadowOpacity, shadowRadius, shadowOffset, elevation, zIndex) and
does NOT set overflow or borderRadius clipping, and an inner wrapper that
applies borderRadius, overflow: 'hidden', borderWidth, borderColor and
backgroundColor to handle rounded corners and content clipping; update any
references to the root style accordingly and verify shadows on iOS
devices/simulator.
---
Duplicate comments:
In `@src/components/DevOverlay.tsx`:
- Around line 519-540: The empty accuracy-history placeholder is rendered twice:
once in the first Typography (style={styles.chartValue, chartValueStyle}) and
again inside the Typography with testID="dev-overlay-accuracy-history"; remove
the duplicate by rendering the '---' placeholder only in the
testID="dev-overlay-accuracy-history" block and make the earlier Typography
return null when accuracyChartBlocks.length === 0 (or simply omit the
placeholder there); apply the same change to the parallel accuracy/history block
handled later in DevOverlay (the other accuracy chart rendering).
---
Nitpick comments:
In `@src/components/DevOverlay.test.tsx`:
- Around line 84-95: The mockUseAtomValue implementation used in
DevOverlay.test.tsx currently falls back to returning undefined for unknown
atoms, which can hide missing dependencies; update the mockImplementation for
mockUseAtomValue so it throws an error for any atom that is not one of the
expected atoms (locationAtom, accuracyHistoryAtom,
backgroundLocationTrackingAtom) — e.g., detect the unexpected atom in the final
branch and throw a descriptive Error including the atom identity to fail fast
and surface new atom requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50091b64-189c-4554-9a82-05878fa84814
📒 Files selected for processing (2)
src/components/DevOverlay.test.tsxsrc/components/DevOverlay.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/DevOverlay.tsx (1)
36-51:⚠️ Potential issue | 🟡 Minor
overflow: 'hidden'と shadow を同じレイヤーに置くと、iOS で影が欠けます。
styles.rootはまだ shadow 描画と内容クリップを同居させているので、iOS ではカード外側の影が切れます。shadow 用の外側ラッパーと、borderRadius/overflow: 'hidden'用の内側ラッパーを分けた方が安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.tsx` around lines 36 - 51, styles.root currently mixes shadow styles and overflow: 'hidden', which causes clipped shadows on iOS; update the DevOverlay JSX/CSS by splitting the wrapper into two: an outer wrapper (e.g., DevOverlayShadowWrapper) that contains the shadow props (shadowColor, shadowOpacity, shadowRadius, shadowOffset, elevation, zIndex) and the positioning, and an inner wrapper (e.g., DevOverlayClipWrapper) that contains borderRadius and overflow: 'hidden' plus the PANEL_BORDER/PANEL_BG styles and children; remove overflow: 'hidden' and borderRadius from styles.root so shadows render on the outer layer while content is clipped by the inner layer.
🧹 Nitpick comments (1)
src/components/DevOverlay.test.tsx (1)
117-192: 横画面分岐を固定したケースも 1 本ほしいです。
DevOverlayは縦横で JSX が大きく分岐しますが、今の assertions はどちらの分岐でも通るものが中心なので、今回の landscape レイアウト変更を実質固定できていません。寸法を明示的に切り替えたケースを追加し、必要なら分岐コンテナに軽いtestIDを付けておくと、この PR の主変更点を守りやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.test.tsx` around lines 117 - 192, Tests currently don't assert the landscape-specific JSX branch of DevOverlay; add a new test (e.g., "横画面レイアウトを表示する") that forces landscape dimensions before calling render(<DevOverlay />) (mock the window dimensions or use a helper like setWindowSize/override Dimensions) and then assert a landscape-only element is present; if the branch lacks a stable selector, add a lightweight testID to the landscape container (e.g., testID="dev-overlay-landscape") in the DevOverlay component and assert getByTestId('dev-overlay-landscape') exists and/or its specific text/layout differences to lock the landscape branch behavior.
🤖 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/components/DevOverlay.tsx`:
- Around line 428-451: The two pan responder handlers are asymmetric:
onPanResponderRelease sets hasDraggedRef.current = true but
onPanResponderTerminate does not, causing a false "not dragged" state when
dragTranslation triggers the effect; update the onPanResponderTerminate handler
(the block using dragTranslation.stopAnimation and computing clampedPosition via
clampPosition with basePositionRef, panelWidth and panelHeight) to also set
hasDraggedRef.current = true before calling setBasePosition and
dragTranslation.setValue({ x: 0, y: 0 }) so both handlers behave identically.
---
Duplicate comments:
In `@src/components/DevOverlay.tsx`:
- Around line 36-51: styles.root currently mixes shadow styles and overflow:
'hidden', which causes clipped shadows on iOS; update the DevOverlay JSX/CSS by
splitting the wrapper into two: an outer wrapper (e.g., DevOverlayShadowWrapper)
that contains the shadow props (shadowColor, shadowOpacity, shadowRadius,
shadowOffset, elevation, zIndex) and the positioning, and an inner wrapper
(e.g., DevOverlayClipWrapper) that contains borderRadius and overflow: 'hidden'
plus the PANEL_BORDER/PANEL_BG styles and children; remove overflow: 'hidden'
and borderRadius from styles.root so shadows render on the outer layer while
content is clipped by the inner layer.
---
Nitpick comments:
In `@src/components/DevOverlay.test.tsx`:
- Around line 117-192: Tests currently don't assert the landscape-specific JSX
branch of DevOverlay; add a new test (e.g., "横画面レイアウトを表示する") that forces
landscape dimensions before calling render(<DevOverlay />) (mock the window
dimensions or use a helper like setWindowSize/override Dimensions) and then
assert a landscape-only element is present; if the branch lacks a stable
selector, add a lightweight testID to the landscape container (e.g.,
testID="dev-overlay-landscape") in the DevOverlay component and assert
getByTestId('dev-overlay-landscape') exists and/or its specific text/layout
differences to lock the landscape branch behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1be44186-3aae-481d-b9f0-cdaeae0a2683
📒 Files selected for processing (3)
src/components/DevOverlay.test.tsxsrc/components/DevOverlay.tsxsrc/components/Permitted.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/DevOverlay.tsx (3)
286-295: 速度計算のロジックは正しいですが、若干複雑です。
coordsSpeedを計算した後、speedKMHで再度speedをチェックしています。意図通り動作しますが、将来のメンテナンスのため、coordsSpeedのみを使用する方がシンプルです。♻️ 簡略化の提案
const speedKMH = useMemo( () => - ( - (speed && Math.round((coordsSpeed * 3600) / 1000)) ?? - 0 - ).toLocaleString(), - [coordsSpeed, speed] + Math.round((coordsSpeed * 3600) / 1000).toLocaleString(), + [coordsSpeed] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.tsx` around lines 286 - 295, The speedKMH computation currently re-checks speed even after coordsSpeed was derived; simplify by computing kilometers-per-hour solely from coordsSpeed inside the useMemo for speedKMH (remove the secondary speed null/undefined checks), convert coordsSpeed to km/h via Math.round((coordsSpeed * 3600) / 1000) (or 0 when coordsSpeed is falsy), call .toLocaleString() on that result, and update the useMemo dependency array to only [coordsSpeed] so speedKMH depends exclusively on coordsSpeed.
305-307:?? undefinedは冗長です。オプショナルチェーンが失敗した場合、既に
undefinedが返されます。♻️ 冗長なコードの削除
const nextStationNumber = nextStation?.stationNumbers?.find((item) => !!item?.stationNumber) - ?.stationNumber ?? undefined; + ?.stationNumber;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.tsx` around lines 305 - 307, The expression assigning nextStationNumber uses a redundant nullish coalescing "?? undefined"; remove the "?? undefined" so nextStationNumber is assigned directly from nextStation?.stationNumbers?.find(item => !!item?.stationNumber)?.stationNumber, relying on optional chaining to yield undefined when absent; update the assignment of nextStationNumber accordingly (reference symbols: nextStationNumber, nextStation, stationNumbers, stationNumber).
529-671: 横画面・縦画面のレイアウトが条件分岐で実装されています。両レイアウト間でコード重複がありますが、レイアウト固有の調整が必要なため、現在の実装は許容範囲です。将来的にメトリックカード群を共通コンポーネントとして抽出することで、保守性が向上する可能性があります。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DevOverlay.tsx` around lines 529 - 671, There is duplicated JSX between the isLandscape true/false branches (accuracy chart + three MetricCard instances) — extract a shared presentational component (e.g., MetricCardsGroup) that renders the three MetricCard elements using props: accuracyChartBlocks, speedKMH, distanceToNextStation, nextStationMeta, metricCardStyle, metricLabelStyle, metricValueStyle, metricMetaStyle and allow layout overrides (props for container width/gap e.g., metricWidth, leftMetricWidth, nextCardWidth, metricsGap, nextTargetCardStyle); keep the existing accuracy chart rendering (accuracyChartBlocks, chartShellStyle, chartValueStyle) and conditional container wrappers (isLandscape branch uses styles.landscapeTopRow/landscapeSubGrid, portrait uses styles.metricsGrid) but replace the duplicated MetricCard blocks with the new MetricCardsGroup component so layout-specific widths/styles are passed in from each branch and functionality of MetricCard, accuracy, speedKMH, and nextStationMeta remains unchanged.
🤖 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/components/DevOverlay.tsx`:
- Around line 286-295: The speedKMH computation currently re-checks speed even
after coordsSpeed was derived; simplify by computing kilometers-per-hour solely
from coordsSpeed inside the useMemo for speedKMH (remove the secondary speed
null/undefined checks), convert coordsSpeed to km/h via Math.round((coordsSpeed
* 3600) / 1000) (or 0 when coordsSpeed is falsy), call .toLocaleString() on that
result, and update the useMemo dependency array to only [coordsSpeed] so
speedKMH depends exclusively on coordsSpeed.
- Around line 305-307: The expression assigning nextStationNumber uses a
redundant nullish coalescing "?? undefined"; remove the "?? undefined" so
nextStationNumber is assigned directly from
nextStation?.stationNumbers?.find(item => !!item?.stationNumber)?.stationNumber,
relying on optional chaining to yield undefined when absent; update the
assignment of nextStationNumber accordingly (reference symbols:
nextStationNumber, nextStation, stationNumbers, stationNumber).
- Around line 529-671: There is duplicated JSX between the isLandscape
true/false branches (accuracy chart + three MetricCard instances) — extract a
shared presentational component (e.g., MetricCardsGroup) that renders the three
MetricCard elements using props: accuracyChartBlocks, speedKMH,
distanceToNextStation, nextStationMeta, metricCardStyle, metricLabelStyle,
metricValueStyle, metricMetaStyle and allow layout overrides (props for
container width/gap e.g., metricWidth, leftMetricWidth, nextCardWidth,
metricsGap, nextTargetCardStyle); keep the existing accuracy chart rendering
(accuracyChartBlocks, chartShellStyle, chartValueStyle) and conditional
container wrappers (isLandscape branch uses
styles.landscapeTopRow/landscapeSubGrid, portrait uses styles.metricsGrid) but
replace the duplicated MetricCard blocks with the new MetricCardsGroup component
so layout-specific widths/styles are passed in from each branch and
functionality of MetricCard, accuracy, speedKMH, and nextStationMeta remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba7855f4-1065-49ab-999e-6de0f1aca18f
📒 Files selected for processing (2)
src/components/DevOverlay.test.tsxsrc/components/DevOverlay.tsx
This was referenced Mar 11, 2026
Merged
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
1 participant
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 by CodeRabbit