SDKS-5067: Standardize SDK Configuration#118
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (35)
📝 WalkthroughWalkthroughThree React todo sample apps (davinci, journey, oidc) migrate OIDC/SDK configuration from individual environment variables ( ChangesSDK Config Migration to config.json
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
We should update reactjs-todo-login-widget's configuration too
SteinGabriel
force-pushed
the
SDKS-5067
branch
6 times, most recently
from
363e5dc to
93a2e26
Compare
The login widget app uses |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
javascript/reactjs-todo-davinci/client/context/oidc.context.js (1)
34-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
jsonin useEffect dependency array.The
useEffectat line 34 uses thejsonparameter insideinitOidcClient()(line 48) but omitsjsonfrom the dependency array at line 63. This breaks the React hook contract: ifjsonchanges, the OIDC client will not be re-initialized with the new configuration. Thereactjs-todo-journeyversion of this file correctly includes[json, oidcClient].[major]
🔧 Proposed fix: Add `json` to the dependency array
}, [oidcClient]); + }, [json, oidcClient]);🤖 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 `@javascript/reactjs-todo-davinci/client/context/oidc.context.js` around lines 34 - 63, The useEffect hook starting at line 34 uses the json parameter inside the initOidcClient function when calling makeOidcConfig(json), but json is missing from the dependency array at line 63. Add json to the dependency array alongside oidcClient so that when json changes, the OIDC client will be re-initialized with the new configuration, ensuring the React hook contract is followed.
🤖 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 `@javascript/reactjs-todo-davinci/client/constants.js`:
- Line 29: The rawConfig constant assignment does not handle errors when parsing
process.env.SDK_CONFIG, which will cause the entire module to fail loading if
the JSON is malformed. Wrap the JSON.parse call in a try-catch block, and if
parsing throws an error, fall back to using sdkConfigJson as the default value.
This ensures the application can still initialize even when SDK_CONFIG contains
invalid JSON.
In `@javascript/reactjs-todo-davinci/playwright.config.ts`:
- Line 3: The import statement for testFidoConfig is triggering a no-unused-vars
lint error because the variable is only referenced in commented code and not
actively used. Either remove the import statement for testFidoConfig entirely if
it is not needed, or uncomment and properly integrate the code that uses
testFidoConfig so the variable is actively referenced in the configuration.
In `@javascript/reactjs-todo-davinci/README.md`:
- Around line 65-70: The README.md file contains an inconsistency in the
redirect URI configuration examples, showing
https://localhost:8443/callback.html in one location and
http://localhost:8443/callback.html in another. Update the redirect URI in the
OIDC configuration example to use the same protocol (http or https) consistently
throughout the entire README file to prevent configuration errors during user
onboarding.
In `@javascript/reactjs-todo-journey/client/constants.js`:
- Around line 31-33: The JSON parsing of environment variables can cause module
load-time crashes if the JSON is malformed. In
javascript/reactjs-todo-journey/client/constants.js lines 31-33, wrap the
JSON.parse call for import.meta.env.VITE_SDK_CONFIG in a try/catch block and
fallback to sdkConfigJson on parse failure. Apply the same guarded
parse/fallback pattern in javascript/reactjs-todo-oidc/client/constants.js line
28 for process.env.SDK_CONFIG to ensure both modules gracefully handle invalid
JSON without crashing at load time.
- Around line 35-40: The CONFIG object in both files unconditionally overwrites
the redirectUri property, ignoring any pre-configured value from rawConfig. In
javascript/reactjs-todo-journey/client/constants.js at lines 35-40, modify the
oidc.redirectUri assignment to conditionally use the value from
rawConfig.oidc.redirectUri when it exists, falling back to the
window.location.origin-based URL only when it is missing or undefined. Apply the
identical conditional fallback pattern to
javascript/reactjs-todo-oidc/client/constants.js at lines 30-35 so that any
configured redirect URI in rawConfig is preserved instead of being clobbered.
In `@javascript/reactjs-todo-journey/vite.config.js`:
- Line 44: The current code uses new URL(...).pathname to construct a file path
for the config.json file, which is unreliable on Windows and other platforms due
to leading slashes and unencoded characters. Import the fileURLToPath function
from the built-in url module and replace the pathname-based approach in the
optionalConfigJson call with fileURLToPath(new URL('./config.json',
import.meta.url)) to ensure the path works correctly across all platforms when
checked by existsSync.
---
Outside diff comments:
In `@javascript/reactjs-todo-davinci/client/context/oidc.context.js`:
- Around line 34-63: The useEffect hook starting at line 34 uses the json
parameter inside the initOidcClient function when calling makeOidcConfig(json),
but json is missing from the dependency array at line 63. Add json to the
dependency array alongside oidcClient so that when json changes, the OIDC client
will be re-initialized with the new configuration, ensuring the React hook
contract is followed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 809bdce5-7376-4f24-8136-1a1fa2a83af3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
javascript/reactjs-todo-davinci/.env.examplejavascript/reactjs-todo-davinci/.gitignorejavascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/components/davinci-client/hooks/create-client.utils.jsjavascript/reactjs-todo-davinci/client/constants.jsjavascript/reactjs-todo-davinci/client/context/oidc.context.jsjavascript/reactjs-todo-davinci/config.example.jsonjavascript/reactjs-todo-davinci/config.test.fido.jsonjavascript/reactjs-todo-davinci/config.test.jsonjavascript/reactjs-todo-davinci/package.jsonjavascript/reactjs-todo-davinci/playwright.config.tsjavascript/reactjs-todo-davinci/webpack.config.jsjavascript/reactjs-todo-journey/.env.examplejavascript/reactjs-todo-journey/.gitignorejavascript/reactjs-todo-journey/README.mdjavascript/reactjs-todo-journey/client/components/journey/journey.hook.jsjavascript/reactjs-todo-journey/client/constants.jsjavascript/reactjs-todo-journey/client/context/oidc.context.jsjavascript/reactjs-todo-journey/config.example.jsonjavascript/reactjs-todo-journey/config.test.jsonjavascript/reactjs-todo-journey/e2e/embedded-login.spec.jsjavascript/reactjs-todo-journey/package.jsonjavascript/reactjs-todo-journey/playwright.config.jsjavascript/reactjs-todo-journey/vite.config.jsjavascript/reactjs-todo-oidc/.env.examplejavascript/reactjs-todo-oidc/.gitignorejavascript/reactjs-todo-oidc/README.mdjavascript/reactjs-todo-oidc/client/README.mdjavascript/reactjs-todo-oidc/client/constants.jsjavascript/reactjs-todo-oidc/client/context/oidc.context.jsjavascript/reactjs-todo-oidc/config.example.jsonjavascript/reactjs-todo-oidc/config.test.pingam.jsonjavascript/reactjs-todo-oidc/config.test.pingone.jsonjavascript/reactjs-todo-oidc/package.jsonjavascript/reactjs-todo-oidc/playwright.config.tsjavascript/reactjs-todo-oidc/webpack.config.js
You're right, my bad. Login widget is unrelated here. |
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
It's great that we are explaining how to configure things the new way. We should continue to explain in the README that this unified config is optional. We should do this for all three sample apps: davinci, journey, oidc. This way the sample app consumers (humans / ai agents) know that configuration works both ways and they can use either, and that this is backward compatible.
Good point. I added a disclaimer saying the new config is optional. |
feat(reactjs-todo-oidc): migrate SDK config to config.json feat(reactjs-todo-journey): migrate SDK config to config.json feat(sample-apps): inject SDK config via env var for e2e, fall back to config.json fix(reactjs-todo-davinci): migrate fido playwright block to unified config fix(reactjs-todo-journey): replace deprecated assert with import attribute fix(reactjs-todo-oidc): remove REST_OAUTH vars from env.example, add CI clarification docs: update README setup sections to reflect config.json migration feat(reactjs-todo-davinci): wire make*Config from sdk-utilities feat(reactjs-todo-oidc): wire makeOidcConfig from sdk-utilities feat(reactjs-todo-journey): wire make*Config from sdk-utilities fix(sample-apps): derive redirectUri dynamically from window.location.origin fix(reactjs-todo-davinci): guard SDK_CONFIG JSON.parse against malformed input fix(reactjs-todo-davinci): remove unused testFidoConfig import fix(reactjs-todo-davinci): align README redirect URI and CORS origins to https fix(reactjs-todo-journey,reactjs-todo-oidc): guard SDK_CONFIG JSON.parse against malformed input fix(reactjs-todo-journey,reactjs-todo-oidc): preserve oidc.redirectUri from config when present fix(reactjs-todo-journey): use fileURLToPath instead of .pathname in vite config fix(reactjs-todo-davinci): add json to useEffect dependency array in oidc context docs: add backward-compat note to SDK credentials section in all three READMEs test(reactjs-todo-journey): remove unnecessary afterEach cleanup hook
|
Note: CI checks are failing because |
Sounds good. There are already such PRs waiting for 2.1 to be released. We can label the PR as |
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Tested locally, all three oidc, journey, and davinci work as expected, looks great!
2 participants
Summary
https://pingidentity.atlassian.net/browse/SDKS-5067
Migrates SDK credentials out of environment variables and into a per-app
config.jsonfile using the unified SDK configuration schema. All threeJavaScript sample apps (reactjs-todo-davinci, reactjs-todo-oidc,
reactjs-todo-journey) now accept a structured JSON config object detected and
mapped to internal config automatically by the factory discriminator.
Changes
reactjs-todo-davinciconstants.js— replacesWEB_OAUTH_CLIENT/SCOPE/WELLKNOWN_URLenvvars with
CONFIGimported fromconfig.json; falls back toSDK_CONFIGenv var (JSON string) for e2e/CI
config.example.json— new committed template for the unified SDK configshape
config.test.json/config.test.fido.json— committed test fixturesreplacing hardcoded Playwright env vars
playwright.config.ts— usesSDK_CONFIG: JSON.stringify(testConfig)instead of individual credential vars
webpack.config.js— removesWEB_OAUTH_CLIENT/SCOPE/WELLKNOWN_URL;injects
SDK_CONFIG.env.example— removes SDK credential vars; adds migration note.gitignore— addsconfig.jsonreactjs-todo-oidcconstants.js— same pattern as davinci; preservesSERVERenv var(PingAM/PingOne display-name logic)
config.example.json— new committed templateconfig.test.pingam.json/config.test.pingone.json— per-server testfixtures
playwright.config.ts— usesSDK_CONFIGper project configwebpack.config.js— same removals/additions as davinci.env.example,.gitignore,README.md,client/README.md— updatedreactjs-todo-journeyconstants.js— Vite JSON import (import.meta.env.VITE_SDK_CONFIGoverride for CI)
config.example.json— new committed templateconfig.test.json— committed test fixtureplaywright.config.js— usesVITE_SDK_CONFIG: JSON.stringify(testConfig).env.example,.gitignore,README.md— updatedTests
SDK_CONFIG/VITE_SDK_CONFIGinjection patternpre-existing
babel-loader/validateOptionsfailure atbc9ebefunrelatedto this change)
How to test
1. Local dev setup
Copy
config.example.json→config.jsonin each app root, fill inclientId,discoveryEndpoint,scopes,redirectUri. Copy.env.example→
.env, fill remaining runtime vars. Runnpm run start:reactjs-todo-dv(or journey/oidc variant) from
javascript/. Verify auth flow completes.2. E2E run
Run
npm run e2e --workspace reactjs-todo-davinci,--workspace reactjs-todo-journey, and--workspace reactjs-todo-oidcfromjavascript/. All suites should pass. Confirmconfig.jsonis not committed(gitignored).
3. Verify backward-compat
Confirm no
WEB_OAUTH_CLIENT,SCOPE, orWELLKNOWN_URL(orVITE_variants) remain in
.env.example,webpack.config.js, or Playwrightconfigs.
Summary by CodeRabbit
Release Notes
Documentation
.envexamples to focus only on runtime settings (API URL, debug flags, port).Chores