[WIP] fix(modpack): 将 CurseForge API Key 改为编译期可选常量#118
Conversation
|
@BegoniaHe is attempting to deploy a commit to the retrofor Team on Vercel. A member of the Team first needs to authorize it. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @BegoniaHe, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
Pull request overview
This PR aims to make the CurseForge API key optional at compile time to avoid local build failures when CURSEFORGE_API_KEY is unset, but it also includes substantial additional backend/frontend functionality changes (game lifecycle, instance import/export/repair, Java module refactors, dev server/CSP tweaks).
Changes:
- Make CurseForge API key compile-time optional via
option_env!()and add.envhandling in the build script. - Add game lifecycle tracking (start/stop +
game-exitedpayload), instance operations locking, and instance import/export/repair APIs. - Refactor parts of the Java subsystem (provider retries/bounded concurrency, persistence/cache, detection split) and update the React UI to use the new lifecycle/events and instance tooling.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src-tauri/build.rs |
Attempts to load .env during build to support optional build-time vars. |
src-tauri/Cargo.toml |
Adds dotenvy as a build-dependency. |
src-tauri/src/core/modpack.rs |
Switches CurseForge key to option_env!() and errors gracefully when missing. |
src-tauri/tauri.conf.json |
Tweaks dev URL and dev CSP configuration. |
src-tauri/capabilities/default.json |
Allows dev-server remote URLs for Tauri capabilities. |
src-tauri/src/utils/zip.rs |
Adds path sanitization for archive extraction to prevent traversal. |
src-tauri/src/main.rs |
Introduces GameProcessState, stop_game, path resolution changes, and new instance commands. |
src-tauri/src/core/instance.rs |
Adds per-instance operation locking, shared-cache path resolution, and import/export/repair features. |
src-tauri/src/core/downloader.rs |
Hardens Java download queue persistence with a global file lock + atomic writes + helper APIs. |
src-tauri/src/core/java/mod.rs |
Adds provider-pluggable APIs and new resume result types; changes TS export paths. |
src-tauri/src/core/java/error.rs |
Adds structured resume-failure reason enum + tests; changes TS export paths. |
src-tauri/src/core/java/cache.rs |
Moves Java catalog cache logic into a dedicated module with atomic writes. |
src-tauri/src/core/java/persistence.rs |
Uses atomic write strategy for Java config persistence; adds load_java_config_result. |
src-tauri/src/core/java/providers/adoptium.rs |
Adds bounded concurrency and retry/backoff for catalog requests + tests. |
src-tauri/src/core/java/validation.rs |
Reads Java version from stdout when stderr is empty. |
src-tauri/src/core/java/install.rs |
Extracts Java download+install flow into a dedicated module. |
src-tauri/src/core/java/detection/* |
Splits Java detection into common/linux/macos/windows/unix modules (replacing monolith). |
packages/ui/vite.config.ts |
Pins dev server to 127.0.0.1:5173 with strict port. |
packages/ui/src/client.ts |
Adds new instance APIs and stop-game; (still types resumeJavaDownloads as old return type). |
packages/ui/src/stores/game-store.ts |
Adds lifecycle listener + start/stop integration and per-instance running/launching state. |
packages/ui/src/pages/index.tsx |
Initializes game lifecycle listener on app load. |
packages/ui/src/pages/instances-view.tsx |
Adds start/stop controls per instance + import/export/repair UI actions. |
packages/ui/src/components/bottom-bar.tsx |
Migrates start/stop behavior to the centralized game store and instance selection UI. |
packages/ui/src/models/instance.ts |
Adds import/export/repair helpers and more robust active-instance handling. |
packages/ui/src/models/auth.ts |
Improves login error handling and polling interval safety. |
.gitignore |
Ignores .env / .env.local and adds .vscode/. |
.env.example |
Documents optional CURSEFORGE_API_KEY setup. |
.github/copilot-instructions.md |
Updates contributor instructions (currently references non-existent packages/ui-new). |
Comments suppressed due to low confidence (1)
src-tauri/src/core/instance.rs:353
delete_instanceacquires an operation lock viabegin_operation, but there are multiple early-?returns (e.g. instance not found,save()failure, directory deletion failure) beforeend_operationis called. That can permanently “busy-lock” an instance. Ensureend_operationis always executed (e.g. RAII guard /dropguard pattern, or structuredmatch/finally-style cleanup).
pub fn delete_instance(&self, id: &str) -> Result<(), String> {
self.begin_operation(id, InstanceOperation::Delete)?;
let mut config = self.instances.lock().unwrap();
// Find the instance
let instance_index = config
.instances
.iter()
.position(|i| i.id == id)
.ok_or_else(|| format!("Instance {} not found", id))?;
You can also share your feedback on Copilot code review. Take the survey.
src-tauri/src/core/java/error.rs
Outdated
| #[ts( | ||
| export, | ||
| export_to = "../../packages/ui-new/src/types/bindings/java/index.ts" | ||
| )] |
There was a problem hiding this comment.
ResumeJavaDownloadFailureReason is exported via ts-rs to ../../packages/ui-new/..., but this repo uses TS_RS_EXPORT_DIR=./packages/ui/src/types/bindings and doesn’t have packages/ui-new. This will break/fragment the generated bindings. Export it relative to the configured bindings dir instead.
| #[ts( | |
| export, | |
| export_to = "../../packages/ui-new/src/types/bindings/java/index.ts" | |
| )] | |
| #[ts(export, export_to = "java/index.ts")] |
| export function resumeJavaDownloads(): Promise<JavaInstallation[]> { | ||
| return invoke<JavaInstallation[]>("resume_java_downloads"); | ||
| } |
There was a problem hiding this comment.
resume_java_downloads now returns ResumeJavaDownloadsResult on the Rust side, but the TS client still types it as Promise<JavaInstallation[]>. This will cause a type mismatch and likely runtime breakage for callers. Update the client type (and any consumers) to match the new return shape.
| pub fn export_instance(&self, id: &str, archive_path: &Path) -> Result<PathBuf, String> { | ||
| self.begin_operation(id, InstanceOperation::ImportExport)?; | ||
| let instance = self | ||
| .get_instance(id) | ||
| .ok_or_else(|| format!("Instance {} not found", id))?; |
There was a problem hiding this comment.
export_instance takes an operation lock (begin_operation) but only releases it at the very end. Any error during file creation, zip writing, or directory traversal will return early and keep the lock held. Ensure end_operation runs on all paths (RAII guard / scope guard).
| let imported = self.create_instance(final_name, app_handle)?; | ||
| self.begin_operation(&imported.id, InstanceOperation::ImportExport)?; | ||
|
|
||
| for index in 0..archive.len() { | ||
| let mut entry = archive.by_index(index).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
import_instance calls begin_operation(&imported.id, ...) but does not guarantee end_operation on error paths (zip iteration/extraction, update_instance failure, etc.). If any of those ? return, the instance can remain locked as “busy”. Use a scope guard to always release the operation lock.
.github/copilot-instructions.md
Outdated
| @@ -33,59 +34,80 @@ src-tauri/ # Rust backend | |||
| version_merge.rs # Parent version inheritance merging | |||
| utils/ | |||
| zip.rs # Native library extraction | |||
| ui/ # Svelte 5 frontend | |||
| src/ | |||
| App.svelte # Main app component, enforces dark mode | |||
| stores/ # Svelte 5 runes state management ($state, $effect) | |||
| auth.svelte.ts # Authentication state with device code polling | |||
| game.svelte.ts # Game state (running, logs) | |||
| settings.svelte.ts # Settings + Java detection | |||
| ui.svelte.ts # UI state (toasts, modals, active view) | |||
| components/ # UI components (HomeView, VersionsView, SettingsView, etc.) | |||
| lib/ # Reusable components (DownloadMonitor, GameConsole) | |||
| packages/ | |||
| ui-new/ # React frontend used by Tauri | |||
| src/ | |||
| main.tsx # React Router setup (hash routing) | |||
| pages/ # Route views (Home, Versions, Settings, ...) | |||
| stores/ # Zustand stores | |||
| components/ # UI components | |||
| ui/ # Legacy Svelte 5 frontend | |||
| src/ | |||
| App.svelte # Main app component, enforces dark mode | |||
| stores/ # Svelte 5 runes state management ($state, $effect) | |||
| auth.svelte.ts # Authentication state with device code polling | |||
| game.svelte.ts # Game state (running, logs) | |||
| settings.svelte.ts # Settings + Java detection | |||
| ui.svelte.ts # UI state (toasts, modals, active view) | |||
| components/ # UI components (HomeView, VersionsView, SettingsView, etc.) | |||
| lib/ # Reusable components (DownloadMonitor, GameConsole) | |||
| ``` | |||
|
|
|||
| ## Critical Development Workflows | |||
|
|
|||
| ### Development Mode | |||
|
|
|||
| ```bash | |||
| cargo tauri dev # Starts frontend dev server (Vite on :5173) + Tauri window | |||
| cargo tauri dev # Runs ui-new dev server (Vite on :5173) + Tauri window | |||
| ``` | |||
| - Frontend uses **Rolldown-based Vite fork** (`npm:rolldown-vite@7.2.5`) with hot reload | |||
|
|
|||
| - `src-tauri/tauri.conf.json` runs `pnpm --filter @dropout/ui-new dev` | |||
| - Frontend uses **Rolldown-based Vite fork** (`npm:rolldown-vite@^7`) with hot reload | |||
There was a problem hiding this comment.
.github/copilot-instructions.md now references packages/ui-new as the active React UI and updates commands (pnpm --filter @dropout/ui-new ...), but this repo only contains packages/ui (no ui-new). This makes the contributor instructions incorrect/misleading; update the paths/commands to match the actual workspace layout.
| if let Ok(path) = dotenvy::dotenv() { | ||
| // Tell Cargo to re-run this script if .env changes | ||
| println!("cargo:rerun-if-changed={}", path.display()); | ||
| } |
There was a problem hiding this comment.
dotenvy::dotenv() in a build script only affects the build-script process; it does not automatically make CURSEFORGE_API_KEY visible to the crate compilation where option_env!() is evaluated. As written, values from .env likely won’t be embedded, contradicting the comment and PR description. If you want .env to inject the key at compile time, emit cargo:rustc-env=CURSEFORGE_API_KEY=... when the var is present (and keep the rerun-if-* lines).
| } | |
| } | |
| // If CURSEFORGE_API_KEY is set (from .env or the environment), forward it to rustc | |
| if let Ok(value) = std::env::var("CURSEFORGE_API_KEY") { | |
| println!("cargo:rustc-env=CURSEFORGE_API_KEY={}", value); | |
| } |
| struct RunningGameProcess { | ||
| child: Child, | ||
| instance_id: String, | ||
| version_id: String, | ||
| } | ||
|
|
||
| pub struct GameProcessState { | ||
| running_game: AsyncMutex<Option<RunningGameProcess>>, | ||
| } | ||
|
|
||
| impl Default for GameProcessState { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } | ||
|
|
||
| impl GameProcessState { | ||
| pub fn new() -> Self { | ||
| Self { | ||
| running_game: AsyncMutex::new(None), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| struct GameExitedEvent { | ||
| instance_id: String, | ||
| version_id: String, | ||
| exit_code: Option<i32>, | ||
| was_stopped: bool, | ||
| } | ||
|
|
There was a problem hiding this comment.
PR metadata/description focuses on making CURSEFORGE_API_KEY optional at build time, but this PR also adds game lifecycle management, instance import/export/repair commands, Java module refactors, CSP/dev server changes, etc. That scope mismatch makes it hard to review/land safely; consider splitting into focused PRs (API key fix vs lifecycle/instance/java changes).
| self.begin_operation(id, InstanceOperation::ImportExport)?; | ||
| let source_instance = self | ||
| .get_instance(id) | ||
| .ok_or_else(|| format!("Instance {} not found", id))?; | ||
|
|
||
| { |
There was a problem hiding this comment.
duplicate_instance calls begin_operation but only calls end_operation on the success path. Any error before line 496 (missing source instance, invalid name, copy failure, insert/save failure) will leave the operation lock held and block future operations on that instance. Add guaranteed cleanup for the lock.
| let data = fs::read(&path).map_err(|e| e.to_string())?; | ||
| writer.write_all(&data).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
append_directory_to_zip reads each file fully into memory (fs::read) before writing to the zip. Large instance directories (world saves, logs, mods) can cause high peak memory usage or OOM. Prefer streaming via std::io::copy from a File into the zip writer, or chunked reads.
| let data = fs::read(&path).map_err(|e| e.to_string())?; | |
| writer.write_all(&data).map_err(|e| e.to_string())?; | |
| let mut file = fs::File::open(&path).map_err(|e| e.to_string())?; | |
| std::io::copy(&mut file, writer).map_err(|e| e.to_string())?; |
src-tauri/src/core/java/mod.rs
Outdated
| #[derive(Debug, Clone, Serialize, Deserialize, TS)] | ||
| #[ts( | ||
| export, | ||
| export_to = "../../packages/ui-new/src/types/bindings/java/index.ts" | ||
| )] | ||
| pub struct ResumeJavaDownloadFailure { | ||
| pub major_version: u32, | ||
| pub image_type: String, | ||
| pub install_path: String, | ||
| pub reason: ResumeJavaDownloadFailureReason, | ||
| pub error: String, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize, TS)] | ||
| #[ts( | ||
| export, | ||
| export_to = "../../packages/ui-new/src/types/bindings/java/index.ts" | ||
| )] | ||
| pub struct ResumeJavaDownloadsResult { | ||
| pub successful_installations: Vec<JavaInstallation>, | ||
| pub failed_downloads: Vec<ResumeJavaDownloadFailure>, | ||
| pub total_pending: usize, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, TS)] | ||
| #[serde(rename_all = "lowercase")] | ||
| #[ts( | ||
| export, | ||
| export_to = "../../packages/ui-new/src/types/bindings/java/index.ts" | ||
| )] |
There was a problem hiding this comment.
The #[ts(export_to = "../../packages/ui-new/..." )] paths point to a non-existent packages/ui-new directory in this repo and also bypass the existing TS_RS_EXPORT_DIR configuration (see .cargo/config.toml). This will break ts-rs type generation and leave the frontend bindings out of sync. Please export relative to the configured bindings dir (e.g. java/index.ts under packages/ui/src/types/bindings) and keep all exports consistent.
Replace env!("CURSEFORGE_API_KEY") with option_env!() so the build never
fails when the key is absent. dotenvy in build.rs reads an optional .env
file, letting maintainers supply the key locally without touching system
environment variables. .gitignore updated to exclude .env files.
Reviewed-by: Claude Sonnet 4.6
BegoniaHe
force-pushed
the
fix/curseforge-api-key
branch
from
02bbbae to
41163b9
Compare
HsiangNianian
merged commit 8c913f2
into
HydroRoll-Team:feature/multi-instances
fix(modpack): 将 CurseForge API Key 改为编译期可选常量
修复
env!()宏在开发者本地无CURSEFORGE_API_KEY时导致编译失败的问题，改用option_env!()+ build.rs 中的dotenvy读取 .env 文件，实现编译期嵌入、缺失时优雅降级。更改类型
LLM 生成代码声明
相关 Issue
相关 #110 #117
更改内容
后端 (Rust)
env!("CURSEFORGE_API_KEY")替换为const CURSEFORGE_API_KEY: Option<&str> = option_env!("CURSEFORGE_API_KEY")，key 不存在时编译为None，调用 CurseForge 功能时返回友好错误而非 panicdotenvy::dotenv()调用，允许通过 .env 文件在编译期注入 key，并注册cargo:rerun-if-changed/cargo:rerun-if-env-changed确保增量构建正确前端 (Svelte)
配置
[build-dependencies]中添加dotenvy = { version = "0.15", default-features = false }.env.local忽略规则，防止 key 被意外提交测试
测试环境
测试用例
测试步骤
CURSEFORGE_API_KEY，不创建 .env 文件，直接执行cargo check→ 应编译通过（无报错）CURSEFORGE_API_KEY=test_key，执行cargo check→ 应编译通过，key 被嵌入二进制检查清单
代码质量
测试验证
文档更新
依赖项
附加说明
dotenvy仅作为 build-dependency，不会进入最终二进制。官方发布构建通过 CI 环境变量注入 key，普通开发者无需任何操作即可正常编译和运行。