CU-868f1731u Bug fixes for status sheet. #152
Conversation
|
Task linked: CU-868f1731u Set Active and State on View Call Page |
WalkthroughRefactors StatusBottomSheet to use touchable card selections and checks, adds guarded submit with isSubmitting and toast feedback, moves active-call synchronization to submit, introduces noteType-driven step flow and complex preselection/display logic, and expands tests and i18n keys for submission and save outcomes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SBS as StatusBottomSheet
participant SVC as saveUnitStatus
participant CORE as Core State (Active Call)
participant TOAST as Toast Store
U->>SBS: Open bottom sheet
SBS->>SBS: Preselect destination/status if eligible
U->>SBS: Select status/destination/note
U->>SBS: Submit
SBS->>SBS: Set isSubmitting, disable navigation
SBS->>SVC: saveUnitStatus(payload)
alt Success
SVC-->>SBS: OK
SBS->>CORE: Update active call if changed
SBS->>TOAST: showToast(success)
else Failure
SVC-->>SBS: Error
SBS->>TOAST: showToast(error)
end
SBS->>SBS: Clear isSubmitting, re-enable actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)src/components/status/status-bottom-sheet.tsx (5)
🔇 Additional comments (24)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🔭 Outside diff range comments (1)
src/components/status/status-bottom-sheet.tsx (1)
482-486: Fix inverted note type display text.The display text for note types is inverted. When
Note === 1(optional), it shows "note_required", and whenNote === 2(required), it shows "note_optional".{status.Note > 0 && ( <Text className="text-xs text-gray-500 dark:text-gray-500"> - {status.Note === 1 && t('status.note_required')} - {status.Note === 2 && t('status.note_optional')} + {status.Note === 1 && t('status.note_optional')} + {status.Note === 2 && t('status.note_required')} </Text> )}
🧹 Nitpick comments (3)
test-note-types.js (1)
1-37: Consider moving this diagnostic script to a more appropriate location.This appears to be a diagnostic/demo script. A few suggestions:
- Move it to a
scripts/ordocs/directory to keep the root clean- Add a shebang line (
#!/usr/bin/env node) if it's meant to be executable- The implementation details on lines 33-34 don't match the actual code - in status-bottom-sheet.tsx,
isNoteRequiredisnoteType === 2andisNoteOptionalisnoteType === 1(reversed from what's shown here)+#!/usr/bin/env node // Simple test script to verify NoteType behavior // This demonstrates the three NoteType states based on user requirements const noteTypeTests = [ { name: "NoteType 0 - Don't show note step", noteType: 0, expected: 'No note step shown, submits directly', }, { name: 'NoteType 1 - Note is optional', noteType: 1, expected: "Note step shown with '(Optional)' indicator, can submit without note", }, { name: 'NoteType 2 - Note is required', noteType: 2, expected: "Note step shown without '(Optional)' indicator, submit disabled until note provided", }, ]; console.log('Note Type Behavior Mapping:'); console.log('='.repeat(50)); noteTypeTests.forEach((test) => { console.log(`${test.name}:`); console.log(` NoteType: ${test.noteType}`); console.log(` Behavior: ${test.expected}`); console.log(''); }); console.log('Implementation Details:'); -console.log('- isNoteRequired = noteType === 2'); -console.log('- isNoteOptional = noteType === 1'); +console.log('- isNoteRequired = noteType === 2 // NoteType 2 = required'); +console.log('- isNoteOptional = noteType === 1 // NoteType 1 = optional'); console.log('- Note step shown when noteType > 0'); console.log('- Submit validation: !isNoteRequired || note.trim().length > 0');src/components/status/status-bottom-sheet.tsx (2)
254-291: Consider consolidating the pre-selection logic to avoid potential race conditions.The two
useLayoutEffecthooks (lines 254-280 and 283-290) that handle pre-selection could potentially race with each other. Consider combining them into a single effect to ensure predictable behavior.- // Pre-select active call when opening with calls enabled - React.useLayoutEffect(() => { - // Reset the pre-selection flag when bottom sheet closes - if (!isOpen) { - hasPreselectedRef.current = false; - return; - } - - // Immediate pre-selection: if we have the conditions met, pre-select right away - // This runs on every render to catch the case where availableCalls loads in - if (isOpen && selectedStatus && (selectedStatus.Detail === 2 || selectedStatus.Detail === 3) && activeCallId && !selectedCall && selectedDestinationType === 'none' && !hasPreselectedRef.current) { - // Check if we have calls available (loaded) or should wait - if (!isLoading && availableCalls.length > 0) { - const activeCall = availableCalls.find((call) => call.CallId === activeCallId); - if (activeCall) { - // Update both states immediately in the same render cycle - setSelectedDestinationType('call'); - setSelectedCall(activeCall); - hasPreselectedRef.current = true; - } - } else if (isLoading || availableCalls.length === 0) { - // If still loading, immediately set destination type to 'call' to prevent "No Destination" from showing - // We'll set the actual call once it loads - setSelectedDestinationType('call'); - hasPreselectedRef.current = true; - } - } - }, [isOpen, isLoading, selectedStatus, activeCallId, availableCalls, selectedCall, selectedDestinationType, setSelectedCall, setSelectedDestinationType]); - - // Additional effect to set the actual call once it becomes available - React.useLayoutEffect(() => { - if (isOpen && selectedStatus && (selectedStatus.Detail === 2 || selectedStatus.Detail === 3) && activeCallId && !selectedCall && selectedDestinationType === 'call' && !isLoading && availableCalls.length > 0) { - const activeCall = availableCalls.find((call) => call.CallId === activeCallId); - if (activeCall) { - setSelectedCall(activeCall); - } - } - }, [isOpen, selectedStatus, activeCallId, selectedCall, selectedDestinationType, isLoading, availableCalls, setSelectedCall]); + // Pre-select active call when opening with calls enabled + React.useLayoutEffect(() => { + // Reset the pre-selection flag when bottom sheet closes + if (!isOpen) { + hasPreselectedRef.current = false; + return; + } + + // Check if we should pre-select + const shouldPreselect = isOpen && selectedStatus && + (selectedStatus.Detail === 2 || selectedStatus.Detail === 3) && + activeCallId && !selectedCall; + + if (!shouldPreselect) return; + + // Handle pre-selection based on loading state + if (selectedDestinationType === 'none' && !hasPreselectedRef.current) { + if (!isLoading && availableCalls.length > 0) { + const activeCall = availableCalls.find((call) => call.CallId === activeCallId); + if (activeCall) { + setSelectedDestinationType('call'); + setSelectedCall(activeCall); + hasPreselectedRef.current = true; + } + } else if (isLoading || availableCalls.length === 0) { + // Set destination type while loading to prevent "No Destination" from showing + setSelectedDestinationType('call'); + hasPreselectedRef.current = true; + } + } else if (selectedDestinationType === 'call' && !isLoading && availableCalls.length > 0) { + // Set the actual call once it becomes available + const activeCall = availableCalls.find((call) => call.CallId === activeCallId); + if (activeCall) { + setSelectedCall(activeCall); + } + } + }, [isOpen, isLoading, selectedStatus, activeCallId, availableCalls, selectedCall, selectedDestinationType, setSelectedCall, setSelectedDestinationType]);
25-676: Consider breaking down this component for better maintainability.The StatusBottomSheet component has grown to 676 lines with complex state management and multiple conditional renders. Consider extracting sub-components for better maintainability:
StatusSelectionStep- for status selection UI (lines 456-505)DestinationSelectionStep- for destination selection UI (lines 507-615)NoteInputStep- for note input UI (lines 635-671)StepIndicator- for step progress display (lines 445-450)This would make the code more modular and easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/status/__tests__/status-bottom-sheet.test.tsx(24 hunks)src/components/status/status-bottom-sheet.tsx(17 hunks)src/translations/ar.json(3 hunks)src/translations/en.json(3 hunks)src/translations/es.json(3 hunks)test-note-types.js(1 hunks)
🔇 Additional comments (13)
src/translations/en.json (1)
330-330: LGTM!The new translation keys are properly formatted and follow consistent naming conventions. The English text is grammatically correct and aligns with the UI messaging patterns used throughout the application.
Also applies to: 593-593, 607-607, 610-611
src/translations/ar.json (1)
330-330: LGTM!The Arabic translations are properly formatted and appear to be accurate translations of the English text. The keys match across all locale files.
Also applies to: 593-593, 607-607, 610-611
src/translations/es.json (1)
330-330: LGTM!The Spanish translations are properly formatted and accurately translated. The keys are consistent across all locale files.
Also applies to: 593-593, 607-607, 610-611
src/components/status/__tests__/status-bottom-sheet.test.tsx (10)
8-8: LGTM! Toast store mock setup follows established patterns.The toast store import and mock configuration is correctly implemented and consistent with the existing mock patterns in the test file.
Also applies to: 35-37, 122-122, 133-133, 204-204
77-77: Translation keys properly added for enhanced user feedback.The new translation keys for submission state and save outcomes are well-named and follow the existing conventions.
Also applies to: 90-90, 99-100
339-380: Well-structured test validating deferred active call assignment.Good test coverage ensuring that active call is not prematurely set during selection, maintaining proper state management flow.
679-869: Excellent comprehensive test coverage for active call management during submission.These tests thoroughly validate all scenarios for active call assignment at submission time, including edge cases and preventing unnecessary updates.
871-947: Excellent end-to-end test for multi-step active call management.This integration test effectively validates that active call assignment is deferred until the final submission, ensuring proper state management through the entire user flow.
1251-1586: Thorough test coverage for active call pre-selection logic.Excellent suite of tests covering all pre-selection scenarios with proper async handling and edge case validation.
1588-1717: Good visual state validation for pre-selection behavior.These tests ensure proper visual feedback during pre-selection, preventing confusing UI states where multiple options appear selected.
1738-1777: Tests properly validate the UI redesign to touchable cards.Good coverage of the new TouchableOpacity-based UI with checkmark indicators for both status and destination selection.
2237-2409: Comprehensive test coverage for submission flow and user feedback.Excellent tests validating submission state management, toast notifications, and protection against double submission. The async handling is properly implemented.
2410-2518: Good edge case coverage for destination display logic.These tests ensure robust handling of transitional states and incomplete data scenarios in the destination display.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/status/status-bottom-sheet.tsx(17 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/status/status-bottom-sheet.tsx (5)
src/api/units/unitStatuses.ts (1)
saveUnitStatus(23-32)src/services/aptabase.service.ts (1)
reset(146-150)src/lib/logging/index.tsx (1)
error(74-76)src/components/ui/index.tsx (1)
TouchableOpacity(18-18)src/components/ui/button/index.tsx (2)
Button(334-334)ButtonText(334-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (22)
src/components/status/status-bottom-sheet.tsx (22)
1-1: LGTM!Good addition of the Check icon import to replace the previous radio-based selection UI with card-like selection items.
14-14: LGTM!Appropriate addition of toast store for user feedback on submit operations.
29-31: LGTM!Good addition of submission state management and preselection tracking to prevent double submissions and control preselection behavior.
125-133: LGTM!Consistent application of the noteType logic for flow control. The implementation correctly handles the three states: no note (0), optional note (1), and required note (2).
158-165: LGTM!Excellent implementation of double-submission prevention with early return and proper state management.
203-207: LGTM!Good implementation that only updates the active call when necessary, avoiding unnecessary state changes when the selected call is already active.
210-220: LGTM!Excellent user feedback implementation with success and error toasts, plus proper cleanup in the finally block.
294-309: LGTM!Well-implemented memoized logic for determining when to show "No Destination" as selected. The logic correctly handles edge cases and prevents premature selection.
314-316: LGTM!Consistent usage of the noteType semantics throughout the component. The variable naming clearly indicates the purpose.
358-359: LGTM!Correct application of the noteType logic in the total steps calculation, maintaining consistency with the rest of the component.
393-394: LGTM!Good guard to prevent navigation while submitting, maintaining UI consistency and preventing potential race conditions.
408-434: LGTM!Excellent robust destination display logic that handles multiple scenarios including loading states, fallbacks, and edge cases. The prioritization logic (selectedCall > selectedStation > fallback lookup) is well thought out.
461-495: LGTM!Excellent refactor from radio buttons to card-based selection with TouchableOpacity and Check icons. The visual feedback and accessibility are improved with the new design.
484-486: LGTM!Consistent display of noteType values with proper user-friendly labels. The conditional rendering correctly maps the backend values to UI text.
514-517: LGTM!Good use of the shouldShowNoDestinationAsSelected logic to prevent visual inconsistencies during preselection scenarios.
544-571: LGTM!Excellent implementation of card-based selection for calls with proper loading states, visual feedback, and consistent styling.
576-602: LGTM!Consistent implementation of card-based selection for stations, maintaining the same pattern as calls with proper visual feedback.
628-631: LGTM!Good implementation of loading state feedback during submission with spinner and disabled state management.
638-644: LGTM!Good addition of selected status display in the note step, providing better context for the user about their current selection.
661-668: LGTM!Consistent implementation of loading state feedback and proper disabled state management for both Previous and Submit buttons.
254-281: Confirm Complex Preselection Logic StabilityI didn’t find any other
hasPreselectedRef–driven effects or similar multi-state layout effects elsewhere in the repo, so this is the only place with this degree of conditional branching. Because it updates two pieces of state in one layout pass and relies on several flags toggling in quick succession, it remains prone to race conditions or intermittent “no destination” flashes whenisOpenor loading status changes rapidly.Please review and consider:
- src/components/status/status-bottom-sheet.tsx (lines 254–281)
• Can this effect be split into smaller hooks or utility functions to isolate concerns?
• Verify thathasPreselectedRefreliably prevents duplicate pre-selection across renders.
• Add targeted unit or integration tests covering open/close and loading transitions.
114-122: Confirm noteType semantics: 0 = no note, 1 = optional note, 2 = required noteAll evidence aligns with this mapping:
- src/models/v4/statuses/statusesResultData.ts declares
public Note: number = 0; // Note Type- CustomStatusResultData (used by getStatusProperty) likewise defaults
Note: number = 0- Tests in src/components/status/tests/status-bottom-sheet.test.tsx assert that
•Note: 1renders “Note optional”
•Note: 2renders “Note required”- Code in status-bottom-sheet.tsx uses
•noteType === 2to enforce a required note
•noteType > 0to show an add-note stepNo inconsistencies found—no changes required.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores