fix: role hierarchy, error handling, and toast UX improvements#2037
fix: role hierarchy, error handling, and toast UX improvements#2037nick-inkeep wants to merge 12 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2374f78 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 |
|
📚 Docs Review: No additional documentation changes needed. The PR already includes comprehensive documentation updates for the role hierarchy changes:
LGTM from a docs perspective! 🚀 |
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
toast.tsx:18Unhandled promise rejection on clipboard failure - 🟠 Major:
toast.tsx:11Icon-only button missing visible focus state
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
toast.tsx:1Missing 'use client' directive for SSR safety
💭 Consider (5) 💭
💭 1) .changeset/unnecessary-blue-marsupial.md Changeset message could better reflect the main feature
Issue: The changeset says "make text selectable, move close button to top-right" but the primary new capability is the copy button that appears on all toasts.
Why: The changelog should accurately represent user-visible changes — the copy button is what customers will notice.
Fix: Consider updating to: "Add copy button to all toasts for copying error details, move close button to top-right"
💭 2) toast.tsx Toast wrapper duplicates existing copy pattern
Issue: ToastCopyAction reimplements the copy-to-clipboard functionality, but the codebase has useCopyToClipboard hook and CopyButton component.
Why: The existing hook handles SSR safety and has been tested. Reusing it would reduce code duplication.
Fix: Consider using useCopyToClipboard from @/hooks/use-copy-to-clipboard.tsx for consistency.
Refs: useCopyToClipboard
💭 3) toast.tsx:56 'Notice' label may be unclear to users
Issue: Default toast calls use 'Notice' as the type label in copied text. When users share "Notice: Operation completed" with support, the term may not clearly communicate the toast's semantic type.
Why: Minor clarity concern — 'Info' might align better with common UX terminology.
💭 4) access-control.mdx:41 Docs mention org deletion without UI
Issue: Documentation states "only Owners can delete the organization" but there's no corresponding UI or documented API for this capability.
Why: Documenting a capability that users can't find may cause confusion. The PR description acknowledges this as a future consideration.
Fix: Consider adding a note that org deletion is not yet available in the UI, or defer documenting this permission until the feature ships.
💭 5) sonner.tsx:24 Style injection creates element on each render
Issue: Using dangerouslySetInnerHTML for the style block creates a new <style> element on every Toaster re-render (e.g., theme changes).
Why: Minor performance concern — the styles are static and could be hoisted to a CSS file.
Fix: Consider extracting to globals.css or memoizing the style element.
🧹 While You're Here (1) 🧹
🧹 1) auth.ts Inconsistent error handling across organization hooks
Issue: The beforeUpdateMemberRole hook (line 362-371) now correctly throws APIError on SpiceDB sync failures, but afterAcceptInvitation (line 329-332) and afterRemoveMember (line 383-385) silently swallow errors with only console.error.
Why: This creates inconsistent user experience — some SpiceDB failures are surfaced, others are not. If sync fails during afterAcceptInvitation, users see success but their permissions aren't set up correctly.
Fix: Consider a follow-up to make error handling consistent across all hooks, or document why the after* hooks intentionally swallow errors.
Refs: auth.ts lines 329-332
💡 APPROVE WITH SUGGESTIONS
Summary: The role hierarchy fix and error handling improvements in auth.ts are solid — using APIError instead of plain Error correctly ensures error messages reach the client, and the creatorRole: OWNER change aligns with standard RBAC patterns. The IAM changes have no security concerns.
The toast UX improvements would benefit from two accessibility/error-handling improvements: (1) add .catch() to handle clipboard failures, and (2) add focus-visible styling for keyboard accessibility. Both are straightforward fixes shown in the inline suggestions.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
toast.tsx |
Silent failure when clipboard unavailable | Merged into the "unhandled promise rejection" finding — same root cause |
| Role terminology | Role names inconsistent | Not an issue — terminology is actually consistent across all surfaces (Owner/Admin/Member) |
| CSS-in-JS pattern | dangerouslySetInnerHTML is a concern | Not a real issue — the CSS string is static, not vulnerable to injection, and follows established sonner customization patterns |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-product |
4 | 0 | 3 | 0 | 0 | 0 | 1 |
pr-review-consistency |
5 | 0 | 2 | 0 | 0 | 0 | 3 |
pr-review-frontend |
4 | 0 | 1 | 0 | 3 | 0 | 0 |
pr-review-errors |
3 | 0 | 0 | 1 | 0 | 0 | 2 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 16 | 0 | 6 | 1 | 3 | 0 | 6 |
Note: pr-review-docs was rate limited and did not return findings. Manual review of docs found no issues.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review: Delta Changes Only
This re-review is scoped to the 1 commit (0cfb181f) pushed since the prior automated review.
Delta Summary: All 3 issues from the prior review have been correctly addressed:
| Prior Issue | Fix Applied |
|---|---|
🟠 Missing 'use client' directive |
✅ Added at line 1 |
| 🟠 Icon-only button missing focus state | ✅ Added focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 |
| 🟠 Unhandled promise rejection | ✅ Added .catch(() => {}) to handle clipboard failures |
The fixes are minimal and correct — no new functionality introduced, just the requested accessibility and error handling improvements.
Prior "Consider" Items (unchanged)
The 5 Consider items from the prior review remain optional suggestions that the author can address at their discretion:
- Changeset message could better reflect the copy button feature
- Toast wrapper duplicates existing
useCopyToClipboardhook - 'Notice' label may be unclear to users
- Docs mention org deletion without UI
- Style injection creates element on each render
✅ APPROVE
Summary: All blocking issues have been addressed. The role hierarchy fix (creatorRole: OWNER), error handling improvements (APIError in beforeUpdateMemberRole), and toast UX enhancements are solid. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No subagent reviewers dispatched for this delta-only re-review — the changes were minimal fixes to the toast.tsx file that addressed all prior feedback.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review: Delta Changes Only
This re-review is scoped to the 6 commits pushed since the prior automated review (0cfb181f).
Delta Summary: The changes since the last review consist of a merge commit pulling in infrastructure improvements from main:
| PR | Description |
|---|---|
| #2035 | Skip Claude PR review for bot-initiated PRs |
| #2036 | Reduce Cypress CI memory pressure and improve reliability |
| #2040 | Upgrade @openrouter/ai-sdk-provider to v2.x for AI SDK v6 compatibility |
| #2042 | Resolve agent-tools.cy.ts E2E test flakiness |
| #2043 | Align turbo cache keys between CI and Cypress workflows |
These are all infrastructure/CI changes that were already reviewed and merged to main in separate PRs. None affect the core PR functionality (role hierarchy fix, error handling, toast UX improvements).
Prior "Consider" Items (unchanged)
The 5 Consider items from the initial review remain optional suggestions:
- Changeset message could better reflect the copy button feature
- Toast wrapper duplicates existing
useCopyToClipboardhook - 'Notice' label may be unclear to users
- Docs mention org deletion without UI
- Style injection creates element on each render
✅ APPROVE
Summary: The core PR changes (role hierarchy fix with creatorRole: OWNER, error handling with APIError in beforeUpdateMemberRole, and toast UX enhancements) were approved in the prior review. All blocking issues from that review were addressed. The delta changes are infrastructure improvements from main that don't affect the PR's functionality. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No subagent reviewers dispatched — the delta consists entirely of infrastructure/CI changes from main branch merges that were already reviewed in separate PRs.
- 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>
…eference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…o toast copy button Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0206df4 to
7585bbb
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review: No Delta
This re-review is at the same commit (7585bbb3) that was approved in two prior reviews. No new changes since last approval.
Verification: All 3 issues from the initial review remain correctly addressed:
| Prior Issue | Status |
|---|---|
🟠 Missing 'use client' directive |
✅ Fixed at line 1 |
| 🟠 Icon-only button missing focus state | ✅ Fixed with focus-visible:ring-2 classes |
| 🟠 Unhandled promise rejection | ✅ Fixed with .catch(() => {}) |
Core changes confirmed correct:
creatorRole: OrgRoles.OWNER— aligns with standard Owner > Admin > Member hierarchyAPIErrorinbeforeUpdateMemberRole— ensures error messages reach the client- Bootstrap user → Owner in
init.ts— consistent withcreatorRole - Toast copy button with accessibility and error handling
Prior "Consider" Items (unchanged)
The 5 optional suggestions from the initial review remain at the author's discretion:
- Changeset message could better reflect the copy button feature
- Toast wrapper could reuse existing
useCopyToClipboardhook - 'Notice' label may be unclear to users
- Docs mention org deletion without UI
- Style injection creates element on each render
✅ APPROVE
Summary: No changes since last approval. Role hierarchy fix, error handling improvements, and toast UX enhancements are solid. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No subagent reviewers dispatched — no new commits since last approved review.
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
Re-review: Delta Changes Only
This re-review is scoped to the 1 commit (f2b9875) pushed since the prior approved review.
Delta Summary: Test infrastructure CSS change only.
| Commit | File | Change |
|---|---|---|
f2b9875 |
agents-manage-ui/src/lib/utils/test-utils/styles.css |
Added caret-color: transparent !important; to hide cursor blink in screenshot tests |
This is a standard technique for stabilizing visual regression tests — hiding the blinking cursor prevents flaky screenshot comparisons. The change:
- Affects test utilities only (no production impact)
- Is scoped to the test-utils CSS file
- Uses a well-known pattern for screenshot test stability
Core PR Changes (unchanged since approval)
The main PR functionality remains as previously approved:
✅ Role hierarchy fix — creatorRole: OrgRoles.OWNER aligns with Owner > Admin > Member hierarchy
✅ Error handling — APIError in beforeUpdateMemberRole ensures error messages reach the client
✅ Toast UX — Copy button with accessibility fixes ('use client', focus-visible, .catch())
✅ Docs updates — Owner role documented in access-control.mdx and authentication.mdx
Prior "Consider" Items (unchanged)
The 5 optional suggestions from the initial review remain at the author's discretion:
- Changeset message could better reflect the copy button feature
- Toast wrapper could reuse existing
useCopyToClipboardhook - 'Notice' label may be unclear to users
- Docs mention org deletion without UI
- Style injection creates element on each render
✅ APPROVE
Summary: No substantive changes since last approval. The delta is a 4-line test infrastructure improvement to stabilize screenshot tests — completely standard and safe. Role hierarchy fix, error handling improvements, and toast UX enhancements are solid. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No subagent reviewers dispatched — the delta consists of a single test infrastructure CSS change with no production impact.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review: Delta Changes Only
This re-review is scoped to the 2 commits pushed since the prior approved review (f2b9875).
Delta Summary: Infrastructure/CI improvements merged from main via PR #2045.
| Commit | Files | Change |
|---|---|---|
4a7e153f |
ci.yml, cypress.yml, next.config.ts |
Enable Turbopack filesystem cache for agents-manage-ui builds |
2374f78a |
— | Merge commit from main |
These changes:
- Add Next.js build cache steps to CI and Cypress workflows
- Enable
turbopackFileSystemCacheForBuild: truein Next.js config - Were already reviewed and merged to main in PR #2045
- Have no impact on the core PR functionality
Core PR Changes (unchanged since approval)
The main PR functionality remains as previously approved:
✅ Role hierarchy fix — creatorRole: OrgRoles.OWNER aligns with Owner > Admin > Member hierarchy
✅ Error handling — APIError in beforeUpdateMemberRole ensures error messages reach the client
✅ Toast UX — Copy button with accessibility fixes ('use client', focus-visible, .catch())
✅ Docs updates — Owner role documented in access-control.mdx and authentication.mdx
Prior "Consider" Items (unchanged)
The 5 optional suggestions from the initial review remain at the author's discretion:
- Changeset message could better reflect the copy button feature
- Toast wrapper could reuse existing
useCopyToClipboardhook - 'Notice' label may be unclear to users
- Docs mention org deletion without UI
- Style injection creates element on each render
✅ APPROVE
Summary: The delta consists entirely of build infrastructure improvements (Turbopack cache) that were already reviewed in PR #2045. The core PR changes (role hierarchy fix, error handling, toast UX) remain solid. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No subagent reviewers dispatched — the delta consists of infrastructure/CI changes from main that were already reviewed in PR #2045.
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