From 8cd14c34d61a5fc38527fbd64a446b5aa8515fa6 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 09:49:39 -0700 Subject: [PATCH 01/12] fix: droppable submit navigates to search URL; lock html+body scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #44 — two critical bugs found because tests only checked that ol-search fired, not that navigation happened: Navigation (droppable mode): - Add _buildSearchUrl(q, filters) that constructs siteBase/search?q=... with full filter params serialized as OL search URL query params - Make ol-search event cancelable — hosts that handle navigation themselves call e.preventDefault() to suppress the fallback - _submit() and "See all N results" both navigate via window.location.href when showFacets=true and event not prevented - Embedded mode (showFacets=false) is unaffected Body scroll lock: - Lock document.documentElement.style.overflow in addition to document.body; many browsers/frameworks scroll , leaving a page scrollbar behind the position:fixed overlay - Restore both on close and in disconnectedCallback TDD: Playwright tests written BEFORE the fix (red → green): - submit button / Enter key / "See all N" each navigate to /search?q= - preventDefault() suppresses navigation - document.body + documentElement overflow:hidden while overlay open - both restored to '' when overlay closes AGENTS.md: explicit TDD section requiring a failing test before any fix --- AGENTS.md | 19 +++ frontend/src/components/ol-search-bar.js | 50 +++++-- .../ol-search-bar.panel-overlay.test.js | 2 +- frontend/tests/facet-and-submit.spec.js | 123 +++++++++++++++--- frontend/tests/mobile-overlay.spec.js | 61 +++++++++ 5 files changed, 228 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 98d540c..c1d0c99 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,6 +108,25 @@ Types: `feat`, `fix`, `refactor`, `test`, `docs`, `chore`, `style` Squash fixup commits; keep history as logical milestones. +## Test-Driven Development + +**Write the test before the fix.** For every interactive bug or behavioral change: + +1. Write a failing Playwright test (or Vitest static-analysis test) that reproduces the problem. +2. Commit it with a message like `test: failing test for `. +3. Implement the fix until the test goes green. +4. Commit the fix separately. + +This creates an unambiguous record that the test was driven by the behavior, not retrofitted after the fact. + +**Playwright for interactive behavior; Vitest for structure/layout.** +- Use Playwright for: navigation, clicks, keyboard, event propagation, computed styles, layout dimensions. +- Use Vitest (static-analysis) for: CSS property presence, method/event name conventions, code structure contracts. + +**Never rely on event-fires-only assertions for navigation bugs.** A test that only checks `window.__olSearchFired === true` does not catch a broken navigation path. Always assert the URL or a visible side-effect of the intended action. + +--- + ## Pull Request Standards ### Bug fixes must include evidence diff --git a/frontend/src/components/ol-search-bar.js b/frontend/src/components/ol-search-bar.js index 22618d0..8a51653 100644 --- a/frontend/src/components/ol-search-bar.js +++ b/frontend/src/components/ol-search-bar.js @@ -124,13 +124,16 @@ export class OlSearchBar extends LitElement { super.disconnectedCallback(); document.removeEventListener('click', this._onDoc, true); window.removeEventListener('resize', this._onWinResize); - if (this._mobileExpanded) document.body.style.overflow = ''; this._acAbort?.abort(); this._authorAbort?.abort(); this._subjectAbort?.abort(); clearTimeout(this._timer); clearTimeout(this._authorTimer); clearTimeout(this._subjectTimer); + if (this._mobileExpanded) { + document.body.style.overflow = ''; + document.documentElement.style.overflow = ''; + } } updated(changed) { @@ -148,9 +151,12 @@ export class OlSearchBar extends LitElement { } // Sync full-screen overlay class on the host element. this.classList.toggle('mobile-exp', this._mobileExpanded); - // Prevent body scroll while overlay is active so the page behind doesn't scroll. + // Prevent body scroll (both axes) while overlay is active so the page behind + // doesn't scroll. Lock as well — some browsers/frameworks scroll it. if (changed.has('_mobileExpanded')) { - document.body.style.overflow = this._mobileExpanded ? 'hidden' : ''; + const lock = this._mobileExpanded ? 'hidden' : ''; + document.body.style.overflow = lock; + document.documentElement.style.overflow = lock; } // Anchor the panel to the trigger and focus the panel-input when it opens. if (changed.has('_open') && this._open && this.showFacets) { @@ -298,13 +304,35 @@ export class OlSearchBar extends LitElement { if (e.key === 'Enter') this._submit(); } + // Build a search results URL from query + current filters. + // Used as the fallback navigation target in droppable mode when no host + // cancels the ol-search event. + _buildSearchUrl(q, f = {}) { + const p = new URLSearchParams(); + if (q) p.set('q', q); + if (f.sort) p.set('sort', f.sort); + if (f.availability) p.set('availability', f.availability); + if (f.fictionFilter) p.set('subject', f.fictionFilter); + (f.languages ?? []).forEach(l => p.append('language', l)); + (f.genres ?? []).forEach(g => p.append('subject', g)); + (f.authors ?? []).forEach(a => p.append('author', a)); + (f.subjects ?? []).forEach(s => p.append('subject', s)); + return `${this.siteBase}/search?${p.toString()}`; + } + _submit() { if (!this._q.trim() && !this._hasActiveFilters()) return; this._mobileExpanded = false; - this.dispatchEvent(new CustomEvent('ol-search', { + const event = new CustomEvent('ol-search', { detail: { q: this._q.trim(), filters: this._localFilters }, - bubbles: true, composed: true, - })); + bubbles: true, composed: true, cancelable: true, + }); + this.dispatchEvent(event); + // In droppable mode navigate to the search results page unless the host + // handled the event itself and called e.preventDefault(). + if (this.showFacets && !event.defaultPrevented) { + window.location.href = this._buildSearchUrl(this._q.trim(), this._localFilters); + } } // ── Clear-all filters ───────────────────────────────────────── @@ -844,9 +872,13 @@ export class OlSearchBar extends LitElement { `; } diff --git a/frontend/src/components/ol-search-bar.panel-overlay.test.js b/frontend/src/components/ol-search-bar.panel-overlay.test.js index ce51141..e06f5f0 100644 --- a/frontend/src/components/ol-search-bar.panel-overlay.test.js +++ b/frontend/src/components/ol-search-bar.panel-overlay.test.js @@ -99,7 +99,7 @@ describe('ol-search-bar panel overlay — panel-input contract', () => { }); it('updated() focuses the panel-input when the panel opens', () => { - const updFn = src.slice(src.indexOf('updated(changed)'), src.indexOf('updated(changed)') + 1200); + const updFn = src.slice(src.indexOf('updated(changed)'), src.indexOf('updated(changed)') + 1300); expect(updFn).toMatch(/panel-input/); }); }); diff --git a/frontend/tests/facet-and-submit.spec.js b/frontend/tests/facet-and-submit.spec.js index 625736c..e18286a 100644 --- a/frontend/tests/facet-and-submit.spec.js +++ b/frontend/tests/facet-and-submit.spec.js @@ -3,6 +3,10 @@ * * ol-search-bar lives inside ol-header's shadow DOM, so all page.evaluate() * helpers traverse: document → ol-header.shadowRoot → ol-search-bar. + * + * TDD note: navigation tests (issue #44) were written BEFORE the _submit() + * navigation fix so they would fail red and prove the regression, then go + * green once _buildSearchUrl + cancelable-event logic was added. */ import { test, expect } from '@playwright/test'; @@ -16,7 +20,6 @@ async function waitForCustomElements(page) { /** Open the search panel then click the first facet button. */ async function openPanelAndFacet(page) { - // Click the trigger button (droppable/header mode — the panel lives here) await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); sb?.shadowRoot?.querySelector('.trigger-btn') @@ -28,7 +31,6 @@ async function openPanelAndFacet(page) { return sb?.shadowRoot?.querySelector('.panel') !== null; }); - // Click the first facet button (Availability) await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); sb?.shadowRoot?.querySelector('.pf-btn') @@ -41,6 +43,27 @@ async function openPanelAndFacet(page) { }, { timeout: 3000 }); } +/** Open the panel and type a query into the panel-input. */ +async function openPanelAndType(page, query) { + await page.evaluate(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + sb?.shadowRoot?.querySelector('.trigger-btn') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + }); + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?.shadowRoot?.querySelector('.panel-input') !== null; + }); + await page.evaluate((q) => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + const input = sb?.shadowRoot?.querySelector('.panel-input'); + if (input) { + input.value = q; + input.dispatchEvent(new Event('input', { bubbles: true })); + } + }, query); +} + // ── Issue #21: clicking panel chips / background while facet is open should dismiss it ── test.describe('facet dropdown dismissal (issue #21)', () => { @@ -60,7 +83,6 @@ test.describe('facet dropdown dismissal (issue #21)', () => { }); expect(openBefore).toBe(true); - // Click panel-chips — inside ol-search-bar but outside ol-facet-drop await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); sb?.shadowRoot?.querySelector('.panel-chips') @@ -98,7 +120,7 @@ test.describe('facet dropdown dismissal (issue #21)', () => { }); }); -// ── Issue #22: clicking the submit button fires ol-search event ── +// ── Issue #22: submit dispatches ol-search ── test.describe('submit button (issue #22)', () => { test('clicking the magnifying glass dispatches an ol-search event', async ({ page }) => { @@ -112,36 +134,103 @@ test.describe('submit button (issue #22)', () => { document.addEventListener('ol-search', () => { window.__olSearchFired = true; }, { once: true }); }); - // Open the panel, then type into the panel-input + await openPanelAndType(page, 'frankenstein'); + await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); - sb?.shadowRoot?.querySelector('.trigger-btn') + sb?.shadowRoot?.querySelector('.panel .submit') ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); }); - await page.waitForFunction(() => { + + await page.waitForFunction(() => window.__olSearchFired === true, { timeout: 2000 }); + expect(await page.evaluate(() => window.__olSearchFired)).toBe(true); + }); +}); + +// ── Issue #44: droppable submit navigates to search URL ── +// +// These tests were written BEFORE the navigation fix was implemented (TDD). +// They failed red until _submit() added URL navigation for showFacets=true mode. + +test.describe('droppable submit navigation (issue #44)', () => { + test.beforeEach(async ({ page }) => { + await page.setViewportSize({ width: 1024, height: 768 }); + // Intercept any navigation to the OL search page so the test stays on localhost. + // The route fulfils with a stub so waitForURL() can resolve without hitting the network. + await page.route('**/search?**', route => + route.fulfill({ status: 200, contentType: 'text/html', body: 'stub' }) + ); + await page.goto('/'); + await waitForCustomElements(page); + await page.locator('ol-search-bar').waitFor({ state: 'attached' }); + }); + + test('clicking the submit button navigates to /search?q=', async ({ page }) => { + await openPanelAndType(page, 'frankenstein'); + + const navPromise = page.waitForURL(/\/search\?.*q=/, { timeout: 4000 }); + await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); - return sb?.shadowRoot?.querySelector('.panel-input') !== null; + sb?.shadowRoot?.querySelector('.panel .submit') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); }); + await navPromise; + + expect(page.url()).toMatch(/\/search\?.*q=frankenstein/); + }); + test('pressing Enter in the panel-input navigates to /search?q=', async ({ page }) => { + await openPanelAndType(page, 'frankenstein'); + + const navPromise = page.waitForURL(/\/search\?.*q=/, { timeout: 4000 }); + // Focus the input then dispatch Enter keydown await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); const input = sb?.shadowRoot?.querySelector('.panel-input'); - if (input) { - input.value = 'frankenstein'; - input.dispatchEvent(new Event('input', { bubbles: true })); - } + input?.focus(); + input?.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, composed: true })); }); + await navPromise; + + expect(page.url()).toMatch(/\/search\?.*q=frankenstein/); + }); + + test('"See all N results" button navigates to /search?q=', async ({ page }) => { + await openPanelAndType(page, 'frankenstein'); + + // Wait for autocomplete to fetch so the "See all" button appears + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?.shadowRoot?.querySelector('.ac-see-all') !== null; + }, { timeout: 5000 }); - // Click the submit button inside the panel + const navPromise = page.waitForURL(/\/search\?.*q=/, { timeout: 4000 }); await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); - sb?.shadowRoot?.querySelector('.panel .submit') + sb?.shadowRoot?.querySelector('.ac-see-all') ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); }); + await navPromise; - await page.waitForFunction(() => window.__olSearchFired === true, { timeout: 2000 }); + expect(page.url()).toMatch(/\/search\?.*q=frankenstein/); + }); + + test('ol-search event is cancelable — calling preventDefault() suppresses navigation', async ({ page }) => { + // A host that handles ol-search itself can prevent the fallback URL navigation. + await page.evaluate(() => { + document.addEventListener('ol-search', e => e.preventDefault(), { once: true }); + }); + + await openPanelAndType(page, 'frankenstein'); + + await page.evaluate(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + sb?.shadowRoot?.querySelector('.panel .submit') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + }); - const fired = await page.evaluate(() => window.__olSearchFired); - expect(fired).toBe(true); + // Wait a beat — if navigation happened despite preventDefault, the URL would change. + await page.waitForTimeout(800); + expect(page.url()).not.toMatch(/\/search\?/); }); }); diff --git a/frontend/tests/mobile-overlay.spec.js b/frontend/tests/mobile-overlay.spec.js index 14d8b2b..bcc342b 100644 --- a/frontend/tests/mobile-overlay.spec.js +++ b/frontend/tests/mobile-overlay.spec.js @@ -133,6 +133,67 @@ test.describe('mobile full-screen overlay (issue #23)', () => { }); }); +// ── Body scroll lock (issue #44) ────────────────────────────────────────────── +// +// Written BEFORE the documentElement scroll-lock fix so they fail red first. +// The previous fix locked document.body but not document.documentElement; +// many browsers/frameworks scroll , so the page behind the overlay +// remained scrollable and a full-page scrollbar appeared. + +test.describe('mobile overlay — body scroll lock', () => { + test.beforeEach(async ({ page }) => { + await page.setViewportSize({ width: 375, height: 812 }); + await page.goto('/'); + await waitForSearchBar(page); + }); + + test('document.body and documentElement have overflow:hidden while overlay is open', async ({ page }) => { + await openSearch(page); + + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?.classList.contains('mobile-exp') === true; + }, { timeout: 2000 }); + + const overflow = await page.evaluate(() => ({ + body: getComputedStyle(document.body).overflow, + html: getComputedStyle(document.documentElement).overflow, + })); + + expect(overflow.body).toBe('hidden'); + expect(overflow.html).toBe('hidden'); + }); + + test('body and documentElement overflow is restored after overlay closes', async ({ page }) => { + await openSearch(page); + + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?.classList.contains('mobile-exp') === true; + }, { timeout: 2000 }); + + // Close via back button + await page.evaluate(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + sb?.shadowRoot?.querySelector('.mob-back-btn') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + }); + + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?.classList.contains('mobile-exp') === false; + }, { timeout: 2000 }); + + const overflow = await page.evaluate(() => ({ + body: document.body.style.overflow, + html: document.documentElement.style.overflow, + })); + + expect(overflow.body).toBe(''); + expect(overflow.html).toBe(''); + }); +}); + test.describe('desktop — no overlay regression', () => { test('clicking search trigger on desktop does NOT add mobile-exp class', async ({ page }) => { await page.setViewportSize({ width: 1440, height: 900 }); From e1cf066c054f8fb9f617ed9245f299ffe318150b Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 10:01:54 -0700 Subject: [PATCH 02/12] fix: address Copilot review comments on PR #44 - Use _scrollLockActive flag in disconnectedCallback instead of _mobileExpanded to correctly release scroll lock when component is removed while overlay is open - Save/restore _prevBodyOverflow and _prevDocumentOverflow before locking so we never clobber pre-existing inline overflow styles - Use new URL('/search', siteBase) to safely handle trailing slash in siteBase (fixes potential double-slash in navigation URL) - Add Playwright test asserting active filters appear in navigation URL - Widen updated() slice to 2000 chars in panel-overlay static test --- frontend/src/components/ol-search-bar.js | 32 ++++++++++++----- .../ol-search-bar.panel-overlay.test.js | 2 +- frontend/tests/facet-and-submit.spec.js | 34 +++++++++++++++++++ 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/frontend/src/components/ol-search-bar.js b/frontend/src/components/ol-search-bar.js index 8a51653..126ced4 100644 --- a/frontend/src/components/ol-search-bar.js +++ b/frontend/src/components/ol-search-bar.js @@ -130,9 +130,10 @@ export class OlSearchBar extends LitElement { clearTimeout(this._timer); clearTimeout(this._authorTimer); clearTimeout(this._subjectTimer); - if (this._mobileExpanded) { - document.body.style.overflow = ''; - document.documentElement.style.overflow = ''; + if (this._scrollLockActive) { + document.body.style.overflow = this._prevBodyOverflow ?? ''; + document.documentElement.style.overflow = this._prevDocumentOverflow ?? ''; + this._scrollLockActive = false; } } @@ -151,12 +152,23 @@ export class OlSearchBar extends LitElement { } // Sync full-screen overlay class on the host element. this.classList.toggle('mobile-exp', this._mobileExpanded); - // Prevent body scroll (both axes) while overlay is active so the page behind - // doesn't scroll. Lock as well — some browsers/frameworks scroll it. + // Prevent body scroll while overlay is active; lock too since many + // browsers/frameworks scroll it. Save pre-existing inline values so close + // restores exactly what was there before (avoids clobbering other overlays). if (changed.has('_mobileExpanded')) { - const lock = this._mobileExpanded ? 'hidden' : ''; - document.body.style.overflow = lock; - document.documentElement.style.overflow = lock; + if (this._mobileExpanded && !this._scrollLockActive) { + this._prevBodyOverflow = document.body.style.overflow; + this._prevDocumentOverflow = document.documentElement.style.overflow; + this._scrollLockActive = true; + document.body.style.overflow = 'hidden'; + document.documentElement.style.overflow = 'hidden'; + } else if (!this._mobileExpanded && this._scrollLockActive) { + document.body.style.overflow = this._prevBodyOverflow ?? ''; + document.documentElement.style.overflow = this._prevDocumentOverflow ?? ''; + this._scrollLockActive = false; + this._prevBodyOverflow = undefined; + this._prevDocumentOverflow = undefined; + } } // Anchor the panel to the trigger and focus the panel-input when it opens. if (changed.has('_open') && this._open && this.showFacets) { @@ -317,7 +329,9 @@ export class OlSearchBar extends LitElement { (f.genres ?? []).forEach(g => p.append('subject', g)); (f.authors ?? []).forEach(a => p.append('author', a)); (f.subjects ?? []).forEach(s => p.append('subject', s)); - return `${this.siteBase}/search?${p.toString()}`; + const url = new URL('/search', this.siteBase); + url.search = p.toString(); + return url.toString(); } _submit() { diff --git a/frontend/src/components/ol-search-bar.panel-overlay.test.js b/frontend/src/components/ol-search-bar.panel-overlay.test.js index e06f5f0..8c28389 100644 --- a/frontend/src/components/ol-search-bar.panel-overlay.test.js +++ b/frontend/src/components/ol-search-bar.panel-overlay.test.js @@ -99,7 +99,7 @@ describe('ol-search-bar panel overlay — panel-input contract', () => { }); it('updated() focuses the panel-input when the panel opens', () => { - const updFn = src.slice(src.indexOf('updated(changed)'), src.indexOf('updated(changed)') + 1300); + const updFn = src.slice(src.indexOf('updated(changed)'), src.indexOf('updated(changed)') + 2000); expect(updFn).toMatch(/panel-input/); }); }); diff --git a/frontend/tests/facet-and-submit.spec.js b/frontend/tests/facet-and-submit.spec.js index e18286a..b002a7e 100644 --- a/frontend/tests/facet-and-submit.spec.js +++ b/frontend/tests/facet-and-submit.spec.js @@ -215,6 +215,40 @@ test.describe('droppable submit navigation (issue #44)', () => { expect(page.url()).toMatch(/\/search\?.*q=frankenstein/); }); + test('active filters are included in the navigation URL', async ({ page }) => { + // Set an availability filter directly on the droppable ol-search-bar's local state, + // then submit — the resulting URL must include the availability param. + await page.evaluate(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + sb?.shadowRoot?.querySelector('.trigger-btn') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + }); + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?.shadowRoot?.querySelector('.panel-input') !== null; + }); + + // Type a query and set a filter via ol-filter-change event on the component + await page.evaluate(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + const input = sb?.shadowRoot?.querySelector('.panel-input'); + if (input) { input.value = 'frankenstein'; input.dispatchEvent(new Event('input', { bubbles: true })); } + // Directly mutate _localFilters to simulate a filter selection + if (sb) sb._localFilters = { ...sb._localFilters, availability: 'readable' }; + }); + + const navPromise = page.waitForURL(/\/search\?.*q=/, { timeout: 4000 }); + await page.evaluate(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + sb?.shadowRoot?.querySelector('.panel .submit') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + }); + await navPromise; + + expect(page.url()).toMatch(/q=frankenstein/); + expect(page.url()).toMatch(/availability=readable/); + }); + test('ol-search event is cancelable — calling preventDefault() suppresses navigation', async ({ page }) => { // A host that handles ol-search itself can prevent the fallback URL navigation. await page.evaluate(() => { From 654a5c41d5a8734a7ad926f4526ee1c03259df3f Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 10:11:06 -0700 Subject: [PATCH 03/12] test: failing tests for scroll lock on all droppable modes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vitest: assert scroll lock is gated on _open + showFacets, not only _mobileExpanded, so desktop droppable panel also locks scroll. Playwright: two desktop tests — overflow:hidden while panel open, overflow restored after Escape closes it. Tests fail red against current code; will go green once the lock trigger is moved from changed.has('_mobileExpanded') to changed.has('_open'). --- .../ol-search-bar.mobile-overlay.test.js | 31 +++++++++++ frontend/tests/mobile-overlay.spec.js | 55 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/frontend/src/components/ol-search-bar.mobile-overlay.test.js b/frontend/src/components/ol-search-bar.mobile-overlay.test.js index b6ad2f8..9c88714 100644 --- a/frontend/src/components/ol-search-bar.mobile-overlay.test.js +++ b/frontend/src/components/ol-search-bar.mobile-overlay.test.js @@ -117,3 +117,34 @@ describe('ol-search-bar mobile overlay JS contract', () => { expect(dcFn).toMatch(/document\.body\.style\.overflow/); }); }); + +// ── Scroll lock — any droppable mode (issue #44 follow-up) ─────────────────── +// +// Scroll lock must engage whenever the droppable search panel is open, not only +// when the mobile full-screen overlay is active. The gate is _open (the +// universal open state) checked against showFacets (droppable mode), NOT +// _mobileExpanded (mobile-only). + +describe('ol-search-bar scroll lock — any droppable panel open', () => { + // Find the line that acquires the lock so we can inspect its context. + const lockIdx = src.search(/this\._scrollLockActive\s*=\s*true/); + const lockCtx = lockIdx !== -1 ? src.slice(Math.max(0, lockIdx - 300), lockIdx) : ''; + + it('_scrollLockActive = true exists in source', () => { + expect(lockIdx).not.toBe(-1); + }); + + it('scroll lock acquire is gated on _open change (not _mobileExpanded) so desktop panel also locks scroll', () => { + expect(lockCtx).toMatch(/changed\.has\s*\(\s*'_open'\s*\)/); + }); + + it('scroll lock acquire condition checks showFacets so embedded mode does not lock scroll', () => { + expect(lockCtx).toMatch(/showFacets/); + }); + + it('disconnectedCallback restores overflow via _scrollLockActive flag', () => { + const dcFn = src.slice(src.indexOf('disconnectedCallback'), src.indexOf('disconnectedCallback') + 500); + expect(dcFn).toMatch(/_scrollLockActive/); + expect(dcFn).toMatch(/document\.body\.style\.overflow/); + }); +}); diff --git a/frontend/tests/mobile-overlay.spec.js b/frontend/tests/mobile-overlay.spec.js index bcc342b..62de0fa 100644 --- a/frontend/tests/mobile-overlay.spec.js +++ b/frontend/tests/mobile-overlay.spec.js @@ -211,3 +211,58 @@ test.describe('desktop — no overlay regression', () => { expect(hasMobileExp).toBe(false); }); }); + +// ── Desktop droppable — body scroll lock ───────────────────────────────────── +// +// Written BEFORE the fix that extended scroll locking from mobile-only to all +// droppable modes. These tests fail red on the old code and go green once +// updated() gates the lock on _open && showFacets instead of _mobileExpanded. + +test.describe('desktop droppable — body scroll lock', () => { + test.beforeEach(async ({ page }) => { + await page.setViewportSize({ width: 1024, height: 768 }); + await page.goto('/'); + await waitForSearchBar(page); + }); + + test('body and documentElement have overflow:hidden while droppable panel is open', async ({ page }) => { + await openSearch(page); + + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?._open === true; + }, { timeout: 2000 }); + + const overflow = await page.evaluate(() => ({ + body: getComputedStyle(document.body).overflow, + html: getComputedStyle(document.documentElement).overflow, + })); + + expect(overflow.body).toBe('hidden'); + expect(overflow.html).toBe('hidden'); + }); + + test('body and documentElement overflow is restored after panel closes', async ({ page }) => { + await openSearch(page); + + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?._open === true; + }, { timeout: 2000 }); + + await page.keyboard.press('Escape'); + + await page.waitForFunction(() => { + const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); + return sb?._open === false; + }, { timeout: 2000 }); + + const overflow = await page.evaluate(() => ({ + body: document.body.style.overflow, + html: document.documentElement.style.overflow, + })); + + expect(overflow.body).toBe(''); + expect(overflow.html).toBe(''); + }); +}); From 63c5e37120c41b1790e02c7620fb80e9a599fe1c Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 10:11:22 -0700 Subject: [PATCH 04/12] feat: lock body scroll for all droppable panel states, not only mobile overlay Move scroll-lock trigger from changed.has('_mobileExpanded') to changed.has('_open'), with condition this._open && this.showFacets. This ensures the page behind the search panel cannot scroll on desktop viewports just as it cannot on mobile. --- frontend/src/components/ol-search-bar.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frontend/src/components/ol-search-bar.js b/frontend/src/components/ol-search-bar.js index 126ced4..cd579e1 100644 --- a/frontend/src/components/ol-search-bar.js +++ b/frontend/src/components/ol-search-bar.js @@ -152,17 +152,18 @@ export class OlSearchBar extends LitElement { } // Sync full-screen overlay class on the host element. this.classList.toggle('mobile-exp', this._mobileExpanded); - // Prevent body scroll while overlay is active; lock too since many - // browsers/frameworks scroll it. Save pre-existing inline values so close - // restores exactly what was there before (avoids clobbering other overlays). - if (changed.has('_mobileExpanded')) { - if (this._mobileExpanded && !this._scrollLockActive) { + // Lock body scroll whenever the droppable panel is open on any viewport. + // Lock too since many browsers/frameworks scroll it. + // Save pre-existing inline values so close restores them exactly. + if (changed.has('_open')) { + const shouldLock = this._open && this.showFacets; + if (shouldLock && !this._scrollLockActive) { this._prevBodyOverflow = document.body.style.overflow; this._prevDocumentOverflow = document.documentElement.style.overflow; this._scrollLockActive = true; document.body.style.overflow = 'hidden'; document.documentElement.style.overflow = 'hidden'; - } else if (!this._mobileExpanded && this._scrollLockActive) { + } else if (!shouldLock && this._scrollLockActive) { document.body.style.overflow = this._prevBodyOverflow ?? ''; document.documentElement.style.overflow = this._prevDocumentOverflow ?? ''; this._scrollLockActive = false; From 4ff2391446aac8f03ae61f0f16929f742a1e419d Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 10:23:58 -0700 Subject: [PATCH 05/12] test: failing tests for mobile overlay flex height chain --- .../src/components/ol-search-bar.mobile-overlay.test.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frontend/src/components/ol-search-bar.mobile-overlay.test.js b/frontend/src/components/ol-search-bar.mobile-overlay.test.js index 9c88714..93b764f 100644 --- a/frontend/src/components/ol-search-bar.mobile-overlay.test.js +++ b/frontend/src/components/ol-search-bar.mobile-overlay.test.js @@ -45,11 +45,20 @@ describe('ol-search-bar mobile overlay CSS contract', () => { expect(src).toMatch(/:host\(\.mobile-exp\)\s+\.ac-scroll[^}]*min-height\s*:\s*0/); }); + it(':host(.mobile-exp) .ac-scroll removes max-height cap so content fills the space', () => { + expect(src).toMatch(/:host\(\.mobile-exp\)\s+\.ac-scroll[^}]*max-height\s*:\s*none/); + }); + it(':host(.mobile-exp) .panel is a flex column so children stack and ac-scroll can flex-grow', () => { expect(src).toMatch(/:host\(\.mobile-exp\)\s+\.panel[^}]*display\s*:\s*flex/); expect(src).toMatch(/:host\(\.mobile-exp\)\s+\.panel[^}]*flex-direction\s*:\s*column/); }); + it(':host(.mobile-exp) .panel and .search-outer both have min-height:0 to allow flex shrink', () => { + expect(src).toMatch(/:host\(\.mobile-exp\)\s+\.panel[^}]*min-height\s*:\s*0/); + expect(src).toMatch(/:host\(\.mobile-exp\)\s+\.search-outer[^}]*min-height\s*:\s*0/); + }); + it('@media 600px .ac-scroll rule is scoped to :host(:not(.mobile-exp)) — no specificity conflict', () => { const mediaBlock = src.slice(src.indexOf('@media (max-width: 600px)'), src.indexOf('@media (max-width: 600px)') + 400); expect(mediaBlock).toMatch(/:host\(:not\(\.mobile-exp\)\)\s+\.ac-scroll/); From 4acf1fd5bac2a4311cc7b33c621ba21f42d97344 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 10:37:33 -0700 Subject: [PATCH 06/12] test: update stale scroll-lock assertions to match _open+showFacets implementation --- .../components/ol-search-bar.mobile-overlay.test.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/ol-search-bar.mobile-overlay.test.js b/frontend/src/components/ol-search-bar.mobile-overlay.test.js index 93b764f..968b926 100644 --- a/frontend/src/components/ol-search-bar.mobile-overlay.test.js +++ b/frontend/src/components/ol-search-bar.mobile-overlay.test.js @@ -115,14 +115,15 @@ describe('ol-search-bar mobile overlay JS contract', () => { expect(src).toMatch(/classList\.toggle\s*\(\s*['"]mobile-exp['"]\s*,\s*this\._mobileExpanded\s*\)/); }); - it('locks body scroll when mobile overlay opens and restores it when it closes', () => { - const updatedFn = src.slice(src.indexOf('updated(changed)'), src.indexOf('updated(changed)') + 1200); + it('locks body scroll when droppable panel opens and restores it when it closes', () => { + const updatedFn = src.slice(src.indexOf('updated(changed)'), src.indexOf('updated(changed)') + 2000); expect(updatedFn).toMatch(/document\.body\.style\.overflow/); - expect(updatedFn).toMatch(/_mobileExpanded.*hidden|hidden.*_mobileExpanded/s); + expect(updatedFn).toMatch(/this\._open.*this\.showFacets|this\.showFacets.*this\._open/s); }); - it('restores body scroll in disconnectedCallback in case component is removed while expanded', () => { - const dcFn = src.slice(src.indexOf('disconnectedCallback()'), src.indexOf('disconnectedCallback()') + 400); + it('restores body scroll in disconnectedCallback via _scrollLockActive flag', () => { + const dcFn = src.slice(src.indexOf('disconnectedCallback()'), src.indexOf('disconnectedCallback()') + 600); + expect(dcFn).toMatch(/_scrollLockActive/); expect(dcFn).toMatch(/document\.body\.style\.overflow/); }); }); From ff090043da1ed53abc1b331b5bd288ee139717b8 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 10:37:48 -0700 Subject: [PATCH 07/12] =?UTF-8?q?docs:=20Copilot=20only=20reviews=20once?= =?UTF-8?q?=20=E2=80=94=20no=20follow-up=20ScheduleWakeup=20needed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- AGENTS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c1d0c99..c195bd6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -143,7 +143,7 @@ The PR body should state which test proves the fix, or link the screenshot inlin ### Wait for Copilot review after opening a PR -After opening a PR, **schedule a reminder to check for Copilot feedback in ~8 minutes** using `ScheduleWakeup`. Copilot typically posts its review within a few minutes of the PR being created; 8 minutes gives it enough time to finish without waiting too long. +After opening a PR, **schedule a one-time reminder to check for Copilot feedback in ~8 minutes** using `ScheduleWakeup`. Copilot posts a single initial review shortly after the PR is created; it does **not** re-review subsequent commits, so only one check is needed. `ScheduleWakeup` fires in the **same session** that opened the PR, so the prompt can be short — all project context, AGENTS.md rules, and the reply+resolve workflow are already in conversation history. The prompt only needs to identify the PR number and the action: @@ -156,7 +156,7 @@ When the reminder fires: 2. Fetch the overview review: `gh pr view --repo ArchiveLabs/openlibrary-components --json reviews` 3. Address every comment, push a fix commit, then reply and resolve each thread (see rule below). -If there are no comments yet when the reminder fires, wait another few minutes before concluding there is no feedback. +If there are no comments yet when the reminder fires, wait another few minutes — but do **not** schedule further checks. Copilot only reviews once. ### Responding to review comments (Copilot or human) From f881caa065394d6052ed5308e19868e695ecb592 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 21:25:42 -0700 Subject: [PATCH 08/12] test: failing tests for facet label pluralization and auto-seed on open - _facetLabel should use 'Authors'/'Subjects' (plural) - _toggleFacet should auto-seed author/subject facet with current _q --- .../ol-search-bar.facet-seed.test.js | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 frontend/src/components/ol-search-bar.facet-seed.test.js diff --git a/frontend/src/components/ol-search-bar.facet-seed.test.js b/frontend/src/components/ol-search-bar.facet-seed.test.js new file mode 100644 index 0000000..77fa982 --- /dev/null +++ b/frontend/src/components/ol-search-bar.facet-seed.test.js @@ -0,0 +1,51 @@ +import { readFileSync } from 'fs'; +import { describe, it, expect } from 'vitest'; + +const src = readFileSync(new URL('./ol-search-bar.js', import.meta.url), 'utf8'); + +// ── Facet label pluralization ───────────────────────────────────────────────── + +describe('ol-search-bar facet label pluralization', () => { + const facetLabelFn = src.slice(src.indexOf('_facetLabel(name)'), src.indexOf('_facetLabel(name)') + 600); + + it('_facetLabel returns "Authors" (plural) when no authors are selected', () => { + expect(facetLabelFn).toMatch(/['"]Authors['"]/); + }); + + it('_facetLabel returns "Subjects" (plural) when no subjects are selected', () => { + expect(facetLabelFn).toMatch(/['"]Subjects['"]/); + }); + + it('_facetLabel does NOT use singular "Author" (without s) as a standalone label', () => { + // Ensure there is no bare 'Author' string (without trailing 's' or count) + // The label must be plural: 'Authors' or 'Authors (N)' + expect(facetLabelFn).not.toMatch(/:\s*['"]Author['"]/); + }); + + it('_facetLabel does NOT use singular "Subject" (without s) as a standalone label', () => { + expect(facetLabelFn).not.toMatch(/:\s*['"]Subject['"]/); + }); +}); + +// ── Auto-seed facet on open ─────────────────────────────────────────────────── + +describe('ol-search-bar _toggleFacet auto-seeds author/subject with current query', () => { + const toggleFn = src.slice(src.indexOf('_toggleFacet(name, e)'), src.indexOf('_toggleFacet(name, e)') + 600); + + it('_toggleFacet body checks this._q before seeding', () => { + expect(toggleFn).toMatch(/this\._q/); + }); + + it('_toggleFacet calls _onDropAuthorSearch when opening the author facet with a query', () => { + expect(toggleFn).toMatch(/_onDropAuthorSearch/); + }); + + it('_toggleFacet calls _onDropSubjectSearch when opening the subject facet with a query', () => { + expect(toggleFn).toMatch(/_onDropSubjectSearch/); + }); + + it('_toggleFacet seeds only when opening (not closing) the facet', () => { + // The seed call must be guarded by an "opening" check, not unconditional + expect(toggleFn).toMatch(/opening|this\._openFacet\s*!==\s*name/); + }); +}); From a0fb1b5b281b4abf578cb8ec4266b2ba5a1b1797 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 21:26:13 -0700 Subject: [PATCH 09/12] feat: pluralize author/subject facet labels and auto-seed with current query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _facetLabel: 'Author'→'Authors', 'Subject'→'Subjects' - _toggleFacet: when opening author/subject facet with a non-empty _q, immediately call _onDropAuthorSearch/_onDropSubjectSearch to pre-populate results instead of showing an empty list --- frontend/src/components/ol-search-bar.facet-seed.test.js | 2 +- frontend/src/components/ol-search-bar.js | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/ol-search-bar.facet-seed.test.js b/frontend/src/components/ol-search-bar.facet-seed.test.js index 77fa982..6be558c 100644 --- a/frontend/src/components/ol-search-bar.facet-seed.test.js +++ b/frontend/src/components/ol-search-bar.facet-seed.test.js @@ -6,7 +6,7 @@ const src = readFileSync(new URL('./ol-search-bar.js', import.meta.url), 'utf8') // ── Facet label pluralization ───────────────────────────────────────────────── describe('ol-search-bar facet label pluralization', () => { - const facetLabelFn = src.slice(src.indexOf('_facetLabel(name)'), src.indexOf('_facetLabel(name)') + 600); + const facetLabelFn = src.slice(src.indexOf('_facetLabel(name)'), src.indexOf('_facetLabel(name)') + 700); it('_facetLabel returns "Authors" (plural) when no authors are selected', () => { expect(facetLabelFn).toMatch(/['"]Authors['"]/); diff --git a/frontend/src/components/ol-search-bar.js b/frontend/src/components/ol-search-bar.js index cd579e1..9a6ee1a 100644 --- a/frontend/src/components/ol-search-bar.js +++ b/frontend/src/components/ol-search-bar.js @@ -428,7 +428,12 @@ export class OlSearchBar extends LitElement { _toggleFacet(name, e) { e.stopPropagation(); if (this._openFacet !== name) this._lastFacetBtn = e.currentTarget; + const opening = this._openFacet !== name; this._openFacet = this._openFacet === name ? null : name; + if (opening && this._q.trim()) { + if (name === 'author') this._onDropAuthorSearch({ detail: { q: this._q.trim() } }); + if (name === 'subject') this._onDropSubjectSearch({ detail: { q: this._q.trim() } }); + } } _onDropFacetChange(e) { @@ -761,8 +766,8 @@ export class OlSearchBar extends LitElement { const total = (f.genres?.length ?? 0) + (f.fictionFilter ? 1 : 0); return total ? `Genre (${total})` : 'Genre'; } - case 'author': return f.authors?.length ? `Author (${f.authors.length})` : 'Author'; - case 'subject': return f.subjects?.length ? `Subject (${f.subjects.length})` : 'Subject'; + case 'author': return f.authors?.length ? `Authors (${f.authors.length})` : 'Authors'; + case 'subject': return f.subjects?.length ? `Subjects (${f.subjects.length})` : 'Subjects'; } } From a6fafa05d5198c49ffe833b5a61f11c8b94a3fec Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 21:37:41 -0700 Subject: [PATCH 10/12] test: update facet-seed tests for query-scoped _defaultAuthors/_defaultSubjects Replace tests checking _onDropAuthorSearch/Subject (wrong: populates typeahead results, not open defaults) with tests for _seedFacetsForQuery + _facetCache architecture that correctly updates _defaultAuthors/_defaultSubjects. --- .../ol-search-bar.facet-seed.test.js | 72 +++++++++++++++---- 1 file changed, 60 insertions(+), 12 deletions(-) diff --git a/frontend/src/components/ol-search-bar.facet-seed.test.js b/frontend/src/components/ol-search-bar.facet-seed.test.js index 6be558c..9af3ad7 100644 --- a/frontend/src/components/ol-search-bar.facet-seed.test.js +++ b/frontend/src/components/ol-search-bar.facet-seed.test.js @@ -17,8 +17,6 @@ describe('ol-search-bar facet label pluralization', () => { }); it('_facetLabel does NOT use singular "Author" (without s) as a standalone label', () => { - // Ensure there is no bare 'Author' string (without trailing 's' or count) - // The label must be plural: 'Authors' or 'Authors (N)' expect(facetLabelFn).not.toMatch(/:\s*['"]Author['"]/); }); @@ -27,25 +25,75 @@ describe('ol-search-bar facet label pluralization', () => { }); }); -// ── Auto-seed facet on open ─────────────────────────────────────────────────── +// ── Query-scoped facet seeding ──────────────────────────────────────────────── +// +// When the user opens an author or subject facet while a search query is active, +// the default suggestions shown (before the user types anything in the facet's own +// search box) should be derived from the search results, not from POPULAR_AUTHORS / +// POPULAR_SUBJECTS. Results are cached per query so re-opening is instant. -describe('ol-search-bar _toggleFacet auto-seeds author/subject with current query', () => { +describe('ol-search-bar _seedFacetsForQuery — query-scoped defaults', () => { + it('_seedFacetsForQuery method is defined in source', () => { + expect(src).toMatch(/_seedFacetsForQuery/); + }); + + it('_seedFacetsForQuery uses _facetCache to avoid redundant fetches', () => { + const fnStart = src.indexOf('_seedFacetsForQuery'); + const fnBody = fnStart !== -1 ? src.slice(fnStart, fnStart + 600) : ''; + expect(fnBody).toMatch(/_facetCache/); + }); + + it('_seedFacetsForQuery sets _defaultAuthors from the fetched results', () => { + const fnStart = src.indexOf('_seedFacetsForQuery'); + const fnBody = fnStart !== -1 ? src.slice(fnStart, fnStart + 600) : ''; + expect(fnBody).toMatch(/_defaultAuthors/); + }); + + it('_seedFacetsForQuery sets _defaultSubjects from the fetched results', () => { + const fnStart = src.indexOf('_seedFacetsForQuery'); + const fnBody = fnStart !== -1 ? src.slice(fnStart, fnStart + 600) : ''; + expect(fnBody).toMatch(/_defaultSubjects/); + }); + + it('_facetCache is initialised in constructor', () => { + const ctor = src.slice(src.indexOf('constructor()'), src.indexOf('constructor()') + 800); + expect(ctor).toMatch(/_facetCache/); + }); + + it('fetchQueryFacets is imported from utils/facets.js', () => { + expect(src).toMatch(/fetchQueryFacets/); + }); +}); + +describe('ol-search-bar _toggleFacet delegates to _seedFacetsForQuery', () => { const toggleFn = src.slice(src.indexOf('_toggleFacet(name, e)'), src.indexOf('_toggleFacet(name, e)') + 600); - it('_toggleFacet body checks this._q before seeding', () => { + it('_toggleFacet calls _seedFacetsForQuery when opening author or subject facet', () => { + expect(toggleFn).toMatch(/_seedFacetsForQuery/); + }); + + it('_toggleFacet guards the seed call with this._q so empty queries skip it', () => { expect(toggleFn).toMatch(/this\._q/); }); - it('_toggleFacet calls _onDropAuthorSearch when opening the author facet with a query', () => { - expect(toggleFn).toMatch(/_onDropAuthorSearch/); + it('_toggleFacet seeds only when opening (not closing) the facet', () => { + expect(toggleFn).toMatch(/opening/); + }); + + it('_toggleFacet does NOT directly call _onDropAuthorSearch for seeding', () => { + // _onDropAuthorSearch is for user-typed search inside the facet, not query seeding + expect(toggleFn).not.toMatch(/_onDropAuthorSearch/); }); - it('_toggleFacet calls _onDropSubjectSearch when opening the subject facet with a query', () => { - expect(toggleFn).toMatch(/_onDropSubjectSearch/); + it('_toggleFacet does NOT directly call _onDropSubjectSearch for seeding', () => { + expect(toggleFn).not.toMatch(/_onDropSubjectSearch/); }); +}); - it('_toggleFacet seeds only when opening (not closing) the facet', () => { - // The seed call must be guarded by an "opening" check, not unconditional - expect(toggleFn).toMatch(/opening|this\._openFacet\s*!==\s*name/); +describe('ol-search-bar dice respects query-scoped cache', () => { + const template = src.slice(src.indexOf('ol-facet-shuffle-authors'), src.indexOf('ol-facet-shuffle-authors') + 400); + + it('dice handler checks _facetCache (or _q) to pick pool when query is active', () => { + expect(template).toMatch(/_facetCache|_q/); }); }); From 7581a371bde24a11f1bc036846b6fa79df1c5d66 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 21:39:12 -0700 Subject: [PATCH 11/12] feat: query-scoped facet suggestions with per-query caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - backend: /api/search/facets now aggregates subject_facet and author_facet fields from OL search.json docs (rows=30) instead of calling the HTML partial. Returns top subjects and authors ranked by occurrence count. - facets.js: add fetchQueryFacets(q) — calls /api/search/facets and returns { authors, subjects } shaped for ol-facet-drop's defaultAuthors/defaultSubjects. - ol-search-bar: add _facetCache (Map) and _seedFacetsForQuery(q) method. Opening author or subject facet while a query is active seeds _defaultAuthors / _defaultSubjects from query-scoped facets; cache hit avoids re-fetching on subsequent opens of the same query. Empty query keeps POPULAR_AUTHORS/SUBJECTS. Dice handlers shuffle from the cached query pool when one exists. --- backend/main.py | 41 ++++++++++++++----- .../ol-search-bar.facet-seed.test.js | 2 +- frontend/src/components/ol-search-bar.js | 41 ++++++++++++++++--- frontend/src/utils/facets.js | 21 ++++++++++ 4 files changed, 88 insertions(+), 17 deletions(-) diff --git a/backend/main.py b/backend/main.py index 30c1616..7c1f284 100644 --- a/backend/main.py +++ b/backend/main.py @@ -1,6 +1,5 @@ from __future__ import annotations -import json from pathlib import Path from typing import Optional @@ -136,20 +135,42 @@ async def subject_search(q: str = "", limit: int = 10): @app.get("/api/search/facets") -async def search_facets(q: str = ""): - data = json.dumps({ - "param": {"q": q}, - "path": "/search", - "query": f"?q={q}&mode=everything", - }) +async def search_facets(q: str = "", rows: int = 30, limit: int = 25): + """Aggregate subject_facet and author_facet from OL search results for a query.""" + if not q.strip(): + return {"authors": [], "subjects": []} + async with httpx.AsyncClient() as client: resp = await client.get( - f"{OL_BASE}/partials/SearchFacets.json", - params={"data": data}, + f"{OL_BASE}/search.json", + params={"q": q, "fields": "subject_facet,author_facet", "rows": rows}, timeout=10.0, ) resp.raise_for_status() - return resp.json() + + docs = resp.json().get("docs", []) + subj_counts: dict[str, int] = {} + auth_index: dict[str, dict] = {} + + for doc in docs: + for s in doc.get("subject_facet", []): + subj_counts[s] = subj_counts.get(s, 0) + 1 + for a in doc.get("author_facet", []): + parts = a.split(" ", 1) + if len(parts) == 2: + key, name = parts + if name not in auth_index: + auth_index[name] = {"name": name, "key": key, "count": 0} + auth_index[name]["count"] += 1 + + subjects = sorted( + [{"name": k, "count": v} for k, v in subj_counts.items()], + key=lambda x: -x["count"], + )[:limit] + + authors = sorted(auth_index.values(), key=lambda x: -x["count"])[:12] + + return {"authors": list(authors), "subjects": subjects} _static = Path(__file__).parent / "static" diff --git a/frontend/src/components/ol-search-bar.facet-seed.test.js b/frontend/src/components/ol-search-bar.facet-seed.test.js index 9af3ad7..2e083d3 100644 --- a/frontend/src/components/ol-search-bar.facet-seed.test.js +++ b/frontend/src/components/ol-search-bar.facet-seed.test.js @@ -56,7 +56,7 @@ describe('ol-search-bar _seedFacetsForQuery — query-scoped defaults', () => { }); it('_facetCache is initialised in constructor', () => { - const ctor = src.slice(src.indexOf('constructor()'), src.indexOf('constructor()') + 800); + const ctor = src.slice(src.indexOf('constructor()'), src.indexOf('constructor()') + 1500); expect(ctor).toMatch(/_facetCache/); }); diff --git a/frontend/src/components/ol-search-bar.js b/frontend/src/components/ol-search-bar.js index 9a6ee1a..d41372e 100644 --- a/frontend/src/components/ol-search-bar.js +++ b/frontend/src/components/ol-search-bar.js @@ -5,7 +5,7 @@ import { getSortLabel, buildChips, } from '../utils/filters.js'; import { BREAKPOINTS } from '../utils/breakpoints.js'; -import { fetchAuthorSuggestions, fetchSubjectSuggestions } from '../utils/facets.js'; +import { fetchAuthorSuggestions, fetchSubjectSuggestions, fetchQueryFacets } from '../utils/facets.js'; import './ol-howto-modal.js'; import './ol-facet-drop.js'; @@ -87,6 +87,8 @@ export class OlSearchBar extends LitElement { this._acAbort = null; // AbortController for in-flight autocomplete fetch this._authorAbort = null; // AbortController for author search this._subjectAbort = null; // AbortController for subject search + this._facetAbort = null; // AbortController for query-facet fetch + this._facetCache = new Map(); // query → { authors, subjects } — avoids re-fetching this._lastFacetBtn = null; // button that opened the current facet dropdown (for focus return) this._onWinResize = () => { @@ -430,9 +432,30 @@ export class OlSearchBar extends LitElement { if (this._openFacet !== name) this._lastFacetBtn = e.currentTarget; const opening = this._openFacet !== name; this._openFacet = this._openFacet === name ? null : name; - if (opening && this._q.trim()) { - if (name === 'author') this._onDropAuthorSearch({ detail: { q: this._q.trim() } }); - if (name === 'subject') this._onDropSubjectSearch({ detail: { q: this._q.trim() } }); + if (opening && (name === 'author' || name === 'subject') && this._q.trim()) { + this._seedFacetsForQuery(this._q.trim()); + } + } + + async _seedFacetsForQuery(q) { + if (this._facetCache.has(q)) { + const cached = this._facetCache.get(q); + this._defaultAuthors = shufflePick(cached.authors, 6); + this._defaultSubjects = shufflePick(cached.subjects, 6); + return; + } + this._facetAbort?.abort(); + this._facetAbort = new AbortController(); + this._facetsLoading = true; + try { + const result = await fetchQueryFacets(q, { signal: this._facetAbort.signal, apiBase: this.apiBase }); + this._facetCache.set(q, result); + this._defaultAuthors = shufflePick(result.authors, 6); + this._defaultSubjects = shufflePick(result.subjects, 6); + } catch { + // Keep current defaults on error + } finally { + if (!this._facetAbort?.signal.aborted) this._facetsLoading = false; } } @@ -804,8 +827,14 @@ export class OlSearchBar extends LitElement { @ol-facet-change=${this._onDropFacetChange} @ol-facet-search-authors=${this._onDropAuthorSearch} @ol-facet-search-subjects=${this._onDropSubjectSearch} - @ol-facet-shuffle-authors=${() => { this._defaultAuthors = shufflePick(POPULAR_AUTHORS, 6); }} - @ol-facet-shuffle-subjects=${() => { this._defaultSubjects = shufflePick(POPULAR_SUBJECTS, 6); }} + @ol-facet-shuffle-authors=${() => { + const pool = (this._q.trim() && this._facetCache.get(this._q.trim())?.authors) || POPULAR_AUTHORS; + this._defaultAuthors = shufflePick(pool, 6); + }} + @ol-facet-shuffle-subjects=${() => { + const pool = (this._q.trim() && this._facetCache.get(this._q.trim())?.subjects) || POPULAR_SUBJECTS; + this._defaultSubjects = shufflePick(pool, 6); + }} > ` : ''} `; diff --git a/frontend/src/utils/facets.js b/frontend/src/utils/facets.js index 1b62c8f..0850eab 100644 --- a/frontend/src/utils/facets.js +++ b/frontend/src/utils/facets.js @@ -39,3 +39,24 @@ export async function fetchSubjectSuggestions(q, { signal, apiBase = '' } = {}) throw err; } } + +/** + * Fetch query-scoped author and subject facets by aggregating OL search results. + * Returns the top subjects/authors from works matching the query — suitable for + * pre-populating facet dropdowns when a search term is active. + * + * @param {string} q - Main search query (< 2 chars → empty result) + * @param {{ signal?: AbortSignal, apiBase?: string }} [opts] + * @returns {Promise<{ authors: object[], subjects: object[] }>} + */ +export async function fetchQueryFacets(q, { signal, apiBase = '' } = {}) { + if (!q || q.trim().length < 2) return { authors: [], subjects: [] }; + try { + const url = `${apiBase}/api/search/facets?q=${encodeURIComponent(q.trim())}`; + const d = await (await fetch(url, { signal })).json(); + return { authors: d.authors ?? [], subjects: d.subjects ?? [] }; + } catch (err) { + if (err.name === 'AbortError') return { authors: [], subjects: [] }; + throw err; + } +} From d64857aef609dbca721290f08e813acd6d9c2da1 Mon Sep 17 00:00:00 2001 From: "Michael E. Karpeles (Mek)" Date: Mon, 27 Apr 2026 21:39:44 -0700 Subject: [PATCH 12/12] fix: correct misleading comment in facet-and-submit.spec.js The comment said 'ol-filter-change event' but the code directly mutates _localFilters. Copilot review comment #3151575651. --- frontend/tests/facet-and-submit.spec.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frontend/tests/facet-and-submit.spec.js b/frontend/tests/facet-and-submit.spec.js index b002a7e..0da78b6 100644 --- a/frontend/tests/facet-and-submit.spec.js +++ b/frontend/tests/facet-and-submit.spec.js @@ -228,12 +228,11 @@ test.describe('droppable submit navigation (issue #44)', () => { return sb?.shadowRoot?.querySelector('.panel-input') !== null; }); - // Type a query and set a filter via ol-filter-change event on the component + // Type a query and mutate _localFilters directly to simulate a filter selection await page.evaluate(() => { const sb = document.querySelector('ol-header')?.shadowRoot?.querySelector('ol-search-bar'); const input = sb?.shadowRoot?.querySelector('.panel-input'); if (input) { input.value = 'frankenstein'; input.dispatchEvent(new Event('input', { bubbles: true })); } - // Directly mutate _localFilters to simulate a filter selection if (sb) sb._localFilters = { ...sb._localFilters, availability: 'readable' }; });