Skip to content

feat: add identity management with RBAC permission model#914

Open
lashinijay wants to merge 17 commits into
wso2:mainfrom
lashinijay:feat-permission-model
Open

feat: add identity management with RBAC permission model#914
lashinijay wants to merge 17 commits into
wso2:mainfrom
lashinijay:feat-permission-model

Conversation

@lashinijay
Copy link
Copy Markdown

@lashinijay lashinijay commented May 19, 2026

  • Add Users, Roles, and Groups pages under Identities section in console
Screenshot 2026-05-21 at 08 34 07 Screenshot 2026-05-21 at 08 34 31 Screenshot 2026-05-21 at 08 34 37 - Implement identity controller and routes - proxying Thunder user/role/group APIs - Add invite user flow via Thunder USER_ONBOARDING flow (returns copyable invite link) - Add server-side pagination (offset/limit) with TablePagination component for all three pages - Fix Thunder list response decoding to use totalResults instead of total - Replace browser confirm() dialogs with themed ConfirmationDialog for delete actions - Wire RBAC permission model across all existing routes using HandleFuncWithValidationAndAuthz - Add RBAC permissions and predefined roles definitions - Add amp-system-client and related bootstrap scripts for Thunder OAuth app provisioning - Update Helm chart bootstrap to include email attribute in engineer user schema

Purpose

Describe the problems, issues, or needs driving this feature/fix and include links to related issues in the following format: Resolves issue1, issue2, etc.

Goals

Describe the solutions that this feature/fix will introduce to resolve the problems described above

Approach

Describe how you are implementing the solutions. Include an animated GIF or screenshot if the change affects the UI (email documentation@wso2.com to review all UI text). Include a link to a Markdown file or Google doc if the feature write-up is too long to paste here.

User stories

Summary of user stories addressed by this change>

Release note

Brief description of the new feature or bug fix as it will appear in the release notes

Documentation

Link(s) to product documentation that addresses the changes of this PR. If no doc impact, enter �N/A� plus brief explanation of why there�s no doc impact

Training

Link to the PR for changes to the training content in https://github.com/wso2/WSO2-Training, if applicable

Certification

Type �Sent� when you have provided new/updated certification questions, plus four answers for each question (correct answer highlighted in bold), based on this change. Certification questions/answers should be sent to certification@wso2.com and NOT pasted in this PR. If there is no impact on certification exams, type �N/A� and explain why.

Marketing

Link to drafts of marketing content that will describe and promote this feature, including product page changes, technical articles, blog posts, videos, etc., if applicable

Automation tests

  • Unit tests

    Code coverage information

  • Integration tests

    Details about the test cases and coverage

Security checks

Samples

Provide high-level details about the samples related to this feature

Related PRs

List any other related PRs

Migrations (if applicable)

Describe migration steps and platforms on which migration has been tested

Test environment

List all JDK versions, operating systems, databases, and browser/versions on which this feature/fix was tested

Learning

Describe the research phase and any blog posts, patterns, libraries, or add-ons you used to solve the problem.

Summary by CodeRabbit

  • New Features

    • Full identity management (org-scoped Users, Groups, Roles, invites, membership and role-assignment workflows) in UI and API.
  • New Features

    • Site-wide RBAC enforcement on API endpoints with runtime toggle.
  • Chores

    • Provisioning tooling to register permission resources and predefined roles/permission catalog for bootstrapping access controls.

Review Change Stack

- Add Users, Roles, and Groups pages under Identities section in console
- Implement identity controller and routes proxying Thunder user/role/group APIs
- Add invite user flow via Thunder USER_ONBOARDING flow (returns copyable invite link)
- Add server-side pagination (offset/limit) with TablePagination component for all three pages
- Fix Thunder list response decoding to use totalResults instead of total
- Replace browser confirm() dialogs with themed ConfirmationDialog for delete actions
- Wire RBAC permission model across all existing routes using HandleFuncWithValidationAndAuthz
- Add RBAC permissions and predefined roles definitions
- Add amp-system-client and related bootstrap scripts for Thunder OAuth app provisioning
- Update Helm chart bootstrap to include email attribute in engineer user schema

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds typed RBAC permissions and middleware, applies RBAC enforcement to HTTP routes, integrates Thunder identity client and controller, registers authenticated identity routes, implements frontend identities UI (types, API, hooks, pages), and bootstraps AMP resource-server permissions with deployment scripts.

Changes

RBAC Authorization System & Identity Management

