Skip to content

refactor: read the viewer's space role from the spaces list on web and mobile - #1161

Open
luminoso wants to merge 3 commits into
open-noodle:mainfrom
luminoso:refactor/space-role-from-spaces-list
Open

luminoso wants to merge 3 commits into
open-noodle:mainfrom
luminoso:refactor/space-role-from-spaces-list

Conversation

@luminoso

@luminoso luminoso commented Oct 4, 2026

Copy link
Copy Markdown

Summary

To decide whether the viewer may rename or edit a person who belongs to a shared space, both clients called getMembers once per space, even though the spaces list they already load (getAllSpaces on web, getAll on mobile) carries every space's members. Both clients also showed edit controls until that extra call answered, and kept them if it failed, so a viewer could briefly see controls that the server would reject. This PR reads the viewer's own member role from the spaces list instead, deletes the extra call, and fails closed: space people stay read-only until the role is known, or if the list cannot be loaded. There is no server change, and the server still enforces the role on every write.

Where the role comes from

  • Web: the userInteraction.recentSpaces cache the sidebar already used. A new loadSpaces() in user.svelte.ts fills it with one getAllSpaces call, and the sidebar, the People page and the person page all share that fetch.
  • Mobile: the existing sharedSpacesProvider, so the answer follows that provider's refreshes and login changes.

Changes

  • web/src/lib/services/person.service.ts: isSpaceEditor(spaces, spaceId, userId) is now synchronous. It returns true when the viewer's own member row in that space is Owner or Editor. The per-space getMembers cache and the fail-open fallback are gone.
  • web/src/lib/stores/user.svelte.ts: new loadSpaces(), deduplicated while a request is in flight. On failure it shows the existing "failed to load spaces" toast and leaves the cache empty, and the next call retries.
  • web/src/routes/(user)/people/+page.svelte and people/[personId]/.../+page.svelte: call loadSpaces() from an effect and work out editability from the cached list. Edit controls appear once the list arrives.
  • web/src/lib/components/shared-components/side-bar/recent-spaces.svelte: uses loadSpaces() instead of its own fetch, so emptying the cache now triggers one getAllSpaces call instead of two. It keeps showing the previous list while the new one loads.
  • mobile/lib/providers/infrastructure/people.provider.dart: driftSpaceEditableProvider reads sharedSpacesProvider through spaceIsWritable, and never errors: a failed list counts as not editable.
  • mobile/lib/repositories/shared_space_api.repository.dart: deleted isSpaceEditor, which no longer has callers.
  • mobile/.../person.page.dart and people_grid.widget.dart: .value ?? true becomes .valueOrNull ?? false.
  • CLAUDE.md: the Mobile section's description of driftSpaceEditableProvider now matches this behaviour.
  • Tests: updated the person service, user store, People page and person page specs, plus the mobile provider test. The mobile isSpaceEditor repository tests were deleted.

Behaviour changes

  • Fail closed instead of fail open. While the spaces list is loading, or if it fails to load, people in a shared space have no rename, birthday, hide, merge, representative-face or suggestion controls, on both clients. Personal people are unaffected.
  • Web: if getAllSpaces fails on the People page or the person page, the user now sees the "failed to load spaces" toast, which previously only the sidebar showed. The page stays read-only for space people until the next retry. The next retry happens when the people list changes or when the user navigates.
  • Mobile: spaceIsWritable also treats the space creator as writable. That makes no difference here, because the server always makes the creator an Owner member and refuses to demote or remove them. Web checks the role alone, so both clients follow the same rule.
  • Album targets on mobile (album_permissions.dart) still fail open on purpose. Hiding a valid album as a target has a different cost from hiding a rename button.

Testing

  • web/: npx vitest run on the touched specs (person service, user store, sidebar, people routes, space albums page). Result: 188 passed in 12 files, plus 69 passed in space-albums-page.spec.ts.
  • web/: pnpm check:svelte reports 640 files with 0 errors and 0 warnings. pnpm check:typescript is clean. npx eslint . --max-warnings 0 --concurrency 2 is clean.
  • web/: full unit suite (pnpm test -- --run): 6406 passed and 1 failed. The failure was a global-search test, and a different one in each of two full runs (global-search.spec.ts "allows ArrowDown..." and then global-search-manager.svelte.spec.ts "clears stale status..."). Both spec files pass 525/525 in isolation, three times in a row, and global search does not use any module this PR changes. I read this as timing flakiness under full-suite load.
  • Mobile: flutter test was not run locally because Flutter is not installed on the machine this was written on. The Dart changes rely on CI and a line-by-line review. test/providers/infrastructure/space_editable_provider_test.dart covers owner, editor, viewer, a space that is not in the list, no user, a failed list, and loading.
  • git merge-tree against the parallel web person-scope branch is clean in both merge orders.

Follow-ups

  • Web does not refresh roles when they change. userInteraction.recentSpaces is cleared on space asset and album events and on logout, but not when a member's role changes, so a promotion or demotion made elsewhere shows up only after one of those events or a reload. The old per-space cache never refreshed either, so this is not a regression. Clearing the cache on a member-role event would fix it.
  • mobile/lib/utils/space_permissions.dart says getAll does not guarantee that the creator appears in members. The server says otherwise, so that comment and the creator short-circuit could be revisited separately.

The People page and the person detail page asked getMembers once per space
to decide whether a space person is editable, and offered edit controls
until the answer arrived (fail open). getAllSpaces already embeds members,
and the app already caches that list in userInteraction.recentSpaces for the
sidebar.

isSpaceEditor now reads the role from that cached list, and loadSpaces fills
the cache with one getAllSpaces call when it is empty. Until the list is
known, space people are read-only (fail closed); the controls appear
reactively once it loads. The server still enforces the role on every write.
driftSpaceEditableProvider called SharedSpaceApiRepository.isSpaceEditor,
which fetched getMembers per space and failed open. It now reads the role
from sharedSpacesProvider (getAll embeds members) via spaceIsWritable, so it
re-resolves whenever that list refreshes. Callers treat loading and error as
read-only instead of editable. isSpaceEditor is deleted.

Mirrors the web change; the server still enforces the role on every write.
Mobile: driftSpaceEditableProvider never errors; a failed spaces list
reads as not editable. Both call sites use valueOrNull, because on
Riverpod 2 AsyncValue.value rethrows an error state. Adds the error-case
test. The creator short-circuit in spaceIsWritable is redundant for this
check (the server always makes the creator an Owner member) but
equivalent, so both clients apply "own role is Owner or Editor".

Web: the sidebar's recent spaces go through loadSpaces, so a cache reset
fires one getAllSpaces call instead of two. The failure toast moves into
loadSpaces, and the sidebar keeps its last list on screen while it
refetches.

Docs: the Mobile section of AGENTS.md (CLAUDE.md) now describes the
spaces-list-derived, fail-closed behaviour.
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

Label error. Requires exactly 1 of: changelog:.*. Found: 🖥️web, 📱mobile. A maintainer will add the required label.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant