feat(newsletters): native block-composer editor with template selection

[dealako]

Restores #1133 (reverted from main via #1200 to unblock scheduled releases).

This is a draft — stays off the release path until testing completes on this branch. Content is identical to the original #1133 (cherry-picked, authorship preserved).

Refs: LFXV2-2386

[coderabbitai]

Walkthrough

Adds a newsletter block-composer flow with structured layouts, template manifests, rendering APIs, Angular editing UI, persistence integration, and Playwright coverage for modern and legacy drafts.

Changes

Newsletter block composer

Layer / File(s)	Summary
Layout contracts and template APIs packages/shared/src/interfaces/newsletter.interface.ts, packages/shared/src/constants/newsletter.constants.ts, apps/lfx-one/src/server/..., apps/lfx-one/src/app/shared/services/newsletter-manifest.service.ts, apps/lfx-one/src/app/shared/services/newsletter.service.ts	Adds structured layout types, template manifest endpoints, render-preview APIs, payload validation, size limits, and cached manifest loading.
Declarative newsletter renderer apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts, packages/shared/src/utils/*	Parses templates, resolves bindings and slots, renders editable previews, sanitizes rich text and URLs, and supports wrapper chrome.
Block composer and field editing apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/*, apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/*	Adds palette, outline, drag-and-drop nesting, preview controls, inline editing, spacing controls, and reactive block fields.
Editor modes and newsletter persistence apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/*, apps/lfx-one/src/app/modules/newsletters/newsletter-manage/*, apps/lfx-one/src/app/modules/newsletters/components/newsletter-review/*	Connects Blocks and Simple modes, hydrates and saves body_layout, synchronizes rendered HTML, updates preview and save gating, and displays template metadata.
Wizard flow coverage apps/lfx-one/e2e/newsletter-composer-wizard.spec.ts	Covers layout hydration, composition, mode switching, legacy drafts, panel controls, and template-library compatibility.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Editor as NewsletterBlockComposerComponent
  participant Manifest as NewsletterManifestService
  participant API as NewsletterService
  participant Server as NewsletterController
  participant Manage as NewsletterManageComponent

  Editor->>Manifest: Load template manifest
  Manifest->>API: Request manifest
  API->>Server: GET template manifest
  Editor->>API: Request render preview
  API->>Server: POST body_layout
  Server-->>API: Return rendered body_html
  API-->>Editor: Return preview response
  Editor-->>Manage: Emit layoutChange
  Manage->>Server: Save body_layout and body_html

Loading

Possibly related PRs

• linuxfoundation/lfx-self-serve#1133: Implements the same newsletter block-composer files, APIs, layout types, and E2E flow.
• linuxfoundation/lfx-self-serve#1200: Reverts the related block-composer, template-rendering, API, and E2E changes.
• linuxfoundation/lfx-self-serve#1204: Removes obsolete helpers related to bodyPersistable in the newsletter management component.

Suggested labels: do-not-merge

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding a native newsletter block-composer editor with template selection.
Description check	✅ Passed	The description identifies this PR as a restoration of the newsletter block-composer work and explains its draft status and tracking reference.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/LFXV2-2386-newsletter-composer

---

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

PR Summary

Medium Risk
Large new editor surface changes newsletter authoring, save semantics, and send/preview gating; mitigated by E2E coverage and explicit layout/html mode handling.

Overview
Reintroduces the newsletter block composer in the creation wizard Content step (restoring work from #1133), so authors build body_layout instead of only rich-text body_html.

The Content step now defaults to Blocks (lfx-newsletter-block-composer) with a Simple toggle for the legacy rich-text + AI path. Switching modes confirms when content would be discarded and keeps a single authoritative body (body_layout vs body_html, including explicit null to clear a stored layout).

newsletter-manage gains a bodyLayout form control, persists body_layout on create/update, syncs server-rendered body_html after save in blocks mode, and tightens preview/send/test gates until layout edits are saved and rendered. Review shows template/neutral library labeling and treats composed blocks as “has body” before body_html catches up.

New UI includes manifest-driven palette/canvas (drag-drop, containers, template library switch with unsupported blocks dropped), schema-driven Fields panel, live preview/source/Gmail size hints, and a large Playwright spec for hydration, persistence, legacy drafts, and template switching.

^{Reviewed by Cursor Bugbot for commit 2f8bc9c. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

Pull request overview

Restores the draft newsletter block-composer feature with runtime template selection and server-rendered layouts.

Changes:

• Adds structured newsletter layout contracts and BFF endpoints.
• Introduces block composition, inline editing, previews, and template switching.
• Updates draft persistence, validation, review UI, and E2E coverage.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/shared/src/utils/url.utils.spec.ts	Tests safe URL validation.
packages/shared/src/utils/string.utils.ts	Adds field-key humanization.
packages/shared/src/interfaces/newsletter.interface.ts	Defines layout and manifest contracts.
packages/shared/src/constants/newsletter.constants.ts	Adds spacing and template defaults.
apps/lfx-one/src/server/services/newsletter.service.ts	Exposes template and preview operations.
apps/lfx-one/src/server/services/newsletter-service.client.ts	Proxies new upstream endpoints.
apps/lfx-one/src/server/routes/newsletters.route.ts	Registers template and preview routes.
apps/lfx-one/src/server/controllers/newsletter.controller.ts	Validates and handles layout requests.
apps/lfx-one/src/app/shared/services/newsletter.service.ts	Adds client preview rendering.
apps/lfx-one/src/app/shared/services/newsletter-manifest.service.ts	Loads and caches runtime manifests.
apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts	Renders declarative templates safely.
apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts	Integrates layout persistence and gating.
apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.html	Updates preview and draft actions.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-review/newsletter-review.component.ts	Adds layout-aware review details.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-review/newsletter-review.component.html	Displays template and preview state.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.ts	Implements editor-mode switching.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.html	Hosts composer and simple editors.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.ts	Builds schema-driven field forms.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.html	Renders block and spacing controls.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts	Implements composer behavior.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.scss	Styles rendered email previews.
apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.html	Defines composer palette, canvas, and sidebars.
apps/lfx-one/e2e/newsletter-composer-wizard.spec.ts	Covers hydration, persistence, and switching.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts (1)

504-510: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Close formatMissing before the next method
Missing the closing } before protected onAddAudienceEmail leaves newsletter-manage.component.ts unparsable and blocks the TypeScript build.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts`
around lines 504 - 510, Close the private formatMissing method with a closing
brace before protected onAddAudienceEmail, preserving its existing formatting
and return behavior so the component parses correctly.

Source: Linters/SAST tools

🧹 Nitpick comments (6)

apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.ts (1)

214-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Orphaned doc comment.

The Build the reactive form… one-liner at Line 214 now sits above fieldsKey's own doc block, so buildForm (Line 224) reads as undocumented and fieldsKey appears to have two descriptions. Move it directly above buildForm.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.ts`
around lines 214 - 224, Move the “Build the reactive form for a block and wire
its value-change emit” doc comment from above fieldsKey to directly above
buildForm. Keep fieldsKey’s existing schema-signature documentation immediately
above fieldsKey.

apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts (2)

1017-1032: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Doc block is attached to the wrong method.

The JSDoc at Lines 1017-1024 describes container reconciliation but sits above moveTopLevelBlock; reconcileContainers (Line 1034) is left undocumented.

♻️ Move the doc block

-  /**
-   * After the manifest resolves, promote any top-level block the manifest
-   * declares a container but that hydrated as a leaf. An empty (childless)
-   * container comes back from the service without a `blocks` array (omitempty),
-   * and `hydrate` runs before the manifest is available, so it can't classify it
-   * — left unreconciled it would render without its drop zone and can't accept
-   * nested blocks. Containers never nest, so only the top level needs this.
-   */
+  /** Move a top-level block from one index to another, then re-emit. */
   private moveTopLevelBlock(from: number, to: number): void {

and re-attach the container text above reconcileContainers.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts`
around lines 1017 - 1032, Move the existing container-reconciliation JSDoc from
above moveTopLevelBlock to directly above reconcileContainers, leaving
moveTopLevelBlock without that documentation and preserving the text unchanged.

---

431-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Prefer the app's notification/dialog services over window.alert / window.prompt.

Native modals are blocking, unstyled, and inconsistent with the MessageService/ConfirmationService pattern used elsewhere in the newsletter module (see newsletter-manage.component.ts). The link prompt in particular would be better as an inline popover so the contentEditable selection isn't disturbed.

Also applies to: 577-595

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts`
around lines 431 - 446, Replace the native window.alert calls in the newsletter
block composer, including the failure branch around ensureLoaded and the
additional block-removal notification, with the newsletter module’s established
MessageService or dialog notification pattern used by
newsletter-manage.component.ts. Preserve the existing messages and control flow
while avoiding blocking native modals; apply the same service-based approach to
the link prompt range noted in the review, using an inline popover that does not
disrupt the contentEditable selection.

apps/lfx-one/e2e/newsletter-composer-wizard.spec.ts (1)

315-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated draft-GET override into a shared helper.

The same "override the per-newsletter GET route with a modified draft, fallback otherwise" block is repeated three times (container draft, empty-layout draft, legacy draft). Consider a small helper to keep the intent obvious and avoid drift as more scenarios are added.

♻️ Proposed helper extraction

+async function stubDraftGetOverride(page: Page, overrides: Partial<Newsletter> = {}): Promise<void> {
+  await page.route(`**/api/projects/${MOCK_FOUNDATION_UID}/newsletters/${MOCK_NEWSLETTER_ID}`, (route) => {
+    if (route.request().method() === 'GET') {
+      return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(buildDraft(overrides)) });
+    }
+    return route.fallback();
+  });
+}

