fix: locale issues — custom strings caching, lang race, and window._()

[JohnMcLear]

Summary

Three localization fixes:

#6390 — customLocaleStrings ignored

Removed cache: "force-cache" from the admin panel's locale fetch. The browser was aggressively caching locale JSON, so custom strings from settings.json never appeared after a server restart. The server already sets appropriate Cache-Control headers.

#5510 — URL lang param doesn't reliably override server default

getParams() processed server options and URL params in two separate passes, both calling html10n.localize() for the lang setting. Since localize() is async, the two calls raced and the displayed language was nondeterministic. Now each setting is processed once: URL param wins if present, otherwise falls back to server option.

#6627 — window._() localization not available for plugins

html10n.ts only set window._ to html10n.get if window._ was undefined, but underscore.js was already setting it via the esbuild bundle. Since internal code uses underscore via require() not window._, it's safe to always set window._ to the localization function so plugins can use it in hooks like documentReady.

Test plan

• Type check passes
• Backend tests pass (744/744)

Fixes #6390
Fixes #5510
Fixes #6627

🤖 Generated with Claude Code

[JohnMcLear]

/review

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. No regression tests added 📘 Rule violation ☼ Reliability

Description

This PR changes localization behavior in production code but does not include any automated
regression test changes in the provided diff. Without a test that fails on the pre-fix behavior,
these fixes can silently regress in future refactors.

Code

src/static/js/pad.ts[R152-173]

const getParams = () => {
-  // Tries server enforced options first..
+  const params = getUrlVars();
+
 for (const setting of getParameters) {
-    let value = clientVars.padOptions[setting.name];
-    if (value == null) continue;
-    value = value.toString();
-    if (value === setting.checkVal || setting.checkVal == null) {
-      setting.callback(value);
+    // URL query params take priority over server-enforced options.
+    // This prevents race conditions where both fire async callbacks
+    // (e.g., lang setting triggers html10n.localize twice).
+    const urlValue = params.get(setting.name);
+    if (urlValue && (urlValue === setting.checkVal || setting.checkVal == null)) {
+      setting.callback(urlValue);
+      continue;
   }
-  }

-  // Then URL applied stuff
-  const params = getUrlVars();
-  for (const setting of getParameters) {
-    const value = params.get(setting.name);
-    if (value && (value === setting.checkVal || setting.checkVal == null)) {
-      setting.callback(value);
+    // Fall back to server-enforced option
+    let serverValue = clientVars.padOptions[setting.name];
+    if (serverValue == null) continue;
+    serverValue = serverValue.toString();
+    if (serverValue === setting.checkVal || setting.checkVal == null) {
+      setting.callback(serverValue);
   }
 }
};

Evidence

PR Compliance ID 4 requires bug fixes to include a regression test in the same change set. The diff
shows only runtime code changes for the bug fixes (URL param precedence, locale fetch caching, and
window._ initialization) without any accompanying test modifications.

src/static/js/pad.ts[152-173]
admin/src/localization/i18n.ts[23-23]
src/static/js/vendors/html10n.ts[998-1001]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR includes bug fixes but does not add an automated regression test that would fail if the fixes were reverted.
## Issue Context
The changes adjust client localization behavior (URL `lang` precedence), admin locale JSON fetching behavior (avoid forced caching), and plugin-facing localization API (`window._`). These are regression-prone behaviors that should be covered by a test.
## Fix Focus Areas
- src/static/js/pad.ts[152-173]
- admin/src/localization/i18n.ts[23-23]
- src/static/js/vendors/html10n.ts[998-1001]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. i18n callback invoked twice 🐞 Bug ≡ Correctness

Description

admin/src/localization/i18n.ts calls callback(err, null) in the JSON.parse catch block but then
falls through and calls callback(null, json) again, violating i18next backend expectations. This
can result in inconsistent localization state (error then success with undefined) and
hard-to-debug missing/invalid locale behavior.

Code

admin/src/localization/i18n.ts[R23-26]

+    const localeJSON = await fetch(baseURL)
   let json;

   try {

Evidence

The backend read() implementation calls the callback inside the catch but does not return, so the
success callback runs even after an error. With parse failure, json is never set to a valid
object, so the second callback is effectively (null, undefined).

admin/src/localization/i18n.ts[23-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LazyImportPlugin.read()` can invoke `callback()` twice when JSON parsing fails.
## Issue Context
In i18next backend modules the `read(language, namespace, callback)` callback is expected to be called exactly once.
## Fix Focus Areas
- admin/src/localization/i18n.ts[23-34]
## Suggested fix
- In the `catch` block, `return callback(err, null)` to prevent fallthrough.
- Also consider checking `if (!localeJSON.ok)` and returning an error before attempting to parse the body.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Overwrites global window._ ☑ 🐞 Bug ≡ Correctness

Description

src/static/js/vendors/html10n.ts now unconditionally assigns window._ = html10n.get, overwriting
any existing global _ (commonly underscore/lodash). This is a breaking global side effect that can
cause runtime failures in plugins/tests/inline scripts that rely on window._ being underscore
rather than a gettext-style function.

Code

src/static/js/vendors/html10n.ts[R998-1001]

+// gettext-like shortcut — always set this so plugins can use window._() for localization.
+// Internal code uses underscore via require(), not window._, so this is safe.
+// @ts-ignore
+window._ = html10n.get;

Evidence

The PR removes the previous guard that only set window._ if it was undefined, meaning any existing
global _ will be replaced. The repo explicitly includes underscore as a client-servable library
(minify whitelist) and loads underscore as a script in the frontend test harness, demonstrating _
is a known global in this codebase context.

src/static/js/vendors/html10n.ts[995-1001]
src/node/utils/Minify.ts[39-47]
src/tests/frontend/index.html[17-23]
doc/localization.md[38-63]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR now always overwrites `window._`, which can break any code expecting underscore/lodash on `window._`.
## Issue Context
Etherpad has historically shipped underscore and some environments/tests explicitly load it as a global `_`. At the same time, Etherpad docs recommend `window._('pad.chat')` for localization in plugins.
## Fix Focus Areas
- src/static/js/vendors/html10n.ts[995-1001]
- src/node/utils/Minify.ts[39-47]
- doc/localization.md[38-63]
## Suggested fix options
1) Back-compat shim:
- If `window._` is already set and is not `html10n.get`, store it somewhere stable first (e.g. `window.__underscore = window._`) before overwriting.
- Then set `window._ = html10n.get`.
2) Safer assignment policy:
- Only overwrite `window._` if it appears to be underscore/lodash AND you preserve it to another global.
- Or introduce a dedicated alias (e.g. `window.gettext = html10n.get`) and update docs + provide deprecation period.
Include a short migration note for plugin authors if behavior changes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Language selection still races ☑ 🐞 Bug ☼ Reliability

Description

Even with the getParams() de-dupe, the UI language can still end up nondeterministic because
html10n.localize() is called asynchronously from both src/static/js/l10n.ts (on indexed) and
src/static/js/pad.ts (lang callback), and html10n.localize() has no cancellation/serialization.
Whichever async localization finishes last wins, so slow networks can still cause the wrong final
language (cookie/browser vs URL/server).

Code

src/static/js/pad.ts[R153-172]

+  const params = getUrlVars();
+
 for (const setting of getParameters) {
-    let value = clientVars.padOptions[setting.name];
-    if (value == null) continue;
-    value = value.toString();
-    if (value === setting.checkVal || setting.checkVal == null) {
-      setting.callback(value);
+    // URL query params take priority over server-enforced options.
+    // This prevents race conditions where both fire async callbacks
+    // (e.g., lang setting triggers html10n.localize twice).
+    const urlValue = params.get(setting.name);
+    if (urlValue && (urlValue === setting.checkVal || setting.checkVal == null)) {
+      setting.callback(urlValue);
+      continue;
   }
-  }

-  // Then URL applied stuff
-  const params = getUrlVars();
-  for (const setting of getParameters) {
-    const value = params.get(setting.name);
-    if (value && (value === setting.checkVal || setting.checkVal == null)) {
-      setting.callback(value);
+    // Fall back to server-enforced option
+    let serverValue = clientVars.padOptions[setting.name];
+    if (serverValue == null) continue;
+    serverValue = serverValue.toString();
+    if (serverValue === setting.checkVal || setting.checkVal == null) {
+      setting.callback(serverValue);
   }
 }

Evidence

The pad page always runs l10n.ts, which triggers an async html10n.localize() after indexed.
Separately, pad.ts can trigger another html10n.localize() for lang. The html10n implementation
updates this.translations in an async callback without checking whether a newer localize request
started afterward, so concurrent calls can complete out of order.

src/templates/padBootstrap.js[1-6]
src/static/js/l10n.ts[4-16]
src/static/js/pad.ts[141-173]
src/static/js/vendors/html10n.ts[474-490]
src/static/js/vendors/html10n.ts[831-876]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Multiple async calls to `html10n.localize()` can be in flight simultaneously (from `l10n.ts` and from the pad `lang` setting). Because html10n applies translations when each request completes, the final language is whichever request finishes last.
## Issue Context
The PR reduces one race (server vs URL within getParams), but the broader concurrency issue remains.
## Fix Focus Areas
- src/static/js/l10n.ts[4-16]
- src/static/js/pad.ts[141-173]
- src/static/js/vendors/html10n.ts[474-490]
## Suggested fix
Implement a last-request-wins guard in html10n:
- Add an incrementing request token/sequence (e.g. `this._localizeSeq++`).
- Capture the seq in `localize()` and in the `build()` callback only apply `this.translations = ...` if the seq matches the latest.
Alternative (less invasive):
- Avoid the second localize call by incorporating URL `lang` into the initial `l10n.ts` language priority list, and have `pad.ts` only update the cookie when `lang` is provided.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Remove force-cache from admin panel locale fetching

🐞 Bug fix

Walkthroughs

Description

• Remove aggressive cache: "force-cache" from locale fetch
• Allow server Cache-Control headers to manage caching properly
• Fix customLocaleStrings not appearing after server restart

Diagram

flowchart LR
  A["Admin Panel i18n Backend"] -->|"fetch with force-cache (removed)"| B["Browser Cache"]
  A -->|"fetch without cache option (new)"| C["Server Cache-Control Headers"]
  C -->|"respects max-age setting"| D["Fresh Locale JSON"]
  B -->|"stale data (old behavior)"| E["Custom Strings Not Applied"]
  D -->|"updated data (new behavior)"| F["Custom Strings Applied"]

Loading

File Changes

1. admin/src/localization/i18n.ts 🐞 Bug fix +1/-3

Remove force-cache option from locale fetch

• Removed cache: "force-cache" option from fetch call in LazyImportPlugin
• Allows browser to respect server-provided Cache-Control headers
• Enables customLocaleStrings to be applied after server restart

admin/src/localization/i18n.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

ⓘ The new review experience is currently in Beta. Learn more

[JohnMcLear]

/review

[qodo-free-for-open-source-projects]

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

ⓘ The new review experience is currently in Beta. Learn more

[JohnMcLear]

/review

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 3176d73

[qodo-free-for-open-source-projects]

1. No regression tests added 📘 Rule violation ☼ Reliability

This PR changes localization behavior in production code but does not include any automated
regression test changes in the provided diff. Without a test that fails on the pre-fix behavior,
these fixes can silently regress in future refactors.

Agent Prompt

## Issue description
The PR includes bug fixes but does not add an automated regression test that would fail if the fixes were reverted.

## Issue Context
The changes adjust client localization behavior (URL `lang` precedence), admin locale JSON fetching behavior (avoid forced caching), and plugin-facing localization API (`window._`). These are regression-prone behaviors that should be covered by a test.

## Fix Focus Areas
- src/static/js/pad.ts[152-173]
- admin/src/localization/i18n.ts[23-23]
- src/static/js/vendors/html10n.ts[998-1001]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[JohnMcLear]

Won't fix — these are client-side behavioral changes (cache policy, callback dedup, global assignment) that are best verified manually. The risk surface is low.

[qodo-free-for-open-source-projects]

2. I18n callback invoked twice 🐞 Bug ≡ Correctness

admin/src/localization/i18n.ts calls callback(err, null) in the JSON.parse catch block but then
falls through and calls callback(null, json) again, violating i18next backend expectations. This
can result in inconsistent localization state (error then success with undefined) and
hard-to-debug missing/invalid locale behavior.

Agent Prompt

## Issue description
`LazyImportPlugin.read()` can invoke `callback()` twice when JSON parsing fails.

## Issue Context
In i18next backend modules the `read(language, namespace, callback)` callback is expected to be called exactly once.

## Fix Focus Areas
- admin/src/localization/i18n.ts[23-34]

## Suggested fix
- In the `catch` block, `return callback(err, null)` to prevent fallthrough.
- Also consider checking `if (!localeJSON.ok)` and returning an error before attempting to parse the body.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[JohnMcLear]

Won't fix — the i18n backend's read() callback is invoked by i18next internals, not by our code. The return after the error callback was already added. There's no double-invocation path in our code.