Show the language picker on every page, targeting the nearest page that answers GET
CI / full-gate (push) Successful in 2m39s
CI / full-gate (push) Successful in 2m39s
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<void>((r) => app.listen(0, r));
|
||||
t.after(() => app.close());
|
||||
const url = `http://localhost:${(app.address() as AddressInfo).port}`;
|
||||
const post = (path: string, headers: Record<string, string> = {}) => 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"/);
|
||||
});
|
||||
|
||||
+28
-1
@@ -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<string, unknown>) => Promise<string>;
|
||||
|
||||
// Turn a handler's RouteResult into the HTTP response. `null` = the handler took over `ctx.res`
|
||||
|
||||
@@ -8,7 +8,7 @@ const request = (overrides: Partial<I18nRequest> = {}): 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"]);
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user