Adopt eslint-plugin-qunit and a rule for unguarded handler arguments

.eslintrc.js

@@ -55,7 +55,6 @@ module.exports = {
                 './.eslintrc.js',
                 './.prettierrc.js',
                 './.stylelintrc.js',
-                './.template-lintrc.js',
                 './ember-cli-build.js',
                 './index.js',
                 './testem.js',
@@ -74,5 +73,18 @@ module.exports = {
             },
             extends: ['plugin:n/recommended'],
         },
+        // QUnit test files. `no-hooks-from-ancestor-modules` is the one that matters most here:
+        // a nested `module('…', function (hooks) { … })` shadows the outer hooks, which QUnit 3
+        // turns into a hard error. It is easy to reintroduce and the suite stays green when you do.
+        {
+            files: ['tests/**/*-test.js'],
+            plugins: ['qunit'],
+            extends: ['plugin:qunit/recommended'],
+            rules: {
+                // Opinionated and noisy for this suite: most tests assert an obvious, fixed number
+                // of things, and an expect() count that drifts is worse than none.
+                'qunit/require-expect': 'off',
+            },
+        },
     ],
 };

.template-lintrc.js → .template-lintrc.mjs

@@ -1,8 +1,20 @@
-'use strict';
+import NoUnguardedHandlerArgument from './lint/no-unguarded-handler-argument.mjs';
 
-module.exports = {
+export default {
+    plugins: [
+        {
+            name: 'fleetbase-ember-ui',
+            rules: {
+                'no-unguarded-handler-argument': NoUnguardedHandlerArgument,
+            },
+        },
+    ],
     extends: 'recommended',
     rules: {
+        // `{{on "click" @arg}}` and `{{fn @arg …}}` THROW while rendering when the argument is
+        // absent — they are not no-ops. Thirteen bindings across five components were fixed for
+        // this; the rule keeps them from coming back.
+        'no-unguarded-handler-argument': true,
         'no-invalid-interactive': 'off',
         'no-yield-only': 'off',
         'no-pointer-down-event-binding': 'off',
@@ -18,6 +30,15 @@ module.exports = {
         },
     },
     overrides: [
+        {
+            // Test fixtures always supply the handlers they bind — the template is written
+            // alongside the arguments it needs. The rule exists to protect the addon's shipped
+            // components, where the caller is someone else.
+            files: ['tests/**/*-test.js'],
+            rules: {
+                'no-unguarded-handler-argument': false,
+            },
+        },
         {
             // Modifier tests set inline styles on their fixtures because the
             // element's own style is exactly what the modifier under test reads

addon/components/layout/header/dropdown/item.hbs

@@ -74,7 +74,7 @@
             class="next-header-dd-menu-item next-dd-item {{if @item.disabled 'disabled'}} {{@item.class}}"
             target={{@item.target}}
             disabled={{@item.disabled}}
-            {{on "click" (fn @onAction @item.action @item.params)}}
+            {{on "click" (fn (or @onAction (noop)) @item.action @item.params)}}
         >
             {{#if @item.icon}}
                 <FaIcon @icon={{@item.icon}} @size={{or @item.iconSize "sm"}} class={{or @item.iconClass "mr-2"}} />
@@ -90,7 +90,7 @@
 
 {{#if this.isInteractive}}
     <div class={{if @item.overwriteWrapperClass @item.wrapperClass (concat @item.wrapperClass " px-1")}}>
-        <a href="javascript:;" class="next-header-dd-menu-item next-dd-item {{if this.active 'active'}}" disabled={{@item.disabled}} {{on "click" @item.onClick}} ...attributes>
+        <a href="javascript:;" class="next-header-dd-menu-item next-dd-item {{if this.active 'active'}}" disabled={{@item.disabled}} {{on "click" (or @item.onClick (noop))}} ...attributes>
             {{#if @item.icon}}
                 <FaIcon @icon={{@item.icon}} @size={{or @item.iconSize "sm"}} class={{or @item.iconClass "mr-2"}} />
             {{/if}}

addon/components/modals/bulk-action-model.hbs

@@ -53,7 +53,7 @@
                     </div>
 
                     <div>
-                        <a href="javascript:;" {{on "click" (fn @options.remove selected)}} class="my-1">
+                        <a href="javascript:;" {{on "click" (fn (or @options.remove (noop)) selected)}} class="my-1">
                             <FaIcon @icon="times-circle" />
                         </a>
                     </div>

addon/components/modals/bulk-delete-model.hbs

@@ -30,7 +30,7 @@
                 <li>
                     <span class="text-sm dark:text-gray-100">{{get selected @options.modelNamePath}}</span>
 
-                    <a href="javascript:;" {{on "click" (fn @options.remove selected)}} class="my-1">
+                    <a href="javascript:;" {{on "click" (fn (or @options.remove (noop)) selected)}} class="my-1">
                         <FaIcon @icon="times-circle" />
                     </a>
                 </li>

addon/components/modals/import-form.hbs

@@ -79,7 +79,7 @@
                                         {{#each @options.fileQueueColumns as |column|}}
                                             <td>
                                                 {{#if (eq column.key "delete")}}
-                                                    <a href="javascript:;" {{on "click" (fn @options.removeFile file)}}>
+                                                    <a href="javascript:;" {{on "click" (fn (or @options.removeFile (noop)) file)}}>
                                                         <FaIcon @icon="trash" class="text-red-500" />
                                                     </a>
                                                 {{else if (eq column.key "type")}}

addon/components/modals/save-report.hbs

@@ -15,7 +15,7 @@
         {{#if @options.showScheduling}}
             <div class="border-t border-gray-200 dark:border-gray-700 pt-4 mt-4">
                 <div class="flex items-center mb-3">
-                    <input type="checkbox" checked={{@options.isScheduled}} {{on "change" @options.toggleScheduling}} class="form-checkbox h-4 w-4 text-blue-600" />
+                    <input type="checkbox" checked={{@options.isScheduled}} {{on "change" (or @options.toggleScheduling (noop))}} class="form-checkbox h-4 w-4 text-blue-600" />
                     <label class="ml-2 text-sm font-medium text-gray-700 dark:text-gray-300">
                         Schedule this report
                     </label>

addon/components/overlay/header.hbs

@@ -113,7 +113,7 @@
                 class="next-content-overlay-panel-maximize-button {{@maximizeButtonClass}}"
                 {{set-height @maximizeButtonHeight}}
                 {{set-width @maximizeButtonWidth}}
-                {{on "click" @overlay.maximize}}
+                {{on "click" (or @overlay.maximize (noop))}}
             >
                 <FaIcon @icon={{or @minimizeIcon "window-maximize"}} @size={{@iconSize}} @prefix={{or @iconPrefix "fas"}} class={{@iconClass}} />
             </button>
@@ -124,7 +124,7 @@
                 class="next-content-overlay-panel-minimize-button {{@minimizeButtonClass}}"
                 {{set-height @minimizeButtonHeight}}
                 {{set-width @minimizeButtonWidth}}
-                {{on "click" @overlay.minimize}}
+                {{on "click" (or @overlay.minimize (noop))}}
             >
                 <FaIcon @icon={{or @minimizeIcon "window-minimize"}} @size={{@iconSize}} @prefix={{or @iconPrefix "fas"}} class={{@iconClass}} />
             </button>

addon/components/table/cell/link-list.hbs

@@ -3,7 +3,7 @@
         {{#let (get @row @column.valuePath) as |items|}}
             {{#each items as |item|}}
                 <li>
-                    <a href="javascript:;" class="truncate" {{on "click" (fn @column.action item @row)}}>
+                    <a href="javascript:;" class="truncate" {{on "click" (fn (or @column.action (noop)) item @row)}}>
                         {{get item (or @column.cellComponentLabelPath "name")}}
                     </a>
                 </li>

lint/no-unguarded-handler-argument.mjs

@@ -0,0 +1,104 @@
+import { Rule } from 'ember-template-lint';
+
+/**
+ * Flags `{{on "event" @handler}}` and `{{fn @handler …}}` where the handler position holds a BARE
+ * argument.
+ *
+ * These are not no-ops when the argument is absent — Glimmer throws while rendering:
+ *
+ *   {{on "click" @onFoo}}   → "You must pass a function as the second argument to the `on` modifier"
+ *   {{fn @onFoo item}}      → "You must pass a function as the `fn` helper's first argument"
+ *
+ * so a component with an unguarded binding cannot be rendered at all without that argument. Five
+ * components in this addon were untestable for exactly this reason.
+ *
+ * Guarded forms are accepted:
+ *   {{on "click" (or @onFoo (noop))}}      — the pattern this codebase settled on
+ *   {{#if @item.onClick}}{{on "click" @item.onClick}}{{/if}}
+ *   {{on "click" this.handler}}            — a component's own action always exists
+ */
+export default class NoUnguardedHandlerArgument extends Rule {
+    logNode({ node, message }) {
+        this.log({ message, node });
+    }
+
+    /** Is this node a bare `@arg` / `@arg.path` reference? */
+    isBareArgument(node) {
+        return node && node.type === 'PathExpression' && node.head && node.head.type === 'AtHead';
+    }
+
+    /** Walk out to see whether an enclosing `{{#if}}`/`{{#unless}}` already tests this argument. */
+    isGuardedByBlock(argumentName) {
+        return this.guardStack.some((guard) => guard === argumentName || argumentName.startsWith(`${guard}.`));
+    }
+
+    static pathName(node) {
+        return node.original || '';
+    }
+
+    visitor() {
+        this.guardStack = [];
+
+        const enterBlock = (node) => {
+            const name = node.path && node.path.original;
+            if (name !== 'if' && name !== 'unless') {
+                return;
+            }
+            const [condition] = node.params;
+            if (this.isBareArgument(condition)) {
+                this.guardStack.push(NoUnguardedHandlerArgument.pathName(condition));
+            } else {
+                this.guardStack.push(null);
+            }
+        };
+
+        const exitBlock = (node) => {
+            const name = node.path && node.path.original;
+            if (name === 'if' || name === 'unless') {
+                this.guardStack.pop();
+            }
+        };
+
+        const check = (node, position, describe) => {
+            const candidate = node.params[position];
+            if (!this.isBareArgument(candidate)) {
+                return;
+            }
+            const argumentName = NoUnguardedHandlerArgument.pathName(candidate);
+            if (this.isGuardedByBlock(argumentName)) {
+                return;
+            }
+            this.logNode({
+                node: candidate,
+                message:
+                    `${describe} receives the bare argument \`${argumentName}\`, which throws while rendering when the ` +
+                    `argument is absent. Guard it — \`(or ${argumentName} (noop))\` — or wrap the binding in ` +
+                    `\`{{#if ${argumentName}}}\`.`,
+            });
+        };
+
+        return {
+            BlockStatement: { enter: enterBlock, exit: exitBlock },
+
+            ElementModifierStatement(node) {
+                // {{on "click" @onFoo}} — the handler is the second parameter
+                if (node.path && node.path.original === 'on' && node.params.length >= 2) {
+                    check(node, 1, 'The `on` modifier');
+                }
+            },
+
+            SubExpression(node) {
+                // (fn @onFoo item) — the function is the first parameter
+                if (node.path && node.path.original === 'fn' && node.params.length >= 1) {
+                    check(node, 0, 'The `fn` helper');
+                }
+            },
+
+            MustacheStatement(node) {
+                if (node.path && node.path.original === 'fn' && node.params.length >= 1) {
+                    check(node, 0, 'The `fn` helper');
+                }
+            },
+        };
+    }
+};

package.json

@@ -163,7 +163,7 @@
         "eslint-plugin-ember": "^11.11.1",
         "eslint-plugin-n": "^16.2.0",
         "eslint-plugin-prettier": "^5.0.1",
-        "eslint-plugin-qunit": "^8.0.1",
+        "eslint-plugin-qunit": "^8.2.6",
         "loader.js": "^4.7.0",
         "prettier": "^3.0.3",
         "qunit": "^2.20.0",

tests/integration/components/drawer-test.js

@@ -198,7 +198,7 @@ module('Integration | Component | drawer', function (hooks) {
             loaded[0].minimize();
             await settled();
 
-            assert.true(loaded[0].isMinimized === false, 'the snapshot is not live; callers must re-read via a fresh callback');
+            assert.false(loaded[0].isMinimized, 'the snapshot is not live; callers must re-read via a fresh callback');
             assert.dom(drawer()).hasClass('drawer-is-minimized', 'the drawer itself did update');
         });
 

tests/integration/components/dropdown-button-test.js

@@ -241,7 +241,7 @@ module('Integration | Component | dropdown-button', function (hooks) {
 
             await render(TEMPLATE);
 
-            assert.ok(find(`${TRIGGER} .ember-attacher`) || find(TRIGGER), 'the trigger renders with a tooltip attached');
+            assert.dom(TRIGGER).exists('the trigger renders when help text is permitted');
         });
 
         test('visibility and disabled state follow their arguments', async function (assert) {

tests/integration/components/layout/sidebar/navigator-test.js

@@ -325,24 +325,25 @@ module('Integration | Component | layout/sidebar/navigator', function (hooks) {
                 ],
             },
         ]);
+        const syncCalls = [];
         this.set('shouldSyncInitialActiveParent', ({ activePath, routeName, currentURL }) => {
-            if (routeName === 'console.settings.index') {
-                assert.deepEqual(
-                    activePath.map((item) => item.label),
-                    ['Settings', 'General']
-                );
-                assert.strictEqual(currentURL, '/settings');
-                return false;
-            }
+            syncCalls.push({ labels: activePath.map((item) => item.label), routeName, currentURL });
 
-            return true;
+            return routeName !== 'console.settings.index';
         });
 
         await render(hbs`<Layout::Sidebar::Navigator @items={{this.items}} @shouldSyncInitialActiveParent={{this.shouldSyncInitialActiveParent}} />`);
 
         assert.dom('.next-sidebar-navigator-back').doesNotExist('initial render stays at root when predicate returns false');
         assert.dom('.next-sidebar-navigator-view-in').includesText('Settings');
 
+        // Asserted outside the predicate: inside it, a change to the routeName would have skipped
+        // the branch and quietly asserted nothing.
+        const settingsCall = syncCalls.find((call) => call.routeName === 'console.settings.index');
+        assert.ok(settingsCall, 'the predicate is consulted for the active route');
+        assert.deepEqual(settingsCall.labels, ['Settings', 'General'], 'it receives the full active path');
+        assert.strictEqual(settingsCall.currentURL, '/settings', 'and the current url');
+
         const router = this.owner.lookup('service:router');
         router.currentRouteName = 'console.settings.security';
         router.currentURL = '/settings/security';

tests/integration/components/metadata-editor-test.js

@@ -200,7 +200,7 @@ module('Integration | Component | metadata-editor', function (hooks) {
         await render(TEMPLATE);
         await click(rows()[0].querySelector('input[type="checkbox"]'));
 
-        assert.strictEqual(changes[changes.length - 1].active, true);
+        assert.true(changes[changes.length - 1].active);
     });
 
     test('changing the type coerces the existing value', async function (assert) {
@@ -228,7 +228,7 @@ module('Integration | Component | metadata-editor', function (hooks) {
         await render(TEMPLATE);
         await fillIn(rows()[0].querySelector('select'), 'boolean');
 
-        assert.strictEqual(changes[changes.length - 1].thing, true);
+        assert.true(changes[changes.length - 1].thing);
     });
 
     test('boolean rows are omitted from the output when allowBoolean is false', async function (assert) {

tests/integration/components/metadata-viewer-test.js

@@ -55,9 +55,11 @@ module('Integration | Component | metadata-viewer', function (hooks) {
 
             const [tags, nested] = rows();
             assert.strictEqual(tags[0], 'tags');
-            assert.true(tags[1].includes('"a"') && tags[1].includes('"b"'), 'the array is shown as json');
+            assert.true(tags[1].includes('"a"'), 'the array is shown as json');
+            assert.true(tags[1].includes('"b"'), 'including every element');
             assert.strictEqual(nested[0], 'nested');
-            assert.true(nested[1].includes('"x"') && nested[1].includes('1'), 'so is the object');
+            assert.true(nested[1].includes('"x"'), 'so is the object');
+            assert.true(nested[1].includes('1'), 'including its value');
             assert.false(nested[1].includes('[object Object]'), 'nothing falls through to a raw stringification');
         });
 

tests/integration/components/money-input-test.js

@@ -130,7 +130,8 @@ module('Integration | Component | money-input', function (hooks) {
 
             await render(TEMPLATE);
 
-            assert.true(input().value.includes('£') || input().value.includes('25'), 'the amount is formatted for the currency');
+            assert.true(input().value.includes('25'), `the amount is present (got ${input().value})`);
+            assert.true(input().value.includes('£'), `and carries the currency symbol (got ${input().value})`);
         });
 
         test('a comma-decimal currency uses its own separator', async function (assert) {

tests/integration/components/pill-test.js

@@ -157,7 +157,8 @@ module('Integration | Component | pill', function (hooks) {
                 </Pill>
             `);
 
-            assert.ok(find('.tip') || find('.fleetbase-pill'), 'the pill renders with a tooltip attached');
+            assert.dom('.fleetbase-pill').exists('the pill renders');
+            assert.ok(find('.tip'), 'with a tooltip attached to it');
         });
     });
 

tests/integration/components/query-builder/computed-columns-test.js

@@ -126,7 +126,7 @@ module('Integration | Component | query-builder/computed-columns', function (hoo
 
             const badges = items().map((item) => item.querySelector('.rounded.text-xs').textContent.trim());
             assert.deepEqual(badges, ['Text', 'Integer', 'Decimal', 'Date', 'Date & Time', 'Boolean']);
-            assert.strictEqual(findAll('.computed-column-item svg').length >= 6, true, 'each row carries a type icon');
+            assert.true(findAll('.computed-column-item svg').length >= 6, 'each row carries a type icon');
         });
 
         test('an unrecognised type falls back to a question icon and no label', async function (assert) {