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 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 3cce531 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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughClient API and tests updated so that HTTP error responses containing an error payload with a Step object including a numeric Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / UI"
participant Client as "JourneyClient"
participant Fetch as "fetch (network)"
participant Server as "Auth Server"
participant Renderer as "Renderer"
User->>Client: start()/next(payload)
Client->>Fetch: HTTP request
Fetch->>Server: forward request
Server-->>Fetch: 4xx response with error body { error: { data: Step{ code } } }
Fetch-->>Client: error-shaped response
alt error.data.code ∈ LOGIN_FAILURE_CODES
Client->>Client: extract error.data as Step\nreturn JourneyLoginFailure
Client->>Renderer: invoke renderError()
Renderer-->>User: display login failure
else otherwise
Client->>Client: return GenericError
Client->>Renderer: render generic error
Renderer-->>User: display generic error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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 3cce531
☁️ 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❌ Patch coverage is
❌ Your project status has failed because the head coverage (15.72%) 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% 15.72% -55.19%
===========================================
Files 53 154 +101
Lines 2021 26677 +24656
Branches 377 1132 +755
===========================================
+ Hits 1433 4194 +2761
- Misses 588 22483 +21895
🚀 New features to boost your workflow:
|
|
Deployed 6ae11ee to https://ForgeRock.github.io/ping-javascript-sdk/pr-574/6ae11ee617b4e7edd176b4141f005819912238b0 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-9.9 KB, -100.0%) 📊 Minor Changes📉 @forgerock/device-client - 9.9 KB (-0.0 KB) ➖ No Changes➖ @forgerock/oidc-client - 25.2 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 (data) { | ||
| return createJourneyObject(data); | ||
| } | ||
| return createJourneyObject(data); | ||
|
|
||
| const genericError: GenericError = { | ||
| error: 'no_response_data', | ||
| message: 'No data received from server when starting journey', | ||
| type: 'unknown_error', | ||
| }; | ||
| return genericError; |
There was a problem hiding this comment.
Is this just a stylistic change, or was there a functional reason for it? I ask because we have the pattern of error conditions first, then operate with success condition last. This would break that pattern.
| if (data) { | ||
| return createJourneyObject(data); | ||
| } | ||
| return createJourneyObject(data); | ||
|
|
||
| const genericError: GenericError = { | ||
| error: 'no_response_data', | ||
| message: 'No data received from server when submitting step', | ||
| type: 'unknown_error', | ||
| }; | ||
| return genericError; |
There was a problem hiding this comment.
Same here. Doing success last, rather than first, helps narrow types and sometimes prevents complex conditionals and nesting.
| /** | ||
| * 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.
4 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
UI Updates