feat(search): add direct page-jump control to document search pagination (#340) #346
Reference in New Issue
Block a user
Delete Branch "feat/issue-340-pagination-page-jump"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #340
What was implemented
Numbered page-jump buttons with ellipsis pattern (e.g.
1 … 4 **5** 6 … 12) added to the document search pagination bar.Decisions applied (from Decision Queue on #340)
…spans for gaps. Maximum 7 elements rendered at a time.sm:(640px) viahidden sm:flex. The existing prev / "Seite X von Y" / next layout is preserved on mobile.makeHref(entry - 1)uses the existing clamping contract; no backend changes.Files changed
frontend/src/lib/components/Pagination.svelte— addedpageWindow$derived.by()computation + numbered button row with brand-navy active state, ellipsis spans, andsm:responsive hidingfrontend/src/lib/components/Pagination.svelte.spec.ts— 12 new tests covering: buttons visible/hidden by totalPages,aria-currenton active page,bg-brand-navyclass, 44px touch target,makeHreflinkage, first/last always shown, left/right ellipsis presence, mobilehiddenclassfrontend/messages/de.json,en.json,es.json— newpagination_page_buttoni18n key for aria-labelTest results
21 tests pass (9 existing + 12 new), all green. Lint and Prettier clean.
🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
The architecture is clean. All page-jump logic lives exclusively inside
Pagination.svelte— the+page.svelteorchestrator and itsbuildPageHref()/buildSearchParams()functions are untouched. ThemakeHrefprop contract remains the single URL-building authority. No new API endpoints, no new services, no cross-layer dependencies.Observations
Component boundary is correct.
pageWindowis a$derived.by()inside the component, as recommended. The algorithm is view logic and stays in the view layer. The parent doesn't need to know about it.Data contract is unchanged.
totalPagescomes from the backend viaDocumentSearchResult.paged()and is passed to the component exactly as before. No frontend/backend contract changes were needed — confirms the existing model was sufficient.Responsive strategy is appropriate.
hidden sm:flexon the button container is the correct approach: preserve the mobile layout untouched, layer the desktop enhancement on top. This avoids a second rendering branch.$derived.by()for multi-statement computation is exactly the right Svelte 5 primitive here. Not a$state+$effectpair.Suggestions
Minor —
pageWindowleaks a subtle edge case fortotalPages === 2. When there are exactly 2 pages (first=1, last=2, current=1), the loopfor (let p = Math.max(windowStart, first + 1); p <= Math.min(windowEnd, last - 1); p++)runsfor (p = 2; p <= 1)— empty.last > firstis true solast=2is pushed. Result:[1, 2]— correct. Edge case holds. No change needed, but worth noting in a comment alongside thelast > firstguard.No ADR needed. This is additive UI behavior within a single component. The decision was made in the issue's Decision Queue and the answer is committed there. Nothing structural enough to warrant a formal ADR entry.
Layering is fully clean. Nothing to block on.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The core implementation is solid and follows the project conventions correctly.
$derived.by(),makeHref(entry - 1), i18n keys, no business logic in template, clean props contract — all correct. My concerns are about test quality and one naming smell.Blockers
None.
Suggestions
1. Test name typo: "page button buttons" —
Pagination.svelte.spec.ts, last test in thedescribe('page number buttons')block:Should be
'page buttons container has hidden class on mobile (sm: prefix)'. The duplicate "button" is a noise signal, but since tests are documentation, it matters.2.
{#each pageWindow as entry, i (i)}uses the array index as key. The persona rule is explicit: always use a stable key.ias a key is position-based — it defeats DOM reconciliation benefits if the window shifts. A better key would beentry ?? ('null-' + i)or simply rendering the null entries as separate structural elements outside the{#each}. SincepageWindowis a derived value recomputed on everypagechange, reconciliation does still happen — but a null entry at position 1 and a null entry at position 5 are semantically different spans and deserve distinct keys. Suggestion: use(entry === null ? 'ellipsis-' + i : entry)as the key expression.3.
activePageBasehas partial overlap withcontrolBase. The string:...shares 9 tokens with
controlBase. The only diff isborder-brand-navy bg-brand-navy text-whitereplacingborder-line bg-white text-inkand the removal ofgap-1.5. This is worth a comment, not necessarily a refactor — the KISS trade-off is fine here. But thegap-1.5token is incontrolBaseand not inactivePageBase; the active button has no icon so that's intentional. A// no gap: active page shows number onlycomment would make it self-documenting.4. Missing test for focus ring on inactive page link buttons. The existing prev/next tests don't assert focus ring either, so this isn't a regression — but the issue spec called out
focus-visible:ring-2 focus-visible:ring-brand-navyas a requirement. ThelinkBaseclass is used on inactive page buttons and does include these tokens, so the behavior is present. A test assertingtoHaveClass(/focus-visible:ring-brand-navy/)on a page link would close that gap cheaply.5.
aria-current="page"is present on both the mobile<span>label AND the active desktop<span>button. Two elements witharia-current="page"in the same<nav>is technically valid (ARIA allows multiplearia-currentpeers in a set), but screen readers may announce both. When the desktop buttons are visible (sm:breakpoint), the mobile span issm:hidden. The hidden span should also havearia-hidden="true"atsm:breakpoint — but that's a CSS media condition, which can't be expressed with a static HTML attribute. The current implementation is acceptable sincesm:hiddenremoves it from layout but not the accessibility tree. Low priority suggestion: addaria-hidden="true"to the mobile label unconditionally and keeparia-current="page"only on the desktop active button. The mobile label is redundant once the desktop buttons are visible; hiding it from AT avoids double-announcement on wide screens. This is a nice-to-have, not a blocker.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Infrastructure footprint for this PR: zero. No new containers, no new npm dependencies, no CI configuration changes, no Docker Compose changes, no environment variables. This is purely frontend view logic.
What I checked
package.jsonorpackage-lock.json. The page-jump control uses native HTML elements (<a>,<span>) and Svelte's built-in reactivity. Correct call — no library justified for a component this size.DocumentControllerandDocumentServiceare unchanged. The@Min(0) @Max(100_000)guards on thepageparameter remain the server-side safety net.Pagination.svelte.spec.tsautomatically via the existingnpm run testconfiguration./api/documents/search→ backend port 8080) without any proxy config changes.hidden sm:flexresponsive utilities are standard Tailwind tokens already in the CSS output.One note for future consideration
The
PAGE_SIZE = 50constant is defined in+page.server.ts. If it ever changes,totalPagesfrom the backend adjusts automatically and the page-jump control requires no modification. This coupling-free design is correct and requires no infrastructure annotation.Nothing to block. Ship it.
🔒 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
No new attack surface introduced. The page-jump control is a pure navigation component — it reads a derived value, passes it through
makeHref(entry - 1), and renders<a href={...}>. No user input reaches the DOM as HTML, no eval, no raw string concatenation into URLs.What I audited
URL construction path is clean. All page button hrefs are generated by
makeHref(entry - 1). ThemakeHrefprop is alwaysbuildPageHreffrom+page.svelte, which callsbuildSearchParams()— a function that only constructs a/documents?...relative URL. An open redirect via a crafted page number is structurally impossible because:entryis anumber | nullfrompageWindow— never a user-typed stringmakeHrefalways returns a relative path; it never accepts a hostnameNo injection vector. The page numbers rendered in the DOM (
{entry}) are TypeScript numbers, not strings from external input. Numbers cannot carry XSS payloads.The
@Max(100_000)backend guard is unchanged and remains the correct server-side overflow protection. The frontendpageWindowcomputation clamps implicitly viaMath.max(first, current - 1)andMath.min(last, current + 1)— entries can never exceedtotalPages.No
aria-labelinjection.m.pagination_page_button({ page: entry })uses Paraglide's i18n function which substitutes a number. No user-controlled string reaches an attribute value.One observation (smell, not vulnerability)
The
{#each pageWindow as entry, i (i)}key uses the array index. This is a reconciliation concern (Felix flagged it), not a security issue — but worth noting that position-keyed lists can create subtle DOM reuse bugs when lists shift. Not exploitable, just fragile.Recommendation confirmed
The issue's Decision Queue answer for AC3 ("client-side clamping only") is the correct security posture. The clamping happens implicitly through
pageWindow'sMath.max/minbounds — no backend change needed or appropriate.🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
The unit test suite is solid and well-structured. 12 new tests using
vitest-browser-sveltewith real DOM, role/testid selectors, and meaningful behavioral assertions. The existing test patterns are respected. My concerns are about a few missing cases and a test name issue.Blockers
None.
Suggestions
1. Missing edge case:
totalPages === 2. The test suite coverstotalPages: 1(hidden) andtotalPages: 12(full window), but not 2. With exactly 2 pages,pageWindowshould produce[1, 2]with no ellipsis. There's no test asserting this boundary. The algorithm handles it correctly (manually verified), but the test suite doesn't prove it. A test'renders both pages without ellipsis when totalPages is 2'would close this gap.2. Missing test for
aria-labelon inactive page buttons. The issue spec called out accessible labels on all controls. The test suite assertsaria-currenton the active button but doesn't assert that inactive buttons havearia-label. Since thelinkBase<a>usesaria-label={m.pagination_page_button({ page: entry })}, one test should confirm that attribute is present.3. Test name inconsistency:
'page button buttons'— already flagged by Felix. The test name reads as a typo, which reduces readability in CI output.4. The
spy = vi.fn(makeHref)test only asserts thehrefattribute, not that the spy was called. The test'inactive page buttons link to their target page via makeHref'assertstoHaveAttribute('href', '/documents?page=0')— which is correct and sufficient. However, the spy is constructed but never asserted viaexpect(spy).toHaveBeenCalledWith(0). This is fine (the href assertion is the real check), but the spy setup is misleading if it's never used. Either assertspy.toHaveBeenCalledWith(0)or drop the spy and use the plainmakeHref.5. No E2E test added. The issue spec from Sara (in the issue review comments) called for one E2E Playwright test: navigate to page 3 via the jump control, assert URL, assert results render. This isn't in scope for the current PR's files (no
frontend/e2e/changes visible in the diff), but it should be a follow-up issue or noted as an open item. The unit tests cover clamping and rendering; the E2E layer would cover the full SSR load cycle.Overall: 21 tests (9 existing + 12 new), all described as passing. The test structure is clean and follows the established patterns. The gaps above are enhancements, not blockers.
🎨 Leonie Voss — UI/UX Design Lead
Verdict: ✅ Approved
The implementation follows the design recommendations from the Decision Queue exactly. Brand compliance is correct, touch targets are present, the responsive strategy matches the agreed approach. A few polish observations below.
What looks right
Active page button styling matches the spec from issue #340 review:
bg-brand-navy+text-whiteis the correct primary action style. ✅Ellipsis spans use
aria-hidden="true"andpx-2 text-sm text-ink-2— no border, no background, non-interactive. Matches the spec exactly. ✅Inactive page buttons reuse
linkBase— the samehover:bg-surface focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2pattern as prev/next. Visual consistency preserved. ✅Mobile fallback —
hidden sm:flexon the page buttons container,sm:hiddenon the "Seite X von Y" mobile label. The mobile layout is completely untouched below 640px. ✅44px touch targets on both active and inactive buttons. ✅
Observations
1. The
"Seite X von Y"span retainsaria-current="page"even on desktop. Atsm:breakpoint and above, the span is visually hidden viasm:hidden, but it remains in the accessibility tree witharia-current="page". The active page<span>in the desktop buttons also hasaria-current="page". On wide screens, screen readers will encounter two elements announcing current page: once from the hidden mobile label and once from the active desktop button. This is a Medium concern, not Critical —sm:hiddendoes applydisplay:nonewhich does remove it from the AT tree in all major screen readers. But it's worth verifying this behavior holds with VoiceOver on iPad (the primary tablet target), where CSSdisplay:noneinteraction witharia-currentcan be browser-specific.Recommendation: Add
aria-hidden="true"to the mobilepagination-page-labelspan unconditionally. The desktop numbered buttons fully replace its semantic role on wider screens, so the mobile label's AT presence is redundant regardless of breakpoint.2. Focus ring on inactive page buttons.
linkBaseincludesfocus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2 focus-visible:outline-none— this is correct and matches the existing prev/next button focus style. No change needed. ✅3. The ellipsis character is
…(U+2026 HORIZONTAL ELLIPSIS), not three dots. Correct. Screen readers will announce it as "ellipsis" witharia-hidden="true", which means they skip it. ✅4. Maximum 7 elements in the rendered window — confirmed by the algorithm: first + optional left-ellipsis + up to 3 window items + optional right-ellipsis + last = max 7. Fits comfortably on tablet at 640px+. ✅
Overall impression: The implementation is visually faithful to the spec. The only actionable item is the
aria-hiddensuggestion on the mobile label, which is low-effort and improves AT experience on the senior target audience's primary device (iPad/tablet).📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
All four acceptance criteria from issue #340 are met. The Decision Queue resolutions are implemented correctly. No scope creep. No missing i18n keys.
AC Verification
totalPages > 1{#if totalPages > 1}wraps the entire<nav>; desktop button row insidehidden sm:flexcontainer<a href={makeHref(entry - 1)}>— navigation is a standard link, results load via SvelteKit SSRpageWindowclamps viaMath.max(first, current-1)andMath.min(last, current+1)— entries are always within[1, totalPages]. Client-side only, as decided.bg-brand-navy text-white; inactive buttons usebg-white text-ink;aria-current="page"attribute presentDecision Queue Resolutions
sm:, preserve prev/next layout — implemented viahidden sm:flex. ✅i18n Coverage
New key
pagination_page_buttonwith{page}parameter added to all three message catalogs:de.json,en.json,es.json. ✅Open Items (non-blocking)
1. E2E coverage gap (identified by Sara): The unit tests cover rendering behavior thoroughly, but there is no Playwright test for the full page-jump journey (click button → assert URL changes → assert results load). This was flagged in the original issue review. It should be tracked as a follow-up, not a blocker for this PR.
2.
aria-hiddenon mobile label (identified by Leonie): Thepagination-page-labelspan remains in the AT tree on wide screens despitesm:hidden. Low-effort fix; recommend addressing in this PR or as a one-line follow-up.3.
totalPages === 2unit test missing (identified by Sara): The boundary case between "no buttons" (1 page) and "full window" (12 pages) is untested. Non-blocking but worth a follow-up issue.Verdict
All Must-have requirements from the original issue are satisfied. The three open items above are enhancements or test gaps, not requirement violations. The PR is ready to merge.
🗳️ Decision Queue — PR #346
Consolidated open decisions and action items surfaced during review. All are non-blocking for merge; the overall verdict is ⚠️ Approved with concerns (Felix, Sara).
1. Add
aria-hidden="true"to the mobile page labelRaised by: Leonie, Felix
The
pagination-page-labelspan hasaria-current="page"and is hidden onsm:screens viasm:hidden(which appliesdisplay:noneand removes it from the AT tree). However, on mobile the span and on desktop the active page button both carryaria-current="page", which is redundant on the mobile view. To be safe against VoiceOver/iPad edge cases withdisplay:none+aria-current:Option A: Add
aria-hidden="true"to the mobile label unconditionally and rely solely on the desktop buttons foraria-currentsemantics (cleaner, recommended).Option B: Leave as-is —
display:noneremoves elements from AT tree in all major browsers/screen readers.Recommendation: Option A — low-effort, eliminates any ambiguity on the senior audience's primary device.
2. Fix
{#each pageWindow as entry, i (i)}key expressionRaised by: Felix, Nora
The array index
iis used as the{#each}key. Per project code style, stable keys are required. Position-based keys defeat DOM reconciliation when the window shifts.Recommended fix: Use
(entry === null ? 'ellipsis-' + i : entry)as the key expression, making each entry uniquely identifiable regardless of position.3. Fix test name typo: "page button buttons"
Raised by: Felix, Sara
it('page button buttons have hidden class on mobile (sm: prefix)', ...)— should be'page buttons container has hidden class on mobile (sm: prefix)'. Tests are documentation.4. Follow-up: E2E Playwright test for page-jump journey
Raised by: Sara, Elicit
No Playwright E2E test covers: navigate via page button → assert URL contains
page=N→ assert results render. Recommended as a follow-up issue (not a blocker for this PR). The unit tests cover the rendering layer; this gap is at the integration/E2E layer.5. Follow-up:
totalPages === 2unit testRaised by: Sara, Markus
The boundary between "no buttons" (1 page) and "full ellipsis window" (12 pages) at exactly 2 pages is untested. Algorithm is correct (produces
[1, 2]with no ellipsis), but unproven by the test suite.Items 1–3 are small enough to address before merge. Items 4–5 are follow-up issues.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
The Decision Queue items have been addressed correctly in three atomic commits. Each concern became exactly one commit — the commit log matches the work. Clean.
What was fixed
Decision Queue #3 (test name typo) — renamed
'page button buttons have hidden class on mobile (sm: prefix)'to'page buttons container has hidden class on mobile (sm: prefix)'. Also correctly renamed the top-levelaria-currenttest to reflect the new semantic. Both renames are accurate descriptions of behavior.Decision Queue #2 (stable
{#each}key) —(i)replaced with(entry === null ? 'ellipsis-' + i : entry). The key change was the right fix. It also exposed a real algorithm bug: the original bridge-page conditions (windowStart === first + 1/windowEnd === last - 1) caused the loop and the branch to push the same page number, creatingeach_key_duplicateerrors. The bug was fixed correctly by shifting the thresholds tofirst + 2/last - 2. This was the right fix, and it was discovered by making the code correct, not by patching around the symptom.Decision Queue #1 (aria-hidden on mobile label) —
aria-current="page"replaced witharia-hidden="true"on the mobile span. The comment explaining why (desktop numbered buttons carry the aria-current="page" role; this label is redundant on wide screens) follows the project convention of comments that explain intent, not what the code does.Minor observations
Spy in the
makeHreftest is still unused — the original review noted thatconst spy = vi.fn(makeHref)is created butexpect(spy).toHaveBeenCalledWith(0)is never asserted. This wasn't in the Decision Queue so it wasn't fixed, and thehrefattribute assertion is the real check. Cosmetic — can be addressed in a follow-up or left as-is.The
{#if i === 1}ellipsis discriminator — still relies on the array index to distinguish left vs. right ellipsis. This is correct becausenullat index 1 always means left ellipsis (only entry afterfirst), but it's fragile if the algorithm ever changes. A'ellipsis-left'/'ellipsis-right'string in the array instead ofnullwould be more self-documenting. Not a blocker — the current approach is correct.Conclusion
All three in-scope Decision Queue items resolved. Algorithm bug found and fixed as a direct consequence of applying the correct key. TDD was followed: tests fail before the fix, pass after. Commits are atomic and conventionally named. The branch is clean and ready to merge.
🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
The follow-up commits are coherent and stay within the component boundary established in the original PR. No layer violations introduced.
Algorithm correctness
The bug fix in
pageWindowis worth examining closely. The original conditions:These were incorrect because the loop
for (p = Math.max(windowStart, first + 1); ...)runs whenwindowStart === first + 1, and it starts atwindowStart— so theelse ifbranch was a duplicate push. The fix shifts the bridge condition towindowStart === first + 2/windowEnd === last - 2, which is correct: at that threshold, the loop has already reached its upper bound and the bridge page (first + 1/last - 1) sits between the loop's last value andlast. The fix is sound.The edge case Markus noted in the first review —
totalPages === 2producing[1, 2]— is now covered by a test. Good.Accessible semantics
Moving
aria-hidden="true"onto the mobile label and keepingaria-current="page"only on the active desktop button is the correct semantic model. The mobile label was always decorative from the perspective of the accessibility tree on wide screens. On narrow screens it's the only visible indicator — but since it's nowaria-hidden, mobile screen reader users (VoiceOver, TalkBack) will navigate using the desktop buttons which are technicallydisplay:noneviasm:hidden.Wait — this is a concern: on true mobile viewports, the desktop button container (
hidden sm:flex) hasdisplay:none. Witharia-hidden="true"on the mobile label, mobile screen reader users now have no element announcing the current page. The active page<span aria-current="page">is inside thehidden sm:flexcontainer which isdisplay:noneat mobile widths —display:noneremoves elements from the AT tree.This is a regression from the original PR. Previously, the mobile label had
aria-current="page"and was visible to AT on mobile. Now it hasaria-hidden="true"and no other element announces current page on narrow viewports.Recommendation: Either restore
aria-current="page"on the mobile label (and removearia-hidden), or use a visually-hidden element that's always in the AT tree:<span class="sr-only" aria-current="page">{m.pagination_page_of(...)}</span>. Thesr-onlyapproach (position absolute, width 1px, height 1px, overflow hidden) makes the element visible to AT regardless of breakpoint while staying invisible on screen.This is a blocker concern. The aria-hidden fix was the right intent but created a mobile AT regression.
Everything else
Architecture boundary: clean.
pageWindowstays inPagination.svelte. No new dependencies. The three commits are atomic and independently revertable. The PR is otherwise ready.🔒 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
No new security surface introduced in the follow-up commits.
Audit of follow-up changes
Algorithm fix — The duplicate-push bug that was hiding under the index-based key is a correctness issue, not a security issue. Page numbers are always bounded integers; no user-controlled input reaches the array. The fix is clean.
aria-hiddenchange — Not a security concern. The change is semantic markup; no input path, no injection vector.Key expression —
(entry === null ? 'ellipsis-' + i : entry)— the key is a template literal with integer values. No user-controlled string reaches this expression.Accessibility regression note (not security, but flagged for completeness)
Markus raised a valid concern about mobile AT: with
aria-hidden="true"on the mobile label anddisplay:noneon the desktop button container at mobile widths, mobile screen reader users have noaria-currentanchor. This isn't exploitable, but it is a functional regression for assistive technology users on mobile — which includes a significant portion of the target senior audience on tablets.From a security-conscious accessibility standpoint: users who cannot perceive the current page state via AT may accidentally re-navigate to the page they're already on, or be confused about state. Confused navigation is not a security issue, but it is a usability regression for the 60+ iPad audience.
No changes needed from security perspective
The three commits are clean. The security posture of the PR is unchanged from the initial review.
🧪 Sara Holt — QA Engineer
Verdict: ✅ Approved
The test suite improvements from the Decision Queue are clean. All items that were in scope are addressed.
What was addressed
DQ #3 (test name typo) ✅ —
'page button buttons'renamed to'page buttons container'. Also fixed the top-levelaria-currenttest, which was renamed to accurately reflect the new behavior. Both are now accurate sentence-descriptions.DQ #5 (totalPages === 2 test) ✅ —
'renders both pages without ellipsis when totalPages is 2'added. It asserts both page buttons exist and neither ellipsis span exists. Four assertions, four behavioral guarantees. Correct boundary test.DQ #1 (aria-hidden test) ✅ — Two tests now assert
aria-hidden="true"on the mobile label: one in-place (renaming the oldaria-currenttest) and one new one with a differentpage/totalPagescombination. Good coverage.Bonus: algorithm bug caught by test suite
The stable-key fix (DQ #2) caused a runtime
each_key_duplicateerror in existing tests — which is exactly the correct outcome. The tests caught a latent bug in thepageWindowalgorithm (duplicate page number pushes). The fix was correct and the tests confirm it. This is TDD working as intended.Remaining open items (non-blocking)
Unused spy in
makeHreftest —const spy = vi.fn(makeHref)is constructed but never asserted. This wasn't in scope for this commit; thehrefattribute assertion is sufficient. Follow-up candidate.E2E gap (DQ #4) — Still no Playwright test for the full page-jump journey. This was explicitly deferred as a follow-up issue. Confirmed still open.
Mobile AT regression (raised by Markus) — The
aria-hiddenchange creates a gap for mobile screen reader users: the desktop button container isdisplay:noneon mobile, and the mobile label is nowaria-hidden. Noaria-currentis reachable via AT on narrow viewports. A test assertingaria-current="page"accessibility on mobile would catch this gap. Suggest adding ansr-onlyelement witharia-currentas the fix, with a corresponding test.Test count: 23 passing (up from 21). All green. ✅
🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
The Decision Queue items are addressed. The algorithm fix and the key stabilization are invisible to users. The
aria-hiddenchange has a specific AT regression on mobile that must be resolved.What looks right
Test name fix — Not a visual concern. ✅
{#each}key fix — Stable key expressions improve DOM reconciliation when the window shifts (e.g., user navigates from page 1 to page 5). This is a correctness improvement for animation smoothness if transitions are ever added. ✅Algorithm fix (bridge conditions) — The corrected
first + 2/last - 2thresholds now correctly show[1, 2, 3, 4, 5, null, 12]instead of accidentally duplicating values. Visual rendering is now correct for small-window cases. ✅Blocker: mobile screen reader users have no aria-current
The original mobile label had:
After this PR it has:
And the active desktop button has:
...which lives inside:
On mobile viewports (below 640px),
display:noneapplies to the desktop buttons container.display:noneremoves elements from the AT tree in all major browsers. The mobile label hasaria-hidden="true", so it's also excluded. Mobile screen reader users on narrow viewports now have noaria-currentindicator for the current page.This is a regression. Our primary audience includes seniors on iPad/tablet — this is Critical for that user group.
Fix: Use a visually-hidden
sr-onlyspan alongside the visual mobile label:Tailwind
sr-only=position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0, 0, 0, 0); white-space: nowrap; border-width: 0;. It is neverdisplay:none, so it stays in the AT tree at all breakpoints.At sm+ breakpoints, the desktop active button's
aria-currenttakes over for visual users while thesr-onlyspan provides the AT anchor for keyboard/screen-reader navigation.No other concerns
Everything else in the follow-up commits is correct. The focus ring, 44px touch targets, brand colors, and ellipsis handling are all as reviewed previously.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in the follow-up commits. The three new commits are frontend-only: two spec changes and one component change. Zero operational impact.
What I checked
pageWindowis a derived computation over an array that has at most 7 elements.The follow-up commits are clean from an infrastructure perspective.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
All four original acceptance criteria remain met. The follow-up commits address Decision Queue items 1–3 and 5. One new concern has been surfaced that touches AC4 in the mobile context.
AC Re-verification
totalPages > 1Decision Queue Resolution Status
aria-hiddenon mobile label{#each}keytotalPages === 2unit testNew Open Item: mobile AT gap
The intent of DQ #1 was to prevent double-announcement of
aria-currenton wide screens. The implementation (aria-hidden="true"on the mobile label) achieves this, but removes the only AT-accessiblearia-currentindicator on narrow viewports, because:aria-hidden="true"→ excluded from AT treedisplay:noneon mobile (hidden sm:flex) → excluded from AT treeRequirement impact: AC4 states "current page is visually distinguished." This is met visually. The accompanying acceptance criterion in the spec also implies screen reader accessibility. Mobile screen reader users no longer receive a current-page announcement — this is a regression against the implicit accessibility requirement.
Recommended fix (from Leonie): Add a
sr-onlyspan witharia-current="page"that is always in the AT tree. This satisfies the no-double-announcement requirement on desktop while restoring AT coverage on mobile.This item should be resolved before merge. The fix is small (4 lines) and a corresponding test (
aria-currentis accessible on narrow viewport) would close the requirement gap cleanly.Summary
All prior blockers and in-scope suggestions are resolved. One new blocker has been surfaced during this review pass. It requires a small follow-up commit before the PR can be merged cleanly.
🗳️ Decision Queue — PR #346 (Round 2)
Updated status after the three follow-up commits. Overall verdict: ⚠️ Approved with concerns (Leonie, Elicit). One new blocker surfaced.
✅ Resolved items
DQ #2 — Stable
{#each}key — Fixed:(entry === null ? 'ellipsis-' + i : entry). This also exposed and fixed a hidden duplicate-push bug inpageWindow(bridge conditions shifted fromfirst+1/last-1tofirst+2/last-2). ✅DQ #3 — Test name typo —
'page button buttons'renamed to'page buttons container'. ✅DQ #5 —
totalPages === 2unit test —'renders both pages without ellipsis when totalPages is 2'added. ✅🔴 New Blocker: mobile AT gap from aria-hidden change (DQ #1 partial)
Raised by: Markus, Leonie, Sara, Elicit
Problem: The
aria-hidden="true"fix on the mobile label removes the onlyaria-currentindicator visible to screen readers on narrow viewports. The desktop button container (hidden sm:flex) isdisplay:noneon mobile, which removes it from the AT tree. Combined witharia-hidden="true"on the mobile label, mobile screen reader users have no current-page announcement.Recommended fix (Leonie):
The
sr-onlyspan is neverdisplay:none, so it stays in the AT tree at all breakpoints. On sm+ screens, the desktop active button also hasaria-current="page"— AT may announce both, but this is acceptable (they carry the same information). Alternatively, addaria-hidden="true"to thesr-onlyspan on desktop using a CSS-only approach, but that is not possible with a static attribute.Test to add:
📌 Still deferred
DQ #4 — E2E Playwright test — Deferred as a follow-up issue. No change to this position.
Recommendation
Address the
sr-onlyfix in one additional commit. The rest of the PR is clean and ready to merge once this regression is resolved.Felix (Developer) — Round 2 Review
Reviewing the full set of 5 commits end-to-end.
pageWindowalgorithm (commitd97b62c0) — the bridge logic is now correct. Threshold change fromfirst+1/last-1tofirst+2/last-2prevents the loop from double-pushing the bridge page. I traced it manually:totalPages=2, page=0: first=1, last=2. windowStart=1, windowEnd=2. Neither bridge fires (windowStart=first, sowindowStart > first+2false;windowStart === first+2false). Loop:pfrom max(1,2)=2 to min(2,1)=1 → empty.last > first→ push 2. Result:[1, 2]. ✅totalPages=12, page=5 (0-idx): first=1, last=12. current=6. windowStart=5, windowEnd=7.windowStart > first+2(5>3) → push null. Loop: p from max(5,2)=5 to min(7,11)=7 → push 5,6,7.windowEnd < last-2(7<10) → push null. Push 12. Result:[1, null, 5, 6, 7, null, 12]. ✅totalPages=5, page=1 (0-idx): current=2, windowStart=1, windowEnd=3.windowStart === first+2? 1===3 → false.windowStart > first+2? 1>3 → false. Loop: max(1,2)=2 to min(3,4)=3 → push 2,3.windowEnd < last-2? 3<3 → false.windowEnd === last-2? 3===3 → push 4. Push 5. Result:[1,2,3,4,5]. ✅ No ellipsis when small range.{#each}key expression —(entry === null ? 'ellipsis-' + i : entry)is clean. The null check distinguishing left vs right ellipsis by positional index is pragmatic and correct.sr-onlyspan (commitdc7e5a1f) — addresses the mobile AT regression cleanly.sr-onlynever usesdisplay:none, so it stays in the AT tree at all breakpoints while remaining visually hidden. Thearia-current="page"on this span is the reliable cross-breakpoint anchor for screen readers.One minor nit (non-blocking): the comment on the mobile label says "desktop numbered buttons carry the aria-current='page' role; this label is redundant on wide screens" — after the sr-only fix, the reason
aria-hiddenis better stated as "the sr-only span below is the canonical AT source; this visual label is decorative". The current comment in the final file is already updated to say this correctly: "aria-hidden: decorative visual label; AT uses the sr-only span below for aria-current". ✅Tests — 24 tests, each with a single logical assertion, sentence-style names. The
totalPages===2boundary test (d97b62c0) directly validates the bridge-off condition. The sr-only test verifies both presence andaria-current. All concerns from round 1 are addressed.Verdict: APPROVE — implementation is correct, tests are thorough, algorithm is sound.
Sara (QA) — Round 2 Review
Reviewing test suite completeness after all 5 commits.
Test inventory (24 tests):
Core rendering:
renders the page-of-total label for the current page✅mobile page label is aria-hidden (desktop buttons carry the aria-current role)✅mobile page label is aria-hidden so screen readers skip it on wide screens✅ (duplicate assertion but documents the intent explicitly — acceptable)sr-only span always provides aria-current="page" for screen readers at all breakpoints✅ — this is the key regression test for the mobile AT fixPage number buttons (11 tests):
renders page number buttons when totalPages > 1✅does not render page number buttons when totalPages <= 1✅marks the active page button with aria-current="page"✅active page button has brand-navy background✅active page button has 44px touch target✅inactive page buttons link to their target page via makeHref✅renders first and last page buttons always visible✅renders ellipsis span between first page and window when gap exists✅renders ellipsis span between window and last page when gap exists✅does not render left ellipsis when window is adjacent to first page✅does not render right ellipsis when window is adjacent to last page✅page buttons container has hidden class on mobile (sm: prefix)✅renders both pages without ellipsis when totalPages is 2✅ — the boundary test that caught the duplicate bugNavigation:
renders prev as a link pointing at page - 1 when not on first page✅renders disabled prev as an aria-hidden non-link so screen readers skip it✅renders next as a link pointing at page + 1 when not on last page✅renders disabled next as an aria-hidden non-link on the last page✅calls makeHref with p-1 and p+1✅renders decorative chevron inside aria-hidden span so screen readers skip it✅prev and next have min 44px touch targets✅Coverage assessment:
The
totalPages===2boundary test is particularly valuable — it directly exercises the condition that triggered theeach_key_duplicateSvelte error when the key was fixed. If the bridge thresholds regress, this test fails immediately.Missing coverage (non-blocking, DQ item from round 1):
Test style compliance:
expectcalls but they test the same attribute pair, e.g.,min-h+min-w— logically one "touch target" assertion)vitest-browser-sveltereal DOM ✅Thread.sleep/setTimeoutanti-patterns ✅Verdict: APPROVE — test suite is production-quality. The sr-only regression test closes the mobile AT coverage gap from round 1.
Leonie (UX/Accessibility) — Round 2 Review
Reviewing the accessibility changes across commits
451904daanddc7e5a1f.Mobile AT fix analysis:
Round 1 flagged: the original code had
aria-current="page"on the mobile label, which was the onlyaria-currentindicator at narrow widths. The fix in451904dacorrectly removed that (it shouldn't be on a visually-hidden-via-sm:hiddenelement), but introduced a regression: on mobile, the desktop button container isdisplay:none(viahidden sm:flex), which removes it from the AT entirely. Result: noaria-currentfor mobile screen reader users.The
sr-onlyfix indc7e5a1fresolves this correctly:sr-only=position:absolute; width:1px; height:1px; overflow:hidden; clip:rect(0,0,0,0)— visually hidden but never usesdisplay:none, so it stays in the AT tree at all breakpoints. Screen readers on mobile will announce this span. Screen readers on desktop will also announce this span AND the active page button (aria-current="page") — two sources, same information, which is the correct pattern for cross-breakpoint accessible pagination.ARIA pattern compliance:
<nav aria-label="Seitennavigation">✅ — landmark provides contextaria-current="page"on the active page button (desktop) ✅aria-current="page"on sr-only span (all breakpoints) ✅«»wrapped inaria-hidden="true"spans ✅aria-hidden="true"✅ — cosmetic, screen readers don't need to hear "…"aria-hidden="true"spans (not links) ✅ — avoids "Previous, link, dimmed" announcementsaria-labelon each page button withpagination_page_buttoni18n key ✅ — "Seite 5" is readable, not just the numberTouch target:
min-h-[44px] min-w-[44px]on all interactive elements ✅ — WCAG 2.5.5 AAA target size.One observation (non-blocking): The
sm:hiddenclass on the mobile label means it's visually hidden on wide screens viadisplay:none. Since it's alsoaria-hidden="true", this is fine — both visual and AT access are removed on wide screens. The sr-only span is the canonical AT anchor. The design is clean.Verdict: APPROVE — the mobile AT regression is fully resolved. The sr-only pattern is the correct WCAG-compliant solution.
Markus (Architect) — Round 2 Review
Reviewing structural and component design concerns.
Component contract —
Pagination.svelteprops are unchanged:page(0-indexed),totalPages,makeHref,ariaLabel?. The PR adds no new props. Internal state (pageWindow,hasPrev,hasNext) is$derived— reactive, no side effects, no stores. Clean Svelte 5 idiom. ✅$derived.by()usage — multi-statement computation correctly uses$derived.by()per Svelte 5 convention. A plain$derivedexpression would not work for the imperative loop buildingresult. ✅Separation of concerns — the
pageWindowfunction is a pure algorithm: no DOM access, no side effects, input ispage + totalPages, output is(number | null)[]. This is directly unit-testable without rendering. The tests call it indirectly via render but could be extracted to a pure function if needed. Given the component is small this is fine — KISS wins. ✅i18n keys —
pagination_page_buttonadded to all three locales (de,en,es) in the same commit. No orphan keys. ✅No new dependencies — the feature uses only existing Tailwind utilities and Svelte primitives. No new packages. ✅
Algorithm correctness — the
first + 2/last - 2bridge thresholds ensure:totalPages=2(no ellipsis),totalPages=1(hidden), first/last pages always presentThe pure-function nature of
pageWindowmeans the algorithm can be validated by inspection against a table of (page, totalPages) → expected array. Felix traced several cases in his review — I'm satisfied.Potential concern (non-blocking): The
{#each}keyentry === null ? 'ellipsis-' + i : entryrelies on the invariant thatnullentries are never adjacent (only one left ellipsis, one right ellipsis). The algorithm guarantees this by design —windowStart > first + 2fires at most once, andwindowEnd < last - 2fires at most once. If that invariant ever breaks,'ellipsis-0'and'ellipsis-2'would still be unique keys. ✅Verdict: APPROVE — architecture is clean, no structural concerns remain.
Nora (Security) — Round 2 Review
This PR is a pure frontend pagination component. Security surface is minimal but I'll confirm it explicitly.
XSS / injection:
makeHref(entry - 1)produces thehreffor page links.entryis always a number from thepageWindowcomputation — no user input reaches the href.pageandtotalPagesare props passed by the route's load function, which constructs them from validated URL search params. No dynamic HTML injection. ✅No new API calls — the component is fully client-side. No fetch, no backend requests, no auth tokens touched. ✅
No new permissions or auth surface — the
Paginationcomponent has no awareness of the current user. ✅i18n messages — Paraglide message functions are type-safe; the
{ page, total }interpolation values are numbers, not raw user strings. ✅Verdict: APPROVE — no security concerns.
Tobias (DevOps) — Round 2 Review
Infra and CI impact assessment.
Bundle size — no new npm packages.
Pagination.svelteadds ~50 lines of Svelte template and ~40 lines of TypeScript logic. Compiled output is trivial, well under any meaningful bundle size threshold. ✅Test count — 24 unit tests (up from 9 original). All use
vitest-browser-sveltewhich runs in a real browser process during CI. These are fast unit tests, not E2E — no Docker Compose required. CI time impact is negligible. ✅No environment dependencies — the component has no API calls, no env vars, no config. Works identically in dev, CI, and prod. ✅
Lint/Prettier — commit messages show formatter was run (
npx prettier --write) before the final commit to clear whitespace violations from the hook. The tree is clean. ✅i18n files — 3 JSON message files updated atomically in the original feature commit. No key drift between locales — all three files have
pagination_page_buttonwith locale-appropriate values. ✅Verdict: APPROVE — no CI or infra concerns.
Elicit (Requirements Engineer) — Round 2 Review
Verifying implementation against #340 acceptance criteria.
From issue #340:
hidden sm:flexon the button container. Mobile shows prev + label + next. ✅activePageBaseclass =bg-brand-navy. Active page is a<span>not an<a>. ✅pageWindowalgorithm.nullentries render as…. ✅aria-current="page"on both the active button AND the sr-only span.aria-label={m.pagination_page_button({ page: entry })}on each button.min-h-[44px] min-w-[44px]on all controls. ✅All
…spans havearia-hidden="true". ✅pagination_page_buttonin all three message files. ✅Additional behaviors verified (not in #340 but relevant):
totalPages === 1→ entire<nav>hidden. ✅totalPages === 2→ both pages shown, no ellipsis. ✅aria-current="page"via sr-only span. ✅ (this was a regression introduced and fixed during the PR lifecycle)All acceptance criteria satisfied.
Verdict: APPROVE — closes #340 completely.
Decision Queue — Round 2
All round 1 concerns resolved. All personas approve.
Resolved This Round
aria-current="page"— semantically wrong on asm:hiddenelement451904da: changed toaria-hidden="true"+sm:hidden{#each}used positional indexias key — unstable, hides DOM reconciliation bugsd97b62c0: key is nowentry === null ? 'ellipsis-' + i : entryd97b62c0: bridge thresholds corrected tofirst+2/last-2aria-hiddenon label +display:noneon button container = noaria-currentfor mobile screen readersdc7e5a1f: sr-only span witharia-current="page"added — always in AT tree regardless of breakpointOpen Items
None blocking merge.
Recommendation
MERGE. PR #346 is ready. All 6 personas approve. The sr-only pattern correctly preserves accessibility across all breakpoints. The algorithm is verified correct for all boundary cases. 24 tests provide thorough regression coverage.
Post-merge action: create a follow-up issue for DQ-4 (Playwright E2E keyboard nav test).