Scicat proposal page refactor 2

[Junjiequan]

Description

Short description of the pull request

Motivation

Background on use case, changes needed

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 proposal dashboard into modular components and simplify filters, search, and pagination using facet counts

New Features:

• Introduce ProposalSearchBarComponent, ProposalSideFilterComponent, and ProposalTableComponent to replace the monolithic dynamic table
• Add shared FullTextSearchBarModule and SharedFilterModule for reusable search and filter UI
• Support nested data rendering in tables via getColumnValue utility

Enhancements:

• Consolidate query parameters into a single searchQuery object and dispatch fetchFacetCountsAction alongside fetchProposalsAction
• Replace separate count endpoint with facet counts in effects, selectors, actions, and reducer
• Simplify pagination and sorting logic by merging into combineLatest of route params

Build:

• Bump @scicatproject/scicat-sdk-ts-angular to 4.19.0 and exceljs to 4.4.0

Tests:

• Update Cypress tests to click the search button and use new data-cy selectors across proposals, datasets, and elastic-search flows
• Adjust unit tests for effects, reducers, selectors, and actions to use fetchFacetCounts semantics instead of the old count actions

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• There are still lingering TODO comments (e.g., “Should we hardcode the facet counts list here?”)—please address or remove them to avoid technical debt.
• You’re using combineLatest([this.params$]) for a single observable; you can simplify that to just subscribing directly to params$.
• The tableDefaultSettingsConfig is duplicated in two components—consider extracting it into a shared constant or service to reduce code duplication.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- There are still lingering TODO comments (e.g., “Should we hardcode the facet counts list here?”)—please address or remove them to avoid technical debt.
- You’re using combineLatest([this.params$]) for a single observable; you can simplify that to just subscribing directly to params$.
- The tableDefaultSettingsConfig is duplicated in two components—consider extracting it into a shared constant or service to reduce code duplication.

## Individual Comments

### Comment 1
<location> `src/app/proposals/proposal-dashboard/proposal-dashboard.component.ts:92` </location>
<code_context>
-        if (queryParams.textSearch) {
-          this.globalTextSearch = queryParams.textSearch;
-        }
+        const searchQuery = JSON.parse(queryParams.searchQuery || "{}");

         this.store.dispatch(
</code_context>

<issue_to_address>
Consider handling JSON.parse errors for malformed searchQuery.

Wrap JSON.parse in a try/catch block or use a safe parsing utility to prevent runtime errors from invalid input.
</issue_to_address>

### Comment 2
<location> `src/app/proposals/proposal-table/proposal-table.component.ts:148` </location>
<code_context>
+  dataSource!: BehaviorSubject<ProposalClass[]>;
+
+  @Input()
+  defaultPageSize: number;
+
+  constructor(
</code_context>

<issue_to_address>
defaultPageSize input should have a default value.

Setting a default value (such as 10) for defaultPageSize will prevent issues if the parent does not provide one.
</issue_to_address>

### Comment 3
<location> `src/app/proposals/proposal-table/proposal-table.component.ts:240` </location>
<code_context>
-    });
-  }
-
-  onGlobalTextSearchChange(text: string) {
-    this.router.navigate([], {
-      queryParams: {
</code_context>

<issue_to_address>
onGlobalTextSearchChange is unused and could be removed.

The global text search functionality has moved to a separate search bar, making this method redundant. Removing it will help keep the codebase clear.

Suggested implementation:

```typescript

```

If `onGlobalTextSearchChange` is referenced in the component's template (HTML file) or elsewhere in the TypeScript file, those references should also be removed to prevent errors.
</issue_to_address>

### Comment 4
<location> `src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.ts:66` </location>
<code_context>
+    }
+  }
+
+  applyFilters() {
+    const { queryParams } = this.route.snapshot;
+    const searchQuery = JSON.parse(queryParams.searchQuery || "{}");
</code_context>

<issue_to_address>
applyFilters always merges text from previous searchQuery.

Users may be unable to clear the text filter as expected. Please allow explicit clearing or clarify this behavior in the documentation.
</issue_to_address>

### Comment 5
<location> `src/app/shared/modules/full-text-search-bar/full-text-search-bar.component.ts:69` </location>
<code_context>
-    this.searchTermSubject.next(terms);
-  }
-
-  onClear(): void {
-    this.searchTerm = "";
-    this.searchTermSubject.next(undefined);
</code_context>

<issue_to_address>
onClear emits undefined for searchTermSubject, which may cause issues.

Downstream consumers may expect a string, so emitting an empty string would be more consistent and help prevent potential issues.
</issue_to_address>

### Comment 6
<location> `src/app/shared/modules/full-text-search-bar/full-text-search-bar.component.ts:21` </location>
<code_context>
-  ],
-  standalone: true,
-})
-export class FullTextSearchBarComponent implements OnInit, OnDestroy {
-  @Input() prefilledValue = "";
-  @Input() placeholder = "Text Search";
</code_context>

<issue_to_address>
Consider refactoring to use Angular FormControl and takeUntil for teardown instead of manual Subjects and subscription management.

Consider dropping the manual `BehaviorSubject`/`Subject` + `merge` + manual subscription list in favor of:

1) an Angular `FormControl` for your text input  
2) a single `Subject` (if you still want to debounce button‐clicks)  
3) an `onDestroy$` + `takeUntil` teardown pattern  

You end up with two small pipelines instead of one big merged stream and no manual `.unsubscribe()` calls:

```ts
import {
  Component,
  Input,
  Output,
  EventEmitter,
  OnInit,
  OnDestroy
} from "@angular/core";
import { FormControl } from "@angular/forms";
import { Subject } from "rxjs";
import {
  debounceTime,
  distinctUntilChanged,
  takeUntil,
  withLatestFrom,
  tap,
  startWith
} from "rxjs/operators";

@Component({
  selector: "full-text-search-bar",
  templateUrl: "./full-text-search-bar.component.html",
  styleUrls: ["./full-text-search-bar.component.scss"]
})
export class FullTextSearchBarComponent implements OnInit, OnDestroy {
  @Input() prefilledValue = "";
  @Input() placeholder = "Text Search";
  @Input() clear = false;

  @Output() textChange = new EventEmitter<string>();
  @Output() searchAction = new EventEmitter<void>();

  searchControl = new FormControl(this.prefilledValue);
  private searchClick$ = new Subject<void>();
  private onDestroy$ = new Subject<void>();

  ngOnInit() {
    // 1) Debounced text changes
    this.searchControl.valueChanges
      .pipe(
        debounceTime(200),
        distinctUntilChanged(),
        takeUntil(this.onDestroy$),
        tap(term => this.textChange.emit(term ?? ""))
      )
      .subscribe();

    // 2) Debounced “search” clicks
    this.searchClick$
      .pipe(
        debounceTime(250),
        withLatestFrom(
          this.searchControl.valueChanges.pipe(startWith(this.prefilledValue))
        ),
        takeUntil(this.onDestroy$),
        tap(() => this.searchAction.emit())
      )
      .subscribe();
  }

  onSearch(): void {
    this.searchClick$.next();
  }

  onClear(): void {
    this.searchControl.setValue("");
    this.searchClick$.next();
  }

  ngOnDestroy(): void {
    this.onDestroy$.next();
    this.onDestroy$.complete();
  }
}
```

Benefits:

- No `subscriptions: Subscription[]` array
- Predictable teardown via `takeUntil(onDestroy$)`
- Leverages `FormControl.valueChanges` instead of a `BehaviorSubject`
- Keeps all existing behavior (debounce + distinctUntilChanged + withLatestFrom) intact.
</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.}