Skip to content

fix(a11y): add primary navigation landmark - #1144

Merged
khaira777 merged 4 commits into
mainfrom
codex/sidebar-nav-landmark
Oct 15, 2026
Merged

khaira777 merged 4 commits into
mainfrom
codex/sidebar-nav-landmark

Conversation

@khaira777

@khaira777 khaira777 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Intent

PR 9 - Fix the active navigation landmark (Fixes #1097)
Branch: codex/sidebar-nav-landmark
Title: fix(a11y): add primary navigation landmark

Requirements:

  • Wrap the application's actual primary link group in a labelled semantic navigation landmark.
  • Keep profile, theme and other non-navigation controls outside it.
  • Use existing localization patterns for the accessible label.
  • Preserve the shared sidebar primitive, collapse/mobile behaviour, styling and keyboard interaction.

Acceptance: the authenticated app has one primary-navigation landmark containing its navigation links. Verify desktop/mobile, RTL, keyboard access and collapse.

What Changed

  • Wrapped the primary SidebarMenu link list in AppSidebar (frontend/src/components/layout/Sidebar.tsx) with a <nav aria-label={tCommon('menu')}> landmark while keeping header, theme, and profile controls outside the navigation element.
  • Added end-to-end assertions in frontend/e2e/layout-integrity.spec.ts covering the primary navigation landmark across desktop, collapsed icon-rail, RTL (fa), keyboard focus/tab traversal, and mobile drawer states.

Risk Assessment

✅ Low: The change cleanly wraps the primary sidebar link menu in a localized

landmark while keeping header, footer, theme toggle, profile menu, and rail controls outside it, with comprehensive E2E coverage across desktop, collapsed, keyboard, RTL, and mobile states.

Testing

Built the backend and desktop static frontend, stood up an isolated FloCafe E2E server instance on task-local ports, ran the Playwright sidebar suite in frontend/e2e/layout-integrity.spec.ts (including the new #1097 primary navigation landmark test and existing sidebar geometry and theme-toggle tests), and captured four visual screenshot artifacts covering desktop expanded with profile menu open, collapsed icon rail, RTL Persian localization, and mobile drawer states; all checks passed.

  • Live validation: ✅ go - 5 of 5 scenarios driven live against the product
Scenario Result Live Evidence
Authenticated user on desktop expanded sidebar sees a single localized <nav aria-label="Menu"> landmark wrapping all primary navigation links ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-desktop-expanded-nav-landmark.png
Non-navigation sidebar controls (header brand link, theme toggle, resize rail, and opened user profile dropdown menu items Support/Logout) remain outside the primary <nav> landmark ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-desktop-expanded-nav-landmark.png
Keyboard Tab traversal and Control+b collapsed icon-rail toggle preserve the single primary navigation landmark and link focusability ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-desktop-collapsed-icon-rail.png
Switching to RTL locale (Persian fa) localizes the primary navigation landmark label to "منو" and positions the landmark on the right (inline-start) viewport edge with theme toggle excluded ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-rtl-persian-nav-landmark.png
On a 390x844 mobile viewport, zero navigation landmarks exist while closed, opening the mobile drawer mounts <nav aria-label="Menu"> inside the mobile sheet excluding theme/profile controls, and click… ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-mobile-drawer-nav-landmark.png

Desktop expanded sidebar with primary navigation landmark and profile menu open outside <nav>
Desktop collapsed icon-rail sidebar preserving primary navigation landmark and keyboard focus
RTL Persian sidebar with localized primary navigation landmark (منو) on the right viewport edge
Mobile drawer open with primary navigation landmark inside sheet and non-navigation controls outside <nav>

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • Live validation: ✅ go - 5 of 5 scenarios driven live against the product
Scenario Result Live Evidence
Authenticated user on desktop expanded sidebar sees a single localized <nav aria-label="Menu"> landmark wrapping all primary navigation links ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-desktop-expanded-nav-landmark.png
Non-navigation sidebar controls (header brand link, theme toggle, resize rail, and opened user profile dropdown menu items Support/Logout) remain outside the primary <nav> landmark ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-desktop-expanded-nav-landmark.png
Keyboard Tab traversal and Control+b collapsed icon-rail toggle preserve the single primary navigation landmark and link focusability ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-desktop-collapsed-icon-rail.png
Switching to RTL locale (Persian fa) localizes the primary navigation landmark label to "منو" and positions the landmark on the right (inline-start) viewport edge with theme toggle excluded ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-rtl-persian-nav-landmark.png
On a 390x844 mobile viewport, zero navigation landmarks exist while closed, opening the mobile drawer mounts <nav aria-label="Menu"> inside the mobile sheet excluding theme/profile controls, and click… ✅ pass live ~/.no-mistakes/evidence/01M4YEGCQ45MR6T5WJJ1DJR8AR/sidebar-mobile-drawer-nav-landmark.png
  • npm run build && npm run build:frontend
  • CI=1 E2E_BASE_URL=http://127.0.0.1:43101 E2E_KDS_BASE_URL=http://127.0.0.1:43102 E2E_SERVER_APP_BASE_URL=http://127.0.0.1:43103 npx --prefix frontend playwright test --config=frontend/playwright.config.ts --project=chromium frontend/e2e/layout-integrity.spec.ts -g "1097"
  • CI=1 E2E_BASE_URL=http://127.0.0.1:43101 E2E_KDS_BASE_URL=http://127.0.0.1:43102 E2E_SERVER_APP_BASE_URL=http://127.0.0.1:43103 npx --prefix frontend playwright test --config=frontend/playwright.config.ts --project=chromium frontend/e2e/layout-integrity.spec.ts -g "sidebar"
  • Live Playwright screenshot capture across desktop expanded, collapsed icon-rail, RTL Persian (fa), and mobile drawer (390x844) against isolated tests/e2e-server.cjs
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: FreeOpenSourcePOS/FloCafe/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9be0386a-849e-4ee4-8414-10c9a7914ca5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@khaira777

Copy link
Copy Markdown
Contributor Author

@greptile review

@khaira777 khaira777 changed the title fix(frontend): add primary navigation landmark to sidebar fix(a11y): add primary navigation landmark Oct 15, 2026
@khaira777

Copy link
Copy Markdown
Contributor Author

@greptileai review

…`frontend/e2e/rtl-setup-auth-settings.spec.ts` to `page.locator('[data-slot="tabs"] nav')` so it does not collide in Playwright strict mode with the new primary sidebar `<nav aria-label={tCommon('menu')}>` landmark introduced by this PR
@greptile-apps

greptile-apps Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no actionable issues were found.

Summary

Adds a localized navigation landmark around the sidebar’s primary links.

  • The sidebar’s primary links have a navigation landmark with a localized name.
  • The existing login, setup, and RTL tests keep checking the same flows.

No actionable issues were found. Tests and visual checks were not run during this review.

Reviews (3) · Last reviewed commit: "Merge branch 'main' into codex/sidebar-n..." · Reviewed by Greptile

….ts` with `oxfmt` (`npx oxfmt --write -- e2e/layout-integrity.spec.ts e2e/rtl-setup-auth-settings.spec.ts src/components/layout/Sidebar.tsx` from `frontend/`) so the changed-file formatting gate in `Lint & Build Validation (Linux)` and its downstream `Required Checks Gate` succeed. Verified with `npx oxfmt --check -- e2e/layout-integrity.spec.ts e2e/rtl-setup-auth-settings.spec.ts src/components/layout/Sidebar.tsx` and `git diff --check`
@khaira777

Copy link
Copy Markdown
Contributor Author

@greptileai review

@khaira777

Copy link
Copy Markdown
Contributor Author

@greptileai review

@khaira777
khaira777 merged commit c2093d6 into main Oct 15, 2026
24 checks passed
@khaira777
khaira777 deleted the codex/sidebar-nav-landmark branch October 15, 2026 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(a11y): wrap sidebar navigation items in semantic nav landmark

1 participant