Conversation
| if (lower.includes('.m3u8')) return FeedFormat.HLS; | ||
| if (lower.includes('.mpd')) return FeedFormat.DASH; | ||
| if (lower.startsWith('rtsp://')) return FeedFormat.RTSP; | ||
| if (lower.includes('youtube.com') || lower.includes('youtu.be')) return FeedFormat.YouTubeLive; |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds check‑in timers (polling, offline queueing, iOS Live Activity, Android notifications), weather alerts (models, API, store, UI, SignalR), call video feeds (models, API, store, UI/player), POI support (models, store, UI, map markers), an Expo iOS plugin for Live Activity, many new hooks/services/stores, broad i18n additions, and dependency/tooling bumps. Check‑in / Weather / Video / POI Feature Cohort
sequenceDiagram
participant UI as App UI (CallDetail / Sidebar)
participant Store as CheckInTimerStore
participant PollHook as useCheckInTimerPolling
participant API as CheckIn API
participant Native as iOS LiveActivity
participant Notifee as Android Notification Service
participant Offline as OfflineEventManager
UI->>Store: startPolling(callId, interval)
Store->>PollHook: subscribe/status refresh
PollHook->>API: GET /CheckIn/Statuses?callId
API-->>PollHook: statuses
PollHook->>Store: set timerStatuses
PollHook->>Native: start/update Live Activity (iOS)
PollHook->>Notifee: start/update notification (Android)
UI->>Store: performCheckIn(input)
Store->>API: POST /CheckIn
alt API online success
API-->>Store: success
Store->>PollHook: refresh statuses
Store-->>UI: return "success"
else API offline / network error
Store->>Offline: queueCheckInEvent(payload)
Store-->>UI: return "queued"
end
UI->>Native: end Live Activity (on stop/unmount)
UI->>Notifee: stop notification (on stop/unmount)
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/call/new/index.tsx (1)
2-2:⚠️ Potential issue | 🟠 MajorRemove unused
@testing-library/react-nativeimport from production code.Line 2 imports
renderfrom@testing-library/react-native, but the function is never used. Test utilities should not appear in shipped code—this unnecessarily bloats the production bundle and may cause runtime issues in environments where test dependencies are absent. Remove the import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/call/new/index.tsx` at line 2, The file imports the test helper "render" from "@testing-library/react-native" but never uses it; remove the unused import statement (the `render` import from `@testing-library/react-native`) from the module (index.tsx) so test utilities are not bundled into production; after removal, run the linter/build to confirm no remaining references to `render` or other test-only imports.src/app/(app)/index.tsx (1)
1-29:⚠️ Potential issue | 🟠 MajorFix import sort to unblock CI.
ESLint
simple-import-sort/importsis failing on this file (per pipeline failure logs). Lines 28-29 adduseWeatherAlertsStore(from@/stores/...) beforeWeatherAlertBanner(from@/components/...), breaking the existing grouping/ordering convention.🔧 Proposed fix
import MapPins from '@/components/maps/map-pins'; import Mapbox from '@/components/maps/mapbox'; import PinDetailModal from '@/components/maps/pin-detail-modal'; import { StopMarker } from '@/components/routes/stop-marker'; import { FocusAwareStatusBar } from '@/components/ui/focus-aware-status-bar'; +import { WeatherAlertBanner } from '@/components/weather-alerts/weather-alert-banner'; import { useAnalytics } from '@/hooks/use-analytics'; ... import { useRoutesStore } from '@/stores/routes/store'; import { useToastStore } from '@/stores/toast/store'; -import { useWeatherAlertsStore } from '@/stores/weather-alerts/store'; -import { WeatherAlertBanner } from '@/components/weather-alerts/weather-alert-banner'; +import { useWeatherAlertsStore } from '@/stores/weather-alerts/store';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(app)/index.tsx around lines 1 - 29, Reorder the imports so they satisfy simple-import-sort: move the WeatherAlertBanner import to come before useWeatherAlertsStore (i.e. ensure '@/components/...' imports appear before '@/stores/...' imports), and then run the linter; specifically adjust the import block containing WeatherAlertBanner and useWeatherAlertsStore so WeatherAlertBanner is listed earlier and the grouped/import ordering convention (external packages, then app components, hooks, libs, models, services, stores) is preserved.
♻️ Duplicate comments (1)
src/components/call-video-feeds/feed-format-utils.ts (1)
63-63:⚠️ Potential issue | 🟡 MinorUse hostname matching for YouTube detection.
includes('youtube.com')matches anywhere in the URL, sohttps://malicious.example/?ref=youtube.com/...is mis-classified asYouTubeLiveand routed to the YouTube branch in the player. Parse the URL and check the hostname against an allowlist instead.🛡️ Proposed fix
- if (lower.includes('youtube.com') || lower.includes('youtu.be')) return FeedFormat.YouTubeLive; + try { + const host = new URL(url).hostname.toLowerCase(); + if (host === 'youtube.com' || host.endsWith('.youtube.com') || host === 'youtu.be') { + return FeedFormat.YouTubeLive; + } + } catch { + // not a valid absolute URL; fall through + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/call-video-feeds/feed-format-utils.ts` at line 63, The current string includes check mis-classifies URLs; update the YouTube detection in the function that returns FeedFormat.YouTubeLive to parse the input as a URL (using new URL(...)) and match URL.hostname against an allowlist (e.g. "youtube.com", "www.youtube.com", "m.youtube.com", and "youtu.be") or use endsWith(".youtube.com") for subdomains; wrap parsing in a try/catch and only return FeedFormat.YouTubeLive when the hostname matches the allowlist, otherwise fall back to the existing non-YouTube logic (do not rely on lower.includes('youtube.com') or similar).
🟡 Minor comments (24)
src/models/v4/destinations/destinationEntityTypes.ts-1-6 (1)
1-6:⚠️ Potential issue | 🟡 MinorFile naming convention violation.
The file name
destinationEntityTypes.tsuses camelCase, but the project guidelines require lowercase-hyphenated names. Please rename this file todestination-entity-types.ts.As per coding guidelines, "Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/v4/destinations/destinationEntityTypes.ts` around lines 1 - 6, Rename the file containing the DestinationEntityTypes enum from camelCase to the project's lowercase-hyphenated convention: change destinationEntityTypes.ts to destination-entity-types.ts and update any imports that reference DestinationEntityTypes (e.g., import { DestinationEntityTypes } ...) so they point to the new filename; ensure the exported symbol name DestinationEntityTypes and its enum members remain unchanged.src/models/v4/checkIn/checkInRecordResultData.ts-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor
CallIdshould be typed asstring, notnumber—inconsistent with other models.Every other ID model (
CallResultData,CallFileResultData,CallVideoFeedResultData) declaresCallId: string. The code atcheck-in-sidebar-widget.tsx:28converts it withparseInt(activeCall.CallId, 10), confirming it arrives as a string from the backend. Note:callNoteResultData.tshas the identical issue.- public CallId: number = 0; + public CallId: string = '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/v4/checkIn/checkInRecordResultData.ts` at line 3, Change the CallId property types from number to string in the affected model(s): update CallId on the CheckInRecordResultData class (where currently declared as "public CallId: number = 0;") to be a string with a string default (e.g., ""), and make the same change in callNoteResultData.ts where CallId is incorrectly typed; this aligns these models with CallResultData/CallFileResultData/CallVideoFeedResultData and avoids needless parseInt usage elsewhere like in check-in-sidebar-widget.tsx.src/translations/fr.json-118-118 (1)
118-118:⚠️ Potential issue | 🟡 Minor
minutes_agotranslation is missing the temporal "ago" marker, making it inconsistent with all other languages.The French translation
"min"lacks the temporal marker that other languages include: English"min ago", Spanish"min atrás", Italian"min fa", Polish"min temu", Swedish"min sedan", German"Min. her". The phrase renders as{minutesAgo} min(e.g., "5 min"), which is ambiguous—it could indicate duration or time elapsed rather than explicitly conveying the "ago" meaning.Align with the pattern used by other languages by adding a temporal suffix to match their approach (e.g., following Romance language conventions similar to Spanish).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/translations/fr.json` at line 118, Update the minutes_ago translation so it explicitly includes the temporal "ago" marker; replace the current "minutes_ago" value ("min") with a French phrasing that conveys elapsed time (e.g., "il y a {minutesAgo} min") so the rendered string becomes "il y a 5 min" instead of just "5 min". Ensure the change is applied to the "minutes_ago" JSON entry so consumers using the minutes_ago key get the temporal suffix.src/services/check-in-notification.service.ts-50-56 (1)
50-56:⚠️ Potential issue | 🟡 MinorCountdown interval keeps re-displaying every second forever, even after the timer reaches zero.
Math.max(0, this.currentSeconds - 1)clamps at 0, butsetIntervalcontinues firing anddisplayNotificationkeeps re-issuingnotifee.displayNotificationindefinitely with the same0:00body. That's wasted work and continual system-notification updates untilstopNotificationis called externally. Consider stopping the countdown when reaching zero (or transitioning to an "Overdue" steady state without a per-second redraw).🛠️ Proposed adjustment
this.countdownInterval = setInterval(async () => { - this.currentSeconds = Math.max(0, this.currentSeconds - 1); - await this.displayNotification(callName, callNumber, timerName); + if (this.currentSeconds <= 0) { + this.stopCountdown(); + return; + } + this.currentSeconds -= 1; + await this.displayNotification(callName, callNumber, timerName); }, 1000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/check-in-notification.service.ts` around lines 50 - 56, The countdown interval in check-in-notification.service keeps firing after currentSeconds reaches 0; modify the setInterval callback in the method that starts the timer so that after decrementing currentSeconds you detect when it has reached 0, call stopCountdown() (or clearInterval(this.countdownInterval)) and then call displayNotification one final time (or transition to the "Overdue" state) instead of continuing to schedule per-second updates; update the logic around this.countdownInterval, stopCountdown(), currentSeconds, and displayNotification to ensure the interval is cleared as soon as currentSeconds === 0 so no further notifee.displayNotification calls occur.src/stores/call-video-feeds/store.ts-37-44 (1)
37-44:⚠️ Potential issue | 🟡 MinorGuard against non-array
Databefore spreading.If the backend returns
null/undefinedforresult.Data(e.g., empty payload or auth edge case),[...result.Data].sort(...)will throw aTypeErrorand the catch will surface it as a confusing "is not iterable" error instead of an empty list. The rest of the codebase consistently usesArray.isArray(...)guards (seesrc/stores/calls/store.ts).- const result = await getCallVideoFeeds(callId); - const sorted = [...result.Data].sort((a, b) => a.SortOrder - b.SortOrder); - set({ feeds: sorted, isLoadingFeeds: false }); + const result = await getCallVideoFeeds(callId); + const data = Array.isArray(result.Data) ? result.Data : []; + const sorted = [...data].sort((a, b) => a.SortOrder - b.SortOrder); + set({ feeds: sorted, isLoadingFeeds: false });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/call-video-feeds/store.ts` around lines 37 - 44, The code assumes result.Data is iterable before sorting, causing a TypeError when it's null/undefined; update the fetch handling in the async block that calls getCallVideoFeeds to guard result.Data with Array.isArray and default to an empty array before spreading/sorting (e.g., const items = Array.isArray(result.Data) ? result.Data : []; const sorted = [...items].sort(...)), then call set({ feeds: sorted, isLoadingFeeds: false }) and keep the existing error handling that sets feedsError and logs via logger.error; ensure you reference the same state keys (feeds, isLoadingFeeds, feedsError) so callers relying on an empty array still work.src/api/dispatch/dispatch.ts-20-22 (1)
20-22:⚠️ Potential issue | 🟡 MinorRemove the unused
getSetUnitStatefunction.The search found zero callers of
getSetUnitStatein the codebase. All internal code imports and usesgetSetUnitStatusDatadirectly. Since this is an internal application with no external consumers, this unused wrapper should be removed entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/dispatch/dispatch.ts` around lines 20 - 22, Remove the unused wrapper function getSetUnitState: delete the exported async function getSetUnitState which simply forwards to getSetUnitStatusData, since there are no callers and internal code imports getSetUnitStatusData directly; ensure no external exports reference getSetUnitState and update any export lists or barrel files if necessary so only getSetUnitStatusData remains exported.src/lib/poi-utils.ts-95-101 (1)
95-101:⚠️ Potential issue | 🟡 MinorUntranslated user-visible fallback string.
The
`Type ${poiTypeId}`fallback at Line 98 will be rendered to users (it's used asgroup.titleindestination-poi-selector.tsxand elsewhere) but isn't translated. Since this is a pure utility and can't calluseTranslationdirectly, consider either:
- Accepting an optional
defaultTitle?: string(or formatter) parameter so callers can passt('pois.unknown_type', { id: poiTypeId }), or- Returning the raw
poiTypeIdand letting callers format the title.As per coding guidelines: "Wrap all user-visible text in
t()fromreact-i18nextfor internationalization."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/poi-utils.ts` around lines 95 - 101, The fallback title `Type ${poiTypeId}` used when getPoiTypeName returns falsy is user-visible and must be localized; update the utility that builds group objects (the code mapping [...groups.entries()] to { poiTypeId, title, items }) to accept an optional parameter (e.g. defaultTitle?: string | (id: string) => string) or a formatter function, and replace the hardcoded ``Type ${poiTypeId}`` with a call to that parameter (if provided) or else return the raw poiTypeId so callers can format it; then update callers such as destination-poi-selector.tsx to pass t('pois.unknown_type', { id: poiTypeId }) or a formatter that calls t(), ensuring all user-visible titles are produced by react-i18next rather than in this pure utility.src/stores/pois/store.ts-56-58 (1)
56-58:⚠️ Potential issue | 🟡 MinorHardcoded English error strings won't honor app locale.
The error messages stored in
state.error('Failed to fetch POI types','Failed to fetch destination POIs','Failed to fetch POIs','Failed to fetch POI') are raw English. If the UI rendersstate.errordirectly, non-English users will see English text. Prefer storing a translation key (or an error code) and let the consumer pass it throught().As per coding guidelines: "Ensure all text is wrapped in
t()fromreact-i18nextfor translations".Also applies to: 79-81, 107-108, 137-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/pois/store.ts` around lines 56 - 58, Replace hardcoded English error messages stored in state.error with translation keys or error codes so the UI can call t() and render localized text: update the four set(...) calls (the ones setting error to 'Failed to fetch POI types', 'Failed to fetch destination POIs', 'Failed to fetch POIs', and 'Failed to fetch POI') to assign keys like 'errors.fetch_poi_types', 'errors.fetch_destination_pois', 'errors.fetch_pois', 'errors.fetch_poi' (or numeric codes) and preserve isLoading/state behavior; ensure consumers read state.error and pass it through i18next's t() rather than assuming a literal message.src/stores/pois/store.ts-111-122 (1)
111-122:⚠️ Potential issue | 🟡 Minor
Number(poiId)silently producesNaNfor non-numeric input.If
poiIdarrives as a non-numeric string (e.g., a deep-link param),Number(poiId)becomesNaN, the cache lookup returnsundefined, andgetPoi(NaN)is dispatched to the API. Consider validating early and short-circuiting:🛡️ Suggested guard
fetchPoi: async (poiId: number | string, force = false) => { const normalizedPoiId = Number(poiId); + if (!Number.isFinite(normalizedPoiId)) { + set({ selectedPoi: null, error: 'Invalid POI id' }); + return null; + } const cachedPoi = get().poiDetails[normalizedPoiId];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/pois/store.ts` around lines 111 - 122, fetchPoi currently uses Number(poiId) which yields NaN for non-numeric input and then calls getPoi(NaN); update fetchPoi to validate normalizedPoiId after conversion (Number(poiId)) using Number.isFinite or !Number.isNaN and short-circuit if invalid by setting error (set({ error: new Error('Invalid poiId') })) and clearing isLoadingDetail, returning early (and do not call getPoi), or alternatively try a safe numeric parse (e.g., parseInt) before validation; refer to the fetchPoi function, the normalizedPoiId variable, the poiDetails cache lookup, and the getPoi call when implementing this guard so you never dispatch getPoi with NaN and you set selectedPoi/error state appropriately.src/stores/pois/store.ts-43-110 (1)
43-110:⚠️ Potential issue | 🟡 MinorTTL guard re-fetches forever when the API legitimately returns an empty list.
fetchPoiTypes,fetchDestinationPois, andfetchAllPoiDataall use<list>.length > 0as part of theisFreshcheck. If the backend returns[](e.g., the org has no POI types or no destination POIs),lastFetchedAtis updated but the next call still seeslength === 0→isFresh = false→ it re-fetches every time the consumer calls the action. Track freshness vialastFetchedAt(and/or a separate "ever fetched" flag) independently of list length.♻️ Suggested approach
- const { poiTypes, lastFetchedAt } = get(); - const isFresh = poiTypes.length > 0 && Date.now() - lastFetchedAt < STORE_TTL_MS; + const { lastFetchedAt } = get(); + const isFresh = lastFetchedAt > 0 && Date.now() - lastFetchedAt < STORE_TTL_MS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/pois/store.ts` around lines 43 - 110, The TTL guard currently uses "<list>.length > 0" so empty API results never mark the store as fresh; update fetchPoiTypes, fetchDestinationPois, and fetchAllPoiData to determine freshness only from lastFetchedAt (and whether it has ever been set) rather than list length — e.g., compute isFresh as lastFetchedAt > 0 && Date.now() - lastFetchedAt < STORE_TTL_MS; keep setting lastFetchedAt = Date.now() on every successful fetch (even when the returned arrays are empty) so subsequent calls respect the TTL; modify the isFresh checks in fetchPoiTypes, fetchDestinationPois, and fetchAllPoiData accordingly.src/app/(app)/index.tsx-67-75 (1)
67-75:⚠️ Potential issue | 🟡 MinorEdge case: dismissal won't reset if alert count is unchanged.
useEffectonextremeAlerts.lengthonly resetsisBannerDismissedwhen the array length changes. If one extreme alert is removed and another added in the same update tick (count unchanged), the previously dismissed banner stays hidden even though the alert set has changed. Consider keying off the alerts identity (e.g., a stable hash ofWeatherAlertIds) if showing the banner for newly arrived alerts is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(app)/index.tsx around lines 67 - 75, The reset effect currently depends on extremeAlerts.length so dismissal won't reset when the set changes but count stays the same; update the logic to derive a stable identity for the alert set (e.g., a memoized key computed from extremeAlerts' WeatherAlertId values — sort and join or hash the IDs) and change the useEffect dependency from extremeAlerts.length to that memoized key; in short, create a memo (e.g., alertsKey) from extremeAlerts.map(a => a.WeatherAlertId) and call setIsBannerDismissed(false) inside the useEffect that depends on alertsKey so the banner resets whenever the actual alerts change, not just when the count changes.src/app/call/[id].tsx-324-338 (1)
324-338:⚠️ Potential issue | 🟡 MinorMinor: extra leading whitespace flagged by linter.
Static analysis is reporting trailing whitespace/extra leading spaces on lines 324–329 (Prettier
Delete ·). Re-indent these to match the surrounding<Box>siblings (which use 14-space indentation, not 15) so CI lint passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/call/`[id].tsx around lines 324 - 338, The JSX block rendering the address/destination has one extra leading space on several lines causing a Prettier "Delete ·" lint failure; open the JSX around the Box/Text elements that reference call, destinationLabel, call.DestinationTypeName and call.DestinationAddress and re-indent the lines inside that conditional to match the surrounding siblings (use the same 14-space indentation used by other <Box> blocks) so there are no stray leading spaces; verify the indentation around the conditional render of destinationLabel and the nested Text that joins DestinationTypeName and DestinationAddress.src/app/call/[id].tsx-201-210 (1)
201-210:⚠️ Potential issue | 🟡 MinorAdd validation to prevent NaN from being passed to
startPolling.The
parseInt(call.CallId, 10)conversion is necessary sincecall.CallIdis a string andstartPollingexpects a number. However, ifCallIdis non-numeric (unlikely but possible),parseIntreturnsNaN, which is then passed tostartPollingandfetchTimerStatuseswith no validation. Add a guard to ensureCallIdis numeric before callingstartPolling:if (call?.CheckInTimersEnabled && call?.CallId) { const numericCallId = parseInt(call.CallId, 10); if (!isNaN(numericCallId)) { startPolling(numericCallId, 30000); } }The dependency array is correctly structured—it uses the extracted values (
call?.CheckInTimersEnabled,call?.CallId) rather than the object reference, so there is no polling restart thrashing when thecallobject is replaced with the same property values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/call/`[id].tsx around lines 201 - 210, The useEffect may pass NaN to startPolling when parseInt(call.CallId, 10) fails; update the effect around startPolling (in the block using startPolling, stopPolling, resetTimers) to first check call?.CallId exists, parse it to a number (e.g., numericCallId = parseInt(call.CallId, 10)) and only call startPolling(numericCallId, 30000) if !isNaN(numericCallId); keep the return cleanup calling stopPolling() and resetTimers() and leave the dependency array as-is.src/hooks/use-quick-check-in.ts-23-30 (1)
23-30:⚠️ Potential issue | 🟡 MinorUnsafe
UnitIdparsing —parseIntreturnsNaNfor non-numeric strings.
parseInt(activeUnit.UnitId, 10)will returnNaNifUnitIdis not strictly numeric (e.g., 'unit1', UUID, or alphanumeric). Test mocks in the codebase use this format (UnitId: 'unit1'), indicating IDs may not be purely numeric. NaN then propagates into the API payload and fails server-side validation or silently serializes as null.Either validate that
UnitIdis numeric before parsing with a guard likeNumber.isFinite(parsed), or changePerformCheckInInput.UnitIdto accept the raw string if the backend supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-quick-check-in.ts` around lines 23 - 30, The quickCheckIn function builds a PerformCheckInInput using parseInt(activeUnit.UnitId, 10) which can produce NaN for non-numeric IDs (e.g., 'unit1'), so update quickCheckIn to either validate and only use a numeric UnitId (parse -> Number.isFinite(parsed) and set UnitId to parsed when valid) or pass the raw string UnitId through if the backend accepts strings; adjust the CheckInType logic (activeUnit / CHECK_IN_TYPE_UNIT) accordingly so UnitId is only transformed when safe and otherwise assigned as the original activeUnit.UnitId string or omitted.src/components/check-in-timers/check-in-history-list.tsx-20-20 (1)
20-20:⚠️ Potential issue | 🟡 MinorHardcoded "Unit:" / "User:" labels bypass i18n.
These user-visible strings should be wrapped in
t()so they translate across the seven supported languages.🌐 Proposed fix
- <Text className="text-xs text-gray-500">{item.UnitId ? `Unit: ${item.UnitId}` : `User: ${item.UserId}`}</Text> + <Text className="text-xs text-gray-500"> + {item.UnitId ? t('check_in.unit_label', { id: item.UnitId }) : t('check_in.user_label', { id: item.UserId })} + </Text>Note:
twould need to be threaded intorenderItem(e.g., moverenderIteminside the component or accepttas a closure).As per coding guidelines: "Wrap all user-facing text in t() from react-i18next".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/check-in-timers/check-in-history-list.tsx` at line 20, The hardcoded labels in the renderItem output ("Unit:" / "User:") bypass i18n; update the render so those strings are wrapped in t() from react-i18next (e.g., t('Unit') and t('User')) and interpolate the id values, and make sure t is available to renderItem by either moving renderItem inside the component that calls useTranslation() or passing t into renderItem as a closure/prop; update the Text line that renders item.UnitId/item.UserId to use the translated label via t() and preserve the existing conditional logic using item.UnitId and item.UserId.src/components/check-in-timers/check-in-history-list.tsx-33-39 (1)
33-39:⚠️ Potential issue | 🟡 MinorEmpty state likely uses the wrong translation key.
check_in.historyis the "History" heading label (used elsewhere as a section title). Showing it centered in an empty list reads as just "History" rather than an empty-state message like "No check-in history". Use a dedicated empty-state key (e.g.,check_in.no_history).🛠️ Proposed fix
if (records.length === 0) { return ( <Box className="p-4"> - <Text className="text-center text-gray-500">{t('check_in.history')}</Text> + <Text className="text-center text-gray-500">{t('check_in.no_history')}</Text> </Box> ); }Add the corresponding key to
src/translations/*.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/check-in-timers/check-in-history-list.tsx` around lines 33 - 39, In check-in-history-list.tsx where the empty-state branch checks records.length === 0, replace the t('check_in.history') usage with a dedicated empty-state translation key such as t('check_in.no_history') and add that key/value to your translations JSON files (e.g., src/translations/*.json) for all supported locales; ensure keys are consistent across locales so the UI shows "No check-in history" (or localized equivalent) instead of the section title.src/components/check-in-timers/check-in-tab-content.tsx-66-71 (1)
66-71:⚠️ Potential issue | 🟡 MinorConfusing toggle label: heading and button both say "History".
When collapsed, the button reads
t('check_in.history')("History") right next to a heading also rendered ast('check_in.history'). Use a distinct show/hide label (e.g.,check_in.show_history/check_in.hide_history, or reusecommon.show/common.hide).🛠️ Proposed fix
- <Button variant="link" size="sm" onPress={() => setShowHistory(!showHistory)}> - <ButtonText>{showHistory ? t('common.close') : t('check_in.history')}</ButtonText> - </Button> + <Button variant="link" size="sm" onPress={() => setShowHistory((prev) => !prev)}> + <ButtonText>{showHistory ? t('check_in.hide_history') : t('check_in.show_history')}</ButtonText> + </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/check-in-timers/check-in-tab-content.tsx` around lines 66 - 71, The heading and toggle Button in check-in-tab-content.tsx both render t('check_in.history'), causing confusion; update the ButtonText (the Button with onPress={() => setShowHistory(!showHistory)}) to use distinct translation keys based on the showHistory state (e.g., t('check_in.hide_history') when showHistory is true and t('check_in.show_history') or t('common.show')/t('common.hide') when false) so the Heading (Heading size="sm") remains "History" while the Button clearly indicates show vs hide; keep the existing setShowHistory toggle behavior unchanged.src/components/check-in-timers/check-in-tab-content.tsx-63-63 (1)
63-63:⚠️ Potential issue | 🟡 MinorReplace non-scrollable
FlatListwith.map()for this small, bounded list.A non-scrollable
FlatListloses virtualization benefits and produces aVirtualizedList nested inside ScrollViewwarning becauseCheckInTabContentis rendered as tab content within the parentScrollViewin the call detail screen. For the typical 3-item dataset intimerStatuses, usetimerStatuses.map(...)instead, or if virtualization becomes necessary, lift the list to be the scroll container itself usingListHeaderComponentto wrap the surrounding chrome (button, history section, bottom sheet).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/check-in-timers/check-in-tab-content.tsx` at line 63, The FlatList usage in CheckInTabContent (the line using FlatList with data={timerStatuses}, renderItem={renderTimerCard}, keyExtractor={keyExtractor}, scrollEnabled={false}) should be replaced with a simple timerStatuses.map(...) render because the list is small and non-scrollable; change the component to render timerStatuses.map((item, index) => <container key={keyExtractor(item, index)}>{renderTimerCard({ item, index })}</container>) (or call renderTimerCard with the same shape it expects) and remove the FlatList-specific props (scrollEnabled, removeClippedSubviews, maxToRenderPerBatch) — if you later need virtualization, lift the list to be the scroll container using ListHeaderComponent instead.src/components/sidebar/check-in-sidebar-widget.tsx-28-29 (1)
28-29:⚠️ Potential issue | 🟡 MinorGuard
useQuickCheckInhook invocation to only whenactiveCallis defined.The hook is called unconditionally with
callId = 0whenactiveCallis falsy (lines 28–29), even though the component returns null immediately after ifactiveCallis not present (lines 32–34). Move the hook call inside the guard condition or check thatactiveCallexists before invoking it to avoid unnecessary hook registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/check-in-sidebar-widget.tsx` around lines 28 - 29, Currently useQuickCheckIn is invoked before we check activeCall; instead, move the activeCall guard up so the component returns null before any hooks are called. Concretely, check if (!activeCall) return null at the top of the component, then compute const callId = parseInt(activeCall.CallId, 10) and call const { quickCheckIn, isCheckingIn } = useQuickCheckIn(callId) after that guard; this keeps the hook invocation tied to an existing activeCall and avoids registering the hook when activeCall is absent.src/components/call-video-feeds/video-feed-form-sheet.tsx-156-181 (1)
156-181:⚠️ Potential issue | 🟡 MinorWire
Controller'sonBlurso RHF tracks touched/blurred state.The
Urlfield overridesonBlurto callhandleUrlBlur, but it never invokesfield.onBlurfromController, so react-hook-form never marks the field as touched. TheNamefield also hasrules={{ required: true }}but itsonBlurisn't wired either, and there's no error UI rendered for required violations — submitting an empty name silently fails validation with no user feedback.♻️ Proposed fix
- render={({ field: { onChange, value } }) => ( + render={({ field: { onChange, onBlur, value }, fieldState: { error } }) => ( <Box className="rounded-lg border border-outline-200 p-2"> - <TextInput value={value} onChangeText={onChange} placeholder={t('video_feeds.name')} /> + <TextInput value={value} onBlur={onBlur} onChangeText={onChange} placeholder={t('video_feeds.name')} /> </Box> + {error ? <Text className="text-xs text-red-500">{t('form.required')}</Text> : null} )}And for
Url:- render={({ field: { onChange, value } }) => ( + render={({ field: { onChange, onBlur, value } }) => ( <Box className="rounded-lg border border-outline-200 p-2"> - <TextInput value={value} onChangeText={onChange} onBlur={() => handleUrlBlur(value)} placeholder="https://" autoCapitalize="none" keyboardType="url" /> + <TextInput + value={value} + onChangeText={onChange} + onBlur={() => { onBlur(); handleUrlBlur(value); }} + placeholder="https://" + autoCapitalize="none" + keyboardType="url" + /> </Box> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/call-video-feeds/video-feed-form-sheet.tsx` around lines 156 - 181, The Controller render callbacks for the "Name" and "Url" fields are not calling the provided field.onBlur, so react-hook-form never marks them as touched and required validation feedback is not shown; update both Controller render functions (the ones with name "Name" and "Url") to call the controller's field.onBlur when the TextInput blurs (for "Url" call field.onBlur before/after invoking handleUrlBlur(value)), and add UI to render validation errors from formState.errors for these fields so required violations are visible to the user.src/components/routes/active-routes-list.tsx-81-83 (1)
81-83:⚠️ Potential issue | 🟡 MinorPull-to-refresh remounts the loader instead of refreshing the list.
if (isLoading) return <Loading />;runs on every refresh — including when the user triggersRefreshControl. TheFlatListunmounts mid-pull, theRefreshControlindicator disappears, and the user sees a full-screen spinner instead of an inline pull-refresh. Gate the early loader on first load only:♻️ Proposed fix
- if (isLoading) { + if (isLoading && routePlans.length === 0) { return <Loading text={t('routes.loading')} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/routes/active-routes-list.tsx` around lines 81 - 83, The current early return in ActiveRoutesList using if (isLoading) return <Loading /> causes the loader to remount on every RefreshControl pull; update the condition to only show the full-screen Loading on first load (e.g., when isLoading is true AND there are no items yet) so the FlatList and its RefreshControl remain mounted during pull-to-refresh. Locate the isLoading check and replace it with a gate like "isLoading && routes.length === 0" (or a similar "hasLoadedOnce" flag) so the inline refresh indicator is preserved while subsequent loads still show the spinner only when the list is empty.src/stores/weather-alerts/store.ts-47-71 (1)
47-71:⚠️ Potential issue | 🟡 MinorInitialization can NPE if
WeatherAlertSettingsDatais missing.
settingsResponse.Datais typed/treated as non-null at line 49 and line 52, but if the backend ever returnsnull/undefinedData(timeout, partial response, etc.) the access tosettings.WeatherAlertsEnabledwill throw and be swallowed by the catch, leavingisInitialized: truewith stale state. Add a defensive null check before readingWeatherAlertsEnabled.🛡️ Suggested fix
const settingsResponse = await getWeatherAlertSettings(); - const settings = settingsResponse.Data; + const settings = settingsResponse?.Data ?? null; set({ settings }); - if (settings.WeatherAlertsEnabled) { + if (settings?.WeatherAlertsEnabled) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/weather-alerts/store.ts` around lines 47 - 71, The code assumes settingsResponse.Data is non-null before accessing settings.WeatherAlertsEnabled; update the initializer (the async block that calls getWeatherAlertSettings) to first check if settingsResponse?.Data is present and valid (e.g., const settings = settingsResponse?.Data ?? null) and only call settings.WeatherAlertsEnabled when settings is non-null; if Data is missing, call set({ error: 'Failed to initialize weather alerts', isLoading: false, isInitialized: false }) or provide a safe default settings object, and avoid setting isInitialized: true or touching alerts until you successfully fetch/validate settings; apply the same defensive null-check pattern around settingsResponse.Data and anywhere settings is used before calling getActiveAlerts or sortAlertsBySeverity to prevent NPEs.src/lib/weather-alert-utils.ts-51-88 (1)
51-88:⚠️ Potential issue | 🟡 MinorPolygon parser does not validate parsed JSON shape or numeric coordinates.
Two related robustness issues in
parsePolygonGeoJSON:
JSON.parse(polygonStr)can legitimately returnnull(e.g., the input string is"null"), at which pointparsed.typethrows — not caught by the outertry, since this is the outer try. It will fall through, but the same risk applies ifparsedis a primitive number/string with no.typereading boolean false; thennullis returned correctly. However anullparsed value will throw a TypeError on property access, which is caught — so behavior is "fall through to coordinate parsing" by accident. Add an explicit object check.- The coordinate-pair fallback uses
Number(...)without validating finiteness. Inputs like"abc,def"produce[NaN, NaN]pairs that pass thelength < 3check and are emitted as a Polygon Feature withNaNcoordinates. The closing-vertex checkfirst[0] !== last[0]always evaluates totrueforNaN, so a duplicateNaNvertex is appended. Downstream Mapbox rendering will silently fail or warn.🛡️ Suggested hardening
try { // Try parsing as GeoJSON first const parsed = JSON.parse(polygonStr); + if (!parsed || typeof parsed !== 'object') return null; if (parsed.type === 'Feature') return parsed; if (parsed.type === 'Polygon' || parsed.type === 'MultiPolygon') { return { type: 'Feature', properties: {}, geometry: parsed }; } return null; } catch { // Try parsing as coordinate pairs "lat,lng lat,lng ..." try { - const coords = polygonStr + const coords: number[][] = polygonStr .trim() .split(/\s+/) .map((pair) => { const [lat, lng] = pair.split(',').map(Number); return [lng, lat]; - }); + }) + .filter(([lng, lat]) => Number.isFinite(lng) && Number.isFinite(lat)); if (coords.length < 3) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/weather-alert-utils.ts` around lines 51 - 88, The parser parsePolygonGeoJSON should explicitly guard against non-object JSON results and validate numeric coordinates: after JSON.parse(polygonStr) verify parsed is an object (not null) and that parsed.type is a string before accessing it; for the coordinate-pairs fallback, split each pair into exactly two parts, convert with Number and require Number.isFinite(lat) && Number.isFinite(lng), discard any invalid pairs, ensure at least 3 valid coordinate pairs remain, and use numeric comparisons (not relying on !== which fails for NaN) to decide whether to close the ring (compare first[0] === last[0] && first[1] === last[1]); only then build and return the Feature/Polygon.src/models/v4/unitStatus/unitStatusResultData.ts-10-19 (1)
10-19:⚠️ Potential issue | 🟡 MinorInconsistent
GroupIdtype breaks downstream comparisons.
GroupId(and similar fields) now acceptsnumber | stringinunitStatusResultData, inconsistent with the rest of the codebase whereGroupIdis uniformlystring. This creates silent equality bugs: at line 318 instatus-bottom-sheet.tsx,stationId: stringis compared againstitem.GroupId: number | stringusing===, which will fail if types mismatch (e.g.,"123" === 123returns false).Align the type with the established pattern: use
stringexclusively to matchunitResultData,groupsResultData, and other models across the app. This eliminates type coercion risk for all consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/v4/unitStatus/unitStatusResultData.ts` around lines 10 - 19, The GroupId field in unitStatusResultData is typed as number | string which breaks strict comparisons (e.g., stationId: string === item.GroupId); change GroupId's type to string (remove number union) in the unitStatusResultData model so it matches unitResultData and groupsResultData, then run TS checks and update any call sites that assumed numeric GroupId (or explicitly cast there) — look for the GroupId property in the unitStatusResultData definition and the comparison in status-bottom-sheet.tsx to verify compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad6024d0-446c-4ec7-92ea-6e2ef886e147
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (121)
app.config.tspackage.jsonplugins/withCheckInLiveActivity.jssrc/api/call-video-feeds/call-video-feeds.tssrc/api/calls/calls.tssrc/api/check-in-timers/check-in-timers.tssrc/api/dispatch/dispatch.tssrc/api/mapping/mapping.tssrc/api/weather-alerts/weather-alerts.tssrc/app/(app)/_layout.tsxsrc/app/(app)/index.tsxsrc/app/(app)/routes.tsxsrc/app/(app)/weather-alerts.tsxsrc/app/call/[id].tsxsrc/app/call/[id]/edit.tsxsrc/app/call/__tests__/[id].security.test.tsxsrc/app/call/__tests__/[id].test.tsxsrc/app/call/new/index.tsxsrc/app/routes/_layout.tsxsrc/app/routes/history/instance/[id].tsxsrc/app/routes/index.tsxsrc/app/routes/poi/[id].tsxsrc/app/weather-alert/[id].tsxsrc/components/call-video-feeds/__tests__/video-feed-card.test.tsxsrc/components/call-video-feeds/__tests__/video-feed-tab-content.test.tsxsrc/components/call-video-feeds/feed-format-utils.tssrc/components/call-video-feeds/video-feed-card.tsxsrc/components/call-video-feeds/video-feed-form-sheet.tsxsrc/components/call-video-feeds/video-feed-tab-content.tsxsrc/components/call-video-feeds/video-player-modal.tsxsrc/components/calls/call-card.tsxsrc/components/calls/destination-poi-selector.tsxsrc/components/check-in-timers/__tests__/check-in-bottom-sheet.test.tsxsrc/components/check-in-timers/__tests__/check-in-timer-card.test.tsxsrc/components/check-in-timers/check-in-bottom-sheet.tsxsrc/components/check-in-timers/check-in-history-list.tsxsrc/components/check-in-timers/check-in-tab-content.tsxsrc/components/check-in-timers/check-in-timer-card.tsxsrc/components/maps/__tests__/pin-actions.test.tsxsrc/components/maps/pin-detail-modal.tsxsrc/components/routes/active-routes-list.tsxsrc/components/routes/poi-card.tsxsrc/components/routes/poi-list-content.tsxsrc/components/routes/routes-home.tsxsrc/components/sidebar/__tests__/call-sidebar.test.tsxsrc/components/sidebar/check-in-sidebar-widget.tsxsrc/components/sidebar/sidebar-content.tsxsrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/components/status/__tests__/status-gps-debug.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/weather-alerts/__tests__/weather-alert-banner.test.tsxsrc/components/weather-alerts/__tests__/weather-alert-card.test.tsxsrc/components/weather-alerts/severity-filter-tabs.tsxsrc/components/weather-alerts/weather-alert-banner.tsxsrc/components/weather-alerts/weather-alert-card.tsxsrc/components/weather-alerts/weather-alert-detail-map.tsxsrc/hooks/__tests__/use-quick-check-in.test.tssrc/hooks/use-check-in-timer-polling.tssrc/hooks/use-quick-check-in.tssrc/lib/__tests__/poi-utils.test.tssrc/lib/native-modules/check-in-live-activity.tssrc/lib/poi-utils.tssrc/lib/weather-alert-utils.tssrc/models/offline-queue/queued-event.tssrc/models/v4/callVideoFeeds/callVideoFeedResult.tssrc/models/v4/callVideoFeeds/callVideoFeedResultData.tssrc/models/v4/callVideoFeeds/saveCallVideoFeedResult.tssrc/models/v4/calls/callResultData.tssrc/models/v4/checkIn/checkInRecordResult.tssrc/models/v4/checkIn/checkInRecordResultData.tssrc/models/v4/checkIn/checkInTimerStatusResult.tssrc/models/v4/checkIn/checkInTimerStatusResultData.tssrc/models/v4/checkIn/performCheckInResult.tssrc/models/v4/checkIn/resolvedCheckInTimerResult.tssrc/models/v4/checkIn/resolvedCheckInTimerResultData.tssrc/models/v4/customStatuses/customStateDetailTypes.tssrc/models/v4/destinations/destinationEntityTypes.tssrc/models/v4/dispatch/getSetUnitStateResultData.tssrc/models/v4/dispatch/newCallFormResultData.tssrc/models/v4/mapping/getMapDataAndMarkersData.tssrc/models/v4/mapping/poiResultData.tssrc/models/v4/mapping/poiResults.tssrc/models/v4/personnelStatuses/getCurrentStatusResultData.tssrc/models/v4/unitStatus/saveUnitStatusInput.tssrc/models/v4/unitStatus/unitStatusResultData.tssrc/models/v4/weatherAlerts/activeWeatherAlertsResult.tssrc/models/v4/weatherAlerts/weatherAlertEnums.tssrc/models/v4/weatherAlerts/weatherAlertResult.tssrc/models/v4/weatherAlerts/weatherAlertResultData.tssrc/models/v4/weatherAlerts/weatherAlertSettingsData.tssrc/models/v4/weatherAlerts/weatherAlertSettingsResult.tssrc/models/v4/weatherAlerts/weatherAlertZoneResultData.tssrc/models/v4/weatherAlerts/weatherAlertZonesResult.tssrc/services/__tests__/offline-event-manager-gps.test.tssrc/services/__tests__/offline-event-manager.service.test.tssrc/services/__tests__/push-notification.test.tssrc/services/check-in-notification.service.tssrc/services/offline-event-manager.service.tssrc/services/push-notification.tssrc/stores/call-video-feeds/__tests__/store.test.tssrc/stores/call-video-feeds/store.tssrc/stores/calls/store.tssrc/stores/check-in-timers/__tests__/store.test.tssrc/stores/check-in-timers/store.tssrc/stores/pois/store.tssrc/stores/signalr/signalr-store.tssrc/stores/status/__tests__/store.test.tssrc/stores/status/store.tssrc/stores/weather-alerts/__tests__/store.test.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
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/translations/sv.json (1)
240-292:⚠️ Potential issue | 🟠 MajorMany new POI/destination keys are left in English in the Swedish dictionary.
These keys are placed under
calls,map,routes,status, andcheck_inbut their values are English strings, e.g.,"destination_poi": "Destination POI","loading_destination_pois": "Loading destination POIs...","no_destination_pois_available": "No destination POIs available","select_destination_poi": "Select Destination POI","pin_address": "Address","pin_note": "Note","pin_type": "POI Type","view_poi_details": "View POI Details","failed_to_open_poi_maps": "Failed to open maps for this POI","loading_poi": "Loading POI...","loading_pois": "Loading POIs...","no_pois": "No POIs","no_pois_description","no_pois_filtered_description","poi_*"keys at lines 710–724,"route_to_poi","routes_tab","search_pois","set_poi_destination","view_on_map", the"status.*pois*"keys at lines 866–891, and"check_in.notification_channel_*"at lines 328–329. Swedish users will see a mix of Swedish and English copy. Either translate them now or open a follow-up to do so before release; otherwise the locale silently degrades.Want me to draft Swedish translations for these strings as a follow-up patch?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/translations/sv.json` around lines 240 - 292, The Swedish locale contains many untranslated POI/destination strings—update the keys such as "destination_poi", "loading_destination_pois", "no_destination_pois_available", "select_destination_poi", "pin_address", "pin_note", "pin_type", "view_poi_details", "failed_to_open_poi_maps", "loading_poi", "loading_pois", "no_pois", "no_pois_description", "no_pois_filtered_description", the "poi_*" keys, "route_to_poi", "routes_tab", "search_pois", "set_poi_destination", "view_on_map", the "status.*pois*" keys and "check_in.notification_channel_*" to Swedish equivalents (keep phrasing consistent with existing keys like "destination": "Destination" and "no_files": "Inga filer tillgängliga"), ensuring pluralization and placeholders match the English originals; either translate them inline in src/translations/sv.json or create a follow-up patch containing Swedish translations for all these keys so the locale is fully localized.src/translations/es.json (1)
147-902:⚠️ Potential issue | 🟡 MinorMany newly added Spanish translation keys still contain English values.
In this Spanish locale file, a sizeable portion of the new POI / destination / weather-related keys are left in English, which will surface untranslated UI to Spanish users:
call_detail.destination(147),calls.destination(240),calls.destination_poi(241),calls.destination_poi_none(242),calls.loading_destination_pois(260),calls.no_destination_pois_available(272),calls.select_destination_poi(292)check_in.notification_channel_description(328),check_in.notification_channel_name(329)map.pin_address(540),map.pin_note(542),map.pin_type(543),map.view_poi_details(547)routes.*:failed_to_open_poi_maps(676),loading_poi(683),loading_pois(684),no_pois(695),no_pois_description(696),no_pois_filtered_description(697),poi_address(710),poi_coordinates(711),poi_destination_enabled(714),poi_filter_all_types(715),poi_filter_placeholder(716),poi_not_found(717),poi_not_found_description(718),poi_note(719),poi_sort_display(720),poi_sort_placeholder(721),poi_sort_type(722),poi_type_unknown(723),pois_tab(724),route_to_poi(735),routes_tab(736),search_pois(739),set_poi_destination(741),view_on_map(770)status.*:calls_and_pois_destinations_enabled(866),calls_stations_pois_destinations_enabled(867),loading_pois(871),no_pois_available(874),poi_destination_enabled(880),pois_tab(881),stations_and_pois_destinations_enabled(890)Please provide proper Spanish translations for these keys (and audit other locale files in this PR for the same gap).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/translations/es.json` around lines 147 - 902, Many newly added Spanish translation keys remain in English; update the Spanish locale JSON to provide proper Spanish translations for the listed keys (e.g., call_detail.destination, calls.destination, calls.destination_poi, calls.destination_poi_none, calls.loading_destination_pois, calls.no_destination_pois_available, calls.select_destination_poi, check_in.notification_channel_description, check_in.notification_channel_name, map.pin_address, map.pin_note, map.pin_type, map.view_poi_details, routes.failed_to_open_poi_maps, routes.loading_poi, routes.loading_pois, routes.no_pois, routes.no_pois_description, routes.no_pois_filtered_description, routes.poi_address, routes.poi_coordinates, routes.poi_destination_enabled, routes.poi_filter_all_types, routes.poi_filter_placeholder, routes.poi_not_found, routes.poi_not_found_description, routes.poi_note, routes.poi_sort_display, routes.poi_sort_placeholder, routes.poi_sort_type, routes.poi_type_unknown, routes.pois_tab, routes.route_to_poi, routes.routes_tab, routes.search_pois, routes.set_poi_destination, routes.view_on_map, status.calls_and_pois_destinations_enabled, status.calls_stations_pois_destinations_enabled, status.loading_pois, status.no_pois_available, status.poi_destination_enabled, status.pois_tab, status.stations_and_pois_destinations_enabled) by replacing the English strings with accurate Spanish equivalents and ensure pluralization/variables remain intact; after updating these keys, scan other changed locale files in the PR for similar untranslated English strings and apply equivalent translations.src/translations/pl.json (1)
147-292:⚠️ Potential issue | 🟠 MajorSeveral newly added keys are not translated to Polish.
Same issue as in
fr.json: many of the new POI/check-in/routes/map/status keys retain their English source text in this Polish dictionary. Polish users will see English in the corresponding flows. Untranslated keys include:
call_detail.destination(line 147) — should likely be "Cel" / "Miejsce docelowe"calls.destination,calls.destination_poi,calls.destination_poi_none,calls.loading_destination_pois,calls.no_destination_pois_available,calls.select_destination_poi(lines 240-242, 260, 272, 292)map.pin_address,map.pin_note,map.pin_type,map.view_poi_details(lines 540, 542, 543, 547)- All
routes.*poi*androutes.view_on_map/routes.routes_tab/routes.failed_to_open_poi_maps(lines 676, 683-684, 695-697, 710-724, 735-736, 739, 741, 770)status.calls_and_pois_destinations_enabled,status.calls_stations_pois_destinations_enabled,status.loading_pois,status.no_pois_available,status.poi_destination_enabled,status.pois_tab,status.stations_and_pois_destinations_enabled(lines 866, 867, 871, 874, 880, 881, 890)check_in.notification_channel_description,check_in.notification_channel_name(lines 328-329)Please provide Polish translations before release.
I'm happy to draft Polish translations for these keys if helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/translations/pl.json` around lines 147 - 292, Several new i18n keys in the Polish translations file are still in English; update the values for keys such as call_detail.destination, calls.destination, calls.destination_poi, calls.destination_poi_none, calls.loading_destination_pois, calls.no_destination_pois_available, calls.select_destination_poi, map.pin_address, map.pin_note, map.pin_type, map.view_poi_details, all routes.*poi* keys including routes.view_on_map, routes.routes_tab, routes.failed_to_open_poi_maps, status.* keys (e.g. status.calls_and_pois_destinations_enabled, status.calls_stations_pois_destinations_enabled, status.loading_pois, status.no_pois_available, status.poi_destination_enabled, status.pois_tab, status.stations_and_pois_destinations_enabled) and check_in.notification_channel_description/name — replace the English strings with correct Polish translations (e.g., "destination" -> "Cel" or "Miejsce docelowe", "No destination selected" -> "Brak wybranego miejsca docelowego", etc.), ensuring pluralization/context matches other nearby keys and keeping JSON key names unchanged.src/translations/fr.json (1)
240-292:⚠️ Potential issue | 🟠 MajorSeveral newly added keys are not translated to French.
A large portion of the new POI/check-in/routes/map/status keys still hold their English source text in this French dictionary. French users will see English UI in the corresponding flows. Notable untranslated values:
calls.destination_poi,calls.destination_poi_none,calls.loading_destination_pois,calls.no_destination_pois_available,calls.select_destination_poi(lines 241, 242, 260, 272, 292)map.pin_address,map.pin_note,map.pin_type,map.view_poi_details(lines 540, 542, 543, 547)routes.failed_to_open_poi_maps,routes.loading_poi,routes.loading_pois,routes.no_pois*,routes.poi_address,routes.poi_coordinates*,routes.poi_destination_enabled,routes.poi_filter_*,routes.poi_not_found*,routes.poi_note,routes.poi_sort_*,routes.poi_type_unknown,routes.pois_tab,routes.route_to_poi,routes.routes_tab,routes.search_pois,routes.set_poi_destination,routes.view_on_map(lines 676, 683-684, 695-697, 710-724, 735-736, 739, 741, 770)status.calls_and_pois_destinations_enabled,status.calls_stations_pois_destinations_enabled,status.loading_pois,status.no_pois_available,status.poi_destination_enabled,status.pois_tab,status.stations_and_pois_destinations_enabled(lines 866, 867, 871, 874, 880, 881, 890)check_in.notification_channel_description,check_in.notification_channel_name(lines 328-329)Please replace these with French translations before release.
I'm happy to draft French translations for these keys if helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/translations/fr.json` around lines 240 - 292, Multiple newly added translation keys in the French JSON (e.g., calls.destination_poi, calls.destination_poi_none, calls.loading_destination_pois, calls.no_destination_pois_available, calls.select_destination_poi, map.pin_address, map.pin_note, map.pin_type, map.view_poi_details, routes.* keys like routes.loading_pois/routes.no_pois*/routes.poi_address/etc., status.* keys like status.loading_pois/status.no_pois_available, and check_in.notification_channel_name/description) are still in English; replace each English value with the appropriate French translation string in src/translations/fr.json so the UI is fully localized — update the values for the listed keys (and other POI/check-in/routes/map/status keys mentioned in the review) to French equivalents, keeping the key names unchanged and matching existing translation style and punctuation.
♻️ Duplicate comments (2)
src/translations/ar.json (1)
240-292:⚠️ Potential issue | 🟠 MajorSame untranslated-strings issue as
sv.json— many POI/destination/notification keys are English-only inar.json.Identical pattern to the Swedish file:
calls.destination*(lines 240–242, 260, 272, 292),call_detail.destination(line 147),check_in.notification_channel_*(lines 328–329),map.pin_*andmap.view_poi_details(lines 540–547),routes.failed_to_open_poi_maps/loading_poi*/no_pois*/poi_*/route_to_poi/routes_tab/search_pois/set_poi_destination/view_on_map(lines 676–770), andstatus.*pois*/status.poi_destination_enabled/status.stations_and_pois_destinations_enabled(lines 866–890) all carry English values in the Arabic dictionary. RTL Arabic users will see English mixed in. Either translate now or track in a follow-up before release.src/services/check-in-notification.service.ts (1)
85-91:⚠️ Potential issue | 🟡 MinorHardcoded English literals still leak into the notification body.
The past i18n round-trip moved most strings into
NotificationLabels, but two English fragments remain user-visible:
- Line 91:
` - ${timeStr} remaining [${statusLabel}]`hardcodes"remaining"and the bracket framing.- Line 85:
?? 'Check In'fallback is English.For a Spanish/French user, the body will render e.g.
"PAR - 2:30 remaining [OK]"instead of"PAR - 2:30 restante [OK]". Either addremainingSuffix(and optionallybodyFormat) toNotificationLabelsand have the polling hook supply translated strings, or pass the entire pre-formatted body in. Also drop the English fallback oncelabelsis required (it already is instartNotification).♻️ Suggested fix
export interface NotificationLabels { statusLabels: Record<string, string>; channelName: string; channelDescription: string; actionText: string; + remainingSuffix: string; // e.g. t('check_in.remaining') }- const statusLabel = this.currentLabels?.statusLabels[this.currentStatus] ?? this.currentStatus; - const actionText = this.currentLabels?.actionText ?? 'Check In'; + const statusLabel = this.currentLabels?.statusLabels[this.currentStatus] ?? this.currentStatus; + const actionText = this.currentLabels?.actionText ?? ''; + const remaining = this.currentLabels?.remainingSuffix ?? '';- body: `${timerName} - ${timeStr} remaining [${statusLabel}]`, + body: `${timerName} - ${timeStr} ${remaining} [${statusLabel}]`,As per coding guidelines: "Wrap all user-facing text in
t()fromreact-i18nextfor internationalization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/check-in-notification.service.ts` around lines 85 - 91, The notification body still contains hardcoded English fragments: remove the English fallback ('Check In') from this.currentLabels?.actionText and stop assembling the string with the literal "remaining" and brackets inside the notifee.displayNotification call; instead extend NotificationLabels to include a translated remainingSuffix (or a full bodyFormat/bodyText) and have the polling hook / startNotification supply the fully localized pieces (e.g., currentLabels.actionText and currentLabels.remainingSuffix or currentLabels.bodyFormat) so that notifee.displayNotification uses only values from this.currentLabels (and drop the English fallback since labels are required).
🧹 Nitpick comments (14)
src/components/check-in-timers/check-in-timer-card.tsx (2)
62-92: Hardcoded gray/white colors won't adapt to dark mode.
text-gray-500,bg-gray-200, and the inlinecolor: 'white'onButtonTextare fixed in the light palette. Per coding guidelines, components must "Support both dark mode and light mode in styling". Prefer the gluestack tokens (e.g.,text-typography-500,bg-background-100) so the card renders correctly under the system theme — same fix should be applied to the inlinestyle={{ color: statusColor }}chip text wherestatusColor + '20'is used as a background tint (which is fine, but pair it with token-based foregrounds when status colors aren't applicable).As per coding guidelines: "Support both dark mode and light mode in styling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/check-in-timers/check-in-timer-card.tsx` around lines 62 - 92, The card uses hardcoded light-mode classes and inline colors (e.g., "text-gray-500", "bg-gray-200", and inline style color: statusColor on the chip text), which breaks dark-mode support; update all hardcoded color tokens in the CheckInTimerCard render (components: Box, Text, HStack, the progress bar Box, and the chip using statusColor + '20') to use gluestack theme tokens (e.g., replace "text-gray-500" with "text-typography-500", "bg-gray-200" with "bg-background-100", and use a token-based foreground for the chip text instead of a hardcoded inline color), keep statusColor only for the tint background but pair it with a theme foreground token for accessibility and dark-mode compatibility (look for usages of timer.TargetName, timer.DurationMinutes, safeStatusLower, minutesAgo, localElapsed to locate the JSX block).
25-40: WrapCheckInTimerCardinReact.memoand consider one shared ticker for the list.Two related performance points when this card is rendered in a list (
check-in-tab-content.tsx):
- The component is not memoized, so any state change in the parent re-creates
onCheckInand re-runs the render, even thoughtimeris unchanged. As per coding guidelines,React.memo()should be used "for components with static props to prevent unnecessary re-renders" — this is a textbook fit.- Each card runs its own
setInterval(... 1000)(line 36–38). With N timers visible, you get N JS-thread timers per second just to do a+1/60increment. Lifting the ticker to the parent (or the store) and passinglocalElapseddown would let one tick update the whole list.♻️ Minimal memoization
-export const CheckInTimerCard: React.FC<CheckInTimerCardProps> = ({ timer, onCheckIn, showCheckInButton = true }) => { +const CheckInTimerCardComponent: React.FC<CheckInTimerCardProps> = ({ timer, onCheckIn, showCheckInButton = true }) => { ... -}; +}; + +export const CheckInTimerCard = React.memo(CheckInTimerCardComponent);As per coding guidelines: "Use
React.memo()for components with static props to optimize performance".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/check-in-timers/check-in-timer-card.tsx` around lines 25 - 40, The CheckInTimerCard component creates a per-instance setInterval and is not memoized, causing N timers and unnecessary re-renders; wrap the component export (CheckInTimerCard) in React.memo to avoid re-renders when props like timer haven't changed, and remove the internal ticker useEffect (the setInterval that updates localElapsed) so the component consumes a single shared ticker value passed from the parent/store (e.g., accept a prop like sharedElapsed or subscribe to a sharedTicker hook) and use that to derive displayed elapsed time instead of setLocalElapsed; keep the existing props (timer, onCheckIn, showCheckInButton) and ensure onCheckIn is stable (useCallback in parent) so memoization is effective.src/lib/weather-alert-utils.ts (1)
90-100:parseCenterLocation: deadtry/catchand missing range validation.Neither
String.prototype.splitnorNumber(...)throws, so thetry/catchhere is unreachable — the only failure mode the function actually has is theisNaNbranch. Also, lat/lon should be validated against their valid ranges (lat ∈ [-90, 90], lon ∈ [-180, 180]); otherwise pathological strings like"500,1000"parse as a "valid" location.♻️ Suggested fix
-export const parseCenterLocation = (centerStr: string): { latitude: number; longitude: number } | null => { - if (!centerStr) return null; - - try { - const [lat, lng] = centerStr.split(',').map(Number); - if (isNaN(lat) || isNaN(lng)) return null; - return { latitude: lat, longitude: lng }; - } catch { - return null; - } -}; +export const parseCenterLocation = (centerStr: string): { latitude: number; longitude: number } | null => { + if (!centerStr) return null; + const [lat, lng] = centerStr.split(',').map(Number); + if (!Number.isFinite(lat) || !Number.isFinite(lng)) return null; + if (lat < -90 || lat > 90 || lng < -180 || lng > 180) return null; + return { latitude: lat, longitude: lng }; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/weather-alert-utils.ts` around lines 90 - 100, The parseCenterLocation function currently has an unnecessary try/catch and lacks lat/lon range checks; remove the dead try/catch and instead validate that centerStr splits into exactly two numeric parts and that latitude ∈ [-90,90] and longitude ∈ [-180,180] (return null if any check fails). Locate parseCenterLocation and replace the try/catch block with explicit checks: split on ',', ensure two elements, use Number to parse, check !isNaN for both, then verify the numeric ranges before returning { latitude, longitude } or null on failure.src/stores/calls/store.ts (1)
100-108: Side-effect on sharedcallPriorities/callTypesslices.
fetchCallFormDataoverwrites the existingcallPrioritiesandcallTypesslices that are independently fetched byfetchCallPriorities/fetchCallTypes(and used elsewhere in the calls store). IfgetNewCallData()ever returns a smaller/older subset forPriorities/CallTypesthan the dedicated endpoints (or returns them in a different shape), other screens consuming these slices will silently regress until the next dedicated fetch.If the new-call form is the only consumer of these refreshed slices, consider isolating the form data into its own keys (e.g.,
callFormPriorities,callFormCallTypes) — or document thatgetNewCallDatais the canonical source and remove the now-duplicatedfetchCallTypeslength-based guard at lines 78–82 to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/calls/store.ts` around lines 100 - 108, fetchCallFormData currently overwrites the shared slices callPriorities and callTypes fetched independently by fetchCallPriorities/fetchCallTypes, which can cause silent regressions; change fetchCallFormData to store its values under isolated keys (e.g., callFormPriorities, callFormCallTypes, and set isCallFormDataLoaded) instead of writing callPriorities/callTypes, or alternatively remove the duplicated writes and rely on getNewCallData as the canonical source by deleting the assignments to callPriorities/callTypes in fetchCallFormData; update all references to these form-specific values to use the new callForm* keys and keep fetchCallPriorities/fetchCallTypes behavior unchanged.src/models/v4/mapping/getMapDataAndMarkersData.ts (1)
21-27:PoiTypeId?: number | nullis the onlynull-permitting field in this model.Every other optional field uses
field?: T = ''/0semantics (omit means "not provided"), butPoiTypeIdadds a third state vianull. Unless the API truly distinguishes "missing" vs "explicitly null" for this property, consider dropping the| nullso that consumers only need to handle one absence form (undefined). Otherwise nullish checks must use!= nulleverywhere instead of a simple optional check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/v4/mapping/getMapDataAndMarkersData.ts` around lines 21 - 27, The model field PoiTypeId currently allows a third state via "number | null" which is inconsistent with other optional fields; change the declaration in getMapDataAndMarkersData.ts from "PoiTypeId?: number | null = null" to a single optional number (e.g. "PoiTypeId?: number = 0") so consumers only handle undefined as absent, and update any call sites that relied on explicit null checks to use the simple optional/undefined check for PoiTypeId.plugins/withCheckInLiveActivity.js (2)
366-368: Idempotency check may miss repeat runs because group/file refs are still added before the target check.The early-return at line 366–368 only protects against re-creating the target, but earlier in the callback the Swift sources were already written to disk in Step 3 (and on subsequent prebuild runs the target check returns early before any project mutation). That part is fine. However, the file-on-disk write in Step 3 always overwrites the generated Swift sources unconditionally — if a future maintainer hand-edits any of the generated
.swift/.mfiles inios/, the next prebuild silently clobbers them. Worth at minimum a code comment stating that these files are plugin-owned and any edits must be made in the plugin templates, not inios/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/withCheckInLiveActivity.js` around lines 366 - 368, The plugin currently returns early using project.pbxTargetByName(WIDGET_NAME) but only after the Swift source files were already unconditionally written to disk, which allows prebuild to silently clobber any manual edits; fix by moving the target/idempotency check (the project.pbxTargetByName(WIDGET_NAME) / project.pbxTargetByName(`"${WIDGET_NAME}"`)) to before the Step 3 write of Swift sources so writes are skipped when the target already exists, or make the Step 3 writer conditional: only create files if they don't exist (check fs.existsSync) or write to a plugin-owned path and skip overwriting, and also add an explicit code comment near the Swift/M file generation function (referencing WIDGET_NAME and the Swift source write block) stating these generated files are plugin-owned and must not be edited in ios/ so maintainers understand the ownership and idempotency behavior.
195-200:Activity.request(attributes:contentState:)is deprecated since iOS 16.2.The deprecated initializer still works for the iOS 16.1+ deployment target, but it triggers a deprecation warning. The recommended path on iOS 16.2+ is
Activity.request(attributes:content:pushType:)usingActivityContent(state:staleDate:). Since you're generating this code from a plugin, you can keep iOS 16.1 minimum and gate the new API with#available(iOS 16.2, *)to avoid future-removal risk.♻️ Suggested update
- do { - let _ = try Activity.request(attributes: attributes, contentState: state) - resolve(true) - } catch { - reject("LIVE_ACTIVITY_ERROR", error.localizedDescription, error) - } + do { + if `#available`(iOS 16.2, *) { + let content = ActivityContent(state: state, staleDate: nil) + _ = try Activity.request(attributes: attributes, content: content, pushType: nil) + } else { + _ = try Activity.request(attributes: attributes, contentState: state) + } + resolve(true) + } catch { + reject("LIVE_ACTIVITY_ERROR", error.localizedDescription, error) + }A similar treatment applies to
activity.update(using:)on line 215. Replace with a conditional that usesActivityContent(state:staleDate:)andactivity.update(_:)on iOS 16.2+.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/withCheckInLiveActivity.js` around lines 195 - 200, Replace the deprecated Activity.request(attributes:contentState:) call with a runtime-gated implementation: if `#available`(iOS 16.2, *) call Activity.request(attributes:content:pushType:) using an ActivityContent constructed via ActivityContent(state:staleDate:), otherwise fall back to the existing Activity.request(attributes:contentState:) path; apply the same pattern for activity.update(using:)—use activity.update(_:) with ActivityContent(state:staleDate:) when `#available`(iOS 16.2, *), else keep activity.update(using:) for older OS versions, ensuring you reference the existing Activity.request(attributes:contentState:) and activity.update(using:) sites when making the change.src/app/weather-alert/[id].tsx (2)
23-23: UnuseduseRouterimport/binding.
routeris declared at line 23 but never referenced in the component. Remove it (and theuseRouterimport) to keep the file clean.♻️ Suggested change
-import { Stack, useLocalSearchParams, useRouter } from 'expo-router'; +import { Stack, useLocalSearchParams } from 'expo-router';- const { id } = useLocalSearchParams(); - const alertId = Array.isArray(id) ? id[0] : id; - const router = useRouter(); - const { t } = useTranslation(); + const { id } = useLocalSearchParams(); + const alertId = Array.isArray(id) ? id[0] : id; + const { t } = useTranslation();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/weather-alert/`[id].tsx at line 23, Remove the unused router binding by deleting the useRouter import and the line that declares const router = useRouter(); (remove references to useRouter and the local identifier router in the component) so there are no unused imports/variables; search for useRouter and the const router = useRouter() occurrence in the component (e.g., in the [id].tsx file) and remove both the import and the declaration.
155-162: Brittle enum-to-translation mapping via hardcoded ordered arrays.
['immediate', 'expected', 'future', 'past', 'unknown'][alert.Urgency]and['observed', 'likely', 'possible', 'unlikely', 'unknown'][alert.Certainty]assume a specific zero-indexed enum order. If the backend ever reorders, inserts, or returns a non-numeric value, the wrong translation will silently render with no warning. Prefer aRecord<EnumValue, string>keyed by the actual enum constants (or a helper like the existinggetSeverityTranslationKey).♻️ Suggested shape
// in src/lib/weather-alert-utils.ts const URGENCY_KEYS: Record<WeatherAlertUrgency, string> = { [WeatherAlertUrgency.Immediate]: 'weather_alerts.urgency.immediate', [WeatherAlertUrgency.Expected]: 'weather_alerts.urgency.expected', [WeatherAlertUrgency.Future]: 'weather_alerts.urgency.future', [WeatherAlertUrgency.Past]: 'weather_alerts.urgency.past', [WeatherAlertUrgency.Unknown]: 'weather_alerts.urgency.unknown', }; export const getUrgencyTranslationKey = (u: number) => URGENCY_KEYS[u as WeatherAlertUrgency] ?? 'weather_alerts.urgency.unknown';Then in this screen:
- <Text className="text-sm font-medium text-gray-900 dark:text-gray-100">{t(`weather_alerts.urgency.${['immediate', 'expected', 'future', 'past', 'unknown'][alert.Urgency] ?? 'unknown'}`)}</Text> + <Text className="text-sm font-medium text-gray-900 dark:text-gray-100">{t(getUrgencyTranslationKey(alert.Urgency))}</Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/weather-alert/`[id].tsx around lines 155 - 162, Replace the brittle array-index mapping with explicit lookup helpers: add functions like getUrgencyTranslationKey(u: number) and getCertaintyTranslationKey(c: number) (similar to getSeverityTranslationKey) in your weather-alert-utils module that map WeatherAlertUrgency/WeatherAlertCertainty enum values to translation keys (returning 'weather_alerts.urgency.unknown' / 'weather_alerts.certainty.unknown' as default), then update the JSX in the [id] component to call t(getUrgencyTranslationKey(alert.Urgency)) and t(getCertaintyTranslationKey(alert.Certainty)) instead of indexing into hardcoded arrays.src/stores/signalr/signalr-store.ts (1)
73-103: Optional: deduplicate the three weather-alert handlers and the event-name list.The
updateEventsarray (73–84) and themethodslist (92–103) are identical — extract a single shared constant to avoid drift. Similarly, the threeweatherAlertReceived/weatherAlertUpdated/weatherAlertExpiredhandlers (133–161) are structurally identical aside from the dispatched store action; a small helper would tighten this and make adding future events trivial.♻️ Suggested shape
const UPDATE_HUB_EVENTS = [ 'personnelStatusUpdated', 'personnelStaffingUpdated', 'unitStatusUpdated', 'callsUpdated', 'callAdded', 'callClosed', 'weatherAlertReceived', 'weatherAlertUpdated', 'weatherAlertExpired', 'onConnected', ] as const; const wireWeatherAlertEvent = ( event: 'weatherAlertReceived' | 'weatherAlertUpdated' | 'weatherAlertExpired', dispatch: (alertId: string) => void, ) => { signalRService.on(event, (message) => { set({ lastUpdateMessage: JSON.stringify(message), lastUpdateTimestamp: Date.now() }); const alertId = extractAlertId(message); if (alertId) dispatch(alertId); else logger.warn({ message: `${event}: could not extract alertId from message`, context: { message } }); }); };Also applies to: 133-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/signalr/signalr-store.ts` around lines 73 - 103, The updateEvents array and the methods list are duplicated; extract them into a single exported constant (e.g., UPDATE_HUB_EVENTS) and use that constant in both the removeAllListeners loop and the signalRService.connectToHubWithEventingUrl({ methods }) call to avoid drift. Also replace the three near-identical weather alert handlers with a small helper (e.g., wireWeatherAlertEvent) that calls signalRService.on(event, ...) and performs the common set({ lastUpdateMessage, lastUpdateTimestamp }), extracts the alert id (extractAlertId) and conditionally dispatches the appropriate store action (or logs a warning) so the handlers for 'weatherAlertReceived' | 'weatherAlertUpdated' | 'weatherAlertExpired' simply call that helper.src/app/call/[id]/edit.tsx (1)
44-44: Optional: storedestinationPoiIdasnumber | nullin the form to drop the string round-trip.The selector contract is
number | null, the API contract isnumber | null, but the form schema/state stores a string and you convert in three places (String(call.DestinationPoiId)on prefill,Number(value)on render,Number(data.destinationPoiId)on submit, andpoiId.toString()in the selector callback). Switching the schema toz.number().nullable().optional()and usingnullas the default would remove all four conversions and avoid edge cases like a falsy'0'string.♻️ Suggested change
- destinationPoiId: z.string().optional(), + destinationPoiId: z.number().nullable().optional(),- destinationPoiId: '', + destinationPoiId: null,- destinationPoiId: call.DestinationPoiId != null ? String(call.DestinationPoiId) : '', + destinationPoiId: call.DestinationPoiId ?? null,- destinationPoiId: data.destinationPoiId ? Number(data.destinationPoiId) : null, + destinationPoiId: data.destinationPoiId ?? null,- <Controller - control={control} - name="destinationPoiId" - render={({ field: { onChange, value } }) => ( - <DestinationPoiSelector - destinationPois={destinationPois} - poiTypes={poiTypes} - selectedPoiId={value ? Number(value) : null} - isLoading={callDataLoading && destinationPois.length === 0} - onChange={(poiId) => onChange(poiId != null ? poiId.toString() : '')} - /> - )} - /> + <Controller + control={control} + name="destinationPoiId" + render={({ field: { onChange, value } }) => ( + <DestinationPoiSelector + destinationPois={destinationPois} + poiTypes={poiTypes} + selectedPoiId={value ?? null} + isLoading={callDataLoading && destinationPois.length === 0} + onChange={onChange} + /> + )} + />Also applies to: 134-134, 172-172, 235-235, 625-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/call/`[id]/edit.tsx at line 44, Change the form schema for destinationPoiId from a string to a numeric nullable type (use z.number().nullable().optional() with default null) so form state holds number|null instead of string; update the prefill to assign call.DestinationPoiId directly (no String(...)), update the select/render logic to use the numeric value (no Number(value) conversion), update submit to send data.destinationPoiId directly (no Number(...)), and update the selector callback so it passes poiId (number|null) instead of calling poiId.toString(); the identifiers to change are the destinationPoiId schema declaration, the prefill using String(call.DestinationPoiId), the render conversion Number(value), the submit conversion Number(data.destinationPoiId), and the selector callback that currently uses poiId.toString().src/services/check-in-notification.service.ts (1)
61-67:updateNotificationsilently delays for up to ~1s.State is mutated but no re-render is triggered; the next interval tick (up to 1000 ms later) is what eventually surfaces the new
secondsRemaining/statusto the user. If a server-driven update jumps the timer significantly (e.g., reset after a check-in), the notification will keep counting down stale seconds for almost a full second before snapping. Consider awaitingdisplayNotification(...)here so updates are reflected immediately, with the storedcallName/callNumber/timerNamefromstartNotification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/check-in-notification.service.ts` around lines 61 - 67, updateNotification currently only mutates state and waits for the next interval tick to update the UI; fix it by calling and awaiting your displayNotification(...) immediately after updating this.currentSeconds/this.currentStatus (use the stored identifiers set in startNotification: this.callName, this.callNumber, this.timerName) so the notification re-renders instantly; ensure you pass merged labels (merge this.currentLabels with the incoming statusLabels) and await the displayNotification promise before returning to preserve method semantics.src/components/routes/poi-list-content.tsx (2)
34-39: ReusecreatePoiTypeMapfrom@/lib/poi-utils.The same lookup is already exported from
src/lib/poi-utils.ts(and is reused elsewhere — e.g.status-bottom-sheet.tsxuses it). Inlining the reduce here duplicates logic and types(typeof poiTypes)[number]instead of the precisePoiTypeResultData.♻️ Suggested refactor
-import { filterPois, getPoiDisplayName, getPoiTypeName, isPoiDestinationEnabled, type PoiSortOption, sortPois } from '@/lib/poi-utils'; +import { createPoiTypeMap, filterPois, getPoiDisplayName, getPoiTypeName, isPoiDestinationEnabled, type PoiSortOption, sortPois } from '@/lib/poi-utils'; @@ - const poiTypesById = useMemo(() => { - return poiTypes.reduce<Record<number, (typeof poiTypes)[number]>>((accumulator, poiType) => { - accumulator[poiType.PoiTypeId] = poiType; - return accumulator; - }, {}); - }, [poiTypes]); + const poiTypesById = useMemo(() => createPoiTypeMap(poiTypes), [poiTypes]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/routes/poi-list-content.tsx` around lines 34 - 39, Replace the inline reduce that builds poiTypesById with the shared utility createPoiTypeMap from '@/lib/poi-utils': import createPoiTypeMap, remove the manual reduce in the poiTypesById useMemo, and call createPoiTypeMap(poiTypes) inside the useMemo so the map uses the precise PoiTypeResultData typing and avoids duplicated logic; ensure the variable name poiTypesById remains the same and update the useMemo dependency to [poiTypes].
108-126: OptimizeFlatListand extract a stablerenderItem.The inline
renderItemarrow allocates a new function on every render andonPressinside it creates another closure per row. This violates the project guideline to avoid anonymous functions inrenderItem/ event handlers, and the list is also missing the recommended FlatList performance props (removeClippedSubviews,maxToRenderPerBatch,windowSize).♻️ Suggested refactor
+ const handlePoiPress = React.useCallback((poiId: number) => { + router.push({ pathname: '/routes/poi/[id]', params: { id: String(poiId) } }); + }, []); + + const keyExtractor = React.useCallback((item: PoiResultData) => String(item.PoiId), []); + + const renderPoi = React.useCallback( + ({ item }: { item: PoiResultData }) => ( + <PoiCard + poi={item} + poiTypeLabel={getPoiTypeName(item, poiTypesById) || t('routes.poi_type_unknown')} + displayName={getPoiDisplayName(item, poiTypesById)} + isDestinationEnabled={isPoiDestinationEnabled(item, poiTypesById)} + onPress={handlePoiPress} + /> + ), + [handlePoiPress, poiTypesById, t] + ); + <FlatList<PoiResultData> testID="pois-list" data={visiblePois} - keyExtractor={(item) => String(item.PoiId)} + keyExtractor={keyExtractor} refreshControl={<RefreshControl refreshing={isLoading} onRefresh={handleRefresh} />} - renderItem={({ item }) => ( - <PoiCard - poi={item} - poiTypeLabel={getPoiTypeName(item, poiTypesById) || t('routes.poi_type_unknown')} - displayName={getPoiDisplayName(item, poiTypesById)} - isDestinationEnabled={isPoiDestinationEnabled(item, poiTypesById)} - onPress={() => router.push({ pathname: '/routes/poi/[id]', params: { id: item.PoiId } })} - /> - )} + renderItem={renderPoi} + removeClippedSubviews + maxToRenderPerBatch={10} + windowSize={10} ListEmptyComponent={ <ZeroState heading={t('routes.no_pois')} description={searchQuery || selectedPoiTypeId !== 'all' ? t('routes.no_pois_filtered_description') : t('routes.no_pois_description')} icon={RefreshCcwDotIcon} /> } contentContainerStyle={{ paddingBottom: 20 }} />Note:
PoiCardwill need to accept anonPress: (poiId: number) => voidsignature (or partial-apply viauseCallbackper row) to avoid recreating the closure.Also note the cast to
String(item.PoiId)for the route param — Expo Router params should be strings; passing a raw number relies on implicit serialization.As per coding guidelines: "Avoid anonymous functions in renderItem or event handlers" and "Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/routes/poi-list-content.tsx` around lines 108 - 126, The FlatList currently creates anonymous functions per render via the inline renderItem and per-row onPress closures; extract a stable renderItem using useCallback (e.g. memoized function referenced by FlatList.renderItem) and change PoiCard to accept onPress as onPress(poiId: number) => void so you can pass a prebound handler (or a memoized per-id callback) to avoid recreating closures; also add the recommended FlatList performance props removeClippedSubviews, maxToRenderPerBatch and windowSize, and ensure router.push param uses a string id (e.g. id: String(item.PoiId)) when building the navigation call inside your memoized renderItem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01f7a5d0-33d6-4cb5-a32c-7c6200987296
📒 Files selected for processing (55)
plugins/withCheckInLiveActivity.jssrc/api/dispatch/dispatch.tssrc/app/(app)/index.tsxsrc/app/(app)/protocols.tsxsrc/app/(app)/weather-alerts.tsxsrc/app/call/[id].tsxsrc/app/call/[id]/edit.tsxsrc/app/call/new/index.tsxsrc/app/onboarding.tsxsrc/app/routes/poi/[id].tsxsrc/app/weather-alert/[id].tsxsrc/components/calls/__tests__/call-files-modal.test.tsxsrc/components/calls/__tests__/call-images-modal.test.tsxsrc/components/calls/call-files-modal.tsxsrc/components/calls/call-images-modal.tsxsrc/components/calls/destination-poi-selector.tsxsrc/components/check-in-timers/check-in-bottom-sheet.tsxsrc/components/check-in-timers/check-in-timer-card.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/routes/active-routes-list.tsxsrc/components/routes/poi-list-content.tsxsrc/components/settings/bluetooth-device-selection-bottom-sheet.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/ui/flat-list/index.tsxsrc/components/weather-alerts/severity-filter-tabs.tsxsrc/components/weather-alerts/weather-alert-banner.tsxsrc/components/weather-alerts/weather-alert-card.tsxsrc/components/weather-alerts/weather-alert-detail-map.tsxsrc/hooks/__tests__/use-quick-check-in.test.tssrc/hooks/use-check-in-timer-polling.tssrc/hooks/use-quick-check-in.tssrc/lib/poi-utils.tssrc/lib/weather-alert-utils.tssrc/models/v4/dispatch/newCallFormResultData.tssrc/models/v4/mapping/getMapDataAndMarkersData.tssrc/models/v4/mapping/poiResults.tssrc/services/check-in-notification.service.tssrc/services/push-notification.tssrc/services/push-notification.web.tssrc/stores/calls/store.tssrc/stores/check-in-timers/__tests__/store.test.tssrc/stores/check-in-timers/store.tssrc/stores/pois/store.tssrc/stores/signalr/signalr-store.tssrc/stores/status/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
💤 Files with no reviewable changes (2)
- src/components/notifications/NotificationInbox.tsx
- src/app/(app)/protocols.tsx
✅ Files skipped from review due to trivial changes (5)
- src/components/settings/bluetooth-device-selection-bottom-sheet.tsx
- src/components/ui/flat-list/index.tsx
- src/components/calls/tests/call-images-modal.test.tsx
- src/app/(app)/index.tsx
- src/stores/status/store.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- src/models/v4/dispatch/newCallFormResultData.ts
- src/components/routes/active-routes-list.tsx
- src/components/weather-alerts/weather-alert-banner.tsx
- src/components/check-in-timers/check-in-bottom-sheet.tsx
- src/models/v4/mapping/poiResults.ts
- src/components/weather-alerts/severity-filter-tabs.tsx
- src/components/weather-alerts/weather-alert-card.tsx
- src/stores/pois/store.ts
- src/components/calls/destination-poi-selector.tsx
- src/api/dispatch/dispatch.ts
- src/app/call/new/index.tsx
- src/hooks/use-check-in-timer-polling.ts
- src/app/routes/poi/[id].tsx
- src/components/weather-alerts/weather-alert-detail-map.tsx
- src/hooks/use-quick-check-in.ts
- src/app/call/[id].tsx
- src/hooks/tests/use-quick-check-in.test.ts
- src/stores/check-in-timers/tests/store.test.ts
- src/translations/it.json
- src/lib/poi-utils.ts
- src/translations/uk.json
- src/translations/de.json
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/calls/close-call-bottom-sheet.tsx (1)
89-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
console.errorwith the shared logger.Using
console.errorviolates the coding guideline requiring the shared logger singleton for all logging. The logger automatically reports errors to Sentry and provides structured logging with auto-redaction of sensitive data.🔧 Proposed fix
+import { logger } from '@/lib/logging'; + // ... rest of imports} catch (error) { - console.error('Error closing call:', error); + logger.error({ message: 'Error closing call', context: { error, callId } }); // Show error toast showToast('error', t('call_detail.close_call_error'));As per coding guidelines: "Use the shared logger singleton from
@/lib/loggingfor all logging — never use console.log" and "logger.error()automatically reports to Sentry — use it for errors".🤖 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/calls/close-call-bottom-sheet.tsx` at line 89, Replace the direct console.error call in the close-call-bottom-sheet component with the shared logger singleton: import logger from "@/lib/logging" (if not already imported) and call logger.error with the same context and the error object when handling the close-call failure (the location where console.error('Error closing call:', error) is used inside the close-call-bottom-sheet component / its close handler). Ensure you pass the error object as structured data (e.g., logger.error('Error closing call', { error })) so the message is recorded and sent to Sentry via the shared logger.src/components/calls/call-files-modal.tsx (1)
254-282: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse a ternary instead of
&&for conditional rendering.The new conditional mount uses
{isOpen && ( <BottomSheet …/> )}, which violates the project rule of always using the ternary? :operator for conditional rendering.♻️ Proposed fix
- {isOpen && ( - <BottomSheet + {isOpen ? ( + <BottomSheet ref={bottomSheetRef} index={0} … - </BottomSheet> - )} + </BottomSheet> + ) : null}As per coding guidelines: "Always use ternary
? :for conditional rendering — never use&&".🤖 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/calls/call-files-modal.tsx` around lines 254 - 282, Replace the logical-AND conditional mount with a ternary expression: where the JSX currently uses "{isOpen && ( <BottomSheet ...>... )}", change it to use "{isOpen ? ( <BottomSheet ...>... ) : null}" so the BottomSheet (and its children like BottomSheetView, ScrollView, renderFilesContent) is rendered only when isOpen is true and otherwise returns null; ensure you keep the same props (ref={bottomSheetRef}, snapPoints, handleSheetChanges, renderBackdrop, etc.) and preserve testID attributes and styles.src/components/ui/bottom-sheet.tsx (1)
10-31: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEither honor
minHeightor remove it from the public props.
minHeightis still part ofCustomBottomSheetProps, but it's destructured as_minHeightand never used — call sites that pass it (e.g.,min-h-[400px]) will silently have no effect, which is a confusing API contract. Decide one way or the other and update the type accordingly.♻️ Suggested fix (apply the value)
- snapPoints?: number[]; - minHeight?: string; + snapPoints?: number[]; + /** Tailwind min-height utility, e.g. `min-h-[400px]`. */ + minHeight?: string; testID?: string; } -export function CustomBottomSheet({ +export function CustomBottomSheet({ children, isOpen, onClose, isLoading = false, loadingText, snapPoints = [67], - // eslint-disable-next-line `@typescript-eslint/no-unused-vars` - minHeight: _minHeight = 'min-h-[400px]', + minHeight = 'min-h-[400px]', testID, }: CustomBottomSheetProps) { ... - <VStack className="w-full flex-1" space="md"> + <VStack className={`w-full flex-1 ${minHeight}`} space="md">Alternatively, drop
minHeightfrom the props and the destructure entirely.🤖 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/ui/bottom-sheet.tsx` around lines 10 - 31, CustomBottomSheetProps declares minHeight but the component destructures it as _minHeight and never uses it, so either (A) honor it: remove the underscore, accept minHeight: string in CustomBottomSheetProps and in CustomBottomSheet use that value to set the bottom sheet container height (e.g., apply it to className or style on the root element), keep the default 'min-h-[400px]' in the prop signature and remove the eslint-disable comment; or (B) remove it: delete minHeight from CustomBottomSheetProps and remove the destructured _minHeight from CustomBottomSheet so callers’ props are not silently ignored. Reference: CustomBottomSheetProps, CustomBottomSheet, minHeight/_minHeight.src/app/routes/directions.tsx (1)
157-803:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCI failing: Prettier formatting violations.
React Native CI/CD / 5_test.txtreports Prettier mismatches on lines 157, 414, 714, 762, 774, and 803. Runpnpm prettier --write src/app/routes/directions.tsx(or your project equivalent) to auto-fix.🤖 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/routes/directions.tsx` around lines 157 - 803, Prettier formatting violations in src/app/routes/directions.tsx are causing CI failures; run your formatter (e.g., pnpm prettier --write src/app/routes/directions.tsx) and commit the resulting changes so the file is formatted correctly; focus on fixing formatting around the fetchMapboxDirections logic, the buildFallbackRoute function, and the RouteDirectionsScreen component (where the CI reported mismatches), then re-run tests/CI to verify the Prettier errors are resolved.src/app/onboarding.tsx (1)
28-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize user-visible strings via
t().Slide titles/descriptions in
SLIDESand the CTA labels (Skip,Next,Let's Get Started) are hard-coded English. The repository convention is to wrap all user-visible copy int()fromreact-i18nextso strings flow throughsrc/translations.🌐 Sketch of the change
-const SLIDES = [ +const getSlides = (t: (key: string) => string) => [ { - title: 'Resgrid Unit', - description: "Track your unit's location and status in real-time with our advanced mapping and AVL system", + title: t('onboarding.slide1.title'), + description: t('onboarding.slide1.description'), icon: <MapPin size={80} color="#FF7B1A" />, }, // ... ]; @@ - <Text className="text-gray-500">Skip</Text> + <Text className="text-gray-500">{t('onboarding.skip')}</Text> @@ - <Text className="mr-1 text-base font-semibold text-white dark:text-black">Next</Text> + <Text className="mr-1 text-base font-semibold text-white dark:text-black">{t('onboarding.next')}</Text> @@ - <Text className="text-base font-semibold text-white dark:text-black">Let's Get Started</Text> + <Text className="text-base font-semibold text-white dark:text-black">{t('onboarding.getStarted')}</Text>Then memoize with
const slides = useMemo(() => getSlides(t), [t]);inside the component.As per coding guidelines, "Wrap all user-visible text in
t()function from react-i18next".Also applies to: 205-229
🤖 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/onboarding.tsx` around lines 28 - 43, SLIDES currently contains hard-coded English strings; refactor by moving SLIDES into a function getSlides(t) that returns the slide array using t(...) for title and description values and also replace CTA labels ("Skip", "Next", "Let's Get Started") with t(...) keys; inside the component memoize with const slides = useMemo(() => getSlides(t), [t]) and update the component to use slides and the localized CTA values so all user-visible text goes through the react-i18next t function.
🟠 Major comments (27)
src/components/maps/poi-marker-icon.tsx-211-220 (1)
211-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse own-property checks for icon key lookups.
Using
inon a plain object checks prototype properties too. For untrusted/unexpectediconKey, this can return true for non-mapped keys and lead to invalid component resolution paths.Proposed fix
export function hasLucideIcon(iconKey: string): boolean { - return iconKey in MAP_ICON_TO_LUCIDE; + return Object.prototype.hasOwnProperty.call(MAP_ICON_TO_LUCIDE, iconKey); } /** * Returns the lucide icon component for a map-icon key, or undefined. */ export function getLucideIcon(iconKey: string): LucideIcon | undefined { - return MAP_ICON_TO_LUCIDE[iconKey]; + return Object.prototype.hasOwnProperty.call(MAP_ICON_TO_LUCIDE, iconKey) ? MAP_ICON_TO_LUCIDE[iconKey] : undefined; }🤖 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/maps/poi-marker-icon.tsx` around lines 211 - 220, The hasLucideIcon function uses the "in" operator which checks prototype properties; change it to an own-property check (e.g., use Object.prototype.hasOwnProperty.call or Object.hasOwn) against MAP_ICON_TO_LUCIDE in hasLucideIcon and guard getLucideIcon to only return MAP_ICON_TO_LUCIDE[iconKey] when that own-property check passes (otherwise return undefined) to avoid resolving prototype keys; reference functions: hasLucideIcon, getLucideIcon and the MAP_ICON_TO_LUCIDE object..forge/skills/react-native-best-practices/references/js-concurrent-react.md-91-91 (1)
91-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix invalid style prop when
isStaleis false.When
isStaleisfalse, the expressionisStale && { opacity: 0.7 }evaluates tofalse, which is not a valid value for React Native'sstyleprop. This will cause a runtime warning or error.🎨 Proposed fix
Use a ternary operator or wrap in an array:
- <View style={isStale && { opacity: 0.7 }}> + <View style={isStale ? { opacity: 0.7 } : undefined}>Or use array syntax:
- <View style={isStale && { opacity: 0.7 }}> + <View style={[isStale && { opacity: 0.7 }]}>🤖 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 @.forge/skills/react-native-best-practices/references/js-concurrent-react.md at line 91, The View's style prop currently receives the expression "isStale && { opacity: 0.7 }" which yields false when isStale is false and thus passes an invalid value to the style prop; update the style assignment in the View (the component using isStale) to return either a valid style object or undefined/empty by using a ternary (e.g., isStale ? { opacity: 0.7 } : undefined) or by using array syntax to conditionally include the object (e.g., [ baseStyle, isStale && { opacity: 0.7 } ]) so the style prop always receives valid values..forge/skills/react-native-best-practices/references/js-uncontrolled-components.md-13-23 (1)
13-23:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffMisleading pattern:
defaultValuebound to changing state.The "After" example shows
defaultValue={text}, butdefaultValueonly sets the initial value and does not update whentextchanges. If the parent component'stextstate changes after mount (e.g., reset button, external data load), the input will show stale data.For a truly uncontrolled input, use a hardcoded initial value:
<TextInput defaultValue="" onChangeText={setText} />Or if you need to pass an initial value from props, use a prop that won't change:
<TextInput defaultValue={initialText} onChangeText={setText} />Note: Your "Full Migration Example" at line 159 correctly uses
defaultValue="".📝 Suggested fix for Quick Pattern
**After (uncontrolled - native owns state):** ```jsx -<TextInput defaultValue={text} onChangeText={setText} /> +<TextInput defaultValue="" onChangeText={setText} /></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@.forge/skills/react-native-best-practices/references/js-uncontrolled-components.md
around lines 13 - 23, The "After" quick-pattern uses defaultValue={text} which
is misleading because defaultValue is only applied once; update the uncontrolled
example (the TextInput shown in the Quick Pattern) to use a true initial value
instead of a changing state—e.g., replace defaultValue={text} with a stable
initial value (hardcoded like "" or a prop that won't change such as
initialText) so the TextInput remains truly uncontrolled; ensure the Full
Migration Example already using defaultValue="" remains unchanged.</details> </blockquote></details> <details> <summary>.forge/skills/react-native-best-practices/references/js-uncontrolled-components.md-71-87 (1)</summary><blockquote> `71-87`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Same issue: `defaultValue` bound to state.** This example shows `defaultValue={value}` where `value` is state (line 82). As the comment correctly notes, `defaultValue` "only sets initial value," but using a state variable here is contradictory. If `value` changes later, the input won't update. Use a hardcoded initial value instead: ```diff - defaultValue={value} // Only sets initial value + defaultValue="" // Only sets initial value🤖 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 @.forge/skills/react-native-best-practices/references/js-uncontrolled-components.md around lines 71 - 87, The UncontrolledInput example incorrectly binds defaultValue to the React state variable value from useState, which contradicts the uncontrolled pattern; change the component so defaultValue is a hardcoded initial string (or a literal constant) instead of {value}, and either remove the useState entirely or rename/set state only from onChangeText if you still need to track changes—update the TextInput's defaultValue usage and the UncontrolledInput function to reflect this (referencing UncontrolledInput, useState, value, defaultValue, TextInput)..forge/skills/react-native-best-practices/references/bundle-barrel-exports.md-143-146 (1)
143-146:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftIncomplete: Add configuration example for metro-serializer-esbuild.
The section mentions installing
@rnx-kit/metro-serializer-esbuildbut doesn't show how to configure it inmetro.config.js. Without configuration instructions, users cannot implement this solution.📚 Suggested addition
Add configuration example after the installation command:
### metro-serializer-esbuild \`\`\`bash npm install `@rnx-kit/metro-serializer-esbuild` \`\`\` \`\`\`javascript // metro.config.js const { getDefaultConfig } = require('expo/metro-config'); const esbuildSerializer = require('@rnx-kit/metro-serializer-esbuild'); const config = getDefaultConfig(__dirname); config.serializer.customSerializer = esbuildSerializer(); module.exports = config; \`\`\`🤖 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 @.forge/skills/react-native-best-practices/references/bundle-barrel-exports.md around lines 143 - 146, Add a minimal configuration example showing how to wire up `@rnx-kit/metro-serializer-esbuild` in metro.config.js: require getDefaultConfig (e.g., getDefaultConfig from 'expo/metro-config' or equivalent), require('@rnx-kit/metro-serializer-esbuild') as esbuildSerializer, call getDefaultConfig(__dirname) to obtain config, set config.serializer.customSerializer = esbuildSerializer(), and export the config via module.exports so readers can copy the exact setup..forge/skills/react-native-best-practices/references/bundle-barrel-exports.md-149-151 (1)
149-151:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftIncomplete: Expand Re.Pack section with actionable guidance.
Stating "Tree shaking built-in" without setup instructions, configuration examples, or migration guidance makes this section unhelpful. Users need practical steps to adopt Re.Pack if it's presented as a solution.
📚 Suggested expansion
### Re.Pack (Webpack/Rspack) Re.Pack provides built-in tree shaking through Webpack/Rspack. \`\`\`bash npm install `@callstack/repack` \`\`\` \`\`\`javascript // webpack.config.js (generated by Re.Pack) module.exports = { mode: 'production', optimization: { usedExports: true, // Tree shaking enabled sideEffects: false, }, }; \`\`\` See [Re.Pack documentation](https://re-pack.netlify.app/) for migration guide.🤖 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 @.forge/skills/react-native-best-practices/references/bundle-barrel-exports.md around lines 149 - 151, Expand the "Re.Pack (Webpack/Rspack)" section (header: "Re.Pack (Webpack/Rspack)") to include actionable guidance: add an install command (e.g., npm install `@callstack/repack`), a short sample webpack.config.js showing optimization.usedExports: true and sideEffects: false to demonstrate enabling tree-shaking, and a brief migration pointer with a link to Re.Pack docs (https://re-pack.netlify.app/) so readers have concrete steps to adopt Re.Pack.src/lib/auth/api.tsx-17-22 (1)
17-22: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd test coverage for the authentication API interceptor.
Similar to the main API client, this interceptor calls
getBaseApiUrl()on every request. The performance verification inclient.tsxapplies here as well.The interceptor logic should be tested to ensure it correctly updates the
baseURLon each request. Consider adding tests in__tests__/api.test.tsthat verify:
- The interceptor is registered
config.baseURLis updated before each request- Changes to the server URL are reflected in subsequent requests
Based on learnings: "Generate tests for all new components, services, and logic".
🤖 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/lib/auth/api.tsx` around lines 17 - 22, Add unit tests for the authentication API interceptor: create tests in __tests__/api.test.ts that assert authApi.interceptors.request.use has been called/registered, mock getBaseApiUrl to return different URLs across calls and assert the interceptor updates config.baseURL before each request, and verify that changing the mocked server URL between requests yields the new baseURL; reference authApi, authApi.interceptors.request.use, and getBaseApiUrl, reset mocks between cases, and inspect the registered interceptor handler (or invoke it directly with a fake config) to confirm the mutation.src/app/(app)/__tests__/calls.test.tsx-55-57 (1)
55-57: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd test coverage for dispatch fetching functionality.
The mocks for
fetchCallDispatchesandcallDispatchesare added, but no test cases verify:
fetchCallDispatchesis called when calls load- Dispatches are correctly passed to
CallCard- Error handling for failed dispatch fetches
🧪 Suggested test cases
describe('dispatch functionality', () => { it('fetches dispatches when calls are loaded', () => { const mockCalls = [ { CallId: 'call-1', Nature: 'Emergency Call 1', Priority: 1 }, { CallId: 'call-2', Nature: 'Emergency Call 2', Priority: 2 }, ]; mockCallsStore.calls = mockCalls; useCallsStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector(mockCallsStore) : mockCallsStore ); render(<CallsScreen />); expect(mockCallsStore.fetchCallDispatches).toHaveBeenCalledWith(['call-1', 'call-2']); }); it('passes dispatches to CallCard', () => { const mockCalls = [{ CallId: 'call-1', Nature: 'Test Call', Priority: 1 }]; const mockDispatches = { 'call-1': [ { Id: 'disp-1', Name: 'Unit 1', Type: 'Unit' }, { Id: 'disp-2', Name: 'Personnel 1', Type: 'Personnel' }, ], }; mockCallsStore.calls = mockCalls; mockCallsStore.callDispatches = mockDispatches; useCallsStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector(mockCallsStore) : mockCallsStore ); const { CallCard } = require('@/components/calls/call-card'); const callCardMock = jest.fn(() => <div>Mock CallCard</div>); jest.mock('@/components/calls/call-card', () => ({ CallCard: callCardMock, })); render(<CallsScreen />); expect(callCardMock).toHaveBeenCalledWith( expect.objectContaining({ dispatches: mockDispatches['call-1'], }), expect.anything() ); }); it('does not fetch dispatches when calls array is empty', () => { mockCallsStore.calls = []; useCallsStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector(mockCallsStore) : mockCallsStore ); render(<CallsScreen />); expect(mockCallsStore.fetchCallDispatches).not.toHaveBeenCalled(); }); });Based on learnings: Generate tests for all new components, services, and logic.
🤖 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/`(app)/__tests__/calls.test.tsx around lines 55 - 57, The tests added mock fetchCallDispatches and callDispatches but lack assertions for dispatch fetching, passing dispatches into CallCard, and no-empty behavior; add unit tests in src/app/(app)/__tests__/calls.test.tsx that (1) set mockCallsStore.calls to a non-empty array and assert mockCallsStore.fetchCallDispatches is called with the array of CallId values when rendering CallsScreen, (2) set mockCallsStore.callDispatches to a map and mock/spy the CallCard component to assert it receives a dispatches prop for the corresponding CallId, and (3) set mockCallsStore.calls = [] and assert fetchCallDispatches is not called; use useCallsStore.mockImplementation to return mockCallsStore for selector functions and reference symbols fetchCallDispatches, callDispatches, CallCard, CallsScreen, and mockCallsStore to locate the code to change.src/components/calls/call-card.tsx-43-53 (1)
43-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAnimation cleanup is incomplete—may leak on unmount during overdue state.
The effect returns a cleanup function that stops the animation when
isTimerOverduechanges tofalse, but if the component unmounts whileisTimerOverdueis stilltrue, the animation continues running. This can cause memory leaks and performance degradation, especially on low-end devices.🔧 Proposed fix
useEffect(() => { + let animation: Animated.CompositeAnimation | null = null; if (isTimerOverdue) { - const animation = Animated.loop( + animation = Animated.loop( Animated.sequence([Animated.timing(pulseAnim, { toValue: 0.3, duration: 600, useNativeDriver: true }), Animated.timing(pulseAnim, { toValue: 1, duration: 600, useNativeDriver: true })]) ); animation.start(); - return () => animation.stop(); } else { pulseAnim.setValue(1); } + return () => { + if (animation) { + animation.stop(); + } + }; }, [isTimerOverdue, pulseAnim]);Based on learnings: Optimize for low-end devices when building components and services.
🤖 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/calls/call-card.tsx` around lines 43 - 53, The effect currently only returns a cleanup when isTimerOverdue is true, so the Animated.loop started for pulseAnim may continue when the component unmounts; fix by ensuring the animation instance is tracked (e.g., store the Animated.loop result in a ref or a local variable visible to the cleanup) and always return a cleanup function from the useEffect that stops the animation (call animation.stop()) and resets pulseAnim to 1 on unmount or when the effect re-runs; update the useEffect that references isTimerOverdue, pulseAnim and Animated.loop / Animated.timing to create/start the animation when overdue and always stop it in the returned cleanup.src/app/(app)/calls.tsx-46-52 (1)
46-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd error handling and logging for dispatch fetching.
The
fetchCallDispatchescall lacks error handling and logging, which could lead to silent failures and difficulty debugging issues in production.🛡️ Proposed fix
+import { logger } from '@/lib/logging'; + // Fetch dispatch data for all active calls after calls load (cached per callId) useEffect(() => { if (calls.length > 0) { const callIds = calls.map((c) => c.CallId); - fetchCallDispatches(callIds); + fetchCallDispatches(callIds).catch((error) => { + logger.error({ + message: 'Failed to fetch call dispatches', + context: { error, callIds }, + }); + }); } }, [calls, fetchCallDispatches]);As per coding guidelines: All async operations must have proper try/catch blocks with logging, and use the shared logger singleton from
@/lib/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/app/`(app)/calls.tsx around lines 46 - 52, The useEffect that invokes fetchCallDispatches(calls.map(c => c.CallId)) needs to guard the async call with try/catch and log failures via the shared logger; change the effect to call an async function (or async IIFE) and await fetchCallDispatches, wrapping it in try { await fetchCallDispatches(callIds) } catch (err) { logger.error("fetchCallDispatches failed", { err, callIds }) } using the logger from '@/lib/logging' and include the callIds in the error log for context.src/app/(app)/calls.tsx-27-28 (1)
27-28:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd test coverage for dispatch fetching logic and CallCard props.
The dispatch fetching effect (lines 46-52) and the dispatches prop passed to CallCard (line 91) lack test coverage. Missing tests:
- Verify
fetchCallDispatchesis called with correct callIds when calls load- Verify CallCard receives the correct dispatches value from
callDispatches[item.CallId]- Test error scenarios and dependency updates
🤖 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/`(app)/calls.tsx around lines 27 - 28, Add unit/tests that exercise the effect which calls fetchCallDispatches and the CallCard props: mock useCallsStore to control `calls` and `callDispatches`, spy on `fetchCallDispatches` to assert it is invoked with the correct array of callIds when `calls` becomes populated, and assert that each rendered CallCard receives `dispatches={callDispatches[item.CallId]}` (mock different values per CallId). Also add tests for error paths (simulate fetchCallDispatches rejection) and for dependency updates (change `calls` between renders and ensure fetchCallDispatches is called with the new ids). Use the component that imports `fetchCallDispatches`, `callDispatches`, `useCallsStore`, and `CallCard` to locate targets.src/components/calls/close-call-bottom-sheet.tsx-139-161 (1)
139-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd accessibility labels to dropdown options.
The dropdown option buttons lack accessibility attributes for screen readers.
♿ Proposed fix
<RNPressable key={type.value} style={[ styles.typeOption, closeCallType === type.value && styles.typeOptionSelected, ]} onPress={() => { setCloseCallType(type.value); setIsTypeDropdownOpen(false); }} + accessibilityRole="button" + accessibilityLabel={t(type.translationKey)} + accessibilityState={{ selected: closeCallType === type.value }} testID={`close-call-type-option-${type.value}`} >As per coding guidelines: "Follow WCAG guidelines for mobile accessibility" and "Use semantic components and accessible labels for accessibility".
🤖 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/calls/close-call-bottom-sheet.tsx` around lines 139 - 161, The dropdown option RNPressable elements in the isTypeDropdownOpen block lack accessibility attributes; update each RNPressable (inside the CLOSE_CALL_TYPES.map) to include accessibilityRole="button", an accessibilityLabel that uses the localized label (e.g., t(type.translationKey) or `\${t(type.translationKey)} option`), and accessibilityState={{ selected: closeCallType === type.value }} so screen readers announce the option and its selected state when setCloseCallType is invoked; keep existing testID and visual styles unchanged.src/components/calls/close-call-bottom-sheet.tsx-128-137 (1)
128-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd accessibility labels and extract anonymous function.
The dropdown trigger is missing accessibility attributes for screen readers and uses an inline anonymous function.
♿ Proposed fix
+ const handleTypeDropdownToggle = React.useCallback(() => { + setIsTypeDropdownOpen(!isTypeDropdownOpen); + }, [isTypeDropdownOpen]); + // ... existing code <RNPressable style={styles.typeTrigger} - onPress={() => setIsTypeDropdownOpen(!isTypeDropdownOpen)} + onPress={handleTypeDropdownToggle} + accessibilityRole="button" + accessibilityLabel={t('call_detail.close_call_type')} + accessibilityHint={t('call_detail.close_call_type_hint')} + accessibilityState={{ expanded: isTypeDropdownOpen }} testID="close-call-type-select" >As per coding guidelines: "Follow WCAG guidelines for mobile accessibility" and "Avoid anonymous functions in event handlers to prevent re-renders".
🤖 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/calls/close-call-bottom-sheet.tsx` around lines 128 - 137, The RNPressable dropdown trigger uses an inline anonymous onPress and lacks accessibility attributes; extract the handler into a named function (e.g., toggleTypeDropdown) that calls setIsTypeDropdownOpen(!isTypeDropdownOpen) and replace the inline arrow with that function, and add appropriate WCAG-friendly accessibility props to the RNPressable (e.g., accessibilityRole="button", accessibilityLabel describing the control using selectedTypeLabel or closeCallType, and accessibilityState={{ expanded: isTypeDropdownOpen }}), keeping the existing testID "close-call-type-select".src/components/calls/close-call-bottom-sheet.tsx-186-259 (1)
186-259:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImplement dark mode support with semantic color tokens.
All colors in the StyleSheet are hardcoded, preventing proper dark mode support. The coding guidelines require supporting both light and dark modes using semantic color tokens or
useColorScheme().🌓 Recommended approach
Import and use the color scheme hook:
+import { useColorScheme } from 'nativewind';Then create dynamic styles:
export const CloseCallBottomSheet: React.FC<CloseCallBottomSheetProps> = ({ ... }) => { const { colorScheme } = useColorScheme(); const isDark = colorScheme === 'dark'; // ... existing code const dynamicStyles = React.useMemo(() => StyleSheet.create({ backdrop: { flex: 1, backgroundColor: isDark ? 'rgba(0,0,0,0.7)' : 'rgba(0,0,0,0.4)', justifyContent: 'flex-end', }, sheet: { backgroundColor: isDark ? '#1F2937' : 'white', borderTopLeftRadius: 16, borderTopRightRadius: 16, maxHeight: '90%', paddingBottom: 34, paddingTop: 8, }, // ... apply to all other styles with hardcoded colors }), [isDark]); return ( <Modal ...> <RNPressable style={dynamicStyles.backdrop} ...> <RNPressable style={dynamicStyles.sheet} ...>Alternatively, use Tailwind/NativeWind
classNameinstead of StyleSheet for automatic dark mode support where gluestack-ui components are available.As per coding guidelines: "Support both dark mode and light mode" and "Use semantic color tokens from Tailwind config instead of hardcoded hex values".
🤖 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/calls/close-call-bottom-sheet.tsx` around lines 186 - 259, The styles object uses hardcoded hex colors and must be converted to theme-aware dynamic styles: import useColorScheme (or your app's theme hook) into the CloseCallBottomSheet component, compute isDark = colorScheme === 'dark', then create dynamicStyles via React.useMemo that mirrors the current styles keys (backdrop, sheet, handle, typeTrigger, typeText, typePlaceholder, typeDropdown, typeOption, typeOptionSelected, typeOptionText, typeOptionTextSelected) but replace hardcoded colors with semantic tokens or conditional values (e.g., isDark ? tokenDark : tokenLight or Tailwind tokens) and return/use dynamicStyles instead of the static styles constant in the JSX; ensure StyleSheet.create is used inside the useMemo or remove the old static styles to avoid duplication.src/components/calls/call-images-modal.tsx-260-264 (1)
260-264: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace
&&with a ternary for conditional rendering.The new
renderActiveImageuses{item.Url && ( ... )}which violates the project rule to always use the ternary operator for conditional rendering.♻️ Proposed fix
- {item.Url && ( - <Text className="mt-1 px-2 text-center text-xs text-gray-400" numberOfLines={2}> - URL: {item.Url} - </Text> - )} + {item.Url ? ( + <Text className="mt-1 px-2 text-center text-xs text-gray-400" numberOfLines={2}> + URL: {item.Url} + </Text> + ) : null}As per coding guidelines: "Always use ternary
? :for conditional rendering — never use&&".🤖 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/calls/call-images-modal.tsx` around lines 260 - 264, The JSX uses `{item.Url && (...)}` which breaks the rule to always use ternary rendering; update the renderActiveImage (or the component that returns this JSX) to replace that expression with a ternary like `item.Url ? (<Text ...>URL: {item.Url}</Text>) : null`, preserving the Text props (`className="mt-1 px-2 text-center text-xs text-gray-400"` and `numberOfLines={2}`) and behavior; ensure you only change the conditional operator and not the markup or styling.src/components/calls/call-files-modal.tsx-122-126 (1)
122-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize the server-provided filename before joining with
documentDirectory.
file.FileName/file.Namecome from API data and are concatenated directly into the on-device URI. A value containing path separators or..segments could cause the write to land outside the intended directory (overwriting other files in the app sandbox), or fail with confusing errors due to invalid characters. Strip directory components and unsafe characters before composingfileUri.🛡️ Proposed fix
- const fileName = file.FileName || file.Name || `file_${file.Id}`; - if (!documentDirectory) { - throw new Error('Document directory is unavailable'); - } - const fileUri = `${documentDirectory}${fileName}`; + if (!documentDirectory) { + throw new Error('Document directory is unavailable'); + } + const rawName = file.FileName || file.Name || `file_${file.Id}`; + // Keep only the basename and strip characters that are unsafe in file paths. + const safeName = + rawName + .split(/[\\/]/) + .pop()! + .replace(/[^A-Za-z0-9._-]/g, '_') + .replace(/^\.+/, '') || `file_${file.Id}`; + const fileUri = `${documentDirectory}${safeName}`;🤖 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/calls/call-files-modal.tsx` around lines 122 - 126, The server-supplied filename (file.FileName / file.Name stored in fileName) is used raw when composing fileUri and must be sanitized: implement a sanitizeFilename helper (used where fileName is chosen in the call-files-modal component) that strips directory components (equivalent to taking a basename), removes any ".." segments or path separators, strips or replaces unsafe characters (allow only a safe whitelist like letters, digits, "-", "_", and "."), enforces a reasonable max length and a safe fallback (e.g., `file_<Id>.dat`) if the result is empty, then use the sanitized value when constructing fileUri with documentDirectory to avoid path traversal or invalid characters.src/components/calls/call-detail-menu.tsx-23-27 (2)
23-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLabel the icon-only menu button for screen readers.
Line 25 renders only the kebab icon, so TalkBack/VoiceOver will not announce what action this button performs. Please add an
accessibilityRoleand a translatedaccessibilityLabel. As per coding guidelines "Follow WCAG guidelines for mobile accessibility".🤖 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/calls/call-detail-menu.tsx` around lines 23 - 27, The HeaderRightMenuButton renders an icon-only Pressable which is not announced by screen readers; update the Pressable inside HeaderRightMenuButton to include accessibilityRole="button" and a translated accessibilityLabel (use the app's i18n helper, e.g., accessibilityLabel={t('calls.kebabMenu') } or similar) so VoiceOver/TalkBack announce the control; keep the existing onPress and testID props intact and ensure the translation key is added to the locale files.
23-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the hardcoded menu colors with theme-aware tokens.
This modal now bakes in fixed hex/
whitevalues for the sheet, handle, icons, and destructive text, so it will not adapt cleanly to dark mode and can miss the app’s semantic color system. Please derive these colors from the current scheme/tokens instead of static values. As per coding guidelines "Support both dark mode and light mode — respond to system color scheme viauseColorScheme()" and "Use semantic color tokens from Tailwind config ... instead of hardcoded hex values".Also applies to: 97-113, 131-158
🤖 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/calls/call-detail-menu.tsx` around lines 23 - 27, The component currently uses hardcoded colors (hex and "white") for the sheet, handle, icons, and destructive text; update HeaderRightMenuButton and the modal parts (the sheet, handle, icon color, and destructive text rendering referenced later in this file) to use the app's theme tokens and respond to system color scheme: import and call useColorScheme() (or use the existing theme hook), map scheme to semantic Tailwind tokens (e.g., text-primary/text-secondary, bg-surface, border-handle, text-destructive) instead of static values, and pass those token-based classNames or resolved color values into the Pressable/MoreVerticalIcon (replace color="#6B7280") and into the sheet container, sheet handle, and destructive-label elements so they adapt to dark/light mode.src/components/routes/poi-list-content.tsx-1-19 (1)
1-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix import ordering (CI failing).
The
testcheck is failing onsimple-import-sort. In the internal-alias group,@/components/routes/filter-contextshould sort before@/components/routes/poi-card.♻️ Suggested fix
import { Loading } from '@/components/common/loading'; import ZeroState from '@/components/common/zero-state'; -import { PoiCard } from '@/components/routes/poi-card'; import { useFilterContext } from '@/components/routes/filter-context'; +import { PoiCard } from '@/components/routes/poi-card'; import { Box } from '@/components/ui/box';As per coding guidelines: "ESLint enforces
simple-import-sortfor consistent import ordering".🤖 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/routes/poi-list-content.tsx` around lines 1 - 19, The imports in poi-list-content.tsx are out of order causing simple-import-sort to fail; move the internal-alias import for useFilterContext (from '@/components/routes/filter-context') to appear before the import for PoiCard (from '@/components/routes/poi-card') so the internal group is sorted correctly—ensure the relative order among other internal imports (e.g., getPoiTypeName/getPoiDisplayName imports) remains unchanged and run the linter to verify.src/components/routes/routes-home.tsx-1-19 (1)
1-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix import ordering (CI failing).
lucide-react-nativeandnativewindare external packages but currently appear after the internal-alias block, breaking thesimple-import-sortorder (side-effects → external → internal aliases → relative → types).♻️ Suggested fix
import React from 'react'; +import { Check } from 'lucide-react-native'; +import { useColorScheme } from 'nativewind'; import { useTranslation } from 'react-i18next'; import { Pressable, ScrollView, View } from 'react-native'; import { ActiveRoutesList } from '@/components/routes/active-routes-list'; import { FilterProvider, useFilterContext } from '@/components/routes/filter-context'; import { PoiListContent } from '@/components/routes/poi-list-content'; import { Box } from '@/components/ui/box'; import { CustomBottomSheet } from '@/components/ui/bottom-sheet'; import { Heading } from '@/components/ui/heading'; import { HStack } from '@/components/ui/hstack'; import { SharedTabs, type TabItem } from '@/components/ui/shared-tabs'; import { Text } from '@/components/ui/text'; import { VStack } from '@/components/ui/vstack'; import { type PoiSortOption } from '@/lib/poi-utils'; import { usePoisStore } from '@/stores/pois/store'; -import { Check } from 'lucide-react-native'; -import { useColorScheme } from 'nativewind';As per coding guidelines: "Imports must follow ESLint
simple-import-sortorder: side-effects, external packages, internal aliases, relative imports, type imports".🤖 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/routes/routes-home.tsx` around lines 1 - 19, The import order violates simple-import-sort: move the external package imports (Check from lucide-react-native and useColorScheme from nativewind) so they appear with other external imports before the internal-alias imports (e.g., before imports like '@/components/...', '@/lib/...', '@/stores/...'); keep side-effect imports first, then externals, then internal aliases, then relative imports and type-only imports last, and re-run the linter to ensure ordering is fixed (check symbols: Check and useColorScheme, and the modules ActiveRoutesList, FilterProvider, PoiListContent, etc., to verify correct grouping).src/app/routes/start.tsx-89-89 (1)
89-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter drops valid stops at 0° latitude/longitude.
s.Latitude && s.Longitudetreats0as falsy, so any stop on the equator or the prime meridian (e.g., London) is excluded from the map. The sibling implementation insrc/app/routes/directions.tsx(lines 414-415) already uses the correct!= null && isFinite(...)pattern — please align here.🛠️ Proposed fix
- const mapStops = useMemo(() => sortedStops.filter((s) => s.Latitude && s.Longitude), [sortedStops]); + const mapStops = useMemo( + () => sortedStops.filter((s) => s.Latitude != null && s.Longitude != null && isFinite(s.Latitude) && isFinite(s.Longitude)), + [sortedStops] + );🤖 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/routes/start.tsx` at line 89, The mapStops useMemo filter drops valid coordinates because it uses the falsy check `s.Latitude && s.Longitude`; update the predicate in the `mapStops` computation to mirror the sibling pattern in directions.tsx by checking that `s.Latitude` and `s.Longitude` are not null/undefined and are finite numbers (e.g., `s.Latitude != null && s.Longitude != null && isFinite(Number(s.Latitude)) && isFinite(Number(s.Longitude))`) so zeros on the equator/prime meridian are included; locate `mapStops` and replace the existing arrow function predicate accordingly.src/app/routes/directions.tsx-256-259 (1)
256-259: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSilently swallowing fetch errors masks production failures.
The empty
catch {}returnsnullwithout logging, so a Mapbox outage, malformed payload, or network timeout is invisible in Sentry. Per coding guidelines, async operations must have try/catch with logging via the shared logger.🛠️ Proposed fix
- } catch { - return null; - } + } catch (error) { + logger.error({ message: 'Failed to fetch Mapbox directions', context: { error, waypointCount: waypoints.length } }); + return null; + }Add the import:
import { logger } from '@/lib/logging';As per coding guidelines: "All async operations must have proper try/catch blocks with logging" and "Use
logger.error()for error logging; errors are automatically reported to Sentry".🤖 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/routes/directions.tsx` around lines 256 - 259, The catch block in the async function that wraps the Mapbox fetch (in src/app/routes/directions.tsx) silently returns null; update it to catch the error object and log it via the shared logger so failures surface to Sentry: add the import line "import { logger } from '@/lib/logging';", change the empty catch to "catch (error) { logger.error('Failed to fetch directions', error); return null; }", and ensure this is applied inside the function that calls Mapbox (the function surrounding the shown try/catch) so network/Mapbox/payload errors are logged.src/app/routes/directions.tsx-111-132 (1)
111-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUser-visible labels are not internationalized.
'No traffic data','Severe traffic','Heavy traffic','Moderate traffic', and'Light traffic'are rendered to the user (line 759) but bypasst(). Either return a translation key from this helper and callt()at the call site, or passtin.🛠️ Suggested shape
-const deriveDrivingCondition = (congestion: CongestionSegment[]): { label: string; color: string; icon: typeof TrafficConeIcon } => { +const deriveDrivingCondition = ( + congestion: CongestionSegment[], +): { labelKey: string; color: string; icon: typeof TrafficConeIcon } => { if (congestion.length === 0) { - return { label: 'No traffic data', color: '#9ca3af', icon: TrafficConeIcon }; + return { labelKey: 'routes.traffic.no_data', color: '#9ca3af', icon: TrafficConeIcon }; } ... };Then at line 759:
{t(drivingCondition.labelKey)}. Add the corresponding keys undersrc/translations/....As per coding guidelines: "Wrap all user-visible text in
t()function from react-i18next".🤖 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/routes/directions.tsx` around lines 111 - 132, deriveDrivingCondition currently returns user-visible strings; change it to return a translation key (e.g., labelKey) instead of literal labels and keep color/icon (e.g., return { labelKey: 'directions.noTraffic', color: '#9ca3af', icon: TrafficConeIcon }), update the call site that consumes deriveDrivingCondition to call t(drivingCondition.labelKey) (or pass t into deriveDrivingCondition if you prefer), and add the corresponding keys (directions.noTraffic, directions.severeTraffic, directions.heavyTraffic, directions.moderateTraffic, directions.lightTraffic) to the translations files; references: deriveDrivingCondition, CongestionSegment, TrafficConeIcon, AlertTriangleIcon.src/app/routes/start.tsx-91-91 (1)
91-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
useRef<any>with a properly typed Camera ref interface.Per coding guidelines,
anyis disallowed in TypeScript. Define a structural interface for the Camera ref methods and use it for type safety:interface CameraRef { fitBounds: (ne: [number, number], sw: [number, number], pad?: number | number[], duration?: number) => void; setCamera: (options: { centerCoordinate?: [number, number]; zoomLevel?: number; heading?: number; pitch?: number; animationDuration?: number }) => void; flyTo: (coordinatesOrOptions: any, duration?: number) => void; } const cameraRef = useRef<CameraRef>(null);This issue appears in:
src/app/routes/start.tsxline 91src/app/routes/directions.tsxline 384src/app/routes/active.tsxline 72🤖 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/routes/start.tsx` at line 91, Replace the untyped ref useRef<any>(null) with a concrete Camera ref interface: define an interface CameraRef that lists the expected methods (fitBounds, setCamera, flyTo) and their parameter types, then change the ref declarations (cameraRef in start.tsx, directions.tsx, active.tsx) to use useRef<CameraRef>(null); ensure the method signatures match how fitBounds, setCamera, and flyTo are invoked elsewhere so TypeScript type-checks calls against the new CameraRef interface..forge/skills/react-native-best-practices/references/bundle-code-splitting.md-52-68 (1)
52-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove or replace Re.Pack guidance; it is incompatible with this managed Expo app.
Re.Pack is not officially supported in Expo managed apps and requires replacing Metro—which breaks compatibility with Expo CLI, EAS, and Expo Go. Using Re.Pack in this project would require native build configuration outside Expo prebuild, violating the project's managed Expo constraints.
If code splitting is needed, revise this section to recommend Expo-compatible alternatives (e.g., lazy loading via React Router,
React.lazy(), or native libraries supported through Expo).🤖 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 @.forge/skills/react-native-best-practices/references/bundle-code-splitting.md around lines 52 - 68, The guidance to install and initialize Re.Pack (the "Re.Pack installed" prerequisite and "Initialize Re.Pack" step) must be removed because Re.Pack replaces Metro and is incompatible with managed Expo; edit bundle-code-splitting.md to delete the Re.Pack prereq and npx `@callstack/repack-init` instructions and replace them with Expo-compatible alternatives such as using React.lazy/dynamic import, React.Suspense, route-based code splitting with expo-router or React Navigation lazy screens, and a short note stating that Re.Pack is not supported in managed Expo apps and native builds would be required for it..forge/skills/react-native-best-practices/references/bundle-native-assets.md-72-116 (1)
72-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRewrite these iOS asset setup steps to use Expo config plugins and prebuild instead of manual native edits.
The current instructions (creating
ios/RNAssets.xcassets, editing Xcode build phases, and running native build commands directly) are manual native-project changes that violate Expo-managed constraints. In this repo, any native modifications must be automated through config plugins during prebuild or EAS build—manual edits inios/will be overwritten on the next prebuild run.Instead, direct users to:
- Use a config plugin (e.g.,
expo-assetor a custom plugin) to generate the asset catalog and wire up Xcode build phases automatically- Run
prebuildoreas buildto apply the plugin changes to the native project- Avoid manually editing native files or running native build commands directly
🤖 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 @.forge/skills/react-native-best-practices/references/bundle-native-assets.md around lines 72 - 116, Replace the manual iOS native-edit instructions (creating RNAssets.xcassets and adding export EXTRA_PACKAGER_ARGS in the "Bundle React Native code and images" build phase and running native bundle commands) with guidance to implement an Expo config plugin that automates the asset-catalog creation and build-phase wiring; specifically, instruct using an existing plugin such as expo-asset or a custom config plugin to generate the RNAssets.xcassets and inject the EXTRA_PACKAGER_ARGS build-phase change, then run expo prebuild or eas build to apply those changes to the native project rather than editing ios/ or running native bundle commands manually..forge/skills/react-native-best-practices/references/js-animations-reanimated.md-118-124 (1)
118-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
useAnimatedStylepure; this example can emit duplicate analytics.
useAnimatedStylecallbacks must remain side-effect free. CallingscheduleOnRN(...)inside it violates this constraint—the worklet can re-run many times, firingtrackAnalyticsrepeatedly after animation completion. Move side effects touseAnimatedReaction(recommended) or the animation completion callback.Suggested doc fix
+import { useAnimatedReaction } from 'react-native-reanimated'; import { scheduleOnRN } from 'react-native-worklets'; // Regular JS function const trackAnalytics = (value) => { analytics.track('animation_complete', { value }); }; const AnimatedComponent = () => { const progress = useSharedValue(0); - const animatedStyle = useAnimatedStyle(() => { - // When animation completes, call JS function - if (progress.value === 1) { - scheduleOnRN(trackAnalytics, progress.value); - } - return { opacity: progress.value }; - }); + useAnimatedReaction( + () => progress.value === 1, + (isComplete, wasComplete) => { + if (isComplete && !wasComplete) { + scheduleOnRN(trackAnalytics, 1); + } + } + ); + + const animatedStyle = useAnimatedStyle(() => { + return { opacity: progress.value }; + }); return <Animated.View style={animatedStyle} />; };🤖 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 @.forge/skills/react-native-best-practices/references/js-animations-reanimated.md around lines 118 - 124, The useAnimatedStyle callback (animatedStyle) is performing a side effect by calling scheduleOnRN(trackAnalytics, progress.value), which can run multiple times; remove that call from useAnimatedStyle and instead create a useAnimatedReaction (or use the animation completion callback) that watches progress.value and when it transitions to 1 invokes scheduleOnRN(trackAnalytics, progress.value); update references to animatedStyle, scheduleOnRN, trackAnalytics and add useAnimatedReaction to ensure the analytics call runs once on completion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ui/__tests__/bottom-sheet.test.tsx`:
- Around line 286-290: The test currently dereferences vstacks[1] (variable
contentVstack) without checking vstacks length which yields opaque
undefined.props errors if DOM structure changes; update the assertions to first
assert the expected number of VStack elements (e.g.,
expect(vstacks.length).toBeGreaterThan(1) or toEqual(expectedCount)) before
accessing vstacks[1], then perform the existing checks on
contentVstack.props.className and contentVstack.props.space; apply the same
guard to the other occurrence around lines 296-300 so both index dereferences
are protected by explicit length expectations.
- Around line 57-65: Reset the mocked useColorScheme implementation in
beforeEach to avoid state leakage by calling its mock reset/restore (e.g.,
useColorScheme.mockReset() or mockRestore()) so each test gets the default
behavior; keep existing cleanup in afterEach (cleanup(), jest.clearAllMocks(),
jest.useRealTimers()). Also make the tests that access vstacks[1] and centers[1]
resilient by asserting the arrays have the expected length (e.g.,
expect(vstacks).toHaveLength(>=2) and expect(centers).toHaveLength(>=2)) before
indexing so they fail with clear diagnostics instead of throwing.
🪄 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: 224a0ecd-7ea7-4185-8e42-ba01ad4002d0
📒 Files selected for processing (25)
babel.config.jsplugins/withCheckInLiveActivity.jssrc/app/(app)/weather-alerts.tsxsrc/app/onboarding.tsxsrc/app/routes/directions.tsxsrc/components/calls/call-card.tsxsrc/components/calls/call-detail-menu.tsxsrc/components/calls/call-files-modal.tsxsrc/components/calls/call-images-modal.tsxsrc/components/calls/close-call-bottom-sheet.tsxsrc/components/check-in-timers/check-in-timer-card.tsxsrc/components/routes/active-routes-list.tsxsrc/components/routes/poi-list-content.tsxsrc/components/routes/routes-home.tsxsrc/components/ui/__tests__/bottom-sheet.test.tsxsrc/components/ui/bottom-sheet.tsxsrc/components/ui/shared-tabs.tsxsrc/lib/weather-alert-utils.tssrc/services/__tests__/push-notification.test.tssrc/services/push-notification.tssrc/services/push-notification.web.tssrc/stores/calls/store.tssrc/stores/check-in-timers/store.tssrc/stores/weather-alerts/store.tssrc/utils/action-sheet.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/components/routes/routes-home.tsx
- src/components/calls/call-files-modal.tsx
- src/app/(app)/weather-alerts.tsx
- src/components/calls/call-detail-menu.tsx
- src/components/calls/close-call-bottom-sheet.tsx
- src/components/routes/poi-list-content.tsx
- src/components/check-in-timers/check-in-timer-card.tsx
- src/components/routes/active-routes-list.tsx
- plugins/withCheckInLiveActivity.js
- src/app/routes/directions.tsx
- src/app/onboarding.tsx
- src/components/ui/shared-tabs.tsx
- src/components/calls/call-images-modal.tsx
- src/components/ui/bottom-sheet.tsx
- src/components/calls/call-card.tsx
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| cleanup(); | ||
| jest.clearAllMocks(); | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mutable mock usage vs lifecycle reset strategy in this test file.
fd -i 'bottom-sheet.test.tsx' --exec rg -n 'beforeEach\\(|afterEach\\(|clearAllMocks\\(|resetAllMocks\\(|useColorScheme\\.mockReturnValue' {}Repository: Resgrid/Unit
Length of output: 1892
🏁 Script executed:
# First, check if the file exists and its size
wc -l src/components/ui/__tests__/bottom-sheet.test.tsxRepository: Resgrid/Unit
Length of output: 110
🏁 Script executed:
# Read the beforeEach/afterEach setup (lines 57-65)
sed -n '50,75p' src/components/ui/__tests__/bottom-sheet.test.tsxRepository: Resgrid/Unit
Length of output: 506
🏁 Script executed:
# Read the area around lines 286-300 to check array indexing
sed -n '280,310p' src/components/ui/__tests__/bottom-sheet.test.tsxRepository: Resgrid/Unit
Length of output: 1229
🏁 Script executed:
# Search for all useColorScheme.mockReturnValue calls to understand the pattern
rg -n 'useColorScheme' src/components/ui/__tests__/bottom-sheet.test.tsxRepository: Resgrid/Unit
Length of output: 442
🏁 Script executed:
# Get more context around the array indexing tests (lines 280-310)
sed -n '280,315p' src/components/ui/__tests__/bottom-sheet.test.tsxRepository: Resgrid/Unit
Length of output: 1476
🏁 Script executed:
# Check if there are explicit length assertions elsewhere in the test
rg -n 'getAllByTestId.*length|\.length|expect.*length' src/components/ui/__tests__/bottom-sheet.test.tsx | head -20Repository: Resgrid/Unit
Length of output: 739
🏁 Script executed:
# Check the mock initialization on line 45 to understand the factory function
sed -n '40,50p' src/components/ui/__tests__/bottom-sheet.test.tsxRepository: Resgrid/Unit
Length of output: 411
🏁 Script executed:
# Verify if resetAllMocks would be more appropriate than clearAllMocks
# Check if mock needs default reset or just implementation
sed -n '1,50p' src/components/ui/__tests__/bottom-sheet.test.tsx | head -50Repository: Resgrid/Unit
Length of output: 1659
Reset useColorScheme mock to default in beforeEach to prevent state leakage between tests.
jest.clearAllMocks() only clears call history, not mock implementations. Tests at lines 187, 196, 205, and 212 override useColorScheme.mockReturnValue() without resetting, which can cause color scheme state to leak to subsequent tests.
Fix
beforeEach(() => {
jest.useFakeTimers();
+ useColorScheme.mockReturnValue({ colorScheme: 'light' });
});
afterEach(() => {
cleanup();
jest.clearAllMocks();
jest.useRealTimers();
});Also: Add length assertions before accessing array indices at lines 287 (vstacks[1]) and 297 (centers[1]) to avoid brittle tests that assume component structure.
🤖 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/ui/__tests__/bottom-sheet.test.tsx` around lines 57 - 65,
Reset the mocked useColorScheme implementation in beforeEach to avoid state
leakage by calling its mock reset/restore (e.g., useColorScheme.mockReset() or
mockRestore()) so each test gets the default behavior; keep existing cleanup in
afterEach (cleanup(), jest.clearAllMocks(), jest.useRealTimers()). Also make the
tests that access vstacks[1] and centers[1] resilient by asserting the arrays
have the expected length (e.g., expect(vstacks).toHaveLength(>=2) and
expect(centers).toHaveLength(>=2)) before indexing so they fail with clear
diagnostics instead of throwing.
| const vstacks = screen.getAllByTestId('vstack'); | ||
| // Content wrapper VStack (second one) should have w-full and space="md" | ||
| const contentVstack = vstacks[1]; | ||
| expect(contentVstack.props.className).toContain('w-full'); | ||
| expect(contentVstack.props.space).toBe('md'); |
There was a problem hiding this comment.
Guard array indexing with explicit length expectations.
Both tests dereference index [1] directly. If structure changes, failures become opaque undefined.props errors instead of clear assertion failures.
Proposed fix
const vstacks = screen.getAllByTestId('vstack');
+expect(vstacks.length).toBeGreaterThan(1);
// Content wrapper VStack (second one) should have w-full and space="md"
const contentVstack = vstacks[1];
expect(contentVstack.props.className).toContain('w-full');
expect(contentVstack.props.space).toBe('md');
const centers = screen.getAllByTestId('center');
+expect(centers.length).toBeGreaterThan(1);
// Loading Center (second one) should have h-32
const loadingCenter = centers[1];
expect(loadingCenter.props.className).toContain('h-32');Also applies to: 296-300
🤖 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/ui/__tests__/bottom-sheet.test.tsx` around lines 286 - 290,
The test currently dereferences vstacks[1] (variable contentVstack) without
checking vstacks length which yields opaque undefined.props errors if DOM
structure changes; update the assertions to first assert the expected number of
VStack elements (e.g., expect(vstacks.length).toBeGreaterThan(1) or
toEqual(expectedCount)) before accessing vstacks[1], then perform the existing
checks on contentVstack.props.className and contentVstack.props.space; apply the
same guard to the other occurrence around lines 296-300 so both index
dereferences are protected by explicit length expectations.
|
Approve |
Summary by CodeRabbit