From ef4be50eed56fe5b9a23be7f39b4bd50eb8d44dc Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Sun, 9 Aug 2026 21:15:46 +0800 Subject: [PATCH] Fix five behavioural defects; two need a product decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Group C of the defect triage. Each fix is pinned by tests that could not be written before it — three of them were written during the coverage work, failed against the production code, and had to be deleted. - filter/multi-option: `search` now RETURNS its matches, as power-select expects. The remote path called `this.fetchOptions(...)`, a Task object rather than a function, and threw; the local path assigned `this.options` from inside a modifier update, which raised a backtracking assertion and permanently discarded every non-matching option, so clearing the query could not bring them back. The four tests deleted for this are restored, including one asserting that clearing the query restores the full list. - overlay: the resize clamps tested WIDTH whatever the position and returned before the horizontal/vertical fork, so a top/bottom overlay whose width fell outside [min, max] — which a full-width drawer always does — could never be resized, and each vertical drag silently rewrote its width to the clamp. Clamps the dimension being dragged now, with minResizeHeight/maxResizeHeight alongside the width pair. - modal: `@usesTransition('_fade')` named a getter, but the decorator reads `this.args[prop]`, so `this.args._fade` was always undefined and `@fade={{false}}` never disabled the transitions. Names the argument. - query-builder sort-by / group-by / conditions: the three `validate*` actions existed but nothing called them, so narrowing the selected columns left the panel sorting, grouping and filtering by columns that were no longer selected. Wired to `{{did-update}}` on the column list. - attach/popover: removed the inert `isOffset` flag. It was never assigned from an argument or anywhere else, and the method it guarded, `isCursorBetweenTargetAndAttachment`, does not exist on the component — so had anything ever set it, every mousemove would have thrown. Left for a product decision, unchanged here: model-select's infinite scroll (#105) needs the options to come from ember-infinity, which is feature work, not a repair; layout/resource/panel's redundant save path (#98) duplicates a button panel/header-actions already renders. 4983 tests pass, 0 skips. Coverage 92.44% statements / 87.97% branches / 95.29% functions / 92.89% lines. --- addon/components/attach/popover.js | 7 +- addon/components/filter/multi-option.js | 15 +++- addon/components/modal.js | 4 +- addon/components/overlay.js | 23 ++++-- addon/components/query-builder/conditions.hbs | 4 +- addon/components/query-builder/group-by.hbs | 4 +- addon/components/query-builder/sort-by.hbs | 4 +- .../components/filter/multi-option-test.js | 72 +++++++++++++++++++ tests/integration/components/modal-test.js | 14 ++++ tests/integration/components/overlay-test.js | 28 ++++++++ .../components/query-builder/sort-by-test.js | 36 +++++++++- 11 files changed, 195 insertions(+), 16 deletions(-) diff --git a/addon/components/attach/popover.js b/addon/components/attach/popover.js index 0bca26de..3ce4fc36 100644 --- a/addon/components/attach/popover.js +++ b/addon/components/attach/popover.js @@ -13,7 +13,6 @@ export default class AttachPopoverComponent extends Component { @tracked hideDuration = 300; @tracked hideOn = 'mouseleave blur escapekey'; @tracked interactive = false; - @tracked isOffset = false; @tracked isShown = false; @tracked lazyRender = false; @tracked modifiers = null; @@ -192,7 +191,11 @@ export default class AttachPopoverComponent extends Component { } // If cursor is not on the attachment or target, hide the popover - if (!target.contains(event.target) && !(this.isOffset && this.isCursorBetweenTargetAndAttachment(event)) && this.floatingElement && !this.floatingElement.contains(event.target)) { + // NOTE: this used to read `!(this.isOffset && this.isCursorBetweenTargetAndAttachment(event))`. + // `isOffset` was never assigned from an argument or anywhere else, so it was permanently + // false — and `isCursorBetweenTargetAndAttachment` does not exist on this component, so + // had anything ever set the flag the popover would have thrown on every mousemove. + if (!target.contains(event.target) && this.floatingElement && !this.floatingElement.contains(event.target)) { // Remove this listener before hiding the attachment delete this.hideListenersOnDocumentByEvent.mousemove; document.removeEventListener('mousemove', this.hideIfMouseOutsideTargetOrAttachment, this.useCapture); diff --git a/addon/components/filter/multi-option.js b/addon/components/filter/multi-option.js index 045f3570..faf4584f 100644 --- a/addon/components/filter/multi-option.js +++ b/addon/components/filter/multi-option.js @@ -38,14 +38,19 @@ export default class FilterMultiOptionComponent extends Component { } } + // power-select expects `@search` to RETURN the matches (or a promise of them). The previous + // version called `this.fetchOptions(...)` — a Task object, not a function, so the remote path + // threw — and on the local path assigned `this.options`, which both raised a backtracking + // assertion (it runs inside a modifier update) and permanently discarded every non-matching + // option, so clearing the query could not bring them back. @action search(query) { const { optionLabel, fetchUri, fetchParams = {} } = this.args; if (typeof fetchUri === 'string') { - return this.fetchOptions(fetchUri, { query, ...fetchParams }); + return this.fetchOptions.perform(fetchUri, { query, ...fetchParams }); } - this.options = this.options.filter((option) => { + return this.options.filter((option) => { const optionText = get(option, optionLabel ?? 'name') ?? option; if (typeof optionText === 'string') { @@ -65,9 +70,15 @@ export default class FilterMultiOptionComponent extends Component { try { const options = yield this.fetch.get(uri, queryParams); this.options = options; + + // Returned as well as stored: `search` hands this task's promise straight to + // power-select, which uses the resolved value as the result list. + return options; } catch (err) { debug('Error loading options: ' + err.message); } + + return []; } parseValue(value) { diff --git a/addon/components/modal.js b/addon/components/modal.js index 26f7d6ed..b482633e 100644 --- a/addon/components/modal.js +++ b/addon/components/modal.js @@ -289,7 +289,9 @@ export default class Modal extends Component { * @readonly * @private */ - @usesTransition('_fade') usesTransition; + // Must name the ARGUMENT: the decorator reads `this.args[prop]`, and `_fade` is a getter on + // the component, so `this.args._fade` was always undefined and `@fade={{false}}` was ignored. + @usesTransition('fade') usesTransition; destinationElement = getDestinationElement(this); diff --git a/addon/components/overlay.js b/addon/components/overlay.js index 8e76dca6..3fefb97e 100644 --- a/addon/components/overlay.js +++ b/addon/components/overlay.js @@ -189,16 +189,25 @@ export default class OverlayComponent extends Component { const height = dy * multiplier + this.overlayHeight; const minResizeWidth = this.args.minResizeWidth ?? 560; const maxResizeWidth = this.args.maxResizeWidth ?? 900; - - // Min resize width - if (width <= minResizeWidth) { - overlayPanelNode.style.width = `${minResizeWidth}px`; + const minResizeHeight = this.args.minResizeHeight ?? 0; + const maxResizeHeight = this.args.maxResizeHeight ?? Number.MAX_SAFE_INTEGER; + + // Clamp the dimension actually being dragged. These guards used to test the WIDTH whatever + // the position and `return` before the fork below, so a top/bottom overlay whose width sat + // outside [min, max] — which a full-width drawer always does — could never be resized at + // all, and each vertical drag silently rewrote its width to the clamp. + const size = isHorizontal ? width : height; + const minSize = isHorizontal ? minResizeWidth : minResizeHeight; + const maxSize = isHorizontal ? maxResizeWidth : maxResizeHeight; + const dimension = isHorizontal ? 'width' : 'height'; + + if (size <= minSize) { + overlayPanelNode.style[dimension] = `${minSize}px`; return; } - // Max resize width - if (width >= maxResizeWidth) { - overlayPanelNode.style.width = `${maxResizeWidth}px`; + if (size >= maxSize) { + overlayPanelNode.style[dimension] = `${maxSize}px`; return; } diff --git a/addon/components/query-builder/conditions.hbs b/addon/components/query-builder/conditions.hbs index 9b967693..3e1d2623 100644 --- a/addon/components/query-builder/conditions.hbs +++ b/addon/components/query-builder/conditions.hbs @@ -1,4 +1,6 @@ -
+{{! Re-validate when the caller narrows the column list: without this the panel keeps + sorting/grouping/filtering by columns that are no longer selected. }} +
diff --git a/addon/components/query-builder/group-by.hbs b/addon/components/query-builder/group-by.hbs index 3987e9ad..d394443f 100644 --- a/addon/components/query-builder/group-by.hbs +++ b/addon/components/query-builder/group-by.hbs @@ -1,4 +1,6 @@ -
+{{! Re-validate when the caller narrows the column list: without this the panel keeps + sorting/grouping/filtering by columns that are no longer selected. }} +
diff --git a/addon/components/query-builder/sort-by.hbs b/addon/components/query-builder/sort-by.hbs index 4db954b0..dbddca52 100644 --- a/addon/components/query-builder/sort-by.hbs +++ b/addon/components/query-builder/sort-by.hbs @@ -1,4 +1,6 @@ -
+{{! Re-validate when the caller narrows the column list: without this the panel keeps + sorting/grouping/filtering by columns that are no longer selected. }} +
diff --git a/tests/integration/components/filter/multi-option-test.js b/tests/integration/components/filter/multi-option-test.js index 0a2c0818..360d63d7 100644 --- a/tests/integration/components/filter/multi-option-test.js +++ b/tests/integration/components/filter/multi-option-test.js @@ -4,6 +4,7 @@ import { render, settled, findAll, find } from '@ember/test-helpers'; import { hbs } from 'ember-cli-htmlbars'; import Service from '@ember/service'; import { selectChoose, getDropdownItems } from 'ember-power-select/test-support'; +import { clickTrigger, typeInSearch } from 'ember-power-select/test-support/helpers'; const FILTER = { param: 'status', label: 'Status' }; const OPTIONS = [ @@ -253,4 +254,75 @@ module('Integration | Component | filter/multi-option', function (hooks) { assert.deepEqual(requests, []); }); }); + module('searching', function (hooks) { + // The search box only renders when the filter definition asks for it. + hooks.beforeEach(function () { + this.set('filter', { ...FILTER, multiOptionSearchEnabled: true }); + }); + + test('a local option list is narrowed by the query, and restored when it is cleared', async function (assert) { + this.set('options', OPTIONS); + + await render(TEMPLATE); + await clickTrigger(SCOPE); + await typeInSearch(SCOPE, 'act'); + + assert.deepEqual( + findAll('.ember-power-select-option').map((node) => node.textContent.trim()), + ['Active'], + 'only the matching option is offered' + ); + + await typeInSearch(SCOPE, ''); + + assert.deepEqual( + findAll('.ember-power-select-option').map((node) => node.textContent.trim()), + ['Active', 'Pending', 'Cancelled'], + 'clearing the query brings every option back rather than losing them for good' + ); + }); + + test('with no optionLabel the option itself is matched', async function (assert) { + this.set('options', ['active', 'pending']); + this.set('optionLabel', undefined); + + await render(TEMPLATE); + await clickTrigger(SCOPE); + await typeInSearch(SCOPE, 'pend'); + + assert.deepEqual( + findAll('.ember-power-select-option').map((node) => node.textContent.trim()), + ['pending'] + ); + }); + + test('an option with no text to match on is dropped rather than throwing', async function (assert) { + this.set('options', [{ name: 'Active', id: 'active' }, { id: 7 }]); + + await render(TEMPLATE); + await clickTrigger(SCOPE); + await typeInSearch(SCOPE, 'a'); + + assert.deepEqual( + findAll('.ember-power-select-option').map((node) => node.textContent.trim()), + ['Active'], + 'the option with no name is filtered out' + ); + }); + + test('with a fetch uri the query is sent to the server instead', async function (assert) { + this.set('fetchUri', 'statuses'); + this.set('fetchParams', { scope: 'orders' }); + + await render(TEMPLATE); + assert.strictEqual(requests.length, 1, 'the initial load'); + + await clickTrigger(SCOPE); + await typeInSearch(SCOPE, 'act'); + + assert.strictEqual(requests.length, 2, 'typing searches remotely rather than filtering locally'); + assert.strictEqual(requests[1].params.query, 'act', 'the query is forwarded'); + assert.strictEqual(requests[1].params.scope, 'orders', 'alongside the standing fetch params'); + }); + }); }); diff --git a/tests/integration/components/modal-test.js b/tests/integration/components/modal-test.js index 93bc777e..ccb20d50 100644 --- a/tests/integration/components/modal-test.js +++ b/tests/integration/components/modal-test.js @@ -230,4 +230,18 @@ module('Integration | Component | modal', function (hooks) { assert.strictEqual(dialog(), null, 'closing removes it again'); }); }); + // The decorator reads `this.args[prop]`, so it had been pointed at the `_fade` getter and + // `this.args._fade` was always undefined — @fade={{false}} never disabled anything. + test('@fade={{false}} renders without the fade class', async function (assert) { + await render(hbs``); + + assert.dom('.flb--modal-backdrop').doesNotHaveClass('fade', 'the backdrop is not faded in'); + assert.dom('.flb--modal-backdrop').hasClass('show', 'and is shown straight away'); + }); + + test('the backdrop fades by default', async function (assert) { + await render(hbs``); + + assert.dom('.flb--modal-backdrop').hasClass('fade'); + }); }); diff --git a/tests/integration/components/overlay-test.js b/tests/integration/components/overlay-test.js index 78feede1..a208359f 100644 --- a/tests/integration/components/overlay-test.js +++ b/tests/integration/components/overlay-test.js @@ -393,4 +393,32 @@ module('Integration | Component | overlay', function (hooks) { mouse('mouseup', document, { clientX: 700 }); }); }); + test('a bottom-positioned overlay resizes by height', async function (assert) { + await render(hbs``); + + const startingHeight = panel().getBoundingClientRect().height; + + // Dragging a bottom panel upward grows it: the multiplier is inverted. + mouse('mousedown', gutter(), { clientY: 0 }); + mouse('mousemove', document, { clientY: -700 }); + + assert.strictEqual(named('onResize').length, 1, 'the movement is reported'); + assert.strictEqual(panel().style.height, `${startingHeight + 700}px`, 'the drag distance is added to the starting height'); + assert.strictEqual(panel().style.width, '', 'and its width is left alone by a vertical drag'); + assert.strictEqual(document.body.style.cursor, 'row-resize'); + + mouse('mouseup', document, { clientY: -700 }); + }); + + test('a vertical resize clamps on height, not on width', async function (assert) { + await render(hbs``); + + mouse('mousedown', gutter(), { clientY: 0 }); + mouse('mousemove', document, { clientY: -5000 }); + + assert.strictEqual(panel().style.height, '400px', 'the height clamp applies'); + assert.strictEqual(panel().style.width, '', 'the width clamp does not fire on a vertical drag'); + + mouse('mouseup', document, { clientY: -5000 }); + }); }); diff --git a/tests/integration/components/query-builder/sort-by-test.js b/tests/integration/components/query-builder/sort-by-test.js index 1d9b627c..79dfba23 100644 --- a/tests/integration/components/query-builder/sort-by-test.js +++ b/tests/integration/components/query-builder/sort-by-test.js @@ -1,6 +1,6 @@ import { module, test } from 'qunit'; import { setupRenderingTest } from 'dummy/tests/helpers'; -import { render, click, findAll, triggerEvent } from '@ember/test-helpers'; +import { render, click, findAll, settled, triggerEvent } from '@ember/test-helpers'; import { hbs } from 'ember-cli-htmlbars'; import { selectChoose, getDropdownItems } from 'ember-power-select/test-support'; @@ -334,4 +334,38 @@ module('Integration | Component | query-builder/sort-by', function (hooks) { assert.dom('.query-builder-panel').hasAttribute('data-test-sort-by', 'yes'); }); + // Regression: `validateSortItems` existed but nothing called it, so narrowing the column list + // left the panel sorting by a column that was no longer selected. + async function addSortByLabel(label) { + await selectChoose(COLUMN_SELECT, label); + await click(addButton()); + } + + test('narrowing the selected columns drops a sort that no longer applies', async function (assert) { + await render(TEMPLATE); + await addSortByLabel('Status'); + await addSortByLabel('Total'); + + assert.strictEqual(sortItems().length, 2, 'both sorts are listed'); + + this.set('allSelectedColumns', [COLUMNS[1]]); + this.set('selectedColumns', [COLUMNS[1]]); + await settled(); + + assert.strictEqual(sortItems().length, 1, 'the sort on the removed column is dropped'); + assert.true(itemText(0).includes('Total'), 'the surviving sort is the one still selected'); + assert.strictEqual(changes[changes.length - 1].length, 1, 'and the change is reported'); + }); + + test('removing every column clears the sorting', async function (assert) { + await render(TEMPLATE); + await addSortByLabel('Status'); + + this.set('allSelectedColumns', []); + this.set('selectedColumns', []); + await settled(); + + assert.strictEqual(sortItems().length, 0); + assert.deepEqual(changes[changes.length - 1], [], 'the empty list is reported'); + }); });