diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php index 151a69cce..8e8668df5 100644 --- a/lib/Controller/PageController.php +++ b/lib/Controller/PageController.php @@ -42,7 +42,6 @@ use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; -use OCP\Dashboard\IManager; use OCP\IRequest; use OCP\IUserSession; use OCP\Util; @@ -62,7 +61,6 @@ class PageController extends Controller { * Constructor. * * @param IRequest $request The request. - * @param IManager $dashboardManager Nextcloud dashboard widget manager. * @param IInitialState $initialState The Nextcloud initial-state service. * @param IUserSession $userSession Active user session. * @param WidgetService $widgetService Available-widgets descriptor formatter. @@ -95,7 +93,6 @@ class PageController extends Controller { */ public function __construct( IRequest $request, - private readonly IManager $dashboardManager, private readonly IInitialState $initialState, private readonly IUserSession $userSession, private readonly WidgetService $widgetService, @@ -160,8 +157,10 @@ public function index(string $deepLink = ''): TemplateResponse { Util::addScript(application: Application::APP_ID, file: 'launchpad-main'); Util::addStyle(application: Application::APP_ID, file: 'launchpad'); - // Load all widget scripts so legacy widgets can register their callbacks. - $this->loadWidgetScripts(); + // Settings are read once and reused below; getSettings() resolves every + // safe default, so neither read throws on an unset key. + $settings = $this->adminSettingsService->getSettings(); + $bridgedWidgets = $this->resolveBridgedWidgets(settings: $settings); $userId = $this->resolveUserId(); @@ -193,7 +192,7 @@ public function index(string $deepLink = ''): TemplateResponse { ); $builder - ->setWidgets($this->widgetService->getAvailableWidgets()) + ->setWidgets($bridgedWidgets) ->setLayout($activeState['layout']) ->setPrimaryGroup($primaryGroupId) ->setPrimaryGroupName($primaryGroupName) @@ -217,7 +216,7 @@ public function index(string $deepLink = ''): TemplateResponse { // no-match fallback target. `getSettings()` already resolves the // safe 'none' default when unset/invalid, so this never throws. $quicksearchFallback = (string)( - $this->adminSettingsService->getSettings()['quicksearchFallbackTarget'] ?? AdminSettingsService::DEFAULT_QUICKSEARCH_FALLBACK_TARGET + $settings['quicksearchFallbackTarget'] ?? AdminSettingsService::DEFAULT_QUICKSEARCH_FALLBACK_TARGET ); $builder @@ -245,6 +244,38 @@ public function index(string $deepLink = ''): TemplateResponse { return $response; }//end index() + /** + * Resolve the widgets to bridge into the workspace, if any. + * + * Legacy-widget-bridge spec: touching the Nextcloud widget registry at all is + * what costs us. `IManager::getWidgets()` is not a getter — it calls + * loadLazyPanels(), which calls load() on EVERY widget of every app enabled + * for the user, ignoring the dashboard layout, and load() is where widgets + * call `Util::addScript()`. Measured on this instance that is ~147 MB of + * widget JS injected into a page rendering no Nextcloud dashboard, every one + * of which then throws because `OCA.Dashboard` exists only on + * `/apps/dashboard`. + * + * So the registry is touched only when the bridge is actually on. When it is + * off the workspace renders without paying for widgets it will never bridge, + * and the SPA still has everything it needs: Views.vue fetches the + * available-widget list from `GET /api/widgets` on boot. + * + * @param array $settings Resolved admin settings (see AdminSettingsService). + * + * @return array The widget descriptors to bridge, empty when the bridge is off. + * + * @spec openspec/specs/runtime-shell/spec.md + */ + private function resolveBridgedWidgets(array $settings): array { + $bridgeEnabled = (bool)($settings['legacyWidgetBridgeEnabled'] ?? false); + if ($bridgeEnabled === false) { + return []; + } + + return $this->widgetService->getAvailableWidgets(); + }//end resolveBridgedWidgets() + /** * Resolve the primary group this request routes through. * @@ -550,22 +581,4 @@ public function publicShare(): TemplateResponse { return $response; }//end publicShare() - /** - * Load scripts for all available dashboard widgets. - * - * This ensures legacy widgets can register their callbacks via - * OCA.Dashboard.register. - * - * @return array Map of widget id to widget. - */ - private function loadWidgetScripts(): array { - $widgets = $this->dashboardManager->getWidgets(); - - foreach ($widgets as $widget) { - // Call the widget's load() method to inject its scripts. - $widget->load(); - } - - return $widgets; - }//end loadWidgetScripts() }//end class diff --git a/lib/Service/AdminSettingsService.php b/lib/Service/AdminSettingsService.php index 1dda62125..6188a6705 100644 --- a/lib/Service/AdminSettingsService.php +++ b/lib/Service/AdminSettingsService.php @@ -130,9 +130,15 @@ public function getSettings(): array { // dashboard default; forced-share groups default to none. 'defaultSharePermissionLevel' => $settings[$sharePermKey] ?? $permDef, 'forcedShareGroups' => array_values($forcedShareGroups), - // Legacy-widget-bridge spec: bridge defaults to ON so existing - // bridged placements keep rendering after upgrade. - 'legacyWidgetBridgeEnabled' => $settings[$bridgeKey] ?? true, + // Legacy-widget-bridge spec: the bridge defaults to OFF because + // turning it on makes the workspace read the Nextcloud widget + // registry, and IManager::getWidgets() injects the scripts of EVERY + // widget of every enabled app — measured at 118.6 MB of JS and ~30s + // of load on this fleet, for widgets the workspace never renders. + // Instances that rely on bridged placements switch it back on in + // Beheer; the setting is read on every render, so it takes effect + // immediately. + 'legacyWidgetBridgeEnabled' => $settings[$bridgeKey] ?? false, // Dashboard-quota-limits REQ-QUOTA-001: numeric governance // quotas. `0` = unlimited (no enforcement). 'maxDashboardsPerUser' => $this->clampQuota(value: $settings[$maxDashKey] ?? 0), diff --git a/tests/Unit/Controller/PageControllerBridgedWidgetsTest.php b/tests/Unit/Controller/PageControllerBridgedWidgetsTest.php new file mode 100644 index 000000000..129bb9211 --- /dev/null +++ b/tests/Unit/Controller/PageControllerBridgedWidgetsTest.php @@ -0,0 +1,152 @@ + + * @copyright 2024 Conduction b.v. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT:auto + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\LaunchPad\Tests\Unit\Controller; + +use OCA\LaunchPad\Controller\PageController; +use OCA\LaunchPad\Service\AdminSettingsService; +use OCA\LaunchPad\Service\AdminTemplateService; +use OCA\LaunchPad\Service\DashboardService; +use OCA\LaunchPad\Service\DashboardTreeService; +use OCA\LaunchPad\Service\RoleFeaturePermissionService; +use OCA\LaunchPad\Service\WidgetService; +use OCP\AppFramework\Services\IInitialState; +use OCP\IRequest; +use OCP\IUserSession; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use ReflectionMethod; + +/** + * The workspace renders no Nextcloud dashboard, but reading the widget registry + * to bridge legacy widgets is expensive out of proportion to that: `getWidgets()` + * is not a getter — it calls `loadLazyPanels()`, which calls `load()` on EVERY + * widget of every enabled app, and `load()` is where widgets call + * `Util::addScript()`. Measured at ~147 MB of widget JS on a page that renders + * no dashboard. + * + * So the decision "do we touch the registry at all" is load-bearing, and these + * tests pin both directions of it. + */ +class PageControllerBridgedWidgetsTest extends TestCase { + /** + * Build a controller whose only meaningful collaborator is the widget + * service — every other dependency is an unused mock. + * + * @param WidgetService $widgetService The widget service double. + * + * @return PageController The controller under test. + */ + private function makeController(WidgetService $widgetService): PageController { + return new PageController( + request: $this->createMock(IRequest::class), + initialState: $this->createMock(IInitialState::class), + userSession: $this->createMock(IUserSession::class), + widgetService: $widgetService, + dashboardService: $this->createMock(DashboardService::class), + adminTemplateService: $this->createMock(AdminTemplateService::class), + roleFeaturePerm: $this->createMock(RoleFeaturePermissionService::class), + treeService: $this->createMock(DashboardTreeService::class), + logger: $this->createMock(LoggerInterface::class), + adminSettingsService: $this->createMock(AdminSettingsService::class), + ); + }//end makeController() + + /** + * Invoke the private resolver. + * + * @param PageController $controller The controller under test. + * @param array $settings The resolved admin settings. + * + * @return array The bridged widget descriptors. + */ + private function resolve(PageController $controller, array $settings): array { + $method = new ReflectionMethod( + PageController::class, + 'resolveBridgedWidgets' + ); + $method->setAccessible(accessible: true); + + return $method->invoke($controller, $settings); + }//end resolve() + + /** + * Bridge ON: the registry IS read and its descriptors are returned. + * + * @return void + */ + public function testBridgeEnabledReadsTheWidgetRegistry(): void { + $widgetService = $this->createMock(WidgetService::class); + $widgetService->expects($this->once()) + ->method('getAvailableWidgets') + ->willReturn([['id' => 'deals'], ['id' => 'leads']]); + + $result = $this->resolve( + controller: $this->makeController(widgetService: $widgetService), + settings: ['legacyWidgetBridgeEnabled' => true] + ); + + $this->assertCount(2, $result); + $this->assertSame('deals', $result[0]['id']); + }//end testBridgeEnabledReadsTheWidgetRegistry() + + /** + * Bridge OFF: the registry is NOT touched at all. + * + * `never()` is the assertion that matters — the point of this change is not + * that the returned array is empty, it is that `getAvailableWidgets()` (and + * through it `IManager::getWidgets()`) is never called, so no widget's + * `load()` runs and no scripts are injected. An assertion on the return + * value alone would still pass if the registry were read and discarded. + * + * @return void + */ + public function testBridgeDisabledNeverTouchesTheRegistry(): void { + $widgetService = $this->createMock(WidgetService::class); + $widgetService->expects($this->never()) + ->method('getAvailableWidgets'); + + $result = $this->resolve( + controller: $this->makeController(widgetService: $widgetService), + settings: ['legacyWidgetBridgeEnabled' => false] + ); + + $this->assertSame([], $result); + }//end testBridgeDisabledNeverTouchesTheRegistry() + + /** + * An absent key defaults to OFF, matching AdminSettingsService's default. + * + * This pins the fail-safe direction: a settings row that has never been + * written must not opt an instance into the expensive path. + * + * @return void + */ + public function testMissingSettingDefaultsToBridgeOff(): void { + $widgetService = $this->createMock(WidgetService::class); + $widgetService->expects($this->never()) + ->method('getAvailableWidgets'); + + $resolved = $this->resolve( + controller: $this->makeController(widgetService: $widgetService), + settings: [] + ); + + $this->assertSame([], $resolved); + }//end testMissingSettingDefaultsToBridgeOff() +}//end class diff --git a/tests/Unit/Service/AdminSettingsServiceTest.php b/tests/Unit/Service/AdminSettingsServiceTest.php index 3d014ca0a..97d26194b 100644 --- a/tests/Unit/Service/AdminSettingsServiceTest.php +++ b/tests/Unit/Service/AdminSettingsServiceTest.php @@ -229,8 +229,9 @@ public function testGetSettingsSharingAndBridgeDefaults(): void { // Default share permission mirrors the dashboard default. $this->assertSame(Dashboard::PERMISSION_ADD_ONLY, $settings['defaultSharePermissionLevel']); $this->assertSame([], $settings['forcedShareGroups']); - // Bridge defaults ON so existing placements keep rendering. - $this->assertTrue($settings['legacyWidgetBridgeEnabled']); + // Bridge defaults OFF: enabling it makes the workspace read the + // Nextcloud widget registry, which injects every widget's scripts. + $this->assertFalse($settings['legacyWidgetBridgeEnabled']); }//end testGetSettingsSharingAndBridgeDefaults() public function testGetSettingsSharingAndBridgeStoredValues(): void { @@ -238,7 +239,10 @@ public function testGetSettingsSharingAndBridgeStoredValues(): void { [ AdminSetting::KEY_DEFAULT_SHARE_PERMISSION_LEVEL => 'full', AdminSetting::KEY_FORCED_SHARE_GROUPS => ['marketing', 'sales'], - AdminSetting::KEY_LEGACY_WIDGET_BRIDGE_ENABLED => false, + // Stored TRUE, the opposite of the default, so this assertion + // actually proves the stored value is read rather than passing + // on the default. + AdminSetting::KEY_LEGACY_WIDGET_BRIDGE_ENABLED => true, ] ); @@ -246,7 +250,7 @@ public function testGetSettingsSharingAndBridgeStoredValues(): void { $this->assertSame('full', $settings['defaultSharePermissionLevel']); $this->assertSame(['marketing', 'sales'], $settings['forcedShareGroups']); - $this->assertFalse($settings['legacyWidgetBridgeEnabled']); + $this->assertTrue($settings['legacyWidgetBridgeEnabled']); }//end testGetSettingsSharingAndBridgeStoredValues() public function testUpdateSettingsPersistsForcedShareGroupsDeduplicated(): void {