Conversation
…wig template rendering refactoring (twig-load removed). Webpack removed.
uldisrudzitis
added
Enhancement
dependencies
WalkthroughSwitches the editor build to Angular DevKit v18, removes custom webpack/twig-loader, adds prebuild scripts to copy and bundle backend .twig files into a generated TWIG_TEMPLATES map, and introduces a runtime TwigTemplateRenderService used across many renderers. Also modernizes NGXS selectors, combineLatest usage, HttpClient provisioning, and various import paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component
participant RenderSvc as SpecificRenderService
participant TwigSvc as TwigTemplateRenderService
participant Templates as TWIG_TEMPLATES
note over Component,RenderSvc: Runtime Twig rendering replaces static Template imports
User->>Component: Trigger render()
Component->>RenderSvc: startRender(viewData)
RenderSvc->>TwigSvc: render(templateKey, viewData)
TwigSvc->>Templates: lookup(templateKey)
alt template found
TwigSvc->>TwigSvc: compile & cache (Twig.twig)
TwigSvc-->>RenderSvc: html string
RenderSvc-->>Component: html string
Component-->>User: Display HTML
else missing/failed
TwigSvc-->>RenderSvc: throw / error
RenderSvc-->>Component: '' (logged)
Component-->>User: Fallback display
end
sequenceDiagram
autonumber
participant NPM as npm run build
participant Pre as prebuild
participant Copy as copy-twig-templates.js
participant Bundle as bundle-twig-templates.js
participant FS as FileSystem
participant App as Angular build
note over NPM,Bundle: New prebuild pipeline replaces webpack twig-loader step
NPM->>Pre: prebuild
Pre->>Copy: copy backend .twig -> src/templates
Copy->>FS: read/write
Pre->>Bundle: scan src/templates -> emit src/app/render/twig-templates.ts
Bundle->>FS: write generated TS
NPM->>App: build with @angular-devkit builder
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/src/app/shop/settings/shop-settings.state.ts (1)
59-78: Restore the unused state argument to keep selector parameters aligned.In NGXS, selector handlers still receive the local state slice as the first argument, even when you supply dependencies. By dropping the
_parameter,currentSiteSettingsnow receives the entireShopSettingsModelobject, so the subsequent.find(...)call will throw (currentSiteSettings.find is not a function). Please reintroduce the leading state parameter (e.g._state) so the dependency output continues to land in the second slot, or adjust the decorator accordingly.- static getCurrentWeightUnit(currentSiteSettings) { + static getCurrentWeightUnit( + _state: ShopSettingsModel, + currentSiteSettings + ) { ... - static getCurrentCurrency(currentSiteSettings) { + static getCurrentCurrency( + _state: ShopSettingsModel, + currentSiteSettings + ) {editor/src/app/sites/template-settings/site-templates.state.ts (1)
35-44: Fix the selector dependency orderingIncluding
SiteTemplatesStatein the dependency list shifts the argument order sotemplateSlugnow receives the entire state object. The selector then looks upstate['[object Object]'], always returningundefined, which breaks every site-template lookup.- @Selector([SiteTemplatesState, SiteSettingsState.getCurrentSiteTemplate]) + @Selector([SiteSettingsState.getCurrentSiteTemplate]) static getCurrentTemplate( state: SiteTemplatesStateModel, templateSlug: string ) {
🧹 Nitpick comments (6)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
76-76: Add explicit parameter types for consistency.The parameters
stateandsitelack explicit type annotations, which is inconsistent with thegetCurrentSiteSectionsmethod (lines 64-66) that was updated with explicit types.Apply this diff to add explicit types:
- static getCurrentSiteShopSections(state, site): SiteSectionStateModel[] { + static getCurrentSiteShopSections( + state: SiteSectionStateModel[], + site: string + ): SiteSectionStateModel[] {editor/src/app/shop/products/shop-products.component.ts (1)
54-66: LGTM! combineLatest array form is required for RxJS 7 compatibility.The update to
combineLatest([...])array syntax is correct for RxJS 7.8.2 (the deprecated multi-argument signature was removed). The explicit type array provides good type safety for the map callback.Optional improvement: Consider replacing the
anytype for products (line 65) with a specific interface type for better type safety. However, this appears to be pre-existing technical debt rather than something introduced in this PR.editor/src/app/sites/site.component.ts (2)
134-143: Consider simplifying the nested switchMap.The nested
switchMapstructure is functionally correct but could be simplified by selecting the sites state earlier in the operator chain.Consider this refactor:
- switchMap(() => - this.store - .select((state) => state.sites) - .pipe( - take(1), - map((sitesState: SiteStateModel[]) => - sitesState.find((site) => site.order === this.site.order) - ) - ) - ) + switchMap(() => this.store.select((state) => state.sites)), + take(1), + map((sitesState: SiteStateModel[]) => + sitesState.find((site) => site.order === this.site.order) + )
167-179: Consider simplifying the nested switchMap.Similar to the rename flow, this nested
switchMapstructure could be simplified for better readability.Consider this refactor:
- switchMap(() => - this.store - .select((state) => state.sites) - .pipe( - take(1), - map((sitesState: SiteStateModel[]) => { - return sitesState.find( - (site) => siteNames.indexOf(site.name) === -1 - ); - }) - ) - ) + switchMap(() => this.store.select((state) => state.sites)), + take(1), + map((sitesState: SiteStateModel[]) => + sitesState.find((site) => siteNames.indexOf(site.name) === -1) + )editor/copy-twig-templates.js (1)
1-3: Update the outdated comment.The comment on line 3 references "twig-loader," but according to the PR summary, the webpack twig-loader has been removed in favor of a runtime Twig rendering service. Consider updating the comment to reflect the current approach.
Apply this diff:
// Angular uses the same twig templates from backend // This copy twig templates from backend to frontend -// so that they can be used in the Angular app via twig-loader +// so that they can be bundled and used in the Angular app via TwigTemplateRenderServiceeditor/src/app/sites/sections/entries/galleries/gallery-column-render.service.ts (1)
105-113: Consider error propagation strategy for template rendering failures.The try/catch blocks correctly handle rendering errors by logging and returning an empty string. However, silently returning empty strings might mask template issues in production.
Consider one of these approaches:
- Re-throw errors in development mode to catch issues early
- Return a minimal error placeholder in edit mode
- Add monitoring/alerting for rendering failures
Example for development-mode error propagation:
} catch (error) { console.error('Failed to render template:', error); if (!environment.production) { throw error; // Fail fast in development } return ''; }Also applies to: 130-138
📜 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 (68)
editor/angular.json(2 hunks)editor/bundle-twig-templates.js(1 hunks)editor/copy-twig-templates.js(1 hunks)editor/extra-webpack.config.js(0 hunks)editor/package.json(2 hunks)editor/src/app/app-state/app-state.service.ts(2 hunks)editor/src/app/app.component.ts(4 hunks)editor/src/app/app.module.ts(3 hunks)editor/src/app/login/login.component.ts(1 hunks)editor/src/app/preview/preview.component.ts(7 hunks)editor/src/app/preview/preview.service.ts(10 hunks)editor/src/app/preview/style.service.ts(1 hunks)editor/src/app/profile-dropdown/profile-dropdown.component.ts(1 hunks)editor/src/app/render/default-template-render.service.ts(4 hunks)editor/src/app/render/mashup-template-render.service.ts(4 hunks)editor/src/app/render/messy-template-render.service.ts(4 hunks)editor/src/app/render/template-render.service.ts(5 hunks)editor/src/app/render/twig-template-render.service.ts(1 hunks)editor/src/app/render/twig-templates.ts(1 hunks)editor/src/app/render/white-template-render.service.ts(4 hunks)editor/src/app/shop/orders/shop-orders.state.ts(1 hunks)editor/src/app/shop/products/shop-products.component.ts(2 hunks)editor/src/app/shop/products/shop-products.state.ts(1 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.component.ts(1 hunks)editor/src/app/shop/regional-costs/shop-regional-costs.state.ts(1 hunks)editor/src/app/shop/settings/shop-settings.component.ts(1 hunks)editor/src/app/shop/settings/shop-settings.state.ts(2 hunks)editor/src/app/shop/shop-cart-render.service.ts(5 hunks)editor/src/app/shop/shop-state.service.ts(1 hunks)editor/src/app/shop/shop.state.ts(1 hunks)editor/src/app/sites/sections/additional-footer-text-render.service.ts(3 hunks)editor/src/app/sites/sections/additional-text-render.service.ts(3 hunks)editor/src/app/sites/sections/background-gallery-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts(1 hunks)editor/src/app/sites/sections/entries/galleries/gallery-column-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/galleries/gallery-link-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/galleries/gallery-pile-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/galleries/gallery-render.service.ts(1 hunks)editor/src/app/sites/sections/entries/galleries/gallery-row-render.service.ts(3 hunks)editor/src/app/sites/sections/entries/galleries/gallery-slideshow-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/mashup-entries-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/portfolio-thumbnails-render.service.ts(2 hunks)editor/src/app/sites/sections/entries/section-entry-render.service.ts(4 hunks)editor/src/app/sites/sections/grid-view-render.service.ts(2 hunks)editor/src/app/sites/sections/section-footer-render.service.ts(2 hunks)editor/src/app/sites/sections/section-head-render.service.ts(2 hunks)editor/src/app/sites/sections/section.component.ts(3 hunks)editor/src/app/sites/sections/sections-menu-render.service.ts(2 hunks)editor/src/app/sites/sections/sections-state/site-sections.state.ts(2 hunks)editor/src/app/sites/sections/site-sections.component.ts(3 hunks)editor/src/app/sites/sections/tags/section-tags.state.ts(1 hunks)editor/src/app/sites/settings/site-settings.component.ts(3 hunks)editor/src/app/sites/settings/site-settings.state.ts(3 hunks)editor/src/app/sites/shared/file-upload.service.ts(1 hunks)editor/src/app/sites/shared/sites-shared.module.ts(1 hunks)editor/src/app/sites/site.component.ts(2 hunks)editor/src/app/sites/sites-banners-render.service.ts(2 hunks)editor/src/app/sites/sites-header-render.service.ts(3 hunks)editor/src/app/sites/sites-menu-render.service.ts(2 hunks)editor/src/app/sites/sites-state/sites.state.ts(2 hunks)editor/src/app/sites/sites.component.ts(1 hunks)editor/src/app/sites/social-media-links-render.service.ts(2 hunks)editor/src/app/sites/template-settings/site-template-settings.component.ts(2 hunks)editor/src/app/sites/template-settings/site-template-settings.state.ts(2 hunks)editor/src/app/sites/template-settings/site-templates.state.ts(3 hunks)editor/src/app/themes/themes.component.ts(1 hunks)editor/src/app/user/user.service.ts(1 hunks)editor/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- editor/extra-webpack.config.js
🧰 Additional context used
🧬 Code graph analysis (28)
editor/src/app/themes/themes.component.ts (1)
editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)
editor/src/app/sites/sections/grid-view-render.service.ts (2)
editor/src/app/render/messy-template-render.service.ts (1)
Injectable(25-357)editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/login/login.component.ts (1)
editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)
editor/src/app/sites/sites-state/sites.state.ts (5)
editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
DeleteSiteSectionsAction(104-107)editor/src/app/sites/settings/site-settings.actions.ts (1)
DeleteSiteSettingsAction(39-42)editor/src/app/sites/template-settings/site-template-settings.actions.ts (1)
DeleteSiteTemplateSettingsAction(36-39)editor/src/app/sites/sections/tags/section-tags.actions.ts (1)
DeleteSiteSectionsTagsAction(44-47)editor/src/app/sites/sections/entries/entries-state/section-entries.actions.ts (1)
DeleteSiteSectionsEntriesAction(45-48)
editor/src/app/shop/shop-cart-render.service.ts (1)
editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/sections/sections-menu-render.service.ts (4)
editor/src/app/render/default-template-render.service.ts (1)
Injectable(23-125)editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/shop/shop-cart-render.service.ts (1)
Injectable(6-721)editor/src/app/sites/sites-header-render.service.ts (1)
Injectable(5-168)
editor/src/app/preview/preview.component.ts (3)
editor/src/app/user/user.state.ts (1)
isLoggedIn(49-51)editor/src/app/user/user.actions.ts (1)
UserLogoutAction(12-15)editor/src/app/app-state/app.actions.ts (1)
AppShowLoading(16-18)
editor/src/app/sites/sections/entries/portfolio-thumbnails-render.service.ts (2)
editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/template-settings/site-templates.state.ts (1)
editor/src/app/sites/template-settings/site-templates.interface.ts (1)
TemplateSiteModel(15-21)
editor/src/app/sites/sites-banners-render.service.ts (2)
editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/sections/background-gallery-render.service.ts (5)
editor/src/app/render/default-template-render.service.ts (1)
Injectable(23-125)editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/sites/sections/grid-view-render.service.ts (1)
Injectable(7-109)editor/src/app/sites/sections/sections-menu-render.service.ts (1)
Injectable(7-349)editor/src/app/sites/sites-header-render.service.ts (1)
Injectable(5-168)
editor/src/app/sites/sections/section-head-render.service.ts (2)
editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/site.component.ts (2)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/sites/sites-state/sites.actions.ts (1)
CloneSiteAction(22-25)
editor/src/app/sites/sections/section-footer-render.service.ts (2)
editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/sites.component.ts (2)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/sites/sites-state/sites.actions.ts (1)
CreateSiteAction(3-6)
editor/src/app/sites/sections/entries/mashup-entries-render.service.ts (2)
editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/template-settings/site-template-settings.state.ts (1)
editor/src/app/sites/template-settings/site-template-settings.interface.ts (1)
SitesTemplateSettingsStateModel(3-5)
editor/src/app/profile-dropdown/profile-dropdown.component.ts (1)
editor/src/app/user/user.actions.ts (1)
UserLogoutAction(12-15)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)
editor/src/app/sites/sections/entries/galleries/gallery-render.service.ts (2)
editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)editor/src/app/sites/sections/entries/galleries/gallery-row-render.service.ts (1)
Injectable(5-188)
editor/src/app/sites/sections/entries/section-entry-render.service.ts (1)
editor/src/app/shared/helpers.ts (3)
toHtmlAttributes(271-285)formatPrice(351-358)toCartAttributes(360-365)
editor/src/app/render/twig-template-render.service.ts (3)
editor/src/app/render/default-template-render.service.ts (1)
Injectable(23-125)editor/src/app/render/template-render.service.ts (1)
Injectable(34-390)editor/src/app/render/twig-templates.ts (1)
TWIG_TEMPLATES(4-1500)
editor/src/app/sites/settings/site-settings.component.ts (2)
editor/src/app/user/user.state.model.ts (1)
UserStateModel(1-19)editor/src/app/app-state/app-state.interface.ts (1)
AppStateModel(7-27)
editor/src/app/sites/sections/additional-text-render.service.ts (1)
_api_app/app/Sites/SocialMediaLinksRenderService.php (1)
SocialMediaLinksRenderService(5-53)
editor/src/app/app.component.ts (2)
editor/src/app/app-state/app.actions.ts (2)
AppHideOverlay(12-14)AppShowOverlay(8-10)editor/src/app/user/user.actions.ts (1)
UserLoginAction(1-10)
editor/src/app/sites/social-media-links-render.service.ts (4)
editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)editor/src/app/sites/sections/additional-footer-text-render.service.ts (1)
Injectable(8-94)editor/src/app/sites/sections/additional-text-render.service.ts (1)
Injectable(6-145)editor/src/app/sites/sections/section-render.service.ts (1)
Injectable(6-141)
editor/src/app/sites/sites-menu-render.service.ts (1)
editor/src/app/render/twig-template-render.service.ts (1)
Injectable(6-60)
editor/src/app/sites/sections/site-sections.component.ts (1)
editor/src/app/sites/sections/sections-state/site-sections.actions.ts (1)
CreateSectionAction(4-7)
🔇 Additional comments (50)
editor/src/app/sites/shared/file-upload.service.ts (1)
24-27: LGTM! Modern RxJS syntax adopted.The migration from variadic
combineLatest(obsA, obsB)to array formcombineLatest([obsA, obsB])correctly aligns with RxJS 7.x best practices. The variadic form was deprecated in RxJS 6.5, and this change ensures forward compatibility while maintaining identical behavior.editor/src/app/sites/sections/section.component.ts (2)
7-7: LGTM! Import paths correctly updated to relative paths.The transition from absolute to relative import paths aligns with the broader project refactoring and correctly resolves to the intended modules.
Also applies to: 17-17
230-233: LGTM! Correctly updated combineLatest to array syntax for RxJS 7.x.The refactor from
combineLatest(obs1, obs2)tocombineLatest([obs1, obs2])is necessary for RxJS 7.x compatibility, as the multi-argument signature was deprecated. The functional behavior remains identical, and the downstream destructuring in themapoperator continues to work correctly.editor/src/app/sites/sections/sections-state/site-sections.state.ts (2)
63-73: LGTM! Selector decorator correctly updated.The decorator change from
@Selector([AppState.getSite])to@Selector([SiteSectionsState, AppState.getSite])correctly declares the state dependency, aligning with NGXS requirements. The explicit parameter typing also improves type safety.
75-75: LGTM! Decorator correctly updated.The decorator change to include
SiteSectionsStateis correct and consistent with the NGXS selector pattern.editor/src/app/shop/products/shop-products.component.ts (2)
1-4: LGTM! Import modernization aligns with Angular 18 and NGXS patterns.The import changes correctly update to modern patterns:
- Removed unused
isDevModeandtapimports- Using
Store.select()pattern instead of@Selectdecorator is a valid NGXS approach
12-14: LGTM! Import paths correctly updated to relative paths.The change from absolute (
src/app/...) to relative (../../...) import paths follows Angular best practices and resolves correctly to the target modules.editor/src/app/sites/sections/tags/section-tags.state.ts (1)
40-43: Approve Selector Parameter FixThe updated
@Selector([SectionTagsState, AppState.getSite])now correctly passes the state object then the site key. All existinggetCurrentSiteTagscalls (store.select/selectSnapshot) remain compatible and will return the expected tag arrays.editor/src/app/shop/regional-costs/shop-regional-costs.component.ts (1)
6-6: LGTM!The import path has been correctly updated to use a relative path.
editor/src/app/shop/regional-costs/shop-regional-costs.state.ts (1)
65-71: LGTM! Selector dependency update follows NGXS best practices.Adding
ShopRegionalCostsStateas the first dependency ensures the selector properly recomputes when the state slice changes. This change is consistent with the broader selector refactoring pattern across shop state files.editor/src/app/shop/orders/shop-orders.state.ts (1)
35-38: LGTM! Selector dependency update follows NGXS best practices.Adding
ShopOrdersStateas the first dependency ensures proper reactivity and memoization. This change aligns with the selector refactoring pattern across the codebase.editor/src/app/sites/sections/entries/entries-state/section-entries.state.ts (1)
57-61: LGTM! Selector dependency update follows NGXS best practices.Adding
SectionEntriesStateas the first dependency ensures the selector recomputes when the state changes. This is consistent with the broader refactoring pattern in this PR.editor/src/app/shop/products/shop-products.state.ts (1)
57-63: LGTM! Selector dependency update follows NGXS best practices.Adding
ShopProductsStateas the first dependency ensures proper selector reactivity. This change completes the consistent selector refactoring pattern across all shop state files.editor/src/app/shop/shop.state.ts (1)
70-70: Verify root state and selector usage
- Ensure
AppStateModeldeclares a top-levelsitesproperty.- Restore or replace the inline selector with the typed, memoized
SitesState.getSitesto preserve type safety and performance.editor/src/app/sites/sites.component.ts (1)
49-49: Approve NGXS async createSite flow
CreateSiteAction handler returns an observable that updates state before completion; component selectOnce then correctly retrieves the new site.editor/tsconfig.json (1)
7-7: LGTM!The addition of
esModuleInterop: trueis appropriate for this upgrade, as it improves interoperability with CommonJS modules used in the new Twig rendering infrastructure.editor/src/app/themes/themes.component.ts (1)
77-88: LGTM!The migration to using a state projection (
state => state.app) instead of selectingAppStatedirectly is consistent with the broader refactoring pattern in this PR and aligns with NGXS best practices.editor/src/app/shop/shop-state.service.ts (1)
42-45: LGTM!The migration to the array form of
combineLatest([...])is correct for RxJS 7+ and aligns with the modern RxJS API conventions used throughout this PR.editor/src/app/app-state/app-state.service.ts (2)
54-57: LGTM!The migration to
combineLatest([...])array form is correct and consistent with RxJS 7+ conventions.
137-140: LGTM!The migration to
combineLatest([...])array form is correct and consistent with RxJS 7+ conventions.editor/src/app/sites/social-media-links-render.service.ts (2)
2-2: LGTM!The integration of
TwigTemplateRenderServiceis part of the broader architectural shift to centralized Twig template rendering in this PR.
36-44: LGTM!The try/catch error handling with fallback to an empty string is a good practice and consistent with the pattern used across other render services in this PR.
editor/angular.json (5)
37-37: LGTM!The migration to
@angular-devkit/build-angular:applicationis the correct approach for Angular 18, replacing the deprecated custom webpack builder.
45-45: LGTM!The migration of
polyfillsfrom a string to an array is required for the new application builder in Angular 18.
50-51: LGTM!The addition of
twig.min.jsas a script and thebrowserentry point align with the new Twig rendering architecture introduced in this PR.
68-72: LGTM!The addition of a
developmentconfiguration with appropriate optimization and source map settings is a good practice for Angular 18 projects.
39-42: Re-run build to confirm emptybrowserpath
The build failed (noeditordirectory due to ``). Please re-run with the repo cloned and verify../engine/distincludes the browser output:cd editor npm run build ls -la ../engine/dist/editor/src/app/login/login.component.ts (1)
124-128: LGTM!The selector change from
AppStateto(state) => state.appcorrectly accesses the app slice of the state and properly types the subscription callback. This aligns with the broader state access refactoring across the PR.editor/src/app/sites/template-settings/site-template-settings.state.ts (2)
43-57: LGTM!The selector properly declares dependencies on multiple states (
SiteTemplateSettingsState,AppState.getSite,SiteSettingsState.getCurrentSiteTemplate) and correctly types the first parameter asSitesTemplateSettingsStateModelinstead of the state class. This follows NGXS best practices for multi-state selectors.
59-73: LGTM!The addition of optional chaining (
currentSiteTemplateSettings?.find) improves null safety and prevents potential runtime errors whencurrentSiteTemplateSettingsis undefined.editor/src/app/sites/sections/entries/galleries/gallery-slideshow-render.service.ts (1)
159-192: LGTM with same verification as gallery-link-render.service.ts.The changes correctly migrate to
twigTemplateRenderService.renderwith proper error handling. Since this service also extendsGalleryRenderService, the verification script in the gallery-link-render.service.ts review applies here as well.editor/src/app/preview/preview.service.ts (4)
5-5: LGTM!The addition of
mergeMapimport is correctly used at line 106 for the dispatch-then-select pattern in the siteSettings route.
106-157: LGTM!The changes from direct property access (
state.siteSettings) to bracket notation (state['siteSettings']) enable dynamic property access while maintaining type safety through explicit type annotations (line 107). This pattern is consistent with the broader state structure refactoring across the PR.
170-228: LGTM!The bracket notation changes for
state['siteSections']are consistent with the refactoring pattern.
241-458: LGTM!The bracket notation changes for
state['sectionTags']andstate['sectionEntries']are consistent with the refactoring pattern throughout the file.editor/src/app/sites/sections/entries/galleries/gallery-row-render.service.ts (2)
3-3: LGTM!The import path update for
AppStateModelfrom'../../../../app-state/app-state.interface'aligns with the module structure.
153-186: LGTM with same verification as gallery-link-render.service.ts.The changes correctly migrate to
twigTemplateRenderService.renderwith proper error handling. Since this service also extendsGalleryRenderService, the verification script in the gallery-link-render.service.ts review applies here as well.editor/src/app/sites/sections/background-gallery-render.service.ts (3)
4-4: LGTM!The import of
TwigTemplateRenderServiceis correctly added to support the rendering migration.
12-13: LGTM!The constructor properly injects
TwigTemplateRenderService, which is then used in the render method.
150-158: LGTM!The migration from direct Template invocation to
twigTemplateRenderService.renderwith try/catch error handling is correctly implemented and consistent with the broader PR migration pattern.editor/src/app/sites/sections/entries/galleries/gallery-link-render.service.ts (1)
121-154: Approve changes—GalleryRenderServiceconstructor publicly injectsTwigTemplateRenderService, sothis.twigTemplateRenderService.renderis properly available.editor/src/app/app.module.ts (2)
3-6: HTTP client migration to DI-based provisioning looks good.The migration from
HttpClientModuletoprovideHttpClient(withInterceptorsFromDi())correctly adopts the Angular 18+ DI-based HTTP client pattern. ThewithInterceptorsFromDi()function ensures backward compatibility with existing class-based interceptors.Also applies to: 89-89
56-56: No changes needed to bootstrap configuration. The project uses module-based bootstrap inmain.ts(platformBrowserDynamic().bootstrapModule(AppModule)), sobootstrap: [AppComponent]inAppModuleis expected.editor/src/app/sites/sections/entries/galleries/gallery-render.service.ts (1)
9-9: LGTM! Proper DI setup for template rendering service.The
publicvisibility ontwigTemplateRenderServicecorrectly allows child render services (e.g.,GalleryColumnRenderService,GalleryPileRenderService) to access the template rendering functionality.editor/src/app/sites/sites-state/sites.state.ts (2)
63-63: LGTM! Selector dependency correction.Adding
SitesStateto the selector dependencies ensures proper recomputation when the sites state changes. This aligns with NGXS best practices for selector memoization.
233-239: LGTM! Efficient action batching.Dispatching the deletion actions as an array is the correct NGXS pattern for batching related state changes, ensuring they're processed together and reducing state change notifications.
editor/src/app/sites/sections/entries/portfolio-thumbnails-render.service.ts (1)
16-16: LGTM! Consistent TwigTemplateRenderService integration.The migration to
TwigTemplateRenderServicefollows the same pattern as other render services in this PR, with appropriate error handling and fallback behavior.Also applies to: 104-112
editor/src/app/sites/sections/entries/galleries/gallery-pile-render.service.ts (1)
105-113: LGTM! Consistent template rendering migration.The migration follows the same pattern as other gallery render services, maintaining the existing conditional logic while adopting the centralized
TwigTemplateRenderService.Also applies to: 130-138
editor/src/app/sites/sites-banners-render.service.ts (1)
9-9: LGTM! Consistent service migration pattern.The TwigTemplateRenderService integration follows the established pattern across all render services in this PR, with appropriate constructor injection and error handling.
Also applies to: 100-108
editor/src/app/shop/settings/shop-settings.component.ts (1)
58-69: Approve changes Direct state projection ofstate.shopSettingsConfigmatches the NGXS state definition inShopSettingsConfigState.
|
|
||
| this.store | ||
| .select(ShopSettingsConfigState) | ||
| .select((state) => state.sitesettingsConfig) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the state property name.
The property name sitesettingsConfig (lowercase 's' in settings) appears unusual. Confirm that this matches the actual state property name. If the property is siteSettingsConfig (camelCase), this will cause a runtime error.
Run the following script to verify the correct property name:
🏁 Script executed:
#!/bin/bash
# Search for the state property definition to verify the correct name
rg -n "sitesettingsConfig|siteSettingsConfig" --type=ts -C3Length of output: 4762
Fix incorrect state property name: In editor/src/app/preview/style.service.ts line 46, change state.sitesettingsConfig to state.siteSettingsConfig to match the registered state.
🤖 Prompt for AI Agents
In editor/src/app/preview/style.service.ts around line 46, the selector
references the wrong state property name; replace state.sitesettingsConfig with
state.siteSettingsConfig (camel-cased S) to match the registered state property
so the selector reads state.siteSettingsConfig.
| const htmlOutput = this.twigTemplateRenderService.render( | ||
| 'Sites/Sections/defaultTemplate', | ||
| viewData | ||
| ); |
There was a problem hiding this comment.
Render the messy Twig template, not the default layout.
MessyTemplateRenderService should render Sites/Sections/messyTemplate. Pointing it at 'Sites/Sections/defaultTemplate' will load the wrong markup, breaking messy layout features (background gallery, floating banners, etc.). Please switch the template key back to the messy template.
- const htmlOutput = this.twigTemplateRenderService.render(
- 'Sites/Sections/defaultTemplate',
+ const htmlOutput = this.twigTemplateRenderService.render(
+ 'Sites/Sections/messyTemplate',
viewData
);📝 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 htmlOutput = this.twigTemplateRenderService.render( | |
| 'Sites/Sections/defaultTemplate', | |
| viewData | |
| ); | |
| const htmlOutput = this.twigTemplateRenderService.render( | |
| 'Sites/Sections/messyTemplate', | |
| viewData | |
| ); |
🤖 Prompt for AI Agents
In editor/src/app/render/messy-template-render.service.ts around lines 347 to
350, the service is mistakenly rendering 'Sites/Sections/defaultTemplate'
instead of the messy template; change the template key passed to
this.twigTemplateRenderService.render from 'Sites/Sections/defaultTemplate' to
'Sites/Sections/messyTemplate' so the messy layout (background gallery, floating
banners, etc.) is used, keeping the existing viewData argument unchanged.
| try { | ||
| const htmlOutput = this.twigTemplateRenderService.render( | ||
| 'Sites/Sections/whiteTemplate', | ||
| viewData | ||
| ); | ||
| this.replaceIframeContent(contentWindow, htmlOutput); | ||
| } catch (error) { | ||
| console.error('Failed to render template:', error); | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Don't return a string from startRender()
TemplateRenderService.startRender has a void signature. Returning '' in the override widens it to string | void, which TypeScript treats as incompatible and fails the build. Just log the error, optionally clear the iframe, and exit without a value.
} catch (error) {
console.error('Failed to render template:', error);
- return '';
+ this.replaceIframeContent(contentWindow, '');
+ return;
}📝 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.
| try { | |
| const htmlOutput = this.twigTemplateRenderService.render( | |
| 'Sites/Sections/whiteTemplate', | |
| viewData | |
| ); | |
| this.replaceIframeContent(contentWindow, htmlOutput); | |
| } catch (error) { | |
| console.error('Failed to render template:', error); | |
| return ''; | |
| } | |
| try { | |
| const htmlOutput = this.twigTemplateRenderService.render( | |
| 'Sites/Sections/whiteTemplate', | |
| viewData | |
| ); | |
| this.replaceIframeContent(contentWindow, htmlOutput); | |
| } catch (error) { | |
| console.error('Failed to render template:', error); | |
| this.replaceIframeContent(contentWindow, ''); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In editor/src/app/render/white-template-render.service.ts around lines 117 to
126, the catch block currently returns an empty string which widens the method
return type and breaks the build; remove the returned string and simply handle
the error by logging it and optionally clearing the iframe content (call
replaceIframeContent(contentWindow, '') or similar) before exiting the function
without a value so the method remains void-compatible.
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 (3)
editor/src/app/sites/site.component.ts (1)
122-146: Fix: prevent leak and wrong comparison after rename
- Using
this.site.nameafter dispatch likely compares against the new name, not the old; the filter may never pass.filter + take(1)onqueryParamscan hang indefinitely if the condition is false, leaking a subscription.- Also,
find(...)can return undefined; guard before navigating.Apply this diff:
updateField(field: string, value: string) { if (field === 'name') { - this.store + const prevName = this.site.name; + this.store .dispatch(new RenameSiteAction(this.site, value)) .pipe( - switchMap((state) => { - return this.route.queryParams.pipe( - take(1), - filter((params) => params.site && params.site === this.site.name), - map(() => state) - ); - }), - switchMap(() => this.store.select((state) => state.sites)), - take(1), + // Only update URL if the currently viewed site was the one renamed + filter(() => this.route.snapshot.queryParams.site === prevName), + switchMap(() => + this.store.select((state) => state.sites).pipe(take(1)) + ), map((sitesState: SiteStateModel[]) => sitesState.find((site) => site.order === this.site.order) ) ) - .subscribe((renamedSite) => { - this.router.navigate([], { + .subscribe((renamedSite) => { + if (!renamedSite) { + return; + } + this.router.navigate([], { replaceUrl: true, queryParams: { site: renamedSite.name }, queryParamsHandling: 'merge', }); });editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
75-84: Critical: Fix selector deps and null-guard @attributes; compose for memoizationSame dependency issue here. Also section['@attributes'].type will throw when @attributes is undefined. Compose from getCurrentSiteSections and guard access.
Apply:
- @Selector([SiteSectionsState, AppState.getSite]) - static getCurrentSiteShopSections( - state: SiteSectionStateModel[], - site: string - ): SiteSectionStateModel[] { - return SiteSectionsState.getCurrentSiteSections(state, site).filter( - (section) => - section['@attributes'].type && section['@attributes'].type === 'shop' - ); - } + @Selector([SiteSectionsState.getCurrentSiteSections]) + static getCurrentSiteShopSections( + sections: SiteSectionStateModel[] + ): SiteSectionStateModel[] { + return sections.filter((section) => section['@attributes']?.type === 'shop'); + }editor/src/app/render/mashup-template-render.service.ts (1)
90-101: Fix return type to match possible null values.getMainColumnAttributes returns nulls but declares string-only fields. This breaks under strictNullChecks.
Use nullable types:
- getMainColumnAttributes(siteTemplateSettings): { - class: string; - 'data-paddingtop': string; - } { + getMainColumnAttributes(siteTemplateSettings): { + class: string | null; + 'data-paddingtop': string | null; + } {Alternatively, omit keys when not set and mark them optional.
♻️ Duplicate comments (1)
editor/bundle-twig-templates.js (1)
41-44: Backslash escaping fix looks good.Escaping backslashes before other characters prevents JS template literal escape interpretation and preserves Twig content. Thanks for addressing this.
🧹 Nitpick comments (7)
editor/src/app/preview/style.service.ts (2)
13-16: Remove unused import.The
ShopSettingsConfigStateimport is no longer used after switching to the projection function on line 46.Apply this diff to remove the unused import:
import { ShopSettingsConfigModel, - ShopSettingsConfigState, } from '../shop/settings/shop-settings-config.state';
45-50: Use typed selector for shopSettingsConfig or remove unused importThe
state.shopSettingsConfigprojection matches the registered state name but bypasses compile‐time checks; revert to the typed selector (ShopSettingsConfigState…) for type safety and consistency, or drop the now-unusedShopSettingsConfigStateimport.editor/src/app/sites/site.component.ts (1)
152-166: Minor: use Set for membership check in clone flowReplace array lookup with a Set for O(1) membership and simpler intent.
cloneSite() { this.store .select((state) => state.sites) .pipe( take(1), - map((sites: SiteStateModel[]) => { - return sites.map((site) => site.name); - }), - switchMap((siteNames) => + map((sites: SiteStateModel[]) => new Set(sites.map((s) => s.name))), + switchMap((siteNames) => this.store.dispatch(new CloneSiteAction(this.site)).pipe( - switchMap(() => this.store.select((state) => state.sites)), - take(1), - map((sitesState: SiteStateModel[]) => - sitesState.find((site) => siteNames.indexOf(site.name) === -1) - ) + switchMap(() => this.store.select((state) => state.sites)), + take(1), + map((sitesState: SiteStateModel[]) => + sitesState.find((site) => !siteNames.has(site.name)) + ) ) ) )editor/src/app/render/twig-templates.ts (1)
1-2: Avoid embedding a timestamp to keep the generated file deterministic.The "Generated at" line will change every run and create noisy diffs. Prefer stable output for VCS hygiene.
Adjust the generator (editor/bundle-twig-templates.js) to omit or gate the timestamp so this file doesn't churn on every build.
editor/bundle-twig-templates.js (1)
34-36: Make the generator reproducible and robust (no timestamp, sorted, ensure output dir).
- Remove timestamp from header for deterministic diffs.
- Sort templates by name to avoid churn from FS ordering.
- Ensure output directory exists before write.
Apply this diff:
-const templates = findTwigFiles(TWIG_DIR); +const templates = findTwigFiles(TWIG_DIR).sort((a, b) => a.name.localeCompare(b.name)); -const output = `// Auto-generated file - do not edit manually -// Generated at: ${new Date().toISOString()} +const output = `// Auto-generated file - do not edit manually + export const TWIG_TEMPLATES: Record<string, string> = { ${templates .map( (t) => ` '${t.name}': \`${t.content .replace(/\\/g, "\\\\") .replace(/`/g, "\\`") .replace(/\$/g, "\\$")}\`` ) .join(",\n")} }; `; -fs.writeFileSync(OUTPUT_FILE, output, "utf8"); +fs.mkdirSync(path.dirname(OUTPUT_FILE), { recursive: true }); +fs.writeFileSync(OUTPUT_FILE, output, "utf8");Optionally, if you still want timestamps for logging, print them only to stdout.
Also applies to: 32-33, 50-50 </blockquote></details> <details> <summary>editor/src/app/render/mashup-template-render.service.ts (1)</summary><blockquote> `166-174`: **Optional: clear iframe on render error.** For a consistent fallback, clear the iframe when rendering fails. ```diff } catch (error) { console.error('Failed to render template:', error); + this.replaceIframeContent(contentWindow, ''); }editor/src/app/render/default-template-render.service.ts (1)
114-122: Approved—startRendercorrectly returns void. Optionally clear the iframe on error for consistency:} catch (error) { console.error('Failed to render template:', error); + this.replaceIframeContent(contentWindow, ''); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
editor/bundle-twig-templates.js(1 hunks)editor/copy-twig-templates.js(1 hunks)editor/src/app/preview/style.service.ts(1 hunks)editor/src/app/render/default-template-render.service.ts(4 hunks)editor/src/app/render/mashup-template-render.service.ts(4 hunks)editor/src/app/render/messy-template-render.service.ts(4 hunks)editor/src/app/render/twig-templates.ts(1 hunks)editor/src/app/render/white-template-render.service.ts(4 hunks)editor/src/app/sites/sections/sections-state/site-sections.state.ts(2 hunks)editor/src/app/sites/site.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- editor/src/app/render/messy-template-render.service.ts
- editor/copy-twig-templates.js
- editor/src/app/render/white-template-render.service.ts
🧰 Additional context used
🧬 Code graph analysis (2)
editor/src/app/sites/site.component.ts (2)
editor/src/app/sites/sites-state/site-state.model.ts (1)
SiteStateModel(1-9)editor/src/app/sites/sites-state/sites.actions.ts (1)
CloneSiteAction(22-25)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)
🔇 Additional comments (3)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (2)
15-15: assignByPath export and import path verified
HelperassignByPathis exported fromsrc/app/shared/helpers.tsand used consistently; no further action required.
63-73: No changes needed to @selector dependencies
In NGXS, specifying selectors overrides the implicit state, so you must includeSiteSectionsStatealongsideAppState.getSiteto receive both values.Likely an incorrect or invalid review comment.
editor/bundle-twig-templates.js (1)
1-9: Sanity-check for lingering Twig usage
Manually ensure there are notwig-loaderimports,.twigfile references,new Template(...), orrender(Template...)calls remaining in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
editor/copy-twig-templates.js (1)
7-9: Consider adding error handling for better diagnostics.While build scripts typically allow errors to propagate, wrapping the
cpx.copySynccall in a try/catch block with a descriptive error message would improve debuggability when the source directory is missing or permission issues occur.Apply this diff to add error handling:
+try { cpx.copySync("../_api_app/app/**/*.twig", "src/templates", { clean: true, }); - console.log("✓ Twig files copied from backend to Angular app."); + console.log("✓ Twig files copied from backend to Angular app."); +} catch (error) { + console.error("✗ Failed to copy Twig files:", error.message); + process.exit(1); +}editor/bundle-twig-templates.js (1)
10-30: Consider adding error handling for file system operations.The script performs multiple file system operations (
fs.readdirSync,fs.statSync,fs.readFileSync) without error handling. Adding try/catch blocks with descriptive error messages would help diagnose issues during the build process.Apply this diff to add error handling:
function findTwigFiles(dir, fileList = [], baseDir = dir) { + try { const files = fs.readdirSync(dir); files.forEach((file) => { const filePath = path.join(dir, file); const stat = fs.statSync(filePath); if (stat.isDirectory()) { findTwigFiles(filePath, fileList, baseDir); } else if (file.endsWith(".twig")) { const relativePath = path.relative(baseDir, filePath); const content = fs.readFileSync(filePath, "utf8"); const templateName = relativePath .replace(/\.twig$/, "") .replace(/\\/g, "/"); fileList.push({ name: templateName, content }); } }); + } catch (error) { + console.error(`Error processing directory ${dir}:`, error.message); + throw error; + } return fileList; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
editor/bundle-twig-templates.js(1 hunks)editor/copy-twig-templates.js(1 hunks)editor/src/app/preview/style.service.ts(2 hunks)editor/src/app/render/mashup-template-render.service.ts(5 hunks)editor/src/app/render/twig-templates.ts(1 hunks)editor/src/app/sites/sections/sections-state/site-sections.state.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/app/preview/style.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (1)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionStateModel(1-37)
🔇 Additional comments (6)
editor/src/app/sites/sections/sections-state/site-sections.state.ts (3)
15-15: LGTM! Import path correctly updated to relative path.The import path change from absolute to relative is correct for the file location and aligns with Angular best practices.
63-73: LGTM! Selector properly modernized with explicit state dependency.The decorator now correctly includes
SiteSectionsStateas the first dependency, which is the proper NGXS pattern for selectors that need access to their own state. The added explicit typing improves type safety.
75-82: Excellent selector composition! Architectural improvement.The refactored selector now properly composes from
getCurrentSiteSections, eliminating logic duplication and improving maintainability. Key benefits:
- DRY principle: Reuses site filtering logic instead of reimplementing it
- Efficiency: Operates on pre-filtered sections rather than the entire state
- Maintainability: Changes to
getCurrentSiteSectionsautomatically propagate- Clarity: Explicit dependency chain shows this selector builds on current site sections
The optional chaining for
@attributesis good defensive programming.editor/src/app/render/twig-templates.ts (1)
1-1499: LGTM! Auto-generated file.This auto-generated file correctly exports the centralized Twig template registry consumed by TwigTemplateRenderService across the application.
editor/src/app/render/mashup-template-render.service.ts (2)
21-21: LGTM! Consistent with new Twig rendering architecture.The addition of
TwigTemplateRenderServicedependency aligns with the project-wide migration to centralized Twig template rendering.Also applies to: 46-46, 67-67
90-93: LGTM! More accurate return type.The updated return type correctly reflects that
classanddata-paddingtopcan benull, matching the implementation's actual behavior (lines 96, 100).
| ` '${t.name}': \`${t.content | ||
| .replace(/\\/g, "\\\\") | ||
| .replace(/`/g, "\\`") | ||
| .replace(/\$/g, "\\$")}\`` | ||
| ) |
There was a problem hiding this comment.
Escape single quotes in template names to prevent syntax errors.
The template name t.name is embedded directly in single quotes without escaping. If a .twig file contains a single quote in its path (e.g., test's-template.twig), the generated TypeScript will have a syntax error.
Apply this diff to fix the issue:
` '${t.name}': \`${t.content
+ .replace(/'/g, "\\'")
.replace(/\\/g, "\\\\")
.replace(/`/g, "\\`")
.replace(/\$/g, "\\$")}\``Actually, better approach - use template name escaping:
- ` '${t.name}': \`${t.content
+ ` '${t.name.replace(/'/g, "\\'")}': \`${t.content
.replace(/\\/g, "\\\\")
.replace(/`/g, "\\`")
.replace(/\$/g, "\\$")}\``📝 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.
| ` '${t.name}': \`${t.content | |
| .replace(/\\/g, "\\\\") | |
| .replace(/`/g, "\\`") | |
| .replace(/\$/g, "\\$")}\`` | |
| ) | |
| // … | |
| ` '${t.name.replace(/'/g, "\\'")}': \`${t.content | |
| .replace(/\\/g, "\\\\") | |
| .replace(/`/g, "\\`") | |
| .replace(/\$/g, "\\$")}\`` | |
| // … |
🤖 Prompt for AI Agents
In editor/bundle-twig-templates.js around lines 40-44, the template name t.name
is interpolated into single quotes without escaping which breaks generated
TypeScript when names contain a single quote; update the generation to escape
single quotes in t.name before embedding (e.g., replace all "'" with "\\'") so
the produced object key remains syntactically valid, or alternatively switch to
a safe quoting strategy such as using JSON.stringify for the key to ensure all
necessary escapes are handled.
| try { | ||
| const htmlOutput = this.twigTemplateRenderService.render( | ||
| 'Sites/Sections/mashupTemplate', | ||
| viewData | ||
| ); | ||
| this.replaceIframeContent(contentWindow, htmlOutput); | ||
| } catch (error) { | ||
| console.error('Failed to render template:', error); | ||
| } |
There was a problem hiding this comment.
Silent failure leaves user with blank page.
The error is caught and logged but the method doesn't throw, return an error, or update the UI. If template rendering fails, the user sees a blank iframe with only a console error. Consider either throwing the error to let callers handle it, or calling replaceIframeContent with an error message.
Apply this diff to improve error handling:
try {
const htmlOutput = this.twigTemplateRenderService.render(
'Sites/Sections/mashupTemplate',
viewData
);
this.replaceIframeContent(contentWindow, htmlOutput);
} catch (error) {
console.error('Failed to render template:', error);
+ // Display error message to user instead of leaving blank page
+ this.replaceIframeContent(
+ contentWindow,
+ '<div style="padding: 20px; color: red;">Failed to render template. Check console for details.</div>'
+ );
}📝 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.
| try { | |
| const htmlOutput = this.twigTemplateRenderService.render( | |
| 'Sites/Sections/mashupTemplate', | |
| viewData | |
| ); | |
| this.replaceIframeContent(contentWindow, htmlOutput); | |
| } catch (error) { | |
| console.error('Failed to render template:', error); | |
| } | |
| try { | |
| const htmlOutput = this.twigTemplateRenderService.render( | |
| 'Sites/Sections/mashupTemplate', | |
| viewData | |
| ); | |
| this.replaceIframeContent(contentWindow, htmlOutput); | |
| } catch (error) { | |
| console.error('Failed to render template:', error); | |
| // Display error message to user instead of leaving blank page | |
| this.replaceIframeContent( | |
| contentWindow, | |
| '<div style="padding: 20px; color: red;">Failed to render template. Check console for details.</div>' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In editor/src/app/render/mashup-template-render.service.ts around lines 166 to
174, the catch block currently only logs the render error which leaves the
iframe blank; update the catch to call this.replaceIframeContent(contentWindow,
errorHtml) with a simple user-facing error HTML (message + optional details) so
the iframe shows an explanatory error, then rethrow the error (or return a
rejected Promise) so upstream callers can handle it; ensure the errorHtml is
safe/escaped and keep the console.error for diagnostics.
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores