Skip to content

Refactor user read page into categorized split-table layout#376

Open
somethingnew2-0 wants to merge 3 commits intomainfrom
refactor/users-read-categorized-split-tables
Open

Refactor user read page into categorized split-table layout#376
somethingnew2-0 wants to merge 3 commits intomainfrom
refactor/users-read-categorized-split-tables

Conversation

@somethingnew2-0
Copy link
Copy Markdown
Collaborator

@somethingnew2-0 somethingnew2-0 commented Feb 28, 2026

Screenshot 2026-02-27 at 4 49 07 PM

Summary

  • Reorganizes the user read page from two flat tables (Owner of Groups/Roles, Member of Groups/Roles) into three sections by group type: Access Roles, App Groups, and Standard Groups
  • Each section shows side-by-side Ownerships and Memberships tables, replacing the previous separate full-width tables
  • App Groups are further sub-grouped by app, with clickable app name headers linking to /apps/{appName}
  • Sections are hidden when both ownerships and memberships are empty for that type
  • Replaces the duplicated OwnerTable/MemberTable components with a single unified GroupTable component parameterized by an owner boolean
  • Drops the Type column from tables since items are now categorized by section

Reorganize the user read page from two flat tables (Owner/Member) into
three sections by group type (Access Roles, App Groups, Standard Groups),
each with side-by-side Ownerships and Memberships tables. App Groups are
further sub-grouped by app with clickable app name headers. Sections are
hidden when both sides are empty for that type.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@somethingnew2-0 somethingnew2-0 requested a review from a team March 10, 2026 00:33
Copy link
Copy Markdown
Contributor

@barborico barborico left a comment

Choose a reason for hiding this comment

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

Aesthetics look good to me. I hade Claude take a pass, and it found a but that seems legit.

Comment thread src/pages/users/Read.tsx Outdated
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.

The useCallback dep array is missing owner and user.id, both of which are captured from
props and drive the mutation body.

The old OwnerTable/MemberTable each hard-coded their API body, so the only closed-over
variable was user.id (already a pre-existing gap). The refactor adds owner as a new
closed-over prop value, making the stale-closure risk concrete: if React Query's
putGroupUsers object happens to be stable across a re-render where owner has changed,
the callback fires the wrong mutation — silently calling owners_to_remove when
members_to_remove was intended, or vice versa.

Suggested change
+ [putGroupUsers, owner, user.id],

Copy link
Copy Markdown
Collaborator Author

@somethingnew2-0 somethingnew2-0 Apr 17, 2026

Choose a reason for hiding this comment

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

Good catch, fixed in 0d9f38a. Agreed the deps were incomplete, worth noting the stale-closure isn't reachable, today since each , is rendered with a literal owner and a stable user.id from the route, but the fix is still correct and guards future refactors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants