Skip to content

feat(colors): add padOptions.fadeInactiveAuthorColors toggle#7554

Open
JohnMcLear wants to merge 2 commits intoether:developfrom
JohnMcLear:feat/disable-inactive-color-fade-7138
Open

feat(colors): add padOptions.fadeInactiveAuthorColors toggle#7554
JohnMcLear wants to merge 2 commits intoether:developfrom
JohnMcLear:feat/disable-inactive-color-fade-7138

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

Adds a new pad option fadeInactiveAuthorColors (default true). When false, each author's background color stays at their chosen value instead of fading toward white as they go inactive.

Configurable via:

  • settings.jsonpadOptions.fadeInactiveAuthorColors
  • Docker env var PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS
  • Per-pad URL parameter ?fadeInactiveAuthorColors=false

Motivation (from issue): a user who picks a light color becomes visually indistinguishable after the fade, and switching machines multiplies the effective color count because each session fades its own near-white variant — "Zeno's white".

Default is true so existing deployments are unchanged.

Closes #7138

Test plan

  • Backend: padOptions.fadeInactiveAuthorColors default is true (new test in src/tests/backend/specs/settings.ts)
  • Playwright: clientVars.padOptions.fadeInactiveAuthorColors defaults to true
  • Playwright: URL override ?fadeInactiveAuthorColors=false flips clientVars.padOptions.fadeInactiveAuthorColors to false
  • pnpm run ts-check clean locally

🤖 Generated with Claude Code

Adds a new pad option fadeInactiveAuthorColors (default true). When false,
each author's background color stays at their chosen value instead of
fading toward white as the author goes inactive. Settable server-side via
settings.json / settings.json.docker and per-pad via the
?fadeInactiveAuthorColors=false URL parameter.

Motivation (from issue): a user who picks a light color can become
visually indistinguishable after the fade, and switching machines
multiplies the effective color count because each session fades its own
near-white variant.

Default is unchanged (true) so existing deployments keep legacy behavior.

Closes ether#7138

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add fadeInactiveAuthorColors option to control inactive author color fading

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add padOptions.fadeInactiveAuthorColors toggle to control author color fade behavior
• Default true preserves existing fade-on-inactive behavior for backward compatibility
• Configurable via settings.json, Docker env var, and URL parameter
• Solves issue where light colors become indistinguishable after fade
Diagram
flowchart LR
  A["Settings Configuration"] -->|"padOptions.fadeInactiveAuthorColors"| B["Backend Settings"]
  C["Docker Environment"] -->|"PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS"| B
  D["URL Parameter"] -->|"?fadeInactiveAuthorColors=false"| E["Client Variables"]
  B -->|"clientVars.padOptions"| E
  E -->|"Controls fade logic"| F["Ace2Inner Color Rendering"]
  F -->|"Applies or skips fade"| G["Author Background Color"]
Loading

Grey Divider

File Changes

1. src/node/utils/Settings.ts ⚙️ Configuration changes +2/-0

Add fadeInactiveAuthorColors to Settings type

• Added fadeInactiveAuthorColors: boolean field to padOptions type definition
• Set default value to true in settings object to preserve legacy behavior

src/node/utils/Settings.ts


2. src/static/js/ace2_inner.ts ✨ Enhancement +7/-1

Conditionally apply color fade based on setting

• Added logic to check clientVars.padOptions.fadeInactiveAuthorColors setting
• Conditionally applies color fade only when setting is enabled (default true)
• Includes explanatory comment about the feature and its motivation

src/static/js/ace2_inner.ts


3. src/static/js/pad.ts ✨ Enhancement +7/-0

Add URL parameter support for fadeInactiveAuthorColors

• Added new URL parameter handler for fadeInactiveAuthorColors
• Checks for ?fadeInactiveAuthorColors=false and updates clientVars.padOptions
• Allows per-pad override of the fade behavior

src/static/js/pad.ts


View more (4)
4. src/tests/backend/specs/settings.ts 🧪 Tests +11/-0

Add regression test for fadeInactiveAuthorColors default

• Added regression test for issue #7138
• Verifies padOptions.fadeInactiveAuthorColors defaults to true
• Ensures backward compatibility with existing deployments

src/tests/backend/specs/settings.ts


5. src/tests/frontend-new/specs/inactive_color_fade.spec.ts 🧪 Tests +22/-0

Add Playwright tests for fadeInactiveAuthorColors feature

• New Playwright test file for fadeInactiveAuthorColors feature
• Tests that setting defaults to true (legacy behavior preserved)
• Tests that URL parameter ?fadeInactiveAuthorColors=false disables fade

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


6. settings.json.docker ⚙️ Configuration changes +2/-1

Add fadeInactiveAuthorColors to Docker settings

• Added fadeInactiveAuthorColors field to padOptions configuration
• Uses Docker environment variable PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS with default true
• Allows Docker deployments to configure the fade behavior

settings.json.docker


7. settings.json.template ⚙️ Configuration changes +7/-1

Add fadeInactiveAuthorColors to template settings

• Added fadeInactiveAuthorColors field to padOptions configuration
• Set default value to true
• Included explanatory comment about the feature's purpose and use case

settings.json.template


Grey Divider

Qodo Logo

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

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

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. https:// URL in comment 📘 Rule violation ⚙ Maintainability
Description
A protocol-specific https:// URL was introduced in a code comment, which conflicts with the
project requirement to use protocol-independent URLs where appropriate. This can create inconsistent
documentation style and mixed-content concerns if copied into contexts expecting protocol-relative
links.
Code

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

+  // Regression test for https://github.com/ether/etherpad/issues/7138.
Evidence
PR Compliance ID 10 requires protocol-independent URLs instead of hardcoding http:// or
https://. The added regression test comment includes a hardcoded https://github.com/... URL.

src/tests/backend/specs/settings.ts[151-151]
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 new comment includes a hardcoded `https://` URL, but the project compliance rule requires protocol-independent URLs where appropriate.
## Issue Context
The added regression test comment references issue #7138 with an `https://github.com/...` link.
## Fix Focus Areas
- src/tests/backend/specs/settings.ts[151-151]

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


2. Unused browser context🐞 Bug ☼ Reliability
Description
The new Playwright spec creates a new BrowserContext and clears its cookies, but then navigates with
the existing page fixture, so cookie clearing has no effect and the new context is never closed.
This can leak contexts across tests and degrade test reliability/performance.
Code

src/tests/frontend-new/specs/inactive_color_fade.spec.ts[R4-8]

+test.beforeEach(async ({page, browser}) => {
+  const context = await browser.newContext();
+  await context.clearCookies();
+  await goToNewPad(page);
+});
Evidence
inactive_color_fade.spec.ts creates a new context and clears cookies on it, but does not create a
page from that context; instead it calls goToNewPad(page) which navigates the existing page
fixture. Therefore the new context is unused and unclosed, and cookie clearing does not affect the
actual test page.

src/tests/frontend-new/specs/inactive_color_fade.spec.ts[4-8]
src/tests/frontend-new/helper/padHelper.ts[117-123]

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 creates a new `BrowserContext` and clears cookies on it, but uses the existing `page` fixture for navigation, so cookie clearing is ineffective and the new context is never closed.
### Issue Context
The intent seems to be to start each test with a clean cookie jar. In Playwright, the `page` fixture already has a context (`page.context()`), so cookie operations should target that context unless you also create a new page from the new context.
### Fix Focus Areas
- src/tests/frontend-new/specs/inactive_color_fade.spec.ts[4-8]
### Suggested change
Replace the `browser.newContext()` usage with one of:
1) **Simplest:** `await page.context().clearCookies();`
2) If you truly need a new context, create a `page = await context.newPage()` and use that page (and ensure `await context.close()` in `afterEach`).

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



Advisory comments

3. Docker docs missing env var🐞 Bug ⚙ Maintainability
Description
settings.json.docker adds the new env var PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS, but the
Docker documentation’s Pad Options tables do not list it. Users relying on docs will not discover
how to configure the new toggle via environment variables.
Code

settings.json.docker[R291-292]

+    "lang":             "${PAD_OPTIONS_LANG:null}",
+    "fadeInactiveAuthorColors": "${PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS:true}"
Evidence
The Docker settings file now supports configuring the new option via
PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS, but the published Docker documentation still lists
PAD_OPTIONS_LANG as the last Pad Options env var, omitting the new one.

settings.json.docker[280-293]
doc/docker.md[97-112]
doc/docker.adoc[179-233]

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

## Issue description
`settings.json.docker` supports `PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS`, but the Docker docs do not list this env var under Pad Options.
### Issue Context
This PR explicitly advertises Docker env var configurability; the docs should reflect the new supported variable to avoid confusion.
### Fix Focus Areas
- doc/docker.md[97-112]
- doc/docker.adoc[179-233]
### Suggested change
Add a row for `PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS` (default `true`) in both Pad Options tables, with a short description (e.g., "When false, do not fade inactive author colors toward white").

ⓘ 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

Addresses Qodo review feedback on ether#7554:

1. doc/docker.md's Pad Options env-var table was missing
   PAD_OPTIONS_FADE_INACTIVE_AUTHOR_COLORS. Users following the docs
   had no way to discover the new toggle — add the entry with its
   description and default.

2. The Playwright spec's `browser.newContext() + clearCookies()` pattern
   was ineffective — tests navigate with the `page` fixture from the
   default context, so clearing cookies on a separate context is a
   no-op and the context leaks. Replace with
   `page.context().clearCookies()`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

Add ability to disable inactive color changing

1 participant