From e27753045f13205ca38475d24e1e1df1a971b66f Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 10:37:49 +0100 Subject: [PATCH 1/8] use cleaner 'void' prefix --- packages/components/src/components/Cell.tsx | 2 +- packages/components/src/components/viewers/CellPanel.tsx | 2 +- packages/components/src/components/viewers/ImageView.tsx | 2 +- packages/components/src/components/viewers/MarkdownView.tsx | 2 +- packages/components/src/components/viewers/ParquetView.tsx | 2 +- packages/components/src/components/viewers/TextView.tsx | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/components/src/components/Cell.tsx b/packages/components/src/components/Cell.tsx index e70f7df3..f6bf6f19 100644 --- a/packages/components/src/components/Cell.tsx +++ b/packages/components/src/components/Cell.tsx @@ -65,7 +65,7 @@ export default function CellView({ source, row, col, config }: CellProps) { if (loading === LoadingState.NotLoaded) { // use loading state to ensure we only load content once setLoading(LoadingState.Loading) - loadCellData().catch(() => undefined) + void loadCellData() } }, [resolveUrl, requestInit, col, row, loading, setError]) diff --git a/packages/components/src/components/viewers/CellPanel.tsx b/packages/components/src/components/viewers/CellPanel.tsx index 855c7a6a..aa6dc7bd 100644 --- a/packages/components/src/components/viewers/CellPanel.tsx +++ b/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 = <> diff --git a/packages/components/src/components/viewers/ImageView.tsx b/packages/components/src/components/viewers/ImageView.tsx index 26ec6994..b404a66c 100644 --- a/packages/components/src/components/viewers/ImageView.tsx +++ b/packages/components/src/components/viewers/ImageView.tsx @@ -57,7 +57,7 @@ export default function ImageView({ source, setError }: ViewerProps) { setLoading((loading) => { // use loading state to ensure we only load content once if (loading !== LoadingState.NotLoaded) return loading - loadContent().catch(() => undefined) + void loadContent() return LoadingState.Loading }) }, [fileName, resolveUrl, requestInit, setError]) diff --git a/packages/components/src/components/viewers/MarkdownView.tsx b/packages/components/src/components/viewers/MarkdownView.tsx index 03ecf62d..baa201e2 100644 --- a/packages/components/src/components/viewers/MarkdownView.tsx +++ b/packages/components/src/components/viewers/MarkdownView.tsx @@ -50,7 +50,7 @@ export default function MarkdownView({ source, setError }: ViewerProps) { setLoading((loading) => { // use loading state to ensure we only load content once if (loading !== LoadingState.NotLoaded) return loading - loadContent().catch(() => undefined) + void loadContent() return LoadingState.Loading }) }, [resolveUrl, requestInit, setError]) diff --git a/packages/components/src/components/viewers/ParquetView.tsx b/packages/components/src/components/viewers/ParquetView.tsx index 0ff6a645..046c809e 100644 --- a/packages/components/src/components/viewers/ParquetView.tsx +++ b/packages/components/src/components/viewers/ParquetView.tsx @@ -58,7 +58,7 @@ export default function ParquetView({ source, setProgress, setError, config }: V } if (loading === LoadingState.NotLoaded) { setLoading(LoadingState.Loading) - loadParquetDataFrame().catch(() => undefined) + void loadParquetDataFrame() } }, [loading, resolveUrl, requestInit, setError, setProgress]) diff --git a/packages/components/src/components/viewers/TextView.tsx b/packages/components/src/components/viewers/TextView.tsx index 10da9f76..7474a14f 100644 --- a/packages/components/src/components/viewers/TextView.tsx +++ b/packages/components/src/components/viewers/TextView.tsx @@ -42,7 +42,7 @@ export default function TextView({ source, setError }: ViewerProps) { } } - loadContent().catch(() => undefined) + void loadContent() }, [resolveUrl, requestInit, setError]) const headers = <> From f84aa671a43a44cbcc862a59fb2af620142482f7 Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 10:45:57 +0100 Subject: [PATCH 2/8] ensure isLoading is set synchronously before fetching new data --- .../src/components/viewers/ImageView.tsx | 20 +++++-------------- .../src/components/viewers/JsonView.tsx | 5 +++-- .../src/components/viewers/MarkdownView.tsx | 20 +++++-------------- .../src/components/viewers/TextView.tsx | 2 +- 4 files changed, 14 insertions(+), 33 deletions(-) diff --git a/packages/components/src/components/viewers/ImageView.tsx b/packages/components/src/components/viewers/ImageView.tsx index b404a66c..282e2e14 100644 --- a/packages/components/src/components/viewers/ImageView.tsx +++ b/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,8 +18,8 @@ interface Content { * Image viewer component. */ export default function ImageView({ source, setError }: ViewerProps) { - const [loading, setLoading] = useState(LoadingState.NotLoaded) const [content, setContent] = useState() + const [isLoading, setIsLoading] = useState(true) const { fileName, resolveUrl, requestInit } = source @@ -50,16 +44,12 @@ 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 - void loadContent() - return LoadingState.Loading - }) + setIsLoading(true) + void loadContent() }, [fileName, resolveUrl, requestInit, setError]) return @@ -68,7 +58,7 @@ export default function ImageView({ source, setError }: ViewerProps) { className='image' src={content.dataUri} />} - {loading &&
} + {isLoading &&
}
} diff --git a/packages/components/src/components/viewers/JsonView.tsx b/packages/components/src/components/viewers/JsonView.tsx index d59b5744..d8ecb9b1 100644 --- a/packages/components/src/components/viewers/JsonView.tsx +++ b/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 @@ -27,7 +27,6 @@ export default function JsonView({ source, setError }: ViewerProps) { useEffect(() => { async function loadContent() { try { - setIsLoading(true) const res = await fetch(resolveUrl, requestInit) const futureText = res.text() if (res.status === 401) { @@ -53,6 +52,8 @@ export default function JsonView({ source, setError }: ViewerProps) { setIsLoading(false) } } + + setIsLoading(true) void loadContent() }, [resolveUrl, requestInit, setError]) diff --git a/packages/components/src/components/viewers/MarkdownView.tsx b/packages/components/src/components/viewers/MarkdownView.tsx index baa201e2..c359991d 100644 --- a/packages/components/src/components/viewers/MarkdownView.tsx +++ b/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,8 +14,8 @@ interface ViewerProps { * Markdown viewer component. */ export default function MarkdownView({ source, setError }: ViewerProps) { - const [loading, setLoading] = useState(LoadingState.NotLoaded) const [content, setContent] = useState() + const [isLoading, setIsLoading] = useState(true) const { resolveUrl, requestInit } = source @@ -43,21 +37,17 @@ 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 - void loadContent() - return LoadingState.Loading - }) + setIsLoading(true) + void loadContent() }, [resolveUrl, requestInit, setError]) return - { loading === LoadingState.Loading &&
} + { isLoading &&
}
} diff --git a/packages/components/src/components/viewers/TextView.tsx b/packages/components/src/components/viewers/TextView.tsx index 7474a14f..5b35872d 100644 --- a/packages/components/src/components/viewers/TextView.tsx +++ b/packages/components/src/components/viewers/TextView.tsx @@ -23,7 +23,6 @@ export default function TextView({ source, setError }: ViewerProps) { 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 @@ -42,6 +41,7 @@ export default function TextView({ source, setError }: ViewerProps) { } } + setIsLoading(true) void loadContent() }, [resolveUrl, requestInit, setError]) From b4e0b954e2f8bb53843bc62f17d9f1e512b6f4fb Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 10:56:35 +0100 Subject: [PATCH 3/8] remove unused loading state --- packages/components/src/components/Cell.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/components/src/components/Cell.tsx b/packages/components/src/components/Cell.tsx index f6bf6f19..30232468 100644 --- a/packages/components/src/components/Cell.tsx +++ b/packages/components/src/components/Cell.tsx @@ -24,7 +24,6 @@ enum LoadingState { * Cell viewer displays a single cell from a table. */ export default function CellView({ source, row, col, config }: CellProps) { - const [loading, setLoading] = useState(LoadingState.NotLoaded) const [text, setText] = useState() const [progress, setProgress] = useState() const [error, setError] = useState() @@ -57,17 +56,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) - void loadCellData() - } - }, [resolveUrl, requestInit, col, row, loading, setError]) + setProgress(0) + void loadCellData() + }, [resolveUrl, requestInit, col, row, setError]) return ( From 0ae62ea7903b0a7b2035e04cc38ea43ec65f9f44 Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 10:56:48 +0100 Subject: [PATCH 4/8] remove unused setProgress prop --- packages/components/src/components/viewers/TextView.tsx | 1 - packages/components/src/components/viewers/Viewer.tsx | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/components/src/components/viewers/TextView.tsx b/packages/components/src/components/viewers/TextView.tsx index 5b35872d..781030aa 100644 --- a/packages/components/src/components/viewers/TextView.tsx +++ b/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 } /** diff --git a/packages/components/src/components/viewers/Viewer.tsx b/packages/components/src/components/viewers/Viewer.tsx index cf87fc69..fe3a7a45 100644 --- a/packages/components/src/components/viewers/Viewer.tsx +++ b/packages/components/src/components/viewers/Viewer.tsx @@ -45,6 +45,6 @@ export default function Viewer({ // Default to text viewer return ( - + ) } From 76866181ebcfcd8a8880c8617245b07554428de5 Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 10:59:19 +0100 Subject: [PATCH 5/8] remove unnecessary dependency --- packages/components/src/components/Cell.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/components/Cell.tsx b/packages/components/src/components/Cell.tsx index 30232468..dc6869e4 100644 --- a/packages/components/src/components/Cell.tsx +++ b/packages/components/src/components/Cell.tsx @@ -62,7 +62,7 @@ export default function CellView({ source, row, col, config }: CellProps) { setProgress(0) void loadCellData() - }, [resolveUrl, requestInit, col, row, setError]) + }, [resolveUrl, requestInit, col, row]) return ( From 70072843f99e4289690a437277cd27d121697395 Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 11:07:08 +0100 Subject: [PATCH 6/8] replace loading with isLoading --- .../src/components/viewers/ParquetView.tsx | 30 +++++++------------ 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/packages/components/src/components/viewers/ParquetView.tsx b/packages/components/src/components/viewers/ParquetView.tsx index 046c809e..4397d265 100644 --- a/packages/components/src/components/viewers/ParquetView.tsx +++ b/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,13 +26,13 @@ interface Content extends ContentSize { * Parquet file viewer */ export default function ParquetView({ source, setProgress, setError, config }: ViewerProps) { - const [loading, setLoading] = useState(LoadingState.NotLoaded) + const [isLoading, setIsLoading] = useState(true) const [content, setContent] = useState() const [cell, setCell] = useState<{ row: number, col: number } | undefined>() - const { resolveUrl, requestInit, sourceId } = source useEffect(() => { async function loadParquetDataFrame() { + const { resolveUrl, requestInit } = source try { setProgress(0.33) const asyncBuffer = await asyncBufferFromUrl({ url: resolveUrl, requestInit }) @@ -52,20 +46,15 @@ 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) - void loadParquetDataFrame() - } - }, [loading, resolveUrl, requestInit, setError, setProgress]) - // Clear loading state on content change - useEffect(() => { - setLoading(LoadingState.NotLoaded) - }, [source]) + setContent(undefined) + setIsLoading(true) + void loadParquetDataFrame() + }, [setError, setProgress, source]) // Close cell view on escape key useEffect(() => { @@ -81,6 +70,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 +98,13 @@ export default function ParquetView({ source, setProgress, setError, config }: V const mainContent = {content?.dataframe && } - {loading === LoadingState.Loading &&
} + {isLoading &&
}
let panelContent From 9844d5c327b77ae8d065bfc68f33ab33f504418f Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 11:10:15 +0100 Subject: [PATCH 7/8] harmonize where we set isLoading --- packages/components/src/components/viewers/ImageView.tsx | 3 +-- packages/components/src/components/viewers/JsonView.tsx | 3 +-- packages/components/src/components/viewers/MarkdownView.tsx | 3 +-- packages/components/src/components/viewers/ParquetView.tsx | 6 ++---- packages/components/src/components/viewers/TextView.tsx | 3 +-- 5 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/components/src/components/viewers/ImageView.tsx b/packages/components/src/components/viewers/ImageView.tsx index 282e2e14..db7a17aa 100644 --- a/packages/components/src/components/viewers/ImageView.tsx +++ b/packages/components/src/components/viewers/ImageView.tsx @@ -26,6 +26,7 @@ export default function ImageView({ source, setError }: ViewerProps) { useEffect(() => { async function loadContent() { try { + setIsLoading(true) const res = await fetch(resolveUrl, requestInit) if (res.status === 401) { const text = await res.text() @@ -47,8 +48,6 @@ export default function ImageView({ source, setError }: ViewerProps) { setIsLoading(false) } } - - setIsLoading(true) void loadContent() }, [fileName, resolveUrl, requestInit, setError]) diff --git a/packages/components/src/components/viewers/JsonView.tsx b/packages/components/src/components/viewers/JsonView.tsx index d8ecb9b1..2dfc6902 100644 --- a/packages/components/src/components/viewers/JsonView.tsx +++ b/packages/components/src/components/viewers/JsonView.tsx @@ -27,6 +27,7 @@ export default function JsonView({ source, setError }: ViewerProps) { useEffect(() => { async function loadContent() { try { + setIsLoading(true) const res = await fetch(resolveUrl, requestInit) const futureText = res.text() if (res.status === 401) { @@ -52,8 +53,6 @@ export default function JsonView({ source, setError }: ViewerProps) { setIsLoading(false) } } - - setIsLoading(true) void loadContent() }, [resolveUrl, requestInit, setError]) diff --git a/packages/components/src/components/viewers/MarkdownView.tsx b/packages/components/src/components/viewers/MarkdownView.tsx index c359991d..37d73995 100644 --- a/packages/components/src/components/viewers/MarkdownView.tsx +++ b/packages/components/src/components/viewers/MarkdownView.tsx @@ -23,6 +23,7 @@ export default function MarkdownView({ source, setError }: ViewerProps) { 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 @@ -40,8 +41,6 @@ export default function MarkdownView({ source, setError }: ViewerProps) { setIsLoading(false) } } - - setIsLoading(true) void loadContent() }, [resolveUrl, requestInit, setError]) diff --git a/packages/components/src/components/viewers/ParquetView.tsx b/packages/components/src/components/viewers/ParquetView.tsx index 4397d265..3ed5e759 100644 --- a/packages/components/src/components/viewers/ParquetView.tsx +++ b/packages/components/src/components/viewers/ParquetView.tsx @@ -32,9 +32,10 @@ export default function ParquetView({ source, setProgress, setError, config }: V useEffect(() => { async function loadParquetDataFrame() { - const { resolveUrl, requestInit } = source 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) @@ -50,9 +51,6 @@ export default function ParquetView({ source, setProgress, setError, config }: V setProgress(1) } } - - setContent(undefined) - setIsLoading(true) void loadParquetDataFrame() }, [setError, setProgress, source]) diff --git a/packages/components/src/components/viewers/TextView.tsx b/packages/components/src/components/viewers/TextView.tsx index 781030aa..04c9d20c 100644 --- a/packages/components/src/components/viewers/TextView.tsx +++ b/packages/components/src/components/viewers/TextView.tsx @@ -22,6 +22,7 @@ export default function TextView({ source, setError }: ViewerProps) { 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 @@ -39,8 +40,6 @@ export default function TextView({ source, setError }: ViewerProps) { setIsLoading(false) } } - - setIsLoading(true) void loadContent() }, [resolveUrl, requestInit, setError]) From 93435a59d10c9e2518cd2044522c2fe4b25d0af3 Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Tue, 4 Mar 2025 11:14:29 +0100 Subject: [PATCH 8/8] fix lint --- packages/components/src/components/Cell.tsx | 6 ------ packages/components/src/components/Json.tsx | 6 +++--- packages/components/test/components/Folder.test.tsx | 6 +++--- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/components/src/components/Cell.tsx b/packages/components/src/components/Cell.tsx index dc6869e4..c6271b0b 100644 --- a/packages/components/src/components/Cell.tsx +++ b/packages/components/src/components/Cell.tsx @@ -14,12 +14,6 @@ interface CellProps { config?: CellConfig } -enum LoadingState { - NotLoaded, - Loading, - Loaded, -} - /** * Cell viewer displays a single cell from a table. */ diff --git a/packages/components/src/components/Json.tsx b/packages/components/src/components/Json.tsx index 82cab97d..92a88baa 100644 --- a/packages/components/src/components/Json.tsx +++ b/packages/components/src/components/Json.tsx @@ -72,11 +72,11 @@ function JsonObject({ obj, label }: { obj: object, label?: string }): ReactNode {'{'}
    - {Object.entries(obj).map(([key, value]) => ( + {Object.entries(obj).map(([key, value]) =>
  • -
  • - ))} + , + )}
{'}'}
diff --git a/packages/components/test/components/Folder.test.tsx b/packages/components/test/components/Folder.test.tsx index 2b15aa91..77debeb1 100644 --- a/packages/components/test/components/Folder.test.tsx +++ b/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/') })