Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds configurable amber and red work-time thresholds across lane editing, persistence, offline replay, and indicators; enables explicit orientation lists on several modals; and changes iOS microphone handling to check permission status without requesting it directly. ChangesLane work-time thresholds
Modal orientation support
iOS microphone permission handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AddLaneSheet
participant CommandStore
participant saveCommandNode
participant LandscapeStructureBoard
participant WorkTimeLight
AddLaneSheet->>CommandStore: submit threshold values
CommandStore->>saveCommandNode: persist WorkTimeAmberMinutes and WorkTimeRedMinutes
CommandStore-->>LandscapeStructureBoard: update lane node
LandscapeStructureBoard->>WorkTimeLight: pass lane-specific thresholds
WorkTimeLight-->>LandscapeStructureBoard: render threshold-based color
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/stores/app/__tests__/livekit-store.test.ts (1)
263-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
anycasts from the iOS permission fixtures.
mockGetRecordingPermissionsAsyncis alreadyjest.MockedFunction<typeof getRecordingPermissionsAsync>, so the fixture object is checked againstgetRecordingPermissionsAsync’s return type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/app/__tests__/livekit-store.test.ts` around lines 263 - 268, Remove the redundant any cast from the object passed to mockGetRecordingPermissionsAsync in the iOS permission fixtures, relying on the existing jest.MockedFunction typing to validate the fixture against getRecordingPermissionsAsync’s return type.Source: Coding guidelines
src/components/command/landscape-structure-board.tsx (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the WorkTimeLight props into an interface.
The updated component uses an inline object type instead of a named props interface. Define
WorkTimeLightPropsand useReact.FC<WorkTimeLightProps>.As per coding guidelines, use interfaces for props and type components as
React.FC<Props>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/command/landscape-structure-board.tsx` around lines 41 - 47, Define a named WorkTimeLightProps interface containing the existing WorkTimeLight properties, then update WorkTimeLight to use React.FC<WorkTimeLightProps> instead of the inline object type; preserve all optional fields and defaults.Source: Coding guidelines
src/stores/command/store.ts (1)
640-641: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd end-to-end regression coverage for threshold persistence and replay.
The new fields cross optimistic state, online saving, and offline replay, but the supplied tests only cover the form payload.
src/stores/command/store.ts#L640-L641: assert optimistic nodes retain amber/red values and disabled0values.src/stores/command/store.ts#L670-L671: assert the onlinesaveCommandNodepayload contains both values.src/services/offline-event-manager.service.ts#L388-L389: assert queued replay forwards both values correctly.As per coding guidelines, new logic and persistence paths should have tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/command/store.ts` around lines 640 - 641, Extend regression coverage for threshold persistence and replay: in src/stores/command/store.ts at lines 640-641, verify optimistic nodes retain amber/red values and disabled values as 0; at lines 670-671, verify the online saveCommandNode payload includes both fields; and in src/services/offline-event-manager.service.ts at lines 388-389, verify queued replay forwards both values correctly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/command/__tests__/add-lane-sheet.test.tsx`:
- Around line 57-67: Call the destructured unmount function from the render
result at the end of the test after the onSave assertion, ensuring the
AddLaneSheet render is cleaned up before subsequent tests.
In `@src/components/command/add-lane-sheet.tsx`:
- Around line 194-214: Add accessibilityLabel to the workTimeAmberMinutes and
workTimeRedMinutes InputField elements, using the corresponding translated
threshold labels from command.limit_worktime_amber and
command.limit_worktime_red. Preserve the existing values, handlers,
placeholders, and test IDs.
In `@src/components/command/lane-details-sheet.tsx`:
- Around line 58-60: Update the lane-details form state initialization and save
handling around workTimeAmber and workTimeRed so unset thresholds remain
distinguishable from an explicit 0. Seed empty fields with the shared
WorkTimeLight defaults (20 and 40 minutes) when the lane has no configured
values, and avoid parseInt(... ) || 0 converting unset values into disabled
indicators; preserve explicit 0 as disabled when saving.
- Around line 301-319: Add translated accessibilityLabel values to the amber and
red InputField elements in the worktime thresholds section, using the
corresponding threshold label translations so VoiceOver/TalkBack can distinguish
them after values are entered. Keep the existing input behavior and testIDs
unchanged.
In `@src/stores/app/livekit-store.ts`:
- Around line 407-414: Update the iOS microphone permission handling in
livekit-store.ts at lines 407-414 and 940-946 to treat !granted && !canAskAgain
as failure, guide the user to Settings, and stop before allowing WebRTC to
prompt; preserve the existing behavior for permissions that can still be
requested. Update the corresponding test in
src/stores/app/__tests__/livekit-store.test.ts lines 282-296 so permanently
denied access no longer returns true.
---
Nitpick comments:
In `@src/components/command/landscape-structure-board.tsx`:
- Around line 41-47: Define a named WorkTimeLightProps interface containing the
existing WorkTimeLight properties, then update WorkTimeLight to use
React.FC<WorkTimeLightProps> instead of the inline object type; preserve all
optional fields and defaults.
In `@src/stores/app/__tests__/livekit-store.test.ts`:
- Around line 263-268: Remove the redundant any cast from the object passed to
mockGetRecordingPermissionsAsync in the iOS permission fixtures, relying on the
existing jest.MockedFunction typing to validate the fixture against
getRecordingPermissionsAsync’s return type.
In `@src/stores/command/store.ts`:
- Around line 640-641: Extend regression coverage for threshold persistence and
replay: in src/stores/command/store.ts at lines 640-641, verify optimistic nodes
retain amber/red values and disabled values as 0; at lines 670-671, verify the
online saveCommandNode payload includes both fields; and in
src/services/offline-event-manager.service.ts at lines 388-389, verify queued
replay forwards both values correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86382e9c-321e-4dc0-b918-ffff500a72f9
📒 Files selected for processing (30)
src/components/call-video-feeds/video-player-modal.tsxsrc/components/calls/call-detail-menu.tsxsrc/components/calls/call-images-modal.tsxsrc/components/calls/call-notes-modal.tsxsrc/components/calls/close-call-bottom-sheet.tsxsrc/components/command/__tests__/add-lane-sheet.test.tsxsrc/components/command/__tests__/work-time-light.test.tsxsrc/components/command/add-lane-sheet.tsxsrc/components/command/command-details-sheet.tsxsrc/components/command/landscape-structure-board.tsxsrc/components/command/lane-details-sheet.tsxsrc/components/command/structure-section.tsxsrc/components/ui/bottom-sheet.tsxsrc/components/ui/side-drawer.tsxsrc/models/offline-queue/queued-event.tssrc/models/v4/incidentCommand/commandStructureNode.tssrc/models/v4/incidentCommand/incidentCommandModels.tssrc/services/offline-event-manager.service.tssrc/stores/app/__tests__/livekit-store.test.tssrc/stores/app/livekit-store.tssrc/stores/command/store.tssrc/translations/ar.jsonsrc/translations/de.jsonsrc/translations/en.jsonsrc/translations/es.jsonsrc/translations/fr.jsonsrc/translations/it.jsonsrc/translations/pl.jsonsrc/translations/sv.jsonsrc/translations/uk.json
| it('saves custom work-time thresholds and omits cleared ones', () => { | ||
| const onSave = jest.fn(); | ||
| const { getByTestId, unmount } = render(<AddLaneSheet isOpen={true} onClose={jest.fn()} onSave={onSave} />); | ||
|
|
||
| fireEvent.changeText(getByTestId('lane-name-input'), 'Rehab'); | ||
| fireEvent.press(getByTestId('lane-limits-toggle')); | ||
| fireEvent.changeText(getByTestId('limit-worktime-amber'), '15'); | ||
| fireEvent.changeText(getByTestId('limit-worktime-red'), ''); | ||
| fireEvent.press(getByTestId('lane-save')); | ||
|
|
||
| expect(onSave).toHaveBeenCalledWith('Rehab', CommandNodeType.Division, undefined, { workTimeAmberMinutes: 15 }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unmount the new render during cleanup.
The test destructures unmount on Line 59 but never calls it after the assertion, allowing the rendered tree to leak into later tests.
As per coding guidelines, tests must always call unmount() during cleanup.
Proposed fix
expect(onSave).toHaveBeenCalledWith('Rehab', CommandNodeType.Division, undefined, { workTimeAmberMinutes: 15 });
+ unmount();📝 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.
| it('saves custom work-time thresholds and omits cleared ones', () => { | |
| const onSave = jest.fn(); | |
| const { getByTestId, unmount } = render(<AddLaneSheet isOpen={true} onClose={jest.fn()} onSave={onSave} />); | |
| fireEvent.changeText(getByTestId('lane-name-input'), 'Rehab'); | |
| fireEvent.press(getByTestId('lane-limits-toggle')); | |
| fireEvent.changeText(getByTestId('limit-worktime-amber'), '15'); | |
| fireEvent.changeText(getByTestId('limit-worktime-red'), ''); | |
| fireEvent.press(getByTestId('lane-save')); | |
| expect(onSave).toHaveBeenCalledWith('Rehab', CommandNodeType.Division, undefined, { workTimeAmberMinutes: 15 }); | |
| it('saves custom work-time thresholds and omits cleared ones', () => { | |
| const onSave = jest.fn(); | |
| const { getByTestId, unmount } = render(<AddLaneSheet isOpen={true} onClose={jest.fn()} onSave={onSave} />); | |
| fireEvent.changeText(getByTestId('lane-name-input'), 'Rehab'); | |
| fireEvent.press(getByTestId('lane-limits-toggle')); | |
| fireEvent.changeText(getByTestId('limit-worktime-amber'), '15'); | |
| fireEvent.changeText(getByTestId('limit-worktime-red'), ''); | |
| fireEvent.press(getByTestId('lane-save')); | |
| expect(onSave).toHaveBeenCalledWith('Rehab', CommandNodeType.Division, undefined, { workTimeAmberMinutes: 15 }); | |
| unmount(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/command/__tests__/add-lane-sheet.test.tsx` around lines 57 -
67, Call the destructured unmount function from the render result at the end of
the test after the onSave assertion, ensuring the AddLaneSheet render is cleaned
up before subsequent tests.
Source: Coding guidelines
| <HStack space="sm"> | ||
| <VStack className="flex-1" space="xs"> | ||
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_amber')}</Text> | ||
| <Input size="sm" variant="outline"> | ||
| <InputField | ||
| keyboardType="number-pad" | ||
| placeholder={t('command.limit_none')} | ||
| value={limits.workTimeAmberMinutes} | ||
| onChangeText={(v) => setLimit('workTimeAmberMinutes', v)} | ||
| testID="limit-worktime-amber" | ||
| /> | ||
| </Input> | ||
| </VStack> | ||
| <VStack className="flex-1" space="xs"> | ||
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_red')}</Text> | ||
| <Input size="sm" variant="outline"> | ||
| <InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={limits.workTimeRedMinutes} onChangeText={(v) => setLimit('workTimeRedMinutes', v)} testID="limit-worktime-red" /> | ||
| </Input> | ||
| </VStack> | ||
| </HStack> | ||
| <Text className="text-xs text-gray-500 dark:text-gray-400">{t('command.limit_worktime_help')}</Text> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching add-lane-sheet:"
fd -a 'add-lane-sheet\.tsx$' . || true
if [ -f src/components/command/add-lane-sheet.tsx ]; then
echo
echo "Outline:"
ast-grep outline src/components/command/add-lane-sheet.tsx || true
echo
echo "Relevant lines 160-230:"
sed -n '160,230p' src/components/command/add-lane-sheet.tsx | nl -ba -v160
echo
echo "Imports/labels pattern:"
rg -n "InputField|accessibilityLabel|accessibilityLabelledBy|limitWorktimeAmber|workTimeAmberMinutes|limit_worktime" src/components/command/add-lane-sheet.tsx
fiRepository: Resgrid/IC
Length of output: 895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Imports and relevant input/limit sections:"
sed -n '1,45p' src/components/command/add-lane-sheet.tsx
printf '\n---\n'
sed -n '170,230p' src/components/command/add-lane-sheet.tsx
echo
echo "Search for gluestack accessibilityLabel APIs nearby:"
rg -n "InputField|accessibilityLabel|accessibilityLabelledBy|limit_worktime|workTimeAmberMinutes|workTimeRedMinutes" src/components src/components/command/add-lane-sheet.tsx || trueRepository: Resgrid/IC
Length of output: 25660
Add accessible labels to the work-time threshold inputs.
The nearby <Text> labels are not programmatic labels for the native text fields, so screen readers can expose both inputs only via identical placeholders. Add accessibilityLabel to both <InputField> elements using t('command.limit_worktime_amber') and t('command.limit_worktime_red').
[low_effort_and_highreward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/command/add-lane-sheet.tsx` around lines 194 - 214, Add
accessibilityLabel to the workTimeAmberMinutes and workTimeRedMinutes InputField
elements, using the corresponding translated threshold labels from
command.limit_worktime_amber and command.limit_worktime_red. Preserve the
existing values, handlers, placeholders, and test IDs.
Source: Coding guidelines
| <VStack space="xs"> | ||
| <Text className="text-sm font-medium text-gray-600 dark:text-gray-300">{t('command.worktime_thresholds_label')}</Text> | ||
| <HStack space="sm"> | ||
| <VStack className="flex-1" space="xs"> | ||
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_amber')}</Text> | ||
| <Input size="sm" variant="outline"> | ||
| <InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeAmber} onChangeText={(v) => setWorkTimeAmber(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-amber" /> | ||
| </Input> | ||
| </VStack> | ||
| <VStack className="flex-1" space="xs"> | ||
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_red')}</Text> | ||
| <Input size="sm" variant="outline"> | ||
| <InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeRed} onChangeText={(v) => setWorkTimeRed(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-red" /> | ||
| </Input> | ||
| </VStack> | ||
| </HStack> | ||
| <Text className="text-xs text-gray-500 dark:text-gray-400">{t('command.limit_worktime_help')}</Text> | ||
| </VStack> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add accessible labels to the threshold inputs.
The amber/red labels are separate Text elements, while each InputField only has the generic “none” placeholder. VoiceOver/TalkBack may therefore expose two indistinguishable fields, especially after values are entered. Add translated accessibilityLabel values for both inputs.
Proposed accessibility fix
-<InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeAmber} ...
+<InputField accessibilityLabel={t('command.limit_worktime_amber')} keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeAmber} ...
-<InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeRed} ...
+<InputField accessibilityLabel={t('command.limit_worktime_red')} keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeRed} ...As per coding guidelines, follow WCAG mobile accessibility practices, including accessible labels.
📝 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.
| <VStack space="xs"> | |
| <Text className="text-sm font-medium text-gray-600 dark:text-gray-300">{t('command.worktime_thresholds_label')}</Text> | |
| <HStack space="sm"> | |
| <VStack className="flex-1" space="xs"> | |
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_amber')}</Text> | |
| <Input size="sm" variant="outline"> | |
| <InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeAmber} onChangeText={(v) => setWorkTimeAmber(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-amber" /> | |
| </Input> | |
| </VStack> | |
| <VStack className="flex-1" space="xs"> | |
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_red')}</Text> | |
| <Input size="sm" variant="outline"> | |
| <InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeRed} onChangeText={(v) => setWorkTimeRed(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-red" /> | |
| </Input> | |
| </VStack> | |
| </HStack> | |
| <Text className="text-xs text-gray-500 dark:text-gray-400">{t('command.limit_worktime_help')}</Text> | |
| </VStack> | |
| <VStack space="xs"> | |
| <Text className="text-sm font-medium text-gray-600 dark:text-gray-300">{t('command.worktime_thresholds_label')}</Text> | |
| <HStack space="sm"> | |
| <VStack className="flex-1" space="xs"> | |
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_amber')}</Text> | |
| <Input size="sm" variant="outline"> | |
| <InputField accessibilityLabel={t('command.limit_worktime_amber')} keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeAmber} onChangeText={(v) => setWorkTimeAmber(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-amber" /> | |
| </Input> | |
| </VStack> | |
| <VStack className="flex-1" space="xs"> | |
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_red')}</Text> | |
| <Input size="sm" variant="outline"> | |
| <InputField accessibilityLabel={t('command.limit_worktime_red')} keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeRed} onChangeText={(v) => setWorkTimeRed(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-red" /> | |
| </Input> | |
| </VStack> | |
| </HStack> | |
| <Text className="text-xs text-gray-500 dark:text-gray-400">{t('command.limit_worktime_help')}</Text> | |
| </VStack> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/command/lane-details-sheet.tsx` around lines 301 - 319, Add
translated accessibilityLabel values to the amber and red InputField elements in
the worktime thresholds section, using the corresponding threshold label
translations so VoiceOver/TalkBack can distinguish them after values are
entered. Keep the existing input behavior and testIDs unchanged.
Source: Coding guidelines
| keyboardType="number-pad" | ||
| placeholder={t('command.limit_none')} | ||
| value={limits.workTimeAmberMinutes} | ||
| onChangeText={(v) => setLimit('workTimeAmberMinutes', v)} |
There was a problem hiding this comment.
Inline arrow functions in JSX props create new function instances on every render, impacting performance. Move these handlers outside the render method or memoize them.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/components/command/add-lane-sheet.tsx:
Line 202:
Inline arrow functions in JSX props create new function instances on every render, impacting performance. Move these handlers outside the render method or memoize them.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| minTimeInRole: '', | ||
| maxTimeInRole: '', | ||
| workTimeAmberMinutes: '20', | ||
| workTimeRedMinutes: '40', |
There was a problem hiding this comment.
Magic number '40' is duplicated as the work-time red threshold in both the useState initializer and reset logic. Extract a named constant such as DEFAULT_WORK_TIME_RED_MINUTES = 40 and reuse it in both locations to prevent drift.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/components/command/add-lane-sheet.tsx:
Line 67:
Magic number `'40'` is duplicated as the work-time red threshold in both the `useState` initializer and reset logic. Extract a named constant such as `DEFAULT_WORK_TIME_RED_MINUTES = 40` and reuse it in both locations to prevent drift.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| export const workTimeColor = (minutes: number) => (minutes < WORK_TIME_AMBER_MINUTES ? '#22c55e' : minutes < WORK_TIME_RED_MINUTES ? '#f59e0b' : '#ef4444'); | ||
| export const workTimeColor = (minutes: number, amberAfterMinutes: number = DEFAULT_WORK_TIME_AMBER_MINUTES, redAfterMinutes: number = DEFAULT_WORK_TIME_RED_MINUTES) => | ||
| redAfterMinutes > 0 && minutes >= redAfterMinutes ? '#ef4444' : amberAfterMinutes > 0 && minutes >= amberAfterMinutes ? '#f59e0b' : '#22c55e'; |
There was a problem hiding this comment.
Hex color literals '#ef4444', '#f59e0b', and '#22c55e' are inlined in multiple locations within the file, risking palette drift. Define them once as named constants such as WORK_TIME_RED, WORK_TIME_AMBER, and WORK_TIME_GREEN and reference those everywhere.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/components/command/landscape-structure-board.tsx:
Line 34:
Hex color literals `'#ef4444'`, `'#f59e0b'`, and `'#22c55e'` are inlined in multiple locations within the file, risking palette drift. Define them once as named constants such as `WORK_TIME_RED`, `WORK_TIME_AMBER`, and `WORK_TIME_GREEN` and reference those everywhere.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| WorkTimeAmberMinutes: parseInt(workTimeAmber, 10) || 0, | ||
| WorkTimeRedMinutes: parseInt(workTimeRed, 10) || 0, |
There was a problem hiding this comment.
LaneDetailsSheet persists 0 instead of undefined for blank work-time threshold fields, permanently disabling the default 20/40 fatigue indicator for any edited lane. The expression parseInt('', 10) || 0 produces 0, which fails the > 0 guard in workTimeColor and silently turns the indicator green. Omit the field from the patch when blank so undefined preserves default behavior.
WorkTimeAmberMinutes: workTimeAmber ? parseInt(workTimeAmber, 10) : undefined,
WorkTimeRedMinutes: workTimeRed ? parseInt(workTimeRed, 10) : undefined,
Prompt for LLM
File src/components/command/lane-details-sheet.tsx:
Line 96 to 97:
LaneDetailsSheet persists `0` instead of `undefined` for blank work-time threshold fields, permanently disabling the default 20/40 fatigue indicator for any edited lane. The expression `parseInt('', 10) || 0` produces `0`, which fails the `> 0` guard in `workTimeColor` and silently turns the indicator green. Omit the field from the patch when blank so `undefined` preserves default behavior.
Suggested Code:
WorkTimeAmberMinutes: workTimeAmber ? parseInt(workTimeAmber, 10) : undefined,
WorkTimeRedMinutes: workTimeRed ? parseInt(workTimeRed, 10) : undefined,
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <VStack className="flex-1" space="xs"> | ||
| <Text className="text-xs font-medium text-gray-600 dark:text-gray-300">{t('command.limit_worktime_amber')}</Text> | ||
| <Input size="sm" variant="outline"> | ||
| <InputField keyboardType="number-pad" placeholder={t('command.limit_none')} value={workTimeAmber} onChangeText={(v) => setWorkTimeAmber(v.replace(/[^0-9]/g, ''))} testID="lane-worktime-amber" /> |
There was a problem hiding this comment.
The digit-only sanitization v.replace(/[^0-9]/g, '') is duplicated across both work-time inputs. Extract a shared helper such as const sanitizeMinutes = (v: string) => v.replace(/[^0-9]/g, ''); and reuse it for the amber and red fields.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/components/command/lane-details-sheet.tsx:
Line 307:
The digit-only sanitization `v.replace(/[^0-9]/g, '')` is duplicated across both work-time inputs. Extract a shared helper such as `const sanitizeMinutes = (v: string) => v.replace(/[^0-9]/g, '');` and reuse it for the amber and red fields.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| // prompt itself when a track is published. | ||
| const micPermission = await getRecordingPermissionsAsync(); | ||
|
|
||
| if (!micPermission.granted && !micPermission.canAskAgain) { |
There was a problem hiding this comment.
Duplicated logic at lines 407-414 and 949-956 checks permanently-denied permissions, logs warnings, and returns early with only the variable name differing. Extract a helper such as handlePermanentlyDeniedPermission(mic, Platform.OS) to encapsulate the !granted && !canAskAgain check and return false from both sites.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/stores/app/livekit-store.ts:
Line 407:
Duplicated logic at lines 407-414 and 949-956 checks permanently-denied permissions, logs warnings, and returns early with only the variable name differing. Extract a helper such as `handlePermanentlyDeniedPermission(mic, Platform.OS)` to encapsulate the `!granted && !canAskAgain` check and return false from both sites.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
PR Description
This pull request includes fixes and enhancements across three main areas:
1. Per-Lane Work-Time (Crew Fatigue) Thresholds
Adds configurable amber and red work-time thresholds at the lane/division level in the command structure board. Previously, work-time indicators used fixed defaults (amber at 20 minutes, red at 40 minutes). Now, users can set custom thresholds per lane when creating or editing a lane. A value of
0disables that specific color indicator. If left unset, the defaults of 20/40 minutes apply. This includes updates to the data models, offline queue, command store, and both the add-lane and lane-details UIs.2. iOS Microphone Permission Deadlock Fix
Resolves an app freeze on iOS caused by
expo-audio'srequestRecordingPermissionsAsync, which activatesAVAudioSessionin record mode and deadlocks againstexpo-av's active session. The LiveKit store now only performs a non-activating permission check on iOS and allows WebRTC to prompt the user natively when a microphone track is published.3. Multi-Orientation Support for Modals
Adds
supportedOrientationsto all modal components (bottom sheets, side drawers, call modals, video player, command details, and location picker) to prevent iOS from forcing landscape iPads into portrait orientation when a modal opens.