fix: RTL URL parameter rtl=false now correctly disables RTL mode by JohnMcLear · Pull Request #7464 · ether/etherpad

JohnMcLear · 2026-04-05T09:22:03Z

Summary

The ?rtl=false URL parameter was ignored — once RTL was enabled (via ?rtl=true or cookie), it couldn't be disabled via URL.

Root Cause

The rtl parameter in getParameters had checkVal: 'true', meaning the callback only fired when the value was exactly 'true'. When rtl=false was passed, the callback didn't run and the setting stayed at whatever the cookie had.

Also, changeViewOption('rtlIsTrue', ...) was only called when true, never when false, so even if the setting was correctly set to false, the view wasn't updated.

Fix

Changed checkVal to null so the callback fires for any value
Callback now sets rtlIsTrue = (val === 'true') — explicitly false for non-true values
changeViewOption is always called with the current setting, not conditionally

Test plan

Type check passes
Backend tests pass (746/746)

Fixes #5559

🤖 Generated with Claude Code

The rtl parameter callback only handled rtl=true (checkVal was 'true'), so rtl=false was ignored and the layout stayed in RTL from the cookie. Now accepts any value and sets rtlIsTrue = (val === 'true'). Also always applies the RTL setting instead of only when true, so switching from rtl=true to rtl=false takes effect. Fixes ether#5559 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

JohnMcLear · 2026-04-05T09:22:05Z

/review

qodo-free-for-open-source-projects · 2026-04-05T09:22:12Z

Code Review by Qodo

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

1. rtl fix lacks test 📘 Rule violation ☼ Reliability

Description

This PR fixes a user-facing regression in the rtl URL parameter handling but does not include an
automated regression test to prevent reintroduction. Without a test that covers ?rtl=true then
?rtl=false, the bug can easily return unnoticed.

Code

src/static/js/pad.ts[R121-125]

name: 'rtl',
-    checkVal: 'true',
+    checkVal: null,
callback: (val) => {
-      settings.rtlIsTrue = true;
+      settings.rtlIsTrue = val === 'true';
},

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test. The changed code in
src/static/js/pad.ts implements the ?rtl=false behavior fix, but the provided PR diff contains
no accompanying test changes to validate the new behavior.

src/static/js/pad.ts[121-125]
src/static/js/pad.ts[554-554]
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 bug fix was made to ensure `?rtl=false` disables RTL mode, but there is no automated regression test to ensure the behavior remains correct.
## Issue Context
The fixed behavior is: after enabling RTL (via URL/cookie), loading with `?rtl=false` must switch back to LTR (and not remain stuck in RTL).
## Fix Focus Areas
- src/tests/frontend-new/specs/language.spec.ts[51-87]
- src/tests/frontend-new/helper/padHelper.ts[107-130]
- src/static/js/pad.ts[121-126]
- src/static/js/pad.ts[548-559]

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

2. ~~RTL preference forced to false~~ ☑ 🐞 Bug ≡ Correctness

Description

pad._afterHandshake() now always calls changeViewOption('rtlIsTrue', false) for the common case
where settings.rtlIsTrue is false, which overwrites any existing rtlIsTrue cookie and forces LTR
even when RTL should come from cookie or language direction. This can break persisted RTL mode and
prevent RTL-by-language defaults from taking effect unless the URL explicitly enables RTL.

Code

src/static/js/pad.ts[554]

+    this.changeViewOption('rtlIsTrue', settings.rtlIsTrue === true);

Evidence

getParams() applies the server default padOptions.rtl (which is present and defaults to
false), leaving settings.rtlIsTrue false in the normal case. _afterHandshake() then
unconditionally calls changeViewOption('rtlIsTrue', false), and handleOptionsChange() persists
that value to the prefs cookie via padcookie.setPref(), overwriting any previously saved
rtlIsTrue=true. Later, postAceInit() only reapplies the RTL cookie when it is true, so after
being overwritten to false it will not restore RTL. Also, the editor’s RTL default falls back to
html10n.getDirection() only when the view option is unset, so forcing rtlIsTrue=false suppresses
that default behavior.

