From 1ec746ad2056e8723bcb167f5f68e438b8e97680 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 7 Sep 2026 18:10:13 +0200 Subject: [PATCH 1/3] fix(navigation): Lazy load as much of navigation as possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are still a few computed stuff but it should already be an improvement for code paths which are not using navigation. Signed-off-by: Côme Chilliet --- lib/private/NavigationManager.php | 125 ++++++++++++++++++---------- lib/public/INavigationManager.php | 2 +- tests/lib/NavigationManagerTest.php | 40 ++++----- 3 files changed, 104 insertions(+), 63 deletions(-) diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php index 32aac71e2657a..ca97f7bd6aa03 100644 --- a/lib/private/NavigationManager.php +++ b/lib/private/NavigationManager.php @@ -54,23 +54,31 @@ class NavigationManager implements INavigationManager { 'activity' => -88, ]; - protected ?string $activeEntry = null; - /** @var array */ - protected array $entries = []; + /** @var list */ + private array $newEntries = []; /** @var list */ - protected array $closureEntries = []; + private array $closureEntries = []; + + private ?string $defaultEntryId = null; + + private ?string $activeEntry = null; + /** @var array */ + private array $entries = []; /** User defined app order (cached for the `add` function) */ - protected ?array $customAppOrder = null; + private ?array $customAppOrder = null; /** @var array */ - protected array $unreadCounters = []; + private array $unreadCounters = []; /** true if the internal state has been initialized */ - protected bool $initAppOrderDone = false; + private bool $initAppOrderDone = false; /** true if all apps have been loaded by the App Manager */ - protected bool $initSetupDone = false; + private bool $initSetupDone = false; /** List of loaded app info */ private array $loadedAppInfo = []; + private ?bool $isAdmin = null; + private bool $eventFired = false; + public function __construct( protected IAppManager $appManager, private IURLGenerator $urlGenerator, @@ -89,12 +97,20 @@ public function add(array|callable $entry): void { $this->closureEntries[] = $entry; return; } + $this->newEntries[] = $entry; + } + + /** + * @param NavigationEntry $entry + */ + private function processEntry($entry): void { // if needed initialize the internal state to allow setting app order and default app $this->initCustomAppOrder(); $id = $entry['id']; $entry['active'] = false; + $entry['default'] = false; $entry['unread'] = $this->unreadCounters[$id] ?? 0; if (!isset($entry['icon'])) { $entry['icon'] = ''; @@ -120,18 +136,10 @@ public function add(array|callable $entry): void { } $this->entries[$id] = $entry; - - // Needs to be done after adding the new entry to account for the default entries containing this new entry. - $this->updateDefaultEntries(); } private function updateDefaultEntries(): void { - $defaultEntryId = $this->getDefaultEntryIdForUser($this->userSession->getUser(), false); - foreach ($this->entries as $id => $entry) { - if ($entry['type'] === 'link') { - $this->entries[$id]['default'] = $id === $defaultEntryId; - } - } + $this->defaultEntryId = $this->getDefaultEntryIdForUser($this->userSession->getUser(), false); } #[Override] @@ -155,23 +163,29 @@ public function getAll(string $type = 'link'): array { * @return array */ private function proceedNavigation(array $list, string $type): array { + $noDefault = true; + if ($this->defaultEntryId !== null && isset($list[$this->defaultEntryId])) { + $list[$this->defaultEntryId]['default'] = true; + $noDefault = false; + } + uasort($list, function ($a, $b) { - if (($a['default'] ?? false) xor ($b['default'] ?? false)) { + if ($a['default'] xor $b['default']) { // Always sort the default app first - return ($a['default'] ?? false) ? -1 : 1; + return $a['default'] ? -1 : 1; } elseif (isset($a['order']) && isset($b['order'])) { // Sort by order - return ($a['order'] < $b['order']) ? -1 : 1; + return $a['order'] <=> $b['order']; } elseif (isset($a['order']) || isset($b['order'])) { // Sort the one that has an order property first return isset($a['order']) ? -1 : 1; } else { // Sort by name otherwise - return ($a['name'] < $b['name']) ? -1 : 1; + return $a['name'] <=> $b['name']; } }); - if ($type === 'all' || $type === 'link') { + if ($noDefault && ($type === 'all' || $type === 'link')) { // There might be the case that no default app was set, in this case the first app is the default app. // Otherwise, the default app is already the ordered first, so setting the default prop will make no difference. foreach ($list as $index => &$navEntry) { @@ -184,15 +198,8 @@ private function proceedNavigation(array $list, string $type): array { } $activeEntry = $this->getActiveEntry(); - if ($activeEntry !== null) { - foreach ($list as $index => &$navEntry) { - if ($navEntry['id'] == $activeEntry) { - $navEntry['active'] = true; - } else { - $navEntry['active'] = false; - } - } - unset($navEntry); + if ($activeEntry !== null && isset($list[$activeEntry])) { + $list[$activeEntry]['active'] = true; } return $list; @@ -204,6 +211,9 @@ private function proceedNavigation(array $list, string $type): array { public function clear(bool $resetInit = true): void { $this->entries = []; $this->closureEntries = []; + $this->newEntries = []; + $this->defaultEntryId = null; + $this->activeEntry = null; if ($resetInit) { $this->loadedAppInfo = []; @@ -247,9 +257,6 @@ private function initCustomAppOrder(): void { * @internal - This is only used by Nextcloud core to setup the navigation manager. It is not intended for use by apps. */ public function setup(): void { - // Resolve dynamically added navigation entries via event listeners - $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); - // mark setup as done to allow performance optimizations $this->initSetupDone = true; } @@ -263,13 +270,15 @@ public function setup(): void { * So we need to resolve the navigation entries here, even if not all apps are loaded yet. */ private function resolveAppNavigationEntries(): void { - if ($this->userSession->isLoggedIn()) { - $user = $this->userSession->getUser(); + $user = $this->userSession->getUser(); + if ($user !== null) { $apps = $this->appManager->getEnabledAppsForUser($user); } else { $apps = $this->appManager->getEnabledApps(); } + $this->isAdmin ??= $this->isAdmin(); + foreach ($apps as $app) { if (in_array($app, $this->loadedAppInfo, true)) { // already loaded @@ -279,12 +288,12 @@ private function resolveAppNavigationEntries(): void { // app is not loaded yet, skip it continue; } + $this->loadedAppInfo[] = $app; // load plugins and collections from info.xml $info = $this->appManager->getAppInfo($app); if (!isset($info['navigations']['navigation'])) { // this app does not have any navigation entries, skip it - $this->loadedAppInfo[] = $app; continue; } @@ -298,7 +307,7 @@ private function resolveAppNavigationEntries(): void { continue; } $role = $nav['@attributes']['role'] ?? 'all'; - if ($role === 'admin' && !$this->isAdmin()) { + if ($role === 'admin' && !$this->isAdmin) { continue; } $id = $nav['id'] ?? $app . ($key === 0 ? '' : $key); @@ -329,12 +338,11 @@ private function resolveAppNavigationEntries(): void { } $l = $this->l10nFac->get($app); - $this->loadedAppInfo[] = $app; $this->add(array_merge([ // Navigation id 'id' => $id, // Order where this entry should be shown - 'order' => $order, + 'order' => (int)$order, // Target of the navigation entry 'href' => $route, // The icon used for the navigation entry @@ -351,8 +359,16 @@ private function resolveAppNavigationEntries(): void { } } + $updateDefaultEntries = false; + // once all apps are loaded we can resolve the app navigation closures if ($this->initSetupDone) { + if (!$this->eventFired) { + // Resolve dynamically added navigation entries via event listeners + $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); + $this->eventFired = true; + } + // This has to be done on every call, // as apps might add new navigation entries via closures at any time while ($c = array_pop($this->closureEntries)) { @@ -362,12 +378,26 @@ private function resolveAppNavigationEntries(): void { $this->logger->debug('Closure of navigation entry returned null, skipping'); continue; } - $this->add($entry); + $this->processEntry($entry); + $updateDefaultEntries = true; } catch (\Throwable $e) { $this->logger->error('Failed to add navigation entry from closure', ['exception' => $e]); } } } + + while ($entry = array_pop($this->newEntries)) { + try { + $this->processEntry($entry); + $updateDefaultEntries = true; + } catch (\Throwable $e) { + $this->logger->error('Failed to add navigation entry', ['exception' => $e, 'entry' => $entry]); + } + } + + if ($updateDefaultEntries) { + $this->updateDefaultEntries(); + } } private function isAdmin(): bool { @@ -386,12 +416,21 @@ public function setUnreadCounter(string $id, int $unreadCounter): void { #[Override] public function get(string $id): ?array { $this->resolveAppNavigationEntries(); - return $this->entries[$id] ?? null; + if (!isset($this->entries[$id])) { + return null; + } + $entry = $this->entries[$id]; + if ($this->defaultEntryId === $id) { + $entry['default'] = true; + } + if ($this->activeEntry === $id) { + $entry['active'] = true; + } + return $entry; } #[Override] public function getDefaultEntryIdForUser(?IUser $user = null, bool $withFallbacks = true): string { - $this->resolveAppNavigationEntries(); // Disable fallbacks here, as we need to override them with the user defaults if none are configured. $defaultEntryIds = $this->getDefaultEntryIds(false); diff --git a/lib/public/INavigationManager.php b/lib/public/INavigationManager.php index 49337c81f6e95..7bf7f17a1d2ec 100644 --- a/lib/public/INavigationManager.php +++ b/lib/public/INavigationManager.php @@ -38,7 +38,7 @@ * type: 'link'|'action'|'settings'|'guest'|'quota', * name: string, * app?: string, - * default?: bool, + * default: bool, * active: bool, * classes: string, * unread: int, diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php index 6eb3fa2001c25..6ca8faf1e0cb9 100644 --- a/tests/lib/NavigationManagerTest.php +++ b/tests/lib/NavigationManagerTest.php @@ -25,24 +25,16 @@ use Psr\Log\LoggerInterface; class NavigationManagerTest extends TestCase { - /** @var AppManager&MockObject */ - protected $appManager; - /** @var IURLGenerator&MockObject */ - protected $urlGenerator; - /** @var IFactory&MockObject */ - protected $l10nFac; - /** @var IUserSession&MockObject */ - protected $userSession; - /** @var IGroupManager&MockObject */ - protected $groupManager; - /** @var IConfig&MockObject */ - protected $config; - - protected IEventDispatcher&MockObject $dispatcher; - - /** @var NavigationManager */ - protected $navigationManager; - protected LoggerInterface $logger; + private AppManager&MockObject $appManager; + private IURLGenerator&MockObject $urlGenerator; + private IFactory&MockObject $l10nFac; + private IUserSession&MockObject $userSession; + private IGroupManager&MockObject $groupManager; + private IConfig&MockObject $config; + private IEventDispatcher&MockObject $dispatcher; + private LoggerInterface&MockObject $logger; + + private NavigationManager $navigationManager; #[\Override] protected function setUp(): void { @@ -92,7 +84,8 @@ public static function addArrayData(): array { 'active' => false, 'type' => 'settings', 'classes' => '', - 'unread' => 0 + 'unread' => 0, + 'default' => false, ] ], [ @@ -389,6 +382,7 @@ public static function providesNavigationConfig(): array { 'type' => 'settings', 'classes' => '', 'unread' => 0, + 'default' => false, ]], ['navigations' => [ 'navigation' => [ @@ -590,6 +584,7 @@ public function testDefaultAppOrderIsSkippedForCustomOrder(): void { $this->userSession->method('isLoggedIn')->willReturn(true); $this->appManager->method('getEnabledAppsForUser')->willReturn([]); $this->appManager->method('isEnabledForUser')->willReturn(true); + $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); $this->config->method('getUserValue') ->willReturnCallback(static function (string $userId, string $appName, string $key, mixed $default = '') { return $key === 'apporder' ? json_encode(['other' => ['app' => 'other', 'order' => 0]]) : $default; @@ -619,6 +614,7 @@ public function testResolveOnlyLoadedApps(): void { $this->userSession->method('getUser')->willReturn($user); $this->userSession->method('isLoggedIn')->willReturn(true); $this->appManager->method('getEnabledAppsForUser')->with($user)->willReturn(['test']); + $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); // The app is enabled but not booted yet ... $this->appManager->expects($this->atLeastOnce()) @@ -658,6 +654,7 @@ public function testAppInfoResolvedOnlyOnce(): void { $this->userSession->method('isLoggedIn')->willReturn(true); $this->appManager->method('getEnabledAppsForUser')->with($user)->willReturn(['test']); $this->appManager->method('isAppLoaded')->with('test')->willReturn(true); + $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); // App has no navigation entries; info.xml must only be read once $this->appManager->expects($this->once()) @@ -683,6 +680,7 @@ public function testClearResetsResolvedStateOnlyWhenRequested(): void { $this->userSession->method('isLoggedIn')->willReturn(true); $this->appManager->method('getEnabledAppsForUser')->with($user)->willReturn(['test']); $this->appManager->method('isAppLoaded')->with('test')->willReturn(true); + $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); // Resolved once for the initial getAll(), then again after clear(true) resets the state $this->appManager->expects($this->exactly(2)) @@ -874,6 +872,7 @@ public function testGetDefaultEntryIdForUser(string $defaultApps, string $userDe }); $this->appManager->method('getEnabledApps')->willReturn(['files']); + $this->appManager->method('getEnabledAppsForUser')->willReturn(['files']); $this->appManager->expects($this->atLeastOnce()) ->method('isAppLoaded') ->willReturnMap([ @@ -899,6 +898,7 @@ public function testGetDefaultEntryIdForUser(string $defaultApps, string $userDe ['user1', 'core', 'defaultapp', '', $userDefaultApps], ['user1', 'core', 'apporder', '[]', $userApporder], ]); + $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); $this->navigationManager->setup(); $this->assertEquals($expectedApp, $this->navigationManager->getDefaultEntryIdForUser(null, $withFallbacks)); @@ -906,6 +906,8 @@ public function testGetDefaultEntryIdForUser(string $defaultApps, string $userDe public function testDefaultEntryUpdated(): void { $this->appManager->method('getEnabledApps')->willReturn([]); + $this->appManager->method('getEnabledAppsForUser')->willReturn([]); + $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); From 6b8d1cc0e5c69c0ab6134c9ec963f8175806ad0d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 9 Sep 2026 12:56:25 +0200 Subject: [PATCH 2/3] fix: Make default nullable again Signed-off-by: Carl Schwan --- lib/private/NavigationManager.php | 7 ++++--- lib/public/INavigationManager.php | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php index ca97f7bd6aa03..2bdb8bebaf788 100644 --- a/lib/private/NavigationManager.php +++ b/lib/private/NavigationManager.php @@ -110,7 +110,6 @@ private function processEntry($entry): void { $id = $entry['id']; $entry['active'] = false; - $entry['default'] = false; $entry['unread'] = $this->unreadCounters[$id] ?? 0; if (!isset($entry['icon'])) { $entry['icon'] = ''; @@ -123,6 +122,8 @@ private function processEntry($entry): void { } if ($entry['type'] === 'link') { + $entry['default'] = false; + // app might not be set when using closures, in this case try to fallback to ID if (!isset($entry['app']) && $this->appManager->isEnabledForUser($id)) { $entry['app'] = $id; @@ -170,9 +171,9 @@ private function proceedNavigation(array $list, string $type): array { } uasort($list, function ($a, $b) { - if ($a['default'] xor $b['default']) { + if (($a['default'] ?? false) xor ($b['default'] ?? false)) { // Always sort the default app first - return $a['default'] ? -1 : 1; + return ($a['default'] ?? false) ? -1 : 1; } elseif (isset($a['order']) && isset($b['order'])) { // Sort by order return $a['order'] <=> $b['order']; diff --git a/lib/public/INavigationManager.php b/lib/public/INavigationManager.php index 7bf7f17a1d2ec..49337c81f6e95 100644 --- a/lib/public/INavigationManager.php +++ b/lib/public/INavigationManager.php @@ -38,7 +38,7 @@ * type: 'link'|'action'|'settings'|'guest'|'quota', * name: string, * app?: string, - * default: bool, + * default?: bool, * active: bool, * classes: string, * unread: int, From aa4e9ebd50d62b7bf74f5b602889fbd8f01b950b Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 9 Sep 2026 23:30:45 +0200 Subject: [PATCH 3/3] fix: adapt tests Co-authored-by: Carl Schwan Signed-off-by: Carl Schwan --- tests/lib/NavigationManagerTest.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php index 6ca8faf1e0cb9..fc84c7b51a7f8 100644 --- a/tests/lib/NavigationManagerTest.php +++ b/tests/lib/NavigationManagerTest.php @@ -85,7 +85,6 @@ public static function addArrayData(): array { 'type' => 'settings', 'classes' => '', 'unread' => 0, - 'default' => false, ] ], [ @@ -382,7 +381,6 @@ public static function providesNavigationConfig(): array { 'type' => 'settings', 'classes' => '', 'unread' => 0, - 'default' => false, ]], ['navigations' => [ 'navigation' => [