Conversation
📝 WalkthroughWalkthroughAdds end-to-end Single Sign-On (SSO) support: new SSO UI, OIDC and SAML hooks, discovery service, backend exchange API, auth-store integration, tests, mocks, new Expo dependencies, and translation strings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as SsoLogin Screen
participant SsoService as SSO Discovery
participant Backend as Resgrid Backend
participant OidcHook as useOidcLogin Hook
participant IdP as OIDC Provider
participant Store as Auth Store
User->>App: Enter username/departmentId
App->>SsoService: fetchSsoConfigForUser(username, departmentId)
SsoService->>Backend: GET /connect/sso-config-for-user
Backend-->>SsoService: { ssoEnabled, providerType, ... }
SsoService-->>App: SsoConfig
App->>OidcHook: promptAsync()
OidcHook->>IdP: Authorization Request (PKCE)
IdP-->>OidcHook: Authorization Code
App->>OidcHook: exchangeForResgridToken(username)
OidcHook->>IdP: Exchange code for id_token
IdP-->>OidcHook: id_token
OidcHook->>Backend: POST /connect/external-token (id_token, provider=oidc)
Backend-->>OidcHook: { access_token, refresh_token, expires_in }
App->>Store: ssoLogin({ externalToken, provider, username })
Store->>Store: Persist tokens, update profile
Store-->>App: Success
App->>User: Redirect to Home
sequenceDiagram
participant User
participant App as SsoLogin Screen
participant SamlHook as useSamlLogin Hook
participant Browser as WebBrowser
participant IdP as SAML IdP
participant DeepLink as System Deep Link
participant Backend as Resgrid Backend
participant Store as Auth Store
User->>App: Enter username/departmentId
App->>SamlHook: startSamlLogin(idpSsoUrl)
SamlHook->>Browser: openBrowserAsync(idpSsoUrl)
Browser->>IdP: User authenticates
IdP->>DeepLink: Redirect to resgridunit://auth/callback?saml_response=...
DeepLink->>SamlHook: handleDeepLink(url, username)
SamlHook->>Backend: POST /connect/external-token (saml_response, provider=saml2)
Backend-->>SamlHook: { access_token, refresh_token, expires_in }
App->>Store: ssoLogin({ externalToken, provider, username })
Store->>Store: Persist tokens, update profile
Store-->>App: Success
App->>User: Redirect to Home
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
🧹 Nitpick comments (12)
src/hooks/use-saml-login.ts (2)
52-80: Consider adding a timeout to the token exchange request.The axios POST to
/connect/external-tokenhas no timeout configured. If the backend is slow or unresponsive, this could leave the user waiting indefinitely.♻️ Add request timeout
- const resgridResponse = await axios.post<SamlExchangeResult>(`${getBaseApiUrl()}/connect/external-token`, params.toString(), { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } }); + const resgridResponse = await axios.post<SamlExchangeResult>( + `${getBaseApiUrl()}/connect/external-token`, + params.toString(), + { + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + timeout: 30000, // 30 second timeout + } + );🤖 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 52 - 80, The axios POST in handleDeepLink (function handleDeepLink) lacks a timeout, so add a request timeout to the token exchange call to avoid hanging; update the axios.post call to include a timeout (e.g., 10000 ms) in the request config alongside the existing headers or create an axios instance with a default timeout and use it for the call to `${getBaseApiUrl()}/connect/external-token`, ensuring the timeout is applied to the external-token exchange and handling any timeout errors in the existing catch block.
85-87: Consider a more robust callback URL check.The current implementation uses simple
includes()checks which could match unintended URLs (e.g.,https://evil.com/auth/callback?saml_response=...). Consider verifying the URL scheme matches your app's scheme.♻️ More robust URL validation
function isSamlCallback(url: string): boolean { - return url.includes('auth/callback') && url.includes('saml_response'); + const lowerUrl = url.toLowerCase(); + return ( + lowerUrl.startsWith('resgridunit://') && + lowerUrl.includes('auth/callback') && + lowerUrl.includes('saml_response') + ); }🤖 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 85 - 87, The isSamlCallback function currently uses string includes and should instead parse and validate the URL components: use the URL constructor to parse the input, verify the protocol and host/hostname (compare against your app's expected scheme/origin or a configured allowedOrigins list), ensure pathname exactly matches '/auth/callback' (or the canonical callback path), and confirm the presence of the 'saml_response' query parameter via url.searchParams.has('saml_response'); update isSamlCallback to perform these checks so arbitrary external URLs can't falsely match.src/stores/roles/store.ts (1)
101-101: Consider line length for readability.This line is quite long (~180 chars). The logic is correct, but splitting across lines would improve readability. This is a minor nit.
♻️ Optional: Split for readability
- const [rolesResponse, unitRoles, personnelResponse] = await Promise.all([getAllUnitRolesAndAssignmentsForDepartment(), getRoleAssignmentsForUnit(unitId), getAllPersonnelInfos('')]); + const [rolesResponse, unitRoles, personnelResponse] = await Promise.all([ + getAllUnitRolesAndAssignmentsForDepartment(), + getRoleAssignmentsForUnit(unitId), + getAllPersonnelInfos(''), + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/roles/store.ts` at line 101, The long Promise.all line assigning rolesResponse, unitRoles, personnelResponse should be split for readability: break the array argument across multiple lines and place each awaited call on its own line so the call to getAllUnitRolesAndAssignmentsForDepartment(), getRoleAssignmentsForUnit(unitId), and getAllPersonnelInfos('') are each on separate lines while keeping the await Promise.all([...]) structure and the destructuring into const [rolesResponse, unitRoles, personnelResponse].src/services/sso-discovery.ts (1)
37-61: Consider logging or differentiating error types.The 404 check (lines 55-58) and the generic fallback (line 59) both return the same result. While this is safe, consider logging non-404 errors for debugging, as they indicate unexpected failures (network issues, 500s) vs. expected "user not found" scenarios.
♻️ Add error logging for debugging
} catch (error: unknown) { if (axios.isAxiosError(error) && error.response?.status === 404) { // User not a member of the specified department return { config: null, userExists: false }; } + // Log unexpected errors for debugging + console.warn('SSO config fetch failed:', error instanceof Error ? error.message : error); return { config: null, userExists: false }; }🤖 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 37 - 61, In fetchSsoConfigForUser update the catch block to log unexpected/non-404 errors for debugging: keep the existing axios.isAxiosError(error) && error.response?.status === 404 branch returning {config: null, userExists: false}, but for other cases call your logger (e.g., logger.error or console.error) with the error, error.message and error.response?.status/details (using axios.isAxiosError(error) checks) before returning the same fallback; reference fetchSsoConfigForUser, the catch error variable, and axios.isAxiosError when making this change.src/hooks/__tests__/use-saml-login.test.ts (1)
93-97: Strengthen request-body assertions for SAML exchange.This currently verifies only
provider=saml2; add checks forexternal_tokenandusernameto lock the contract.🧪 Suggested assertion expansion
expect(mockedAxios.post).toHaveBeenCalledWith( 'https://api.resgrid.com/api/v4/connect/external-token', - expect.stringContaining('provider=saml2'), + expect.stringContaining('provider=saml2'), + expect.any(Object), + ); + expect(mockedAxios.post.mock.calls[0][1]).toEqual(expect.stringContaining('external_token=base64SamlResponse')); + expect(mockedAxios.post.mock.calls[0][1]).toEqual(expect.stringContaining('username=john.doe')); - expect.any(Object), - );Based on learnings: Applies to **/*.{test,spec}.{ts,tsx} : Generate tests for all components, services and logic generated.
🤖 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 93 - 97, Update the test assertion that checks mockedAxios.post in use-saml-login.test.ts to also verify the request body contains the external token and username fields; specifically modify the expect for mockedAxios.post (the call being asserted in the test) to include expect.stringContaining('external_token=') and expect.stringContaining('username=') alongside the existing expect.stringContaining('provider=saml2') so the POST payload contract is fully validated.src/hooks/use-oidc-login.ts (1)
83-92: Prefer the shared SSO API helper to avoid request-contract drift.This hook duplicates
/connect/external-tokenrequest assembly instead of usingsrc/lib/auth/api.tsx:ssoExternalTokenRequest, which centralizes provider payload and logging behavior.♻️ Refactor sketch
-import axios from 'axios'; +import { ssoExternalTokenRequest } from '@/lib/auth/api'; ... - const params = new URLSearchParams({ - provider: 'oidc', - external_token: idToken, - username, - scope: 'openid email profile offline_access mobile', - }); - - const resgridResponse = await axios.post<OidcExchangeResult>(`${getBaseApiUrl()}/connect/external-token`, params.toString(), { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } }); + const exchange = await ssoExternalTokenRequest({ + provider: 'oidc', + externalToken: idToken, + username, + }); + if (!exchange.successful || !exchange.authResponse) return null; logger.info({ message: 'OIDC Resgrid token exchange successful' }); - return resgridResponse.data; + return exchange.authResponse;🤖 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 83 - 92, The hook currently builds URLSearchParams and calls axios.post directly to /connect/external-token (producing resgridResponse); replace that manual assembly with the shared helper ssoExternalTokenRequest from src/lib/auth/api.tsx to avoid drift: call ssoExternalTokenRequest with the same inputs (provider: 'oidc', external_token: idToken, username, scope) and use its returned result (typed as OidcExchangeResult) in place of resgridResponse while preserving the existing error handling and logging in use-oidc-login.ts.src/lib/auth/__tests__/sso-api.test.ts (1)
48-51: Expand POST payload assertions beyond provider.To harden this test against request-shape regressions, also assert
external_tokenandusernamein both success cases.🧪 Suggested assertions
expect(mockPost).toHaveBeenCalledWith( '/connect/external-token', expect.stringContaining('provider=oidc'), ); + expect(mockPost.mock.calls[0][1]).toEqual(expect.stringContaining('external_token=idp-id-token')); + expect(mockPost.mock.calls[0][1]).toEqual(expect.stringContaining('username=john.doe')); ... expect(mockPost).toHaveBeenCalledWith( '/connect/external-token', expect.stringContaining('provider=saml2'), ); + expect(mockPost.mock.calls[1][1]).toEqual(expect.stringContaining('external_token=base64SamlResponse')); + expect(mockPost.mock.calls[1][1]).toEqual(expect.stringContaining('username=john.doe'));Based on learnings: Applies to **/*.{test,spec}.{ts,tsx} : Create and use Jest to test to validate all generated components and ensure tests run without errors.
Also applies to: 73-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/__tests__/sso-api.test.ts` around lines 48 - 51, The test currently only asserts that mockPost was called with '/connect/external-token' and a payload containing 'provider=oidc'; update the assertions for both success cases in sso-api.test.ts to also validate that the POST payload includes the expected external_token and username values—locate the expect(mockPost).toHaveBeenCalledWith(...) call(s) and extend the payload checks to assert those two fields (use the same string/object matching style used for provider so the test verifies provider, external_token, and username together).src/hooks/__tests__/use-oidc-login.test.ts (1)
43-51: Add an assertion for redirect URI construction.Current tests don’t validate the
makeRedirectUriinput, so scheme/path regressions can slip through.✅ Suggested assertion
it('renders without error', () => { const { result } = renderHook(() => useOidcLogin({ authority: 'https://idp.example.com', clientId: 'client123' }), ); + expect(mockedAuthSession.makeRedirectUri).toHaveBeenCalledWith({ + scheme: 'resgridunit', + path: 'auth/callback', + }); expect(result.current.request).toBeDefined();Based on learnings: Applies to /tests//*.{ts,tsx} : Generate tests for all components, services, and logic; ensure tests run without errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/__tests__/use-oidc-login.test.ts` around lines 43 - 51, Add an assertion to this test to validate the redirect URI produced for the auth request: after rendering useOidcLogin, call the same makeRedirectUri parameters used by the hook (or construct the expected URI string) and assert that result.current.request.redirectUri equals that expected value and contains the expected scheme/path segments; reference the hook useOidcLogin and the request.redirectUri property (or makeRedirectUri helper) to locate where to compare and ensure the test fails if the scheme or path changes.src/app/login/__tests__/index.test.tsx (1)
108-123: Add SSO button wiring in the mockedLoginFormand assert navigation.The mock currently cannot trigger
onSsoPress, so/login/ssorouting is untested.🧪 Suggested test-harness update
- LoginForm: ({ onSubmit, isLoading, error, onServerUrlPress }: any) => + LoginForm: ({ onSubmit, isLoading, error, onServerUrlPress, onSsoPress }: any) => React.createElement(View, { testID: 'login-form' }, [ ... + onSsoPress && React.createElement(TouchableOpacity, { + key: 'sso', + onPress: onSsoPress, + testID: 'sso-button' + }, 'SSO'),Based on learnings: Applies to /tests//*.{ts,tsx} : Generate tests for all components, services, and logic; ensure tests run without errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/__tests__/index.test.tsx` around lines 108 - 123, The mocked LoginForm is missing wiring for onSsoPress so the SSO flow and navigation to '/login/sso' aren't tested; modify the LoginForm mock to render a TouchableOpacity with testID 'sso-button' that calls the passed onSsoPress prop when pressed (similar to the existing submit and server-url buttons), then update the test to fire a press on 'sso-button' and assert that navigation occurred to the '/login/sso' route.src/lib/auth/api.tsx (1)
84-97: Minor: Docstring path doesn't match actual endpoint.The docstring references
/api/v4/connect/external-tokenbut the actual POST is to/connect/external-token. SinceauthApi.baseURLhandles the base path, the code is correct, but the comment could be updated for accuracy.🤖 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 84 - 97, The docstring for ssoExternalTokenRequest incorrectly states the full path "/api/v4/connect/external-token" while the function posts to the relative path "/connect/external-token" (authApi.baseURL provides the base); update the comment to either mention the relative endpoint "/connect/external-token" or note that authApi.baseURL includes "/api/v4" so the current POST target is correct, ensuring the docstring and the authApi usage are consistent.src/app/login/sso.tsx (1)
275-275: Type assertion on Modal props may hide type errors.The spread
{...({} as any)}suppresses type checking. If this is a workaround for a library typing issue, consider adding a comment explaining why, or filing an issue upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/sso.tsx` at line 275, The Modal usage includes an unsafe spread `{...({} as any)}` which silences type errors; remove that spread and either pass the explicit props Modal requires or narrow the type properly (e.g., cast a well-typed props object) in the component using isErrorModalVisible and setIsErrorModalVisible; if this was a deliberate workaround for a library typing bug, replace the spread with a short comment referencing the upstream issue/PR and add a TODO to remove the cast once fixed so Modal props typing (used in the Modal element) remains enforced.src/stores/auth/store.tsx (1)
89-133: LGTM with optional refactor suggestion.The
ssoLoginimplementation correctly mirrors the existingloginflow: it validates the response, decodes the ID token, persists credentials, updates state, and schedules token refresh. The timeout cleanup logic properly prevents stacked timeouts.Consider extracting the shared post-authentication logic (token decoding, state updates, refresh scheduling) into a helper function to reduce duplication with
login. This is a nice-to-have for maintainability.🤖 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 89 - 133, Extract the shared post-authentication steps from ssoLogin into a helper (e.g., applyAuthResponse) and call it from both ssoLogin and the existing login to remove duplication: the helper should accept the AuthResponse and provider/credentials context, perform ID token decoding via sanitizeJson(base64.decode(...)), persist the auth response with setItem<AuthResponse>('authResponse', ...), compute and store refreshTokenExpiresOn, update state fields (accessToken, refreshToken, profile, userId, status, error) using set(...), clear any existing refresh timeout using refreshTimeoutId and clearTimeout, schedule the refresh with setTimeout(() => get().refreshAccessToken(), computed refreshDelayMs) and store the timeout id in refreshTimeoutId, and emit the same logger.info call; update ssoLogin to call applyAuthResponse(response.authResponse, credentials.provider) and only handle the error branch as before.
🤖 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/index.tsx`:
- Around line 49-51: The client-side logger call in onSubmit is recording raw
user identifiers (logger.info with context { username: data.username }), which
risks exposing PII; update the onSubmit handler to remove or redact the raw
username before logging (e.g., log a non-identifying event or use a
hashed/obfuscated identifier), modifying the logger.info invocation in the
onSubmit function (type LoginFormProps['onSubmit']) that wraps the call to login
so it no longer includes data.username in the log context.
In `@src/app/login/login-form.tsx`:
- Line 47: The function signature for the component LoginForm has extra
whitespace around the default empty function for onSubmit and/or around the
default values; remove the extra spaces so the signature is consistently
formatted (e.g., change "onSubmit = () => { }" to "onSubmit = () => {}") and
ensure spacing around commas and default assignments in the LoginForm
declaration is normalized; update the LoginForm component declaration to use
compact, consistent spacing for the default props (onSubmit, isLoading, error,
onServerUrlPress, onSsoPress).
In `@src/app/login/sso.tsx`:
- Around line 96-116: handleDeepLink currently performs the Resgrid token
exchange, so passing result.access_token into ssoLogin causes a duplicate
exchange; update the SAML deep-link handler inside the useEffect to either (A)
stop calling ssoLogin and instead consume the tokens returned by handleDeepLink
(set session/token state directly using the value returned from handleDeepLink
and call setIsSsoLoading(false)), or (B) change the argument passed to ssoLogin
to be the original SAML response (or the field handleDeepLink returns that
represents the raw SAML response) so ssoLogin performs the single expected
exchange; locate the logic around isSamlCallback, handleDeepLink,
pendingUsernameRef.current and ssoLogin in this file and implement one of these
fixes and ensure errors still set setIsErrorModalVisible(true) and
setIsSsoLoading(false).
- Around line 71-94: The current useEffect calls exchangeForResgridToken and
then passes result.access_token into ssoLogin, causing a double exchange;
instead, update the auth state directly with the Resgrid tokens returned by
exchangeForResgridToken (use the returned result to set whatever store/update
function currently used by ssoLogin) and remove the ssoLogin call in the effect
(or alternatively change exchangeForResgridToken to return only the IdP id_token
and let ssoLogin perform the backend exchange) — locate this logic in the
useEffect that references oidc.exchangeForResgridToken and modify either the
effect to consume result (Resgrid tokens) directly or refactor
exchangeForResgridToken to return the raw id_token so ssoLogin remains
responsible for calling /connect/external-token.
In `@src/hooks/use-oidc-login.ts`:
- Around line 34-37: The redirect URI scheme passed to
AuthSession.makeRedirectUri uses 'ResgridUnit' which is cased incorrectly;
update the scheme value in the redirectUri construction
(AuthSession.makeRedirectUri in use-oidc-login.ts) to the lowercase
'resgridunit' so it matches the Android intent filter and iOS
LSApplicationQueriesSchemes registrations and allows the OIDC callback to route
back into the app.
In `@src/services/__tests__/sso-discovery.test.ts`:
- Around line 84-95: Update the test description string in the it(...) block
that currently reads 'returns { config: null, userExists: false } when
ssoEnabled is false' to accurately describe the assertion (e.g. 'returns config
with ssoEnabled false and userExists true when ssoEnabled is false') so the test
name matches the behavior exercised by fetchSsoConfigForUser; the test uses
mockedAxios.get to return { Data: { ssoEnabled: false, allowLocalLogin: true } }
and asserts the resulting object, so only the it(...) description should be
changed.
In `@src/stores/app/livekit-store.ts`:
- Line 457: Replace the hardcoded Alert.alert call in livekit-store.ts with a
translated string using the shared i18n accessor (t from react-i18next); locate
the timeout handling where Alert.alert('Voice Connection Timeout', `The
connection to "${roomInfo.Name}" took too long. Please try again.`, ...) is used
(e.g., inside the voice connection timeout handler in the LiveKitStore class or
related function) and change both the title and message to use translation keys
with parameters — e.g., t('voice.connectionTimeoutTitle') for the title and
t('voice.connectionTimeoutMessage', { name: roomInfo.Name }) for the message —
so the alert uses t(...) and the roomInfo.Name is passed as a param to the
translation call.
---
Nitpick comments:
In `@src/app/login/__tests__/index.test.tsx`:
- Around line 108-123: The mocked LoginForm is missing wiring for onSsoPress so
the SSO flow and navigation to '/login/sso' aren't tested; modify the LoginForm
mock to render a TouchableOpacity with testID 'sso-button' that calls the passed
onSsoPress prop when pressed (similar to the existing submit and server-url
buttons), then update the test to fire a press on 'sso-button' and assert that
navigation occurred to the '/login/sso' route.
In `@src/app/login/sso.tsx`:
- Line 275: The Modal usage includes an unsafe spread `{...({} as any)}` which
silences type errors; remove that spread and either pass the explicit props
Modal requires or narrow the type properly (e.g., cast a well-typed props
object) in the component using isErrorModalVisible and setIsErrorModalVisible;
if this was a deliberate workaround for a library typing bug, replace the spread
with a short comment referencing the upstream issue/PR and add a TODO to remove
the cast once fixed so Modal props typing (used in the Modal element) remains
enforced.
In `@src/hooks/__tests__/use-oidc-login.test.ts`:
- Around line 43-51: Add an assertion to this test to validate the redirect URI
produced for the auth request: after rendering useOidcLogin, call the same
makeRedirectUri parameters used by the hook (or construct the expected URI
string) and assert that result.current.request.redirectUri equals that expected
value and contains the expected scheme/path segments; reference the hook
useOidcLogin and the request.redirectUri property (or makeRedirectUri helper) to
locate where to compare and ensure the test fails if the scheme or path changes.
In `@src/hooks/__tests__/use-saml-login.test.ts`:
- Around line 93-97: Update the test assertion that checks mockedAxios.post in
use-saml-login.test.ts to also verify the request body contains the external
token and username fields; specifically modify the expect for mockedAxios.post
(the call being asserted in the test) to include
expect.stringContaining('external_token=') and
expect.stringContaining('username=') alongside the existing
expect.stringContaining('provider=saml2') so the POST payload contract is fully
validated.
In `@src/hooks/use-oidc-login.ts`:
- Around line 83-92: The hook currently builds URLSearchParams and calls
axios.post directly to /connect/external-token (producing resgridResponse);
replace that manual assembly with the shared helper ssoExternalTokenRequest from
src/lib/auth/api.tsx to avoid drift: call ssoExternalTokenRequest with the same
inputs (provider: 'oidc', external_token: idToken, username, scope) and use its
returned result (typed as OidcExchangeResult) in place of resgridResponse while
preserving the existing error handling and logging in use-oidc-login.ts.
In `@src/hooks/use-saml-login.ts`:
- Around line 52-80: The axios POST in handleDeepLink (function handleDeepLink)
lacks a timeout, so add a request timeout to the token exchange call to avoid
hanging; update the axios.post call to include a timeout (e.g., 10000 ms) in the
request config alongside the existing headers or create an axios instance with a
default timeout and use it for the call to
`${getBaseApiUrl()}/connect/external-token`, ensuring the timeout is applied to
the external-token exchange and handling any timeout errors in the existing
catch block.
- Around line 85-87: The isSamlCallback function currently uses string includes
and should instead parse and validate the URL components: use the URL
constructor to parse the input, verify the protocol and host/hostname (compare
against your app's expected scheme/origin or a configured allowedOrigins list),
ensure pathname exactly matches '/auth/callback' (or the canonical callback
path), and confirm the presence of the 'saml_response' query parameter via
url.searchParams.has('saml_response'); update isSamlCallback to perform these
checks so arbitrary external URLs can't falsely match.
In `@src/lib/auth/__tests__/sso-api.test.ts`:
- Around line 48-51: The test currently only asserts that mockPost was called
with '/connect/external-token' and a payload containing 'provider=oidc'; update
the assertions for both success cases in sso-api.test.ts to also validate that
the POST payload includes the expected external_token and username values—locate
the expect(mockPost).toHaveBeenCalledWith(...) call(s) and extend the payload
checks to assert those two fields (use the same string/object matching style
used for provider so the test verifies provider, external_token, and username
together).
In `@src/lib/auth/api.tsx`:
- Around line 84-97: The docstring for ssoExternalTokenRequest incorrectly
states the full path "/api/v4/connect/external-token" while the function posts
to the relative path "/connect/external-token" (authApi.baseURL provides the
base); update the comment to either mention the relative endpoint
"/connect/external-token" or note that authApi.baseURL includes "/api/v4" so the
current POST target is correct, ensuring the docstring and the authApi usage are
consistent.
In `@src/services/sso-discovery.ts`:
- Around line 37-61: In fetchSsoConfigForUser update the catch block to log
unexpected/non-404 errors for debugging: keep the existing
axios.isAxiosError(error) && error.response?.status === 404 branch returning
{config: null, userExists: false}, but for other cases call your logger (e.g.,
logger.error or console.error) with the error, error.message and
error.response?.status/details (using axios.isAxiosError(error) checks) before
returning the same fallback; reference fetchSsoConfigForUser, the catch error
variable, and axios.isAxiosError when making this change.
In `@src/stores/auth/store.tsx`:
- Around line 89-133: Extract the shared post-authentication steps from ssoLogin
into a helper (e.g., applyAuthResponse) and call it from both ssoLogin and the
existing login to remove duplication: the helper should accept the AuthResponse
and provider/credentials context, perform ID token decoding via
sanitizeJson(base64.decode(...)), persist the auth response with
setItem<AuthResponse>('authResponse', ...), compute and store
refreshTokenExpiresOn, update state fields (accessToken, refreshToken, profile,
userId, status, error) using set(...), clear any existing refresh timeout using
refreshTimeoutId and clearTimeout, schedule the refresh with setTimeout(() =>
get().refreshAccessToken(), computed refreshDelayMs) and store the timeout id in
refreshTimeoutId, and emit the same logger.info call; update ssoLogin to call
applyAuthResponse(response.authResponse, credentials.provider) and only handle
the error branch as before.
In `@src/stores/roles/store.ts`:
- Line 101: The long Promise.all line assigning rolesResponse, unitRoles,
personnelResponse should be split for readability: break the array argument
across multiple lines and place each awaited call on its own line so the call to
getAllUnitRolesAndAssignmentsForDepartment(), getRoleAssignmentsForUnit(unitId),
and getAllPersonnelInfos('') are each on separate lines while keeping the await
Promise.all([...]) structure and the destructuring into const [rolesResponse,
unitRoles, personnelResponse].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90d139f0-4925-4591-addc-b294f77a823b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
__mocks__/expo-auth-session.ts__mocks__/expo-web-browser.tsapp.config.tspackage.jsonsrc/app/login/__tests__/index.test.tsxsrc/app/login/index.tsxsrc/app/login/login-form.tsxsrc/app/login/sso.tsxsrc/components/maps/static-map.tsxsrc/components/roles/role-assignment-item.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/__tests__/sso-api.test.tssrc/lib/auth/api.tsxsrc/lib/auth/index.tsxsrc/lib/auth/types.tsxsrc/services/__tests__/sso-discovery.test.tssrc/services/sso-discovery.tssrc/stores/app/livekit-store.tssrc/stores/auth/store.tsxsrc/stores/roles/store.tssrc/translations/ar.jsonsrc/translations/en.jsonsrc/translations/es.json
| }; | ||
|
|
||
| export const LoginForm = ({ onSubmit = () => {}, isLoading = false, error = undefined, onServerUrlPress }: LoginFormProps) => { | ||
| export const LoginForm = ({ onSubmit = () => { }, isLoading = false, error = undefined, onServerUrlPress, onSsoPress }: LoginFormProps) => { |
There was a problem hiding this comment.
Fix formatting: extra whitespace.
Static analysis flagged extra whitespace in the function signature.
-export const LoginForm = ({ onSubmit = () => { }, isLoading = false, error = undefined, onServerUrlPress, onSsoPress }: LoginFormProps) => {
+export const LoginForm = ({ onSubmit = () => {}, isLoading = false, error = undefined, onServerUrlPress, onSsoPress }: LoginFormProps) => {📝 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
| export const LoginForm = ({ onSubmit = () => { }, isLoading = false, error = undefined, onServerUrlPress, onSsoPress }: LoginFormProps) => { | |
| export const LoginForm = ({ onSubmit = () => {}, isLoading = false, error = undefined, onServerUrlPress, onSsoPress }: LoginFormProps) => { |
🧰 Tools
🪛 GitHub Check: test
[warning] 47-47:
Delete ·
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/login/login-form.tsx` at line 47, The function signature for the
component LoginForm has extra whitespace around the default empty function for
onSubmit and/or around the default values; remove the extra spaces so the
signature is consistently formatted (e.g., change "onSubmit = () => { }" to
"onSubmit = () => {}") and ensure spacing around commas and default assignments
in the LoginForm declaration is normalized; update the LoginForm component
declaration to use compact, consistent spacing for the default props (onSubmit,
isLoading, error, onServerUrlPress, onSsoPress).
Comment on lines
+84
to
+95
| it('returns { config: null, userExists: false } when ssoEnabled is false', async () => { | ||
| mockedAxios.get = jest.fn().mockResolvedValueOnce({ | ||
| data: { Data: { ssoEnabled: false, allowLocalLogin: true } }, | ||
| }); | ||
|
|
||
| const result = await fetchSsoConfigForUser('localuser'); | ||
|
|
||
| expect(result).toEqual({ | ||
| config: { ssoEnabled: false, allowLocalLogin: true }, | ||
| userExists: true, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test description doesn't match assertion.
The test is named 'returns { config: null, userExists: false } when ssoEnabled is false' but the assertion expects { config: { ssoEnabled: false, allowLocalLogin: true }, userExists: true }. The assertion is correct (SSO disabled still returns the config), but the description is misleading.
🔧 Fix test description
- it('returns { config: null, userExists: false } when ssoEnabled is false', async () => {
+ it('returns config with userExists=true when ssoEnabled is false', async () => {
mockedAxios.get = jest.fn().mockResolvedValueOnce({
data: { Data: { ssoEnabled: false, allowLocalLogin: true } },
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/__tests__/sso-discovery.test.ts` around lines 84 - 95, Update
the test description string in the it(...) block that currently reads 'returns {
config: null, userExists: false } when ssoEnabled is false' to accurately
describe the assertion (e.g. 'returns config with ssoEnabled false and
userExists true when ssoEnabled is false') so the test name matches the behavior
exercised by fetchSsoConfigForUser; the test uses mockedAxios.get to return {
Data: { ssoEnabled: false, allowLocalLogin: true } } and asserts the resulting
object, so only the it(...) description should be changed.
| `The connection to "${roomInfo.Name}" took too long. Please try again.`, | ||
| [{ text: 'OK' }] | ||
| ); | ||
| Alert.alert('Voice Connection Timeout', `The connection to "${roomInfo.Name}" took too long. Please try again.`, [{ text: 'OK' }]); |
There was a problem hiding this comment.
Localize newly touched alert strings instead of hardcoding text
This modified alert message is still hardcoded and bypasses the translation pipeline. Please replace it with translation keys/params (including roomInfo.Name) via your shared i18n accessor.
As per coding guidelines, "Ensure all text is wrapped in t() from react-i18next for translations with dictionary files stored in src/translations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/app/livekit-store.ts` at line 457, Replace the hardcoded
Alert.alert call in livekit-store.ts with a translated string using the shared
i18n accessor (t from react-i18next); locate the timeout handling where
Alert.alert('Voice Connection Timeout', `The connection to "${roomInfo.Name}"
took too long. Please try again.`, ...) is used (e.g., inside the voice
connection timeout handler in the LiveKitStore class or related function) and
change both the title and message to use translation keys with parameters —
e.g., t('voice.connectionTimeoutTitle') for the title and
t('voice.connectionTimeoutMessage', { name: roomInfo.Name }) for the message —
so the alert uses t(...) and the roomInfo.Name is passed as a param to the
translation call.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/login/sso.tsx (3)
71-94: Consider adding cleanup to prevent state updates after unmount.The
.then()callback may execute after the component unmounts (e.g., user navigates away during the exchange), causing React warnings about state updates on unmounted components.🛡️ Proposed fix with abort flag
useEffect(() => { if (oidc.response?.type !== 'success') return; + let cancelled = false; setIsSsoLoading(true); oidc .exchangeForResgridToken() .then((idToken) => { + if (cancelled) return; if (!idToken) { setIsSsoLoading(false); setIsErrorModalVisible(true); return; } ssoLogin({ provider: 'oidc', externalToken: idToken, username: pendingUsernameRef.current, }); }) .catch(() => { + if (cancelled) return; setIsSsoLoading(false); setIsErrorModalVisible(true); }); + + return () => { cancelled = true; }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [oidc.response]);🤖 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 71 - 94, The effect handling oidc.exchangeForResgridToken can update state after the component unmounts; add a local "cancelled/mounted" flag (or AbortController) inside the useEffect and check it before calling setIsSsoLoading, setIsErrorModalVisible, and ssoLogin to prevent updates post-unmount; ensure you set the flag to true on cleanup (return () => { cancelled = true } ) and reference the existing symbols oidc.exchangeForResgridToken, setIsSsoLoading, setIsErrorModalVisible, ssoLogin, and pendingUsernameRef.current when gating the callbacks.
119-139: Network errors appear as "SSO not found" to the user.Since
fetchSsoConfigForUserreturns{ config: null, userExists: false }for both "user not found" and network errors (persso-discovery.ts), users cannot distinguish between configuration issues and connectivity problems. Consider exposing error state for better UX feedback.💡 Suggestion to distinguish error states
The discovery service could return a more detailed result:
type SsoLookupResult = | { status: 'found'; config: DepartmentSsoConfig } | { status: 'not_found' } | { status: 'error'; message?: string };Then display appropriate messaging:
not_found: "SSO not configured for this user"error: "Unable to check SSO configuration. Please try again."🤖 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 119 - 139, triggerSsoLookup currently treats all failures from fetchSsoConfigForUser as "not found"; update the contract of fetchSsoConfigForUser to return a detailed result (e.g., { status: 'found'|'not_found'|'error', config?: DepartmentSsoConfig, message?: string }) and then change triggerSsoLookup to inspect that status: setIsLookingUpSso(false) on all paths, setSsoConfig(config) only for 'found', and set a new error state (e.g., setSsoLookupError or reuse an existing state) for 'error' with the returned message so the UI can display "Unable to check SSO configuration" distinct from "SSO not configured". Ensure pendingUsernameRef handling remains unchanged and include logging via logger.info/error that records the status and message.
276-276: Remove{...({} as any)}spread on Modal to avoid TypeScript workarounds.The spread of an empty object typed as
anydoes nothing at runtime but violates the guideline to avoidany. This pattern appears in multiple files (sso.tsx line 276, _layout.tsx, full-screen-image-modal.tsx, and others) with Modal and Drawer components. Since the Modal component has proper TypeScript types (IModalPropsincludes all valid props), investigate why the workaround is needed and resolve the underlying type mismatch instead of masking it.
🤖 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 26-29: The zod schema ssoFormSchema currently uses hardcoded
English messages for username (required and min) — replace those strings with
translation keys (e.g. 'errors.username.required', 'errors.username.minLength')
for both the username and any other messages (departmentId remains optional),
then in the form error rendering logic that displays zod errors (the component
that reads errors from ssoFormSchema) call t(error.message as translationKey) to
resolve the key at render time; ensure you update both the .min and
required_error usages on ssoFormSchema.username and map any displayed error
messages through react-i18next's t() before showing to the user.
---
Nitpick comments:
In `@src/app/login/sso.tsx`:
- Around line 71-94: The effect handling oidc.exchangeForResgridToken can update
state after the component unmounts; add a local "cancelled/mounted" flag (or
AbortController) inside the useEffect and check it before calling
setIsSsoLoading, setIsErrorModalVisible, and ssoLogin to prevent updates
post-unmount; ensure you set the flag to true on cleanup (return () => {
cancelled = true } ) and reference the existing symbols
oidc.exchangeForResgridToken, setIsSsoLoading, setIsErrorModalVisible, ssoLogin,
and pendingUsernameRef.current when gating the callbacks.
- Around line 119-139: triggerSsoLookup currently treats all failures from
fetchSsoConfigForUser as "not found"; update the contract of
fetchSsoConfigForUser to return a detailed result (e.g., { status:
'found'|'not_found'|'error', config?: DepartmentSsoConfig, message?: string })
and then change triggerSsoLookup to inspect that status:
setIsLookingUpSso(false) on all paths, setSsoConfig(config) only for 'found',
and set a new error state (e.g., setSsoLookupError or reuse an existing state)
for 'error' with the returned message so the UI can display "Unable to check SSO
configuration" distinct from "SSO not configured". Ensure pendingUsernameRef
handling remains unchanged and include logging via logger.info/error that
records the status and message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e345197d-bbff-4947-b57a-2c7ee2f6cc97
📒 Files selected for processing (4)
src/app/login/index.tsxsrc/app/login/sso.tsxsrc/hooks/__tests__/use-oidc-login.test.tssrc/hooks/use-oidc-login.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/use-oidc-login.ts
- src/hooks/tests/use-oidc-login.test.ts
Comment on lines
+26
to
+29
| const ssoFormSchema = z.object({ | ||
| username: z.string({ required_error: 'Username is required' }).min(3, 'Username must be at least 3 characters'), | ||
| departmentId: z.string().optional(), | ||
| }); |
There was a problem hiding this comment.
Validation error messages should use translation keys.
The hardcoded English strings violate the i18n requirement to wrap all user-facing text in t(). While zod schemas don't directly support async translation lookup, the error messages can reference translation keys that are resolved at render time.
🌐 Proposed fix using translation keys
-const ssoFormSchema = z.object({
- username: z.string({ required_error: 'Username is required' }).min(3, 'Username must be at least 3 characters'),
- departmentId: z.string().optional(),
-});
+const ssoFormSchema = z.object({
+ username: z.string({ required_error: 'validation.username_required' }).min(3, 'validation.username_min_length'),
+ departmentId: z.string().optional(),
+});Then in the error display (line 215), resolve the key:
-<FormControlErrorText className="text-red-500">{errors?.username?.message}</FormControlErrorText>
+<FormControlErrorText className="text-red-500">{t(errors?.username?.message ?? '')}</FormControlErrorText>As per coding guidelines: "Wrap all user-facing text in t() from react-i18next".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/login/sso.tsx` around lines 26 - 29, The zod schema ssoFormSchema
currently uses hardcoded English messages for username (required and min) —
replace those strings with translation keys (e.g. 'errors.username.required',
'errors.username.minLength') for both the username and any other messages
(departmentId remains optional), then in the form error rendering logic that
displays zod errors (the component that reads errors from ssoFormSchema) call
t(error.message as translationKey) to resolve the key at render time; ensure you
update both the .min and required_error usages on ssoFormSchema.username and map
any displayed error messages through react-i18next's t() before showing to the
user.
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
Documentation
Refactor