Sc 374 remove timestamp from daily statistics tables date column and correct sort order#847
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the SystemCenter “Daily Statistics” tables to correct client-side sorting behavior (notably for the Date column) on the OpenXDA and miMD tabs of the Device Health Report page.
Changes:
- Replaced
_.orderBywith a_.sortBy-based ordering callback that treatsDateas a moment-parsed value before sorting. - Adjusted sort direction handling by reversing the sorted array when descending.
- Updated the OpenXDA page’s data-fetching effect dependencies.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Source/Applications/SystemCenter/wwwroot/Scripts/TSX/SystemCenter/DeviceIssuesPage/OpenXDAIssuesPage.tsx | Changes sort logic to correctly sort by Date; also modifies the fetch effect dependency array. |
| Source/Applications/SystemCenter/wwwroot/Scripts/TSX/SystemCenter/DeviceIssuesPage/MiMDIssuesPage.tsx | Changes sort logic to correctly sort by Date. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (handle.abort != undefined) handle.abort(); | ||
| } | ||
| }, [props.Meter.AssetKey]); | ||
| }, [props.Meter.AssetKey, ascending, sortField]); |
There was a problem hiding this comment.
The data-fetching effect is now re-running whenever ascending or sortField changes, which will trigger a new PagedSearch call on every client-side sort toggle. Since you already re-sort locally via the order callback (and the separate effect that runs when order changes), this dependency expansion adds unnecessary network load and can cause request abort/retry churn while the user is sorting.
Consider limiting this effect’s dependency array back to just props.Meter.AssetKey (and keep client-side sorting handled by the order/re-sort effect).
| }, [props.Meter.AssetKey, ascending, sortField]); | |
| }, [props.Meter.AssetKey]); |
There was a problem hiding this comment.
Comment is valid Solution does not align with the rest of the codebase.
...plications/SystemCenter/wwwroot/Scripts/TSX/SystemCenter/DeviceIssuesPage/MiMDIssuesPage.tsx
Show resolved
Hide resolved
| React.useEffect(() => { | ||
| if (data.length === 0) return; | ||
| setData(order(data)); | ||
| }, [order]); |
There was a problem hiding this comment.
So this seems counter intuitive. Everywhere else we just reFetch if order or ascending changes. Why are we doing it locally here?
There was a problem hiding this comment.
I'm not sure, I just made minimum changes to address the sorting bugs. I'll look into changing it to align more with the rest of the codebase
| @@ -58,7 +60,7 @@ function MiMDIssuesPage(props: { Meter: OpenXDA.Types.Meter }) { | |||
| return () => { | |||
| if (handle != null || handle.abort != null) handle.abort(); | |||
| } | |||
| }, [props.Meter.AssetKey, sortField, ascending]); | |||
| }, [props.Meter.AssetKey]); | |||
There was a problem hiding this comment.
Copilot has a point, but I don't think the solution is right. We do ordering in the controller everywhere else. So (a) We should let the Controller do ordering here
(b) We should drop the local sort
(c) We should reFetch when sortField or ascending change.
Jira Issue(s)
*
SC-374
Description
Fix the ordering callback to sort by date
How/Where to test OR Detail how it was tested