Allows sorting Audiobooks on added date - #516
Conversation
| seriesNumber?: string | ||
| seriesMemberships?: AudiobookSeriesMembership[] | ||
| description?: string | ||
| added: Date |
There was a problem hiding this comment.
While this is a full UTC timestamp the JSON never delivers this as a date, it should be typed as a string.
There was a problem hiding this comment.
I'm not sure I follow what you mean, we expect it to be a Date fetched from the API, the transfer/conversion logic should not determine what data type we want to use right ?
There was a problem hiding this comment.
The backend value is a DateTime, but once it comes through JSON the frontend receives a plain string, not a JavaScript Date. Nothing is converting it back automatically.
So added: Date is misleading right now because TS would allow a.added.getTime(), but that would fail at runtime because it's a string. Unless we add a mapping step that converts added with new Date(...), the accurate API type should be added: string.
There was a problem hiding this comment.
Addressed. The frontend transport contract is now added?: string | null, matching the JSON payload rather than pretending it is a JavaScript Date. Consumers explicitly parse the serialized UTC string where timestamp semantics are needed.
| table: "Audiobooks", | ||
| type: "TEXT", | ||
| nullable: false, | ||
| defaultValue: new DateTime(2026, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified)); |
There was a problem hiding this comment.
Shouldn't we use DateTimeKing.Utc? It's what we use elsewhere. Also why 2026-1-1 as the default for existing rows? Why not DateTime.MinValue?
There was a problem hiding this comment.
The reasoning was the following:
- When a user enters a filtering date through the date picker, it will be relative to his timezone thus comparing to UTC could lead to strange behavior. We can fix that by converting the inputed date to UTC based on user known (?) timezone probably
- The default here is to give something in the past (relative to the date this will be merged/released that is not too absurd for the user (should we want to display that added date somewhere in the future) so filtering will work as expected for the user but we can definitely go for DateTime.MinValue if you prefer, non tech people seeing 1970-01-01 will probably freak out comparing to seeing 2026-01-01
There was a problem hiding this comment.
We can fix that by converting the inputed date to UTC based on user known (?) timezone probably
Turns out the browser already handles timezone when working with date selector!
There was a problem hiding this comment.
The main thing I’m trying to avoid is introducing an Unspecified DateTime into a field that otherwise behaves like a UTC timestamp. If we keep 2026-01-01 as the backfill/default for existing rows, I think it should still be:
new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc)
I’m less attached to MinValue specifically. The important part is that whatever sentinel/default we choose should be UTC and intentional. DateTimeKind.Unspecified is the bit that can create inconsistent serialization/comparison behavior later.
There was a problem hiding this comment.
Addressed by removing the fabricated sentinel entirely. The regenerated EF migration adds a nullable Added column with no default. After migration, startup repair backfills legacy rows only from durable evidence: earliest Added history first, then earliest AudiobookFile.CreatedAt as fallback. Rows with no evidence remain null, so we never invent a date. Known values are normalized back to UTC on SQLite materialization.
| av = new Date(a.added).getTime().toString() | ||
| bv = new Date(b.added).getTime().toString() |
There was a problem hiding this comment.
Converting it to string is going to break the comparison. Like I said: this will not sort as expected because it's trying to sort literal string like "Fri Apr 17 2026" and "Mon Apr 27 2026" which will not sort as expected.
We need to change this to:
av = new Date(a.added).getTime()
bv = new Date(b.added).getTime()
Then later before the return add a new conditional, like line 1109:
if (typeof av === 'number' && typeof bv === 'number') {
return (av - bv) * (sortOrder.value === 'asc' ? 1 : -1)
}
There was a problem hiding this comment.
Addressed. Date Added sorting now parses the serialized timestamp with Date.parse() and compares the numeric epoch values. Missing or invalid legacy dates are kept last in both directions, and selecting Date Added defaults to descending/newest-first.
| seriesNumber?: string | ||
| seriesMemberships?: AudiobookSeriesMembership[] | ||
| description?: string | ||
| added: Date |
There was a problem hiding this comment.
The backend value is a DateTime, but once it comes through JSON the frontend receives a plain string, not a JavaScript Date. Nothing is converting it back automatically.
So added: Date is misleading right now because TS would allow a.added.getTime(), but that would fail at runtime because it's a string. Unless we add a mapping step that converts added with new Date(...), the accurate API type should be added: string.
| table: "Audiobooks", | ||
| type: "TEXT", | ||
| nullable: false, | ||
| defaultValue: new DateTime(2026, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified)); |
There was a problem hiding this comment.
The main thing I’m trying to avoid is introducing an Unspecified DateTime into a field that otherwise behaves like a UTC timestamp. If we keep 2026-01-01 as the backfill/default for existing rows, I think it should still be:
new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc)
I’m less attached to MinValue specifically. The important part is that whatever sentinel/default we choose should be UTC and intentional. DateTimeKind.Unspecified is the bit that can create inconsistent serialization/comparison behavior later.
|
Is this still on your radar @T4g1? |
|
Yes, its on hold but I can update it when you think we should merge it |
ae132e2 to
0859c66
Compare
|
Superseded by #841. I recreated this work on a maintainer-owned branch so the remaining hardening and review fixes can be landed directly without depending on contributor-fork write access. The replacement PR explicitly credits @T4g1 for the original feature/implementation and preserves the current #516 code state as its starting point. Closing this PR as superseded rather than rejected. |
Summary
Requested in #513
Allows user to sort audiobooks on added date and create custom filters for it too
Changes
Added
Testing
Manualy tested