Skip to content

Commit

Permalink
SWC-6486 - Fix case where cached annotations would not load into the …
Browse files Browse the repository at this point in the history 
…schematized annotation editor

State management errors in useGetJson and SchemaDrivenAnnotationEditor led to a case where

1. The cache was 'hot', but since `useGetJson`'s return `annotations` was stored in state instead of derived from state, it was undefined when the data was loaded; i.e. there was one instance where `annotations` was `undefined` but `isLoading` was `false`
2. SchemaDrivenAnnotationEditor's `formData` was initialized with the undefined `annotations` value
3. RJSF takes the undefined `formData` value and transforms it to an empty object and somehow components become uncontrolled and get out of sync-this seems to be a bug in RJSF.

We can fix this by
1. Derive the value of `annotations` from the Entity JSON data, so it will never be undefined when `isLoading` is false
2. Initialize `formData` using `annotations`, so `undefined` will not passed into the `Form` component when the cache is hot
3. Prevent rendering the `Form` component when `formData` is undefined
  • Loading branch information
@nickgros
nickgros committed Aug 28, 2023
1 parent c6f3aef commit 0e59da0
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 180 deletions.
8 changes: 4 additions & 4 deletions packages/synapse-react-client/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@
"@react-google-maps/api": "^2.18.1",
"@react-hook/resize-observer": "^1.2.6",
"@react-hookz/web": "^23.1.0",
"@rjsf/core": "^5.9.0",
"@rjsf/mui": "^5.9.0",
"@rjsf/utils": "^5.9.0",
"@rjsf/validator-ajv8": "^5.9.0",
"@rjsf/core": "5.12.1",
"@rjsf/mui": "5.12.1",
"@rjsf/utils": "5.12.1",
"@rjsf/validator-ajv8": "5.12.1",
"@sage-bionetworks/react-base-table": "^1.13.4",
"@tanstack/react-table": "^8.9.3",
"@upsetjs/react": "^1.11.0",
Expand Down
279 changes: 143 additions & 136 deletions ...react-client/src/components/SchemaDrivenAnnotationEditor/SchemaDrivenAnnotationEditor.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Form from '@rjsf/mui'
import { JSONSchema7, JSONSchema7Definition } from 'json-schema'
import isEmpty from 'lodash-es/isEmpty'
import React, { useCallback, useEffect, useMemo, useRef } from 'react'
import { Alert, Box, Divider, Link } from '@mui/material'
import { Alert, Box, Divider, Link, Typography } from '@mui/material'
import AddToList from '../../assets/icons/AddToList'
import {
BackendDestinationEnum,
Expand Down Expand Up @@ -113,11 +113,6 @@ export function SchemaDrivenAnnotationEditor(
const localRef = useRef<RJSF>(null)
const ref = formRefFromParent ?? localRef

// Annotation fields fetched and modified via the form
const [formData, setFormData] = React.useState<
Record<string, unknown> | undefined
>(undefined)

// Client-side validation errors
const [validationError, setValidationError] = React.useState<
RJSFValidationError[] | undefined
Expand All @@ -138,6 +133,11 @@ export function SchemaDrivenAnnotationEditor(
useErrorBoundary: true,
})

// Annotation fields fetched and modified via the form
const [formData, setFormData] = React.useState<
Record<string, unknown> | undefined
>(annotations)

/**
* patternProperties lets us define how to treat additionalProperties in a JSON schema by property name.
* In all cases, let's ban properties that collide with entity properties by making their schema "not: {}"
Expand Down Expand Up @@ -223,139 +223,146 @@ export function SchemaDrivenAnnotationEditor(
</b>
</Alert>
)}
{entityJson &&
(!formData || isEmpty(formData)) &&
schema === null && (
<Alert severity="info">
<b>{entityJson.name}</b> has no annotations. Click the{' '}
<AddToList /> button to annotate.
</Alert>
)}
<Form
validator={validator}
className="AnnotationEditorForm"
liveValidate={liveValidate}
noHtml5Validate={true}
experimental_defaultFormStateBehavior={{
emptyObjectFields: 'skipDefaults',
}}
fields={{
ObjectField: CustomObjectField,
}}
templates={{
ArrayFieldDescriptionTemplate: ArrayFieldDescriptionTemplate,
ArrayFieldItemTemplate: ArrayFieldItemTemplate,
ArrayFieldTemplate: ArrayFieldTemplate,
ArrayFieldTitleTemplate: ArrayFieldTitleTemplate,
BaseInputTemplate: BaseInputTemplate,
FieldErrorTemplate: FieldErrorTemplate,
FieldTemplate: FieldTemplate,
ObjectFieldTemplate: ObjectFieldTemplate,
WrapIfAdditionalTemplate: WrapIfAdditionalTemplate,
ButtonTemplates: ButtonTemplate,
DescriptionFieldTemplate: DescriptionFieldTemplate,
/* Errors are displayed by an Alert component below, so we don't show the builtin ErrorList */
ErrorListTemplate: () => null,
}}
ref={ref}
disabled={mutation.isLoading}
schema={
{
...(validationSchema ?? {}),
patternProperties: {
...(validationSchema?.patternProperties ?? {}),
...patternPropertiesBannedKeys,
},
additionalProperties:
validationSchema?.additionalProperties ?? true,
} as JSONSchema7
}
uiSchema={{
'ui:options': {
copyable: true,
duplicateKeySuffixSeparator: '_',
},
additionalProperties: {
'ui:field': AdditionalPropertiesSchemaField,
},
}}
transformErrors={transformErrors}
formData={formData}
onChange={({ formData }) => {
if (onChange) {
onChange(formData)
{entityJson && isEmpty(formData) && schema === null && (
<Alert severity="info">
<Box display={'flex'} alignItems={'center'} gap={0.5}>
<Typography variant={'smallText1'}>
<b>{entityJson.name}</b> has no annotations. Click the{' '}
</Typography>
<AddToList />
<Typography variant={'smallText1'}>
button to annotate.
</Typography>
</Box>
</Alert>
)}
{formData != undefined && (
<Form
validator={validator}
className="AnnotationEditorForm"
liveValidate={liveValidate}
noHtml5Validate={true}
experimental_defaultFormStateBehavior={{
emptyObjectFields: 'skipDefaults',
}}
fields={{
ObjectField: CustomObjectField,
}}
templates={{
ArrayFieldDescriptionTemplate: ArrayFieldDescriptionTemplate,
ArrayFieldItemTemplate: ArrayFieldItemTemplate,
ArrayFieldTemplate: ArrayFieldTemplate,
ArrayFieldTitleTemplate: ArrayFieldTitleTemplate,
BaseInputTemplate: BaseInputTemplate,
FieldErrorTemplate: FieldErrorTemplate,
FieldTemplate: FieldTemplate,
ObjectFieldTemplate: ObjectFieldTemplate,
WrapIfAdditionalTemplate: WrapIfAdditionalTemplate,
ButtonTemplates: ButtonTemplate,
DescriptionFieldTemplate: DescriptionFieldTemplate,
/* Errors are displayed by an Alert component below, so we don't show the builtin ErrorList */
ErrorListTemplate: () => null,
}}
ref={ref}
disabled={mutation.isLoading}
schema={
{
...(validationSchema ?? {}),
patternProperties: {
...(validationSchema?.patternProperties ?? {}),
...patternPropertiesBannedKeys,
},
additionalProperties:
validationSchema?.additionalProperties ?? true,
} as JSONSchema7
}
setFormData(formData)
setValidationError(undefined)
}}
onSubmit={({ formData, errors }, event) => {
event.preventDefault()
if (errors && errors.length > 0) {
uiSchema={{
'ui:options': {
copyable: true,
duplicateKeySuffixSeparator: '_',
},
additionalProperties: {
'ui:field': AdditionalPropertiesSchemaField,
},
}}
transformErrors={transformErrors}
formData={formData}
onChange={({ formData }) => {
if (onChange) {
onChange(formData)
}
setFormData(formData)
setValidationError(undefined)
}}
onSubmit={({ formData, errors }, event) => {
event.preventDefault()
if (errors && errors.length > 0) {
setValidationError(errors)
}
setShowSubmissionError(false)
setFormData(formData)
submitChangedEntity()
}}
onError={(errors: RJSFValidationError[]) => {
// invoked when submit is clicked and there are client-side validation errors
setValidationError(errors)
}
setShowSubmissionError(false)
setFormData(formData)
submitChangedEntity()
}}
onError={(errors: RJSFValidationError[]) => {
// invoked when submit is clicked and there are client-side validation errors
setValidationError(errors)
if (validationError && entityId) {
setShowConfirmation(true)
}
}}
widgets={{
TextWidget: TextWidget,
DateTimeWidget: DateTimeWidget,
SelectWidget: SelectWidget,
CheckboxWidget: BooleanWidget,
}}
>
{validationError && (
<Alert severity="error" sx={{ my: 2 }}>
<b>Validation errors found:</b>
<ul>
{validationError.map(
(e: RJSFValidationError, index: number) => {
return (
<li key={index}>
<b>{`${getFriendlyPropertyName(e)}: `}</b>{' '}
{`${e.message}`}
</li>
)
},
)}
</ul>
</Alert>
)}
if (validationError && entityId) {
setShowConfirmation(true)
}
}}
widgets={{
TextWidget: TextWidget,
DateTimeWidget: DateTimeWidget,
SelectWidget: SelectWidget,
CheckboxWidget: BooleanWidget,
}}
>
{validationError && (
<Alert severity="error" sx={{ my: 2 }}>
<b>Validation errors found:</b>
<ul>
{validationError.map(
(e: RJSFValidationError, index: number) => {
return (
<li key={index}>
<b>{`${getFriendlyPropertyName(e)}: `}</b>{' '}
{`${e.message}`}
</li>
)
},
)}
</ul>
</Alert>
)}

{submissionError && showSubmissionError && (
<Alert severity="error" sx={{ my: 2 }}>
Annotations could not be updated: {submissionError.reason}
</Alert>
)}
{!formRefFromParent && (
<>
<Divider sx={{ my: 2 }} />
<Box
display="flex"
justifyContent="space-between"
sx={{ gridRowStart: 5 }}
>
<ConfirmationButtons
hasCancelButton={onCancel !== undefined}
onCancel={() => {
onCancel && onCancel()
}}
onConfirm={() => {
ref.current!.formElement.current.requestSubmit()
}}
confirmButtonText={entityId ? 'Save' : 'Validate'}
/>
</Box>
</>
)}
</Form>
{submissionError && showSubmissionError && (
<Alert severity="error" sx={{ my: 2 }}>
Annotations could not be updated: {submissionError.reason}
</Alert>
)}
{!formRefFromParent && (
<>
<Divider sx={{ my: 2 }} />
<Box
display="flex"
justifyContent="space-between"
sx={{ gridRowStart: 5 }}
>
<ConfirmationButtons
hasCancelButton={onCancel !== undefined}
onCancel={() => {
onCancel && onCancel()
}}
onConfirm={() => {
ref.current!.formElement.current.requestSubmit()
}}
confirmButtonText={entityId ? 'Save' : 'Validate'}
/>
</Box>
</>
)}
</Form>
)}
{showConfirmation && (
<ConfirmationDialog
open={true}
Expand Down
Loading

0 comments on commit 0e59da0

Please sign in to comment.