Skip to content

Conversation

@loganrosen
Copy link
Contributor

Summary

As part of #5688, removing usage of runBlocking from SettingsActivity. This involved changing setAppActive into a suspend function and then making sure it's invoked from a coroutine.

Checklist

  • New or updated tests have been added to cover the changes following the testing guidelines.
  • The code follows the project's code style and best_practices.
  • The changes have been thoroughly tested, and edge cases have been considered.
  • Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Copilot AI review requested due to automatic review settings October 12, 2025 01:55
Copy link

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 removes the usage of runBlocking from SettingsActivity as part of issue #5688. The change converts the synchronous blocking call into proper asynchronous coroutine usage.

Key changes:

  • Converted setAppActive(serverId, active) from using runBlocking to a proper suspend function
  • Updated the call site in SettingsFragment to use lifecycleScope.launch instead of direct invocation
  • Removed the unnecessary runBlocking import

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
SettingsActivity.kt Converted setAppActive to suspend function and removed runBlocking usage
SettingsFragment.kt Updated call site to properly invoke the suspend function within a coroutine scope

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +548 to 553
(activity as? SettingsActivity)?.let { settingsActivity ->
lifecycleScope.launch {
settingsActivity.setAppActive(serverAuth, true)
}
}
parentFragmentManager.commit {
Copy link
Member

Choose a reason for hiding this comment

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

@jpelgrom @dshokouhi could you test if you see any issue on this change?

I did test on Android 9 old device, that is quite slow with 2 servers and 1 lock and it seems to be ok.

Copy link
Member

@jpelgrom jpelgrom Oct 13, 2025

Choose a reason for hiding this comment

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

The fragment's comment suggests a possible issue when toggling it on/off:

// Prevent requesting authentication after just enabling the app lock
presenter.setAppActive(true)

Now the code runs async so it might end up locking the app? Is that what you tested or just app lock on the main settings screen?

Copy link
Member

Choose a reason for hiding this comment

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

I simply tested app lock in both dashboard and settings

@jpelgrom
Copy link
Member

After thinking about it a bit more: by setting the active state in the lifecycleScope, we risk that the app is not set as inactive before the activity/fragment/app is quit, as the scope may be cancelled. That should never happen as it defeats the purpose of an app lock.

(This may also apply in other places, not only here)

@loganrosen
Copy link
Contributor Author

loganrosen commented Oct 16, 2025

After thinking about it a bit more: by setting the active state in the lifecycleScope, we risk that the app is not set as inactive before the activity/fragment/app is quit, as the scope may be cancelled. That should never happen as it defeats the purpose of an app lock.

(This may also apply in other places, not only here)

@jpelgrom Do we need to use an application-level scope here?

@jpelgrom
Copy link
Member

@jpelgrom Do we need to use an application-level scope here?

I missed that this PR was waiting for a response from me, sorry.

I'm tempted to say yes, but would like another opinion. Maybe I'm making this too big, and it would increase the scope of the PR. @TimoPtr what do you think?

@TimoPtr
Copy link
Member

TimoPtr commented Oct 27, 2025

@jpelgrom Do we need to use an application-level scope here?

I missed that this PR was waiting for a response from me, sorry.

I'm tempted to say yes, but would like another opinion. Maybe I'm making this too big, and it would increase the scope of the PR. @TimoPtr what do you think?

In general I'm not a big fan of application scope. It could be in a viewModel.

Now that I think a bit more about it, we could still have race condition if for some reason the localStorage is slow. We might need to make the IntegrationRepositoryImpl.appActive thread safe.

It could be made in a first PR before this one. Also I would like to have tests for such changes.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants