SDKS-4670: Add support for extension in PhoneNumberCollector#573
SDKS-4670: Add support for extension in PhoneNumberCollector#573
Conversation
🦋 Changeset detectedLatest commit: b271274 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 |
📝 WalkthroughWalkthroughAdds a PhoneNumberExtensionCollector and PhoneNumberExtensionField types; updates collector factories, client/store typings, reducer logic, UI rendering, and tests so PHONE_NUMBER fields with showExtension:true expose an extension input and propagate extension values through update and submit flows. ChangesPhone number extension feature
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as ObjectValueComponent
participant Client as DavinciClient.update
participant Reducer as NodeReducer
participant Store as ClientStore
participant Server as SubmitHandler
User->>UI: enter phoneNumber + extension
UI->>Client: update(PhoneNumberExtensionInputValue)
Client->>Reducer: dispatch updateCollectorValues
Reducer->>Store: persist collector.input.value {phoneNumber,countryCode,extension}
User->>UI: submit form
UI->>Server: POST form with collector outputs (includes extension)
Server->>Store: process submitted values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
View your CI Pipeline Execution ↗ for commit b271274
☁️ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/davinci-client/src/lib/davinci.types.ts (1)
154-162:⚠️ Potential issue | 🟡 Minor
showExtensionas required may bite older DaVinci environments.Marking
showExtension: booleanas required enforces it at the type level, but DaVinci responses in older/unupdated environments may omit the field. Consider making it optional (showExtension?: boolean) and defaulting tofalseincollector.utils.tsto preserve backward compatibility. Otherwise, this is effectively a runtime coupling on a specific DaVinci version.🤖 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 154 - 162, The PhoneNumberField type currently requires showExtension which breaks with older DaVinci payloads; make showExtension optional by changing its declaration to showExtension?: boolean in the PhoneNumberField type and update the code that consumes it (see collector.utils.ts) to treat missing values as false by default (i.e., when accessing field.showExtension, coerce undefined to false). Ensure all usages of PhoneNumberField and any destructuring in collector.utils.ts handle the optional property.packages/davinci-client/src/lib/collector.utils.ts (1)
646-665:⚠️ Potential issue | 🟡 MinorConfirm
field.showExtensionis always present from DaVinci.
PhoneNumberField.showExtensionis declared as a requiredbooleanin davinci.types.ts, andoptions = { showExtension: field.showExtension }passes it through directly. However,PhoneNumberOptions.showExtensionis optional in collector.types.ts. If DaVinci ever returns a PHONE field withoutshowExtension(older environments or feature-flag rollouts),options.showExtensionwill beundefinedrather than a safe default. Consider adding a defensive default to avoid ambiguity:Suggested fix
- options = { showExtension: field.showExtension }; + options = { showExtension: field.showExtension ?? false };🤖 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 646 - 665, The code passes field.showExtension directly into options but PhoneNumberOptions.showExtension is optional; make this defensive by ensuring options.showExtension is always a boolean (e.g., use nullish coalescing or Boolean conversion) so it defaults when PhoneNumberField may omit it; update the assignment of options (the variable named options and the property field.showExtension) to coerce a safe default (for example false) rather than letting undefined through.
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/collector.types.test-d.ts (1)
410-432: Outputvaluein the type-check literal is missingextension.The concrete
PhoneNumberCollectorliteral at line 428 setsoutput.value: { countryCode: '+1', phoneNumber: '5555555555' }withoutextension. That's fine becausePhoneNumberOutputValue.extensionis optional, but the companion test at line 382 does includeextension: ''in both input and output values — consistency here would better document the intended shape and guard against future regressions ifextensionis ever tightened to required on the output type.- value: { countryCode: '+1', phoneNumber: '5555555555' }, + value: { countryCode: '+1', phoneNumber: '5555555555', extension: '' },🤖 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 410 - 432, The test literal for PhoneNumberCollector omits the optional extension field in output.value; update the PhoneNumberCollector test object (the variable named collector used in the spec) so its output.value includes extension: '' to match the input.value and the PhoneNumberOutputValue shape (i.e., set output.value to { countryCode: '+1', phoneNumber: '5555555555', extension: '' }) so the literal remains consistent with PhoneNumberCollector and guards against future tightening of the type.packages/davinci-client/src/lib/collector.utils.ts (1)
680-686: Inconsistentoptionsfallback in output spread.
...(options && { options: options || [] })mixes object/array semantics. ForPhoneNumberCollector,optionsis{ showExtension: boolean }(an object), so the inner|| []fallback would yield a type‑incompatible empty array. It's also dead code — the outer&&already guaranteesoptionsis truthy. Consider simplifying to avoid future confusion:♻️ Proposed simplification
- ...(options && { options: options || [] }), - ...(defaultValue && { value: defaultValue }), + ...(options !== undefined && { options }), + ...(defaultValue !== undefined && { value: defaultValue }),🤖 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 680 - 686, The spread for options in the output object mixes array fallback and redundant checks; replace ...(options && { options: options || [] }) with a simple ...(options && { options }) (or equivalently include options only when defined) inside the output object in collector.utils.ts (the object that builds output: { key, label, type, ... }). This avoids returning an incompatible [] for PhoneNumberCollector (where options is an object) and removes the dead || [] code.
🤖 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.types.ts`:
- Around line 309-311: The PhoneNumberOptions interface currently makes
showExtension optional, but downstream code and collector.utils.ts
unconditionally sets options: { showExtension: field.showExtension }, so update
the type to reflect reality by changing PhoneNumberOptions to require
showExtension: boolean; also check collector.utils.ts and any other creators to
ensure they return a boolean (coerce if necessary) so the required type is
always satisfied across producers and consumers.
- Around line 297-307: The change made PhoneNumberInputValue.extension to be
required in collector.types.ts but tests and consumers still construct
PhoneNumberInputValue without extension; update all tests and any call sites
that create PhoneNumberInputValue (look for usages constructing objects with
countryCode and phoneNumber) to include extension: '' or appropriate value, and
add a changeset entry documenting this public breaking change so consumers are
aware; ensure PhoneNumberInputValue, PhoneNumberOutputValue, and any utility
that fallbacks extension remain consistent.
---
Outside diff comments:
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 646-665: The code passes field.showExtension directly into options
but PhoneNumberOptions.showExtension is optional; make this defensive by
ensuring options.showExtension is always a boolean (e.g., use nullish coalescing
or Boolean conversion) so it defaults when PhoneNumberField may omit it; update
the assignment of options (the variable named options and the property
field.showExtension) to coerce a safe default (for example false) rather than
letting undefined through.
In `@packages/davinci-client/src/lib/davinci.types.ts`:
- Around line 154-162: The PhoneNumberField type currently requires
showExtension which breaks with older DaVinci payloads; make showExtension
optional by changing its declaration to showExtension?: boolean in the
PhoneNumberField type and update the code that consumes it (see
collector.utils.ts) to treat missing values as false by default (i.e., when
accessing field.showExtension, coerce undefined to false). Ensure all usages of
PhoneNumberField and any destructuring in collector.utils.ts handle the optional
property.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.types.test-d.ts`:
- Around line 410-432: The test literal for PhoneNumberCollector omits the
optional extension field in output.value; update the PhoneNumberCollector test
object (the variable named collector used in the spec) so its output.value
includes extension: '' to match the input.value and the PhoneNumberOutputValue
shape (i.e., set output.value to { countryCode: '+1', phoneNumber: '5555555555',
extension: '' }) so the literal remains consistent with PhoneNumberCollector and
guards against future tightening of the type.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 680-686: The spread for options in the output object mixes array
fallback and redundant checks; replace ...(options && { options: options || []
}) with a simple ...(options && { options }) (or equivalently include options
only when defined) inside the output object in collector.utils.ts (the object
that builds output: { key, label, type, ... }). This avoids returning an
incompatible [] for PhoneNumberCollector (where options is an object) and
removes the dead || [] code.
🪄 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: 9343cbc3-031f-4e2b-b31e-cae954dcdb2c
📒 Files selected for processing (9)
.changeset/long-singers-do.mde2e/davinci-app/components/object-value.tse2e/davinci-suites/src/form-fields.test.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/node.reducer.test.ts
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (17.61%) 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 #573 +/- ##
===========================================
- Coverage 70.90% 17.61% -53.29%
===========================================
Files 53 154 +101
Lines 2021 24243 +22222
Branches 377 1159 +782
===========================================
+ Hits 1433 4271 +2838
- Misses 588 19972 +19384
🚀 New features to boost your workflow:
|
@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: |
|
Deployed 515784a to https://ForgeRock.github.io/ping-javascript-sdk/pr-573/515784a101b4b09f00ae11b0fb09325dce99cbb6 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%) 📊 Minor Changes📈 @forgerock/davinci-client - 48.9 KB (+0.3 KB) ➖ 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 |
ancheetah
force-pushed
the
SDKS-4670-phone-ext
branch
from
866974e to
9aa183c
Compare
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
Leaving some comments for discussion. I think we can make this non-breaking
| key: field.key, | ||
| label: field.label, | ||
| type: field.type, | ||
| ...(options && { options: options || [] }), |
There was a problem hiding this comment.
This fallback is no longer really necessary since options always has the property of showExtension?
I think this may be a time for us to pause and re-evaluate this structure of making collectors. This function's becoming overloaded with logic and I think it's only going to be harder to manage what is happening here.
There was a problem hiding this comment.
This is a bit messy because options can be an array of device options or a phone options object (showExtension). I would change this to options ?? null
There was a problem hiding this comment.
Since options is required and it's either an [] or a phone extension object, we don't even need null, we can just do options,
There was a problem hiding this comment.
Let's chat on Slack about the use of this options object for extensionLabel. I think it's actually the wrong place to for this property, and is possibly confusing us.
| required: boolean; | ||
| defaultCountryCode: string | null; | ||
| validatePhoneNumber: boolean; | ||
| showExtension: boolean; |
There was a problem hiding this comment.
Is this always going to be there or is it possible this can be optional?
There was a problem hiding this comment.
I have no clue! It seemed to still show up when I toggled it off.
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Added a few comments.
Ryan already mentioned about separating device authentication collector type from phone number collector type
| key: field.key, | ||
| label: field.label, | ||
| type: field.type, | ||
| ...(options && { options: options || [] }), |
There was a problem hiding this comment.
Since options is required and it's either an [] or a phone extension object, we don't even need null, we can just do options,
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@forgerock/davinci-client': minor | |||
There was a problem hiding this comment.
Should we mark this as major if it is a breaking change?
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/object-value.ts`:
- Around line 160-177: The change blocks clearing extension values by returning
early when extensionValue is empty; update the handler for extensionInput.change
(the event listener using extensionInput and phoneInput) to allow empty values
so clearing updates state: remove the early return and always construct a
PhoneNumberExtensionInputValue (use extension: extensionValue || '' and
countryCode: collector.output.value?.countryCode || '') and call
phoneNumberExtensionUpdater(phoneNumberExtensionInputValue) so an empty
extension clears the stored value.
In `@e2e/davinci-suites/src/form-fields.test.ts`:
- Around line 46-49: The test accesses parsedData.parameters.data without
guarding for missing POST body; update the extraction of data (variables
postData/parsedData/data) to safely handle absent or malformed bodies by
checking postData and parsedData.parameters exist before accessing .data (e.g.,
use optional chaining or provide default objects), and ensure data is assigned a
safe default when missing so no TypeError is thrown during the test.
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 402-407: The input.validation type on
PhoneNumberExtensionCollector is too narrow; update the type declaration for
input.validation in the PhoneNumberExtensionCollector (or the related input
interface) to include ValidationPhoneNumber alongside ValidationRequired so that
returnObjectCollector can push validatePhoneNumber rules for PHONE_NUMBER fields
(including extension-enabled ones). Locate the input shape referenced as input:
{ key: string; value: PhoneNumberExtensionInputValue; type: string; validation:
ValidationRequired[] | null; } and change the validation union to accept
ValidationPhoneNumber[] (or a union type like (ValidationRequired |
ValidationPhoneNumber)[] | null) so validatePhoneNumber and other phone-specific
validators are allowed. Ensure references to returnObjectCollector and
validatePhoneNumber remain type-safe after the change.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 326-333: The null case isn't guarded before doing property checks
on action.payload.value in the extension update path; update the validation in
the reducer so you first confirm action.payload.value is a non-null object
(e.g., check value !== null && typeof value === 'object' or use a truthy object
check) before running the 'phoneNumber' / 'countryCode' / 'extension' in checks
so null payloads throw the same predictable Error('Value argument must be an
object') instead of causing a runtime crash.
🪄 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: 6d44dea6-6496-49d2-8bd6-4e68463a3807
📒 Files selected for processing (14)
.changeset/long-singers-do.mde2e/davinci-app/components/object-value.tse2e/davinci-app/main.tse2e/davinci-suites/src/form-fields.test.tspackages/davinci-client/src/lib/client.store.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/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.ts
✅ Files skipped from review due to trivial changes (3)
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/node.types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/long-singers-do.md
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
| const postData = request.postData(); | ||
| const parsedData = postData ? JSON.parse(postData) : {}; | ||
| const data = parsedData.parameters.data; | ||
|
|
There was a problem hiding this comment.
Guard parameters.data access when POST body is absent.
At Line 47, parsedData can be {}, but Line 48 still assumes parsedData.parameters.data exists. This can fail with a TypeError and hide the real failure cause.
🔧 Proposed fix
const postData = request.postData();
-const parsedData = postData ? JSON.parse(postData) : {};
-const data = parsedData.parameters.data;
+expect(postData).toBeTruthy();
+const parsedData = JSON.parse(postData!);
+const data = parsedData?.parameters?.data;
+expect(data).toBeTruthy();📝 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 postData = request.postData(); | |
| const parsedData = postData ? JSON.parse(postData) : {}; | |
| const data = parsedData.parameters.data; | |
| const postData = request.postData(); | |
| expect(postData).toBeTruthy(); | |
| const parsedData = JSON.parse(postData!); | |
| const data = parsedData?.parameters?.data; | |
| expect(data).toBeTruthy(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/davinci-suites/src/form-fields.test.ts` around lines 46 - 49, The test
accesses parsedData.parameters.data without guarding for missing POST body;
update the extraction of data (variables postData/parsedData/data) to safely
handle absent or malformed bodies by checking postData and parsedData.parameters
exist before accessing .data (e.g., use optional chaining or provide default
objects), and ensure data is assigned a safe default when missing so no
TypeError is thrown during the test.
1529c56 to
0e0e8bd
Compare
| key: field.key, | ||
| label: field.label, | ||
| type: field.type, | ||
| ...(options && { options: options || [] }), |
There was a problem hiding this comment.
options can now be an object or array...
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We determined this timeout failure in @forgerock/api-report:test is unrelated to the PR's changes, as the api-report project is not touched and its tests use entirely synthetic fixtures independent of davinci-client. The CI machine appears to be under resource pressure from the broader build, causing computationally intensive api-extractor tests to exceed their 5000ms timeout threshold.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 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 we are super close with the exception of the use of options. Let's chat in Slack about it.
| key: string; | ||
| label: string; | ||
| type: string; | ||
| options: PhoneNumberExtensionOptions; |
There was a problem hiding this comment.
Is this really an "option", now that we have a dedicated collector for it. To me, this seems to be an additional label. Is there anything that would prevent us from putting it at the same level as label?
output: {
key: string;
label: string;
type: string;
extensionLabel: string;
value: PhoneNumberExtensionOutputValue;
};
I ask because the intention of options was to communicate the "options" from which a user chooses, like the options of a select element, for example.
There was a problem hiding this comment.
I actually wanted to put it at the same level as label initially but wasn't sure if this went against any collector patterns. I'm going to go ahead and make this change but we can still discuss in Slack.
There was a problem hiding this comment.
Ok it's been updated. You can view the changes in the last commit. Lmk if it looks good and I'll squash and merge.
| key: field.key, | ||
| label: field.label, | ||
| type: field.type, | ||
| ...(options && { options: options || [] }), |
There was a problem hiding this comment.
Let's chat on Slack about the use of this options object for extensionLabel. I think it's actually the wrong place to for this property, and is possibly confusing us.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/davinci-suites/src/phone-number-field.test.ts (1)
108-108: Use a stricter absence assertion for the extension field.The test currently uses
getByText('Extension').not.toBeVisible(), which relies on broad text matching and a visibility negation. Per Playwright semantics,not.toBeVisible()succeeds when the locator matches zero elements, buttoHaveCount(0)is more semantically precise for asserting complete DOM absence. Additionally,getByRolewith a specific role is more robust thangetByText, which could inadvertently match unrelated "Extension" text elsewhere on the page.Suggested test adjustment
- await expect(page.getByText('Extension')).not.toBeVisible(); // Tests standard PhoneNumberCollector + await expect(page.getByRole('textbox', { name: 'Extension' })).toHaveCount(0); // Tests standard PhoneNumberCollector🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/davinci-suites/src/phone-number-field.test.ts` at line 108, Replace the broad visibility negation using page.getByText('Extension') with a stricter DOM-absence assertion: locate the extension field by role (e.g., page.getByRole('textbox', { name: 'Extension' })) and assert expect(...).toHaveCount(0) instead of expect(...).not.toBeVisible(); update the test code where expect(page.getByText('Extension')).not.toBeVisible() is used to use page.getByRole(...) and expect(...).toHaveCount(0) so the assertion verifies the element is completely absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/davinci-suites/src/phone-number-field.test.ts`:
- Line 108: Replace the broad visibility negation using
page.getByText('Extension') with a stricter DOM-absence assertion: locate the
extension field by role (e.g., page.getByRole('textbox', { name: 'Extension' }))
and assert expect(...).toHaveCount(0) instead of expect(...).not.toBeVisible();
update the test code where expect(page.getByText('Extension')).not.toBeVisible()
is used to use page.getByRole(...) and expect(...).toHaveCount(0) so the
assertion verifies the element is completely absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8c3d661-e128-4282-94d7-65affe4b7e6a
📒 Files selected for processing (16)
.changeset/long-singers-do.md.gitignoree2e/davinci-app/components/object-value.tse2e/davinci-app/main.tse2e/davinci-suites/src/form-fields.test.tse2e/davinci-suites/src/phone-number-field.test.tspackages/davinci-client/src/lib/client.store.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/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.ts
✅ Files skipped from review due to trivial changes (3)
- packages/davinci-client/src/lib/node.types.test-d.ts
- .gitignore
- packages/davinci-client/src/lib/node.types.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e/davinci-app/main.ts
- .changeset/long-singers-do.md
- packages/davinci-client/src/lib/client.store.ts
- e2e/davinci-suites/src/form-fields.test.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/collector.types.test-d.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
- e2e/davinci-app/components/object-value.ts
ancheetah
force-pushed
the
SDKS-4670-phone-ext
branch
from
d946302 to
507902e
Compare
| throw new Error('Index argument must be a string'); | ||
| } | ||
| if (typeof action.payload.value !== 'object') { | ||
| throw new Error('Value argument must be an object'); |
There was a problem hiding this comment.
We need to keep reducers pure.
| !('countryCode' in action.payload.value) || | ||
| !('extension' in action.payload.value) | ||
| ) { | ||
| throw new Error( |
There was a problem hiding this comment.
throwing in a reducer is impure.
There was a problem hiding this comment.
We throw in this reducer for all the other collector categories
There was a problem hiding this comment.
we shouldn't then, that's bad.
There was a problem hiding this comment.
I agree this could make our node updates fragile but since the reducer has historically been written this way I don't think we should fix this right now. We can create a a ticket to fix and test this in the future.
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Minor comment on whether it makes sense to combine types, looks good overall! Approving!
| phoneNumber?: string; | ||
| } | ||
|
|
||
| export interface PhoneNumberExtensionInputValue { |
There was a problem hiding this comment.
What's the need for two different types here when both PhoneNumberExtensionInputValue and PhoneNumberExtensionOutputValue have the same properties? While one type does have properties as optional I don't see the benefit of separating those properties. I looked at how they are used in the code and using separate types makes it harder readability wise. I think we can just use PhoneNumberExtensionOutputValue and call it PhoneNumberExtensionValue, unless we have a pattern of always separating input and output types, regardless of properties.
There was a problem hiding this comment.
This makes the types more strict. They represent different things. Output is what we get from DaVinci. These are prefilled values that may or may not be provided by the flow. Input is what we collect from the end user send to DaVinci. These properties are required. I suppose we could do something like PhoneOutput = Partial<PhoneInput> to make it a little cleaner but there's not much of a benefit. And if the requirements change later on it will be harder to modify these types. Lmk what you think.
| defaultValue = { | ||
| countryCode: prefilledCountryCode ? prefilledCountryCode : field.defaultCountryCode || '', | ||
| phoneNumber: prefilledPhone || '', | ||
| extension: prefilledExtension ?? '', |
There was a problem hiding this comment.
Nbd, but ?? '' might be redundant here, since prefilledExtension value defaults to empty string on line 674.
There was a problem hiding this comment.
prefillData.extension is an optional property so this can be string | undefined.
ancheetah
force-pushed
the
SDKS-4670-phone-ext
branch
from
507902e to
b271274
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/davinci-app/components/object-value.ts (1)
88-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
PhoneNumberCollectorguard blocks clearing and has a misleading error message.
if (!selectedValue)prevents the updater from being called when the user clears the phone field, leaving stale state — the same problem that was already caught and fixed for the extension input. The error message ('No value found for the selected option') is also a copy-paste from theDeviceAuthenticationCollectorbranch and is semantically wrong here.🛠 Proposed fix
phoneInput.addEventListener('change', (event) => { const target = event.target as HTMLInputElement; const selectedValue = target.value; - if (!selectedValue) { - console.error('No value found for the selected option'); - return; - } - const phoneNumberInputValue: PhoneNumberInputValue = { phoneNumber: selectedValue, countryCode: collector.output.value?.countryCode || '', }; const phoneNumberUpdater = updater as Updater<PhoneNumberCollector>; phoneNumberUpdater(phoneNumberInputValue); });🤖 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 `@e2e/davinci-app/components/object-value.ts` around lines 88 - 104, The change handler for phoneInput (the listener that constructs PhoneNumberInputValue and calls updater as Updater<PhoneNumberCollector>) currently bails out on falsy selectedValue and logs a wrong error message; remove that guard so the updater is always called even when the field is cleared, populate PhoneNumberInputValue.phoneNumber with the empty string when selectedValue is empty, and either remove or replace the incorrect log message ('No value found for the selected option') with a correct one (e.g., 'Phone input cleared') so stale state is not retained by PhoneNumberCollector and the updater(…) is invoked on clears.
♻️ Duplicate comments (1)
e2e/davinci-suites/src/form-fields.test.ts (1)
46-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
parsedData.parameters.datais still unguarded after the partial fix.If
postDatais absent,parsedDatadefaults to{}and Line 48 immediately throwsTypeError: Cannot read properties of undefined (reading 'data'). The guard onpostDatadoesn't protect the downstream access.🛠 Proposed fix
- const postData = request.postData(); - const parsedData = postData ? JSON.parse(postData) : {}; - const data = parsedData.parameters.data; + const postData = request.postData(); + expect(postData).toBeTruthy(); + const parsedData = JSON.parse(postData!); + const data = parsedData?.parameters?.data; + expect(data).toBeTruthy();🤖 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 `@e2e/davinci-suites/src/form-fields.test.ts` around lines 46 - 48, The code unguards parsedData.parameters before accessing .data, causing a TypeError when postData is absent; update the extraction so you safely handle missing parameters and data (e.g., use optional chaining and a sensible default). Specifically, change the logic around request.postData(), parsedData and data so that parsedData is parsed only when present and data is read as parsedData.parameters?.data (or parsedData?.parameters?.data) with a fallback (e.g., {} or null) to avoid the runtime exception; target the variables postData, parsedData and data in the test to apply this safe-access fix.
🤖 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 `@e2e/davinci-app/components/object-value.ts`:
- Line 129: Replace the invalid assignment to the readonly style property by
setting the element's cssText: find the statement assigning to divEl.style (the
line "divEl.style = 'display: flex; gap: 8px;';") and change it to assign the
CSS string to divEl.style.cssText so TypeScript's readonly CSSStyleDeclaration
typing is respected.
---
Outside diff comments:
In `@e2e/davinci-app/components/object-value.ts`:
- Around line 88-104: The change handler for phoneInput (the listener that
constructs PhoneNumberInputValue and calls updater as
Updater<PhoneNumberCollector>) currently bails out on falsy selectedValue and
logs a wrong error message; remove that guard so the updater is always called
even when the field is cleared, populate PhoneNumberInputValue.phoneNumber with
the empty string when selectedValue is empty, and either remove or replace the
incorrect log message ('No value found for the selected option') with a correct
one (e.g., 'Phone input cleared') so stale state is not retained by
PhoneNumberCollector and the updater(…) is invoked on clears.
---
Duplicate comments:
In `@e2e/davinci-suites/src/form-fields.test.ts`:
- Around line 46-48: The code unguards parsedData.parameters before accessing
.data, causing a TypeError when postData is absent; update the extraction so you
safely handle missing parameters and data (e.g., use optional chaining and a
sensible default). Specifically, change the logic around request.postData(),
parsedData and data so that parsedData is parsed only when present and data is
read as parsedData.parameters?.data (or parsedData?.parameters?.data) with a
fallback (e.g., {} or null) to avoid the runtime exception; target the variables
postData, parsedData and data in the test to apply this safe-access fix.
🪄 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: 62800af6-f3d4-4daf-b182-ec8f06107272
📒 Files selected for processing (16)
.changeset/long-singers-do.md.gitignoree2e/davinci-app/components/object-value.tse2e/davinci-app/main.tse2e/davinci-suites/src/form-fields.test.tse2e/davinci-suites/src/phone-number-field.test.tspackages/davinci-client/src/lib/client.store.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/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.ts
✅ Files skipped from review due to trivial changes (6)
- .changeset/long-singers-do.md
- .gitignore
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/davinci-app/main.ts
- packages/davinci-client/src/lib/collector.types.test-d.ts
- packages/davinci-client/src/lib/davinci.types.ts
- e2e/davinci-suites/src/phone-number-field.test.ts
- packages/davinci-client/src/lib/collector.utils.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
| extensionInput.setAttribute('placeholder', 'Enter extension'); | ||
|
|
||
| const divEl = document.createElement('div'); | ||
| divEl.style = 'display: flex; gap: 8px;'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript lib.dom.d.ts ElementCSSInlineStyle style readonly CSSStyleDeclaration 2025
💡 Result:
In TypeScript's lib.dom.d.ts (as of 2024/2025 DOM updates), the ElementCSSInlineStyle interface declares the style property as readonly CSSStyleDeclaration. This means you cannot reassign the style property itself (e.g., element.style = someOtherDeclaration), but you can mutate the returned CSSStyleDeclaration object by setting individual properties (e.g., element.style.color = 'red'), using cssText (e.g., element.style.cssText = 'color: red'), or calling methods like setProperty. This design matches the DOM spec and browser behavior: the style property returns a live CSSStyleDeclaration object tied to the element's inline styles, which is mutable despite the readonly getter. Direct string assignment like element.style = 'color: red' works in browsers (forwarded to cssText), but TypeScript types it as readonly to prevent replacing the object, avoiding type errors for incompatible assignments. A long-open GitHub issue (#38838) requests allowing string setters per spec, but it remains open in the Backlog as of 2025. No changes in 2025 DOM updates (e.g., PR #60987, #61647) alter this; recent updates add CSS properties and accessors but keep style readonly. For dynamic keys, cast or use setProperty. Example: const el = document.createElement('div') as HTMLElement; el.style.color = 'red'; // OK el.style.cssText = 'color: red; font-size: 16px'; // OK el.style.setProperty('background', 'blue'); // OK el.style = /* CSSStyleDeclaration */; // Error: readonly
Citations:
- 1: https://typhonjs-typedoc.github.io/ts-lib-docs/2024/dom/interfaces/ElementCSSInlineStyle.html
- 2: ElementCSSInlineStyle.style should not be read-only microsoft/TypeScript#38838
- 3: https://typhonjs-typedoc.github.io/ts-lib-docs/2024/dom/interfaces/CSSStyleDeclaration.html
- 4: https://github.com/microsoft/TypeScript/blob/e60c210c572a12de38551ac1d1e8716587dbcc33/lib/lib.dom.d.ts
- 5: https://raw.githubusercontent.com/microsoft/TypeScript/main/src/lib/dom.generated.d.ts
- 6: DOM update 2025/01/16 microsoft/TypeScript#60987
- 7: https://developer.mozilla.org/en/DOM/element.style
🏁 Script executed:
# Check if the file exists and read around line 129
file_path="e2e/davinci-app/components/object-value.ts"
if [ -f "$file_path" ]; then
echo "=== File exists ==="
# Show lines around 129 with context
sed -n '120,140p' "$file_path" | cat -n
else
echo "File not found: $file_path"
# Try to find similar files
find . -name "object-value.ts" -type f 2>/dev/null
fiRepository: ForgeRock/ping-javascript-sdk
Length of output: 1080
🏁 Script executed:
# Check for tsconfig files and TypeScript configuration
echo "=== Looking for tsconfig files ==="
find . -name "tsconfig*.json" -type f 2>/dev/null | head -20
echo ""
echo "=== Checking project structure around the file ==="
ls -la e2e/davinci-app/components/ 2>/dev/null | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 1851
🏁 Script executed:
# Check the tsconfig files for davinci-app
echo "=== e2e/davinci-app/tsconfig.json ==="
cat e2e/davinci-app/tsconfig.json
echo ""
echo "=== e2e/davinci-app/tsconfig.app.json ==="
cat e2e/davinci-app/tsconfig.app.jsonRepository: ForgeRock/ping-javascript-sdk
Length of output: 1186
divEl.style = '...' is a TypeScript compile error (Cannot assign to 'style' because it is a read-only property).
The style property is typed as readonly CSSStyleDeclaration in TypeScript's DOM type definitions. Direct string assignment is not permitted, even though browsers forward it to cssText. Use divEl.style.cssText instead.
🛠 Proposed fix
- divEl.style = 'display: flex; gap: 8px;';
+ divEl.style.cssText = 'display: flex; gap: 8px;';📝 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.
| divEl.style = 'display: flex; gap: 8px;'; | |
| divEl.style.cssText = 'display: flex; gap: 8px;'; |
🤖 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 `@e2e/davinci-app/components/object-value.ts` at line 129, Replace the invalid
assignment to the readonly style property by setting the element's cssText: find
the statement assigning to divEl.style (the line "divEl.style = 'display: flex;
gap: 8px;';") and change it to assign the CSS string to divEl.style.cssText so
TypeScript's readonly CSSStyleDeclaration typing is respected.
6 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4670
Description
Added
PhoneNumberExtensionCollector— a new first-class collector type for DaVinciPHONE_NUMBERfields that include an extension. When a field hasshowExtension: true, the SDK now produces aPhoneNumberExtensionCollectorinstead of aPhoneNumberCollector.New Types (
collector.types.ts)PhoneNumberExtensionInputValue—{ countryCode: string; phoneNumber: string; extension: string }PhoneNumberExtensionOutputValue—{ countryCode?: string; phoneNumber?: string; extension?: string }PhoneNumberExtensionCollector—ObjectValueCollectorwithextensionLabel: stringas a direct property onoutputObjectValueCollectorTypes,InferValueObjectCollectorType, andObjectValueCollectorsNew Types (
davinci.types.ts)PhoneNumberExtensionField— extendsPhoneNumberFieldwithshowExtension: booleanandextensionLabel: stringComplexValueFieldsUpdated Functions (
collector.utils.ts)returnObjectCollectorFieldconstraint widened to includePhoneNumberExtensionFieldprefillDataparameter type widened toPhoneNumberOutputValue | PhoneNumberExtensionOutputValuereturnObjectValueCollectorshowExtension === true: produces aPhoneNumberExtensionCollectorwithextensionLabelonoutputandextensionininput.valuePhoneNumberCollectoras beforeextensionLabeluses a sentinelnullso an empty-string label is correctly included viaextensionLabel !== nullconditional spreadReducer (
node.reducer.ts)PhoneNumberExtensionCollectoradded to the initial state type unionnode/updatehandler validates and applies{ countryCode, phoneNumber, extension }tocollector.input.valuenode/nextPHONE_NUMBER prefill cast updated toPhoneNumberOutputValue | PhoneNumberExtensionOutputValueBreaking Changes
None.
PhoneNumberInputValueandPhoneNumberOutputValueare unchanged frommain.returnObjectCollector's signature changes are purely additive.Test Coverage
collector.utils.test.ts(48 passing) andnode.reducer.test.ts(29 passing)Summary by CodeRabbit
New Features
showExtension: true, users can now input both a phone number and an extension value, with both values captured in form submissions.Chores