-
Notifications
You must be signed in to change notification settings - Fork 33
refactor: complete checkbox filter #2065
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
Merged
Junjiequan
merged 5 commits into
master
from
SWAP-5000-scicat-fe-complete-check-box-filter
Oct 23, 2025
Merged
refactor: complete checkbox filter #2065
Junjiequan
merged 5 commits into
master
from
SWAP-5000-scicat-fe-complete-check-box-filter
Oct 23, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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:
- ProposalSideFilterComponent is subscribing to filterConfigs$ but never unsubscribing—implement ngOnDestroy to clear subscriptions and avoid memory leaks.
- There’s a lot of duplicated add/remove filter dispatch logic across the proposals and datasets components—consider extracting that into a shared helper or service to DRY up the code.
- The TODO about restructuring externalSettings hints at a breaking migration; please define a clear migration plan so user settings aren’t disrupted when updating the backend schema.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ProposalSideFilterComponent is subscribing to filterConfigs$ but never unsubscribing—implement ngOnDestroy to clear subscriptions and avoid memory leaks.
- There’s a lot of duplicated add/remove filter dispatch logic across the proposals and datasets components—consider extracting that into a shared helper or service to DRY up the code.
- The TODO about restructuring externalSettings hints at a breaking migration; please define a clear migration plan so user settings aren’t disrupted when updating the backend schema.
## Individual Comments
### Comment 1
<location> `src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.ts:31` </location>
<code_context>
standalone: false,
})
export class ProposalSideFilterComponent implements OnInit {
+ private subscriptions: Subscription[] = [];
appConfig = this.appConfigService.getConfig();
activeFilters: Record<string, string[] | DateRange> = {};
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider unsubscribing from subscriptions in ngOnDestroy to prevent memory leaks.
Currently, subscriptions are added in ngOnInit but not cleaned up in ngOnDestroy. Please implement ngOnDestroy to unsubscribe from all subscriptions and prevent memory leaks.
</issue_to_address>
### Comment 2
<location> `src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.ts:97-101` </location>
<code_context>
+ }
+ });
+
+ this.router.navigate([], {
+ queryParams: {
+ searchQuery: JSON.stringify(searchQuery),
+ },
+ queryParamsHandling: "merge",
+ });
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Navigating with updated queryParams on every filterConfigs$ emission may cause excessive route changes.
Frequent emissions from filterConfigs$ may trigger unnecessary navigations and impact performance. Debounce or only navigate when queryParams actually change.
Suggested implementation:
```typescript
// Debounce navigation and only navigate if queryParams actually change
if (!this._previousSearchQuery || JSON.stringify(this._previousSearchQuery) !== JSON.stringify(searchQuery)) {
clearTimeout(this._debounceNavTimeout);
this._debounceNavTimeout = setTimeout(() => {
this.router.navigate([], {
queryParams: {
searchQuery: JSON.stringify(searchQuery),
},
queryParamsHandling: "merge",
});
this._previousSearchQuery = { ...searchQuery };
}, 300); // 300ms debounce, adjust as needed
}
}
}),
```
You will need to add the following class properties to your component:
```typescript
// At the top of your component class
private _previousSearchQuery: any = null;
private _debounceNavTimeout: any;
```
If your component is destroyed, clear the timeout in ngOnDestroy:
```typescript
ngOnDestroy() {
clearTimeout(this._debounceNavTimeout);
}
```
</issue_to_address>
### Comment 3
<location> `src/app/shared/modules/filters/multiselect-filter.component.html:8` </location>
<code_context>
*ngFor="let item of currentFilter$ | async"
[removable]="true"
(removed)="itemRemoved(item)"
- >{{ item || "No item" }}
+ >{{ getLabel(item) || "No item" }}
<mat-icon matChipRemove>cancel</mat-icon>
</mat-chip-row>
</code_context>
<issue_to_address>
**issue (bug_risk):** getLabel usage assumes item is an id, but item may be an object.
Consider normalizing item to a string id before calling getLabel, or modify getLabel to accept objects as input.
</issue_to_address>
### Comment 4
<location> `src/app/state-management/reducers/proposals.reducer.ts:161-166` </location>
<code_context>
+ const filters = {
+ ...state.proposalFilters.fields,
+ };
+ if (filterType === "multiSelect") {
+ const newValue = (state.proposalFilters.fields[key] || [])
+ .concat(value)
+ .filter((val, i, self) => self.indexOf(val) === i); // Unique
+
+ filters[key] = newValue;
+ } else {
+ filters[key] = value;
</code_context>
<issue_to_address>
**issue:** MultiSelect filter addition does not handle case where value is already an array.
Flatten the concatenated array to prevent nested arrays and ensure filters[key] remains a flat array of strings.
</issue_to_address>
### Comment 5
<location> `src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.ts:112` </location>
<code_context>
}
setFilter(filterKey: string, value: string[]) {
+ // Text filter type is not supported for proposal side panel filters
+ // This is to seperate the logic of side filter panel and top text search box
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the filter update logic into a single helper method to centralize action dispatching and reduce code duplication.
You have three very similar code-paths (checkbox, date, multi-select) all doing:
1. mutate `activeFilters`
2. dispatch one of two actions
3. optionally call `applyFilters()`
We can collapse them into one helper.
```ts
type FilterType = 'checkbox' | 'multiSelect' | 'text';
private updateFilter(
key: string,
value: any,
filterType: FilterType,
remove: boolean = false
) {
const hasValue = Array.isArray(value)
? value.length > 0
: !!(value?.begin || value?.end);
if (hasValue && !remove) {
this.activeFilters[key] = value;
this.store.dispatch(addProposalFilterAction({ key, value, filterType }));
} else {
delete this.activeFilters[key];
this.store.dispatch(removeProposalFilterAction({ key, filterType }));
}
if (this.appConfig.checkBoxFilterClickTrigger) {
this.applyFilters();
}
}
```
Then your three methods become one-liners:
```ts
setFilter(key: string, values: string[]) {
this.updateFilter(key, values, 'checkbox');
}
setDateFilter(key: string, range: DateRange) {
this.updateFilter(key, range, 'text');
}
selectionChange({ event, key, value }: MultiSelectFilterValue) {
const current = (this.activeFilters[key] as string[]) || [];
const newVals = event === 'add'
? [...current, value._id]
: current.filter(v => v !== value._id);
this.updateFilter(key, newVals, 'multiSelect', event === 'remove');
}
```
— this removes the two helper methods (`addMultiSelectFilter…`, `removeMultiSelectFilter…`), centralizes dispatch logic, and flattens out the branching.
Also remember to unsubscribe your `this.subscriptions` in `ngOnDestroy`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.ts
Show resolved
Hide resolved
src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.ts
Show resolved
Hide resolved
7f70ea9 to
81841fe
Compare
81841fe to
1262998
Compare
nitrosx
approved these changes
Oct 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR makes multiselect-filter and checkbox-filter ready to be reused across different components.
It’s mainly a refactoring step that lays the groundwork for separating filters into:
Motivation
Right now, hard and soft filters are mixed in the same UI, even though they work differently.
For example, Location, Group, and Keyword use dropdown selections, while Text and PID require typing.
This inconsistent behavior confuses users and makes the filter logic harder to maintain.
Changes:
defaultProposalsListSettings.filters.Next steps:
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
Implement complete checkbox filter support by centralizing filter configurations in the store, introducing new filter actions and selectors, refactoring filter components to consume appConfig and Redux state, and updating shared filter utilities and templates.
New Features:
Enhancements:
Tests: