Feat/add navigation

[piotrNBTN]

No description provided.

[claude]

Claude finished @piotrNBTN's task in 2m 46s —— View job

---

PR Review — Feat/add navigation

Verdict: REQUEST_CHANGES (informational — the human decides)

The slice is small, well-tested, and follows the red→green convention cleanly. The main blocker is a dead link shipped to main; everything else is non-blocking.

---

Spec compliance

• ⚠️ No linked issue. PR body is empty, branch is feat/add-navigation (not afk/issue-<num> / bugfix/issue-<num> / burst/<num>), and there's no Closes #N. Per CLAUDE.md → Workflow constitution this PR violates "Branch per issue" and "One PR per issue. PR links the issue with 'Closes #'.". The red commit references "AC1-AC6" but those acceptance criteria don't exist in docs/PRD.md or docs/modules/, so spec compliance can't be objectively verified. Recommend opening an issue retroactively or linking the intended one.
• ✅ TDD pattern is respected: 7382419 test(red) precedes 5dea359 feat(green).

Architecture findings

• 🛑 Dead-link in shipped UI — apps/web/src/components/AppNav.tsx:17 points the doctor's "My Schedule" entry at /doctor/schedule, but no such route is registered in apps/web/src/main.tsx (only doctorRoute, patientRoute, patientDocumentsRoute, loginRoute, indexRoute). Doctors who click it will hit TanStack Router's not-found state. Either land the schedule route in this PR (true vertical slice) or remove the link until Module 03 / issue Slice 2a (Module 03): Slot table + list-own + read-only slot management page #20 lands. (ARCHITECTURE_RULES.md is silent on this, but the project's "vertical slice" rule in CLAUDE.md calls for end-to-end value.) Fix this →
• ⚠️ Domain-aware component in components/. ARCHITECTURE_RULES.md line 28: "Shared components in components/ must be domain-agnostic." AppNav hardcodes patient/doctor labels and routes — that's a domain concept. Consider moving to features/auth/AppNav.tsx or features/nav/.
• ⚠️ Untyped to defeats TanStack Router's compile-time path safety. In AppNav.tsx:5-19, to: string is passed to <Link to={to}>. TanStack Router types to against the registered route tree — typing to as the union of valid paths would have caught the /doctor/schedule problem at tsc time. Use LinkProps['to'] or a typed const.
• ⚠️ Triplicated logout handler. apps/web/src/routes/patient.tsx:35-39, apps/web/src/routes/doctor.tsx:36-40, apps/web/src/routes/patient/documents.tsx:42-46 are byte-identical. Extract useLogout() into features/auth/ and reuse — fits the "feature folders own their queries/hooks" rule.
• ✅ No barrel files, no class components, no any, named exports, kebab-case for non-component files, PascalCase for the component, co-located test. Clean.

Security findings

• ✅ userName is interpolated as a child ({userName}), so React escapes it — no XSS surface.
• ✅ role is provided by routes that already enforce role guards in beforeLoad; nothing here weakens that.
• ✅ No secrets, no PII pulled in, no new fetch paths.

Test quality

• ✅ Tests assert external behavior (accessible roles, hrefs, visible text), not implementation details. getByRole('link', { name: ... }) is the right shape.
• ✅ RBAC isolation is covered both ways (apps/web/src/components/AppNav.test.tsx:33-36, 62-65).
• ⚠️ Active-link branch is dead-tested. The mock pins useRouterState to { pathname: '/' } (line 9), so the isActive branch in AppNav.tsx:41-50 is never executed by any test. Add a test that swaps the mock to /patient and asserts the active styling (or a aria-current="page" if you add one — would also improve a11y).
• ⚠️ onLogout is asserted to render but never invoked. Add a userEvent.click(button) test that confirms the prop is called.
• ⚠️ Duplicate userName="Anna" setup across patient tests — minor; a beforeEach or shared renderPatient() helper would tighten things.
• ℹ️ Mocking @tanstack/react-router at module level is fine for a unit test on this presentational component, but watch for drift: if Link's real prop names change (e.g. to → params), this mock won't notice.

Suggestions (non-blocking)

• The vitest package is referenced by apps/web ("test": "vitest run") but not declared in apps/web/package.json devDependencies. It currently resolves via pnpm hoisting from the root, which works but is implicit — adding it to apps/web/package.json would make the workspace honest.
• Consider <nav> on the existing <nav> and aria-current="page" on the active link for a11y.
• The Role type duplicates what's likely already in @medbridge/contracts (auth user role). Importing the canonical type avoids drift.
• mb-2 truncate on the username (AppNav.tsx:61) is good, but a title={userName} would surface the full name on hover when it's truncated.

---

• Identify linked issue & read acceptance criteria (none linked — flagged)
• Read ARCHITECTURE_RULES, GLOSSARY, ADRs
• Review changed files (AppNav + route changes + vitest setup)
• Check tests for AC coverage & external behavior
• Post final review

ℹ️ I was unable to run pnpm verify locally — pnpm is not installed in this runner and global install requires permission. The findings above are based on careful code reading, not test execution.
· Branch feat/add-navigation