Fix angular shop issues with multisite usage#584
Conversation
WalkthroughIntroduces a shop-specific route guard and switches multiple shop-related NGXS states to site-aware initialization using AppState.getSite. Removes several “Add...SiteAction” actions and refactors Init handlers to use patchState. Updates Shop UI structure and Orders template. Simplifies ShopStateService gating to rely on non-null site. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Router
participant AuthGuardShop as AuthGuardShopService
participant Store
participant ShopState as ShopState (NGXS)
participant AppState as AppState.getSite
participant Service as ShopStateService
participant API
rect rgba(230,240,255,0.5)
note over Router,AuthGuardShop: Route entry
Router->>AuthGuardShop: canMatch('shop')
AuthGuardShop->>Store: selectSnapshot(UserState) / features
AuthGuardShop-->>Router: allow if token && 'shop' feature
end
par Site-aware initialization
ShopState->>AppState: select(getSite)
AppState-->>ShopState: site (non-null, distinct)
ShopState->>Service: getInitialState(site)
Service->>API: GET /shop/init?site=site
API-->>Service: initial data
Service-->>ShopState: initial data
ShopState->>ShopState: dispatch InitShopAction (sections, urls)
and Per-feature states
participant Orders as ShopOrdersState
participant Products as ShopProductsState
participant Regions as ShopRegionalCostsState
participant Settings as ShopSettingsState
AppState-->>Orders: site
AppState-->>Products: site
AppState-->>Regions: site
AppState-->>Settings: site
Orders->>API: fetch orders for site
Products->>API: fetch products for site
Regions->>API: fetch regional costs for site
Settings->>API: fetch settings for site
API-->>Orders: {orders}
API-->>Products: {products}
API-->>Regions: {regionalCosts}
API-->>Settings: {settings}
Orders->>Orders: dispatch InitShopOrdersAction { [site]: orders } (patchState)
Products->>Products: dispatch InitShopProductsAction { [site]: products } (patchState)
Regions->>Regions: dispatch InitShopRegionalCostsAction { [site]: regionalCosts } (patchState)
Settings->>Settings: dispatch InitShopSettingsAction { [site]: settings } (patchState)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 8
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/shop/products/shop-products.state.ts (1)
88-110: Add error handling for product updates.This subscription lacks error handling. If
getInitialStatefails during product updates triggered by entry sync, errors will propagate unhandled and could leave the state inconsistent.Apply this diff:
this.actions$ .pipe( ofActionSuccessful(UpdateSectionEntryFromSyncAction), filter((action) => { const actions = ['cartTitle', 'cartPrice', 'cartAttributes']; const prop = action.path.split('/').pop(); return actions.indexOf(prop) > -1; }), switchMap(() => this.store$.select(AppState.getSite).pipe( filter((site) => site !== null), switchMap((site) => this.stateService.getInitialState(site, 'products', true).pipe( take(1), - map((products) => ({ site, products })) + map((products) => ({ site, products })), + catchError((error) => { + console.error(`Failed to reload products for site ${site}:`, error); + return []; + }) ) ) ) ) ) .subscribe(({ site, products }) => { dispatch(new InitShopProductsAction({ [site]: products[site] })); });
🧹 Nitpick comments (4)
editor/src/app/shop/shop-state.service.ts (2)
46-47: Gate on user token too to avoid an initial 401Since the request uses user.token, also require it before firing. Prevents a futile first call if invoked before login.
Apply this diff:
- filter(([appState]) => appState.site !== null), + filter(([appState, user]) => appState.site !== null && !!user.token), take(1), tap(() => this.store.dispatch(new AppShowLoading())), // `exhaustMap` waits for the first request to complete instead of canceling and starting new ones. exhaustMap(([, user]) => { return this.http.get('/_api/v1/plugin/shop', { headers: { 'X-Authorization': 'Bearer ' + user.token }, params: site ? { site: site } : {}, }); }),Also applies to: 50-55
53-54: Minor: pass undefined instead of empty params objectAvoid constructing an empty params object when no site is provided.
- params: site ? { site: site } : {}, + params: site ? { site } : undefined,editor/src/app/shop/settings/shop-settings-config.state.ts (2)
1-1: Remove unused imports and add required onespairwise, tap, concat, and UserState are unused; map and distinct are needed after the change above.
-import { take, pairwise, filter, switchMap, tap } from 'rxjs/operators'; +import { take, filter, switchMap, map, distinct } from 'rxjs/operators'; @@ -import { concat } from 'rxjs'; -import { UserState } from '../../user/user.state'; import { Injectable } from '@angular/core'; import { AppState } from 'src/app/app-state/app.state';Also applies to: 9-13
62-65: Nit: method nameresetProducts is misleading in a settings config state. Consider renaming to resetShopSettingsConfig for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
editor/src/app/app-routing.module.ts(2 hunks)editor/src/app/auth-guard-shop.service.ts(1 hunks)editor/src/app/shop/orders/shop-orders.actions.ts(0 hunks)editor/src/app/shop/orders/shop-orders.component.ts(1 hunks)editor/src/app/shop/orders/shop-orders.state.ts(2 hunks)editor/src/app/shop/products/shop-products.actions.ts(0 hunks)editor/src/app/shop/products/shop-products.state.ts(3 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts(2 hunks)editor/src/app/shop/settings/shop-settings-config.state.ts(3 hunks)editor/src/app/shop/settings/shop-settings.actions.ts(0 hunks)editor/src/app/shop/settings/shop-settings.state.ts(2 hunks)editor/src/app/shop/shop-state.service.ts(1 hunks)editor/src/app/shop/shop.component.ts(2 hunks)editor/src/app/shop/shop.state.ts(4 hunks)
💤 Files with no reviewable changes (3)
- editor/src/app/shop/products/shop-products.actions.ts
- editor/src/app/shop/orders/shop-orders.actions.ts
- editor/src/app/shop/settings/shop-settings.actions.ts
🧰 Additional context used
🧬 Code graph analysis (9)
editor/src/app/shop/shop.component.ts (1)
editor/src/app/shared/animations.ts (1)
Animations(9-29)
editor/src/app/shop/shop.state.ts (2)
editor/src/app/shop/shop.interface.ts (1)
ShopModel(1-4)editor/src/app/shop/shop.actions.ts (1)
InitShopAction(7-10)
editor/src/app/shop/settings/shop-settings-config.state.ts (2)
editor/src/app/shared/helpers.ts (1)
initSettingConfigGroup(144-198)editor/src/app/shop/settings/shop-settings.actions.ts (1)
InitShopSettingsConfigAction(29-32)
editor/src/app/auth-guard-shop.service.ts (2)
editor/src/app/shop/shop-state.service.ts (1)
Injectable(32-140)editor/src/app/user/user.state.model.ts (1)
UserStateModel(1-19)
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (1)
editor/src/app/shop/regional-costs/shop-regional-costs.actions.ts (1)
InitShopRegionalCostsAction(82-85)
editor/src/app/shop/shop-state.service.ts (1)
editor/src/app/app-state/app.actions.ts (1)
AppShowLoading(16-18)
editor/src/app/shop/orders/shop-orders.state.ts (1)
editor/src/app/shop/orders/shop-orders.actions.ts (1)
InitShopOrdersAction(12-15)
editor/src/app/shop/settings/shop-settings.state.ts (1)
editor/src/app/shop/settings/shop-settings.actions.ts (1)
InitShopSettingsAction(25-28)
editor/src/app/shop/products/shop-products.state.ts (1)
editor/src/app/shop/products/shop-products.actions.ts (1)
InitShopProductsAction(23-26)
🔇 Additional comments (3)
editor/src/app/app-routing.module.ts (1)
95-98: Good switch to canMatch for lazy shop routeUsing canMatch on the lazy-loaded shop module is the right choice on Angular 20; it prevents preload and activation when blocked. Based on learnings
editor/src/app/shop/products/shop-products.state.ts (2)
18-19: LGTM: Import additions are appropriate.The
distinctandmapoperators are correctly imported for the refactored reactive pipeline below.
114-119: LGTM: Correct refactor topatchStateand proper naming.The method is correctly renamed from
initializeShopOrderstoinitShopProducts, and the switch fromsetStatetopatchStateis the right choice for efficiently applying partial state updates without replacing the entire state.
| canMatch() { | ||
| const user: UserStateModel = this.store.selectSnapshot( | ||
| (state) => state.user | ||
| ); | ||
| return !!user.token && user.features.includes('shop'); | ||
| } |
There was a problem hiding this comment.
Redirect to /login when blocked (return UrlTree, not false)
Returning false may fall through to NotFound. Return a UrlTree to login for a better UX.
Apply this diff:
-import { CanMatch } from '@angular/router';
+import { CanMatch, Router, UrlTree } from '@angular/router';
@@
-export class AuthGuardShopService implements CanMatch {
- constructor(private store: Store) {}
+export class AuthGuardShopService implements CanMatch {
+ constructor(private store: Store, private router: Router) {}
@@
- canMatch() {
+ canMatch(): boolean | UrlTree {
const user: UserStateModel = this.store.selectSnapshot(
(state) => state.user
);
- return !!user.token && user.features.includes('shop');
+ const allowed = !!user.token && user.features.includes('shop');
+ return allowed ? true : this.router.createUrlTree(['/login']);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canMatch() { | |
| const user: UserStateModel = this.store.selectSnapshot( | |
| (state) => state.user | |
| ); | |
| return !!user.token && user.features.includes('shop'); | |
| } | |
| import { CanMatch, Router, UrlTree } from '@angular/router'; | |
| export class AuthGuardShopService implements CanMatch { | |
| constructor(private store: Store, private router: Router) {} | |
| canMatch(): boolean | UrlTree { | |
| const user: UserStateModel = this.store.selectSnapshot( | |
| (state) => state.user | |
| ); | |
| const allowed = !!user.token && user.features.includes('shop'); | |
| return allowed ? true : this.router.createUrlTree(['/login']); | |
| } | |
| } |
🤖 Prompt for AI Agents
In editor/src/app/auth-guard-shop.service.ts around lines 12 to 17, the canMatch
currently returns false when access is blocked which can fall through to
NotFound; change it to return a UrlTree to /login instead. Inject Router into
the service (add it to the constructor), update the canMatch signature/return
type to boolean | UrlTree, and replace the failing return value with
this.router.createUrlTree(['/login']) (or this.router.parseUrl('/login')) so the
guard redirects to /login when user.token is missing or user.features does not
include 'shop'.
| .subscribe(({ site, orders }) => { | ||
| dispatch(new InitShopOrdersAction({ [site]: orders[site] })); | ||
| }); |
There was a problem hiding this comment.
Fix payload: orders is already the section value
You’re selecting the 'orders' section when calling getInitialState(site, 'orders'), so orders[site] is undefined. Dispatch the section value directly.
- .subscribe(({ site, orders }) => {
- dispatch(new InitShopOrdersAction({ [site]: orders[site] }));
- });
+ .subscribe(({ site, orders }) => {
+ dispatch(new InitShopOrdersAction({ [site]: orders }));
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .subscribe(({ site, orders }) => { | |
| dispatch(new InitShopOrdersAction({ [site]: orders[site] })); | |
| }); | |
| .subscribe(({ site, orders }) => { | |
| dispatch(new InitShopOrdersAction({ [site]: orders })); | |
| }); |
🤖 Prompt for AI Agents
In editor/src/app/shop/orders/shop-orders.state.ts around lines 52 to 54, the
dispatched payload wraps orders with [site] but getInitialState(site, 'orders')
already returns the section value, so orders[site] is undefined; change the
dispatch to pass the orders section directly (dispatch(new
InitShopOrdersAction({ [site]: orders }) should become dispatch(new
InitShopOrdersAction(orders) or dispatch(new InitShopOrdersAction({ site: orders
}) depending on expected shape) — ensure the action receives the section value
rather than indexing into it.
| this.store$ | ||
| .select(AppState.getSite) | ||
| .pipe( | ||
| filter((site) => site !== null), | ||
| distinct((site) => site), | ||
| switchMap((site) => | ||
| this.stateService.getInitialState(site, 'products').pipe( | ||
| take(1), | ||
| map((products) => ({ site, products })) | ||
| ) | ||
| ) | ||
| ) | ||
| ).subscribe((products) => { | ||
| dispatch(new InitShopProductsAction(products)); | ||
| }); | ||
| .subscribe(({ site, products }) => { | ||
| dispatch(new InitShopProductsAction({ [site]: products[site] })); | ||
| }); |
There was a problem hiding this comment.
Replace distinct with distinctUntilChanged and add error handling.
The distinct operator keeps all previously seen values in memory indefinitely. In a multisite application with frequent site switching, this accumulates unbounded memory over the application lifetime. Use distinctUntilChanged() instead, which only compares consecutive emissions.
Additionally, the subscription lacks error handling. If getInitialState fails, the error will propagate unhandled.
Apply this diff:
this.store$
.select(AppState.getSite)
.pipe(
filter((site) => site !== null),
- distinct((site) => site),
+ distinctUntilChanged(),
switchMap((site) =>
this.stateService.getInitialState(site, 'products').pipe(
take(1),
- map((products) => ({ site, products }))
+ map((products) => ({ site, products })),
+ catchError((error) => {
+ console.error(`Failed to load products for site ${site}:`, error);
+ return [];
+ })
)
)
)
.subscribe(({ site, products }) => {
dispatch(new InitShopProductsAction({ [site]: products[site] }));
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In editor/src/app/shop/products/shop-products.state.ts around lines 71 to 85,
replace the RxJS operator distinct(...) with distinctUntilChanged() to avoid
unbounded memory growth, and add error handling to the observable
chain/subscription so failures from getInitialState are caught and handled;
specifically, swap to distinctUntilChanged((a,b)=>a===b) (or default
comparator), and add either a catchError to the pipe that logs/returns EMPTY or
provide an error callback in subscribe that dispatches/logs the error so the app
doesn’t crash on getInitialState failure.
| filter((site) => site !== null), | ||
| distinct((site) => site), | ||
| switchMap((site) => | ||
| this.stateService.getInitialState(site, 'regionalCosts').pipe( | ||
| take(1), | ||
| map((regionalCosts) => ({ site, regionalCosts })) | ||
| ) | ||
| ) | ||
| ) | ||
| ).subscribe((regionalCosts) => { | ||
| dispatch(new InitShopRegionalCostsAction(regionalCosts)); | ||
| }); | ||
| .subscribe(({ site, regionalCosts }) => { | ||
| dispatch( | ||
| new InitShopRegionalCostsAction({ [site]: regionalCosts[site] }) | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| @Action(InitShopRegionalCostsAction) |
There was a problem hiding this comment.
Replace distinct with distinctUntilChanged so sites re-initialize after logout
distinct keeps every emitted site name in its internal Set until the stream completes. Because this subscription lives for the whole app lifetime, switching back to a previously visited site—or logging out (which dispatches ResetShopRegionalCostsAction) and then logging in again with the same site—will never emit that site again. The state stays empty and the regional costs UI never reloads, which regresses multi-site usage. Swap to distinctUntilChanged() so we only skip consecutive duplicates but still re-run initialization when the same site appears later.
-import {
- take,
- tap,
- catchError,
- pairwise,
- filter,
- switchMap,
- distinct,
- map,
-} from 'rxjs/operators';
+import {
+ take,
+ tap,
+ catchError,
+ pairwise,
+ filter,
+ switchMap,
+ distinctUntilChanged,
+ map,
+} from 'rxjs/operators';
...
- filter((site) => site !== null),
- distinct((site) => site),
+ filter((site) => site !== null),
+ distinctUntilChanged(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter((site) => site !== null), | |
| distinct((site) => site), | |
| switchMap((site) => | |
| this.stateService.getInitialState(site, 'regionalCosts').pipe( | |
| take(1), | |
| map((regionalCosts) => ({ site, regionalCosts })) | |
| ) | |
| ) | |
| ) | |
| ).subscribe((regionalCosts) => { | |
| dispatch(new InitShopRegionalCostsAction(regionalCosts)); | |
| }); | |
| .subscribe(({ site, regionalCosts }) => { | |
| dispatch( | |
| new InitShopRegionalCostsAction({ [site]: regionalCosts[site] }) | |
| ); | |
| }); | |
| } | |
| @Action(InitShopRegionalCostsAction) | |
| // editor/src/app/shop/regional-costs/shop-regional-costs.state.ts | |
| // --- Update the import block --- | |
| import { | |
| take, | |
| tap, | |
| catchError, | |
| pairwise, | |
| filter, | |
| switchMap, | |
| distinctUntilChanged, | |
| map, | |
| } from 'rxjs/operators'; | |
| … | |
| // --- In your stream pipeline --- | |
| someSite$ | |
| .pipe( | |
| filter((site) => site !== null), | |
| distinctUntilChanged(), | |
| switchMap((site) => | |
| this.stateService.getInitialState(site, 'regionalCosts').pipe( | |
| take(1), | |
| map((regionalCosts) => ({ site, regionalCosts })) | |
| ) | |
| ) | |
| ) | |
| .subscribe(({ site, regionalCosts }) => { | |
| dispatch( | |
| new InitShopRegionalCostsAction({ [site]: regionalCosts[site] }) | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In editor/src/app/shop/regional-costs/shop-regional-costs.state.ts around lines
85-101, replace the use of the RxJS operator distinct(...) with
distinctUntilChanged() so the stream only skips consecutive duplicate site
values but will re-emit the same site later (e.g., after logout/login), and
update imports to include distinctUntilChanged from 'rxjs/operators' (remove or
replace the distinct import) so site initialization runs again when a previously
visited site reappears.
| this.store$ | ||
| .select(AppState.getSite) | ||
| .pipe( | ||
| filter((site) => site !== null), | ||
| take(1), | ||
| switchMap((site) => | ||
| this.stateService | ||
| .getInitialState(site, 'settingsConfig') | ||
| .pipe(take(1)) | ||
| ) |
There was a problem hiding this comment.
Initialize per-site, not just once
Using take(1) on the site stream initializes only the first site. Mirror the orders state: dedupe by site and fetch once per site.
Apply this diff:
- .pipe(
- filter((site) => site !== null),
- take(1),
- switchMap((site) =>
- this.stateService
- .getInitialState(site, 'settingsConfig')
- .pipe(take(1))
- )
- )
+ .pipe(
+ filter((site) => site !== null),
+ distinct((site) => site),
+ switchMap((site) =>
+ this.stateService
+ .getInitialState(site, 'settingsConfig')
+ .pipe(take(1), map((settingsConfig) => ({ site, settingsConfig })))
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.store$ | |
| .select(AppState.getSite) | |
| .pipe( | |
| filter((site) => site !== null), | |
| take(1), | |
| switchMap((site) => | |
| this.stateService | |
| .getInitialState(site, 'settingsConfig') | |
| .pipe(take(1)) | |
| ) | |
| this.store$ | |
| .select(AppState.getSite) | |
| .pipe( | |
| filter((site) => site !== null), | |
| distinct((site) => site), | |
| switchMap((site) => | |
| this.stateService | |
| .getInitialState(site, 'settingsConfig') | |
| .pipe( | |
| take(1), | |
| map((settingsConfig) => ({ site, settingsConfig })) | |
| ) | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In editor/src/app/shop/settings/shop-settings-config.state.ts around lines
29-38, the code uses take(1) on the site stream which only initializes the first
site; instead dedupe by site and initialize once per site like the orders state.
Remove the take(1) that closes the site stream, add a distinctUntilChanged (or
distinctUntilKeyChanged) comparator on the site (e.g. compare site.id or map to
site.id and use distinctUntilChanged()) before switchMap, and keep the inner
getInitialState(...) call wrapped with take(1) so you fetch initial state once
per distinct site.
| .subscribe((settingsConfig) => { | ||
| const settingGroups = {}; | ||
|
|
||
| for (const groupSlug in settingsConfig) { | ||
| settingGroups[groupSlug] = initSettingConfigGroup( | ||
| settingsConfig[groupSlug] | ||
| ); | ||
| delete settingGroups[groupSlug][groupSlug]; | ||
| } | ||
| for (const groupSlug in settingsConfig) { | ||
| settingGroups[groupSlug] = initSettingConfigGroup( | ||
| settingsConfig[groupSlug] | ||
| ); | ||
| delete settingGroups[groupSlug][groupSlug]; | ||
| } | ||
|
|
||
| dispatch(new InitShopSettingsConfigAction(settingGroups)); | ||
| }); | ||
| dispatch(new InitShopSettingsConfigAction(settingGroups)); | ||
| }); |
There was a problem hiding this comment.
Critical: payload must be keyed by site and reducer should patch, not replace
Currently you dispatch raw settingGroups without the site key and overwrite the whole state with setState. This breaks multi‑site. Key by site and use patchState.
Apply this diff:
- .subscribe((settingsConfig) => {
+ .subscribe(({ site, settingsConfig }) => {
const settingGroups = {};
for (const groupSlug in settingsConfig) {
settingGroups[groupSlug] = initSettingConfigGroup(
settingsConfig[groupSlug]
);
delete settingGroups[groupSlug][groupSlug];
}
- dispatch(new InitShopSettingsConfigAction(settingGroups));
+ dispatch(new InitShopSettingsConfigAction({ [site]: settingGroups }));
});And update the action handler to patch:
- initShopSettingsConfig(
- { setState }: StateContext<ShopSettingsConfigModel>,
- action: InitShopSettingsConfigAction
- ) {
- setState(action.payload);
- }
+ initShopSettingsConfig(
+ { patchState }: StateContext<ShopSettingsConfigModel>,
+ action: InitShopSettingsConfigAction
+ ) {
+ patchState(action.payload);
+ }Also applies to: 55-60
🤖 Prompt for AI Agents
In editor/src/app/shop/settings/shop-settings-config.state.ts around lines 40 to
51 (and similarly for lines 55 to 60), the dispatched payload is an unkeyed
settingGroups object and the reducer replaces the entire state; change the
payload to be keyed by the current site (e.g., { [siteId]: settingGroups }) and
update the action handler to use patchState to merge the new site entry instead
of setState so existing sites are preserved; ensure the action creator payload
structure matches the keyed shape and update any consumers accordingly.
| .select(AppState.getSite) | ||
| .pipe( | ||
| filter((site) => site !== null), | ||
| distinct((site) => site), | ||
| switchMap((site) => | ||
| this.stateService.getInitialState(site, 'settings').pipe( | ||
| take(1), | ||
| map((settings) => ({ site, settings })) | ||
| ) | ||
| ) | ||
| ) | ||
| ).subscribe((settings) => { | ||
| const newState: { [k: string]: any } = {}; | ||
|
|
||
| for (const siteSlug in settings) { | ||
| newState[siteSlug] = this.initializeShopSettingsForSite( | ||
| settings[siteSlug] | ||
| ); | ||
| } | ||
|
|
||
| dispatch(new InitShopSettingsAction(newState)); | ||
| }); | ||
| .subscribe(({ site, settings }) => { | ||
| const newState: { [k: string]: any } = {}; | ||
| newState[site] = this.initializeShopSettingsForSite(settings[site]); | ||
| dispatch(new InitShopSettingsAction(newState)); | ||
| }); |
There was a problem hiding this comment.
Use distinctUntilChanged instead of distinct to allow re-initialization
Here too, distinct permanently memorizes every site value. After ResetShopSettingsAction on logout, logging back in with the same site never emits, leaving shop settings empty. Switching back to a previously visited site in the same session also fails to re-trigger initialization if the state was cleared. Swap to distinctUntilChanged() so we only suppress consecutive duplicates:
-import {
- take,
- tap,
- catchError,
- filter,
- switchMap,
- map,
- distinct,
-} from 'rxjs/operators';
+import {
+ take,
+ tap,
+ catchError,
+ filter,
+ switchMap,
+ map,
+ distinctUntilChanged,
+} from 'rxjs/operators';
...
- filter((site) => site !== null),
- distinct((site) => site),
+ filter((site) => site !== null),
+ distinctUntilChanged(),This keeps the initialization firing anytime the same site appears again after a reset.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .select(AppState.getSite) | |
| .pipe( | |
| filter((site) => site !== null), | |
| distinct((site) => site), | |
| switchMap((site) => | |
| this.stateService.getInitialState(site, 'settings').pipe( | |
| take(1), | |
| map((settings) => ({ site, settings })) | |
| ) | |
| ) | |
| ) | |
| ).subscribe((settings) => { | |
| const newState: { [k: string]: any } = {}; | |
| for (const siteSlug in settings) { | |
| newState[siteSlug] = this.initializeShopSettingsForSite( | |
| settings[siteSlug] | |
| ); | |
| } | |
| dispatch(new InitShopSettingsAction(newState)); | |
| }); | |
| .subscribe(({ site, settings }) => { | |
| const newState: { [k: string]: any } = {}; | |
| newState[site] = this.initializeShopSettingsForSite(settings[site]); | |
| dispatch(new InitShopSettingsAction(newState)); | |
| }); | |
| // --- at the top of shop-settings.state.ts --- | |
| -import { | |
| - take, | |
| - tap, | |
| - catchError, | |
| - filter, | |
| - switchMap, | |
| - map, | |
| - distinct, | |
| import { | |
| take, | |
| tap, | |
| catchError, | |
| filter, | |
| switchMap, | |
| map, | |
| distinctUntilChanged, | |
| } from 'rxjs/operators'; | |
| … | |
| // --- around lines 86–101: replace the distinct call in your pipe --- | |
| .select(AppState.getSite) | |
| .pipe( | |
| filter((site) => site !== null), | |
| distinctUntilChanged(), | |
| switchMap((site) => | |
| this.stateService.getInitialState(site, 'settings').pipe( | |
| take(1), | |
| map((settings) => ({ site, settings })) | |
| ) | |
| ) | |
| ) | |
| .subscribe(({ site, settings }) => { | |
| const newState: { [k: string]: any } = {}; | |
| newState[site] = this.initializeShopSettingsForSite(settings[site]); | |
| dispatch(new InitShopSettingsAction(newState)); | |
| }); |
🤖 Prompt for AI Agents
In editor/src/app/shop/settings/shop-settings.state.ts around lines 86 to 101,
the pipeline uses distinct(...) which permanently memorizes values; replace it
with distinctUntilChanged() from rxjs/operators so only consecutive duplicate
site values are suppressed and re-initialization can occur after resets; update
the imports to import distinctUntilChanged and remove or replace the existing
distinct import; keep the rest of the pipe logic unchanged.
| this.store$ | ||
| .select(AppState.getSite) | ||
| .pipe( | ||
| filter((site) => site !== null), | ||
| take(1), | ||
| switchMap((site) => | ||
| this.stateService.getInitialState(site).pipe(take(1)) | ||
| ) | ||
| ) | ||
| ).subscribe((state) => { | ||
| dispatch( | ||
| new InitShopAction({ | ||
| sections: Object.keys(state).filter((key) => { | ||
| return ( | ||
| Object.keys(defaultState).indexOf(key) === -1 && | ||
| !/config$/i.test(key) | ||
| ); | ||
| }), | ||
| urls: state.urls, | ||
| }) | ||
| ); | ||
| }); | ||
| .subscribe((state) => { | ||
| dispatch( | ||
| new InitShopAction({ | ||
| sections: Object.keys(state).filter((key) => { | ||
| return ( | ||
| Object.keys(defaultState).indexOf(key) === -1 && | ||
| !/config$/i.test(key) | ||
| ); | ||
| }), | ||
| urls: state.urls, | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Remove the outer take(1) so new logins/sites reinitialize
By calling take(1) before switchMap, this subscription completes after the first non-null site. Any later site change—or a logout followed by a login (after ResetShopAction clears the state)—never triggers InitShopAction, so ShopState.getURLs keeps pointing at the previous site and multi-site flows break. Keep the stream alive and just ignore consecutive duplicates, e.g.:
- .pipe(
- filter((site) => site !== null),
- take(1),
- switchMap((site) =>
- this.stateService.getInitialState(site).pipe(take(1))
- )
- )
+ .pipe(
+ filter((site) => site !== null),
+ distinctUntilChanged(),
+ switchMap((site) =>
+ this.stateService.getInitialState(site).pipe(take(1))
+ )
+ )(Remember to import distinctUntilChanged.) This keeps initialization working for every site visit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.store$ | |
| .select(AppState.getSite) | |
| .pipe( | |
| filter((site) => site !== null), | |
| take(1), | |
| switchMap((site) => | |
| this.stateService.getInitialState(site).pipe(take(1)) | |
| ) | |
| ) | |
| ).subscribe((state) => { | |
| dispatch( | |
| new InitShopAction({ | |
| sections: Object.keys(state).filter((key) => { | |
| return ( | |
| Object.keys(defaultState).indexOf(key) === -1 && | |
| !/config$/i.test(key) | |
| ); | |
| }), | |
| urls: state.urls, | |
| }) | |
| ); | |
| }); | |
| .subscribe((state) => { | |
| dispatch( | |
| new InitShopAction({ | |
| sections: Object.keys(state).filter((key) => { | |
| return ( | |
| Object.keys(defaultState).indexOf(key) === -1 && | |
| !/config$/i.test(key) | |
| ); | |
| }), | |
| urls: state.urls, | |
| }) | |
| ); | |
| }); | |
| this.store$ | |
| .select(AppState.getSite) | |
| .pipe( | |
| filter((site) => site !== null), | |
| distinctUntilChanged(), | |
| switchMap((site) => | |
| this.stateService.getInitialState(site).pipe(take(1)) | |
| ) | |
| ) | |
| .subscribe((state) => { | |
| dispatch( | |
| new InitShopAction({ | |
| sections: Object.keys(state).filter((key) => { | |
| return ( | |
| Object.keys(defaultState).indexOf(key) === -1 && | |
| !/config$/i.test(key) | |
| ); | |
| }), | |
| urls: state.urls, | |
| }) | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In editor/src/app/shop/shop.state.ts around lines 105-126, remove the outer
take(1) so the subscription does not complete after the first non-null site;
instead keep filter((site) => site !== null), add distinctUntilChanged() (import
it) after the filter to ignore consecutive duplicate sites, and keep the inner
getInitialState(...).pipe(take(1)) inside switchMap so each new (distinct) site
triggers a fresh InitShopAction without completing the outer stream.
1 participant
Angular shop refactoring:
Summary by CodeRabbit
New Features
Refactor