From 3f49ef8d21b5081e3a21cfa756758c7c91b17828 Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 2 Sep 2026 19:01:17 +0200 Subject: [PATCH] fix(plugins): readable save button, honest respawn, scrolling hosts dialog (#2170, #2159) The instance-settings save button paired bg-accent with a hard text-white and vanished in dark mode; it uses the accent pair token now. A failed activate() after saving no longer reports success over a dead plugin: the config stays, the response says what happened. The allowed-hosts dialog scrolls like the ones e70d7397 fixed, and the discover cards get the press-scale hardening from #2158, on desktop and on the phone panel. --- .../Admin/AdminPluginsPanel.test.tsx | 37 +++++++++++++++ .../components/Admin/AdminPluginsPanel.tsx | 12 +++-- .../screens/admin/MAdminPluginsPanel.tsx | 2 + .../mobile/admin/MAdminPluginsPanel.test.tsx | 10 +++++ server/src/nest/plugins/plugins.controller.ts | 27 +++++++++-- .../plugins/plugin-instance-settings.test.ts | 45 ++++++++++++++++++- 6 files changed, 125 insertions(+), 8 deletions(-) diff --git a/client/src/components/Admin/AdminPluginsPanel.test.tsx b/client/src/components/Admin/AdminPluginsPanel.test.tsx index 7a68862db..89a870b57 100644 --- a/client/src/components/Admin/AdminPluginsPanel.test.tsx +++ b/client/src/components/Admin/AdminPluginsPanel.test.tsx @@ -858,6 +858,21 @@ describe('AdminPluginsPanel — row actions', () => { expect(screen.getByText('View error log')).toBeInTheDocument() // menu is open expect(screen.queryByText('Allowed hosts')).not.toBeInTheDocument() }) + + it('FE-COMP-PLUGINS-PANEL-050: a long host list scrolls inside the dialog instead of growing past the viewport (#2159)', async () => { + server.use(http.get('*/api/admin/plugins/trek-gotify/egress-hosts', () => + HttpResponse.json({ supported: true, hosts: ['gotify.lan', 'ntfy.lan'] }))) + await openRowMenu() + + fireEvent.click(screen.getByText('Allowed hosts')) + + // The card is capped and its body scrolls, so the input, the Add button and the + // restart note below the list stay reachable however many hosts the admin entered. + const body = (await screen.findByText('gotify.lan')).closest('.overflow-y-auto') + expect(body).not.toBeNull() + expect(body).toContainElement(screen.getByPlaceholderText('gotify.example.com')) + expect(body!.parentElement!.className).toContain('max-h-[88vh]') + }) }) describe('AdminPluginsPanel — enabling a plugin', () => { @@ -1463,6 +1478,18 @@ describe('AdminPluginsPanel — Discover cards and the detail modal', () => { expect(await screen.findByText('Could not load plugin details.')).toBeInTheDocument() }) + it('FE-COMP-PLUGINS-PANEL-041a: the card opts out of the global press-scale (#2158)', async () => { + // jsdom cannot replay the browser mechanics behind #2158: the :active scale on + // the card shifted the Install button out from under the pointer, so the click + // retargeted onto the card and opened the detail modal instead of installing. + // The data-no-press attribute is the pin. + discoverWith({}) + render() + await clickDiscover() + + expect((await screen.findByText('Gotify')).closest('[role="button"]')).toHaveAttribute('data-no-press') + }) + it('FE-COMP-PLUGINS-PANEL-042: the detail modal lists setup fields, metadata and a separate homepage', async () => { discoverWith( { downloadCount: 999_500, reviewedAt: '2026-01-05T00:00:00Z', homepage: 'https://gotify.example', trek: '>=3.2.0' }, @@ -2343,4 +2370,14 @@ describe('AdminPluginsPanel — instance settings', () => { expect(await screen.findByText('"Client ID" is required', {}, { timeout: 5000 })).toBeInTheDocument() expect(called).toBe(false) }) + + it('FE-COMP-PLUGINS-CFG-012: Save paints its label with the accent-text token, so a light accent stays readable', async () => { + await openSettings() + + // `--accent-text` flips per scheme and per light/dark; a hard white label vanishes on + // the near-white accent the dark theme uses by default. + const save = screen.getByRole('button', { name: /^save$/i }) + expect(save.className).toContain('text-accent-text') + expect(save.className).not.toContain('text-white') + }) }) diff --git a/client/src/components/Admin/AdminPluginsPanel.tsx b/client/src/components/Admin/AdminPluginsPanel.tsx index df02b1bc7..7a7488a79 100644 --- a/client/src/components/Admin/AdminPluginsPanel.tsx +++ b/client/src/components/Admin/AdminPluginsPanel.tsx @@ -1013,12 +1013,12 @@ export default function AdminPluginsPanel() { {/* Operator-supplied egress hosts */} {egressFor && (
setEgressFor(null)}> -
e.stopPropagation()}> -
+
e.stopPropagation()}> +
{egressFor.id} — {t('admin.plugins.allowedHosts')}
-
+
{!egressFor.supported ? (

{t('admin.plugins.allowedHosts.unsupported')}

) : ( @@ -1109,7 +1109,7 @@ export default function AdminPluginsPanel() {
@@ -1506,6 +1506,10 @@ function RegistryGrid({ items, onInstall, onOpenDetail, busy, t, installedIds, f const offer = installOffer(item, t) return (
onOpenDetail(item)} + // No press-scale on the card: shrinking it mid-click slides the Install + // button out from under the pointer, so the click retargets onto the card + // and opens the detail modal instead of installing (#2158). + data-no-press onKeyDown={e => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onOpenDetail(item) } }} className="group border border-edge rounded-2xl bg-surface-card overflow-hidden flex flex-col cursor-pointer hover:-translate-y-0.5 hover:shadow-elevated hover:border-edge-faint transition-all duration-150">
diff --git a/client/src/mobile/screens/admin/MAdminPluginsPanel.tsx b/client/src/mobile/screens/admin/MAdminPluginsPanel.tsx index a0f011e28..5fb0e6579 100644 --- a/client/src/mobile/screens/admin/MAdminPluginsPanel.tsx +++ b/client/src/mobile/screens/admin/MAdminPluginsPanel.tsx @@ -1544,6 +1544,8 @@ function RegistryList({ items, onInstall, onOpenDetail, busy, t, installedIds, f const offer = installOffer(item, t) return (
onOpenDetail(item)} + // Same press-scale opt-out as the desktop Discover card (#2158). + data-no-press onKeyDown={e => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onOpenDetail(item) } }} className="overflow-hidden rounded-[18px] border border-[color:var(--m-rowbr)] bg-[color:var(--m-sheetop)]">
diff --git a/client/tests/unit/mobile/admin/MAdminPluginsPanel.test.tsx b/client/tests/unit/mobile/admin/MAdminPluginsPanel.test.tsx index 56ee3939b..3aadd7a99 100644 --- a/client/tests/unit/mobile/admin/MAdminPluginsPanel.test.tsx +++ b/client/tests/unit/mobile/admin/MAdminPluginsPanel.test.tsx @@ -452,6 +452,16 @@ describe('MAdminPluginsPanel — Discover', () => { fireEvent.keyDown(card, { key: 'Enter' }); expect(await screen.findByRole('dialog')).toBeInTheDocument(); }); + + it('FE-MOB-PLUGP-093: the card opts out of the global press-scale (#2158)', async () => { + // jsdom cannot replay the browser mechanics behind #2158: the :active scale on + // the card shifted the Install button out from under the pointer, so the tap + // retargeted onto the card and opened the detail sheet instead of installing. + // The data-no-press attribute is the pin. + await openDiscover([registryEntry()]); + + expect((await screen.findByText('Acme')).closest('[role="button"]')).toHaveAttribute('data-no-press'); + }); }); describe('MAdminPluginsPanel — the registry detail sheet', () => { diff --git a/server/src/nest/plugins/plugins.controller.ts b/server/src/nest/plugins/plugins.controller.ts index 0e6e0ea70..784703864 100644 --- a/server/src/nest/plugins/plugins.controller.ts +++ b/server/src/nest/plugins/plugins.controller.ts @@ -147,17 +147,38 @@ export class PluginsController { /** * Save the admin-owned `scope:'instance'` settings. A RUNNING plugin gets its config * once, in the child's init envelope — so a save re-spawns it (like the egress-hosts - * PUT), and `restarted` tells the UI whether that happened. + * PUT), and `restarted` tells the UI whether that happened. A respawn that fails + * answers 409 `{ error, code: 'RESTART_FAILED', config }`: the save itself landed, + * so the envelope still carries the stored config. */ @Put(':id/config') async updateConfig(@Param('id') id: string, @Body() body: PluginConfigDto): Promise { + // Same gate as the egress-hosts twin: the respawn below is a spawn, and the kill + // switch is read live, so a save must not start a child while plugins are off. + if (!pluginsEnabled()) throw new HttpException({ error: 'Plugins are disabled by server configuration' }, 503); + let config: Record; try { - const config = this.plugins.updateInstanceConfig(id, body || {}); - return { config, restarted: await this.runtime.respawnIfActive(id) }; + config = this.plugins.updateInstanceConfig(id, body || {}); } catch (e) { if (e instanceof MissingRequiredSettingError) throw new HttpException({ error: e.message }, 400); throw e; } + try { + return { config, restarted: await this.runtime.respawnIfActive(id) }; + } catch (e) { + // The config IS written by this point, only bringing the child back up failed + // (a widened permission set awaiting re-consent, a dependency that went away). + // Reporting that as a failed save would be a lie, so the envelope carries the + // saved config and names the restart as the part that broke. `disable()` leaves + // the row enabled while no child runs, so record the plugin as off: the enable + // toggle is where the real reason is offered as a decision the admin can take. + await this.runtime.deactivate(id).catch(() => {}); + const reason = e instanceof Error ? e.message : 'restart failed'; + throw new HttpException( + { error: `Settings saved, but ${id} could not be restarted: ${reason}`, code: 'RESTART_FAILED', config }, + 409, + ); + } } /** diff --git a/server/tests/unit/plugins/plugin-instance-settings.test.ts b/server/tests/unit/plugins/plugin-instance-settings.test.ts index e72d165cf..66adca849 100644 --- a/server/tests/unit/plugins/plugin-instance-settings.test.ts +++ b/server/tests/unit/plugins/plugin-instance-settings.test.ts @@ -7,6 +7,7 @@ * the child once, in its init envelope), and an inactive plugin is left alone. */ import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest'; +import { HttpException } from '@nestjs/common'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -25,7 +26,7 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { PluginsService } from '../../../src/nest/plugins/plugins.service'; import { PluginsController } from '../../../src/nest/plugins/plugins.controller'; -import type { PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; +import { PluginConsentRequired, type PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; import type { PluginRegistryService } from '../../../src/nest/plugins/registry/registry.service'; import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { AddonsService } from '../../../src/nest/addons/addons.service'; @@ -260,6 +261,48 @@ describe('admin config endpoints (controller)', () => { expect(out.config).toEqual({}); expect(out.restarted).toBe(false); }); + + it('INS-010: with the kill switch off the save is refused before anything is written', async () => { + install('p'); + declareField('p', 'apiUrl', 'instance'); + process.env.TREK_PLUGINS_ENABLED = 'false'; + const respawnIfActive = vi.fn(async () => false); + + const failed = await controllerWith({ respawnIfActive }) + .updateConfig('p', { apiUrl: 'https://y.example' }) + .then(() => null, (e: unknown) => e as HttpException); + + expect(failed?.getStatus()).toBe(503); + // The respawn is a spawn: it must not run while the whole plugin system is off. + expect(respawnIfActive).not.toHaveBeenCalled(); + expect(svc().getInstanceConfig('p')).toEqual({}); + }); + + it('INS-011: a respawn that fails reports the save that DID happen, and stops claiming the plugin runs', async () => { + install('p'); + declareField('p', 'apiUrl', 'instance'); + const deactivate = vi.fn(async () => {}); + const respawnIfActive = vi.fn(async () => { + throw new PluginConsentRequired('plugin p requires consent for db:read:trips', ['db:read:trips']); + }); + + const failed = await controllerWith({ respawnIfActive, deactivate }) + .updateConfig('p', { apiUrl: 'https://y.example' }) + .then(() => null, (e: unknown) => e as HttpException); + + expect(failed?.getStatus()).toBe(409); + // The envelope carries the saved config and names the restart as the part that broke: + // the message must not read as "your settings were lost", because they were not. + expect(failed?.getResponse()).toMatchObject({ + code: 'RESTART_FAILED', + config: { apiUrl: 'https://y.example' }, + error: expect.stringMatching(/Settings saved.*db:read:trips/), + }); + expect(svc().getInstanceConfig('p')).toEqual({ apiUrl: 'https://y.example' }); + // disable() leaves the row enabled, so the admin list would keep showing a plugin + // that no longer has a child. The enable toggle is where the reason is offered. + expect(deactivate).toHaveBeenCalledWith('p'); + }); }); describe('defaults reach the child at spawn', () => {