feat(campaigns): address a brief by delivery type and stage by mrautela365 · Pull Request #2189 · linuxfoundation/lfx-self-serve

mrautela365 · 2026-09-04T04:07:03Z

What this changes

An event does not have "a brief". It has a paid brief and an email series, and the series has one brief per stage. This is the UI half of that model.

Before: the planner asked campaign-service for "the brief for this event". With a paid brief and two email stages in storage, that question no longer names one thing — and the answer came back as whichever row the narrow key happened to match.

Now the planner answers which send before it looks anything up. The email type selector moves above the planner, its stage joins the lookup key, and switching type re-issues the lookup rather than filtering a result — a different send is a different brief.

⚠️ Deploy order — this PR goes second

Depends on lfx-v2-campaign-service#203 landing and deploying first.

That PR adds migration 000030 (the delivery_type + stage columns and the widened unique index) and the delivery/stage-scoped repository. Without it the new query parameters are silently ignored upstream and every lookup answers with whatever brief the narrow key matches — the exact bug this PR exists to fix. Shipping this half first is a no-op at best and a wrong-brief-restored at worst.

Commits

Commit	What
`25e385e6b`	scope brief restore to its delivery type
`a1847f76e`	refuse a write that replaces the other surface's brief
`aeae7a958`	enforce the extraction cap and surface the conflict
`16000eea5`	address a brief by delivery type and stage
`728995c14`	forward the email stage on a brief lookup
`bb44690b2`	choose the email send before looking its brief up

Two of these correct earlier commits on this same branch, and the messages say so rather than hiding it:

An earlier revision smuggled deliveryType into the free-form targeting blob to dodge a migration, then added a write guard refusing any save whose surface differed from the stored row's. That guard enforced exactly the constraint the product does not have — it made an event's second channel permanently unsavable and no stage past the first reachable. Both are gone; the storage key does that job now.
The read-side surface check stays as defence in depth. It is no longer what keeps the surfaces apart, but a paid brief opened on the email planner carries RSA headlines and a keyword list, and a stale upstream mid-rollout is exactly when a mismatched row would arrive.

The bug only a live stack found

campaign.controller.ts read delivery_type from the query and not stage, so every lookup asked for the empty stage — which is the paid brief's stage. An email caller naming a real stage was answered "no brief" for a row sitting in the database.

Both halves either side were correct: the client sent the stage, the service keyed on it. Only the controller dropped it in between, which is why every unit test passed throughout. It took driving a browser against a live service and a seeded database to see.

The stage is now validated against CAMPAIGN_EMAIL_STAGES rather than forwarded — an unrecognised value addresses the paid slot instead of a brief nobody meant, the same narrowing delivery_type already gets.

Also in here

extractableHtml() cap was not enforced. A JSON-LD fix let the extracted string exceed the 60k character cap — measured at 500,055 characters on a real page. Now a 20k JSON-LD budget plus a 40k remainder, verified empirically.
CAMPAIGN_EMAIL_STAGES moves to shared constants with CampaignEmailStage derived from it, so the runtime list that validates a wire value and the type that describes it cannot drift.
Three wrapper templates (input-text, autocomplete, input-number) got [placeholder]="placeholder() ?? ''" — the templates passed string | undefined into a string input.
normalizeEventDetails() replaces an unchecked cast in the planner.

Verification

Browser-driven against a live campaign-service and a seeded Postgres, on tlf/kubecon-japan:

Check	Result
Paid lookup	returns the paid brief, unchanged
`email/CFP Launch`	returns its own brief
`email/Registration Push`	returns its own brief — three distinct requests, one per stage
`email/Post-Event` with nothing stored	correctly offers nothing
Restore → edit → save	updates in place; 4 briefs before, 4 after
New stage creation	404 → 201 → 200

Mutation-verified: reverting the stage forwarding fails seven tests; re-sending the empty stage fails the new planner test.

Not covered here: Build Audience / Stage Draft against a real HubSpot connection — the UAT database has no connection row for tlf, so that path shows "HubSpot is not connected" and was not exercised.

Gates

2,471 server tests · 1,743 app tests · 0 lint errors · build clean · no dev bypasses.

Refs LFXV2-3198

Briefs are keyed `(project_id, event_slug)` with no delivery dimension, so one event holds one row however many surfaces plan it. The email restore path was therefore disabled outright — restoring under Email could hand back a paid brief's RSA headlines, keyword list and platform selection. Record which surface authored a brief and scope the read to it. The row now carries `deliveryType` in `targeting` (free-form JSONB, so no migration and no change to campaign-service's brief contract), and `loadBrief` reports a brief from the other surface as `none` rather than returning it. That makes the gate unnecessary, so Email can restore its own briefs. Absence reads as paid: every row written before this came from the paid surface, the only one whose restore was ever enabled, so paid callers keep restoring exactly as before. Also in this change, each found while testing the above end to end: - Extraction was handed `html.slice(0, 30_000)`. On the Open Source Summit Japan page that window is 62KB of `<style>` and 38KB of `<script>`; the venue string sits at byte 30,351 — 351 past the cut — and no date survived at all. The model reported the facts absent and downstream copy invented replacements. Strip script/style/svg/comments first: the same page drops to 44,829 bytes with the venue at byte 1,396. - `event.data as CampaignEventDetails` asserted a shape instead of building one, so a scrape that omitted a field put the string "undefined" into the page and into the edit form. Normalise at the boundary so the declared non-optional type is true for every reader. - `input<string>()` with no default renders the literal text "undefined" as a placeholder. Three wrappers lacked a default; the other seven that bind `[placeholder]` already pass one and are untouched. - The template picker rendered up to 100 items inline, pushing the audience and staging controls past 7000px on an 8000px page, where wheel-scrolling stalled inside the list. Cap and scroll it internally. - The HubSpot draft id was on the platform result and discarded; the success message now names it. Shown rather than linked: a deep link needs the portal id, which the connection row does not reliably carry. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Review of the delivery-scoped read found the write side left open, and scoping only the read made the hazard worse rather than better. Ownership is keyed `(project, event)` with no delivery component, so a session that restored the PAID brief still holds a valid id for it after switching to Email: the ownership guard passes and email content replaces the paid row. Before delivery scoping the paid brief at least stayed readable; now the replaced row records `deliveryType: 'email'`, so the paid surface's own read answers `none` and the loss is silent. `saveBrief` now compares the stored brief's delivery type against the incoming one and refuses when they differ, under its own conflict token — `unowned-brief-exists` is wrong here because the caller does own the row, and its message tells the user to reload and re-enter the URL, advice that loops forever when the brief belongs to a surface they are not on. Also from review: - `extractableHtml` stripped every `<script>`, including `application/ld+json`, which is where event pages most reliably publish startDate/endDate/location as schema.org data — the exact fields the extraction prompt asks for. It is now preserved, and prepended so a long page cannot push it past the cap. - The comment claimed `fetchSafeUrl` bounds what can be downloaded. It does not: it accumulates chunks with only a 15s timeout, so this cap is the only size bound in the path. Corrected rather than left asserting a limit that is not there. - Six frontend tests pinned behaviour this change reverses, and were failing. `does not look up a saved brief in email mode` is inverted to assert the lookup happens AND carries `'email'`; five sibling assertions gain the delivery argument, which also pins the paid default. - `extractableHtml` and `normalizeEventDetails` had no coverage. Eight tests added, each verified by mutation: reverting the fix fails the test. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Reviewer simulation found two defects in the previous commit's fixes, both of the same shape: a fix that opened the next hole. The 60k extraction cap stopped holding. Preserving JSON-LD composed the result as `jsonLd + stripped.slice(0, 60_000)`, which bounds only the second term — a page with a 500KB `ld+json` block produced 500,055 characters against a documented cap of 60,000. JSON-LD is the more attacker-controllable half: machine-written, invisible on the rendered page, and nothing upstream bounds it either, since `fetchSafeUrl` has a 15s timeout and no byte ceiling. The budget now splits between the two parts so the ceiling is real, and JSON-LD still gets the space it needs for the structured facts. The existing cap test could not have caught it — it feeds only prose, which exercises the one path that was already bounded. `other-delivery-type-brief-exists` was unreachable from the surface that triggers it. `persistEmailBrief` reports failure as an empty brief id and discards `conflict`, so all three email actions rendered "The brief could not be saved… Try again." for a refusal retrying can never clear — the same dead end the `unowned-brief-exists` copy was reworded to stop promising, one surface over. And email is where it fires: an event already planned on paid trips the guard on the first email action. The write guard also read `deliveryType` through `fromBriefResponse`, which returns null for reasons unrelated to delivery type — an unparseable row was laundered into "paid" and replaceable. It now reads the one validated string from `targeting` directly. Refusing on null instead was worse: it broke sixteen ordinary saves whose stored row simply had no `event_details`. Also: the paid event strip kept the unguarded interpolation the event card just lost, so a fact the scrape could not find rendered an icon beside nothing. The fix's own comment predicted this — "a per-site fix is one grep away from missing the next one". Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Paid and email are parallel channels on one event, and an email campaign is a series rather than a document — a CFP Launch and a Final Countdown for the same event are both live, not two versions of one brief. An earlier revision of this branch smuggled `deliveryType` into the free-form `targeting` blob to avoid a migration, then added a write guard refusing any save whose surface differed from the stored row's. That guard enforced exactly the constraint the product does not have: it made an event's second channel permanently unsavable, and no stage of an email series reachable past the first. Both are gone. The delivery type and stage are now sent as top-level wire fields, matching campaign-service's columns, and the find sends all four parts of the key. That is what makes a series addressable: a lookup naming only the slug matches an arbitrary member of the event's brief set. The read-side surface check stays as defence in depth. It is no longer what keeps the surfaces apart — the storage key is — but a paid brief opened on the email planner carries RSA headlines and a keyword list, which is bad enough to be worth one comparison, and a stale upstream mid-rollout is exactly when a mismatched row would arrive. `CAMPAIGN_EMAIL_STAGES` moves to the shared constants and `CampaignEmailStage` is derived from it, so the runtime list that validates a wire value and the type that describes it cannot drift. Requires the campaign-service half of LFXV2-3198 (migration 000030 and the delivery/stage-scoped repository) to be deployed first: without it the new query parameters are ignored and every lookup answers with whatever brief the event happens to have. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

The controller read `delivery_type` from the query and not `stage`, so every lookup asked campaign-service for the empty stage — which is the PAID brief's stage. An email caller naming a real stage was answered `none` for a brief sitting in the database, and no send in a series past the first was reachable. Both halves either side of it were correct, which is why nothing caught this: the client sent the stage and the service keyed on it. Only the controller dropped it in between. It took driving a browser against a live service and a seeded database to see; every unit test passed throughout. The stage is validated against `CAMPAIGN_EMAIL_STAGES` rather than forwarded, so an unrecognised value addresses the paid slot instead of a brief nobody meant — the same narrowing the delivery type already gets. Verified end to end through the full stack: with three briefs stored for one event, a paid lookup returns PAID-PLAN, `email/CFP Launch` returns CFP-COPY, and `email/Registration Push` returns REG-COPY. Mutation-verified: dropping the stage again fails seven tests. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

The planner could not reach an email series. It asked for the empty stage — the paid brief's stage — so an event with a CFP Launch and a Registration Push in storage was answered "no brief", and no send past the first was reachable from the UI. The stage was already derived from the email type; it was simply chosen on the Implement tab, after a brief exists. But the stage is part of a brief's identity upstream, so it has to be answered BEFORE the lookup: "the brief for this event" no longer names one thing. The type selector therefore moves above the planner, and its stage joins the lookup key — the combineLatest, the distinctUntilChanged, the staleness guard and the emitted brief. Switching type re-asks rather than filtering, because a different send is a different brief. Verified in the browser against a live service and a seeded database: selecting Registration Push offers its stored brief, switching to CFP Launch issues its own lookup and offers that one, and Post-Event — which has no stored brief — correctly offers nothing. Three distinct requests, one per stage. Mutation-verified: sending the empty stage again fails the new test. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

coderabbitai · 2026-09-04T04:07:09Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Essentials

Run ID: 846b0548-87c0-4c3e-b457-858a2466c425

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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

cursor · 2026-09-04T04:07:10Z

PR Summary

High Risk
Changes brief lookup, restore, persist, and ownership across paid and multi-stage email flows plus BFF validation; wrong deploy order or race handling could restore or save against the wrong brief row.

Overview
This PR aligns the campaigns UI and BFF with per-send brief identity: a paid brief plus one email brief per stage, keyed on (project, event_slug, delivery_type, stage) instead of a single row per event.

Planning and email flow: The email type control moves above the email planner so stage is chosen before loadBrief / restore; Implement shows the type read-only to avoid desync between stage, cached emailBriefId, and copy generation. The planner passes deliveryType, emailStage, and restoreSavedEmailBrief; saved-brief lookup runs for email (scoped to that surface and stage) and re-runs when stage or delivery changes, clearing stale restore offers and in-progress drafts. Brief ownership in the parent uses the widened four-part ownershipKey; stage changes drop cached id/output (and cancel staging polls) only when the stage actually changes, not when two types share a stage.

Email persist and errors: Email saves surface named conflicts (matching paid), stage overwrite promotion only after the warning renders, and email-specific “re-select this email type” copy outside shared paid banners. Restoring a saved email brief records ownership, adopts program type, and caches id only when approved.

BFF: loadBrief validates and forwards delivery_type and stage, rejects impossible pairs (e.g. email without stage, paid with a series stage), malformed/repeated query params, and unknown stages instead of silently querying another brief.

Smaller fixes: normalizeEventDetails() and guarded event-detail chips avoid "undefined" in the UI; HubSpot staging success includes draft id; email template list is scroll-capped; shared inputs coalesce optional placeholders; fetchSafeUrl enforces a 5 MiB response ceiling with an integration-style spec.

Deploy: Requires campaign-service #203 (migration + widened key) deployed first, or lookups still behave like the old single-key model.

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

Copilot

Pull request overview

Aligns campaign planning with campaign-service’s delivery-type-and-stage brief identity.

Changes:

Adds delivery/stage-scoped brief persistence and restoration.
Improves page extraction limits and event-data normalization.
Fixes shared input placeholders and campaign UI feedback.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`packages/shared/src/interfaces/campaign.interface.ts`	Adds brief delivery and stage fields.
`packages/shared/src/constants/campaign.constants.ts`	Defines valid email stages.
`apps/lfx-one/src/server/services/campaign-service.service.ts`	Sends full brief identity upstream.
`apps/lfx-one/src/server/services/campaign-service.service.spec.ts`	Tests scoped persistence and lookup.
`apps/lfx-one/src/server/services/campaign-proxy.service.ts`	Improves extraction preprocessing and caps.
`apps/lfx-one/src/server/services/campaign-proxy.service.spec.ts`	Tests extraction behavior and limits.
`apps/lfx-one/src/server/controllers/campaign.controller.ts`	Parses delivery and stage query parameters.
`apps/lfx-one/src/server/controllers/campaign.controller.spec.ts`	Tests query forwarding and defaults.
`apps/lfx-one/src/app/shared/services/campaign.service.ts`	Sends brief identity from Angular.
`apps/lfx-one/src/app/shared/components/input-text/input-text.component.html`	Normalizes absent placeholders.
`apps/lfx-one/src/app/shared/components/input-number/input-number.component.html`	Normalizes absent placeholders.
`apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.html`	Normalizes absent placeholders.
`apps/lfx-one/src/app/modules/dashboards/campaigns/components/planning-tab/planning-tab.component.ts`	Adds stage-aware lookup and normalization.
`apps/lfx-one/src/app/modules/dashboards/campaigns/components/planning-tab/planning-tab.component.spec.ts`	Tests stage switching and payload normalization.
`apps/lfx-one/src/app/modules/dashboards/campaigns/components/planning-tab/planning-tab.component.html`	Hides absent extracted facts.
`apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.ts`	Adds email restore and improved status handling.
`apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html`	Moves stage selection into planning and bounds template lists.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

