NAS-138379 / 26.04 / Require Connect or Enterprise to enable Spotlight

[william-gr]

Testing:

Try to enable spotlight in SMB Settings

Downstream

Affects	Reasoning
Documentation	Require Connect or Enterprise to enable Spotlight in SMB
Testing	Require Connect or Enterprise to enable Spotlight in SMB

[bugclerk]

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

[claude[bot]]

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

---

Code Review Complete ✅

Great work on this feature! 🎉 The implementation effectively gates Spotlight behind Connect/Enterprise licensing with excellent test coverage and accessibility.

Positive aspects:

• Excellent use of Angular signals and computed properties for reactive state management
• Comprehensive test suite covering all edge cases including accessibility
• Clean separation of concerns with the TruenasConnectService handling modal logic

Issues to address:

1. Missing i18n for aria-label (service-smb.component.html:108)

The aria-label attribute contains hardcoded English text that won't be translated:

[aria-label]="'Configure TrueNAS Connect to enable Spotlight Search feature' | translate"

This should be added to the helptext file and referenced properly.

2. Potential race condition in constructor effect (service-smb.component.ts:110-117)

The effect() in the constructor may trigger before the form is fully initialized from ngOnInit(). Consider using afterNextRender() or moving the effect logic into ngOnInit() after form population:

ngOnInit(): void {
  this.isFormLoading.set(true);
  this.api.call('smb.config').pipe(takeUntilDestroyed(this.destroyRef)).subscribe({
    next: (config) => {
      // ... existing code ...
      this.isFormLoading.set(false);
      
      // Apply initial enable/disable state after form is populated
      if (this.isSpotlightEnabled()) {
        this.form.controls.spotlight_search.enable();
      } else {
        this.form.controls.spotlight_search.disable();
      }
    },
    // ... rest
  });
}

3. Incomplete test description (truenas-connect-button.component.spec.ts:38)

The test name says "should popup the TNC service status" but only verifies the method was called, not that a modal actually appears. This is acceptable but the description could be more accurate: "should call openStatusModal when button is clicked".

4. Missing keyboard event preventDefault test coverage

The onTruenasConnectLinkKeydown method calls preventDefault() on Space key (line 246), but the tests don't verify this behavior. Consider adding:

it('should prevent default behavior when Space is pressed', async () => {
  // ... setup ...
  const event = new KeyboardEvent('keydown', { key: ' ' });
  const preventDefaultSpy = jest.spyOn(event, 'preventDefault');
  noticeLink.dispatchEvent(event);
  expect(preventDefaultSpy).toHaveBeenCalled();
});

Minor observations:

• The codecov report shows 3 missing lines - acceptable given 93% patch coverage
• Good use of inject() pattern throughout (following CLAUDE.md guidelines) ✅
• Proper use of @if syntax instead of *ngIf ✅
• Test harnesses are properly used ✅

Overall, this is solid work with just a few items to address! 🚀

[codecov[bot]]

Codecov Report

:x: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 86.28%. Comparing base (3f2f9fd) to head (bd06705). :warning: Report is 1 commits behind head on master. :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ruenas-connect/services/truenas-connect.service.ts	60.00%	2 Missing :warning:
...es/components/service-smb/service-smb.component.ts	97.22%	1 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #12770   +/-   ##
=======================================
  Coverage   86.28%   86.28%           
=======================================
  Files        1826     1826           
  Lines       67703    67734   +31     
  Branches     8162     8168    +6     
=======================================
+ Hits        58415    58444   +29     
- Misses       9288     9290    +2     

: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.

[william-gr]

Claude review seems mostly bogus/not important.

[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.

[william-gr]

time 3:00

[bugclerk]

Time tracking added.