-
-
Notifications
You must be signed in to change notification settings - Fork 97
Rename account on background thread #1727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main-ose
Are you sure you want to change the base?
Rename account on background thread #1727
Conversation
ab8c8fd
to
12fa988
Compare
There was a problem hiding this 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 fixes an IllegalThreadStateException during account rename by ensuring background thread execution and adding proper thread annotations.
- Wraps
automaticSyncManager.updateAutomaticSync()
call inAccountRepository.rename()
withwithContext(defaultDispatcher)
to ensure background thread execution - Adds
@WorkerThread
annotations to methods that require background thread execution - Replaces hardcoded
Dispatchers.Default
with injected@DefaultDispatcher
for better testability and consistency
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
AccountRepository.kt | Main fix: wraps automatic sync update in background context, adds WorkerThread annotation to createBlocking method |
AutomaticSyncManager.kt | Adds WorkerThread annotations to sync-related methods |
TasksAppManager.kt | Adds WorkerThread annotation to selectProvider method |
LoginScreenModel.kt | Replaces hardcoded Dispatchers.Default with injected defaultDispatcher |
TasksModel.kt | Replaces hardcoded Dispatchers.Default with injected defaultDispatcher |
App.kt | Replaces hardcoded Dispatchers.Default with injected defaultDispatcher |
TestAccount.kt | Makes account name configurable for testing |
AccountRepositoryTest.kt | Comprehensive test suite for rename functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, also the tests and that more dispatchers are injected.
One comment
- add worker thread annotations - use injected defaultDispatcher
f596f39
to
ea15abc
Compare
Purpose
Fix IllegalThreadStateException during account rename.
Short description
The important change is in
AccountRepository.rename()
:but please also check the
@WorkerThread
annotations make sense to you. We could annotate the wholeAutomaticSyncManager
class, butdisableAutomaticSync()
does not need an annotation, so only used in on the methods.withContext
and injected default dispatcher.@WorkerThread
annotations where deemed appropriate@DefaultDispatcher
in for this issue relevant places of the app (not everywhere)testRename_checksForAlreadyExisting
andtestRename_renamesAccountInAndroid
. I think functionality of the other methods should be tested in their respective testclasses. If we were to test their functionality here in multiple test methods and additionally somewhere else we end up with a lot of dupliacated testing and tests.Checklist