Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end “Import from Fantome” support to the Workshop feature (frontend UI + React Query hooks + new Tauri commands + Rust-side ZIP/WAD extraction + progress events + error code plumbing).
Changes:
- Introduces Workshop UI flows for selecting a
.fantome/.zip, peeking metadata, and importing into a new project viaImportFantomeDialog. - Adds frontend API types + React Query mutations (
peek_fantome,import_from_fantome) and a progress listener hook. - Implements Rust-side Fantome peek/import commands, progress event emission, and a new
FANTOME/Fantomeerror code.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/errors.ts | Adds FANTOME error code to align with backend. |
| src/routes/workshop/index.tsx | Wires new Fantome import flow into Workshop page and toolbar. |
| src/modules/workshop/components/index.ts | Re-exports ImportFantomeDialog. |
| src/modules/workshop/components/WorkshopToolbar.tsx | Replaces single import button with dropdown (Fantome vs Modpkg). |
| src/modules/workshop/components/ImportFantomeDialog.tsx | New dialog for reviewing Fantome metadata and naming imported project. |
| src/modules/workshop/api/usePeekFantome.ts | New mutation hook for peeking Fantome metadata. |
| src/modules/workshop/api/useImportFromFantome.ts | New mutation hook for importing a Fantome archive as a project. |
| src/modules/workshop/api/useFantomeImportProgress.ts | New hook to listen for Fantome import progress events. |
| src/modules/workshop/api/index.ts | Exposes new Fantome hooks. |
| src/lib/tauri.ts | Adds new IPC types and api.peekFantome / api.importFromFantome. |
| src-tauri/src/workshop/projects.rs | Implements Fantome peek/import logic and ZIP extraction helpers. |
| src-tauri/src/workshop/mod.rs | Adds shared structs for Fantome peek/import args/progress. |
| src-tauri/src/main.rs | Registers new Tauri commands. |
| src-tauri/src/error.rs | Adds Fantome error variants and response mapping. |
| src-tauri/src/commands/workshop.rs | Adds peek_fantome and import_from_fantome Tauri commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/routes/workshop/index.tsx
Outdated
| if (searchQuery) return <NoSearchResultsState />; | ||
| return ( | ||
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportModpkg} /> | ||
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportFantome} /> |
There was a problem hiding this comment.
NoProjectsState’s Import button is now wired to handleImportFantome, but the empty-state copy says “import an existing mod package”, and there’s no way to import a .modpkg when the project list is empty. Consider updating the empty state UI to offer the same Modpkg/Fantome choice (or keep .modpkg import here) and adjust the copy accordingly.
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportFantome} /> | |
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportModpkg} /> |
src/routes/workshop/index.tsx
Outdated
| onImportFantome={handleImportFantome} | ||
| onNewProject={() => setNewProjectOpen(true)} | ||
| isImporting={importFromModpkg.isPending} | ||
| isImporting={importFromModpkg.isPending || peekFantome.isPending} |
There was a problem hiding this comment.
The toolbar loading state only accounts for .modpkg import and the Fantome peek step. During the actual Fantome import (after submitting the dialog), isImporting will be false even though an import is running, which can allow triggering additional imports and gives misleading UI. Consider including importFromFantome.isPending and/or fantomeProgress in this flag.
| isImporting={importFromModpkg.isPending || peekFantome.isPending} | |
| isImporting={ | |
| importFromModpkg.isPending || | |
| peekFantome.isPending || | |
| importFromFantome.isPending || | |
| Boolean(fantomeProgress) | |
| } |
| const form = useAppForm({ | ||
| defaultValues: { | ||
| name: peekResult?.suggestedName ?? "", | ||
| displayName: peekResult?.name ?? "", | ||
| }, |
There was a problem hiding this comment.
useAppForm defaultValues are computed from peekResult, but React Form default values won’t automatically update when peekResult changes. As written, the fields can stay empty instead of being pre-filled after the peek completes. Consider resetting the form with the peek-derived values when filePath/peekResult changes (similar to the pattern used in EditLayerDialog).
|
|
||
| listen<FantomeImportProgress>("fantome-import-progress", (event) => { | ||
| setProgress(event.payload); | ||
|
|
||
| if (event.payload.stage === "complete" || event.payload.stage === "error") { | ||
| setTimeout(() => setProgress(null), 1000); | ||
| } | ||
| }).then((fn) => { | ||
| unlisten = fn; | ||
| }); | ||
|
|
||
| return () => { | ||
| if (unlisten) { | ||
| unlisten(); | ||
| } |
There was a problem hiding this comment.
The setTimeout(() => setProgress(null), 1000) isn’t cleared on unmount, which can trigger a state update after the hook’s consumer unmounts. Consider storing the timeout id and clearing it in the effect cleanup (and optionally guarding the listen(...).then(...) unlisten assignment similarly).
| listen<FantomeImportProgress>("fantome-import-progress", (event) => { | |
| setProgress(event.payload); | |
| if (event.payload.stage === "complete" || event.payload.stage === "error") { | |
| setTimeout(() => setProgress(null), 1000); | |
| } | |
| }).then((fn) => { | |
| unlisten = fn; | |
| }); | |
| return () => { | |
| if (unlisten) { | |
| unlisten(); | |
| } | |
| let timeoutId: ReturnType<typeof setTimeout> | null = null; | |
| let isMounted = true; | |
| listen<FantomeImportProgress>("fantome-import-progress", (event) => { | |
| setProgress(event.payload); | |
| if (event.payload.stage === "complete" || event.payload.stage === "error") { | |
| if (timeoutId !== null) { | |
| clearTimeout(timeoutId); | |
| } | |
| timeoutId = setTimeout(() => { | |
| if (isMounted) { | |
| setProgress(null); | |
| } | |
| }, 1000); | |
| } | |
| }).then((fn) => { | |
| if (!isMounted) { | |
| // Effect already cleaned up; immediately unlisten to avoid leaks. | |
| fn(); | |
| return; | |
| } | |
| unlisten = fn; | |
| }); | |
| return () => { | |
| isMounted = false; | |
| if (unlisten) { | |
| unlisten(); | |
| } | |
| if (timeoutId !== null) { | |
| clearTimeout(timeoutId); | |
| timeoutId = null; | |
| } |
src-tauri/src/workshop/projects.rs
Outdated
| let target_path = base_dir.join(wad_name).join(rel); | ||
| if let Some(parent) = target_path.parent() { | ||
| fs::create_dir_all(parent)?; | ||
| } | ||
| let mut out = fs::File::create(&target_path)?; | ||
| std::io::copy(&mut entry, &mut out)?; |
There was a problem hiding this comment.
Directory-style Fantome extraction writes ZIP entry paths to disk using rel directly. A crafted archive can include ../ or absolute paths in entry names (Zip Slip), causing writes outside project_dir. Please sanitize/validate entry paths (e.g., reject ../absolute components and ensure the final path stays within base_dir.join(wad_name)), or use ZipFile::enclosed_name() to get a safe path.
src-tauri/src/workshop/projects.rs
Outdated
| extract_fantome_file( | ||
| &mut archive, | ||
| "meta/readme.md", | ||
| &project_dir.join("README.md"), | ||
| ); | ||
| extract_fantome_file(&mut archive, "readme.md", &project_dir.join("README.md")); |
There was a problem hiding this comment.
README extraction calls extract_fantome_file twice unconditionally (meta/readme.md then readme.md). If both exist, the second call can overwrite the first. Consider making this a fallback (only try readme.md if meta/readme.md was not found) by having extract_fantome_file return a boolean/Result indicating whether extraction happened.
| extract_fantome_file( | |
| &mut archive, | |
| "meta/readme.md", | |
| &project_dir.join("README.md"), | |
| ); | |
| extract_fantome_file(&mut archive, "readme.md", &project_dir.join("README.md")); | |
| let readme_path = project_dir.join("README.md"); | |
| extract_fantome_file(&mut archive, "meta/readme.md", &readme_path); | |
| if !readme_path.exists() { | |
| extract_fantome_file(&mut archive, "readme.md", &readme_path); | |
| } |
src-tauri/src/workshop/projects.rs
Outdated
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
extract_fantome_wad returns Ok(()) even when no directory entries and no packed WAD entry are found. This can silently produce an imported project with missing content. Consider returning an AppError::Fantome when the requested wad_name cannot be located/extracted.
| Ok(()) | |
| Err(AppError::Fantome(format!( | |
| "Failed to locate or extract WAD '{wad_name}' from Fantome archive" | |
| ))) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src-tauri/src/workshop/projects.rs
Outdated
| self.emit_git_progress("error", None); | ||
| } | ||
|
|
||
| let _ = fs::remove_dir_all(&temp_dir); |
There was a problem hiding this comment.
The temporary directory cleanup at line 556 happens after returning the result, which means if the operation succeeds, we're trying to remove a directory that was already renamed/moved. This is benign since the directory no longer exists at that path, but it's unnecessary. More importantly, if an error occurs before line 546 (the rename), the cleanup is attempted inside the error path at line 552, but the final cleanup at line 556 will also run. Consider restructuring to only clean up the temp directory when it still exists, or move the cleanup into the error-only path.
| let _ = fs::remove_dir_all(&temp_dir); | |
| if temp_dir.exists() { | |
| let _ = fs::remove_dir_all(&temp_dir); | |
| } |
| "URL must include owner and repository name (https://github.com/owner/repo)" | ||
| .to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The URL parsing logic accepts URLs with extra path segments beyond the repository name. For example, "https://github.com/owner/repo/issues/123" would be parsed as owner="owner" and repo="repo", but the extra path "/issues/123" is silently ignored. This could lead to user confusion. Consider validating that there are exactly 2 path segments (owner and repo) or documenting that extra segments are ignored.
| } | |
| } | |
| if parts.len() > 2 { | |
| return Err(AppError::ValidationFailed( | |
| "URL must not contain extra path segments beyond owner and repository name (https://github.com/owner/repo)" | |
| .to_string(), | |
| )); | |
| } |
src/routes/workshop/index.tsx
Outdated
| if (searchQuery) return <NoSearchResultsState />; | ||
| return ( | ||
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportModpkg} /> | ||
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportFantome} /> |
There was a problem hiding this comment.
The NoProjectsState "Import" button now defaults to importing Fantome archives instead of Modpkg files. This changes the user experience for the empty state. Previously users would import Modpkg files by default, now they'll import Fantome files. If this is intentional to promote the Fantome format, consider updating the button text or description to clarify this (e.g., "Import Fantome" or add a description mentioning it). Alternatively, consider making this button open the same dropdown menu as in the toolbar to give users all import options.
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportFantome} /> | |
| <NoProjectsState onCreate={() => setNewProjectOpen(true)} onImport={handleImportModpkg} /> |
| webp = "0.3" | ||
| slug = "0.1" | ||
|
|
||
| reqwest = { version = "0.12", features = ["blocking"] } |
There was a problem hiding this comment.
The reqwest dependency is configured with only the "blocking" feature, which is appropriate for the current blocking implementation. However, if you convert the import_from_git_repo function to async (as recommended in another comment), you should remove the "blocking" feature and let reqwest use its default async implementation, or explicitly enable the necessary features for async operation.
| reqwest = { version = "0.12", features = ["blocking"] } | |
| reqwest = "0.12" |
| .string() | ||
| .min(1, "Repository URL is required") | ||
| .regex( | ||
| /^https?:\/\/github\.com\/[^/]+\/[^/]+/, |
There was a problem hiding this comment.
The URL validation regex allows extra path segments after the repository name (e.g., "https://github.com/owner/repo/issues" would pass validation). While the backend parse_github_url function also accepts and ignores these extra segments, the validation message suggests it should be exactly "https://github.com/owner/repo". Consider making the regex stricter by adding an end anchor and optional trailing slash: /^https?:\/\/github\.com\/[^/]+\/[^/]+(\/)?$/. This would provide clearer validation feedback to users and match the expected format shown in the error message.
| /^https?:\/\/github\.com\/[^/]+\/[^/]+/, | |
| /^https?:\/\/github\.com\/[^/]+\/[^/]+(\/)?$/, |
| let mut entry = archive.by_index(i)?; | ||
| let target_path = base_dir.join(wad_name).join(rel); | ||
| if let Some(parent) = target_path.parent() { | ||
| fs::create_dir_all(parent)?; | ||
| } | ||
| let mut out = fs::File::create(&target_path)?; | ||
| std::io::copy(&mut entry, &mut out)?; |
There was a problem hiding this comment.
This code extracts ZIP entries without validating that the extracted paths don't escape the target directory through path traversal attacks (e.g., entries with "../" in their names). While wad_name is controlled by the scan_fantome_wad_names function which filters to specific patterns, rel (the relative path within a WAD directory) is not validated against path traversal. A malicious ZIP file could potentially write files outside the intended directory. Consider using the zip crate's built-in path sanitization or explicitly checking that the canonicalized target_path is still within base_dir before writing.
| let response = reqwest::blocking::get(&tarball_url) | ||
| .map_err(|e| AppError::Other(format!("Failed to download repository: {}", e)))?; |
There was a problem hiding this comment.
Using reqwest::blocking::get in a Tauri command will block the entire async runtime thread, which can cause UI freezes and poor performance. Tauri commands are async by default for a reason. Replace this with the async version: use reqwest::get(...).await and make the function and command async. This will require changing the command signature to pub async fn import_from_git_repo(...) and using the non-blocking reqwest client.
…ng in overlay and import processes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/routes/workshop/index.tsx
Outdated
| importFromModpkg.isPending || | ||
| peekFantome.isPending || | ||
| importFromFantome.isPending || | ||
| Boolean(fantomeProgress) |
There was a problem hiding this comment.
The isImporting prop doesn't include git import progress state. When a Git repository import is in progress, the toolbar's import button should also show the loading state. Add Boolean(gitImportProgress) to the conditions, similar to how Boolean(fantomeProgress) is included for Fantome imports.
| Boolean(fantomeProgress) | |
| Boolean(fantomeProgress) || | |
| Boolean(gitImportProgress) |
|
|
||
| self.emit_git_progress("extracting", None); | ||
|
|
||
| let temp_dir = workshop_path.join(format!(".git-import-{}", uuid::Uuid::new_v4())); |
There was a problem hiding this comment.
The code uses uuid::Uuid::new_v4() at line 501 but there's no import statement for the uuid crate in this file. This will cause a compilation error. Add use uuid::Uuid; or use the fully qualified path consistently if that's the convention. Looking at other files in the codebase (e.g., src-tauri/src/mods/profiles.rs), the convention is to import use uuid::Uuid; at the top of the file.
| let decoder = flate2::read::GzDecoder::new(std::io::Cursor::new(&bytes)); | ||
| let mut archive = tar::Archive::new(decoder); | ||
| archive.unpack(&temp_dir)?; |
There was a problem hiding this comment.
The tar archive extraction at line 507 uses archive.unpack(&temp_dir) without explicitly validating or sanitizing paths. While the Rust tar crate has some built-in protections against path traversal attacks (it will reject entries with .. components by default), it's best practice to explicitly set archive.set_preserve_mtime(false) and potentially archive.set_preserve_permissions(false) before unpacking to avoid time-of-check-time-of-use (TOCTOU) issues and permission-related vulnerabilities. Additionally, consider validating that all extracted paths are within the expected directory after extraction.
| displayName: peekResult.name ?? "", | ||
| }); | ||
| } | ||
| }, [form, filePath, peekResult]); |
There was a problem hiding this comment.
The useEffect has form in its dependency array, which can cause infinite re-renders because form is an object that may be recreated on every render. The form.reset function should be stable, so only peekResult and filePath should be in the dependency array. This is a common pattern issue with form libraries.
| }, [form, filePath, peekResult]); | |
| }, [filePath, peekResult]); |
…atus in workshop index
No description provided.