refactor(reactjs-todo-login-widget): update configure to accommodate features like logger, middleware, storage from login widget in the sample app - #127
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. A rabbit checks the login gate, 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
| 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.
375ed27 to
b69cea0
Compare
vatsalparikh
changed the title
fbd116d to
36f1aab
Compare
192ef98 to
936288e
Compare
094f1ba
094f1ba to
26220ac
Compare
26220ac to
2bba882
Compare
4 participants
Summary by CodeRabbit
New Features
Bug Fixes