Honor skills#226
Closed
Goooler wants to merge 8 commits into
Closed
Conversation
Move asynchronous screen bootstrap work out of ViewModel init blocks and trigger it explicitly from screen-level LaunchedEffect so initialization stays lifecycle-bound and visible at the call site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace dispose-triggered Global scope writes with lifecycle observer callbacks and ViewModel-scoped persistence so settings are saved from onStop without escaping the screen lifecycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Guard coroutine error handling around suspend calls so CancellationException is rethrown instead of being logged and converted into ordinary failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move profile release out of onCleared Global scope work and perform it explicitly from active ViewModel flows before finishing the screen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace broad ViewModel effect keys with stable semantic keys so screen initialization and log loading effects restart only when their actual inputs change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rethrow CancellationException around suspend file operations so file management coroutines stop cleanly instead of reporting cancellation as an ordinary failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors several Android/Compose ViewModels to better align initialization and persistence with UI/lifecycle events (moving away from Global.launch patterns), and improves coroutine cancellation handling during error reporting.
Changes:
- Replace “persist on composable dispose” patterns with
DefaultLifecycleObserver(onStop) persistence in settings screens. - Add explicit
initialize()entrypoints for some ViewModels and trigger them from UI viaLaunchedEffect. - Ensure
CancellationExceptionis rethrown in severalcatch (Exception)blocks to avoid swallowing coroutine cancellation.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/settings/src/main/kotlin/com/github/kr328/clash/settings/vm/OverrideSettingsViewModel.kt | Moves initial load into initialize() and persists/clears override on lifecycle onStop. |
| ui/settings/src/main/kotlin/com/github/kr328/clash/settings/vm/MetaFeatureSettingsViewModel.kt | Same lifecycle-based init/persist refactor for meta feature overrides. |
| ui/settings/src/main/kotlin/com/github/kr328/clash/settings/vm/AccessControlViewModel.kt | Shifts initial load to initialize() and persists selection/restarts service on onStop. |
| ui/settings/src/main/kotlin/com/github/kr328/clash/settings/ui/OverrideSettingsScreen.kt | Calls viewModel.initialize() and attaches ViewModel as lifecycle observer. |
| ui/settings/src/main/kotlin/com/github/kr328/clash/settings/ui/MetaFeatureSettingsScreen.kt | Calls viewModel.initialize() and attaches ViewModel as lifecycle observer. |
| ui/settings/src/main/kotlin/com/github/kr328/clash/settings/ui/AccessControlScreen.kt | Calls viewModel.initialize() and attaches ViewModel as lifecycle observer. |
| ui/profile/src/main/kotlin/com/github/kr328/clash/profile/vm/ProvidersViewModel.kt | Rethrows CancellationException during provider update failure handling. |
| ui/profile/src/main/kotlin/com/github/kr328/clash/profile/vm/PropertiesViewModel.kt | Improves release/autosave flows and rethrows CancellationException in key catch blocks. |
| ui/profile/src/main/kotlin/com/github/kr328/clash/profile/vm/NewProfileViewModel.kt | Adds initialize() and rethrows CancellationException in profile creation flows. |
| ui/profile/src/main/kotlin/com/github/kr328/clash/profile/vm/FilesViewModel.kt | Rethrows CancellationException in file operations error handling. |
| ui/profile/src/main/kotlin/com/github/kr328/clash/profile/ui/NewProfileScreen.kt | Calls viewModel.initialize() from UI. |
| ui/log/src/main/kotlin/com/github/kr328/clash/log/vm/LogsViewModel.kt | Adds initialize() gating for initial log load. |
| ui/log/src/main/kotlin/com/github/kr328/clash/log/vm/LogcatViewModel.kt | Rethrows CancellationException in export/bind error paths. |
| ui/log/src/main/kotlin/com/github/kr328/clash/log/ui/LogsScreen.kt | Calls viewModel.initialize() to trigger initial log load. |
| ui/log/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt | Tweaks LaunchedEffect key for init(fileName). |
| ui/crash/src/main/kotlin/com/github/kr328/clash/crash/vm/AppCrashedViewModel.kt | Rethrows CancellationException while loading crash logs via runCatching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
33
| fun initialize() { | ||
| if (initialized) return | ||
| initialized = true | ||
|
|
||
| viewModelScope.launch { | ||
| configuration.value = withClash { queryOverride(Clash.OverrideSlot.Persist) } | ||
| } |
Comment on lines
+58
to
66
| fun initialize() { | ||
| if (initialized) return | ||
| initialized = true | ||
|
|
||
| viewModelScope.launch { | ||
| val selected = withContext(Dispatchers.IO) { serviceStore.accessControlPackages } | ||
| uiState.update { it.copy(selected = selected) } | ||
| reloadApps() | ||
| } |
Comment on lines
+36
to
42
| fun initialize() { | ||
| if (initialized) return | ||
| initialized = true | ||
|
|
||
| viewModelScope.launch { | ||
| configuration.value = withClash { queryOverride(Clash.OverrideSlot.Persist) } | ||
| } |
# Conflicts: # ui/settings/src/main/kotlin/com/github/kr328/clash/settings/ui/AccessControlScreen.kt # ui/settings/src/main/kotlin/com/github/kr328/clash/settings/ui/MetaFeatureSettingsScreen.kt # ui/settings/src/main/kotlin/com/github/kr328/clash/settings/ui/OverrideSettingsScreen.kt # ui/settings/src/main/kotlin/com/github/kr328/clash/settings/vm/AccessControlViewModel.kt # ui/settings/src/main/kotlin/com/github/kr328/clash/settings/vm/MetaFeatureSettingsViewModel.kt # ui/settings/src/main/kotlin/com/github/kr328/clash/settings/vm/OverrideSettingsViewModel.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://github.com/chrisbanes/skills