Skip to content

feat(admin): Add undo/redo to query editors - #8580

Merged
garyxu-eng merged 3 commits into
masterfrom
feat/eap-844-admin-query-undo-redo
Oct 21, 2026
Merged

garyxu-eng merged 3 commits into
masterfrom
feat/eap-844-admin-query-undo-redo

Conversation

@garyxu-eng

@garyxu-eng garyxu-eng commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What is the PR

Cmd/Ctrl-Z and Cmd/Ctrl-Shift-Z (or Ctrl-Y) now undo and redo in Snuba Admin's query boxes. That covers:

  • the shared SQL editor (System Queries, Querylog, Tracing, Outcomes Analyzer, Snubsplain)
  • the System/Tracing Shell input

Picking a predefined query is now its own undo step, so Cmd-Z brings back what you had typed before. The shell's own keys (Enter, arrow-key command history, Ctrl-C/U/K/W) still work, and in the shell each prop-driven edit (arrow navigation, Ctrl-U/K/W, clearing after execute) is its own undo step.

Fixes EAP-844

Why

Both editors are CodeMirror 6, which only keeps undo history when the history() extension and its keymap are added. Neither editor had them, so undo did nothing.

The SQL editor also rebuilt its entire state (view.setState(EditorState.create(...))) whenever the value prop changed, which would wipe the history on every predefined-query pick. It now dispatches the new value as a regular change. That change:

  • is isolated in the history, so it doesn't merge with nearby typing;
  • is annotated so it isn't echoed back through onChange. Otherwise QueryEditor would clear the selected predefined query and hide its description.

QueryEditor's useLocalStorage now reads the saved query on the first render (getInitialValueInEffect: false). Before, it loaded it in an effect after mount, so the saved query arrived as an undoable change, and one undo right after load emptied the editor and overwrote the saved query.

In the shell, the history bindings sit after the shell's own keymap, so its bindings keep priority.

yarn.lock dedupes @codemirror/view. @codemirror/commands had its own nested 6.33.0 copy, so historyKeymap's KeyBinding type didn't match the one keymap.of() expects. Its ^6.27.0 range now resolves to the existing 6.32.0, so no versions change.

Alternatives

  • Type cast on historyKeymap instead of deduping yarn.lock: it would have silenced the TypeScript error but left a duplicate @codemirror/view installed under @codemirror/commands.
  • Binding CodeMirror's full defaultKeymap: it would also have added line-moving, comment-toggling and other shortcuts this ticket didn't ask for, so only historyKeymap is bound.
  • Keeping setState and only adding history(): undo would work while typing, but every predefined-query pick would reset the history.

Tests Done

  • New specs: sql_editor.spec.tsx and sql_shell/shell_input.spec.tsx render the real CodeMirror editors and press the keyboard shortcuts. The existing query_editor.spec.tsx mocks the editor with a plain textarea, which is why the bug went unnoticed.
    • On master: 3 of the 4 new tests fail (undo leaves the text unchanged).
    • With this change: all 4 pass.
  • query_editor_undo.spec.tsx: renders QueryEditor with the real editor and a saved query, then checks that one undo right after load keeps both the text and localStorage. It fails without the getInitialValueInEffect fix.
  • Full admin suite: 14 suites / 71 tests pass locally. tsc --noEmit adds no new errors (static/tracing/types.tsx already has one on master). The production bundle builds.
  • Manual, local Snuba Admin:
    • System Queries: undo and redo while typing; undo of a predefined-query pick (the description shows while it's selected); multiple Enter newlines.
    • The same check on Querylog, Tracing and Snubsplain.
    • System Shell: undo and redo; Enter still executes; arrow history; Ctrl-U followed by undo.
    • On prod (master), undo does nothing in these boxes, while plain textareas like RPC Endpoints keep native undo.

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented Oct 20, 2026

Copy link
Copy Markdown

EAP-844

@garyxu-eng
garyxu-eng force-pushed the feat/eap-844-admin-query-undo-redo branch from cbd1ae0 to 6ff9101 Compare October 20, 2026 23:38
The Snuba Admin query editors are CodeMirror 6, which only has undo history when the
history() extension and its keymap are added. Neither SQLEditor nor the SQL shell input did,
so Cmd/Ctrl-Z did nothing in any query box.

Add history() and historyKeymap to both editors. SQLEditor also replaced its whole state
when the value prop changed (e.g. picking a predefined query), which would wipe the history.
It now dispatches the new value as an isolated, undoable change and does not echo it back
through onChange.

Dedupe @codemirror/view in yarn.lock: @codemirror/commands had its own nested copy, so
its KeyBinding type did not match the one keymap.of() expects.

Fixes EAP-844

Co-Authored-By: Claude <[email protected]>
@garyxu-eng
garyxu-eng force-pushed the feat/eap-844-admin-query-undo-redo branch from 6ff9101 to 49aa6b8 Compare October 21, 2026 16:50
// Dispatch rather than replace the state, so the undo history is kept and
// the new value (e.g. a predefined query) is its own undo step
if (value !== currentValue) {
view.dispatch({

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This used to be view.setState(EditorState.create(...)). Replacing the state throws away every extension's state, including the undo history, so each predefined-query pick would have wiped it. Dispatching a change keeps the history, and isolateHistory.of("full") stops it merging with nearby typing, so a single Cmd-Z brings back exactly what was there before the pick.

onChange(update.state.doc.toString());
if (
update.docChanged &&
!update.transactions.some((tr) => tr.annotation(fromProps))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now that prop changes go through dispatch, they reach updateListener. Without this check, picking a predefined query would come back through onChange, and QueryEditor would run setSelectedPredefinedQuery(undefined), hiding the description you just selected. An undo creates its own transaction without the annotation, so it's still reported. Covered by "does not report a value set from props as a change" in sql_editor.spec.tsx.

Comment thread snuba/admin/yarn.lock
integrity sha512-QkEyUiLhsJoZkbumGZlswmAhA7CBU02Wrz7zvH4SrcifbsqwlXShVXg65f3v/ts57W3dqyamEriMhij1Z3Zz4A==

"@codemirror/view@^6.17.0", "@codemirror/view@^6.23.0", "@codemirror/view@^6.32.0":
"@codemirror/view@^6.17.0", "@codemirror/view@^6.23.0", "@codemirror/view@^6.27.0", "@codemirror/view@^6.32.0":

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@codemirror/commands asks for @codemirror/view@^6.27.0, which was resolved separately to 6.33.0 and installed as a nested copy. TypeScript then saw two different KeyBinding types, and keymap.of(historyKeymap) failed to compile. 6.32.0 already satisfies ^6.27.0, so the range now points at the existing entry. No version changes, and only one copy is left.

@garyxu-eng
garyxu-eng marked this pull request as ready for review October 21, 2026 17:34
@garyxu-eng
garyxu-eng requested a review from a team as a code owner October 21, 2026 17:34

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 642c77a. Configure here.

Comment thread snuba/admin/static/common/components/sql_editor.tsx
Comment thread snuba/admin/yarn.lock Outdated
useLocalStorage loaded the saved query in an effect after the editor mounted, so it
arrived as an undoable change. One undo right after load emptied the editor and onChange then
overwrote the saved query. Read it on the first render instead.

Also restore the handlebars entry in yarn.lock, which a local yarn install had pointed at
an internal registry mirror.

Refs EAP-844

Co-Authored-By: Claude <[email protected]>
@garyxu-eng
garyxu-eng merged commit 9c7cd00 into master Oct 21, 2026
69 checks passed
@garyxu-eng
garyxu-eng deleted the feat/eap-844-admin-query-undo-redo branch October 21, 2026 19:46
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.

3 participants