Skip to content

[MBL-19845][All] Fix inbox attachment upload UI stuck on screen rotation #3606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kristofnemere wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[MBL-19845][All] Fix inbox attachment upload UI stuck on screen rotation #3606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb787dd
[MBL-19845][All] Fix inbox attachment upload UI stuck on screen rotation
kristofnemere Mar 25, 2026
e9c7af9
Restore comment on context.packageName mock in test setup
kristofnemere Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions ...s/src/main/java/com/instructure/pandautils/features/inbox/compose/InboxComposeFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,7 @@ class InboxComposeFragment : BaseCanvasFragment(), FragmentInteractions, FileUpl
}

override fun workInfoLiveDataCallback(uuid: UUID?, workInfoLiveData: LiveData<WorkInfo?>) {
workInfoLiveData.observe(viewLifecycleOwner) { workInfo ->
workInfo?.let {
viewModel.updateAttachments(uuid, workInfo)
}
}
uuid?.let { viewModel.onWorkStarted(it) }
}

private fun handleAction(action: InboxComposeViewModelAction) {
Expand Down
14 changes: 12 additions & 2 deletions .../src/main/java/com/instructure/pandautils/features/inbox/compose/InboxComposeViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.work.WorkInfo
import androidx.work.WorkManager
import com.instructure.canvasapi2.models.CanvasContext
import com.instructure.canvasapi2.models.Course
import com.instructure.canvasapi2.models.Recipient
Expand Down Expand Up @@ -62,7 +63,8 @@ class InboxComposeViewModel @Inject constructor(
private val inboxComposeRepository: InboxComposeRepository,
private val attachmentDao: AttachmentDao,
private val featureFlagProvider: FeatureFlagProvider,
private val inboxComposeBehavior: InboxComposeBehavior
private val inboxComposeBehavior: InboxComposeBehavior,
private val workManager: WorkManager
): ViewModel() {
private var canSendToAll = false

Expand Down Expand Up @@ -226,7 +228,15 @@ class InboxComposeViewModel @Inject constructor(
_uiState.update { it.copy(attachments = it.attachments + placeholderAttachments) }
}

fun updateAttachments(uuid: UUID?, workInfo: WorkInfo) {
fun onWorkStarted(uuid: UUID) {
viewModelScope.launch {
workManager.getWorkInfoByIdFlow(uuid).collect { workInfo ->
workInfo?.let { updateAttachments(uuid, it) }
}
}
}

private fun updateAttachments(uuid: UUID?, workInfo: WorkInfo) {
viewModelScope.launch {
uuid?.let { workerId ->
val status = workInfo.state.toAttachmentCardStatus()
Expand Down
109 changes: 81 additions & 28 deletions .../test/java/com/instructure/pandautils/features/inbox/compose/InboxComposeViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import android.content.Context
import androidx.compose.ui.text.input.TextFieldValue
import androidx.lifecycle.SavedStateHandle
import androidx.work.WorkInfo
import androidx.work.WorkManager
import com.instructure.canvasapi2.models.Attachment
import com.instructure.canvasapi2.models.CanvasContext
import com.instructure.canvasapi2.models.Conversation
Expand Down Expand Up @@ -50,6 +51,8 @@ import junit.framework.Assert.assertEquals
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.toList
import kotlinx.coroutines.launch
import kotlinx.coroutines.test.UnconfinedTestDispatcher
Expand All @@ -69,6 +72,7 @@ class InboxComposeViewModelTest {
private val attachmentDao: AttachmentDao = mockk(relaxed = true)
private val featureFlagProvider: FeatureFlagProvider = mockk(relaxed = true)
private val inboxComposeBehavior: InboxComposeBehavior = mockk(relaxed = true)
private val workManager: WorkManager = mockk(relaxed = true)

@Before
fun setup() {
Expand Down Expand Up @@ -502,7 +506,8 @@ class InboxComposeViewModelTest {
val attachmentCardItem = AttachmentCardItem(attachment, AttachmentStatus.UPLOADED, false)
val uuid = UUID.randomUUID()
coEvery { attachmentDao.findByParentId(uuid.toString()) } returns listOf(attachmentEntity)
viewmodel.updateAttachments(uuid, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid)

assertEquals(1, viewmodel.uiState.value.attachments.size)

Expand Down Expand Up @@ -575,7 +580,8 @@ class InboxComposeViewModelTest {
assertEquals(false, viewmodel.uiState.value.isSendButtonEnabled)

// Complete upload - replaces placeholder with real attachment
viewmodel.updateAttachments(uuid, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid)
assertEquals(true, viewmodel.uiState.value.isSendButtonEnabled)
}

Expand All @@ -593,7 +599,8 @@ class InboxComposeViewModelTest {
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments.first().status)

// Complete upload - replaces placeholder
viewmodel.updateAttachments(uuid, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid)
assertEquals(AttachmentStatus.UPLOADED, viewmodel.uiState.value.attachments.first().status)
}

Expand All @@ -606,7 +613,9 @@ class InboxComposeViewModelTest {
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments.first().status)

// Upload fails - updates placeholder status
viewmodel.updateAttachments(UUID.randomUUID(), WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
val uuid = UUID.randomUUID()
coEvery { workManager.getWorkInfoByIdFlow(uuid) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
viewmodel.onWorkStarted(uuid)
assertEquals(AttachmentStatus.FAILED, viewmodel.uiState.value.attachments.first().status)
}

Expand Down Expand Up @@ -637,15 +646,17 @@ class InboxComposeViewModelTest {
assertEquals(false, viewmodel.uiState.value.isSendButtonEnabled)

// First upload completes - replaces first placeholder
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid1)

// Send button still disabled (second placeholder still uploading)
assertEquals(false, viewmodel.uiState.value.isSendButtonEnabled)
assertEquals(1, viewmodel.uiState.value.attachments.count { it.status == AttachmentStatus.UPLOADED })
assertEquals(1, viewmodel.uiState.value.attachments.count { it.status == AttachmentStatus.UPLOADING })

// Second upload completes
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid2)

// Now send button should be enabled
assertEquals(true, viewmodel.uiState.value.isSendButtonEnabled)
Expand All @@ -661,8 +672,12 @@ class InboxComposeViewModelTest {
assertEquals(1, viewmodel.uiState.value.attachments.size)

// Multiple state updates should not add duplicates (placeholders already added)
viewmodel.updateAttachments(UUID.randomUUID(), WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.updateAttachments(UUID.randomUUID(), WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
val uuid1 = UUID.randomUUID()
val uuid2 = UUID.randomUUID()
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.onWorkStarted(uuid1)
viewmodel.onWorkStarted(uuid2)

// Should still only have one attachment
assertEquals(1, viewmodel.uiState.value.attachments.size)
Expand All @@ -686,23 +701,27 @@ class InboxComposeViewModelTest {
viewmodel.addUploadingAttachments(listOf("/storage/test2.pdf"))

// Assign workerIds to placeholders
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.onWorkStarted(uuid1)
viewmodel.onWorkStarted(uuid2)

assertEquals(2, viewmodel.uiState.value.attachments.size)
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments[0].status)
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments[1].status)

// First upload FAILS
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
viewmodel.onWorkStarted(uuid1)

// Verify: First attachment is FAILED, second is still UPLOADING
assertEquals(2, viewmodel.uiState.value.attachments.size)
assertEquals(AttachmentStatus.FAILED, viewmodel.uiState.value.attachments.first { it.workerId == uuid1.toString() }.status)
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments.first { it.workerId == uuid2.toString() }.status)

// Second upload SUCCEEDS
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid2)

// Verify: First attachment still FAILED, second is UPLOADED
assertEquals(2, viewmodel.uiState.value.attachments.size)
Expand All @@ -721,20 +740,24 @@ class InboxComposeViewModelTest {
viewmodel.addUploadingAttachments(listOf("/storage/test2.pdf"))

// Assign workerIds to placeholders
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.onWorkStarted(uuid1)
viewmodel.onWorkStarted(uuid2)

assertEquals(2, viewmodel.uiState.value.attachments.size)

// First upload fails
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
viewmodel.onWorkStarted(uuid1)

// Verify: Only first attachment is FAILED, second is still UPLOADING
assertEquals(AttachmentStatus.FAILED, viewmodel.uiState.value.attachments.first { it.workerId == uuid1.toString() }.status)
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments.first { it.workerId == uuid2.toString() }.status)

// Second upload also fails
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.FAILED, setOf("")))
viewmodel.onWorkStarted(uuid2)

// Verify: Both attachments are FAILED
assertEquals(2, viewmodel.uiState.value.attachments.size)
Expand All @@ -760,21 +783,25 @@ class InboxComposeViewModelTest {
viewmodel.addUploadingAttachments(listOf("/storage/test2.pdf"))

// Assign workerIds
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.onWorkStarted(uuid1)
viewmodel.onWorkStarted(uuid2)

assertEquals(2, viewmodel.uiState.value.attachments.size)

// First upload succeeds
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid1)

// Verify: First replaced with real attachment, second still uploading
assertEquals(2, viewmodel.uiState.value.attachments.size)
assertEquals(1, viewmodel.uiState.value.attachments.count { it.status == AttachmentStatus.UPLOADED })
assertEquals(1, viewmodel.uiState.value.attachments.count { it.status == AttachmentStatus.UPLOADING })

// Second upload succeeds
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
viewmodel.onWorkStarted(uuid2)

// Verify: Both replaced with real attachments
assertEquals(2, viewmodel.uiState.value.attachments.size)
Expand All @@ -793,8 +820,10 @@ class InboxComposeViewModelTest {
viewmodel.addUploadingAttachments(listOf("/storage/test1.pdf", "/storage/test2.pdf"))

// Assign workerIds
viewmodel.updateAttachments(uuid1, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.updateAttachments(uuid2, WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid1) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
coEvery { workManager.getWorkInfoByIdFlow(uuid2) } returns flowOf(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
viewmodel.onWorkStarted(uuid1)
viewmodel.onWorkStarted(uuid2)

// Both should have workerIds assigned
assertEquals(2, viewmodel.uiState.value.attachments.size)
Expand All @@ -806,6 +835,30 @@ class InboxComposeViewModelTest {
assertEquals(AttachmentStatus.UPLOADING, viewmodel.uiState.value.attachments[1].status)
}

@Test
fun `onWorkStarted survives rotation - processes all work states without re-subscription`() = runTest {
val viewmodel = getViewModel()
val uuid = UUID.randomUUID()
val attachment = Attachment(id = 1, displayName = "test.pdf")
val attachmentEntity = com.instructure.pandautils.room.appdatabase.entities.AttachmentEntity(attachment)

coEvery { attachmentDao.findByParentId(uuid.toString()) } returns listOf(attachmentEntity)

// Simulate Fragment calling addUploadingAttachments and onWorkStarted before rotation
viewmodel.addUploadingAttachments(listOf("/storage/test.pdf"))

coEvery { workManager.getWorkInfoByIdFlow(uuid) } returns flow {
emit(WorkInfo(UUID.randomUUID(), WorkInfo.State.RUNNING, setOf("")))
emit(WorkInfo(UUID.randomUUID(), WorkInfo.State.SUCCEEDED, setOf("")))
}

// Fragment calls onWorkStarted once before rotation — viewModelScope keeps the flow alive
viewmodel.onWorkStarted(uuid)

// Both RUNNING and SUCCEEDED states processed without re-subscription after "rotation"
assertEquals(AttachmentStatus.UPLOADED, viewmodel.uiState.value.attachments.first().status)
}

@Test
fun `Download attachment on selection`() {
val fileDownloader: FileDownloader = mockk(relaxed = true)
Expand Down Expand Up @@ -1032,7 +1085,7 @@ class InboxComposeViewModelTest {
attachments = attachments
)
)
val viewmodel = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior)
val viewmodel = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior, workManager)
val uiState = viewmodel.uiState.value

assertEquals(mode, uiState.inboxComposeMode)
Expand Down Expand Up @@ -1060,7 +1113,7 @@ class InboxComposeViewModelTest {
isAttachmentDisabled = true
)
)
val viewmodel = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior)
val viewmodel = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior, workManager)
val disabledFields = viewmodel.uiState.value.disabledFields

assertEquals(true, disabledFields.isContextDisabled)
Expand All @@ -1085,7 +1138,7 @@ class InboxComposeViewModelTest {
isAttachmentHidden = true
)
)
val viewmodel = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior)
val viewmodel = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior, workManager)
val hiddenFields = viewmodel.uiState.value.hiddenFields

assertEquals(true, hiddenFields.isContextHidden)
Expand Down Expand Up @@ -1271,7 +1324,7 @@ class InboxComposeViewModelTest {
)
)

val viewmodelWithReply = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior)
val viewmodelWithReply = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior, workManager)

val events = mutableListOf<InboxComposeViewModelAction>()
backgroundScope.launch(testDispatcher) {
Expand Down Expand Up @@ -1307,7 +1360,7 @@ class InboxComposeViewModelTest {
)
)

val viewmodelWithReply = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior)
val viewmodelWithReply = InboxComposeViewModel(savedStateHandle, context, mockk(relaxed = true), inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior, workManager)

val events = mutableListOf<InboxComposeViewModelAction>()
backgroundScope.launch(testDispatcher) {
Expand Down Expand Up @@ -1392,6 +1445,6 @@ class InboxComposeViewModelTest {
// endregion

private fun getViewModel(fileDownloader: FileDownloader = mockk(relaxed = true)): InboxComposeViewModel {
return InboxComposeViewModel(SavedStateHandle(), context, fileDownloader, inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior)
return InboxComposeViewModel(SavedStateHandle(), context, fileDownloader, inboxComposeRepository, attachmentDao, featureFlagProvider, inboxComposeBehavior, workManager)
}
}
Loading