fix(gui-v2): breadcrumb navigation via router.push + button type + void logout

- FIX A (IMPORTANT): PrimeVue Breadcrumb ignores `route` key; map non-last items with `command: () => router.push(item.to)` for real client-side nav - FIX B: add type="button" to all 6 native <button> chrome elements - FIX C: authStore.logout() bare call matches project no-void pattern - FIX D: document param-route edge case in toBreadcrumbItems - FIX E: regression test asserts command+push on non-last, no command on last, no `route` key on any item Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-16 21:07:57 +02:00
parent 4f1fb7385b
commit 615a114f33
3 changed files with 108 additions and 12 deletions
--- a/apps/app/src/components-v2/layout/AppTopbar.vue
+++ b/apps/app/src/components-v2/layout/AppTopbar.vue
@@ -37,6 +37,7 @@ import Menu from 'primevue/menu'
 import OverlayBadge from 'primevue/overlaybadge'
 import Popover from 'primevue/popover'
 import { computed, ref } from 'vue'
+import { useRouter } from 'vue-router'
 import type { MenuItem } from 'primevue/menuitem'
 import Icon from '@/components/Icon.vue'
 import { useBreadcrumb } from '@/composables/useBreadcrumb'
@@ -45,11 +46,12 @@ import { useShellUiStore } from '@/stores/useShellUiStore'
 import { computeOrgGradient } from '@/utils/v2/gradient'

 // ---------------------------------------------------------------------------
-// Stores
+// Stores / router
 // ---------------------------------------------------------------------------

 const shell = useShellUiStore()
 const authStore = useAuthStore()
+const router = useRouter()

 // ---------------------------------------------------------------------------
 // Breadcrumb — route-driven via useBreadcrumb()
@@ -59,14 +61,27 @@ const { items: breadcrumbItems } = useBreadcrumb()

 /**
 * Map BreadcrumbItem[] → PrimeVue MenuItem[].
- * Non-last items get a `route` for router-link rendering.
- * Last item (current) has no route/command — plain label only.
+ *
+ * The installed PrimeVue Breadcrumb (BreadcrumbItem.vue) renders
+ * `<a :href="item.url || '#'">` and calls `item.command` on click.
+ * It does NOT honour a `route` key — router-link is never invoked.
+ *
+ * Fix: non-last items navigate via `command: () => router.push(item.to)` (client-side,
+ * no full reload, no href="#"). Last/current item has no `to` from useBreadcrumb()
+ * → no command → non-interactive.
 */
 const breadcrumbModel = computed<MenuItem[]>(() =>
-  breadcrumbItems.value.map(item => ({
-    label: item.label,
-    ...(item.to !== undefined ? { route: item.to } : {}),
-  })),
+  breadcrumbItems.value.map(item => {
+    const base: MenuItem = { label: item.label }
+
+    if (item.to !== undefined) {
+      base.command = () => {
+        router.push(item.to!)
+      }
+    }
+
+    return base
+  }),
 )

 // ---------------------------------------------------------------------------
@@ -214,7 +229,9 @@ const userMenuItems = computed<MenuItem[]>(() => [
      {
        label: 'Sign out',
        icon: 'tabler-logout',
-        command: () => authStore.logout(),
+        command: () => {
+          authStore.logout()
+        },
      },
    ],
  },
@@ -235,6 +252,7 @@ const userMenuItems = computed<MenuItem[]>(() => [
        .icon-btn: 38x38, rounded, transparent bg, fg-muted color, hover bg-hover
      -->
      <button
+        type="button"
        class="lg:hidden inline-flex h-[38px] w-[38px] items-center justify-center rounded-[var(--p-border-radius)] border-0 bg-transparent text-[var(--p-text-muted-color)] transition-colors duration-150 hover:bg-[var(--p-content-hover-background)] hover:text-[var(--p-text-color)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-[var(--p-primary-color)] focus-visible:outline-offset-1"
        aria-label="Open menu"
        @click="shell.setMobileOpen(true)"
@@ -295,6 +313,7 @@ const userMenuItems = computed<MenuItem[]>(() => [
      -->
      <button
        v-if="authStore.currentOrganisation"
+        type="button"
        class="ws-mobile-btn-shadow lg:hidden inline-flex h-[38px] w-[38px] flex-shrink-0 items-center justify-center rounded-[var(--p-border-radius)] border-0 text-[12px] font-bold text-white"
        :style="{
          background: `linear-gradient(135deg, ${mobileOrgGradient[0]}, ${mobileOrgGradient[1]})`,
@@ -332,6 +351,7 @@ const userMenuItems = computed<MenuItem[]>(() => [
        .icon-btn: 38x38, inline-flex, centered, rounded, transparent, fg-muted, hover bg-hover
      -->
      <button
+        type="button"
        class="inline-flex h-[38px] w-[38px] items-center justify-center rounded-[var(--p-border-radius)] border-0 bg-transparent text-[var(--p-text-muted-color)] transition-colors duration-150 hover:bg-[var(--p-content-hover-background)] hover:text-[var(--p-text-color)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-[var(--p-primary-color)] focus-visible:outline-offset-1"
        :aria-label="densityAriaLabel"
        @click="toggleDensity"
@@ -344,6 +364,7 @@ const userMenuItems = computed<MenuItem[]>(() => [

      <!-- Theme toggle. -->
      <button
+        type="button"
        class="inline-flex h-[38px] w-[38px] items-center justify-center rounded-[var(--p-border-radius)] border-0 bg-transparent text-[var(--p-text-muted-color)] transition-colors duration-150 hover:bg-[var(--p-content-hover-background)] hover:text-[var(--p-text-color)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-[var(--p-primary-color)] focus-visible:outline-offset-1"
        :aria-label="themeAriaLabel"
        @click="toggleTheme"
@@ -364,6 +385,7 @@ const userMenuItems = computed<MenuItem[]>(() => [
        severity="danger"
      >
        <button
+          type="button"
          class="inline-flex h-[38px] w-[38px] items-center justify-center rounded-[var(--p-border-radius)] border-0 bg-transparent text-[var(--p-text-muted-color)] transition-colors duration-150 hover:bg-[var(--p-content-hover-background)] hover:text-[var(--p-text-color)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-[var(--p-primary-color)] focus-visible:outline-offset-1"
          aria-label="Notifications"
          @click="toggleNotifPopover"
--- a/apps/app/src/components-v2/layout/tests/AppTopbar.spec.ts
+++ b/apps/app/src/components-v2/layout/tests/AppTopbar.spec.ts
@@ -9,9 +9,14 @@
 *  2. Theme toggle click → shell.setTheme with flipped value (light→dark, dark→light)
 *  3. Density toggle click → shell.setDensity with flipped value
 *  4. User-menu Sign out command → authStore.logout called
+ *  5. Breadcrumb model mapping:
+ *     - non-last items carry a `command` that calls router.push with the item's `to`
+ *     - last item has NO `command` (non-interactive) and NO `route` key (FIX A regression)
 *
 * useBreadcrumb() calls useRoute() internally. We provide a minimal
 * vue-router mock via vi.mock so the composable has a route to call.
+ * useRoute is exposed as a vi.fn() so individual tests can override the
+ * matched records without re-importing the module.
 */

 import { createPinia, setActivePinia } from 'pinia'
@@ -22,13 +27,21 @@ import { useAuthStore } from '@/stores/useAuthStore'

 // ---------------------------------------------------------------------------
 // Mock vue-router (useBreadcrumb calls useRoute internally)
+// vi.hoisted() is required so these vi.fn() instances are initialised
+// before the vi.mock() factory runs (vi.mock is hoisted to top of file by
+// the Vitest transform, but const declarations are not).
 // ---------------------------------------------------------------------------

+const { mockRouterPush, mockUseRoute } = vi.hoisted(() => ({
+  mockRouterPush: vi.fn(),
+  mockUseRoute: vi.fn(() => ({ matched: [] as unknown[] })),
+}))
+
 vi.mock('vue-router', () => ({
-  useRoute: () => ({
-    matched: [],
+  useRoute: mockUseRoute,
+  useRouter: () => ({
+    push: mockRouterPush,
  }),
-  useRouter: () => ({}),
 }))

 // ---------------------------------------------------------------------------
@@ -86,6 +99,11 @@ function mountTopbar() {
 describe('AppTopbar', () => {
  beforeEach(() => {
    setActivePinia(createPinia())
+    mockRouterPush.mockReset()
+    mockUseRoute.mockReset()
+
+    // Default: empty matched array (no breadcrumb items)
+    mockUseRoute.mockReturnValue({ matched: [] })
  })

  // -------------------------------------------------------------------------
@@ -221,4 +239,56 @@ describe('AppTopbar', () => {

    expect(logoutSpy).toHaveBeenCalledOnce()
  })
+
+  // -------------------------------------------------------------------------
+  // FIX A regression: breadcrumb model mapping uses command+router.push,
+  // NOT the `route` key that this PrimeVue Breadcrumb version ignores.
+  //
+  // BreadcrumbItem.vue renders <a :href="item.url || '#'"> and calls
+  // item.command on click — it never reads `route`. This test would have
+  // caught the broken mapping that set `route` instead of `command`.
+  // -------------------------------------------------------------------------
+
+  it('breadcrumb model: non-last items have command that calls router.push; last item has no command and no route key', async () => {
+    // Provide two matched records so we get a non-last item (with `to`) and
+    // a last item (current, no `to`).
+    mockUseRoute.mockReturnValue({
+      matched: [
+        { meta: { breadcrumb: 'Dashboard' }, name: 'dashboard', path: '/dashboard' },
+        { meta: { breadcrumb: 'Events' }, name: 'events', path: '/events' },
+      ],
+    })
+
+    const wrapper = mountTopbar()
+
+    const breadcrumbStub = wrapper.findComponent({ name: 'Breadcrumb' })
+
+    expect(breadcrumbStub.exists()).toBe(true)
+
+    const model = breadcrumbStub.props('model') as Array<{
+      label: string
+      command?: () => void
+      route?: unknown
+    }>
+
+    // Should have two items produced by toBreadcrumbItems
+    expect(model).toHaveLength(2)
+
+    const [firstItem, lastItem] = model as [typeof model[0], typeof model[0]]
+
+    // Non-last item: must carry `command`, must NOT carry `route`
+    expect(firstItem.label).toBe('Dashboard')
+    expect(typeof firstItem.command).toBe('function')
+    expect('route' in firstItem).toBe(false)
+
+    // Invoking command must call router.push with the item's resolved `to` path
+    firstItem.command!()
+    expect(mockRouterPush).toHaveBeenCalledOnce()
+    expect(mockRouterPush).toHaveBeenCalledWith('/dashboard')
+
+    // Last/current item: must have NO command (non-interactive), NO `route` key
+    expect(lastItem.label).toBe('Events')
+    expect(lastItem.command).toBeUndefined()
+    expect('route' in lastItem).toBe(false)
+  })
 })
--- a/apps/app/src/composables/useBreadcrumb.ts
+++ b/apps/app/src/composables/useBreadcrumb.ts
@@ -55,7 +55,11 @@ export function toBreadcrumbItems(matched: readonly BreadcrumbRouteRecord[]): Br
    if (isLast)
      return { label }

-    // Prefer path; fall back to name when path is absent/empty
+    // Prefer path; fall back to name when path is absent/empty.
+    // NOTE: a non-last record whose `path` is a param template (e.g. `/events/:id`)
+    // would yield an unresolved-template `to` — acceptable in foundation scope because
+    // param routes are normally the last (current, no-`to`) segment.
+    // TODO TECH-WS-GUI-REDESIGN: resolve param paths if a non-leaf param route ever needs a crumb link
    const to: RouteLocationRaw
      = record.path
        ? record.path