fix(reactjs-todo-login-widget): add the required journey client wellknown configuration to login widget sample app#115
fix(reactjs-todo-login-widget): add the required journey client wellknown configuration to login widget sample app#115vatsalparikh wants to merge 2 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request adds WELLKNOWN_URL environment configuration to the React login widget. The URL is documented in environment examples, threaded through the Webpack build system via DefinePlugin, exported as a constant, imported into the widget, and configured as the journeyClient OIDC discovery endpoint. Test environments receive the same configuration. ChangesWELLKNOWN_URL Environment and Widget Integration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against 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-login-widget/client/index.js`:
- Around line 70-75: The wellknown OIDC discovery URL in
journeyClient.serverConfig currently always appends "/realms/${REALM_PATH}",
producing a duplicated segment when REALM_PATH === "root"; update the URL
construction in journeyClient.serverConfig.wellknown so that if REALM_PATH is
"root" you omit the "/realms/${REALM_PATH}" segment (i.e., use
`${SERVER_URL}oauth2/.well-known/openid-configuration` for root, otherwise
`${SERVER_URL}oauth2/realms/${REALM_PATH}/.well-known/openid-configuration`),
locate this change at the journeyClient.serverConfig.wellknown assignment in
client/index.js and implement the conditional string construction accordingly.
🪄 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: 34319d26-a606-4d22-ad5a-331ca0001769
📒 Files selected for processing (1)
javascript/reactjs-todo-login-widget/client/index.js
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Thanks for doing this. Left some suggestions. How did we get e2e passing before if journey wasn't being picked up? Do we need to add another test?
| theme.mode === 'dark' ? 'cstm_login-link_dark' : '' | ||
| }`} | ||
| onClick={openModal} | ||
| onClick={() => openModal(searchParams.get('journey'))} |
There was a problem hiding this comment.
Could we pass all the params here and extract them in openModal so that if we add more params later we don't have to update this? Something like this:
https://github.com/ForgeRock/sdk-sample-apps/blob/main/javascript/reactjs-todo-journey/client/components/layout/header.jsx#L36-L41
There was a problem hiding this comment.
Thanks, done, makes sense!
| // Required by Journey Client v2: OIDC discovery URL for the realm | ||
| journeyClient: { | ||
| serverConfig: { | ||
| wellknown: `${SERVER_URL}oauth2/realms/root/realms/${REALM_PATH}/.well-known/openid-configuration`, |
There was a problem hiding this comment.
In the other sample apps we have a WELLKNOWN_URL variable and the sdk derives the server url and realm path from this automatically. Let's keep parity here and remove any env variables that aren't needed anymore.
There was a problem hiding this comment.
Right, moved the wellknown url derivation from index.js to .env. Now, index.js only reads the variable defined in .env file.
…nd journey name URL param support
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 `@javascript/reactjs-todo-login-widget/client/index.js`:
- Around line 56-75: The comment claiming "NO journeyClient — never set, ever"
is contradictory because the config object sets
journeyClient.serverConfig.wellknown; remove or update the misleading comment
and ensure it accurately reflects the configuration in configuration().set —
specifically edit the comment above the configuration block (near
configuration()) to either delete the "NO journeyClient" remark or replace it
with a correct note that journeyClient.serverConfig.wellknown is intentionally
set, so future readers understand that journeyClient is configured.
🪄 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: 68a8570c-737f-4647-99a0-add6b6717f29
📒 Files selected for processing (8)
javascript/reactjs-todo-login-widget/.env.examplejavascript/reactjs-todo-login-widget/client/components/layout/header.jsjavascript/reactjs-todo-login-widget/client/constants.jsjavascript/reactjs-todo-login-widget/client/hooks/useLoginWidget.jsjavascript/reactjs-todo-login-widget/client/index.jsjavascript/reactjs-todo-login-widget/e2e/journey-url-param.spec.jsjavascript/reactjs-todo-login-widget/playwright.config.tsjavascript/reactjs-todo-login-widget/webpack.config.js
✅ Files skipped from review due to trivial changes (1)
- javascript/reactjs-todo-login-widget/.env.example
SteinGabriel
left a comment
SteinGabriel
left a comment
There was a problem hiding this comment.
Other than the leftover comment pointed out by CodeRabbit (would be better to remove it, but not a blocker), this seems ready to be merged.
Thanks!
Your point is valid, however, I am now realizing that we never intended to support multiple journeys in login widget. SIgn Up field was disabled and mentioned in the ticket when creating this sample app: https://pingidentity.atlassian.net/browse/SDKS-4765 So I am thinking supporting multiple journeys is not expected, nor desired in this sample app. It's probably a feature to add later on, so while I had added tests and the ability to specify multiple journeys in a commit, I pushed another commit that does not have those things. I think right now, we only need wellknown configuration for this sample app, as it only supports the login journey and nothing else. Oh, and we already support custom journeys through the .env variable Anyway, I think wellknown changes are good enough for this PR so that this app continues to work when we move to the new version of login widget. |
…ort custom journeys through JOURNEY_LOGIN env variable
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 `@javascript/reactjs-todo-login-widget/client/index.js`:
- Around line 53-55: Remove the URL query-param override by deleting the
URLSearchParams/window.location.search usage and the journeyParam variable; set
tree directly to JOURNEY_LOGIN instead of using journeyParam or any conditional
based on it. Specifically, remove the const urlParams = new
URLSearchParams(...), const journeyParam = urlParams.get('journey'), and any
code that assigns or overrides tree from journeyParam, and replace that logic so
tree = JOURNEY_LOGIN is assigned unconditionally (review occurrences around the
symbols URLSearchParams, journeyParam, tree, and JOURNEY_LOGIN).
🪄 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: 6ee44216-3caf-4c93-8482-fe5b6abb1aec
📒 Files selected for processing (1)
javascript/reactjs-todo-login-widget/client/index.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@javascript/reactjs-todo-login-widget/client/index.js`:
- Around line 53-55: Remove the URL query-param override by deleting the
URLSearchParams/window.location.search usage and the journeyParam variable; set
tree directly to JOURNEY_LOGIN instead of using journeyParam or any conditional
based on it. Specifically, remove the const urlParams = new
URLSearchParams(...), const journeyParam = urlParams.get('journey'), and any
code that assigns or overrides tree from journeyParam, and replace that logic so
tree = JOURNEY_LOGIN is assigned unconditionally (review occurrences around the
symbols URLSearchParams, journeyParam, tree, and JOURNEY_LOGIN).
🪄 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: 6ee44216-3caf-4c93-8482-fe5b6abb1aec
📒 Files selected for processing (1)
javascript/reactjs-todo-login-widget/client/index.js
🛑 Comments failed to post (1)
javascript/reactjs-todo-login-widget/client/index.js (1)
53-55:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove URL query-param journey override to match agreed PR scope.
journeyis still read fromwindow.location.searchand used to overridetree. This keeps query-param journey selection enabled, but the PR scope/comments explicitly narrowed behavior to useJOURNEY_LOGINonly. Please removejourneyParamusage and settreedirectly fromJOURNEY_LOGIN.Also applies to: 73-73
🤖 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-login-widget/client/index.js` around lines 53 - 55, Remove the URL query-param override by deleting the URLSearchParams/window.location.search usage and the journeyParam variable; set tree directly to JOURNEY_LOGIN instead of using journeyParam or any conditional based on it. Specifically, remove the const urlParams = new URLSearchParams(...), const journeyParam = urlParams.get('journey'), and any code that assigns or overrides tree from journeyParam, and replace that logic so tree = JOURNEY_LOGIN is assigned unconditionally (review occurrences around the symbols URLSearchParams, journeyParam, tree, and JOURNEY_LOGIN).
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4796
Explanation
While doing QA for https://pingidentity.atlassian.net/browse/SDKS-4703, I found two issues in the sample app:
?journey=URL query parameter was not being passed to the login widget.openModalnow reads thejourneysearch param and forwards it tojourneyEvents.start(), allowing callers to specify a journey via URL (e.g./?journey=MyJourney).Summary by CodeRabbit
Chores
Refactor