Conversation
WalkthroughUpgrades Angular and build tooling to v10, updates TypeScript, core-js and polyfills, narrows a DOM ElementRef type, refactors NGXS handlers to use observable chaining, adapts gallery reorder handlers to ng-sortgrid's NgsgOrderChange, fixes an entries-state height assignment bug, and adjusts tsconfig/tslint/angular.json formatting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Gallery Component
participant NGSG as ng-sortgrid
participant Store as NGXS Store
User->>UI: drag / reorder
UI->>NGSG: grid emits NgsgOrderChange
NGSG-->>UI: NgsgOrderChange { currentOrder: [...] }
UI->>UI: if currentOrder.length < 2 -> return
UI->>Store: dispatch(OrderAction(srcs from currentOrder))
Store->>Store: handle action / persist order
Store-->>UI: state update
UI-->>User: UI reflects new order
sequenceDiagram
autonumber
participant Action as AddShopProductSiteAction
participant State as ShopProductsState
participant Svc as Service
Action->>State: handle(action, ctx)
State->>Svc: getInitialState(...)
Svc-->>State: Observable<products>
State->>State: pipe(take(1), tap(patchState(...)))
State-->>Action: returns observable (no internal subscribe)
sequenceDiagram
autonumber
participant Auth as Logout flow
participant Shop as ShopState
participant Store as NGXS Store
Auth->>Shop: logout$
Shop->>Shop: map(...) -> [oldSite, newSite]
Shop->>Shop: filter(([old,new]) => ...)
Shop->>Store: dispatch([new ResetAppStateAction(), new ResetSitesAction(), ...])
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
editor/src/app/shop/products/shop-products.state.ts (1)
190-195: Add consistent error handling and tighten typesOther handlers (e.g., UpdateShopProductAction) log/rethrow errors. Mirror that here and annotate the tap param for type safety; optionally guard against a missing site key.
- return this.stateService.getInitialState(action.payload, 'products').pipe( - take(1), - tap((products) => - patchState({ [action.payload]: products[action.payload] }) - ) - ); + return this.stateService.getInitialState(action.payload, 'products').pipe( + take(1), + tap((products: ShopProductsModel) => { + const siteProducts = products[action.payload]; + if (siteProducts) { + patchState({ [action.payload]: siteProducts }); + } + }), + catchError((error: HttpErrorResponse | Error) => { + if (error instanceof HttpErrorResponse) { + console.error(error.error?.message ?? error.message); + } else { + console.error(error.message); + } + throw error; + }) + );editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (1)
861-872: Avoid repeated parseInt in a hot map; precompute onceThe explicit radix is great. Minor tidy-up: compute once before the map to avoid repeated parsing and clarify intent.
- const currentState = getState(); + const currentState = getState(); + const targetIndex = Number.parseInt(String(action.fileOrder), 10); patchState({ [action.site]: currentState[action.site].map((entry) => { @@ - file: entry.mediaCacheData.file.map((file, index) => { - if (index === parseInt(action.fileOrder, 10)) { + file: entry.mediaCacheData.file.map((file, index) => { + if (index === targetIndex) { return {editor/src/app/inputs/select-input.component.ts (1)
75-75: Stronger typing on ViewChild — nice.Two tiny follow-ups if you run with strict flags:
- Consider a definite‑assignment assertion to avoid potential strictPropertyInitialization noise: dropDownAnchor!: ElementRef.
- Optionally type handlers (e.g., onKeyDown(event: KeyboardEvent)) for better IntelliSense.
editor/src/app/shop/shop.state.ts (1)
89-89: Tuple destructuring in filter is clearer — approved.Optional: add a lightweight tuple type alias to document intent and improve inference in map/filter/subscribe.
editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
562-564: Reorder handler updated to NgsgOrderChange — approved; add no‑op guard if unchanged.Early return for length<2 is good. Optionally skip dispatch when order hasn’t actually changed (e.g., if the event can fire without reordering) to avoid redundant writes.
Also applies to: 571-571
editor/src/app/sites/sections/background-gallery-editor.component.ts (1)
23-23: Use a type‑only import for the event type.Prevents it from ending up in the runtime bundle.
-import { NgsgOrderChange } from 'ng-sortgrid'; +import type { NgsgOrderChange } from 'ng-sortgrid';editor/package.json (1)
15-23: Align polyfills with Angular 10Update
editor/package.jsonto use Core JS 3 and the matching Zone JS patch:"dependencies": { @@ - "core-js": "^2.5.4", + "core-js": "^3.6.5", @@ - "zone.js": "~0.10.2" + "zone.js": "~0.10.3" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
editor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
editor/package.json(2 hunks)editor/src/app/inputs/select-input.component.ts(1 hunks)editor/src/app/shop/products/shop-products.state.ts(1 hunks)editor/src/app/shop/shop.state.ts(2 hunks)editor/src/app/sites/media/entry-gallery-editor.component.ts(2 hunks)editor/src/app/sites/sections/background-gallery-editor.component.ts(2 hunks)editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts(2 hunks)editor/src/app/user/user.state.ts(1 hunks)editor/tsconfig.json(1 hunks)editor/tslint.json(0 hunks)
💤 Files with no reviewable changes (1)
- editor/tslint.json
🧰 Additional context used
🧬 Code graph analysis (4)
editor/src/app/sites/media/entry-gallery-editor.component.ts (2)
editor/src/app/sites/sections/entries/entries-state/section-entries-state.model.ts (1)
SectionEntryGalleryFile(22-32)editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
OrderEntryGalleryFilesAction(169-177)
editor/src/app/user/user.state.ts (9)
editor/src/app/app-state/app.actions.ts (1)
ResetAppStateAction(24-26)editor/src/app/sites/sites-state/sites.actions.ts (1)
ResetSitesAction(37-39)editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
ResetSiteSectionsAction(109-111)editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
ResetSiteSectionsTagsAction(54-56)editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
ResetSectionEntriesAction(64-66)editor/src/app/sites/settings/site-settings.actions.ts (1)
ResetSiteSettingsAction(44-46)editor/src/app/sites/settings/site-settings-config.actions.ts (1)
ResetSiteSettingsConfigAction(3-5)editor/src/app/sites/template-settings/site-template-settings.actions.ts (1)
ResetSiteTemplateSettingsAction(41-43)editor/src/app/sites/template-settings/site-templates.actions.ts (1)
ResetSiteTemplatesAction(3-5)
editor/src/app/sites/sections/background-gallery-editor.component.ts (2)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
OrderSiteSectionBackgroundAction(47-54)
editor/src/app/shop/shop.state.ts (5)
editor/src/app/shop/shop.actions.ts (1)
ResetShopAction(3-5)editor/src/app/shop/products/shop-products.actions.ts (1)
ResetShopProductsAction(34-36)editor/src/app/shop/orders/shop-orders.actions.ts (1)
ResetShopOrdersAction(23-25)editor/src/app/shop/regional-costs/shop-regional-costs.actions.ts (1)
ResetShopRegionalCostsAction(88-90)editor/src/app/shop/settings/shop-settings.actions.ts (2)
ResetShopSettingsAction(40-42)ResetShopSettingsConfigAction(44-46)
🔇 Additional comments (7)
editor/src/app/shop/products/shop-products.state.ts (1)
190-195: Good RxJS refactor: return observable + tap side‑effectAvoiding internal subscribe is cleaner; NGXS will manage the subscription and action completion. Looks good.
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (1)
713-714: Fix is correct: assign height to heightGood catch; this resolves the wrong-assignment bug for gallery files.
editor/tsconfig.json (1)
12-12: Module set to es2020 — looks good; verify build targets.This aligns with Angular 10 defaults. Ensure your builder/browserslist still yields the intended differential builds with target es2015 + module es2020 (especially for legacy browsers if any).
editor/src/app/shop/shop.state.ts (1)
153-159: Batch dispatching instantiated actions on logout — approved.Good NGXS practice to pass instances and batch. If any downstream logic relied on per‑dispatch completion ordering, confirm behavior remains as expected when dispatched as an array.
editor/src/app/user/user.state.ts (1)
116-126: Consolidated reset actions into a single batch — approved.Order is preserved; NGXS will process them sequentially. Sanity‑check that any listeners awaiting actionsCompleted still behave the same (now the returned observable completes after the batch).
editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
4-4: Importing NgsgOrderChange matches the updated (sorted) event payload — approved.editor/package.json (1)
54-66: Update lint/test toolchain for Angular 10 + TS 4
- bump codelyzer to ^6.0.0 (stackoverflow.com)
- bump ts-node to ^9.1.1 to support TS 4.0.x
- bump @types/jasmine to a 3.x release matching jasmine-core 3.x and verify typing compatibility
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/src/app/sites/sections/background-gallery-editor.component.ts (1)
475-479: Type mismatch in trackBy signature (array vs element).
itemis a single file, but typed as the array type. Use the element type to restore type safety.- identifyGalleryItem( - _, - item: SiteSectionStateModel['mediaCacheData']['file'] - ) { + identifyGalleryItem(_, item: SiteSectionBackgroundFile) { return item['@attributes'].src; }
🧹 Nitpick comments (10)
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (2)
709-714: Fix height assignment — LGTMCorrectly sets newFile['@attributes'].height from response.height. Consider null-safe checks so 0 is not skipped.
- if (response.width) { + if (response.width != null) { newFile['@attributes'].width = response.width; } - if (response.height) { + if (response.height != null) { newFile['@attributes'].height = response.height; }
860-893: Guard against NaN/out‑of‑range fileOrder to avoid silent no‑opsIf parseInt yields NaN or the index is out of bounds, the update is skipped silently. Add a lightweight guard before patchState.
- } else { - const currentState = getState(); - const targetIndex = Number.parseInt(String(action.fileOrder), 10); + } else { + const currentState = getState(); + const targetIndex = Number.parseInt(String(action.fileOrder), 10); + const entryToUpdate = currentState[action.site]?.find( + (e) => e.sectionName === action.section && e.id === action.entryId + ); + const files = entryToUpdate?.mediaCacheData?.file ?? []; + if (!Number.isFinite(targetIndex) || targetIndex < 0 || targetIndex >= files.length) { + console.warn('[updateEntryGalleryImageCrop] fileOrder invalid/out-of-range', { + fileOrder: action.fileOrder, targetIndex, length: files.length + }); + return; + } patchState({ [action.site]: currentState[action.site].map((entry) => { if ( entry.sectionName === action.section && entry.id === action.entryId ) { const mediaCacheData = { ...entry.mediaCacheData, - file: entry.mediaCacheData.file.map((file, index) => { - if (index === targetIndex) { + file: entry.mediaCacheData.file.map((file, index) => { + if (index === targetIndex) { return { ...file, '@attributes': { ...file['@attributes'], src: response.update, width: response.params.width, height: response.params.height, }, }; } return file; }), };editor/src/polyfills.ts (1)
45-45: Confirm Reflect polyfill choice (JIT vs AOT/Ivy).With Angular 10 + Ivy, Reflect polyfills are generally unnecessary unless you use JIT with emitDecoratorMetadata. If you do need it, prefer
reflect-metadata; otherwise drop the import to trim bundle.Suggested options:
- If JIT with decorator metadata:
import 'reflect-metadata';- Otherwise: remove this line.
editor/angular.json (1)
50-53: CommonJS allowances: consider reducing CJS to improve tree‑shaking.You’ve allowed
lodashandwebfontloader. Consider:
- Replacing
lodashimports withlodash-es(ESM) or importing only needed functions (import pick from 'lodash-es/pick').- Lazy‑loading
webfontloader(dynamic import) to constrain its impact.editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
562-572: Skip no‑op dispatches by comparing orders.
sortedmay fire even when order is unchanged. Compare previous/current before dispatching to avoid redundant state writes.- reorder(orderChangeEvent: NgsgOrderChange<SectionEntryGalleryFile>) { - if (orderChangeEvent.currentOrder.length < 2) { + reorder(orderChangeEvent: NgsgOrderChange<SectionEntryGalleryFile>) { + if (orderChangeEvent.currentOrder.length < 2) { return; - } + } + const prev = orderChangeEvent.previousOrder.map(f => f['@attributes'].src); + const curr = orderChangeEvent.currentOrder.map(f => f['@attributes'].src); + if (prev.length === curr.length && prev.every((v, i) => v === curr[i])) { + return; + } this.store.dispatch( new OrderEntryGalleryFilesAction( this.currentSite.name, this.currentEntry.sectionName, this.currentEntry.id, - orderChangeEvent.currentOrder.map((f) => f['@attributes'].src) + curr ) ); }editor/src/app/sites/sections/background-gallery-editor.component.ts (1)
412-421: Optional: avoid no‑op dispatches on unchanged order.Same micro‑optimization as the entry editor—compare
previousOrdervscurrentOrderbefore dispatching.editor/package.json (4)
13-15: Engines target EOL Node versions—confirm CI/runtime and plan upgrade.
node: ^10.13 || ^12.11are EOL. If feasible, plan to raise engines to a supported LTS (this may require a later Angular). For now, ensure CI uses a pinned Node matching Angular 10 constraints.
50-51: Align Jasmine types with jasmine-core 3.x.You’re on
jasmine-core ~3.5.0but@types/jasmineis~2.8.6. Bump types to 3.x to avoid TS type drift.- "@types/jasmine": "~2.8.6", + "@types/jasmine": "~3.5.0",Run tests to confirm no breaking type changes.
Also applies to: 57-63
30-30: Upgrade core-js to ≥3.23.3 (preferably ^3.45.1)Versions below 3.23.3 are no longer maintained and carry known correctness and performance issues. Bump the dependency and then verify your build and remove any obsolete polyfills:
- "core-js": "^3.6.5", + "core-js": "^3.45.1",
32-32: Optional: track compatibility and plan upgrade of ng-sortgrid to ^18.0.0
Latest ng-sortgrid is 18.0.0 and targets Angular 10-era APIs; consider scheduling this bump alongside your Angular upgrade.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
editor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
editor/angular.json(3 hunks)editor/package.json(2 hunks)editor/src/app/inputs/select-input.component.ts(2 hunks)editor/src/app/shop/products/shop-products.state.ts(1 hunks)editor/src/app/sites/media/entry-gallery-editor.component.ts(2 hunks)editor/src/app/sites/sections/background-gallery-editor.component.ts(2 hunks)editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts(3 hunks)editor/src/polyfills.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- editor/src/app/shop/products/shop-products.state.ts
- editor/src/app/inputs/select-input.component.ts
🧰 Additional context used
🧬 Code graph analysis (2)
editor/src/app/sites/media/entry-gallery-editor.component.ts (2)
editor/src/app/sites/sections/entries/entries-state/section-entries-state.model.ts (1)
SectionEntryGalleryFile(22-32)editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
OrderEntryGalleryFilesAction(169-177)
editor/src/app/sites/sections/background-gallery-editor.component.ts (2)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionBackgroundFile(39-47)editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
OrderSiteSectionBackgroundAction(47-54)
🔇 Additional comments (4)
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (1)
860-864: Confirm zero-based indexing for fileOrder
I found no +1/−1 adjustments in the handler (it directly parsesaction.fileOrderas a base-10 integer). Ensure that inentry-gallery-image-editor.component.tsthefileOrderargument you pass intoUpdateEntryGalleryImageCropActionis a 0-based index to align with theArray.mapindex.editor/angular.json (1)
50-52: Verify styles path consistency (build vs test).Build uses
src/styles/styles.scsswhile tests usesrc/styles.scss. Ensure both paths exist or align them to avoid test/build drift.Also applies to: 99-101
editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
4-4: Type‑only import is good.
import type { NgsgOrderChange } ...keeps runtime clean with TS ≥3.8.editor/src/app/sites/sections/background-gallery-editor.component.ts (1)
412-421: Good fix: correct NgsgOrderChange typing and payload mapping.Using
NgsgOrderChange<SiteSectionBackgroundFile>and mapping tosrcis correct and consistent with the entry gallery editor.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
editor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
editor/package.json(1 hunks)editor/src/app/sites/sections/background-gallery-editor.component.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/sites/sections/background-gallery-editor.component.ts (2)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionBackgroundFile(39-47)editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
OrderSiteSectionBackgroundAction(47-54)
🔇 Additional comments (2)
editor/src/app/sites/sections/background-gallery-editor.component.ts (2)
23-421: Reorder handler update looks solid.Importing
NgsgOrderChangeand switching the handler to usecurrentOrderkeeps the payload typed and aligned with ng-sortgrid’s event model. No issues from my side.
475-477: TrackBy typing tweak is correct.The narrowed item type improves trackBy safety without altering behavior.
| "engines": { | ||
| "node": "^10.13.0 || ^12.11.1" | ||
| }, |
There was a problem hiding this comment.
Relax the Node engines constraint.
Pinning "node": "^10.13.0 || ^12.11.1" blocks installs on today’s supported LTS releases (Node 18/20). Angular 10 tooling runs on newer Node as well, so this engines entry will break developer environments that have already moved past the EOL Node 10/12 lines. Please expand the range (or drop the restriction) so current LTS versions remain usable.
🤖 Prompt for AI Agents
In editor/package.json around lines 13 to 15, the Node engines entry is pinned
to obsolete versions; change it to allow current LTS releases by replacing the
value with a broader range such as "node": ">=14" (or explicitly "node": ">=14
<=20" or "node": "^14 || ^16 || ^18 || ^20"), or remove the engines field
entirely if you don’t want to enforce Node versions; update the package.json
accordingly and run a quick install/test to ensure tooling still works.
There was a problem hiding this comment.
@coderabbitai not yet ready for newer node versions
There was a problem hiding this comment.
@uldisrudzitis Understood! I'll leave the Node version constraint as-is for now. Feel free to expand it when the project is ready for newer Node versions.
1 participant
Summary by CodeRabbit