Added support for enabling boundaries for assetbed cameras #8866
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying care-fe with
|
| Latest commit: |
178068d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1afe9c10.care-fe.pages.dev |
| Branch Preview URL: | https://boundary.care-fe.pages.dev |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
boundary
|
| Run status |
|
| Run duration | 03m 14s |
| Commit |
|
| Committer | Khavin Shankar |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
125
|
| View all changes introduced in this branch ↗︎ | |
|
👋 Hi, @khavinshankar, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
github-actions
bot
added
Deploy-Failed
|
@khavinshankar the required backend is still is draft and failing , so moving out of need testing, once its ready for review, change the label |
|
👋 Hi, @khavinshankar, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
Blocking this PR till the camera plug is merged. |
WalkthroughThe changes in this pull request enhance the asset and camera management functionalities. New types and properties are introduced in asset-related interfaces, including boundary management for camera control. The Changes
Assessment against linked issues
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
src/Components/CameraFeed/useOperateCamera.ts (2)
36-41: Consider enhancing type safety for asset_bed_idWhile the
asset_bed_idaddition supports the boundary feature, consider using a more specific type thanstringto ensure type safety.interface RelativeMoveOperation extends BaseOperation { type: "relative_move"; data: PTZPayload; options?: { - asset_bed_id?: string; + asset_bed_id?: `bed_${string}`; // or a specific union type of valid bed IDs }; }
Line range hint
65-91: Add input validation and improve documentationWhile the options handling is well-implemented, consider these improvements:
- Add validation for the options before sending to the API
- Update the JSDoc to document the new options parameter and its usage
/** * This hook is used to control the PTZ of a camera asset and retrieve other related information. * @param id The external id of the camera asset + * @param options Optional configuration for different operation types + * @param options.get_status Options for status operation + * @param options.relative_move Options including asset_bed_id for movement boundaries */ export default function useOperateCamera( id: string, options?: Partial<Record<OperationAction["type"], Record<string, unknown>>>, ) { const [key, setKey] = useState(0); + const validateOptions = ( + type: OperationAction["type"], + opts?: Record<string, unknown> + ) => { + if (!opts) return opts; + // Add validation logic here + return opts; + }; return { key, operate: (action: OperationAction) => { if (action.type === "reset") { setKey((prev) => prev + 1); return request(FeedRoutes.operateAsset, { pathParams: { id }, body: { action: { type: "get_status", - options: options?.get_status, + options: validateOptions("get_status", options?.get_status), }, }, silent: true, }); } return request(FeedRoutes.operateAsset, { pathParams: { id }, - body: { action: { ...action, options: options?.[action.type] } }, + body: { action: { ...action, options: validateOptions(action.type, options?.[action.type]) } }, silent: true, }); }, }; }src/Components/Assets/AssetTypes.tsx (3)
153-153: LGTM! Consider adding JSDoc documentation.The
BoundaryKeystype is well-defined for representing boundary coordinates. Consider adding JSDoc documentation to explain the purpose and usage of these coordinates.+/** Represents the coordinate keys for camera boundary definition + * x0, y0: Starting coordinates + * x1, y1: Ending coordinates + */ export type BoundaryKeys = "x0" | "y0" | "x1" | "y1";
Line range hint
153-183: Consider enhancing type safety and documentation.To improve the implementation:
- Add comprehensive JSDoc documentation for all new types and interfaces
- Consider creating a dedicated namespace or module for boundary-related types
- Add runtime type guards for boundary validation
- Consider extracting service edit tracking into a separate PR
Example structure:
export namespace CameraBoundary { /** Documentation for coordinate system */ export type Coordinates = number; /** Documentation for boundary keys */ export type BoundaryKeys = "x0" | "y0" | "x1" | "y1"; /** Documentation for boundary definition */ export type Definition = Record<BoundaryKeys, Coordinates> | null; /** Runtime type guard */ export const isValidBoundary = (boundary: Definition): boolean => { if (!boundary) return true; return Object.entries(boundary).every(([key, value]) => { // Add validation logic here return typeof value === 'number'; }); }; }
Service edit tracking changes appear unrelated to camera boundaries
Based on the codebase analysis:
- The
AssetServiceEditandAssetServiceinterfaces are part of a complete asset service management system- These interfaces are used in
AssetServiceEditModaland asset management components- Camera boundary related code is isolated to
src/Components/CameraFeed/ConfigureCamera.tsx- There's no apparent connection between the service tracking changes and camera boundary functionality
The service edit tracking changes should be moved to a separate PR to maintain focused, single-purpose changes.
🔗 Analysis chain
Line range hint
168-183: Verify if service edit tracking changes belong in this PR.The addition of
AssetServiceEditand updates toAssetServiceimplement proper audit trailing, but they seem unrelated to the PR's primary objective of implementing camera boundaries.Let's check if these changes are discussed in related issues:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related discussions about service edit tracking rg -l "service.*edit|edit.*service" | xargs -I {} gh issue list --search "file:{}"Length of output: 86
Script:
#!/bin/bash # Let's check the PR description and commits for context about these changes gh pr view --json title,body # Also check commit messages git log --oneline -n 10Length of output: 82
Script:
#!/bin/bash # Let's check if these interfaces are used in relation to camera boundaries rg -B2 -A2 "AssetServiceEdit|AssetService" # Also check for any related camera boundary code rg -l "camera.*boundary|boundary.*camera"Length of output: 7456
src/Components/CameraFeed/FeedControls.tsx (2)
239-239: Consider adding error boundary for additionalControlsThe
additionalControlsrender is not wrapped in an error boundary, which could cause the entire component to fail if the additional controls throw an error.Consider wrapping the additionalControls render in an error boundary:
+import { ErrorBoundary } from "react-error-boundary"; // In inline view -{additionalControls?.(props)} +<ErrorBoundary fallback={null}> + {additionalControls?.(props)} +</ErrorBoundary> // In standard view -{additionalControls?.(props)} +<ErrorBoundary fallback={null}> + {additionalControls?.(props)} +</ErrorBoundary>Also applies to: 254-254
46-55: Good architectural decision for extensibilityThe
additionalControlsprop provides a clean extension point for implementing the boundary-based camera controls without modifying the core control functionality. This approach:
- Maintains separation of concerns
- Makes the boundary controls optional and pluggable
- Allows for future extensions without component changes
src/Components/Facility/ConsultationDetails/ConsultationFeedTab.tsx (1)
55-57: Consider adding error handling and user feedback for boundary limits.To improve user experience, consider:
- Adding error handling for cases where preset or asset_bed is undefined
- Providing visual feedback when users hit boundary limits
- Implementing validation to ensure the camera stays within boundaries
Here's a suggested enhancement:
const { key, operate } = useOperateCamera(asset?.id ?? "", { relative_move: { asset_bed_id: preset?.asset_bed.id }, + onBoundaryHit: (direction) => { + Warn({ msg: t('camera_boundary_reached', { direction }) }); + }, + validate: (movement) => { + if (!preset?.asset_bed) { + console.warn('No asset bed configured for boundary validation'); + return true; + } + return validateMovementWithinBoundary(movement, preset.asset_bed); + } });src/Components/CameraFeed/CameraFeed.tsx (2)
139-151: Consider i18n support for error messages and DRY improvements.While the error handling for boundary violations is good, consider these improvements:
- The error message should support internationalization
- The 4000ms timeout appears in multiple places and could be extracted to a constant
+ const MOVEMENT_TIMEOUT_MS = 4000; + const { res, error } = await props.operate({ type: "relative_move", data, }); props.onMove?.(); setTimeout(() => { setState((state) => (state === "moving" ? undefined : state)); - }, 4000); + }, MOVEMENT_TIMEOUT_MS); if (res?.status === 400 && error) { Notification.Error({ - msg: error.detail, + msg: t('camera.boundary.violation', error.detail), }); }
139-151: Consider adding client-side boundary validation.While the current implementation handles boundary violations through backend validation, consider adding client-side validation to:
- Provide immediate feedback before making API calls
- Reduce server load
- Improve user experience with faster response times
This could be implemented by:
- Adding boundary validation logic in the onMove handler
- Showing visual indicators when approaching boundaries
- Disabling controls that would exceed boundaries
Would you like help implementing these improvements?
src/Locale/en.json (2)
359-359: Consider rephrasing the warning message for clarity.The warning message is quite long and could be made more concise while maintaining its meaning.
- "boundary_manual_edit_warning": "We strongly recommend NOT to set boundary values manually. Use the navigations within camera feed to set the boundary values.", + "boundary_manual_edit_warning": "Please use camera feed navigation controls to set boundary values instead of manual entry.",
730-730: Enhance validation message with specific range information.The validation message could be more helpful by explicitly stating the allowed range.
- "invalid_boundary_values": "Please ensure that all boundary values are set and are within the range of -1 to 1", + "invalid_boundary_values": "All boundary values must be set and must be between -1 and 1",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/Components/Assets/AssetTypes.tsx(2 hunks)src/Components/CameraFeed/CameraFeed.tsx(3 hunks)src/Components/CameraFeed/ConfigureCamera.tsx(9 hunks)src/Components/CameraFeed/FeedControls.tsx(2 hunks)src/Components/CameraFeed/useOperateCamera.ts(4 hunks)src/Components/Facility/ConsultationDetails/ConsultationFeedTab.tsx(1 hunks)src/Locale/en.json(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/Components/CameraFeed/ConfigureCamera.tsx
[error] 637-676: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (12)
src/Components/CameraFeed/useOperateCamera.ts (2)
11-48: Well-structured interface hierarchy!
The introduction of BaseOperation and extending it across all operation interfaces improves type consistency and maintainability while reducing code duplication.
65-68: Verify usage of the updated hook signature
The hook signature change might affect existing components. Let's verify all usages to ensure they're updated accordingly.
✅ Verification successful
Let me analyze the hook usage in more detail by checking the implementation and tests.
Hook signature change is safe and properly used
The verification shows that the hook's signature change is non-breaking and all usages are correct:
- All three usages of
useOperateCameraare properly handling the optionaloptionsparameter - Two components pass options (
ConsultationFeedTabandConfigureCamera) while one doesn't (CameraFeedWithBedPresets) - The implementation merges the options correctly with the operation actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of useOperateCamera
ast-grep --pattern 'useOperateCamera($$$)'
# Search for potential test files
fd -g '*useOperateCamera*.test.{ts,tsx}'
Length of output: 967
Script:
#!/bin/bash
# Check the implementation of useOperateCamera
cat src/Components/CameraFeed/useOperateCamera.ts
# Check test coverage
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs rg "useOperateCamera"
Length of output: 3481
src/Components/Assets/AssetTypes.tsx (1)
163-163: Please clarify the coordinate system and add validation constraints.
The boundary property type is well-defined, but consider:
- What is the coordinate system (pixels, percentages)?
- Are there any valid ranges for these values?
Consider adding:
- Type constraints for valid ranges
- Documentation about the coordinate system
- Validation logic in the backend
src/Components/CameraFeed/FeedControls.tsx (3)
46-55: Well-structured interface with good type safety!
The interface changes are well-designed:
- Making it exportable improves reusability
- Using
OmitforadditionalControlsprevents prop drilling - Optional prop maintains backward compatibility
58-62: LGTM! Clean component signature update.
The function signature changes correctly implement the interface updates while maintaining existing functionality.
245-254: Verify the impact of layout changes
The addition of h-full class and restructured layout might affect parent container sizing and existing layouts.
✅ Verification successful
Layout changes are safe to implement
The addition of h-full class to FeedControls is compatible with the parent CameraFeed component's layout structure. The parent already uses h-full class and has a flex column layout (flex h-full flex-col justify-center), which means the FeedControls' height will properly fit within its container. The restructured layout with flex and grid classes for control organization won't cause any layout conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parent components that might be affected by h-full class
rg -B 2 -A 2 "FeedControls" --type tsx
Length of output: 66
Script:
#!/bin/bash
# Search for parent components that might be affected by h-full class, using ripgrep without file type filter
rg -B 2 -A 2 "FeedControls"
Length of output: 2520
Script:
#!/bin/bash
# Find all TypeScript/React files that might contain FeedControls
fd -e tsx -e ts -e jsx -e js | xargs rg -B 2 -A 2 "FeedControls"
Length of output: 2557
Script:
#!/bin/bash
# Search for the component definition to understand its usage context
ast-grep --pattern 'const FeedControls = $_'
Length of output: 46
Script:
#!/bin/bash
# Search for the parent component's layout structure
rg -B 10 -A 10 "<FeedControls" src/Components/CameraFeed/CameraFeed.tsx
Length of output: 536
Script:
#!/bin/bash
# Look for any height-related classes in CameraFeed component
rg "h-\w+" src/Components/CameraFeed/CameraFeed.tsx
Length of output: 271
src/Components/Facility/ConsultationDetails/ConsultationFeedTab.tsx (2)
55-57: Verify integration with camera plug PR.
Since this PR depends on PR #2610 (camera plug), we should ensure proper integration between the two changes.
Let's check for potential integration points:
✅ Verification successful
Integration with camera movement functionality appears correct
The code shows proper integration with the camera movement system:
- The
useOperateCamerahook is consistently used across components for camera operations - The
relative_moveoperation is properly structured with the expectedasset_bed_idparameter - The implementation in
ConsultationFeedTab.tsxfollows the same pattern as other components using camera movement functionality
Since PR #2610 wasn't found in the codebase (likely merged already), and the current implementation follows the established patterns for camera movement across the codebase, the integration appears to be correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check integration between boundary implementation and camera plug
# Look for camera plug related code
rg -A 5 "camera.*plug|plug.*camera"
# Search for movement validation in camera plug
ast-grep --pattern 'class $CameraPlug {
$$$
validateMovement($$$) {
$$$
}
$$$
}'
Length of output: 132
Script:
#!/bin/bash
# Let's try a broader search for camera-related code and movement validation
# Search for camera-related hooks and utilities
rg -A 5 "useOperateCamera|operateCamera"
# Search for camera movement related code
rg -A 5 "relative_move|camera.*move"
# Look for camera-related types and interfaces
ast-grep --pattern 'interface $_ {
$$$
relative_move$$$
$$$
}'
# Search for camera plug PR reference
rg -n "#2610"
Length of output: 7962
55-57: Verify backend readiness for boundary implementation.
The hook modification looks good and aligns with the PR objectives for implementing camera boundaries. However, since the PR comments indicate that the backend is still in draft status, we should ensure proper coordination.
Let's verify the backend implementation:
✅ Verification successful
Based on the search results, I can see comprehensive boundary implementation in the codebase, particularly in ConfigureCamera.tsx. The code shows:
- Proper boundary validation logic with user feedback
- Complete boundary management system with UI controls
- Backend integration through
partialUpdateAssetBedAPI - Proper error handling for boundary violations
Let's verify the hook implementation:
Let's check one final aspect - the camera movement validation:
Backend integration for camera boundaries is properly implemented
The codebase shows a complete implementation of the camera boundary system:
- The
useOperateCamerahook inConsultationFeedTab.tsxcorrectly integrates with the boundary system by passingasset_bed_idin therelative_moveoptions. - The boundary validation logic is implemented in
ConfigureCamera.tsxwith proper error handling and user feedback. - The backend integration is handled through the
partialUpdateAssetBedAPI endpoint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for boundary-related endpoints and implementations
# Search for boundary-related API endpoints
rg -A 5 "boundary|boundaries" --type ts
# Look for camera movement validation logic
ast-grep --pattern 'function $_($$boundary$$) {
$$$
move($$$)
$$$
}'
Length of output: 20693
Script:
#!/bin/bash
# Check for useOperateCamera hook implementation
ast-grep --pattern 'function useOperateCamera($_) {
$$$
}'
# Search for relative_move implementation
rg -A 5 "relative_move.*asset_bed" --type ts
Length of output: 714
Script:
# Search for camera movement validation logic
rg -A 10 "useOperateCamera" --type ts
# Check for any camera movement validation files
fd camera.*validation -t f
Length of output: 10452
src/Components/CameraFeed/CameraFeed.tsx (3)
9-9: LGTM: Import additions are appropriate.
The new imports support the boundary management feature by providing type safety for controls and error notification capabilities.
Also applies to: 15-15
32-32: LGTM: Props interface update is well-typed.
The additionalControls prop is properly typed using FeedControlsProps, maintaining type safety while supporting the new boundary control feature.
156-156: Verify FeedControls component handles additionalControls properly.
The prop is correctly passed to FeedControls. Let's verify the receiving component's implementation.
✅ Verification successful
additionalControls prop is properly handled in FeedControls
The implementation is correct and follows React best practices:
- Properly typed as an optional function prop in FeedControlsProps interface
- Correctly destructured in component parameters
- Safely called with optional chaining
- Remaining props are passed correctly to the render function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check FeedControls implementation of additionalControls
# Expected: Find the prop usage in FeedControls component
ast-grep --pattern 'interface FeedControlsProps {
$$$
additionalControls?: $_
$$$
}'
ast-grep --pattern 'function FeedControls($_) {
$$$
additionalControls
$$$
}'
Length of output: 1016
Script:
#!/bin/bash
# Let's check how additionalControls is used in the FeedControls component
rg "additionalControls" src/Components/CameraFeed/FeedControls.tsx -A 5
Length of output: 560
src/Locale/en.json (1)
262-262: LGTM! Keys follow consistent naming patterns.
The new boundary-related keys follow the existing naming conventions and provide comprehensive coverage for the boundary feature:
- Action keys: add_boundary, set_as, unset
- Labels: boundary, boundary positions (x0, x1, y0, y1)
- Messages: warnings, errors, validations
Also applies to: 358-358, 359-359, 360-360, 366-366, 367-367, 368-368, 369-369, 730-730, 1115-1115, 1196-1196
| <> | ||
| <TextFormField | ||
| name={direction} | ||
| id={`boundary-textfield-${direction}`} | ||
| type="number" | ||
| min={ | ||
| ( | ||
| { | ||
| x1: boundary.x0 ?? -1, | ||
| y1: boundary.y0 ?? -1, | ||
| } as Record<BoundaryKeys, number> | ||
| )[direction] ?? -1 | ||
| } | ||
| max={ | ||
| ( | ||
| { | ||
| x0: boundary.x1 ?? 1, | ||
| y0: boundary.y1 ?? 1, | ||
| } as Record<BoundaryKeys, number> | ||
| )[direction] ?? 1 | ||
| } | ||
| label={ | ||
| <label> | ||
| {t( | ||
| "camera_preset__boundary_" + | ||
| direction, | ||
| )} | ||
| </label> | ||
| } | ||
| value={ | ||
| boundary[direction]?.toString() ?? "" | ||
| } | ||
| className="mt-1" | ||
| onChange={(e) => | ||
| setBoundary((prev) => ({ | ||
| ...prev, | ||
| [direction]: Number(e.value), | ||
| })) | ||
| } | ||
| errorClassName="hidden" | ||
| /> | ||
| </> | ||
| ), | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Add a unique key prop to elements in the list iteration
In the map function iterating over (["x0", "x1", "y0", "y1"] as BoundaryKeys[]), the returned React elements are wrapped in a Fragment (<>...</>) that lacks a key prop. This can cause issues with React's rendering and reconciliation. Please add a unique key prop to the outermost element in the iteration.
Apply this diff to fix the issue:
...
Alternatively, since there is only one child component, you can remove the Fragment and add the key prop directly to the TextFormField:
...
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 637-676: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Localization