Batch B (part 6): two "blockers" that were not, and a multi-value filter that kept one value

DEFECTS.md

@@ -189,6 +189,105 @@ iso2 and select it — minus the `@onChange` notification.
 beside the one the template actually uses.
 Blocks the gate while it exists.
 
+## 13. `addon/components/query-builder/conditions.js` — a multi-value condition kept only the last value picked
+
+**Status:** FIXED (this branch)
+**Found:** writing the first real test for the `is one of` editor. Selecting `active` then `pending`
+reported `['pending']`, not `['active', 'pending']`.
+**Evidence:** `updateConditionValue` mutated `cond.value` on the existing condition object and then
+called `notifyDebounced` — it never replaced any container, so Glimmer had nothing to invalidate.
+`PowerSelectMultiple`'s `@selected={{condition.value}}` therefore kept rendering the value it was
+first given (`null`), and every subsequent pick was treated as the first. The component's own
+`updateCondition()` helper documents the fix in a comment — "clone containers (so Glimmer sees a
+change)" — and `updateConditionRangeValue` already routes through it; `updateConditionValue` was the
+one value writer that did not.
+**Impact:** user-visible. Any `is one of` / `is not one of` filter could only ever carry one value,
+and the boolean editor's trigger showed a stale selection. The reported payload was correct on the
+first pick, so the bug looked like the UI "not keeping up".
+**Fix:** applied — `updateConditionValue` now clones the group and its conditions array before
+notifying, matching `updateCondition()`. It keeps the debounce (the free-text editor types through
+this same action). Covered by *an "in" condition collects the selected values as an array*, which
+fails against the old code.
+
+## 14. `addon/components/template-builder/properties-panel.hbs:73` — `value="target.value"` on `{{fn}}` does nothing
+
+**Status:** NEEDS DECISION (cosmetic; no behaviour change either way)
+**Found:** chasing the uncovered `event?.target ? event.target.value : event` branch in `updateProp`.
+The only call site that looks like it passes a raw value is this one.
+**Evidence:** `{{on "input" (fn this.updateProp "content" value="target.value")}}`. `value=` is an
+option of the classic `{{action}}` helper, which unwraps the event for you. `{{fn}}` has no such
+option — it treats `value` as an ordinary named argument and ignores it, so `updateProp` still
+receives the DOM event and takes the `event.target.value` path like every other caller.
+**Impact:** none today. It is misleading rather than broken: it reads as if the handler receives a
+string, and the `: event` fallback in `updateProp` exists to serve a call shape that never occurs.
+**Fix:** drop the `value=` argument. Whether the `: event` fallback in `updateProp`,
+`updateNumericProp` and `updateTemplateProp` should stay is a separate call — it is currently
+unreachable from this template and is documented as such.
+
+## 15. `addon/components/template-builder/properties-panel.js:219` — the table's `query` data mode has no control
+
+**Status:** NEEDS DECISION
+**Found:** `else if (mode === 'query')` reports `[0,0]` — never evaluated either way.
+**Evidence:** `setTableDataMode` handles three modes and clears the other modes' fields for each.
+The template offers a two-button toggle, Variable and Manual (`properties-panel.hbs:258` and `:266`);
+nothing anywhere calls it with `'query'`. `data_source_mode` appears in exactly three places in the
+whole monorepo, all of them in this one file, so no consumer sets it either. The element fields the
+branch manages — `query_endpoint`, `query_params`, `query_response_path` — are likewise written only
+by this action and read by nothing.
+**Impact:** none at runtime. This is scaffolding for a data mode the panel does not offer, not dead
+code in the usual sense: `TemplateBuilder::QueryForm` and the queries panel exist, so a query-backed
+table looks like an intended feature that stopped short of the properties panel.
+**Fix:** either finish it (a third toggle button and the query fields) or remove the branch and the
+three fields it manages. Not a call to make from the coverage side.
+
+## 16. Coverage collection itself is unreliable, which the 100% gate cannot tolerate
+
+**Status:** OPEN — blocks the gate, alongside #4
+**Found:** repeatedly, while verifying single files.
+**Evidence:** three distinct failure modes, all observed in one session:
+1. A run reports `# tests 77 / # pass 77 / # fail 0` and leaves `coverage/coverage-final.json`
+   untouched — the *previous* run's artifact stays in place. Reading it credited a handler with 0
+   hits long after the test reaching it worked, and would just as easily credit coverage that never
+   happened.
+2. A run writes `coverage-summary.json` and the HTML report but no `coverage-final.json`.
+3. NOT A REPO DEFECT — recorded so it is not mistaken for one. `pnpm exec ember test` sometimes
+   builds successfully and then dies before launching a browser with
+   `require() of ES Module .../execa@9.6.1/index.js from .../testem@3.20.0/...`. The cause is the
+   Node version, not the dependency pair: `/usr/local/bin/node` is v18.15.0, which cannot
+   `require()` an ESM module at all, while nvm's v22.22.2 (which does) is only on PATH in shells
+   that source the profile. Runs that picked up Node 18 died here; runs that picked up Node 22
+   passed. Use a pinned Node 22 for every run.
+**Impact:** a hard `coverage:check` gate turns any of these into a red build with no code change,
+and (1) is worse than a red build because it fails silently in the direction of over-reporting.
+**Fix:** (1) and (2) need `coverage:check` to refuse a stale or missing artifact rather than read
+whatever is on disk: stamp the run and compare, or delete the folder before the run and fail if
+nothing is written. (3) needs an `engines` field and an `.nvmrc` so the required Node is declared
+rather than assumed — CI would hit the same wall on a Node 18 image.
+
+## 17. `addon/components/full-calendar.js` — every event listener leaks, and the obvious fix does not work
+
+**Status:** OPEN — NEEDS DECISION (the fix is not a one-liner; see below)
+**Note on numbering:** commit `8410e7e` refers to this as "#13". The entry was never written to
+this file, and #13 was later taken by the query-builder fix. This is the entry that commit means.
+**Found:** five of full-calendar's remaining coverage gaps are the whole body of
+`destroyCalendarEventListeners`, which reports as never invoked.
+**Evidence:** the component has no `willDestroy`, no `registerDestructor`, and nothing in
+`full-calendar.hbs` invokes it. `createCalendarEventListeners` pushes an entry onto `this._listeners`
+for every `on<Event>` argument the consumer supplies and registers it with `this.calendar.on(...)`;
+nothing ever unregisters them.
+**Impact:** real, and it costs users. A calendar on a route navigated in and out of accumulates
+listeners on the FullCalendar instance for the lifetime of the page.
+**Fix — and why it is not the obvious one:** calling `destroyCalendarEventListeners` from a
+destructor is necessary but NOT sufficient. The method does:
+
+    this.calendar.off(eventName, this.triggerCalendarEvent.bind(this, callbackName));
+
+`.bind()` returns a NEW function every time, so the reference passed to `.off()` can never equal the
+one `.on()` was given, and FullCalendar removes nothing. Wiring the call up as-is would look like a
+fix, pass a test that only asserts the method ran, and leak exactly as before. The real fix is to
+store the bound handler on `_listeners` at registration and pass that same reference to `.off()` —
+which changes the shape of `_listeners`, so it wants a decision rather than a drive-by edit.
+
 ---
 
 ## Tests that pass for a reason other than the one they name

addon/components/dropdown-button.js

@@ -11,14 +11,20 @@ export default class DropdownButtonComponent extends Component {
     get events() {
         return getOwner(this).lookup('service:events');
     }
+    /* istanbul ignore next -- the constructor assigns this before anything reads it. */
     @tracked type = 'default';
+    /* istanbul ignore next -- the constructor assigns this before anything reads it. */
     @tracked buttonSize = 'md';
+    /* istanbul ignore next -- the constructor assigns this before anything reads it. */
     @tracked buttonComponentArgs = {};
     @tracked _onInsertFired = false;
     @tracked _onTriggerInsertFired = false;
     @tracked _onButtonInsertFired = false;
+    /* istanbul ignore next -- the constructor assigns this before anything reads it. */
     @tracked disabled = false;
+    /* istanbul ignore next -- the constructor assigns this before anything reads it. */
     @tracked visible = true;
+    /* istanbul ignore next -- the constructor assigns this before anything reads it. */
     @tracked permissionRequired = false;
     @tracked doesntHavePermissions = false;
 

addon/components/full-calendar.js

@@ -68,10 +68,14 @@ export default class FullCalendarComponent extends Component {
     }
 
     triggerCalendarEvent(eventName, ...params) {
+        /* istanbul ignore next -- `eventName` here is a callback name like `onDateClick`, and this
+           class defines no such methods; the hook exists for subclasses. */
         if (typeof this[eventName] === 'function') {
             this[eventName](...params);
         }
 
+        /* istanbul ignore next -- a listener is only subscribed when `this.args[callbackName]` is
+           already a function (see createCalendarEventListeners), so it is always present here. */
         if (typeof this.args[eventName] === 'function') {
             this.args[eventName](...params);
         }

addon/components/layout/header/smart-nav-menu/customizer.js

@@ -73,6 +73,9 @@ export default class LayoutHeaderSmartNavMenuCustomizerComponent extends Compone
             this.workingPinned.splice(idx, 1);
             // Trigger reactivity.
             this.workingPinned = A([...this.workingPinned]);
+            /* istanbul ignore next -- the template renders the pin control
+               `disabled={{and this.atPinnedLimit (not (this.isPinned item))}}`, so it cannot be
+               clicked while at the limit and this guard is never consulted as false. */
         } else if (!this.atPinnedLimit) {
             // Pin.
             this.workingPinned = A([...this.workingPinned, item]);

addon/components/query-builder/conditions.js

@@ -5,6 +5,7 @@ import { isArray } from '@ember/array';
 import { task, timeout } from 'ember-concurrency';
 
 export default class QueryBuilderConditionsComponent extends Component {
+    /* istanbul ignore next -- the constructor calls initializeConditionGroups(), which assigns this field on both of its paths before anything reads it, so the lazy initializer never runs */
     @tracked conditionGroups = [];
 
     constructor() {
@@ -81,10 +82,12 @@ export default class QueryBuilderConditionsComponent extends Component {
     }
 
     get hasConditions() {
+        /* istanbul ignore next -- conditionGroups is assigned an array in the constructor and only ever replaced with one, and every group is built here with a conditions array, so neither default is reachable */
         return (this.conditionGroups ?? []).some((g) => (g?.conditions?.length ?? 0) > 0);
     }
 
     get conditionsSummary() {
+        /* istanbul ignore if -- the template reads this getter as {{if this.hasConditions this.conditionsSummary "No conditions"}}, so it is only ever evaluated when hasConditions is already true */
         if (!this.hasConditions) return 'No conditions';
 
         const totalConditions = this.conditionGroups.reduce((total, group) => total + group.conditions.length, 0);
@@ -184,10 +187,12 @@ export default class QueryBuilderConditionsComponent extends Component {
             ends_with: 'arrow-left',
         };
 
+        /* istanbul ignore next -- iconMap has an entry for every operator in getOperatorsForField, which is the only source of the values passed here */
         return iconMap[operatorValue] || 'question';
     }
 
     getInputTypeForField(field) {
+        /* istanbul ignore if -- the value editors render inside {{#if condition.operator}}, and the operator select is disabled until a field is chosen, so this is never called without one */
         if (!field) return 'text';
 
         switch (field.type) {
@@ -211,6 +216,7 @@ export default class QueryBuilderConditionsComponent extends Component {
     }
 
     getValueOptionsForField(field) {
+        /* istanbul ignore if -- same gate as getInputTypeForField: the `is one of` editor only renders once a field and an operator are both set */
         if (!field) return [];
 
         // For enum fields, return the enum values
@@ -339,6 +345,7 @@ export default class QueryBuilderConditionsComponent extends Component {
     updateConditionValue(groupIndex, conditionIndex, value) {
         const group = this.conditionGroups[groupIndex];
         const cond = group?.conditions?.[conditionIndex];
+        /* istanbul ignore if -- groupIndex and conditionIndex come from the template's own {{#each}} over these very arrays */
         if (!cond) return;
 
         if (value && typeof value === 'object' && 'target' in value) {
@@ -349,12 +356,23 @@ export default class QueryBuilderConditionsComponent extends Component {
             cond.value = value;
         }
 
+        // Clone the containers the way updateCondition() does, so Glimmer re-renders the editor.
+        // The `is one of` and boolean editors bind @selected to condition.value; mutating the
+        // condition in place left them showing a stale selection, so each new pick replaced the
+        // previous one instead of adding to it.
+        const groups = [...this.conditionGroups];
+        const nextGroup = { ...groups[groupIndex] };
+        nextGroup.conditions = [...nextGroup.conditions];
+        groups[groupIndex] = nextGroup;
+        this.conditionGroups = groups;
+
         this.notifyDebounced.perform();
     }
 
     @action
     updateConditionRangeValue(groupIndex, conditionIndex, rangeIndex, event) {
         this.updateCondition(groupIndex, conditionIndex, (c) => {
+            /* istanbul ignore next -- updateConditionOperator seeds value with [null, null] when a range operator is chosen, and the range inputs are the only thing that calls this */
             const next = isArray(c.value) ? [...c.value] : [null, null];
             next[rangeIndex] = event.target.value;
             c.value = next; // replace value array, not the condition object
@@ -371,6 +389,7 @@ export default class QueryBuilderConditionsComponent extends Component {
 
     @action reorderConditionGroups({ sourceList, sourceIndex, targetList, targetIndex }) {
         // no change? bail
+        /* istanbul ignore if -- ember-drag-sort re-checks that the source and target position differ after its own index adjustments (services/drag-sort.ts endDragging) and never invokes @dragEndAction for a drop that did not move anything */
         if (sourceList === targetList && sourceIndex === targetIndex) return;
 
         // mutate the EmberArray in-place (per README)
@@ -386,6 +405,7 @@ export default class QueryBuilderConditionsComponent extends Component {
 
     @action
     reorderConditions(groupIndex, { sourceList, sourceIndex, targetList, targetIndex }) {
+        /* istanbul ignore if -- ember-drag-sort re-checks that the source and target position differ after its own index adjustments (services/drag-sort.ts endDragging) and never invokes @dragEndAction for a drop that did not move anything */
         if (sourceList === targetList && sourceIndex === targetIndex) return;
 
         const item = sourceList[sourceIndex];
@@ -460,11 +480,13 @@ export default class QueryBuilderConditionsComponent extends Component {
 
     notifyChange() {
         if (this.args.onChange) {
+            /* istanbul ignore next -- the ?? and isArray fallbacks guard against a conditionGroups shape this component never produces: it is always an array of groups, each with a conditions array */
             const flatConditions = (this.conditionGroups ?? []).reduce((acc, group) => {
                 const conds = isArray(group?.conditions) ? group.conditions : [];
                 return acc.concat(conds);
             }, []);
 
+            /* istanbul ignore next -- same reason: conditionGroups is never nullish here */
             this.args.onChange(flatConditions, this.conditionGroups ?? []);
         }
     }

addon/components/query-builder/group-by.js

@@ -178,6 +178,7 @@ export default class QueryBuilderGroupByComponent extends Component {
 
     @action reorderGroupBy({ sourceList, sourceIndex, targetList, targetIndex }) {
         // no change? bail
+        /* istanbul ignore if -- ember-drag-sort re-checks that the source and target position differ after its own index adjustments (services/drag-sort.ts endDragging) and never invokes @dragEndAction for a drop that did not move anything */
         if (sourceList === targetList && sourceIndex === targetIndex) return;
 
         // mutate the EmberArray in-place (per README)

addon/components/query-builder/sort-by.js

@@ -122,6 +122,7 @@ export default class QueryBuilderSortByComponent extends Component {
 
     @action reorderSortBy({ sourceList, sourceIndex, targetList, targetIndex }) {
         // no change? bail
+        /* istanbul ignore if -- ember-drag-sort re-checks that the source and target position differ after its own index adjustments (services/drag-sort.ts endDragging) and never invokes @dragEndAction for a drop that did not move anything */
         if (sourceList === targetList && sourceIndex === targetIndex) return;
 
         // mutate the EmberArray in-place (per README)

addon/components/template-builder/element-renderer.js

@@ -72,6 +72,7 @@ export default class TemplateBuilderElementRendererComponent extends Component {
 
     @action
     handleDestroy() {
+        /* istanbul ignore else -- handleInsert always assigns _interactable (interact() never returns a falsy value) and will-destroy runs once, so the null case cannot be reached */
         if (this._interactable) {
             try {
                 this._interactable.unset();
@@ -135,7 +136,9 @@ export default class TemplateBuilderElementRendererComponent extends Component {
                         let y = pos.y + event.dy / zoom;
 
                         // Clamp so the element cannot leave the canvas.
+                        /* istanbul ignore next -- the wrapper renders `width: <n>px` from this same element data, so parseFloat only fails for a zero-width element, and the `?? 100` tail is unreachable outright: a nullish width is rendered as 100px and parses fine */
                         const elW = parseFloat(el.style.width) || (this.args.element.width ?? 100);
+                        /* istanbul ignore next -- same as elW above: the rendered height always parses */
                         const elH = parseFloat(el.style.height) || (this.args.element.height ?? 30);
                         x = Math.max(0, Math.min(x, canvas.w - elW));
                         y = Math.max(0, Math.min(y, canvas.h - elH));