Conversation
Contributor
|
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 SSO (OIDC/SAML) authentication flows and hooks, user-defined fields (models, API, renderer, and UI integration), expanded call create/edit (templates, forms, scheduling), Electron notification IPC/preload and main-process changes, platform/storage guards and mocks, generated vendor scripts, and many new components and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginScreen
participant SSOScreen
participant OidcProvider as OIDC Provider
participant AuthServer
participant AppStore
participant AppHome
User->>LoginScreen: Tap SSO button
LoginScreen->>SSOScreen: Navigate
User->>SSOScreen: Enter username/department & start
SSOScreen->>AuthServer: fetchSsoConfigForUser(username)
AuthServer-->>SSOScreen: return SSO config
SSOScreen->>OidcProvider: open auth (promptAsync / browser)
OidcProvider-->>SSOScreen: deep link with code
SSOScreen->>OidcProvider: exchangeCodeAsync(code, code_verifier)
OidcProvider-->>SSOScreen: return id_token
SSOScreen->>AuthServer: externalTokenRequest('oidc', id_token, username, dept)
AuthServer-->>SSOScreen: AuthResponse (access/refresh)
SSOScreen->>AppStore: loginWithSso(authResponse)
AppStore->>AppStore: decode token, save profile/tokens
AppStore-->>SSOScreen: confirm
SSOScreen->>AppHome: navigate (authenticated)
sequenceDiagram
participant Editor as Call Create/Edit
participant UdfRenderer as UDF Renderer
participant UdfApi as UDF API
participant Server
participant FormState as Component State
Editor->>UdfRenderer: mount(entityType, entityId?)
UdfRenderer->>UdfApi: getUdfDefinition(entityType)
UdfApi->>Server: GET /UserDefinedFields/{entityType}
Server-->>UdfApi: definitions
UdfApi-->>UdfRenderer: return definitions
UdfRenderer->>UdfApi: getUdfValues(entityType, entityId)
UdfApi->>Server: GET /UserDefinedFields/Values/{entityType}/{entityId}
Server-->>UdfApi: existing values
UdfApi-->>UdfRenderer: return values
User->>UdfRenderer: edit fields
UdfRenderer->>FormState: onValuesChange(values)
User->>Editor: submit form
Editor->>Server: create/update call (includes callFormData, linkedCallId, scheduledOn)
Server-->>Editor: return callId
Editor->>UdfApi: saveUdfValues(entityType, callId, udfValues)
UdfApi->>Server: POST /UserDefinedFields/Values
Server-->>UdfApi: success
Editor->>FormState: navigate away
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
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 (3)
src/__tests__/app/root/maintenance.test.tsx (1)
93-95:⚠️ Potential issue | 🟡 MinorMissing
awaitbeforewaitFormay cause unreliable test behavior.The
waitForfunction returns a Promise and should be awaited to ensure the assertion is properly evaluated before the test completes.🔧 Proposed fix
it('should redirect to login if maintenance mode is disabled', () => { (Env as any).MAINTENANCE_MODE = false; render( <TestWrapper> <Maintenance /> </TestWrapper> ); - waitFor(() => { + await waitFor(() => { expect(mockReplace).toHaveBeenCalledWith('/login'); }); });Note: The test function also needs to be marked as
async:- it('should redirect to login if maintenance mode is disabled', () => { + it('should redirect to login if maintenance mode is disabled', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/app/root/maintenance.test.tsx` around lines 93 - 95, The test is calling waitFor without awaiting it, which can make the mockReplace assertion flaky; update the test that contains the waitFor(...) call so the test function is declared async and add await before the waitFor call (i.e., await waitFor(() => { expect(mockReplace).toHaveBeenCalledWith('/login'); })), ensuring the assertion completes before the test ends and referencing the existing waitFor and mockReplace usages.src/app/call/new/index.tsx (2)
2-2:⚠️ Potential issue | 🟡 MinorRemove unused test library import.
renderfrom@testing-library/react-nativeis imported but never used in this file. This is a test utility that should not be included in production code — it unnecessarily increases the bundle size.🗑️ Suggested fix
import { zodResolver } from '@hookform/resolvers/zod'; -import { render } from '@testing-library/react-native'; import axios from 'axios';🤖 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, Remove the unused test utility import: delete the line importing "render" from "@testing-library/react-native" in this module (the import statement referencing render) so production code does not include test-only dependencies; ensure no other code in this file references render and run a quick build/lint to confirm removal.
1116-1116:⚠️ Potential issue | 🟡 MinorWrap the PlusIcon in a View component instead of applying className directly to the icon.
Per coding guidelines, lucide-react-native icons should not have style props passed directly. Replace:
Current code
<PlusIcon size={18} className="mr-2" /> <ButtonText>{t('calls.create')}</ButtonText>Should be
<View className="mr-2"> <PlusIcon size={18} /> </View> <ButtonText>{t('calls.create')}</ButtonText>🤖 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 1116, The PlusIcon is receiving style via className which violates the lucide-react-native guideline; wrap the PlusIcon in a View and move the className there instead of passing it to the icon. Update the JSX around PlusIcon and ButtonText so it renders <View className="mr-2"><PlusIcon size={18} /></View> followed by <ButtonText>{t('calls.create')}</ButtonText>, ensuring PlusIcon keeps only its size prop and no style props.
🟡 Minor comments (9)
src/__tests__/app/index.test.tsx-6-6 (1)
6-6:⚠️ Potential issue | 🟡 MinorRename the
Mapimport alias to avoid shadowing the restricted global name.Line 6 shadows the global
Mapconstructor, triggering thenoShadowRestrictedNameslint error. Rename the import alias toMapScreenand update all 11 JSX references (lines 161, 171, 188, 198, 213, 221, 237, 251, 266, 285, 294).♻️ Proposed fix
-import Map from '../../app/(app)/map'; +import MapScreen from '../../app/(app)/map';Update all JSX references:
- const { unmount } = render(<Map />, { wrapper: TestWrapper }); + const { unmount } = render(<MapScreen />, { wrapper: TestWrapper });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/app/index.test.tsx` at line 6, The imported component alias currently named Map shadows the global Map constructor; rename the import alias in the test from Map to MapScreen (i.e., change "import Map from '../../app/(app)/map'" to use MapScreen) and then update all JSX usages of <Map ...> to <MapScreen ...> (there are 11 occurrences across the file) so the linter noShadowRestrictedNames error is resolved while keeping component references consistent.src/app/login/sso.tsx-169-175 (1)
169-175:⚠️ Potential issue | 🟡 MinorValidate the normalized username and localize these errors.
onLookup()trims before submitting, but the schema validates the raw value." "currently passes.min(3)and gets sent as an empty username, and the hard-coded English messages bypass i18n. As per coding guidelines, "Ensure all text is wrapped int()fromreact-i18nextfor translations with the dictionary files stored insrc/translations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/sso.tsx` around lines 169 - 175, The ssoLookupSchema is validating the raw username (so whitespace-only values like " " pass .min(3)) and its error strings are hard-coded English; update validation to validate the normalized/trimmed username and localize messages: ensure onLookup’s trimmed value is what gets passed into ssoLookupSchema (or add a preprocess/transform on the username field in ssoLookupSchema to .trim()), change the schema error strings for username and departmentId to use t() from react-i18next (import t or use the i18n instance) so messages come from your translations, and keep the departmentId refine message localized as well; reference ssoLookupSchema and onLookup when making these changes.app.config.ts-76-83 (1)
76-83:⚠️ Potential issue | 🟡 MinorRemove
autoVerifyfrom custom-scheme intent filters.Android only verifies
http/httpsApp Links; custom URL schemes cannot be verified withautoVerify. Setting this flag on a custom-scheme filter is incorrect and may cause verification diagnostics noise.Suggested change
intentFilters: [ { action: 'VIEW', - autoVerify: true, data: [{ scheme: Env.SCHEME }], category: ['BROWSABLE', 'DEFAULT'], }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.config.ts` around lines 76 - 83, Remove the invalid autoVerify property from the custom-scheme intent filter: locate the intentFilters array (the object with action: 'VIEW' and data: [{ scheme: Env.SCHEME }]) and delete the autoVerify key (or ensure autoVerify is only set on http/https App Link filters). Keep the rest of the filter (action, data, category) intact and do not add autoVerify for custom URL schemes.src/components/calls/call-form-renderer.tsx-67-76 (1)
67-76:⚠️ Potential issue | 🟡 MinorForm data collection may not handle all input types correctly.
The current implementation only captures
el.valuefor all input types. This doesn't properly handle:
- Checkboxes: Need to check
el.checkedproperty- Radio buttons: Need to check
el.checkedand collect only selected value- Multi-select: Need to iterate selected options
If the form schema includes these input types, the collected data will be incomplete or incorrect.
💡 Suggested improvement
function sendData() { var data = {}; var fields = document.querySelectorAll('#callForm input, `#callForm` select, `#callForm` textarea'); fields.forEach(function(el) { if (!el.name) return; if (el.type === 'checkbox') { data[el.name] = el.checked; } else if (el.type === 'radio') { if (el.checked) data[el.name] = el.value; } else if (el.tagName === 'SELECT' && el.multiple) { data[el.name] = Array.from(el.selectedOptions).map(function(opt) { return opt.value; }); } else { data[el.name] = el.value; } }); window.ReactNativeWebView.postMessage(JSON.stringify(data)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/call-form-renderer.tsx` around lines 67 - 76, The sendData function currently reads only el.value so it misses checkboxes, radio groups, and multi-selects; update sendData (and keep the form change/input listeners on the callForm) to: skip elements without a name, for checkboxes use el.checked (store boolean), for radio inputs only set data[el.name] when el.checked (so the selected radio value is captured), and for SELECT elements with multiple iterate el.selectedOptions to produce an array of selected values; all other inputs/textarea still use el.value, then post the JSON as before via window.ReactNativeWebView.postMessage.src/services/push-notification.ts-109-117 (1)
109-117:⚠️ Potential issue | 🟡 MinorAdd error handling for electron notification service initialization.
The
electronNotificationService.initialize()call is awaited but not wrapped in try/catch. If initialization fails, the error will propagate and could prevent the push notification service from completing its own initialization.🛡️ Proposed fix
} else if (Platform.OS === 'web') { // On web / Electron, initialize the electron notification service // which uses the native OS Notification API. - await electronNotificationService.initialize(); + try { + await electronNotificationService.initialize(); + } catch (error) { + logger.warn({ + message: 'Failed to initialize electron notification service', + context: { error }, + }); + } logger.info({ message: 'Push notification service initialized (web/electron)', }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/push-notification.ts` around lines 109 - 117, Wrap the call to electronNotificationService.initialize() in a try/catch inside the web/electron branch so initialization failures don't bubble up; call electronNotificationService.initialize() within the try, move the logger.info(...) into the try's success path, and in the catch use logger.error(...) to record a clear message plus the caught error (include error stack/message) and allow the push-notification initialization to continue (do not rethrow) or perform a graceful fallback. Reference: electronNotificationService.initialize() and logger.info/logger.error in the push-notification service.src/components/calls/linked-calls-modal.tsx-45-55 (1)
45-55:⚠️ Potential issue | 🟡 MinorAdd null-safe property access in the filter function.
The filter accesses
Name,Number,Nature, andAddressproperties directly. If any of these could be undefined or null, this will throw a runtime error when calling.toLowerCase().🛡️ Proposed fix
const filtered = useMemo(() => { if (!searchQuery.trim()) return calls; const q = searchQuery.toLowerCase(); return calls.filter( (c) => - c.Name.toLowerCase().includes(q) || - c.Number.toLowerCase().includes(q) || - c.Nature.toLowerCase().includes(q) || - c.Address.toLowerCase().includes(q), + c.Name?.toLowerCase().includes(q) || + c.Number?.toLowerCase().includes(q) || + c.Nature?.toLowerCase().includes(q) || + c.Address?.toLowerCase().includes(q), ); }, [calls, searchQuery]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/linked-calls-modal.tsx` around lines 45 - 55, The filter in the useMemo for `filtered` calls `.toLowerCase()` directly on `c.Name`, `c.Number`, `c.Nature`, and `c.Address`, which can throw if any are null/undefined; update the predicate in the `calls.filter(...)` used by `filtered` to use null-safe accessors (e.g., coalesce or optional chaining with default empty string) for `c.Name`, `c.Number`, `c.Nature`, and `c.Address` before calling `.toLowerCase()` so the comparison uses a safe string value when any property is missing.src/components/dispatch-console/active-calls-panel.tsx-604-608 (1)
604-608:⚠️ Potential issue | 🟡 MinorUnsafe type cast for height style property.
The cast
'100%' as unknown as numberis a workaround for TypeScript's StyleSheet typing but could cause runtime issues on some platforms. Consider using a numeric value or Dimensions.🛡️ Proposed fix
dispatchBadgeDivider: { width: 1, - height: '100%' as unknown as number, + alignSelf: 'stretch', backgroundColor: 'rgba(255,255,255,0.35)', },Or if a specific height is needed:
dispatchBadgeDivider: { width: 1, - height: '100%' as unknown as number, + height: 14, // Match parent badge height backgroundColor: 'rgba(255,255,255,0.35)', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 604 - 608, dispatchBadgeDivider currently uses an unsafe cast ('100%' as unknown as number) for the height style; replace this with a safe value or approach: either provide a numeric height (e.g., a calculated number or constant) or use a string percentage without casting (height: '100%') or compute the pixel height via Dimensions (Dimensions.get('window').height * 0.x) and assign that numeric value; update the dispatchBadgeDivider style object accordingly to remove the unsafe cast.src/hooks/use-oidc-login.ts-27-36 (1)
27-36:⚠️ Potential issue | 🟡 MinorPlaceholder clientId could cause unexpected behavior.
Using
'placeholder'as a fallback forclientIdmeans the auth request will be created with invalid credentials if called before the actual clientId is available. Consider adding a guard or returning early frompromptAsyncif clientId is not set.🛡️ Suggested defensive approach
const [request, response, promptAsync] = AuthSession.useAuthRequest( { - clientId: clientId || 'placeholder', + clientId: clientId || '', redirectUri, scopes: ['openid', 'email', 'profile', 'offline_access'], usePKCE: true, responseType: AuthSession.ResponseType.Code, }, discovery ); + +// Wrap promptAsync to guard against empty clientId +const safePromptAsync = useCallback( + async (options?: AuthSession.AuthRequestPromptOptions) => { + if (!clientId) { + logger.error({ message: 'SSO OIDC: Cannot prompt - clientId not set' }); + return { type: 'dismiss' } as AuthSession.AuthSessionResult; + } + return promptAsync(options); + }, + [clientId, promptAsync] +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-oidc-login.ts` around lines 27 - 36, The hook currently constructs an auth request with a hard-coded 'placeholder' clientId which can create invalid requests; update the logic so you do not create or invoke the AuthSession request when clientId is not set—either avoid calling AuthSession.useAuthRequest until clientId is truthy or add a guard that returns early from promptAsync when clientId is missing; specifically change how clientId is passed into AuthSession.useAuthRequest (the call that currently uses clientId || 'placeholder') and ensure promptAsync (and any code that relies on request/response) no-ops or signals an error if clientId is absent.src/components/dispatch-console/personnel-actions-panel.tsx-563-569 (1)
563-569:⚠️ Potential issue | 🟡 MinorExpose the expand/collapse state to assistive tech.
The new "Additional Fields" header is interactive, but it doesn't declare a button role or expanded state, so screen readers can't tell that it toggles content.
As per coding guidelines: Ensure the app is accessible, following WCAG guidelines for mobile applications.♿ Suggested fix
- <Pressable onPress={() => setIsAdditionalFieldsExpanded((prev) => !prev)}> + <Pressable + accessibilityRole="button" + accessibilityState={{ expanded: isAdditionalFieldsExpanded }} + onPress={() => setIsAdditionalFieldsExpanded((prev) => !prev)} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/personnel-actions-panel.tsx` around lines 563 - 569, The "Additional Fields" Pressable lacks accessibility semantics; update the Pressable that wraps the HStack (the one toggling setIsAdditionalFieldsExpanded) to expose accessibilityRole="button" and accessibilityState={{ expanded: isAdditionalFieldsExpanded }} (and optionally an accessibilityLabel using the t(...) string) so screen readers know it is a toggle that opens the UdfFieldsRenderer for entityType=1 and entityId=selectedPersonnel.UserId; keep the onPress logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__mocks__/react-native-mmkv.ts`:
- Around line 13-24: Move the in-constructor _fallbackData/_fallbackStorage to
module-level shared storage keyed by MMKV instance id so instances with the same
id share a single backing store; create a module-scope Map (e.g.,
fallbackStores: Map<string, Record<string,string>>) and module-scope factory
that returns a Storage-like wrapper for a given id, then in the MMKV constructor
replace the local _fallbackData/_fallbackStorage and the this.storage assignment
with logic that looks up or creates the shared store for the given id and
returns its Storage wrapper (keep using window.localStorage when available).
Ensure you reference the existing symbols _fallbackData/_fallbackStorage and
this.storage/MMKV when making the change so tests for shared ids observe the
same underlying data.
In `@electron/preload.js`:
- Around line 6-9: Replace the direct global assignment to window.__ELECTRON__
with a contextBridge exposure so the renderer can access the flag when
contextIsolation is enabled: use contextBridge.exposeInMainWorld('__ELECTRON__',
true) instead of setting window.__ELECTRON__ directly; update the preload file
near the existing electronNotifications exposure to call
contextBridge.exposeInMainWorld for '__ELECTRON__' so the renderer can read
window.__ELECTRON__.
In `@src/__tests__/app/login/index.web.test.tsx`:
- Line 6: The test currently mocks the component under test via the jest.mock
call for the module '../../../app/login/index.web' so it only asserts the fake;
remove that jest.mock(...) for the login component and import the real component
from src/app/login/index.web (the component exported from that module, e.g., the
default Login component) so the suite exercises the actual code; if any heavy
dependencies inside the real Login component need isolation, mock those specific
child modules or network calls (not the module that exports the Login component)
so the SSO UI and wiring are exercised by the tests.
In `@src/app/login/sso.tsx`:
- Around line 223-246: In onLookup wrap the async discovery call to
fetchSsoConfigForUser in a try/catch/finally: move the await into try, set
setSsoConfig, setResolvedUsername, setResolvedDepartmentId and
setPhase('sso-options') only on success, setLookupError(...) in catch (use a
localized error like t('sso.error_lookup_failed') or the caught error message)
and ensure setIsLookingUp(false) is called in finally so the loading state is
cleared; reference functions/vars: onLookup, fetchSsoConfigForUser,
setIsLookingUp, setLookupError, setSsoConfig, setResolvedUsername,
setResolvedDepartmentId, setPhase.
- Around line 117-139: The effect currently only listens for runtime Linking
'url' events and misses handling when the app is cold-started by the SAML
redirect; add a mount-time check using Linking.getInitialURL() (in a separate
useEffect or inside the same mount effect before registering the listener) and
feed that URL into the same async logic that calls onAuthStart(),
handleSamlDeepLink(url), onTokenReceived(), onError(...), logger.error(...), and
onAuthEnd(); ensure you reuse the same handler (or extract to a shared
processDeepLink(url) helper) so both the Linking.getInitialURL() path and the
Linking.addEventListener('url', ...) subscription run identical
token-exchange/error/cleanup logic.
In `@src/components/calls/call-form-renderer.tsx`:
- Around line 24-25: Remove the external <script> tags loading jQuery and
form-render.min.js from src/components/calls/call-form-renderer.tsx and instead
bundle/self-host a pinned copy: add form-render (and jquery) as project
dependencies via npm or place a specific version of form-render.min.js into your
app assets, then import them in the CallFormRenderer module (or whichever
component in that file) using standard ES imports or a relative asset path;
ensure you pin the version, update any global access points the component
expects, and remove reliance on the CDN so forms render offline and
deterministically.
In `@src/components/calls/call-form-renderer.web.tsx`:
- Around line 87-95: The message handler in useEffect of
call-form-renderer.web.tsx (the handler that calls onFormDataChange) accepts
messages from any origin; update the handler to validate event.origin (or
event.source) against an allowlist (e.g., window.location.origin or a new
prop/const like allowedMessageOrigin/allowedOrigins) before calling
onFormDataChange, and ignore messages that don't match; ensure the validation
uses the same unique symbol names (handler, onFormDataChange) so reviewers can
find the change and add any necessary prop/type updates to support the allowed
origin value.
- Around line 56-70: The inline script interpolates unescaped formSchemaJson
into JS and uses window.parent.postMessage with '*' — fix by (1) avoiding direct
interpolation into script: serialize and safely pass the JSON to the iframe DOM
(e.g., set a data attribute or a textContent of a <script
type="application/json"> and then parse it inside the iframe) so formSchemaJson
is never executed as HTML/JS before being parsed for $('#callForm').formRender;
reference the symbol formSchemaJson and the callForm initialization to locate
where to change. (2) Replace window.parent.postMessage(JSON.stringify(data),
'*') in sendData with postMessage using a specific allowed origin (derive the
expected parent origin from a config or a constant and use that exact origin
string) to avoid broadcasting sensitive form data.
In `@src/lib/auth/api.tsx`:
- Around line 103-117: Replace direct username logging in the SSO token-exchange
path: in the block around authApi.post('/connect/external-token') remove
username from logger.info and logger.error calls and instead log a non-PII
correlation (e.g., a requestId or a one-way hash of username). Update the logger
invocations referenced (logger.info and logger.error in this function) to
include only safe identifiers (requestId or hashedUsername) and keep
authResponse handling unchanged; ensure the requestId/hash is computed before
the post call so both success and failure logs use the same correlation value.
In `@src/lib/storage/index.web.tsx`:
- Around line 3-4: The storage access check using isLocalStorageAvailable is
insufficient because reading or calling localStorage can throw (e.g., Safari
private mode); update all places that access localStorage so every operation is
protected by try/catch: wrap the exported storage object's methods (the object
named storage) and the zustandStorage.getItem and zustandStorage.removeItem
implementations in try/catch blocks that fall back to the existing in-memory
behavior (or return null/undefined) on error, and ensure any thrown exceptions
are swallowed or logged via console.warn so callers never receive a runtime
exception from localStorage access.
In `@src/services/sso-discovery.ts`:
- Around line 24-29: The catch block in the SSO discovery flow currently
swallows all exceptions and returns null (see the catch using logger.error and
the return null), conflating runtime failures with "no config"; change this so
the function either throws the original/error-wrapped exception or returns a
discriminated failure result (e.g. an object like { status: 'error', error })
instead of null; update the function signature (the SSO discovery function
referenced in this file) and adjust callers to handle the new failure variant or
catch the rethrown error so outages, 5xxs, and parsing errors are
distinguishable from the "no config" case.
- Around line 18-21: The SSO discovery logs currently include the raw username;
update the logging in the SSO discovery routine so it no longer emits the
username (remove username from the logger.info and logger.error calls shown
around the logger.info block and the similar block at lines 25-28). Instead log
a request identifier or coarse outcome (e.g., requestId, ssoEnabled flag, and
success/failure outcome) and ensure any error branch uses safe metadata (no raw
user identifiers) when calling logger methods; locate the logger.info and
logger.error calls in the SSO discovery function and replace the username field
with a non-identifying requestId/outcome field.
In `@src/stores/auth/store.tsx`:
- Around line 194-241: The loginWithSso function currently swallows all errors
in its outer try/catch (mutating state but not rethrowing), so callers cannot
observe failures; update the outer catch in loginWithSso to rethrow the caught
error after logging and setting the store state (or alternatively return a
rejected Promise) so the caller (e.g., src/app/login/sso.tsx awaiting
loginWithSso) receives the error; refer to loginWithSso, the outer catch block
that calls logger.error and set({...}) and ensure it ends by throwing the
original error (or throw a new Error with the same message) so failures
propagate.
---
Outside diff comments:
In `@src/__tests__/app/root/maintenance.test.tsx`:
- Around line 93-95: The test is calling waitFor without awaiting it, which can
make the mockReplace assertion flaky; update the test that contains the
waitFor(...) call so the test function is declared async and add await before
the waitFor call (i.e., await waitFor(() => {
expect(mockReplace).toHaveBeenCalledWith('/login'); })), ensuring the assertion
completes before the test ends and referencing the existing waitFor and
mockReplace usages.
In `@src/app/call/new/index.tsx`:
- Line 2: Remove the unused test utility import: delete the line importing
"render" from "@testing-library/react-native" in this module (the import
statement referencing render) so production code does not include test-only
dependencies; ensure no other code in this file references render and run a
quick build/lint to confirm removal.
- Line 1116: The PlusIcon is receiving style via className which violates the
lucide-react-native guideline; wrap the PlusIcon in a View and move the
className there instead of passing it to the icon. Update the JSX around
PlusIcon and ButtonText so it renders <View className="mr-2"><PlusIcon size={18}
/></View> followed by <ButtonText>{t('calls.create')}</ButtonText>, ensuring
PlusIcon keeps only its size prop and no style props.
---
Minor comments:
In `@app.config.ts`:
- Around line 76-83: Remove the invalid autoVerify property from the
custom-scheme intent filter: locate the intentFilters array (the object with
action: 'VIEW' and data: [{ scheme: Env.SCHEME }]) and delete the autoVerify key
(or ensure autoVerify is only set on http/https App Link filters). Keep the rest
of the filter (action, data, category) intact and do not add autoVerify for
custom URL schemes.
In `@src/__tests__/app/index.test.tsx`:
- Line 6: The imported component alias currently named Map shadows the global
Map constructor; rename the import alias in the test from Map to MapScreen
(i.e., change "import Map from '../../app/(app)/map'" to use MapScreen) and then
update all JSX usages of <Map ...> to <MapScreen ...> (there are 11 occurrences
across the file) so the linter noShadowRestrictedNames error is resolved while
keeping component references consistent.
In `@src/app/login/sso.tsx`:
- Around line 169-175: The ssoLookupSchema is validating the raw username (so
whitespace-only values like " " pass .min(3)) and its error strings are
hard-coded English; update validation to validate the normalized/trimmed
username and localize messages: ensure onLookup’s trimmed value is what gets
passed into ssoLookupSchema (or add a preprocess/transform on the username field
in ssoLookupSchema to .trim()), change the schema error strings for username and
departmentId to use t() from react-i18next (import t or use the i18n instance)
so messages come from your translations, and keep the departmentId refine
message localized as well; reference ssoLookupSchema and onLookup when making
these changes.
In `@src/components/calls/call-form-renderer.tsx`:
- Around line 67-76: The sendData function currently reads only el.value so it
misses checkboxes, radio groups, and multi-selects; update sendData (and keep
the form change/input listeners on the callForm) to: skip elements without a
name, for checkboxes use el.checked (store boolean), for radio inputs only set
data[el.name] when el.checked (so the selected radio value is captured), and for
SELECT elements with multiple iterate el.selectedOptions to produce an array of
selected values; all other inputs/textarea still use el.value, then post the
JSON as before via window.ReactNativeWebView.postMessage.
In `@src/components/calls/linked-calls-modal.tsx`:
- Around line 45-55: The filter in the useMemo for `filtered` calls
`.toLowerCase()` directly on `c.Name`, `c.Number`, `c.Nature`, and `c.Address`,
which can throw if any are null/undefined; update the predicate in the
`calls.filter(...)` used by `filtered` to use null-safe accessors (e.g.,
coalesce or optional chaining with default empty string) for `c.Name`,
`c.Number`, `c.Nature`, and `c.Address` before calling `.toLowerCase()` so the
comparison uses a safe string value when any property is missing.
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 604-608: dispatchBadgeDivider currently uses an unsafe cast
('100%' as unknown as number) for the height style; replace this with a safe
value or approach: either provide a numeric height (e.g., a calculated number or
constant) or use a string percentage without casting (height: '100%') or compute
the pixel height via Dimensions (Dimensions.get('window').height * 0.x) and
assign that numeric value; update the dispatchBadgeDivider style object
accordingly to remove the unsafe cast.
In `@src/components/dispatch-console/personnel-actions-panel.tsx`:
- Around line 563-569: The "Additional Fields" Pressable lacks accessibility
semantics; update the Pressable that wraps the HStack (the one toggling
setIsAdditionalFieldsExpanded) to expose accessibilityRole="button" and
accessibilityState={{ expanded: isAdditionalFieldsExpanded }} (and optionally an
accessibilityLabel using the t(...) string) so screen readers know it is a
toggle that opens the UdfFieldsRenderer for entityType=1 and
entityId=selectedPersonnel.UserId; keep the onPress logic unchanged.
In `@src/hooks/use-oidc-login.ts`:
- Around line 27-36: The hook currently constructs an auth request with a
hard-coded 'placeholder' clientId which can create invalid requests; update the
logic so you do not create or invoke the AuthSession request when clientId is
not set—either avoid calling AuthSession.useAuthRequest until clientId is truthy
or add a guard that returns early from promptAsync when clientId is missing;
specifically change how clientId is passed into AuthSession.useAuthRequest (the
call that currently uses clientId || 'placeholder') and ensure promptAsync (and
any code that relies on request/response) no-ops or signals an error if clientId
is absent.
In `@src/services/push-notification.ts`:
- Around line 109-117: Wrap the call to electronNotificationService.initialize()
in a try/catch inside the web/electron branch so initialization failures don't
bubble up; call electronNotificationService.initialize() within the try, move
the logger.info(...) into the try's success path, and in the catch use
logger.error(...) to record a clear message plus the caught error (include error
stack/message) and allow the push-notification initialization to continue (do
not rethrow) or perform a graceful fallback. Reference:
electronNotificationService.initialize() and logger.info/logger.error in the
push-notification service.
---
Nitpick comments:
In `@electron/main.js`:
- Around line 140-148: The notification click handler currently calls
BrowserWindow.getAllWindows()[0], which can pick the wrong window when multiple
windows exist; update the handler to use a stored reference to the main window
(e.g., a top-level mainWindow variable set when creating the BrowserWindow)
instead of indexing getAllWindows(), and in the notification.on('click')
callback check that mainWindow exists and is not destroyed
(mainWindow.isDestroyed()), restore if minimized (mainWindow.isMinimized() →
mainWindow.restore()), then mainWindow.focus(); keep a defensive fallback to
BrowserWindow.getAllWindows() only if mainWindow is null or destroyed.
In `@electron/preload.js`:
- Around line 36-39: The onNotification registration currently pushes callbacks
into notificationCallbacks with no removal, causing potential leaks; modify
onNotification to return an unsubscribe function (or a disposable object) that
removes the specific callback from the notificationCallbacks array when called,
or alternatively accept/return a token and implement a
removeNotification/token-based deregistration helper; update any related call
sites to call the returned unsubscribe on component unmount. Ensure you
reference the existing onNotification function and the notificationCallbacks
array when implementing the removal logic so only the intended callback is
removed.
In `@src/__tests__/app/calls.test.tsx`:
- Around line 249-259: Replace the placeholder assertions in the FAB navigation
test for CallsScreen with a real interaction: render CallsScreen, query the FAB
using screen.getByTestId('fab'), trigger the press with fireEvent.press(fab) and
assert that router.push was called with the expected route (e.g., '/calls/new');
update the test name to async if needed and remove the expect(true).toBe(true)
placeholder. Ensure you reference CallsScreen, router.push,
screen.getByTestId('fab'), fireEvent.press, and the expected route in the
assertion.
In `@src/__tests__/app/login/login-form.test.tsx`:
- Around line 10-61: MockLoginForm currently lacks support for the new
onSsoPress prop; update the MockLoginForm component to accept onSsoPress and
render an SSO TouchableOpacity (e.g., testID "sso-button" with label "SSO" or
similar) when onSsoPress is provided, wiring its onPress to call onSsoPress;
then add tests mirroring the onServerUrlPress cases: assert the SSO button
renders when onSsoPress is passed, does not render when omitted, and that
pressing the SSO button calls the provided onSsoPress handler.
In `@src/api/userDefinedFields/userDefinedFields.ts`:
- Around line 24-32: The saveUdfValues function currently calls
api.post<void>('/UserDefinedFields/Values', ...) but returns response.data,
causing a type mismatch; either remove the return and let the async function
resolve to void, or update the api.post generic to the actual response type
(e.g., ApiResponseType) so response.data is correctly typed—modify the
saveUdfValues function to use the chosen approach and ensure the api.post
generic and the return statement are consistent.
In `@src/app/call/`[id]/edit.web.tsx:
- Around line 883-898: Replace the unsupported gap usage in the StyleSheet by
removing the gap entries from twoColumnLayout, singleColumnLayout, leftColumn,
and rightColumn and instead apply explicit margin properties to achieve the same
spacing: for twoColumnLayout keep flexDirection: 'row' and add marginRight
(e.g., 24) to the left column style (leftColumn) or marginLeft on the right
column; for singleColumnLayout keep flexDirection: 'column' and add marginBottom
(e.g., 16) to children or to a wrapper item style; ensure any components relying
on gap between children are updated to use those margin styles so the layout
spacing is preserved on web.
In `@src/app/call/new/index.tsx`:
- Around line 209-215: The useEffect that calls getNewCallForm currently
swallows errors in the empty catch, so update the catch on the getNewCallForm
promise to log or surface the error (e.g., using console.error or the app's
logger) including the error object and a brief context message (e.g.,
"getNewCallForm failed in new call form load"), and optionally set an error
state or fallback UI; touch the useEffect block that calls getNewCallForm, its
.then(result => { if (result?.Data?.Data) setCallForm(result.Data); }), and the
.catch(...) to implement proper logging/handling instead of a no-op.
In `@src/app/call/new/index.web.tsx`:
- Around line 1378-1381: The style object twoInputRow currently uses CSS gap
which is unsupported; remove gap: 16 from twoInputRow and instead add margin
spacing to the child input elements used inside that row (e.g., add marginRight:
16 to the first input or marginLeft to the second). Locate where twoInputRow is
applied and give the individual child components a new style (e.g.,
twoInputChild or firstInput style) with the margin so spacing matches the
original intent.
- Around line 1280-1295: The styles twoColumnLayout, singleColumnLayout,
leftColumn and rightColumn use the CSS gap property which is unreliable on web;
remove the gap keys and instead implement margin-based spacing on child elements
(e.g. create a shared child style like columnItem with marginRight for row
layouts and marginBottom for column layouts, or apply conditional last-child
margin removal) and update the components that render children inside
twoColumnLayout/singleColumnLayout/leftColumn/rightColumn to apply that child
margin style so visual spacing is preserved without using gap.
- Around line 360-366: The current useEffect swallowing errors from
getNewCallForm makes debugging hard; update the catch on the getNewCallForm
promise in the useEffect (in index.web.tsx) to log the error instead of
noop—e.g., catch the error and call a logger (console.error or the app's logger)
with a clear message like "Failed to load new call form" and the error object so
failures in getNewCallForm (used to setCallForm) are visible while preserving
the existing success flow in the .then handler.
In `@src/app/login/index.web.tsx`:
- Around line 392-407: Add an icon to the SSO Pressable to match the Server
button: import a suitable icon (e.g., Key, LogIn, or Shield) from
lucide-react-native, then render it inside the second Pressable (the one that
calls router.push('/login/sso')) before the Text that uses t('sso.sso_button'),
using the same size (16) and conditional color logic (isDark ? '#9ca3af' :
'#6b7280') and preserving existing styles (styles.serverUrlButton,
styles.actionButtonFlex and serverUrlButtonText styles) so spacing/layout
remains consistent with the Server icon.
In `@src/components/calls/call-form-renderer.tsx`:
- Around line 94-96: Replace the empty catch block in call-form-renderer.tsx
(the try/catch around the message parsing/handling) so it no longer swallows
errors; change "catch { }" to "catch (err) { console.error('Error parsing
message in call-form-renderer:', err, rawMessageOrPayload) }", ensuring you log
the caught error and the original input being parsed (use the actual variable
name for the raw payload in that scope) to aid debugging.
In `@src/components/calls/call-form-renderer.web.tsx`:
- Around line 22-23: The external <script> tags loading jQuery and form-render
in call-form-renderer.web.tsx should be replaced with a safer, pinned approach:
either install and import the libraries (e.g., npm package "jquery" and the
form-render package or "formbuilder" equivalents) and import them at the top of
the CallFormRenderer component, or if you must keep script tags, pin exact
versioned URLs and add SRI integrity and crossorigin attributes to each tag;
update the CallFormRenderer (or call-form-renderer.web.tsx) entry to use the
bundled/imported modules instead of relying on the CDN so behavior is versioned
and supply-chain risk is reduced.
In `@src/components/calls/call-templates-modal.tsx`:
- Around line 33-37: The effect references loadTemplates but doesn't list it in
the dependency array, causing an ESLint missing-deps warning; fix by making
loadTemplates stable (wrap it in useCallback with its own dependencies) or by
inlining its logic inside the useEffect, and then include loadTemplates (or any
used values) in the useEffect dependency array so the hook and the function
(e.g., loadTemplates, isVisible) are correctly declared as dependencies.
- Around line 127-136: The JSX uses logical AND (&&) for conditional rendering
of the CallName and CallNature blocks; replace each instance with a ternary
expression so the component explicitly renders either the <Text> element or null
— specifically update the template.CallName block and the template.CallNature
block to use template.CallName ? (render Text with t('calls.name'),
styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight) : null and
similarly use template.CallNature ? (render Text with t('calls.nature'),
styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight,
numberOfLines={2}) : null, preserving existing props and style composition.
In `@src/components/calls/contact-picker-modal.tsx`:
- Around line 26-31: The useEffect in ContactPickerModal is missing dependencies
which can cause stale closures: include fetchContacts and contacts.length in the
dependency array (in addition to isVisible) or ensure fetchContacts is memoized
(e.g., from a stable callback) so the effect runs correctly when the contact
loader or contacts length change; keep the existing setIsLoading/fetchContacts
logic inside the effect and update the dependency array for the effect that
references isVisible, fetchContacts, and contacts.length.
- Around line 106-134: Extract the list item into a memoized component (e.g.,
ContactItem) and move per-item logic (getDisplayName, getContactInfo, avatar
text, styles.item/ itemDark/ itemLight, itemName, itemInfo) into it so the
parent no longer creates inline render or onPress handlers; implement
ContactItem as React.memo and inside it create a stable onPress using
useCallback that calls the passed onSelect prop (pass parent handleSelect as
onSelect), then replace the inline map JSX to return <ContactItem
contact={contact} isDark={isDark} onSelect={handleSelect}
key={contact.ContactId} /> so there are no anonymous functions in the onPress or
item renderer.
In `@src/components/calls/linked-calls-modal.tsx`:
- Around line 136-145: Replace the conditional rendering that uses the &&
operator for call.Nature and call.Address with explicit ternary expressions so
they follow the code guideline; specifically, in linked-calls-modal.tsx change
the two blocks rendering <Text> for call.Nature and call.Address to use
call?.Nature ? <Text ...>{call.Nature}</Text> : null and call?.Address ? <Text
...>{call.Address}</Text> : null (preserving the existing style arrays:
styles.itemMeta, styles.itemAddress and the isDark conditional styles) to add
null-safe checks and keep behavior identical.
In `@src/components/calls/protocol-selector-modal.tsx`:
- Around line 149-153: Replace the conditional rendering that uses the logical
AND with an explicit ternary: where the component currently uses
{!!protocol.Code && ( <Text ...>{protocol.Code}</Text> )}, change it to use
protocol.Code ? <Text ...>{protocol.Code}</Text> : null so the UI renders the
Text node only when protocol.Code is truthy; update the JSX around the Text
element that uses StyleSheet.flatten([styles.protocolCode, isDark ?
styles.protocolCodeDark : styles.protocolCodeLight]) accordingly (keep the same
props and styles, only change the conditional to a ternary).
- Around line 53-55: getStableId currently falls back to an unstable index-based
key (protocol-index-{index}); change it so the fallback is a deterministic,
content-based identifier instead of the list index (or enforce that Id is always
present upstream). Update the getStableId function to return protocol.Id if
present, otherwise compute a stable id from immutable protocol fields (e.g., a
concatenation or hashed value of protocol.Name, protocol.Type, createdAt, or
other unique fields) so keys remain stable across reorders; alternatively
throw/assert if Id is missing to force the upstream provider to supply it.
Ensure you reference getStableId and protocol.Id when making the change.
- Around line 41-49: The effect that runs when the modal becomes visible
(useEffect) currently only depends on isVisible and therefore can see stale
initialSelected, protocols, or fetchProtocols; update its dependency array to
include initialSelected, protocols.length (or protocols) and fetchProtocols so
the effect re-runs when those change, and if fetchProtocols is recreated each
render, memoize it (e.g., via useCallback) to avoid unintended loops; keep using
setSelected and setIsLoading inside the effect as they are stable React state
setters.
In `@src/components/calls/udf-fields-renderer.tsx`:
- Around line 53-107: The effect in useEffect re-fetches when initialValues
reference changes; remove initialValues from the dependency array and instead
track/apply them without triggering the full load: keep entityType and entityId
(and notifyChange) as deps, introduce a ref like hasInitializedRef used inside
load to ensure the expensive fetch (getUdfDefinition, getUdfValues) runs only on
mount or when entityType/entityId change, and separately apply any updated
initialValues by reading a stable ref or doing a shallow/deep comparison (or
document that initialValues must be memoized); update load/after-load logic to
call setValues and notifyChange accordingly while leaving setFields and fetch
calls guarded by hasInitializedRef so changes to initialValues don’t re-trigger
getUdfDefinition/getUdfValues.
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 280-319: The fetch logic is racy because fetchedCallIdsRef is
mutated synchronously while async fetches may be overwritten by later renders;
change to derive toFetch from the current fetchedCallIdsRef inside a
useCallback-triggered effect, mark fetches as pending (keep a local pending set)
and only add ids to fetchedCallIdsRef after their fetch succeeds (or on final
resolution), use functional state updaters for setCallDispatchesMap and
setLoadingCallIds to merge results atomically, and include a cleanup/abort flag
in the effect so resolved promises only update state when still mounted;
reference fetchedCallIdsRef, toFetch, getCallExtraData, setCallDispatchesMap and
setLoadingCallIds to locate where to implement these changes.
In `@src/components/dispatch-console/personnel-actions-panel.tsx`:
- Line 569: Replace the magic number entityType={1} passed to UdfFieldsRenderer
with the appropriate named UDF entity constant (e.g., UdfEntityType.Personnel or
UDF_ENTITIES.PERSONNEL) by importing the shared enum/constant and using it in
the JSX; update the call site in personnel-actions-panel.tsx where
UdfFieldsRenderer is rendered (component name: UdfFieldsRenderer, prop:
entityType, context: selectedPersonnel.UserId) so the value is self-documenting
and safe to refactor.
- Line 566: Replace the gluestack Icon wrapper with lucide-react-native icons
directly: import ChevronUp and ChevronDown from 'lucide-react-native' and render
the chosen icon based on isAdditionalFieldsExpanded (e.g., render <ChevronUp
.../> or <ChevronDown .../> instead of <Icon as={...}>), preserving equivalent
props (size/color or style) and removing the Icon component usage; update the
JSX in personnel-actions-panel.tsx where Icon, ChevronUp, ChevronDown, and
isAdditionalFieldsExpanded are referenced.
In `@src/components/dispatch-console/unit-actions-panel.tsx`:
- Around line 448-454: The UI uses the gluestack-ui Icon wrapper around
ChevronUp/ChevronDown inside the Pressable/HStack; replace that wrapper by
importing ChevronUp and ChevronDown directly from lucide-react-native and render
them conditionally (based on isAdditionalFieldsExpanded) in place of <Icon
as={...}>; update imports to include ChevronUp and ChevronDown and remove usage
of the Icon component in this block (symbols: Pressable, HStack, Icon,
ChevronUp, ChevronDown, isAdditionalFieldsExpanded,
setIsAdditionalFieldsExpanded, UdfFieldsRenderer).
In `@src/hooks/use-oidc-login.ts`:
- Around line 49-57: The current cast that extracts the authorization code from
response assumes response.params.code exists; instead add an explicit runtime
check before calling AuthSession.exchangeCodeAsync: verify response is non-null,
response.params exists, and response.params.code is a non-empty string (throw or
return an error if not). Update the code path around the exchange call (the
tokenResponse call using AuthSession.exchangeCodeAsync and the variable
request.codeVerifier) to use the validated code variable rather than the direct
type assertion so you never pass undefined to exchangeCodeAsync.
In `@src/hooks/use-saml-login.ts`:
- Around line 14-57: The hook defines startSamlLogin and handleSamlDeepLink
inline which causes new function instances each render; wrap both in
React.useCallback to memoize them (e.g., useCallback(async function
startSamlLogin() { ... }, [idpSsoUrl]) and useCallback(async function
handleSamlDeepLink(url) { ... }, [username, departmentId])) and add any other
external dependencies (Platform, Linking, WebBrowser, externalTokenRequest,
logger) to the dependency arrays as appropriate so consumers can safely use
these functions in effect dependencies.
In `@src/lib/__tests__/platform.test.ts`:
- Around line 111-124: The test mutates global Notification without restoring it
which can leak state; update the test in src/lib/__tests__/platform.test.ts (the
case using mockPlatformOS and window.__ELECTRON__) to capture the original
global.Notification (e.g., const origNotification = global.Notification), set up
the fake Notification if needed, run the assertions calling
isDesktopNotificationSupported(), and finally restore global.Notification =
origNotification (and remove window.__ELECTRON__) in a finally/after block to
ensure restoration even on failure.
In `@src/models/v4/userDefinedFields/udfDefinitionResult.ts`:
- Around line 1-7: Replace the duplicated Id/PageSize/Status fields in the
UdfDefinitionResult class by inheriting from the shared BaseV4Request type:
update the UdfDefinitionResult declaration to extend BaseV4Request, remove the
public Id, PageSize, and Status properties, and keep only Data:
UdfDefinitionResultData | null; also add the appropriate import for
BaseV4Request so the model aligns with the common v4 response shape.
In `@src/models/v4/userDefinedFields/udfFieldValuesResult.ts`:
- Around line 1-7: UdfFieldValuesResult is re-declaring common response fields
(Id, PageSize, Status); instead extend the shared base class to avoid
duplication and drift: change class UdfFieldValuesResult to extend BaseV4Request
(the same base used by CallQuickTemplatesResult), remove the duplicated
properties (Id, PageSize, Status) from UdfFieldValuesResult, and keep only Data:
UdfFieldValueResultData[] so the model inherits the common fields from
BaseV4Request.
In `@src/services/audio.service.web.ts`:
- Line 38: Replace the unsafe (window as any).webkitAudioContext usage when
creating audioContext: add a proper global type for webkitAudioContext (e.g.,
declare global { interface Window { webkitAudioContext?: typeof AudioContext }
}) or a local type assertion for window that exposes webkitAudioContext, then
instantiate via new (window.AudioContext || window.webkitAudioContext)(); update
the constructor/initialization that assigns this.audioContext so it uses the
typed window.webkitAudioContext instead of any.
In `@src/services/electron-notification.ts`:
- Around line 161-168: The current logic always opens the in-app modal when
payload.eventCode exists
(usePushNotificationModalStore.getState().showNotificationModal), which causes
redundant UX if the app window is focused; modify this block to first check
window focus (e.g., via Electron's BrowserWindow.getFocusedWindow() /
isFocused()) or respect a new config flag (e.g., allowInAppModalWhenFocused) and
only call showNotificationModal when the window is not focused or the config
explicitly allows it; keep the payload.eventCode gating but add the focus/config
conditional around the showNotificationModal invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84957ca2-21a0-4bc4-afe0-31e48f663c97
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (84)
__mocks__/expo-auth-session.ts__mocks__/expo-crypto.ts__mocks__/expo-web-browser.ts__mocks__/react-native-mmkv.tsapp.config.tselectron/main.jselectron/preload.jspackage.jsonsrc/__tests__/app/_layout.auth-guard.test.tsxsrc/__tests__/app/_layout.test.tsxsrc/__tests__/app/call/[id].security.test.tsxsrc/__tests__/app/call/[id].test.tsxsrc/__tests__/app/call/new/address-search.test.tssrc/__tests__/app/call/new/coordinates-search.test.tsxsrc/__tests__/app/call/new/plus-code-search.test.tssrc/__tests__/app/call/new/what3words.test.tsxsrc/__tests__/app/calls.test.tsxsrc/__tests__/app/contacts-pull-to-refresh.integration.test.tsxsrc/__tests__/app/contacts.test.tsxsrc/__tests__/app/index.test.tsxsrc/__tests__/app/initialization.test.tsxsrc/__tests__/app/login/index.test.tsxsrc/__tests__/app/login/index.web.test.tsxsrc/__tests__/app/login/login-form.test.tsxsrc/__tests__/app/protocols.test.tsxsrc/__tests__/app/root/lockscreen.test.tsxsrc/__tests__/app/root/maintenance.test.tsxsrc/__tests__/app/signalr-lifecycle.test.tsxsrc/api/calls/callTemplates.tssrc/api/calls/calls.tssrc/api/forms/forms.tssrc/api/userDefinedFields/userDefinedFields.tssrc/app/call/[id]/edit.tsxsrc/app/call/[id]/edit.web.tsxsrc/app/call/new/index.tsxsrc/app/call/new/index.web.tsxsrc/app/login/index.tsxsrc/app/login/index.web.tsxsrc/app/login/login-form.tsxsrc/app/login/sso.tsxsrc/components/calls/call-card.web.tsxsrc/components/calls/call-form-renderer.tsxsrc/components/calls/call-form-renderer.web.tsxsrc/components/calls/call-templates-modal.tsxsrc/components/calls/contact-picker-modal.tsxsrc/components/calls/linked-calls-modal.tsxsrc/components/calls/protocol-selector-modal.tsxsrc/components/calls/udf-fields-renderer.tsxsrc/components/contacts/contact-details-sheet.tsxsrc/components/dispatch-console/active-calls-panel.tsxsrc/components/dispatch-console/personnel-actions-panel.tsxsrc/components/dispatch-console/unit-actions-panel.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsxsrc/hooks/use-oidc-login.tssrc/hooks/use-saml-login.tssrc/lib/__tests__/platform.test.tssrc/lib/auth/api.tsxsrc/lib/auth/types.tsxsrc/lib/hooks/use-selected-theme.web.tsxsrc/lib/platform.tssrc/lib/storage/index.web.tsxsrc/models/v4/forms/formsResult.tssrc/models/v4/personnel/personnelInfoResultData.tssrc/models/v4/templates/callQuickTemplateResultData.tssrc/models/v4/templates/callQuickTemplatesResult.tssrc/models/v4/units/unitInfoResultData.tssrc/models/v4/userDefinedFields/udfDefinitionResult.tssrc/models/v4/userDefinedFields/udfDefinitionResultData.tssrc/models/v4/userDefinedFields/udfFieldResultData.tssrc/models/v4/userDefinedFields/udfFieldValueInput.tssrc/models/v4/userDefinedFields/udfFieldValueResultData.tssrc/models/v4/userDefinedFields/udfFieldValuesResult.tssrc/services/__tests__/electron-notification.test.tssrc/services/audio.service.web.tssrc/services/electron-notification.tssrc/services/push-notification.tssrc/services/sso-discovery.tssrc/stores/auth/store.tsxsrc/stores/personnel/__tests__/store.test.tssrc/translations/ar.jsonsrc/translations/en.jsonsrc/translations/es.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
src/components/calls/linked-calls-modal.tsx (4)
118-127: Use ternary operator instead of&&for conditional rendering.The coding guidelines require using ternary operators
(? :)for conditional rendering instead of&&.♻️ Suggested fix
- {!!call.Nature && ( - <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={2}> - {call.Nature} - </Text> - )} - {!!call.Address && ( - <Text style={StyleSheet.flatten([styles.itemAddress, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={1}> - {call.Address} - </Text> - )} + {call.Nature ? ( + <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={2}> + {call.Nature} + </Text> + ) : null} + {call.Address ? ( + <Text style={StyleSheet.flatten([styles.itemAddress, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={1}> + {call.Address} + </Text> + ) : null}As per coding guidelines: "Use ternary operator (? :) for conditional rendering and not &&".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/linked-calls-modal.tsx` around lines 118 - 127, Replace the conditional JSX that uses && with ternary expressions: for the blocks rendering call.Nature and call.Address (in the linked-calls-modal component), change the patterns "!!call.Nature && (<Text ...>{call.Nature}</Text>)" and "!!call.Address && (<Text ...>{call.Address}</Text>)" to use the ternary operator e.g. "call.Nature ? (<Text ...>{call.Nature}</Text>) : null" and "call.Address ? (<Text ...>{call.Address}</Text>) : null"; keep the same props/styles (styles.itemMeta, styles.itemAddress, isDark conditional styles and numberOfLines) and only change the conditional operator to satisfy the guideline.
27-31:loadCallsshould be included in the dependency array or defined inside the effect.The
loadCallsfunction is referenced insideuseEffectbut not listed in the dependency array. While this works becauseloadCallscurrently has no external dependencies, it's fragile and would be flagged byreact-hooks/exhaustive-deps.♻️ Suggested fix: move loadCalls inside useEffect
useEffect(() => { + const loadCalls = async () => { + setIsLoading(true); + try { + const result = await getCalls(); + setCalls(result?.Data ?? []); + } catch { + // silently handle + } finally { + setIsLoading(false); + } + }; + if (isVisible) { loadCalls(); } }, [isVisible]); - - const loadCalls = async () => { - setIsLoading(true); - try { - const result = await getCalls(); - setCalls(result?.Data ?? []); - } catch { - // silently handle - } finally { - setIsLoading(false); - } - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/linked-calls-modal.tsx` around lines 27 - 31, The useEffect references loadCalls but doesn't include it in the dependency array; either add loadCalls to the dependency array or move/inline the loadCalls definition inside the effect so the hook has no external dependencies; update the effect to call the in-effect function when isVisible is true and ensure any values used inside loadCalls are also included in the dependency array (or captured inside the effect) to satisfy react-hooks/exhaustive-deps and avoid stale closures.
98-131: Consider usingFlatListinstead ofScrollViewwith.map()for better performance.For lists that could grow,
FlatListprovides virtualization, which only renders visible items. This improves memory usage and scroll performance, especially on low-end devices. Additionally, usingFlatListwithkeyExtractorand a stablerenderItemfunction avoids the anonymous function issue on line 105.♻️ Suggested refactor using FlatList
+ const renderCallItem = useCallback( + ({ item: call }: { item: CallResultData }) => { + const isSelected = call.CallId === selectedCallId; + return ( + <TouchableOpacity + style={StyleSheet.flatten([styles.item, isDark ? styles.itemDark : styles.itemLight, isSelected ? styles.itemSelected : {}])} + onPress={() => handleSelect(call)} + accessibilityRole="button" + accessibilityLabel={call.Name} + > + <View style={styles.itemRow}> + <Text style={StyleSheet.flatten([styles.callNumber, isDark ? styles.callNumberDark : styles.callNumberLight])}>#{call.Number}</Text> + {isSelected ? ( + <View style={styles.selectedBadge}> + <Text style={styles.selectedBadgeText}>{t('calls.linked_calls.linked', 'Linked')}</Text> + </View> + ) : null} + </View> + <Text style={StyleSheet.flatten([styles.itemName, isDark ? styles.itemNameDark : styles.itemNameLight])}>{call.Name}</Text> + {call.Nature ? ( + <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={2}> + {call.Nature} + </Text> + ) : null} + {call.Address ? ( + <Text style={StyleSheet.flatten([styles.itemAddress, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={1}> + {call.Address} + </Text> + ) : null} + </TouchableOpacity> + ); + }, + [isDark, selectedCallId, handleSelect, t] + ); + + const keyExtractor = useCallback((item: CallResultData) => item.CallId, []);Then replace the ScrollView with:
<FlatList data={filtered} keyExtractor={keyExtractor} renderItem={renderCallItem} showsVerticalScrollIndicator={false} style={styles.list} />As per coding guidelines: "Optimize FlatLists with props like
removeClippedSubviews,maxToRenderPerBatch, andwindowSize" and "Avoid anonymous functions inrenderItemor event handlers to prevent re-renders".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/linked-calls-modal.tsx` around lines 98 - 131, Replace the ScrollView+map pattern with a FlatList to gain virtualization and avoid anonymous render functions: use the existing filtered array as data, implement a stable renderCallItem function that renders each call item (using the same internal JSX as in the current map, referencing handleSelect and selectedCallId), and provide a keyExtractor that returns call.CallId; then swap ScrollView for FlatList and pass props like data={filtered}, renderItem={renderCallItem}, keyExtractor, showsVerticalScrollIndicator={false}, style={styles.list} and optimize with removeClippedSubviews/maxToRenderPerBatch/windowSize as needed.
38-39: Consider providing user feedback on API errors.Silently catching the error means users see "No active calls available" when the API fails, which is indistinguishable from genuinely having no calls. Consider showing an error state or a toast notification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/linked-calls-modal.tsx` around lines 38 - 39, The catch block inside the LinkedCallsModal component currently swallows API errors; update it to surface failures to the user by setting an error state (e.g., apiError via setApiError) or invoking the app's toast/notification helper (e.g., showToast or notifyError) instead of doing nothing; then render a simple error message in the LinkedCallsModal UI or trigger the toast so users can distinguish an API failure from "no active calls available."src/app/login/sso.tsx (1)
384-393: Anonymous functions in props may cause unnecessary re-renders.The inline arrow functions for
onAuthStart,onAuthEnd, andonErrorcreate new function references on each render, potentially causing the child component to re-render unnecessarily. Per coding guidelines, avoid anonymous functions in event handlers to prevent re-renders.♻️ Suggested fix using useCallback
+ const handleOidcAuthStart = useCallback(() => { + setIsAuthenticating(true); + setAuthError(null); + }, []); + + const handleAuthEnd = useCallback(() => setIsAuthenticating(false), []); + + const handleAuthError = useCallback((msg: string) => { + setAuthError(msg); + setIsAuthenticating(false); + }, []); // Then in the JSX: <OidcSignInSection ... - onAuthStart={() => { - setIsAuthenticating(true); - setAuthError(null); - }} - onAuthEnd={() => setIsAuthenticating(false)} + onAuthStart={handleOidcAuthStart} + onAuthEnd={handleAuthEnd} onTokenReceived={handleTokenReceived} - onError={(msg) => { - setAuthError(msg); - setIsAuthenticating(false); - }} + onError={handleAuthError} />Also applies to lines 403-412 for
SamlSignInSection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/sso.tsx` around lines 384 - 393, The inline arrow props for onAuthStart, onAuthEnd, and onError cause new function references each render and can trigger unnecessary re-renders; replace them with stable callbacks (e.g., useCallback hooks) that reference setIsAuthenticating and setAuthError and reuse the existing handleTokenReceived; do the same for the SamlSignInSection handlers to ensure onAuthStart, onAuthEnd, onError are stable across renders and passed as the memoized functions instead of anonymous inline functions.scripts/generate-webview-scripts.js (1)
10-11: Consider adding error handling for missing asset files.If the asset files don't exist (e.g., fresh clone without assets), the script crashes with a generic ENOENT error. Wrapping these reads in try/catch with a helpful message would improve DX.
💡 Suggested improvement
-const jquery = fs.readFileSync(path.join(root, 'assets/js/jquery-3.6.0.min.js'), 'utf8'); -const formRender = fs.readFileSync(path.join(root, 'assets/js/form-render.min.js'), 'utf8'); +const jqueryPath = path.join(root, 'assets/js/jquery-3.6.0.min.js'); +const formRenderPath = path.join(root, 'assets/js/form-render.min.js'); + +if (!fs.existsSync(jqueryPath) || !fs.existsSync(formRenderPath)) { + console.error('Error: Missing required asset files in assets/js/'); + console.error(' - jquery-3.6.0.min.js'); + console.error(' - form-render.min.js'); + process.exit(1); +} + +const jquery = fs.readFileSync(jqueryPath, 'utf8'); +const formRender = fs.readFileSync(formRenderPath, 'utf8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-webview-scripts.js` around lines 10 - 11, The script currently reads assets using fs.readFileSync for variables jquery and formRender without handling missing files; wrap each fs.readFileSync call (or the block that reads both) in a try/catch around the reads in generate-webview-scripts.js, catch ENOENT and other errors, and call console.error with a clear message including the missing filename (e.g., the path passed to fs.readFileSync) and process.exit(1) to fail fast; ensure you reference the same variables (jquery, formRender) so callers still get defined content when successful.src/components/calls/call-templates-modal.tsx (1)
109-130: PreferFlatListwith a memoizedrenderItemhere.This list is data-driven and searchable, so
ScrollViewwill mount every row up front, and the inlineonPressallocates a new handler for each render. Refactoring this section toFlatListwithkeyExtractorand a stablerenderItemwill scale better on lower-end devices.As per coding guidelines, "Optimize FlatLists with props like
removeClippedSubviews,maxToRenderPerBatch, andwindowSize" and "Avoid anonymous functions inrenderItemor event handlers to prevent re-renders".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/call-templates-modal.tsx` around lines 109 - 130, Replace the ScrollView mapping with a FlatList that uses filtered as its data and keyExtractor={(item) => item.Id}, and move the item UI into a memoized renderItem (e.g., a useCallback or a small React.memo Item component) so you no longer create inline handlers or JSX per render; have renderItem call the existing handleSelect(template) via a stable callback (pass template.Id or the template object to a stable onPress handler) and reuse styles.item, styles.itemName, etc.; also add FlatList performance props like removeClippedSubviews, maxToRenderPerBatch, and windowSize to follow the guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/calls/call-form-renderer.web.tsx`:
- Around line 73-78: The sendData function currently builds data by assigning
fields[el.name] = el.value which overwrites duplicate names and loses
multi-value inputs; replace this logic with the FormData API: create a FormData
from the form element (document.getElementById('callForm') or using the same
selector), iterate its entries and for each [name,value] push values into an
array on data[name] if a key already exists (or convert to array when a second
value appears) so checkbox groups, radio groups, and <select multiple> are
preserved, and then use that resulting data object for submission.
- Line 28: The hardcoded "Call Form" title is used both in the document built by
buildHtml and as the iframe title prop; import useTranslation from react-i18next
in the component, call const { t } = useTranslation(), get the localized string
via t('call.form.title'), and pass that localized title into buildHtml (so the
embedded <title> uses it) and into the iframe JSX prop (replacing the hardcoded
title) — update references to buildHtml and the iframe title in
call-form-renderer.web.tsx accordingly.
- Around line 29-30: Replace the external CDN script tags for jQuery/form-render
by bundling or self-hosting the renderer and importing it (remove the <script>
tags and instead import a local or packaged form-render module) to eliminate the
supply-chain risk; in the message handler that currently checks origin (the
handler referencing iframeRef.current), add a strict check that event.source ===
iframeRef.current?.contentWindow before processing messages; and replace the
hardcoded "Call Form" strings with calls to t("Call Form") from react-i18next
(ensure the component imports and uses the t function from useTranslation) so
both occurrences are internationalized.
In `@src/components/calls/call-templates-modal.tsx`:
- Around line 31-37: The searchQuery state persists between modal openings;
update the useEffect that watches isVisible (the block calling loadTemplates) to
also clear searchQuery when isVisible becomes false by calling
setSearchQuery(''); locate the useEffect using isVisible, loadTemplates,
searchQuery and setSearchQuery and add the conditional reset so the query is
cleared whenever the modal is closed.
- Around line 39-48: The loadTemplates function currently swallows errors;
update it to track and surface failures by introducing an error state (e.g.,
error and setError) and setting setError(error) inside the catch block (and
optionally clearing templates or preserving previous state as desired), keep
setIsLoading(false) in finally, and ensure callers/UI render use this error
state to show a failure message and retry action instead of the silent fallback;
reference the loadTemplates function, getAllCallQuickTemplates call, and the
setTemplates / setIsLoading state updates when implementing this change.
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 284-318: The code marks IDs as fetched before getCallExtraData
succeeds, causing transient failures to be treated as permanent; update the
logic so fetchedCallIdsRef.current is only updated after a successful fetch (or
remove the id from fetchedCallIdsRef on fetch failure), and ensure handleRefresh
clears fetchedCallIdsRef and loadingCallIds so retries are possible; change the
toFetch handling around getCallExtraData (the Promise.all block that sets call
dispatches via setCallDispatchesMap and updates setLoadingCallIds) to only add
ids to fetchedCallIdsRef.current when dispatches were retrieved (or re-queue
failed ids), and mirror the same fix for the similar block referenced at the
second occurrence (lines ~357-360) to allow retrying failed/stale dispatch
fetches.
In `@src/lib/auth/api.tsx`:
- Around line 96-98: The truthy check for departmentId can incorrectly skip
valid ID 0; in the function where you build the payload (the block that assigns
data.department_id from departmentId), replace the truthy check "if
(departmentId) { ... }" with an explicit undefined/null check (e.g., check
departmentId !== undefined && departmentId !== null) so that 0 is included and
only absent values are omitted; update the conditional guarding the assignment
to ensure data.department_id = String(departmentId) runs for 0 but not for
undefined/null.
In `@src/services/sso-discovery.ts`:
- Around line 13-15: The current truthy check in the SSO discovery function that
sets params.departmentId (the block checking "if (departmentId) {
params.departmentId = departmentId; }") will drop valid zero values; change the
guard to explicitly test for undefined/null (e.g., departmentId !== undefined &&
departmentId !== null) so 0 is preserved — update the conditional around
params.departmentId assignment in the sso-discovery logic to mirror the explicit
existence check used in api.tsx.
---
Nitpick comments:
In `@scripts/generate-webview-scripts.js`:
- Around line 10-11: The script currently reads assets using fs.readFileSync for
variables jquery and formRender without handling missing files; wrap each
fs.readFileSync call (or the block that reads both) in a try/catch around the
reads in generate-webview-scripts.js, catch ENOENT and other errors, and call
console.error with a clear message including the missing filename (e.g., the
path passed to fs.readFileSync) and process.exit(1) to fail fast; ensure you
reference the same variables (jquery, formRender) so callers still get defined
content when successful.
In `@src/app/login/sso.tsx`:
- Around line 384-393: The inline arrow props for onAuthStart, onAuthEnd, and
onError cause new function references each render and can trigger unnecessary
re-renders; replace them with stable callbacks (e.g., useCallback hooks) that
reference setIsAuthenticating and setAuthError and reuse the existing
handleTokenReceived; do the same for the SamlSignInSection handlers to ensure
onAuthStart, onAuthEnd, onError are stable across renders and passed as the
memoized functions instead of anonymous inline functions.
In `@src/components/calls/call-templates-modal.tsx`:
- Around line 109-130: Replace the ScrollView mapping with a FlatList that uses
filtered as its data and keyExtractor={(item) => item.Id}, and move the item UI
into a memoized renderItem (e.g., a useCallback or a small React.memo Item
component) so you no longer create inline handlers or JSX per render; have
renderItem call the existing handleSelect(template) via a stable callback (pass
template.Id or the template object to a stable onPress handler) and reuse
styles.item, styles.itemName, etc.; also add FlatList performance props like
removeClippedSubviews, maxToRenderPerBatch, and windowSize to follow the
guidelines.
In `@src/components/calls/linked-calls-modal.tsx`:
- Around line 118-127: Replace the conditional JSX that uses && with ternary
expressions: for the blocks rendering call.Nature and call.Address (in the
linked-calls-modal component), change the patterns "!!call.Nature && (<Text
...>{call.Nature}</Text>)" and "!!call.Address && (<Text
...>{call.Address}</Text>)" to use the ternary operator e.g. "call.Nature ?
(<Text ...>{call.Nature}</Text>) : null" and "call.Address ? (<Text
...>{call.Address}</Text>) : null"; keep the same props/styles (styles.itemMeta,
styles.itemAddress, isDark conditional styles and numberOfLines) and only change
the conditional operator to satisfy the guideline.
- Around line 27-31: The useEffect references loadCalls but doesn't include it
in the dependency array; either add loadCalls to the dependency array or
move/inline the loadCalls definition inside the effect so the hook has no
external dependencies; update the effect to call the in-effect function when
isVisible is true and ensure any values used inside loadCalls are also included
in the dependency array (or captured inside the effect) to satisfy
react-hooks/exhaustive-deps and avoid stale closures.
- Around line 98-131: Replace the ScrollView+map pattern with a FlatList to gain
virtualization and avoid anonymous render functions: use the existing filtered
array as data, implement a stable renderCallItem function that renders each call
item (using the same internal JSX as in the current map, referencing
handleSelect and selectedCallId), and provide a keyExtractor that returns
call.CallId; then swap ScrollView for FlatList and pass props like
data={filtered}, renderItem={renderCallItem}, keyExtractor,
showsVerticalScrollIndicator={false}, style={styles.list} and optimize with
removeClippedSubviews/maxToRenderPerBatch/windowSize as needed.
- Around line 38-39: The catch block inside the LinkedCallsModal component
currently swallows API errors; update it to surface failures to the user by
setting an error state (e.g., apiError via setApiError) or invoking the app's
toast/notification helper (e.g., showToast or notifyError) instead of doing
nothing; then render a simple error message in the LinkedCallsModal UI or
trigger the toast so users can distinguish an API failure from "no active calls
available."
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acc7217f-dfdf-4220-936a-5c16d6913d99
⛔ Files ignored due to path filters (2)
assets/js/form-render.min.jsis excluded by!**/*.min.jsassets/js/jquery-3.6.0.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (20)
__mocks__/react-native-mmkv.tselectron/preload.jsscripts/generate-webview-scripts.jssrc/__tests__/app/login/index.web.test.tsxsrc/app/login/index.web.tsxsrc/app/login/sso.tsxsrc/components/calls/call-form-renderer.tsxsrc/components/calls/call-form-renderer.web.tsxsrc/components/calls/call-templates-modal.tsxsrc/components/calls/contact-picker-modal.tsxsrc/components/calls/linked-calls-modal.tsxsrc/components/calls/protocol-selector-modal.tsxsrc/components/dispatch-console/active-calls-panel.tsxsrc/components/dispatch-console/ptt-interface.tsxsrc/lib/auth/api.tsxsrc/lib/hooks/use-selected-theme.web.tsxsrc/lib/storage/index.web.tsxsrc/services/sso-discovery.tssrc/stores/auth/store.tsxsrc/utils/webview-scripts.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/dispatch-console/ptt-interface.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- electron/preload.js
- src/components/calls/protocol-selector-modal.tsx
- src/lib/storage/index.web.tsx
- src/lib/hooks/use-selected-theme.web.tsx
- src/components/calls/contact-picker-modal.tsx
- src/components/calls/call-form-renderer.tsx
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <title>Call Form</title> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and its size
wc -l src/components/calls/call-form-renderer.web.tsxRepository: Resgrid/Dispatch
Length of output: 111
🏁 Script executed:
# Read the file to see the context around lines 28 and 114
cat -n src/components/calls/call-form-renderer.web.tsxRepository: Resgrid/Dispatch
Length of output: 5483
🏁 Script executed:
# Check if translations directory exists
ls -la src/translations/ 2>/dev/null || echo "Directory not found"Repository: Resgrid/Dispatch
Length of output: 353
🏁 Script executed:
# Search for react-i18next usage in other web components
rg "useTranslation|from.*react-i18next" src/components --type tsx -A 2 | head -40Repository: Resgrid/Dispatch
Length of output: 87
🏁 Script executed:
# Specifically check if there are similar web components that localize text
rg "\.web\.tsx" src/components --files | head -10Repository: Resgrid/Dispatch
Length of output: 504
🏁 Script executed:
# Find .web.tsx files correctly
fd "\.web\.tsx$" src/components | head -10Repository: Resgrid/Dispatch
Length of output: 504
🏁 Script executed:
# Search for useTranslation in tsx files with a different flag
rg "useTranslation" src/components -A 2 | head -50Repository: Resgrid/Dispatch
Length of output: 3143
🏁 Script executed:
# Check if call-form-renderer.web.tsx already imports useTranslation
rg "useTranslation|react-i18next" src/components/calls/call-form-renderer.web.tsxRepository: Resgrid/Dispatch
Length of output: 42
🏁 Script executed:
# Look at an example of another web component to see the pattern
cat -n src/components/calls/call-card.web.tsx | head -50Repository: Resgrid/Dispatch
Length of output: 2007
🏁 Script executed:
# Check the en.json translation file for any existing "Call Form" or similar entries
rg "Call Form|call.form" src/translations/en.jsonRepository: Resgrid/Dispatch
Length of output: 130
🏁 Script executed:
# Check what translation key would be appropriate by looking at context
cat -n src/translations/en.json | grep -A 5 -B 5 "title.*Call Form"Repository: Resgrid/Dispatch
Length of output: 464
🏁 Script executed:
# Get the structure of the translation file to understand the key path
python3 << 'EOF'
import json
with open('src/translations/en.json', 'r') as f:
data = json.load(f)
if 'title' in data and isinstance(data['title'], dict):
print("'title' is an object:", json.dumps(data['title'], indent=2)[:200])
elif 'title' in data:
print("'title' value:", data['title'])
# Search for "Call Form" anywhere
for key, value in data.items():
if isinstance(value, dict):
for subkey, subval in value.items():
if isinstance(subval, str) and "Call Form" in subval:
print(f"Found at: {key}.{subkey} = {subval}")
elif isinstance(value, str) and "Call Form" in value:
print(f"Found at: {key} = {value}")
EOFRepository: Resgrid/Dispatch
Length of output: 42
Localize the iframe title and embedded document title using the existing translation key.
Both the HTML document title (line 28) and iframe title attribute (line 114) contain hardcoded "Call Form" text. The translation already exists at call.form.title in the translation dictionary. Import useTranslation from react-i18next, obtain the translated string via t('call.form.title'), and pass it to the buildHtml function to use for both the embedded <title> element and the iframe JSX prop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/calls/call-form-renderer.web.tsx` at line 28, The hardcoded
"Call Form" title is used both in the document built by buildHtml and as the
iframe title prop; import useTranslation from react-i18next in the component,
call const { t } = useTranslation(), get the localized string via
t('call.form.title'), and pass that localized title into buildHtml (so the
embedded <title> uses it) and into the iframe JSX prop (replacing the hardcoded
title) — update references to buildHtml and the iframe title in
call-form-renderer.web.tsx accordingly.
Comment on lines
+39
to
+48
| const loadTemplates = async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const result = await getAllCallQuickTemplates(); | ||
| setTemplates((result?.Data ?? []).filter((t) => !t.IsDisabled)); | ||
| } catch { | ||
| // silently handle | ||
| } finally { | ||
| setIsLoading(false); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t swallow template fetch failures.
The empty catch collapses request failures into whatever state templates already has; on a first load that becomes the same “No templates available” UI as a successful empty response. Please track an explicit error state and render a failure message/retry path instead of silently ignoring the exception.
As per coding guidelines, "Handle errors gracefully and provide user feedback".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/calls/call-templates-modal.tsx` around lines 39 - 48, The
loadTemplates function currently swallows errors; update it to track and surface
failures by introducing an error state (e.g., error and setError) and setting
setError(error) inside the catch block (and optionally clearing templates or
preserving previous state as desired), keep setIsLoading(false) in finally, and
ensure callers/UI render use this error state to show a failure message and
retry action instead of the silent fallback; reference the loadTemplates
function, getAllCallQuickTemplates call, and the setTemplates / setIsLoading
state updates when implementing this change.
Comment on lines
+96
to
+98
| if (departmentId) { | ||
| data.department_id = String(departmentId); | ||
| } |
Contributor
There was a problem hiding this comment.
Truthy check may skip departmentId of 0.
If departmentId=0 is a valid identifier, the current truthy check will exclude it from the request payload. Consider using an explicit undefined check if zero is a valid department ID.
🔧 Suggested fix
- if (departmentId) {
+ if (departmentId !== undefined) {
data.department_id = String(departmentId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (departmentId) { | |
| data.department_id = String(departmentId); | |
| } | |
| if (departmentId !== undefined) { | |
| data.department_id = String(departmentId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/api.tsx` around lines 96 - 98, The truthy check for departmentId
can incorrectly skip valid ID 0; in the function where you build the payload
(the block that assigns data.department_id from departmentId), replace the
truthy check "if (departmentId) { ... }" with an explicit undefined/null check
(e.g., check departmentId !== undefined && departmentId !== null) so that 0 is
included and only absent values are omitted; update the conditional guarding the
assignment to ensure data.department_id = String(departmentId) runs for 0 but
not for undefined/null.
Comment on lines
+13
to
+15
| if (departmentId) { | ||
| params.departmentId = departmentId; | ||
| } |
Contributor
There was a problem hiding this comment.
Same truthy check issue with departmentId.
Consistent with the pattern in api.tsx, if departmentId=0 is valid, it will be excluded.
🔧 Suggested fix
- if (departmentId) {
+ if (departmentId !== undefined) {
params.departmentId = departmentId;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (departmentId) { | |
| params.departmentId = departmentId; | |
| } | |
| if (departmentId !== undefined) { | |
| params.departmentId = departmentId; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/sso-discovery.ts` around lines 13 - 15, The current truthy check
in the SSO discovery function that sets params.departmentId (the block checking
"if (departmentId) { params.departmentId = departmentId; }") will drop valid
zero values; change the guard to explicitly test for undefined/null (e.g.,
departmentId !== undefined && departmentId !== null) so 0 is preserved — update
the conditional around params.departmentId assignment in the sso-discovery logic
to mirror the explicit existence check used in api.tsx.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/dispatch-console/active-calls-panel.tsx (1)
258-261:⚠️ Potential issue | 🟠 MajorScope
selectedCallExtraDatato the matching call before syncing.This still writes whichever
selectedCallExtraDatais currently in the store intocallDispatchesMap[selectedCallId], which can mis-assign dispatches during quick selection changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 258 - 261, The current useEffect writes selectedCallExtraData.Dispatches into callDispatchesMap for selectedCallId without verifying the extra-data belongs to that call; restrict the update by first checking that selectedCallExtraData corresponds to the currently selected call (e.g. compare selectedCallExtraData.callId or selectedCallExtraData.id/CallId to selectedCallId) before calling setCallDispatchesMap, and ensure the effect depends on selectedCallId and the specific Dispatches payload to avoid racey updates in useEffect (refer to the useEffect, selectedCallId, selectedCallExtraData, setCallDispatchesMap and callDispatchesMap symbols).
🧹 Nitpick comments (4)
scripts/generate-vendor-sources.js (1)
19-29: Add error handling for missing source files.The script will crash with an unclear error if the source files don't exist. Consider adding a check to provide a helpful error message.
♻️ Suggested improvement
function generate(srcRelative, outFile, exportName) { + const srcPath = path.join(root, srcRelative); + if (!fs.existsSync(srcPath)) { + console.error(`Error: Source file not found: ${srcRelative}`); + console.error('Please ensure the vendor assets are present in assets/js/'); + process.exit(1); + } - const src = fs.readFileSync(path.join(root, srcRelative), 'utf8'); + const src = fs.readFileSync(srcPath, 'utf8'); const content = '// Auto-generated – do not edit manually.\n' + '// Regenerate via: node scripts/generate-vendor-sources.js\n' + 'const ' + exportName + ': string = ' + JSON.stringify(src) + ';\n' + 'export default ' + exportName + ';\n'; fs.writeFileSync(path.join(outDir, outFile), content, 'utf8'); const size = fs.statSync(path.join(outDir, outFile)).size; console.log('Wrote', outFile, '(' + size + ' bytes)'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-vendor-sources.js` around lines 19 - 29, The generate function currently calls fs.readFileSync on path.join(root, srcRelative) without checking existence; update generate (referencing function generate and variables srcRelative, outFile, exportName, root, outDir) to detect missing source files and fail with a clear message: either wrap the fs.readFileSync in a try/catch and on ENOENT call console.error with the missing srcRelative path and process.exit(1), or use fs.existsSync to check the file before reading and log an explicit error (including srcRelative and the full resolved path) then exit non‑zero so callers get a helpful message instead of an uncaught exception.src/components/calls/call-templates-modal.tsx (2)
112-132: Consider extracting template item to avoid inline arrow function.The
onPress={() => handleSelect(template)}creates a new function reference on each render. While acceptable for a small list in ScrollView, extracting to a memoized item component would follow guidelines more strictly.Optional refactor
const TemplateItem: React.FC<{ template: CallQuickTemplateResultData; onSelect: (t: CallQuickTemplateResultData) => void; isDark: boolean; }> = React.memo(({ template, onSelect, isDark }) => { const { t } = useTranslation(); const handlePress = useCallback(() => onSelect(template), [template, onSelect]); return ( <TouchableOpacity style={StyleSheet.flatten([styles.item, isDark ? styles.itemDark : styles.itemLight])} onPress={handlePress} accessibilityRole="button" accessibilityLabel={template.Name} > {/* ... item content ... */} </TouchableOpacity> ); });As per coding guidelines: "Avoid anonymous functions in
renderItemor event handlers to prevent re-renders".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/call-templates-modal.tsx` around lines 112 - 132, The inline arrow onPress={() => handleSelect(template)} inside the filtered.map creates a new function each render; extract the list item into a memoized component (e.g., TemplateItem) that accepts props (template, onSelect, isDark), call useTranslation() inside it, create a stable handlePress via useCallback(() => onSelect(template), [template, onSelect]) and use React.memo(TemplateItem) to prevent unnecessary re-renders, then replace the inline TouchableOpacity in the map with <TemplateItem key={template.Id} template={template} onSelect={handleSelect} isDark={isDark} /> while preserving styles, accessibilityLabel, numberOfLines, and displayed fields (Name, CallName, CallNature).
121-130: Use ternary operator for conditional rendering.The coding guidelines specify using ternary operators instead of
&&for conditional rendering.Suggested fix
- {!!template.CallName && ( - <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])}> + {template.CallName ? ( + <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])}> {t('calls.name')}: {template.CallName} </Text> - )} - {!!template.CallNature && ( - <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={2}> + ) : null} + {template.CallNature ? ( + <Text style={StyleSheet.flatten([styles.itemMeta, isDark ? styles.itemMetaDark : styles.itemMetaLight])} numberOfLines={2}> {t('calls.nature')}: {template.CallNature} </Text> - )} + ) : null}As per coding guidelines: "Use ternary operator (? :) for conditional rendering and not && operator".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calls/call-templates-modal.tsx` around lines 121 - 130, Replace the conditional renderings that use the && operator with ternary expressions that return the Text element when truthy or null when falsy; specifically update the blocks rendering template.CallName and template.CallNature in the CallTemplatesModal component so they use ? : expressions (preserving props like style, numberOfLines, and calls to t('calls.name') / t('calls.nature')) and keep existing style calculations (styles.itemMeta, styles.itemMetaDark, styles.itemMetaLight) and the isDark check intact.src/components/dispatch-console/active-calls-panel.tsx (1)
89-92: Use stableonLayoutcallbacks instead of inline handlers.These inline event handlers are recreated on each render and can add churn in a list-heavy panel.
As per coding guidelines, "Avoid anonymous functions in
renderItemor event handlers to prevent re-renders."Also applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 89 - 92, Replace the inline onLayout lambdas with a stable, memoized callback: create a useCallback handler (e.g., handleLayout) that sets containerWidthRef.current = e.nativeEvent.layout.width and calls startAnim(), include startAnim in the dependency array (containerWidthRef is a ref so it need not be a dep), then pass handleLayout to the onLayout prop; do the same refactor for the other onLayout usage currently at the later block so both handlers are stable across renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 295-321: The Promise.all results handler can be overwritten by
stale in-flight responses; add an epoch/token guard to the fetch cycle: create a
fetchEpochRef (or reuse an existing ref), increment it when starting the toFetch
batch, capture the current epoch in the closure before calling
Promise.all(getCallExtraData...), and inside the .then(...) compare the captured
epoch to fetchEpochRef.current and return early if they differ; only then call
setCallDispatchesMap, fetchedCallIdsRef.current.delete, and setLoadingCallIds so
stale responses don't mutate state. Apply the same epoch-guard pattern to the
other fetch block referenced (around the 362-367 logic).
---
Duplicate comments:
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 258-261: The current useEffect writes
selectedCallExtraData.Dispatches into callDispatchesMap for selectedCallId
without verifying the extra-data belongs to that call; restrict the update by
first checking that selectedCallExtraData corresponds to the currently selected
call (e.g. compare selectedCallExtraData.callId or
selectedCallExtraData.id/CallId to selectedCallId) before calling
setCallDispatchesMap, and ensure the effect depends on selectedCallId and the
specific Dispatches payload to avoid racey updates in useEffect (refer to the
useEffect, selectedCallId, selectedCallExtraData, setCallDispatchesMap and
callDispatchesMap symbols).
---
Nitpick comments:
In `@scripts/generate-vendor-sources.js`:
- Around line 19-29: The generate function currently calls fs.readFileSync on
path.join(root, srcRelative) without checking existence; update generate
(referencing function generate and variables srcRelative, outFile, exportName,
root, outDir) to detect missing source files and fail with a clear message:
either wrap the fs.readFileSync in a try/catch and on ENOENT call console.error
with the missing srcRelative path and process.exit(1), or use fs.existsSync to
check the file before reading and log an explicit error (including srcRelative
and the full resolved path) then exit non‑zero so callers get a helpful message
instead of an uncaught exception.
In `@src/components/calls/call-templates-modal.tsx`:
- Around line 112-132: The inline arrow onPress={() => handleSelect(template)}
inside the filtered.map creates a new function each render; extract the list
item into a memoized component (e.g., TemplateItem) that accepts props
(template, onSelect, isDark), call useTranslation() inside it, create a stable
handlePress via useCallback(() => onSelect(template), [template, onSelect]) and
use React.memo(TemplateItem) to prevent unnecessary re-renders, then replace the
inline TouchableOpacity in the map with <TemplateItem key={template.Id}
template={template} onSelect={handleSelect} isDark={isDark} /> while preserving
styles, accessibilityLabel, numberOfLines, and displayed fields (Name, CallName,
CallNature).
- Around line 121-130: Replace the conditional renderings that use the &&
operator with ternary expressions that return the Text element when truthy or
null when falsy; specifically update the blocks rendering template.CallName and
template.CallNature in the CallTemplatesModal component so they use ? :
expressions (preserving props like style, numberOfLines, and calls to
t('calls.name') / t('calls.nature')) and keep existing style calculations
(styles.itemMeta, styles.itemMetaDark, styles.itemMetaLight) and the isDark
check intact.
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 89-92: Replace the inline onLayout lambdas with a stable, memoized
callback: create a useCallback handler (e.g., handleLayout) that sets
containerWidthRef.current = e.nativeEvent.layout.width and calls startAnim(),
include startAnim in the dependency array (containerWidthRef is a ref so it need
not be a dep), then pass handleLayout to the onLayout prop; do the same refactor
for the other onLayout usage currently at the later block so both handlers are
stable across renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3110fb1-3de7-47d2-9bf3-d2732bc38e16
📒 Files selected for processing (6)
scripts/generate-vendor-sources.jssrc/components/calls/call-form-renderer.web.tsxsrc/components/calls/call-templates-modal.tsxsrc/components/dispatch-console/active-calls-panel.tsxsrc/lib/form-render/form-render-source.tssrc/lib/form-render/jquery-source.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/dispatch-console/active-calls-panel.tsx (1)
257-263:⚠️ Potential issue | 🟠 MajorKeep store-driven dispatch updates scoped to the source call.
selectedCallExtraDatainuseDispatchConsoleStorestill has no companioncallId, so a late write for call A can be mirrored into whateverselectedCallIdprop is current here. Line 261 can therefore populate call B with call A's dispatches again. Store{ callId, data }together, or move this merge into the code path that fetched the extra data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 257 - 263, selectedCallExtraData from useDispatchConsoleStore lacks an associated callId which can cause stale data for call A to be merged into whatever selectedCallId is current; modify the flow so updates are scoped: either change the store shape to return { callId, data } (update useDispatchConsoleStore and the producer that writes selectedCallExtraData) and only merge when selectedCallId === selectedCallExtraData.callId, or (alternatively) perform the setCallDispatchesMap merge at the point where the extra data is fetched/produced (the code path that writes selectedCallExtraData) so the store update is already tied to the originating call; ensure checks reference selectedCallExtraData, selectedCallExtraData.callId, selectedCallId, and setCallDispatchesMap to locate and fix the logic.
🧹 Nitpick comments (1)
src/components/dispatch-console/active-calls-panel.tsx (1)
279-305: Consider avoiding N+1getCallExtraData()calls from the active calls list.This effect now issues one extra request per active call, and
getCallExtraData()returns the full extra-data payload even though this panel only rendersDispatches. On a busy board, initial load and every refresh become a burst of heavyweight calls. A dispatch-summary endpoint, lazy-loading only visible rows, or at least capping concurrency would scale much better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 279 - 305, The effect eagerly issues an N+1 heavy getCallExtraData() per active call; replace that with a scalable approach: fetch only dispatch summaries (call a new API like getDispatchSummariesForCalls(callIds) that returns {callId, dispatches} for many ids) or at minimum limit concurrency when calling getCallExtraData() (use a concurrency limiter to batch calls) and lazy-load only visible rows; update references to fetchedCallIdsRef, setLoadingCallIds and fetchEpochRef to consume the batched/limited results and preserve the existing result shape ({ callId, dispatches }) so the downstream .then(results) logic remains valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 285-287: The global fetch epoch advancement is causing in-flight
calls to be permanently marked fetched (fetchedCallIdsRef) when a later batch
starts; change the logic in the batching/fetch flow so you do not advance the
shared/global epoch for each new batch started. Either (a) only bump the global
epoch on an explicit full refresh (e.g., the refresh action that should
invalidate everything) and leave the existing per-batch requests to complete and
clear their loading state, or (b) implement per-call staleness tracking by
storing a small map of request-specific epochs/IDs (e.g., callFetchEpochs keyed
by call id) and have the completion handler compare the request's epoch against
the current epoch before early-returning so that resolving requests still clear
their own loading state even if a later batch started; update code that adds ids
to fetchedCallIdsRef and the completion/early-return checks to use this per-call
epoch logic.
- Around line 94-97: The ticker placeholder currently uses a hardcoded
translucent white which is unreadable on light priority cards; update the
rendering for the RNText with styles.tickerPlaceholder in active-calls-panel.tsx
(the branches using isLoading and dispatches.length) to derive its color from
the card/component foreground/text color rather than fixed white—use the
computed foreground/textColor from the component/theme and apply the needed
translucency (alpha) so the placeholder follows the same readable foreground
contrast as the rest of the card.
- Around line 60-77: The ticker animation started in startAnim (using animRef
and translateX) can continue running when the component renders placeholders;
add a useEffect that depends on isLoading and dispatches.length to stop and
clear animRef.current whenever isLoading becomes true or dispatches becomes
empty (and also stop on cleanup/unmount): check animRef.current and call
animRef.current.stop(), then set animRef.current = undefined to prevent
background JS-driven loops (especially on web where useNativeDriver is false).
---
Duplicate comments:
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 257-263: selectedCallExtraData from useDispatchConsoleStore lacks
an associated callId which can cause stale data for call A to be merged into
whatever selectedCallId is current; modify the flow so updates are scoped:
either change the store shape to return { callId, data } (update
useDispatchConsoleStore and the producer that writes selectedCallExtraData) and
only merge when selectedCallId === selectedCallExtraData.callId, or
(alternatively) perform the setCallDispatchesMap merge at the point where the
extra data is fetched/produced (the code path that writes selectedCallExtraData)
so the store update is already tied to the originating call; ensure checks
reference selectedCallExtraData, selectedCallExtraData.callId, selectedCallId,
and setCallDispatchesMap to locate and fix the logic.
---
Nitpick comments:
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 279-305: The effect eagerly issues an N+1 heavy getCallExtraData()
per active call; replace that with a scalable approach: fetch only dispatch
summaries (call a new API like getDispatchSummariesForCalls(callIds) that
returns {callId, dispatches} for many ids) or at minimum limit concurrency when
calling getCallExtraData() (use a concurrency limiter to batch calls) and
lazy-load only visible rows; update references to fetchedCallIdsRef,
setLoadingCallIds and fetchEpochRef to consume the batched/limited results and
preserve the existing result shape ({ callId, dispatches }) so the downstream
.then(results) logic remains valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d852328-2215-4631-af3a-68b6a0b06b82
📒 Files selected for processing (1)
src/components/dispatch-console/active-calls-panel.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/dispatch-console/active-calls-panel.tsx (4)
215-216: Remove misleadingvoidstatement.
isFilterActiveis actually used on lines 222 and 228 for styling and conditional rendering, so thevoid isFilterActivestatement on line 216 is incorrect and misleading. This comment suggests the variable is unused when it is not.♻️ Suggested fix
-}) => { - void isFilterActive; // kept in props for selection badge only +}) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 215 - 216, Remove the incorrect no-op "void isFilterActive" statement from the Active Calls Panel props destructure; it falsely marks isFilterActive as unused even though the prop is later referenced for styling/conditional rendering (see uses of isFilterActive in the component). Simply delete the "void isFilterActive" line so the isFilterActive prop is treated as an active variable by the component (no other code changes needed).
273-277: Guard against overwriting cached dispatches with stale store data.When
selectedCallExtraData?.Dispatchesis truthy, this effect overwrites the local cache forselectedCallId. However, if SignalR pushes an update for call A while call B is selected, and then user switches to call A, this could momentarily show stale data from the store before the per-call fetch completes.Consider also checking that the store's extra data actually belongs to the selected call (if such an identifier is available in the store), or only sync when the store data is more recent than the cached data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 273 - 277, The effect that updates callDispatchesMap inside useEffect can overwrite a fresher per-call cache with stale selectedCallExtraData.Dispatches; modify the logic in the useEffect watching selectedCallId and selectedCallExtraData so it only writes into setCallDispatchesMap when the incoming store data actually corresponds to the selected call (e.g. compare an identifier on selectedCallExtraData to selectedCallId or a callId field) or when the store payload is demonstrably newer than the existing entry in callDispatchesMap (e.g. compare timestamps/version), and skip the update if the cached entry for selectedCallId is newer or already present to avoid replacing fresh per-call fetch results for functions/variables referenced: useEffect, selectedCallId, selectedCallExtraData, setCallDispatchesMap, callDispatchesMap.
642-645: Type cast workaround is acceptable but consider alternative.The
as unknown as numbercast forheight: '100%'is a workaround for TypeScript's strict StyleSheet typing. While this works at runtime, an alternative is to useflex: 1if the parent has a defined height, which avoids the type assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 642 - 645, The dispatchBadgeDivider style currently uses a type-casted height ('100%' as unknown as number); update the dispatchBadgeDivider style to avoid the unsafe cast by replacing height with a layout-aware alternative (e.g., use flex: 1 or set an explicit numeric height) so TypeScript StyleSheet typing is satisfied; modify the dispatchBadgeDivider object in active-calls-panel.tsx (the dispatchBadgeDivider style) to use flex: 1 (assuming the parent has a defined height) or an explicit numeric height instead of the '100%' type assertion.
461-476: Consider extracting callback handlers to avoid inline anonymous functions.The inline
onPressandonOpenDetailshandlers create new function references on each render, which could trigger unnecessary re-renders ifCallItemWrapperwere memoized. While the current implementation works, extracting these touseCallbackhandlers would be a performance optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dispatch-console/active-calls-panel.tsx` around lines 461 - 476, Extract the inline closures passed to CallItemWrapper by creating memoized handlers with useCallback: add e.g. const handlePress = useCallback((callId: string) => () => onSelectCall?.(callId), [onSelectCall]) and const handleOpenDetails = useCallback((callId: string) => () => handleOpenCallDetails(callId), [handleOpenCallDetails]), then in the activeCalls.map pass onPress={handlePress(call.CallId)} and onOpenDetails={handleOpenDetails(call.CallId)} so CallItemWrapper no longer receives new anonymous functions each render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/dispatch-console/active-calls-panel.tsx`:
- Around line 215-216: Remove the incorrect no-op "void isFilterActive"
statement from the Active Calls Panel props destructure; it falsely marks
isFilterActive as unused even though the prop is later referenced for
styling/conditional rendering (see uses of isFilterActive in the component).
Simply delete the "void isFilterActive" line so the isFilterActive prop is
treated as an active variable by the component (no other code changes needed).
- Around line 273-277: The effect that updates callDispatchesMap inside
useEffect can overwrite a fresher per-call cache with stale
selectedCallExtraData.Dispatches; modify the logic in the useEffect watching
selectedCallId and selectedCallExtraData so it only writes into
setCallDispatchesMap when the incoming store data actually corresponds to the
selected call (e.g. compare an identifier on selectedCallExtraData to
selectedCallId or a callId field) or when the store payload is demonstrably
newer than the existing entry in callDispatchesMap (e.g. compare
timestamps/version), and skip the update if the cached entry for selectedCallId
is newer or already present to avoid replacing fresh per-call fetch results for
functions/variables referenced: useEffect, selectedCallId,
selectedCallExtraData, setCallDispatchesMap, callDispatchesMap.
- Around line 642-645: The dispatchBadgeDivider style currently uses a
type-casted height ('100%' as unknown as number); update the
dispatchBadgeDivider style to avoid the unsafe cast by replacing height with a
layout-aware alternative (e.g., use flex: 1 or set an explicit numeric height)
so TypeScript StyleSheet typing is satisfied; modify the dispatchBadgeDivider
object in active-calls-panel.tsx (the dispatchBadgeDivider style) to use flex: 1
(assuming the parent has a defined height) or an explicit numeric height instead
of the '100%' type assertion.
- Around line 461-476: Extract the inline closures passed to CallItemWrapper by
creating memoized handlers with useCallback: add e.g. const handlePress =
useCallback((callId: string) => () => onSelectCall?.(callId), [onSelectCall])
and const handleOpenDetails = useCallback((callId: string) => () =>
handleOpenCallDetails(callId), [handleOpenCallDetails]), then in the
activeCalls.map pass onPress={handlePress(call.CallId)} and
onOpenDetails={handleOpenDetails(call.CallId)} so CallItemWrapper no longer
receives new anonymous functions each render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7322a006-1d9f-454d-a968-7d6ca94ee986
📒 Files selected for processing (1)
src/components/dispatch-console/active-calls-panel.tsx
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores