fix(spaces): unify shared-space album sort options across web and mobile (#966) - #971
Open
Deeds67 wants to merge 23 commits into
Open
fix(spaces): unify shared-space album sort options across web and mobile (#966)#971Deeds67 wants to merge 23 commits into
Deeds67 wants to merge 23 commits into
Conversation
) Points space-albums-list.svelte and space-album-grouping.ts's per-group re-sort at sortSpaceAlbums/SpaceAlbumSortBy from Task 1, dropping the AlbumResponseDto casts those call sites no longer need. Disables Year grouping while sorting by Recently linked (S27), matching the existing Date created / Date modified disabled entries. Year's isDisabled() reads the sortBy off the live store rather than the settings object buildSpaceAlbumGroups is called with. Since the store's default sortBy is now RecentlyLinked (Task 2) and RecentlyLinked now disables Year, a few pre-existing tests in space-album-grouping.spec.ts, space-albums-list.spec.ts and space-albums-controls.spec.ts that grouped by Year without pinning a Year-compatible sortBy on the store started falling back to ungrouped. Updated those tests to set a photo-date sortBy alongside groupBy: Year, matching the pattern already used elsewhere in the same files.
…d sort fix (#966) The existing S10-at-list test exercised MostRecentPhoto, which already passed before space-albums-list.svelte switched from upstream sortAlbums to sortSpaceAlbums — so it didn't cover the actual fix. sortAlbums has no entry for RecentlyLinked and silently falls back to DateModified ordering; add a test with albums whose linkedAt and updatedAt orders disagree, so a DateModified fallback would render the opposite order. Verified by temporarily reverting the component to sortAlbums (with the casts it used to need): the new test failed while the other 16 kept passing, then passed again once sortSpaceAlbums was restored.
CI runs in UTC, so a dropped .toUtc() would still pass every test here and only misbehave on a non-UTC device. Document the constraint so it doesn't get "simplified" away.
…l record (#966) Widening albumSortByNames to Record<string, string> and dropping the AlbumSortBy cast removed the only compile-time check that every SpaceAlbumSortBy value has a label. The next sort option upstream adds would silently render a blank menu row and a blank trigger label. Narrow SpaceAlbumSortOptionMetadata.id (and the label record's key type) to the union of SpaceAlbumSortBy's own values instead, so a missing label is a tsc/svelte-check error again.
…ion (#966) - newRemoteAsset gains an optional Value<DateTime?> localDateTime parameter (defaulting to the prior createdAt.toLocal() behaviour) so fixtures can express an asset with no localDateTime. - Add the missing S13 case: an album whose assets all have a null localDateTime must still report the correct assetCount, with startDate/endDate null exactly like an album with no assets at all. - The S20 cross-space linkedAt test only asserted the two dates differed, which passes even if they were swapped; assert the exact expected dates instead.
- _unknownDateLast's docstring said it returns null "when neither side is missing", omitting that it also returns null when both sides are missing (a tie). That omission described exactly the both-null path that crashed the original prescribed comparator. - _utcDay's "do not remove .toUtc()" comment blamed the column's on-disk text format, but synced rows are parsed by mapDateTime via DateTime.tryParse, which already returns isUtc: true for a Z-suffixed string — .toUtc() is a no-op on those. It is genuinely load-bearing only for locally-constructed, non-UTC DateTimes, such as the medium-test fixture's createdAt.toLocal(). Correct the reason so the "do not remove" warning stays credible.
The existing S25 coverage builds a local EnumCodec rather than going through SettingsKey.spaceAlbumsSortMode, so deleting that key's fallback: argument wouldn't fail any test — it would silently change the fallback to SpaceAlbumSortMode.name with nothing to catch it. Assert directly on SettingsKey.spaceAlbumsSortMode.decode.
…#966) Recently linked is the new default sort, so adding it to the Year-disabled list left Year grouping unreachable out of the box. The spaces-albums e2e suite caught it: its Year test lands on the page with fresh storage and finds the option disabled. Year buckets by photo year and orders within each bucket by the active sort, which reads fine as "2024 albums, most recently linked first", so only the two album-metadata-date sorts keep disabling it.
This was referenced Aug 10, 2026
…974) The Year grouping is disabled while the sort is Date created or Date modified. The menu greyed the option out and the list correctly fell back to flat, but the toolbar trigger kept reading "Group by year" because it resolved the raw stored groupBy instead of the effective one. Resolve it through getSelectedSpaceAlbumGroupOption, which the sibling isGrouped derivation on the next line already uses. Upstream's AlbumsControls has the same defect. It is left alone so the file stays byte-clean for rebases.
Web's album search box matches an album's name OR its description; mobile matched the name only, so a query that hit only a description found the album on web and nothing on mobile. The description is already synced into the local shared_space_album table, it just was not projected onto the SpaceAlbum domain model. Carry it through the watchLinkedAlbums join and widen the album filter to check it. _matches now takes the fields to search rather than a single string, so the space list keeps searching names only. Nulls are skipped, matching web's `(a.description ?? '')`. Everything else about the match is unchanged: case-insensitive, trimmed, literal substring, no diacritic folding.
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
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.
Fixes #966.
The sort options for a shared space's album list differed per platform: web offered six, mobile four. Two of those differences turned out to be cosmetic — mobile's "Name" is web's "Title" (
albumName), and mobile's "Recently updated" is web's "Date modified" (updatedAt). Both platforms now offer the same seven options, in the same order, with the same default.The unified contract
albumNameassetCountupdatedAtcreatedAtendDatestartDatelinkedAtMobile gains Date created / Most recent photo / Oldest photo; web gains Recently linked. Both platforms now default to Recently linked, descending — the most useful opening order for a collaborative surface. Only users who never touched a space-album view setting see the change; anyone with a stored preference keeps it.
No server and no i18n change
GET /shared-spaces/{id}/albumsalready returned every field needed, and all seven label keys already existed in all ten maintained locales. This is a client-only change.Web: a fork-only layer, upstream untouched
web/src/lib/utils/album-utils.tsandweb/src/lib/stores/preferences.store.tsare byte-identical toupstream/mainand stay that way. The seventh option lives in a new fork-onlyweb/src/lib/utils/space-album-sort.tsthat handlesRecentlyLinkeditself and delegates the other six to upstream'ssortAlbums.Adding
RecentlyLinkedto upstream'sAlbumSortBywould have been less code but wrong on correctness:AlbumsControls.svelteiterates the same metadata array, so the option would also have appeared on the regular/albumspage, wherelinkedAtdoes not exist and the sort would silently do nothing.Delegating rather than reimplementing also means a future rebase that changes upstream's comparators changes both surfaces together, and the delegation tests notice if the semantics move.
Mobile: derived from the query that already runs
watchLinkedAlbumsalready LEFT JOINs the assets andGROUP BYs to computeassetCount, so the two photo-date aggregates come from that same statement — no extra round trip, and the stream stays reactive.Photo dates are truncated to a UTC calendar day to match the server's
MIN/MAX(("asset"."localDateTime" AT TIME ZONE 'UTC')::date). Without that, two albums whose newest photos share a UTC day would tie on web but be time-ordered on mobile — a visible ordering difference on the very sorts added for parity.Two bugs found and fixed along the way
sortBy, upstream'ssortAlbumsfalls back toDateModifiedwhilefindSortOptionMetadatafalls back toMostRecentPhoto— so the pill showed one option's name while applying another's order.sortSpaceAlbumsresolves through the finder before delegating, so both halves land onRecentlyLinked.EnumCodec.decodeusedvalues.firstWhere(...)with noorElse, and nothing between it andSettingsRepository.ensureInitializedcaught the throw — an unrecognised persisted value crashed the app at launch. Adding three new sort modes would have made that reachable on downgrade, soEnumCodecnow falls back instead of throwing.The four pre-existing Dart enum identifiers are deliberately not renamed (only relabelled) —
EnumCodecpersistsvalue.name, so a rename would silently reset every user who had that option selected.Testing
Web: 4372 passing. Mobile: 3039 passing.
check:typescript,check:svelte(585 files),eslint --max-warnings 0,dart analyze --fatal-infosand thedart formatgate all clean.The design spec (
docs/superpowers/specs/2026-08-10-space-album-sort-parity-design.md) defines 29 numbered behaviour scenarios with a scenario-to-test map; each has a named home in the suites. Photo-date fixtures are built so the two date sorts disagree at the same direction — a comparator wired to the wrong field fails rather than coincidentally passing — and the same fixture is used on both platforms, which is what makes "parity" a checked claim.Edge cases covered: albums with no photos sort last in both directions (matching upstream's
sortUnknownYearAlbums); assets with a nulllocalDateTimecount towardassetCountbut yield no date range; bulk-linked albums sharing alinkedAtorder deterministically; empty and single-album lists across all seven options and both directions.Known divergences, deliberately not closed
localeCompare, mobile code-unittoLowerCase()— Dart has no built-in collator./albums.The last two are filed as follow-ups.