Then each test body becomes, e.g.:

-    await page.route(`**/api/projects/${MOCK_FOUNDATION_UID}/newsletters/${MOCK_NEWSLETTER_ID}`, (route) => {
-      if (route.request().method() === 'GET') {
-        return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(buildDraft({ body_layout: CONTAINER_DRAFT_LAYOUT })) });
-      }
-      return route.fallback();
-    });
+    await stubDraftGetOverride(page, { body_layout: CONTAINER_DRAFT_LAYOUT });

Also applies to: 373-378, 406-415

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/lfx-one/e2e/newsletter-composer-wizard.spec.ts` around lines 315 - 320,
Extract the repeated newsletter draft GET route override into a shared helper
near the existing test utilities, parameterized by the draft or layout needed
for each scenario. Update the container, empty-layout, and legacy draft cases to
call this helper while preserving route fallback for non-GET requests.

packages/shared/src/utils/url.utils.spec.ts (1)

15-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add the two inputs the renderer can actually hand this gate.

passthrough in apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts (Line 604) passes raw resolved attribute values straight through, and isSafeUrlForAttr tolerates leading whitespace before mailto:. Pinning whitespace-padded and protocol-relative inputs keeps that gate honest.

💚 Suggested extra cases

     expect(isValidUrl('javascript:alert(1)')).toBe(false);
     expect(isValidUrl('JavaScript:alert(1)')).toBe(false); // case-insensitive
+    expect(isValidUrl('  javascript:alert(1)')).toBe(false); // leading whitespace
     expect(isValidUrl('data:text/html,<script>alert(1)</script>')).toBe(false);

     expect(isValidUrl('/relative/path')).toBe(false);
+    expect(isValidUrl('//evil.example.com')).toBe(false); // protocol-relative
     expect(isValidUrl('example.com')).toBe(false); // no protocol

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/shared/src/utils/url.utils.spec.ts` around lines 15 - 33, Add test
cases in the URL validation spec for a whitespace-padded mailto value and a
protocol-relative URL, asserting both are rejected by isValidUrl. Keep the
coverage focused on inputs passed through the newsletter renderer’s
isSafeUrlForAttr gate.

apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts (1)

123-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated editable-field rules between collectEditableFields and selfEditableField.

Lines 133-143 re-implement the exact anchor/each=/richtext exclusion logic that selfEditableField (Lines 493-504) already owns — the comment there even says it "mirrors" these rules. Two copies that must stay in sync will drift the first time a tag is added to TAG_MAP.

♻️ Proposed refactor

     const walk = (nodes: TemplateNode[]): void => {
       for (const node of nodes) {
         if (node.kind !== 'element') continue;
-        // Skip structural / repeated cases — those aren't single-field text.
-        if (node.attrs['each'] === undefined) {
-          if (node.tag === 'richtext') {
-            const field = node.attrs['field'] ?? bindingKeyFromChildren(node.children);
-            if (field) fields.add(field);
-          } else {
-            const field = singleTextField(node);
-            const mapped = TAG_MAP[node.tag]?.tag;
-            // Mirror renderNode's exclusion of anchors (link text owns an href).
-            if (field && mapped !== 'a' && node.tag !== 'a' && node.tag !== 'link' && node.tag !== 'gw-link' && node.tag !== 'button') {
-              fields.add(field);
-            }
-          }
-        }
+        // Single source of truth for the marking rules (richtext, `each=`, anchors).
+        const self = selfEditableField(node);
+        if (self) fields.add(self.field);
         walk(node.children);
       }
     };

Note: selfEditableField returns a richtext field even under each=, so confirm that difference is acceptable (or move the each= guard ahead of the richtext branch there).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts`
around lines 123 - 150, Consolidate editable-field detection in
collectEditableFields by reusing selfEditableField instead of duplicating its
each=, richtext, and anchor-exclusion rules. Preserve traversal of child nodes
and Set collection, and ensure the shared helper’s each= behavior matches the
intended contract—move the guard before richtext handling if richtext fields
must also be excluded under each=.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.html`:
- Around line 376-385: Update the block wrapper divs in the canvas, including
the corresponding instance around the second referenced section, to expose
button semantics with role="button" and tabindex="0". Add keyboard handling so
Enter and Space invoke selectBlock(block.id), while preserving the existing
click selection and aria-pressed state.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.ts`:
- Around line 165-190: Update applyMode’s simple branch so switching from Blocks
retains a persistable empty bodyLayout instead of setting it to null. Seed the
same empty NewsletterLayout shape used when entering Blocks, while continuing to
clear bodyHtml, so the discarded state can pass bodyPersistable and be saved.

In `@apps/lfx-one/src/server/controllers/newsletter.controller.ts`:
- Around line 565-571: The test-send reply-address contract must be consistent
across validation, documentation, and shared types. Keep the mandatory
ed_reply_email validation in testSend, update its JSDoc to state that a valid
reply address is required, and make ed_reply_email required in the shared
newsletter interface while removing the omission note.
- Around line 170-198: Move projectUid extraction and logger.startOperation
calls inside the try blocks of getTemplates and getTemplateManifest so
synchronous validation or logging errors are caught and passed to next(error),
matching the established handling in renderPreview and deleteOptOut.

---

Outside diff comments:
In
`@apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts`:
- Around line 504-510: Close the private formatMissing method with a closing
brace before protected onAddAudienceEmail, preserving its existing formatting
and return behavior so the component parses correctly.

---

Nitpick comments:
In `@apps/lfx-one/e2e/newsletter-composer-wizard.spec.ts`:
- Around line 315-320: Extract the repeated newsletter draft GET route override
into a shared helper near the existing test utilities, parameterized by the
draft or layout needed for each scenario. Update the container, empty-layout,
and legacy draft cases to call this helper while preserving route fallback for
non-GET requests.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts`:
- Around line 1017-1032: Move the existing container-reconciliation JSDoc from
above moveTopLevelBlock to directly above reconcileContainers, leaving
moveTopLevelBlock without that documentation and preserving the text unchanged.
- Around line 431-446: Replace the native window.alert calls in the newsletter
block composer, including the failure branch around ensureLoaded and the
additional block-removal notification, with the newsletter module’s established
MessageService or dialog notification pattern used by
newsletter-manage.component.ts. Preserve the existing messages and control flow
while avoiding blocking native modals; apply the same service-based approach to
the link prompt range noted in the review, using an inline popover that does not
disrupt the contentEditable selection.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.ts`:
- Around line 214-224: Move the “Build the reactive form for a block and wire
its value-change emit” doc comment from above fieldsKey to directly above
buildForm. Keep fieldsKey’s existing schema-signature documentation immediately
above fieldsKey.

In
`@apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts`:
- Around line 123-150: Consolidate editable-field detection in
collectEditableFields by reusing selfEditableField instead of duplicating its
each=, richtext, and anchor-exclusion rules. Preserve traversal of child nodes
and Set collection, and ensure the shared helper’s each= behavior matches the
intended contract—move the guard before richtext handling if richtext fields
must also be excluded under each=.

In `@packages/shared/src/utils/url.utils.spec.ts`:
- Around line 15-33: Add test cases in the URL validation spec for a
whitespace-padded mailto value and a protocol-relative URL, asserting both are
rejected by isValidUrl. Keep the coverage focused on inputs passed through the
newsletter renderer’s isSafeUrlForAttr gate.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2a9a8c03-00fb-44d4-8c21-402d8f0ca933

📥 Commits

Reviewing files that changed from the base of the PR and between 65d9d75 and dd1b923.

📒 Files selected for processing (23)

• apps/lfx-one/e2e/newsletter-composer-wizard.spec.ts
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.html
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.scss
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.html
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-fields/newsletter-block-fields.component.ts
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.html
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.ts
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-review/newsletter-review.component.html
• apps/lfx-one/src/app/modules/newsletters/components/newsletter-review/newsletter-review.component.ts
• apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.html
• apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts
• apps/lfx-one/src/app/modules/newsletters/services/newsletter-renderer.service.ts
• apps/lfx-one/src/app/shared/services/newsletter-manifest.service.ts
• apps/lfx-one/src/app/shared/services/newsletter.service.ts
• apps/lfx-one/src/server/controllers/newsletter.controller.ts
• apps/lfx-one/src/server/routes/newsletters.route.ts
• apps/lfx-one/src/server/services/newsletter-service.client.ts
• apps/lfx-one/src/server/services/newsletter.service.ts
• packages/shared/src/constants/newsletter.constants.ts
• packages/shared/src/interfaces/newsletter.interface.ts
• packages/shared/src/utils/string.utils.ts
• packages/shared/src/utils/url.utils.spec.ts

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

aria-pressed on a plain div isn't exposed, and canvas selection is pointer-only.

These block wrappers carry (click) + aria-pressed but no role/tabindex, so screen readers ignore the pressed state and keyboard users can't select a block from the canvas. Adding role="button" + tabindex="0" (or relying solely on the Outline for keyboard selection and dropping aria-pressed here) would make the semantics honest.

Also applies to: 427-435

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.html`
around lines 376 - 385, Update the block wrapper divs in the canvas, including
the corresponding instance around the second referenced section, to expose
button semantics with role="button" and tabindex="0". Add keyboard handling so
Enter and Space invoke selectBlock(block.id), while preserving the existing
click selection and aria-pressed state.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -C4 'bodyPersistable|hasAnythingToSave' apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts

