Repository navigation
fix(platform): surface block counter read failures - #4314
Conversation
yannickmonney
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — TALE-675 independent review of PR #4314
Exact reviewed head: bfae46d548cb60c502d03ae65713fc281bd88caf.
Reviewer: Codex #8 579ebc6d-a30d-43a6-b3fb-a340b6a22eae, run 9b8cb04c;
distinct from author Codex #11 7a7d20b9, implementation run b74afe6e.
Blocking finding — P2: Retry loses keyboard focus during recovery
services/platform/app/features/settings/audit-logs/components/block-counters-table.tsx:108
passes error and bare onRetry={refetch}, but no retry-focus preservation or
stable onErrorFocusLost target. The shared DataTable mounts
ErrorDisplayCompact only while error is truthy (DataTable:614); that display's
focus handoff requires its host to provide onFocusLost (display:74).
Reproduction on this exact head: start with a settled failed first read; focus
Try again; activate with Enter (refetch called once); advance the query to
error:null, isLoading:true; settle another failure. The new Try again button
is not focused: activeElement is the page body. The member loses their place
both while pending and when recovery fails again. This is a real query-state
sequence: installed TanStack query-core src/query.ts:701 resets a fetching
query without data to error:null, status:'pending'.
The independent jsdom probe review-probes.test.tsx:22 fails at the restored
focus assertion, while a deliberately moved-focus control passes. The PR's two
committed tests never advance the query after Retry, so their click plus axe
check does not prove focus-safe recovery.
Please use the shared recovery-focus mechanism (as in #4294's
useRetryFocus/arm/ref wiring) or an equivalent stable, named handoff; preserve
focus deliberately moved elsewhere. Add regression coverage for keyboard
activation → pending → another failure and successful recovery, not just a
refetch spy. This violates the explicit focus-safe Retry acceptance criterion.
Under root's 08:15Z rule this blocker needs independent exact-head closure with
proof before acceptance/landing; green CI or a prior acceptance cannot close it.
Criteria that pass
- Failed reads render shared localized recovery, never the no-blocks reassurance;
Retry calls the same query refetch rather than launching another operation. - A successful
[]keeps “No sign-in blocks” and its existing description. - EN/DE/FR rendered headings and retry names pass independent probes using the
existing shared catalogs; no new translation keys are needed. - Shared error/retry/empty-state separation matches #4294/#4304 in intent, but
the retry-focus lifecycle above is not carried through here. - PR body explicitly contains
Closes #3852andCloses #3854, covering the
source issue and duplicate. They remain open until normal PR completion. - The committed regression replayed against fetched main
3af9e704da068b15f464e0a5f6fd48da1c99e639fails the failed-read assertion;
its successful-empty control passes. Main's actual full SHA is recorded in
verification.md; the component snapshot is retained separately.
Observed verification
Installed Node 24.21.0 / Vitest 4.1.11, one worker, 2048 MiB heap, existing deps:
- Exact-head committed tests: 2/2 pass, including axe accessibility check.
- Independent probes: 4 pass (EN/DE/FR and deliberately moved focus), 1 fails
(keyboard retry → pending → failure loses focus). Combined: 6 pass, 1 fail. - AuditLogsPage and deep-link consumers: 2 files / 18 tests pass.
- Main negative control: 1 expected failure / 1 successful-empty pass.
- Changed-file Oxlint
--type-aware --type-check --threads=1: exit 0. - Changed-file Oxfmt check: pass. PR diff whitespace check: exit 0.
- Passive GitHub snapshot: five Candidate source checks queued; no CI success
claimed and no CI rerun or cancellation.
Harness caveats: initial external-wrapper attempts hit config array merging,
a parse error, file-serving restrictions and external JSX dependency resolution.
The array-merged attempt was stopped promptly because it selected out-of-scope
files; it returned no test proof. Only the corrected explicitly filtered runs
above are evidence. No installs or production changes.
Unrun: real browser keyboard/focus, screen reader, visual analyzer/layout,
HTTP/backend/Postgres integration, E2E, whole-platform tests/typecheck/check,
SAST/manual/docs gates. jsdom and scoped static checks are not those proofs.
No source repair, commit, push, merge, card move or protected-review bypass.
Author/manager owns the focus repair; a distinct reviewer must close this finding
on the new exact head. TALE-359 retains CI/landing reconciliation authority.
TALE-284 Publication ReceiptOwner: Codex #11 ( Verified clean tree and unchanged local commit Performed ONE ordinary fast-forward push on existing Workflow Readback
Check-run readback: 16 queued, 2 in progress, 5 Candidate source skipped. All exact IDs, names and URLs are retained in HandoffIndependent P2 |
yannickmonney
left a comment
There was a problem hiding this comment.
ACCEPT — TALE-720 independent exact-head re-review of PR #4314
Reviewed head: 9c0eff165cd2888c0eb6c05b9a61746fe2fea212 (only).
Reviewer: Codex #8 579ebc6d-a30d-43a6-b3fb-a340b6a22eae, run 2350bacb-284d-40f3-96a5-978cc9321c3f; distinct from implementation author Codex #11 7a7d20b9.
Disposition: P2 631153d9 (retry focus) is CLOSED at this exact head. No blocking findings in this re-review. This supersedes my REQUEST_CHANGES on bfae46d548cb60c502d03ae65713fc281bd88caf for that finding only.
Why the repair closes P2
BlockCountersTablearms shareduseRetryFocusbefore calling the same query'srefetch. Its inner container persists across error, loading, empty and populated states, and the hook is keyed by organization ID.- The outer
role="region", localized caption andtabIndex={-1}provide a stable target for DataTable'sonErrorFocusLost. ShareduseFocusHandoffschedules a handoff only when the disappearing error display actually held focus; the callback acts only if focus is still on body/documentElement. - On another failure, shared
useRetryFocusrecognizes the focused enclosing region as stranded focus and returns it to the replacement Try again. On successful empty/populated recovery, focus remains on the named region. No gratuitous initial-error focus move is introduced. - Deliberate moves to another control during retry survive both failure and success; healing an error without a loading transition also preserves deliberate outside focus. I independently ran the committed regression cases, rather than relying on the author's green-test claim.
- Failed reads still take DataTable's explicit error branch, never the no-blocks reassurance. A successful
[]retains the existing empty title/description. The two previous tests and their assertions are preserved; no assertions, mocks, axe rules or shared helpers were weakened. The repair changes only the component and its test file. - EN/DE/FR use existing complete service/shared catalogs: named regions are “Sign-in block activity”, “Anmeldeblockaden”, and “Activité des blocages de connexion”; retry labels are localized. All three locale render/retry/axe cases pass. No new translation keys are needed.
Verification I ran
Installed /opt/node/bin/node (Node 24.21.0), Vitest 4.1.11, existing dependencies, one worker, 2048 MiB heap. No install or browser/backend stack.
From services/platform:
NODE_OPTIONS=--max-old-space-size=2048 /opt/node/bin/node ../../node_modules/vitest/vitest.mjs run --config vitest.ui.config.ts --maxWorkers=1 app/features/settings/audit-logs/components/block-counters-table.test.tsx app/features/settings/audit-logs/components/audit-logs-page.test.tsx app/features/settings/audit-logs/components/audit-logs-deep-link.test.tsxResult: 3 files, 30 tests passed, 17.75 s (12 BlockCountersTable, 15 AuditLogsPage, 3 deep-link). Includes keyboard Enter → loading → failure/empty/rows, intentional moved focus, direct healing, error/empty separation, EN/DE/FR and the existing axe checks. Node was selected up front; no z.number collection failure occurred.
- Changed-file Oxlint
--type-aware --type-check --threads=1: exit 0, run against both changed TSX files (scoped lint/type checking, not whole-workspace tsc). - Changed-file Oxfmt
--check: pass, both files. git diff origin/main...HEAD --check: exit 0.- Exact head verified and repository worktree clean. No source changes, commits, push, merge, native review decision or protected-review bypass.
Hosted CI — one passive exact-head snapshot
GitHub check-runs read once at 2026-10-04 22:35:42 UTC, all records bound to the full reviewed SHA: 55 completed checks: 36 success, 18 skipped, 1 neutral (Trivy); no failing, queued or in-progress checks at that snapshot. Raw names, SHA and job URLs are in hosted-ci.json.
Success includes Type check, Lint, Format, Unit, UI, all four platform UI shards, Browser, all four platform Playwright shards, Build and Lint commits. Backend integration and candidate-only jobs are skipped, not run evidence. The neutral Trivy result is not described as a successful scan. No CI watch, rerun or cancel.
These are hosted result statuses, not proof every underlying task executed freshly: I did not inspect Turbo cache replays or Playwright retry artifacts. Hosted green browser/Playwright statuses do not constitute my own browser observation.
Explicitly unrun / next owner
Unrun by me: real-browser keyboard/focus (including focus styling and tab order), screen reader/assistive technology, visual analyzer/layout, backend/HTTP/Postgres integration, E2E, whole-platform tests/tsc/check and other full acceptance gates. jsdom axe intentionally does not judge rendered color contrast. I did not replay the author's three baseline-red tests at bfae46d5; the independently observed evidence is the exact-head 30-test run plus source/assertion comparison.
This is an independent light re-review ACCEPT, not a merge or a native protected-human approval. Preserve TALE-284's root pending capture. TALE-359 owns any final CI-log/retry/cache reconciliation, required browser/AT/visual acceptance and authorized landing. This verdict does not apply to a later head.
What changed
Verification
bunx vitest --run --config vitest.ui.config.ts app/features/settings/audit-logs/components/block-counters-table.test.tsxbunx oxlint --type-aware app/features/settings/audit-logs/components/block-counters-table.tsx app/features/settings/audit-logs/components/block-counters-table.test.tsxCloses #3852
Closes #3854