Skip to content
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.

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

fix: duplicated elements on public link page #11137

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

JammingBen
Copy link
Collaborator

Description

Fixes a bug where clicking the ownCloud logo on a public link page would lead to certain UI elements being duplicated.

This happens because some extensions and extension points are being registered more than once because the application layout gets unmounted on mounted again when clicking the logo. The solution is to properly unregister those extensions on unmount.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests
  • Documentation
  • Maintenance (e.g. dependency updates or tooling)

Fixes a bug where clicking the ownCloud logo on a public link page would lead to certain UI elements being duplicated.

This happens because some extensions and extension points are being registered more than once because the application layout gets unmounted on mounted again when clicking the logo. The solution is to properly unregister those extensions on unmount.
@JammingBen JammingBen self-assigned this Jul 4, 2024
@kulmann
Copy link
Member

kulmann commented Jul 4, 2024

An alternative approach would be to not register the extensionPoint and extension in the layout but somewhere earlier in the process... I'd prefer that actually, but back then when I wrote the extensionPoint and extension for the progress bar I couldn't find a good place to do that. Do you have an idea?

@JammingBen
Copy link
Collaborator Author

JammingBen commented Jul 4, 2024

An alternative approach would be to not register the extensionPoint and extension in the layout but somewhere earlier in the process... I'd prefer that actually, but back then when I wrote the extensionPoint and extension for the progress bar I couldn't find a good place to do that. Do you have an idea?

Maybe the main component wrapping the layouts, so in App.vue?

Edit: Also not super optimal when I think about it since other layouts don't need the progress bar. But it's the only option I can think of that doesn't get re-mounted on logo click.

@kulmann
Copy link
Member

kulmann commented Jul 4, 2024

An alternative approach would be to not register the extensionPoint and extension in the layout but somewhere earlier in the process... I'd prefer that actually, but back then when I wrote the extensionPoint and extension for the progress bar I couldn't find a good place to do that. Do you have an idea?

Maybe the main component wrapping the layouts, so in App.vue?

Edit: Also not super optimal when I think about it since other layouts don't need the progress bar. But it's the only option I can think of that doesn't get re-mounted on logo click.

Doesn't even need to be in a component. All the apps provide their extensions and extensionPoints to the web runtime for being registered during app initialization. So why not register extensions and extensionPoints from within the runtime directly before or after app initialization during bootstrap? 🤔

@JammingBen
Copy link
Collaborator Author

Doesn't even need to be in a component. All the apps provide their extensions and extensionPoints to the web runtime for being registered during app initialization. So why not register extensions and extensionPoints from within the runtime directly before or after app initialization during bootstrap? 🤔

Sounds like a good idea. However, it makes the solution for this issue more complicated 😬 That's because the watcher that initializes all apps gets also called each time the logo is being clicked. So we would again re-register existing extensions. I guess it would be best to fix that... although I don't know how (yet). The issue is that clicking the logo toggles publicLinkContextReady off and on, leading to the watcher re-firing.

@kulmann
Copy link
Member

kulmann commented Jul 5, 2024

Doesn't even need to be in a component. All the apps provide their extensions and extensionPoints to the web runtime for being registered during app initialization. So why not register extensions and extensionPoints from within the runtime directly before or after app initialization during bootstrap? 🤔

Sounds like a good idea. However, it makes the solution for this issue more complicated 😬 That's because the watcher that initializes all apps gets also called each time the logo is being clicked. So we would again re-register existing extensions. I guess it would be best to fix that... although I don't know how (yet). The issue is that clicking the logo toggles publicLinkContextReady off and on, leading to the watcher re-firing.

Oooof. I've just recently added the initialization of the dynamic app provider based apps into this exact watcher. That needs fixing. 😢 I've approved this PR for the sake of having an early bug fix, but could you look into the other issue please? 🙈

Prevents all apps from re-initializing when clicking the ownCloud logo on public link pages.

The issue happens because clicking the logo navigates the user to the `resolvePublicLink` page, which results in the public context being reset. We can omit this reset in that case though because we can guarantee that the user will either land on a public page, or on some other route which then resets the context anyways.
Copy link

sonarcloud bot commented Jul 5, 2024

@JammingBen JammingBen requested a review from kulmann July 5, 2024 07:17
@JammingBen JammingBen merged commit 24bc402 into master Jul 5, 2024
3 checks passed
@micbar micbar mentioned this pull request Jul 8, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking the oC logo on public links causes weird issues
2 participants