SDKS-5067: Standardize SDK Configuration#684
Open
SteinGabriel wants to merge 1 commit into
Open
Conversation
🦋 Changeset detectedLatest commit: 23202ab 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 23202ab
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds unified cross-platform SDK configuration support: ChangesUnified SDK config adoption across clients
Sequence DiagramssequenceDiagram
participant App
participant makeOidcConfig
participant validateUnifiedSdkConfig
participant unifiedToOidcConfig
participant oidc as oidc() client
App->>makeOidcConfig: makeOidcConfig(unifiedJson)
makeOidcConfig->>validateUnifiedSdkConfig: validate(unifiedJson)
validateUnifiedSdkConfig-->>makeOidcConfig: Either.right(UnifiedSdkConfig) or throws
makeOidcConfig->>unifiedToOidcConfig: map(UnifiedSdkConfig)
unifiedToOidcConfig-->>makeOidcConfig: Either.right(OidcConfig) or throws
makeOidcConfig-->>App: OidcConfig
App->>oidc: oidc({ config: OidcConfig })
oidc-->>App: { authorize, token, ... }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
@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 (21.99%) 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 #684 +/- ##
==========================================
+ Coverage 18.07% 21.99% +3.92%
==========================================
Files 155 158 +3
Lines 24398 25094 +696
Branches 1203 1513 +310
==========================================
+ Hits 4410 5520 +1110
+ Misses 19988 19574 -414
🚀 New features to boost your workflow:
|
Contributor
|
Deployed e8f6353 to https://ForgeRock.github.io/ping-javascript-sdk/pr-684/e8f6353bdc50db4bb6921dc12d0ce057aae55f13 branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/oidc-client - 30.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 |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
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 has identified a possible root cause for your failed CI:
This CI failure appears to be related to the environment or external dependencies rather than your code changes.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
ryanbas21
reviewed
Jun 9, 2026
ryanbas21
reviewed
Jun 9, 2026
| @@ -4,7 +4,7 @@ | |||
| * This software may be modified and distributed under the terms | |||
| * of the MIT license. See the LICENSE file for details. | |||
| */ | |||
Collaborator
There was a problem hiding this comment.
Let's use @effect/vitest if we're testing effect returning functions
Contributor
Author
There was a problem hiding this comment.
Updated it. Thanks!
ryanbas21
reviewed
Jun 9, 2026
ryanbas21
reviewed
Jun 9, 2026
ryanbas21
reviewed
Jun 9, 2026
|
|
||
| const REQUIRED_OIDC_FIELDS_JOURNEY: ReadonlyArray<keyof UnifiedOidcConfig> = ['discoveryEndpoint']; | ||
|
|
||
| function validateType( |
Collaborator
There was a problem hiding this comment.
per our discussion today, this could be a worthwhile case for Either<_,ConfigValidationError]> and accumulate errors in the error channel
Contributor
Author
There was a problem hiding this comment.
Updated it to use Either instead. Thanks for the suggestion!
ryanbas21
requested changes
Jun 9, 2026
ryanbas21
left a comment
ryanbas21
left a comment
Collaborator
There was a problem hiding this comment.
I think we really need to think about this. I'm concerned about how safe some of our changes here are, breaking changes, and the path we're taking here. I personally feel like we should discuss this more as a team because i'm not positive that this is the right path
ancheetah
reviewed
Jun 10, 2026
ancheetah
left a comment
ancheetah
left a comment
Collaborator
There was a problem hiding this comment.
Couple comments after a quick initial scan
| logger, | ||
| }: { | ||
| config: DaVinciConfig; | ||
| config: DaVinciConfig | Record<string, unknown>; |
Collaborator
There was a problem hiding this comment.
In Andy's design doc, he suggests initializing the client through a new property called json which contains the json config. Is there a reason why you deviated from this? I think it may be a good idea to distinguish between the standard config and json config. The typing could also be a little more strict than Record<string, unknown>. If we have agreed on a standard JSON config as a team then we should be able to create an interface for it. This also removes the need for a isUnifiedSdkConfig utility.
Contributor
Author
There was a problem hiding this comment.
Goal was to accept both legacy and unified configs, but for sure this was not the ideal way to achieve this. An utility function was added that validates and returns the config type each client expects, so the client interfaces stay the same.
vatsalparikh
requested changes
Jun 10, 2026
vatsalparikh
left a comment
vatsalparikh
left a comment
Contributor
There was a problem hiding this comment.
First review, mostly around type definition, usage, and duplications.
Side note: I do want to third (after AJ and Ryan) that we should think about narrowing the acceptable types for config instead of Record<string, unknown>
cerebrl
reviewed
Jun 11, 2026
| */ | ||
| export async function davinci<ActionType extends ActionTypes = ActionTypes>({ | ||
| config, | ||
| config: rawConfig, |
Collaborator
There was a problem hiding this comment.
Rather than handling this new config structure in each client like we are doing here, I'd rather see a utility function that transforms this new config to our existing config. Something like this:
const davinciClient = davinci(configUtil(unifiedConfig));
The configUtil validates the unified config object and returns the strongly typed config structure that is already in use.
Contributor
Author
There was a problem hiding this comment.
Added a utility function for each client that vaidates and creates the right config type from the unified config.
Thanks!
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/oidc-client/api-report/oidc-client.api.md (1)
241-266:⚠️ Potential issue | 🔴 CriticalRemove duplicate
OauthTokensinterface declaration.The
OauthTokensinterface is declared twice inpackages/oidc-client/src/lib/config.types.ts(lines 9-15 and 17-23) with identical content. In TypeScript, the second declaration overrides the first, making the duplicate redundant. Remove one of the declarations.🤖 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 `@packages/oidc-client/api-report/oidc-client.api.md` around lines 241 - 266, The OauthTokens interface is declared twice identically in the source file, causing the duplicate to appear in the API report. Locate the two identical OauthTokens interface declarations (one without the `@public` decorator and one with it) and remove the second duplicate declaration, keeping only the first one. This will eliminate the redundant interface from the API report file.
🧹 Nitpick comments (5)
packages/sdk-utilities/src/lib/config/config.utils.ts (4)
49-51: ⚡ Quick winImprove error message for array inputs.
The check
typeof input !== 'object' || input === nulldoesn't explicitly reject arrays. While arrays will fail validation later, the error message "Expected object, got object" is confusing when the input is an array.📝 Suggested improvement
- if (typeof input !== 'object' || input === null) { - return Either.left([{ field: prefix, message: `Expected object, got ${typeof input}` }]); + if (typeof input !== 'object' || input === null || Array.isArray(input)) { + return Either.left([{ field: prefix, message: `Expected object, got ${Array.isArray(input) ? 'array' : typeof input}` }]); }🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 49 - 51, The type check in the condition at this location does not explicitly reject arrays, which causes confusing error messages since typeof array returns 'object'. Modify the condition to additionally check if the input is an array using Array.isArray(), and update the error message to properly distinguish between array and non-object types by checking for arrays first and returning a message like "Expected object, got array" when an array is detected, otherwise use the existing type-based message.
259-266: ⚡ Quick winType guard doesn't explicitly exclude arrays.
The checks for
oidcandjourneyverify they're objects and not null, buttypeof [] === 'object'is true in JavaScript. While validation will catch array inputs later, the type guard should be more precise to ensure correct branching logic.🔍 Suggested improvement
const oidcIsObject = - 'oidc' in raw && typeof raw['oidc'] === 'object' && raw['oidc'] !== null; + 'oidc' in raw && typeof raw['oidc'] === 'object' && raw['oidc'] !== null && !Array.isArray(raw['oidc']); const journeyIsObject = - 'journey' in raw && typeof raw['journey'] === 'object' && raw['journey'] !== null; + 'journey' in raw && typeof raw['journey'] === 'object' && raw['journey'] !== null && !Array.isArray(raw['journey']);🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 259 - 266, The type guard function isUnifiedSdkConfig does not properly exclude arrays when checking if oidc and journey properties are objects. Since typeof [] === 'object' is true in JavaScript, arrays will pass the current object type checks. Add an explicit Array.isArray() check to exclude arrays for both the oidcIsObject and journeyIsObject conditions, ensuring that only plain objects (not arrays) satisfy the type guard requirements.
146-148: ⚡ Quick winImprove error message clarity for arrays.
The array check is present, but the error message doesn't distinguish between arrays and other non-object types. Use the same pattern as line 108 for consistency.
📝 Suggested improvement
if (typeof raw['journey'] !== 'object' || Array.isArray(raw['journey'])) { - return [{ field: 'journey', message: `Expected object, got ${typeof raw['journey']}` }]; + return [{ field: 'journey', message: `Expected object, got ${Array.isArray(raw['journey']) ? 'array' : typeof raw['journey']}` }]; }🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 146 - 148, The error message for the journey field validation does not distinguish between array and non-object types in the error message. Update the validation logic to provide a more specific error message that explicitly states "Expected object, got array" when the value is an array, following the same pattern used at line 108 for consistency. This requires checking Array.isArray(raw['journey']) separately and providing the appropriate error message for each case rather than using a generic typeof check in the error message.
130-132: ⚡ Quick winImprove error message for array inputs.
Same issue as in
validateUnifiedOidcConfig(line 49): array inputs should be explicitly rejected with a clear error message.📝 Suggested improvement
- if (typeof input !== 'object' || input === null) { - return Either.left([{ field: 'config', message: `Expected object, got ${typeof input}` }]); + if (typeof input !== 'object' || input === null || Array.isArray(input)) { + return Either.left([{ field: 'config', message: `Expected object, got ${Array.isArray(input) ? 'array' : typeof input}` }]); }🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 130 - 132, The validation check at lines 130-132 does not explicitly reject array inputs, even though arrays should be invalid. Since typeof array returns 'object' in JavaScript, arrays would currently pass this validation. Add an explicit check using Array.isArray() to detect and reject array inputs with a clear error message that distinguishes arrays from other invalid types, similar to how the issue is already handled in validateUnifiedOidcConfig at line 49. Update the condition to catch arrays early and return an appropriate error.packages/oidc-client/src/lib/config.types.ts (1)
9-15: ⚡ Quick winRemove the duplicate
OauthTokensdeclaration.
OauthTokensis declared twice in this file. Keeping a single declaration avoids unnecessary declaration merging and reduces maintenance risk.Suggested cleanup
-export interface OauthTokens { - accessToken: string; - idToken: string; - refreshToken?: string; - expiresAt?: number; - expiryTimestamp?: number; -}🤖 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 `@packages/oidc-client/src/lib/config.types.ts` around lines 9 - 15, The OauthTokens interface is declared twice in the file, which causes unnecessary declaration merging. Locate both declarations of the OauthTokens interface and remove the duplicate declaration, keeping only one instance that contains all the necessary properties (accessToken, idToken, refreshToken, expiresAt, and expiryTimestamp). If the two declarations have different properties, merge them into a single definition before removing the duplicate.
🤖 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 @.changeset/sdks-5067-unified-config.md:
- Line 23: The changeset documentation on line 23 references incorrect type
names `OidcDisplayValue` and `OidcPromptValue` as being exported from
sdk-utilities, but the actual canonical type names exported from sdk-types are
`AuthDisplayValue` and `AuthPromptValue`. Either remove these type references
from the sdk-utilities bullet point on line 23, or move them to the sdk-types
section (lines 33-35) with the correct type names `AuthDisplayValue` and
`AuthPromptValue`, clarifying that sdk-types is the canonical source for these
types.
---
Outside diff comments:
In `@packages/oidc-client/api-report/oidc-client.api.md`:
- Around line 241-266: The OauthTokens interface is declared twice identically
in the source file, causing the duplicate to appear in the API report. Locate
the two identical OauthTokens interface declarations (one without the `@public`
decorator and one with it) and remove the second duplicate declaration, keeping
only the first one. This will eliminate the redundant interface from the API
report file.
---
Nitpick comments:
In `@packages/oidc-client/src/lib/config.types.ts`:
- Around line 9-15: The OauthTokens interface is declared twice in the file,
which causes unnecessary declaration merging. Locate both declarations of the
OauthTokens interface and remove the duplicate declaration, keeping only one
instance that contains all the necessary properties (accessToken, idToken,
refreshToken, expiresAt, and expiryTimestamp). If the two declarations have
different properties, merge them into a single definition before removing the
duplicate.
In `@packages/sdk-utilities/src/lib/config/config.utils.ts`:
- Around line 49-51: The type check in the condition at this location does not
explicitly reject arrays, which causes confusing error messages since typeof
array returns 'object'. Modify the condition to additionally check if the input
is an array using Array.isArray(), and update the error message to properly
distinguish between array and non-object types by checking for arrays first and
returning a message like "Expected object, got array" when an array is detected,
otherwise use the existing type-based message.
- Around line 259-266: The type guard function isUnifiedSdkConfig does not
properly exclude arrays when checking if oidc and journey properties are
objects. Since typeof [] === 'object' is true in JavaScript, arrays will pass
the current object type checks. Add an explicit Array.isArray() check to exclude
arrays for both the oidcIsObject and journeyIsObject conditions, ensuring that
only plain objects (not arrays) satisfy the type guard requirements.
- Around line 146-148: The error message for the journey field validation does
not distinguish between array and non-object types in the error message. Update
the validation logic to provide a more specific error message that explicitly
states "Expected object, got array" when the value is an array, following the
same pattern used at line 108 for consistency. This requires checking
Array.isArray(raw['journey']) separately and providing the appropriate error
message for each case rather than using a generic typeof check in the error
message.
- Around line 130-132: The validation check at lines 130-132 does not explicitly
reject array inputs, even though arrays should be invalid. Since typeof array
returns 'object' in JavaScript, arrays would currently pass this validation. Add
an explicit check using Array.isArray() to detect and reject array inputs with a
clear error message that distinguishes arrays from other invalid types, similar
to how the issue is already handled in validateUnifiedOidcConfig at line 49.
Update the condition to catch arrays early and return an appropriate error.
🪄 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: 408272e8-09af-432e-a266-9bc32654662e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
.changeset/sdks-5067-unified-config.mdpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.test.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/config.types.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/config.types.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/logout.request.test.tspackages/oidc-client/src/lib/logout.request.tspackages/oidc-client/src/lib/oidc.api.tspackages/sdk-effects/logger/package.jsonpackages/sdk-effects/logger/src/lib/logger.types.tspackages/sdk-effects/logger/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-types/src/lib/authorize.types.tspackages/sdk-types/src/lib/config.types.tspackages/sdk-utilities/package.jsonpackages/sdk-utilities/src/index.tspackages/sdk-utilities/src/lib/config/config.test.tspackages/sdk-utilities/src/lib/config/config.types.tspackages/sdk-utilities/src/lib/config/config.utils.tspackages/sdk-utilities/src/lib/config/index.ts
SteinGabriel
force-pushed
the
SDKS-5067
branch
4 times, most recently
from
dd74f99 to
a45f2b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.changeset/sdks-5067-unified-config.md:
- Line 24: The changeset bullet for `AuthDisplayValue` and `AuthPromptValue`
types contradicts other parts of the PR documentation by claiming
`sdk-utilities` is the canonical source when `sdk-types` is actually documented
as owning these types. Either reword this bullet point to clarify that
`sdk-utilities` re-exports or consumes these types (removing the "canonical
source" claim), or move the canonical source ownership note into the `sdk-types`
section of the changeset where it accurately reflects the actual location of
these type definitions. Choose whichever approach is consistent with the PR
summary and other documentation sections.
🪄 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: 6fded97f-445e-46e9-a700-ccf208e5d75c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.changeset/sdks-5067-unified-config.mde2e/journey-suites/src/login.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.test.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/config.types.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/config.types.tspackages/journey-client/src/lib/journey.utils.tspackages/journey-client/src/types.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/logout.request.test.tspackages/oidc-client/src/lib/logout.request.tspackages/oidc-client/src/lib/oidc.api.tspackages/sdk-effects/logger/package.jsonpackages/sdk-effects/logger/src/lib/logger.types.tspackages/sdk-effects/logger/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-types/src/lib/authorize.types.tspackages/sdk-types/src/lib/config.types.tspackages/sdk-utilities/package.jsonpackages/sdk-utilities/src/index.tspackages/sdk-utilities/src/lib/config/config.test.tspackages/sdk-utilities/src/lib/config/config.types.tspackages/sdk-utilities/src/lib/config/config.utils.tspackages/sdk-utilities/src/lib/config/index.ts
✅ Files skipped from review due to trivial changes (6)
- packages/journey-client/src/lib/journey.utils.ts
- packages/journey-client/src/types.ts
- e2e/journey-suites/src/login.test.ts
- packages/sdk-effects/logger/tsconfig.lib.json
- packages/oidc-client/src/lib/authorize.request.micros.test.ts
- packages/journey-client/api-report/journey-client.api.md
🚧 Files skipped from review as they are similar to previous changes (30)
- packages/oidc-client/src/lib/client.store.ts
- packages/sdk-utilities/package.json
- packages/journey-client/src/lib/config.types.ts
- packages/oidc-client/src/lib/oidc.api.ts
- packages/sdk-utilities/src/index.ts
- packages/oidc-client/src/lib/logout.request.ts
- packages/oidc-client/src/lib/authorize.request.utils.test.ts
- packages/sdk-effects/oidc/src/lib/authorize.utils.ts
- packages/sdk-effects/logger/package.json
- packages/sdk-effects/logger/src/lib/logger.types.ts
- packages/oidc-client/src/lib/client.store.test.ts
- packages/davinci-client/src/lib/client.store.test.ts
- packages/sdk-utilities/src/lib/config/index.ts
- packages/journey-client/src/lib/client.store.ts
- packages/sdk-types/src/lib/authorize.types.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/journey-client/src/lib/client.store.test.ts
- packages/sdk-types/src/lib/config.types.ts
- packages/oidc-client/api-report/oidc-client.api.md
- packages/davinci-client/src/lib/config.types.ts
- packages/sdk-utilities/src/lib/config/config.types.ts
- packages/oidc-client/src/lib/authorize.request.micros.ts
- packages/davinci-client/api-report/davinci-client.api.md
- packages/oidc-client/src/lib/config.types.ts
- packages/davinci-client/api-report/davinci-client.types.api.md
- packages/journey-client/api-report/journey-client.types.api.md
- packages/sdk-effects/oidc/src/lib/authorize.test.ts
- packages/oidc-client/src/lib/logout.request.test.ts
- packages/sdk-utilities/src/lib/config/config.utils.ts
- packages/sdk-utilities/src/lib/config/config.test.ts
vatsalparikh
left a comment
vatsalparikh
left a comment
Contributor
There was a problem hiding this comment.
Some comments on packages/sdk-utilities/src/lib/config/config.utils.ts file
SteinGabriel
force-pushed
the
SDKS-5067
branch
3 times, most recently
from
7c54ffa to
87a8396
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts`:
- Around line 56-66: The scopes field validation in the requiredFields flatMap
only checks that scopes is an array type using checkType, but does not validate
that each element within the array is a string. Add validation logic to iterate
through the scopes array elements and check that each one is a string, returning
appropriate validation errors for any non-string elements. This validation needs
to be added at the anchor location
(packages/sdk-utilities/src/lib/config/config.utils.ts lines 56-66) in the
requiredFields flatMap after the checkType call, and the same validation pattern
must be applied to the sibling scopes validation sites at lines 68-77, 197-197,
and 252-252.
- Around line 134-136: The validation for the log field currently only checks if
it is a string type using checkType, but does not validate that the string value
is one of the supported LogLevel values. This allows any arbitrary string to
pass validation and later be force-cast by toMappedLogLevel, potentially causing
runtime issues. Instead of just type-checking as a string, add validation to
ensure the log value matches one of the supported LogLevel enum values. Apply
this fix at line 134-136 where logError is assigned (the checkType call for the
log field) and also at line 212 where similar log validation occurs as indicated
by the consolidated comment.
- Around line 194-200: Guard the `oidc.discoveryEndpoint` value in the
`unifiedToOidcConfig` mapper (lines 194-200) to prevent `serverConfig.wellknown`
from being undefined when malformed input is passed directly to the mapper.
Apply the same guard pattern to `unifiedToJourneyConfig` (lines 223-226) and
`unifiedToDavinciConfig` (lines 249-255), ensuring each mapper validates that
their respective discovery endpoint or equivalent configuration value exists
before including it in the returned serverConfig object. This ensures the
mappers fail safely or provide defined values rather than allowing undefined to
propagate to client initialization.
- Around line 96-99: The validation for refreshThreshold in the checkType call
currently only verifies the type is a number, but does not constrain it to be
finite and non-negative, allowing NaN, Infinity, and negative values through
which can cause unstable behavior. Replace or enhance the type check to
additionally validate that refreshThreshold is finite (using Number.isFinite)
and is greater than or equal to zero. Apply this same stricter validation at the
other locations in the file where similar validation occurs for this field.
🪄 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: 747f2b6e-b8d6-49a4-b705-5ad679eb8030
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.changeset/sdks-5067-unified-config.mdpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.test.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/config.types.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/config.types.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/logout.request.test.tspackages/oidc-client/src/lib/logout.request.tspackages/oidc-client/src/lib/oidc.api.tspackages/sdk-effects/logger/package.jsonpackages/sdk-effects/logger/src/lib/logger.types.tspackages/sdk-effects/logger/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-types/src/lib/authorize.types.tspackages/sdk-types/src/lib/config.types.tspackages/sdk-utilities/package.jsonpackages/sdk-utilities/src/index.tspackages/sdk-utilities/src/lib/config/config.test.tspackages/sdk-utilities/src/lib/config/config.types.tspackages/sdk-utilities/src/lib/config/config.utils.tspackages/sdk-utilities/src/lib/config/index.ts
✅ Files skipped from review due to trivial changes (4)
- packages/sdk-utilities/src/lib/config/index.ts
- packages/oidc-client/src/lib/authorize.request.micros.test.ts
- packages/oidc-client/api-report/oidc-client.types.api.md
- packages/journey-client/api-report/journey-client.api.md
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/sdk-utilities/package.json
- packages/sdk-effects/logger/tsconfig.lib.json
- packages/oidc-client/src/lib/client.store.ts
- packages/sdk-effects/oidc/src/lib/authorize.test.ts
- packages/oidc-client/src/lib/logout.request.ts
- packages/oidc-client/src/lib/authorize.request.utils.test.ts
- packages/davinci-client/src/lib/client.store.test.ts
- packages/oidc-client/src/lib/config.types.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/sdk-utilities/src/lib/config/config.types.ts
- packages/sdk-utilities/src/index.ts
- packages/journey-client/src/lib/client.store.ts
- packages/sdk-types/src/lib/authorize.types.ts
- packages/oidc-client/src/lib/oidc.api.ts
- packages/journey-client/src/lib/config.types.ts
- packages/journey-client/src/lib/client.store.test.ts
- packages/sdk-types/src/lib/config.types.ts
- packages/oidc-client/src/lib/authorize.request.micros.ts
- packages/oidc-client/api-report/oidc-client.api.md
- packages/sdk-effects/logger/src/lib/logger.types.ts
- packages/oidc-client/src/lib/client.store.test.ts
- packages/davinci-client/src/lib/config.types.ts
- packages/oidc-client/src/lib/authorize.request.utils.ts
- packages/oidc-client/src/lib/logout.request.test.ts
Comment on lines
+96
to
+99
| const refreshThresholdError = | ||
| raw['refreshThreshold'] !== undefined && raw['refreshThreshold'] !== null | ||
| ? checkType(raw['refreshThreshold'], 'number', `${prefix}.refreshThreshold`) | ||
| : null; |
There was a problem hiding this comment.
Constrain refreshThreshold to finite non-negative numbers.
typeof number currently permits NaN, Infinity, and negatives, which can map to unstable oauthThreshold values.
Suggested fix
const refreshThresholdError =
raw['refreshThreshold'] !== undefined && raw['refreshThreshold'] !== null
- ? checkType(raw['refreshThreshold'], 'number', `${prefix}.refreshThreshold`)
+ ? typeof raw['refreshThreshold'] !== 'number' ||
+ !Number.isFinite(raw['refreshThreshold']) ||
+ raw['refreshThreshold'] < 0
+ ? ({
+ field: `${prefix}.refreshThreshold`,
+ message: 'Expected a finite number greater than or equal to 0',
+ } satisfies ConfigValidationError)
+ : null
: null;Also applies to: 202-202, 257-257
🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 96 - 99,
The validation for refreshThreshold in the checkType call currently only
verifies the type is a number, but does not constrain it to be finite and
non-negative, allowing NaN, Infinity, and negative values through which can
cause unstable behavior. Replace or enhance the type check to additionally
validate that refreshThreshold is finite (using Number.isFinite) and is greater
than or equal to zero. Apply this same stricter validation at the other
locations in the file where similar validation occurs for this field.
Comment on lines
+134
to
+136
| const logError = | ||
| raw['log'] !== undefined && raw['log'] !== null ? checkType(raw['log'], 'string', 'log') : null; | ||
|
|
There was a problem hiding this comment.
Validate log against supported values before mapping.
validateUnifiedSdkConfig currently accepts any string for log, then toMappedLogLevel force-casts it to LogLevel. Unsupported values can silently pass through as runtime config.
Suggested fix
+const ALLOWED_LOG_LEVELS = ['DEBUG', 'INFO', 'WARN', 'ERROR'] as const;
+
const logError =
- raw['log'] !== undefined && raw['log'] !== null ? checkType(raw['log'], 'string', 'log') : null;
+ raw['log'] !== undefined && raw['log'] !== null
+ ? typeof raw['log'] !== 'string'
+ ? ({ field: 'log', message: `Expected string, got ${typeof raw['log']}` } satisfies ConfigValidationError)
+ : !ALLOWED_LOG_LEVELS.includes(raw['log'].toUpperCase() as (typeof ALLOWED_LOG_LEVELS)[number])
+ ? ({ field: 'log', message: `Expected one of ${ALLOWED_LOG_LEVELS.join(', ')}, got ${raw['log']}` } satisfies ConfigValidationError)
+ : null
+ : null;Also applies to: 212-212
🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 134 -
136, The validation for the log field currently only checks if it is a string
type using checkType, but does not validate that the string value is one of the
supported LogLevel values. This allows any arbitrary string to pass validation
and later be force-cast by toMappedLogLevel, potentially causing runtime issues.
Instead of just type-checking as a string, add validation to ensure the log
value matches one of the supported LogLevel enum values. Apply this fix at line
134-136 where logError is assigned (the checkType call for the log field) and
also at line 212 where similar log validation occurs as indicated by the
consolidated comment.
Comment on lines
+194
to
+200
| return Either.right({ | ||
| clientId, | ||
| redirectUri, | ||
| scope: scopes.join(' '), | ||
| serverConfig: { | ||
| wellknown: oidc.discoveryEndpoint, | ||
| ...(config.timeout !== undefined && { timeout: config.timeout }), |
There was a problem hiding this comment.
Guard oidc.discoveryEndpoint in each mapper.
The unifiedTo*Config mappers can currently return serverConfig.wellknown: undefined when called directly with malformed input, which then fails later in client initialization.
Suggested fix
const oidc = config.oidc;
+ if (!oidc.discoveryEndpoint) {
+ return Either.left({ field: 'oidc.discoveryEndpoint', message: 'Required field is missing' });
+ }Apply the same guard in unifiedToOidcConfig, unifiedToJourneyConfig, and unifiedToDavinciConfig.
Also applies to: 223-226, 249-255
🤖 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 `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 194 -
200, Guard the `oidc.discoveryEndpoint` value in the `unifiedToOidcConfig`
mapper (lines 194-200) to prevent `serverConfig.wellknown` from being undefined
when malformed input is passed directly to the mapper. Apply the same guard
pattern to `unifiedToJourneyConfig` (lines 223-226) and `unifiedToDavinciConfig`
(lines 249-255), ensuring each mapper validates that their respective discovery
endpoint or equivalent configuration value exists before including it in the
returned serverConfig object. This ensures the mappers fail safely or provide
defined values rather than allowing undefined to propagate to client
initialization.
feat(sdk-utilities): add pure unified config validation feat(sdk-utilities): add unified config mapping functions feat(sdk-utilities): export config module from sdk-utilities test(sdk-utilities): add unit tests for config mapping and validation feat(sdk-types): add OIDC authorize params to GetAuthorizationUrlOptions feat(sdk-oidc): wire OIDC authorize params into buildAuthorizeParams feat(oidc-client): add signOutRedirectUri to endSession URL feat(oidc-client): accept unified JSON config in oidc factory feat(journey-client): accept unified JSON config in journey factory feat(davinci-client): accept unified JSON config in davinci factory chore: update api-reports for unified config param widening feat(sdk-utilities): align unified config schema with new journey sub-object refactor(sdk-utilities): mapper functions return ConfigMappingResult with single error fix(sdk-utilities): map log field and use config.log in factories fix(sdk-utilities): tighten isUnifiedSdkConfig discriminant to require object values fix(oidc-client): use throw instead of Promise.reject in unified config error paths refactor(oidc-client): migrate logout.request.test.ts to it + Micro.runPromise pattern docs(oidc-client): add JSDoc to OidcConfig and all new fields fix(sdk): tighten config param types to XConfig | UnifiedSdkConfig test(oidc-client): migrate authorize.request.micros tests to @effect/vitest test(oidc-client): migrate authorize.request.utils tests to @effect/vitest refactor(sdk-utilities): use as const satisfies for required-field constants refactor(sdk-utilities): replace mutable error array with effect Either in config validators refactor(sdk): utility-first config — move json→config transform to sdk-utilities refactor(sdk-utilities): remove LogLevel duplicates — use LogLevel from sdk-types docs(changeset): update sdks-5067 changeset to reflect utility-first config API refactor(sdk-utilities): rename *ConfigFromJson to make*Config refactor(sdk-types): move client config types to sdk-types, delete Mapped* mirrors fix(oidc-client): remove duplicate OauthTokens interface declaration fix(oidc-client): remove config.state from OidcConfig — PKCE state must not be overridden refactor(sdk-utilities): remove isUnifiedSdkConfig — dead code after utility-first refactor refactor(sdk-utilities): remove strictOidc param — move strict field checks into mappers fix(sdk-utilities): guard empty scopes array in unifiedToOidcConfig and unifiedToDavinciConfig refactor(sdk-utilities): move impure config factories to config.effects.ts
ryanbas21
requested changes
Jun 17, 2026
| import { it, expect } from '@effect/vitest'; | ||
| import { it } from '@effect/vitest'; | ||
| import { Micro } from 'effect'; | ||
| import { vi, afterEach } from 'vitest'; |
Collaborator
There was a problem hiding this comment.
FWIW - we don't need the vitest import, everything is the same on @effect/vitest. I think for micros/effects we want to use assert also
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
|
|
||
| import { Either } from 'effect'; |
Collaborator
There was a problem hiding this comment.
Let's namespace import these when possible.
| : { field, message: `Expected ${expected}, got ${typeof value}` }; | ||
| } | ||
|
|
||
| export function validateUnifiedOidcConfig( |
Collaborator
There was a problem hiding this comment.
I think this is where I start having issues with the PR. I think we're still doing validation not parsing. And this is a brainful to process in my head. (not sure if this is a phrase but i'm using it!)
I think what we can look at to make this more manageable -
We can create a parser per section we need to parse. So perhaps it could be parser per key.
input -> parser -> (input, Either<ParsedConfig, Error[]>) can roughly be how we imagine this function.
Maybe the typescript is:
type ParseResult<A> = Either<A, ConfigValidationError[]>;
type Parser<A> = (input: Record<string, unknown>) => [
remaining: Record<string, unknown>,
result: ParseResult<A>,
];
So a clientId parser would:
Take the full input object
Pull out clientId, validate and type it
Return the object minus clientId as remaining, plus Right or Left<Error[]> <-- this part is up for debate since json is a bit weird in that everything is immediately available, however the pattern still is here.
This is typically how parsers work, they take some input, and return the input and the new parsed input. The key is that we can continually compose based on the parsing result whatever it is.
then we can apply the next parser, this finishes when there is no input to parse further, or in our case, when there are no parsers left to apply since we have all the keys at one time.
Then we have a list of parser errors on the left or the final result on the right
This can help us:
A) make the parsing testable
B) break down the logic into smaller bits
C) helps us compose future parsers and extend the logic if new json comes in.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
https://pingidentity.atlassian.net/browse/SDKS-5067
Implements a unified cross-platform JSON configuration schema for the JS SDK. A new
sdk-utilitiesconfigmodule validates and maps the unified JSON shape to each factory's native config type. All three factories
(
oidc,journey,davinci) retain their existing typedconfigparameter — callers convert unified JSON viamakeOidcConfig/makeJourneyConfig/makeDavinciConfigbefore passing it in. Non-breaking: existingnative config objects continue to work unchanged.
Changes
packages/sdk-typesconfig.types.ts—OidcConfig,JourneyClientConfig,JourneyServerConfig,DaVinciConfigmoved here ascanonical definitions; extended with
signOutRedirectUri,loginHint,state,nonce,display,prompt,uiLocales,acrValues,query,logonOidcConfigauthorize.types.ts—GetAuthorizationUrlOptionsextended withloginHint,nonce,display,uiLocales,acrValues;promptwidened to include'select_account';AuthDisplayValueandAuthPromptValuedefined herepackages/sdk-utilitiessrc/lib/config/module:config.types.ts—UnifiedSdkConfig,UnifiedOidcConfig,LogLevelString, re-exportsAuthDisplayValue,AuthPromptValuefromsdk-types; no enums,type/interfaceonlyconfig.utils.ts— pure validation and mapping; useseffectEither.Either<T, ConfigValidationError[]>for results:
validateUnifiedOidcConfig/validateUnifiedSdkConfig— required-field and type checks; strict mode foroidc/davinci, lenient (only
discoveryEndpointrequired) for journeyisUnifiedSdkConfig— discriminator:'oidc' in input || 'journey' in inputunifiedToOidcConfig/unifiedToJourneyConfig/unifiedToDavinciConfig— pure mappers:scopes[]→scopestring,refreshThreshold(sec) →oauthThreshold(ms),log→logLevel,discoveryEndpoint→serverConfig.wellknownmakeOidcConfig(json)/makeJourneyConfig(json)/makeDavinciConfig(json)— public entry points;validate + map + throw on invalid; return the native typed config
config/index.ts— barrel exportsrc/index.ts— exportsconfigmodulepackages/oidc-clientconfig.types.ts— re-export shim; canonical types now insdk-typesoidc.api.ts— appendspost_logout_redirect_urito end-session URL whenconfig.signOutRedirectUriis setauthorize.request.utils.ts/authorize.request.micros.ts— useAuthPromptValuefromsdk-utilities(wasduplicate local
PromptValue)client.store.ts—authorize.url()forwards new OIDC params (loginHint,state,nonce,display,prompt,uiLocales,acrValues,query) from config intocreateAuthorizeUrlpackages/journey-clientconfig.types.ts— re-export shim; canonical types now insdk-typespackages/davinci-clientconfig.types.ts— re-export shim; canonical types now insdk-typespackages/sdk-effects/oidcauthorize.utils.ts—buildAuthorizeParamswiresloginHint,nonce,display,uiLocales,acrValuesinto the authorize URL
Tests
config.test.tsinsdk-utilities: valid mapping for all three factories, eachrequired-field-missing error, type-mismatch errors, unknown-field-ignored,
refreshThreshold→oauthThresholdconversion, scopes join, log field mapping,
cookieNamenon-mappingclient.store.test.tsinoidc-client,journey-client,davinci-client: success path + two throw caseseach (validation failure, mapping failure) via
make*Configauthorize.test.tsinsdk-effects/oidc: 6 tests covering each new OIDC param individually and all togetherauthorize.request.micros.test.ts,authorize.request.utils.test.ts,logout.request.test.tsinoidc-clientfor new config shapeHow to test
1. Unit tests