fix(ui): eliminate race condition between refreshDevices and selectDevice

[ashishsinghbora]

Description

Fixes a race condition in FlasherViewModel.kt where an asynchronous refreshDevices() coroutine evaluating currentSelected before _uiState.update could overwrite a synchronously selected device with null. Moving selection evaluation inside the atomic _uiState.update block ensures newly selected or mock test devices are preserved.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 13 complexity · 2 duplication

Metric	Results
Complexity	13
Duplication	2

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

[github-actions]

🤖 FlashCore Autonomous AI PR Review

📋 Overview & Intent

This pull request aims to eliminate a race condition between device list refreshes (refreshDevices) and manual user device selection (selectDevice) in FlasherViewModel. It introduces isSameDevice() to accurately identify matching UsbDiskInfo instances across USB re-enumeration cycles and updates _uiState atomically. Additionally, it updates retry delays and candidate model configurations in the GitHub CI script workflows.

---

🚦 Verdict

CHANGES REQUESTED

While the addition of isSameDevice() and atomic state updates in _uiState.update is a great improvement, there is a race condition where fsm.transition() and _uiState.update() can diverge if selectDevice() is triggered concurrently during a device refresh.

---

🔍 Critical & High Priority Findings

1. FSM & UI State Desynchronization Race Condition

• Location: app/src/main/java/com/ashishsinghbora/flashcore/ui/FlasherViewModel.kt:228-247
• Issue: fsm.transition(FlasherEvent.DevicesUpdated(diskList, selected)) is called using a pre-calculated selected reference before _uiState.update executes. If a user calls selectDevice() concurrently while refreshDevices() is running:
  1. refreshDevices() evaluates selected as Device A.
  2. selectDevice() runs and updates _uiState.selectedDevice to Device B.
  3. refreshDevices() proceeds to call fsm.transition(FlasherEvent.DevicesUpdated(diskList, selected)) with Device A, updating the Finite State Machine to Device A.
  4. _uiState.update runs and sets _uiState.selectedDevice to Device B (via activeSelected), but records fsmState = fsm.state.value (which holds state for Device A).
• Impact: The UI state (_uiState.selectedDevice) and the internal state machine (fsm.state.value) become out of sync. Subsequent operation triggers (e.g., start flashing) will operate on mismatched USB device targets or invalidate safety checks in FlasherFSM.
• Suggested Fix: Resolve the active selected device atomically with the UI state update, transition fsm with the resolved target, and then sync fsmState:

// app/src/main/java/com/ashishsinghbora/flashcore/ui/FlasherViewModel.kt

_uiState.update { current ->
    val currentDev = current.selectedDevice
    val activeSelected = when {
        autoSelectDevice != null -> diskList.find { it.device == autoSelectDevice } ?: diskList.firstOrNull()
        currentDev != null && currentDev.device == null -> currentDev
        currentDev != null -> diskList.firstOrNull { isSameDevice(it, currentDev) } ?: diskList.firstOrNull()
        else -> diskList.firstOrNull()
    }

    current.copy(
        connectedDevices = diskList,
        selectedDevice = activeSelected
    )
}

val activeSelected = _uiState.value.selectedDevice
if (diskList.isNotEmpty() && activeSelected != null) {
    fsm.transition(FlasherEvent.DevicesUpdated(diskList, activeSelected))
} else if (_uiState.value.selectedDevice?.device != null) {
    fsm.transition(FlasherEvent.ResetToIdle)
}

_uiState.update { current ->
    current.copy(fsmState = fsm.state.value)
}

---

💡 Optimizations & Idiomatic Kotlin Suggestions

1. Top-Level or Private Helper Function:
   In FlasherViewModel.kt:210, fun isSameDevice is defined inside the method block. Moving isSameDevice to a private fun on the class level (or companion object) improves code readability and avoids re-allocating function instances during every refresh cycle.

2. CI Script Backoff Strategy:
   The exponential backoff update ((2 ** attempt) * 2 + 1) in pr_reviewer.py and repo_auditor.py is well-implemented and will reduce API 429/503 rate-limiting failures in GitHub Actions workflows.

---

Reviewed autonomously by gemini-3.6-flash via google-genai SDK.

[codacy-production]

Pull Request Overview

The pull request successfully addresses the race condition by moving device evaluation into the _uiState.update block, but the implementation introduces two high-risk issues that should prevent merging in its current state.

First, performing side effects like fsm.transition inside the update block is dangerous because the block may execute multiple times under contention, leading to duplicate or inconsistent state transitions. Second, a logic regression was introduced that prevents the State Machine from transitioning to ResetToIdle when hardware is disconnected if a selection was previously active. This violates the requirement for the UI to handle hardware removal gracefully.

While Codacy reports the PR as 'up to standards', these architectural and logic issues need to be resolved to ensure stability.

About this PR

• The revised logic may leave the State Machine in an inconsistent 'Ready' state even after hardware is disconnected, because the transition to ResetToIdle is now bypassed if a previous selection existed. This breaks the expected behavior for hardware detachment.

Test suggestions

• Verify that refreshDevices preserves a manually selected device if it is still present in the updated disk list.
• Verify that refreshDevices preserves a mock device (device == null) even if the hardware disk list is empty.
• Verify that the FSM transitions to ResetToIdle when the disk list becomes empty and no device was previously selected.
• Check that autoSelectDevice parameter takes precedence over existing selection logic within the atomic update block.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify that refreshDevices preserves a manually selected device if it is still present in the updated disk list.
2. Verify that refreshDevices preserves a mock device (device == null) even if the hardware disk list is empty.
3. Verify that the FSM transitions to ResetToIdle when the disk list becomes empty and no device was previously selected.
4. Check that autoSelectDevice parameter takes precedence over existing selection logic within the atomic update block.

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}

[codacy-production]

_{🔴 HIGH RISK}

This condition introduces a logic regression. By adding 'currentSelected == null', the FSM will no longer transition to ResetToIdle if the hardware is unplugged while a device was selected. If the device is no longer in the diskList, the FSM should be reset regardless of the previous selection state.

Suggested change

	} else if (selected == null && currentSelected == null) {
	fsm.transition(FlasherEvent.ResetToIdle)
	}
	} else {
	fsm.transition(FlasherEvent.ResetToIdle)
	}

[codacy-production]

_{🔴 HIGH RISK}

Side effects such as fsm.transition(...) must be kept outside of the _uiState.update block. Because update is implemented as a compare-and-set loop, this block can be executed multiple times if there is concurrent access. This would cause the FSM to process the same event multiple times, potentially leading to invalid state transitions. Refactor the logic to calculate the new state inside the block, but trigger the transition only once after the update completes.

[codacy-production]

_{⚪ LOW RISK}

Nitpick: The onCleared method should remain protected as defined in the ViewModel base class. Changing it to public is unrelated to the race condition fix and breaks encapsulation.

Suggested change

	public override fun onCleared() {
	protected override fun onCleared() {