Repository navigation
Conversation
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:
|
| await act(async () => { | ||
| jest.advanceTimersByTime(MAP_SIGNALR_REFETCH_DELAY_MS); | ||
| }); | ||
| await waitFor(() => expect(personMarker.remove).toHaveBeenCalled()); |
There was a problem hiding this comment.
Unhandled waitFor rejections can obscure timeout or assertion failures in src/tests/app/map.web.test.tsx:235, src/components/maps/tests/unified-map-view.web.test.tsx:243, src/components/maps/tests/unified-map-view.web.test.tsx:226, src/tests/app/map.web.test.tsx:246, src/components/maps/tests/unified-map-view.web.test.tsx:240-242, and src/components/maps/tests/unified-map-view.web.test.tsx:260-262. Wrap each awaited waitFor call in try/catch to handle or rethrow the failure explicitly.
Kody rule violation: Handle async operations with proper error handling
try {
await waitFor(() => expect(personMarker.remove).toHaveBeenCalled());
} catch (error) {
// Handle or assert the wait failure.
throw error;
}Prompt for LLM
File src/__tests__/app/map.web.test.tsx:
Line 249:
Unhandled waitFor rejections can obscure timeout or assertion failures in src/__tests__/app/map.web.test.tsx:235, src/components/maps/__tests__/unified-map-view.web.test.tsx:243, src/components/maps/__tests__/unified-map-view.web.test.tsx:226, src/__tests__/app/map.web.test.tsx:246, src/components/maps/__tests__/unified-map-view.web.test.tsx:240-242, and src/components/maps/__tests__/unified-map-view.web.test.tsx:260-262. Wrap each awaited waitFor call in try/catch to handle or rethrow the failure explicitly.
Suggested Code:
try {
await waitFor(() => expect(personMarker.remove).toHaveBeenCalled());
} catch (error) {
// Handle or assert the wait failure.
throw error;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const timer = setTimeout(() => { | ||
| handledTimestampRef.current = lastUpdateTimestamp; | ||
| logger.debug({ | ||
| message: 'Refetching map markers after an update hub change', | ||
| context: { timestamp: lastUpdateTimestamp }, | ||
| }); | ||
| refetchRef.current(); |
There was a problem hiding this comment.
useMapSignalRRefetch records the hub timestamp as handled before invoking the asynchronous refetch, so transient network failures in fetchMapData or fetchPins are caught and swallowed, leaving the map stale until a later hub event changes the timestamp. Make refetch return a success-aware Promise, await it in the timer, and update handledTimestampRef only after the fetch succeeds or explicitly requeue the timestamp on rejection.
const timer = setTimeout(async () => {\n logger.debug({\n message: 'Refetching map markers after an update hub change',\n context: { timestamp: lastUpdateTimestamp },\n });\n try {\n await refetchRef.current();\n handledTimestampRef.current = lastUpdateTimestamp;\n } catch {\n // Leave the timestamp unhandled so the event can be retried.\n }\n}, DEBOUNCE_DELAY);Prompt for LLM
File src/hooks/use-map-signalr-updates.ts:
Line 214 to 220:
useMapSignalRRefetch records the hub timestamp as handled before invoking the asynchronous refetch, so transient network failures in fetchMapData or fetchPins are caught and swallowed, leaving the map stale until a later hub event changes the timestamp. Make refetch return a success-aware Promise, await it in the timer, and update handledTimestampRef only after the fetch succeeds or explicitly requeue the timestamp on rejection.
Suggested Code:
const timer = setTimeout(async () => {\n logger.debug({\n message: 'Refetching map markers after an update hub change',\n context: { timestamp: lastUpdateTimestamp },\n });\n try {\n await refetchRef.current();\n handledTimestampRef.current = lastUpdateTimestamp;\n } catch {\n // Leave the timestamp unhandled so the event can be retried.\n }\n}, DEBOUNCE_DELAY);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| it('draws the status pin where the bundled pin goes, stepping down to the bundled pin and then the flag when loads fail', () => { | ||
| const el = createMapMarkerElement(pin({ Type: 1, ImagePath: 'Engine_Responding', PinIconUrl: PIN_ICON_URL, IconStyle: 'responding' })); | ||
| const img = el.querySelector('img')!; |
There was a problem hiding this comment.
The non-null assertion on el.querySelector('img') can permit a runtime null dereference when the query returns no image element. Add an explicit null check and throw an Error with 'Expected marker image' before using img.
Kody rule violation: Add null checks before accessing properties
const img = el.querySelector('img');
if (!img) throw new Error('Expected marker image');Prompt for LLM
File src/lib/__tests__/map-markers-web.test.ts:
Line 128:
The non-null assertion on el.querySelector('img') can permit a runtime null dereference when the query returns no image element. Add an explicit null check and throw an Error with 'Expected marker image' before using img.
Suggested Code:
const img = el.querySelector('img');
if (!img) throw new Error('Expected marker image');
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Status-pin data and rendering src/models/v4/mapping/getMapDataAndMarkersData.ts, src/components/common/type-icon.tsx, src/lib/map-markers-web.ts, src/components/maps/pin-marker.tsx, src/lib/__tests__/map-markers-web.test.ts, src/components/maps/__tests__/pin-marker.test.tsx |
Map marker data adds status-pin URL and style fields. Marker rendering tries uploaded icons, status pins, and bundled images in sequence. Image fitting and marker signatures include the status-pin inputs. |
SignalR-triggered map refetch src/hooks/use-map-signalr-updates.ts, src/app/(app)/map.web.tsx, src/components/maps/unified-map-view.web.tsx, src/hooks/__tests__/use-map-signalr-updates.test.ts, src/__tests__/app/map.web.test.tsx, src/components/maps/__tests__/unified-map-view.web.test.tsx |
A debounced hook requests background pin refreshes when update timestamps change. Both web map views use the hook for internally fetched pins. Tests cover debouncing, marker updates, and unchanged camera position. |
Filter dispatch overlays by availability src/lib/dispatch-board.ts, src/lib/dispatch-map-layers-web.ts, src/hooks/use-dispatch-map-overlay.ts, src/components/maps/dispatch-map-legend.tsx, src/components/maps/location-picker.tsx, src/lib/__tests__/dispatch-board.test.ts, src/translations/*.json |
Dispatch overlays omit unavailable units and personnel from map features, counts, and nearest-point framing. Unavailable resources remain in the recipient picker. Map labels now specify that displayed units and personnel are available. |
Android CI setup
| Layer / File(s) | Summary |
|---|---|
Configure Java for Android CI .github/workflows/react-native-cicd.yml |
The workflow installs Temurin JDK 17 for Android matrix jobs only. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant SignalRStore
participant useMapSignalRRefetch
participant requestPinsRefresh
participant MapView
SignalRStore->>useMapSignalRRefetch: provide changed update timestamp
useMapSignalRRefetch->>requestPinsRefresh: invoke after debounce delay
requestPinsRefresh->>MapView: refresh map pins in background
MapView->>MapView: rebuild markers with updated pin fields
Possibly related PRs
- Resgrid/Dispatch#153: Adds an update-hub event that changes the timestamp used by this PR to trigger map refreshes.
Suggested reviewers: resgrid-bot
Merge Risk: 
ee782
-
A transient fetch failure can leave map pins showing outdated status until another update or refresh. Resolve or explicitly accept that risk before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 17 files. (11 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. Title check The title "Develop" is a generic branch name, not a description of the change. It gives no information about the changeset, which covers custom map pin status/priority icons, a SignalR-driven map refr… Replace the title with a short, specific summary of the main change. For example: "Add status/priority map pins and SignalR-driven map refresh" or "Show only available units on dispatch map." Also add a PR description that lists the main ch… -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 17 files. (11 skipped: 11 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Title check
-
Explanation
The title "Develop" is a generic branch name, not a description of the change. It gives no information about the changeset, which covers custom map pin status/priority icons, a SignalR-driven map refresh hook, and dispatch-board availability filtering. A teammate scanning history cannot tell the primary change from this title.
Resolution
Replace the title with a short, specific summary of the main change. For example: "Add status/priority map pins and SignalR-driven map refresh" or "Show only available units on dispatch map." Also add a PR description that lists the main changes.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
|
||
| it('draws the status pin where the bundled pin goes, stepping down to the bundled pin and then the flag when loads fail', () => { | ||
| const el = createMapMarkerElement(pin({ Type: 1, ImagePath: 'Engine_Responding', PinIconUrl: PIN_ICON_URL, IconStyle: 'responding' })); | ||
| const img = el.querySelector('img')!; |
There was a problem hiding this comment.
The non-null assertion on el.querySelector('img') can permit a runtime null dereference when the query returns no image element. Add an explicit null check and throw an Error with 'Expected marker image' before accessing img.
Kody rule violation: Add null checks to prevent NullReferenceException
const img = el.querySelector('img');
if (!img) throw new Error('Expected marker image');Prompt for LLM
File src/lib/__tests__/map-markers-web.test.ts:
Line 128:
The non-null assertion on el.querySelector('img') can permit a runtime null dereference when the query returns no image element. Add an explicit null check and throw an Error with 'Expected marker image' before accessing img.
Suggested Code:
const img = el.querySelector('img');
if (!img) throw new Error('Expected marker image');
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}
Pull Request Description
Summary
This pull request updates map marker rendering, realtime map refresh behavior, dispatch map overlays, and Android CI configuration.
Changes
Pin status and priority rendering
PinIconUrlandIconStylemap marker fields for status- and priority-specific pin images.containorcoverimage fitting inTypeIcon.Realtime marker updates
UnifiedMapViewremain managed by the parent and are not independently refetched.Dispatch map overlays
Testing
Android CI
Summary by CodeRabbit