feat: up-879: Datasets improvements fixed #411
Conversation
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
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideSorts GA4GH DRS objects by last-updated timestamp and updates the datasets table layout to show created/last-updated timestamps with adjusted column widths and skeleton placeholders. Sequence diagram for loading and sorting GA4GH DRS objectssequenceDiagram
actor User
participant ECCClientGa4ghDrsObjects
participant DrsApi
User->>ECCClientGa4ghDrsObjects: triggerLoadData
activate ECCClientGa4ghDrsObjects
ECCClientGa4ghDrsObjects->>DrsApi: fetchObjects(pageSize, currentPage - 1)
activate DrsApi
DrsApi-->>ECCClientGa4ghDrsObjects: objectsResult
deactivate DrsApi
ECCClientGa4ghDrsObjects->>ECCClientGa4ghDrsObjects: sortObjectsByLastUpdated(objectsResult.objects)
ECCClientGa4ghDrsObjects->>ECCClientGa4ghDrsObjects: update objects and pagination
ECCClientGa4ghDrsObjects-->>User: renderTableWithSortedTimestamps
deactivate ECCClientGa4ghDrsObjects
Updated class diagram for ECCClientGa4ghDrsObjects componentclassDiagram
class ECCClientGa4ghDrsObjects {
+objects DrsObject[]
+pageSize number
+currentPage number
+loadData() void
+render() unknown
+renderPagination() unknown
+handleObjectSelect(objectId string) void
+bytesToSize(bytes number) string
+formatDateTime(dateString string) string
+sortObjectsByLastUpdated(objects DrsObject[]) DrsObject[]
}
class DrsObject {
+id string
+name string
+size number
+created_time string
+updated_time string
}
ECCClientGa4ghDrsObjects "*" --> "*" DrsObject : displays
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
sortObjectsByLastUpdated, consider explicitly handling cases where bothupdated_timeandcreated_timeare missing to avoid callingnew Date(undefined)and getting unexpected ordering orNaNvalues. - The new
formatDateTimehelper hardcodes anen-US24‑hour format; if this component is used in different locales, it may be better to accept a locale/format option or use the runtime’s default locale for consistency with the rest of the app. - The table column previously used a slot for actions (
actions-${object.id}) but now showsLast Updatedinstead; if callers currently rely on that slot for interactive controls, consider either preserving the slot in another column or documenting/removing that extension point more deliberately.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `sortObjectsByLastUpdated`, consider explicitly handling cases where both `updated_time` and `created_time` are missing to avoid calling `new Date(undefined)` and getting unexpected ordering or `NaN` values.
- The new `formatDateTime` helper hardcodes an `en-US` 24‑hour format; if this component is used in different locales, it may be better to accept a locale/format option or use the runtime’s default locale for consistency with the rest of the app.
- The table column previously used a slot for actions (`actions-${object.id}`) but now shows `Last Updated` instead; if callers currently rely on that slot for interactive controls, consider either preserving the slot in another column or documenting/removing that extension point more deliberately.
## Individual Comments
### Comment 1
<location> `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:171-178` </location>
<code_context>
this.loadData();
}
+ private static sortObjectsByLastUpdated(objects: DrsObject[]): DrsObject[] {
+ return [...objects].sort((a, b) => {
+ // Use updated_time if available, otherwise fall back to created_time
+ const aTime = a.updated_time || a.created_time;
+ const bTime = b.updated_time || b.created_time;
+
+ // Sort in reverse chronological order (most recent first)
+ return new Date(bTime).getTime() - new Date(aTime).getTime();
+ });
+ }
</code_context>
<issue_to_address>
**issue:** Consider handling missing/invalid timestamps more defensively in the sort comparator.
If `updated_time`/`created_time` is missing or not a valid date, `new Date(...).getTime()` returns `NaN`, and comparisons with `NaN` can produce non‑deterministic ordering. Consider explicitly handling falsy/invalid timestamps (e.g., normalizing them to a sentinel like `0`/`-Infinity` or special‑casing when `aTime`/`bTime` is invalid) so those entries are consistently placed at the start or end of the list.
</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
171
to
178
| private static sortObjectsByLastUpdated(objects: DrsObject[]): DrsObject[] { | ||
| return [...objects].sort((a, b) => { | ||
| // Use updated_time if available, otherwise fall back to created_time | ||
| const aTime = a.updated_time || a.created_time; | ||
| const bTime = b.updated_time || b.created_time; | ||
|
|
||
| // Sort in reverse chronological order (most recent first) | ||
| return new Date(bTime).getTime() - new Date(aTime).getTime(); |
Contributor
There was a problem hiding this comment.
issue: Consider handling missing/invalid timestamps more defensively in the sort comparator.
If updated_time/created_time is missing or not a valid date, new Date(...).getTime() returns NaN, and comparisons with NaN can produce non‑deterministic ordering. Consider explicitly handling falsy/invalid timestamps (e.g., normalizing them to a sentinel like 0/-Infinity or special‑casing when aTime/bTime is invalid) so those entries are consistently placed at the start or end of the list.
keshxvdayal
changed the title
keshxvdayal
changed the title
anuragxxd
requested changes
Dec 21, 2025
Comment on lines
102
to
104
| this.objects = ECCClientGa4ghDrsObjects.sortObjectsByLastUpdated( | ||
| result.objects | ||
| ); |
Member
There was a problem hiding this comment.
This is incorrect as the sorting should be handled on the backend.
Comment on lines
175
to
187
| private static sortObjectsByLastUpdated(objects: DrsObject[]): DrsObject[] { | ||
| return [...objects].sort((a, b) => { | ||
| const aTime = a.updated_time || a.created_time; | ||
| const bTime = b.updated_time || b.created_time; | ||
|
|
||
| if (!aTime && !bTime) return 0; | ||
| if (!aTime) return 1; | ||
| if (!bTime) return -1; | ||
|
|
||
| return new Date(bTime).getTime() - new Date(aTime).getTime(); | ||
| }); | ||
| } | ||
|
|
Comment on lines
+363
to
379
| private static formatDateTime(dateString: string): string { | ||
| try { | ||
| return new Date(dateString).toLocaleDateString(); | ||
| if (!dateString) return "—"; | ||
|
|
||
| return new Date(dateString).toLocaleString(undefined, { | ||
| year: "numeric", | ||
| month: "2-digit", | ||
| day: "2-digit", | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| second: "2-digit", | ||
| hour12: false, | ||
| }); | ||
| } catch { | ||
| return dateString; | ||
| return dateString || "—"; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Maybe we can open the slot rather than controlling this functionality. Like what is done with the title.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Fixes #(issue)
Checklist
Comments
Summary by Sourcery
Sort GA4GH DRS objects by most recently updated and enrich the objects list table with explicit created/last-updated timestamps.
New Features:
Enhancements: