Camera Preset Not Resetting on Re-selection Bug #9186
Conversation
WalkthroughThe changes introduce a new state variable, Changes
Assessment against linked issues
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
src/components/Facility/ConsultationDetails/ConsultationFeedTab.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc 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 (3)
src/components/Facility/ConsultationDetails/ConsultationFeedTab.tsx (3)
103-103: Remove debug console.log statementDebug logging should be removed before production deployment.
- console.log({ preset, presets });
Line range hint
188-197: Consider documenting the timeout usageThe 3-second delay in updating session storage after camera movement isn't immediately obvious. Consider adding a comment explaining why this delay is necessary.
setSelectedPreset(undefined); setHasMoved(true); + // Allow time for camera movement to complete before capturing final position setTimeout(async () => {
Line range hint
226-236: Fix analytics event parameterWhile the preset selection handling is correct, the analytics event uses the wrong preset name.
onChange={(value) => { triggerGoal("Camera Preset Clicked", { - presetName: preset?.name, + presetName: value.name, consultationId: props.consultationId, userId: authUser.id, result: "success", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/ConsultationDetails/ConsultationFeedTab.tsx(7 hunks)
🔇 Additional comments (5)
src/components/Facility/ConsultationDetails/ConsultationFeedTab.tsx (5)
37-37: LGTM: Good separation of concerns
The introduction of selectedPreset state properly separates the selection state from the active preset state, which is essential for handling preset re-selection correctly.
104-104: LGTM: Proper state initialization
The initialization of selectedPreset alongside preset ensures consistent initial state.
131-134: LGTM: Comprehensive preset update handling
The update logic properly maintains consistency between preset and selectedPreset states while handling all necessary cleanup operations.
186-186: LGTM: Correct preset position binding
Using selectedPreset?.position ensures the camera properly responds to preset re-selection, fixing the core issue.
37-37: Verify the complete preset selection workflow
The implementation looks correct, but please verify these scenarios:
- Selecting a preset moves camera to correct position
- Manual camera movement clears selected preset
- Re-selecting the same preset restores the original position
- Saving a new position updates the preset correctly
Also applies to: 186-186, 188-188, 226-226, 236-236
✅ Verification successful
Based on the search results, I can now verify the implementation. The code shows:
- Proper state management for preset selection:
const [selectedPreset, setSelectedPreset] = useState<CameraPreset>();- Clearing selection on manual movement:
setSelectedPreset(undefined);
setHasMoved(true);- Position update handling:
setSelectedPreset(updated);
setHasMoved(false);- Preset selection and restoration:
onChange={(value) => {
setSelectedPreset(value);
}}Implementation correctly handles all preset selection scenarios
The code properly implements:
- Preset selection state management
- Clearing selection on manual camera movement
- Position restoration on preset selection
- Position updates for existing presets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other camera position handling in the codebase
# Look for other camera position handling
rg -A 5 "position.*preset" --type typescript
# Check for other preset selection handling
ast-grep --pattern 'onChange($param) {
$$$
setSelectedPreset($$$)
$$$
}'
Length of output: 163
Script:
#!/bin/bash
# Search for camera position and preset handling with correct file extensions
rg -A 5 "position.*preset" -t ts -t tsx
# Look for preset selection state updates
rg -A 5 "setSelectedPreset" -t ts -t tsx
# Find camera movement handlers
rg -A 5 "(onCameraMove|handleCameraMove)" -t ts -t tsx
# Search for preset selection handlers
rg -A 5 "(onPresetSelect|handlePresetSelect)" -t ts -t tsx
Length of output: 312
Script:
#!/bin/bash
# Search for files first
fd "\.(ts|tsx)$"
# Then search with just extension
rg -A 5 "position.*preset" --type-add 'ts:*.{ts,tsx}'
# Look for preset selection state updates
rg -A 5 "setSelectedPreset" --type-add 'ts:*.{ts,tsx}'
# Find camera movement handlers
rg -A 5 "(onCameraMove|handleCameraMove)" --type-add 'ts:*.{ts,tsx}'
# Search for preset selection handlers
rg -A 5 "(onPresetSelect|handlePresetSelect)" --type-add 'ts:*.{ts,tsx}'
Length of output: 30801
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes