fix(reactjs-todo-oidc): support P1 user responses and align sample apps#110
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughDocs and .env templates were standardized, SDK/documentation links updated, Protect initialization and related context were removed, centralized OIDC login was refactored into a memoized authorize flow with a single loading message, Playwright configs added dual servers (PingAM + PingOne), and E2E tests/demo-user exports were adjusted for dynamic display names/credentials. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ClientApp
participant AuthServer
participant TokenEndpoint
participant UserInfo
Browser->>ClientApp: Click "Sign In"
ClientApp->>AuthServer: Redirect /authorize (client_id, redirect_uri, state, code_challenge)
AuthServer-->>Browser: Show sign-in form
Browser->>AuthServer: Submit credentials -> returns authorization code
Browser->>ClientApp: Redirect back with code & state
ClientApp->>TokenEndpoint: Exchange code for tokens
TokenEndpoint-->>ClientApp: Return access_token / id_token
ClientApp->>UserInfo: Request userinfo with access_token
UserInfo-->>ClientApp: Return user claims (displayName, email)
ClientApp-->>Browser: Set auth state and render protected UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. Review rate limit: 0/1 reviews remaining, refill in 40 minutes and 14 seconds.Comment |
![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
🧹 Nitpick comments (3)
javascript/reactjs-todo-davinci/client/context/README.md (1)
5-5: Polish the wording for readability.Line 5 reads a bit awkwardly. Consider phrasing it as “If global state management becomes more complex, Redux might be a better option.”
✏️ Suggested doc tweak
-If global state becomes a more complex function of the app, something like Redux might be a better option. +If global state management becomes more complex, something like Redux might be a better option.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-davinci/client/context/README.md` at line 5, Replace the awkward sentence "If global state becomes a more complex function of the app, something like Redux might be a better option." in the README.md with clearer wording; change it to "If global state management becomes more complex, Redux might be a better option." to improve readability and explicitly reference global state management.javascript/reactjs-todo-oidc/e2e/utils/demo-user.js (1)
8-14: Prefer environment-driven E2E credentials over hardcoded values.This works functionally, but storing credentials directly in git increases security/maintenance risk. Consider resolving from
process.envwith fallbacks for local demo runs.🔐 Suggested refactor
-export const amUsername = 'JsAmSampleAppsE2E@user.com'; -export const amPassword = 'Demo_12345!'; -export const amDisplayName = 'Demo User'; +export const amUsername = process.env.E2E_AM_USERNAME ?? 'JsAmSampleAppsE2E@user.com'; +export const amPassword = process.env.E2E_AM_PASSWORD ?? 'Demo_12345!'; +export const amDisplayName = process.env.E2E_AM_DISPLAY_NAME ?? 'Demo User'; -export const p1Username = 'JsDvSampleAppsE2E@user.com'; -export const p1Password = 'Demo_12345!'; -export const p1DisplayName = 'JS DaVinci Sample Apps E2E'; +export const p1Username = process.env.E2E_P1_USERNAME ?? 'JsDvSampleAppsE2E@user.com'; +export const p1Password = process.env.E2E_P1_PASSWORD ?? 'Demo_12345!'; +export const p1DisplayName = + process.env.E2E_P1_DISPLAY_NAME ?? 'JS DaVinci Sample Apps E2E';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/e2e/utils/demo-user.js` around lines 8 - 14, Replace the hardcoded E2E credentials (amUsername, amPassword, amDisplayName, p1Username, p1Password, p1DisplayName) so they are resolved from environment variables with sensible local defaults; e.g., read process.env.AM_USERNAME || 'JsAmSampleAppsE2E@user.com' for amUsername, process.env.AM_PASSWORD || 'Demo_12345!' for amPassword, process.env.AM_DISPLAY_NAME || 'Demo User' and similarly use P1_USERNAME, P1_PASSWORD, P1_DISPLAY_NAME for the p1* exports; ensure the exported constants remain the same names so callers don't change, and document the new env var names in a comment or README for CI usage.javascript/reactjs-todo-oidc/client/README.md (1)
30-30: Use “username” as one word for consistency.Small docs polish: “user name” → “username”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/client/README.md` at line 30, Update the README line that documents SERVER to use "username" as one word instead of "user name"; find the line containing the text "`SERVER` is used to derive the user name from either PingAM or PingOne" and replace "user name" with "username" so it reads that SERVER is used to derive the "username" from either PingAM or PingOne.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@javascript/reactjs-todo-journey/client/views/login.jsx`:
- Around line 56-67: The token exchange and user info async calls
(oidcClient.token.exchange and oidcClient.user.info) currently only inspect
returned objects for an error field but do not catch rejected promises; wrap
both calls in try/catch blocks so any thrown error is caught, call
setLoadingMessage('Sign in failed. Please try again.') in the catch, and log the
full error with console.error (e.g., include context like "Token exchange error"
or "Error getting user") before returning; apply the same try/catch pattern to
the analogous calls around the token/user flow later in the file (the blocks
handling tokenResponse and user).
In `@javascript/reactjs-todo-oidc/.env.example`:
- Around line 2-9: Reorder the environment keys in .env.example into the
canonical/alphabetical order expected by dotenv-linter and add a trailing
newline at the end of the file; specifically ensure the variables PORT,
DEBUGGER_OFF, DEVELOPMENT, WEB_OAUTH_CLIENT, SCOPE, WELLKNOWN_URL, and SERVER
are sorted per the linter’s order (e.g., alphabetical) and append a final
newline character so the file ends with a blank line to eliminate the lint
warnings.
In `@javascript/reactjs-todo-oidc/client/views/login.js`:
- Around line 69-80: The effect inside useEffect (function
handleCentralizedLogin) can call authorize(codeParam, stateParam) multiple
times; add a guard to ensure a given callback is processed once by tracking a
processed flag or the last processed code (e.g., a useRef like
processedCallbackRef) and checking it before calling authorize, or clear/remove
codeParam/stateParam from the URL once handled; ensure the guard is referenced
in the same effect that calls authorize and also applied where similar logic
runs around line 109 so duplicate token exchanges are prevented.
- Around line 103-105: The catch block that currently only logs the error (the
catch handling "Error: centralized login; ${error}") should also update the UI
state to stop the loading indicator and display a failure message; replace or
augment the console.error in the catch with calls to the component's state
setters (e.g., setLoading(false) and setMessage('Login failed') or
setErrorMessage(error.message) — or whatever state setters are used in this file
— so the stale loading text is cleared and a clear failure message is shown to
the user.
In `@javascript/reactjs-todo-oidc/playwright.config.ts`:
- Around line 57-60: Replace hardcoded PingOne tenant/client identifiers in the
Playwright config by reading them from environment variables and validate
presence at startup: for the constants SCOPE, WEB_OAUTH_CLIENT,
REST_OAUTH_CLIENT and the issuer URL, change assignment to use process.env
(e.g., process.env.PINGONE_ISSUER, PINGONE_SCOPE, PINGONE_WEB_CLIENT,
PINGONE_REST_CLIENT) with sensible defaults only where safe, and add a fail-fast
check (throw or process.exit) if required vars are missing so tests don’t run
with stale or leaked identifiers.
In `@javascript/reactjs-todo-oidc/README.md`:
- Line 43: Update the CORS origin in the README: replace the incorrect scheme
"http://localhost:8443" with the correct "https://localhost:8443" so the CORS
Allowed origins entry matches the app URL used by the sample; ensure any other
references to the origin string in the README or config examples use the same
https://localhost:8443 value.
---
Nitpick comments:
In `@javascript/reactjs-todo-davinci/client/context/README.md`:
- Line 5: Replace the awkward sentence "If global state becomes a more complex
function of the app, something like Redux might be a better option." in the
README.md with clearer wording; change it to "If global state management becomes
more complex, Redux might be a better option." to improve readability and
explicitly reference global state management.
In `@javascript/reactjs-todo-oidc/client/README.md`:
- Line 30: Update the README line that documents SERVER to use "username" as one
word instead of "user name"; find the line containing the text "`SERVER` is used
to derive the user name from either PingAM or PingOne" and replace "user name"
with "username" so it reads that SERVER is used to derive the "username" from
either PingAM or PingOne.
In `@javascript/reactjs-todo-oidc/e2e/utils/demo-user.js`:
- Around line 8-14: Replace the hardcoded E2E credentials (amUsername,
amPassword, amDisplayName, p1Username, p1Password, p1DisplayName) so they are
resolved from environment variables with sensible local defaults; e.g., read
process.env.AM_USERNAME || 'JsAmSampleAppsE2E@user.com' for amUsername,
process.env.AM_PASSWORD || 'Demo_12345!' for amPassword,
process.env.AM_DISPLAY_NAME || 'Demo User' and similarly use P1_USERNAME,
P1_PASSWORD, P1_DISPLAY_NAME for the p1* exports; ensure the exported constants
remain the same names so callers don't change, and document the new env var
names in a comment or README for CI usage.
🪄 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: fd4c3d12-ff05-46fc-9b13-5dce26954b79
⛔ Files ignored due to path filters (1)
javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
javascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/context/README.mdjavascript/reactjs-todo-davinci/client/views/home.jsjavascript/reactjs-todo-davinci/e2e/davinci-login.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-logout.spec.jsjavascript/reactjs-todo-davinci/e2e/utils/demo-user.jsjavascript/reactjs-todo-journey/client/context/README.mdjavascript/reactjs-todo-journey/client/views/home.jsxjavascript/reactjs-todo-journey/client/views/login.jsxjavascript/reactjs-todo-login-widget/client/context/README.mdjavascript/reactjs-todo-oidc/.env.examplejavascript/reactjs-todo-oidc/README.mdjavascript/reactjs-todo-oidc/client/README.mdjavascript/reactjs-todo-oidc/client/constants.jsjavascript/reactjs-todo-oidc/client/context/README.mdjavascript/reactjs-todo-oidc/client/context/protect.context.jsjavascript/reactjs-todo-oidc/client/index.jsjavascript/reactjs-todo-oidc/client/views/home.jsjavascript/reactjs-todo-oidc/client/views/login.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-todo.spec.jsjavascript/reactjs-todo-oidc/e2e/utils/demo-user.jsjavascript/reactjs-todo-oidc/playwright.config.tsjavascript/reactjs-todo-oidc/webpack.config.js
💤 Files with no reviewable changes (1)
- javascript/reactjs-todo-oidc/client/context/protect.context.js
Comment on lines
69
to
80
| useEffect(() => { | ||
| async function handleCentralizedLogin() { | ||
| try { | ||
| if (codeParam && stateParam) { | ||
| /** | ||
| * When the user returns to this app after successfully logging in, | ||
| * the URL will include code and state query parameters that need to | ||
| * be passed in to complete the OAuth flow giving the user access | ||
| */ | ||
| setLoadingMessage('Success! Redirecting ...'); | ||
| await authorize(codeParam, stateParam); | ||
| } else if (errorParam) { |
There was a problem hiding this comment.
Guard callback handling to prevent duplicate code exchanges.
This effect can re-run during auth-state updates and invoke authorize(code,state) again for the same callback params. A second token exchange on the same code can fail intermittently.
✅ Suggested hardening
+import React, { useContext, useEffect, useState, useCallback, useRef } from 'react';
...
+ const callbackHandledRef = useRef(false);
...
useEffect(() => {
async function handleCentralizedLogin() {
try {
if (codeParam && stateParam) {
+ if (callbackHandledRef.current) return;
+ callbackHandledRef.current = true;
setLoadingMessage('Success! Redirecting ...');
await authorize(codeParam, stateParam);Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javascript/reactjs-todo-oidc/client/views/login.js` around lines 69 - 80, The
effect inside useEffect (function handleCentralizedLogin) can call
authorize(codeParam, stateParam) multiple times; add a guard to ensure a given
callback is processed once by tracking a processed flag or the last processed
code (e.g., a useRef like processedCallbackRef) and checking it before calling
authorize, or clear/remove codeParam/stateParam from the URL once handled;
ensure the guard is referenced in the same effect that calls authorize and also
applied where similar logic runs around line 109 so duplicate token exchanges
are prevented.
Comment on lines
+57
to
+60
| 'https://auth.pingone.ca/02fb4743-189a-4bc7-9d6c-a919edfe6447/as/.well-known/openid-configuration', | ||
| SCOPE: 'openid profile email revoke', | ||
| WEB_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', | ||
| REST_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', |
There was a problem hiding this comment.
Avoid hardcoded tenant/client identifiers in E2E server env.
These PingOne-specific identifiers are environment-coupled and can become stale or leak internal test configuration. Prefer sourcing them from env vars with safe defaults/fail-fast validation.
🔧 Proposed change
env: {
API_URL: 'http://localhost:9443',
DEBUGGER_OFF: 'true',
DEVELOPMENT: 'false',
PORT: '8444',
SERVER: 'PINGONE',
- WELLKNOWN_URL:
- 'https://auth.pingone.ca/02fb4743-189a-4bc7-9d6c-a919edfe6447/as/.well-known/openid-configuration',
+ WELLKNOWN_URL: process.env.PINGONE_WELLKNOWN_URL || '',
SCOPE: 'openid profile email revoke',
- WEB_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9',
- REST_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9',
+ WEB_OAUTH_CLIENT: process.env.PINGONE_WEB_OAUTH_CLIENT || '',
+ REST_OAUTH_CLIENT: process.env.PINGONE_REST_OAUTH_CLIENT || '',
},📝 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.
Suggested change
| 'https://auth.pingone.ca/02fb4743-189a-4bc7-9d6c-a919edfe6447/as/.well-known/openid-configuration', | |
| SCOPE: 'openid profile email revoke', | |
| WEB_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', | |
| REST_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', | |
| env: { | |
| API_URL: 'http://localhost:9443', | |
| DEBUGGER_OFF: 'true', | |
| DEVELOPMENT: 'false', | |
| PORT: '8444', | |
| SERVER: 'PINGONE', | |
| WELLKNOWN_URL: process.env.PINGONE_WELLKNOWN_URL || '', | |
| SCOPE: 'openid profile email revoke', | |
| WEB_OAUTH_CLIENT: process.env.PINGONE_WEB_OAUTH_CLIENT || '', | |
| REST_OAUTH_CLIENT: process.env.PINGONE_REST_OAUTH_CLIENT || '', | |
| }, |
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 59-59: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javascript/reactjs-todo-oidc/playwright.config.ts` around lines 57 - 60,
Replace hardcoded PingOne tenant/client identifiers in the Playwright config by
reading them from environment variables and validate presence at startup: for
the constants SCOPE, WEB_OAUTH_CLIENT, REST_OAUTH_CLIENT and the issuer URL,
change assignment to use process.env (e.g., process.env.PINGONE_ISSUER,
PINGONE_SCOPE, PINGONE_WEB_CLIENT, PINGONE_REST_CLIENT) with sensible defaults
only where safe, and add a fail-fast check (throw or process.exit) if required
vars are missing so tests don’t run with stale or leaked identifiers.
ancheetah
force-pushed
the
SDKS-4902-update-js-oidc
branch
3 times, most recently
from
9f31853 to
5b4f348
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
javascript/reactjs-todo-oidc/playwright.config.ts (1)
57-60:⚠️ Potential issue | 🟠 MajorAvoid hardcoded PingOne tenant/client identifiers in test server env.
Line 57, Line 59, and Line 60 are environment-coupled values that can go stale or expose internal config. Read them from env vars and fail fast when missing.
🔧 Proposed fix
+const PINGONE_WELLKNOWN_URL = process.env.PINGONE_WELLKNOWN_URL; +const PINGONE_WEB_OAUTH_CLIENT = process.env.PINGONE_WEB_OAUTH_CLIENT; +const PINGONE_REST_OAUTH_CLIENT = process.env.PINGONE_REST_OAUTH_CLIENT; + +if (!PINGONE_WELLKNOWN_URL || !PINGONE_WEB_OAUTH_CLIENT || !PINGONE_REST_OAUTH_CLIENT) { + throw new Error( + 'Missing required PingOne E2E env vars: PINGONE_WELLKNOWN_URL, PINGONE_WEB_OAUTH_CLIENT, PINGONE_REST_OAUTH_CLIENT', + ); +} + export default defineConfig({ @@ SERVER: 'PINGONE', - WELLKNOWN_URL: - 'https://auth.pingone.ca/02fb4743-189a-4bc7-9d6c-a919edfe6447/as/.well-known/openid-configuration', + WELLKNOWN_URL: PINGONE_WELLKNOWN_URL, SCOPE: 'openid profile email revoke', - WEB_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', - REST_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', + WEB_OAUTH_CLIENT: PINGONE_WEB_OAUTH_CLIENT, + REST_OAUTH_CLIENT: PINGONE_REST_OAUTH_CLIENT, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/playwright.config.ts` around lines 57 - 60, Replace hardcoded PingOne values by reading them from environment variables: replace the issuer URL, SCOPE, WEB_OAUTH_CLIENT, and REST_OAUTH_CLIENT literals with process.env-backed variables (e.g. PINGONE_ISSUER, PINGONE_SCOPE, PINGONE_WEB_OAUTH_CLIENT, PINGONE_REST_OAUTH_CLIENT) wherever they are defined in the test server config (identify the constants currently set to the URL and the three token/client strings). Add a fail-fast check during startup (throw or process.exit with a clear error) if any of these env vars is missing so tests fail early instead of using stale/internal defaults.javascript/reactjs-todo-oidc/.env.example (1)
3-9:⚠️ Potential issue | 🟡 MinorReorder
.envkeys to satisfydotenv-linter.Key ordering still violates lint rules (for example,
SCOPE/SERVERshould precedeWEB_OAUTH_CLIENT, andAPI_URL/DEBUGGER_OFF/DEVELOPMENTshould be beforePORT/SCOPE).🔧 Proposed fix
-WEB_OAUTH_CLIENT= -WELLKNOWN_URL= -SCOPE= -#SERVER - 'PINGAM' or 'PINGONE' -SERVER= -API_URL=http://localhost:9443 -PORT=8443 -DEBUGGER_OFF=true -DEVELOPMENT=true +API_URL=http://localhost:9443 +DEBUGGER_OFF=true +DEVELOPMENT=true +PORT=8443 +SCOPE= +#SERVER - 'PINGAM' or 'PINGONE' +SERVER= +WEB_OAUTH_CLIENT= +WELLKNOWN_URL=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/.env.example` around lines 3 - 9, Reorder the environment variable entries so they follow dotenv-linter's expected alphabetical/grouping order: place API_URL, DEBUGGER_OFF, and DEVELOPMENT before PORT and SCOPE; ensure SCOPE and SERVER appear before WEB_OAUTH_CLIENT; keep PORT after API_URL but before SCOPE if required by linter; update the `.env.example` entries (keys: API_URL, DEBUGGER_OFF, DEVELOPMENT, PORT, SCOPE, SERVER, WEB_OAUTH_CLIENT) accordingly to eliminate the lint warnings.javascript/reactjs-todo-oidc/client/views/login.js (1)
69-80:⚠️ Potential issue | 🟠 MajorStill unresolved: prevent duplicate callback processing before token exchange.
Line 72 can be re-entered on effect re-runs, which may call
authorize(codeParam, stateParam)more than once for the same callback and cause intermittent code-exchange failures.🔒 Suggested hardening
-import React, { useContext, useEffect, useState, useCallback } from 'react'; +import React, { useContext, useEffect, useState, useCallback, useRef } from 'react'; ... + const handledCallbackRef = useRef(null); ... try { if (codeParam && stateParam) { + const callbackKey = `${codeParam}:${stateParam}`; + if (handledCallbackRef.current === callbackKey) return; + handledCallbackRef.current = callbackKey; /** * When the user returns to this app after successfully logging in, * the URL will include code and state query parameters that need to * be passed in to complete the OAuth flow giving the user access */ setLoadingMessage('Success! Redirecting ...'); await authorize(codeParam, stateParam);#!/bin/bash # Verify whether provider value/method identity can trigger effect reruns. rg -nC3 'OidcContext\.Provider|value=|useMemo|setUserWrapper|setEmailWrapper|setAuthenticationWrapper' javascript/reactjs-todo-oidc/client/context/oidc.context.js rg -nC3 'useEffect\(|authorize\(codeParam, stateParam\)|\[[^]]*methods|oidcClient|stateParam' javascript/reactjs-todo-oidc/client/views/login.jsAlso applies to: 108-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/client/views/login.js` around lines 69 - 80, The effect in useEffect that runs handleCentralizedLogin may re-enter and call authorize(codeParam, stateParam) multiple times; to fix, add an idempotency guard (e.g., a React ref like processedCallbackRef) checked at the start of handleCentralizedLogin and set to true immediately before calling await authorize(...) so subsequent effect reruns skip processing the same code/state, and/or clear the callback query params (location.replace/history.replaceState) after successful authorize to prevent re-processing; update the useEffect/handleCentralizedLogin logic to reference processedCallbackRef (or similar) and to short-circuit if already processed.
🧹 Nitpick comments (6)
javascript/reactjs-todo-journey/.env.example (4)
3-3: Provide a clearer placeholder forVITE_WEB_OAUTH_CLIENTin the sample.Right now
VITE_WEB_OAUTH_CLIENT=is blank. For an.env.example, it’s usually better to use an explicit placeholder (e.g.,VITE_WEB_OAUTH_CLIENT=YOUR_CLIENT_ID) so users know what to fill in and reduces “why is this blank?” confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/.env.example` at line 3, Replace the empty placeholder for VITE_WEB_OAUTH_CLIENT in the .env.example with a clear explicit placeholder value so users know what to supply; update the line containing the VITE_WEB_OAUTH_CLIENT variable to use an obvious token like YOUR_CLIENT_ID (e.g., VITE_WEB_OAUTH_CLIENT=YOUR_CLIENT_ID) and keep any comment or format consistent with other entries in the file.
10-11: Verify defaults (VITE_JOURNEY_REGISTER,VITE_REALM_PATH) align with README/e2e expectations.Defaults were filled in (
VITE_JOURNEY_REGISTER=Registration,VITE_REALM_PATH=alpha). Please ensure these match what the login/journey E2E tests and README instruct; otherwise local runs will fail in a non-obvious way.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/.env.example` around lines 10 - 11, Verify that the environment defaults for VITE_JOURNEY_REGISTER and VITE_REALM_PATH in .env.example match the README and the E2E/login journey expectations; if they differ, update the values for VITE_JOURNEY_REGISTER and VITE_REALM_PATH to the exact strings used by the tests/README or replace them with empty placeholders and add a comment so local runs and CI use the correct realm and journey names (search for VITE_JOURNEY_REGISTER and VITE_REALM_PATH in the repo and the E2E test config to determine the canonical values to use).
4-6: Clarify port mapping forVITE_API_URLvs appVITE_PORT.You set
VITE_API_URL=http://localhost:9443while the app usesVITE_PORT=8443. That might be correct (backend vs UI ports), but the sample would benefit from a short comment explaining the two-port assumption to avoid local debugging confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/.env.example` around lines 4 - 6, Add a brief inline comment above the environment entries clarifying that VITE_API_URL (currently http://localhost:9443) points to the backend API host/port while VITE_PORT (8443) is the frontend app port so developers don’t confuse the two; update the .env.example by inserting a one-line comment referencing VITE_API_URL and VITE_PORT near those variables and keep the existing variable names/values unchanged.
3-4: Clean up dotenv-linter key ordering to avoid noisy lint failures.
dotenv-linteris reporting multiple[UnorderedKey]warnings about key order (e.g.,VITE_API_URLshould appear beforeVITE_WEB_OAUTH_CLIENT, and several other ordering constraints). Even if harmless at runtime, it can block CI if lint is enforced.Suggested reordering (example)
- VITE_WEB_OAUTH_CLIENT= - VITE_API_URL=http://localhost:9443 + VITE_API_URL=http://localhost:9443 + VITE_WEB_OAUTH_CLIENT=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/.env.example` around lines 3 - 4, Reorder the keys in the .env.example so they match dotenv-linter's expected ordering (e.g., place VITE_API_URL before VITE_WEB_OAUTH_CLIENT) — update the VITE_* entries to be alphabetically ordered (or follow the project's established key order) and ensure any other env variables in the file are similarly reordered to eliminate UnorderedKey warnings.javascript/reactjs-todo-oidc/client/README.md (1)
23-30: ClarifySERVERaccepted values and normalize wording.Consider documenting
SERVERasPINGAM | PINGONEand usingusername(one word) for consistency/readability.✏️ Suggested wording
-SERVER=PINGAM +SERVER=PINGAM # allowed: PINGAM | PINGONE ... -- `SERVER` is used to derive the user name from either PingAM or PingOne +- `SERVER` is used to derive the username from either PingAM or PingOne.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/client/README.md` around lines 23 - 30, Update the README.md docs to clarify the accepted values for SERVER as "PINGAM | PINGONE" (use that exact syntax) and normalize wording to use "username" (one word) everywhere; specifically update the note that currently reads "SERVER is used to derive the user name from either PingAM or PingOne" to instead state that SERVER accepts PINGAM | PINGONE and is used to derive the username from PingAM or PingOne, and ensure WELLKNOWN_URL and SCOPE notes remain but include that SCOPE must include "openid" for logout and userinfo flows.javascript/reactjs-todo-oidc/webpack.config.js (1)
28-29: Add a safe default forSERVERbefore injecting it into the client build.If
SERVERis unset,process.env.SERVERbecomesundefinedin the bundle. Defaulting toPINGAMkeeps behavior deterministic.🔧 Proposed fix
- const SERVER = process.env.SERVER || localEnv.SERVER; + const SERVER = process.env.SERVER || localEnv.SERVER || 'PINGAM';Also applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/webpack.config.js` around lines 28 - 29, The SERVER constant can end up undefined in the client bundle; update the assignment for SERVER to provide a safe fallback string (e.g., 'PINGAM') so the client build never gets undefined—replace the current const SERVER = process.env.SERVER || localEnv.SERVER; with a fallback chain that ensures a string (process.env.SERVER || localEnv.SERVER || 'PINGAM' or using nullish coalescing), and apply the same safe-default change to the other occurrence where SERVER is defined so DefinePlugin (or any injection) always receives a deterministic value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@javascript/reactjs-todo-journey/package.json`:
- Line 33: Update the package.json npm scripts so the existing "lint" script is
non-mutating (remove the "--fix" flag from the "lint" script) and add a new
"lint:fix" script that runs "eslint . --fix" for local dev; leave CI configured
to call the non-mutating "lint" script so checks fail fast rather than
auto-correcting.
In `@javascript/reactjs-todo-login-widget/.env.example`:
- Around line 4-11: The .env.example no longer includes environment keys that
javascript/todo-api/src/app/constants.js and middleware.js expect (notably
SERVER_TYPE and the OAuth client values), causing local auth flows to fail; add
back the missing env variables (e.g., SERVER_TYPE, OAUTH_CLIENT_ID,
OAUTH_CLIENT_SECRET and any OAUTH_ISSUER/OIDC-related keys used by the code) to
.env.example with placeholder values and a short comment, or alternatively
document in the README that these values must be provided from another source so
that the references in constants.js and middleware.js resolve correctly.
In `@javascript/reactjs-todo-oidc/client/views/login.js`:
- Around line 58-63: Normalize the user claims before persisting: when computing
the username (used with methods.setUser) build it from SERVER === 'PINGONE'
using `${user.given_name ?? ''} ${user.family_name ?? ''}` but then .trim() it
and if empty fall back to user.preferred_username, then to user.name; for email
(methods.setEmail) ensure you coerce undefined/null to an empty string or a
sensible fallback (e.g., user.email ?? ''), and optionally lowercase/trim the
value to avoid storing the literal "undefined"; keep
methods.setAuthentication(true) as-is.
---
Duplicate comments:
In `@javascript/reactjs-todo-oidc/.env.example`:
- Around line 3-9: Reorder the environment variable entries so they follow
dotenv-linter's expected alphabetical/grouping order: place API_URL,
DEBUGGER_OFF, and DEVELOPMENT before PORT and SCOPE; ensure SCOPE and SERVER
appear before WEB_OAUTH_CLIENT; keep PORT after API_URL but before SCOPE if
required by linter; update the `.env.example` entries (keys: API_URL,
DEBUGGER_OFF, DEVELOPMENT, PORT, SCOPE, SERVER, WEB_OAUTH_CLIENT) accordingly to
eliminate the lint warnings.
In `@javascript/reactjs-todo-oidc/client/views/login.js`:
- Around line 69-80: The effect in useEffect that runs handleCentralizedLogin
may re-enter and call authorize(codeParam, stateParam) multiple times; to fix,
add an idempotency guard (e.g., a React ref like processedCallbackRef) checked
at the start of handleCentralizedLogin and set to true immediately before
calling await authorize(...) so subsequent effect reruns skip processing the
same code/state, and/or clear the callback query params
(location.replace/history.replaceState) after successful authorize to prevent
re-processing; update the useEffect/handleCentralizedLogin logic to reference
processedCallbackRef (or similar) and to short-circuit if already processed.
In `@javascript/reactjs-todo-oidc/playwright.config.ts`:
- Around line 57-60: Replace hardcoded PingOne values by reading them from
environment variables: replace the issuer URL, SCOPE, WEB_OAUTH_CLIENT, and
REST_OAUTH_CLIENT literals with process.env-backed variables (e.g.
PINGONE_ISSUER, PINGONE_SCOPE, PINGONE_WEB_OAUTH_CLIENT,
PINGONE_REST_OAUTH_CLIENT) wherever they are defined in the test server config
(identify the constants currently set to the URL and the three token/client
strings). Add a fail-fast check during startup (throw or process.exit with a
clear error) if any of these env vars is missing so tests fail early instead of
using stale/internal defaults.
---
Nitpick comments:
In `@javascript/reactjs-todo-journey/.env.example`:
- Line 3: Replace the empty placeholder for VITE_WEB_OAUTH_CLIENT in the
.env.example with a clear explicit placeholder value so users know what to
supply; update the line containing the VITE_WEB_OAUTH_CLIENT variable to use an
obvious token like YOUR_CLIENT_ID (e.g., VITE_WEB_OAUTH_CLIENT=YOUR_CLIENT_ID)
and keep any comment or format consistent with other entries in the file.
- Around line 10-11: Verify that the environment defaults for
VITE_JOURNEY_REGISTER and VITE_REALM_PATH in .env.example match the README and
the E2E/login journey expectations; if they differ, update the values for
VITE_JOURNEY_REGISTER and VITE_REALM_PATH to the exact strings used by the
tests/README or replace them with empty placeholders and add a comment so local
runs and CI use the correct realm and journey names (search for
VITE_JOURNEY_REGISTER and VITE_REALM_PATH in the repo and the E2E test config to
determine the canonical values to use).
- Around line 4-6: Add a brief inline comment above the environment entries
clarifying that VITE_API_URL (currently http://localhost:9443) points to the
backend API host/port while VITE_PORT (8443) is the frontend app port so
developers don’t confuse the two; update the .env.example by inserting a
one-line comment referencing VITE_API_URL and VITE_PORT near those variables and
keep the existing variable names/values unchanged.
- Around line 3-4: Reorder the keys in the .env.example so they match
dotenv-linter's expected ordering (e.g., place VITE_API_URL before
VITE_WEB_OAUTH_CLIENT) — update the VITE_* entries to be alphabetically ordered
(or follow the project's established key order) and ensure any other env
variables in the file are similarly reordered to eliminate UnorderedKey
warnings.
In `@javascript/reactjs-todo-oidc/client/README.md`:
- Around line 23-30: Update the README.md docs to clarify the accepted values
for SERVER as "PINGAM | PINGONE" (use that exact syntax) and normalize wording
to use "username" (one word) everywhere; specifically update the note that
currently reads "SERVER is used to derive the user name from either PingAM or
PingOne" to instead state that SERVER accepts PINGAM | PINGONE and is used to
derive the username from PingAM or PingOne, and ensure WELLKNOWN_URL and SCOPE
notes remain but include that SCOPE must include "openid" for logout and
userinfo flows.
In `@javascript/reactjs-todo-oidc/webpack.config.js`:
- Around line 28-29: The SERVER constant can end up undefined in the client
bundle; update the assignment for SERVER to provide a safe fallback string
(e.g., 'PINGAM') so the client build never gets undefined—replace the current
const SERVER = process.env.SERVER || localEnv.SERVER; with a fallback chain that
ensures a string (process.env.SERVER || localEnv.SERVER || 'PINGAM' or using
nullish coalescing), and apply the same safe-default change to the other
occurrence where SERVER is defined so DefinePlugin (or any injection) always
receives a deterministic value.
🪄 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: da7ac9a5-564f-43b7-ba1b-a1cd9acf2c17
⛔ Files ignored due to path filters (1)
javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
.github/workflows/CI.ymljavascript/reactjs-todo-davinci/.env.examplejavascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/context/README.mdjavascript/reactjs-todo-davinci/client/views/home.jsjavascript/reactjs-todo-davinci/e2e/davinci-login.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-logout.spec.jsjavascript/reactjs-todo-davinci/e2e/utils/demo-user.jsjavascript/reactjs-todo-davinci/webpack.config.jsjavascript/reactjs-todo-journey/.env.examplejavascript/reactjs-todo-journey/README.mdjavascript/reactjs-todo-journey/client/context/README.mdjavascript/reactjs-todo-journey/client/views/home.jsxjavascript/reactjs-todo-journey/client/views/login.jsxjavascript/reactjs-todo-journey/package.jsonjavascript/reactjs-todo-journey/vite.config.jsjavascript/reactjs-todo-login-widget/.env.examplejavascript/reactjs-todo-login-widget/README.mdjavascript/reactjs-todo-login-widget/client/constants.jsjavascript/reactjs-todo-login-widget/client/context/README.mdjavascript/reactjs-todo-login-widget/client/context/auth.context.jsjavascript/reactjs-todo-login-widget/playwright.config.tsjavascript/reactjs-todo-login-widget/webpack.config.jsjavascript/reactjs-todo-oidc/.env.examplejavascript/reactjs-todo-oidc/README.mdjavascript/reactjs-todo-oidc/client/README.mdjavascript/reactjs-todo-oidc/client/constants.jsjavascript/reactjs-todo-oidc/client/context/README.mdjavascript/reactjs-todo-oidc/client/context/protect.context.jsjavascript/reactjs-todo-oidc/client/index.jsjavascript/reactjs-todo-oidc/client/views/home.jsjavascript/reactjs-todo-oidc/client/views/login.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-todo.spec.jsjavascript/reactjs-todo-oidc/e2e/utils/demo-user.jsjavascript/reactjs-todo-oidc/playwright.config.tsjavascript/reactjs-todo-oidc/webpack.config.js
💤 Files with no reviewable changes (2)
- javascript/reactjs-todo-login-widget/client/constants.js
- javascript/reactjs-todo-oidc/client/context/protect.context.js
✅ Files skipped from review due to trivial changes (15)
- javascript/reactjs-todo-journey/vite.config.js
- javascript/reactjs-todo-oidc/client/context/README.md
- javascript/reactjs-todo-login-widget/client/context/README.md
- javascript/reactjs-todo-davinci/e2e/utils/demo-user.js
- javascript/reactjs-todo-journey/client/context/README.md
- javascript/reactjs-todo-journey/client/views/home.jsx
- javascript/reactjs-todo-davinci/client/context/README.md
- javascript/reactjs-todo-oidc/client/views/home.js
- javascript/reactjs-todo-davinci/client/views/home.js
- javascript/reactjs-todo-davinci/e2e/davinci-logout.spec.js
- javascript/reactjs-todo-oidc/e2e/utils/demo-user.js
- javascript/reactjs-todo-journey/README.md
- javascript/reactjs-todo-davinci/README.md
- javascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.js
- javascript/reactjs-todo-login-widget/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- javascript/reactjs-todo-davinci/e2e/davinci-login.spec.js
- javascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.js
- javascript/reactjs-todo-oidc/e2e/oidc-todo.spec.js
- javascript/reactjs-todo-oidc/client/constants.js
- javascript/reactjs-todo-journey/client/views/login.jsx
| "start": "vite", | ||
| "build": "vite build", | ||
| "lint": "eslint .", | ||
| "lint": "eslint . --fix", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CI lint invocation:"
rg -n -C2 'name:\s*Lint|run:\s*npm run lint' .github/workflows/CI.yml
echo
echo "Workspace lint scripts:"
fd package.json javascript -t f -x sh -c '
file="$1"
jq -r --arg f "$file" '"'"'
if .scripts and .scripts.lint then "\($f): \(.scripts.lint)" else empty end
'"'"' "$file"
' sh {}Repository: ForgeRock/sdk-sample-apps
Length of output: 965
Split lint into read-only and mutating variants for CI correctness.
CI should fail fast on violations, not auto-correct them. Currently, npm run lint (called by CI at line 36 of .github/workflows/CI.yml) invokes eslint . --fix, allowing CI to mutate code instead of strictly checking. Keep lint non-mutating and add a separate lint:fix script for local development.
Suggested change
- "lint": "eslint . --fix",
+ "lint": "eslint .",
+ "lint:fix": "eslint . --fix",📝 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.
Suggested change
| "lint": "eslint . --fix", | |
| "lint": "eslint .", | |
| "lint:fix": "eslint . --fix", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javascript/reactjs-todo-journey/package.json` at line 33, Update the
package.json npm scripts so the existing "lint" script is non-mutating (remove
the "--fix" flag from the "lint" script) and add a new "lint:fix" script that
runs "eslint . --fix" for local dev; leave CI configured to call the
non-mutating "lint" script so checks fail fast rather than auto-correcting.
Comment on lines
+58
to
+63
| const username = | ||
| SERVER === 'PINGONE' ? `${user.given_name ?? ''} ${user.family_name ?? ''}` : user.name; | ||
|
|
||
| setState({ | ||
| loadingMessage: 'Success! Redirecting ...', | ||
| }); | ||
| methods.setUser(username); | ||
| methods.setEmail(user.email); | ||
| methods.setAuthentication(true); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For PingOne OIDC userinfo (including DaVinci-mediated responses), which claims are guaranteed for profile/email scopes: given_name, family_name, name, preferred_username, and email?
💡 Result:
For PingOne OIDC UserInfo endpoint responses (including those mediated by DaVinci flows), the profile scope guarantees the claims: given_name, family_name, name, preferred_username. The email scope guarantees: email. These are standard claims provided when the respective scopes are requested in the authorization flow, as confirmed by official PingOne Platform documentation and example UserInfo responses.
Citations:
- 1: https://developer.pingidentity.com/pingone-api/auth/openid-connect-oauth-2/userinfo.html
- 2: https://developer.pingidentity.com/pingone-api/foundations/pingone-roles-scopes-and-permissions/access-services-through-scopes-and-roles/openid-connect-oidc-scopes.html
- 3: https://developer.pingidentity.com/pingone-api/auth/openid-connect-oauth-2/userinfo-1.html
- 4: https://docs.pingidentity.com/developer-resources/openid_connect_developer_guide/userinfo-endpoint.html
🏁 Script executed:
# First, find the login.js file and read the full context around lines 58-63
fd -n "login.js" --type fRepository: ForgeRock/sdk-sample-apps
Length of output: 300
🏁 Script executed:
# Search for login.js file without the invalid flag
fd "login.js" --type fRepository: ForgeRock/sdk-sample-apps
Length of output: 349
🏁 Script executed:
# Read the login.js file with line numbers to see context
cat -n "javascript/reactjs-todo-oidc/client/views/login.js" | head -80Repository: ForgeRock/sdk-sample-apps
Length of output: 3636
🏁 Script executed:
# Search for OIDC configuration and scope settings
rg -i "scope" javascript/reactjs-todo-oidc --type js --type jsx -B2 -A2Repository: ForgeRock/sdk-sample-apps
Length of output: 97
🏁 Script executed:
# Search for OIDC configuration and scope settings (without jsx type)
rg -i "scope" javascript/reactjs-todo-oidc --type js -B2 -A2Repository: ForgeRock/sdk-sample-apps
Length of output: 2109
🏁 Script executed:
# Search for .env files and README to see SCOPE configuration
find javascript/reactjs-todo-oidc -type f \( -name ".env*" -o -name "README*" \) | head -10Repository: ForgeRock/sdk-sample-apps
Length of output: 468
🏁 Script executed:
# Read the .env.example to see default SCOPE configuration
cat "javascript/reactjs-todo-oidc/.env.example"Repository: ForgeRock/sdk-sample-apps
Length of output: 224
🏁 Script executed:
# Read the README to see SCOPE configuration details
cat "javascript/reactjs-todo-oidc/README.md"Repository: ForgeRock/sdk-sample-apps
Length of output: 5890
🏁 Script executed:
# Search for methods.setUser, setEmail, and setAuthentication to see how they handle values
rg "setUser|setEmail|setAuthentication" javascript/reactjs-todo-oidc --type js -B2 -A5Repository: ForgeRock/sdk-sample-apps
Length of output: 9449
🏁 Script executed:
# Let's verify the context initialization to see default values
cat -n "javascript/reactjs-todo-oidc/client/context/oidc.context.js" | head -50Repository: ForgeRock/sdk-sample-apps
Length of output: 2603
Normalize user claims before persisting to context/session storage.
Line 59 can resolve to a space-only username (if both given_name and family_name are empty strings), and line 62 will persist undefined as the string "undefined" if the email claim is missing. Add .trim() to the username logic and provide fallback claims in the correct priority order (including preferred_username as a reliable fallback). Also normalize the email value explicitly.
Suggested fix
- const username =
- SERVER === 'PINGONE' ? `${user.given_name ?? ''} ${user.family_name ?? ''}` : user.name;
+ const pingOneName = `${user.given_name ?? ''} ${user.family_name ?? ''}`.trim();
+ const username =
+ SERVER === 'PINGONE'
+ ? pingOneName || user.name || user.preferred_username || user.email || ''
+ : user.name || user.preferred_username || user.email || '';
+ const email = user.email ?? '';
methods.setUser(username);
- methods.setEmail(user.email);
+ methods.setEmail(email);
methods.setAuthentication(true);📝 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.
Suggested change
| const username = | |
| SERVER === 'PINGONE' ? `${user.given_name ?? ''} ${user.family_name ?? ''}` : user.name; | |
| setState({ | |
| loadingMessage: 'Success! Redirecting ...', | |
| }); | |
| methods.setUser(username); | |
| methods.setEmail(user.email); | |
| methods.setAuthentication(true); | |
| const pingOneName = `${user.given_name ?? ''} ${user.family_name ?? ''}`.trim(); | |
| const username = | |
| SERVER === 'PINGONE' | |
| ? pingOneName || user.name || user.preferred_username || user.email || '' | |
| : user.name || user.preferred_username || user.email || ''; | |
| const email = user.email ?? ''; | |
| methods.setUser(username); | |
| methods.setEmail(email); | |
| methods.setAuthentication(true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javascript/reactjs-todo-oidc/client/views/login.js` around lines 58 - 63,
Normalize the user claims before persisting: when computing the username (used
with methods.setUser) build it from SERVER === 'PINGONE' using
`${user.given_name ?? ''} ${user.family_name ?? ''}` but then .trim() it and if
empty fall back to user.preferred_username, then to user.name; for email
(methods.setEmail) ensure you coerce undefined/null to an empty string or a
sensible fallback (e.g., user.email ?? ''), and optionally lowercase/trim the
value to avoid storing the literal "undefined"; keep
methods.setAuthentication(true) as-is.
5b4f348 to
5b782e4
Compare
ancheetah
requested review from
SteinGabriel,
cerebrl,
ryanbas21 and
vatsalparikh
ancheetah
force-pushed
the
SDKS-4902-update-js-oidc
branch
from
5b782e4 to
ae4ca1d
Compare
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 (2)
javascript/reactjs-todo-journey/README.md (1)
105-110:⚠️ Potential issue | 🟡 MinorCorrect debugger env var name in README (
DEBUGGER_OFF→VITE_DEBUGGER_OFF).The README states: “set …
DEBUGGER_OFFto true.” But the env example usesVITE_DEBUGGER_OFF=true(and Vite reads fromimport.meta.env.VITE_*). To prevent confusion, update the later documentation toVITE_DEBUGGER_OFF.🛠️ Suggested diff
-For local development, if you want to turn these debuggers off, you can set the environment variable of `DEBUGGER_OFF` to true. +For local development, if you want to turn these debuggers off, you can set the environment variable of `VITE_DEBUGGER_OFF` to true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/README.md` around lines 105 - 110, Update the README to use the correct Vite env var name: replace mentions of DEBUGGER_OFF with VITE_DEBUGGER_OFF so examples and instructions align with Vite's import.meta.env.VITE_* convention; ensure the sentence that currently reads “set the environment variable of `DEBUGGER_OFF` to true” instead references `VITE_DEBUGGER_OFF` and, if present, update any example lines (e.g., `VITE_DEBUGGER_OFF=true`) to match exactly.javascript/reactjs-todo-journey/.env.example (1)
14-21:⚠️ Potential issue | 🟡 MinorTrim trailing whitespace in blank env values.
VITE_INIT_PROTECT=andVITE_PINGONE_ENV_ID=include trailing spaces. It’s safer to useVITE_INIT_PROTECT=/VITE_PINGONE_ENV_ID=with no extra whitespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/.env.example` around lines 14 - 21, Remove the trailing spaces after the environment variable assignments in the example file: change "VITE_INIT_PROTECT= " to "VITE_INIT_PROTECT=" and "VITE_PINGONE_ENV_ID= " to "VITE_PINGONE_ENV_ID=" so the blank env values contain no extra whitespace; ensure any other blank env entries follow the same pattern.
♻️ Duplicate comments (5)
javascript/reactjs-todo-oidc/.env.example (1)
1-10: 🛠️ Refactor suggestion | 🟠 Majordotenv-linter key ordering violations persist in
.env.exampleThe
.env.examplekey order still conflicts withdotenv-linter’sUnorderedKeyexpectations (e.g.,SCOPE/SERVERmust come beforeWEB_OAUTH_CLIENT, andAPI_URL/PORT/DEBUGGER_OFF/DEVELOPMENTmust be ordered relative toSCOPE/PORT). This will keep lint output noisy.🔧 Proposed diff
-WEB_OAUTH_CLIENT= -WELLKNOWN_URL= -SCOPE= -#SERVER - 'PINGAM' or 'PINGONE' -SERVER= API_URL=http://localhost:9443 -PORT=8443 DEBUGGER_OFF=true DEVELOPMENT=true + +PORT=8443 +#SERVER - 'PINGAM' or 'PINGONE' +SERVER= +SCOPE= +WELLKNOWN_URL= +WEB_OAUTH_CLIENT=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/.env.example` around lines 1 - 10, The .env.example key order violates dotenv-linter's UnorderedKey rules; reorder the environment variables so keys appear in the expected sequence (place SCOPE and SERVER before WEB_OAUTH_CLIENT, and ensure API_URL, PORT, DEBUGGER_OFF, DEVELOPMENT are ordered relative to SCOPE/PORT as the linter expects). Update the file to list keys in the canonical order: WELLKNOWN_URL, SCOPE, SERVER, WEB_OAUTH_CLIENT (or follow your project's lint ordering), then API_URL, PORT, DEBUGGER_OFF, DEVELOPMENT so the dotenv-linter no longer reports UnorderedKey violations for WEB_OAUTH_CLIENT, WELLKNOWN_URL, SCOPE, SERVER, API_URL, PORT, DEBUGGER_OFF, and DEVELOPMENT.javascript/reactjs-todo-oidc/client/views/login.js (3)
69-80:⚠️ Potential issue | 🟠 MajorGuard callback processing to prevent duplicate code exchange.
This effect can run again while
code/stateare still present, causing a secondauthorize()on the same callback params.Suggested hardening
-import React, { useContext, useEffect, useState, useCallback } from 'react'; +import React, { useContext, useEffect, useState, useCallback, useRef } from 'react'; export default function Login() { + const callbackHandledRef = useRef(false); // ... useEffect(() => { async function handleCentralizedLogin() { try { if (codeParam && stateParam) { + if (callbackHandledRef.current) return; + callbackHandledRef.current = true; setLoadingMessage('Success! Redirecting ...'); await authorize(codeParam, stateParam);Also applies to: 108-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/client/views/login.js` around lines 69 - 80, The effect in useEffect (inside handleCentralizedLogin) can call authorize(codeParam, stateParam) more than once when codeParam/stateParam remain in the URL; add a one-time guard to prevent duplicate callback processing: introduce a persisted flag (e.g., processedCallbackRef or state like callbackProcessed) and check it before calling authorize, set it immediately after starting/succeeding the authorize flow, and skip further calls when the flag is true; update references to codeParam/stateParam/errorParam and setLoadingMessage accordingly so authorize is invoked exactly once per callback.
58-63:⚠️ Potential issue | 🟡 MinorNormalize and fallback user claims before persisting context.
Line 58-63 can store blank username and undefined email. Add trimming + fallback order for username and coerce email to a safe string.
Suggested fix
- const username = - SERVER === 'PINGONE' ? `${user.given_name ?? ''} ${user.family_name ?? ''}` : user.name; + const pingOneName = `${user.given_name ?? ''} ${user.family_name ?? ''}`.trim(); + const username = + SERVER === 'PINGONE' + ? pingOneName || user.preferred_username || user.name || user.email || '' + : user.name || user.preferred_username || user.email || ''; + const email = (user.email ?? '').trim(); methods.setUser(username); - methods.setEmail(user.email); + methods.setEmail(email); methods.setAuthentication(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/client/views/login.js` around lines 58 - 63, The username/email assignment can store blank names or undefined emails; normalize by building username from available claims (prefer user.name, then preferred_username, then `${user.given_name} ${user.family_name}`, trimming the result and falling back to a safe default like "Unknown User" if empty) and coerce email to a safe string (use user.email ?? '' then trim and optionally lowercase) before calling methods.setUser, methods.setEmail, and then methods.setAuthentication(true).
103-105:⚠️ Potential issue | 🟡 MinorSet a failure message in the catch path.
When unexpected exceptions occur, the UI can keep stale loading text because only
console.errorruns.Suggested fix
} catch (error) { + setLoadingMessage('Sign in failed. Please try again.'); console.error(`Error: centralized login; ${error}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/client/views/login.js` around lines 103 - 105, In the catch block that currently only logs the error (the lines with console.error(`Error: centralized login; ${error}`)), set the component's failure state and clear any loading text so the UI doesn't show stale loading; specifically update the relevant state setters used elsewhere in this file (for example, call the handler(s) that stop the spinner / setLoading(false) and set an error/failure message or setLoadingText('Sign-in failed') / setErrorMessage(error.message || 'Sign-in failed')), and still log the error as before.javascript/reactjs-todo-oidc/playwright.config.ts (1)
56-60:⚠️ Potential issue | 🟠 MajorExternalize PingOne issuer/client identifiers (still hardcoded).
At Line 57-60, the PingOne tenant/client identifiers are still hardcoded. This repeats the previously reported issue and keeps tests environment-coupled (also triggers secret-leak scanners).
🔧 Proposed change
+const requireEnv = (name: string): string => { + const value = process.env[name]; + if (!value) throw new Error(`Missing required env var: ${name}`); + return value; +}; + // ... WELLKNOWN_URL: - 'https://auth.pingone.ca/02fb4743-189a-4bc7-9d6c-a919edfe6447/as/.well-known/openid-configuration', - SCOPE: 'openid profile email revoke', - WEB_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', - REST_OAUTH_CLIENT: '724ec718-c41c-4d51-98b0-84a583f450f9', + requireEnv('PINGONE_WELLKNOWN_URL'), + SCOPE: process.env.PINGONE_SCOPE || 'openid profile email revoke', + WEB_OAUTH_CLIENT: requireEnv('PINGONE_WEB_OAUTH_CLIENT'), + REST_OAUTH_CLIENT: requireEnv('PINGONE_REST_OAUTH_CLIENT'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/playwright.config.ts` around lines 56 - 60, The hardcoded PingOne configuration (WELLKNOWN_URL, SCOPE, WEB_OAUTH_CLIENT, REST_OAUTH_CLIENT) should be externalized to environment-config so secrets/tenant IDs aren't in source; update the code that sets these constants to read from process.env (e.g., process.env.PINGONE_WELLKNOWN, PINGONE_SCOPE, PINGONE_WEB_OAUTH_CLIENT, PINGONE_REST_OAUTH_CLIENT) with sensible non-secret defaults or fail-fast if required, and update any Playwright/test setup that references these symbols to use the env-backed values.
🧹 Nitpick comments (3)
javascript/reactjs-todo-davinci/.env.example (2)
9-15: ClarifyINIT_PROTECTbehavior when unset, and tighten thePINGONE_ENV_IDrequirement wordingRight now
INIT_PROTECTis optional (INIT_PROTECT=), but the comments don’t explicitly say what happens when it’s empty (e.g., “Protect is disabled”). Also, thePINGONE_ENV_IDrequirement is described as “required whenProtectCollectoris present”, but in practice that likely maps toINIT_PROTECT=flow(sinceflowrelies on the collector). Consider tightening the comments so users don’t misconfigure envs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-davinci/.env.example` around lines 9 - 15, Clarify the .env.example comments: explicitly state that leaving INIT_PROTECT empty disables Protect, and document the allowed values ('bootstrap' | 'flow') and their behaviors (bootstrap initializes Protect at app startup; flow relies on the ProtectCollector). Change the PINGONE_ENV_ID comment to say it is required when INIT_PROTECT=flow (i.e., when using the ProtectCollector) rather than the vague "when ProtectCollector is present," and mention that PINGONE_ENV_ID can be left empty if INIT_PROTECT is unset or set to bootstrap.
1-7: Fix (optional): reorder env keys to satisfydotenv-linter
dotenv-linteris flagging key ordering forWEB_OAUTH_CLIENTvsWELLKNOWN_URL, and expectsAPI_URL,DEBUGGER_OFF,DEVELOPMENT,PORT, andSCOPEto appear beforeWEB_OAUTH_CLIENT. Since this is a sample template, aligning with the linter will reduce noise in CI/dev. citeturn0search0Suggested reordering
-WELLKNOWN_URL= -WEB_OAUTH_CLIENT= -API_URL=http://localhost:9443 +API_URL=http://localhost:9443 DEBUGGER_OFF=true DEVELOPMENT=true PORT=8443 SCOPE="openid profile email phone name revoke" +WEB_OAUTH_CLIENT= +WELLKNOWN_URL=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-davinci/.env.example` around lines 1 - 7, Reorder the variables in .env.example so they satisfy dotenv-linter: move WEB_OAUTH_CLIENT to appear after API_URL, DEBUGGER_OFF, DEVELOPMENT, PORT, and SCOPE; keep WELLKNOWN_URL at the top (or before the others) so the final order is WELLKNOWN_URL, API_URL, DEBUGGER_OFF, DEVELOPMENT, PORT, SCOPE, WEB_OAUTH_CLIENT, which will remove the linter ordering warning for WEB_OAUTH_CLIENT vs WELLKNOWN_URL.javascript/reactjs-todo-journey/.env.example (1)
1-11: Reorder.env.examplekeys to satisfy dotenv-linterUnorderedKeywarnings.The provided static analysis hints show multiple
dotenv-linter[UnorderedKey]warnings (e.g.,VITE_API_URLshould come beforeVITE_WEB_OAUTH_CLIENT,VITE_DEBUGGER_OFF/VITE_DEVELOPMENTbeforeVITE_PORT,VITE_REALM_PATHbeforeVITE_SCOPE, etc.). If CI treats these warnings as failures, this will break the pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-journey/.env.example` around lines 1 - 11, Reorder the environment keys in .env.example to satisfy dotenv-linter’s UnorderedKey rule by sorting the VITE_* entries into the expected order (e.g., place VITE_API_URL before VITE_WEB_OAUTH_CLIENT, move VITE_DEBUGGER_OFF and VITE_DEVELOPMENT before VITE_PORT, and put VITE_REALM_PATH before VITE_SCOPE); update the block containing VITE_WELLKNOWN_URL, VITE_WEB_OAUTH_CLIENT, VITE_API_URL, VITE_PORT, VITE_DEBUGGER_OFF, VITE_DEVELOPMENT, VITE_SCOPE, VITE_JOURNEY_LOGIN, VITE_JOURNEY_REGISTER, and VITE_REALM_PATH so the keys are consistently ordered (alphabetical or the linter’s canonical order) to eliminate UnorderedKey warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@javascript/reactjs-todo-journey/.env.example`:
- Around line 1-12: Add a VITE_SERVER_URL entry to the .env.example so
import.meta.env.VITE_SERVER_URL used in client/constants.js (which defines
SERVER_URL) is not undefined; update the file by adding a line like
VITE_SERVER_URL=http://localhost:9443 (no surrounding quotes) to match the local
development API/server URL used by VITE_API_URL.
In `@javascript/reactjs-todo-journey/README.md`:
- Around line 65-77: README defines VITE_APP_URL but the app derives SERVER_URL
from import.meta.env.VITE_SERVER_URL (see constants.js where SERVER_URL is set),
so update the README env example to use VITE_SERVER_URL instead of VITE_APP_URL
(or alternatively change constants.js to read VITE_APP_URL if that name is
preferred); also remove VITE_APP_URL from the README if unused to avoid
confusion.
---
Outside diff comments:
In `@javascript/reactjs-todo-journey/.env.example`:
- Around line 14-21: Remove the trailing spaces after the environment variable
assignments in the example file: change "VITE_INIT_PROTECT= " to
"VITE_INIT_PROTECT=" and "VITE_PINGONE_ENV_ID= " to "VITE_PINGONE_ENV_ID=" so
the blank env values contain no extra whitespace; ensure any other blank env
entries follow the same pattern.
In `@javascript/reactjs-todo-journey/README.md`:
- Around line 105-110: Update the README to use the correct Vite env var name:
replace mentions of DEBUGGER_OFF with VITE_DEBUGGER_OFF so examples and
instructions align with Vite's import.meta.env.VITE_* convention; ensure the
sentence that currently reads “set the environment variable of `DEBUGGER_OFF` to
true” instead references `VITE_DEBUGGER_OFF` and, if present, update any example
lines (e.g., `VITE_DEBUGGER_OFF=true`) to match exactly.
---
Duplicate comments:
In `@javascript/reactjs-todo-oidc/.env.example`:
- Around line 1-10: The .env.example key order violates dotenv-linter's
UnorderedKey rules; reorder the environment variables so keys appear in the
expected sequence (place SCOPE and SERVER before WEB_OAUTH_CLIENT, and ensure
API_URL, PORT, DEBUGGER_OFF, DEVELOPMENT are ordered relative to SCOPE/PORT as
the linter expects). Update the file to list keys in the canonical order:
WELLKNOWN_URL, SCOPE, SERVER, WEB_OAUTH_CLIENT (or follow your project's lint
ordering), then API_URL, PORT, DEBUGGER_OFF, DEVELOPMENT so the dotenv-linter no
longer reports UnorderedKey violations for WEB_OAUTH_CLIENT, WELLKNOWN_URL,
SCOPE, SERVER, API_URL, PORT, DEBUGGER_OFF, and DEVELOPMENT.
In `@javascript/reactjs-todo-oidc/client/views/login.js`:
- Around line 69-80: The effect in useEffect (inside handleCentralizedLogin) can
call authorize(codeParam, stateParam) more than once when codeParam/stateParam
remain in the URL; add a one-time guard to prevent duplicate callback
processing: introduce a persisted flag (e.g., processedCallbackRef or state like
callbackProcessed) and check it before calling authorize, set it immediately
after starting/succeeding the authorize flow, and skip further calls when the
flag is true; update references to codeParam/stateParam/errorParam and
setLoadingMessage accordingly so authorize is invoked exactly once per callback.
- Around line 58-63: The username/email assignment can store blank names or
undefined emails; normalize by building username from available claims (prefer
user.name, then preferred_username, then `${user.given_name}
${user.family_name}`, trimming the result and falling back to a safe default
like "Unknown User" if empty) and coerce email to a safe string (use user.email
?? '' then trim and optionally lowercase) before calling methods.setUser,
methods.setEmail, and then methods.setAuthentication(true).
- Around line 103-105: In the catch block that currently only logs the error
(the lines with console.error(`Error: centralized login; ${error}`)), set the
component's failure state and clear any loading text so the UI doesn't show
stale loading; specifically update the relevant state setters used elsewhere in
this file (for example, call the handler(s) that stop the spinner /
setLoading(false) and set an error/failure message or setLoadingText('Sign-in
failed') / setErrorMessage(error.message || 'Sign-in failed')), and still log
the error as before.
In `@javascript/reactjs-todo-oidc/playwright.config.ts`:
- Around line 56-60: The hardcoded PingOne configuration (WELLKNOWN_URL, SCOPE,
WEB_OAUTH_CLIENT, REST_OAUTH_CLIENT) should be externalized to
environment-config so secrets/tenant IDs aren't in source; update the code that
sets these constants to read from process.env (e.g.,
process.env.PINGONE_WELLKNOWN, PINGONE_SCOPE, PINGONE_WEB_OAUTH_CLIENT,
PINGONE_REST_OAUTH_CLIENT) with sensible non-secret defaults or fail-fast if
required, and update any Playwright/test setup that references these symbols to
use the env-backed values.
---
Nitpick comments:
In `@javascript/reactjs-todo-davinci/.env.example`:
- Around line 9-15: Clarify the .env.example comments: explicitly state that
leaving INIT_PROTECT empty disables Protect, and document the allowed values
('bootstrap' | 'flow') and their behaviors (bootstrap initializes Protect at app
startup; flow relies on the ProtectCollector). Change the PINGONE_ENV_ID comment
to say it is required when INIT_PROTECT=flow (i.e., when using the
ProtectCollector) rather than the vague "when ProtectCollector is present," and
mention that PINGONE_ENV_ID can be left empty if INIT_PROTECT is unset or set to
bootstrap.
- Around line 1-7: Reorder the variables in .env.example so they satisfy
dotenv-linter: move WEB_OAUTH_CLIENT to appear after API_URL, DEBUGGER_OFF,
DEVELOPMENT, PORT, and SCOPE; keep WELLKNOWN_URL at the top (or before the
others) so the final order is WELLKNOWN_URL, API_URL, DEBUGGER_OFF, DEVELOPMENT,
PORT, SCOPE, WEB_OAUTH_CLIENT, which will remove the linter ordering warning for
WEB_OAUTH_CLIENT vs WELLKNOWN_URL.
In `@javascript/reactjs-todo-journey/.env.example`:
- Around line 1-11: Reorder the environment keys in .env.example to satisfy
dotenv-linter’s UnorderedKey rule by sorting the VITE_* entries into the
expected order (e.g., place VITE_API_URL before VITE_WEB_OAUTH_CLIENT, move
VITE_DEBUGGER_OFF and VITE_DEVELOPMENT before VITE_PORT, and put VITE_REALM_PATH
before VITE_SCOPE); update the block containing VITE_WELLKNOWN_URL,
VITE_WEB_OAUTH_CLIENT, VITE_API_URL, VITE_PORT, VITE_DEBUGGER_OFF,
VITE_DEVELOPMENT, VITE_SCOPE, VITE_JOURNEY_LOGIN, VITE_JOURNEY_REGISTER, and
VITE_REALM_PATH so the keys are consistently ordered (alphabetical or the
linter’s canonical order) to eliminate UnorderedKey warnings.
🪄 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: 6680e48b-4c44-401c-982b-35accdfab7aa
⛔ Files ignored due to path filters (1)
javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
.github/workflows/CI.ymljavascript/reactjs-todo-davinci/.env.examplejavascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/context/README.mdjavascript/reactjs-todo-davinci/client/views/home.jsjavascript/reactjs-todo-davinci/e2e/davinci-login.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-logout.spec.jsjavascript/reactjs-todo-davinci/e2e/utils/demo-user.jsjavascript/reactjs-todo-davinci/webpack.config.jsjavascript/reactjs-todo-journey/.env.examplejavascript/reactjs-todo-journey/README.mdjavascript/reactjs-todo-journey/client/context/README.mdjavascript/reactjs-todo-journey/client/views/home.jsxjavascript/reactjs-todo-journey/client/views/login.jsxjavascript/reactjs-todo-journey/package.jsonjavascript/reactjs-todo-journey/vite.config.jsjavascript/reactjs-todo-login-widget/.env.examplejavascript/reactjs-todo-login-widget/README.mdjavascript/reactjs-todo-login-widget/client/constants.jsjavascript/reactjs-todo-login-widget/client/context/README.mdjavascript/reactjs-todo-login-widget/client/context/auth.context.jsjavascript/reactjs-todo-login-widget/playwright.config.tsjavascript/reactjs-todo-login-widget/webpack.config.jsjavascript/reactjs-todo-oidc/.env.examplejavascript/reactjs-todo-oidc/README.mdjavascript/reactjs-todo-oidc/client/README.mdjavascript/reactjs-todo-oidc/client/constants.jsjavascript/reactjs-todo-oidc/client/context/README.mdjavascript/reactjs-todo-oidc/client/context/protect.context.jsjavascript/reactjs-todo-oidc/client/index.jsjavascript/reactjs-todo-oidc/client/views/home.jsjavascript/reactjs-todo-oidc/client/views/login.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-todo.spec.jsjavascript/reactjs-todo-oidc/e2e/utils/demo-user.jsjavascript/reactjs-todo-oidc/playwright.config.tsjavascript/reactjs-todo-oidc/webpack.config.js
💤 Files with no reviewable changes (2)
- javascript/reactjs-todo-oidc/client/context/protect.context.js
- javascript/reactjs-todo-login-widget/client/constants.js
✅ Files skipped from review due to trivial changes (10)
- javascript/reactjs-todo-journey/client/context/README.md
- javascript/reactjs-todo-davinci/client/context/README.md
- javascript/reactjs-todo-journey/client/views/home.jsx
- javascript/reactjs-todo-journey/vite.config.js
- javascript/reactjs-todo-davinci/e2e/utils/demo-user.js
- javascript/reactjs-todo-oidc/client/views/home.js
- javascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.js
- javascript/reactjs-todo-davinci/client/views/home.js
- javascript/reactjs-todo-oidc/client/constants.js
- javascript/reactjs-todo-oidc/client/context/README.md
🚧 Files skipped from review as they are similar to previous changes (14)
- javascript/reactjs-todo-davinci/webpack.config.js
- javascript/reactjs-todo-login-widget/client/context/README.md
- javascript/reactjs-todo-davinci/e2e/davinci-login.spec.js
- .github/workflows/CI.yml
- javascript/reactjs-todo-davinci/e2e/davinci-logout.spec.js
- javascript/reactjs-todo-davinci/README.md
- javascript/reactjs-todo-oidc/e2e/oidc-todo.spec.js
- javascript/reactjs-todo-oidc/webpack.config.js
- javascript/reactjs-todo-login-widget/README.md
- javascript/reactjs-todo-oidc/e2e/utils/demo-user.js
- javascript/reactjs-todo-login-widget/playwright.config.ts
- javascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.js
- javascript/reactjs-todo-oidc/client/index.js
- javascript/reactjs-todo-journey/client/views/login.jsx
ancheetah
force-pushed
the
SDKS-4902-update-js-oidc
branch
from
ae4ca1d to
bb7d1d7
Compare
cerebrl
previously approved these changes
Apr 29, 2026
Contributor
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This all looks good. I just have one question about the centralized login logic in the journey app.
Comment on lines
124
to
152
| @@ -134,7 +144,10 @@ export default function Login() { | |||
| return ( | |||
| <div className="cstm_container_v-centered container-fluid d-flex align-items-center"> | |||
| <div className="w-100"> | |||
| <Loading message={state.loadingMessage} /> | |||
| <BackHome /> | |||
| <Card> | |||
| <Loading message={loadingMessage} /> | |||
| </Card> | |||
| </div> | |||
| </div> | |||
Contributor
There was a problem hiding this comment.
I'd like to actually remove all of this centralized login support, since we have a dedicated OIDC Client app. Do you want me to write up a dedicated ticket for it, or would you like to go ahead and remove it now?
Contributor
Author
There was a problem hiding this comment.
I can remove it now, no problem
Contributor
Author
There was a problem hiding this comment.
Ok updated. It's in the last commit. I need another approval from someone before I can merge.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
javascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.js (2)
26-26: ⚡ Quick winReduce brittleness in
client_idassertion.Line 26 hard-codes a tenant-specific UUID; this makes the test fragile across environment/client rotations. Assert presence/format instead of one fixed value.
Suggested assertion update
- expect(authorizeUrl.searchParams.get('client_id')).toBe('724ec718-c41c-4d51-98b0-84a583f450f9'); + expect(authorizeUrl.searchParams.get('client_id')).toMatch( + /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.js` at line 26, The test currently asserts a hard-coded tenant client_id via authorizeUrl.searchParams.get('client_id') which is brittle; change the assertion to verify the query param exists and matches a UUID format (or at minimum is non-empty). Update the expectation that reads authorizeUrl.searchParams.get('client_id') to assert truthiness and match a UUID regex (e.g. validate pattern of 8-4-4-4-12 hex groups) instead of comparing to the fixed UUID string so the test is resilient to client rotations.
80-81: Simplify logout URL verification by waiting only for the final URL.The intermediate
/logoutURL wait is unnecessary. When the logout action redirects to/logoutthen back to the base URL, a single wait for the final URL is sufficient. Playwright automatically handles redirect chains, so explicitly waiting for intermediate URLs adds complexity without benefit.Proposed simplification
- await page.waitForURL(BASE_URL + '/logout'); - await page.waitForURL(BASE_URL); + await page.waitForURL(new RegExp(`^${BASE_URL}/?$`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.js` around lines 80 - 81, Remove the redundant intermediate waitForURL call: delete the await page.waitForURL(BASE_URL + '/logout') invocation and leave a single await page.waitForURL(BASE_URL) so Playwright follows the redirect chain automatically; locate the two occurrences of page.waitForURL (the calls with BASE_URL + '/logout' and BASE_URL) in the oidc-login-pingone.spec.js logout flow and remove the former, keeping only the final waitForURL(BASE_URL).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@javascript/reactjs-todo-davinci/.env.example`:
- Around line 1-7: Reorder the keys in .env.example to satisfy dotenv-linter's
UnorderedKey rule: sort the environment variable entries (WELLKNOWN_URL,
WEB_OAUTH_CLIENT, API_URL, DEBUGGER_OFF, DEVELOPMENT, PORT, SCOPE) into the
expected alphabetical order and update the file accordingly so CI dotenv-linter
checks pass; ensure you only reorder lines and do not change values or quoting.
In `@javascript/reactjs-todo-journey/package.json`:
- Around line 22-28: Package `@forgerock/protect` is missing from package.json
causing import failures in client/context/protect.context.js and
client/components/journey/protect.jsx; add "@forgerock/protect" to the
"dependencies" section of package.json (use the same versioning pattern as the
other Forgerock packages, e.g., "latest" or a specific semver), run npm/yarn
install, and confirm imports in protect.context.js and protect.jsx resolve
correctly.
In `@javascript/reactjs-todo-oidc/README.md`:
- Line 52: The scopes listed for PingOne are inconsistent: the README scope list
includes "revoke" while the .env example omits it (and the same inconsistency
appears later around the other scope mentions). Update the README and .env
sample to match: either add "revoke" to the .env sample scopes or remove
"revoke" from the README scope lists so both show the same scopes; ensure all
occurrences (the initial scope list and the later scope references) are updated
to the chosen canonical scope set to avoid confusion for PingOne users.
---
Nitpick comments:
In `@javascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.js`:
- Line 26: The test currently asserts a hard-coded tenant client_id via
authorizeUrl.searchParams.get('client_id') which is brittle; change the
assertion to verify the query param exists and matches a UUID format (or at
minimum is non-empty). Update the expectation that reads
authorizeUrl.searchParams.get('client_id') to assert truthiness and match a UUID
regex (e.g. validate pattern of 8-4-4-4-12 hex groups) instead of comparing to
the fixed UUID string so the test is resilient to client rotations.
- Around line 80-81: Remove the redundant intermediate waitForURL call: delete
the await page.waitForURL(BASE_URL + '/logout') invocation and leave a single
await page.waitForURL(BASE_URL) so Playwright follows the redirect chain
automatically; locate the two occurrences of page.waitForURL (the calls with
BASE_URL + '/logout' and BASE_URL) in the oidc-login-pingone.spec.js logout flow
and remove the former, keeping only the final waitForURL(BASE_URL).
🪄 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: d5e5c117-2b6b-4355-a65d-d771689f55e7
⛔ Files ignored due to path filters (1)
javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
.github/workflows/CI.ymljavascript/reactjs-todo-davinci/.env.examplejavascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/context/README.mdjavascript/reactjs-todo-davinci/client/views/home.jsjavascript/reactjs-todo-davinci/e2e/davinci-login.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-logout.spec.jsjavascript/reactjs-todo-davinci/e2e/utils/demo-user.jsjavascript/reactjs-todo-davinci/webpack.config.jsjavascript/reactjs-todo-journey/.env.examplejavascript/reactjs-todo-journey/README.mdjavascript/reactjs-todo-journey/client/constants.jsjavascript/reactjs-todo-journey/client/context/README.mdjavascript/reactjs-todo-journey/client/views/home.jsxjavascript/reactjs-todo-journey/client/views/login.jsxjavascript/reactjs-todo-journey/e2e/centralized-login.spec.jsjavascript/reactjs-todo-journey/package.jsonjavascript/reactjs-todo-journey/playwright.config.jsjavascript/reactjs-todo-journey/vite.config.jsjavascript/reactjs-todo-login-widget/.env.examplejavascript/reactjs-todo-login-widget/README.mdjavascript/reactjs-todo-login-widget/client/constants.jsjavascript/reactjs-todo-login-widget/client/context/README.mdjavascript/reactjs-todo-login-widget/client/context/auth.context.jsjavascript/reactjs-todo-login-widget/playwright.config.tsjavascript/reactjs-todo-login-widget/webpack.config.jsjavascript/reactjs-todo-oidc/.env.examplejavascript/reactjs-todo-oidc/README.mdjavascript/reactjs-todo-oidc/client/README.mdjavascript/reactjs-todo-oidc/client/constants.jsjavascript/reactjs-todo-oidc/client/context/README.mdjavascript/reactjs-todo-oidc/client/context/protect.context.jsjavascript/reactjs-todo-oidc/client/index.jsjavascript/reactjs-todo-oidc/client/views/home.jsjavascript/reactjs-todo-oidc/client/views/login.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-login-pingone.spec.jsjavascript/reactjs-todo-oidc/e2e/oidc-todo.spec.jsjavascript/reactjs-todo-oidc/e2e/utils/demo-user.jsjavascript/reactjs-todo-oidc/playwright.config.tsjavascript/reactjs-todo-oidc/webpack.config.js
💤 Files with no reviewable changes (4)
- javascript/reactjs-todo-login-widget/client/constants.js
- javascript/reactjs-todo-journey/e2e/centralized-login.spec.js
- javascript/reactjs-todo-journey/playwright.config.js
- javascript/reactjs-todo-oidc/client/context/protect.context.js
✅ Files skipped from review due to trivial changes (14)
- javascript/reactjs-todo-login-widget/client/context/README.md
- javascript/reactjs-todo-journey/client/context/README.md
- javascript/reactjs-todo-journey/vite.config.js
- javascript/reactjs-todo-journey/client/views/home.jsx
- javascript/reactjs-todo-davinci/webpack.config.js
- javascript/reactjs-todo-oidc/e2e/oidc-login-pingam.spec.js
- javascript/reactjs-todo-davinci/client/views/home.js
- javascript/reactjs-todo-oidc/client/views/home.js
- javascript/reactjs-todo-davinci/e2e/davinci-logout.spec.js
- javascript/reactjs-todo-davinci/e2e/utils/demo-user.js
- .github/workflows/CI.yml
- javascript/reactjs-todo-davinci/client/context/README.md
- javascript/reactjs-todo-oidc/e2e/utils/demo-user.js
- javascript/reactjs-todo-oidc/client/context/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- javascript/reactjs-todo-login-widget/client/context/auth.context.js
- javascript/reactjs-todo-login-widget/webpack.config.js
- javascript/reactjs-todo-davinci/README.md
- javascript/reactjs-todo-davinci/e2e/davinci-login.spec.js
- javascript/reactjs-todo-journey/README.md
- javascript/reactjs-todo-oidc/client/views/login.js
Comment on lines
+1
to
7
| WELLKNOWN_URL= | ||
| WEB_OAUTH_CLIENT= | ||
| API_URL=http://localhost:9443 | ||
| DEBUGGER_OFF=true | ||
| DEVELOPMENT=true | ||
| PORT=8443 | ||
| SCOPE="openid profile email phone name revoke" |
There was a problem hiding this comment.
Reorder env keys to satisfy dotenv-linter
Line 1 through Line 7 currently trigger [UnorderedKey] warnings. If dotenv-linter is enforced in CI, this can fail checks even though values are correct.
Suggested reorder
-WELLKNOWN_URL=
-WEB_OAUTH_CLIENT=
API_URL=http://localhost:9443
DEBUGGER_OFF=true
DEVELOPMENT=true
PORT=8443
SCOPE="openid profile email phone name revoke"
+WEB_OAUTH_CLIENT=
+WELLKNOWN_URL=📝 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.
Suggested change
| WELLKNOWN_URL= | |
| WEB_OAUTH_CLIENT= | |
| API_URL=http://localhost:9443 | |
| DEBUGGER_OFF=true | |
| DEVELOPMENT=true | |
| PORT=8443 | |
| SCOPE="openid profile email phone name revoke" | |
| API_URL=http://localhost:9443 | |
| DEBUGGER_OFF=true | |
| DEVELOPMENT=true | |
| PORT=8443 | |
| SCOPE="openid profile email phone name revoke" | |
| WEB_OAUTH_CLIENT= | |
| WELLKNOWN_URL= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 2-2: [UnorderedKey] The WEB_OAUTH_CLIENT key should go before the WELLKNOWN_URL key
(UnorderedKey)
[warning] 3-3: [UnorderedKey] The API_URL key should go before the WEB_OAUTH_CLIENT key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The DEBUGGER_OFF key should go before the WEB_OAUTH_CLIENT key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The DEVELOPMENT key should go before the WEB_OAUTH_CLIENT key
(UnorderedKey)
[warning] 6-6: [UnorderedKey] The PORT key should go before the WEB_OAUTH_CLIENT key
(UnorderedKey)
[warning] 7-7: [UnorderedKey] The SCOPE key should go before the WEB_OAUTH_CLIENT key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javascript/reactjs-todo-davinci/.env.example` around lines 1 - 7, Reorder the
keys in .env.example to satisfy dotenv-linter's UnorderedKey rule: sort the
environment variable entries (WELLKNOWN_URL, WEB_OAUTH_CLIENT, API_URL,
DEBUGGER_OFF, DEVELOPMENT, PORT, SCOPE) into the expected alphabetical order and
update the file accordingly so CI dotenv-linter checks pass; ensure you only
reorder lines and do not change values or quoting.
|
|
||
| ##### Resources (scopes) | ||
|
|
||
| 1. openid profile email phone name revoke |
There was a problem hiding this comment.
Scope examples are inconsistent between PingOne setup and .env sample.
Line 52 includes revoke, but the .env example omits it. This can mislead PingOne users when following setup steps.
Suggested doc tweak
-SCOPE="openid profile email"
+SCOPE="openid profile email revoke"Also applies to: 62-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javascript/reactjs-todo-oidc/README.md` at line 52, The scopes listed for
PingOne are inconsistent: the README scope list includes "revoke" while the .env
example omits it (and the same inconsistency appears later around the other
scope mentions). Update the README and .env sample to match: either add "revoke"
to the .env sample scopes or remove "revoke" from the README scope lists so both
show the same scopes; ensure all occurrences (the initial scope list and the
later scope references) are updated to the chosen canonical scope set to avoid
confusion for PingOne users.
5865281 to
099a003
Compare
ancheetah
force-pushed
the
SDKS-4902-update-js-oidc
branch
from
099a003 to
5e7f107
Compare
SteinGabriel
approved these changes
Apr 29, 2026
cerebrl
approved these changes
Apr 30, 2026
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
3 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.
https://pingidentity.atlassian.net/browse/SDKS-4902
OIDC JS Sample App:
Other JS Sample Apps:
/contextfolder.env.exampletemplates and aligns READMEsSummary by CodeRabbit