fix(pad): keep menu_right visible on readonly pads by default#7783
Merged
Conversation
PR #X (issue #5182) added a client-side `$('#editbar .menu_right').hide()` for readonly pads, opt-out via `?showMenuRight=true`. The intent — clean chrome for iframe-embedded readonly announcement pads — was good but the implementation hid the userlist toggle along with import/export. That has two unwanted effects on non-embed deployments: * Plugins like ep_guest inject their "Log In" button into `#myuser` inside the userlist popup, which lives under `.menu_right`. When the guest user (readOnly: true) lands on a readonly pad, the button they need to escape readonly is hidden. Chicken-and-egg. * Settings, embed, home, timeslider, showusers are all legitimately useful for readonly viewers and got removed alongside the actual write-only controls. The server already does the right thing without help from the client: * src/node/utils/toolbar.ts:282-290 strips `savedrevision` from the right toolbar when isReadOnly is true. * src/static/css/pad/popup_import_export.css:1 has `.readonly .acl-write { display: none }`, which hides the Import column of the import/export popup. Export stays visible (and is legitimately useful in readonly). Drop the client-side blanket hide. The iframe-embed use case from #5182 is still served by `?showMenuRight=false` (the existing handler at src/static/js/pad.ts:91-107), and `?showControls=false` continues to hide the entire editbar for callers who want even the left menu gone. Tests: rewrite `hide_menu_right.spec.ts`. The "readonly pad hides .menu_right by default" assertion is inverted; a new test confirms `?showMenuRight=false` still hides on readonly pads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoKeep menu_right visible on readonly pads by default
WalkthroughsDescription• Remove client-side blanket hide of .menu_right on readonly pads • Server-side mechanisms already handle readonly toolbar correctly • Restore access to userlist for auth plugins like ep_guest • Keep iframe-embed opt-out via ?showMenuRight=false parameter Diagramflowchart LR
A["Readonly Pad Load"] --> B{"showMenuRight param?"}
B -->|false| C["Hide menu_right<br/>iframe-embed case"]
B -->|true or absent| D["Show menu_right<br/>default behavior"]
D --> E["Server strips<br/>savedrevision"]
D --> F["CSS hides Import<br/>column only"]
D --> G["Userlist accessible<br/>for auth plugins"]
File Changes1. src/static/js/pad.ts
|
Code Review by Qodo
Context used✅ Tickets:
🎫 Hide menu_right on ReadOnly pads 1. Readonly shows menu_right toolbar
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
789
to
+803
| $('#chaticon').hide(); | ||
| $('#options-chatandusers').parent().hide(); | ||
| $('#options-stickychat').parent().hide(); | ||
| // Hide the right-side toolbar on readonly pads — import/export, | ||
| // timeslider, settings, share, users are all noise for viewers | ||
| // who can't interact with the pad. Callers who need those | ||
| // controls visible on a readonly pad can force them back via | ||
| // `?showMenuRight=true`, which runs in getParameters() above. | ||
| if (getUrlVars().get('showMenuRight') !== 'true') { | ||
| $('#editbar .menu_right').hide(); | ||
| } | ||
| // The right-side toolbar stays visible on readonly pads. The | ||
| // server-side `toolbar.menu(buttons, isReadOnly)` (see | ||
| // src/node/utils/toolbar.ts) already strips `savedrevision`, and | ||
| // `.readonly .acl-write { display: none }` hides the Import column | ||
| // inside the import/export popup, so the remaining controls | ||
| // (export, timeslider, settings, embed, home, showusers) are all | ||
| // safe for readonly viewers — and the userlist is the surface that | ||
| // plugins like ep_guest hang their "Log In" button off, so hiding | ||
| // it traps guests in readonly with no way out. Iframe-embed use | ||
| // cases that want a clean look (issue #5182) opt in to the hide | ||
| // via `?showMenuRight=false`, or hide the whole editbar via | ||
| // `?showControls=false`. |
There was a problem hiding this comment.
1. Readonly shows menu_right toolbar 📎 Requirement gap ⛨ Security
The PR changes the default behavior so #editbar .menu_right remains visible on read-only pads. This violates the requirement that read-only pads (especially iframe/embed) must not expose the menu_right toolbar options.
Agent Prompt
## Issue description
The compliance requirement states that when a pad is opened in read-only mode (especially embedded via iframe), the `menu_right` toolbar must not be visible. The current PR explicitly keeps `#editbar .menu_right` visible on readonly pads by default, and the frontend test suite was updated to assert that visibility.
## Issue Context
To comply, readonly pads should hide `#editbar .menu_right` by default, while still allowing explicit overrides via the existing `showMenuRight` URL parameter.
## Fix Focus Areas
- src/static/js/pad.ts[785-803]
- src/tests/frontend-new/specs/hide_menu_right.spec.ts[47-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The client-side `$('#editbar .menu_right').hide()` introduced for issue #5182 hides the userlist toggle alongside import/export when a pad is readonly. That breaks any auth plugin that hangs a "Log In" button off the userlist popup (notably ep_guest, which injects its button into `#myuser`): the readOnly guest user can see the pad but has no way to escape readOnly mode because the very button they need is hidden.
It also throws useful chrome (settings, embed, home, timeslider, userlist) out with the truly write-only stuff (savedrevision, import) — none of those are write-only, and the server already strips the ones that are.
Root cause
Two server-side mechanisms already handle readonly correctly:
The blanket client-side hide is redundant against both and only serves the iframe-embed clean-chrome use case from #5182 — which is already served by the explicit `?showMenuRight=false` opt-out (handler at `src/static/js/pad.ts:91-107`), or by `?showControls=false` which hides the entire editbar.
Change
The `?showMenuRight=true` and `?showMenuRight=false` overrides still work; iframe-embed deployments switch from `?showMenuRight=true` (no-op now) to `?showMenuRight=false` (still hides).
Tests
`src/tests/frontend-new/specs/hide_menu_right.spec.ts`:
Test plan
🤖 Generated with Claude Code