refactor: multipart upload support with storageMode API#44
Open
NyllRE wants to merge 1 commit into
Open
Conversation
Reviewer's GuideRefactors the storage API to introduce a new multipart upload path (both client composable and server utilities), keeps the legacy DataURL/JSON flow behind deprecations, and updates playground/tests to exercise both modes and shared filesystem logic. Sequence diagram for multipart file upload flowsequenceDiagram
actor User
participant BrowserInput as Browser_file_input
participant Composable as useFileStorage_Multipart
participant NuxtClient as Nuxt_client_$fetch
participant ApiRoute as api_files_handler
participant Storage as storage_utils
participant FS as FileSystem
User->>BrowserInput: select_files
BrowserInput->>Composable: handleFileInput(event)
Composable->>Composable: clearFiles_if_deleteOldFiles
Composable->>Composable: append_files_to_FormData
Composable-->>NuxtClient: files_ref_FormData
User->>NuxtClient: click_submit
NuxtClient->>ApiRoute: POST_formdata(files_ref)
ApiRoute->>ApiRoute: readMultipartFormData(event)
ApiRoute->>Storage: storeFile(file,fileNameOrIdLength,filelocation)
Storage->>Storage: writeToFilesystem(filename,fallbackExt,buffer,fileNameOrIdLength,filelocation)
Storage->>FS: writeFile(targetPath,buffer)
FS-->>Storage: write_success
Storage-->>ApiRoute: stored_filename
ApiRoute-->>NuxtClient: fileNames_array
NuxtClient-->>User: show_success_message_and_links
Sequence diagram for legacy DataURL JSON upload flowsequenceDiagram
actor User
participant BrowserInput as Browser_file_input
participant Composable as useFileStorage_DataURL
participant NuxtClient as Nuxt_client_$fetch
participant ApiRoute as api_files_json_handler
participant Storage as storage_utils
participant FS as FileSystem
User->>BrowserInput: select_files
BrowserInput->>Composable: handleFileInput_or_handleJsonFileInput
Composable->>Composable: serializeFile_to_DataURL
Composable->>Composable: push_to_jsonFiles
Composable-->>NuxtClient: jsonFiles_ref
User->>NuxtClient: click_submit
NuxtClient->>ApiRoute: POST_json_body(jsonFiles)
ApiRoute->>ApiRoute: readBody_files_array
ApiRoute->>Storage: storeFileJson(file,fileNameOrIdLength,filelocation)
Storage->>Storage: parseDataUrl(file.content)
Storage->>Storage: writeToFilesystem(file.name,ext,binaryString,fileNameOrIdLength,filelocation)
Storage->>FS: writeFile(targetPath,binaryString)
FS-->>Storage: write_success
Storage-->>ApiRoute: stored_filename
ApiRoute-->>NuxtClient: fileNames_array
NuxtClient-->>User: show_success_message_and_links
Class diagram for updated file storage types and utilitiesclassDiagram
class File {
+string name
+number size
+string type
+number lastModified
+string content
}
class ServerFile {
+string name
+string content
+string type
+string size
+string lastModified
}
class MultipartFileEntry {
+string name
+string filename
+string type
+Uint8Array data
}
class ClientFile {
+string name
+number lastModified
+string_or_ArrayBuffer_or_null_or_undefined content
}
class StorageModule {
+storeFile(file,fileNameOrIdLength,filelocation) Promise~string~
+storeFileJson(file,fileNameOrIdLength,filelocation) Promise~string~
+storeFileLocally(file,fileNameOrIdLength,filelocation) Promise~string~
+getFileLocally(filename,filelocation) string
+getFilesLocally(filelocation) Promise~string[]~
+deleteFile(filename,filelocation) Promise~void~
+retrieveFileLocally(event,filename,filelocation) Promise~ReadableStream~
+parseDataUrl(file) binaryString_and_ext
+writeToFilesystem(rawName,fallbackExt,buffer,fileNameOrIdLength,filelocation) Promise~string~
+resolveFilename(sourceName,fileNameOrIdLength,fallbackExt) string
+generateRandomId(length) string
}
class UseFileStorageComposable {
+StorageMode storageMode
+boolean deleteOldFiles
+Ref_files files
+Ref_jsonFiles jsonFiles
+handleFileInput(event) Promise~void~
+handleJsonFileInput(event) Promise~void~
+clearFiles() void
+serializeFile(file) Promise~SerializedFile~
}
class SerializedFile {
+string name
+number size
+string type
+number lastModified
+string_or_ArrayBuffer_or_null content
}
File <|-- ServerFile
SerializedFile ..> File
UseFileStorageComposable o--> SerializedFile
UseFileStorageComposable o--> ClientFile
StorageModule ..> File
StorageModule ..> MultipartFileEntry
StorageModule ..> ServerFile
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 4 issues, and left some high level feedback:
- The new server-side
Fileinterface name can easily conflict with the standard DOMFiletype in TypeScript; consider renaming (e.g.JsonFile/StoredFile) or namespacing it to avoid confusion and accidental misuse. - In
useFileStorage,clearFilesforMultipartmode only callsFormData.delete('files'), which keeps the sameFormDatainstance and any other fields; if the intent is a full reset, reassigningfiles.valueto a freshFormDatawould be clearer and less error-prone for consumers who add extra form fields. - The helper is named
writeToFilesystemin code but referred to aswriteToFileSystem()in the PR description; aligning the naming (and deciding whether it should be part of the public API) would help keep the API surface and docs consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new server-side `File` interface name can easily conflict with the standard DOM `File` type in TypeScript; consider renaming (e.g. `JsonFile`/`StoredFile`) or namespacing it to avoid confusion and accidental misuse.
- In `useFileStorage`, `clearFiles` for `Multipart` mode only calls `FormData.delete('files')`, which keeps the same `FormData` instance and any other fields; if the intent is a full reset, reassigning `files.value` to a fresh `FormData` would be clearer and less error-prone for consumers who add extra form fields.
- The helper is named `writeToFilesystem` in code but referred to as `writeToFileSystem()` in the PR description; aligning the naming (and deciding whether it should be part of the public API) would help keep the API surface and docs consistent.
## Individual Comments
### Comment 1
<location path="src/runtime/server/utils/storage.ts" line_range="27-32" />
<code_context>
- const { binaryString, ext } = parseDataUrl(file.content)
- const location = getMount()
- if (!location) throw new Error('fileStorage.mount is not configured')
+export const parseDataUrl = (file: string): { binaryString: Buffer; ext: string } => {
+ const arr = file.split(',')
+ const mimeMatch = arr[0].match(/:(.*?);/)
+ if (!mimeMatch) throw new Error('Invalid data URL')
+ const mime = mimeMatch[1]
+ const base64String = arr[1]
+ const binaryString = Buffer.from(base64String, 'base64')
+ const ext = mime.split('/')[1]
</code_context>
<issue_to_address>
**issue (bug_risk):** Strengthen validation of the data URL structure before accessing segments
`parseDataUrl` currently assumes `file.split(',')[1]` always exists. If the string has no comma or an empty payload, `base64String` will be `undefined` and passed to `Buffer.from`. Please validate the split result (e.g., `arr.length === 2` and `arr[1]` is non-empty) and throw a clear error when the data segment is missing or malformed.
</issue_to_address>
### Comment 2
<location path="test/storage.test.ts" line_range="76-85" />
<code_context>
- // Attempt to delete outside mount
- await expect(deleteFile('../etc/passwd', '')).rejects.toThrow()
+describe('storeFile', () => {
+ it('stores a file from raw Uint8Array without base64 conversion', async () => {
+ const rawContent = new TextEncoder().encode('hello from multipart\n')
+ const entry = {
+ name: 'files',
+ filename: 'hello.txt',
+ type: 'text/plain',
+ data: rawContent,
+ }
+
+ const filename = await storeFile(entry, 'greeting', '/uploads')
+ expect(filename).toBe('greeting.txt')
+
+ const filePath = getFileLocally(filename, '/uploads')
+ const content = await readFile(filePath, 'utf-8')
+ expect(content).toBe('hello from multipart\n')
+ })
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for mismatched or missing extension handling in `storeFile` to cover `resolveFilename`’s warning/extension-correction logic.
The new logic warns and normalizes when the provided filename has no or an incorrect extension. Current tests cover ID generation and multipart writing but not this behavior. Please add a test where `file.type` (e.g. `image/png`) conflicts with `fileNameOrIdLength` (e.g. `photo.jpg`), and assert that the stored filename ends with `.png` and, ideally, that a warning is logged. This helps prevent regressions in extension normalization.
Suggested implementation:
```typescript
it('stores a file from raw Uint8Array without base64 conversion', async () => {
const rawContent = new TextEncoder().encode('hello from multipart\n')
const entry = {
name: 'files',
filename: 'hello.txt',
type: 'text/plain',
data: rawContent,
}
const filename = await storeFile(entry, 'greeting', '/uploads')
expect(filename).toBe('greeting.txt')
const filePath = getFileLocally(filename, '/uploads')
const content = await readFile(filePath, 'utf-8')
expect(content).toBe('hello from multipart\n')
})
it('normalizes mismatched filename extension based on content type', async () => {
// Simulate a multipart file where the original filename has a .jpg extension
// but the content-type is image/png. resolveFilename should normalize this.
const pngHeader = new Uint8Array([0x89, 0x50, 0x4e, 0x47]) // "\x89PNG"
const entry = {
name: 'files',
filename: 'photo.jpg',
type: 'image/png',
data: pngHeader,
}
// Spy on warnings so we can assert a normalization warning is emitted
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
const storedFilename = await storeFile(entry, 'photo', '/uploads')
// The filename should have a .png extension after normalization
expect(storedFilename.endsWith('.png')).toBe(true)
expect(storedFilename).not.toBe('photo.jpg')
// And a warning should be logged about extension mismatch/normalization
expect(warnSpy).toHaveBeenCalled()
warnSpy.mockRestore()
})
```
- If this test suite uses Jest instead of Vitest, replace `vi.spyOn` with `jest.spyOn`.
- If `resolveFilename` logs via a project-specific logger instead of `console.warn`, adjust the spy to target that logger (e.g. `logger.warn`) and import/mock it as needed.
</issue_to_address>
### Comment 3
<location path="test/storage.test.ts" line_range="52-61" />
<code_context>
+describe('storeFileJson', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `storeFileJson` tests to cover invalid data URLs and ensure `parseDataUrl` error paths are exercised.
`parseDataUrl` throws on malformed data URLs (e.g. missing MIME match or base64 segment), but current tests only cover valid inputs. Please add a test with an invalid `content` value (such as `'not-a-data-url'` or a data URL missing the MIME section) and assert that `storeFileJson` rejects, so the error path and failure behavior remain covered and stable.
Suggested implementation:
```typescript
describe('storeFileJson', () => {
it('stores a file from base64 data URL', async () => {
const file = { name: 'test.txt', content: dataUrl, size: 12, type: 'text/plain', lastModified: Date.now() }
const filename = await storeFileJson(file, 8, 'sub')
expect(filename).toMatch(/^[A-Za-z0-9]{8}\.txt$/)
const filePath = getFileLocally(filename, 'sub')
const content = await readFile(filePath, 'utf-8')
expect(content).toContain('hello world')
})
it('rejects when given an invalid data URL', async () => {
const invalidFile = {
name: 'invalid.txt',
content: 'not-a-data-url',
size: 12,
type: 'text/plain',
lastModified: Date.now(),
}
await expect(storeFileJson(invalidFile, 8, 'sub')).rejects.toThrow()
})
```
If `storeFileJson` wraps errors differently (e.g. it always throws a specific error type or message when `parseDataUrl` fails), you may want to tighten the expectation, for example:
- `rejects.toThrow(/invalid data url/i)` or
- `rejects.toThrow(MyCustomError)`
to better match the actual behavior of `parseDataUrl`/`storeFileJson` in your codebase.
</issue_to_address>
### Comment 4
<location path="test/storage.test.ts" line_range="151-60" />
<code_context>
+ })
+})
+
+// ─── storeFileLocally (legacy alias) ─────────────────────────────────────────
+
+describe('storeFileLocally (legacy alias)', () => {
+ it('dispatches to storeFile for multipart entries', async () => {
+ const entry = {
+ name: 'files',
+ filename: 'alias.txt',
+ type: 'text/plain',
+ data: new TextEncoder().encode('via alias\n'),
+ }
+
+ const filename = await storeFileLocally(entry, 'aliased', '')
+ expect(filename).toBe('aliased.txt')
+
+ const filePath = getFileLocally(filename, '')
+ const content = await readFile(filePath, 'utf-8')
+ expect(content).toBe('via alias\n')
+ })
+
+ it('dispatches to storeFileJson for base64 entries', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add dedicated tests for the new `useFileStorage` composable modes (`Multipart` vs `DataURL`, `deleteOldFiles`, and `handleJsonFileInput`).
These changes introduce new behavior (`storageMode`, `deleteOldFiles`, `jsonFiles`, `handleJsonFileInput`) without any direct tests on `useFileStorage`. That makes regressions in `files` ref shape, clearing logic, and JSON serialization easy to miss. Please add a focused test suite (e.g. `test/useFileStorage.test.ts`) that mounts the composable and verifies: (1) `Multipart` sets `files.value` to `FormData` and appends inputs, (2) `DataURL` populates `files`/`jsonFiles` correctly and respects `clearFiles`/`deleteOldFiles`, and (3) `handleJsonFileInput` preserves legacy behavior.
Suggested implementation:
```typescript
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { nextTick, ref } from 'vue'
import { useFileStorage } from '@/composables/useFileStorage'
/**
* NOTE:
* These tests assume the following `useFileStorage` API shape:
* const {
* files, // ref<FormData | File[] | null>
* jsonFiles, // ref<Record<string, unknown>[] | null>
* handleFileInput,
* handleJsonFileInput,
* clearFiles,
* } = useFileStorage(options)
*
* where `options` may include:
* - storageMode: 'Multipart' | 'DataURL'
* - deleteOldFiles?: boolean
*
* If your actual API differs, adjust the imports/options accordingly.
*/
describe('useFileStorage - Multipart mode', () => {
let composable: ReturnType<typeof useFileStorage>
beforeEach(() => {
composable = useFileStorage({ storageMode: 'Multipart' })
})
it('initializes files as FormData in Multipart mode', () => {
expect(composable.files.value).toBeInstanceOf(FormData)
})
it('appends selected files to FormData', async () => {
const file = new File(['hello'], 'test.txt', { type: 'text/plain' })
// Simulate a native file input change event
const input = document.createElement('input')
input.type = 'file'
Object.defineProperty(input, 'files', {
value: [file],
})
const event = { target: input } as unknown as Event
composable.handleFileInput(event)
await nextTick()
const formData = composable.files.value as FormData
const entries: [string, FormDataEntryValue][] = []
for (const entry of (formData as any).entries()) {
entries.push(entry)
}
expect(entries.length).toBe(1)
expect(entries[0][0]).toBe('files')
expect(entries[0][1]).toBeInstanceOf(File)
expect((entries[0][1] as File).name).toBe('test.txt')
})
})
describe('useFileStorage - DataURL mode', () => {
const base64Content = btoa('{"foo":"bar"}')
const dataUrl = `data:application/json;base64,${base64Content}`
let composable: ReturnType<typeof useFileStorage>
beforeEach(() => {
composable = useFileStorage({
storageMode: 'DataURL',
deleteOldFiles: false,
})
})
it('populates files and jsonFiles for DataURL inputs', async () => {
const file = {
name: 'data.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
// emulate what the caller would pass into the composable
composable.handleFileInput(file as any)
await nextTick()
// files should reflect the logical representation (e.g. array or map)
expect(composable.files.value).toBeTruthy()
// jsonFiles should contain parsed JSON representation
expect(composable.jsonFiles.value).toBeTruthy()
expect(Array.isArray(composable.jsonFiles.value)).toBe(true)
expect(composable.jsonFiles.value![0]).toMatchObject({ foo: 'bar' })
})
it('clearFiles clears files and jsonFiles', async () => {
const file = {
name: 'data.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
composable.handleFileInput(file as any)
await nextTick()
expect(composable.files.value).toBeTruthy()
expect(composable.jsonFiles.value).toBeTruthy()
composable.clearFiles()
await nextTick()
expect(composable.files.value).toBeNull()
expect(composable.jsonFiles.value).toBeNull()
})
it('deleteOldFiles=true replaces existing files on new input', async () => {
// re-create composable with deleteOldFiles enabled
composable = useFileStorage({
storageMode: 'DataURL',
deleteOldFiles: true,
})
const first = {
name: 'first.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
const second = {
name: 'second.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
composable.handleFileInput(first as any)
await nextTick()
const firstSnapshot = composable.files.value
composable.handleFileInput(second as any)
await nextTick()
// With deleteOldFiles=true we expect the previous files ref to be replaced, not appended
expect(composable.files.value).not.toBe(firstSnapshot)
})
})
describe('useFileStorage - handleJsonFileInput (legacy behavior)', () => {
const base64Content = btoa('{"legacy":true}')
const dataUrl = `data:application/json;base64,${base64Content}`
it('handleJsonFileInput preserves legacy JSON parsing behavior', async () => {
const composable = useFileStorage({
storageMode: 'DataURL',
})
const fileEventPayload = {
name: 'legacy.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
// legacy path
composable.handleJsonFileInput(fileEventPayload as any)
await nextTick()
expect(composable.jsonFiles.value).toBeTruthy()
expect(composable.jsonFiles.value!.length).toBe(1)
expect(composable.jsonFiles.value![0]).toMatchObject({ legacy: true })
// ensure `files` is also updated consistently with previous behavior
expect(composable.files.value).toBeTruthy()
})
})
```
1. Adjust the `useFileStorage` import path (`@/composables/useFileStorage`) to match your project’s aliasing (e.g. `~/composables/useFileStorage` or a relative path).
2. If `storageMode` is an enum instead of a string literal, replace `'Multipart'` and `'DataURL'` with the appropriate enum values (e.g. `FileStorageMode.Multipart` / `FileStorageMode.DataURL`).
3. If the composable’s API differs (e.g. different option names, different `files`/`jsonFiles` shapes, or `handleFileInput`/`handleJsonFileInput` signatures), align the tests to the actual API while preserving the intent:
- Multipart mode: `files.value` is a `FormData` and each call to `handleFileInput` appends new entries.
- DataURL mode: `files` and `jsonFiles` are populated with the expected structures, `clearFiles` resets both, and `deleteOldFiles: true` replaces existing entries instead of appending.
- `handleJsonFileInput` continues to parse JSON and update `jsonFiles`/`files` as it did prior to the new `storageMode`/`deleteOldFiles` additions.
</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
+76
to
+85
| it('stores a file from raw Uint8Array without base64 conversion', async () => { | ||
| const rawContent = new TextEncoder().encode('hello from multipart\n') | ||
| const entry = { | ||
| name: 'files', | ||
| filename: 'hello.txt', | ||
| type: 'text/plain', | ||
| data: rawContent, | ||
| } | ||
|
|
||
| const filename = await storeFile(entry, 'greeting', '/uploads') |
There was a problem hiding this comment.
suggestion (testing): Add a test case for mismatched or missing extension handling in storeFile to cover resolveFilename’s warning/extension-correction logic.
The new logic warns and normalizes when the provided filename has no or an incorrect extension. Current tests cover ID generation and multipart writing but not this behavior. Please add a test where file.type (e.g. image/png) conflicts with fileNameOrIdLength (e.g. photo.jpg), and assert that the stored filename ends with .png and, ideally, that a warning is logged. This helps prevent regressions in extension normalization.
Suggested implementation:
it('stores a file from raw Uint8Array without base64 conversion', async () => {
const rawContent = new TextEncoder().encode('hello from multipart\n')
const entry = {
name: 'files',
filename: 'hello.txt',
type: 'text/plain',
data: rawContent,
}
const filename = await storeFile(entry, 'greeting', '/uploads')
expect(filename).toBe('greeting.txt')
const filePath = getFileLocally(filename, '/uploads')
const content = await readFile(filePath, 'utf-8')
expect(content).toBe('hello from multipart\n')
})
it('normalizes mismatched filename extension based on content type', async () => {
// Simulate a multipart file where the original filename has a .jpg extension
// but the content-type is image/png. resolveFilename should normalize this.
const pngHeader = new Uint8Array([0x89, 0x50, 0x4e, 0x47]) // "\x89PNG"
const entry = {
name: 'files',
filename: 'photo.jpg',
type: 'image/png',
data: pngHeader,
}
// Spy on warnings so we can assert a normalization warning is emitted
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
const storedFilename = await storeFile(entry, 'photo', '/uploads')
// The filename should have a .png extension after normalization
expect(storedFilename.endsWith('.png')).toBe(true)
expect(storedFilename).not.toBe('photo.jpg')
// And a warning should be logged about extension mismatch/normalization
expect(warnSpy).toHaveBeenCalled()
warnSpy.mockRestore()
})- If this test suite uses Jest instead of Vitest, replace
vi.spyOnwithjest.spyOn. - If
resolveFilenamelogs via a project-specific logger instead ofconsole.warn, adjust the spy to target that logger (e.g.logger.warn) and import/mock it as needed.
Comment on lines
+52
to
61
| describe('storeFileJson', () => { | ||
| it('stores a file from base64 data URL', async () => { | ||
| const file = { name: 'test.txt', content: dataUrl, size: 12, type: 'text/plain', lastModified: Date.now() } | ||
| const filename = await storeFileJson(file, 8, 'sub') | ||
| expect(filename).toMatch(/^[A-Za-z0-9]{8}\.txt$/) | ||
| const filePath = getFileLocally(filename, 'sub') | ||
| const content = await readFile(filePath, 'utf-8') | ||
| expect(content).toContain('hello world') | ||
| }) | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Extend storeFileJson tests to cover invalid data URLs and ensure parseDataUrl error paths are exercised.
parseDataUrl throws on malformed data URLs (e.g. missing MIME match or base64 segment), but current tests only cover valid inputs. Please add a test with an invalid content value (such as 'not-a-data-url' or a data URL missing the MIME section) and assert that storeFileJson rejects, so the error path and failure behavior remain covered and stable.
Suggested implementation:
describe('storeFileJson', () => {
it('stores a file from base64 data URL', async () => {
const file = { name: 'test.txt', content: dataUrl, size: 12, type: 'text/plain', lastModified: Date.now() }
const filename = await storeFileJson(file, 8, 'sub')
expect(filename).toMatch(/^[A-Za-z0-9]{8}\.txt$/)
const filePath = getFileLocally(filename, 'sub')
const content = await readFile(filePath, 'utf-8')
expect(content).toContain('hello world')
})
it('rejects when given an invalid data URL', async () => {
const invalidFile = {
name: 'invalid.txt',
content: 'not-a-data-url',
size: 12,
type: 'text/plain',
lastModified: Date.now(),
}
await expect(storeFileJson(invalidFile, 8, 'sub')).rejects.toThrow()
})If storeFileJson wraps errors differently (e.g. it always throws a specific error type or message when parseDataUrl fails), you may want to tighten the expectation, for example:
rejects.toThrow(/invalid data url/i)orrejects.toThrow(MyCustomError)
to better match the actual behavior ofparseDataUrl/storeFileJsonin your codebase.
| const filePath = getFileLocally(filename, 'sub') | ||
| const content = await readFile(filePath, 'utf-8') | ||
| expect(content).toContain('hello world') | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Add dedicated tests for the new useFileStorage composable modes (Multipart vs DataURL, deleteOldFiles, and handleJsonFileInput).
These changes introduce new behavior (storageMode, deleteOldFiles, jsonFiles, handleJsonFileInput) without any direct tests on useFileStorage. That makes regressions in files ref shape, clearing logic, and JSON serialization easy to miss. Please add a focused test suite (e.g. test/useFileStorage.test.ts) that mounts the composable and verifies: (1) Multipart sets files.value to FormData and appends inputs, (2) DataURL populates files/jsonFiles correctly and respects clearFiles/deleteOldFiles, and (3) handleJsonFileInput preserves legacy behavior.
Suggested implementation:
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { nextTick, ref } from 'vue'
import { useFileStorage } from '@/composables/useFileStorage'
/**
* NOTE:
* These tests assume the following `useFileStorage` API shape:
* const {
* files, // ref<FormData | File[] | null>
* jsonFiles, // ref<Record<string, unknown>[] | null>
* handleFileInput,
* handleJsonFileInput,
* clearFiles,
* } = useFileStorage(options)
*
* where `options` may include:
* - storageMode: 'Multipart' | 'DataURL'
* - deleteOldFiles?: boolean
*
* If your actual API differs, adjust the imports/options accordingly.
*/
describe('useFileStorage - Multipart mode', () => {
let composable: ReturnType<typeof useFileStorage>
beforeEach(() => {
composable = useFileStorage({ storageMode: 'Multipart' })
})
it('initializes files as FormData in Multipart mode', () => {
expect(composable.files.value).toBeInstanceOf(FormData)
})
it('appends selected files to FormData', async () => {
const file = new File(['hello'], 'test.txt', { type: 'text/plain' })
// Simulate a native file input change event
const input = document.createElement('input')
input.type = 'file'
Object.defineProperty(input, 'files', {
value: [file],
})
const event = { target: input } as unknown as Event
composable.handleFileInput(event)
await nextTick()
const formData = composable.files.value as FormData
const entries: [string, FormDataEntryValue][] = []
for (const entry of (formData as any).entries()) {
entries.push(entry)
}
expect(entries.length).toBe(1)
expect(entries[0][0]).toBe('files')
expect(entries[0][1]).toBeInstanceOf(File)
expect((entries[0][1] as File).name).toBe('test.txt')
})
})
describe('useFileStorage - DataURL mode', () => {
const base64Content = btoa('{"foo":"bar"}')
const dataUrl = `data:application/json;base64,${base64Content}`
let composable: ReturnType<typeof useFileStorage>
beforeEach(() => {
composable = useFileStorage({
storageMode: 'DataURL',
deleteOldFiles: false,
})
})
it('populates files and jsonFiles for DataURL inputs', async () => {
const file = {
name: 'data.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
// emulate what the caller would pass into the composable
composable.handleFileInput(file as any)
await nextTick()
// files should reflect the logical representation (e.g. array or map)
expect(composable.files.value).toBeTruthy()
// jsonFiles should contain parsed JSON representation
expect(composable.jsonFiles.value).toBeTruthy()
expect(Array.isArray(composable.jsonFiles.value)).toBe(true)
expect(composable.jsonFiles.value![0]).toMatchObject({ foo: 'bar' })
})
it('clearFiles clears files and jsonFiles', async () => {
const file = {
name: 'data.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
composable.handleFileInput(file as any)
await nextTick()
expect(composable.files.value).toBeTruthy()
expect(composable.jsonFiles.value).toBeTruthy()
composable.clearFiles()
await nextTick()
expect(composable.files.value).toBeNull()
expect(composable.jsonFiles.value).toBeNull()
})
it('deleteOldFiles=true replaces existing files on new input', async () => {
// re-create composable with deleteOldFiles enabled
composable = useFileStorage({
storageMode: 'DataURL',
deleteOldFiles: true,
})
const first = {
name: 'first.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
const second = {
name: 'second.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
composable.handleFileInput(first as any)
await nextTick()
const firstSnapshot = composable.files.value
composable.handleFileInput(second as any)
await nextTick()
// With deleteOldFiles=true we expect the previous files ref to be replaced, not appended
expect(composable.files.value).not.toBe(firstSnapshot)
})
})
describe('useFileStorage - handleJsonFileInput (legacy behavior)', () => {
const base64Content = btoa('{"legacy":true}')
const dataUrl = `data:application/json;base64,${base64Content}`
it('handleJsonFileInput preserves legacy JSON parsing behavior', async () => {
const composable = useFileStorage({
storageMode: 'DataURL',
})
const fileEventPayload = {
name: 'legacy.json',
content: dataUrl,
size: base64Content.length,
type: 'application/json',
lastModified: Date.now(),
}
// legacy path
composable.handleJsonFileInput(fileEventPayload as any)
await nextTick()
expect(composable.jsonFiles.value).toBeTruthy()
expect(composable.jsonFiles.value!.length).toBe(1)
expect(composable.jsonFiles.value![0]).toMatchObject({ legacy: true })
// ensure `files` is also updated consistently with previous behavior
expect(composable.files.value).toBeTruthy()
})
})- Adjust the
useFileStorageimport path (@/composables/useFileStorage) to match your project’s aliasing (e.g.~/composables/useFileStorageor a relative path). - If
storageModeis an enum instead of a string literal, replace'Multipart'and'DataURL'with the appropriate enum values (e.g.FileStorageMode.Multipart/FileStorageMode.DataURL). - If the composable’s API differs (e.g. different option names, different
files/jsonFilesshapes, orhandleFileInput/handleJsonFileInputsignatures), align the tests to the actual API while preserving the intent:- Multipart mode:
files.valueis aFormDataand each call tohandleFileInputappends new entries. - DataURL mode:
filesandjsonFilesare populated with the expected structures,clearFilesresets both, anddeleteOldFiles: truereplaces existing entries instead of appending. handleJsonFileInputcontinues to parse JSON and updatejsonFiles/filesas it did prior to the newstorageMode/deleteOldFilesadditions.
- Multipart mode:
There was a problem hiding this comment.
Pull request overview
This PR refactors the file-storage module to support multipart/form-data uploads end-to-end (client composable → server storage utils → playground), while retaining a deprecated JSON/base64 (“DataURL”) path for legacy users.
Changes:
- Add multipart upload support via new
storeFile()(multipart) andstoreFileJson()(JSON/base64), withstoreFileLocally()kept as a legacy dispatcher. - Extend runtime types with
MultipartFileEntry(multipart) and a new JSON/base64Fileinterface. - Update playground and tests to exercise multipart uploads and legacy behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
test/storage.test.ts |
Expands/organizes storage tests to cover multipart + JSON/base64 + legacy alias behavior. |
src/runtime/types.ts |
Introduces MultipartFileEntry and a new JSON/base64 File type; retains ServerFile. |
src/runtime/server/utils/storage.ts |
Refactors storage into shared writeToFilesystem() and adds storeFile()/storeFileJson() APIs. |
src/runtime/composables/useFileStorage.ts |
Adds storageMode and switches client handling between FormData (multipart) and JSON/base64. |
playground/server/api/files.ts |
Updates playground API to use readMultipartFormData() and storeFile(). |
playground/app/app.vue |
Updates demo UI to use multipart mode and generate client-side previews. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+50
to
+54
| // DataURL mode — serialise to base64 for JSON upload | ||
| const nativeFiles = files.value as File[] | ||
| for (const file of Array.from(target.files)) { | ||
| nativeFiles.push(file) | ||
| const serialized = await serializeFile(file) |
Comment on lines
5
to
+9
| type Options = { | ||
| clearOldFiles: boolean | ||
| /** If true, selecting new files clears the previous list. Default: true */ | ||
| deleteOldFiles?: boolean | ||
| /** Upload mode: 'DataURL' (base64 JSON, deprecated) or 'Multipart' (FormData, recommended). Default: 'DataURL' */ | ||
| storageMode?: StorageMode |
Comment on lines
+44
to
+48
| if (storageMode === 'Multipart') { | ||
| const fd = files.value as FormData | ||
| for (const file of Array.from(target.files)) { | ||
| fd.append('files', file) | ||
| } |
Comment on lines
+183
to
+187
| export async function storeFileLocally( | ||
| file: MultipartFileEntry, | ||
| fileNameOrIdLength: string | number, | ||
| filelocation?: string, | ||
| ): Promise<string> |
| @@ -1,5 +1,5 @@ | |||
| import { writeFile, rm, mkdir, readdir } from 'fs/promises' | |||
| import type { ServerFile } from '../../types' | |||
| import type { File, ServerFile, MultipartFileEntry } from '../../types' | |||
Comment on lines
+9
to
+13
| export interface File { | ||
| name: string | ||
| size: number | ||
| type: string | ||
| lastModified: number |
Comment on lines
+67
to
+71
| watch(files, (newFiles) => { | ||
| // FormData entries — iterate to build previews | ||
| if (newFiles instanceof FormData) { | ||
| previews.value = [] | ||
| for (const [_, value] of newFiles.entries()) { |
Comment on lines
+67
to
+76
| watch(files, (newFiles) => { | ||
| // FormData entries — iterate to build previews | ||
| if (newFiles instanceof FormData) { | ||
| previews.value = [] | ||
| for (const [_, value] of newFiles.entries()) { | ||
| if (value instanceof File) { | ||
| previews.value.push(URL.createObjectURL(value)) | ||
| } | ||
| } | ||
| } |
| fileNames.push( | ||
| await storeFile( | ||
| file, // the file object | ||
| 'fileName', // you can add a name for the file or length of Unique ID that will be automatically generated! |
Align implementation with documented API design at file-storage.nuxt.space: Composable (useFileStorage): - Add storageMode option: 'DataURL' (default, deprecated) | 'Multipart' (recommended) - Rename clearOldFiles to deleteOldFiles - Multipart mode: files ref is FormData, ready to send directly - DataURL mode: jsonFiles ref for base64 JSON body (legacy) - Add handleJsonFileInput for backwards compat Server storage: - Add storeFile() for MultipartFileEntry (multipart/form-data) - Add storeFileJson() for File with base64 content (deprecated) - storeFileLocally() kept as auto-dispatching alias for backwards compat - Extract shared write logic into writeToFilesystem() helper Types: - Add MultipartFileEntry interface - Add new File interface for JSON method (matches docs spec) - ServerFile kept for legacy compat Playground: - Updated to use Multipart mode with FormData direct upload - Server API uses readMultipartFormData + storeFile() Tests: - 34 tests passing: 12 filename + 16 storage + 6 path-safety
NyllRE
force-pushed
the
refactor/multipart-storage-mode
branch
from
703baef to
3cf2a93
Compare
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
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
Align implementation with documented API design at file-storage.nuxt.space.
Composable (
useFileStorage)storageModeoption:Multipart(recommended) orDataURL(default, deprecated)clearOldFiles→deleteOldFilesfilesref is aFormData, ready to send directly via$fetchjsonFilesref for base64 JSON body (legacy)handleJsonFileInputfor backwards compatibilityServer storage
storeFile()— forMultipartFileEntryfromreadMultipartFormData()storeFileJson()— forFilewith base64content(deprecated)storeFileLocally()— kept as auto-dispatching alias for backwards compatwriteToFileSystem()helperTypes
MultipartFileEntryinterface for multipart uploadsFileinterface matching the docs spec for JSON methodServerFileretained for legacy compatPlayground
readMultipartFormData+storeFile()Tests
Migration example:
Summary by Sourcery
Add multipart/form-data upload support across client composable, server utils, and playground while keeping the legacy JSON/base64 path for backwards compatibility.
New Features:
Enhancements:
Tests: