fix(cacheable-section): synchronously flush recording state for UI co…

…nsistency (#1394)
dhis2 · Nov 27, 2024 · 50d216c · 50d216c
1 parent 2aa8141
commit 50d216c
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 42 deletions.
diff --git a/services/data/src/react/hooks/useDataQuery.ts b/services/data/src/react/hooks/useDataQuery.ts
@@ -1,11 +1,10 @@
 import { useState, useRef, useCallback, useDebugValue } from 'react'
 import { useQuery, setLogger } from 'react-query'
-import {
-    JsonMap,
-    type Query,
-    type QueryOptions,
-    type QueryResult,
-    type QueryVariables,
+import type {
+    Query,
+    QueryOptions,
+    QueryResult,
+    QueryVariables,
 } from '../../engine'
 import type { FetchError } from '../../engine/types/FetchError'
 import type { QueryRenderInput, QueryRefetchFunction } from '../../types'

diff --git a/services/offline/src/__tests__/integration.test.tsx b/services/offline/src/__tests__/integration.test.tsx
@@ -43,7 +43,7 @@ const TestControls = ({
             />
             <div data-testid={`is-cached-${id}`}>{isCached ? 'yes' : 'no'}</div>
             <div data-testid={`last-updated-${id}`}>
-                {lastUpdated || 'never'}
+                {lastUpdated?.toISOString() ?? 'never'}
             </div>
             <div data-testid={`recording-state-${id}`}>{recordingState}</div>
         </>
@@ -112,7 +112,7 @@ describe('Coordination between useCacheableSection and CacheableSection', () =>
         expect(getByTestId(/controls-rc/)).toBeInTheDocument()
     })
 
-    it.skip('handles a successful recording', async (done) => {
+    it('handles a successful recording', async (done) => {
         const { getByTestId, queryByTestId } = screen
 
         const onStarted = () => {
@@ -147,7 +147,7 @@ describe('Coordination between useCacheableSection and CacheableSection', () =>
         expect.assertions(7)
     })
 
-    it.skip('handles a recording that encounters an error', async (done) => {
+    it('handles a recording that encounters an error', async (done) => {
         // Suppress the expected error from console (in addition to 'act' warning)
         jest.spyOn(console, 'error').mockImplementation((...args) => {
             const actPattern =
@@ -255,7 +255,7 @@ describe('Performant state management', () => {
         expect(getByTestId('section-rc-2')).toHaveTextContent('1')
     })
 
-    it.skip('isolates rerenders from other consumers', async (done) => {
+    it('isolates rerenders from other consumers', async (done) => {
         const { getByTestId } = screen
         // Make assertions
         const onCompleted = () => {
@@ -300,12 +300,12 @@ describe('useCacheableSection can be used inside a child of CacheableSection', (
         )
     }
 
-    it.skip('handles a successful recording', async (done) => {
-        const { getByTestId, queryByTestId, findByTestId } = screen
+    it('handles a successful recording', async (done) => {
+        const { getByTestId, queryByTestId } = screen
 
         const onStarted = async () => {
             await waitFor(() => {
-                expect(getByTestId(/recording-state/)).toBeInTheDocument(
+                expect(getByTestId(/recording-state/)).toHaveTextContent(
                     'recording'
                 )
                 expect(getByTestId(/loading-mask/)).toBeInTheDocument()
@@ -326,9 +326,9 @@ describe('useCacheableSection can be used inside a child of CacheableSection', (
 
         render(<ChildTest makeRecordingHandler={makeRecordingHandler} />)
 
-        // await act(async () => {
-        await fireEvent.click(getByTestId(/start-recording/))
-        // })
+        await act(async () => {
+            await fireEvent.click(getByTestId(/start-recording/))
+        })
 
         await waitFor(() => {
             // At this stage, should be pending

diff --git a/services/offline/src/lib/__tests__/cacheable-section-state.test.tsx b/services/offline/src/lib/__tests__/cacheable-section-state.test.tsx
@@ -1,10 +1,10 @@
 import { renderHook } from '@testing-library/react'
-import React, { FC } from 'react'
+import React, { FC, PropsWithChildren } from 'react'
 import { mockOfflineInterface } from '../../utils/test-mocks'
 import { useCachedSection, useRecordingState } from '../cacheable-section-state'
 import { OfflineProvider } from '../offline-provider'
 
-const wrapper: FC = ({ children }) => (
+const wrapper: FC<PropsWithChildren> = ({ children }) => (
     <OfflineProvider offlineInterface={mockOfflineInterface}>
         {children}
     </OfflineProvider>

diff --git a/services/offline/src/lib/__tests__/use-cacheable-section.test.tsx b/services/offline/src/lib/__tests__/use-cacheable-section.test.tsx
@@ -27,7 +27,7 @@ afterEach(() => {
     ;(console.error as jest.Mock).mockRestore()
 })
 
-it.skip('renders in the default state initially', () => {
+it('renders in the default state initially', () => {
     const wrapper: FC<PropsWithChildren> = ({ children }) => (
         <OfflineProvider offlineInterface={mockOfflineInterface}>
             {children}
@@ -42,7 +42,7 @@ it.skip('renders in the default state initially', () => {
 })
 
 it('has stable references', () => {
-    const wrapper: FC = ({ children }) => (
+    const wrapper: FC<PropsWithChildren> = ({ children }) => (
         <OfflineProvider offlineInterface={mockOfflineInterface}>
             {children}
         </OfflineProvider>
@@ -66,9 +66,9 @@ it('has stable references', () => {
     expect(result.current.remove).toBe(origRemove)
 })
 
-it.skip('handles a successful recording', async (done) => {
+it('handles a successful recording', async (done) => {
     const [sectionId, timeoutDelay] = ['one', 1234]
-    const testOfflineInterface = {
+    const recordingSuccessOfflineInterface = {
         ...mockOfflineInterface,
         getCachedSections: jest
             .fn()
@@ -78,7 +78,7 @@ it.skip('handles a successful recording', async (done) => {
             ]),
     }
     const wrapper: FC<PropsWithChildren> = ({ children }) => (
-        <OfflineProvider offlineInterface={testOfflineInterface}>
+        <OfflineProvider offlineInterface={recordingSuccessOfflineInterface}>
             {children}
         </OfflineProvider>
     )
@@ -93,8 +93,16 @@ it.skip('handles a successful recording', async (done) => {
         expect(result.current.recordingState).toBe('default')
 
         // Test that 'isCached' gets updated
-        expect(testOfflineInterface.getCachedSections).toBeCalledTimes(2)
-        await waitFor(() => expect(result.current.isCached).toBe(true))
+        expect(
+            recordingSuccessOfflineInterface.getCachedSections
+        ).toBeCalledTimes(2)
+        // Recording states are updated synchronously, but getting isCached
+        // state is asynchronous -- need to wait for that here.
+        // An assertion is not used as the waitFor condition because it may skew
+        // the total number assertions in this test if it needs to retry. Number
+        // of assertions is checked at the bottom of this test to make sure both
+        // of these callbacks are called.
+        await waitFor(() => result.current.isCached === true)
         expect(result.current.isCached).toBe(true)
         expect(result.current.lastUpdated).toBeInstanceOf(Date)
 
@@ -125,7 +133,7 @@ it.skip('handles a successful recording', async (done) => {
     expect.assertions(11)
 })
 
-it.skip('handles a recording that encounters an error', async (done) => {
+it('handles a recording that encounters an error', async (done) => {
     // Suppress the expected error from console (in addition to 'act' warning)
     jest.spyOn(console, 'error').mockImplementation((...args) => {
         const actPattern =
@@ -138,12 +146,12 @@ it.skip('handles a recording that encounters an error', async (done) => {
         }
         return originalError.call(console, ...args)
     })
-    const testOfflineInterface = {
+    const recordingErrorOfflineInterface = {
         ...mockOfflineInterface,
         startRecording: errorRecordingMock,
     }
     const wrapper: FC<PropsWithChildren> = ({ children }) => (
-        <OfflineProvider offlineInterface={testOfflineInterface}>
+        <OfflineProvider offlineInterface={recordingErrorOfflineInterface}>
             {children}
         </OfflineProvider>
     )
@@ -181,13 +189,13 @@ it.skip('handles a recording that encounters an error', async (done) => {
     expect.assertions(6)
 })
 
-it.skip('handles an error starting the recording', async () => {
-    const testOfflineInterface = {
+it('handles an error starting the recording', async () => {
+    const messageErrorOfflineInterface = {
         ...mockOfflineInterface,
         startRecording: failedMessageRecordingMock,
     }
     const wrapper: FC<PropsWithChildren> = ({ children }) => (
-        <OfflineProvider offlineInterface={testOfflineInterface}>
+        <OfflineProvider offlineInterface={messageErrorOfflineInterface}>
             {children}
         </OfflineProvider>
     )
@@ -198,9 +206,9 @@ it.skip('handles an error starting the recording', async () => {
     )
 })
 
-it.skip('handles remove and updates sections', async () => {
+it('handles remove and updates sections', async () => {
     const sectionId = 'one'
-    const testOfflineInterface = {
+    const sectionOpsOfflineInterface = {
         ...mockOfflineInterface,
         getCachedSections: jest
             .fn()
@@ -210,7 +218,7 @@ it.skip('handles remove and updates sections', async () => {
             .mockResolvedValueOnce([]),
     }
     const wrapper: FC<PropsWithChildren> = ({ children }) => (
-        <OfflineProvider offlineInterface={testOfflineInterface}>
+        <OfflineProvider offlineInterface={sectionOpsOfflineInterface}>
             {children}
         </OfflineProvider>
     )
@@ -228,14 +236,14 @@ it.skip('handles remove and updates sections', async () => {
 
     expect(success).toBe(true)
     // Test that 'isCached' gets updated
-    expect(testOfflineInterface.getCachedSections).toBeCalledTimes(2)
+    expect(sectionOpsOfflineInterface.getCachedSections).toBeCalledTimes(2)
     await waitFor(() => expect(result.current.isCached).toBe(false))
     expect(result.current.isCached).toBe(false)
     expect(result.current.lastUpdated).toBeUndefined()
 })
 
-it.skip('handles a change in ID', async () => {
-    const testOfflineInterface = {
+it('handles a change in ID', async () => {
+    const idChangeOfflineInterface = {
         ...mockOfflineInterface,
         getCachedSections: jest
             .fn()
@@ -244,7 +252,7 @@ it.skip('handles a change in ID', async () => {
             ]),
     }
     const wrapper: FC<PropsWithChildren> = ({ children }) => (
-        <OfflineProvider offlineInterface={testOfflineInterface}>
+        <OfflineProvider offlineInterface={idChangeOfflineInterface}>
             {children}
         </OfflineProvider>
     )
@@ -259,7 +267,7 @@ it.skip('handles a change in ID', async () => {
     rerender('id-two')
 
     // Test that 'isCached' gets updated
-    // expect(testOfflineInterface.getCachedSections).toBeCalledTimes(2)
+    // expect(idChangeOfflineInterface.getCachedSections).toBeCalledTimes(2)
     await waitFor(() => expect(result.current.isCached).toBe(false))
     expect(result.current.isCached).toBe(false)
     expect(result.current.lastUpdated).toBeUndefined()

diff --git a/services/offline/src/lib/cacheable-section.tsx b/services/offline/src/lib/cacheable-section.tsx
@@ -1,5 +1,6 @@
 import PropTypes from 'prop-types'
 import React, { useCallback, useEffect, useMemo } from 'react'
+import { flushSync } from 'react-dom'
 import { RecordingState } from '../types'
 import { useRecordingState, useCachedSection } from './cacheable-section-state'
 import { useOfflineInterface } from './offline-interface'
@@ -95,15 +96,23 @@ export function useCacheableSection(id: string): CacheableSectionControls {
                     sectionId: id,
                     recordingTimeoutDelay,
                     onStarted: () => {
-                        onRecordingStarted()
+                        // Flush this state update synchronously so that the
+                        // right recordingState is set before any other callbacks
+                        flushSync(() => {
+                            onRecordingStarted()
+                        })
                         onStarted && onStarted()
                     },
                     onCompleted: () => {
-                        onRecordingCompleted()
+                        flushSync(() => {
+                            onRecordingCompleted()
+                        })
                         onCompleted && onCompleted()
                     },
                     onError: (error) => {
-                        onRecordingError(error)
+                        flushSync(() => {
+                            onRecordingError(error)
+                        })
                         onError && onError(error)
                     },
                 })