Conversation
WalkthroughAngular project updated toward v9: schematics in angular.json modernized; dependencies in package.json upgraded; multiple NGXS state classes annotated with @Injectable; minor component/init tweak in preview and a ViewChild option removed; obsolete service, ThemeItem class, and Karma bootstrap removed; TypeScript config libs updated. Changes
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
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
editor/src/app/user/user.state.ts (2)
116-124: Likely NGXS dispatch misuse after upgrades — dispatch action instances, not classes.You’re passing action classes to
dispatch(...)instead of instances. NGXS expects instances (so it can read the statictypeoff the constructor); 3.7 tightened action typing and this can break after the Angular/NGXS upgrade. Suggest batching into a single dispatch with an array of instances.Apply:
- /* Reset the state of app */ - dispatch(ResetAppStateAction); - dispatch(ResetSitesAction); - dispatch(ResetSiteSectionsAction); - dispatch(ResetSiteSectionsTagsAction); - dispatch(ResetSectionEntriesAction); - dispatch(ResetSiteSettingsAction); - dispatch(ResetSiteSettingsConfigAction); - dispatch(ResetSiteTemplateSettingsAction); - dispatch(ResetSiteTemplatesAction); + /* Reset the state of app */ + dispatch([ + new ResetAppStateAction(), + new ResetSitesAction(), + new ResetSiteSectionsAction(), + new ResetSiteSectionsTagsAction(), + new ResetSectionEntriesAction(), + new ResetSiteSettingsAction(), + new ResetSiteSettingsConfigAction(), + new ResetSiteTemplateSettingsAction(), + new ResetSiteTemplatesAction(), + ]);
77-85: Harden LocalStorage parsing (JSON.parse(null) throws).On a fresh browser or cleared storage,
JSON.parse(features | profileUrl | intercom | helpcrunch)can throw. Add a tiny safe‑parse helper with sensible fallbacks.patchState({ - name: name, - token: token, - features: JSON.parse(features), - profileUrl: JSON.parse(profileUrl), - intercom: JSON.parse(intercom), - helpcrunch: JSON.parse(helpcrunch), + name, + token, + features: (() => { try { return features ? JSON.parse(features) : []; } catch { return []; } })(), + profileUrl: (() => { try { return profileUrl ? JSON.parse(profileUrl) : null; } catch { return null; } })(), + intercom: (() => { try { return intercom ? JSON.parse(intercom) : null; } catch { return null; } })(), + helpcrunch: (() => { try { return helpcrunch ? JSON.parse(helpcrunch) : null; } catch { return null; } })(), });editor/src/app/shop/shop.state.ts (2)
85-91: Bug: RxJS filter predicate receives the tuple, not two args.
filter((oldSiteName, newSiteName) => ...)treats the 2nd param as the emission index, notnewSiteName. Destructure the tuple.- filter((oldSiteName, newSiteName) => !!oldSiteName || !!newSiteName) + filter(([oldSiteName, newSiteName]) => !!oldSiteName || !!newSiteName)
151-160: Critical: instantiate dispatched actions as objects
In editor/src/app/shop/shop.state.ts at lines 151–160, prefix each action withnew:- dispatch([ - ResetShopAction, - ResetShopProductsAction, - ResetShopOrdersAction, - ResetShopRegionalCostsAction, - ResetShopSettingsAction, - ResetShopSettingsConfigAction, - ]); + dispatch([ + new ResetShopAction(), + new ResetShopProductsAction(), + new ResetShopOrdersAction(), + new ResetShopRegionalCostsAction(), + new ResetShopSettingsAction(), + new ResetShopSettingsConfigAction(), + ]);editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (1)
709-714: Bug: Height assigned towidthby mistake.
newFile['@attributes'].width = response.height;should setheight.- if (response.height) { - newFile['@attributes'].width = response.height; - } + if (response.height) { + newFile['@attributes'].height = response.height; + }
🧹 Nitpick comments (6)
editor/src/app/sites/template-settings/site-templates.state.ts (1)
88-105: Optional: guard long‑lived subscriptions (tests/HMR) and centralize effects.These subscriptions live for the app lifetime, which is fine; however, tests or HMR can create multiple instances. Consider:
- Add a small teardown (e.g.,
takeUntil(destroy$)withOnDestroy) or convert to NGXS actions (@Action(UserLoginAction)+ service call) to avoid manualsubscribe.Also applies to: 106-121
editor/src/app/shop/products/shop-products.state.ts (2)
190-196: Return an Observable from action handler instead of a Subscription.In
addProductsSite, prefer returning the observable so callers (and NGXS) can compose/await it; avoidsubscribehere.- return this.stateService - .getInitialState(action.payload, 'products') - .pipe(take(1)) - .subscribe((products) => { - patchState({ [action.payload]: products[action.payload] }); - }); + return this.stateService + .getInitialState(action.payload, 'products') + .pipe( + take(1), + tap((products) => + patchState({ [action.payload]: products[action.payload] }) + ) + );
57-63: Optional: selector could provide a safe default.Returning
state[site] || []avoids undefined at call sites when a site has no products yet.- ): Array<ShopProduct> { - return state[site]; - } + ): Array<ShopProduct> { + return state[site] || []; + }editor/src/app/inputs/select-input.component.ts (1)
75-75: Safe Ivy-ready ViewChild; add definite assignment and null-safe blur.Omitting
{ static }is correct in Angular 9. To satisfy TS 3.8 strict init and avoid edge-case NPEs, also:
- Add definite assignment and a concrete element type.
- Use optional chaining when blurring.
- @ViewChild('dropDownAnchor') dropDownAnchor: ElementRef; + @ViewChild('dropDownAnchor') dropDownAnchor!: ElementRef<HTMLButtonElement>;- this.dropDownAnchor.nativeElement.blur(); + this.dropDownAnchor?.nativeElement.blur();- this.dropDownAnchor.nativeElement.blur(); + this.dropDownAnchor?.nativeElement.blur();Also applies to: 97-101, 147-148
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (2)
869-873: Specify radix in parseInt for deterministic behavior.Use base 10 to avoid accidental octal/hex parsing.
- if (index === parseInt(action.fileOrder)) { + if (index === parseInt(action.fileOrder, 10)) {
109-113: Guard against undefined site entry arrays.When adding the first entry for a site,
currentState[action.site]can be undefined, causing.map()to throw.- [action.site]: [ - ...currentState[action.site].map((entry) => { + [action.site]: [ + ...((currentState[action.site] || [])).map((entry) => {
📜 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 (26)
editor/angular.json(1 hunks)editor/package.json(2 hunks)editor/src/app/app-state/app.state.ts(2 hunks)editor/src/app/error-state/error.state.ts(1 hunks)editor/src/app/inputs/select-input.component.ts(1 hunks)editor/src/app/preview/preview.component.ts(1 hunks)editor/src/app/shop/orders/shop-orders.state.ts(2 hunks)editor/src/app/shop/products/shop-products.state.ts(2 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts(2 hunks)editor/src/app/shop/settings/shop-settings-config.state.ts(2 hunks)editor/src/app/shop/settings/shop-settings.state.ts(2 hunks)editor/src/app/shop/shop.state.ts(2 hunks)editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts(1 hunks)editor/src/app/sites/sections/sections-state/site-sections.state.ts(1 hunks)editor/src/app/sites/sections/tags/section-tags.state.ts(1 hunks)editor/src/app/sites/settings/site-settings-config.state.ts(1 hunks)editor/src/app/sites/settings/site-settings.service.ts(0 hunks)editor/src/app/sites/settings/site-settings.state.ts(1 hunks)editor/src/app/sites/sites-state/sites.state.ts(1 hunks)editor/src/app/sites/template-settings/site-template-settings.state.ts(1 hunks)editor/src/app/sites/template-settings/site-templates.state.ts(1 hunks)editor/src/app/user/user.state.ts(2 hunks)editor/src/test.ts(0 hunks)editor/src/tsconfig.spec.json(0 hunks)editor/src/types/theme-item.ts(0 hunks)editor/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- editor/src/app/sites/settings/site-settings.service.ts
- editor/src/test.ts
- editor/src/tsconfig.spec.json
- editor/src/types/theme-item.ts
🧰 Additional context used
🧬 Code graph analysis (13)
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (3)
editor/src/app/app-state/app.state.ts (1)
State(51-239)editor/src/app/error-state/error.state.ts (1)
State(17-55)editor/src/app/shop/orders/shop-orders.state.ts (1)
State(29-113)
editor/src/app/sites/template-settings/site-template-settings.state.ts (2)
editor/src/app/app-state/app.state.ts (1)
State(51-239)editor/src/app/sites/template-settings/site-templates.state.ts (1)
State(29-168)
editor/src/app/user/user.state.ts (3)
editor/src/app/auth-guard.service.ts (1)
Injectable(15-53)editor/src/app/render/template-render.service.ts (1)
Injectable(36-383)editor/src/app/render/render.service.ts (1)
Injectable(10-59)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (3)
editor/src/app/app-state/app.state.ts (1)
State(51-239)editor/src/app/shop/orders/shop-orders.state.ts (1)
State(29-113)editor/src/app/sites/sites-state/sites.state.ts (1)
State(57-324)
editor/src/app/sites/sites-state/sites.state.ts (2)
editor/src/app/app-state/app.state.ts (1)
State(51-239)editor/src/app/error-state/error.state.ts (1)
State(17-55)
editor/src/app/sites/template-settings/site-templates.state.ts (2)
editor/src/app/sites/sites-state/sites.state.ts (1)
State(57-324)editor/src/app/sites/template-settings/site-template-settings.state.ts (1)
State(37-309)
editor/src/app/shop/shop.state.ts (1)
editor/src/app/shop/shop-state.service.ts (1)
Injectable(32-145)
editor/src/app/shop/settings/shop-settings-config.state.ts (1)
editor/src/app/preview/style.service.ts (1)
Injectable(18-264)
editor/src/app/sites/settings/site-settings.state.ts (2)
editor/src/app/app-state/app.state.ts (1)
State(51-239)editor/src/app/error-state/error.state.ts (1)
State(17-55)
editor/src/app/error-state/error.state.ts (1)
editor/src/app/app-state/app.state.ts (1)
State(51-239)
editor/src/app/sites/sections/tags/section-tags.state.ts (2)
editor/src/app/app-state/app.state.ts (1)
State(51-239)editor/src/app/error-state/error.state.ts (1)
State(17-55)
editor/src/app/shop/settings/shop-settings.state.ts (2)
editor/src/app/sites/shared/file-upload.service.ts (1)
Injectable(9-53)editor/src/app/preview/preview.service.ts (1)
Injectable(50-613)
editor/src/app/sites/settings/site-settings-config.state.ts (1)
editor/src/app/shop/settings/shop-settings-config.state.ts (1)
State(19-64)
🔇 Additional comments (16)
editor/src/app/preview/preview.component.ts (1)
78-79: Good defensive initialization.Setting a sanitized
about:blankupfront prevents the iframe from briefly navigating to an incorrect URL while the store selectors settle. Looks solid.editor/src/app/sites/template-settings/site-templates.state.ts (2)
33-33: Decorator placement is correct; keep it without providedIn.
@Injectable()(noprovidedIn) avoids creating a second provider since NGXS registers the state. This prevents duplicate instances.
27-27: Good: Added Injectable import and decoratorThis aligns the state with Angular 9/Ivy expectations so constructor injection works reliably in AOT.
Please manually verify that all other NGXS
@Stateclasses include@Injectable()immediately after@State(...).editor/src/app/user/user.state.ts (1)
25-25: Good Ivy readiness: add@Injectable()to NGXS state.This is the right move for Angular 9 to ensure DI metadata is emitted. No
providedInis correct since NGXS registers states.Also applies to: 40-40
editor/src/app/sites/settings/site-settings-config.state.ts (1)
24-24: Ivy/DI metadata added — looks right.Adding
@Injectable()to the state class aligns with Angular 9/Ivy and NGXS best practices.Also applies to: 30-30
editor/src/app/app-state/app.state.ts (1)
27-27: Good: add@Injectable()to NGXS AppState for Angular 9.This ensures DI metadata under Ivy without changing Store registration.
Also applies to: 55-55
editor/src/app/shop/products/shop-products.state.ts (1)
35-35: Correct Ivy metadata for state.
@Injectable()addition is appropriate and non‑intrusive.Also applies to: 55-55
editor/tsconfig.json (1)
13-14: Verified Angular 9.x and TypeScript 3.8.x in editor/package.json – lib bump is safe.editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (1)
38-64: DI metadata addition looks good.Adding
@Injectable()to the state aligns with Angular 9 + NGXS expectations for Ivy, so this update is on point.editor/src/app/sites/sections/tags/section-tags.state.ts (1)
32-39: State now DI-ready under Ivy.The added
@Injectable()decorator ensures this NGXS state retains the necessary metadata after the Angular 9 upgrade.editor/src/app/shop/settings/shop-settings.state.ts (1)
36-52: Confirmed Angular 9 compatibility.Bringing in
@Injectable()keeps the state injectable when Ivy is enabled—change looks correct.editor/src/app/sites/settings/site-settings.state.ts (1)
38-45: Good call on the Injectable decorator.This mirrors the NGXS guidance for Angular 9 upgrades and keeps the state class resolvable.
editor/src/app/sites/sites-state/sites.state.ts (1)
55-62: LGTM for Ivy-ready metadata.Marking the state
@Injectable()is the right move for Angular 9; no further issues spotted.editor/src/app/shop/shop.state.ts (1)
43-44: DI metadata added — required for Ivy/AOT.Marking the state with
@Injectable()is correct since the class has injected deps. No further changes needed.Also applies to: 54-55
editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
55-56: DI metadata added — good.Annotating NGXS state with
@Injectable()aligns with Angular 9 Ivy/AOT and your broader PR pattern.Also applies to: 61-63
editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (1)
49-50: DI metadata added — good.
@Injectable()on this NGXS state is correct for Ivy/AOT.Also applies to: 55-57
| "@angular/animations": "^9.1.13", | ||
| "@angular/cdk": "^9.1.13", | ||
| "@angular/common": "^9.1.13", | ||
| "@angular/compiler": "^9.1.13", | ||
| "@angular/core": "^9.1.13", | ||
| "@angular/forms": "^9.1.13", | ||
| "@angular/platform-browser": "^9.1.13", | ||
| "@angular/platform-browser-dynamic": "^9.1.13", | ||
| "@angular/router": "^9.1.13", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Tooling alignment for Angular 9: tests and ngcc.
-
Tests:
karma1.x andjasmine-core2.x are very old and may break with@angular-devkit/build-angular0.901.x. Align to Angular 9 CLI scaffolded versions (Karma 4/5, Jasmine 3.x, compatible launchers/plugins). -
ngcc: add a
postinstallto process View Engine libs under Ivy for reliable CI/first-run builds.
"scripts": {
"ng": "ng",
"start": "ng serve",
"build": "ng build",
"dev": "ng build --watch",
"test": "ng test",
"lint": "ng lint",
- "e2e": "ng e2e"
+ "e2e": "ng e2e",
+ "postinstall": "ngcc --properties es2015 browser module main --first-only"
},Also applies to: 36-36, 39-46, 49-51, 64-64
🏁 Script executed:
#!/bin/bash
# Inspect Angular builder + test deps
jq -r '.projects|keys[] as $k | "\($k): " + (.[$k].architect.test.builder // "")' editor/angular.json 2>/dev/null
rg -n 'karma|jasmine-core|karma-jasmine|karma-chrome-launcher|karma-coverage-istanbul-reporter' editor/package.jsonLength of output: 291
🏁 Script executed:
#!/bin/bash
set -e
# Pack schematics package for Angular 9.1.13 and inspect default package.json template
npm pack @schematics/angular@9.1.13 --silent
TARBALL=$(ls *.tgz | grep "schematics-angular" | head -n1)
echo "Using tarball: $TARBALL"
# Extract the default package.json template
tar -xzf "$TARBALL" "package/files/package.json"
# Show devDependencies section including test deps
sed -n '/"devDependencies"/,/"dependencies":/p' package/files/package.jsonLength of output: 184
🏁 Script executed:
#!/bin/bash
set -e
TARBALL=$(ls schematics-angular-9.1.13.tgz)
echo "Contents of $TARBALL:"
tar -tzf "$TARBALL" | grep -E 'files|package.json'Length of output: 4104
🏁 Script executed:
#!/bin/bash
TARBALL=$(ls schematics-angular-9.1.13.tgz)
tar -xOzf "$TARBALL" package/workspace/files/package.json.template | sed -n '/"devDependencies"/,/"dependencies":/p'Length of output: 674
Update test devDependencies to Angular 9 defaults.
- In editor/package.json, bump test libraries:
• jasmine-core:~2.99.1→~3.5.0
• karma:~1.7.1→~5.0.0
• karma-chrome-launcher:~2.2.0→~3.1.0
• karma-coverage-istanbul-reporter:~2.0.0→~2.1.0
• karma-jasmine:~1.1.1→~3.0.1
• karma-jasmine-html-reporter:^0.2.2→^1.4.2
• add jasmine-spec-reporter:~4.2.1 - Confirm your test builder remains
@angular-devkit/build-angular:karma.
Optional: Postinstall ngcc—CLI v9+ invokes ngcc on demand; add "postinstall": "ngcc" only if you encounter library compatibility issues (github.com)
🤖 Prompt for AI Agents
In editor/package.json around lines 15-23, the test devDependencies need to be
updated to Angular 9 defaults: bump jasmine-core to ~3.5.0, karma to ~5.0.0,
karma-chrome-launcher to ~3.1.0, karma-coverage-istanbul-reporter to ~2.1.0,
karma-jasmine to ~3.0.1, karma-jasmine-html-reporter to ^1.4.2, and add
jasmine-spec-reporter at ~4.2.1; ensure the test builder remains
"@angular-devkit/build-angular:karma" in the scripts/config and, if you
encounter library compatibility issues, optionally add a "postinstall": "ngcc"
script.
1 participant
Summary by CodeRabbit