fix(frontend): fix many s3 file picker bugs (#5428)

* fix(frontend): fix many s3 file picker bugs * missing * nit
2025-03-06 10:39:37 +01:00
parent 35e719d6bb
commit 538d00ef9f
2 changed files with 126 additions and 92 deletions
--- a/frontend/src/lib/components/S3FilePicker.svelte
+++ b/frontend/src/lib/components/S3FilePicker.svelte
@@ -23,6 +23,7 @@
 	import ConfirmationModal from './common/confirmationModal/ConfirmationModal.svelte'
 	import FileUploadModal from './common/fileUpload/FileUploadModal.svelte'
 	import PdfViewer from './display/PdfViewer.svelte'
+	import { twMerge } from 'tailwind-merge'

 	let deletionModalOpen = false
 	let fileDeletionInProgress = false
@@ -44,6 +45,7 @@
 	let initialFileKeyInternalCopy: { s3: string }
 	export let selectedFileKey: { s3: string } | undefined = undefined
 	export let folderOnly = false
+	export let regexFilter: RegExp | undefined = undefined

 	let csvSeparatorChar: string = ','
 	let csvHasHeader: boolean = true
@@ -63,6 +65,7 @@
 			collapsed: boolean
 			parentPath: string | undefined
 			nestingLevel: number
+			count: number
 		}
 	> = {}
 	let displayedFileKeys: string[] = []
@@ -93,6 +96,7 @@
 	const maxKeys = 1000

 	let count = 0
+	let displayedCount = 0

 	let filter = ''

@@ -105,6 +109,10 @@
 			timeout && clearTimeout(timeout)
 			timeout = setTimeout(() => {
 				page = 0
+				count = 0
+				displayedCount = 0
+				allFilesByKey = {}
+				displayedFileKeys = []
 				listMarkers = []
 				loadFiles()
 			}, 500)
@@ -113,6 +121,7 @@
 		}
 	}

+	let lastKeyFolders: string[] = []
 	async function loadFiles() {
 		fileListLoading = true
 		let availableFiles = await HelpersService.listStoredFiles({
@@ -132,13 +141,20 @@
 			return
 		}
 		fileListUnavailable = false
-		allFilesByKey = {}
-		displayedFileKeys = []
-		for (let file_path of availableFiles.windmill_large_files) {
+		for (let [index, file_path] of availableFiles.windmill_large_files.entries()) {
+			if (regexFilter && !regexFilter.test(file_path.s3)) {
+				continue
+			}
+			displayedCount += 1
 			let split_path = file_path.s3.split('/')
 			let parent_path: string | undefined = undefined
 			let current_path: string | undefined = undefined
 			let nestingLevel = 0
+
+			if (index === availableFiles.windmill_large_files.length - 1 && split_path.length > 1) {
+				lastKeyFolders = split_path.slice(0, -1)
+			}
+
 			for (let i = 0; i < split_path.length; i++) {
 				parent_path = current_path
 				current_path = current_path === undefined ? split_path[i] : current_path + split_path[i]
@@ -149,6 +165,7 @@

 				nestingLevel = i * 2
 				if (allFilesByKey[current_path] !== undefined) {
+					allFilesByKey[current_path].count += 1
 					continue
 				}
 				allFilesByKey[current_path] = {
@@ -157,7 +174,8 @@
 					display_name: split_path[i],
 					collapsed: true, // folders collapsed by default
 					parentPath: parent_path,
-					nestingLevel: nestingLevel
+					nestingLevel: nestingLevel,
+					count: 1
 				}
 				if (i == 0) {
 					displayedFileKeys.push(current_path)
@@ -165,15 +183,14 @@
 			}
 		}
 		if (listMarkers.length == page) {
-			count = availableFiles.windmill_large_files.length
+			count += availableFiles.windmill_large_files.length
 			const nextMarker =
 				availableFiles.windmill_large_files?.[availableFiles.windmill_large_files.length - 1]?.s3
 			if (nextMarker) listMarkers.push(nextMarker)
 		}
-		displayedFileKeys = displayedFileKeys.sort()

 		// before returning, un-collapse the folders containing the selected file (if any)
-		if (selectedFileKey !== undefined && !emptyString(selectedFileKey.s3)) {
+		if (selectedFileKey !== undefined && !emptyString(selectedFileKey.s3) && page === 0) {
 			let split_path = selectedFileKey.s3.split('/')
 			let current_path: string | undefined = undefined
 			for (let i = 0; i < split_path.length; i++) {
@@ -181,12 +198,19 @@
 				if (i < split_path.length - 1) {
 					current_path += '/'
 				}
-				let indexOf = displayedFileKeys.indexOf(current_path)
-				if (indexOf >= 0) {
-					selectItem(indexOf, true)
+				const folder = allFilesByKey[current_path]
+				if (folder) {
+					folder.collapsed = false
+				}
+				for (let file_key in allFilesByKey) {
+					let file_info = allFilesByKey[file_key]
+					if (file_info.parentPath === current_path) {
+						displayedFileKeys.push(file_key)
+					}
 				}
 			}
 		}
+		displayedFileKeys = displayedFileKeys.sort()
 		fileListLoading = false
 		fileInfoLoading = false
 	}
@@ -262,6 +286,7 @@
 		fileInfoLoading = false
 	}

+	let render = 0
 	async function deleteFileFromS3(fileKey: string | undefined) {
 		fileDeletionInProgress = true
 		if (fileKey === undefined) {
@@ -278,13 +303,12 @@
 			deletionModalOpen = false
 		}
 		sendUserToast(`${fileKey} deleted from S3 bucket`)
-		selectedFileKey = { s3: '' }
-		const idx = displayedFileKeys.indexOf(fileKey)
-		if (idx >= 0) {
-			displayedFileKeys.splice(idx, 1)
-			displayedFileKeys = [...displayedFileKeys]
-		}
+		displayedFileKeys = [...displayedFileKeys.filter((key) => key !== fileKey)]
 		delete allFilesByKey[fileKey]
+		filePreview = undefined
+		fileMetadata = undefined
+		selectedFileKey = { s3: '' }
+		render++
 	}

 	async function moveS3File(srcFileKey: string | undefined, destFileKey: string | undefined) {
@@ -321,6 +345,7 @@
 		displayedFileKeys = []
 		allFilesByKey = {}
 		count = 0
+		displayedCount = 0
 		page = 0
 		filter = ''
 		listMarkers = []
@@ -334,6 +359,7 @@
 		if (initialFileKey !== undefined) {
 			initialFileKeyInternalCopy = { ...initialFileKey }
 		}
+		fileListLoading = true
 		try {
 			await HelpersService.datasetStorageTestConnection({
 				workspace: $workspaceStore!,
@@ -341,6 +367,7 @@
 			})
 			workspaceSettingsInitialized = true
 		} catch (e) {
+			fileListLoading = false
 			console.error('Workspace not connected to object storage: ', e)
 			workspaceSettingsInitialized = false
 			return
@@ -425,7 +452,6 @@
 >
 	<DrawerContent
 		title="S3 file browser"
-		overflow_y={false}
 		on:close={exit}
 		tooltip="Files present in the Workspace S3 bucket. You can set the workspace S3 bucket in the settings."
 		documentationLink="https://www.windmill.dev/docs/integrations/s3"
@@ -477,91 +503,101 @@
 			{/if}
 			<div class="flex flex-row border rounded-md h-full">
 				{#if !fileListUnavailable}
-					<div class="min-w-[30%] border-r h-full flex flex-col">
-						<div class="w-12/12 pb-2 flex flex-row mb-1 gap-1">
-							<input type="text" placeholder="Folder prefix" bind:value={filter} class="text-2xl" />
+					<div class="min-w-[30%] border-r flex flex-col">
+						<div class="w-full p-1 border-b">
+							<input type="text" placeholder="Folder prefix" bind:value={filter} class="text-xl" />
 						</div>
 						{#if fileListLoading === false && displayedFileKeys.length === 0}
 							<div class="p-4 text-tertiary text-xs text-center italic">
 								No files in the workspace S3 bucket at that prefix
 							</div>
 						{:else}
-							<div class="grow max-h-3/4" bind:clientHeight={listDivHeight}>
-								<VirtualList
-									width="100%"
-									height={listDivHeight}
-									itemCount={displayedFileKeys.length}
-									itemSize={42}
-								>
-									<div
-										slot="item"
-										let:index
-										let:style
-										{style}
-										class="hover:bg-surface-hover border"
+							<div class="grow" bind:clientHeight={listDivHeight}>
+								{#key render}
+									<VirtualList
+										width="100%"
+										height={listDivHeight}
+										itemCount={displayedFileKeys.length}
+										itemSize={42}
 									>
-										{@const file_info = allFilesByKey[displayedFileKeys[index]]}
 										<div
-											on:click={() => selectItem(index)}
-											class={`flex flex-row h-full font-semibold text-xs items-center justify-start ${
-												selectedFileKey !== undefined && selectedFileKey.s3 === file_info.full_key
-													? 'bg-surface-hover'
-													: ''
-											} `}
+											slot="item"
+											let:index
+											let:style
+											{style}
+											class={twMerge(
+												'hover:bg-surface-hover border-b',
+												index === displayedFileKeys.length - 1 && 'border-b-0'
+											)}
 										>
-											<div
-												class={`flex flex-row w-full ml-${
-													2 + file_info.nestingLevel
-												} gap-2 h-full items-center`}
-											>
-												{#if file_info.type === 'folder'}
-													{#if file_info.collapsed}<FolderClosed size={16} />{:else}<FolderOpen
-															size={16}
-														/>{/if}
-													<div class="truncate text-ellipsis w-56">
-														{file_info.display_name}
+											{@const file_info = allFilesByKey[displayedFileKeys[index]]}
+											{#if file_info}
+												<div
+													on:click={() => selectItem(index)}
+													class={twMerge(
+														'flex flex-row h-full font-semibold text-xs items-center justify-start',
+														selectedFileKey !== undefined &&
+															selectedFileKey.s3 === file_info.full_key
+															? 'bg-surface-hover'
+															: ''
+													)}
+												>
+													<div
+														class={`flex flex-row w-full gap-2 h-full items-center`}
+														style={`margin-left: ${(2 + file_info.nestingLevel) * 0.25}rem;`}
+													>
+														{#if file_info.type === 'folder'}
+															{#if file_info.collapsed}<FolderClosed size={16} />{:else}<FolderOpen
+																	size={16}
+																/>{/if}
+															<div class="truncate text-ellipsis w-56">
+																{file_info.display_name} ({file_info.count}{count % 1000 === 0 &&
+																lastKeyFolders[file_info.nestingLevel / 2] ===
+																	file_info.display_name
+																	? '+'
+																	: ''} item{file_info.count === 1 ? '' : 's'})
+															</div>
+														{:else}
+															<FileIcon size={16} />
+															<div class="truncate text-ellipsis w-56">
+																{file_info.display_name}
+															</div>
+														{/if}
 													</div>
-												{:else}
-													<FileIcon size={16} />
-													<div class="truncate text-ellipsis w-56">
-														{file_info.display_name}
-													</div>
-												{/if}
-											</div>
+												</div>
+											{/if}
 										</div>
-									</div></VirtualList
-								>
+									</VirtualList>
+								{/key}
 							</div>
 							<div
-								class="flex gap-2 text-2xs items-center text-secondary px-2 w-full h-max-[30px] pt-2 border-t"
+								class="flex flex-col gap-2 text-2xs justify-center items-center text-secondary w-full border-t h-16"
 							>
-								<div>{count} items on this page</div>
-								<div>Page {page + 1}</div>
+								{#if fileListLoading === true}
+									<div class="flex text-secondary mt-1 text-xs justify-center items-center w-full">
+										<Loader2 size={12} class="animate-spin mr-1" /> Loading content
+									</div>
+								{:else}
+									<div>
+										{displayedCount}{count % maxKeys === 0 ? '+' : ''}
+										{displayedCount !== count ? 'filtered ' : ''}items (including inside folders)
+									</div>

-								{#if count == maxKeys}
-									<button
-										class="text-secondary border p-1 underline text-2xs whitespace-nowrap text-center"
-										on:click={() => {
-											page -= 1
-											loadFiles()
-										}}
-										>Previous
-									</button>
-									<button
-										class="text-secondary border p-1 underline text-2xs whitespace-nowrap text-center"
-										on:click={() => {
-											page += 1
-											loadFiles()
-										}}
-										>Next
-									</button>
+									{#if count % maxKeys === 0}
+										<Button
+											variant="border"
+											color="light"
+											size="xs2"
+											on:click={() => {
+												page += 1
+												loadFiles()
+											}}
+										>
+											Load more
+										</Button>
+									{/if}
 								{/if}
 							</div>
-							{#if fileListLoading === true}
-								<div class="flex text-secondary mt-1 text-xs justify-center items-center w-full">
-									<Loader2 size={12} class="animate-spin mr-1" /> Loading content
-								</div>
-							{/if}
 						{/if}
 					</div>
 				{/if}
@@ -578,7 +614,7 @@
 						</div>
 					{:else}
 						<div class="p-4 gap-2">
-							<Section label={fileMetadata.fileKey}>
+							<Section label={fileMetadata.fileKey} breakAll>
 								<div slot="action" class="flex gap-2">
 									{#if filePreview !== undefined}
 										<Button
@@ -689,12 +725,8 @@
 									Previewing a {filePreview.contentType?.toLowerCase()} file.
 								{/if}
 							</div>
-							<pre class="grow whitespace-no-wrap break-words">
-									{#if !emptyString(filePreview.contentPreview)}
-									{filePreview.contentPreview}
-								{:else if filePreview.contentType !== undefined}
-									Preview impossible.
-								{/if}
+							<pre class="grow whitespace-no-wrap break-words"
+								>{#if !emptyString(filePreview.contentPreview)}{filePreview.contentPreview}{:else if filePreview.contentType !== undefined}Preview impossible.{/if}
 							</pre>
 						{/if}
 					</div>
--- a/frontend/src/lib/components/Section.svelte
+++ b/frontend/src/lib/components/Section.svelte
@@ -16,6 +16,7 @@
 	export let collapsed: boolean = true
 	export let headless: boolean = false
 	export let animate: boolean = false
+	export let breakAll: boolean = false
 </script>

 <div class={twMerge('w-full flex flex-col', wrapperClass)}>
@@ -24,6 +25,7 @@
 			<h2
 				class={twMerge(
 					'font-semibold flex flex-row items-center gap-1',
+					breakAll ? 'break-all' : '',
 					small ? 'text-sm' : 'text-base'
 				)}
 			>