From 4c71e84696bc079676c745dd9da1e17fcac3125c Mon Sep 17 00:00:00 2001
From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com>
Date: Thu, 13 Aug 2026 15:14:53 -0600
Subject: [PATCH] ui: two upload dialogs that had drifted apart (#664)
* ui: two upload dialogs that had drifted apart
The dashboard and the file browser each had their own upload component: two
dropzones, two overwrite toggles, two copies of the progress-bar rendering,
against the same `useUploadFile` state. The file browser's dialog was made
fullscreen with a drop target that grows; the dashboard's stayed a `large`
dialog with a strip to drop onto, because it was a different file.
They differ in one thing that is real: the file browser knows the destination
because the user selected it, and the dashboard has to ask. That is now the
`destination` prop -- `undefined` asks, a string uploads there, and '' is the
media directory rather than a missing destination.
Everything else is one component, so the next change to either entry point
lands on both.
* review: an upload dialog that looped on the media directory
Pressing Upload in the file browser with nothing selected passes `''` -- the
media directory. Three checks along that path read it as "no destination":
* The clearing effect ran, and `doClear` is a new function every render whose
own setState scheduled the next one. React ended it with "Maximum update
depth exceeded", 16 warnings deep, on the default Upload path. The effect
now clears only while asking, and `doClear` is stable.
* The title read `Upload to: /media/wrolpi/`.
* `doUpload` bailed, so a drop on the media root uploaded nothing.
All three predate this branch; moving the dialog is what put them behind one
component. Tests cover the title, what the hook is told, and that a fixed
destination is never the thing that empties the queue.
---
app/src/DashboardPage.js | 19 ++--
app/src/components/FileBrowser.js | 154 +-------------------------
app/src/components/Upload.js | 178 ++++++++++++++++++++++--------
app/src/components/Upload.test.js | 90 +++++++++++++++
app/src/hooks/customHooks.js | 11 +-
5 files changed, 238 insertions(+), 214 deletions(-)
create mode 100644 app/src/components/Upload.test.js
diff --git a/app/src/DashboardPage.js b/app/src/DashboardPage.js
index 2ff83051..3faf7a5e 100644
--- a/app/src/DashboardPage.js
+++ b/app/src/DashboardPage.js
@@ -30,7 +30,7 @@ import {ProgressPlaceholder} from "./components/Placeholder";
import {refreshFiles} from "./api";
import _ from "lodash";
import {TagsDashboard} from "./Tags";
-import {Upload} from "./components/Upload";
+import {UploadModal} from "./components/Upload";
import {SearchView, useSearch, useSearchSuggestions} from "./components/Search";
import {OutdatedZimsMessage} from "./components/Zim";
import {useSearchFilter, useSearchRecentFiles, useWROLMode} from "./hooks/customHooks";
@@ -269,16 +269,13 @@ export function Getters() {
;
} else if (selectedGetter === 'upload') {
- getterModal = handleSetGetter(null, null)}
- >
- Upload from your device
-
-
-
- ;
+ // The same dialog the file browser opens; it asks for a destination because, unlike the
+ // browser, the dashboard has no selected directory to upload into.
+ getterModal = handleSetGetter(null, null)}
+ disabled={gettersDisabled}
+ />;
}
if (wrolModeEnabled) {
diff --git a/app/src/components/FileBrowser.js b/app/src/components/FileBrowser.js
index 371e4e27..cd267f67 100644
--- a/app/src/components/FileBrowser.js
+++ b/app/src/components/FileBrowser.js
@@ -7,11 +7,8 @@ import {
IconButton,
IconStack,
Modal,
- Panel,
Placeholder,
- Progress,
Skeleton,
- Stack,
Table,
TextInput,
} from "./ui";
@@ -21,24 +18,22 @@ import {
ErrorMessage,
FileIcon,
humanFileSize,
- mimetypeIconName,
- Toggle,
useIsIgnoredDirectory
} from "./Common";
import React, {useContext, useEffect, useState} from "react";
import {IconArrowsMove, IconChevronsUp, IconCursorText, IconTags} from "@tabler/icons-react";
-import {useDropzone} from "react-dropzone";
import {useHotkeys} from "react-hotkeys-hook";
import {deleteFile, ignoreDirectory, makeDirectory, movePaths, renamePath, unignoreDirectory} from "../api";
import _ from 'lodash';
import {SortableTable} from "./SortableTable";
-import {useBrowseFiles, useElementWidth, useMediaDirectory, useUploadFile, useWROLMode} from "../hooks/customHooks";
+import {useBrowseFiles, useElementWidth, useMediaDirectory, useWROLMode} from "../hooks/customHooks";
import {FileRowTagIcon, FilesRefreshButton} from "./Files";
import {FilePreviewContext} from "./FilePreview";
import {DragSelectionContext, DragSelectionProvider, SettingsContext, ThemeContext} from "../contexts/contexts";
import {BulkTagModal} from "./BulkTagModal";
import {InlineErrorBoundary} from "./ErrorBoundary";
import {TaggedDeleteConfirmModal} from "./TaggedDeleteConfirmModal";
+import {UploadModal} from "./Upload";
function depthIndentation(path) {
// Repeated spaces for every folder a path is in.
@@ -456,7 +451,7 @@ export function FileBrowser() {
disabled={wrolModeEnabled || (selectedPathsCount > 0 && !singleDirectorySelected)}
onClick={() => setUploadOpen(true)}
>{label('Upload')}
- setUploadOpen(false)}
destination={singleDirectorySelected && selectedPaths[0] ? selectedPaths[0].slice(0, -1) : ''}
@@ -847,146 +842,3 @@ export function MakeDirectoryModal({open, onClose, parent, onSubmit}) {
}
-
-export function FileBrowserUploadModal({open, onClose, destination, onComplete}) {
- const mediaDirectory = useMediaDirectory();
-
- const {
- setFiles,
- progresses,
- setDestination,
- doClear,
- tagsSelector,
- overwrite,
- setOverwrite,
- overallProgress,
- inProgress,
- } = useUploadFile();
-
- const onDrop = React.useCallback(async (acceptedFiles) => {
- if (acceptedFiles && acceptedFiles.length > 0) {
- setFiles(acceptedFiles);
- }
- }, [setFiles]);
-
- const {getRootProps, getInputProps} = useDropzone({onDrop, disabled: inProgress});
-
- // Set destination when modal opens or destination changes
- useEffect(() => {
- if (open) {
- setDestination(destination);
- }
- }, [open, destination, setDestination]);
-
- // Clear upload state when destination is cleared
- useEffect(() => {
- if (!destination && open) {
- doClear();
- }
- }, [destination, open, doClear]);
-
- const handleClose = () => {
- if (!inProgress) {
- doClear();
- onClose();
- if (onComplete) {
- onComplete();
- }
- }
- };
-
- let progressBars;
- if (!_.isEmpty(progresses)) {
- progressBars = Object.entries(progresses).map(([name, value]) => {
- const {percent, status, type} = value;
- let color = 'grey';
- let statusString;
- if (status === 'complete') {
- color = 'green';
- statusString = 'Complete:';
- } else if (status === 'pending') {
- statusString = 'Pending:';
- } else if (status === 'failed') {
- color = 'red';
- statusString = 'Failed:';
- } else if (status === 'conflicting') {
- color = 'orange';
- statusString = 'Already Exists:';
- }
-
- return
-
-
-
-
-
- })
- }
-
- const displayPath = destination ? `${mediaDirectory}/${destination}` : mediaDirectory;
-
- /*
- * Fullscreen, for the drop target. At 620px the target was a strip about 90px tall in a
- * dialog covering a fifth of the screen -- a small thing to drag a file onto, and the
- * page behind it was the file browser it came from. Given the room, the target takes it.
- */
- return
- Upload to: {displayPath}
-
-
-
- {/* The target takes the room the dialog gives it rather than sitting as a strip
- at the top of it. `min-height` as well as `flex`, so it stays a large target
- once the progress bars below it start claiming their own space. */}
-
-
}
-
-
- {progressBars}
-
-
-
-
-
-
-}
diff --git a/app/src/components/Upload.js b/app/src/components/Upload.js
index ce59bc3c..f8a7cb76 100644
--- a/app/src/components/Upload.js
+++ b/app/src/components/Upload.js
@@ -1,39 +1,22 @@
import React from "react";
import {useDropzone} from "react-dropzone";
-import {Box, Group, Header, Icon, Panel, Progress, Stack} from "./ui";
+import {Box, Button, Group, Header, Icon, Modal, Panel, Progress, Stack} from "./ui";
import {DirectorySearch, mimetypeIconName, RequiredAsterisk, Toggle} from "./Common";
-import {useUploadFile} from "../hooks/customHooks";
+import {useMediaDirectory, useUploadFile} from "../hooks/customHooks";
import _ from "lodash";
-export function Upload({disabled}) {
- const {
- setFiles,
- progresses,
- destination,
- setDestination,
- doClear,
- tagsSelector,
- overwrite,
- setOverwrite,
- overallProgress,
- inProgress,
- } = useUploadFile();
-
- const onDrop = React.useCallback(async (acceptedFiles) => {
- if (acceptedFiles && acceptedFiles.length > 0) {
- setFiles(acceptedFiles);
- }
- }, [destination]);
-
- const {getRootProps, getInputProps} = useDropzone({onDrop, disabled: inProgress});
-
- React.useEffect(() => {
- if (!destination) {
- // Destination was cleared, clear progresses.
- doClear();
- }
- }, [destination]);
+/*
+ * `destination` says where the files go:
+ *
+ * undefined -- ask, with a DirectorySearch.
+ * a string -- upload there. '' is the media directory, a real destination; a falsy check
+ * against it reads the media root as "nowhere".
+ */
+export function UploadForm({
+ askForDestination, destination, setDestination, disabled, inProgress, progresses, overallProgress,
+ tagsSelector, overwrite, setOverwrite, getRootProps, getInputProps,
+}) {
let progressBars;
if (!_.isEmpty(progresses)) {
progressBars = Object.entries(progresses).map(([name, value]) => {
@@ -62,9 +45,18 @@ export function Upload({disabled}) {
})
}
+ // A destination that was asked for and not yet given: there is nowhere to put a file.
+ const canDrop = !askForDestination || !!destination;
+
+ /*
+ * A fragment, and the `
- {destination ?
-
-
+ {/* The target grows into the dialog's leftover height. `min-height` as well as
+ `flex`, so it stays a large target once progress bars claim their own space. */}
+ {canDrop ?
+
+
-
- Click here, or drop files here to upload
-
+ Click here, or drop files here to upload
- : You must search for a directory to place your files
+ // The instruction takes the room the target would, rather than sitting as a line
+ // above an empty dialog.
+ :
+ You must search for a directory to place your files
+
}
-
+ {progresses && Object.keys(progresses).length > 1 &&
+
+
+
}
- {progresses && Object.keys(progresses).length > 1 && }
-
-
+
{progressBars}
}
+
+/**
+ * The upload dialog. Pass a `destination` to upload into it, or leave it off to have the user
+ * search for one.
+ */
+export function UploadModal({open, onClose, destination, onComplete, disabled}) {
+ const mediaDirectory = useMediaDirectory();
+ const asksForDestination = destination === undefined;
+
+ const upload = useUploadFile();
+ const {setFiles, setDestination, doClear, inProgress} = upload;
+
+ const onDrop = React.useCallback(async (acceptedFiles) => {
+ if (acceptedFiles && acceptedFiles.length > 0) {
+ setFiles(acceptedFiles);
+ }
+ }, [setFiles]);
+
+ const {getRootProps, getInputProps} = useDropzone({onDrop, disabled: inProgress});
+
+ // A fixed destination is the caller's to declare; only the searching form sets its own.
+ React.useEffect(() => {
+ if (open && !asksForDestination) {
+ setDestination(destination);
+ }
+ }, [open, destination, asksForDestination, setDestination]);
+
+ /*
+ * The user cleared their search, so the queued files have nowhere to go.
+ *
+ * Only while asking. `''` is a fixed destination -- the media directory -- and clearing on
+ * it fired this effect on every render, because `doClear` is a new function each time and
+ * its own setState scheduled the next render. React caps that at "Maximum update depth
+ * exceeded"; the file browser reached it whenever Upload was pressed with nothing selected.
+ */
+ React.useEffect(() => {
+ if (asksForDestination && !upload.destination && open) {
+ doClear();
+ }
+ }, [asksForDestination, upload.destination, open, doClear]);
+
+ const handleClose = () => {
+ if (inProgress) return;
+ doClear();
+ onClose();
+ if (onComplete) onComplete();
+ };
+
+ // No trailing slash for the media directory itself.
+ const displayPath = asksForDestination ? null
+ : (destination ? `${mediaDirectory}/${destination}` : mediaDirectory);
+
+ // Fullscreen, so the drop target is something you can drag a file onto without aiming.
+ return
+ {displayPath ? `Upload to: ${displayPath}` : 'Upload from your device'}
+
+
+
+
+
+
+
+}
diff --git a/app/src/components/Upload.test.js b/app/src/components/Upload.test.js
new file mode 100644
index 00000000..153467f1
--- /dev/null
+++ b/app/src/components/Upload.test.js
@@ -0,0 +1,90 @@
+import React from 'react';
+import {screen} from '@testing-library/react';
+import {UploadModal} from './Upload';
+import {renderWithProviders} from '../test-utils';
+
+const mockUseUploadFile = {
+ setFiles: jest.fn(),
+ progresses: {},
+ destination: '',
+ setDestination: jest.fn(),
+ doClear: jest.fn(),
+ tagsSelector: null,
+ overwrite: false,
+ setOverwrite: jest.fn(),
+ overallProgress: 0,
+ inProgress: false,
+};
+
+jest.mock('../hooks/customHooks', () => require('../test-utils').mockModule(
+ jest.requireActual('../hooks/customHooks'),
+ {useUploadFile: () => mockUseUploadFile},
+));
+
+jest.mock('react-dropzone', () => ({
+ useDropzone: () => ({getRootProps: () => ({}), getInputProps: () => ({})}),
+}));
+
+// The real `useMediaDirectory`, reading the settings the providers supply.
+const render = ui => renderWithProviders(ui, {settings: {media_directory: '/media/wrolpi'}});
+
+/*
+ * The dashboard and the file browser open the same dialog, and differ only in whether it asks
+ * where the files go. That fork, and the `''` contract underneath it, are what these pin.
+ */
+describe('one upload dialog, two entry points', () => {
+ beforeEach(() => jest.clearAllMocks());
+
+ it('asks for a destination when the caller has none', () => {
+ render( {}}/>);
+
+ expect(screen.getByText('Destination')).toBeInTheDocument();
+ // Nowhere to put a file yet, so the target is replaced by the instruction.
+ expect(screen.getByText(/must search for a directory/)).toBeInTheDocument();
+ expect(screen.queryByText(/drop files here/)).not.toBeInTheDocument();
+ });
+
+ it('uploads into the destination it is given, and names it', () => {
+ render( {}} destination='videos'/>);
+
+ expect(screen.getByText('Upload to: /media/wrolpi/videos')).toBeInTheDocument();
+ expect(screen.queryByText('Destination')).not.toBeInTheDocument();
+ expect(screen.getByText(/drop files here/)).toBeInTheDocument();
+ expect(mockUseUploadFile.setDestination).toHaveBeenCalledWith('videos');
+ });
+
+ /*
+ * '' is the media directory -- where the file browser uploads when no folder is selected,
+ * which is the state its Upload button is enabled in. Every falsy check along this path
+ * reads it as "no destination", so each is asserted separately: the fork, the title, and
+ * what the hook is told.
+ */
+ it('treats the media directory as a destination, not a missing one', () => {
+ render( {}} destination=''/>);
+
+ expect(screen.queryByText('Destination')).not.toBeInTheDocument();
+ expect(screen.getByText(/drop files here/)).toBeInTheDocument();
+ expect(screen.getByText('Upload to: /media/wrolpi')).toBeInTheDocument();
+ expect(mockUseUploadFile.setDestination).toHaveBeenCalledWith('');
+ });
+
+ /*
+ * The clearing effect ran on every render for a `''` destination, and `doClear` sets state,
+ * so each run scheduled the render that ran it again -- "Maximum update depth exceeded" on
+ * the file browser's default Upload. A fixed destination is never cleared, so it is never
+ * the thing that empties the queue.
+ */
+ it('does not clear the queue it is about to upload', () => {
+ const {rerender} = render( {}} destination=''/>);
+ rerender( {}} destination=''/>);
+
+ expect(mockUseUploadFile.doClear).not.toHaveBeenCalled();
+ });
+
+ // Asking is the case that does clear: the user emptied their own search.
+ it('clears the queue when the user clears their search', () => {
+ render( {}}/>);
+
+ expect(mockUseUploadFile.doClear).toHaveBeenCalled();
+ });
+});
diff --git a/app/src/hooks/customHooks.js b/app/src/hooks/customHooks.js
index 0e4a605a..35a98e5d 100644
--- a/app/src/hooks/customHooks.js
+++ b/app/src/hooks/customHooks.js
@@ -1788,7 +1788,9 @@ export const useUploadFile = () => {
}
const doUpload = async () => {
- if (!files || files.length === 0 || !destination) {
+ // `''` is the media directory, which the file browser uploads to when no folder is
+ // selected; only a missing destination means there is nowhere to put the files.
+ if (!files || files.length === 0 || destination === null || destination === undefined) {
return;
}
@@ -1890,14 +1892,17 @@ export const useUploadFile = () => {
setInProgress(false);
}
- const doClear = () => {
+ // Stable across renders. Callers put it in effect dependencies, and every call sets state
+ // with fresh objects -- an unstable identity there re-runs the effect on the render its own
+ // setState caused, which React ends with "Maximum update depth exceeded".
+ const doClear = React.useCallback(() => {
setFiles([]);
setProgresses({});
setTotalSize(0);
setUploadedSize(0);
setOverallProgress(0);
setInProgress(false);
- }
+ }, []);
useEffect(() => {
doUpload()