Remove throw reducers#626
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe ChangesCollector update error-as-state refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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)
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 |
ryanbas21
force-pushed
the
remove-throw-reducers
branch
from
fe20632 to
1e2483b
Compare
|
View your CI Pipeline Execution ↗ for commit 4ae3a06
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
remove-throw-reducers
branch
from
1e2483b to
1781cf3
Compare
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
===========================================
- Coverage 70.90% 57.90% -13.01%
===========================================
Files 53 23 -30
Lines 2021 2644 +623
Branches 377 401 +24
===========================================
+ Hits 1433 1531 +98
- Misses 588 1113 +525
🚀 New features to boost your workflow:
|
|
Deployed 68ea625 to https://ForgeRock.github.io/ping-javascript-sdk/pr-626/68ea625235b7d64346d62121345809086cf20c57 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 - 53.5 KB (+0.0 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 |
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I'd like to discuss this further with you. This actually changes the developer facing API. Currently, we explicitly return an error if something is wrong, but now we implicitly store the error in some UnknownCollector.
Can we just pull out the super defensive condition checks out of the reducer and use it prior to calling dispatch? That way, once we call dispatch, we've already filtered out many of these error cases of incompatible actions for collectors.
We still need to consider what happens for errors for compatible collector but incorrect values. I think in this instance, we can write an error to the collector itself. I feel like we could put the error on the input object, since that's where it's most relevant.
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Some minor findings
|
|
||
| if (!collector) { | ||
| throw new Error('No collector found to update'); | ||
| state.push({ |
There was a problem hiding this comment.
If we did not find the collector in state.find, then it means the collector does not exist in server. Pushing an unknown collector into state means that we are pushing invalid collectors that do not exist in davinci.
So we should remove this and instead just do a collector.error with no collector found to update like before.
There was a problem hiding this comment.
I think one of the difficulties here is where do we even put collector.error if we weren't able to find the collector in our state in the first place.
Alternatively, would it be possible to move the collector matching (state.find) outside of the reducer and into client.store right before the update dispatch? The action payload would then accept an additional collector input so that you already have a verified existing collector when you hit the reducer. The Updater function would either return null or an error if the collector doesn't exist.
| collector.error = 'Value argument must be a string'; | ||
| return; | ||
| } | ||
| collector.error = null; |
There was a problem hiding this comment.
Instead of setting collector.null in so many places during reset, we can reset it in one place. This way we don't have to remember to do console.error = null for every new collector.
ryanbas21
force-pushed
the
remove-throw-reducers
branch
from
1781cf3 to
45a79f0
Compare
ryanbas21
force-pushed
the
remove-throw-reducers
branch
from
45a79f0 to
8455027
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
remove-throw-reducers
branch
from
8455027 to
f59478a
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/davinci-client/src/lib/node.reducer.ts (1)
233-238:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winLine 235 still throws, breaking the new error-as-state contract.
Because Line 336 in
packages/davinci-client/src/lib/client.store.tsnow dispatches without try/catch, this throw can escape and crash the caller path instead of writingcollector.error.Suggested fix
if (collector.type === 'ValidatedBooleanCollector') { if (typeof action.payload.value !== 'boolean') { - throw new Error('Value argument must be a boolean'); + collector.error = 'Value argument must be a boolean'; + return; } + collector.error = null; collector.input.value = action.payload.value; return; }🤖 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 233 - 238, In the ValidatedBooleanCollector validation block in the node.reducer.ts file, replace the throw statement with code that stores the error in collector.error instead. When the value is not a boolean, set collector.error to an appropriate error message (such as "Value argument must be a boolean") and then return early, rather than throwing. This aligns with the error-as-state contract where errors should be captured in the collector state instead of being thrown to crash the caller path.
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/client.store.ts (1)
335-338: ⚡ Quick winAdd regression coverage for
update()’s new dispatch-only behavior.Lines 336-337 are now unguarded and always return
null; add a targeted test to assert how invalid updates are surfaced via state (collector.error) so this contract shift stays explicit.🤖 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.ts` around lines 335 - 338, Add a regression test to cover the dispatch-only behavior of the update function in the nodeSlice.actions.update call. The test should verify that when invalid updates are dispatched to the store (through the function that always returns null), the errors are properly surfaced and accessible via the collector.error state property. This ensures the contract shift where the function no longer validates but instead relies on dispatch-based error handling remains explicit and detectable through test coverage.packages/davinci-client/src/lib/node.reducer.test.ts (1)
432-563: ⚡ Quick winAdd tests for remaining runtime edge paths not covered by the new suite.
Please add cases for: (1) non-boolean update on
ValidatedBooleanCollector, and (2)value: nullon phone/FIDO object collectors. These are currently untested and directly protect the no-throw/error-state contract.🤖 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 432 - 563, Add two new test cases to the test suite in node.reducer.test.ts following the same pattern as the existing tests. The first test case should verify that when a non-boolean value (e.g., a string or number) is passed to a ValidatedBooleanCollector via the node/update action, the collector's error field is set with an appropriate error message. The second test case should verify that when value: null is passed to phone or FIDO object collectors via the node/update action, the collector's error field is set with an appropriate error message. Both tests should follow the existing pattern: set up initial state with the collector, create a node/update action with the invalid payload, call nodeCollectorReducer, and assert that the resulting collector has the expected error string set.
🤖 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 307-313: The type check using typeof action.payload.value !==
'object' incorrectly allows null values to pass, since typeof null returns
'object' in JavaScript. This causes the subsequent 'key' in action.payload.value
operations to throw a TypeError at runtime. Fix all four affected locations
(lines 307-313, 325-333, 348-353, and 367-371) by modifying the initial type
validation to explicitly exclude null, either by adding a null check
(action.payload.value === null) to the existing condition or by checking that
the value is both an object type AND not null before attempting to use the in
operator.
---
Outside diff comments:
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 233-238: In the ValidatedBooleanCollector validation block in the
node.reducer.ts file, replace the throw statement with code that stores the
error in collector.error instead. When the value is not a boolean, set
collector.error to an appropriate error message (such as "Value argument must be
a boolean") and then return early, rather than throwing. This aligns with the
error-as-state contract where errors should be captured in the collector state
instead of being thrown to crash the caller path.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/client.store.ts`:
- Around line 335-338: Add a regression test to cover the dispatch-only behavior
of the update function in the nodeSlice.actions.update call. The test should
verify that when invalid updates are dispatched to the store (through the
function that always returns null), the errors are properly surfaced and
accessible via the collector.error state property. This ensures the contract
shift where the function no longer validates but instead relies on
dispatch-based error handling remains explicit and detectable through test
coverage.
In `@packages/davinci-client/src/lib/node.reducer.test.ts`:
- Around line 432-563: Add two new test cases to the test suite in
node.reducer.test.ts following the same pattern as the existing tests. The first
test case should verify that when a non-boolean value (e.g., a string or number)
is passed to a ValidatedBooleanCollector via the node/update action, the
collector's error field is set with an appropriate error message. The second
test case should verify that when value: null is passed to phone or FIDO object
collectors via the node/update action, the collector's error field is set with
an appropriate error message. Both tests should follow the existing pattern: set
up initial state with the collector, create a node/update action with the
invalid payload, call nodeCollectorReducer, and assert that the resulting
collector has the expected error string set.
🪄 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: 0e48a8ed-2835-41e5-bb9c-8bfabeadef30
📒 Files selected for processing (3)
packages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.ts
| if (typeof action.payload.value !== 'object') { | ||
| throw new Error('Value argument must be an object'); | ||
| collector.error = 'Value argument must be an object'; | ||
| return; | ||
| } | ||
| if (!('phoneNumber' in action.payload.value) || !('countryCode' in action.payload.value)) { | ||
| throw new Error('Value argument must contain a phoneNumber and countryCode property'); | ||
| collector.error = 'Value argument must contain a phoneNumber and countryCode property'; | ||
| return; |
There was a problem hiding this comment.
null payloads can crash object collectors (in operator on null).
At Lines 307/325/348/367, typeof action.payload.value !== 'object' allows null, then Lines 311/330/352/370 run 'key' in action.payload.value, which throws a runtime TypeError.
Suggested fix pattern (apply in all four branches)
- if (typeof action.payload.value !== 'object') {
+ if (
+ typeof action.payload.value !== 'object' ||
+ action.payload.value === null ||
+ Array.isArray(action.payload.value)
+ ) {
collector.error = 'Value argument must be an object';
return;
}Also applies to: 325-333, 348-353, 367-371
🤖 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 307 - 313, The
type check using typeof action.payload.value !== 'object' incorrectly allows
null values to pass, since typeof null returns 'object' in JavaScript. This
causes the subsequent 'key' in action.payload.value operations to throw a
TypeError at runtime. Fix all four affected locations (lines 307-313, 325-333,
348-353, and 367-371) by modifying the initial type validation to explicitly
exclude null, either by adding a null check (action.payload.value === null) to
the existing condition or by checking that the value is both an object type AND
not null before attempting to use the in operator.
|
|
||
| if (collector.category === 'ActionCollector') { | ||
| throw new Error('ActionCollectors are read-only'); | ||
| collector.error = 'ActionCollectors are read-only'; |
There was a problem hiding this comment.
I think this is a good place to store the error, however, how can the application check for an error on the collector? When the collector error is updated immediately after calling davinciClient.update() this doesn't trigger a node change so the set of collectors that the application currently has is stale.
While we could technically subscribe to the store after an update, this seems like a bit much to be doing each time update is called. Is there a better way to handle this? Can we perhaps return error objects from an updater dispatch?
Additionally, is this change considered breaking if we are asking the developer to do an additional error check on the collector after update is called?
Add comprehensive test coverage for error handling in node.reducer for: - NoValueCollector update rejection - Undefined value detection - Type validation for SingleValueCollector (must be string) - MultiValueCollector object rejection - PollingCollector update rejection All tests fail as expected in RED phase (throws are unhandled by reducer).
…e writes Eliminates thrown errors from updateCollectorValues — all validation failures now write to collector.error instead, keeping Redux state predictable and observable rather than blowing up the call stack. Also adds an explicit PollingCollector type guard before the category branches so that polling collectors (which are categorized as SingleValueAutoCollector) are correctly rejected as read-only.
…update Stale validation errors persisted in state after a user corrected their input — a subsequent valid dispatch left the previous error message visible. Mirror the pattern already used in pollCollectorValues by resetting collector.error to null on every successful value-assignment path in updateCollectorValues.
ryanbas21
force-pushed
the
remove-throw-reducers
branch
from
f59478a to
4ae3a06
Compare
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We removed the erroneous PollingCollector type check that was blocking collector.input.value from being set, which broke the polling event flow: davinci.utils.ts relies on a non-empty collector.input.value to set eventType: 'polling' and include the status in formData sent to the server. By letting PollingCollector (category SingleValueAutoCollector) fall through to the existing category branch, the poll status (e.g. 'timedOut', 'expired') is correctly stored and transmitted, restoring the server response that the E2E tests depend on.
Tip
✅ We verified this fix by re-running @forgerock/davinci-suites:e2e-ci--src/polling.test.ts.
Suggested Fix changes
diff --git a/packages/davinci-client/src/lib/node.reducer.test.ts b/packages/davinci-client/src/lib/node.reducer.test.ts
index 0833da1..7b4c063 100644
--- a/packages/davinci-client/src/lib/node.reducer.test.ts
+++ b/packages/davinci-client/src/lib/node.reducer.test.ts
@@ -543,7 +543,7 @@ describe('The node collector reducer', () => {
);
});
- it('should set error on PollingCollector when update is attempted', () => {
+ it('should update PollingCollector input value when a string status is provided', () => {
const state: PollingCollector[] = [
{
category: 'SingleValueAutoCollector',
@@ -555,11 +555,11 @@ describe('The node collector reducer', () => {
output: { key: 'poll', type: 'POLLING', config: { pollInterval: 1000, pollRetries: 3 } },
},
];
- const action = { type: 'node/update', payload: { id: 'poll-0', value: 'anything' } };
+ const action = { type: 'node/update', payload: { id: 'poll-0', value: 'timedOut' } };
const result = nodeCollectorReducer(state, action);
- expect(result.find((c) => c.id === 'poll-0')?.error).toBe(
- 'This collector type does not support value updates',
- );
+ const collector = result.find((c) => c.id === 'poll-0') as PollingCollector | undefined;
+ expect(collector?.error).toBeNull();
+ expect(collector?.input.value).toBe('timedOut');
});
it('should handle QR_CODE field type', () => {
diff --git a/packages/davinci-client/src/lib/node.reducer.ts b/packages/davinci-client/src/lib/node.reducer.ts
index debccdb..c5c1deb 100644
--- a/packages/davinci-client/src/lib/node.reducer.ts
+++ b/packages/davinci-client/src/lib/node.reducer.ts
@@ -215,11 +215,6 @@ export const nodeCollectorReducer = createReducer(initialCollectorValues, (build
return;
}
- if (collector.type === 'PollingCollector') {
- collector.error = 'This collector type does not support value updates';
- return;
- }
-
if (action.payload.value === undefined) {
collector.error = 'Value argument cannot be undefined';
return;
Or Apply changes locally with:
npx nx-cloud apply-locally vTNK-XvkW
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
5 participants
JIRA Ticket
N/A
Description
Remove throws from reducers, since they are impure. This is tricky because we have to determine what we want in state then. I opted for an unknown collector but im not positive of this its weird.
Summary by CodeRabbit
Bug Fixes
Tests
errorappropriately across collector types and invalid/edge-case inputs.