feat(reactjs-todo-davinci): add ValidatedPasswordCollector support#116
Conversation
|
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:
📝 WalkthroughWalkthroughAdds ChangesDaVinci validated password support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant Password
participant DaVinciClient
User->>Form: Open validated password form
Form->>Password: Pass validator and verify
User->>Password: Enter password
Password->>DaVinciClient: Validate collector
DaVinciClient-->>Password: Return validation errors
Password-->>User: Render requirements or errors
User->>Password: Enter confirmation
Password-->>User: Render mismatch state
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/package.json`:
- Line 42: The package.json currently pins `@forgerock/davinci-client` to a
preview pkg.pr.new URL; replace that entry in
javascript/reactjs-todo-davinci/package.json so the dependency references the
published npm version string (e.g. "`@forgerock/davinci-client`":
">=<stable-version>" or a specific semver) instead of the pkg.pr.new URL, then
regenerate the lockfile (run npm install/npm ci locally) so package-lock.json no
longer contains a resolved pkg.pr.new URL; ensure the final package-lock.json
committed to the repo points to the stable registry artifact so CI installs
reliably.
🪄 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: 518e875a-ed2f-4793-b3ca-eb263fe249c5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
javascript/reactjs-todo-davinci/client/components/davinci-client/form.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/hooks/davinci.hook.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/password.jsjavascript/reactjs-todo-davinci/e2e/davinci-validated-password.spec.jsjavascript/reactjs-todo-davinci/package.jsonjavascript/reactjs-todo-davinci/playwright.config.ts
41d7602 to
cf50b48
Compare
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Left some comments about error handling.
| /** | ||
| * Override configs keyed by clientId URL param — allows tests to target a | ||
| * different PingOne tenant without recompiling the app. | ||
| */ | ||
| const CLIENT_CONFIGS = { | ||
| 'fb456db5-2e08-46d3-adf0-05bf8d26ad60': { | ||
| clientId: 'fb456db5-2e08-46d3-adf0-05bf8d26ad60', | ||
| redirectUri: `${window.location.origin}/callback.html`, | ||
| scope: 'openid profile email phone name revoke', | ||
| serverConfig: { | ||
| wellknown: | ||
| 'https://auth.pingone.ca/356a254c-cba3-4ade-be1a-860136e8df01/as/.well-known/openid-configuration', | ||
| }, | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why are we hardcoding a tenant here? I feel like this should be moved into a test and not something exposed to the customer.
| } catch (error) { | ||
| console.error('Error creating validator for collector:', error); | ||
| return () => []; | ||
| } |
There was a problem hiding this comment.
I don't believe validate can throw. Can we remove this? If we want to handle errors then we should look for them like if (error) in validateResult {}.
|
|
||
| if (validator) { | ||
| const errors = validator(value); | ||
| setValidationErrors(Array.isArray(errors) ? errors : []); |
There was a problem hiding this comment.
Do we need to do the Array.isArray check? Validators should always return an array even if it's empty.
| const result = updater(value); | ||
| if (result && result.error) { | ||
| console.error('Error updating password collector:', result.error.message); | ||
| } | ||
|
|
||
| // Keep confirm error in sync as the primary value changes | ||
| if (confirmValue && value !== confirmValue) { | ||
| setConfirmError('Passwords do not match'); | ||
| } else if (confirmValue) { | ||
| setConfirmError(''); | ||
| } |
There was a problem hiding this comment.
Is there anything blocking the user from submitting the form if there is an error? Or are these just UI hints?
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Approved. I added a do not merge label. Let's remember to update the deps to latest once we release and this is ready.
- Upgrades @forgerock/davinci-client to latest (2.0.0) which ships ValidatedPasswordCollector with password policy validation support - Extends the Password component to render a static requirements list from the password policy, inline validation errors on keystroke, and an optional confirm field with real-time mismatch feedback - Wires ValidatedPasswordCollector into form.js via a new validator function exposed from useDavinci with proper error handling - Adds a second webServer (port 8444, 356a254c tenant) in playwright config for ValidatedPasswordCollector e2e tests; skipped in CI where only the 02fb4743 tenant is available
…ssword tests on port 8443
…ed collectors list
- Add defensive null check in request listener to handle non-JSON requests - Use optional chaining for safe property access - Fix swapped assertions in collector test (was checking bootstrap log instead of collector log) - Tests now pass with beta packages which have different request patterns These bugs were pre-existing but masked by original package's request order. Beta package's different request pattern exposed them.
- Upgrade @playwright/test from 1.41.2 to 1.60.0 for better ESM support - Remove 'type: module' from reactjs-todo-journey/package.json - Convert playwright.config.js to CommonJS (require/module.exports) - Journey e2e tests now run (5 passed, 7 failed due to pre-existing backend issues) The ESM config issue was caused by Playwright 1.41.2 not properly handling ESM configs when package.json has 'type: module'. Upgrading Playwright and using CommonJS config resolves the issue.
- Remove hardcoded CLIENT_CONFIGS from create-client.utils.js (production code should not contain test-specific config) - Remove unnecessary try/catch from validator function (davinciClient.validate doesn't throw) - Remove redundant Array.isArray check in password.js (validators always return arrays) - Update e2e test to use standard CONFIG instead of clientId URL param override
17d4a80 to
6300736
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
javascript/reactjs-todo-journey/package.json (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winjavascript/reactjs-todo-journey/package.json:25 — Pin
@forgerock/sdk-utilitiesto an explicit version.latestmakes installs non-reproducible; use2.1.0or another fixed semver range instead.🤖 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-journey/package.json` at line 25, Replace the floating "latest" version for `@forgerock/sdk-utilities` in package.json with an explicit pinned semver version, preferably 2.1.0, to ensure reproducible installs.javascript/reactjs-todo-davinci/client/components/davinci-client/password.js (1)
114-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd accessible names to the visibility toggle buttons.
The password visibility toggle buttons are icon-only. Without an
aria-labelor equivalent accessible text, screen readers will only announce them amorphously as "button", leaving visually impaired users without context.
javascript/reactjs-todo-davinci/client/components/davinci-client/password.js#L114-L120: Add anaria-label(e.g.,aria-label={isPrimaryVisible ? "Hide password" : "Show password"}) to the primary password toggle button.javascript/reactjs-todo-davinci/client/components/davinci-client/password.js#L142-L148: Add anaria-label(e.g.,aria-label={isConfirmVisible ? "Hide password" : "Show password"}) to the confirm password toggle button.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/password.js` around lines 114 - 120, Add accessible labels to both icon-only visibility toggle buttons in the password component: update the primary toggle near lines 114-120 and the confirm toggle near lines 142-148 with labels that indicate “Show password” when hidden and “Hide password” when visible, using isPrimaryVisible and isConfirmVisible respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/password.js`:
- Around line 77-83: Update the primary password change handler’s confirm-error
synchronization logic by changing the `else if (confirmValue)` branch to an
unconditional `else`, so `setConfirmError('')` runs when the confirm field is
empty as well as when the values match. Keep the existing mismatch condition
unchanged and mirror `handleConfirmChange` behavior.
In `@javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js`:
- Around line 31-37: Move each postDataJSON() call in davinci-protect.spec.js
inside its corresponding POST/customHTMLTemplate condition at lines 31-37 and
76-82, so non-JSON requests are never parsed; preserve the existing request
tracking and payload processing behavior.
---
Nitpick comments:
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/password.js`:
- Around line 114-120: Add accessible labels to both icon-only visibility toggle
buttons in the password component: update the primary toggle near lines 114-120
and the confirm toggle near lines 142-148 with labels that indicate “Show
password” when hidden and “Hide password” when visible, using isPrimaryVisible
and isConfirmVisible respectively.
In `@javascript/reactjs-todo-journey/package.json`:
- Line 25: Replace the floating "latest" version for `@forgerock/sdk-utilities` in
package.json with an explicit pinned semver version, preferably 2.1.0, to ensure
reproducible installs.
🪄 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: 803e364a-3df1-429b-82a9-ec211e1a9bfb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
javascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/components/davinci-client/form.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/hooks/davinci.hook.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/password.jsjavascript/reactjs-todo-davinci/e2e/davinci-protect.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-validated-password.spec.jsjavascript/reactjs-todo-journey/package.jsonjavascript/reactjs-todo-journey/playwright.config.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (5)
- javascript/reactjs-todo-davinci/README.md
- javascript/reactjs-todo-davinci/client/components/davinci-client/form.js
- javascript/reactjs-todo-davinci/client/components/davinci-client/hooks/davinci.hook.js
- javascript/reactjs-todo-journey/playwright.config.js
- javascript/reactjs-todo-davinci/e2e/davinci-validated-password.spec.js
| // Keep confirm error in sync as the primary value changes | ||
| if (confirmValue && value !== confirmValue) { | ||
| setConfirmError('Passwords do not match'); | ||
| } else if (confirmValue) { | ||
| setConfirmError(''); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the confirm error when both fields are emptied.
If a user types in both fields (triggering a mismatch error), clears the Confirm field, and then clears the Primary field, the "Passwords do not match" error remains visible even when both fields are empty. This happens because when confirmValue is empty, both conditions in the if/else if block evaluate to false, leaving the confirmError state unchanged.
Change else if (confirmValue) to else to ensure the error clears when both fields become empty, mirroring the logic in handleConfirmChange.
🐛 Proposed fix
// Keep confirm error in sync as the primary value changes
if (confirmValue && value !== confirmValue) {
setConfirmError('Passwords do not match');
- } else if (confirmValue) {
+ } else {
setConfirmError('');
}📝 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.
| // Keep confirm error in sync as the primary value changes | |
| if (confirmValue && value !== confirmValue) { | |
| setConfirmError('Passwords do not match'); | |
| } else if (confirmValue) { | |
| setConfirmError(''); | |
| } | |
| } | |
| // Keep confirm error in sync as the primary value changes | |
| if (confirmValue && value !== confirmValue) { | |
| setConfirmError('Passwords do not match'); | |
| } else { | |
| setConfirmError(''); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/password.js`
around lines 77 - 83, Update the primary password change handler’s confirm-error
synchronization logic by changing the `else if (confirmValue)` branch to an
unconditional `else`, so `setConfirmError('')` runs when the confirm field is
empty as well as when the values match. Keep the existing mismatch condition
unchanged and mirror `handleConfirmChange` behavior.
6300736 to
3c94b3f
Compare
3c94b3f to
70eaca6
Compare
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Discussed with Ryan about the fact that we're submitting even with invalid password, but it's just a sample app, so I think it's nbd, so approving. Everything else seems to be working as expected.
4 participants
Summary
@forgerock/davinci-clientto PR#638 preview build (addsValidatedPasswordCollectorsupport)Passwordcomponent to render a static requirements list, inline validation errors on keystroke, and an optional confirm field (whenverify: true) with real-time mismatch feedbackValidatedPasswordCollectorintoform.jsvia a newvalidatorfunction exposed fromuseDavinci356a254cPingOne tenant (which returns apasswordPolicyon the registration password field)Changes
package.json@forgerock/davinci-client→pkg.pr.newPR#638 buildhooks/davinci.hook.jsvalidator(collector)with try/catch safety fallbackpassword.jsform.jsValidatedPasswordCollectorcase inmapCollectorsToComponentse2e/davinci-validated-password.spec.jsplaywright.config.ts356a254ctenant for ValidatedPassword testsNotes
pkg.pr.newURL should be replaced with the published npm tag once PR#638 is merged to the SDKverify: trueconfirm field is implemented but not reachable in the current PingOne environment (collector.output.verifyisfalse) — the code is correct and will activate automatically when the environment is configured356a254ctenant'spingOneSSOConnector/createUserstep is returning "requestTimedOut" so a happy-path registration e2e test is not included — covered by the existingdavinci-register-user.spec.jsagainst02fb4743Test plan
npm run e2e -- --grep "ValidatedPasswordCollector"— 4 passing (Chromium + Firefox)npm run e2e -- --grep "Login"— existing login tests still pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
ValidatedPasswordCollectoras a supported collector.Bug Fixes
Tests