feat(davinci-client): add ValidatedPasswordCollector with embedded password policy#572
feat(davinci-client): add ValidatedPasswordCollector with embedded password policy#572
Conversation
🦋 Changeset detectedLatest commit: d8648fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 PR introduces support for a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
|
View your CI Pipeline Execution ↗ for commit d8648fb
☁️ Nx Cloud last updated this comment at |
![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
🧹 Nitpick comments (5)
packages/davinci-client/src/lib/collector.types.ts (1)
193-210: Consider extendingSingleValueCollectorNoValueinstead of hand-rolling the shape.
PasswordVerifyCollectoris structurally identical toSingleValueCollectorNoValue<'PasswordVerifyCollector'>plus optionaloutput.passwordPolicy. Defining it as an intersection would reduce drift risk if the base shape evolves (e.g.,input.validation, etc.).Alternative definition
-export interface PasswordVerifyCollector { - category: 'SingleValueCollector'; - error: string | null; - type: 'PasswordVerifyCollector'; - id: string; - name: string; - input: { - key: string; - value: string | number | boolean; - type: string; - }; - output: { - key: string; - label: string; - type: string; - passwordPolicy?: PasswordPolicy; - }; -} +export type PasswordVerifyCollector = SingleValueCollectorNoValue<'PasswordVerifyCollector'> & { + output: { passwordPolicy?: PasswordPolicy }; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.types.ts` around lines 193 - 210, PasswordVerifyCollector duplicates the shape of SingleValueCollectorNoValue<'PasswordVerifyCollector'> and risks drifting; refactor PasswordVerifyCollector to extend or be defined as an intersection with SingleValueCollectorNoValue<'PasswordVerifyCollector'> and add the optional output.passwordPolicy field, so the type becomes SingleValueCollectorNoValue<'PasswordVerifyCollector'> & { output: { passwordPolicy?: PasswordPolicy } } (or equivalent), keeping the unique type tag 'PasswordVerifyCollector' and preserving existing fields like input/output.packages/davinci-client/src/lib/davinci.types.ts (1)
64-83: Consider modelingenvironmentonPasswordPolicy.The mock in
mock-form-fields.data.ts(lines 51-54) includesenvironment: { id: string }on the embedded policy, matching what PingOne returns. SincePasswordPolicyis the contract consumers will type against when readingcollector.output.passwordPolicy, missing this field means users accessingpolicy.environment?.idwould need a cast. Consider adding:Proposed addition
export interface PasswordPolicy { id?: string; + environment?: { id?: string }; name?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/davinci.types.ts` around lines 64 - 83, The PasswordPolicy interface is missing an environment property so consumers reading collector.output.passwordPolicy must cast to access environment.id; update the PasswordPolicy type to include environment?: { id?: string } (or a more complete environment shape if available) so the mock shape in mock-form-fields.data.ts matches the typed contract; ensure references to PasswordPolicy (e.g., usages of collector.output.passwordPolicy) compile without casts after this change.packages/davinci-client/src/lib/collector.utils.ts (1)
450-483: Duplicates error-building logic fromreturnSingleValueCollector.The
key/label/typepresence checks here mirror the block inreturnSingleValueCollector(lines 154-163). SincePasswordVerifyFieldis a compile-time-narrowed type with requiredkey,label,type, theseinchecks never fail at runtime for well-typed callers — they exist only as defensive parity with the shared helper. Consider delegating toreturnSingleValueCollectorwith a'PasswordVerifyCollector'branch, then addingpasswordPolicyon top, to keep validation centralized.Non-blocking; current implementation is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 450 - 483, Replace the duplicated presence-check logic in returnPasswordVerifyCollector by calling the shared helper returnSingleValueCollector (passing the same field and idx and specifying type 'PasswordVerifyCollector' or otherwise using its branch) to produce the base collector, then extend/override that returned object's type and output to include the passwordPolicy when present; keep references to PasswordVerifyCollector and PasswordVerifyField so the resulting collector adds the passwordPolicy field on top of the single-value collector instead of reimplementing the key/label/type checks.e2e/davinci-app/components/password.ts (2)
46-54: PreferRegExp.test()overString.prototype.match()for boolean checks.
match()allocates a result array;test()returns a boolean directly and reads more intentionally here. Also, consider hoisting the regexes so they aren't recompiled on every iteration.♻️ Suggested refactor
+ const UPPER_RE = /^[A-Z]+$/; + const LOWER_RE = /^[a-z]+$/; + const DIGIT_RE = /^[0-9]+$/; if (policy.minCharacters) { for (const [charset, count] of Object.entries(policy.minCharacters)) { const li = document.createElement('li'); - if (charset.match(/^[A-Z]+$/)) { + if (UPPER_RE.test(charset)) { li.textContent = `At least ${count} uppercase letter(s)`; - } else if (charset.match(/^[a-z]+$/)) { + } else if (LOWER_RE.test(charset)) { li.textContent = `At least ${count} lowercase letter(s)`; - } else if (charset.match(/^[0-9]+$/)) { + } else if (DIGIT_RE.test(charset)) { li.textContent = `At least ${count} number(s)`; } else { li.textContent = `At least ${count} special character(s)`; } requirementsList.appendChild(li); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/davinci-app/components/password.ts` around lines 46 - 54, The code in password.ts uses String.prototype.match() for boolean checks inside the block that sets li.textContent; replace those match() calls with precompiled RegExp.test() calls to avoid allocating arrays and improve intent: create hoisted regex constants (e.g., UPPERCASE_REGEX, LOWERCASE_REGEX, DIGIT_REGEX, SPECIAL_REGEX) outside the loop or function and then use UPPERCASE_REGEX.test(charset), LOWERCASE_REGEX.test(charset), DIGIT_REGEX.test(charset), else SPECIAL_REGEX.test(charset) to pick the appropriate message for the li element (the code paths around li.textContent remain the same).
31-62: Optional: associate the requirements list with the password input for a11y.Even for an e2e sample, wiring
aria-describedbyon the<input>to an id on the<ul>(or usingaria-labelledbywith a heading) makes the intent visible to assistive tech and is a nicer reference pattern for SDK consumers to copy. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/davinci-app/components/password.ts` around lines 31 - 62, Add an accessible reference between the rendered requirements list and the password input: give the created requirementsList element a stable id (e.g., "password-requirements-<unique>") and set that id on the password input's aria-describedby so assistive tech can read the requirements; locate the password input where the form is built (the password input variable or the element found via selector) and assign aria-describedby to match requirementsList.id only when requirementsList is appended (use the same conditional that appends requirementsList to formEl and clean up/reset the attribute if policy is not present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/davinci-app/components/password.ts`:
- Around line 37-41: The current rendering uses policy.length.min and
policy.length.max directly, which produces "undefined" when one side is missing;
update the logic around policy.length to check length.min and length.max
independently (e.g., if both present render "min–max characters", if only min
render "at least {min} characters", if only max render "up to {max}
characters"), only create/append the li element to requirementsList when at
least one of length.min or length.max exists, and use the existing li variable
for the text content assignment.
---
Nitpick comments:
In `@e2e/davinci-app/components/password.ts`:
- Around line 46-54: The code in password.ts uses String.prototype.match() for
boolean checks inside the block that sets li.textContent; replace those match()
calls with precompiled RegExp.test() calls to avoid allocating arrays and
improve intent: create hoisted regex constants (e.g., UPPERCASE_REGEX,
LOWERCASE_REGEX, DIGIT_REGEX, SPECIAL_REGEX) outside the loop or function and
then use UPPERCASE_REGEX.test(charset), LOWERCASE_REGEX.test(charset),
DIGIT_REGEX.test(charset), else SPECIAL_REGEX.test(charset) to pick the
appropriate message for the li element (the code paths around li.textContent
remain the same).
- Around line 31-62: Add an accessible reference between the rendered
requirements list and the password input: give the created requirementsList
element a stable id (e.g., "password-requirements-<unique>") and set that id on
the password input's aria-describedby so assistive tech can read the
requirements; locate the password input where the form is built (the password
input variable or the element found via selector) and assign aria-describedby to
match requirementsList.id only when requirementsList is appended (use the same
conditional that appends requirementsList to formEl and clean up/reset the
attribute if policy is not present).
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 193-210: PasswordVerifyCollector duplicates the shape of
SingleValueCollectorNoValue<'PasswordVerifyCollector'> and risks drifting;
refactor PasswordVerifyCollector to extend or be defined as an intersection with
SingleValueCollectorNoValue<'PasswordVerifyCollector'> and add the optional
output.passwordPolicy field, so the type becomes
SingleValueCollectorNoValue<'PasswordVerifyCollector'> & { output: {
passwordPolicy?: PasswordPolicy } } (or equivalent), keeping the unique type tag
'PasswordVerifyCollector' and preserving existing fields like input/output.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 450-483: Replace the duplicated presence-check logic in
returnPasswordVerifyCollector by calling the shared helper
returnSingleValueCollector (passing the same field and idx and specifying type
'PasswordVerifyCollector' or otherwise using its branch) to produce the base
collector, then extend/override that returned object's type and output to
include the passwordPolicy when present; keep references to
PasswordVerifyCollector and PasswordVerifyField so the resulting collector adds
the passwordPolicy field on top of the single-value collector instead of
reimplementing the key/label/type checks.
In `@packages/davinci-client/src/lib/davinci.types.ts`:
- Around line 64-83: The PasswordPolicy interface is missing an environment
property so consumers reading collector.output.passwordPolicy must cast to
access environment.id; update the PasswordPolicy type to include environment?: {
id?: string } (or a more complete environment shape if available) so the mock
shape in mock-form-fields.data.ts matches the typed contract; ensure references
to PasswordPolicy (e.g., usages of collector.output.passwordPolicy) compile
without casts after this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 369808f1-fb4b-4b90-aba0-5b1ec2fe0fe2
📒 Files selected for processing (17)
.changeset/embed-password-policy-in-component.mde2e/davinci-app/components/password.tse2e/davinci-app/main.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (17.85%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #572 +/- ##
===========================================
- Coverage 70.90% 17.85% -53.06%
===========================================
Files 53 155 +102
Lines 2021 24271 +22250
Branches 377 1181 +804
===========================================
+ Hits 1433 4333 +2900
- Misses 588 19938 +19350
🚀 New features to boost your workflow:
|
|
Deployed f8ba74f to https://ForgeRock.github.io/ping-javascript-sdk/pr-572/f8ba74f0cbd26640e625ab8daabd455d33988a9c branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Can we check to see if the native platforms are providing a .validate() function for this collector? We have this for ValidatedSingleValueCollector.
| const requirementsList = document.createElement('ul'); | ||
| requirementsList.className = 'password-requirements'; | ||
|
|
||
| if (policy.length) { |
There was a problem hiding this comment.
Wow, this really threw me. I was thinking policy was an array due to the .length use, which made the other code confusing :)
| } | ||
|
|
||
| // Component-level policy takes precedence over root-level | ||
| const passwordPolicy = field.passwordPolicy ?? rootPasswordPolicy; |
There was a problem hiding this comment.
Do we want to fallback to this "root" policy? Is this what the other platforms are doing?
There was a problem hiding this comment.
No they aren't it was just a best guess i can remove it.
|
|
||
| export type SingleValueCollectors = | ||
| | SingleValueCollectorNoValue<'PasswordCollector'> | ||
| | PasswordVerifyCollector |
There was a problem hiding this comment.
I'm curious as to why you're treating this new collector differently than the others. Could you elaborate on the reason for deviation?
| * @param {PasswordPolicy} [rootPasswordPolicy] - Optional root-level password policy for backward compatibility. | ||
| * @returns {PasswordVerifyCollector} The constructed PasswordVerifyCollector object. | ||
| */ | ||
| export function returnPasswordVerifyCollector( |
There was a problem hiding this comment.
Could we extend/reuse the PasswordCollector functions and just add the passwordPolicy property to the collector output?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/davinci-client/src/lib/collector.types.test-d.ts (1)
147-156:⚠️ Potential issue | 🟡 MinorMake the
SingleValueCollectorTypesfixture actually exhaustive.Because
validTypesis annotated asSingleValueCollectorTypes[],(typeof validTypes)[number]widens to the full union even when the array omits'ValidatedPasswordCollector'.🧪 Proposed type-test fix
- const validTypes: SingleValueCollectorTypes[] = [ + const validTypes = [ 'TextCollector', 'PasswordCollector', + 'ValidatedPasswordCollector', + 'SingleValueCollector', 'SingleSelectCollector', - ]; + 'SingleSelectObjectCollector', + 'ValidatedTextCollector', + ] as const satisfies readonly SingleValueCollectorTypes[]; // Type assertion to ensure SingleValueCollectorTypes includes all these values expectTypeOf<SingleValueCollectorTypes>().toEqualTypeOf<(typeof validTypes)[number]>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.types.test-d.ts` around lines 147 - 156, The test's fixture isn't truly exhaustive because annotating validTypes as SingleValueCollectorTypes[] widens the type; change validTypes to a const literal tuple (remove the explicit SingleValueCollectorTypes[] annotation and use a const assertion) and include the missing 'ValidatedPasswordCollector' value so the const tuple equals the exact union; keep the assertion expectTypeOf<SingleValueCollectorTypes>().toEqualTypeOf<(typeof validTypes)[number]>(), referencing SingleValueCollectorTypes and the validTypes variable to force compile-time exhaustiveness.
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/davinci.types.ts (1)
91-98: Consider discriminated unions to strengthen type safety for PASSWORD_VERIFY.The code intentionally separates the
PASSWORD_VERIFYtype tag from theverifyflag because collector selection depends onpasswordPolicypresence, not the type. However, documenting that the server sendsverify: truealongsidePASSWORD_VERIFYdoesn't enforce that relationship at the type level.A discriminated union would make this semantic explicit and prevent future misuse:
Optional type refinement
-export type PasswordField = { - type: 'PASSWORD' | 'PASSWORD_VERIFY'; +type PasswordFieldBase = { key: string; label: string; required?: boolean; - verify?: boolean; passwordPolicy?: PasswordPolicy; }; + +export type PasswordField = + | (PasswordFieldBase & { type: 'PASSWORD'; verify?: false }) + | (PasswordFieldBase & { type: 'PASSWORD_VERIFY'; verify?: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/davinci.types.ts` around lines 91 - 98, The PasswordField type currently allows any combination of type ('PASSWORD' | 'PASSWORD_VERIFY') with optional verify and passwordPolicy, which doesn't enforce the invariant that when type === 'PASSWORD_VERIFY' the server includes verify: true; refactor PasswordField into a discriminated union with two variants (e.g., PasswordFieldPassword and PasswordFieldPasswordVerify) so that the 'type' field discriminates: the 'PASSWORD_VERIFY' variant must include verify: true (and any fields required for that case) while the 'PASSWORD' variant has verify?: boolean and optional passwordPolicy as before; update usages that construct or consume PasswordField to handle the two variants (using type narrowing) to preserve type safety around verify and passwordPolicy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/davinci-client/src/lib/collector.types.test-d.ts`:
- Around line 147-156: The test's fixture isn't truly exhaustive because
annotating validTypes as SingleValueCollectorTypes[] widens the type; change
validTypes to a const literal tuple (remove the explicit
SingleValueCollectorTypes[] annotation and use a const assertion) and include
the missing 'ValidatedPasswordCollector' value so the const tuple equals the
exact union; keep the assertion
expectTypeOf<SingleValueCollectorTypes>().toEqualTypeOf<(typeof
validTypes)[number]>(), referencing SingleValueCollectorTypes and the validTypes
variable to force compile-time exhaustiveness.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/davinci.types.ts`:
- Around line 91-98: The PasswordField type currently allows any combination of
type ('PASSWORD' | 'PASSWORD_VERIFY') with optional verify and passwordPolicy,
which doesn't enforce the invariant that when type === 'PASSWORD_VERIFY' the
server includes verify: true; refactor PasswordField into a discriminated union
with two variants (e.g., PasswordFieldPassword and PasswordFieldPasswordVerify)
so that the 'type' field discriminates: the 'PASSWORD_VERIFY' variant must
include verify: true (and any fields required for that case) while the
'PASSWORD' variant has verify?: boolean and optional passwordPolicy as before;
update usages that construct or consume PasswordField to handle the two variants
(using type narrowing) to preserve type safety around verify and passwordPolicy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d353af1d-2e88-4ad4-abfb-cdda4372942c
📒 Files selected for processing (22)
.changeset/embed-password-policy-in-component.md.nxignore.opensource/forgerock-javascript-sdke2e/davinci-app/components/password.tse2e/davinci-app/main.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/davinci.utils.test.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.tspackages/davinci-client/src/lib/mock-data/node.next.mock.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
✅ Files skipped from review due to trivial changes (4)
- packages/davinci-client/src/lib/davinci.utils.test.ts
- .nxignore
- .opensource/forgerock-javascript-sdk
- packages/davinci-client/src/lib/mock-data/node.next.mock.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- .changeset/embed-password-policy-in-component.md
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
- packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/src/lib/client.types.ts
- packages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
ryanbas21
force-pushed
the
password-rules-toplevel
branch
from
0159bb8 to
49c8247
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/collector.utils.ts (1)
942-953: Nit: thepolicyternary is always truthy.
returnSingleValueCollectorsetspasswordPolicyto{}(line 193-194) when the field lacks one, and the collector'soutput.passwordPolicytype is required, sopolicyhere is always truthy and[]is never returned via the else branch. The rules already short-circuit on missing fields, so the guard is dead code and can be dropped for clarity — or, if you want the guard to remain meaningful, makeValidatedPasswordCollector['output']['passwordPolicy']optional and stop defaulting to{}in the factory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 942 - 953, The guard in returnPasswordPolicyValidator is dead because ValidatedPasswordCollector.output.passwordPolicy is always an object (set to {} by returnSingleValueCollector), so remove the ternary and simply apply passwordPolicyRules to collector.output.passwordPolicy and the value (i.e., always run pipe( passwordPolicyRules, Arr.flatMap(rule => rule(collector.output.passwordPolicy, value)) )); this clarifies intent and relies on the rules' own short-circuiting—alternatively, if you prefer keeping the guard, make ValidatedPasswordCollector['output']['passwordPolicy'] optional and stop defaulting it to {} in returnSingleValueCollector so the check becomes meaningful.e2e/davinci-app/components/password.ts (1)
81-111: Minor: reuse theinputreference rather than re-querying.
inputis already in scope from line 27, soformEl?.querySelector(\#${collectorKey}`)is an unnecessary DOM lookup (and a fragile one — any futurecollectorKeythat isn't a valid CSS ident would break#…). Same for error cleanup via.${collectorKey}-error`: caching the reference would avoid re-querying on every keystroke.♻️ Proposed refactor
- const inputEl = formEl?.querySelector(`#${collectorKey}`); - const shouldValidate = collector.type === 'ValidatedPasswordCollector' && !!validator; - - inputEl?.addEventListener('input', (event: Event) => { + const shouldValidate = collector.type === 'ValidatedPasswordCollector' && !!validator; + let errorEl: HTMLUListElement | null = null; + + input.addEventListener('input', (event: Event) => { const value = (event.target as HTMLInputElement).value; if (shouldValidate) { const result = validator(value); if (Array.isArray(result) && result.length) { - let errorEl = formEl?.querySelector<HTMLUListElement>(`.${collectorKey}-error`); if (!errorEl) { errorEl = document.createElement('ul'); errorEl.className = `${collectorKey}-error`; - inputEl.after(errorEl); + input.after(errorEl); } const items = result.map((msg) => { const li = document.createElement('li'); li.textContent = msg; return li; }); errorEl.replaceChildren(...items); return; } - formEl?.querySelector(`.${collectorKey}-error`)?.remove(); + errorEl?.remove(); + errorEl = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/davinci-app/components/password.ts` around lines 81 - 111, The input DOM is being re-queried inside the input event listener using formEl?.querySelector(`#${collectorKey}`) and the error element is re-queried for cleanup; instead reuse the existing input variable from outer scope (the one created at line ~27) inside the listener and cache a single errorEl reference (named e.g. errorEl) tied to `${collectorKey}-error` so you only create/query it once when needed, update its children with replaceChildren, and call errorEl?.remove() for cleanup instead of re-querying via formEl.querySelector; keep the existing logic around shouldValidate, validator, and updater but replace the repeated DOM queries with the cached input and cached errorEl references to avoid fragile `#${collectorKey}` lookups and redundant work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/davinci-client/api-report/davinci-client.api.md`:
- Around line 150-157: The public API currently exposes
ValidatedPasswordCollector and requires output.passwordPolicy, but the intended
contract is PasswordVerifyCollector/PasswordVerifyField where passwordPolicy is
optional (omitted when absent); update the source type declarations so the
exported union uses PasswordVerifyCollector instead of
ValidatedPasswordCollector, change the field/type name to PasswordVerifyField if
necessary, make passwordPolicy optional on that collector/type, ensure any
discriminated union uses type: 'PasswordVerifyCollector' (or 'PASSWORD_VERIFY')
for switching, then regenerate the API report so davinci-client.api.md reflects
the corrected exported types and CollectorValueType behavior.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 901-910: The maxRepeatedCharactersRule currently uses total
character counts (via countChars) but must instead compute the longest
consecutive run of any character; update maxRepeatedCharactersRule to iterate
the input string (value), tracking currentChar and currentRun length, updating
maxRun when the character changes and at the end, then compare maxRun to
policy.maxRepeatedCharacters and return the same error message if exceeded;
remove the now-unused countChars helper (lines ~872–876) after this change.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 172-180: The switch handling for 'PASSWORD'/'PASSWORD_VERIFY'
currently uses field.passwordPolicy as the discriminator; change it to route
first on field.type so that case 'PASSWORD' always returns
returnPasswordCollector(field, idx) and case 'PASSWORD_VERIFY' always returns
returnValidatedPasswordCollector(field, idx) (or a dedicated verify-path
function) while using field.passwordPolicy only to populate or omit the policy
payload on the collector output; update the branch logic around
returnValidatedPasswordCollector and returnPasswordCollector to no longer flip
types based on passwordPolicy but instead to attach policy data conditionally
when building the PASSWORD_VERIFY collector.
---
Nitpick comments:
In `@e2e/davinci-app/components/password.ts`:
- Around line 81-111: The input DOM is being re-queried inside the input event
listener using formEl?.querySelector(`#${collectorKey}`) and the error element
is re-queried for cleanup; instead reuse the existing input variable from outer
scope (the one created at line ~27) inside the listener and cache a single
errorEl reference (named e.g. errorEl) tied to `${collectorKey}-error` so you
only create/query it once when needed, update its children with replaceChildren,
and call errorEl?.remove() for cleanup instead of re-querying via
formEl.querySelector; keep the existing logic around shouldValidate, validator,
and updater but replace the repeated DOM queries with the cached input and
cached errorEl references to avoid fragile `#${collectorKey}` lookups and
redundant work.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 942-953: The guard in returnPasswordPolicyValidator is dead
because ValidatedPasswordCollector.output.passwordPolicy is always an object
(set to {} by returnSingleValueCollector), so remove the ternary and simply
apply passwordPolicyRules to collector.output.passwordPolicy and the value
(i.e., always run pipe( passwordPolicyRules, Arr.flatMap(rule =>
rule(collector.output.passwordPolicy, value)) )); this clarifies intent and
relies on the rules' own short-circuiting—alternatively, if you prefer keeping
the guard, make ValidatedPasswordCollector['output']['passwordPolicy'] optional
and stop defaulting it to {} in returnSingleValueCollector so the check becomes
meaningful.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc3dca3f-02a6-4b06-b008-bef7e60d04b7
📒 Files selected for processing (21)
.changeset/embed-password-policy-in-component.md.nxignore.opensource/forgerock-javascript-sdke2e/davinci-app/components/password.tse2e/davinci-app/main.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/davinci.utils.test.tspackages/davinci-client/src/lib/mock-data/node.next.mock.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
✅ Files skipped from review due to trivial changes (4)
- .nxignore
- packages/davinci-client/src/lib/davinci.utils.test.ts
- packages/davinci-client/src/lib/mock-data/node.next.mock.ts
- .opensource/forgerock-javascript-sdk
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/davinci-app/main.ts
- packages/davinci-client/src/lib/node.types.ts
- .changeset/embed-password-policy-in-component.md
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
- packages/davinci-client/api-report/davinci-client.types.api.md
| const maxRepeatedCharactersRule: PasswordPolicyRule = (policy, value) => { | ||
| const max = policy.maxRepeatedCharacters; | ||
| if (max == null) return []; | ||
| const maxCount = pipe( | ||
| countChars(value), | ||
| (counts) => Array.from(counts.values()), | ||
| Arr.reduce(0, (acc, n) => (n > acc ? n : acc)), | ||
| ); | ||
| return maxCount > max ? [`Password cannot repeat any character more than ${max} times`] : []; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PingOne password policy maxRepeatedCharacters — does it mean consecutive repeats or total occurrences of a character?
💡 Result:
PingOne's password policy setting maxRepeatedCharacters refers to the maximum number of consecutive repeated characters allowed in a password. It limits runs of the same character in sequence, such as no more than 2 or 3 identical characters in a row (e.g., "goood" with three 'o's would violate if max is 2, but "good-apple" with two 'o's separated is allowed). It does not limit total occurrences of a character across the password.
Citations:
- 1: https://docs.pingidentity.com/pingone/authentication/p1_passwordpolicies.html
- 2: https://developer.pingidentity.com/pingone-api/platform/password-policies.html
- 3: https://registry.terraform.io/providers/pingidentity/pingone/1.17.0/docs/resources/password_policy
- 4: https://registry.terraform.io/providers/pingidentity/pingone/latest/docs/resources/password_policy
🏁 Script executed:
# Check if file exists and get the current implementation
head -920 packages/davinci-client/src/lib/collector.utils.ts | tail -30Repository: ForgeRock/ping-javascript-sdk
Length of output: 1085
🏁 Script executed:
# Search for countChars usage in the file
rg "countChars" packages/davinci-client/src/lib/collector.utils.ts -nRepository: ForgeRock/ping-javascript-sdk
Length of output: 172
🏁 Script executed:
# Search for maxRepeatedCharactersRule usage
rg "maxRepeatedCharactersRule" packages/davinci-client/src/lib/collector.utils.ts -nRepository: ForgeRock/ping-javascript-sdk
Length of output: 184
🏁 Script executed:
# Get context around lines 872-876 to see countChars definition
sed -n '872,876p' packages/davinci-client/src/lib/collector.utils.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 274
maxRepeatedCharacters should count consecutive runs, not total occurrences.
The rule as written takes the max total count of any character across the entire password. PingOne's password policy documentation defines this constraint as the maximum number of times a character may appear consecutively — preventing values like "aaabbb" or "goood" (if max is 2). The current implementation wrongly rejects valid passwords and accepts invalid ones.
Concrete divergence with the current implementation, assuming maxRepeatedCharacters = 2:
"abcabcabc"is flagged (3 totalas) even though no character repeats consecutively."aabaab"is flagged (4 totalas) even though the longest run is 2, which is within the limit.
🐛 Proposed fix — measure the longest consecutive run
-const maxRepeatedCharactersRule: PasswordPolicyRule = (policy, value) => {
- const max = policy.maxRepeatedCharacters;
- if (max == null) return [];
- const maxCount = pipe(
- countChars(value),
- (counts) => Array.from(counts.values()),
- Arr.reduce(0, (acc, n) => (n > acc ? n : acc)),
- );
- return maxCount > max ? [`Password cannot repeat any character more than ${max} times`] : [];
-};
+const longestConsecutiveRun = (value: string): number => {
+ let longest = 0;
+ let current = 0;
+ let prev: string | undefined;
+ for (const ch of value) {
+ if (ch === prev) {
+ current += 1;
+ } else {
+ current = 1;
+ prev = ch;
+ }
+ if (current > longest) longest = current;
+ }
+ return longest;
+};
+
+const maxRepeatedCharactersRule: PasswordPolicyRule = (policy, value) => {
+ const max = policy.maxRepeatedCharacters;
+ if (max == null) return [];
+ return longestConsecutiveRun(value) > max
+ ? [`Password cannot repeat the same character more than ${max} times in a row`]
+ : [];
+};After this change, countChars (lines 872–876) is unused and can be removed.
📝 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.
| const maxRepeatedCharactersRule: PasswordPolicyRule = (policy, value) => { | |
| const max = policy.maxRepeatedCharacters; | |
| if (max == null) return []; | |
| const maxCount = pipe( | |
| countChars(value), | |
| (counts) => Array.from(counts.values()), | |
| Arr.reduce(0, (acc, n) => (n > acc ? n : acc)), | |
| ); | |
| return maxCount > max ? [`Password cannot repeat any character more than ${max} times`] : []; | |
| }; | |
| const longestConsecutiveRun = (value: string): number => { | |
| let longest = 0; | |
| let current = 0; | |
| let prev: string | undefined; | |
| for (const ch of value) { | |
| if (ch === prev) { | |
| current += 1; | |
| } else { | |
| current = 1; | |
| prev = ch; | |
| } | |
| if (current > longest) longest = current; | |
| } | |
| return longest; | |
| }; | |
| const maxRepeatedCharactersRule: PasswordPolicyRule = (policy, value) => { | |
| const max = policy.maxRepeatedCharacters; | |
| if (max == null) return []; | |
| return longestConsecutiveRun(value) > max | |
| ? [`Password cannot repeat the same character more than ${max} times in a row`] | |
| : []; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 901 - 910,
The maxRepeatedCharactersRule currently uses total character counts (via
countChars) but must instead compute the longest consecutive run of any
character; update maxRepeatedCharactersRule to iterate the input string (value),
tracking currentChar and currentRun length, updating maxRun when the character
changes and at the end, then compare maxRun to policy.maxRepeatedCharacters and
return the same error message if exceeded; remove the now-unused countChars
helper (lines ~872–876) after this change.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Initial scan, this looks good. One question on the type syntax, and I'd also like to ask what the api-report stuff is. Should I assume this relates to how you are generating the API migration guide and stuff?
I demo'd this on Friday, but we use api-extractor in the generation / validation of exports. From a keeping it in git perspective, it shows the public api changes per PR in a digestable way as well. So it's two fold. |
There was a problem hiding this comment.
I feel like we need to start breaking this code up into separate files. We don't have to do it in this PR, but wow, a near 1000 line file is kind of a lot.
| if (collectorToUpdate.type === 'ValidatedPasswordCollector') { | ||
| return returnPasswordPolicyValidator(collectorToUpdate); | ||
| } | ||
|
|
||
| if (collectorToUpdate.type === 'PasswordCollector') { | ||
| return handleUpdateValidateError( | ||
| 'PasswordCollector cannot be validated; pass a ValidatedPasswordCollector', | ||
| 'state_error', | ||
| log.error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Can we flip this to be consistent with our success cases come after all error cases?
| /** | ||
| * A single policy check: given the policy and a candidate value, produce zero or more | ||
| * human-readable error strings. Rules are pure and independent — new ones can be added | ||
| * by extending `passwordPolicyRules` below. | ||
| */ |
There was a problem hiding this comment.
At the very least, can we pull all of these "rule" functions out into their own file?
ryanbas21
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/davinci-client/src/lib/collector.utils.ts (1)
186-210:⚠️ Potential issue | 🟠 MajorDon't materialize an empty
passwordPolicy.For
PASSWORD_VERIFYwithout an embedded policy, the collector should omitoutput.passwordPolicyinstead of forcing{}. The current fallback changes the contract from “optional when present” to “always present”, and downstream checks will treat “no policy” as a real policy object.🛠️ Suggested change
- const passwordPolicy = - 'passwordPolicy' in field && field.passwordPolicy ? field.passwordPolicy : {}; + const passwordPolicy = 'passwordPolicy' in field ? field.passwordPolicy : undefined; const verify = 'verify' in field ? field.verify === true : false; return { category: 'SingleValueCollector', error: error || null, type: collectorType, @@ output: { key: field.key, label: field.label, type: field.type, verify, - passwordPolicy, + ...(passwordPolicy ? { passwordPolicy } : {}), },Please align the exported
ValidatedPasswordCollectorshape, reducer tests, and regenerated API report topasswordPolicy?: PasswordPolicyin the same change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 186 - 210, The current factory for collectorType 'ValidatedPasswordCollector' always materializes passwordPolicy as {} (via the passwordPolicy fallback), but it should omit output.passwordPolicy when none is provided; change the logic in the ValidatedPasswordCollector branch so that you do not set passwordPolicy = {}—instead detect presence ('passwordPolicy' in field && field.passwordPolicy) and only add output.passwordPolicy when present (or set it to undefined/omit the key), update the ValidatedPasswordCollector/output shape accordingly, and then run/update the reducer tests and regenerate the API report to reflect passwordPolicy?: PasswordPolicy.packages/davinci-client/src/lib/password-policy.rules.ts (1)
19-23:⚠️ Potential issue | 🟠 MajorCount consecutive runs, not total character frequency.
PingOne documents
maxRepeatedCharactersas a cap on consecutive repeats; e.g.good-appleis acceptable whilegoood-apppleis not. The currentcountChars()/maxCountpath measures total occurrences across the whole password, so it will reject valid values likeabcabcabc. (docs.pingidentity.com)🐛 Proposed fix
-const countChars = (value: string): ReadonlyMap<string, number> => { - const counts = new Map<string, number>(); - for (const ch of value) counts.set(ch, (counts.get(ch) ?? 0) + 1); - return counts; -}; +const longestConsecutiveRun = (value: string): number => { + let longest = 0; + let current = 0; + let prev: string | undefined; + + for (const ch of value) { + current = ch === prev ? current + 1 : 1; + prev = ch; + if (current > longest) longest = current; + } + + return longest; +}; const maxRepeatedCharactersRule: PasswordPolicyRule = (policy, value) => { const max = policy.maxRepeatedCharacters; if (max == null) return []; - const maxCount = pipe( - countChars(value), - (counts) => Array.from(counts.values()), - Arr.reduce(0, (acc, n) => (n > acc ? n : acc)), - ); - return maxCount > max ? [`Password cannot repeat any character more than ${max} times`] : []; + return longestConsecutiveRun(value) > max + ? [`Password cannot repeat any character more than ${max} times in a row`] + : []; };Also applies to: 48-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/password-policy.rules.ts` around lines 19 - 23, The logic in countChars incorrectly tallies total occurrences rather than consecutive repeats; update the implementation used by the maxCount check to compute the maximum consecutive run length per character instead of total frequency. Replace or modify countChars (or introduce a new helper like getMaxConsecutiveRuns) to iterate the string once, track the current run and update the per-character max run (e.g., currentChar and currentRun -> update map with Math.max(existing, currentRun) when char changes), and then use those max-consecutive values when enforcing maxRepeatedCharacters in the password rule code paths that reference countChars / maxCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 186-210: The current factory for collectorType
'ValidatedPasswordCollector' always materializes passwordPolicy as {} (via the
passwordPolicy fallback), but it should omit output.passwordPolicy when none is
provided; change the logic in the ValidatedPasswordCollector branch so that you
do not set passwordPolicy = {}—instead detect presence ('passwordPolicy' in
field && field.passwordPolicy) and only add output.passwordPolicy when present
(or set it to undefined/omit the key), update the
ValidatedPasswordCollector/output shape accordingly, and then run/update the
reducer tests and regenerate the API report to reflect passwordPolicy?:
PasswordPolicy.
In `@packages/davinci-client/src/lib/password-policy.rules.ts`:
- Around line 19-23: The logic in countChars incorrectly tallies total
occurrences rather than consecutive repeats; update the implementation used by
the maxCount check to compute the maximum consecutive run length per character
instead of total frequency. Replace or modify countChars (or introduce a new
helper like getMaxConsecutiveRuns) to iterate the string once, track the current
run and update the per-character max run (e.g., currentChar and currentRun ->
update map with Math.max(existing, currentRun) when char changes), and then use
those max-consecutive values when enforcing maxRepeatedCharacters in the
password rule code paths that reference countChars / maxCount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2ff15f4-f5a3-49ec-8a62-387bd227f850
📒 Files selected for processing (10)
.changeset/embed-password-policy-in-component.mdpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/password-policy.rules.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/embed-password-policy-in-component.md
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Another scan through the PR.
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import { Array as Arr, Option, pipe } from 'effect'; |
There was a problem hiding this comment.
Is this going to pull in more of the Effect library? Historically, we only imported Micro, yeah? In order to keep this bundle as small as possible, I'd like to make sure this is really necessary.
There was a problem hiding this comment.
No, it won't. Micro uses Option/Array already
There was a problem hiding this comment.
I think this is ok because Option and Array are basic data types that don't affect the bundle size.
https://effect.website/docs/micro/new-users/
Bundle Size
Utilizing major Effect modules beyond basic data modules like Option, Either, Array, will incorporate the Effect runtime into your bundle, negating the benefits of Micro.
There was a problem hiding this comment.
Cool, I'm good with this then. Just wanted to check. Thanks you two!
| case 'PASSWORD_VERIFY': { | ||
| return returnValidatedPasswordCollector(field, idx); |
There was a problem hiding this comment.
Is this correct? I thought we were driving whether the collector was ValidatedPasswordCollector or not by the presence of policies, yeah?
There was a problem hiding this comment.
Right?
There was a problem hiding this comment.
@ryanbas21 can you copy paste what you're referring to here? The link is taking me to the top of your PR for some reason.
If PASSWORD_VERIFY does not have any policies then should it fall back to a PasswordCollector?
| expect((result[0] as ValidatedPasswordCollector).output.passwordPolicy).toEqual({}); | ||
| }); | ||
|
|
||
| it('should produce PasswordCollector when a PASSWORD field carries a policy (policy ignored)', () => { |
There was a problem hiding this comment.
should produce PasswordCollector when a PASSWORD field carries a policy (policy ignored)
huh?
ryanbas21
force-pushed
the
password-rules-toplevel
branch
2 times, most recently
from
f9533cb to
11d4052
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Looks alright to me. Left a comment asking for clarification.
| case 'PASSWORD_VERIFY': { | ||
| return returnValidatedPasswordCollector(field, idx); |
There was a problem hiding this comment.
@ryanbas21 can you copy paste what you're referring to here? The link is taking me to the top of your PR for some reason.
If PASSWORD_VERIFY does not have any policies then should it fall back to a PasswordCollector?
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import { Array as Arr, Option, pipe } from 'effect'; |
There was a problem hiding this comment.
I think this is ok because Option and Array are basic data types that don't affect the bundle size.
https://effect.website/docs/micro/new-users/
Bundle Size
Utilizing major Effect modules beyond basic data modules like Option, Either, Array, will incorporate the Effect runtime into your bundle, negating the benefits of Micro.
|
ryanbas21
force-pushed
the
password-rules-toplevel
branch
from
11d4052 to
9ba1b33
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 (1)
packages/davinci-client/src/lib/collector.types.ts (1)
21-28:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
SingleValueCollector<T>no longer matches the public type union.
SingleValueCollectorTypesnow includes the password variants, butSingleValueCollector<T>still expands them into the generic value/no-value shapes. That makesSingleValueCollector<'PasswordCollector'>andSingleValueCollector<'ValidatedPasswordCollector'>type to the wrong structure.Please either keep the password variants out of this generic or add dedicated branches so the exported API stays sound.
Also applies to: 179-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.types.ts` around lines 21 - 28, The exported union SingleValueCollectorTypes now includes 'PasswordCollector' and 'ValidatedPasswordCollector' but the generic mapped type SingleValueCollector<T> still resolves only to the generic value/no-value shapes, causing SingleValueCollector<'PasswordCollector'> to have the wrong structure; to fix, update SingleValueCollector<T> to include explicit branches for 'PasswordCollector' and 'ValidatedPasswordCollector' that map to the correct password-specific shape (or alternatively remove those two variants from SingleValueCollectorTypes), and apply the same change to the related generic/type mapping used around the other occurrence referenced (the similar mapping at the later block).
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/collector.utils.ts (1)
472-493: ⚡ Quick winConstrain factory inputs to their password field discriminants.
These helpers currently accept both
PASSWORDandPASSWORD_VERIFY. Narrowing each function’s input type will prevent accidental cross-routing at compile time.Type-safe signature refinement
-export function returnPasswordCollector(field: PasswordField, idx: number): PasswordCollector { +export function returnPasswordCollector( + field: Extract<PasswordField, { type: 'PASSWORD' }>, + idx: number, +): PasswordCollector { return returnSingleValueCollector(field, idx, 'PasswordCollector') as PasswordCollector; } export function returnValidatedPasswordCollector( - field: PasswordField, + field: Extract<PasswordField, { type: 'PASSWORD_VERIFY' }>, idx: number, ): ValidatedPasswordCollector { return returnSingleValueCollector(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 472 - 493, The two factory functions accept the broader PasswordField union; narrow their first-parameter types to the specific discriminants so they only accept the correct variant: change returnPasswordCollector(field: PasswordField, ...) to accept the PasswordField variant whose type is 'PASSWORD' (e.g. PasswordField & { type: 'PASSWORD' } or the existing concrete interface for PASSWORD fields), and change returnValidatedPasswordCollector(field: PasswordField, ...) to accept only the 'PASSWORD_VERIFY' variant (e.g. PasswordField & { type: 'PASSWORD_VERIFY' } or its concrete interface); keep the idx parameter and return types the same (PasswordCollector and ValidatedPasswordCollector) so callers that pass wrong discriminants will fail at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/davinci-client/src/lib/collector.utils.test.ts`:
- Around line 1266-1267: Update the stale test comment to reflect current
reducer routing: change the description that says the reducer selects
returnPasswordCollector when a field has no policy to state that the reducer now
routes PASSWORD_VERIFY to returnValidatedPasswordCollector regardless of policy;
mention that the test specifically exercises the factory's defensive fallback
for callers bypassing the reducer (referencing PASSWORD_VERIFY,
returnValidatedPasswordCollector, returnPasswordCollector, and the factory code)
so readers understand why the fallback test remains necessary.
In `@packages/davinci-client/src/lib/davinci.types.ts`:
- Around line 85-90: Update the PasswordField docblock to reflect current
runtime routing: state that the server sets the `type` to `PASSWORD` or
`PASSWORD_VERIFY` and that collector selection is driven by the `type` field at
runtime (not by presence of `passwordPolicy`), and mention how
`PasswordCollector` vs `ValidatedPasswordCollector` are resolved now; edit the
comment near the PasswordField/type definition to remove the misleading
"policy-driven" wording and explicitly describe the `PASSWORD` vs
`PASSWORD_VERIFY` behavior.
---
Outside diff comments:
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 21-28: The exported union SingleValueCollectorTypes now includes
'PasswordCollector' and 'ValidatedPasswordCollector' but the generic mapped type
SingleValueCollector<T> still resolves only to the generic value/no-value
shapes, causing SingleValueCollector<'PasswordCollector'> to have the wrong
structure; to fix, update SingleValueCollector<T> to include explicit branches
for 'PasswordCollector' and 'ValidatedPasswordCollector' that map to the correct
password-specific shape (or alternatively remove those two variants from
SingleValueCollectorTypes), and apply the same change to the related
generic/type mapping used around the other occurrence referenced (the similar
mapping at the later block).
---
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 472-493: The two factory functions accept the broader
PasswordField union; narrow their first-parameter types to the specific
discriminants so they only accept the correct variant: change
returnPasswordCollector(field: PasswordField, ...) to accept the PasswordField
variant whose type is 'PASSWORD' (e.g. PasswordField & { type: 'PASSWORD' } or
the existing concrete interface for PASSWORD fields), and change
returnValidatedPasswordCollector(field: PasswordField, ...) to accept only the
'PASSWORD_VERIFY' variant (e.g. PasswordField & { type: 'PASSWORD_VERIFY' } or
its concrete interface); keep the idx parameter and return types the same
(PasswordCollector and ValidatedPasswordCollector) so callers that pass wrong
discriminants will fail at compile time.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 765fd4fc-fa76-4ac8-a4e8-ac9f2a553249
📒 Files selected for processing (21)
.changeset/embed-password-policy-in-component.mde2e/davinci-app/components/password.tse2e/davinci-app/main.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/davinci.utils.test.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.tspackages/davinci-client/src/lib/mock-data/node.next.mock.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/src/lib/password-policy.rules.tspackages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
✅ Files skipped from review due to trivial changes (3)
- packages/davinci-client/src/lib/mock-data/node.next.mock.ts
- e2e/davinci-app/main.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/davinci-client/src/lib/davinci.utils.test.ts
- packages/davinci-client/src/lib/node.types.test-d.ts
- .changeset/embed-password-policy-in-component.md
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
- packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
- packages/davinci-client/src/lib/password-policy.rules.ts
- e2e/davinci-app/components/password.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
password-rules-toplevel
branch
2 times, most recently
from
9e2fce3 to
66eb586
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
password-rules-toplevel
branch
from
66eb586 to
df0ad30
Compare
…ssword policy DV-16053: PingOne moves password policy from the response root onto the PASSWORD_VERIFY field. Surfaces a typed ValidatedPasswordCollector that exposes the embedded policy and validates input against it, while leaving plain PASSWORD fields on the simpler PasswordCollector path. - Splits PASSWORD vs PASSWORD_VERIFY into PasswordCollector and ValidatedPasswordCollector; reducer discriminates by field.type. - New password-policy.rules.ts with pure rule functions (length, minUniqueCharacters, maxRepeatedCharacters, minCharacters) and returnPasswordPolicyValidator() for use by client.validate(). - client.validate() routes ValidatedPasswordCollector through the policy validator and rejects plain PasswordCollector with a typed error. - Updates collector type plumbing, mock data, sample app rendering, and regenerates API reports.
ryanbas21
force-pushed
the
password-rules-toplevel
branch
from
df0ad30 to
d8648fb
Compare
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We fixed a routing bug in the node reducer where PASSWORD and PASSWORD_VERIFY fields were dispatched based on the presence of field.passwordPolicy rather than field.type. This caused PASSWORD_VERIFY fields without a policy to incorrectly produce a PasswordCollector, and PASSWORD fields with a policy to incorrectly produce a ValidatedPasswordCollector. Splitting the cases to route strictly by field.type aligns the implementation with the PR intent and fixes both failing tests.
Warning
❌ We could not verify this fix.
diff --git a/packages/davinci-client/src/lib/node.reducer.ts b/packages/davinci-client/src/lib/node.reducer.ts
index d631c0cbc..922e655cb 100644
--- a/packages/davinci-client/src/lib/node.reducer.ts
+++ b/packages/davinci-client/src/lib/node.reducer.ts
@@ -176,11 +176,11 @@ export const nodeCollectorReducer = createReducer(initialCollectorValues, (build
// Intentional fall-through
return returnObjectSelectCollector(field, idx);
}
- case 'PASSWORD':
+ case 'PASSWORD': {
+ return returnPasswordCollector(field, idx);
+ }
case 'PASSWORD_VERIFY': {
- return field.passwordPolicy
- ? returnValidatedPasswordCollector(field, idx)
- : returnPasswordCollector(field, idx);
+ return returnValidatedPasswordCollector(field, idx);
}
case 'PHONE_NUMBER': {
const prefillData = data as PhoneNumberOutputValue;
Or Apply changes locally with:
npx nx-cloud apply-locally YMRJ-yYAU
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I think this looks good. I do have a question, but I'm not sure about it, so I'm asking you you all feel about it. Should passwordPolicy be in the input object or the output object?
| if ( | ||
| collectorToUpdate.type !== 'ValidatedPasswordCollector' && | ||
| !('validation' in collectorToUpdate.input) | ||
| ) { |
There was a problem hiding this comment.
This makes me think ... should the policies property be on the input or the output? The case for being on the input object is that it relates directly to the input value. But, these policies can also be rendered to screen, so it's kind of like output too. Hmm ...
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import { Array as Arr, Option, pipe } from 'effect'; |
There was a problem hiding this comment.
Cool, I'm good with this then. Just wanted to check. Thanks you two!
4 participants
Summary
PASSWORDandPASSWORD_VERIFYfields into two distinct collector types:PasswordCollector(no policy) andValidatedPasswordCollector(carries an embeddedpasswordPolicy).client.validate()to run a value through the embedded policy when given aValidatedPasswordCollector.What changed
PasswordPolicyinterface (davinci.types.ts); newValidatedPasswordCollectorinterface (collector.types.ts);PasswordFieldextracted fromStandardFieldto carry optionalpasswordPolicy.returnValidatedPasswordCollector(); existingreturnPasswordCollector()unchanged.field.type:PASSWORD→PasswordCollector;PASSWORD_VERIFY→ValidatedPasswordCollector(the embeddedpasswordPolicyis attached when present, otherwise an empty policy object is emitted).password-policy.rules.tsmodule: pure rule functions (length,minUniqueCharacters,maxRepeatedCharacters,minCharacters) andreturnPasswordPolicyValidator(collector).client.validate()ValidatedPasswordCollectorthroughreturnPasswordPolicyValidator; returns a descriptive error when called on a plainPasswordCollector.SingleValueCollectorTypes,InferSingleValueCollectorType,SingleValueCollectors,Collectors,CollectorValueTypeupdated to includeValidatedPasswordCollector.PASSWORD_VERIFYfield carries the policy (Solution 2 shape — embedded in the field, no root-level duplicate).Test plan
nx test davinci-client— 312 tests pass (22 files)nx typecheck davinci-client— cleannx lint davinci-client— no new warningsPASSWORD_VERIFY+ feature flagdv-16053-embed_password_policy_in_componentSummary by CodeRabbit