NAS-139073 / 26.04 / Refactor users page filtering

[william-gr]

Changes:

Simplify logic overall.
Replace UntilDestroy.
Show Local and DS users by default.
Switch Built-in to a toggle available only when Local is selected.
Do not show "root" user by default.

Screen.Recording.2025-12-29.at.1.36.39.PM.mov

Testing:

Self explanatory, make sure it just works as expected.

Downstream

Affects	Reasoning
Documentation	Toggle for built-in users. No root by default.
Testing

[bugclerk]

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

[claude]

Claude finished @william-gr's task —— View job

---

Code Review Complete! ✅

Great refactoring work! The changes successfully simplify the users page filtering logic while maintaining good test coverage. The modern Angular patterns (signals, inject(), takeUntilDestroyed) are well-applied throughout.

🎯 Previous Review Feedback Status

The PR addresses the previous reviewer's concerns:

1. ✅ "Filter by Type" spacing - The SCSS now includes responsive max-width: 300px and min-width: 200px for the select, allowing "Directory Services" to fit on one line (users-search.component.scss:77-81)
2. ✅ Basic/Advanced mode consistency - Mode switching now properly resets to consistent defaults via resetToModeDefaults() (users-search.component.ts:407-421)
3. ✅ Small screen support - The SCSS includes comprehensive mobile breakpoints with @media (max-width: $breakpoint-sm) for stacking layout and full-width controls (users-search.component.scss:11-13, 65-69, 83-87)

💡 Positive Aspects

• Excellent state management: The use of computed signals (isBuiltinFilterActive, isLocalFilterActive, userPresets) eliminates manual synchronization and keeps the code reactive
• Comprehensive test coverage: 96% coverage with thorough test cases covering basic/advanced search, mode switching, filter conflicts, and Active Directory integration
• Clean architecture: Extracting filter logic to users-search-presets.ts creates reusable utilities that benefit both the component and data provider
• Proper cleanup: Successfully removed the unused hideBuiltinUsers preference from the store and related actions

🔍 Issues Found

1. Mode switching behavior inconsistency (users-search.component.ts:407-421)

When switching from basic to advanced mode, the code resets user type selection to defaults (Local + Directory) even if the user had customized it:

private resetToModeDefaults(targetMode: 'basic' | 'advanced'): void {
  if (targetMode === 'basic') {
    // Resets selection regardless of previous user choice
    this.selectedUserTypes.set([UserType.Local, UserType.Directory]);
    this.showBuiltinUsers.set(false);
    this.onUserTypeChange(this.selectedUserTypes());
  } else {
    // Advanced mode also applies defaults
    const defaultFilters = getDefaultUserTypeFilters();
    this.dataProvider().setParams([defaultFilters]);
    this.dataProvider().load();
  }
}

Issue: If a user selects only "Local" users, switches to advanced mode briefly, then switches back, their "Local only" selection is lost and reset to "Local + Directory". This could be surprising.

Suggestion: Consider preserving the user's basic mode selections (user types and builtin toggle) when switching between modes, only clearing the search query text. This would provide a more intuitive experience.

2. Empty search handling could be more explicit (users-search.component.ts:268-271)

protected onSearch(query: SearchQuery<User>): void {
  if (!query) {
    return;
  }
  // ...
}

Issue: The early return silently ignores null/undefined queries without any feedback or default behavior.

Suggestion: Consider whether an empty query should trigger a default search or reset to show all (filtered by type). The current behavior works but could be more explicit about intent.

3. Filter deduplication could be more generic (users-search.component.ts:373-405)

The removeConflictingFilters() method hardcodes handling for builtin and local filters specifically:

private removeConflictingFilters(query: AdvancedSearchQuery<User>): AdvancedSearchQuery<User> {
  const builtinFilters: QueryFilter<User>[] = [];
  const localFilters: QueryFilter<User>[] = [];
  const otherFilters: QueryFilters<User> = [];

  query.filters.forEach((filterItem) => {
    if (Array.isArray(filterItem) && filterItem.length === 3) {
      const [property] = filterItem;
      if (property === 'builtin') {
        builtinFilters.push(filterItem as QueryFilter<User>);
      } else if (property === 'local') {
        localFilters.push(filterItem as QueryFilter<User>);
      } else {
        otherFilters.push(filterItem);
      }
    } else {
      otherFilters.push(filterItem);
    }
  });
  // ...
}

Observation: While this works well for the current use case, adding more special filters would require modifying this method. The logic is correct but not as extensible as it could be.

Suggestion: Consider a more generic approach like a Map<string, QueryFilter<User>> that automatically deduplicates any property, or document why only these two properties need special handling.

4. Minor: Redundant signal update check (users-search.component.ts:272-277)

// Update signal only if changed to avoid unnecessary rerenders
// For advanced queries, signal is already updated in onQueryChange
if (!this.queriesEqual(this.searchQuery(), query)) {
  this.searchQuery.set(query);
}

Observation: The comment mentions avoiding unnecessary rerenders, but signals in Angular already use referential equality checks internally. This check using queriesEqual is actually doing a deep comparison (via isEqual for advanced queries), which is more expensive than the signal's built-in check would be.

Suggestion: If the goal is performance, either rely on signal's built-in checks or clarify the comment to explain that deep equality is needed for semantic reasons (not just performance).

🎨 Style & Best Practices

The code follows Angular best practices well:

• ✅ Uses inject() for dependency injection consistently
• ✅ Properly uses takeUntilDestroyed(this.destroyRef) for subscription cleanup
• ✅ OnPush change detection strategy
• ✅ Standalone component
• ✅ Tests use harnesses appropriately
• ✅ Good use of computed signals for derived state
• ✅ Comprehensive inline documentation

📊 Test Coverage

The test suite is excellent with 96% coverage. Tests properly cover:

• ✅ Basic and advanced search functionality
• ✅ User type filtering (Local, Directory Services)
• ✅ Built-in toggle behavior and disabled states
• ✅ Mode switching and default state handling
• ✅ Filter conflict resolution
• ✅ Active Directory integration scenarios
• ✅ Wildcard and regex escaping

The only missing coverage appears to be in error paths (the 4 uncovered lines mentioned in Codecov).

---

Overall, this is solid refactoring work that successfully achieves its goals! The issues identified are relatively minor and mostly relate to edge cases and potential future extensibility. The core logic is sound and well-tested. 🎉

[codecov]

Codecov Report

❌ Patch coverage is 96.19048% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.77%. Comparing base (1bfbc47) to head (2dc4393).
⚠️ Report is 29 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...s/all-users/users-search/users-search.component.ts	97.36%	2 Missing ⚠️
.../app/pages/credentials/users/store/user.effects.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13033      +/-   ##
==========================================
+ Coverage   86.69%   86.77%   +0.08%     
==========================================
  Files        1845     1847       +2     
  Lines       69324    69378      +54     
  Branches     8551     8578      +27     
==========================================
+ Hits        60097    60206     +109     
+ Misses       9227     9172      -55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[AlexKarpov98]

Looks good overall, I think some improvements needed thouth ⏬ :

1. I think Filter by Type can take more space to fit in one line in the select option.

2. Switching from basic to advanced differs by search results without any user input.

Screen.Recording.2025-12-31.at.13.25.22.mov

3. Would be great to take care of the smaller screens as well at this stage:

[william-gr]

Looks good overall, I think some improvements needed thouth ⏬ :

1. I think Filter by Type can take more space to fit in one line in the select option.

2. Switching from basic to advanced differs by search results without any user input.

Screen.Recording.2025-12-31.at.13.25.22.mov
3. Would be great to take care of the smaller screens as well at this stage:

Good points! Please check again

[AlexKarpov98]

Perfect, thanks!

[bugclerk]

JIRA ticket https://ixsystems.atlassian.net/browse/NAS-139073 is targeted to the following versions which have not received their corresponding PRs: 25.10.2

[bugclerk]

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.