fix(journey-client): create JourneyLoginFailure step, handle Login Failure case#574
fix(journey-client): create JourneyLoginFailure step, handle Login Failure case#574vatsalparikh wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: dcf367f The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes mapping of RTK Query results into a single JourneyResult via ChangesCore Journey Result Mapping
Tests and Mocks
E2E UI
Sequence Diagram(s)sequenceDiagram
participant Client as Client (caller)
participant JC as JourneyClient.start/next
participant Store as Store.dispatch
participant Backend as Remote API
Client->>JC: call start/next(params)
JC->>Store: dispatch request action
Store->>Backend: network request (/authenticate)
Backend-->>Store: Response (data OR error with error.data)
Store-->>JC: { data, error }
JC->>JC: resolveJourneyResult(data, error) -> createJourneyObject if step-shaped
JC-->>Client: JourneyResult (Step | LoginSuccess | LoginFailure | GenericError)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
View your CI Pipeline Execution ↗ for commit dcf367f
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/journey-client/src/lib/client.store.test.ts (1)
265-277: Minor:globalThis.windowis assigned but never cleaned up, and the initialdefinePropertyis redundant.Two small points on the new shim:
(globalThis as unknown as { window?: unknown }).window = {}leaks across tests within this file (no matching cleanup inafterEach). It happens to be benign today because this is the only test usingwindow, but a future test run in any other order could observe a straywindowglobal. Consider restoring it alongsidelocationSpy.mockRestore().- The
Object.defineProperty(window, 'location', { get: () => ({ assign: vi.fn() }) })on lines 268–273 is immediately overridden byvi.spyOn(window, 'location', 'get').mockReturnValue(...)on line 274 — the innerassign: vi.fn()is never invoked, and the spread...window.locationon line 275 just reads the spied getter. Keeping only thedefineProperty(so the property exists as a getter forspyOnto replace) with a minimal value, or dropping one of the two, would make the intent clearer.♻️ Suggested cleanup
- // Node test environment doesn't provide `window`, so create a minimal shim - // with a real `location` getter so we can keep using vi.spyOn(..., 'get'). - (globalThis as unknown as { window?: unknown }).window = {}; - Object.defineProperty(window, 'location', { - configurable: true, - get: () => ({ - assign: vi.fn(), - }), - }); - const locationSpy = vi.spyOn(window, 'location', 'get').mockReturnValue({ - ...window.location, - assign: assignMock, - }); + // Node test environment doesn't provide `window`, so create a minimal shim + // with a real `location` getter so we can keep using vi.spyOn(..., 'get'). + (globalThis as unknown as { window: unknown }).window = {}; + Object.defineProperty(window, 'location', { + configurable: true, + get: () => ({ assign: assignMock }), + }); + const locationSpy = vi.spyOn(window, 'location', 'get'); ... locationSpy.mockRestore(); + delete (globalThis as unknown as { window?: unknown }).window;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/client.store.test.ts` around lines 265 - 277, The test creates a global window shim and defines a getter for window.location but never cleans up; modify the test to (1) make the minimal defineProperty for window.location (so vi.spyOn can replace the getter) and remove the redundant inner assign fn, and (2) restore the original state in afterEach by calling locationSpy.mockRestore() and deleting or restoring globalThis.window (or saving and reassigning the original window) so the global isn't leaked; reference the locationSpy, assignMock and the temporary globalThis.window assignment when updating the test.packages/journey-client/src/lib/client.store.ts (1)
159-200: Consider extracting the error-mapping logic into a shared helper.The
startandnextmethods now contain near-identical blocks (lines 159–175 and 182–200) that unwrap{ data, error }, maperror.datawith a definedcodetocreateJourneyObject, and fall back to aGenericErrorwith only themessagestring differing. Extracting a small helper (e.g.,mapJourneyResult(result, fallbackMessage)) would remove the duplication and keep the two methods symmetric going forward.Also, the
errorData as Step | undefinedcast is unchecked — if the server ever returns a non-object body (e.g., a string or HTML),errorStep?.codewould still pass the optional-chain but could be against a non-Stepshape. A narrowtypeof errorData === 'object' && errorData !== nullguard before the cast would be safer.♻️ Proposed refactor
+ const mapResult = ( + { data, error }: { data?: Step; error?: unknown }, + fallbackMessage: string, + ): JourneyResult => { + if (data) { + return createJourneyObject(data); + } + const errorData = (error as FetchBaseQueryError | undefined)?.data; + if (typeof errorData === 'object' && errorData !== null) { + const errorStep = errorData as Step; + if (errorStep.code !== undefined) { + return createJourneyObject(errorStep); + } + } + return { + error: 'no_response_data', + message: fallbackMessage, + type: 'unknown_error', + }; + };Then
start/nextreduce to a singlereturn mapResult(await store.dispatch(...), '...')call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/client.store.ts` around lines 159 - 200, Extract the duplicated error-unwrapping logic used in start and next into a shared helper (e.g., mapJourneyResult or mapJourneyResponse) that accepts the dispatch result and a fallback message; the helper should check for data and return createJourneyObject(data) when present, then safely inspect error data by first guarding typeof errorData === 'object' && errorData !== null before treating it as a Step and returning createJourneyObject(errorStep) if errorStep.code is defined, and finally return a GenericError object with the provided fallbackMessage if neither path yields a Journey object—then replace the duplicated blocks in start and next with a single call to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/journey-client/src/lib/client.store.test.ts`:
- Around line 265-277: The test creates a global window shim and defines a
getter for window.location but never cleans up; modify the test to (1) make the
minimal defineProperty for window.location (so vi.spyOn can replace the getter)
and remove the redundant inner assign fn, and (2) restore the original state in
afterEach by calling locationSpy.mockRestore() and deleting or restoring
globalThis.window (or saving and reassigning the original window) so the global
isn't leaked; reference the locationSpy, assignMock and the temporary
globalThis.window assignment when updating the test.
In `@packages/journey-client/src/lib/client.store.ts`:
- Around line 159-200: Extract the duplicated error-unwrapping logic used in
start and next into a shared helper (e.g., mapJourneyResult or
mapJourneyResponse) that accepts the dispatch result and a fallback message; the
helper should check for data and return createJourneyObject(data) when present,
then safely inspect error data by first guarding typeof errorData === 'object'
&& errorData !== null before treating it as a Step and returning
createJourneyObject(errorStep) if errorStep.code is defined, and finally return
a GenericError object with the provided fallbackMessage if neither path yields a
Journey object—then replace the duplicated blocks in start and next with a
single call to this helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3033ff09-5ae0-426f-9ba5-70f1baf0c83b
📒 Files selected for processing (4)
.changeset/journey-loginfailure-on-error-code.mde2e/journey-app/main.tspackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.ts
💤 Files with no reviewable changes (1)
- e2e/journey-app/main.ts
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (17.83%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #574 +/- ##
===========================================
- Coverage 70.90% 17.83% -53.08%
===========================================
Files 53 150 +97
Lines 2021 24255 +22234
Branches 377 1163 +786
===========================================
+ Hits 1433 4325 +2892
- Misses 588 19930 +19342
🚀 New features to boost your workflow:
|
|
Deployed 520f9f3 to https://ForgeRock.github.io/ping-javascript-sdk/pr-574/520f9f3c3ff7c3004f1f42cfc0397d889328bb29 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 92.4 KB (+0.5 KB) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/journey-client/src/lib/journey.api.ts (1)
139-202: Extract the duplicated login-failure mapping into a helper.The blocks at lines 139-155 and 186-202 are identical. Consider extracting into a small helper to reduce drift risk when the set of failure codes or the shape handling evolves.
♻️ Proposed refactor
+function mapLoginFailureResponse( + response: QueryReturnValue<unknown, FetchBaseQueryError, FetchBaseQueryMeta>, +): QueryReturnValue<Step, FetchBaseQueryError, FetchBaseQueryMeta> { + if ('error' in response) { + const errorData = (response.error as FetchBaseQueryError | undefined)?.data as + | Step + | undefined; + if (errorData?.code && LOGIN_FAILURE_CODES.includes(errorData.code)) { + return { data: errorData }; + } + } + return response as QueryReturnValue<Step, FetchBaseQueryError, FetchBaseQueryMeta>; +}Then use
return mapLoginFailureResponse(response);in bothstartandnext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/journey.api.ts` around lines 139 - 202, Extract the duplicated login-failure mapping into a small helper (e.g. mapLoginFailureResponse) that accepts the endpoint response and returns a QueryReturnValue<Step, FetchBaseQueryError, FetchBaseQueryMeta> when the response body is a Step with a code in LOGIN_FAILURE_CODES, or undefined/falsey otherwise; move the logic that inspects ('error' in response), casts to (response.error as FetchBaseQueryError)?.data as Step, checks data.code and LOGIN_FAILURE_CODES into this helper, then replace the duplicated blocks in both the start and next builder handlers with a single call like: return mapLoginFailureResponse(response) ?? (continue with existing return).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/journey-client/src/lib/journey.api.ts`:
- Around line 139-202: Extract the duplicated login-failure mapping into a small
helper (e.g. mapLoginFailureResponse) that accepts the endpoint response and
returns a QueryReturnValue<Step, FetchBaseQueryError, FetchBaseQueryMeta> when
the response body is a Step with a code in LOGIN_FAILURE_CODES, or
undefined/falsey otherwise; move the logic that inspects ('error' in response),
casts to (response.error as FetchBaseQueryError)?.data as Step, checks data.code
and LOGIN_FAILURE_CODES into this helper, then replace the duplicated blocks in
both the start and next builder handlers with a single call like: return
mapLoginFailureResponse(response) ?? (continue with existing return).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d2a0334-1d00-4e63-85af-4145e953a6d4
📒 Files selected for processing (4)
.changeset/polite-horses-dig.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/journey.api.ts
✅ Files skipped from review due to trivial changes (2)
- packages/journey-client/src/lib/client.store.ts
- .changeset/polite-horses-dig.md
SteinGabriel
left a comment
SteinGabriel
left a comment
There was a problem hiding this comment.
Changes look great! Everything worked as expected when I tested it locally. Just need to update a stale copyright date.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Left some alternative thoughts on this solution.
| /** | ||
| * If the endpoint returned an HTTP error whose body is an AM Step with a | ||
| * login-failure code, treat it as successful data so callers receive the | ||
| * Step via the `data` path (keeps downstream logic simpler). | ||
| */ |
There was a problem hiding this comment.
I'm a bit worried about tightly coupling our understanding of "true errors" to validation or authentication errors by relying solely on the error code. The logic we use in the original SDK was much more "liberal". It essentially treated all errors as FRLoginFailure: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/fr-auth/index.ts#L76.
Rather than doing all of this logic here, we could grab error from this destructure: https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/journey-client/src/lib/client.store.ts#L174, and pass it to createJourneyObject here: https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/journey-client/src/lib/client.store.ts#L183. Finally, just handle the logic here: https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/journey-client/src/lib/journey.utils.ts#L28.
There was a problem hiding this comment.
Interesting, yeah, earlier logic in forgerock sdk was simply treating every error as FRLoginFailure so I tried to think about what constitutes as login failure and used 4xx codes.
Yeah, error codes can feel too restrictive but that's the only reliable data point about login failures. I think it just depends on what we want to classify as login failure. If we want to include 500 error from server as login failure as well, then yes, this is tightly coupled.
I looked at Ryan's closed PR and updated the logic of determining a login failure. If data contains any Step like properties then we treat it as login failure. Open to suggestions on whether you think this is the best way to determine a login failure.
Thanks for the suggestion on moving logic to journey utils. client.store file is now clean and all logic is handled by journey.utils file.
Thanks!
There was a problem hiding this comment.
I see Vatsal may have already implemented Justin's suggestion above but I've proposed another improvement below. We can handle the error either inside or outside of createJourneyObject, but I think it is important to also standardize the type narrowing of RTK errors throughout our SDK, checking for a Fetch error or Serialized error. After we know what type of error it is then we can return a LoginFailure or GenericError.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/journey-client/src/lib/journey.utils.ts (1)
49-50: Nit: consider hoistingresolveStepabovecreateJourneyObjectto drop the eslint-disable.Declaring
resolveStepbefore its only caller removes the need for// eslint-disable-next-line@typescript-eslint/no-use-before-define`` and reads linearly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/journey.utils.ts` around lines 49 - 50, Hoist the helper function resolveStep so it is declared before its only caller createJourneyObject; move the resolveStep function definition above createJourneyObject, remove the now-unnecessary // eslint-disable-next-line `@typescript-eslint/no-use-before-define` comment, and run lint to verify no use-before-define warning remains. Ensure resolveStep and createJourneyObject names are unchanged to preserve references.packages/journey-client/src/lib/client.store.test.ts (1)
388-399: Consider aligning the test name with the new assertion.The test is named
start_NoDataFromServer_ReturnsGenericErrorand previously validatederror === 'no_response_data', but now assertserror === 'request_failed'. SincesetupMockFetch(null)actually causes/authenticateto reject (i.e., the RTK Query error path withstatus: 'FETCH_ERROR'), this test is really covering the "fetch failed" case rather than the "server returned nothing" case. Renaming to something likestart_FetchRejects_ReturnsRequestFailedErrorwould better describe the scenario, and a separatestart_ServerReturnsEmptyBody_ReturnsNoResponseDatacould cover the truly empty-response case viacreateJourneyObject(undefined, undefined)(already covered injourney.utils.test.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/client.store.test.ts` around lines 388 - 399, Rename the test to reflect that the fetch call rejects rather than the server returning an empty body: update the test title from start_NoDataFromServer_ReturnsGenericError to something like start_FetchRejects_ReturnsRequestFailedError, leaving the test body unchanged (it uses setupMockFetch(null), invokes journey({ config: mockConfig }) and client.start(), and asserts via isGenericError(result) and result.error === 'request_failed'); this makes it clear the scenario exercised is the RTK Query fetch-error path rather than a no-response-data case.packages/journey-client/src/lib/journey.utils.test.ts (1)
15-93: Solid coverage for the newcreateJourneyObject(step, error)signature.Good branch coverage across (step with
authId), (step withsuccessUrl), (no step + no error →no_response_data), (no step + error with status →request_failed), (non-Step-likeerror.data→request_failed), and (Step-likeerror.data→LoginFailure).Consider adding one more case to lock in behavior for a 5xx (e.g.,
status: 500) that returns a Step-likeerror.data(i.e., a body containing one ofSTEP_LIKE_KEYSlikecode) — this currently maps toLoginFailureregardless of HTTP status. If that's the intended behavior, a test asserting it prevents future regressions; if the intent is login-specific (401/403) as discussed in earlier review rounds, the utility’s logic should be revisited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/journey.utils.test.ts` around lines 15 - 93, Add a test covering the case where error.status is 5xx but error.data is Step-like to lock in current behavior (or to indicate needed change); specifically update the createJourneyObject tests to include a case that calls createJourneyObject(undefined, { status: 500, data: { code: 500, message: 'Server Err' } }) and assert whether it returns a JourneyLoginFailure (check type === StepType.LoginFailure and payload equals the Step-like data, plus validate getters like getCode/getMessage) to document/lock the handling of STEP_LIKE_KEYS by createJourneyObject.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/journey-client/src/lib/journey.utils.ts`:
- Around line 20-34: The STEP_LIKE_KEYS heuristic is too permissive and causes
non-auth errors to be treated as a Step; tighten the check in resolveStep (and
the downstream createJourneyObject path) by requiring a more specific
login-shaped signature before casting error.data to Step — for example, require
either (a) an HTTP auth status (status or statusCode equal to 401 or 403) OR (b)
a login-specific field combination like authId or successUrl present, rather
than any single key from STEP_LIKE_KEYS; update STEP_LIKE_KEYS/use in
resolveStep to check these conditions and only cast to Step when they pass so
createJourneyObject only produces JourneyLoginFailure for true AM/login failure
shapes.
---
Nitpick comments:
In `@packages/journey-client/src/lib/client.store.test.ts`:
- Around line 388-399: Rename the test to reflect that the fetch call rejects
rather than the server returning an empty body: update the test title from
start_NoDataFromServer_ReturnsGenericError to something like
start_FetchRejects_ReturnsRequestFailedError, leaving the test body unchanged
(it uses setupMockFetch(null), invokes journey({ config: mockConfig }) and
client.start(), and asserts via isGenericError(result) and result.error ===
'request_failed'); this makes it clear the scenario exercised is the RTK Query
fetch-error path rather than a no-response-data case.
In `@packages/journey-client/src/lib/journey.utils.test.ts`:
- Around line 15-93: Add a test covering the case where error.status is 5xx but
error.data is Step-like to lock in current behavior (or to indicate needed
change); specifically update the createJourneyObject tests to include a case
that calls createJourneyObject(undefined, { status: 500, data: { code: 500,
message: 'Server Err' } }) and assert whether it returns a JourneyLoginFailure
(check type === StepType.LoginFailure and payload equals the Step-like data,
plus validate getters like getCode/getMessage) to document/lock the handling of
STEP_LIKE_KEYS by createJourneyObject.
In `@packages/journey-client/src/lib/journey.utils.ts`:
- Around line 49-50: Hoist the helper function resolveStep so it is declared
before its only caller createJourneyObject; move the resolveStep function
definition above createJourneyObject, remove the now-unnecessary //
eslint-disable-next-line `@typescript-eslint/no-use-before-define` comment, and
run lint to verify no use-before-define warning remains. Ensure resolveStep and
createJourneyObject names are unchanged to preserve references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23eb68ce-90c9-4575-adf7-55f70f46f8d7
📒 Files selected for processing (7)
.changeset/whole-mangos-find.mde2e/journey-app/main.tspackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/journey.api.tspackages/journey-client/src/lib/journey.utils.test.tspackages/journey-client/src/lib/journey.utils.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/whole-mangos-find.md
- packages/journey-client/src/lib/journey.api.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/journey-client/src/lib/journey.utils.ts (1)
20-34:⚠️ Potential issue | 🟠 MajorTighten the Step-like error heuristic before routing to
LoginFailure.
STEP_LIKE_KEYSstill includes very common error fields likecode,status,detail, andok, so unrelated API failures can be misclassified asJourneyLoginFailure. That makes the new error-path handling broader than the login-specific behavior this PR is trying to add.🔧 Suggested tightening
-const STEP_LIKE_KEYS = [ - 'authId', - 'callbacks', - 'code', - 'description', - 'detail', - 'header', - 'ok', - 'realm', - 'reason', - 'stage', - 'status', - 'successUrl', - 'tokenId', -] as const; +const LOGIN_FAILURE_STATUSES = new Set([401, 403]); +const STEP_LIKE_KEYS = ['authId', 'callbacks', 'successUrl', 'tokenId'] as const; @@ - if (data && typeof data === 'object' && STEP_LIKE_KEYS.some((key) => key in data)) { + if ( + data && + typeof data === 'object' && + ((typeof errorObj?.status === 'number' && LOGIN_FAILURE_STATUSES.has(errorObj.status)) || + STEP_LIKE_KEYS.some((key) => key in data)) + ) {Also applies to: 80-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/journey.utils.ts` around lines 20 - 34, The STEP_LIKE_KEYS array is too permissive and causes unrelated API errors to be treated as login failures; restrict the heuristic by removing generic fields ('code', 'status', 'detail', 'ok', 'description', 'reason') and only keep login-specific identifiers (e.g., 'authId', 'callbacks', 'header', 'realm', 'tokenId', 'successUrl', 'stage') so only objects with explicit login-related shape route to JourneyLoginFailure; update the STEP_LIKE_KEYS constant and any other usage sites (the other occurrence referenced around the same file) to use the tightened list so the LoginFailure path is only triggered for genuine login-shaped payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/journey-client/src/lib/journey.utils.ts`:
- Around line 20-34: The STEP_LIKE_KEYS array is too permissive and causes
unrelated API errors to be treated as login failures; restrict the heuristic by
removing generic fields ('code', 'status', 'detail', 'ok', 'description',
'reason') and only keep login-specific identifiers (e.g., 'authId', 'callbacks',
'header', 'realm', 'tokenId', 'successUrl', 'stage') so only objects with
explicit login-related shape route to JourneyLoginFailure; update the
STEP_LIKE_KEYS constant and any other usage sites (the other occurrence
referenced around the same file) to use the tightened list so the LoginFailure
path is only triggered for genuine login-shaped payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce5ba34-661e-4c44-9881-10433079f8b2
📒 Files selected for processing (6)
e2e/journey-app/main.tspackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/journey.api.tspackages/journey-client/src/lib/journey.utils.test.tspackages/journey-client/src/lib/journey.utils.ts
✅ Files skipped from review due to trivial changes (2)
- packages/journey-client/src/lib/journey.api.ts
- packages/journey-client/src/lib/journey.utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/journey-app/main.ts
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Left some comments about type imports and an alternative to how we could do the error handling.
vatsalparikh
force-pushed
the
sdks-4796
branch
3 times, most recently
from
14a5f3d to
8649b37
Compare
vatsalparikh
force-pushed
the
sdks-4796
branch
3 times, most recently
from
c00f12b to
f3f5efa
Compare
vatsalparikh
force-pushed
the
sdks-4796
branch
3 times, most recently
from
7cfc7c2 to
cca2340
Compare
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I'd like to really think about this. If we are going to increase the complexity of how we handle the Journey responses, I'd start moving towards the pattern we have in DaVinci Client, rather than inventing a new pattern here.
DaVinci uses the concept of handling the response at the RTK API layer, rather than the store layer.
| } | ||
| return createJourneyObject(data); | ||
| const { data, error } = await store.dispatch(journeyApi.endpoints.start.initiate(options)); | ||
| return resolveJourneyResult(data, error); |
There was a problem hiding this comment.
Rather than having this new function swallow up createJourneyObject, I'd rather flatten them out. If the main concern here is evaluating the error in a more nuanced manner, then let's do that, but without it absorbing the original function.
For example:
// pseudocode
const { data, error } = await store.dispatch(journeyApi.endpoints.start.initiate(options));
let resolvedError;
if (error) {
resolvedError = resolveJourneyError(error);
if (resolveError.catosrophic) {
return { /* GenericError */ };
}
}
return createJourneyObject(data || resolvedError):
There was a problem hiding this comment.
If we called createJourneyObject with createJourneyObject(data || resolvedError), then we're going back to the initial implementation where I was error handling in journey.api.ts file, which is
At the time, you mentioned that "Rather than doing all of this logic here, we could grab error from this destructure: main/packages/journey-client/src/lib/client.store.ts#L174, and pass it to createJourneyObject here: main/packages/journey-client/src/lib/client.store.ts#L183. Finally, just handle the logic here: main/packages/journey-client/src/lib/journey.utils.ts#L28."
And I feel like error handling with resolveJourneyResult handles all errors in one place. If we split like your suggestion, we'll handle some errors in journey.api.ts file, some errors in client.store.ts file and some errors in journey.utils.ts file.
Would be great to discuss further on this over a call to reach a consensus.
| function isStepLike(value: unknown): boolean { | ||
| return ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| [ | ||
| 'authId', | ||
| 'callbacks', | ||
| 'code', | ||
| 'description', | ||
| 'detail', | ||
| 'header', | ||
| 'ok', | ||
| 'realm', | ||
| 'reason', | ||
| 'stage', | ||
| 'status', | ||
| 'successUrl', | ||
| 'tokenId', | ||
| ].some((key) => key in value) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm not sure how I feel about this kind of "meta-analysis" on the object. In the legacy SDK, it was much, much more simple, and I'm not sure there's any additional value this provides to offset the cost of complexity.
In the legacy SDK, an object was essentially a Step if it had an authId, a LoginSuccess if it didn't have an authId, but the fetch was .ok, and an LoginFailure if it was neither. You can see that codified here: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/fr-auth/index.ts#L65
There was a problem hiding this comment.
We can treat every failure as login failure then without a concept of Generic Error. I think this goes back to my earlier comment of, what is too tight of a coupling? What can help us recognize that a step is login failure and not generic error?
I am open to returning every failure as login failure, but we have a pattern of generic error in journey client. We are able to simply fallback to login failure because we don't have a concept of generic error in legacy sdk.
We can just check whether error.data exists and is an object instead of scanning the properties, and then any non-object returns GenericError. If error.data is an object we always return login failure, otherwise return generic error, we can simplify it down to this.
There was a problem hiding this comment.
I've removed the isStepLike check and if error.data is an object it is treated as login failure step. I think we still need GenericError, but would be great to discuss this further.
5 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4796
Description
While working on the Journey Client migration, I noticed that when AM returns a failure response (e.g., unauthorized / invalid credentials), the Journey Client was returning a generic “no data” error and the
LoginFailurepath increateJourneyObject()was effectively never reached. This PR closes that gap by ensuringLoginFailureis triggered and aJourneyLoginFailureis returned when the server provides a failure payload with acode.What changed
start()/next()now map RTK Query error responses that include acodeintocreateJourneyObject(), which returnsJourneyLoginFailure(hitting the previously-unreachedLoginFailurebranch).@forgerock/journey-client.Edit
Ryan shared his closed PR that was trying to fix this issue: #541. Will use this as a reference.
How to test
Summary by CodeRabbit
Bug Fixes
Tests
Chores