frontend: Live-sync editor with server updates and add merge option#4269
frontend: Live-sync editor with server updates and add merge option#4269kahirokunn wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
8b191b6 to
22b5e83
Compare
409c38b to
dabe9e5
Compare
dabe9e5 to
6dfc8cd
Compare
|
@joaquimrocha The code has a bit too many lines, but I think this is the ideal editor from a user experience perspective. What do you think? My idea is that if the server-side YAML changes, it will eventually cause an optimistic lock error. Therefore, I think it would be a better experience to reflect the server-side YAML to some extent automatically in the editor. |
6dfc8cd to
4b11025
Compare
|
I think this is likely too complex for us to have it in time for this week's release, but let's try for the next one (and review it as soon as we can). |
|
I think the source code might have even more room for improvement, but I believe the PR and the video effectively convey what we're aiming to achieve. Please let me join in on this challenge together! 💪 |
|
fiy there's also a somewhat related PR that we should make sure it'll work nicely together #4098 |
vyncent-t
left a comment
There was a problem hiding this comment.
Will throw in a copilot review for improvement suggestions, otherwise looks interesting!
There was a problem hiding this comment.
Pull request overview
This PR implements live-sync functionality for the resource YAML editor, enabling the editor to automatically update when the underlying Kubernetes resource changes on the server. When users haven't made edits, the editor auto-updates. When users have modified the content, a warning banner appears with options to reload from server or perform a 3-way merge.
Key changes:
- Added live server update tracking with automatic editor synchronization when not dirty
- Implemented a 3-way merge algorithm with conflict detection for resolving concurrent edits
- Added visual highlighting to show externally-changed lines in both Monaco and simple editors
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/common/Resource/EditorDialog.tsx | Core implementation of live-sync, merge logic, warning banner, and highlighting functionality |
| frontend/src/components/common/Resource/EditButton.tsx | Updated to pass full KubeObject instead of editable object to enable live updates |
| frontend/src/components/common/Resource/SimpleEditor.tsx | Converted to forwardRef to support ref access for highlighting in simple editor mode |
| frontend/src/lib/k8s/KubeObject.ts | Added KUBE_OBJECT_BRAND symbol for cross-bundle instance detection |
| frontend/src/components/resourceMap/KubeObjectGlance/NodeGlance.tsx | Updated to use brand checking instead of instanceof for KubeObject detection |
| frontend/src/plugin/snapshots/pluginLib.snapshot | Updated snapshot to reflect SimpleEditor's forwardRef change |
| frontend/src/i18n/locales/*/translation.json | Added translation strings for merge conflict messages and UI actions (with some locales pending translation) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4b11025 to
2bb4918
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 29 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
47760f8 to
21bea75
Compare
|
Hi @vyncent-t, |
|
Is there anything blocking progress on this PR? 👀 |
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
21bea75 to
a296bfa
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kahirokunn The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi, sorry for the delay. I think the part where it warns about a server update to the object is good and maybe we can limit the PR just to that change? It'll be a lot easier to review and get it in. |
|
Hi, honestly, I share the same concerns. While richer UI definitely improves the user experience, the more features we add, the more lines of code it accumulates. Testing animations and such is quite challenging and difficult to maintain meaningfully. That's because we can't use standard snapshot testing techniques, and it's different from typical frontend or backend tests. Of course, we could write comprehensive tests for it, but those test files would likely be substantial in volume too. I keep thinking it would be great if some library provided this kind of functionality out of the box easily. |
|
For now, I think we should put this PR on hold. |
|
@sniok I was wondering, how about making it possible to replace these standard components with plugins? |
|
I have created an issue regarding this proposal below. |
|
@kahirokunn I’d really love this code to land in headlamp core rather than in a plugin. Currently headlamp is basically broken for changing frequently updated items, so I think this is a very important improvement. I agree with you both that it should have some tests and for it to be simplified as possible. If you like we can work together to refactor the changes into smaller / separated pieces with tests? Or I can try to take it over to add some tests? Up to you. Ps. I haven’t read the plugin proposal, but in general I think we’re usually happy to add extension points where people need them. |
|
@illume |
|
@jaehanbyun reported this issue as well in |
|
@kahirokunn after the LFX application busy period settles down I'll take another look to work to get it merged. Thanks for your understanding. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
This PR improves the resource YAML editor behavior when the underlying resource changes on the server. If the user hasn’t typed anything yet, the editor auto-updates to the latest YAML. If the user has started editing, the editor won’t overwrite changes and instead shows an accessible warning with actions to reload or merge changes.
Related Issue
Fixes #XXXX
Changes
EditButton/EditorDialogso the editor follows live server updates when not dirty.Steps to Test
Screenshots
Notes for the Reviewer
status,metadata.managedFields, andmetadata.resourceVersionto reduce noisy conflicts.