Skip to content

fix(spaces): unify shared-space album sort options across web and mobile (#966) - #971

Open
Deeds67 wants to merge 23 commits into
mainfrom
fix/966-space-album-sort-parity
Open

fix(spaces): unify shared-space album sort options across web and mobile (#966)#971
Deeds67 wants to merge 23 commits into
mainfrom
fix/966-space-album-sort-parity

Conversation

@Deeds67

@Deeds67 Deeds67 commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

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

# Option Field Default dir
1 Title albumName Asc
2 Number of items assetCount Desc
3 Date modified updatedAt Desc
4 Date created createdAt Desc
5 Most recent photo endDate Desc
6 Oldest photo startDate Desc
7 Recently linked linkedAt Desc

Mobile 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}/albums already 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.ts and web/src/lib/stores/preferences.store.ts are byte-identical to upstream/main and stay that way. The seventh option lives in a new fork-only web/src/lib/utils/space-album-sort.ts that handles RecentlyLinked itself and delegates the other six to upstream's sortAlbums.

Adding RecentlyLinked to upstream's AlbumSortBy would have been less code but wrong on correctness: AlbumsControls.svelte iterates the same metadata array, so the option would also have appeared on the regular /albums page, where linkedAt does 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

watchLinkedAlbums already LEFT JOINs the assets and GROUP BYs to compute assetCount, 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

  • A latent upstream inconsistency: on an unrecognised sortBy, upstream's sortAlbums falls back to DateModified while findSortOptionMetadata falls back to MostRecentPhoto — so the pill showed one option's name while applying another's order. sortSpaceAlbums resolves through the finder before delegating, so both halves land on RecentlyLinked.
  • A startup-crash hazard: EnumCodec.decode used values.firstWhere(...) with no orElse, and nothing between it and SettingsRepository.ensureInitialized caught the throw — an unrecognised persisted value crashed the app at launch. Adding three new sort modes would have made that reachable on downgrade, so EnumCodec now falls back instead of throwing.

The four pre-existing Dart enum identifiers are deliberately not renamed (only relabelled) — EnumCodec persists value.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-infos and the dart format gate 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 null localDateTime count toward assetCount but yield no date range; bulk-linked albums sharing a linkedAt order deterministically; empty and single-album lists across all seven options and both directions.

Known divergences, deliberately not closed

  • Title collation: web uses locale-aware localeCompare, mobile code-unit toLowerCase() — Dart has no built-in collator.
  • Search scope: web matches album name or description, mobile name only.
  • Photo-date corpus: web's dates are server-computed over all assets, mobile's over locally synced ones.
  • Web's space album table headers remain static and non-clickable, unlike /albums.

The last two are filed as follow-ups.

Deeds67 added 20 commits August 10, 2026 09:48
)

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.
@Deeds67 Deeds67 added the changelog:fix Bug fix for changelog label Aug 10, 2026
…#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.
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Album sorting options in Shared Space are inconsistent between Web and Mobile

1 participant