migration to services subpage from modals - issue #39#49
Conversation
…ial field in service request schema
…e role assignment logic in service
…ions by role; implement user retrieval by role ID
…PI response formatting
…d adding support for start and end icons
…ed styles and transitions; update event_type enum formatting in schema
…Modal components as part of code cleanup
…rating a direct link for service management
…ut with label, error, and hint support
…ked state and accessibility features
…ster component in layout for notifications
… API error parsing and form integration
…form structure and validation
…ermissions, and roles tabs; add service details and authentication sections
…ault Authly service; enhance user experience by preventing modifications
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughIntroduce per-resource role permissions: add RolePermission model and migration, replace role-level bitmask with permissions arrays, implement propagation logic to bulk-update user permissions and increment user permission versions, update APIs/docs/schemas, and add matching frontend hooks, UI components, and tooling changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminUI as Admin UI
participant RoleSvc as RoleService
participant PermRepo as PermissionRepo
participant UserRepo as UserPermissionRepo
participant DB as Database
AdminUI->>RoleSvc: UpdateRole(updatedRole)
RoleSvc->>PermRepo: Load oldRole (with Permissions)
Note right of RoleSvc: compute per-resource and global diffs (added/removed bits)
RoleSvc->>PermRepo: BulkUpdateUserPermissionsByRole(roleID, resource, addedBits, removedBits)
PermRepo->>DB: UPDATE user_permissions (bitwise ops)
RoleSvc->>PermRepo: BulkDeleteUserPermissionsByRole(roleID, resource) (if resource removed)
PermRepo->>DB: DELETE FROM user_permissions
RoleSvc->>PermRepo: GetUsersByRoleID(roleID)
PermRepo->>DB: SELECT DISTINCT user_id FROM user_permissions
RoleSvc->>UserRepo: IncrementPermissionVersion(userIDs)
UserRepo->>DB: UPDATE users SET permission_version = permission_version + 1
RoleSvc-->>AdminUI: return updated role
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
WalkthroughThis pull request refactors the permission system from a single bitmask field to a granular, resource-based model. It introduces new RolePermission and RolePermissionRequest types, updates role creation/update flows to handle multiple permissions, migrates field names to snake_case convention, and provides new UI components for service management with tabs, form fields, and credential display. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Handler as Role Handler
participant Service as Role Service
participant PermRepo as Permission Repo
participant UserPermRepo as User Permission Repo
participant DB as Database
User->>Handler: UpdateRole(roleID, permissions)
Handler->>DB: Fetch old role with permissions
DB-->>Handler: oldRole with Permissions
Handler->>DB: Update Role + Permissions
DB-->>Handler: updatedRole
Handler->>Service: propagateRoleChanges(oldRole, updatedRole)
Service->>Service: Calculate delta per resource
Service->>PermRepo: GetUsersByRoleID(roleID)
PermRepo-->>Service: List of userIDs
loop For each resource delta
alt Resource added
Service->>UserPermRepo: Create/Update user permissions
UserPermRepo->>DB: Insert/Update records
else Resource removed
Service->>UserPermRepo: BulkDeleteUserPermissionsByRole
UserPermRepo->>DB: Delete records
else Bitmask changed
Service->>UserPermRepo: BulkUpdateUserPermissionsByRole
UserPermRepo->>DB: Update bitmask via bitwise operation
end
end
loop For each affected user
Service->>UserPermRepo: Increment permission version
UserPermRepo->>DB: Update permission_version
end
Handler-->>User: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@apps/api/internal/domain/role/service.go`:
- Around line 350-378: The lookup that finds an existing user permission for
each role permission (inside the role.Permissions loop) misses global
permissions because it only matches up.Resource != nil; update the search over
userPerms to explicitly handle nil resources by treating nil==nil as a match for
"__global__" (i.e. if up.Resource == nil && rp.Resource == nil then set
targetPerm), otherwise if up.Resource != nil compare *up.Resource to
resourceKey; keep the rest of the creation logic but ensure the created
permission's Resource remains nil for global permissions and mark
processedResources["__global__"] appropriately to avoid duplicates.
- Around line 217-223: The loop that calls
s.permissionRepo.IncrementPermissionVersion for each uid currently ignores
errors; change it to propagate failures by checking the returned error and
returning it immediately (or returning a wrapped error with context including
uid and oldRole.ID.String()) so token versions don't stay stale; locate the code
around s.permissionRepo.GetUsersByRoleID(...) and update the for _, uid := range
userIDs loop to handle and return IncrementPermissionVersion errors instead of
discarding them.
- Around line 193-205: The FindUserPermission error is being treated as "not
found" unconditionally; update the handling in the block around
s.permissionRepo.FindUserPermission(userID, oldRole.ServiceID.String(), resPtr)
to import "errors" and use errors.Is(err, gorm.ErrRecordNotFound) to only create
a new permission when the record is truly not found, and return any other err
immediately; keep the creation logic for permission.UserPermission and the
subsequent s.permissionRepo.CreateUserPermission(newPerm) unchanged but guarded
by the not-found check.
- Around line 107-112: The unconditional call to
tx.Model(updatedRole).Association("Permissions").Replace(...) can wipe
permissions when updatedRole.Permissions is unpopulated; guard it so you only
replace associations when permissions were explicitly provided by the caller. In
the role update logic (around the Association("Permissions").Replace call in
service.go), wrap the Replace call in a conditional that checks for nil (not
length) — e.g., if updatedRole.Permissions != nil {
tx.Model(updatedRole).Association("Permissions").Replace(...) } — so an omitted
Permissions field leaves existing role_permissions intact while allowing an
intentional empty slice to clear permissions; leave the
txSvc.propagateRoleChanges(oldRole, updatedRole) call as-is.
In `@apps/api/internal/migrations/000020_create_role_permissions_table.up.sql`:
- Around line 1-12: The role_permissions table currently allows NULL resource
and uses UNIQUE(role_id, resource), which combined with soft deletes
(deleted_at) permits duplicates and blocks re-inserts; change the resource
column to NOT NULL and replace the UNIQUE(role_id, resource) constraint with a
partial unique index that only enforces uniqueness for active rows (WHERE
deleted_at IS NULL). Locate the CREATE TABLE for role_permissions and update the
resource definition to NOT NULL, remove the UNIQUE(role_id, resource) line, and
add a partial unique index like UNIQUE INDEX ON role_permissions(role_id,
resource) WHERE deleted_at IS NULL (ensuring any migration also drops the old
constraint/index if present). Ensure the existing idx_role_permissions_role_id
index is preserved.
In
`@apps/web/src/components/dashboard/admin/services/overview/AuthenticationSection.tsx`:
- Around line 43-90: The list rendering uses key={uri} which will collide for
duplicate entries; change the key to a stable unique value such as
`${uri}-${idx}` or just use the index `idx` in the map (modify the map callback
referencing redirectURIs.map((uri, idx) => ...) and the motion.div key) to avoid
collisions, and add an accessible label to the icon-only add Button (the one
using onAddRedirect and HugeiconsIcon/PlusSignIcon) by adding an aria-label like
"Add redirect URI" or visible text to meet accessibility requirements.
In
`@apps/web/src/components/dashboard/admin/services/overview/CredentialsCard.tsx`:
- Around line 28-37: The Client ID copy button lacks the toast feedback that the
Client Secret uses; update the copyId handler (used by the onClick on the button
rendering copiedId, Tick01Icon/Copy01Icon) to call the same
toast.success("Copied") (or the shared helper the secret uses) when copy
succeeds so both copy actions provide consistent feedback; ensure you
import/toast or reuse the existing toast helper and keep the checkmark visual
behavior unchanged.
In `@apps/web/src/components/dashboard/admin/services/overview/MetadataCard.tsx`:
- Around line 34-43: The copy button lacks an accessible label and a button
type; update the button element that calls copyId(serviceId) (the one rendering
HugeiconsIcon with Copy01Icon/Tick01Icon and using copiedId) to include
type="button" and an appropriate aria-label (e.g., "Copy service ID" or "Copied"
depending on copiedId) so screen readers can announce the action and to prevent
accidental form submission.
In `@apps/web/src/components/dashboard/admin/services/overview/SaveActions.tsx`:
- Around line 9-39: The prop type for onSave in SaveActionsProps is incorrect:
it currently expects a React.FormEvent but the Button passes a click handler, so
update SaveActionsProps.onSave to accept a React.MouseEvent handler (e.g.,
React.MouseEvent<HTMLButtonElement> or a generic React.MouseEvent) and update
SaveActions's destructured prop signature accordingly so the onClick signature
of the Button matches the declared type (refer to SaveActionsProps, onSave,
SaveActions and the Button onClick usage).
In `@apps/web/src/components/dashboard/admin/services/overview/StatusCard.tsx`:
- Around line 15-22: The Switch in StatusCard.tsx is missing an accessible name;
update the Switch usage (the one with props checked={isActive}
onCheckedChange={() => onToggle()} disabled={disabled}) to include an accessible
label—either add an explicit aria-label like "Status toggle" or add
aria-labelledby pointing to the existing heading/span by giving the heading/span
a stable id (e.g., id="status-label") and set aria-labelledby="status-label" on
the Switch so screen readers announce the control.
In `@apps/web/src/components/ui/Switch.tsx`:
- Around line 12-29: The Switch component's internal onClick handler is being
overridden because {...props} is spread after the explicit onClick; update the
rendering of the button in the Switch (the forwardRef component) to either
spread {...props} before the explicit controlled props or compose the click
handlers: capture any consumer onClick from props (e.g., const { onClick:
propsOnClick, ...rest } = props) and set button onClick to an composed function
that first runs the internal toggle (if not disabled, call
onCheckedChange(!checked)) and then calls propsOnClick(event); ensure disabled
still short-circuits and ref and other props are preserved.
In `@apps/web/src/components/ui/TagInput.tsx`:
- Around line 61-75: The icon-only remove button in TagInput.tsx lacks an
accessible label so screen readers can't announce the action; update the button
(the button that calls removeTag(tag) and renders HugeiconsIcon with
Cancel01Icon) to include an accessible name (e.g., add aria-label="Remove tag"
or aria-label={`Remove ${tag}`} and/or a visually-hidden text alternative), and
ensure the label is localized if needed and still present when disabled is false
so assistive tech can identify the remove action.
🧹 Nitpick comments (3)
apps/web/src/components/ui/FormField.tsx (1)
15-18: Consider associating label with input for improved accessibility.The
<label>element is not associated with an input viahtmlFor/id. Screen readers may not correctly announce the label for the input. Sincechildrenis a genericReactNode, you could add an optionalidprop to support this pattern.♿ Optional enhancement for accessibility
interface FormFieldProps { label?: string; error?: string; hint?: string; children: React.ReactNode; className?: string; + id?: string; } -export function FormField({ label, error, hint, children, className }: FormFieldProps) { +export function FormField({ label, error, hint, children, className, id }: FormFieldProps) { return ( <div className={cn("space-y-1.5", className)}> {label && ( - <label className="text-xs font-medium text-white/60 transition-colors group-focus-within:text-white/80"> + <label htmlFor={id} className="text-xs font-medium text-white/60 transition-colors group-focus-within:text-white/80"> {label} </label> )}apps/web/src/components/ui/Input.tsx (1)
16-59: Expose validation state viaaria-invalid.
errorcurrently only affects styling. Addingaria-invalidimproves accessibility and keeps the control’s state machine clear for assistive tech.♿ Suggested change
<input type={type} className={cn( inputClasses, // Padding with icons startIcon && "pl-9 pr-3", endIcon && !startIcon && "pl-3 pr-9", startIcon && endIcon && "pl-9 pr-9", )} ref={ref} disabled={disabled} + aria-invalid={error || undefined} {...props} /> @@ - return <input type={type} className={inputClasses} ref={ref} disabled={disabled} {...props} />; + return ( + <input + type={type} + className={inputClasses} + ref={ref} + disabled={disabled} + aria-invalid={error || undefined} + {...props} + /> + );apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx (1)
55-68: Normalize redirect URIs before validation.Line 55-68: Trim once and reuse so trailing/leading whitespace doesn’t falsely trigger “Invalid URL format.”
♻️ Suggested refactor
const handleAddRedirect = () => { - if (!newRedirectURI.trim()) return; - try { - new URL(newRedirectURI); + const normalized = newRedirectURI.trim(); + if (!normalized) return; + try { + new URL(normalized); } catch { toast.error("Invalid URL format"); return; } - if (redirectURIs.includes(newRedirectURI.trim())) { + if (redirectURIs.includes(normalized)) { toast.error("URI already exists"); return; } - setRedirectURIs([...redirectURIs, newRedirectURI.trim()]); + setRedirectURIs([...redirectURIs, normalized]); setNewRedirectURI(""); toast.success("Redirect URI added"); };
| CREATE TABLE role_permissions ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, | ||
| deleted_at TIMESTAMP WITH TIME ZONE, | ||
| role_id UUID NOT NULL REFERENCES roles(id) ON DELETE CASCADE, | ||
| resource VARCHAR(100), | ||
| bitmask NUMERIC(20) NOT NULL DEFAULT 0, | ||
| UNIQUE(role_id, resource) | ||
| ); | ||
|
|
||
| CREATE INDEX idx_role_permissions_role_id ON role_permissions(role_id); |
There was a problem hiding this comment.
Enforce resource non-null + uniqueness for active rows.
resource is nullable and combined with soft deletes, which allows multiple NULLs and can prevent re-adding a permission after soft delete. Consider making resource NOT NULL and enforcing uniqueness only for active rows.
🛠️ Proposed migration adjustment
CREATE TABLE role_permissions (
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP,
updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP,
deleted_at TIMESTAMP WITH TIME ZONE,
role_id UUID NOT NULL REFERENCES roles(id) ON DELETE CASCADE,
- resource VARCHAR(100),
+ resource VARCHAR(100) NOT NULL,
bitmask NUMERIC(20) NOT NULL DEFAULT 0,
- UNIQUE(role_id, resource)
);
+CREATE UNIQUE INDEX uq_role_permissions_role_id_resource
+ ON role_permissions(role_id, resource)
+ WHERE deleted_at IS NULL;
+
CREATE INDEX idx_role_permissions_role_id ON role_permissions(role_id);🤖 Prompt for AI Agents
In `@apps/api/internal/migrations/000020_create_role_permissions_table.up.sql`
around lines 1 - 12, The role_permissions table currently allows NULL resource
and uses UNIQUE(role_id, resource), which combined with soft deletes
(deleted_at) permits duplicates and blocks re-inserts; change the resource
column to NOT NULL and replace the UNIQUE(role_id, resource) constraint with a
partial unique index that only enforces uniqueness for active rows (WHERE
deleted_at IS NULL). Locate the CREATE TABLE for role_permissions and update the
resource definition to NOT NULL, remove the UNIQUE(role_id, resource) line, and
add a partial unique index like UNIQUE INDEX ON role_permissions(role_id,
resource) WHERE deleted_at IS NULL (ensuring any migration also drops the old
constraint/index if present). Ensure the existing idx_role_permissions_role_id
index is preserved.
apps/web/src/components/dashboard/admin/services/overview/MetadataCard.tsx
Outdated
Show resolved
Hide resolved
| interface SaveActionsProps { | ||
| hasChanges: boolean; | ||
| isSaving: boolean; | ||
| onSave: (e: React.FormEvent) => void; | ||
| disabled?: boolean; | ||
| } | ||
|
|
||
| export function SaveActions({ hasChanges, isSaving, onSave, disabled = false }: SaveActionsProps) { | ||
| return ( | ||
| <div className="pt-6 flex items-center justify-between border-t border-white/10"> | ||
| <div className="text-xs text-white/40"> | ||
| {hasChanges ? ( | ||
| <span className="text-amber-400 flex items-center gap-2"> | ||
| <div className="w-1.5 h-1.5 rounded-full bg-amber-400 animate-pulse" /> | ||
| Unsaved changes | ||
| </span> | ||
| ) : ( | ||
| "All changes saved" | ||
| )} | ||
| </div> | ||
| <Button | ||
| onClick={onSave} | ||
| disabled={!hasChanges || isSaving || disabled} | ||
| icon={isSaving ? <Loader size="sm" /> : <HugeiconsIcon icon={FloppyDiskIcon} size={16} />} | ||
| className={cn( | ||
| "cursor-pointer", | ||
| hasChanges && "bg-emerald-500 hover:bg-emerald-600 border-emerald-600 text-white", | ||
| )} | ||
| > | ||
| {isSaving ? "Saving..." : "Save Changes"} | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where SaveActions is imported/used
rg "SaveActions" --type tsx --type ts -B 2 -A 2Repository: Anvoria/authly
Length of output: 85
🏁 Script executed:
# Check the Button component implementation to understand how it handles onClick/type
fd "Button" --type f | grep -E "\.(tsx|ts)$" | head -20Repository: Anvoria/authly
Length of output: 95
🏁 Script executed:
# Look for the ServiceForm component mentioned in the original scratchpad
find . -name "*ServiceForm*" -type fRepository: Anvoria/authly
Length of output: 133
🏁 Script executed:
# Check Button component implementation
cat apps/web/src/components/ui/Button.tsxRepository: Anvoria/authly
Length of output: 6862
🏁 Script executed:
# Check ServiceForm to see how SaveActions is used and what onSave does
cat apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsxRepository: Anvoria/authly
Length of output: 7260
🏁 Script executed:
# Also search for SaveActions usage with context
rg "SaveActions" -C 5Repository: Anvoria/authly
Length of output: 4762
Fix type annotation for onSave handler to match actual event type.
onSave is typed as React.FormEvent, but it receives a React.MouseEvent from the button's onClick. While the code works at runtime (both event types have preventDefault()), the type mismatch causes incorrect typing that could mislead future developers about the event semantics.
Suggested fix
interface SaveActionsProps {
hasChanges: boolean;
isSaving: boolean;
- onSave: (e: React.FormEvent) => void;
+ onSave: (e: React.MouseEvent<HTMLButtonElement>) => void;
disabled?: boolean;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface SaveActionsProps { | |
| hasChanges: boolean; | |
| isSaving: boolean; | |
| onSave: (e: React.FormEvent) => void; | |
| disabled?: boolean; | |
| } | |
| export function SaveActions({ hasChanges, isSaving, onSave, disabled = false }: SaveActionsProps) { | |
| return ( | |
| <div className="pt-6 flex items-center justify-between border-t border-white/10"> | |
| <div className="text-xs text-white/40"> | |
| {hasChanges ? ( | |
| <span className="text-amber-400 flex items-center gap-2"> | |
| <div className="w-1.5 h-1.5 rounded-full bg-amber-400 animate-pulse" /> | |
| Unsaved changes | |
| </span> | |
| ) : ( | |
| "All changes saved" | |
| )} | |
| </div> | |
| <Button | |
| onClick={onSave} | |
| disabled={!hasChanges || isSaving || disabled} | |
| icon={isSaving ? <Loader size="sm" /> : <HugeiconsIcon icon={FloppyDiskIcon} size={16} />} | |
| className={cn( | |
| "cursor-pointer", | |
| hasChanges && "bg-emerald-500 hover:bg-emerald-600 border-emerald-600 text-white", | |
| )} | |
| > | |
| {isSaving ? "Saving..." : "Save Changes"} | |
| </Button> | |
| interface SaveActionsProps { | |
| hasChanges: boolean; | |
| isSaving: boolean; | |
| onSave: (e: React.MouseEvent<HTMLButtonElement>) => void; | |
| disabled?: boolean; | |
| } | |
| export function SaveActions({ hasChanges, isSaving, onSave, disabled = false }: SaveActionsProps) { | |
| return ( | |
| <div className="pt-6 flex items-center justify-between border-t border-white/10"> | |
| <div className="text-xs text-white/40"> | |
| {hasChanges ? ( | |
| <span className="text-amber-400 flex items-center gap-2"> | |
| <div className="w-1.5 h-1.5 rounded-full bg-amber-400 animate-pulse" /> | |
| Unsaved changes | |
| </span> | |
| ) : ( | |
| "All changes saved" | |
| )} | |
| </div> | |
| <Button | |
| onClick={onSave} | |
| disabled={!hasChanges || isSaving || disabled} | |
| icon={isSaving ? <Loader size="sm" /> : <HugeiconsIcon icon={FloppyDiskIcon} size={16} />} | |
| className={cn( | |
| "cursor-pointer", | |
| hasChanges && "bg-emerald-500 hover:bg-emerald-600 border-emerald-600 text-white", | |
| )} | |
| > | |
| {isSaving ? "Saving..." : "Save Changes"} | |
| </Button> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/dashboard/admin/services/overview/SaveActions.tsx`
around lines 9 - 39, The prop type for onSave in SaveActionsProps is incorrect:
it currently expects a React.FormEvent but the Button passes a click handler, so
update SaveActionsProps.onSave to accept a React.MouseEvent handler (e.g.,
React.MouseEvent<HTMLButtonElement> or a generic React.MouseEvent) and update
SaveActions's destructured prop signature accordingly so the onClick signature
of the Button matches the declared type (refer to SaveActionsProps, onSave,
SaveActions and the Button onClick usage).
| <h4 className="text-xs font-semibold text-white uppercase tracking-wider">Status</h4> | ||
|
|
||
| <div className="flex items-center justify-between"> | ||
| <span className={cn("text-sm font-medium", isActive ? "text-emerald-400" : "text-white/40")}> | ||
| {isActive ? "Active" : "Disabled"} | ||
| </span> | ||
| <Switch checked={isActive} onCheckedChange={() => onToggle()} disabled={disabled} /> | ||
| </div> |
There was a problem hiding this comment.
Add an accessible name for the switch.
Right now the toggle has no accessible label; screen readers will announce an unlabeled “switch.” Provide an aria-label (or link to a labeled element) so the control is discoverable.
♿ Suggested fix
- <Switch checked={isActive} onCheckedChange={() => onToggle()} disabled={disabled} />
+ <Switch
+ checked={isActive}
+ onCheckedChange={onToggle}
+ disabled={disabled}
+ aria-label={`Service status: ${isActive ? "Active" : "Disabled"}`}
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h4 className="text-xs font-semibold text-white uppercase tracking-wider">Status</h4> | |
| <div className="flex items-center justify-between"> | |
| <span className={cn("text-sm font-medium", isActive ? "text-emerald-400" : "text-white/40")}> | |
| {isActive ? "Active" : "Disabled"} | |
| </span> | |
| <Switch checked={isActive} onCheckedChange={() => onToggle()} disabled={disabled} /> | |
| </div> | |
| <h4 className="text-xs font-semibold text-white uppercase tracking-wider">Status</h4> | |
| <div className="flex items-center justify-between"> | |
| <span className={cn("text-sm font-medium", isActive ? "text-emerald-400" : "text-white/40")}> | |
| {isActive ? "Active" : "Disabled"} | |
| </span> | |
| <Switch | |
| checked={isActive} | |
| onCheckedChange={onToggle} | |
| disabled={disabled} | |
| aria-label={`Service status: ${isActive ? "Active" : "Disabled"}`} | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/dashboard/admin/services/overview/StatusCard.tsx`
around lines 15 - 22, The Switch in StatusCard.tsx is missing an accessible
name; update the Switch usage (the one with props checked={isActive}
onCheckedChange={() => onToggle()} disabled={disabled}) to include an accessible
label—either add an explicit aria-label like "Status toggle" or add
aria-labelledby pointing to the existing heading/span by giving the heading/span
a stable id (e.g., id="status-label") and set aria-labelledby="status-label" on
the Switch so screen readers announce the control.
| {!disabled && ( | ||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| removeTag(tag); | ||
| }} | ||
| className="ml-1 p-0.5 rounded-md outline-none transition-colors hover:bg-white/20 cursor-pointer" | ||
| > | ||
| <HugeiconsIcon | ||
| icon={Cancel01Icon} | ||
| size={12} | ||
| className="text-white/40 group-hover/tag:text-white/60" | ||
| /> | ||
| </button> |
There was a problem hiding this comment.
Icon-only remove button needs an accessible label.
Without an aria-label, screen readers can’t announce the remove action.
🛠️ Suggested fix
- <button
+ <button
type="button"
onClick={(e) => {
e.stopPropagation();
removeTag(tag);
}}
className="ml-1 p-0.5 rounded-md outline-none transition-colors hover:bg-white/20 cursor-pointer"
+ aria-label={`Remove ${tag}`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {!disabled && ( | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| removeTag(tag); | |
| }} | |
| className="ml-1 p-0.5 rounded-md outline-none transition-colors hover:bg-white/20 cursor-pointer" | |
| > | |
| <HugeiconsIcon | |
| icon={Cancel01Icon} | |
| size={12} | |
| className="text-white/40 group-hover/tag:text-white/60" | |
| /> | |
| </button> | |
| {!disabled && ( | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| removeTag(tag); | |
| }} | |
| className="ml-1 p-0.5 rounded-md outline-none transition-colors hover:bg-white/20 cursor-pointer" | |
| aria-label={`Remove ${tag}`} | |
| > | |
| <HugeiconsIcon | |
| icon={Cancel01Icon} | |
| size={12} | |
| className="text-white/40 group-hover/tag:text-white/60" | |
| /> | |
| </button> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/ui/TagInput.tsx` around lines 61 - 75, The icon-only
remove button in TagInput.tsx lacks an accessible label so screen readers can't
announce the action; update the button (the button that calls removeTag(tag) and
renders HugeiconsIcon with Cancel01Icon) to include an accessible name (e.g.,
add aria-label="Remove tag" or aria-label={`Remove ${tag}`} and/or a
visually-hidden text alternative), and ensure the label is localized if needed
and still present when disabled is false so assistive tech can identify the
remove action.
…y provided to prevent unintended overwrites
…od to ensure proper permission version incrementing
…urce permissions correctly
…ts in the dashboard overview
…abled and ensure onClick is called
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/internal/domain/service/handler.go (1)
53-61: Document the new 400 response for invalid IDs.GetService can now return 400 on UUID parse failures, but the Swagger annotations only list 404. Please add a 400 failure entry so clients know about the validation error.
📝 Suggested doc update
// `@Param` id path string true "Service ID" // `@Success` 200 {object} utils.Response{data=ServiceDataResponse} "Service details" +// `@Failure` 400 {object} utils.APIError "Invalid ID format" // `@Failure` 404 {object} utils.APIError "Service not found" // `@Router` /admin/services/{id} [get]apps/web/src/components/dashboard/admin/services/ServiceRow.tsx (1)
44-69: Addtype="button"to the copy button.Similar to MetadataCard, this button lacks an explicit
type="button", which could cause unintended form submission if the component is ever nested within a form.🔧 Proposed fix
<TableCell className="py-3"> <button + type="button" onClick={handleCopy} className={cn(apps/web/src/components/dashboard/admin/services/CreateServiceModal.tsx (1)
37-42: Minor regex edge case for slug generation.The regex
/^-|-$/gremoves only a single hyphen from the start or end. If a name produces multiple leading/trailing hyphens, they won't all be removed in one pass.Consider using a more robust pattern:
🔧 Suggested fix
newState.slug = newName .toLowerCase() .replace(/[^a-z0-9-_]/g, "-") .replace(/-+/g, "-") - .replace(/^-|-$/g, "") || ""; + .replace(/^-+|-+$/g, "") || "";
🤖 Fix all issues with AI agents
In `@apps/api/internal/domain/role/model.go`:
- Around line 23-27: The Resource field in RolePermission is a *string so when
nil it serializes as JSON null which conflicts with the Swagger schema; update
the json tag to omit nulls (add ,omitempty) on the Resource field in
RolePermission (and any other structs that define Resource, e.g., the similar
struct at lines ~40-43) so the field is omitted instead of emitting null, or
alternatively update the API model/schema to mark Resource as nullable—pick one
approach and apply consistently to the Resource fields.
In `@apps/api/internal/domain/role/service.go`:
- Around line 119-234: propagateRoleChanges currently builds newMap from
newRole.Permissions and can delete existing permissions when newRole.Permissions
is nil (meaning "not provided"); change the logic so that if newRole.Permissions
== nil you preserve existing permissions by copying oldMap into newMap
(including the global Bitmask) instead of constructing newMap from newRole;
otherwise keep the existing behavior of building newMap from
newRole.Bitmask/newRole.Permissions. Update references inside
propagateRoleChanges to use this preserved newMap when computing added/removed
bits and when collecting addedResources.
In `@apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx`:
- Around line 161-170: The current onToggle handler in StatusCard uses
setIsActive and immediately calls toast.success with "Service
activated/deactivated", which misleads users because the change is only local
until persisted; update the handler in ServiceForm's onToggle so it either
removes the toast or changes the message to reflect a pending change (e.g.,
"Status changed — remember to save") and ensure the toast is only shown when
isDefaultService is false and after calling setIsActive (or after a successful
save if you prefer persistence-confirmation feedback); reference: StatusCard,
onToggle, isActive, setIsActive, toast.success, isDefaultService, and the Save
flow.
In `@apps/web/src/components/ui/FormField.tsx`:
- Around line 14-18: The container div in the FormField component is missing the
Tailwind "group" class so the label's group-focus-within:text-white/80 variant
won't work; update the container element (the div created with cn("space-y-1.5",
className) inside FormField.tsx) to include "group" (ensuring any incoming
className is preserved) so the label's group-focus-within styles take effect
when child inputs are focused.
In `@apps/web/src/components/ui/Input.tsx`:
- Around line 6-10: Edit the EditUserModal component to stop passing the removed
label and string error props directly to the Input component: remove any
label="..." and error={errors.field} usages on Input and instead wrap each Input
in the new FormField component (use <FormField label="FieldLabel"
error={errors.field}> ... </FormField>), move the placeholder/value/onChange
props into the Input, and pass a boolean error to Input as
error={!!errors.field}; update all Input instances in EditUserModal.tsx (where
formData and errors are used) to follow this pattern so labels and error
messages are handled by FormField.
In `@apps/web/src/lib/api/schema.d.ts`:
- Around line 1556-1569: The TypeScript schema marks bitmask as optional in the
internal_domain_role.RolePermissionRequest type but the backend defines it as a
required uint64; update the RolePermissionRequest definition so bitmask is
non-optional (change bitmask?: number to bitmask: number) to match the backend
contract and keep resource optional as-is.
In `@apps/web/src/lib/hooks/admin/usePermissions.ts`:
- Around line 36-44: The specific invalidation only includes service_id but
useServicePermissions composes the queryKey with optional params (resource,
limit, offset), so update the invalidation in the mutation success path to
either include those optional params from variables.body (e.g., include
variables.body.resource, limit, offset in the queryKey alongside service_id) so
it matches the exact query keys, or remove the specific-key invalidate and rely
solely on the broader invalidateQueries({ queryKey: ["get",
"/admin/permissions"] }) call; adjust the code around
queryClient.invalidateQueries and the variables.body references accordingly.
♻️ Duplicate comments (5)
apps/web/src/components/dashboard/admin/services/overview/MetadataCard.tsx (1)
34-44: Addtype="button"to prevent accidental form submission.The
aria-labelwas added (addressing the previous review), buttype="button"is still missing. If this component is ever placed inside a<form>, the button would default totype="submit"and could trigger unintended form submission.🔧 Proposed fix
<button + type="button" onClick={() => copyId(serviceId)} className="absolute right-1 top-1 bottom-1 px-2 text-white/40 hover:text-white hover:bg-white/10 rounded transition-colors cursor-pointer" aria-label={copiedId ? "Service ID copied" : "Copy Service ID"} >apps/web/src/components/dashboard/admin/services/overview/SaveActions.tsx (1)
9-14: Type mismatch betweenonSaveprop and actual usage persists.The
onSavehandler is typed asReact.FormEventbut receives aReact.MouseEvent<HTMLButtonElement>from the Button'sonClick. This was flagged in a previous review and should be addressed.Suggested fix
interface SaveActionsProps { hasChanges: boolean; isSaving: boolean; - onSave: (e: React.FormEvent) => void; + onSave: (e: React.MouseEvent<HTMLButtonElement>) => void; disabled?: boolean; }apps/web/src/components/dashboard/admin/services/overview/CredentialsCard.tsx (1)
28-39: Inconsistent copy feedback between Client ID and Client Secret persists.Client Secret copy shows a toast notification (
toast.success("Copied")), but Client ID copy does not. This was flagged in a previous review. Consider adding consistent feedback for both actions.Option: Add toast feedback for Client ID copy
<button - onClick={() => copyId(clientId)} + onClick={() => { + copyId(clientId); + toast.success("Copied"); + }} className="absolute right-1 top-1 bottom-1 px-2 text-white/40 hover:text-white hover:bg-white/10 rounded transition-colors cursor-pointer"Also applies to: 48-63
apps/web/src/components/dashboard/admin/services/overview/AuthenticationSection.tsx (1)
43-69: Use a stable unique key to prevent collisions with duplicate URIs.Using
key={uri}will cause React key collisions if duplicate redirect URIs are added, leading to unexpected behavior in animations and state.🔧 Suggested fix
{redirectURIs.map((uri, idx) => ( <motion.div - key={uri} + key={`${uri}-${idx}`} initial={{ opacity: 0, height: 0 }} animate={{ opacity: 1, height: "auto" }} exit={{ opacity: 0, height: 0 }} className="flex items-center gap-2 group" >apps/api/internal/migrations/000020_create_role_permissions_table.up.sql (1)
1-12: Previous feedback on nullableresourceand uniqueness constraint still applies.The concern about
resourcebeing nullable combined with theUNIQUE(role_id, resource)constraint and soft deletes was already flagged in a prior review. The recommended fix—makingresourceNOT NULL and using a partial unique index on active rows—should be addressed.
🧹 Nitpick comments (8)
apps/api/internal/domain/service/handler.go (1)
68-70: Consider reusing UUID validation for Update/Delete too.Line 68 introduces a 400 for invalid IDs in GetService, but UpdateService/DeleteService still pass any string to the service layer, which may surface inconsistent 404/500 responses. A shared helper keeps behavior consistent across endpoints.
♻️ Possible helper and usage
+func validateServiceID(c *fiber.Ctx) (string, error) { + id := c.Params("id") + if id == "" { + return "", utils.ErrorResponse(c, utils.NewAPIError("VALIDATION_ERROR", "ID is required", fiber.StatusBadRequest)) + } + if _, err := uuid.Parse(id); err != nil { + return "", utils.ErrorResponse(c, utils.NewAPIError("VALIDATION_ERROR", "Invalid ID format", fiber.StatusBadRequest)) + } + return id, nil +}- id := c.Params("id") - if id == "" { - return utils.ErrorResponse(c, utils.NewAPIError("VALIDATION_ERROR", "ID is required", fiber.StatusBadRequest)) - } + id, err := validateServiceID(c) + if err != nil { return err }apps/web/src/components/dashboard/admin/services/overview/MetadataCard.tsx (1)
21-26: Consider consistent date formatting across the application.
toLocaleString()without explicit options produces locale-dependent output that can vary between users and environments. For an admin dashboard, you may want consistent formatting.💡 Optional: Use explicit locale/options for consistent output
- <span className="text-xs text-white/80">{new Date(createdAt).toLocaleString()}</span> + <span className="text-xs text-white/80"> + {new Date(createdAt).toLocaleString("en-US", { + dateStyle: "medium", + timeStyle: "short", + })} + </span>Apply similarly to
updatedAt. Alternatively, create a shared date formatting utility for consistency across the app.apps/web/src/components/dashboard/admin/services/ServiceRow.tsx (1)
98-107: Redundant accessibility attributes.The Button has both
aria-label="Manage service"and a<span className="sr-only">Manage</span>inside. Screen readers will announce both, which is redundant. Keep one or the other.💡 Option 1: Keep aria-label, remove sr-only span
<Button variant="ghost" size="sm" href={`/dashboard/admin/services/${service.id}`} icon={<HugeiconsIcon icon={ViewIcon} size={16} />} className="px-3" aria-label="Manage service" - > - <span className="sr-only">Manage</span> - </Button> + />apps/web/src/components/ui/TagInput.tsx (1)
64-88: Consider using a unique key instead of tag value.Using
key={tag}works because duplicates are prevented, but if the deduplication logic were ever bypassed or refactored, this could cause React key conflicts. A more defensive approach would use an index or generate unique IDs.apps/web/src/components/ui/Input.tsx (1)
40-45: Simplify redundant padding conditions.The padding logic can be simplified. When both
startIconandendIconexist, line 44 applies, but lines 42-43 also have partial conditions that become irrelevant.Suggested simplification
className={cn( inputClasses, - // Padding with icons - startIcon && "pl-9 pr-3", - endIcon && !startIcon && "pl-3 pr-9", - startIcon && endIcon && "pl-9 pr-9", + startIcon && "pl-9", + endIcon && "pr-9", )}apps/web/src/components/dashboard/admin/services/overview/AuthenticationSection.tsx (1)
76-76: Consider using block syntax for clarity.The comma operator works but is unconventional. Using a block makes the intent clearer.
💡 Suggested improvement
- onKeyDown={(e) => e.key === "Enter" && (e.preventDefault(), onAddRedirect())} + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + onAddRedirect(); + } + }}apps/web/src/components/dashboard/admin/services/OverviewTab.tsx (1)
18-30: Consider distinct handling for missing service vs. error.When
!serviceis true butisErroris false (e.g., service doesn't exist or API returned empty data), the toast won't fire but the error UI will show. Consider adding a distinct message or logging for this edge case.💡 Suggested improvement
useEffect(() => { if (isError) { toast.error("Failed to load service. Please try again."); } - }, [isError]); + }, [isError]); + + useEffect(() => { + if (!isLoading && !isError && !service) { + toast.error("Service not found."); + } + }, [isLoading, isError, service]); if (isLoading) return ( <div className="flex justify-center py-20"> <Loader size="lg" /> </div> ); - if (isError || !service) return <div className="text-red-400 text-center py-20">Failed to load service</div>; + if (isError) return <div className="text-red-400 text-center py-20">Failed to load service</div>; + if (!service) return <div className="text-red-400 text-center py-20">Service not found</div>;apps/web/src/app/dashboard/admin/services/[id]/page.tsx (1)
110-111: TODO placeholders are tracked for future implementation.The Permissions and Roles tabs are scaffolded with TODO comments. Consider creating issues to track this work if not already done.
Would you like me to help open issues to track implementing the Permissions and Roles tabs?
| <StatusCard | ||
| isActive={isActive} | ||
| onToggle={() => { | ||
| if (!isDefaultService) { | ||
| setIsActive(!isActive); | ||
| toast.success(`Service ${!isActive ? "activated" : "deactivated"}`); | ||
| } | ||
| }} | ||
| disabled={isDefaultService} | ||
| /> |
There was a problem hiding this comment.
Misleading feedback: toast suggests change is saved when it's only local state.
The toast on line 166 says "Service activated/deactivated" when the user toggles the switch, but the change isn't actually persisted until the user clicks Save. This could mislead users into thinking the change took effect immediately.
Consider either removing this toast or rewording it to indicate the change is pending (e.g., "Status changed – remember to save").
💡 Suggested fix
onToggle={() => {
if (!isDefaultService) {
setIsActive(!isActive);
- toast.success(`Service ${!isActive ? "activated" : "deactivated"}`);
}
}}Or, if you want to keep feedback:
- toast.success(`Service ${!isActive ? "activated" : "deactivated"}`);
+ toast.info(`Status will be ${!isActive ? "active" : "disabled"} after saving`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <StatusCard | |
| isActive={isActive} | |
| onToggle={() => { | |
| if (!isDefaultService) { | |
| setIsActive(!isActive); | |
| toast.success(`Service ${!isActive ? "activated" : "deactivated"}`); | |
| } | |
| }} | |
| disabled={isDefaultService} | |
| /> | |
| <StatusCard | |
| isActive={isActive} | |
| onToggle={() => { | |
| if (!isDefaultService) { | |
| setIsActive(!isActive); | |
| } | |
| }} | |
| disabled={isDefaultService} | |
| /> |
| <StatusCard | |
| isActive={isActive} | |
| onToggle={() => { | |
| if (!isDefaultService) { | |
| setIsActive(!isActive); | |
| toast.success(`Service ${!isActive ? "activated" : "deactivated"}`); | |
| } | |
| }} | |
| disabled={isDefaultService} | |
| /> | |
| <StatusCard | |
| isActive={isActive} | |
| onToggle={() => { | |
| if (!isDefaultService) { | |
| setIsActive(!isActive); | |
| toast.info(`Status will be ${!isActive ? "active" : "disabled"} after saving`); | |
| } | |
| }} | |
| disabled={isDefaultService} | |
| /> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx`
around lines 161 - 170, The current onToggle handler in StatusCard uses
setIsActive and immediately calls toast.success with "Service
activated/deactivated", which misleads users because the change is only local
until persisted; update the handler in ServiceForm's onToggle so it either
removes the toast or changes the message to reflect a pending change (e.g.,
"Status changed — remember to save") and ensure the toast is only shown when
isDefaultService is false and after calling setIsActive (or after a successful
save if you prefer persistence-confirmation feedback); reference: StatusCard,
onToggle, isActive, setIsActive, toast.success, isDefaultService, and the Save
flow.
| <div className={cn("space-y-1.5", className)}> | ||
| {label && ( | ||
| <label className="text-xs font-medium text-white/60 transition-colors group-focus-within:text-white/80"> | ||
| {label} | ||
| </label> |
There was a problem hiding this comment.
The group-focus-within class won't take effect without a parent group class.
The label uses group-focus-within:text-white/80 to change color when a child input is focused, but the container div at line 14 is missing the group class that Tailwind requires for group-based variants to work.
🔧 Proposed fix
- <div className={cn("space-y-1.5", className)}>
+ <div className={cn("group space-y-1.5", className)}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={cn("space-y-1.5", className)}> | |
| {label && ( | |
| <label className="text-xs font-medium text-white/60 transition-colors group-focus-within:text-white/80"> | |
| {label} | |
| </label> | |
| <div className={cn("group space-y-1.5", className)}> | |
| {label && ( | |
| <label className="text-xs font-medium text-white/60 transition-colors group-focus-within:text-white/80"> | |
| {label} | |
| </label> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/ui/FormField.tsx` around lines 14 - 18, The container
div in the FormField component is missing the Tailwind "group" class so the
label's group-focus-within:text-white/80 variant won't work; update the
container element (the div created with cn("space-y-1.5", className) inside
FormField.tsx) to include "group" (ensuring any incoming className is preserved)
so the label's group-focus-within styles take effect when child inputs are
focused.
| queryClient.invalidateQueries({ | ||
| queryKey: [ | ||
| "get", | ||
| "/admin/permissions", | ||
| { params: { query: { service_id: variables.body.service_id } } }, | ||
| ], | ||
| }); | ||
| // Also invalidate without specific params just in case | ||
| queryClient.invalidateQueries({ queryKey: ["get", "/admin/permissions"] }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the usePermissions.ts file to see the complete context
cat -n apps/web/src/lib/hooks/admin/usePermissions.tsRepository: Anvoria/authly
Length of output: 2521
🏁 Script executed:
#!/bin/bash
# Find how $api.useQuery is called with /admin/permissions endpoint
rg -n "useQuery.*permissions|/admin/permissions" apps/web/src/ -t ts -t tsx -B 2 -A 2Repository: Anvoria/authly
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Locate the $api client implementation to understand query key structure
fd -e ts -e tsx "api" apps/web/src/lib | head -20Repository: Anvoria/authly
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Find the $api client implementation
fd -e ts "client" apps/web/src/lib/api/ 2>/dev/null || fd -e ts "client" apps/web/src/ 2>/dev/null | head -20Repository: Anvoria/authly
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Also check the authly lib path mentioned in imports
find . -path "*authly/lib/api*" -name "*.ts" 2>/dev/null | head -20Repository: Anvoria/authly
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for $api definition and useQuery implementation
rg -n "\$api\s*=" apps/web/src/ -A 5 --max-count=10Repository: Anvoria/authly
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Examine the $api client implementation
cat -n apps/web/src/lib/api/client.tsRepository: Anvoria/authly
Length of output: 3204
🏁 Script executed:
#!/bin/bash
# Look for useQuery and useMutation implementations specifically
rg -n "useQuery|useMutation" apps/web/src/lib/api/client.ts -A 15Repository: Anvoria/authly
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for openapi-react-query package to understand how it constructs query keys
rg -n "openapi-react-query" apps/web/ -A 2 -B 2Repository: Anvoria/authly
Length of output: 1757
🏁 Script executed:
#!/bin/bash
# Check package.json for the version of openapi-react-query
cat -n apps/web/package.json | grep -A 2 -B 2 "openapi-react-query"Repository: Anvoria/authly
Length of output: 234
🌐 Web query:
openapi-react-query useQuery query key structure
💡 Result:
In openapi-react-query the query key is an array: [method, path, params]. "method" is the HTTP verb, "path" is the OpenAPI path string, and "params" are the request params (path/query/header/body as applicable). This is documented for useQuery/useInfiniteQuery/queryOptions. [1][2]
Sources
- openapi-react-query useQuery docs. [1]
- openapi-react-query queryOptions / useInfiniteQuery docs. [2]
The specific query invalidation won't match queries with optional parameters.
The useServicePermissions hook spreads optional params (resource, limit, offset) into its query key, but the invalidation at lines 37-41 only includes service_id. This means queries made with those optional parameters won't be invalidated by the specific key—only the broad fallback at line 44 will invalidate them.
Either include the relevant optional params in the specific invalidation, or rely solely on the partial key match at line 44:
queryClient.invalidateQueries({ queryKey: ["get", "/admin/permissions"] });
🤖 Prompt for AI Agents
In `@apps/web/src/lib/hooks/admin/usePermissions.ts` around lines 36 - 44, The
specific invalidation only includes service_id but useServicePermissions
composes the queryKey with optional params (resource, limit, offset), so update
the invalidation in the mutation success path to either include those optional
params from variables.body (e.g., include variables.body.resource, limit, offset
in the queryKey alongside service_id) so it matches the exact query keys, or
remove the specific-key invalidate and rely solely on the broader
invalidateQueries({ queryKey: ["get", "/admin/permissions"] }) call; adjust the
code around queryClient.invalidateQueries and the variables.body references
accordingly.
… rules; refactor components for better performance and consistency
…dy in service endpoints
… for improved error handling in service endpoints
…ePermissionRequest and RolePermissionRequest for better JSON handling
…nsuring correct bitmask mapping from old roles
…tmask in RolePermissionRequest schema
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/src/app/auth/login/page.tsx (1)
54-54: Use uppercase "S256" forcode_challenge_methodper RFC 7636.The PKCE specification (RFC 7636, Section 4.3) defines the code challenge method as
"S256"(uppercase). Many authorization servers are case-sensitive and may reject or mishandle lowercase values.🔧 Proposed fix
- params.set("code_challenge_method", "s256"); + params.set("code_challenge_method", "S256");apps/web/src/app/auth/register/page.tsx (1)
93-152: Fix type mismatch:errorprop expects boolean but receives string from zod validation.The
Inputcomponent'serrorprop is typed asboolean(line 6 of Input.tsx), but the form passes string values from zod:errors.field?.message. This causes a type error and the error styling won't apply correctly.Options to fix:
- Update
Inputcomponent to acceptstring | booleanand display error messages directly, or- Pass boolean:
error={!!errors.field?.message}and display messages separately (e.g., below input)apps/web/src/app/authorize/page.tsx (1)
184-198: Prevent repeated auto-approval after mutation settles.
WhenconfirmMutation.isPendingflips back tofalse(success/error), the effect can fire again while still on the consent screen, triggering a second non‑idempotent confirmation. Add a one‑shot guard (e.g.,useRef) so auto-approve runs only once.✅ Suggested fix
-import { Suspense, useCallback, useEffect, useMemo } from "react"; +import { Suspense, useCallback, useEffect, useMemo, useRef } from "react"; @@ function AuthorizePageContent() { + const autoApproveOnce = useRef(false); @@ useEffect(() => { if ( state.type === "consent" && state.client.client_id === OIDC_CONFIG.client_id && - !confirmMutation.isPending + !confirmMutation.isPending && + !autoApproveOnce.current ) { + autoApproveOnce.current = true; handleApprove(); } }, [ state.type, confirmMutation.isPending, handleApprove, // `@ts-expect-error` - state.client.client_id is safe here because we check state.type === "consent" in the effect state.client.client_id, ]);apps/web/src/app/dashboard/admin/users/page.tsx (1)
16-34: Pagination is effectively locked to page 1.The effect runs on
pagechanges and immediately resets to 1, preventing users from navigating beyond the first page. The dependency array should be[debouncedSearch]instead, as documented in the component's JSDoc—resetting on search changes, not on pagination changes.🛠️ Suggested fix
useEffect(() => { if (page !== 1) { setPage(1); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [page]); + }, [debouncedSearch]);
🤖 Fix all issues with AI agents
In `@apps/api/internal/domain/role/service.go`:
- Around line 321-333: The code only subtracts oldRole.Bitmask when up.Resource
== nil and otherwise only subtracts the matching resource bitmask, so global
RolePermission entries (RolePermission with Resource == nil) are not removed;
update the logic around oldRoleMask (used in the block handling up.Resource ==
nil and the branch that searches oldRole.Permissions) to OR-in (oldRoleMask |=
rp.Bitmask) any rp where rp.Resource == nil (and still OR the matching resource
bitmask when present), ensuring oldRoleMask accumulates global permission bits
from oldRole.Permissions before computing up.Bitmask = up.Bitmask &^
oldRoleMask; reference variables: up, oldRole, oldRoleMask, oldRole.Permissions,
rp.Resource, and rp.Bitmask.
In `@apps/web/biome.json`:
- Around line 128-778: The overrides array contains three near-identical JS/TS
override blocks, three TS globals blocks, three TS linter blocks and two JSON
formatter blocks; deduplicate by keeping a single JS/TS override (the one that
includes "globals" and "linter.rules") and ensure it contains the extra
"noBlankTarget": "off" entry present in the third occurrence, keep a single TS
globals override (empty "javascript.globals": []), keep a single TS linter rules
override (with "complexity.noArguments", "style.useConst", and the listed
"suspicious" and "correctness" settings), and keep one JSON formatter override
(trailingCommas: "none"); remove all duplicated blocks so the overrides array
contains only these unique entries.
- Around line 61-101: The includes array in apps/web/biome.json contains
repeated entries; remove duplicates so the array has a single "**" entry and a
single set of exclusion patterns ("!.next/**", "!out/**", "!build/**",
"!next-env.d.ts"); locate the includes key in the JSON, collapse repeated blocks
into one concise list and ensure ordering remains ["**", "!.next/**", "!out/**",
"!build/**", "!next-env.d.ts"] to preserve intent.
In `@apps/web/package.json`:
- Around line 54-55: Remove the unused ESLint artifacts: delete the "eslint" and
"eslint-config-next" entries from package.json (where they appear in
dependencies/devDependencies) and remove the unused configuration file
eslint.config.mjs; after removal, run your package manager install to update the
lockfile (npm/yarn/pnpm) so the dependency graph is consistent—verify existing
lint scripts (e.g., "lint" and "lint:fix") continue to reference Biome and no
other code references "eslint" or "eslint-config-next".
In `@apps/web/src/components/dashboard/admin/services/CreateServiceModal.tsx`:
- Around line 155-160: The Description field doesn't display or clear schema
validation errors; update the FormField/Input wiring so it reads
errors.description (from the component state) and passes it to the FormField's
error prop (or Input's error-related prop) and modify the onChange handler for
Input to also clear errors.description by calling the error-updating function
(or setFormData/setErrors) when the user types; locate the FormField and Input
usage around the Description block and ensure errors.description is shown and
removed on change.
In
`@apps/web/src/components/dashboard/admin/services/overview/CredentialsCard.tsx`:
- Around line 28-31: The copy buttons in the CredentialsCard component (the
button using onClick={() => copyId(clientId)} with aria-label tied to copiedId,
and the similar secret-copy button around lines 49-55) can accidentally submit a
surrounding form because they lack an explicit type; update both buttons to
include type="button" to prevent default form submission while preserving their
onClick handlers (reference the button elements that call copyId and the
secret-copy counterpart and the copiedId state/prop for locating them).
In `@apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx`:
- Around line 39-53: The hasChanges memo currently compares name to service.name
directly which yields a false positive when service.name is undefined/null;
update the comparison in hasChanges to normalize service.name (e.g., use
(service.name || "") ) before comparing so both sides are strings, and likewise
ensure any other optional fields already compared (description, domain) use the
same normalization pattern; modify the equality check in the useMemo block that
references name !== service.name to use name !== (service.name || "") and keep
the rest of the sorted redirectURIs/allowedScopes logic unchanged.
- Around line 55-75: handleAddRedirect trims newRedirectURI only on storage
which causes valid URLs with surrounding whitespace to be rejected; fix by
trimming once at the start (e.g., const uri = newRedirectURI.trim()) and use
that trimmed value for validation (new URL(uri)), duplicate check
(redirectURIs.includes(uri)), adding (setRedirectURIs([...redirectURIs, uri]))
and clearing (setNewRedirectURI("")) so all logic consistently uses the same
trimmed string; update references in handleAddRedirect accordingly.
In `@apps/web/src/components/dashboard/Sidebar.tsx`:
- Around line 70-72: The effect that calls setIsMobileOpen(false) in useEffect
no longer depends on route changes, so the mobile sidebar won't auto-close after
navigation; update the effect in Sidebar.tsx (the useEffect that references
setIsMobileOpen) to include the current route (e.g., pathname from useLocation)
in its dependency array so it re-runs on navigation, or alternatively update the
component's doc comment to state that the sidebar no longer auto-closes on route
changes if this behavior is intentional.
In `@apps/web/src/lib/api/utils.ts`:
- Around line 29-35: The current return uses errorBody?.details cast to
Record<string,string> without runtime checks; update the normalization so
details is only accepted when it's a plain object (typeof === "object" &&
!Array.isArray && not null) and then build a Record<string,string> by iterating
Object.entries(errorBody.details), skipping non-enumerable keys, and converting
each value to a string (for primitives use String(value); for objects/arrays use
JSON.stringify(value)) so setError always receives string values; encapsulate
this logic in a small helper (e.g., normalizeErrorDetails) or inline it where
the returned object is constructed, referencing errorBody and details to locate
the change.
♻️ Duplicate comments (2)
apps/web/src/components/dashboard/admin/services/overview/SaveActions.tsx (1)
9-13: AlignonSavetype with click handler.
onClicksupplies aMouseEvent, notFormEvent. The current type misleads callers.✅ Suggested fix
interface SaveActionsProps { hasChanges: boolean; isSaving: boolean; - onSave: (e: React.FormEvent) => void; + onSave: (e: React.MouseEvent<HTMLButtonElement>) => void; disabled?: boolean; }apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx (1)
161-167: Toast implies persistence before save.The success toast fires on toggle even though the change is only local until Save. Consider info wording or deferring to save completion.
🧹 Nitpick comments (4)
apps/web/src/app/layout.tsx (1)
51-56: LGTM! Provider hierarchy is correct.The nesting order (
QueryProvider→AuthProvider→ children) ensures that authentication hooks can use react-query internally. PlacingToasterinsideAuthProviderallows toast notifications to be triggered from anywhere in the app.Consider updating the docstring (lines 41-46) to reflect that content is now also wrapped by
AuthProviderand includes a globalToaster.apps/web/src/app/auth/login/page.tsx (1)
72-82: Consider adding PKCE to the fallback authorization flow.The OIDC path with
oidc_paramsgenerates a code verifier/challenge, but the fallback path (lines 72-82) does not use PKCE. For public clients like SPAs, PKCE is recommended as a security best practice to prevent authorization code interception attacks.♻️ Suggested improvement
} else { + const verifier = generateCodeVerifier(); + const challenge = await generateCodeChallenge(verifier); + LocalStorageTokenService.setOidcCodeVerifier(verifier); + const params = new URLSearchParams({ client_id: OIDC_CONFIG.client_id, redirect_uri: OIDC_CONFIG.redirect_uri, response_type: OIDC_CONFIG.response_type, scope: OIDC_CONFIG.scope, + code_challenge: challenge, + code_challenge_method: "S256", });apps/web/biome.json (1)
132-259: Excessive browser globals list.The globals list contains ~125 browser event handlers (
onanimationend,onclick, etc.). In a typical Next.js/React project, these are rarely needed as globals since event handlers are typically attached via React's synthetic event system.Consider whether this extensive list is necessary, or if it was auto-generated and could be trimmed to only the globals actually used (e.g.,
location,exports).apps/web/.husky/pre-commit (1)
1-1: Consider adding error handling for robustness.The hook works but will fail silently if the
apps/webdirectory doesn't exist. Addingset -eor explicit error handling would make failures more visible.💡 Suggested improvement
-cd apps/web && bunx lint-staged +set -e +cd apps/web +bunx lint-stagedOr with an explicit error message:
cd apps/web || { echo "Failed to change to apps/web directory"; exit 1; } bunx lint-staged
| var oldRoleMask uint64 | ||
| if up.Resource == nil { | ||
| oldRoleMask = oldRole.Bitmask | ||
| } else { | ||
| for _, rp := range oldRole.Permissions { | ||
| if rp.Resource != nil && *rp.Resource == *up.Resource { | ||
| oldRoleMask = rp.Bitmask | ||
| break | ||
| } | ||
| } | ||
| } | ||
| up.Bitmask = up.Bitmask &^ oldRoleMask | ||
| up.RoleID = nil |
There was a problem hiding this comment.
Remove global RolePermission bits when clearing old role.
When up.Resource == nil, you only subtract oldRole.Bitmask. If global permissions are stored via RolePermission{Resource:nil}, those bits remain and users can retain old global access after reassignment. Include nil‑resource RolePermission bitmasks in oldRoleMask.
🐛 Suggested fix
- if up.Resource == nil {
- oldRoleMask = oldRole.Bitmask
- } else {
+ if up.Resource == nil {
+ oldRoleMask = oldRole.Bitmask
+ for _, rp := range oldRole.Permissions {
+ if rp.Resource == nil {
+ oldRoleMask |= rp.Bitmask
+ }
+ }
+ } else {🤖 Prompt for AI Agents
In `@apps/api/internal/domain/role/service.go` around lines 321 - 333, The code
only subtracts oldRole.Bitmask when up.Resource == nil and otherwise only
subtracts the matching resource bitmask, so global RolePermission entries
(RolePermission with Resource == nil) are not removed; update the logic around
oldRoleMask (used in the block handling up.Resource == nil and the branch that
searches oldRole.Permissions) to OR-in (oldRoleMask |= rp.Bitmask) any rp where
rp.Resource == nil (and still OR the matching resource bitmask when present),
ensuring oldRoleMask accumulates global permission bits from oldRole.Permissions
before computing up.Bitmask = up.Bitmask &^ oldRoleMask; reference variables:
up, oldRole, oldRoleMask, oldRole.Permissions, rp.Resource, and rp.Bitmask.
apps/web/src/components/dashboard/admin/services/CreateServiceModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/components/dashboard/admin/services/overview/CredentialsCard.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx
Show resolved
Hide resolved
apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…in frontend generation
…eaner configuration
…g useEffect dependencies
…kage.json for hook path
…opyButton component across ServiceRow, CredentialsCard, and MetadataCard
…y for consistent results
…g client ID to a variable and updating useEffect dependencies
…rom package.json and bun.lock for cleaner project setup
…clearing errors on input change
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/web/biome.json`:
- Around line 242-244: The security rule "noBlankTarget" is currently disabled
which allows target="_blank" without rel attributes; re-enable it by changing
its value from "off" to "error" (or "warn") in the security object (the
"noBlankTarget" property) so the linter enforces adding rel="noopener
noreferrer" for links that use target="_blank".
In `@apps/web/src/components/dashboard/admin/services/ServiceRow.tsx`:
- Around line 37-45: The CopyButton in ServiceRow.tsx currently renders even
when service.client_id is falsy, causing empty strings to be copied and success
toasts to show; update the ServiceRow component so the CopyButton (component
name: CopyButton) is only rendered when service.client_id is truthy (e.g.,
conditional render around the CopyButton using service.client_id), and ensure
the value prop passed is the actual ID (not empty string) and ariaLabel remains
appropriate so no copy action or success toast can occur for missing IDs.
In `@apps/web/src/components/ui/CopyButton.tsx`:
- Around line 18-22: handleClick currently fires toast.success and onCopy
synchronously regardless of whether the clipboard write succeeded; change it to
await the copy operation (or use navigator.clipboard.writeText(value) if
appropriate), then call toast.success(`${ariaLabel} copied!`) and onCopy() only
on success, and catch errors to call toast.error with a helpful message (e.g.,
`${ariaLabel} copy failed`) so failures are handled before showing success.
♻️ Duplicate comments (2)
apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx (1)
162-168: Toast implies save happened, but it’s only local state.Line 164-168 still shows “activated/deactivated” even though the change isn’t persisted until Save. Consider removing the toast or rewording to “will be … after saving”.
apps/web/package.json (1)
54-55: ESLint deps may be redundant with Biome-only scripts.
If ESLint is no longer invoked anywhere (scripts, CI, editors), consider removingeslintandeslint-config-nextto reduce the dependency surface. Please verify usage before removal.#!/bin/bash # Verify ESLint usage before removing deps rg -n --hidden -g '!**/node_modules/**' -e '\beslint\b' fd -H -t f -g '.eslintrc*' fd -H -t f -g 'eslint.config.*'
🧹 Nitpick comments (3)
apps/web/src/components/dashboard/Sidebar.tsx (1)
70-73: Remove the unnecessary lint suppression.
The dependency list already includes all referenced values, so thebiome-ignorejust hides future regressions.♻️ Suggested change
- // biome-ignore lint/correctness/useExhaustiveDependencies: we want to close the mobile sidebar when the route changes useEffect(() => { setIsMobileOpen(false); }, [pathname, setIsMobileOpen]);apps/apps/web/.husky/pre-commit (1)
1-8: Potential duplicate/unused hook path.
Givencore.hooksPathis set toapps/web/.husky, this hook underapps/apps/web/.huskylikely won’t execute. Consider removing it or aligning the hooks path to keep a single source of truth.Makefile (1)
27-27: Scope formatting to generated outputs.
Runningbun formathere formats the wholeapps/webtree, which can rewrite unrelated files during codegen. Consider targeting only generated artifacts (e.g.,src/lib/api/**) to keep diffs focused and speed up the target. Please verify the script accepts the scoped args in your Bun/Biome setup.♻️ Suggested adjustment
- `@cd` apps/web && bun format + `@cd` apps/web && bun format -- src/lib/api
apps/web/src/components/dashboard/admin/services/ServiceRow.tsx
Outdated
Show resolved
Hide resolved
| const handleClick = () => { | ||
| copy(value); | ||
| toast.success(`${ariaLabel} copied!`); | ||
| onCopy?.(); | ||
| }; |
There was a problem hiding this comment.
Handle clipboard failures before toasting success.
Line 18-21 always shows success even if the clipboard write fails. Consider awaiting the copy and only firing success (and onCopy) on success, with a fallback error toast.
💡 Suggested fix
- const handleClick = () => {
- copy(value);
- toast.success(`${ariaLabel} copied!`);
- onCopy?.();
- };
+ const handleClick = async () => {
+ try {
+ await copy(value);
+ toast.success(`${ariaLabel} copied!`);
+ onCopy?.();
+ } catch {
+ toast.error(`Failed to copy ${ariaLabel}`);
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleClick = () => { | |
| copy(value); | |
| toast.success(`${ariaLabel} copied!`); | |
| onCopy?.(); | |
| }; | |
| const handleClick = async () => { | |
| try { | |
| await copy(value); | |
| toast.success(`${ariaLabel} copied!`); | |
| onCopy?.(); | |
| } catch { | |
| toast.error(`Failed to copy ${ariaLabel}`); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@apps/web/src/components/ui/CopyButton.tsx` around lines 18 - 22, handleClick
currently fires toast.success and onCopy synchronously regardless of whether the
clipboard write succeeded; change it to await the copy operation (or use
navigator.clipboard.writeText(value) if appropriate), then call
toast.success(`${ariaLabel} copied!`) and onCopy() only on success, and catch
errors to call toast.error with a helpful message (e.g., `${ariaLabel} copy
failed`) so failures are handled before showing success.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @zeedivx. * #49 (comment) The following files were modified: * `apps/api/internal/domain/service/handler.go` * `apps/web/src/app/auth/login/page.tsx` * `apps/web/src/app/authorize/page.tsx` * `apps/web/src/app/dashboard/admin/services/[id]/page.tsx` * `apps/web/src/app/dashboard/admin/users/page.tsx` * `apps/web/src/app/layout.tsx` * `apps/web/src/app/page.tsx` * `apps/web/src/components/dashboard/admin/services/OverviewTab.tsx` * `apps/web/src/components/dashboard/admin/services/overview/AuthenticationSection.tsx` * `apps/web/src/components/dashboard/admin/services/overview/BasicDetailsSection.tsx` * `apps/web/src/components/dashboard/admin/services/overview/CredentialsCard.tsx` * `apps/web/src/components/dashboard/admin/services/overview/MetadataCard.tsx` * `apps/web/src/components/dashboard/admin/services/overview/SaveActions.tsx` * `apps/web/src/components/dashboard/admin/services/overview/ServiceForm.tsx` * `apps/web/src/components/dashboard/admin/services/overview/StatusCard.tsx` * `apps/web/src/components/dashboard/admin/users/EditUserModal.tsx` * `apps/web/src/components/ui/CopyButton.tsx` * `apps/web/src/components/ui/FormField.tsx` * `apps/web/src/components/ui/TagInput.tsx` * `apps/web/src/lib/hooks/admin/usePermissions.ts` * `apps/web/src/lib/hooks/admin/useRoles.ts` * `apps/web/src/proxy.ts`
…rn' for improved security practices
…is available to enhance UI clarity
1 participant
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes / Validation
✏️ Tip: You can customize this high-level summary in your review settings.