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

fix(ui): handle parsing errors properly in object editor #13931

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MasonM
Copy link
Contributor

@MasonM MasonM commented Nov 23, 2024

Motivation

#13915 introduced a regression where edits made in an object editor that introduce syntax errors cause a full screen error.
image

This is happening because parsing was lifted out of <ObjectEditor> and into a reducer. Before #13915, the object editor used the following to catch parse errors:

try {
onChange(parse(v));
setError(null);
} catch (e) {
setError(e);
}

After #13915, the parsing logic was moved to reducer(), which is executed async, and therefore errors aren't propagated to the catch (e) {} block:

try {
onChange(v);
setError(null);
} catch (e) {
setError(e);
}

Modifications

This moves the parsing logic to the setObject() function (which gets passed as onChange to <ObjectEditor>), which means errors will now be caught by the catch (e) {} block.

Verification

Tested by going http://localhost:8080/workflow-templates, creating a new WorkflowTemplate, and making edits.

fix_error_handling.mp4

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]>
@MasonM MasonM changed the title fix(ui): handle parsing errors properly fix(ui): handle parsing errors properly in object editor Nov 23, 2024
@MasonM MasonM marked this pull request as ready for review November 23, 2024 21:12
@Adrien-D
Copy link
Member

Tested on local env and it works well

Do you think its also possible to have the try/catch logic in the reducer and return a parsingError state ? So the hook would be 'safe' to use by itself without throwing an error ?

@MasonM
Copy link
Contributor Author

MasonM commented Nov 25, 2024

@Adrien-D The problem is the parsing error needs to be propagated to <ObjectEditor> at the time onChange() is called, and I don't see any way of doing that when the parsing logic is in the reducer without lifting up the error state, which would involve a lot more changes.

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

Successfully merging this pull request may close these issues.

2 participants