From c6e991c9ae9195f97fb91757025a083bcd09f392 Mon Sep 17 00:00:00 2001 From: Pierre Marais Date: Mon, 10 Aug 2026 10:15:14 +0200 Subject: [PATCH 1/3] fix(spaces): offer a specific space album from every add-to-collection entry point (#965) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a photo to an album inside a Shared Space depended on which screen you started from, and web and mobile disagreed about which screens worked. Web never listed space-linked albums at all: `CollectionPickerModal` loaded personal albums plus writable spaces and stopped there, so no web surface could target an album inside a space. A space row with linked albums now expands, on click, into an "Add to space" child plus one row per linked album — fetched lazily and cached for the life of the modal, so opening the picker still costs two requests however many spaces the user is in. The rows stay one flat keyboard-navigable sequence, and a space with no linked albums behaves exactly as before. Mobile had the right picker already but mounted it on only two surfaces; the rest still used upstream's album-only `AlbumSelector`. `CollectionPicker` is now wired into the album, favorites, archive and on-device-album sheets and into the asset viewer's "+". To make the viewer work it gained `source` (it dispatches as `ActionSource.viewer`) and `assets` — the viewer has no multiselect, and falling back to the empty one would read as "nothing non-owned" and offer space targets for someone else's photo. No server change: `Permission.AlbumAssetCreate` already grants a space Owner/Editor add-permission on every album linked to that space, so a space-linked album dispatches exactly like a personal one. The partner-detail sheet is deliberately left on the album-only selector — a partner's asset can never reach any space target, and web hides its "+" there for the same reason. --- ...08-10-965-space-album-add-parity-design.md | 205 ++++++++++++++++++ .../add_action_button.widget.dart | 64 ++---- .../archive_bottom_sheet.widget.dart | 33 +-- .../favorite_bottom_sheet.widget.dart | 53 +---- .../local_album_bottom_sheet.widget.dart | 33 +-- .../remote_album_bottom_sheet.widget.dart | 35 +-- .../collection/collection_picker.widget.dart | 42 +++- .../space_collection_section.widget.dart | 12 +- .../add_to_collection_surfaces_test.dart | 186 ++++++++++++++++ .../collection/collection_picker_test.dart | 151 +++++++++++++ .../space_collection_section_test.dart | 31 +++ .../collection-selection-utils.spec.ts | 126 +++++++++++ .../collection-selection-utils.ts | 78 ++++++- .../space-list-item.svelte | 13 +- .../lib/modals/CollectionPickerModal.spec.ts | 144 ++++++++++++ .../lib/modals/CollectionPickerModal.svelte | 109 +++++++++- 16 files changed, 1111 insertions(+), 204 deletions(-) create mode 100644 docs/superpowers/specs/2026-08-10-965-space-album-add-parity-design.md create mode 100644 mobile/test/presentation/widgets/bottom_sheet/add_to_collection_surfaces_test.dart diff --git a/docs/superpowers/specs/2026-08-10-965-space-album-add-parity-design.md b/docs/superpowers/specs/2026-08-10-965-space-album-add-parity-design.md new file mode 100644 index 0000000000000..a519ab45be9ce --- /dev/null +++ b/docs/superpowers/specs/2026-08-10-965-space-album-add-parity-design.md @@ -0,0 +1,205 @@ +# Adding photos to a specific Shared Space album, from every entry point (#965) + +Status: draft 2026-08-10. Closes the entry-point gap left by +`2026-07-25-space-add-to-collection-design.md` (web `CollectionPickerModal`) and +`2026-07-26-mobile-spaces-ux-design.md` (mobile `CollectionPicker`). + +## Problem + +Whether "add this photo to that Shared Space album" is possible depends on which screen the user +started from, and the two clients disagree about which screens work. + +| Entry point | Personal album | Space pool | **Album inside a space** | +| ---------------------------------------- | -------------- | ---------- | ------------------------ | +| Web — any surface (one shared picker) | yes | yes | **no** | +| Mobile — main timeline / search / person | yes | yes | yes | +| Mobile — inside a personal album | yes | **no** | **no** | +| Mobile — favorites, archive, local album | yes | **no** | **no** | +| Mobile — asset viewer `+` → Album | yes | **no** | **no** | + +Two independent causes: + +1. **Web never lists space-linked albums at all.** `CollectionPickerModal` loads personal albums + (`getAllAlbums`) plus writable spaces (`getAllSpaces`) and stops there. The only place it lists a + space's linked albums is `restrictToSpaceId` mode — reached solely from a space surface with a + non-owned selection (#764 contribution). So the target simply does not exist in the normal picker, + on any web surface. +2. **Mobile has two different pickers and only one of them knows about spaces.** The fork's + `CollectionPicker` (album selector **+** `SpaceCollectionSection`) is wired into + `general_bottom_sheet` and `space_bottom_sheet`; every other add-to-collection surface still + mounts upstream's bare `AlbumSelector`. + +Note that the web gap is uniform — every web surface is equally broken — while the mobile gap is +per-surface. That is why the issue reads as "inconsistent depending on entry point" on mobile and +"missing everywhere" on web. + +## What the server already permits — no server change + +`Permission.AlbumAssetCreate` has a `checkSpaceLinkedAlbumAccess` arm +(`server/src/utils/access.ts:195`, `access.repository.ts:147`): **every album linked to a space where +the caller is Owner or Editor** grants add-permission, even when the caller is neither album owner +nor album user. `POST /albums/:id/assets` is therefore the one and only call needed; a space-linked +album is dispatched exactly like a personal album. + +`GET /shared-spaces/:id/albums` (`SharedSpaceRead`) lists a space's linked albums, and +`GET /shared-spaces` already returns `albumCount` per space (`shared-space.service.ts:143`), which is +what lets a client know a space is expandable without fetching its albums. + +So this is a pure client change on both platforms. + +## Design + +### Guiding rule: one picker per platform, same shape on both + +The fix is not "add space albums to N places". It is "there is exactly one add-to-collection picker +per platform, and it offers albums, spaces, and space albums". Web already has one picker used +everywhere, so web needs only the missing rows. Mobile has the right picker already built — it just +is not mounted on every surface. + +### Row shape — accordion, mirroring mobile + +Mobile's `SpaceCollectionSection` is the reference: a space row with linked albums is expandable; +expanding reveals an "Add to space" child (the pool) plus one child row per linked album. At most one +space is expanded at a time. Web adopts the same shape rather than a flat "Space › Album" list, +because: + +- it keeps the picker short when a user has many spaces with many albums; +- it avoids fanning out `getSharedSpaceAlbums` for every space on modal open — the call happens once, + lazily, when a space is expanded, and is cached for the life of the modal; +- it is the interaction users already know from mobile. + +A space with `albumCount === 0` stays a plain row whose click adds to the pool — unchanged from +today, and identical to mobile. + +**Accepted wart (web):** a space can appear twice, once under `RECENT` and once under `ALL`. +Expansion is keyed by space id, so both occurrences expand together and render the same children. +The alternative — expandable only in `ALL` — would give the same visual row two different click +behaviours, which is worse. + +### Search + +Search filters **top-level rows only**, on both platforms: album names/descriptions and space +names. Children of an expanded space are not filtered. This is mobile's current behaviour and is kept +verbatim on web so the two stay in step. Making search reach into space albums requires eagerly +loading every space's albums (web: N requests; mobile: N live Drift subscriptions, which +`SpaceCollectionSection` deliberately bounds to one) and is out of scope. + +### Web changes + +`web/src/lib/components/shared-components/collection-selection/collection-selection-utils.ts` + +- `CollectionModalRow` gains `expandable?`, `expanded?`, `indented?`. +- New row type `SPACE_POOL_CHILD` — selectable, carries the space collection, rendered indented + under an expanded space row. Added to `isSelectableRowType` so keyboard nav counts it. +- `toModalRows` gains `expandedSpaceId?` and `expandedSpaceAlbums?: PickerCollection[]`. After + pushing a space row whose id matches `expandedSpaceId` it pushes: the pool child, then one indented + `COLLECTION_ITEM` per linked album, or a `MESSAGE` row (`no_albums_in_space_yet`) when the space has + none. `expandedSpaceAlbums === undefined` means the fetch is still in flight and is deliberately + distinct from `[]` — only the pool child renders, so "this space has no albums yet" never flashes + before the answer is known. +- Children are pushed inside `pushItem` so the running `index` — and therefore arrow-key order — + stays a single flat sequence over visible selectable rows. + +`web/src/lib/modals/CollectionPickerModal.svelte` + +- `expandedSpaceId = $state(null)` and + `spaceAlbumCache = $state>({})`. +- Clicking a space row: `albumCount > 0` → toggle expansion (fetching + caching + `getSharedSpaceAlbums` on first expand, `handleError` on failure and collapse); otherwise → select + the pool, as today. +- Multi-select on a space row still means the pool. Space-album children participate in multi-select + like any album row. +- Restricted mode (`restrictToSpaceId`) is untouched — it already lists exactly one space's albums + and never lists spaces. + +`web/src/lib/components/shared-components/collection-selection/space-list-item.svelte` + +- New `expandable` / `expanded` props render a chevron and set `aria-expanded`. + +Dispatch (`collection.service.ts`) needs **no change**: a space-linked album arrives as +`{ kind: 'album' }` and goes through `addAssetsToAlbums`, which is `POST /albums/:id/assets` — the +endpoint that carries the space-linked permission arm. + +### Mobile changes + +Replace the bare `AlbumSelector` with `CollectionPicker` on every add-to-collection surface: + +| File | Surface | +| --------------------------------------------- | ------------------------- | +| `bottom_sheet/remote_album_bottom_sheet.dart` | selection inside an album | +| `bottom_sheet/favorite_bottom_sheet.dart` | favorites | +| `bottom_sheet/archive_bottom_sheet.dart` | archive | +| `bottom_sheet/local_album_bottom_sheet.dart` | on-device album | +| `action_buttons/add_action_button.dart` | asset viewer `+` → Album | + +To make that possible `CollectionPicker` gains two things: + +- **`source` (`ActionSource`, default `timeline`)** — the asset viewer dispatches against + `ActionSource.viewer`, and `_addToAlbum` / `_addToTarget` must pass it through instead of + hard-coding `timeline`. +- **`assets`** — `SpaceCollectionSection` currently reads `multiSelectProvider.selectedAssets` to + decide its notices (non-owned / locked / over-cap). In the asset viewer the multiselect is empty, + which would read as "nothing non-owned" and wrongly offer space targets for someone else's photo. + `CollectionPicker` resolves the asset set from `source` (timeline → multiselect, viewer → + `assetViewerProvider.currentAsset`) and passes it down; `SpaceCollectionSection` takes an optional + `assets` and falls back to the multiselect so its existing tests and callers are unaffected. +- **`onCompleted`** (optional) — the asset viewer needs its existing post-add behaviour preserved: + invalidate `albumsContainingAssetProvider` (the info panel's "Appears in" list) and pop the sheet. + +**Deliberately excluded: `partner_detail_bottom_sheet`.** A partner's asset can never reach any space +target — the pool rejects the whole request (`AssetShare`), and the #764 contribution arm needs the +asset to be visible _through_ a space the caller edits, which a partner asset is not. Mounting the +picker there would add a permanently-empty Spaces section with a "hidden because the selection is not +yours" notice on every selection. Web agrees: `getSelectionCapabilities` returns +`canAddToAlbum: false` on a partner surface, so web hides the `+` entirely there. Left as-is. + +`drift_album.page.dart` also mounts `AlbumSelector`, but as an album **browser** (tap navigates to the +album), not a picker. Out of scope. + +## Out of scope + +**Selections over `MAX_SPACE_ASSETS_PER_REQUEST` (50 000).** Web already hides every space row above +that cap, with a notice, because `POST /shared-spaces/:id/assets` cannot take the request. Space +albums go through the album endpoint and are not capped, but they are only reachable by expanding a +space row — so above the cap they disappear along with the spaces. Adding a second, uncapped route to +them would complicate the picker for a case that needs a 50 000-asset selection to reach; the notice +already explains why the section is gone. + +**Duplicate rows for an album you own that is linked to a space.** It appears both as a personal +album at the top level and as a child of its space. Mobile has behaved this way since the spaces +section shipped, and the nesting is informative rather than wrong. + +The issue's closing note — extending the Album filter from `All / Has album / Has no album` to +"filter by a specific album" so photos can be found from a Space album surface — is a separate +feature request against the filter system, not an entry-point inconsistency. Tracked separately. + +Issue #966 (album sort options differing between web and mobile inside a Space) is a different +parity bug and is not touched here. + +## Test plan + +TDD, red first, per platform. + +**Web** — `CollectionPickerModal.spec.ts`, `collection-selection-utils` converter spec: + +1. a space with `albumCount > 0` renders as expandable and does **not** immediately fetch its albums; +2. clicking it calls `getSharedSpaceAlbums({ id })` once and renders one child row per linked album + plus the "Add to space" pool child; +3. clicking a space-album child confirms with `{ kind: 'album', id: }`; +4. clicking the pool child confirms with `{ kind: 'space', id: }`; +5. expanding a second space collapses the first, and re-expanding the first does not re-fetch; +6. a space with `albumCount === 0` is not expandable and its click still confirms with the space; +7. a failed `getSharedSpaceAlbums` calls `handleError` and leaves the row collapsed; +8. arrow-key order walks the children in visual order; +9. restricted mode is unchanged (existing suite must stay green). + +**Mobile** — `collection_picker_test.dart`, `space_collection_section_test.dart`, plus one test per +newly-wired surface: + +1. `CollectionPicker` dispatches against the `source` it was given (viewer vs timeline); +2. `CollectionPicker` passes viewer assets to `SpaceCollectionSection`, so a non-owned asset in the + viewer shows the notice and offers no space target; +3. `SpaceCollectionSection` with an explicit `assets` argument ignores the multiselect; +4. `onCompleted` fires after a successful add and not after a failure; +5. each rewired surface renders the collection-picker header (`collection-picker-header`) rather than + a bare `AlbumSelector`. diff --git a/mobile/lib/presentation/widgets/action_buttons/add_action_button.widget.dart b/mobile/lib/presentation/widgets/action_buttons/add_action_button.widget.dart index dc48ed57ecb17..daca06bc3f692 100644 --- a/mobile/lib/presentation/widgets/action_buttons/add_action_button.widget.dart +++ b/mobile/lib/presentation/widgets/action_buttons/add_action_button.widget.dart @@ -1,19 +1,19 @@ +import 'dart:async'; + import 'package:easy_localization/easy_localization.dart'; import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; -import 'package:immich_mobile/extensions/translate_extensions.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/base_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/unarchive_action_button.widget.dart'; -import 'package:immich_mobile/providers/asset_viewer/asset_viewer.provider.dart'; import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; -import 'package:immich_mobile/providers/infrastructure/action.provider.dart'; +import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; +import 'package:immich_mobile/providers/asset_viewer/asset_viewer.provider.dart'; import 'package:immich_mobile/providers/infrastructure/album.provider.dart'; import 'package:immich_mobile/providers/routes.provider.dart'; import 'package:immich_mobile/widgets/common/immich_toast.dart'; import 'package:immich_mobile/providers/user.provider.dart'; -import 'package:immich_mobile/domain/models/album/album.model.dart'; import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/constants/enums.dart'; @@ -113,9 +113,16 @@ class _AddActionButtonState extends ConsumerState { return; } + // #965: the same picker every other surface offers. The viewer has no multiselect, so it + // states its source and its one asset explicitly — the spaces section judges ownership + // from that rather than from an empty selection. final List slivers = [ const CreateAlbumButton(), - AlbumSelector(onAlbumSelected: (album) => _addCurrentAssetToAlbum(album)), + CollectionPicker( + source: ActionSource.viewer, + assets: [currentAsset], + onCompleted: () => _onAddCompleted(currentAsset), + ), ]; showModalBottomSheet( @@ -136,51 +143,18 @@ class _AddActionButtonState extends ConsumerState { ); } - Future _addCurrentAssetToAlbum(RemoteAlbum album) async { - final latest = ref.read(assetViewerProvider).currentAsset; - - if (latest == null) { - ImmichToast.show(context: context, msg: "Cannot load asset information.", toastType: ToastType.error); - return; - } - - final result = await ref.read(actionProvider.notifier).addToAlbum(ActionSource.viewer, album); - - if (!context.mounted) { - return; - } - - if (!result.success) { - ImmichToast.show(context: context, msg: 'scaffold_body_error_occurred'.tr(), toastType: ToastType.error); - return; - } - - // Only report the failure when nothing was added; if some succeeded we show "added". - if (result.count > 0) { - ImmichToast.show( - context: context, - msg: 'add_to_album_bottom_sheet_added'.tr(namedArgs: {'album': album.name}), - ); - + /// The picker owns the dispatch and the toasts; the viewer only has to refresh what it + /// shows and get out of the way. + void _onAddCompleted(BaseAsset asset) { + final remoteId = asset.remoteId; + if (remoteId != null) { // Refresh the "Appears in" list on the asset's info panel. - ref.invalidate(albumsContainingAssetProvider(latest.remoteId!)); - } else if (result.failedCount > 0) { - ImmichToast.show( - context: context, - msg: 'assets_cannot_be_added_to_album_count'.t(context: context, args: {'count': result.failedCount}), - toastType: ToastType.error, - ); - } else { - ImmichToast.show( - context: context, - msg: 'add_to_album_bottom_sheet_already_exists'.tr(namedArgs: {'album': album.name}), - ); + ref.invalidate(albumsContainingAssetProvider(remoteId)); } - if (!context.mounted) { return; } - await Navigator.of(context).maybePop(); + unawaited(Navigator.of(context).maybePop()); } @override diff --git a/mobile/lib/presentation/widgets/bottom_sheet/archive_bottom_sheet.widget.dart b/mobile/lib/presentation/widgets/bottom_sheet/archive_bottom_sheet.widget.dart index 3c9c0c692e424..af2e049061e90 100644 --- a/mobile/lib/presentation/widgets/bottom_sheet/archive_bottom_sheet.widget.dart +++ b/mobile/lib/presentation/widgets/bottom_sheet/archive_bottom_sheet.widget.dart @@ -1,8 +1,6 @@ -import 'package:easy_localization/easy_localization.dart'; import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; -import 'package:immich_mobile/domain/models/album/album.model.dart'; import 'package:immich_mobile/presentation/actions/action.widget.dart'; import 'package:immich_mobile/presentation/actions/favorite.action.dart'; import 'package:immich_mobile/presentation/actions/timeline.action.dart'; @@ -18,12 +16,10 @@ import 'package:immich_mobile/presentation/widgets/action_buttons/stack_action_b import 'package:immich_mobile/presentation/widgets/action_buttons/trash_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/unarchive_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/unstack_action_button.widget.dart'; -import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; import 'package:immich_mobile/presentation/widgets/bottom_sheet/base_bottom_sheet.widget.dart'; -import 'package:immich_mobile/providers/infrastructure/action.provider.dart'; +import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; import 'package:immich_mobile/providers/server_info.provider.dart'; import 'package:immich_mobile/providers/timeline/multiselect.provider.dart'; -import 'package:immich_mobile/widgets/common/immich_toast.dart'; class ArchiveBottomSheet extends ConsumerStatefulWidget { const ArchiveBottomSheet({super.key}); @@ -52,26 +48,6 @@ class _ArchiveBottomSheetState extends ConsumerState { final multiselect = ref.watch(multiSelectProvider); final isTrashEnable = ref.watch(serverInfoProvider.select((state) => state.serverFeatures.trash)); - Future addToAlbum(RemoteAlbum album) async { - final result = await ref.read(actionProvider.notifier).addToAlbum(ActionSource.timeline, album); - - if (!context.mounted) { - return; - } - - if (!result.success) { - ImmichToast.show(context: context, msg: 'scaffold_body_error_occurred'.tr(), toastType: ToastType.error); - return; - } - - ImmichToast.show( - context: context, - msg: result.count == 0 - ? 'add_to_album_bottom_sheet_already_exists'.tr(namedArgs: {'album': album.name}) - : 'add_to_album_bottom_sheet_added'.tr(namedArgs: {'album': album.name}), - ); - } - Future onKeyboardExpand() { return sheetController.animateTo(0.85, duration: const Duration(milliseconds: 200), curve: Curves.easeInOut); } @@ -102,10 +78,9 @@ class _ArchiveBottomSheetState extends ConsumerState { ], if (multiselect.hasMerged) const DeleteLocalActionButton(source: ActionSource.timeline), ], - slivers: [ - const AddToAlbumHeader(), - AlbumSelector(onAlbumSelected: addToAlbum, onKeyboardExpanded: onKeyboardExpand), - ], + // #965: the same picker the main timeline offers, so a space album is reachable from + // the archive too. + slivers: [CollectionPicker(onKeyboardExpanded: onKeyboardExpand)], ); } } diff --git a/mobile/lib/presentation/widgets/bottom_sheet/favorite_bottom_sheet.widget.dart b/mobile/lib/presentation/widgets/bottom_sheet/favorite_bottom_sheet.widget.dart index bcb9fc6fe32bf..4c90208f5947f 100644 --- a/mobile/lib/presentation/widgets/bottom_sheet/favorite_bottom_sheet.widget.dart +++ b/mobile/lib/presentation/widgets/bottom_sheet/favorite_bottom_sheet.widget.dart @@ -1,9 +1,6 @@ import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; -import 'package:immich_mobile/domain/models/album/album.model.dart'; -import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; -import 'package:immich_mobile/extensions/translate_extensions.dart'; import 'package:immich_mobile/presentation/actions/action.widget.dart'; import 'package:immich_mobile/presentation/actions/favorite.action.dart'; import 'package:immich_mobile/presentation/actions/timeline.action.dart'; @@ -19,12 +16,10 @@ import 'package:immich_mobile/presentation/widgets/action_buttons/share_link_act import 'package:immich_mobile/presentation/widgets/action_buttons/stack_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/trash_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/unstack_action_button.widget.dart'; -import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; import 'package:immich_mobile/presentation/widgets/bottom_sheet/base_bottom_sheet.widget.dart'; -import 'package:immich_mobile/providers/infrastructure/album.provider.dart'; +import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; import 'package:immich_mobile/providers/server_info.provider.dart'; import 'package:immich_mobile/providers/timeline/multiselect.provider.dart'; -import 'package:immich_mobile/widgets/common/immich_toast.dart'; class FavoriteBottomSheet extends ConsumerWidget { const FavoriteBottomSheet({super.key}); @@ -34,46 +29,6 @@ class FavoriteBottomSheet extends ConsumerWidget { final multiselect = ref.watch(multiSelectProvider); final isTrashEnable = ref.watch(serverInfoProvider.select((state) => state.serverFeatures.trash)); - Future addAssetsToAlbum(RemoteAlbum album) async { - final selectedAssets = multiselect.selectedAssets; - if (selectedAssets.isEmpty) { - return; - } - - final remoteAssets = selectedAssets.whereType(); - final result = await ref - .read(remoteAlbumProvider.notifier) - .addAssets(album.id, remoteAssets.map((e) => e.id).toList()); - - if (selectedAssets.length != remoteAssets.length) { - ImmichToast.show( - context: context, - msg: 'add_to_album_bottom_sheet_some_local_assets'.t(context: context), - ); - } - - // Only report the failure when nothing was added; if some succeeded we show "added". - if (result.added > 0) { - ImmichToast.show( - context: context, - msg: 'add_to_album_bottom_sheet_added'.t(args: {"album": album.name}), - ); - } else if (result.failed > 0) { - ImmichToast.show( - context: context, - msg: 'assets_cannot_be_added_to_album_count'.t(context: context, args: {'count': result.failed}), - toastType: ToastType.error, - ); - } else { - ImmichToast.show( - context: context, - msg: 'add_to_album_bottom_sheet_already_exists'.t(args: {"album": album.name}), - ); - } - - ref.read(multiSelectProvider.notifier).reset(); - } - final assets = multiselect.selectedAssets.toList(growable: false); final actions = [FavoriteAction(assets: assets)]; @@ -99,9 +54,9 @@ class FavoriteBottomSheet extends ConsumerWidget { ], if (multiselect.hasMerged) const DeleteLocalActionButton(source: ActionSource.timeline), ], - slivers: multiselect.hasRemote - ? [const AddToAlbumHeader(), AlbumSelector(onAlbumSelected: addAssetsToAlbum)] - : [], + // #965: the same picker the main timeline offers, so a space album is reachable from + // favorites too. + slivers: multiselect.hasRemote ? [const CollectionPicker()] : [], ); } } diff --git a/mobile/lib/presentation/widgets/bottom_sheet/local_album_bottom_sheet.widget.dart b/mobile/lib/presentation/widgets/bottom_sheet/local_album_bottom_sheet.widget.dart index ac8c77af03d8d..9671fbfdbe8ff 100644 --- a/mobile/lib/presentation/widgets/bottom_sheet/local_album_bottom_sheet.widget.dart +++ b/mobile/lib/presentation/widgets/bottom_sheet/local_album_bottom_sheet.widget.dart @@ -1,15 +1,11 @@ -import 'package:easy_localization/easy_localization.dart'; import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; -import 'package:immich_mobile/domain/models/album/album.model.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/delete_local_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/share_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/upload_action_button.widget.dart'; -import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; import 'package:immich_mobile/presentation/widgets/bottom_sheet/base_bottom_sheet.widget.dart'; -import 'package:immich_mobile/providers/infrastructure/action.provider.dart'; -import 'package:immich_mobile/widgets/common/immich_toast.dart'; +import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; class LocalAlbumBottomSheet extends ConsumerStatefulWidget { const LocalAlbumBottomSheet({super.key}); @@ -35,26 +31,6 @@ class _LocalAlbumBottomSheetState extends ConsumerState { @override Widget build(BuildContext context) { - Future addToAlbum(RemoteAlbum album) async { - final result = await ref.read(actionProvider.notifier).addToAlbum(ActionSource.timeline, album); - - if (!context.mounted) { - return; - } - - if (!result.success) { - ImmichToast.show(context: context, msg: 'scaffold_body_error_occurred'.tr(), toastType: ToastType.error); - return; - } - - ImmichToast.show( - context: context, - msg: result.count == 0 - ? 'add_to_album_bottom_sheet_already_exists'.tr(namedArgs: {'album': album.name}) - : 'add_to_album_bottom_sheet_added'.tr(namedArgs: {'album': album.name}), - ); - } - Future onKeyboardExpand() { return sheetController.animateTo(0.85, duration: const Duration(milliseconds: 200), curve: Curves.easeInOut); } @@ -69,10 +45,9 @@ class _LocalAlbumBottomSheetState extends ConsumerState { DeleteLocalActionButton(source: ActionSource.timeline), UploadActionButton(source: ActionSource.timeline), ], - slivers: [ - const AddToAlbumHeader(), - AlbumSelector(onAlbumSelected: addToAlbum, onKeyboardExpanded: onKeyboardExpand), - ], + // #965: the same picker the main timeline offers. A selection here is local-only, and + // the space paths upload before they add, so a space album is a valid destination. + slivers: [CollectionPicker(onKeyboardExpanded: onKeyboardExpand)], ); } } diff --git a/mobile/lib/presentation/widgets/bottom_sheet/remote_album_bottom_sheet.widget.dart b/mobile/lib/presentation/widgets/bottom_sheet/remote_album_bottom_sheet.widget.dart index a292c1899c749..d0002629e9bfc 100644 --- a/mobile/lib/presentation/widgets/bottom_sheet/remote_album_bottom_sheet.widget.dart +++ b/mobile/lib/presentation/widgets/bottom_sheet/remote_album_bottom_sheet.widget.dart @@ -2,7 +2,6 @@ import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; import 'package:immich_mobile/domain/models/album/album.model.dart'; -import 'package:immich_mobile/extensions/translate_extensions.dart'; import 'package:immich_mobile/presentation/actions/action.widget.dart'; import 'package:immich_mobile/presentation/actions/favorite.action.dart'; import 'package:immich_mobile/presentation/actions/timeline.action.dart'; @@ -20,13 +19,11 @@ import 'package:immich_mobile/presentation/widgets/action_buttons/share_link_act import 'package:immich_mobile/presentation/widgets/action_buttons/stack_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/trash_action_button.widget.dart'; import 'package:immich_mobile/presentation/widgets/action_buttons/unstack_action_button.widget.dart'; -import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; import 'package:immich_mobile/presentation/widgets/bottom_sheet/base_bottom_sheet.widget.dart'; -import 'package:immich_mobile/providers/infrastructure/action.provider.dart'; +import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; import 'package:immich_mobile/providers/server_info.provider.dart'; import 'package:immich_mobile/providers/timeline/multiselect.provider.dart'; import 'package:immich_mobile/providers/user.provider.dart'; -import 'package:immich_mobile/widgets/common/immich_toast.dart'; class RemoteAlbumBottomSheet extends ConsumerStatefulWidget { final RemoteAlbum album; @@ -57,30 +54,6 @@ class _RemoteAlbumBottomSheetState extends ConsumerState final isTrashEnable = ref.watch(serverInfoProvider.select((state) => state.serverFeatures.trash)); final ownsAlbum = ref.watch(currentUserProvider)?.id == widget.album.ownerId; - Future addToAlbum(RemoteAlbum album) async { - final result = await ref.read(actionProvider.notifier).addToAlbum(ActionSource.timeline, album); - - if (!context.mounted) { - return; - } - - if (!result.success) { - ImmichToast.show( - context: context, - msg: 'scaffold_body_error_occurred'.t(context: context), - toastType: ToastType.error, - ); - return; - } - - ImmichToast.show( - context: context, - msg: result.count == 0 - ? 'add_to_album_bottom_sheet_already_exists'.t(context: context, args: {"album": album.name}) - : 'add_to_album_bottom_sheet_added'.t(context: context, args: {"album": album.name}), - ); - } - Future onKeyboardExpand() { return sheetController.animateTo(0.85, duration: const Duration(milliseconds: 200), curve: Curves.easeInOut); } @@ -120,9 +93,9 @@ class _RemoteAlbumBottomSheetState extends ConsumerState if (ownsAlbum && multiselect.selectedAssets.length == 1) SetAlbumCoverActionButton(source: ActionSource.timeline, albumId: widget.album.id), ], - slivers: ownsAlbum - ? [const AddToAlbumHeader(), AlbumSelector(onAlbumSelected: addToAlbum, onKeyboardExpanded: onKeyboardExpand)] - : null, + // #965: the same picker the main timeline offers, so a space album is reachable from + // inside an album too — not only from the timeline. + slivers: ownsAlbum ? [CollectionPicker(onKeyboardExpanded: onKeyboardExpand)] : null, ); } } diff --git a/mobile/lib/presentation/widgets/collection/collection_picker.widget.dart b/mobile/lib/presentation/widgets/collection/collection_picker.widget.dart index 7a0882f94cbf6..0de144018b694 100644 --- a/mobile/lib/presentation/widgets/collection/collection_picker.widget.dart +++ b/mobile/lib/presentation/widgets/collection/collection_picker.widget.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; import 'package:immich_mobile/domain/models/album/album.model.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/collection_target.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; @@ -18,7 +19,14 @@ import 'package:sliver_tools/sliver_tools.dart'; /// `AddToAlbumHeader` is not reused because it hardcodes the `add_to_album` key; this /// picker supplies its own header so the sheet can honestly say "album or space". class CollectionPicker extends ConsumerStatefulWidget { - const CollectionPicker({super.key, this.excludeSpaceId, this.onKeyboardExpanded}); + const CollectionPicker({ + super.key, + this.excludeSpaceId, + this.onKeyboardExpanded, + this.source = ActionSource.timeline, + this.assets, + this.onCompleted, + }); /// Set on a space's own surface so that space is not offered as a destination for /// its own assets. @@ -26,6 +34,18 @@ class CollectionPicker extends ConsumerStatefulWidget { final Function? onKeyboardExpanded; + /// Where the assets to file come from. The asset viewer dispatches against + /// [ActionSource.viewer]; every multi-select surface uses the default. + final ActionSource source; + + /// The assets being filed, for the spaces section's ownership / cap notices. Omit on a + /// multi-select surface — the section then reads the timeline selection itself. + final Iterable? assets; + + /// Called after an add that succeeded. Surfaces that dismiss themselves (the asset viewer + /// sheet) hook this; a failed add deliberately does not fire it, so the sheet stays open. + final VoidCallback? onCompleted; + @override ConsumerState createState() => _CollectionPickerState(); } @@ -37,7 +57,7 @@ class _CollectionPickerState extends ConsumerState { Future _addToAlbum(RemoteAlbum album) async { if (_isBusy) return; setState(() => _isBusy = true); - final result = await ref.read(actionProvider.notifier).addToAlbum(ActionSource.timeline, album); + final result = await ref.read(actionProvider.notifier).addToAlbum(widget.source, album); if (!mounted) return; setState(() => _isBusy = false); @@ -45,12 +65,22 @@ class _CollectionPickerState extends ConsumerState { _toastError(); return; } + if (result.count == 0 && result.failedCount > 0) { + // Nothing landed and the server said why — "already in this album" would be a lie. + ImmichToast.show( + context: context, + msg: 'assets_cannot_be_added_to_album_count'.t(context: context, args: {'count': result.failedCount}), + toastType: ToastType.error, + ); + return; + } ImmichToast.show( context: context, msg: result.count == 0 ? 'add_to_album_bottom_sheet_already_exists'.t(context: context, args: {'album': album.name}) : 'add_to_album_bottom_sheet_added'.t(context: context, args: {'album': album.name}), ); + widget.onCompleted?.call(); } Future _addToTarget(CollectionTarget target) async { @@ -62,14 +92,14 @@ class _CollectionPickerState extends ConsumerState { final String? successMessage; switch (target) { case AlbumTarget(:final album): - result = await notifier.addToAlbum(ActionSource.timeline, album); + result = await notifier.addToAlbum(widget.source, album); successMessage = null; case SpacePoolTarget(:final space): - result = await notifier.addToSpace(ActionSource.timeline, space); + result = await notifier.addToSpace(widget.source, space); // The pool endpoint is 204 with no body, so this count is the request length. successMessage = 'added_to_space_count'; case SpaceAlbumTarget(:final spaceId, :final album): - result = await notifier.addToSpaceAlbum(ActionSource.timeline, spaceId, album); + result = await notifier.addToSpaceAlbum(widget.source, spaceId, album); // This one IS the server's count, so duplicates are already excluded. successMessage = 'space_album_add_photos_success'; } @@ -88,6 +118,7 @@ class _CollectionPickerState extends ConsumerState { toastType: ToastType.success, ); } + widget.onCompleted?.call(); } void _toastError() { @@ -124,6 +155,7 @@ class _CollectionPickerState extends ConsumerState { excludeSpaceId: widget.excludeSpaceId, isBusy: _isBusy, searchQuery: _searchQuery, + assets: widget.assets, ), ), ], diff --git a/mobile/lib/presentation/widgets/collection/space_collection_section.widget.dart b/mobile/lib/presentation/widgets/collection/space_collection_section.widget.dart index a14bca4553522..5159bad316fec 100644 --- a/mobile/lib/presentation/widgets/collection/space_collection_section.widget.dart +++ b/mobile/lib/presentation/widgets/collection/space_collection_section.widget.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/collection.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/collection_target.dart'; import 'package:immich_mobile/domain/models/space_album.model.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; @@ -27,6 +28,7 @@ class SpaceCollectionSection extends ConsumerStatefulWidget { this.excludeSpaceId, this.isBusy = false, this.searchQuery = '', + this.assets, }); final void Function(CollectionTarget target) onTargetSelected; @@ -34,6 +36,13 @@ class SpaceCollectionSection extends ConsumerStatefulWidget { /// Set on a space's own surface so it is not offered as a destination for its own assets. final String? excludeSpaceId; + /// The assets the picker is about to file, for the ownership / cap notices. + /// + /// Defaults to the timeline multiselect. The asset viewer has no multiselect, so it passes + /// its one asset — falling back to the empty selection there would read as "nothing + /// non-owned" and offer space targets for a photo that can never reach one. + final Iterable? assets; + /// Disables every row while an add is in flight. final bool isBusy; @@ -73,7 +82,8 @@ class _SpaceCollectionSectionState extends ConsumerState Widget build(BuildContext context) { final spacesAsync = ref.watch(sharedSpacesProvider); final userId = ref.watch(currentUserProvider.select((user) => user?.id)); - final selection = ref.watch(multiSelectProvider.select((state) => state.selectedAssets)); + final multiSelection = ref.watch(multiSelectProvider.select((state) => state.selectedAssets)); + final selection = widget.assets ?? multiSelection; final spaces = spacesAsync.valueOrNull; // Offline or still loading: the album half of the picker still works, so stay out of diff --git a/mobile/test/presentation/widgets/bottom_sheet/add_to_collection_surfaces_test.dart b/mobile/test/presentation/widgets/bottom_sheet/add_to_collection_surfaces_test.dart new file mode 100644 index 0000000000000..6d7b71ed34532 --- /dev/null +++ b/mobile/test/presentation/widgets/bottom_sheet/add_to_collection_surfaces_test.dart @@ -0,0 +1,186 @@ +// #965: "add to a Shared Space album" used to depend on which screen you started from — +// only the surfaces mounting the fork's `CollectionPicker` offered spaces at all, and the +// rest mounted upstream's bare `AlbumSelector`. These tests pin the wiring: every +// add-to-collection surface mounts the one picker. +// +// The picker's own behaviour (which spaces, which albums, which dispatch) is covered by +// `collection/collection_picker_test.dart` and `collection/space_collection_section_test.dart`; +// what can silently regress here is a surface being left behind on the album-only selector. +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/domain/models/album/album.model.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; +import 'package:immich_mobile/domain/models/config/app_config.dart'; +import 'package:immich_mobile/domain/models/user.model.dart'; +import 'package:immich_mobile/domain/services/user.service.dart'; +import 'package:immich_mobile/constants/enums.dart'; +import 'package:immich_mobile/models/albums/album_search.model.dart'; +import 'package:immich_mobile/presentation/widgets/action_buttons/add_action_button.widget.dart'; +import 'package:immich_mobile/presentation/widgets/action_buttons/base_action_button.widget.dart'; +import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; +import 'package:immich_mobile/presentation/widgets/bottom_sheet/archive_bottom_sheet.widget.dart'; +import 'package:immich_mobile/presentation/widgets/bottom_sheet/base_bottom_sheet.widget.dart'; +import 'package:immich_mobile/presentation/widgets/bottom_sheet/favorite_bottom_sheet.widget.dart'; +import 'package:immich_mobile/presentation/widgets/bottom_sheet/local_album_bottom_sheet.widget.dart'; +import 'package:immich_mobile/presentation/widgets/bottom_sheet/remote_album_bottom_sheet.widget.dart'; +import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; +import 'package:immich_mobile/providers/asset_viewer/asset_viewer.provider.dart'; +import 'package:immich_mobile/providers/infrastructure/album.provider.dart'; +import 'package:immich_mobile/providers/infrastructure/remote_album.provider.dart'; +import 'package:immich_mobile/providers/infrastructure/settings.provider.dart'; +import 'package:immich_mobile/providers/routes.provider.dart'; +import 'package:immich_mobile/providers/server_info.provider.dart'; +import 'package:immich_mobile/services/server_info.service.dart'; +import 'package:immich_mobile/providers/shared_space.provider.dart'; +import 'package:immich_mobile/providers/timeline/multiselect.provider.dart'; +import 'package:immich_mobile/providers/user.provider.dart'; +import 'package:mocktail/mocktail.dart'; + +import '../../../fixtures/user.stub.dart'; +import '../../../unit/factories/remote_album_factory.dart'; +import '../../../widget_tester_extensions.dart'; + +class _MockUserService extends Mock implements UserService {} + +class _StubCurrentUserNotifier extends CurrentUserProvider { + _StubCurrentUserNotifier(super.service, UserDto? user) { + state = user; + } +} + +/// `AlbumSelector` fires a post-frame `refresh()` against a live `RemoteAlbumService` that +/// this harness has no reason to stand up; the picker composes it, so stub both. +class _StubRemoteAlbumNotifier extends RemoteAlbumNotifier { + @override + RemoteAlbumState build() => const RemoteAlbumState(albums: []); + + @override + Future refresh() async {} + + @override + List searchAlbums( + List albums, + String query, + String? userId, [ + QuickFilterMode filterMode = QuickFilterMode.all, + ]) => albums; +} + +class _MockServerInfoService extends Mock implements ServerInfoService {} + +class _StubAssetViewerNotifier extends AssetViewerStateNotifier { + _StubAssetViewerNotifier(this.asset); + + final BaseAsset asset; + + @override + AssetViewerState build() => AssetViewerState(currentAsset: asset); +} + +void main() { + final user = UserStub.user1; + + RemoteAsset asset(String id) => RemoteAsset( + id: id, + name: id, + ownerId: user.id, + checksum: id, + type: AssetType.image, + createdAt: DateTime(2026, 1, 1), + updatedAt: DateTime(2026, 1, 1), + isEdited: false, + ); + + RemoteAlbum ownedAlbum() => RemoteAlbumFactory.create(ownerId: user.id, ownerName: user.name, assetCount: 1); + + Future pumpSheet(WidgetTester tester, Widget sheet) async { + final userService = _MockUserService(); + when(() => userService.tryGetMyUser()).thenReturn(user); + when(() => userService.watchMyUser()).thenAnswer((_) => const Stream.empty()); + + // The localized helper, not the raw one: these sheets are mostly action buttons, and + // every one of them resolves an i18n key at build time. + await tester.pumpConsumerWidget( + sheet, + overrides: [ + currentUserProvider.overrideWith((ref) => _StubCurrentUserNotifier(userService, user)), + remoteAlbumProvider.overrideWith(() => _StubRemoteAlbumNotifier()), + appConfigProvider.overrideWithValue(const AppConfig()), + sharedSpacesProvider.overrideWith((ref) async => const []), + serverInfoProvider.overrideWith((ref) => ServerInfoNotifier(_MockServerInfoService())), + multiSelectProvider.overrideWith( + () => MultiSelectNotifier(MultiSelectState(selectedAssets: {asset('a')}, lockedSelectionAssets: const {})), + ), + ], + ); + await tester.pump(); + } + + /// These sheets open at 0.22–0.4 of the screen, so the add-to-collection slivers sit below + /// the viewport and a lazy sliver list never builds them — `find.byType` would report + /// nothing whichever picker is wired up. Assert on what the sheet was handed instead; + /// what the picker then renders is covered by the picker's own tests. + void expectPickerNotBareSelector(WidgetTester tester) { + final slivers = tester.widget(find.byType(BaseBottomSheet)).slivers ?? const []; + expect(slivers.whereType(), hasLength(1)); + // The bare selector is upstream's album-only picker: mounting it directly is the bug. + expect(slivers.whereType(), isEmpty); + } + + testWidgets('a selection inside an owned album offers spaces', (tester) async { + await pumpSheet(tester, RemoteAlbumBottomSheet(album: ownedAlbum())); + expectPickerNotBareSelector(tester); + }); + + testWidgets('a selection in favorites offers spaces', (tester) async { + await pumpSheet(tester, const FavoriteBottomSheet()); + expectPickerNotBareSelector(tester); + }); + + testWidgets('a selection in the archive offers spaces', (tester) async { + await pumpSheet(tester, const ArchiveBottomSheet()); + expectPickerNotBareSelector(tester); + }); + + testWidgets('a selection in an on-device album offers spaces', (tester) async { + await pumpSheet(tester, const LocalAlbumBottomSheet()); + expectPickerNotBareSelector(tester); + }); + + testWidgets('the asset viewer + button offers spaces, judged against the viewed asset', (tester) async { + final viewed = asset('viewed'); + final userService = _MockUserService(); + when(() => userService.tryGetMyUser()).thenReturn(user); + when(() => userService.watchMyUser()).thenAnswer((_) => const Stream.empty()); + + await tester.pumpConsumerWidget( + const AddActionButton(), + overrides: [ + currentUserProvider.overrideWith((ref) => _StubCurrentUserNotifier(userService, user)), + assetViewerProvider.overrideWith(() => _StubAssetViewerNotifier(viewed)), + // Reads the auto_route stack, which this harness has none of. + inLockedViewProvider.overrideWithValue(false), + remoteAlbumProvider.overrideWith(() => _StubRemoteAlbumNotifier()), + appConfigProvider.overrideWithValue(const AppConfig()), + sharedSpacesProvider.overrideWith((ref) async => const []), + multiSelectProvider.overrideWith( + () => MultiSelectNotifier(const MultiSelectState(selectedAssets: {}, lockedSelectionAssets: {})), + ), + ], + ); + + await tester.tap(find.byType(BaseActionButton).first); + await tester.pumpAndSettle(); + await tester.tap(find.widgetWithText(BaseActionButton, 'Album')); + await tester.pumpAndSettle(); + + final picker = tester + .widget(find.byType(BaseBottomSheet)) + .slivers! + .whereType() + .single; + expect(picker.source, ActionSource.viewer); + // The viewer has no multiselect, so it must state the asset the notices reason about. + expect(picker.assets, [viewed]); + }); +} diff --git a/mobile/test/presentation/widgets/collection/collection_picker_test.dart b/mobile/test/presentation/widgets/collection/collection_picker_test.dart index 0f91ffa833bef..ce825e4de6865 100644 --- a/mobile/test/presentation/widgets/collection/collection_picker_test.dart +++ b/mobile/test/presentation/widgets/collection/collection_picker_test.dart @@ -1,6 +1,9 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:immich_mobile/constants/enums.dart'; import 'package:immich_mobile/domain/models/album/album.model.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/config/app_config.dart'; import 'package:immich_mobile/domain/models/user.model.dart'; import 'package:immich_mobile/domain/services/user.service.dart'; @@ -8,6 +11,7 @@ import 'package:immich_mobile/models/albums/album_search.model.dart'; import 'package:immich_mobile/presentation/widgets/collection/collection_picker.widget.dart'; import 'package:immich_mobile/presentation/widgets/collection/space_collection_section.widget.dart'; import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart'; +import 'package:immich_mobile/providers/infrastructure/action.provider.dart'; import 'package:immich_mobile/providers/infrastructure/album.provider.dart'; import 'package:immich_mobile/providers/infrastructure/remote_album.provider.dart'; import 'package:immich_mobile/providers/infrastructure/settings.provider.dart'; @@ -54,6 +58,31 @@ class _StubRemoteAlbumNotifier extends RemoteAlbumNotifier { ]) => albums; } +/// Captures which [ActionSource] the picker dispatched against, and lets a test make the +/// dispatch fail, without standing up the real action plumbing. +class _RecordingActionNotifier extends ActionNotifier { + _RecordingActionNotifier({this.succeeds = true}); + + final bool succeeds; + final List albumSources = []; + final List spaceSources = []; + + @override + void build() {} + + @override + Future addToAlbum(ActionSource source, RemoteAlbum album) async { + albumSources.add(source); + return ActionResult(count: succeeds ? 1 : 0, success: succeeds); + } + + @override + Future addToSpace(ActionSource source, SharedSpaceResponseDto space) async { + spaceSources.add(source); + return ActionResult(count: succeeds ? 1 : 0, success: succeeds); + } +} + void main() { SharedSpaceMemberResponseDto member(String userId, SharedSpaceRole role) => SharedSpaceMemberResponseDto( userId: userId, @@ -147,4 +176,126 @@ void main() { expect(find.byKey(const Key('space-row-s2')), findsOneWidget); }); + + // #965: the same picker is now mounted from surfaces that have no timeline multiselect — + // the asset viewer above all — so the source it dispatches against and the assets it + // reasons about both have to be things the caller can state. + group('mounted outside the timeline', () { + RemoteAsset asset(String id, {String ownerId = 'user-1'}) => RemoteAsset( + id: id, + name: id, + ownerId: ownerId, + checksum: id, + type: AssetType.image, + createdAt: DateTime(2026, 1, 1), + updatedAt: DateTime(2026, 1, 1), + isEdited: false, + ); + + Future pumpPicker( + WidgetTester tester, { + required Widget picker, + List spaces = const [], + List extraOverrides = const [], + }) async { + final userService = _MockUserService(); + final user = UserStub.user1; // id: 'user-1' + when(() => userService.tryGetMyUser()).thenReturn(user); + when(() => userService.watchMyUser()).thenAnswer((_) => const Stream.empty()); + + await tester.pumpConsumerWidgetRaw( + CustomScrollView(slivers: [picker]), + overrides: [ + currentUserProvider.overrideWith((ref) => _StubCurrentUserNotifier(userService, user)), + remoteAlbumProvider.overrideWith(() => _StubRemoteAlbumNotifier()), + appConfigProvider.overrideWithValue(const AppConfig()), + sharedSpacesProvider.overrideWith((ref) async => spaces), + multiSelectProvider.overrideWith( + () => MultiSelectNotifier(const MultiSelectState(selectedAssets: {}, lockedSelectionAssets: {})), + ), + ...extraOverrides, + ], + ); + await tester.pump(); + } + + /// `ImmichToast` schedules a 3s fluttertoast Timer outside the frame scheduler, so a + /// plain `pumpAndSettle()` leaves it pending and teardown fails with "A Timer is still + /// pending". Pump past its lifetime instead. + Future tapSpaceRow(WidgetTester tester, String id) async { + await tester.tap(find.byKey(Key('space-row-$id'))); + await tester.pumpAndSettle(); + await tester.pump(const Duration(seconds: 4)); + await tester.pumpAndSettle(); + } + + testWidgets('judges space targets by the assets it was given, not the empty multiselect', (tester) async { + await pumpPicker( + tester, + picker: CollectionPicker(assets: [asset('a', ownerId: 'someone-else')]), + spaces: [space('s1', 'Family')], + ); + + expect(find.byKey(const Key('space-row-s1')), findsNothing); + expect(find.byKey(const Key('space-collection-notice')), findsOneWidget); + }); + + testWidgets('dispatches the space pool against the source it was given', (tester) async { + final notifier = _RecordingActionNotifier(); + await pumpPicker( + tester, + picker: CollectionPicker(source: ActionSource.viewer, assets: [asset('a')]), + spaces: [space('s1', 'Family')], + extraOverrides: [actionProvider.overrideWith(() => notifier)], + ); + + await tapSpaceRow(tester, 's1'); + + expect(notifier.spaceSources, [ActionSource.viewer]); + }); + + testWidgets('still defaults to the timeline source', (tester) async { + final notifier = _RecordingActionNotifier(); + await pumpPicker( + tester, + picker: const CollectionPicker(), + spaces: [space('s1', 'Family')], + extraOverrides: [actionProvider.overrideWith(() => notifier)], + ); + + await tapSpaceRow(tester, 's1'); + + expect(notifier.spaceSources, [ActionSource.timeline]); + }); + + testWidgets('reports completion only when the add succeeded', (tester) async { + var completions = 0; + final notifier = _RecordingActionNotifier(succeeds: true); + await pumpPicker( + tester, + picker: CollectionPicker(onCompleted: () => completions++), + spaces: [space('s1', 'Family')], + extraOverrides: [actionProvider.overrideWith(() => notifier)], + ); + + await tapSpaceRow(tester, 's1'); + + expect(completions, 1); + }); + + testWidgets('does not report completion when the add failed', (tester) async { + var completions = 0; + final notifier = _RecordingActionNotifier(succeeds: false); + await pumpPicker( + tester, + picker: CollectionPicker(onCompleted: () => completions++), + spaces: [space('s1', 'Family')], + extraOverrides: [actionProvider.overrideWith(() => notifier)], + ); + + await tapSpaceRow(tester, 's1'); + + expect(completions, 0, reason: 'the sheet must stay open so the user can retry'); + }); + }); } diff --git a/mobile/test/presentation/widgets/collection/space_collection_section_test.dart b/mobile/test/presentation/widgets/collection/space_collection_section_test.dart index cf57e54ebaae4..0cf0166aa9b4f 100644 --- a/mobile/test/presentation/widgets/collection/space_collection_section_test.dart +++ b/mobile/test/presentation/widgets/collection/space_collection_section_test.dart @@ -89,6 +89,7 @@ void main() { required List spaces, Map> albums = const {}, List? selection, + List? assets, String? excludeSpaceId, String? userId = 'user-1', String searchQuery = '', @@ -115,6 +116,7 @@ void main() { onTargetSelected: targets.add, excludeSpaceId: excludeSpaceId, searchQuery: searchQuery, + assets: assets, ); if (raw) { await tester.pumpConsumerWidgetRaw(widget, overrides: overrides); @@ -309,6 +311,35 @@ void main() { expect(find.byKey(const Key('space-row-s1')), findsNothing); }); + // #965: the asset viewer has no multiselect, so it hands the section the one asset the + // viewer is on. Falling back to the (empty) multiselect there would read as "nothing + // non-owned" and offer space targets for a photo that can never reach one. + testWidgets('an explicit asset list is used instead of the multiselect', (tester) async { + await pump( + tester, + spaces: [space('s1')], + selection: [asset('a')], // owned — on its own this would show the rows + assets: [asset('b', ownerId: 'other')], + ); + + expect( + find.text("Your selection includes photos owned by other members, so it can't be added to a space."), + findsOneWidget, + ); + expect(find.byKey(const Key('space-row-s1')), findsNothing); + }); + + testWidgets('an explicit owned asset list still offers the rows', (tester) async { + await pump( + tester, + spaces: [space('s1')], + selection: [asset('a', ownerId: 'other')], // ignored + assets: [asset('b')], + ); + + expect(find.byKey(const Key('space-row-s1')), findsOneWidget); + }); + testWidgets('an unknown current user hides the rows (fail closed)', (tester) async { await pump(tester, spaces: [space('s1')], userId: null); diff --git a/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.spec.ts b/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.spec.ts index 11454f7b57ebf..d46e684793d3b 100644 --- a/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.spec.ts +++ b/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.spec.ts @@ -201,6 +201,7 @@ describe('CollectionModalRowConverter', () => { expect(isSelectableRowType(CollectionModalRowType.NEW_ALBUM)).toBe(true); expect(isSelectableRowType(CollectionModalRowType.NEW_SPACE)).toBe(true); expect(isSelectableRowType(CollectionModalRowType.COLLECTION_ITEM)).toBe(true); + expect(isSelectableRowType(CollectionModalRowType.SPACE_POOL_CHILD)).toBe(true); expect(isSelectableRowType(CollectionModalRowType.SECTION)).toBe(false); expect(isSelectableRowType(CollectionModalRowType.MESSAGE)).toBe(false); }); @@ -214,6 +215,131 @@ describe('CollectionModalRowConverter', () => { expect(items[0].collection!.kind).toBe('album'); }); + // #965: a space row with linked albums expands into "Add to space" plus one row per linked + // album, mirroring mobile's SpaceCollectionSection. A linked album is an ordinary `album` + // collection, so the dispatch (POST /albums/:id/assets) is unchanged. + describe('space expansion (#965)', () => { + const withAlbums = (id: string, name: string, albumCount: number) => s(id, name, { albumCount }); + const items = (rows: ReturnType) => + rows.filter((r) => r.type === CollectionModalRowType.COLLECTION_ITEM); + + it('marks a space with linked albums expandable, and one without not', () => { + const all = [withAlbums('s1', 'Family', 2), s('s2', 'Empty', { albumCount: 0 }), s('s3', 'Unknown')]; + const rows = items(conv.toModalRows('', [], all, -1, [], opts)); + expect(rows.find((r) => r.collection!.id === 's1')!.expandable).toBe(true); + expect(rows.find((r) => r.collection!.id === 's2')!.expandable).toBe(false); + // albumCount absent (older server) — treated as "nothing to expand into". + expect(rows.find((r) => r.collection!.id === 's3')!.expandable).toBe(false); + }); + + it('emits no children until the space is the expanded one', () => { + const all = [withAlbums('s1', 'Family', 2)]; + const rows = conv.toModalRows('', [], all, -1, [], opts); + expect(rows.some((r) => r.type === CollectionModalRowType.SPACE_POOL_CHILD)).toBe(false); + expect(rows.find((r) => r.collection?.id === 's1')!.expanded).toBe(false); + }); + + it('expands into the pool child then one indented album row per linked album', () => { + const all = [withAlbums('s1', 'Family', 2)]; + const rows = conv.toModalRows('', [], all, -1, [], { + ...opts, + expandedSpaceId: 's1', + expandedSpaceAlbums: [a('sa1', 'Holiday'), a('sa2', 'Birthday')], + }); + const kinds = rows + .filter( + (r) => + r.type === CollectionModalRowType.COLLECTION_ITEM || r.type === CollectionModalRowType.SPACE_POOL_CHILD, + ) + .map((r) => `${r.type}:${r.collection!.id}`); + expect(kinds).toEqual([ + `${CollectionModalRowType.COLLECTION_ITEM}:s1`, + `${CollectionModalRowType.SPACE_POOL_CHILD}:s1`, + `${CollectionModalRowType.COLLECTION_ITEM}:sa1`, + `${CollectionModalRowType.COLLECTION_ITEM}:sa2`, + ]); + expect(rows.find((r) => r.collection?.id === 's1')!.expanded).toBe(true); + // The children are the ones that carry the indent, not the space row itself. + expect(rows.find((r) => r.type === CollectionModalRowType.SPACE_POOL_CHILD)!.indented).toBe(true); + expect(rows.find((r) => r.collection?.id === 'sa1')!.indented).toBe(true); + expect(rows.find((r) => r.collection?.id === 's1')!.indented).toBeFalsy(); + }); + + it('emits only the pool child while the albums are still loading', () => { + const all = [withAlbums('s1', 'Family', 2)]; + // `expandedSpaceAlbums` undefined == request in flight. Showing "no albums yet" here would + // be a lie that flashes on every expand. + const rows = conv.toModalRows('', [], all, -1, [], { ...opts, expandedSpaceId: 's1' }); + expect(rows.some((r) => r.type === CollectionModalRowType.SPACE_POOL_CHILD)).toBe(true); + expect(rows.some((r) => r.type === CollectionModalRowType.MESSAGE)).toBe(false); + expect(items(rows)).toHaveLength(1); // the space row only + }); + + it('explains an expanded space that turned out to have no linked albums', () => { + const all = [withAlbums('s1', 'Family', 2)]; + const rows = conv.toModalRows('', [], all, -1, [], { + ...opts, + expandedSpaceId: 's1', + expandedSpaceAlbums: [], + }); + const message = rows.find((r) => r.type === CollectionModalRowType.MESSAGE); + expect(message!.text).toBe('no_albums_in_space_yet'); + expect(message!.indented).toBe(true); + }); + + it('keeps arrow-key order flat: children take the indices right after their space', () => { + const all = [a('a1', 'Aardvark'), withAlbums('s1', 'Family', 1)]; + const selectedAt = (i: number) => + conv.toModalRows('', [], all, i, [], { + ...opts, + expandedSpaceId: 's1', + expandedSpaceAlbums: [a('sa1', 'Holiday')], + }); + // NewAlbum(0) NewSpace(1) a1(2) s1(3) pool(4) sa1(5) + expect(selectedAt(2).find((r) => r.selected && r.collection)!.collection!.id).toBe('a1'); + expect(selectedAt(3).find((r) => r.selected && r.collection)!.collection!.id).toBe('s1'); + const pool = selectedAt(4).find((r) => r.selected)!; + expect(pool.type).toBe(CollectionModalRowType.SPACE_POOL_CHILD); + expect(selectedAt(5).find((r) => r.selected && r.collection)!.collection!.id).toBe('sa1'); + }); + + it('multi-selects a linked album by its own key, and the pool by the space key', () => { + const all = [withAlbums('s1', 'Family', 1)]; + const rows = conv.toModalRows('', [], all, -1, ['album:sa1'], { + ...opts, + expandedSpaceId: 's1', + expandedSpaceAlbums: [a('sa1', 'Holiday')], + }); + expect(rows.find((r) => r.collection?.id === 'sa1')!.multiSelected).toBe(true); + expect(rows.find((r) => r.type === CollectionModalRowType.SPACE_POOL_CHILD)!.multiSelected).toBe(false); + }); + + it('renders the children under every occurrence of the space, RECENT included', () => { + const recent = [withAlbums('s1', 'Family', 1)]; + const all = [withAlbums('s1', 'Family', 1)]; + const rows = conv.toModalRows('', recent, all, -1, [], { + ...opts, + expandedSpaceId: 's1', + expandedSpaceAlbums: [a('sa1', 'Holiday')], + }); + // Same row, same affordance, in both sections — rather than the same row behaving + // differently depending on which section it was rendered in. + expect(rows.filter((r) => r.type === CollectionModalRowType.SPACE_POOL_CHILD)).toHaveLength(2); + expect(rows.filter((r) => r.collection?.id === 'sa1')).toHaveLength(2); + }); + + it('emits nothing space-related when spaces are hidden, even if a space is marked expanded', () => { + const all = [a('a1', 'A'), withAlbums('s1', 'Family', 1)]; + const rows = conv.toModalRows('', [], all, -1, [], { + showSpaces: false, + expandedSpaceId: 's1', + expandedSpaceAlbums: [a('sa1', 'Holiday')], + }); + expect(rows.some((r) => r.type === CollectionModalRowType.SPACE_POOL_CHILD)).toBe(false); + expect(items(rows).map((r) => r.collection!.id)).toEqual(['a1']); + }); + }); + // Restricted (space-contribution) mode: no create rows, and both empty-state messages // are overridable so they never name a collection type that was not on offer. describe('allowCreate / message overrides', () => { diff --git a/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.ts b/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.ts index e3e2fe8291266..aea5c522706bd 100644 --- a/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.ts +++ b/web/src/lib/components/shared-components/collection-selection/collection-selection-utils.ts @@ -71,6 +71,8 @@ export enum CollectionModalRowType { SECTION = 'section', MESSAGE = 'message', COLLECTION_ITEM = 'collectionItem', + /** "Add to space" — the space's own pool, offered as a child of an expanded space row (#965). */ + SPACE_POOL_CHILD = 'spacePoolChild', } export type CollectionModalRow = { @@ -79,12 +81,31 @@ export type CollectionModalRow = { multiSelected?: boolean; text?: string; collection?: PickerCollection; + /** Space rows only: the space has linked albums, so clicking it toggles instead of selecting. */ + expandable?: boolean; + /** Space rows only: this space is the currently-expanded one. */ + expanded?: boolean; + /** A child of an expanded space row — the pool, a linked album, or the empty-state message. */ + indented?: boolean; }; export const isSelectableRowType = (type: CollectionModalRowType): boolean => - [CollectionModalRowType.NEW_ALBUM, CollectionModalRowType.NEW_SPACE, CollectionModalRowType.COLLECTION_ITEM].includes( - type, - ); + [ + CollectionModalRowType.NEW_ALBUM, + CollectionModalRowType.NEW_SPACE, + CollectionModalRowType.COLLECTION_ITEM, + CollectionModalRowType.SPACE_POOL_CHILD, + ].includes(type); + +/** + * Whether a space row can expand into linked-album children. + * + * `albumCount` comes back on every `GET /shared-spaces` row, so this needs no extra request — + * that is the whole point of the accordion: a space's albums are fetched only once the user + * asks for them. An absent count (older server) reads as "nothing to expand into". + */ +const isExpandableSpace = (collection: PickerCollection): boolean => + collection.kind === 'space' && (collection.space.albumCount ?? 0) > 0; export class CollectionModalRowConverter { toModalRows( @@ -93,7 +114,19 @@ export class CollectionModalRowConverter { all: PickerCollection[], selectedRowIndex: number, multiSelectedKeys: string[], - options: { showSpaces: boolean; allowCreate?: boolean; emptyText?: string; noMatchText?: string }, + options: { + showSpaces: boolean; + allowCreate?: boolean; + emptyText?: string; + noMatchText?: string; + /** The one space currently expanded into its linked albums, if any (#965). */ + expandedSpaceId?: string | null; + /** + * The expanded space's linked albums. `undefined` means the request is still in flight — + * distinct from `[]`, which means the space genuinely has none. + */ + expandedSpaceAlbums?: PickerCollection[]; + }, ): CollectionModalRow[] { const $t = get(t); // Restricted mode passes allowCreate:false — a freshly created album is not linked to the @@ -124,14 +157,45 @@ export class CollectionModalRowConverter { } let index = createCount; + const pushSelectable = (row: CollectionModalRow) => { + rows.push({ ...row, selected: selectedRowIndex === index }); + index++; + }; const pushItem = (c: PickerCollection) => { - rows.push({ + pushSelectable({ type: CollectionModalRowType.COLLECTION_ITEM, - selected: selectedRowIndex === index, multiSelected: multiSelectedKeys.includes(collectionKey(c)), collection: c, + expandable: isExpandableSpace(c), + expanded: c.kind === 'space' && c.id === options.expandedSpaceId, }); - index++; + if (c.kind !== 'space' || c.id !== options.expandedSpaceId) { + return; + } + // The pool keeps the space's own collection key, so ticking the parent row and ticking + // "Add to space" are the same multi-select — they name the same destination. + pushSelectable({ + type: CollectionModalRowType.SPACE_POOL_CHILD, + multiSelected: multiSelectedKeys.includes(collectionKey(c)), + collection: c, + indented: true, + }); + const linked = options.expandedSpaceAlbums; + if (linked === undefined) { + return; // still loading — anything else here would be a claim we cannot back yet + } + if (linked.length === 0) { + rows.push({ type: CollectionModalRowType.MESSAGE, text: $t('no_albums_in_space_yet'), indented: true }); + return; + } + for (const child of linked) { + pushSelectable({ + type: CollectionModalRowType.COLLECTION_ITEM, + multiSelected: multiSelectedKeys.includes(collectionKey(child)), + collection: child, + indented: true, + }); + } }; if (recentToShow.length > 0) { diff --git a/web/src/lib/components/shared-components/collection-selection/space-list-item.svelte b/web/src/lib/components/shared-components/collection-selection/space-list-item.svelte index eb9b875398418..6a38c94f6429d 100644 --- a/web/src/lib/components/shared-components/collection-selection/space-list-item.svelte +++ b/web/src/lib/components/shared-components/collection-selection/space-list-item.svelte @@ -6,7 +6,7 @@ import { normalizeSearchString } from '$lib/utils/string-utils'; import type { SharedSpaceResponseDto } from '@immich/sdk'; import { Icon } from '@immich/ui'; - import { mdiAccountMultipleOutline, mdiCheckCircle } from '@mdi/js'; + import { mdiAccountMultipleOutline, mdiCheckCircle, mdiChevronDown, mdiChevronUp } from '@mdi/js'; import type { Action } from 'svelte/action'; import { t } from 'svelte-i18n'; @@ -15,6 +15,9 @@ searchQuery?: string; selected: boolean; multiSelected?: boolean; + /** The space has linked albums, so clicking the row opens them instead of picking the pool. */ + expandable?: boolean; + expanded?: boolean; onSpaceClick: () => void; onMultiSelect: () => void; } @@ -24,6 +27,8 @@ searchQuery = '', selected = false, multiSelected = false, + expandable = false, + expanded = false, onSpaceClick, onMultiSelect, }: Props = $props(); @@ -86,6 +91,7 @@ class:dark:bg-gray-700={selected} use:longPress={{ onLongPress: () => handleMultiSelectClicked() }} data-testid="space-row" + aria-expanded={expandable ? expanded : undefined} > @@ -110,6 +116,11 @@ {/if} + {#if expandable} + + + + {/if} {#if mouseOver || multiSelected} diff --git a/web/src/lib/modals/CollectionPickerModal.spec.ts b/web/src/lib/modals/CollectionPickerModal.spec.ts index d2ece5949be30..5d7556e9b41d4 100644 --- a/web/src/lib/modals/CollectionPickerModal.spec.ts +++ b/web/src/lib/modals/CollectionPickerModal.spec.ts @@ -135,6 +135,150 @@ describe('CollectionPickerModal', () => { }); }); +// --------------------------------------------------------------------------- +// #965: a space with linked albums expands into "Add to space" plus one row per +// linked album, so a specific space album is reachable from every surface — the +// same accordion mobile's SpaceCollectionSection already offers. +// --------------------------------------------------------------------------- + +describe('CollectionPickerModal — expanding a space into its albums', () => { + const spaceWithAlbums = (id: string, name: string, albumCount: number) => + ({ ...space(id, name), albumCount }) as unknown as SharedSpaceResponseDto; + const linkedAlbum = (id: string, name: string) => + ({ + id, + albumName: name, + assetCount: 2, + albumThumbnailAssetId: null, + shared: true, + updatedAt: '2024-01-01T00:00:00Z', + ownerId: 'someone-else', + showInTimeline: true, + addedById: 'me', + linkedAt: '2024-01-01T00:00:00Z', + }) as never; + + // A space shows up in both RECENT and All, so every row lookup takes the first occurrence. + const expandSpace = async (id: string) => { + const rows = await screen.findAllByTestId(`row-space-${id}`); + await fireEvent.click(within(rows[0]).getByTestId('space-row')); + }; + + it('marks a space with linked albums expandable without fetching them up front', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 2)]); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + const rows = await screen.findAllByTestId('row-space-s1'); + expect(within(rows[0]).getByTestId('space-row').getAttribute('aria-expanded')).toBe('false'); + // The whole point of the accordion: albumCount already says it is expandable. + expect(sdkMock.getSharedSpaceAlbums).not.toHaveBeenCalled(); + }); + + it('fetches and lists the linked albums plus the pool child on expand', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 2)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday'), linkedAlbum('sa2', 'Birthday')]); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + await expandSpace('s1'); + + await waitFor(() => expect(screen.getAllByTestId('row-album-sa1').length).toBeGreaterThan(0)); + expect(sdkMock.getSharedSpaceAlbums).toHaveBeenCalledWith({ id: 's1' }); + expect(screen.getAllByTestId('row-album-sa2').length).toBeGreaterThan(0); + expect(screen.getAllByTestId('space-pool-child-s1').length).toBeGreaterThan(0); + }); + + it('confirms with the linked album when one is chosen', async () => { + const onClose = vi.fn(); + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday')]); + render(CollectionPickerModal, { assetCount: 3, onClose }); + + await expandSpace('s1'); + const albumRows = await screen.findAllByTestId('row-album-sa1'); + await fireEvent.click(within(albumRows[0]).getByRole('button', { name: /Holiday/ })); + + expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'album', id: 'sa1' })]); + }); + + it('confirms with the space itself when the pool child is chosen', async () => { + const onClose = vi.fn(); + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday')]); + render(CollectionPickerModal, { assetCount: 3, onClose }); + + await expandSpace('s1'); + const poolRows = await screen.findAllByTestId('space-pool-child-s1'); + await fireEvent.click(poolRows[0]); + + expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'space', id: 's1' })]); + }); + + it('keeps one space open at a time and does not re-fetch a space it already loaded', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1), spaceWithAlbums('s2', 'Friends', 1)]); + sdkMock.getSharedSpaceAlbums.mockImplementation(({ id }: { id: string }) => + Promise.resolve(id === 's1' ? [linkedAlbum('sa1', 'Holiday')] : [linkedAlbum('sa2', 'Birthday')]), + ); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + await expandSpace('s1'); + await waitFor(() => expect(screen.getAllByTestId('row-album-sa1').length).toBeGreaterThan(0)); + + await expandSpace('s2'); + await waitFor(() => expect(screen.getAllByTestId('row-album-sa2').length).toBeGreaterThan(0)); + expect(screen.queryAllByTestId('row-album-sa1')).toHaveLength(0); + + await expandSpace('s1'); + await waitFor(() => expect(screen.getAllByTestId('row-album-sa1').length).toBeGreaterThan(0)); + expect(sdkMock.getSharedSpaceAlbums).toHaveBeenCalledTimes(2); // s1 and s2, once each + }); + + it('collapses again on a second click', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday')]); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + await expandSpace('s1'); + await waitFor(() => expect(screen.getAllByTestId('row-album-sa1').length).toBeGreaterThan(0)); + await expandSpace('s1'); + await waitFor(() => expect(screen.queryAllByTestId('row-album-sa1')).toHaveLength(0)); + }); + + it('still adds straight to the pool for a space with no linked albums', async () => { + const onClose = vi.fn(); + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 0)]); + render(CollectionPickerModal, { assetCount: 3, onClose }); + + await expandSpace('s1'); + + expect(sdkMock.getSharedSpaceAlbums).not.toHaveBeenCalled(); + expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'space', id: 's1' })]); + }); + + it('reports a failed album load and leaves the row collapsed', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 2)]); + sdkMock.getSharedSpaceAlbums.mockRejectedValue(new Error('boom')); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + await expandSpace('s1'); + + await waitFor(() => expect(mockHandleError).toHaveBeenCalledOnce()); + const rows = screen.getAllByTestId('row-space-s1'); + expect(within(rows[0]).getByTestId('space-row').getAttribute('aria-expanded')).toBe('false'); + expect(screen.queryAllByTestId('space-pool-child-s1')).toHaveLength(0); + }); + + it('says so when an expanded space turns out to have no linked albums', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 2)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([]); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + await expandSpace('s1'); + + // Raw i18n key in unit tests. + await waitFor(() => expect(screen.getAllByText('no_albums_in_space_yet').length).toBeGreaterThan(0)); + }); +}); + // --------------------------------------------------------------------------- // Restricted mode: the selection contains assets the user does not own, so the // only targets that can accept the whole selection are albums linked to THIS diff --git a/web/src/lib/modals/CollectionPickerModal.svelte b/web/src/lib/modals/CollectionPickerModal.svelte index 597af4fd1ee8f..f96c5966a4954 100644 --- a/web/src/lib/modals/CollectionPickerModal.svelte +++ b/web/src/lib/modals/CollectionPickerModal.svelte @@ -1,5 +1,6 @@ + +
{ + if (!usingMobileDevice) { + mouseOver = true; + } + }} + onmouseleave={() => (mouseOver = false)} +> + + + {#if mouseOver || multiSelected} + + {/if} +
diff --git a/web/src/lib/modals/CollectionPickerModal.spec.ts b/web/src/lib/modals/CollectionPickerModal.spec.ts index 5d7556e9b41d4..98a5d790e50cd 100644 --- a/web/src/lib/modals/CollectionPickerModal.spec.ts +++ b/web/src/lib/modals/CollectionPickerModal.spec.ts @@ -254,6 +254,30 @@ describe('CollectionPickerModal — expanding a space into its albums', () => { expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'space', id: 's1' })]); }); + // The caret is an index into a row list that changes shape when a space opens. Clearing it + // (or leaving it pointing at a shifted row) stranded a keyboard user: they could open a space + // and then had no way to arrow into the children they had just revealed. + it('keeps the keyboard caret on the space row across expand and collapse', async () => { + const onClose = vi.fn(); + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday')]); + render(CollectionPickerModal, { assetCount: 3, onClose }); + await screen.findAllByTestId('row-space-s1'); + const search = screen.getByPlaceholderText('search'); + + // NewAlbum, NewSpace, then the RECENT occurrence of the space. + await fireEvent.keyDown(search, { key: 'ArrowDown' }); + await fireEvent.keyDown(search, { key: 'ArrowDown' }); + await fireEvent.keyDown(search, { key: 'ArrowDown' }); + await fireEvent.keyDown(search, { key: 'Enter' }); // expand + await waitFor(() => expect(screen.getAllByTestId('row-album-sa1').length).toBeGreaterThan(0)); + + // The very next ArrowDown must reach the pool child, not jump back to the top of the list. + await fireEvent.keyDown(search, { key: 'ArrowDown' }); + await fireEvent.keyDown(search, { key: 'Enter' }); + expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'space', id: 's1' })]); + }); + it('reports a failed album load and leaves the row collapsed', async () => { sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 2)]); sdkMock.getSharedSpaceAlbums.mockRejectedValue(new Error('boom')); @@ -267,6 +291,65 @@ describe('CollectionPickerModal — expanding a space into its albums', () => { expect(screen.queryAllByTestId('space-pool-child-s1')).toHaveLength(0); }); + // A space-linked album owned by another member has no `album_user` row for the caller, so + // `getAllAlbums` never returns it — it exists only in the expand-time cache. Resolving + // multi-select keys against the album list alone dropped it and closed the modal as a cancel. + it('multi-selects a linked album and actually submits it', async () => { + const onClose = vi.fn(); + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday')]); + render(CollectionPickerModal, { assetCount: 3, onClose }); + + await expandSpace('s1'); + const albumRow = (await screen.findAllByTestId('row-album-sa1'))[0]; + await fireEvent.mouseEnter(within(albumRow).getByRole('group')); + await fireEvent.click(within(albumRow).getByRole('checkbox')); + await fireEvent.click(await screen.findByTestId('add-collections-button')); + + expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'album', id: 'sa1' })]); + }); + + it('multi-selects the pool child, and shows the tick on it', async () => { + const onClose = vi.fn(); + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + sdkMock.getSharedSpaceAlbums.mockResolvedValue([linkedAlbum('sa1', 'Holiday')]); + render(CollectionPickerModal, { assetCount: 3, onClose }); + + await expandSpace('s1'); + const poolRow = (await screen.findAllByTestId('space-pool-child-s1'))[0].closest('[role="group"]')!; + await fireEvent.mouseEnter(poolRow); + await fireEvent.click(within(poolRow as HTMLElement).getByRole('checkbox')); + + // The pool carries the space's own key, so the tick must be visible on the child too — + // not merely computed in the row model. + expect( + within(poolRow as HTMLElement) + .getByRole('checkbox') + .getAttribute('aria-checked'), + ).toBe('true'); + await fireEvent.click(await screen.findByTestId('add-collections-button')); + expect(onClose).toHaveBeenCalledWith([expect.objectContaining({ kind: 'space', id: 's1' })]); + }); + + it('does not fire a second request when re-expanded while the first is still in flight', async () => { + sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 1)]); + let resolveFetch: (albums: never[]) => void = () => {}; + sdkMock.getSharedSpaceAlbums.mockReturnValue( + new Promise((resolve) => { + resolveFetch = resolve; + }), + ); + render(CollectionPickerModal, { assetCount: 3, onClose: vi.fn() }); + + await expandSpace('s1'); // fetch starts + await expandSpace('s1'); // collapse + await expandSpace('s1'); // re-expand before the first response lands + resolveFetch([linkedAlbum('sa1', 'Holiday')]); + + await waitFor(() => expect(screen.getAllByTestId('row-album-sa1').length).toBeGreaterThan(0)); + expect(sdkMock.getSharedSpaceAlbums).toHaveBeenCalledTimes(1); + }); + it('says so when an expanded space turns out to have no linked albums', async () => { sdkMock.getAllSpaces.mockResolvedValue([spaceWithAlbums('s1', 'Family', 2)]); sdkMock.getSharedSpaceAlbums.mockResolvedValue([]); diff --git a/web/src/lib/modals/CollectionPickerModal.svelte b/web/src/lib/modals/CollectionPickerModal.svelte index f96c5966a4954..a0a09cc10374b 100644 --- a/web/src/lib/modals/CollectionPickerModal.svelte +++ b/web/src/lib/modals/CollectionPickerModal.svelte @@ -1,6 +1,5 @@