CodeQL flagged `extractableHtml`'s script stripper as a bad HTML filtering regexp: it does not match `</script >`. HTML permits whitespace between a closing tag's name and its `>`, and browsers honour it, so a regex anchored on `</script>` matches neither the tag nor the element and leaves the WHOLE thing in the output -- body included. All four patterns here had it, not only the one reported. Verified by probe: `</script >`, `</script\t>`, `</script\n>`, `</style >` and `</svg >` each leaked their body through. That is not cosmetic. What survives is script and style bodies from an untrusted fetched page, and this string becomes an LLM extraction prompt -- attacker-controllable text reaching the model under the guise of page content. The JSON-LD matcher fails the other way: a block closed `</script >` goes unmatched there and is then removed as an ordinary script, so the structured event facts the extraction depends on are silently lost rather than leaked. Five tests, each mutation-verified against its OWN regex rather than in bulk. That mattered: reverting all four at once made the JSON-LD test pass, because the unmatched block then survived stripping and the date leaked through by accident -- two defects cancelling. Reverted alone, it fails as it should. The comment says plainly that this is a heuristic strip on untrusted input and not a sanitiser: the output is never rendered as HTML, only read as prompt text, so a residual edge case degrades extraction quality rather than crossing a trust boundary. Verified: 2476 server tests, 1743 app tests, 0 lint errors, types checked with the turbo cache forced off, build clean. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

Suppressed comments (8)

Previously missed (3) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.ts:207

With the widened key, a brief on another delivery type no longer causes unowned-brief-exists; that conflict can only come from the same delivery type and stage. The added clause therefore gives users an impossible diagnosis and can send them away from the stage they actually need to reopen.
apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:187
This user-facing claim does not match the model: CAMPAIGN_EMAIL_TYPES has 12 types mapped onto six stages, and types sharing a stage address the same brief. Saying every type is its own brief promises separate storage that switching between, for example, Registration Launch and Main Registration Push does not provide.
packages/shared/src/interfaces/campaign.interface.ts:221
This public interface documentation describes the superseded implementation: the field is now a top-level delivery_type column and the upstream key includes both delivery type and stage. Keeping the old targeting/single-row explanation directly contradicts the emailStage contract below and can lead future callers to omit part of the key.

apps/lfx-one/src/server/services/campaign-service.service.ts:619

Although the normal find now uses the full key, ambiguous-write recovery still issues its GET with only event_slug (reconcileLostWrite at lines 1446–1448). In campaign-service#203, omitted dimensions default to paid-marketing/empty, so an email POST or PUT that commits but loses its response cannot recover its own row and may inspect the paid row instead. Pass envelope.brief.delivery_type and stage on every recovery read, and include them in storedBriefMatches.

    const existing = await this.findBrief(req, basePath, eventSlug, brief.deliveryType ?? 'paid-marketing', brief.emailStage ?? '');

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.ts:1787

ownershipKey() still contains only project and event (lines 3591–3608). This restore therefore stores paid, email, and every email stage in the same knownBriefIds slot; after stage B replaces stage A's cached ID, returning to stage A sends B's ID and is refused as unowned-brief-exists. Extend the ownership and epoch key to include delivery type and normalized stage.

    const key = this.ownershipKey(this.activeFoundationSlug(), brief);

apps/lfx-one/src/app/modules/dashboards/campaigns/components/planning-tab/planning-tab.component.ts:435

This eager invalidation watches only the delivery type. Since emailStage is now also part of the lookup key and the replacement lookup is debounced, changing stages leaves the previous stage's Restore offer clickable until the new response arrives. Clear the saved-brief state when either identity input changes.

    this.deliveryType$.pipe(skip(1), takeUntilDestroyed(this.destroyRef)).subscribe(() => {
      this.savedBrief.set(null);

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:175

This adds a selector above Plan but leaves the existing selector in the Implementation view. Changing that copy to a type mapped to another stage updates selectedEmailStage, while onSelectEmailType leaves emailBriefOutput.emailStage and emailBriefId pointing at the previous stage; copy generation and staging then run under the wrong brief. Remove the Implementation selector or reset and load the new stage before allowing those actions.

          <div class="flex flex-col gap-2" data-testid="campaigns-email-stage-picker">
            <label for="email-stage" class="font-mono text-xs uppercase tracking-wide text-gray-400">Email type</label>
            <lfx-select

apps/lfx-one/src/server/controllers/campaign.controller.ts:781

These coercions turn malformed supplied query values into another valid brief address. For example, delivery_type=email&stage=Not A Stage becomes email/'', and repeated parameters collapse to defaults; campaign-service#203 instead declares both fields as enums. Default only omitted parameters and return a validation error for any supplied invalid or non-string value.

    const stageParam = typeof req.query['stage'] === 'string' ? req.query['stage'] : '';
    const stage = (CAMPAIGN_EMAIL_STAGES as readonly string[]).includes(stageParam) ? stageParam : '';
    const deliveryType: CampaignDeliveryType = deliveryTypeParam === 'email' ? 'email' : 'paid-marketing';

CodeQL reported the same rule a second time, against the fix for the first: `</script\t\n bar>`. An HTML end tag is `</` name then ANYTHING up to `>` -- browsers skip whatever sits between -- so `\s*>` closed only the whitespace cases and still left the whole element in place for the rest. All four patterns now use `<\/tag(\s[^>]*)?>`. The leading `\s` is load bearing: without it `</scriptx>` would match, and since that names a different tag and closes nothing, the strip would end at the wrong place and silently delete page content up to the next real close. Two new cases and one negative test, each mutation-verified. The negative test needed a second pass -- its first version asserted only that the script body was gone, which the `\s`-less pattern also achieves by stopping at the decoy. It now asserts on text that sits AFTER the decoy and still inside the element, which is what actually separates the two patterns. Verified: 2479 server tests, 1743 app tests, 0 lint errors, types checked with the turbo cache forced off, build clean. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

Suppressed comments (7)

Previously missed (2) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:195

The restore event already carries approved, but this binding drops it and onRestoreSavedEmailBrief caches the ID unconditionally. That bypasses the fresh-save invariant at lines 3216-3221, which deliberately refuses to cache unapproved IDs because audience building and campaign creation reject them. An unapproved restored email brief therefore exposes actions that can only fail. Propagate the approval flag and retain/gate it for the email flow as the paid restore path does.
apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:187
This copy promises one brief per email type, but CAMPAIGN_EMAIL_TYPES has 12 types mapped onto only six stage keys (for example, Registration Launch and Main Registration Push both use Registration Push). Those types intentionally share one stored brief and switching between them does not trigger a new lookup, so the UI is misleading. Describe the per-stage behavior instead.

apps/lfx-one/src/server/services/campaign-service.service.ts:619

The normal lookup now uses the full identity, but the ambiguous-write recovery path still GETs with only event_slug (reconcileLostWrite, lines 1446-1448). Upstream defaults omitted dimensions to paid-marketing/empty stage, so an email POST or PUT that commits but loses its response is reconciled against the paid row instead. Recovery then fails, the client never learns the committed email brief ID, and a retry is refused as unowned-brief-exists. Thread envelope.brief.delivery_type and stage into every reconciliation GET as well.

    // Keyed on the brief's OWN identity, so a save looks for the row it is about to replace rather
    // than for whatever brief this event happens to have. Without the delivery type an email save
    // would find the paid brief, decide it already owns the event, and PUT over it.
    const existing = await this.findBrief(req, basePath, eventSlug, brief.deliveryType ?? 'paid-marketing', brief.emailStage ?? '');

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.ts:1788

This restore records ownership through ownershipKey, but that helper still keys only (project, eventSlug) (lines 3591-3608). Brief identity now also includes delivery type and stage, so restoring or saving one sibling overwrites another sibling's cached ID; returning to the first brief then submits the wrong ID and is refused as unowned-brief-exists. Widen the ownership and epoch keys to include the normalized delivery type and stage, and update every lookup/write of those maps.

  protected onRestoreSavedEmailBrief(brief: CampaignBriefOutput, briefId: string, etag: string | null | undefined): void {
    const key = this.ownershipKey(this.activeFoundationSlug(), brief);
    if (key !== null) {

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:193

Changing this stage only re-queries the child planner. The parent’s onSelectEmailType leaves emailBriefOutput, emailBriefId, and the built audience from the previous stage intact, so after implementing CFP Launch a user can return to Plan, select Final Countdown, reopen Implement, and generate/stage against the CFP brief ID. Reset stage-scoped parent state whenever the derived stage actually changes; same-stage type aliases can retain it.

            [emailStage]="selectedEmailStage() ?? ''"

apps/lfx-one/src/app/modules/dashboards/campaigns/components/planning-tab/planning-tab.component.ts:403

The response-key staleness guard drops a late result, but an emailStage change never eagerly clears the already-rendered restore offer. During the 500 ms debounce, the button still holds the previous stage’s brief and can emit it under the newly selected stage. Add the same immediate saved-brief/id/ETag clear used for delivery changes when emailStage$ changes.

      .subscribe(({ slug, project, delivery, stage, result }) => {
        // `delivery` joins the staleness check for the same reason `slug` and `project` are in it:
        // `switchMap` cancels the previous REQUEST, but a response already in flight when the user
        // flips Paid <-> Email still arrives, and it answers a question about the other surface.
        // Applying it would put a paid brief's Restore offer on the email planner — the exact
        // outcome the delivery scoping exists to prevent, reintroduced as a race.
        if (slug !== this.currentSlug || project !== this.activeFoundationSlug() || delivery !== this.deliveryType() || stage !== this.emailStage()) {

apps/lfx-one/src/server/controllers/campaign.controller.ts:781

An unknown stage is converted to '' while deliveryType remains email, so this does not address the paid slot; it addresses the distinct (email, '') key. The write path can create that key because emailStage is optional, making a typo resolve a different brief rather than fail as the shared constant documents. Reject non-empty unknown stages with a 400 instead of changing the lookup identity.

    const stageParam = typeof req.query['stage'] === 'string' ? req.query['stage'] : '';
    const stage = (CAMPAIGN_EMAIL_STAGES as readonly string[]).includes(stageParam) ? stageParam : '';
    const deliveryType: CampaignDeliveryType = deliveryTypeParam === 'email' ? 'email' : 'paid-marketing';

The lookup took all four parts of the key; the state around it did not. Five places still assumed one brief per event, each found by review. The ownership map keyed on (project, event). Since campaign-service keys a brief on (project, event_slug, delivery_type, stage), an event's paid brief and every stage of its email series shared one entry -- so recording one overwrote the cached id and ETag of its siblings, and the next save of one of THOSE sent the wrong knownBriefId and was refused as `unowned-brief-exists`. The key now carries all four, read off the BRIEF rather than the current selection, since the brief in hand is not always the one on screen. Lost-write reconciliation recovered with only `event_slug`, so upstream applied its defaults and a timed-out EMAIL write reconciled against the PAID brief: either a 404 that rethrew, or a 200 for a row this request never wrote whose version could satisfy the version check and hand back another brief's id. The first attempt at this fix read `envelope.brief.deliveryType`; the wire type is snake_case, so both fields were undefined and it silently changed nothing. The new test is what caught it -- it asserts on the recovery GET's query rather than on a read having happened. Two email-type selectors were bound to the same form control, but only the planner's lookup follows a change -- so switching type on Implement moved the stage while emailBriefId still named the previous stage's brief, and generation sent the new stage against the old brief's id. Implement now displays the type; the choice lives above the planner, where the lookup can answer it. A stage change did not clear the restore offer -- only a delivery-type change did -- so the previous stage's Restore stayed clickable through the debounce window, long enough to restore the wrong send. The controller folded an unrecognised stage to `''` and a comment claimed that addressed the paid slot. It did not: delivery_type stayed `email`, so the lookup addressed `(email, '')`, a real and different key, and the caller got a confident answer about a brief nobody asked for. Now rejected with a 400. Unlike delivery_type, stages are siblings with no narrower fallback. The test pinning the old behaviour is deleted rather than left contradicting the new one. Two stale doc comments corrected: `deliveryType` no longer lives in the free-form `targeting` blob, and briefs are no longer keyed on (project, event). Every fix mutation-verified, including the camelCase slip above. Verified: 2483 server tests, 1745 app tests, 0 lint errors, types checked with the turbo cache forced off, build clean. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.ts:207

The new fallback advice is no longer possible under this PR’s full-key save. unowned-brief-exists comes from finding an existing brief with the same delivery type and stage, so a brief on the other delivery type cannot cause this refusal. Telling the user that an empty reload means “the other delivery type” sends them away from the exact stage they need to reopen.

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:178

Changing this selector to a different stage does not invalidate the parent’s existing emailBriefOutput or emailBriefId; onSelectEmailType only clears generated copy. After returning to Planning, selecting another stage, and clicking Implement directly, ensureEmailBriefId reuses the previous stage’s ID while generateEmailCopy sends the newly selected stage, recreating the cross-stage mismatch this PR is intended to prevent. Clear the stage-scoped brief/id state when the resolved stage changes (while preserving it for type aliases that map to the same stage), and add a regression test for Planning → stage change → Implement.

            <lfx-select
              [form]="selectorForm"
              control="emailType"
              id="email-stage"

apps/lfx-one/src/server/controllers/campaign.controller.ts:792

Stage is validated independently of delivery type, so email with an empty stage and paid-marketing with a named email stage both pass and address noncanonical keys. The persist path likewise serializes these combinations, allowing authenticated callers to create slots outside the model (“empty for paid; one named stage per email brief”). Validate the pair on both read and write boundaries: paid requires '', while email requires one of CAMPAIGN_EMAIL_STAGES.

    const stageParam = typeof req.query['stage'] === 'string' ? req.query['stage'] : '';
    if (stageParam !== '' && !(CAMPAIGN_EMAIL_STAGES as readonly string[]).includes(stageParam)) {
      next(

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.html:305

This repeats the incorrect claim that each type has a distinct brief. Several types intentionally share a stage, and therefore share the same persisted brief identity.

                  <p class="text-xs text-gray-500">Change the type above the planner — each type is its own brief.</p>

The desync this branch claimed to fix was still reachable through the other door. Moving the email-type picker above the planner re-pointed the PLANNER's lookup, but `emailBriefId` is the parent's own cached state and survived the change -- so `onSelectEmailType` moved the stage while generate and stage kept addressing the previous stage's row under the new stage's label. Staging is the one that writes: it clones a HubSpot draft against that brief's audience. Two reviewers found it independently and each proved it by probe. My own test reproduced it before the fix. Cleared on the STAGE moving, not on any type change: twelve types collapse onto six stages, so switching between two types that share one (CFP Launch has three) addresses the same brief and must not discard it. Only the brief identity is cleared, deliberately NOT `resetEmailBriefDerivedState()` -- that also drops the template selection, which is the operator's own choice and which four existing specs protect. One existing test asserted the bug: it cached a brief id, switched to a different stage, and expected generation against that same id. Its real intent -- stage, not type id -- is preserved by choosing the type before caching. Also from this round: - `id`/`data-testid` on the relocated picker were not the wrapper's inputs. `lfx-select` forwards `[id]` to the host container and `[inputId]` to the focusable element, so the `<label for>` pointed at something unfocusable and clicking it stopped focusing the control; `data-testid` landed on the host, so the spec passed against the wrong element. Now `inputId`/`dataTest`, and the spec queries `data-test` -- the trap its own sibling test documents. - `emailSaveFailureMessage` discarded its `consequence` on the conflict branch, so all three callers rendered one sentence. For staging that omitted the part the operator most needs: that nothing was staged. - Six comments asserted the pre-000030 key as present-tense fact, including one calling this very PR's widening "LFXV2-3198's remaining half", one still claiming `deliveryType` lives in the `targeting` blob, one naming the dropped index, and the user-facing conflict string blaming the other delivery type -- which can no longer be the cause, since the surfaces are separate rows now. - `fromBriefResponse` now guards `stage` with `typeof` first, matching the line above it, rather than relying on `Array.includes` rejecting `undefined`. - An orphaned docblock and a doubled comment. Verified: 2483 server tests, 1747 app tests, app type-checked directly with tsc (yarn check-types covers only the shared package), 0 lint errors, build clean. The fix and its stage gate are both mutation-verified: removing the reset fails the desync test, and resetting on every type change fails the same-stage test. Refs LFXV2-3198 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

mrautela365 · 2026-09-05T03:22:27Z

Confirmed and fixed in 58b6db4b6. Reproduced: <script type=application/ld+json> was stripped as an ordinary script, so the block's structured data never reached the prompt.

The pattern now accepts all three forms, with the unquoted one bounded by whitespace or >:

unquoted            : collected
double-quoted       : collected
single-quoted       : collected
type=text/javascript: not collected
type= inside another attribute value: not collected

The last two matter as much as the first: widening this is exactly where a fix can quietly start collecting the wrong scripts, so both negative cases are pinned as tests alongside the three positive ones. All five verified against the quoted-only pattern.

Copilot

Pull request overview

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

dealako · 2026-09-05T03:41:51Z

Hi @mrautela365 — thank you for the fast round-10 follow-up. Two commits (e1272f055, 58b6db4b6) land the quote-aware tag scan, forward inert cursor, unquoted JSON-LD types, and foreign-content self-closing fixes.

👏 Nice work:

inertCursor + advanceInert replace the backward lastIndexOf('<!--') quadratic path in svgBlockEnd (e1272f055, :1057-1091)
startTagEnd quote-aware scanning fixes > inside attribute values and long-root <svg> recognition (e1272f055, :741-759)
Unquoted type=application/ld+json collected without widening to text/javascript (58b6db4b6, :712, spec :1366-1390)
Foreign-content <style/>/<script/> inside <svg> no longer report the graphic unclosed (58b6db4b6, :1025-1035, spec :1395-1402)

Revision tracking (round 9 → round 10):

✅ Resolved: svgBlockEnd backward comment search — forward inertCursor + bounded advanceInert (e1272f055)
✅ Resolved: Linearity guard for svgBlockEnd at scale — sibling + deep-nesting tests added (e1272f055, spec :1345-1361)

Structured issue count:

🔴 Blocking: 2 issues — advanceInert re-scans its window per inert region (quadratic on icon/comment-dense svg); isJsonLdTag re-runs JSON_LD_ATTR_RE over the whole tag remainder at every attribute boundary
🟡 Minor: 0 issues
⚪ Nit: 2 issues — duplicate superseded comment block in svgBlockEnd; linearity tests keep an svg token between regions so packed inert-only shapes stay uncovered

AI comment reconciliation: No new bot comments since round 9.

Final decision: 🔴 Needs changes before approval

dealako

Round-9 items are addressed — quote-aware scanning and the forward inert cursor are the right direction.

🔴 Needs changes before approval — two follow-ups:

🔴 Resume advanceInert searches across the window instead of re-slicing/re-scanning from zero each iteration
🔴 Anchor JSON_LD_ATTR_RE at each attribute boundary instead of re-testing the whole tag remainder

⚪ Nit: delete the duplicate comment paragraph in svgBlockEnd; add a linearity case that packs inert regions inside one <svg>

dealako · 2026-09-05T03:42:54Z

Round 10 supplement (for @mrautela365 — formal review already posted at 58b6db4b6; two additional items from late reviewer synthesis)

The two blocking items in the round-10 review stand. Two more medium correctness issues on the same commits:

1. startTagEnd opens quote mode on any quote, not only after = (:751-757)

startTagEnd enters quote mode on any ' or " inside a tag. Per WHATWG, a quote in the unquoted-attribute-value state is a parse error appended to the value — > still ends the tag. Here a stray apostrophe opens a phantom value and the scan runs to the next matching quote anywhere in the document (or EOF), so openEnd is -1 or lands far past the real tag end and the main loop drops the remainder.

Verified: <svg data-x=it's><text>SECRET</text></svg><p>Tokyo's venue</p> → output is " " (everything after the tag gone). parse5 keeps the paragraph.

Fix: Track whether the scan just passed = (whitespace preserves; other non-quote clears) and only enter quote mode when a quote arrives in that state.

2. advanceInert is not quote-aware inside svgBlockEnd (:1079-1085)

startTagEnd and isJsonLdTag were made quote-aware, but advanceInert is not — and inertCursor is left at the previous tag's index rather than past its end, so each call re-reads the preceding tag's attribute span. A <script, <style, or <!-- inside an attribute value is treated as a real inert opening; its close is never found, advanceInert returns -1, and the page tail is dropped.

Verified: <svg><g data-x="<script>"/><svg>a</svg>b</svg><p>Tokyo</p> → OLD keeps Tokyo, NEW returns " ".

Fix: Apply quote state while locating inert candidates; advance inertCursor to tagEnd + 1 after counting a real svg tag.

Nits still on file from round 10: duplicate comment block in svgBlockEnd (:1062-1071); packed inert-only linearity test; duplicate test+exec in isJsonLdTag.

dealako · 2026-09-05T03:43:38Z

Round 10 repro guards (for @mrautela365 — from reviewer harness, to close the two blockers + the missing linearity test)

Local repro (outside repo)

Reviewer harness at /tmp/scanbench/ (throwaway): scan.ts = HEAD lines 683–1160 verbatim, base.ts = same at 536086d8, fixed2.ts = verified fix. node cmp.ts / node verify2.ts on Node v24.11.0 reproduce quoted timings.

Missing linearity guard (nit from round 10)

Pack one inert kind inside a single <svg> so the window grows — sibling/nesting tests keep an svg token between regions and stay green while these fail:

// comments — 1,048,511 chars: 14,965 ms → 3 ms with fix (0 ms at 536086d8)
`<svg>${'<!-- c -->'.repeat(104_850)}</svg>`

// raw-text (stronger) — 1,048,491 chars: 19,477 ms → 18 ms with fix (0 ms at 536086d8)
`<svg>${'<style>a</style>'.repeat(65_530)}</svg>`

`isJsonLdTag` blocker guard

// 1,048,019 chars: 18,487 ms → 24 ms with sticky-anchor fix (0 ms at 536086d8)
`<script ${'a '.repeat(524_000)}>x</script>`

All three sit inside MAX_SCAN_CHARS — reachable without hitting the 5 MiB download ceiling.

dealako · 2026-09-05T03:45:57Z

Round 10 complete finding record (for @mrautela365 — consolidates formal review + supplements; nothing dropped)

🔴 Blocking (inline comments filed)

advanceInert re-scans window per inert region (campaign-proxy.service.ts:998) — O(N×W) when one <svg> packs many inert regions; ~8–19s at 1 MiB
isJsonLdTag re-tests whole tag at every attribute boundary (:789) — O(M×tagLength); ~18s on long <script> open tags

🟡 Minor

startTagEnd enters quote mode without requiring = (:751-757) — stray apostrophe in unquoted attribute drops page tail (supplement posted)
advanceInert not quote-aware inside svgBlockEnd (:1079-1085) — <script in attribute value drops page tail (supplement posted)
startTagEnd -1 on unbalanced attribute quote blanks extraction (:753-760) — new whole-page-drop path, undocumented/unpinned. <svg aria-label="oops><path/></svg><p>Tokyo</p> and <script src="a.js>x</script><p>Tokyo</p> return "" where pre-patch kept Tokyo. Unescaped quotes in title/alt/src are ordinary CMS output.
Unquoted JSON-LD boundary untested (spec:1378-1383, JSON_LD_ATTR_RE at :712) — (?=[\s/>]|$) lookahead is load-bearing but unpinned. type=text/javascript negative passes on literal alone; decoy case passes via isInsideQuotes. Deleting the lookahead leaves all tests green. Guard is live: type=application/ld+jsonx rejected, type=application/ld+json defer collected.

⚪ Nit

Duplicate superseded comment block in svgBlockEnd (:1062-1071)
Packed inert-only linearity test missing (sibling/nesting tests keep svg token between regions)
Duplicate test()+exec() in isJsonLdTag
Local slice named window shadows browser global (:998-1001) — no lint rule catches it
advanceInert -1 on unterminated nested raw-text inside svg untested (:1036-1039) — <svg><style>x</svg><p>Tokyo</p> returns single space

Revision verdicts (round 9 → 10)

✅ svgBlockEnd backward comment search — resolved (forward inertCursor)
✅ Linearity guard at scale — resolved (sibling + nesting tests; packed inert shape still missing — item 8)
⚠️ Stale spec comments naming stripSvgBlocks/boundedSource/strippers — not in this diff; needs pre-merge sweep (symbols no longer exist)

Repro guards: #2189 (comment)

@dealako

LFXV2-3198 Two quadratic paths, both reported as blocking and both reproduced first. 1. `advanceInert` re-sliced `html.slice(cursor, at)` and searched it from index 0 on every iteration. When one element holds many inert regions with no intervening svg token the window never shrinks, so N regions cost O(N x window): 155ms / 619ms / 2466ms at 20k / 40k / 80k comments inside a single `<svg>`. Two earlier attempts got half of it. Resuming the searches from a carried cursor fixed this shape but removed the bound, and every sibling `<svg>` then scanned the rest of the page again (16.5s). Rejecting a late hit after an unbounded `indexOf` does not help either -- the scan cost is already paid, the same mistake as bounding a regex with `lastIndex`. The bound has to be a fixed STRIDE. A 4096-character lookahead costs the same per step regardless of how much document remains, and a clear stretch advances the cursor by its whole length. Steps overlap by the longest token so a region cannot straddle a boundary. 2. `isJsonLdTag` tested an unanchored pattern at every attribute boundary, each scanning the whole tag remainder -- O(attributes x tag length), 184ms / 737ms / 2917ms at 50k / 100k / 200k attributes. The pattern is now sticky and tested at the boundary itself, which is the only question a boundary can answer, and the walk becomes one pass. `isInsideQuotes` is gone with it: the value-skipping in the main loop already provides what it checked. All four shapes now scale linearly -- 80k comments in one element 40ms, 32k siblings 6ms, 8k nesting levels 2ms, 400k attributes 7ms -- and 5 MiB of real svg markup stays at 9ms. Two tests, using the shapes the existing linearity tests miss: they keep an svg token between inert regions, so the window shrinks and they stayed green at ~5ms while this stalled. Both verified to fail against their own defect. Addresses review comments from @dealako. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

@dealako

LFXV2-3198 Two paragraphs said the same thing: the first was written when this scan only knew about comments, the second when it was generalised to inert regions, and generalising it did not remove the one it replaced. The third paragraph had also gone stale. It credited the forward cursor with making each region cost one visit, which was true of that revision but not of this one -- 7e6a9e8 replaced the window slice with a fixed-stride lookahead, and the cursor now only stops the work being repeated PER TAG. It points at advanceInert for the bound rather than restating it, so the two cannot drift apart again. Comment-only; no behaviour change. Addresses the nit from @dealako. The other half of that nit -- a linearity case packing inert regions inside one <svg> -- landed in 7e6a9e8. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.spec.ts:3749

This comment is factually incorrect: email flows also persisted briefs before delivery_type and stage existed, so not every pre-000030 row was paid (the same caveat is documented in campaign.interface.ts:227-233 and issue #2214). The normalization assertion is valid, but the comment should describe key compatibility without claiming the legacy row's original surface.

mrautela365 · 2026-09-07T03:00:21Z

@dealako — round 10 is addressed; head is ff451f2d1.

Both blocking items were already fixed when you filed them: your review was submitted against 58b6db4b6, and 7e6a9e85d landed just before it. Details are in the two threads, but in short:

advanceInert re-slicing — reproduced at 155/619/2466ms for 20k/40k/80k comments in one element. Two attempts got half of it: resuming from a carried cursor fixed that shape but removed the bound, and every sibling <svg> then rescanned the page at 16.5s. The working fix is a fixed 4096-char stride, which costs the same per step regardless of remaining document.
JSON_LD_ATTR_RE anchoring — took the sticky-flag fix as specified. It also let me delete isInsideQuotes entirely: it only existed to reject matches the unanchored search could reach inside quoted values.

Your nit was still live and is fixed in ff451f2d1. Two paragraphs in svgBlockEnd said the same thing — the first written when the scan only knew about comments, the second when it was generalised to inert regions, and generalising it did not remove the one it replaced.

While merging them I found a third that had gone stale: it credited the forward cursor with making each region cost one visit, which was true of that revision but not of this one, since 7e6a9e85d replaced the window slice with the stride. It now points at advanceInert for the bound rather than restating it, so the two cannot drift apart again. The other half of the nit — a linearity case packing inert regions inside one <svg> — landed in 7e6a9e85d.

All four shapes are linear: 80k comments in one element 40ms, 32k siblings 6ms, 8k nesting levels 2ms, 400k attributes 7ms, and 5 MiB of real svg markup 9ms.

State: 11/11 checks green, 0 unresolved threads, no conflicts, and Copilot's pass on the current head generated no comments. 2,840 server + 1,935 app tests, build clean.

Ready for another look whenever you have a moment.

LFXV2-3198 Two more copies of the assertion that every pre-field brief was authored on the paid surface, which the email flow's own pre-cutover writes disprove. One was reported (campaigns.component.spec.ts); the other (campaign.controller.ts) I found by sweeping for the claim rather than for the wording I had already fixed, which is what let five of these become seven across three rounds. Both now describe the default as a wire convention and point at #2214. The spec's assertion is unchanged and its reason is now the accurate one: a legacy row must land on the same ownership key as an explicit paid brief, or restoring it orphans the record written under the explicit identity. That is a key-compatibility requirement and holds whatever surface the row came from. Comment-only; no behaviour change. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/shared/components/input-text/input-text.component.html:9

The template comment hard-codes exact usage counts ("139 usages across 74 files"), which will quickly drift and become misleading. It’s safer to keep the wording qualitative so the comment stays true over time.

LFXV2-3198 The comment claimed "139 usages across 74 files". Five days later the real figures are 131 and 71, which is the reviewer's point demonstrated: an exact count in a comment is a fact about one afternoon, and nothing keeps it true. The mechanism it explains is unchanged and is the part worth keeping -- an unset `input<string>()` stringifies to the literal "undefined" in the DOM, so every call site omitting `placeholder` rendered that as the field's hint. The scale is now stated qualitatively. Comment-only; no behaviour change. Reported by Copilot. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

apps/lfx-one/src/server/services/campaign-proxy.service.ts:952

rawTextBlockEnd does not recognize </script/> or </style/>, although / is a valid delimiter after a raw-text end-tag name. The browser/parser closes the element there, but this scanner treats it as unclosed and drops the remainder of the document, so valid prose such as event details after the tag never reaches extraction. Include / in the closing-tag boundary (and cover this form in the raw-text tests).

…ery-aware-brief-restore

Copilot

Pull request overview

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

cursor

Stale Bugbot comment from a previous run.

LFXV2-3198 `emailBriefConflict` is single shared state, but it was the one field on this path written without a generation check. A stage change nulls `emailBriefPersistInFlight` so a newer persist can start, and the abandoned one then resolves and writes ITS outcome over the current send's -- either overwriting the live conflict token or, on its success arm, clearing it. Two consequences, both silent: the operator sees a generic retry banner for a refusal belonging to a send they have moved off, and `emailSaveFailureMessage` drops a pending overwrite because the conflict it matches against was wiped -- so the escape granted by a warning the operator DID see is lost. Both writes are now guarded, matching the discipline every neighbouring write on this path already follows. The stale-success arm no longer clears at all: an earlier revision reasoned a successful persist should not leave a conflict behind, which is true of its own send but not of this branch, where the page has already moved on and the token belongs to a different send. One test, verified to fail against the unguarded shape. It drives the real path -- a persist held open across a type change, answering after the current send has recorded its own refusal -- rather than asserting on state the test set itself, which an earlier draft did and could not have failed. Reported by Cursor. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

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

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

apps/lfx-one/src/server/controllers/campaign.controller.ts:869

stage is part of the brief identity and is now validated/forwarded, but it isn’t included in the operation metadata. Including it will make logs unambiguous when diagnosing brief lookups across multiple email stages.

This issue also appears on line 873 of the same file.
apps/lfx-one/src/server/services/campaign-proxy.service.ts:1060

There’s a formatting glitch here: the JSDoc for svgBlockEnd starts on the same line as the closing brace of advanceInert (} /**). This makes the comment easy to miss and prevents it from being recognized as a proper JSDoc attached to svgBlockEnd.

apps/lfx-one/src/server/controllers/campaign.controller.ts:875

The success log for campaign_load_brief omits the stage, so multiple email-stage lookups are indistinguishable in logs. Log deliveryType and stage alongside status/briefId to make troubleshooting stage-scoped restores possible.

      // `status` is logged on every arm, `unreadable` included: it is the one outcome that says
      // a stored brief exists and this build cannot open it, and nothing else would record it.
      logger.success(req, 'campaign_load_brief', startTime, { eventSlug, projectSlug, status: result.status, briefId: result.briefId });

…ery-aware-brief-restore

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

mrautela365 · 2026-09-08T18:46:57Z

@dealako — ready for re-review at 041c06cec. Your last review was against 58b6db4b6, which is now 7 commits back.

Since then, everything you raised is closed:

Your item	Status
`advanceInert` re-slicing (blocking)	fixed `7e6a9e85d` — fixed-stride lookahead
`JSON_LD_ATTR_RE` anchoring (blocking)	fixed `7e6a9e85d` — sticky flag, as you specified
duplicate paragraph in `svgBlockEnd` (nit)	fixed `ff451f2d1`
linearity case packing inert regions (nit)	landed `7e6a9e85d`

One more real bug found and fixed since, reported by Cursor and worth calling out because it is the kind only a reviewer catches: emailBriefConflict was the single field on the persist path written without a generation guard, so an abandoned persist could overwrite or wipe the live send's conflict token. The second-order effect was the bad one — emailSaveFailureMessage matches a pending overwrite against that token, so wiping it silently dropped an escape the operator had already earned. Fixed in e728c7cf1, mutation-verified.

Integration verified rather than assumed. The branch was 14 commits behind; it is now merged up to date with main (041c06cec) with zero conflicts, and I ran the full suites against the merged tree: 2,937 server + 2,041 app tests, build clean. That is what confirms this does not break the campaign files that moved on main — #2185 in particular touched the same files.

Current state: 11/11 checks green · 0 unresolved threads of 97 · 0 commits behind main · no outstanding bot comments.

Deploy order unchanged and still required: campaign-service#203 is merged (Sep 4) and must be deployed before this. Shipping this half against an un-migrated upstream is a no-op at best.

Two things I could not verify myself and would flag for your judgment rather than assert: whether cs#203 is actually deployed in the target environment, and the design calls themselves — the widened key and the single-scan extraction rewrite. Happy to walk through either.

@dealako

LFXV2-3198 Two medium correctness items from the round-10 supplement. Both drop the page tail, and both reproduced exactly as reported. 1. `startTagEnd` entered quote mode on ANY quote. A stray apostrophe in an unquoted value -- `<svg data-x=it's>` -- opened a phantom value that ran to the next matching quote anywhere in the document, so the tag never ended and everything after it was discarded. WHATWG puts a quote in the unquoted-attribute-value state into the VALUE and lets `>` still close the tag. Quote mode now opens only on a quote that follows `=`, with whitespace between allowed. 2. `advanceInert` was not quote-aware, and `inertCursor` stayed at the previous tag's START rather than past its end -- so each call re-read that tag's attribute span. A `<script`, `<style` or `<!--` inside an attribute value was read as a real inert opening, its close was never found, and the tail went with it. The candidate search now skips hits inside a quoted value, and the cursor advances to `tagEnd + 1` after a real svg tag is counted. `startTagEnd` and `isJsonLdTag` were already quote-aware; this is the third reader that needed the same rule, which is what the shared `isInsideAttributeValue` helper now expresses in one place. Six tests: the apostrophe case, three inert tokens inside attribute values, and the reviewer's two packed-inert linearity shapes -- one inert kind inside a single `<svg>`, where the window grows and the existing sibling/nesting tests stay green. All verified to fail against their own defect. The three perf shapes from the supplement were already fixed in 7e6a9e8 and are re-measured here: 132ms, 215ms and 12ms against the reported 15s, 19s and 18s. Reported by @dealako. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

mrautela365 · 2026-09-08T20:08:09Z

@dealako — the two medium items from the round-10 supplement are fixed in d3c459215. Both reproduced exactly as you wrote them.

I owe you an apology for the delay on these: you posted them as issue comments rather than review threads, and my checks were keyed on unresolved threads. So my count read 0 and I reported the PR clean three times while these sat open. That is my process failing, not your report — the finding record you posted was unambiguous.

1. startTagEnd opened quote mode on any quote — confirmed: <svg data-x=it's>…<p>Tokyo's venue</p> returned " ", whole tail gone. Quote mode now opens only on a quote following =, whitespace between allowed, matching the WHATWG unquoted-attribute-value state you cited.

2. advanceInert not quote-aware, cursor left at the tag start — confirmed: <svg><g data-x="<script>"/>… returned " ". Both halves of your fix applied: the candidate search skips hits inside a quoted value, and inertCursor advances to tagEnd + 1 after a real svg tag is counted.

Worth noting the shape of this: startTagEnd and isJsonLdTag were already quote-aware and advanceInert was the third reader needing the same rule. It is now a shared isInsideAttributeValue helper rather than a third copy — same consolidation the single-scan rewrite was for.

Your three perf shapes were already fixed in 7e6a9e85d; re-measured on this head with your exact inputs:

shape	reported	now
`<svg>` + 104,850 comments	14,965ms	132ms
`<svg>` + 65,530 `<style>`	19,477ms	215ms
`<script>` + 524k attributes	18,487ms	12ms

Six tests: the apostrophe case, three inert tokens inside attribute values, and both packed-inert linearity shapes — which is the nit you raised, since the sibling and nesting tests keep an svg token between regions and stay green on these. Each verified to fail against its own defect.

State at d3c459215: up to date with main (0 behind), 0 unresolved threads, all checks green, 2,943 server + 2,041 app tests.

Anything still open on your list, please say — I would rather you point at it than have me miss it in a comment surface again.

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

cursor

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

^{❌ 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 d3c4592. Configure here.}

dealako · 2026-09-08T20:18:41Z

@mrautela365 Re-review at head d3c4592. This one's been a long haul, and it holds up. The core idea is right: an event doesn't have "a brief," it has a paid brief plus a per-stage email series, so the planner now decides which send before it looks anything up and carries delivery_type + stage through the whole lookup key. The controller bug you found only against a live stack (reading delivery_type but dropping stage) is the kind that unit tests structurally can't catch, and validating the stage against CAMPAIGN_EMAIL_STAGES rather than forwarding it is the correct narrowing.

Every finding I raised across the prior rounds is resolved, and the HTML-extraction hardening in campaign-proxy.service.ts is now sound: byte ceiling enforced in fetchSafeUrl, MAX_SOURCE_CHARS / MAX_SCAN_CHARS bounds respected on every pass, and the walker's quote-, comment-, and nesting-awareness all landed with tests that fail against their own defect. The head commit (treat attribute values as text) closes my round-10 items: isInsideAttributeValue mirrors the startTagEnd quote rule so <script/<style/<!-- inside an attribute value is no longer read as a real opening, and inertCursor now advances past the counted tag. The added scans stay linear because they're bounded by INERT_LOOKAHEAD.

I re-reviewed the full branch diff against origin/main with parallel security/privacy, correctness/performance, and style/docs passes. No new issues surfaced. No PII concerns: this path handles event marketing copy and an access-key correctness fix, with no user-identifiable data reaching a log, error, or response sink.

Issue count

🔴 Blocking: 0
🟡 Minor: 0
⚪ Nit: 0
❔ Question: 0

All 98 review threads are resolved. CI is green (Code Quality, CodeQL, DCO, license, PR-title all pass). The only thing holding merge is my own prior CHANGES_REQUESTED, which this review clears.

Revision tracking

Findings from my earlier rounds, all confirmed fixed on disk:

Unbounded HTML preprocessing / JSON-LD quadratic scans → bounded to ldSource with the unscanned suffix re-attached; MAX_RESPONSE_BYTES (5 MiB) ceiling in fetchSafeUrl (3514424f5, ac770a483, 57c2d5377).
Case-sensitive close-tag walk / self-closing SVG / missing separators → 9102aeb63.
Closing-tag prefix match (</scriptx>) / overwrite outrunning the conflict warning → 2e1277f8f.
Walker treating nested/sibling openings as unclosed → per-tag-name bound, 0bbb26e04.
reset() clearing HubSpot UTM on stage change → b886aaae7.
Comment/style pre-pass swallowing prose; SVG-comment false pairing → b273231ec, 536086d8e, e1272f055.
Stale-persist clobbering the conflict token → e728c7cf1.
advanceInert / isJsonLdTag re-scanning windows (quadratic) → sticky-flag + resume, 7e6a9e85d.
Attribute-value tokens read as markup (round 10) → d3c459215 (head).
Vacuous / non-exercising tests → reworked to seed state and use input shapes that actually reach the walker.

Also confirmed the stale "no byte ceiling" and legacy-origin comments were corrected everywhere they appeared.

Final decision

✅ Approved

Genuinely strong work under sustained review pressure — the replies traced each finding to its mechanism and reproduced before fixing, which is exactly right. Ship it once campaign-service#203 is deployed (deploy-order note in the description is correct: this PR goes second).

dealako

Re-review at head d3c4592. All prior findings across ~15 rounds are resolved on disk, all 98 review threads closed, CI green. The delivery-type/stage lookup-key model is correct and the HTML-extraction walker is now quote-, comment-, and nesting-aware with bounded, linear scans. No new issues from the full-branch security/privacy, correctness/performance, and style/docs passes. Clearing my prior CHANGES_REQUESTED. See the summary comment for the full breakdown.

mrautela365 added 6 commits September 3, 2026 16:42

Copilot AI balanced review requested due to automatic review settings September 4, 2026 04:07

mrautela365 requested a review from a team as a code owner September 4, 2026 04:07

Copilot started reviewing on behalf of mrautela365 September 4, 2026 04:07 View session

github-advanced-security Bot found potential problems Sep 4, 2026

View reviewed changes

Comment thread apps/lfx-one/src/server/services/campaign-proxy.service.ts Fixed

Copilot AI reviewed Sep 4, 2026

View reviewed changes

mrautela365 requested a review from dealako September 4, 2026 04:11

cursor Bot reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 04:31

Copilot started reviewing on behalf of mrautela365 September 4, 2026 04:31 View session

github-advanced-security Bot found potential problems Sep 4, 2026

View reviewed changes

Comment thread apps/lfx-one/src/server/services/campaign-proxy.service.ts Fixed

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 04:40

Copilot started reviewing on behalf of mrautela365 September 4, 2026 04:41 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 05:12

Copilot started reviewing on behalf of mrautela365 September 4, 2026 05:13 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 05:51

Copilot AI reviewed Sep 5, 2026

View reviewed changes

dealako requested changes Sep 5, 2026

View reviewed changes

Comment thread apps/lfx-one/src/server/services/campaign-proxy.service.ts Outdated

Comment thread apps/lfx-one/src/server/services/campaign-proxy.service.ts Outdated

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat/LFXV2-3198-deliv…

f2869c3

…ery-aware-brief-restore

Copilot AI reviewed Sep 8, 2026

View reviewed changes

cursor Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread apps/lfx-one/src/app/modules/dashboards/campaigns/campaigns.component.ts Outdated

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat/LFXV2-3198-deliv…

041c06c

…ery-aware-brief-restore

Uh oh!

Conversation

mrautela365 commented Sep 4, 2026

What this changes

⚠️ Deploy order — this PR goes second

Commits

The bug only a live stack found

Also in here

Verification

Gates

Uh oh!

coderabbitai Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

cursor Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrautela365 commented Sep 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dealako commented Sep 5, 2026

Uh oh!

dealako left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dealako commented Sep 5, 2026

Uh oh!

dealako commented Sep 5, 2026

Local repro (outside repo)

Missing linearity guard (nit from round 10)

isJsonLdTag blocker guard

Uh oh!

dealako commented Sep 5, 2026

🔴 Blocking (inline comments filed)

🟡 Minor

⚪ Nit

Revision verdicts (round 9 → 10)

Uh oh!

coderabbitai Bot commented Sep 4, 2026 •

edited

Loading

cursor Bot commented Sep 4, 2026 •

edited

Loading

`isJsonLdTag` blocker guard

cursor Bot left a comment •

edited

Loading