Add editor file upload support#347
Merged
Merged
Conversation
Reviewer's GuideImplements resumable, deduplicated multipart uploads persisted in IndexedDB and wires all editor file/image/PDF/paste flows to use background uploads with local fallbacks, plus adds URL helpers and tooling for IndexedDB debugging and Playwright coverage. Sequence diagram for resumable multipart upload with persistencesequenceDiagram
actor User
participant Editor
participant UploadService as uploadFileMultipart
participant Store as multipartUploadStore
participant FileAPI as FileStorageAPI
User->>Editor: trigger file upload
Editor->>UploadService: uploadFileMultipart(workspaceId, viewId, file, onProgress)
UploadService->>UploadService: getActiveUploadKey / dedupe activeUploads
UploadService->>Store: getSession(workspaceId, viewId, file)
alt existing session
Store-->>UploadService: PersistedMultipartUpload
else new session
UploadService->>FileAPI: createMultipartUpload(workspaceId, parentDir, file, fileId)
FileAPI-->>UploadService: { upload_id, file_id }
UploadService->>Store: saveSession(session)
end
UploadService->>UploadService: createChunks(file)
UploadService->>UploadService: syncUploadedParts(session)
UploadService->>FileAPI: listUploadedParts(workspaceId, parentDir, fileId, uploadId)
FileAPI-->>UploadService: UploadPartInfo[]
loop missing chunks
UploadService->>FileAPI: uploadPart(...)
FileAPI-->>UploadService: UploadPartInfo
UploadService->>Store: saveSession(updatedSession)
UploadService-->>Editor: onProgress(phase="uploading")
end
UploadService-->>Editor: onProgress(phase="completing")
UploadService->>FileAPI: completeMultipartUpload(..., parts)
FileAPI-->>UploadService: fileUrl
UploadService->>Store: deleteSession(session.id)
UploadService-->>Editor: fileUrl
Editor-->>User: file inserted with remote URL
rect rgb(255,230,230)
UploadService->>FileAPI: [isStaleSessionError]
UploadService->>FileAPI: abortMultipartUpload(...)
UploadService->>Store: deleteSession(session.id)
end
Sequence diagram for editor paste flow with background uploadsequenceDiagram
actor User
participant Editor
participant WithInsert as withInsertData
participant FileHandler
participant UploadFn as editor.uploadFile
participant Blocks as CustomEditor
User->>Editor: paste files
Editor->>WithInsert: insertData(fileArray)
WithInsert->>FileHandler: handleFileUpload(file[])
FileHandler-->>WithInsert: { id, url }[]
WithInsert->>Blocks: addBelowBlock(..., ImageBlock/FileBlock, { url: '', retry_local_url })
WithInsert-->>Editor: blocks with local placeholders
par background uploads
WithInsert->>UploadFn: uploadFile(file)
UploadFn-->>WithInsert: remote url
WithInsert->>FileHandler: cleanup(retry_local_url)
WithInsert->>Blocks: setBlockData(blockId, { url, retry_local_url: '' })
and failure / changed block
UploadFn-->>WithInsert: error or undefined
WithInsert->>Blocks: [skip update if url set or retry_local_url changed]
end
Editor-->>User: document shows blocks, URLs filled as uploads complete
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
uploadFileMultipartInternal,uploadedBytesis mutated inside anexecuteWithConcurrencyloop and can suffer from lost updates under concurrent callbacks; consider using a per-chunk progress accumulator or passing a progress callback intoexecuteWithConcurrencyinstead of a shared mutable counter. - The
activeUploadsmap keyed byworkspaceId:viewIdis never cleaned up when its correspondingWeakMapbecomes empty; if many distinct destinations are used over time this can lead to unbounded growth of the outer map, so consider deleting the destKey when the last upload finishes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `uploadFileMultipartInternal`, `uploadedBytes` is mutated inside an `executeWithConcurrency` loop and can suffer from lost updates under concurrent callbacks; consider using a per-chunk progress accumulator or passing a progress callback into `executeWithConcurrency` instead of a shared mutable counter.
- The `activeUploads` map keyed by `workspaceId:viewId` is never cleaned up when its corresponding `WeakMap` becomes empty; if many distinct destinations are used over time this can lead to unbounded growth of the outer map, so consider deleting the destKey when the last upload finishes.
## Individual Comments
### Comment 1
<location path="src/application/services/js-services/http/multipart-upload.ts" line_range="44-53" />
<code_context>
+// sharing metadata never collide; the same File uploaded to different
+// workspace/view destinations also stays independent. A duplicate dispatch with
+// the *same* File instance and *same* destination still dedupes.
+const activeUploads = new Map<string, WeakMap<File, Promise<string>>>();
+
+function getActiveUploadKey(workspaceId: string, viewId: string): string {
</code_context>
<issue_to_address>
**suggestion (performance):** Consider cleaning up empty destination maps in `activeUploads` to avoid long‑lived entries.
`uploadFileMultipart` clears the `File` from the `WeakMap` in `finally`, but `activeUploads` keeps the `destKey` forever:
```ts
const destKey = getActiveUploadKey(workspaceId, viewId);
...
const upload = uploadFileMultipartInternal(...).finally(() => {
activeUploads.get(destKey)?.delete(file);
});
```
You can remove empty destination maps in the same `finally` block:
```ts
const upload = uploadFileMultipartInternal(...).finally(() => {
const map = activeUploads.get(destKey);
map?.delete(file);
if (map && !Array.from(map.keys()).length) {
activeUploads.delete(destKey);
}
});
```
Because `WeakMap` doesn’t expose `size`, consider a small wrapper or companion `Set` to track keys if you want an O(1) emptiness check.
</issue_to_address>
### Comment 2
<location path="src/components/editor/plugins/withInsertData.ts" line_range="136" />
<code_context>
- fileId = res.id;
- }
-
+ for (let i = 0; i < fileArray.length; i++) {
+ const file = fileArray[i];
+ const fileId = fileIds[i];
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the placeholder creation and async upload-finalization logic into dedicated helpers so the paste handler loop reads as simple orchestration over descriptors and promises.
You can reduce the cognitive load by extracting the repeated “placeholder + async finalize” logic into small helpers. That keeps the paste handler focused on orchestration and makes the image/file branches much clearer.
For example, you can:
1. Extract a helper to create the placeholder block and its “descriptor”.
2. Extract a helper to finalize the upload for a descriptor (including the stale‑block guards).
3. Keep the main paste handler as a simple loop building descriptors + `Promise.all`.
### 1. Extract placeholder creation
```ts
type PastedFileDescriptor = {
blockId: string;
file: File;
fileId: string; // retry_local_url
isImage: boolean;
};
function createPastedFilePlaceholder(
editor: Editor,
prevBlockId: string,
file: File,
fileId: string
): PastedFileDescriptor | null {
const isImage = file.type.startsWith('image/');
let insertedBlockId: string | undefined;
if (isImage) {
const data: ImageBlockData = {
url: '',
image_type: undefined,
retry_local_url: fileId,
};
insertedBlockId = CustomEditor.addBelowBlock(editor, prevBlockId, BlockType.ImageBlock, data);
} else {
const data: FileBlockData = {
url: '',
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: fileId,
};
insertedBlockId = CustomEditor.addBelowBlock(editor, prevBlockId, BlockType.FileBlock, data);
}
if (!insertedBlockId) return null;
return {
blockId: insertedBlockId,
file,
fileId,
isImage,
};
}
```
Then in the loop:
```ts
const descriptors: PastedFileDescriptor[] = [];
for (let i = 0; i < fileArray.length; i++) {
const file = fileArray[i];
const fileId = fileIds[i];
const descriptor = createPastedFilePlaceholder(e, newBlockId, file, fileId);
if (!descriptor) continue;
descriptors.push(descriptor);
newBlockId = descriptor.blockId;
}
```
### 2. Extract upload finalization
You can encapsulate the nested async block (including stale block checks) into a reusable helper:
```ts
async function finalizePastedFileUpload(
editor: Editor,
fileHandler: FileHandler,
descriptor: PastedFileDescriptor,
uploadFile?: (file: File) => Promise<string | undefined>
): Promise<void> {
const { blockId, file, fileId, isImage } = descriptor;
let url: string | undefined;
try {
url = await uploadFile?.(file);
} catch {
return;
}
if (!url) return;
if (fileId) {
void fileHandler.cleanup(fileId).catch(() => undefined);
}
let currentData: { url?: string; retry_local_url?: string } | undefined;
try {
const entry = findSlateEntryByBlockId(editor, blockId);
currentData = entry
? ((entry[0] as { data?: { url?: string; retry_local_url?: string } }).data ?? undefined)
: undefined;
} catch {
return;
}
if (!currentData) return;
if (currentData.url) return;
if ((currentData.retry_local_url ?? '') !== fileId) return;
if (isImage) {
CustomEditor.setBlockData(editor, blockId, {
url,
image_type: ImageType.External,
retry_local_url: '',
} as ImageBlockData);
} else {
CustomEditor.setBlockData(editor, blockId, {
url,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: '',
} as FileBlockData);
}
}
```
Then the paste handler’s async part becomes something like:
```ts
const descriptors: PastedFileDescriptor[] = []; // built as above
const pendingUploads = descriptors.map((descriptor) =>
finalizePastedFileUpload(e, fileHandler, descriptor, e.uploadFile)
);
void Promise.all(pendingUploads).catch((err) => {
Log.warn('withInsertData: failed to finalize pasted file upload', err);
});
```
This keeps all existing behavior (local snapshot, deferred upload, stale‑block guards) but flattens the control flow and removes the deeply nested `pendingUploads.push((async () => { ... })())` block from the paste handler.
</issue_to_address>
### Comment 3
<location path="src/components/editor/components/block-popover/FileBlockPopoverContent.tsx" line_range="106" />
<code_context>
+ await fileHandler.cleanup(retryLocalUrl).catch(() => undefined);
+ }, []);
+
+ const uploadIntoFileBlock = useCallback(
+ async (targetBlockId: string, file: File, pendingData: FileBlockData) => {
const url = await uploadFileRemote(file);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the stale-block guard and pending-upload handling into small helpers to keep the popover’s upload flow flatter and easier to read.
The new logic in `uploadIntoFileBlock` / `handleChangeUploadFiles` does increase complexity, mostly by inlining a reusable pattern (pending data + remote upload + guarded block update). You can keep all behavior but centralize that pattern to simplify the popover.
### 1. Extract guarded block-update into a helper
All the “stale-block guard” logic can live in a small helper that takes a callback to produce final data:
```ts
function withLiveFileBlock(
editor: Editor,
blockId: string,
pendingData: FileBlockData,
update: (current: FileBlockData) => FileBlockData | void
) {
let entry: ReturnType<typeof findSlateEntryByBlockId>;
try {
entry = findSlateEntryByBlockId(editor, blockId);
} catch {
return;
}
const currentData = entry
? ((entry[0] as { data?: FileBlockData }).data ?? undefined)
: undefined;
if (!currentData) return;
if (currentData.url) return;
if ((currentData.retry_local_url ?? '') !== (pendingData.retry_local_url ?? '')) return;
const next = update(currentData);
if (!next) return;
CustomEditor.setBlockData(editor, blockId, next);
}
```
Then `uploadIntoFileBlock` becomes much flatter:
```ts
const uploadIntoFileBlock = useCallback(
async (targetBlockId: string, file: File, pendingData: FileBlockData) => {
const url = await uploadFileRemote(file);
if (!url) return;
await cleanupLocalFile(pendingData.retry_local_url);
withLiveFileBlock(editor, targetBlockId, pendingData, () => ({
url,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: '',
}));
},
[cleanupLocalFile, editor, uploadFileRemote]
);
```
This removes the nested `try/catch` and repeated guard checks from the core async flow, making it easier to read and reuse in other popovers.
### 2. Encapsulate “pending upload” object
You can also reduce manual `[file, data]` pairing in `handleChangeUploadFiles` by working with a small structured type:
```ts
type PendingFileBlock = {
file: File;
data: FileBlockData;
};
const handleChangeUploadFiles = useCallback(
async (files: File[]) => {
if (!files.length) return;
setUploading(true);
try {
const datas = await Promise.all(files.map((f) => getData(f)));
const [primary, ...rest] = datas.map<PendingFileBlock>((data, i) => ({
file: files[i],
data,
}));
CustomEditor.setBlockData(editor, blockId, primary.data);
const pendingUploads: Promise<void>[] = [
uploadIntoFileBlock(blockId, primary.file, primary.data),
];
for (const { file, data } of rest.slice().reverse()) {
const newId = CustomEditor.addBelowBlock(editor, blockId, BlockType.FileBlock, data);
if (newId) {
pendingUploads.push(uploadIntoFileBlock(newId, file, data));
}
}
onClose();
await Promise.all(pendingUploads);
} finally {
setUploading(false);
}
},
[blockId, editor, getData, onClose, uploadIntoFileBlock]
);
```
This keeps the ordering logic as-is but avoids ad-hoc `files` / `otherDatas[i]` indexing, making it more obvious what each element represents.
Both helpers are small, local, and reusable in image/PDF popovers later, but already make this popover’s core flow easier to follow without changing behavior.
</issue_to_address>
### Comment 4
<location path="src/components/editor/components/block-popover/PDFBlockPopoverContent.tsx" line_range="73" />
<code_context>
);
- const processFileUpload = useCallback(
+ const createPendingFileData = useCallback(
async (file: File): Promise<PDFBlockData> => {
- const url = await uploadFileRemote(file);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new deferred PDF upload flow into a shared helper so this component only supplies PDF‑specific data-building logic.
The new `createPendingFileData` / `cleanupLocalFile` / `uploadIntoPdfBlock` / `handleChangeUploadFiles` flow is clearer than before for PDFs, but it does re‑implement a pattern that already exists in file/image popovers and the paste handler. You can reduce complexity and future maintenance cost by extracting the common “deferred upload into a block” flow into a shared helper and then using it here.
### Suggested direction: shared deferred‑upload helper
Centralize the “create pending data → insert/update block → upload → guard stale block → cleanup” pattern in a utility, then use it from this component (and others later).
For example, a minimal, focused helper in a shared module:
```ts
// uploadHelpers.ts
type PendingData = { retry_local_url?: string; url?: string; name?: string };
type DeferredUploadParams<TData extends PendingData> = {
editor: Editor;
blockId: string;
blockType: BlockType;
files: File[];
createPendingData: (file: File) => Promise<TData>;
uploadRemote: (file: File) => Promise<string | undefined>;
cleanupLocal: (retryLocalUrl?: string) => Promise<void>;
// Allows caller to build final block data shape
buildFinalData: (file: File, url: string, pending: TData) => TData;
};
export async function enqueueDeferredUpload<TData extends PendingData>({
editor,
blockId,
blockType,
files,
createPendingData,
uploadRemote,
cleanupLocal,
buildFinalData,
}: DeferredUploadParams<TData>): Promise<void> {
if (!files.length) return;
const [primaryData, ...otherDatas] = await Promise.all(
files.map((f) => createPendingData(f))
);
const [primaryFile, ...otherFiles] = files;
CustomEditor.setBlockData(editor, blockId, primaryData);
const pendingUploads: Promise<void>[] = [];
const uploadIntoBlock = async (targetBlockId: string, file: File, pending: TData) => {
const url = await uploadRemote(file);
if (!url) return;
await cleanupLocal(pending.retry_local_url);
let currentData: TData | undefined;
try {
const entry = findSlateEntryByBlockId(editor, targetBlockId);
currentData = entry ? ((entry[0] as { data?: TData }).data ?? undefined) : undefined;
} catch {
return;
}
if (!currentData) return;
if (currentData.url) return;
if ((currentData.retry_local_url ?? '') !== (pending.retry_local_url ?? '')) return;
CustomEditor.setBlockData(editor, targetBlockId, buildFinalData(file, url, pending));
};
pendingUploads.push(uploadIntoBlock(blockId, primaryFile, primaryData));
const reversedPairs = otherFiles.map((f, i) => [f, otherDatas[i]] as const).reverse();
for (const [f, d] of reversedPairs) {
const newId = CustomEditor.addBelowBlock(editor, blockId, blockType, d);
if (newId) {
pendingUploads.push(uploadIntoBlock(newId, f, d));
}
}
await Promise.all(pendingUploads);
}
```
Then your PDF popover becomes simpler and focused on PDF‑specific data:
```ts
const createPendingFileData = useCallback(async (file: File): Promise<PDFBlockData> => {
const data: PDFBlockData = {
url: undefined,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
};
try {
const fileHandler = new FileHandler();
const res = await fileHandler.handleFileUpload(file);
URL.revokeObjectURL(res.url);
data.retry_local_url = res.id;
} catch {
data.retry_local_url = '';
}
return data;
}, []);
const cleanupLocalFile = useCallback(async (retryLocalUrl?: string) => {
if (!retryLocalUrl) return;
const fileHandler = new FileHandler();
await fileHandler.cleanup(retryLocalUrl).catch(() => undefined);
}, []);
const handleChangeUploadFiles = useCallback(
async (files: File[]) => {
if (!files.length) return;
setUploading(true);
try {
await enqueueDeferredUpload<PDFBlockData>({
editor,
blockId,
blockType: BlockType.PDFBlock,
files,
createPendingData: createPendingFileData,
uploadRemote: uploadFileRemote,
cleanupLocal: cleanupLocalFile,
buildFinalData: (file, url, pending) => ({
...pending,
url,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: '',
}),
});
onClose();
} finally {
setUploading(false);
}
},
[blockId, editor, onClose, createPendingFileData, uploadFileRemote, cleanupLocalFile]
);
```
This keeps all of your current behavior (parallel snapshotting, retry_local_url guards, cleanup, ordering) but:
- Concentrates the complex guard/cleanup/upload logic in one place.
- Makes the PDF popover (and the file/image popovers later) mostly about “how to build pending/final data”, reducing duplication and cognitive load.
</issue_to_address>
### Comment 5
<location path="src/components/editor/components/block-popover/ImageBlockPopoverContent.tsx" line_range="133" />
<code_context>
+ [cleanupLocalFile, editor, uploadFileRemote]
);
const handleChangeUploadFiles = useCallback(
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the upload orchestration and placeholder insertion logic from `handleChangeUploadFiles` into a reusable helper to keep this handler focused on layout and UX concerns.
You can reduce the cognitive load in `handleChangeUploadFiles` (and make this reusable with file/PDF popovers) by extracting the placeholder+upload orchestration into a small helper that encapsulates:
- `getData` for each file
- creating/updating image blocks in order
- wiring `pendingUploads` via `uploadIntoImageBlock`
That keeps `handleChangeUploadFiles` focused on *where* to insert blocks and scroll/focus behavior, not *how* uploads work.
### Suggested refactor
Extract a helper (can live in a shared module with file/PDF versions) that returns the last block and pending uploads:
```ts
type InsertResult = {
lastBlockId?: string;
pendingUploads: Promise<void>[];
};
function insertImageBlocksWithUploads(
editor: Editor,
anchorBlockId: string,
files: File[],
getData: (file: File) => Promise<ImageBlockData>,
uploadIntoImageBlock: (targetBlockId: string, file: File, pendingData: ImageBlockData) => Promise<void>,
): Promise<InsertResult> {
return (async () => {
if (!files.length) return { lastBlockId: anchorBlockId, pendingUploads: [] };
const [primaryData, ...otherDatas] = await Promise.all(files.map(getData));
const [file, ...otherFiles] = files;
CustomEditor.setBlockData(editor, anchorBlockId, primaryData);
let belowBlockId: string | undefined = anchorBlockId;
const pendingUploads: Promise<void>[] = [
uploadIntoImageBlock(anchorBlockId, file, primaryData),
];
for (let i = 0; i < otherFiles.length; i++) {
const f = otherFiles[i];
const d = otherDatas[i];
const newId = belowBlockId
? CustomEditor.addBelowBlock(editor, belowBlockId, BlockType.ImageBlock, d)
: undefined;
if (newId) {
belowBlockId = newId;
pendingUploads.push(uploadIntoImageBlock(newId, f, d));
}
}
return { lastBlockId: belowBlockId, pendingUploads };
})();
}
```
Then `handleChangeUploadFiles` becomes a lot flatter:
```ts
const handleChangeUploadFiles = useCallback(
async (files: File[]) => {
if (!files.length) return;
setUploading(true);
try {
const { lastBlockId, pendingUploads } = await insertImageBlocksWithUploads(
editor,
blockId,
files,
getData,
uploadIntoImageBlock,
);
if (!lastBlockId) return;
const paragraphId = CustomEditor.addBelowBlock(
editor,
lastBlockId,
BlockType.Paragraph,
{},
);
const entry = paragraphId ? findSlateEntryByBlockId(editor, paragraphId) : null;
if (!entry) return;
const [node, path] = entry;
onClose();
if (path) {
editor.select(editor.start(path));
}
setTimeout(() => {
if (!node) return;
const el = ReactEditor.toDOMNode(editor, node);
el?.scrollIntoView({ behavior: 'smooth', block: 'center' });
}, 250);
await Promise.all(pendingUploads);
} finally {
setUploading(false);
}
},
[blockId, editor, getData, onClose, uploadIntoImageBlock],
);
```
Benefits:
- `handleChangeUploadFiles` now only handles:
- early return
- calling a single helper
- paragraph insertion + selection + scrolling
- waiting for uploads
- Upload orchestration (placeholder creation, ordering, `pendingUploads`) is isolated and can be reused for file/PDF popovers by adjusting block type and data type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+44
to
+53
| const activeUploads = new Map<string, WeakMap<File, Promise<string>>>(); | ||
|
|
||
| function getActiveUploadKey(workspaceId: string, viewId: string): string { | ||
| return `${workspaceId}:${viewId}`; | ||
| } | ||
|
|
||
| function isStaleSessionError(error: unknown): boolean { | ||
| if (typeof error !== 'object' || error === null) return false; | ||
| const e = error as { | ||
| response?: { status?: number; data?: { code?: number; message?: string } }; |
There was a problem hiding this comment.
suggestion (performance): Consider cleaning up empty destination maps in activeUploads to avoid long‑lived entries.
uploadFileMultipart clears the File from the WeakMap in finally, but activeUploads keeps the destKey forever:
const destKey = getActiveUploadKey(workspaceId, viewId);
...
const upload = uploadFileMultipartInternal(...).finally(() => {
activeUploads.get(destKey)?.delete(file);
});You can remove empty destination maps in the same finally block:
const upload = uploadFileMultipartInternal(...).finally(() => {
const map = activeUploads.get(destKey);
map?.delete(file);
if (map && !Array.from(map.keys()).length) {
activeUploads.delete(destKey);
}
});Because WeakMap doesn’t expose size, consider a small wrapper or companion Set to track keys if you want an O(1) emptiness check.
| fileId = res.id; | ||
| } | ||
|
|
||
| for (let i = 0; i < fileArray.length; i++) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the placeholder creation and async upload-finalization logic into dedicated helpers so the paste handler loop reads as simple orchestration over descriptors and promises.
You can reduce the cognitive load by extracting the repeated “placeholder + async finalize” logic into small helpers. That keeps the paste handler focused on orchestration and makes the image/file branches much clearer.
For example, you can:
- Extract a helper to create the placeholder block and its “descriptor”.
- Extract a helper to finalize the upload for a descriptor (including the stale‑block guards).
- Keep the main paste handler as a simple loop building descriptors +
Promise.all.
1. Extract placeholder creation
type PastedFileDescriptor = {
blockId: string;
file: File;
fileId: string; // retry_local_url
isImage: boolean;
};
function createPastedFilePlaceholder(
editor: Editor,
prevBlockId: string,
file: File,
fileId: string
): PastedFileDescriptor | null {
const isImage = file.type.startsWith('image/');
let insertedBlockId: string | undefined;
if (isImage) {
const data: ImageBlockData = {
url: '',
image_type: undefined,
retry_local_url: fileId,
};
insertedBlockId = CustomEditor.addBelowBlock(editor, prevBlockId, BlockType.ImageBlock, data);
} else {
const data: FileBlockData = {
url: '',
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: fileId,
};
insertedBlockId = CustomEditor.addBelowBlock(editor, prevBlockId, BlockType.FileBlock, data);
}
if (!insertedBlockId) return null;
return {
blockId: insertedBlockId,
file,
fileId,
isImage,
};
}Then in the loop:
const descriptors: PastedFileDescriptor[] = [];
for (let i = 0; i < fileArray.length; i++) {
const file = fileArray[i];
const fileId = fileIds[i];
const descriptor = createPastedFilePlaceholder(e, newBlockId, file, fileId);
if (!descriptor) continue;
descriptors.push(descriptor);
newBlockId = descriptor.blockId;
}2. Extract upload finalization
You can encapsulate the nested async block (including stale block checks) into a reusable helper:
async function finalizePastedFileUpload(
editor: Editor,
fileHandler: FileHandler,
descriptor: PastedFileDescriptor,
uploadFile?: (file: File) => Promise<string | undefined>
): Promise<void> {
const { blockId, file, fileId, isImage } = descriptor;
let url: string | undefined;
try {
url = await uploadFile?.(file);
} catch {
return;
}
if (!url) return;
if (fileId) {
void fileHandler.cleanup(fileId).catch(() => undefined);
}
let currentData: { url?: string; retry_local_url?: string } | undefined;
try {
const entry = findSlateEntryByBlockId(editor, blockId);
currentData = entry
? ((entry[0] as { data?: { url?: string; retry_local_url?: string } }).data ?? undefined)
: undefined;
} catch {
return;
}
if (!currentData) return;
if (currentData.url) return;
if ((currentData.retry_local_url ?? '') !== fileId) return;
if (isImage) {
CustomEditor.setBlockData(editor, blockId, {
url,
image_type: ImageType.External,
retry_local_url: '',
} as ImageBlockData);
} else {
CustomEditor.setBlockData(editor, blockId, {
url,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: '',
} as FileBlockData);
}
}Then the paste handler’s async part becomes something like:
const descriptors: PastedFileDescriptor[] = []; // built as above
const pendingUploads = descriptors.map((descriptor) =>
finalizePastedFileUpload(e, fileHandler, descriptor, e.uploadFile)
);
void Promise.all(pendingUploads).catch((err) => {
Log.warn('withInsertData: failed to finalize pasted file upload', err);
});This keeps all existing behavior (local snapshot, deferred upload, stale‑block guards) but flattens the control flow and removes the deeply nested pendingUploads.push((async () => { ... })()) block from the paste handler.
| await fileHandler.cleanup(retryLocalUrl).catch(() => undefined); | ||
| }, []); | ||
|
|
||
| const uploadIntoFileBlock = useCallback( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the stale-block guard and pending-upload handling into small helpers to keep the popover’s upload flow flatter and easier to read.
The new logic in uploadIntoFileBlock / handleChangeUploadFiles does increase complexity, mostly by inlining a reusable pattern (pending data + remote upload + guarded block update). You can keep all behavior but centralize that pattern to simplify the popover.
1. Extract guarded block-update into a helper
All the “stale-block guard” logic can live in a small helper that takes a callback to produce final data:
function withLiveFileBlock(
editor: Editor,
blockId: string,
pendingData: FileBlockData,
update: (current: FileBlockData) => FileBlockData | void
) {
let entry: ReturnType<typeof findSlateEntryByBlockId>;
try {
entry = findSlateEntryByBlockId(editor, blockId);
} catch {
return;
}
const currentData = entry
? ((entry[0] as { data?: FileBlockData }).data ?? undefined)
: undefined;
if (!currentData) return;
if (currentData.url) return;
if ((currentData.retry_local_url ?? '') !== (pendingData.retry_local_url ?? '')) return;
const next = update(currentData);
if (!next) return;
CustomEditor.setBlockData(editor, blockId, next);
}Then uploadIntoFileBlock becomes much flatter:
const uploadIntoFileBlock = useCallback(
async (targetBlockId: string, file: File, pendingData: FileBlockData) => {
const url = await uploadFileRemote(file);
if (!url) return;
await cleanupLocalFile(pendingData.retry_local_url);
withLiveFileBlock(editor, targetBlockId, pendingData, () => ({
url,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: '',
}));
},
[cleanupLocalFile, editor, uploadFileRemote]
);This removes the nested try/catch and repeated guard checks from the core async flow, making it easier to read and reuse in other popovers.
2. Encapsulate “pending upload” object
You can also reduce manual [file, data] pairing in handleChangeUploadFiles by working with a small structured type:
type PendingFileBlock = {
file: File;
data: FileBlockData;
};
const handleChangeUploadFiles = useCallback(
async (files: File[]) => {
if (!files.length) return;
setUploading(true);
try {
const datas = await Promise.all(files.map((f) => getData(f)));
const [primary, ...rest] = datas.map<PendingFileBlock>((data, i) => ({
file: files[i],
data,
}));
CustomEditor.setBlockData(editor, blockId, primary.data);
const pendingUploads: Promise<void>[] = [
uploadIntoFileBlock(blockId, primary.file, primary.data),
];
for (const { file, data } of rest.slice().reverse()) {
const newId = CustomEditor.addBelowBlock(editor, blockId, BlockType.FileBlock, data);
if (newId) {
pendingUploads.push(uploadIntoFileBlock(newId, file, data));
}
}
onClose();
await Promise.all(pendingUploads);
} finally {
setUploading(false);
}
},
[blockId, editor, getData, onClose, uploadIntoFileBlock]
);This keeps the ordering logic as-is but avoids ad-hoc files / otherDatas[i] indexing, making it more obvious what each element represents.
Both helpers are small, local, and reusable in image/PDF popovers later, but already make this popover’s core flow easier to follow without changing behavior.
| ); | ||
|
|
||
| const processFileUpload = useCallback( | ||
| const createPendingFileData = useCallback( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the new deferred PDF upload flow into a shared helper so this component only supplies PDF‑specific data-building logic.
The new createPendingFileData / cleanupLocalFile / uploadIntoPdfBlock / handleChangeUploadFiles flow is clearer than before for PDFs, but it does re‑implement a pattern that already exists in file/image popovers and the paste handler. You can reduce complexity and future maintenance cost by extracting the common “deferred upload into a block” flow into a shared helper and then using it here.
Suggested direction: shared deferred‑upload helper
Centralize the “create pending data → insert/update block → upload → guard stale block → cleanup” pattern in a utility, then use it from this component (and others later).
For example, a minimal, focused helper in a shared module:
// uploadHelpers.ts
type PendingData = { retry_local_url?: string; url?: string; name?: string };
type DeferredUploadParams<TData extends PendingData> = {
editor: Editor;
blockId: string;
blockType: BlockType;
files: File[];
createPendingData: (file: File) => Promise<TData>;
uploadRemote: (file: File) => Promise<string | undefined>;
cleanupLocal: (retryLocalUrl?: string) => Promise<void>;
// Allows caller to build final block data shape
buildFinalData: (file: File, url: string, pending: TData) => TData;
};
export async function enqueueDeferredUpload<TData extends PendingData>({
editor,
blockId,
blockType,
files,
createPendingData,
uploadRemote,
cleanupLocal,
buildFinalData,
}: DeferredUploadParams<TData>): Promise<void> {
if (!files.length) return;
const [primaryData, ...otherDatas] = await Promise.all(
files.map((f) => createPendingData(f))
);
const [primaryFile, ...otherFiles] = files;
CustomEditor.setBlockData(editor, blockId, primaryData);
const pendingUploads: Promise<void>[] = [];
const uploadIntoBlock = async (targetBlockId: string, file: File, pending: TData) => {
const url = await uploadRemote(file);
if (!url) return;
await cleanupLocal(pending.retry_local_url);
let currentData: TData | undefined;
try {
const entry = findSlateEntryByBlockId(editor, targetBlockId);
currentData = entry ? ((entry[0] as { data?: TData }).data ?? undefined) : undefined;
} catch {
return;
}
if (!currentData) return;
if (currentData.url) return;
if ((currentData.retry_local_url ?? '') !== (pending.retry_local_url ?? '')) return;
CustomEditor.setBlockData(editor, targetBlockId, buildFinalData(file, url, pending));
};
pendingUploads.push(uploadIntoBlock(blockId, primaryFile, primaryData));
const reversedPairs = otherFiles.map((f, i) => [f, otherDatas[i]] as const).reverse();
for (const [f, d] of reversedPairs) {
const newId = CustomEditor.addBelowBlock(editor, blockId, blockType, d);
if (newId) {
pendingUploads.push(uploadIntoBlock(newId, f, d));
}
}
await Promise.all(pendingUploads);
}Then your PDF popover becomes simpler and focused on PDF‑specific data:
const createPendingFileData = useCallback(async (file: File): Promise<PDFBlockData> => {
const data: PDFBlockData = {
url: undefined,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
};
try {
const fileHandler = new FileHandler();
const res = await fileHandler.handleFileUpload(file);
URL.revokeObjectURL(res.url);
data.retry_local_url = res.id;
} catch {
data.retry_local_url = '';
}
return data;
}, []);
const cleanupLocalFile = useCallback(async (retryLocalUrl?: string) => {
if (!retryLocalUrl) return;
const fileHandler = new FileHandler();
await fileHandler.cleanup(retryLocalUrl).catch(() => undefined);
}, []);
const handleChangeUploadFiles = useCallback(
async (files: File[]) => {
if (!files.length) return;
setUploading(true);
try {
await enqueueDeferredUpload<PDFBlockData>({
editor,
blockId,
blockType: BlockType.PDFBlock,
files,
createPendingData: createPendingFileData,
uploadRemote: uploadFileRemote,
cleanupLocal: cleanupLocalFile,
buildFinalData: (file, url, pending) => ({
...pending,
url,
name: file.name,
uploaded_at: Date.now(),
url_type: FieldURLType.Upload,
retry_local_url: '',
}),
});
onClose();
} finally {
setUploading(false);
}
},
[blockId, editor, onClose, createPendingFileData, uploadFileRemote, cleanupLocalFile]
);This keeps all of your current behavior (parallel snapshotting, retry_local_url guards, cleanup, ordering) but:
- Concentrates the complex guard/cleanup/upload logic in one place.
- Makes the PDF popover (and the file/image popovers later) mostly about “how to build pending/final data”, reducing duplication and cognitive load.
| [cleanupLocalFile, editor, uploadFileRemote] | ||
| ); | ||
|
|
||
| const handleChangeUploadFiles = useCallback( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the upload orchestration and placeholder insertion logic from handleChangeUploadFiles into a reusable helper to keep this handler focused on layout and UX concerns.
You can reduce the cognitive load in handleChangeUploadFiles (and make this reusable with file/PDF popovers) by extracting the placeholder+upload orchestration into a small helper that encapsulates:
getDatafor each file- creating/updating image blocks in order
- wiring
pendingUploadsviauploadIntoImageBlock
That keeps handleChangeUploadFiles focused on where to insert blocks and scroll/focus behavior, not how uploads work.
Suggested refactor
Extract a helper (can live in a shared module with file/PDF versions) that returns the last block and pending uploads:
type InsertResult = {
lastBlockId?: string;
pendingUploads: Promise<void>[];
};
function insertImageBlocksWithUploads(
editor: Editor,
anchorBlockId: string,
files: File[],
getData: (file: File) => Promise<ImageBlockData>,
uploadIntoImageBlock: (targetBlockId: string, file: File, pendingData: ImageBlockData) => Promise<void>,
): Promise<InsertResult> {
return (async () => {
if (!files.length) return { lastBlockId: anchorBlockId, pendingUploads: [] };
const [primaryData, ...otherDatas] = await Promise.all(files.map(getData));
const [file, ...otherFiles] = files;
CustomEditor.setBlockData(editor, anchorBlockId, primaryData);
let belowBlockId: string | undefined = anchorBlockId;
const pendingUploads: Promise<void>[] = [
uploadIntoImageBlock(anchorBlockId, file, primaryData),
];
for (let i = 0; i < otherFiles.length; i++) {
const f = otherFiles[i];
const d = otherDatas[i];
const newId = belowBlockId
? CustomEditor.addBelowBlock(editor, belowBlockId, BlockType.ImageBlock, d)
: undefined;
if (newId) {
belowBlockId = newId;
pendingUploads.push(uploadIntoImageBlock(newId, f, d));
}
}
return { lastBlockId: belowBlockId, pendingUploads };
})();
}Then handleChangeUploadFiles becomes a lot flatter:
const handleChangeUploadFiles = useCallback(
async (files: File[]) => {
if (!files.length) return;
setUploading(true);
try {
const { lastBlockId, pendingUploads } = await insertImageBlocksWithUploads(
editor,
blockId,
files,
getData,
uploadIntoImageBlock,
);
if (!lastBlockId) return;
const paragraphId = CustomEditor.addBelowBlock(
editor,
lastBlockId,
BlockType.Paragraph,
{},
);
const entry = paragraphId ? findSlateEntryByBlockId(editor, paragraphId) : null;
if (!entry) return;
const [node, path] = entry;
onClose();
if (path) {
editor.select(editor.start(path));
}
setTimeout(() => {
if (!node) return;
const el = ReactEditor.toDOMNode(editor, node);
el?.scrollIntoView({ behavior: 'smooth', block: 'center' });
}, 250);
await Promise.all(pendingUploads);
} finally {
setUploading(false);
}
},
[blockId, editor, getData, onClose, uploadIntoImageBlock],
);Benefits:
handleChangeUploadFilesnow only handles:- early return
- calling a single helper
- paragraph insertion + selection + scrolling
- waiting for uploads
- Upload orchestration (placeholder creation, ordering,
pendingUploads) is isolated and can be reused for file/PDF popovers by adjusting block type and data type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Checks
Summary by Sourcery
Add resumable, deduplicated multipart upload support for editor-driven file uploads and wire editor file, image, PDF, and paste flows through this upload state, with local snapshot persistence and cleanup.
New Features:
Enhancements:
Tests: