fix(journey-client): align config with DaVinci and OIDC clients#557
fix(journey-client): align config with DaVinci and OIDC clients#557
Conversation
🦋 Changeset detectedLatest commit: 5fe2f41 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 selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughJourneyClientConfig now extends AsyncLegacyConfigOptions and JourneyServerConfig accepts an optional timeout. journey() logs a single warning when legacy client-side fields (e.g., clientId, scope, redirectUri, or serverConfig.timeout) are provided but ignored. Tests and a changeset were added. Changes
Sequence Diagram(s)(omitted — change is a localized runtime warning addition; no multi-component flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 5fe2f41
☁️ 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
🤖 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/client.store.ts`:
- Around line 89-110: The code currently warns only for top-level ignored keys
via ignoredProperties/providedIgnored, so serverConfig.timeout is silently
ignored; update the check to detect nested serverConfig.timeout (e.g., inspect
config.serverConfig?.timeout) and include it in the warning (or add 'timeout' to
ignoredProperties when appropriate) so that the log.warn call reports when
timeout is provided but not used by journey-client; reference the existing
identifiers ignoredProperties, providedIgnored, config and the
wellknown/serverConfig usage to locate where to add the nested check and include
the key in the joined warning string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67c4f0be-178a-450f-92f1-0c7cf246840d
📒 Files selected for processing (5)
.changeset/journey-client-config-alignment.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/config.types.test-d.tspackages/journey-client/src/lib/config.types.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: |
|
Deployed 2a3b3f6 to https://ForgeRock.github.io/ping-javascript-sdk/pr-557/2a3b3f6dc2efc6fba3484fee2e7603895ad51b1e branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-9.7 KB, -100.0%) 📊 Minor Changes📉 @forgerock/device-client - 9.7 KB (-0.0 KB) ➖ No Changes➖ @forgerock/sdk-utilities - 11.2 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (14.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 #557 +/- ##
===========================================
- Coverage 70.90% 14.83% -56.07%
===========================================
Files 53 153 +100
Lines 2021 26285 +24264
Branches 377 1059 +682
===========================================
+ Hits 1433 3900 +2467
- Misses 588 22385 +21797
🚀 New features to boost your workflow:
|
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Looks good. Left a couple questions.
| it('should reject config without wellknown', () => { | ||
| // @ts-expect-error - wellknown is required on serverConfig | ||
| const config: JourneyClientConfig = { serverConfig: {} }; | ||
| expectTypeOf(config).toMatchTypeOf<JourneyClientConfig>(); |
There was a problem hiding this comment.
Should the assertion be not to match the type?
There was a problem hiding this comment.
yeah i can do this to remove the expect error line
There was a problem hiding this comment.
Fixed in the latest commit (9ce0aff) — changed to .not.toMatchObjectType<Required<JourneyClientConfig>>(). The @ts-expect-error on the line above already proves the type error exists; the assertion now aligns with the test's intent.
ryanbas21
force-pushed
the
fix-jc-missing-properties
branch
from
d78e883 to
9ce0aff
Compare
![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 inline the .filter() callback onto a single line to satisfy the Prettier formatting rule that flagged the multiline arrow function with trailing comma as an error. This change directly fixes the prettier/prettier ESLint error at client.store.ts:104 introduced by the PR. No logic is altered — only formatting.
Tip
✅ We verified this fix by re-running @forgerock/journey-client:lint.
diff --git a/packages/journey-client/src/lib/client.store.ts b/packages/journey-client/src/lib/client.store.ts
index b2560ca9d..cbd85480f 100644
--- a/packages/journey-client/src/lib/client.store.ts
+++ b/packages/journey-client/src/lib/client.store.ts
@@ -101,9 +101,7 @@ export async function journey<ActionType extends ActionTypes = ActionTypes>({
'type',
] as const;
- const providedIgnored: string[] = ignoredProperties.filter(
- (prop) => config[prop] !== undefined,
- );
+ const providedIgnored: string[] = ignoredProperties.filter((prop) => config[prop] !== undefined);
if (config.serverConfig?.timeout !== undefined) {
providedIgnored.push('serverConfig.timeout');
Or Apply changes locally with:
npx nx-cloud apply-locally hdNj-E8JL
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
CI is failing but approving now assuming it will be fixed
…gOptions Allow the same config object to be shared across journey-client, davinci-client, and oidc-client. Properties like clientId, scope, and redirectUri are accepted but not used — a warning is logged when they are provided.
ryanbas21
force-pushed
the
fix-jc-missing-properties
branch
from
9ce0aff to
5fe2f41
Compare
3 participants
Summary
JourneyClientConfigfromAsyncLegacyConfigOptionsso the same config object can be shared across journey-client, davinci-client, and oidc-clientclientId,scope, andredirectUriare now accepted but ignored — a warning is logged when they are providedserverConfig.wellknownremains requiredTest plan
JourneyClientConfigextendsAsyncLegacyConfigOptionsserverConfig.wellknownis required (negative@ts-expect-errortest)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests