fix(editor): only fire change event on non-programmatic changes #433
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the code editor was incorrectly firing 'change' events for programmatic document switches. The fix replaces a boolean flag with CodeMirror's annotation system to properly track programmatic changes.
- Introduces
_programmaticChangeannotation to mark all programmatic edits - Removes the
_valueChangingFromOutsideboolean flag mechanism - Updates event firing logic to check transaction annotations instead of the flag
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/playground-code-editor.ts | Replaces boolean flag with annotation system for tracking programmatic changes |
| src/test/playground-code-editor_test.ts | Adds comprehensive tests for document switching and change event behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const editorInternals = editor as unknown as { | ||
| _editorView: EditorView; | ||
| }; |
There was a problem hiding this comment.
Accessing private members through type assertion is fragile and could break if the internal structure changes. Consider exposing a test-only public method or using a more robust testing approach.
There was a problem hiding this comment.
ehhhhhh i agree but I'm hoping CM7 aint coming soon
There was a problem hiding this comment.
Seems fine for this test, but could you also have fired a keyboard event into the widget? Just asking, no change needed.
|
cc: @justinfagnani for review as well |
e111077
force-pushed
the
programmatic-changes-cm6
branch
from
ec15c09 to
168a35c
Compare
justinfagnani
left a comment
justinfagnani
left a comment
There was a problem hiding this comment.
A couple of comments / questions
| const editorInternals = editor as unknown as { | ||
| _editorView: EditorView; | ||
| }; |
There was a problem hiding this comment.
Seems fine for this test, but could you also have fired a keyboard event into the widget? Just asking, no change needed.
e111077
force-pushed
the
programmatic-changes-cm6
branch
from
168a35c to
af5b5be
Compare
e111077
force-pushed
the
programmatic-changes-cm6
branch
from
af5b5be to
e85767a
Compare
We maybe could – not quite sure what triggers CM6's editor correctly, but we use that internal editorView to test other things like document changes, caching, and to make sure that the editor is actually showing what |
3 participants
We were guarding firing the
changeevent against programmatic changes from Lit bindings, but we were not guarding against programmatic changes from document switching which is a new concept in CM6.This PR marks all of our programmatic changes as programmatic and then checks the transactions for a programmatic change. In practice, user changes shouldn't overlap with these transactions.
Also making
tabchangeevent bubble so that users can still determine when tabs have changed since we do not fire change events for that