fix: exclude gpu devices while mining #1560
Conversation
📝 WalkthroughWalkthroughThis pull request simplifies the management of GPU mining toggle states. In the GPU device component, several mining status variables are removed and a new variable, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as GpuDevices Component
participant S as MiningStore
U->>C: Toggle GPU device switch
C->>S: Invoke setExcludedGpuDevice()
S->>S: Set isExcludingGpuDevices = true
S->>S: Check active CPU/GPU mining status
S->>S: Exclude the selected GPU device(s)
S->>S: If all GPUs excluded, disable GPU mining
S->>S: If mining was initiated, restart mining
S->>S: Set isExcludingGpuDevices = false
S-->>C: Return updated mining state
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/store/useMiningStore.ts (1)
88-118: 🛠️ Refactor suggestionImprove error handling and state management in setExcludedGpuDevice.
While the implementation correctly handles the GPU device exclusion workflow, there are a few areas that could be improved:
- Error handling for mining pause/restart operations
- Potential race conditions between state checks and actions
- Input validation for excludedGpuDevices array
Consider applying these improvements:
setExcludedGpuDevice: async (excludedGpuDevices) => { + if (!Array.isArray(excludedGpuDevices)) { + throw new Error('excludedGpuDevices must be an array'); + } + set({ isExcludingGpuDevices: true }); - const metricsState = useMiningMetricsStore.getState(); + try { + const metricsState = useMiningMetricsStore.getState(); - if (metricsState.cpu_mining_status.is_mining || metricsState.gpu_mining_status.is_mining) { - console.info('Pausing mining...'); - await pauseMining(); - } + if (metricsState.cpu_mining_status.is_mining || metricsState.gpu_mining_status.is_mining) { + console.info('Pausing mining...'); + await pauseMining(); + } - try { await invoke('set_excluded_gpu_devices', { excludedGpuDevices }); const totalGpuDevices = useMiningMetricsStore.getState().gpu_devices?.length || 0; if (excludedGpuDevices.length === totalGpuDevices) { const appConfigStore = useAppConfigStore.getState(); appConfigStore.setGpuMiningEnabled(false); } set({ excludedGpuDevices }); - } catch (e) { - const appStateStore = useAppStateStore.getState(); - console.error('Could not set excluded gpu device: ', e); - appStateStore.setError(e as string); - set({ excludedGpuDevices: undefined }); - } - if (useMiningStore.getState().miningInitiated) { - console.info('Restarting mining...'); - await startMining(); - } - set({ isExcludingGpuDevices: false }); + if (useMiningStore.getState().miningInitiated) { + console.info('Restarting mining...'); + await startMining(); + } + } catch (e) { + const appStateStore = useAppStateStore.getState(); + console.error('Error in setExcludedGpuDevice: ', e); + appStateStore.setError(e as string); + set({ excludedGpuDevices: undefined }); + } finally { + set({ isExcludingGpuDevices: false }); + } },
🧹 Nitpick comments (2)
src/containers/floating/Settings/sections/mining/GpuDevices.tsx (2)
66-71: Enhance ToggleSwitch performance and accessibility.While the toggle switch implementation is functionally correct, consider these improvements:
<ToggleSwitch key={device.name} checked={!excludedDevices.includes(i)} disabled={isExcludingGpuDevices || !miningAllowed || !isGpuMiningEnabled} onChange={() => handleSetExcludedDevice(i)} + aria-label={`Toggle ${device.name}`} + title={isExcludingGpuDevices ? t('gpu-device-excluding-in-progress') : undefined} />Also consider memoizing the checked state:
const isDeviceEnabled = useMemo( () => !excludedDevices.includes(i), [excludedDevices, i] );
29-40: Consider memoizing excluded devices array operations.The
handleSetExcludedDevicecallback could benefit from memoizing the array operations to prevent unnecessary re-renders.const handleSetExcludedDevice = useCallback( async (device: number) => { + const newExcludedDevices = excludedDevices.includes(device) + ? excludedDevices.filter(d => d !== device) + : [...excludedDevices, device]; - if (!excludedDevices.includes(device)) { - excludedDevices.push(device); - await setExcludedDevice([...excludedDevices]); - } else { - excludedDevices.splice(excludedDevices.indexOf(device), 1); - await setExcludedDevice([...excludedDevices]); - } + await setExcludedDevice(newExcludedDevices); }, [excludedDevices, setExcludedDevice] );
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/containers/floating/Settings/sections/mining/GpuDevices.tsx(2 hunks)src/store/useMiningStore.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tauri-build
🔇 Additional comments (3)
src/store/useMiningStore.ts (2)
10-21: LGTM! State interface is well-defined.The new
isExcludingGpuDevicesboolean property is appropriately added to theStateinterface.
32-43: LGTM! Initial state is properly initialized.The
isExcludingGpuDevicesproperty is correctly initialized tofalsein the initial state.src/containers/floating/Settings/sections/mining/GpuDevices.tsx (1)
27-27: LGTM! State hook is correctly implemented.The
isExcludingGpuDevicesstate is properly accessed using the store hook.
Description
Allow user to enable/disable specific GPU devices when mining in progress. After each change mining is paused and restarted to apply changes.
What process can a PR reviewer use to test or verify this change?
Summary by CodeRabbit
New Features
Refactor