Conversation
📝 WalkthroughWalkthroughAdds complete SSO (OIDC + SAML) flows (hooks, UI, store, API), deep-link/URL-scheme support, multi-day calendar/date utilities and rendering updates, new translations and tests, plus related dependency and Expo config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App (React Native)
participant IdP as Identity Provider
participant API as Resgrid API
rect rgba(100,150,200,0.5)
Note over User,API: OIDC Authorization Code + PKCE Flow
User->>App: Start SSO (OIDC)
App->>API: fetchUserSsoConfig(username)
API-->>App: Return authority/clientId
App->>IdP: promptAsync() (opens browser)
IdP-->>App: Redirect to app with code
App->>IdP: exchangeCodeAsync(code, verifier)
IdP-->>App: Return id_token
App->>API: externalTokenRequest(provider:'oidc', external_token:id_token)
API-->>App: Return LoginResponse (auth token)
App->>App: Store token & navigate
end
rect rgba(150,200,100,0.5)
Note over User,API: SAML2 Browser-Based Flow
User->>App: Start SSO (SAML)
App->>API: fetchUserSsoConfig(username)
API-->>App: Return metadata/entityId
App->>App: Persist dept code & open IdP SSO URL
IdP-->>App: Deep link with saml_response
App->>App: handleSamlCallbackUrl(url)
App->>API: externalTokenRequest(provider:'saml2', external_token:saml_response)
API-->>App: Return LoginResponse (auth token)
App->>App: Clear stored dept & store token
end
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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (20)
src/hooks/__tests__/use-saml-login.test.ts (1)
106-158: Remove the duplicateddescribe('handleSamlCallbackUrl')block.The second suite repeats the same cases and assertions, increasing test runtime and maintenance cost without adding coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/__tests__/use-saml-login.test.ts` around lines 106 - 158, Remove the duplicated test suite for describe('handleSamlCallbackUrl') so only one suite remains; locate the repeated block that contains the same beforeEach and four it(...) tests (cases for missing saml_response, no pending dept code, successful login clears storage, and loginWithSso throwing) and delete the second copy, ensuring the remaining describe retains the mockedParse/mockGetItem/mockLoginWithSso usages and assertions including mockRemoveItem and PENDING_SAML_DEPT_CODE_KEY checks.app.config.ts (1)
29-29: Consolidate hardcoded scheme values to a single source of truth.
'resgrid'is hardcoded in three locations (app.config.ts lines 29 and 83, use-oidc-login.ts line 33), whileEnv.SCHEME('ResgridRespond') is separately maintained. Extract a shared scheme constant for the deep-link callback and android intent filter to prevent drift when schemes change.Also applies to: 80-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.config.ts` at line 29, Create a single exported scheme constant (e.g., APP_SCHEME or SCHEME) and use it everywhere instead of the mixed hardcoded 'resgrid' and Env.SCHEME; update the scheme array in the app.config.ts "scheme" property, replace the hardcoded 'resgrid' occurrences around lines 80-86, and use the same exported constant in use-oidc-login (where the deep-link callback is constructed) and the Android intent filter code so all places reference the one shared symbol.src/components/calendar/__tests__/calendar-item-details-sheet.test.tsx (1)
243-250: RedundantmockReset()calls can be simplified.The
mockTrackEvent.mockReset()is called 5 times consecutively (lines 245-249). A single call is sufficient.Proposed fix
beforeEach(() => { jest.clearAllMocks(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); + mockTrackEvent.mockReset(); mockUseTranslation.mockReturnValue({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/__tests__/calendar-item-details-sheet.test.tsx` around lines 243 - 250, In beforeEach(), remove the four redundant consecutive calls to mockTrackEvent.mockReset() so there is only a single mockTrackEvent.mockReset() call (keep the existing jest.clearAllMocks() as-is); this simplifies the setup in the test file where beforeEach and mockTrackEvent.mockReset() are used.src/components/calendar/__tests__/calendar-item-details-sheet-minimal.test.tsx (1)
134-144: RedundantmockReset()calls can be simplified.The
mockTrackEvent.mockReset()is called 5 times consecutively (lines 136-140). A single call is sufficient.Proposed fix
beforeEach(() => { jest.clearAllMocks(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); + mockTrackEvent.mockReset(); (useAnalytics as jest.Mock).mockReturnValue({ trackEvent: mockTrackEvent, }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/__tests__/calendar-item-details-sheet-minimal.test.tsx` around lines 134 - 144, The beforeEach block calls mockTrackEvent.mockReset() five times redundantly; replace the repeated calls with a single mockTrackEvent.mockReset() call inside the beforeEach (which also already calls jest.clearAllMocks()), keeping the existing (useAnalytics as jest.Mock).mockReturnValue({ trackEvent: mockTrackEvent }) line and leaving mockTrackEvent and useAnalytics identifiers unchanged.src/app/login/sso.tsx (2)
38-42: Clarify fallback logic foridpSsoUrl.The fallback
ssoConfig?.metadataUrl ?? ssoConfig?.authority ?? ''may be confusing. IfmetadataUrlis the preferred SAML endpoint, consider adding a comment explaining whyauthorityis used as a fallback.🤖 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 38 - 42, The idpSsoUrl fallback is unclear; update the call to useSamlLogin (the idpSsoUrl argument) to include a short clarifying comment and explicit intent that metadataUrl is preferred and authority is a fallback, e.g., annotate why ssoConfig?.authority is acceptable if metadataUrl is missing; reference the useSamlLogin call and the idpSsoUrl, ssoConfig?.metadataUrl and ssoConfig?.authority symbols so reviewers can see the rationale next to the code and avoid changing the fallback behavior unintentionally.
65-76: Missing dependency in useEffect may cause stale closure.The
oidc.exchangeCodeForResgridTokenfunction is called inside the effect but not listed in the dependency array. While the eslint-disable comment acknowledges this, the current pattern could use a stale reference if the hook recreates the function. Consider extracting the function reference or restructuring to avoid the lint suppression.🤖 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 65 - 76, The effect is using oidc.exchangeCodeForResgridToken without listing it in dependencies which risks a stale closure; destructure the needed members from oidc (e.g. const { response, exchangeCodeForResgridToken } = oidc) and update the effect to reference response and exchangeCodeForResgridToken, include both in the dependency array, remove the eslint-disable comment, and keep the existing error handling (logger.error and setIsErrorModalVisible) inside the effect so the call uses the current function reference.src/app/login/__tests__/login-form.test.tsx (2)
203-211: RedundantmockReset()andmockClear()calls can be simplified.The
mockTrackEvent.mockReset()is called 5 times consecutively (lines 204-208), followed bymockClear(). A singlemockReset()(which also clears) is sufficient.Proposed fix
beforeEach(() => { jest.clearAllMocks(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockClear(); + mockTrackEvent.mockReset(); mockServerUrlBottomSheet.mockClear(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/__tests__/login-form.test.tsx` around lines 203 - 211, The teardown block currently calls jest.clearAllMocks() then mockTrackEvent.mockReset() five times and mockTrackEvent.mockClear(); simplify by keeping a single call to mockTrackEvent.mockReset() (mockReset already clears history) and removing the repeated mockReset/mockClear calls, and keep the single mockServerUrlBottomSheet.mockClear() as-is; update the block referencing mockTrackEvent and mockServerUrlBottomSheet to reflect this change.
213-249: Consider adding explicit test for SSO button behavior.The tests verify button count but don't explicitly test the SSO button's presence or behavior. Adding a test case that verifies the SSO button triggers navigation to the SSO flow would improve coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/__tests__/login-form.test.tsx` around lines 213 - 249, Add a new test that explicitly verifies the SSO button behavior: render LoginForm with mockOnSubmit, locate the SSO button via screen.getAllByTestId('button')[2] (the third button), fireEvent.press on it, then assert mockTrackEvent was called with an SSO-specific event (e.g., 'login_sso_pressed' and timestamp) and assert the SSO navigation handler was invoked by creating/using a jest mock like mockNavigateToSSO (or the existing navigation prop/handler used by LoginForm) and expecting it toHaveBeenCalled(); this ensures LoginForm, mockOnSubmit, mockTrackEvent and the SSO navigation mock are exercised.src/components/calendar/__tests__/calendar-item-details-sheet-analytics.test.tsx (1)
209-214: Remove redundantmockReset()calls.
jest.clearAllMocks()on line 209 already resets all mocks. The five consecutivemockTrackEvent.mockReset()calls are redundant.🧹 Suggested cleanup
beforeEach(() => { jest.clearAllMocks(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); - mockTrackEvent.mockReset(); mockUseTranslation.mockReturnValue({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/__tests__/calendar-item-details-sheet-analytics.test.tsx` around lines 209 - 214, The test currently calls jest.clearAllMocks() then repeats mockTrackEvent.mockReset() five times; remove the redundant mockReset() calls (delete all extra mockTrackEvent.mockReset() lines) since jest.clearAllMocks() already resets mocks—if you need to ensure only mockTrackEvent is reset instead of all mocks, replace the repeated calls with a single mockTrackEvent.mockReset() and keep jest.clearAllMocks() usage consistent in the test setup where mockTrackEvent is referenced.src/components/calendar/compact-calendar-item.tsx (1)
78-94: Consider extracting the IIFE into a named helper for readability.The inline IIFE for computing the end date works correctly but reduces readability. A helper function or
useMemowould make the render logic cleaner.♻️ Suggested refactor
+ const formatEndDate = () => { + if (item.IsAllDay) { + const lastDayStr = resolveAllDayEndDate(item.End); + const parts = lastDayStr.split('-'); + const lastDay = new Date(parseInt(parts[0] ?? '0', 10), parseInt(parts[1] ?? '0', 10) - 1, parseInt(parts[2] ?? '0', 10)); + return lastDay.toLocaleDateString([], { month: 'short', day: 'numeric' }); + } + return formatDate(item.End, false); + }; + return ( <Pressable onPress={onPress} testID={testID} className="mb-2"> ... {item.IsMultiDay ? ( <Text className="text-xs text-gray-600 dark:text-gray-300"> {formatDate(item.Start, item.IsAllDay)} {' – '} - {(() => { - if (item.IsAllDay) { - const lastDayStr = resolveAllDayEndDate(item.End); - const parts = lastDayStr.split('-'); - const lastDay = new Date(parseInt(parts[0] ?? '0', 10), parseInt(parts[1] ?? '0', 10) - 1, parseInt(parts[2] ?? '0', 10)); - return lastDay.toLocaleDateString([], { month: 'short', day: 'numeric' }); - } - return formatDate(item.End, false); - })()} + {formatEndDate()} </Text> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/compact-calendar-item.tsx` around lines 78 - 94, The inline IIFE in the JSX that computes the displayed end date reduces readability; extract that logic into a named helper (e.g., getDisplayEndDate or computeEndDateDisplay) that accepts (end: string, isAllDay: boolean) and internally uses resolveAllDayEndDate and the same parsing/Date->toLocaleDateString flow (or calls formatDate for non-all-day), then replace the IIFE in compact-calendar-item's render with a call to this helper (or wrap it in useMemo if you want to avoid recomputation based on item.End and item.IsAllDay).src/components/calendar/calendar-card.tsx (1)
98-125: Consider extracting the IIFE into a named function.The inline IIFE (lines 102-110) for multi-day end date formatting works correctly but reduces readability. Consider extracting it to a named helper function for clarity.
✏️ Suggested refactor
+ const formatEndDate = () => { + if (item.IsAllDay) { + const lastDayStr = resolveAllDayEndDate(item.End); + const parts = lastDayStr.split('-'); + const lastDay = new Date(parseInt(parts[0] ?? '0', 10), parseInt(parts[1] ?? '0', 10) - 1, parseInt(parts[2] ?? '0', 10)); + return lastDay.toLocaleDateString([], { weekday: 'short', month: 'short', day: 'numeric' }); + } + return formatDate(item.End, false); + }; + // In render: {item.IsMultiDay ? ( <Text className="text-sm text-gray-600 dark:text-gray-300"> {formatDate(item.Start, item.IsAllDay)} {' – '} - {(() => { - if (item.IsAllDay) { - const lastDayStr = resolveAllDayEndDate(item.End); - const parts = lastDayStr.split('-'); - const lastDay = new Date(parseInt(parts[0] ?? '0', 10), parseInt(parts[1] ?? '0', 10) - 1, parseInt(parts[2] ?? '0', 10)); - return lastDay.toLocaleDateString([], { weekday: 'short', month: 'short', day: 'numeric' }); - } - return formatDate(item.End, false); - })()} + {formatEndDate()} </Text> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/calendar-card.tsx` around lines 98 - 125, Extract the inline IIFE used to render the multi-day end date into a descriptive helper (e.g., formatMultiDayEndDate or getMultiDayEndDisplay) that accepts the event end and IsAllDay flag and returns the formatted string; replace the IIFE in the JSX with a call to that helper, reusing resolveAllDayEndDate and the same date-parsing/formatting logic (and keep formatDate for non-all-day end dates) so the JSX around item.IsMultiDay, formatDate, resolveAllDayEndDate, and getEventDuration remains unchanged but much clearer.src/stores/calendar/__tests__/store.test.ts (1)
139-144: Minor: Underscore prefix on used parameter.The
_eventEndparameter has an underscore prefix (typically indicating unused), but it's actually used on line 142. Consider renaming toeventEndfor clarity.✏️ Suggested fix
- mockedUtils.isDateInRange.mockImplementation((date: string, eventStart: string, _eventEnd: string) => { + mockedUtils.isDateInRange.mockImplementation((date: string, eventStart: string, eventEnd: string) => { const dateStr = date.slice(0, 10); const startStr = eventStart.slice(0, 10); - const endStr = _eventEnd.slice(0, 10); + const endStr = eventEnd.slice(0, 10); return dateStr >= startStr && dateStr <= endStr; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/calendar/__tests__/store.test.ts` around lines 139 - 144, The mock implementation for mockedUtils.isDateInRange uses a parameter named _eventEnd even though it’s used; rename the parameter to eventEnd in the mockImplementation signature and update its internal usage (the slice/endStr reference) to match, e.g., change (_eventEnd: string) to (eventEnd: string) so the parameter name accurately reflects usage in mockedUtils.isDateInRange.src/stores/auth/store.tsx (1)
81-159: Consider extracting shared JWT processing logic.Both
loginandloginWithSsocontain identical JWT decoding and state update logic (lines 86-125 and 166-206). Consider extracting this into a shared helper to reduce duplication and ensure consistent updates.♻️ Suggested approach
// Extract shared logic const processAuthResponse = ( response: LoginResponse, context: { username?: string; provider?: string }, set: (state: Partial<AuthState>) => void ) => { const idToken = response.authResponse?.id_token; if (!idToken) { logger.error({ message: 'No ID token received', context }); throw new Error('No ID token received'); } const tokenParts = idToken.split('.'); if (tokenParts.length < 3 || !tokenParts[1]) { logger.error({ message: 'Invalid ID token format', context }); throw new Error('Invalid ID token format'); } const payload = sanitizeJson(decodeJwtPayload(tokenParts[1])); const now = Date.now(); const authResponseWithTimestamp = { ...response.authResponse!, obtained_at: now }; setItem<AuthResponse>('authResponse', authResponseWithTimestamp); const profileData = JSON.parse(payload) as ProfileModel; set({ accessToken: response.authResponse?.access_token ?? null, refreshToken: response.authResponse?.refresh_token ?? null, accessTokenObtainedAt: now, refreshTokenObtainedAt: now, status: 'signedIn', error: null, profile: profileData, userId: profileData.sub, isFirstTime: false, }); return { profileData, now }; };Also applies to: 161-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/auth/store.tsx` around lines 81 - 159, The login and loginWithSso handlers duplicate JWT parsing, validation, storage and state updates; extract that logic into a shared helper (e.g., processAuthResponse) that accepts the LoginResponse, a context object ({ username?, provider? }), and the store set function, and performs id_token checks, tokenParts parsing, sanitizeJson(decodeJwtPayload(...)), sets obtained_at on the AuthResponse and persists with setItem<AuthResponse>, parses ProfileModel, and calls set(...) to update accessToken/refreshToken, timestamps, status, profile, userId and isFirstTime; replace the duplicated blocks in login and loginWithSso with calls to this helper and propagate thrown errors so existing logging/try/catch continues working.src/components/calendar/enhanced-calendar-view.tsx (2)
233-236: Stale comment references removed function.The comment on line 234 mentions
isSameDatebut the code now usesisDateInRange. Update the comment to match the implementation.✏️ Suggested fix
const eventsForDay = selectedMonthItems.filter((item) => { - // Use isSameDate for timezone-safe date comparison with .NET backend timezone-aware dates + // Use isDateInRange for timezone-safe date comparison, handling multi-day events return selectedDate ? isDateInRange(selectedDate, item.Start, item.End, item.IsAllDay) : false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/enhanced-calendar-view.tsx` around lines 233 - 236, The inline comment above the filter is stale (mentions isSameDate) — update it to reflect the current implementation that uses isDateInRange for timezone-safe comparisons; locate the filter where eventsForDay is computed (selectedMonthItems.filter) and replace or reword the comment to reference isDateInRange and its purpose (timezone-safe date-range comparison with .NET backend timezone-aware dates).
69-73: Consider validation for date string parsing.The fallback to
'0'for undefined parts would produce year 0 dates if the string format is unexpected. WhilestartDateandendDateshould always be valid YYYY-MM-DD from the utility functions, adding a guard or logging for malformed dates would improve robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/calendar/enhanced-calendar-view.tsx` around lines 69 - 73, The parsing of startDate/endDate into rangeStart/rangeEnd (using startParts/endParts and new Date) should validate the split parts before using fallbacks like '0'; update the code in enhanced-calendar-view (where startDate, endDate, startParts, endParts, rangeStart, rangeEnd, and current are computed) to check that startParts.length === 3 and endParts.length === 3 and that each part parses to a sensible integer (e.g., year > 0, month 1-12, day 1-31); if validation fails, log a warning/error and either throw or use a safe default behavior (e.g., skip rendering or use today) instead of falling back to '0', so malformed YYYY-MM-DD strings are detected and handled safely.src/app/login/sso-section.tsx (2)
6-7: Consolidate duplicate imports from 'react-native'.
ImageandScrollVieware imported from 'react-native' in separate statements. Consolidate them for cleaner code.Proposed fix
-import { Image } from 'react-native'; -import { ScrollView } from 'react-native'; +import { Image, ScrollView } from 'react-native';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/sso-section.tsx` around lines 6 - 7, Consolidate the duplicate imports by combining Image and ScrollView into a single import from 'react-native' (replace the two separate import statements with one import that lists both Image and ScrollView); update the import that currently references Image and the one referencing ScrollView so only a single import statement remains exporting both symbols.
45-66: Exception fromonLookupUserleaves loading state stuck.If
onLookupUserthrows an exception (e.g., network error), thefinallyblock will still run and clearlocalLoading, which is correct. However, the error is silently swallowed. Consider catching and displaying unexpected errors to the user.Proposed fix to handle unexpected errors
const onSubmit = useCallback( async (data: DepartmentFormFields) => { setLookupError(null); setLocalLoading(true); try { const trimmedUsername = data.username.trim(); const deptIdRaw = data.departmentId.trim(); const departmentId = deptIdRaw !== '' ? parseInt(deptIdRaw, 10) : undefined; const config = await onLookupUser(trimmedUsername, departmentId); if (!config) { setLookupError(t('login.sso.user_not_found')); } else if (!config.ssoEnabled) { setLookupError(t('login.sso.sso_not_enabled')); } else { onSsoConfigResolved(trimmedUsername, config); } + } catch (error) { + logger.error({ message: 'SSO lookup failed', context: { error: error instanceof Error ? error.message : String(error) } }); + setLookupError(t('common.errorOccurred')); } finally { setLocalLoading(false); } }, [onLookupUser, onSsoConfigResolved, t] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/sso-section.tsx` around lines 45 - 66, The onSubmit callback currently lets exceptions from onLookupUser be swallowed; add a try/catch around the await onLookupUser(...) call (inside the existing try) to catch unexpected errors, log them (e.g., console.error or processLogger) and call setLookupError with a user-facing message (use t('login.sso.lookup_failed') or error.message sanitized) before allowing the existing finally to clear loading; update the onSubmit function (references: onSubmit, onLookupUser, setLookupError, setLocalLoading, onSsoConfigResolved) to implement this catch so network/throwing errors surface to the UI instead of being silent.src/hooks/use-oidc-login.ts (1)
39-49: Placeholder clientId may cause unintended requests.When
clientIdis empty/undefined,'__placeholder__'is used. Whileauthority ? discovery : nullprevents premature requests when authority is missing, ifauthorityis set butclientIdis not yet available,useAuthRequestwill prepare a request with the placeholder clientId. This could potentially cause confusing errors ifpromptAsyncis called in this state.Consider adding a similar guard for
clientId:Proposed fix
const [request, response, promptAsync] = AuthSession.useAuthRequest( { - clientId: clientId || '__placeholder__', + clientId: clientId || '', redirectUri, scopes: ['openid', 'email', 'profile', 'offline_access'], usePKCE: true, responseType: AuthSession.ResponseType.Code, }, // Pass null discovery when authority is not yet set to prevent premature requests - authority ? discovery : null + authority && clientId ? discovery : null );🤖 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 39 - 49, The current useAuthRequest call can build a request with a placeholder clientId ('__placeholder__') which may produce confusing errors; update the component so useAuthRequest is only given the discovery/config when both authority and a real clientId exist—i.e., guard the call by passing null for the discovery/config (or avoid calling useAuthRequest) when clientId is falsy. Modify the invocation around AuthSession.useAuthRequest (and any use of promptAsync) to check clientId alongside authority so no auth request is prepared until a real clientId is available.src/hooks/use-saml-login.ts (2)
70-71:removeItemis async but not awaited.The
removeItemfunction is async (returnsPromise<void>), but it's called withoutawait. This could cause the function to returntruebefore the key is actually deleted, potentially leading to race conditions if another handler attempts to process the same deep link.Proposed fix
await loginWithSso({ provider: 'saml2', externalToken: samlResponse, departmentCode, }); - removeItem(PENDING_SAML_DEPT_CODE_KEY); + await removeItem(PENDING_SAML_DEPT_CODE_KEY); return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-saml-login.ts` around lines 70 - 71, The call to removeItem(PENDING_SAML_DEPT_CODE_KEY) is asynchronous but not awaited, which can let the function return true before the key is deleted; update the surrounding function in use-saml-login.ts to await removeItem (or return the Promise chain) so deletion completes before returning, ensuring the function signature (e.g., the handler or function containing this code) is marked async and its return type adjusted if needed; reference removeItem and PENDING_SAML_DEPT_CODE_KEY to locate and modify the call.
91-120: Consider adding deduplication to prevent duplicate token exchanges.Per the context snippets,
_layout.tsxregisters two handlers (cold-start viagetInitialURLand warm-start viaaddEventListener), andloginWithSsohas no idempotency check. If both handlers fire for the same URL, duplicate API calls toexternalTokenRequestcould occur.Consider adding a simple guard (e.g., a module-level flag or checking/clearing the stored key atomically) to prevent processing the same SAML response twice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-saml-login.ts` around lines 91 - 120, handleSamlCallbackUrl can be invoked twice (cold/warm start) and may trigger duplicate token exchanges; add a deduplication guard by introducing a module-level processing flag (e.g., isProcessingSaml) or make the pending key read-and-remove atomic: when entry to handleSamlCallbackUrl begins, check and set the flag (or read-and-remove PENDING_SAML_DEPT_CODE_KEY immediately) so only the first caller proceeds to call useAuthStore.getState().loginWithSso, and ensure you clear/reset the flag or restore state on failure inside the catch block before returning false; reference handleSamlCallbackUrl, PENDING_SAML_DEPT_CODE_KEY, getItem/removeItem and useAuthStore.getState().loginWithSso when applying the change.
🤖 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/app/login/sso.tsx`:
- Around line 116-118: The code passes ssoConfig with a non-null assertion to
SsoLoginButtons (SsoLoginButtons departmentCode={username}
ssoConfig={ssoConfig!}), which can crash if ssoConfig is null; change this to
guard against null by either conditionally rendering SsoLoginButtons only when
ssoConfig is defined (e.g., if (ssoConfig) render SsoLoginButtons) or pass
ssoConfig without the bang and update SsoLoginButtons to accept an optional
ssoConfig prop and handle undefined internally so runtime errors are avoided.
- Around line 113-125: When ssoPhase === 'login' and ssoEnabled === false the
component returns null leaving the user with a blank screen; update the
conditional render to show a fallback UI instead (e.g. a small message and a
"Change department" or "Use password login" action) when ssoEnabled is false
during the 'login' phase. Modify the JSX branch that currently returns null to
render a new fallback component or inline markup (referenced as e.g.
NonSsoFallback or a fragment) that calls the existing handleChangeDepartment
handler and/or navigates to the standard login flow; ensure SsoLoginButtons and
SsoDepartmentForm logic is unchanged and that oidc/saml calls remain in their
branch.
In `@src/hooks/__tests__/use-saml-login.test.ts`:
- Around line 20-22: Replace the loose any[] signatures used when wiring
mocks—specifically the mockSetItem, mockGetItem, and mockRemoveItem wrappers in
use-saml-login.test.ts—and replace their parameter typing with the correct
explicit types that match the storage and auth store interfaces used by the hook
(e.g., Storage-like key/value types and the AuthStore get/set/remove method
signatures); update all occurrences where (...args: any[]) is used (including
the three wrapper lines and the other mock boundaries referenced) so the mocks
accept and return the precise types from your Storage/AuthStore interfaces and
preserve strong type checking in tests.
In `@src/hooks/use-saml-login.ts`:
- Around line 105-119: In handleSamlCallbackUrl: await the async cleanup call
(await removeItem(PENDING_SAML_DEPT_CODE_KEY)) where you currently call
removeItem without awaiting, and ensure you also clear that pending key in the
catch block so a failed login doesn't leave the key for the other handler;
specifically wrap the useAuthStore.getState().loginWithSso(...) call in
try/catch, await removeItem(PENDING_SAML_DEPT_CODE_KEY) after a successful
login, and call/await removeItem(PENDING_SAML_DEPT_CODE_KEY) inside the catch
before returning false so both cold-start and warm-start handlers registered in
_layout.tsx can't reuse the stale key.
- Around line 72-78: In the catch block inside the useSamlLogin hook where the
token exchange failure is logged (currently logging 'SAML: token exchange
failed'), also clear the stale pending department code by removing
PENDING_SAML_DEPT_CODE_KEY from storage before returning false; use the same
storage API used elsewhere in this module (e.g., localStorage.removeItem or
storage.remove) and reference the PENDING_SAML_DEPT_CODE_KEY constant and the
useSamlLogin function so the pending SAML dept code cannot be reused after an
error.
In `@src/models/v4/calendar/calendarItemResultData.ts`:
- Line 16: The IsMultiDay property defaulting to false masks missing backend
data; make IsMultiDay nullable/optional (remove the hard false default) and
ensure the mapping layer (e.g., the function that constructs
CalendarItemResultData / mapCalendarItemResultData or
CalendarItemResultData.fromApi) computes and sets IsMultiDay when the API omits
it by comparing Start and End (treat as multi-day when End date > Start date or
spans calendar days, accounting for timezone/ISO strings); leave any explicit
API-provided IsMultiDay value intact if present.
In `@src/services/sso-discovery.ts`:
- Around line 19-29: The catch block in fetchDepartmentSsoConfig currently
swallows all errors and returns null, making network/server failures
indistinguishable from a legitimate "no config" result; change this to preserve
an error signal by either removing the blanket catch so the exception bubbles,
or rethrowing a wrapped Error with context (e.g., include departmentCode and
original error) from fetchDepartmentSsoConfig, and update its callers to handle
network errors separately from a null/empty DepartmentSsoConfig result; apply
the same pattern to the similar function later in the file (lines ~37-48) so all
SSO lookup failures surface as errors instead of nulls.
In `@src/utils/InCallAudio.ts`:
- Line 55: The code uses an unsafe cast (config as any). Replace that by
preserving the typed shape from SOUNDS: when mapping entries, assert the entry
name as SoundName and access config.android directly, e.g. map(([name, config])
=> InCallAudioModule.loadSound(name as SoundName, config.android)); this removes
the any cast while keeping compile-time types for SOUNDS, SoundName and
InCallAudioModule.loadSound and assigns the result to preloadPromises as before.
- Around line 104-108: The callback passed to Audio.Sound.createAsync references
the local variable sound before it’s guaranteed to be assigned, which can cause
undefined access; refactor to the two-step pattern: await
Audio.Sound.createAsync(...) to get the sound instance first (from the returned
object), then call sound.setOnPlaybackStatusUpdate or attach the status callback
separately, and inside that callback call sound.unloadAsync() when
status.isLoaded && status.didJustFinish; update usages around
Audio.Sound.createAsync, the sound local variable, and unloadAsync accordingly.
---
Nitpick comments:
In `@app.config.ts`:
- Line 29: Create a single exported scheme constant (e.g., APP_SCHEME or SCHEME)
and use it everywhere instead of the mixed hardcoded 'resgrid' and Env.SCHEME;
update the scheme array in the app.config.ts "scheme" property, replace the
hardcoded 'resgrid' occurrences around lines 80-86, and use the same exported
constant in use-oidc-login (where the deep-link callback is constructed) and the
Android intent filter code so all places reference the one shared symbol.
In `@src/app/login/__tests__/login-form.test.tsx`:
- Around line 203-211: The teardown block currently calls jest.clearAllMocks()
then mockTrackEvent.mockReset() five times and mockTrackEvent.mockClear();
simplify by keeping a single call to mockTrackEvent.mockReset() (mockReset
already clears history) and removing the repeated mockReset/mockClear calls, and
keep the single mockServerUrlBottomSheet.mockClear() as-is; update the block
referencing mockTrackEvent and mockServerUrlBottomSheet to reflect this change.
- Around line 213-249: Add a new test that explicitly verifies the SSO button
behavior: render LoginForm with mockOnSubmit, locate the SSO button via
screen.getAllByTestId('button')[2] (the third button), fireEvent.press on it,
then assert mockTrackEvent was called with an SSO-specific event (e.g.,
'login_sso_pressed' and timestamp) and assert the SSO navigation handler was
invoked by creating/using a jest mock like mockNavigateToSSO (or the existing
navigation prop/handler used by LoginForm) and expecting it toHaveBeenCalled();
this ensures LoginForm, mockOnSubmit, mockTrackEvent and the SSO navigation mock
are exercised.
In `@src/app/login/sso-section.tsx`:
- Around line 6-7: Consolidate the duplicate imports by combining Image and
ScrollView into a single import from 'react-native' (replace the two separate
import statements with one import that lists both Image and ScrollView); update
the import that currently references Image and the one referencing ScrollView so
only a single import statement remains exporting both symbols.
- Around line 45-66: The onSubmit callback currently lets exceptions from
onLookupUser be swallowed; add a try/catch around the await onLookupUser(...)
call (inside the existing try) to catch unexpected errors, log them (e.g.,
console.error or processLogger) and call setLookupError with a user-facing
message (use t('login.sso.lookup_failed') or error.message sanitized) before
allowing the existing finally to clear loading; update the onSubmit function
(references: onSubmit, onLookupUser, setLookupError, setLocalLoading,
onSsoConfigResolved) to implement this catch so network/throwing errors surface
to the UI instead of being silent.
In `@src/app/login/sso.tsx`:
- Around line 38-42: The idpSsoUrl fallback is unclear; update the call to
useSamlLogin (the idpSsoUrl argument) to include a short clarifying comment and
explicit intent that metadataUrl is preferred and authority is a fallback, e.g.,
annotate why ssoConfig?.authority is acceptable if metadataUrl is missing;
reference the useSamlLogin call and the idpSsoUrl, ssoConfig?.metadataUrl and
ssoConfig?.authority symbols so reviewers can see the rationale next to the code
and avoid changing the fallback behavior unintentionally.
- Around line 65-76: The effect is using oidc.exchangeCodeForResgridToken
without listing it in dependencies which risks a stale closure; destructure the
needed members from oidc (e.g. const { response, exchangeCodeForResgridToken } =
oidc) and update the effect to reference response and
exchangeCodeForResgridToken, include both in the dependency array, remove the
eslint-disable comment, and keep the existing error handling (logger.error and
setIsErrorModalVisible) inside the effect so the call uses the current function
reference.
In
`@src/components/calendar/__tests__/calendar-item-details-sheet-analytics.test.tsx`:
- Around line 209-214: The test currently calls jest.clearAllMocks() then
repeats mockTrackEvent.mockReset() five times; remove the redundant mockReset()
calls (delete all extra mockTrackEvent.mockReset() lines) since
jest.clearAllMocks() already resets mocks—if you need to ensure only
mockTrackEvent is reset instead of all mocks, replace the repeated calls with a
single mockTrackEvent.mockReset() and keep jest.clearAllMocks() usage consistent
in the test setup where mockTrackEvent is referenced.
In
`@src/components/calendar/__tests__/calendar-item-details-sheet-minimal.test.tsx`:
- Around line 134-144: The beforeEach block calls mockTrackEvent.mockReset()
five times redundantly; replace the repeated calls with a single
mockTrackEvent.mockReset() call inside the beforeEach (which also already calls
jest.clearAllMocks()), keeping the existing (useAnalytics as
jest.Mock).mockReturnValue({ trackEvent: mockTrackEvent }) line and leaving
mockTrackEvent and useAnalytics identifiers unchanged.
In `@src/components/calendar/__tests__/calendar-item-details-sheet.test.tsx`:
- Around line 243-250: In beforeEach(), remove the four redundant consecutive
calls to mockTrackEvent.mockReset() so there is only a single
mockTrackEvent.mockReset() call (keep the existing jest.clearAllMocks() as-is);
this simplifies the setup in the test file where beforeEach and
mockTrackEvent.mockReset() are used.
In `@src/components/calendar/calendar-card.tsx`:
- Around line 98-125: Extract the inline IIFE used to render the multi-day end
date into a descriptive helper (e.g., formatMultiDayEndDate or
getMultiDayEndDisplay) that accepts the event end and IsAllDay flag and returns
the formatted string; replace the IIFE in the JSX with a call to that helper,
reusing resolveAllDayEndDate and the same date-parsing/formatting logic (and
keep formatDate for non-all-day end dates) so the JSX around item.IsMultiDay,
formatDate, resolveAllDayEndDate, and getEventDuration remains unchanged but
much clearer.
In `@src/components/calendar/compact-calendar-item.tsx`:
- Around line 78-94: The inline IIFE in the JSX that computes the displayed end
date reduces readability; extract that logic into a named helper (e.g.,
getDisplayEndDate or computeEndDateDisplay) that accepts (end: string, isAllDay:
boolean) and internally uses resolveAllDayEndDate and the same
parsing/Date->toLocaleDateString flow (or calls formatDate for non-all-day),
then replace the IIFE in compact-calendar-item's render with a call to this
helper (or wrap it in useMemo if you want to avoid recomputation based on
item.End and item.IsAllDay).
In `@src/components/calendar/enhanced-calendar-view.tsx`:
- Around line 233-236: The inline comment above the filter is stale (mentions
isSameDate) — update it to reflect the current implementation that uses
isDateInRange for timezone-safe comparisons; locate the filter where
eventsForDay is computed (selectedMonthItems.filter) and replace or reword the
comment to reference isDateInRange and its purpose (timezone-safe date-range
comparison with .NET backend timezone-aware dates).
- Around line 69-73: The parsing of startDate/endDate into rangeStart/rangeEnd
(using startParts/endParts and new Date) should validate the split parts before
using fallbacks like '0'; update the code in enhanced-calendar-view (where
startDate, endDate, startParts, endParts, rangeStart, rangeEnd, and current are
computed) to check that startParts.length === 3 and endParts.length === 3 and
that each part parses to a sensible integer (e.g., year > 0, month 1-12, day
1-31); if validation fails, log a warning/error and either throw or use a safe
default behavior (e.g., skip rendering or use today) instead of falling back to
'0', so malformed YYYY-MM-DD strings are detected and handled safely.
In `@src/hooks/__tests__/use-saml-login.test.ts`:
- Around line 106-158: Remove the duplicated test suite for
describe('handleSamlCallbackUrl') so only one suite remains; locate the repeated
block that contains the same beforeEach and four it(...) tests (cases for
missing saml_response, no pending dept code, successful login clears storage,
and loginWithSso throwing) and delete the second copy, ensuring the remaining
describe retains the mockedParse/mockGetItem/mockLoginWithSso usages and
assertions including mockRemoveItem and PENDING_SAML_DEPT_CODE_KEY checks.
In `@src/hooks/use-oidc-login.ts`:
- Around line 39-49: The current useAuthRequest call can build a request with a
placeholder clientId ('__placeholder__') which may produce confusing errors;
update the component so useAuthRequest is only given the discovery/config when
both authority and a real clientId exist—i.e., guard the call by passing null
for the discovery/config (or avoid calling useAuthRequest) when clientId is
falsy. Modify the invocation around AuthSession.useAuthRequest (and any use of
promptAsync) to check clientId alongside authority so no auth request is
prepared until a real clientId is available.
In `@src/hooks/use-saml-login.ts`:
- Around line 70-71: The call to removeItem(PENDING_SAML_DEPT_CODE_KEY) is
asynchronous but not awaited, which can let the function return true before the
key is deleted; update the surrounding function in use-saml-login.ts to await
removeItem (or return the Promise chain) so deletion completes before returning,
ensuring the function signature (e.g., the handler or function containing this
code) is marked async and its return type adjusted if needed; reference
removeItem and PENDING_SAML_DEPT_CODE_KEY to locate and modify the call.
- Around line 91-120: handleSamlCallbackUrl can be invoked twice (cold/warm
start) and may trigger duplicate token exchanges; add a deduplication guard by
introducing a module-level processing flag (e.g., isProcessingSaml) or make the
pending key read-and-remove atomic: when entry to handleSamlCallbackUrl begins,
check and set the flag (or read-and-remove PENDING_SAML_DEPT_CODE_KEY
immediately) so only the first caller proceeds to call
useAuthStore.getState().loginWithSso, and ensure you clear/reset the flag or
restore state on failure inside the catch block before returning false;
reference handleSamlCallbackUrl, PENDING_SAML_DEPT_CODE_KEY, getItem/removeItem
and useAuthStore.getState().loginWithSso when applying the change.
In `@src/stores/auth/store.tsx`:
- Around line 81-159: The login and loginWithSso handlers duplicate JWT parsing,
validation, storage and state updates; extract that logic into a shared helper
(e.g., processAuthResponse) that accepts the LoginResponse, a context object ({
username?, provider? }), and the store set function, and performs id_token
checks, tokenParts parsing, sanitizeJson(decodeJwtPayload(...)), sets
obtained_at on the AuthResponse and persists with setItem<AuthResponse>, parses
ProfileModel, and calls set(...) to update accessToken/refreshToken, timestamps,
status, profile, userId and isFirstTime; replace the duplicated blocks in login
and loginWithSso with calls to this helper and propagate thrown errors so
existing logging/try/catch continues working.
In `@src/stores/calendar/__tests__/store.test.ts`:
- Around line 139-144: The mock implementation for mockedUtils.isDateInRange
uses a parameter named _eventEnd even though it’s used; rename the parameter to
eventEnd in the mockImplementation signature and update its internal usage (the
slice/endStr reference) to match, e.g., change (_eventEnd: string) to (eventEnd:
string) so the parameter name accurately reflects usage in
mockedUtils.isDateInRange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b37cc8c-c7fc-417a-a6df-2df9d96742f6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
app.config.tspackage.jsonsrc/app/(app)/calendar.tsxsrc/app/_layout.tsxsrc/app/login/__tests__/login-form.test.tsxsrc/app/login/index.tsxsrc/app/login/login-form.tsxsrc/app/login/sso-section.tsxsrc/app/login/sso.tsxsrc/components/calendar/__tests__/calendar-card.test.tsxsrc/components/calendar/__tests__/calendar-item-details-sheet-analytics.test.tsxsrc/components/calendar/__tests__/calendar-item-details-sheet-minimal.test.tsxsrc/components/calendar/__tests__/calendar-item-details-sheet.security.test.tsxsrc/components/calendar/__tests__/calendar-item-details-sheet.test.tsxsrc/components/calendar/__tests__/compact-calendar-item.test.tsxsrc/components/calendar/__tests__/component-comparison.test.tsxsrc/components/calendar/calendar-card.tsxsrc/components/calendar/compact-calendar-item.tsxsrc/components/calendar/enhanced-calendar-view.tsxsrc/hooks/__tests__/use-oidc-login.test.tssrc/hooks/__tests__/use-saml-login.test.tssrc/hooks/use-oidc-login.tssrc/hooks/use-saml-login.tssrc/lib/auth/api.tsxsrc/lib/auth/index.tsxsrc/lib/auth/types.tsxsrc/lib/utils.tssrc/models/v4/calendar/calendarItemResultData.tssrc/services/__tests__/sso-discovery.test.tssrc/services/callkeep.service.android.tssrc/services/sso-discovery.tssrc/stores/auth/store.tsxsrc/stores/calendar/__tests__/store.test.tssrc/stores/calendar/store.tssrc/translations/ar.jsonsrc/translations/en.jsonsrc/translations/es.jsonsrc/utils/InCallAudio.ts
Comment on lines
+20
to
+22
| setItem: (...args: any[]) => mockSetItem(...args), | ||
| getItem: (...args: any[]) => mockGetItem(...args), | ||
| removeItem: (...args: any[]) => mockRemoveItem(...args), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts '\bany\b' src/hooks/__tests__/use-saml-login.test.ts -C2Repository: Resgrid/Responder
Length of output: 766
Replace any type casts with explicit types in mock setup.
Lines 20–22, 39, and 54 use any in mock boundaries, weakening type checks. Replace with proper types matching the storage and auth store interfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/__tests__/use-saml-login.test.ts` around lines 20 - 22, Replace the
loose any[] signatures used when wiring mocks—specifically the mockSetItem,
mockGetItem, and mockRemoveItem wrappers in use-saml-login.test.ts—and replace
their parameter typing with the correct explicit types that match the storage
and auth store interfaces used by the hook (e.g., Storage-like key/value types
and the AuthStore get/set/remove method signatures); update all occurrences
where (...args: any[]) is used (including the three wrapper lines and the other
mock boundaries referenced) so the mocks accept and return the precise types
from your Storage/AuthStore interfaces and preserve strong type checking in
tests.
Comment on lines
+105
to
+119
| try { | ||
| await useAuthStore.getState().loginWithSso({ | ||
| provider: 'saml2', | ||
| externalToken: samlResponse, | ||
| departmentCode, | ||
| }); | ||
| removeItem(PENDING_SAML_DEPT_CODE_KEY); | ||
| return true; | ||
| } catch (error) { | ||
| logger.error({ | ||
| message: 'SAML cold-start: token exchange failed', | ||
| context: { error: error instanceof Error ? error.message : String(error) }, | ||
| }); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Same issues in standalone handler: missing await and no cleanup on error.
The standalone handleSamlCallbackUrl function has the same two issues as the hook's handleDeepLink:
- Line 111:
removeItemis not awaited - Lines 113-118: The pending department code is not cleared when token exchange fails
Additionally, per context snippet 2, _layout.tsx registers both a cold-start and warm-start handler that call this function. Without deduplication or clearing the key on error, a failed attempt leaves the key for the next handler to potentially reuse incorrectly.
Proposed fix
try {
await useAuthStore.getState().loginWithSso({
provider: 'saml2',
externalToken: samlResponse,
departmentCode,
});
- removeItem(PENDING_SAML_DEPT_CODE_KEY);
+ await removeItem(PENDING_SAML_DEPT_CODE_KEY);
return true;
} catch (error) {
logger.error({
message: 'SAML cold-start: token exchange failed',
context: { error: error instanceof Error ? error.message : String(error) },
});
+ await removeItem(PENDING_SAML_DEPT_CODE_KEY);
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/use-saml-login.ts` around lines 105 - 119, In
handleSamlCallbackUrl: await the async cleanup call (await
removeItem(PENDING_SAML_DEPT_CODE_KEY)) where you currently call removeItem
without awaiting, and ensure you also clear that pending key in the catch block
so a failed login doesn't leave the key for the other handler; specifically wrap
the useAuthStore.getState().loginWithSso(...) call in try/catch, await
removeItem(PENDING_SAML_DEPT_CODE_KEY) after a successful login, and call/await
removeItem(PENDING_SAML_DEPT_CODE_KEY) inside the catch before returning false
so both cold-start and warm-start handlers registered in _layout.tsx can't reuse
the stale key.
| const preloadPromises = Object.entries(SOUNDS).map(([name, config]) => | ||
| InCallAudioModule.loadSound(name, (config as any).android) | ||
| ); | ||
| const preloadPromises = Object.entries(SOUNDS).map(([name, config]) => InCallAudioModule.loadSound(name, (config as any).android)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no `any` casts remain in this utility
rg -nP '\bas\s+any\b|:\s*any\b' src/utils/InCallAudio.tsRepository: Resgrid/Responder
Length of output: 207
🏁 Script executed:
cat -n src/utils/InCallAudio.ts | head -80Repository: Resgrid/Responder
Length of output: 3234
🏁 Script executed:
rg -B 20 -A 5 'const preloadPromises' src/utils/InCallAudio.tsRepository: Resgrid/Responder
Length of output: 916
🏁 Script executed:
rg -B 5 'type SoundName|interface.*Sound' src/utils/InCallAudio.tsRepository: Resgrid/Responder
Length of output: 227
Remove any cast; use type-safe property access.
Line 55 uses (config as any).android, which weakens type safety. Since SOUNDS is defined with as const and SoundName = keyof typeof SOUNDS, you can access android properties directly without the cast.
♻️ Suggested refactor
- const preloadPromises = Object.entries(SOUNDS).map(([name, config]) => InCallAudioModule.loadSound(name, (config as any).android));
+ const preloadPromises = (Object.keys(SOUNDS) as SoundName[]).map((name) =>
+ InCallAudioModule.loadSound(name, SOUNDS[name].android),
+ );This aligns with the guideline: "Avoid using any; strive for precise types."
📝 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
| const preloadPromises = Object.entries(SOUNDS).map(([name, config]) => InCallAudioModule.loadSound(name, (config as any).android)); | |
| const preloadPromises = (Object.keys(SOUNDS) as SoundName[]).map((name) => | |
| InCallAudioModule.loadSound(name, SOUNDS[name].android), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/InCallAudio.ts` at line 55, The code uses an unsafe cast (config as
any). Replace that by preserving the typed shape from SOUNDS: when mapping
entries, assert the entry name as SoundName and access config.android directly,
e.g. map(([name, config]) => InCallAudioModule.loadSound(name as SoundName,
config.android)); this removes the any cast while keeping compile-time types for
SOUNDS, SoundName and InCallAudioModule.loadSound and assigns the result to
preloadPromises as before.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/hooks/use-saml-login.ts (1)
114-119:⚠️ Potential issue | 🟡 MinorCold-start catch path still leaves stale pending key.
In Line 114-Line 119, the error path logs and returns
falsewithout clearingPENDING_SAML_DEPT_CODE_KEY, so stale department state can persist.Proposed fix
} catch (error) { + await removeItem(PENDING_SAML_DEPT_CODE_KEY); logger.error({ message: 'SAML cold-start: token exchange failed', context: { error: error instanceof Error ? error.message : String(error) }, }); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-saml-login.ts` around lines 114 - 119, The catch block for the SAML cold-start error currently logs the failure and returns false but leaves the PENDING_SAML_DEPT_CODE_KEY value set, allowing stale department state to persist; update the catch to clear that pending key (use the same storage API used elsewhere in this module, e.g. localStorage.removeItem(PENDING_SAML_DEPT_CODE_KEY) or the module's storage helper) before returning false, and ensure any other early-return error paths in the same function also remove PENDING_SAML_DEPT_CODE_KEY so no stale state remains.
🧹 Nitpick comments (1)
src/app/login/sso-section.tsx (1)
1-17: Imports look good.Minor style suggestion: Lines 6-7 could be consolidated into a single import from
react-native:import { Image, ScrollView } from 'react-native';Using the default
Imagecomponent is acceptable here since it's for static local branding assets wherereact-native-fast-imagecaching benefits aren't needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/sso-section.tsx` around lines 1 - 17, Consolidate the two separate react-native imports into one: replace the separate "import { Image } from 'react-native';" and "import { ScrollView } from 'react-native';" with a single import statement "import { Image, ScrollView } from 'react-native';" — keep using the default Image component for local static branding assets (no change to usage of Image or ScrollView elsewhere in this file, e.g., within the SSO section components).
🤖 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/app/login/sso-section.tsx`:
- Around line 161-165: The spinner color is hardcoded to
colors.light.neutral[400] which ignores the current theme; update the
ButtonSpinner usage in the SSOSection component to use the colorScheme value
from useColorScheme() instead of the literal colors.light reference (e.g.,
derive the color via colors[colorScheme].neutral[400] or a small helper like
getNeutralColor(colorScheme)), and replace the existing
colors.light.neutral[400] in the ButtonSpinner props so the spinner matches dark
mode as well.
In `@src/hooks/use-saml-login.ts`:
- Around line 45-50: The code writes PENDING_SAML_DEPT_CODE_KEY via setItem then
calls WebBrowser.openBrowserAsync without error cleanup; wrap the
WebBrowser.openBrowserAsync call in a try/catch so that on any failure you
remove the pending key (call removeItem<string>(PENDING_SAML_DEPT_CODE_KEY) or
equivalent) and log the error with logger.error, and optionally surface user
feedback; update the async callback that references idpSsoUrl and departmentCode
to perform this cleanup in the catch block.
In `@src/models/v4/calendar/calendarItemResultData.ts`:
- Around line 49-53: computeIsMultiDay currently compares start and end dates
directly which treats exclusive all-day end dates (end set to day after) as
multi-day; update computeIsMultiDay to accept an isAllDay flag (or access
raw.IsAllDay) and when IsAllDay is true treat the end date as inclusive by
subtracting one day from the end date before comparing; specifically change
computeIsMultiDay(start: string, end: string) to computeIsMultiDay(start:
string, end: string, isAllDay?: boolean) (or read raw.IsAllDay where used),
resolve endDate = end.slice(0,10) and if isAllDay adjust endDate =
(dateMinusOneDay(endDate)).slice(0,10) then return endDate > startDate; apply
the same inclusive-end logic to the other occurrence using the same helper (the
other block at lines handling 62-66).
---
Duplicate comments:
In `@src/hooks/use-saml-login.ts`:
- Around line 114-119: The catch block for the SAML cold-start error currently
logs the failure and returns false but leaves the PENDING_SAML_DEPT_CODE_KEY
value set, allowing stale department state to persist; update the catch to clear
that pending key (use the same storage API used elsewhere in this module, e.g.
localStorage.removeItem(PENDING_SAML_DEPT_CODE_KEY) or the module's storage
helper) before returning false, and ensure any other early-return error paths in
the same function also remove PENDING_SAML_DEPT_CODE_KEY so no stale state
remains.
---
Nitpick comments:
In `@src/app/login/sso-section.tsx`:
- Around line 1-17: Consolidate the two separate react-native imports into one:
replace the separate "import { Image } from 'react-native';" and "import {
ScrollView } from 'react-native';" with a single import statement "import {
Image, ScrollView } from 'react-native';" — keep using the default Image
component for local static branding assets (no change to usage of Image or
ScrollView elsewhere in this file, e.g., within the SSO section components).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2df5d941-7935-4026-9533-f56404e5dd2f
📒 Files selected for processing (11)
src/app/login/sso-section.tsxsrc/app/login/sso.tsxsrc/hooks/use-saml-login.tssrc/models/v4/calendar/calendarItemResultData.tssrc/services/__tests__/sso-discovery.test.tssrc/services/sso-discovery.tssrc/stores/calendar/store.tssrc/translations/ar.jsonsrc/translations/en.jsonsrc/translations/es.jsonsrc/utils/InCallAudio.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/app/login/sso.tsx
- src/services/tests/sso-discovery.test.ts
- src/services/sso-discovery.ts
- src/translations/ar.json
- src/translations/es.json
Comment on lines
+45
to
+50
| // Persist department code so the cold-start deep-link handler can retrieve it | ||
| await setItem<string>(PENDING_SAML_DEPT_CODE_KEY, departmentCode); | ||
|
|
||
| logger.info({ message: 'SAML: opening IdP SSO URL', context: { idpSsoUrl } }); | ||
| await WebBrowser.openBrowserAsync(idpSsoUrl); | ||
| }, [idpSsoUrl, departmentCode]); |
There was a problem hiding this comment.
Clear pending department key when browser launch fails.
If WebBrowser.openBrowserAsync fails after Line 46 writes the key, stale state remains and may be reused later. Add cleanup in a catch path.
Proposed fix
// Persist department code so the cold-start deep-link handler can retrieve it
await setItem<string>(PENDING_SAML_DEPT_CODE_KEY, departmentCode);
logger.info({ message: 'SAML: opening IdP SSO URL', context: { idpSsoUrl } });
- await WebBrowser.openBrowserAsync(idpSsoUrl);
+ try {
+ await WebBrowser.openBrowserAsync(idpSsoUrl);
+ } catch (error) {
+ await removeItem(PENDING_SAML_DEPT_CODE_KEY);
+ logger.error({
+ message: 'SAML: failed to open IdP SSO URL',
+ context: { error: error instanceof Error ? error.message : String(error) },
+ });
+ throw error;
+ }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/hooks/use-saml-login.ts` around lines 45 - 50, The code writes
PENDING_SAML_DEPT_CODE_KEY via setItem then calls WebBrowser.openBrowserAsync
without error cleanup; wrap the WebBrowser.openBrowserAsync call in a try/catch
so that on any failure you remove the pending key (call
removeItem<string>(PENDING_SAML_DEPT_CODE_KEY) or equivalent) and log the error
with logger.error, and optionally surface user feedback; update the async
callback that references idpSsoUrl and departmentCode to perform this cleanup in
the catch block.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/login/sso-section.tsx`:
- Around line 91-92: Validation allows whitespace-only usernames; update the
rules prop to validate trimmed input (e.g., rules={{ validate: v => v?.trim() ?
true : t('login.sso.username_required') }}) and in the render block that
destructures field ({ onChange, onBlur, value }) ensure any lookup or submission
logic uses value.trim() and aborts/returns early if trimmed value is empty so
lookup is never performed for whitespace-only input.
- Around line 45-67: onSubmit can be invoked multiple times causing overlapping
onLookupUser calls and racey state updates; add a lookup-in-flight guard (e.g.,
lookupInFlightRef) checked at the start of onSubmit to return early if a lookup
is already running, set the ref true when starting and false in finally, and
keep using setLocalLoading for UI while ensuring you only clear loading/error
when the current request completes (use the ref to identify the active run).
Update references in the component to use lookupInFlightRef alongside the
existing setLocalLoading, onLookupUser, and onSsoConfigResolved logic to prevent
concurrent submissions.
In `@src/models/v4/calendar/calendarItemResultData.ts`:
- Around line 47-50: The dateMinusOneDay function uses local-time parsing and
UTC serialization which causes timezone drift; update dateMinusOneDay to perform
UTC-safe arithmetic by parsing the YYYY-MM-DD string into year/month/day
integers, creating a Date via Date.UTC or new Date(Date.UTC(...)), subtracting
one day with setUTCDate(getUTCDate() - 1), and returning the date portion from
toISOString().slice(0,10); ensure you only change the implementation of
dateMinusOneDay so callers (e.g., IsMultiDay logic) get a correct UTC-based
previous date.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da2afc02-3168-4696-88a9-109c1111ce1e
📒 Files selected for processing (2)
src/app/login/sso-section.tsxsrc/models/v4/calendar/calendarItemResultData.ts
Comment on lines
+45
to
+67
| const onSubmit = useCallback( | ||
| async (data: DepartmentFormFields) => { | ||
| setLookupError(null); | ||
| setLocalLoading(true); | ||
| try { | ||
| const trimmedUsername = data.username.trim(); | ||
| const deptIdRaw = data.departmentId.trim(); | ||
| const departmentId = deptIdRaw !== '' ? parseInt(deptIdRaw, 10) : undefined; | ||
| const config = await onLookupUser(trimmedUsername, departmentId); | ||
| if (!config) { | ||
| setLookupError(t('login.sso.user_not_found')); | ||
| } else if (!config.ssoEnabled) { | ||
| setLookupError(t('login.sso.sso_not_enabled')); | ||
| } else { | ||
| onSsoConfigResolved(trimmedUsername, config); | ||
| } | ||
| } catch { | ||
| setLookupError(t('login.sso.lookup_network_error')); | ||
| } finally { | ||
| setLocalLoading(false); | ||
| } | ||
| }, | ||
| [onLookupUser, onSsoConfigResolved, t] |
There was a problem hiding this comment.
Prevent concurrent lookup submissions to avoid request/state races.
onSubmit can be triggered multiple times before the first request settles, which can cause overlapping onLookupUser calls and stale UI state (loading/error flips from older responses).
🔧 Suggested fix
-import React, { useCallback, useState } from 'react';
+import React, { useCallback, useRef, useState } from 'react';
const [lookupError, setLookupError] = useState<string | null>(null);
const [localLoading, setLocalLoading] = useState(false);
+ const lookupInFlightRef = useRef(false);
const onSubmit = useCallback(
async (data: DepartmentFormFields) => {
+ if (lookupInFlightRef.current || isLoading) return;
+ lookupInFlightRef.current = true;
setLookupError(null);
setLocalLoading(true);
try {
const trimmedUsername = data.username.trim();
const deptIdRaw = data.departmentId.trim();
const departmentId = deptIdRaw !== '' ? parseInt(deptIdRaw, 10) : undefined;
const config = await onLookupUser(trimmedUsername, departmentId);
if (!config) {
setLookupError(t('login.sso.user_not_found'));
} else if (!config.ssoEnabled) {
setLookupError(t('login.sso.sso_not_enabled'));
} else {
onSsoConfigResolved(trimmedUsername, config);
}
} catch {
setLookupError(t('login.sso.lookup_network_error'));
} finally {
+ lookupInFlightRef.current = false;
setLocalLoading(false);
}
},
- [onLookupUser, onSsoConfigResolved, t]
+ [isLoading, onLookupUser, onSsoConfigResolved, t]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/login/sso-section.tsx` around lines 45 - 67, onSubmit can be invoked
multiple times causing overlapping onLookupUser calls and racey state updates;
add a lookup-in-flight guard (e.g., lookupInFlightRef) checked at the start of
onSubmit to return early if a lookup is already running, set the ref true when
starting and false in finally, and keep using setLocalLoading for UI while
ensuring you only clear loading/error when the current request completes (use
the ref to identify the active run). Update references in the component to use
lookupInFlightRef alongside the existing setLocalLoading, onLookupUser, and
onSsoConfigResolved logic to prevent concurrent submissions.
Comment on lines
+91
to
+92
| rules={{ required: t('login.sso.username_required') }} | ||
| render={({ field: { onChange, onBlur, value } }) => ( |
There was a problem hiding this comment.
Reject whitespace-only usernames before lookup.
Current validation allows " "; after trimming, this becomes empty and still hits lookup, leading to misleading “user not found” instead of field validation feedback.
🔧 Suggested fix
- rules={{ required: t('login.sso.username_required') }}
+ rules={{
+ required: t('login.sso.username_required'),
+ validate: (value) =>
+ value.trim().length > 0 || t('login.sso.username_required'),
+ }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/login/sso-section.tsx` around lines 91 - 92, Validation allows
whitespace-only usernames; update the rules prop to validate trimmed input
(e.g., rules={{ validate: v => v?.trim() ? true :
t('login.sso.username_required') }}) and in the render block that destructures
field ({ onChange, onBlur, value }) ensure any lookup or submission logic uses
value.trim() and aborts/returns early if trimmed value is empty so lookup is
never performed for whitespace-only input.
Comment on lines
+47
to
+50
| function dateMinusOneDay(dateStr: string): string { | ||
| const d = new Date(dateStr + 'T00:00:00'); | ||
| d.setDate(d.getDate() - 1); | ||
| return d.toISOString().slice(0, 10); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, does new Date('YYYY-MM-DDT00:00:00')parse in local time, and cantoISOString().slice(0,10) shift the date in UTC+ offsets? What is the UTC-safe way to subtract one day from a YYYY-MM-DD string?
💡 Result:
- Yes: per the spec/MDN, ISO “date-time” strings without an offset (e.g.
YYYY-MM-DDT00:00:00) are parsed as local time, while date-only strings (YYYY-MM-DD) are parsed as UTC. [1], [2] - Yes:
toISOString()always outputs UTC (Z). So if you parsed a local-midnight date-time,toISOString().slice(0,10)can shift the calendar day for non‑UTC time zones (e.g., UTC+ offsets often become the previous UTC date). [1]
UTC-safe “subtract one day” from a YYYY-MM-DD string
function subtractOneDayUTC(ymd) {
const [y, m, d] = ymd.split("-").map(Number);
const dt = new Date(Date.UTC(y, m - 1, d)); // midnight UTC
dt.setUTCDate(dt.getUTCDate() - 1);
return dt.toISOString().slice(0, 10);
}This avoids local-time parsing entirely by constructing the instant at 00:00:00 UTC via Date.UTC(...). [3], [1]
Sources:
[1] MDN: Date (parsing rules; toISOString() returns UTC Z) (developer.mozilla.org)
[2] TC39: uniform date parsing proposal (date-only = UTC, date-time = local when no offset) (tc39.es)
[3] MDN: Date.UTC() (developer.mozilla.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date?utm_source=openai
- 2: https://tc39.es/proposal-uniform-interchange-date-parsing/?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/UTC?utm_source=openai
Use UTC-safe date math in dateMinusOneDay to prevent timezone drift.
The function parses in local time and serializes in UTC. In UTC+ timezones, local midnight becomes the previous UTC date, causing an unintended extra day subtraction that will misclassify IsMultiDay.
🩹 Suggested fix
function dateMinusOneDay(dateStr: string): string {
- const d = new Date(dateStr + 'T00:00:00');
- d.setDate(d.getDate() - 1);
+ const [year, month, day] = dateStr.split('-').map(Number);
+ const d = new Date(Date.UTC(year, month - 1, day));
+ d.setUTCDate(d.getUTCDate() - 1);
return d.toISOString().slice(0, 10);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/v4/calendar/calendarItemResultData.ts` around lines 47 - 50, The
dateMinusOneDay function uses local-time parsing and UTC serialization which
causes timezone drift; update dateMinusOneDay to perform UTC-safe arithmetic by
parsing the YYYY-MM-DD string into year/month/day integers, creating a Date via
Date.UTC or new Date(Date.UTC(...)), subtracting one day with
setUTCDate(getUTCDate() - 1), and returning the date portion from
toISOString().slice(0,10); ensure you only change the implementation of
dateMinusOneDay so callers (e.g., IsMultiDay logic) get a correct UTC-based
previous date.
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
Improvements
Localization