fix: app restarts after sleepmode #1237
Conversation
|
This issue still persists. After putting machine to sleep mode I still had setup view again, looks like the problem is with |
📝 WalkthroughWalkthroughThis pull request updates localization files across multiple languages by adding new key-value pairs to support auto-update error messages and resume-process UI components. It enhances the application's update and resumption logic by integrating system power state monitoring. New command functions, hooks, and components are introduced to listen for system sleep mode events and manage process resumption. Additionally, a dependency is added to Cargo for the psp package, and new utility modules are included for system status monitoring. Changes
Sequence Diagram(s)sequenceDiagram
participant SS as SystemStatus
participant Main as main.rs
participant Shutdown as shutdown_utils
participant UI as Resume Modal
participant Hook as useListenForAppResuming
SS->>Main: start_listener()
Note right of Main: Listens for power events
SS->>Main: receive_power_event() [System wakes]
Main->>Shutdown: resume_all_processes()
Shutdown->>UI: Emit "resuming-all-processes" event
UI->>Hook: Update resume payload state
Hook->>UI: Show ResumeApplicationModal
sequenceDiagram
participant UM as UpdatesManager
participant SS as SystemStatus
participant UI as AutoUpdateDialog
UM->>SS: Check get_sleep_mode_watcher()
alt Machine Locked or In Sleep
UM->>UI: Emit CouldNotUpdatePayload event
UI->>User: Display update failure message
else
UM->>UM: Proceed with update
end
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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: 26
🔭 Outside diff range comments (2)
src/store/appStateStore.ts (1)
126-141: 🛠️ Refactor suggestionClear appResumePayload when setup is complete.
The appResumePayload should be cleared when setup is complete to prevent stale state.
export const setSetupComplete = async () => { const visualMode = useAppConfigStore.getState().visual_mode; if (visualMode) { const canvas = document.getElementById('canvas'); if (canvas) { canvas.style.opacity = '1'; setAnimationState('showVisual'); } } // Proceed with auto mining when enabled const { mine_on_app_start, cpu_mining_enabled, gpu_mining_enabled } = useAppConfigStore.getState(); if (mine_on_app_start && (cpu_mining_enabled || gpu_mining_enabled)) { startMining(); } - useAppStateStore.setState({ setupComplete: true }); + useAppStateStore.setState({ + setupComplete: true, + appResumePayload: undefined + }); };src/containers/main/SideBar/components/MiningButton/MiningButton.tsx (1)
21-32: 🛠️ Refactor suggestionConsider adding sleep mode state handling.
Given that the app restarts after sleep mode and users are prompted with the setup view, this component should properly handle system sleep/resume states to prevent mining state inconsistencies.
Consider these improvements:
- Add a useEffect to handle sleep/resume events
- Reset mining states appropriately on system resume
- Add proper error boundaries to handle state recovery
Example implementation:
import { useEffect } from 'react'; import { useListenForAppResuming } from '@app/hooks/app/useListenForAppResuming'; // Add at the beginning of the component const handleAppResume = useCallback(() => { if (isMining) { // Verify mining state and reconnect if necessary stopMining().then(() => startMining()); } }, [isMining]); useListenForAppResuming(handleAppResume);
🧹 Nitpick comments (15)
src-tauri/src/utils/system_status.rs (3)
33-54: Consider adjusting error handling innew().
The call toSystemStatus::start_listener(&power_monitor).expect("Error starting listener")in the constructor forces a panic on any failure starting the listener, which may not be desirable in all scenarios. Also, note the spelling difference betweencancelation_token(field) andCancellationToken(type).fn new() -> Self { let power_monitor = PowerMonitor::new(); - SystemStatus::start_listener(&power_monitor).expect("Error starting listener"); + if let Err(e) = SystemStatus::start_listener(&power_monitor) { + log::error!("Failed to start PowerMonitor: {:?}", e); + // Optionally, return an error or handle gracefully + } let (sleep_mode_watcher_sender, sleep_mode_watcher_receiver) = watch::channel(false); Self { power_monitor, - cancelation_token: RwLock::new(None), + cancellation_token: RwLock::new(None), // Adjust spelling for consistency sleep_mode_watcher_sender, sleep_mode_watcher_receiver, } }
61-89: Correct the spelling ofrecive_power_event.
Typo in the function name (“recive” instead of “receive”). Although not critical, renaming it would improve clarity. Additionally, consider handling thesend()result in case the watch channel is closed.-async fn recive_power_event(&self) -> Result<(), Error> { +async fn receive_power_event(&self) -> Result<(), Error> { if let Ok(event) = self.power_monitor.event_receiver().try_recv() { ...
91-112: Consider making poll interval configurable.
Spawning a task that continuously loops with a fixed 5-second sleep can be acceptable but might benefit from a configurable or adaptive interval. Also, confirm that callingSystemStatus::current()from within the loop does not introduce difficulties in testing or concurrency.src-tauri/src/utils/shutdown_utils.rs (1)
143-392:resume_all_processesis comprehensive but large.
The function orchestrates multiple managers, progress tracking, emission events, and restarts. While it works, consider splitting it into smaller helper functions or modules to reduce complexity. This modularization can improve readability and facilitate targeted testing for each stage.src/containers/floating/ResumeApplicationModal/styles.ts (2)
8-9: Use theme variables for consistent styling.Consider using theme variables for colors and font sizes to maintain consistency across the application.
- background: #fff; + background: ${({ theme }) => theme.colors.background}; - color: #000; + color: ${({ theme }) => theme.colors.text}; - font-size: 14px; + font-size: ${({ theme }) => theme.typography.sizes.md};Also applies to: 35-36, 44-45
16-20: Add z-index for proper modal stacking.Since this is a modal component with fixed positioning, ensure proper stacking order with other floating elements.
position: fixed; + z-index: ${({ theme }) => theme.zIndex.modal}; bottom: 20px;src-tauri/src/events.rs (1)
41-47: Add documentation and validation for the ResumingAllProcessesPayload struct.Consider adding:
- Documentation explaining the purpose and usage of this struct
- Validation to ensure stage_progress doesn't exceed stage_total
#[derive(Debug, Serialize, Clone)] +/// Represents the state of processes being resumed after system sleep/lock +/// - title: Localization key for the current stage +/// - stage_progress: Current progress (must not exceed stage_total) +/// - stage_total: Total number of stages +/// - is_resuming: Whether the resuming process is active pub struct ResumingAllProcessesPayload { pub title: String, pub stage_progress: u32, pub stage_total: u32, pub is_resuming: bool, + + #[serde(skip)] + #[doc(hidden)] + pub fn validate(&self) -> Result<(), &'static str> { + if self.stage_progress > self.stage_total { + return Err("stage_progress cannot exceed stage_total"); + } + Ok(()) + } }src/containers/floating/AutoUpdateDialog/AutoUpdateDialog.tsx (2)
31-40: Improve subtitle resolution with enum-based approach.The current switch-true pattern could be replaced with a more maintainable enum-based approach.
+enum UpdateDialogState { + Downloading = 'downloading', + Failed = 'failed', + Initial = 'initial', +} + -const resolveSubtitle = (isDownloading: boolean, couldNotUpdate: boolean) => { +const resolveSubtitle = (state: UpdateDialogState) => { - switch (true) { - case isDownloading: + switch (state) { + case UpdateDialogState.Downloading: return 'installing-latest-version'; - case couldNotUpdate: + case UpdateDialogState.Failed: return 'could-not-auto-update'; - default: + case UpdateDialogState.Initial: return 'would-you-like-to-install'; } };
72-75: Enhance error handling for could_not_update event.Consider logging the error and providing more context to users.
case 'could_not_update': setDialogToShow('autoUpdate'); setCouldNotUpdate(true); + console.error('Auto-update failed:', payload.error); + addToast({ + title: t('update-failed'), + text: t('update-failed-description'), + type: 'error', + }); break;src/store/appStateStore.ts (1)
115-115: Add validation in setAppResumePayload.Consider validating the payload before setting it in the store.
- setAppResumePayload: (appResumePayload) => set({ appResumePayload }), + setAppResumePayload: (appResumePayload) => { + if (appResumePayload.stage_progress > appResumePayload.stage_total) { + console.error('Invalid resume payload: progress exceeds total'); + return; + } + set({ appResumePayload }); + },src-tauri/src/updates_manager.rs (1)
125-136: Consider adding error handling for screen lock status check.While the screen lock check is implemented correctly, consider handling potential errors from the
get_sleep_mode_watcher()call to make the code more robust.-let is_screen_locked = *SystemStatus::current().get_sleep_mode_watcher().borrow(); +let is_screen_locked = match SystemStatus::current().get_sleep_mode_watcher().try_borrow() { + Ok(guard) => *guard, + Err(e) => { + warn!(target: LOG_TARGET, "Failed to get sleep mode status: {}", e); + false // Default to not locked to allow update attempt + } +};src-tauri/src/main.rs (1)
907-930: Consider adding cleanup for the sleep mode monitoring task.The sleep mode monitoring task runs indefinitely. Consider adding a shutdown mechanism to gracefully stop the task when the application exits.
let app_handle_clone: tauri::AppHandle = app.clone(); +let shutdown_signal = state.shutdown.to_signal(); tauri::async_runtime::spawn(async move { let mut receiver = SystemStatus::current().get_sleep_mode_watcher(); let mut last_state = *receiver.borrow(); loop { + if shutdown_signal.is_triggered() { + info!(target: LOG_TARGET, "Shutting down sleep mode monitor"); + break; + } if receiver.changed().await.is_ok() {src-tauri/src/commands.rs (1)
847-848: Add error logging for reset_settings completion.Consider adding a log message to track successful completion of the reset operation.
+ info!(target: LOG_TARGET, "Settings reset completed successfully"); Ok(())public/locales/tr/setup-view.json (1)
5-5: Consider addressing the core issue with app restarts.While these localization changes improve user feedback when updates fail during sleep mode, they don't address the core issue reported by user MCozhusheck where the app still restarts and shows the setup view after sleep mode. Consider investigating the
useEffecthook that triggers theapplication_setupfunction unexpectedly.src/containers/main/SideBar/components/MiningButton/MiningButton.tsx (1)
47-53: Add error handling for mining state transitions.The mining start/stop operations should handle potential errors during sleep/resume transitions.
Consider wrapping the mining operations with proper error handling:
const handleClick = useCallback(async () => { + try { if (!isMining) { await startMining(); } else { await stopMining(); } + } catch (error) { + console.error('Mining operation failed:', error); + // Handle the error appropriately + } }, [isMining]);
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
public/locales/af/common.json(1 hunks)public/locales/af/setup-view.json(1 hunks)public/locales/cn/common.json(1 hunks)public/locales/cn/setup-view.json(1 hunks)public/locales/de/common.json(1 hunks)public/locales/de/setup-view.json(1 hunks)public/locales/en/common.json(1 hunks)public/locales/en/components.json(1 hunks)public/locales/en/setup-view.json(1 hunks)public/locales/fr/common.json(1 hunks)public/locales/fr/setup-view.json(1 hunks)public/locales/hi/common.json(1 hunks)public/locales/hi/setup-view.json(1 hunks)public/locales/id/common.json(1 hunks)public/locales/id/setup-view.json(1 hunks)public/locales/ja/common.json(1 hunks)public/locales/ja/setup-view.json(1 hunks)public/locales/ko/common.json(1 hunks)public/locales/ko/setup-view.json(1 hunks)public/locales/pl/common.json(1 hunks)public/locales/pl/setup-view.json(1 hunks)public/locales/ru/common.json(1 hunks)public/locales/ru/setup-view.json(1 hunks)public/locales/tr/common.json(1 hunks)public/locales/tr/setup-view.json(1 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/src/commands.rs(3 hunks)src-tauri/src/events.rs(1 hunks)src-tauri/src/main.rs(5 hunks)src-tauri/src/updates_manager.rs(3 hunks)src-tauri/src/utils/mod.rs(1 hunks)src-tauri/src/utils/shutdown_utils.rs(3 hunks)src-tauri/src/utils/system_status.rs(1 hunks)src/App/AppEffects.tsx(2 hunks)src/containers/floating/AutoUpdateDialog/AutoUpdateDialog.tsx(4 hunks)src/containers/floating/FloatingElements.tsx(2 hunks)src/containers/floating/ResumeApplicationModal/ResumeApplicationModal.tsx(1 hunks)src/containers/floating/ResumeApplicationModal/styles.ts(1 hunks)src/containers/main/SideBar/components/MiningButton/MiningButton.tsx(1 hunks)src/hooks/app/useListenForAppResuming.ts(1 hunks)src/store/appStateStore.ts(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Run linters
src/containers/floating/ResumeApplicationModal/ResumeApplicationModal.tsx
[warning] 18-18:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 31-31:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 35-35:
React Hook useEffect has a missing dependency: 'setMiningControlsEnabled'. Either include it or remove the dependency array
src/containers/main/SideBar/components/MiningButton/MiningButton.tsx
[warning] 33-33:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 34-34:
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
[warning] 36-36:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 37-37:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 38-38:
Unexpected console statement. Only these console methods are allowed: info, warn, debug, error, group, groupCollapsed, groupEnd
[warning] 39-39:
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 (20)
src-tauri/src/utils/system_status.rs (6)
1-22: License header is consistent and clear.
No issues found; it meets standard open source license conventions.
23-32: Imports and global instance seem appropriate.
UsingLazyLock<SystemStatus>is a valid approach for a globally accessible singleton instance; just ensure that the Rust version used supportsLazyLockif targeting older toolchains.
56-59:start_listenerwrapper is straightforward.
No significant concerns here. Wrapping the error inanyhow!is fine, but adding a custom context string could further aid debugging.
114-121: Orderly shutdown is well-handled.
Stopping the listener by canceling the token is a clean approach; no issues detected.
123-125: Public watcher retrieval is straightforward.
Returning a cloned receiver is a good pattern for read-only subscription.
127-129: Singleton accessor is acceptable.
Exposing a static reference to a per-applicationSystemStatusis clear for this design.src-tauri/src/utils/shutdown_utils.rs (1)
51-54: Graceful listener shutdown is included.
EnsuringSystemStatus::current().stop_listener()is invoked during overall application shutdown helps prevent orphaned tasks.src/hooks/app/useListenForAppResuming.ts (2)
5-10: Interface definition is concise.
The structure is clear and matches the emitted event.
12-24: Event listener hook is well-structured.
Uses the TaurilistenAPI and updates global state correctly. Good cleanup on unmount. No issues found.src/App/AppEffects.tsx (1)
24-25:❓ Verification inconclusive
Verify hook execution order and state persistence.
Given the reported issue of the setup view appearing after sleep mode, consider:
- The hook execution order might affect state persistence
- The app state might be reset during the resuming process
Let's check for any state reset patterns in the codebase:
🏁 Script executed:
#!/bin/bash # Search for state reset patterns in useEffect hooks rg -A 5 "useEffect.*\[\]" src/ # Search for setup view triggers ast-grep --pattern 'useEffect($$$) { $$$ application_setup($$$) $$$ }'Length of output: 111
Action Required: Verify hook implementations to ensure proper state persistence
The initial search for explicit state reset patterns (e.g. within useEffect hooks) didn’t return any evidence. However, it’s still critical to confirm that the hooks’ internal implementations—especially for
useListenForAppResuminganduseListenForAppUpdated—are correctly managing state persistence when the app resumes from sleep mode. In particular, verify that:
useListenForAppUpdateddoes not inadvertently reset state when triggered.useListenForAppResumingproperly maintains persisted state and its execution order relative to the updated hook.Please review the hook definitions (e.g., in their respective source files) to ensure their behavior aligns with the intended state management strategy.
If needed, consider running additional searches on the hook definitions by executing:
#!/bin/bash # Search for the definition of useListenForAppResuming and useListenForAppUpdated in the repository rg -n "function useListenForAppResuming" -n src/ rg -n "function useListenForAppUpdated" -n src/This extra verification can help confirm whether any internal state reset logic exists.
src-tauri/src/utils/mod.rs (1)
32-32:❓ Verification inconclusive
Verify system_status integration with state persistence.
The new system_status module is crucial for handling sleep mode. Ensure it properly integrates with state persistence mechanisms.
Let's check the integration points:
🏁 Script executed:
#!/bin/bash # Search for system_status usage rg -A 5 "system_status::" src-tauri/src/ # Check for state persistence handlers rg -A 5 "(save|load|persist).*state" src-tauri/src/Length of output: 1644
Action Required: Confirm the state persistence handling in the system_status module
The system_status module is integrated across several parts of the code (e.g., in
shutdown_utils.rs,commands.rs,updates_manager.rs, andmain.rs). However, our search didn't reveal any explicit usage of state persistence operations (like save, load, or persist) linked to system_status. Please verify manually whether state persistence is managed implicitly or if additional implementation is needed to handle sleep mode state persistence.src/containers/floating/FloatingElements.tsx (1)
36-36:❓ Verification inconclusive
Coordinate modal visibility with other floating elements.
Since multiple modals can be active, ensure proper coordination between ResumeApplicationModal and other dialogs, especially during sleep/resume cycles.
Let's check for potential modal conflicts:
🏁 Script executed:
#!/bin/bash # Search for modal visibility state management rg -A 5 "isVisible|isOpen|show.*Modal" src/containers/floating/ # Check for modal coordination logic ast-grep --pattern 'const $_ = use$_Store($$$)'Length of output: 10636
Centralize Modal Coordination
The addition of
ResumeApplicationModalrequires a careful review to ensure it integrates seamlessly with the other floating modals. In particular:
- Verify that its state management (using
appResumePayload?.is_resuming) aligns with the approaches in modals like PaperWalletModal, StagedSecurity, and ShareRewardModal.- Confirm that modal lifecycle events—especially around sleep/resume cycles—do not lead to overlapping or conflicting displays.
- Consider whether a centralized modal management strategy could help simplify and standardize coordination across all modals.
src-tauri/src/updates_manager.rs (1)
55-68: LGTM! Well-structured payload for update failure notifications.The new
CouldNotUpdatePayloadstruct is well-designed with appropriate fields for event type and version.src-tauri/src/commands.rs (1)
852-859: LGTM! Well-implemented language resolution command.The
resolve_application_languagecommand properly handles the language configuration update.public/locales/cn/common.json (1)
907-930:❓ Verification inconclusive
Verify the frontend setup view behavior.
While the backend changes for sleep mode handling look good, the reported issue with the setup view reappearing might be related to the frontend
useEffecthook. Let's verify the frontend code:
🏁 Script executed:
#!/bin/bash # Search for useEffect hooks in setup view components echo "Searching for useEffect in setup view components..." rg -A 5 "useEffect.*setup" src/Length of output: 146
Please Verify the Frontend Setup View Behavior
The search for
useEffectusage in setup-related components produced no output, which makes it unclear whether the intended hook is implemented or if it’s simply not present. Given the reported issue with the setup view reappearing, please manually verify that the frontend component managing the setup view is correctly handling its lifecycle (and that there isn’t an unintendeduseEffecttrigger causing a re-render during state transitions).
- Action Items:
- Manually check the Setup view component(s) for any lifecycle or effect hooks that could be responsible for re-triggering the view.
- Confirm that all conditions controlling the display of the setup view are correctly implemented to prevent unintended re-mounting or re-update when the system resumes from sleep/lock mode.
public/locales/en/common.json (1)
31-33: LGTM!The English translations are correct and consistent with the rest of the file.
public/locales/tr/common.json (1)
1-34:❓ Verification inconclusive
Verify the sleep mode fix.
Based on the PR objectives and user feedback, the app still restarts and shows the setup view after sleep mode despite these localization changes. The issue might be related to the
useEffecthook triggering theapplication_setupfunction unexpectedly.Let's search for the relevant code:
🏁 Script executed:
#!/bin/bash # Search for useEffect hooks that might trigger application_setup ast-grep --pattern 'useEffect(() => { $$$ application_setup $$$ }, $_)' # Search for SystemStatus utility implementation rg -A 5 "class SystemStatus"Length of output: 110
Let's run additional commands to search for every invocation of
application_setupand any sleep mode or visibility-related event handlers that might be causing the issue. This will help us verify whether the unintended triggering of the setup view is coming from these areas.
🏁 Script executed:
#!/bin/bash # Search all invocations of application_setup echo "Searching for 'application_setup' invocations:" rg -n "application_setup" . # Search for any handler or event related to sleep mode or visibility changes echo "Searching for sleep mode or visibility change event handlers:" rg -n "visibilitychange" . rg -n "sleepMode" . rg -n "wake" .Length of output: 59193
Sleep Mode Issue Needs Further Verification
- The localization file (
public/locales/tr/common.json) looks properly updated.- Our searches for unexpected invocations of
application_setup—including withinuseEffecthooks—and for sleep mode/visibility event handlers yielded no clear evidence of unintended triggers.- The root cause of the app restarting after sleep mode isn’t evident from these searches. It might be necessary to review the sleep mode handling logic (e.g., event listeners for
visibilitychange, wake events, etc.) and add logging or tests to isolate whyapplication_setupis being invoked.Please verify the sleep mode event handling code manually to ensure that the app does not restart unexpectedly after sleep mode.
public/locales/en/components.json (1)
22-31: LGTM! Clear and descriptive messages for the resume process.The new resume-app-modal section provides clear feedback during the application resuming process.
public/locales/ja/setup-view.json (1)
1-33:❓ Verification inconclusive
Verify if these localization changes address the reported issue.
Based on the PR objectives and user feedback, the app still restarts after sleep mode and shows the setup view unexpectedly. While these localization changes improve the user experience by providing feedback during updates and resuming, they don't appear to address the core issue which might be related to the
useEffecthook in the setup view.To help investigate the issue:
Check for any setup-related effects that might trigger unnecessarily:
Check for any sleep mode or power status handling:
🏁 Script executed:
#!/bin/bash # Search for useEffect hooks in setup-related components rg -A 5 "useEffect.*setup" --type ts --type tsxLength of output: 77
🏁 Script executed:
#!/bin/bash # Search for power status or sleep mode related code rg -A 5 "PowerStatus|SystemStatus|sleep|lock" --type ts --type tsx --type rsLength of output: 106
Let's re-run the verification commands using corrected file globs since the previous commands failed due to the unrecognized file type "tsx". Please execute the following updated shell scripts to gather more reliable results:
Please run these commands and share the outputs so we can further verify whether the localization changes truly address any lingering issues.
🏁 Script executed:
#!/bin/bash # Search for useEffect hooks in setup-related components in .ts and .tsx files using file globs rg -A 5 "useEffect.*setup" -g "*.ts" -g "*.tsx"Length of output: 49
🏁 Script executed:
#!/bin/bash # Search for power status, sleep mode, or lock-related code in .ts, .tsx, and .rs files using file globs rg -A 5 "PowerStatus|SystemStatus|sleep|lock" -g "*.ts" -g "*.tsx" -g "*.rs"Length of output: 66172
Action Required: Confirm Underlying Restart Issue Beyond Localization
- The localization updates in
public/locales/ja/setup-view.jsonimprove in-app messaging but do not tackle the core behavior where the app restarts after sleep mode and the setup view appears unexpectedly.- Our automated search for
useEffecthooks related to the setup component in TypeScript/TSX files did not yield any relevant results, suggesting that the problematic behavior might not originate from such hooks—or that it resides in a different part of the lifecycle.- The investigation of sleep, lock, and power status handling shows these are managed within the Rust code (e.g., in
src-tauri/src/download_utils.rsand others), further indicating that the localization changes are not directly linked to the root issue.Please conduct a manual review of the setup view lifecycle and any asynchronous logic (e.g., event listeners or lifecycle methods) that might be causing the unwanted restart behavior.
public/locales/en/setup-view.json (1)
5-5: LGTM!The new message is clear, grammatically correct, and consistent with the existing messages.
src/containers/floating/ResumeApplicationModal/ResumeApplicationModal.tsx
Outdated
Show resolved
Hide resolved
src/containers/floating/ResumeApplicationModal/ResumeApplicationModal.tsx
Outdated
Show resolved
Hide resolved
src/containers/main/SideBar/components/MiningButton/MiningButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/hooks/mining/useMiningUiStateMachine.ts (2)
16-16:⚠️ Potential issueAvoid direct window object access which may be unreliable after sleep mode.
The direct access to
window.glAppmay be unreliable when the app resumes from sleep mode, potentially contributing to the restart issue.Consider injecting these dependencies through a context or store:
- const statusIndex = window?.glApp?.stateManager?.statusIndex; + const { statusIndex, status } = useGlAppStore((state) => ({ + statusIndex: state.statusIndex, + status: state.status + })); useEffect(() => { - const status = window?.glApp?.stateManager?.status; - const notStarted = !status || status == 'not-started' || status == 'stop'; + const notStarted = !status || status === 'not-started' || status === 'stop';Also applies to: 19-20
26-33:⚠️ Potential issueConsider resume state in stop animation logic.
The second
useEffectdoesn't consider theisResumingstate, which could lead to incorrect animation states during resume.Add
isResumingcheck to prevent unnecessary stops:useEffect(() => { const notStopped = window?.glApp?.stateManager?.status !== 'not-started'; - const preventStop = !setupComplete || isMiningInitiated || isChangingMode; + const preventStop = !setupComplete || isMiningInitiated || isChangingMode || isResuming; const shouldStop = !isMining && notStopped && !preventStop; if (shouldStop) { setAnimationState('stop'); } - }, [statusIndex, setupComplete, isMiningInitiated, isMining, isChangingMode]); + }, [statusIndex, setupComplete, isMiningInitiated, isMining, isChangingMode, isResuming]);
🧹 Nitpick comments (6)
public/locales/tr/components.json (1)
21-32: Review: New "resume-app-modal" Localization Section in Turkish FileThe new
"resume-app-modal"block has been added with keys for various resuming states. All string values are in English, which is consistent across the board. However, since this is the Turkish locale file, please verify that these values are intended to remain in English or if they should be translated.public/locales/ru/components.json (1)
21-32: Review: New "resume-app-modal" Section in Russian LocalizationThe
"resume-app-modal"section is consistently added with expected keys. Currently, the values are maintained in English. Considering the rest of the file is localized to Russian, please confirm if the new entries should be translated into Russian now or later in the localization process.public/locales/hi/components.json (1)
21-32: Review: "resume-app-modal" Section in Hindi LocaleThe new
"resume-app-modal"block appears uniformly with all keys provided in English. Since other parts of the file are in Hindi, it might be beneficial to localize these messages too, if that is the intended behavior. Please confirm the plan for these strings.public/locales/id/components.json (1)
21-32: Review: "resume-app-modal" Localization Update in Indonesian FileThe additions under the
"resume-app-modal"key are clear and consistent with other locale files. Note that all values are still in English. Verify whether these strings should be localized into Indonesian now or if they are placeholders pending translation.public/locales/af/components.json (1)
21-32: Review: New "resume-app-modal" Section in Afrikaans LocaleThe
"resume-app-modal"section is successfully added with all expected keys. As in the other files, the values remain in English. Please review if this is intentional for the Afrikaans file or if translations will be provided, to ensure consistency with the remainder of the content that is localized.src-tauri/src/main.rs (1)
905-928: Consider implementing a state persistence layer.The current implementation handles process management during sleep/wake cycles but doesn't address state persistence. This architectural gap likely causes the reported issue where users are prompted to go through the setup view again after sleep mode.
Consider the following architectural improvements:
- Implement a state persistence layer that saves critical application state before sleep mode
- Add state validation and recovery mechanisms during wake-up
- Add a state version system to handle migrations
- Consider using a transaction-like approach where state changes are atomic
This would help prevent the application from losing its state during sleep/wake cycles and avoid unexpected setup view prompts.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
public/locales/af/components.json(1 hunks)public/locales/cn/components.json(1 hunks)public/locales/de/components.json(1 hunks)public/locales/fr/components.json(1 hunks)public/locales/hi/components.json(1 hunks)public/locales/id/components.json(1 hunks)public/locales/ja/components.json(1 hunks)public/locales/ko/components.json(1 hunks)public/locales/pl/components.json(1 hunks)public/locales/ru/components.json(1 hunks)public/locales/tr/components.json(1 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/src/commands.rs(2 hunks)src-tauri/src/main.rs(6 hunks)src-tauri/src/utils/shutdown_utils.rs(2 hunks)src-tauri/src/utils/system_status.rs(1 hunks)src/containers/floating/ResumeApplicationModal/ResumeApplicationModal.tsx(1 hunks)src/hooks/mining/useMiningUiStateMachine.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/Cargo.toml
- src-tauri/src/commands.rs
- src/containers/floating/ResumeApplicationModal/ResumeApplicationModal.tsx
- src-tauri/src/utils/system_status.rs
🔇 Additional comments (6)
src/hooks/mining/useMiningUiStateMachine.ts (1)
13-13:❓ Verification inconclusive
Review setupComplete dependency which may trigger unnecessary setup.
The dependency on
setupCompletein combination with sleep mode might be causing the app to unnecessarily trigger setup again.Let's verify the setup logic:
Also applies to: 28-29
🏁 Script executed:
#!/bin/bash # Search for setupComplete usage and state updates ast-grep --pattern 'setupComplete = $_'Length of output: 41
Action Required: Verify
setupCompleteDependency ImpactAfter running the initial search, we found no direct assignments (e.g. using
setupComplete = ...) that would indicate an unexpected change to this state. This suggests that the usage ofsetupCompletein the hook is read-only. However, because the original concern was that combining this dependency with the sleep mode logic might trigger an unnecessary setup, please double-check the related logic—especially around lines 28-29—to confirm that any re-renders driven bysetupCompleteare intentional and correctly managed.
- Verify in the codebase:
- That
setupCompleteis only read from the store (as observed) and not modified in a way that could cause spurious re-executions.- The sleep mode logic does not inadvertently cause changes that re-trigger the setup.
Once you've ensured that the dependency does not trigger an undesired setup, consider adding documentation or refining the hook’s dependency list to make the intended behavior clear.
public/locales/de/components.json (1)
22-31: LGTM! Well-structured localization strings.The added "resume-app-modal" section provides clear and informative German translations for the app resumption process.
src-tauri/src/utils/shutdown_utils.rs (3)
138-167: LGTM! Well-structured progress tracking.The progress tracking implementation dynamically adjusts total stages based on enabled features (Tor and P2Pool), providing accurate progress feedback to users.
225-253: LGTM! Robust error handling for database corruption.The implementation properly handles database corruption by attempting to clean and retry, with appropriate logging and error propagation.
138-387:❓ Verification inconclusive
Verify the fix addresses the reported issue.
Based on the user feedback that the app still prompts the setup view after sleep mode, this implementation might not fully address the root cause. The issue might be in the React
useEffecthook as mentioned in the PR comments.Let's search for the setup view trigger:
🏁 Script executed:
#!/bin/bash # Search for setup view related code rg -A 5 "setup.*view|useEffect.*setup"Length of output: 65577
Action Required: Confirm React Side Handling of the Setup View Trigger
The Rust code in
src-tauri/src/utils/shutdown_utils.rs—specifically in theresume_all_processesfunction—appears to resume all required processes as expected. Our search did not reveal any code in this file that would directly trigger the setup view. Given the user feedback that the app still prompts the setup view after sleep mode, it’s likely that the issue lies on the frontend. The ReactuseEffecthook (or related state management) may be inadvertently causing the setup view to reappear after sleep.
- The shutdown/resume logic in Rust seems correct.
- No indications of a "setup view" trigger were found in the Rust code.
- It is recommended to review the React side—especially the
useEffecthook logic—to ensure that state updates or side effects correctly preserve the application’s resumed state.src-tauri/src/main.rs (1)
45-45: LGTM!The imports for
SystemStatusand shutdown utilities are correctly added.Also applies to: 92-92
| @@ -95,6 +95,7 @@ hex = "0.4.3" | |||
| openssl = { version = "0.10", features = ["vendored"] } | |||
| ring-compat = "0.8.0" | |||
| der = "0.7.9" | |||
| psp = { git = "https://github.com/Misieq01/psp.git", rev = "ddef0c6" } | |||
There was a problem hiding this comment.
If we're running a forked version of the plugin it'll need to be hosted under the tari-projects organisation, and we need an explanation of why we're using the forked version.
| @@ -124,3 +134,255 @@ pub async fn stop_all_processes( | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| #[allow(clippy::too_many_lines)] | |||
| pub async fn resume_all_processes(app_handle: tauri::AppHandle) -> Result<(), anyhow::Error> { | |||
There was a problem hiding this comment.
Disclaimer: This is a question for discussion, not a change request
The thing I get worried about here is that it is similar but different.
Without comparing line by line, how does another developer know what makes this resume sequence different than our startup sequence? And if they're the same is it possible to re-use or create a common startup sequence?
I know we have changes coming up for the sequencing and order of the node, mmproxy, and p2pool. I'll be worried we'll change it in one place and not the other. Maybe it's okay to have to know we need to make changes in both places but that returns to the question of what exactly makes them different.
There was a problem hiding this comment.
I don't like it that we have similar code for setup and resuming and would love to have some kind builder/manager which would store all logic related to setting up the app and starting processes but I didn't want to rewrite the setup function on this stage as any bug created there can stop people from entering the app.
As for what is different, resuming function cuts out everything related to starting process from setup function and also change events emitted, so we could with some work unify at least this part I think
src-tauri/src/utils/system_status.rs
Outdated
| } | ||
|
|
||
| pub fn recive_power_event(&self, power_monitor: &PowerMonitor) -> Result<(), Error> { | ||
| info!(target: LOG_TARGET, "Reciving power event"); |
There was a problem hiding this comment.
I believe this is the log line that is spamming the logs. Maybe we need some kind of debounce because I'm seeing it 50x a second. Which may mean this function is simply being called/polled way to often.
The fix here isn't just to remove the log line, but lets make sure we're not hamming the function to often and wasting unneeded cycles.
There was a problem hiding this comment.
This function is triggered when event loop receives new event, so its mainly spam when you move the app window around and I think it should not be any performance issue as its only checks the queue. Also I don't know if there is even fix for that as we cannot skip events ( like trigger if every 10 events ) as we can miss sleep mode entering moment, and I'm not sure if adding timeout will make the difference as we still will trigger it 50 times
|
Mostly it works really smooth. I tested on MacOS by putting the machine to sleep via the menu, and by closing the laptop lid. Upon waking up and entering my password the machine was already in the resume sequence 3/7. I tested with mining on when going to sleep and saw the miners resume on wake. Overall it was quite nice. I don't have a method to test the auto-update sequence while away at the moment. I am open to suggestions for attempting to test this. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src-tauri/src/main.rs (1)
1356-1362: 🛠️ Refactor suggestionAdd proper error handling for power monitor initialization.
The power monitor initialization logs errors but doesn't handle them effectively. Consider adding proper error handling to ensure the application can respond appropriately if power monitoring fails.
- let power_monitor = SystemStatus::current().start_listener(); - app.run(move |app_handle, event| { - // We can only receive system events from the event loop so this needs to be here - let _unused = SystemStatus::current().receive_power_event(&power_monitor).inspect_err(|e| { - error!(target: LOG_TARGET, "Could not receive power event: {:?}", e) - }); + match SystemStatus::current().start_listener() { + Ok(power_monitor) => { + app.run(move |app_handle, event| { + // We can only receive system events from the event loop so this needs to be here + if let Err(e) = SystemStatus::current().receive_power_event(&power_monitor) { + error!(target: LOG_TARGET, "Could not receive power event: {:?}", e); + }
🧹 Nitpick comments (3)
src-tauri/src/utils/shutdown_utils.rs (1)
139-387: Add timeout handling and cleanup for process resumption.The
resume_all_processesfunction has good error handling but lacks timeout protection and cleanup mechanisms. This could lead to the application hanging indefinitely if a process fails to start properly.Consider adding timeout handling for each process start operation and implementing cleanup for partially started processes on failure:
+use tokio::time::timeout; +const PROCESS_START_TIMEOUT: Duration = Duration::from_secs(30); // Example for one of the process starts: -state.wallet_manager.ensure_started(...).await?; +match timeout(PROCESS_START_TIMEOUT, state.wallet_manager.ensure_started(...)).await { + Ok(result) => result?, + Err(_) => { + error!(target: LOG_TARGET, "Wallet manager start timed out"); + // Clean up already started processes + stop_all_processes(app_handle.clone(), false).await?; + return Err(anyhow!("Wallet manager start timed out")); + } +}Also, consider extracting common emitting code into a helper function to reduce duplication.
src-tauri/src/utils/system_status.rs (2)
30-30: Fix incorrect log target name.The
LOG_TARGETconstant refers to "external_dependencies" which is incorrect for this file. Update it to reflect the current module.-const LOG_TARGET: &str = "tari::universe::external_dependencies"; +const LOG_TARGET: &str = "tari::universe::system_status";
59-83: Consider more fine-grained handling of power events.The current implementation treats any non-specific power event as equivalent to a "resume" event by setting sleep mode to false. This might lead to incorrect behavior in some edge cases.
Consider explicitly handling known events and ignoring unknown ones:
- _ => { - info!(target: LOG_TARGET, "Other event"); - self.sleep_mode_watcher_sender.send(false)?; - } + _ => { + info!(target: LOG_TARGET, "Unhandled power event: {:?}", event); + // Don't change sleep mode state for unknown events + }Also, consider reducing the verbosity of logs in the
receive_power_eventmethod, as it might be called frequently during normal operation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src-tauri/src/main.rs(6 hunks)src-tauri/src/updates_manager.rs(3 hunks)src-tauri/src/utils/shutdown_utils.rs(2 hunks)src-tauri/src/utils/system_status.rs(1 hunks)
🔇 Additional comments (2)
src-tauri/src/updates_manager.rs (2)
55-68: Well-structured payload implementation for update failure notifications.The new
CouldNotUpdatePayloadstruct provides a clean way to communicate update failures to the frontend when the device is in sleep mode or locked.
125-133: Good user experience improvement for handling updates during screen lock.The implementation correctly prevents updates when the screen is locked and informs the user through a notification. This addresses the issue where updates might cause problems when the system is in sleep mode.
| let app_handle_clone: tauri::AppHandle = app.clone(); | ||
| tauri::async_runtime::spawn(async move { | ||
| let mut receiver = SystemStatus::current().get_sleep_mode_watcher(); | ||
| let mut last_state = *receiver.borrow(); | ||
| loop { | ||
| if receiver.changed().await.is_ok() { | ||
| let current_state = *receiver.borrow(); | ||
|
|
||
| if last_state && !current_state { | ||
| info!(target: LOG_TARGET, "System is no longer in sleep mode"); | ||
| let _unused = resume_all_processes(app_handle_clone.clone()).await; | ||
| } | ||
|
|
||
| if !last_state && current_state { | ||
| info!(target: LOG_TARGET, "System entered sleep mode"); | ||
| let _unused = stop_all_processes(app_handle_clone.clone(), false).await; | ||
| } | ||
|
|
||
| last_state = current_state; | ||
| } else { | ||
| error!(target: LOG_TARGET, "Failed to receive sleep mode change"); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review the sleep mode handling logic.
While the implementation addresses the issue with the app restarting after sleep mode, there are a few potential improvements:
- The loop doesn't have an exit condition, so it will continue running even when the app is shutting down.
- There's no signal debouncing for sleep mode changes, which could lead to rapid toggling in edge cases.
- Consider preserving application state before stopping processes to ensure smooth resumption.
Based on the PR objectives, there's a reported issue where "after putting the machine into sleep mode, users were prompted to go through the setup view again." Let's check if any state persistence is implemented:
🏁 Script executed:
#!/bin/bash
# Description: Check if there's state preservation before stopping processes
# Search for any state saving before stopping processes
rg -A 5 "System entered sleep mode" --include="*.rs"Length of output: 496
Please address the sleep mode handling logic issues: add an exit condition, debouncing, and state preservation
The current implementation in src-tauri/src/main.rs (lines 905–928) raises a few concerns:
- Infinite Loop: The loop lacks any exit condition, meaning it will run indefinitely—even during application shutdown.
- Signal Debouncing: There’s no debouncing mechanism for sleep mode changes. In edge cases, rapid or spurious state transitions might lead to unintended toggling.
- State Preservation: There’s no indication that application state is being saved before processes are stopped. This gap could be why users are forced to go through the setup view again after wake.
To move forward, please verify if a state-preservation mechanism (such as saving critical application state before stopping processes) is implemented elsewhere in the codebase. If not, consider adding such a mechanism to ensure a seamless resumption. Similarly, implementing an exit condition for the loop (e.g., on app shutdown) and debouncing the sleep mode signals would be beneficial.
To further check for any state persistence logic in the codebase, please execute the following shell script:
Based on the outcome of this verification, you may need to adjust the implementation accordingly.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Re-run state preservation scan using a corrected syntax (using -g flag to search Rust files)
rg -A 5 "System entered sleep mode" -g "*.rs"Length of output: 554
Review sleep mode handling: add exit condition, signal debouncing, and state preservation
The sleep mode watcher code in src-tauri/src/main.rs (lines 905–928) still presents actionable issues:
- Infinite Loop: The loop lacks an exit condition, meaning it continues running even when the app is shutting down.
- Signal Debouncing: There is no debouncing mechanism, which could lead to rapid toggling if the sleep mode change triggers in quick succession.
- State Preservation: There is no mechanism to persist critical application state before stopping processes. This absence could be why users are forced through the setup view again upon wake.
Please address these points by:
- Implementing an exit condition (e.g., tying the loop's termination to an app shutdown signal).
- Adding debouncing logic to handle rapid or spurious sleep mode changes.
- Introducing a state-saving mechanism prior to stopping processes to ensure a smooth resumption.
| pub fn start_listener(&self) -> PowerMonitor { | ||
| let power_monitor = PowerMonitor::new(); | ||
| let _unused = power_monitor.start_listening().inspect_err(|e| { | ||
| error!(target: LOG_TARGET, "Failed to start power monitor: {:?}", e); | ||
| }); | ||
|
|
||
| power_monitor | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in power monitor initialization.
The start_listener method logs errors but doesn't propagate them, which prevents callers from properly handling initialization failures.
- pub fn start_listener(&self) -> PowerMonitor {
- let power_monitor = PowerMonitor::new();
- let _unused = power_monitor.start_listening().inspect_err(|e| {
- error!(target: LOG_TARGET, "Failed to start power monitor: {:?}", e);
- });
-
- power_monitor
+ pub fn start_listener(&self) -> Result<PowerMonitor, Error> {
+ let power_monitor = PowerMonitor::new();
+ power_monitor.start_listening()?;
+ Ok(power_monitor)Update the return type to properly propagate any errors to the caller.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn start_listener(&self) -> PowerMonitor { | |
| let power_monitor = PowerMonitor::new(); | |
| let _unused = power_monitor.start_listening().inspect_err(|e| { | |
| error!(target: LOG_TARGET, "Failed to start power monitor: {:?}", e); | |
| }); | |
| power_monitor | |
| } | |
| pub fn start_listener(&self) -> Result<PowerMonitor, Error> { | |
| let power_monitor = PowerMonitor::new(); | |
| power_monitor.start_listening()?; | |
| Ok(power_monitor) | |
| } |
Fixes #877
[ Summary ]
SystemStatusutility which listen to PowerStatus of device, for exampleScreenLocked,Suspend,ScreenUnlockedSuspendorScreenLockedstateSummary by CodeRabbit