Bundle dlls for windows app #1121
Conversation
|
Warning Rate limit exceeded@devgony has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Tauri bundle resources for DLLs in stable/staging configs, updates CI to emit Windows staging artifacts and limit tarball to macOS, refreshes English/Korean locale payloads, silences unused parameter warnings in Windows detectors, and implements macOS app listing with a non-macOS stub. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Detect as detect::list
participant FS as FileSystem (/Applications, ~/Applications)
participant Plist as Info.plist Parser
User->>Detect: list_installed_apps()
alt macOS
Detect->>FS: DFS .app directories
loop For each .app
Detect->>Plist: Read Contents/Info.plist
Plist-->>Detect: CFBundleIdentifier, DisplayName/Name
Detect-->>Detect: Build InstalledApp entry
end
Detect-->>User: Sorted Vec<InstalledApp>
else non-macOS
Detect-->>User: []
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
apps/desktop/src-tauri/dlls/msvcp140.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/msvcp140_1.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/vcruntime140.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/vcruntime140_1.dllis excluded by!**/*.dll
📒 Files selected for processing (1)
apps/desktop/src-tauri/tauri.conf.stable.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
- Add panic hook to capture crashes before ORT init - Add detailed logging to ONNX model loading - Add logging to Whisper model initialization steps - Disable Sentry DSN in debug builds for local crash handling - This will help identify exact failure point on new laptops
- Disable GPU processing to isolate issue - Add granular logging around ctx.create_state() call - Crash occurs on Intel Core Ultra 7 155U vs working i7-11700 - Root cause: CPU instruction set incompatibility in whisper.cpp - Need to rebuild whisper-rs with broader CPU compatibility
…y validation - Fix ORT_DYLIB_PATH to use installation directory instead of specific DLL path for better flexibility - Add comprehensive installer validation for all required DLLs (onnxruntime.dll, DirectML.dll, Visual C++ runtime) - Add runtime dependency checks before AI initialization with detailed logging and graceful error handling - Force environment variable refresh during installation to ensure immediate availability This addresses deployment failures on Windows laptops without Visual Studio installed by providing better diagnostics and validation.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
apps/desktop/src-tauri/src/lib.rs (2)
38-41: Keep debug DSN overrideable via env instead of hard-disabling Sentry.Allow opting into Sentry in debug without code changes.
- { - "" - } + { + option_env!("SENTRY_DSN").unwrap_or("") + }
47-49: Confirm ClientOptions field name; prefer creating the cache dir explicitly.Please verify that
database_pathis a valid field for yoursentrycrate version; many versions usecache_dir. Also ensure the directory exists.- #[cfg(debug_assertions)] - database_path: Some(std::env::temp_dir().join("hyprnote_crashes")), + #[cfg(debug_assertions)] + { + let cache = std::env::temp_dir().join("hyprnote_crashes"); + let _ = std::fs::create_dir_all(&cache); + // If your version uses `cache_dir`, prefer it: + cache_dir: Some(cache) + },apps/desktop/src-tauri/windows/hooks.nsh (1)
10-27: Tighten vcredist install flow and fix typo.
- Fix “wether” → “whether”.
- If the MSI isn’t present, log once and continue.
- Keep the bundled MSI (optional), or gate deletion on successful install.
; Install from bundled MSI if not installed ${If} ${FileExists} "$INSTDIR\resources\vc_redist.x64.msi" DetailPrint "Installing Visual C++ Redistributable..." @@ - ; Check wether installation process exited successfully (code 0) or not + ; Check whether installation process exited successfully (code 0) or not ${If} $0 == 0 DetailPrint "Visual C++ Redistributable installed successfully" ${Else} MessageBox MB_ICONEXCLAMATION "Visual C++ installation failed. Some features may not work." ${EndIf} - ; Clean up setup files from TEMP and your installed app + ; Clean up setup files from TEMP Delete "$TEMP\vc_redist.x64.msi" - Delete "$INSTDIR\resources\vc_redist.x64.msi" ${EndIf} + ${IfNot} ${FileExists} "$INSTDIR\resources\vc_redist.x64.msi" + DetailPrint "Skipped vcredist install: bundled MSI not found" + ${EndIf}apps/desktop/src-tauri/src/ext.rs (4)
38-47: Minor: lift DLL lists to consts for readability and reuse.Keeps the function focused and avoids allocations.
- // Check optional DLLs (warn but don't fail) - let optional_dlls = ["msvcp140_1.dll", "vcruntime140_1.dll"]; + // Optional (warn-only) + const OPTIONAL_DLLS: &[&str] = &["msvcp140_1.dll", "vcruntime140_1.dll", "DirectML.dll"]; - for dll_name in &optional_dlls { + for dll_name in OPTIONAL_DLLS {
49-71: Set ORT_DYLIB_PATH for the current process when missing or invalid.Reduces reliance on global env and improves first-run success after install.
// Check ORT_DYLIB_PATH environment variable match std::env::var("ORT_DYLIB_PATH") { Ok(path) => { @@ - } else { - tracing::warn!("ORT_DYLIB_PATH points to non-existent path: {}", path); - } + } else { + tracing::warn!("ORT_DYLIB_PATH points to non-existent path: {}", path); + } }, Err(_) => { - tracing::warn!("ORT_DYLIB_PATH environment variable not set"); + tracing::warn!("ORT_DYLIB_PATH environment variable not set; defaulting to app dir"); + std::env::set_var("ORT_DYLIB_PATH", app_dir); } }
9-82: Guideline check: comments read as “what”, not “why”.Consider trimming or rephrasing comments to capture the rationale (e.g., “Validate before ORT init to fail fast with actionable diagnostics”) per repo guidelines.
84-90: Trait uses RPITIT (impl Trait in trait returns). Confirm MSRV.Ensure the toolchain supports RPITIT; otherwise use
async-traitor explicit boxed futures.If needed:
-pub trait AppExt<R: tauri::Runtime> { +#[async_trait::async_trait] +pub trait AppExt<R: tauri::Runtime + Send + Sync> { - fn setup_local_ai(&self) -> impl Future<Output = Result<(), String>>; + async fn setup_local_ai(&self) -> Result<(), String>; - fn setup_db_for_local(&self) -> impl Future<Output = Result<(), String>>; + async fn setup_db_for_local(&self) -> Result<(), String>; - fn setup_db_for_cloud(&self) -> impl Future<Output = Result<(), String>>; + async fn setup_db_for_cloud(&self) -> Result<(), String>; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapps/desktop/src-tauri/dlls/DirectML.dllis excluded by!**/*.dll
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/ext.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(1 hunks)apps/desktop/src-tauri/tauri.conf.stable.json(1 hunks)apps/desktop/src-tauri/windows/hooks.nsh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/tauri.conf.stable.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/ext.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/ext.rs (2)
crates/moonshine/src/model.rs (1)
new(28-55)plugins/local-stt/src/model.rs (1)
info(186-199)
🔇 Additional comments (1)
apps/desktop/src-tauri/windows/hooks.nsh (1)
31-40: LGTM overall.Post-install validation and user-facing messages are clear and helpful.
Also applies to: 48-66, 80-82
- Make msvcp140_1.dll and vcruntime140_1.dll required for whisper-rs - Add panic catch around ctx.create_state() to handle native library crashes - Provide clear error messages when whisper dependencies are missing
- Wrap WhisperError in Error::LocalWhisperError for proper type conversion - Ensure panic handling returns correct error type
- Check for required SSE2 support before creating whisper state - Log AVX availability for performance diagnostics - Prevent crashes on CPUs missing required instruction sets
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/ext.rs (2)
20-27: Don’t hard-require DirectML.dll; keep GPU optional.Blocking on DirectML prevents CPU-only usage and contradicts installer behavior.
- let required_dlls = [ - "onnxruntime.dll", - "DirectML.dll", - "msvcp140.dll", - "vcruntime140.dll", - "msvcp140_1.dll", - "vcruntime140_1.dll" - ]; + let required_dlls = [ + "onnxruntime.dll", + "msvcp140.dll", + "vcruntime140.dll", + "msvcp140_1.dll", + "vcruntime140_1.dll", + ]; + // GPU path is optional: warn-only. + let optional_dlls = [ + "DirectML.dll", + "msvcp140_2.dll", + "msvcp140_atomic_wait.dll", + "concrt140.dll", + ];
116-124: Don’t bubble Err here if the caller stillunwrap()s; degrade gracefully or fix call site.Returning
Errwill panic at the existing call site (per prior review). Prefer logging and continuing; otherwise remove unwrap in lib.rs.- { - if let Err(e) = validate_ort_dependencies() { - tracing::error!("ONNX Runtime validation failed: {}", e); - return Err(format!("ONNX Runtime dependencies missing: {}", e)); - } - } + { + if let Err(e) = validate_ort_dependencies() { + tracing::error!("onnx_runtime_validation_failed {}", e); + // do not fail entire setup; servers below are already gated by presence + // of downloaded models and will fail to start if deps are truly missing + } + }Run to confirm the call site was fixed (no
.await.unwrap()onsetup_local_ai()):#!/bin/bash # Expected: zero matches printing `.await.unwrap()` on setup_local_ai rg -nP -C2 '\bsetup_local_ai\s*\(\s*\)\.await\.unwrap\(\)' -- apps/desktop/src-tauri | cat # Also check generic unwrap on awaited futures from setup_local_ai rg -nP -C2 '(?s)setup_local_ai\s*\(\s*\)\s*\.await\s*[\)\s]*\.\s*unwrap\(' -- apps/desktop/src-tauri | cat
🧹 Nitpick comments (4)
crates/whisper-local/src/model.rs (2)
55-62: Unify logging totracingfor span correlation.- log::info!("Model file exists, initializing WhisperContext with GPU={}", context_param.use_gpu); + tracing::info!("init_whisper_context gpu={}", context_param.use_gpu); - let ctx = WhisperContext::new_with_params(&model_path, context_param) - .map_err(|e| { - log::error!("Failed to create WhisperContext: {:?}", e); - e - })?; - log::info!("WhisperContext created successfully"); + let ctx = WhisperContext::new_with_params(&model_path, context_param).map_err(|e| { + tracing::error!("whisper_context_new_failed error={:?}", e); + e + })?; + tracing::info!("whisper_context_created");
63-81: CPU feature check: avoid redundant SSE2 on x86_64; gate logs under cfg.SSE2 is guaranteed on x86_64; logging “missing SSE2” there is dead code and noisy. Keep AVX advisory; only enforce SSE2 on 32-bit x86.
- log::info!("Creating WhisperState..."); - log::info!("Validating CPU features..."); - - // Check if CPU supports required features for whisper.cpp - #[cfg(target_arch = "x86_64")] - { - if !std::arch::is_x86_feature_detected!("sse2") { - log::error!("CPU missing SSE2 support required for whisper.cpp"); - return Err(crate::error::Error::LocalWhisperError(whisper_rs::WhisperError::InitError)); - } - log::info!("✓ CPU has SSE2 support"); - - if std::arch::is_x86_feature_detected!("avx") { - log::info!("✓ CPU has AVX support"); - } else { - log::warn!("CPU missing AVX - performance may be reduced"); - } - } + tracing::info!("creating_whisper_state"); + #[cfg(target_arch = "x86")] + { + if !std::arch::is_x86_feature_detected!("sse2") { + tracing::error!("cpu_missing_sse2 whisper_requires_sse2"); + return Err(crate::error::Error::LocalWhisperError( + whisper_rs::WhisperError::InitError, + )); + } + tracing::info!("cpu_has_sse2"); + } + #[cfg(target_arch = "x86_64")] + { + if std::arch::is_x86_feature_detected!("avx") { + tracing::info!("cpu_has_avx"); + } else { + tracing::warn!("cpu_no_avx perf_may_degrade=true"); + } + }apps/desktop/src-tauri/src/ext.rs (2)
40-49: Avoid duplicate optional list and prefer single source of truth.You define
optional_dllslater too; keep one list.- // Check optional DLLs (warn but don't fail) - let optional_dlls = ["msvcp140_2.dll", "msvcp140_atomic_wait.dll", "concrt140.dll"]; + // Optional DLLs (warn but don't fail) + // NOTE: DirectML enables GPU; absence should not fail startup. + let optional_dlls = ["DirectML.dll", "msvcp140_2.dll", "msvcp140_atomic_wait.dll", "concrt140.dll"];
13-27: Comment style: capture “why”, trim “what”.Comments narrate what the code does. Keep only intent/rationale to match guidelines.
Example:
- // Get the application directory + // why: resolve the directory to locate bundled DLLs next to the exe - // Check ORT_DYLIB_PATH environment variable as a fallback/override + // why: allow system/global ORT install to satisfy the runtime DLLAlso applies to: 40-49, 51-73
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/ext.rs(2 hunks)crates/whisper-local/src/model.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/whisper-local/src/model.rsapps/desktop/src-tauri/src/ext.rs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/lib.rs (1)
53-60: Chain existing panic hook and append to the log; current code clobbers prior hooks and overwrites the file.Without chaining, Sentry’s own hook won’t run; also use append to avoid truncation.
- // Add panic hook to capture crashes before ORT initialization - std::panic::set_hook(Box::new(|panic_info| { - let crash_file = std::env::temp_dir().join("hyprnote_panic.txt"); - let crash_msg = format!("PANIC: {}\nLocation: {:?}\nTime: {:?}\n", - panic_info, panic_info.location(), std::time::SystemTime::now()); - std::fs::write(&crash_file, crash_msg).ok(); - eprintln!("Crash logged to: {:?}", crash_file); - })); + // Capture crashes before ORT init, but preserve previously-registered hooks (e.g., Sentry) + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |panic_info| { + use std::io::Write; + let crash_file = std::env::temp_dir().join("hyprnote_panic.txt"); + let _ = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(&crash_file) + .and_then(|mut f| { + writeln!( + f, + "PANIC: {}\nLocation: {:?}\nTime: {:?}\n", + panic_info, + panic_info.location(), + std::time::SystemTime::now() + ) + }); + eprintln!("Crash logged to: {:?}", crash_file); + prev(panic_info); + }));
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/lib.rs (1)
38-41: Debug DSN set to empty string disables Sentry — confirm this behavior is intended.Passing "" disables the Sentry client in Rust; if you still want opt‑in local reporting, consider allowing an env override. (docs.rs, getsentry.github.io)
Apply to keep debug disabled by default but allow
SENTRY_DSNlocally:- "" + option_env!("SENTRY_DSN").unwrap_or("")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/lib.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src-tauri/src/lib.rs
- Set target-cpu=x86-64 and disable AVX/AVX2/FMA/F16C/BMI features - Add GGML/WHISPER environment variables to prevent CPU-specific optimizations - Ensure builds from 8th gen Intel work on 11th gen Intel laptops
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/whisper-local/build.rs (2)
9-12: Env vars set here won’t influence dependency compilation; unify config in one place
WHISPER_NO_{AVX,AVX2,FMA}set in this build.rs won’t affect whisper-rs since it’s already built. Prefer the existing .cargo/config (force = true) and drop these to avoid drift.- std::env::set_var("WHISPER_NO_AVX", "1"); - std::env::set_var("WHISPER_NO_AVX2", "1"); - std::env::set_var("WHISPER_NO_FMA", "1");
2-8: Tighten comments to “why”, and remove dead/commented code
Keep rationale (“avoid illegal instruction on older CPUs; broader Windows compatibility”), not “what”; remove commented CFLAGS/CXXFLAGS.- // Override whisper.cpp build with conservative CPU flags + // Why: ensure compatibility on older CPUs/VMs (no AVX/AVX2/FMA) to avoid illegal-instruction crashes on Windows. - // Set conservative CPU target for whisper.cpp - // std::env::set_var("CXXFLAGS", "-march=x86-64 -mtune=generic -mno-avx2 -mno-fma"); - // std::env::set_var("CFLAGS", "-march=x86-64 -mtune=generic -mno-avx2 -mno-fma");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/desktop_cd.yaml(1 hunks)crates/whisper-local/build.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/whisper-local/build.rs
crates/whisper-local/build.rs
Outdated
| @@ -0,0 +1,13 @@ | |||
| fn main() { | |||
| // Override whisper.cpp build with conservative CPU flags | |||
| println!("cargo:rustc-env=WHISPER_DONT_GENERATE_BINDINGS=1"); | |||
There was a problem hiding this comment.
cargo:rustc-env won’t reach dependency build scripts (whisper-rs); move this to .cargo/config
WHISPER_DONT_GENERATE_BINDINGS needs to be present before whisper-rs builds; setting it here affects only this crate’s compilation, not dependencies.
Apply:
- println!("cargo:rustc-env=WHISPER_DONT_GENERATE_BINDINGS=1");And set it globally (workspace-level .cargo/config.toml):
[env]
WHISPER_DONT_GENERATE_BINDINGS = { value = "1", force = true }🤖 Prompt for AI Agents
In crates/whisper-local/build.rs around line 3, the build script sets
WHISPER_DONT_GENERATE_BINDINGS via println!("cargo:rustc-env=...") which only
affects this crate and not dependent crates like whisper-rs; remove that println
from build.rs and instead add a workspace-level .cargo/config.toml with an [env]
table that sets WHISPER_DONT_GENERATE_BINDINGS = { value = "1", force = true }
so the variable is exported to dependency build scripts before they run.
- Changed whisper-rs dependency from version 0.15.0 to devgony's fork - Fixed API compatibility issues with the fork: - Renamed WhisperTokenId to WhisperToken - Updated segment retrieval methods to use full_get_segment_t0/t1/text - Fixed num_segments to handle Result type - Added Windows artifact upload to desktop_cd.yaml workflow for .msi and .exe files
Convert all environment variables in debug-config.toml from simple strings to table format with value and force fields to match the main config.toml structure and resolve merge conflicts.
- Add platform-specific compilation guards for macOS-only code using plist crate - Provide stub implementation for non-macOS platforms in list_installed_apps() - Fix unused variable warnings in Windows detector implementations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src-tauri/.cargo/debug-config.toml (1)
23-24: Avoid globally forcing DEBUG/_DEBUG in Cargo config.These env vars bleed into all C/C++ sub-builds and can switch code paths unintentionally (including third-party deps). Prefer driving debug via build type/profile, not global env.
-DEBUG = { value = "1", force = true } -_DEBUG = { value = "1", force = true } +# Rely on CMake/Cargo build types instead of hard forcing these here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/desktop_cd.yaml(2 hunks)apps/desktop/src-tauri/.cargo/debug-config.toml(1 hunks)apps/desktop/src-tauri/tauri.conf.stable.json(1 hunks)apps/desktop/src-tauri/tauri.conf.staging.json(1 hunks)crates/onnx/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src-tauri/tauri.conf.staging.json
- .github/workflows/desktop_cd.yaml
- apps/desktop/src-tauri/tauri.conf.stable.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (2)
crates/onnx/Cargo.toml (1)
6-12: Gate ORT’sload-dynamicbehind this crate feature
ORT 2.0.0-rc.10 is a wrapper for ONNX Runtime 1.22 — ensure any bundled onnxruntime.dll is v1.22.x when usingload-dynamicviaORT_DYLIB_PATH(docs.rs). Latest stableortis v1.16.3 (wrapper for ONNX Runtime 1.16) and still exposes theload-dynamicfeature (docs.rs).Apply one of these:
Option A (recommended: default-on but disable-able by dependents)
[features] -default = [] +default = ["load-dynamic"] cuda = ["ort/cuda"] coreml = ["ort/coreml"] directml = ["ort/directml"] load-dynamic = ["ort/load-dynamic"] [dependencies] -ort = { version = "=2.0.0-rc.10", features = ["ndarray", "load-dynamic"] } +ort = { version = "=2.0.0-rc.10", features = ["ndarray"] }Option B (always-on: remove pass-through feature)
[features] -default = [] -cuda = ["ort/cuda"] -coreml = ["ort/coreml"] -directml = ["ort/directml"] -load-dynamic = ["ort/load-dynamic"] +default = [] +cuda = ["ort/cuda"] +coreml = ["ort/coreml"] +directml = ["ort/directml"] [dependencies] ort = { version = "=2.0.0-rc.10", features = ["ndarray", "load-dynamic"] }apps/desktop/src-tauri/.cargo/debug-config.toml (1)
13-21: Drop forced /MDd flags and unify CRT selection via CMake
- In apps/desktop/src-tauri/.cargo/debug-config.toml, remove all explicit
/MDdflags and instead set
CMAKE_MSVC_RUNTIME_LIBRARY = { value = "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL", force = true }.- Verify no Rust
rustflagshard-pin the debug CRT and manually search the repo for any remainingMDd/msvcrtdreferences.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/onnx/Cargo.toml (2)
6-12: Don’t enableload-dynamictwice; make it opt‑in or always‑on—consistently.You already expose a crate feature
load-dynamic = ["ort/load-dynamic"], but the dependency also hard-enables it, making the crate feature moot. Prefer one of:
- Opt‑in: remove it from the dependency and keep the crate feature mapping.
- Always‑on: keep it in the dependency and remove the crate feature mapping.
Apply (opt‑in) to keep the feature meaningful:
-ort = { version = "=2.0.0-rc.10", features = ["ndarray", "load-dynamic"] } +ort = { version = "=2.0.0-rc.10", features = ["ndarray"] }Also applies to: 15-15
15-15: Relaxortversion to stable 1.16.3 and verify Windows dynamic‐loading setup
- You’re currently pinned to pre-release
2.0.0-rc.10, which is ahead of the latest stable1.16.3. Unless you require RC-only features, change toorort = "1.16.3"^1.16to pick up ongoing fixes.ORT_DYLIB_PATHis still the correct env var for locatingonnxruntime.dllon Windows.- Address known Windows caveats when using
load-dynamic:
• System32 may contain an olderonnxruntime.dll; bundle the correct DLL alongside your executable.
•ORT_DYLIB_PATHis resolved relative to the executable’s working directory (tests/examples undertarget/<profile>/…can break).
• Leveragecopy-dylibsfor binary targets; manually copy DLLs for tests, benchmarks, and examples.apps/desktop/src-tauri/.cargo/debug-config.toml (1)
26-26: Typo in comment.“# ORG” → “# ORT”.
-# ORG +# ORT
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/desktop_cd.yaml(2 hunks)apps/desktop/src-tauri/.cargo/debug-config.toml(1 hunks)apps/desktop/src-tauri/tauri.conf.stable.json(1 hunks)apps/desktop/src-tauri/tauri.conf.staging.json(1 hunks)crates/onnx/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src-tauri/tauri.conf.stable.json
- .github/workflows/desktop_cd.yaml
- apps/desktop/src-tauri/tauri.conf.staging.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (1)
apps/desktop/src-tauri/.cargo/debug-config.toml (1)
27-27: Debug config won’t affect packaged app.ORT_DYLIB_PATHindebug-config.tomlapplies only to development runs (e.g.cargo tauri dev) and isn’t included in the packaged build, so it can’t override the installer-set environment variable at runtime.Likely an incorrect or invalid review comment.
|
c1845d5 |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/detect/src/list.rs (1)
12-21: Fix temporary reference to format! result; use PathBuf Vec (compilation blocker).
&format!(...)creates a temporary; storing it in an array of&stryields a dangling reference and won’t compile. Also, avoid re-allocations by iteratingPathBufs directly.Apply:
- let app_dirs = [ - "/Applications", - &format!("{}/Applications", std::env::var("HOME").unwrap_or_default()), - ]; + let app_dirs = vec![ + PathBuf::from("/Applications"), + PathBuf::from("/System/Applications"), + PathBuf::from(format!("{}/Applications", std::env::var("HOME").unwrap_or_default())), + ]; @@ - for dir in &app_dirs { - let path = PathBuf::from(dir); + for path in app_dirs {
🧹 Nitpick comments (12)
crates/detect/src/mic/windows.rs (1)
5-5: Add a minimal “why” comment for future readers.Clarify that Windows mic detection isn’t implemented yet and the callback is intentionally ignored.
Apply this diff:
impl crate::Observer for Detector { - fn start(&mut self, _f: crate::DetectCallback) {} + // Windows mic detection not implemented yet; callback intentionally unused. + fn start(&mut self, _f: crate::DetectCallback) {} fn stop(&mut self) {} }crates/detect/src/app/windows.rs (1)
5-5: Optional: document intentional no-op.A one-liner “why” comment helps set expectation that Windows app detection isn’t wired yet.
Apply this diff:
impl crate::Observer for Detector { - fn start(&mut self, _f: crate::DetectCallback) {} + // Windows app detection placeholder; callback intentionally unused until implemented. + fn start(&mut self, _f: crate::DetectCallback) {} fn stop(&mut self) {} }crates/detect/src/list.rs (2)
43-44: Minor: use unstable sort for a tiny speed win.Ordering doesn’t require stability here.
- apps.sort_by(|a, b| a.localized_name.cmp(&b.localized_name)); + apps.sort_unstable_by(|a, b| a.localized_name.cmp(&b.localized_name));
63-67: Graceful fallback for app name extraction.If both CFBundleDisplayName and CFBundleName are missing or localized dictionaries, fall back to the bundle’s file name to avoid dropping valid apps.
- let localized_name = plist - .get("CFBundleDisplayName") - .and_then(|v| v.as_string()) - .or_else(|| plist.get("CFBundleName").and_then(|v| v.as_string()))? - .to_string(); + let localized_name = plist + .get("CFBundleDisplayName") + .and_then(|v| v.as_string()) + .or_else(|| plist.get("CFBundleName").and_then(|v| v.as_string())) + .map(|s| s.to_string()) + .or_else(|| app_path.file_stem().and_then(|n| n.to_str()).map(|s| s.to_string()))?;.cargo/config.toml (1)
1-2: Toolchain env assumptions: ensure VS dev environment or remove from repo-global config.Hard-coding linker/CC/CXX/AR to link.exe/cl.exe requires running under a VS Developer Prompt. If that’s already enforced elsewhere (e.g., script), consider moving these to a local/debug config to avoid impacting non-Windows devs/CI.
Options:
- Move to apps/desktop/src-tauri/.cargo/*.toml (scoped) and/or your PS build script.
- Or leave only
[target.x86_64-pc-windows-msvc].linker = 'link.exe'here and drop CC/CXX/AR.Check CI and local builds on:
- Windows without VS env
- macOS/Linux (ensure they ignore these entries)
Also applies to: 7-11
build_with_dlls.ps1 (1)
25-29: Quote paths and surface the effective target dir in logs.Minor robustness and better visibility.
Apply:
-# Clean previous build artifacts -cargo clean --target-dir D:\temp\cargo-target +Write-Host "Using CARGO_TARGET_DIR=$env:CARGO_TARGET_DIR" -ForegroundColor Yellow +cargo clean --target-dir "$env:CARGO_TARGET_DIR" -# Try to build with verbose output -cargo build --verbose 2>&1 | Tee-Object -FilePath build_log.txt +# Try to build with verbose output +cargo build --verbose 2>&1 | Tee-Object -FilePath "$repoRoot\build_log.txt"crates/whisper-local/src/model.rs (1)
40-44: Stubbed transcribe: verify downstream behavior.Returning empty segments can mask real failures. Ensure UI/tests treat “no segments” as “not available” rather than “silence”.
If helpful without adding error handling: add a feature flag (e.g.,
stub-whisper) so tests/UI can branch explicitly.apps/desktop/src-tauri/Cargo.toml (1)
32-33: Gate hypr-whisper-local to Windows to shrink build surface and avoid unused deps on macOS/Linux.
It’s only used under Windows-gated code; move it to a Windows target section.[dependencies] hypr-data = { workspace = true, optional = true } @@ -hypr-whisper-local = { workspace = true } hypr-whisper-local-model = { workspace = true } @@ +[target.'cfg(target_os = "windows")'.dependencies] +hypr-whisper-local = { workspace = true }crates/whisper-local/Cargo.toml (1)
32-32: Gate raw-cpuid to x86_64 (or consider dropping it for std’s detector).
Avoid compiling CPUID code on non-x86 targets; optionally replace usage withis_x86_feature_detected!.-[dependencies] +[dependencies] hypr-audio-utils = { workspace = true } hypr-whisper = { workspace = true } @@ -raw-cpuid = "11.0.1" +[target.'cfg(target_arch = "x86_64")'.dependencies] +raw-cpuid = "11.0.1"If you switch to
is_x86_feature_detected!, you can removeraw-cpuidentirely in a follow-up.apps/desktop/src-tauri/src/lib.rs (1)
381-408: Use the executable directory (not “.”) and prepend an absolute path to PATH. Also fix AVX vs AVX2 log wording.
“.” depends on the current working directory; processes started via installers or updaters often have a different CWD. Using the exe’s directory is robust. The log currently mentions AVX2 although the predicate is “AVX support” (which may be AVX without AVX2). Both points can be addressed without adding new error handling.#[cfg(target_os = "windows")] fn setup_cpu_aware_dll_loading() { - use std::path::PathBuf; - use hypr_whisper_local::cpu_supports_avx; + use std::path::PathBuf; + use hypr_whisper_local::cpu_supports_avx; - // Select the appropriate DLL file in the same directory as the executable - let dll_filename = if cpu_supports_avx() { - tracing::info!("CPU supports AVX2, using optimized whisper DLL"); + // Select the appropriate DLL + let dll_filename = if cpu_supports_avx() { + tracing::info!("CPU supports AVX/AVX2; using optimized whisper DLL"); "whisper_avx.dll" } else { - tracing::info!("CPU does not support AVX2, using scalar whisper DLL"); + tracing::info!("CPU does not support AVX/AVX2; using scalar whisper DLL"); "whisper_scalar.dll" }; - let dll_path = std::path::PathBuf::from("./").join(dll_filename); + // Resolve alongside the executable + let base_dir = std::env::current_exe() + .ok() + .and_then(|p| p.parent().map(|p| p.to_owned())) + .unwrap_or_else(|| PathBuf::from(".")); + let dll_path = base_dir.join(dll_filename); - // Add the current directory to PATH if the DLL exists + // Prepend exe dir to PATH if the DLL exists if dll_path.exists() { let current_path = std::env::var("PATH").unwrap_or_default(); - let new_path = format!(".;{}", current_path); + let new_path = format!("{};{}", base_dir.display(), current_path); std::env::set_var("PATH", new_path); - tracing::info!("Updated PATH to include current directory for DLL: {}", dll_filename); + tracing::info!("Updated PATH to include {:?} for DLL: {}", base_dir, dll_filename); } else { tracing::warn!("DLL file not found: {} - using system defaults", dll_path.display()); } }Verification:
- Please confirm the optimized DLL truly requires AVX2 or only AVX. If it requires AVX2, switch the predicate to an explicit AVX2 check in
hypr_whisper_local::cpu_supports_avx()or rename the DLL to avoid confusion. This may be the source of sporadic EXCEPTION_ILLEGAL_INSTRUCTION on some machines.apps/desktop/src/locales/en/messages.ts (2)
1-1: Minor wording nit“Email separated by commas” → “Email addresses separated by commas”.
-"KSl1BE":["Email separated by commas"] +"KSl1BE":["Email addresses separated by commas"]
1-1: Optional: Consolidate duplicate “Today” message ID
The string “Today” is defined twice (keys"Today"and"ecUA8p"). Consider reusing a single ID to remove the duplicate and reduce translation overhead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (13)
Cargo.lockis excluded by!**/*.lockapps/desktop/src-tauri/dlls/ggml-base.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml-base_avx.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml-base_scalar.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml-cpu.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml-cpu_avx.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml-cpu_scalar.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml_avx.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/ggml_scalar.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/whisper.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/whisper_avx.dllis excluded by!**/*.dllapps/desktop/src-tauri/dlls/whisper_scalar.dllis excluded by!**/*.dll
📒 Files selected for processing (15)
.cargo/config.toml(1 hunks).claude/settings.local.json(1 hunks)apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/commands.rs(1 hunks)apps/desktop/src-tauri/src/lib.rs(3 hunks)apps/desktop/src/locales/en/messages.ts(1 hunks)apps/desktop/src/locales/ko/messages.ts(1 hunks)build_with_dlls.ps1(1 hunks)crates/detect/src/app/windows.rs(1 hunks)crates/detect/src/list.rs(2 hunks)crates/detect/src/mic/windows.rs(1 hunks)crates/whisper-local/Cargo.toml(2 hunks)crates/whisper-local/src/error.rs(1 hunks)crates/whisper-local/src/lib.rs(2 hunks)crates/whisper-local/src/model.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/locales/ko/messages.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/detect/src/mic/windows.rscrates/whisper-local/src/lib.rscrates/whisper-local/src/error.rsapps/desktop/src/locales/en/messages.tsapps/desktop/src-tauri/src/commands.rscrates/detect/src/app/windows.rsapps/desktop/src-tauri/src/lib.rscrates/detect/src/list.rscrates/whisper-local/src/model.rs
🧬 Code graph analysis (7)
crates/detect/src/mic/windows.rs (4)
crates/detect/src/app/windows.rs (1)
start(5-5)crates/detect/src/lib.rs (2)
start(29-29)start(51-53)crates/detect/src/mic/macos.rs (1)
start(96-330)crates/detect/src/mic/mod.rs (1)
start(17-19)
crates/whisper-local/src/lib.rs (2)
plugins/local-stt/src/commands.rs (1)
list_ggml_backends(17-21)plugins/local-stt/src/ext.rs (2)
list_ggml_backends(22-22)list_ggml_backends(76-78)
apps/desktop/src/locales/en/messages.ts (1)
apps/desktop/src/locales/ko/messages.ts (1)
messages(1-1)
apps/desktop/src-tauri/src/commands.rs (1)
crates/whisper-local/src/lib.rs (1)
cpu_supports_avx(78-102)
crates/detect/src/app/windows.rs (5)
crates/detect/src/mic/windows.rs (1)
start(5-5)crates/detect/src/lib.rs (2)
start(29-29)start(51-53)crates/detect/src/app/macos.rs (1)
start(26-76)crates/detect/src/app/mod.rs (1)
start(17-19)crates/detect/src/utils.rs (1)
start(37-54)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src-tauri/src/commands.rs (1)
check_cpu_avx_support(86-88)crates/whisper-local/src/lib.rs (1)
cpu_supports_avx(78-102)
crates/whisper-local/src/model.rs (1)
crates/whisper-local/src/stream.rs (4)
transcribe(47-47)transcribe(57-64)transcribe(68-68)transcribe(76-83)
🔇 Additional comments (12)
crates/detect/src/mic/windows.rs (1)
5-5: LGTM: unused callback correctly marked.The
_frename cleanly silences the unused-parameter warning and aligns with the guidelines.crates/detect/src/app/windows.rs (1)
5-5: LGTM: consistent with trait and suppresses warning.Parameter underscore is idiomatic; matches macOS/windows parity choice to keep stubbed start.
crates/detect/src/list.rs (1)
47-50: Confirm downstream handling of non-macOS stub.Returning an empty list on non-macOS is fine if callers handle “no apps found” gracefully. Please verify no UI/logic regressions on Windows/Linux given this PR’s Windows focus.
.claude/settings.local.json (1)
1-12: Scope creep vs. PR goal (Windows DLL bundling).This file is unrelated to DLL bundling and may distract review/audit. Please split into a separate PR or justify why it must ship with the bundling changes.
crates/whisper-local/src/lib.rs (1)
22-26: Confirm empty‐list handling in downstream consumers
The stub now returns an empty Vec, but callers in
- plugins/local-stt/src/commands.rs:20
- plugins/local-stt/src/ext.rs:77
- plugins/tray/src/ext.rs:137
do not explicitly guard on a non-empty list. Verify UI/feature flows gracefully handle zero backends and don’t assume any are available.crates/whisper-local/src/error.rs (1)
7-8: Ignore API break concern: no LocalWhisperError or external WhisperError references found; change is localized and safe.Likely an incorrect or invalid review comment.
crates/whisper-local/src/model.rs (1)
1-2: Acknowledged stub.Fine as a temporary measure; keep the “why” noted in the TODO up to date with re-enable criteria.
apps/desktop/src-tauri/src/commands.rs (1)
83-88: Windows-only AVX check command looks good and is correctly gated.
No unused imports; matches Specta wiring on Windows.crates/whisper-local/Cargo.toml (1)
9-16: Keeping feature flags as no-ops for compatibility is fine.
This preserves downstream conditional builds without pulling whisper-rs.apps/desktop/src-tauri/src/lib.rs (2)
18-21: Right place to run DLL setup (before any library init).
This minimizes early-load surprises from the Windows loader.
360-379: Specta builder refactor is sound; Windows command is conditionally registered.
Keeps non-Windows builds clean and avoids unused exports.apps/desktop/src/locales/en/messages.ts (1)
1-1: LGTM on catalog wiringImport typing, JSON payload shape, and cast to Messages look correct for Lingui’s runtime catalog.
resolves #66
Todo
- [x] Installer Hooks - Added NSIS installer script (windows/hooks.nsh)