Skip to content

perf: Load entries lazily again in NavigationManager #64109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
come-nc wants to merge 1 commit into
base: master
Choose a base branch
from
+104 −63
Open

perf: Load entries lazily again in NavigationManager #64109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 82 additions & 43 deletions lib/private/NavigationManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,23 +54,31 @@ class NavigationManager implements INavigationManager {
'activity' => -88,
];

protected ?string $activeEntry = null;
/** @var array<string, NavigationEntryOutput> */
protected array $entries = [];
/** @var list<NavigationEntry> */
private array $newEntries = [];
/** @var list<callable(): ?NavigationEntry> */
protected array $closureEntries = [];
private array $closureEntries = [];

private ?string $defaultEntryId = null;

private ?string $activeEntry = null;
/** @var array<string, NavigationEntryOutput> */
private array $entries = [];
/** User defined app order (cached for the `add` function) */
protected ?array $customAppOrder = null;
private ?array $customAppOrder = null;
/** @var array<string, int> */
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,
Expand All @@ -89,12 +97,20 @@ public function add(array|callable $entry): void {
$this->closureEntries[] = $entry;
return;
}
$this->newEntries[] = $entry;
}

/**
* @param NavigationEntry $entry
*/
private function addEntry($entry): void {

@susnux susnux Sep 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think add vs addEntry is confusing its rather something like process here 👀

// 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'] = '';
Expand All @@ -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]
Expand All @@ -155,23 +163,29 @@ public function getAll(string $type = 'link'): array {
* @return array<string, NavigationEntryOutput>
*/
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) {
Expand All @@ -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;
Expand All @@ -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 = [];
Expand Down Expand Up @@ -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;
}
Expand All @@ -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
Expand All @@ -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;
}

Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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)) {
Expand All @@ -362,12 +378,26 @@ private function resolveAppNavigationEntries(): void {
$this->logger->debug('Closure of navigation entry returned null, skipping');
continue;
}
$this->add($entry);
$this->addEntry($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->addEntry($entry);
$updateDefaultEntries = true;
} catch (\Throwable $e) {
$this->logger->error('Failed to add navigation entry from closure', ['exception' => $e]);

@susnux susnux Sep 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy Pasta

Suggested change
$this->logger->error('Failed to add navigation entry from closure', ['exception' => $e]);
$this->logger->error('Failed to add navigation entry', ['exception' => $e, 'entry' => $entry]);

}
}

if ($updateDefaultEntries) {
$this->updateDefaultEntries();
}
}

private function isAdmin(): bool {
Expand All @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion lib/public/INavigationManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
* type: 'link'|'action'|'settings'|'guest'|'quota',
* name: string,
* app?: string,
* default?: bool,
* default: bool,

@susnux susnux Sep 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this makes sense to add this to all output types (e.g. action)

* active: bool,
* classes: string,
* unread: int,
Expand Down
40 changes: 21 additions & 19 deletions tests/lib/NavigationManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -92,7 +84,8 @@ public static function addArrayData(): array {
'active' => false,
'type' => 'settings',
'classes' => '',
'unread' => 0
'unread' => 0,
'default' => false,
]
],
[
Expand Down Expand Up @@ -389,6 +382,7 @@ public static function providesNavigationConfig(): array {
'type' => 'settings',
'classes' => '',
'unread' => 0,
'default' => false,
]],
['navigations' => [
'navigation' => [
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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())
Expand All @@ -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))
Expand Down Expand Up @@ -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([
Expand All @@ -899,13 +898,16 @@ 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));
}

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');
Expand Down
Loading