Bug 2039662 - Added a preload on summary settings on summarization#256
Bug 2039662 - Added a preload on summary settings on summarization#256fmasalha wants to merge 1 commit into
Conversation
|
View this pull request in Lando to land it once approved. |
| ) | ||
| ) | ||
| } | ||
| LaunchedEffect(Unit) { settingsStore.dispatch(SettingsPreLoaded) } |
There was a problem hiding this comment.
I don't think this Action accurately describes an event that has occurred in the system - it seems more like we are wanting to start the process of pre-loading the settings.
Overall, this approach seems like it's reinforcing the code smell of having two stores here. I think the ideal situation we want is to allow the SummarizationStore to be able to react to ViewAppeared and have that result in an updated SettingsState that is delivered to the settings screen.
This could be a good discussion for team time tomorrow I think. I don't feel strongly that we have to address it as part of this patch, but I think this would be a good motivation for doing so.
As an aside, could you update the bug to better describe the problem we are trying to solve here rather than the steps of the solution? The problem is relatively clear from the code, but it would be good to have a digestible reference for future readers.
2 participants
No description provided.