Skip to content

feat(editor): add showMenuRight URL param to hide right-side toolbar#7553

Open
JohnMcLear wants to merge 4 commits intoether:developfrom
JohnMcLear:fix/hide-menu-right-5182
Open

feat(editor): add showMenuRight URL param to hide right-side toolbar#7553
JohnMcLear wants to merge 4 commits intoether:developfrom
JohnMcLear:fix/hide-menu-right-5182

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

Adds a new URL/embed parameter showMenuRight=false that hides the right-side toolbar (menu_right — import/export, timeslider, settings, share, users) without disabling those features for other pads.

Requested for read-only / announcement-pad use cases where viewers shouldn't see those controls, but the same server also hosts editable pads where those buttons should remain. Globally disabling via settings.toolbar.right is not a fit.

Follows the same pattern as the existing showControls URL parameter. Default behavior (menu visible) is unchanged.

Closes #5182

Test plan

  • Playwright: pad with ?showMenuRight=false.menu_right hidden, .menu_left still visible
  • Playwright: pad without the param → .menu_right visible (regression guard)
  • Playwright: pad with ?showMenuRight=true (or any non-false value) → .menu_right visible
  • pnpm run ts-check clean locally

🤖 Generated with Claude Code

Adds a showMenuRight URL/embed parameter. When set to false, the right-side
toolbar (.menu_right — import/export, timeslider, settings, share, users)
is hidden. Default behavior (menu shown) is unchanged.

Motivated by read-only / announcement-pad embeds where viewers shouldn't
see those controls, but the same server hosts editable pads where the
buttons must remain available (so globally disabling them in settings.json
is not a fit).

Closes ether#5182

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 showMenuRight URL parameter to hide right-side toolbar

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add showMenuRight URL parameter to conditionally hide right toolbar
• Hides import/export, timeslider, settings, share, users controls
• Follows existing showControls parameter pattern
• Includes comprehensive Playwright test coverage for all scenarios
Diagram
flowchart LR
  A["URL Parameter<br/>showMenuRight=false"] -- "triggers callback" --> B["Hide menu_right<br/>element"]
  C["Default behavior<br/>or showMenuRight!=false"] -- "no change" --> D["menu_right<br/>remains visible"]
Loading

Grey Divider

File Changes

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

Add showMenuRight URL parameter handler

• Added new showMenuRight parameter to getParameters array
• Parameter checks for value 'false' to trigger hiding
• Callback hides #editbar .menu_right element when condition met
• Follows same pattern as existing showControls parameter

src/static/js/pad.ts


2. src/tests/frontend-new/specs/hide_menu_right.spec.ts 🧪 Tests +26/-0

Add comprehensive tests for showMenuRight parameter

• New Playwright test suite for showMenuRight parameter
• Tests default behavior with parameter absent (menu visible)
• Tests showMenuRight=false hides right menu while keeping left menu visible
• Tests non-false values keep right menu visible

src/tests/frontend-new/specs/hide_menu_right.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 19, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. showMenuRight ignores readonly mode📎 Requirement gap ≡ Correctness
Description
The new logic hides #editbar .menu_right only when showMenuRight=false is explicitly provided,
rather than automatically when window.clientVars.readonly is true (including iframe embeds). As a
result, read-only pads can still display right-side controls unless callers remember to pass the URL
param, failing the read-only-hides-controls requirement.
Code

src/static/js/pad.ts[R80-84]

+    name: 'showMenuRight',
+    checkVal: 'false',
+    callback: (val) => {
+      $('#editbar .menu_right').hide();
+    },
Evidence
PR Compliance ID 1 requires menu_right to be hidden on read-only pads; the added parameter handler
only checks a URL/server option value and does not reference window.clientVars.readonly or embed
context, so read-only mode alone will not hide menu_right.

Hide right-side menu (menu_right) controls on read-only pads when embedded
src/static/js/pad.ts[79-85]

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

## Issue description
`menu_right` is only hidden when `showMenuRight=false` is provided, but compliance requires it to be hidden whenever the pad is read-only (including embeds).
## Issue Context
There is already a read-only indicator available via `window.clientVars.readonly`.
## Fix Focus Areas
- src/static/js/pad.ts[79-85]

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



Remediation recommended

2. Unused Playwright context🐞 Bug ☼ Reliability
Description
The new spec creates a new BrowserContext and clears its cookies, but the tests continue using the
existing fixture page from a different context, making the cookie clearing ineffective. The
created context is also never closed, adding unnecessary resource usage in the test suite.
Code

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

+test.beforeEach(async ({page, browser}) => {
+  const context = await browser.newContext();
+  await context.clearCookies();
+  await goToNewPad(page);
+});
Evidence
The spec creates a new context and clears cookies on it, but then calls goToNewPad(page), which
navigates the provided page (the Playwright fixture page) rather than a page created from the new
context. Because the new context is not used and not closed, the setup does extra work without
affecting the actual test page state.

src/tests/frontend-new/specs/hide_menu_right.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`, clears its cookies, but then continues using the existing `page` fixture (which belongs to a different context). This makes the cookie clearing ineffective and leaves an extra context unclosed.
### Issue Context
`goToNewPad(page)` navigates the passed-in `page` via `page.goto(...)`, so any cookie cleanup must be applied to `page.context()` (or the test must create/use a page from the new context and close it).
### Fix Focus Areas
- src/tests/frontend-new/specs/hide_menu_right.spec.ts[4-8]
- src/tests/frontend-new/helper/padHelper.ts[117-123]
### Suggested change
Prefer one of:
1) Simplest: replace the `browser.newContext()` usage with `await page.context().clearCookies()`.
2) If isolation via new context is desired: create a page from that context (`const page2 = await context.newPage()`), use that page for navigation/assertions, and close the context in `afterEach`.

ⓘ 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

Comment thread src/static/js/pad.ts
JohnMcLear and others added 2 commits April 19, 2026 18:37
…ght=true override

Addresses Qodo review feedback on ether#7553:

1. Readonly pads now hide the right-side toolbar automatically. The
   original issue (ether#5182) was specifically about readonly embeds; the
   previous implementation only honoured an explicit `?showMenuRight=false`
   URL parameter, which meant that vanilla readonly pads still showed
   import/export/timeslider/settings/share/users controls — all noise
   for viewers who can't interact with the pad anyway.

2. Callers who still want the menu visible on readonly pads can opt
   back in with `?showMenuRight=true`. The URL-param callback now
   accepts both values instead of just `false`.

3. The Playwright spec's `browser.newContext() + clearCookies()` pattern
   was a no-op because the test navigated with the existing `page`
   fixture (different context). Switch to `page.context().clearCookies()`,
   and cover both the auto-hide and the explicit-override paths on a
   readonly-URL navigation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous test looked up  (capital-I) and called
inputValue() on it. The real element is  (lowercase)
and it's a toggle checkbox, not a URL field. The readonly URL itself
is in `#linkinput`, updated live when the readonly checkbox is
checked. Wire the test to that flow.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JohnMcLear JohnMcLear force-pushed the fix/hide-menu-right-5182 branch from e921b77 to f2a97a9 Compare April 19, 2026 17:56
Playwright's stability check kept retrying the click while the popup
was animating open ("element is not stable"). Wait for
#embed.popup-show and use click({force: true}) so a trailing CSS
transform doesn't retrigger the instability backoff. Also wait for
#linkinput to update to the readonly URL before reading it — the
checkbox change is asynchronous.

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.

Hide menu_right on ReadOnly pads

1 participant