Skip to content

Fix review comments from PRs and cleanups#147

Merged
Goooler merged 9 commits intotrunkfrom
copilot/fix-review-comments-updates
Apr 27, 2026
Merged

Fix review comments from PRs and cleanups#147
Goooler merged 9 commits intotrunkfrom
copilot/fix-review-comments-updates

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 27, 2026

Addresses Copilot review feedback across three ViewModel refactoring PRs (#143, #121, #127).

ProxyViewModel (#143)

  • O(1) linkIndex lookup: precompute nameIndexMap via associate {} instead of indexOf per proxy (was O(groups × proxies))
  • Spurious ReLaunch on init: added @Volatile initialized flag set at the end of fetchInitialState(); ProfileLoaded handler early-returns until initialized
  • Overlapping initial fetches: track fetchInitialStateJob, cancel-and-replace on each onStart(), clear in onStop()/onCleared()

FilesViewModel + FilesActivity (#121)

  • configurationEditable regression: restore profile.type == Profile.Type.Url (was incorrectly changed to != Profile.Type.File)
  • stack thread-safety: onDelete/onRename/onImportResult/onExportResult now use viewModelScope.launch + withContext(Dispatchers.IO) so fetch() — and its stack reads — always run on Main
  • Stale directory listings: fetch() now cancels the previous fetchJob before launching a new one
  • Crash on missing UUID: FilesActivity replaces checkNotNull(intent.uuid) with a graceful early finish()

PropertiesViewModel (#127)

  • Infinite auto-save loop: onStop() patch now calls .onSuccess { } to update originalProfile and reset hasUnsavedChanges, preventing re-patching on every subsequent stop
  • GlobalScope usage: replace with Global.launch (structured app-level scope used elsewhere in the codebase)
  • Hard-coded error string: replace "Unknown error" with R.string.unknown for localization consistency

@Goooler Goooler requested a review from Copilot April 27, 2026 02:24
@Goooler Goooler changed the title Fix review comments from PRs #143, #121, #127 Fix review comments from PRs and cleanups Apr 27, 2026
Copy link
Copy Markdown

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 consolidates fixes responding to prior review feedback across multiple ViewModel refactor PRs, primarily around lifecycle/concurrency handling, background scoping, and a few UX/state correctness adjustments.

Changes:

  • Replace ad-hoc CoroutineScope(Dispatchers.IO) usages with the app-level Global scope for “persist on dispose” operations in multiple settings ViewModels.
  • Improve lifecycle behavior in ViewModels: cancel/replace overlapping jobs (e.g., files fetch, proxy initial fetch) and adjust state updates to prevent repeated auto-save.
  • Minor correctness/consistency fixes: restore configurationEditable logic, use AndroidViewModel.application for string/resources, and localize the fallback “unknown” error in Properties.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
app/src/main/kotlin/com/github/kr328/clash/settings/vm/OverrideSettingsViewModel.kt Switch persist/reset override operations to Global.launch for non-viewModelScope work.
app/src/main/kotlin/com/github/kr328/clash/settings/vm/MetaFeatureSettingsViewModel.kt Same Global.launch scoping change for persist/reset override operations.
app/src/main/kotlin/com/github/kr328/clash/settings/vm/AppSettingsViewModel.kt Use AndroidViewModel.application property for component/icon operations.
app/src/main/kotlin/com/github/kr328/clash/settings/vm/AccessControlViewModel.kt Use Global.launch for persist-on-dispose selection persistence/restart flow.
app/src/main/kotlin/com/github/kr328/clash/proxy/vm/ProxyViewModel.kt Add initial-state job tracking and an initialized gate to avoid early ProfileLoaded handling; cancel/clear job on stop/clear.
app/src/main/kotlin/com/github/kr328/clash/profile/vm/PropertiesViewModel.kt Prevent repeated auto-save by updating originalProfile on success; replace GlobalScope with Global; localize unknown-error fallback; use application for strings.
app/src/main/kotlin/com/github/kr328/clash/files/vm/FilesViewModel.kt Restore configurationEditable condition; add cancel/replace for fetch job; move file operations to main-launched coroutines while keeping stack access on main.
Comments suppressed due to low confidence (1)

app/src/main/kotlin/com/github/kr328/clash/files/vm/FilesViewModel.kt:156

  • fetch() cancels the previous fetchJob, but the launched coroutine catches all Exceptions. When a job is canceled, it will typically throw CancellationException, which will be caught here and surfaced as an "Unknown error" message (and potentially overwrite a newer fetch result). Handle cancellation explicitly (rethrow/return on CancellationException) so normal cancel-and-replace doesn’t produce user-visible errors.
  private fun fetch() {
    fetchJob?.cancel()
    val documentId = stack.lastOrNull() ?: root
    if (root.isEmpty()) return
    val inBaseDir = stack.isEmpty()
    fetchJob = viewModelScope.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")
      }
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Goooler Goooler merged commit 9309968 into trunk Apr 27, 2026
2 checks passed
@Goooler Goooler deleted the copilot/fix-review-comments-updates branch April 27, 2026 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants