NAS-137108 / 26.04 / Create a draft for a new ACL management page

[AlexKarpov98]

Testing: It's a draft page with a mock data.

go to /acl-prototype route.

Preview:

https://github.com/user-attachments/assets/2f482bba-7a27-4f17-b406-48ab394cf9f1

[claude[bot]]

Claude finished @AlexKarpov98's task —— View job

---

PR Review Progress

• [x] Analyze main ACL prototype component
• [x] Review modal components (entry edit, preset selection)
• [x] Check routing configuration
• [x] Evaluate code architecture and patterns
• [x] Check compliance with project conventions
• [x] Provide comprehensive feedback

✨ PR Review Results

This is a well-structured prototype that demonstrates good Angular practices and thoughtful UX design! The ACL management interface provides a comprehensive set of features with clear separation of concerns.

🎯 Positive Aspects

The code follows Angular best practices with modern patterns - uses signals, standalone components, OnPush change detection, and the inject() function consistently. The component architecture is well-organized with clear separation between main component and modals.

🔧 Architecture & Code Quality Issues

Constructor Anti-pattern in Modal (acl-entry-edit-modal.component.ts:36)

export class AclEntryEditModalComponent implements OnDestroy {
  private destroy$ = new Subject<void>();
  // ... other inject() calls

  constructor() {
    this.form.controls.type.valueChanges
      .pipe(takeUntil(this.destroy$))
      .subscribe((type) => {
        this.selectedType.set(type || '');
      });
  }
}

Issue: Mixes modern inject() pattern with manual subscription management. The main component correctly uses takeUntilDestroyed().

Solution: Use takeUntilDestroyed() consistently:

private destroyRef = inject(DestroyRef);

constructor() {
  this.form.controls.type.valueChanges
    .pipe(takeUntilDestroyed(this.destroyRef))
    .subscribe((type) => {
      this.selectedType.set(type || '');
    });
}

Inconsistent ESLint Disable Comments

Multiple files use different approaches to disable the same rule. The main component correctly explains why takeUntilDestroyed is preferred.

Missing Form Validation on Entry Modal

The entry edit modal doesn't validate that USER/GROUP types have names, which could lead to invalid ACL entries.

🎨 UX & Accessibility Concerns

Missing Internationalization Support

Many hardcoded strings aren't wrapped in translation markers:

// In acl-prototype.component.ts:338
getTypeDisplayName(type: AclEntry['type']): string {
  const typeNames: Record<AclEntry['type'], string> = {
    USER_OBJ: 'Owner',      // Should be: T('Owner')
    GROUP_OBJ: 'Owning Group',
    // ... others
  };
}

Potential Accessibility Issues

• Color-only status indication in validation results (lines 44-55 in template) may not be accessible to colorblind users
• Permission codes (r/w/x) could benefit from screen reader descriptions

🔒 Security & Data Validation

Weak ID Generation (acl-prototype.component.ts:472)

id: `${isDefault ? 'd' : 'a'}-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`,

While acceptable for prototype, production should use crypto.randomUUID() for better uniqueness guarantees.

Missing Input Sanitization

Entry names and form inputs lack validation against potentially problematic characters or patterns.

🚀 Performance Considerations

Efficient Signal Usage

Good use of signals for reactive state management, though some computed values could be extracted to computed signals for better memoization.

Proper Change Detection

OnPush change detection is correctly implemented across all components.

📋 Minor Issues

1. Unused Imports: Some modal components import unused Angular Material modules
2. Magic Numbers: Permission calculation logic uses magic numbers that could be constants
3. CSS Organization: SCSS file has some commented sections that should be cleaned up

📝 Recommendations

1. Standardize subscription management across all components to use takeUntilDestroyed()
2. Add comprehensive form validation for entry names and types
3. Implement proper i18n for all user-facing strings
4. Add aria-labels and screen reader support for permission indicators
5. Extract validation logic into a service for reusability
6. Add unit tests to cover the current 0% test coverage

The prototype successfully demonstrates the intended UX improvements and provides a solid foundation for the actual implementation! 🎉

---

[bugclerk]

Jira URL: https://ixsystems.atlassian.net/browse/NAS-137108

[codecov[bot]]

Codecov Report

:x: Patch coverage is 0% with 234 lines in your changes missing coverage. Please review. :warning: Please upload report for BASE (master@42bdfaf). Learn more about missing BASE report. :warning: Report is 14 commits behind head on master. :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...app/pages/acl-prototype/acl-prototype.component.ts	0.00%	151 Missing :warning:
...entry-edit-modal/acl-entry-edit-modal.component.ts	0.00%	46 Missing :warning:
...election-modal/preset-selection-modal.component.ts	0.00%	36 Missing :warning:
src/app/admin.routes.ts	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #12417   +/-   ##
=========================================
  Coverage          ?   85.32%           
=========================================
  Files             ?     1825           
  Lines             ?    66319           
  Branches          ?     7733           
=========================================
  Hits              ?    56584           
  Misses            ?     9735           
  Partials          ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.