src/node/utils/Settings.ts[395-410]
src/static/js/pad.ts[153-173]
src/static/js/pad.ts[429-556]
src/static/js/pad.ts[598-617]
src/static/js/pad_cookie.ts[64-72]
src/static/js/pad.ts[446-471]
src/static/js/pad_editor.ts[153-165]

Agent prompt

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

## Issue description
`pad._afterHandshake()` now unconditionally calls `changeViewOption('rtlIsTrue', settings.rtlIsTrue === true)`. Because `settings.rtlIsTrue` is typically `false` (server defaults include `padOptions.rtl: false`), this writes `rtlIsTrue=false` into view options and persists it to the prefs cookie, overwriting an existing `rtlIsTrue=true` cookie and suppressing RTL-by-language defaults.
## Issue Context
- `changeViewOption()` flows into `handleOptionsChange()`, which persists view options to the prefs cookie via `padcookie.setPref()`.
- Cookie restore logic for RTL only re-applies when the cookie is `true`, so overwriting it to `false` prevents restore.
## Fix focus areas
- Only apply `rtlIsTrue` view option when there is an explicit override (e.g., URL contains `rtl`, or a server config explicitly enables RTL). Avoid calling `changeViewOption('rtlIsTrue', false)` as a default.
- Consider using presence checks (`URLSearchParams.has('rtl')`) or a tri-state/flag to distinguish “unset” from “explicit false”.
### Suggested approach
1. In `_afterHandshake()`, replace the unconditional call with logic like:
- If `getUrlVars().has('rtl')`, set `rtlIsTrue` based on the URL value (`params.get('rtl') === 'true'`).
- Else if server config explicitly sets RTL on (e.g., `clientVars.padOptions.rtl === true`), set it to `true`.
- Else do nothing so cookie and `html10n.getDirection()` defaults can apply.
## Fix Focus Areas (exact locations)
- src/static/js/pad.ts[153-173]
- src/static/js/pad.ts[429-556]
- src/static/js/pad.ts[598-617]

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

3. rtl fix lacks test 📘 Rule violation ☼ Reliability

Description

This PR fixes a user-facing regression in the rtl URL parameter handling but does not include an
automated regression test to prevent reintroduction. Without a test that covers ?rtl=true then
?rtl=false, the bug can easily return unnoticed.

Code

src/static/js/pad.ts[R121-125]

 name: 'rtl',
-    checkVal: 'true',
+    checkVal: null,
 callback: (val) => {
-      settings.rtlIsTrue = true;
+      settings.rtlIsTrue = val === 'true';
 },

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test. The changed code in
src/static/js/pad.ts implements the ?rtl=false behavior fix, but the provided PR diff contains
no accompanying test changes to validate the new behavior.

src/static/js/pad.ts[121-125]
src/static/js/pad.ts[554-554]
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 bug fix was made to ensure `?rtl=false` disables RTL mode, but there is no automated regression test to ensure the behavior remains correct.
## Issue Context
The fixed behavior is: after enabling RTL (via URL/cookie), loading with `?rtl=false` must switch back to LTR (and not remain stuck in RTL).
## Fix Focus Areas
- src/tests/frontend-new/specs/language.spec.ts[51-87]
- src/tests/frontend-new/helper/padHelper.ts[107-130]
- src/static/js/pad.ts[121-126]
- src/static/js/pad.ts[548-559]

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

View more (5)

4. ~~RTL preference forced to false~~ ☑ 🐞 Bug ≡ Correctness

Description

pad._afterHandshake() now always calls changeViewOption('rtlIsTrue', false) for the common case
where settings.rtlIsTrue is false, which overwrites any existing rtlIsTrue cookie and forces LTR
even when RTL should come from cookie or language direction. This can break persisted RTL mode and
prevent RTL-by-language defaults from taking effect unless the URL explicitly enables RTL.

Code

src/static/js/pad.ts[554]

+    this.changeViewOption('rtlIsTrue', settings.rtlIsTrue === true);

Evidence

