Skip to content

refactor(web): one person-scope module for personal vs space people - #1160

Merged
Deeds67 merged 4 commits into
open-noodle:mainfrom
luminoso:refactor/web-person-scope
Oct 6, 2026
Merged

Deeds67 merged 4 commits into
open-noodle:mainfrom
luminoso:refactor/web-person-scope

Conversation

@luminoso

@luminoso luminoso commented Oct 4, 2026

Copy link
Copy Markdown

Summary

The web app built /shared-spaces/{spaceId}/people/{id}/thumbnail URLs by hand in 16 places. Those sites disagreed on which field holds the space person id: primaryProfile.id, spacePersonId, the raw row id, or spaceId ?? space.id. This PR moves that decision into web/src/lib/utils/global-person-route.ts, and every thumbnail site and every write in person.service.ts now goes through it. It also removes the People page's second hide implementation. The page had its own hide and favorite handlers that called the owner-only updatePerson directly, and they were safe only because the action menu is shown for personal people alone. The page now uses the space-aware actions that person.service.ts already had.

The id rule

  • A space person is a (spaceId, id) pair, returned by getSpaceProfile(person, spaceId?).
  • The pair comes from spacePersonId, but only when the caller passes the space it is viewing through. Today that is only the asset detail Info panel.
  • Otherwise it comes from a space-person primaryProfile that has both id and spaceId. This covers people lists and search, plus filter suggestions outside a space.
  • A SharedSpacePersonResponseDto row already is the pair. Its spaceId is required by the API, so the space.id fallbacks are gone.
  • Inside a space, filter suggestion and smart-search facet rows are that space's people, keyed by their own id. getPhotosPersonFilterThumbnailUrl(p, spaceId) owns this rule.
  • Anything else is personal and is addressed by primaryProfile?.id ?? id.

Changes

  • global-person-route.ts (fork-only) has getSpaceProfile, getSpacePersonThumbnailUrl(row, updatedAt?) and getGlobalPersonThumbnailUrl(person, { spaceId?, updatedAt? }), next to the existing getGlobalPersonHref.
  • person.service.ts (upstream) drops its private getSpaceProfile and imports the shared one. Write routing is unchanged.
  • The 15 other hand-built sites now call the module. Global search: person row, person preview, live typed filter section. Utils: photos-filter-options, map-filter-config, space-search, person-avatar. Components and routes: manage-space-people-visibility, the space timeline page, the space people page, the space person detail page (4 sites), and the person detail page (upstream).
  • People page (upstream): handleHidePerson and handleToggleFavorite are deleted. The menu renders HidePerson, Favorite and Unfavorite from getPersonActions. The list refreshes through the existing PersonUpdate event. canShowActions={isPersonalPrimary} is unchanged, because the menu's merge option is personal-only.
  • Tests: global-person-route.spec.ts is now one table-driven spec. It runs personal and space rows (each also with isFavorite: true) through the id and thumbnail URL, which menu actions are offered, and which endpoint hide and rename call. It also checks that the other endpoint is never called. Overlapping tests were removed from person.service.spec.ts and photos-filter-options.spec.ts.

Behaviour changes

  • The People page favorite error toast now comes from the service, which always passes favorite: false. A failed add therefore reads as a failed remove. The deleted page handler had the inverse mistake for removes. See follow-ups.
  • The People page no longer shows a Hide entry for a person who is already hidden. In practice this cannot happen, because the grid lists only visible people and search excludes hidden ones.
  • getSpaceProfile requires primaryProfile.id. A space-person profile with no id, which is only possible through the loosely typed typed-search preview, now gets a personal /people/{id}/thumbnail URL instead of /shared-spaces/{spaceId}/people/undefined/thumbnail. Both 404; the new one avoids building a broken URL. This is intended.
  • No URL changes for any input the server actually sends. Space-person rows now take spaceId from the row instead of the route, and the server guarantees the two match.

Testing

Run from web/ after building packages/sdk (pnpm build), which @immich/sdk imports need.

  • pnpm test --run src/lib/utils/global-person-route.spec.ts: 16 passed.
  • Touched specs (module, person.service, photos-filter-options, person-avatar, map-filter-config, space-search, People page, live typed filter section, src/lib/components/spaces, space people routes): 34 files, 664 tests passed.
  • Full web unit suite, run on the first two commits: 391 files passed, 1 skipped; 6407 tests passed.
  • pnpm check:svelte: 640 files, 0 errors, 0 warnings. pnpm check:typescript: exit 0.
  • npx eslint . --max-warnings 0 --concurrency 2: exit 0. pnpm lint (concurrency 6) crashed locally with Atomics.wait() failed: timed-out inside the tailwind ESLint plugin, on an untouched file. That is a load issue on this machine, not a lint finding, and lower concurrency passes.
  • The commit message autolink grep from CLAUDE.md prints nothing.

Follow-ups

  • The People page still spells the scope rule inline in isPersonalPrimary, isSpacePrimary and the space-editability $effect. The same applies to the person detail page, and the same check appears in global-search-manager.svelte.ts. These were left alone to avoid colliding with the space-role PR, which rewrites the $effect. Once both land, they can call getSpaceProfile.
  • web/src/lib/utils/scoped-person-ref.ts (toScopedPersonRef, isSpaceScopedPerson) is a second module that answers "is this a space person", and it keeps a spaceId ?? fallbackSpaceId fallback. It could be rebuilt on getSpaceProfile.
  • Upstream bug: handleFavoritePerson and handleUnfavoritePerson in person.service.ts both pass { favorite: false } to errors.unable_to_add_remove_favorites, so a failed add reads as a failed remove. This is identical in upstream Immich, so it is not patched here.
  • Mobile still has its own person-scope branches (Person.spaceId routing, image_url_builder.dart). Out of scope for this web PR.

