Sdks 5097#670
Conversation
🦋 Changeset detectedLatest commit: 6a2eea6 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughCentralizes RTK Query response parsing into parseJourneyResponse and exported JourneyResult, refactors client.start()/next() to use Either-based parsing, updates public re-exports and API reports, and adds unit and E2E tests plus a changeset documenting the login-failure routing fix. ChangesLogin failure response handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 348a636
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We moved the numeric-status FetchBaseQueryError branch before the !res.data guard in parseJourneyResponse, so AM failure payloads carried in error.data are now correctly routed to createJourneyObject as LoginFailure. Previously, data being undefined (the RTK Query behaviour for non-2xx responses) caused the function to short-circuit at no_response_data before the numeric-status branch was ever reached. This fix ensures the LoginFailure path introduced by the PR is reachable and all four related tests pass.
Tip
✅ We verified this fix by re-running @forgerock/journey-client:test.
Suggested Fix changes
diff --git a/packages/journey-client/src/lib/journey.utils.ts b/packages/journey-client/src/lib/journey.utils.ts
index 22c77d5..f0340e2 100644
--- a/packages/journey-client/src/lib/journey.utils.ts
+++ b/packages/journey-client/src/lib/journey.utils.ts
@@ -86,6 +86,21 @@ export function parseJourneyResponse(res: {
});
}
+ // https://redux-toolkit.js.org/rtk-query/api/fetchBaseQuery#signature
+ // FetchBaseQueryError with numeric status = AM failure step over HTTP error
+ if (res.error && 'status' in res.error && typeof res.error.status === 'number') {
+ if (typeof res.error.data === 'object' && res.error.data !== null) {
+ // Object body = valid AM failure payload, treat as a Step to classify
+ return right(res.error.data as Step);
+ }
+ // Non-object body = unexpected response format
+ return left({
+ error: 'request_failed',
+ message: 'Request failed with server error',
+ type: 'unknown_error',
+ });
+ }
+
if (!res.data) {
return left({
error: 'no_response_data',
@@ -94,17 +109,5 @@ export function parseJourneyResponse(res: {
});
}
- // https://redux-toolkit.js.org/rtk-query/api/fetchBaseQuery#signature
- // FetchBaseQueryError with numeric status + object body = AM failure step over HTTP error
- if (
- res.error &&
- 'status' in res.error &&
- typeof res.error.status === 'number' &&
- typeof res.error.data === 'object' &&
- res.error.data !== null
- ) {
- return right(res.error.data);
- }
-
return right(res.data);
}
Or Apply changes locally with:
npx nx-cloud apply-locally o4xt-x66H
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/journey-app/main.ts`:
- Around line 211-213: The call to journeyClient.start({ journey: journeyName })
can return non-Step results, so guard the result before calling renderForm();
check the returned value (the variable step) is a valid Step (e.g., truthy and
has the expected method/property your flow relies on) and only call renderForm()
when that check passes; if the result is not a valid Step, set errorEl.innerHTML
= errorHtml (or update the UI appropriately) and bail out/return to avoid
throwing during retry handling. Ensure you reference the journeyClient.start
call and the step variable when adding the conditional guard around
renderForm().
In `@packages/journey-client/src/lib/client.store.ts`:
- Around line 159-162: The parser parseJourneyResponse currently returns a
no_response_data branch before it inspects error.data, which prevents
createJourneyObject from receiving error payloads (e.g., LoginFailure); update
parseJourneyResponse in journey.utils.ts to check and parse error.data
(extracting the payload/error body) before falling back to the no_response_data
case so the onRight path can yield the proper error-based JourneyResult; apply
the same reorder/fix to the other similar branch referenced in the review so
both parsing paths prioritize error.data parsing ahead of the no-data return.
In `@packages/journey-client/src/lib/journey.utils.ts`:
- Around line 89-107: The HTTP-error branch that checks res.error with a numeric
status and object body (the block that returns right(res.error.data)) must be
moved above the no-data guard that currently returns left({ error:
'no_response_data', ... }); update the logic in journey.utils.ts so the
numeric-status/object-data check (the res.error && 'status' in res.error &&
typeof res.error.status === 'number' && typeof res.error.data === 'object' &&
res.error.data !== null) runs before the if (!res.data) guard, ensuring HTTP
error bodies are returned via right(...) instead of being swallowed by the
no_response_data branch.
🪄 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: 45f63ebb-1565-4bb0-a212-035f2eca3bb3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/whole-mangos-find.mde2e/journey-app/main.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/package.jsonpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/journey.utils.test.tspackages/journey-client/src/lib/journey.utils.tspackages/journey-client/src/types.ts
| step = await journeyClient.start({ journey: journeyName }); | ||
| renderForm(); | ||
| errorEl.innerHTML = errorHtml; |
There was a problem hiding this comment.
Guard the restart result before calling renderForm().
journeyClient.start() can return non-Step results. Calling renderForm() unconditionally can throw and break the submit flow on retry failures.
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 212-212: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorEl.innerHTML = errorHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 212-212: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: errorEl.innerHTML = errorHtml
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/journey-app/main.ts` around lines 211 - 213, The call to
journeyClient.start({ journey: journeyName }) can return non-Step results, so
guard the result before calling renderForm(); check the returned value (the
variable step) is a valid Step (e.g., truthy and has the expected
method/property your flow relies on) and only call renderForm() when that check
passes; if the result is not a valid Step, set errorEl.innerHTML = errorHtml (or
update the UI appropriately) and bail out/return to avoid throwing during retry
handling. Ensure you reference the journeyClient.start call and the step
variable when adding the conditional guard around renderForm().
@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: |
|
Deployed 8a67e47 to https://ForgeRock.github.io/ping-javascript-sdk/pr-670/8a67e47957304f803c9559c82f54b57a7a5457bc branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (20.54%) 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 #670 +/- ##
==========================================
+ Coverage 18.07% 20.54% +2.47%
==========================================
Files 155 154 -1
Lines 24398 24825 +427
Branches 1203 1372 +169
==========================================
+ Hits 4410 5101 +691
+ Misses 19988 19724 -264
🚀 New features to boost your workflow:
|
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/device-client - 10.0 KB (new) ➖ No Changes➖ @forgerock/protect - 144.6 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
Overall I approve, however in line with what Gabriel said I do think LoginFailure should be a Left in the Either data structure.
However - let's not merge this immediately. I think this PR warrants a team conversation as it's introducing a new way of doing something, and having feedback, and also Vatsal, your opinion and thoughts after stepping away from the code for a little bit would be valuable.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
i think this looks good. There are currently two comments that I think are relevant, but if they are resolved, then this is ready for me.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/journey-suites/src/login.test.ts (1)
29-30: ⚡ Quick winAvoid asserting exact console copy for failure detection.
At Line 29 and Line 49, matching
"Journey failed"ties these tests to log wording rather than behavior. Givene2e/journey-app/main.tslogsError: ..., this can become brittle. Prefer asserting the rendered error content (#errorMessage) and keep console checks generic (or remove them).Suggested update
- expect(errorMessages.some((msg) => msg.includes('Journey failed'))).toBe(true); + await expect(page.locator('`#errorMessage`')).toContainText(/.+/); - expect(errorMessages.some((msg) => msg.includes('Journey failed'))).toBe(true); + await expect(page.locator('`#errorMessage`')).toContainText(/.+/);Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/journey-suites/src/login.test.ts` around lines 29 - 30, The test is asserting exact console text ("Journey failed") via expect(errorMessages.some((msg) => msg.includes('Journey failed'))).toBe(true); which is brittle; update the test to remove or loosen that console assertion and instead assert the rendered error element (`#errorMessage`) contains the expected message (e.g., get the element via document.querySelector('`#errorMessage`') and expect its textContent to include the error substring), and if you still need a console check make it generic (e.g., expect(errorMessages.length).toBeGreaterThan(0)) or remove it; apply the same change for the duplicate assertion at the other occurrence (the second expect that checks for "Journey failed").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/journey-suites/src/login.test.ts`:
- Around line 29-30: The test is asserting exact console text ("Journey failed")
via expect(errorMessages.some((msg) => msg.includes('Journey
failed'))).toBe(true); which is brittle; update the test to remove or loosen
that console assertion and instead assert the rendered error element
(`#errorMessage`) contains the expected message (e.g., get the element via
document.querySelector('`#errorMessage`') and expect its textContent to include
the error substring), and if you still need a console check make it generic
(e.g., expect(errorMessages.length).toBeGreaterThan(0)) or remove it; apply the
same change for the duplicate assertion at the other occurrence (the second
expect that checks for "Journey failed").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 375718d9-d747-4ab0-b60b-5ab0069262ff
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
e2e/journey-suites/src/login.test.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/package.jsonpackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/journey.utils.test.tspackages/journey-client/src/lib/journey.utils.tspackages/journey-client/src/types.ts
✅ Files skipped from review due to trivial changes (2)
- packages/journey-client/api-report/journey-client.types.api.md
- packages/journey-client/api-report/journey-client.api.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/journey-client/src/lib/client.store.ts
- packages/journey-client/package.json
- packages/journey-client/src/lib/journey.utils.test.ts
- packages/journey-client/src/lib/journey.utils.ts
- packages/journey-client/src/types.ts
One thing I noticed while working on oidc client is we should be using Micro for start and next in journey-client to be consistent with the rest of the packages. About thinking again how the code is structured I just feel that it's been reviewed quite thoroughly and I can't seem to think of a way of making this better without doing more refactor. If we refactor more, we can start to get better results. As far as this PR is concerned, I feel good about the structure right now. I don't think this is the correct PR for the larger refactor, that's all. I also moved the Either right to the very bottom, so lefts are all handled first. Just made that change today. |
5 participants
JIRA Ticket
pingidentity.atlassian.net/browse/SDKS-5097
Note
This is just a continuation of the #574 PR.
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.Ryan shared his closed PR that was trying to fix this issue: #541. Will use this as a reference.
Edit
Added an Either effect type to handle errors in journey client
What changed
start()/next()now map RTK Query error responses that include acodeintocreateJourneyObject(), which returnsJourneyLoginFailure(hitting the previously-unreachedLoginFailurebranch).@forgerock/journey-client.How to test
Summary by CodeRabbit