Skip to content

feat: [ENG-2547] M2.17 WebUI numbered pagination + Filter dropdown + active filter tags#598

Merged
ncnthien merged 5 commits intoproj/persis-task-historyfrom
feat/ENG-2547
May 4, 2026
Merged

feat: [ENG-2547] M2.17 WebUI numbered pagination + Filter dropdown + active filter tags#598
ncnthien merged 5 commits intoproj/persis-task-historyfrom
feat/ENG-2547

Conversation

@ncnthien
Copy link
Copy Markdown
Collaborator

@ncnthien ncnthien commented May 4, 2026

Summary

  • Replace cursor useInfiniteQuery with single useQuery numbered pagination (page / pageSize); shared Pagination primitives + page-size Select.
  • New unified Filter dropdown (task-filter-menu.tsx) with submenus: Type, Provider, Model, Time (Calendar via task-date-filter-panel.tsx), Duration. Active dot indicator on the trigger.
  • New task-filter-tags.tsx row renders each active filter as a removable shared Tag plus a "Clear filters" CTA.
  • URL is the source of truth via use-task-filter-params.ts (status/types/providers/models/from/to/duration/q/page/pageSize) — shareable URLs, browser back/forward work, and filter changes auto-reset to page 1.
  • Status pills use a second useGetTasks call (no status filter) so per-status counts stay stable when the active filter has zero matches.
  • task-subscription-initializer.tsx invalidates ['tasks', 'list'] on the six task lifecycle events (CREATED / STARTED / COMPLETED / ERROR / CANCELLED / DELETED).
  • Type-guarded URL parsing (no as casts), Record<…>-based status/duration lookups, useMemo for derived filter args / provider Maps / model options, and cn({…: cond}) for conditional pagination classes.
  • Native title tooltip on the truncated Input cell + leading-none fix for the TYPE badge optical centering.

Test plan

  • Open WebUI with >50 tasks — list shows the first page; numbered pagination + page-size selector visible.
  • Click page 2/3/last — URL updates with ?page=N; correct slice loads.
  • Open the Filter dropdown:
    • Type submenu — toggle Curate / Query; rows filter; active dot appears on trigger.
    • Provider submenu — only providers from the index appear; toggling filters the list and narrows the Model submenu.
    • Model submenu — choose a model; rows filter.
    • Time submenu — pick a date range on the Calendar; click Apply; rows filter; Clear resets.
    • Duration submenu — radio select a bucket; rows filter.
  • Apply 3 filters — three Tags appear; click each X to remove individually; "Clear filters" wipes everything.
  • Search box (debounced 300ms) — types narrow results; URL gets ?q=....
  • Apply a filter that returns zero rows for one status pill — other pill counts stay accurate (not zeroed out).
  • Refresh page with all filters in URL — same filtered view restored.
  • Switch projects — page resets, filters re-apply for the new project.
  • Hover a truncated input cell — full prompt appears in the native tooltip.
  • CURATE / QUERY badges sit vertically centered in the Type column.
  • npm run typecheck && npm run lint clean.
  • npx mocha --forbid-only "test/unit/webui/features/tasks/**/*.test.ts" — all passing.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Claude finished @ncnthien's task in 3m 23s —— View job


PR Review: feat: [ENG-2547] M2.17 WebUI numbered pagination + Filter dropdown + active filter tags

  • Gather context and understand the PR
  • Read changed files
  • Review correctness, security, and code quality
  • Post final review with inline comments and summary

Summary

Score: 4/5

A well-structured refactor that correctly migrates from useInfiniteQuery cursor pagination to server-side numbered pagination, and introduces a solid URL-as-source-of-truth filter model. The useTaskFilterParams hook is clean, the type guards are proper (no as casts), and the useMemo usage for derived filter args is correct. Test coverage for all the new utilities is good.

There is one real bug and a few architectural concerns worth addressing before merge.


Issues Found

Bug

  • closeTask not stable (task-list-view.tsx): closeTask is an inline arrow function used as a useEffect dependency (line 165). Since it's not wrapped in useCallback, a new reference is created on every render, causing the retry effect to fire on every render whenever retrySeed is set. See inline comment on line 83.

