Skip to content

fix(space): allow space owners to manage member roles in Space Admin console#90

Merged
an9xyz merged 2 commits into
mainfrom
fix/space-owner-member-role
Jun 12, 2026
Merged

fix(space): allow space owners to manage member roles in Space Admin console#90
an9xyz merged 2 commits into
mainfrom
fix/space-owner-member-role

Conversation

@an9xyz

@an9xyz an9xyz commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Lets a Space owner manage member roles from the Space self-service admin console (/admin/space/:spaceId/members). Previously owners could see the member list and remove members, but the role action was hidden โ€” the panel only exposed role controls for the super-admin scope and called the manager API directly.

The backend user-side endpoint (PUT /v1/space/:space_id/members/:uid/role, owner permission enforced server-side) already exists; this PR is the octo-admin frontend wiring.

Changes

  • src/api/space-user.ts โ€” add updateSpaceUserMemberRole calling PUT /v1/space/:space_id/members/:uid/role.
  • src/hooks/useSpaceScope.ts โ€” add updateMemberRole to SpaceScope.api; super-admin scope routes to the manager endpoint, space scope routes to the user endpoint.
  • src/pages/Spaces/SpaceMembersPanel.tsx โ€” call scope.api.updateMemberRole instead of importing the manager API directly; canChangeRole is enabled for super-admin and for space scope only when the viewer is owner (role === 2).

Behavior

  • Super-admin console role management is unchanged (still uses the manager endpoint).
  • Space owner sees the role action; admin/member do not.
  • Owner can promote memberโ†’admin, demote adminโ†’member, and set another member as owner (ownership transfer).
  • The current owner's own row exposes no role/remove action, so no local self-demotion; backend rejections (illegal transfer, self-demotion) are surfaced via the backend error message โ€” no local optimistic success.

Test plan

  • npm run build (tsc + vite build) passes
  • Super-admin console: role management still works (no regression)
  • Space owner at /admin/space/:spaceId/members: role action visible; promote/demote/transfer refresh the list with correct roles
  • Space admin/member: no role action
  • Backend rejection (owner self-demote / illegal transfer): error shown, no local fake success

Related

โ€ฆconsole

Space owners could see the member list and remove members in the Space
self-service console, but role management was hidden because the panel
only exposed role actions for the super-admin scope and called the
manager API directly.

- add user-side role update API in space-user.ts:
  PUT /v1/space/:space_id/members/:uid/role
- expose updateMemberRole on SpaceScope.api, routing super-admin to the
  manager endpoint and space scope to the user endpoint
- SpaceMembersPanel now calls scope.api.updateMemberRole instead of
  importing the manager API directly
- canChangeRole stays enabled for super-admin and is enabled for space
  scope only when role is owner (role=2); admin/member see no role action
- owner rows expose no role/remove action, so the current owner cannot be
  locally demoted; ownership transfer and backend rejections are surfaced
  via the backend error message

Closes #89
@an9xyz an9xyz requested a review from a team as a code owner June 11, 2026 02:31
@github-actions github-actions Bot added the size/S PR size: S label Jun 11, 2026
lml2468
lml2468 previously approved these changes Jun 11, 2026

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE โ€” clean, well-scoped authorization wiring. Verified against head 42f8302fe7a3. tsc --noEmit passes; the gating logic is correct and defense-in-depth holds.

This is a permissions change, so I focused on the auth path:

