test: regression tests for Settings CJS compat (#7543)

[JohnMcLear]

What happened:

Someone installed Etherpad, added the ep_font_color plugin, and the pad page crashed with Cannot read
properties of undefined (reading 'indexOf').

Why:

Etherpad used to be written in plain JavaScript. We rewrote the Settings file in TypeScript and made it
"modern" — saying export default settings.

Plugins like ep_font_color still do the old thing: require('…/Settings').toolbar. Under the new modern
rules, that now gives you { default: { toolbar: … } } instead of the old flat { toolbar: … }. So
settings.toolbar is undefined, and then .indexOf(…) blows up.

The fix (already on develop, not in v2.6.1):
PR #7421 added a small shim at the bottom of Settings.ts that puts every top-level field (like toolbar,
skinName) back directly on module.exports. Plugins can read .toolbar again without knowing anything
changed.

What I did today:
The original fix shipped with no test. If someone refactors that shim in six months, nobody would notice
until plugins crash again. I opened PR #7551 with 4 tests that lock in the contract:

1. require().toolbar isn't undefined
2. toolbar has left/right/timeslider arrays
3. No hidden .default wrapper
4. Setters still work

For the user stuck on v2.6.1: fix isn't in any release yet — run develop, or uninstall
ep_font_color/ep_font_size/ep_plugin_helpers, until the next tag.

Summary

Pins the contract that #7421 restored: plugins using
require('ep_etherpad-lite/node/utils/Settings') must be able to read
top-level fields (.toolbar, .skinName, .padOptions, …) directly,
without fishing through a .default wrapper.

The original bug (#7543) manifested as:

TypeError: Cannot read properties of undefined (reading 'indexOf')
    at Object.skip (ep_font_color/index.js:7:47)
    at ... pad.html:67

because settings.toolbar came back undefined under tsx's ESM/CJS
interop. #7421 added a CJS compat shim that re-exposes each top-level
field on module.exports via accessor properties, but shipped without
a regression test. This PR adds four focused tests:

• exposes top-level fields directly on require() result — covers the
  exact failure mode of Crash after fresh install and addition of plugins. #7543.
• toolbar has the shape plugins index into (left/right/timeslider) —
  covers JSON.stringify(toolbar).indexOf(...) in ep_font_color.
• does not hide the real value under a .default wrapper — guards
  against a future refactor that forgets the shim.
• setters propagate so reloadSettings() changes are visible to plugins —
  covers the setter path added by fix: add setters to CJS compatibility layer in Settings #7481.

No production code changes — tests only. Fix is already on develop.

Test plan

• CI Linux without plugins and Linux with Plugins both green.
• The new block prints 4 passing assertions locally under
  npx mocha --import=tsx --timeout 120000 tests/backend/specs/settings.ts -g "CJS compatibility".
• No changes to src/node/utils/Settings.ts or toolbar.ts.

Closes #7543.

🤖 Generated with Claude Code

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

Review Summary by Qodo

Add regression tests for Settings CJS compatibility (#7543)

🧪 Tests

Walkthroughs

Description

• Adds regression tests for Settings CJS compatibility
• Verifies top-level fields accessible via require() without .default wrapper
• Tests toolbar structure with left/right/timeslider arrays
• Validates setter propagation for reloadSettings() visibility

Diagram

flowchart LR
  A["Settings CJS Compat Bug #7543"] --> B["Require returns undefined fields"]
  B --> C["Plugins crash on toolbar.indexOf"]
  D["CJS Compat Shim #7421"] --> E["Re-expose fields via accessors"]
  E --> F["Tests verify contract"]
  F --> G["Top-level fields accessible"]
  F --> H["Toolbar structure preserved"]
  F --> I["Setters propagate changes"]

Loading

File Changes

1. src/tests/backend/specs/settings.ts 🧪 Tests +58/-0

Regression tests for Settings CJS compatibility

• Added comprehensive regression test suite for Settings CJS compatibility
• Tests verify top-level fields (toolbar, skinName, padOptions) are directly accessible via
 require()
• Validates toolbar object structure with left/right/timeslider arrays
• Confirms setter propagation for reloadSettings() visibility to plugins

src/tests/backend/specs/settings.ts

---

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

Code Review by Qodo

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

1. Protocol-specific GitHub URL 📘 Rule violation ⚙ Maintainability

Description

A new protocol-specific URL (https://...) is introduced in a comment instead of a
protocol-independent form. This may violate the project guideline to prefer protocol-independent
URLs where applicable.

Code

src/tests/backend/specs/settings.ts[93]

+  // Regression test for https://github.com/ether/etherpad/issues/7543.

Evidence

PR Compliance ID 10 requires protocol-independent URLs where applicable, but the added comment
includes an https:// URL.

src/tests/backend/specs/settings.ts[93-93]
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
A comment introduces a protocol-specific URL (`https://...`) where the checklist prefers protocol-independent URLs (`//...`) when applicable.

## Issue Context
This is a newly added external URL in a test comment.

## Fix Focus Areas
- src/tests/backend/specs/settings.ts[93-93]

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

---

2. Setter test too weak 🐞 Bug ≡ Correctness

Description

The new "setters propagate" test only verifies that assigning to cjs.title changes cjs.title,
which would also pass if the CJS compat layer regressed to copying values onto module.exports
(breaking live updates from reloadSettings()). This can give false confidence and fail to catch
the exact class of regression the test name claims to cover.

Code

src/tests/backend/specs/settings.ts[R139-147]

+    it('setters propagate so reloadSettings() changes are visible to plugins', function () {
+      const cjs = require('../../../node/utils/Settings');
+      const original = cjs.title;
+      try {
+        cjs.title = 'cjs-shim-test';
+        assert.strictEqual(cjs.title, 'cjs-shim-test');
+      } finally {
+        cjs.title = original;
+      }

Evidence

Settings.ts implements CJS compatibility via accessor properties on module.exports that forward
reads/writes to the underlying settings object, ensuring CJS consumers see updates over time. The
test only checks round-tripping a write on the cjs object itself, which does not prove the
property is an accessor or that it forwards to the underlying settings/default export, so it
would not fail if a future refactor used one-time copied values instead of accessors.

src/node/utils/Settings.ts[668-683]
src/tests/backend/specs/settings.ts[139-147]

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 test `setters propagate so reloadSettings() changes are visible to plugins` currently only verifies that `cjs.title` can be assigned and read back. This would pass even if `title` is a plain data property on the CJS wrapper (or if values are copied once onto exports), which would *not* guarantee that future `reloadSettings()` updates are visible to plugin consumers.

### Issue Context
`src/node/utils/Settings.ts` provides CJS compatibility by defining accessor properties on `module.exports` that forward get/set to the internal `settings` object. The regression test should assert this forwarding/live-binding contract, not merely local mutation.

### Fix Focus Areas
- src/tests/backend/specs/settings.ts[139-147]

### Suggested change
Strengthen the test by asserting at least one of:
- `Object.getOwnPropertyDescriptor(cjs, 'title')` has both `get` and `set` functions (and does not have a `value` field), **or**
- When setting `cjs.title`, the underlying default export reflects the change too (e.g., if `cjs.default` exists: `assert.strictEqual(cjs.default.title, 'cjs-shim-test')`), ensuring writes propagate to the underlying settings object.

Keep the existing `try/finally` restore to avoid global state leakage.

ⓘ 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