fix(NavigationManager): only resolve navigations of booted apps#61120
Draft
susnux wants to merge 2 commits into
Draft
fix(NavigationManager): only resolve navigations of booted apps#61120susnux wants to merge 2 commits into
susnux wants to merge 2 commits into
Conversation
susnux
requested review from
leftybournes,
provokateurin and
salmart-dev
and removed request for
a team
come-nc
approved these changes
Jun 9, 2026
This comment was marked as resolved.
This comment was marked as resolved.
susnux
commented
Jun 9, 2026
Contributor
Author
|
We discovered more issues, will work on it. |
susnux
force-pushed
the
fix/navigation-manager
branch
3 times, most recently
from
0ccc152 to
b838ee0
Compare
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…tion Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
susnux
force-pushed
the
fix/navigation-manager
branch
from
281ccd1 to
e870c6f
Compare
come-nc
reviewed
Jun 23, 2026
|
|
||
| public function __construct( | ||
| private readonly IL10N $l10n, | ||
| private readonly IAppManager $appManger, |
Contributor
There was a problem hiding this comment.
Suggested change
| private readonly IAppManager $appManger, | |
| private readonly IAppManager $appManager, |
come-nc
reviewed
Jun 23, 2026
Comment on lines
+39
to
+41
| if (!$this->userSession->isLoggedIn()) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Not needed, there is already the null check on $user in the child function
| return; | ||
| } | ||
|
|
||
| if ($this->appManger->isAppLoaded(Application::APP_ID)) { |
Contributor
There was a problem hiding this comment.
Why is it needed to check that? Could you add a comment explaining we need the settings application to be loaded and why. (reading the code it’s not obvious this is the settings application)
Comment on lines
+45
to
+47
| if (!$this->userSession->isLoggedIn()) { | ||
| return; | ||
| } |
| public function __construct( | ||
| private readonly IConfig $config, | ||
| private readonly IL10N $l10n, | ||
| private readonly IAppManager $appManger, |
Contributor
There was a problem hiding this comment.
Suggested change
| private readonly IAppManager $appManger, | |
| private readonly IAppManager $appManager, |
What’s a manger 🙃
| return; | ||
| } | ||
|
|
||
| if ($this->appManger->isAppLoaded(Application::APP_ID)) { |
Contributor
There was a problem hiding this comment.
Is it possible this get called when settings is not loaded? This listener is registered by the settings application.
|
|
||
| public function __construct( | ||
| public readonly IFactory $l10nFactory, | ||
| private readonly IAppManager $appManger, |
Comment on lines
+42
to
+44
| if (!$this->userSession->isLoggedIn()) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
This one may be needed as it’s not checked in the method.
| */ | ||
| private function registerNavigationEntries(): void { | ||
| // Register the logout button in the user settings | ||
| $logoutUrl = \OC_User::getLogoutUrl($this->urlGenerator); |
Comment on lines
+217
to
+218
| // we do not really know the current bootstrapping state | ||
| // but we know that the files app is always enabled and loaded when "filesystem" is loaded thus the server is ready or close-to-ready. |
Contributor
There was a problem hiding this comment.
🥷 Not sure I like that. But not sure I have a better idea.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prevent log flooded with "route not found"
Checklist
3. to review, feature component)stable32)AI (if applicable)