Skip to content

fix(catalog): preserve virtual result rows #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ian-pascoe merged 1 commit into from
Jun 28, 2026
+60 −5
Merged

fix(catalog): preserve virtual result rows #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 48 additions & 5 deletions apps/catalog/src/scripts/virtual-results.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export function initVirtualCatalogSearch(
const rows = parseRows(index.textContent ?? "[]");
let visibleRows = [...rows];
let lastFocusedControl: HTMLElement | null = null;
const renderedRows = new Map<string, HTMLElement>();
const virtualizer = new Virtualizer<Window, HTMLElement>({
count: visibleRows.length,
getScrollElement: () => window,
Expand Down Expand Up @@ -128,7 +129,36 @@ export function initVirtualCatalogSearch(
function renderVirtualRows(): void {
const items = virtualizer.getVirtualItems();
resultSpacerEl.style.height = `${Math.max(virtualizer.getTotalSize(), visibleRows.length ? estimateRowHeight() : 1)}px`;
resultListEl.replaceChildren(...items.map((item) => renderRow(item, visibleRows[item.index])));
const nextKeys = new Set<string>();
let cursor: ChildNode | null = resultListEl.firstChild;

for (const item of items) {
const row = visibleRows[item.index];
const key = virtualRowKey(item, row);
nextKeys.add(key);

let element = renderedRows.get(key);
if (!element) {
element = renderRow(item, row);
renderedRows.set(key, element);
} else {
updateRowPosition(element, item, row);
}

if (element !== cursor) {
resultListEl.insertBefore(element, cursor);
}
cursor = element.nextSibling;
}

for (const element of Array.from(
resultListEl.querySelectorAll<HTMLElement>("[data-result-row]"),
)) {
const key = element.dataset.virtualKey;
if (key && nextKeys.has(key)) continue;
element.remove();
if (key) renderedRows.delete(key);
}
}

function navigateFromRowClick(event: MouseEvent): void {
Expand Down Expand Up @@ -249,10 +279,7 @@ function renderRow(item: VirtualItem, row: CatalogSearchRow | undefined): HTMLEl
element.className = "catalog-result-row";
element.role = "row";
element.dataset.resultRow = "";
element.dataset.detailHref = row?.detailHref ?? "";
element.dataset.index = String(item.index);
element.setAttribute("aria-rowindex", String(item.index + 2));
element.style.transform = `translateY(${item.start}px)`;
updateRowPosition(element, item, row);
if (!row) return element;
element.innerHTML = `
<div class="catalog-result-row__name" role="cell">
Expand All @@ -278,6 +305,22 @@ function renderRow(item: VirtualItem, row: CatalogSearchRow | undefined): HTMLEl
return element;
}

function updateRowPosition(
element: HTMLElement,
item: VirtualItem,
row: CatalogSearchRow | undefined,
): void {
element.dataset.virtualKey = virtualRowKey(item, row);
element.dataset.detailHref = row?.detailHref ?? "";
element.dataset.index = String(item.index);
element.setAttribute("aria-rowindex", String(item.index + 2));
element.style.transform = `translateY(${item.start}px)`;
}

function virtualRowKey(item: VirtualItem, row: CatalogSearchRow | undefined): string {
return row?.id ?? String(item.key);
}
Comment on lines +320 to +322

@greptile-apps greptile-apps Bot Jun 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Index-based key collides across filter changes for ID-less rows

virtualRowKey falls back to String(item.key) when row.id is absent. item.key is the virtualizer's output of getItemKey, which is itself visibleRows[index]?.id ?? index — so for an ID-less row the key reduces to String(index). After applySearch replaces visibleRows, index N in the new filtered set may point to a completely different row than index N in the old set. renderedRows.get("N") returns the old element, and because updateRowPosition only patches transform/aria attributes (not innerHTML), the row is displayed with the wrong name, description, and icon. The cleanup pass does not evict it because "N" is still present in nextKeys. If CatalogSearchRow.id is always defined in production data this is dormant, but the explicit fallback in both getItemKey and virtualRowKey signals the type permits absent IDs. Clearing renderedRows (or at minimum evicting non-ID entries) at the start of applySearch before calling renderVirtualRows would close the gap.

Fix in Codex


function renderCapletIcon(row: CatalogSearchRow): string {
if (!row.icon) {
return `<span class="catalog-result-row__icon catalog-result-row__icon--fallback" aria-hidden="true">${escapeHtml(row.name.slice(0, 1).toUpperCase())}</span>`;
Expand Down
12 changes: 12 additions & 0 deletions apps/catalog/test/virtual-results.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ describe("virtual catalog results", () => {
expect(icon.getAttribute("loading")).toBe("lazy");
});

it("reuses row nodes that remain visible while scrolling", async () => {
mountSearchShell(manyCatalogSearchRows(200));

const { initVirtualCatalogSearch } = await import("../src/scripts/virtual-results");
initVirtualCatalogSearch();
const firstRow = resultRows()[0];

window.scrollTo({ top: 72 });

expect(resultRows()).toContain(firstRow);
});

it("hydrates filters from the url and resets to the first row", async () => {
mountSearchShell(manyCatalogSearchRows(80), "http://localhost:3000/?q=caplet-70");

Expand Down