fix(NavigationManager): resolve entries only when needed

The `init` method previously contained two different logics:
1. It set up the internal state of default apps and app order
2. It resolved the app navigation entries

The 1. is needed before `add` can be called, so it was always called by
the `add` method, but this also resolved all appinfo.xml entries on the
first `add` call even if never used.
The 2. is only needed when the navigations are actually fetched.

This splits the logic into two functions:
- `init` for the bare initialization
- `resolveAppNavigationEntries` for resolving the entries when
  requesting to output them.

This should give a small performance improvement for API calls and fixes
a problem when navigations are added before all apps are registered.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Author: susnux

cypress/e2e/core/header_app-menu.cy.ts

@@ -12,20 +12,18 @@ const getAppMenu = () => getNextcloudHeader().find('.app-menu')
 // the next-best stable selectors.
 const getWaffleTrigger = () => getAppMenu().find('.app-menu__waffle')
 
-describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => {
-	beforeEach(() => {
-		clearState()
-	})
+before(clearState)
 
-	describe('Open and click', () => {
+describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => {
+	describe('Normal user', () => {
 		beforeEach(() => {
 			cy.createRandomUser().then(($user) => {
 				cy.login($user)
 				cy.visit('/')
 			})
 		})
 
-		it('opens the popover and navigates when a tile is clicked', () => {
+		it('Open and click opens the popover and navigates when a tile is clicked', () => {
 			getWaffleTrigger().click()
 			cy.get('.app-menu__popover').should('be.visible')
 			getWaffleTrigger().should('have.attr', 'aria-expanded', 'true')
@@ -39,9 +37,16 @@ describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => {
 					cy.location('pathname').should('include', '/apps/')
 				})
 		})
+
+		it('has all correct app navigation items', () => {
+			waffleMenuShouldContainApps([
+				{ name: 'Files', href: '/apps/files' },
+				{ name: 'Dashboard', href: '/apps/dashboard' },
+			])
+		})
 	})
 
-	describe('Admin gating: "More apps" tile', () => {
+	describe('Admin', () => {
 		const admin = new User('admin', 'admin')
 
 		beforeEach(() => {
@@ -54,5 +59,33 @@ describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => {
 			cy.get('.app-menu__popover').should('be.visible')
 			cy.findByRole('menuitem', { name: 'More apps' }).should('be.visible')
 		})
+
+		it('has all correct app navigation items', () => {
+			waffleMenuShouldContainApps([
+				{ name: 'Files', href: '/apps/files' },
+				{ name: 'Dashboard', href: '/apps/dashboard' },
+				{ name: 'Appstore', href: '/settings/apps' },
+			])
+		})
 	})
 })
+
+/**
+ * Check that the waffle menu contains the given apps, by name and href.
+ *
+ * @param apps - The apps that should be present in the waffle menu, with their expected name and href.
+ */
+function waffleMenuShouldContainApps(apps: { name: string, href: string }[]) {
+	getWaffleTrigger().click()
+	getWaffleTrigger().should('have.attr', 'aria-expanded', 'true')
+	cy.findByRole('menu', { name: 'Apps' }).should('be.visible')
+
+	cy.findAllByRole('menuitem')
+		.then((items) => {
+			apps.forEach((app) => {
+				const item = items.toArray().find((i) => i.textContent?.includes(app.name))
+				expect(item, `App menu should contain ${app.name}`).to.exist
+				expect(item?.getAttribute('href')).to.match(new RegExp(`${app.href}(\\?.+|/?$)`))
+			})
+		})
+}

lib/private/NavigationManager.php

@@ -37,6 +37,8 @@ class NavigationManager implements INavigationManager {
 	protected bool $init = false;
 	/** User defined app order (cached for the `add` function) */
 	private ?array $customAppOrder = null;
+	/** List of loaded app info */
+	private array $loadedAppInfo = [];
 
 	public function __construct(
 		protected IAppManager $appManager,
@@ -56,7 +58,7 @@ public function add(array|callable $entry): void {
 			$this->closureEntries[] = $entry;
 			return;
 		}
-		$this->init(false);
+		$this->init();
 
 		$id = $entry['id'];
 
@@ -99,7 +101,7 @@ private function updateDefaultEntries(): void {
 
 	#[Override]
 	public function getAll(string $type = 'link'): array {
-		$this->init();
+		$this->resolveAppNavigationEntries();
 
 		$result = $this->entries;
 		if ($type !== 'all') {
@@ -180,7 +182,16 @@ public function getActiveEntry(): ?string {
 		return $this->activeEntry;
 	}
 
-	private function init(bool $resolveClosures = true): void {
+	/**
+	 * Initialize the internal state.
+	 * This loads the default app mapping and user mapping for app ordering.
+	 */
+	private function init(): void {
+		if ($this->init) {
+			return;
+		}
+		$this->init = true;
+
 		if ($this->customAppOrder === null) {
 			if ($this->userSession->isLoggedIn()) {
 				$user = $this->userSession->getUser();
@@ -189,21 +200,23 @@ private function init(bool $resolveClosures = true): void {
 				$this->customAppOrder = [];
 			}
 		}
+	}
 
-		if ($resolveClosures) {
-			while ($c = array_pop($this->closureEntries)) {
-				$this->add($c());
-			}
+	/**
+	 * Resolve the app navigation entries from closures and info.xml files.
+	 */
+	private function resolveAppNavigationEntries(): void {
+		// Resolve app navigation closures
+		while ($c = array_pop($this->closureEntries)) {
+			$this->add($c());
 		}
 
-		if ($this->init) {
-			return;
+		// Resolve dynamically added navigation entries via event listeners
+		if ($this->loadedAppInfo === []) {
+			$this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent());
 		}
-		$this->init = true;
-
-		$l = $this->l10nFac->get('lib');
-		$this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent());
 
+		// Resolve classic info.xml based navigation entries
 		if ($this->userSession->isLoggedIn()) {
 			$user = $this->userSession->getUser();
 			$apps = $this->appManager->getEnabledAppsForUser($user);
@@ -212,6 +225,11 @@ private function init(bool $resolveClosures = true): void {
 		}
 
 		foreach ($apps as $app) {
+			// skip already loaded apps
+			if (in_array($app, $this->loadedAppInfo)) {
+				continue;
+			}
+
 			// load plugins and collections from info.xml
 			$info = $this->appManager->getAppInfo($app);
 			if (!isset($info['navigations']['navigation'])) {
@@ -230,7 +248,6 @@ private function init(bool $resolveClosures = true): void {
 				if ($role === 'admin' && !$this->isAdmin()) {
 					continue;
 				}
-				$l = $this->l10nFac->get($app);
 				$id = $nav['id'] ?? $app . ($key === 0 ? '' : $key);
 				$order = $nav['order'] ?? 100;
 				$type = $nav['type'];
@@ -249,7 +266,14 @@ private function init(bool $resolveClosures = true): void {
 				if ($icon === null) {
 					$icon = $this->urlGenerator->imagePath('core', 'places/default-app-icon.svg');
 				}
+				if ($type === 'link' && $route === '') {
+					// This means either the route is invalid in the info.xml or the app was not year loaded by the router
+					$this->logger->debug('Missing or invalid navigation route for app ' . $app, ['entry' => $nav]);
+					continue;
+				}
 
+				$l = $this->l10nFac->get($app);
+				$this->loadedAppInfo[] = $app;
 				$this->add(array_merge([
 					// Navigation id
 					'id' => $id,
@@ -287,13 +311,13 @@ public function setUnreadCounter(string $id, int $unreadCounter): void {
 
 	#[Override]
 	public function get(string $id): ?array {
-		$this->init();
+		$this->resolveAppNavigationEntries();
 		return $this->entries[$id];
 	}
 
 	#[Override]
 	public function getDefaultEntryIdForUser(?IUser $user = null, bool $withFallbacks = true): string {
-		$this->init();
+		$this->resolveAppNavigationEntries();
 		// Disable fallbacks here, as we need to override them with the user defaults if none are configured.
 		$defaultEntryIds = $this->getDefaultEntryIds(false);
 
@@ -335,7 +359,7 @@ public function getDefaultEntryIdForUser(?IUser $user = null, bool $withFallback
 
 	#[Override]
 	public function getDefaultEntryIds(bool $withFallbacks = true): array {
-		$this->init();
+		$this->resolveAppNavigationEntries();
 		$storedIds = explode(',', $this->config->getSystemValueString('defaultapp', $withFallbacks ? 'dashboard,files' : ''));
 		$ids = [];
 		$entryIds = array_keys($this->entries);
@@ -349,7 +373,7 @@ public function getDefaultEntryIds(bool $withFallbacks = true): array {
 
 	#[Override]
 	public function setDefaultEntryIds(array $ids): void {
-		$this->init();
+		$this->resolveAppNavigationEntries();
 		$entryIds = array_keys($this->entries);
 
 		foreach ($ids as $id) {