Authorization โ€” correct

  • canChangeRole = !readOnly && (scope.kind === 'super' || scope.role === 2) (SpaceMembersPanel.tsx:48). scope.role is the viewer's own role in the space, sourced from SpaceUserDetail.role (the user-side GET /v1/space/:id) โ†’ useSpaceScope(detail.role) in SpaceAdmin/tabs.tsx:16. So only an owner (role 2) or super-admin sees role controls. === 2 is type-valid against SpaceScopeRole = 'super' | 0 | 1 | 2.
  • No self-demotion / owner-removal via UI: owner rows (record.role === 2) expose no role action (L153 gates all role items behind record.role !== 2) and no remove button (L187 canRemove && record.role !== 2). An owner can't demote themselves or strip the owner from the list.
  • Defense in depth: the user endpoint PUT /v1/space/:space_id/members/:uid/role is owner-enforced server-side (per PR body + octo-server#339), so the client gate is UX, not the security boundary. Illegal transfers / self-demotion are rejected server-side and surfaced via message.error((error as Error).message) โ€” no optimistic fake success (handleChangeRole only shows success inside onOk after the await resolves).
  • Super console unchanged: buildSuperScope.updateMemberRole still routes to manager.updateSpaceMemberRole (useSpaceScope.ts:127); SpaceDetailDrawer uses useSpaceScope() with kind==='super', so its behavior is identical to before.

Correctness / wiring โ€” verified

  • New updateSpaceUserMemberRole (space-user.ts:100) hits the correct user endpoint; buildUserScope.updateMemberRole routes to it (useSpaceScope.ts:198).
  • The panel dropped its direct import { updateSpaceMemberRole } from '../../api/space' and now goes through scope.api.updateMemberRole โ€” proper scope abstraction. api/space.ts still exports updateSpaceMemberRole/SpaceMemberRole (used by the manager/super path), so no dangling refs. The panel's local type SpaceMemberRole = 0 | 1 | 2 matches.
  • Ownership transfer (roleโ†’2) shows the dedicated members.changeRole.ownerContent warning; demote/promote use the default confirm. All 4 new-ish i18n keys (members.action.toOwner/promoteAdmin/demoteMember, members.changeRole.ownerContent) exist in both locales.
  • record.status !== 1 rows render โ€” (no actions on inactive/pending members) โ€” sensible, unchanged.

Verification

  • tsc --noEmit clean.
  • No unit tests exist for SpaceMembersPanel/useSpaceScope (pre-existing gap, not introduced here). The manual test plan in the PR body covers the right paths (super no-regression, owner promote/demote/transfer, admin/member no action, backend rejection).

Nits (non-blocking)

  • Consider a regression test for the gating matrix (owner sees action / admin+member don't / owner's own row has no action) โ€” this is exactly the kind of auth logic worth locking down, especially after the role check moved from a simple kind === 'super' to include scope.role === 2.
  • role: 0 | 1 | 2 is repeated as an inline literal in several signatures (space-user.ts, useSpaceScope.ts, the panel). Minor: the exported SpaceMemberRole type could be reused for consistency. Not worth churn.

Solid, minimal, and the security-relevant gating is right. LGTM.

Jerry-Xin
Jerry-Xin previously approved these changes Jun 11, 2026

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope check passed: this PR is relevant to octo-admin and wires existing space member role-management UI to the user-side space API without introducing a frontend permission bypass.

๐Ÿ’ฌ Non-blocking

๐ŸŸก Warning โ€” After an owner transfers ownership, the member table refreshes, but the current viewerโ€™s scope does not. SpaceMembersPanel derives canChangeRole from scope.role at src/pages/Spaces/SpaceMembersPanel.tsx:48, and that scope comes from detail.role in src/pages/SpaceAdmin/tabs.tsx:15-17. detail is only loaded on layout mount / space change in src/pages/SpaceAdmin/SpaceAdminLayout.tsx:127-147, while successful role changes only call fetchData() at src/pages/Spaces/SpaceMembersPanel.tsx:88-90. So after transferring ownership away, the old owner can still see role actions until reload, though the backend should reject them. Consider exposing an onRoleChanged callback that refreshes /space/my and getSpaceUserDetail, or locally updating the viewer role after owner transfer.

๐Ÿ”ต Suggestion โ€” src/pages/Spaces/SpaceMembersPanel.tsx:19 now duplicates SpaceMemberRole instead of reusing the existing type from src/api/space.ts or a shared scope type. This is small, but centralizing the role type would reduce drift.

โœ… Highlights

The new API function in src/api/space-user.ts:100-104 matches the described user-side endpoint.

The scope abstraction change in src/hooks/useSpaceScope.ts:127-199 keeps super-admin calls on the manager endpoint while routing space-console calls to the user endpoint, which fits the existing architecture.

Verification note: I attempted npm run build, but this checkout does not have dependencies installed, so it fails immediately with sh: tsc: command not found.

mochashanyao
mochashanyao previously approved these changes Jun 11, 2026

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Octo-Q ยท automated review]

Verdict: Approve โ€” no blocking findings; notes below (data-flow traced).


octo-admin PR#90 Review Report

Reviewer: Octo-Q (automated review)
PR: fix(space): allow space owners to manage member roles in Space Admin console
Head SHA: 42f8302fe7a3b8ef2478945e264d05e18cde040e
Files changed: 3 (+15 / โˆ’3)


1. Verification Summary

