fix(ui): improve editor performance and fix Submit button. Fixes #13892 #13915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…proj#1382 The `<ObjectEditor>` component used by all the edit pages encapsulates the logic for serializing/deserializing the object being edited, but it's extremely inefficient. For example, if you're editing a `WorkflowTemplate`, every single keystroke will cause `parse()` to be called twice and `stringify()` to be called 4 times. For large documents, this serializatino/deserialization overhead adds up quickly. Additionally, the "Submit" button is sometimes incorrectly grayed out, e.g. after clicking the "Update" button. This is happening because `useEditableObject()` is using reference comparisons on the object being edited, since it doesn't have access to the serialized string version encapsulated by `<ObjectEditor>`. This decouples `<ObjectEditor>` from serialization/deseralization and lifts the state involved up to the `useEditableObject()` hook, as discussed in argoproj#13593 (comment). Doing so eliminates unnecessary serialization/deseralization: now, if you're editing a `WorkflowTemplate`, each keystroke will only call `parse()` once and won't call `stringify()` at all. This also fixes issues with the Submit button not reflecting the current page state, since `useEditableObject()` now uses string comparisons instead of ref comparisons. Signed-off-by: Mason Malone <[email protected]>
MasonM
changed the title
4 tasks
Signed-off-by: Mason Malone <[email protected]>
Adrien-D
approved these changes
Nov 22, 2024
Joibel
approved these changes
Nov 22, 2024
Joibel
pushed a commit
that referenced
this pull request
Nov 22, 2024
… (#13915) Signed-off-by: Mason Malone <[email protected]> (cherry picked from commit 1392ef5)
|
@MasonM, I have done some more testing and this doesn't work properly. For me if you go to any of the editing pages, and scroll to the bottom, and press I haven't reverted this at this point. |
MasonM
added a commit
to MasonM/argo-workflows
that referenced
this pull request
Nov 23, 2024
argoproj#13915 introduced a regression where edits made in an object editor that introduce syntax errors cause a full screen error. The editor uses the following `try-catch` block to handle parse errors, but the `catch` block no longer works because the parsing logic was moved to `reducer()`, which is executed async: https://github.com/argoproj/argo-workflows/blob/main/ui/src/shared/components/object-editor.tsx#L117-L122 This moves the parsing logic to the `setObject()` function, which means errors are now propagated to the caller. Tested by going http://localhost:8080/workflow-templates, creating a new `WorkflowTemplate`, and making edits. Signed-off-by: Mason Malone <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13892
Motivation
The
<ObjectEditor>component used by all the edit pages encapsulates the logic for serializing/deserializing the object being edited, but it's extremely inefficient. For example, if you're editing aWorkflowTemplate, every single keystroke will cause parse() to be called twice and stringify() to be called 4 times. For large documents, this serialization/deserialization overhead adds up quickly.Additionally, the "Submit" button is sometimes incorrectly grayed out, e.g. after clicking the "Update" button. This is happening because
useEditableObject()is using reference comparisons on the object being edited, since it doesn't have access to the serialized string version encapsulated by<ObjectEditor>.Modifications
This decouples
<ObjectEditor>from the serialization/deseralization logic and lifts the state involved up to theuseEditableObject()hook, as discussed in #13593 (comment). Doing so eliminates unnecessary serialization/deseralization: now, if you're editing aWorkflowTemplate, each keystroke will only callparse()once and won't callstringify()at all. This also fixes issues with the Submit button not reflecting the current page state, sinceuseEditableObject()now uses string comparisons instead of ref comparisons.Also, I did some minor cleanup:
ui/src/cluster-workflow-templates/cluster-workflow-template-editor.tsxbecause it was identical toui/src/workflow-templates/workflow-template-editor.tsxui/src/shared/components/resource-editor/resource.scssbecause it stopped being used in feat(ui): Argo Events API and UI. Fixes #888 #4470ui/src/shared/components/resource-editor/resource-editor.tsx, which was a thin wrapper around<ObjectEditor>, because the only files using it weren't actually passing any of the props (e.g.onSubmit) that enabled the extra functionality.Verification
Followed these steps to verify on each of the following pages: http://localhost:8080/workflow-templates, http://localhost:8080/cluster-workflow-templates, http://localhost:8080/cron-workflows, http://localhost:8080/event-sources, and http://localhost:8080/sensors
Recording of the http://localhost:8080/workflow-templates page:
test-workflowtemplate.mp4
I also verified I can edit objects at http://localhost:8080/event-flow and http://localhost:8080/workflow-event-bindings