WEBDEV-8164: Improve navigability of items on Facets > More pane

[bfalling]

• Adds a “Filter by” text field, which allows case-insensitive filtering of the values.
• To make it easier to sweep through and see an overview of values, replaces the pagination with a horizontal scrollbar, preserving the columnar look but thus allowing quick scrubbing through all columns. Only does this replacement if there are <1000 items, because with too many items, the whole UI gums up.

[github-actions]

PR Preview Action v1.8.1
🚀 View preview at https://internetarchive.github.io/iaux-collection-browser/pr/pr-548/
Built to branch gh-pages at 2026-03-12 04:09 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[codecov]

Codecov Report

❌ Patch coverage is 95.01247% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.51%. Comparing base (26b10a5) to head (5006be8).

Files with missing lines	Patch %	Lines
src/collection-facets/more-facets-content.ts	93.95%	40 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #548      +/-   ##
==========================================
+ Coverage   92.43%   92.51%   +0.07%     
==========================================
  Files         113      113              
  Lines       18942    19621     +679     
  Branches     1377     1439      +62     
==========================================
+ Hits        17509    18152     +643     
- Misses       1403     1439      +36     
  Partials       30       30              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jbuckner]

I'm curious about this getComputedStyle block and the associated parseInt(styles.columnCount, 10) below. Do we have access to columnCount / columnGap from within this element directly or do we have to pull it from the child elements? This feels like a bit of a roundabout way to get these values instead of explicitly referencing them.

[bfalling]

Yes, getComputedStyle is the correct approach here. The columnCount and columnGap values are defined in CSS stylesheets (and vary by media query), so the only way to read the resolved values at runtime is through the computed style API. There's no direct DOM property for CSS multi-column layout dimensions — getComputedStyle is the standard mechanism for this.

[jbuckner]

I question this assertion. I've never had to do a getComputedStyle() for anything. There's a layout hierarchy and this seems to invert that by requiring the parent to query its children. The parent should determine its children's layout, not the other way around.

[jbuckner]

An alternative approach would be to define these css vars in this parent class and pass them into its children so this class is the canonical source and the children still have access to the same vars

[bfalling]

Good point — you're right that the parent shouldn't need to query values back from its children when it's the one defining them in the first place.

Fixed: getColumnStep() no longer uses getComputedStyle. The column count is now derived from isCompactView (which already tracks the same <= 560px breakpoint as the CSS media query that switches --facetsColumnCount between 3 and 1). The column gap is a static constant since --facetsColumnGap is never overridden anywhere.

[bfalling]

(Hey, Jason! Thanks for bringing up the great points you did. Claude's been posting as me. I'll try to add a "(Claude)" prefix to its posts going forward!)

[jbuckner]

Claude Code Review: PR #548 — WEBDEV-8164: Improve navigability of items on Facets > More pane

Overview

Well-structured PR that adds meaningful UX improvements to the facets modal: a filter input, hybrid scroll/pagination display, compact pagination for narrow viewports, and scroll navigation arrows. The code is clean, well-documented with JSDoc, and has solid test coverage.

That said, I found several issues worth addressing before merging.

---

Critical Issues

1. Memory Leak: Escape Key Listener Is Never Removed

File: src/collection-facets/more-facets-content.ts:356-366

setupEscapeListeners adds an inline arrow function to document. The removeEventListener in the else branch passes a different empty arrow function — it's a no-op. If this component is created/destroyed multiple times (modal open/close cycles), each instance permanently leaks an event listener holding a reference to this.modalManager.

// Current (broken):
document.addEventListener('keydown', (e: KeyboardEvent) => { ... });
// ...
document.removeEventListener('keydown', () => {}); // different reference, does nothing

Fix: Store the handler as a class field (like scrollHandler already is) and remove it in disconnectedCallback.

Note: This bug predates this PR, but since disconnectedCallback was added here, it's a good opportunity to fix it.

---

2. Scroll Listener Attached to Wrong Element After Mode Switch

File: src/collection-facets/more-facets-content.ts:258-273

When the display flips between horizontal-scroll and pagination mode, the .facets-content element is replaced by a different DOM node (different render branches at lines ~807 vs ~823). The @query('.facets-content') decorator always re-queries the live DOM, so after a mode switch this.facetsContentEl points to the new node. removeScrollListener then calls removeEventListener on the new element — not the old one the listener was actually attached to. The old listener is orphaned on a now-detached DOM node.

Fix: Save a reference to the element the listener was actually attached to:

private scrollListenerTarget?: HTMLElement;

private attachScrollListener(): void {
  if (this.scrollListenerAttached || !this.facetsContentEl) return;
  this.scrollListenerTarget = this.facetsContentEl;
  this.scrollListenerTarget.addEventListener('scroll', this.scrollHandler, { passive: true });
  this.scrollListenerAttached = true;
}

private removeScrollListener(): void {
  if (!this.scrollListenerAttached || !this.scrollListenerTarget) return;
  this.scrollListenerTarget.removeEventListener('scroll', this.scrollHandler);
  this.scrollListenerTarget = undefined;
  this.scrollListenerAttached = false;
}

---

3. No Error Handling in updateSpecificFacets — Loading Spinner Stuck on Error

File: src/collection-facets/more-facets-content.ts:382-413

updateSpecificFacets is called fire-and-forget from updated(). If the search service throws (network error, timeout, etc.), this.facetsLoading = false is never reached. The user sees an infinite loading spinner with no way to recover.

Fix: Wrap in try/finally:

async updateSpecificFacets(): Promise<void> {
  // ...
  try {
    const results = await this.searchService?.search(params, this.searchType);
    this.aggregations = results?.success?.response.aggregations;
  } finally {
    this.facetsLoading = false;
  }
}

---

Important Issues

4. Arrow Flash on Initial Render

File: src/collection-facets/more-facets-content.ts:164

The initial state is atScrollStart = true, atScrollEnd = false. The arrow visibility condition !atScrollStart || !atScrollEnd evaluates to true, so the right arrow renders briefly before updateScrollState() runs in requestAnimationFrame and determines there's no overflow. This causes a visible flash.

Fix: Initialize atScrollEnd = true so both arrows are hidden until measurement actually occurs.

---

5. emitPageClick Fires Spuriously on Initial Pagination Render

File: src/collection-facets/more-facets-pagination.ts:37-48

In willUpdate, emitPageClick() fires whenever currentPage changes — including the initial render when currentPage is set for the first time. This dispatches a spurious pageNumberClicked event with { page: 1 }, which triggers the parent's analytics handler to log a phantom page change.

Fix: Guard against the initial render:

if (changed.has('currentPage') && changed.get('currentPage') !== undefined) {
  this.emitPageClick();
}

---

6. Hidden Scroll Arrows Remain Keyboard-Focusable on Mobile

File: src/collection-facets/more-facets-content.ts (CSS at @media (max-width: 560px))

The scroll arrows are hidden via display: none on mobile, but they're still rendered in the DOM when !atScrollStart || !atScrollEnd is true. While display: none does prevent focus in most browsers, this creates a fragile dependency. Consider also adding aria-hidden="true" or conditionally not rendering them when isCompactView is true, since the mobile layout doesn't use horizontal scrolling.

---

7. pageChanged Event Missing composed: true

File: src/collection-facets/more-facets-content.ts:741

The pageChanged CustomEvent doesn't set bubbles: true, composed: true. It won't cross the Shadow DOM boundary if any parent component needs to listen. Compare with facetsChanged at line 853, which correctly sets both flags.

---

Minor Issues

8. ResizeObserver Triggers Unnecessary Re-Renders

File: src/collection-facets/more-facets-content.ts:344-351

The ResizeObserver callback unconditionally assigns this.isCompactView on every resize, triggering a LitElement update cycle even when the boolean value hasn't changed (e.g., resizing from 600px to 601px). Add a guard:

const compact = entry.contentRect.width <= 560;
if (compact !== this.isCompactView) {
  this.isCompactView = compact;
}

9. .gitignore Missing Trailing Newline

The .gitignore change ends with \ No newline at end of file. POSIX convention expects a trailing newline.

---

What Looks Good 👍

• The hybrid scroll/pagination approach with the PAGINATION_THRESHOLD constant is a clean design
• Filter logic correctly uses collectionTitles for collection facets (matching what the user actually sees)
• Scroll snapping via snapToColumn / getColumnStep is thoughtful
• Compact pagination logic (updatePagesCompact) with good test coverage for edge cases
• Proper cleanup of ResizeObserver and scroll listener in disconnectedCallback
• tabindex="-1" on the toggle switch button is a nice a11y fix
• Good use of passive: true on the scroll listener
• font-family: inherit fix on buttons prevents inconsistent fonts

[bfalling]

Responses to Claude Code Review Items

Thanks for the thorough review! Here's what was addressed:

Critical

1. Memory Leak: Escape Key Listener — Fixed. The handler is now stored as a class field (escapeHandler) and properly removed in disconnectedCallback.

2. Scroll Listener DOM Issue — Fixed. Added a scrollListenerTarget field that stores a direct reference to the element the listener was attached to, so removeEventListener always targets the correct node even after DOM replacement on mode switch.

3. Missing Error Handling in updateSpecificFacets — Fixed. Wrapped the async body in try/finally so this.facetsLoading = false always executes, preventing a stuck loading spinner on errors.

Important

4. Arrow Flash on Initial Render — Fixed. atScrollEnd now initializes to true (matching atScrollStart), so both arrows are hidden until the first requestAnimationFrame measurement in attachScrollListener.

5. Spurious emitPageClick on Initial Render — Fixed. Added a guard changed.get('currentPage') !== undefined so the event only fires on actual user-driven page changes, not the initial property hydration.

6. Hidden Scroll Arrows Keyboard-Focusable on Mobile — No change needed. The arrows use display: none on mobile via CSS media query, and elements with display: none are not focusable by the browser.

7. pageChanged Event Missing composed: true — Fixed. Added bubbles: true, composed: true to match the pattern used by facetsChanged.

Minor

8. ResizeObserver Unnecessary Re-Renders — Fixed. Added a value guard so isCompactView is only assigned when the compact state actually changes.

9. .gitignore Missing Trailing Newline — Fixed.