getParams() applies the server default padOptions.rtl (which is present and defaults to
false), leaving settings.rtlIsTrue false in the normal case. _afterHandshake() then
unconditionally calls changeViewOption('rtlIsTrue', false), and handleOptionsChange() persists
that value to the prefs cookie via padcookie.setPref(), overwriting any previously saved
rtlIsTrue=true. Later, postAceInit() only reapplies the RTL cookie when it is true, so after
being overwritten to false it will not restore RTL. Also, the editor’s RTL default falls back to
html10n.getDirection() only when the view option is unset, so forcing rtlIsTrue=false suppresses
that default behavior.

src/node/utils/Settings.ts[395-410]
src/static/js/pad.ts[153-173]
src/static/js/pad.ts[429-556]
src/static/js/pad.ts[598-617]
src/static/js/pad_cookie.ts[64-72]
src/static/js/pad.ts[446-471]
src/static/js/pad_editor.ts[153-165]

Agent prompt

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

## Issue description
`pad._afterHandshake()` now unconditionally calls `changeViewOption('rtlIsTrue', settings.rtlIsTrue === true)`. Because `settings.rtlIsTrue` is typically `false` (server defaults include `padOptions.rtl: false`), this writes `rtlIsTrue=false` into view options and persists it to the prefs cookie, overwriting an existing `rtlIsTrue=true` cookie and suppressing RTL-by-language defaults.
## Issue Context
- `changeViewOption()` flows into `handleOptionsChange()`, which persists view options to the prefs cookie via `padcookie.setPref()`.
- Cookie restore logic for RTL only re-applies when the cookie is `true`, so overwriting it to `false` prevents restore.
## Fix focus areas
- Only apply `rtlIsTrue` view option when there is an explicit override (e.g., URL contains `rtl`, or a server config explicitly enables RTL). Avoid calling `changeViewOption('rtlIsTrue', false)` as a default.
- Consider using presence checks (`URLSearchParams.has('rtl')`) or a tri-state/flag to distinguish “unset” from “explicit false”.
### Suggested approach
1. In `_afterHandshake()`, replace the unconditional call with logic like:
- If `getUrlVars().has('rtl')`, set `rtlIsTrue` based on the URL value (`params.get('rtl') === 'true'`).
- Else if server config explicitly sets RTL on (e.g., `clientVars.padOptions.rtl === true`), set it to `true`.
- Else do nothing so cookie and `html10n.getDirection()` defaults can apply.
## Fix Focus Areas (exact locations)
- src/static/js/pad.ts[153-173]
- src/static/js/pad.ts[429-556]
- src/static/js/pad.ts[598-617]

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

5. rtl fix lacks test 📘 Rule violation ☼ Reliability

Description

This PR fixes a user-facing regression in the rtl URL parameter handling but does not include an
automated regression test to prevent reintroduction. Without a test that covers ?rtl=true then
?rtl=false, the bug can easily return unnoticed.

Code

src/static/js/pad.ts[R121-125]

  name: 'rtl',
-    checkVal: 'true',
+    checkVal: null,
  callback: (val) => {
-      settings.rtlIsTrue = true;
+      settings.rtlIsTrue = val === 'true';
  },

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test. The changed code in
src/static/js/pad.ts implements the ?rtl=false behavior fix, but the provided PR diff contains
no accompanying test changes to validate the new behavior.

src/static/js/pad.ts[121-125]
src/static/js/pad.ts[554-554]
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 bug fix was made to ensure `?rtl=false` disables RTL mode, but there is no automated regression test to ensure the behavior remains correct.
## Issue Context
The fixed behavior is: after enabling RTL (via URL/cookie), loading with `?rtl=false` must switch back to LTR (and not remain stuck in RTL).
## Fix Focus Areas
- src/tests/frontend-new/specs/language.spec.ts[51-87]
- src/tests/frontend-new/helper/padHelper.ts[107-130]
- src/static/js/pad.ts[121-126]
- src/static/js/pad.ts[548-559]

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

6. ~~RTL preference forced to false~~ ☑ 🐞 Bug ≡ Correctness

Description

