Improve permissions UI with grouped policies and member previews#1681
Merged
lyubov-voloshko merged 8 commits intorocket-admin:mainfrom Mar 25, 2026
Merged
Conversation
…and better UX Redesign cedar policy list with collapsible groups by category, duplicate detection, used-resource hints, and member avatar previews on group headers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds grouped, collapsible policy list UI and unsaved-change protection to the Cedar policy editor dialog; introduces AISuggestionsService with models and local suggestion generation/history; enhances users UI with group member previews and multiple styling/theme variable updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dialog as Cedar Policy Editor Dialog
participant List as Cedar Policy List Component
participant Confirm as Confirmation Dialog
User->>Dialog: Click Save / Close / backdrop / Escape
Dialog->>List: hasPendingChanges()
alt pending changes
List-->>Dialog: true
Dialog->>Confirm: show discard confirmation
Confirm->>User: prompt "Discard changes?"
alt user confirms
User->>Confirm: confirm
Confirm-->>Dialog: confirmed
Dialog->>List: discardPending()
List-->>Dialog: cleared
Dialog->>Dialog: proceed with save/close
else user cancels
User->>Confirm: cancel
Confirm-->>Dialog: canceled
Dialog->>Dialog: abort action
end
else no pending changes
List-->>Dialog: false
Dialog->>Dialog: proceed with save/close
end
sequenceDiagram
participant Component as Table Component
participant Service as AISuggestionsService
participant Storage as Local Storage / History
Component->>Service: buildTableContext(schema, fks)
Service-->>Component: AISuggestionTableContext
Component->>Service: buildUIContext(filters, selection)
Service-->>Component: AISuggestionUIContext
Component->>Service: buildConversationContext(messages)
Service-->>Component: AISuggestionConversation
Component->>Service: buildSuggestionInput(tableCtx, uiCtx, convCtx)
Service->>Storage: read suggestion_history
Storage-->>Service: recently_shown_ids,recently_clicked_ids
Service-->>Component: AISuggestionInput
Component->>Service: generateLocalSuggestions(input)
Service->>Service: analyze schema, detect intent, build suggestions
Service-->>Component: AISuggestionOutput (table + navigation suggestions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the permissions UX by grouping Cedar policies into collapsible categories, adding group member previews in the user groups list, and introducing an AI suggestions model/service intended to generate contextual table-related prompts.
Changes:
- Redesign Cedar policy list UI: grouped/collapsible sections, per-action icons/short labels, “used resource” hinting in selects, and duplicate prevention on add.
- Add group header member previews (initials avatars + member count) and “system” badge for Admin group.
- Add foundational AI suggestions types and a local suggestion generator service.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/services/users.service.ts | Tweaks policy save snackbar copy. |
| frontend/src/app/services/ai-suggestions.service.ts | Adds AI suggestion context builders + local suggestion generation logic. |
| frontend/src/app/models/ai-suggestions.ts | Introduces types for AI suggestion inputs/outputs. |
| frontend/src/app/components/users/users.component.ts | Adds helpers to fetch group users and derive user initials for previews. |
| frontend/src/app/components/users/users.component.html | Renders “system” badge and member avatar/count previews on group headers; updates tooltip copy. |
| frontend/src/app/components/users/users.component.css | Adjusts expansion-panel header layout and adds styles for member previews/badge. |
| frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts | Adds grouping/collapse state, used-resource maps, duplicate prevention on add, and filtered action groups. |
| frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html | Renders grouped/collapsible policy list and adds “used” styling hooks in selects. |
| frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.css | Adds grouped policy list styling and option hint styling. |
| frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts | Adds unsaved-changes prompt when saving from form mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+33
| <span class="group-avatar" *ngFor="let user of groupUsers.slice(0, 3)" | ||
| [matTooltip]="user.name || user.email"> | ||
| {{ getUserInitials(user) }} |
There was a problem hiding this comment.
Template binds to user.name, but the GroupUser interface (src/app/models/user.ts) does not define a name property. With fullTemplateTypeCheck enabled, this is likely to produce a template type-check error. Consider extending GroupUser to include name?: string (or using the correct user type returned by the API) and avoid relying on untyped properties in the template.
Comment on lines
202
to
+216
| @@ -127,6 +213,7 @@ export class CedarPolicyListComponent { | |||
| this.policies = updated; | |||
| this.policiesChange.emit(this.policies); | |||
| this.editingIndex = null; | |||
| this._refreshViews(); | |||
There was a problem hiding this comment.
Duplicate policy prevention is implemented in addPolicy(), but saveEdit() can still create duplicates (e.g., editing an item to match an existing action/table/dashboard combination). Add a duplicate check in saveEdit() (excluding the edited index) and prevent saving (ideally with user feedback) when a duplicate would be created.
Comment on lines
+9
to
+16
| <div class="policy-group__header" (click)="toggleGroup(group.label)"> | ||
| <div class="policy-group__icon-box policy-group__icon-box--{{ group.colorClass }}"> | ||
| <mat-icon>{{ group.icon }}</mat-icon> | ||
| </div> | ||
| <div class="policy-item__actions"> | ||
| <button mat-icon-button type="button" (click)="startEdit(i)" matTooltip="Edit"> | ||
| <mat-icon>edit</mat-icon> | ||
| </button> | ||
| <button mat-icon-button type="button" (click)="removePolicy(i)" matTooltip="Delete"> | ||
| <mat-icon>delete</mat-icon> | ||
| </button> | ||
| </div> | ||
| </ng-container> | ||
| <span class="policy-group__label">{{ group.label }}</span> | ||
| <span class="policy-group__count">{{ group.policies.length }}</span> | ||
| <mat-icon class="policy-group__chevron" [class.policy-group__chevron--collapsed]="isCollapsed(group.label)">expand_more</mat-icon> | ||
| </div> |
There was a problem hiding this comment.
policy-group__header is a clickable <div> without keyboard interaction or ARIA state, which makes collapsing/expanding groups inaccessible to keyboard and assistive tech users. Use a semantic button (or add role="button", tabindex="0", and handle Enter/Space) and expose aria-expanded / aria-controls for the collapsible region.
Comment on lines
+220
to
+230
| .policy-option--used { | ||
| background: color-mix(in srgb, #3b82f6, transparent 92%); | ||
| } | ||
|
|
||
| .policy-option--used[data-hint]::after { | ||
| content: attr(data-hint); | ||
| font-size: 11px; | ||
| opacity: 0.5; | ||
| margin-left: 8px; | ||
| font-style: italic; | ||
| } |
There was a problem hiding this comment.
Styles for .policy-option--used are defined in the component stylesheet, but mat-select options render in the global overlay container, so these styles typically won't apply under view encapsulation. To make the “used” highlight/hint work, move these rules to a global stylesheet or apply them via ::ng-deep .mat-mdc-select-panel .mat-mdc-option.policy-option--used ... (similar to saved-filters-panel.component.css:246).
Comment on lines
160
to
+165
| savePolicy() { | ||
| if (this.editorMode === 'form' && this.policyList?.hasPendingChanges()) { | ||
| const discard = confirm('You have an unsaved policy in the form. Discard it and save?'); | ||
| if (!discard) return; | ||
| this.policyList.discardPending(); | ||
| } |
There was a problem hiding this comment.
Using window.confirm() here is inconsistent with the rest of the app’s confirmation flows (which use MatDialog confirmation dialogs) and is not themeable/test-friendly. Consider replacing this with a Material dialog-based confirmation, and ensure the message is localized consistently with the rest of the UI.
Comment on lines
+163
to
+178
| // Basic suggestions | ||
| suggestions.push({ | ||
| id: 'recent_rows', | ||
| title: 'Последние записи', | ||
| message: 'Покажи последние 10 записей в таблице', | ||
| confidence: 0.9, | ||
| risk: 'low' | ||
| }); | ||
|
|
||
| suggestions.push({ | ||
| id: 'table_overview', | ||
| title: 'Обзор таблицы', | ||
| message: 'Дай краткий обзор структуры таблицы и основных данных', | ||
| confidence: 0.85, | ||
| risk: 'low' | ||
| }); |
There was a problem hiding this comment.
generateLocalSuggestions() returns user-facing titles/messages in Russian while the surrounding UI appears to be English (e.g., Users/Policy UI). Either localize these strings via the app’s i18n approach or switch to English to avoid mixed-language UX.
Comment on lines
+306
to
+317
| const lastMessage = conversation?.last_user_message?.toLowerCase() || ''; | ||
|
|
||
| if (lastMessage.includes('покажи') || lastMessage.includes('найди') || lastMessage.includes('где')) { | ||
| intents.push('SEARCH'); | ||
| } | ||
| if (lastMessage.includes('сколько') || lastMessage.includes('группир') || lastMessage.includes('распределен')) { | ||
| intents.push('SEGMENT'); | ||
| } | ||
| if (lastMessage.includes('дублик') || lastMessage.includes('null') || lastMessage.includes('пуст')) { | ||
| intents.push('QUALITY'); | ||
| } | ||
| if (lastMessage.includes('обнови') || lastMessage.includes('измени')) { |
There was a problem hiding this comment.
Intent detection currently keys off Russian keywords (e.g., 'покажи', 'сколько'), which won’t work for English queries and will yield incorrect intents for most users if the app locale is English. Consider making intent detection locale-aware or using a language-agnostic heuristic.
Suggested change
| const lastMessage = conversation?.last_user_message?.toLowerCase() || ''; | |
| if (lastMessage.includes('покажи') || lastMessage.includes('найди') || lastMessage.includes('где')) { | |
| intents.push('SEARCH'); | |
| } | |
| if (lastMessage.includes('сколько') || lastMessage.includes('группир') || lastMessage.includes('распределен')) { | |
| intents.push('SEGMENT'); | |
| } | |
| if (lastMessage.includes('дублик') || lastMessage.includes('null') || lastMessage.includes('пуст')) { | |
| intents.push('QUALITY'); | |
| } | |
| if (lastMessage.includes('обнови') || lastMessage.includes('измени')) { | |
| const lastMessageRaw = conversation?.last_user_message || ''; | |
| const lastMessage = lastMessageRaw.toLowerCase(); | |
| const searchKeywords = ['покажи', 'найди', 'где', 'show', 'find', 'search', 'filter']; | |
| const segmentKeywords = ['сколько', 'группир', 'распределен', 'how many', 'count', 'group by', 'segment']; | |
| const qualityKeywords = ['дублик', 'null', 'пуст', 'duplicate', 'duplicates', 'missing', 'empty']; | |
| const updateKeywords = ['обнови', 'измени', 'update', 'change', 'edit', 'modify']; | |
| if (searchKeywords.some(keyword => lastMessage.includes(keyword))) { | |
| intents.push('SEARCH'); | |
| } | |
| if (segmentKeywords.some(keyword => lastMessage.includes(keyword))) { | |
| intents.push('SEGMENT'); | |
| } | |
| if (qualityKeywords.some(keyword => lastMessage.includes(keyword))) { | |
| intents.push('QUALITY'); | |
| } | |
| if (updateKeywords.some(keyword => lastMessage.includes(keyword))) { |
| if (table.relations.length > 0 && ui_context?.selected_row) { | ||
| for (const relation of table.relations.slice(0, 3)) { | ||
| const fkValue = ui_context.selected_row[relation.from_column]; | ||
| if (fkValue) { |
There was a problem hiding this comment.
if (fkValue) will skip valid foreign key values like 0 or empty-string identifiers. Use an explicit null/undefined check so navigation suggestions are generated for all valid values.
Suggested change
| if (fkValue) { | |
| if (fkValue !== null && fkValue !== undefined) { |
Comment on lines
+287
to
+297
| /** | ||
| * Update shown history | ||
| */ | ||
| private updateShownHistory(shownIds: string[]): void { | ||
| const history = this.historySubject.value; | ||
| const shown = [...(history.recently_shown_ids || []), ...shownIds].slice(-20); | ||
| this.historySubject.next({ | ||
| ...history, | ||
| recently_shown_ids: shown | ||
| }); | ||
| } |
There was a problem hiding this comment.
updateShownHistory() is never called, so recently_shown_ids is never updated. This makes suggestion_history incomplete and leaves dead code behind. Either call this when suggestions are displayed/generated or remove the unused method/history field until it’s wired up.
Suggested change
| /** | |
| * Update shown history | |
| */ | |
| private updateShownHistory(shownIds: string[]): void { | |
| const history = this.historySubject.value; | |
| const shown = [...(history.recently_shown_ids || []), ...shownIds].slice(-20); | |
| this.historySubject.next({ | |
| ...history, | |
| recently_shown_ids: shown | |
| }); | |
| } |
Comment on lines
162
to
175
| addPolicy() { | ||
| if (!this.newAction) return; | ||
| if (this.needsTable && !this.newTableName) return; | ||
| if (this.needsDashboard && !this.newDashboardId) return; | ||
|
|
||
| const duplicate = this.policies.some((p) => { | ||
| if (p.action !== this.newAction) return false; | ||
| if (this.needsTable) return p.tableName === this.newTableName; | ||
| if (this.needsDashboard) return p.dashboardId === this.newDashboardId; | ||
| return true; | ||
| }); | ||
| if (duplicate) return; | ||
|
|
||
| const item: CedarPolicyItem = { action: this.newAction }; |
There was a problem hiding this comment.
There are existing unit tests for CedarPolicyListComponent (cedar-policy-list.component.spec.ts), but the newly added behaviors (duplicate prevention, grouping/collapse state, used-table/dashboard hint maps) are not covered. Add tests for duplicate prevention (including via edit) and for _refreshViews() grouping/output to prevent regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts (1)
202-216:⚠️ Potential issue | 🟠 MajorApply the duplicate-policy guard to edits too.
addPolicy()blocks duplicates, butsaveEdit()writes the edited item back unconditionally. Editing an entry to match anotheraction + resourcepair reintroduces the duplicates this PR is supposed to prevent.Suggested fix
saveEdit(index: number) { if (!this.editAction) return; if (this.editNeedsTable && !this.editTableName) return; if (this.editNeedsDashboard && !this.editDashboardId) return; + const nextItem: CedarPolicyItem = { + action: this.editAction, + tableName: this.editNeedsTable ? this.editTableName : undefined, + dashboardId: this.editNeedsDashboard ? this.editDashboardId : undefined, + }; + + const duplicate = this.policies.some((policy, policyIndex) => { + if (policyIndex === index || policy.action !== nextItem.action) return false; + if (nextItem.tableName) return policy.tableName === nextItem.tableName; + if (nextItem.dashboardId) return policy.dashboardId === nextItem.dashboardId; + return true; + }); + if (duplicate) return; + const updated = [...this.policies]; - updated[index] = { - action: this.editAction, - tableName: this.editNeedsTable ? this.editTableName : undefined, - dashboardId: this.editNeedsDashboard ? this.editDashboardId : undefined, - }; + updated[index] = nextItem; this.policies = updated;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts` around lines 202 - 216, saveEdit currently overwrites the policy at index without checking for duplicates; replicate the duplicate guard used in addPolicy by constructing the new policy object (use action: this.editAction, tableName: this.editNeedsTable ? this.editTableName : undefined, dashboardId: this.editNeedsDashboard ? this.editDashboardId : undefined), then scan this.policies but skip the current index to detect any existing policy with the same action + resource (same tableName or dashboardId); if a duplicate exists, abort the save (or surface the same validation/error path addPolicy uses), otherwise apply the update to updated[index], emit policiesChange, clear editingIndex and call this._refreshViews().
🧹 Nitpick comments (12)
frontend/src/app/components/users/users.component.html (2)
30-36: Use@ifand@forwithtrackfor better performance.The new member preview code uses structural directives, but coding guidelines require built-in control flow syntax for new code. Additionally, calling
getGroupUsers()directly in the template will re-execute on every change detection cycle.♻️ Suggested refactor
- <span class="group-members-preview" *ngIf="getGroupUsers(groupItem.group.id) as groupUsers"> - <span class="group-avatar" *ngFor="let user of groupUsers.slice(0, 3)" - [matTooltip]="user.name || user.email"> - {{ getUserInitials(user) }} - </span> - <span class="group-members-count">{{ groupUsers.length }} {{ groupUsers.length === 1 ? 'member' : 'members' }}</span> - </span> + `@if` (getGroupUsers(groupItem.group.id); as groupUsers) { + <span class="group-members-preview"> + `@for` (user of groupUsers.slice(0, 3); track user.id) { + <span class="group-avatar" [matTooltip]="user.name || user.email"> + {{ getUserInitials(user) }} + </span> + } + <span class="group-members-count">{{ groupUsers.length }} {{ groupUsers.length === 1 ? 'member' : 'members' }}</span> + </span> + }Consider caching the result of
getGroupUsers()in the component to avoid repeated lookups during change detection, or use acomputed()signal for derived state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/users.component.html` around lines 30 - 36, Replace the inline call to getGroupUsers(groupItem.group.id) and the *ngFor with the framework's control-flow directives and a cached/computed value: compute or cache group users in the component (e.g., a map or a computed/signal method like getCachedGroupUsers(groupId) or groupUsersMap[groupId]) and expose that property for the template, then use the template `@if` to bind the cached array (e.g., "let groupUsers = cachedGroupUsers[groupId]") and `@for` with a track function (implement trackByUserId(user) in the component that returns user.id) to render the first three users and the members count; keep getUserInitials(user) as-is but ensure the cached lookup replaces direct calls to getGroupUsers in the template to avoid repeated change-detection execution.
21-21: New code should use@ifinstead of*ngIf.As per coding guidelines, new code should use Angular's built-in control flow syntax. Consider updating to:
`@if` (groupItem.group.title === 'Admin') { <span class="group-system-badge">system</span> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/users.component.html` at line 21, Replace the template's structural directive usage: remove the old "*ngIf" expression on the span that checks "groupItem.group.title === 'Admin'" and rewrite it using the new Angular control flow syntax with "@if (groupItem.group.title === 'Admin') { ... }", keeping the same span element and its class "group-system-badge" and preserving the displayed text "system"; ensure the closing brace matches the template block so the span only renders for Admin groups.frontend/src/app/components/users/users.component.css (2)
107-113: Dark mode styles look good, but consider using CSS custom properties for consistency.The dark mode override uses hardcoded colors (
rgba(99, 102, 241, 0.2),#a5b4fc). For better maintainability with Material theming, consider using CSS custom properties that align with your theme's color palette.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/users.component.css` around lines 107 - 113, The dark-mode rule for .group-avatar uses hardcoded colors; replace those literals with theme CSS custom properties (e.g., use var(--app-primary-rgba, rgba(99,102,241,0.2)) for background, var(--app-primary-contrast, `#a5b4fc`) for color, and keep the existing var(...) for border-color with a sensible fallback) so the .group-avatar selector inside the `@media` (prefers-color-scheme: dark) block reads from your theme variables and still falls back to the current hardcoded values if the variables are not defined.
25-32:::ng-deepis deprecated but acceptable here.The Stylelint error flagging
::ng-deepas an unknown pseudo-element is a false positive — it's Angular-specific. However,::ng-deepis deprecated in Angular. While it still works, consider adding a comment noting this technical debt, or migrating to::part()/ global styles instyles.cssin a future refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/users.component.css` around lines 25 - 32, The CSS uses the deprecated Angular pseudo-element ::ng-deep in the selector "::ng-deep .mat-expansion-panel-header"; update the file to annotate this technical debt by adding an inline comment above that selector noting ::ng-deep is deprecated and should be replaced in a future refactor (suggest migrating the styles to global styles.css or switching to ::part() where applicable), or move the rules for .mat-expansion-panel-header into global styles to avoid ::ng-deep; keep the existing rules (height, min-height, padding) unchanged so behavior is preserved.frontend/src/app/models/ai-suggestions.ts (2)
34-38: Avoidanytype forvalueproperty.The
value: anyweakens type safety. Consider using a union type or generic to preserve type information.🔧 Suggested improvement
-export interface AISuggestionFilter { - column: string; - op: string; - value: any; -} +export interface AISuggestionFilter<T = string | number | boolean | null> { + column: string; + op: string; + value: T; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/models/ai-suggestions.ts` around lines 34 - 38, The AISuggestionFilter interface uses value: any which weakens type safety; update the interface AISuggestionFilter to replace value: any with a safer type—either make the interface generic (e.g., AISuggestionFilter<T>) so value: T, or restrict value to a union of expected types (e.g., string | number | boolean | Date | string[]), and update call sites that construct AISuggestionFilter to supply the concrete type or conform to the union.
40-44: Avoidanytype forselected_row.
Record<string, any>loses type safety for row values. Consider using a generic or a more specific union type.As per coding guidelines: "Avoid any types - use specific types or generics instead".
🔧 Suggested improvement
-export interface AISuggestionUIContext { +export interface AISuggestionUIContext<TRow = Record<string, unknown>> { active_filters?: AISuggestionFilter[]; - selected_row?: Record<string, any>; + selected_row?: TRow; selected_column?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/models/ai-suggestions.ts` around lines 40 - 44, The selected_row property currently uses Record<string, any>, which loses type safety; change AISuggestionUIContext to be generic (e.g., interface AISuggestionUIContext<T = Record<string, unknown>> { active_filters?: AISuggestionFilter[]; selected_row?: T; selected_column?: string; }) or replace any with a narrower union (e.g., Record<string, string | number | boolean | null | undefined>) and then update all usages of AISuggestionUIContext/selected_row to supply the concrete type parameter or adapt to the narrower union to restore type safety.frontend/src/app/services/ai-suggestions.service.ts (5)
21-27: Private members should be prefixed with underscore.The private
BehaviorSubjectmembers should follow the underscore prefix convention for private members.As per coding guidelines: "Prefix private members with underscore (e.g.,
_privateMethod,_http)".🔧 Suggested fix
- private suggestionsSubject = new BehaviorSubject<AISuggestionOutput | null>(null); - public suggestions$ = this.suggestionsSubject.asObservable(); + private _suggestionsSubject = new BehaviorSubject<AISuggestionOutput | null>(null); + public suggestions$ = this._suggestionsSubject.asObservable(); - private historySubject = new BehaviorSubject<AISuggestionHistory>({ + private _historySubject = new BehaviorSubject<AISuggestionHistory>({ recently_shown_ids: [], recently_clicked_ids: [] });Note: After renaming, update all references to these subjects throughout the class (e.g.,
this._historySubject.value,this._historySubject.next(...)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/ai-suggestions.service.ts` around lines 21 - 27, Rename the private BehaviorSubject members to use the underscore prefix: change suggestionsSubject -> _suggestionsSubject and historySubject -> _historySubject, keep the public suggestions$ observable but rewire it to this._suggestionsSubject.asObservable(), and update all usages inside the class (reads like this._suggestionsSubject.value and writes like this._suggestionsSubject.next(...), as well as any references to historySubject) to the new names so the class follows the private-member naming convention.
328-352: Add explicit return type and type the array.The method lacks an explicit return type annotation, and
suggestionsis an implicitly typedany[]array. This weakens type safety.As per coding guidelines: "Always add type annotations to function parameters and return types in TypeScript".
🔧 Suggested fix
- private _generateNavigationSuggestions(input: AISuggestionInput) { + private _generateNavigationSuggestions(input: AISuggestionInput): AINavigationSuggestion[] { const { table, ui_context } = input; - const suggestions = []; + const suggestions: AINavigationSuggestion[] = [];Note: You'll need to import
AINavigationSuggestionfrom the models file (it's currently not imported).import { AISuggestionInput, AISuggestionOutput, AISuggestionTableContext, AISuggestionColumn, AISuggestionRelation, AISuggestionUIContext, AISuggestionConversation, AISuggestionHistory, AITableSuggestion, + AINavigationSuggestion, } from '../models/ai-suggestions';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/ai-suggestions.service.ts` around lines 328 - 352, _Add an explicit return type and strongly type the suggestions array in _generateNavigationSuggestions: change the method signature to return AINavigationSuggestion[] and declare suggestions as AINavigationSuggestion[] (or ReadonlyArray if appropriate), then import AINavigationSuggestion from the models file at the top of the module; ensure all pushed objects conform to the AINavigationSuggestion shape (fields like id, title, target_table, prefilled_query, why, confidence) so TypeScript verifies correctness._
29-29: Remove empty constructor.The empty constructor serves no purpose and can be omitted in Angular services using
providedIn: 'root'.🔧 Suggested fix
- constructor() {} - /** * Build table context from existing table data */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/ai-suggestions.service.ts` at line 29, The empty constructor in the AiSuggestionsService class (constructor()) is unnecessary for an Angular service providedIn: 'root'; remove the constructor declaration from the class definition to clean up the code and rely on the default constructor behavior.
126-263: Consider extracting suggestion generators for maintainability.The
generateLocalSuggestionsmethod is over 130 lines long. Consider extracting the column-type detection and suggestion-building logic into smaller private helper methods to improve readability and testability.For example, you could extract:
_findDateColumns(columns)_findStatusColumns(columns)_buildDateTrendSuggestion(column)_buildStatusDistributionSuggestion(column)This would make the main method a high-level orchestrator that's easier to follow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/ai-suggestions.service.ts` around lines 126 - 263, generateLocalSuggestions is too large and mixes column detection with suggestion construction; refactor by extracting the detection and builder logic into private helpers (e.g., implement _findDateColumns(columns), _findStatusColumns(columns), _findIdentifierColumns(columns), _findNumericColumns(columns)) and suggestion factories (e.g., _buildDateTrendSuggestion(column), _buildStatusDistributionSuggestion(column), _buildDuplicateSuggestion(column), _buildNumericStatsSuggestion(column), _buildNullsOverviewSuggestion(columns)); have generateLocalSuggestions call these helpers to collect columns and push returned suggestion objects, preserving existing ids/titles/messages and returning intent via this._detectIntent and navigation via this._generateNavigationSuggestions so behavior remains unchanged.
69-87: Consider adding explicit return type annotation.While the return type is inferred, adding an explicit annotation improves readability and ensures the method contract is clear.
🔧 Minor improvement
buildUIContext( activeFilters?: Record<string, any>, selectedRow?: Record<string, any>, selectedColumn?: string - ): AISuggestionUIContext { + ): AISuggestionUIContext { // Already correct - ignore this suggestionThe return type is already correctly annotated. The method is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/ai-suggestions.service.ts` around lines 69 - 87, The buildUIContext method should have an explicit return type of AISuggestionUIContext; confirm the function signature for buildUIContext includes ": AISuggestionUIContext" and if it were missing add that explicit annotation so the method contract is clear (the current code already has this annotation, so no change is required).frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts (1)
161-165: Use a Material confirmation flow instead ofconfirm().The new unsaved-changes prompt bypasses the app’s dialog stack, so it won’t inherit the editor’s M2 styling, dark-mode behavior, or the same interaction hooks as the rest of this screen. Please route this through a Material dialog/confirmation service instead.
Based on learnings, "Use Material Design 2 (M2) APIs for UI components and custom theming".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts` around lines 161 - 165, Replace the window.confirm flow in cedar-policy-editor-dialog.component (the block checking this.editorMode === 'form' && this.policyList?.hasPendingChanges()) with the app's Material confirmation dialog instead of confirm(); call the shared/Material confirmation service or MatDialog to open a confirmation dialog, await the user's response, and only call this.policyList.discardPending() when the user confirms; ensure the dialog uses existing M2 theming and interaction hooks used across the app and remove the direct use of confirm().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html`:
- Around line 9-16: Replace the non-interactive div with a semantic button for
the collapsible header: change the element with class "policy-group__header" to
a <button type="button"> while keeping the (click)="toggleGroup(group.label)"
handler, preserve inner content (icon, label, count, chevron), and add
[attr.aria-expanded]="!isCollapsed(group.label)" to announce state; ensure CSS
selectors referencing .policy-group__header still apply or update them to target
the button if needed so toggleGroup and isCollapsed continue to work as before.
- Around line 8-90: The template uses structural directives (*ngFor/*ngIf) for
the grouped policy UI; replace them with the repo's built-in control flow syntax
using `@for` and `@if` while preserving tracking keys and bindings: iterate outer
loop with "@for (group of groupedPolicies; track group.label)" and inner loop
with "@for (entry of group.policies; track entry.originalIndex)"; replace
"*ngIf=\"!isCollapsed(group.label)\"" and the edit-mode checks ("editingIndex
!== entry.originalIndex" / "editingIndex === entry.originalIndex") with
corresponding "@if" blocks; keep existing identifiers used by trackByGroup,
trackByPolicy, getActionIcon/getActionLabel/getShortActionLabel, and preserve
mat-select option tracking/value usage (action.value) and all [(ngModel)]
bindings (editAction, editTableName, editDashboardId) and buttons
(startEdit/saveEdit/removePolicy/cancelEdit) exactly as-is.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`:
- Around line 188-191: When removing a policy in removePolicy(index: number)
adjust the editor state first to avoid stale editingIndex references: if there
is an active edit (editingIndex is not null/undefined) and the removed index is
before the current editingIndex, decrement editingIndex accordingly; if the
removed index equals the editingIndex or the editor has pending changes
(hasPendingChanges in cedar-policy-editor-dialog.component.ts), clear/cancel the
edit state (reset editingIndex and any edit buffer) before mutating
this.policies, then emit policiesChange and call _refreshViews() so the editor
always maps to the correct policy or is closed when needed.
- Around line 62-77: The component currently keeps UI state in plain properties
(groupedPolicies, addActionGroups, editActionGroups, usedTables, usedDashboards)
and manually updates them in ngOnChanges()/_refreshViews() and startEdit(),
causing possible template drift; convert relevant state to Angular Signals and
derived computed() values: make the input/writable policies a signal (e.g.,
policiesSignal) and remove ngOnChanges()/_refreshViews(); implement computed()
for groupedPolicies, usedTables, usedDashboards based on policiesSignal and any
static config; implement computed() for addActionGroups and editActionGroups
that also read edit context (e.g., activeEditId or editContext signal) instead
of mutating arrays in startEdit(); update startEdit() to set the edit-context
signal rather than mutating editActionGroups directly and remove manual Map
mutations, ensuring the template reads the computed signals.
In `@frontend/src/app/components/users/users.component.html`:
- Line 32: The template uses user.name fallback on a GroupUser which doesn't
have name; update the matTooltip binding in users.component.html so the
[matTooltip] uses user.email only (replace the expression that references
user.name with user.email) — locate the matTooltip attribute on the user
row/item where the template variable is named user and change it to use
user.email.
In `@frontend/src/app/components/users/users.component.ts`:
- Around line 187-196: The getUserInitials function can throw on empty strings
and uses an any cast; change it to avoid unsafe indexing by refining the type
(extend GroupUser or create a view-specific type that includes optional name:
string | undefined) instead of casting to any, then defensively handle
empty/whitespace values: check that name is a non-empty string after trim, build
parts = name.trim().split(/\s+/).filter(Boolean) and ensure parts[0] and
parts[0][0] exist before accessing them; if name is absent or yields no valid
characters, fall back to a safe email fallback by verifying user.email is a
non-empty string and using user.email[0].toUpperCase(), otherwise return a
sensible default (e.g., '?').
In `@frontend/src/app/services/ai-suggestions.service.ts`:
- Around line 287-297: Remove the dead private method updateShownHistory from
ai-suggestions.service.ts: delete the updateShownHistory(shownIds: string[]):
void { ... } implementation and any direct references (none expected) and ensure
historySubject usage elsewhere remains intact; run a quick project-wide search
for "updateShownHistory" to confirm no callers exist and remove any leftover
imports or comments that were only used by this method.
---
Outside diff comments:
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`:
- Around line 202-216: saveEdit currently overwrites the policy at index without
checking for duplicates; replicate the duplicate guard used in addPolicy by
constructing the new policy object (use action: this.editAction, tableName:
this.editNeedsTable ? this.editTableName : undefined, dashboardId:
this.editNeedsDashboard ? this.editDashboardId : undefined), then scan
this.policies but skip the current index to detect any existing policy with the
same action + resource (same tableName or dashboardId); if a duplicate exists,
abort the save (or surface the same validation/error path addPolicy uses),
otherwise apply the update to updated[index], emit policiesChange, clear
editingIndex and call this._refreshViews().
---
Nitpick comments:
In
`@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts`:
- Around line 161-165: Replace the window.confirm flow in
cedar-policy-editor-dialog.component (the block checking this.editorMode ===
'form' && this.policyList?.hasPendingChanges()) with the app's Material
confirmation dialog instead of confirm(); call the shared/Material confirmation
service or MatDialog to open a confirmation dialog, await the user's response,
and only call this.policyList.discardPending() when the user confirms; ensure
the dialog uses existing M2 theming and interaction hooks used across the app
and remove the direct use of confirm().
In `@frontend/src/app/components/users/users.component.css`:
- Around line 107-113: The dark-mode rule for .group-avatar uses hardcoded
colors; replace those literals with theme CSS custom properties (e.g., use
var(--app-primary-rgba, rgba(99,102,241,0.2)) for background,
var(--app-primary-contrast, `#a5b4fc`) for color, and keep the existing var(...)
for border-color with a sensible fallback) so the .group-avatar selector inside
the `@media` (prefers-color-scheme: dark) block reads from your theme variables
and still falls back to the current hardcoded values if the variables are not
defined.
- Around line 25-32: The CSS uses the deprecated Angular pseudo-element
::ng-deep in the selector "::ng-deep .mat-expansion-panel-header"; update the
file to annotate this technical debt by adding an inline comment above that
selector noting ::ng-deep is deprecated and should be replaced in a future
refactor (suggest migrating the styles to global styles.css or switching to
::part() where applicable), or move the rules for .mat-expansion-panel-header
into global styles to avoid ::ng-deep; keep the existing rules (height,
min-height, padding) unchanged so behavior is preserved.
In `@frontend/src/app/components/users/users.component.html`:
- Around line 30-36: Replace the inline call to
getGroupUsers(groupItem.group.id) and the *ngFor with the framework's
control-flow directives and a cached/computed value: compute or cache group
users in the component (e.g., a map or a computed/signal method like
getCachedGroupUsers(groupId) or groupUsersMap[groupId]) and expose that property
for the template, then use the template `@if` to bind the cached array (e.g., "let
groupUsers = cachedGroupUsers[groupId]") and `@for` with a track function
(implement trackByUserId(user) in the component that returns user.id) to render
the first three users and the members count; keep getUserInitials(user) as-is
but ensure the cached lookup replaces direct calls to getGroupUsers in the
template to avoid repeated change-detection execution.
- Line 21: Replace the template's structural directive usage: remove the old
"*ngIf" expression on the span that checks "groupItem.group.title === 'Admin'"
and rewrite it using the new Angular control flow syntax with "@if
(groupItem.group.title === 'Admin') { ... }", keeping the same span element and
its class "group-system-badge" and preserving the displayed text "system";
ensure the closing brace matches the template block so the span only renders for
Admin groups.
In `@frontend/src/app/models/ai-suggestions.ts`:
- Around line 34-38: The AISuggestionFilter interface uses value: any which
weakens type safety; update the interface AISuggestionFilter to replace value:
any with a safer type—either make the interface generic (e.g.,
AISuggestionFilter<T>) so value: T, or restrict value to a union of expected
types (e.g., string | number | boolean | Date | string[]), and update call sites
that construct AISuggestionFilter to supply the concrete type or conform to the
union.
- Around line 40-44: The selected_row property currently uses Record<string,
any>, which loses type safety; change AISuggestionUIContext to be generic (e.g.,
interface AISuggestionUIContext<T = Record<string, unknown>> { active_filters?:
AISuggestionFilter[]; selected_row?: T; selected_column?: string; }) or replace
any with a narrower union (e.g., Record<string, string | number | boolean | null
| undefined>) and then update all usages of AISuggestionUIContext/selected_row
to supply the concrete type parameter or adapt to the narrower union to restore
type safety.
In `@frontend/src/app/services/ai-suggestions.service.ts`:
- Around line 21-27: Rename the private BehaviorSubject members to use the
underscore prefix: change suggestionsSubject -> _suggestionsSubject and
historySubject -> _historySubject, keep the public suggestions$ observable but
rewire it to this._suggestionsSubject.asObservable(), and update all usages
inside the class (reads like this._suggestionsSubject.value and writes like
this._suggestionsSubject.next(...), as well as any references to historySubject)
to the new names so the class follows the private-member naming convention.
- Around line 328-352: _Add an explicit return type and strongly type the
suggestions array in _generateNavigationSuggestions: change the method signature
to return AINavigationSuggestion[] and declare suggestions as
AINavigationSuggestion[] (or ReadonlyArray if appropriate), then import
AINavigationSuggestion from the models file at the top of the module; ensure all
pushed objects conform to the AINavigationSuggestion shape (fields like id,
title, target_table, prefilled_query, why, confidence) so TypeScript verifies
correctness._
- Line 29: The empty constructor in the AiSuggestionsService class
(constructor()) is unnecessary for an Angular service providedIn: 'root'; remove
the constructor declaration from the class definition to clean up the code and
rely on the default constructor behavior.
- Around line 126-263: generateLocalSuggestions is too large and mixes column
detection with suggestion construction; refactor by extracting the detection and
builder logic into private helpers (e.g., implement _findDateColumns(columns),
_findStatusColumns(columns), _findIdentifierColumns(columns),
_findNumericColumns(columns)) and suggestion factories (e.g.,
_buildDateTrendSuggestion(column), _buildStatusDistributionSuggestion(column),
_buildDuplicateSuggestion(column), _buildNumericStatsSuggestion(column),
_buildNullsOverviewSuggestion(columns)); have generateLocalSuggestions call
these helpers to collect columns and push returned suggestion objects,
preserving existing ids/titles/messages and returning intent via
this._detectIntent and navigation via this._generateNavigationSuggestions so
behavior remains unchanged.
- Around line 69-87: The buildUIContext method should have an explicit return
type of AISuggestionUIContext; confirm the function signature for buildUIContext
includes ": AISuggestionUIContext" and if it were missing add that explicit
annotation so the method contract is clear (the current code already has this
annotation, so no change is required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e82272d-45a5-42ad-8495-d2ac94c521bb
⛔ Files ignored due to path filters (1)
frontend/src/assets/app.icois excluded by!**/*.ico
📒 Files selected for processing (10)
frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.tsfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.cssfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.htmlfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.tsfrontend/src/app/components/users/users.component.cssfrontend/src/app/components/users/users.component.htmlfrontend/src/app/components/users/users.component.tsfrontend/src/app/models/ai-suggestions.tsfrontend/src/app/services/ai-suggestions.service.tsfrontend/src/app/services/users.service.ts
Comment on lines
+8
to
+90
| <div *ngFor="let group of groupedPolicies; trackBy: trackByGroup" class="policy-group"> | ||
| <div class="policy-group__header" (click)="toggleGroup(group.label)"> | ||
| <div class="policy-group__icon-box policy-group__icon-box--{{ group.colorClass }}"> | ||
| <mat-icon>{{ group.icon }}</mat-icon> | ||
| </div> | ||
| <div class="policy-item__actions"> | ||
| <button mat-icon-button type="button" (click)="startEdit(i)" matTooltip="Edit"> | ||
| <mat-icon>edit</mat-icon> | ||
| </button> | ||
| <button mat-icon-button type="button" (click)="removePolicy(i)" matTooltip="Delete"> | ||
| <mat-icon>delete</mat-icon> | ||
| </button> | ||
| </div> | ||
| </ng-container> | ||
| <span class="policy-group__label">{{ group.label }}</span> | ||
| <span class="policy-group__count">{{ group.policies.length }}</span> | ||
| <mat-icon class="policy-group__chevron" [class.policy-group__chevron--collapsed]="isCollapsed(group.label)">expand_more</mat-icon> | ||
| </div> | ||
|
|
||
| <!-- Edit mode --> | ||
| <ng-container *ngIf="editingIndex === i"> | ||
| <div class="policy-item__edit-form"> | ||
| <mat-form-field appearance="outline" class="policy-field"> | ||
| <mat-label>Action</mat-label> | ||
| <mat-select [(ngModel)]="editAction" [ngModelOptions]="{standalone: true}"> | ||
| <mat-optgroup *ngFor="let group of actionGroups" [label]="group.group"> | ||
| <mat-option *ngFor="let action of group.actions" [value]="action.value"> | ||
| {{ action.label }} | ||
| </mat-option> | ||
| </mat-optgroup> | ||
| </mat-select> | ||
| </mat-form-field> | ||
| <div class="policy-group__items" *ngIf="!isCollapsed(group.label)"> | ||
| <div *ngFor="let entry of group.policies; trackBy: trackByPolicy" class="policy-item"> | ||
| <!-- Display mode --> | ||
| <ng-container *ngIf="editingIndex !== entry.originalIndex"> | ||
| <div class="policy-item__content"> | ||
| <mat-icon class="policy-item__action-icon" [matTooltip]="getActionLabel(entry.item.action)">{{ getActionIcon(entry.item.action) }}</mat-icon> | ||
| <span class="policy-item__label">{{ getShortActionLabel(entry.item.action) }}</span> | ||
| <span *ngIf="entry.item.tableName" class="policy-item__table"> | ||
| {{ getTableDisplayName(entry.item.tableName) }} | ||
| </span> | ||
| <span *ngIf="entry.item.dashboardId" class="policy-item__table"> | ||
| {{ getDashboardDisplayName(entry.item.dashboardId) }} | ||
| </span> | ||
| </div> | ||
| <div class="policy-item__actions"> | ||
| <button mat-icon-button type="button" (click)="startEdit(entry.originalIndex)" matTooltip="Edit"> | ||
| <mat-icon>edit</mat-icon> | ||
| </button> | ||
| <button mat-icon-button type="button" (click)="removePolicy(entry.originalIndex)" matTooltip="Delete"> | ||
| <mat-icon>delete</mat-icon> | ||
| </button> | ||
| </div> | ||
| </ng-container> | ||
|
|
||
| <mat-form-field *ngIf="editNeedsTable" appearance="outline" class="policy-field"> | ||
| <mat-label>Table</mat-label> | ||
| <mat-select [(ngModel)]="editTableName" [ngModelOptions]="{standalone: true}"> | ||
| <mat-option value="*">All tables</mat-option> | ||
| <mat-option *ngFor="let table of availableTables" [value]="table.tableName"> | ||
| {{ table.displayName }} | ||
| </mat-option> | ||
| </mat-select> | ||
| </mat-form-field> | ||
| <!-- Edit mode --> | ||
| <ng-container *ngIf="editingIndex === entry.originalIndex"> | ||
| <div class="policy-item__edit-form"> | ||
| <mat-form-field appearance="outline" class="policy-field"> | ||
| <mat-label>Action</mat-label> | ||
| <mat-select [(ngModel)]="editAction" [ngModelOptions]="{standalone: true}"> | ||
| <mat-optgroup *ngFor="let actionGroup of editActionGroups; trackBy: trackByActionGroup" [label]="actionGroup.group"> | ||
| <mat-option *ngFor="let action of actionGroup.actions; trackBy: trackByAction" [value]="action.value"> | ||
| {{ action.label }} | ||
| </mat-option> | ||
| </mat-optgroup> | ||
| </mat-select> | ||
| </mat-form-field> | ||
|
|
||
| <mat-form-field *ngIf="editNeedsDashboard" appearance="outline" class="policy-field"> | ||
| <mat-label>Dashboard</mat-label> | ||
| <mat-select [(ngModel)]="editDashboardId" [ngModelOptions]="{standalone: true}"> | ||
| <mat-option value="*">All dashboards</mat-option> | ||
| <mat-option *ngFor="let dashboard of availableDashboards" [value]="dashboard.id"> | ||
| {{ dashboard.name }} | ||
| </mat-option> | ||
| </mat-select> | ||
| </mat-form-field> | ||
| <mat-form-field *ngIf="editNeedsTable" appearance="outline" class="policy-field"> | ||
| <mat-label>Table</mat-label> | ||
| <mat-select [(ngModel)]="editTableName" [ngModelOptions]="{standalone: true}"> | ||
| <mat-option value="*">All tables</mat-option> | ||
| <mat-option *ngFor="let table of availableTables" [value]="table.tableName" | ||
| [class.policy-option--used]="usedTables.has(table.tableName)" | ||
| [attr.data-hint]="getTableUsedHint(table.tableName) || null"> | ||
| {{ table.displayName }} | ||
| </mat-option> | ||
| </mat-select> | ||
| </mat-form-field> | ||
|
|
||
| <div class="policy-item__edit-actions"> | ||
| <button mat-button color="primary" type="button" (click)="saveEdit(i)" | ||
| [disabled]="!editAction || (editNeedsTable && !editTableName) || (editNeedsDashboard && !editDashboardId)"> | ||
| Save | ||
| </button> | ||
| <button mat-button type="button" (click)="cancelEdit()">Cancel</button> | ||
| </div> | ||
| <mat-form-field *ngIf="editNeedsDashboard" appearance="outline" class="policy-field"> | ||
| <mat-label>Dashboard</mat-label> | ||
| <mat-select [(ngModel)]="editDashboardId" [ngModelOptions]="{standalone: true}"> | ||
| <mat-option value="*">All dashboards</mat-option> | ||
| <mat-option *ngFor="let dashboard of availableDashboards" [value]="dashboard.id" | ||
| [class.policy-option--used]="usedDashboards.has(dashboard.id)" | ||
| [attr.data-hint]="getDashboardUsedHint(dashboard.id) || null"> | ||
| {{ dashboard.name }} | ||
| </mat-option> | ||
| </mat-select> | ||
| </mat-form-field> | ||
|
|
||
| <div class="policy-item__edit-actions"> | ||
| <button mat-button color="primary" type="button" (click)="saveEdit(entry.originalIndex)" | ||
| [disabled]="!editAction || (editNeedsTable && !editTableName) || (editNeedsDashboard && !editDashboardId)"> | ||
| Save | ||
| </button> | ||
| <button mat-button type="button" (click)="cancelEdit()">Cancel</button> | ||
| </div> | ||
| </div> | ||
| </ng-container> | ||
| </div> | ||
| </ng-container> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get its total line count
if [ -f "frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html" ]; then
echo "File exists"
wc -l "frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html"
else
echo "File not found at expected path"
# Try to find it
find . -name "cedar-policy-list.component.html" -type f 2>/dev/null
fiRepository: rocket-admin/rocketadmin
Length of output: 168
🏁 Script executed:
cat -n "frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html"Repository: rocket-admin/rocketadmin
Length of output: 9706
Convert the new grouped template blocks to @for / @if.
This grouped-policy UI was added with *ngFor/*ngIf, but the repo template rules require built-in control flow for new code. When you switch, preserve the existing tracking keys (group.label, entry.originalIndex, action values).
As per coding guidelines: "Use built-in control flow syntax (@if, @for, @switch) instead of structural directives (*ngIf, *ngFor, *ngSwitch) in all new code" and "Use track function in @for loops for better performance (e.g., @for (item of items; track item.id))".
Applies to lines 8–90 and 99–128.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html`
around lines 8 - 90, The template uses structural directives (*ngFor/*ngIf) for
the grouped policy UI; replace them with the repo's built-in control flow syntax
using `@for` and `@if` while preserving tracking keys and bindings: iterate outer
loop with "@for (group of groupedPolicies; track group.label)" and inner loop
with "@for (entry of group.policies; track entry.originalIndex)"; replace
"*ngIf=\"!isCollapsed(group.label)\"" and the edit-mode checks ("editingIndex
!== entry.originalIndex" / "editingIndex === entry.originalIndex") with
corresponding "@if" blocks; keep existing identifiers used by trackByGroup,
trackByPolicy, getActionIcon/getActionLabel/getShortActionLabel, and preserve
mat-select option tracking/value usage (action.value) and all [(ngModel)]
bindings (editAction, editTableName, editDashboardId) and buttons
(startEdit/saveEdit/removePolicy/cancelEdit) exactly as-is.
Comment on lines
+9
to
+16
| <div class="policy-group__header" (click)="toggleGroup(group.label)"> | ||
| <div class="policy-group__icon-box policy-group__icon-box--{{ group.colorClass }}"> | ||
| <mat-icon>{{ group.icon }}</mat-icon> | ||
| </div> | ||
| <div class="policy-item__actions"> | ||
| <button mat-icon-button type="button" (click)="startEdit(i)" matTooltip="Edit"> | ||
| <mat-icon>edit</mat-icon> | ||
| </button> | ||
| <button mat-icon-button type="button" (click)="removePolicy(i)" matTooltip="Delete"> | ||
| <mat-icon>delete</mat-icon> | ||
| </button> | ||
| </div> | ||
| </ng-container> | ||
| <span class="policy-group__label">{{ group.label }}</span> | ||
| <span class="policy-group__count">{{ group.policies.length }}</span> | ||
| <mat-icon class="policy-group__chevron" [class.policy-group__chevron--collapsed]="isCollapsed(group.label)">expand_more</mat-icon> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.htmlRepository: rocket-admin/rocketadmin
Length of output: 9706
Convert the collapsible group header to a <button> element for accessibility.
The <div> at line 9 with a click handler lacks keyboard semantics and cannot be accessed via keyboard navigation. Using <button type="button"> will provide proper keyboard support and allow assistive technologies to understand this as an interactive control. Additionally, add [attr.aria-expanded]="!isCollapsed(group.label)" to announce the expanded/collapsed state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html`
around lines 9 - 16, Replace the non-interactive div with a semantic button for
the collapsible header: change the element with class "policy-group__header" to
a <button type="button"> while keeping the (click)="toggleGroup(group.label)"
handler, preserve inner content (icon, label, count, chevron), and add
[attr.aria-expanded]="!isCollapsed(group.label)" to announce state; ensure CSS
selectors referencing .policy-group__header still apply or update them to target
the button if needed so toggleGroup and isCollapsed continue to work as before.
Comment on lines
+62
to
+77
| collapsedGroups = new Set<string>(); | ||
|
|
||
| availableActions = POLICY_ACTIONS; | ||
| actionGroups = POLICY_ACTION_GROUPS; | ||
|
|
||
| groupedPolicies: PolicyGroup[] = []; | ||
| addActionGroups: PolicyActionGroup[] = []; | ||
| editActionGroups: PolicyActionGroup[] = []; | ||
|
|
||
| usedTables = new Map<string, string[]>(); | ||
| usedDashboards = new Map<string, string[]>(); | ||
|
|
||
| ngOnChanges(changes: SimpleChanges): void { | ||
| if (changes['policies']) { | ||
| this._refreshViews(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts | head -100Repository: rocket-admin/rocketadmin
Length of output: 3726
🏁 Script executed:
wc -l frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.tsRepository: rocket-admin/rocketadmin
Length of output: 154
🏁 Script executed:
cat -n frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts | sed -n '274,330p'Repository: rocket-admin/rocketadmin
Length of output: 2190
🏁 Script executed:
cat -n frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts | sed -n '100,273p'Repository: rocket-admin/rocketadmin
Length of output: 6553
Refactor this component's state management to use Angular Signals and computed() instead of manual synchronization.
The state properties groupedPolicies, addActionGroups, editActionGroups, usedTables, and usedDashboards are plain class properties manually synchronized through ngOnChanges() → _refreshViews(), and direct mutations in startEdit(). Per the coding guidelines for this codebase, all new frontend state must use Angular Signals with computed() for derived values. This ensures the template cannot drift when any mutation path forgets to trigger the refresh.
Refactor as follows:
- Convert writable state (
policiesmutations) to signals - Use
computed()forgroupedPolicies,usedTables, andusedDashboards(these derive frompolicies+ internal config) - Use
computed()foraddActionGroupsandeditActionGroups(these derive from current policies and edit context) - Remove
ngOnChanges()and_refreshViews()bookkeeping
Also applies to: 274-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`
around lines 62 - 77, The component currently keeps UI state in plain properties
(groupedPolicies, addActionGroups, editActionGroups, usedTables, usedDashboards)
and manually updates them in ngOnChanges()/_refreshViews() and startEdit(),
causing possible template drift; convert relevant state to Angular Signals and
derived computed() values: make the input/writable policies a signal (e.g.,
policiesSignal) and remove ngOnChanges()/_refreshViews(); implement computed()
for groupedPolicies, usedTables, usedDashboards based on policiesSignal and any
static config; implement computed() for addActionGroups and editActionGroups
that also read edit context (e.g., activeEditId or editContext signal) instead
of mutating arrays in startEdit(); update startEdit() to set the edit-context
signal rather than mutating editActionGroups directly and remove manual Map
mutations, ensuring the template reads the computed signals.
Comment on lines
188
to
+191
| removePolicy(index: number) { | ||
| this.policies = this.policies.filter((_, i) => i !== index); | ||
| this.policiesChange.emit(this.policies); | ||
| this._refreshViews(); |
There was a problem hiding this comment.
Rebase or clear the edit state before deleting another policy.
If a policy is being edited and Line 189 removes an earlier entry, every later originalIndex shifts but editingIndex does not. After _refreshViews() the edit form can jump to the wrong row—or disappear while hasPendingChanges() in frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts stays true—so a subsequent save can overwrite the wrong policy.
Suggested fix
removePolicy(index: number) {
+ if (this.editingIndex !== null) {
+ if (this.editingIndex === index) {
+ this.cancelEdit();
+ } else if (this.editingIndex > index) {
+ this.editingIndex -= 1;
+ }
+ }
this.policies = this.policies.filter((_, i) => i !== index);
this.policiesChange.emit(this.policies);
this._refreshViews();
+ if (this.editingIndex !== null) {
+ this.editActionGroups = this._buildFilteredGroups(this.editingIndex);
+ }
}📝 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
| removePolicy(index: number) { | |
| this.policies = this.policies.filter((_, i) => i !== index); | |
| this.policiesChange.emit(this.policies); | |
| this._refreshViews(); | |
| removePolicy(index: number) { | |
| if (this.editingIndex !== null) { | |
| if (this.editingIndex === index) { | |
| this.cancelEdit(); | |
| } else if (this.editingIndex > index) { | |
| this.editingIndex -= 1; | |
| } | |
| } | |
| this.policies = this.policies.filter((_, i) => i !== index); | |
| this.policiesChange.emit(this.policies); | |
| this._refreshViews(); | |
| if (this.editingIndex !== null) { | |
| this.editActionGroups = this._buildFilteredGroups(this.editingIndex); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.ts`
around lines 188 - 191, When removing a policy in removePolicy(index: number)
adjust the editor state first to avoid stale editingIndex references: if there
is an active edit (editingIndex is not null/undefined) and the removed index is
before the current editingIndex, decrement editingIndex accordingly; if the
removed index equals the editingIndex or the editor has pending changes
(hasPendingChanges in cedar-policy-editor-dialog.component.ts), clear/cancel the
edit state (reset editingIndex and any edit buffer) before mutating
this.policies, then emit policiesChange and call _refreshViews() so the editor
always maps to the correct policy or is closed when needed.
| </button> | ||
| <span class="group-members-preview" *ngIf="getGroupUsers(groupItem.group.id) as groupUsers"> | ||
| <span class="group-avatar" *ngFor="let user of groupUsers.slice(0, 3)" | ||
| [matTooltip]="user.name || user.email"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if GroupUser interface has a name property
ast-grep --pattern $'interface GroupUser {
$$$
}'Repository: rocket-admin/rocketadmin
Length of output: 489
🏁 Script executed:
# Find and examine the users.component.html file around line 32
fd users.component.htmlRepository: rocket-admin/rocketadmin
Length of output: 122
🏁 Script executed:
# Get more details about user-related types
rg "interface.*User" frontend/src/app/models/user.ts -A 10Repository: rocket-admin/rocketadmin
Length of output: 1025
🏁 Script executed:
# Check the users component TypeScript file to see what type `user` is
fd users.component.tsRepository: rocket-admin/rocketadmin
Length of output: 120
🏁 Script executed:
# Read the TypeScript component to see type declarations
cat -n frontend/src/app/components/users/users.component.ts | head -100Repository: rocket-admin/rocketadmin
Length of output: 4865
🏁 Script executed:
# Read the HTML template around line 32 with context
cat -n frontend/src/app/components/users/users.component.html | sed -n '20,45p'Repository: rocket-admin/rocketadmin
Length of output: 2287
🏁 Script executed:
# Find the getGroupUsers method to verify the return type
rg "getGroupUsers" frontend/src/app/components/users/users.component.ts -A 5Repository: rocket-admin/rocketadmin
Length of output: 218
Remove user.name access on GroupUser type.
The user variable is typed as GroupUser, which does not define a name property. Only user.email is available. Update line 32 to use [matTooltip]="user.email" instead of the current fallback expression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/components/users/users.component.html` at line 32, The
template uses user.name fallback on a GroupUser which doesn't have name; update
the matTooltip binding in users.component.html so the [matTooltip] uses
user.email only (replace the expression that references user.name with
user.email) — locate the matTooltip attribute on the user row/item where the
template variable is named user and change it to use user.email.
Comment on lines
+187
to
+196
| getUserInitials(user: GroupUser): string { | ||
| // biome-ignore lint/suspicious/noExplicitAny: name comes from API but not typed | ||
| const name = (user as any).name as string | undefined; | ||
| if (name) { | ||
| const parts = name.trim().split(/\s+/); | ||
| if (parts.length >= 2) return (parts[0][0] + parts[1][0]).toUpperCase(); | ||
| return parts[0][0].toUpperCase(); | ||
| } | ||
| return user.email[0].toUpperCase(); | ||
| } |
There was a problem hiding this comment.
Add defensive checks for empty strings and improve typing.
The method has potential edge cases:
- Line 192: If
name.trim()results in[""](empty string after split), accessingparts[0][0]will throw. - Line 195: If
user.emailis an empty string,user.email[0]returnsundefined.
Additionally, the any cast is a type safety concern. Consider extending GroupUser or creating a view-specific type.
🛡️ Proposed fix with defensive checks
getUserInitials(user: GroupUser): string {
- // biome-ignore lint/suspicious/noExplicitAny: name comes from API but not typed
- const name = (user as any).name as string | undefined;
- if (name) {
+ // TODO: Add 'name' to GroupUser interface when API types are updated
+ const name = (user as GroupUser & { name?: string }).name;
+ if (name?.trim()) {
const parts = name.trim().split(/\s+/);
- if (parts.length >= 2) return (parts[0][0] + parts[1][0]).toUpperCase();
- return parts[0][0].toUpperCase();
+ if (parts.length >= 2 && parts[0] && parts[1]) {
+ return (parts[0][0] + parts[1][0]).toUpperCase();
+ }
+ if (parts[0]) return parts[0][0].toUpperCase();
}
- return user.email[0].toUpperCase();
+ return user.email?.[0]?.toUpperCase() || '?';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/components/users/users.component.ts` around lines 187 - 196,
The getUserInitials function can throw on empty strings and uses an any cast;
change it to avoid unsafe indexing by refining the type (extend GroupUser or
create a view-specific type that includes optional name: string | undefined)
instead of casting to any, then defensively handle empty/whitespace values:
check that name is a non-empty string after trim, build parts =
name.trim().split(/\s+/).filter(Boolean) and ensure parts[0] and parts[0][0]
exist before accessing them; if name is absent or yields no valid characters,
fall back to a safe email fallback by verifying user.email is a non-empty string
and using user.email[0].toUpperCase(), otherwise return a sensible default
(e.g., '?').
Comment on lines
+287
to
+297
| /** | ||
| * Update shown history | ||
| */ | ||
| private updateShownHistory(shownIds: string[]): void { | ||
| const history = this.historySubject.value; | ||
| const shown = [...(history.recently_shown_ids || []), ...shownIds].slice(-20); | ||
| this.historySubject.next({ | ||
| ...history, | ||
| recently_shown_ids: shown | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that updateShownHistory is not called anywhere in the codebase
# Search for any calls to updateShownHistory
rg -n "updateShownHistory" --type tsRepository: rocket-admin/rocketadmin
Length of output: 180
Remove unused private method updateShownHistory.
This private method is defined but never called anywhere in the codebase, making it dead code that should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/services/ai-suggestions.service.ts` around lines 287 - 297,
Remove the dead private method updateShownHistory from
ai-suggestions.service.ts: delete the updateShownHistory(shownIds: string[]):
void { ... } implementation and any direct references (none expected) and ensure
historySubject usage elsewhere remains intact; run a quick project-wide search
for "updateShownHistory" to confirm no callers exist and remove any leftover
imports or comments that were only used by this method.
- Wrap dialog in form with ngSubmit for consistent submit behavior - Add confirmation on backdrop click/Escape when pending changes exist - Use CSS variables for theme colors instead of hardcoded values - Add alternative color variable to custom theme - Polish cedar policy list styling (counts, chevrons, borders) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…to the button on Add policy click
There was a problem hiding this comment.
🧹 Nitpick comments (7)
frontend/src/app/components/users/users.component.css (1)
65-75: The.group-system-badgemay need dark mode consideration.This badge uses
--alternative-colorwhich currently lacks a dark mode override in the theme file. If the theme variable gets a dark mode override (as suggested in the theme file review), this component will automatically benefit. Otherwise, the purple#6d28d9on an 88% transparent background may have suboptimal contrast in dark mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/users.component.css` around lines 65 - 75, The .group-system-badge uses the CSS variable --alternative-color without a dark-mode fallback which can produce poor contrast in dark themes; update users.component.css to use a safer fallback or add a dark-mode rule: reference .group-system-badge and ensure background and text colors consider dark mode by either (a) using color-mix with a fallback token like var(--alternative-color, `#6d28d9`) and a contrasting background fallback or (b) adding a `@media` (prefers-color-scheme: dark) or .theme--dark selector that overrides --alternative-color or sets explicit background and color for .group-system-badge to a higher-contrast pair in dark mode.frontend/src/custom-theme.scss (1)
35-41: Consider adding a dark mode override for--alternative-color.The new
--alternative-colorvariable lacks a dark mode override, unlike other semantic colors. Looking at the dark mode block (lines 44-55),--error-colorand--success-colorare redefined for better contrast on dark backgrounds. The purple#6d28d9may appear too dark against dark mode backgrounds.The relevant code snippets show
cedar-policy-list.component.cssalready works around this with hardcoded rgba values in dark mode, suggesting the base variable needs adjustment.Suggested fix
`@media` (prefers-color-scheme: dark) { html { --mat-expansion-container-background-color: var(--surface-dark-color) !important; --mat-table-background-color: var(--surface-dark-color) !important; --mat-paginator-container-background-color: var(--surface-dark-color) !important; --mat-snack-bar-button-color: var(--color-accentedPalette-500) !important; --error-color: var(--color-warnDarkPalette-500); --warning-color: `#f79008`; --info-color: `#296ee9`; --success-color: `#4caf50`; + --alternative-color: `#a78bfa`; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/custom-theme.scss` around lines 35 - 41, The new CSS variable --alternative-color (`#6d28d9`) has no dark-mode override and may be too dark on dark backgrounds; add a dark-theme override in the dark mode block (the selector that redefines --error-color and --success-color) that sets --alternative-color to a lighter/more contrasting purple (for example a lighter hex or an rgba with higher lightness) so components using var(--alternative-color) render correctly in dark mode; update the dark-mode section where --error-color and --success-color are redefined to also redefine --alternative-color.frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.html (3)
39-43: Use@iffor the Add policy button conditional.♻️ Proposed fix
- <button *ngIf="editorMode === 'form' && !formParseError && !loading && !policyList?.showAddForm" - type="button" mat-button color="primary" - (click)="onAddPolicyClick()"> - <mat-icon>add</mat-icon> Add policy - </button> + `@if` (editorMode === 'form' && !formParseError && !loading && !policyList?.showAddForm) { + <button type="button" mat-button color="primary" + (click)="onAddPolicyClick()"> + <mat-icon>add</mat-icon> Add policy + </button> + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.html` around lines 39 - 43, Replace the existing Angular structural directive usage on the Add policy button: change the *ngIf="editorMode === 'form' && !formParseError && !loading && !policyList?.showAddForm" to the new `@if` form (i.e. `@if`="editorMode === 'form' && !formParseError && !loading && !policyList?.showAddForm") while leaving the rest of the button (type, mat-button, color, (click)="onAddPolicyClick()", mat-icon and label) unchanged so the visibility logic uses `@if` instead of *ngIf.
11-14: Use@ifcontrol flow syntax instead of*ngIf.Per coding guidelines, new code should use Angular's built-in control flow syntax (
@if) instead of structural directives (*ngIf).♻️ Proposed fix
- <div *ngIf="formParseError" class="form-parse-warning"> + `@if` (formParseError) { + <div class="form-parse-warning"> <mat-icon>warning</mat-icon> <span>This policy uses advanced Cedar syntax that cannot be represented in form mode. Please use the code editor.</span> </div> + }As per coding guidelines: "Use built-in control flow syntax (
@if,@for,@switch) instead of structural directives (*ngIf,*ngFor,*ngSwitch) in all new code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.html` around lines 11 - 14, The template uses the structural directive *ngIf to conditionally show the warning when formParseError is truthy; replace this with Angular's built-in control-flow syntax using `@if` so the same conditional rendering is done via the new syntax (target the block displaying the warning that references formParseError in cedar-policy-editor-dialog.component.html and convert the *ngIf conditional into the equivalent `@if` ... then .../else structure while preserving the mat-icon, warning text, and CSS class form-parse-warning).
16-36: Use@ifcontrol flow syntax for remaining conditionals.Lines 16 and 26 also use
*ngIfinstead of the preferred@ifsyntax.♻️ Proposed fix
- <div *ngIf="editorMode === 'form' && !formParseError"> + `@if` (editorMode === 'form' && !formParseError) { + <div> <app-cedar-policy-list [policies]="policyItems" [availableTables]="availableTables" [availableDashboards]="availableDashboards" [loading]="loading" (policiesChange)="onPolicyItemsChange($event)"> </app-cedar-policy-list> </div> + } - <div *ngIf="editorMode === 'code'"> + `@if` (editorMode === 'code') { + <div> <p class="cedar-hint">Edit policy in <a href="https://www.cedarpolicy.com/en" target="_blank" rel="noopener">Cedar</a> format</p> <div class="code-editor-box"> <ngs-code-editor [theme]="codeEditorTheme" [codeModel]="cedarPolicyModel" [options]="codeEditorOptions" (valueChanged)="onCedarPolicyChange($event)"> </ngs-code-editor> </div> </div> + }As per coding guidelines: "Use built-in control flow syntax (
@if,@for,@switch) instead of structural directives."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.html` around lines 16 - 36, Replace the structural directives using *ngIf with the built-in control flow syntax `@if` for the two editor blocks: the form block that checks "editorMode === 'form' && !formParseError" (which renders <app-cedar-policy-list> and binds policyItems, availableTables, availableDashboards, loading and (policiesChange)="onPolicyItemsChange($event)"), and the code block that checks "editorMode === 'code'" (which renders the hint and <ngs-code-editor> bound to cedarPolicyModel, codeEditorTheme, codeEditorOptions and (valueChanged)="onCedarPolicyChange($event)"); convert those conditionals to `@if/`@endif preserving the inner bindings, event handlers and the formParseError condition so behavior is unchanged.frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts (2)
74-97: Mixed dependency injection patterns - consider consistency.The component uses
inject(DestroyRef)at line 74 but constructor injection for all other dependencies (lines 76-84). Per coding guidelines, prefer theinject()function pattern consistently.This is a minor consistency issue that could be addressed in a future refactor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts` around lines 74 - 97, The component mixes inject() and constructor DI for dependencies; replace the property-based inject(DestroyRef) with a constructor-injected DestroyRef to be consistent: remove the private _destroyRef = inject(DestroyRef) field and add a constructor parameter (e.g., private _destroyRef: DestroyRef) so existing usages like takeUntilDestroyed(this._destroyRef) and the dialogRef.backdropClick()/keydownEvents() subscriptions continue to work without changing call sites.
173-187: Nativeconfirm()works but is inconsistent with Material Design.The unsaved changes prompts use the browser's native
confirm()dialog, which functions correctly but looks out of place in a Material Design UI. Consider using Angular Material'sMatDialogfor confirmation in a future iteration for visual consistency.The logic for checking
hasPendingChanges()and callingdiscardPending()is correct and well-coordinated with theCedarPolicyListComponent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts` around lines 173 - 187, Replace the native confirm() calls in confirmClose() and savePolicy() with an Angular Material confirmation dialog: inject MatDialog into the component, open a small confirm dialog (e.g., ConfirmDialogComponent or MatDialog.open with a simple template) passing the prompt text, await the dialogRef.afterClosed() result, and only call this.policyList.discardPending() when the result is affirmative; keep the existing hasPendingChanges() checks and the this.dialogRef.close() behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.html`:
- Around line 39-43: Replace the existing Angular structural directive usage on
the Add policy button: change the *ngIf="editorMode === 'form' &&
!formParseError && !loading && !policyList?.showAddForm" to the new `@if` form
(i.e. `@if`="editorMode === 'form' && !formParseError && !loading &&
!policyList?.showAddForm") while leaving the rest of the button (type,
mat-button, color, (click)="onAddPolicyClick()", mat-icon and label) unchanged
so the visibility logic uses `@if` instead of *ngIf.
- Around line 11-14: The template uses the structural directive *ngIf to
conditionally show the warning when formParseError is truthy; replace this with
Angular's built-in control-flow syntax using `@if` so the same conditional
rendering is done via the new syntax (target the block displaying the warning
that references formParseError in cedar-policy-editor-dialog.component.html and
convert the *ngIf conditional into the equivalent `@if` ... then .../else
structure while preserving the mat-icon, warning text, and CSS class
form-parse-warning).
- Around line 16-36: Replace the structural directives using *ngIf with the
built-in control flow syntax `@if` for the two editor blocks: the form block that
checks "editorMode === 'form' && !formParseError" (which renders
<app-cedar-policy-list> and binds policyItems, availableTables,
availableDashboards, loading and
(policiesChange)="onPolicyItemsChange($event)"), and the code block that checks
"editorMode === 'code'" (which renders the hint and <ngs-code-editor> bound to
cedarPolicyModel, codeEditorTheme, codeEditorOptions and
(valueChanged)="onCedarPolicyChange($event)"); convert those conditionals to
`@if/`@endif preserving the inner bindings, event handlers and the formParseError
condition so behavior is unchanged.
In
`@frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts`:
- Around line 74-97: The component mixes inject() and constructor DI for
dependencies; replace the property-based inject(DestroyRef) with a
constructor-injected DestroyRef to be consistent: remove the private _destroyRef
= inject(DestroyRef) field and add a constructor parameter (e.g., private
_destroyRef: DestroyRef) so existing usages like
takeUntilDestroyed(this._destroyRef) and the
dialogRef.backdropClick()/keydownEvents() subscriptions continue to work without
changing call sites.
- Around line 173-187: Replace the native confirm() calls in confirmClose() and
savePolicy() with an Angular Material confirmation dialog: inject MatDialog into
the component, open a small confirm dialog (e.g., ConfirmDialogComponent or
MatDialog.open with a simple template) passing the prompt text, await the
dialogRef.afterClosed() result, and only call this.policyList.discardPending()
when the result is affirmative; keep the existing hasPendingChanges() checks and
the this.dialogRef.close() behavior intact.
In `@frontend/src/app/components/users/users.component.css`:
- Around line 65-75: The .group-system-badge uses the CSS variable
--alternative-color without a dark-mode fallback which can produce poor contrast
in dark themes; update users.component.css to use a safer fallback or add a
dark-mode rule: reference .group-system-badge and ensure background and text
colors consider dark mode by either (a) using color-mix with a fallback token
like var(--alternative-color, `#6d28d9`) and a contrasting background fallback or
(b) adding a `@media` (prefers-color-scheme: dark) or .theme--dark selector that
overrides --alternative-color or sets explicit background and color for
.group-system-badge to a higher-contrast pair in dark mode.
In `@frontend/src/custom-theme.scss`:
- Around line 35-41: The new CSS variable --alternative-color (`#6d28d9`) has no
dark-mode override and may be too dark on dark backgrounds; add a dark-theme
override in the dark mode block (the selector that redefines --error-color and
--success-color) that sets --alternative-color to a lighter/more contrasting
purple (for example a lighter hex or an rgba with higher lightness) so
components using var(--alternative-color) render correctly in dark mode; update
the dark-mode section where --error-color and --success-color are redefined to
also redefine --alternative-color.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6c8afe1-bbc0-4a17-827d-1165a153b836
📒 Files selected for processing (8)
frontend/src/app/components/dashboard/db-table-view/db-table-actions/db-table-actions.component.cssfrontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.cssfrontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.htmlfrontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.tsfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.cssfrontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.htmlfrontend/src/app/components/users/users.component.cssfrontend/src/custom-theme.scss
✅ Files skipped from review due to trivial changes (2)
- frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.css
- frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/components/users/cedar-policy-list/cedar-policy-list.component.html
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style