feat: engine selection #1469
feat: engine selection #1469
Conversation
WalkthroughThe changes update the GPU engine management and related localization across the application. New entries for GPU engine configuration have been added to all locale JSON files. The core configuration and GPU mining modules now support a new field along with getter/setter methods. Function names and parameter types have been updated across commands, events, and adapters. New React components and hooks have been introduced for GPU engine selection, and the store and type definitions have been restructured accordingly to support the new engine functionality and refined device management. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as GpuEngine Component
participant Store as Mining Store
participant API as Tauri API (invoke)
participant Miner as GPU Miner
UI->>Store: handleEngineChange(selectedEngine)
Store->>API: set_selected_engine(selectedEngine)
API->>Miner: Update selected engine configuration
Miner-->>API: Acknowledge update
API-->>Store: Confirm change
Store-->>UI: Refresh engine state
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (23)
src-tauri/src/gpu_status_file.rs (2)
64-69:GpuStatusFile::load(...)method
The usage ofserde_json::from_readeris straightforward. Consider adding context in case of deserialization errors for easier debugging, for example:- let config = serde_json::from_reader(reader)?; + let config = serde_json::from_reader(reader) + .map_err(|e| anyhow!("Failed to parse GPU status json: {}", e))?;
71-81:GpuStatusFile::save(...)method
Implementation for writing JSON content is appropriate. Ensure the parent directory exists before writing. Otherwise, this could fail if the directory is missing.src-tauri/src/gpu_miner_adapter.rs (2)
24-26: Typo in the fieldcurent_selected_engine
For naming consistency, consider renaming tocurrent_selected_engine.- pub(crate) curent_selected_engine: EngineType, + pub(crate) current_selected_engine: EngineType,Also applies to: 64-64, 66-66
92-116:GpuMinerAdapter::set_mode(...)
ModesEcoandLudicrousoverride actual device settings with static thread counts. If you intended to respect each device’s specific maximum, you might consider referencingv.status.max_grid_size. Otherwise, this is acceptable for consistent system-wide behavior.- max_gpu_threads: 2, + max_gpu_threads: v.status.max_grid_size.min(2),src-tauri/src/gpu_miner.rs (3)
51-75: NewEngineTypeenum
The string conversion methods are clear and cover the valid variants. Consider supporting lowercase inputs (e.g., "cuda") if user input is loosely formatted.
247-279:get_available_gpu_engines(...)
Splitting file names on underscores is clever but could fail if file naming changes or contains multiple underscores. If so, you might handle unexpected patterns more gracefully and log a warning.
308-335:toggle_device_exclusion(...)
Logic updates a single device’sis_excludedflag, then persists changes. If multiple processes might modify the same file concurrently, consider adding file-level locks to prevent race conditions.src-tauri/src/main.rs (2)
681-685: Strengthen error handling for GPU detection
Currently, detection failures are only logged. Consider returning specific error messages or fallback behavior if an unsupported engine or misconfiguration is discovered.
1356-1356: Add unit tests for selecting GPU engine
set_selected_engineis vital for user control. If not already in place, add tests to confirm correct handling of invalid or missing engine types.src/containers/floating/Settings/sections/experimental/ExperimentalSettings.tsx (1)
25-25: Check UI states when no GPU is available
Consider conditionally rendering<GpuEngine />only if a GPU device is detected, or show a fallback message if none is found, to avoid user confusion.src/App/AppEffects.tsx (1)
25-25: Possible logging or error-handling
If GPU engine updates fail or produce errors, consider logging or gracefully handling them in this hook to help with debugging.src/containers/floating/Settings/sections/mining/GpuDevices.tsx (1)
40-40: Remove debug console.log statement.Debug console.log statement should be removed before merging.
- console.log('gpuDevices', gpuDevices);🧰 Tools
🪛 GitHub Check: Run linters
[warning] 40-40:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEndsrc/store/useMiningMetricsStore.ts (1)
60-61: Replace console.log with console.info for consistency.Use console.info for logging state changes to maintain consistency with other logging in the codebase.
- console.log('Setting GPU mining enabled'); + console.info('Setting GPU mining enabled'); - console.log('Setting GPU mining disabled'); + console.info('Setting GPU mining disabled');Also applies to: 66-67
🧰 Tools
🪛 GitHub Check: Run linters
[warning] 60-60:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEndsrc/store/useMiningStore.ts (2)
114-114: Consolidate logging statements.Remove redundant logging of the engine setting.
- console.log('Setting engine: ', engine); try { console.info('Setting engine: ', engine);Also applies to: 117-117
113-127: Consider improving error recovery in setEngine.The error handling could be improved by:
- Adding specific error types/messages for different failure scenarios
- Considering retry logic for transient failures
- Notifying the user about the fallback to the previous engine
setEngine: async (engine) => { - console.log('Setting engine: ', engine); const current_engine = useMiningStore.getState().engine; try { console.info('Setting engine: ', engine); await invoke('set_selected_engine', { selectedEngine: engine }); set({ engine }); await useMiningStore.getState().restartMining(); } catch (e) { const appStateStore = useAppStateStore.getState(); - console.error('Could not set engine: ', e); + const errorMessage = `Failed to set engine to ${engine}: ${e}`; + console.error(errorMessage); + appStateStore.setError(errorMessage); + appStateStore.setNotification({ + type: 'warning', + message: `Falling back to previous engine: ${current_engine || 'default'}` + }); set({ engine: current_engine || undefined }); } },src-tauri/src/commands.rs (1)
1816-1846: Consider enhancing error handling for invalid engine types.While the implementation is solid, consider these improvements:
- Add validation for the engine type string before conversion
- Include specific error messages for invalid engine types
- Consider logging the previous engine type for debugging
pub async fn set_selected_engine( selected_engine: &str, state: tauri::State<'_, UniverseAppState>, app: tauri::AppHandle, ) -> Result<(), String> { info!(target: LOG_TARGET, "set_selected_engine called with engine: {:?}", selected_engine); let timer = Instant::now(); info!(target: LOG_TARGET, "Setting selected engine"); + // Log previous engine type for debugging + if let Ok(current_engine) = state.gpu_miner.read().await.get_selected_engine() { + info!(target: LOG_TARGET, "Current engine: {:?}", current_engine); + } + + // Validate engine type string + if selected_engine.trim().is_empty() { + return Err("Engine type cannot be empty".to_string()); + } + let engine_type = EngineType::from_string(selected_engine).map_err(|e| e.to_string())?; info!(target: LOG_TARGET, "Selected engine set to {:?}", engine_type);public/locales/en/settings.json (1)
9-9: Consider improving the label text for better readability.The current text could be more descriptive and follow proper title case.
- "change-gpu-engine": "Change Gpu Engine", + "change-gpu-engine": "Change GPU Engine",public/locales/hi/settings.json (1)
9-9: Fix capitalization in "Gpu".For consistency with other GPU-related strings in the file (e.g., "GPU माइनिंग"), use "GPU" instead of "Gpu".
- "change-gpu-engine": "Change Gpu Engine", + "change-gpu-engine": "Change GPU Engine",public/locales/tr/settings.json (1)
9-9: Fix capitalization in "Gpu".For consistency with other GPU-related strings in the file (e.g., "GPU Madenciliği"), use "GPU" instead of "Gpu".
- "change-gpu-engine": "Change Gpu Engine", + "change-gpu-engine": "Change GPU Engine",public/locales/af/settings.json (1)
9-9: Fix capitalization in "Gpu".For consistency with other GPU-related strings in the file (e.g., "GPU Mynbou"), use "GPU" instead of "Gpu".
- "change-gpu-engine": "Change Gpu Engine", + "change-gpu-engine": "Change GPU Engine",public/locales/id/settings.json (1)
9-9: Fix capitalization in "Gpu".For consistency with other GPU-related strings in the file (e.g., "Penambangan GPU"), use "GPU" instead of "Gpu".
- "change-gpu-engine": "Change Gpu Engine", + "change-gpu-engine": "Change GPU Engine",public/locales/ru/settings.json (1)
9-9: Fix capitalization in "Gpu".For consistency with other GPU-related strings in the file (e.g., "Майнинг на GPU"), use "GPU" instead of "Gpu".
- "change-gpu-engine": "Change Gpu Engine", + "change-gpu-engine": "Change GPU Engine",public/locales/pl/settings.json (1)
9-9: Consider implementing a comprehensive localization strategy.The GPU engine text is currently in English across all locale files. For better user experience:
- Ensure all new strings are properly translated for each supported language.
- Use consistent capitalization for acronyms (e.g., "GPU" instead of "Gpu").
- Consider using a translation management system (TMS) to handle translations systematically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
public/locales/af/settings.json(1 hunks)public/locales/cn/settings.json(1 hunks)public/locales/de/settings.json(1 hunks)public/locales/en/settings.json(1 hunks)public/locales/fr/settings.json(1 hunks)public/locales/hi/settings.json(1 hunks)public/locales/id/settings.json(1 hunks)public/locales/ja/settings.json(1 hunks)public/locales/ko/settings.json(1 hunks)public/locales/pl/settings.json(1 hunks)public/locales/ru/settings.json(1 hunks)public/locales/tr/settings.json(1 hunks)src-tauri/src/app_config.rs(10 hunks)src-tauri/src/commands.rs(4 hunks)src-tauri/src/events.rs(2 hunks)src-tauri/src/gpu_miner.rs(7 hunks)src-tauri/src/gpu_miner_adapter.rs(7 hunks)src-tauri/src/gpu_status_file.rs(1 hunks)src-tauri/src/main.rs(3 hunks)src/App/AppEffects.tsx(2 hunks)src/containers/floating/Settings/sections/experimental/ExperimentalSettings.tsx(2 hunks)src/containers/floating/Settings/sections/experimental/GpuEngine.tsx(1 hunks)src/containers/floating/Settings/sections/mining/GpuDevices.tsx(3 hunks)src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx(1 hunks)src/hooks/app/useListenForGpuEngines.ts(1 hunks)src/hooks/app/useTauriEventsListener.ts(1 hunks)src/store/miningStoreActions.ts(0 hunks)src/store/useAppConfigStore.ts(1 hunks)src/store/useMiningMetricsStore.ts(3 hunks)src/store/useMiningStore.ts(3 hunks)src/types/app-status.ts(1 hunks)src/types/invoke.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/store/miningStoreActions.ts
✅ Files skipped from review due to trivial changes (1)
- public/locales/ko/settings.json
🧰 Additional context used
🪛 GitHub Check: Run linters
src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx
[warning] 31-31:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 35-35:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
src/store/useAppConfigStore.ts
[warning] 157-157:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 167-167:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 175-175:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
src/hooks/app/useListenForGpuEngines.ts
[warning] 37-37:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 45-45:
React Hook useEffect has a missing dependency: 'setGpuDevices'. Either include it or remove the dependency array
src/containers/floating/Settings/sections/mining/GpuDevices.tsx
[warning] 40-40:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
src/store/useMiningMetricsStore.ts
[warning] 60-60:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 66-66:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tauri-build
- GitHub Check: cargo checks (fmt, clippy, check)
🔇 Additional comments (39)
src-tauri/src/gpu_status_file.rs (9)
1-6: Imports look good
No potential issues observed with these imports.
11-16:GpuStatusstruct
Fields and serde usage look straightforward.
18-22:GpuSettingsstruct
Fields are appropriately defined for toggling GPU usage states.
24-31:Defaultimplementation forGpuSettings
Implementation is clear and consistent with typical usage.
33-39:GpuDevicestruct
Combines both status and settings into a single entity—design is concise.
41-44:GpuStatusFilestruct
A convenient container for a map of device name to device metadata.
46-52:DefaultforGpuStatusFile
Appropriate default returning an empty map.
54-62:GpuStatusFile::new(...)
Resolves settings from file if available, otherwise falls back to detected devices. Logic is sensible.
83-115:resolve_settings_for_detected_devices(...)
Merges newly detected devices with previously stored settings, which is a solid approach. The fallback strategy is aligned with typical user expectations.src-tauri/src/gpu_miner_adapter.rs (2)
71-90:GpuMinerAdapter::new(...)
Storing devices in aHashMap<String, GpuDevice>is a good improvement for random access by device name. The approach for creatinggpu_grid_sizefrom each device’smax_grid_sizeis consistent.
150-156: Engine argument and status-file path
Adding the--gpu-status-fileand--enginearguments is consistent with the new engine-based approach to GPU management. The usage of cloned path strings is clear.Also applies to: 185-186, 191-192
src-tauri/src/gpu_miner.rs (5)
25-26: Imports and usage ofHashMapandtauri::{AppHandle, Emitter}
No issues found here; usage is aligned with the rest of the codebase.Also applies to: 32-32
80-82: Fields added toGpuMiner
Switch toHashMap<String, GpuDevice>and storingcurent_selected_engineenable better device referencing. Spelling of “curent” is noted above.
164-245:detect(...)logic
• Adds the engine to the path naming, ensuring each engine’s status file is isolated.
• Emitting events for available engines and devices is a neat approach for async UI updates.
337-369:set_selected_engine(...)
Appropriately updates the adapter’s engine and reloads the mapped devices. Emitting the new device list ensures the UI stays in sync.
371-375:get_gpu_engines_statuses_path(...)
Naming is explicit, returning a consistent path.src-tauri/src/main.rs (2)
111-111: Confirm usage of the new module
It’s great to see thegpu_status_filemodule introduced. Ensure it is fully integrated and leveraged (e.g., reading/writing GPU status) in the codebase to avoid dead code.
1351-1351: Command looks good
The newtoggle_device_exclusioncommand is well-placed. Continue ensuring consistent usage and proper testing in the frontend.src/containers/floating/Settings/sections/experimental/ExperimentalSettings.tsx (1)
10-10: New GpuEngine import is clear
No concerns. This neatly organizes the GPU engine UI into its own component.src/App/AppEffects.tsx (1)
11-11: Hook import is appropriate
useListenForGpuEnginesaligns with other app-level hooks. No issues found in naming or structure.src/hooks/app/useListenForGpuEngines.ts (1)
7-14: LGTM! Well-structured interfaces.The interfaces are well-defined and properly typed for the event payloads.
src/containers/floating/Settings/sections/experimental/GpuEngine.tsx (3)
18-22: LGTM! Clean state management.Good use of selector pattern to extract only needed state from the store.
24-29: LGTM! Optimized options computation.Good use of useMemo to optimize the options array creation.
31-36: Consider error handling for setEngine.The async operation might fail but errors are not handled.
Consider adding try-catch:
const handleEngineChange = useCallback( async (value: string) => { + try { await setEngine(value); + } catch (error) { + console.error('Failed to set engine:', error); + // Consider showing user feedback + } }, [setEngine] );src-tauri/src/events.rs (2)
43-47: LGTM! Well-defined event struct.The
DetectedAvailableGpuEnginesstruct is properly defined with appropriate fields and derives.
49-52: LGTM! Clean device event struct.The
DetectedDevicesstruct is well-structured and properly derives Debug, Serialize, and Clone.src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx (1)
23-28: LGTM! Clear availability check.The GPU mining availability check is well-implemented with proper null checks and array validation.
src/hooks/app/useTauriEventsListener.ts (1)
4-4:❓ Verification inconclusive
Verify the removal of GPU devices update handling.
The removal of the
GpuDevicesUpdateevent type suggests that GPU device updates are now handled elsewhere. Please ensure that:
- GPU device updates are properly handled by the new engine selection system
- No other components rely on receiving GPU device updates through this event listener
Also applies to: 10-45
🏁 Script executed:
#!/bin/bash # Search for any remaining references to GpuDevicesUpdate rg "GpuDevicesUpdate" -A 5Length of output: 1054
Action: Verify GPU Device Update Handling Refactoring
The removal of GPU device update handling from
useTauriEventsListener.tshas been confirmed. However, our search shows that the backend still emitsGpuDevicesUpdateevents insrc-tauri/src/events_emitter.rs. Please ensure that:
- These backend-emitted GPU device updates are properly integrated with the new engine selection system.
- No UI or other components are inadvertently relying on the legacy
GpuDevicesUpdateevents.src/containers/floating/Settings/sections/mining/GpuDevices.tsx (1)
31-38: LGTM! Improved device management logic.The changes improve the device management by:
- Using device_index as a stable key for React components
- Using device_name for display
- Simplifying device exclusion logic with toggleDeviceExclusion
Also applies to: 56-71
src/store/useMiningMetricsStore.ts (1)
55-69: LGTM! Improved GPU mining state management.The changes correctly manage GPU mining state by:
- Enabling GPU mining when available devices are not excluded
- Disabling GPU mining when all devices are excluded
🧰 Tools
🪛 GitHub Check: Run linters
[warning] 60-60:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 66-66:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEndsrc/store/useMiningStore.ts (1)
91-112: LGTM! Improved device exclusion logic.The device exclusion logic correctly:
- Updates device exclusion state
- Handles GPU mining enabled state
- Updates the store with the modified devices
src/types/invoke.ts (1)
23-23: LGTM! The new function signatures enhance GPU management capabilities.The changes improve the API by:
- Adding
set_selected_enginefor GPU engine selection- Replacing the old
set_excluded_gpu_deviceswith a more flexibletoggle_device_exclusionAlso applies to: 81-84
src/types/app-status.ts (1)
181-197: LGTM! Well-structured interfaces for GPU management.The new interfaces provide a clear separation of concerns:
GpuStatus: Performance parametersGpuSettings: Device stateGpuDevice: Complete device representationsrc/store/useAppConfigStore.ts (1)
174-179: LGTM! Improved device exclusion handling.The code automatically resets device exclusions when enabling GPU mining, which improves user experience by ensuring all devices are available for mining.
🧰 Tools
🪛 GitHub Check: Run linters
[warning] 175-175:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEndsrc-tauri/src/app_config.rs (3)
123-124: LGTM! Well-integrated GPU engine configuration.The GPU engine configuration is properly added to both structs with appropriate serialization support.
Also applies to: 292-292
505-510: LGTM! Robust engine type handling.The
gpu_enginemethod safely handles invalid engine types by defaulting to OpenCL.
921-923: LGTM! Sensible default engine choice.OpenCL is a good default choice as it's widely supported across different GPU vendors.
src-tauri/src/commands.rs (2)
1015-1032: LGTM! Well-structured device exclusion toggle implementation.The implementation includes proper error handling, logging, and state management.
33-35:❓ Verification inconclusive
Verify the impact of GpuStatus type change.
The hardware status type has been changed from
PublicDevicePropertiestoGpuStatus. This change might affect consumers of theGpuMinerMetricsstruct.Also applies to: 96-96
🏁 Script executed:
#!/bin/bash # Description: Find all usages of GpuMinerMetrics to verify compatibility with the new type # Search for struct usage rg -A 5 "GpuMinerMetrics" # Search for hardware field access rg "\.hardware"Length of output: 324
Verify the updated hardware field type in GpuMinerMetrics
The
GpuMinerMetricsstruct now defines itshardwarefield asVec<GpuStatus>(see src-tauri/src/commands.rs, lines 33–35 and line 96). This change replaces the formerPublicDevicePropertiestype. Although our search confirms that the struct is updated as intended—and there were no additional usages of the.hardwareproperty detected—please manually verify that any consumers relying on this field are fully compatible with the newGpuStatustype.
src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx
Outdated
Show resolved
Hide resolved
src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src-tauri/src/commands.rs (2)
1848-1848: Fix incorrect warning message.The warning message references "proceed_with_update" instead of "set_selected_engine".
- warn!(target: LOG_TARGET, "proceed_with_update took too long: {:?}", timer.elapsed()); + warn!(target: LOG_TARGET, "set_selected_engine took too long: {:?}", timer.elapsed());
1816-1852: Consider implementing atomic updates for state consistency.The function updates two separate states (GPU miner and config). If the second update fails after the first succeeds, the system could end up in an inconsistent state. Consider implementing a transaction-like pattern or reverting the first change if the second fails.
Example approach:
pub async fn set_selected_engine( selected_engine: &str, state: tauri::State<'_, UniverseAppState>, app: tauri::AppHandle, ) -> Result<(), String> { info!(target: LOG_TARGET, "set_selected_engine called with engine: {:?}", selected_engine); let timer = Instant::now(); let engine_type = EngineType::from_string(selected_engine).map_err(|e| e.to_string())?; let config = app.path().app_config_dir().expect("Could not get config dir"); // First update config (less likely to fail) state .config .write() .await .set_gpu_engine(selected_engine) .await .map_err(|e| e.to_string())?; // Then update GPU miner state if let Err(e) = state .gpu_miner .write() .await .set_selected_engine(engine_type, config, app) .await { // Revert config change on failure if let Err(revert_err) = state .config .write() .await .set_gpu_engine(/* previous engine value */) .await { error!(target: LOG_TARGET, "Failed to revert config change: {:?}", revert_err); } return Err(e.to_string()); } if timer.elapsed() > MAX_ACCEPTABLE_COMMAND_TIME { warn!(target: LOG_TARGET, "set_selected_engine took too long: {:?}", timer.elapsed()); } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src-tauri/src/app_config.rs(10 hunks)src-tauri/src/commands.rs(4 hunks)src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx(1 hunks)src/hooks/app/useListenForGpuEngines.ts(1 hunks)src/store/useAppConfigStore.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/containers/floating/Settings/sections/mining/GpuMiningMarkup.tsx
- src/hooks/app/useListenForGpuEngines.ts
- src-tauri/src/app_config.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tauri-build
- GitHub Check: cargo checks (fmt, clippy, check)
🔇 Additional comments (3)
src-tauri/src/commands.rs (2)
33-35: LGTM! The import changes and struct modifications align with the GPU engine management updates.The changes to imports and the
GpuMinerMetricsstruct's hardware field type reflect the architectural shift in how GPU status information is handled.Also applies to: 94-99
1015-1032: LGTM! Well-structured command implementation with proper error handling.The
toggle_device_exclusioncommand follows Rust best practices with:
- Proper async/await usage
- Good error handling with
inspect_err- Clear logging of errors
src/store/useAppConfigStore.ts (1)
160-161: LGTM!Good variable extraction that improves code readability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/containers/floating/Settings/sections/experimental/GpuEngine.tsx (2)
16-20: Add TypeScript types for store values.Consider adding explicit types for
availableEngines,selectedEngine, andsetEngineto improve type safety and code maintainability.- const availableEngines = useMiningStore((state) => state.availableEngines); - const selectedEngine = useMiningStore((state) => state.engine); - const setEngine = useMiningStore((state) => state.setEngine); + const availableEngines: string[] = useMiningStore((state) => state.availableEngines); + const selectedEngine: string = useMiningStore((state) => state.engine); + const setEngine: (engine: string) => Promise<void> = useMiningStore((state) => state.setEngine);
49-49: Extract hardcoded height value to constants.Consider moving the hardcoded height value to a constants file for better maintainability and consistency across the application.
+const SELECT_HEIGHT = 36; + export default function GpuEngine() { // ... - forceHeight={36} + forceHeight={SELECT_HEIGHT}src/store/useMiningMetricsStore.ts (1)
21-21:❓ Verification inconclusive
Verify the architectural decision to separate CPU and GPU device management.
The removal of
cpu_devicesand type change forgpu_devicessuggests an architectural shift. Please confirm if this is intentional and aligns with the broader application architecture.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to cpu_devices rg "cpu_devices" --type tsLength of output: 26
Verify the Separation of CPU and GPU Device Management
The update where
gpu_devicesnow uses theGpuDevice[]type while thecpu_devicesproperty is removed suggests an intentional architectural change to segregate CPU and GPU handling. Although the preliminary check usingrg "cpu_devices"returned no references, please manually verify that all necessary adjustments related to CPU device management have been made and that this change aligns with the overall design strategy.
- Verify that no modules or components depend on
cpu_devices.- Confirm that the new
GpuDevicetype fully meets the requirements previously addressed byPublicDeviceParameters[].- Ensure that the architectural separation is consistently applied across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/containers/floating/Settings/sections/experimental/GpuEngine.tsx(1 hunks)src/containers/floating/Settings/sections/mining/GpuDevices.tsx(3 hunks)src/store/useMiningMetricsStore.ts(3 hunks)src/store/useMiningStore.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/containers/floating/Settings/sections/mining/GpuDevices.tsx
- src/store/useMiningStore.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cargo checks (fmt, clippy, check)
- GitHub Check: tauri-build
🔇 Additional comments (4)
src/containers/floating/Settings/sections/experimental/GpuEngine.tsx (2)
1-14: LGTM! Clean imports and styling setup.The imports are well-organized, using absolute paths with the
@appalias, and the styled component provides a proper flex container layout.
54-54:✅ Verification successful
Verify translation key naming convention.
The translation key
'gpu-device-no-found'seems inconsistent. Should it be'gpu-device-not-found'?
🏁 Script executed:
#!/bin/bash # Description: Check translation key in locale files rg -A 1 "gpu-device-no-found" public/locales/Length of output: 1973
Translation key naming verified – no changes required.
The translation key"gpu-device-no-found"is used consistently across all locale files (e.g., inpublic/locales/en/settings.jsonand others). Please keep the key as-is.src/store/useMiningMetricsStore.ts (2)
1-1: LGTM! Import changes align with the new GPU device management.The imports correctly reflect the transition to the
GpuDevicetype and add the necessary store for GPU mining state management.Also applies to: 6-6
10-10:❓ Verification inconclusive
Verify all consumers of setGpuDevices have been updated.
The parameter type change from
PublicDeviceParameters[]toGpuDevice[]is a breaking change.
🏁 Script executed:
#!/bin/bash # Search for all usages of setGpuDevices to ensure they've been updated rg -A 3 "setGpuDevices"Length of output: 1774
Action Required: Confirm All Consumers Align with the New GpuDevice[] Type
We verified that the updated
setGpuDevicesis used in the following locations:
- src/hooks/app/useListenForGpuEngines.ts – The call
setGpuDevices(payload.devices);should be checked to ensure thatpayload.devicesconforms to theGpuDevice[]type.- src/store/useMiningStore.ts – The invocation
useMiningMetricsStore.getState().setGpuDevices(updatedDevices);should similarly pass a value matching the new type.Since these changes represent a breaking change, please verify that all consumers are updated accordingly and that any downstream expectations about the data structure are aligned with
GpuDevice[].
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src-tauri/src/events_emitter.rs (1)
106-118: 💡 Verification agent🧩 Analysis chain
Verify the need for #[allow(dead_code)].
The method appears to be part of the GPU engine selection feature but is marked as unused. Consider removing the attribute if the method will be used in this PR.
🏁 Script executed:
#!/bin/bash # Search for calls to emit_gpu_devices_update rg "emit_gpu_devices_update" -A 2Length of output: 445
Action: Remove the unnecessary
#[allow(dead_code)]attributeThe
emit_gpu_devices_updatemethod is actively used insrc-tauri/src/events_manager.rs(as confirmed by the grep results). Since the method is not dead code, please remove the#[allow(dead_code)]attribute to reflect its proper usage.src-tauri/src/gpu_miner.rs (1)
164-244: 🛠️ Refactor suggestionImprove error handling and reduce code duplication.
- The error handling in the
detectmethod could be more descriptive.- The GPU status file path construction is duplicated across methods.
pub async fn detect( &mut self, app: AppHandle, config_dir: PathBuf, engine: EngineType, ) -> Result<(), anyhow::Error> { info!(target: LOG_TARGET, "Verify if gpu miner can work on the system"); self.curent_selected_engine = engine; + // Extract path construction to a method + let gpu_status_file_path = self.get_gpu_status_file_path(&config_dir)?; + let config_file = config_dir .join("gpuminer") .join("config.json") .to_string_lossy() .to_string(); // ... rest of the code ... match output.status.code() { Some(0) => { // ... success case ... } _ => { self.is_available = false; - Err(anyhow::anyhow!( - "Non-zero exit code: {:?}", - output.status.code() - )) + Err(anyhow::anyhow!( + "GPU detection failed with exit code: {:?}. Check the GPU miner logs for details.", + output.status.code() + )) } } } +impl GpuMiner { + fn get_gpu_status_file_path(&self, config_dir: &Path) -> Result<PathBuf, anyhow::Error> { + Ok(get_gpu_engines_statuses_path(config_dir).join(format!( + "{}_gpu_status.json", + self.curent_selected_engine.to_string() + ))) + } +}
🧹 Nitpick comments (8)
src-tauri/src/gpu_status_file.rs (2)
77-82: Consider enhancing error handling in the load method.The current implementation could benefit from more specific error handling and user-friendly error messages.
pub fn load(path: &PathBuf) -> Result<Self, anyhow::Error> { - let file = File::open(path)?; - let reader = BufReader::new(file); - let config = serde_json::from_reader(reader)?; - Ok(config) + let file = File::open(path) + .map_err(|e| anyhow!("Failed to open GPU status file '{}': {}", path.display(), e))?; + let reader = BufReader::new(file); + let config = serde_json::from_reader(reader) + .map_err(|e| anyhow!("Failed to parse GPU status file '{}': {}", path.display(), e))?; + Ok(config) }
84-94: Consider security implications of debug logging.The debug log might expose sensitive device information. Consider masking or limiting the logged information.
- debug!( - "Updating gpu status file with {:?}, at path: {:?}", - new_content, path - ); + debug!( + "Updating gpu status file at path: {:?} with {} devices", + path, + new_content.gpu_devices.len() + );src-tauri/src/gpu_miner_adapter.rs (2)
63-66: Fix typo in field name.The field name 'curent_selected_engine' has a typo (missing 'r').
- pub(crate) curent_selected_engine: EngineType, + pub(crate) current_selected_engine: EngineType,
149-155: Consider using PathBuf methods for path manipulation.The current path manipulation could be simplified using PathBuf methods.
- let gpu_engine_statuses = config_dir - .join("gpuminer") - .join("engine_statuses") - .clone() - .to_string_lossy() - .to_string(); + let gpu_engine_statuses = config_dir + .join("gpuminer") + .join("engine_statuses") + .to_string_lossy() + .into_owned();src-tauri/src/gpu_miner.rs (4)
51-75: Consider using string constants for engine type names.The string literals for engine types are inconsistent between the enum variants and their string representations. Consider using constants to maintain consistency and prevent typos.
#[derive(Debug, PartialEq, Clone)] pub enum EngineType { Cuda, OpenCL, Metal, } +const CUDA_ENGINE: &str = "CUDA"; +const OPENCL_ENGINE: &str = "OpenCL"; +const METAL_ENGINE: &str = "Metal"; + impl EngineType { pub fn to_string(&self) -> String { match self { - EngineType::Cuda => "CUDA".to_string(), - EngineType::OpenCL => "OpenCL".to_string(), - EngineType::Metal => "Metal".to_string(), + EngineType::Cuda => CUDA_ENGINE.to_string(), + EngineType::OpenCL => OPENCL_ENGINE.to_string(), + EngineType::Metal => METAL_ENGINE.to_string(), } } pub fn from_string(engine_type: &str) -> Result<EngineType, anyhow::Error> { match engine_type { - "CUDA" => Ok(EngineType::Cuda), - "OpenCL" => Ok(EngineType::OpenCL), - "Metal" => Ok(EngineType::Metal), + CUDA_ENGINE => Ok(EngineType::Cuda), + OPENCL_ENGINE => Ok(EngineType::OpenCL), + METAL_ENGINE => Ok(EngineType::Metal), _ => Err(anyhow::anyhow!("Invalid engine type")), } } }
77-82: Fix typo in field name.The field name
curent_selected_enginehas a typo (missing 'r').pub(crate) struct GpuMiner { watcher: Arc<RwLock<ProcessWatcher<GpuMinerAdapter>>>, is_available: bool, gpu_devices: HashMap<String, GpuDevice>, - curent_selected_engine: EngineType, + current_selected_engine: EngineType, }
341-369: Optimize engine selection performance.
- Unnecessary clone of the engine parameter
- Process watcher lock is held longer than needed
pub async fn set_selected_engine( &mut self, - engine: EngineType, + engine: &EngineType, config_dir: PathBuf, app: AppHandle, ) -> Result<(), anyhow::Error> { - self.curent_selected_engine = engine.clone(); + self.curent_selected_engine = engine.clone(); + + // Update adapter in a separate scope to minimize lock duration + { + let mut process_watcher = self.watcher.write().await; + process_watcher.adapter.curent_selected_engine = engine.clone(); + } - let mut process_watcher = self.watcher.write().await; - process_watcher.adapter.curent_selected_engine = engine; // ... rest of the code ... Ok(()) }
376-378: Remove unnecessary clone call.The
clone()call is redundant asPathBuf::joinreturns a newPathBuf.fn get_gpu_engines_statuses_path(config_dir: &Path) -> PathBuf { - config_dir.join("gpuminer").join("engine_statuses").clone() + config_dir.join("gpuminer").join("engine_statuses") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src-tauri/src/events.rs(2 hunks)src-tauri/src/events_emitter.rs(1 hunks)src-tauri/src/events_manager.rs(1 hunks)src-tauri/src/gpu_miner.rs(7 hunks)src-tauri/src/gpu_miner_adapter.rs(7 hunks)src-tauri/src/gpu_status_file.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/events.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tauri-build
- GitHub Check: cargo checks (fmt, clippy, check)
🔇 Additional comments (6)
src-tauri/src/gpu_status_file.rs (4)
33-38: LGTM! Well-structured GPU status representation.The struct provides a clear and concise representation of GPU status with appropriate fields for grid and block sizes.
40-53: LGTM! Clear settings structure with sensible defaults.The struct and its Default implementation provide a clean way to manage GPU device settings with reasonable initial values.
55-61: LGTM! Comprehensive device representation.The struct effectively combines device information, status, and settings into a cohesive unit.
63-74: LGTM! Efficient device storage using HashMap.The struct provides a good data structure for managing multiple GPU devices with O(1) lookup.
src-tauri/src/events_manager.rs (1)
127-139:❓ Verification inconclusive
Verify the need for #[allow(dead_code)].
The method appears to be part of the GPU engine selection feature but is marked as unused. Consider removing the attribute if the method will be used in this PR.
🏁 Script executed:
#!/bin/bash # Search for calls to handle_gpu_devices_update rg "handle_gpu_devices_update" -A 2Length of output: 215
GPU Devices Update Method – Confirm Usage
The grep output confirms that there are no call sites for
handle_gpu_devices_updatein the repository. If this method is intended to be integrated as part of the GPU engine selection feature in this PR, please remove the#[allow(dead_code)]attribute and add the appropriate usage/invocation. Otherwise, if it’s meant for future expansion, consider documenting its planned integration or removing it to avoid dead code.src-tauri/src/gpu_miner_adapter.rs (1)
190-192: LGTM! Clean engine selection implementation.The engine selection is properly integrated into the command-line arguments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src-tauri/src/gpu_status_file.rs (2)
40-53: Consider adding validation for device state flags.The
is_excludedandis_availableflags could potentially be in conflicting states. Consider adding validation to ensure that an excluded device cannot be marked as available.impl Default for GpuSettings { fn default() -> Self { Self { is_excluded: false, is_available: true, } } } +impl GpuSettings { + pub fn set_excluded(&mut self, excluded: bool) { + self.is_excluded = excluded; + if excluded { + self.is_available = false; + } + } + + pub fn set_available(&mut self, available: bool) { + self.is_available = available; + if available { + self.is_excluded = false; + } + } +}
76-95: Enhance file operations with better error handling and atomic writes.Consider the following improvements:
- Add context to error messages in the
loadmethod.- Use atomic file operations in the
savemethod to prevent data corruption.impl GpuStatusFile { pub fn load(path: &PathBuf) -> Result<Self, anyhow::Error> { - let file = File::open(path)?; - let reader = BufReader::new(file); - let config = serde_json::from_reader(reader)?; + let file = File::open(path) + .map_err(|e| anyhow!("Failed to open GPU status file '{}': {}", path.display(), e))?; + let reader = BufReader::new(file); + let config = serde_json::from_reader(reader) + .map_err(|e| anyhow!("Failed to parse GPU status file '{}': {}", path.display(), e))?; Ok(config) } pub fn save(new_content: GpuStatusFile, path: &Path) -> Result<(), anyhow::Error> { debug!( "Updating gpu status file with {:?}, at path: {:?}", new_content, path ); let content = serde_json::to_string_pretty(&new_content)?; - std::fs::write(path, content) - .map_err(|e| anyhow!("Failed to save gpu status file: {}", e))?; + let temp_path = path.with_extension("tmp"); + std::fs::write(&temp_path, &content) + .map_err(|e| anyhow!("Failed to write temporary GPU status file: {}", e))?; + std::fs::rename(&temp_path, path) + .map_err(|e| anyhow!("Failed to save GPU status file: {}", e))?; Ok(()) } }src-tauri/src/gpu_miner.rs (4)
70-78: Make engine type string conversion case-insensitive.Consider making the string conversion case-insensitive to handle variations in input.
impl EngineType { pub fn from_string(engine_type: &str) -> Result<EngineType, anyhow::Error> { - match engine_type { - "CUDA" => Ok(EngineType::Cuda), - "OpenCL" => Ok(EngineType::OpenCL), - "Metal" => Ok(EngineType::Metal), + match engine_type.to_uppercase().as_str() { + "CUDA" => Ok(EngineType::Cuda), + "OPENCL" => Ok(EngineType::OpenCL), + "METAL" => Ok(EngineType::Metal), _ => Err(anyhow::anyhow!("Invalid engine type")), } }
167-247: Enhance error messages in detect method.Consider adding more descriptive error messages to help with debugging.
- Err(anyhow::anyhow!( - "Non-zero exit code: {:?}", - output.status.code() - )) + let stderr = String::from_utf8_lossy(&output.stderr); + Err(anyhow::anyhow!( + "GPU detection failed with exit code {:?}: {}", + output.status.code(), + stderr + ))
344-372: Add validation for engine type selection.Consider validating that the selected engine is available before setting it.
pub async fn set_selected_engine( &mut self, engine: EngineType, config_dir: PathBuf, app: AppHandle, ) -> Result<(), anyhow::Error> { + let available_engines = self.get_available_gpu_engines(config_dir.clone()).await?; + if !available_engines.contains(&engine) { + return Err(anyhow::anyhow!( + "Engine {} is not available. Available engines: {:?}", + engine, + available_engines + )); + } + self.curent_selected_engine = engine.clone(); let mut process_watcher = self.watcher.write().await; process_watcher.adapter.curent_selected_engine = engine;
379-381: Remove unnecessary clone.The
clone()call is unnecessary asjoinreturns a newPathBuf.fn get_gpu_engines_statuses_path(config_dir: &Path) -> PathBuf { - config_dir.join("gpuminer").join("engine_statuses").clone() + config_dir.join("gpuminer").join("engine_statuses") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src-tauri/src/gpu_miner.rs(7 hunks)src-tauri/src/gpu_status_file.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cargo checks (fmt, clippy, check)
- GitHub Check: tauri-build
🔇 Additional comments (5)
src-tauri/src/gpu_status_file.rs (2)
33-38: LGTM!The
GpuStatusstruct is well-designed with appropriate fields for GPU configuration.
55-61: LGTM!The
GpuDevicestruct is well-designed with appropriate fields for device metadata and configuration.src-tauri/src/gpu_miner.rs (3)
249-285: Remove commented code and improve robustness of engine type extraction.The commented-out line should be removed, and the string splitting operation should be made more robust.
315-342: Improve error handling for device exclusion.The device lookup should handle the case when the device is not found.
374-376: LGTM!The method correctly returns a vector of GPU devices.
[ Summary ]
[ Testing ]
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor