diff --git a/plugins/EXTRACTING-A-PLUGIN.md b/plugins/EXTRACTING-A-PLUGIN.md index c64e3841..bcdd61ad 100644 --- a/plugins/EXTRACTING-A-PLUGIN.md +++ b/plugins/EXTRACTING-A-PLUGIN.md @@ -78,9 +78,11 @@ Music added the field; see its PLUGIN.md for what it is guarding against. **Nothing may branch on provenance** except `mountPrefix()`. First-party and third-party differing anywhere else means two systems, and only one gets tested. -**Uninstall never destroys data.** The generated schema barrel follows plugin **directories**, not the -install table — `db:push` drops what it cannot see, so following installs would delete a plugin's tables -on uninstall. Only deleting a plugin's source can lose its data. +**Only an explicit tick destroys data.** The generated schema barrel follows plugin **directories**, not +the install table — `db:push` drops what it cannot see, so following installs would delete a plugin's +tables the moment it was stopped. Deleting a plugin removes its directory, and the confirmation's +"also delete all its data" checkbox is the one thing that drops its tables. Unticked, the rows outlive +the plugin and show up in the orphan list on the same screen. --- @@ -125,9 +127,9 @@ Then through `/plugins`, watching PM2 and the browser at each step: | permissions page | its permission is listed, read/write/none | | disable | route 404s, sidecar stops, **tables and rows survive** | | enable | comes back | -| uninstall | route gone, `pm2 list` loses it, **data still there** | -| `bun db:push` while uninstalled | **do not run this — see "What changed on 2026-08-16"** | -| install again | identical to the first install | +| delete, checkbox unticked | route gone, `pm2 list` loses it, directory gone, **data still there** | +| install again | fresh clone, and it finds those rows | +| delete, checkbox ticked | tables dropped too — a machine that never had it | A normal refresh is enough; the shell is `no-store`. When the log's last line appears, the bundle exists. @@ -220,15 +222,21 @@ no terminal it applies NOTHING and exits 0, so an install reports success and th query with 42P01. `--force` would be worse if it worked: the destructive statements are `drop table` for every plugin whose directory is absent. -So: the row in the verification table that says to run `bun db:push` while uninstalled is struck out. The -property it was checking — uninstall does not destroy data — is now checked by reinstalling and finding -your rows still there. +So: the row in the verification table that said to run `bun db:push` while stopped is gone. The property +it was checking — stopping a plugin does not destroy data — is now checked by deleting it with the +checkbox unticked, installing again, and finding your rows still there. -**Use `purge` to test the install twice.** Uninstall deliberately keeps the directory and the tables, so a -second install is not a fresh one: the marketplace hides a plugin whose directory exists, and the tables -are already there. The Marketplace screen has "Delete and all its data" — it drops the tables, -deletes the directory and regenerates the barrel, and afterwards the plugin installs from the catalogue -exactly as a stranger's machine would. **That is the only honest test of an install.** +**Delete WITH the checkbox to test the install twice.** Anything less is not a fresh install: the +marketplace hides a plugin whose directory exists, and tables that are already there make `db:push` a +no-op, so a broken schema stays broken and passes. Ticked, it drops the tables, deletes the directory and +regenerates the barrel — and afterwards the plugin installs from the catalogue exactly as a stranger's +machine would. **That is the only honest test of an install.** + +**Uninstall was removed on 2026-08-21.** It sat between disable and delete, keeping the directory and the +data, and the screen could not show the difference: an uninstalled plugin stayed in the list under a +heading that said "Installed" while its directory silently decided what the next install would run. That +is not hypothetical — it is how a jellyfin install on alpha re-applied a checkout from two days earlier, +reporting success at every step. The verbs are now enable, disable and delete. **The install now verifies its own tables.** After the push it asks Postgres whether each declared table exists and fails the install naming what is missing. If your extraction reports "schema push reported diff --git a/src/servers/api/plugins/router.ts b/src/servers/api/plugins/router.ts index e4a51bb9..8e8d4bb7 100644 --- a/src/servers/api/plugins/router.ts +++ b/src/servers/api/plugins/router.ts @@ -12,7 +12,6 @@ import { pluginProcessStatus, setPluginRunning, purgePlugin, - uninstallPlugin, type PluginActionResult, } from '../../plugins/install'; import { updatePlugin, syncCapabilities } from '../../plugins/update'; @@ -218,15 +217,20 @@ pluginsRouter.post('/:appName/install', async (ctx) => { return ctx.json(result, result.ok ? 200 : 400); }); -// Destructive, and the only route here that is. Named `purge` rather than being a flag on uninstall so -// nothing can reach it by passing a body field to the verb people already use. -pluginsRouter.post('/:appName/purge', async (ctx) => { - return ctx.json(await purgePlugin(ctx.req.param('appName'))); +// ── Two doors, because "delete" means two things ── +// +// `delete` takes the plugin off the machine and leaves its tables. `purge` takes the data as well. Two +// routes rather than one route and a `dropData` body field: a flag can be added to a request by anything +// that already knows how to call the safe verb, and the difference here is somebody's rows. +// +// The UI's checkbox chooses which one it calls, so the confirmation the owner reads and the door that +// opens are the same decision. +pluginsRouter.post('/:appName/delete', async (ctx) => { + return ctx.json(await purgePlugin(ctx.req.param('appName'), { dropData: false })); }); -pluginsRouter.post('/:appName/uninstall', async (ctx) => { - const result = await uninstallPlugin(ctx.req.param('appName')); - return ctx.json(result, result.ok ? 200 : 400); +pluginsRouter.post('/:appName/purge', async (ctx) => { + return ctx.json(await purgePlugin(ctx.req.param('appName'), { dropData: true })); }); pluginsRouter.post('/:appName/enable', async (ctx) => { @@ -239,7 +243,7 @@ pluginsRouter.post('/:appName/disable', async (ctx) => { return ctx.json(result, result.ok ? 200 : 400); }); -// ── The same four verbs, streamed ── +// ── The same verbs, streamed ── // // An install writes an ecosystem entry, starts a process and rebuilds the router. Collecting all of that // and answering once means a spinner that stops, with no way to tell "started the sidecar" from "could @@ -261,10 +265,11 @@ type VerbFn = ( const VERBS = { install: (appName, onStep, userId, onLog) => installPlugin(appName, onStep, userId, onLog), - uninstall: (appName, onStep, _userId, onLog) => uninstallPlugin(appName, onStep, onLog), enable: (appName, onStep) => setPluginRunning(appName, true, onStep), disable: (appName, onStep) => setPluginRunning(appName, false, onStep), - purge: (appName, onStep, _userId, onLog) => purgePlugin(appName, onStep, onLog), + // Same function, and the verb IS the flag — see the two routes above. + delete: (appName, onStep, _userId, onLog) => purgePlugin(appName, { dropData: false }, onStep, onLog), + purge: (appName, onStep, _userId, onLog) => purgePlugin(appName, { dropData: true }, onStep, onLog), // A pull and then the ordinary install path — see plugins/update.ts. It is a verb rather than its own // route so it inherits the streaming door and the UI that already drives install. update: (appName, onStep, userId, onLog) => updatePlugin(appName, onStep, userId, onLog), diff --git a/src/servers/plugins/install.ts b/src/servers/plugins/install.ts index e26c6c46..4f2cd968 100644 --- a/src/servers/plugins/install.ts +++ b/src/servers/plugins/install.ts @@ -472,10 +472,18 @@ export async function installPlugin( } /** - * Uninstall: stop answering, stop running, forget. + * Stop answering, stop running, forget — the first half of deleting a plugin. * - * Keeps every table and row the plugin owns, and keeps the directory. Reinstalling is therefore a restore - * rather than a fresh start, which is the whole reason not to drop anything here. + * NOT a verb any more. Until 2026-08-21 this was its own button beside Disable, and the screen offered + * three states that all read as "stopped": disabled, uninstalled, and uninstalled-with-the-directory- + * still-there. The difference that mattered was invisible — an uninstalled plugin stayed in the list + * under a heading saying "Installed", so a successful uninstall looked like a failed one, and the + * directory it left behind silently decided what the next install would run. + * + * So it lives on as a step: `purgePlugin` calls it before dropping anything, because a sidecar must not + * be running against tables that are about to vanish. Keeps every table, row and file — what it removes + * is the PLATFORM's record of the plugin: the install row, the PM2 entry, the published copies, and the + * grants naming permissions that are about to stop existing. */ export async function uninstallPlugin(appName: string, onStep?: OnStep, onLog?: OnLog): Promise { const steps: string[] = []; @@ -529,16 +537,21 @@ export async function uninstallPlugin(appName: string, onStep?: OnStep, onLog?: /** * Delete a plugin completely: its rows, its tables, and its source. * - * ── Why this exists next to an uninstall that deliberately keeps everything ── + * ── One function, two verbs, and the checkbox that chooses ── * - * `uninstallPlugin` is careful on purpose. It stops the plugin and forgets it is installed, and every - * table and row survives, because "stop running this" must never mean "delete my data" — a reinstall - * puts you back where you were, which is the property that makes uninstalling safe to try. + * `dropData` is the difference between the two things "delete" can mean, and they are not close enough to + * guess at. WITHOUT it the plugin leaves the machine and its tables stay: the platform stops offering it, + * the directory goes, and reinstalling later finds the rows where they were. WITH it the tables go too, + * which is the only way back to a machine that has never seen the plugin — the state an install needs to + * be testable twice, because the second install finds the directory gone AND the schema unapplied. * - * That leaves no way to get back to a CLEAN machine. There was no way to test an install twice: the - * second one finds the directory already there, so the catalogue hides it, and finds the tables already - * there, so `db:push` has nothing to do and a broken schema stays broken. This is the verb for that, and - * it is deliberately a different word with a different button and a confirmation. + * It is an argument rather than a body field on one route: `POST /:appName/delete` and + * `POST /:appName/purge` are separate doors, so nothing reaches the destructive one by adding a field to + * the request people already send. The UI asks with a checkbox and picks the door. + * + * Uninstall used to sit between these as its own verb. It was removed on 2026-08-21 — three states that + * all looked stopped, and the difference between two of them was invisible on screen. What it did is + * still here, as a step inside this rather than a button of its own. * * ── Order, and why this one ── * @@ -550,7 +563,17 @@ export async function uninstallPlugin(appName: string, onStep?: OnStep, onLog?: * longer exists is exactly the failure that stopped a server booting on 2026-08-16. Leaving it stale here * would break the next `db:push` rather than this command. */ -export async function purgePlugin(appName: string, onStep?: OnStep, onLog?: OnLog): Promise { +export type PurgeOptions = { + /** Drop the plugin's tables. False leaves every row where it is — see the note above. */ + dropData: boolean; +}; + +export async function purgePlugin( + appName: string, + { dropData }: PurgeOptions, + onStep?: OnStep, + onLog?: OnLog, +): Promise { const steps: string[] = []; const plugin = await findPlugin(appName); if (!plugin) return { ok: false, appName, steps, error: `"${appName}" is not on this machine` }; @@ -574,8 +597,14 @@ export async function purgePlugin(appName: string, onStep?: OnStep, onLog?: OnLo if (tables.length) { const present = await existingTables(tables); - await dropTables(present); - await step(steps, onStep, present.length ? `dropped: ${present.join(', ')}` : 'no tables to drop'); + if (dropData) { + await dropTables(present); + await step(steps, onStep, present.length ? `dropped: ${present.join(', ')}` : 'no tables to drop'); + } else if (present.length) { + // Named, not summarised. These outlive the plugin by request, and they will show up in the orphan + // scan on this same screen a moment from now — saying which ones here is what connects the two. + await step(steps, onStep, `data kept: ${present.join(', ')}`); + } } await rm(plugin.dir, { recursive: true, force: true }); @@ -586,7 +615,13 @@ export async function purgePlugin(appName: string, onStep?: OnStep, onLog?: OnLo generatePluginSchemas(plugins); await step(steps, onStep, 'schema barrel regenerated'); - await step(steps, onStep, `"${appName}" can now be installed fresh from the marketplace`); + await step( + steps, + onStep, + dropData + ? `"${appName}" can now be installed fresh from the marketplace` + : `"${appName}" can be installed again from the marketplace — its data is still here`, + ); return { ok: true, appName, steps }; } diff --git a/src/servers/plugins/npm-deps.ts b/src/servers/plugins/npm-deps.ts index 165c51cb..09487c7b 100644 --- a/src/servers/plugins/npm-deps.ts +++ b/src/servers/plugins/npm-deps.ts @@ -154,8 +154,8 @@ export const anyPluginWantsPackages = (plugins: DiscoveredPlugin[]): boolean => // wired. Removed rather than left dangling — an export with no caller is an unfinished thought pretending // to be a feature. // -// The behaviour it would have served is right anyway, for a reason worth stating. Uninstall keeps the -// plugin's DIRECTORY (see the runbook), so `discoverPlugins` still finds it and its packages are still -// declared. Only "Delete … and all its data" removes the directory, and after that the next install of -// any plugin regenerates the merged file without it. So the packages linger until the next install and -// then go, with nothing needing to remember them. +// The behaviour it would have served is right anyway, for a reason worth stating. Disabling a plugin keeps +// its DIRECTORY, so `discoverPlugins` still finds it and its packages are still declared — a disabled +// plugin that is re-enabled needs them, and nothing had to remember that. Deleting removes the directory, +// and the next install of any plugin regenerates the merged file without it. So the packages linger until +// then and go on their own. diff --git a/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx b/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx index 9d765935..cea195fe 100644 --- a/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx +++ b/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx @@ -5,6 +5,8 @@ import { useQuery } from '@tanstack/react-query'; import ReactMarkdown from 'react-markdown'; import remarkGfm from 'remark-gfm'; import { useClient } from 'hooks/useClient'; +import { Checkbox } from '@/components/ui/checkbox'; +import { Dialog, DialogContent, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog'; import { usePlugins, type AvailablePlugin, @@ -15,7 +17,7 @@ import { type PluginVerb, } from './usePlugins'; -// The right panel: one plugin, and the four verbs. +// The right panel: one plugin, and its verbs — install, enable, disable, update, delete. // // Reads `?selected=` itself rather than being handed a plugin by the list — neither panel tells the other // anything, so they cannot disagree. @@ -284,72 +286,109 @@ const Readme = ({ appName }: { appName: string }) => { }; /** - * Delete a plugin and everything it owns. + * Delete a plugin: take it off the machine, and choose whether its data goes too. * - * Separated from the row of ordinary verbs above by a rule and its own confirmation, because it is the - * only control on this screen that destroys data. Uninstall deliberately keeps every table and row so a - * reinstall puts you back where you were; this is the opposite, and it should not be one slip away from - * the button next to it. + * ── Why a checkbox and not two buttons ── * - * The confirmation is inline rather than a dialog: it states exactly what will go, and it names the - * plugin. A modal that says "Are you sure?" tests whether you can click twice, not whether you know what - * you are about to lose. + * "Delete" is one intention with two scopes, and which one somebody wants depends on why they are + * deleting. Moving a plugin aside to reinstall it means keeping the rows; getting a machine back to + * never-having-had-it means dropping them. Two buttons would make that a choice between two words that + * sound identical; a checkbox inside the confirmation makes it a decision taken while reading what each + * one does, which is the only moment it can be made honestly. * - * ── Only once it is uninstalled ── + * Unticked by default, and it stays unticked — the destructive scope is never the one you get by + * clicking through. What it will drop is named on the line beside it rather than described, because + * "all its data" means nothing until you can see it says `jellyfin_servers`. * - * This used to be offered in every state, including while the plugin was running. Two clicks — one of - * them a confirmation nobody reads the second time — separated a working plugin from its tables being - * dropped. Now deleting is the SECOND of two verbs: uninstall stops it and forgets it, and only then can - * it be deleted. That is also the order the buttons appear in, so the screen teaches the sequence. + * ── Only when it is not running ── * - * Nothing is lost by the gate. The state this exists for — a directory sitting on disk keeping the plugin - * out of the marketplace — is by definition an uninstalled one. + * Offered on a disabled or not-installed plugin, never a live one. Disable is a single click and it is + * reversible; requiring it first means a running plugin is always two deliberate steps from being gone, + * with the confirmation as the second. It also matches what the code does — `purgePlugin` stops the + * sidecar before dropping anything, because dropping a table under a running process gives it a 42P01 + * rather than a shutdown. */ -const Purge = ({ plugin, busy, onPurge }: { plugin: PluginItem; busy: boolean; onPurge: () => void }) => { - const [confirming, setConfirming] = useState(false); +const DeletePlugin = ({ + plugin, + busy, + onDelete, +}: { + plugin: PluginItem; + busy: boolean; + onDelete: (dropData: boolean) => void; +}) => { + const [open, setOpen] = useState(false); + const [dropData, setDropData] = useState(false); - // Installed means uninstall first. The button is absent rather than disabled: a disabled control invites - // a hunt for what would enable it, and the answer here is a different button three inches away. - if (plugin.installed) return null; + if (plugin.enabled) return null; - if (!confirming) { - return ( -
- -

