Skip to content

refactor: replace old tables with dynamic-mat-table and remove column icons#2349

Open
abdimo101 wants to merge 11 commits into
masterfrom
replace-old-tables-with-dynamic-mat-table
Open

refactor: replace old tables with dynamic-mat-table and remove column icons#2349
abdimo101 wants to merge 11 commits into
masterfrom
replace-old-tables-with-dynamic-mat-table

Conversation

@abdimo101
Copy link
Copy Markdown
Member

@abdimo101 abdimo101 commented Apr 22, 2026

Description

This PR replaces old tables with dynamic-mat-table in datafiles.component and related-datasets.component and removes column header icons to keep table headers consistent.

Datafiles table:
image

Related Datasets table:
image

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 dataset-related tables to use dynamic-mat-table with centralized pagination and selection handling while aligning table headers and visuals.

New Features:

  • Introduce dynamic-mat-table data source, pagination, and row event handling for datafiles and related datasets views.

Bug Fixes:

  • Ensure selected file size is accurately derived from the underlying files collection after row and master selection changes.
  • Keep related datasets pagination and counts in sync with store-driven view model updates.

Enhancements:

  • Replace legacy table components in datafiles and related datasets views with dynamic-mat-table, including server-side pagination and standardized empty states.
  • Simplify selection logic in the datafiles component by delegating row and master selection handling to dynamic-mat-table events.
  • Remove column header icons across multiple tables to maintain a consistent, cleaner table header appearance.
  • Adjust datafiles header styling to support variable height content.

@abdimo101 abdimo101 requested a review from a team as a code owner April 22, 2026 14:41
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In DatafilesComponent, paginationMode is set to 'server-side' but there is no (paginationChange) handler wired up, so changing pages in the dynamic-mat-table will not update dataSource; either implement a paginationChange handler that slices/requests data and calls dataSource.next(...) or switch to 'client-side' pagination if all files are already loaded.
  • In RelatedDatasetsComponent you are subscribing to vm$ and datasetsPerPage$ manually just to push into dataSource and update pagination, while also using vm$ with the async pipe in the template; consider binding the table directly to vm.relatedDatasets/vm.relatedDatasetsCount via the async pipe and deriving pagination properties in the template to avoid extra BehaviorSubjects and explicit subscriptions.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In DatafilesComponent, `paginationMode` is set to `'server-side'` but there is no `(paginationChange)` handler wired up, so changing pages in the dynamic-mat-table will not update `dataSource`; either implement a paginationChange handler that slices/requests data and calls `dataSource.next(...)` or switch to `'client-side'` pagination if all files are already loaded.
- In RelatedDatasetsComponent you are subscribing to `vm$` and `datasetsPerPage$` manually just to push into `dataSource` and update `pagination`, while also using `vm$` with the async pipe in the template; consider binding the table directly to `vm.relatedDatasets`/`vm.relatedDatasetsCount` via the async pipe and deriving pagination properties in the template to avoid extra BehaviorSubjects and explicit subscriptions.

## Individual Comments

### Comment 1
<location path="src/app/datasets/datafiles/datafiles.component.ts" line_range="126-128" />
<code_context>
+    DataFiles_File[]
+  >([]);
+
+  paginationMode: TablePaginationMode = "server-side";
+
+  pagination: TablePagination = {
+    pageSizeOptions: [10, 25, 50],
+    pageIndex: 0,
</code_context>
<issue_to_address>
**issue (bug_risk):** Table pagination is configured as server-side but the component never handles pagination changes or slices the data.

With `paginationMode: "server-side"` and `pagination.length = files.length`, the table is configured for server-side pagination, but the component never handles `paginationChange` and always passes the full `files` array to `dataSource`. Either implement `paginationChange` handling (and update `dataSource` / fetch new pages) to match server-side behavior, or change `paginationMode` to the client-side option if pagination should be done entirely in memory.
</issue_to_address>

### Comment 2
<location path="src/app/datasets/related-datasets/related-datasets.component.ts" line_range="83" />
<code_context>
+    },
+  };
+
+  dataSource: BehaviorSubject<object[]> = new BehaviorSubject<object[]>([]);
+
+  paginationMode: TablePaginationMode = "server-side";
</code_context>
<issue_to_address>
**suggestion:** Using `object[]` for the dataSource loses type safety for related datasets.

Given the table and view model both use `OutputDatasetObsoleteDto`/`DatasetClass`-shaped data, keeping `dataSource` as `BehaviorSubject<object[]>` weakens compile-time safety (e.g. when accessing `sender.row` in `onRowEvent` or formatting rows). Please narrow this to a concrete type such as `BehaviorSubject<DatasetClass[]>` (or the relevant DTO) so the table interactions remain type-safe.

Suggested implementation:

```typescript
  dataSource: BehaviorSubject<DatasetClass[]> = new BehaviorSubject<DatasetClass[]>([]);

```

1. Ensure `DatasetClass` (or the specific DTO you actually use for related datasets, e.g. `OutputDatasetObsoleteDto`) is imported at the top of `related-datasets.component.ts`:
   - For example:
     - `import { DatasetClass } from '...';`
   - Or replace `DatasetClass` with `OutputDatasetObsoleteDto` if that is the correct model:
     - `dataSource: BehaviorSubject<OutputDatasetObsoleteDto[]> = new BehaviorSubject<OutputDatasetObsoleteDto[]>([]);`
2. If the table template or other code assumes a different concrete type (e.g. `OutputDatasetObsoleteDto`), align the generic type argument here with whatever type is used in row event handlers (`sender.row`) and formatting logic, so the compiler enforces consistency across the component.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/app/datasets/datafiles/datafiles.component.ts Outdated
Comment thread src/app/datasets/related-datasets/related-datasets.component.ts Outdated
@abdimo101 abdimo101 requested review from Junjiequan and nitrosx April 27, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant