feat(spaces): nestable folders for space albums - #931
Open
Deeds67 wants to merge 41 commits into
Open
Conversation
DB layer for nestable folders inside Shared Space albums: a new shared_space_album_folder table (self-referencing tree, CASCADE on space/parent deletion) and a nullable shared_space_album.folderId FK (SET NULL on folder deletion). Sibling names are unique case-insensitively via two PG14-compatible partial indexes (root vs nested), since NULLS NOT DISTINCT isn't available until PG15. Also updates scripts/revert-to-immich.sql for the new table/migration so the downgrade path and its regression spec stay in sync.
Kysely CRUD/tree primitives on SharedSpaceRepository for the shared_space_album_folder table added in the previous task: create/get/list/count, recursive-CTE ancestor chain and subtree queries, case-insensitive sibling-name check, rename/reparent, a transactional promote-then-delete, and setting an album link's folder placement. Medium tests appended to shared-space-album-folder.repository.spec.ts cover each primitive plus the cross-space isolation and null-safety edge cases.
Adds moveAlbumFolderChecked to the shared-space repository (per-space advisory-lock-serialized transaction, cycle check via a fresh ancestor read under the lock) and wires it into the service's updateAlbumFolder, which now handles rename, move, and combined rename+move with self-move, ancestor-cycle, depth-cap, and destination name-collision guards ahead of the write.
Combines the reparent and rename into a single UPDATE inside the locked transaction (moveAlbumFolderChecked gains an optional name param) so a combined PATCH can no longer land the row at the destination under its old name and hit the uniqueness index before the rename runs. Namespaces the advisory lock with a two-argument hashtext key so it no longer shares a keyspace with other pg_advisory_lock callers. Replaces the end-to-end C-01 race test's sole coverage with two deterministic mechanism tests that hold the real lock from a second transaction and prove the move blocks (and that a different space's lock does not), since the original race cannot reliably fail on this connection setup. Also fixes the M-04/M-05 it.each test that silently ran the same case twice, and a truthiness check on a nullable id.
Adds SharedSpaceService.setAlbumFolder, gated on space Editor with a space-scoped folder lookup guarding the cross-space placement invariant. Surfaces folderId on the linked-album listing and on shared_space_album sync (payload-omitted regression test) without changing the mobile sync payload.
Tasks 2/4/5 inserted ~200 lines of album-folder methods between hasAlbumLink and its neighbouring gated query in shared-space.repository.ts, pushing the guard's 50-line proximity window past the only visibility marker that had been accidentally satisfying it. hasAlbumLink selects only spaceId from shared_space_album and maps it to a boolean link-existence check; it reads no asset rows, so it has nothing to gate. Paired with the existing hasLibraryLink allowlist entry (same reason, same shape).
linkAlbum takes an optional folderId query param (not a body, to keep the field truly optional through OpenAPI codegen) and places the album in that folder after the link is created. The folder is validated before the link so an invalid folderId cannot leave a half-finished link behind.
…der album linking Regenerated open-api spec, TypeScript SDK, and Dart client for the new optional folderId query param on PUT /shared-spaces/:id/albums/:albumId. linkAlbum gains an optional named parameter in both clients; no body was added and no existing parameter changed shape.
…ry-DTO schema tests Add a regression test for the idempotent re-link path: addAlbum resolving falsy (already linked) must still call setAlbumLinkFolder, proving the placement write sits outside the if(result) side-effect branch. Also add direct schema tests for SharedSpaceAlbumLinkQueryDto alongside the other shared-space param DTOs.
getFolderPreviewAssetIds took the first 4 albums in array order and only sorted those by recency, so a folder whose newest albums appear later in the input array could show stale covers instead of the newest ones. Sort the whole filtered subtree by recency first, then slice. Fixes the U-06 test fixture (the old expectation matched slice-then-sort, which is how the bug passed review) and adds a test where the newest album sits last in the input array, which the old ordering fails.
Task 5 made folderId a required field on SharedSpaceLinkedAlbumDto, which left pnpm check:typescript red across five files whose fixtures predate space album folders. Add folderId: null to the shared factory and to the inline fixtures that still needed it after that — null is the correct default, meaning "at the space root", where every album sat before folders existed.
getFolderContents' non-root branch had no self-reference guard, so a folder with parentId === id (the U-02 case already handled in buildFolderTree) listed itself as its own child -- a folder you could navigate into forever in the UI. buildFolderTree also silently dropped any folder pair or chain forming a cycle without self-reference (e.g. A.parentId = B, B.parentId = A): each finds a "valid" parent in the other, so neither is ever pushed to roots and both vanish from the tree. Add an O(n) reachability sweep from the roots found on the first pass, promoting anything unreached to root and severing it from whatever cyclic parent it was attached to, so a promoted node can't also linger as a child in a structure that would recurse forever.
Implements W-11, W-17, W-20: SpaceAlbumFolderCard (kebab menu as a sibling of the clickable surface, draggable only for editors), SpaceAlbumFolderBreadcrumb (collapses past four levels), SpaceAlbumFolderPickerModal (disables a moved folder's own subtree as an illegal destination), and SpaceAlbumFolderNameModal for create/rename since modalManager.showDialog can only resolve a boolean. Adds the 17 space_album_folder_* i18n keys these consume.
Adds the spec the plan omitted: trim/whitespace-only-resolves-undefined behavior is load-bearing for Task 10's create/rename flow. Also drops the unused SpaceAlbumFolderNameModal import from SpaceAlbumFolderPickerModal.spec.ts — tsc/svelte-check already catch a dangling import on every run, so it bought no safety and was misleading to a reader of that spec's imports.
Integrates the folder tree module and Task 9 components into the existing albums tab: SpaceAlbumsList renders folders above albums (never grouped) and flattens to a tree-wide search when a query is active; SpaceAlbumsTable gains folder rows for List view; the toolbar gains a New folder button; the page owns folder state, ?folder= URL sync (with orphan fallback), the breadcrumb, and folder CRUD; linking/creating an album inside a folder now lands it there in one request.
Fixes 8 review findings and 2 trivia items on the folders-in-albums-tab integration: the breadcrumb now hides during a search instead of misrepresenting where space-wide results come from; search results keep the active sort order and respect the List/Cover view toggle instead of always falling back to cover cards; the list never renders a fully blank pane, and a folders-fetch failure no longer blocks the albums refresh or hides every folder-scoped album; ?folder= is stripped for a space that now has zero folders, not just an unknown one; New folder is reachable from the empty state; +page.svelte and its spec now use $app/state (matching the rest of the codebase) instead of the deprecated $app/stores; and the W-07 test's vacuous DOM assertion is replaced with a real document-order check.
…rumb A scoped re-review of the prior fix round found two small issues in that diff itself. First, W-04's assertion destructured goto's last mock call with a `?? []` fallback before checking its options, so it silently passed even when goto was never called — add an explicit toHaveBeenCalled() guard. Second, foldersUnavailable was driven purely by "did the most recent folders fetch fail", so a transient refetch failure after a prior success (which deliberately keeps the stale-but-usable folders list) still flattened the album list to every album in the space, while the breadcrumb — derived from that same stale list — kept claiming you were inside a specific folder. foldersUnavailable now only fires when there is genuinely no folder data to scope by. Also deduplicates the failure toast when both the albums and folders fetches reject in the same reload.
- R-08: assert the cross-space arrange POST actually succeeded before asserting the created folder is absent from this space's listing - reuse a real otherSpace folder id for the cross-space-400 test instead of a globally-nonexistent UUID - document R-09's accidental but load-bearing name-collision property
Twelve prior tasks deferred `make sql` (now `mise sql`) to avoid repeated touches to the shared dev DB. Runs it now, adding query blocks for the album-folder repository methods from Tasks 2, 4 and 5. Also adds the missing @GenerateSql to moveAlbumFolderChecked, which was the only sibling mutator without one; deleteAlbumFolderPromoting- Children is the working precedent for a transactional method carrying the decorator.
Three defects found during implementation, corrected against what was actually built and verified: - X-02: the album soft-delete trigger does not remove the shared_space_album link row (only audit/tombstone rows and grants); the migration's own restore branch depends on the row surviving. A folder placement therefore survives a trash/restore cycle. - C-01: FOR UPDATE on the moved folder's ancestor chain does not serialise the mutual X->Y / Y->X race, since the two transactions lock disjoint rows. Documents the per-space advisory lock that replaced it. - R-09: NestJS runs Guards before Pipes, so a malformed body 400s from the global ZodValidationPipe before requireRole ever runs. Reworded to what's actually true: the role gate precedes content/business checks, not schema validation. Also moves D-01-D-05 and F-08/F-10 in §9 from the server-unit row to the medium row, since their semantics are proven by the medium P-05 tests and the space-scoped getAlbumFolderById rather than the service, which only delegates.
…e mutation gaps Maps the 23505 unique-violation that deleteAlbumFolderPromotingChildren's promote UPDATE could raise (when a child's name already exists at the destination parent, per spec F-04) to a 400 instead of letting it escape as an unmapped 500. The repository now returns a discriminated 'ok' | 'notfound' | 'conflict' outcome, checked inside the same transaction before the promote write, with a narrow backstop catch on the write itself for the racing-editor case. Also closes three coverage gaps a prior review's mutation testing found: the web canDrop guard in handleDropItem, the server move depth-cap fixture (unpinned at exactly one over the cap), and the repository's nested (non-root) sibling-name collision check. Documents why the three `folderId ?? undefined` call sites are load-bearing rather than stylistic.
The committed shared.space.repository.sql was regenerated against a DB without the new shared_space_album_folder table, recording `rollback` for deleteAlbumFolderPromotingChildren and moveAlbumFolderChecked. With the table present (as it will be on CI, since migrations run first), both transactions complete without error and end in `commit` instead. Statement sequence is unchanged.
… modal specs bits-ui's BodyScrollLock schedules a 24ms `window.setTimeout(resetBodyStyle)` from its onDestroyEffect when the last dialog unmounts. Nothing drained that timer in these two specs, so on CI it landed after vitest tore down the happy-dom environment and threw an unhandled `ReferenceError: document is not defined` — failing the Test Web job even though all assertions passed. Instrumenting window.setTimeout confirmed both specs leave exactly one uncleared 24ms timer pending after testing-library's auto-cleanup, and that the drain leaves none. The picker spec is included because it is exposed identically; it had simply not lost the race yet. Uses the drain already established in ShortcutsModal.spec, RepresentativeFacePickerModal.spec, person-merge-suggestion-modal.spec and global-search.spec. Assertions are unchanged.
…lays The Sort and Group dropdown panels in space-albums-controls.svelte were `absolute … z-10`. Each album card (and, since this branch, each folder card) carries an `absolute inset-e-6 top-6 z-10 opacity-0 group-hover:opacity-100` hover-kebab overlay. Equal z-index means the tie is broken by DOM order, and the album grid comes after the toolbar — so the kebab painted on top of the open dropdown. It is invisible at `opacity: 0` but still hit-tests, so it silently swallowed any click landing on it. The collision is geometry-dependent: the panels are right-anchored, so which menu item overlaps a kebab depends on where the toolbar's trigger button sits. Adding the "New folder" button to the control cluster shifted the right-aligned Group panel roughly 80px left, moving "Group by year" directly onto the second column's kebab hit box. That made the pre-existing z-index tie deterministic rather than latent, and the click never became actionable: spaces-albums.e2e-spec.ts › group by Year renders space-album-group-* headers locator.click: Test timeout of 60000ms exceeded <svg …> from <div class="px-4 pt-4">…</div> subtree intercepts pointer events Fixed in the toolbar rather than the test or the card: a dropdown opened from a toolbar must render above the content it drops onto, at any viewport width and any column count. z-30 matches the dropdown panel in space-card.svelte and clears every in-content overlay on this page. This also closes the same latent hazard on the Sort panel, which only escaped because its first option happened to sit above the grid.
The back button hardcoded /spaces/{id}/albums, dropping the ?folder= param, so
opening an album from inside a folder and going back dumped the user at the space
root and cost them their place in the tree.
The folder is taken from the album's own link row rather than from how the user
arrived: the loader already reads linkedAlbums from the parent layout and those
carry folderId, so this also works on a refresh, a deep link, and a search result
(where search flattens tree-wide and there is no folder context to inherit).
…nges The [spaceId] layout caches linkedAlbums, and each row's folderId is what the album detail page's back button navigates to. Moving an album refreshed only the albums page's own state, so opening a just-moved album and pressing back returned to the folder it used to live in. Deleting a folder has the same effect by a different route — promotion repoints its albums' folderId — so it invalidates too. Folder create, rename and folder-to-folder moves leave every album's folderId untouched and do not need it. Matches the existing unlink/link/create handlers, which already invalidate for this same reason.
Deeds67
added
the
rc
🧪 Release candidate buildLatest RC Images published
How to run this RCIn the directory containing your services:
immich-server:
image: ghcr.io/open-noodle/gallery-server:pr-931-rc.3Then pull and restart: docker compose pull immich-server
docker compose up -dEach push publishes a new numbered tag, so update the image line to move to a newer RC. To roll back, point it at an earlier Previous builds (3)
Last updated Sat, 08 Aug 2026 15:54:13 GMT — every push while the |
* docs: add mobile design spec for space album folders
* docs: fold review findings into the mobile space album folders spec
* docs: add mobile implementation plan for space album folders
* docs: fold review findings into the mobile plan and spec
* feat(server): add shared_space_album_folder audit table and delete trigger
Adds the tombstone table and AFTER-DELETE trigger that let synced clients
learn when a shared-space album folder is deleted (direct delete or
cascade from a shared_space delete). Task 1 of the mobile space-album-folders
feature; the sync entity that consumes this lands in Task 2.
Also updates scripts/revert-to-immich.sql for the new fork table/migration,
required by the existing revert-to-immich.spec.ts guard.
* test(server): add override-parity guards for shared_space_album_folder_delete_audit
Review follow-up on the folder tombstone trigger (Task 1): the new
function+trigger pair had no entry in migration-override-parity.spec.ts /
trigger-override-parity.spec.ts, the DB-less CI guards that catch the
decorator/migration DDL drifting from functions.ts. Added matching entries,
modeled on album_space_asset_delete_audit. Also asserts deletedAt is
populated in the first medium test.
* feat(server): add the space album folder sync entity
* feat(server): include folderId in the album link sync payload
Inverts the web S-01 regression test (which pinned folderId's absence
pending a mobile-facing decision) now that docs/superpowers/specs/
2026-08-05-space-album-folders-mobile-design.md makes that call: mobile
needs an album's placement to render the folder tree, so folderId now
rides the existing SharedSpaceAlbumLinkV1 sync payload as V-06. The
inverted test keeps a guard in both directions — removing the field
again would fail just as loudly as adding it once did.
Also regenerates the OpenAPI spec/SDKs, which picks up the folder sync
entity DTOs from the prior task (never regenerated at the time).
* feat(mobile): add the space album folder Drift table and migrate to v37
Adds SharedSpaceAlbumFolderEntity (spaceId cascade FK, parentId
deliberately without an FK since sync gives no ordering guarantee
between a folder and its parent) and a nullable folderId on the
existing album-link table, bumping the local schema to v37. The
from36To37 step creates the folder table plus its index and rebuilds
the link table via TableMigration; R-07/R-08 lock in that every
existing link row survives the rebuild with folderId null and that the
new table starts empty.
Also adds the folderId field to the SpaceAlbum domain model and a new
SpaceAlbumFolder model, both consumed by later tasks in this plan.
* feat(mobile): handle the space album folder sync stream
* feat(mobile): add the space album folder tree module
* fix(mobile): break buildFolderTree cycles at the root test, harden preview sort
buildFolderTree could return a genuinely cyclic .children object graph for any
folder cycle with no self-reference (mutual A<->B, or a longer N-cycle): none
of the cycle members passed the old root test, so they were left attached as
each other's children, and the after-the-fact promotion pass added them to
roots without clearing those edges. A plain recursive walk over the result
never terminates. Fixed by deciding cycle membership inside the root test
itself, so cycle members become roots at construction time and folders
hanging below a cycle member still nest normally; the now-unreachable
promotion pass was removed.
Also: folderPreviewAlbums now drops albums with no space-visible cover before
sorting/taking (previously it could return an all-blank collage from a folder
full of good covers), and its sort is now fully deterministic via an id
tiebreaker instead of relying on List.sort()'s stability, which degrades past
~32 elements.
* feat(mobile): query space album folders and album placements
Adds SpaceAlbumRepository.watchFolders(spaceId) and projects
link.folderId onto SpaceAlbum so watchLinkedAlbums reports each
album's folder placement (null = space root). No join to the folder
table — an album's folderId may name a folder that has not synced
yet, and that must still resolve to root, not disappear.
* feat(mobile): add space album folder mutations
* feat(mobile): add the space album folder card and picker
Adds SpaceAlbumFolderCard (folder tile for the space-albums grid, mirroring
the album card's cover/name/count structure) and SpaceAlbumFolderPickerSheet
/ showSpaceAlbumFolderPicker (move-destination picker for folder and album
moves), wiring the Task 6 isDescendant guard so a folder's own subtree is
never offered as a move target.
* test(mobile): pin per-tile slot order in the folder card collage tests
The 1/2/3/4-cover loop only checked that N Thumbnail widgets existed
somewhere, not which album landed in which slot -- a review demonstrated
that duplicating previewAlbums[0] into every tile slot left all tests
green. Assert verifyInOrder over the mocked AssetService.getRemoteAsset
calls instead, which is order-sensitive across slots and so also catches
a plain two-tile swap, not just a duplicate.
Also retitles a test whose "U-12" label and description contradicted its
own (correct) assertion, and underscore-prefixes the file-private
fixture helpers to match the sibling space_albums_shelf_test.dart.
* feat(mobile): browse space album folders on the albums page
Wires the folder tree (Tasks 6-9) into SpaceAlbumsPage: an optional
folderId route param lets the page recurse into itself as folders are
tapped, folder cards render above album cards at the current level,
search flattens tree-wide and hides folders, and the page reactively
pops if the folder it's browsing disappears from sync.
* fix(mobile): wire folder create/rename/move/delete and restore the empty-space search guard
Task 9's folder card already declared onRename/onMove/onDelete, but the page
only ever passed onTap, so every ⋮ item on an editor's folder card was a
no-op and Task 8's whole folder-mutation layer had no callers. Wires the
app-bar "New folder" action (creates in the current folder, not always the
root) and the card's Rename / Move to folder… / Delete, reusing the same
picker and picked-vs-folderId==null distinction as the existing album move.
Also restores the pre-folder-tree guard that a genuinely empty space takes
priority over the search no-match state, scoped to the search branch only so
it doesn't regress the folder-specific empty state.
* fix(web): show an album at the root when its folder is not loaded
* fix(mobile,server): apply final review fixes for space album folders
I-1: re-apply the active sort to tree-wide search results — flattenForSearch
returns raw server order, so a query was silently discarding the user's
chosen sort (mirrors web's space-albums-list.svelte fix).
I-2/M-2/M-4/M-1: add regression-guard test coverage for blind spots the
whole-suite run couldn't catch — a page-level test proving folder cards read
the whole-space subtree rather than just the current level, dispatch/parsing
tests for the three new SharedSpaceAlbumFolder* sync types, a second-space
fixture that would catch an "any space" privacy-gate regression, and a
migration test that seeds non-default values so a wrong columnTransformer
can't hide behind matching defaults.
M-5: map folder-mutation errors to the existing (previously unused on both
web and mobile) space_album_folder_name_taken/depth_exceeded/limit_reached
keys via the server's actual error text, instead of always showing the
generic per-action toast.
M-6a: stream folder rows before album-link rows so the grid doesn't briefly
render every album flat before re-nesting.
* test(mobile): assert the real generic folder-error string, not a nonexistent one
* fix(mobile): let folder drill-down push the space albums route onto itself
DuplicateGuard compares route names only, ignoring args, so tapping a folder
card — which pushes SpaceAlbumsRoute onto SpaceAlbumsRoute with a different
folderId — was silently blocked and nothing happened.
Exempt the one route that is legitimately self-recursive. Making the guard
compare args instead is not viable: SpaceAlbumsRouteArgs carries callbacks, and
closures are never equal across rebuilds, which would disable the guard for
every route that has them.
* style(server): format the space album folder sync spec
* chore(server): sync generated SQL for the space album folder queries
* feat(mobile): create space albums, and link into the current folder
Adds a "New album" app-bar action to the space albums page: create the album,
then link it to the space at the folder the user is currently viewing — mirroring
web's handleCreateAlbum.
Also fixes the same gap for linking an EXISTING album. onLink was a bare
VoidCallback owned by the space-detail page, which has no idea which folder the
albums page is showing, so linking from inside a folder silently dropped the
album at the space root. It now carries the current folder.
* feat(mobile): show an empty state for a space album with no photos
An empty album opened on a blank screen, which reads as a failed load rather
than an album waiting for photos. Mirrors the main Photos timeline's first-run
state — the polaroid illustration is extracted to a shared widget so the two
stay one design — and offers Add photos to editors only.
* fix(mobile): never let the folder sync type ride the version-gate fallback
sharedSpaceAlbumFoldersV1 was added to the version-gated fallback list
alongside the original five Phase-2B space-album types. Older fork
servers (v5.2.0-v5.2.2, or any server whose /server/features fetch
fails) never declared support for it and reject unknown enum values
with a 400 for the whole /sync/stream request, causing a total sync
outage.
Split the const into _legacySpaceAlbumSyncTypes (the five types safe
for the version-gate fallback) and _spaceAlbumSyncTypes (adds the
folder type, used only when the server explicitly declares support).
A type introduced after capability signalling shipped must join only
the declared-capability list, never the fallback.
* test(mobile): fix the H-05 comment's false "only reachable via declaration" claim
The comment claimed a bare version number can never express "albums
yes, folders no", but the sibling v5.2.0 regression test right above
it proves the version-gate fallback reaches exactly that state for a
pre-declaration server. Reworded to say H-05 exercises the declaration
path directly, without the false exclusivity claim.
* fix(mobile): defer a buried space-albums folder page's self-pop until topmost
context.maybePop() always pops the navigator's topmost route, so when a
buried SpaceAlbumsPage's folder vanishes (folder drill-down pushes this
route onto itself), it wrongly popped the valid topmost page instead of
itself. Surgically removing a buried page's own route in place isn't safe
with auto_route 11.1.0: AutoRoutePage.canUpdate keys on the route name, and
since SpaceAlbumsRoute is deliberately self-recursive, Flutter's page-diff
can't tell stacked instances apart and crashes.
Defer instead: a topmost page whose folder vanishes still pops immediately;
a buried one records a pending flag and self-pops the moment it becomes
topmost, via a listener on the router's navigationHistory (which notifies
on every visible-route change, unlike StackRouter's own notifyListeners).
* fix(mobile): harden the space-albums self-pop against three review gaps
navigationHistory's notifyListeners only fires when the computed UrlState
actually changes, so two stacked SpaceAlbumsRoutes sharing the same
folderId (reachable via double-tap, since the route deliberately omits a
duplicate guard) produced an identical UrlState across the covering pop
and the buried page never heard about it. Add a URL-string-independent
per-frame poll as a safety net alongside the listener.
Also: consolidate both the listener and the poll through one guarded
trySelfPop() that checks-then-clears the pending flag synchronously, so a
second notification arriving before the next rebuild tears the old one
down can never fire a second maybePop() that takes the route below this
page with it. And clear the pending flag when the folder reappears, so a
transient false-vanish emission doesn't cause a stale pop once the page
resurfaces with a valid folder again.
* fix(server): map raced 23505 unique-violations to 400 on album folders
createAlbumFolder, updateAlbumFolder's rename path, and its move path
(moveAlbumFolderChecked) all rely on an optimistic pre-check for folder
name conflicts, which a concurrent request can still race past. The
partial unique indexes then raised Postgres 23505, which escaped as an
unhandled 500 on all three paths instead of the same 400 the pre-check
throws. Add a private helper that maps only code 23505 to that 400 and
rethrows everything else untouched.
* fix(server): narrow updateAlbumFolder's repo dto to name-only
updateAlbumFolder(spaceId, folderId, dto) accepted an optional
parentId, but a parentId write through this method would bypass the
advisory-lock + cycle machinery in moveAlbumFolderChecked, the only
path meant to reparent a folder. The service only ever calls it with
{ name }; narrow the type so the compiler forbids the footgun.
The @GenerateSql params already only pass { name: 'Travel' }, so the
committed SQL doc is unaffected by this compile-time-only change.
* test(server): replace three cannot-fail album-folder tests
A-03 and A-04 in shared-space.service.spec.ts were mocked unit tests
that could not fail: A-03 asserted only that setAlbumLinkFolder was
CALLED twice (true of any non-throwing stub), and A-04 was
byte-identical to A-01's arrange/act/assert. Delete both and add the
real idempotency and per-space-scoping properties as medium tests
against a real database.
C-03 in shared-space-album-folder.repository.spec.ts raced
deleteAlbumFolderPromotingChildren against setAlbumLinkFolder inside a
Promise.allSettled, swallowing both rejections, with its only
assertion sitting in an `if` branch either outcome could make
unreachable. Replace it with two deterministic, sequenced orderings:
placing the link then deleting the folder (repoints to the folder's
parent), and deleting the folder then placing the link (rejects with
a 23503 foreign-key violation).
* test(server): drive the album-folder sync handler through ctx.syncStream
syncSharedSpaceAlbumFoldersV1 had zero coverage of its own — the existing
spec only exercised the SharedSpaceAlbumFolderSync repository methods
directly. Add five handler-level tests covering upsert wire shape,
ack/checkpoint gating, tombstone wire shape plus delete-arm idempotency,
late-joiner backfill, and non-member exclusion.
* test(server): guard that album folders sync before album links
SYNC_TYPES_ORDER's comment says folders must land before links so mobile
never renders an album flat at the root for one frame before it re-nests
once the folder sync catches up. Add a pairwise ordering assertion in the
same style as the existing library-links-before-assets guard.
* test(server): strengthen album-folder sync stream test assertions
Turn the non-member exclusion test into a positive-control (M-4 style):
the streaming user is now a member of an unrelated space with its own
folder, proving the stream would have delivered the excluded space's
folder/tombstone had exclusion been broken, instead of an assertion that
passes vacuously if the arm returns nothing for everyone. Also pin the
backfill arm's payload shape exactly (it is a separate send call site
from the upsert arm and could leak updateId independently), assert the
backfill-complete ack's id component exactly rather than by prefix, and
prove the backfill arm is idempotent once acked.
* test(e2e): close folder RBAC/wiring holes in the space album-folder spec
Extends shared-space-album-folder.e2e-spec.ts with the linkAlbum folderId
query wiring (happy path with a GET-verified body assertion, cross-space
400, non-UUID 400), the PATCH move half (sibling move, null-to-root move,
empty-body 400, combined rename+move), and the PUT album-folder-placement
RBAC matrix (owner happy path verified via GET, viewer 403, non-member 403).
* fix(mobile): block a double-tap push of the same SpaceAlbumsRoute
SpaceAlbumsRoute dropped DuplicateGuard entirely because it's legitimately
self-recursive (folder drill-down pushes it onto itself with a different
folderId) and DuplicateGuard only compares route names. That reintroduced
the double-tap bug the guard existed for: two quick taps on "See all" (or a
folder card) before the push animation starts stacked two identical pages.
Add SpaceAlbumsDuplicateGuard, an args-aware guard scoped to this one route:
it blocks a push whose spaceId + folderId both match the current topmost
route's, while still allowing a different folderId through. auto_route
11.1.0 exposes both the pending and current route's typed args
(resolver.route.args / router.current.args), and the generated
SpaceAlbumsRouteArgs already excludes the route's callback fields from
equality (auto_route_generator filters function-typed params out of
generated == / hashCode), so comparing args here is safe.
* fix(mobile): stop create-album toast from lying when only the space-link fails
createAlbum wrapped album creation and the subsequent space-link in one
try/catch, both mapped to the same "Unable to create album" toast. When
creation succeeded and only the link failed, the album existed (unlinked,
invisible in the space) while the toast claimed creation itself had failed
— and a retry created a duplicate album.
Split the two failure domains into separate try/catch blocks: a creation
failure keeps the existing space_album_error_create toast; a link failure
after a successful creation now shows a new space_album_error_link_after_create
toast that tells the truth (the album was created, but could not be added to
this space). No existing i18n key covered this — the album-link flow's
spaces_linked_albums_error_link ("Failed to link album") doesn't convey that
creation succeeded, which is the whole point of the fix.
* docs(mobile): fix stale double-tap comments and add the guard's spaceId test
Review round 1 on Task 6 found two comments left false by the prior commit's
own guard: the pollNextFrame justification in space_albums.page.dart still
claimed SpaceAlbumsRoute "deliberately omits _duplicateGuard" and cited the
pre-fix router.dart line range, and the U-11 stacked-pages test harness's doc
comment still called an args-aware guard "a separate future task" after this
branch shipped exactly that. Both are rewritten to state what's true now and
why pollNextFrame still matters regardless (the self-pop must not depend on
the guard existing).
Also adds a router test pinning the guard's spaceId half: every existing test
held spaceId fixed and varied folderId, so a mutation dropping the spaceId
comparison would have passed the whole suite. Fixes a misordered import
picked up in the same review pass.
* docs(mobile): fix a third stale double-tap comment and a current/topRoute mixup
The U-11 stacked identical-folderId test's comment still claimed the double-tap
was reachable in production and that SpaceAlbumsRoute omitted its duplicate
guard - both false since SpaceAlbumsDuplicateGuard landed, and the router.dart
line range it cited had since shifted to a different set of routes. Bring it
in line with the two sibling comments a prior round already corrected.
Also fix space_albums.page.dart's isTopmost() comment: StackRouter.current
does not drill into nested child routers (current => currentChild ?? routeData,
scoped to its own controller) - it's topRoute that drills, via
_topMostRouter(...).current. The page comment had this backwards, disagreeing
with space_albums_duplicate_guard.dart's correct description of the same API.
Finally, add a caveat to sync_api.repository.dart's version-gate comment: read
literally, "every future gallery-fork-only request type MUST be gated the same
way" instructs adding new types to the legacy version-gate list - the exact
defect a prior fix removed. Point at the M14 paragraph that actually supersedes
it for types introduced after capability signalling shipped.
* refactor(server): pin the folder name-conflict message to one shared const
The optimistic pre-check (assertNoAlbumFolderNameConflict) and the raced-23505
mapper (withAlbumFolderNameConflictMapped) each hard-coded their own copy of
'A folder with that name already exists here'. Nothing pinned the two
together, and only one of the pre-check's tests asserted the literal - the
other asserted just the exception type, so a reword of the pre-check message
alone would have passed CI while silently diverging from the raced-insert
path's wording and degrading mobile's specific name-taken toast (which
substring-matches 'already exists here') to its generic fallback.
Extract SHARED_SPACE_ALBUM_FOLDER_NAME_CONFLICT_MESSAGE and use it at both
throw sites, with a comment pointing at the mobile match site. Strengthen the
previously exception-type-only test (N-02) to assert the shared const, and
switch the other three literal-string assertions to the same const so no test
can drift from the source of truth either.
…ocales The folders feature landed 24 new EN strings (plus cmdk_open_person_page from #922) that were never translated, leaving the nine fork-maintained locales 26 keys behind en.json. Fill them in, matching each locale's existing terminology (Ordner/Carpeta/Dossier/Cartella/Map/Folder/Папка/文件夹/資料夾) and keeping "Space" untranslated where that locale already does. Plural categories follow CLDR per locale: pl and ru get one/few/many/other, zh keeps a single form. Verified by rendering every plural string through intl-messageformat at n=0,1,2,3,5,11,21,101 for all nine locales. date_format is deliberately left untranslated: it is a Luxon format pattern rather than prose, and a pre-existing gap on main unrelated to this feature.
* docs: design spec for space album multi-select Covers selection of albums and folders (never mixed) on the space albums surface, on web and mobile, with bulk unlink / move-to-folder / timeline toggle and bulk folder move / delete. 31 BDD scenarios and 20 edge cases, a per-layer TDD test plan, and the two safety properties the implementation must hold: bulk endpoints reuse the per-item authorization rather than one space-level check, and bulk folder move delegates to moveAlbumFolderChecked so the advisory-lock and cycle machinery cannot be bypassed. * docs: correct seven defects found reviewing the multi-select spec Verified every claim against the code rather than re-reading the prose. Critical: the spec built its 'a selection can never span two folder levels' guarantee on AppNavigate, but +layout.svelte suppresses that event for same-route transitions and entering a folder only changes ?folder= on the same route. Selection would not have cleared, and scenario S-9 would have failed. Clearing is now three explicit triggers, each separately tested, because a single 'clears on navigation' test passes against the broken design. Also: BulkIdsDto has no min(1), so reusing it contradicted the empty-array 400 requirement; the mobile selection PopScope could veto the folder-vanished self-pop's maybePop and resurrect a known deferred issue; the 'flat list the page already computes' does not exist; search is local state, not URL-backed; and the collapsed-group rule read as contradicting the keep-selected rule. Scenarios 31 -> 34, edge cases 20 -> 23. i18n locale set re-verified: exactly en plus the nine fork-maintained locales carry space_album strings. * docs: implementation plan for space album multi-select 15 TDD tasks across server, contracts, web and mobile. Every task writes the failing test first and captures RED/GREEN; refactor and pinning tasks use mutation evidence instead. Covers all 34 spec scenarios and all 23 edge cases, with a coverage matrix mapping each to its task. Two planning findings are recorded as deviations rather than silently applied: the .max(1000) request cap narrows E-16, and one-activity-row-per-batch costs a new activity type, a feed renderer branch and a plural string in ten locales (planned per spec, following the existing person_merge precedent). * docs: fix six defects found reviewing the multi-select plan Verified the plan's code against the codebase rather than re-reading it. Critical: moveAlbumFolderChecked is a REPOSITORY method taking (spaceId, folderId, newParentId, name?) and RESOLVING with 'ok'|'cycle'| 'notfound' — it takes no auth and never throws. The plan called it as a service method with an auth argument, so an implementer would likely have called the repository directly and skipped the depth guard, the name-conflict pre-check and the outcome mapping that sit above it in the service — exactly the bypass the task exists to prevent. Task 4 now extracts #moveAlbumFolderOrThrow from updateAlbumFolder's move branch and both callers use it. Its cycle test also mocked a rejection, which would have passed against an implementation that ignores the outcome value entirely; it now mocks the resolved outcome, and a new depth test proves the service-level guard runs before the repository is touched. Also: 9 test bodies were empty stubs while the coverage matrix claimed the scenarios were covered; the :id param dto is UUIDParamDto, not SharedSpaceIdParamDto; and the e2e spec uses asBearerAuth(), not a raw Authorization header. * feat(server): add fork-local bulk request dtos for space albums * refactor(server): extract checked cores from the single-item space album paths Pulls the authorization + validation + mutation logic out of unlinkAlbum, updateAlbumLink, and setAlbumFolder into private #unlinkAlbumChecked, #setAlbumTimelineChecked, and #setAlbumFolderChecked methods, leaving the public methods as thin wrappers that add the activity log write and grant reconcile queueing. No behaviour change: the checked cores keep their requireRole/rbac-6 checks so a future bulk caller (Task 3) can loop over them and authorize each item individually, while batching the activity row and reconcile job once per request instead of once per item. * feat(server): add bulk unlink, folder and timeline space album operations * test(server): close 5 mutation-survival gaps in the bulk album spec Fix round 1 from reviewer mutation testing (18 mutations run, 11 survived): - C1 (critical): bulkSetAlbumFolder/bulkSetAlbumTimeline had no proof that authorization and the folder's cross-space guard re-run per item rather than once for the batch. Adds getMember/getAlbumFolderById call-count assertions mirroring the coverage bulkUnlinkAlbums already had. - I2: #runBulk's sequential-not-parallel contract was unpinned — every prior assertion was result-order based and Promise.all preserves order. Adds an in-flight-concurrency tracking test. - I3: bulkUnlinkAlbums's activity/reconcile payloads were only ever asserted on all-success batches, where "succeeded" and the full id list are the same array. Re-asserts both payloads inside the existing partial-failure test. - I4: bulkSetAlbumFolder never asserted which folder id actually reached the repository. Adds toHaveBeenCalledWith assertions mirroring the sibling timeline test. - I5: no test drove #bulkErrorFor's ForbiddenException branch or its catch-all resolve-not-throw behaviour. Adds both. - Minors: a showInTimeline=false case, distinct per-album fixture names so the activity row's albumName is actually pinned, and a length assertion on the "every id fails" test so it isn't vacuously true for an empty result array. Every new assertion was proven against the mutation it targets (applied, captured a verbatim failure, reverted) — see task-3-report.md's "Fix round 1" section. No production code changed. * feat(server): add bulk move and delete for space album folders * docs: carve albums/bulk-unlink out of the blanket 403 rule Implementation surfaced that S-28/S-29's 'any bulk endpoint returns 403' cannot hold for albums/bulk-unlink without narrowing the rbac-6 owner arm, which lets an album's owner revoke a link to their own album even without space membership. The other four endpoints are Editor-only, so a hoisted requireRole is equivalent to per-item checking and gives the cleaner 403. Adds S-29a for the per-item behaviour and records why the endpoint differs. * fix(server): hoist requireRole(Editor) on the two album bulk methods bulkSetAlbumFolder and bulkSetAlbumTimeline had no request-level authorization gate, unlike their folder siblings (bulkMoveAlbumFolders / bulkDeleteAlbumFolders), so a viewer got 200 with per-item no_permission instead of the 403 spec S-28/S-29 require. Add the same hoisted requireRole(Editor) those already use, on top of (not instead of) the per-item check inside #setAlbumFolderChecked / #setAlbumTimelineChecked. bulkUnlinkAlbums is deliberately left alone: its owner arm lets an album's owner revoke a link to their own album without space membership, so it must keep authorizing per item (S-29a). * feat(server): expose bulk space album and folder endpoints Add the five HTTP routes from the multi-select design spec §6.1: POST :id/albums/bulk-unlink -> bulkUnlinkAlbums PUT :id/albums/bulk-folder -> bulkSetAlbumFolder PUT :id/albums/bulk-timeline -> bulkSetAlbumTimeline PUT :id/album-folders/bulk-parent -> bulkMoveAlbumFolders POST :id/album-folders/bulk-delete -> bulkDeleteAlbumFolders All return BulkIdResponseDto[] with 200, including the two POST routes (which need an explicit @httpcode(HttpStatus.OK) since Nest defaults POST to 201). The two album bulk-folder/bulk-timeline PUT routes are declared before PUT :id/albums/:albumId so that param route can't swallow them. e2e coverage (shared-space-album-folder.e2e-spec.ts) proves the full RBAC matrix with real requests: per-item not_found for a vanished album vs validation for a missing/foreign folder (never blurred together), the S-29a per-item authorization on bulk-unlink vs the hoisted 403 on the other four, and the previously-untested folderId: null root-move case for bulk-folder. * docs: correct the unachievable 404-on-unknown-space claim requireMembership throws ForbiddenException, and an unknown spaceId is indistinguishable from one the caller is not a member of, so all five bulk endpoints return 403. Pre-existing behaviour across SharedSpaceController, and the better answer: a 404 would leak which space ids exist. * test(e2e): pin bulk-folder/bulk-timeline response bodies and widen RBAC coverage Fix round 1 findings from independent review: - R-24/R-25 (bulk-folder, bulk-timeline) had no response-body assertion that could fail: R-24 used body.every(...), which is vacuously true on an empty array, and R-25 didn't read the body at all. Both endpoints could silently return 200 [] and every test would still pass. Pin the exact BulkIdResponseDto[] shape on both. - R-32/R-33 (viewer/non-member refused) didn't cover bulk-delete for a non-member, only for a viewer (R-31). Add it to the shared matrix. - R-22 used expect.arrayContaining, which pins neither array length nor request order. Switch to an exact toEqual, matching R-20's pattern and #runBulk's per-item, request-ordered contract. * test(server): cover bulk album folder batching against a real database Medium tests against real Postgres for what mocked unit tests cannot verify: that an earlier item in a bulk folder move genuinely changes what is legal for a later one, that bulk delete promotes children without ever unlinking an album, and that a bulk move's two row changes collapse into a single sync checkpoint advance for a member. * chore: regenerate api clients for the space album bulk endpoints * feat(i18n): add space album multi-select strings across all nine locales * docs: quote albumName in the bulk-unlink activity string The feed renders the line as one flat truncatable text node, so the quotes are the only delimiter between template text and user-controlled data, and the sibling merged-people string already quotes its name in all ten locales. Unquoted, an album named 'Summer 2024 and 3 others' renders as 'Alex unlinked Summer 2024 and 3 others and 2 others'. * fix(i18n): address translation-quality review findings for space album multi-select strings - it: fix ungrammatical hoisted "altri" at n=1 in the bulk-unlink activity line (I-1) - de: replace the ambiguous "Sie werden entfernt" confirm with an unambiguous album-scoped sentence, and align the unlink title/feed verb to the file's established "Verknüpfung aufheben" instead of the coined "entknüpfen" (I-2, I-4) - quote {albumName} with each locale's own marks across all ten locales, matching the sibling spaces_activity_merged_people convention (I-3) - pl: switch selected_count to the invariant "Wybrano: {count}" shape used by filter_sheet_picker_selection_count, avoiding wybrane/wybrany agreement issues - zh_Hans/zh_Hant: fix asymmetric numeral spacing and zh_Hant's mainland-flavoured "加入到" - de: "Zur Zeitleiste" article fix - es/fr: restore the dropped noun in "y # más" / "et # autres" - fr: de-anglicise the folder-delete confirm to match the existing sibling's grammar - nl: de-pleonasm the unlink confirm's second sentence * feat(web): add the space album multi-select manager Self-contained selection state for the space albums page (Task 9 of the multi-select plan). Tracks a never-mixed album/folder selection, anchor-based inclusive ranges (forward and backward), a non-committing preview for shift-drag, and reconciliation against a page's current ids. Knows nothing about grouping, search, or view modes — callers pass the flat visual order. * docs: set the anchor in selectRange, as spec E-7 requires The plan's own selectRange omitted the anchor set that spec E-7 mandates, and its E-7 test asserted only the resulting ids, so the omission was unpinned in both directions. A Shift-click as the first interaction left the anchor null forever: no hover preview, and the next Shift-click selected only the two endpoints. * fix(web): set the range anchor on selectRange and clear stale previews Two gaps found by review: a Shift-click range as the first interaction never set the anchor, so a subsequent preview showed no candidates and a subsequent range silently skipped items between the two clicks (spec E-7). Reconcile also left a stale preview behind when a candidate id disappeared from the page's data. Adds coverage for both, plus for the previously-untested read accessors (has, isCandidate, count) and the empty-selection kind-reset and candidates-clear invariants across toggle, selectRange, reconcile and clear. * feat(web): add space album bulk action helpers Thin wrappers over the five generated bulk SDK functions plus applyBulkResult, which decides what stays selected after a bulk request: exact failures on a partial result, everything on a thrown request (offline/500/network), nothing on full success. * feat(web): add multi-select affordances to the space albums page Wires the Task 9 selection manager into the space albums UI: check-circle affordances on album/folder cards and table rows, click routing (open vs. toggle vs. Shift-range), Shift-hover range preview, a new selection bar, and three independent clearing triggers (currentFolderId change, searchQuery change, AppNavigate) plus Escape. The manager and its wiring live in space-albums-list.svelte rather than +page.svelte, since two of the three clearing triggers are that component's own props. * fix(web): close the collapsed-group, cross-space, and role-downgrade selection gaps Addresses review findings on the space-album multi-select wiring: - Add spaceId as a fourth clearing trigger — switching spaces is a same-route transition, so AppNavigate doesn't fire, and reconcile isn't a reliable backstop for an album linked to both spaces. - Clear the selection when canManage goes false (role downgrade mid-selection), so cards stay openable instead of every click silently toggling an invisible selection. - Clear the Shift-hover preview when Shift is released, not only on the next mouseenter elsewhere. - Add test coverage for the collapsed-group range/reconcile rules (§4.3, E-14), the List-view and search+List-view table wiring, and the table row kebab's stopPropagation guard — all previously unexercised. - Correct two comments that overstated what AppNavigate covers. * test(web): cover the grouped-List SpaceAlbumsTable selection wiring SpaceAlbumsTable renders at three call sites in space-albums-list.svelte, not two — search+List and ungrouped List were covered, but the grouped List branch ({#if isGrouped}) had zero selection-wiring coverage since no existing test set groupBy away from its default None. Adds a test that sets view=List and groupBy=Year, with a positive control (the group header) proving the render actually reached the grouped branch, then exercises the check circle and asserts the selected-row styling. No production code change: the props at this call site are textually identical in shape to the already-tested ungrouped one. * feat(web): wire space album bulk actions and multi-drag Hangs the space-album-multi-select-manager's selection bar buttons onto the real bulk endpoints via space-album-bulk-actions.ts's action wrappers, composing partial-failure re-selection through the manager's reconcile primitive. Widens the folder drag-and-drop payload to carry a whole selection (buildDragPayload) so dragging a selected card moves the batch, and adds the batch-unlink row to the space activity feed. * fix(web): clear a moved-out-of-view selection after a multi-id drag I-1: a multi-id drag-move (folder-card or breadcrumb drop) left the moved items selected once they left the current folder level, since none of the existing clearing triggers observe a data move within the same space. SpaceAlbumsList gains a fifth "Trigger" effect driven by a selectionMoveSignal counter the page bumps once a multi-id move settles, covering both drop targets through the one handler that already serves both. Also folds in three review minors: bulk folder moves now exclude every selected folder (not just none) from the destination picker, a drag payload is filtered down to the canDrop-legal subset before dispatch so an illegal member never rides along, and S-25 (total failure keeps the whole selection) gets a test that actually names and guards it. * fix(web): carry which ids moved, not a bare counter, for I-1's reconcile Round 1's selectionMoveSignal was a plain counter, so Trigger 5 could only clear the whole selection on any settle. That collided with two open questions (a drag failure should stay selected, same as every other bulk action here; an unselected card dragged alone must not disturb an unrelated live selection) and, worse, missed a real bypass: filtering a drag payload down to the canDrop-legal subset can turn a genuinely multi-id drag into a single-item move, which never bumped at all. selectionMove now carries { seq, movedIds } instead. Trigger 5 reconciles against exactly the ids that moved, which is safe to bump unconditionally on every move path (single-item or bulk, drag or the kebab's shared move-to-folder) since an id that was never selected reconciles to a no-op — closing the bypass and both open questions with one mechanism. * fix(web): reconcile the selection when the folder kebab moves a member moveFolder (the kebab-only counterpart to moveAlbumToFolder) was the last remaining path where an item could leave the current folder level while still counted in a live selection: the folder kebab is always in the DOM, only opacity-gated by group-hover, so hovering a card while a selection is live reaches it. A multi-folder selection with one moved via the kebab left the bar counting an invisible folder and offering "Delete folder" against it. Bumps markSelectionMoved([folderId]) after the request succeeds, same placement as every other move path. Also promotes two previously probe-only branches to named, mutation-proved tests: a fully-failed drag keeps the whole selection, and a failed single-item drag keeps its lone selection. * feat(mobile): add the space album selection provider * test(mobile): cover reconcile([]) and same-kind toggle notifications * feat(mobile): add multi-select gestures and selection bar to space albums Long-press an album or folder card to enter selection mode; tap toggles while a selection is active and opens/enters otherwise. The AppBar is replaced by a selection bar (count + kind-gated action icons, wired in Task 15) while selected, and a PopScope exits selection on back before letting a second back pop the page. canPop is gated on the selection being empty so it stays inert on every page with no selection, and the pre-existing folder-vanished self-pop clears the selection first (before scheduling or performing its own pop) so a live selection can never veto it. * fix(mobile): clear space-album selection on space/folder/search change The selection provider is a single global, app-lifetime-scoped state (Task 13), so nothing previously cleared it when the browsing context changed without an actual page pop: a same-shaped router.replaceAll (401 redirect, Android VIEW intent, splash-screen login redirect) can reuse this page's existing Element for a different space, and typing into the search field never pops the page at all. Both left a stale selection bar reading against the wrong space or invisible items. Adds one last-seen-value effect over (spaceId, folderId, searchQuery) that clears the selection on a genuine change, deferred via a microtask since Riverpod forbids mutating provider state while the widget tree is building (flutter_hooks runs useEffect synchronously mid-build, not post-frame). Also strengthens the S-10 test to assert the card's own selected badge rather than only the bar's visibility, and adds folder long-press coverage (selection bar shows folder-only actions, not the album card's). * feat(mobile): wire space album bulk actions Wires the Task 14 selection bar's unlink/move/timeline-toggle/delete action icons to the generated bulk endpoints (bulkUnlinkAlbums, bulkSetAlbumFolder, bulkSetAlbumTimeline, bulkMoveAlbumFolders, bulkDeleteAlbumFolders), via thin repository wrappers and new SpaceAlbumActions.bulkX methods that fold each response into "the ids that failed". Also wires SpaceAlbumSelectionNotifier.reconcile against the live album/folder stream, which had no call site before this task, so an item another member unlinks/deletes drops out of an active selection. * fix(mobile): close the bulk-move exclusion gap and pin the bulk failure contract per-action Review round 1: extends the space-album folder picker's single excludeFolderId to a list so a multi-folder bulk move excludes every selected folder (and its subtree), not just a batch of one - selecting Trips + Archive no longer offers Trips as a destination for the whole batch. Adds partial-failure tests for bulkMove, bulkToggleTimeline, and bulkDeleteFolders (previously only pinned for bulkUnlink), a test proving the bulk payload is sourced from the selection rather than the rendered list, and a test mixing not_found and validation failures to pin that neither is treated as success. Also fixes three doc comments in shared_space_api.repository.dart that named the wrong HTTP verb/path. * style(web): wrap the over-width line in space-activity-feed.spec.ts Line 124 was 129 chars, over the repo's printWidth of 120, so `prettier --check` failed on this file. mise's ci-unit task list runs :format before :check/:test, so this alone was enough to kill the whole Test Web CI job before a single test ran. * fix(web): clear a stale range anchor when reconcile drops just the anchor reconcile() only nulled #anchor when the whole selection emptied. A partial-failure bulk action can reconcile away exactly the succeeded ids while the rest of the selection survives, so an anchor pointing at one of those succeeded ids went stale but #ids.size stayed > 0. #range()'s indexOf(from) === -1 fallback then always returns [toId], and selectRange's `#anchor ??= toId` never overwrites a non-null stale value — so Shift-click silently degraded to a plain add, indefinitely, until the user happened to make a plain click. Now reconcile also clears #anchor when it's no longer present in the reconciled set, letting the next Shift-click re-arm it via the same `??=` used for the first-interaction case (E-7). * test(server): correct the sequential-guarantee claim on the bulk folder move test The comment overstated this medium test as "the test that actually needs a SEQUENTIAL implementation to pass" — its Promise.all detection is probabilistic (~20-27% of runs, depending on real Postgres connection-pool timing), not deterministic. The deterministic guarantee lives in three unit pins (expect(maxInFlight).toBe(1)) in shared-space.service.spec.ts, one each for bulkUnlinkAlbums, bulkMoveAlbumFolders and bulkDeleteAlbumFolders. Rewrote the comment to state what this test actually contributes: real-DB coverage of a committed sibling write observed against the real partial unique index, which is structurally unmockable. Comment only, no test logic changed. * fix(mobile): stop the space-albums app-bar actions crowding the back button The three editor actions (New folder / New album / Link) are TextButton.icons carrying M3's default 28dp of padding each. On a phone-width toolbar the row overran the available space: NavigationToolbar gave the title zero width and pushed the whole row left until it sat underneath the back button. Trim each action's padding to 8dp either side (54dp of chrome per action down to 42dp) and add the 8dp back as actionsPadding so the trailing action keeps a conventional 16dp end margin. The regression test asserts the per-action chrome rather than an absolute toolbar width: the widget-test font renders every glyph as a fixed-width box, so label widths in tests are ~1.8x their on-device value and no absolute "fits at 411dp" assertion would mean anything.
* docs: design spec for space album rename and delete
* docs: close review gaps in the space album rename/delete spec
* docs: implementation plan for space album rename and delete
* docs: correct plan test placements, helper names and generated-file facts
* docs: pin that space album rename allows duplicate names (E6)
* feat(server): rename a space-linked album as editor or owner
* docs: correct the plan's gate commands (no make check-/lint- targets exist)
* refactor(server): extract the shared-space album editor-or-owner gate
renameAlbum's authorization gate was a near-verbatim copy of
#unlinkAlbumChecked's, differing only in the Permission value. Extract
#requireEditorOrAlbumAccess(auth, spaceId, albumId, permission) and route
both callers through it so a future change to the gate has one call site
to update. Behaviour of #unlinkAlbumChecked is unchanged (same exception
types/messages/call order); its existing tests catch that.
* docs: correct the rename-gate owner arm to owner-or-editor
§3's capability table and §4.1 described the rename route's fallback
arm as album-owner-only. The code uses Permission.AlbumUpdate, which
resolves to owner OR classic album-level editor (access.ts:208-214) —
deliberate, since a classic editor can already rename via PATCH
/albums/{id}. Amend the wording to match; no code changed.
* feat(server): bulk-delete space albums, owner-gated per item
* docs: add the separate prettier CI gate to the plan's global constraints
* style(server): collapse renameAlbum's signature onto one line
Task 1's helper extraction shortened renameAlbum's parameter list enough that
prettier now wants it on a single line; the gate-extraction commit didn't run
prettier --write afterwards. No behaviour change.
* test: cover space album rename/delete RBAC, scopes and delete cascades
Adds real-database medium coverage for album-delete cascade behaviour
(shared_space_album/shared_space_album_user FK cascades and the
shared_space_album_delete_audit trigger's tombstone fan-out) and e2e
coverage for the rename/bulk-delete RBAC matrix, API-key scope pinning,
and bulk-delete route placement.
The tombstone test found that shared_space_album_delete_audit's
per-member and per-creator branches both match a space creator who is
also a member (true for every real space), producing a duplicate
shared_space_album_user_audit row per album delete. Harmless for sync
correctness (getDeletes delivery is idempotent) but real, unintended
duplicate audit-row growth - pinned rather than hidden, flagged for a
follow-up decision.
* chore: regenerate api clients for space album rename and bulk delete
* feat(i18n): add space album rename/delete strings across ten locales
* fix(i18n): use ICU plural branches for bulk album delete strings
space_album_bulk_delete_title and spaces_activity_bulk_deleted_albums
used a flat {count} across all ten locales while their bulk-unlink
siblings one line away already use full plural branches (few/many for
pl/ru). Mirror the sibling's branch structure and noun inflection
instead of a fixed plural form, fixing grammar for 2-4 counts in pl/ru.
* feat(web): capability-gated rename and delete on space album cards
* feat(web): bulk-delete owned space albums and rename from the list
Wires the select bar, list selection, page handlers, and activity feed
for the per-action capability model: rename is canManage OR ownership,
delete is ownership only, and a space viewer who owns a linked album
can now select and delete it (their bar shows Delete alone).
Beyond the plan's literal file list, also widens the per-row check
circle in space-album-card.svelte/space-albums-table.svelte so a
viewer-owner can actually enter selection (it was still gated on
canManage alone), and adds a per-row canSelectAlbum predicate to
SpaceAlbumsTable since its canRename/canDelete are table-wide scalars
that cannot express mixed ownership across rows on their own.
* fix(web): make space-albums-table rename/delete gating per-row
canRename/canDelete on SpaceAlbumsTable were table-wide scalars, so
List view could only grant Rename/Delete uniformly across every row
in one render -- under-granting Delete to every role, including album
owners and editors, since Delete is isOwner-only with no canManage
escape hatch. Convert both to per-row predicates (album) => boolean,
mirroring the existing canSelectAlbum shape, and wire the three table
call sites with real per-album isOwner-based predicates.
Header/folder-row column-alignment guards (Task 6) now key off a new
anyAlbumEditable threshold derived from albums.some(...) instead of
the old scalar, since neither has a single album to test against.
* feat(web): rename and delete a space album from its detail page
* feat(mobile): resolve space album ownership from the local album-user table
* feat(mobile): space album rename and bulk delete actions
RemoteAlbumNotifier holds a snapshot, not a Drift watch, so renaming or
bulk-deleting a space album without an explicit refresh leaves the Albums
tab and every picker showing stale rows. SpaceAlbumActions.renameAlbum and
bulkDeleteAlbums thread a refresh callback (wired to
remoteAlbumProvider.notifier.refresh() in the provider) that fires on
success and is skipped on failure, matching the existing single-item
throw / bulk failed-subset conventions.
* feat(mobile): rename and delete space albums from the albums list
* fix(mobile): resolve the album name for a single-album delete confirmation
The selection bar's delete action never passed singleAlbumName, so
selecting exactly one owned album rendered the confirmation as
Delete ""? instead of naming the album — the mainline flow for a
viewer who owns exactly one album in the space.
* feat(mobile): rename and delete a space album from its detail page
* fix(web): name the album in a single-selection delete confirmation
The select bar reaches handleBulkDeleteAlbums through runBulkAction, which
calls the action with `ids` alone, so a selection of exactly one album took
the singular copy branch with no name and rendered `Delete ""?`. Resolve the
name from the page's own album list when the caller has none, mirroring the
mobile fix in 353ee4f.
The cancelled-confirm test never inspected showDialog's arguments, which is
why it stayed green; it now pins the real name and the empty-name regression.
* fix(mobile): let a viewer extend and leave a space-album selection
Task 11 widened long-press wiring and the selection bar per album, but the
tap router still branched on the page-wide canEdit. A viewer who owns two
albums could enter a selection by long-press, then tapping a second owned
album pushed the detail route and abandoned it — bulk delete was unreachable
for the persona the capability model was widened for, and they could not
deselect by tapping either.
onAlbumTap now routes on an active selection first and gates the toggle on
the same per-album canSelectAlbum used by long-press, leaving an
unselectable card inert mid-selection instead of navigating, matching web's
handleAlbumClick.
* fix(web): keep the space-albums table column grid uniform across rows
The leading select cell and the trailing actions cell were decided per row
while the header and folder rows used a table-wide flag, so for a viewer who
owns only some linked albums the owned rows grew a 32px cell the others did
not have and the two rows' album names no longer lined up.
Both edge cells are now rendered structurally, table-wide, with only their
contents gated per row. The actions cell's testid and click-swallowing stay
tied to the per-row capability so a filler cell behaves like any other cell.
* docs(spaces): pin the albums list's rename under-grant, and cover the arm
The rename gate is space Editor OR Permission.AlbumUpdate (owner or classic
album-level editor). The album detail page implements the whole rule; the
albums list can only manage `canManage || isOwner`, because
SharedSpaceLinkedAlbumDto carries ownerId and no albumUsers, so the
album-editor arm is underivable there. Record that divergence in the design
spec and beside isOwner rather than widening the DTO for it.
The classic-album-editor arm itself had no coverage anywhere, so add the e2e
case: album owner adds a space Viewer as an album-level editor, who renames
it and gets 204 — the exact negative counterpart already sits below it.
* fix(web): narrow, not clear, an album selection on a role downgrade
The M-3 guard cleared the whole selection whenever canManage went false,
reasoning that a selection could only be entered while it was true. That
stopped holding when the check circle widened to canManage || canRename ||
canDelete: a viewer's legitimate selection of albums they own was thrown
away too.
Album selections now narrow to the ids canSelectAlbum still accepts. Folder
selections still clear outright — folders have no owner, and leaving one
alive reproduces the original bug, since the bar is hidden for a folder kind
without canManage while every card click keeps being swallowed.
* fix(web): resolve space album ownership by role, not albumUsers order
isOwned read albumUsers[0], which is wrong in both directions: a
first-listed viewer counted as the owner, and an owner listed later did not.
Match on AlbumUserRole.Owner instead, like isAlbumEditor right above it.
Also click the editor-arm Rename through on the detail page, which only had
a menu-item-renders assertion — the point of that arm is that it reaches
renameSharedSpaceAlbum rather than the owner-only PATCH /albums/{id}.
* fix(mobile): label the New album dialog with the album name field
createAlbum reused the folder prompt wholesale, so "New album" asked for a
"Folder name". Task 11 made label/keyPrefix parameters, so pass the album
label and the space-album-name prefix the rename dialog already uses.
Also correct two comments that overstated what SpaceAlbumActions touches:
renameAlbum fires the owned-albums refresh unconditionally, including for a
space Editor renaming an album they do not own, and the design spec claimed
otherwise. Left unconditional deliberately — a cheap local re-read beats
threading ownership into a repository-only class. Drops a dead pointer to a
gitignored task report from the server duplicate-tombstone comment.
* docs: design spec for the space album empty-state entry, audit dedup and dialog dedup
* docs: rework the follow-ups spec into BDD scenarios with TDD sequencing
* docs: implementation plan for the space album follow-ups
* docs: correct plan errors found in review — RED expectations, drift gate, line ranges
* fix(mobile): let an editor reach album management from an empty space
* fix(server): stop the album-delete audit double-tombstoning the space creator
* refactor(mobile): give the album name and confirm dialogs one implementation
* fix(mobile): correct the merged album call-site count in space_album_dialogs.dart
The doc comment said two album call sites, counting only the list page in
isolation. The shared dialog now also serves the detail page's rename, so
there are three.
* test(server): pin one tombstone per user now the delete audit is deduped
* docs: fix stale onSeeAll/case-2 and trigger-merge comments
- space_albums_shelf.widget.dart / space_top_sliver.widget.dart: onSeeAll
doc claimed a null callback leaves the header entry visible-but-inert;
the render guard is now showSeeAll && onSeeAll != null, so it isn't
rendered at all. Also updates the shelf's case-2 description to mention
the "Manage ▸" header entry it now renders.
- shared-space-album-delete-triggers.spec.ts: the M2 section header still
described the pre-dedup two-INSERT trigger body in present tense; rewrite
it to describe the merged INSERT ... SELECT DISTINCT / CROSS JOIN LATERAL
while keeping the historical context for why the merge happened.
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
1 participant
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.
Members of a Shared Space can organise that space's linked albums into arbitrarily-nested folders. Folders are purely visual: they carry no permissions of their own and have no effect on the space timeline, asset visibility, or album membership.
Server + web. Mobile is deliberately out of scope and continues to list every linked album flat, which degrades gracefully — a mobile user sees all the albums, just not the organisation.
Design:
docs/superpowers/specs/2026-08-04-space-album-folders-design.mdWhat it does
?folder=<uuid>with a breadcrumb, so folder views are deep-linkable and browser back works.?folder=untouched so clearing the box returns you where you were.Design decisions worth knowing
Placement lives on the
shared_space_albumjoin row, not onalbum. This makes "space albums only" a property of the schema rather than a UI convention — there is no column onalbumwith which to nest a personal album. It also gives an album linked into two spaces an independent placement in each, for free.Sibling-name uniqueness needs two partial unique indexes, not one. Gallery targets PostgreSQL 14, so
NULLS NOT DISTINCTis unavailable. A single index over(spaceId, parentId, lower(name))would let two root folders share a name, because Postgres treats NULL parents as distinct.Cross-space placement is enforced in the service, not the schema. The composite FK that would enforce it needs per-column
ON DELETE SET NULL, which is PG15+; on PG14 it would nullspaceIdtoo, and that column isNOT NULLand half the primary key. Pinned by tests instead, and documented in the spec.Folder moves take a per-space transaction-scoped advisory lock. Row locking is insufficient: two editors concurrently moving X into Y and Y into X lock disjoint rows, both ancestor checks pass, and the result is a detached cycle.
Folder-aware linking uses a query param, not a request body. A NestJS
@Body() dtoemitsrequired: truein the OpenAPI document even when every field is optional, which would change the generated DartlinkAlbumsignature and break mobile's existing call.Mobile safety
mobile/lib/is untouched; onlymobile/openapi/is regenerated, additively.SharedSpaceAlbumLinkUpdateDto.showInTimelineremains required and non-nullable, andlinkAlbumgained only an optional named parameter, so existing call sites still compile.folderIdis deliberately absent from the mobile sync payload — a regression test pins that, since adding it is a mobile-facing decision that needs its own spec.Testing
The privacy-relevant assertion is that a non-member listing a space's folders receives 403, never an empty 200 — a folder name is itself information, and an empty list would confirm the space exists.
Known follow-ups
CYCLEguard. Safe today, since the advisory-locked move path is the only writer ofparentId.space_album_folder_depth_exceeded,_limit_reached,_name_taken) —handleErrorsurfaces the server message instead.