Conversation
Goooler
force-pushed
the
unwrap-proxy-design
branch
3 times, most recently
from
01293ee to
c45e95c
Compare
Goooler
force-pushed
the
unwrap-proxy-design
branch
from
c45e95c to
c78c0dc
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the Proxy page to “unwrap” the old Design-based implementation into a Compose screen backed by a dedicated ViewModel (per #119).
Changes:
- Introduces
ProxyViewModelto own proxy UI state, events, and reload/selection operations. - Replaces
ProxyDesign/DesignActivityflow with a ComposeProxyScreenhosted byProxyActivity. - Removes the now-unused
ProxyStatemodel.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/src/main/kotlin/com/github/kr328/clash/proxy/vm/ProxyViewModel.kt | New ViewModel containing proxy state, event handling, and remote query/patch logic. |
| app/src/main/kotlin/com/github/kr328/clash/proxy/ui/ProxyScreen.kt | Converts Proxy UI to a ViewModel-driven Compose screen and removes the old ProxyDesign implementation. |
| app/src/main/kotlin/com/github/kr328/clash/proxy/ProxyActivity.kt | Switches activity hosting from DesignActivity to Compose setContent { ProxyScreen(...) }. |
| app/src/main/kotlin/com/github/kr328/clash/model/ProxyState.kt | Removes obsolete Compose state wrapper used by the previous design. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val sources = | ||
| withContext(Dispatchers.Default) { | ||
| group.proxies.map { proxy -> | ||
| UiState.ProxyItemSource( | ||
| proxy = proxy, | ||
| linkIndex = if (proxy.type.group) names.indexOf(proxy.name) else -1, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Building ProxyItemSource uses names.indexOf(proxy.name) for every group proxy. This makes source generation O(groupNames * proxies) per reload and can become noticeable with larger lists. Consider precomputing a Map<String, Int> (name→index) once per reload and doing O(1) lookups when setting linkIndex.
| override fun onStart(owner: LifecycleOwner) { | ||
| broadcastEventsJob?.cancel() | ||
| broadcastEventsJob = viewModelScope.launch { | ||
| Remote.broadcasts.event.collect { event -> | ||
| when (event) { | ||
| Broadcasts.Event.ProfileLoaded -> { | ||
| val newNames = withClash { queryProxyGroupNames(uiStore.proxyExcludeNotSelectable) } | ||
| if (newNames != uiState.value.groupNames) { | ||
| eventState.value = EventState.ReLaunch | ||
| } | ||
| } |
There was a problem hiding this comment.
ProfileLoaded handler compares newNames against uiState.value.groupNames, but groupNames is still empty until fetchInitialState() completes. If ProfileLoaded arrives early, this can spuriously set ReLaunch (potentially causing a relaunch loop). Consider fetching initial state before starting the broadcast collection, or guard/queue ProfileLoaded events until initialization has populated groupNames (even if empty).
| override fun onStart(owner: LifecycleOwner) { | ||
| broadcastEventsJob?.cancel() | ||
| broadcastEventsJob = viewModelScope.launch { | ||
| Remote.broadcasts.event.collect { event -> | ||
| when (event) { | ||
| Broadcasts.Event.ProfileLoaded -> { | ||
| val newNames = withClash { queryProxyGroupNames(uiStore.proxyExcludeNotSelectable) } | ||
| if (newNames != uiState.value.groupNames) { | ||
| eventState.value = EventState.ReLaunch | ||
| } | ||
| } | ||
| else -> Unit | ||
| } | ||
| } | ||
| } | ||
|
|
||
| viewModelScope.launch { fetchInitialState() } | ||
| } |
There was a problem hiding this comment.
fetchInitialState() is launched every onStart() without cancellation or an “already initialized” guard. Since it triggers reloadAll(), this can cause repeated expensive reloads (and possible overlapping loads) whenever the activity stops/starts. Consider tracking a fetchJob (cancel+replace) or skipping the initial fetch if state is already loaded, similar to MainViewModel.fetchJob (app/.../MainViewModel.kt:34,111-114).
Agent-Logs-Url: https://github.com/Goooler/MihomoForAndroid/sessions/d0627b29-1c66-489f-b1c5-f651fcae1870 Co-authored-by: Goooler <10363352+Goooler@users.noreply.github.com>
Refs #119.