WEB-859: Follow-up-fix - #3384
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
View Layer - Template & Styling src/app/system/roles-and-permissions/view-role/view-role.component.html, view-role.component.scss |
Refactored single-column layout into two-pane design with permission-toolbar header, search-field panel, left grouping-list, and right permissions panel. Added conditional rendering for search-active vs. normal states, dynamic match-count badges, and no-results messaging. Updated CSS to support new responsive toolbar, search input styling, scrollable panes, and submit-actions footer. |
Search & Filter Logic view-role.component.ts |
Introduced search state (searchText, isSearchActive, groupingMatchCounts, filteredGroupPermissions) and methods (filterPermissions, updateFilteredGroupPermissions, clearSearch). Integrated Material icons (MatIcon, MatIconButton). Updated showPermissions() to trigger filtered results refresh. |
Translations src/assets/translations/en-US.json |
Added three new translation keys: "Search Permissions", "No permissions found", and "Search Results". |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- WEB-859: Update Role Permission Search Field #3381 — Modifies the same view-role component to add permissions search UI and related filtering state/methods, suggesting potential duplicate or iterative work.
Suggested reviewers
- IOhacker
- adamsaghy
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'WEB-859: Follow-up-fix' is vague and does not convey meaningful information about the changeset's main improvements. | Use a more descriptive title that highlights the primary change, such as 'Add search and filter functionality to role permissions' or 'Refactor role permission view with two-pane layout and search feature'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📝 Coding Plan
- Generate coding plan for human review comments
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/system/roles-and-permissions/view-role/view-role.component.scss (1)
181-195:⚠️ Potential issue | 🟡 MinorDon't stack
gapand button margins in the footer.
submit-actionsnow usesgap: 8px, but the nestedbuttonrule still addsmargin-right: 8px, so the two actions render 16px apart. Keep one spacing mechanism here, not both.🎨 Suggested fix
.submit-actions { display: flex; justify-content: flex-end; gap: 8px; padding: 16px 0 0; margin: 0; border-top: 1px solid $light-grey; button { - margin-right: 8px; - - &:last-child { - margin-right: 0; - } + margin-right: 0; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/system/roles-and-permissions/view-role/view-role.component.scss` around lines 181 - 195, The .submit-actions container already uses gap: 8px for spacing, but the nested button rule adds margin-right: 8px (and &:last-child resets it), causing double spacing; remove the button margin rules inside .submit-actions (delete margin-right: 8px and the &:last-child block) so spacing is handled solely by gap on .submit-actions and update any tests/styles that expect the old 16px spacing if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/system/roles-and-permissions/view-role/view-role.component.html`:
- Around line 113-116: The clear-search icon button in the template currently
lacks an accessible name; update the button element that calls clearSearch()
(the <button matSuffix mat-icon-button (click)="clearSearch()"> in
view-role.component.html) to include the standard translation key as an
accessible label by adding an aria-label bound to labels.buttons.Close (or the
i18n/translation pipe used in the project) so the icon-only button is
screen-reader friendly.
- Around line 121-136: The grouping selector currently uses <mat-list> and
non-button <mat-list-item>s which lack proper keyboard/focus support; replace
the container with <mat-action-list> and convert each <mat-list-item> into
button[mat-list-item] (keeping the (click)="showPermissions(grouping)",
[ngClass] binding using selectedItem, and the *ngFor over groupings), add
MatActionList to the component's imports and include it in the component's
imports array, and update the component SCSS to target button[mat-list-item]
selectors instead of mat-list-item so styles and MDC class hierarchy remain
correct.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.ts`:
- Around line 245-269: Search is comparing perm.code directly instead of the
user-visible label produced by permissionName(permission.code), causing
mismatches (e.g., 'READ' vs 'View'). Add a shared normalizer (e.g.,
getSearchablePermissionText(grouping: string, code: string)) that transforms the
code the same way permissionName does (replace underscores, map 'READ' to 'View'
for 'report' grouping, etc.) and use that in filterPermissions (replace
readableName logic) and in updateFilteredGroupPermissions so both badge counts
and filtered lists use the same lowercased searchable text.
---
Outside diff comments:
In `@src/app/system/roles-and-permissions/view-role/view-role.component.scss`:
- Around line 181-195: The .submit-actions container already uses gap: 8px for
spacing, but the nested button rule adds margin-right: 8px (and &:last-child
resets it), causing double spacing; remove the button margin rules inside
.submit-actions (delete margin-right: 8px and the &:last-child block) so spacing
is handled solely by gap on .submit-actions and update any tests/styles that
expect the old 16px spacing if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5f5ca8e-0b20-4a64-a406-b5ca22d3841c
📒 Files selected for processing (4)
src/app/system/roles-and-permissions/view-role/view-role.component.htmlsrc/app/system/roles-and-permissions/view-role/view-role.component.scsssrc/app/system/roles-and-permissions/view-role/view-role.component.tssrc/assets/translations/en-US.json
| @if (searchText) { | ||
| <button matSuffix mat-icon-button (click)="clearSearch()"> | ||
| <mat-icon>close</mat-icon> | ||
| </button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.html | head -130 | tail -30Repository: openMF/web-app
Length of output: 1338
🏁 Script executed:
# Check if labels.buttons.Clear translation exists
find . -name "*.json" -path "*/assets/i18n/*" | head -5Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Search for Clear translation in translation files
grep -r "\"Clear\"" --include="*.json" | grep -i "labels.buttons" | head -10Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check the component file for imports
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.ts | head -40Repository: openMF/web-app
Length of output: 2118
🏁 Script executed:
# Search for how mat-icon-button is used elsewhere in the app with aria-label
rg "mat-icon-button.*aria-label" --type html -A 2 | head -30Repository: openMF/web-app
Length of output: 3351
🏁 Script executed:
# Find translation files with broader search
find . -name "*.json" -path "*/assets/i18n/*" | head -10Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check the structure of translation files
cat $(find . -name "*.json" -path "*/assets/i18n/*" | head -1)Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Search for button-related translations more broadly
grep -r "Clear\|Close" $(find . -name "*.json" -path "*/assets/i18n/*" | head -1) | head -20Repository: openMF/web-app
Length of output: 2346
🏁 Script executed:
# Check what labels.buttons translations exist
rg "labels\.buttons\." --type json | head -30Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Search for labels.buttons keys in the codebase to understand the pattern
rg "labels\.buttons\." --type html -o | sort | uniq | head -30Repository: openMF/web-app
Length of output: 3105
🏁 Script executed:
# Check if there's a Clear button translation or if Close is used instead
rg "'labels\.buttons\." src/app --type html | grep -i "clear\|close" | head -20Repository: openMF/web-app
Length of output: 2131
🏁 Script executed:
# Look at the dropdown component that uses aria-label for clear button pattern
cat -n src/app/shared/dropdown/dropdown.component.html | grep -A 5 -B 5 "Clear"Repository: openMF/web-app
Length of output: 441
Add an accessible name to the clear-search icon button.
mat-icon is aria-hidden by default, and Angular Material expects icon-only buttons to provide a meaningful aria-label. Currently this button has none, making the clear action inaccessible. Use labels.buttons.Close (the standard translation key in this codebase for close/clear actions) on the button element.
♿ Suggested fix
- <button matSuffix mat-icon-button (click)="clearSearch()">
+ <button
+ matSuffix
+ mat-icon-button
+ type="button"
+ [attr.aria-label]="'labels.buttons.Close' | translate"
+ (click)="clearSearch()"
+ >
<mat-icon>close</mat-icon>
</button>📝 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.
| @if (searchText) { | |
| <button matSuffix mat-icon-button (click)="clearSearch()"> | |
| <mat-icon>close</mat-icon> | |
| </button> | |
| `@if` (searchText) { | |
| <button | |
| matSuffix | |
| mat-icon-button | |
| type="button" | |
| [attr.aria-label]="'labels.buttons.Close' | translate" | |
| (click)="clearSearch()" | |
| > | |
| <mat-icon>close</mat-icon> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.html`
around lines 113 - 116, The clear-search icon button in the template currently
lacks an accessible name; update the button element that calls clearSearch()
(the <button matSuffix mat-icon-button (click)="clearSearch()"> in
view-role.component.html) to include the standard translation key as an
accessible label by adding an aria-label bound to labels.buttons.Close (or the
i18n/translation pipe used in the project) so the icon-only button is
screen-reader friendly.
| <div class="layout-row permissions-panel"> | ||
| <div class="layout-column flex-30 grouping-list"> | ||
| <mat-list> | ||
| @for (grouping of groupings; track grouping) { | ||
| <mat-list-item | ||
| [ngClass]="selectedItem === grouping ? 'active' : 'inactive'" | ||
| (click)="showPermissions(grouping)" | ||
| > | ||
| <span class="grouping-name"> | ||
| {{ formatName(grouping) }} | ||
| @if (isSearchActive) { | ||
| <span class="match-count-badge">{{ groupingMatchCounts[grouping] || 0 }}</span> | ||
| } | ||
| </span> | ||
| </mat-list-item> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file at the specified lines
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.html | sed -n '115,140p'Repository: openMF/web-app
Length of output: 1080
🏁 Script executed:
# Check Angular Material version
cat package.json | grep -A 2 "@angular/material"Repository: openMF/web-app
Length of output: 190
🏁 Script executed:
# Check the component's imports and setup
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.ts | head -50Repository: openMF/web-app
Length of output: 2390
🏁 Script executed:
# Check if MatActionList is available in the Material package
rg "mat-action-list|MatActionList" --type typescript --type htmlRepository: openMF/web-app
Length of output: 92
🏁 Script executed:
# Look for existing usage of action-list or selection-list in the codebase
rg "mat-selection-list|MatSelectionList|action-list" --type typescript --type htmlRepository: openMF/web-app
Length of output: 92
🌐 Web query:
Angular Material v20 mat-action-list button mat-list-item MDC-based list component
💡 Result:
In Angular Material v20, the MDC-based action-list is still the normal list package:
<mat-action-list>is theMatActionListcomponent (selector: 'mat-action-list') and its host classes includemat-mdc-action-list … mdc-list, confirming it’s MDC-based. [1]- Action-list items are buttons: you don’t wrap a button in
<mat-list-item>—instead you putmat-list-itemon the<button>:This matches the list API which supports<mat-action-list> <button mat-list-item (click)="save()">Save</button> <button mat-list-item (click)="undo()">Undo</button> </mat-action-list>
button[mat-list-item]. [1][2]
Sources:
[1] Angular Material v20.1.6 compiled source map for MatActionList / MatListItem (UNPKG) (app.unpkg.com)
[2] Angular Material List “Action lists” docs (example usage) (v12.material.angular.io)
Citations:
- 1: https://app.unpkg.com/%40angular/material%4020.1.6/files/fesm2022/list.mjs.map
- 2: https://v12.material.angular.io/docs-content/overviews/material/list/list?utm_source=openai
🏁 Script executed:
# Check what's in STANDALONE_SHARED_IMPORTS to see if MatActionList is included there
rg "STANDALONE_SHARED_IMPORTS|MatActionList" -A 20 src/standalone-shared.module.tsRepository: openMF/web-app
Length of output: 129
🏁 Script executed:
# Check the actual component imports more completely
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.ts | sed -n '38,55p'Repository: openMF/web-app
Length of output: 593
🏁 Script executed:
# Find the standalone-shared module file
find . -name "*standalone*" -type f | head -20Repository: openMF/web-app
Length of output: 95
🏁 Script executed:
# Also check if MatActionList is imported anywhere in the codebase
rg "MatActionList" --type html --type tsRepository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check the STANDALONE_SHARED_IMPORTS definition
cat -n src/app/standalone-shared.module.ts | grep -A 30 "STANDALONE_SHARED_IMPORTS"Repository: openMF/web-app
Length of output: 781
🏁 Script executed:
# Check if MatActionList is available in the Material list package
cat -n src/app/standalone-shared.module.ts | head -5Repository: openMF/web-app
Length of output: 282
🏁 Script executed:
# Look for all list-related imports in the codebase
rg "from '@angular/material/list'" -A 5 -B 5Repository: openMF/web-app
Length of output: 26672
🏁 Script executed:
# Look for the component's SCSS file to understand the current styling
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.scss | head -100Repository: openMF/web-app
Length of output: 2361
🏁 Script executed:
# Search for mat-action-list across the entire codebase to see if it's used anywhere
rg "mat-action-list"Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check if MatActionList is available in Angular Material v20 imports
cat -n src/app/standalone-shared.module.ts | tail -30Repository: openMF/web-app
Length of output: 858
🏁 Script executed:
# Continue reading the SCSS file to see the styling for mat-list-item
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.scss | sed -n '99,150p'Repository: openMF/web-app
Length of output: 1265
🏁 Script executed:
# Continue reading the SCSS file to find the .active class
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.scss | sed -n '150,200p'Repository: openMF/web-app
Length of output: 1048
Use mat-action-list with button items for the grouping selector.
Angular Material's mat-list is styling-only by default. Since these grouping rows drive the permissions panel with click handlers and state management, use mat-action-list with button[mat-list-item] to provide proper keyboard navigation and focus behavior.
Update the component imports to add MatActionList:
-import { MatList, MatListItem } from '@angular/material/list';
+import { MatList, MatListItem, MatActionList } from '@angular/material/list';And update the imports array in @Component:
imports: [
...STANDALONE_SHARED_IMPORTS,
FaIconComponent,
MatList,
MatListItem,
+ MatActionList,⌨️ Suggested HTML changes
<div class="layout-column flex-30 grouping-list">
- <mat-list>
+ <mat-action-list>
`@for` (grouping of groupings; track grouping) {
- <mat-list-item
+ <button
+ type="button"
+ mat-list-item
[ngClass]="selectedItem === grouping ? 'active' : 'inactive'"
(click)="showPermissions(grouping)"
>
<span class="grouping-name">
{{ formatName(grouping) }}
`@if` (isSearchActive) {
<span class="match-count-badge">{{ groupingMatchCounts[grouping] || 0 }}</span>
}
</span>
- </mat-list-item>
+ </button>
}
- </mat-list>
+ </mat-action-list>Note: Update component SCSS selectors to target button[mat-list-item] instead of mat-list-item, as the button element structure changes the DOM hierarchy and MDC class application.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.html`
around lines 121 - 136, The grouping selector currently uses <mat-list> and
non-button <mat-list-item>s which lack proper keyboard/focus support; replace
the container with <mat-action-list> and convert each <mat-list-item> into
button[mat-list-item] (keeping the (click)="showPermissions(grouping)",
[ngClass] binding using selectedItem, and the *ngFor over groupings), add
MatActionList to the component's imports and include it in the component's
imports array, and update the component SCSS to target button[mat-list-item]
selectors instead of mat-list-item so styles and MDC class hierarchy remain
correct.
| filterPermissions(searchValue: string) { | ||
| this.searchText = searchValue; | ||
| if (!searchValue || searchValue.trim() === '') { | ||
| this.isSearchActive = false; | ||
| this.groupingMatchCounts = {}; | ||
| this.filteredGroupPermissions = []; | ||
| return; | ||
| } | ||
| this.isSearchActive = true; | ||
| const lowerSearch = searchValue.toLowerCase(); | ||
| this.groupingMatchCounts = {}; | ||
| for (const grouping of this.groupings) { | ||
| const group = this.tempPermissionUIData[grouping]; | ||
| if (group) { | ||
| let count = 0; | ||
| for (const perm of group.permissions) { | ||
| const readableName = perm.code.replace(/_/g, ' ').toLowerCase(); | ||
| if (readableName.includes(lowerSearch) || perm.code.toLowerCase().includes(lowerSearch)) { | ||
| count++; | ||
| } | ||
| } | ||
| this.groupingMatchCounts[grouping] = count; | ||
| } | ||
| } | ||
| this.updateFilteredGroupPermissions(); |
There was a problem hiding this comment.
Make search match the permission label users see.
The new matcher normalizes perm.code directly, but the checkbox text comes from permissionName(permission.code). In the report grouping that means users see View ... in the UI while the search logic still only sees READ, so both the badge counts and the filtered list can return false negatives. Please route both code paths through one shared normalizer so the search stays aligned with the rendered label.
🔎 Suggested fix
filterPermissions(searchValue: string) {
- this.searchText = searchValue;
- if (!searchValue || searchValue.trim() === '') {
+ this.searchText = searchValue;
+ const lowerSearch = searchValue.trim().toLowerCase();
+ if (!lowerSearch) {
this.isSearchActive = false;
this.groupingMatchCounts = {};
this.filteredGroupPermissions = [];
return;
}
this.isSearchActive = true;
- const lowerSearch = searchValue.toLowerCase();
this.groupingMatchCounts = {};
for (const grouping of this.groupings) {
const group = this.tempPermissionUIData[grouping];
if (group) {
let count = 0;
for (const perm of group.permissions) {
- const readableName = perm.code.replace(/_/g, ' ').toLowerCase();
+ const readableName = this.getSearchablePermissionText(grouping, perm.code);
if (readableName.includes(lowerSearch) || perm.code.toLowerCase().includes(lowerSearch)) {
count++;
}
}
this.groupingMatchCounts[grouping] = count;
@@
updateFilteredGroupPermissions() {
if (!this.isSearchActive || !this.permissions) {
this.filteredGroupPermissions = [];
return;
}
- const lowerSearch = this.searchText.toLowerCase();
+ const lowerSearch = this.searchText.trim().toLowerCase();
this.filteredGroupPermissions = this.permissions.permissions.filter((perm) => {
- const readableName = perm.code.replace(/_/g, ' ').toLowerCase();
+ const readableName = this.getSearchablePermissionText(this.previousGrouping, perm.code);
return readableName.includes(lowerSearch) || perm.code.toLowerCase().includes(lowerSearch);
});
}private getSearchablePermissionText(grouping: string, code: string): string {
let value = code.replace(/_/g, ' ');
if (grouping === 'report') {
value = value.replace(/READ/g, 'View');
}
return value.toLowerCase();
}Also applies to: 275-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.ts` around
lines 245 - 269, Search is comparing perm.code directly instead of the
user-visible label produced by permissionName(permission.code), causing
mismatches (e.g., 'READ' vs 'View'). Add a shared normalizer (e.g.,
getSearchablePermissionText(grouping: string, code: string)) that transforms the
code the same way permissionName does (replace underscores, map 'READ' to 'View'
for 'report' grouping, etc.) and use that in filterPermissions (replace
readableName logic) and in updateFilteredGroupPermissions so both badge counts
and filtered lists use the same lowercased searchable text.
1 participant
Description
This PR contains the final styling adjustment discussed during the review of WEB-859.
The previous PR was merged before I pushed the last change, so this PR applies the remaining update.
Related Issues
Adds Improvement to #3381
Summary by CodeRabbit