-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
fix(editor): Fix save keybind in expression editor and unfocused node details view #13640
fix(editor): Fix save keybind in expression editor and unfocused node details view #13640
Conversation
471ad31
to
266c159
Compare
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
This looks great! Love the tests in particular. One complication to be aware of is that the NodeCreator (right hand + button to add nodes) and the AI Assistant (needs some local setup, shoot me a dm if you want to try this) both also do some custom listening. Your code handles this fine though, typing in the AI Assistant still works while on the NDV, and while the NDV is open cmd+s saves (without it open the existing behavior of the pop up remains, which is fine or follow up at worst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, and everything works well locally 🙌
n8n
|
Project |
n8n
|
Branch Review |
ado-3120-bug-cmd-s-doesnt-save-when-in-expression-editor
|
Run status |
|
Run duration | 04m 40s |
Commit |
|
Committer | Jaakko Husso |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
2
|
|
5
|
|
0
|
|
440
|
View all changes introduced in this branch ↗︎ |
✅ All Cypress E2E specs passed |
Added a couple of more unit tests tests increasing coverage on the NDV, if you could take a look at this again tomorrow that would be great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work 🙌
✅ All Cypress E2E specs passed |
Summary
Cmd+S
/Ctrl+S
keybind to save the workflow at Node details view wasn't saving the workflow if NDV wasn't focused or if the expressions editor modal was open. This PR fixes that by registering adocument
levelkeydown
capture listener while NDV is open, and unregistering it when closed.Kapture.2025-03-03.at.15.04.50.mp4
When NDV is opened it isn't initially focused and as previously the listener was bound to the div so before clicking the NDV saving wouldn't work.
After focusing the NDV save would work, but if user then opened expression editor modal save keybind would again stop working.
codemirror
editor that opens would also register capture keydown handlers that would consume the events after handling them, and NDV'sonKeyDown
would never trigger. This has probably been broken since #12285 was merged.Since
NDVFloatingNodes
already registers a document levelkeydown
capture listener I initially solved this at 7965627 by listening to save keybind there and emiting the event back up toNodeDetailsView
, but I didn't like this solution as it left the@keydown.capture
listener onNodeDetailsVIew
that wasn't really doing what was expected.My second and proposed solution is to register/unregister another
document
levelkeydown
listener on NDV when the modal is opened or closed.I'm still not convinced if this is the cleanest way to fix this, so feel free to give feedback or alternative ideas on how to fix this.
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-3120/bug-cmd-s-doesnt-save-when-in-expression-editor
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)