Skip to content

Conversation

@AlexKarpov98
Copy link
Contributor

Testing: see ticket. It only happened on the first action. Now it's fixed.

Preview:

  1. Searching
    Before:
Screen.Recording.2026-01-02.at.11.11.16.mov

After:

Screen.Recording.2026-01-02.at.11.10.05.mov
  1. Sorting
    Before:
Screen.Recording.2026-01-02.at.11.11.32.mov

After:

Screen.Recording.2026-01-02.at.11.09.45.mov

@claude
Copy link

claude bot commented Jan 2, 2026

Claude finished @AlexKarpov98's task —— View job


Code Review Complete! 🎉

Great fix for the search/sort state persistence issue! The solution is well-architected and follows best practices by centralizing state management in the store.

✅ What's Working Well

The centralization of searchQuery and sortingInfo in InstalledAppsStore is the right architectural approach - it ensures state persists across component lifecycle events and prevents the reset issue when selecting apps for the first time.

🔍 Issues Found

1. Deprecated RxJS operator usage (installed-apps-store.service.ts:3)

The code still uses @ngneat/until-destroy which is deprecated according to CLAUDE.md guidelines:

import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';

Should use: takeUntilDestroyed(this.destroyRef) from @angular/core/rxjs-interop

import { Injectable, inject, DestroyRef } from '@angular/core';
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';

export class InstalledAppsStore extends ComponentStore<InstalledAppsState> {
  private destroyRef = inject(DestroyRef);
  
  // Then replace:
  untilDestroyed(this) 
  // with:
  takeUntilDestroyed(this.destroyRef)
}

2. Duplicate SortableField enum (installed-apps-store.service.ts:20-24)

The enum is defined in both installed-apps-store.service.ts and installed-apps-list.component.ts. Consider moving it to a shared location to follow DRY principles and avoid potential inconsistencies.

Suggestion: Create src/app/pages/apps/enums/sortable-field.enum.ts:

export enum SortableField {
  Application = 'application',
  State = 'state',
  Updates = 'updates',
}

💡 Observations

  • Template changes correctly bind to reactive state via signals (sortingInfo())
  • Test coverage is comprehensive with proper mock implementations using BehaviorSubject
  • The fix properly addresses the root cause: state was previously stored in component instance variables that got reset during Angular's change detection cycles

@bugclerk
Copy link
Contributor

bugclerk commented Jan 2, 2026

@bugclerk bugclerk changed the title NAS-138655: Sort/Search in app view reset when selecting an app (for the first time) NAS-138655 / 26.04 / Sort/Search in app view reset when selecting an app (for the first time) Jan 2, 2026
@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.69%. Comparing base (b9b45d0) to head (a893c6d).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...p/pages/apps/store/installed-apps-store.service.ts 60.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13057      +/-   ##
==========================================
- Coverage   86.69%   86.69%   -0.01%     
==========================================
  Files        1847     1847              
  Lines       69389    69423      +34     
  Branches     8579     8585       +6     
==========================================
+ Hits        60160    60186      +26     
- Misses       9229     9237       +8     

☔ 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 AlexKarpov98 marked this pull request as ready for review January 2, 2026 11:23
@AlexKarpov98 AlexKarpov98 requested a review from a team as a code owner January 2, 2026 11:23
@AlexKarpov98 AlexKarpov98 requested review from mattwyatt-ix and removed request for a team January 2, 2026 11:23
Copy link
Contributor

@mattwyatt-ix mattwyatt-ix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants