refactor(frontend): consolidate column-filter popover plumbing into shared lib

[cristim]

Background

PR LeanerCloud/cloud-commitments-cli#570 extracted parseNumericFilter + applyColumnFilters<TRow, TColumnId> into frontend/src/lib/column-filters.ts. Three follow-up PRs (LeanerCloud/cloud-commitments-cli#789 RI Exchange, LeanerCloud/cloud-commitments-cli#790 History, LeanerCloud/cloud-commitments-cli#791 Plans) wired the lib into the three deferred consumers. All four pages now use the shared parser + filter pipeline.

However, the popover UI layer was NOT extracted. Each consumer has its own ~280 LOC of structurally-similar popover plumbing — open/close lifecycle, position-relative-to-trigger, build-popover-DOM, resync-on-rebind, attach/detach event handlers, click-outside-to-close, ESC-to-close. Four near-identical copies now live in:

• frontend/src/recommendations.ts (the canonical, pre-existing one)
• frontend/src/plans.ts (PR feat(frontend/plans): inline column filters via shared lib (refs #166) cloud-commitments-cli#791)
• frontend/src/history.ts + frontend/src/lib/history-filter-popover.ts (PR feat(frontend/history): inline column filters via shared lib (refs #166) cloud-commitments-cli#790 — already lifted a partial helper; the queue + history tables share it)
• frontend/src/riexchange.ts (PR feat(frontend/riexchange): inline column filters via shared lib (refs #166) cloud-commitments-cli#789)

Each of the three follow-up PR agents independently flagged this duplication AND independently judged it premature to extract until all four call sites were visible — scope-discipline call. Now that they ARE all visible, the consolidation is the next logical refactor.

What we want

Lift the popover skeleton into a shared frontend/src/lib/column-filter-popover.ts (or extend the existing lib/history-filter-popover.ts if its API is close enough). The helper takes the call site's state-accessor trio as parameters:

export interface ColumnFilterPopoverDeps<ColumnId extends string> {
    getFilters(): Partial<Record<ColumnId, ColumnFilterKind>>;
    setFilter(col: ColumnId, kind: ColumnFilterKind | undefined): void;
    clearAll(): void;
    distinctCategoricalValues(col: ColumnId): string[];
    columnLabel(col: ColumnId): string;
}

export function attachColumnFilterPopover<ColumnId extends string>(
    triggerEl: HTMLElement,
    columnId: ColumnId,
    deps: ColumnFilterPopoverDeps<ColumnId>,
): () => void;

Returns a teardown function. The helper owns:

• Build popover DOM (text input for numeric expr; checkbox group for categorical)
• Position relative to trigger (existing logic — usually below + clamped to viewport)
• Click-outside / ESC / blur to close
• Resync open popover on filter-state change from elsewhere
• Inline error rendering for invalid expr (the lib already returns ParsedNumericFilter.error)

Each consumer's existing per-page popover code shrinks to:

attachColumnFilterPopover(triggerEl, 'count', {
    getFilters: getPlansColumnFilters,
    setFilter: setPlansColumnFilter,
    clearAll: clearAllPlansColumnFilters,
    distinctCategoricalValues: distinctPlansValueExtractor,
    columnLabel: plansColumnLabel,
});

Estimated impact

Net LOC removed across the 4 consumers: ~600-900 lines (4 × ~150-220 LOC of popover plumbing per consumer minus the ~250 LOC of the new shared helper). Test surface area shrinks too — popover-rendering tests can mostly move to the lib's own test file, with each consumer keeping 1-2 thin integration tests asserting the right state slice is hit.

Sequencing

Do this AFTER LeanerCloud/cloud-commitments-cli#789, LeanerCloud/cloud-commitments-cli#790, LeanerCloud/cloud-commitments-cli#791 all merge. Doing it in parallel with those would create a fourth concurrent change to the popover layer and turn merges into a 4-way conflict mess.

Why not consolidate up-front

Each of the three follow-up PR agents (Plans, History, RI Exchange) flagged this duplication in their reports and judged it premature to extract until all four call sites were visible. That call was correct — extracting on partial data risks bad abstraction shape (e.g. the recommendations popover has SP-group + resync-on-rebind + MutationObserver hooks that the simpler tables don't need, and projecting them onto the History tables would have leaked complexity).

Now that all four call sites are visible AND LeanerCloud/cloud-commitments-cli#790 has already lifted a partial lib/history-filter-popover.ts helper (the History queue + history tables share it), the abstraction shape is grounded in actual usage.

Test plan

• New frontend/src/__tests__/column-filter-popover.test.ts covering: open/close, click-outside, ESC, set filter writes through deps.setFilter, clear via deps.clearAll, invalid expr shows inline error, resync when external state changes.
• Each consumer's existing column-filter regression suite (plans-column-filters.test.ts, history-column-filters.test.ts, approval-queue-column-filters.test.ts, riexchange-column-filters.test.ts, recommendations.test.ts popover cases) keeps working as a thin integration test.

Related

• refactor(frontend): extract column-filter primitives into a shared module — adopt on Plans / History / RI Exchange cloud-commitments-cli#166 (the original consolidation issue)
• PR refactor(frontend): extract parseNumericFilter to shared column-filter lib cloud-commitments-cli#570 (extraction of parseNumericFilter + applyColumnFilters)
• PR feat(frontend/riexchange): inline column filters via shared lib (refs #166) cloud-commitments-cli#789, feat(frontend/history): inline column filters via shared lib (refs #166) cloud-commitments-cli#790, feat(frontend/plans): inline column filters via shared lib (refs #166) cloud-commitments-cli#791 (the three consumer wirings — all must merge first)