From 37b88b2fe66c4a809f078966fdc6d657553b1cc5 Mon Sep 17 00:00:00 2001 From: lilleman Date: Tue, 4 Aug 2026 09:37:03 +0200 Subject: [PATCH] Show the language picker on every page, targeting the nearest page that answers GET --- AGENTS.md | 13 ++++++++----- README.md | 10 ++++++---- e2e-tests/full-flow.spec.ts | 16 +++++++++++----- src/http/app.test.ts | 30 ++++++++++++++++++++++++++++++ src/http/app.ts | 29 ++++++++++++++++++++++++++++- src/i18n/view-locals.test.ts | 11 ++++++----- src/i18n/view-locals.ts | 11 ++++------- 7 files changed, 93 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 344f411..2383c9e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -127,11 +127,14 @@ them. Revisit only if the stated reason stops holding. so a localized list page doesn't hand a plugin a phantom `locale` filter; the i18n view locals (`t`, `locale`, `locales`, `localeHref`, `localeParam`, `localeSwitch`, `dir`) are likewise reserved names, merged after a handler's `data` so a collision loses the key instead of breaking the shell. -- **The language picker never appears on a POST-rendered page.** That URL frequently answers no GET - (`POST /admin/users/:id/recovery` renders a page and has no GET sibling), so a link there dead-ends - on a 405; on a re-rendered form it would also discard the visitor's input. `i18nLocals` returns an - empty `localeSwitch` for any non-GET/HEAD method, and the picker renders nothing below two choices. - Decided 2026-08-03 after a review reproduced the 405. +- **The language picker is on every page, POST-rendered ones included.** Maintainer's call + 2026-08-04, overriding an earlier decision to hide it there. The problem it was hiding is real: a + POST-rendered URL frequently answers no GET (`POST /admin/users/:id/recovery`), so a link back to + it dead-ends on a 405. The host therefore resolves the picker's target (`app.ts` → `switchBase`): + this path when it answers GET, else the same-origin Referer, else `/`. Accepted cost: switching + language on such a page leaves that POST's own result behind (a re-rendered form's input, or a + one-time recovery code). Valid while the picker is expected on literally every page — if that ever + softens, hiding it after a POST is the simpler answer. - **A plugin-owned render always runs on that plugin's context.** The landing slots (`home`, `dashboard`) and an `onRequest` short-circuit dispatch a plugin's handler, so they build the context with `contextFor(pluginId)` exactly as a plugin route does — otherwise `ctx.t` is the core diff --git a/README.md b/README.md index c19eed2..2166ae6 100644 --- a/README.md +++ b/README.md @@ -940,10 +940,12 @@ is stored, so a link is shareable and a page is what its address says it is. Whe for a language, the host carries `?locale=` onto every link *it* renders (menu, sign-in, its own redirects) and `ctx.localeHref(href)` does the same for a plugin's links. The picker in the sidebar footer (and on the auth pages) lists every installed locale, each a plain link to the -same page in that language. It renders only when more than one is installed, and **not at all on a -page rendered in response to a POST** — that URL often answers no GET (following the link would -dead-end), and on a re-rendered form it would throw away what the visitor typed. A plugin building -its own picker from `ctx.locales` should do the same (`ctx.req.method`). +same page in that language; it renders whenever more than one locale is installed — **on every +page**. After a POST the current URL may answer no GET at all (`POST /admin/users/:id/recovery` +renders a page and has no GET sibling), so the host points the picker at the nearest page that does: +this path when it answers GET, else the page the form was submitted from, else `/`. Switching +language there therefore leaves the POST's own result behind — a re-rendered form's input, or a +one-time code — which is the accepted cost of having the picker everywhere. **Writing a catalog.** `en-US.ts` exports the object and its type; every other locale is written against that type, so a missing or misspelled key is a type error before the app ever boots: diff --git a/e2e-tests/full-flow.spec.ts b/e2e-tests/full-flow.spec.ts index 9e4745d..aa7faa5 100644 --- a/e2e-tests/full-flow.spec.ts +++ b/e2e-tests/full-flow.spec.ts @@ -74,9 +74,9 @@ test.describe.serial("authenticated admin journey", () => { await expect(page.locator("html")).toHaveAttribute("lang", "sv-SE"); }); - // A POST that re-renders a page: the write must keep the language, and the picker must not offer - // a link to a URL that only answers POST. - test("a write keeps the visitor's language, and offers no language link on the POST-rendered page", async () => { + // A POST that re-renders a page: the write must keep the language, and the picker — which is on + // every page — must point somewhere that answers GET rather than at the POST-only URL. + test("a write keeps the visitor's language, and the picker still works on the POST-rendered page", async () => { await page.goto("/admin/users?locale=sv-SE"); await page.getByRole("link", { name: "Ny användare" }).click(); await page.fill('input[name="email"]', `lang-${suffix}@plainpages.local`); @@ -88,10 +88,16 @@ test.describe.serial("authenticated admin journey", () => { const row = page.locator("tr", { hasText: `lang-${suffix}@plainpages.local` }); const editHref = await row.locator('a[href^="/admin/users/"]').first().getAttribute("href"); await page.goto(`${editHref}`); - await expect(page.locator('summary[aria-label="Språk"]')).toHaveCount(1); // control: it IS there on the GET + await expect(page.locator('summary[aria-label="Språk"]')).toHaveCount(1); await page.getByRole("button", { name: "Skapa återställningskod" }).click(); // POST-only route await expect(page.getByText("Återställningskod skapad")).toBeVisible(); - await expect(page.locator('summary[aria-label="Språk"]')).toHaveCount(0); // no dead-end link offered + + // The picker is here too, and following it lands on a real page in the other language. + await page.locator('summary[aria-label="Språk"]').click(); + await page.getByRole("link", { name: /English/i }).click(); + expect(page.url()).toContain("locale=en-US"); + await expect(page.locator("html")).toHaveAttribute("lang", "en-US"); + await expect(page.getByRole("heading", { name: "Edit user" })).toBeVisible(); // not a 405 }); test("menu filters by permission: an admin sees the gated Admin section + the plugin", async () => { diff --git a/src/http/app.test.ts b/src/http/app.test.ts index a9b6334..8e77f5b 100644 --- a/src/http/app.test.ts +++ b/src/http/app.test.ts @@ -1503,3 +1503,33 @@ test("an error page renders without composing the menu — it exists for when th assert.match(await res.text(), /Page not found/); assert.equal(built, 0); }); + +test("a POST-rendered page still offers the language picker, pointed at a page that answers GET", async (t) => { + const dir = mkdtempSync(join(tmpdir(), "pp-post-lang-")); + mkdirSync(join(dir, "demo", "views"), { recursive: true }); + t.after(() => rmSync(dir, { force: true, recursive: true })); + // The view renders the picker exactly as the shell does. + writeFileSync(join(dir, "demo", "views", "page.ejs"), `<%- include("partials/locale-switch") %>`); + const demo: Plugin = { + apiVersion: "1.0.0", + id: "demo", + routes: [ + { handler: () => ({ view: "page" }), method: "GET", path: "/thing" }, + { handler: () => ({ view: "page" }), method: "POST", path: "/thing" }, + { handler: () => ({ view: "page" }), method: "POST", path: "/thing/act" }, // POST-only: no GET sibling + ], + }; + const app = createApp({ i18n: createI18n(await loadI18n()), plugins: [demo], pluginsDir: dir }); + await new Promise((r) => app.listen(0, r)); + t.after(() => app.close()); + const url = `http://localhost:${(app.address() as AddressInfo).port}`; + const post = (path: string, headers: Record = {}) => fetch(url + path, { headers, method: "POST" }); + + // A POST whose path also answers GET → the picker points at that page. + assert.match(await (await post("/demo/thing?locale=sv-SE")).text(), /href="\/demo\/thing\?locale=en-US"/); + // A POST-only path → the page the form was submitted from, so the link can't dead-end on a 405. + const fromForm = await post("/demo/thing/act?locale=sv-SE", { referer: `${url}/demo/thing?locale=sv-SE` }); + assert.match(await fromForm.text(), /href="\/demo\/thing\?locale=en-US"/); + // …and with no referer to fall back on, the front page. + assert.match(await (await post("/demo/thing/act?locale=sv-SE")).text(), /href="\/\?locale=en-US"/); +}); diff --git a/src/http/app.ts b/src/http/app.ts index e2cea3b..55ca751 100644 --- a/src/http/app.ts +++ b/src/http/app.ts @@ -30,6 +30,7 @@ import type { SystemCapabilities } from "../plugin-host/system.ts"; import { allowedMethods, isAuthorized, matchRoute } from "../plugin-host/router.ts"; import { buildAuthRoutes } from "../auth/routes.ts"; import { securityHeaders } from "./security-headers.ts"; +import { localPath } from "./safe-url.ts"; import { routePublic, serveStatic } from "./static.ts"; import { renderPluginView } from "../plugin-host/view-resolver.ts"; @@ -120,6 +121,18 @@ export function createApp(options: AppOptions = {}): Server { // it. A plugin's context carries that plugin's translator, so its own catalog wins in its own views. // They are merged LAST: these names are reserved (README → Building plugins), and a handler that // happens to use one loses that key rather than breaking the shell that renders around it. + // Where the language picker on this page should point. Normally the page itself; after a POST + // that URL may answer no GET (POST /admin/users/:id/delete has no GET sibling), so fall back to + // the page the form was submitted from, then to the front page — the picker is on every page, so + // every one of its links has to land somewhere real. + const switchBase = (req: IncomingMessage, url: URL): string => { + const method = (req.method ?? "GET").toUpperCase(); + if (method === "GET" || method === "HEAD") return `${url.pathname}${url.search}`; + const answersGet = matchRoute(plugins, "GET", url.pathname) !== null + || matchBuiltinRoute(builtinRoutes, "GET", url.pathname) !== undefined; + return answersGet ? url.pathname : (sameOriginPath(req) ?? "/"); + }; + // Named field by field on purpose: spreading the context would trigger its lazy `chrome` getter, // composing the menu for every render — including the standalone error pages, which exist to // render when the shell's own data is what failed. @@ -127,7 +140,7 @@ export function createApp(options: AppOptions = {}): Server { locale: ctx.locale, localeHref: ctx.localeHref, locales: ctx.locales, - method: ctx.req.method ?? "GET", + switchBase: switchBase(ctx.req, ctx.url), t: ctx.t, url: ctx.url, }); @@ -403,6 +416,20 @@ export function createApp(options: AppOptions = {}): Server { }); } +// The Referer as a host-relative path, when it is one of ours — the page a form was submitted +// from. Anything off-origin or malformed is discarded rather than trusted into a link. +function sameOriginPath(req: IncomingMessage): string | null { + const referer = req.headers.referer; + if (typeof referer !== "string") return null; + try { + const url = new URL(referer); + if (req.headers.host !== undefined && url.host !== req.headers.host) return null; + return localPath(`${url.pathname}${url.search}`); + } catch { + return null; + } +} + type ViewRenderer = (view: string, data: Record) => Promise; // Turn a handler's RouteResult into the HTTP response. `null` = the handler took over `ctx.res` diff --git a/src/i18n/view-locals.test.ts b/src/i18n/view-locals.test.ts index 0df872c..3b9380d 100644 --- a/src/i18n/view-locals.test.ts +++ b/src/i18n/view-locals.test.ts @@ -8,7 +8,7 @@ const request = (overrides: Partial = {}): I18nRequest => ({ locale: "sv-SE", localeHref: (href) => href, locales: ["en-US", "sv-SE"], - method: "GET", + switchBase: "/admin/users?q=ada", t: ENGLISH, url: new URL("http://localhost/admin/users?q=ada"), ...overrides, @@ -32,8 +32,9 @@ test("dir follows the locale's script", () => { assert.equal(i18nLocals(request({ locale: "ar-EG" })).dir, "rtl"); }); -test("a page rendered from a POST offers no language links — that URL may have no GET at all", () => { - // Following one would dead-end on a 405 (a POST-only route), or silently discard a re-rendered - // form's input. The picker renders nothing below two choices, so an empty list hides it. - assert.deepEqual(i18nLocals(request({ method: "POST" })).localeSwitch, []); +test("the picker points wherever the host says — after a POST that is the nearest page answering GET", () => { + // The picker is on every page; on a POST-rendered one its own URL may answer no GET, so the host + // resolves the target (app.ts → switchBase) and this just renders it. + const locals = i18nLocals(request({ switchBase: "/admin/users/u1" })); + assert.deepEqual(locals.localeSwitch.map((c) => c.href), ["/admin/users/u1?locale=en-US", "/admin/users/u1?locale=sv-SE"]); }); diff --git a/src/i18n/view-locals.ts b/src/i18n/view-locals.ts index f16f1ef..b669948 100644 --- a/src/i18n/view-locals.ts +++ b/src/i18n/view-locals.ts @@ -32,7 +32,9 @@ export interface I18nRequest { locale: string; localeHref: (href: string) => string; locales: string[]; - method: string; // a page rendered in response to a POST has no linkable URL — see localeSwitch + // Where the language picker points — "this page", except after a POST, whose URL may answer no + // GET at all; the host then resolves the nearest page that does (app.ts → switchBase). + switchBase: string; t: Translate; url: URL; } @@ -50,11 +52,6 @@ export const ENGLISH_LOCALS: I18nLocals = { }; export function i18nLocals(ctx: I18nRequest): I18nLocals { - const here = `${ctx.url.pathname}${ctx.url.search}`; - // The picker links to this same page in another language. After a POST that page's URL often has - // no GET at all (the admin's recovery-code screen, say), so linking there would dead-end on a 405 - // — and on a re-rendered form it would silently discard what the user typed. Offer nothing. - const linkable = ctx.method === "GET" || ctx.method === "HEAD"; // ctx.localeHref is a no-op unless the URL asked for a locale, so it is also the honest answer to // "did it?" — asking the function that decides keeps the two from drifting apart. const carried = ctx.localeHref("/") === "/" ? null : ctx.locale; @@ -63,7 +60,7 @@ export function i18nLocals(ctx: I18nRequest): I18nLocals { locale: ctx.locale, localeHref: (href) => ctx.localeHref(href), localeParam: carried, - localeSwitch: linkable ? ctx.locales.map((tag) => ({ current: tag === ctx.locale, href: localeHref(here, tag), label: localeLabel(tag), tag })) : [], + localeSwitch: ctx.locales.map((tag) => ({ current: tag === ctx.locale, href: localeHref(ctx.switchBase, tag), label: localeLabel(tag), tag })), locales: ctx.locales, t: ctx.t, };