Bug 1876567 - Fix Middleware Issue with Processing Translations States #5302
Conversation
| is TranslationsAction.TranslateAction -> translate(context.store, next, action) | ||
| is TranslationsAction.TranslateRestoreAction -> translateRestoreOriginal(context.store, next, action) |
There was a problem hiding this comment.
Could we reorder these TranslationsAction to be after the EngineAction? Just nice to keep all the EngineAction altogether.
| ), | ||
| ) | ||
|
|
||
| assertEquals(false, store.state.findTab(tab.id)?.translationsState?.isTranslateProcessing) |
There was a problem hiding this comment.
Use assertFalse. Same thing below
There was a problem hiding this comment.
Updated, is it okay to force unwrap in a test?
There was a problem hiding this comment.
Definitely. The test will just fail anyway if it crashes
| store.waitUntilIdle() | ||
|
|
||
| verify(engineSession).requestTranslate(any(), any(), any()) | ||
| assertEquals(true, store.state.findTab(tab.id)?.translationsState?.isTranslateProcessing) |
There was a problem hiding this comment.
Use assertTrue. Same thing below
| is TranslationsAction.TranslateAction -> translate(context.store, action) | ||
| is TranslationsAction.TranslateRestoreAction -> translateRestoreOriginal(context.store, action) | ||
| is TranslationsAction.TranslateAction -> translate(context.store, next, action) | ||
| is TranslationsAction.TranslateRestoreAction -> translateRestoreOriginal(context.store, next, action) | ||
| is EngineAction.ClearDataAction -> clearData(context.store, action) | ||
| is EngineAction.PurgeHistoryAction -> purgeHistory(context.state) | ||
| else -> next(action) |
There was a problem hiding this comment.
It's possible that this else should just be a no-op and we should always just be calling next(action). What do you think @MatthewTighe?
We don't have to make this change here in this PR, but might be worth a follow up bug.
There was a problem hiding this comment.
It looks generally like this Middleware intercepts actions and then dispatches a different action to complete the reducer chain. Since this is pre-existing, core browser functionality I would hesitate to update the existing code without an investigation.
Visually, it might make the distinction between handling more clear and quickly communicated to readers to write the translate actions like:
is TranslationsAction.TranslateAction -> {
next(action)
translate(context.store, action)
}
is TranslationsAction.TranslateRestoreAction -> {
next(action)
translateRestoreOriginal(context.store, action)
}
One other thing to note is that the context.store reference in Middleware::invoke will actually update as a result of the reducer chain, so the state will be different after calling next. If you want the original state, you can capture it before calling next or handling the middleware work first (if it won't slow the rest of the chain noticeably)
For example:
is TranslationsAction.TranslateAction -> {
val currentState = context.store.state
next(action)
translate(currentState, action)
}
// or
is TranslationsAction.TranslateRestoreAction -> {
translateRestoreOriginal(context.store, action)
next(action)
}
There was a problem hiding this comment.
+1 Definitely worth a followup investigation for adding the next(action) outside of the else. No action needed here in the PR, but let's file a followup bug to keep track of this and link the description to the comments here.
+1 to add the next(action) inside of the is TranslationsAction.TranslateAction -> {} instead of the translate() function.
There was a problem hiding this comment.
One other thing to note is that the context.store reference in Middleware::invoke will actually update as a result of the reducer chain, so the state will be different after calling next. If you want the original state, you can capture it before calling next or handling the middleware work first (if it won't slow the rest of the chain noticeably)
This is my intention:
- action(next) switches in inProgress on the store
- translate/restore will remove inProgress when complete
I think I probably want translate/restore to use the updated/most current store, is that right?
Made bug 1876598. Please add to the bug or let me know if I misunderstood anything in the filing.
This patch corrects an issue where `isTranslateProcessing` and `isRestoreProcessing` states were not updating due to not sending along pre-process actions in `EngineDelegateMiddleware`.
github-actions
bot
added
approved
This patch corrects an issue where
isTranslateProcessingandisRestoreProcessingstates were not updating due to not sending along pre-process actions inEngineDelegateMiddleware.Pull Request checklist
After merge
To download an APK when reviewing a PR (after all CI tasks finished running):
Checksat the top of the PR page.firefoxci-taskclustergroup on the left to expand all tasks.build-apk-{fenix,focus,klar}-debugtask you're interested in.View task in Taskclusterin the newDETAILSsection.GitHub Automation
https://bugzilla.mozilla.org/show_bug.cgi?id=1876567