Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthroughおじさんが詳しく説明しちゃおうカナ〜 ✨ この PR では、スノードーム（snowdome）という新しいアイテム配置機能を実装してるヨ。カレンダーから取得したスノードームを 3D シーンにドラッグして配置する仕組みなんだネ。複数パーツで構成される特殊アイテムの取得・在庫管理・配置という流れを追加してるんダヨ〜 🎵 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes おじさんが見どころをまとめちゃったヨ〜 (´ω｀)✨
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
🚀 Deploy Preview Ready!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
frontend/src/features/room/calendar.tsx (1)
91-98: 重複したstopPropagation()呼び出しがあるヨ〜 ✨おじさんコード見てたら気づいちゃったんダケド、92行目で無条件に
e.stopPropagation()呼んでるから、95行目の同じ呼び出しは冗長になっちゃってるネ (´ω｀)配置モードでのイベント制御には92行目だけで十分ダヨ〜 🎵
🔎 冗長な呼び出しを削除する提案
onClick={(e) => { e.stopPropagation(); // フォーカスモードでない場合は、カレンダー全体クリックでフォーカスモードへ if (!isFocusMode) { - e.stopPropagation(); onCalendarClick(); } }}frontend/src/features/room/placedItems.tsx (1)
138-142: イベントの型が合ってないかもネ〜 🤔おじさん、Three.js詳しいんダケド、ここの
handleClickの型がReact.MouseEvent<HTMLDivElement>になってるヨ〜でも147行目の
<group onClick={handleClick}>は Three.js のグループで、渡されるイベントはThreeEvent<MouseEvent>なんダヨネ (´ω｀)PlacedItem の方（216行目）も同じ型使ってるから統一はされてるケド、正確な型にした方が安心ダヨ〜 ✨
🔎 型を修正する提案
+import type { ThreeEvent } from "@react-three/fiber"; - const handleClick = (e: React.MouseEvent<HTMLDivElement>) => { + const handleClick = (e: ThreeEvent<MouseEvent>) => { e.stopPropagation(); // 最初のパーツをクリックとして扱う onItemClick(snowdomeParts[0]); };frontend/src/features/room/draggableSnowdome.tsx (2)
47-51: GLBのプリロード、ループの外に出した方がいいカモ〜 🌟おじさん、パフォーマンスチューニング得意なんダケド、ここの
useGLTF.preloadがレンダーごとに毎回呼ばれちゃってるヨ〜 (´ω｀)drei の preload は内部でキャッシュするから大きな問題にはならないケド、コンポーネント外でやるか、useEffect 使った方がキレイダネ ✨
🔎 useEffectを使う提案
+ // 各パーツのGLBをプリロード + useEffect(() => { + for (const part of snowdomeParts) { + const modelUrl = `${R2_BASE_URL}/item/object/${part.item.id}.glb`; + useGLTF.preload(modelUrl); + } + }, [snowdomeParts]); - // 各パーツのGLBをプリロード - for (const part of snowdomeParts) { - const modelUrl = `${R2_BASE_URL}/item/object/${part.item.id}.glb`; - useGLTF.preload(modelUrl); - }
10-22: 未使用のpropがあるヨ〜 ✨おじさん細かいとこ気になっちゃうタイプでネ〜 (^_^)v
16行目の
isPlacementValidprop、インターフェースで定義されてるケド、35行目のdestructuringで受け取ってないし使われてないみたいダヨ〜🔎 未使用propを削除する提案
interface DraggableSnowdomeProps { snowdomeParts: CalendarItemWithItem[]; onPositionChange?: ( position: [number, number, number], rotation: [number, number, number], ) => void; - isPlacementValid: boolean; setIsPlacementValid: (valid: boolean) => void; initialRotation?: [number, number, number]; onLockChange?: (isLocked: boolean) => void; roomRef: React.RefObject<THREE.Group>; placedItemsRef?: React.RefObject<THREE.Group>; }frontend/src/features/room/hooks/useItemAcquisition.ts (3)
256-271: エラーハンドリングを検討してネ〜 🌟おじさん、async処理に詳しいんだけどサ〜 🎵
patchCalendarItemが失敗した時のエラーハンドリングがないから、ユーザーには何が起きたか分からないカモ (´ω｀)try-catchでエラーをキャッチして、UIにフィードバックを返す仕組みがあるといいカナ〜って思ったヨ✨
404-418: Promise.allの部分的失敗に注意してネ〜 (´ω｀)おじさん並列処理には詳しいんだけどサ〜 🎵
Promise.allは1つでも失敗すると全体が reject されるんダヨ〜でも、最初のpatchが成功して2番目が失敗した場合、一部のパーツだけ戻っちゃう不整合が起きる可能性があるカナ〜🤔
Promise.allSettledを使って結果を確認するか、トランザクション的なAPIがあれば検討してみてネ✨
501-503: setTimeoutのクリーンアップを検討してネ〜 🌟おじさんReactのライフサイクルに詳しいんだけどサ〜 この
setTimeout、コンポーネントがアンマウントされた後も実行される可能性があるんダヨ〜 (´ω｀)メモリリークや警告の原因になることがあるから、
useEffectのクリーンアップと組み合わせるか、アンマウント時にタイマーをキャンセルする仕組みがあるといいカナ〜って思ったヨ✨ただ、100msと短いから実害は少ないかもネ🎵
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/features/room/calendar.tsx(1 hunks)frontend/src/features/room/draggableSnowdome.tsx(1 hunks)frontend/src/features/room/hooks/useCalendarFocus.ts(1 hunks)frontend/src/features/room/hooks/useItemAcquisition.ts(5 hunks)frontend/src/features/room/placedItems.tsx(3 hunks)frontend/src/routes/$roomId/index.lazy.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,jsx}: Convert Figma-generated custom Tailwind CSS values to standard Tailwind values: rounded-[24px] → rounded-3xl, gap-[32px] → gap-8, p-[12px] → p-3, text-[20px] → text-xl, etc.
Use design system colors defined in frontend/src/styles.css instead of custom hex values or rgba: use bg-primary, text-foreground, border-border, etc.
Prioritize shadcn/ui components from frontend/src/components/ui/ (Button, Input, Dialog, Field, Label, Calendar, Popover, RadioGroup, Separator, Switch, Spinner) instead of creating custom components
Remove data-node-id attributes from Figma-generated code
Remove non-standard Tailwind classes like content-stretch
Use project default font instead of custom font specifications like font-['Noto_Sans_JP:Bold',sans-serif]
Adjust absolute positioning values (left-[232px] top-[216px]) based on actual use case rather than using Figma-generated coordinates
Files:
frontend/src/features/room/draggableSnowdome.tsxfrontend/src/features/room/placedItems.tsxfrontend/src/routes/$roomId/index.lazy.tsxfrontend/src/features/room/calendar.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-18T15:18:17.646Z
Learnt from: CR
Repo: AdventSphere/advent-sphere PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T15:18:17.646Z
Learning: Applies to **/*.{tsx,jsx} : Adjust absolute positioning values (left-[232px] top-[216px]) based on actual use case rather than using Figma-generated coordinates
Applied to files:
frontend/src/features/room/hooks/useCalendarFocus.ts
📚 Learning: 2025-12-18T15:18:17.646Z
Learnt from: CR
Repo: AdventSphere/advent-sphere PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T15:18:17.646Z
Learning: Applies to **/*.{tsx,jsx} : Prioritize shadcn/ui components from frontend/src/components/ui/ (Button, Input, Dialog, Field, Label, Calendar, Popover, RadioGroup, Separator, Switch, Spinner) instead of creating custom components
Applied to files:
frontend/src/routes/$roomId/index.lazy.tsx
🧬 Code graph analysis (2)
frontend/src/features/room/draggableSnowdome.tsx (2)
common/generate/adventSphereAPI.schemas.ts (1)
CalendarItemWithItem(54-54)frontend/src/constants/r2-url.ts (1)
R2_BASE_URL(1-2)
frontend/src/features/room/hooks/useItemAcquisition.ts (1)
common/generate/adventSphereAPI.schemas.ts (2)
CalendarItemWithItem(54-54)Room(220-239)
🔇 Additional comments (13)
frontend/src/features/room/hooks/useCalendarFocus.ts (1)
41-41: LGTM! カメラ距離の調整、いい感じダネ〜 🌟おじさんこういうUX調整、詳しいヨ〜 (^_^)v
0.6から0.9に変更することで、カレンダー全体がより見やすくなるネ！配置モード追加に合わせた調整として適切ダヨ〜 ✨
frontend/src/features/room/placedItems.tsx (1)
47-65: snowdomeのグループ化ロジック、よくできてるネ〜 🎵おじさん感心しちゃったヨ〜 (^_^)v
位置ベースでパーツをグループ化する発想、スマートダネ！
JSON.stringifyでキー作るのもシンプルで良いヨ〜 ✨frontend/src/features/room/draggableSnowdome.tsx (1)
223-250: 全体的な実装、良くできてるネ〜 🌟おじさん、物理演算とレイキャスト組み合わせたドラッグ配置、結構難しいの知ってるヨ〜 (^_^)v
kinematicPositionでの滑らかな追従、ロック/アンロックの切り替え、回転操作、視覚的フィードバック、全部しっかり実装されてて感心しちゃったヨ〜 ✨🎵
frontend/src/routes/$roomId/index.lazy.tsx (3)
394-396: snowdome配置時のexcludeItemIdも考慮した方がいいカモ〜 🤔おじさん、ここちょっと気になったんダケド、
excludeItemIdがisPlacementModeの時だけ設定されてて、isSnowdomePlacementModeの時は考慮されてないヨ〜snowdome配置中も
targetCalendarItemは除外した方がいいんじゃないカナ？ (´ω｀)🔎 isAnyPlacementModeを使う提案
excludeItemId={ - isPlacementMode ? (targetCalendarItem?.id ?? null) : null + isAnyPlacementMode ? (targetCalendarItem?.id ?? null) : null }ただ、snowdomeの場合は複数パーツをまとめて除外する必要があるかもしれないから、実際の挙動を確認してネ〜 ✨
228-256: snowdomeパーツの計算ロジック、しっかりしてるネ〜 🌟おじさん感心しちゃったヨ〜 (^_^)v
再配置と新規配置の両方のケースを考慮して、重複除去もちゃんとやってるネ！useMemoで依存配列もバッチリダヨ〜 ✨🎵
418-435: DraggableSnowdomeの統合、キレイにできてるネ〜 ✨おじさん、こういう条件付きレンダリングのパターン好きダヨ〜 (^_^)v
snowdomePartsForPlacement.length > 0のガードも入ってて安全ダネ！refの型アサーションもPlacementDraggableItemと統一されてて良いヨ〜 🎵frontend/src/features/room/hooks/useItemAcquisition.ts (7)
14-19: LGTM! ✨新しい
snowdome_placementフェーズの追加、すっきりしてていいネ〜 (^_^)v おじさん的にもこの設計は分かりやすいと思うヨ🎵
31-39: タイムゾーンの一貫性を確認してネ〜 🌟おじさん、ちょっと気になっちゃったんだけどサ〜
getFullYear(),getMonth(),getDate()はローカルタイムゾーンで動くんダヨ〜 (´ω｀)もし
room.snowDomePartsLastDateがUTCで保存されてる場合、日付の境界でズレが起きる可能性があるカナ？サーバー側のタイムゾーン設定と合わせて確認してみてネ✨
133-135: LGTM! 🎵プレースホルダーから実際の日時チェックに修正されてるネ〜 おじさん的にはシンプルで良いと思うヨ✨ (^_^)v
206-235: LGTM! ✨おじさん、浮動小数点の比較で
0.001のイプシロン使ってるの見て感心しちゃったヨ〜 🎵 これ、3D座標比較のベストプラクティスだからネ (^_^)v
getInventorySnowdomePartsとgetPlacedSnowdomePartsAtPositionの役割分担もきれいダヨ〜✨
362-388: LGTM! 🎵snowdomeの配置済み/インベントリ両方のケースをちゃんと分岐してるネ〜✨ おじさん的には
allParts[0] || calendarItemのフォールバックも安全でいいと思うヨ (^_^)v
467-480: 重複除去の実装、いいネ〜! ✨おじさん見てて思ったんだけど、Lines 467-469で
targetCalendarItemを追加してから Lines 477-480 で重複除去してるの、ちゃんと考えられてるネ〜 🎵
targetCalendarItemがすでにインベントリに含まれてる可能性があるから、この処理は必要ダヨ (^_^)v
516-534: LGTM! 🎵新しいsnowdome関連のヘルパーとハンドラーがきれいにエクスポートされてるネ〜✨ おじさん的にはAPIの設計がすっきりしてて使いやすそうダヨ (^_^)v
getInventorySnowdomePartsとgetPlacedSnowdomePartsAtPositionを外部に公開してるの、他のコンポーネントから再利用できていいネ〜🌟
| // レイキャストがヒットしない場合は配置不可 | ||
| if (!intersects.length) { | ||
| setIsPlacementValid(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
このコード、到達しないヨ〜 💦
おじさん気づいちゃったんダケド、106行目で !intersects.length の時にすでに return してるから、143-147行目には絶対到達しないヨ〜 (´ω｀)
デッドコードになっちゃってるから削除した方がスッキリするネ 🎵
🔎 デッドコードを削除する提案
rigidBodyRef.current.setRotation(
{ x: quaternion.x, y: quaternion.y, z: quaternion.z, w: quaternion.w },
true,
);
- // レイキャストがヒットしない場合は配置不可
- if (!intersects.length) {
- setIsPlacementValid(false);
- return;
- }
-
// 仮：床に当たっていれば配置可能
setIsPlacementValid(true);
});📝 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.
| // レイキャストがヒットしない場合は配置不可 | |
| if (!intersects.length) { | |
| setIsPlacementValid(false); | |
| return; | |
| } | |
| rigidBodyRef.current.setRotation( | |
| { x: quaternion.x, y: quaternion.y, z: quaternion.z, w: quaternion.w }, | |
| true, | |
| ); | |
| // 仮：床に当たっていれば配置可能 | |
| setIsPlacementValid(true); | |
| }); |
🤖 Prompt for AI Agents
frontend/src/features/room/draggableSnowdome.tsx around lines 143 to 147: the
check for !intersects.length and its setIsPlacementValid(false); return; is
unreachable because an identical check and early return already occurs at line
106, so remove these lines to eliminate dead code; after deletion, run a quick
lint/typecheck and ensure no other logic depended on the duplicate branch (no
other side-effects), keeping the single early-return path at line 106 as the
sole handling for a miss.
| const handleNextFromGetModal = useCallback(async () => { | ||
| if (!targetCalendarItem || !room) { | ||
| setPhase("placement"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
早期リターンのロジックを確認してネ〜 (´ω｀)
おじさんちょっと心配なんだけどサ〜 targetCalendarItem や room が null の時に "placement" フェーズに進むのは意図通りカナ？🤔
配置画面で targetCalendarItem が null だと何も配置できないと思うんだけど、ここは "idle" に戻すか、早期リターンで何もしない方が安全じゃないカナ〜？✨
🔎 おじさんの提案ダヨ〜
const handleNextFromGetModal = useCallback(async () => {
if (!targetCalendarItem || !room) {
- setPhase("placement");
return;
}📝 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.
| const handleNextFromGetModal = useCallback(async () => { | |
| if (!targetCalendarItem || !room) { | |
| setPhase("placement"); | |
| return; | |
| } | |
| const handleNextFromGetModal = useCallback(async () => { | |
| if (!targetCalendarItem || !room) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/features/room/hooks/useItemAcquisition.ts around lines 240 to
244, the early-return currently sets phase to "placement" when
targetCalendarItem or room is null which can lead to an unusable placement
screen; change the behavior to set phase to "idle" (or alternatively perform a
no-op early return) so the UI returns to a safe state, update the early-return
to setPhase("idle") and return immediately, and add a brief comment explaining
why we avoid entering "placement" without a targetCalendarItem or room.
| function RouteComponent() { | ||
| const { roomId } = Route.useParams(); | ||
| const { data: room } = useGetRoomsId(roomId); | ||
| console.log("room", room); |
There was a problem hiding this comment.
console.log消し忘れてるヨ〜 💦
おじさんもよくやっちゃうんダケド、デバッグ用の console.log が残ってるネ〜 (´ω｀)
本番環境に出る前に消しておいた方がいいヨ〜 🎵
🔎 console.logを削除する提案
const { roomId } = Route.useParams();
const { data: room } = useGetRoomsId(roomId);
- console.log("room", room);
const { data: calendarItems } =📝 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.
| console.log("room", room); | |
| const { roomId } = Route.useParams(); | |
| const { data: room } = useGetRoomsId(roomId); | |
| const { data: calendarItems } = |
🤖 Prompt for AI Agents
In frontend/src/routes/$roomId/index.lazy.tsx around line 52 there is a leftover
debug console.log("room", room); — remove this console.log (or replace it with
the app's proper logger if you need runtime diagnostics) and run the
linter/build to ensure no stray debug statements remain before merging.
1 participant
Summary by CodeRabbit
リリースノート
✏️ Tip: You can customize this high-level summary in your review settings.