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) <noreply@anthropic.com>

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
         )
     }
 

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

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(

app/src/main/java/no/naiv/implausibly/ui/sitepicker/SitePickerViewModel.kt

@@ -35,7 +35,14 @@ data class SitePickerUiState(
     val editSiteValue: String = "",
     val siteToDelete: Pair<String, String>? = 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,40 +115,19 @@ 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) {
+        _uiState.update {
+            it.updateGroup(instanceId) { group ->
                 val reordered = group.sites.toMutableList().apply {
                     add(toIndex, removeAt(fromIndex))
                 }
                 group.copy(sites = reordered)
-                    } else group
             }
-            )
         }
     }
 

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 = ?;