Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snuba admin breaks browser undo #6786

Open
onewland opened this issue Jan 16, 2025 · 3 comments
Open

Snuba admin breaks browser undo #6786

onewland opened this issue Jan 16, 2025 · 3 comments

Comments

@onewland
Copy link
Contributor

If you delete a string in the System Queries tool (probably other SQL text entries as well) and then Control+ or Command+Z in the browser, the undo doesn't perform any action. We should not override browser behavior if we're not doing anything else useful.

@onewland
Copy link
Contributor Author

onewland commented Feb 3, 2025

the answer for this is probably to use basicSetup to give codemirror reasonable defaults: https://codemirror.net/docs/ref/#codemirror.basicSetup

currently: https://github.com/getsentry/snuba/blob/master/snuba/admin/static/common/components/sql_editor.tsx#L31-L37

just want to check with @gggritso that there's nothing special in our logic here

@onewland
Copy link
Contributor Author

onewland commented Feb 4, 2025

basicSetup and minimalSetup seem to do something that conflicts with the DOM so maybe we just add in a custom history handler and call it a day:

hook.js:608  Warning: validateDOMNesting(...): <form> cannot appear as a descendant of <form>. Error Component Stack
    at form (<anonymous>)
    at QueryEditor (query_editor.tsx:51:29)
    at form (<anonymous>)
    at div (<anonymous>)
    at div (<anonymous>)
    at ClickhouseQueries (index.tsx:8:63)
    at div (<anonymous>)
    at Body (body.tsx:12:11)
    at div (<anonymous>)
    at div (<anonymous>)
Uncaught Error: Unrecognized extension value in extension set ([object Object]). This sometimes happens because multiple instances of @codemirror/state are loaded, breaking instanceof checks.
    at inner (index.js:2044:23)
    at inner (index.js:2019:17)
    at inner (index.js:2019:17)
    at inner (index.js:2019:17)
    at flatten (index.js:2048:5)
    at _Configuration.resolve (index.js:1956:25)
    at _EditorState.create (index.js:2787:43)
    at sql_editor.tsx:44:31
    at commitHookEffectListMount (react-dom.development.js:23150:26)
    at commitLayoutEffectOnFiber (react-dom.development.js:23268:17)

@gggritso
Copy link
Member

gggritso commented Feb 4, 2025

👋🏻 the important parts of the custom extensions are

  1. multiNewLine which allows multiple consecutive newlines. Otherwise CodeMirror collapses them for some reason
  2. sql() for specific syntax highlighting
  3. updateListener to send the text changes upwards in React. This lets the parent React component take the updated text and store it in localStorage with a specific key, so all editors save their state independently from each other

If the basic setup (or minimal) are able to do all that, you're good to go! At the time when I put this together I don't think it did

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

No branches or pull requests

2 participants