pad._afterHandshake() now always calls changeViewOption('rtlIsTrue', false) for the common case
where settings.rtlIsTrue is false, which overwrites any existing rtlIsTrue cookie and forces LTR
even when RTL should come from cookie or language direction. This can break persisted RTL mode and
prevent RTL-by-language defaults from taking effect unless the URL explicitly enables RTL.

Code

src/static/js/pad.ts[554]

+    this.changeViewOption('rtlIsTrue', settings.rtlIsTrue === true);

Evidence

getParams() applies the server default padOptions.rtl (which is present and defaults to
false), leaving settings.rtlIsTrue false in the normal case. _afterHandshake() then
unconditionally calls changeViewOption('rtlIsTrue', false), and handleOptionsChange() persists
that value to the prefs cookie via padcookie.setPref(), overwriting any previously saved
rtlIsTrue=true. Later, postAceInit() only reapplies the RTL cookie when it is true, so after
being overwritten to false it will not restore RTL. Also, the editor’s RTL default falls back to
html10n.getDirection() only when the view option is unset, so forcing rtlIsTrue=false suppresses
that default behavior.

src/node/utils/Settings.ts[395-410]
src/static/js/pad.ts[153-173]
src/static/js/pad.ts[429-556]
src/static/js/pad.ts[598-617]
src/static/js/pad_cookie.ts[64-72]
src/static/js/pad.ts[446-471]
src/static/js/pad_editor.ts[153-165]

Agent prompt

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

## Issue description
`pad._afterHandshake()` now unconditionally calls `changeViewOption('rtlIsTrue', settings.rtlIsTrue === true)`. Because `settings.rtlIsTrue` is typically `false` (server defaults include `padOptions.rtl: false`), this writes `rtlIsTrue=false` into view options and persists it to the prefs cookie, overwriting an existing `rtlIsTrue=true` cookie and suppressing RTL-by-language defaults.
## Issue Context
- `changeViewOption()` flows into `handleOptionsChange()`, which persists view options to the prefs cookie via `padcookie.setPref()`.
- Cookie restore logic for RTL only re-applies when the cookie is `true`, so overwriting it to `false` prevents restore.
## Fix focus areas
- Only apply `rtlIsTrue` view option when there is an explicit override (e.g., URL contains `rtl`, or a server config explicitly enables RTL). Avoid calling `changeViewOption('rtlIsTrue', false)` as a default.
- Consider using presence checks (`URLSearchParams.has('rtl')`) or a tri-state/flag to distinguish “unset” from “explicit false”.
### Suggested approach
1. In `_afterHandshake()`, replace the unconditional call with logic like:
- If `getUrlVars().has('rtl')`, set `rtlIsTrue` based on the URL value (`params.get('rtl') === 'true'`).
- Else if server config explicitly sets RTL on (e.g., `clientVars.padOptions.rtl === true`), set it to `true`.
- Else do nothing so cookie and `html10n.getDirection()` defaults can apply.
## Fix Focus Areas (exact locations)
- src/static/js/pad.ts[153-173]
- src/static/js/pad.ts[429-556]
- src/static/js/pad.ts[598-617]

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

7. rtl fix lacks test 📘 Rule violation ☼ Reliability

Description

This PR fixes a user-facing regression in the rtl URL parameter handling but does not include an
automated regression test to prevent reintroduction. Without a test that covers ?rtl=true then
?rtl=false, the bug can easily return unnoticed.

Code

src/static/js/pad.ts[R121-125]

   name: 'rtl',
-    checkVal: 'true',
+    checkVal: null,
   callback: (val) => {
-      settings.rtlIsTrue = true;
+      settings.rtlIsTrue = val === 'true';
   },

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test. The changed code in
src/static/js/pad.ts implements the ?rtl=false behavior fix, but the provided PR diff contains
no accompanying test changes to validate the new behavior.

src/static/js/pad.ts[121-125]
src/static/js/pad.ts[554-554]
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 bug fix was made to ensure `?rtl=false` disables RTL mode, but there is no automated regression test to ensure the behavior remains correct.
## Issue Context
The fixed behavior is: after enabling RTL (via URL/cookie), loading with `?rtl=false` must switch back to LTR (and not remain stuck in RTL).
## Fix Focus Areas
- src/tests/frontend-new/specs/language.spec.ts[51-87]
- src/tests/frontend-new/helper/padHelper.ts[107-130]
- src/static/js/pad.ts[121-126]
- src/static/js/pad.ts[548-559]

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

8. ~~RTL preference forced to false~~ ☑ 🐞 Bug ≡ Correctness

Description

pad._afterHandshake() now always calls changeViewOption('rtlIsTrue', false) for the common case
where settings.rtlIsTrue is false, which overwrites any existing rtlIsTrue cookie and forces LTR
even when RTL should come from cookie or language direction. This can break persisted RTL mode and
prevent RTL-by-language defaults from taking effect unless the URL explicitly enables RTL.

Code

src/static/js/pad.ts[554]

+    this.changeViewOption('rtlIsTrue', settings.rtlIsTrue === true);

Evidence

getParams() applies the server default padOptions.rtl (which is present and defaults to
false), leaving settings.rtlIsTrue false in the normal case. _afterHandshake() then
unconditionally calls changeViewOption('rtlIsTrue', false), and handleOptionsChange() persists
that value to the prefs cookie via padcookie.setPref(), overwriting any previously saved
rtlIsTrue=true. Later, postAceInit() only reapplies the RTL cookie when it is true, so after
being overwritten to false it will not restore RTL. Also, the editor’s RTL default falls back to
html10n.getDirection() only when the view option is unset, so forcing rtlIsTrue=false suppresses
that default behavior.

src/node/utils/Settings.ts[395-410]
src/static/js/pad.ts[153-173]
src/static/js/pad.ts[429-556]
src/static/js/pad.ts[598-617]
src/static/js/pad_cookie.ts[64-72]
src/static/js/pad.ts[446-471]
src/static/js/pad_editor.ts[153-165]

Agent prompt

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

## Issue description
`pad._afterHandshake()` now unconditionally calls `changeViewOption('rtlIsTrue', settings.rtlIsTrue === true)`. Because `settings.rtlIsTrue` is typically `false` (server defaults include `padOptions.rtl: false`), this writes `rtlIsTrue=false` into view options and persists it to the prefs cookie, overwriting an existing `rtlIsTrue=true` cookie and suppressing RTL-by-language defaults.
## Issue Context
- `changeViewOption()` flows into `handleOptionsChange()`, which persists view options to the prefs cookie via `padcookie.setPref()`.
- Cookie restore logic for RTL only re-applies when the cookie is `true`, so overwriting it to `false` prevents restore.
## Fix focus areas
- Only apply `rtlIsTrue` view option when there is an explicit override (e.g., URL contains `rtl`, or a server config explicitly enables RTL). Avoid calling `changeViewOption('rtlIsTrue', false)` as a default.
- Consider using presence checks (`URLSearchParams.has('rtl')`) or a tri-state/flag to distinguish “unset” from “explicit false”.
### Suggested approach
1. In `_afterHandshake()`, replace the unconditional call with logic like:
 - If `getUrlVars().has('rtl')`, set `rtlIsTrue` based on the URL value (`params.get('rtl') === 'true'`).
 - Else if server config explicitly sets RTL on (e.g., `clientVars.padOptions.rtl === true`), set it to `true`.
 - Else do nothing so cookie and `html10n.getDirection()` defaults can apply.
## Fix Focus Areas (exact locations)
- src/static/js/pad.ts[153-173]
- src/static/js/pad.ts[429-556]
- src/static/js/pad.ts[598-617]

ⓘ 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 · 2026-04-05T09:22:15Z

Review Summary by Qodo

Fix RTL URL parameter rtl=false disabling RTL mode correctly

🐞 Bug fix

Walkthroughs

Description

• Fix RTL URL parameter rtl=false being ignored after RTL enabled
• Changed callback to accept any value, not just 'true'
• Explicitly set rtlIsTrue based on parameter value equality
• Always apply RTL setting to view, not conditionally

Diagram

flowchart LR
  A["rtl URL parameter"] -->|checkVal null| B["Callback fires for any value"]
  B -->|val === 'true'| C["rtlIsTrue set correctly"]
  C -->|Always call| D["changeViewOption applied"]
  D -->|Result| E["RTL mode toggles properly"]

File Changes

1. src/static/js/pad.ts 🐞 Bug fix +3/-5

Fix RTL parameter handling and view updates

• Changed rtl parameter checkVal from 'true' to null to accept any value
• Updated callback to set rtlIsTrue = val === 'true' instead of always true
• Simplified RTL view option application to always call changeViewOption with boolean result
• Removed conditional check that only applied RTL when true

src/static/js/pad.ts

qodo-free-for-open-source-projects · 2026-04-05T09:22:16Z

Code Review by Qodo

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

1. rtl fix lacks test 📘 Rule violation ☼ Reliability

Description

This PR fixes a user-facing regression in the rtl URL parameter handling but does not include an
automated regression test to prevent reintroduction. Without a test that covers ?rtl=true then
?rtl=false, the bug can easily return unnoticed.

Code

src/static/js/pad.ts[R121-125]

    name: 'rtl',
-    checkVal: 'true',
+    checkVal: null,
    callback: (val) => {
-      settings.rtlIsTrue = true;
+      settings.rtlIsTrue = val === 'true';
    },

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test. The changed code in
src/static/js/pad.ts implements the ?rtl=false behavior fix, but the provided PR diff contains
no accompanying test changes to validate the new behavior.

src/static/js/pad.ts[121-125]
src/static/js/pad.ts[554-554]
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 bug fix was made to ensure `?rtl=false` disables RTL mode, but there is no automated regression test to ensure the behavior remains correct.

## Issue Context
The fixed behavior is: after enabling RTL (via URL/cookie), loading with `?rtl=false` must switch back to LTR (and not remain stuck in RTL).

## Fix Focus Areas
- src/tests/frontend-new/specs/language.spec.ts[51-87]
- src/tests/frontend-new/helper/padHelper.ts[107-130]
- src/static/js/pad.ts[121-126]
- src/static/js/pad.ts[548-559]

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

2. RTL preference forced to false 🐞 Bug ≡ Correctness

Description

pad._afterHandshake() now always calls changeViewOption('rtlIsTrue', false) for the common case
where settings.rtlIsTrue is false, which overwrites any existing rtlIsTrue cookie and forces LTR
even when RTL should come from cookie or language direction. This can break persisted RTL mode and
prevent RTL-by-language defaults from taking effect unless the URL explicitly enables RTL.

Code

src/static/js/pad.ts[554]

+    this.changeViewOption('rtlIsTrue', settings.rtlIsTrue === true);

Evidence

getParams() applies the server default padOptions.rtl (which is present and defaults to
false), leaving settings.rtlIsTrue false in the normal case. _afterHandshake() then
unconditionally calls changeViewOption('rtlIsTrue', false), and handleOptionsChange() persists
that value to the prefs cookie via padcookie.setPref(), overwriting any previously saved
rtlIsTrue=true. Later, postAceInit() only reapplies the RTL cookie when it is true, so after
being overwritten to false it will not restore RTL. Also, the editor’s RTL default falls back to
html10n.getDirection() only when the view option is unset, so forcing rtlIsTrue=false suppresses
that default behavior.

src/node/utils/Settings.ts[395-410]
src/static/js/pad.ts[153-173]
src/static/js/pad.ts[429-556]
src/static/js/pad.ts[598-617]
src/static/js/pad_cookie.ts[64-72]
src/static/js/pad.ts[446-471]
src/static/js/pad_editor.ts[153-165]

Agent prompt

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

## Issue description
`pad._afterHandshake()` now unconditionally calls `changeViewOption('rtlIsTrue', settings.rtlIsTrue === true)`. Because `settings.rtlIsTrue` is typically `false` (server defaults include `padOptions.rtl: false`), this writes `rtlIsTrue=false` into view options and persists it to the prefs cookie, overwriting an existing `rtlIsTrue=true` cookie and suppressing RTL-by-language defaults.

