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(comments): lost comment message while document is reconnecting #5928

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

hermanwikner
Copy link
Member

Description

This pull request addresses an issue where the comment input unmounts during document reconnection, leading to the loss of the comment being authored. To resolve this, the following solutions have been implemented:

  • Introduction of a new CommentsAuthoringPathContext. This context is needed because if the "open state" of the comment input is managed locally within the component, the state resets when unmounting. By tracking the user's current authoring path in a context, it ensures the comment input remains open even when the component is re-rendered.
  • Caching the message to make sure that it is not lost when re-rendering. Unlike the "authoring path", the comment message is not stored in a context to avoid potential performance degradation, as this would require all consumers to re-render whenever a comment is being authored.

What to Review

Ensure that the comment input remains open during form reconnection and that the comment being authored is not lost. While working to debug this issue, I changed the useConnectionState hook to toggle between the "connected" and "reconnecting" states using a setTimeout, as shown below:

export function useConnectionState(publishedDocId: string, docTypeName: string): ConnectionState {
  const [state, setState] = useState<ConnectionState>(INITIAL)

  useEffect(() => {
    setTimeout(() => {
      setState('reconnecting')

      setTimeout(() => {
        setState('connected')
      }, 5000)
    }, 5000)
  }, [])

  return state
}

Notes for release

N/A

@hermanwikner hermanwikner requested review from a team and jtpetty and removed request for a team March 7, 2024 13:42
Copy link

vercel bot commented Mar 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Mar 7, 2024 3:03pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 7, 2024 3:03pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Mar 7, 2024 3:03pm

Copy link
Contributor

github-actions bot commented Mar 7, 2024

No changes to documentation

Copy link
Contributor

github-actions bot commented Mar 7, 2024

Component Testing Report Updated Mar 7, 2024 3:07 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 32s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 11s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 12s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 32s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 59s 15 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 3s 18 0 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 20s 9 0 0

@jtpetty
Copy link
Contributor

jtpetty commented Mar 7, 2024

This implementation seems fine to me, but can I ask, is there a way to prevent the input from being unmounted at all when the form is set to readonly?

@hermanwikner
Copy link
Member Author

This implementation seems fine to me, but can I ask, is there a way to prevent the input from being unmounted at all when the form is set to readonly?

I'm not sure – I wasn't able to figure it out. Not sure if it possible in this situation, unfortunately. @jtpetty

Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

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

This looks fine as a temporary (and clever!) workaround. Although it feels like a bug that it's not possible to prevent unmounts on readOnly change, making a ticket for it so we can track it.

const handleOnChange = useCallback(
(nextValue: CommentMessage) => {
setValue(nextValue)
messageCache.set(stringPath, nextValue)
Copy link
Member

Choose a reason for hiding this comment

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

should the key also include the document id? (potentially also projectId+dataset?) to avoid it leaking between documents?

Copy link
Member

Choose a reason for hiding this comment

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

I see now that you'll get a modal when trying to navigate away, so probably won't be an issue.

@hermanwikner hermanwikner added this pull request to the merge queue Mar 8, 2024
Merged via the queue into next with commit ec4da46 Mar 8, 2024
40 checks passed
@hermanwikner hermanwikner deleted the edx-1156 branch March 8, 2024 08:45
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.

3 participants