feat(mobile): nestable folders for space albums - #936
Merged
Deeds67 merged 42 commits intoAug 6, 2026
Conversation
…igger 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.
…r_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.
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).
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.
…eview 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.
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.
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.
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.
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.
…pty-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.
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.
…tself 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.
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.
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.
…lback 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.
…ation" 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.
|
Suggestion: |
… 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).
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.
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.
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.
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).
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.
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.
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.
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).
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.
…ink 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.
…Id 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.
…oute 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.
…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.
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
2 participants
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.
Brings nestable album folders to the mobile app, reaching parity with the web implementation in #931.
Stacked on #931 — this branch forked from
feat/space-album-folders, so it targets that branch. GitHub will retarget it tomainonce #931 merges.Folders are a visual organisation layer only: an album's permissions always come from its Space, never from the folder it sits in.
What's in it
Server — a
shared_space_album_folder_audittable with a delete trigger, folder sync arms (upsert + delete) insync.repository.ts, andfolderIdadded to the album-link sync payload. Both sync arms are gated byaccessibleSpaces.Mobile — a Drift table and v37 migration, sync handlers, a pure tree module, repository queries, folder create/rename/move/delete actions, a folder card and picker, and the space-albums page integration.
Web — one divergence fix (see below).
Two behaviours worth knowing about
An album whose folder hasn't synced yet renders at the space root rather than disappearing. Sync delivers folder rows and album links independently in no guaranteed order, so an album pointing at a not-yet-arrived folder is routine transient state. Nothing joins albums to folders or filters on "folder exists" anywhere in the pipeline. Web previously hid such albums; the last web commit here fixes that so both platforms behave the same.
Folder state on device is owned by sync, not by the UI. Actions call the server and let the sync stream write the local rows — no optimistic layer, which would race sync and flip back.
Notes for review
buildFolderTreeis total: it cannot throw or loop on a danglingparentId, a self-reference, or a cycle. Cycle members are classified as roots, which makes every node reachable by construction; folders hanging below a cycle stay nested rather than being flattened.folderId. Every album in every space is linked through that table, so it uses nocolumnTransformer— drift copies every pre-existing column verbatim — and indexes are re-created fromsqlite_master.SharedSpaceAlbumFoldersV1is behind the existing server-version capability gate, so folder-less servers are never asked for it.Testing
Mobile 3156 passed / 0 failed / 1 skipped. Server unit 5318 passed / 14 skipped; both medium sync specs green. Web 4286 tests across 305 files.
dart analyze --fatal-infos,dart format, servercheck/lint/prettier, and the web type checks all clean.make sqlwas not run locally — the only live dev database is shared across worktrees and lacks this branch's migrations. CI'ssql-schema-up-to-datejob covers it on a fresh Postgres.Manual testing
＋while inside another folder — it should nest there, not at the space root.＋New folder action and no⋮on folder cards.