Layer / File(s) Summary
RBAC Permission Definitions & Predefined Roles
agent-manager-service/rbac/permissions.go, agent-manager-service/rbac/predefined_roles.go
Defines typed Permission, ResourceServer prefix, Scope() helper, grouped permission constants, and a predefined role→permissions map.
Authorization middleware & path helpers
agent-manager-service/middleware/authorization.go, agent-manager-service/middleware/path_params.go
Adds RequirePermission and RequireDynamicPermission middleware and mux helpers HandleFuncWithValidationAndAuthz / HandleFuncWithValidationAndDynamicAuthz that combine path-param validation with static/dynamic RBAC enforcement.
RBAC Enforcement on API Routes
multiple agent-manager-service/api/*.go files
Switches many route registrations from validation-only to validation+authz middleware and supplies per-endpoint rbac.* permission constants across agents, environment, gateway, LLM, evaluator, monitor, catalog, repository, infra, git secrets, token, and repository endpoints.
Thunder Identity Types & Client Constructor
agent-manager-service/clients/thundersvc/identity_types.go, agent-manager-service/clients/thundersvc/client.go
Adds Thunder identity model types, a package HTTP client timeout constant, and a NewIdentityClient constructor.
Thunder Identity Client Implementation
agent-manager-service/clients/thundersvc/identity_client.go
Implements users/groups/roles CRUD, group membership and role assignment operations, AMP permissions discovery, InviteUser flow, centralized doRequest, NotFound mapping, and GetRootOUID.
Identity Controller & Routes
agent-manager-service/controllers/identity_controller.go, agent-manager-service/api/identity_routes.go, agent-manager-service/api/app.go, agent-manager-service/utils/constants.go, agent-manager-service/config/*
Adds IdentityController interface and concrete controller with handlers for users/groups/roles and AMP permissions; registers identity routes using authz-aware helpers; introduces RBACEnabled config flag and path param constants.
Controller tests
agent-manager-service/controllers/identity_controller_test.go
Adds tests validating resolveOuID caching and concurrent behavior with a scripted Thunder stub.
Wiring / DI
agent-manager-service/wiring/*
Wires Thunder identity client and identity controller into the app via Wire, adds ProvideIdentityClient, and assigns AppParams.IdentityController in generated wiring and test initializers.
Frontend types & API client
console/workspaces/libs/types/src/api/identities.ts, console/workspaces/libs/api-client/src/apis/identities.ts
Adds TypeScript API types and implements HTTP client wrappers for identity endpoints (users, groups, roles, AMP permissions).
Frontend React Query hooks
console/workspaces/libs/api-client/src/hooks/identities.ts
Wraps API functions in hooks for listing/getting/mutations, with token delegation and cache invalidation strategies (and pagination helpers).
Identity Management UI pages & routing
console/workspaces/pages/identities/src/*, console/workspaces/core-ui/*, console/rush.json, console/workspaces/pages/identities/package.json
Implements identity pages (users, invites, create/edit, groups, roles), IdentitiesOrganization route subtree, navigation items, lazy exports, tsconfigs, eslint, workspace registration, and package config.
Thunder bootstrap & deployment
deployments/scripts/register-amp-resources.sh, deployments/helm-charts/wso2-amp-thunder-extension/*
Adds script and Helm bootstrap template entries to register the AMP resource server with a full permission hierarchy and ensures email attribute is present in user schema and OAuth client attributes.
Runtime config & docker
deployments/docker-compose.yml, console/apps/web-ui/public/config.js
Adds RBAC_ENABLED environment variable and updates web-ui runtime config placeholders for token validation settings.

Estimated code review effort:
🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • rasika2012
  • hanzjk
  • menakaj

🐰 "I hopped through routes and guards with glee,
I added scopes and pages for identity,
Thunder signs the permissions, UI shows the light,
Roles and users now dance into the night. 🥕"

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Nitpick comments (5)
console/apps/web-ui/public/config.js (1)

36-36: ⚡ Quick win

Refactor the self-comparison to a clearer placeholder pattern.

The expression 'true' === 'true' compares a string literal to itself, which always evaluates to true. While this appears to be a placeholder pattern where the first 'true' would be replaced at build/runtime, the self-comparison creates confusion and triggers static analysis warnings. Consider using a more explicit placeholder pattern, such as a distinct template variable (e.g., '%%DISABLE_AUTH%%' === 'true') or an environment-driven approach that doesn't rely on in-place string replacement of identical literals.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/apps/web-ui/public/config.js` at line 36, The disableAuth assignment
uses a tautological self-comparison ('true' === 'true') which is confusing and
triggers linters; change the expression to use a clear placeholder or env-driven
value such as comparing a distinct template token or environment variable (e.g.,
replace the left-hand literal with a build-time placeholder token like
'%%DISABLE_AUTH%%' or read from process.env) so that disableAuth is computed as
('%%DISABLE_AUTH%%' === 'true') or an equivalent env check; update the config.js
disableAuth line and any build/runtime replacement logic to ensure the
placeholder is substituted correctly.
deployments/scripts/register-amp-resources.sh (1)

40-51: 💤 Low value

Suppressed curl errors may hide network issues.

The 2>/dev/null on the curl commands silently discards connection errors and SSL warnings. When debugging failures, this makes it harder to diagnose network problems vs API errors.

💡 Suggested improvement
 api_call() {
   local method="$1" endpoint="$2" data="${3:-}"
+  local curl_stderr
   if [[ -z "$data" ]]; then
-    curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \
+    curl_stderr=$(mktemp)
+    result=$(curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \
       -H "Content-Type: application/json" \
-      -H "Authorization: Bearer $TOKEN" 2>/dev/null || echo -e "\n000"
+      -H "Authorization: Bearer $TOKEN" 2>"$curl_stderr") || { cat "$curl_stderr" >&2; rm -f "$curl_stderr"; echo -e "\n000"; return; }
+    rm -f "$curl_stderr"
+    echo "$result"
   else
-    curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \
+    curl_stderr=$(mktemp)
+    result=$(curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \
       -H "Content-Type: application/json" \
       -H "Authorization: Bearer $TOKEN" \
-      -d "$data" 2>/dev/null || echo -e "\n000"
+      -d "$data" 2>"$curl_stderr") || { cat "$curl_stderr" >&2; rm -f "$curl_stderr"; echo -e "\n000"; return; }
+    rm -f "$curl_stderr"
+    echo "$result"
   fi
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deployments/scripts/register-amp-resources.sh` around lines 40 - 51, In the
api_call function, stop suppressing curl stderr so network/SSL errors are
visible: remove the "2>/dev/null" from both curl invocations and instead run
curl with flags like --fail --show-error (or capture stderr into a variable) and
surface that stderr when the call fails; update the two curl invocations inside
api_call (the branches with and without -d) to preserve and/or log curl's error
output and return a non-zero status or meaningful message instead of silently
emitting "\n000".
deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml (1)

159-159: 💤 Low value

Fragile string check for schema attribute detection.

The grep '"email"' may match email values elsewhere in the response (e.g., user emails in a paginated list), causing the patch to be skipped even when the schema lacks the attribute. Consider parsing the JSON schema structure specifically.

💡 Suggested improvement
-      if [[ -n "$SCHEMA_ID" ]] && ! echo "$BODY" | grep -q '"email"'; then
+      HAS_EMAIL_ATTR=$(echo "$BODY" | python3 -c "import sys,json; schemas=json.load(sys.stdin); schemas=schemas if isinstance(schemas,list) else schemas.get('schemas',[]); s=[s for s in schemas if s.get('name')=='{{ .Values.thunder.bootstrap.userSchema.name }}']; print('yes' if s and 'email' in s[0].get('schema',{}) else 'no')" 2>/dev/null || echo 'no')
+      if [[ -n "$SCHEMA_ID" ]] && [[ "$HAS_EMAIL_ATTR" != "yes" ]]; then
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml`
at line 159, The current string check uses grep '"email"' against BODY which can
match values elsewhere; instead parse the JSON in BODY to detect the schema
attribute explicitly. Replace the grep check around SCHEMA_ID/BODY with a
jq-based test, e.g. use echo "$BODY" | jq -e '(.attributes[]?.name == "email")
or (.properties.email != null)' >/dev/null to determine presence of the email
attribute and branch accordingly; update the conditional that references
SCHEMA_ID and BODY to use that jq test so only the schema's attribute
list/properties is inspected.
console/workspaces/pages/identities/src/UsersPage.tsx (1)

67-75: ⚡ Quick win

Make delete confirmation callback explicitly async-safe.

Returning deleteUser(...) directly from onConfirm can leak rejected promises depending on dialog callback handling.

Proposed fix
   const handleDelete = (user: ThunderUser) => {
     addConfirmation({
@@
-      onConfirm: () => deleteUser({ orgName: orgId, userId: user.id }),
+      onConfirm: async () => {
+        try {
+          await deleteUser({ orgName: orgId, userId: user.id });
+        } catch {
+          // rely on global mutation error handler
+        }
+      },
     });
   };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/workspaces/pages/identities/src/UsersPage.tsx` around lines 67 - 75,
The onConfirm currently returns deleteUser(...) directly which can leak rejected
promises; update the onConfirm in handleDelete to be an explicit async function
that awaits deleteUser({ orgName: orgId, userId: user.id }) and catches errors
(e.g., async () => { try { await deleteUser(...) } catch (err) { /*
handle/log/notify error */ } }) so the dialog callback is async-safe; reference
handleDelete and the deleteUser call when making this change.
console/workspaces/pages/identities/src/RolesPage.tsx (1)

67-75: ⚡ Quick win

Guard delete mutation errors inside confirmation callback.

onConfirm currently returns deleteRole(...) directly. If the mutation rejects and the dialog utility does not await/catch it, this can surface as an unhandled rejection.

Proposed fix
   const handleDelete = (role: ThunderRole) => {
     addConfirmation({
       title: "Delete Role",
       description: `Are you sure you want to delete "${role.name}"? This action cannot be undone.`,
       confirmButtonText: "Delete",
       confirmButtonColor: "error",
       confirmButtonIcon: <Trash size={16} />,
-      onConfirm: () => deleteRole({ orgName: orgId, roleId: role.id }),
+      onConfirm: async () => {
+        try {
+          await deleteRole({ orgName: orgId, roleId: role.id });
+        } catch {
+          // let global mutation error handling surface this
+        }
+      },
     });
   };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/workspaces/pages/identities/src/RolesPage.tsx` around lines 67 - 75,
The onConfirm callback passed to addConfirmation in handleDelete currently
returns deleteRole(...) directly which can cause unhandled rejections; modify
the onConfirm to call an async wrapper that awaits deleteRole({ orgName: orgId,
roleId: role.id }) inside a try/catch (or attaches a .catch handler) and
surface/log errors appropriately so any rejection is handled within the
confirmation flow (update the onConfirm defined in handleDelete to be async ()
=> { try { await deleteRole(...) } catch (err) { /* handle/log */ } } or
equivalent).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Line 660: The code in doRequest ignores the error returned by
io.ReadAll(resp.Body) when reading the response into the variable body; change
this to capture and handle the error (e.g., body, err := io.ReadAll(resp.Body)),
close resp.Body if not already closed, and return or wrap a clear error
(including HTTP status and the read error) instead of proceeding to decoding;
update any callers to expect this error path so truncated/failed responses are
surfaced immediately.

In `@agent-manager-service/controllers/identity_controller.go`:
- Around line 439-443: Clamp negative offset values to zero in the paginated
handlers that currently only normalize limit: after calling getIntQueryParam(r,
"offset", 0) in the GetGroupMembers and ListRoles handlers (and any other
paginated handlers similar to the shown snippet), add a check like if offset < 0
{ offset = 0 } so negative offsets are normalized before use; keep the existing
limit validation (if limit <= 0 || limit > 100 { limit = 20 }) unchanged and
ensure you update every occurrence matching this pattern (the offset variable
returned from getIntQueryParam) so invalid negative offsets cannot propagate
upstream.
- Around line 180-181: Remove raw user identifiers from error logs in the
CreateUser handling code: change the log.Error calls that currently include
"username" (and "email" in the other occurrence) so they only log a generic
message plus the error (e.g., log.Error("CreateUser failed", "error", err));
update both the occurrence shown (log.Error("CreateUser failed", "username",
body.Username, "error", err)) and the similar one at the other location (lines
264-265) to drop body.Username/body.Email; if extra correlation is needed, log a
non-identifying request ID or hashed identifier instead of the raw
username/email.
- Around line 66-71: The current use of sync.Once (rootOUOnce) in resolveOuID
caches both successes and failures permanently; change the logic so only
successful OU lookups are cached. Replace sync.Once with a sync.Mutex (or
RWMutex) and fields rootOUID/rootOUErr such that resolveOuID acquires the lock,
returns cached rootOUID if non-empty, otherwise attempts the Thunder Identity
lookup; on success set rootOUID (and clear rootOUErr) and persist for future
calls, but on failure do NOT persist rootOUErr (or persist it only transiently
with retry logic) — simply return the error so callers can retry. Update any
code referencing rootOUOnce to use the new mutex-based pattern (identify
resolveOuID, rootOUID, rootOUErr, and rootOUOnce in the diff) and add minimal
unit tests to ensure a failed first lookup does not block subsequent successful
retries.

In `@console/workspaces/libs/api-client/src/hooks/identities.ts`:
- Around line 179-193: The onSuccess handlers in useAddGroupMembers and
useRemoveGroupMembers only invalidate the 'identity-group-members' query,
leaving useGetUserGroups caches stale; update both handlers (the onSuccess in
the useApiMutation for mutationFn addGroupMembers and removeGroupMembers) to
also invalidate user-group caches by calling queryClient.invalidateQueries for
the user-groups key (e.g. 'identity-user-groups' or your useGetUserGroups
queryKey) for each affected userId from the mutation args (body.userIds) so both
group-member and per-user group lists are refreshed after add/remove operations.

In `@console/workspaces/pages/identities/package.json`:
- Around line 4-7: The package.json declares "type": "module" while also
including a "require" field that points to an ESM file, which causes
ERR_REQUIRE_ESM for CommonJS consumers; open the package.json where "type":
"module", "import": "./dist/index.js", "require": "./dist/index.js", and
"types": "dist/index.d.ts" are defined and remove the "require" entry (and any
other "require" mappings in similar page packages) so only "import" remains for
ESM packages, ensuring CommonJS consumers don't get an invalid require mapping.

In `@console/workspaces/pages/identities/src/GroupsPage.tsx`:
- Around line 107-112: The empty-state check should distinguish between "no
groups at all" and "no groups on the current page" so pagination is not hidden
when total > 0; update the render logic in GroupsPage to use both groups and
total (the variables named groups and total) and only show the "No groups yet"
ListingTable.EmptyState when total === 0, while when groups.length === 0 &&
total > 0 render a page-empty message (or a different ListingTable.EmptyState
variant) but keep the pagination component visible; adjust both occurrences that
currently check groups.length (the two ListingTable.EmptyState blocks) to follow
this rule so users can navigate back to previous pages.

In `@console/workspaces/pages/identities/src/RoleCreatePage.tsx`:
- Around line 52-65: handleSubmit currently awaits createRole directly which can
cause unhandled promise rejections; wrap the createRole call in a try/catch
inside handleSubmit so errors are caught and prevent bubbling, only call
navigate(rolesPath) on success, and keep existing name/description handling.
Specifically, in handleSubmit wrap await createRole({...}) in try { await
createRole(...) ; navigate(rolesPath); } catch (err) { /* no-op since
createError/snackbar already handles it or optionally log */ } so the mutation
error is caught and does not produce an unhandled rejection.

In `@console/workspaces/pages/identities/src/UserCreatePage.tsx`:
- Around line 64-83: The submit handler handleSubmit awaits createUser (the
mutateAsync call) but doesn't catch rejections, causing possible unhandled
promise rejections; wrap the await createUser(...) call in a try/catch so
failures are caught before calling navigate(usersPath) — on success navigate, on
error exit the handler (optionally log or let the mutation's onError/snackbar
handle user-visible errors) to prevent the rejected promise from escaping;
update handleSubmit accordingly so createUser, mutateAsync, and navigate are
referenced and the navigation only happens when createUser succeeds.

In `@console/workspaces/pages/identities/src/UserInvitePage.tsx`:
- Around line 61-77: handleSubmit currently awaits inviteUser directly which can
lead to unhandled promise rejections; wrap the inviteUser call in a try/catch
inside handleSubmit, keep the existing validation and
setEmailError/setInviteLink logic, and in the catch block set an appropriate
error state or log the error (and avoid swallowing it) so failures are handled
gracefully; reference the handleSubmit function and the inviteUser call where
setInviteLink is currently invoked.

---

Nitpick comments:
In `@console/apps/web-ui/public/config.js`:
- Line 36: The disableAuth assignment uses a tautological self-comparison
('true' === 'true') which is confusing and triggers linters; change the
expression to use a clear placeholder or env-driven value such as comparing a
distinct template token or environment variable (e.g., replace the left-hand
literal with a build-time placeholder token like '%%DISABLE_AUTH%%' or read from
process.env) so that disableAuth is computed as ('%%DISABLE_AUTH%%' === 'true')
or an equivalent env check; update the config.js disableAuth line and any
build/runtime replacement logic to ensure the placeholder is substituted
correctly.

In `@console/workspaces/pages/identities/src/RolesPage.tsx`:
- Around line 67-75: The onConfirm callback passed to addConfirmation in
handleDelete currently returns deleteRole(...) directly which can cause
unhandled rejections; modify the onConfirm to call an async wrapper that awaits
deleteRole({ orgName: orgId, roleId: role.id }) inside a try/catch (or attaches
a .catch handler) and surface/log errors appropriately so any rejection is
handled within the confirmation flow (update the onConfirm defined in
handleDelete to be async () => { try { await deleteRole(...) } catch (err) { /*
handle/log */ } } or equivalent).

In `@console/workspaces/pages/identities/src/UsersPage.tsx`:
- Around line 67-75: The onConfirm currently returns deleteUser(...) directly
which can leak rejected promises; update the onConfirm in handleDelete to be an
explicit async function that awaits deleteUser({ orgName: orgId, userId: user.id
}) and catches errors (e.g., async () => { try { await deleteUser(...) } catch
(err) { /* handle/log/notify error */ } }) so the dialog callback is async-safe;
reference handleDelete and the deleteUser call when making this change.

In
`@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml`:
- Line 159: The current string check uses grep '"email"' against BODY which can
match values elsewhere; instead parse the JSON in BODY to detect the schema
attribute explicitly. Replace the grep check around SCHEMA_ID/BODY with a
jq-based test, e.g. use echo "$BODY" | jq -e '(.attributes[]?.name == "email")
or (.properties.email != null)' >/dev/null to determine presence of the email
attribute and branch accordingly; update the conditional that references
SCHEMA_ID and BODY to use that jq test so only the schema's attribute
list/properties is inspected.

In `@deployments/scripts/register-amp-resources.sh`:
- Around line 40-51: In the api_call function, stop suppressing curl stderr so
network/SSL errors are visible: remove the "2>/dev/null" from both curl
invocations and instead run curl with flags like --fail --show-error (or capture
stderr into a variable) and surface that stderr when the call fails; update the
two curl invocations inside api_call (the branches with and without -d) to
preserve and/or log curl's error output and return a non-zero status or
meaningful message instead of silently emitting "\n000".
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aaa5ed89-e020-4388-9685-63104e336017

📥 Commits

Reviewing files that changed from the base of the PR and between 23940a2 and 4640a79.

⛔ Files ignored due to path filters (1)
  • console/common/config/rush/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (63)
  • agent-manager-service/api/agent_config_routes.go
  • agent-manager-service/api/agent_routes.go
  • agent-manager-service/api/agent_token_routes.go
  • agent-manager-service/api/app.go
  • agent-manager-service/api/catalog_routes.go
  • agent-manager-service/api/environment_routes.go
  • agent-manager-service/api/evaluator_routes.go
  • agent-manager-service/api/gateway_routes.go
  • agent-manager-service/api/git_secret_routes.go
  • agent-manager-service/api/identity_routes.go
  • agent-manager-service/api/infra_resource_routes.go
  • agent-manager-service/api/llm_deployment_routes.go
  • agent-manager-service/api/llm_provider_apikey_routes.go
  • agent-manager-service/api/llm_proxy_apikey_routes.go
  • agent-manager-service/api/llm_proxy_deployment_routes.go
  • agent-manager-service/api/llm_routes.go
  • agent-manager-service/api/monitor_routes.go
  • agent-manager-service/api/repository_routes.go
  • agent-manager-service/clients/thundersvc/client.go
  • agent-manager-service/clients/thundersvc/identity_client.go
  • agent-manager-service/clients/thundersvc/identity_types.go
  • agent-manager-service/config/config.go
  • agent-manager-service/config/config_loader.go
  • agent-manager-service/controllers/identity_controller.go
  • agent-manager-service/middleware/authorization.go
  • agent-manager-service/middleware/path_params.go
  • agent-manager-service/rbac/permissions.go
  • agent-manager-service/rbac/predefined_roles.go
  • agent-manager-service/utils/constants.go
  • agent-manager-service/wiring/params.go
  • agent-manager-service/wiring/wire.go
  • agent-manager-service/wiring/wire_gen.go
  • console/apps/web-ui/public/config.js
  • console/rush.json
  • console/workspaces/core-ui/package.json
  • console/workspaces/core-ui/src/Layouts/OxygenLayout/navigationItems.tsx
  • console/workspaces/core-ui/src/Route/Route.tsx
  • console/workspaces/core-ui/src/pages/index.tsx
  • console/workspaces/libs/api-client/src/apis/identities.ts
  • console/workspaces/libs/api-client/src/apis/index.ts
  • console/workspaces/libs/api-client/src/hooks/identities.ts
  • console/workspaces/libs/api-client/src/hooks/index.ts
  • console/workspaces/libs/types/src/api/identities.ts
  • console/workspaces/libs/types/src/api/index.ts
  • console/workspaces/libs/types/src/routes/generated-route.map.ts
  • console/workspaces/libs/types/src/routes/routes.map.ts
  • console/workspaces/pages/identities/eslint.config.js
  • console/workspaces/pages/identities/package.json
  • console/workspaces/pages/identities/src/GroupCreatePage.tsx
  • console/workspaces/pages/identities/src/GroupsPage.tsx
  • console/workspaces/pages/identities/src/Identities.Organization.tsx
  • console/workspaces/pages/identities/src/RoleCreatePage.tsx
  • console/workspaces/pages/identities/src/RolesPage.tsx
  • console/workspaces/pages/identities/src/UserCreatePage.tsx
  • console/workspaces/pages/identities/src/UserInvitePage.tsx
  • console/workspaces/pages/identities/src/UsersPage.tsx
  • console/workspaces/pages/identities/src/index.ts
  • console/workspaces/pages/identities/tsconfig.json
  • console/workspaces/pages/identities/tsconfig.lib.json
  • deployments/docker-compose.yml
  • deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml
  • deployments/helm-charts/wso2-amp-thunder-extension/values.yaml
  • deployments/scripts/register-amp-resources.sh

Comment thread agent-manager-service/clients/thundersvc/identity_client.go Outdated
Comment thread agent-manager-service/controllers/identity_controller.go
Comment thread agent-manager-service/controllers/identity_controller.go Outdated
Comment thread agent-manager-service/controllers/identity_controller.go
Comment thread console/workspaces/libs/api-client/src/hooks/identities.ts Outdated
Comment thread console/workspaces/pages/identities/package.json
Comment thread console/workspaces/pages/identities/src/GroupsPage.tsx Outdated
Comment thread console/workspaces/pages/identities/src/RoleCreatePage.tsx
Comment thread console/workspaces/pages/identities/src/UserCreatePage.tsx
Comment thread console/workspaces/pages/identities/src/UserInvitePage.tsx
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
console/workspaces/pages/identities/src/UsersPage.tsx (1)

111-116: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep pagination visible when dataset exists but current page is empty.

When total > 0 and the current page has no rows, this renders a global empty state and hides pagination, so users can’t navigate back to a valid page.

Suggested fix
+import React, { useEffect, useMemo, useState } from "react";
...
   const users = useMemo(() => data?.users ?? [], [data]);
   const total = data?.total ?? 0;
+  const hasAnyUsers = total > 0;
+
+  useEffect(() => {
+    if (page > 0 && page * rowsPerPage >= total) {
+      setPage(Math.max(0, Math.ceil(total / rowsPerPage) - 1));
+    }
+  }, [page, rowsPerPage, total]);
...
-        {users.length === 0 ? (
+        {!hasAnyUsers ? (
           <ListingTable.EmptyState
             illustration={<Users size={64} />}
             title="No users yet"
             description='Click "Invite User" to invite one.'
           />
         ) : (
...
-        {users.length > 0 && (
+        {hasAnyUsers && (
           <TablePagination

Also applies to: 150-163

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/workspaces/pages/identities/src/UsersPage.tsx` around lines 111 -
116, The current conditional hides pagination when the dataset exists but the
current page is empty; change the EmptyState render guard to only show the
global empty state when total === 0 (i.e., use if users.length === 0 && total
=== 0 or simply total === 0) so that when total > 0 but users.length === 0 the
table still renders with pagination enabled; update the conditional around
ListingTable.EmptyState (and the analogous block at the other location
referenced) to check total before deciding to render the global empty state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent-manager-service/api/identity_routes.go`:
- Line 46: Replace the OrgView permission with the role-read permission on the
group-role listing route: update the middleware.HandleFuncWithValidationAndAuthz
registration that wires ctrl.GetGroupRoles so it requires rbac.RoleRead instead
of rbac.OrgView, ensuring only callers with role-read privilege can enumerate
roles via GetGroupRoles.

In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Around line 309-319: The loop over resp.Members currently swallows GetUser
errors and returns a users slice smaller than resp.TotalResults; change this to
propagate failures instead of continue-ing: when c.GetUser(ctx, m.ID) returns an
error, return nil, 0 (or resp.TotalResults if you prefer) and that error
immediately so the caller sees the failure; update the loop in the function that
iterates resp.Members to return the error from GetUser rather than skipping
members, and adjust any callers/tests that assumed partial results if necessary.
- Around line 322-351: GetGroupRoles currently pages all roles via ListRoles
then calls GetRoleAssignments for each ThunderRole causing a fan-out; change
this to use a direct Thunder query or cached/indexed mapping for role
assignments (e.g., a new ListRoleAssignmentsByGroup or lookup that accepts
groupID) instead of per-role calls, and if you cannot implement the direct
lookup immediately, at minimum stop swallowing assignment fetch errors in
GetGroupRoles (do not continue on err from GetRoleAssignments — return the
error) to avoid returning partial/incomplete data; update references in
GetGroupRoles, ListRoles, GetRoleAssignments, and any cache/index to reflect the
new lookup approach.

In `@console/workspaces/libs/api-client/src/hooks/identities.ts`:
- Around line 199-205: The new hook useGetGroupRoles registers queries under the
'identity-group-roles' key but role-changing mutation handlers still don't
invalidate that key, causing stale group role data; update the role rename, role
delete, and assignee add/remove mutation handlers to call the React Query client
invalidateQueries for ['identity-group-roles', { orgName, groupId }] (or the
closest shape used by useGetGroupRoles) so invalidation is scoped to the
affected group(s) when groupId is available, falling back to invalidating the
broader 'identity-group-roles' key if not; use the same key shape as
useGetGroupRoles and add these invalidateQueries calls in the mutation
success/settled handlers.

In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 240-242: The UI currently treats rolesData == null as loading and
shows a spinner forever (including when the query fails); update the
GroupEditPage rendering to explicitly check the roles query status instead of
rolesData == null—use the query object's isLoading/isError (or error) flags
(e.g., rolesQuery.isLoading, rolesQuery.isError, rolesQuery.error) and render
the CircularProgress only when loading, render an error message or error
fallback when isError (including the actual error text), and only render the
empty-state UI when the query succeeded but roles.length === 0; update the
conditional around rolesData/roles to follow this explicit status check so
errors are not masked by the spinner.
- Around line 50-53: The member lists are hard-capped at 100 by the fixed {
offset: 0, limit: 100 } passed to useGetGroupMembers (and the other member list
call around the subsequent block), preventing edits to members beyond the first
page; change the calls to remove the fixed 100 limit and instead support
full/paginated fetching—either by passing a configurable limit/offset or by
implementing looped page fetching until no more results (update
useGetGroupMembers usage and its caller logic to accept/handle pagination
parameters or a “fetchAll” behavior so add/remove operations can see all
members).

In `@console/workspaces/pages/identities/src/UserEditPage.tsx`:
- Around line 57-60: The UserEditPage currently fetches only the first 100
groups via useListGroups({ orgName: orgId }, { offset: 0, limit: 100 }),
truncating group selection; update the fetching logic in UserEditPage to
retrieve all groups (e.g., remove the hardcoded limit and implement paginated
fetching using offset/limit or cursor until no more results) and merge results
into allGroupsData so the selector shows every group — adjust related loading
state (isLoadingAllGroups) and ensure duplicates are handled when concatenating
pages.
- Around line 75-77: The effect in UserEditPage that calls
setSelectedGroups(initialGroups) on every initialGroups change can overwrite
in-progress edits; change it to only initialize selectedGroups once or when the
user hasn't modified the selection: track user edits (e.g., a hasEditedGroups
boolean/ref toggled from the groups change handler) and in the useEffect for
initialGroups call setSelectedGroups(initialGroups) only if hasEditedGroups is
false (or when selectedGroups is undefined/uninitialized). Update handlers that
change selectedGroups to set the hasEditedGroups flag so background refetches
won't clobber unsaved edits; keep references to setSelectedGroups,
initialGroups, and the edit handler names to locate the changes.

---

Outside diff comments:
In `@console/workspaces/pages/identities/src/UsersPage.tsx`:
- Around line 111-116: The current conditional hides pagination when the dataset
exists but the current page is empty; change the EmptyState render guard to only
show the global empty state when total === 0 (i.e., use if users.length === 0 &&
total === 0 or simply total === 0) so that when total > 0 but users.length === 0
the table still renders with pagination enabled; update the conditional around
ListingTable.EmptyState (and the analogous block at the other location
referenced) to check total before deciding to render the global empty 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 36ee2f84-cc29-4357-8fd3-62c8fa683243

📥 Commits

Reviewing files that changed from the base of the PR and between 891dec3 and e2afe13.

⛔ Files ignored due to path filters (1)
  • console/common/config/rush/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (15)
  • agent-manager-service/api/identity_routes.go
  • agent-manager-service/clients/thundersvc/identity_client.go
  • agent-manager-service/clients/thundersvc/identity_types.go
  • agent-manager-service/controllers/identity_controller.go
  • agent-manager-service/wiring/wire.go
  • agent-manager-service/wiring/wire_gen.go
  • console/apps/web-ui/public/config.js
  • console/workspaces/core-ui/package.json
  • console/workspaces/libs/api-client/src/apis/identities.ts
  • console/workspaces/libs/api-client/src/hooks/identities.ts
  • console/workspaces/pages/identities/src/GroupEditPage.tsx
  • console/workspaces/pages/identities/src/GroupsPage.tsx
  • console/workspaces/pages/identities/src/Identities.Organization.tsx
  • console/workspaces/pages/identities/src/UserEditPage.tsx
  • console/workspaces/pages/identities/src/UsersPage.tsx
✅ Files skipped from review due to trivial changes (2)
  • console/workspaces/core-ui/package.json
  • agent-manager-service/wiring/wire_gen.go

Comment thread agent-manager-service/api/identity_routes.go Outdated
Comment thread agent-manager-service/clients/thundersvc/identity_client.go
Comment thread agent-manager-service/clients/thundersvc/identity_client.go
Comment on lines +199 to +205
export function useGetGroupRoles(params: GroupPathParams) {
const { getToken } = useAuthHooks();
return useApiQuery<{ roles: ThunderRole[] }>({
queryKey: ['identity-group-roles', params],
queryFn: () => getGroupRoles(params, getToken),
enabled: !!params.orgName && !!params.groupId,
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Invalidate identity-group-roles from role mutations.

This adds a new cache key, but role-changing mutations still only invalidate role-list / assignment queries. After a role rename, delete, or assignee add/remove, group detail views can keep showing stale assigned roles until a full refresh. Add an identity-group-roles invalidation in those mutation handlers, scoped to affected groupIds when available.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/workspaces/libs/api-client/src/hooks/identities.ts` around lines 199
- 205, The new hook useGetGroupRoles registers queries under the
'identity-group-roles' key but role-changing mutation handlers still don't
invalidate that key, causing stale group role data; update the role rename, role
delete, and assignee add/remove mutation handlers to call the React Query client
invalidateQueries for ['identity-group-roles', { orgName, groupId }] (or the
closest shape used by useGetGroupRoles) so invalidation is scoped to the
affected group(s) when groupId is available, falling back to invalidating the
broader 'identity-group-roles' key if not; use the same key shape as
useGetGroupRoles and add these invalidateQueries calls in the mutation
success/settled handlers.

