fix(pad): suppress deletion-token modal when allowPadDeletionByAllUsers is on (#7926)#7929
fix(pad): suppress deletion-token modal when allowPadDeletionByAllUsers is on (#7926)#7929JohnMcLear wants to merge 1 commit into
Conversation
…PadDeletionByAllUsers is on When `allowPadDeletionByAllUsers` is true, anyone can delete a pad with no token at all (handlePadDelete's flagOk branch), so the one-time deletion token is pointless and the "Save your pad deletion token" modal only overwhelms users who will never need it. Gate token issuance on `!settings.allowPadDeletionByAllUsers` so no token reaches clientVars; the client's showDeletionTokenModalIfPresent() then returns early and the modal never appears. No new setting — it derives automatically from the existing one. Closes #7926. Co-Authored-By: Claude Opus 4.8 (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? |
Code Review by Qodo
Context used 1. delete-pad-with-token still rendered
|
PR Summary by QodoSuppress pad deletion-token modal when open deletion is enabled WalkthroughsUser DescriptionProblemCloses #7926. The one-time pad deletion token is minted for the creator on their first visit, and the client pops a "Save your pad deletion token" modal. As the reporter notes, for many users (children/students, less technical teachers) this modal is confusing and overwhelming. When an instance sets ChangeGate token issuance on const padDeletionToken =
isCreator && !settings.requireAuthentication && !settings.allowPadDeletionByAllUsers
? await padDeletionManager.createDeletionTokenIfAbsent(sessionInfo.padId)
: null;With no token in This PR is intentionally minimal and unconditionally safe (a token is never needed when everyone can delete). A follow-up PR will handle the related ideas discussed on the issue — suppressing the token for authenticated/OIDC sessions that carry a durable cross-device identity, and the deletion-button naming. Test
Full 🤖 Generated with Claude Code AI Description• Stop minting pad deletion tokens when any user can delete pads via configuration. • Prevent the client from showing the save-token modal by omitting the token from clientVars. • Add socket.io backend tests covering token issuance vs. allowPadDeletionByAllUsers behavior. Diagramgraph TD
A["Browser client"] --> B["Socket.IO handshake"] --> C["PadMessageHandler.handleClientReady"] --> D["Token gating (settings)"] --> E["padDeletionManager.createDeletionTokenIfAbsent"]
D --> F["clientVars.padDeletionToken"] --> A
High-Level AssessmentThe following are alternative approaches to this PR: 1. Client-side suppression only
2. Introduce a dedicated setting to disable the modal/token
3. Always mint token but omit from clientVars for specific sessions
Recommendation: Keep the current approach: gate token issuance on !allowPadDeletionByAllUsers (and existing !requireAuthentication). It’s the simplest, least surprising behavior—when anyone can delete, a recovery token provides no value—while also preventing the modal without adding new configuration knobs. The added backend tests appropriately lock in the intended behavior. File ChangesBug fix (1)
Tests (1)
|
| const padDeletionToken = | ||
| isCreator && !settings.requireAuthentication && !settings.allowPadDeletionByAllUsers | ||
| ? await padDeletionManager.createDeletionTokenIfAbsent(sessionInfo.padId) | ||
| : null; |
There was a problem hiding this comment.
1. delete-pad-with-token still rendered 📎 Requirement gap ≡ Correctness
When allowPadDeletionByAllUsers is true, the UI still includes deletion-token-related controls/text (the "Delete with token" section) even though the token is unnecessary. This violates the requirement to fully suppress deletion token UI in this configuration, not just the one-time modal.
Agent Prompt
## Issue description
The PR suppresses issuance of `clientVars.padDeletionToken` when `settings.allowPadDeletionByAllUsers` is enabled, which prevents the one-time modal. However, the settings UI still renders the token-related delete controls ("Delete with token" section), which violates the requirement that deletion-token UI should not appear at all when `allowPadDeletionByAllUsers` is `true`.
## Issue Context
The token-related controls are rendered in `pad.html` under a condition that only checks `!settings.requireAuthentication`, so they remain present even when `allowPadDeletionByAllUsers` is enabled.
## Fix Focus Areas
- src/templates/pad.html[390-398]
- src/node/handler/PadMessageHandler.ts[1297-1300]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Thanks — this is a deliberate scoping decision, not an oversight. This PR is intentionally minimal: it fixes the issue's primary complaint (the overwhelming "Save your pad deletion token" modal) with an unconditionally-safe server change.
The remaining deletion-token controls (relabeling the "Delete with token" disclosure to "Delete Pad" when no token is needed) are handled in the stacked follow-up #7930, which adds a canDeleteWithoutToken clientVar and switches the disclosure label accordingly. The maintainer (@JohnMcLear) explicitly asked to split this into two PRs — modal suppression first, the UI/label bits second.
Note we keep the disclosure present rather than removing it: with enablePadWideSettings: false it can be the only delete affordance, so hiding it outright would be unsafe. #7930 relabels instead.
Problem
Closes #7926.
The one-time pad deletion token is minted for the creator on their first visit, and the client pops a "Save your pad deletion token" modal. As the reporter notes, for many users (children/students, less technical teachers) this modal is confusing and overwhelming.
When an instance sets
allowPadDeletionByAllUsers: true, the token is pointless: anyone can already delete the pad with no token at all (handlePadDelete'sflagOkbranch). So in that configuration the modal is pure noise.Change
Gate token issuance on
!settings.allowPadDeletionByAllUsersinhandleClientReady:With no token in
clientVars, the client'sshowDeletionTokenModalIfPresent()returns early and the modal never appears. This implements the reporter's first (preferred) option — it derives automatically from the existing setting, no new setting required.This PR is intentionally minimal and unconditionally safe (a token is never needed when everyone can delete). A follow-up PR will handle the related ideas discussed on the issue — suppressing the token for authenticated/OIDC sessions that carry a durable cross-device identity, and the deletion-button naming.
Test
src/tests/backend/specs/socketio.ts— new "Pad deletion token issuance (#7926)" block:clientVarswhenallowPadDeletionByAllUsersis true.Full
socketio.tssuite: 41 passing.🤖 Generated with Claude Code