Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds MCP settings and permission management: new MCP settings pages and components, permission services/types/fetchers, mapping utilities and a permissions hook, UI permission controls (cards, selectors, switches), RBAC review API, and a Radix-based Switch dependency. Changes
Sequence DiagramssequenceDiagram
participant User
participant MCP_UI as "MCP UI (Pages/Components)"
participant PermissionsHook as "useMcpResourcePermissions"
participant PermissionAPI as "Permission API Client"
participant Backend
User->>MCP_UI: open MCP tab / select resource / toggle override / open selector
MCP_UI->>PermissionsHook: read resources + permissions (init)
PermissionsHook->>PermissionAPI: GET permissions (mcp:run / mcp:use), GET subjects
PermissionAPI->>Backend: HTTP GET /permissions, /subjects
Backend-->>PermissionAPI: permissions[], subjects[]
PermissionAPI-->>PermissionsHook: data
PermissionsHook-->>MCP_UI: permissionsByResource, globalOverrideByResource, preselectedSubjectIds
User->>MCP_UI: toggle global override / apply subject selection
MCP_UI->>PermissionsHook: setGlobalOverride / allowSelectiveAccess
PermissionsHook->>PermissionAPI: POST/PATCH/DELETE /permissions
PermissionAPI->>Backend: HTTP POST/PATCH/DELETE /permissions
Backend-->>PermissionAPI: success
PermissionAPI-->>PermissionsHook: result
PermissionsHook->>MCP_UI: refetch/update state
MCP_UI->>User: updated UI
Possibly related PRs
Suggested Reviewers
🚥 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🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
e9ae835 to
95a60f2
Compare
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
6f4e30c to
dd849e5
Compare
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
dd849e5 to
57196ee
Compare
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
57196ee to
373ef27
Compare
- Fix re-render loop: init selected state only on open transition via prevOpenRef, read preselectedSubjectIds through a stable ref so parent re-renders don't wipe user selections - Fix incomplete onAllow payload: add fetchPermissionSubjectsByIds and seed selectedSubjects upfront so preselected subjects on other pages are always included - Allow applying empty selection (remove last subject) by disabling Apply only when selection is unchanged from initial, not when count is zero - Debounce search input with useDebouncedValue(300ms) to avoid per-keystroke API calls - Remove unnecessary useMemo around selectedCount - Add PAGE_SIZE to query key - Replace Avatar with type-appropriate icon for team/group subjects - Replace typeLabel function with a TYPE_LABELS record map - Wrap subjects derivation in useMemo to fix exhaustive-deps lint warning
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
373ef27 to
1c133c0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/api/services/rbac.ts (2)
96-105: Add a fast-fail guard for empty subjects before API call.At Line 96-Line 105, a quick input guard prevents avoidable network calls with invalid payloads and gives earlier error feedback.
Refactor suggestion
export async function reviewSubjectAccess( payload: SubjectAccessReviewRequest ): Promise<SubjectAccessReviewResponse> { + if (!payload.subjects?.length) { + throw new Error("Subject access review requires at least one subject"); + } + const response = await Rback.post<SubjectAccessReviewResponse>( "/subject-access-reviews", payload );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/rbac.ts` around lines 96 - 105, Add a fast-fail input guard at the start of reviewSubjectAccess to validate the incoming SubjectAccessReviewRequest and return/throw immediately if the subject list is missing or empty (e.g., check payload.subjects or payload.subject depending on the request shape) before calling Rback.post; ensure the guard uses the same error/result type as SubjectAccessReviewResponse or throws a clear error so callers get immediate feedback and no network call is made when subjects are empty.
63-67: Deduplicate the repeated"mcp:run"action literal.Using a shared constant keeps request/response types in sync and avoids drift.
Refactor suggestion
+export const MCP_RUN_ACTION = "mcp:run" as const; + export type SubjectAccessReviewRequest = { resource: SubjectAccessReviewResource; - action: "mcp:run"; + action: typeof MCP_RUN_ACTION; subjects: string[]; }; @@ export type SubjectAccessReviewResponse = { resource: SubjectAccessReviewResource; - action: "mcp:run"; + action: typeof MCP_RUN_ACTION; results: SubjectAccessReviewResult[]; };Also applies to: 90-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/rbac.ts` around lines 63 - 67, Replace repeated "mcp:run" string literals with a single exported constant and use its type in the request/response types: add an exported const ACTION_MCP_RUN = "mcp:run" (as const) and update SubjectAccessReviewRequest.action and the corresponding SubjectAccessReviewResponse.action (and any other occurrences around the SubjectAccessReview* types) to use typeof ACTION_MCP_RUN so they stay in sync and avoid duplication.src/ui/FormControls/Switch.tsx (2)
32-51: Consider movingsizeClassesoutside the component.This constant object is recreated on every render. Moving it outside the component scope would be a minor optimization.
♻️ Optional refactor
+const SIZE_CLASSES: Record< + SwitchSize, + { container: string; item: string; activeItem: string } +> = { + sm: { + container: "rounded-md p-0.5", + item: "px-2 py-1 text-xs", + activeItem: "shadow-sm" + }, + default: { + container: "rounded-lg p-0.5", + item: "p-1.5 lg:pl-2.5 lg:pr-3.5 text-sm", + activeItem: "shadow-sm" + }, + lg: { + container: "rounded-lg p-1", + item: "px-3 py-2 text-sm lg:px-4 lg:py-2.5", + activeItem: "shadow" + } +}; + export function Switch<T extends string | number | boolean>({ // ... }: Props<T>) { // ... - const sizeClasses: Record<...> = { ... }; + // Use SIZE_CLASSES[size] instead of sizeClasses[size]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/FormControls/Switch.tsx` around lines 32 - 51, Move the sizeClasses constant out of the component so it's not recreated on every render: extract the sizeClasses: Record<SwitchSize, { container: string; item: string; activeItem: string }> object (and any dependent types like SwitchSize if needed) from the Switch component file scope into module scope above the component, then update the component to reference the module-level sizeClasses variable. This preserves the same keys/values (sm, default, lg) and keeps the component code unchanged except for using the relocated identifier.
67-69: Potential unnecessary re-renders whenoptionsarray reference changes.Including
fallbackOptionin the dependency array means the effect will re-run wheneveroptions[0]changes. If the parent component recreates theoptionsarray on each render (even with the same values), this effect will trigger unnecessarily.Consider memoizing the fallback or comparing by value:
♻️ Optional fix using stable comparison
+ const fallbackOptionValue = options[0]; + useEffect(() => { - setActiveOption(value ?? fallbackOption); - }, [fallbackOption, value]); + setActiveOption(value ?? fallbackOptionValue); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [value, JSON.stringify(fallbackOptionValue)]);Alternatively, ensure parent components memoize their
optionsarrays.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/FormControls/Switch.tsx` around lines 67 - 69, The effect in useEffect that calls setActiveOption(value ?? fallbackOption) causes unnecessary re-renders when the parent recreates the options array because fallbackOption is included in the dependency list; update the component to derive a stable fallback and avoid using the raw fallbackOption reference in the deps: compute a memoized fallback (e.g., via useMemo from options[0] or a shallow/value comparison) and then use [value, memoizedFallback] as the dependency array for useEffect (or only run the effect when value changes and set fallback once on mount), referencing the useEffect, setActiveOption, value and fallbackOption identifiers to locate the code to change.src/pages/Settings/mcp/McpViewsPage.tsx (1)
20-23: Consider pagination for large view counts.The query fetches up to 1000 views with a hardcoded limit. While this is likely sufficient for most deployments, it could become a limitation if the view count grows significantly. Consider whether pagination or a higher limit might be needed in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Settings/mcp/McpViewsPage.tsx` around lines 20 - 23, The useQuery call fetching views currently hardcodes a 1000-item limit via getAllViews([{ id: "name", desc: false }], 0, 1000), which risks missing data at scale; update this to support proper pagination or an adjustable limit by replacing the single-shot getAllViews call with a paginated approach (e.g., useInfiniteQuery or passing page/limit from component state/props) and wire the component to request subsequent pages (or expose a configurable max limit) so functions like useQuery/queryKey and getAllViews are updated to accept and handle page and size parameters instead of the fixed 1000.src/api/services/permissions.ts (1)
21-27: Reuse the sharedsubject_typeunion here.The new MCP flow now creates permission rows with
subject_type = "group", butFetchPermissionsInput.subject_typestill can't express that value. Reusing the backend type avoids another drift point.♻️ Suggested cleanup
- subject_type?: - | "playbook" - | "team" - | "person" - | "notification" - | "component" - | "role"; + subject_type?: NonNullable<PermissionTable["subject_type"]>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/permissions.ts` around lines 21 - 27, Replace the inline literal union for FetchPermissionsInput.subject_type with the shared backend subject_type union so the type includes "group" and stays in sync; locate the FetchPermissionsInput type (symbol: FetchPermissionsInput) in this module and import the shared SubjectType (or whatever the exported name is) from the central types file, then set subject_type?: SubjectType (or the exact exported identifier) instead of the explicit string union.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/services/permissions.ts`:
- Around line 162-175: The helpers fetchMcpRunPermissions and
fetchMcpUserPermissions currently hard-code limit=5000 which truncates results;
replace the single GET with a paging loop that requests pages (e.g.
pageSize=5000) using offset (or whichever PostgREST pagination param your
backend supports), call IncidentCommander.get repeatedly with the same query
plus &limit={pageSize}&offset={offset}, accumulate response.data (guarding for
null) into a results array, increment offset by pageSize and stop when the
returned page length is less than pageSize (or no rows); apply the same change
to both fetchMcpRunPermissions and fetchMcpUserPermissions and return the full
accumulated array, keeping MCP_SETTINGS_PERMISSION_SOURCE and the original query
filters intact.
In `@src/components/Permissions/PermissionAccessCard.tsx`:
- Around line 25-31: The card rendered in PermissionAccessCard is clickable only
via mouse when globalOverride === "none" and onViewSubjects exists; make it
keyboard-accessible by adding tabIndex={0}, role="button", and a keyDown handler
that calls onViewSubjects when Enter or Space is pressed, and ensure the
existing onClick still delegates to onViewSubjects; update the element rendering
(the div with className using globalOverride and onViewSubjects) to
conditionally include these attributes and the key handler so screen-reader and
keyboard users can activate it.
In `@src/components/Permissions/SubjectSelectorPanel.tsx`:
- Around line 146-149: displayedSubjects currently renders only the paginated
search results, so preloaded selections returned by fetchPermissionSubjectsByIds
never appear; update the displayedSubjects computation in SubjectSelectorPanel
to merge the preloaded selected subjects into the rendered list (or render a
separate “Selected” section) by taking the union of (data?.data ?? []) and the
subjects returned by fetchPermissionSubjectsByIds, deduplicating by
PermissionSubject id, and ensuring selected items are visible (e.g., prepend
selected subjects or create a Selected subsection) while keeping existing
pagination/search behavior intact; reference displayedSubjects,
fetchPermissionSubjectsByIds, PermissionSubject and the SubjectSelectorPanel
component when making the change.
- Around line 110-121: The current useEffect unconditionally merges
subjectsByIds into selectedSubjects, which can resurrect user-removed entries;
instead make selectedIds the source of truth: update the effect that currently
spreads subjectsByIds into selectedSubjects (and the similar logic at 322-325)
to only add subjects whose id exists in selectedIds and remove any
selectedSubjects whose id is not in selectedIds, derive selectedSubjects from
selectedIds+subjectsByIds (or keep a memoized map) rather than merging blindly,
and change the Apply button/enabled logic to be disabled until every id in
selectedIds has a hydrated PermissionSubject in subjectsByIds/selectedSubjects
so late preload responses cannot resurrect or submit incomplete selections.
- Around line 272-277: The Switch in SubjectSelectorPanel is missing an
accessible name; update the Switch (the one using
checked={!!selectedIds[subject.id]} and onCheckedChange={checked =>
toggleSubject(subject, checked)}) to include an aria-label or aria-labelledby
that wires the visible subject name into the control (e.g.
aria-label={subject.name} or create a <span
id={`subject-label-${subject.id}`}>{subject.name}</span> and set aria-labelledby
to that id); ensure the label text uniquely identifies the subject and keep the
existing checked/onCheckedChange behavior unchanged.
In `@src/components/Permissions/UserAccessCard.tsx`:
- Around line 31-35: The icon condition in UserAccessCard.tsx currently checks
user.type only for "team" and "permission_subject_group" and shows HiUserGroup;
update the condition that computes which icon to render (the ternary using
user.type) to also treat "role" as a group-type so roles render HiUserGroup
instead of HiUser; locate the ternary that references HiUserGroup and HiUser and
add "role" alongside "team" and "permission_subject_group" in that check
(aligning with the grouping logic in mcpPermissionCardMappings).
In `@src/lib/permissions/mcpPermissionCardMappings.ts`:
- Around line 160-166: The loop that populates globalOverrideByResource from
matchedResources can produce nondeterministic results when both allow and deny
rows exist; update the logic in the isGlobalOverride branch so you compute the
candidate value from permission.deny and then merge it deterministically with
any existing entry for the same resource id (use the resource id key lookups on
globalOverrideByResource). Implement a stable tie-breaker such that deny wins:
if an existing value is "deny" or the new candidate is deny, store "deny";
otherwise store "allow". Ensure this uses the symbols globalOverrideByResource,
matchedResources, resource.id, and permission.deny to locate and update the
code.
In `@src/lib/permissions/useMcpResourcePermissions.ts`:
- Around line 241-244: The current Promise.all combining addPermission(payload)
and deletePermission(id) calls can leave permissions partially applied if any
call fails; update useMcpResourcePermissions to perform an atomic update: either
call a new transactional bulk endpoint (preferred) like
bulkUpdatePermissions(payloads, deleteIds) from the same module/service, or if
backend change isn't available, sequence operations in a deterministic order and
implement a compensating rollback on failure (e.g., if adds succeed but a delete
fails, call delete for the added IDs), and ensure the mutation's onSettled
always refetches the resource permissions to reconcile state; locate usages of
addPermission, deletePermission, payloads, deleteIds and update the mutation
handler and its onSettled to perform the transactional or sequenced+compensating
flow.
In `@src/pages/Settings/mcp/McpOverviewPage.tsx`:
- Around line 160-172: The code uses a non-null assertion created_by: user?.id!
when calling addPermission, which can pass undefined if user is absent; update
the MCP permission creation path (inside the if (!primaryPermission) block where
addPermission is called) to explicitly guard for missing user by validating user
and user.id before calling addPermission (e.g., return or throw an error / skip
the operation if user is undefined) or derive a safe fallback id, ensuring
created_by is always a defined value when invoking addPermission with
MCP_OBJECT, MCP_ACTION and MCP_SETTINGS_PERMISSION_SOURCE; reference the user
variable, addPermission call, and created_by field to locate and fix the code.
---
Nitpick comments:
In `@src/api/services/permissions.ts`:
- Around line 21-27: Replace the inline literal union for
FetchPermissionsInput.subject_type with the shared backend subject_type union so
the type includes "group" and stays in sync; locate the FetchPermissionsInput
type (symbol: FetchPermissionsInput) in this module and import the shared
SubjectType (or whatever the exported name is) from the central types file, then
set subject_type?: SubjectType (or the exact exported identifier) instead of the
explicit string union.
In `@src/api/services/rbac.ts`:
- Around line 96-105: Add a fast-fail input guard at the start of
reviewSubjectAccess to validate the incoming SubjectAccessReviewRequest and
return/throw immediately if the subject list is missing or empty (e.g., check
payload.subjects or payload.subject depending on the request shape) before
calling Rback.post; ensure the guard uses the same error/result type as
SubjectAccessReviewResponse or throws a clear error so callers get immediate
feedback and no network call is made when subjects are empty.
- Around line 63-67: Replace repeated "mcp:run" string literals with a single
exported constant and use its type in the request/response types: add an
exported const ACTION_MCP_RUN = "mcp:run" (as const) and update
SubjectAccessReviewRequest.action and the corresponding
SubjectAccessReviewResponse.action (and any other occurrences around the
SubjectAccessReview* types) to use typeof ACTION_MCP_RUN so they stay in sync
and avoid duplication.
In `@src/pages/Settings/mcp/McpViewsPage.tsx`:
- Around line 20-23: The useQuery call fetching views currently hardcodes a
1000-item limit via getAllViews([{ id: "name", desc: false }], 0, 1000), which
risks missing data at scale; update this to support proper pagination or an
adjustable limit by replacing the single-shot getAllViews call with a paginated
approach (e.g., useInfiniteQuery or passing page/limit from component
state/props) and wire the component to request subsequent pages (or expose a
configurable max limit) so functions like useQuery/queryKey and getAllViews are
updated to accept and handle page and size parameters instead of the fixed 1000.
In `@src/ui/FormControls/Switch.tsx`:
- Around line 32-51: Move the sizeClasses constant out of the component so it's
not recreated on every render: extract the sizeClasses: Record<SwitchSize, {
container: string; item: string; activeItem: string }> object (and any dependent
types like SwitchSize if needed) from the Switch component file scope into
module scope above the component, then update the component to reference the
module-level sizeClasses variable. This preserves the same keys/values (sm,
default, lg) and keeps the component code unchanged except for using the
relocated identifier.
- Around line 67-69: The effect in useEffect that calls setActiveOption(value ??
fallbackOption) causes unnecessary re-renders when the parent recreates the
options array because fallbackOption is included in the dependency list; update
the component to derive a stable fallback and avoid using the raw fallbackOption
reference in the deps: compute a memoized fallback (e.g., via useMemo from
options[0] or a shallow/value comparison) and then use [value, memoizedFallback]
as the dependency array for useEffect (or only run the effect when value changes
and set fallback once on mount), referencing the useEffect, setActiveOption,
value and fallbackOption identifiers to locate the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65e29788-e355-4a75-a8ac-12a58357308c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
package.jsonsrc/App.tsxsrc/api/services/permissions.tssrc/api/services/playbooks.tssrc/api/services/rbac.tssrc/api/types/permissions.tssrc/components/Configs/Sidebar/__tests__/ConfigDetails.unit.test.tsxsrc/components/Connections/__tests__/ConnectionsList.unit.test.tsxsrc/components/MCP/McpTabsLinks.tsxsrc/components/Permissions/PermissionAccessCard.tsxsrc/components/Permissions/SubjectSelectorPanel.tsxsrc/components/Permissions/UserAccessCard.tsxsrc/components/Tokens/Add/CreateTokenForm.tsxsrc/components/Tokens/Add/TokenScopeFieldsGroup.tsxsrc/components/Users/SetupMcpModal.tsxsrc/components/ui/switch.tsxsrc/lib/permissions/mcpPermissionCardMappings.tssrc/lib/permissions/useMcpResourcePermissions.tssrc/pages/Settings/mcp/McpOverviewPage.tsxsrc/pages/Settings/mcp/McpPlaybooksPage.tsxsrc/pages/Settings/mcp/McpViewsPage.tsxsrc/services/permissions/features.tssrc/ui/FormControls/Switch.tsxsrc/ui/Modal/index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/lib/permissions/useMcpResourcePermissions.ts (1)
247-267:⚠️ Potential issue | 🔴 CriticalSelective-access updates are still non-atomic.
The rollback only deletes
addedPermissionIds. If anydeletePermission(id)succeeds before a later delete fails, those removals are permanent and the permission set is left half-applied. This still needs a transactional bulk endpoint, or a full compensating rollback that recreates deleted rows fromlatestPermissions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/permissions/useMcpResourcePermissions.ts` around lines 247 - 267, The current try/catch only rolls back newly added permissions (addedPermissionIds) but does not restore any permissions that were deleted before a failure, leaving partial state; update the error handler in useMcpResourcePermissions to perform a full compensating rollback: on error, 1) delete any successfully added permissions (addedPermissionIds) as currently done, and 2) recreate any permissions that were removed by the earlier deletePermission(id) calls by using the pre-change snapshot (latestPermissions) to reconstruct and call addPermission for each deleted permission that no longer exists; alternatively, if available, replace the sequence with a single transactional bulk endpoint call to apply payloadsToAdd and deleteIds atomically so neither addedPermissionIds nor deleted rows can be left half-applied.src/components/Permissions/SubjectSelectorPanel.tsx (1)
272-283:⚠️ Potential issue | 🟠 MajorGive each switch an accessible name.
The visible
subject.nameis not programmatically associated with theSwitch, so assistive tech will announce a list of unnamed toggles. Addaria-labeloraria-labelledby.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Permissions/SubjectSelectorPanel.tsx` around lines 272 - 283, The Switch controls in SubjectSelectorPanel.tsx lack an accessible name; update the Switch (the one using checked={!!selectedIds[subject.id]} and onCheckedChange={(checked) => toggleSubject(subject, checked)}) to include a programmatic label by either adding aria-label={`Toggle ${subject.name}`} or aria-labelledby that points to the visible name element (e.g., give the div that renders {subject.name} a stable id like subject-label-{subject.id} and set aria-labelledby on the Switch). Ensure the id is unique per subject.id so assistive tech correctly announces each toggle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Permissions/SubjectSelectorPanel.tsx`:
- Around line 164-186: selectedHydratedSubjects and displayedSubjects drop
unresolved IDs from selectedIds so stale IDs (e.g., deleted users/teams) remain
selected but invisible, blocking the Apply button; modify SubjectSelectorPanel
to surface unresolved selections and allow removal or include raw selectedIds
when calling onAllow so missing subjects can be cleared. Specifically, update
logic around selectedHydratedSubjects/displayedSubjects (and where Apply
enablement is computed) to either append unresolved IDs as placeholder
PermissionSubject entries (rendered in a removable "missing" section) or change
the onAllow signature/usage to accept selectedIds directly so callers
(useMcpResourcePermissions) can clear stale IDs; ensure UI exposes a remove
action for those placeholder entries and that Apply respects removal to
re-enable changes.
In `@src/components/Permissions/UserAccessCard.tsx`:
- Around line 42-47: The display of the user's name in UserAccessCard can be
blank when user.name is missing; update the JSX that renders and sets title for
the identity label to use a visible fallback (use user.name || user.id) so the
title and visible label never empty, and apply the same fallback wherever
user.name is rendered (including the other instance referenced around the
{user.name} at Line 84) to keep behavior consistent.
- Around line 59-61: The isMutating flag only set visual/ARIA blocking but
doesn’t stop keyboard events; update the interactive controls in UserAccessCard
(the toggle/checkbox/button that calls onChangeAccess) to be fully disabled when
isMutating by (1) adding the appropriate disabled prop to the input/button
elements and (2) adding an early guard inside the onChangeAccess handler to
return immediately if isMutating is true. Apply the same disabled prop +
handler-guard pattern to the other interactive block referenced (lines 73–81) so
keyboard and programmatic events are prevented while mutating.
In `@src/lib/permissions/mcpPermissionCardMappings.ts`:
- Around line 172-178: The current everyone-override branch only filters by
subject_type/subject and ignores action/source, so unrelated permission rows
leak into permissionsByResource; update the logic in the grouping helper (the
block referencing permission.deny, permission.subject_type, everyoneSubjectType,
everyoneSubjectId and pushing into permissionsByResource) to also verify that
permission.action and permission.source match the requested action/source slice
before treating it as the everyone override or adding it to
permissionsByResource, and ensure any grouping keyed by subject respects the
requested action/source so only relevant permission rows are included.
In `@src/lib/permissions/useMcpResourcePermissions.ts`:
- Around line 133-176: The mutation only calls refetchPermissions in onSuccess
so partial writes leave stale state if a later operation fails; update the
mutation handlers to always refresh after the operation by calling
refetchPermissions from onError as well or move the refresh into onSettled, and
keep the existing toastError(error) call in onError (referencing
refetchPermissions, onError, onSuccess, onSettled, and toastError).
---
Duplicate comments:
In `@src/components/Permissions/SubjectSelectorPanel.tsx`:
- Around line 272-283: The Switch controls in SubjectSelectorPanel.tsx lack an
accessible name; update the Switch (the one using
checked={!!selectedIds[subject.id]} and onCheckedChange={(checked) =>
toggleSubject(subject, checked)}) to include a programmatic label by either
adding aria-label={`Toggle ${subject.name}`} or aria-labelledby that points to
the visible name element (e.g., give the div that renders {subject.name} a
stable id like subject-label-{subject.id} and set aria-labelledby on the
Switch). Ensure the id is unique per subject.id so assistive tech correctly
announces each toggle.
In `@src/lib/permissions/useMcpResourcePermissions.ts`:
- Around line 247-267: The current try/catch only rolls back newly added
permissions (addedPermissionIds) but does not restore any permissions that were
deleted before a failure, leaving partial state; update the error handler in
useMcpResourcePermissions to perform a full compensating rollback: on error, 1)
delete any successfully added permissions (addedPermissionIds) as currently
done, and 2) recreate any permissions that were removed by the earlier
deletePermission(id) calls by using the pre-change snapshot (latestPermissions)
to reconstruct and call addPermission for each deleted permission that no longer
exists; alternatively, if available, replace the sequence with a single
transactional bulk endpoint call to apply payloadsToAdd and deleteIds atomically
so neither addedPermissionIds nor deleted rows can be left half-applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f15ad7a-9af3-4e12-9f5e-441b368be8ef
📒 Files selected for processing (5)
src/components/Permissions/SubjectSelectorPanel.tsxsrc/components/Permissions/UserAccessCard.tsxsrc/lib/permissions/mcpPermissionCardMappings.tssrc/lib/permissions/useMcpResourcePermissions.tssrc/pages/Settings/mcp/McpOverviewPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Settings/mcp/McpOverviewPage.tsx
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
3a65bca to
b612f75
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/Permissions/SubjectSelectorPanel.tsx (1)
320-325:⚠️ Potential issue | 🟠 MajorGive each subject switch an accessible name.
These row switches still have no programmatic label, so a screen reader will announce an unnamed control for every subject.
♿ Minimal fix
<Switch + aria-label={`Allow ${subject.name}`} checked={!!selectedIds[subject.id]} onCheckedChange={(checked) => toggleSubject(subject, checked) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Permissions/SubjectSelectorPanel.tsx` around lines 320 - 325, The Switch for each subject in SubjectSelectorPanel is missing an accessible name; update the Switch (the JSX using checked={!!selectedIds[subject.id]} and onCheckedChange={checked => toggleSubject(subject, checked)}) to include a programmatic label — e.g. add aria-label or aria-labelledby that uses a stable subject identifier like subject.name or subject.id (for example "Select subject {subject.name}" or reference the row's visible label element) so screen readers announce each control meaningfully.src/lib/permissions/useMcpResourcePermissions.ts (1)
249-267:⚠️ Potential issue | 🔴 CriticalDelete failures still leave the permission set partially rewritten.
The add-first sequencing helps, but once the delete loop starts, a later
deletePermissionfailure leaves earlier deletes committed and the catch block only removes newly added rows. The refetch inonSettledwill reconcile the UI, but it cannot undo that partial write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/permissions/useMcpResourcePermissions.ts` around lines 249 - 267, The current sequence in the try/catch (looping payloadsToAdd with addPermission then looping deleteIds with deletePermission) can leave the permission set partially modified if a deletePermission call fails; change the approach to make the operation atomic by performing deletions before additions or, preferably, delegate both deletes and adds to a single backend transactional endpoint; update the logic around payloadsToAdd, addPermission, deleteIds, deletePermission and addedPermissionIds so that either (a) run all deletePermission calls first (and abort before any addPermission runs if any delete fails) or (b) call a new combined API that applies deletes+adds inside a DB transaction and return success/failure to avoid partial commits.
🧹 Nitpick comments (3)
src/pages/Settings/mcp/McpOverviewPage.tsx (1)
174-177: Type assertionas anyfor updatePermission.Similar to the
addPermissioncall, this usesas anyto bypass type checking. Consider creating a properUpdatePermissionInputtype that reflects what the PATCH endpoint actually requires (typically justidand the fields being updated).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Settings/mcp/McpOverviewPage.tsx` around lines 174 - 177, The call to updatePermission is using a blanket type assertion (`as any`) which bypasses type safety; replace this with a proper input type (e.g., UpdatePermissionInput) that models the PATCH payload (at minimum { id: string; deny?: boolean }) and use it in the updatePermission call instead of `as any`; update the updatePermission function signature (or the wrapper that calls the PATCH endpoint) to accept UpdatePermissionInput and pass { id: primaryPermission.id, deny: targetDeny } typed as UpdatePermissionInput, mirroring how addPermission was handled.src/components/MCP/UserList.tsx (1)
8-9: Consider importingMCP_OBJECTandMCP_ACTIONfrom a shared location.These constants are duplicated in
McpOverviewPage.tsx(lines 20-21). If they need to stay in sync, consider extracting them to a shared constants file or importing from the permissions API module to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MCP/UserList.tsx` around lines 8 - 9, MCP_OBJECT and MCP_ACTION are duplicated and risk drifting; extract them into a single shared constant (e.g., export const MCP_OBJECT / MCP_ACTION) in a central module (or reuse the permissions API module) and replace the local declarations in UserList.tsx and McpOverviewPage.tsx with imports of those exported symbols so both files reference the same source of truth.src/components/Permissions/SubjectAccessCard.tsx (1)
77-94: The Switch component doesn't have adisabledprop, so the current accessibility pattern cannot be improved without modifying the Switch component itself.The Switch component from
@flanksource-ui/ui/FormControls/Switchdoes not include adisabledprop in its type definition. To properly disable the switch for keyboard users, the Switch component would need to be modified to:
- Accept and handle a
disabledprop- Prevent focus and keyboard interactions when disabled
The current pattern of using
aria-disabledon the wrapper div withpointer-events-noneandopacity-60is a workaround that handles mouse users but doesn't prevent keyboard access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Permissions/SubjectAccessCard.tsx` around lines 77 - 94, SubjectAccessCard's current pattern uses a wrapper div to mimic disabling but the Switch component (from `@flanksource-ui/ui/FormControls/Switch`) lacks a disabled prop and still receives keyboard focus; update the Switch component to accept a disabled:boolean prop, ignore/return early from its onChange when disabled, and ensure it prevents focus/keyboard interactions (e.g., set tabIndex to -1 and prevent key handlers) when disabled; then in SubjectAccessCard pass disabled={isMutating} to the Switch (and keep aria-disabled on the wrapper for semantics) so onChangeAccess and OPTION_TO_ACCESS mapping won't be invoked while isMutating is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Permissions/ResourceAccessCard.tsx`:
- Around line 64-67: The container currently using canOpenSubjects and
onClick={handleCardClick} should be made keyboard-operable: when canOpenSubjects
is true add role="button" and tabIndex={0} to the div and implement an onKeyDown
handler that triggers handleCardClick for Enter and Space key presses (mirroring
ViewSection.tsx), or replace the div with the Button component from
src/components/ui/button.tsx and wire handleCardClick to its onClick to get
keyboard/accessibility for free; ensure checks still respect canOpenSubjects
before invoking handleCardClick.
In `@src/lib/permissions/useMcpResourcePermissions.ts`:
- Around line 91-106: The preselectedSubjectIds filter in
useMcpResourcePermissions is incorrectly treating the synthetic EVERYONE
override as a normal selectable subject; update the permission filters (e.g.,
the predicate used in preselectedSubjectIds and the similar filter at the other
location around lines 197-209) to explicitly exclude the global override by
checking that p.subject !== EVERYONE_SUBJECT_ID or p.subject_type !==
EVERYONE_SUBJECT_TYPE (or both) so rows where subject equals EVERYONE_SUBJECT_ID
and subject_type equals EVERYONE_SUBJECT_TYPE are ignored, keeping
buildPermissionAccessCardMaps behavior intact and preventing the synthetic
"everyone" row from being hydrated as a deletable selectable subject.
In `@src/pages/Settings/mcp/McpOverviewPage.tsx`:
- Around line 158-166: Replace the unsafe "as any" cast by defining a proper
CreatePermissionInput type (e.g., Omit<PermissionTable, 'id' | 'created_at' |
'updated_at'> & { created_by: string }) and update the addPermission function
signature to accept that type; then construct the payload currently passed to
addPermission (using MCP_OBJECT, MCP_ACTION, subjectId,
mapPermissionSubjectType(subjectType), targetDeny,
MCP_SETTINGS_PERMISSION_SOURCE, user.id) as a CreatePermissionInput and remove
the cast so TypeScript enforces required/optional fields and prevents missing
properties like id/timestamps from being ignored.
---
Duplicate comments:
In `@src/components/Permissions/SubjectSelectorPanel.tsx`:
- Around line 320-325: The Switch for each subject in SubjectSelectorPanel is
missing an accessible name; update the Switch (the JSX using
checked={!!selectedIds[subject.id]} and onCheckedChange={checked =>
toggleSubject(subject, checked)}) to include a programmatic label — e.g. add
aria-label or aria-labelledby that uses a stable subject identifier like
subject.name or subject.id (for example "Select subject {subject.name}" or
reference the row's visible label element) so screen readers announce each
control meaningfully.
In `@src/lib/permissions/useMcpResourcePermissions.ts`:
- Around line 249-267: The current sequence in the try/catch (looping
payloadsToAdd with addPermission then looping deleteIds with deletePermission)
can leave the permission set partially modified if a deletePermission call
fails; change the approach to make the operation atomic by performing deletions
before additions or, preferably, delegate both deletes and adds to a single
backend transactional endpoint; update the logic around payloadsToAdd,
addPermission, deleteIds, deletePermission and addedPermissionIds so that either
(a) run all deletePermission calls first (and abort before any addPermission
runs if any delete fails) or (b) call a new combined API that applies
deletes+adds inside a DB transaction and return success/failure to avoid partial
commits.
---
Nitpick comments:
In `@src/components/MCP/UserList.tsx`:
- Around line 8-9: MCP_OBJECT and MCP_ACTION are duplicated and risk drifting;
extract them into a single shared constant (e.g., export const MCP_OBJECT /
MCP_ACTION) in a central module (or reuse the permissions API module) and
replace the local declarations in UserList.tsx and McpOverviewPage.tsx with
imports of those exported symbols so both files reference the same source of
truth.
In `@src/components/Permissions/SubjectAccessCard.tsx`:
- Around line 77-94: SubjectAccessCard's current pattern uses a wrapper div to
mimic disabling but the Switch component (from
`@flanksource-ui/ui/FormControls/Switch`) lacks a disabled prop and still receives
keyboard focus; update the Switch component to accept a disabled:boolean prop,
ignore/return early from its onChange when disabled, and ensure it prevents
focus/keyboard interactions (e.g., set tabIndex to -1 and prevent key handlers)
when disabled; then in SubjectAccessCard pass disabled={isMutating} to the
Switch (and keep aria-disabled on the wrapper for semantics) so onChangeAccess
and OPTION_TO_ACCESS mapping won't be invoked while isMutating is true.
In `@src/pages/Settings/mcp/McpOverviewPage.tsx`:
- Around line 174-177: The call to updatePermission is using a blanket type
assertion (`as any`) which bypasses type safety; replace this with a proper
input type (e.g., UpdatePermissionInput) that models the PATCH payload (at
minimum { id: string; deny?: boolean }) and use it in the updatePermission call
instead of `as any`; update the updatePermission function signature (or the
wrapper that calls the PATCH endpoint) to accept UpdatePermissionInput and pass
{ id: primaryPermission.id, deny: targetDeny } typed as UpdatePermissionInput,
mirroring how addPermission was handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53ce4231-1aba-4acd-b818-72ffddfd907b
📒 Files selected for processing (9)
src/components/MCP/UserList.tsxsrc/components/Permissions/ResourceAccessCard.tsxsrc/components/Permissions/SubjectAccessCard.tsxsrc/components/Permissions/SubjectSelectorPanel.tsxsrc/components/Tokens/Add/CreateTokenForm.tsxsrc/lib/permissions/useMcpResourcePermissions.tssrc/pages/Settings/mcp/McpOverviewPage.tsxsrc/pages/Settings/mcp/McpPlaybooksPage.tsxsrc/pages/Settings/mcp/McpViewsPage.tsx
✅ Files skipped from review due to trivial changes (1)
- src/pages/Settings/mcp/McpViewsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Tokens/Add/CreateTokenForm.tsx
adityathebe
force-pushed
the
feat/permissions-bulk-selection-ui
branch
from
b612f75 to
e0ef0f4
Compare
Open
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Settings/mcp/McpOverviewPage.tsx`:
- Around line 208-215: The icon-only button that opens the MCP setup modal (the
button with key "setup-mcp" that calls setIsSetupMcpModalOpen(true) and renders
AiFillPlusCircle) lacks an explicit accessible name; add an aria-label (e.g.,
aria-label="Setup MCP") to the button element so screen readers announce the
action consistently while keeping the existing title prop and onClick handler
unchanged.
- Around line 138-188: The mutation currently only calls refetchPermissions() in
onSuccess so partial failures (Promise.all rejecting) leave the UI stale; update
the mutation callbacks to call refetchPermissions() in onSettled (or both
onSuccess and onError) so it always runs after the Promise.all of
deletePermission/updatePermission completes or fails—look for the mutation
definition using onSuccess/onError here (and the Promise.all logic with
primaryPermission, duplicatePermissionIds, deletePermission, updatePermission)
and move or add the refetchPermissions() call into an onSettled handler to
guarantee a refresh regardless of partial failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 969b4d8a-54bc-41ba-810c-5d5f2c58f6db
📒 Files selected for processing (7)
src/components/MCP/UserList.tsxsrc/components/Permissions/ResourceAccessCard.tsxsrc/components/Permissions/SubjectAccessCard.tsxsrc/components/Tokens/Add/CreateTokenForm.tsxsrc/pages/Settings/mcp/McpOverviewPage.tsxsrc/pages/Settings/mcp/McpPlaybooksPage.tsxsrc/pages/Settings/mcp/McpViewsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/components/Tokens/Add/CreateTokenForm.tsx
- src/components/Permissions/SubjectAccessCard.tsx
- src/pages/Settings/mcp/McpPlaybooksPage.tsx
- src/pages/Settings/mcp/McpViewsPage.tsx
- src/components/MCP/UserList.tsx
- src/components/Permissions/ResourceAccessCard.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Permissions/PermissionAccessCheckModal.tsx`:
- Around line 152-183: The async checkAccess call in
PermissionAccessCheckModal.tsx can update state after the modal closes; guard
against stale updates by tracking cancellation and only calling setResult /
setRequestError / helpers.setSubmitting when the modal is still open. Wrap the
try/catch/finally body with a local flag (e.g., let cancelled = false) or an
AbortController tied to the modal's close handler, set cancelled = true when the
modal is closed, and before each state update check !cancelled (or
controller.signal.aborted) to avoid applying results after close; ensure the
cancel flag is set where the modal close logic runs so in-flight promises won't
mutate state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea958173-2c9f-4fab-b1e0-b3ea3b57dbbf
📒 Files selected for processing (5)
src/api/services/rbac.tssrc/components/Forms/Formik/FormikResourceSelectorDropdown.tsxsrc/components/Permissions/PermissionAccessCheckModal.tsxsrc/components/Permissions/PermissionsView.tsxsrc/components/Playbooks/Settings/PlaybookPermissionsModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/services/rbac.ts
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.
Summary by CodeRabbit
New Features
UI/UX Improvements
Tests