Skip to content

Add creator-owned pad settings defaults#7545

Open
JohnMcLear wants to merge 7 commits intoether:developfrom
JohnMcLear:feat-pad-vs-user-settings
Open

Add creator-owned pad settings defaults#7545
JohnMcLear wants to merge 7 commits intoether:developfrom
JohnMcLear:feat-pad-vs-user-settings

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

@JohnMcLear JohnMcLear commented Apr 18, 2026

Summary

  • rename the settings popup and split it into User Settings plus creator-only Pad-wide Settings with pad-scoped defaults
  • let users override pad-wide defaults unless enforcement is enabled for other users, so creators can keep private overrides in teacher/pupil-style flows
  • add Disable Chat, the enforced-settings notice, layout refinements, and frontend coverage for creator visibility, precedence, enforcement, override behavior, and chat suppression

Testing

  • pnpm --filter ep_etherpad-lite run ts-check
  • NODE_ENV=production pnpm exec playwright test tests/frontend-new/specs/pad_settings.spec.ts tests/frontend-new/specs/chat.spec.ts tests/frontend-new/specs/font_type.spec.ts tests/frontend-new/specs/language.spec.ts tests/frontend-new/specs/rtl_url_param.spec.ts tests/frontend-new/specs/change_user_color.spec.ts --project=chromium

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add creator-owned pad settings with enforcement and My View defaults

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add creator-owned pad settings with enforcement capability
• Separate user preferences from pad defaults with override logic
• Seed new pads from user's My View preferences
• Implement chat suppression and settings UI controls
Diagram
flowchart LR
  A["Pad Creator"] -->|"Set Pad Settings"| B["PadSettings Storage"]
  B -->|"enforceSettings=true"| C["Enforce on All Users"]
  B -->|"enforceSettings=false"| D["Allow User Overrides"]
  E["User My View Prefs"] -->|"New Pad Creation"| F["Seed Pad Defaults"]
  D -->|"Merge with User Prefs"| G["Effective Options"]
  C -->|"Ignore User Prefs"| G
  G -->|"Apply to UI"| H["Rendered Pad"]
Loading

Grey Divider

File Changes

1. src/node/db/Pad.ts ✨ Enhancement +54/-0

Add pad settings storage and normalization

src/node/db/Pad.ts


2. src/node/handler/PadMessageHandler.ts ✨ Enhancement +44/-2

Handle pad options messages and creator validation

src/node/handler/PadMessageHandler.ts


3. src/static/js/chat.ts ✨ Enhancement +7/-6

Add hideChat check and preference persistence control

src/static/js/chat.ts


View more (9)
4. src/static/js/pad.ts ✨ Enhancement +211/-53

Implement pad vs user settings with effective options logic

src/static/js/pad.ts


5. src/static/js/pad_editor.ts ✨ Enhancement +63/-25

Bind pad and user settings controls to handlers

src/static/js/pad_editor.ts


6. src/static/js/types/SocketIOMessage.ts ✨ Enhancement +4/-3

Update message types for pad settings and defaults

src/static/js/types/SocketIOMessage.ts


7. src/templates/pad.html ✨ Enhancement +57/-3

Add pad settings section and disable chat option

src/templates/pad.html


8. src/locales/en.json 📝 Documentation +3/-0

Add localization strings for new settings

src/locales/en.json


9. src/tests/frontend-new/specs/pad_settings.spec.ts 🧪 Tests +119/-0

Add comprehensive pad settings feature tests

src/tests/frontend-new/specs/pad_settings.spec.ts


10. src/tests/frontend-new/specs/font_type.spec.ts 🧪 Tests +1/-1

Update selector for nice-select dropdown

src/tests/frontend-new/specs/font_type.spec.ts


11. src/tests/frontend-new/specs/language.spec.ts 🧪 Tests +5/-5

Update selectors for open nice-select dropdowns

src/tests/frontend-new/specs/language.spec.ts


12. src/tests/frontend-new/specs/rtl_url_param.spec.ts 🧪 Tests +4/-4

Update RTL test expectations for pad settings

src/tests/frontend-new/specs/rtl_url_param.spec.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 18, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. Pad settings missing feature flag📘 Rule violation ☼ Reliability
Description
The PR introduces creator-owned pad settings (including enforcement and chat suppression) and
changes new-pad defaults, but the new behavior is enabled unconditionally with no feature flag or
default-off gating. This violates the requirement to gate new features behind a disabled-by-default
feature flag to preserve prior default behavior.
Code

src/static/js/pad.ts[R267-271]

sessionID: Cookies.get(`${cp}sessionID`) || Cookies.get('sessionID'),
token,
userInfo,
+    padSettingsDefaults: getMyViewDefaults(),
};
Evidence
PR Compliance ID 6 requires new functionality to be gated behind a feature flag disabled by default.
The client always sends padSettingsDefaults and the server applies it when the pad is first
created, enabling the new pad-scoped settings behavior for everyone without any feature-flag check.

src/static/js/pad.ts[263-271]
src/node/handler/PadMessageHandler.ts[901-907]
src/templates/pad.html[170-220]
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
Creator-owned pad settings and new-pad default seeding are enabled unconditionally. Compliance requires new features be behind a feature flag that is disabled by default, preserving the old behavior when the flag is off.
## Issue Context
`padSettingsDefaults` is always sent in `CLIENT_READY`, and on first pad creation the server persists these defaults, changing behavior for all deployments.
## Fix Focus Areas
- src/static/js/pad.ts[263-271]
- src/node/handler/PadMessageHandler.ts[901-907]
- src/templates/pad.html[170-220]

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


2. Cookie documentation not updated📘 Rule violation ⚙ Maintainability
Description
The PR changes how the language cookie is set (moved to new My View logic) and introduces new
preference behavior, but repository documentation is not updated accordingly. This makes the
documented cookie behavior inaccurate for users/admins.
Code

src/static/js/pad.ts[R546-550]

+  setMyViewLanguage: (lang) => {
+    const cp = (window as any).clientVars?.cookiePrefix || '';
+    Cookies.set(`${cp}language`, lang);
+    pad.refreshMyViewControls();
+    pad.applyOptionsChange();
Evidence
PR Compliance ID 1 requires documentation updates in the same PR for user-facing/config behavior
changes. doc/cookies.md states the language cookie is set in pad_editor.js, but the PR removes
that path and sets it via pad.setMyViewLanguage() in pad.ts instead, without updating the docs.

doc/cookies.md[5-10]
src/static/js/pad.ts[546-550]
src/static/js/pad_editor.ts[58-82]
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
Documentation about cookies is now inaccurate due to the changes in where/how the `language` cookie is set and the new My View preference behavior.
## Issue Context
`doc/cookies.md` currently points readers to the old implementation location for `language`. The PR changes the implementation but does not update docs.
## Fix Focus Areas
- doc/cookies.md[5-10]
- src/static/js/pad.ts[546-550]
- src/static/js/pad_editor.ts[58-82]

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


3. Pad creator can be lost🐞 Bug ⛨ Security
Description
handlePadOptionsMessage() calls padManager.getPad(session.padId) without passing the session
authorId, so if a padoptions message is processed before the normal pad creation path completes, the
pad can be created with revision 0 author set to '' and isPadCreator() will fail for everyone
(locking pad settings/delete).
Code

src/node/handler/PadMessageHandler.ts[R269-274]

+const handlePadOptionsMessage = async (
+    socket: any, message: PadOptionsMessage & {data: {payload: PadOptionsMessage}}) => {
+  const session = sessioninfos[socket.id];
+  if (!session || !session.author || !session.padId) throw new Error('session not ready');
+  const pad = await padManager.getPad(session.padId);
+  if (!await isPadCreator(pad, session.author)) {
Evidence
The pad settings handler loads/creates the pad without an authorId, but PadManager defaults authorId
to '' when creating a new pad. Creator checks are based on revision 0 author, so an early-created
pad will have no recognized creator.

src/node/handler/PadMessageHandler.ts[267-295]
src/node/db/PadManager.ts[109-144]
src/node/db/Pad.ts[437-458]
src/node/db/Pad.ts[227-230]

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

## Issue description
`handlePadOptionsMessage()` calls `padManager.getPad(session.padId)` without providing `session.author`. If the pad does not yet exist at that moment, `PadManager.getPad()` will create it with the default `authorId = ''`, which makes revision 0’s author empty. Because creator checks rely on revision 0’s author, the pad can end up with no valid creator.
### Issue Context
- Creator is determined via `pad.getRevisionAuthor(0)`.
- `PadManager.getPad()` uses `authorId = ''` by default.
### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[267-295]
- src/node/db/PadManager.ts[109-144]
### What to change
- In `handlePadOptionsMessage()`, avoid implicitly creating a pad with an empty author:
- Option A (preferred): Check existence first (`doesPadExist`). If it does not exist, reject the message (or defer until after CLIENT_READY completes).
- Option B: Call `padManager.getPad(session.padId, null, session.author)` so any accidental creation still assigns the correct creator.
- Keep the creator authorization check, but ensure the pad is not created as a side effect of unauthorized/early messages.

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


View more (1)
4. Defaults override server config🐞 Bug ≡ Correctness
Description
getMyViewDefaults() hard-codes fallback defaults (e.g., showChat/showLineNumbers true) when cookies
are absent, and these values are sent as padSettingsDefaults for new pads, overriding server-side
defaults such as settings.padOptions.showChat/showLineNumbers.
Code

src/static/js/pad.ts[R205-218]

+const getMyViewDefaults = () => {
+  const overrides = getMyViewOverrides();
+  return {
+    showChat: overrides.showChat == null ? true : overrides.showChat,
+    alwaysShowChat: overrides.alwaysShowChat === true,
+    chatAndUsers: overrides.chatAndUsers === true,
+    lang: overrides.lang || 'en',
+    view: {
+      showAuthorColors: overrides.view.showAuthorColors == null ? true : overrides.view.showAuthorColors,
+      showLineNumbers: overrides.view.showLineNumbers == null ? true : overrides.view.showLineNumbers,
+      rtlIsTrue: overrides.view.rtlIsTrue === true,
+      padFontFamily: overrides.view.padFontFamily || '',
+    },
+  };
Evidence
The client computes padSettingsDefaults with true fallbacks and sends them during CLIENT_READY. The
server’s normalization uses settings.padOptions.* as the default only when the raw value is
null/undefined, but because the client always supplies explicit booleans, the server defaults are
bypassed for newly created pads.

src/static/js/pad.ts[205-218]
src/static/js/pad.ts[235-271]
src/node/handler/PadMessageHandler.ts[863-907]
src/node/db/Pad.ts[87-103]

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

## Issue description
New pads are seeded from `padSettingsDefaults`, but the client computes these via `getMyViewDefaults()` which uses hard-coded fallbacks like `showChat: true` and `showLineNumbers: true` when cookies are unset. This makes newly created pad defaults override server/admin defaults (e.g., `settings.padOptions.showChat`), because the server only applies its defaults when the incoming value is null/undefined.
### Issue Context
- Server defaulting behavior is in `Pad.normalizePadSettings()`.
- Client sends `padSettingsDefaults` in `CLIENT_READY`.
### Fix Focus Areas
- src/static/js/pad.ts[187-271]
- src/node/handler/PadMessageHandler.ts[898-907]
- src/node/db/Pad.ts[87-103]
### What to change
- Replace `padSettingsDefaults: getMyViewDefaults()` with a payload that only includes **explicit user overrides** (no hard-coded fallbacks). For example:
- Send `getMyViewOverrides()` instead of `getMyViewDefaults()`.
- Ensure any `undefined`/`null` values are omitted so the server can apply `settings.padOptions.*` defaults.
- Optionally, add a server-side guard when applying `padSettingsDefaults` to treat missing values as “unset” (so server defaults win) rather than trusting client fallbacks.

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



Remediation recommended

5. Options precedence lacks comments 📘 Rule violation ⚙ Maintainability
Description
The new effective-options precedence and normalization logic is non-obvious (pad defaults vs My View
overrides vs enforcement) but is introduced without explanatory comments. This increases maintenance
risk and makes future changes more error-prone.
Code

src/static/js/pad.ts[R472-484]

+  getEffectivePadOptions: () => {
+    const effectiveOptions = $.extend(true, {}, pad.padOptions);
+    if (pad.padOptions.enforceSettings) return normalizeChatOptions(effectiveOptions);
+    const overrides = getMyViewOverrides();
+    for (const key of ['showChat', 'alwaysShowChat', 'chatAndUsers', 'lang']) {
+      if (overrides[key] != null) effectiveOptions[key] = overrides[key];
+    }
+    if (!effectiveOptions.view) effectiveOptions.view = {};
+    for (const [key, value] of Object.entries(overrides.view)) {
+      if (value != null) effectiveOptions.view[key] = value;
+    }
+    return normalizeChatOptions(effectiveOptions);
+  },
Evidence
PR Compliance ID 9 requires comments for complex or non-obvious logic. The added code implements
multi-source settings precedence and special-case normalization for chat-related options without
documenting intended precedence rules or rationale.

src/static/js/pad.ts[472-484]
src/static/js/pad.ts[221-233]
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 precedence rules for `enforceSettings` vs My View overrides vs pad defaults (and the chat normalization side-effects) are not obvious from the code alone.
## Issue Context
Future maintainers may break enforcement/override behavior if the intended precedence and invariants are not documented.
## Fix Focus Areas
- src/static/js/pad.ts[472-484]
- src/static/js/pad.ts[221-233]

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


Grey Divider

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

Grey Divider

Qodo Logo

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread src/static/js/pad.ts
Comment thread src/static/js/pad.ts
Comment thread src/node/handler/PadMessageHandler.ts
Comment thread src/static/js/pad.ts Outdated
JohnMcLear and others added 5 commits April 18, 2026 14:37
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JohnMcLear JohnMcLear requested a review from SamTV12345 April 18, 2026 15:53
@@ -1005,6 +1050,8 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => {

// Warning: never ever send sessionInfo.padId to the client. If the client is read only you
// would open a security hole 1 swedish mile wide...
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) Great comment

Comment thread src/static/js/pad.ts
};

const msg = {
const msg: any = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does any make sense here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants