From 50edfcdafb473ac09a10ed296c932de26e24f616 Mon Sep 17 00:00:00 2001 From: Ole-Morten Duesund Date: Tue, 7 Apr 2026 12:51:17 +0200 Subject: [PATCH] refactor: fix correctness bugs and simplify site picker code - Rethrow CancellationException in goals query catch block to preserve structured concurrency - Replace non-atomic delete+insert in updateSite with single UPDATE statement to prevent data loss on crash or site ID collision - Add updateSiteId SQL query to StoredSite.sq - Extract updateGroup helper on SitePickerUiState to deduplicate 8 instances of the groups.map pattern (-66 lines) - Remove unnecessary WHAT comments in SitePickerScreen/ViewModel Co-Authored-By: Claude Opus 4.6 (1M context) --- .../data/repository/SiteRepository.kt | 9 +- .../data/repository/StatsRepository.kt | 2 + .../ui/sitepicker/SitePickerScreen.kt | 6 - .../ui/sitepicker/SitePickerViewModel.kt | 118 +++++------------- .../no/naiv/implausibly/StoredSite.sq | 3 + 5 files changed, 36 insertions(+), 102 deletions(-) diff --git a/app/src/main/java/no/naiv/implausibly/data/repository/SiteRepository.kt b/app/src/main/java/no/naiv/implausibly/data/repository/SiteRepository.kt index 1583084..34efb61 100644 --- a/app/src/main/java/no/naiv/implausibly/data/repository/SiteRepository.kt +++ b/app/src/main/java/no/naiv/implausibly/data/repository/SiteRepository.kt @@ -53,13 +53,10 @@ class SiteRepository @Inject constructor( } fun updateSite(oldSiteId: String, newSiteId: String, instanceId: String) { - val sites = getSitesForInstance(instanceId) - val oldSortOrder = sites.find { it.id == oldSiteId }?.sortOrder?.toLong() ?: 0L - database.storedSiteQueries.delete(site_id = oldSiteId, instance_id = instanceId) - database.storedSiteQueries.insert( + database.storedSiteQueries.updateSiteId( site_id = newSiteId, - instance_id = instanceId, - sort_order = oldSortOrder + site_id_ = oldSiteId, + instance_id = instanceId ) } diff --git a/app/src/main/java/no/naiv/implausibly/data/repository/StatsRepository.kt b/app/src/main/java/no/naiv/implausibly/data/repository/StatsRepository.kt index 494cc41..e02ff05 100644 --- a/app/src/main/java/no/naiv/implausibly/data/repository/StatsRepository.kt +++ b/app/src/main/java/no/naiv/implausibly/data/repository/StatsRepository.kt @@ -204,6 +204,8 @@ class StatsRepository @Inject constructor( pagination = Pagination(limit = 10) ) ) + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e } catch (_: Exception) { // Goals may not be configured — return null instead of failing the whole dashboard null diff --git a/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerScreen.kt b/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerScreen.kt index 87cebf2..78a5eb7 100644 --- a/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerScreen.kt +++ b/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerScreen.kt @@ -68,7 +68,6 @@ fun SitePickerScreen( ) { val uiState by viewModel.uiState.collectAsState() - // Site delete confirmation dialog uiState.siteToDelete?.let { (instanceId, siteId) -> AlertDialog( onDismissRequest = viewModel::dismissDeleteSite, @@ -87,7 +86,6 @@ fun SitePickerScreen( ) } - // Instance delete confirmation dialog uiState.instanceToDelete?.let { instance -> val group = uiState.groups.find { it.instance.id == instance.id } val siteCount = group?.sites?.size ?: 0 @@ -175,7 +173,6 @@ fun SitePickerScreen( ) } - // Expanded content: sites + add-site input if (group.isExpanded) { items( items = group.sites, @@ -216,7 +213,6 @@ fun SitePickerScreen( } } - // Inline add-site input with validation item(key = "add_${group.instance.id}") { AddSiteRow( value = group.newSiteId, @@ -230,13 +226,11 @@ fun SitePickerScreen( } } - // Spacer between instance groups item(key = "spacer_${group.instance.id}") { Spacer(modifier = Modifier.height(8.dp)) } } - // "Add new instance" button at bottom item(key = "add_instance") { Spacer(modifier = Modifier.height(8.dp)) OutlinedButton( diff --git a/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerViewModel.kt b/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerViewModel.kt index 514610e..0cdda49 100644 --- a/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerViewModel.kt +++ b/app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerViewModel.kt @@ -35,7 +35,14 @@ data class SitePickerUiState( val editSiteValue: String = "", val siteToDelete: Pair? = null, val instanceToDelete: PlausibleInstance? = null -) +) { + fun updateGroup( + instanceId: String, + transform: (InstanceWithSites) -> InstanceWithSites + ): SitePickerUiState = copy( + groups = groups.map { if (it.instance.id == instanceId) transform(it) else it } + ) +} @HiltViewModel class SitePickerViewModel @Inject constructor( @@ -79,27 +86,11 @@ class SitePickerViewModel @Inject constructor( } fun toggleExpanded(instanceId: String) { - _uiState.update { state -> - state.copy( - groups = state.groups.map { group -> - if (group.instance.id == instanceId) { - group.copy(isExpanded = !group.isExpanded) - } else group - } - ) - } + _uiState.update { it.updateGroup(instanceId) { g -> g.copy(isExpanded = !g.isExpanded) } } } fun onNewSiteIdChanged(instanceId: String, value: String) { - _uiState.update { state -> - state.copy( - groups = state.groups.map { group -> - if (group.instance.id == instanceId) { - group.copy(newSiteId = value, siteTestError = null) - } else group - } - ) - } + _uiState.update { it.updateGroup(instanceId) { g -> g.copy(newSiteId = value, siteTestError = null) } } } /** @@ -112,16 +103,7 @@ class SitePickerViewModel @Inject constructor( val siteId = group.newSiteId.trim() if (siteId.isBlank() || group.isTestingSite) return - // Set testing state - _uiState.update { state -> - state.copy( - groups = state.groups.map { g -> - if (g.instance.id == instanceId) { - g.copy(isTestingSite = true, siteTestError = null) - } else g - } - ) - } + _uiState.update { it.updateGroup(instanceId) { g -> g.copy(isTestingSite = true, siteTestError = null) } } viewModelScope.launch { val instance = group.instance @@ -133,41 +115,20 @@ class SitePickerViewModel @Inject constructor( ) if (error != null) { - // Validation failed — show error, don't add - _uiState.update { state -> - state.copy( - groups = state.groups.map { g -> - if (g.instance.id == instanceId) { - g.copy(isTestingSite = false, siteTestError = error) - } else g - } - ) - } + _uiState.update { it.updateGroup(instanceId) { g -> g.copy(isTestingSite = false, siteTestError = error) } } return@launch } - // Validation passed — add the site siteRepository.addSite(siteId, instanceId) val updatedSites = siteRepository.getSitesForInstance(instanceId) - _uiState.update { state -> - state.copy( - groups = state.groups.map { g -> - if (g.instance.id == instanceId) { - g.copy( - sites = updatedSites, - newSiteId = "", - isTestingSite = false, - siteTestError = null - ) - } else g - } - ) + _uiState.update { + it.updateGroup(instanceId) { g -> + g.copy(sites = updatedSites, newSiteId = "", isTestingSite = false, siteTestError = null) + } } } } - // -- Site deletion -- - fun requestDeleteSite(instanceId: String, siteId: String) { _uiState.update { it.copy(siteToDelete = instanceId to siteId) } } @@ -181,21 +142,13 @@ class SitePickerViewModel @Inject constructor( viewModelScope.launch { siteRepository.removeSite(siteId, instanceId) val updatedSites = siteRepository.getSitesForInstance(instanceId) - _uiState.update { state -> - state.copy( - groups = state.groups.map { g -> - if (g.instance.id == instanceId) { - g.copy(sites = updatedSites) - } else g - }, - siteToDelete = null - ) + _uiState.update { + it.updateGroup(instanceId) { g -> g.copy(sites = updatedSites) } + .copy(siteToDelete = null) } } } - // -- Instance deletion -- - fun requestDeleteInstance(instance: PlausibleInstance) { _uiState.update { it.copy(instanceToDelete = instance) } } @@ -228,8 +181,6 @@ class SitePickerViewModel @Inject constructor( } } - // -- Site editing -- - fun startEditing(siteId: String) { _uiState.update { it.copy(editingSiteId = siteId, editSiteValue = siteId) } } @@ -250,16 +201,9 @@ class SitePickerViewModel @Inject constructor( viewModelScope.launch { siteRepository.updateSite(oldId, newId, instanceId) val updatedSites = siteRepository.getSitesForInstance(instanceId) - _uiState.update { s -> - s.copy( - groups = s.groups.map { g -> - if (g.instance.id == instanceId) { - g.copy(sites = updatedSites) - } else g - }, - editingSiteId = null, - editSiteValue = "" - ) + _uiState.update { + it.updateGroup(instanceId) { g -> g.copy(sites = updatedSites) } + .copy(editingSiteId = null, editSiteValue = "") } } } @@ -268,24 +212,18 @@ class SitePickerViewModel @Inject constructor( _uiState.update { it.copy(editingSiteId = null, editSiteValue = "") } } - // -- Site reordering -- - /** * Move a site within an instance's list. Updates UI state immediately * for responsive drag feedback — persisted on drag end via saveSiteOrder. */ fun moveSite(instanceId: String, fromIndex: Int, toIndex: Int) { - _uiState.update { state -> - state.copy( - groups = state.groups.map { group -> - if (group.instance.id == instanceId) { - val reordered = group.sites.toMutableList().apply { - add(toIndex, removeAt(fromIndex)) - } - group.copy(sites = reordered) - } else group + _uiState.update { + it.updateGroup(instanceId) { group -> + val reordered = group.sites.toMutableList().apply { + add(toIndex, removeAt(fromIndex)) } - ) + group.copy(sites = reordered) + } } } diff --git a/app/src/main/sqldelight/no/naiv/implausibly/StoredSite.sq b/app/src/main/sqldelight/no/naiv/implausibly/StoredSite.sq index 8aaff31..ad1848b 100644 --- a/app/src/main/sqldelight/no/naiv/implausibly/StoredSite.sq +++ b/app/src/main/sqldelight/no/naiv/implausibly/StoredSite.sq @@ -27,3 +27,6 @@ UPDATE stored_sites SET sort_order = ? WHERE site_id = ? AND instance_id = ?; maxSortOrderForInstance: SELECT MAX(sort_order) AS max_order FROM stored_sites WHERE instance_id = ?; + +updateSiteId: +UPDATE stored_sites SET site_id = ? WHERE site_id = ? AND instance_id = ?;