Repository navigation
fix: sprint 10 release-audit findings (stacked on #319) - #320
Merged
Merged
Conversation
daniilperkin
requested review from
BabuPlk,
DavidLeuter and
kiranfin
and
a balanced review from Copilot
October 7, 2026 10:45
Collaborator
Author
|
gg, maybe @kiranfin, @DavidLeuter or @BabuPlk ? |
daniilperkin
force-pushed
the
fix/sprint10-release-fixes
branch
from
October 7, 2026 10:55
bf66ec1 to
973f498
Compare
The search box only adopted `?q=` on Back/Forward, on the theory that every other URL change was this hook's own write echoing back. That missed in-app navigations: pressing the sidebar's Knowledge Base entry (or any link that carries its own `?q=`) while the page is mounted pushes a new URL, the list refetches against it, and the box keeps the old text — the reader sees filtered results next to a search box that says something else. Adoption now compares the URL against the settled copy of the input (the debounced value), not against the navigation type: a URL equal to it is this hook's own write echoing back and is skipped — adopting that one is what used to eat the characters typed since the write — and anything else is somebody else's change, so the box follows it. Tracking the last write (a ref read during render, or a state mirror written from an effect) is what the compiler lint rightly forbids; the settled value answers the same question without either. Tests: two new cases in the hook suite — a pushed `?q=` reaches the box, and a push that clears `?q=` empties it — alongside the existing Back/Forward and debounce coverage (55 pass).
Deleting every upload on the last page shrank the list, the out-of-range clamp moved the page, and the report — "N deleted", plus every failure with its reason — was wiped before the reader could read it: the report's scope key was the whole list identity, page included, so the delete's own after-effect dropped the delete's own outcome. The hook now also computes a filter-set key (project, search, facets — no page, size or order), and the report keys on that: a filter change still drops a report that describes a list the reader has left, while the page moving under it — the clamp, or the reader paging themselves — leaves the outcome in place. Selection keeps the full list key, because a ticked row must not survive onto a page it is no longer on. Tests: the component test now covers both halves — the report survives a page-only key change and still drops on a filter-set change; the prop is renamed `filterScopeKey` everywhere.
The drawer's module carries Prism with every language grammar and KaTeX — by far the biggest chunk the build emits (about 916 kB raw / 313 kB gzip) — and the widget is mounted at the app root, so the static import put the whole thing in the entry's import graph: every visitor fetched it before the first page settled, the login screen included. The widget now lazy-imports the drawer and renders it under a null Suspense fallback: nothing is lost visually (the drawer only exists once a citation is opened), and the chunk loads on the click that needs it. The Knowledge Base page and the buddy page keep their own route-lazy references to the same chunk, so nothing is duplicated.
daniilperkin
force-pushed
the
fix/sprint10-release-fixes
branch
from
October 7, 2026 10:55
973f498 to
1ad24c2
Compare
The interval field was a free number, and the backend turns it into `0 */n * * * *`: a stored 45 fires at minute 0 and 45 and then again at the next hour's 0, and a stored 120 collapses to once an hour — both silently. The field is now a select of the values `*/n` can actually express (n dividing 60, capped at 60 = hourly), with a stored out-of-list value still shown as an option so the control never lies about what is saved (the same treatment the page-size control gives a hand-edited size). The other half of the finding — a naive local pick sent as server-local time, so 02:00 Berlin runs at 02:00 UTC — needs a backend that carries a timezone; it is a follow-up, not something a frontend change can honor on its own. Tests: the field's own suite selects 30 and pins that a stored 120 still appears; the page- and panel-level schedule suites drive the select too (their 45-minute edits become 15 — 45 is exactly the kind of value the control no longer offers).
Nothing wrapped the routes: a lazy chunk that failed to load (a stale deploy's hashed file, a dropped network) or any render throw inside a page travelled to the React root and unmounted the entire app — sidebar included — as a blank screen. `RouteErrorBoundary` now wraps the route tree inside the page transition: the failure stays in the page area with an alert and a "Reload page" action (a reload is the one retry that works — React.lazy caches a rejected import for the page's lifetime, so an in-place retry cannot). The boundary is keyed by pathname, so navigating away remounts it and a broken page never traps the reader. Tests: renders children while healthy; shows the failure card and reloads on request.
Three raw-fetch call sites built the Authorization header by interpolation, so an unauthenticated moment produced the literal string `Bearer undefined` — a malformed credential sent to the backend instead of no credential at all. All three now guard on `keycloak.token`, matching the pattern `apiClient` and `knowledgeService` already use (apiClient also owns the redirect side; these streams keep their existing update-token flow and simply omit the header when there is nothing to send). Sites: onboardingService's personalize stream, aiStreamService's progress stream, and buddyService's reply stream — the last one was missing from the audit's list.
The three-tab axe walk hard-coded a 10 s timeout while the suite's configured timeout is 30 s (vite.config.ts `testTimeout`). Under the a11y suite's parallel load, three full-DOM scans can exceed ten seconds on a busy machine, failing the gate for load rather than for violations. The override is gone; the test inherits the suite default like every other a11y test.
daniilperkin
force-pushed
the
fix/sprint10-release-fixes
branch
from
October 7, 2026 11:13
1ad24c2 to
f059a0e
Compare
LinseCed
approved these changes
Oct 7, 2026
…nting, guard the lazy drawer
A deep review pass over this stack found two real issues and three small
ones; this commit addresses all five.
The boundary must reset, not remount:
- `RouteErrorBoundary` was keyed by pathname, so every pathname change
unmounted and remounted the whole route tree below it — silently
reversing three documented behaviors. Switching buddy conversations
remounted the page (`PageTransition` keeps one key for all `/buddy`
addresses precisely so it does not), PM section switches remounted the
workspace (a fresh `SlidingTabPanel` has no previous panel, so the
slide became an instant swap, and the tab pill's layout morph died),
and the project-analysis launcher was thrown away on the way out of
the overview, which its own comment forbids ("it owns the run").
- The boundary now takes `resetKey` (AppRouter passes the pathname) and
clears the failure in `componentDidUpdate`: the next route gets a
clean attempt as a state reset, without the remount. The tree keeps
exactly the mount lifecycle `PageTransition`'s coarse keys give it.
The lazy drawer gets what the routes got:
- The citation drawer's chunk is fetched on click from a widget mounted
OUTSIDE `AppRouter`'s boundary, so a rejected chunk (a stale deploy)
would have travelled to the React root and blanked the whole app. It
is now wrapped in the same boundary — generalized with an optional
`fallback` — showing a compact "The artifact couldn't open." card with
a reload.
- A loading state answers the click while the 313 kB-gzip chunk arrives;
the fallback-less Suspense read as a dead click on a cold cache.
Small ones:
- The interval docstring no longer reads as an exhaustive list — 4, 6
and 12 divide 60 too; the list is curated to values the schedule
cannot quietly reinterpret.
- The no-token guards are pinned: buddyService and onboardingService
each assert that a missing token sends NO Authorization header, and a
new aiStreamService suite covers both branches.
Tests: three boundary cases (reset on key change, no remount across it,
host fallback), the three token-guard cases. Narrowed gates: prettier,
build, eslint, touched suites — 10 files / 188 tests green.
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
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.
What
A deep audit of the sprint 10 release (frontend side, checked against #317's dev→main diff) produced twelve findings; this stack carries the frontend fixes that need no backend change — one commit per finding, each tested with the suites that cover the touched files.
Fixes
?q=on Back/Forward, so an in-app navigation that changed or cleared the query (pressing the sidebar's Knowledge Base entry, any link carrying aq) left the box saying something other than the list showed. Adoption now compares the URL against the settled text instead of the navigation type.BuddyWidget, so every visitor fetched it before the first page settled. Now lazy, loaded on the click that opens it.45(fires 0/45 then 0) and120(collapses to hourly) be saved silently; the field now offers the values a cron step can express, with a stored out-of-list value still shown. The timezone half of that finding needs a backend change — left as a follow-up.RouteErrorBoundaryreset by pathname now keeps the shell and offers a reload (a state reset, not a remount — the grouped transitions keep their mounted trees).Bearer undefined— three raw-fetch sites sent the literal string when no token was held; all guard now (the audit missed the buddy reply stream).Verification
npm run buildcleanNotes (checked, deliberately not changed)
main), the unmergedfeature/243-PM-attention-countendpoint, and cron timezone handling are backend/release items, reported separately.TODO(backend)comments remain (the release checklist's DoD item) — noted for the sprint wrap-up.Stacked on #319, which has since merged — this PR now targets
devdirectly and carries the seven audit fixes from the release sweep.Review round
A deep review pass over this stack found two real issues and three small ones; all five are addressed in dd58091:
resetKey(state only, no remount).AppRouter), so a rejected chunk would have blanked the app; it now has the same boundary with a compact fallback, plus a loading state while the chunk arrives.