From 3a7a08425cd68e03c62a2f18b93ce55e3b20ded3 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Sat, 29 Aug 2026 12:26:29 -0500 Subject: [PATCH] fix(engagement): the six defects the browser pass found (Phase 4b) Driving the two screens in Chrome, after the API walk had already found the two in Phase 4a's code. None of these is visible from a test or from curl. Two cost an operator something real: - The Audience dropdown rendered EMPTY before a trigger was chosen. There is genuinely nothing it may offer without a ceiling, but a select with zero options reads as broken rather than as waiting. It now says "Choose a trigger first..." and is disabled. - A `members` audience with no saved audience reaches NOBODY, and only the preview button said so. That is the design, but it is also the default the instant a members-ceiling trigger is picked - so the rule saves, gets switched on, and mails nobody with nothing on screen saying so. The editor now says it inline, and stands down once a preview has answered the same question more precisely. One the server was already refusing, just too late: - The composer offered "exclude" on the only row, building an `and` whose every child is a complement. The server refuses it correctly but only after a save, and it is one checkbox away at all times. Now refused inline, in the operator's words. Three wording and layout: - the template-key input truncated its placeholder, and said "optional until Phase 5" - a sentence about the plan document, not about the deployment - "segment" leaked into a screen that says "saved audience" everywhere else. The API, schema and docs keep saying segment (one word for one table); translated at the point of display only - the composer repeated its AUDIENCE heading above every row Client only - no server change, so swagger and the route manifest are untouched. Client suite 316/316; all six verified in the browser after the fix. - [x] AI-assisted: written with Claude Code (Opus) Co-Authored-By: Claude --- client/src/lib/engagementRules.js | 56 ++++++++++++++++++- .../admin/views/EngagementAudiences.jsx | 6 +- .../routes/admin/views/EngagementRules.jsx | 21 ++++++- client/test/engagementRules.test.js | 30 ++++++++++ 4 files changed, 105 insertions(+), 8 deletions(-) diff --git a/client/src/lib/engagementRules.js b/client/src/lib/engagementRules.js index 5611e80..7e1eb59 100644 --- a/client/src/lib/engagementRules.js +++ b/client/src/lib/engagementRules.js @@ -101,15 +101,56 @@ export function segmentChoicesFor(trigger, ceilings, segments) { */ export function describeReach(preview) { if (!preview) return '' - if (preview.dormant) return `Resolves to nobody right now — ${preview.reason || 'dormant'}.` + const why = operatorWords(preview.reason) + if (preview.dormant) return `Resolves to nobody right now — ${why || 'dormant'}.` if (preview.permitted === false) { return `Reaches ${preview.count}, but this trigger does not permit that audience — saving will be refused.` } - if (preview.reason) return `${preview.count} right now — ${preview.reason}.` + if (why) return `${preview.count} right now — ${why}.` if (preview.capped) return `At least ${preview.count} people (the preview stops counting there).` return preview.count === 1 ? '1 person right now.' : `${preview.count} people right now.` } +/** + * The server says "segment"; these screens say "saved audience". + * + * The API, the schema and the docs all call it a segment and should keep doing + * so - it is one word for one table. But an operator meets the concept here, + * under a heading that says "Audiences", and a sentence that switches vocabulary + * mid-screen reads as a sentence about something else. + */ +export function operatorWords(text) { + if (!text) return text + // Word-wise rather than a regex, so "segmented" and the like are left alone. + const swap = { segment: 'saved audience', segments: 'saved audiences' } + return String(text) + .split(' ') + .map((word) => swap[word] || word) + .join(' ') +} + +/** + * The one audience choice that silently reaches nobody, said out loud. + * + * `members` is the ceiling for "a module-declared list". Without a saved + * audience naming WHICH list there is no list, and core knows no game vocabulary + * with which to guess - so the rule resolves to the empty set every time it + * fires. It is also the DEFAULT the moment an operator picks a `members`-ceiling + * trigger, which is what makes it a trap rather than a curiosity: the rule saves, + * switches on, and mails nobody, with nothing on the screen saying so unless the + * operator happens to press Preview. + * + * Returns a sentence, or null when there is nothing to warn about. + */ +export function audienceWarning(form) { + if (!form) return null + if (form.audienceSegmentId) return null + if (form.audience === 'members') { + return 'This reaches nobody as it stands. “Members of a module-declared list” needs a saved audience naming which list.' + } + return null +} + // ── Segment expressions ──────────────────────────────────────────────────── /** @@ -127,7 +168,16 @@ export function notPlacementError(expression) { if (!node || typeof node !== 'object') return null if (!node.op) return null if (node.op === 'not' && !underAnd) { - return 'A "not" can only be used inside an "all of" group — on its own it would mean "everyone except…".' + return 'An excluded audience can only be used alongside an included one — on its own it would mean “everyone except…”.' + } + // The same rule from the other side: a group of nothing but exclusions has + // no set to take them from. The composer offers "exclude" on every row, so + // this is one checkbox away at all times and is worth saying before the + // round trip - the server refuses it, correctly, but only after a save. + if ((node.op === 'and' || node.op === 'or') && (node.nodes || []).length) { + if ((node.nodes || []).every((c) => c && c.op === 'not')) { + return 'At least one audience has to be included — a list made only of exclusions has nothing to exclude from.' + } } for (const child of node.nodes || []) { const err = walk(child, node.op === 'and') diff --git a/client/src/routes/admin/views/EngagementAudiences.jsx b/client/src/routes/admin/views/EngagementAudiences.jsx index 0bd892b..de09168 100644 --- a/client/src/routes/admin/views/EngagementAudiences.jsx +++ b/client/src/routes/admin/views/EngagementAudiences.jsx @@ -50,12 +50,13 @@ const toGroup = (expression) => // ── One leaf: an audience and its declared parameters ────────────────────── -function LeafRow({ audiences, node, onChange, onRemove, negated, onToggleNegate, canNegate }) { +function LeafRow({ audiences, node, onChange, onRemove, negated, onToggleNegate, canNegate, first }) { const declared = audiences.find((a) => a.id === node.audienceId) return (