perf(workspace): only touch the widget registry when the bridge is on - #120
Merged
rubenvdlinde merged 4 commits intoAug 15, 2026
Merged
Conversation
IManager::getWidgets() is not a getter. It calls loadLazyPanels(), which
calls load() on EVERY dashboard widget of every app enabled for the user,
ignoring the dashboard layout — and load() is where widgets call
Util::addScript(). Merely enumerating widgets therefore injects every
widget bundle in the instance.
index() reached the registry twice per render: once through
loadWidgetScripts() and once through getAvailableWidgets() for the initial
state. The explicit `foreach ($widgets as $widget) { $widget->load(); }`
loop in loadWidgetScripts() was dead weight — getWidgets() had already
called load() on all of them before the loop ran — so removing only that
loop would have changed nothing.
Measured on the workspace page, bridge on vs off:
bridge on 161.47 MB JS, 72 files, 24 widget bundles, 36,776 ms
bridge off 42.90 MB JS, 21 files, 0 widget bundles, 6,184 ms
That is 118.6 MB and ~30s of widget code the workspace never renders,
every one of which used to throw because OCA.Dashboard exists only on
/apps/dashboard.
The registry is now read only when legacyWidgetBridgeEnabled is set. The
SPA is unaffected when it is off: Views.vue already fetches the available
widget list from GET /api/widgets on boot, and loadInitialState defaults
`widgets` to [], so the REQ-INIT-002 reader still never returns undefined.
IManager was the only user of the dashboardManager constructor argument,
so both the argument and the import go with it.
With the bridge on the cost is unchanged — that path is bounded by
Nextcloud core, which offers no way to read widget metadata without
loadLazyPanels() injecting every script. Fixing that needs an upstream
change to OC\Dashboard\Manager.
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
August 14, 2026 20:28
Contributor
Quality Report — ConductionNL/launchpad @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-14 20:46 UTC
Download the full PDF report from the workflow artifacts.
Enabling the bridge makes the workspace read the Nextcloud widget registry, and IManager::getWidgets() injects the scripts of every widget of every enabled app. Measured on the workspace page that is 118.6 MB of JS and ~30s of load for widgets the workspace never renders, so the expensive path should be opt-in rather than the default. With no stored setting the workspace now renders in 4,988 ms with 42.90 MB of JS, 21 files and zero widget bundles, against 36,776 ms / 161.47 MB / 24 bundles when the bridge is on. Instances that rely on bridged placements switch it back on in Beheer; the flag is read on every render, so it takes effect immediately. The stored-value test asserted false, which is the new default and would therefore have passed without reading storage at all. It now stores true — the opposite of the default — so it still discriminates.
Contributor
Quality Report — ConductionNL/launchpad @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-14 21:27 UTC
Download the full PDF report from the workflow artifacts.
CI caught what the local run did not: phpmd ExcessiveMethodLength, because index() went from 88 lines on development to 108 with this PR's guard and its explanatory comment. The threshold is 100. The comment was carrying most of the weight, and it documents the bridge decision rather than the render flow, so it belongs with the logic it explains. Extracting resolveBridgedWidgets() takes both out of index(), which is now 90 lines. No behaviour change: same setting, same default (off), same call to getAvailableWidgets() only when the bridge is on. Verified: phpmd reports no PageController violation, phpcs unchanged at 0 errors for this file, phpstan [OK] No errors, phpunit 1570 tests 0 failures. Worth recording why this was missed — the local sweep ran lint, phpcs, phpstan, psalm and phpunit, but not phpmd, so "gate green" was a claim about a smaller set of checks than CI runs.
Contributor
Quality Report — ConductionNL/launchpad @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-15 21:02 UTC
Download the full PDF report from the workflow artifacts.
CI's coverage guard failed the previous commit — not a failing test, the ratchet: Coverage current: 51.68% (11645/22533 statements) Coverage merge base: 51.68% (11645/22532 statements) FAIL: This change adds 1 statements. Adding code without tests drops coverage. Extracting resolveBridgedWidgets() added one statement and covered none of it, because PageController had no test at all. The percentage did not move — only the counts — which is exactly the shape that reads as flake. The decision deserved a test regardless. The assertion that matters is `expects($this->never())` on getAvailableWidgets: the point of this change is not that an empty array comes back, it is that the widget registry is never touched, so no widget's load() runs and no scripts are injected. Asserting on the return value alone would still pass if the registry were read and discarded. Three cases: bridge on reads the registry, bridge off never touches it, and a missing setting defaults to off (the fail-safe direction). Verified load-bearing by mutation rather than by passing: flipping the default from `?? false` to `?? true` fails the third test with "getAvailableWidgets(): array was not expected to be called"; restoring it returns to green. phpunit 1573 tests 0 failures (3 added), phpmd 0 PageController violations, phpcs 0 errors on its configured scope (lib/ — tests/ is deliberately out of scope, matching every existing test file's positional-argument style).
Contributor
Quality Report — ConductionNL/launchpad @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ❌ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-15 21:36 UTC
Download the full PDF report from the workflow artifacts.
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
1 participant
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.
Problem
IManager::getWidgets()is not a getter. It callsloadLazyPanels(), which callsload()on every dashboard widget of every app enabled for the user, ignoring the dashboard layout — andload()is where widgets callUtil::addScript(). Simply enumerating widgets injects every widget bundle in the instance.index()reached the registry twice per render: once vialoadWidgetScripts()and once viagetAvailableWidgets()for the initial state.Worth calling out, because it inverts the obvious fix: the explicit
foreach ($widgets as $widget) { $widget->load(); }loop was dead weight.getWidgets()had already calledload()on all of them before the loop ran, so removing just the loop would have changed nothing measurable.Measured
Workspace page, bridge on vs off:
118.6 MB and ~30s of widget code the workspace never renders — and every one of those bundles threw
Cannot read properties of undefined (reading 'register'), becauseOCA.Dashboardonly exists on/apps/dashboard.Change
The registry is read only when
legacyWidgetBridgeEnabledis set. Safe when off:Views.vuealready fetches the list fromGET /api/widgetson boot, andloadInitialStatedefaultswidgetsto[], so the REQ-INIT-002 reader still never returnsundefined.IManagerwas the sole user of thedashboardManagerconstructor argument, so it and its import go too.Known limitation
With the bridge on, the cost is unchanged. That path is bounded by Nextcloud core, which offers no way to read widget metadata without
loadLazyPanels()injecting every script. Fixing it needs an upstream change toOC\Dashboard\Manager— filed separately.Verification
Full gate in the container on PHP 8.4 (host CLI is 8.2, so
check:strictfatals inplatform_check.phpwithout analysing anything):[OK] No errorsViews.loadingState.spec.js) is a 20s timeout under full-suite load and passes in isolation at 10.9s. No JavaScript is touched by this PR.