Merge pull request 'feat(modules): PublicLayout takes a shell, MODULE_API_VERSION 1.5.0' (#148) from fix/public-layout-shell into edge

Reviewed-on: #148

client/src/components/PublicLayout.jsx

@@ -1,12 +1,36 @@
 import SiteHeader from './SiteHeader.jsx'
 import SiteFooter from './SiteFooter.jsx'
+import { shellClass } from '../lib/pageShell.js'
 
 // Standard page chrome for the public site + wiki.
-export default function PublicLayout({ section = 'website', header = true, children }) {
+//
+// ── `shell` — added in MODULE_API_VERSION 1.5.0 ────────────────────────────
+//
+// This component supplies the chrome and NOT the body: every core public page
+// wraps its own content in `<div className="shell-… page-body">`, which is what
+// centres it in a max-width column, gives it its top and bottom padding, and —
+// through `page-body { flex: 1 }` — pushes the footer to the bottom of the
+// viewport. Nine of nine core pages do it, so the omission has never shown.
+//
+// A module page cannot: it is handed `PublicLayout` through the UI kit
+// (MODULE_API.md §3.4) and has no way to learn about two class names that appear
+// in no contract. The Integration Kit's acceptance run built a module exactly as
+// the kit teaches and it rendered full-bleed at x=0 with the footer riding up
+// under the content — the precise failure §3.4 says the kit exists to prevent
+// ("a module page that does not look like the site it is installed in").
+//
+// So the wrapper moves behind the component a module already has. `shell` is
+// OPT-IN and omitting it is exactly today's behaviour, which is why core's own
+// nine pages are untouched by this change — they keep their own wrapper, and a
+// page wanting an unusual body still writes its own. The width mapping and its
+// fallback are in lib/pageShell.js, where the DOM-less test runner can reach them.
+export default function PublicLayout({ section = 'website', header = true, shell, children }) {
+  const bodyClass = shellClass(shell)
+
   return (
     <div className="page">
       {header && <SiteHeader section={section} />}
-      {children}
+      {bodyClass ? <div className={bodyClass}>{children}</div> : children}
       <SiteFooter />
     </div>
   )

client/src/lib/pageShell.js

@@ -0,0 +1,26 @@
+// The page-body shell core's public pages sit in, as plain JS.
+//
+// Extracted from PublicLayout.jsx for the reason lib/adminNav.js was: the client
+// test runner has no DOM and cannot import a .jsx file at all
+// (client/test/moduleRegistry.test.js says the same about modules/shared.js), so
+// anything with a rule worth asserting has to live outside the component.
+//
+// The rule worth asserting here is the fallback. `shell` is part of the module
+// contract as of MODULE_API_VERSION 1.5.0 (MODULE_API.md §3.4), which means the
+// value can come from a module core has never seen, written against a version of
+// this list that is older or newer than the one running. An unknown width must
+// therefore still produce a wrapper: a module page at the wrong width looks like
+// the site, and a page with no wrapper does not — it renders full-bleed with the
+// footer riding up under it, which is the defect the prop exists to fix.
+
+const SHELLS = { narrow: 'shell-narrow', mid: 'shell-mid', wide: 'shell-wide' }
+
+export const SHELL_WIDTHS = Object.keys(SHELLS)
+
+// Returns the className for a page body, or null when no shell was asked for —
+// null is "render children bare", which is every core page written before 1.5.0
+// and stays the default forever.
+export function shellClass(shell) {
+  if (!shell) return null
+  return `${SHELLS[shell] || SHELLS.narrow} page-body`
+}

client/src/modules/shared.js

@@ -40,7 +40,8 @@ import { useSite } from '../contexts/SiteContext.jsx'
 import { request, ApiError, BASE } from '../api/client.js'
 
 // The UI kit is CURATED AND CLOSED (§3.4), not a re-export of components/. These
-// seven are what the smallest UO page already needs beyond React and the router:
+// eight exports — five table rows in §3.4, since `PageState` contributes three —
+// are what the smallest UO page already needs beyond React and the router:
 // without them a module either reaches into core's tree — violating the
 // zero-import rule the whole boundary rests on — or ships its own copies, which
 // means a module page that does not look like the site it is installed in, and
@@ -50,11 +51,12 @@ import { request, ApiError, BASE } from '../api/client.js'
 // is a MAJOR one. That is a real constraint on core's own refactoring and it is
 // the price of the boundary being worth anything.
 //
-// `AdminPage` appears in §3.4's table and is deliberately absent: core has no
+// `AdminPage` was in an early draft of §3.4's table and is deliberately absent:
-// such component — admin views are plain markup inside AdminLayout — and
+// core has no such component — admin views are plain markup inside AdminLayout —
-// inventing one to satisfy a table would be a core change with no consumer until
+// and inventing one to satisfy a table would be a core change with no consumer
-// Phase 3. The contract is amended rather than the code padded, and adding it
+// until Phase 3. The contract was amended rather than the code padded (it no
-// later costs a minor bump, which is exactly the case the versioning is for.
+// longer lists it), and adding it later costs a minor bump, which is exactly the
+// case the versioning is for.
 const ui = {
   PublicLayout,
   PageHeader,

client/src/modules/version.js

@@ -11,6 +11,18 @@
 // that the two files can drift, so a test asserts they agree
 // (client/test/moduleRegistry.test.js) rather than trusting a bump to remember
 // both.
+// 1.5.0 — `PublicLayout` takes an optional `shell` prop ('narrow' | 'mid' |
+// 'wide') that renders the `shell-… page-body` wrapper core's own pages write by
+// hand. Additive: omitting it is 1.4.0's behaviour, so §3.4's "changing a kit
+// component's props is major" does not bite — nothing already written changes
+// meaning. It exists because the kit's acceptance run proved a module cannot
+// discover the wrapper: the class names are theme.css's and appear in no
+// contract, so a module page rendered outside the site's column while doing
+// everything the kit said (docs/modules/kit-acceptance.md).
+// 1.4.0 — a rule, not a member: §2.7 forbids a module opening a connection to a
+// game server from the website process (it talks to a sidecar, which owns the
+// durable copy). Nothing on window.__rg changed and nothing on the server's ctx
+// changed either; this half bumps because the two halves state ONE version.
 // 1.3.0 — three additions, all from Phase 3 slice 3 needing them: a nav item may
 // carry an `icon` component (§3.3), core declares a third slot
 // `player.invite.accepted` (§3.7), and `window.__rg.api` gained `BASE`, which
@@ -26,4 +38,4 @@
 // but the two halves state ONE version: a module declares a single coreApi range
 // and is served one chunk, so a client that claimed 1.0.0 while the server
 // answered 1.1.0 would be two answers to one question.
-export const MODULE_API_VERSION = '1.3.0'
+export const MODULE_API_VERSION = '1.5.0'

client/test/pageShell.test.js

@@ -0,0 +1,59 @@
+import { test } from 'node:test'
+import assert from 'node:assert/strict'
+import fs from 'node:fs'
+import path from 'node:path'
+import { fileURLToPath } from 'node:url'
+
+import { shellClass, SHELL_WIDTHS } from '../src/lib/pageShell.js'
+
+// `PublicLayout`'s `shell` prop (MODULE_API.md §3.4, MODULE_API_VERSION 1.5.0).
+// The component itself is .jsx and unreachable from this runner — there is no DOM
+// here — so the rule lives in lib/pageShell.js and is asserted here, and the
+// rendering is proved in a browser (MODULE_API.md §7.7), which is where the
+// defect that produced this prop was found in the first place.
+
+const HERE = path.dirname(fileURLToPath(import.meta.url))
+
+test('no shell means no wrapper — the behaviour every page had before 1.5.0', () => {
+  // null, not an empty string: PublicLayout branches on it to render `children`
+  // bare, and '' would render a <div class=""> that changes core's nine pages.
+  assert.equal(shellClass(undefined), null)
+  assert.equal(shellClass(null), null)
+  assert.equal(shellClass(''), null)
+  assert.equal(shellClass(false), null)
+})
+
+test('each documented width maps to its theme.css class, plus page-body', () => {
+  assert.equal(shellClass('narrow'), 'shell-narrow page-body')
+  assert.equal(shellClass('mid'), 'shell-mid page-body')
+  assert.equal(shellClass('wide'), 'shell-wide page-body')
+})
+
+test('page-body is always present — it is what pushes the footer down', () => {
+  // `.page` is a flex column and `.page-body { flex: 1 }` is the only thing
+  // filling it. A width class on its own centres the content and still lets the
+  // footer ride up under it, which is half the reported defect and the half that
+  // is easy to lose in a refactor.
+  for (const w of SHELL_WIDTHS) {
+    assert.match(shellClass(w), /\bpage-body\b/)
+  }
+})
+
+test('an unknown width still renders a wrapper, at the narrow default', () => {
+  // The value can arrive from a module built against a different version of this
+  // list, so the failure mode has to be "wrong width" and never "no wrapper".
+  assert.equal(shellClass('enormous'), 'shell-narrow page-body')
+  assert.equal(shellClass(true), 'shell-narrow page-body')
+  assert.equal(shellClass('NARROW'), 'shell-narrow page-body')
+})
+
+test('every width this module offers is a class theme.css actually defines', () => {
+  // The contract now names these widths to module authors, so a rename in
+  // theme.css has to fail here rather than silently in a module's page.
+  const css = fs.readFileSync(path.join(HERE, '../src/styles/theme.css'), 'utf8')
+  for (const w of SHELL_WIDTHS) {
+    const cls = shellClass(w).split(' ')[0]
+    assert.ok(css.includes(`.${cls} {`), `theme.css defines .${cls}`)
+  }
+  assert.ok(css.includes('.page-body {'), 'theme.css defines .page-body')
+})

server/src/modules/version.js

@@ -9,6 +9,24 @@
 // Deliberately separate from PROTOCOL_VERSION (which versions the shard wire and
 // has nothing to say about a website module) and from any module's own version.
 
+// 1.5.0 — a CLIENT addition: `PublicLayout` takes an optional `shell` prop that
+// renders the page body wrapper core's own pages write by hand (MODULE_API.md
+// §3.4). Minor, not major: §3.4 makes *changing* a kit component's props a major
+// bump because that breaks a call already written, and adding an optional one
+// breaks nothing — omitting `shell` is 1.4.0's behaviour exactly. Nothing on the
+// server changed; this file bumps for the reason below. Found by the Integration
+// Kit's acceptance run (docs/modules/kit-acceptance.md), where a module built
+// exactly as the kit teaches rendered outside the site's page column.
+//
+// 1.4.0 — no member changed. §2.7 gained one prohibition: a module does not open
+// a connection to a game server from the website process; it talks to a sidecar,
+// which owns the durable copy of the game's state. Minor rather than major
+// because the SURFACE is identical to 1.3.0 — module-uo's `coreApi: "^1.3.0"`
+// still resolves, and it already complies — but a module written against 1.3.0
+// could satisfy every member and still be built the wrong way round, which is
+// what this number now says. The one §2.7 rule with no CI behind it: an outbound
+// socket is not statically detectable the way an internal require is.
+//
 // 1.3.0 — three CLIENT additions from Phase 3 slice 3: a nav item may carry an
 // `icon`, core declares a `player.invite.accepted` slot, and `window.__rg.api`
 // gained `BASE` (which §3.5 always specified and shared.js never published).
@@ -26,6 +44,6 @@
 // an admin action a module performs belongs in core's one audit log, the
 // extension slot needs the user its prefix names, and §2.7 forbids a module
 // reading core's `APP_BASE_URL` for itself. Additions only, so minor.
-const MODULE_API_VERSION = '1.3.0'
+const MODULE_API_VERSION = '1.5.0'
 
 module.exports = { MODULE_API_VERSION }

server/src/router/v1/admin/modules.controller.js

@@ -265,9 +265,13 @@ async function disable(req, res) {
 // this is about to delete, so after an uninstall there is nothing left to purge
 // with. Ticking the box is the last moment the file exists.
 //
-// The order below is the whole of it, and each step depends on the one above:
+// The order below is the whole of it: locate the purge file, STOP, purge, then
-// purge while the SQL is still readable, stop while the code is still loaded,
+// delete. Only the last step is destructive to the filesystem, so the file stays
-// then delete.
+// readable the whole way through — which is why stopping comes first. Dropping a
+// module's tables while it is still started leaves it serving and ingesting
+// against a schema that no longer exists: module-uo's uo-link WebSocket keeps
+// writing shard events for as long as its onShutdown takes to run, and requests
+// in flight answer 500 where a stopped module answers 404.
 async function remove(req, res) {
   const { id } = req.params
   const purge = req.query.purge === 'true' || req.query.purge === '1'
@@ -276,23 +280,26 @@ async function remove(req, res) {
     const onVolume = install.isInstalled(id)
     if (!current && !onVolume) return res.status(404).json({ message: 'No such module.' })
 
-    let purged = null
+    // Resolved before anything happens, because this branch is a 400: a request
+    // that is going to be refused must not stop the module on its way out.
+    let purgeSql = null
     if (purge) {
-      const file = install.purgeFile(id)
+      purgeSql = install.purgeFile(id)
-      if (!file) {
+      if (!purgeSql) {
         return res.status(400).json({
           message: 'This module ships no purge.sql, so its data cannot be deleted. Uninstall without purging instead.',
         })
       }
-      purged = await schema.runPurge(file)
     }
 
-    // Stop it before its files vanish. A module whose directory is deleted out
+    // Stop it before its tables and then its files vanish. A module whose
-    // from under a running onShutdown is being asked to tear down a world whose
+    // directory is deleted out from under a running onShutdown is being asked to
-    // code may already be half-unreadable — and its sockets would otherwise stay
+    // tear down a world whose code may already be half-unreadable — and its
-    // open until the restart, holding a connection on behalf of a module that no
+    // sockets would otherwise stay open until the restart, holding a connection
-    // longer exists on disk.
+    // on behalf of a module that no longer exists on disk.
     await lifecycle.stop(id)
+
+    const purged = purgeSql ? await schema.runPurge(purgeSql) : null
     const removed = await install.removeDir(id)
 
     // A purge leaves nothing: no directory, no tables, no data. Keeping a

server/test/adminModules.test.js

@@ -11,9 +11,11 @@
 //   - **enable must not touch the loader.** Disable ran the module's onShutdown;
 //     there is no onBoot re-dispatch, so flipping the record back would put a
 //     module with closed sockets and cleared timers back on the nav.
-//   - **purge must run before the directory is removed.** purge.sql lives inside
+//   - **uninstall's four steps have exactly one valid order**: stop, purge,
-//     that directory. Reorder those two lines and the feature silently stops
+//     remove the directory, remove the row. purge.sql lives inside that
-//     working, with a 200 and no data deleted.
+//     directory, so purging after the delete silently does nothing with a 200 —
+//     and purging before the stop drops the tables under a module that is still
+//     running. See the test itself for why neither is visible in a response.
 //
 // Point the DB at a closed port BEFORE requiring anything that builds the pool.
 process.env.DB_HOST = '127.0.0.1'
@@ -333,10 +335,20 @@ test('a shutdown hook that failed is reported rather than swallowed', async () =
 
 // ── uninstall and purge ────────────────────────────────────────────────────
 
-test('uninstall purges BEFORE it removes the directory', async () => {
+test('uninstall stops the module, THEN purges, THEN removes the directory', async () => {
-  // The ordering that makes decision 5 work at all: purge.sql is a file inside
+  // Two orderings pinned by one assertion, because both are one line away from
-  // the directory being deleted. Swap these two and the endpoint still answers
+  // being wrong and neither failure is visible in the response:
-  // 200 and deletes nothing.
+  //
+  //   - purge BEFORE removeDir, or decision 5 stops working entirely — purge.sql
+  //     is a file inside the directory being deleted, and the endpoint would
+  //     still answer 200 having deleted nothing.
+  //   - stop BEFORE purge, or the tables are dropped under a module that is
+  //     still started. It keeps serving and ingesting against a schema that no
+  //     longer exists for as long as its onShutdown takes (module-uo's uo-link
+  //     WebSocket keeps writing shard events), and requests in flight answer 500
+  //     where a stopped module answers 404. Nothing about the old order was
+  //     required: purge.sql stays readable until removeDir, which is the only
+  //     step that touches the filesystem.
   const order = []
   modules.get = async (id) => ({ id, state: 'started' })
   install.isInstalled = () => true
@@ -349,12 +361,31 @@ test('uninstall purges BEFORE it removes the directory', async () => {
   const res = mockRes()
   await ctrl.remove(req({ params: { id: 'uo' }, query: { purge: 'true' } }), res)
 
-  assert.deepEqual(order, ['purge', 'stop', 'removeDir', 'removeRow'])
+  assert.deepEqual(order, ['stop', 'purge', 'removeDir', 'removeRow'])
   assert.equal(res.body.purged, 12)
   assert.equal(res.body.restartRequired, true)
   assert.equal(logged[0].action, 'module.purge')
 })
 
+test('a refused purge does not stop the module on its way out', async () => {
+  // The 400 branch below is resolved BEFORE anything is stopped. A request that
+  // is going to be refused must leave the module exactly as it found it —
+  // otherwise "you cannot delete this module's data" would silently take the
+  // module down as a side effect of saying no.
+  const order = []
+  modules.get = async (id) => ({ id, state: 'started' })
+  install.isInstalled = () => true
+  install.purgeFile = () => null
+  lifecycle.stop = async () => { order.push('stop'); return { stopped: true, error: null } }
+  install.removeDir = async () => { order.push('removeDir'); return true }
+
+  const res = mockRes()
+  await ctrl.remove(req({ params: { id: 'uo' }, query: { purge: 'true' } }), res)
+
+  assert.equal(res.statusCode, 400)
+  assert.deepEqual(order, [])
+})
+
 test('a plain uninstall keeps the row and does not purge', async () => {
   const order = []
   modules.get = async (id) => ({ id, state: 'started' })

server/test/loginProtection.test.js

@@ -60,7 +60,13 @@ test('backoffGuard returns a generic 429 while locked out', async () => {
     a.post('/login', lp.backoffGuard, (req, res) => res.json({ ok: true }))
   })
   try {
-    lp.recordFailure('203.0.113.40') // lock the test client IP
+    // Lock the test client IP. FIVE failures, not one: the lock is
+    // `BASE_MS * 2 ** (count - 1)`, so a single failure locks for exactly one
+    // second and this test then races the round trip. It lost that race on CI
+    // (200 instead of 429, request arriving 1,456 ms after the lock). Five
+    // failures lock for sixteen seconds, which is not a race. What is under
+    // test is the guard's ANSWER while locked out, and that is unchanged.
+    for (let i = 0; i < 5; i += 1) lp.recordFailure('203.0.113.40')
     const res = await fetch(`${app.url}/login`, {
       method: 'POST',
       headers: { 'X-Forwarded-For': '203.0.113.40' },