Skip to content

Conversation

@WaterWhisperer
Copy link

Description

Implements handling for app lifecycle events as requested in #458.

Changes

  • Added Event::Pause handler to save app state and stop sync service
  • Added Event::Resume/Event::Foreground handler to restore state and restart sync service
  • Added Event::Background handler for fallback state saving
  • Implemented redundancy tracking to avoid duplicate saves across lifecycle events

Testing

  • ✅ Code compiles without errors
  • ⚠️ Unable to test on iOS (no device available)

Fixes #458

@kevinaboos kevinaboos added the waiting-on-review This issue is waiting to be reviewed label Oct 19, 2025
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for making your first contribution here!

There are a few things missing; I left one comment but will explain the other more significant issues here.

  1. All of the app state — including window geom, AppState, and TSP-related state — should be saved or restored all at once, as a single indivisible entity. If you look through your current code flow, you're saving only select parts of those states in each event, and they're different across each event.

    • Instead, you should refactor all of the state-saving code into one function, and all of the state-restoring code into another function. Then, you should ensure that they are properly called from each of the event entry points as needed.
    • Similarly, if saving (or restoring) one part of the app state fails, you should continue on to saving (or restoring) the other parts as well, in a best-effort approach. An attempt to save the state should be treated the as if the state was actually saved, regardless of whether individual pieces of said state were successfully saved or not.
  2. Also, make sure that the pause+bg event path and the resume+fg path are true inverses of one another. We don't want to allow for a strange situation where the restore path on mobile differs from that on desktop.

  3. Different platforms will fire off different subsets of the lifecycle events. There are two basic "directions":
    a. Incoming: Startup --> Foreground --> Resume
    b. Outgoing: Pause --> Background --> Shutdown

    • The behavior of each platform is such that not every event in either directional sequence is guaranteed to be fired. For example, on Android, you are only guaranteed to get Resume and Pause, but the other ones like Shutdown may or may not happen. Similarly, on desktop platforms, you typically expect to receive Startup and Shutdown, but the other events are not guaranteed to occur.
    • In keeping with my comment in Number 1 above, we need to be sure to handle all of these cases by ensuring that the full save or full restore sequence can happen on any of those events, even if we only receive one of them.
    • Similarly, we need to ensure that we don't duplicate the save or restore operation if multiple events occur in a given direction (as described previously). Note that this must apply to both the incoming and the outgoing direction.
  4. Under the "Testing" heading, you wrote:

    ✅ Code compiles without errors
    ⚠️ Unable to test on iOS (no device available)

    While I appreciate that, neither of those qualify as testing. Please thoroughly test it on at least one platform, and ideally both one desktop and one mobile platform if you have them available to you. We wouldn't be able to accept the PR until it was tested thoroughly and verified to work.

src/app.rs Outdated
Comment on lines 144 to 145
/// Track whether app state has been saved during Pause to avoid redundant saves.
#[rust] state_saved_on_pause: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This boolean is more about whether or not the state has been saved since the last Resume sequence occurred (resume/foreground/etc). The goal is to avoid saving the app state multiple times in quick succession when it's not necessary to re-save it, i.e., when a resume/fg event did not occur since the last
  2. I don't see anywhere in the code where you reset this state to false, so it won't actually work like we want it to.
  3. Thus, it's probably be more appropriate to call this something like "was_state_changed_since_last_save", which makes the intent clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also need to avoid redundant restore operations too, not just redundant saves.

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I clicked submit too early. See my above comments.

src/app.rs Outdated
Comment on lines 144 to 145
/// Track whether app state has been saved during Pause to avoid redundant saves.
#[rust] state_saved_on_pause: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also need to avoid redundant restore operations too, not just redundant saves.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Oct 23, 2025
Copilot AI review requested due to automatic review settings November 7, 2025 09:17
@WaterWhisperer WaterWhisperer force-pushed the handle-app-lifecycle-events branch from 407288c to 660c6fc Compare November 7, 2025 09:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements comprehensive app lifecycle state management by adding save/restore functionality for app state, window geometry, and TSP wallet state across different lifecycle events (Pause, Background, Shutdown, Resume, Foreground).

Key Changes:

  • Added needs_save and needs_restore boolean flags to track state management requirements
  • Introduced new lifecycle event handling for Pause/Background/Shutdown and Foreground/Resume events
  • Refactored state save/restore logic into dedicated save_all_state() and restore_all_state() methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@WaterWhisperer WaterWhisperer force-pushed the handle-app-lifecycle-events branch from 660c6fc to 417cc27 Compare November 7, 2025 09:28
@WaterWhisperer
Copy link
Author

@kevinaboos Apologies for the delay in responding. Thank you for the detailed review and guidance.
Since this is my first time working with app lifecycle events, it took me some time to fully understand your feedback and refactor the code. I've tested this on my Ubuntu 22.04 laptop (window minimize/restore cycles, shutdown/restart) and verified that state persistence works. Unfortunately I don't have access to a suitable mobile device to test the mobile behavior. Please let me know if there's anything else that needs adjustment. Thanks again for the thorough review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-author This issue is waiting on the original author for a response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle app lifecycle events: Pause, Resume, etc

2 participants