luminoso and others added 4 commits October 6, 2026 08:47
global-person-route.ts now decides once which (spaceId, id) a person
resolves to: spacePersonId inside a known space, else a space-person
primaryProfile; a shared-space person row already is that pair. Every
hand-built /shared-spaces/.../people/.../thumbnail URL goes through
getSpacePersonThumbnailUrl / getGlobalPersonThumbnailUrl, and
person.service.ts reuses the same getSpaceProfile for its writes.

Replaces the per-site URL tests with one table-driven spec covering
personal and space rows for id, thumbnail, hide, favorite and rename.
The page had its own hide and favorite handlers calling the owner-only
updatePerson, safe only because the action menu is gated to personal
people. Use the HidePerson / Favorite / Unfavorite actions from
person.service.ts instead, so the page has one hide implementation.
The canShowActions gating is unchanged.
- Action table checks Unfavorite too, adds favorited personal and
  space rows, and asserts the other endpoint is never called.
- The write-routing comment moves from person.service.ts into the
  global-person-route.ts header, with a note on the profile id guard.
- Drop a redundant updatedAt argument and wrap the merge selector's
  thumbnail callback so a future map() cannot pass an index as updatedAt.
Drop people-utils' positional getSpacePersonThumbnailUrl, added on main
alongside this branch, and point SpacePersonSidePanel and SpaceFaceEditor
at the person-scope module's helper. Same URL, one definition.
@Deeds67
Deeds67 force-pushed the refactor/web-person-scope branch from 391b717 to d367962 Compare October 6, 2026 06:49
@Deeds67 Deeds67 added the changelog:chore Chore/maintenance for changelog label Oct 6, 2026
@Deeds67
Deeds67 merged commit 25b7d2c into open-noodle:main Oct 6, 2026
52 of 53 checks passed
Deeds67 added a commit that referenced this pull request Oct 6, 2026
…1160)

* refactor(web): one person-scope module for space vs personal people

global-person-route.ts now decides once which (spaceId, id) a person
resolves to: spacePersonId inside a known space, else a space-person
primaryProfile; a shared-space person row already is that pair. Every
hand-built /shared-spaces/.../people/.../thumbnail URL goes through
getSpacePersonThumbnailUrl / getGlobalPersonThumbnailUrl, and
person.service.ts reuses the same getSpaceProfile for its writes.

Replaces the per-site URL tests with one table-driven spec covering
personal and space rows for id, thumbnail, hide, favorite and rename.

* refactor(web): use the space-aware person actions on the People page

The page had its own hide and favorite handlers calling the owner-only
updatePerson, safe only because the action menu is gated to personal
people. Use the HidePerson / Favorite / Unfavorite actions from
person.service.ts instead, so the page has one hide implementation.
The canShowActions gating is unchanged.

* refactor(web): fold review into the person-scope module

- Action table checks Unfavorite too, adds favorited personal and
  space rows, and asserts the other endpoint is never called.
- The write-routing comment moves from person.service.ts into the
  global-person-route.ts header, with a note on the profile id guard.
- Drop a redundant updatedAt argument and wrap the merge selector's
  thumbnail callback so a future map() cannot pass an index as updatedAt.

* refactor(web): one space-person thumbnail helper

Drop people-utils' positional getSpacePersonThumbnailUrl, added on main
alongside this branch, and point SpacePersonSidePanel and SpaceFaceEditor
at the person-scope module's helper. Same URL, one definition.

---------

Co-authored-by: Pierre Marais <pierremarais67@gmail.com>
Deeds67 added a commit that referenced this pull request Oct 6, 2026
…1160)

* refactor(web): one person-scope module for space vs personal people

global-person-route.ts now decides once which (spaceId, id) a person
resolves to: spacePersonId inside a known space, else a space-person
primaryProfile; a shared-space person row already is that pair. Every
hand-built /shared-spaces/.../people/.../thumbnail URL goes through
getSpacePersonThumbnailUrl / getGlobalPersonThumbnailUrl, and
person.service.ts reuses the same getSpaceProfile for its writes.

Replaces the per-site URL tests with one table-driven spec covering
personal and space rows for id, thumbnail, hide, favorite and rename.

* refactor(web): use the space-aware person actions on the People page

The page had its own hide and favorite handlers calling the owner-only
updatePerson, safe only because the action menu is gated to personal
people. Use the HidePerson / Favorite / Unfavorite actions from
person.service.ts instead, so the page has one hide implementation.
The canShowActions gating is unchanged.

* refactor(web): fold review into the person-scope module

- Action table checks Unfavorite too, adds favorited personal and
  space rows, and asserts the other endpoint is never called.
- The write-routing comment moves from person.service.ts into the
  global-person-route.ts header, with a note on the profile id guard.
- Drop a redundant updatedAt argument and wrap the merge selector's
  thumbnail callback so a future map() cannot pass an index as updatedAt.

* refactor(web): one space-person thumbnail helper

Drop people-utils' positional getSpacePersonThumbnailUrl, added on main
alongside this branch, and point SpacePersonSidePanel and SpaceFaceEditor
at the person-scope module's helper. Same URL, one definition.

---------

Co-authored-by: Pierre Marais <pierremarais67@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:chore Chore/maintenance for changelog 🖥️web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants