Refactor FXIOS-15120 [Translations] Make leading page actions generic and decouple from ToolbarActions by razvanlitianu · Pull Request #33859 · mozilla-mobile/firefox-ios

razvanlitianu · 2026-05-18T08:37:19Z

📜 Tickets

💡 Description

Move 5 translation lifecycle cases (didStartTranslatingPage, translationCompleted, receivedTranslationLanguage, didReceiveErrorTranslating, didTranslationSettingsChange) off ToolbarActionType onto TranslationsActionType. Teach AddressBarState.reducer to observe TranslationsAction so the translate icon updates without forcing translation state changes through the toolbar reducer.

Collapse the toggleTranslationsEnabled double-dispatch in TranslationSettingsMiddleware into a single TranslationsAction. Both the toolbar reducer and the settings state reducer now react to the same action, removing the redux smell flagged in PR #32529.

Remove the in-code apology comment on ToolbarActionType.

🎥 Demos

Before	After

Demo

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If adding or modifying strings, I read the guidelines and will request a string review from l10n
If needed, I updated documentation and added comments to complex code

mobiletest-ci-bot · 2026-05-18T08:50:14Z

	Messages
📖	Project coverage: 42.26%

💪 Quality guardian

4 tests files modified. You're a champion of test coverage! 🚀

🧩 Neat Piece

This PR changes 578 lines. It's a substantial update,
but still review-friendly if there’s a clear description. Thanks for keeping things moving! 🚀

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

✅ New file code coverage

No new file detected so code coverage gate wasn't ran.

✅ Existing file code coverage

All modified files meet their thresholds.

Client.app: Coverage: 40.76

File	Coverage
AddressBarState.swift	96.25%	✅
TranslationSettingsMiddleware.swift	88.89%	✅
TranslationSettingsState.swift	83.45%	✅
TranslationsAction.swift	100.0%	✅
ToolbarState.swift	87.22%	✅
ToolbarAction.swift	100.0%	✅
TranslationsMiddleware.swift	88.24%	✅
TranslationSettingsViewController.swift	66.13%	✅
TranslationsConfiguration.swift	86.57%	✅

Generated by 🚫 Danger Swift against 4cab273

ih-codes

Hi @razvanlitianu ! I'm just taking a first pass on this one (haven't run the code). I have a few concerns about some of the refactors, my comments will explain a bit more. Mostly I'm not sure I'm a big fan of how some of the lower-level reducer helper methods are no longer specialized to a certain action type, but appear to only really do anything productive for specific action types. But correct me if I'm wrong on those on a case-by-case basis!

I will take another look after hearing your thoughts on this!

ih-codes · 2026-05-19T20:12:35Z

    @MainActor
    private static func leadingPageActions(
-        action: ToolbarAction,
+        action: Action,


Should this be specialized to a ToolbarAction? Or does this method really handle other action types? If it's the latter, in my opinion we should have a guard at the top of the method that clarifies the valid action types.

Added guard action is ToolbarAction || action is TranslationsAction else { return actions } at the top to explicitly document and enforce the valid action types handled by this path.

ih-codes · 2026-05-19T20:16:29Z

+    // Pulls the `translationConfiguration` payload off whichever action carries it.
+    // Both `ToolbarAction` (e.g. urlDidChange) and `TranslationsAction` (e.g. didStartTranslatingPage)
+    // can drive the leading translate icon, so the reducer reads the field generically.
+    private static func translationConfiguration(from action: Action) -> TranslationConfiguration? {
+        if let toolbarAction = action as? ToolbarAction { return toolbarAction.translationConfiguration }
+        if let translationsAction = action as? TranslationsAction { return translationsAction.translationConfiguration }
+        return nil
+    }
+


This logic is essentially duplicated here and in TranslationsMiddleware. 🤔

Can you think of a better way to organize this utility method? How about as an optional init for TranslationConfiguration itself?

Moved the extraction logic into TranslationConfiguration.init?(from action: Action) on the struct itself (TranslationsConfiguration.swift) to centralize the conversion logic. Removed the private helper from AddressBarState, as well as the identical unused/dead copy from TranslationsMiddleware.

ih-codes · 2026-05-19T20:17:38Z

    // MARK: - Helper
    @MainActor
-    private static func toolbarPosition(action: ToolbarAction) -> AddressToolbarPosition? {
+    private static func toolbarPosition(action: Action) -> AddressToolbarPosition? {


Similar comment here about specializing the Action... Do we need this change?

ih-codes · 2026-05-19T20:20:51Z


    @MainActor
-    private static func shouldUseAlternativeLocationColor(action: ToolbarAction) -> Bool {
+    private static func shouldUseAlternativeLocationColor(action: Action) -> Bool {


Another similar comment. It seems the caller should check the action type and we should leave these methods specialized instead of introducing additional checks if the only actions this function can process are ToolbarActions.

If the call sites get really messy I can understand doing this, perhaps. Can you demonstrate that, if that's the problem? In that case I hope we can add a guard here at the top showing this method really only handles the ToolbarActions.

Reverted both helpers back to ToolbarAction parameters. In leadingPageActions, the caller now casts to ToolbarAction? and only invokes the helpers when non-nil. For the TranslationsAction path, the alternative location color is computed inline from toolbarState. This also simplified the internal logic by removing redundant casts, e.g. (action as? ToolbarAction)?.toolbarPosition became action.toolbarPosition.

ih-codes · 2026-05-19T20:21:47Z

+        // Both `ToolbarAction` (lifecycle) and `TranslationsAction` (translation events) reach this
+        // handler — translation actions carry only `isTranslationsEnabled` + payload, so every other
+        // ToolbarAction-only field falls back to state when the action is not a `ToolbarAction`.
+        let toolbarAction = action as? ToolbarAction


Thanks for context 👍

ih-codes · 2026-05-19T20:26:07Z

+            return state.copyWithUpdates(
+                isTranslationsEnabled: action.isTranslationsEnabled ?? state.isTranslationsEnabled
+            )
+        }


Now that this reducer is supporting so many different action types, what do you think about writing it like this? 🤔

switch action.self { case is TranslationSettingsMiddlewareAction: // TODO... case is TranslationSettingsViewAction: // TODO... // etc... default: return defaultState(from: state) }

Alternatively, we could switch over the actionType instead, like the ToolbarAction reducer.

Just a thought...!

Restructured the logic from chained if let casts to a switch action { case let action as Type: ... } pattern, making the supported action types immediately visible and easier to scan.

ih-codes · 2026-05-19T20:28:51Z

    /// If auto-translate is enabled, triggers translation to the user's top preferred language.
    /// Returns `true` if auto-translation was initiated (caller should skip manual offer).
-    private func tryAutoTranslate(for action: ToolbarAction, on tab: Tab?) async -> Bool {
+    private func tryAutoTranslate(for action: Action, on tab: Tab?) async -> Bool {


Same gripe from me here too haha 😄

Narrowed the method signatures from Action to WindowUUID, since that’s the only value these paths actually use. This updated tryAutoTranslate, handleUpdatingTranslationIcon, retrieveTranslations, performTranslation, and handleErrorFromTranslatingPage to take windowUUID: WindowUUID instead of for action: Action.

ih-codes · 2026-05-19T20:40:07Z

Oh, one other thing. I remember chatting with @cyndichin about making the leading toolbar actions more generic at a higher level than this... My memory is a bit foggy but maybe she remembers more of this discussion. In any, case this PR is already large, but there may be more work we want to do in this area before we mark the parent ticket as completed, or perhaps we break this into 2 tickets. 🤔

mergify · 2026-05-20T12:34:36Z

This pull request has conflicts when rebasing. Could you fix it @razvanlitianu? 🙏

razvanlitianu · 2026-05-20T12:44:24Z

Oh, one other thing. I remember chatting with @cyndichin about making the leading toolbar actions more generic at a higher level than this... My memory is a bit foggy but maybe she remembers more of this discussion. In any, case this PR is already large, but there may be more work we want to do in this area before we mark the parent ticket as completed, or perhaps we break this into 2 tickets. 🤔

Thanks for the heads up, could you elaborate on what you mean by “more generic at a higher level”? Happy to break this into a follow-up ticket once I understand the intended direction.

… and decouple from ToolbarActions Move 5 translation lifecycle cases (didStartTranslatingPage, translationCompleted, receivedTranslationLanguage, didReceiveErrorTranslating, didTranslationSettingsChange) off ToolbarActionType onto TranslationsActionType. Teach AddressBarState.reducer to observe TranslationsAction so the translate icon updates without forcing translation state changes through the toolbar reducer. Collapse the toggleTranslationsEnabled double-dispatch in TranslationSettingsMiddleware into a single TranslationsAction. Both the toolbar reducer and the settings state reducer now react to the same action, removing the redux smell flagged in PR #32529. Remove the in-code apology comment on ToolbarActionType.

…uplication - Add guard for valid action types in leadingPageActions - Move translationConfiguration extraction to TranslationConfiguration.init?(from:) - Revert toolbarPosition and shouldUseAlternativeLocationColor to ToolbarAction - Restructure TranslationSettingsState reducer with switch pattern - Narrow tryAutoTranslate call chain from Action to WindowUUID

razvanlitianu requested review from cyndichin and ih-codes May 18, 2026 08:37

razvanlitianu marked this pull request as ready for review May 18, 2026 08:57

razvanlitianu requested a review from a team as a code owner May 18, 2026 08:57

ih-codes reviewed May 19, 2026

View reviewed changes

razvanlitianu added 3 commits May 20, 2026 15:44

Fix SwiftLint line length violations

4cab273

razvanlitianu force-pushed the rlitianu/translations-leading-page-actions-generic branch from 9fafe70 to 4cab273 Compare May 20, 2026 12:47

Conversation

razvanlitianu commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Tickets

💡 Description

🎥 Demos

📝 Checklist

Uh oh!

mobiletest-ci-bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💪 Quality guardian

🧩 Neat Piece

💬 Description craftsman

✅ New file code coverage

✅ Existing file code coverage

Client.app: Coverage: 40.76

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ih-codes commented May 19, 2026

Uh oh!

mergify Bot commented May 20, 2026

Uh oh!

razvanlitianu commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

razvanlitianu commented May 18, 2026 •

edited

Loading

mobiletest-ci-bot commented May 18, 2026 •

edited

Loading