fix: regression: sort day by fileCreatedAt again #18732
Conversation
server/src/dtos/time-bucket.dto.ts
Outdated
| @ApiProperty({ type: 'array', items: { type: 'string', nullable: true } }) | ||
| fileCreatedAtTimeZone!: string[]; |
There was a problem hiding this comment.
Why do we need the time zone? I thought the plan was fileCreatedAt + local day?
There was a problem hiding this comment.
As it turns out, local day is a little problematic. The asset-store actually has multiple data sources, 1) loading time-buckets from the /api - but also, 2) websocket updates, and 3) manually updates calling addAsset or updateAssets, or updateAssetOperation (a result from some ui-actions)
Since loading assets or updating assets in this way do not use a TimeBucketAssetResponseDto - we can't really use dayGroup. In fact, we could probably just drop dayGroup from the TimeBucketAssetResponseDto - kept it in for now, since @zackpollard specifically requested it. Not sure if based on this new info would you change your mind?
The new technique is to send timeZone with fileCreatedDate. With fileCreatedDate always being UTC, it can be used directly for sorting. With the TZ info, we can derive localDateTime from the UTC by converting the UTC back to tz-relative date, and then dropping the TZ. (TimelineAsset dosn't store tz in its plainDateFormats anyways) this derived localDateTime is used for grouping days/months - and for the day's formatted date string (title/heading). the timeZone is only a little bit bigger than the day (which is a plain number), and frequently repeated, so with compression, it should also not be a big deal for time-bucket api response
There was a problem hiding this comment.
The timeZone can be in one of many formats, including longer ones like America/New_York. It's a little unfortunate if we need to include it. It definitely shouldn't be in addition to the dayGroup.
There was a problem hiding this comment.
Latest approach removes need for dayGroup and timeZone, instead, we calculate the utcOffset (in minutes) server side, in the db, and send a up to 3 digit number (as a json-number) over the wire. Client side, we apply the offset to get back to a full localDateTime. Ideally, this should perform better than doing the date/timezone transformations in the db. The db is limited to doing a trivial arithmetic operations. The on-the-wire size minimal (comparable to dayGroup) and its easy to derive client-side. Additionally, it gives us the full localDateTime.
|
Deploying preview environment to https://pr-18732.preview.internal.immich.cloud/ |
|
Tested and confirmed the ordering issue. has been fixed |
|
@midzelis Can you help resolve the conflict of the PR? @mertalev @zackpollard can you help with a review and stamp if they look good from your end? |
- Resolved conflict in assets-store.svelte.ts: kept HEAD refactor and added missing isSelectingAllAssets export - Resolved conflict in asset-factory.ts: kept HEAD imports for TimelineAsset and fromISODateTimeUTCToObject
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| )::real / 3600 as "localOffsetHours", | ||
| "assets"."ownerId", | ||
| "assets"."status", | ||
| "assets"."fileCreatedAt", |
There was a problem hiding this comment.
If you add at time zone 'utc', it will remove the +00:00 offset in these timestamps. It's a nice improvement if the client can handle that
There was a problem hiding this comment.
Nice try, but web specs are kinda messed up. ...
When the time zone offset is absent, date-only forms are interpreted as a UTC time and date-time forms are interpreted as a local time. The interpretation as a UTC time is due to a historical spec error that was not consistent with ISO 8601 but could not be changed due to web compatibility. See Broken Parser – A Web Reality Issue.
Server can't know what the users's browser's tz is set to, so even if we wanted to, server could not send a version of a date-time string without a zone in a way that would be consistent for the client.
There was a problem hiding this comment.
DateTime can interpret it as UTC though, right?
There was a problem hiding this comment.
Yes, it should. I'll update
server/src/dtos/time-bucket.dto.ts
Outdated
| @ApiProperty({ | ||
| type: 'string', | ||
| format: 'uuid', | ||
| required: false, | ||
| description: 'Filter assets by specific user ID', | ||
| }) |
There was a problem hiding this comment.
It's a bit annoying that you repeat a lot of stuff here that's already in the other decorators. I thought you can just add a comment above the field and it'll automatically turn it into a description? So like
// Filter assets by specific user Id
@ValidateUUID({ optional: true })
userId?: string;There was a problem hiding this comment.
Latest commit pushes the descriptions into the existing decorator (and the changes needed in the custom decorators)
Adding comments above the decorator did not work, for either the custom decorators, or in the case of the response dtos that did not have any decorator.
The commit represents the minimal changes needed to add descriptions to all the relevant timeline dtos.
* fix: regression: sort day by fileCreatedAt again * lint * e2e test * inline function * e2e * Address comments. Drop dayGroup and timezone in favor of localOffsetMinutes * lint and some api-doc * lint, more api-doc * format * Move minutes to fractional hours * make sql * merge/conflict * merge fallout, review comments * spelling * drop offset from returned date * move description into decorator where possible, regen api
Description
Recent change introduced caused regression - days were sorted by localDateTime instead of fileCreatedAt. This change fixes that regression.
docs and new tests in diff PR
Fixes # (issue)