Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Southclaws
force-pushed
the
role-ordering
branch
from
8fd1789 to
541f523
Compare
📝 WalkthroughWalkthroughThis PR adds metadata field support to roles across the database, ORM, and API layers, and introduces a new PATCH /roles/order endpoint to reorder custom roles. The frontend adds drag-and-drop UI for role ordering while preventing reordering of default roles (Guest, Member, Admin). Changes
Sequence DiagramsequenceDiagram
participant Client as Frontend Client
participant Handler as HTTP Handler
participant Writer as Role Writer
participant Querier as Role Querier
participant DB as Database
Client->>Handler: PATCH /roles/order with role IDs
Handler->>Handler: Validate input (non-empty, unique)
Handler->>Writer: UpdateSortOrder(ctx, roleIDs)
Writer->>Querier: Retrieve current roles
Querier->>DB: Query existing roles
DB-->>Querier: Return roles
Writer->>Writer: Validate custom roles only
Writer->>Writer: Compute new SortKey values
Writer->>DB: Begin transaction
loop For each role
Writer->>DB: SetSortKey update
end
Writer->>DB: Commit transaction
Writer-->>Handler: Success
Handler->>Querier: ListFor() reload
Querier->>DB: Query updated roles
DB-->>Querier: Return ordered roles
Querier-->>Handler: Return role list
Handler-->>Client: 200 with reordered roles
Client->>Client: Update UI with new order
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
web/src/components/role/RoleEdit/RoleEditModal.tsx (1)
48-61: Preferaria-labelovertitlefor screen-reader accessibility on this icon-only button.
titleis not reliably announced by screen readers (it's a tooltip hint, not an accessible name per WCAG 2.1 SC 4.1.2). Anaria-labelprovides a proper accessible name for assistive technology.♿ Proposed fix
- <IconButton - variant="ghost" - size="xs" - minWidth="5" - width="5" - height="5" - padding="0" - color="fg.muted" - disabled={cannotEdit} - title={titleLabel} - onClick={disclosure.onOpen} - > + <IconButton + variant="ghost" + size="xs" + minWidth="5" + width="5" + height="5" + padding="0" + color="fg.muted" + disabled={cannotEdit} + title={titleLabel} + aria-label={titleLabel} + onClick={disclosure.onOpen} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/role/RoleEdit/RoleEditModal.tsx` around lines 48 - 61, The IconButton uses a title prop which is not reliably exposed to assistive tech; update the IconButton in RoleEditModal to add an aria-label (e.g., aria-label={titleLabel}) instead of or in addition to title so the icon-only button has an accessible name, keeping existing props like disabled={cannotEdit}, onClick={disclosure.onOpen}, and the EditIcon intact; ensure the aria-label value is meaningful (use the existing titleLabel variable).app/resources/account/role/role_writer/role_writer.go (1)
293-312:nextCustomSortKeycorrectly computes the next sort key but loads all custom roles just for a max.This works correctly and is simple. For a future optimization, you could use
Order(ent_role.ByField(ent_role.FieldSortKey, sql.OrderDesc())).Limit(1)to only fetch the role with the highest sort key, avoiding loading all custom roles into memory. Not a concern at current scale, just a note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/resources/account/role/role_writer/role_writer.go` around lines 293 - 312, The nextCustomSortKey function currently loads all custom roles to compute the max SortKey; change it to fetch only the single role with the highest SortKey by replacing the .All(ctx) call with a query that applies the same Where filter and then .Order(ent_role.ByField(ent_role.FieldSortKey, sql.OrderDesc())).Limit(1).First(ctx) (or .Only/First depending on existing patterns) and compute the next key from that single result; update imports to include the sql package used for ordering and handle the case where no role is returned the same way as before.web/src/screens/role/RoleScreen.tsx (1)
92-96: MissingKeyboardSensorreduces keyboard accessibility.Only
PointerSensoris configured, so keyboard-only users cannot reorder roles. AddingKeyboardSensorwithsortableKeyboardCoordinatesis recommended by@dnd-kitfor accessible drag-and-drop.♿ Proposed fix to add keyboard support
import { DndContext, DragEndEvent, + KeyboardSensor, PointerSensor, useSensor, useSensors, } from "@dnd-kit/core"; import { SortableContext, arrayMove, rectSortingStrategy, + sortableKeyboardCoordinates, useSortable, } from "@dnd-kit/sortable";const sensors = useSensors( useSensor(PointerSensor, { activationConstraint: { distance: 3 }, }), + useSensor(KeyboardSensor, { + coordinateGetter: sortableKeyboardCoordinates, + }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/screens/role/RoleScreen.tsx` around lines 92 - 96, The drag-and-drop setup in RoleScreen.tsx only registers PointerSensor, which prevents keyboard-only users from reordering roles; to fix, import KeyboardSensor and sortableKeyboardCoordinates from `@dnd-kit` (or their respective packages) and add useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates }) to the useSensors(...) call alongside the existing useSensor(PointerSensor, ...) so the sensors constant supports keyboard interaction for the sortable list.web/src/components/role/RoleCard.tsx (1)
43-55: UnnecessaryFragmentwrapper around the conditional badge.The
<>...</>fragment on lines 45-51 wraps a single expression (the ternary). You can remove it and use the ternary directly.✨ Proposed simplification
<HStack> {isDefault && ( - <> - {isCustomDefault ? ( - <Badge size="sm">Default + Custom</Badge> - ) : ( - <Badge size="sm">Default</Badge> - )} - </> + isCustomDefault ? ( + <Badge size="sm">Default + Custom</Badge> + ) : ( + <Badge size="sm">Default</Badge> + ) )} {dragHandle} </HStack>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/role/RoleCard.tsx` around lines 43 - 55, Remove the unnecessary React fragment wrapping the ternary inside the HStack: when rendering the conditional badge based on isDefault and isCustomDefault in RoleCard (the Badge rendering block), replace the fragment-wrapped ternary with the ternary expression directly so the code reads {isDefault && (isCustomDefault ? <Badge size="sm">Default + Custom</Badge> : <Badge size="sm">Default</Badge>)} while keeping dragHandle and HStack unchanged.app/resources/account/role/role_querier/role_querier.go (1)
55-70:List()callslookupDefaultRolesthree times via individual getters — 3 redundant DB queries.
GetMemberRole,GetGuestRole, andGetAdminRoleeach internally calllookupDefaultRoles, which executes a DB query. InList()all three are called sequentially, tripling the same query. This was already 2 calls before this PR and is now 3.Consider calling
lookupDefaultRolesonce directly and mapping the results inline, similar to howListFor(line 102) already does it.♻️ Proposed consolidation
- defaultRole, err := q.GetMemberRole(ctx) - if err != nil { - return nil, fault.Wrap(err, fctx.With(ctx)) - } - - guestRole, err := q.GetGuestRole(ctx) - if err != nil { - return nil, fault.Wrap(err, fctx.With(ctx)) - } - - adminRole, err := q.GetAdminRole(ctx) - if err != nil { - return nil, fault.Wrap(err, fctx.With(ctx)) - } - - mapped = append(mapped, defaultRole, guestRole, adminRole) + guestEnt, memberEnt, adminEnt, err := q.lookupDefaultRoles(ctx) + if err != nil { + return nil, fault.Wrap(err, fctx.With(ctx)) + } + + for _, pair := range []struct { + ent *ent.Role + fallback *role.Role + }{ + {guestEnt, &role.DefaultRoleGuest}, + {memberEnt, &role.DefaultRoleMember}, + {adminEnt, &role.DefaultRoleAdmin}, + } { + if pair.ent != nil { + r, err := role.Map(pair.ent) + if err != nil { + return nil, fault.Wrap(err, fctx.With(ctx)) + } + mapped = append(mapped, r) + } else { + mapped = append(mapped, pair.fallback) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/resources/account/role/role_querier/role_querier.go` around lines 55 - 70, List() is making three redundant DB queries by calling GetMemberRole, GetGuestRole, and GetAdminRole (each calls lookupDefaultRoles); instead call lookupDefaultRoles(ctx) once, handle its returned slice/map and assign the defaultRole, guestRole, and adminRole values into mapped inline (like ListFor does), replacing the three separate calls to GetMemberRole/GetGuestRole/GetAdminRole and preventing duplicate DB queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/resources/account/role/role_querier/role_querier.go`:
- Around line 55-70: List() is making three redundant DB queries by calling
GetMemberRole, GetGuestRole, and GetAdminRole (each calls lookupDefaultRoles);
instead call lookupDefaultRoles(ctx) once, handle its returned slice/map and
assign the defaultRole, guestRole, and adminRole values into mapped inline (like
ListFor does), replacing the three separate calls to
GetMemberRole/GetGuestRole/GetAdminRole and preventing duplicate DB queries.
In `@app/resources/account/role/role_writer/role_writer.go`:
- Around line 293-312: The nextCustomSortKey function currently loads all custom
roles to compute the max SortKey; change it to fetch only the single role with
the highest SortKey by replacing the .All(ctx) call with a query that applies
the same Where filter and then .Order(ent_role.ByField(ent_role.FieldSortKey,
sql.OrderDesc())).Limit(1).First(ctx) (or .Only/First depending on existing
patterns) and compute the next key from that single result; update imports to
include the sql package used for ordering and handle the case where no role is
returned the same way as before.
In `@web/src/components/role/RoleCard.tsx`:
- Around line 43-55: Remove the unnecessary React fragment wrapping the ternary
inside the HStack: when rendering the conditional badge based on isDefault and
isCustomDefault in RoleCard (the Badge rendering block), replace the
fragment-wrapped ternary with the ternary expression directly so the code reads
{isDefault && (isCustomDefault ? <Badge size="sm">Default + Custom</Badge> :
<Badge size="sm">Default</Badge>)} while keeping dragHandle and HStack
unchanged.
In `@web/src/components/role/RoleEdit/RoleEditModal.tsx`:
- Around line 48-61: The IconButton uses a title prop which is not reliably
exposed to assistive tech; update the IconButton in RoleEditModal to add an
aria-label (e.g., aria-label={titleLabel}) instead of or in addition to title so
the icon-only button has an accessible name, keeping existing props like
disabled={cannotEdit}, onClick={disclosure.onOpen}, and the EditIcon intact;
ensure the aria-label value is meaningful (use the existing titleLabel
variable).
In `@web/src/screens/role/RoleScreen.tsx`:
- Around line 92-96: The drag-and-drop setup in RoleScreen.tsx only registers
PointerSensor, which prevents keyboard-only users from reordering roles; to fix,
import KeyboardSensor and sortableKeyboardCoordinates from `@dnd-kit` (or their
respective packages) and add useSensor(KeyboardSensor, { coordinateGetter:
sortableKeyboardCoordinates }) to the useSensors(...) call alongside the
existing useSensor(PointerSensor, ...) so the sensors constant supports keyboard
interaction for the sortable list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.