Conversation
📝 WalkthroughWalkthroughThe pull request adds incident command models, API/state handling, a command tab, destination POI routing and map selection, notification tap deduplication, localized strings, and related weather-alert and layout adjustments with corresponding tests. ChangesIncident command
Destination routing
Notification tap deduplication
Related API and UI adjustments
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CallDetail
participant IncidentCommandTabPanel
participant useIncidentCommandStore
participant getResourceIncidentView
CallDetail->>IncidentCommandTabPanel: render command tab with callId
IncidentCommandTabPanel->>useIncidentCommandStore: fetch incident view
useIncidentCommandStore->>getResourceIncidentView: request resource incident view
getResourceIncidentView-->>useIncidentCommandStore: return typed result
useIncidentCommandStore-->>IncidentCommandTabPanel: provide view or state
sequenceDiagram
participant CallDetail
participant SidebarCallCard
participant Maps
CallDetail->>Maps: open destination directions
SidebarCallCard->>Maps: open coordinate directions
SidebarCallCard->>Maps: open address directions when coordinates are absent
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/call/[id].tsx (1)
515-574: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win"Call Location" map toggle is a dead control when the call has no valid call/dispatch coordinates.
showingDestination = hasDestinationCoordinates && (mapTarget === 'destination' || !hasCallCoordinates)forcesshowingDestinationtotruewheneverhasCallCoordinatesisfalse, regardless ofmapTarget. But the toggle button pair (lines 563-574) is rendered based solely onhasDestinationCoordinates, so when a call has a valid destination POI but no valid dispatch coordinates, the "Call Location" button is shown, yet pressing it can never flip the map or its ownvariantstyling — a visible no-op control.🐛 Suggested fix: only show the toggle when there's an actual choice
- {hasDestinationCoordinates ? ( + {hasDestinationCoordinates && hasCallCoordinates ? ( <HStack className="w-full"> <Button onPress={() => setMapTarget('call')} variant={showingDestination ? 'outline' : 'solid'} size="sm" className="flex-1 rounded-none" testID="call-detail-map-toggle-call">🤖 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/app/call/`[id].tsx around lines 515 - 574, Only render the map toggle buttons when both valid destination coordinates and valid call coordinates exist, so the controls appear only when switching locations is possible. Update the condition around the toggle pair near StaticMap, while preserving destination-only map rendering and existing toggle behavior when both coordinate sets are available.
🧹 Nitpick comments (8)
src/components/sidebar/__tests__/call-sidebar.test.tsx (1)
128-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new explicit
anytypes from test mocks.
src/components/sidebar/__tests__/call-sidebar.test.tsx#L128-L131: remove the unusedpropsparameter fromNavigation.src/components/sidebar/__tests__/call-sidebar.test.tsx#L569-L577: infer or explicitly type the Zustand selector.src/components/sidebar/__tests__/call-sidebar.test.tsx#L585-L593: infer or explicitly type the Zustand selector.src/components/sidebar/__tests__/call-sidebar.test.tsx#L601-L609: infer or explicitly type the Zustand selector.src/components/sidebar/__tests__/call-sidebar.test.tsx#L629-L637: infer or explicitly type the Zustand selector.🤖 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/sidebar/__tests__/call-sidebar.test.tsx` around lines 128 - 131, Remove the unused props parameter from the Navigation test mock in src/components/sidebar/__tests__/call-sidebar.test.tsx:128-131. At src/components/sidebar/__tests__/call-sidebar.test.tsx:569-577, 585-593, 601-609, and 629-637, remove explicit any usage from the Zustand selector mocks by allowing inference or applying the appropriate concrete selector type, while preserving their existing behavior.Source: Coding guidelines
src/models/v4/incidentCommand/resourceIncidentView.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lowercase-hyphenated paths for the new incident-command modules.
src/models/v4/incidentCommand/resourceIncidentView.ts#L1-L1: rename the directory toincident-commandand the file toresource-incident-view.ts.src/models/v4/incidentCommand/resourceIncidentViewResult.ts#L1-L1: rename the file toresource-incident-view-result.tsin the same renamed directory.src/api/calls/incidentCommand.ts#L1-L1: rename the file toincident-command.ts.- Update all affected alias imports consistently.
As per coding guidelines, “Directory and file names should be lowercase and hyphenated.”
🤖 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/models/v4/incidentCommand/resourceIncidentView.ts` at line 1, Rename the incident-command modules to lowercase-hyphenated paths: change src/models/v4/incidentCommand/resourceIncidentView.ts to src/models/v4/incident-command/resource-incident-view.ts, resourceIncidentViewResult.ts to resource-incident-view-result.ts in the same directory, and src/api/calls/incidentCommand.ts to incident-command.ts. Update every affected alias import and reference consistently.Source: Coding guidelines
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the declared enum types for enum-backed fields.
numberaccepts invalid status/category values despite the explicit wire enums above. Type these asTacticalObjectiveType,TacticalObjectiveStatus,IncidentNeedCategory, andIncidentNeedStatus, retaining their corresponding zero-value defaults.As per coding guidelines, “Never use
anytype; use precise types and interfaces with TypeScript strict mode enabled.”Also applies to: 65-66
🤖 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/models/v4/incidentCommand/resourceIncidentView.ts` around lines 45 - 46, Update the enum-backed fields in the relevant resource view classes to use their declared types: TacticalObjectiveType and TacticalObjectiveStatus for the objective fields, and IncidentNeedCategory and IncidentNeedStatus for the incident-need fields. Preserve each field’s existing zero-value default while replacing the broad number annotations with these precise enums.Source: Coding guidelines
src/app/call/__tests__/[id].security.test.tsx (1)
322-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the icon mock props.
anybypasses the lucide icon prop contract. Use the library’s exported icon-props type, or a narrow local interface containing only the props this mock supports.As per coding guidelines, “Never use
anytype; use precise types and interfaces with TypeScript strict mode enabled.”🤖 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/app/call/__tests__/`[id].security.test.tsx at line 322, Replace the any annotation on the ClipboardListIcon mock with the library’s exported icon-props type, or a narrow interface covering size and the forwarded div props. Preserve the existing size-based style and data-testid behavior while ensuring the mock conforms to TypeScript strict typing.Source: Coding guidelines
src/stores/calls/__tests__/incident-command-store.test.ts (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep store and API fixtures type-safe.
These
anycasts allow mocked core state and API payloads to drift from their real contracts. Define typed fixture factories for the core-store state andResourceIncidentViewResult, then reuse them for each scenario.As per coding guidelines, “Never use
anytype; use precise types and interfaces with TypeScript strict mode enabled.”Also applies to: 67-76, 100-104, 133-136, 178-181, 200-201
🤖 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/calls/__tests__/incident-command-store.test.ts` at line 5, Replace the any-based fixtures and mock selectors in the incident command store tests with typed factory helpers for core-store state and ResourceIncidentViewResult. Reuse those factories across the scenarios identified in the comment, preserving each test’s overrides while ensuring mocked state, API payloads, and the select callback conform to their real contracts.Source: Coding guidelines
src/stores/calls/incident-command-store.ts (1)
43-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog failed incident-command fetches.
The error is reduced to local UI state but never reported through the shared logger, so request failures are absent from Sentry/operational diagnostics.
As per coding guidelines, “All async operations must have proper try/catch with logging.”
🤖 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/calls/incident-command-store.ts` around lines 43 - 48, Update the catch block in the incident-command fetch method to log the caught error through the shared logger before updating state. Preserve the existing view, error-message, and isLoading assignments while ensuring failed requests are reported with useful error details.Source: Coding guidelines
src/app/call/__tests__/[id].test.tsx (1)
1313-1324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the mocked call-detail store.
call: unknownandselector: anylet this helper bypass the store selector contract. Define a narrow typed fixture/state interface so routing tests fail at compile time when the call-detail API changes.As per coding guidelines, “Never use
anytype; use precise types and interfaces with TypeScript strict mode enabled.”🤖 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/app/call/__tests__/`[id].test.tsx around lines 1313 - 1324, Update mockDetailStore to use the call-detail store’s precise state/interface type instead of an untyped object, and type its selector parameter against that store contract rather than any. Replace call: unknown with the appropriate call fixture type while preserving the existing selector-or-store behavior, so API changes are caught by TypeScript.Source: Coding guidelines
src/components/incident-command/__tests__/incident-command-tab-panel.test.tsx (1)
359-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a failure-path test for
openContactUrl.Current tests only cover the happy path (
openURLresolving). Add a case whereLinking.openURLrejects to confirmlogger.erroris invoked and the app doesn't crash — this also gives a regression hook for the PII-in-context concern raised in the source file.🤖 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/incident-command/__tests__/incident-command-tab-panel.test.tsx` around lines 359 - 383, Add a failure-path test alongside the existing dialer and mail tests for openContactUrl: mock Linking.openURL to reject, trigger a contact action through IncidentCommandTabPanel, and assert logger.error is called while the interaction does not throw. Reuse the existing test setup and restore mocks and unmount the rendered panel afterward.
🤖 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/incident-command/incident-command-tab-panel.tsx`:
- Around line 81-90: The openContactUrl error logging currently exposes the full
contact URL. Update the logger.error context in openContactUrl to omit url or
include only its scheme, while preserving the existing error logging and failure
handling.
In `@src/components/sidebar/call-sidebar.tsx`:
- Around line 129-140: Update the map-launch handling around
openMapsWithDirections and openMapsWithAddress to inspect each awaited boolean
result and call showLocationAlert() when it is false, while retaining
catch-based alerts for thrown errors. Add regression coverage for both the
coordinate and address branches, verifying alerts occur when the helpers return
false.
In `@src/services/__tests__/push-notification.test.ts`:
- Around line 430-438: Replace the any type on pushNotificationService in the
test setup with the exported type from ../push-notification, while preserving
the dynamically reloaded singleton assignment in beforeEach. Ensure the
declaration remains type-checked against the module’s pushNotificationService
export.
In `@src/stores/calls/incident-command-store.ts`:
- Around line 19-50: Update the incident-command store’s fetchIncidentView and
reset flow to track a request sequence or current call ID. Invalidate the active
request in reset(), and before applying success or error state from
fetchIncidentView, verify that the request is still the latest; ignore
superseded or reset-invalidated results so they cannot overwrite the current
view.
---
Outside diff comments:
In `@src/app/call/`[id].tsx:
- Around line 515-574: Only render the map toggle buttons when both valid
destination coordinates and valid call coordinates exist, so the controls appear
only when switching locations is possible. Update the condition around the
toggle pair near StaticMap, while preserving destination-only map rendering and
existing toggle behavior when both coordinate sets are available.
---
Nitpick comments:
In `@src/app/call/__tests__/`[id].security.test.tsx:
- Line 322: Replace the any annotation on the ClipboardListIcon mock with the
library’s exported icon-props type, or a narrow interface covering size and the
forwarded div props. Preserve the existing size-based style and data-testid
behavior while ensuring the mock conforms to TypeScript strict typing.
In `@src/app/call/__tests__/`[id].test.tsx:
- Around line 1313-1324: Update mockDetailStore to use the call-detail store’s
precise state/interface type instead of an untyped object, and type its selector
parameter against that store contract rather than any. Replace call: unknown
with the appropriate call fixture type while preserving the existing
selector-or-store behavior, so API changes are caught by TypeScript.
In
`@src/components/incident-command/__tests__/incident-command-tab-panel.test.tsx`:
- Around line 359-383: Add a failure-path test alongside the existing dialer and
mail tests for openContactUrl: mock Linking.openURL to reject, trigger a contact
action through IncidentCommandTabPanel, and assert logger.error is called while
the interaction does not throw. Reuse the existing test setup and restore mocks
and unmount the rendered panel afterward.
In `@src/components/sidebar/__tests__/call-sidebar.test.tsx`:
- Around line 128-131: Remove the unused props parameter from the Navigation
test mock in src/components/sidebar/__tests__/call-sidebar.test.tsx:128-131. At
src/components/sidebar/__tests__/call-sidebar.test.tsx:569-577, 585-593,
601-609, and 629-637, remove explicit any usage from the Zustand selector mocks
by allowing inference or applying the appropriate concrete selector type, while
preserving their existing behavior.
In `@src/models/v4/incidentCommand/resourceIncidentView.ts`:
- Line 1: Rename the incident-command modules to lowercase-hyphenated paths:
change src/models/v4/incidentCommand/resourceIncidentView.ts to
src/models/v4/incident-command/resource-incident-view.ts,
resourceIncidentViewResult.ts to resource-incident-view-result.ts in the same
directory, and src/api/calls/incidentCommand.ts to incident-command.ts. Update
every affected alias import and reference consistently.
- Around line 45-46: Update the enum-backed fields in the relevant resource view
classes to use their declared types: TacticalObjectiveType and
TacticalObjectiveStatus for the objective fields, and IncidentNeedCategory and
IncidentNeedStatus for the incident-need fields. Preserve each field’s existing
zero-value default while replacing the broad number annotations with these
precise enums.
In `@src/stores/calls/__tests__/incident-command-store.test.ts`:
- Line 5: Replace the any-based fixtures and mock selectors in the incident
command store tests with typed factory helpers for core-store state and
ResourceIncidentViewResult. Reuse those factories across the scenarios
identified in the comment, preserving each test’s overrides while ensuring
mocked state, API payloads, and the select callback conform to their real
contracts.
In `@src/stores/calls/incident-command-store.ts`:
- Around line 43-48: Update the catch block in the incident-command fetch method
to log the caught error through the shared logger before updating state.
Preserve the existing view, error-message, and isLoading assignments while
ensuring failed requests are reported with useful error details.
🪄 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
Run ID: aa1198a9-c506-4805-84d2-0bebf48f1595
📒 Files selected for processing (27)
src/api/calls/incidentCommand.tssrc/api/weather-alerts/weather-alerts.tssrc/app/(app)/_layout.tsxsrc/app/call/[id].tsxsrc/app/call/__tests__/[id].security.test.tsxsrc/app/call/__tests__/[id].test.tsxsrc/components/incident-command/__tests__/incident-command-tab-panel.test.tsxsrc/components/incident-command/incident-command-tab-panel.tsxsrc/components/sidebar/__tests__/call-sidebar.test.tsxsrc/components/sidebar/call-sidebar.tsxsrc/components/weather-alerts/severity-filter-tabs.tsxsrc/models/v4/incidentCommand/resourceIncidentView.tssrc/models/v4/incidentCommand/resourceIncidentViewResult.tssrc/services/__tests__/push-notification.test.tssrc/services/push-notification.tssrc/stores/calls/__tests__/incident-command-store.test.tssrc/stores/calls/incident-command-store.tssrc/stores/weather-alerts/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
| let pushNotificationService: any; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| jest.resetModules(); | ||
|
|
||
| jest.unmock('../push-notification'); | ||
| const module = require('../push-notification'); | ||
| pushNotificationService = module.pushNotificationService; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/services/__tests__/push-notification.test.ts'
# Show the relevant region with line numbers.
sed -n '410,470p' "$file" | cat -n
# Find the module's exported shape for typing.
rg -n "export (const|function|class|let|type|interface)|module\.exports|export default" src/services/push-notification.ts src/services/__tests__/push-notification.test.tsRepository: Resgrid/Unit
Length of output: 2911
Remove the any from the reloaded singleton.
Type pushNotificationService from ../push-notification so the test stays type-checked.
Proposed fix
- let pushNotificationService: any;
+ let pushNotificationService: typeof import('../push-notification').pushNotificationService;
...
- const module = require('../push-notification');
- pushNotificationService = module.pushNotificationService;
+ const reloadedModule = require('../push-notification') as typeof import('../push-notification');
+ pushNotificationService = reloadedModule.pushNotificationService;📝 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.
| let pushNotificationService: any; | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| jest.resetModules(); | |
| jest.unmock('../push-notification'); | |
| const module = require('../push-notification'); | |
| pushNotificationService = module.pushNotificationService; | |
| let pushNotificationService: typeof import('../push-notification').pushNotificationService; | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| jest.resetModules(); | |
| jest.unmock('../push-notification'); | |
| const reloadedModule = require('../push-notification') as typeof import('../push-notification'); | |
| pushNotificationService = reloadedModule.pushNotificationService; |
🤖 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/services/__tests__/push-notification.test.ts` around lines 430 - 438,
Replace the any type on pushNotificationService in the test setup with the
exported type from ../push-notification, while preserving the dynamically
reloaded singleton assignment in beforeEach. Ensure the declaration remains
type-checked against the module’s pushNotificationService export.
Source: Coding guidelines
This comment has been minimized.
This comment has been minimized.
| export const getResourceIncidentView = async (callId: string | number, unitId?: string | number | null) => { | ||
| const getResourceIncidentViewApi = createApiEndpoint(`/IncidentCommand/GetResourceIncidentView/${encodeURIComponent(String(callId))}`); | ||
| const params = unitId !== undefined && unitId !== null && unitId !== '' ? { unitId } : undefined; | ||
| const response = await getResourceIncidentViewApi.get<ResourceIncidentViewResult>(params); |
There was a problem hiding this comment.
Unhandled rejection in getResourceIncidentView — the awaited getResourceIncidentViewApi.get call has no try/catch, violating Rule 1 which requires every awaited async operation to be guarded. Wrap the await in a try/catch, log the failure with context (callId, err), and rethrow.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/api/calls/incidentCommand.ts:
Line 16:
Unhandled rejection in `getResourceIncidentView` — the awaited `getResourceIncidentViewApi.get` call has no try/catch, violating Rule 1 which requires every awaited async operation to be guarded. Wrap the await in a try/catch, log the failure with context (`callId`, `err`), and rethrow.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| export const getResourceIncidentView = async (callId: string | number, unitId?: string | number | null) => { | ||
| const getResourceIncidentViewApi = createApiEndpoint(`/IncidentCommand/GetResourceIncidentView/${encodeURIComponent(String(callId))}`); | ||
| const params = unitId !== undefined && unitId !== null && unitId !== '' ? { unitId } : undefined; | ||
| const response = await getResourceIncidentViewApi.get<ResourceIncidentViewResult>(params); |
There was a problem hiding this comment.
Missing error handling around the network call — Rule 30 requires external/network calls to be wrapped in try/catch with context added and mapped to application-level errors. Wrap the GET in try/catch, log the operation name and identifiers (callId, unitId), and rethrow or map to a domain error such as ApiError.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File src/api/calls/incidentCommand.ts:
Line 16:
Missing error handling around the network call — Rule 30 requires external/network calls to be wrapped in try/catch with context added and mapped to application-level errors. Wrap the GET in try/catch, log the operation name and identifiers (`callId`, `unitId`), and rethrow or map to a domain error such as `ApiError`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| * assignment (MyAssignment). When no unit id is available the endpoint is | ||
| * called without it and the server resolves no assignment. | ||
| */ | ||
| export const getResourceIncidentView = async (callId: string | number, unitId?: string | number | null) => { |
There was a problem hiding this comment.
Missing JSDoc Promise and error annotations on getResourceIncidentView — Rule 23 requires async functions to document the resolve value and rejection conditions. Add @returns {Promise<ResourceIncidentViewResult>} and @throws {Error} when the API request fails or the response is invalid. to the existing JSDoc block.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/api/calls/incidentCommand.ts:
Line 13:
Missing JSDoc Promise and error annotations on `getResourceIncidentView` — Rule 23 requires async functions to document the resolve value and rejection conditions. Add `@returns {Promise<ResourceIncidentViewResult>}` and `@throws {Error} when the API request fails or the response is invalid.` to the existing JSDoc block.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <ButtonIcon as={MapPinIcon} /> | ||
| <ButtonText className="text-xs">{t('call_detail.call_location')}</ButtonText> | ||
| </Button> | ||
| <Button onPress={() => setMapTarget('destination')} variant={showingDestination ? 'solid' : 'outline'} size="sm" className="flex-1 rounded-none" testID="call-detail-map-toggle-destination"> |
There was a problem hiding this comment.
Inline arrow function in the JSX onPress prop creates a new function instance on every render, causing unnecessary re-renders of child components. Extract the handler as a stable callback via useCallback and pass it directly as onPress.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/app/call/[id].tsx:
Line 569:
Inline arrow function in the JSX `onPress` prop creates a new function instance on every render, causing unnecessary re-renders of child components. Extract the handler as a stable callback via `useCallback` and pass it directly as `onPress`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch (error) { | ||
| logger.error({ | ||
| message: 'Failed to open contact link', | ||
| context: { error, url }, |
There was a problem hiding this comment.
PII exposure in logs — the url field in the structured log context contains raw tel:${contact.Phone} or mailto:${contact.Email} URLs, leaking phone numbers and email addresses. Redact or hash the personal-data portion of the URL before logging, emitting only the scheme or a hashed identifier.
Kody rule violation: Mask PII and secrets in logs
Prompt for LLM
File src/components/incident-command/incident-command-tab-panel.tsx:
Line 87:
PII exposure in logs — the `url` field in the structured log context contains raw `tel:${contact.Phone}` or `mailto:${contact.Email}` URLs, leaking phone numbers and email addresses. Redact or hash the personal-data portion of the URL before logging, emitting only the scheme or a hashed identifier.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch (error) { | ||
| logger.error({ | ||
| message: 'Failed to open contact link', | ||
| context: { error, url }, |
There was a problem hiding this comment.
PII exposure in diagnostic output — the url field in the structured log context contains unredacted phone numbers and email addresses via tel:/mailto: URLs, violating GDPR data minimization. Strip or mask the personal-data portion of the URL before logging.
Kody rule violation: Redact PII in logs and metrics by default
Prompt for LLM
File src/components/incident-command/incident-command-tab-panel.tsx:
Line 87:
PII exposure in diagnostic output — the `url` field in the structured log context contains unredacted phone numbers and email addresses via `tel:`/`mailto:` URLs, violating GDPR data minimization. Strip or mask the personal-data portion of the URL before logging.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| {contact.Email ? ( | ||
| <Pressable onPress={() => openContactUrl(`mailto:${contact.Email}`)} testID={`${testID}-email`}> | ||
| <HStack className="mt-1 items-center"> | ||
| <MailIcon size={14} color="#3B82F6" /> |
There was a problem hiding this comment.
Duplicated hardcoded hex literal #3B82F6 — the color is inline in both icon declarations and should be defined once as a module-level constant. Extract const LINK_ICON_COLOR = '#3B82F6' and reference it in both places.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/components/incident-command/incident-command-tab-panel.tsx:
Line 114:
Duplicated hardcoded hex literal `#3B82F6` — the color is inline in both icon declarations and should be defined once as a module-level constant. Extract `const LINK_ICON_COLOR = '#3B82F6'` and reference it in both places.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| {contact.Phone ? ( | ||
| <Pressable onPress={() => openContactUrl(`tel:${contact.Phone}`)} testID={`${testID}-phone`}> | ||
| <HStack className="mt-1 items-center"> | ||
| <PhoneIcon size={14} color="#3B82F6" /> |
There was a problem hiding this comment.
Unnamed numeric literal 14 used as an icon size degrades maintainability. Extract a module-level constant const ICON_SIZE = 14 and replace the inline value.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/components/incident-command/incident-command-tab-panel.tsx:
Line 106:
Unnamed numeric literal `14` used as an icon size degrades maintainability. Extract a module-level constant `const ICON_SIZE = 14` and replace the inline value.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector({ | ||
| activeCall: callWithAddressOnly, | ||
| activePriority: mockPriority, | ||
| setActiveCall: mockSetActiveCall, | ||
| }) : { | ||
| activeCall: callWithAddressOnly, | ||
| activePriority: mockPriority, | ||
| setActiveCall: mockSetActiveCall, | ||
| }); |
There was a problem hiding this comment.
Duplicated test setup — the mockUseCoreStore.mockImplementation(...) block is repeated seven times (lines 582–590, 620–628, 636–644, 652–660, 680–688, 699–707, 734–742) with only the activeCall value changing. Extract a helper like setupMockStore(call) and call it with the desired call object to reduce drift risk.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/components/sidebar/__tests__/call-sidebar.test.tsx:
Line 582 to 590:
Duplicated test setup — the `mockUseCoreStore.mockImplementation(...)` block is repeated seven times (lines 582–590, 620–628, 636–644, 652–660, 680–688, 699–707, 734–742) with only the `activeCall` value changing. Extract a helper like `setupMockStore(call)` and call it with the desired call object to reduce drift risk.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }; | ||
|
|
||
| // Check if the call carries a routable destination POI (coordinates or address) | ||
| const hasDestinationData = (call: typeof activeCall) => { |
There was a problem hiding this comment.
The (0,0) no-data sentinel passes the != null check in hasDestinationData and handleDestinationDirections, causing openMapsWithDirections(0, 0, name) to open navigation to null island (Gulf of Guinea) when the server sends DestinationLatitude=0 / DestinationLongitude=0. Mirror the exclusion already applied at src/app/call/[id].tsx line 519 by adding the (0,0) guard in both functions.
const hasDestinationData = (call: typeof activeCall) => {
if (!call) return false;
const hasCoordinates =
call.DestinationLatitude != null &&
call.DestinationLongitude != null &&
(call.DestinationLatitude !== 0 || call.DestinationLongitude !== 0);
const hasAddress = !!call.DestinationAddress && call.DestinationAddress.trim() !== '';
return hasCoordinates || hasAddress;
};
// and in handleDestinationDirections:
// if (latitude != null && longitude != null && (latitude !== 0 || longitude !== 0)) {Prompt for LLM
File src/components/sidebar/call-sidebar.tsx:
Line 119:
The (0,0) no-data sentinel passes the `!= null` check in `hasDestinationData` and `handleDestinationDirections`, causing `openMapsWithDirections(0, 0, name)` to open navigation to null island (Gulf of Guinea) when the server sends `DestinationLatitude=0` / `DestinationLongitude=0`. Mirror the exclusion already applied at `src/app/call/[id].tsx` line 519 by adding the (0,0) guard in both functions.
Suggested Code:
const hasDestinationData = (call: typeof activeCall) => {
if (!call) return false;
const hasCoordinates =
call.DestinationLatitude != null &&
call.DestinationLongitude != null &&
(call.DestinationLatitude !== 0 || call.DestinationLongitude !== 0);
const hasAddress = !!call.DestinationAddress && call.DestinationAddress.trim() !== '';
return hasCoordinates || hasAddress;
};
// and in handleDestinationDirections:
// if (latitude != null && longitude != null && (latitude !== 0 || longitude !== 0)) {
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch { | ||
| showLocationAlert(); | ||
| } |
There was a problem hiding this comment.
Silent error swallowing — the catch block uses optional catch binding (catch {) without capturing the error object, discarding exception details needed to debug failed map-direction opens. Capture the error variable (catch (error)) and log it with structured context before calling showLocationAlert.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File src/components/sidebar/call-sidebar.tsx:
Line 141 to 143:
Silent error swallowing — the catch block uses optional catch binding (`catch {`) without capturing the error object, discarding exception details needed to debug failed map-direction opens. Capture the error variable (`catch (error)`) and log it with structured context before calling `showLocationAlert`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public DepartmentId: number = 0; | ||
| public CallId: number = 0; | ||
| public Name: string = ''; | ||
| public ObjectiveType: number = 0; |
There was a problem hiding this comment.
Type mismatch — ObjectiveType is typed as number but a dedicated TacticalObjectiveType enum is defined in the same file (lines 4–8) and already encodes the valid values. Change the type to TacticalObjectiveType to restore compile-time guarantees for downstream consumers.
Kody rule violation: Derive TypeScript types from validation schemas
Prompt for LLM
File src/models/v4/incidentCommand/resourceIncidentView.ts:
Line 45:
Type mismatch — `ObjectiveType` is typed as `number` but a dedicated `TacticalObjectiveType` enum is defined in the same file (lines 4–8) and already encodes the valid values. Change the type to `TacticalObjectiveType` to restore compile-time guarantees for downstream consumers.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| * getLastNotificationResponseAsync — dedupe by identifier so it shows once while | ||
| * distinct notifications still each get handled. | ||
| */ | ||
| private handledResponseIds = new Set<string>(); |
There was a problem hiding this comment.
Unbounded memory growth in the PushNotification singleton — the handledResponseIds Set accumulates an entry on every notification tap (line 241) and is never cleared, not even in cleanup() (lines 470–485). Clear handledResponseIds in cleanup() via this.handledResponseIds.clear().
public cleanup(): void {
if (this.notificationListener) {
this.notificationListener.remove();
this.notificationListener = null;
}
if (this.responseListener) {
this.responseListener.remove();
this.responseListener = null;
}
if (this.notifeeForegroundUnsubscribe) {
this.notifeeForegroundUnsubscribe();
this.notifeeForegroundUnsubscribe = null;
}
this.handledResponseIds.clear();
}Prompt for LLM
File src/services/push-notification.ts:
Line 83:
Unbounded memory growth in the `PushNotification` singleton — the `handledResponseIds` Set accumulates an entry on every notification tap (line 241) and is never cleared, not even in `cleanup()` (lines 470–485). Clear `handledResponseIds` in `cleanup()` via `this.handledResponseIds.clear()`.
Suggested Code:
public cleanup(): void {
if (this.notificationListener) {
this.notificationListener.remove();
this.notificationListener = null;
}
if (this.responseListener) {
this.responseListener.remove();
this.responseListener = null;
}
if (this.notifeeForegroundUnsubscribe) {
this.notifeeForegroundUnsubscribe();
this.notifeeForegroundUnsubscribe = null;
}
this.handledResponseIds.clear();
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| fetchIncidentView: async (callId: string) => { | ||
| const seq = ++requestSeq; | ||
| set({ isLoading: true, error: null }); |
There was a problem hiding this comment.
Stale view data leaks across calls because fetchIncidentView sets isLoading and error but never clears the previous view in the useIncidentCommandStore singleton, causing IncidentCommandTabPanel to render the previous incident's commander, objectives, needs, important-information, and notes for one committed frame before the spinner appears. Clear view at the start of fetchIncidentView by setting view: null.
fetchIncidentView: async (callId: string) => {
const seq = ++requestSeq;
set({ isLoading: true, error: null, view: null });Prompt for LLM
File src/stores/calls/incident-command-store.ts:
Line 31 to 33:
Stale view data leaks across calls because `fetchIncidentView` sets `isLoading` and `error` but never clears the previous `view` in the `useIncidentCommandStore` singleton, causing `IncidentCommandTabPanel` to render the previous incident's commander, objectives, needs, important-information, and notes for one committed frame before the spinner appears. Clear `view` at the start of `fetchIncidentView` by setting `view: null`.
Suggested Code:
fetchIncidentView: async (callId: string) => {
const seq = ++requestSeq;
set({ isLoading: true, error: null, view: null });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const result = await getResourceIncidentView(callId, activeUnitId ?? undefined); | ||
| if (seq !== requestSeq) return; | ||
|
|
||
| if (result && result.Data && result.Status !== 'NotFound') { |
There was a problem hiding this comment.
Magic string 'NotFound' compared against result.Status — status values represent a finite set that should be expressed as an enum or named constant for type safety and discoverability. Define a shared constant or enum and compare with result.Status !== IncidentStatus.NotFound.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/stores/calls/incident-command-store.ts:
Line 39:
Magic string `'NotFound'` compared against `result.Status` — status values represent a finite set that should be expressed as an enum or named constant for type safety and discoverability. Define a shared constant or enum and compare with `result.Status !== IncidentStatus.NotFound`.
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/stores/calls/__tests__/incident-command-store.test.ts (1)
199-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest state-transition logic is correct. Each case aligns with the store's
requestSeqsupersession guard (synchronous clear, superseded-result ignore, post-reset success/error ignore).One guideline nit: the repeated
as anycasts on mock views and resolved values (e.g., Lines 200, 216-217, 226, 238, 267) sidestep type checking and won't catch drift in theResourceIncidentViewResult/ResourceIncidentViewshape. Prefer typingcreateMockView()and the mocked resolutions to the real result type.As per coding guidelines: "Never use
anytype; use precise types and interfaces with TypeScript strict mode enabled".🤖 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/calls/__tests__/incident-command-store.test.ts` around lines 199 - 306, Replace the any casts throughout the incident-command fetch tests with the actual ResourceIncidentView and ResourceIncidentViewResult types. Type createMockView and the mocked promise resolve values used by fetchIncidentView, including the superseded, reset-success, and reset-error cases, so TypeScript validates the real response shape without weakening strict checking.Source: Coding guidelines
src/components/sidebar/__tests__/call-sidebar.test.tsx (2)
556-606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
unmount()cleanup to new tests.None of these new test cases call
unmount()after their assertions, per the repo's testing guideline for co-located test files.As per coding guidelines, "Always call
unmount()in tests to clean up after assertions."✅ Example fix
render(<SidebarCallCard />); fireEvent.press(screen.getByTestId('map-pin-icon')); await new Promise(resolve => setTimeout(resolve, 0)); expect(mockAlert.alert).toHaveBeenCalledWith(...); + unmount(); });Also applies to: 697-809
🤖 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/sidebar/__tests__/call-sidebar.test.tsx` around lines 556 - 606, Update the new tests for map directions and address handling, along with the additional tests in the referenced range, to capture the render result and call unmount() after assertions. Preserve each test’s existing setup, async behavior, and expectations while ensuring every rendered SidebarCallCard is cleaned up.Source: Coding guidelines
557-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
waitForfor these async assertions. ImportwaitForfrom@testing-library/react-nativeand replace thesetTimeout(0)flush in these cases withawait waitFor(...)(including the destination routing checks).🤖 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/sidebar/__tests__/call-sidebar.test.tsx` around lines 557 - 572, Update the asynchronous assertions in the SidebarCallCard tests, including the destination routing checks, to import and use waitFor from `@testing-library/react-native` instead of manually flushing with setTimeout(0). Place the relevant expectations inside await waitFor callbacks while preserving their existing assertion behavior.
🤖 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.
Nitpick comments:
In `@src/components/sidebar/__tests__/call-sidebar.test.tsx`:
- Around line 556-606: Update the new tests for map directions and address
handling, along with the additional tests in the referenced range, to capture
the render result and call unmount() after assertions. Preserve each test’s
existing setup, async behavior, and expectations while ensuring every rendered
SidebarCallCard is cleaned up.
- Around line 557-572: Update the asynchronous assertions in the SidebarCallCard
tests, including the destination routing checks, to import and use waitFor from
`@testing-library/react-native` instead of manually flushing with setTimeout(0).
Place the relevant expectations inside await waitFor callbacks while preserving
their existing assertion behavior.
In `@src/stores/calls/__tests__/incident-command-store.test.ts`:
- Around line 199-306: Replace the any casts throughout the incident-command
fetch tests with the actual ResourceIncidentView and ResourceIncidentViewResult
types. Type createMockView and the mocked promise resolve values used by
fetchIncidentView, including the superseded, reset-success, and reset-error
cases, so TypeScript validates the real response shape without weakening strict
checking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b15724c2-fec1-4d19-a24e-605f50b84e22
📒 Files selected for processing (9)
src/app/call/[id].tsxsrc/components/incident-command/incident-command-tab-panel.tsxsrc/components/sidebar/__tests__/call-sidebar.test.tsxsrc/components/sidebar/call-sidebar.tsxsrc/models/v4/incidentCommand/resourceIncidentView.tssrc/models/v4/incidentCommand/resourceIncidentViewResult.tssrc/services/__tests__/push-notification.test.tssrc/stores/calls/__tests__/incident-command-store.test.tssrc/stores/calls/incident-command-store.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/models/v4/incidentCommand/resourceIncidentViewResult.ts
- src/stores/calls/incident-command-store.ts
- src/services/tests/push-notification.test.ts
- src/components/sidebar/call-sidebar.tsx
- src/app/call/[id].tsx
- src/components/incident-command/incident-command-tab-panel.tsx
| const hasDestinationData = (call: typeof activeCall) => { | ||
| if (!call) return false; | ||
| // (0,0) is the server's no-data sentinel, not a real destination | ||
| const hasCoordinates = call.DestinationLatitude != null && call.DestinationLongitude != null && (call.DestinationLatitude !== 0 || call.DestinationLongitude !== 0); |
There was a problem hiding this comment.
Duplicate null-plus-(0,0)-sentinel coordinate validation at src/components/sidebar/call-sidebar.tsx:137 risks divergence if the validation logic changes. Extract a shared helper such as isValidCoordinate(lat, lng) to centralize the null and sentinel checks for both hasDestinationData and the navigation handler.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/components/sidebar/call-sidebar.tsx:
Line 122:
Duplicate null-plus-(0,0)-sentinel coordinate validation at `src/components/sidebar/call-sidebar.tsx:137` risks divergence if the validation logic changes. Extract a shared helper such as `isValidCoordinate(lat, lng)` to centralize the null and sentinel checks for both `hasDestinationData` and the navigation handler.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Pull Request Description
This PR introduces several new features, bug fixes, and improvements:
New: Incident Command Tab
Adds a resource-facing incident command view to the call detail screen, displaying:
Includes the API endpoint, data models, a Zustand store (with stale-request and reset guards), and the tab panel UI component.
New: Destination POI Routing
Fix: Duplicate Push Notification Modal on Cold Start
Resolves an issue where the same cold-start notification tap could trigger the notification modal twice — once from the notification response listener and again from
getLastNotificationResponseAsync. Both paths now flow through a shared handler that deduplicates by request identifier while still allowing distinct notifications to be processed.Other Fixes
GetWeatherAlertnow uses a path parameter instead of a query parameter; the detail store also clears stale alert data before refetchingas anyprops cast with propersize/anchorprops and adjusted the sidebar widthgrow-0Summary by CodeRabbit