refactor(reactjs-todo-login-widget): update configure to accommodate … - #127
refactor(reactjs-todo-login-widget): update configure to accommodate …#127vatsalparikh wants to merge 2 commits into
Conversation
…features like logger, middleware, storage from login widget in the sample app
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe React login widget now uses expanded SDK configuration, request middleware, session storage, and environment-provided logging. Playwright coverage validates authentication options, logout, protection, token storage, middleware behavior, and protected todo operations. ChangesLogin Widget integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant LoginWidget
participant JourneyMiddleware
participant OIDCMiddleware
participant Journey
participant OIDC
Browser->>LoginWidget: Start authentication
LoginWidget->>JourneyMiddleware: Send Journey action
JourneyMiddleware->>Journey: Add X-Session-ID and continue request
LoginWidget->>OIDCMiddleware: Send OIDC action
OIDCMiddleware->>OIDC: Classify and continue request
LoginWidget->>LoginWidget: Renew and retrieve user tokens
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
javascript/reactjs-todo-login-widget/package.jsonParsing error: Unexpected token : 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
javascript/reactjs-todo-login-widget/playwright.config.ts (1)
47-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
REST_OAUTH_SECRETto the AIC todo API or use a public token source.AIC token validation sends
Basic Authorization: CONFIDENTIAL_CLIENT, whose value isREST_OAUTH_CLIENT:REST_OAUTH_SECRET, but the Playwright todo-api web server only setsREST_OAUTH_CLIENT. Protected/usersand/todosrequests will have invalid Basic auth and fail validation.🤖 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/playwright.config.ts` around lines 47 - 53, Update the Playwright `env` configuration used by the AIC todo API to provide `REST_OAUTH_SECRET` alongside `REST_OAUTH_CLIENT`, using the matching configured secret so Basic authorization validation succeeds for protected `/users` and `/todos` requests; alternatively configure a public token source if that is the established setup.
🤖 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`:
- Line 59: Update the oidcClient.oauthThreshold configuration to use the value
60, representing a 60-second renewal window, and revise its JSDoc description to
specify seconds rather than milliseconds.
In `@javascript/reactjs-todo-login-widget/e2e/config-logger.spec.js`:
- Around line 19-23: Update the assertion after the authorizeRequest wait to use
Playwright’s expect.poll around the consoleLines debug check, allowing retries
until a line with type "debug" appears; keep the existing authorizeRequest
navigation flow unchanged.
In `@javascript/reactjs-todo-login-widget/e2e/config-middleware.spec.js`:
- Around line 54-58: Update the OIDC login test around the TOKEN_EXCHANGE and
AUTHORIZE assertions to capture the browser network request made during token
exchange or authorization, then assert its x-session-id header matches the
Journey request header. Retain the existing middleware log assertions while
using the captured request to validate correlation-header propagation.
---
Outside diff comments:
In `@javascript/reactjs-todo-login-widget/playwright.config.ts`:
- Around line 47-53: Update the Playwright `env` configuration used by the AIC
todo API to provide `REST_OAUTH_SECRET` alongside `REST_OAUTH_CLIENT`, using the
matching configured secret so Basic authorization validation succeeds for
protected `/users` and `/todos` requests; alternatively configure a public token
source if that is the established setup.
🪄 Autofix
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 Plus
Run ID: ed13795b-c551-403a-a800-dcd89279cb22
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
javascript/reactjs-todo-login-widget/client/index.jsjavascript/reactjs-todo-login-widget/e2e/config-logger.spec.jsjavascript/reactjs-todo-login-widget/e2e/config-middleware.spec.jsjavascript/reactjs-todo-login-widget/e2e/config-oidc-options.spec.jsjavascript/reactjs-todo-login-widget/e2e/logout.spec.jsjavascript/reactjs-todo-login-widget/e2e/protect.spec.jsjavascript/reactjs-todo-login-widget/e2e/storage-session.spec.jsjavascript/reactjs-todo-login-widget/e2e/todos-crud.spec.jsjavascript/reactjs-todo-login-widget/e2e/todos-navigation.spec.jsjavascript/reactjs-todo-login-widget/package.jsonjavascript/reactjs-todo-login-widget/playwright.config.tsjavascript/reactjs-todo-login-widget/webpack.config.js
| * - oidcClient.clientId: the OAuth 2.0 client registered in Ping AM | ||
| * - oidcClient.redirectUri: URI this app redirects to after OAuth authorization | ||
| * - oidcClient.scope: the OAuth 2.0 scopes requested from Ping AM | ||
| * - oidcClient.oauthThreshold: ms before expiry to trigger background renewal |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
version='0.0.0-beta-20260804174729'
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
tarball_url="$(
curl --fail --silent --show-error \
"https://registry.npmjs.org/%40forgerock%2Flogin-widget/${version}" |
jq -er '.dist.tarball'
)"
curl --fail --silent --show-error -L "$tarball_url" |
tar -xz -C "$tmp_dir"
rg -n -i -C 4 'oauthThreshold' "$tmp_dir/package"Repository: ForgeRock/sdk-sample-apps
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
version='0.0.0-beta-20260804174729'
tarball_url="$(
curl --fail --silent --show-error \
"https://registry.npmjs.org/%40forgerock%2Flogin-widget/${version}" |
jq -er '.dist.tarball'
)"
curl --fail --silent --show-error -L "$tarball_url" > "$tmp_dir/tarball.tgz"
tar -tzf "$tmp_dir/tarball.tgz" | sed -n '1,80p'
# Search only small non-minified/config text files for explicit oauthThreshold language.
find "$tmp_dir/package" -maxdepth 3 \( -type f \) \
\( -name '*.md' -o -name '*.txt' -o -name 'package.json' \) \
-print0 | xargs -0 rg -n -i -C 3 'oauthThreshold|renew|threshold' || true
# Search source with whitespace-sensitive literal and count matches, skipping minified-looking files.
python3 - <<'PY' "$tmp_dir/package"
from pathlib import Path
import re, sys
root=Path(sys.argv[1])
ids=re.compile(r'''['"`]oauthThreshold['"`]\s*:\s*(\d+)''')
kw=re.compile(r'oauthThreshold', re.I)
for p in root.rglob('*'):
if not p.is_file() or len(p.read_bytes()) > 2_000_000:
continue
text=p.read_text(errors='ignore')
for i,line in enumerate(text.splitlines(),1):
if 'oauthThreshold' in line or re.search(r'oauthThreshold', line, re.I):
print(f"{p}:{i}:{line[:240]}")
PY
# Read client.js only the relevant configured lines if present.
if [ -f javascript/reactjs-todo-login-widget/client/index.js ]; then
sed -n '50,95p' javascript/reactjs-todo-login-widget/client/index.js | cat -n
fiRepository: ForgeRock/sdk-sample-apps
Length of output: 38124
🌐 Web query:
ForgeRock login-widget sdk documentation oidcClient.oauthThreshold seconds
💡 Result:
In the context of the ForgeRock (now Ping Identity) Login Widget and the associated JavaScript SDK, the oauthThreshold property is defined as a threshold, measured in seconds, used to trigger the refresh of an OAuth 2.0 token before the access_token expires [1][2][3]. Key details regarding this property include: - Default Value: It defaults to 30 seconds [1][2][3]. - Functionality: If tokens are not within this threshold (i.e., they are close to expiring), the SDK will attempt a silent refresh of the tokens [4]. - Recent Changes: In some versions of the SDK, this property may be managed internally by the @forgerock/oidc-client package [5]. Some documentation notes that while the property may still be accepted in configuration, it is sometimes ignored in favor of internal management [4]. When configuring the SDK, you can typically set this value within your configuration object, though you should verify your specific version's implementation in the official developer documentation [1][6][2].
Citations:
- 1: https://developer.pingidentity.com/login-widget/login-widget/tutorial/04-configure-sdk.html
- 2: https://docs.pingidentity.com/sdks/latest/sdks/sdkconfiguration/configure-sdk-javascript.html
- 3: https://cdn-docs.pingidentity.com/archive/pdf/login-widget/login_widget.pdf
- 4: https://github.com/ForgeRock/ping-javascript-sdk/blob/main/interface_mapping.md
- 5: https://developer.pingidentity.com/login-widget/release-notes/breaking.html
- 6: https://forgerock.github.io/ping-javascript-sdk/interfaces/_forgerock_oidc-client.index.OidcConfig.html
Use seconds for oidcClient.oauthThreshold.
oauthThreshold is a threshold in seconds, not milliseconds. Set this to 60 for a 60-second renewal window and update the JSDoc at line 59 to match; otherwise, tokens expiring sooner than 16.7 hours will enter the renewal window immediately.
🤖 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` at line 59, Update the
oidcClient.oauthThreshold configuration to use the value 60, representing a
60-second renewal window, and revise its JSDoc description to specify seconds
rather than milliseconds.
| const authorizeRequest = page.waitForRequest((request) => request.url().includes('/authorize?')); | ||
| await page.goto('https://localhost:8443/'); | ||
| await authorizeRequest; | ||
|
|
||
| expect(consoleLines.some((line) => line.type === 'debug')).toBe(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced test and nearby code.
file="javascript/reactjs-todo-login-widget/e2e/config-logger.spec.js"
if [ -f "$file" ]; then
echo "== file exists =="
wc -l "$file"
echo "== relevant lines =="
sed -n '1,120p' "$file" | cat -n
else
echo "file missing: $file"
echo "== find similar =="
fd -a 'config-logger\.spec\.js|todo-login-widget|e2e' .
fi
echo "== Playwright dependency/version mentions =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock poetry.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '"`@playwright/test`"|playwright' "$f" || true
fi
done
echo "== e2e config/logger tests around consoleLines expectations =="
if [ -d javascript/reactjs-todo-login-widget/e2e ]; then
rg -n "consoleLines|expect\.poll|waitForRequest|authorize\?" javascript/reactjs-todo-login-widget/e2e -S
fiRepository: ForgeRock/sdk-sample-apps
Length of output: 4092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral/source check for the relevant Playwright assertion semantics from installed package, if present.
if [ -d javascript/reactjs-todo-login-widget/node_modules/@playwright/test ]; then
pkg="javascript/reactjs-todo-login-widget/node_modules/@playwright/test"
echo "== playwright package present =="
find "$pkg" -maxdepth 4 -name '*.d.ts' | head -50
rg -n "not waiting for .*condition|non-retrying|expect\.poll|poll|expect:\s*class|toBe\(" "$pkg" -S | head -120 || true
else
echo "== playwright test package not present in repo =="
fiRepository: ForgeRock/sdk-sample-apps
Length of output: 213
🌐 Web query:
Playwright expect values are non retrying use expect poll asynchronous
💡 Result:
In Playwright, standard assertions (like expect(locator).toBeVisible) are auto-retrying and asynchronous, meaning they will automatically retry until they pass or reach the specified timeout [1][2]. However, some assertions do not auto-retry, and for these or other complex asynchronous conditions, you should use expect.poll to handle retries [1][3]. expect.poll is specifically designed to convert a synchronous or asynchronous function into an asynchronous polling assertion [1][4]. It repeatedly executes a provided function and evaluates the result against an expectation until it passes or times out [5][3]. Key characteristics of expect.poll: - Asynchronous: Because it is a polling mechanism, it must be awaited [1][4]. - Configurable: You can set custom timeout durations (defaulting to 5 seconds) and define custom polling intervals (the default intervals are milliseconds) [1][5][3]. - Flexibility: It can be combined with expect.soft to perform soft assertions within the polling logic, allowing the test to continue even if a specific poll iteration fails [1][3]. Example of using expect.poll: await expect.poll(async => { const response = await page.request.get('https://api.example.com'); return response.status; }, { message: 'ensure API request succeeds', timeout: 10000, }).toBe(200);
Citations:
- 1: https://playwright.dev/docs/test-assertions
- 2: https://playwright.dev/docs/next/test-assertions
- 3: https://github.com/microsoft/playwright/blob/main/docs/src/test-assertions-js.md
- 4: https://github.com/microsoft/playwright/blob/4d289016/docs/src/test-assertions-js.md
- 5: https://github.com/microsoft/playwright/blob/main/packages/playwright/src/matchers/expect.ts
🌐 Web query:
Playwright test assertions direct values non retrying expect values non-retrying
💡 Result:
In Playwright, non-retrying assertions are used to validate immediate, synchronous values where waiting for an asynchronous state change is unnecessary [1]. Unlike "web-first" assertions (which automatically retry until a condition is met or a timeout is reached), these non-retrying assertions evaluate the condition exactly once [2][1][3]. Key Characteristics and Usage: - No Auto-retry: Because they do not retry, these assertions are susceptible to flakiness if used against asynchronous web elements that are not yet ready [2][4]. They should generally be reserved for static values or variables that are already available in memory [1]. - Implementation: They are invoked using the standard expect(value) syntax [1]. - Examples of non-retrying matchers include expect(value).toBe, expect(value).toEqual, expect(value).toBeDefined, expect(value).toContain, and expect(value).toMatch [2][5]. - Best Practices: Playwright documentation strongly recommends using auto-retrying, web-first assertions whenever possible for testing web page elements [2][1][3]. If a complex asynchronous condition needs to be checked and standard web-first assertions are insufficient, use expect.poll or expect.toPass instead of relying on basic non-retrying matchers [2][4]. Distinction from Soft Assertions: It is important to distinguish between retrying behavior and assertion severity. A non-retrying assertion can still be used as a "soft" assertion by using expect.soft(value).toBe(...) [6]. While expect.soft will continue test execution upon failure rather than aborting immediately, it preserves the retrying behavior of the underlying matcher [6]. Therefore, a soft assertion applied to a web-first matcher (e.g., expect.soft(locator).toHaveText) will still retry, whereas a soft assertion applied to a generic value matcher (e.g., expect.soft(value).toBe) will remain non-retrying [6].
Citations:
- 1: https://playwright.dev/docs/writing-tests
- 2: https://playwright.dev/docs/test-assertions
- 3: https://www.checklyhq.com/docs/learn/playwright/assertions/
- 4: https://playwright.dev/docs/next/test-assertions
- 5: https://github.com/microsoft/playwright/blob/main/docs/src/test-assertions-js.md
- 6: https://qaskills.sh/blog/playwright-soft-assertions-expect-guide
Wait for the debug console event.
waitForRequest() completes when the browser issues the authorize request. The SDK debug message can arrive after the request event. Use await expect.poll(() => consoleLines.some((line) => line.type === 'debug')).toBe(true) so Playwright retries the asynchronous condition.
🤖 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/e2e/config-logger.spec.js` around lines
19 - 23, Update the assertion after the authorizeRequest wait to use
Playwright’s expect.poll around the consoleLines debug check, allowing retries
until a line with type "debug" appears; keep the existing authorizeRequest
navigation flow unchanged.
| // OIDC middleware saw the OIDC client's requests (token exchange after login). | ||
| await expect | ||
| .poll(() => logText().some((text) => text.includes('[oidc-middleware] TOKEN_EXCHANGE'))) | ||
| .toBe(true); | ||
| expect(logText().some((text) => text.includes('[oidc-middleware] AUTHORIZE'))).toBe(true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate files:"
git ls-files | rg 'javascript/reactjs-todo-login-widget/e2e/config-middleware.spec.js|javascript/reactjs-todo-login-widget|oidc-middleware|x-session-id|X-Session-ID' || true
echo
echo "Target file outline and relevant lines:"
if [ -f javascript/reactjs-todo-login-widget/e2e/config-middleware.spec.js ]; then
wc -l javascript/reactjs-todo-login-widget/e2e/config-middleware.spec.js
sed -n '1,140p' javascript/reactjs-todo-login-widget/e2e/config-middleware.spec.js | cat -n
fi
echo
echo "Search for header/header setup and OIDC middleware logs:"
rg -n "X-Session-ID|x-session-id|TOKEN_EXCHANGE|AUTHORIZE|oidc-middleware|session-id|Journey" javascript/reactjs-todo-login-widget -S || true
echo
echo "Package/config references to middleware:"
fd -a 'package.json|middleware|oidc|config|src|app' javascript/reactjs-todo-login-widget -t f | sed 's#^\./##' | head -200Repository: ForgeRock/sdk-sample-apps
Length of output: 11842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only behavioral/source-shape verifier: determine whether the target test has any network-request-based
# assertions covering the OIDC token exchange request or X-Session-ID propagation.
python3 - <<'PY'
from pathlib import Path
import re
p = Path('javascript/reactjs-todo-login-widget/e2e/config-middleware.spec.js')
if not p.exists():
print('missing target')
raise SystemExit(0)
text = p.read_text()
checks = {
'token_exchange_log_assertion': 'TOKEN_EXCHANGE' in text,
'authorize_log_assertion': 'AUTHORIZE' in text,
'journey_header_assertion': bool(re.search(r'Journey|X-Session-ID|x-session-id', text)),
'network_request_capture_patterns': bool(re.search(r'\.intercept|waitForRequest|response\(req\)|\.headers|headers\(\)', text)),
'x_session_id_assertion': bool(re.search(r'x-session-id|X-Session-ID', text)),
'token_exchange_request_assertion': bool(re.search(r'TOKEN_EXCHANGE|token.*exchange|token-exchange', text)),
}
for k, v in checks.items():
print(f'{k}={v}')
# Show all test step blocks with relevant matching lines.
for m in re.finditer(r'it\([^\)]*\)|test\([^\)]*\)', text):
seg_start = m.start()
seg = re.search(r'\n\}\n\}\)', text[seg_start:])
end = (seg_start + seg.end()) if seg else min(seg_start + 6000, len(text))
segment = text[seg_start:end]
if any(term in segment for term in ['TOKEN_EXCHANGE', 'AUTHORIZE', 'token_exchange', 'headers(', 'intercept', 'waitForRequest']):
print(f'\n--- block {seg_start} ---')
print('\n'.join(f'{i+1:4}: {line}' for i,line in enumerate(segment.splitlines()) if any(q in line for q in ['TOKEN_EXCHANGE', 'AUTHORIZE', 'token_exchange', 'headers(', 'intercept', 'waitForRequest', 'expect', 'logText'])))
PYRepository: ForgeRock/sdk-sample-apps
Length of output: 1400
Assert the OIDC correlation header on a network request.
The TOKEN_EXCHANGE log only shows that oidcMiddleware ran. Capture the browser request made during token exchange or authorization, then assert that x-session-id equals the Journey request header. This validates OIDC header propagation and the shared correlation ID.
🤖 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/e2e/config-middleware.spec.js` around
lines 54 - 58, Update the OIDC login test around the TOKEN_EXCHANGE and
AUTHORIZE assertions to capture the browser network request made during token
exchange or authorization, then assert its x-session-id header matches the
Journey request header. Retain the existing middleware log assertions while
using the captured request to validate correlation-header propagation.
…ound renew true after login
1 participant
…features like logger, middleware, storage from login widget in the sample app
Summary by CodeRabbit
New Features
Bug Fixes