test(components): backfill navigation coverage#351
Open
bntvllnt wants to merge 5 commits into
Open
Conversation
bntvllnt
commented
May 18, 2026
Collaborator
Author
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — approval recommended, final approval reserved for bntvllnt
BLOCKING
None.
WARN
W1 — SearchDialog tests surface an existing DialogContent accessibility warning- Evidence: the targeted Vitest rerun passed, but each SearchDialog open case emitted Radix's
Missing Description or aria-describedby={undefined} for {DialogContent}warning. - Why it matters: this PR is test-only and does not introduce the warning, so I am not blocking the navigation coverage batch on it. It is still a useful follow-up for the accessible component library surface.
- Suggested next step: handle in a dedicated SearchDialog/CommandDialog accessibility PR or expand future tests to assert the accessible description contract once the component is fixed.
- Evidence: the targeted Vitest rerun passed, but each SearchDialog open case emitted Radix's
VERIFIED CLEAN
- Reviewed all 5 changed files at head
cc41e3f73bdf75664558fa962c03c9fc48bfec0b:packages/ui/src/components/filter-bar/filter-bar.test.tsxpackages/ui/src/components/navbar-saas/navbar-saas.test.tsxpackages/ui/src/components/search-bar/search-bar.test.tsxpackages/ui/src/components/search-dialog/search-dialog.test.tsxpackages/ui/src/components/sidebar/sidebar.test.tsx
- Cross-checked the added tests against the corresponding component implementations for filter callbacks, URL search behavior, dialog shortcut/select flow, responsive navbar state, and sidebar resize/overlay/link behavior.
- PR metadata matches the current test-only diff and links the batch with
Related to #231. - Live prepublish check confirmed PR #351 is open, non-draft,
CLEAN/mergeable, with the expected 5 changed files and all 10 visible checks passing.
VALIDATION
- Ran:
pnpm -F @vllnt/ui exec vitest run src/components/filter-bar/filter-bar.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/sidebar/sidebar.test.tsx— 5 files, 26 tests passed. - Ran:
pnpm -F @vllnt/ui exec eslint src/components/filter-bar/filter-bar.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/sidebar/sidebar.test.tsx— passed. - Ran:
git diff --check origin/main...HEAD— passed. - Did not rerun full
pnpm build, full workspacepnpm test:once, Storybook, or visual regression locally; GitHub currently reports the visible PR checks green for those gates.
Verdict: no blockers found. Approval is recommended, but I am leaving final approval to bntvllnt per review policy.
|
Preview ready · Updated 2026-05-20T17:22:45Z
Inspect
|
Collaborator
Author
Review update — manual-review-ready@bntvllnt The previous metadata-only VERIFIED LIVE
OUTCOME
|
bntvllnt
commented
May 19, 2026
Collaborator
Author
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — manual approval recommended
@bntvllnt I re-reviewed PR #351 at head 4a9f2f209853c6b9f9c1eefaa177fb2e68de3c17. I found no blockers; one existing accessibility warning remains worth tracking separately.
BLOCKING
None.
WARN
W1 — SearchDialog tests still surface the existing Radix DialogContent description warning- Evidence: the targeted local Vitest run passed, but
search-dialog.test.tsxemittedWarning: Missing Description or aria-describedby={undefined} for {DialogContent}on the dialog-open cases. - Why it matters: the warning points at the underlying
SearchDialog/CommandDialogaccessible-description contract. This PR is test-only and did not introduce that production warning, so I am not blocking the #231 coverage batch on it. - Suggested next step: fix the component accessibility contract in a dedicated SearchDialog/CommandDialog PR, then tighten tests around the description behavior.
- Evidence: the targeted local Vitest run passed, but
VERIFIED CLEAN
- Current live state verified: open, non-draft,
mergeStateStatus: CLEAN,mergeable: MERGEABLE, changedFiles=5, head matches the assigned SHA. - Full changed-file coverage reviewed:
packages/ui/src/components/filter-bar/filter-bar.test.tsxpackages/ui/src/components/navbar-saas/navbar-saas.test.tsxpackages/ui/src/components/search-bar/search-bar.test.tsxpackages/ui/src/components/search-dialog/search-dialog.test.tsxpackages/ui/src/components/sidebar/sidebar.test.tsx
- Reviewed surrounding component behavior for filter callback payloads, search URL generation, dialog keyboard/select flow, navbar mobile state, and sidebar resize/overlay/link handling.
- Rule verdicts: PASS R3 PR body freshness; PASS R5 issue link (
Related to #231); PASS issue #231 test-only scope; PASS generated-artifact policy (none touched); WARN R15 accessibility due existing DialogContent warning; PASS no shipped-code comments/runtime changes.
VALIDATION
- Ran locally in the PR worktree:
pnpm exec vitest run src/components/filter-bar/filter-bar.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/sidebar/sidebar.test.tsx— 5 files, 26 tests passed, with the warning above. - Live checks observed: issue-link enforcement, preview deploy, build/sign/scan/deploy, Storybook, visual regression, CodeQL, health scan, quality gates, and story verification all pass; only the superseded legacy preview check is neutral.
Verdict: no blockers. Approval is recommended, but final approval is reserved for bntvllnt per review policy.
- Add data-testid="navbar-saas-mobile-trigger" to mobile menu Button - Add data-testid="sidebar-overlay" to mobile overlay div - Add aria-hidden to collapsible panel div (driven by isOpen state) - Update tests to use getByTestId and aria-hidden assertions - Mirror all source changes to apps/registry registry copies
bntvllnt
commented
May 19, 2026
Collaborator
Author
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review for HEAD a9b7481.
Would request changes, but GitHub does not allow this authenticated account to request changes on its own pull request. Treat the finding below as blocking before merge.
Validation reviewed:
- PR metadata: open/non-draft, base main, linked issue present (
Related to #231), body references current HEAD. - GitHub checks: 13/13 observed checks are passing, including Quality Gates, CodeQL, Storybook Tests, Visual Regression, Verify Stories, PR Issue Link, and preview deploy.
- Local targeted validation:
pnpm -F @vllnt/ui exec vitest run src/components/filter-bar/filter-bar.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/sidebar/sidebar.test.tsxpassed: 5 files / 26 tests.
Note: SearchDialog tests emit non-fatal missing Dialog description warnings; I did not treat those as introduced by this PR.
| /> | ||
| </button> | ||
| <div | ||
| aria-hidden={!isOpen} |
Collaborator
Author
There was a problem hiding this comment.
Blocking: this adds aria-hidden to a wrapper that still contains real Link anchors when the section is collapsed. aria-hidden does not remove descendants from the tab order, and ARIA-hidden content must not contain focusable descendants; keyboard focus can still move into visually collapsed links while assistive tech is told to ignore them. Please remove the focusable descendants while collapsed (for example unmount/hidden/inert, or make the links non-focusable while closed) and keep the registry mirror in sync.
Collaborator
Author
|
Addressed the collapsed sidebar accessibility review item. Changes:
Validation run locally from
|
bntvllnt
commented
May 20, 2026
Collaborator
Author
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review for HEAD 66561de55c35b9bb8dca3b538a7ce0bc44f10e57.
Would request changes, but GitHub does not allow this authenticated account to request changes on its own pull request. Treat the metadata finding below as blocking before merge.
BLOCKING
R3 — PR body still describes the previous head, not the reviewed head- Evidence: the live PR body still says
Current head: a9b7481fad9a1fa11b41aca48d9e2f7521742b4b,9 files changed, 636 insertions, and says the body was refreshed to remove the stale head reference. The live current head is66561de55c35b9bb8dca3b538a7ce0bc44f10e57, and GitHub reports9 files changed, 643 insertions. - Why it matters: repo rule R3 requires the PR body to match HEAD before merge. The body also describes the source/registry edits as stable test IDs, but the current diff includes the
hidden={!isOpen}sidebar behavior/semantics fix that resolved the earlier focusability blocker; that behavior change should be named explicitly. - Fix: refresh the PR body for head
66561de55c35b9bb8dca3b538a7ce0bc44f10e57: update the current head/stat lines, validation timestamp/evidence, and source/registry scope text to include the sidebarhidden={!isOpen}fix alongside thedata-testidadditions.
- Evidence: the live PR body still says
WARN
W1 — SearchDialog tests still surface the existing DialogContent description warning- Evidence: the current-head targeted Vitest rerun passed, but the SearchDialog cases still emit
Warning: Missing Description or aria-describedby={undefined} for {DialogContent}. - Why it matters: this is an existing SearchDialog/CommandDialog accessibility contract warning rather than the blocker fixed by the current sidebar change, so I am not treating it as introduced by this PR.
- Suggested next step: track/fix the SearchDialog accessible-description contract separately, then tighten tests around it.
- Evidence: the current-head targeted Vitest rerun passed, but the SearchDialog cases still emit
VERIFIED CLEAN
- Current live state verified: PR open, non-draft,
mergeStateStatus: CLEAN,mergeable: MERGEABLE, head66561de55c35b9bb8dca3b538a7ce0bc44f10e57. - Reviewed all 9 changed files:
apps/registry/registry/default/navbar-saas/navbar-saas.tsxapps/registry/registry/default/sidebar/sidebar.tsxpackages/ui/src/components/filter-bar/filter-bar.test.tsxpackages/ui/src/components/navbar-saas/navbar-saas.test.tsxpackages/ui/src/components/navbar-saas/navbar-saas.tsxpackages/ui/src/components/search-bar/search-bar.test.tsxpackages/ui/src/components/search-dialog/search-dialog.test.tsxpackages/ui/src/components/sidebar/sidebar.test.tsxpackages/ui/src/components/sidebar/sidebar.tsx
- Prior sidebar blocker is fixed/superseded: the collapsed section wrapper now uses
hidden={!isOpen}in both package source and registry copy, so collapsed focusable descendants are removed from the accessibility tree/tab discovery path instead of only being visually collapsed/ARIA-hidden. - Added tests cover filter callbacks, search URL behavior, SearchDialog shortcut/select flow, responsive navbar trigger state, and sidebar resize/overlay/link/collapsible behavior. The selector additions are mirrored between package source and registry copies.
- Viewed-state tracking was applied successfully to all 9 changed files.
VALIDATION
- GitHub checks observed at current head: all visible checks passing/neutral, including Quality Gates, CodeQL, Storybook Tests, Visual Regression, Verify Stories, issue-linked PR enforcement, preview deploy, and registry/storybook deploy checks.
- Ran locally in the review worktree:
pnpm -F @vllnt/ui exec vitest run src/components/filter-bar/filter-bar.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/sidebar/sidebar.test.tsx— 5 files / 26 tests passed. - Ran locally in the review worktree:
pnpm -F @vllnt/ui exec eslint src/components/filter-bar/filter-bar.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/navbar-saas/navbar-saas.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/sidebar/sidebar.test.tsx src/components/sidebar/sidebar.tsx— exit 0. - Registry files are ignored by the app lint config;
--no-ignorereports project-service inclusion noise for the registry artifact paths rather than PR-specific code errors. Current-head CI registry/storybook checks are 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
1 participant
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.
Summary
filter-bar,search-bar,search-dialog,navbar-saas, andsidebar.navbar-saas/sidebarsource and registry examples only to add stable test IDs used by the new tests; this is not test-only.Related to #231
Diff scope at current HEAD
Current head:
a9b7481fad9a1fa11b41aca48d9e2f7521742b4bpackages/ui/src/components/filter-bar/filter-bar.test.tsxpackages/ui/src/components/navbar-saas/navbar-saas.test.tsxpackages/ui/src/components/search-bar/search-bar.test.tsxpackages/ui/src/components/search-dialog/search-dialog.test.tsxpackages/ui/src/components/sidebar/sidebar.test.tsxpackages/ui/src/components/navbar-saas/navbar-saas.tsxpackages/ui/src/components/sidebar/sidebar.tsxapps/registry/registry/default/navbar-saas/navbar-saas.tsxapps/registry/registry/default/sidebar/sidebar.tsxValidation
Current-head GitHub checks observed green as of 2026-05-19T18:54:00Z:
Quality Gates: SUCCESSAnalyze (actions): SUCCESSEnforce issue-linked PRs: SUCCESSVerify Stories: SUCCESSbuild · sign · scan · deploy (ui-registry): SUCCESSScan codebase health: SUCCESSAnalyze (javascript-typescript): SUCCESSbuild · sign · scan · deploy (storybook): SUCCESSBuild Storybook: SUCCESSStorybook Tests: SUCCESSVisual Regression: SUCCESSCodeQL: SUCCESSvllnt-pilot / preview deploy: SUCCESSAdditional validation evidence retained from the branch before the final stable-selector refresh:
pnpm -F @vllnt/ui exec vitest run src/components/filter-bar/filter-bar.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/sidebar/sidebar.test.tsx— 5 files, 26 tests passedpnpm -F @vllnt/ui exec eslint src/components/filter-bar/filter-bar.test.tsx src/components/search-bar/search-bar.test.tsx src/components/search-dialog/search-dialog.test.tsx src/components/navbar-saas/navbar-saas.test.tsx src/components/sidebar/sidebar.test.tsxpnpm -F @vllnt/ui lintpnpm -F @vllnt/ui exec tsc --noEmit --project tsconfig.build.jsonpnpm buildpnpm test:once— 221 files, 1241 tests passedpnpm -F @vllnt/ui check:use-clientpnpm -F @vllnt/ui exec tsx scripts/check-story-coverage.tspnpm -F @vllnt/ui exec tsx scripts/verify-stories.tspnpm -F @vllnt/ui build-storybookgit diff --checkReview status
mergeStateStatusisCLEANat the current head.