Add FXiOS-15127 [AI Kill Switch] Delete models when translation is toggled off#32995
Add FXiOS-15127 [AI Kill Switch] Delete models when translation is toggled off#32995
Conversation
Cramsden
requested review from
razvanlitianu
and removed request for
razvanlitianu
💪 Quality guardian5 tests files modified. You're a champion of test coverage! 🚀 🥇 Perfect PR sizeSmaller PRs are easier to review. Thanks for making life easy for reviewers! ✨ 🧑💻 New
|
| File | Coverage | |
|---|---|---|
| TranslationSettingsMiddleware.swift | 88.46% | ✅ |
| TranslationSettingsAction.swift | 100.0% | ✅ |
| AIControlsSettingsView.swift | 0.0% | |
| ASTranslationModelsFetcher.swift | 70.98% | ✅ |
| AIControlsModel.swift | 100.0% | ✅ |
| SettingsCoordinator.swift | 82.32% | ✅ |
Generated by 🚫 Danger Swift against ea5b6f8
| prefs.setBool(false, forKey: PrefsKeys.Settings.translationsFeature) | ||
| prefs.setBool(false, forKey: PrefsKeys.Summarizer.summarizeContentFeature) | ||
| } | ||
| store.dispatch(TranslationSettingsViewAction( |
There was a problem hiding this comment.
I think we might have an edge case that breaks: user manually turns translations off (pref = false) → then turns kill switch ON → middleware reads pref (false) → sets to true. The pref ends up opposite to what the kill switch intended. In TranslationSettingsViewActionType.toggleTranslationsEnabled action let newValue = !current just flips whatever is currently in prefs.
There was a problem hiding this comment.
yep you are right. I found this logic a bit strange but was just trying to follow the existing pattern. I think it makes more sense to just pass in the bool value we expect to set it to. What do you think?
There was a problem hiding this comment.
Yes. That makes sense.
| /// Resets any local storage of models. | ||
| func resetStorage() async { | ||
| do { | ||
| try modelsClient?.resetStorage() |
There was a problem hiding this comment.
If modelsClient is nil, the reset is silently skipped, no log, no error. Is it worth at least a logger.log at warning level for the nil case so it's observable if it ever happens in the field?
adudenamedruby
left a comment
adudenamedruby
left a comment
There was a problem hiding this comment.
aside from @razvanlitianu 's comments, this is ok with me for the most part. I'll approve pending the resolution to those comments.
| isEditing: false, | ||
| pendingLanguages: nil, | ||
| preferredLanguages: [], | ||
| supportedLanguages: ["en", "fr", "de"] |
There was a problem hiding this comment.
I'm not familliar with translations, but, I feel like...... should this be hardcoded somewhere central that we can easily keep track of, and we get it from one place so further updates don't get missed out on? Or is this the one place?
There was a problem hiding this comment.
I guess I am still not sure where we land on test data.. I have always been in the camp that test data should be hard coded therefore if the code changes the test will catch it. This is not actually needed here so I can just remove it. I copied this from the middleware tests, but generally it is a discussion I would be interested in discussing.
4 participants
📜 Tickets
Jira ticket
Github issue
💡 Description
resetStoragewhen toggling off translations either via kill switch or via the toggle setting🎥 Demos
Demo
📝 Checklist