Check Result Evidence
Build (tsc + vite) โœ… Pass npm run build succeeds on head SHA
Super-admin path unchanged โœ… buildSuperScope().api.updateMemberRole โ†’ manager.updateSpaceMemberRole (same manager endpoint as before)
Space-owner path wired โœ… buildUserScope().api.updateMemberRole โ†’ user.updateSpaceUserMemberRole โ†’ PUT /v1/space/:space_id/members/:uid/role
canChangeRole gate โœ… `!readOnly && (scope.kind === 'super'
Role data flow โœ… SpaceAdminLayout fetches getSpaceUserDetail โ†’ detail.role โ†’ useSpaceScope(detail.role) โ†’ scope.role
Error handling โœ… Backend rejections surface via ApiError.message in message.error()
Old import cleanup โœ… updateSpaceMemberRole and SpaceMemberRole imports removed from space.ts; local type alias SpaceMemberRole = 0 | 1 | 2 is structurally identical

2. Findings

No P0/P1 findings.

P2 โ€” Owner self-row exposes role-change actions (doomed API call)

Diff-scope: new (amplified by this PR โ€” previously canChangeRole was false for all space-scope users, so the self-row issue didn't exist)

Description: When the viewer is an owner, canChangeRole = true and the action column renders role-change options for every row where record.role !== 2, including the viewer's own row. An owner can click "Promote to admin" or "Transfer ownership" on themselves. The backend will reject illegal operations (self-demotion, invalid transfer), and the error is surfaced via message.error. No data corruption occurs.

Impact: Minor UX issue โ€” the owner sees an action that will fail when clicked. The PR description explicitly delegates this to the backend: "backend rejections (illegal transfer, self-demotion) are surfaced via the backend error message โ€” no local optimistic success."

Suggestion (non-blocking): Consider filtering the viewer's own row from role-change actions. The auth store exposes uid (useAuthStore(s => s.uid)); the action column could skip role items when record.uid === viewerUid. This avoids a doomed API round-trip. Low priority since the backend is the authority and handles it correctly.

Severity: P2 (UX improvement, no data loss or broken path)

P2 โ€” No frontend ownership-transfer confirmation detail

Diff-scope: pre-existing (the confirmation dialog for role=2 transfer existed before this PR for super-admin; this PR extends its reach to space owners)

Description: When setting a member to owner (role=2), Modal.confirm shows members.changeRole.ownerContent as the warning text. This is adequate. However, the dialog doesn't differentiate between "promote adminโ†’owner" and "transfer ownership away from current owner" โ€” both show the same confirmation. This is a pre-existing design choice and not introduced by this PR.

Severity: P2 (pre-existing UX note, not blocking)

3. Suggestions

  1. Self-row guard (optional): Pass the viewer's uid into SpaceMembersPanel (or read from auth store) and skip role-change menu items when record.uid === viewerUid. Prevents a confusing error-toast on self-action.
  2. Type reuse (nit): The local type SpaceMemberRole = 0 | 1 | 2 could import from space-user.ts (SpaceUserMember['role']) for single-source-of-truth, though structurally identical.

4. Extra Observations

  • The updateSpaceUserMemberRole function doesn't use unwrap() โ€” consistent with other write-only endpoints (removeSpaceUserMembers, manager-side updateSpaceMemberRole) where the caller doesn't read the response body.
  • The SpaceScope.api.updateMemberRole is non-optional in the interface (no ?), and both buildSuperScope and buildUserScope implement it. No risk of calling undefined.
  • readOnly prop is correctly checked first in canChangeRole. The super-admin drawer passes readOnly={true} when space.status !== 1, which correctly disables role changes for inactive spaces.

5. Data Flow Trace

SpaceAdminLayout.tsx:135  getSpaceUserDetail(spaceId)
  โ†’ detail.role (0|1|2) from GET /v1/space/:spaceId response
  โ†’ setDetail(d) stores in component state
  โ†’ <Outlet context={{ detail }} />

tabs.tsx:15-16  const { detail } = useOutletContext<Ctx>()
  โ†’ useSpaceScope(detail.role)

useSpaceScope.ts:238-242
  โ†’ scope = useAuthStore(s => s.scope)  // 'super' | 'space'
  โ†’ if scope === 'super': buildSuperScope() โ†’ role='super', kind='super'
  โ†’ else: buildUserScope(role ?? 0) โ†’ kind='space', role=detail.role

SpaceMembersPanel.tsx:48
  โ†’ canChangeRole = !readOnly && (scope.kind === 'super' || scope.role === 2)
  โ†’ For space-scope owner (role=2): canChangeRole = true โœ…
  โ†’ For space-scope admin (role=1): canChangeRole = false โœ…
  โ†’ For space-scope member (role=0): canChangeRole = false โœ…

SpaceMembersPanel.tsx:88
  โ†’ scope.api.updateMemberRole(spaceId, uid, role)
  โ†’ For super: manager.updateSpaceMemberRole โ†’ PUT /v1/manager/spaces/:id/members/:uid/role
  โ†’ For space: user.updateSpaceUserMemberRole โ†’ PUT /v1/space/:id/members/:uid/role

All data flows verified โ€” detail.role is populated by the backend API response before the component renders (guarded by loading || !detail check in SpaceAdminLayout.tsx:311). No empty/undefined/short-circuit risks.

6. Blindspot Checklist (R5)

C1 โ€” Dual-path parity: โœ… Clear
Both buildSuperScope and buildUserScope implement updateMemberRole. The SpaceScope interface declares it as non-optional. Both paths are wired and tested via build.

C2 โ€” Control-flow ordering / nested reuse: โœ… Clear
updateMemberRole is called from a single site (SpaceMembersPanel.tsx:88 inside handleChangeRole's Modal.confirm.onOk). No nested/compound call risk. No security controls (regex/escape/sanitize) involved.

C3 โ€” Authorization boundary โ‰  capability boundary: โœ… Clear
Frontend gates canChangeRole to super-admin or space-owner only. Backend enforces owner permission on PUT /v1/space/:space_id/members/:uid/role (per PR description and related octo-server#339). The frontend doesn't bypass backend auth โ€” it only controls UI visibility. Even if canChangeRole were bypassed, the backend would reject unauthorized requests.


[Octo-Q] verdict: APPROVE โ€” No P0/P1 findings. Clean, minimal diff that correctly wires the user-side API for space-owner role management. Two P2 UX suggestions (self-row guard, type reuse) are non-blocking.

yujiawei
yujiawei previously approved these changes Jun 11, 2026

@yujiawei yujiawei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review โ€” PR #90 (octo-admin)

Verdict: APPROVED โ€” no blocking issues. A few non-blocking nits below.

Summary

Clean, well-scoped frontend wiring (15 lines across 3 files) that lets a Space owner manage member roles from the self-service Space Admin console. The change matches the issue #89 acceptance criteria exactly: it adds the user-side role API, exposes updateMemberRole on the scope abstraction, and routes super-admin โ†’ manager endpoint vs. space scope โ†’ user endpoint.

1. Verification

  • โœ… Correct API routing. useSpaceScope.ts:127-128 routes super scope to manager.updateSpaceMemberRole (PUT /v1/manager/spaces/:id/members/:uid/role), and useSpaceScope.ts:198-199 routes space scope to user.updateSpaceUserMemberRole (PUT /v1/space/:id/members/:uid/role, space-user.ts:100-104). Super-admin behavior is unchanged.
  • โœ… Decoupling done right. SpaceMembersPanel.tsx:88 now calls scope.api.updateMemberRole instead of importing the manager API directly; the manager import was removed (SpaceMembersPanel.tsx:17) and no dangling references remain anywhere in src/.
  • โœ… Gating is correct. canChangeRole = !readOnly && (scope.kind === 'super' || scope.role === 2) (SpaceMembersPanel.tsx:48) โ€” only super-admin and space owners get the role action; admin/member do not.
  • โœ… No local self-demotion. Owner rows are excluded from both the role menu (record.role !== 2 guard at SpaceMembersPanel.tsx:153) and the remove button (SpaceMembersPanel.tsx:187), so the current owner cannot locally act on their own row. Permission enforcement is correctly left to the backend; no optimistic success โ€” fetchData() re-reads after the call resolves.
  • โœ… Type-safe & tested. tsc --noEmit passes, vitest run is green (25/25), and all referenced i18n keys (members.changeRole.*, members.action.*) exist in both en-US and zh-CN locales.

2. Issues

None at P0/P1.

3. Non-blocking suggestions (P2 / nit)

  • Stale scope.role after ownership transfer (UX nit). scope.role derives from detail.role, which SpaceAdminLayout loads once at mount (SpaceAdminLayout.tsx:135) and does not refresh after a role change. If an owner transfers ownership to another member, they are demoted to admin server-side, but canChangeRole stays true locally until the page reloads, so the role action remains visible. Any action taken in that stale window will be correctly rejected by the backend (so this is not a security issue โ€” backend is authoritative), but the user sees the control before it disappears. Consider re-fetching the space detail after a successful role change, or surfacing a "your permissions changed, refreshingโ€ฆ" hint. Out of scope for this fix; worth a follow-up.

  • Local type duplication (nit). type SpaceMemberRole = 0 | 1 | 2 is re-declared in SpaceMembersPanel.tsx:19, duplicating the identical type already exported from api/space.ts and the MemberItem.role shape in useSpaceScope.ts. The local copy is what lets the panel drop the manager import (a reasonable trade), but a shared type SpaceMemberRole re-exported from useSpaceScope.ts would avoid the third copy drifting.

  • Param shadowing (nit). In buildUserScope, the updateMemberRole: (spaceId, uid, role) => ... arrow shadows the outer role parameter of buildUserScope(role). Harmless here, but a rename (e.g. nextRole) would read more clearly.

4. Additional observations

  • The PR description's manual test plan items (super-admin no-regression, owner promote/demote/transfer, admin/member hidden, backend-rejection error surfacing) are unchecked. The static review confirms the code paths are correct; a quick manual pass on the owner transfer + backend-rejection flows before merge would close the loop, but none of this blocks approval.

Non-blocking follow-ups from PR #90 review:

- refresh viewer scope after a role change: SpaceAdminLayout exposes a
  refreshDetail callback via Outlet context, wired to SpaceMembersPanel
  as onRoleChanged. After an owner transfers ownership and is demoted,
  the role action now disappears without a manual page reload.
- guard the viewer's own row: role actions are hidden when
  record.uid === viewerUid, avoiding a doomed self-demotion/transfer
  request that the backend would reject anyway.
- single-source the role type: export SpaceMemberRole from useSpaceScope
  and reuse it across the scope API, MemberItem, and the panel instead of
  redeclaring 0 | 1 | 2 in several places.
- rename the shadowed role param in buildUserScope.updateMemberRole to
  nextRole for clarity.
@an9xyz an9xyz dismissed stale reviews from yujiawei, mochashanyao, Jerry-Xin, and lml2468 via c0838c1 June 11, 2026 14:55
@github-actions github-actions Bot added size/M PR size: M and removed size/S PR size: S labels Jun 11, 2026

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE โ€” re-review at c0838c1819d5. All three follow-up ๐ŸŸก from the last round are addressed correctly. tsc --noEmit clean, npm run build passes. The authorization logic from the prior approval is intact.

๐ŸŸก โ†’ fixed: stale scope.role after ownership transfer (Jerry-Xin)

  • SpaceAdminLayout adds a refreshDetail callback (useCallback, L159) that re-fetches getMySpaces() + getSpaceUserDetail(spaceId), re-runs the managed-space guard (kicks to /space if the viewer lost management rights), and setDetail(d). Passed via Outlet context={{ detail, refreshDetail }}.
  • Threaded through tabs.tsx (Ctx.refreshDetail) โ†’ MembersTab โ†’ SpaceMembersPanel as onRoleChanged, invoked in handleChangeRole's onOk after the await + fetchData() (SpaceMembersPanel.tsx:98). So after an owner transfers ownership and gets demoted, detail.role refreshes โ†’ scope.role updates โ†’ the role controls correctly disappear. Verified the wiring end-to-end.

๐ŸŸก โ†’ fixed: self-row role actions (mochashanyao)

  • if (canChangeRole && record.uid !== viewerUid) (SpaceMembersPanel.tsx:161) โ€” viewerUid from useAuthStore(s => s.uid). The viewer no longer sees role actions on their own row, eliminating the doomed self-targeted call. Note: in super scope uid is '' (super login doesn't set it), so the guard is a harmless no-op there and super-admin still manages every row โ€” correct.

๐ŸŸก โ†’ fixed: type reuse (Jerry-Xin + me)

  • SpaceMemberRole = 0 | 1 | 2 now exported from useSpaceScope.ts and reused across MemberItem.role, SpaceScope.api.updateMemberRole, buildUserScope, useSpaceScope, and imported by the panel (local redeclaration removed). space-user.ts uses SpaceUserMember['role']. Consistent; no dangling local type.

Verification

  • tsc --noEmit clean; npm run build passes (the >500kB chunk warning is pre-existing, unrelated).
  • Super console unchanged: SpaceDetailDrawer renders SpaceMembersPanel without onRoleChanged โ€” fine, it's optional and a super-admin's role isn't space-scoped.
  • Authorization unchanged and still correct: owner/super-only gating, owner rows expose no actions, backend owner-enforcement remains the real boundary.

Remaining (non-blocking, optional)

  • Still no unit test for the gating matrix (owner sees / admin+member don't / own row hidden / post-transfer refresh). Now that the logic has grown (role check + self-row guard + refresh callback), a small regression test would be well worth it โ€” but not a merge blocker.

Clean, complete follow-up. LGTM.

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is in scope for octo-admin and correctly wires space-owner role management through the existing scoped API abstraction.

๐Ÿ’ฌ Non-blocking

๐ŸŸก Warning: There is no automated coverage for the new scoped role-change behavior. The risky paths are the permission gating in src/pages/Spaces/SpaceMembersPanel.tsx:53, self-row suppression at src/pages/Spaces/SpaceMembersPanel.tsx:162, and endpoint routing in src/hooks/useSpaceScope.ts:128 and src/hooks/useSpaceScope.ts:199. A small component/unit test around super-admin vs owner vs admin/member visibility would reduce regression risk.

๐Ÿ”ต Suggestion: In src/pages/Spaces/SpaceMembersPanel.tsx:95 and src/pages/Spaces/SpaceMembersPanel.tsx:98, fetchData() and onRoleChanged?.() are both fire-and-forget after a successful role update. This is acceptable, but awaiting the refreshes would make post-transfer state ordering more deterministic, especially when ownership transfer changes the current viewerโ€™s role.

โœ… Highlights

๐Ÿ”ต The PR preserves the super-admin API path while adding the user-side endpoint through SpaceScope.api.updateMemberRole, which fits the existing scope abstraction.

๐Ÿ”ต The UI now hides role actions from non-owner space admins/members and avoids showing role changes on the current viewerโ€™s own row, matching the described behavior.

Validation note: I attempted npm run build and npm test, but this checkout does not have dependencies installed; both failed because tsc and vitest were not found.

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Octo-Q ยท automated review]

Verdict: Approve โ€” no blocking findings; notes below (data-flow traced).


octo-admin PR#90 Review Report

Reviewer: Octo-Q (automated review)
Head SHA: c0838c1819d5f45665cd38686257e71e0e824e17
PR: fix(space): allow space owners to manage member roles in Space Admin console
Files: 5 changed (+67 / โˆ’16)


1. Verification Summary

Item Status Evidence
API wiring (updateSpaceUserMemberRole) โœ… src/api/space-user.ts:100-104 โ€” correct PUT endpoint
Scoped routing (updateMemberRole) โœ… useSpaceScope.ts:129 (superโ†’manager), :200 (spaceโ†’user)
canChangeRole gate โœ… SpaceMembersPanel.tsx:53 โ€” scope.kind === 'super' || scope.role === 2
Self-row protection โœ… SpaceMembersPanel.tsx:160 โ€” record.uid !== viewerUid
refreshDetail chain โœ… SpaceAdminLayout.tsx:161-180 โ†’ outlet context โ†’ tabs.tsx:27 โ†’ onRoleChanged
Type consistency (SpaceMemberRole) โœ… useSpaceScope.ts:7 defines 0|1|2, structurally identical to api/space.ts:6
Backward compat (super-admin path) โœ… SpaceDetailDrawer.tsx:78 โ€” no onRoleChanged passed, optional prop, no-op
Backend authorization โœ… Server-side owner check on PUT /v1/space/:space_id/members/:uid/role (per PR body, octo-server#339)

2. Findings

F1 โ€” fetchData() and onRoleChanged?.() fire-and-forget after role change [P2 / nit]

File: src/pages/Spaces/SpaceMembersPanel.tsx:95-98

await scope.api.updateMemberRole(spaceId, uid, role)
message.success(t('members.changeRole.success'))
fetchData()          // not awaited
onRoleChanged?.()    // not awaited

Diff-scope: new (introduced by this PR โ€” onRoleChanged is new; fetchData() was pre-existing but now runs alongside onRoleChanged).

Both calls are fire-and-forget. If refreshDetail (via onRoleChanged) completes before fetchData, the scope updates first and the member list refreshes against the new scope โ€” harmless. If fetchData completes first, the list refreshes under the old scope, then the scope updates โ€” also harmless since the next render picks up the new scope. Both operations are idempotent.

Impact: Theoretical brief UI inconsistency (member list shows old roles for a fraction of a second while scope already updated). Not user-visible in practice.

Suggestion: Consider await Promise.all([fetchData(), onRoleChanged?.()]) for deterministic ordering. Non-blocking.

F2 โ€” refreshDetail lacks loading indicator [P2 / nit]

File: src/pages/SpaceAdmin/SpaceAdminLayout.tsx:161-180

The initial load effect (line 131) sets setLoading(true) before fetching, but refreshDetail does not toggle any loading state. During the refresh, the UI shows stale detail data until the fetch completes.

Diff-scope: new (this PR introduces refreshDetail).

Impact: After ownership transfer, the old role controls remain visible for a brief moment until refreshDetail completes. The user sees the correct state after the async refresh. Not confusing in practice since the success toast already confirms the action.

Suggestion: Consider setLoading(true) at the start of refreshDetail for visual consistency. Non-blocking.

3. Suggestions

  • F1 fix: await Promise.all([fetchData(), onRoleChanged?.()]) in handleChangeRole onOk handler.
  • F2 fix: Add setLoading(true) / setLoading(false) around the refreshDetail fetch, or use a separate refreshing state to avoid hiding the entire content.

4. Additional Observations

  • Good defense-in-depth: The record.uid !== viewerUid check (line 160) prevents self-targeting at the UI layer, complementing the backend rejection. This avoids unnecessary error toasts for actions that are guaranteed to fail.
  • Clean scope abstraction: Routing updateMemberRole through SpaceScope.api keeps the panel scope-agnostic. The super-admin path (SpaceDetailDrawer) continues to work without changes since onRoleChanged is optional.
  • SpaceMemberRole type alias in useSpaceScope.ts is a clean refactor โ€” eliminates the inline 0 | 1 | 2 repetition across interfaces.

5. Data Flow Tracing

Consumed Data Upstream Source Flows Correctly?
scope.role (in canChangeRole) detail.role โ† getSpaceUserDetail(spaceId) API โ†’ setDetail(d) โ†’ <Outlet context={{detail}}> โ†’ useSpaceScope(detail.role) โ†’ buildUserScope(role) โ†’ scope.role โœ… Yes. After refreshDetail, setDetail(d) triggers re-render, useSpaceScope useMemo recomputes with new role.
scope.api.updateMemberRole Super: manager.updateSpaceMemberRole โ†’ PUT /v1/manager/spaces/:id/members/:uid/role. Space: user.updateSpaceUserMemberRole โ†’ PUT /v1/space/:id/members/:uid/role โœ… Yes. Both functions exist and are correctly wired in buildSuperScope / buildUserScope.
viewerUid (self-row gate) useAuthStore(s => s.uid) โ€” set by loginSpace(). For super-admin, uid='' but super-admin bypasses the check via scope.kind === 'super' in canChangeRole. โœ… Yes. No false positives for either scope.
onRoleChanged callback SpaceAdminLayout.refreshDetail โ†’ outlet context โ†’ tabs.tsx destructures โ†’ passes as onRoleChanged prop to SpaceMembersPanel. SpaceDetailDrawer does not pass it (optional, no-op). โœ… Yes. Chain is complete and correctly wired.
refreshDetail โ†’ setMySpaces(managed) getMySpaces() API โ†’ filter role >= 1 โ†’ setMySpaces. If current space no longer in managed list โ†’ navigate('/space'). โœ… Yes. Correctly handles ownership transfer (new owner's role=2 โ‰ฅ 1, stays; old owner's role=0 < 1, redirected).

6. R5 Blind-Spot Checklist

C1 โ€” Dual-path parity: Clear.

  • updateMemberRole is symmetrically wired: superโ†’manager endpoint, spaceโ†’user endpoint. Both endpoints exist and are correctly typed.
  • No addโ†”remove asymmetry introduced. addMembers / removeMembers paths are unchanged.
  • No create-gate โ†” execute-path asymmetry. The canChangeRole gate is the only entry point for role changes, and it correctly checks both super and owner.

C2 โ€” Control-flow ordering / nesting: Clear.

  • refreshDetail and fetchData are independent async operations with no ordering dependency. Both are idempotent.
  • No nested/compound security controls that could be bypassed by ordering.
  • canChangeRole is a single boolean gate, no compound conditions that could short-circuit.

C3 โ€” Authorization boundary โ‰  capability boundary: Clear.

  • UI gate (canChangeRole) and backend authorization (PUT /v1/space/:space_id/members/:uid/role owner check) are aligned.
  • The record.uid !== viewerUid check prevents self-targeting at UI layer; backend also rejects self-demotion.
  • No new endpoints or capabilities are exposed โ€” this PR only wires existing backend endpoints to the frontend.

[Octo-Q] verdict: APPROVE

No P0/P1 findings. Two P2 nits (fire-and-forget ordering, missing loading indicator) are non-blocking quality improvements. The core logic โ€” scoped API routing, permission gating, self-row protection, and post-change scope refresh โ€” is correctly implemented with clean data flow.

@yujiawei yujiawei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review โ€” PR #90 (octo-admin)

Scope: Allow space owners (role 2) to manage member roles in the Space Admin console, routing role-change calls through the correct scoped API and refreshing the viewer's own scope after a change.

Verdict: APPROVED โ€” no blocking issues. Verified against head c0838c1. Local tsc --noEmit, vitest (25 tests), and vite build all pass.

Verification

Item Status Evidence
Role-change routed through scoped API (no longer hardcoded to manager endpoint) โœ… SpaceMembersPanel.tsx:93 now calls scope.api.updateMemberRole; super โ†’ manager.updateSpaceMemberRole (useSpaceScope.ts:128), user โ†’ user.updateSpaceUserMemberRole (useSpaceScope.ts:199)
New user-side endpoint added โœ… space-user.ts:100-104 PUT /v1/space/${spaceId}/members/${uid}/role mirrors the existing manager endpoint at space.ts:131-135
Owner gating โœ… SpaceMembersPanel.tsx:53 `canChangeRole = !readOnly && (scope.kind === 'super'
Self-row protected โœ… SpaceMembersPanel.tsx:162 record.uid !== viewerUid hides role actions on the viewer's own row
Post-transfer demotion handled โœ… After a change, onRoleChanged() โ†’ refreshDetail (SpaceAdminLayout.tsx:161-178) re-fetches /space/my + detail, rebuilds scope, and navigates out if the viewer drops below manager โ€” so the owner who transfers ownership immediately loses the role-edit entry
Super-admin drawer path unaffected โœ… SpaceDetailDrawer.tsx:78-83 does not pass onRoleChanged (no-op), and super scope does not depend on the target space's role โ€” no regression
Type extraction sound โœ… SpaceMemberRole extracted in useSpaceScope.ts:7; consumers updated; no any introduced
i18n keys present โœ… members.action.toOwner / promoteAdmin / demoteMember / changeRole.* exist in both en-US and zh-CN spaces.json

The ownership-transfer flow is the strongest part of this change: handling the viewer's own demotion via refreshDetail (rather than leaving a stale scope.role that keeps the edit menu visible) is exactly the right call and is easy to get wrong.

Non-blocking observations (P2 / nits)

  1. Authorization is frontend-only (by design, but worth stating explicitly). All gating here is UX. The actual security boundary is the backend PUT .../members/{uid}/role handler, which must enforce that only owners (and super) can change roles and reject self-demotion. The code comment at SpaceMembersPanel.tsx:160-161 acknowledges this ("ๆณจๅฎš่ขซๅŽ็ซฏๆ‹’็ป"). This PR is correct to not rely solely on client gating โ€” just confirm the server side enforces the same rules.

  2. Duplicate SpaceMemberRole type definition. useSpaceScope.ts:7 and api/space.ts:5 each define SpaceMemberRole = 0 | 1 | 2 independently. They are structurally identical so nothing breaks, but now that the panel imports the type from useSpaceScope, consider consolidating to a single source of truth to avoid future drift.

  3. Redundant fetches after a role change. handleChangeRole triggers fetchData() plus onRoleChanged() โ†’ refreshDetail (which itself does getMySpaces + getSpaceUserDetail) โ€” three requests per change. Acceptable for an admin console, but the member-list refetch and the detail refetch could potentially be coordinated.

  4. Role-management asymmetry for admins. Admins (role 1) can remove members (canRemoveMembers: isManager, useSpaceScope.ts:165) but cannot change roles. This appears intentional given the PR title ("space owners"); flagging only to confirm it is the desired product behavior.

  5. No test coverage for the new logic. The repo has a working vitest setup. A small unit test around useSpaceScope role gating, the canChangeRole condition, or the self-row hiding would lock in this behavior. Suggestion, not a blocker.

None of the above blocks merge.

@an9xyz an9xyz merged commit 0c1f5e9 into main Jun 12, 2026
21 checks passed
@an9xyz an9xyz deleted the fix/space-owner-member-role branch June 12, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(space): allow space owners to manage member roles in Space Admin console

5 participants