fix(journey-client): add query param handling back to resume function#583
fix(journey-client): add query param handling back to resume function#583vatsalparikh wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 9f4d2ba 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 52 minutes and 39 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 (4)
📝 WalkthroughWalkthroughThis pull request introduces a major version release for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 9f4d2ba
☁️ 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/journey-client/src/lib/client.store.test.ts (1)
252-296: Good coverage — consider also asserting theauthIndexValue→journeyfallback.The two new tests cover legacy parameter forwarding and the URL-vs-
options.queryprecedence rules well. However, the changeset specifically calls out thatauthIndexValueis now used as a fallback journey value, and there's no test exercising that path (i.e., aresume(url)whereurlcontainsauthIndexValueandoptions.journeyis omitted, asserting that the start/next request usesauthIndexValueas the journey, and thatoptions.journeyoverrides it when both are supplied).Adding a test or two here would lock down the
journeyfallback behavior introduced at lines 272–274 ofclient.store.tsand prevent silent regressions.🤖 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 252 - 296, Add tests to cover the authIndexValue → journey fallback: create a resume call where the resumeUrl contains authIndexValue (e.g., ...?authIndexValue=theJourney) and options.journey is omitted, then inspect the outgoing request (mockFetch.mock.calls[1][0]) and assert url.searchParams.get('journey') === 'theJourney'; also add a complementary case where both authIndexValue in the URL and options.journey are provided and assert that url.searchParams.get('journey') equals the options.journey value (verifying client.resume's fallback/override behavior around authIndexValue and the journey parameter).packages/journey-client/src/lib/client.store.ts (1)
255-275: Precedence order is correct, but consider documenting it on theresume()JSDoc.The construction order — URL-extracted params first, then
...options.querylast — correctly makes caller-suppliedoptions.querywin over redirect-URL values, and tests at lines 280–296 ofclient.store.test.tslock that in. Same foroptions.journey ?? authIndexValue.Two small notes (non-blocking):
- URL params with empty-string values are silently dropped due to the
value && { value }truthy guard, while values fromoptions.queryare not filtered. This asymmetry is fine for OAuth-spec-compliant flows, but worth being aware of if a server ever sends?error=(empty).resume()lacks a JSDoc block describing parameter parsing and theoptions> URL >authIndexValueprecedence rules — given this is a behavioral restore that's now part of the public contract (per the changeset/major bump), an inline doc comment would prevent future regressions.📝 Optional doc-comment suggestion
+ /** + * Resumes a journey after an external redirect. + * + * Parses the following query parameters from `url` and forwards them via `resumeOptions.query`: + * `code`, `state`, `form_post_entry`, `responsekey`, `error`, `errorCode`, `errorMessage`, + * `nonce`, `RelayState`, `scope`, `suspendedId`. The `authIndexValue` parameter is used as a + * fallback for `journey` when `options.journey` is not provided. + * + * Precedence: values supplied in `options.query` / `options.journey` override values parsed + * from the URL. + */ resume: async (url: string, options?: ResumeOptions): Promise<JourneyResult> => {🤖 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 255 - 275, Add a JSDoc block to the resume() function describing how parameters are parsed and the precedence rules: list that URL-extracted params (e.g., code, error, errorCode, errorMessage, form_post_entry, nonce, RelayState, responsekey, scope, state, suspendedId) are merged into resumeOptions first, then options.query is spread last so caller-supplied options.query wins, and journey is set via options.journey ?? authIndexValue; also note the asymmetry that URL params with empty-string values are currently dropped by the truthy guards (e.g., ...(code && { code })) while options.query values are not filtered, and document this behavior and its implications for OAuth flows to prevent future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface_mapping.md`:
- Line 329: The migration note is stale: it incorrectly tells users to manually
extract suspendedId, RelayState, and authIndexValue into options.query even
though the table above and the resume() implementation now auto-parse and
forward those parameters; update interface_mapping.md to either remove the note
or rewrite it to state that suspendedId, RelayState, and authIndexValue are now
auto-parsed and will be forwarded by resume(), and warn only if consumers
intentionally want to override those SDK-extracted values by supplying
options.query.
---
Nitpick comments:
In `@packages/journey-client/src/lib/client.store.test.ts`:
- Around line 252-296: Add tests to cover the authIndexValue → journey fallback:
create a resume call where the resumeUrl contains authIndexValue (e.g.,
...?authIndexValue=theJourney) and options.journey is omitted, then inspect the
outgoing request (mockFetch.mock.calls[1][0]) and assert
url.searchParams.get('journey') === 'theJourney'; also add a complementary case
where both authIndexValue in the URL and options.journey are provided and assert
that url.searchParams.get('journey') equals the options.journey value (verifying
client.resume's fallback/override behavior around authIndexValue and the journey
parameter).
In `@packages/journey-client/src/lib/client.store.ts`:
- Around line 255-275: Add a JSDoc block to the resume() function describing how
parameters are parsed and the precedence rules: list that URL-extracted params
(e.g., code, error, errorCode, errorMessage, form_post_entry, nonce, RelayState,
responsekey, scope, state, suspendedId) are merged into resumeOptions first,
then options.query is spread last so caller-supplied options.query wins, and
journey is set via options.journey ?? authIndexValue; also note the asymmetry
that URL params with empty-string values are currently dropped by the truthy
guards (e.g., ...(code && { code })) while options.query values are not
filtered, and document this behavior and its implications for OAuth flows to
prevent future regressions.
🪄 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: 5c8ad935-7f40-4494-89d3-26828f676f75
📒 Files selected for processing (4)
.changeset/good-numbers-act.mdinterface_mapping.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.ts
| | `nonce`, `scope` | Extracted, passed as query params | Same | | ||
| | `authIndexValue` | Extracted, used as fallback journey name | Same | | ||
|
|
||
| > **Migration note:** If your app relies on `suspendedId`, `RelayState`, or `authIndexValue` URL parameters being auto-parsed, you must extract them manually from the URL and pass them via `options.query` in the new SDK. |
There was a problem hiding this comment.
Stale migration note contradicts the restored auto-parsing behavior.
The note tells consumers they must manually extract suspendedId, RelayState, and authIndexValue and pass them via options.query, but the table directly above (and the resume() implementation in this PR) now auto-parses and forwards exactly those parameters. Either drop this note or invert its meaning so readers aren't told to do redundant work that may even override the SDK-extracted values.
📝 Suggested fix
-> **Migration note:** If your app relies on `suspendedId`, `RelayState`, or `authIndexValue` URL parameters being auto-parsed, you must extract them manually from the URL and pass them via `options.query` in the new SDK.
+> **Migration note:** `suspendedId`, `RelayState`, and `authIndexValue` (used as a journey fallback) are auto-parsed and forwarded — no manual extraction is required. If you need to override any of these, pass them explicitly via `options.query` (or `options.journey`), which take precedence over values parsed from the URL.📝 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.
| > **Migration note:** If your app relies on `suspendedId`, `RelayState`, or `authIndexValue` URL parameters being auto-parsed, you must extract them manually from the URL and pass them via `options.query` in the new SDK. | |
| > **Migration note:** `suspendedId`, `RelayState`, and `authIndexValue` (used as a journey fallback) are auto-parsed and forwarded — no manual extraction is required. If you need to override any of these, pass them explicitly via `options.query` (or `options.journey`), which take precedence over values parsed from the URL. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface_mapping.md` at line 329, The migration note is stale: it
incorrectly tells users to manually extract suspendedId, RelayState, and
authIndexValue into options.query even though the table above and the resume()
implementation now auto-parse and forward those parameters; update
interface_mapping.md to either remove the note or rewrite it to state that
suspendedId, RelayState, and authIndexValue are now auto-parsed and will be
forwarded by resume(), and warn only if consumers intentionally want to override
those SDK-extracted values by supplying options.query.
vatsalparikh
force-pushed
the
sdks-4796-resume-query-params
branch
from
49a3524 to
9f4d2ba
Compare
@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 #583 +/- ##
===========================================
- Coverage 70.90% 15.72% -55.18%
===========================================
Files 53 154 +101
Lines 2021 26682 +24661
Branches 377 1136 +759
===========================================
+ Hits 1433 4196 +2763
- Misses 588 22486 +21898
🚀 New features to boost your workflow:
|
|
Deployed 98a1cf5 to https://ForgeRock.github.io/ping-javascript-sdk/pr-583/98a1cf5b60e2028fb109bdc7048a87763cf0e099 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/davinci-client - 48.0 KB 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
https://pingidentity.atlassian.net/browse/SDKS-4796
Description
resumefunction in journey client is now handling query params again. We were already handling this in forgerock legacy sdk and after discussion on slack, decided to add back these query params to resume function.Did you add a changeset?
Yes
Summary by CodeRabbit
New Features
resume()method now automatically parses and forwards additional redirect parameters (error details, security tokens, and session identifiers) for improved OAuth/SSO integration. Added support for fallback journey selection via URL parameters.Documentation