- Removes its tables, its data and plugins/{plugin.appName} — and puts it back in the marketplace. -

-
- ); - } + const hasTables = !!plugin.has?.schema; return ( -
-

Delete {plugin.label} completely?

-
    -
  • - its tables are dropped — every row the plugin stored is gone - {plugin.has?.schema ? '' : ' (this one declares no tables)'} -
    • -
  • - the source directory plugins/{plugin.appName} is deleted -
    • -
  • it stops, unmounts, and its sidecar entry is removed
    • -
-

- Afterwards it can be installed fresh from the marketplace. This cannot be undone. -

-
- - -
-
+ <> + + + + + + Delete {plugin.label}? + + +
    +
  • it stops, unmounts, and its sidecar entry is removed
    • +
  • + the source directory plugins/{plugin.appName} is deleted +
    • +
  • its permission grants are removed — reinstalling means granting again
    • +
+ + {hasTables ? ( + + ) : ( +

This plugin declares no tables, so there is no data to keep.

+ )} + + + + + + + + ); }; @@ -457,7 +496,15 @@ export const PluginDetail = () => { > Install - ) : ( + ) : null} + {!plugin.installed ? ( + run(dropData ? 'purge' : 'delete', plugin.appName)} + /> + ) : null} + {plugin.installed ? ( <> {plugin.enabled ? ( - + {/* Absent while enabled — disable first. `DeletePlugin` decides that itself rather than being + conditioned here, so the rule lives next to the confirmation that explains it. */} + run(dropData ? 'purge' : 'delete', plugin.appName)} + /> - )} + ) : null} - {/* Uninstall keeps every table and row the plugin owns, so this is worth saying rather than - leaving someone to guess whether the button destroys their data. */} + {/* Disable and delete are different in kind, not in degree, so the screen says which is which + rather than leaving someone to infer it from two buttons sitting next to each other. */}

{plugin.installed - ? 'Disabling unmounts its routes and stops its sidecar. Uninstalling also forgets the install — neither deletes anything the plugin stored.' - : 'Not installed, but the code is still on this machine — which is why the marketplace does not offer it. Installing again reuses this directory as it stands; deleting it below gets you a fresh clone.'} + ? 'Disabling unmounts its routes and stops its sidecar, and changes nothing else — it is reversible in one click. Deleting needs it disabled first.' + : 'Not installed, but the code is still on this machine — which is why the marketplace does not offer it. Installing again reuses this directory as it stands; deleting gets you a fresh clone.'}

- - run('purge', plugin.appName)} /> ); }; diff --git a/src/workspaces/officerdev/src/apps/Plugins/PluginsList.tsx b/src/workspaces/officerdev/src/apps/Plugins/PluginsList.tsx index c8124ae3..373a8355 100644 --- a/src/workspaces/officerdev/src/apps/Plugins/PluginsList.tsx +++ b/src/workspaces/officerdev/src/apps/Plugins/PluginsList.tsx @@ -160,7 +160,7 @@ export const PluginsList = () => { * Tables in the database that no code on this machine claims. * * Deliberately at the BOTTOM and deliberately quiet. It is not an error — a plugin's tables outlive it on - * purpose, so an uninstalled plugin that is still on disk owns its own — and most machines will never show + * purpose, so a plugin deleted without its data leaves its tables behind — and most machines will never show * this at all. * * It exists because the last time these were present nothing said so. `bun db:push` saw tables the schema @@ -195,8 +195,8 @@ const Orphans = ({ orphans }: { orphans: OrphanTable[] }) => { ))}

- Kept, not deleted — uninstalling a plugin never drops its data. Install the plugin to adopt them, or drop - them yourself if they are from something gone for good. + Kept, not deleted — a plugin deleted without ticking "also delete all its data" leaves these behind. Install + the plugin to adopt them, or drop them yourself if they are from something gone for good.

diff --git a/src/workspaces/officerdev/src/apps/Plugins/usePlugins.ts b/src/workspaces/officerdev/src/apps/Plugins/usePlugins.ts index 9338da98..ecf8c73d 100644 --- a/src/workspaces/officerdev/src/apps/Plugins/usePlugins.ts +++ b/src/workspaces/officerdev/src/apps/Plugins/usePlugins.ts @@ -2,7 +2,7 @@ import { useCallback, useRef, useState } from 'react'; import { useQuery, useQueryClient } from '@tanstack/react-query'; import { useClient, getHeaders } from 'hooks/useClient'; -// Reading and driving the plugin system. One query, four verbs. +// Reading and driving the plugin system. One query, and the verbs that change it. // // Not the app store. That installs sidecars from a compiled-in catalogue, provisioning containers and // asking questions; this installs plugins from the tree and asks nothing. @@ -84,7 +84,8 @@ export type UpdateAllResult = { failed: { appName: string; error?: string }[]; }; -export type PluginVerb = 'install' | 'uninstall' | 'enable' | 'disable' | 'purge' | 'update'; +// `delete` and `purge` are the same action at two scopes — see the two routes in api/plugins/router.ts. +export type PluginVerb = 'install' | 'enable' | 'disable' | 'delete' | 'purge' | 'update'; /** * One line in the action log. @@ -98,7 +99,7 @@ export type ActionEntry = { kind: 'step' | 'log'; text: string }; /** * A table in the database that no code on this machine claims. * - * Not an error — a plugin's tables outlive it deliberately, so an uninstalled-but-present plugin still + * Not an error — a plugin's tables outlive it deliberately, so a plugin deleted without its data still * owns its own. These are the ones with nothing left to explain them, and they are worth showing because * the last time some existed they silently broke an install and were found by reading `\dt` by hand. */