feat(journey-client): wellknown-endpoint-config-support#525
feat(journey-client): wellknown-endpoint-config-support#525
Conversation
🦋 Changeset detectedLatest commit: 072a55e 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 |
📝 WalkthroughWalkthroughThis PR introduces OIDC well-known endpoint discovery across the SDK ecosystem. A new shared well-known module in Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant JC as journey-client
participant RTK as RTK Query<br/>(wellknownApi)
participant WKEndpoint as Well-known<br/>Endpoint
participant Redux as Redux Store
participant AM as AM OAuth2<br/>Endpoint
App->>JC: journey({ serverConfig: { wellknown } })
JC->>RTK: Dispatch wellknownApi.endpoints.configuration
RTK->>WKEndpoint: GET /.well-known/openid-configuration
WKEndpoint-->>RTK: WellknownResponse
RTK->>Redux: Cache response in wellknown state
RTK-->>JC: WellknownResponse
JC->>JC: inferRealmFromIssuer(issuer)
JC->>JC: inferBaseUrlFromWellknown(wellknownUrl)
JC->>Redux: setConfig(InternalJourneyClientConfig)
JC-->>App: JourneyClient | GenericError
App->>JC: isJourneyClient(result)
Note over App: Type guard validation
App->>JC: client.start()
JC->>Redux: getState() → config
JC->>AM: POST /authenticate
AM-->>JC: Step (with callbacks)
JC-->>App: Step
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes The diff introduces heterogeneous changes across multiple packages with distinct reasoning required for each: new well-known discovery module logic, journey-client configuration restructuring with breaking API changes, parallel refactoring in davinci-client and oidc-client to consume shared modules, type consolidation across packages, E2E integration updates, and comprehensive test coverage for new paths. The breadth of files (70+) and mix of new features, refactoring, and type changes demands careful cross-package verification. 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 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 072a55e
☁️ Nx Cloud last updated this comment at |
@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 (19.36%) 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 #525 +/- ##
==========================================
+ Coverage 18.79% 19.36% +0.57%
==========================================
Files 140 148 +8
Lines 27640 28005 +365
Branches 980 1028 +48
==========================================
+ Hits 5195 5424 +229
- Misses 22445 22581 +136
🚀 New features to boost your workflow:
|
|
Deployed 6040463 to https://ForgeRock.github.io/ping-javascript-sdk/pr-525/60404630041fd0447a1f86618c6bcd942cea5b9b branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/sdk-utilities - 9.9 KB (+1.4 KB, +16.0%) 📊 Minor Changes📈 @forgerock/oidc-client - 23.8 KB (+0.3 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![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/journey-client/src/lib/client.store.ts (1)
68-179: Apply request middleware to well-known discovery.
The discovery call is made via a temp store that doesn’t includerequestMiddleware, so any custom headers/auth/fetch logic won’t apply to the well-known request. That can break environments that rely on middleware.🔧 Proposed fix
async function resolveAsyncConfig( config: JourneyConfigInput & { serverConfig: { wellknown: string } }, log: ReturnType<typeof loggerFn>, + requestMiddleware?: RequestMiddleware[], ): Promise<InternalJourneyClientConfig> { @@ - const tempStore = createJourneyStore({ config: tempConfig, logger: log }); + const tempStore = createJourneyStore({ config: tempConfig, logger: log, requestMiddleware }); @@ - resolvedConfig = await resolveAsyncConfig(config, log); + resolvedConfig = await resolveAsyncConfig(config, log, requestMiddleware);
🤖 Fix all issues with AI agents
In `@packages/sdk-effects/oidc/package.json`:
- Line 31: Replace the explicit version for the dependency key
"@reduxjs/toolkit" in package.json (currently "^2.8.0") with the monorepo
catalog spec so it follows the workspace pattern — set the version to
"catalog:^2.8.2" (i.e., change the value for "@reduxjs/toolkit" to
"catalog:^2.8.2") so the package uses the single source of truth defined in
pnpm-workspace.yaml.
🧹 Nitpick comments (4)
packages/sdk-utilities/src/lib/wellknown/wellknown.utils.ts (1)
85-98: Consider adding IPv6 localhost support.The function allows HTTP for
localhostand127.0.0.1, but not for::1(IPv6 localhost) or[::1]. This is a minor edge case but could cause unexpected behavior in IPv6-only development environments.♻️ Optional: Add IPv6 localhost support
// Allow HTTP only for localhost (development) - const isLocalhost = url.hostname === 'localhost' || url.hostname === '127.0.0.1'; + const isLocalhost = + url.hostname === 'localhost' || + url.hostname === '127.0.0.1' || + url.hostname === '[::1]' || + url.hostname === '::1'; const isSecure = url.protocol === 'https:'; const isHttpLocalhost = url.protocol === 'http:' && isLocalhost;packages/oidc-client/src/lib/wellknown.api.ts (2)
33-38: Selector created on every call defeats memoization.
createSelectoris invoked inside the function body, creating a new memoized selector on each call towellknownSelector. This negates the memoization benefits since each call produces a fresh selector instance.Consider creating the selector once per
wellknownUrlor usingcreateWellknownSelectorfrom the re-exported utilities if it handles this pattern.♻️ Option: Cache selectors by URL
+const selectorCache = new Map<string, ReturnType<typeof createSelector>>(); + export function wellknownSelector(wellknownUrl: string, state: RootState) { - const selector = createSelector( - wellknownApi.endpoints.configuration.select(wellknownUrl), - (result) => result?.data, - ); + let selector = selectorCache.get(wellknownUrl); + if (!selector) { + selector = createSelector( + wellknownApi.endpoints.configuration.select(wellknownUrl), + (result) => result?.data, + ); + selectorCache.set(wellknownUrl, selector); + } return selector(state); }Alternatively, if
createWellknownSelector(already exported) provides this functionality, consider using it directly instead of this wrapper.
18-21: Consider combining export and import statements.The separate re-export (line 18) and import (line 21) from the same package can be consolidated.
♻️ Suggested consolidation
-export { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; - -// Import locally for use in selector below -import { wellknownApi } from '@forgerock/sdk-oidc'; +import { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; + +export { wellknownApi, createWellknownSelector };packages/journey-client/src/lib/wellknown.utils.test.ts (1)
64-69: Consider adding a comment explaining intentional type casts.The type assertions (
as AsyncJourneyClientConfig,as JourneyClientConfig) are used to bypass TypeScript's checks and test edge cases with invalid inputs. A brief comment would clarify this intent for future maintainers.const config: JourneyConfigInput = { serverConfig: { baseUrl: 'https://am.example.com/am/', wellknown: '', }, - } as AsyncJourneyClientConfig; + } as AsyncJourneyClientConfig; // Intentionally cast to test runtime behavior with empty wellknown
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
e2e/am-mock-api/package.json
Outdated
| "body-parser": "^2.2.0", | ||
| "body-parser": "^2.2.2", | ||
| "cookie-parser": "^1.4.7", | ||
| "cors": "^2.8.5", | ||
| "express": "^4.21.2", | ||
| "express": "^5.2.1", | ||
| "superagent": "^10.2.3", | ||
| "uuid": "^13.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/express": "^4.17.17" |
There was a problem hiding this comment.
This is because i rebased my open deps pr because i was having issues with commands that were fixed via upgrading
| export default function (app) { | ||
| // Passthrough route that enforces authentication | ||
| app.all('/resource/*', async (req, res, next) => { | ||
| app.all('/resource/{*splat}', async (req, res, next) => { |
There was a problem hiding this comment.
fixed when i rebased open-deps (part of this express upgrade)
| "lint-staged": "^15.0.0", | ||
| "madge": "8.0.0", | ||
| "nx": "21.2.3", | ||
| "nx": "22.3.3", |
There was a problem hiding this comment.
all of this is because of dependency upgrades
| return async () => { | ||
| await serverInfo.set(serverSlice); | ||
| const setResult = await serverInfo.set(serverSlice); | ||
| if (isGenericError(setResult)) { |
There was a problem hiding this comment.
helps prevent type casting.
| end_session_endpoint: 'https://example.com/logout', | ||
| pushed_authorization_request_endpoint: '', | ||
| check_session_iframe: '', | ||
| introspection_endpoint: '', |
There was a problem hiding this comment.
Per the https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata, only a few properties are REQUIRED - most are optional.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/journey-client/src/lib/journey.api.ts (1)
118-186: Guard against missingjourneyslice to avoid a TypeError.If the store is misconfigured,
state.journeycan be undefined and throw before the explicit error is raised.🛡️ Suggested defensive guard
- const state = api.getState() as JourneyRootState; - const config = state.journey.config; + const state = api.getState() as JourneyRootState; + const config = state?.journey?.config; if (!config?.serverConfig) { throw new Error('Server configuration is missing.'); } @@ - const state = api.getState() as JourneyRootState; - const config = state.journey.config; + const state = api.getState() as JourneyRootState; + const config = state?.journey?.config; if (!config?.serverConfig) { throw new Error('Server configuration is missing.'); } @@ - const state = api.getState() as JourneyRootState; - const config = state.journey.config; + const state = api.getState() as JourneyRootState; + const config = state?.journey?.config; if (!config?.serverConfig) { throw new Error('Server configuration is missing.'); }
🤖 Fix all issues with AI agents
In `@e2e/am-mock-api/package.json`:
- Line 14: Update the devDependency entry for "@types/express" to a
v5-compatible range (e.g., "^5.0.0") so it matches the installed "express" v5.x
and avoids type mismatches; modify the "@types/express" version in package.json
(devDependencies) and then reinstall/update the lockfile (npm/yarn) to ensure
the new types are applied.
In `@package.json`:
- Around line 62-73: Run the NX migration to apply breaking-change updates by
executing npx nx migrate latest and npx nx migrate --run-migrations, then update
configuration related to the nx-release-publish target (verify its release
config structure matches NX 22 expectations), ensure any custom plugins use
createNodesV2 and that TypeScript settings explicitly set
useLegacyTypescriptPlugin if needed (defaults changed to false), and remove
references or reliance on removed items like NX_DISABLE_DB, experimental JS
executor inlining, and legacy options; finally run the workspace
lint/build/tests to confirm everything works.
- Around line 87-88: Upgrade to Vitest 4.0.9 may introduce breaking changes; run
the full test suite (unit, integration, and snapshot tests) and verify
mocking/reporters after bumping "@vitest/coverage-v8" and "@vitest/ui". If
failures occur, update test config and code references: replace any legacy
pool/worker options with modern names (e.g., maxThreads → maxWorkers), ensure
coverage settings use coverage.include and provider: 'v8' (remove
coverage.all/coverage.extensions), adapt any deprecated test API signatures and
mocking/snapshot usage, and regenerate snapshots where appropriate; confirm
reporter output still matches expected formats.
In `@packages/davinci-client/src/lib/config.types.test-d.ts`:
- Around line 94-122: Tests assert that introspection_endpoint and
revocation_endpoint are required, but the comment (and RFC 8414) says they're
optional; fix by making the test and comment consistent: update the comment near
WellKnownResponse to state that only issuer, authorization_endpoint,
token_endpoint, and userinfo_endpoint are required, then remove the expectTypeOf
assertions for introspection_endpoint and revocation_endpoint (the other
expectTypeOf calls for
issuer/authorization_endpoint/token_endpoint/userinfo_endpoint should remain),
or alternatively change the WellKnownResponse type to mark
introspection_endpoint and revocation_endpoint optional if you control that
type; reference symbols: WellKnownResponse and the expectTypeOf assertions in
the test.
In `@packages/journey-client/package.json`:
- Around line 33-34: Remove the build/test tools from the runtime dependencies:
delete the "vite" and "vitest-canvas-mock" entries from the dependencies block
so they exist only under devDependencies; locate the package.json dependencies
array (look for the "vite" and "vitest-canvas-mock" keys) and remove those keys,
leaving the current devDependencies entries intact.
In `@packages/journey-client/src/lib/wellknown.api.ts`:
- Line 14: Add createWellknownError to the barrel export so consumers can import
it alongside wellknownApi and createWellknownSelector; update the export
statement in wellknown.api.ts (currently exporting wellknownApi and
createWellknownSelector) to also export createWellknownError from
'@forgerock/sdk-oidc' to maintain API parity with davinci-client and support
usage in client.store.ts and wellknown.utils.ts.
In `@packages/journey-client/src/lib/wellknown.utils.ts`:
- Around line 38-46: The type guard hasWellknownConfig currently only checks for
serverConfig.wellknown and can incorrectly narrow JourneyConfigInput to
AsyncJourneyClientConfig; update hasWellknownConfig to also validate that
config.serverConfig has a non-empty string baseUrl (i.e., check 'baseUrl' in
config.serverConfig && typeof config.serverConfig.baseUrl === 'string' &&
config.serverConfig.baseUrl.length > 0) so the guard reliably asserts
AsyncJourneyClientConfig.
In `@tools/user-scripts/package.json`:
- Around line 25-26: The package.json currently lists "vitest" under
dependencies; remove the duplicate "vitest" entry from the dependencies object
so vitest remains only in devDependencies (leave "@effect/vitest" as-is if
intended); update the dependencies block to delete the "vitest" key to ensure
test-only packages are not shipped in runtime dependencies.
🧹 Nitpick comments (2)
package.json (1)
52-52: Emptydependenciesobject is unnecessary.The root package.json typically doesn't need a
dependenciesfield for a monorepo. If this was added intentionally for a specific tooling requirement, consider adding a comment explaining its purpose; otherwise, it can be removed.packages/oidc-client/src/lib/wellknown.api.ts (1)
12-38: Use the shared selector factory instead of recreating selectors.The
createWellknownSelectorhelper from@forgerock/sdk-oidcreturns a memoized selector function, which can be directly called with state. Refactoring to use it aligns with the documented intent and avoids recreating the selector on each call.♻️ Proposed refactor
-import { createSelector } from '@reduxjs/toolkit'; - import type { RootState } from './client.types.js'; /** * Re-export the shared wellknown RTK Query API from `@forgerock/sdk-oidc`. @@ export { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; // Import locally for use in selector below -import { wellknownApi } from '@forgerock/sdk-oidc'; +import { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; @@ export function wellknownSelector(wellknownUrl: string, state: RootState) { - const selector = createSelector( - wellknownApi.endpoints.configuration.select(wellknownUrl), - (result) => result?.data, - ); - return selector(state); + return createWellknownSelector(wellknownUrl)(state); }
| * @param value - The value to check | ||
| * @returns True if value is a non-null object | ||
| */ | ||
| function isObject(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
Can't add a generic to this since the type guard would become unsafe.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/journey-client/src/lib/config.types.ts`:
- Around line 11-30: The JourneyClientConfig docs require baseUrl but the type
inherits optional serverConfig from BaseConfig; update JourneyClientConfig to
override serverConfig as required with baseUrl enforced (e.g., replace the
inherited optional serverConfig with a non-optional field that ensures baseUrl:
string is present) so the type matches the documentation; alternatively, if
serverConfig may remain optional, update the doc comment to remove the
"required" wording — change either the JourneyClientConfig type (override
serverConfig) or the doc text to keep them consistent (refer to
JourneyClientConfig, BaseConfig, and serverConfig/baseUrl in your change).
🧹 Nitpick comments (7)
e2e/davinci-app/package.json (1)
19-19: Optional: drop emptydevDependenciesblock.
Keeps the manifest leaner if no dev-only deps are required.♻️ Proposed cleanup
- "devDependencies": {},package.json (1)
52-52: Optional: remove emptydependencies.
Avoids a redundant block if not needed.♻️ Proposed cleanup
- "dependencies": {},packages/journey-client/src/lib/device/device-profile.test.ts (1)
10-10: Type update fromSpyInstancetoMockis correct.The change aligns with Vitest's API evolution where
SpyInstancewas deprecated in favor ofMock. However, the explicit callable signatures in the generic parameter are verbose and may not be necessary.Consider simplifying to:
let warnSpy: Mock<typeof console.warn>;This achieves the same type safety with less boilerplate.
Also applies to: 89-92
packages/sdk-utilities/src/lib/wellknown/wellknown.utils.test.ts (1)
12-72: Test coverage is good; consider simplifying nested describe blocks.The test cases cover the main validation scenarios well. However, the nested
describeblocks are redundant—each wraps only a singleit()and duplicates the test description.♻️ Suggested simplification
describe('isValidWellknownUrl', () => { - describe('isValidWellknownUrl_HttpsUrl_ReturnsTrue', () => { - it('should return true for HTTPS URL', () => { - expect(isValidWellknownUrl('https://am.example.com/.well-known/openid-configuration')).toBe( - true, - ); - }); - }); + it('should return true for HTTPS URL', () => { + expect(isValidWellknownUrl('https://am.example.com/.well-known/openid-configuration')).toBe( + true, + ); + }); // ... similarly for other test casesAdditionally, consider adding edge cases:
- URLs without ports (e.g.,
https://am.example.com/.well-known/openid-configuration)- URLs with query strings or fragments
- Null/undefined inputs (if the function signature allows)
packages/journey-client/src/lib/config.types.ts (1)
32-50: Reduce duplication by extending PathsConfig.
WellknownServerConfigmirrorsPathsConfigfields. ExtendingPathsConfigkeeps future additions consistent.♻️ Proposed refactor
-export interface WellknownServerConfig { - /** Base URL for AM-specific endpoints (authenticate, sessions) */ - baseUrl: string; - /** URL to the OIDC well-known configuration endpoint */ - wellknown: string; - /** Custom path overrides for endpoints */ - paths?: PathsConfig['paths']; - /** Request timeout in milliseconds */ - timeout?: number; -} +export interface WellknownServerConfig extends PathsConfig { + /** URL to the OIDC well-known configuration endpoint */ + wellknown: string; +}packages/journey-client/src/lib/journey.api.ts (1)
118-123: Consider extracting config retrieval into a helper function.The same config retrieval and validation pattern is repeated in
start,next, andterminateendpoints (lines 118-123, 152-157, 183-188). A helper could reduce duplication:♻️ Optional refactor
function getConfigFromState(api: BaseQueryApi): { realmPath?: string; serverConfig: ServerConfig } { const state = api.getState() as JourneyRootState; const config = state.journey.config; if (!config?.serverConfig) { throw new Error('Server configuration is missing.'); } return { realmPath: config.realmPath, serverConfig: config.serverConfig }; }Then in each endpoint:
-const state = api.getState() as JourneyRootState; -const config = state.journey.config; -if (!config?.serverConfig) { - throw new Error('Server configuration is missing.'); -} -const { realmPath, serverConfig } = config; +const { realmPath, serverConfig } = getConfigFromState(api);packages/journey-client/src/lib/client.store.ts (1)
131-135: Structured error information is discarded.
createWellknownErrorreturns aGenericErrorwitherror,message,type, andstatusfields, but onlymessageis used when throwing. Consider preserving the structured error or using a custom error class:♻️ Preserve structured error information
Option 1 - Create a custom error class:
class WellknownError extends Error { constructor(public readonly genericError: GenericError) { super(genericError.message); this.name = 'WellknownError'; } } // Usage: throw new WellknownError(genericError);Option 2 - Add cause to the error:
-throw new Error(genericError.message); +throw new Error(genericError.message, { cause: genericError });
ryanbas21
force-pushed
the
well-known
branch
2 times, most recently
from
85a4e00 to
95eee18
Compare
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This looks good; left a few comments.
There was a problem hiding this comment.
Is this file even needed now?
There was a problem hiding this comment.
I think i can remove the re-exports and move them to where its used
There was a problem hiding this comment.
Do we need these tests anymore if the wellknown effect in the OIDC module is also tested? I would think we could move or delete these.
There was a problem hiding this comment.
I believe These tests are needed - they test journey-client specific code:
- hasWellknownConfig - type guard specific to journey-client's config types
- inferRealmFromIssuer - AM-specific utility that lives in journey-client
| import { createSelector } from '@reduxjs/toolkit'; | ||
| import { createApi, fetchBaseQuery } from '@reduxjs/toolkit/query'; | ||
|
|
||
| import type { WellKnownResponse } from '@forgerock/sdk-types'; |
There was a problem hiding this comment.
Let's try to be consistent with how we write "wellknown". It's mostly written wellknown, but some instances, like this, we have wellKnown.
There was a problem hiding this comment.
I think the difference here is that it's a type, versus a variable name. Would you still want the type lower case?
There was a problem hiding this comment.
I just would like consistency with how we write the word. I think historically, we've written it as wellknown. If we change it to wellKnown, then we need to update all the other instances to match. Again, I'm just advocating for consistency.
There was a problem hiding this comment.
Right but this is a Type. we typically capitalize types. I'm not sticking to this, if we want to write it lower case I will but theres the consistency in how we write the word, and consistency in how we write types.
Just not sure which one takes "precedence".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/davinci-client/src/lib/client.store.ts`:
- Around line 252-257: The catch blocks produce unhelpful "undefined"/"null"
strings by using String(err); change the error extraction in the resume and
update catch handlers to guard nullish thrown values: compute errorMessage with
something like err instanceof Error ? err.message : (err == null ? '' :
String(err)), use log.error(errorMessage) and keep the existing fallback when
building the returned error object so the fallback 'An unexpected error
occurred...' is used; apply the same fix to the update catch at the block that
constructs errorMessage around Line 339 (update/resume catch handlers and the
errorMessage variable/log.error usage).
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/client.store.utils.ts (1)
16-16: Consider creating a localwellknown.api.tswrapper for consistency with other clients.While the direct import from
@forgerock/sdk-oidcwill work (the dependency is properly configured and the export exists), other clients in the workspace (oidc-client,journey-client) follow a pattern of wrapping this import in a localwellknown.api.tsfile. Adopting this pattern indavinci-clientwould improve consistency across the codebase and make it easier to manage the wellknown API if changes are needed.
upgrades nx dependencies and touch points
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-types/src/lib/legacy-config.types.ts (1)
50-79:⚠️ Potential issue | 🟠 MajorAdd a backward-compatible alias for the renamed type.
Renaming a public exported type breaks downstream TypeScript builds. If this is not a major release, provide a compatibility alias (or deprecate the old name).🧩 Suggested compatibility alias
export interface WellknownResponse { issuer: string; authorization_endpoint: string; pushed_authorization_request_endpoint?: string; token_endpoint: string; userinfo_endpoint: string; end_session_endpoint: string; ping_end_idp_session_endpoint?: string; introspection_endpoint: string; revocation_endpoint: string; jwks_uri?: string; device_authorization_endpoint?: string; claims_parameter_supported?: boolean; request_parameter_supported?: boolean; request_uri_parameter_supported?: boolean; require_pushed_authorization_requests?: boolean; scopes_supported?: string[]; response_types_supported?: string[]; response_modes_supported?: string[]; grant_types_supported?: string[]; subject_types_supported?: string[]; id_token_signing_alg_values_supported?: string[]; userinfo_signing_alg_values_supported?: string[]; request_object_signing_alg_values_supported?: string[]; token_endpoint_auth_methods_supported?: string[]; token_endpoint_auth_signing_alg_values_supported?: string[]; claim_types_supported?: string[]; claims_supported?: string[]; code_challenge_methods_supported?: string[]; } + +/** `@deprecated` Use WellknownResponse */ +export type WellKnownResponse = WellknownResponse;
🤖 Fix all issues with AI agents
In `@nx.json`:
- Around line 150-155: The CI is invoking the wrong target name: the Vitest
plugin configured testTargetName as "nxTest" but the workflow and targetDefaults
use "test", so CI won't run tests and nxTest won't inherit cache/settings;
either change the plugin option testTargetName to "test" and update package.json
scripts that call "nx nxTest" to "nx test", or update the CI workflow command to
use "nxTest" and add corresponding targetDefaults entries for "nxTest" (copy
inputs, outputs, dependsOn, cache settings from the existing "test" defaults) so
the generated nxTest targets run in CI and inherit the same configuration.
In `@packages/oidc-client/src/lib/config.types.ts`:
- Around line 25-27: Rename the PascalCase property WellknownResponse on the
InternalDaVinciConfig interface to camelCase wellknownResponse to match
TypeScript conventions and the equivalent type in davinci-client; update the
property name in InternalDaVinciConfig (extending OidcConfig) and any references
to InternalDaVinciConfig.WellknownResponse so they use wellknownResponse to keep
naming consistent across packages.
🧹 Nitpick comments (6)
package.json (1)
52-52: Consider removing the emptydependenciesblock.If no runtime deps exist, dropping the empty object keeps the root manifest cleaner.
Proposed change
- "dependencies": {},e2e/davinci-app/package.json (1)
19-19: Consider removing the emptydevDependenciesblock.If unused, it’s safe to drop for cleanliness.
Proposed change
- "devDependencies": {},packages/oidc-client/src/lib/wellknown.api.ts (1)
8-38: Use the sharedcreateWellknownSelectorto avoid drift.
The doc comment says it wraps the shared selector, but the code re-implements it. Using the shared helper keeps behavior consistent and removes duplication.🔧 Suggested refactor
-import { createSelector } from '@reduxjs/toolkit'; - import type { RootState } from './client.types.js'; /** * Re-export the shared wellknown RTK Query API from `@forgerock/sdk-oidc`. * * The wellknown API provides OIDC endpoint discovery functionality via * the `.well-known/openid-configuration` endpoint. */ -export { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; +export { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; -// Import locally for use in selector below -import { wellknownApi } from '@forgerock/sdk-oidc'; +// Import locally for use in selector below +import { wellknownApi, createWellknownSelector } from '@forgerock/sdk-oidc'; export function wellknownSelector(wellknownUrl: string, state: RootState) { - const selector = createSelector( - wellknownApi.endpoints.configuration.select(wellknownUrl), - (result) => result?.data, - ); - return selector(state); + const selector = createWellknownSelector(wellknownUrl); + return selector(state); }packages/journey-client/src/lib/wellknown.utils.test.ts (1)
48-61: Consider adding a test case for emptybaseUrl.The
hasWellknownConfigimplementation checksconfig.serverConfig.baseUrl.length > 0, but there's no test verifying this behavior. Consider adding a test to ensure emptybaseUrlreturnsfalse.📝 Suggested additional test
describe('hasWellknownConfig_EmptyBaseUrl_ReturnsFalse', () => { it('should return false when baseUrl is an empty string', () => { const config: JourneyConfigInput = { serverConfig: { baseUrl: '', wellknown: 'https://am.example.com/.well-known/openid-configuration', }, } as AsyncJourneyClientConfig; const result = hasWellknownConfig(config); expect(result).toBe(false); }); });packages/oidc-client/src/lib/authorize.request.ts (1)
22-33: JSDoc type reference is inconsistent with actual parameter type.Line 25 references
{WellKnownResponse}in the JSDoc but the actual parameter type at line 33 isWellknownResponse(lowercase 'k'). Update the JSDoc to match.📝 Proposed fix
/** * `@function` authorizeµ * `@description` Creates an authorization URL for the OIDC client. - * `@param` {WellKnownResponse} wellknown - The well-known configuration for the OIDC server. + * `@param` {WellknownResponse} wellknown - The well-known configuration for the OIDC server. * `@param` {OidcConfig} config - The OIDC client configuration.packages/oidc-client/src/lib/authorize.request.utils.ts (1)
47-59: Minor JSDoc formatting inconsistency.Line 51 is missing a space before the dash in the parameter description:
wellknown-vsconfig-on line 52.📝 Proposed fix
- * `@param` {WellknownResponse} wellknown- The well-known configuration for the OIDC server. - * `@param` { OidcConfig } config- The OIDC client configuration. - * `@param` { GetAuthorizationUrlOptions } options- Optional parameters for the authorization request. + * `@param` {WellknownResponse} wellknown - The well-known configuration for the OIDC server. + * `@param` {OidcConfig} config - The OIDC client configuration. + * `@param` {GetAuthorizationUrlOptions} options - Optional parameters for the authorization request.
| "plugin": "@nx/vitest", | ||
| "options": { | ||
| "testTargetName": "nxTest" | ||
| }, | ||
| "include": ["packages/**/**/*", "e2e/**/**/*", "tools/**/**/*"] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full nx.json file to see targetDefaults structure
cat nx.json | head -200Repository: ForgeRock/ping-javascript-sdk
Length of output: 4536
🏁 Script executed:
# Search for testTargetName and nxTest references
rg -n "testTargetName|nxTest" .Repository: ForgeRock/ping-javascript-sdk
Length of output: 2267
🏁 Script executed:
# Look for test target references in CI workflows
find .github/workflows -type f -name "*.yml" -o -name "*.yaml" | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 200
🏁 Script executed:
# Search for nx test/affected calls in workflows and scripts
rg -n "nx\s+(affected|run-many|test)" .github/workflows/ 2>/dev/null || echo "No matches in workflows"
rg -n "\bnx\s+test\b|\bnx\s+affected.*test" . --type yml --type yaml 2>/dev/null || echo "Checking workflows separately"Repository: ForgeRock/ping-javascript-sdk
Length of output: 292
🏁 Script executed:
# Check for targetDefaults entries for test-related targets
rg -n -A5 '"targetDefaults"' nx.jsonRepository: ForgeRock/ping-javascript-sdk
Length of output: 222
CI workflow will not run vitest tests due to target name mismatch.
The @nx/vitest plugin generates targets named nxTest, but .github/workflows/ci-fork.yml:58 explicitly calls nx affected -t ... test ..., which refers to a target named test that doesn't exist. This means vitest tests won't execute in CI.
Additionally, targetDefaults only includes test, test:watch, and test:coverage with no corresponding nxTest entries, so the generated nxTest targets won't inherit the cache, inputs, or outputs settings defined for test.
Choose one approach:
- Rename
testTargetNametotestand update all package.json test scripts to callnx testinstead ofnx nxTest, OR - Update CI workflow to call
nx affected -t ... nxTest ...and addtargetDefaults.nxTestentries mirroring thetestdefaults (inputs, outputs, cache, dependsOn).
🤖 Prompt for AI Agents
In `@nx.json` around lines 150 - 155, The CI is invoking the wrong target name:
the Vitest plugin configured testTargetName as "nxTest" but the workflow and
targetDefaults use "test", so CI won't run tests and nxTest won't inherit
cache/settings; either change the plugin option testTargetName to "test" and
update package.json scripts that call "nx nxTest" to "nx test", or update the CI
workflow command to use "nxTest" and add corresponding targetDefaults entries
for "nxTest" (copy inputs, outputs, dependsOn, cache settings from the existing
"test" defaults) so the generated nxTest targets run in CI and inherit the same
configuration.
| export interface InternalDaVinciConfig extends OidcConfig { | ||
| wellknownResponse: WellKnownResponse; | ||
| WellknownResponse: WellknownResponse; | ||
| } |
There was a problem hiding this comment.
Property name uses PascalCase instead of camelCase.
The property WellknownResponse on InternalDaVinciConfig uses PascalCase, which is inconsistent with TypeScript conventions and differs from the equivalent property in packages/davinci-client/src/lib/config.types.ts (line 15) which uses wellknownResponse.
🔧 Proposed fix
export interface InternalDaVinciConfig extends OidcConfig {
- WellknownResponse: WellknownResponse;
+ wellknownResponse: WellknownResponse;
}📝 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.
| export interface InternalDaVinciConfig extends OidcConfig { | |
| wellknownResponse: WellKnownResponse; | |
| WellknownResponse: WellknownResponse; | |
| } | |
| export interface InternalDaVinciConfig extends OidcConfig { | |
| wellknownResponse: WellknownResponse; | |
| } |
🤖 Prompt for AI Agents
In `@packages/oidc-client/src/lib/config.types.ts` around lines 25 - 27, Rename
the PascalCase property WellknownResponse on the InternalDaVinciConfig interface
to camelCase wellknownResponse to match TypeScript conventions and the
equivalent type in davinci-client; update the property name in
InternalDaVinciConfig (extending OidcConfig) and any references to
InternalDaVinciConfig.WellknownResponse so they use wellknownResponse to keep
naming consistent across packages.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/journey-client/README.md`:
- Around line 118-123: The example calls journey() with the type name
JourneyClientConfig instead of an actual value; replace that type with a
concrete config value (for example a variable like clientConfig or an inline
config object) that matches the JourneyClientConfig shape and pass that to
journey(config: ...), ensuring the argument is a runtime object rather than the
type name JourneyClientConfig.
🧹 Nitpick comments (1)
packages/journey-client/src/lib/client.store.test.ts (1)
311-332: Consider adding a clarifying comment for the resume behavior difference.The test at line 311 uses
foo=barURL params which does not trigger storage lookup, while the test at line 232 usescode=123&state=abcwhich does. This behavior difference is a key aspect of the resume logic but isn't explicitly documented in the test.Consider adding a comment explaining that OIDC-specific params (
code,state) indicate a redirect callback requiring the stored step, while arbitrary params indicate a fresh start with query parameters.📝 Suggested clarification
test('resume_NoPreviousStepRequired_CallsStartWithUrlParams', async () => { // Arrange mockStorageInstance.get.mockResolvedValue(undefined); const mockStepResponse: Step = { authId: 'test-auth-id', callbacks: [] }; setupMockFetch(mockStepResponse); // Act const result = await journey({ config: mockConfig }); if (!isJourneyClient(result)) throw new Error('Expected client'); + // URL without OIDC params (code/state) doesn't require previous step from storage const resumeUrl = 'https://app.com/callback?foo=bar'; const step = await result.resume(resumeUrl, {});
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Re-implement well known configuration and abstract into reusable package - Remove internal re-export file (wellknown.api.ts) - Import directly from source packages in internal modules - Keep consumer-facing exports in types.ts as the single public API - Add IPv6 localhost comment explaining design decision - Remove redundant inline comments (JSDoc is sufficient)
Re-implement well known configuration and abstract into reusable package - Remove internal re-export file (wellknown.api.ts) - Import directly from source packages in internal modules - Keep consumer-facing exports in types.ts as the single public API - Add IPv6 localhost comment explaining design decision - Remove redundant inline comments (JSDoc is sufficient) [Self-Healing CI Rerun]
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Actionable comments posted: 2
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/src/lib/wellknown.api.ts (1)
8-38:⚠️ Potential issue | 🟠 MajorFix memoization loss and correct the misleading docstring.
The selector is recreated on every function call, defeating memoization. Additionally, the docstring incorrectly claims the function "wraps the shared
createWellknownSelector" when it doesn't use it at all. Use the RTK Query selector directly and extract only the data field:♻️ Corrected implementation
-import { createSelector } from '@reduxjs/toolkit'; - // Import locally for use in selector below import { wellknownApi } from '@forgerock/sdk-oidc'; @@ /** * Selector to retrieve the cached well-known response from Redux state. * - * This is a convenience function that wraps the shared createWellknownSelector - * for easier use with oidc-client's RootState type. + * Extracts just the data field from the cached query result. * * `@param` wellknownUrl - The well-known endpoint URL used as the cache key * `@param` state - The Redux root state * `@returns` The cached WellknownResponse or undefined if not yet fetched */ export function wellknownSelector(wellknownUrl: string, state: RootState) { - const selector = createSelector( - wellknownApi.endpoints.configuration.select(wellknownUrl), - (result) => result?.data, - ); - return selector(state); + return wellknownApi.endpoints.configuration.select(wellknownUrl)(state)?.data; }
🤖 Fix all issues with AI agents
In `@e2e/journey-app/main.ts`:
- Around line 58-70: Guard against errorEl being null before using its
textContent: after obtaining errorEl (the element used to display init errors)
add a null check and handle the missing element (e.g., log an error and return
or create/fallback element) so the early-return path that dereferences
errorEl.textContent won't throw; ensure this check occurs before the block that
logs journeyClientResult.message and before assigning journeyClient (the
narrowed const) so error display is safe in the error branch.
In `@packages/journey-client/src/lib/client.store.ts`:
- Around line 160-169: The stored config.middleware in
InternalJourneyClientConfig is never used during request execution; update the
initialization to ensure user-provided middleware is applied by merging
config.middleware into the requestMiddleware chain passed to createJourneyStore
(or by reading resolvedConfig.middleware when composing thunk extraArgument).
Specifically, modify the code paths around journey() / createJourneyStore and
the thunk extraArgument where requestMiddleware is constructed so that it
includes the resolvedConfig.middleware (or merge config.middleware into the
existing requestMiddleware array) before creating the store and before any
requests are dispatched.
| const journeyClientResult = await journey({ config: config, requestMiddleware }); | ||
| if (!isJourneyClient(journeyClientResult)) { | ||
| console.error('Failed to initialize journey client:', journeyClientResult.message); | ||
| errorEl.textContent = journeyClientResult.message ?? 'Unknown error'; | ||
| return; | ||
| } | ||
| /** | ||
| * Re-assign to a new const after type narrowing. | ||
| * TypeScript's type narrowing doesn't persist into closures (event handlers, callbacks) | ||
| * because it can't prove the variable wasn't reassigned between the guard and closure execution. | ||
| * Creating a new const binding after the guard preserves the narrowed type for nested functions. | ||
| */ | ||
| const journeyClient = journeyClientResult; |
There was a problem hiding this comment.
Guard errorEl before dereferencing.
document.getElementById can return null; the new early-error path would throw before showing the initialization error.
🛡️ Suggested fix
if (!isJourneyClient(journeyClientResult)) {
console.error('Failed to initialize journey client:', journeyClientResult.message);
- errorEl.textContent = journeyClientResult.message ?? 'Unknown error';
+ if (errorEl) {
+ errorEl.textContent = journeyClientResult.message ?? 'Unknown error';
+ }
return;
}📝 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.
| const journeyClientResult = await journey({ config: config, requestMiddleware }); | |
| if (!isJourneyClient(journeyClientResult)) { | |
| console.error('Failed to initialize journey client:', journeyClientResult.message); | |
| errorEl.textContent = journeyClientResult.message ?? 'Unknown error'; | |
| return; | |
| } | |
| /** | |
| * Re-assign to a new const after type narrowing. | |
| * TypeScript's type narrowing doesn't persist into closures (event handlers, callbacks) | |
| * because it can't prove the variable wasn't reassigned between the guard and closure execution. | |
| * Creating a new const binding after the guard preserves the narrowed type for nested functions. | |
| */ | |
| const journeyClient = journeyClientResult; | |
| const journeyClientResult = await journey({ config: config, requestMiddleware }); | |
| if (!isJourneyClient(journeyClientResult)) { | |
| console.error('Failed to initialize journey client:', journeyClientResult.message); | |
| if (errorEl) { | |
| errorEl.textContent = journeyClientResult.message ?? 'Unknown error'; | |
| } | |
| return; | |
| } | |
| /** | |
| * Re-assign to a new const after type narrowing. | |
| * TypeScript's type narrowing doesn't persist into closures (event handlers, callbacks) | |
| * because it can't prove the variable wasn't reassigned between the guard and closure execution. | |
| * Creating a new const binding after the guard preserves the narrowed type for nested functions. | |
| */ | |
| const journeyClient = journeyClientResult; |
🤖 Prompt for AI Agents
In `@e2e/journey-app/main.ts` around lines 58 - 70, Guard against errorEl being
null before using its textContent: after obtaining errorEl (the element used to
display init errors) add a null check and handle the missing element (e.g., log
an error and return or create/fallback element) so the early-return path that
dereferences errorEl.textContent won't throw; ensure this check occurs before
the block that logs journeyClientResult.message and before assigning
journeyClient (the narrowed const) so error display is safe in the error branch.
| // Step 5: Build the resolved internal configuration | ||
| const resolvedConfig: InternalJourneyClientConfig = { | ||
| serverConfig: { | ||
| baseUrl: normalizeBaseUrl(resolvedBaseUrl), | ||
| paths, | ||
| timeout, | ||
| }, | ||
| realmPath: resolvedRealm, | ||
| middleware: config.middleware, | ||
| wellknownResponse, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the structure and find the client.store.ts file
find . -name "client.store.ts" -type fRepository: ForgeRock/ping-javascript-sdk
Length of output: 219
🏁 Script executed:
# Search for middleware usage in request execution
rg -n "middleware" packages/journey-client/src/lib/client.store.ts -A 2 -B 2Repository: ForgeRock/ping-javascript-sdk
Length of output: 776
🏁 Script executed:
# Search for usage of config.middleware
rg -n "config\.middleware|resolvedConfig\.middleware|resolved.*middleware" packages/journey-client -g '**/*.ts' -A 2Repository: ForgeRock/ping-javascript-sdk
Length of output: 291
🏁 Script executed:
# Look for request execution logic
rg -n "executeRequest|sendRequest|request\(" packages/journey-client/src/lib/client.store.ts -A 3Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Search for where resolvedConfig is returned or used
rg -n "resolvedConfig" packages/journey-client/src/lib/client.store.ts -A 3 -B 3Repository: ForgeRock/ping-javascript-sdk
Length of output: 599
🏁 Script executed:
# Look for request handling in the entire journey-client
rg -n "middleware" packages/journey-client/src -g '**/*.ts' | head -30Repository: ForgeRock/ping-javascript-sdk
Length of output: 1611
🏁 Script executed:
# Find the constructor to see what parameters are passed
rg -n "constructor|export.*function" packages/journey-client/src/lib/client.store.ts -A 5Repository: ForgeRock/ping-javascript-sdk
Length of output: 429
🏁 Script executed:
# Search for request execution across the client module
fd . packages/journey-client/src -name '*.ts' -type f | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 303
🏁 Script executed:
# Read the full middleware setup in client.store.utils.ts
cat -n packages/journey-client/src/lib/client.store.utils.ts | sed -n '20,50p'Repository: ForgeRock/ping-javascript-sdk
Length of output: 861
🏁 Script executed:
# Search for all references to InternalJourneyClientConfig or how it's used
rg -n "InternalJourneyClientConfig" packages/journey-client/src -A 2 -B 2Repository: ForgeRock/ping-javascript-sdk
Length of output: 2604
🏁 Script executed:
# Check journey.api.ts to see how requests are made and middleware is applied
rg -n "middleware|request" packages/journey-client/src/lib/journey.api.ts -A 1 -B 1 | head -40Repository: ForgeRock/ping-javascript-sdk
Length of output: 1599
🏁 Script executed:
# Check if resolvedConfig.middleware is ever accessed
rg -n "\.middleware\s*\)" packages/journey-client/src -g '**/*.ts'Repository: ForgeRock/ping-javascript-sdk
Length of output: 267
🏁 Script executed:
# Search for any access to config.middleware from the stored state
rg -n "state\.config\.middleware|config\.middleware" packages/journey-client/src -g '**/*.ts'Repository: ForgeRock/ping-javascript-sdk
Length of output: 159
🏁 Script executed:
# Check if there are any selectors that access the middleware from config
rg -n "selector|useSelector" packages/journey-client/src -A 3 -B 1 | grep -i "middleware\|config" -A 2 -B 2Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Verify the full InternalJourneyClientConfig type definition
cat -n packages/journey-client/src/lib/config.types.ts | sed -n '50,75p'Repository: ForgeRock/ping-javascript-sdk
Length of output: 870
config.middleware is never applied to requests.
config.middleware is stored in the internal config (line 168) but is never accessed during request execution. The journey API uses requestMiddleware from the store's thunk extraArgument, which comes from the journey() function parameter—not from the stored config. User-provided middleware in the config object is silently ignored. Either merge config.middleware into the requestMiddleware chain passed to createJourneyStore, or retrieve and apply it from the stored config when making requests.
🤖 Prompt for AI Agents
In `@packages/journey-client/src/lib/client.store.ts` around lines 160 - 169, The
stored config.middleware in InternalJourneyClientConfig is never used during
request execution; update the initialization to ensure user-provided middleware
is applied by merging config.middleware into the requestMiddleware chain passed
to createJourneyStore (or by reading resolvedConfig.middleware when composing
thunk extraArgument). Specifically, modify the code paths around journey() /
createJourneyStore and the thunk extraArgument where requestMiddleware is
constructed so that it includes the resolvedConfig.middleware (or merge
config.middleware into the existing requestMiddleware array) before creating the
store and before any requests are dispatched.
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4665
Description
Add support for well known endpoint
Summary by CodeRabbit
Release Notes
New Features
isJourneyClienttype guard for safer client initializationBug Fixes
Breaking Changes
wellknownURL;baseUrlis automatically inferredJourneyClientConfiginterface restructured withserverConfignestingWellKnownResponse→WellknownResponse