-
Notifications
You must be signed in to change notification settings - Fork 34
fix: nan issue when typing incomplete scientific notation #2016
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the updatedConditions mapping logic into a dedicated helper function to improve readability and testability.
- Ensure tempConditionValues is initialized or reset whenever conditionConfigs changes to avoid stale or misaligned buffered inputs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the updatedConditions mapping logic into a dedicated helper function to improve readability and testability.
- Ensure tempConditionValues is initialized or reset whenever conditionConfigs changes to avoid stale or misaligned buffered inputs.
## Individual Comments
### Comment 1
<location> `src/app/datasets/datasets-filter/datasets-filter.component.ts:248-249` </location>
<code_context>
+ condition: {
+ ...config.condition,
+ rhs: isNumeric ? Number(value) : value,
+ relation: isNumeric
+ ? ("EQUAL_TO_NUMERIC" as ScientificCondition["relation"])
+ : ("EQUAL_TO_STRING" as ScientificCondition["relation"]),
+ },
</code_context>
<issue_to_address>
**nitpick:** Type assertion for relation may be unnecessary if type inference is sufficient.
Remove the type assertion if the assignment is already type-safe and the type checker does not require it.
</issue_to_address>
### Comment 2
<location> `src/app/datasets/datasets-filter/datasets-filter.component.ts:271-272` </location>
<code_context>
+ return config;
+ });
+
+ updatedConditions.forEach((oldCondition) => {
this.store.dispatch(
removeScientificConditionAction({
</code_context>
<issue_to_address>
**suggestion:** Variable naming: 'oldCondition' is misleading after mapping to updatedConditions.
Please rename 'oldCondition' to 'condition' or 'updatedCondition' to better reflect its current meaning.
```suggestion
updatedConditions.forEach((condition) => {
this.store.dispatch(
removeScientificConditionAction({
condition: condition.condition,
);
});
```
</issue_to_address>
### Comment 3
<location> `src/app/datasets/datasets-filter/datasets-filter.component.ts:234` </location>
<code_context>
this.conditionConfigs$.pipe(take(1)).subscribe((conditionConfigs) => {
- (conditionConfigs || []).forEach((oldCondition) => {
+ const updatedConditions = (conditionConfigs || []).map((config, i) => {
+ if (this.tempConditionValues[i] !== undefined) {
+ const value = this.tempConditionValues[i];
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the condition transformation logic into a helper function to improve readability and testability.
Here’s one way to collapse that nested‐branching and spread logic into a single helper, then use it inside your subscription. This keeps exactly the same behavior but is much easier to read and test.
```ts
// add somewhere in your component (or in a util file)
private transformConfigWithTempValue(
config: ConditionConfig,
tempVal?: string
): ConditionConfig {
if (tempVal === undefined) {
return config;
}
const isNumeric = tempVal !== '' && !isNaN(Number(tempVal));
const newCondition: ScientificCondition = {
...config.condition,
rhs: isNumeric ? Number(tempVal) : tempVal,
// if it was previously EQUAL_TO(_NUMERIC|_STRING), we overwrite;
// otherwise we just keep the old relation
relation: isNumeric
? 'EQUAL_TO_NUMERIC'
: 'EQUAL_TO_STRING',
};
return { ...config, condition: newCondition };
}
```
Then in your subscription:
```ts
this.conditionConfigs$.pipe(take(1)).subscribe((conditionConfigs) => {
const updatedConditions = (conditionConfigs || []).map((cfg, idx) =>
this.transformConfigWithTempValue(cfg, this.tempConditionValues[idx])
);
// remove old, add new, update settings, fetch…
updatedConditions.forEach(c =>
this.store.dispatch(removeScientificConditionAction({ condition: c.condition }))
);
updatedConditions.forEach(c => {
if (c.enabled && c.condition.lhs && c.condition.rhs) {
this.store.dispatch(addScientificConditionAction({ condition: c.condition }));
}
});
this.store.dispatch(updateUserSettingsAction({
property: { conditions: updatedConditions },
}));
this.store.dispatch(fetchDatasetsAction());
this.store.dispatch(fetchFacetCountsAction());
});
```
• Extracting the mapping makes the transformation logic reusable and removes all the nested `if/else` and重复 spread blocks.
• You can now unit‐test `transformConfigWithTempValue` in isolation without wiring up the whole RxJS pipeline.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
relation: isNumeric | ||
? ("EQUAL_TO_NUMERIC" as ScientificCondition["relation"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Type assertion for relation may be unnecessary if type inference is sufficient.
Remove the type assertion if the assignment is already type-safe and the type checker does not require it.
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Variable naming: 'oldCondition' is misleading after mapping to updatedConditions.
Please rename 'oldCondition' to 'condition' or 'updatedCondition' to better reflect its current meaning.
); | |
}); | |
updatedConditions.forEach((condition) => { | |
this.store.dispatch( | |
removeScientificConditionAction({ | |
condition: condition.condition, | |
); | |
}); |
|
||
this.conditionConfigs$.pipe(take(1)).subscribe((conditionConfigs) => { | ||
(conditionConfigs || []).forEach((oldCondition) => { | ||
const updatedConditions = (conditionConfigs || []).map((config, i) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting the condition transformation logic into a helper function to improve readability and testability.
Here’s one way to collapse that nested‐branching and spread logic into a single helper, then use it inside your subscription. This keeps exactly the same behavior but is much easier to read and test.
// add somewhere in your component (or in a util file)
private transformConfigWithTempValue(
config: ConditionConfig,
tempVal?: string
): ConditionConfig {
if (tempVal === undefined) {
return config;
}
const isNumeric = tempVal !== '' && !isNaN(Number(tempVal));
const newCondition: ScientificCondition = {
...config.condition,
rhs: isNumeric ? Number(tempVal) : tempVal,
// if it was previously EQUAL_TO(_NUMERIC|_STRING), we overwrite;
// otherwise we just keep the old relation
relation: isNumeric
? 'EQUAL_TO_NUMERIC'
: 'EQUAL_TO_STRING',
};
return { ...config, condition: newCondition };
}
Then in your subscription:
this.conditionConfigs$.pipe(take(1)).subscribe((conditionConfigs) => {
const updatedConditions = (conditionConfigs || []).map((cfg, idx) =>
this.transformConfigWithTempValue(cfg, this.tempConditionValues[idx])
);
// remove old, add new, update settings, fetch…
updatedConditions.forEach(c =>
this.store.dispatch(removeScientificConditionAction({ condition: c.condition }))
);
updatedConditions.forEach(c => {
if (c.enabled && c.condition.lhs && c.condition.rhs) {
this.store.dispatch(addScientificConditionAction({ condition: c.condition }));
}
});
this.store.dispatch(updateUserSettingsAction({
property: { conditions: updatedConditions },
}));
this.store.dispatch(fetchDatasetsAction());
this.store.dispatch(fetchFacetCountsAction());
});
• Extracting the mapping makes the transformation logic reusable and removes all the nested if/else
and重复 spread blocks.
• You can now unit‐test transformConfigWithTempValue
in isolation without wiring up the whole RxJS pipeline.
Description
This PR refactors the handling of condition values in datasets filter component.
Motivation
Previously the condition values was updated in the global state immediately as the user typed, causing issues when entering partial scientific notation (e.g typing "3e" would result in the value being set to NaN).
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Refactor the datasets filter component to buffer raw input values and defer updating the global state until filter application, fixing NaN errors when entering incomplete scientific notation
Bug Fixes:
Enhancements: