feat: dynamic datafiles table#2429
Draft
abdellah257 wants to merge 1 commit into
Draft
Conversation
abdellah257
marked this pull request as draft
June 17, 2026 14:03
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
TimeDurationPipe.transform, the returned string for minutes/seconds currently usesmfor both units (${leftMinutes}m ${leftSeconds}m); this should likely usesfor seconds to avoid confusing output. - In
DynamicDatafilesComponent.updateActionItemsFiles, the files array is rebuilt on every selection update, buttooLargeFile,totalFileSize, andselectedFileSizeare all recalculated from that array rather than from the selected subset; consider explicitly basing these aggregates on the selected files to avoid unnecessary work and make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TimeDurationPipe.transform`, the returned string for minutes/seconds currently uses `m` for both units (`${leftMinutes}m ${leftSeconds}m`); this should likely use `s` for seconds to avoid confusing output.
- In `DynamicDatafilesComponent.updateActionItemsFiles`, the files array is rebuilt on every selection update, but `tooLargeFile`, `totalFileSize`, and `selectedFileSize` are all recalculated from that array rather than from the selected subset; consider explicitly basing these aggregates on the selected files to avoid unnecessary work and make the intent clearer.
## Individual Comments
### Comment 1
<location path="src/app/shared/pipes/time-duration.pipe.ts" line_range="15-16" />
<code_context>
+ const leftMinutes = minutes % 60;
+ const leftSeconds = Math.round(seconds % 60);
+
+ if (hours === 0) {
+ if (leftMinutes === 0) return `${leftSeconds}s`;
+ return `${leftMinutes}m ${leftSeconds}m`;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix unit typo for seconds in the minutes-only branch
In the `hours === 0 && leftMinutes !== 0` case, the string currently uses `${leftMinutes}m ${leftSeconds}m`. The second unit should be seconds, so this should be `${leftMinutes}m ${leftSeconds}s` to avoid outputs like `2m 30m`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
abdellah257
force-pushed
the
dynamic-datafile-table
branch
6 times, most recently
from
June 19, 2026 07:20
b225afd to
c1fb025
Compare
abdellah257
force-pushed
the
dynamic-datafile-table
branch
from
June 19, 2026 07:52
c1fb025 to
8decc49
Compare
abdellah257
marked this pull request as ready for review
June 19, 2026 09:14
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The origdatablock key logic is duplicated between
getFileOrigdatablockKeyinfiles.store.tsand a private helper inDynamicDatafilesComponent; consider reusing the exported helper to keep key generation consistent and avoid divergence. - The
TimeDurationPipeis registered under the pipe namesecondsTimeDurationbut is referenced in configuration/docs astimeDuration(and mapped astimedurationinapplyColumnPipe); aligning the Angular pipe name with the configuration string would reduce confusion and make it usable directly in templates. - In
getTablePaginationConfig,pageIndexandpageSizeare taken directly fromqueryParams(strings) and passed into the pagination model; coercing these to numbers before assigning would make the typing/usage more robust and avoid subtle issues with components expecting numeric values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The origdatablock key logic is duplicated between `getFileOrigdatablockKey` in `files.store.ts` and a private helper in `DynamicDatafilesComponent`; consider reusing the exported helper to keep key generation consistent and avoid divergence.
- The `TimeDurationPipe` is registered under the pipe name `secondsTimeDuration` but is referenced in configuration/docs as `timeDuration` (and mapped as `timeduration` in `applyColumnPipe`); aligning the Angular pipe name with the configuration string would reduce confusion and make it usable directly in templates.
- In `getTablePaginationConfig`, `pageIndex` and `pageSize` are taken directly from `queryParams` (strings) and passed into the pagination model; coercing these to numbers before assigning would make the typing/usage more robust and avoid subtle issues with components expecting numeric values.
## Individual Comments
### Comment 1
<location path="src/app/datasets/datafiles/dynamic-datafiles/dynamic-datafiles.component.ts" line_range="162-164" />
<code_context>
+
+ this.files = Array.from(files.values());
+
+ this.selectedFileSize = this.files
+ .filter((file) => file.selected)
+ .reduce((sum, file) => sum + file.size, 0);
+ this.tooLargeFile = this.hasTooLargeFiles(this.files);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against undefined `size` values when summing `selectedFileSize` to avoid `NaN`.
Since `DataFile.size` is optional, `sum + file.size` will become `NaN` if any selected file has `size === undefined`, breaking the selected size display. Use a default instead, e.g.:
```ts
this.selectedFileSize = this.files
.filter((file) => file.selected)
.reduce((sum, file) => sum + (file.size ?? 0), 0);
```
</issue_to_address>
### Comment 2
<location path="src/app/datasets/datafiles/dynamic-datafiles/dynamic-datafiles.component.ts" line_range="299-305" />
<code_context>
+ return null;
+ }
+
+ getTablePaginationConfig(dataCount = 0): TablePagination {
+ const { queryParams } = this.route.snapshot;
+
+ return {
+ pageSizeOptions: this.defaultPageSizeOptions,
+ pageIndex: queryParams.pageIndex,
+ pageSize: queryParams.pageSize || this.defaultPageSize,
+ length: dataCount,
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** Normalize `pageIndex`/`pageSize` from `queryParams` to numbers before passing to pagination.
Since `queryParams.pageIndex` and `queryParams.pageSize` are strings from the URL but `TablePagination` expects numbers (as used in `onPaginationChange`), this type mismatch can lead to subtle pagination and arithmetic issues. Consider coercing them to numbers before returning, e.g.:
```ts
const pageIndex = queryParams.pageIndex ? +queryParams.pageIndex : 0;
const pageSize = queryParams.pageSize
? +queryParams.pageSize
: this.defaultPageSize;
return {
pageSizeOptions: this.defaultPageSizeOptions,
pageIndex,
pageSize,
length: dataCount,
};
```
</issue_to_address>
### Comment 3
<location path="docs/contributors/default-list-settings.md" line_range="76" />
<code_context>
+### Datafiles Columns
+
+`defaultDatafilesColumnsList` is an array of column definitions. Each column
+usually points to a property on the file origdatablock row. Datafile fields are
+available under `dataFileList`.
+
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding a possessive to clarify "file origdatablock row".
This wording is a bit ungrammatical. Rephrase to something like “file’s origdatablock row” (or similar for your domain) to make the relationship clearer.
Suggested implementation:
```
### Datafiles Columns
`defaultDatafilesColumnsList` is an array of column definitions. Each column
usually points to a property on the file’s origdatablock row. Datafile fields
are available under `dataFileList`.
This page explains where to configure default list and table behavior in the
frontend.
```
If there are other occurrences of the phrase “file origdatablock row” elsewhere in the documentation, they should similarly be updated to “file’s origdatablock row” (or another possessive construction consistent with your domain terminology).
</issue_to_address>
### Comment 4
<location path="src/app/datasets/datafiles/dynamic-datafiles/dynamic-datafiles.component.ts" line_range="80" />
<code_context>
+
+ files: Array<DataFile> = [];
+ origDatablocks: FileOrigdatablock[] = [];
+ selectedOrigDatablocks: FileOrigdatablock[] = [];
+ dataset$ = this.store.select(selectCurrentDataset);
+ datasetId?: string;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying selection handling by using the store as the single source of truth and a shared key helper so that all derived selection state is computed in one place instead of being duplicated across multiple methods and fields.
You can simplify this component meaningfully by reducing duplicated selection state and reusing existing helpers.
### 1. Use the store as the single source of truth for selection
Right now `selectedOrigDatablocks` is kept both in the store and locally, and manually synced via `refreshSelectedOrigDatablocks`, `updateRowSelectionModel`, `updateActionItemsFiles`, etc. You can remove most of that by subscribing once to the selection selector and deriving everything else from it.
Example refactor:
```ts
// fields
origDatablocks: FileOrigdatablock[] = [];
selectedOrigDatablocks: FileOrigdatablock[] = [];
rowSelectionModel = new SelectionModel<FileOrigdatablock>(true, []);
files: DataFile[] = [];
// ngOnInit
ngOnInit(): void {
this.subscriptions.push(
this.store.select(selectSelectedOrigDatablocks).subscribe(selected => {
this.selectedOrigDatablocks = selected;
this.updateSelectionDerivedState();
}),
);
this.subscriptions.push(
this.store
.select(selectCurrentDatasetFilesWithCountAndTableSettings)
.subscribe(({ origDatablocks, count, tablesSettings }) => {
this.origDatablocks = origDatablocks;
this.tablesSettings = tablesSettings;
this.count = count;
this.pending = false;
this.updateSelectionDerivedState();
this.dataSource.next(origDatablocks);
// ...table init logic
}),
);
}
// centralised derivation
private updateSelectionDerivedState(): void {
const selectedKeys = new Set(
this.selectedOrigDatablocks.map(odb => getFileOrigdatablockKey(odb)),
);
this.rowSelectionModel = new SelectionModel<FileOrigdatablock>(
true,
this.origDatablocks.filter(odb =>
selectedKeys.has(getFileOrigdatablockKey(odb)),
),
);
this.files = this.origDatablocks.map(odb => ({
...odb.dataFileList,
selected: selectedKeys.has(getFileOrigdatablockKey(odb)),
}));
this.selectedFileSize = this.files
.filter(f => f.selected)
.reduce((sum, f) => sum + f.size, 0);
this.tooLargeFile = this.hasTooLargeFiles(this.files);
if (this.actionItems.datasets.length) {
this.actionItems = {
...this.actionItems,
datasets: this.actionItems.datasets.map((dataset, index) =>
index === 0 ? { ...dataset, files: this.files } : dataset,
),
};
}
}
```
With this in place, you can drop:
- `refreshSelectedOrigDatablocks`
- `updateRowSelectionModel`
- `updateActionItemsFiles`
- The `store.select(...).pipe(take(1))` calls in `onRowClick`
And simplify `onRowClick` to just dispatch:
```ts
onRowClick({ event, sender }: IRowEvent<FileOrigdatablock>) {
if (event === RowEventType.RowSelectionChange) {
const fileOrigdatablock = sender.row;
if (!fileOrigdatablock) return;
this.store.dispatch(
sender.checked
? selectOrigDatablockAction({ origDatablock: fileOrigdatablock })
: deselectOrigDatablockAction({ origDatablock: fileOrigdatablock }),
);
} else if (event === RowEventType.MasterSelectionChange) {
this.store.dispatch(
sender.checked
? selectAllOrigDatablocksAction({
origDatablocks: sender.selectionModel?.selected ?? [],
})
: deselectOrigDatablocksAction({ origDatablocks: this.origDatablocks }),
);
}
}
```
The selection flow then becomes: store → selector subscriptions → `updateSelectionDerivedState` → UI, without one-off subscriptions.
### 2. Reuse the shared `getFileOrigdatablockKey`
Instead of redefining `getFileOrigdatablockKey` locally:
```ts
private getFileOrigdatablockKey = (origDatablock: FileOrigdatablock): string =>
[origDatablock.datasetId, origDatablock.dataFileList?.path].join(":");
```
import and use the shared helper from `files.store.ts` (or whichever module currently exports it):
```ts
import { getFileOrigdatablockKey } from "state-management/state/files.store";
// then use directly:
const key = getFileOrigdatablockKey(origDatablock);
```
This reduces duplication and guarantees consistent keying logic across the app.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+162
to
+164
| this.selectedFileSize = this.files | ||
| .filter((file) => file.selected) | ||
| .reduce((sum, file) => sum + file.size, 0); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard against undefined size values when summing selectedFileSize to avoid NaN.
Since DataFile.size is optional, sum + file.size will become NaN if any selected file has size === undefined, breaking the selected size display. Use a default instead, e.g.:
this.selectedFileSize = this.files
.filter((file) => file.selected)
.reduce((sum, file) => sum + (file.size ?? 0), 0);
Comment on lines
+299
to
+305
| getTablePaginationConfig(dataCount = 0): TablePagination { | ||
| const { queryParams } = this.route.snapshot; | ||
|
|
||
| return { | ||
| pageSizeOptions: this.defaultPageSizeOptions, | ||
| pageIndex: queryParams.pageIndex, | ||
| pageSize: queryParams.pageSize || this.defaultPageSize, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Normalize pageIndex/pageSize from queryParams to numbers before passing to pagination.
Since queryParams.pageIndex and queryParams.pageSize are strings from the URL but TablePagination expects numbers (as used in onPaginationChange), this type mismatch can lead to subtle pagination and arithmetic issues. Consider coercing them to numbers before returning, e.g.:
const pageIndex = queryParams.pageIndex ? +queryParams.pageIndex : 0;
const pageSize = queryParams.pageSize
? +queryParams.pageSize
: this.defaultPageSize;
return {
pageSizeOptions: this.defaultPageSizeOptions,
pageIndex,
pageSize,
length: dataCount,
};| ### Datafiles Columns | ||
|
|
||
| `defaultDatafilesColumnsList` is an array of column definitions. Each column | ||
| usually points to a property on the file origdatablock row. Datafile fields are |
Contributor
There was a problem hiding this comment.
suggestion (typo): Consider adding a possessive to clarify "file origdatablock row".
This wording is a bit ungrammatical. Rephrase to something like “file’s origdatablock row” (or similar for your domain) to make the relationship clearer.
Suggested implementation:
### Datafiles Columns
`defaultDatafilesColumnsList` is an array of column definitions. Each column
usually points to a property on the file’s origdatablock row. Datafile fields
are available under `dataFileList`.
This page explains where to configure default list and table behavior in the
frontend.
If there are other occurrences of the phrase “file origdatablock row” elsewhere in the documentation, they should similarly be updated to “file’s origdatablock row” (or another possessive construction consistent with your domain terminology).
|
|
||
| files: Array<DataFile> = []; | ||
| origDatablocks: FileOrigdatablock[] = []; | ||
| selectedOrigDatablocks: FileOrigdatablock[] = []; |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying selection handling by using the store as the single source of truth and a shared key helper so that all derived selection state is computed in one place instead of being duplicated across multiple methods and fields.
You can simplify this component meaningfully by reducing duplicated selection state and reusing existing helpers.
1. Use the store as the single source of truth for selection
Right now selectedOrigDatablocks is kept both in the store and locally, and manually synced via refreshSelectedOrigDatablocks, updateRowSelectionModel, updateActionItemsFiles, etc. You can remove most of that by subscribing once to the selection selector and deriving everything else from it.
Example refactor:
// fields
origDatablocks: FileOrigdatablock[] = [];
selectedOrigDatablocks: FileOrigdatablock[] = [];
rowSelectionModel = new SelectionModel<FileOrigdatablock>(true, []);
files: DataFile[] = [];
// ngOnInit
ngOnInit(): void {
this.subscriptions.push(
this.store.select(selectSelectedOrigDatablocks).subscribe(selected => {
this.selectedOrigDatablocks = selected;
this.updateSelectionDerivedState();
}),
);
this.subscriptions.push(
this.store
.select(selectCurrentDatasetFilesWithCountAndTableSettings)
.subscribe(({ origDatablocks, count, tablesSettings }) => {
this.origDatablocks = origDatablocks;
this.tablesSettings = tablesSettings;
this.count = count;
this.pending = false;
this.updateSelectionDerivedState();
this.dataSource.next(origDatablocks);
// ...table init logic
}),
);
}
// centralised derivation
private updateSelectionDerivedState(): void {
const selectedKeys = new Set(
this.selectedOrigDatablocks.map(odb => getFileOrigdatablockKey(odb)),
);
this.rowSelectionModel = new SelectionModel<FileOrigdatablock>(
true,
this.origDatablocks.filter(odb =>
selectedKeys.has(getFileOrigdatablockKey(odb)),
),
);
this.files = this.origDatablocks.map(odb => ({
...odb.dataFileList,
selected: selectedKeys.has(getFileOrigdatablockKey(odb)),
}));
this.selectedFileSize = this.files
.filter(f => f.selected)
.reduce((sum, f) => sum + f.size, 0);
this.tooLargeFile = this.hasTooLargeFiles(this.files);
if (this.actionItems.datasets.length) {
this.actionItems = {
...this.actionItems,
datasets: this.actionItems.datasets.map((dataset, index) =>
index === 0 ? { ...dataset, files: this.files } : dataset,
),
};
}
}With this in place, you can drop:
refreshSelectedOrigDatablocksupdateRowSelectionModelupdateActionItemsFiles- The
store.select(...).pipe(take(1))calls inonRowClick
And simplify onRowClick to just dispatch:
onRowClick({ event, sender }: IRowEvent<FileOrigdatablock>) {
if (event === RowEventType.RowSelectionChange) {
const fileOrigdatablock = sender.row;
if (!fileOrigdatablock) return;
this.store.dispatch(
sender.checked
? selectOrigDatablockAction({ origDatablock: fileOrigdatablock })
: deselectOrigDatablockAction({ origDatablock: fileOrigdatablock }),
);
} else if (event === RowEventType.MasterSelectionChange) {
this.store.dispatch(
sender.checked
? selectAllOrigDatablocksAction({
origDatablocks: sender.selectionModel?.selected ?? [],
})
: deselectOrigDatablocksAction({ origDatablocks: this.origDatablocks }),
);
}
}The selection flow then becomes: store → selector subscriptions → updateSelectionDerivedState → UI, without one-off subscriptions.
2. Reuse the shared getFileOrigdatablockKey
Instead of redefining getFileOrigdatablockKey locally:
private getFileOrigdatablockKey = (origDatablock: FileOrigdatablock): string =>
[origDatablock.datasetId, origDatablock.dataFileList?.path].join(":");import and use the shared helper from files.store.ts (or whichever module currently exports it):
import { getFileOrigdatablockKey } from "state-management/state/files.store";
// then use directly:
const key = getFileOrigdatablockKey(origDatablock);This reduces duplication and guarantees consistent keying logic across the app.
abdellah257
marked this pull request as draft
July 10, 2026 08:52
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
adding the option to opt for a dynamic datafiles table.
Motivation
Supports custom columns, and a dynamic fetching of datafiles per batch using
/api/v3/origdatablocks/fullqueryinstead of storing all files in memory with a/api/v3/datasets/{pid}/origdatablocksrequest.Changes:
This PR include multiple changes:
fileswith adatasetIdfilter to support per batch loading of files.static-datafiles(old view) ordynamic-datatfilessupported in this PR.dynamicDatafilesViewEnabledanddefaultDatafilesColumnsListto configure the dynamic datafiles table.pipeoption at table columns configuration with 3 pipes currently supported (filesize,date,timeDuration).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
Summary by Sourcery
Introduce a configurable, server-side backed dynamic datafiles table for dataset details, including selection state management and integration with existing tables and actions.
New Features:
Bug Fixes:
Enhancements:
Documentation: