refactor: replace old tables with dynamic-mat-table and remove column icons - #2349
refactor: replace old tables with dynamic-mat-table and remove column icons#2349abdimo101 wants to merge 45 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In DatafilesComponent,
paginationModeis set to'server-side'but there is no(paginationChange)handler wired up, so changing pages in the dynamic-mat-table will not updatedataSource; either implement a paginationChange handler that slices/requests data and callsdataSource.next(...)or switch to'client-side'pagination if all files are already loaded. - In RelatedDatasetsComponent you are subscribing to
vm$anddatasetsPerPage$manually just to push intodataSourceand updatepagination, while also usingvm$with the async pipe in the template; consider binding the table directly tovm.relatedDatasets/vm.relatedDatasetsCountvia 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Junjiequan
left a comment
Junjiequan
left a comment
There was a problem hiding this comment.
lgtm, please merge it after dateformat update
8d20922 to
524d5e9
Compare
…m:SciCatProject/frontend into replace-old-tables-with-dynamic-mat-table
Code Review: Branch
|
Code Review: Branch
|
| Criteria | Rating | Notes |
|---|---|---|
| Logic Correctness | Core logic works, but has critical performance issue with count query | |
| Edge Case Handling | Handles basic cases well, but missing several important edge cases | |
| Code Makes Sense | ✅ Yes | Architectural direction is sound and necessary |
| No Unreachable Code | Some dead code remains, but major unreachable code was correctly removed | |
| Production Ready | ❌ No, not yet | Performance issue and edge cases need addressing |
Decision: DO NOT MERGE YET
While the architectural direction is correct and the code is generally well-structured, there are critical issues that must be resolved:
- The performance regression from fetching all files for count makes this worse than the old implementation for large datasets
- The selection state behavior across pagination is unclear and potentially confusing
- Missing edge case handling could cause runtime errors
Path Forward:
-
Fix the count query - This is the blocker. Options:
- API change to return count with paginated results (preferred)
- Use a separate count endpoint
- Accept the workaround with clear documentation and TODO
-
Clarify and fix pagination selection behavior - Define the expected UX and implement consistently
-
Add null checks and validation - Prevent runtime errors
-
Clean up dead code - Remove unused observables and imports
-
Add comprehensive tests - Ensure robustness
Once these issues are addressed, the branch will be production-ready and represents a significant improvement over the current implementation.
Report generated by analyzing git diff between master and replace-old-tables-with-dynamic-mat-table branch
Generated with Mistral AI
Security Review: Branch
|
| Type | Status | Location | Risk | Notes |
|---|---|---|---|---|
XSS via [innerHTML] |
datafiles.component.html:5 |
Medium | [innerHTML]="hasFileAboveMaxSizeWarning()" - Template string from config is rendered as HTML. NOT introduced by this branch |
|
| Template Injection | ✅ Safe | hasFileAboveMaxSizeWarning() |
Low | String replacement of placeholders (<maxDirectDownloadSize>, <sftpHost>, <sourceFolder>) with sanitized values. Uses simple .replace() with controlled values |
| API Parameter Injection | ✅ Safe | datasets.effects.ts |
Low | Query parameters (pid, skip, limit) are properly typed and come from route params or controlled UI inputs |
| Code Injection | ✅ Safe | All components | None | No eval(), new Function(), or dynamic code execution |
Details:
The only potential injection point is the [innerHTML] binding in datafiles.component.html line 5:
<span [innerHTML]="hasFileAboveMaxSizeWarning()"></span>This method constructs HTML by replacing placeholders in a template string:
warning = warning.replace(
"<" + key + ">",
`<strong>${valueMapping[key]}</strong>`,
);Risk: If an administrator sets a malicious maxFileSizeWarning in the config, it could inject HTML/JS. However:
- This vulnerability existed in master before this branch
- It requires admin access to the config file
- The replacement values (
sftpHost,sourceFolder,maxDirectDownloadSize) are controlled by the application and are sanitized
Verdict: No new injection vulnerabilities introduced by this branch.
2. Sensitive User Data Exposure
Assessment: No NEW sensitive data exposure ✅
| Data Type | Status | Location | Risk | Notes |
|---|---|---|---|---|
| JWT Tokens | ✅ No change | datafiles.component.ts |
None | Same token handling as master (auth_token, jwt) for download forms |
| File Metadata | ✅ Same as master | datafiles.component.ts |
Low | Exposes: path, size, time, uid, gid, perm, hash - same fields as before |
| Dataset Metadata | ✅ Same as master | related-datasets.component.ts |
Low | Exposes: pid, name, sourceFolder, size, type, creationTime, owner - same fields as before |
| File Paths | ✅ No change | Both components | Low | sourceFolder paths displayed, same as master |
| Owner Information | ✅ No change | related-datasets.component.ts |
Low | owner field displayed, same as master |
Details:
The changes only affect how data is displayed (which table component is used), not what data is displayed. The same fields that were exposed in the old app-table are exposed in the new dynamic-mat-table.
- Datafiles: Same
DataFiles_Fileinterface fields (path, size, time, chk, uid, gid, perm, hash, selected) - Related Datasets: Same
OutputDatasetObsoleteDtofields (pid, datasetName, sourceFolder, size, type, creationTime, owner)
No new sensitive data is exposed through:
- Table columns
- API responses
- Error messages
- Logs
Verdict: No new sensitive user data exposure introduced by this branch.
3. Insecure API Usage
Assessment: No insecure API usage ✅
| API | Status | Location | Notes |
|---|---|---|---|
| OrigdatablocksV4Service.origDatablocksV4ControllerFindAllFilesV4() | ✅ Secure | datasets.effects.ts |
Official SciCat SDK v4 endpoint, replaces v3 endpoint |
| DatasetsService.datasetsControllerFindAllOrigDatablocksV3() | ❌ Removed | Was in master | Replaced by v4 service |
| DatasetsService.datasetsControllerFindAllDatablocksV3() | ✅ Same | datasets.effects.ts |
Still used for datablocks, unchanged |
Details:
API Changes:
- Old:
datasetsService.datasetsControllerFindAllOrigDatablocksV3(pid)- v3 endpoint - New:
origdatablocksService.origDatablocksV4ControllerFindAllFilesV4(JSON.stringify(query))- v4 endpoint
Security Analysis:
- Both are official SciCat SDK methods
- Both require the same authentication
- The new v4 endpoint accepts a JSON query string, but the query is constructed from controlled parameters (pid from route, skip/limit from pagination)
- No hardcoded credentials or API keys
- No use of insecure protocols (no http:, only https: via SDK)
- No CORS bypass attempts
- No credential transmission in URLs
Query Construction:
const paginatedQuery = {
where: { datasetId: pid }, // pid from route params
limits: {
skip: filters?.skip || 0, // from pagination
limit: filters?.limit || 5, // from pagination
},
};The query is properly structured and uses controlled values. The JSON.stringify() is safe as it only serializes objects.
Verdict: No insecure API usage. The API migration from v3 to v4 is secure and follows the same patterns as the existing code.
4. Authentication Bypass
Assessment: No authentication bypass vulnerabilities ✅
| Attack Vector | Status | Analysis |
|---|---|---|
| Token Manipulation | ❌ Not possible | Tokens come from authService.getToken() and usersService.usersControllerGetUserJWTV3() - unchanged from master |
| PID Manipulation | ❌ Not possible | Dataset PID comes from route parameters; users can only access datasets they have permission for (enforced server-side) |
| Filter Manipulation | ❌ Not possible | Pagination filters (skip, limit) only affect client-side display; server enforces authorization |
| Route-based Access | ✅ Same as master | No changes to route guards or authentication flows |
| Service Injection | ✅ Safe | New OrigdatablocksV4Service is from official SDK, injected via DI like all other services |
Details:
Authentication Flow:
- No changes to how users authenticate
- No changes to token storage or transmission
- No new ways to obtain or manipulate tokens
Authorization:
- The branch does not add or remove any authorization checks
- Server-side authorization (via SciCat API) remains the same
- Client-side, the same data access patterns are used
Data Access:
- Dataset PID comes from the route:
this.datasetPid = dataset.pid - The PID is used to fetch data:
fetchOrigDatablocksAction({ pid: this.datasetPid, ... }) - Users can only navigate to datasets they have access to (enforced by route guards, not shown in this diff)
- No way to access other users' datasets through manipulation
JWT/Token Handling:
The token handling for file downloads is identical to master:
// Same in both master and this branch:
if (this.appConfig.multipleDownloadUseAuthToken) {
this.auth_token = `Bearer ${this.authService.getToken().id}`;
this[`${form}Element`].nativeElement.auth_token.value = this.auth_token;
}
if (!this.jwt) {
this.subscriptions.push(
this.usersService.usersControllerGetUserJWTV3().subscribe((jwt) => {
this.jwt = jwt;
this[`${form}Element`].nativeElement.jwt.value = jwt.jwt;
}),
);
}Verdict: No authentication bypass vulnerabilities introduced by this branch.
Summary
| Security Concern | Introduced by Branch? | Severity | Status |
|---|---|---|---|
XSS via [innerHTML] |
❌ No (pre-existing) | Medium | Not addressed |
| Template injection | ❌ No | Low | Safe |
| Sensitive data exposure | ❌ No | None | Safe |
| Insecure API usage | ❌ No | None | Safe |
| Authentication bypass | ❌ No | None | Safe |
Overall Security Assessment: ✅ SECURE
Final Verdict: The branch introduces NO NEW security vulnerabilities.
The replace-old-tables-with-dynamic-mat-table branch maintains the same security posture as master. It is a pure refactoring that changes how tables are rendered (migrating from app-table to dynamic-mat-table) without changing:
- What data is displayed
- How data is fetched
- How authentication works
- What API endpoints are called
Pre-existing Security Issue (Not Introduced by This Branch):
- XSS vulnerability via
[innerHTML]="hasFileAboveMaxSizeWarning()"indatafiles.component.html- Requires malicious admin config
Recommendation: This branch is secure to merge from a security perspective. The pre-existing XSS issue should be addressed separately (e.g., by sanitizing the HTML or using [textContent] instead of [innerHTML]).
Security review conducted by analyzing git diff between master and replace-old-tables-with-dynamic-mat-table branch
Generated with Mistral AI
…a count dispatch for datafiles component and dataset details dashboard inside fetchDataForTab
|
UPDATE: Loading spinner has been added to datafiles & related datasets table paginators when the count request is in a pending state. Similar to how it was done in #2399 |
Junjiequan
left a comment
Junjiequan
left a comment
There was a problem hiding this comment.
Holding this PR until new SDK release
| }), | ||
| ); | ||
| this.store.dispatch( | ||
| fetchOrigDatablocksCountAction({ |
There was a problem hiding this comment.
We can avoid fetching the count at each pagination change, only needed at first time.
There was a problem hiding this comment.
Then latest count change will be applied only on page reloading, not the page change.
But maybe it's more reasonable to update count only when page is reloaded?
There was a problem hiding this comment.
I think it is a good compromise to update page count only when page is reloaded
| break; | ||
| case TAB.datafiles: | ||
| { | ||
| this.store.dispatch( |
There was a problem hiding this comment.
fetching here without specifying the pagination parameters would result in using the default {skip: 0, limit:5} which is inconsistent with what we have in datafiles component {limit:10}. I suggest specifying these here.
Also I suggest we go for 25 for consistency through all tables
There was a problem hiding this comment.
I agree. Let's make it consistent and use 25 as default
There was a problem hiding this comment.
set default limit to 25 for both related datasets and data files in datasets component
Description
This PR replaces old tables with dynamic-mat-table in
datafiles.componentandrelated-datasets.componentand removes column header icons to keep table headers consistent.Datafiles table:
Related Datasets table:
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
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
Refactor dataset-related tables to use dynamic-mat-table with centralized pagination and selection handling while aligning table headers and visuals.
New Features:
Bug Fixes:
Enhancements: