refactor: move condition filter logic to shared-filter component

[abdimo101]

Description

Moved the condition filter functionality from datasets-filter.component to shared-filter.component, so it can be reused across both datasets and samples pages.

• Added conditionType property to ConditionConfig interface to separate datasets vs samples conditions
• Added dynamic action dispatchers (addConditionAction, removeConditionAction) as inputs to decouple store actions
• Updated removeCharacteristicsFilterAction to use lhs instead of index for cleaner removal

Samples page before:

Samples page after:

Motivation

Initially this was only meant to be a bugfix where in the samples page the Condition shows > 0 on item selection and could not be modified. But it was later discussed that the samples page needed a condition filter functionality identical to the datasets page. Rather than duplicating the complex condition filter logic, this refactor makes it easier to implement the functionality on other pages in the future.

Fixes:

Please provide a list of the fixes implemented in this PR

• Items added

Changes:

Please provide a list of the changes implemented by this PR

• changes made

Tests included

• Included for each change/fix?
• Passing? (Merge will not be approved unless this is checked)

Documentation

• swagger documentation updated [required]
• official documentation updated [nice-to-have]

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

• Does it require a specific version of the backend
• which version of the backend is required:

Summary by Sourcery

Refactor the scientific condition filtering functionality into the shared filter component and integrate it for both datasets and samples pages, including support for condition typing and unit-aware conditions.

New Features:

• Enable reusable condition filter UI and logic through the shared-filter component, configurable for datasets and samples via inputs and conditionType.
• Add condition management to the samples dashboard using the shared-filter component, aligning samples filtering with datasets condition filters.

Bug Fixes:

• Change sample characteristics removal to target conditions by lhs key instead of index to avoid incorrect deletions.

Enhancements:

• Extend shared-filter to manage condition configurations in NgRx user settings, including metadata mappings, operator/value handling, and units support.
• Simplify datasets and samples components by delegating condition add/remove/apply logic to the shared-filter component.
• Add a selector to expose whether any filters are applied on samples and use it to control filter reset button state.
• Allow scientific condition rhs values to be arrays of strings or numbers and track condition type (datasets or samples) in ConditionConfig for clearer separation.

Summary by Sourcery

Refactor scientific condition filtering into the shared filter component so it can be reused across datasets and samples, while aligning samples filtering behaviour and store structures with datasets.

New Features:

• Expose reusable condition filter UI and logic on the shared-filter component, configurable via inputs and conditionType for datasets and samples.
• Add condition-based filtering to the samples dashboard using the shared-filter component, including apply/reset controls integrated with the samples store.

Bug Fixes:

• Change sample characteristics removal to target filters by lhs key instead of array index to avoid removing the wrong characteristic.

Enhancements:

• Move datasets condition filter logic, metadata mappings, and units handling from datasets-filter into shared-filter to centralize behaviour.
• Extend ScientificCondition and dialog form controls to support array rhs values for both strings and numbers.
• Track conditionType on ConditionConfig to distinguish datasets and samples conditions and keep them isolated in state.
• Add a selector to expose whether any filters are applied on samples and use it to control the disabled state of the Clear button.
• Simplify datasets and samples components by delegating add/remove/apply condition logic to shared-filter and wiring it to NgRx actions.
• Tidy datasets and samples styling by relocating condition panel styles into the shared-filter styles and simplifying datasets filter tests to match updated dispatch behaviour.

Tests:

• Update datasets, samples reducer, samples actions, and selector tests to reflect the new condition handling, removal semantics, and extended view model.

[sourcery-ai]

Hey there - I've reviewed your changes and found some issues that need to be addressed.

• SharedFilterComponent is now responsible for both generic filter widgets and the full scientific-condition workflow (store interaction, dialogs, snackbars, units), which makes it quite large and tightly coupled; consider extracting the condition-specific logic and template into a dedicated shared-condition-filter component to keep concerns separated and the shared filter simpler.
• The subscriptions array in SharedFilterComponent is populated but none of the subscribe calls (e.g. from ngOnInit, addCondition, removeCondition, etc.) actually add their subscriptions to it, so either wire these up to this.subscriptions or switch consistently to take(1)/async/takeUntil patterns so ngOnDestroy is not misleading.
• The new removeCharacteristicsFilterAction API and reducer logic now remove by lhs, which will drop all matching entries; if the UI ever allows multiple conditions with the same lhs, you may want to clarify or enforce uniqueness to avoid unintentionally removing more than one condition.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- SharedFilterComponent is now responsible for both generic filter widgets and the full scientific-condition workflow (store interaction, dialogs, snackbars, units), which makes it quite large and tightly coupled; consider extracting the condition-specific logic and template into a dedicated `shared-condition-filter` component to keep concerns separated and the shared filter simpler.
- The `subscriptions` array in `SharedFilterComponent` is populated but none of the `subscribe` calls (e.g. from `ngOnInit`, `addCondition`, `removeCondition`, etc.) actually add their subscriptions to it, so either wire these up to `this.subscriptions` or switch consistently to `take(1)`/`async`/`takeUntil` patterns so `ngOnDestroy` is not misleading.
- The new `removeCharacteristicsFilterAction` API and reducer logic now remove by `lhs`, which will drop all matching entries; if the UI ever allows multiple conditions with the same `lhs`, you may want to clarify or enforce uniqueness to avoid unintentionally removing more than one condition.

## Individual Comments

### Comment 1
<location> `src/app/shared/modules/shared-filter/shared-filter.component.ts:53-58` </location>
<code_context>
-    end: null,
-  };
+export class SharedFilterComponent implements OnChanges, OnInit, OnDestroy {
+  private subscriptions: Subscription[] = [];
+  private dateRange: DateRange = { begin: null, end: null };
+
</code_context>

<issue_to_address>
**issue (bug_risk):** Subscriptions array is never populated, so ngOnDestroy doesn't actually clean anything up.

The only subscription you create (`filterForm.get('textField')!.valueChanges.subscribe(...)`) is never added to `this.subscriptions`, so `ngOnDestroy` currently does nothing. Either push that (and any future) subscriptions into `this.subscriptions`, or switch to a clearer pattern like `takeUntil(destroy$)`. As written, the cleanup is dead code and the valueChanges subscription isn’t explicitly torn down with the component.
</issue_to_address>

### Comment 2
<location> `src/app/samples/sample-dashboard/sample-dashboard.component.ts:220-229` </location>
<code_context>
-      (conditionConfigs || []).forEach((config) => {
-        this.applyUnitsOptions(config.condition);
-        if (config.enabled && config.condition.lhs && config.condition.rhs) {
-          this.store.dispatch(
-            addScientificConditionAction({
-              condition: config.condition,
</code_context>

<issue_to_address>
**issue (bug_risk):** Resetting samples clears all conditions globally, including dataset-specific conditions.

In `reset()` you call `updateConditionsConfigs({ conditionConfigs: [] })` and `updateUserSettingsAction({ property: { conditions: [] } })`, which clears all `ConditionConfig` entries. Because `ConditionConfig` is shared and differentiated only by `conditionType`, this removes dataset conditions as well as sample ones. When resetting the samples dashboard, you likely want to preserve non-sample configs: filter existing configs by `conditionType !== 'samples'` and pass that filtered list into `updateConditionsConfigs` / `updateUserSettingsAction` instead of an empty array.
</issue_to_address>

### Comment 3
<location> `src/app/shared/modules/shared-filter/shared-filter.component.ts:122` </location>
<code_context>
   }>();
+  @Output() conditionsApplied = new EventEmitter<void>();
+
+  allConditions$ = this.store.select(selectConditions);
+
+  conditionConfigs$ = this.allConditions$.pipe(
</code_context>

<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for condition handling, value mapping, and subscriptions to flatten these methods and remove repeated observable and condition-building logic.

You can reduce complexity and duplication without changing behaviour by extracting a few small helpers and removing unused patterns.

### 1. Centralize repeated `allConditions$` patterns

`addCondition`, `removeCondition`, `updateConditionField`, `toggleConditionEnabled`, `applyConditions`, `applyEnabledConditions` all do:

- `allConditions$.pipe(take(1)).subscribe(...)`
- filter into `myConditions` / `otherConditions`
- find an index for a given condition
- build `updatedConditions` and call `updateStore`

Extract helpers to make each call site smaller and focused:

```ts
private withConditionsForType(
  callback: (ctx: {
    all: ConditionConfig[];
    mine: ConditionConfig[];
    others: ConditionConfig[];
  }) => void,
): void {
  this.allConditions$.pipe(take(1)).subscribe((allConditions) => {
    const all = allConditions || [];
    const mine = all.filter((c) => c.conditionType === this.conditionType);
    const others = all.filter((c) => c.conditionType !== this.conditionType);
    callback({ all, mine, others });
  });
}

private findConditionIndex(
  all: ConditionConfig[],
  condition: ScientificCondition,
): number {
  return all.findIndex(
    (c) => c.condition.lhs === condition.lhs && c.conditionType === this.conditionType,
  );
}
```

Then e.g. `removeCondition` becomes much flatter:

```ts
removeCondition(condition: ConditionConfig, index: number) {
  this.withConditionsForType(({ all }) => {
    const actualIndex = this.findConditionIndex(all, condition.condition);
    if (actualIndex === -1) return;

    const updatedConditions = [...all];
    updatedConditions.splice(actualIndex, 1);
    this.tempConditionValues.splice(index, 1);

    if (condition.enabled) {
      this.removeConditionAction?.(condition.condition);
      this.store.dispatch(
        deselectColumnAction({ name: condition.condition.lhs, columnType: "custom" }),
      );
    }

    if (condition.condition.lhs) {
      this.unitsOptionsService.clearUnitsOptions(condition.condition.lhs);
    }

    this.updateStore(updatedConditions);
  });
}
```

Apply the same `withConditionsForType` helper to `addCondition`, `updateConditionField`, `toggleConditionEnabled`, `applyConditions`, `applyEnabledConditions` to remove a lot of duplicated observable/filter/index boilerplate.

### 2. Consolidate condition-building logic

The logic to keep `type` / `human_name` in sync and to map `EQUAL_TO` to string/numeric variants is repeated in several places. Extract small pure helpers:

```ts
private buildBaseCondition(
  condition: ScientificCondition,
): ScientificCondition {
  const lhs = condition.lhs;
  return {
    ...condition,
    type: this.fieldTypeMap[lhs],
    human_name: this.humanNameMap[lhs],
  };
}

private applyTempValue(
  config: ConditionConfig,
  value: string | undefined,
): ConditionConfig {
  if (value === undefined) {
    return { ...config, condition: this.buildBaseCondition(config.condition) };
  }

  const fieldType = this.fieldTypeMap[config.condition.lhs];
  const isNumeric = value !== "" && !isNaN(Number(value));
  const rhs = isNumeric ? Number(value) : value;

  if (config.condition.relation === "EQUAL_TO") {
    const relation: ScientificCondition["relation"] =
      fieldType === "string" || !isNumeric ? "EQUAL_TO_STRING" : "EQUAL_TO_NUMERIC";

    return {
      ...config,
      condition: { ...this.buildBaseCondition(config.condition), rhs, relation },
    };
  }

  return {
    ...config,
    condition: { ...this.buildBaseCondition(config.condition), rhs },
  };
}
```

Then `applyConditions` becomes much clearer:

```ts
applyConditions() {
  this.withConditionsForType(({ mine, others, all }) => {
    const updatedMine = mine.map((config, i) =>
      this.applyTempValue(config, this.tempConditionValues[i]),
    );

    // remove old
    mine.forEach((c) => this.removeConditionAction?.(c.condition));

    // add updated enabled
    updatedMine.forEach((config) => {
      const { condition, enabled } = config;
      if (
        enabled &&
        condition.lhs &&
        condition.rhs != null &&
        condition.rhs !== ""
      ) {
        this.addConditionAction?.(condition);
      }
    });

    this.updateStore([...others, ...updatedMine]);
    this.tempConditionValues = [];
    this.conditionsApplied.emit();
  });
}
```

This eliminates deep nesting and scattered type/human-name handling.

### 3. Avoid `AsyncPipe` inside the component

`AsyncPipe` injection for `updateConditionRangeValue` is unusual and makes reasoning harder. Instead, keep a local snapshot of condition configs:

```ts
private conditionConfigsSnapshot: ConditionConfig[] = [];

ngOnInit() {
  this.filterForm.get("textField")!.valueChanges.subscribe(() => {
    this.checkboxDisplaylimit = 10;
  });

  this.subscriptions.push(
    this.conditionConfigs$.subscribe((configs) => {
      this.conditionConfigsSnapshot = configs || [];
    }),
  );

  if (this.showConditions) {
    this.buildMetadataMaps();
    this.applyEnabledConditions();
  }
}
```

Then:

```ts
updateConditionRangeValue(index: number, event: Event, rangeIndex: 0 | 1) {
  const newValue = (event.target as HTMLInputElement).value;
  const currentRhs = this.conditionConfigsSnapshot[index]?.condition.rhs;
  const rhs = Array.isArray(currentRhs) ? [...currentRhs] : [undefined, undefined];
  rhs[rangeIndex] = Number(newValue);
  this.updateConditionField(index, { rhs });
}
```

This removes the surprising `AsyncPipe` usage and makes use of the existing `subscriptions` array/`ngOnDestroy` pattern.

### 4. Either remove or fully use `subscriptions`

Right now you define `private subscriptions: Subscription[] = [];` and unsubscribe in `ngOnDestroy`, but only the text-field subscription is stored. If you adopt the snapshot subscription above, you’re now using `subscriptions` meaningfully; otherwise, consider removing the array and `ngOnDestroy` to avoid misleading patterns.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}