Comment thread console/workspaces/pages/identities/src/GroupEditPage.tsx
Comment thread console/workspaces/pages/identities/src/GroupEditPage.tsx Outdated
Comment thread console/workspaces/pages/identities/src/UserEditPage.tsx Outdated
Comment thread console/workspaces/pages/identities/src/UserEditPage.tsx
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Around line 447-460: The loop over resp.Assignments currently swallows
failures from GetUser/GetGroup (in the switch cases) by using continue,
producing partial role assignment results; change this to fail fast by returning
the error (or a wrapped error with context) whenever c.GetUser(ctx, a.ID) or
c.GetGroup(ctx, a.ID) returns an error so callers don't receive incomplete
data—update the switch in the loop that iterates resp.Assignments to return the
error immediately (including which assignment ID and type) instead of appending
nothing and continuing, and ensure any callers expect/handle that error from
this method.

In `@agent-manager-service/controllers/identity_controller_test.go`:
- Around line 167-169: The test's unbounded loop waiting for ctrl.rootOUID can
hang CI; modify the loop around ctrl.resolveOuID(newRequest()) so it is bounded
(e.g., add a maxRetries counter or timeout) and break with a clear test failure
if rootOUID is not set after the limit; also stop ignoring errors from
resolveOuID—capture its error return and fail the test immediately on unexpected
errors or on exceeding retries to avoid infinite hangs.

In `@console/workspaces/libs/api-client/src/hooks/identities.ts`:
- Around line 126-144: useAllGroups registers a distinct cache key
'identity-groups-all' that currently isn't invalidated by group mutations;
update the group mutation hooks (e.g., createGroup, updateGroup, deleteGroup or
whatever uses useApiMutation for group changes) to also invalidate the
'identity-groups-all' cache key for the affected org (same params shape used in
useAllGroups). Specifically, add invalidation of ['identity-groups-all', params]
(or include it in the existing invalidation list/options passed to
useApiMutation/queryClient.invalidateQueries) alongside any existing
'identity-groups' invalidation so pages using useAllGroups get refreshed after
mutations.

In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 105-113: The dropdown filtering in the availableUsers useMemo only
excludes initialMembers from the current page which allows selecting members
that exist on other pages and leads handleSave to attempt duplicate adds; modify
the logic to exclude all existing member IDs by replacing/augmenting
initialMembers with a full list (e.g., allMemberIds) obtained from a
paginated-members hook, then build the excluded Set from
[...allMemberIds.filter(id not removed?), ...pendingAdds.map(u => u.id),
...removedIds?] and update the useMemo dependencies to include that full-list
variable; ensure handleSave deduplicates pendingAdds against this global member
ID set before submitting.

