Set isLoading the same way in all the viewers (#164)

* use cleaner 'void' prefix

* ensure isLoading is set synchronously before fetching new data

* remove unused loading state

* remove unused setProgress prop

* remove unnecessary dependency

* replace loading with isLoading

* harmonize where we set isLoading

* fix lint

Author: severo

packages/components/src/components/Cell.tsx

@@ -14,17 +14,10 @@ interface CellProps {
   config?: CellConfig
 }
 
-enum LoadingState {
-  NotLoaded,
-  Loading,
-  Loaded,
-}
-
 /**
  * Cell viewer displays a single cell from a table.
  */
 export default function CellView({ source, row, col, config }: CellProps) {
-  const [loading, setLoading] = useState<LoadingState>(LoadingState.NotLoaded)
   const [text, setText] = useState<string | undefined>()
   const [progress, setProgress] = useState<number>()
   const [error, setError] = useState<Error>()
@@ -57,17 +50,13 @@ export default function CellView({ source, row, col, config }: CellProps) {
         setError(error as Error)
         setText(undefined)
       } finally {
-        setLoading(LoadingState.Loaded)
         setProgress(undefined)
       }
     }
 
-    if (loading === LoadingState.NotLoaded) {
-      // use loading state to ensure we only load content once
-      setLoading(LoadingState.Loading)
-      loadCellData().catch(() => undefined)
-    }
-  }, [resolveUrl, requestInit, col, row, loading, setError])
+    setProgress(0)
+    void loadCellData()
+  }, [resolveUrl, requestInit, col, row])
 
   return (
     <Layout progress={progress} error={error} title={fileName}>

packages/components/src/components/Json.tsx

@@ -72,11 +72,11 @@ function JsonObject({ obj, label }: { obj: object, label?: string }): ReactNode
       <span className={styles.object}>{'{'}</span>
     </div>
     <ul>
-      {Object.entries(obj).map(([key, value]) => (
+      {Object.entries(obj).map(([key, value]) =>
         <li key={key}>
           <Json json={value as unknown} label={key} />
-        </li>
-      ))}
+        </li>,
+      )}
     </ul>
     <div className={styles.object}>{'}'}</div>
   </>

packages/components/src/components/viewers/CellPanel.tsx

@@ -37,7 +37,7 @@ export default function CellPanel({ df, row, col, setProgress, setError, onClose
       }
     }
 
-    loadCellData().catch(() => undefined)
+    void loadCellData()
   }, [df, col, row, setProgress, setError])
 
   const headers = <>

packages/components/src/components/viewers/ImageView.tsx

@@ -4,12 +4,6 @@ import { contentTypes, parseFileSize } from '../../lib/utils.js'
 import { Spinner } from '../Layout.js'
 import ContentHeader from './ContentHeader.js'
 
-enum LoadingState {
-  NotLoaded,
-  Loading,
-  Loaded
-}
-
 interface ViewerProps {
   source: FileSource
   setError: (error: Error | undefined) => void
@@ -24,14 +18,15 @@ interface Content {
  * Image viewer component.
  */
 export default function ImageView({ source, setError }: ViewerProps) {
-  const [loading, setLoading] = useState(LoadingState.NotLoaded)
   const [content, setContent] = useState<Content>()
+  const [isLoading, setIsLoading] = useState(true)
 
   const { fileName, resolveUrl, requestInit } = source
 
   useEffect(() => {
     async function loadContent() {
       try {
+        setIsLoading(true)
         const res = await fetch(resolveUrl, requestInit)
         if (res.status === 401) {
           const text = await res.text()
@@ -50,16 +45,10 @@ export default function ImageView({ source, setError }: ViewerProps) {
         setContent(undefined)
         setError(error as Error)
       } finally {
-        setLoading(LoadingState.Loaded)
+        setIsLoading(false)
       }
     }
-
-    setLoading((loading) => {
-      // use loading state to ensure we only load content once
-      if (loading !== LoadingState.NotLoaded) return loading
-      loadContent().catch(() => undefined)
-      return LoadingState.Loading
-    })
+    void loadContent()
   }, [fileName, resolveUrl, requestInit, setError])
 
   return <ContentHeader content={content}>
@@ -68,7 +57,7 @@ export default function ImageView({ source, setError }: ViewerProps) {
       className='image'
       src={content.dataUri} />}
 
-    {loading && <div className='center'><Spinner /></div>}
+    {isLoading && <div className='center'><Spinner /></div>}
   </ContentHeader>
 }
 

packages/components/src/components/viewers/JsonView.tsx

@@ -1,10 +1,10 @@
 import { useEffect, useState } from 'react'
 import type { FileSource } from '../../lib/sources/types.js'
 import { parseFileSize } from '../../lib/utils.js'
+import styles from '../../styles/Json.module.css'
 import Json from '../Json.js'
 import { Spinner } from '../Layout.js'
 import ContentHeader, { TextContent } from './ContentHeader.js'
-import styles from '../../styles/Json.module.css'
 
 interface ViewerProps {
   source: FileSource

packages/components/src/components/viewers/MarkdownView.tsx

@@ -5,12 +5,6 @@ import { Spinner } from '../Layout.js'
 import Markdown from '../Markdown.js'
 import ContentHeader, { TextContent } from './ContentHeader.js'
 
-enum LoadingState {
-  NotLoaded,
-  Loading,
-  Loaded
-}
-
 interface ViewerProps {
   source: FileSource
   setError: (error: Error | undefined) => void
@@ -20,15 +14,16 @@ interface ViewerProps {
  * Markdown viewer component.
  */
 export default function MarkdownView({ source, setError }: ViewerProps) {
-  const [loading, setLoading] = useState(LoadingState.NotLoaded)
   const [content, setContent] = useState<TextContent>()
+  const [isLoading, setIsLoading] = useState(true)
 
   const { resolveUrl, requestInit } = source
 
   // Load markdown content
   useEffect(() => {
     async function loadContent() {
       try {
+        setIsLoading(true)
         const res = await fetch(resolveUrl, requestInit)
         const text = await res.text()
         const fileSize = parseFileSize(res.headers) ?? text.length
@@ -43,21 +38,15 @@ export default function MarkdownView({ source, setError }: ViewerProps) {
         setError(error as Error)
         setContent(undefined)
       } finally {
-        setLoading(LoadingState.Loaded)
+        setIsLoading(false)
       }
     }
-
-    setLoading((loading) => {
-      // use loading state to ensure we only load content once
-      if (loading !== LoadingState.NotLoaded) return loading
-      loadContent().catch(() => undefined)
-      return LoadingState.Loading
-    })
+    void loadContent()
   }, [resolveUrl, requestInit, setError])
 
   return <ContentHeader content={content}>
     <Markdown className='markdown' text={content?.text ?? ''} />
 
-    { loading === LoadingState.Loading && <div className='center'><Spinner /></div> }
+    { isLoading && <div className='center'><Spinner /></div> }
   </ContentHeader>
 }

packages/components/src/components/viewers/ParquetView.tsx

@@ -9,12 +9,6 @@ import CellPanel from './CellPanel.js'
 import ContentHeader, { ContentSize } from './ContentHeader.js'
 import SlidePanel, { SlidePanelConfig } from './SlidePanel.js'
 
-enum LoadingState {
-  NotLoaded,
-  Loading,
-  Loaded
-}
-
 export type ParquetViewConfig = SlidePanelConfig & RoutesConfig
 
 interface ViewerProps {
@@ -32,15 +26,16 @@ interface Content extends ContentSize {
  * Parquet file viewer
  */
 export default function ParquetView({ source, setProgress, setError, config }: ViewerProps) {
-  const [loading, setLoading] = useState<LoadingState>(LoadingState.NotLoaded)
+  const [isLoading, setIsLoading] = useState<boolean>(true)
   const [content, setContent] = useState<Content>()
   const [cell, setCell] = useState<{ row: number, col: number } | undefined>()
 
-  const { resolveUrl, requestInit, sourceId } = source
   useEffect(() => {
     async function loadParquetDataFrame() {
       try {
+        setIsLoading(true)
         setProgress(0.33)
+        const { resolveUrl, requestInit } = source
         const asyncBuffer = await asyncBufferFromUrl({ url: resolveUrl, requestInit })
         const from = { url: resolveUrl, byteLength: asyncBuffer.byteLength, requestInit }
         setProgress(0.66)
@@ -52,20 +47,12 @@ export default function ParquetView({ source, setProgress, setError, config }: V
       } catch (error) {
         setError(error as Error)
       } finally {
-        setLoading(LoadingState.Loaded)
+        setIsLoading(false)
         setProgress(1)
       }
     }
-    if (loading === LoadingState.NotLoaded) {
-      setLoading(LoadingState.Loading)
-      loadParquetDataFrame().catch(() => undefined)
-    }
-  }, [loading, resolveUrl, requestInit, setError, setProgress])
-
-  // Clear loading state on content change
-  useEffect(() => {
-    setLoading(LoadingState.NotLoaded)
-  }, [source])
+    void loadParquetDataFrame()
+  }, [setError, setProgress, source])
 
   // Close cell view on escape key
   useEffect(() => {
@@ -81,6 +68,7 @@ export default function ParquetView({ source, setProgress, setError, config }: V
     return () => { window.removeEventListener('keydown', handleKeyDown) }
   }, [cell])
 
+  const { sourceId } = source
   const getCellRouteUrl = useCallback(({ col, row }: {col: number, row: number}) => {
     const url = config?.routes?.getCellRouteUrl?.({ sourceId, col, row })
     if (url) {
@@ -108,13 +96,13 @@ export default function ParquetView({ source, setProgress, setError, config }: V
 
   const mainContent = <ContentHeader content={content} headers={headers}>
     {content?.dataframe && <HighTable
-      cacheKey={resolveUrl}
+      cacheKey={source.resolveUrl}
       data={content.dataframe}
       onDoubleClickCell={onDoubleClickCell}
       onMouseDownCell={onMouseDownCell}
       onError={setError} />}
 
-    {loading === LoadingState.Loading && <div className='center'><Spinner /></div>}
+    {isLoading && <div className='center'><Spinner /></div>}
   </ContentHeader>
 
   let panelContent

packages/components/src/components/viewers/TextView.tsx

@@ -7,7 +7,6 @@ import ContentHeader, { TextContent } from './ContentHeader.js'
 interface ViewerProps {
   source: FileSource
   setError: (error: Error | undefined) => void
-  setProgress: (progress: number | undefined) => void
 }
 
 /**
@@ -41,8 +40,7 @@ export default function TextView({ source, setError }: ViewerProps) {
         setIsLoading(false)
       }
     }
-
-    loadContent().catch(() => undefined)
+    void loadContent()
   }, [resolveUrl, requestInit, setError])
 
   const headers = <>

packages/components/src/components/viewers/Viewer.tsx

@@ -45,6 +45,6 @@ export default function Viewer({
 
   // Default to text viewer
   return (
-    <TextView source={source} setError={setError} setProgress={setProgress} />
+    <TextView source={source} setError={setError} />
   )
 }

packages/components/test/components/Folder.test.tsx

@@ -152,7 +152,7 @@ describe('Folder Component', () => {
 
     // Type a search query and hit enter
     const searchInput = getByPlaceholderText('Search...')
-    await act(async () => {
+    act(() => {
       fireEvent.keyUp(searchInput, { target: { value: 'file1' } })
     })
 
@@ -163,7 +163,7 @@ describe('Folder Component', () => {
     expect(location.href).toBe('/files?key=file1.txt')
   })
 
-  it('jumps to search box when user types /', async () => {
+  it('jumps to search box when user types /', () => {
     const dirSource: DirSource = {
       sourceId: 'test-source',
       sourceParts: [{ text: 'test-source', sourceId: 'test-source' }],
@@ -179,7 +179,7 @@ describe('Folder Component', () => {
     expect(document.activeElement).toBe(searchInput)
 
     // Typing inside the search box should work including /
-    await act(async () => {
+    act(() => {
       fireEvent.keyUp(searchInput, { target: { value: 'file1/' } })
       expect(searchInput.value).toBe('file1/')
     })