-
Notifications
You must be signed in to change notification settings - Fork 33
feat: replace instruments table with the new dynamic material table #1793
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
base: master
Are you sure you want to change the base?
feat: replace instruments table with the new dynamic material table #1793
Conversation
…com/SciCatProject/frontend into SWAP-4585-replace-instruments-table
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 @martin-trajanovski - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the table configuration logic into a separate service or utility function to improve readability and maintainability.
- The component now fetches data on every query parameter change; consider debouncing or throttling these requests to avoid excessive API calls.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
const { pageIndex: page, pageSize: limit } = event; | ||
this.store.dispatch(changePageAction({ page, limit })); | ||
ngOnInit(): void { | ||
this.subscriptions.push( |
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.
suggestion: Clarify the pending state management in multiple subscriptions.
Both the instruments data subscription and the queryParams subscription set 'this.pending'. It might be prudent to separate concerns or better coordinate the pending state updates to avoid potential race conditions or inconsistent UI states.
Suggested implementation:
) {}
// New pending flags for separate subscriptions
private instrumentPending: boolean = true;
private queryPending: boolean = true;
// Helper method to update the overall pending state
private updatePendingState(): void {
this.pending = this.instrumentPending || this.queryPending;
}
ngOnInit(): void {
this.subscriptions.push(
this.instrumentsWithCountAndTableSettings$.subscribe(
({ instruments, count, tablesSettings }) => {
this.tablesSettings = tablesSettings;
this.dataSource.next(instruments);
this.instrumentPending = false;
this.updatePendingState();
You will need to update any other subscription (for example, the queryParams subscription) that currently sets this.pending:
• Replace its direct assignment (e.g. this.pending = false) with setting its own flag (e.g. this.queryPending = false) followed by a call to updatePendingState().
This will separate the pending state management for different subscriptions, avoiding potential race conditions or inconsistent UI states.
customMetadata: this.jsonHeadPipe.transform(instrument.customMetadata), | ||
})), | ||
})), | ||
export class InstrumentsDashboardComponent implements OnInit, OnDestroy { |
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.
issue (complexity): Consider refactoring the component to use RxJS's takeUntil
for subscription management and move table configuration logic to a dedicated service or utility to reduce its responsibilities and improve focus.
Consider refactoring to reduce component responsibilities:
1. **Simplify Subscription Management:**
Replace the manual subscriptions array with RxJS’s `takeUntil`. For example, add a private subject and pipe your observables with `takeUntil(this.unsubscribe$)`:
```typescript
private unsubscribe$ = new Subject<void>();
ngOnInit(): void {
this.instrumentsWithCountAndTableSettings$
.pipe(takeUntil(this.unsubscribe$))
.subscribe(({ instruments, count, tablesSettings }) => {
// ... existing logic
});
this.route.queryParams
.pipe(takeUntil(this.unsubscribe$))
.subscribe(queryParams => {
// ... existing query param handling
});
}
ngOnDestroy(): void {
this.unsubscribe$.next();
this.unsubscribe$.complete();
}
-
Extract Table Configuration Logic:
Move helper methods likegetTableSort
,getTablePaginationConfig
, andinitTable
into a dedicated service or utility. For instance:// table-config.service.ts @Injectable({ providedIn: 'root' }) export class TableConfigService { getTableSort(route: ActivatedRoute): ITableSetting["tableSort"] { const { queryParams } = route.snapshot; return queryParams.sortDirection && queryParams.sortColumn ? { sortColumn: queryParams.sortColumn, sortDirection: queryParams.sortDirection } : null; } getPaginationConfig(route: ActivatedRoute, dataCount: number, defaultPageSize: number, defaultOptions: number[]): TablePagination { const { queryParams } = route.snapshot; return { pageSizeOptions: defaultOptions, pageIndex: queryParams.pageIndex, pageSize: queryParams.pageSize || defaultPageSize, length: dataCount, }; } }
Then, inject and use it in your component:
constructor( private store: Store, private router: Router, private route: ActivatedRoute, private tableConfigService: TableConfigService ) {} ngOnInit(): void { // ... const tableSort = this.tableConfigService.getTableSort(this.route); const paginationConfig = this.tableConfigService.getPaginationConfig(this.route, count, this.defaultPageSize, this.defaultPageSizeOptions); // ... }
These steps isolate concerns and make the component code more focused.
@@ -195,6 +195,29 @@ Cypress.Commands.add("createProposal", (proposal) => { | |||
}); | |||
}); | |||
}); | |||
|
|||
Cypress.Commands.add("createInstrument", (instrument) => { |
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.
issue (complexity): Consider flattening the nested .then
calls in createInstrument
and removeInstruments
by returning promises to improve readability and maintainability of the code..
Actionable Comment:
The additional nested .then()
calls can be flattened by returning each promise to chain them instead of nesting callbacks. For example, refactor createInstrument
as follows:
Cypress.Commands.add("createInstrument", (instrument) => {
return cy.getCookie("user")
.then((userCookie) => {
const user = JSON.parse(decodeURIComponent(userCookie.value));
return user; // pass the user along
})
.then((user) =>
cy.getToken().then((token) => ({ user, token }))
)
.then(({ user, token }) => {
cy.log("Instrument: " + JSON.stringify(instrument, null, 2));
cy.log("User: " + JSON.stringify(user, null, 2));
return cy.request({
method: "POST",
url: lbBaseUrl + "/Instruments",
headers: {
Authorization: token,
Accept: "application/json",
"Content-Type": "application/json",
},
body: instrument,
});
});
});
Similarly, in removeInstruments
, avoid nesting by returning promises:
Cypress.Commands.add("removeInstruments", () => {
cy.login(Cypress.env("username"), Cypress.env("password"));
return cy.getToken()
.then((token) =>
cy.request({
method: "GET",
url: lbBaseUrl + "/instruments",
headers: {
Authorization: token,
Accept: "application/json",
"Content-Type": "application/json",
},
})
)
.its("body")
.then((instruments) => {
cy.login(
Cypress.env("secondaryUsername"),
Cypress.env("secondaryPassword")
);
return cy.getToken()
.then((token) => {
// Use Cypress._.each if you want to maintain chainability
return Cypress._.each(instruments, (instrument) => {
cy.request({
method: "DELETE",
url: lbBaseUrl + "/instruments/" + encodeURIComponent(instrument.pid),
headers: {
Authorization: token,
Accept: "application/json",
"Content-Type": "application/json",
},
});
});
});
});
});
Steps to Reduce Complexity:
- Return Promises: Ensure each
.then()
returns a promise so that you can chain further operations rather than nesting. - Flatten Callbacks: Combine sequential asynchronous operations into a single chain.
- Optional: Consider using helper functions for repeated logic (like setting headers).
These changes keep the functionality intact while making the code easier to read and maintain.
const { name, display, index, width } = column; | ||
|
||
return { name, display, index, width }; |
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.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable
)
const { name, display, index, width } = column; | |
return { name, display, index, width }; | |
return column; | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
Description
Replaces the instruments table with the new dynamic material table component
Motivation
All the existing tables should be replaced by the new dynamic material table for better performance, flexibility and alignment.
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
Replaces the instruments table with the new dynamic material table component, and refactors the proposal dashboard to use the new dynamic material table component.
Enhancements:
Summary by Sourcery
Replace the existing instruments table with a new dynamic material table component, improving performance, flexibility, and user experience
New Features:
Enhancements:
Tests: