feat(knowledge-base): server-side pagination, URL-state filters, sort, date/language facets, bulk delete and AI status - #264
Merged
Conversation
Renders an external link anchor for an artifact's source URL. Labels the link per source system (GitHub, Jira, Confluence, etc.) and falls back to a generic 'Open source' label for unknown systems. UPLOAD artifacts are excluded from the badge by convention. Includes unit tests covering all supported source system labels, null/whitespace URL guards, and link attribute correctness.
…ader Renders the new SourceLinkBadge alongside the RepositoryBadge (for GitHub artifacts that have both) or in place of it (for Jira, Confluence, and other connector artifacts). UPLOAD artifacts and artifacts with a null/whitespace sourceUrl show neither. Updates the ArtifactViewerDrawer unit tests to cover all four cases: GitHub with repo + sourceUrl, Jira, Confluence, UPLOAD suppression, and null/whitespace URL suppression.
…nges Opening or closing the artifact drawer updates the ?artifact= search param via setSearchParams. useScrollRestoration was treating every search-param change as a navigation that warranted a scroll-to-top, causing the page to jump back to y=0 whenever a drawer was opened or closed on a scrolled page. Two-part fix: - KnowledgeBasePage: pass preventScrollReset:true to setSearchParams so the router does not reset scroll on param-only changes. - useScrollRestoration: only scroll-to-top on Push/Replace navigations when the pathname itself changed (or on the initial render), and respect the preventScrollReset location-state flag. Adds a useScrollRestoration unit test that asserts scroll position is retained across ?artifact param open/close cycles on the same pathname.
Focus management in SidePanel was calling .focus() without preventScroll:true. On some browsers this caused the page to scroll to bring the newly focused element into view when the panel opened or closed, overriding the scroll position the user was at.
Root cause: the desktop sidebar used position:sticky. When SidePanel opens it calls useScrollLock, which applies overflow:hidden to <html>. This turns <html> into a scroll container with scrollTop=0. A sticky element's position is calculated relative to its nearest scroll container, so the sidebar snapped to document y=0 while the viewport was at y=400+, making it appear to scroll up or disappear entirely. Fix: - SideBar: change the desktop aside from sticky to position:fixed (top-0 bottom-0 left-0). Fixed elements are always positioned relative to the viewport and are immune to scroll-container changes. - App: add lg:ml-[286px] to <main> when signed in and not in focus mode, so the content area starts after the now out-of-flow sidebar. margin-left shifts the element box itself (padding-left would have left the box at x=0 and blocked pointer events on the sidebar). - useScrollLock: add scrollTop snapshot/restore to lockElement and restore window.scrollY after unlocking, so page position is preserved on panel close.
Follows up the server-side pagination and faceted search work with the findings from a full review of both sides of the change. Everything here is either a correctness fix or the removal of surface that stopped meaning what its name says. Removed dead surface: * `knowledgeService.getUnifiedArtifacts` — after the move to server-side paging it returned page 1 capped at 100 rows while still being called "unified", so the next caller would silently get a truncated corpus. `loadKnowledgeBaseArtifacts` went with it; the route prefetch already warms the paged queries directly. * `queryKeys.knowledgeBase.byProject` — a byte-identical duplicate of `.project`, which is the scope prefix every invalidation uses. * `filteredArtifacts` / `paginatedArtifacts` on the hook — three names for the current page. The hook returns `artifacts` once; the page and the tests read that. * `location.state.preventScrollReset` in `useScrollRestoration` — react-router carries that option on the navigation, never in `location.state`, so the guard was dead. A search-only navigation already keeps its scroll position, because the pathname rule covers it; the comment now says why. Contract tightened to what the backend sends: * `PageMetadata` declared `pageNumber`, `pageSize`, `isFirst`, `isLast` and an optional `metadata` envelope — none of which the endpoint has ever returned. They came from one stale fixture; all five are gone and the six real fields are required. Design system: * the sidebar's width and the margin the page leaves for it are now one token, `--app-sidebar-width`, instead of 286px written out three times. * the artifact viewer drawer is `w-full max-w-5xl` rather than an off-scale `max-w-[1000px]` plus two redundant breakpoints. Docs: three comments still pointed at the deleted method (the page TSDoc, the drawer's delete remarks, `getRecentArtifacts`'s note). Gates: lint, format:check, 348 files / 2972 tests, build.
daniilperkin
marked this pull request as draft
September 23, 2026 20:14
Every Knowledge Base filter used to live in seven `useState`s inside `useKnowledgeBase` (tab, search, sources, format, repositories, page and the `pagedProjectId` guard). A refresh threw them away, a link could not carry them and Back left the page instead of undoing the last click. This moves them into the query string and makes that the only place they are stored. New: `features/knowledge-base/hooks/useKnowledgeBaseUrlState.ts` * Params: `tab` (omitted for ALL), `q`, `sources`, `repos`, `format`, `page` (omitted for 1), `size` (omitted for 20), `artifact`. Short names because these URLs are meant to be pasted into chats and tickets; sets are written comma-separated (`?sources=GITHUB,JIRA&repos=acme/api`) with commas and slashes left unescaped, and repeated params are accepted on read too. * `parseKnowledgeBaseSearch` validates by dropping what cannot be honoured, never by failing: unknown tabs/sources/formats are ignored (no `types=BOGUS` 400), enums match case-insensitively because people type URLs, page and size are clamped to the backend's `@Min(1) @max(100)`. A `format` only counts while Uploads is selected and `repos` only while GitHub is - the same invariant the toggles enforce - so a hand-built link cannot carry a filter the panel does not even show. * History semantics: discrete choices (tab, facet, page, size, clear) push so Back undoes one click; search typing, the drawer's `?artifact=`, page clamping and the project-switch clean-up replace, so Back never walks through keystrokes or states nobody chose. A write that changes nothing adds no entry (Back would otherwise look broken). * Writes start from a ref holding the latest search string, not from the render snapshot. React Router 7.18 applies location updates inside `startTransition`, so two writes in one event (toggle Uploads then PDF, or a fast double click) would otherwise both start from the same stale snapshot and the second would undo the first. A test pins this. * It is the only writer of the page's query string: the `?artifact=` deep-link effect moved in from `KnowledgeBasePage`, because two independent writers of one `URLSearchParams` overwrite each other's params. Project switch (fact-check correction 2, and one step further) * The page resolves `selectedProjectId || profile.projectIds[0]`, and the project context's selection is `""` until its list has loaded. So on a cold load `projectId` goes null -> fallback -> stored selection. Treating either hop as a switch wiped the filters of every shared link on arrival. The hook only clears when a *settled* project changes to another settled one; the page passes `projectSettled: !isProjectLoading`. A page test reproduces the loading -> loaded sequence and fails without the flag. * On a real switch the project-scoped params (everything but `size`, which is a reading preference) go in one `replace`. Until that navigation lands, the hook already reports the cleared state (the stale query string is masked), so no request is ever sent for project B with project A's filters and the panel never flashes them. This replaces the old render-phase reset block; `pagedProjectId` is gone because nothing is keyed by it any more. Page clamping (fact-check correction 1) * The old hook called `setCurrentPage(totalPages)` during render. Navigation cannot happen during render, so the hook now derives the clamped page for rendering and writes it back with a `replace` in an effect. * It only clamps against an answer for *this* query: with `keepPreviousData` the placeholder still describes the previous filter, and clamping against it would drag a Back navigation to page 3 down to the previous filter's single page. Search input * The input keeps a local copy of `?q=` and follows the URL only on a POP navigation (Back/Forward) or a project switch. Every other change of `?q=` was written from the input itself, and adopting that echo a transition late would eat the characters typed in between. `useDeferredValue` went away: the list is now keyed on the router location, which the router already updates inside a transition. API: `useKnowledgeBase` keeps every existing field and adds `pageSize`, `setPageSize`, `selectedArtifactId` and `setSelectedArtifactId`, plus an optional second argument forwarded to the URL hook. Nothing in the rendered page changes; the route prefetch key still matches the default state (undefined params are dropped from TanStack's key hash). Tests * new `useKnowledgeBaseUrlState.test.ts` (30): parsing/validation/clamping, serialisation, push vs replace via real Back navigations in a MemoryRouter, no-op writes, several writes in one act, dependent facets, clear keeps size + artifact, all project-switch cases including "the very render that sees project B reports B's clean state". * `useKnowledgeBase.test.ts`: every render now has a router; +5 tests (a shared link produces exactly its request, clamped page is written back without a history entry, Back restores filters *and* the search input, `?artifact=` round trip, project switch empties the input). * `KnowledgeBasePage.test.tsx`: +1 (filters survive the project context resolving). Gates: lint, format:check, build; knowledge-base suites 14 files / 215 tests green (baseline full suite before this series: 348 files / 2972 tests).
…d server After the previous commit `?q=` was written on every keystroke, and both the list and the facets queries are keyed on it - so typing "readme" cost six list requests and six facet requests, five of them for words nobody was looking for. Server-side search makes each of them a database query. New `src/hooks/useDebouncedValue.ts`: * a small typed `useDebouncedValue<T>(value, delayMs)`; `src/hooks` had no debounce helper and this is the only shape the page needs (a value, not a debounced callback - the consumer is a query key, not an event). * the first render returns the value unchanged, so a deep-linked `?q=` is searched immediately rather than 300 ms after the page opened. * tests (fake timers): initial value, the exact delay boundary (299 ms vs 300 ms), a five-keystroke burst emitting only the last value, and no update after unmount. `useKnowledgeBase`: * the input keeps its local `searchQuery` (it has to anyway - see the previous commit: react-router applies location changes inside a transition, so a field bound to the URL would lag). Only `useDebouncedValue(searchQuery, 300)` is written to `?q=`, still with `replace`, and the queries read `?q=` - so a burst of keystrokes is exactly one list request and one facets request. * `KB_SEARCH_DEBOUNCE_MS = 300` is exported and documented: below the pause between words, above the gap between keystrokes. * the write lives in an effect guarded by a ref holding the last settled value it acted on. The effect also re-runs when `?q=` changes, and without the guard Back would break: Back moves `?q=` to the restored text immediately, but the debounced copy of that text only catches up 300 ms later, so the effect would see "settled value differs from URL" and write the previous settled text back over the one the user just navigated to. With the guard it only writes when the settled value itself changed. A test covers exactly this. * `hasActiveFilters` still reads the live input, so "Clear filters" appears on the first character rather than when the debounce fires (plan requirement). * `handleSearchChange` now only updates the input; the page reset still happens together with the `?q=` write, in the same history entry. * The plan asked to keep `useDeferredValue`. It was already removed in the previous commit and stays removed: the query key is fed from the router location, which react-router 7 updates inside `startTransition`, so the deferral it provided is already built in. A second deferral of the same value would only add a render. `ArtifactFilters`: * a muted "Searches titles and links" hint under the search box, wired with `aria-describedby` (via `useId`) so screen readers announce it with the field. The backend matches title and source link only, never content; without the hint a phrase from inside a document returning nothing reads as "the knowledge base does not have it" instead of "search does not look there". * the hint is plain text in `app-text-muted`; no new colour, no new primitive. Tests: * useKnowledgeBase: a five-keystroke burst is one `getArtifactPage` and one `getArtifactFacets` call with the settled text; the URL stays untouched until the debounce fires while `searchQuery`/`hasActiveFilters` follow every key; Back restores text that is not overwritten after the debounce window. * the P1 "restores filters and the search input on Back" test now waits for `?q=` to settle before clicking a source - with the debounce a click inside the window folds the text into the click's own history entry, which is the intended behaviour, not a regression. * ArtifactFilters: the hint text is rendered and is the field's accessible description. Gates: lint, format:check, build; vitest knowledge-base + page + a11y + useDebouncedValue: 15 files / 222 tests.
Phase 3 of the Knowledge Base upgrade. With server-side paging in place the
list can be thousands of artifacts deep and a GitHub org can bring dozens of
repositories into the source menu. This commit is about the reader finding
their way through that: where they are in the result, how many rows they see,
how a long facet list is searched, and what the counts in the menu mean.
MultiSelectFilter (ui primitive, backwards compatible):
* New optional section fields `searchable` and `visibleLimit`, and a new
optional `footnote` prop. All three default to off, so a caller that passes
none of them renders exactly as before. ArtifactFilters is the component's
only consumer; FilterSelect merely shares usePopoverMenu with it and is not
touched (its tests pass unchanged). The existing MultiSelectFilter test
file still passes as-is and was extended rather than duplicated.
* `searchable` adds a filter box above a section's options, but only once the
section holds more than SECTION_SEARCH_THRESHOLD (8) options. Opt-in per
section so a four-item Sources list never grows a search box; threshold
inside the primitive so each caller does not reinvent "when is a list long".
The box is the ui/Input primitive (size sm), labelled "Filter <section>",
and matching is a case-insensitive substring on the visible label - the
text the reader is actually scanning.
* `visibleLimit` folds a long section to its first N options with a
"Show all (N)" / "Show fewer" ghost Button. Ticked options stay visible
whatever their position, because a filter the reader cannot see is a filter
they cannot untick. A typed query ignores the limit: when someone searches,
every match is shown. Fold and query state are kept per section id, so
opening one section's list does not expand another's.
* A query with no matches says "No matches" instead of leaving an empty group.
* `footnote` renders a muted line at the foot of the menu.
* Selection is still one shared set keyed by value (unchanged), so the test id
convention `${testId}-option-${value.toLowerCase()}` is untouched; new
targets are `${testId}-section-${id}-search|show-all|no-matches` and
`${testId}-footnote`. The show-all button toggles (aria-expanded) between
"Show all (N)" and "Show fewer" rather than being two controls.
ArtifactFilters:
* Repositories section is `searchable` with `visibleLimit: 10` (the plan's
numbers). Sources and File format stay short fixed lists.
* Menu footnote "Counts show what you would get if you added this option."
The counts follow the standard facet rule (each dimension counted without
its own selection), so a ticked GitHub next to "Jira 40" does not mean 40 of
the current rows are Jira - it means ticking Jira adds 40. Readers kept
misreading that; one sentence at the point of use is cheaper than a tooltip.
* The result line now reads "21–40 of 412 artifacts" (or "412 artifacts" when
one page holds everything, "1 artifact", "No artifacts") via the new pure
`formatResultRange` in features/knowledge-base/resultRange.ts. It lives in
its own module because react-refresh forbids non-component exports from a
component file, and the page's live region words the total the same way.
`resultCount` keeps its meaning (total across pages); the new optional
`resultRange` carries the 1-based positions on this page.
useKnowledgeBase:
* Returns `resultRange`, computed from the page metadata the server answered
with (number/size) plus the number of rows actually on the page, not from
the requested page - while placeholder data is on screen the range must
describe the rows the reader is looking at.
Page size:
* New ArtifactPageSizeSelect (ui/Select, visible "Per page" label via useId,
data-testid kb-page-size) offering PAGE_SIZE_OPTIONS = 20/50/100, which now
live beside DEFAULT_PAGE_SIZE/MAX_PAGE_SIZE in useKnowledgeBaseUrlState.
A sibling of Pagination rather than a Pagination prop: Pagination returns
null for a single page and is shared by five other screens, while the size
control must stay reachable exactly when everything fits on one page.
It shows once the total exceeds the smallest size. A hand-edited ?size=35
is shown as its own option instead of the control silently claiming 20.
* Changing it goes through setPageSize: push navigation, page reset to 1.
* Pagination keeps its own mt-6; the footer wrapper gives the select the same
top margin so the two align without fighting over conflicting utilities.
Accessibility:
* A visually hidden `aria-live="polite"` `aria-atomic` region announces
"412 artifacts" once the total has been stable for 600 ms (the phase-2
useDebouncedValue), so ticking four facets in a row is one announcement,
not four; it stays empty while loading or failing so a stale total is never
read out. The existing `role="alert" aria-live="assertive"` element was
re-checked: it is the fetch-error banner, which is exactly what assertive is
reserved for, so it stays.
* New tests/unit/a11y/MultiSelectFilter.a11y.test.tsx runs axe against a
menu with a searchable folded section and a footnote.
Tests:
* MultiSelectFilter.test.tsx: threshold, filter box narrowing, No matches,
fold/expand/collapse, ticked options beyond the fold, footnote.
* ArtifactFilters.test.tsx: range wording, No artifacts, footnote, long repo
list gets the filter box and folds to ten.
* resultRange.test.ts, ArtifactPageSizeSelect.test.tsx: new.
* KnowledgeBasePage.test.tsx: polite live region announces the total; the
page-size select appears for 45 results and requests `size: 50, page: 1`.
Gates: lint, format:check, build; full vitest run 353 files / 3037 tests
(baseline 348 / 2972); npm run a11y 52 files / 64 tests.
Phase 4 of the Knowledge Base upgrade, frontend half. The backend gains a
`sort` parameter on the list endpoint (pinned contract): ADDED_DESC (default,
identical to today's ingestedAt DESC, id ASC), CHANGED_DESC
(coalesce(lastChangedAt, ingestedAt) DESC, id ASC) and TITLE_ASC
(lower(title) ASC NULLS LAST, id ASC). An unknown value is a 400; the facets
endpoint ignores sort entirely.
Type and labels
* `ArtifactSort` in types.ts, `KnowledgeListParams.sort?`.
* tabs.ts, next to the other facet label maps: `DEFAULT_ARTIFACT_SORT`,
`ARTIFACT_SORT_ORDER` and `SORT_LABELS` ("Newest added", "Recently
changed", "Title A–Z"). "Added" rather than "Newest" alone because the
default order is by ingestion time, and "Recently changed" is the order
that follows content changes - the two are different questions and the
labels have to say which one each answers.
Service: one filter builder for both endpoints
`getArtifactPage` and `getArtifactFacets` each had their own copy of the
search/types/sources/repositories/format serialisation. They are now one
`buildFilterQuery`, and the list adds only what is list-only: page, size and
sort. This is the parity guarantee the next two phases rely on (date range
and language must reach list AND facets): a filter added to the builder
reaches both calls by construction, so a facet can never count against a
different predicate than the rows it describes. The flip side is pinned too:
the facets call drops page/size/sort even when a caller passes them, because
a count must not depend on which page is on screen or how it is ordered.
Behaviour of the existing params is unchanged (search is still trimmed and
omitted when blank; sets are still repeated params).
URL state
* `?sort=` joins the param map. It is parsed case-insensitively and an
unknown value falls back to the default instead of being sent - the
backend 400s on unknown orders, and a hand-edited or stale link must not be
able to break the page.
* The default is omitted from the URL, like every other default.
* A sort change pushes history (Back restores the previous order) and drops
`?page=`: page 3 of a different order is an arbitrary slice of the result,
so the new order starts at its top.
* Sort is a reading preference, not a filter: it survives "Clear filters"
and a project switch (like `size`), and it is not counted by
`hasActiveFilters`, so choosing an order does not make "Clear filters"
appear.
Hook
`listParams.sort` is `undefined` for the default order. That keeps the
default request byte-identical to before (no `sort=ADDED_DESC` on the wire,
which also means an older backend that does not know the param sees exactly
what it saw yesterday) and keeps its React Query key identical to the one
`routePrefetch` warms, so hovering the nav link still pre-fills the first
page. `facetsParams` carries no sort, so changing the order never refetches
the counts.
UI
A native `ui/Select` ("Sort artifacts", `data-testid="kb-sort"`) in the
Tier-3 action row, between "Clear filters" and the source filter. Native on
purpose, per the Select primitive's own guidance: three fixed options need no
custom popup. `sort`/`onSortChange` are optional props on ArtifactFilters and
the control only renders when a handler is given, so the component stays
usable without it.
Fix carried along: `fieldClasses` makes every field `w-full`, so the `w-20`
the page-size select (previous commit) put on the `<select>` itself fought
the base class and its width depended on CSS order. Both it and the new sort
control now take their width from a wrapper, like the source filter does.
Tests
* knowledgeService: sort is sent with the list; omitted when not given;
facets never receive page/size/sort while still receiving the filters.
* useKnowledgeBaseUrlState: case-insensitive parse, unknown -> default;
default omitted; page reset; push (Back restores); survives Clear filters
and a project switch.
* useKnowledgeBase: list gets `sort` and page 1, facets never do; the
default request has `sort: undefined`; an order is not an active filter.
* ArtifactFilters: three options with their labels, change reported; no
control without a handler.
Gates: lint, format:check, build; vitest knowledge-base + page + service: 16 files / 255 tests.
Phase 5 of the Knowledge Base upgrade, frontend half. The pinned contract adds
`from` and `to` (yyyy-MM-dd, both inclusive) to the list AND the facets
endpoint, filtering on `ingestedAt` with UTC day boundaries; either end may be
absent and `from > to` is a 400.
Deviation from the plan, on purpose: the plan described an "Activity" filter
on COALESCE(lastChangedAt, ingestedAt). The user-approved contract filters on
`ingestedAt` only, and the contract overrides the plan. The UI therefore says
"Added" everywhere ("Filter by date added", "Added since …", "Added from" /
"Added to") - labelling it "Activity" or "Changed" would promise a predicate
the server does not run. It also lines up with the sort labels of the
previous commit ("Newest added" vs "Recently changed").
dateRange.ts (pure, tested in isolation)
* `isIsoDate` - shape check plus a round trip through Date, because
"2026-02-30" is well-formed but not a date and the backend would reject it.
* `todayUtc` - today as the server counts days. Deliberately not the local
date: west of UTC the local date can be a day behind, and a `to` of that
date would end before "now" and hide the newest artifacts. The UTC day that
contains now always contains now.
* Presets (Last 7 / 30 / 90 days) resolve to ABSOLUTE dates at the moment
they are picked (`resolvePreset`: from = today-(N-1), to = today, so "last
7 days" is 7 calendar days including today). Absolute on purpose: the URL
then holds `from=2026-09-18&to=2026-09-24`, and a link shared today shows
the same window next week instead of silently sliding with the calendar.
`matchPreset` recognises a range as a preset only while it still equals
one today; the same link a day later reads as a custom range.
* `normalizeDateRange` - invalid ends dropped, a reversed pair swapped. Used
by the URL parser and the writer, so nothing that reaches the request can
be a 400: a hand-edited link with from > to is far more likely a slip than
a request for nothing.
* Display uses the artifact cards' date style with `timeZone: "UTC"`; without
it every reader west of UTC would see the day before the one filtered on.
Service
`from`/`to` go into the shared `buildFilterQuery` from the previous commit,
so they reach the list and the facets by construction. A service test pins
exactly that difference against `sort` (list only).
URL state
`?from=` / `?to=` join the param map, are project-scoped (a switch drops
them - project A's window says nothing about project B) and are filters
("Clear filters" drops them). `setDateRange(range, mode)` normalises, removes
an open end, resets the page, and takes a history mode: presets and clearing
push; typing into a date field replaces, so one custom edit is one history
entry rather than one per completed segment of the native date control.
Hook
The filter criteria are now built once (`facetsParams`) and the list params
are that object plus page/size/sort, mirroring the service's builder: the two
requests cannot drift apart. `hasActiveFilters` includes the range;
`dateRange`/`setDateRange` are exposed.
UI: ArtifactDateRangeFilter (new, in the action row)
* A native `ui/Select`: Any time / Last 7 days / Last 30 days / Last 90 days
/ Custom range…. Native for the same reason as the sort control.
* "Custom range…" reveals two `ui/Input type="date"` fields ("Added from",
"Added to") with min/max hints. A reversed pair is refused with a visible
message, `aria-invalid` and `aria-describedby` on both fields, instead of
being written - colour is not the only signal. The component keeps a local
draft so a half-entered range is not pushed to the URL, and re-syncs the
draft when the range changes from outside (Back, Clear filters, the chip).
* An active range is named in a `ui/Badge` chip ("Added Sep 1, 2026 –
Sep 24, 2026", "Added since …") with an icon-only "Clear date range"
button next to it.
* Wired into ArtifactFilters through optional `dateRange` /
`onDateRangeChange` props (rendered only when a handler is given, like the
sort control); a stable empty-range constant avoids re-syncing the filter
every render when the prop is omitted.
Tests
* dateRange.test.ts: real-date check, UTC today (20:00 at UTC-8 is the next
UTC day), month/year shifts, preset resolution, preset matching expiring a
day later, normalisation (swap / drop), chip wording, UTC display.
* ArtifactDateRangeFilter: preset -> absolute dates + push; select shows the
matching preset or Custom; custom fields replace; reversed range refused
with an accessible error; chip clear pushes; external clear returns to Any
time. Plus an axe run with the custom fields and the error visible.
* useKnowledgeBaseUrlState: parse (valid / invalid / reversed), write and
page reset, push vs replace through Back, cleared by Clear filters and a
project switch.
* useKnowledgeBase: list and facets both receive the same from/to; a range
is an active filter that Clear filters removes.
* knowledgeService: from/to on both endpoints, sort on the list only.
Gates: lint, format:check, build; vitest knowledge-base + page + service + KB/date a11y: 20 files / 280 tests.
Adds the language facet from phase 6 of the KB plan. The backend now returns Artifact.language (display name derived from the file extension) and a `languages` facet; this commit makes both usable. URL: `?languages=` (comma list), project-scoped and a filter param like `sources`/`repos`, so it is dropped on a project switch, cleared by Clear filters and resets the page on change. Parsing de-duplicates ignoring case and the toggle folds case too: the backend matches case-insensitively, so a hand-typed `kotlin` must be undone by unticking "Kotlin", or the reader could not remove a filter they see. API: `languages` is sent repeated to the list AND the facets (it narrows the row set, so every count must respect it), via the shared buildFilterQuery, so the two requests cannot drift apart. Options: facet values with a count plus every selected value (count 0 when the facet omits it), matched ignoring case; a selected entry keeps the URL's spelling as its value so unticking hits it. Sorted alphabetically, not by count, so ticking one never reshuffles the list. The section is not gated on a source (a language narrows every source) and stays hidden when the project has no language values; `ArtifactFacets.languages` is optional so an older backend reads as "no languages" instead of crashing. ArtifactFilters prefixes language values with `lang:` inside the one shared selected set: languages are free strings, and an unprefixed "Markdown" would be mistaken for the MARKDOWN format both in the toggle routing and in the option test ids. The trigger summary names one language or counts several, like repositories. tabs.ts already read `language === "markdown"` when classifying uploads; now that the field is actually populated, tabs.test.ts pins that a Markdown upload without an extension is MARKDOWN and that "Kotlin"/"Plain Text" uploads fall to OTHER, while connector artifacts stay out of the format facet's scope. The mocked service in useKnowledgeBase.test.ts learns `languages` (own dimension excluded, document kinds dropped, selected zero-count returned) per plan risk 5, so tests assert the real request.
Phase 7 of the KB plan. The upload endpoint already takes a batch of
artifact ids; the page only ever sent one. PMs and admins can now
tick uploads on the visible page and delete them in one request.
Service: deleteUploads(projectId, uploadIds, removerId) sends the
batch and returns { deletedIds, failed }. An empty 204 (backend
predating the per-item answer) reads as "all deleted", its only
success signal. An id the new answer names in neither list counts as
failed: a deletion nobody confirmed must not be shown as a success.
deleteUpload now delegates to it and throws when its id is failed,
so the drawer's single delete stops being silent on a 200 failure
and the two paths cannot drift.
UI: an explicit Select toggle in the action row (aria-pressed, only
passed for PM/ADMIN, the same DELETE_ALLOWED_GROUPS gate as the
drawer) reveals checkboxes on upload cards only. The checkbox sits
beside the card, not inside it, so the card stays one button that
opens the drawer and no control nests in it. A toolbar ("N uploads
selected · Delete · Clear selection") opens a ui/Modal alertdialog
that reuses the drawer's consequence wording, including the removal
of the indexed content.
Outcome: "N deleted" in a polite live region, and a visible danger
alert naming each upload that could not be deleted with the
backend's reason. A failed request keeps the dialog open with the
error and the selection intact for a retry. Success invalidates the
knowledgeBase.project prefix so list, facets and detail refetch.
Selection is page-scoped: useUploadSelection keys it on the list's
identity (filters, page, size, sort, via listScopeKey), dropping it
on any change without an effect, and it is cleared on refresh and
after a delete. A selection that survives a filter change is a trap,
the reader can no longer see what they are about to delete. The
page also re-reads ticks against the visible uploads, so nothing
off screen is ever sent.
Tests: service (batch payload, 204, partial, unconfirmed ids,
single-path throw), selection hook, list checkboxes, bulk actions
(count, one request, invalidation, failures, request error), page
toggle gating and clearing, and an axe pass over the whole flow.
Phase 8 of the KB plan. Every card on the visible page gets a chip
saying what the AI assistant's index records for it, from one
batched GET /projects/{id}/artifacts/ai-status request per page.
Mapping per kb-contract part 3: INDEXED "Indexed", PROCESSING
"Indexing", FAILED "Failed", DEINDEXED/UNKNOWN "Not indexed". Each
chip is ui/Badge with icon + text + token colour, never colour
alone. The card is a role="button" whose children are
presentational, so the spoken form ("indexed for the AI
assistant", ...) is appended to the card's aria-label. The tooltip
says the chip reports the index record, not that the assistant
will find the content: a failed vector removal can leave the
record at "indexed" for a while.
No chip at all when aiAvailable is false (an AI outage answers
UNKNOWN for everything and must not print "Not indexed" on every
card), when the request fails, or for ids the response omits. A
response without aiAvailable (pre-amendment backend) is read as
unavailable.
useArtifactAiStatus keys on the visible page's ids (no request for
an empty page), retry false, staleTime 5 min, and polls every 10 s
only while some item is PROCESSING. The key sits under the
knowledgeBase.project prefix, so a delete or upload refreshes it.
Card titles are now h2: the new page a11y scan waits for the list,
and h1 -> h3 failed axe heading-order as soon as cards were on
screen (a latent issue the old scan never reached).
Tests: service (batched ids, empty page, missing aiAvailable,
error), hook (no request for empty page or project, one request
per page, unavailable and failed give no chips, no retry, polling
only while PROCESSING), card per state, and axe over every state
and the page with chips.
daniilperkin
changed the title
This was referenced Sep 24, 2026
The date filter now means "updated in this window": the artifact's last content change, or its import date if it never changed. Until now it filtered on the import date only. Why: the plan decided on activity semantics. The pinned API contract narrowed it to ingestedAt by mistake. For onboarding, "what changed recently that I should re-read" is the more useful question. A README imported months ago but edited yesterday should show up under "Last 7 days". - Labels follow the new meaning: the chip reads "Updated ...", the preset select is "Filter by last update (changed, or added if never changed)", and the custom fields are "Updated from" / "Updated to". - Doc comments on the component, dateRange helpers, URL state and hook now describe activity (`lastChangedAt`, else `ingestedAt`). - The request is unchanged (`from` / `to`, inclusive UTC days). The meaning moved to the backend (sprintstart-backend#258), where the window uses the same key as the CHANGED_DESC sort. Gates: format:check, lint, build clean; unit 307 files / 3061 tests; a11y 55 / 68.
Only uploads can be bulk-deleted, yet the Select toggle sat in the filter row in every view, pushing the filters onto a second line below the result range. It now appears only when the Uploads source is ticked (and the role may delete). Unticking Uploads also leaves select mode, so no checkboxes linger without a toggle to turn them off; ticking it again shows the toggle, not the old mode.
daniilperkin
marked this pull request as ready for review
September 24, 2026 11:07
…ogins Three small hardening changes to the GitHub metadata readers, plus a format-check exclude for local worktrees. They were first written on feature/easter-eggs-overhaul (#184) against the KB code already on dev, where they had no business living: two of the touched files overlap this branch and made #184 and #264 conflict whichever landed second. They now live here, next to the code they harden, and #184 drops them. - githubMetadata: getArtifactRepository now reads through the same per-artifact WeakMap cache as the org-login lookup. The card list, the drawer and the facet pass all call it on every render, and each call re-ran JSON.parse (and re-logged the warning for a malformed payload) for metadata that never changes for a given artifact object. The uncached core is one private computeRepositoryInfo both public readers share, so the two can no longer disagree. - orgMetadata: a blank login is rejected and a padded one is trimmed. The owner-half repository match compares against a trimmed `owner/repo` string, so " org " silently never matched its own repositories. This is the same normalisation the repo parser already applies to repositoryFullName. A new test pins both cases. - githubMetadata test: the malformed-blob case now silences and asserts the expected parser warning, so real warnings stay visible in the test output. - .prettierignore: skip .worktrees. eslint and vitest on this branch already exclude it, but format:check still walked every per-task worktree copy and flagged files formatted against other branches. Not ported: #184's facet-count Set hoisting in useKnowledgeBase. This branch moved facet counting server-side, so that loop no longer exists. The vite/eslint worktree excludes are already here in this branch's own form. Gates: build, lint (0 warnings), format:check clean; unit 362 files / 3131 tests.
daniilperkin
added a commit
that referenced
this pull request
Sep 24, 2026
This branch carried seven files that have nothing to do with easter eggs: hardening in the knowledge-base metadata readers, a Set hoist in useKnowledgeBase, and .worktrees excludes for vite, eslint and prettier. They were review fixes to code already on dev, parked here while this branch was the only open one. Now #264 is open and rewrites the same KB code: merge-tree showed useKnowledgeBase.ts (about 240 lines) and vite.config.ts conflicting whichever PR landed second. All seven now match origin/dev again: - githubMetadata/orgMetadata hardening + its test: moved to #264 (9cd5081) with a new trim test the original lacked. - useKnowledgeBase Set hoist: dropped. #264 moved facet counting server-side, so the loop it optimised no longer exists. - vite/eslint .worktrees excludes: #264 carries its own versions. This also drops the duplicate `vitest/config` import the vite change left. - .prettierignore: moved to #264 with the KB commit. No worktree nests inside this branch's own checkout, so its gates are unaffected.
daniilperkin
added a commit
that referenced
this pull request
Sep 24, 2026
The drawer test for "an Escape already handled inside is ignored" was appended to SidePanel.test.tsx. #264 also adds imports and tests at the end of that file, so whichever PR merged second hit a conflict on its import lines and on its final block. The test moves unchanged into SidePanel.escape.test.tsx, and SidePanel.test.tsx matches dev again. merge-tree against feature/KB-updates-final is now conflict-free.
10 tasks
…k see SVG targets Addresses the verified findings from the PR #264 review. 1. The "x-y of N" line was off by one page. `resultRange` computed `start = page.number * page.size + 1`, treating `number` as Spring's 0-based `Page.number`. The backend does not send that: ArtifactQueryService takes the 1-based `?page=` (`@Min(1)`, default 1), builds `PageRequest.of(page - 1, size)` and reports `number = page`. So page 1 of 100 read "21-40 of 100", and a 5-item project read "21-25 of 5". Now `(number - 1) * size + 1`. `ingestionService.mapRunPageMetadata` already treated the same DTO as 1-based (defaults `number` to 1), so this was the one consumer out of line. The `PageMetadata` TSDoc now states the 1-based contract, the KnowledgeBasePage page-size test fixture that sent `number: 0` now sends 1, and a new hook test pins page 1 -> 1-20 and page 2 -> 21-25. 2. useScrollLock blocked wheel/touch scrolling over icons. Both handlers started their walk to a scrollable ancestor with `event.target instanceof HTMLElement`. An SVG icon (every Lucide icon inside a drawer) is an SVGElement, not an HTMLElement, so the walk never started, `canScroll` stayed false and the event was prevented. Both now use `Element` (which has scrollTop/scrollHeight and parentElement), so the walk starts at the icon and finds the panel. New test: a wheel dispatched on an <svg> inside a scrollable panel is not prevented. keycloak-theme/ has no copy of the scroll lock, so nothing to port. Findings checked and deliberately not changed: - Bulk delete `null` sourceId collision: `Artifact.sourceId` is typed `string`, not `string | null`. Uploads without an id are filtered out before the request and reported as failures on their own; the backend only echoes ids it was sent, so no failure is ever looked up by an empty key. - act(...) warnings in the KB suites: not reproducible - 22 files / 315 tests run with zero "not wrapped in act" warnings. Gates: prettier format:check clean, npm run build OK, npm run lint 0 problems, npm run test 362 files / 3133 tests passed (chatQueue "frees the queue after a failed answer" failed once under full-suite load, passes alone and on the full rerun; untouched by this branch).
DavidLeuter
reviewed
Sep 25, 2026
DavidLeuter
left a comment
DavidLeuter
left a comment
Collaborator
There was a problem hiding this comment.
Went through the full source diff (URL state, debounced search, server-side list/facets, bulk delete, AI status chip, scroll lock, sidebar token). Overall this is really solid work: one shared buildFilterQuery for list + facets, the placeholder-aware page clamping, the scoped upload selection and the honest per-item delete report are all well thought through, and the test coverage is great. No blockers from my side, just a few points below (one about the global scroll lock that is worth a look before merging, the rest are nits).
Process note: the description still has Manual browser pass and E2E unchecked and says the E2E checklist runs "before this leaves draft", but the PR is no longer a draft. Could you tick those off (or update the text) once done?
Happy to approve once the scroll-lock point is either addressed or consciously accepted.
…s, prefetch key, stale delete report Responds to review 5317856963 on PR #264. Every point was checked against the code; all four were real. 1. useScrollLock blocked zoom and sideways scrolling (useScrollLock.ts). The window-level wheel/touch guard runs for every SidePanel (lockScroll defaults to true), so its edge cases reach well past the KB drawer: - Ctrl+wheel and trackpad pinch arrive as `wheel` with `ctrlKey: true` and were prevented whenever the pointer was not over a scrollable node, taking browser zoom away while a panel was open. The wheel handler now returns early on `ctrlKey`. The touch twin had the same problem - a two-finger pinch was walked and prevented like a scroll - so touchmove now only guards single-finger gestures. - A purely horizontal gesture was judged on the vertical axis only, so in a short drawer a wide `overflow-x-auto` code block could not be scrolled sideways. The guard now checks the dominant axis (|deltaX| > |deltaY| -> overflowX / scrollWidth / scrollLeft). - The wheel and touch walks were copy-pasted apart from the delta source; both now call one module-level `canScrollFrom(target, dx, dy)` so they cannot drift (the SVG-target fix from the previous commit had to be applied twice for exactly that reason). Touch now tracks the start X as well as Y. New tests: Ctrl+wheel not prevented; sideways wheel over a horizontally scrollable block passes while a vertical wheel over it is still prevented; a two-finger touchmove is not prevented. 2. Orphaned TSDoc (ArtifactFilters.tsx). The "what the numbers beside each option mean" block sat above NO_DATE_RANGE, which has its own doc, so FACET_COUNT_FOOTNOTE had none on hover. Moved it to the constant. 3. Prefetch key hard-coded `size: 20` (routePrefetch.ts). The hook keys the list on DEFAULT_PAGE_SIZE; the prefetch only hit because both are 20. It now uses DEFAULT_PAGE_SIZE for key and loader, so a changed default cannot silently turn the warm-up into a miss. 4. Stale bulk-delete report (ArtifactBulkActions.tsx). "N deleted" and the failure alert stayed up while the reader changed filters or pages, describing a list no longer on screen. The component now takes `listScopeKey` (the page's existing list identity) and clears the report when it changes, with the render-phase "adjust state on prop change" pattern so the stale report never paints over the new list. The delete's own refetch keeps the key, so the report still survives the refresh that follows a delete. Opening a new confirmation also clears it. New test covers both: same key keeps the report, a new key drops it. Trade-off: if deleting empties the last page, the page clamp changes the key and drops the report with it - acceptable, the list changed. Not changed here: the process note (tick "Manual browser pass" / "E2E" in the description) needs those passes actually run first. Gates: format:check clean, npm run build OK, npm run lint 0 problems, npm run test 362 files / 3137 tests passed.
Collaborator
Author
|
Thanks @DavidLeuter. All four inline points checked against the code, all real, fixed in
Gates on Process note: fair point. The manual browser pass and E2E checklist haven't been run yet, so the boxes stay unticked until they have. |
DavidLeuter
approved these changes
Sep 26, 2026
DavidLeuter
left a comment
DavidLeuter
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @daniilperkin, went through 4debd8b9, all four points are addressed cleanly:
- Scroll lock: early return on
ctrlKey, single-finger-only touch guard, dominant-axis check and the sharedcanScrollFromlook good, and the new tests cover exactly the cases I raised. - TSDoc: now sits on
FACET_COUNT_FOOTNOTE. - Prefetch: key and loader both use
DEFAULT_PAGE_SIZE. - Delete report: the render-phase reset on
listScopeKeyis the right call (no stale frame), and the trade-off when a delete empties the last page is fine by me.
CI is green, so approving. One thing before merging: please still do the manual browser pass / E2E checklist and tick the boxes in the description, as you said you would.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
SprintStartProject/Wiki#303 —
[Epic]: Knowledge Base Server-Side Pagination & Faceted SearchShort summary
The Knowledge Base page no longer downloads the whole corpus. It asks for one page of artifacts and one set of facet counts, so a 10k-artifact project costs two requests instead of ~100 sequential ones — and the large
contentTEXT column never leaves the database.API & query layer
knowledgeService.getArtifactPage(projectId, params)→GET /api/v1/projects/{projectId}/artifactswithpage,size,search,types,sources,repositories,format(repeatable where a facet allows more than one value).knowledgeService.getArtifactFacets(projectId, params)→GET /api/v1/projects/{projectId}/artifacts/facets.knowledgeService.getArtifactById(projectId, artifactId)→GET /api/v1/projects/{projectId}/artifacts/{artifactId}, used only when a deep-linked artifact is not on the loaded page.knowledgeBase.project(scope prefix),.list,.facets,.detail; the route prefetch warms page 1 and the facets for the default filter state.Hook (
useKnowledgeBase)totalElementsfeeds the result count; the current page is clamped when a filter shrinks the result set.?artifact=<id>) resolves from the loaded page and falls back touseArtifactByIdwhen the artifact is not there.Drawer & layout polish
SourceLinkBadgein the drawer header for direct external links;ArtifactViewerDrawerwidened tow-full max-w-5xl.useScrollLock,SidePanel) so the background cannot scroll or shift while a panel is open.--app-sidebar-width.Checks
npm run format:check,npm run lint,npm run build)useKnowledgeBase.test.ts,knowledgeService.test.ts,KnowledgeBasePage.test.tsx,KnowledgeBasePage.a11y.test.tsx,ArtifactViewerDrawer.test.tsx,SourceLinkBadge.test.tsx,useScrollLock.test.tsx)Review round (upgrade)
Everything below came out of reviewing this branch against the code and the epic's plan; it ships in
upgrade(knowledge-base): finish the server-side pagination upgrade.getUnifiedArtifacts— after the move to server-side paging it returned page 1 capped at 100 rows while still being named "unified", so the next caller would silently get a truncated corpus; its loader, the duplicatebyProjectquery key and the hook'sfilteredArtifacts/paginatedArtifactsaliases went with it;PageMetadata/ArtifactPageto the six fields the backend actually sends — themetadataenvelope,pageNumber,pageSize,isFirstandisLastcame from one stale fixture and were never real;location.state.preventScrollResetguard inuseScrollRestoration: react-router carries that option on the navigation, never inlocation.state, and the pathname rule already keeps a filter-only navigation in place;--app-sidebar-widthtoken instead of 286px written out in three places; the drawer width moved onto the shared scale;Knowledge Base UX & filtering upgrade (v1, phases 1-8)
Builds on the pagination work above: one commit per phase, each green on format:check, lint, build, unit and a11y before it was committed.
da20b83cd326696cf7b561b0f5066d167a2c6f3e,0f44bb764c5898b89e38726a2734761e2528577cBackend counterpart: SprintStartProject/sprintstart-backend#258. AI counterpart: SprintStartProject/sprintstart-ai#204.
Phase 1: URL is the source of truth
useKnowledgeBaseUrlState:tab,q,sources,repos,format,languages,sort,from,to,page,size,artifact. Short names and comma lists (?sources=GITHUB,JIRA&repos=acme/api) because these links get pasted into chats; repeated params are accepted on read.formatonly counts while Uploads is selected,reposonly while GitHub is, the same invariant the toggles enforce.?artifact=, page clamping and the project-switch clean-up replace.Phase 2: debounced search
src/hooks/useDebouncedValue.ts(typed, first render returns the value unchanged so a deep-linked?q=searches immediately). The input keeps local state; only the debounced value reaches?q=and the query keys.Phase 3: navigating large result sets
MultiSelectFilter(backwards compatible, all opt-in):searchablesections grow a filter box past 8 options,visibleLimitfolds long sections behind "Show all (N)" (ticked options stay visible), optionalfootnoteexplains what the counts mean.ArtifactPageSizeSelect), and a debounced polite live region announcing the result count.Phase 4: sort
sort=ADDED_DESC(default, unchanged order) /CHANGED_DESC/TITLE_ASC. List and facets now share onebuildFilterQuery, so any filter added later reaches both requests by construction (facets ignore sort).Phase 5: "Updated" date filter
from/to(yyyy-MM-dd, inclusive, UTC days) on list and facets. Presets (7 / 30 / 90 days) resolve to absolute dates when picked, so a shared link means the same range tomorrow. "Today" is the UTC day, so west of UTC the newest artifacts are never cut off.COALESCE(lastChangedAt, ingestedAt)(decision 1 below).Phase 6: language facet
?languages=(comma list, project-scoped, case-insensitive) sent repeated to list AND facets. Options sorted alphabetically so ticking one never reshuffles the list;ArtifactFacets.languagesis optional so an older backend reads as "no languages".lang:insideArtifactFilters' shared selection so "Markdown" (language) can never collide withMARKDOWN(format) in toggle routing or test ids.Artifact.language;tabs.tsalready used it to classify Markdown uploads, now pinned by tests.Phase 7: bulk delete uploads
DELETE_ALLOWED_GROUPSgate as the drawer) and only while the Uploads source is picked, so other views keep the filter row free: a Select toggle (aria-pressed) reveals checkboxes on upload cards; the checkbox sits beside the card so the card stays one button.ui/Modalalertdialog confirm with the drawer's consequence wording; oneDELETE /api/v1/uploadsfor the batch.deleteUploadsreads{ deletedIds, failed }; an empty 204 (older backend) means "all deleted"; an id named in neither list counts as failed.deleteUploaddelegates to it and now throws on a reported failure (it used to be silent).Phase 8: AI index status chip
GET /projects/{id}/artifacts/ai-status?ids=...per visible page (useArtifactAiStatus): retry off, 5 min stale time, polls every 10 s only while an item is PROCESSING. Key sits underknowledgeBase.project, so deletes/uploads refresh it.ui/Badgewith icon + text + colour: Indexed / Indexing / Failed / Not indexed. The spoken status is appended to the card'saria-label(the card isrole="button").aiAvailableis false, the request fails, or an id is missing, so an AI outage never prints "Not indexed" on every card. The tooltip says the chip reports the index record, not a retrieval guarantee.h3toh2: the page a11y scan now waits for the loaded list, whereh1thenh3failed axe heading-order (latent before).Checks for this upgrade
2528577ckb-select-toggle,artifact-select-<id>,kb-bulk-*,artifact-ai-status[data-status])Decisions (resolved 2026-09-24)
from/tonow matchCOALESCE(lastChangedAt, ingestedAt), the same key as theCHANGED_DESCsort; UI label "Updated". Backend04e68596, frontend0f44bb76.RequestBuilder.timeout()method tosync(timeout), so@Suppress("TooManyFunctions")is gone. Backend3512a0ae.type(scope): subjecthistory; recorded as the SprintStart exception in the workspace rules. No history rewrite.Manual browser pass and the E2E checklist were both run by the author against the local stack (frontend :5173, backend :8080, AI :8000, project with 1456 artifacts).