In `@console/workspaces/pages/identities/src/RoleEditPage.tsx`:
- Around line 60-67: The current calls to useListUsers and useListGroups in
RoleEditPage (allUsersData/allGroupsData) hard-code offset:0, limit:100 which
truncates results; update the autocomplete source to fetch/search/paginate
instead of loading only the first 100: remove the fixed offset/limit usage and
wire the autocomplete component (the assignee/user/group picker) to a
paginated/search API flow that calls useListUsers/useListGroups with a
search/query string and page/offset params (or use the hook’s provided
pagination callbacks), implement debounced query changes and "load more" or
virtualized pagination in the picker so users can find and select assignees
beyond the first page. Ensure useListUsers/useListGroups calls accept and pass
through search and pagination args, and replace any reliance on
allUsersData/allGroupsData as the single source for the autocomplete options.
- Around line 166-177: Currently the code loops and calls
addAssignees/removeAssignees for each individual ID (using pendingUserAdds,
removedUserIds, pendingGroupAdds, removedGroupIds), which is slow and fragile;
instead aggregate the IDs into arrays and call addAssignees once with body: {
userIds: [...] } (if any), call removeAssignees once with body: { userIds: [...]
} (if any), and do the same for groupIds, reusing the existing params object —
keep the existing function names addAssignees and removeAssignees and ensure you
only invoke them when the aggregated array is non-empty.

In `@console/workspaces/pages/identities/src/RolesPage.tsx`:
- Line 107: When server-side pagination deletes items and the current page
becomes empty despite total > 0, clamp the page index to the last valid page
instead of rendering the empty-table state; in RolesPage.tsx detect the
condition roles.length === 0 && total > 0 and call the page setter (e.g.,
setPage or setPageIndex) to Math.max(0, Math.ceil(total / pageSize) - 1) before
rendering, then return/loading or re-fetch so the UI displays the last non-empty
page. Apply the same guard/logic to the other conditional block around lines
146-158 that also checks total and renders the table.

In `@console/workspaces/pages/identities/src/UsersPage.tsx`:
- Line 114: When a paged response returns users.length === 0 but total > 0 the
table shows empty because the current page is out of range; add page clamping in
the UsersPage component: add a useEffect that watches total, users.length, page
and limit, compute lastPage = Math.max(1, Math.ceil(total / limit)) and if
users.length === 0 && total > 0 && page !== lastPage call the page setter (e.g.,
setPage(lastPage) or onPageChange(lastPage)) to move to the last valid page;
ensure the effect guards against infinite loops by only updating when page !==
lastPage. Also apply the same useEffect/clamping logic to the other paginated
list in this file (the block referenced around lines 153-165) using its
corresponding state variables (total, items length, page, limit and setter).
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3263313e-949a-4c8b-ace2-84a9d2dd375f

📥 Commits

Reviewing files that changed from the base of the PR and between e2afe13 and f662b73.

📒 Files selected for processing (34)
  • agent-manager-service/api/identity_routes.go
  • agent-manager-service/clients/thundersvc/identity_client.go
  • agent-manager-service/clients/thundersvc/identity_types.go
  • agent-manager-service/controllers/identity_controller.go
  • agent-manager-service/controllers/identity_controller_test.go
  • agent-manager-service/wiring/wire_gen.go
  • console/workspaces/libs/api-client/src/hooks/identities.ts
  • console/workspaces/pages/add-new-agent/package.json
  • console/workspaces/pages/add-new-project/package.json
  • console/workspaces/pages/agent-kind/package.json
  • console/workspaces/pages/agent-security/package.json
  • console/workspaces/pages/build/package.json
  • console/workspaces/pages/configure-agent/package.json
  • console/workspaces/pages/deploy/package.json
  • console/workspaces/pages/eval/package.json
  • console/workspaces/pages/gateways/package.json
  • console/workspaces/pages/identities/package.json
  • console/workspaces/pages/identities/src/GroupCreatePage.tsx
  • console/workspaces/pages/identities/src/GroupEditPage.tsx
  • console/workspaces/pages/identities/src/GroupsPage.tsx
  • console/workspaces/pages/identities/src/Identities.Organization.tsx
  • console/workspaces/pages/identities/src/RoleCreatePage.tsx
  • console/workspaces/pages/identities/src/RoleEditPage.tsx
  • console/workspaces/pages/identities/src/RolesPage.tsx
  • console/workspaces/pages/identities/src/UserCreatePage.tsx
  • console/workspaces/pages/identities/src/UserEditPage.tsx
  • console/workspaces/pages/identities/src/UserInvitePage.tsx
  • console/workspaces/pages/identities/src/UsersPage.tsx
  • console/workspaces/pages/llm-providers/package.json
  • console/workspaces/pages/logs/package.json
  • console/workspaces/pages/metrics/package.json
  • console/workspaces/pages/overview/package.json
  • console/workspaces/pages/test/package.json
  • console/workspaces/pages/traces/package.json
✅ Files skipped from review due to trivial changes (2)
  • console/workspaces/pages/eval/package.json
  • agent-manager-service/wiring/wire_gen.go

Comment thread agent-manager-service/clients/thundersvc/identity_client.go
Comment thread agent-manager-service/controllers/identity_controller_test.go Outdated
Comment thread console/workspaces/libs/api-client/src/hooks/identities.ts
Comment thread console/workspaces/pages/identities/src/GroupEditPage.tsx Outdated
Comment thread console/workspaces/pages/identities/src/RoleEditPage.tsx Outdated
Comment thread console/workspaces/pages/identities/src/RoleEditPage.tsx Outdated
Comment thread console/workspaces/pages/identities/src/RolesPage.tsx
Comment thread console/workspaces/pages/identities/src/UsersPage.tsx
@lashinijay lashinijay force-pushed the feat-permission-model branch from 62c010f to 9b9b285 Compare May 21, 2026 06:42
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
console/workspaces/pages/identities/src/GroupEditPage.tsx (1)

151-157: ⚡ Quick win

Batch add/remove member updates instead of sending one request per user.

Current per-user calls increase request volume and partial-update risk. Send aggregated userIds in a single add and single remove call.

