fix: address issues from filters and conditions refactor #1956
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @abdimo101 - I've reviewed your changes - here's some feedback:
- applyFilters currently uses two separate take(1) subscriptions which can lead to race conditions—consider merging the removal and addition loops into a single observable chain for deterministic dispatch ordering.
- The addCondition method builds humanNameMap and fieldTypeMap by iterating over every dataset on each call; extracting that metadata mapping into a shared selector or utility would reduce duplication and improve readability.
- The updateCondition/updateConditionField logic relies on manual array splices which makes intent hard to follow—refactoring to use an immutable array update (for example with map or a dedicated helper) would simplify the flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- applyFilters currently uses two separate take(1) subscriptions which can lead to race conditions—consider merging the removal and addition loops into a single observable chain for deterministic dispatch ordering.
- The addCondition method builds humanNameMap and fieldTypeMap by iterating over every dataset on each call; extracting that metadata mapping into a shared selector or utility would reduce duplication and improve readability.
- The updateCondition/updateConditionField logic relies on manual array splices which makes intent hard to follow—refactoring to use an immutable array update (for example with map or a dedicated helper) would simplify the flow.
## Individual Comments
### Comment 1
<location> `src/app/datasets/datasets-filter/datasets-filter.component.ts:227` </location>
<code_context>
}
applyFilters() {
+ this.conditionConfigs$.pipe(take(1)).subscribe((oldConditions) => {
+ (oldConditions || []).forEach((oldCondition) => {
+ this.store.dispatch(
+ removeScientificConditionAction({
+ condition: oldCondition.condition,
+ }),
+ );
+ });
+ });
+
</code_context>
<issue_to_address>
Multiple subscriptions to conditionConfigs$ in applyFilters may cause race conditions.
Subscribing to conditionConfigs$ twice may result in inconsistent state if the observable emits different values between subscriptions. Use a single subscription or combineLatest to maintain consistency.
</issue_to_address>
### Comment 2
<location> `src/app/datasets/datasets-filter/datasets-filter.component.ts:446` </location>
<code_context>
- condition: { ...oldCondition, ...updates },
- };
+ // Adds the new condition if enabled
+ if (newCondition) {
+ updatedConditions.splice(index, 0, newCondition);
- // Adds the updated condition if enabled
- if (condition.enabled) {
- this.store.dispatch(
- addScientificConditionAction({
- condition: updatedConditions[index].condition,
- }),
- );
- this.store.dispatch(
- selectColumnAction({
- name: updatedConditions[index].condition.lhs,
- columnType: "custom",
- }),
- );
+ if (newCondition.enabled) {
+ this.store.dispatch(
+ addScientificConditionAction({ condition: newCondition.condition }),
+ );
+ this.store.dispatch(
+ selectColumnAction({
+ name: newCondition.condition.lhs,
+ columnType: "custom",
+ }),
+ );
+ }
}
</code_context>
<issue_to_address>
No check for newCondition.enabled before dispatching addScientificConditionAction.
Also check for the presence of newCondition.condition.lhs and rhs before dispatching, as done in applyFilters, to prevent dispatching incomplete conditions and ensure consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Adds the new condition if enabled
if (newCondition) {
updatedConditions.splice(index, 0, newCondition);
if (newCondition.enabled) {
this.store.dispatch(
addScientificConditionAction({ condition: newCondition.condition }),
);
this.store.dispatch(
selectColumnAction({
name: newCondition.condition.lhs,
columnType: "custom",
}),
);
}
}
=======
// Adds the new condition if enabled and complete
if (
newCondition &&
newCondition.enabled &&
newCondition.condition &&
newCondition.condition.lhs &&
newCondition.condition.rhs
) {
updatedConditions.splice(index, 0, newCondition);
this.store.dispatch(
addScientificConditionAction({ condition: newCondition.condition }),
);
this.store.dispatch(
selectColumnAction({
name: newCondition.condition.lhs,
columnType: "custom",
}),
);
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ct/frontend into datasets-filter-conditionsV2
There was a problem hiding this comment.
Looks fine to me.
Two minor UI related issues I found:
- In the scientificMetadataCondition modal can we also mark the item that is selected from the list?
- When metadata key is too long, the text is cut off. Can we show the full text no matter how long the key is?
| after(() => { | ||
| cy.removeDatasets(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
There should be at least one removeDatasets on the top level to clean up test datasets, either using afterEach or after. In this case, maybe after is better to clean up all test datasets at the end
There was a problem hiding this comment.
I agree here. Maybe one general afterEach block or just after if that works.
| after(() => { | ||
| cy.removeDatasets(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I agree here. Maybe one general afterEach block or just after if that works.
| proposalId, | ||
| ...overrides, | ||
| datasetName, | ||
| ...overwrites, |
There was a problem hiding this comment.
I see that you named the overwrites correctly here but maybe as I suggested earlier would be nice to pack all of these different dataset params into the overwrites. I know it might require additional work to change all the places where this command is used but I guess it's worth it as it will just make the usage of the command easier. I will give you an example here:
In current case usages we have for example:
cy.createDataset( "raw", testData.rawDataset.datasetName, undefined, "small", { scientificMetadata: { extra_entry_end_time: { type: "number", value: 2, unit: "" }, }, isPublished: true, }, );
In this case you need to pass all other 4 parameters to be able to overwrite the scientificMetadata for example. But if you do the small refactor that I suggested it might look like this:
cy.createDataset( { type: "raw", dataFileSize: "small", scientificMetadata: { extra_entry_end_time: { type: "number", value: 2, unit: "" }, }, isPublished: true, }, );
Let me know if you need some help or better clarification. I will be happy to help
Description
This PR resolves issues and implements improvements identified after merging #1927
Motivation
Background on use case, changes needed
Fixes:
Changes:
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
Fix scientific condition filtering regressions and enhance condition handling after the filters refactor, improve default settings logic, extend condition value types, and update Cypress tests and UI controls.
Bug Fixes:
Enhancements:
CI:
Tests: