From 4a6ee84872b2bc28461b6c51ef51bc68b8201fbd Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Tue, 11 Aug 2026 15:22:39 +0200 Subject: [PATCH 1/3] fix(quick-search): Enter opens the tile, and only non-matches dim (#95) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two shipped defects, one root cause: a WidgetPlacement id is an integer and a DOM attribute is a string. * `activateSearchResult()` called `.replace()` on the id. `Number.prototype.replace` does not exist, so every activation threw a TypeError inside a Vue event handler — where nothing surfaces it — and pressing Enter on a search result silently did nothing. The truthiness guard above it could not help: a non-zero integer is truthy. * `applySearchDimming()` compared `getAttribute('data-placement-id')` against the raw ids with `Array.includes`, which uses SameValueZero and does not coerce. `[7].includes('7')` is false, so EVERY tile dimmed on every query, including the matches the user was searching for. Why the unit suite was green throughout: all five existing fixtures in WorkspaceApp.spec.js seed a placement id as a STRING ('p1', 'match'), a type the API never sends. Re-running the same assertions with integer ids against the unfixed source fails twice, one of them with the TypeError verbatim. Those integer-id regressions are added here. The e2e side lands with the fix rather than before or after it: * REQ-QSEARCH-003 enter-opens-the-selected-tile now has a real test. It records the anchor's href from a capture-phase click listener with preventDefault, because the seeded links point at example.invalid and a real navigation would fail for a network reason instead of a product one. The two splitting probes from the investigation are kept, so a regression names its own cause. * The filtering test's dimming assertion was 'dimmed > 0', which 'every tile is dimmed' satisfies — it was passing for the wrong reason, and was disclosed as such rather than quietly tightened, because tightening it before the fix would simply have been red. It is now per-element and by identity: the non-matching tile carries the class and the matching one does not, with the aggregate count kept as a control so a refactor that stops applying the class at all cannot pass. No @e2e exclude was added while the feature was broken. Per .github#345 the gate reads an exclusion as positive coverage, so excluding a scenario the browser can plainly observe buys a green with a false statement. gate-19: 143 -> 142. Verified against a local NC fixture: 9/9 Playwright, 26/26 vitest. Closes #95 --- src/views/WorkspaceApp.vue | 34 ++++- src/views/__tests__/WorkspaceApp.spec.js | 91 ++++++++++++++ tests/e2e/tile-quick-search.spec.ts | 151 ++++++++++++++++++++--- 3 files changed, 258 insertions(+), 18 deletions(-) diff --git a/src/views/WorkspaceApp.vue b/src/views/WorkspaceApp.vue index bf177638..8c9f324d 100644 --- a/src/views/WorkspaceApp.vue +++ b/src/views/WorkspaceApp.vue @@ -522,13 +522,26 @@ export default { if (!items) { return } + /* + * A PLACEMENT ID IS A NUMBER; A DOM ATTRIBUTE IS A STRING. + * `matchIds` comes from `searchableTiles()`, which copies + * `placement.id` straight off the API row — an integer. The value + * read back out of a rendered cell is `getAttribute()`, which is + * always a string. `Array.prototype.includes` compares with + * SameValueZero, i.e. no coercion at all, so `[7].includes('7')` + * is `false` — EVERY tile was dimmed on every query, including + * the matches the user was looking for (launchpad#95). + * Normalising both sides to strings makes the comparison one + * between two values of the same type. + */ + const wanted = matchIds === null ? null : matchIds.map((id) => String(id)) items.forEach((el) => { - if (matchIds === null) { + if (wanted === null) { el.classList.remove('launchpad-grid-item--dimmed') return } const id = el.getAttribute('data-placement-id') - el.classList.toggle('launchpad-grid-item--dimmed', matchIds.includes(id) === false) + el.classList.toggle('launchpad-grid-item--dimmed', wanted.includes(id) === false) }) }, @@ -544,7 +557,22 @@ export default { * @return {void} */ activateSearchResult(item) { - const placementId = item?.placement?.id + /* + * `String(...)`, not the raw value. `placement.id` is an INTEGER + * off the API row, and the line below used to call + * `placementId.replace(...)` on it. `Number.prototype.replace` + * does not exist, so this threw a `TypeError` on every single + * activation — inside a Vue event handler, where nothing surfaces + * it, so pressing Enter on a search result silently did nothing + * (launchpad#95). The truthiness guard above did not catch it: a + * non-zero integer is truthy. + * + * `?? ''` rather than a bare cast so that `null`/`undefined` + * become the empty string and are rejected by the guard, instead + * of being stringified into the literal `"null"` and sent to + * `querySelector` as a real id to look for. + */ + const placementId = String(item?.placement?.id ?? '') if (!placementId || !this.$el) { return } diff --git a/src/views/__tests__/WorkspaceApp.spec.js b/src/views/__tests__/WorkspaceApp.spec.js index 00f57d3c..62a05768 100644 --- a/src/views/__tests__/WorkspaceApp.spec.js +++ b/src/views/__tests__/WorkspaceApp.spec.js @@ -334,6 +334,97 @@ describe('WorkspaceApp', () => { expect(clickSpy).toHaveBeenCalled() }) + /* + * launchpad#95 — TWO DEFECTS, ONE ROOT CAUSE, AND A FIXTURE THAT HID + * BOTH. + * + * Every test above seeds a placement id as a STRING (`'p1'`, + * `'match'`). `WidgetPlacement` rows arrive from + * `GET /api/dashboard/{id}` with an INTEGER `id` — the column is an + * auto-increment primary key — so no fixture in this file had ever + * exercised the type the product actually handles, and both defects + * below were invisible to a green suite: + * + * 1. `applySearchDimming()` compared `getAttribute()` (always a + * string) against the raw ids with `Array.includes`, which does + * not coerce. With integer ids nothing ever matched, so EVERY + * tile was dimmed — including the ones the user searched for. + * 2. `activateSearchResult()` called `.replace()` on the id. + * `Number.prototype.replace` does not exist, so Enter threw a + * `TypeError` inside a Vue event handler and silently did + * nothing. + * + * These two tests are the regression guard, and they are written + * against the production shape on purpose. Both are RED on the code + * as it stood before the fix: the first because `matchEl` is dimmed, + * the second because the call throws before reaching the link. + */ + it('onSearchFilter leaves an INTEGER-id match undimmed (launchpad#95)', () => { + const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) + const grid = wrapper.find('.workspace-shell__grid').element + const matchEl = document.createElement('div') + matchEl.className = 'launchpad-grid-item' + matchEl.setAttribute('data-placement-id', '7') + const otherEl = document.createElement('div') + otherEl.className = 'launchpad-grid-item' + otherEl.setAttribute('data-placement-id', '8') + grid.appendChild(matchEl) + grid.appendChild(otherEl) + + // The ids are numbers, exactly as `searchableTiles()` copies them + // off the API row — NOT the strings the older fixtures used. + wrapper.vm.onSearchFilter([7]) + + expect( + matchEl.classList.contains('launchpad-grid-item--dimmed'), + 'a matching tile must not be de-emphasised', + ).toBe(false) + expect( + otherEl.classList.contains('launchpad-grid-item--dimmed'), + 'CONTROL: a non-matching tile must still be de-emphasised, or the assertion above is satisfied by "nothing is ever dimmed"', + ).toBe(true) + }) + + it('onSearchOpen activates a tile whose placement id is an INTEGER (launchpad#95)', () => { + const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) + const grid = wrapper.find('.workspace-shell__grid').element + const el = document.createElement('div') + el.className = 'launchpad-grid-item' + el.setAttribute('data-placement-id', '7') + const link = document.createElement('a') + link.setAttribute('href', '#deck') + el.appendChild(link) + grid.appendChild(el) + + const scrollSpy = vi.fn() + el.scrollIntoView = scrollSpy + const clickSpy = vi.spyOn(link, 'click') + + wrapper.vm.onSearchOpen({ id: 7, label: 'Deck', placement: { id: 7 } }) + + expect(scrollSpy, 'the matched tile must be scrolled into view').toHaveBeenCalled() + expect(clickSpy, 'Enter must activate the tile\'s rendered link').toHaveBeenCalled() + }) + + it('onSearchOpen ignores a placement with no id rather than looking for the string "null"', () => { + const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) + const grid = wrapper.find('.workspace-shell__grid').element + const el = document.createElement('div') + el.className = 'launchpad-grid-item' + // A cell literally labelled "null" — the shape a bare String() + // cast would go looking for, and find. + el.setAttribute('data-placement-id', 'null') + const link = document.createElement('a') + link.setAttribute('href', '#deck') + el.appendChild(link) + grid.appendChild(el) + const clickSpy = vi.spyOn(link, 'click') + + wrapper.vm.onSearchOpen({ id: null, label: 'Deck', placement: { id: null } }) + + expect(clickSpy, 'a null id must not activate the tile that happens to be called "null"').not.toHaveBeenCalled() + }) + it('onSearchFallback opens a web-search URL in a new tab', () => { const wrapper = mountShell({ inject: { activeDashboardId: 'd1' } }) const openSpy = vi.spyOn(window, 'open').mockImplementation(() => {}) diff --git a/tests/e2e/tile-quick-search.spec.ts b/tests/e2e/tile-quick-search.spec.ts index 944829d2..f5881461 100644 --- a/tests/e2e/tile-quick-search.spec.ts +++ b/tests/e2e/tile-quick-search.spec.ts @@ -373,9 +373,49 @@ test.describe('tile quick-search — filtering (REQ-QSEARCH-002)', () => { 'non-matching tiles must be de-emphasised, not removed from the grid layout', ).toBe(gridCountBefore) + /* + * THIS ASSERTION USED TO BE `dimmed > 0`, AND THAT PASSED FOR THE + * WRONG REASON. + * + * "Every single tile is dimmed" satisfies `> 0`, and that is exactly + * what the app did: `applySearchDimming()` compared a string + * `getAttribute('data-placement-id')` against numeric ids with + * `Array.includes`, which does not coerce, so nothing ever matched + * and the matches were de-emphasised alongside everything else + * (launchpad#95). The requirement is not "something is dimmed", it is + * that the dimming DISTINGUISHES matches from non-matches — and a + * count cannot say that. + * + * So it is now asserted per element, by identity. This is red on the + * unfixed code (the two matching cells carry the class), which is why + * it lands with the fix rather than before or after it. + */ + // `.first()` on both: `evaluate()` throws on a locator that resolves to + // more than one node, and a strictness error would read as a product + // failure rather than as the selector problem it is. + const matching = page.locator(GRID_ITEM).filter({ hasText: `Zaaksysteem ${STAMP}` }).first() + const notMatching = page.locator(GRID_ITEM).filter({ hasText: `Verlof aanvragen ${STAMP}` }).first() + await expect(matching, 'the matching tile must be on screen').toBeVisible({ timeout: 15_000 }) + await expect(notMatching, 'the non-matching tile must be on screen').toBeVisible({ timeout: 15_000 }) + + await expect + .poll(async () => notMatching.evaluate(el => el.classList.contains('launchpad-grid-item--dimmed')), { + message: 'a tile whose label does not match the query must be visibly de-emphasised', + timeout: 15_000, + }) + .toBe(true) + + expect( + await matching.evaluate(el => el.classList.contains('launchpad-grid-item--dimmed')), + 'a MATCHING tile must not be de-emphasised — dimming everything satisfies a bare "something is dimmed" count while telling the user nothing', + ).toBe(false) + + // And the aggregate still has to move, so a future refactor that stops + // applying the class at all cannot pass the two checks above by making + // `classList.contains` false everywhere. await expect .poll(async () => page.locator(DIMMED).count(), { - message: 'the tiles that do not match must be visibly de-emphasised', + message: 'CONTROL: the class must actually be applied somewhere', timeout: 15_000, }) .toBeGreaterThan(0) @@ -503,8 +543,81 @@ test.describe('tile quick-search — keyboard navigation (REQ-QSEARCH-003)', () }) /* - * REQ-QSEARCH-003 "Enter opens the selected tile" HAS NO TEST HERE, AND - * THAT IS DELIBERATE — the product is broken. See launchpad#95. + * REQ-QSEARCH-003 "Enter opens the selected tile" — NOW COVERED. The + * defect described in the block below is fixed in this same change; the + * history is kept because the way it hid is the interesting part. + * + * The activation is proven by recording the navigation the tile's own + * anchor performs, NOT by waiting for a page load. Seeded tiles link to + * `https://example.invalid/...`, which is unresolvable on purpose — a + * real navigation there would hang the test for its whole timeout and + * then fail for a network reason. So `click` is intercepted at the + * document level and the anchor's `href` recorded, with `preventDefault` + * to stop the browser acting on it. That records exactly what + * `activateSearchResult()` did: which anchor it clicked, or none. + */ + // @e2e tile-quick-search::enter-opens-the-selected-tile + test('Enter activates the selected result\'s tile link (launchpad#95)', async ({ page }) => { + await openWorkspace(page) + + await page.evaluate(() => { + (window as unknown as { __lpActivations: string[] }).__lpActivations = [] + document.addEventListener('click', (event) => { + const anchor = (event.target as HTMLElement | null)?.closest?.('a[href]') + if (anchor) { + event.preventDefault(); + (window as unknown as { __lpActivations: string[] }).__lpActivations + .push(anchor.getAttribute('href') ?? '') + } + }, true) + }) + + const input = page.locator(INPUT) + await input.fill('Zaaksysteem') + + // Exactly one match, so the selected option is unambiguous and the + // assertion below cannot be satisfied by the wrong tile. + await expect + .poll(async () => optionLabels(page), { message: 'the query must resolve to exactly one tile', timeout: 15_000 }) + .toEqual([`Zaaksysteem ${STAMP}`]) + + /* + * SPLITTING PROBE, kept from the investigation. Both of these passed + * while Enter still did nothing, which is what narrowed the cause to + * `activateSearchResult()` itself rather than to the harness: the + * anchor exists, and the input holds focus at the moment Enter is + * pressed. If either regresses, the failure names its own cause + * instead of being read as "the fix was reverted". + */ + await expect( + page.locator(GRID_ITEM).filter({ hasText: `Zaaksysteem ${STAMP}` }).first().locator('a[href]').first(), + 'PROBE: the rendered tile must carry an anchor, or activation has nothing to click', + ).toHaveCount(1) + await expect(input, 'PROBE: the search input must still hold focus when Enter is pressed').toBeFocused() + + await page.keyboard.press('Enter') + + const readActivations = async () => page.evaluate( + () => (window as unknown as { __lpActivations: string[] }).__lpActivations, + ) + + await expect + .poll(readActivations, { + message: 'Enter must activate the selected tile\'s link — an empty list is the launchpad#95 symptom ' + + '(a TypeError thrown inside a Vue event handler, where nothing surfaces it)', + timeout: 15_000, + }) + .not.toHaveLength(0) + + const activations = await readActivations() + expect( + activations.join(' | '), + 'the activated link must be the SELECTED tile\'s, not just any anchor on the page', + ).toContain(encodeURIComponent(`Zaaksysteem ${STAMP}`)) + }) + + /* + * HOW launchpad#95 HID, kept because the shape recurs. * * The test existed, ran in CI, and failed with an empty activation list. * Rather than adjust the harness a second time, two splitting probes were @@ -528,19 +641,27 @@ test.describe('tile quick-search — keyboard navigation (REQ-QSEARCH-003)', () * help — a non-zero integer is truthy — and the throw happens inside a * Vue event handler, so nothing surfaces and Enter simply does nothing. * - * NO `@e2e exclude` IS ADDED FOR THIS SCENARIO. The scenario is - * browser-observable; the reason it has no passing test is that the - * feature is broken. An exclusion would record "a browser cannot see - * this", which is false. The gate-19 finding stays open against #95. + * NO `@e2e exclude` WAS ADDED WHILE IT WAS BROKEN, and that is the part + * worth carrying elsewhere. The scenario was always browser-observable; + * the reason it had no passing test was that the feature did not work. + * An exclusion would have recorded "a browser cannot see this", which was + * false — and per `.github#345` the gate scores an exclusion as POSITIVE + * coverage, so it would have bought a green with a false statement. + * + * Second defect, same root cause, also #95: the filtering test's dimming + * assertion required only `dimmed > 0`, which "EVERY tile is dimmed" + * satisfies — and that is what `applySearchDimming()` did, comparing a + * string `getAttribute('data-placement-id')` against numeric ids with + * `Array.includes`. The assertion was disclosed as passing for the wrong + * reason rather than quietly tightened, because tightening it before the + * fix would simply have been red. It is now per-element and by identity, + * and it lands in the same change as the fix. * - * Related, from the same root cause and also filed in #95: the dimming - * assertion in the filtering test above requires only `dimmed > 0`, and - * that is satisfied by "EVERY tile is dimmed" — which is what - * `applySearchDimming()` actually does, because it compares a string - * `getAttribute('data-placement-id')` against numeric ids with - * `Array.includes`, so no tile ever matches. Tightening that assertion - * to "matches are NOT dimmed" belongs with the fix, since it would be - * red on current `development`. + * WHY THE UNIT TESTS DID NOT CATCH EITHER ONE: every fixture in + * `src/views/__tests__/WorkspaceApp.spec.js` seeded a placement id as a + * STRING (`'p1'`), and the API sends an integer. `Array.includes` and + * `Number.prototype` are both type-exact, so a string fixture made both + * defects invisible. That file now carries integer-id regressions. */ // @e2e tile-quick-search::escape-clears-and-returns-focus From c25ddf48cc52c68650ca6f146819c98869b69526 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Tue, 11 Aug 2026 16:57:48 +0200 Subject: [PATCH 2/3] test(e2e): cover 14 conditional-visibility-editor scenarios, and find three defects doing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gate-19: 142 -> 128, all of it real tests. Zero @e2e exclusions added, and no spec file touched. Negative control, same tree, one command apart: with the file 128 file removed 142 (+14, exactly its anchor count, all in this capability) restored 128 Verified against a local NC 34 + PostgreSQL 16 fixture: 13/13 Playwright. PostgreSQL matters here — it is the shared workflow's default database, and two of the three defects below are invisible on SQLite and MySQL. WHAT IS COVERED (14 of the capability's 23 scenarios) REQ-CVUI-001 the section loads and renders stored rules; edit sends PUT /api/rules/{id} with the updated ruleConfig; remove sends DELETE and the row goes. REQ-CVUI-002 time / date / attribute operands, asserted on the REQUEST BODY rather than on the re-rendered row — the requirement is about the canonical shape on the wire, and a purely local component could fake the row. Includes the open-ended date range, where the point is that `endDate` is ABSENT, not empty. REQ-CVUI-003 the include/exclude distinction survives every colour in the document being overridden to one value; the empty state, with a control proving it disappears when a rule exists. REQ-CVUI-004/5 preview evaluates an unsaved row and persists nothing (with a positive control, because "no rules stored" is what a broken editor also produces); preview and save send byte-identical ruleConfig; an unknown ruleType is refused 400 while a valid one is accepted; an anonymous caller is refused while the same request with credentials succeeds. THREE DEFECTS FOUND, ALL FILED, NONE PAPERED OVER #96 No EXCLUDE rule can be created at all. `isInclude: false` is a boolean bound into a `smallint` column, so POST and PUT both answer 400 on PostgreSQL. The controller reports it via ResponseHelper::error() WITHOUT a logger, so nothing reaches nextcloud.log either. #97 The group picker is always empty — Views.vue never passes :available-groups — so group rules cannot be authored or previewed. #98 Conditional visibility is NEVER ENFORCED. checkRulesForPlacement() has one production caller and it is the editor's own read; isWidgetVisible() does not exist. Preview says Hidden and the dashboard renders the widget anyway. Proven three ways: browser, a 200 from GET /api/dashboard/{id} still carrying the placement at isVisible:1, and a caller search taken with a positive control. NINE SCENARIOS LEFT UNCOVERED AND DELIBERATELY NOT EXCLUDED Every one of them is browser-observable; what a browser observes is that the feature does not work. Per .github#345 the gate scores an `@e2e exclude` as POSITIVE coverage, so excluding them would have bought nine findings with a false statement. Each is recorded in the file where its test would sit, with the evidence, so the next person does not rediscover it. `includeexclude-toggle` IS covered, narrowly: that requirement is about what the row EMITS and where it MOVES, both observable before the request leaves the browser. It deliberately does not assert persistence, which would be asserting #96 is fixed. TWO TRAPS WORTH CARRYING ELSEWHERE, both recorded in the file * NcCheckboxRadioSwitch has NO