Architecture / Performance

  • Duplicate useGetProviders call (task-filter-menu.tsx:63): The component fetches providers internally to build a providerNames Map, but the parent task-list-view.tsx already calls the same hook and passes providers down as a prop to TaskFilterTags. TaskFilterMenu could simply accept a providerNames: Map<string, string> prop (or a providers array) rather than issuing a second hook call.
  • Extra useGetTasks counts call (task-list-view.tsx:108): The second call with pageSize: 1 fires two requests on every filter change. If the server always includes aggregate counts in the primary response's counts field (which it appears to, based on the shape), the second call may be avoidable. See inline comment.
  • useGetTasks removed queryConfig (get-tasks.ts:22): The rest of the codebase uses a queryConfig escape hatch for enabled/staleTime overrides. This hook now diverges from that pattern and loses the ability to be conditionally enabled. See inline comment.

Code Quality

  • Duplicate isDurationPreset type guard (use-task-filter-params.ts:28): Also defined in task-filter-menu.tsx:28 with a derived set, vs. a hardcoded one here. Export a single canonical guard from duration-presets.ts.
  • tags array not memoized (task-filter-tags.tsx:59): Rebuilt on every render via imperative pushes. Wrap in useMemo. See inline comment.
  • ellipsis-${idx} key (task-list-pagination.tsx:61): Use 'ellipsis-left' / 'ellipsis-right' stable string keys instead of index. See inline comment.
  • selected?.from !== null (task-date-filter-panel.tsx:23): Always false since DateRange.from is typed as Date | undefined. See inline comment.

Missing Test Coverage

use-task-filter-params.ts (the URL parsing/serialization hook) has no dedicated tests despite being the core routing/state primitive. Boundary cases like ?page=0, ?page=abc, out-of-range pageSize, unknown status values, and multi-value comma lists are handled in parseFilters but untested. Given CLAUDE.md's 80% minimum and "critical paths must be covered" requirement, this should have test coverage.

Minor UX Note

NoMatchState in task-list-table.tsx (the search-query-aware empty state with a "Clear search" link) is now a dead code path: since filtering is fully server-side, tasks.length === 0 is always caught by EmptyState in task-list-view.tsx before the table renders. The search-specific empty UX within the table no longer fires. Intentional or oversight?


What's Good

  • URL-as-truth design is clean: useTaskFilterParams returns stable setters, resets page on filter changes, and handles empty/default values correctly (no polluting the URL with default params).
  • Type-guarded parseFilters with no as casts — good.
  • nonStatusFilters memoized correctly, avoiding query refetches on unrelated renders.
  • task-subscription-initializer.tsx cleanup is correct — invalidateQueries on lifecycle events is the right approach for React Query.
  • pageNumbersToShow algorithm handles edge cases (≤7 pages, adjacent ellipsis) well.
  • Tests for all pure utilities (duration-presets, status-filter-to-server, time-presets, get-tasks) are well-written and cover the important paths.

Comment thread src/webui/features/tasks/components/task-list-view.tsx
Comment thread src/webui/features/tasks/components/task-list-view.tsx
Comment thread src/webui/features/tasks/components/task-filter-menu.tsx Outdated
Comment thread src/webui/features/tasks/components/task-list-pagination.tsx
Comment thread src/webui/features/tasks/components/task-date-filter-panel.tsx
Comment thread src/webui/features/tasks/hooks/use-task-filter-params.ts
Comment thread src/webui/features/tasks/components/task-filter-tags.tsx Outdated
Comment thread src/webui/features/tasks/api/get-tasks.ts
…edup useGetProviders + isDurationPreset, memo tags
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Claude finished @ncnthien's task in 2m 30s —— View job


PR Review: feat: [ENG-2547] M2.17 WebUI numbered pagination + Filter dropdown + active filter tags

  • Gather context and understand the PR
  • Read changed files
  • Review correctness, security, and code quality
  • Post final review with inline comments and summary

Summary

Score: 4/5

The latest commit (ed0d3b2) addressed 5 of the 9 issues from the previous bot review — good iteration. The codebase is now in solid shape. Remaining items are minor-to-medium in severity; the one notable gap is missing test coverage for use-task-filter-params.ts, which CLAUDE.md requires for critical paths.


What Was Fixed ✅

  • closeTask is now wrapped in useCallback — the retry effect no longer fires on every render.
  • serverStatus is now memoized with useMemo.
  • isDurationPreset is consolidated — use-task-filter-params.ts imports from duration-presets.ts instead of redefining it.
  • TaskFilterMenu no longer calls useGetProviders() internally; it accepts providers as a prop.
  • tags in task-filter-tags.tsx is now built inside useMemo.

Remaining Issues

Missing Test Coverage (Medium — CLAUDE.md violation)

use-task-filter-params.ts — the hook that owns all URL parsing/serialization — has no test file. This is the most critical routing primitive in this PR, yet boundary cases remain untested:

  • ?page=0Number.parseInt("0", 10) || DEFAULT_PAGE_SIZE silently falls back to 50 (0 is falsy)
  • ?page=abcparseInt returns NaN, NaN || 1 gives 1 — unspecified but works
  • ?duration=bogusisDurationPreset returns false, falls back to 'all' — correct but untested
  • Multi-value comma parsing: ?models=a,b,c
  • setPage(1) → deletes ?page from URL (URL cleanliness)
  • clearAllFilters → removes all filter keys but preserves other params

CLAUDE.md mandates 80% coverage minimum and that critical paths must be covered. Add tests for this hook →

Redundant Null Check (Nitpick)

task-date-filter-panel.tsx:23

const hasSelection = selected?.from !== undefined && selected?.from !== null

DateRange.from is typed as Date | undefined — it is never null. The !== null check is always true and provides no guard. Use selected?.from !== undefined or the truthiness Boolean(selected?.from).

Fix this →

Unstable Ellipsis Keys (Nitpick)

task-list-pagination.tsx:61

<PaginationItem key={`ellipsis-${idx}`}>

Array index as part of a key for items whose position can shift when the current page changes. Since there are at most two ellipsis items (left and right), stable string keys would be cleaner:

<PaginationItem key={idx === 1 ? 'ellipsis-left' : 'ellipsis-right'}>

Fix this →

Double-Fetch for Counts (Performance — Medium)

task-list-view.tsx:108

const {data: countsData} = useGetTasks({page: 1, pageSize: 1, ...nonStatusFilters})

This fires a second request on every filter change purely to get stable per-status counts. If the server always returns aggregate counts in the counts field regardless of the status filter (which it likely does, since the primary query also has a counts key), then the primary data.counts already carries what you need.

If the two-request pattern is intentional (e.g. the server only returns counts for the filtered status bucket), add a comment explaining why — the coupling is non-obvious to future readers.

queryConfig Escape Hatch Removed (Minor Architecture)

src/webui/features/tasks/api/get-tasks.ts:19

export const useGetTasks = (options: UseGetTasksOptions = {}) =>
  useQuery({
    queryFn: () => getTasks(options),
    queryKey: ['tasks', 'list', options],
  })

The previous version exposed a queryConfig prop for callers needing enabled/staleTime overrides. This diverges from the rest of the codebase's pattern. Consider adding an optional queryConfig?: { enabled?: boolean } to UseGetTasksOptions for parity.

NoMatchState Dead Code (Minor UX)

task-list-table.tsx:87–90

{filtered.length === 0 ? (
  <TableRow><NoMatchState /></TableRow>
) : (

Since filtered is a sorted version of tasks, and TaskListView guards tasks.length === 0 before rendering TaskTable at all, filtered.length === 0 can never be true inside TaskTable. NoMatchState with its search-specific "Clear search" link is effectively unreachable. Intentional or oversight?


What's Good

  • All five previous-review fixes landed cleanly — no regressions introduced.
  • pageNumbersToShow algorithm handles edge cases (≤7 pages, left/right ellipsis) correctly.
  • nonStatusFilters memoized correctly; debouncedSearch keeps query args stable until user stops typing.
  • use-task-filter-params.ts setters are stable (useCallback/useMemo) — no spurious re-renders from the hook itself.
  • TaskSubscriptionInitializer lifecycle event invalidations are clean and correct.
  • setPage(1) deletes ?page from the URL rather than writing ?page=1 — keeps URLs clean.

wzlng
wzlng previously approved these changes May 4, 2026
…eat/ENG-2547

# Conflicts:
#	src/webui/features/tasks/components/task-list-view.tsx
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Claude finished @ncnthien's task in 3m 5s —— View job


PR Review: feat: [ENG-2547] M2.17 WebUI numbered pagination + Filter dropdown + active filter tags

  • Gather context and understand the PR
  • Read changed files and check prior review issue status
  • Review correctness, security, and code quality
  • Post final review with inline comments and summary

Summary

Score: 4/5

The latest push is a merge from origin/proj/persis-task-history (resolving a conflict in task-list-view.tsx). It brings in bulk-delete, delete, clear-completed, and task-detail APIs from the base branch — these integrate cleanly with ENG-2547's filter/pagination model. No regressions introduced by the merge.

Five issues from the previous review were fixed in ed0d3b2. The remaining items are the same four from the prior round plus the ongoing test coverage gap; the double-fetch pattern was confirmed intentional.


What Was Fixed in This Iteration ✅

Everything landed by ed0d3b2 is still intact after the merge:

  • closeTask wrapped in useCallback — retry effect no longer fires on every render.
  • serverStatus memoized with useMemo.
  • isDurationPreset consolidated — imported from duration-presets.ts, no duplication.
  • TaskFilterMenu accepts providers as a prop, no internal useGetProviders() call.
  • tags in task-filter-tags.tsx built inside useMemo.

Remaining Issues

Missing Test Coverage (Medium — CLAUDE.md violation)

src/webui/features/tasks/hooks/use-task-filter-params.tsparseFilters and the URL setters have no test file after three review rounds. This is the most critical routing primitive in the PR. CLAUDE.md mandates 80% minimum coverage and "critical paths must be covered."

Untested boundary cases:

  • ?page=0parseInt("0") || 1 silently gives 1 (0 is falsy — could surprise callers expecting page 0 to remain as-is)
  • ?page=abcNaN || 11
  • ?duration=bogus → falls back to 'all'
  • ?models=a,b,c → parses to ['a', 'b', 'c']
  • setPage(1) deletes ?page (URL stays clean)
  • clearAllFilters preserves non-filter params like ?task=...

Add tests →

Redundant Null Check (Nitpick — third round)

task-date-filter-panel.tsx:23selected?.from !== null is always true since DateRange.from is Date | undefined. Inline comment posted.

Unstable Ellipsis Keys (Nitpick — third round)

task-list-pagination.tsx:61ellipsis-${idx} uses the array index as a key component. Stable 'ellipsis-left' / 'ellipsis-right' keys would be cleaner. Inline comment posted.

queryConfig Escape Hatch Removed (Minor Architecture)

src/webui/features/tasks/api/get-tasks.ts:19 — The hook no longer accepts enabled/staleTime overrides. Low-priority if no current caller needs it, but it diverges from the rest of the codebase's pattern. Inline comment posted.

NoMatchState Dead Code (Minor UX)

task-list-table.tsx — The NoMatchState empty state inside TaskTable is unreachable: TaskListView guards tasks.length === 0 before rendering TaskTable, and filtered is just a sorted copy of tasks. The "Clear search" link never fires. The line falls outside the PR diff so no inline comment, but it's worth resolving in a follow-up (either remove it or move a search-specific empty state up to TaskListView).

Double-Fetch for Counts (Confirmed Intentional)

task-list-view.tsx:112 — Confirmed intentional per ncnthien's reply: the server only returns counts for the filtered status bucket, so a second status-less call is required for stable pill counts. A comment in the code explaining this would help future readers, since the coupling isn't obvious.


What's Good

  • Merge conflict in task-list-view.tsx resolved correctly — bulk-delete, delete, and clear-completed mutations coexist cleanly with ENG-2547's filter/pagination model.
  • URL-as-truth design is solid: useTaskFilterParams setters are stable, page resets on filter changes, setPage(1) keeps URLs clean, clearAllFilters preserves non-filter params.
  • Type-guarded parseFilters — no as casts anywhere in the hook.
  • nonStatusFilters and durationRange correctly memoized to avoid spurious query refetches.
  • pageNumbersToShow algorithm handles ≤7 pages and left/right ellipsis correctly.
  • TaskSubscriptionInitializer lifecycle invalidations are correct and clean.

Comment thread src/webui/features/tasks/components/task-date-filter-panel.tsx
Comment thread src/webui/features/tasks/components/task-list-pagination.tsx
Comment thread src/webui/features/tasks/hooks/use-task-filter-params.ts
Comment thread src/webui/features/tasks/api/get-tasks.ts
@ncnthien ncnthien merged commit 66becd6 into proj/persis-task-history May 4, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants