Feat switch contents between sites#595
Conversation
WalkthroughAdds a site content-swap feature: backend route and SitesController endpoint that stages folder moves via Storage; Storage gains a public moveFolderContents and exposes XML root props; frontend adds dialog/icon, NGXS actions/handlers, component wiring, styles, and shop/rerender integrations to trigger and react to swaps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as SiteComponent (frontend)
participant D as SitesSwapContentsComponent (dialog)
participant NGXS as NGXS Store
participant API as Backend API
participant S as Storage Utility
U->>C: Click "Swap contents" button
C->>D: Open dialog with currentSite and sites
D-->>C: Return selectedSiteSlug
C->>NGXS: Dispatch SwapContentsSitesAction {siteSlugFrom, siteSlugTo}
NGXS->>API: PUT /sites/swap-contents-between-sites {siteSlugFrom, siteSlugTo}
API->>S: moveFolderContents(source -> tmp, ignore main root if needed)
API->>S: moveFolderContents(dest -> source, ignore main root if needed)
API->>S: moveFolderContents(tmp -> dest)
API-->>NGXS: 200 OK
NGXS->>NGXS: Dispatch domain SwapContents actions (sections, settings, template-settings, tags, entries)
NGXS-->>C: State updates propagate to UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (2)
editor/src/app/shop/settings/shop-settings.state.ts (2)
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 |
uldisrudzitis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_api_app/app/Shared/Storage.php (1)
403-415: Correct symlink check and add types to delFolder.Use is_link on the child path; return consistent boolean.
- public function delFolder($dir) + public function delFolder(string $dir): bool { - if (! is_dir($dir)) { - return; - } + if (! is_dir($dir)) { + return false; + } $files = array_diff(scandir($dir), ['.', '..']); foreach ($files as $file) { - (is_dir("$dir/$file") && ! is_link($dir)) ? $this->delFolder("$dir/$file") : unlink("$dir/$file"); + $path = "$dir/$file"; + (is_dir($path) && ! is_link($path)) ? $this->delFolder($path) : @unlink($path); } - return rmdir($dir); + return (bool) rmdir($dir); }Based on coding guidelines.
🧹 Nitpick comments (7)
editor/src/app/sites/sites-switch-contents.component.ts (1)
13-27: Type dialog data, avoid mutating injected data, and improve UX.
- Don’t bind ngModel to injected data; use a local selectedSiteSlug.
- Type DIALOG_DATA for safety.
- Track by site.name; disable “Switch” until selected.
- @for (site of data.sites; track site) { + @for (site of data.sites; track site.name) { ... - <input + <input type="radio" name="selectedSiteSlug" [value]="site.name" - [(ngModel)]="data.selectedSiteSlug" + [(ngModel)]="selectedSiteSlug" /> ... - <button (click)="dialogRef.close(data.selectedSiteSlug)" class="button"> + <button (click)="dialogRef.close(selectedSiteSlug)" class="button" [disabled]="!selectedSiteSlug"> Switch </button>-export class SitesSwitchContentsComponent { - dialogRef = inject<DialogRef<string>>(DialogRef<string>); - data = inject(DIALOG_DATA); - constructor() {} -} +type SwitchContentsDialogData = { + currentSite: { name: string; title?: string }; + sites: Array<{ name: string; title?: string }>; + selectedSiteSlug?: string; +}; + +export class SitesSwitchContentsComponent { + dialogRef = inject<DialogRef<string>>(DialogRef<string>); + data = inject<SwitchContentsDialogData>(DIALOG_DATA); + selectedSiteSlug = this.data.selectedSiteSlug ?? ''; +}Also applies to: 31-35, 39-43
_api_app/app/Shared/Storage.php (1)
18-22: Avoid making root path fields public (encapsulation).Prefer getters over public properties to prevent accidental mutation.
- public $XML_MAIN_ROOT; - public $XML_STORAGE_ROOT; - public $XML_SITES_ROOT; + protected $XML_MAIN_ROOT; + protected $XML_STORAGE_ROOT; + protected $XML_SITES_ROOT; + // Consider adding getters: getXmlMainRoot(), getXmlStorageRoot(), getXmlSitesRoot()editor/src/app/sites/sites.component.ts (3)
35-45: Initialize state and tighten types.Prevent undefined iteration and add explicit types.
-export class SitesComponent implements OnInit { - destroyRef: DestroyRef; - sites$: Observable<SiteStateModel[]>; - sitesList: SiteStateModel[]; +export class SitesComponent implements OnInit { + destroyRef: DestroyRef; + sites$: Observable<SiteStateModel[]>; + sitesList: SiteStateModel[] = []; @@ - ngOnInit() { + ngOnInit(): void { this.sites$.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((sites) => { this.sitesList = [...sites]; }); }Also applies to: 47-51
53-55: Add missing type annotations to methods.Small hygiene to improve readability and catch mistakes.
- updateComponentFocus(isFocused) { + updateComponentFocus(isFocused: boolean): void { @@ - onDrop(event: CdkDragDrop<string[]>) { + onDrop(event: CdkDragDrop<SiteStateModel[]>): void {Also applies to: 83-92
39-46: Option: inject DestroyRef as a readonly field.Minor ergonomics; avoids assigning in constructor.
-export class SitesComponent implements OnInit { - destroyRef: DestroyRef; +export class SitesComponent implements OnInit { + private readonly destroyRef = inject(DestroyRef); @@ - constructor( + constructor( private store: Store, private router: Router, - ) { - this.destroyRef = inject(DestroyRef); + ) { this.sites$ = this.store.select((state) => state.sites); }editor/src/app/sites/site.component.ts (2)
214-217: Consider more robust null/undefined checking.The current check only handles
undefined. Consider using a more robust check to handle bothnullandundefined.- dialogRef.closed.subscribe((selectedSiteSlug) => { - if (selectedSiteSlug === undefined) { - return; - } + dialogRef.closed.subscribe((selectedSiteSlug) => { + if (!selectedSiteSlug) { + return; + }
227-239: Consider alternatives towindow.location.reload()for better UX.The TODO comment correctly identifies that
window.location.reload()is a temporary workaround. This causes a full page reload, which results in a poor user experience (loss of scroll position, flash of white screen, etc.). As noted in the PR description, this is a known issue to be addressed.Consider these alternatives:
- Reload only the preview iframe if the content changes only affect the preview
- Update specific state slices by re-fetching only the affected data (entries, sections, etc.)
- Use a state refresh pattern by dispatching actions that reload necessary state without a full page refresh
Would you like me to help identify which state slices need to be refreshed, or would you prefer to track this as a separate issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
_api_app/app/Http/Controllers/StateController.php(1 hunks)_api_app/app/Shared/Storage.php(2 hunks)_api_app/app/Sites/SitesController.php(1 hunks)_api_app/routes/api.php(1 hunks)editor/src/app/sites/shared/icon-switch.component.ts(1 hunks)editor/src/app/sites/shared/sites-shared.module.ts(3 hunks)editor/src/app/sites/site.component.ts(6 hunks)editor/src/app/sites/sites-state/sites.actions.ts(1 hunks)editor/src/app/sites/sites-state/sites.state.ts(2 hunks)editor/src/app/sites/sites-switch-contents.component.ts(1 hunks)editor/src/app/sites/sites.component.ts(4 hunks)editor/src/app/sites/sites.module.ts(3 hunks)editor/src/styles/_dialog.scss(1 hunks)editor/src/styles/styles.scss(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
_api_app/**/*.php
📄 CodeRabbit inference engine (_api_app/CLAUDE.md)
_api_app/**/*.php: Always use curly braces for control structures, even for single-line statements.
Use PHP 8 constructor property promotion in__construct()methods.
Do not allow empty__construct()methods with zero parameters.
Always use explicit return type declarations for methods and functions.
Use appropriate PHP type hints for method parameters.
Prefer PHPDoc blocks over inline comments. Avoid comments within code unless the logic is very complex.
Add useful array shape type definitions in PHPDoc blocks when working with arrays.
Enum keys should be TitleCase, for example:FavoritePerson,BestLake,Monthly.
Usephp artisan make:commands to create new Laravel files. Useartisan make:classfor generic PHP classes.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
When generating links, prefer named routes and theroute()function.
_api_app/**/*.php: Always use curly braces for PHP control structures, even for single-line bodies
Use PHP 8 constructor property promotion in __construct() and avoid empty constructors
Always use explicit return type declarations for methods and functions
Use appropriate PHP type hints for method parameters
Prefer PHPDoc blocks over inline code comments; only add inline comments for very complex logic
Add useful PHPDoc array shape type definitions where appropriate
Enum case names should be TitleCase (e.g., FavoritePerson, BestLake, Monthly)
Prefer Eloquent models and relationships over raw database queries (avoid DB:: where possible; use Model::query())
Use Faker via $this->faker or fake() consistently with existing conventions in tests and factories
Files:
_api_app/routes/api.php_api_app/app/Sites/SitesController.php_api_app/app/Http/Controllers/StateController.php_api_app/app/Shared/Storage.php
_api_app/{app/**/*.php,bootstrap/**/*.php,routes/**/*.php,database/**/*.php}
📄 CodeRabbit inference engine (_api_app/.cursor/rules/laravel-boost.mdc)
Do not use env() outside configuration files; use config('...') instead
Files:
_api_app/routes/api.php_api_app/app/Sites/SitesController.php_api_app/app/Http/Controllers/StateController.php_api_app/app/Shared/Storage.php
_api_app/{app/Http/Resources/**/*.php,routes/api.php}
📄 CodeRabbit inference engine (_api_app/.cursor/rules/laravel-boost.mdc)
For APIs, default to Eloquent API Resources and API versioning unless existing routes dictate otherwise
Files:
_api_app/routes/api.php
_api_app/{app,bootstrap,config,routes,database,tests}/**/*.php
📄 CodeRabbit inference engine (_api_app/AGENTS.md)
_api_app/{app,bootstrap,config,routes,database,tests}/**/*.php: Always use curly braces for control structures in PHP, even for single-line bodies
Use PHP 8 constructor property promotion in __construct() and do not include empty constructors
Always declare explicit return types and parameter type hints for functions and methods
Prefer PHPDoc blocks over inline comments; only comment complex logic and add useful array shape types when appropriate
Files:
_api_app/routes/api.php_api_app/app/Sites/SitesController.php_api_app/app/Http/Controllers/StateController.php_api_app/app/Shared/Storage.php
_api_app/{app,bootstrap,routes,database,tests}/**/*.php
📄 CodeRabbit inference engine (_api_app/AGENTS.md)
_api_app/{app,bootstrap,routes,database,tests}/**/*.php: Avoid DB:: facade; prefer Model::query() and Eloquent ORM features
Never use env() outside config files; use config('app.name') etc. instead
Files:
_api_app/routes/api.php_api_app/app/Sites/SitesController.php_api_app/app/Http/Controllers/StateController.php_api_app/app/Shared/Storage.php
_api_app/{resources/views/**/*.blade.php,app/Http/Controllers/**/*.php,routes/**/*.php}
📄 CodeRabbit inference engine (_api_app/AGENTS.md)
Prefer named routes and route() for URL generation
Files:
_api_app/routes/api.php_api_app/app/Http/Controllers/StateController.php
_api_app/app/**/*.php
📄 CodeRabbit inference engine (_api_app/AGENTS.md)
Enum case names should be TitleCase (e.g., FavoritePerson, Monthly)
Files:
_api_app/app/Sites/SitesController.php_api_app/app/Http/Controllers/StateController.php_api_app/app/Shared/Storage.php
_api_app/**/app/Http/Controllers/*.php
📄 CodeRabbit inference engine (_api_app/CLAUDE.md)
Always create Form Request classes for validation rather than inline validation in controllers.
Files:
_api_app/app/Http/Controllers/StateController.php
_api_app/{app/Http/Controllers/**/*.php,app/Models/**/*.php}
📄 CodeRabbit inference engine (_api_app/.cursor/rules/laravel-boost.mdc)
Prevent N+1 queries by using eager loading when accessing relationships
Files:
_api_app/app/Http/Controllers/StateController.php
_api_app/{resources/views/**/*.blade.php,app/Http/Controllers/**/*.php}
📄 CodeRabbit inference engine (_api_app/.cursor/rules/laravel-boost.mdc)
Generate links using named routes and the route() helper
Files:
_api_app/app/Http/Controllers/StateController.php
_api_app/app/Http/Controllers/**/*.php
📄 CodeRabbit inference engine (_api_app/.cursor/rules/laravel-boost.mdc)
Do not perform inline validation in controllers; use dedicated Form Request classes
Use Form Request classes for validation instead of inline validation in controllers; include rules and custom messages
Files:
_api_app/app/Http/Controllers/StateController.php
🧬 Code graph analysis (8)
editor/src/app/sites/sites.module.ts (1)
editor/src/app/sites/shared/sites-shared.module.ts (1)
NgModule(32-94)
_api_app/routes/api.php (1)
_api_app/app/Sites/SitesController.php (1)
SitesController(16-238)
_api_app/app/Sites/SitesController.php (1)
_api_app/app/Shared/Storage.php (3)
Storage(9-640)moveFolderContents(324-401)delFolder(403-415)
editor/src/app/sites/shared/icon-switch.component.ts (1)
editor/src/app/sites/sites-switch-contents.component.ts (1)
Component(4-43)
editor/src/app/sites/sites-switch-contents.component.ts (1)
editor/src/app/sites/shared/icon-switch.component.ts (1)
Component(3-21)
editor/src/app/sites/sites.component.ts (2)
editor/src/app/sites/site.component.ts (1)
Component(27-285)editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)
editor/src/app/sites/site.component.ts (4)
editor/src/app/sites/sites.component.ts (1)
Component(15-93)editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)editor/src/app/sites/sites-state/sites.actions.ts (1)
SwitchContentsSitesAction(62-65)
editor/src/app/sites/sites-state/sites.state.ts (1)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwitchContentsSitesAction(62-65)
🪛 Biome (2.1.2)
editor/src/app/sites/sites-state/sites.state.ts
[error] 329-329: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🔇 Additional comments (9)
editor/src/app/sites/sites-state/sites.actions.ts (1)
62-65: LGTM!The action class follows the established pattern in this file and clearly defines the payload structure for switching contents between sites.
editor/src/app/sites/shared/sites-shared.module.ts (1)
30-30: LGTM!The
IconSwitchComponentis correctly imported, declared, and exported following the established module pattern.Also applies to: 64-64, 90-90
_api_app/app/Http/Controllers/StateController.php (1)
37-37: LGTM!The route mapping for site content switching follows the established pattern in the state URLs array.
editor/src/app/sites/sites.module.ts (1)
4-4: Module wiring looks correct for the new dialog and ngModel usage.Imports and declarations are sufficient; no issues found.
Also applies to: 14-15, 19-19, 21-21, 30-30
editor/src/app/sites/site.component.ts (5)
1-25: LGTM!All new imports are properly utilized in the component.
32-32: LGTM!The active class binding correctly uses the
currentSiteSlug$observable.
120-125: LGTM!Property declarations are correct and properly typed.
135-137: LGTM!Constructor correctly initializes
currentSiteSlug$using theAppState.getSiteselector.
65-72: Code is correct - sites input properly passed and used.Verification confirms that the
sitesinput is correctly defined in SiteComponent (@Input('sites') sites: SiteStateModel[]at line 120) and properly passed from the parent SitesComponent via[sites]="sites$ | async"binding. The conditional rendering at lines 65-72 correctly checkssites.length > 1before displaying the switch contents button.
| public function moveFolderContents($src, $dst, $ignore = []) | ||
| { | ||
| if (! is_dir($src)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (! is_dir($dst)) { | ||
| mkdir($dst, 0755, true); | ||
| } | ||
|
|
||
| // Normalize ignore paths (remove trailing slashes) | ||
| $ignoreNormalized = array_map(function ($path) { | ||
| return rtrim($path, '/\\'); | ||
| }, $ignore); | ||
|
|
||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($src, \RecursiveDirectoryIterator::SKIP_DOTS), | ||
| \RecursiveIteratorIterator::SELF_FIRST | ||
| ); | ||
|
|
||
| // Copy everything (except ignored items) | ||
| foreach ($iterator as $item) { | ||
| $subPath = $iterator->getSubPathName(); | ||
|
|
||
| // Check if this item should be ignored | ||
| $shouldIgnore = false; | ||
| foreach ($ignoreNormalized as $ignorePath) { | ||
| // Check if current path matches or is inside an ignored path | ||
| if ($subPath === $ignorePath || strpos($subPath, $ignorePath . DIRECTORY_SEPARATOR) === 0) { | ||
| $shouldIgnore = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ($shouldIgnore) { | ||
| continue; | ||
| } | ||
|
|
||
| $destPath = $dst . DIRECTORY_SEPARATOR . $subPath; | ||
|
|
||
| if ($item->isDir()) { | ||
| mkdir($destPath, 0755, true); | ||
| } else { | ||
| copy($item, $destPath); | ||
| } | ||
| } | ||
|
|
||
| // Clean up source (except ignored items and source folder itself) | ||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($src, \RecursiveDirectoryIterator::SKIP_DOTS), | ||
| \RecursiveIteratorIterator::CHILD_FIRST // Delete children first | ||
| ); | ||
|
|
||
| foreach ($iterator as $item) { | ||
| $subPath = $iterator->getSubPathName(); | ||
|
|
||
| // Check if this item should be ignored | ||
| $shouldIgnore = false; | ||
| foreach ($ignoreNormalized as $ignorePath) { | ||
| if ($subPath === $ignorePath || strpos($subPath, $ignorePath . DIRECTORY_SEPARATOR) === 0) { | ||
| $shouldIgnore = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ($shouldIgnore) { | ||
| continue; | ||
| } | ||
|
|
||
| if ($item->isDir()) { | ||
| @rmdir($item); // @ suppresses warnings if dir not empty (due to ignored items) | ||
| } else { | ||
| unlink($item); | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix SplFileInfo misuse and add type declarations.
copy(), unlink(), and rmdir() are called with SplFileInfo objects; use paths. Also add types and basic mkdir guard.
- public function moveFolderContents($src, $dst, $ignore = [])
+ public function moveFolderContents(string $src, string $dst, array $ignore = []): bool
{
if (! is_dir($src)) {
return false;
}
- if (! is_dir($dst)) {
- mkdir($dst, 0755, true);
- }
+ if (! is_dir($dst) && !@mkdir($dst, 0755, true) && !is_dir($dst)) {
+ throw new \RuntimeException("Failed to create destination directory: {$dst}");
+ }
...
- if ($item->isDir()) {
- mkdir($destPath, 0755, true);
- } else {
- copy($item, $destPath);
- }
+ if ($item->isDir()) {
+ @mkdir($destPath, 0755, true);
+ } else {
+ copy($item->getPathname(), $destPath);
+ }
...
- if ($item->isDir()) {
- @rmdir($item); // @ suppresses warnings if dir not empty (due to ignored items)
- } else {
- unlink($item);
- }
+ if ($item->isDir()) {
+ @rmdir($item->getPathname()); // suppress warnings if dir not empty (due to ignored items)
+ } else {
+ @unlink($item->getPathname());
+ }
}
return true;
}Based on coding guidelines.
🤖 Prompt for AI Agents
In _api_app/app/Shared/Storage.php around lines 324 to 401, the code incorrectly
passes SplFileInfo objects to copy(), unlink(), and rmdir() and lacks
parameter/return types and mkdir guards; update the function signature to add
types (string $src, string $dst, array $ignore = []): bool, use
$item->getPathname() for source file paths and $destPath for destinations when
calling copy() and unlink(), and call rmdir() with the directory path (e.g.
$item->getPathname() or $destPath for created dirs) rather than the SplFileInfo
object; additionally add checks that mkdir() returns true (or directory already
exists) before proceeding and handle mkdir failures gracefully (return false)
when creating $dst or nested dirs.
| public function switchContentsBetweenSites(Request $request) | ||
| { | ||
| $json = $request->json()->all(); | ||
|
|
||
| $siteSlugFrom = $json['siteSlugFrom']; | ||
| $siteSlugTo = $json['siteSlugTo']; | ||
|
|
||
| $siteFromStorage = new Storage($siteSlugFrom); |
There was a problem hiding this comment.
Add return type and validate request payload.
- Declare an explicit return type.
- Validate required keys and constrain slug format; reject identical slugs early.
Apply:
- public function switchContentsBetweenSites(Request $request)
+ /**
+ * Switch contents between two sites.
+ *
+ * @return \Illuminate\Http\JsonResponse
+ */
+ public function switchContentsBetweenSites(Request $request): \Illuminate\Http\JsonResponse
{
- $json = $request->json()->all();
+ $json = $request->validate([
+ 'siteSlugFrom' => ['required', 'string', 'regex:/^[a-z0-9_-]+$/i'],
+ 'siteSlugTo' => ['required', 'string', 'regex:/^[a-z0-9_-]+$/i', 'different:siteSlugFrom'],
+ ]);Based on coding guidelines.
🤖 Prompt for AI Agents
In _api_app/app/Sites/SitesController.php around lines 106 to 113, the method
switchContentsBetweenSites lacks an explicit return type and does not validate
the request payload; add a return type (e.g. : JsonResponse), validate that
'siteSlugFrom' and 'siteSlugTo' exist and are non-empty strings, enforce a slug
pattern (e.g. /^[a-z0-9\-]+$/i) for both, and immediately return a 400/422
JsonResponse if validation fails or if the two slugs are identical; perform
validation before creating Storage instances and use the validated values
thereafter.
| public function switchContentsBetweenSites(Request $request) | ||
| { | ||
| $json = $request->json()->all(); | ||
|
|
||
| $siteSlugFrom = $json['siteSlugFrom']; | ||
| $siteSlugTo = $json['siteSlugTo']; | ||
|
|
||
| $siteFromStorage = new Storage($siteSlugFrom); | ||
| $siteToStorage = new Storage($siteSlugTo); | ||
|
|
||
| $tmpDir = $siteFromStorage->XML_SITES_ROOT . '/' . bin2hex(random_bytes(8)); | ||
|
|
||
| // move siteFrom to tmp folder | ||
| $siteFromStorage->moveFolderContents( | ||
| $siteFromStorage->XML_STORAGE_ROOT, | ||
| $tmpDir, | ||
| $siteFromStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : [] | ||
| ); | ||
|
|
||
| // move to siteFrom folder from siteTo | ||
| $siteFromStorage->moveFolderContents( | ||
| $siteToStorage->XML_STORAGE_ROOT, | ||
| $siteFromStorage->XML_STORAGE_ROOT, | ||
| $siteToStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : [] | ||
| ); | ||
|
|
||
| // move from tmp to siteTo folder | ||
| $siteFromStorage->moveFolderContents( | ||
| $tmpDir, | ||
| $siteToStorage->XML_STORAGE_ROOT, | ||
| ); | ||
|
|
||
| // delete tmp folder | ||
| $siteFromStorage->delFolder($tmpDir); | ||
|
|
||
| return response()->json($json); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Authorization and audit trail.
Confirm this route is behind auth and authorized (policy/gate) for both slugs; consider logging the operation.
Run:
🏁 Script executed:
#!/bin/bash
# Verify route and middleware/policy for switch-contents
rg -nP "switch-contents-between-sites|switchContentsBetweenSites" -C3 _api_app
rg -nP "Route::(put|post)\(.*switch-contents-between-sites" -C2 _api_appLength of output: 2026
I need to verify the middleware and authorization setup. Let me check the route group context and controller middleware:<!-- [scratchpad_update]
[observations_updated]
- Web search confirms that api routes in routes/api.php are assigned to 'api' middleware group automatically
- The 'api' middleware group typically does NOT include authentication by default
- Need to check if this route group has explicit auth middleware or if it's wrapping the routes in an auth group
- The route definition at line 37 doesn't show explicit middleware on the route itself
[pending_update]
- Check if routes/api.php has a middleware group wrapping these routes
- Check if auth middleware is applied anywhere (in the file)
- Verify authorization logic within the controller method
- Verify logging implementation
[scratchpad_end] -->
I need to check the route file structure and controller middleware setup:
🏁 Script executed:
#!/bin/bash
# Check route group middleware and controller middleware
head -50 _api_app/routes/api.phpLength of output: 3027
🏁 Script executed:
#!/bin/bash
# Check SitesController class for middleware declarations
head -30 _api_app/app/Sites/SitesController.phpLength of output: 1037
🏁 Script executed:
#!/bin/bash
# Search for authorization checks in switchContentsBetweenSites or related methods
rg -nP "authorize|policy|gate|can\(" _api_app/app/Sites/SitesController.php -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for logging in switchContentsBetweenSites method
rg -nP "Log::|logger|log\(" _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the entire route group structure in api.php
cat _api_app/routes/api.php | head -60Length of output: 3426
🏁 Script executed:
#!/bin/bash
# Search for auth middleware in api.php routes
rg -nP "middleware.*auth|auth.*middleware" _api_app/routes/api.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for siteSlug validation or authorization logic
rg -nP "siteSlug|fromStorage|toStorage" _api_app/app/Sites/SitesController.php -C2Length of output: 360
🏁 Script executed:
#!/bin/bash
# Check if Storage class has any auth checks
rg -nP "class Storage" _api_app -A10Length of output: 597
🏁 Script executed:
#!/bin/bash
# Check bootstrap/app.php for api middleware group configuration
grep -A20 "api" _api_app/bootstrap/app.php | head -30Length of output: 513
🏁 Script executed:
#!/bin/bash
# Check for policy classes related to Site
fd "SitePolicy" _api_app/appLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check for authorization or policy usage with sites
rg -nP "can.*site|policy.*site" _api_app/app -iLength of output: 354
🏁 Script executed:
#!/bin/bash
# Search for auth middleware usage on routes
rg -nP "->middleware\(" _api_app/routes/api.php -C1 | head -40Length of output: 404
🏁 Script executed:
#!/bin/bash
# Check for authorization gate definitions
rg -nP "Gate::" _api_app/appLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for Storage instantiation patterns to understand access control
rg -nP "new Storage|Storage::" _api_app/app -C2Length of output: 6394
🏁 Script executed:
#!/bin/bash
# Look for audit logging patterns in the codebase
rg -nP "Log::|Activity::|audit" _api_app/app -iLength of output: 209
🏁 Script executed:
#!/bin/bash
# Full context of switchContentsBetweenSites method
cat _api_app/app/Sites/SitesController.php | sed -n '106,142p'Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Check return type and full method signature
rg -nP "public function switchContentsBetweenSites" _api_app/app/Sites/SitesController.php -A40Length of output: 1503
🏁 Script executed:
#!/bin/bash
# Look for middleware applied to entire SitesController
rg -nP "__construct\(" _api_app/app/Sites/SitesController.php -A10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any middleware applied to entire api routes group
sed -n '1,50p' _api_app/routes/api.phpLength of output: 3027
🏁 Script executed:
#!/bin/bash
# Check for user authorization context in request
rg -nP "\$request->user\(\)|auth\(\)->user\(\)|Auth::" _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check for 'verify' validation middleware (request validation)
rg -nP "validate|FormRequest|validated\(\)" _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for activity logging or event listener implementations
fd "Activity|Audit|Log" _api_app/app --type f | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if audit logging middleware exists
fd -e php _api_app/app -path "*Middleware*" -name "*udit*" -o -path "*Middleware*" -name "*og*"Length of output: 168
🏁 Script executed:
#!/bin/bash
# Look at imports in SitesController to understand dependencies
head -20 _api_app/app/Sites/SitesController.phpLength of output: 596
🏁 Script executed:
#!/bin/bash
# Check if there's a middleware.php or app configuration for global logging
rg -nP "Route::|middleware" _api_app/bootstrap/app.phpLength of output: 92
🏁 Script executed:
#!/bin/bash
# Check entire route file for auth patterns
wc -l _api_app/routes/api.php
tail -30 _api_app/routes/api.phpLength of output: 1613
🏁 Script executed:
#!/bin/bash
# Check if the route group has any middleware
sed -n '30,50p' _api_app/routes/api.phpLength of output: 1614
🏁 Script executed:
#!/bin/bash
# Search for Site Model and authorization patterns
rg -nP "class Site" _api_app/app -A10Length of output: 17016
🏁 Script executed:
#!/bin/bash
# Check for UserPolicy or SitePolicy authorizations
fd "Policy" _api_app/appLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for before/after hooks in the controller
rg -nP "boot\(\)|register\(\)|before|after" _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for all middleware declarations in this controller
rg -nP "middleware|Middleware" _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check kernel for middleware setup
head -80 _api_app/app/Http/Kernel.php | tail -40Length of output: 87
🏁 Script executed:
#!/bin/bash
# Final check: see full routes/api.php to understand group structure
cat _api_app/routes/api.phpLength of output: 7132
🏁 Script executed:
#!/bin/bash
# Check for facade imports - Log, Auth, etc
grep -n "use " _api_app/app/Sites/SitesController.phpLength of output: 458
🏁 Script executed:
#!/bin/bash
# Try to identify if there's a service provider handling auth/logging
rg -nP "SitesController" _api_app/app -lLength of output: 38
🏁 Script executed:
#!/bin/bash
# Look for Laravel activity/audit log packages or custom logging
rg -nP "spatie|logs|activities" _api_app -i | head -20Length of output: 1651
🏁 Script executed:
#!/bin/bash
# Check composer.json for audit logging packages
jq '.require' _api_app/composer.json 2>/dev/null | head -20Length of output: 324
🏁 Script executed:
#!/bin/bash
# Get a clearer understanding of authorization: are there uses of authorize() or $this->authorize()?
rg -nP 'authorize|Gate::check|Gate::allows' _api_app/app/Sites/SitesController.php -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a middleware wrapping the api routes
sed -n '1,5p' _api_app/routes/api.php && echo "---" && sed -n '30,40p' _api_app/routes/api.phpLength of output: 1041
🏁 Script executed:
#!/bin/bash
# Check Request validation or Form Request
rg -nP "FormRequest|validate|Validator" _api_app/app/Sites/SitesController.php -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look specifically at authentication context available to the route
rg -nP "auth:sanctum|auth:api|auth" _api_app/routes/api.php -B2 -A2Length of output: 394
🏁 Script executed:
#!/bin/bash
# Check for rate limiting or other security middleware
rg -nP "throttle|RateLimit" _api_app/routes/api.php -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if Storage class itself validates access
fd "Storage.php" _api_app/app -exec head -50 {} \;Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check how siteSlug is used - is it validated against user permissions?
rg -nP "siteSlugFrom|siteSlugTo" _api_app/app -C3Length of output: 804
🏁 Script executed:
#!/bin/bash
# Any authorization gate or policy definitions at all
rg -nP "Gate::|allow|deny" _api_app/app/Providers -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check service providers to understand auth setup
ls -la _api_app/app/Providers/Length of output: 185
🏁 Script executed:
#!/bin/bash
# Check all uses of 'authorize' in the app
rg -rn "\->authorize\(|\$this->authorize\(" _api_app/app/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a verify request ownership pattern
rg -nP "user\(\)" _api_app/app/Sites/SitesController.php -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any logging at all in the controller methods
rg -nP "Log" _api_app/app/Sites/SitesController.php -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Final comprehensive check of the entire SitesController file
wc -l _api_app/app/Sites/SitesController.php && echo "---" && cat _api_app/app/Sites/SitesController.phpLength of output: 7800
🏁 Script executed:
#!/bin/bash
# Check bootstrap/app.php for full middleware configuration
cat _api_app/bootstrap/app.phpLength of output: 887
🏁 Script executed:
#!/bin/bash
# Check entire routes/api.php file for any middleware setup
cat _api_app/routes/api.phpLength of output: 7132
🏁 Script executed:
#!/bin/bash
# Verify if Illuminate/Support/Facades/Auth is imported
grep -n "use.*Auth\|use.*Log" _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for method comments that might indicate middleware
rg -nP "Middleware|@middleware|middleware|authorize" _api_app/app/Sites/SitesController.php -iLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the entire route definition including any wrapping context
sed -n '1,100p' _api_app/routes/api.php | tail -70Length of output: 4967
🏁 Script executed:
#!/bin/bash
# Check if StorageController uses auth validation when creating Storage instances
grep -rn "class Storage" _api_app/app --include="*.php" -A30Length of output: 1662
🏁 Script executed:
#!/bin/bash
# Final check: look for any Route group middleware wrapping the sites routes
rg -nP "Route::(group|middleware)" _api_app/routes/api.php -B2 -A10Length of output: 3992
🏁 Script executed:
#!/bin/bash
# Look at lines around the switchContentsBetweenSites route
sed -n '20,45p' _api_app/routes/api.phpLength of output: 1944
🏁 Script executed:
#!/bin/bash
# Check if there's any implicit authorization via database records
rg -nP "findOrFail|firstOrFail|findOrAbort" _api_app/app/Sites/SitesController.php -C3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check entire SitesController for any method that validates site access
cat _api_app/app/Sites/SitesController.php | grep -n "public\|private\|protected" | head -20Length of output: 644
🏁 Script executed:
#!/bin/bash
# Get method names and their line ranges
ast-grep --pattern 'class SitesController { $$$ }' _api_app/app/Sites/SitesController.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Try simpler approach - check for constructor with middleware
sed -n '1,50p' _api_app/app/Sites/SitesController.phpLength of output: 2005
🏁 Script executed:
#!/bin/bash
# Check for Sanctum token auth in the route file itself
rg "auth:sanctum|Sanctum" _api_app/routes/api.phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for custom authorization in the project
find _api_app/app -name "*Policy.php" -o -name "*Gate*.php"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if user verification happens via Storage constructor
grep -n "function __construct" _api_app/app -r --include="*.php" | grep -i storageLength of output: 98
🏁 Script executed:
#!/bin/bash
# Check for any logging in Storage class
rg -n "Log::|logger\(" _api_app/app -i | grep -i storageLength of output: 81
🏁 Script executed:
#!/bin/bash
# Verify if the codebase uses Laravel auditing packages
grep -i "audit\|activity\|spatie" _api_app/composer.jsonLength of output: 0
🏁 Script executed:
#!/bin/bash
# One final comprehensive check of the route and controller
echo "=== ROUTES API.PHP ===" && cat _api_app/routes/api.php && echo -e "\n=== SITES CONTROLLER ===" && cat _api_app/app/Sites/SitesController.phpLength of output: 14935
Add authorization check for both site slugs and implement audit logging.
The route is behind authentication (Authenticate::class middleware at routes/api.php:25), but lacks authorization to verify the user can access both specific sites. The method accepts arbitrary siteSlugFrom and siteSlugTo from the request without validating user permissions. Additionally, there is no audit trail for this sensitive operation—add logging before executing the file swaps and also add explicit return type to the method (currently missing).
| $siteFromStorage = new Storage($siteSlugFrom); | ||
| $siteToStorage = new Storage($siteSlugTo); | ||
|
|
||
| $tmpDir = $siteFromStorage->XML_SITES_ROOT . '/' . bin2hex(random_bytes(8)); | ||
|
|
There was a problem hiding this comment.
Harden against invalid/non-existent sites.
Assert both site roots exist before proceeding to avoid operating on unexpected paths.
$siteFromStorage = new Storage($siteSlugFrom);
$siteToStorage = new Storage($siteSlugTo);
- $tmpDir = $siteFromStorage->XML_SITES_ROOT . '/' . bin2hex(random_bytes(8));
+ foreach ([$siteFromStorage->XML_STORAGE_ROOT, $siteToStorage->XML_STORAGE_ROOT] as $root) {
+ if (!is_dir($root)) {
+ abort(422, 'Invalid site slug.');
+ }
+ }
+ $tmpDir = $siteFromStorage->XML_SITES_ROOT . '/' . bin2hex(random_bytes(8));Based on coding guidelines.
🤖 Prompt for AI Agents
In _api_app/app/Sites/SitesController.php around lines 113 to 117, the code
assumes both site storage roots exist; add explicit checks for the existence and
directory-ness of $siteFromStorage->XML_SITES_ROOT and
$siteToStorage->XML_SITES_ROOT before constructing the temp path. If either does
not exist or is not a directory, return/throw a clear error (HTTP 404 or a
domain-specific exception) and avoid proceeding; do not create or operate on
paths when roots are invalid. Use PHP's is_dir (or file_exists + is_dir) to
validate and include the site slug in the error message for diagnostics.
| // move siteFrom to tmp folder | ||
| $siteFromStorage->moveFolderContents( | ||
| $siteFromStorage->XML_STORAGE_ROOT, | ||
| $tmpDir, | ||
| $siteFromStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : [] | ||
| ); | ||
|
|
||
| // move to siteFrom folder from siteTo | ||
| $siteFromStorage->moveFolderContents( | ||
| $siteToStorage->XML_STORAGE_ROOT, | ||
| $siteFromStorage->XML_STORAGE_ROOT, | ||
| $siteToStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : [] | ||
| ); | ||
|
|
||
| // move from tmp to siteTo folder | ||
| $siteFromStorage->moveFolderContents( | ||
| $tmpDir, | ||
| $siteToStorage->XML_STORAGE_ROOT, | ||
| ); | ||
|
|
||
| // delete tmp folder | ||
| $siteFromStorage->delFolder($tmpDir); | ||
|
|
||
| return response()->json($json); |
There was a problem hiding this comment.
Make the three-step swap robust (try/finally + check return values).
Wrap file ops in try/finally to ensure tmp cleanup and fail fast if a move fails. Also use clearer ignore condition for the “to” side.
- // move siteFrom to tmp folder
- $siteFromStorage->moveFolderContents(
+ try {
+ // move siteFrom to tmp folder
+ $ok = $siteFromStorage->moveFolderContents(
$siteFromStorage->XML_STORAGE_ROOT,
$tmpDir,
$siteFromStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : []
- );
+ );
+ if (!$ok) { abort(500, 'Failed moving source to temp.'); }
- // move to siteFrom folder from siteTo
- $siteFromStorage->moveFolderContents(
+ // move to siteFrom folder from siteTo
+ $ok = $siteFromStorage->moveFolderContents(
$siteToStorage->XML_STORAGE_ROOT,
$siteFromStorage->XML_STORAGE_ROOT,
- $siteToStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : []
- );
+ $siteToStorage->XML_STORAGE_ROOT == $siteToStorage->XML_MAIN_ROOT ? ['-sites'] : []
+ );
+ if (!$ok) { abort(500, 'Failed moving destination to source.'); }
- // move from tmp to siteTo folder
- $siteFromStorage->moveFolderContents(
+ // move from tmp to siteTo folder
+ $ok = $siteFromStorage->moveFolderContents(
$tmpDir,
$siteToStorage->XML_STORAGE_ROOT,
- );
-
- // delete tmp folder
- $siteFromStorage->delFolder($tmpDir);
+ );
+ if (!$ok) { abort(500, 'Failed moving temp to destination.'); }
+ } finally {
+ // delete tmp folder
+ $siteFromStorage->delFolder($tmpDir);
+ }Based on coding guidelines.
| $siteFromStorage->moveFolderContents( | ||
| $siteToStorage->XML_STORAGE_ROOT, | ||
| $siteFromStorage->XML_STORAGE_ROOT, | ||
| $siteToStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : [] | ||
| ); |
There was a problem hiding this comment.
Use symmetrical comparison for ignore logic.
Use $siteToStorage’s own main root for clarity.
- $siteToStorage->XML_STORAGE_ROOT == $siteFromStorage->XML_MAIN_ROOT ? ['-sites'] : []
+ $siteToStorage->XML_STORAGE_ROOT == $siteToStorage->XML_MAIN_ROOT ? ['-sites'] : []🤖 Prompt for AI Agents
In _api_app/app/Sites/SitesController.php around lines 126 to 130, the ignore
logic compares $siteToStorage->XML_STORAGE_ROOT to
$siteFromStorage->XML_MAIN_ROOT which is asymmetrical and confusing; change the
comparison to $siteToStorage->XML_MAIN_ROOT so it reads: if
$siteToStorage->XML_STORAGE_ROOT == $siteToStorage->XML_MAIN_ROOT then pass
['-sites'] else [] — keep the rest of the moveFolderContents call unchanged.
| Route::delete('sites', [SitesController::class, 'delete']); | ||
| Route::post('sites/theme-preview', [SitesController::class, 'themePreview'])->name('site_theme_preview'); | ||
| Route::put('sites/theme-apply', [SitesController::class, 'themeApply'])->name('site_theme_apply'); | ||
| Route::put('sites/switch-contents-between-sites', [SitesController::class, 'switchContentsBetweenSites'])->name('site_switch_contents_between_sites'); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Form Request usage in the controller.
The route definition follows the established pattern and is appropriately protected by authentication middleware. However, ensure that the switchContentsBetweenSites controller method uses a Form Request class for validation rather than inline validation.
As per coding guidelines
Run the following script to check if a Form Request is used:
🏁 Script executed:
#!/bin/bash
# Check if switchContentsBetweenSites uses Form Request for validation
rg -nP -A 20 'function\s+switchContentsBetweenSites' _api_app/app/Sites/SitesController.phpLength of output: 839
Create a Form Request class for the switchContentsBetweenSites method.
The method currently uses inline validation by directly accessing $json['siteSlugFrom'] and $json['siteSlugTo'] without validation. Create a dedicated Form Request class (e.g., SwitchContentsBetweenSitesRequest) with appropriate validation rules, then type-hint the controller method parameter accordingly.
| const availableSites = this.sites.filter((s) => s.name !== this.site.name); | ||
| const dialogRef = this.dialog.open<string>(SitesSwitchContentsComponent, { | ||
| data: { | ||
| currentSite: this.site, | ||
| sites: availableSites, | ||
| selectedSiteSlug: availableSites[0].name, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add safety check for empty availableSites array.
Line 210 accesses availableSites[0].name without verifying the array is non-empty. Although the button is only shown when sites.length > 1, the sites input could be undefined, stale, or modified between render and click, leading to a runtime error.
Apply this diff to add a safety check:
switchContentsBetweenOtherSite() {
const availableSites = this.sites.filter((s) => s.name !== this.site.name);
+ if (availableSites.length === 0) {
+ return;
+ }
const dialogRef = this.dialog.open<string>(SitesSwitchContentsComponent, {
data: {
currentSite: this.site,
sites: availableSites,
selectedSiteSlug: availableSites[0].name,
},
});📝 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.
| const availableSites = this.sites.filter((s) => s.name !== this.site.name); | |
| const dialogRef = this.dialog.open<string>(SitesSwitchContentsComponent, { | |
| data: { | |
| currentSite: this.site, | |
| sites: availableSites, | |
| selectedSiteSlug: availableSites[0].name, | |
| }, | |
| }); | |
| switchContentsBetweenOtherSite() { | |
| const availableSites = this.sites.filter((s) => s.name !== this.site.name); | |
| if (availableSites.length === 0) { | |
| return; | |
| } | |
| const dialogRef = this.dialog.open<string>(SitesSwitchContentsComponent, { | |
| data: { | |
| currentSite: this.site, | |
| sites: availableSites, | |
| selectedSiteSlug: availableSites[0].name, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In editor/src/app/sites/site.component.ts around lines 205-212, accessing
availableSites[0].name can throw if availableSites is empty or this.sites is
undefined; ensure availableSites is created safely (e.g. const availableSites =
(this.sites ?? []).filter(s => s.name !== this.site?.name)), compute a safe
fallback selectedSiteSlug using availableSites[0]?.name ?? this.site?.name ?? ''
(or undefined if you prefer), and pass that fallback into the dialog data so you
never index into an empty array.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
editor/src/app/sites/site.component.ts (1)
204-212: Add safety checks for empty or undefined arrays.Line 205 doesn't check if
this.sitesis defined, and line 210 accessesavailableSites[0].namewithout verifying the array is non-empty. Although the template button is conditionally shown whensites.length > 1, thesitesinput could be undefined, stale, or modified between render and click, leading to a runtime error.Apply this diff to add defensive checks:
switchContentsBetweenOtherSite() { - const availableSites = this.sites.filter((s) => s.name !== this.site.name); + const availableSites = (this.sites ?? []).filter((s) => s.name !== this.site.name); + if (availableSites.length === 0) { + return; + } const dialogRef = this.dialog.open<string>(SitesSwitchContentsComponent, { data: { currentSite: this.site, sites: availableSites, selectedSiteSlug: availableSites[0].name, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
editor/src/app/sites/site.component.ts(6 hunks)editor/src/app/sites/sites-state/sites.state.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
editor/src/app/sites/site.component.ts (3)
editor/src/app/sites/sites.component.ts (1)
Component(15-93)editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)editor/src/app/sites/sites-state/sites.actions.ts (1)
SwitchContentsSitesAction(62-65)
editor/src/app/sites/sites-state/sites.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwitchContentsSitesAction(62-65)editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)
🔇 Additional comments (7)
editor/src/app/sites/sites-state/sites.state.ts (2)
28-28: LGTM!The import is correctly added to support the new action handler.
328-340: LGTM! Previous issue resolved.The empty object pattern issue from the previous review has been correctly addressed by using
_to indicate the unused state context parameter. The handler implementation is clean, and the TODO comment aligns with the PR objectives to update state in future work.editor/src/app/sites/site.component.ts (5)
1-25: LGTM!All necessary imports are correctly added to support the new content switching functionality.
120-125: LGTM!The new properties are correctly defined. The
sitesinput will be used for filtering,currentSiteSlug$tracks the active site, and thedialoginjection follows modern Angular patterns.
135-137: LGTM!The constructor correctly initializes the
currentSiteSlug$observable by selecting from the app state.
241-250: Excellent error handling improvement!The error handling has been properly implemented with user-facing feedback via a popup notification. This provides a much better user experience compared to silent console-only errors.
227-239: Temporary workaround acknowledged.The window reload is a temporary solution as noted in the TODO comment and PR objectives. The planned improvement to update state and re-render the iframe instead is tracked for future work.
| @if (sites.length > 1) { | ||
| <button | ||
| title="switch contents between other site" | ||
| (click)="switchContentsBetweenOtherSite()" | ||
| > | ||
| <bt-icon-switch /> | ||
| </button> | ||
| } |
There was a problem hiding this comment.
Add null check for sites input.
The condition checks sites.length > 1 without verifying that sites is defined. If the sites input is undefined or null, this will cause a runtime error.
Apply this diff to add a safety check:
- @if (sites.length > 1) {
+ @if (sites?.length > 1) {
<button
title="switch contents between other site"
(click)="switchContentsBetweenOtherSite()"🤖 Prompt for AI Agents
In editor/src/app/sites/site.component.ts around lines 65 to 72, the template
condition uses sites.length > 1 without guarding against sites being
null/undefined; update the condition to first check that sites is defined (e.g.,
sites && sites.length > 1 or using optional chaining like sites?.length > 1) so
the button render is skipped when sites is null/undefined, preventing runtime
errors.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
editor/src/app/sites/sections/tags/section-tags.actions.ts(1 hunks)editor/src/app/sites/sections/tags/section-tags.state.ts(2 hunks)editor/src/app/sites/sites-state/sites.state.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/app/sites/sites-state/sites.state.ts
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/sites/sections/tags/section-tags.state.ts (2)
editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
SwitchContentsSitesTagsAction(60-63)editor/src/app/sites/sections/tags/section-tags-state.model.ts (1)
SectionTagsStateModel(1-5)
🔇 Additional comments (2)
editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
60-63: LGTM!The action class follows the established patterns in the file. The type string is consistent with other site-level actions, and the payload structure clearly captures the two sites involved in the swap operation.
editor/src/app/sites/sections/tags/section-tags.state.ts (1)
30-30: LGTM!The import statement correctly adds the new action class.
| @Action(SwitchContentsSitesTagsAction) | ||
| switchContentsSitesTags( | ||
| { getState, setState }: StateContext<SectionTagsStateModel>, | ||
| action: SwitchContentsSitesTagsAction, | ||
| ) { | ||
| const state = getState(); | ||
| setState({ | ||
| ...state, | ||
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | ||
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add validation to ensure both sites exist before swapping.
The handler doesn't verify that both siteSlugFrom and siteSlugTo exist in the state before performing the swap. If either site is missing, undefined will be assigned to the other site, potentially causing issues downstream.
Add defensive checks consistent with other handlers in this file:
@Action(SwitchContentsSitesTagsAction)
switchContentsSitesTags(
{ getState, setState }: StateContext<SectionTagsStateModel>,
action: SwitchContentsSitesTagsAction,
) {
const state = getState();
+
+ if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) {
+ return;
+ }
+
setState({
...state,
[action.payload.siteSlugFrom]: state[action.payload.siteSlugTo],
[action.payload.siteSlugTo]: state[action.payload.siteSlugFrom],
});
}📝 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.
| @Action(SwitchContentsSitesTagsAction) | |
| switchContentsSitesTags( | |
| { getState, setState }: StateContext<SectionTagsStateModel>, | |
| action: SwitchContentsSitesTagsAction, | |
| ) { | |
| const state = getState(); | |
| setState({ | |
| ...state, | |
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | |
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | |
| }); | |
| } | |
| @Action(SwitchContentsSitesTagsAction) | |
| switchContentsSitesTags( | |
| { getState, setState }: StateContext<SectionTagsStateModel>, | |
| action: SwitchContentsSitesTagsAction, | |
| ) { | |
| const state = getState(); | |
| if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) { | |
| return; | |
| } | |
| setState({ | |
| ...state, | |
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | |
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | |
| }); | |
| } |
🤖 Prompt for AI Agents
In editor/src/app/sites/sections/tags/section-tags.state.ts around lines 236 to
247, the SwitchContentsSitesTagsAction handler performs a blind swap and can
write undefined if either siteSlugFrom or siteSlugTo is missing; add defensive
validation by reading state, checking that state[action.payload.siteSlugFrom]
and state[action.payload.siteSlugTo] both exist before calling setState, and if
either is missing simply return (or optionally log/throw consistent with other
handlers in this file) so the state is not mutated incorrectly; then perform the
swap only when both entries are present.
…vigation. Improve dialog UX.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
editor/src/app/sites/sections/tags/section-tags.state.ts (1)
236-247: Add validation to ensure both sites exist before swapping.The handler doesn't verify that both
siteSlugFromandsiteSlugToexist in the state before performing the swap. If either site is missing,undefinedwill be assigned to the other site, potentially causing issues downstream.Add defensive checks:
@Action(SwapContentsSitesTagsAction) swapContentsSitesTags( { getState, setState }: StateContext<SectionTagsStateModel>, action: SwapContentsSitesTagsAction, ) { const state = getState(); + + if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) { + return; + } + setState({ ...state, [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], }); }editor/src/app/sites/site.component.ts (2)
65-72: Add null check forsitesinput.The condition checks
sites.length > 1without verifying thatsitesis defined. If thesitesinput is undefined or null, this will cause a runtime error.Apply this diff to add a safety check:
- @if (sites.length > 1) { + @if (sites?.length > 1) { <button title="swap content between other site" (click)="swapContentsBetweenOtherSite()" > <bt-icon-switch /> </button> }
204-212: Add safety check for emptyavailableSitesarray.Line 210 accesses
availableSites[0].namewithout verifying the array is non-empty or thatthis.sitesis defined. Although the button is only shown whensites.length > 1, defensive programming is warranted.Apply this diff to add safety checks:
swapContentsBetweenOtherSite() { - const availableSites = this.sites.filter((s) => s.name !== this.site.name); + const availableSites = (this.sites ?? []).filter((s) => s.name !== this.site.name); + if (availableSites.length === 0) { + return; + } + const dialogRef = this.dialog.open<string>(SitesSwapContentsComponent, { data: { currentSite: this.site, sites: availableSites, selectedSiteSlug: availableSites[0].name, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
_api_app/app/Http/Controllers/StateController.php(1 hunks)_api_app/app/Sites/SitesController.php(1 hunks)_api_app/routes/api.php(1 hunks)editor/src/app/sites/sections/tags/section-tags.actions.ts(1 hunks)editor/src/app/sites/sections/tags/section-tags.state.ts(2 hunks)editor/src/app/sites/shared/icon-switch.component.ts(1 hunks)editor/src/app/sites/shared/sites-shared.module.ts(3 hunks)editor/src/app/sites/site.component.ts(6 hunks)editor/src/app/sites/sites-state/sites.actions.ts(1 hunks)editor/src/app/sites/sites-state/sites.state.ts(3 hunks)editor/src/app/sites/sites-swap-contents.component.ts(1 hunks)editor/src/app/sites/sites.module.ts(3 hunks)editor/src/styles/_dialog.scss(1 hunks)editor/src/styles/_utils.scss(1 hunks)editor/src/styles/styles.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- editor/src/styles/_utils.scss
🚧 Files skipped from review as they are similar to previous changes (9)
- editor/src/app/sites/shared/icon-switch.component.ts
- _api_app/routes/api.php
- editor/src/app/sites/sections/tags/section-tags.actions.ts
- _api_app/app/Sites/SitesController.php
- _api_app/app/Http/Controllers/StateController.php
- editor/src/styles/_dialog.scss
- editor/src/app/sites/sites.module.ts
- editor/src/app/sites/sites-state/sites.actions.ts
- editor/src/styles/styles.scss
🧰 Additional context used
🧬 Code graph analysis (4)
editor/src/app/sites/site.component.ts (5)
editor/src/app/sites/sites-swap-contents.component.ts (1)
Component(4-44)editor/src/app/sites/sites.component.ts (1)
Component(15-93)editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
editor/src/app/sites/sites-state/sites.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
SwapContentsSitesTagsAction(60-63)
editor/src/app/sites/sites-swap-contents.component.ts (2)
editor/src/app/sites/shared/icon-switch.component.ts (1)
Component(3-21)editor/src/app/sites/site.component.ts (1)
Component(27-292)
editor/src/app/sites/sections/tags/section-tags.state.ts (2)
editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
SwapContentsSitesTagsAction(60-63)editor/src/app/sites/sections/tags/section-tags-state.model.ts (1)
SectionTagsStateModel(1-5)
🔇 Additional comments (2)
editor/src/app/sites/shared/sites-shared.module.ts (1)
30-30: LGTM!The IconSwapComponent has been correctly imported, declared, and exported following Angular module patterns.
Also applies to: 64-64, 90-90
editor/src/app/sites/site.component.ts (1)
227-237: Page reload workaround masks incomplete state management.The full page reload is used because the state swap actions are incomplete (only tags are currently swapped in
sites.state.tslines 344-355). This prevents proper reactive UI updates and creates a poor user experience.As noted in the PR objectives, this is a known limitation. However, verify that:
- All necessary state swap actions are implemented before removing the reload
- The iframe can be selectively reloaded/re-rendered instead of the entire page
- Consider dispatching a verification action after swap to ensure state consistency
This is related to the incomplete implementation flagged in
sites.state.tslines 345-352 where multiple swap actions remain commented out.
| export class SitesSwapContentsComponent { | ||
| dialogRef = inject<DialogRef<string>>(DialogRef<string>); | ||
| data = inject(DIALOG_DATA); | ||
| constructor() {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type safety for injected data and remove unnecessary constructor.
The DIALOG_DATA injection lacks type safety, and the empty constructor serves no purpose.
Define an interface for the dialog data and apply proper typing:
+interface SitesSwapDialogData {
+ currentSite: SiteStateModel;
+ sites: SiteStateModel[];
+ selectedSiteSlug: string;
+}
+
export class SitesSwapContentsComponent {
dialogRef = inject<DialogRef<string>>(DialogRef<string>);
- data = inject(DIALOG_DATA);
- constructor() {}
+ data = inject<SitesSwapDialogData>(DIALOG_DATA);
}You'll need to import SiteStateModel:
import { SiteStateModel } from './sites-state/site-state.model';🤖 Prompt for AI Agents
In editor/src/app/sites/sites-swap-contents.component.ts around lines 40 to 43,
the injected DIALOG_DATA is untyped and the empty constructor is unnecessary;
define a proper interface (or use the provided SiteStateModel) for the dialog
payload, import SiteStateModel from './sites-state/site-state.model', change the
injection to inject<DIALOG_DATA_TYPE>(DIALOG_DATA) with that type, and remove
the empty constructor so the class only contains the typed dialogRef and typed
data properties.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
editor/src/app/sites/sections/sections-state/site-sections.actions.ts(1 hunks)editor/src/app/sites/sections/sections-state/site-sections.state.ts(2 hunks)editor/src/app/sites/sections/tags/section-tags.actions.ts(1 hunks)editor/src/app/sites/sites-state/sites.state.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/app/sites/sites-state/sites.state.ts
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (3)
editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
SwapContentsSitesSectionsAction(105-108)editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)_api_app/app/Sites/Sections/SiteSectionsDataService.php (1)
getState(203-212)
🔇 Additional comments (3)
editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
60-63: LGTM!The new action class follows the established patterns in this file. The payload structure is clear and type-safe, enabling content swapping between two site slugs at the tag level.
editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
105-108: LGTM!The new action class follows the established patterns in this file. The payload structure is consistent with the parallel
SwapContentsSitesTagsActionin the tags actions file, providing a uniform interface for the swap operation.editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
42-42: Import added correctly.The import statement properly adds the new
SwapContentsSitesSectionsActionto support the swap functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts(1 hunks)editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts(2 hunks)editor/src/app/sites/sections/sections-state/site-sections.state.ts(2 hunks)editor/src/app/sites/settings/site-settings.actions.ts(1 hunks)editor/src/app/sites/settings/site-settings.state.ts(2 hunks)editor/src/app/sites/sites-state/sites.state.ts(2 hunks)editor/src/app/sites/template-settings/site-template-settings.actions.ts(1 hunks)editor/src/app/sites/template-settings/site-template-settings.state.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (3)
editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
SwapContentsSitesSectionsAction(105-108)editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)_api_app/app/Sites/Sections/SiteSectionsDataService.php (1)
getState(203-212)
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (3)
editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
SwapContentsSitesSectionsEntriesAction(65-68)editor/src/app/sites/sections/entries/entries-state/section-entries-state.model.ts (1)
SectionEntriesStateModel(1-3)_api_app/app/Sites/Sections/Entries/SectionEntriesDataService.php (1)
getState(330-341)
editor/src/app/sites/sites-state/sites.state.ts (6)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
SwapContentsSitesSectionsAction(105-108)editor/src/app/sites/settings/site-settings.actions.ts (1)
SwapContentsSitesSettingsAction(48-51)editor/src/app/sites/template-settings/site-template-settings.actions.ts (1)
SwapContentsSitesTemplateSettingsAction(39-42)editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
SwapContentsSitesTagsAction(60-63)editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
SwapContentsSitesSectionsEntriesAction(65-68)
editor/src/app/sites/settings/site-settings.state.ts (2)
editor/src/app/sites/settings/site-settings.actions.ts (1)
SwapContentsSitesSettingsAction(48-51)editor/src/app/sites/settings/site-settings.interface.ts (1)
SitesSettingsStateModel(3-5)
editor/src/app/sites/template-settings/site-template-settings.state.ts (2)
editor/src/app/sites/template-settings/site-template-settings.actions.ts (1)
SwapContentsSitesTemplateSettingsAction(39-42)editor/src/app/sites/template-settings/site-template-settings.interface.ts (1)
SitesTemplateSettingsStateModel(3-5)
🔇 Additional comments (5)
editor/src/app/sites/settings/site-settings.actions.ts (1)
48-51: LGTM! Action structure is consistent.The action follows the established pattern and naming conventions for swap operations across the codebase.
editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
65-68: LGTM! Consistent action definition.The action structure aligns well with the other swap actions introduced in this PR.
editor/src/app/sites/template-settings/site-template-settings.actions.ts (1)
39-42: LGTM! Action follows established pattern.Consistent with other swap actions across different state domains.
editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
632-648: LGTM! Swap logic correctly handles array-based state.The implementation correctly maps over sections and swaps the
site_namefield. This approach is appropriate for the array-based state structure and avoids the undefined issues present in object-based swaps.editor/src/app/sites/sites-state/sites.state.ts (1)
333-364: Good coordination of swap operations, but clarify the TODO.The implementation correctly coordinates the swap by calling the API first, then dispatching all relevant state update actions. The cascading state updates across different domains (sections, settings, template settings, tags, entries) ensure consistency.
However, the TODO comment on line 359 is vague. Consider clarifying what shop state updates are needed:
- // TODO: make sure shop states are required to update as well + // TODO: Verify if shop-specific state (e.g., products, orders) needs swap actions + // If shop data is tied to sites, add SwapContentsShopAction hereAlso, ensure the guards I suggested for the individual swap handlers are added to prevent undefined state corruption if the API returns an unexpected response.
| @Action(SwapContentsSitesSectionsEntriesAction) | ||
| swapContentsSitesSectionsEntries( | ||
| { getState, setState }: StateContext<SectionEntriesStateModel>, | ||
| action: SwapContentsSitesSectionsEntriesAction, | ||
| ) { | ||
| const state = getState(); | ||
| setState({ | ||
| ...state, | ||
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | ||
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add guard to prevent undefined state corruption.
Similar to the site-settings swap handler, if either site doesn't exist in the state, the swap will assign undefined to the other site. Add a guard to prevent this.
Apply this fix:
@Action(SwapContentsSitesSectionsEntriesAction)
swapContentsSitesSectionsEntries(
{ getState, setState }: StateContext<SectionEntriesStateModel>,
action: SwapContentsSitesSectionsEntriesAction,
) {
const state = getState();
+ // Guard against swapping with non-existent sites
+ if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) {
+ console.error('Cannot swap entries: one or both sites do not exist in state');
+ return;
+ }
setState({
...state,
[action.payload.siteSlugFrom]: state[action.payload.siteSlugTo],
[action.payload.siteSlugTo]: state[action.payload.siteSlugFrom],
});
}🤖 Prompt for AI Agents
In editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts
around lines 333 to 344, the swap handler can assign undefined when one of the
site keys is missing; add a guard that reads the current state, checks that both
state[action.payload.siteSlugFrom] and state[action.payload.siteSlugTo] are
defined, and if either is missing simply return (or optionally log/throw)
without mutating state; only perform the setState swap when both entries exist
to avoid corrupting the state.
| @Action(SwapContentsSitesSettingsAction) | ||
| swapContentsSitesSettings( | ||
| { getState, setState }: StateContext<SitesSettingsStateModel>, | ||
| action: SwapContentsSitesSettingsAction, | ||
| ) { | ||
| const state = getState(); | ||
| setState({ | ||
| ...state, | ||
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | ||
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add guard to prevent undefined state corruption.
If either siteSlugFrom or siteSlugTo doesn't exist in the state, the swap will assign undefined to the other site, potentially corrupting the state. While the API likely validates site existence, defensive programming would prevent edge cases.
Consider adding a guard:
@Action(SwapContentsSitesSettingsAction)
swapContentsSitesSettings(
{ getState, setState }: StateContext<SitesSettingsStateModel>,
action: SwapContentsSitesSettingsAction,
) {
const state = getState();
+ // Guard against swapping with non-existent sites
+ if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) {
+ console.error('Cannot swap: one or both sites do not exist in state');
+ return;
+ }
setState({
...state,
[action.payload.siteSlugFrom]: state[action.payload.siteSlugTo],
[action.payload.siteSlugTo]: state[action.payload.siteSlugFrom],
});
}📝 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.
| @Action(SwapContentsSitesSettingsAction) | |
| swapContentsSitesSettings( | |
| { getState, setState }: StateContext<SitesSettingsStateModel>, | |
| action: SwapContentsSitesSettingsAction, | |
| ) { | |
| const state = getState(); | |
| setState({ | |
| ...state, | |
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | |
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | |
| }); | |
| } | |
| @Action(SwapContentsSitesSettingsAction) | |
| swapContentsSitesSettings( | |
| { getState, setState }: StateContext<SitesSettingsStateModel>, | |
| action: SwapContentsSitesSettingsAction, | |
| ) { | |
| const state = getState(); | |
| // Guard against swapping with non-existent sites | |
| if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) { | |
| console.error('Cannot swap: one or both sites do not exist in state'); | |
| return; | |
| } | |
| setState({ | |
| ...state, | |
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | |
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | |
| }); | |
| } |
🤖 Prompt for AI Agents
In editor/src/app/sites/settings/site-settings.state.ts around lines 461 to 472,
the swapContentsSitesSettings action can assign undefined if either siteSlugFrom
or siteSlugTo is missing; add a guard that reads the current state, checks that
both state[action.payload.siteSlugFrom] and state[action.payload.siteSlugTo] are
defined, and if either is missing return early (no-op) or setState unchanged (or
optionally log/throw an error) to avoid corrupting the state; only perform the
swap and call setState when both entries exist.
| @Action(SwapContentsSitesTemplateSettingsAction) | ||
| swapContentsSitesTemplateSettings( | ||
| { getState, setState }: StateContext<SitesTemplateSettingsStateModel>, | ||
| action: SwapContentsSitesTemplateSettingsAction, | ||
| ) { | ||
| const state = getState(); | ||
| setState({ | ||
| ...state, | ||
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | ||
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add guard to prevent undefined state corruption.
Same issue as the other swap handlers: if either site doesn't exist in the state, the swap will assign undefined to the other site.
Apply this fix:
@Action(SwapContentsSitesTemplateSettingsAction)
swapContentsSitesTemplateSettings(
{ getState, setState }: StateContext<SitesTemplateSettingsStateModel>,
action: SwapContentsSitesTemplateSettingsAction,
) {
const state = getState();
+ // Guard against swapping with non-existent sites
+ if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) {
+ console.error('Cannot swap template settings: one or both sites do not exist in state');
+ return;
+ }
setState({
...state,
[action.payload.siteSlugFrom]: state[action.payload.siteSlugTo],
[action.payload.siteSlugTo]: state[action.payload.siteSlugFrom],
});
}📝 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.
| @Action(SwapContentsSitesTemplateSettingsAction) | |
| swapContentsSitesTemplateSettings( | |
| { getState, setState }: StateContext<SitesTemplateSettingsStateModel>, | |
| action: SwapContentsSitesTemplateSettingsAction, | |
| ) { | |
| const state = getState(); | |
| setState({ | |
| ...state, | |
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | |
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | |
| }); | |
| } | |
| @Action(SwapContentsSitesTemplateSettingsAction) | |
| swapContentsSitesTemplateSettings( | |
| { getState, setState }: StateContext<SitesTemplateSettingsStateModel>, | |
| action: SwapContentsSitesTemplateSettingsAction, | |
| ) { | |
| const state = getState(); | |
| // Guard against swapping with non-existent sites | |
| if (!state[action.payload.siteSlugFrom] || !state[action.payload.siteSlugTo]) { | |
| console.error('Cannot swap template settings: one or both sites do not exist in state'); | |
| return; | |
| } | |
| setState({ | |
| ...state, | |
| [action.payload.siteSlugFrom]: state[action.payload.siteSlugTo], | |
| [action.payload.siteSlugTo]: state[action.payload.siteSlugFrom], | |
| }); | |
| } |
🤖 Prompt for AI Agents
In editor/src/app/sites/template-settings/site-template-settings.state.ts around
lines 230 to 241, the swapContentsSitesTemplateSettings action unconditionally
assigns values from possibly-missing keys, which can write undefined into the
state; add a guard that checks both state[action.payload.siteSlugFrom] and
state[action.payload.siteSlugTo] are defined before performing the swap and
otherwise exit early (leave state unchanged) to prevent corrupting the state.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
editor/src/app/sites/site.component.ts (3)
65-72: Add optional chaining to prevent runtime errors.The condition
sites.length > 1will throw a runtime error ifsitesis undefined or null. This was flagged in a previous review and remains unaddressed.Apply this diff:
-@if (sites.length > 1) { +@if (sites?.length > 1) { <button title="swap content between other site" (click)="swapContentsBetweenOtherSite()"
204-212: Add safety checks for array operations.Two issues remain from previous reviews:
- Line 205:
this.sites.filter()will throw ifsitesis undefined- Line 210: Accessing
availableSites[0].namewithout verifying the array is non-empty can cause a runtime errorWhile the button is conditionally rendered when
sites.length > 1, thesitesinput could still be undefined, stale, or modified between render and click.Apply this diff:
swapContentsBetweenOtherSite() { - const availableSites = this.sites.filter((s) => s.name !== this.site.name); + const availableSites = (this.sites ?? []).filter((s) => s.name !== this.site.name); + if (availableSites.length === 0) { + return; + } const dialogRef = this.dialog.open<string>(SitesSwapContentsComponent, { data: { currentSite: this.site, sites: availableSites, selectedSiteSlug: availableSites[0].name, }, });
119-120: Add null safety forsitesinput.The
sitesinput is not marked as optional or given a default value, but it's accessed in multiple places without null checks (lines 65, 205). If the parent component doesn't provide this input, runtime errors will occur.Apply this diff to add a default value:
-@Input('sites') sites: SiteStateModel[]; +@Input('sites') sites: SiteStateModel[] = [];Or alternatively, add optional chaining at usage sites (lines 65 and 205).
🧹 Nitpick comments (1)
editor/src/app/sites/site.component.ts (1)
228-230: Consider adding navigation options for consistency.The navigation after a successful swap doesn't use
replaceUrl: trueorqueryParamsHandling: 'merge', which are used in other navigation calls in this component (e.g., lines 168-171). This inconsistency might cause unexpected behavior or loss of query parameters.Apply this diff for consistency:
this.router.navigate(['/multisite'], { + replaceUrl: true, queryParams: { site: this.site.name }, + queryParamsHandling: 'merge', });Note: Since a full page reload is triggered by
SwapContentsSitesAction(seetemplate-rerender.service.ts), the navigation might not complete before the reload occurs. Consider whether the navigation is necessary or if the reload alone is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.vscode/extensions.json(1 hunks)_api_app/app/User/UserAuthServiceProvider.php(1 hunks)editor/src/app/preview/preview.component.ts(2 hunks)editor/src/app/rerender/template-rerender.service.ts(3 hunks)editor/src/app/sites/site.component.ts(6 hunks)
✅ Files skipped from review due to trivial changes (3)
- _api_app/app/User/UserAuthServiceProvider.php
- editor/src/app/preview/preview.component.ts
- .vscode/extensions.json
🧰 Additional context used
🧬 Code graph analysis (2)
editor/src/app/rerender/template-rerender.service.ts (1)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
editor/src/app/sites/site.component.ts (3)
editor/src/app/sites/sites-swap-contents.component.ts (1)
Component(4-44)editor/src/app/sites/sites.component.ts (1)
Component(15-93)editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
🔇 Additional comments (5)
editor/src/app/rerender/template-rerender.service.ts (3)
47-47: LGTM! Import properly added.The import of
SwapContentsSitesActionis correctly placed and follows the existing pattern.
99-114: LGTM! Action properly registered for reload trigger.The
SwapContentsSitesActionis correctly added to theofActionSuccessfuloperator, ensuring the reload logic is triggered when the action completes successfully.
160-171: LGTM! Reload condition properly implemented.The reload condition for
SwapContentsSitesActionfollows the established pattern and correctly triggers a full page reload when the action succeeds.editor/src/app/sites/site.component.ts (2)
124-137: LGTM! Observable and dependency injection properly implemented.The
currentSiteSlug$observable anddialogservice are correctly initialized and follow Angular best practices.
232-241: LGTM! Error handling properly implemented.The error handling now provides user-facing feedback via a popup notification, addressing the previous review comment. Users will be informed if the content swap fails.
There was a problem hiding this comment.
Actionable comments posted: 2
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/settings/shop-settings.state.ts (1)
1-11: Use distinctUntilChanged instead of distinct to avoid suppressing future site revisits.distinct caches all seen keys; switching back to a previously visited site won’t re‑init. Import distinctUntilChanged instead.
Apply this diff to the operators import:
import { take, tap, catchError, filter, switchMap, map, - distinct, + distinctUntilChanged, startWith, } from 'rxjs/operators';
🧹 Nitpick comments (1)
editor/src/app/shop/settings/shop-settings.state.ts (1)
90-92: Drop counter; mark DestroyRef as readonly.Counter becomes unnecessary with distinctUntilChanged; also prefer readonly for DI fields.
- private destroyRef = inject(DestroyRef); - private counter = 0; + private readonly destroyRef = inject(DestroyRef);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/src/app/shop/settings/shop-settings.state.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/shop/settings/shop-settings.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/user/user.state.ts (1)
hasFeatureShop(54-56)
🔇 Additional comments (6)
editor/src/app/shop/settings/shop-settings.state.ts (6)
19-21: Actions stream import looks good.
95-95: Injecting Actions is correct for reacting to SwapContentsSitesAction.
105-109: Init trigger via startWith(null) is fine.This ensures combineLatest emits immediately.
112-116: Mapping action to a boolean flag is clear.No issues here.
130-130: Proper teardown with takeUntilDestroyed.Good lifecycle hygiene.
122-129: Code change verified as correct.The
ShopStateService.getInitialStatesignature and all call sites across the codebase are consistent. The call at line 124 correctly passesisSwapSitesContentsActionas the third parameter (force: boolean), matching the method signaturegetInitialState(site: string = '', section?: string, force = false).
| tap(() => { | ||
| console.log('>>>', this.counter); | ||
| }), | ||
| distinct(({ site, isSwapSitesContentsAction }) => | ||
| isSwapSitesContentsAction ? this.counter++ : site, | ||
| ), |
There was a problem hiding this comment.
Fix init suppression and remove debug log/side‑effects.
- console.log should not ship.
- distinct with a side‑effectful counter is brittle and still suppresses revisits to the same site later.
- Use distinctUntilChanged with a comparator: only suppress consecutive identical sites unless a swap action occurred.
- tap(() => {
- console.log('>>>', this.counter);
- }),
- distinct(({ site, isSwapSitesContentsAction }) =>
- isSwapSitesContentsAction ? this.counter++ : site,
- ),
+ distinctUntilChanged((prev, curr) =>
+ !curr.isSwapSitesContentsAction && prev.site === curr.site,
+ ),Note: This relies on the import change to distinctUntilChanged above.
📝 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.
| tap(() => { | |
| console.log('>>>', this.counter); | |
| }), | |
| distinct(({ site, isSwapSitesContentsAction }) => | |
| isSwapSitesContentsAction ? this.counter++ : site, | |
| ), | |
| distinctUntilChanged((prev, curr) => | |
| !curr.isSwapSitesContentsAction && prev.site === curr.site, | |
| ), |
🤖 Prompt for AI Agents
In editor/src/app/shop/settings/shop-settings.state.ts around lines 116 to 121,
remove the debug console.log and the side‑effectful use of this.counter in
distinct; instead switch to distinctUntilChanged with a comparator that returns
false (allowing emission) when isSwapSitesContentsAction is true and otherwise
compares the previous and current site for equality so only consecutive
identical sites are suppressed; ensure you remove the counter increment and any
references to it, and rely on the updated import of distinctUntilChanged as
noted.
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/shop/settings/shop-settings.state.ts (1)
2-11: Replacedistinct()withdistinctUntilChanged()comparator and remove counter field to fix event suppression logic.The
distinct()operator maintains a global Set and prevents re-emitting the same site permanently. The counter side-effect only masks this for swap actions—after A→B→A, site A won't reinitialize becausedistinct()doesn't keep history but relies on the Set. Replace withdistinctUntilChanged()using a comparator that suppresses only consecutive duplicates unless there's a swap action.Apply changes at:
- Lines 2-11: Replace import
distinctwithdistinctUntilChanged- Line 91: Remove
private counter = 0;- Lines 116–117: Replace the
distinct()call withdistinctUntilChanged((prev, curr) => !curr.isSwapSitesContentsAction && prev.site === curr.site,)
♻️ Duplicate comments (1)
editor/src/app/shop/settings/shop-settings.state.ts (1)
36-41: Remove unused Subject import.Subject isn’t used; drop it to satisfy linters and reduce noise.
-import { combineLatest, Subject } from 'rxjs'; +import { combineLatest } from 'rxjs';
🧹 Nitpick comments (1)
editor/src/app/shop/settings/shop-settings.state.ts (1)
119-126: Harden pipeline: handle getInitialState errors to avoid stream teardown.Without catchError here, a single backend failure completes the stream; subsequent swaps/site changes won’t reload settings. Return EMPTY to keep the subscription alive.
@@ - this.stateService + this.stateService .getInitialState(site, 'settings', isSwapSitesContentsAction) .pipe( take(1), map((settings) => ({ site, settings })), + catchError((err) => { + console.error(err); + return EMPTY; + }), ),And import EMPTY:
-import { combineLatest } from 'rxjs'; +import { combineLatest, EMPTY } from 'rxjs';Also applies to: 36-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/src/app/shop/settings/shop-settings.state.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/shop/settings/shop-settings.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/user/user.state.ts (1)
hasFeatureShop(54-56)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
editor/src/app/shop/settings/shop-settings.state.ts (1)
116-118: ReplacedistinctwithdistinctUntilChangedto fix the side-effectful counter logic.The current implementation uses a side-effectful counter increment inside
distinct, which violates RxJS best practices and won't properly handle revisiting the same site after subsequent swap actions. This issue was previously flagged but remains unresolved.Apply this diff to fix the logic:
- distinct(({ site, isSwapSitesContentsAction }) => - isSwapSitesContentsAction ? this.counter++ : site, - ), + distinctUntilChanged((prev, curr) => + !curr.isSwapSitesContentsAction && prev.site === curr.site, + ),This comparator properly suppresses only consecutive identical sites unless a swap action occurred, without side effects.
Based on learnings from past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/src/app/shop/settings/shop-settings.state.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/shop/settings/shop-settings.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/user/user.state.ts (1)
hasFeatureShop(54-56)
| distinct, | ||
| startWith, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import distinctUntilChanged instead of distinct for the fix below.
To properly handle the duplicate site initialization issue, you'll need to replace distinct with distinctUntilChanged in the imports.
Apply this diff:
map,
- distinct,
+ distinctUntilChanged,
startWith,
} from 'rxjs/operators';Also applies to: 19-20
🤖 Prompt for AI Agents
In editor/src/app/shop/settings/shop-settings.state.ts around lines 9-10 and
also update lines 19-20, replace the RxJS import named "distinct" with
"distinctUntilChanged" so the code imports distinctUntilChanged instead; update
any corresponding import specifiers to use distinctUntilChanged and adjust any
usages if necessary to call distinctUntilChanged to prevent duplicate site
initialization.
| private destroyRef = inject(DestroyRef); | ||
| private counter = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the counter field once the distinct operator is fixed.
The counter field is only used for the side-effectful workaround in the distinct operator. Once you switch to distinctUntilChanged (as suggested below), this field will be unused and should be removed.
Apply this diff after fixing the distinct operator:
private destroyRef = inject(DestroyRef);
- private counter = 0;
-📝 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.
| private destroyRef = inject(DestroyRef); | |
| private counter = 0; | |
| private destroyRef = inject(DestroyRef); |
🤖 Prompt for AI Agents
In editor/src/app/shop/settings/shop-settings.state.ts around lines 90 to 91,
the private counter field is a temporary workaround for the current distinct
operator; after you replace the workaround with distinctUntilChanged (as
suggested elsewhere) remove the counter declaration and any references to it.
Delete the line declaring "private counter = 0;" and ensure no other code
depends on it so the class no longer contains this unused field.
… and update the state accordingly
There was a problem hiding this comment.
Actionable comments posted: 5
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/orders/shop-orders.state.ts (1)
85-87: Fix payload shape: remove site indexing from orders.The issue is confirmed.
getInitialState(site, 'orders')returnsstateCache['orders']directly—the orders section value, not keyed by site. The code then attemptsorders[site], which will be undefined.The action handler's
patchState(action.payload)expects{ [site]: <orders_data> }, so the fix is to useordersdirectly instead oforders[site]:dispatch(new InitShopOrdersAction({ [site]: orders }));
🧹 Nitpick comments (7)
editor/src/app/shop/orders/shop-orders.state.ts (1)
7-8: Optional: import startWith from 'rxjs` for consistency with RxJS 7+ style.Not required, but many code paths import startWith from 'rxjs'. Align if you’re standardizing across modules.
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (3)
78-80: Keep destroyRef/actions$; drop counter after fixing distinct logic.destroyRef + actions$ are appropriate. The counter is only used to game distinct; after switching to distinctUntilChanged you won’t need it.
Also applies to: 85-86
106-113: Keep stream alive on fetch errors. Add catchError (and import EMPTY).Without catchError, any error from getInitialState tears down the subscription; future site changes or swaps won’t refetch.
Apply:
-import { combineLatest } from 'rxjs'; +import { combineLatest, EMPTY } from 'rxjs'; @@ switchMap(({ site, isSwapSitesContentsAction }) => this.stateService .getInitialState(site, 'regionalCosts', isSwapSitesContentsAction) .pipe( take(1), - map((regionalCosts) => ({ site, regionalCosts })), + map((regionalCosts) => ({ site, regionalCosts })), + catchError((err) => { + console.error(err); + return EMPTY; + }), ), ), takeUntilDestroyed(this.destroyRef),Also applies to: 114-114
92-96: Optional: extract swapTrigger$ for clarity.Minor readability improvement; behavior unchanged.
- combineLatest([ - this.store$.select(AppState.getSite), - this.store$.select(UserState.hasFeatureShop), - this.actions$.pipe( - ofActionSuccessful(SwapContentsSitesAction), - startWith(null), // Emit initial value so combineLatest starts immediately - ), - ]) + const swapTrigger$ = this.actions$.pipe( + ofActionSuccessful(SwapContentsSitesAction), + map(() => true), + startWith(false), + ); + + combineLatest([ + this.store$.select(AppState.getSite), + this.store$.select(UserState.hasFeatureShop), + swapTrigger$, + ]) @@ - map(([site, , action]) => ({ + map(([site, , isSwapSitesContentsAction]) => ({ site, - isSwapSitesContentsAction: action !== null, + isSwapSitesContentsAction, })),Also applies to: 99-103
editor/src/app/shop/products/shop-products.state.ts (3)
104-106: Replace distinct + counter with distinctUntilChanged (no side effects, no key cache growth).Avoid mutable state in operator and unbounded key retention from distinct().
Apply:
- distinct(({ site, forceRefresh }) => - forceRefresh ? this.counter++ : site, - ), + distinctUntilChanged( + (prev, curr) => prev.site === curr.site && !curr.forceRefresh, + ),Also update imports (see separate note) and drop the counter field.
13-21: Adjust RxJS imports for the above change.Apply:
switchMap, - distinct, map, startWith, + distinctUntilChanged,
67-69: Remove the counter field (no longer needed).Apply:
private destroyRef = inject(DestroyRef); - private counter = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
editor/src/app/shop/orders/shop-orders.state.ts(3 hunks)editor/src/app/shop/products/shop-products.state.ts(4 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
editor/src/app/shop/products/shop-products.state.ts (2)
editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
UpdateSectionEntryFromSyncAction(87-94)editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/user/user.state.ts (1)
hasFeatureShop(54-56)
editor/src/app/shop/orders/shop-orders.state.ts (1)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
🔇 Additional comments (4)
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (3)
9-10: Imports look correct and consistent.No issues with the added operators, NGXS Actions utilities, or Angular interop helpers.
Also applies to: 19-21, 40-43
92-96: Action trigger setup is solid.ofActionSuccessful(SwapContentsSitesAction) with startWith(null) correctly seeds combineLatest.
106-113: No issues found - code is correct.The dispatch correctly accesses
regionalCosts[site]because the HTTP endpoint (/_api/v1/plugin/shopwith site query parameter) returns an object keyed by site. The same pattern is consistently used across shop-orders.state.ts, shop-products.state.ts, and shop-settings.state.ts, all following this design.editor/src/app/shop/products/shop-products.state.ts (1)
34-37: Good use of DestroyRef + takeUntilDestroyed for cleanup.Pattern is compatible with Angular ≥16; keep it.
| import { | ||
| take, | ||
| filter, | ||
| switchMap, | ||
| distinct, | ||
| map, | ||
| startWith, | ||
| } from 'rxjs/operators'; |
There was a problem hiding this comment.
Replace distinct with distinctUntilChanged; remove counter. Current logic suppresses future same-site inits and grows an unbounded key set.
distinct remembers all keys forever, so returning to a previously seen site (without a swap) will never re-init. It also stores every site/counter key. Use distinctUntilChanged with a comparator that only suppresses consecutive duplicates when no swap occurred. The counter becomes unnecessary.
Apply:
import {
take,
filter,
switchMap,
- distinct,
+ distinctUntilChanged,
map,
startWith,
} from 'rxjs/operators';
@@
- private counter = 0;
+ // counter removed; distinctUntilChanged comparator handles swap bursts
@@
- distinct(({ site, isSwapSitesContentsAction }) =>
- isSwapSitesContentsAction ? this.counter++ : site,
- ),
+ distinctUntilChanged(
+ (prev, curr) =>
+ // Suppress only consecutive duplicates when NOT triggered by a swap
+ !curr.isSwapSitesContentsAction && prev.site === curr.site,
+ ),Also applies to: 49-51, 72-75
🤖 Prompt for AI Agents
In editor/src/app/shop/orders/shop-orders.state.ts around lines 1-8 (and also
apply the same change at lines 49-51 and 72-75): the code uses rxjs distinct
with a counter key which causes unbounded memory growth and prevents
re-initialization for previously seen sites; replace distinct(...) with
distinctUntilChanged(...) and remove the counter entirely, implementing a
comparator function that only suppresses emissions when the current and previous
values are identical and no swap flag changed (i.e., only block consecutive
duplicates), ensuring site inits can re-run when revisiting a previously seen
site while eliminating the counter and the global key set.
| combineLatest([ | ||
| this.store$.select(AppState.getSite), | ||
| this.actions$.pipe( | ||
| ofActionSuccessful(SwapContentsSitesAction), | ||
| startWith(null), // Emit initial value so combineLatest starts immediately | ||
| ), | ||
| ]) | ||
| .pipe( |
There was a problem hiding this comment.
Fix combineLatest filter: destructure the tuple; current predicate never filters null site.
combineLatest emits [site, action]; filter((site) => ...) checks the array, not the site. Destructure to avoid running with null/undefined site.
Apply:
- .pipe(
- filter((site) => site !== null),
- map(([site, action]) => ({
+ .pipe(
+ filter(([site]) => site != null),
+ map(([site, action]) => ({
site,
isSwapSitesContentsAction: action !== null,
})),Also applies to: 68-71
🤖 Prompt for AI Agents
In editor/src/app/shop/orders/shop-orders.state.ts around lines 59 to 66 (and
similarly lines 68 to 71), the combineLatest subscription passes a tuple [site,
action] into downstream operators but the filter predicate treats its parameter
as if it were the site; change the filter to destructure the tuple (e.g. ([site,
action]) => ...) and then check site for null/undefined so the stream skips
emissions when site is falsy; apply the same destructuring/falsy check to the
other occurrence at lines 68-71.
| .pipe( | ||
| filter(([site]) => site !== null), | ||
| map(([site, action]) => ({ | ||
| site, | ||
| forceRefresh: action !== null, | ||
| })), |
There was a problem hiding this comment.
Guard against undefined/empty site before fetching.
Current check only excludes null; undefined or '' will pass and trigger invalid requests.
Apply:
- filter(([site]) => site !== null),
+ // Ensure site is a non-empty string
+ filter(([site]) => typeof site === 'string' && site.length > 0),📝 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.
| .pipe( | |
| filter(([site]) => site !== null), | |
| map(([site, action]) => ({ | |
| site, | |
| forceRefresh: action !== null, | |
| })), | |
| .pipe( | |
| // Ensure site is a non-empty string | |
| filter(([site]) => typeof site === 'string' && site.length > 0), | |
| map(([site, action]) => ({ | |
| site, | |
| forceRefresh: action !== null, | |
| })), |
🤖 Prompt for AI Agents
In editor/src/app/shop/products/shop-products.state.ts around lines 98 to 103,
the current filter only excludes null sites so undefined or empty string values
can pass and cause invalid fetches; update the filter to explicitly guard
against undefined and empty string (e.g., check site !== null && site !==
undefined && site !== '') or use a truthy check appropriate for your site type,
and keep the subsequent map unchanged so forceRefresh is computed only when site
is valid.
| distinct(({ site, isSwapSitesContentsAction }) => | ||
| isSwapSitesContentsAction ? this.counter++ : site, | ||
| ), |
There was a problem hiding this comment.
Bug: distinct() will block future re-fetches for previously visited sites. Use distinctUntilChanged().
distinct de-duplicates keys forever; revisiting an already seen site won’t re-fetch unless a swap happened that changes the key. Replace with distinctUntilChanged and remove the counter.
Apply:
-import {
- take,
- tap,
- catchError,
- filter,
- switchMap,
- distinct,
- map,
- startWith,
-} from 'rxjs/operators';
+import {
+ take,
+ tap,
+ catchError,
+ filter,
+ switchMap,
+ distinctUntilChanged,
+ map,
+ startWith,
+} from 'rxjs/operators';
@@
- private counter = 0;
+ // counter no longer needed with distinctUntilChanged
@@
- distinct(({ site, isSwapSitesContentsAction }) =>
- isSwapSitesContentsAction ? this.counter++ : site,
- ),
+ distinctUntilChanged(
+ (prev, curr) =>
+ // Suppress only consecutive repeats when NOT triggered by swap
+ !curr.isSwapSitesContentsAction && prev.site === curr.site,
+ ),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In editor/src/app/shop/regional-costs/shop-regional-costs.state.ts around lines
103 to 105, the use of distinct(...) causes keys to be de-duplicated permanently
and prevents re-fetch on revisits; replace the distinct(...) call with
distinctUntilChanged(...) and remove the counter increment logic and the counter
field entirely. Ensure you import distinctUntilChanged from rxjs/operators
(remove any unused counter variable and its references) so the stream only
suppresses consecutive duplicates rather than remembering all seen keys.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
editor/src/app/shop/products/shop-products.state.ts (2)
98-102: Harden site guard to avoid invalid fetches for undefined/empty values.Apply:
- filter(([site]) => site !== null), + // Ensure site is a non-empty string + filter(([site]) => typeof site === 'string' && site.length > 0),Based on past review comments.
85-93: Include tag updates in the filter; consider dropping cartPrice. Keeps product grouping in sync.Apply:
- // Apply filter for UpdateSectionEntryFromSyncAction - const actions = ['cartTitle', 'cartPrice', 'cartAttributes']; - const prop = action.path.split('/').pop(); - return actions.indexOf(prop) > -1; + // Apply filter for UpdateSectionEntryFromSyncAction + const props = new Set(['cartTitle', 'cartAttributes', 'tag']); + const prop = action.path.split('/').pop(); + return props.has(prop as string);Based on past review comments.
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (1)
102-104: Use distinctUntilChanged and drop counter. Prevents suppressed re-fetches and key buildup.Apply:
@@ -import { - take, - tap, - catchError, - filter, - switchMap, - distinct, - map, -} from 'rxjs/operators'; +import { + take, + tap, + catchError, + filter, + switchMap, + distinctUntilChanged, + map, +} from 'rxjs/operators'; @@ - private destroyRef = inject(DestroyRef); - private counter = 0; + private destroyRef = inject(DestroyRef); @@ - distinct(({ site, isSwapSitesContentsAction }) => - isSwapSitesContentsAction ? this.counter++ : site, - ), + distinctUntilChanged( + (prev, curr) => + !curr.isSwapSitesContentsAction && prev.site === curr.site, + ),Based on past review comments.
Also applies to: 77-79, 1-9
editor/src/app/shop/settings/shop-settings.state.ts (1)
9-10: Replace distinct + counter with distinctUntilChanged; remove counter.Eliminates suppressed re-inits and counter side effects.
Apply:
@@ -import { - take, - tap, - catchError, - filter, - switchMap, - map, - distinct, -} from 'rxjs/operators'; +import { + take, + tap, + catchError, + filter, + switchMap, + map, + distinctUntilChanged, +} from 'rxjs/operators'; @@ - private destroyRef = inject(DestroyRef); - private counter = 0; + private destroyRef = inject(DestroyRef); @@ - distinct(({ site, isSwapSitesContentsAction }) => - isSwapSitesContentsAction ? this.counter++ : site, - ), + distinctUntilChanged( + (prev, curr) => + !curr.isSwapSitesContentsAction && prev.site === curr.site, + ),Based on past review comments.
Also applies to: 115-117, 89-91
🧹 Nitpick comments (10)
editor/src/app/shop/shop.component.ts (1)
3-4: Import reorganization looks good.The move of
startWithfromrxjs/operatorstorxjsis functionally correct and aligns with modern RxJS patterns.Optionally, for consistency, consider importing all pipeable operators (
map,mergeMap) fromrxjsas well in a future refactor.editor/src/app/shop/products/shop-products.state.ts (1)
106-114: Keep stream alive on init errors. Add catchError(() => EMPTY).Apply:
-import { combineLatest, startWith } from 'rxjs'; +import { combineLatest, startWith, EMPTY } from 'rxjs'; @@ switchMap(({ site, forceRefresh }) => this.stateService .getInitialState(site, 'products', forceRefresh) .pipe( take(1), - map((products) => ({ site, products })), + map((products) => ({ site, products })), + catchError(() => EMPTY), ), ),Also applies to: 33-37
editor/src/app/shop/orders/shop-orders.state.ts (2)
61-65: Strengthen site guard to block undefined/empty strings.Apply:
- filter(([site]) => site !== null), + // Ensure site is a non-empty string + filter(([site]) => typeof site === 'string' && site.length > 0),
69-76: Add catchError(() => EMPTY) to keep init stream resilient to HTTP errors.Apply:
switchMap(({ site, isSwapSitesContentsAction }) => this.stateService .getInitialState(site, 'orders', isSwapSitesContentsAction) .pipe( take(1), map((orders) => ({ site, orders })), + catchError(() => EMPTY), ), ),editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (3)
96-101: Tighten site guard; keep feature check.Apply:
- filter(([site, hasFeatureShop]) => site !== null && hasFeatureShop), + filter( + ([site, hasFeatureShop]) => + typeof site === 'string' && site.length > 0 && hasFeatureShop, + ),
105-114: Add catchError(() => EMPTY) to prevent stream termination on init failure.Apply:
-import { combineLatest, startWith } from 'rxjs'; +import { combineLatest, startWith, EMPTY } from 'rxjs'; @@ switchMap(({ site, isSwapSitesContentsAction }) => this.stateService .getInitialState(site, 'regionalCosts', isSwapSitesContentsAction) .pipe( take(1), map((regionalCosts) => ({ site, regionalCosts })), + catchError(() => EMPTY), ), ),Also applies to: 40-43
431-434: Rename handler for clarity.Method name says resetProducts, but this state manages regional costs. Rename to resetRegionalCosts (decorator binding remains unchanged).
Apply:
- @Action(ResetShopRegionalCostsAction) - resetProducts({ setState }: StateContext<ShopRegionalCostsModel>) { + @Action(ResetShopRegionalCostsAction) + resetRegionalCosts({ setState }: StateContext<ShopRegionalCostsModel>) { setState(defaultState); }editor/src/app/shop/settings/shop-settings.state.ts (3)
110-114: Harden site guard; preserve feature check.Apply:
- filter(([site, hasFeatureShop]) => site !== null && hasFeatureShop), + filter( + ([site, hasFeatureShop]) => + typeof site === 'string' && site.length > 0 && hasFeatureShop, + ),
118-126: Add catchError(() => EMPTY) to keep init stream resilient.Apply:
-import { combineLatest, startWith } from 'rxjs'; +import { combineLatest, startWith, EMPTY } from 'rxjs'; @@ switchMap(({ site, isSwapSitesContentsAction }) => this.stateService .getInitialState(site, 'settings', isSwapSitesContentsAction) .pipe( take(1), map((settings) => ({ site, settings })), + catchError(() => EMPTY), ), ),Also applies to: 36-40
242-245: Rename handler for clarity.resetProducts -> resetSettings.
Apply:
- @Action(ResetShopSettingsAction) - resetProducts({ setState }: StateContext<ShopSettingsModel>) { + @Action(ResetShopSettingsAction) + resetSettings({ setState }: StateContext<ShopSettingsModel>) { setState(defaultState); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
editor/src/app/shop/orders/shop-orders.state.ts(4 hunks)editor/src/app/shop/products/shop-products.state.ts(3 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts(3 hunks)editor/src/app/shop/settings/shop-settings.state.ts(4 hunks)editor/src/app/shop/shop.component.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/user/user.state.ts (1)
hasFeatureShop(54-56)
editor/src/app/shop/orders/shop-orders.state.ts (1)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
editor/src/app/shop/products/shop-products.state.ts (2)
editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
UpdateSectionEntryFromSyncAction(87-94)editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)
editor/src/app/shop/settings/shop-settings.state.ts (2)
editor/src/app/sites/sites-state/sites.actions.ts (1)
SwapContentsSitesAction(62-65)editor/src/app/user/user.state.ts (1)
hasFeatureShop(54-56)
| distinct(({ site, isSwapSitesContentsAction }) => | ||
| isSwapSitesContentsAction ? this.counter++ : site, | ||
| ), |
There was a problem hiding this comment.
Switch to distinctUntilChanged; remove counter. Avoids suppressed re-inits and memory growth.
Use comparator to suppress only consecutive duplicates when not triggered by swap.
Apply:
@@
-import { startWith } from 'rxjs';
+import { startWith, EMPTY } from 'rxjs';
@@
-import { take, filter, switchMap, distinct, map } from 'rxjs/operators';
+import { take, filter, switchMap, distinctUntilChanged, map, catchError } from 'rxjs/operators';
@@
- private destroyRef = inject(DestroyRef);
- private counter = 0;
+ private destroyRef = inject(DestroyRef);
@@
- distinct(({ site, isSwapSitesContentsAction }) =>
- isSwapSitesContentsAction ? this.counter++ : site,
- ),
+ distinctUntilChanged(
+ (prev, curr) =>
+ !curr.isSwapSitesContentsAction && prev.site === curr.site,
+ ),Also applies to: 43-50, 1-3
🤖 Prompt for AI Agents
In editor/src/app/shop/orders/shop-orders.state.ts around lines 66-68 (and also
update the similar usages at lines 43-50 and 1-3): replace the distinct
projection that used this.counter with distinctUntilChanged using a comparator
that only treats consecutive emissions as equal when the current emission is NOT
a swap action; remove the counter field entirely. Concretely, change to
distinctUntilChanged((prev, curr) => { if (curr.isSwapSitesContentsAction)
return false; return prev.site === curr.site; }) so swap actions always pass
through and only true consecutive duplicate sites are suppressed.
| distinct(({ site, forceRefresh }) => | ||
| forceRefresh ? this.counter++ : site, | ||
| ), |
There was a problem hiding this comment.
Replace distinct + counter with distinctUntilChanged; remove counter. Prevents stale data and unbounded key growth.
Current distinct stores all keys and suppresses re-init for previously visited sites; the side-effect counter grows without bound. Use distinctUntilChanged to only suppress consecutive duplicates when not forced by an action; drop the counter.
Apply:
@@
-import {
- take,
- tap,
- catchError,
- filter,
- switchMap,
- distinct,
- map,
-} from 'rxjs/operators';
+import {
+ take,
+ tap,
+ catchError,
+ filter,
+ switchMap,
+ distinctUntilChanged,
+ map,
+} from 'rxjs/operators';
@@
- private destroyRef = inject(DestroyRef);
- private counter = 0;
+ private destroyRef = inject(DestroyRef);
@@
- distinct(({ site, forceRefresh }) =>
- forceRefresh ? this.counter++ : site,
- ),
+ distinctUntilChanged(
+ (prev, curr) => !curr.forceRefresh && prev.site === curr.site,
+ ),Also applies to: 66-68, 13-20
🤖 Prompt for AI Agents
In editor/src/app/shop/products/shop-products.state.ts around lines 103-105 (and
similarly at 66-68 and 13-20), replace the distinct(...) usage that relies on a
side-effecting counter with distinctUntilChanged so duplicates are only
suppressed when consecutive and not forced; remove the counter field entirely.
Modify the observable pipeline to map to the site value (or an object including
forceRefresh) and use distinctUntilChanged with a comparator that treats
forceRefresh as not-equal when true (i.e., bypass suppression on a force),
ensuring no global key accumulation and no unbounded counter growth.
1 participant
Summary by CodeRabbit
New Features
UI
State
Style