Conversation
There was a problem hiding this comment.
Pull request overview
Implements instance modpack export (Modrinth + MultiMC) end-to-end, addressing #1082 by wiring a new export modal to new Tauri backend commands and ZIP creation utilities.
Changes:
- Add frontend export modal UI, translations, and service methods for exporting and listing exportable files.
- Add backend modpack export pipeline: file listing, manifest generation, remote-file detection, and async ZIP writing.
- Register new Tauri commands and add required Rust dependencies (
walkdir,async_zip).
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/instance.ts | Adds exportModpack / listModpackFiles service calls. |
| src/models/instance/misc.ts | Adds ModpackFileList type for exportable file list payload. |
| src/locales/zh-Hans.json | Adds Export Modpack modal strings + General.optional. |
| src/locales/en.json | Adds Export Modpack modal strings + General.optional. |
| src/layouts/instance-details-layout.tsx | Wires “Export Modpack” menu action to open the new shared modal. |
| src/components/special/shared-modals-provider.tsx | Registers the new export-modpack shared modal. |
| src/components/modals/export-modpack-modal.tsx | New modal UI for selecting export format/options and included files. |
| src-tauri/src/utils/fs.rs | Adds async ZIP writer helper for modpack export packaging. |
| src-tauri/src/resource/helpers/modrinth/mod.rs | Switches local file reads to async tokio::fs::read. |
| src-tauri/src/resource/helpers/curseforge/mod.rs | Switches local file reads to async tokio::fs::read. |
| src-tauri/src/lib.rs | Registers list_modpack_files / export_modpack commands. |
| src-tauri/src/instance/models/misc.rs | Adds backend ModpackFileList struct. |
| src-tauri/src/instance/helpers/modpack/modrinth.rs | Updates Modrinth manifest structs/defaults for export support. |
| src-tauri/src/instance/helpers/modpack/mod.rs | Exposes new export helper module. |
| src-tauri/src/instance/helpers/modpack/export.rs | New core backend export logic (listing, manifest generation, remote file collection). |
| src-tauri/src/instance/commands.rs | Adds new Tauri commands for listing/exporting modpacks. |
| src-tauri/Cargo.toml | Adds dependencies required for export implementation. |
| src-tauri/Cargo.lock | Locks new dependency graph for export implementation. |
| for rel in files { | ||
| let full = base_path.join(&rel); | ||
| if tokio::fs::try_exists(&full).await.unwrap_or(false) { | ||
| selected_files.push((rel, full)); | ||
| } |
There was a problem hiding this comment.
files comes from the frontend and is joined directly onto base_path. If a caller passes an absolute path or a path containing .., base_path.join(rel) can escape the instance directory and end up zipping arbitrary files from the filesystem. Validate that each rel is a safe relative path (no root/drive prefix, no .. components), and ensure the resolved path stays within base_path (e.g., via canonicalize + starts_with) before adding it to selected_files and before using it as a ZIP entry name.
There was a problem hiding this comment.
selected_files 是前端从 file list 中选出的，前端应当保证正确
| let mut tasks = Vec::new(); | ||
| let semaphore = Arc::new(Semaphore::new( | ||
| std::thread::available_parallelism().unwrap().into(), | ||
| )); | ||
|
|
||
| for (rel, full) in selected_files { | ||
| let rel = rel.clone(); | ||
| let full = full.clone(); | ||
| let app = app.clone(); | ||
| let permit = semaphore | ||
| .clone() | ||
| .acquire_owned() | ||
| .await | ||
| .map_err(|_| InstanceError::SemaphoreAcquireFailed)?; | ||
|
|
||
| let task = tokio::spawn(async move { | ||
| let result = if is_remote_candidate(&rel) && !no_create_remote_files { | ||
| build_modrinth_remote_file(&app, &rel, &full, skip_curseforge) | ||
| .await | ||
| .ok() | ||
| .flatten() | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| drop(permit); | ||
| (rel, full, result) | ||
| }); | ||
|
|
||
| tasks.push(task); | ||
| } |
There was a problem hiding this comment.
collect_modrinth_files spawns one Tokio task per selected file and stores every JoinHandle in tasks. With large instances this can create thousands of tasks and increase memory/CPU overhead even though concurrency is gated by a semaphore. Consider using a bounded pattern (e.g., futures::stream::iter(...).buffer_unordered(limit) or tokio::task::JoinSet with a fixed number of in-flight tasks) to avoid unbounded task creation.
| inputProps={{ fontSize: "xs-sm" }} | ||
| formErrMsgProps={{ fontSize: "xs-sm" }} | ||
| checkError={checkVersionError} | ||
| localeKey="ExportModpackModal.errorMessage" |
There was a problem hiding this comment.
Editable looks up validation messages via t(${localeKey}.error-${code}), but the new ExportModpackModal.errorMessage entries in the locale JSON are keyed as "1"/"2"/"3" (not "error-1", etc.), so the inline validation message won’t render. Also, modpackVersion reuses the same localeKey/error codes as the name field, which would display the wrong message once the keys are fixed—consider a separate errorMessageVersion (or distinct codes/messages).
| localeKey="ExportModpackModal.errorMessage" | |
| localeKey="ExportModpackModal.errorMessageVersion" |
| const parsedMinMemory = minMemoryInput.trim() | ||
| ? Number.parseInt(minMemoryInput.trim(), 10) | ||
| : undefined; | ||
|
|
||
| const options: ExportModpackOptions = { | ||
| format: exportFormat, | ||
| name: modpackName, | ||
| version: modpackVersion, | ||
| author: author || undefined, | ||
| description: description || undefined, | ||
| packWithLauncher: packWithLauncher || undefined, | ||
| minMemory: Number.isNaN(parsedMinMemory || NaN) | ||
| ? undefined | ||
| : parsedMinMemory, | ||
| noCreateRemoteFiles: noCreateRemoteFiles || undefined, | ||
| skipCurseForgeRemoteFiles: skipCurseForgeRemoteFiles || undefined, | ||
| }; |
There was a problem hiding this comment.
minMemory is deserialized as Option<u32> on the Rust side; if the user enters a negative value (e.g. -1), parseInt will produce a number and the frontend will send it, causing the command payload to fail deserialization. Add validation to only allow non-negative integers (and ideally a reasonable range) before setting minMemory in options (or change the backend type and validate there).
| "1": "Name cannot be empty", | ||
| "2": "Name contains invalid characters", | ||
| "3": "Name is too long (max 255 characters)" |
There was a problem hiding this comment.
Editable expects validation keys in the form error-<code> (e.g. error-1) under the provided localeKey, but this section is keyed as "1"/"2"/"3". As a result, the inline validation message for the export modal won’t resolve. Rename these keys to error-1, error-2, error-3 (and consider adding separate messages for version validation if needed).
| "1": "Name cannot be empty", | |
| "2": "Name contains invalid characters", | |
| "3": "Name is too long (max 255 characters)" | |
| "error-1": "Name cannot be empty", | |
| "error-2": "Name contains invalid characters", | |
| "error-3": "Name is too long (max 255 characters)" |
| if let Ok(res) = task.await { | ||
| match res { | ||
| (_, _, Some(modrinth_file)) => { | ||
| modrinth_files.push(modrinth_file); | ||
| } | ||
| (rel, full, None) => { | ||
| override_files.push((rel.to_string(), full)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Join errors from tokio::spawn are currently ignored (if let Ok(res) = task.await). If any task panics/cancels, the corresponding file will be silently dropped from both modrinth_files and override_files, producing an incomplete export without an error. Propagate the JoinError (or convert it into an SJMCLResult error) so export fails loudly.
| if let Ok(res) = task.await { | |
| match res { | |
| (_, _, Some(modrinth_file)) => { | |
| modrinth_files.push(modrinth_file); | |
| } | |
| (rel, full, None) => { | |
| override_files.push((rel.to_string(), full)); | |
| } | |
| } | |
| let res = task | |
| .await | |
| .map_err(|_| InstanceError::SemaphoreAcquireFailed)?; | |
| match res { | |
| (_, _, Some(modrinth_file)) => { | |
| modrinth_files.push(modrinth_file); | |
| } | |
| (rel, full, None) => { | |
| override_files.push((rel.to_string(), full)); | |
| } |
There was a problem hiding this comment.
应该修改为：task 失败则进入 override_files
| const response = await InstanceService.exportModpack( | ||
| instanceId, | ||
| savePath, | ||
| options, | ||
| selectedFileList | ||
| ); |
There was a problem hiding this comment.
The modal currently allows exporting with selectedFileList.length === 0. In that case the backend returns a generic ModpackManifestParseError, but the UI already has a localized error string (ExportModpackModal.error.noCategorySelected). Add a frontend guard before invoking exportModpack to show a clear message when no files are selected.
| const FileTreeItem: React.FC<{ node: FileTreeNode; depth?: number }> = ({ | ||
| node, | ||
| depth = 0, | ||
| }) => { | ||
| const expanded = expandedPaths.has(node.path); | ||
| const leafPaths = useMemo(() => collectLeafPaths(node), [node]); | ||
| const selectedCount = useMemo( |
There was a problem hiding this comment.
FileTreeItem is declared inside ExportModpackModal, which recreates the component type on every parent render. That forces React to treat all tree nodes as new components (remounting) and can noticeably hurt performance for large file lists. Move FileTreeItem to module scope (and optionally wrap with React.memo) and pass the needed state/handlers as props.
| "1": "名称不能为空", | ||
| "2": "名称包含无效字符", | ||
| "3": "名称过长（最多 255 个字符）" |
There was a problem hiding this comment.
Editable expects validation keys in the form error-<code> (e.g. error-1) under the provided localeKey, but this section is keyed as "1"/"2"/"3". As a result, the inline validation message for the export modal won’t resolve. Rename these keys to error-1, error-2, error-3 (and consider adding separate messages for version validation if needed).
| "1": "名称不能为空", | |
| "2": "名称包含无效字符", | |
| "3": "名称过长（最多 255 个字符）" | |
| "error-1": "名称不能为空", | |
| "error-2": "名称包含无效字符", | |
| "error-3": "名称过长（最多 255 个字符）" |
| let semaphore = Arc::new(Semaphore::new( | ||
| std::thread::available_parallelism().unwrap().into(), | ||
| )); |
There was a problem hiding this comment.
available_parallelism().unwrap() can panic (e.g. if the OS can’t report CPU count). Since this runs in a Tauri command, a panic would crash the backend. Prefer a safe fallback like available_parallelism().map(|n| n.get()).unwrap_or(4) when sizing the semaphore.
There was a problem hiding this comment.
同上面的并行问题，目前与其他地方保持一致
UNIkeEN
requested review from
SundayChen,
hans362,
pangbo13 and
xunying123
and removed request for
Reqwey
1 participant
Checklist
This PR is a ..
Related Issues
#1082
Description
支持的整合包类型：
目前性能似乎有些问题。
Additional Context