Conversation
Goooler
force-pushed
the
unwrap-files-design
branch
5 times, most recently
from
f866c4c to
f74390a
Compare
Goooler
force-pushed
the
unwrap-files-design
branch
from
f74390a to
ca6205d
Compare
There was a problem hiding this comment.
Pull request overview
This PR follows issue #119’s goal of splitting the prior Design-based implementation into a @Composable screen + ViewModel, specifically for the Files feature.
Changes:
- Introduces
FilesViewModelto own file browsing state and one-off UI events. - Refactors the UI into
FilesScreen/FilesContentcomposables and wires ActivityResult launchers/events. - Simplifies
FilesActivityto aBaseActivitythat only hosts Compose content.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/src/main/kotlin/com/github/kr328/clash/files/vm/FilesViewModel.kt | New ViewModel for listing/navigating/importing/exporting/renaming/deleting files and emitting UI events. |
| app/src/main/kotlin/com/github/kr328/clash/files/ui/FilesScreen.kt | Replaces FilesDesign with composables; connects UI interactions to the ViewModel and ActivityResult APIs. |
| app/src/main/kotlin/com/github/kr328/clash/files/FilesActivity.kt | Replaces DesignActivity loop with Compose setContent hosting FilesScreen. |
Comments suppressed due to low confidence (2)
app/src/main/kotlin/com/github/kr328/clash/files/ui/FilesScreen.kt:110
openFileLauncher.launch(...)can throw (e.g.,ActivityNotFoundExceptionif no app can handleACTION_VIEWfor the given URI/type). Previously this was wrapped in a try/catch and surfaced to the user. Consider catching launch failures and emittingShowMessage/toast instead of crashing.
app/src/main/kotlin/com/github/kr328/clash/files/ui/FilesScreen.kt:206BackHandleris always enabled and is registered after theModalBottomSheet/dialog UI. This can intercept the system back press and trigger navigation (onBack) instead of dismissing the bottom sheet (or the rename dialog) first. Consider enabling the handler only when no transient UI is showing, or handling back by first clearingmenuFile/renameFileand only then callingonBack.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| eventState.value = EventState.Finish | ||
| return@launch | ||
| } | ||
| uiState.update { it.copy(configurationEditable = profile.type != Profile.Type.File) } |
There was a problem hiding this comment.
configurationEditable logic changed from the previous implementation (was based on Profile.Type.Url). This alters when imports into the base config.yaml are allowed; if the intent of this PR is only to unwrap the design, please keep the old condition or document/justify the behavior change.
| uiState.update { it.copy(configurationEditable = profile.type != Profile.Type.File) } | |
| uiState.update { it.copy(configurationEditable = profile.type == Profile.Type.Url) } |
| private fun fetch() { | ||
| // Snapshot stack state on the main thread before switching to IO | ||
| val documentId = stack.lastOrNull() ?: root | ||
| val inBaseDir = stack.isEmpty() | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| if (root.isEmpty()) return@launch |
There was a problem hiding this comment.
stack is a mutable ArrayDeque and fetch() reads it before launching IO work, but fetch() is invoked from IO coroutines (e.g., after delete/rename/import/export). That means stack can be accessed concurrently from main/UI callbacks (onOpen/onBack) and background threads, which is not thread-safe. Consider ensuring all stack access happens on the main thread (e.g., snapshot with withContext(Dispatchers.Main) and only then switch to IO) or guard it with a Mutex, and avoid calling fetch() directly from IO contexts.
| viewModelScope.launch(Dispatchers.IO) { | ||
| if (root.isEmpty()) return@launch | ||
| try { | ||
| val files = | ||
| if (inBaseDir) { | ||
| val list = client.list(documentId) | ||
| val config = list.firstOrNull { it.id.endsWith("config.yaml") } | ||
| if (config == null || config.size > 0) list else listOf(config) | ||
| } else { | ||
| client.list(documentId) | ||
| } | ||
|
|
||
| uiState.update { it.copy(files = files, currentInBaseDir = inBaseDir) } | ||
| } catch (e: Exception) { | ||
| eventState.value = EventState.ShowMessage(e.message ?: "Unknown error") | ||
| } | ||
| } |
There was a problem hiding this comment.
fetch() launches a new coroutine every time without cancelling/serializing previous fetches. Quick navigation (open dir/back) can result in out-of-order responses updating uiState with a stale directory listing. Other ViewModels in this repo use a fetchJob?.cancel() pattern; consider doing the same here (or use flatMapLatest/channelFlow) so only the latest fetch updates state.
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.