Repository: linuxfoundation/lfx-self-serve

Length of output: 3089

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the mode-switch logic and save gating around the relevant component.
sed -n '120,230p' apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.ts
printf '\n---\n'
sed -n '230,340p' apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts
printf '\n---\n'
sed -n '1048,1085p' apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts

Repository: linuxfoundation/lfx-self-serve

Length of output: 11019

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C3 'canSaveDraft|saveDraft\(|hasAnythingToSave\(|saveTrigger\$|manual save|Save draft' apps/lfx-one/src/app/modules/newsletters -g '*.ts' -g '*.html'

Repository: linuxfoundation/lfx-self-serve

Length of output: 13067

---

Blocks → Simple discard can’t be persisted Switching to simple sets bodyLayout = null and bodyHtml = '', but canSaveDraft() and autosave both require bodyPersistable(). The cleared state never reaches the server, so the discarded blocks layout comes back on reload. Seed a persistable empty layout here too, or delay clearing until the author makes a new change.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/newsletters/components/newsletter-content-step/newsletter-content-step.component.ts`
around lines 165 - 190, Update applyMode’s simple branch so switching from
Blocks retains a persistable empty bodyLayout instead of setting it to null.
Seed the same empty NewsletterLayout shape used when entering Blocks, while
continuing to clear bodyHtml, so the discarded state can pass bodyPersistable
and be saved.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

New GET handlers throw outside try, the exact hazard renderPreview documents two methods below.

requireProjectUid(req) (and logger.startOperation) run before the try in both getTemplates and getTemplateManifest. On a missing/blank projectUid the synchronous ServiceValidationError rejects the returned promise without reaching next(error) — under Express 4 that leaves the request hanging, which is precisely why renderPreview (Lines 208-211) and deleteOptOut (Lines 351-352) keep these calls inside the try.

🛡️ Proposed fix

   public async getTemplates(req: Request, res: Response, next: NextFunction): Promise<void> {
-    const projectUid = this.requireProjectUid(req);
-    const startTime = logger.startOperation(req, 'newsletter_templates_list', { project_uid: projectUid });
-
     try {
+      const projectUid = this.requireProjectUid(req);
+      const startTime = logger.startOperation(req, 'newsletter_templates_list', { project_uid: projectUid });
       const result = await this.newsletterService.getTemplates(req, projectUid);
       logger.success(req, 'newsletter_templates_list', startTime, { count: result.templates.length });
       res.json(result);
     } catch (error) {
       next(error);
     }
   }
 
   public async getTemplateManifest(req: Request, res: Response, next: NextFunction): Promise<void> {
-    const projectUid = this.requireProjectUid(req);
-    const templateKey = String(req.params['templateKey'] || '').trim();
-    const startTime = logger.startOperation(req, 'newsletter_template_manifest', { project_uid: projectUid, template_key: templateKey });
-
     try {
+      const projectUid = this.requireProjectUid(req);
+      const templateKey = String(req.params['templateKey'] || '').trim();
+      const startTime = logger.startOperation(req, 'newsletter_template_manifest', { project_uid: projectUid, template_key: templateKey });
       const manifest = await this.newsletterService.getTemplateManifest(req, projectUid, templateKey);

📝 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.

Suggested change

	public async getTemplates(req: Request, res: Response, next: NextFunction): Promise<void> {
	const projectUid = this.requireProjectUid(req);
	const startTime = logger.startOperation(req, 'newsletter_templates_list', { project_uid: projectUid });
	try {
	const result = await this.newsletterService.getTemplates(req, projectUid);
	logger.success(req, 'newsletter_templates_list', startTime, { count: result.templates.length });
	res.json(result);
	} catch (error) {
	next(error);
	}
	}
	/**
	* GET /api/projects/:projectUid/newsletters/templates/:templateKey/manifest
	*/
	public async getTemplateManifest(req: Request, res: Response, next: NextFunction): Promise<void> {
	const projectUid = this.requireProjectUid(req);
	const templateKey = String(req.params['templateKey'] || '').trim();
	const startTime = logger.startOperation(req, 'newsletter_template_manifest', { project_uid: projectUid, template_key: templateKey });
	try {
	const manifest = await this.newsletterService.getTemplateManifest(req, projectUid, templateKey);
	logger.success(req, 'newsletter_template_manifest', startTime, { template_key: templateKey, block_count: manifest.blocks.length });
	res.json(manifest);
	} catch (error) {
	next(error);
	}
	}
	public async getTemplates(req: Request, res: Response, next: NextFunction): Promise<void> {
	try {
	const projectUid = this.requireProjectUid(req);
	const startTime = logger.startOperation(req, 'newsletter_templates_list', { project_uid: projectUid });
	const result = await this.newsletterService.getTemplates(req, projectUid);
	logger.success(req, 'newsletter_templates_list', startTime, { count: result.templates.length });
	res.json(result);
	} catch (error) {
	next(error);
	}
	}
	/**
	* GET /api/projects/:projectUid/newsletters/templates/:templateKey/manifest
	*/
	public async getTemplateManifest(req: Request, res: Response, next: NextFunction): Promise<void> {
	try {
	const projectUid = this.requireProjectUid(req);
	const templateKey = String(req.params['templateKey'] || '').trim();
	const startTime = logger.startOperation(req, 'newsletter_template_manifest', { project_uid: projectUid, template_key: templateKey });
	const manifest = await this.newsletterService.getTemplateManifest(req, projectUid, templateKey);
	logger.success(req, 'newsletter_template_manifest', startTime, { template_key: templateKey, block_count: manifest.blocks.length });
	res.json(manifest);
	} catch (error) {
	next(error);
	}
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/lfx-one/src/server/controllers/newsletter.controller.ts` around lines
170 - 198, Move projectUid extraction and logger.startOperation calls inside the
try blocks of getTemplates and getTemplateManifest so synchronous validation or
logging errors are caught and passed to next(error), matching the established
handling in renderPreview and deleteOptOut.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Test-send reply-address contract is inconsistent across three sites. The new validator makes ed_reply_email mandatory while the shared type and the route doc still describe it as omittable; one decision needs to propagate to all of them.

• apps/lfx-one/src/server/controllers/newsletter.controller.ts#L565-L571: keep the requirement (and update the testSend JSDoc at Lines 86-92) or gate it on layoutValid so HTML-only test sends stay accepted without a reply address.
• packages/shared/src/interfaces/newsletter.interface.ts#L92-L95: make ed_reply_email required and drop the "Omitted for a test send that carries no reply address" note if the server requirement stands.

📍 Affects 2 files

• apps/lfx-one/src/server/controllers/newsletter.controller.ts#L565-L571 (this comment)
• packages/shared/src/interfaces/newsletter.interface.ts#L92-L95

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/lfx-one/src/server/controllers/newsletter.controller.ts` around lines
565 - 571, The test-send reply-address contract must be consistent across
validation, documentation, and shared types. Keep the mandatory ed_reply_email
validation in testSend, update its JSDoc to state that a valid reply address is
required, and make ed_reply_email required in the shared newsletter interface
while removing the omission note.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/lfx-one/src/server/controllers/newsletter.controller.ts (1)

570-580: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject a malformed supplied layout before falling back to body_html.

If a request contains body_layout: {} and valid body_html, layoutValid is false and this method accepts the request. This differs from validateCommonPayload, which rejects a present layout without a blocks array. Reject a present invalid layout so the test-send path cannot forward an invalid structured payload upstream.

Proposed fix

-    const layoutValid =
-      payload?.body_layout !== undefined && payload?.body_layout !== null && Array.isArray(payload.body_layout.blocks) && payload.body_layout.blocks.length > 0;
-    if (!layoutValid) {
+    const layoutPresent = payload?.body_layout !== undefined && payload?.body_layout !== null;
+    const layoutValid = layoutPresent && Array.isArray(payload.body_layout.blocks) && payload.body_layout.blocks.length > 0;
+    if (layoutPresent && !layoutValid) {
+      fieldErrors['body_layout'] = 'A block layout must have a non-empty blocks array';
+    } else if (!layoutValid) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/lfx-one/src/server/controllers/newsletter.controller.ts` around lines
570 - 580, Update the validation flow around layoutValid so any supplied
body_layout that is not an object with a blocks array is rejected before
body_html fallback, matching validateCommonPayload behavior. Preserve the
existing body_html validation only when body_layout is absent, and continue
applying the serialized size limit to valid layouts.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@apps/lfx-one/src/server/controllers/newsletter.controller.ts`:
- Around line 570-580: Update the validation flow around layoutValid so any
supplied body_layout that is not an object with a blocks array is rejected
before body_html fallback, matching validateCommonPayload behavior. Preserve the
existing body_html validation only when body_layout is absent, and continue
applying the serialized size limit to valid layouts.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4e8e2547-b1d7-4ec5-96a8-70a3807d9903

📥 Commits

Reviewing files that changed from the base of the PR and between dd1b923 and 6ae82e6.

📒 Files selected for processing (7)

• apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts
• apps/lfx-one/src/app/shared/services/newsletter.service.ts
• apps/lfx-one/src/server/controllers/newsletter.controller.ts
• apps/lfx-one/src/server/services/newsletter-service.client.ts
• apps/lfx-one/src/server/services/newsletter.service.ts
• packages/shared/src/constants/newsletter.constants.ts
• packages/shared/src/interfaces/newsletter.interface.ts

🚧 Files skipped from review as they are similar to previous changes (5)

• apps/lfx-one/src/app/shared/services/newsletter.service.ts
• apps/lfx-one/src/server/services/newsletter-service.client.ts
• apps/lfx-one/src/server/services/newsletter.service.ts
• packages/shared/src/interfaces/newsletter.interface.ts
• apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts

[cursor]

Spurious preview fetch emissions

Medium Severity

initServerHtml feeds toObservable from a computed that returns a fresh { layout, wrapperContent } object on every recompute, with no value-based distinctUntilChanged before switchMap. Angular compares computed results with Object.is, so each upstream notification retriggers the debounced renderPreview request even when the layout payload is unchanged.

 

^{Triggered by learned rule: computed() returning objects causes spurious toObservable() emissions — use primitive keys}

^{Reviewed by Cursor Bugbot for commit 6ae82e6. Configure here.}

[github-actions]

🚀 Deployment Status

Your branch has been deployed to: https://ui-pr-1201.dev.v2.cluster.linuxfound.info

Deployment Details:

• Environment: Development
• Namespace: ui-pr-1201
• ArgoCD App: ui-pr-1201

The deployment will be automatically removed when this PR is closed.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (2)

apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts:281

• Blocks-mode test sends are disabled until the draft has been saved: bodyUsable() requires rendered body_html and a clean snapshot. However, this request sends the current body_layout, and the newsletter service recompiles that layout for the test send. A new block draft cannot autosave before an audience is selected, so users cannot test it even though test-send has no audience requirement. Gate this action on the live subject/layout content instead.

    () => this.subjectFilled() && this.bodyUsable() && this.bodyFilled() && this.hasContext() && this.edEmail().length > 0 && !this.testSending()

apps/lfx-one/src/server/controllers/newsletter.controller.ts:580

• A present but malformed or empty body_layout falls through to the HTML-only branch when body_html is valid, yet the unchanged layout is still forwarded. The upstream contract treats any present body_layout as the sole layout trigger, so this either bypasses the intended non-empty check or produces an upstream render error instead of a local validation response. Reject invalid present layouts; only fall back to body_html when the field is absent/null.

    const layoutValid =
      payload?.body_layout !== undefined && payload?.body_layout !== null && Array.isArray(payload.body_layout.blocks) && payload.body_layout.blocks.length > 0;
    if (!layoutValid) {

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 2f8bc9c. Configure here.}

[cursor]

Stale inline editors after deselection

Medium Severity

The inline-editing effect only adds contenteditable on the newly selected block and never clears it from the previously selected one. Because renderedBlocks does not depend on selection, a pure selection change leaves the old block editable until some later content-driven re-render replaces its DOM, so more than one block can stay inline-editable at once contrary to the selected-only gating.

 

^{Reviewed by Cursor Bugbot for commit 2f8bc9c. Configure here.}

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.

Suppressed comments (3)

apps/lfx-one/src/server/controllers/newsletter.controller.ts:210

• The project UID extraction and logger setup are outside the try, so either synchronous throw bypasses next(error) under Express 4 and can leave the request unresolved. Keep the whole handler setup in the guarded block, matching renderPreview.

    const projectUid = this.requireProjectUid(req);
    const templateKey = String(req.params['templateKey'] || '').trim();
    const startTime = logger.startOperation(req, 'newsletter_template_manifest', { project_uid: projectUid, template_key: templateKey });

apps/lfx-one/src/server/controllers/newsletter.controller.ts:592

• This unconditional check contradicts the new shared contract, which makes ed_reply_email optional and explicitly permits omission when there is no reply address; the upstream test-send DTO is optional too. As written, valid clients following that contract receive a BFF 400. Validate the address only when it is supplied (or make it required consistently across the interface and upstream contract).

    // Parity with validateCommonPayload: the client sends ed_reply_email on every
    // test send, and a layout test recompiles the wrapper's "To reply, email …"
    // row (and the email's reply-to) from it, so require a valid address here too
    // rather than silently accepting a missing/malformed one.
    if (!payload?.ed_reply_email || typeof payload.ed_reply_email !== 'string' || !payload.ed_reply_email.includes('@')) {
      fieldErrors['ed_reply_email'] = 'A valid ed_reply_email is required';
    }

apps/lfx-one/src/app/modules/newsletters/components/newsletter-block-composer/newsletter-block-composer.component.ts:1353

• The form/test-send path carries a reply address, but preview wrapper content never supplies edition.reply_email. Both restored upstream wrappers guard their “To reply, email …” row on that value, so the canvas and authoritative server preview omit content that appears in real/test sends; the reported size/source is therefore understated. Pass the current reply address into the composer and include it here.

    return {
      edition: {
        date: 'Newsletter preview',
        view_online_link: '',
        unsubscribe_url: '#',
        manage_subscriptions_url: '#',
      },