Sdks 5097#670
Conversation
🦋 Changeset detectedLatest commit: 7bac6f3 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 Review limit reached
More reviews will be available in 48 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR refactors login-failure response handling in the journey client. It introduces a new response parsing layer using the Effect library's Either type, moves the ChangesLogin failure response handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 e5badbe
☁️ 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 02e0085 to https://ForgeRock.github.io/ping-javascript-sdk/pr-670/02e008591d5cd72c7879bd87a6066c17ffbd847c 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 1371 +168
==========================================
+ Hits 4410 5101 +691
+ Misses 19988 19724 -264
🚀 New features to boost your workflow:
|
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/protect - 144.6 KB (new) 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
2 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
JourneyLoginFailurewhen a failure payload with a login-failure code is received, fixing a previously unreached code path.