fix: role downgrade error handling and toast UX#2028
fix: role downgrade error handling and toast UX#2028nick-inkeep wants to merge 9 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 50f3b09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟡 Minor (1) 🟡
🟡 1) packages/agents-core/src/auth/auth.ts:334-371 Missing test coverage for role change hook
Issue: The beforeUpdateMemberRole hook contains critical business logic (SpiceDB role sync, promotion detection, project membership revocation) but has no unit test coverage. The PR adds error handling that surfaces user-friendly messages, but there's no automated verification that this behavior works as expected.
Why: Without tests: (1) the promotion detection logic (isPromotion condition) could regress silently, (2) the error message could be accidentally changed or removed, (3) the try-catch could be removed in a refactor. The test plan mentions "Full test suite passes" but there are no existing tests that exercise this specific hook.
Fix: Consider adding tests to packages/agents-core/src/auth/authz/__tests__/sync.test.ts or a new test file:
- Test
changeOrgRoleatomically swaps roles - Test
revokeAllProjectMembershipscleans up project roles on promotion - Test that SpiceDB failures result in
APIErrorwith the expected message
Alternatively, this can be covered by manual QA as noted in the test plan, with a follow-up issue to add automated tests.
Refs:
Inline Comments:
- 💭 Consider:
auth.ts:370Error message uses internal "sync authorization" terminology
🧹 While You're Here (2) 🧹
🧹 1) packages/agents-core/src/auth/init.ts:132 Init script assigns ADMIN while creatorRole is now OWNER
Issue: The init.ts bootstrap script assigns OrgRoles.ADMIN to the initial user, while the creatorRole config now assigns OrgRoles.OWNER to users who create organizations through the normal flow. This creates an inconsistent experience between deployment paths.
Why: Self-hosted customers using the init script get an ADMIN initial user, while their users who later create organizations become OWNERs with higher privilege. This asymmetry could be intentional (allowing the bootstrap admin to be downgraded), but it's worth documenting or aligning.
Fix: If intentional, add a code comment explaining why. If it should be consistent, update line 132 to use OrgRoles.OWNER.
Refs:
🧹 2) agents-docs/content/visual-builder/access-control.mdx:37-43 Documentation doesn't mention the OWNER role
Issue: The access control documentation lists only "Admin" and "Member" roles, but the UI displays "Owner" for organization creators. With this PR, all new org creators will see themselves as "Owner" in the Team Members UI, but the docs don't explain what this role means.
Why: Customers encountering an "Owner" badge will find no explanation of how it differs from Admin. The docs should explain that Owners have all Admin permissions plus protection from role changes by Admins.
Fix: Update the access control documentation to include the Owner role in the Organization Roles table.
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-scoped bug fix that correctly addresses the role hierarchy issue and improves error handling. The code changes are clean and follow existing patterns. The main suggestion is to add test coverage for the new error handling path, which could be a follow-up task. The "While You're Here" items are pre-existing gaps that are worth addressing alongside this change but aren't blockers.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
auth.ts:286 |
creatorRole change from ADMIN to OWNER | This is the intentional fix, not a bug — documented in PR description |
globals.css:289 |
Missing -ms-user-select CSS prefix | Very low confidence, modern browsers don't need this prefix |
sonner.tsx:13 |
Mobile UX tradeoff for swipe-to-dismiss | Intentional design decision documented in PR description |
org-role-selector.tsx |
OWNER excluded from selectable options | This is correct behavior, noted for awareness only |
Reviewers (8)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
4 | 0 | 0 | 2 | 1 | 0 | 1 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-breaking-changes |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-tests |
2 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
2 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 10 | 1 | 0 | 2 | 1 | 0 | 4 |
Review feedback responseThanks for the review. Here's how each item was addressed: Minor: Missing test coverage for role change hook — DeclinedThe Inline: Error message terminology — Accepted ✅Updated in d5fbc3e: "Failed to sync authorization for role change" → "Unable to update role permissions. Please try again, or contact support if the problem persists." While You're Here #1: init.ts assigns ADMIN — Addressing in this PRValid observation. The init script bootstrap user should use While You're Here #2: Docs missing OWNER role — Addressing in this PRValid observation. The access control docs need to include the Owner role. Adding documentation for the Owner role. Changeset bot — Addressed ✅Added patch changesets for both |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the previous review.
Changes reviewed:
- ✅
.changeset/unnecessary-blue-marsupial.md— Changeset for@inkeep/agents-manage-uipatch (toast UX) - ✅
.changeset/young-moccasin-scallop.md— Changeset for@inkeep/agents-corepatch (role hierarchy + error handling) - ✅
packages/agents-core/src/auth/auth.ts:369— Error message updated to customer-friendly wording
Prior feedback addressed:
The inline comment suggestion about internal jargon ("sync authorization") was implemented. The error message now correctly reads: "Unable to update role permissions. Please try again, or contact support if the problem persists."
🕐 Pending Recommendations (3)
Items from the prior review that remain applicable:
- 🟡
auth.ts:334-371Missing test coverage for role change hook - 🧹
init.ts:132Init script assigns ADMIN while creatorRole is now OWNER - 🧹
access-control.mdxDocumentation doesn't mention the OWNER role
✅ APPROVE
Summary: The delta cleanly addresses the prior inline comment with proper changeset documentation. The error message now uses customer-friendly language. The outstanding items from the prior review (test coverage, init.ts consistency, docs update) are "nice to haves" that don't block merge — they can be addressed in follow-up work if desired. Ship it! 🚀
- Add Owner role to Organization Roles table as supreme role above Admin - Update "Managing Team Members" step to include Owner role option - Update Tip to mention Owners alongside Admins having full project access Ref: PR #2028
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the previous review. The delta is 1 commit (87038f18) that addresses two "While You're Here" items from the prior review.
Delta files reviewed:
- ✅
packages/agents-core/src/auth/init.ts— Bootstrap user role changed fromADMINtoOWNERfor consistency withcreatorRole - ✅
agents-docs/content/visual-builder/access-control.mdx— Owner role added to documentation
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
access-control.mdx:52Documentation incorrectly lists Owner as a selectable role
🕐 Pending Recommendations (1)
- 🟡
auth.ts:334-371Missing test coverage for role change hook (author declined with reasoning)
💡 APPROVE WITH SUGGESTIONS
Summary: The delta cleanly addresses the two "While You're Here" items from the prior review — the init script now uses OrgRoles.OWNER consistently, and the Owner role is documented in the access control docs. The one minor issue is a documentation inaccuracy where step 4 lists Owner as a selectable role, but the UI only allows Admin or Member selection. This is a simple fix. The prior pending item (test coverage) was reasonably declined by the author. Ship it with the docs fix! 🚀
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
access-control.mdx:41-45 |
Owner description emphasizes protection over capability | Stylistic preference — current framing is valid; protection from role changes is the key differentiator from Admin |
authentication.mdx:160-162 |
Cross-doc coherence suggestion | Outside delta scope — file not changed in this PR; nice-to-have for future, not a blocker |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-docs |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-product |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
| Total | 3 | 0 | 0 | 0 | 1 | 0 | 2 |
| 2. View current members and their roles | ||
| 3. Click **Add** to add new team members | ||
| 4. Select a role (Admin or Member) for the new team member | ||
| 4. Select a role (Owner, Admin, or Member) for the new team member |
There was a problem hiding this comment.
🟡 Minor: Documentation lists Owner as a selectable role
Issue: The instruction says "Select a role (Owner, Admin, or Member)" but the UI's OrgRoleSelector component only allows selecting Admin or Member. Owner is automatically assigned to organization creators and cannot be manually assigned.
Why: Users following this documentation will find the UI doesn't match — they won't see Owner as an option when adding team members.
Fix:
| 4. Select a role (Owner, Admin, or Member) for the new team member | |
| 4. Select a role (Admin or Member) for the new team member |
Refs:
There was a problem hiding this comment.
Fixed in 86aea30 — updated to "Select a role (Admin or Member)" to match the UI behavior.
|
@claude --full-review |
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟡 Minor (1) 🟡
🟡 1) packages/agents-core/src/auth/authz/sync.ts Missing test coverage for SpiceDB sync functions
Issue: The changeOrgRole (lines 50-108) and revokeAllProjectMemberships (lines 374-435) functions have no test coverage. These are the testable units invoked by the beforeUpdateMemberRole hook that this PR modifies. Unlike the hook itself (which requires Better Auth mocking infrastructure), these functions can be tested directly by mocking the SpiceDB client.
Why: Without tests: (1) the early-return optimization in changeOrgRole when oldRole === newRole could regress, (2) the atomic batch write structure (DELETE + TOUCH in single transaction) could be accidentally broken, (3) the parallel deletion across project role types in revokeAllProjectMemberships could miss a role type due to a typo. The author noted in prior feedback that testing the hook requires infrastructure that doesn't exist — but testing these functions is straightforward with SpiceDB client mocking.
Fix: Add tests to packages/agents-core/src/auth/authz/__tests__/sync.test.ts:
describe('changeOrgRole', () => {
it('should atomically delete old role and add new role', async () => {
await changeOrgRole({ tenantId: 'tenant-1', userId: 'user-1', oldRole: 'member', newRole: 'admin' });
expect(mockSpiceClient.promises.writeRelationships).toHaveBeenCalledWith({
updates: expect.arrayContaining([
expect.objectContaining({ operation: 3 /* DELETE */, relationship: expect.objectContaining({ relation: 'member' }) }),
expect.objectContaining({ operation: 2 /* TOUCH */, relationship: expect.objectContaining({ relation: 'admin' }) }),
]),
});
});
it('should skip update when roles are the same', async () => {
await changeOrgRole({ tenantId: 'tenant-1', userId: 'user-1', oldRole: 'admin', newRole: 'admin' });
expect(mockSpiceClient.promises.writeRelationships).not.toHaveBeenCalled();
});
});This can be a follow-up task and doesn't block merge.
Refs:
- sync.ts:50-108 — changeOrgRole function
- sync.ts:374-435 — revokeAllProjectMemberships function
- sync.test.ts — existing SpiceDB tests
🕐 Pending Recommendations (1)
- 🟡
access-control.mdx:52Documentation lists Owner as a selectable role but UI only allows Admin/Member
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-scoped bug fix that correctly addresses the role hierarchy issue (creatorRole: OWNER), adds proper error handling with APIError for client-facing messages, and improves toast UX. The security model is sound — the IAM reviewer verified that Admins cannot modify Owner roles (enforced by both UI and Better Auth's role hierarchy). The one actionable suggestion is adding test coverage for the underlying SpiceDB sync functions, which can be a follow-up. The documentation issue from the prior review (line 52 lists Owner as selectable) remains open. Ship it! 🚀
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
auth.ts:362-371 |
Generic error catching without structured logging | Valid concern but out of scope for a bug fix PR — adding observability improvements is a separate enhancement |
auth.ts:340-360 |
Partial state corruption if revokeAllProjectMemberships fails after changeOrgRole succeeds |
MEDIUM confidence, complex architectural change. The current approach (throw APIError to prevent DB commit) is reasonable; SpiceDB reconciliation is a future consideration |
auth.ts:329-332, 383-385 |
Inconsistent error handling between hooks (some throw, some log-and-continue) | Pre-existing code pattern, not introduced by this PR. Valid "While You're Here" candidate but not blocking |
sonner.tsx:13 |
swipeDirections={[]} approach |
Verified as correct and idiomatic by frontend reviewer |
globals.css:289-305 |
CSS for text selection and close button positioning | Verified as correct by frontend reviewer |
access-control.mdx:41-45 |
Owner description emphasizes protection over capability | Stylistic preference, not a defect |
Reviewers (8)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-docs |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
pr-review-tests |
2 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-frontend |
3 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 10 | 1 | 0 | 0 | 0 | 1 | 4 |
Note: Frontend reviewer returned 3 positive findings (confirming correct implementation), counted as 0 issues.
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the previous review. The delta is 1 commit (2edaeba4) that addresses two prior inline comments.
Changes reviewed:
- ✅
agents-manage-ui/src/lib/toast.tsx:24— Added accessiblesr-onlyspan with dynamic text ⚠️ agents-manage-ui/src/lib/toast.tsx:13-16— Added clipboard API optional chaining (but introduces a gap)
Prior feedback addressed:
- Accessibility label — ✅ Implemented correctly
- Clipboard error handling —
⚠️ Partially implemented (see Minor finding below)
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
toast.tsx:13-16Optional chaining won't prevent TypeError on.then()when clipboard API unavailable
🕐 Pending Recommendations (1)
- 🟡
sync.ts:50-108, 374-435Missing test coverage for SpiceDB sync functions (author declined with reasoning)
💡 APPROVE WITH SUGGESTIONS
Summary: The delta correctly adds the accessibility label (sr-only span) for the copy button. The clipboard error handling is improved but has a subtle gap: when navigator.clipboard is undefined, the optional chaining returns undefined and calling .then() on undefined throws a TypeError. This only affects the rare case of non-secure HTTP contexts (not localhost), so it's minor. The fix is simple — either add ?.then() or use an if-guard like the existing useCopyToClipboard hook. Otherwise, the PR is ready to ship! 🚀
Discarded (0)
No findings discarded.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 0 | 0 |
Note: pr-review-frontend returned 0 findings — delta correctly implements prior feedback.
agents-manage-ui/src/lib/toast.tsx
Outdated
| navigator.clipboard?.writeText(text).then(() => { | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: Optional chaining won't prevent TypeError on .then()
Issue: When navigator.clipboard is undefined (e.g., in non-secure HTTP contexts), navigator.clipboard?.writeText(text) returns undefined, and calling .then() on undefined throws TypeError: Cannot read properties of undefined (reading 'then').
Why: The optional chaining only guards the writeText call, but .then() is chained directly without a guard. This is a rare edge case (only affects plain HTTP on non-localhost), but would cause a runtime error when clicking the copy button in that scenario.
Fix: Either add optional chaining on .then() or use the same guard pattern as the existing hook:
| navigator.clipboard?.writeText(text).then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }); | |
| if (navigator.clipboard?.writeText) { | |
| navigator.clipboard.writeText(text).then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }); | |
| } |
Refs:
There was a problem hiding this comment.
Good catch — fixed in 7ec1ce0. Changed to an if (navigator.clipboard) guard so .writeText() is only called when the API is available.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the previous review. The delta is 1 commit (7ec1ce07) that addresses the prior inline comment about the clipboard API guard.
Changes reviewed:
- ✅
agents-manage-ui/src/lib/toast.tsx:12-18— Changed from optional chaining to if-guard for clipboard API safety
Prior feedback addressed:
The delta correctly implements the suggested fix. The previous code:
navigator.clipboard?.writeText(text).then(() => { ... });...would throw TypeError: Cannot read properties of undefined (reading 'then') when navigator.clipboard is undefined, because optional chaining only guards the writeText call, not the .then() chain.
The new code:
if (navigator.clipboard) {
navigator.clipboard.writeText(text).then(() => { ... });
}...properly guards the entire operation and matches the pattern used in the existing useCopyToClipboard hook.
🕐 Pending Recommendations (1)
- 🟡
sync.ts:50-108, 374-435Missing test coverage for SpiceDB sync functions (author declined with reasoning — thebeforeUpdateMemberRolehook requires Better Auth mocking infrastructure that doesn't exist, and this is disproportionate for a bug fix PR)
✅ APPROVE
Summary: The delta cleanly addresses the prior feedback about clipboard API error handling. The if-guard pattern is the correct fix and matches existing codebase conventions. All prior review items have been addressed or reasonably declined. This PR is ready to ship! 🚀
Discarded (0)
No findings discarded — delta is clean.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| (orchestrator direct) | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta was 6 lines in 1 file directly implementing prior feedback — subagent reviewers were skipped in favor of orchestrator direct review.
- Change creatorRole from ADMIN to OWNER (standard hierarchy) - Add try-catch to beforeUpdateMemberRole hook with descriptive error - Disable toast swipe-to-dismiss for text selectability - Move toast close button from left to right - Add user-select: text CSS for toast content Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The beforeUpdateMemberRole hook was throwing a plain Error, but better-call's router returns Response(null, 500) for non-APIError errors, losing the message. Using APIError ensures the descriptive message flows through to the client SDK and appears in the toast. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rephrase APIError message to customer-facing language per review feedback - Add patch changesets for agents-core and agents-manage-ui Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…trol docs - Update init.ts bootstrap user from ADMIN to OWNER for consistency with creatorRole - Add Owner role to access control documentation with hierarchy explanation - Update team member management instructions to include all three roles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a toast wrapper module that auto-injects a copy button on every toast (error, success, info, warning, default). Clicking copies toast details (type, message, description, timestamp, page) to clipboard and shows a green checkmark for 2 seconds matching the existing CopyButton pattern. Migrates 74 files from direct sonner imports to the wrapper. Also cleans up the failed text-selection approach: removes swipe/dismiss overrides from the Toaster, removes data-button CSS from globals.css, and restores default toast dismiss behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner is auto-assigned to organization creators and cannot be manually selected in the UI. Fixes inaccuracy flagged in PR review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…button Add sr-only label matching existing CopyButton pattern. Use optional chaining on clipboard API and move checkmark state into .then() so it only shows on successful copy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nick-inkeep
force-pushed
the
fix/role-downgrade-toast-ux
branch
from
7ec1ce0 to
b159a11
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR correctly addresses two related concerns:
-
Role hierarchy fix —
creatorRolechanged fromADMINtoOWNER, aligning with industry-standard hierarchy (Owner > Admin > Member). ThebeforeUpdateMemberRolehook now properly handles SpiceDB sync failures by throwingAPIErrorinstead of plainError, ensuring error messages surface to users. -
Toast UX improvements — New centralized toast wrapper with copy-to-clipboard functionality, CSS overrides to move close button from left to right, and proper accessibility labels.
Prior Feedback Status
All prior review items have been addressed:
- ✅ Clipboard API guard fixed (if-check pattern)
- ✅ Accessibility label added (
sr-onlyspan) - ✅ Error message terminology updated (customer-friendly)
- ✅ Init script aligned to OWNER role
- ✅ Documentation updated with Owner role
- ✅ Test coverage reasonably declined (Better Auth hook requires mocking infrastructure that doesn't exist)
🕐 Pending Recommendations (1)
- 🟡
sync.tsMissing test coverage forchangeOrgRoleandrevokeAllProjectMembershipsSpiceDB sync functions — author reasonably declined noting this is disproportionate for a bug fix PR and the Better Auth hook mocking infrastructure doesn't exist
✅ APPROVE
Summary: Clean implementation of role hierarchy correction and toast UX improvements. All prior feedback has been addressed. The error handling pattern correctly uses APIError for proper error serialization, and the toast wrapper establishes a good centralized pattern. Ship it! 🚀
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
auth.ts:317-332 |
Silent failure in afterAcceptInvitation hook | Pre-existing code not modified by this PR — outside scope |
auth.ts:373-385 |
Silent failure in afterRemoveMember hook | Pre-existing code not modified by this PR — outside scope |
toast.tsx:13-18 |
Clipboard copy silently fails on permission denial | Already addressed by prior feedback; silent failure is acceptable UX for copy buttons |
toast.tsx:36 |
SSR safety for window.location.pathname | Not applicable — function only executes in client-side click handlers, never during SSR |
Reviewers (8)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-frontend |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 4 | 0 | 0 | 0 | 0 | 0 | 4 |
…eference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the previous review. The delta is 1 commit (50f3b092) with 2 documentation files changed.
Changes reviewed:
- ✅
agents-docs/content/deployment/(docker)/authentication.mdx:162— Added "or Owner" to the troubleshooting guidance for project access - ✅
agents-docs/content/visual-builder/access-control.mdx:41-43— Enhanced role descriptions with clearer permission boundaries
Documentation verification:
- Owner role description accurately reflects the code: Owners are protected from role changes by Admins (
creatorRole: OrgRoles.OWNERin auth.ts) - Admin description correctly states they can manage members (matches UI behavior)
- Member description correctly notes they cannot manage org settings (matches permissions.ts checks)
- The note about org creators being auto-assigned Owner is accurate (line 45)
- Step 4 correctly lists "Admin or Member" as selectable roles (matches
OrgRoleSelectorcomponent)
🕐 Pending Recommendations (1)
- 🟡
sync.tsMissing test coverage forchangeOrgRoleandrevokeAllProjectMembershipsSpiceDB sync functions — author reasonably declined noting this is disproportionate for a bug fix PR
✅ APPROVE
Summary: The delta adds documentation clarity for the Owner vs Admin distinction. The role descriptions are accurate, consistent with the code implementation, and align with the UI behavior (Owner auto-assigned to creators, only Admin/Member selectable). All prior feedback has been addressed. Ship it! 🚀
Discarded (0)
No findings discarded — delta is clean documentation improvements.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| (orchestrator direct) | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta was 4 lines in 2 docs files — subagent reviewers were skipped in favor of orchestrator direct review.
1 participant
Summary
Fixes generic error toast when admin role changes fail, corrects the role hierarchy configuration, adds a copy-to-clipboard button on all toasts, and improves toast close button positioning.
Motivation
When an admin tried to change another admin's role, the operation could fail with a generic "Failed to update role" toast because the
beforeUpdateMemberRolehook had no error handling. The error message was lost in the better-call router (plainError→Response(null, 500)→ null body). Additionally,creatorRolewas set toADMINinstead ofOWNER, inverting the intended role hierarchy. Toast UX was lacking — no way to copy error details for support, and the close button was positioned on the wrong side.Approach
Role hierarchy fix
creatorRole: OWNER— aligns with industry-standard role hierarchy (Owner > Admin > Member) where Owner is the protected creator roleAPIErrorinstead of plainError— better-call's router only serializesAPIErrorinstances to JSON responses; plainErrorproducesResponse(null, 500)with null body, losing the messageinit.tsbootstrap user updated from ADMIN to OWNER for consistencyToast UX improvements
@/lib/toastwrapper auto-injects a copy button (Copy → Check animation) into every toast. Copies type, message, description, timestamp, and page path. Useful for sharing error details with support.import { toast } from 'sonner'toimport { toast } from '@/lib/toast'so the copy button is injected globallysonner.tsxmove the close button from left to right (matches standard UX conventions)Changes
packages/agents-core/src/auth/auth.tscreatorRolefromOrgRoles.ADMINtoOrgRoles.OWNERimport { APIError } from 'better-auth/api'beforeUpdateMemberRolehook in try-catch withAPIErrorthrow (customer-facing message: "Unable to update role permissions")packages/agents-core/src/auth/init.tsOrgRoles.ADMINtoOrgRoles.OWNERfor consistency withcreatorRolechangeagents-manage-ui/src/lib/toast.tsx(new)ToastCopyActionbutton into every toast via sonner'sactionpropCopyButtoncomponent patternbuildCopyText()assembles type, message, description, timestamp, and page pathif (navigator.clipboard)for non-secure contexts<span className="sr-only">label on copy buttonagents-manage-ui/src/components/ui/sonner.tsx<style dangerouslySetInnerHTML>to reposition close button from left to right (LTR and RTL)74 files — import migration
import { toast } from 'sonner'→import { toast } from '@/lib/toast'across the manage UI codebaseagents-docs/content/visual-builder/access-control.mdxagents-docs/content/deployment/(docker)/authentication.mdxChangesets
@inkeep/agents-corepatch: role hierarchy fix and error handling@inkeep/agents-manage-uipatch: toast copy button and close button positioningTest plan
agents-corepasses cleanFuture considerations
changeOrgRoleandrevokeAllProjectMembershipsSpiceDB sync functionsadmintoownerrolePOST /api/auth/organization/deleteroute exists via Better Auth (authz-gated to Owner-only), but there is no UI or documented API for it. Consider adding a deletion flow in Settings with confirmation dialog, or explicitly disable the route viadisableOrganizationDeletionif org deletion should not be supported.Spec:
SPEC.md(in worktree root)Generated with Claude Code