From 1a3d72efd515f0ec4ff42a790dfa33263efc9da5 Mon Sep 17 00:00:00 2001 From: Mason Malone <651224+MasonM@users.noreply.github.com> Date: Sat, 23 Nov 2024 12:39:09 -0800 Subject: [PATCH] fix(ui): handle parsing errors properly https://github.com/argoproj/argo-workflows/pull/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 <651224+MasonM@users.noreply.github.com> --- ui/src/shared/use-editable-object.test.ts | 41 ++++++++++++++--------- ui/src/shared/use-editable-object.ts | 38 +++++++++++++-------- 2 files changed, 49 insertions(+), 30 deletions(-) diff --git a/ui/src/shared/use-editable-object.test.ts b/ui/src/shared/use-editable-object.test.ts index 11acd670927a..cbf6ead2d7e6 100644 --- a/ui/src/shared/use-editable-object.test.ts +++ b/ui/src/shared/use-editable-object.test.ts @@ -1,4 +1,4 @@ -import {createInitialState, reducer} from './use-editable-object'; +import {createInitialState, reducer, setObjectActionCreator} from './use-editable-object'; describe('createInitialState', () => { test('without object', () => { @@ -59,8 +59,8 @@ describe('reducer', () => { }); }); - test('setObject with string', () => { - const newState = reducer(testState, {type: 'setObject', payload: 'a: 2'}); + test('setObject with object and string', () => { + const newState = reducer(testState, {type: 'setObject', payload: {object: {a: 2}, serialization: 'a: 2'}}); expect(newState).toEqual({ object: {a: 2}, serialization: 'a: 2', @@ -70,8 +70,8 @@ describe('reducer', () => { }); }); - test('setObject with object', () => { - const newState = reducer(testState, {type: 'setObject', payload: {a: 2}}); + test('setObject with only object', () => { + const newState = reducer(testState, {type: 'setObject', payload: {object: {a: 2}}}); expect(newState).toEqual({ object: {a: 2}, serialization: 'a: 2\n', @@ -81,17 +81,6 @@ describe('reducer', () => { }); }); - test('resetObject with string', () => { - const newState = reducer(testState, {type: 'resetObject', payload: 'a: 2'}); - expect(newState).toEqual({ - object: {a: 2}, - serialization: 'a: 2', - lang: 'yaml', - initialSerialization: 'a: 2', - edited: false - }); - }); - test('resetObject with object', () => { const newState = reducer(testState, {type: 'resetObject', payload: {a: 2}}); expect(newState).toEqual({ @@ -103,3 +92,23 @@ describe('reducer', () => { }); }); }); + +describe('setObjectActionCreator', () => { + test('with object', () => { + expect(setObjectActionCreator({a: 2})).toEqual({ + type: 'setObject', + payload: {object: {a: 2}} + }); + }); + + test('with string', () => { + expect(setObjectActionCreator('a: 2')).toEqual({ + type: 'setObject', + payload: {object: {a: 2}, serialization: 'a: 2'} + }); + }); + + test("with string that can't be parsed", () => { + expect(() => setObjectActionCreator('@')).toThrow('Plain value cannot start with reserved character @'); + }); +}); diff --git a/ui/src/shared/use-editable-object.ts b/ui/src/shared/use-editable-object.ts index a04d3174b4ca..5380fcaceabc 100644 --- a/ui/src/shared/use-editable-object.ts +++ b/ui/src/shared/use-editable-object.ts @@ -3,7 +3,8 @@ import {useReducer} from 'react'; import {Lang, parse, stringify} from '../shared/components/object-parser'; import {ScopedLocalStorage} from '../shared/scoped-local-storage'; -type Action = {type: 'setLang'; payload: Lang} | {type: 'setObject'; payload: string | T} | {type: 'resetObject'; payload: string | T}; +type SetObjectAction = {type: 'setObject'; payload: {object: T; serialization?: string}}; +type Action = {type: 'setLang'; payload: Lang} | SetObjectAction | {type: 'resetObject'; payload: T}; interface State { /** The parsed form of the object, kept in sync with "serialization" */ @@ -24,20 +25,17 @@ const storage = new ScopedLocalStorage('object-editor'); export function reducer(state: State, action: Action) { const newState = {...state}; switch (action.type) { - case 'resetObject': case 'setObject': - if (typeof action.payload === 'string') { - newState.object = parse(action.payload); - newState.serialization = action.payload; - } else { - newState.object = action.payload; - newState.serialization = stringify(action.payload, newState.lang); - } - if (action.type === 'resetObject') { - newState.initialSerialization = newState.serialization; - } + newState.object = action.payload.object; + newState.serialization = action.payload.serialization ?? stringify(newState.object, newState.lang); newState.edited = newState.initialSerialization !== newState.serialization; return newState; + case 'resetObject': + newState.object = action.payload; + newState.serialization = stringify(newState.object, newState.lang); + newState.initialSerialization = newState.serialization; + newState.edited = false; + return newState; case 'setLang': newState.lang = action.payload; storage.setItem('lang', newState.lang, defaultLang); @@ -61,19 +59,31 @@ export function createInitialState(object?: T): State { }; } +/** + * Action creator for setObject that can accept a string and parse it. + * The reason the parsing logic isn't in the reducer is because we want parse + * errors to be propagated to the caller. + */ +export function setObjectActionCreator(value: string | T): SetObjectAction { + return { + type: 'setObject', + payload: typeof value === 'string' ? {object: parse(value), serialization: value} : {object: value} + }; +} + /** * useEditableObject is a React hook to manage the state of an object that can be serialized and edited, encapsulating the logic to * parse/stringify the object as necessary. */ export function useEditableObject(object?: T): State & { setObject: (value: string | T) => void; - resetObject: (value: string | T) => void; + resetObject: (value: T) => void; setLang: (lang: Lang) => void; } { const [state, dispatch] = useReducer(reducer, object, createInitialState); return { ...state, - setObject: value => dispatch({type: 'setObject', payload: value}), + setObject: value => dispatch(setObjectActionCreator(value)), resetObject: value => dispatch({type: 'resetObject', payload: value}), setLang: value => dispatch({type: 'setLang', payload: value}) };