fix: duplicated elements on public link page

owncloud / web

:dragon_face: Next generation frontend for ownCloud Infinite Scale

https://owncloud.dev/clients/web/

GNU Affero General Public License v3.0

421 stars 157 forks source link

fix: duplicated elements on public link page #11137

Closed JammingBen closed 2 weeks ago

JammingBen commented 2 weeks ago

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

Fixes https://github.com/owncloud/web/issues/10371

Types of changes

[x] 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)

kulmann commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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? 🙈

sonarcloud[bot] commented 2 weeks ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud