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

Feature/meditor 844 resolve sluggish text input #71

Merged
merged 6 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions packages/app/components/document/form-actions.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import Button from 'react-bootstrap/Button'
import cloneDeep from 'lodash.clonedeep'
import isEqual from 'lodash.isequal'
import { useRouter } from 'next/router'
import { useEffect, useState } from 'react'
import Button from 'react-bootstrap/Button'
import { clearEmpties } from '../../utils/object'
import Spinner from 'react-bootstrap/Spinner'
import styles from './form-actions.module.css'
import { clearEmpties } from '../../utils/object'
import { useEffect, useState } from 'react'
import { useRouter } from 'next/router'
import { wait } from '../../utils/time'

const DELETED_STATE = 'Deleted'
const DELETE_CONFIRMATION =
Expand All @@ -22,12 +24,14 @@ const FormActions = ({
onDelete = null,
CustomActions = null,
allowValidationErrors = false,
largeModel = false,
}) => {
const canSave = privileges.includes('edit') || privileges.includes('create')
const router = useRouter()

const [initialFormData, setInitialFormData] = useState(null)
const [isDirty, setIsDirty] = useState(false)
const [isSaving, setIsSaving] = useState(false)

useEffect(() => {
if (!formData) return
Expand Down Expand Up @@ -79,7 +83,12 @@ const FormActions = ({
}
}

function handleSave() {
async function handleSave(largeModel: boolean) {
if (largeModel) {
// This hack is in place to handle RJSF's ajv8 validator performance issues.
await wait(500)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benforshey save performance isn't as noticeable after upgrading AJV, reducing this save delay

}

let brokenLinks = localStorage.getItem('brokenLinks')
let hasBrokenLinks =
brokenLinks && Object.values(JSON.parse(brokenLinks)).includes('false')
Expand Down Expand Up @@ -189,9 +198,24 @@ const FormActions = ({
<Button
className={styles.button}
variant="secondary"
onClick={handleSave}
onClick={async () => {
setIsSaving(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should use a setTimeout here to remove the spinner in case the document fails to validate and save.


await handleSave(largeModel)
}}
>
Save
{isSaving && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the addition of the spinner!

<Spinner
animation="border"
role="status"
size="sm"
variant="light"
className="ml-2"
>
<span className="sr-only">Saving&hellip;</span>
</Spinner>
)}
</Button>
)}

Expand Down
4 changes: 3 additions & 1 deletion packages/app/components/document/form.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Button from 'react-bootstrap/Button'
import dynamic from 'next/dynamic'
import { useState } from 'react'
import Button from 'react-bootstrap/Button'

// JsonSchemaForm widgets rely heavily on global window, so we'll need to load them in separately
// as the server side doesn't have a window!
Expand All @@ -18,6 +18,7 @@ const Form = ({
onChange = (data: any) => {},
}) => {
const [expandAll, setExpandAll] = useState(false)
const largeModel = model?.largeModel || false

let layout = JSON.parse(model?.uiSchema || model?.layout || '{}')
let formData = document?.doc || document || {}
Expand Down Expand Up @@ -64,6 +65,7 @@ const Form = ({
imageUploadUrl={process.env.NEXT_PUBLIC_IMAGE_UPLOAD_URL}
linkCheckerUrl="/meditor/api/validate/url-resolves"
allowValidationErrors={allowValidationErrors}
largeModel={largeModel}
/>
</>
)
Expand Down
128 changes: 122 additions & 6 deletions packages/app/components/jsonschemaform/jsonschemaform.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import Form from '@rjsf/core'
import Form, { FormProps } from '@rjsf/core'
import type {
FormContextType,
FormValidation,
RJSFSchema,
StrictRJSFSchema,
} from '@rjsf/utils'
import validator from '@rjsf/validator-ajv8'
import jp from 'jsonpath'
import filter from 'lodash/filter'
import isEqual from 'lodash/isEqual'
Expand All @@ -7,8 +14,115 @@ import { useEffect, useRef } from 'react'
import fields from './fields/'
import templates from './templates/'
import widgets from './widgets/'
import validator from '@rjsf/validator-ajv8'
import type { FormValidation } from '@rjsf/utils'

class LargeModelForm<
T = any,
Copy link
Contributor Author

@benforshey benforshey Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hacky shenanigans...move the change event to be triggered on blur. This form is only used if the accompanying model is marked as largeModel.

S extends StrictRJSFSchema = RJSFSchema,
F extends FormContextType = any
> extends Form {
// Set a reference to the super's onChange method as a class field to prevent intermediate value errors.
superOnChange = this.onChange
// A list of element node names that we will target for overriding default behavior.
targetNodeNames = ['INPUT']
// A store of events that we conditionally re-propagate to RJSF.
eventStore = {}
// A store of state locks that get set on various events.
nodeShouldPropagateStore = {}

constructor(props: FormProps<T, S, F>) {
super(props)
}

componentDidMount() {
this.formElement.current.addEventListener('input', this.handleInput, {
capture: true,
})
this.formElement.current.addEventListener('change', this.handleChange, {
capture: true,
})
this.formElement.current.addEventListener('blur', this.handleBlur, {
capture: true,
})

if (super.componentDidMount) {
super.componentDidMount()
}
}

componentWillUnmount() {
this.formElement.current.removeEventListener('input', this.handleInput, {
capture: true,
})
this.formElement.current.removeEventListener('change', this.handleChange, {
capture: true,
})
this.formElement.current.removeEventListener('blur', this.handleBlur, {
capture: true,
})

if (super.componentWillUnmount) {
super.componentWillUnmount()
}
}

static debounce(fn: Function, delay = 600) {
let timerId: NodeJS.Timeout

return (...args: any[]) => {
globalThis.clearTimeout(timerId)

timerId = setTimeout(() => fn(...args), delay)
}
}

/*
* Targeting only nodes in our allowlist, we conditionally stop propagation so that the ajv8 validator does not get triggered.
* We state lock the event so that later logic does not propagate its change event.
*/
handleInput = (event: any) => {
if (this.targetNodeNames.includes(event.target.nodeName)) {
event.stopPropagation()

this.nodeShouldPropagateStore[event.target.id] = false
}
}

/*
* Targeting only nodes in our allowlist, we conditionally stop propagation so that the ajv8 validator does not get triggered.
* We store the event for later propagation.
*/
handleChange = (event: any) => {
if (
this.targetNodeNames.includes(event.target.nodeName) &&
!this.nodeShouldPropagateStore[event.target.id]
) {
event.stopPropagation()

this.eventStore[event.target.id] = event

return
}
}

/*
* Targeting only nodes in our allowlist, we check to make sure node's event has been stored.
* Because the ajv8 validator is slow on large schemas, we wait until the `blur` event before triggering an update.
* We mark the node as ready to propagate, re-dispatching the event.
* Debounce this handler for rapid tabbing, which triggers unnecessary state updates.
*/
handleBlur = LargeModelForm.debounce((event: any) => {
if (
this.targetNodeNames.includes(event.target.nodeName) &&
this.eventStore[event.target.id]
) {
this.nodeShouldPropagateStore[event.target.id] = true

this.eventStore[event.target.id].target.dispatchEvent(
this.eventStore[event.target.id]
)
}
})
}

const JsonSchemaForm = ({
schema,
Expand All @@ -18,9 +132,11 @@ const JsonSchemaForm = ({
layout,
liveValidate = false,
allowValidationErrors = false,
onInit = (form: any) => {},
onChange = (event: any) => {},
largeModel = false,
onInit = (_form: any) => {},
onChange = (_event: any) => {},
}) => {
const FormType = largeModel ? LargeModelForm : Form
const formEl: any = useRef(null)

useEffect(() => {
Expand Down Expand Up @@ -75,7 +191,7 @@ const JsonSchemaForm = ({
}

return (
<Form
<FormType
ref={formEl}
schema={schema}
formData={document}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useState, useEffect } from 'react'
import type { WidgetProps } from '@rjsf/utils'
import React, { useEffect, useState } from 'react'
import { getTemplate } from '@rjsf/utils'
import { ID_PREFIX } from './constants'
import type { WidgetProps } from '@rjsf/utils'

const DEFAULT_DELIMETER = ' > '

Expand Down Expand Up @@ -82,7 +82,6 @@ export default function ConcatenatedWidget(props: WidgetProps) {
// update the concatenated value anytime this field blurs or changes
el.onblur = () => updateConcatenatedFieldValueFromFields(fields)
el.onchange = () => updateConcatenatedFieldValueFromFields(fields)
el.onkeyup = () => updateConcatenatedFieldValueFromFields(fields)
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was causing a small, but perceptible lag when dealing with concatenated fields. I think updating the concatenated field on blur and change is good enough

}, 500)
}
Expand Down
1 change: 1 addition & 0 deletions packages/app/models/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export interface Model {
workflow?: string
notificationTemplate?: string
templates?: Template[]
largeModel?: boolean
}

export interface ModelWithWorkflow extends Omit<Model, 'workflow'> {
Expand Down
10 changes: 5 additions & 5 deletions packages/app/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"@json2csv/node": "^7.0.1",
"@rjsf/core": "^5.16.1",
"@rjsf/utils": "^5.16.1",
"@rjsf/validator-ajv8": "^5.16.1",
"@rjsf/validator-ajv8": "^6.0.0-alpha.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically running alpha versions is not a good idea...however this resolves a major performance issues with RJSF v5

"@yaireo/tagify": "3.8.0",
"bootstrap": "^4.5.0",
"brace": "^0.11.1",
Expand Down
2 changes: 2 additions & 0 deletions packages/app/pages/[modelName]/[documentTitle]/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const EditDocumentPage = ({
const params = router.query
const documentTitle = decodeURIComponent(params.documentTitle as string)
const modelName = params.modelName as string
const largeModel = model?.largeModel || false

const [currentVersion, setCurrentVersion] = useState(null)
const [form, setForm] = useState(null)
Expand Down Expand Up @@ -372,6 +373,7 @@ const EditDocumentPage = ({
allowValidationErrors={
model.workflow.currentNode.allowValidationErrors
}
largeModel={largeModel}
/>
</div>
)
Expand Down
Loading