refactor(davinci-client): remove throws in node reducer updater - #753
Conversation
🦋 Changeset detectedLatest commit: e1b8adb The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates DaVinci collector types, centralizes collector validation, changes invalid reducer updates to no-ops, updates tests, and adds a patch-release changeset. ChangesCollector update refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientStore
participant ValidationUtils
participant NodeReducer
Client->>ClientStore: submit collector update
ClientStore->>ValidationUtils: validate category and value
ValidationUtils-->>ClientStore: narrowed value or error
ClientStore->>NodeReducer: dispatch valid update
NodeReducer-->>ClientStore: updated state or unchanged state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
0eecf39 to
fc3136f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/davinci-client/src/lib/node.reducer.ts (1)
226-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist
getUpdateValueto module scope.The reducer re-creates this function on every
node/updateaction. It closes over nothing from the case body. Declare it once at module level next to the reducer.♻️ Proposed refactor
Add above
nodeCollectorReducer:/** * Validates `value` against `collector` and returns the narrowed value, or * `null` if validation failed. Discards the validation error: the * reducer only needs to know whether to no-op, not why. */ function getUpdateValue<T extends UpdatableCollectors>( collector: T, value: CollectorValueTypes, ): CollectorValueType<T> | null { const result = resolveCollectorUpdateValue(collector, value); return Either.isLeft(result) ? null : result.right; }Then remove the inner declaration:
- /** - * Validates `value` against `collector` and returns the narrowed value, or - * `null` if validation failed. Discards the validation error: the - * reducer only needs to know whether to no-op, not why. - */ - function getUpdateValue<T extends UpdatableCollectors>( - collector: T, - value: CollectorValueTypes, - ): CollectorValueType<T> | null { - const result = resolveCollectorUpdateValue(collector, value); - return Either.isLeft(result) ? null : result.right; - } -🤖 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 `@packages/davinci-client/src/lib/node.reducer.ts` around lines 226 - 237, Move the standalone getUpdateValue function from the node/update case body to module scope alongside nodeCollectorReducer, preserving its existing generic signature, validation logic, and documentation. Remove the inner declaration so the reducer reuses the module-level helper.packages/davinci-client/src/lib/node.reducer.test.ts (1)
357-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd no-op coverage for invalid values on other updatable categories.
These tests cover the category gate and the MetadataCollector value check. The new
resolveCollectorUpdateValuefailure paths for the other categories are untested in this reducer. Add cases such as a boolean value sent to aTextCollectorand an object value sent to aMultiSelectCollector, and assert the state stays unchanged.Also applies to: 388-429, 431-456, 2396-2414
🤖 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 `@packages/davinci-client/src/lib/node.reducer.test.ts` around lines 357 - 386, Extend the reducer tests around nodeCollectorReducer with no-op cases for invalid values across the remaining updatable collector categories, including a boolean value for a TextCollector and an object value for a MultiSelectCollector. Reuse the existing collector fixtures and action structure where appropriate, and assert each invalid update returns state unchanged to cover the resolveCollectorUpdateValue failure paths.packages/davinci-client/src/lib/client.store.utils.ts (2)
113-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMove the logging effect out of this
*.utils.tsfile.
handleUpdateValidateErrorinvokescb(message)at line 118, so calling it writes a log entry. That makes the utility effectful. Keep*.utils.tspure, and place the single logging effect in an*.effects.tsmodule, or let each caller log before it returns the error response.As per coding guidelines: "Keep
*.utils.tsfiles pure and stateless; never put effectful logic in them. Place single isolated effects in*.effects.tsor multi-step workflows in*.micros.ts".🤖 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 `@packages/davinci-client/src/lib/client.store.utils.ts` around lines 113 - 128, Remove the cb(message) side effect from handleUpdateValidateError so the utility only constructs and returns the InternalErrorResponse. Move the single logging call into an appropriate effects module or into each caller immediately before returning the validation error, preserving one log entry per error.Source: Coding guidelines
169-259: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a fallback for unmatched collectors.
Match.exhaustivethrows when runtime input bypasses TypeScript's exhaustiveness checks. Although current callers useisValidCollectorCategory, the exportedresolveCollectorUpdateValuecan receive an unmatched collector. Returnerr('Collector does not fall into a category that can be updated')withMatch.orElseinstead.🤖 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 `@packages/davinci-client/src/lib/client.store.utils.ts` around lines 169 - 259, Update resolveCollectorUpdateValue’s Match chain by replacing Match.exhaustive with Match.orElse that returns err('Collector does not fall into a category that can be updated') for unmatched runtime collectors, while preserving all existing collector-specific validation branches.packages/davinci-client/src/lib/client.types.test-d.ts (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
toMatchTypeOfwithtoExtend. The pinned Vitest version resolvesexpect-type1.2.2, which deprecatestoMatchTypeOf.toExtend<CollectorValueTypes>()preserves this assignability check; the assertion remains weaker than the previous exact-equality check.🤖 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 `@packages/davinci-client/src/lib/client.types.test-d.ts` at line 182, In the updater type assertion, replace the deprecated toMatchTypeOf call with toExtend while retaining CollectorValueTypes as the generic type, preserving the existing assignability check.
🤖 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 `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 259-271: Update the MultiValueCollector branch in the reducer to
honor action.payload.index when applying scalar updates, inserting or replacing
the value at the specified position as required by the Updater contract while
preserving full-array replacement behavior. If indexed updates are not
supported, instead remove index consistently from the Updater signature and
action payload.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/client.store.utils.ts`:
- Around line 113-128: Remove the cb(message) side effect from
handleUpdateValidateError so the utility only constructs and returns the
InternalErrorResponse. Move the single logging call into an appropriate effects
module or into each caller immediately before returning the validation error,
preserving one log entry per error.
- Around line 169-259: Update resolveCollectorUpdateValue’s Match chain by
replacing Match.exhaustive with Match.orElse that returns err('Collector does
not fall into a category that can be updated') for unmatched runtime collectors,
while preserving all existing collector-specific validation branches.
In `@packages/davinci-client/src/lib/client.types.test-d.ts`:
- Line 182: In the updater type assertion, replace the deprecated toMatchTypeOf
call with toExtend while retaining CollectorValueTypes as the generic type,
preserving the existing assignability check.
In `@packages/davinci-client/src/lib/node.reducer.test.ts`:
- Around line 357-386: Extend the reducer tests around nodeCollectorReducer with
no-op cases for invalid values across the remaining updatable collector
categories, including a boolean value for a TextCollector and an object value
for a MultiSelectCollector. Reuse the existing collector fixtures and action
structure where appropriate, and assert each invalid update returns state
unchanged to cover the resolveCollectorUpdateValue failure paths.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 226-237: Move the standalone getUpdateValue function from the
node/update case body to module scope alongside nodeCollectorReducer, preserving
its existing generic signature, validation logic, and documentation. Remove the
inner declaration so the reducer reuses the module-level helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d0697d4-1d15-4665-b68e-0db06f1c4295
📒 Files selected for processing (10)
.changeset/five-badgers-rule.mdpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.store.utils.tspackages/davinci-client/src/lib/client.types.test-d.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.ts
| if (collector.category === 'MultiValueCollector') { | ||
| if (typeof action.payload.value !== 'string' && !Array.isArray(action.payload.value)) { | ||
| throw new Error('MultiValueCollector does not accept an object'); | ||
| const value = getUpdateValue(collector, action.payload.value); | ||
| if (value === null) { | ||
| return; | ||
| } | ||
| if (Array.isArray(action.payload.value)) { | ||
| collector.input.value = [...action.payload.value]; | ||
|
|
||
| if (Array.isArray(value)) { | ||
| collector.input.value = [...value]; | ||
| } else { | ||
| collector.input.value.push(action.payload.value); | ||
| collector.input.value.push(value); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every producer and consumer of the update payload `index` field.
set -euo pipefail
fd -e ts -e tsx . packages/davinci-client/src | xargs rg -n -C4 '\bindex\b' -g '!*.test.ts' \
| rg -n -C4 'payload|Updater|update\(' || true
# Show the full node/update reducer case for confirmation.
ast-grep run --pattern 'addCase(updateCollectorValues, ($$$) => { $$$ })' --lang typescript packages/davinci-client/src/lib/node.reducer.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 4726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- updater type and implementation ---'
sed -n '110,140p' packages/davinci-client/src/lib/client.types.ts
sed -n '320,375p' packages/davinci-client/src/lib/client.store.ts
printf '%s\n' '--- reducer action and relevant branches ---'
sed -n '40,85p' packages/davinci-client/src/lib/node.reducer.ts
sed -n '225,280p' packages/davinci-client/src/lib/node.reducer.ts
printf '%s\n' '--- update action declarations and all index references ---'
rg -n -C3 '\bindex\b|updateCollectorValues|nodeSlice\.actions\.update' packages/davinci-client/src/lib \
-g '*.ts' -g '!*.test.ts' -g '!*.test-d.ts'
printf '%s\n' '--- multi-value collector shape and update-value helper ---'
rg -n -C5 'MultiValueCollector|getUpdateValue|resolveCollectorUpdateValue' packages/davinci-client/src/lib \
-g '*.ts' -g '!*.test.ts' -g '!*.test-d.ts'Repository: ForgeRock/ping-javascript-sdk
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/davinci-client/src/lib/node.reducer.ts")
text = p.read_text()
start = text.index("if (collector.category === 'MultiValueCollector')")
end = text.index("\n if (collector.type === 'DeviceAuthenticationCollector')", start)
branch = text[start:end]
print("--- MultiValueCollector reducer branch ---")
print(branch)
print("--- payload.index references in branch ---")
print(branch.count("action.payload.index"))
PY
printf '%s\n' '--- tests and documentation for updater index semantics ---'
rg -n -C4 'index|MultiValueCollector|update\(' packages/davinci-client/src/lib \
-g '*.test.ts' -g '*.test-d.ts' -g '*.md' \
| head -n 240
printf '%s\n' '--- multi-value factory and input initialization ---'
sed -n '700,760p' packages/davinci-client/src/lib/collector.utils.ts
rg -n -C4 'MultiValueCollector' packages/davinci-client/src/lib/collector.types.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 13675
Handle action.payload.index for MultiValueCollector updates. The public Updater contract exposes index, but the reducer ignores it and always appends scalar values. If index is obsolete, remove it from the Updater signature and action payload.
🤖 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 `@packages/davinci-client/src/lib/node.reducer.ts` around lines 259 - 271,
Update the MultiValueCollector branch in the reducer to honor
action.payload.index when applying scalar updates, inserting or replacing the
value at the specified position as required by the Updater contract while
preserving full-array replacement behavior. If indexed updates are not
supported, instead remove index consistently from the Updater signature and
action payload.
| ]) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If we do this check at the store.ts level, do we need to do it here too?
There was a problem hiding this comment.
isValidCollectorCategory is needed here to narrow the collector type down to UpdatableCollectors. This is required in order to use the resolveCollectorUpdateValue helper. Since we have to find the collector off of state again in the reducer via it's id, the collector initially can be any collector type.
| collector.input.value = [...value]; | ||
| } else { | ||
| collector.input.value.push(action.payload.value); | ||
| collector.input.value.push(value); |
There was a problem hiding this comment.
Is this considered safe because it uses Immer? I can't recall, i know immer does the mutation but I would have to look up the push.
We could just use a concat if we needed instead.
There was a problem hiding this comment.
Is this considered safe because it uses Immer?
Yes, that is correct. You can see it explained here: https://redux-toolkit.js.org/usage/immer-reducers#redux-toolkit-and-immer. Though, I have no issue with explicitly writing it in with immutable grammar.
| return err('Value argument cannot be undefined'); | ||
| } | ||
|
|
||
| return Match.value<UpdatableCollectors>(collector).pipe( |
There was a problem hiding this comment.
Honestly i really like this.
The only piece that irks me slightly is that my brain tells me we are handling the errors in the wrong place, however it's not a big deal. I think this makes sense.
I also don't mind the ok and err notation here since Result<T, E> = Ok<T> | Err<E> will be the basic type going foward in effect, however in this context it is mixing two different vocabularies for the same underlying structure.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I like this, but I do worry about mulling in more of the Effect library due to Match. If we can get some numbers around this for context, that would be great. I very much do like the use of Match though :)
| // Every branch below is a no-op rather than a throw: `update()` in | ||
| // client.store.ts already validates the id exists and the category is | ||
| // updatable before dispatching, so reaching an unmatched case here means | ||
| // the action was dispatched directly, bypassing that gate. |
There was a problem hiding this comment.
Since these reducers are not accessible via our public API, do we need these checks? I like writing strong, defensive code for our public APIs, but for private methods we call internally, I'm not sure of the value.
There was a problem hiding this comment.
Are you asking about the if (value === null) return checks? The reason why I'm explicitly calling getUpdateValue() in each branch is to narrow the value type to one that is accepted by the collector. If we dont return after getting a null value (i.e. an error) then the value type could potentially include null which doesn't play well when you try to assign it to the collector.
That said, we should never really get null in the reducer unless for some reason someone decided to call this reducer directly without going through the davinciClient.update() first to check for errors. Hope that answers your question.
There was a problem hiding this comment.
That said, we should never really get null in the reducer unless for some reason someone decided to call this reducer directly without going through the davinciClient.update() first to check for errors.
Yes, this is exactly my point. These reducers are design specifically to be called from the store's API. They are not designed to be generic or public utility functions.
There was a problem hiding this comment.
Right, ok. One way I could see around this is rewriting getUpdateValue to take a callback that does the mutation. And is a no-op if there is a Left result (error).
function updateCollector<T extends UpdatableCollectors>(
collector: T,
value: CollectorValueTypes,
cb: (resolvedValue: CollectorValueType<T>) => void,
): void {
const result = resolveCollectorUpdateValue(collector, value);
if (Either.isRight(result)) {
const resolvedValue = result.right;
cb(resolvedValue);
}
}
if (collector.category === 'MultiValueCollector') {
updateCollector(collector, action.payload.value, (resolvedValue) => {
if (Array.isArray(resolvedValue)) {
collector.input.value = [...resolvedValue];
} else {
collector.input.value.push(resolvedValue);
}
});
return;
}
| collector.input.value = [...value]; | ||
| } else { | ||
| collector.input.value.push(action.payload.value); | ||
| collector.input.value.push(value); |
There was a problem hiding this comment.
Is this considered safe because it uses Immer?
Yes, that is correct. You can see it explained here: https://redux-toolkit.js.org/usage/immer-reducers#redux-toolkit-and-immer. Though, I have no issue with explicitly writing it in with immutable grammar.
fc3136f to
2960502
Compare
ancheetah
changed the title
|
View your CI Pipeline Execution ↗ for commit e1b8adb
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/recognize
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (24.11%) 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 #753 +/- ##
==========================================
+ Coverage 18.07% 24.11% +6.03%
==========================================
Files 155 162 +7
Lines 24398 25787 +1389
Branches 1203 1686 +483
==========================================
+ Hits 4410 6218 +1808
+ Misses 19988 19569 -419
🚀 New features to boost your workflow:
|
|
Deployed 869952f to https://ForgeRock.github.io/ping-javascript-sdk/pr-753/869952f6572c9cac7ae9e813b553aa919c1f7add branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/davinci-client - 59.1 KB (+2.4 KB, +4.3%) 🆕 New Packages🆕 @forgerock/journey-client - 92.6 KB (new) ➖ No Changes➖ @forgerock/sdk-types - 9.1 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
|
@ryanbas21 @cerebrl including For reference here is the 2.1.1 bundle size (56.7 KB). #755 (comment) |
2960502 to
e1b8adb
Compare
|
Simplified the node reducer's collector-update logic by replacing the repeated "resolve or return" pattern with a single |
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5177
Description
What
Refactors collector-value validation in
davinci-clientsonode.reducer.tsno longer throws on invalid updates, moving that validation into a single shared function used by bothclient.store.ts'supdate()method and the reducer.update()also becomes lazy: it now defers collector-state lookup and validation until the returned updater function is invoked, rather than at call time.Changes
resolveCollectorUpdateValue(client.store.utils.ts), anEffect Match-based validator that is the single source of truth for the collector → accepted-value-type mapping. Returns anEitherinstead of throwing.isValidCollectorCategory, a reusable type guard for narrowing aCollectorsunion member by category.node.reducer.ts'snode/updatecase no longer throws; every unmatched/invalid branch is now a documented no-op, sinceupdate()already validates before dispatching. Added explicitreturns after each branch (some previously relied on fallthrough).client.store.ts'supdate()now returns its validation-and-dispatch logic entirely inside the returned updater function (previously it validated eagerly whenupdate(collector)was called, then returned a dispatch-only closure). Failure paths now log vialog.errorbefore returning anInternalErrorResponse, consistent withvalidate().UpdatableCollectorstype (client.types.ts) to replace the repeatedSingleValueCollectors | MultiSelectCollector | ObjectValueCollectors | AutoCollectorsunion used by bothupdate()andUpdater<T>.CollectorValueTypeforMultiSelectCollector/MultiValueCollectorfromstring[]tostring | string[], matching the reducer's existing support for pushing a single string value.CollectorCategorytype alias (node.types.ts).Summary by CodeRabbit
New Features
"continue"status.Bug Fixes