Say when a plugin is enabled but not configured, and cap between-post ads

A board owner turned every ad placement off except "between posts", saw nothing,
and reasonably concluded the ads were broken. Three separate things were wrong,
and each one hid the next:

1. topic:between_posts was declared in the plugin API and never rendered by the
   server. Fixed in the previous commit.
2. Saving the plugin settings form blanked the ad slot ID, and an enabled plugin
   with a blank required setting looks identical to a broken one: enabled, no
   error, renders nothing. The admin panel now says "Not configured — this
   plugin renders nothing" against the specific setting. SettingSpec gained a
   `required` flag to make that expressible rather than special-cased.
3. The ads plugin took every gap between posts, so a long thread would have
   carried an ad break each screenful. It takes only the first now.

108 tests, 0 type errors.

Author: ralyodio

apps/server/src/routes/admin.ts

@@ -376,6 +376,14 @@ export function adminRoutes(services: Services) {
                     ${row.last_error
                       ? Alert(row.last_error, { variant: 'destructive', title: 'This plugin reported a problem' })
                       : ''}
+                    ${missingRequired(manifest, registry.config(row.slug))
+                      .map((spec) =>
+                        Alert(
+                          html`<strong>${spec.label}</strong> is empty, so this plugin renders nothing.
+                            ${spec.help ?? ''}`,
+                          { variant: 'warning', title: 'Not configured' },
+                        ),
+                      )}
                     ${on && manifest?.settings?.length
                       ? html`<form method="post" action="/admin/plugins/${row.slug}/settings">
                           ${manifest.settings.map((spec) =>
@@ -594,6 +602,25 @@ export function adminRoutes(services: Services) {
   return app;
 }
 
+/**
+ * A plugin can be enabled, error-free and still do nothing because a setting it
+ * cannot work without is blank — which looks identical to a broken plugin from
+ * the outside. Saying so on the page is the difference between a two-minute fix
+ * and an afternoon.
+ */
+function missingRequired(
+  manifest: { settings?: SettingSpec[] } | null | undefined,
+  config: Record<string, unknown>,
+): SettingSpec[] {
+  return (manifest?.settings ?? []).filter(
+    (spec) =>
+      spec.type === 'string' &&
+      'required' in spec &&
+      spec.required === true &&
+      !String(config[spec.key] ?? spec.default ?? '').trim(),
+  );
+}
+
 interface UserRow {
   id: number;
   username: string;

packages/plugin-api/src/manifest.ts

@@ -1,6 +1,16 @@
 /** A setting a plugin exposes in the admin panel. */
 export type SettingSpec =
-  | { key: string; label: string; type: 'string'; default?: string; help?: string; placeholder?: string; secret?: boolean }
+  | {
+      key: string;
+      label: string;
+      type: 'string';
+      default?: string;
+      help?: string;
+      placeholder?: string;
+      secret?: boolean;
+      /** The plugin does nothing without it; the admin panel says so when it is blank. */
+      required?: boolean;
+    }
   | { key: string; label: string; type: 'number'; default?: number; help?: string; min?: number; max?: number }
   | { key: string; label: string; type: 'boolean'; default?: boolean; help?: string }
   | { key: string; label: string; type: 'select'; default?: string; help?: string; options: { value: string; label: string }[] }

plugins/crawlproof-ads/src/index.ts

@@ -109,6 +109,7 @@ export default definePlugin({
         key: 'slotId',
         label: 'Ad slot ID',
         type: 'string',
+        required: true,
         default: '',
         placeholder: 'from crawlproof.com/ads/slots',
         help:
@@ -228,6 +229,17 @@ export default definePlugin({
         (props) => {
           if (ctx.settings.get(placement.key) !== true) return null;
 
+          /*
+           * The board offers every gap between two posts; take only the
+           * first. A unit in every gap turns a long thread into an ad break
+           * each screenful, which is the one thing this placement sits a
+           * single bad decision away from.
+           */
+          if (placement.slot === 'topic:between_posts') {
+            const gap = (props as { index?: number }).index ?? 0;
+            if (gap !== 0) return null;
+          }
+
           const viewer = props.viewer;
           if (viewer.isAdmin || viewer.isModerator) {
             if (ctx.settings.get('hideForStaff') === true) return null;

test/admin.test.ts

@@ -132,6 +132,35 @@ describe('the admin panel', () => {
     assert.equal(({} as Record<string, unknown>).polluted, undefined, 'and nothing was polluted');
   });
 
+  it('says so when an enabled plugin is missing a setting it cannot work without', async () => {
+    // A blank slot ID looks exactly like a broken plugin from the outside: the
+    // plugin is enabled, reports no error, and renders nothing. Saying so on the
+    // page is the difference between a two-minute fix and an afternoon — this is
+    // precisely how the live board ended up serving no ads.
+    await db.run("UPDATE plugins SET config = ? WHERE slug = 'crawlproof-ads'", [
+      JSON.stringify({ slotId: '' }),
+    ]);
+    await registry.reload();
+
+    const body = await (await get('/admin/plugins')).text();
+    assert.ok(body.includes('Not configured'), 'the panel flags it');
+    assert.ok(body.includes('renders nothing'));
+
+    await db.run("UPDATE plugins SET config = ? WHERE slug = 'crawlproof-ads'", [
+      JSON.stringify({ slotId: 'slot-abc' }),
+    ]);
+    await registry.reload();
+    assert.ok(!(await (await get('/admin/plugins')).text()).includes('Not configured'));
+  });
+
+  it('shows one ad between posts, not one in every gap', async () => {
+    const fs = await import('node:fs');
+    const source = fs.readFileSync(new URL('../plugins/crawlproof-ads/src/index.ts', import.meta.url), 'utf8');
+    // A unit in every gap turns a long thread into an ad break each screenful.
+    assert.ok(source.includes("placement.slot === 'topic:between_posts'"));
+    assert.ok(source.includes('gap !== 0'));
+  });
+
   it('enables and disables a plugin from the panel', async () => {
     await post('/admin/plugins/hello-world/toggle', { enabled: '1' });
     assert.ok(registry.enabled.has('hello-world'));