♻️ Suggested refactor
-      const deduped = pendingAdds.filter((u) => !allMemberIds.has(u.id));
-      for (const u of deduped) {
-        await addMembers({ params: { orgName: orgId, groupId }, body: { userIds: [u.id] } });
-      }
-      for (const id of removedIds) {
-        await removeMembers({ params: { orgName: orgId, groupId }, body: { userIds: [id] } });
-      }
+      const userIdsToAdd = [...new Set(pendingAdds.map((u) => u.id).filter((id) => !allMemberIds.has(id)))];
+      const userIdsToRemove = [...removedIds];
+
+      if (userIdsToAdd.length > 0) {
+        await addMembers({ params: { orgName: orgId, groupId }, body: { userIds: userIdsToAdd } });
+      }
+      if (userIdsToRemove.length > 0) {
+        await removeMembers({ params: { orgName: orgId, groupId }, body: { userIds: userIdsToRemove } });
+      }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/workspaces/pages/identities/src/GroupEditPage.tsx` around lines 151 -
157, The current loop sends one API call per user; instead collect IDs and call
addMembers/removeMembers once: compute deduped = pendingAdds.filter(u =>
!allMemberIds.has(u.id)), build an array addIds = deduped.map(u => u.id) and if
addIds.length > 0 call addMembers({ params: { orgName: orgId, groupId }, body: {
userIds: addIds } }); similarly, if removedIds has items call removeMembers once
with body: { userIds: removedIds } to perform batched updates and avoid per-user
requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 58-60: The useAllGroupMemberIds query currently falls back to an
empty set on failure which makes members appear editable and weakens dedupe;
update the code where useAllGroupMemberIds is used (the call returning
allMemberIdsData and isLoadingAllMemberIds) to also read its error/isError state
and treat an error as a blocking state: when error/isError is true, do not
enable member edits or proceed with save, surface an explicit error UI/message
or disable the relevant controls, and avoid using an empty array/map as the
fallback for existingMemberIds; apply the same pattern to the other occurrences
of useAllGroupMemberIds usage (the similar variables and blocks referenced
elsewhere) so any query failure prevents edits and forces an explicit error
handling path.

---

Nitpick comments:
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 151-157: The current loop sends one API call per user; instead
collect IDs and call addMembers/removeMembers once: compute deduped =
pendingAdds.filter(u => !allMemberIds.has(u.id)), build an array addIds =
deduped.map(u => u.id) and if addIds.length > 0 call addMembers({ params: {
orgName: orgId, groupId }, body: { userIds: addIds } }); similarly, if
removedIds has items call removeMembers once with body: { userIds: removedIds }
to perform batched updates and avoid per-user requests.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f428f703-197d-42c8-b909-a7b3b4dd7814

📥 Commits

Reviewing files that changed from the base of the PR and between f662b73 and daf15a3.

⛔ Files ignored due to path filters (1)
  • console/common/config/rush/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (35)
  • agent-manager-service/api/identity_routes.go
  • agent-manager-service/clients/thundersvc/identity_client.go
  • agent-manager-service/config/config.go
  • agent-manager-service/config/config_loader.go
  • agent-manager-service/controllers/identity_controller.go
  • agent-manager-service/controllers/identity_controller_test.go
  • console/workspaces/core-ui/src/Layouts/OxygenLayout/navigationItems.tsx
  • console/workspaces/libs/api-client/src/apis/identities.ts
  • console/workspaces/libs/api-client/src/hooks/identities.ts
  • console/workspaces/pages/add-new-agent/package.json
  • console/workspaces/pages/add-new-project/package.json
  • console/workspaces/pages/agent-kind/package.json
  • console/workspaces/pages/agent-security/package.json
  • console/workspaces/pages/build/package.json
  • console/workspaces/pages/configure-agent/package.json
  • console/workspaces/pages/deploy/package.json
  • console/workspaces/pages/eval/package.json
  • console/workspaces/pages/gateways/package.json
  • console/workspaces/pages/identities/package.json
  • console/workspaces/pages/identities/src/GroupCreatePage.tsx
  • console/workspaces/pages/identities/src/GroupEditPage.tsx
  • console/workspaces/pages/identities/src/GroupsPage.tsx
  • console/workspaces/pages/identities/src/RoleCreatePage.tsx
  • console/workspaces/pages/identities/src/RoleEditPage.tsx
  • console/workspaces/pages/identities/src/RolesPage.tsx
  • console/workspaces/pages/identities/src/UserCreatePage.tsx
  • console/workspaces/pages/identities/src/UserEditPage.tsx
  • console/workspaces/pages/identities/src/UserInvitePage.tsx
  • console/workspaces/pages/identities/src/UsersPage.tsx
  • console/workspaces/pages/llm-providers/package.json
  • console/workspaces/pages/logs/package.json
  • console/workspaces/pages/metrics/package.json
  • console/workspaces/pages/overview/package.json
  • console/workspaces/pages/test/package.json
  • console/workspaces/pages/traces/package.json

Comment on lines +58 to +60
const { data: allMemberIdsData, isLoading: isLoadingAllMemberIds } = useAllGroupMemberIds(
{ orgName: orgId, groupId: groupId ?? "" },
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle all-member-ids query failures explicitly before enabling member edits.

If this query fails, the fallback empty set makes existing members appear selectable and weakens dedupe during save. Treat this as a blocking/error state instead of silently continuing.

💡 Suggested fix
-  const { data: allMemberIdsData, isLoading: isLoadingAllMemberIds } = useAllGroupMemberIds(
+  const {
+    data: allMemberIdsData,
+    isLoading: isLoadingAllMemberIds,
+    isError: isAllMemberIdsError,
+  } = useAllGroupMemberIds(
     { orgName: orgId, groupId: groupId ?? "" },
   );
@@
-  const isLoading = isLoadingMembers || isLoadingAllMemberIds || isLoadingUsers;
+  const isLoading = isLoadingMembers || isLoadingAllMemberIds || isLoadingUsers;
+  const hasBlockingError = isAllMemberIdsError;
@@
+  if (hasBlockingError) {
+    return (
+      <PageLayout title="Edit Group" disableIcon>
+        <Alert severity="error">Failed to load group membership data. Please refresh and try again.</Alert>
+      </PageLayout>
+    );
+  }

Also applies to: 81-84, 112-119, 166-166

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@console/workspaces/pages/identities/src/GroupEditPage.tsx` around lines 58 -
60, The useAllGroupMemberIds query currently falls back to an empty set on
failure which makes members appear editable and weakens dedupe; update the code
where useAllGroupMemberIds is used (the call returning allMemberIdsData and
isLoadingAllMemberIds) to also read its error/isError state and treat an error
as a blocking state: when error/isError is true, do not enable member edits or
proceed with save, surface an explicit error UI/message or disable the relevant
controls, and avoid using an empty array/map as the fallback for
existingMemberIds; apply the same pattern to the other occurrences of
useAllGroupMemberIds usage (the similar variables and blocks referenced
elsewhere) so any query failure prevents edits and forces an explicit error
handling path.

@lashinijay lashinijay force-pushed the feat-permission-model branch from 8428575 to 640c75e Compare May 26, 2026 07:00
…der set

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lashinijay lashinijay force-pushed the feat-permission-model branch from a3134cf to 22599c6 Compare May 26, 2026 07:37
Copy link
Copy Markdown
Contributor

@rasika2012 rasika2012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants