fix(web): scene settings fov not applied #1223
Conversation
WalkthroughThe changes in this pull request focus on updating the logic for initializing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Published/hooks.ts (1)
51-61: Improve type safety and object initialization.While the implementation correctly addresses the FOV issue, consider these improvements:
- Add type checking for the camera object structure
- Avoid spreading empty objects when camera settings are null
Consider this safer implementation:
- const initialCamera = - sceneProperty?.camera?.camera || sceneProperty?.camera?.fov - ? { - ...(sceneProperty?.camera?.camera ?? {}), - ...(sceneProperty?.camera?.fov - ? { fov: sceneProperty.camera.fov } - : {}) - } - : undefined; + const initialCamera = (() => { + const cameraSettings = sceneProperty?.camera?.camera; + const fov = sceneProperty?.camera?.fov; + if (!cameraSettings && !fov) return undefined; + return { + ...(cameraSettings || {}), + ...(fov ? { fov } : {}) + } as Camera; + })();web/src/beta/features/Editor/Visualizer/hooks.ts (1)
101-109: LGTM! Consider simplifying the camera initialization logic.The implementation correctly addresses the FOV issue by including it in the camera settings. The use of optional chaining and spread operators ensures safe property access.
Consider simplifying the nested conditionals for better readability:
- const initialCamera = - sceneProperty?.camera?.camera || sceneProperty?.camera?.fov - ? { - ...(sceneProperty?.camera?.camera ?? {}), - ...(sceneProperty?.camera?.fov - ? { fov: sceneProperty.camera.fov } - : {}) - } - : undefined; + const initialCamera = sceneProperty?.camera && { + ...(sceneProperty.camera.camera ?? {}), + ...(sceneProperty.camera.fov && { fov: sceneProperty.camera.fov }) + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
web/src/beta/features/Editor/Visualizer/hooks.ts(1 hunks)web/src/beta/features/Published/hooks.ts(1 hunks)
🔇 Additional comments (2)
web/src/beta/features/Published/hooks.ts (1)
Line range hint 28-33: Verify camera state synchronization.
The current implementation uses separate state variables and an effect for camera synchronization. While this works, there could be a brief moment where currentCamera and initialCamera are out of sync during initialization.
Let's verify the camera state management implementation:
Also applies to: 77-79
✅ Verification successful
Based on the search results, I can now generate the final response:
Camera state management is implemented correctly
The search results show that the camera state synchronization pattern is consistently implemented across the codebase:
- The camera state updates are handled through a straightforward
useEffectdependency oninitialCamera - Similar patterns are used in other components (
PluginPlayground,Editor/Visualizer) - The state update is immediate and atomic, making race conditions unlikely in practice
The current implementation follows React's state management best practices and doesn't require modification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other camera state management patterns in the codebase
echo "Searching for camera state management patterns..."
rg -A 5 "const \[.*Camera.*useState.*\]"
# Search for potential race conditions in camera state updates
echo "Searching for camera state updates..."
rg -A 5 "setCurrentCamera"
Length of output: 4960
web/src/beta/features/Editor/Visualizer/hooks.ts (1)
110-111: Verify camera state initialization sequence.
The conditional setting of initialCamera followed by the useEffect that updates currentCamera could potentially lead to a race condition. While the current implementation works, it's worth verifying that the camera state is always properly initialized.
Let's check for any potential issues in the camera state initialization:
Also applies to: 337-339
Overview
Fov was not included in camera when set from scene properties.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes