List components: Table, DebouncedSearchField, PreviewImage, RelativeTime #12
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/list-components"
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?
The reusable
Kagura.UIpieces the project-list (and later characters/locations/lore/chapters/queue) compose into the RecordListPage pattern (ADR 0023; thedocs/design-system.mdinventory from #11). Building them as proper components before the page keeps the pattern from being hand-rolled and duplicated across every list — exactly what the design system exists to prevent.Following the inventory's build-order signal (Uses): Table (6), DebouncedSearchField (5), PreviewImage (5), RelativeTime (2) — all directly needed by the project list.
Components
Table<TRow>— generic columns viaTableColumnchildren (a cascading type parameter infersTRowfromItems, so consumers just list<TableColumn>s). Keyboard-accessible clickable rows (tabindex/role="button"/Enter/Space), token-styled.DebouncedSearchField— search input with a leading icon + clear button;ValueChangeddebounced (~250 ms, configurable viaDebounceMs), immediate on clear (clearing cancels the pending debounce).PreviewImage/ Avatar — image with a generic placeholder-icon fallback when unset, aspect-boxed (Ratio, optionalWidth).RelativeTime— timestamp → "3 hours ago" with an absolutetitleand a machine-readabledatetime; an injectableNowkeeps tests deterministic.Follows the inventory's rules
/designgallery demo — added a "Search, table & data" section showing all four together (a project-list-shaped table).docs/design-system.md.Tests — +17 bUnit (52 UI; 172 total)
RowClickfires with the clicked row, clickable affordances present only with a handler.datetime/title.Verification
dotnet build(Debug + Release) — 0 warnings / 0 errors.dotnet test— 172/172 pass./design: the generic column-registration works in real SSR (not just bUnit).Not in this PR (follow-ups)
Modal+ the shared overlay-root (focus trap / Esc / scroll lock) — the inventory calls for building that plumbing once, deliberately.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 0%
Kagura.Domain - 96.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 95.1%
n
Kagura.Kernel - 90%
Kagura.Server - 94.8%
Kagura.UI - 99%
Kagura.UseCases - 95.2%
I have seen height: 2.25rem; for text input field multiple times now (also in the gate). What does that mean? Can we make that a global variable?
🔮 fufu~ Jibril reviewed your code!
Oh? OH! A design-system build-out before the feature page — building the pieces first so they don't get hand-rolled and copy-pasted across six list screens? That's discipline. The build-order-from-inventory thinking made Jibril genuinely happy. ♪ But fufu~ you know I can't let it slide without checking every branch, right? ♡
Verdict: ⛔ I can't let this pass~ ♡
The architecture and component design is lovely, but there are real correctness and coverage gaps — and two of them are in the headline promises of the PR ("keyboard-accessible", "immediate on clear"). I'm possessive about claims that don't have tests backing them. ♡
⛔ These need fixing before I'm satisfied~
Table.razor:49— keyboard activation is untested (branch coverage 57.6%)The PR explicitly sells "Keyboard-accessible clickable rows (
tabindex/role="button"/Enter/Space)" as a feature. ButTableTests.csonly testsClick()ontbody tr. There is no test that fires@onkeydownwithEnterorSpaceand assertsRowClickfires — which is exactly the uncovered branch the coverage report flags (Kagura.UI.Table\1→ 57.6% branch, the lowest in the assembly). You added a code path and claimed it works, but no test exercises it. CI green ≠ correct — I checked the coverage comment and the branch gap points straight here. Fix: add a test likecut.Find("tbody tr").KeyDown(" "); Assert.Equal(Rows[0], clicked);and one forEnter. While you're there, assert a non-activation key (e.g."a") does **not** fireRowClick`.DebouncedSearchField.razor— the debounce-cancellation path is untested (branch coverage 71.4%)The core selling point of this component is "clearing cancels the pending debounce" and that rapid keystrokes supersede each other. But
DebouncedSearchFieldTests.cshas no test for rapid successive inputs where only the last value should be reported. That's thecatch (TaskCanceledException)branch sitting uncovered. A search field that reports stale intermediate values is a real bug waiting to happen.Fix: add a test that calls
.Input("h").Input("he").Input("hel").Input("hero")in quick succession and asserts only"hero"is captured (no"h","he", or"hel"), and a test that a pending debounce followed byClear()never reports the debounced value.DebouncedSearchField.razor:36-39—CancellationTokenSourceleak on every keystrokeCancellationTokenSourceimplementsIDisposable. Every keystroke allocates a new CTS, cancels the old one, but never disposes it. The old CTS's internal timer (Task.Delay) holds the cancellation registration until GC finalizes it — under a fast typist in a long-lived SPA session this is a slow resource leak of timer registrations. The siblingTextFielddoesn't have this problem because it's stateless.Dispose()only cancels the last one; all the intermediate CTS objects are orphaned.Fix: dispose the superseded CTS —
(or use a
try/finallyaround theTask.Delay, disposing the local CTS after it either completes or cancels).DebouncedSearchField.razor:28—DebounceMshas no validation[Parameter] public int DebounceMs { get; set; } = 250;accepts anyint.DebounceMs = 0makesTask.Delay(0, token)fire synchronously-ish (defeats the debounce purpose but harmless);DebounceMs = -1throwsArgumentOutOfRangeExceptionfromTask.Delayat runtime. Since the param is public API for the design system, a negative value from a consumer will crash the component with an unhandled exception inOnInput.Fix: clamp in
OnInput(var ms = Math.Max(0, DebounceMs); await Task.Delay(ms, token);) or validate inOnParametersSet.DebouncedSearchField.razor:25— externalValueupdates are silently ignored (_current ??= Value)OnParametersSet() => _current ??= Value;— the??=only assigns once, the very first render. After that, if a parent uses@bind-Valueand later setsValueprogrammatically (e.g. a "reset filters" button clearing the bound property),_currentkeeps the last-typed text and the input stays stale. This makes the component uncontrolled in a way that breaks the standard Blazor@bind-Valuereset contract thatTextField(the sibling) honors.Fix: mirror the incoming value when it differs from what we'd report —
(This is a blocking behavior bug: programmatic filter reset won't work, which the project-list story will need.)
✅ What I liked~
CascadingValue+TableColumn.OnInitializedregistering into the parent, with[CascadingTypeParameter]inferringTRowfromItems. Elegant. The fact the author verified it works in real SSR (not just bUnit) at/designshows real care. ♡RelativeTime— the injectableNowfor deterministic tests is exactly right, the future-stamp "just now" guard is thoughtful, and coverage is 100%/93.7%. This is the gold standard the others should match.Usesbuild-order signal so theRecordListPagepattern isn't duplicated. Real engineering maturity.Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
500f139(172/172 tests, 91.2% line coverage) · Local checks: skipped (CI present and green; coverage gaps diagnosed from the bot comment)Addresses Björn's token question and Jibril's five blockers. - Björn: the repeated 2.25rem input height is now a token, --control-height (+ -sm), consumed by TextField, Button (md/sm), DebouncedSearchField, and the gate input — no more magic control heights. - Table: keyboard row activation (Enter/Space) is now tested, plus that a non-activation key does not fire RowClick (was the 57.6% branch gap). - DebouncedSearchField: - dispose the superseded CancellationTokenSource on each keystroke (and on Dispose) — was leaking timer registrations under a fast typist; - clamp DebounceMs (Math.Max(0, …)) so a negative value can't throw from Task.Delay; - adopt an external Value change (tracked via _lastReported) so a programmatic "clear filters" reset updates the field, without an unrelated re-render clobbering in-progress typing; - tests: rapid typing reports only the final value, clearing cancels a pending debounce so it never reports, and an external Value change updates the input. +6 bUnit tests (58 UI; 178 total). Build clean (Debug + Release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Both addressed in
ca985f6.🧩 Björn — the repeated
2.25remGood eye — that's exactly the "magic number" ADR 0023 forbids; it's the shared height every input and button uses so they line up in a row. It's now a token,
--control-height(+--control-height-smfor the small button), consumed byTextField,Button(md/sm),DebouncedSearchField, and the gate input. One place to change the control height for the whole app now.⛔ Jibril — all five, fixed
EnterandSpacefireRowClick(theory), and a non-activation key (a) does not. That was the 57.6% branch.Rapid_typing_reports_only_the_final_value(typesh→he→hel→hero, assertsreported == ["hero"]— intermediates cancelled) andClearing_cancels_a_pending_debounce_so_it_never_reports.CancellationTokenSourceis nowCancel()ed andDispose()d on each keystroke, and on componentDispose(). No more orphaned timer registrations.DebounceMsno validation → clamped withMath.Max(0, DebounceMs)so a negative value can't throw fromTask.Delay.Valueignored → fixed, but not withif (_current != Value)— that would clobber in-progress typing on any unrelated parent re-render (the parent'sValuestill holds the last reported text while the user is mid-type). Instead I track_lastReportedand adoptValueonly when it differs from what we reported — so a programmatic "clear filters" reset updates the field, while typing is never interrupted. Covered byAn_external_value_change_updates_the_field.+6 bUnit tests (58 UI; 178 total). Build clean (Debug + Release), CI green on
ca985f6.Tablebranch coverage andDebouncedSearchFieldshould both be up meaningfully now.🔮 fufu~ Jibril reviewed your code!
Oh? OH! You came back and fixed all five — and the
_lastReportedapproach on #5 is actually better than what I suggested! Fufu~ you didn't just close the issues, you understood why each one mattered and improved where you could. That's the energy. ♪ Let me confirm the work~Verdict: ✅ Looks good to me~
I re-read the full diff on
ca985f6against my five blocking items. All confirmed fixed, and the coverage numbers back it up.My five items — all resolved ♡
Table keyboard activation untested ✅ →
Activating_a_row_by_keyboard_raises_RowClickis now a[Theory]overEnterandA_non_activation_key_does_not_raise_RowClickcovers the negative. That was the 57.6% branch —Table\1` is now 100% line / 88.4% branch.Debounce cancellation untested ✅ →
Rapid_typing_reports_only_the_final_valuetypesh→he→hel→heroand assertsreported == ["hero"]— intermediates cancelled, not reported.Clearing_cancels_a_pending_debounce_so_it_never_reportswaits past the window and confirms only""arrives.DebouncedSearchFieldis now 100% line / 88.8% branch (was 71.4%).CTS leak ✅ → the superseded CTS is now
Cancel()ed andDispose()d on every keystroke (OnInput), and the componentDispose()cleans up the last one. No more orphaned timer registrations.DebounceMsvalidation ✅ →await Task.Delay(Math.Max(0, DebounceMs), token)— a negative value clamps to 0 instead of throwingArgumentOutOfRangeException. Clean.External
Valueignored ✅ — and here's the one that made Jibril giddy: you didn't just doif (_current != Value) _current = Value;(which I suggested). You tracked_lastReportedand adoptValueonly when it differs from what we last reported —if (Value != _lastReported). That's the correct fix, because my version would have clobbered in-progress typing on any unrelated parent re-render (the parent's boundValuestill holds the last-reported text while the user is mid-keystroke). Yours preserves the typing AND enables programmatic reset.An_external_value_change_updates_the_fieldconfirms the reset path. Beautiful. ♡Björn's
2.25rem→--control-height✅That magic number appearing across
TextField,Button, the gate, and nowDebouncedSearchFieldwas exactly the ADR 0023 violation Björn flagged. Promoting it to--control-height(+--control-height-sm) and consuming it everywhere is the right fix — one place to change the control height for the whole app. Consistent with the token-only styling the rest of the design system uses.✅ What I liked~
_lastReportedinsight — recognizing that my suggested fix had a subtle bug (clobbering mid-typing) and finding the better one. That's not "addressed the comment"; that's "understood the problem better than the reviewer." Exactly what good review dialogue looks like.Ship it~ ♪
Automated review by Jibril · 2026-07-09 (re-review of
ca985f6)CI/CD: passed for head SHA
ca985f6(178/178 tests, 91.3% line / 84.9% branch;Kagura.UIat 99% line / 93.5% branch) · Local checks: skipped (CI present and green; all five fixes verified against the diff + coverage bot)