feat(reactjs-todo-davinci): add new collectors (SDKS-5052)#117
Open
ancheetah wants to merge 4 commits into
Open
feat(reactjs-todo-davinci): add new collectors (SDKS-5052)#117ancheetah wants to merge 4 commits into
ancheetah wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR expands DaVinci collector support by introducing new component types (Boolean, QR Code, Device, Phone Number, Polling), a rich-content interpolation utility, enhanced ReadOnly renderer for multiple collector types, pollStatus hook exposure, and form routing updates integrating all new components with dependency pinning. ChangesDaVinci Collector Components and Integration
Sequence DiagramsequenceDiagram
participant Form as Form Component
participant Component as Collector Component
participant Hook as useDavinci Hook
participant DavinciClient
rect rgba(100, 150, 200, 0.5)
note over Form,DavinciClient: Device/Phone Collector Flow
Form->>Component: render with collector props
Component->>Component: initialize state from collector
Component->>Hook: call updater(value)
Hook->>DavinciClient: submit collector response
DavinciClient-->>Hook: return result/error
Hook-->>Component: return status
end
rect rgba(100, 150, 200, 0.5)
note over Form,DavinciClient: Polling Collector Flow
Form->>Component: render PollingComponent
Component->>Hook: call pollStatus(collector)
Hook->>DavinciClient: pollStatus(collector)
DavinciClient-->>Hook: return polling status
Hook-->>Component: return status
Component->>Hook: call updater(status)
Hook->>DavinciClient: submit collector response
DavinciClient-->>Hook: return result/error
Hook-->>Component: return outcome
Component->>Hook: call submitForm()
Hook-->>Component: advance flow
end
rect rgba(100, 150, 200, 0.5)
note over Form,Component: ReadOnly/Boolean Rendering
Form->>Component: render ReadOnly or BooleanComponent
Component->>Component: interpolate rich content if present
Component-->>Form: render label/content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
javascript/reactjs-todo-davinci/client/components/davinci-client/error.js (1)
22-22:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix JSX interpolation syntax error.
Line 22 uses
${...}inside JSX text content, which renders the literal characters$,{,}instead of interpolating the expression. In JSX, dynamic content must be wrapped in{...}(single braces), not${...}(template literal syntax).Additionally, there's a typo: "Occured" should be "Occurred".
🐛 Proposed fix for interpolation and typo
- return error ? <pre>${error?.message ?? 'An Error Occured'}</pre> : null; + return error ? <pre>{error?.message ?? 'An Error Occurred'}</pre> : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/error.js` at line 22, The JSX return uses template literal syntax instead of JSX expression braces and has a typo; change the returned text so the expression uses {error?.message ?? 'An Error Occurred'} instead of `${error?.message ?? 'An Error Occured'}` and correct "Occured" to "Occurred" in the fallback string; update the return expression that references the error variable accordingly.
🧹 Nitpick comments (4)
javascript/reactjs-todo-davinci/client/components/davinci-client/boolean.js (1)
36-47: 💤 Low valueConsider wrapping input inside label for better accessibility.
The current structure places the
<input>outside the<label>, relying solely onhtmlForfor association. Wrapping the input inside the label improves usability by allowing clicks anywhere on the label text to toggle the checkbox, which is a common UX pattern for checkboxes.♻️ Optional refactor for better UX
return ( <div className={'mb-5 form-check'}> - <label htmlFor={fieldId} className="form-label"> - {label} - </label> - <input - type="checkbox" - id={fieldId} - name={fieldId} - checked={isChecked} - onChange={handleChange} - className="form-check-input" - required={required} - /> + <label htmlFor={fieldId} className="form-label form-check-label"> + <input + type="checkbox" + id={fieldId} + name={fieldId} + checked={isChecked} + onChange={handleChange} + className="form-check-input" + required={required} + /> + {label} + </label> </div> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/boolean.js` around lines 36 - 47, Wrap the checkbox input inside the label to improve accessibility and click area: move the <input> element to be a child of the <label> that uses the label variable (currently using htmlFor={fieldId}), keeping the existing props (id/name={fieldId}, checked={isChecked}, onChange={handleChange}, className="form-check-input", required={required}) intact and remove the separate htmlFor association if redundant so clicking the label text toggles the checkbox.javascript/reactjs-todo-davinci/client/components/davinci-client/readonly.js (1)
14-46: ⚡ Quick winAdd default case for unknown collector types.
The function branches on
collectorTypefor three specific types but has no default handler. If an unknown collector type is passed (due to a typo, API change, or new collector type), the function will implicitly returnundefined, rendering nothing without any warning or error.🔧 Proposed fix to add default case
</div> ); } + + // Unknown collector type + console.warn(`ReadOnly: Unknown collector type "${collectorType}"`); + return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/readonly.js` around lines 14 - 46, The ReadOnly component currently returns nothing for unknown collector types; update the end of the if/else chain in ReadOnly to handle a default case that both logs the unexpected collector.type (use console.warn(`ReadOnly: unknown collector type: ${collectorType}`, collector) or similar) and returns a safe fallback UI (e.g., null or a small placeholder like a <p> stating the type is unsupported) so the component never implicitly returns undefined; reference the ReadOnly function and the collectorType variable when adding this default handler.javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js (1)
31-34: 💤 Low valueRemove unnecessary
event.preventDefault()in select handler.Calling
event.preventDefault()on aselectelement'schangeevent is unnecessary—the default behavior of a select change doesn't need to be prevented. If the object accessed or function called using this operator is undefined or null, the expression short circuits and evaluates to undefined instead of throwing an error. This doesn't affect theselectchange event, which doesn't have a preventable default action like form submission.♻️ Proposed simplification
const handleChangeDevice = (event) => { - event.preventDefault(); setSelectedDevice(event.target.value); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js` around lines 31 - 34, The select change handler handleChangeDevice should not call event.preventDefault(); remove that call and simply read the selected value and call setSelectedDevice(event.target.value) (i.e., update state directly in handleChangeDevice) so the handler is simplified and avoids an unnecessary default-prevention call.javascript/reactjs-todo-davinci/client/components/davinci-client/form.js (1)
176-176: 💤 Low valueUse template literals for consistent key and inputName generation.
The code uses string concatenation (
idx + collectorName) to generate keys andinputNamevalues, which creates strings like"0collectorName". While functional, this is inconsistent with other cases that use justcollectorNameoridx + 'err', and less readable than template literals. Consider using`${idx}-${collectorName}`for clarity and consistency across all collector cases.♻️ Proposed improvement
- return <ReadOnly key={idx + collectorName} collector={collector} />; + return <ReadOnly key={`${idx}-${collectorName}`} collector={collector} />;<BooleanComponent - key={idx + collectorName} + key={`${idx}-${collectorName}`} collector={collector}<ObjectValueComponent - inputName={idx + collectorName} + inputName={`${idx}-${collectorName}`} collector={collector} updater={updater(collector)} - key={idx + collectorName} + key={`${idx}-${collectorName}`} />Also applies to: 181-181, 191-191, 194-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/form.js` at line 176, Replace the string concatenation used to build React keys and inputName values with template literals for clarity and consistency: update occurrences such as the ReadOnly render (key currently built as idx + collectorName) and the other collector cases that build keys/inputName (e.g., uses of idx + 'err' or idx + collectorName) to use format like `${idx}-${collectorName}` or `${idx}-err` so keys/inputName are clear and consistent across the components (look for the ReadOnly render and the other collector render sites to apply this change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/boolean.js`:
- Line 17: The current fallback for fieldId uses the static string
'checkbox-field', which causes duplicate HTML ids when multiple
ValidatedBooleanCollector instances render without collector.output.key; change
the fallback to generate a unique id (e.g., use React's useId() or an
incremental/UUID suffix) and assign it to the same fieldId variable so the
input's id and the label's htmlFor both use this unique value (refer to
collector.output.key and the fieldId usage within the ValidatedBooleanCollector
component).
- Line 14: The BooleanComponent declares an unused prop collectorName which is
not provided by the parent and results in an ineffective wrapper key; remove the
collectorName parameter from the BooleanComponent signature and delete the
redundant key on the wrapper div (the key usage around the component instance is
already handled by the parent), leaving the props as ({ collector, updater })
and keeping internal logic unchanged.
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js`:
- Line 15: The initializer for selectedDevice (in the useState call for
selectedDevice / setSelectedDevice) assumes collector.output.options has a 0th
element and accesses .value directly, which will throw if options is an empty
array; change the expression to guard the element access (e.g., check
options.length or use optional chaining on the element) so you read
collector.output.options?.[0]?.value ?? null (or equivalent length check) to
safely default to null when options is missing or empty.
- Around line 22-29: The effect in useEffect should include updater and
collector.type in its dependency array to avoid stale closures: update the
dependencies from [selectedDevice] to [selectedDevice, updater, collector.type]
(since updater is derived from updater(collector) in the parent and
collector.type can change) so the effect re-runs when the updater function or
collector type changes while preserving the existing conditional and call to
updater(selectedDevice).
- Around line 95-97: The current computed const required =
collector.input.validation?.some(...) is applied to both phone and extension
inputs but may only reflect phone-number validation; update the rendering/props
so the phone input uses that computed required flag (from
collector.input.validation) while the extension input derives its own required
state (likely false) or checks a separate validation rule (e.g., look for an
"extension" key in collector.input.validation) instead of reusing required for
both; locate the constant required and the JSX where the phone and extension
inputs are created and adjust the extension input prop to use a dedicated check
(or explicit false) so extension is optional unless a specific
extension-required rule is present.
- Around line 83-86: ObjectValueComponent is passing countryCode:
collector.input.value?.countryCode into updater for PhoneNumberCollector and
PhoneNumberExtensionCollector (calls to updater in ObjectValueComponent), which
can send undefined into davinciClient.update; fix by ensuring countryCode is
defined before calling updater — either compute a sane default (e.g. derive from
collector.metadata or fallback constant) and pass that instead, or build the
payload conditionally so you omit countryCode when
collector.input.value?.countryCode is falsy; update the updater invocation sites
in ObjectValueComponent (the PhoneNumberCollector/PhoneNumberExtensionCollector
branches) so the payload never contains countryCode: undefined before calling
davinciClient.update.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/qr-code.js`:
- Around line 21-30: The component currently accesses collector.output.src
directly and may render a broken image; update the QR code rendering in
qr-code.js to validate collector.output and collector.output.src (use optional
chaining or an explicit null check) before rendering the <img>, add an onError
handler for the element with id "qr-code-image" to hide the image and reveal a
fallback (the existing paragraph with id "qr-code-fallback" or a placeholder)
when loading fails, and ensure the fallback paragraph only renders when either
src is missing or the image load errors to avoid runtime exceptions.
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/readonly.js`:
- Around line 36-38: The early exit in the component checks if
(!componentEnabled) { return; } which implicitly returns undefined; change this
to explicitly return null so the React component signals "render nothing".
Update the return in the component function where componentEnabled is checked
(the if block that currently returns) to return null instead of an empty return;
no other changes required.
- Line 17: Remove the trailing whitespace on line 17 in the readonly.js
component file (the Davinci client read-only component) by deleting any extra
spaces at end-of-line and reformatting the file (run Prettier or eslint --fix)
so the file complies with the Prettier/ESLint formatting rule.
- Around line 18-25: The code accesses output.content and
richContent.replacements without null checks; update the ReadOnlyCollector /
RichTextCollector branch to defensively guard these values by verifying output
and output.content exist before rendering and that richContent and
richContent.replacements are arrays before checking length (use optional
chaining or explicit checks); refer to the collectorType variable and the output
and richContent objects (and the replacements property) when adding the guards
and provide a sensible fallback (e.g., empty string or alternative message)
instead of directly dereferencing possibly undefined values.
In `@javascript/reactjs-todo-davinci/client/components/utilities/rich-content.js`:
- Around line 12-35: The function interpolateRichContent currently assumes
richContent.content and richContent.replacements exist; add defensive validation
at the top of interpolateRichContent to guard against malformed input by
checking that richContent is an object, that richContent.content is a string
(fallback to empty string) and richContent.replacements is an array (fallback to
[]), then compute segments = richContent.content.split(...) and replacementMap =
new Map(richContent.replacements.map(...)) using those validated/fallback values
so the rest of the function can run without throwing on null/undefined.
In `@javascript/reactjs-todo-davinci/client/styles/README.md`:
- Line 3: Fix the subject-verb agreement in the README sentence that currently
reads "This ensure explicit communication..." by changing "ensure" to "ensures"
so the sentence reads "This ensures explicit communication..." — update the line
in client/styles/README.md that contains the phrase "This ensure explicit
communication that we are overriding this component with a custom class." to use
"ensures".
In `@javascript/reactjs-todo-davinci/package.json`:
- Line 42: The package.json currently pins "`@forgerock/davinci-client`" to a
temporary pkg.pr.new URL; update package.json to reference a stable source
instead—either a published semver version (e.g., "`@forgerock/davinci-client`":
"x.y.z") or your permanent private registry URL, and regenerate the lockfile
(run npm/yarn/pnpm install) so package-lock.json or yarn.lock no longer contains
the pkg.pr.new URL; look for the dependency key "`@forgerock/davinci-client`" in
package.json and update it, then commit the updated lockfile.
In `@javascript/reactjs-todo-davinci/README.md`:
- Line 11: The README currently lists "ValidatedPasswordCollector" as supported
but the PR scope says "ValidatedPasswordCollector will be addressed in a
separate PR"; remove the "ValidatedPasswordCollector" entry from the supported
list in README.md (or replace it with a note that it's out-of-scope) so the
documentation matches the PR scope; look for the literal string
ValidatedPasswordCollector in the README and update the list accordingly.
---
Outside diff comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/error.js`:
- Line 22: The JSX return uses template literal syntax instead of JSX expression
braces and has a typo; change the returned text so the expression uses
{error?.message ?? 'An Error Occurred'} instead of `${error?.message ?? 'An
Error Occured'}` and correct "Occured" to "Occurred" in the fallback string;
update the return expression that references the error variable accordingly.
---
Nitpick comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/boolean.js`:
- Around line 36-47: Wrap the checkbox input inside the label to improve
accessibility and click area: move the <input> element to be a child of the
<label> that uses the label variable (currently using htmlFor={fieldId}),
keeping the existing props (id/name={fieldId}, checked={isChecked},
onChange={handleChange}, className="form-check-input", required={required})
intact and remove the separate htmlFor association if redundant so clicking the
label text toggles the checkbox.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/form.js`:
- Line 176: Replace the string concatenation used to build React keys and
inputName values with template literals for clarity and consistency: update
occurrences such as the ReadOnly render (key currently built as idx +
collectorName) and the other collector cases that build keys/inputName (e.g.,
uses of idx + 'err' or idx + collectorName) to use format like
`${idx}-${collectorName}` or `${idx}-err` so keys/inputName are clear and
consistent across the components (look for the ReadOnly render and the other
collector render sites to apply this change).
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js`:
- Around line 31-34: The select change handler handleChangeDevice should not
call event.preventDefault(); remove that call and simply read the selected value
and call setSelectedDevice(event.target.value) (i.e., update state directly in
handleChangeDevice) so the handler is simplified and avoids an unnecessary
default-prevention call.
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/readonly.js`:
- Around line 14-46: The ReadOnly component currently returns nothing for
unknown collector types; update the end of the if/else chain in ReadOnly to
handle a default case that both logs the unexpected collector.type (use
console.warn(`ReadOnly: unknown collector type: ${collectorType}`, collector) or
similar) and returns a safe fallback UI (e.g., null or a small placeholder like
a <p> stating the type is unsupported) so the component never implicitly returns
undefined; reference the ReadOnly function and the collectorType variable when
adding this default handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0dd2136d-8b86-442a-8791-52da1dd2af4f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
javascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/components/davinci-client/boolean.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/error.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/form.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/object-value.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/qr-code.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/readonly.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/single-select.jsjavascript/reactjs-todo-davinci/client/components/utilities/rich-content.jsjavascript/reactjs-todo-davinci/client/styles/README.mdjavascript/reactjs-todo-davinci/package.json
Comment on lines
+83
to
+86
| updater({ | ||
| phoneNumber: updatedPhone, | ||
| countryCode: collector.input.value?.countryCode, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify countryCode usage and initialization patterns
# Check for countryCode initialization patterns
echo "=== CountryCode initialization patterns ==="
rg -n -A3 -B3 'countryCode\s*[:=]' --type=js --type=jsx
# Check for countryCode validation or default handling
echo -e "\n=== CountryCode validation/default handling ==="
rg -n 'countryCode.*default|countryCode.*\?\?|countryCode.*undefined' --type=js --type=jsxRepository: ForgeRock/sdk-sample-apps
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced file around the updater call
echo "=== object-value.js context (lines 60-170) ==="
sed -n '60,170p' javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js | cat -n
# Locate updater definition/call chain nearby
echo
echo "=== Find updater definition/calls in object-value.js ==="
rg -n "function updater|const updater|updater\s*=" javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js || true
rg -n "updater\(" javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js
# Repo-wide search for countryCode usage and default/validation patterns.
# (Avoid invalid --type=jsx; just search file extensions explicitly.)
echo
echo "=== Repo-wide countryCode occurrences (limited context) ==="
rg -n --hidden \
--glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' \
'countryCode' \
javascript/reactjs-todo-davinci/client/components/davinci-client javascript/reactjs-todo-davinci/client || true
echo
echo "=== countryCode default/validation patterns ==="
rg -n --hidden \
--glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' \
'(countryCode.*default|default.*countryCode|countryCode.*\?\?|countryCode.*undefined|typeof\s+countryCode|countryCode\s*==\s*null|countryCode\s*===\s*null|if\s*\(.*countryCode|switch\s*\(.*countryCode)' \
javascript/reactjs-todo-davinci/client/components/davinci-client javascript/reactjs-todo-davinci/client || trueRepository: ForgeRock/sdk-sample-apps
Length of output: 5089
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js"
echo "=== object-value.js (header + updater prop/usage) ==="
sed -n '1,120p' "$FILE" | cat -n
echo
echo "=== object-value.js: all updater references ==="
rg -n "updater" "$FILE" || true
echo
echo "=== Repo-wide: all countryCode references (context) ==="
rg -n --hidden --glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' --glob='*.json' \
'countryCode' . | head -n 200
echo
echo "=== Repo-wide: validation/default handling for countryCode (context) ==="
rg -n --hidden --glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' \
'(default.*countryCode|countryCode.*default|countryCode.*\?\?|countryCode.*undefined|countryCode\s*==\s*null|countryCode\s*===\s*null|typeof\s+countryCode\s*===\s*["'"'"']undefined["'"'"']|if\s*\(.*countryCode|switch\s*\(.*countryCode|validate.*countryCode)' . | head -n 200
echo
echo "=== Repo-wide: payload building around phoneNumber/countryCode ==="
rg -n --hidden --glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' \
'(phoneNumber.*countryCode|countryCode.*phoneNumber|{[^}]*phoneNumber[^}]*countryCode|{[^}]*countryCode[^}]*phoneNumber)' . | head -n 200Repository: ForgeRock/sdk-sample-apps
Length of output: 6045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js"
echo "=== object-value.js (lines 120-220) ==="
sed -n '120,220p' "$FILE" | cat -n
echo
echo "=== Where ObjectValueComponent is used (and how updater is provided) ==="
rg -n "ObjectValueComponent" javascript/reactjs-todo-davinci/client/components -S --glob='*.js' --glob='*.jsx' || true
echo
echo "=== Find PhoneNumberCollector / PhoneNumberExtensionCollector handling ==="
rg -n "PhoneNumberCollector|PhoneNumberExtensionCollector" javascript/reactjs-todo-davinci/client -S --glob='*.js' --glob='*.jsx' || true
echo
echo "=== Trace updater implementation/expectations (look for updater prop names in parent components) ==="
rg -n "updater\s*=" javascript/reactjs-todo-davinci/client -S --glob='*.js' --glob='*.jsx' || true
echo
echo "=== Search for phoneNumber/countryCode payload handling around updater calls ==="
rg -n "phoneNumber.*countryCode|countryCode.*phoneNumber" javascript/reactjs-todo-davinci/client -S --glob='*.js' --glob='*.jsx' || trueRepository: ForgeRock/sdk-sample-apps
Length of output: 3456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="javascript/reactjs-todo-davinci/client/components/davinci-client/form.js"
echo "=== form.js (lines 1-260) ==="
sed -n '1,260p' "$FILE" | cat -nRepository: ForgeRock/sdk-sample-apps
Length of output: 10934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate davinci.hook.js ==="
fd -a -t f "davinci.hook.js" javascript/reactjs-todo-davinci/client/components javascript/reactjs-todo-davinci/client || true
HOOK="javascript/reactjs-todo-davinci/client/components/davinci-client/hooks/davinci.hook.js"
if [ -f "$HOOK" ]; then
echo
echo "=== davinci.hook.js (lines 1-260) ==="
sed -n '1,260p' "$HOOK" | cat -n
echo
echo "=== davinci.hook.js: search for updater implementation ==="
rg -n "function updater|updater\s*=\s*|const updater|updater\(" "$HOOK" || true
else
echo "Missing expected hook file: $HOOK"
fi
echo
echo "=== Repo-wide: countryCode occurrences (limited context) ==="
rg -n --hidden \
--glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' --glob='*.json' \
'countryCode' . | head -n 200
echo
echo "=== Repo-wide: phoneNumber/countryCode payload patterns ==="
rg -n --hidden \
--glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' --glob='*.json' \
'(phoneNumber.*countryCode|countryCode.*phoneNumber|{[^}]*phoneNumber[^}]*countryCode|{[^}]*countryCode[^}]*phoneNumber)' . | head -n 200Repository: ForgeRock/sdk-sample-apps
Length of output: 8429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Repo-wide: all occurrences of 'countryCode' ==="
rg -n --hidden --glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' --glob='*.json' --glob='*.md' --glob='*.yml' --glob='*.yaml' \
'countryCode' . || true
echo
echo "=== Repo-wide: PhoneNumberCollector/PhoneNumberExtensionCollector examples ==="
rg -n --hidden --glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' --glob='*.json' --glob='*.md' --glob='*.yml' --glob='*.yaml' \
'(PhoneNumberCollector|PhoneNumberExtensionCollector)' . || true
echo
echo "=== Repo-wide: any defaulting/normalization patterns for country code ==="
rg -n --hidden --glob='*.js' --glob='*.jsx' --glob='*.ts' --glob='*.tsx' --glob='*.json' --glob='*.md' \
'(default.*country|country.*default|country.*code.*default|countryCode.*(default|fallback|||=)|countryCode\s*\?\?|\bnull\b.*countryCode|countryCode\s*===\s*null|countryCode\s*===\s*undefined)' . || trueRepository: ForgeRock/sdk-sample-apps
Length of output: 1857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
README="javascript/reactjs-todo-davinci/README.md"
if [ -f "$README" ]; then
sed -n '1,200p' "$README" | cat -n
else
echo "README not found at $README"
fiRepository: ForgeRock/sdk-sample-apps
Length of output: 6420
Guard against undefined countryCode when updating phone collectors.
ObjectValueComponent sends countryCode: collector.input.value?.countryCode in the updater(...) payload for PhoneNumberCollector and PhoneNumberExtensionCollector (83-86, 114-118, 136-140). There is no other countryCode defaulting/validation in this codebase, so countryCode: undefined can produce an inconsistent update payload for davinciClient.update(...). Either derive a default before calling updater, or conditionally omit countryCode from the payload when it’s not present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js`
around lines 83 - 86, ObjectValueComponent is passing countryCode:
collector.input.value?.countryCode into updater for PhoneNumberCollector and
PhoneNumberExtensionCollector (calls to updater in ObjectValueComponent), which
can send undefined into davinciClient.update; fix by ensuring countryCode is
defined before calling updater — either compute a sane default (e.g. derive from
collector.metadata or fallback constant) and pass that instead, or build the
payload conditionally so you omit countryCode when
collector.input.value?.countryCode is falsy; update the updater invocation sites
in ObjectValueComponent (the PhoneNumberCollector/PhoneNumberExtensionCollector
branches) so the payload never contains countryCode: undefined before calling
davinciClient.update.
Comment on lines
+12
to
+35
| export function interpolateRichContent(richContent) { | ||
| /** | ||
| * Split the content on `{{key}}` placeholders. Because the regex has a | ||
| * capture group, `split` interleaves results: even indices are literal | ||
| * text, odd indices are replacement keys. | ||
| * | ||
| * Sample rich content: | ||
| * { | ||
| * "content": "A translatable rich text link to {{link1}}.", | ||
| * "replacements": [ | ||
| * { | ||
| * "key": "link1", | ||
| * "type": "link", | ||
| * "value": "Ping Identity", | ||
| * "href": "https://www.pingidentity.com", | ||
| * "target": "_self" | ||
| * } | ||
| * ] | ||
| * } | ||
| */ | ||
| const segments = richContent.content.split(/\{\{(\w+)\}\}/); | ||
|
|
||
| // Index replacements by key so each placeholder can be looked up in O(1). | ||
| const replacementMap = new Map(richContent.replacements.map((r) => [r.key, r])); |
There was a problem hiding this comment.
Add defensive null checks for richContent structure.
The function directly accesses richContent.content (line 32) and richContent.replacements (line 35) without validating that these properties exist. If the caller passes a malformed or partial richContent object, this will throw a runtime error.
🛡️ Proposed fix to add validation
export function interpolateRichContent(richContent) {
+ if (!richContent?.content || !richContent?.replacements) {
+ return null;
+ }
+
/**
* Split the content on `{{key}}` placeholders. Because the regex has a📝 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.
Suggested change
| export function interpolateRichContent(richContent) { | |
| /** | |
| * Split the content on `{{key}}` placeholders. Because the regex has a | |
| * capture group, `split` interleaves results: even indices are literal | |
| * text, odd indices are replacement keys. | |
| * | |
| * Sample rich content: | |
| * { | |
| * "content": "A translatable rich text link to {{link1}}.", | |
| * "replacements": [ | |
| * { | |
| * "key": "link1", | |
| * "type": "link", | |
| * "value": "Ping Identity", | |
| * "href": "https://www.pingidentity.com", | |
| * "target": "_self" | |
| * } | |
| * ] | |
| * } | |
| */ | |
| const segments = richContent.content.split(/\{\{(\w+)\}\}/); | |
| // Index replacements by key so each placeholder can be looked up in O(1). | |
| const replacementMap = new Map(richContent.replacements.map((r) => [r.key, r])); | |
| export function interpolateRichContent(richContent) { | |
| if (!richContent?.content || !richContent?.replacements) { | |
| return null; | |
| } | |
| /** | |
| * Split the content on `{{key}}` placeholders. Because the regex has a | |
| * capture group, `split` interleaves results: even indices are literal | |
| * text, odd indices are replacement keys. | |
| * | |
| * Sample rich content: | |
| * { | |
| * "content": "A translatable rich text link to {{link1}}.", | |
| * "replacements": [ | |
| * { | |
| * "key": "link1", | |
| * "type": "link", | |
| * "value": "Ping Identity", | |
| * "href": "https://www.pingidentity.com", | |
| * "target": "_self" | |
| * } | |
| * ] | |
| * } | |
| */ | |
| const segments = richContent.content.split(/\{\{(\w+)\}\}/); | |
| // Index replacements by key so each placeholder can be looked up in O(1). | |
| const replacementMap = new Map(richContent.replacements.map((r) => [r.key, r])); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@javascript/reactjs-todo-davinci/client/components/utilities/rich-content.js`
around lines 12 - 35, The function interpolateRichContent currently assumes
richContent.content and richContent.replacements exist; add defensive validation
at the top of interpolateRichContent to guard against malformed input by
checking that richContent is an object, that richContent.content is a string
(fallback to empty string) and richContent.replacements is an array (fallback to
[]), then compute segments = richContent.content.split(...) and replacementMap =
new Map(richContent.replacements.map(...)) using those validated/fallback values
so the rest of the function can run without throwing on null/undefined.
ancheetah
commented
Jun 8, 2026
| } | ||
|
|
||
| return <p>{interpolateRichContent(richContent)}</p>; | ||
| } else if (collectorType === 'AgreementCollector') { |
Contributor
Author
There was a problem hiding this comment.
I placed the AgreementCollector here in readonly.js but since it goes along with single checkbox it could possibly be combined with boolean.ts in a separate file. Thoughts?
ryanbas21
requested changes
Jun 8, 2026
| return <ReadOnly key={idx + collectorName} collector={collector} />; | ||
| case 'ValidatedBooleanCollector': | ||
| return ( | ||
| <BooleanComponent |
Contributor
There was a problem hiding this comment.
We aren't passing in the collectorName here but we use the prop in boolean.js
Contributor
There was a problem hiding this comment.
@ancheetah I think this is the only outstanding thing from my review.
Contributor
There was a problem hiding this comment.
nevermind, I see we removed the prop
| } = output; | ||
|
|
||
| if (!componentEnabled) { | ||
| return; |
Contributor
There was a problem hiding this comment.
I think it has to be return null; here right? undefined can break react?
| const fieldId = collector.output.key || 'checkbox-field'; | ||
|
|
||
| const { richContent } = collector.output; | ||
| const label = richContent?.replacements.length |
Contributor
There was a problem hiding this comment.
maybe null chain replacements also since i think its possible (at least in davinci) the field could not be there
| } else if (collectorType === 'RichTextCollector') { | ||
| const { richContent } = output; | ||
|
|
||
| if (!richContent.replacements.length) { |
Contributor
There was a problem hiding this comment.
should we null check this also?
ancheetah
force-pushed
the
SDKS-5052-add-collectors
branch
2 times, most recently
from
ef0ab5c to
9507c76
Compare
Contributor
Author
|
Added polling collector support and addressed PR comments. Please re-review. |
ryanbas21
previously approved these changes
Jun 11, 2026
cerebrl
reviewed
Jun 11, 2026
cerebrl
left a comment
cerebrl
left a comment
Contributor
There was a problem hiding this comment.
A few comments after an initial scan.
Contributor
There was a problem hiding this comment.
This component is getting quite complex. Should we split this into different components? To keep components simple for testing purposes, I'm okay if splitting this up even if it leads to some duplication of code.
Contributor
Author
There was a problem hiding this comment.
I had this thought as well and I'd like to split it up. I don't think object-value best describes this component anyways because one of the device collectors I believe accepts a string, not object.
Comment on lines
+224
to
+225
| cacheKey={node?.cache?.key} | ||
| key={collectorName} |
Contributor
There was a problem hiding this comment.
Does the component use key and cacheKey? If not, can we remove them?
Contributor
Author
There was a problem hiding this comment.
key is on every component and not necessarily used in the component. It's just for assigning a unique key to components so React doesn't complain. Ideally each of these keys should probably also have the idx appended to them to truly make them unique.
cacheKey is needed off the node to trigger a re-render of the polling component in continue mode. Otherwise the collector component will look exactly the same and not trigger a re-poll.
Contributor
Author
|
Split object value component into device and phone number components in last commit |
ancheetah
force-pushed
the
SDKS-5052-add-collectors
branch
from
0fbebf9 to
ec9bb05
Compare
chore(reactjs-todo-davinci): update boolean and read-only components
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/boolean.js`:
- Around line 24-26: The code accesses collector.input.validation without
verifying that collector.input exists first, which can cause a runtime error
when collector.input is absent. Add optional chaining at the input level before
accessing validation, and provide a default value of false to ensure the
required constant has a safe fallback when the input chain is broken. Update the
assignment to the required constant to use optional chaining syntax with a
logical OR to default to false.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/device.js`:
- Around line 32-39: The label and select element in the DeviceComponent both
hard-code the id "device-select", which creates duplicate DOM IDs when multiple
instances are rendered and breaks accessibility. Generate a unique id per
component instance using React's useId() hook, then replace the hard-coded
"device-select" string in both the label's htmlFor attribute and the select
element's id attribute with this generated unique identifier.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/polling.js`:
- Around line 30-57: The handlePoll function calls async operations (pollStatus
on line 35 and submitForm on line 55) without try/catch/finally protection,
which means if these operations throw an error, setIsPolling(false) will never
execute and the isPolling state will remain stuck as true. Wrap the entire async
logic in the handlePoll function with a try/catch/finally block, moving all the
existing logic into the try block, handling any thrown errors in the catch block
(similar to how the error object properties are already being handled), and
ensuring setIsPolling(false) is called in the finally block so it always
executes regardless of success or failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab92e254-e5bc-4ee6-91ba-7fa0e32dbb98
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
javascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/components/davinci-client/boolean.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/device.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/error.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/form.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/hooks/davinci.hook.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/object-value.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/phone-number.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/polling.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/qr-code.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/readonly.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/single-select.jsjavascript/reactjs-todo-davinci/client/components/utilities/rich-content.jsjavascript/reactjs-todo-davinci/client/styles/README.mdjavascript/reactjs-todo-davinci/package.json
💤 Files with no reviewable changes (1)
- javascript/reactjs-todo-davinci/client/components/davinci-client/object-value.js
✅ Files skipped from review due to trivial changes (3)
- javascript/reactjs-todo-davinci/client/styles/README.md
- javascript/reactjs-todo-davinci/client/components/davinci-client/single-select.js
- javascript/reactjs-todo-davinci/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- javascript/reactjs-todo-davinci/client/components/davinci-client/qr-code.js
- javascript/reactjs-todo-davinci/client/components/utilities/rich-content.js
- javascript/reactjs-todo-davinci/client/components/davinci-client/error.js
- javascript/reactjs-todo-davinci/client/components/davinci-client/form.js
Comment on lines
+24
to
+26
| const required = collector.input.validation?.some( | ||
| (validation) => validation.type === 'required' && validation.rule === true, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard collector.input before accessing validation.
This can throw at runtime when collector.input is absent. Add optional chaining at input level and default to false to keep rendering resilient.
Proposed fix
- const required = collector.input.validation?.some(
+ const required = collector.input?.validation?.some(
(validation) => validation.type === 'required' && validation.rule === true,
- );
+ ) ?? false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/boolean.js`
around lines 24 - 26, The code accesses collector.input.validation without
verifying that collector.input exists first, which can cause a runtime error
when collector.input is absent. Add optional chaining at the input level before
accessing validation, and provide a default value of false to ensure the
required constant has a safe fallback when the input chain is broken. Update the
assignment to the required constant to use optional chaining syntax with a
logical OR to default to false.
Comment on lines
+32
to
+39
| htmlFor="device-select" | ||
| className={`form-label cstm_subhead-text mb-4 fw-bold text-center ${theme.textMutedClass}`} | ||
| > | ||
| {collector.output.label || 'select an option'} | ||
| </label> | ||
| <select | ||
| id="device-select" | ||
| className="form-select form-select-lg w-100" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a unique id per DeviceComponent instance.
Line 32 and Line 38 hard-code device-select; multiple rendered collectors will duplicate DOM ids and break label targeting/accessibility. Generate an instance-specific id (e.g., from collector metadata or useId()).
Suggested fix
import React, { useEffect, useContext, useState } from 'react';
+import { useId } from 'react';
@@
export default function DeviceComponent({ collector, updater }) {
+ const deviceSelectId = useId();
@@
<label
- htmlFor="device-select"
+ htmlFor={deviceSelectId}
@@
<select
- id="device-select"
+ id={deviceSelectId}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/device.js`
around lines 32 - 39, The label and select element in the DeviceComponent both
hard-code the id "device-select", which creates duplicate DOM IDs when multiple
instances are rendered and breaks accessibility. Generate a unique id per
component instance using React's useId() hook, then replace the hard-coded
"device-select" string in both the label's htmlFor attribute and the select
element's id attribute with this generated unique identifier.
Comment on lines
+30
to
+57
| async function handlePoll() { | ||
| setIsPolling(true); | ||
| setError(null); | ||
| setResult(null); | ||
|
|
||
| const status = await pollStatus(); | ||
|
|
||
| if (typeof status !== 'string' && status && 'error' in status) { | ||
| const message = status.error?.message || 'Polling error'; | ||
| console.error(message); | ||
| setError(message); | ||
| setIsPolling(false); | ||
| return; | ||
| } | ||
|
|
||
| const updateResult = updater(status); | ||
| if (updateResult && 'error' in updateResult) { | ||
| const message = updateResult.error?.message || 'Polling error'; | ||
| console.error(message); | ||
| setError(message); | ||
| setIsPolling(false); | ||
| return; | ||
| } | ||
|
|
||
| setResult(status); | ||
| await submitForm(); | ||
| setIsPolling(false); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle thrown async failures in the polling flow.
Line 35/55 await async operations without a try/catch/finally. If pollStatus(), updater(status), or submitForm() throws/rejects, this leaves an unhandled rejection and can keep isPolling stuck true.
Proposed fix
useEffect(() => {
async function handlePoll() {
- setIsPolling(true);
- setError(null);
- setResult(null);
-
- const status = await pollStatus();
-
- if (typeof status !== 'string' && status && 'error' in status) {
- const message = status.error?.message || 'Polling error';
- console.error(message);
- setError(message);
- setIsPolling(false);
- return;
- }
-
- const updateResult = updater(status);
- if (updateResult && 'error' in updateResult) {
- const message = updateResult.error?.message || 'Polling error';
- console.error(message);
- setError(message);
- setIsPolling(false);
- return;
- }
-
- setResult(status);
- await submitForm();
- setIsPolling(false);
+ setIsPolling(true);
+ setError(null);
+ setResult(null);
+ try {
+ const status = await pollStatus();
+
+ if (typeof status !== 'string' && status && 'error' in status) {
+ const message = status.error?.message || 'Polling error';
+ console.error(message);
+ setError(message);
+ return;
+ }
+
+ const updateResult = updater(status);
+ if (updateResult && 'error' in updateResult) {
+ const message = updateResult.error?.message || 'Polling error';
+ console.error(message);
+ setError(message);
+ return;
+ }
+
+ setResult(status);
+ await submitForm();
+ } catch (e) {
+ const message = e?.message || 'Polling error';
+ console.error(message);
+ setError(message);
+ } finally {
+ setIsPolling(false);
+ }
}🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 39-39: Avoid using the initial state variable in setState
Context: setError(message)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 48-48: Avoid using the initial state variable in setState
Context: setError(message)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 53-53: Avoid using the initial state variable in setState
Context: setResult(status)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/polling.js`
around lines 30 - 57, The handlePoll function calls async operations (pollStatus
on line 35 and submitForm on line 55) without try/catch/finally protection,
which means if these operations throw an error, setIsPolling(false) will never
execute and the isPolling state will remain stuck as true. Wrap the entire async
logic in the handlePoll function with a try/catch/finally block, moving all the
existing logic into the try block, handling any thrown errors in the catch block
(similar to how the error object properties are already being handled), and
ensuring setIsPolling(false) is called in the finally block so it always
executes regardless of success or failure.
Source: Learnings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://pingidentity.atlassian.net/browse/SDKS-5052
DO NOT MERGE: Pending 2.1 release. Dependencies need to be updated to latest.
Added support for the following collectors:
Note: ValidatedPasswordCollector will be addressed in a separate PR (#116)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores