Skip to content

feat: add navigation actions for office - #130

Draft
susnux wants to merge 4 commits into
mainfrom
feat/frontend-router
Draft

feat: add navigation actions for office#130
susnux wants to merge 4 commits into
mainfrom
feat/frontend-router

Conversation

@susnux

@susnux susnux commented Aug 24, 2026

Copy link
Copy Markdown
Contributor
  1. Added frontend routes support so that we can generates routes for different office documents
  2. Register navigation actions based on registered creators
Bildschirmaufnahme_20260824_175411.webm

TODO

In richdocuments the creators need to get proper order, use setOrder properly.

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

- Update psalm to v6
- Fix invalid path in psalm config

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added 3. to review Waiting for reviews enhancement New feature request labels Aug 24, 2026
@susnux
susnux requested a review from moodyjmz August 24, 2026 15:45
@susnux
susnux force-pushed the feat/frontend-router branch from e9a4fa2 to 7602e7d Compare August 24, 2026 16:01
susnux added 3 commits August 24, 2026 18:21
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@moodyjmz

moodyjmz commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

TL;DR: Solid feature — real routing, good test discipline, no security issues, translations work correctly. One real accessibility bug slipped through because the existing tests assert on props passed to stubbed components rather than rendered output. Proposed fixes below.

Findings and proposed fixes

1. Sidebar doesn't dedupe creators sharing a category (medium)

AppMenuActionListener.php explicitly dedupes by category id before registering app-menu entries:

$seen = [];
foreach ($this->categoryService->listCreators() as $creator) {
    $id = $this->categoryService->categoryId($creator);
    if (isset($seen[$id])) {
        continue;
    }
    $seen[$id] = true;
    ...
}

OfficeOverview.vue's sidebar (v-for="creator in creators") has no equivalent, and CreatorCategoryService::listCategories() emits one row per creator with colliding ids by design (two suites both offering "Documents" get the same id). With two office suites installed, the sidebar shows two identically-labeled entries pointing at the same route — and because NcAppNavigationItem computes aria-current as props.active || (props.to && isActive), both end up aria-current="page" simultaneously, since isActive comes from the shared route target. A screen reader announces two indistinguishable "Documents, current page" links, and clicking the second silently does nothing (routes to the first).

Proposed fix — mirror the backend's dedupe on the frontend:

// One nav entry per category — two suites can register a creator for the
// same category, and only the first is addressable at its URL.
const navigationCreators = computed(() => {
  const seen = new Set<string>()
  return creators.value.filter(creator => {
    const id = categoryId(creator)
    if (seen.has(id)) return false
    seen.add(id)
    return true
  })
})

and use navigationCreators instead of creators in the v-for.

2. That exact scenario is untested (medium)

The new OfficeOverview.spec.ts tests assert item.props('to') / item.props('active') on the stubbed component — never the rendered <a>/aria-current. Finding 1 above is invisible to CI as a result. Suggest a regression test once the fix above lands, e.g.:

it('renders one nav entry per category, even when two creators share one', async () => {
  const other = makeCreator({ app: 'otheroffice', extension: '.odt', label: 'OtherOffice Doc' })
  getTemplatesMock.mockResolvedValue([documents, other])
  const wrapper = await mountWithBothCategories('/documents') // or equivalent setup

  const items = wrapper.findAllComponents({ name: 'NcAppNavigationItem' })
  expect(items).toHaveLength(1)
})

(Adjust to however the suite's existing fixtures/mocks are wired — the point is asserting on count/rendered state, not just the props passed in.)

3. router.replace() is un-awaited and uncaught (low)

The route-correction watcher in OfficeOverview.vue fires router.replace() without handling the returned promise. An overlapping navigation (e.g. rapid clicks between categories) rejects with NavigationCancelled/NavigationDuplicated, surfacing as an unhandled rejection. Suggest:

router.replace(creatorRoute(creator)).catch(() => {})

or checking isNavigationFailure() if you want to distinguish real failures from expected cancellations.

4. AppMenuActionListener now runs on every authenticated page render (low, FYI)

LoadAdditionalEntriesEvent fires instance-wide, not just on Files-app pages, so this listener's ITemplateManager::listCreators() call plus ~1.6–1.7KB of base64'd icon data now lands in every logged-in page's initial state, not just office pages. That's the existing platform event shape, not something this PR invented — flagging for awareness rather than asking for a change here.

Checked and clean

  • NcAppNavigationItem's :to prop is a genuine accessibility improvement (real anchor semantics, keyboard/middle-click/new-tab all work).
  • Category color is decorative only — every entry always has a name and icon too.
  • The SVG-as-data-URI icon is safe (rendered as a CSS background-image, which cannot execute embedded script).
  • The catch-all frontend route is inert — Nextcloud's dispatcher only binds parameters the controller method declares, so the unused {path} placeholder is simply ignored.
  • Translations: the four category labels moved from a frontend t('office', ...) call to $this->l10n->t(...) server-side — same source strings, same shared per-app catalog, already translated in every language that had them before. IL10N injected bare resolves to the current request's language via IFactory::get($appName), so labels are still per-user.
  • vue-router does add genuine new bundle weight (~12–14KB gz) — it's a fair cost for the feature, not something to avoid, just noting it's new rather than free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants