permissions: backend-driven permission catalog endpoint#1799
Open
gugu wants to merge 5 commits into
Open
Conversation
Adds GET /permissions/available, derived from CEDAR_SCHEMA.RocketAdmin.actions so the catalog stays in sync with the Cedar action enum. Replaces the hardcoded POLICY_ACTION_GROUPS list in the frontend with a signal-backed httpResource on UsersService, and switches PolicyAction's needsTable/needsDashboard booleans to a single resource discriminator. New ActionEvent and Panel categories now appear in the Cedar policy editor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a GET /permissions/available endpoint that returns AvailablePermissionDs derived from CEDAR_SCHEMA; protects the route with AuthMiddleware. Frontend is migrated to fetch and consume the dynamic permission catalog via UsersService and uses new permission-display helpers for labels, short labels, icons, and grouping. ChangesPermission Catalog System
Sequence DiagramsequenceDiagram
participant Client
participant PermissionController
participant buildPermissionCatalog
participant CEDAR_SCHEMA
Client->>PermissionController: GET /permissions/available
PermissionController->>buildPermissionCatalog: call
buildPermissionCatalog->>CEDAR_SCHEMA: read RocketAdmin.actions
CEDAR_SCHEMA-->>buildPermissionCatalog: actions list
buildPermissionCatalog-->>PermissionController: AvailablePermissionDs[]
PermissionController-->>Client: AvailablePermissionsResponseDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
backend/src/entities/permission/permission-catalog.builder.ts (1)
7-12: ⚡ Quick winUse interfaces for object-shape declarations.
ActionMetadataOverrideandSchemaShapeare object shapes declared astype; guideline requiresinterfacefor object shapes.Suggested fix
-type ActionMetadataOverride = { +interface ActionMetadataOverride { label?: string; shortLabel?: string; icon?: string; resourceOverride?: 'none' | string; -}; +} -type SchemaShape = { +interface SchemaShape { RocketAdmin: { actions: Record<string, { appliesTo: { principalTypes: Array<string>; resourceTypes: Array<string> } }>; }; -}; +}As per coding guidelines, "Use interfaces for object shapes and type for unions and primitives".
Also applies to: 177-181
🤖 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 `@backend/src/entities/permission/permission-catalog.builder.ts` around lines 7 - 12, Change the object-shape declarations from type aliases to interfaces: replace the `type ActionMetadataOverride = { ... }` declaration with `interface ActionMetadataOverride { ... }` and likewise convert the `SchemaShape` type alias to an `interface SchemaShape { ... }`; keep the same property names and optional markers (label, shortLabel, icon, resourceOverride, etc.) and preserve exported/visibility modifiers and any JSDoc so all usages of ActionMetadataOverride and SchemaShape continue to work.backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts (1)
76-79: ⚡ Quick winAssert
panel:*wildcard to lock the full wildcard contract.The catalog builder includes a Panel wildcard path; adding this assertion prevents silent regressions for panel permissions.
Suggested fix
t.true(values.has('*'), 'catalog must include the General * wildcard'); t.true(values.has('table:*'), 'catalog must include the table:* wildcard'); t.true(values.has('dashboard:*'), 'catalog must include the dashboard:* wildcard'); +t.true(values.has('panel:*'), 'catalog must include the panel:* wildcard');🤖 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 `@backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts` around lines 76 - 79, The test currently checks that the catalog includes '*' , 'table:*' and 'dashboard:*' but misses the panel wildcard; update the test in non-saas-permission-catalog-e2e.test.ts to also assert that values.has('panel:*') is true by adding an assertion using the existing t.true(values.has('panel:*'), 'catalog must include the panel:* wildcard') so the catalog builder's panel wildcard is covered and prevents regressions (look for the variable values and the nearby t.true assertions to place this check).frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts (1)
67-68: ⚡ Quick winAlign private signal fields with frontend naming/modifier rules.
Use underscore-prefixed private members and mark computed signal fields as readonly.
✏️ Suggested cleanup
- private availableActions = computed(() => this._users.availablePermissions()); - private availableGroups = computed(() => this._users.availablePermissionGroups()); + private readonly _availableActions = computed(() => this._users.availablePermissions()); + private readonly _availableGroups = computed(() => this._users.availablePermissionGroups());As per coding guidelines,
frontend/**/*.ts: "Prefix private members with underscore (e.g.,_privateMethod,_dataService)" andfrontend/**/*.component.ts: "Signals for component state must be declared asprotectedorprivate readonlyand initialized withsignal()orcomputed()".🤖 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 `@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts` around lines 67 - 68, The signals availableActions and availableGroups must follow component naming/modifier rules: rename them to private readonly _availableActions and private readonly _availableGroups and update their usages accordingly; keep the computed initialization but change the declarations from "private" to "private readonly" and prefix the identifiers with an underscore so computed(() => this._users.availablePermissions()) and computed(() => this._users.availablePermissionGroups()) are assigned to _availableActions and _availableGroups respectively.
🤖 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 `@backend/src/entities/permission/permission-catalog.builder.ts`:
- Around line 52-53: The code currently filters emitted groups to only those in
GROUP_ORDER, causing any new/unknown Cedar action groups to be dropped; update
the grouping logic (where GROUP_ORDER is used and where groups are emitted —
e.g., the code that builds groupedActions/resultGroups around lines referencing
GROUP_ORDER) so that after adding groups in GROUP_ORDER order you append any
remaining group keys from the actual grouping map (preserve their original names
and either original detection order or sorted lexicographically) rather than
omitting them; ensure the same change is applied to the other occurrence noted
(lines 83–86) so unknown groups are included in API output.
- Around line 62-175: Convert the standalone function declarations into const
arrow function expressions while preserving their names, signatures and return
types: replace appendAction, buildAction, buildWildcardAllAction,
buildPrefixWildcard, deriveResource, resolveGroupName, autoLabel,
autoShortLabel, and capitalize with const <name> = (<params>): <returnType> => {
... } forms; ensure existing type annotations (e.g., AvailablePermissionDs,
Array<string>, Map types) remain intact and behavior is unchanged (including use
of throw/return and side effects like map mutation in appendAction).
In
`@backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts`:
- Line 41: Remove the explicit call to app.getHttpServer().listen(0) in the test
setup; supertest directly uses app.getHttpServer(), so delete the listen
invocation (reference: the call to app.getHttpServer().listen(0)) to avoid
redundant server startup and teardown flakiness and let tests use the provided
server instance without awaiting or manually listening.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`:
- Around line 281-283: _actionResource currently returns undefined when
_findAction(value) is missing, causing downstream flags
(needsTable/needsDashboard) to be false; modify _actionResource to return a
deterministic fallback resource when _findAction(value) is undefined by deriving
a resource object from the action string (for example parse the value to infer
resource type and id or return a stable default resource shape such as { type:
inferredType, id: inferredId } ), so code that relies on
PolicyAction['resource'] (and the needsTable/needsDashboard logic) always
receives a predictable resource; reference the _actionResource and _findAction
symbols and ensure the fallback logic covers missing catalog/availability states
instead of returning undefined.
---
Nitpick comments:
In `@backend/src/entities/permission/permission-catalog.builder.ts`:
- Around line 7-12: Change the object-shape declarations from type aliases to
interfaces: replace the `type ActionMetadataOverride = { ... }` declaration with
`interface ActionMetadataOverride { ... }` and likewise convert the
`SchemaShape` type alias to an `interface SchemaShape { ... }`; keep the same
property names and optional markers (label, shortLabel, icon, resourceOverride,
etc.) and preserve exported/visibility modifiers and any JSDoc so all usages of
ActionMetadataOverride and SchemaShape continue to work.
In
`@backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts`:
- Around line 76-79: The test currently checks that the catalog includes '*' ,
'table:*' and 'dashboard:*' but misses the panel wildcard; update the test in
non-saas-permission-catalog-e2e.test.ts to also assert that
values.has('panel:*') is true by adding an assertion using the existing
t.true(values.has('panel:*'), 'catalog must include the panel:* wildcard') so
the catalog builder's panel wildcard is covered and prevents regressions (look
for the variable values and the nearby t.true assertions to place this check).
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`:
- Around line 67-68: The signals availableActions and availableGroups must
follow component naming/modifier rules: rename them to private readonly
_availableActions and private readonly _availableGroups and update their usages
accordingly; keep the computed initialization but change the declarations from
"private" to "private readonly" and prefix the identifiers with an underscore so
computed(() => this._users.availablePermissions()) and computed(() =>
this._users.availablePermissionGroups()) are assigned to _availableActions and
_availableGroups respectively.
🪄 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: 605fe83c-45ad-45e0-810e-39fdda36adca
📒 Files selected for processing (9)
backend/src/entities/permission/application/data-structures/available-permissions.ds.tsbackend/src/entities/permission/permission-catalog.builder.tsbackend/src/entities/permission/permission.controller.tsbackend/src/entities/permission/permission.module.tsbackend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.tsfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.spec.tsfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.tsfrontend/src/app/lib/cedar-policy-items.tsfrontend/src/app/services/users.service.ts
Comment on lines
+52
to
+53
| const GROUP_ORDER = ['General', 'Connection', 'Group', 'Table', 'ActionEvent', 'Dashboard', 'Panel']; | ||
|
|
There was a problem hiding this comment.
Don't drop unknown Cedar action groups from the response.
The current return path only emits groups listed in GROUP_ORDER. If Cedar adds a new action prefix, actions get grouped but then omitted from API output, which breaks the “schema-driven sync” goal.
Suggested fix
- return GROUP_ORDER.filter((name) => grouped.has(name)).map((name) => ({
- group: name,
- actions: grouped.get(name)!,
- }));
+ const orderedKnownGroups = GROUP_ORDER.filter((name) => grouped.has(name));
+ const unknownGroups = Array.from(grouped.keys()).filter((name) => !GROUP_ORDER.includes(name)).sort();
+ const orderedGroups = [...orderedKnownGroups, ...unknownGroups];
+
+ return orderedGroups.map((name) => ({
+ group: name,
+ actions: grouped.get(name)!,
+ }));Also applies to: 83-86
🤖 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 `@backend/src/entities/permission/permission-catalog.builder.ts` around lines
52 - 53, The code currently filters emitted groups to only those in GROUP_ORDER,
causing any new/unknown Cedar action groups to be dropped; update the grouping
logic (where GROUP_ORDER is used and where groups are emitted — e.g., the code
that builds groupedActions/resultGroups around lines referencing GROUP_ORDER) so
that after adding groups in GROUP_ORDER order you append any remaining group
keys from the actual grouping map (preserve their original names and either
original detection order or sorted lexicographically) rather than omitting them;
ensure the same change is applied to the other occurrence noted (lines 83–86) so
unknown groups are included in API output.
Comment on lines
+62
to
+175
| export function buildPermissionCatalog(): Array<AvailablePermissionGroupDs> { | ||
| const schemaActions = (CEDAR_SCHEMA as SchemaShape).RocketAdmin.actions; | ||
| const grouped = new Map<string, Array<AvailablePermissionDs>>(); | ||
|
|
||
| for (const [value, definition] of Object.entries(schemaActions)) { | ||
| const action = buildAction(value, definition.appliesTo.resourceTypes); | ||
| const groupName = resolveGroupName(value); | ||
| appendAction(grouped, groupName, action); | ||
| } | ||
|
|
||
| appendAction(grouped, 'General', buildWildcardAllAction()); | ||
|
|
||
| for (const groupName of WILDCARD_GROUPS) { | ||
| const actions = grouped.get(groupName); | ||
| if (actions && actions.length > 1) { | ||
| const prefix = groupName.toLowerCase(); | ||
| const resource = actions.find((a) => a.resource)?.resource; | ||
| actions.unshift(buildPrefixWildcard(prefix, groupName, resource)); | ||
| } | ||
| } | ||
|
|
||
| return GROUP_ORDER.filter((name) => grouped.has(name)).map((name) => ({ | ||
| group: name, | ||
| actions: grouped.get(name)!, | ||
| })); | ||
| } | ||
|
|
||
| function appendAction( | ||
| target: Map<string, Array<AvailablePermissionDs>>, | ||
| groupName: string, | ||
| action: AvailablePermissionDs, | ||
| ): void { | ||
| const list = target.get(groupName); | ||
| if (list) { | ||
| list.push(action); | ||
| } else { | ||
| target.set(groupName, [action]); | ||
| } | ||
| } | ||
|
|
||
| function buildAction(value: string, resourceTypes: Array<string>): AvailablePermissionDs { | ||
| const meta = ACTION_DISPLAY_METADATA[value] ?? {}; | ||
| const derivedResource = deriveResource(resourceTypes); | ||
| const resource = | ||
| meta.resourceOverride === NONE_RESOURCE_OVERRIDE ? undefined : (meta.resourceOverride ?? derivedResource); | ||
|
|
||
| const action: AvailablePermissionDs = { | ||
| value, | ||
| label: meta.label ?? autoLabel(value), | ||
| shortLabel: meta.shortLabel ?? autoShortLabel(value), | ||
| icon: meta.icon ?? 'help_outline', | ||
| }; | ||
| if (resource) { | ||
| action.resource = resource; | ||
| } | ||
| return action; | ||
| } | ||
|
|
||
| function buildWildcardAllAction(): AvailablePermissionDs { | ||
| return { | ||
| value: '*', | ||
| label: 'Full access (all permissions)', | ||
| shortLabel: 'Full access', | ||
| icon: 'shield', | ||
| }; | ||
| } | ||
|
|
||
| function buildPrefixWildcard(prefix: string, groupName: string, resource: string | undefined): AvailablePermissionDs { | ||
| const labels = WILDCARD_LABELS[groupName] ?? { label: `Full ${prefix} access`, shortLabel: 'Full access' }; | ||
| const action: AvailablePermissionDs = { | ||
| value: `${prefix}:*`, | ||
| label: labels.label, | ||
| shortLabel: labels.shortLabel, | ||
| icon: 'shield', | ||
| }; | ||
| if (resource) { | ||
| action.resource = resource; | ||
| } | ||
| return action; | ||
| } | ||
|
|
||
| function deriveResource(resourceTypes: Array<string>): string | undefined { | ||
| for (const type of resourceTypes) { | ||
| const candidate = type.charAt(0).toLowerCase() + type.slice(1); | ||
| if (UI_RENDERABLE_RESOURCES.has(candidate)) { | ||
| return candidate; | ||
| } | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| function resolveGroupName(actionValue: string): string { | ||
| const [prefix] = actionValue.split(':'); | ||
| return PREFIX_TO_GROUP[prefix] ?? capitalize(prefix); | ||
| } | ||
|
|
||
| function autoLabel(value: string): string { | ||
| const [prefix, verb = ''] = value.split(':'); | ||
| const groupName = PREFIX_TO_GROUP[prefix] ?? capitalize(prefix); | ||
| const verbWords = verb.split('-').map(capitalize).join(' '); | ||
| return verbWords ? `${groupName} ${verbWords.toLowerCase()}` : groupName; | ||
| } | ||
|
|
||
| function autoShortLabel(value: string): string { | ||
| const verb = value.split(':')[1] ?? value; | ||
| return verb | ||
| .split('-') | ||
| .map((part, index) => (index === 0 ? capitalize(part) : part)) | ||
| .join(' '); | ||
| } | ||
|
|
||
| function capitalize(text: string): string { | ||
| return text ? text.charAt(0).toUpperCase() + text.slice(1) : text; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify function declarations in this file
rg -nP '^\s*function\s+\w+\s*\(' backend/src/entities/permission/permission-catalog.builder.tsRepository: rocket-admin/rocketadmin
Length of output: 665
Use arrow functions for helper declarations in backend/src/entities/permission/permission-catalog.builder.ts.
This file still defines multiple helpers with function declarations (appendAction, buildAction, buildWildcardAllAction, buildPrefixWildcard, deriveResource, resolveGroupName, autoLabel, autoShortLabel, capitalize); convert them to const ... = (...) => {} to match the TS style guideline.
🤖 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 `@backend/src/entities/permission/permission-catalog.builder.ts` around lines
62 - 175, Convert the standalone function declarations into const arrow function
expressions while preserving their names, signatures and return types: replace
appendAction, buildAction, buildWildcardAllAction, buildPrefixWildcard,
deriveResource, resolveGroupName, autoLabel, autoShortLabel, and capitalize with
const <name> = (<params>): <returnType> => { ... } forms; ensure existing type
annotations (e.g., AvailablePermissionDs, Array<string>, Map types) remain
intact and behavior is unchanged (including use of throw/return and side effects
like map mutation in appendAction).
| }), | ||
| ); | ||
| await app.init(); | ||
| app.getHttpServer().listen(0); |
There was a problem hiding this comment.
Remove the explicit server listen in this E2E setup.
supertest uses app.getHttpServer() directly; listen(0) here is redundant and may create teardown flakiness because it isn’t awaited.
Suggested fix
- app.getHttpServer().listen(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.getHttpServer().listen(0); |
🤖 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
`@backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts`
at line 41, Remove the explicit call to app.getHttpServer().listen(0) in the
test setup; supertest directly uses app.getHttpServer(), so delete the listen
invocation (reference: the call to app.getHttpServer().listen(0)) to avoid
redundant server startup and teardown flakiness and let tests use the provided
server instance without awaiting or manually listening.
Comment on lines
+281
to
+283
| private _actionResource(value: string): PolicyAction['resource'] | undefined { | ||
| return this._findAction(value)?.resource; | ||
| } |
There was a problem hiding this comment.
Add a deterministic fallback when action metadata is missing.
At Line 282, _actionResource relies only on catalog lookup. If an action is not present in availablePermissions (loading/error/stale catalog), needsTable / needsDashboard become false and save paths can emit policies without required tableName/dashboardId.
💡 Proposed fix
private _actionResource(value: string): PolicyAction['resource'] | undefined {
- return this._findAction(value)?.resource;
+ const resource = this._findAction(value)?.resource;
+ if (resource != null) return resource;
+ if (value.startsWith('table:')) return 'table';
+ if (value.startsWith('dashboard:')) return 'dashboard';
+ return undefined;
}🤖 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
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`
around lines 281 - 283, _actionResource currently returns undefined when
_findAction(value) is missing, causing downstream flags
(needsTable/needsDashboard) to be false; modify _actionResource to return a
deterministic fallback resource when _findAction(value) is undefined by deriving
a resource object from the action string (for example parse the value to infer
resource type and id or return a stable default resource shape such as { type:
inferredType, id: inferredId } ), so code that relies on
PolicyAction['resource'] (and the needsTable/needsDashboard logic) always
receives a predictable resource; reference the _actionResource and _findAction
symbols and ensure the fallback logic covers missing catalog/availability states
instead of returning undefined.
Drop labels, short labels, icons, and synthesized wildcards from the backend
response. Each action is now { value, resource } only, where resource is the
first appliesTo.resourceTypes entry lowercased. Wildcards (*, table:*, etc.)
and display copy move to the frontend: a new lib/permission-display.ts derives
labels/icons algorithmically, and CedarPolicyListComponent synthesizes the
General and per-prefix wildcard entries at render time.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backend now returns { actions: [...] } instead of pre-grouped categories.
Group names ('Connection', 'ActionEvent', etc.) and group ordering live in
permission-display.ts on the frontend, where groupNameForAction(value)
derives them from the action prefix. UsersService computes
availablePermissionGroups from the flat list at read time.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Artuomka
approved these changes
May 26, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts (1)
288-291:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd deterministic fallback when catalog lookup misses an action.
_scopeResourcestill returnsundefinedwhendisplayActions()has no matching entry (Line 290), which can incorrectly disable required scope checks for table/dashboard actions.Suggested fix
private _scopeResource(value: string): PolicyActionResource | undefined { if (value === 'dashboard:create') return undefined; - return this._findAction(value)?.resource; + const resource = this._findAction(value)?.resource; + if (resource != null) return resource; + if (value.startsWith('table:')) return 'table'; + if (value.startsWith('dashboard:')) return 'dashboard'; + if (value.startsWith('panel:')) return 'panel'; + if (value.startsWith('connection:')) return 'connection'; + if (value.startsWith('group:')) return 'group'; + if (value.startsWith('actionEvent:')) return 'actionEvent'; + return undefined; }🤖 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 `@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts` around lines 288 - 291, _scopeResource currently returns undefined when _findAction(value) misses, which can disable scope checks; update _scopeResource to return a deterministic fallback by parsing the resource token from the action string (split value on ':'), map that token to the PolicyActionResource enum or a small lookup (e.g., "table" => PolicyActionResource.Table, "dashboard" => PolicyActionResource.Dashboard), and return that mapped value when _findAction(value) is undefined (keeping the existing special-case for 'dashboard:create' if required); implement the mapping logic inside _scopeResource so callers no longer receive undefined for unknown/missed actions.
🧹 Nitpick comments (4)
backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts (1)
59-61: ⚡ Quick winUse interfaces for the response/action object shapes in this test.
The inline object literal assertion here conflicts with the TS object-shape guideline.
♻️ Suggested minimal change
+interface AvailablePermissionAction { + value: string; + resource?: string; +} + +interface AvailablePermissionsResponseBody { + actions: Array<AvailablePermissionAction>; +} + - const body = response.body as { - actions: Array<{ value: string; resource?: string }>; - }; + const body = response.body as AvailablePermissionsResponseBody;As per coding guidelines, "Use interfaces for object shapes and type for unions and primitives."
🤖 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 `@backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts` around lines 59 - 61, Replace the inline type literal used for response.body with named interfaces: declare an interface (e.g., PermissionAction with properties value: string and optional resource?: string) and an interface (e.g., PermissionResponse with actions: PermissionAction[]), then cast response.body to that interface (e.g., const body = response.body as PermissionResponse) so the test uses named interfaces instead of an inline object literal; update any references to actions accordingly in non-saas-permission-catalog-e2e.test.ts.frontend/src/app/lib/permission-display.ts (1)
21-80: ⚡ Quick winConvert utility function declarations to arrow functions for guideline compliance.
Lines 21–80 introduce several function declarations; this repo’s JS/TS guideline prefers arrow functions. Please convert these helpers to
const fn = (...) => ...style for consistency.As per coding guidelines, "Prefer arrow functions over function declarations".
🤖 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 `@frontend/src/app/lib/permission-display.ts` around lines 21 - 80, Convert the top-level helper function declarations to arrow-function expressions to follow repo guidelines: replace the function declarations for groupNameForAction, actionLabel, actionShortLabel, actionIcon, verbInLabel, and capitalize with const <name> = (params): returnType => { ... } (preserving exports like export on groupNameForAction and actionLabel), keep existing parameter and return types, preserve internal logic (including early returns and constants like WILDCARD_ICON, VERB_TO_ICON, etc.), and ensure no behavioral changes or scope differences occur when switching to arrow functions.frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts (1)
70-71: ⚡ Quick winAlign private computed signal fields with frontend naming/read-only conventions.
displayGroupsanddisplayActionsare private state signals; they should follow the private-member underscore convention and bereadonly.As per coding guidelines, "Prefix private members with underscore (e.g.,
_privateMethod,_dataService)" and "Signals for component state must be declared asprotectedorprivate readonlyand initialized withsignal()orcomputed()".🤖 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 `@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts` around lines 70 - 71, The private computed signals displayGroups and displayActions violate naming and immutability conventions; rename them to _displayGroups and _displayActions and declare them as private readonly computed signals (private readonly _displayGroups = computed(...), private readonly _displayActions = computed(...)) and update all usages in this component to reference _displayGroups() and _displayActions() (and where displayGroups() or displayActions() are used, switch to the new names) so they follow the underscore private-member convention and read-only signal rule; keep the computation logic in _buildDisplayGroups() unchanged.frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.spec.ts (1)
270-272: ⚡ Quick winUse a named testable type alias for protected signal access.
Instead of the inline intersection cast, define a dedicated type alias for test access and reuse it in the spec.
As per coding guidelines, "Use
Partial<ServiceType>instead ofas anyfor mock services in tests, and create testable type aliases for accessing protected signals".🤖 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 `@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.spec.ts` around lines 270 - 272, Replace the inline intersection cast used for test access with a named testable type alias: define a type alias (e.g., TestableCedarPolicyListComponent) that extends CedarPolicyListComponent and declares the protected accessor (addActionGroups: () => PolicyActionGroup[]), then cast component to that alias (const testable = component as TestableCedarPolicyListComponent) instead of the inline intersection to access the protected signal; this follows the guideline to create testable type aliases for protected access and avoids ad-hoc casts.
🤖 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 `@frontend/src/app/services/users.service.ts`:
- Around line 65-68: The current return drops groups not listed in
PERMISSION_GROUP_ORDER; change it to include known groups in the defined order
first and then append any unknown groups from byGroup. Concretely, build the
orderedKnown array by filtering PERMISSION_GROUP_ORDER for names present in
byGroup and mapping to {group, actions} (same as current), build unknown array
by iterating Array.from(byGroup.keys()) and selecting keys not in
PERMISSION_GROUP_ORDER and mapping them similarly, and finally return
[...orderedKnown, ...unknown] so backend-driven groups are preserved after the
known ordered groups.
---
Duplicate comments:
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`:
- Around line 288-291: _scopeResource currently returns undefined when
_findAction(value) misses, which can disable scope checks; update _scopeResource
to return a deterministic fallback by parsing the resource token from the action
string (split value on ':'), map that token to the PolicyActionResource enum or
a small lookup (e.g., "table" => PolicyActionResource.Table, "dashboard" =>
PolicyActionResource.Dashboard), and return that mapped value when
_findAction(value) is undefined (keeping the existing special-case for
'dashboard:create' if required); implement the mapping logic inside
_scopeResource so callers no longer receive undefined for unknown/missed
actions.
---
Nitpick comments:
In
`@backend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.ts`:
- Around line 59-61: Replace the inline type literal used for response.body with
named interfaces: declare an interface (e.g., PermissionAction with properties
value: string and optional resource?: string) and an interface (e.g.,
PermissionResponse with actions: PermissionAction[]), then cast response.body to
that interface (e.g., const body = response.body as PermissionResponse) so the
test uses named interfaces instead of an inline object literal; update any
references to actions accordingly in non-saas-permission-catalog-e2e.test.ts.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.spec.ts`:
- Around line 270-272: Replace the inline intersection cast used for test access
with a named testable type alias: define a type alias (e.g.,
TestableCedarPolicyListComponent) that extends CedarPolicyListComponent and
declares the protected accessor (addActionGroups: () => PolicyActionGroup[]),
then cast component to that alias (const testable = component as
TestableCedarPolicyListComponent) instead of the inline intersection to access
the protected signal; this follows the guideline to create testable type aliases
for protected access and avoids ad-hoc casts.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`:
- Around line 70-71: The private computed signals displayGroups and
displayActions violate naming and immutability conventions; rename them to
_displayGroups and _displayActions and declare them as private readonly computed
signals (private readonly _displayGroups = computed(...), private readonly
_displayActions = computed(...)) and update all usages in this component to
reference _displayGroups() and _displayActions() (and where displayGroups() or
displayActions() are used, switch to the new names) so they follow the
underscore private-member convention and read-only signal rule; keep the
computation logic in _buildDisplayGroups() unchanged.
In `@frontend/src/app/lib/permission-display.ts`:
- Around line 21-80: Convert the top-level helper function declarations to
arrow-function expressions to follow repo guidelines: replace the function
declarations for groupNameForAction, actionLabel, actionShortLabel, actionIcon,
verbInLabel, and capitalize with const <name> = (params): returnType => { ... }
(preserving exports like export on groupNameForAction and actionLabel), keep
existing parameter and return types, preserve internal logic (including early
returns and constants like WILDCARD_ICON, VERB_TO_ICON, etc.), and ensure no
behavioral changes or scope differences occur when switching to arrow functions.
🪄 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: 364edf3d-6d70-4467-bfbc-6dd19b6c1159
📒 Files selected for processing (11)
backend/src/entities/permission/application/data-structures/available-permissions.ds.tsbackend/src/entities/permission/permission-catalog.builder.tsbackend/src/entities/permission/permission.controller.tsbackend/test/ava-tests/non-saas-tests/non-saas-permission-catalog-e2e.test.tsfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.htmlfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.spec.tsfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.tsfrontend/src/app/lib/cedar-policy-items.tsfrontend/src/app/lib/permission-display.spec.tsfrontend/src/app/lib/permission-display.tsfrontend/src/app/services/users.service.ts
Comment on lines
+65
to
+68
| return PERMISSION_GROUP_ORDER.filter((name) => byGroup.has(name)).map((name) => ({ | ||
| group: name, | ||
| actions: byGroup.get(name)!, | ||
| })); |
There was a problem hiding this comment.
Do not drop unknown permission groups from backend.
Line 65 currently filters groups strictly by PERMISSION_GROUP_ORDER, so any new backend prefix is silently omitted from the UI. That breaks the backend-driven catalog contract and can make valid permissions unassignable. Keep ordered known groups first, then append unknown groups.
Suggested fix
- return PERMISSION_GROUP_ORDER.filter((name) => byGroup.has(name)).map((name) => ({
- group: name,
- actions: byGroup.get(name)!,
- }));
+ const known = PERMISSION_GROUP_ORDER.filter((name) => byGroup.has(name)).map((name) => ({
+ group: name,
+ actions: byGroup.get(name)!,
+ }));
+ const unknown = [...byGroup.entries()]
+ .filter(([name]) => !PERMISSION_GROUP_ORDER.includes(name))
+ .map(([group, actions]) => ({ group, actions }));
+ return [...known, ...unknown];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return PERMISSION_GROUP_ORDER.filter((name) => byGroup.has(name)).map((name) => ({ | |
| group: name, | |
| actions: byGroup.get(name)!, | |
| })); | |
| const known = PERMISSION_GROUP_ORDER.filter((name) => byGroup.has(name)).map((name) => ({ | |
| group: name, | |
| actions: byGroup.get(name)!, | |
| })); | |
| const unknown = [...byGroup.entries()] | |
| .filter(([name]) => !PERMISSION_GROUP_ORDER.includes(name)) | |
| .map(([group, actions]) => ({ group, actions })); | |
| return [...known, ...unknown]; |
🤖 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 `@frontend/src/app/services/users.service.ts` around lines 65 - 68, The current
return drops groups not listed in PERMISSION_GROUP_ORDER; change it to include
known groups in the defined order first and then append any unknown groups from
byGroup. Concretely, build the orderedKnown array by filtering
PERMISSION_GROUP_ORDER for names present in byGroup and mapping to {group,
actions} (same as current), build unknown array by iterating
Array.from(byGroup.keys()) and selecting keys not in PERMISSION_GROUP_ORDER and
mapping them similarly, and finally return [...orderedKnown, ...unknown] so
backend-driven groups are preserved after the known ordered groups.
…-test pollution AppComponent never unsubscribes from authCast, so prior tests' component instances re-fire on cast.next and queue their own setTimeouts, overwriting the captured callback. Gate the capture behind a flag set by THIS app's mocked initializeUserSession so we only grab the timeout from this instance's restoration branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Adds GET /permissions/available, derived from CEDAR_SCHEMA.RocketAdmin.actions so the catalog stays in sync with the Cedar action enum. Replaces the hardcoded POLICY_ACTION_GROUPS list in the frontend with a signal-backed httpResource on UsersService, and switches PolicyAction's needsTable/needsDashboard booleans to a single resource discriminator. New ActionEvent and Panel categories now appear in the Cedar policy editor.
Summary by CodeRabbit
New Features
Refactor
Tests