## Issue Context
- `changeViewOption()` flows into `handleOptionsChange()`, which persists view options to the prefs cookie via `padcookie.setPref()`.
- Cookie restore logic for RTL only re-applies when the cookie is `true`, so overwriting it to `false` prevents restore.

## Fix focus areas
- Only apply `rtlIsTrue` view option when there is an explicit override (e.g., URL contains `rtl`, or a server config explicitly enables RTL). Avoid calling `changeViewOption('rtlIsTrue', false)` as a default.
- Consider using presence checks (`URLSearchParams.has('rtl')`) or a tri-state/flag to distinguish “unset” from “explicit false”.

### Suggested approach
1. In `_afterHandshake()`, replace the unconditional call with logic like:
  - If `getUrlVars().has('rtl')`, set `rtlIsTrue` based on the URL value (`params.get('rtl') === 'true'`).
  - Else if server config explicitly sets RTL on (e.g., `clientVars.padOptions.rtl === true`), set it to `true`.
  - Else do nothing so cookie and `html10n.getDirection()` defaults can apply.

## Fix Focus Areas (exact locations)
- src/static/js/pad.ts[153-173]
- src/static/js/pad.ts[429-556]
- src/static/js/pad.ts[598-617]

ⓘ 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 · 2026-04-05T09:26:07Z

    name: 'rtl',
-    checkVal: 'true',
+    checkVal: null,
    callback: (val) => {
-      settings.rtlIsTrue = true;
+      settings.rtlIsTrue = val === 'true';
    },


1. rtl fix lacks test 📘 Rule violation ☼ Reliability

This PR fixes a user-facing regression in the rtl URL parameter handling but does not include an automated regression test to prevent reintroduction. Without a test that covers ?rtl=true then ?rtl=false, the bug can easily return unnoticed.

Agent Prompt

## Issue description A bug fix was made to ensure `?rtl=false` disables RTL mode, but there is no automated regression test to ensure the behavior remains correct. ## Issue Context The fixed behavior is: after enabling RTL (via URL/cookie), loading with `?rtl=false` must switch back to LTR (and not remain stuck in RTL). ## Fix Focus Areas - src/tests/frontend-new/specs/language.spec.ts[51-87] - src/tests/frontend-new/helper/padHelper.ts[107-130] - src/static/js/pad.ts[121-126] - src/static/js/pad.ts[548-559]

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

The unconditional changeViewOption('rtlIsTrue', false) overwrote cookie-persisted RTL preferences and language-direction defaults. Track explicit setting with rtlIsExplicit flag so we only override when the user or server actually specified an rtl value. Adds regression tests for rtl=true, rtl=false, and cookie persistence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The RTL changeViewOption call was racing with padeditor.init() — the async setViewOptions(initialViewOptions) at the end of init overwrote the URL-param-based RTL setting. Moving it into postAceInit ensures padeditor is fully initialized. Also switched tests to use Playwright auto-retrying assertions for robustness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Three issues fixed: - setCheckbox used .attr('checked') instead of .prop('checked'), so the JS checked property was never set and Playwright saw unchecked state - html10n localized event overwrote RTL setting from URL params and cookie preferences; now skips override when either is active - Server default padOptions.rtl:false was treated as explicit, overwriting cookie-persisted RTL; added fromUrl flag to distinguish URL from server All 94 Playwright tests and 740 backend tests pass locally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

`?showLineNumbers=false` and `?useMonospaceFont=true` were being silently clobbered shortly after pad load. Same race that affected `?rtl=false` before #7464: 1. _afterHandshake → getParams() sets settings.LineNumbersDisabled (or useMonospaceFontGlobal, noColors). 2. _afterHandshake calls padeditor.init(view).then(postAceInit) — async; ace iframes still loading. 3. Sync tail of _afterHandshake hits the URL-param overrides and calls changeViewOption('showLineNumbers', false) etc. These queue setProperty('showslinenumbers', false) in Ace2Editor's actionsPendingInit queue (loaded=false). 4. ace.init resolves → loaded=true → queue flushes → URL-driven value applied. 5. padeditor.init resumes past its own await and calls setViewOptions(initialViewOptions) — initialViewOptions is built from clientVars.initialOptions.view (server defaults ∨ cookie), which does NOT carry the URL preference. The resulting setProperty('showslinenumbers', true) runs against loaded=true ace and immediately re-shows the gutter. #7464 noticed this race for RTL and moved the override into postAceInit. The neighbouring blocks for showLineNumbers / noColors / useMonospaceFontGlobal were left at the synchronous-tail site — generalise the same fix to all three. Direct-browser users typically had a `prefs` cookie with showLineNumbers=false from a prior in-pad toggle, so the initialViewOptions value happened to match the URL param and the race was unobservable. Cross-context iframe embeds (the reporter's configuration) start with no cookie, so the server default true fights the URL false and the race becomes visible. Adds src/tests/frontend-new/specs/url_view_options.spec.ts covering the showLineNumbers=false / =true / useMonospaceFont=true cases on initial-load navigation (the path where the race actually fires). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…7843) * fix(pad): URL view-option params lost to padeditor.init race (#7840) `?showLineNumbers=false` and `?useMonospaceFont=true` were being silently clobbered shortly after pad load. Same race that affected `?rtl=false` before #7464: 1. _afterHandshake → getParams() sets settings.LineNumbersDisabled (or useMonospaceFontGlobal, noColors). 2. _afterHandshake calls padeditor.init(view).then(postAceInit) — async; ace iframes still loading. 3. Sync tail of _afterHandshake hits the URL-param overrides and calls changeViewOption('showLineNumbers', false) etc. These queue setProperty('showslinenumbers', false) in Ace2Editor's actionsPendingInit queue (loaded=false). 4. ace.init resolves → loaded=true → queue flushes → URL-driven value applied. 5. padeditor.init resumes past its own await and calls setViewOptions(initialViewOptions) — initialViewOptions is built from clientVars.initialOptions.view (server defaults ∨ cookie), which does NOT carry the URL preference. The resulting setProperty('showslinenumbers', true) runs against loaded=true ace and immediately re-shows the gutter. #7464 noticed this race for RTL and moved the override into postAceInit. The neighbouring blocks for showLineNumbers / noColors / useMonospaceFontGlobal were left at the synchronous-tail site — generalise the same fix to all three. Direct-browser users typically had a `prefs` cookie with showLineNumbers=false from a prior in-pad toggle, so the initialViewOptions value happened to match the URL param and the race was unobservable. Cross-context iframe embeds (the reporter's configuration) start with no cookie, so the server default true fights the URL false and the race becomes visible. Adds src/tests/frontend-new/specs/url_view_options.spec.ts covering the showLineNumbers=false / =true / useMonospaceFont=true cases on initial-load navigation (the path where the race actually fires). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(types): annotate navigateWithParam helper params Fixes CI ts-check: parameters page/padId/param implicitly had `any`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

qodo-free-for-open-source-projects Bot reviewed Apr 5, 2026

View reviewed changes

JohnMcLear and others added 3 commits April 5, 2026 16:15

JohnMcLear merged commit ef0b257 into ether:develop Apr 6, 2026
14 checks passed

JohnMcLear mentioned this pull request May 25, 2026

fix(pad): URL view-option params lost to padeditor.init race (#7840) #7843

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: RTL URL parameter rtl=false now correctly disables RTL mode#7464

fix: RTL URL parameter rtl=false now correctly disables RTL mode#7464
JohnMcLear merged 4 commits into
ether:developfrom
JohnMcLear:fix/rtl-url-param-5559

JohnMcLear commented Apr 5, 2026

Uh oh!

JohnMcLear commented Apr 5, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Apr 5, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Apr 5, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Apr 5, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot Apr 5, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

JohnMcLear commented Apr 5, 2026

Summary

Root Cause

Fix

Test plan

Uh oh!

JohnMcLear commented Apr 5, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Apr 5, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-free-for-open-source-projects Bot commented Apr 5, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Apr 5, 2026 •

edited

Loading