Remove useDefineForClassFields from tsconfig#598
Conversation
uldisrudzitis
added
Enhancement
dependencies
WalkthroughThis pull request systematically replaces NGXS @select decorators with explicit store.select() assignments in constructors across multiple components and services. Additionally, several properties are made readonly, type exports are added to a regional-costs state file, and a TypeScript compiler option is removed. Changes
Sequence DiagramsequenceDiagram
participant Component
participant Constructor
participant Store
rect rgb(200, 220, 255)
Note over Component,Store: Before: @Select Decorator Pattern
Component->>Component: @Select(State) observable$
Note over Component: Observable initialized via<br/>decorator metadata at class load
end
rect rgb(220, 240, 220)
Note over Component,Store: After: Explicit Constructor Pattern
Component->>Constructor: Component instantiation
Constructor->>Store: store.select(State)
Store-->>Constructor: Observable stream
Constructor->>Component: this.observable$ = result
Note over Component: Observable explicitly<br/>assigned in constructor
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Areas requiring attention during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/src/app/sites/settings/site-settings.component.ts (1)
284-305: Fix wrong equality and add null-guard in children reuse logic
- Bug: comparing prevChild.setting === child.config mixes types and prevents reuse; should compare setting-to-setting (or config-to-config).
- Robustness: prevSettingChildren may be undefined; length access can throw.
Suggested diff:
- const prevSettingChildren = - prevSettingGroup.settings[index].children; + const prevSettingChildren = + prevSettingGroup.settings[index].children; - if (prevSettingChildren.length > 0) { + if (prevSettingChildren?.length > 0) { - return row.map((child, i) => { + return row.map((child, i) => { const prevChild = prevSettingRow[i]; - if ( - prevChild && - prevChild.setting === child.config - ) { + if ( + prevChild && + prevChild.setting === child.setting + ) { return prevChild; }Also applies to: 293-296
🧹 Nitpick comments (4)
editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
451-486: Avoid nested subscriptions; add automatic teardownUse RxJS composition + takeUntilDestroyed to prevent leaks and simplify flow.
Minimal change:
+import { DestroyRef, inject } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; export class EntryGalleryEditorComponent implements OnInit { + private destroyRef = inject(DestroyRef); ngOnInit() { - this.route.paramMap - .pipe( - filter((p) => p['params']['section'] !== undefined && p['params']['entry_id'] !== undefined), - ) - .subscribe((params) => { - const sectionName = params['params']['section']; - const entryId = params['params']['entry_id']; - - combineLatest([ this.currentSite$, ... ]) - .subscribe(([site, template, section, entry]) => { ... }); - }); + this.route.paramMap.pipe( + filter((p) => p['params']['section'] !== undefined && p['params']['entry_id'] !== undefined), + map((params) => ({ + sectionName: params['params']['section'], + entryId: params['params']['entry_id'], + })), + switchMap(({ sectionName, entryId }) => + combineLatest([ this.currentSite$, /* ... */ ]).pipe( + map(([site, template, section, entry]) => ({ site, template, section, entry })), + ) + ), + takeUntilDestroyed(this.destroyRef), + ).subscribe(({ site, template, section, entry }) => { /* assign fields, setSelectedFile() */ }); }editor/src/app/app.component.ts (2)
234-246: Teardown for live subscriptionsuser$, isLoggedIn$, and isSetup$ subscriptions are long‑lived but not unsubscribed. Prefer takeUntilDestroyed to avoid leaks and align with Angular 16+.
Example:
+import { DestroyRef, inject } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; export class AppComponent implements OnInit, OnDestroy { + private destroyRef = inject(DestroyRef); - this.isLoggedIn$.subscribe((isLoggedIn) => { ... }); + this.isLoggedIn$.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((isLoggedIn) => { ... }); - this.isSetup$.pipe(filter((isSetup) => isSetup)).subscribe(() => { ... }); + this.isSetup$.pipe(filter(Boolean), takeUntilDestroyed(this.destroyRef)).subscribe(() => { ... }); - this.user$.subscribe((user) => { ... }); + this.user$.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((user) => { ... });Also applies to: 247-271
305-318: Use new Observable instead of deprecated Observable.createRxJS 7 deprecates Observable.create. Minor cleanup:
- return Observable.create((observer) => { + return new Observable((observer) => {editor/src/app/shop/regional-costs/shop-regional-costs.component.ts (1)
165-165: Consider marking regionalCosts$ as readonlyAligns with other files in this PR and communicates intent.
- regionalCosts$: Observable<ShopRegion[]>; + readonly regionalCosts$: Observable<ShopRegion[]>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
editor/src/app/app.component.ts(2 hunks)editor/src/app/header/header.component.ts(2 hunks)editor/src/app/header/preview-toggle.component.ts(2 hunks)editor/src/app/profile-dropdown/profile-dropdown.component.ts(2 hunks)editor/src/app/shop/orders/shop-orders.component.ts(2 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.component.ts(3 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts(1 hunks)editor/src/app/sites/media/entry-gallery-editor.component.ts(2 hunks)editor/src/app/sites/media/entry-gallery-image-editor.component.ts(3 hunks)editor/src/app/sites/media/site-media.component.ts(4 hunks)editor/src/app/sites/sections/background-gallery-editor.component.ts(2 hunks)editor/src/app/sites/settings/site-settings.component.ts(2 hunks)editor/src/app/user/auth-guard.service.ts(2 hunks)editor/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (1)
- editor/tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (9)
editor/src/app/profile-dropdown/profile-dropdown.component.ts (1)
editor/src/app/user/user.state.model.ts (1)
UserStateModel(1-19)
editor/src/app/sites/media/entry-gallery-image-editor.component.ts (1)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)
editor/src/app/shop/regional-costs/shop-regional-costs.component.ts (1)
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (1)
ShopRegion(44-49)
editor/src/app/header/header.component.ts (1)
editor/src/app/user/user.state.model.ts (1)
UserStateModel(1-19)
editor/src/app/sites/settings/site-settings.component.ts (2)
editor/src/app/user/user.state.model.ts (1)
UserStateModel(1-19)editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)
editor/src/app/sites/sections/background-gallery-editor.component.ts (2)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (2)
SiteSectionStateModel(1-37)SiteSectionBackgroundFile(39-47)
editor/src/app/sites/media/entry-gallery-editor.component.ts (3)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)editor/src/app/sites/sections/entries/entries-state/section-entries-state.model.ts (2)
SectionEntry(5-11)SectionEntryGalleryFile(22-32)
editor/src/app/sites/media/site-media.component.ts (1)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)
editor/src/app/app.component.ts (1)
editor/src/app/user/user.state.model.ts (1)
UserStateModel(1-19)
🔇 Additional comments (15)
editor/src/app/header/preview-toggle.component.ts (1)
54-62: LGTM! Clean refactor from @select decorator to constructor initialization.The explicit observable declaration and constructor-based initialization using the named selector
AppState.getLastRoutefollows best practices and maintains type safety.editor/src/app/user/auth-guard.service.ts (1)
13-20: Excellent refactor with proper visibility and immutability.The change to
private readonlyis ideal for this use case sinceisLoggedIn$is only used within the class and should never be reassigned. The constructor initialization with the named selectorUserState.isLoggedInmaintains type safety.editor/src/app/sites/media/entry-gallery-image-editor.component.ts (1)
127-149: Clean refactor maintaining template compatibility.The explicit observable declaration and constructor initialization properly maintains the public visibility needed for template usage at line 166. The named selector
SitesState.getCurrentSiteensures type safety.editor/src/app/shop/orders/shop-orders.component.ts (1)
132-138: LGTM! Proper refactor of multiple observables.Both observables are correctly declared and initialized in the constructor with their respective named selectors. The public visibility is appropriate for template usage throughout lines 12-114.
editor/src/app/sites/media/site-media.component.ts (2)
99-125: LGTM! Proper use of readonly and correct initialization.The
readonlymodifier is appropriate sincecurrentSite$is never reassigned after constructor initialization. The named selectorSitesState.getCurrentSitemaintains type safety.
35-35: Correct track expression update.The track expression correctly accesses
section.section.nameto match the data structure wheresectionsListcontains objects with a nestedsectionproperty (as defined at lines 101-104).editor/src/app/sites/sections/background-gallery-editor.component.ts (1)
329-344: Excellent refactor with improved type safety and visibility.The
private readonlymodifier is ideal sincecurrentSite$is only used within the class (assigned tothis.currentSiteat line 359). The explicit boolean type annotations on lines 334-335 improve code clarity. The constructor initialization withSitesState.getCurrentSitemaintains type safety.editor/src/app/profile-dropdown/profile-dropdown.component.ts (1)
114-118: No changes needed—implementation is consistent with established codebase patterns.The review comment incorrectly suggests using a named selector. However, verification shows that raw state access
(state) => state.useris the consistent pattern throughout the codebase for accessing the full user state (used in 9+ files), while named selectors likeUserState.isLoggedInare reserved for derived values. No full-state selector exists or is expected. The code in profile-dropdown.component.ts follows the correct pattern.Likely an incorrect or invalid review comment.
editor/src/app/header/header.component.ts (1)
122-132: Let me check the complete UserState file to see all available selectors:The review comment appears based on incomplete information about available selectors. Verification requires access to the full UserState file.
From the script output, I found that
UserStatehas selectors likeisLoggedIn,hasFeatureShop, andhasFeatureMultisite. However, the script only showed partial context, not all available selectors. To conclusively determine whether the raw state access(state) => state.usershould be replaced with a named selector, I need to see the completeuser.state.tsfile to verify:
- Whether
UserStateprovides a dedicated selector for the full user state (e.g.,@Selector() static user(...))- Whether the raw access pattern is intentional or an oversight
The evidence is inconclusive because grep with context flags only showed a subset of methods. If a
userselector exists inUserState, the suggestion is valid. If it doesn't exist, raw access may be the intended pattern.editor/src/app/sites/settings/site-settings.component.ts (2)
4-4: Decorator removal and Store import — looks goodSwitching from @select to Store.select aligns with the PR’s direction.
172-173: Move to readonly Observables initialized in constructor — good; verify null-safety in template callsThe change is clean and explicit. Please verify initial emissions for user$ and appState$ are never undefined when passed into methods from the template (isDisabled/disabledReason). If there’s any chance of undefined, guard in the methods or startWith a safe default.
Also applies to: 179-182
editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
431-431: Constructor-based store selection and explicit typing — LGTM
- private readonly currentSite$ via store.select: good.
- Explicit boolean inits: good.
Also applies to: 437-438, 446-448
editor/src/app/app.component.ts (1)
144-144: Explicit store.select wiring + readonly fields — solidClearer initialization, reduced decorator magic, and tighter immutability. Nice.
Also applies to: 149-150, 151-156, 159-161, 168-174
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (1)
44-60: Exporting interfaces — good moveMaking ShopRegion, ShopRegionalCost, and ShopRegionalCostsModel exported enables safer typing in components/selectors.
editor/src/app/shop/regional-costs/shop-regional-costs.component.ts (1)
165-165: Constructor-wired regionalCosts$ — LGTMDirect Store.select improves clarity and avoids decorator coupling. Works well with exported ShopRegion type.
Also applies to: 185-188
1 participant
truevalue.@Selectdecorator.Summary by CodeRabbit