Skip to content

fix(manager): Enter saves / Escape cancels per-key data limit dialog - #2883

Open
whoalin1 wants to merge 2 commits into
OutlineFoundation:masterfrom
whoalin1:fix/per-key-data-limit-enter-escape
Open

whoalin1 wants to merge 2 commits into
OutlineFoundation:masterfrom
whoalin1:fix/per-key-data-limit-enter-escape

Conversation

@whoalin1

@whoalin1 whoalin1 commented Oct 7, 2026

Copy link
Copy Markdown

Summary

Closes #1892

The per-key data limits dialog did not handle Enter/Escape like other Manager inputs. Wire @keydown on the paper-dialog: Escape closes/cancels; Enter triggers save when the save action is enabled (and skips when focus is already on a paper-button).

Test plan

  • Open per-key data limit dialog in Server Manager
  • Press Escape → dialog closes without saving
  • Set a valid limit, press Enter → saves and closes
  • Invalid input leaves save disabled → Enter does not save

@whoalin1
whoalin1 requested a review from a team as a code owner October 7, 2026 01:58
@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Adds keyboard shortcuts to a settings dialog.

This PR should not merge until Enter obeys the disabled save button.

Findings

  1. P1 Enter bypasses disabled save ▶
  2. P2 Repeated Enter starts extra saves ▶

Summary

The per-key data limit dialog now supports Enter to save and Escape to cancel. These shortcuts work through the dialog’s key handler while preserving button activation.

  • Enter saves and Escape cancels in the per-key limit dialog.

Reviews (1) · Last reviewed commit: "fix(manager): Enter saves / Escape cance..." · Reviewed by Greptile

Comment on lines +404 to +406
if (!this._enableSave && this._showDataLimit) {
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Enter bypasses disabled save

If a manager enters an invalid limit and then unchecks the limit checkbox, the save button stays disabled. Pressing Enter still calls _onSaveButtonTapped() because this guard also requires _showDataLimit to be true. The server then removes the key’s limit. Make the Enter guard match the save button’s disabled state.

Suggested change
if (!this._enableSave && this._showDataLimit) {
return;
}
if (!this._enableSave) {
return;
}

Comment on lines +407 to +408
event.preventDefault();
void this._onSaveButtonTapped();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Repeated Enter starts extra saves

Holding Enter or pressing it again before a save finishes calls _onSaveButtonTapped() again. Neither the key handler nor the save handler marks a save as pending, so the manager sends the same server request again and can see duplicate save notifications. Ignore further Enter presses until the first save finishes.

/**
* Enter saves (when enabled); Escape cancels — same as other Manager dialogs.
*/
private _onDialogKeydown(event: KeyboardEvent) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we are reinventing the wheel here. Shouldn't the standard components have the behavior you want? Perhaps look into the dialog API instead.

(also there are the other issues from greptile)

paper-dialog already cancels on Escape (IronOverlayBehavior,
no-cancel-on-esc-key is not set), so drop the custom dialog keydown
handler. Handle Enter only on the data limit input, respect the save
button's disabled state and ignore repeated saves while one is pending.
@whoalin1

whoalin1 commented Oct 8, 2026

Copy link
Copy Markdown
Author

Thanks, fair point. I looked into it: paper-dialog already cancels on Escape, so I
removed that part. For Enter there's nothing built in: dialog-confirm only reacts to
taps, and a <form method="dialog"> wrapper doesn't help because paper-input's inner
<input> lives in its shadow root, so the form never sees the implicit submit.

So I kept a small Enter handler, but only on the limit input (same as
outline-validated-input) instead of the whole dialog. Also fixed the two greptile issues:
Enter now respects the disabled Save button, and repeated Enter/clicks are ignored while a
save is pending.

If you'd rather move this dialog to a native <dialog> with a form (or another component
you're migrating to), I'm happy to do that instead. Just let me know which you prefer.

This branch has not been deployed

No deployments
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.

Enter and Escape keys should do the expected thing for the per-key data limits dialog

2 participants