Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #31
+65
−970
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis pull request comprehensively removes the inactive tabs feature from Firefox iOS by deleting related models, UI components, accessibility identifiers, action handlers, middleware logic, state management, and associated tests across the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
171-179: Scroll targets an invalid section after inactive-tabs removal.The tab list is now a single section at index 0. Using section 1 makes the index path invalid, so scrolling never happens.
🛠️ Proposed fix
- let indexPath = IndexPath(row: scrollState.toIndex, section: 1) + let indexPath = IndexPath(row: scrollState.toIndex, section: 0)
🤖 Fix all issues with AI agents
In `@firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift`:
- Around line 130-148: The off-by-one occurs in createTabScrollBehavior when
returning ScrollState(toIndex: state.tabs.count, ...), which is out of bounds;
change the fallback to use the last valid index (state.tabs.count - 1) and
ensure this branch only executes when state.tabs is non-empty (the existing
guard !state.tabs.isEmpty already covers this), so replace the toIndex value
with state.tabs.count - 1 in the ScrollState returned by that else block to
avoid invalid IndexPath errors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
firefox-ios/Client.xcodeproj/project.pbxprojfirefox-ios/Client/Application/AccessibilityIdentifiers.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Action/TabPanelAction.swiftfirefox-ios/Client/Frontend/Browser/Tabs/LayoutManager/InactiveTabsSectionManager.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Models/InactiveTabsModel.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Models/TabDisplayModel.swiftfirefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Views/Animation/TabAnimation.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsCell.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsFooterView.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsHeaderView.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayDiffableDataSource.swiftfirefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swiftfirefox-ios/Client/Frontend/Browser/ToastType.swiftfirefox-ios/Shared/Strings.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayDiffableDataSourceTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayPanelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swiftfirefox-ios/firefox-ios-tests/Tests/XCUITests/FxScreenGraph.swiftfirefox-ios/firefox-ios-tests/Tests/XCUITests/TabsTests.swiftfirefox-ios/firefox-ios-tests/Tests/XCUITests/registerSettingsNavigation.swift
💤 Files with no reviewable changes (12)
- firefox-ios/firefox-ios-tests/Tests/XCUITests/FxScreenGraph.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Models/TabDisplayModel.swift
- firefox-ios/Client/Application/AccessibilityIdentifiers.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsCell.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsHeaderView.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Models/InactiveTabsModel.swift
- firefox-ios/Client/Frontend/Browser/Tabs/LayoutManager/InactiveTabsSectionManager.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Action/TabPanelAction.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsFooterView.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/Animation/TabAnimation.swift
- firefox-ios/Client.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (5)
firefox-ios/firefox-ios-tests/Tests/XCUITests/registerSettingsNavigation.swift (2)
firefox-ios/Client/Redux/GlobalState/AppState.swift (1)
screenState(16-52)firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift (1)
tap(534-543)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayPanelTests.swift (2)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayDiffableDataSourceTests.swift (2)
createSubject(50-74)createTabs(76-85)firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (1)
createTabs(240-244)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayDiffableDataSourceTests.swift (1)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (1)
createTabs(240-244)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (2)
firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift (1)
createTabScrollBehavior(130-156)firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
scrollToTab(171-181)
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
firefox-ios/Client/Frontend/Browser/Tabs/LayoutManager/TabsSectionManager.swift (2)
experimentLayoutSection(70-123)layoutSection(32-68)
🔇 Additional comments (17)
firefox-ios/Client/Frontend/Browser/ToastType.swift (1)
20-34: LGTM — toast title mapping remains consistent after removing inactive variants.firefox-ios/firefox-ios-tests/Tests/XCUITests/registerSettingsNavigation.swift (1)
187-190: LGTM!The test navigation correctly reflects the removal of the inactive tabs toggle. With the inactive tabs feature removed, the TabGroups switch is now at index 0, and the test properly maps the single remaining switch to
Action.ToggleTabGroups.firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayDiffableDataSource.swift (1)
8-25: LGTM!Clean simplification of the diffable data source following the inactive tabs removal. The single-section model with only
.tabsand.tab(TabModel)cases is appropriate, and theupdateSnapshotmethod correctly maps the active tabs from state to display items.firefox-ios/Shared/Strings.swift (1)
8461-8482: The review comment should not be submitted as-is. The fundamental premise is incorrect: the inactive tabs feature is still active in the codebase and has not been removed.Evidence:
- Active (non-OldStrings) InactiveTabs strings still exist:
TabsTrayInactiveTabsSectionTitle,InactiveTabs(settings), andInactiveTabsDescription- Feature state management is present:
inactiveTabsarray andisInactiveTabsExpandedinTabViewState.swift- Feature actions are active:
refreshInactiveTabsaction in tests- Contextual hints for the feature remain in active code
The v147 strings being added to OldStrings appear to be deprecated or superseded versions of localization keys, not strings from a removed feature. The constants themselves are unused, but this reflects a refactoring of old string variants, not feature removal.
Likely an incorrect or invalid review comment.
firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift (2)
87-110: Reducer updates align with active-tabs-only model.The middleware cases now return state using only active tabs, which matches the removal of inactive tabs.
124-128: Default state simplification looks good.Returning only active tabs here keeps state surface consistent across the reducer.
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
224-233: Layout closure simplification is consistent with single-section UI.Choosing the experiment vs. standard layout without inactive-tab branching is the right direction.
firefox-ios/firefox-ios-tests/Tests/XCUITests/TabsTests.swift (3)
49-51: Identifier update matches single-section tab tray.
94-98: TabDisplayView identifier adjusted correctly to 0-based section.
351-355: Tab cell lookup now aligned with active-tabs-only section.firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayPanelTests.swift (2)
20-33: Test expectations updated appropriately for active-tabs-only state.
38-59: Helper updates correctly remove inactive-tabs inputs.firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayDiffableDataSourceTests.swift (2)
25-46: Section-count assertions now match the single-section data source.
54-58: Subject state construction correctly drops inactive tabs.firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (3)
64-79: State construction update aligns with removed inactive tabs.
104-167: Scroll-behavior tests are consistent with active-tabs-only state.
169-217: Tab UUID scroll tests remain valid after state simplification.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
130
to
148
| static func createTabScrollBehavior( | ||
| forState state: TabsPanelState, | ||
| withScrollBehavior scrollBehavior: TabScrollBehavior | ||
| ) -> TabsPanelState.ScrollState? { | ||
| guard !(state.tabs.isEmpty && state.inactiveTabs.isEmpty) else { return nil } | ||
| guard !state.tabs.isEmpty else { return nil } | ||
|
|
||
| if case .scrollToSelectedTab(let shouldAnimate) = scrollBehavior { | ||
| if let selectedTabIndex = state.tabs.firstIndex(where: { $0.isSelected }) { | ||
| return ScrollState(toIndex: selectedTabIndex, isInactiveTabSection: false, withAnimation: shouldAnimate) | ||
| return ScrollState(toIndex: selectedTabIndex, withAnimation: shouldAnimate) | ||
| } else if !state.tabs.isEmpty { | ||
| // If the user switches between the normal and private tab panels, there's a chance this subset of tabs does | ||
| // not contain a selected tab. In that case, we should scroll to the bottom of the panel. | ||
| // Note: Could optimize further by scrolling to the most recent tab if we had `lastExecutedTime` in our model | ||
| return ScrollState(toIndex: state.tabs.count - 1, isInactiveTabSection: false, withAnimation: shouldAnimate) | ||
| return ScrollState(toIndex: state.tabs.count, withAnimation: shouldAnimate) | ||
| } | ||
| } else if case .scrollToTab(let tabUUID, let shouldAnimate) = scrollBehavior { | ||
| if let tabIndex = state.tabs.firstIndex(where: { $0.tabUUID == tabUUID }) { | ||
| return ScrollState(toIndex: tabIndex, isInactiveTabSection: false, withAnimation: shouldAnimate) | ||
| } else if let tabIndex = state.inactiveTabs.firstIndex(where: { $0.tabUUID == tabUUID }) { | ||
| return ScrollState(toIndex: tabIndex, isInactiveTabSection: true, withAnimation: shouldAnimate) | ||
| return ScrollState(toIndex: tabIndex, withAnimation: shouldAnimate) | ||
| } else { |
There was a problem hiding this comment.
Fix off-by-one when scrolling with no selected tab.
state.tabs.count is out of bounds for the last item, so IndexPath becomes invalid and the scroll is skipped. Use count - 1 instead.
🛠️ Proposed fix
- return ScrollState(toIndex: state.tabs.count, withAnimation: shouldAnimate)
+ return ScrollState(toIndex: state.tabs.count - 1, withAnimation: shouldAnimate)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static func createTabScrollBehavior( | |
| forState state: TabsPanelState, | |
| withScrollBehavior scrollBehavior: TabScrollBehavior | |
| ) -> TabsPanelState.ScrollState? { | |
| guard !(state.tabs.isEmpty && state.inactiveTabs.isEmpty) else { return nil } | |
| guard !state.tabs.isEmpty else { return nil } | |
| if case .scrollToSelectedTab(let shouldAnimate) = scrollBehavior { | |
| if let selectedTabIndex = state.tabs.firstIndex(where: { $0.isSelected }) { | |
| return ScrollState(toIndex: selectedTabIndex, isInactiveTabSection: false, withAnimation: shouldAnimate) | |
| return ScrollState(toIndex: selectedTabIndex, withAnimation: shouldAnimate) | |
| } else if !state.tabs.isEmpty { | |
| // If the user switches between the normal and private tab panels, there's a chance this subset of tabs does | |
| // not contain a selected tab. In that case, we should scroll to the bottom of the panel. | |
| // Note: Could optimize further by scrolling to the most recent tab if we had `lastExecutedTime` in our model | |
| return ScrollState(toIndex: state.tabs.count - 1, isInactiveTabSection: false, withAnimation: shouldAnimate) | |
| return ScrollState(toIndex: state.tabs.count, withAnimation: shouldAnimate) | |
| } | |
| } else if case .scrollToTab(let tabUUID, let shouldAnimate) = scrollBehavior { | |
| if let tabIndex = state.tabs.firstIndex(where: { $0.tabUUID == tabUUID }) { | |
| return ScrollState(toIndex: tabIndex, isInactiveTabSection: false, withAnimation: shouldAnimate) | |
| } else if let tabIndex = state.inactiveTabs.firstIndex(where: { $0.tabUUID == tabUUID }) { | |
| return ScrollState(toIndex: tabIndex, isInactiveTabSection: true, withAnimation: shouldAnimate) | |
| return ScrollState(toIndex: tabIndex, withAnimation: shouldAnimate) | |
| } else { | |
| static func createTabScrollBehavior( | |
| forState state: TabsPanelState, | |
| withScrollBehavior scrollBehavior: TabScrollBehavior | |
| ) -> TabsPanelState.ScrollState? { | |
| guard !state.tabs.isEmpty else { return nil } | |
| if case .scrollToSelectedTab(let shouldAnimate) = scrollBehavior { | |
| if let selectedTabIndex = state.tabs.firstIndex(where: { $0.isSelected }) { | |
| return ScrollState(toIndex: selectedTabIndex, withAnimation: shouldAnimate) | |
| } else if !state.tabs.isEmpty { | |
| // If the user switches between the normal and private tab panels, there's a chance this subset of tabs does | |
| // not contain a selected tab. In that case, we should scroll to the bottom of the panel. | |
| // Note: Could optimize further by scrolling to the most recent tab if we had `lastExecutedTime` in our model | |
| return ScrollState(toIndex: state.tabs.count - 1, withAnimation: shouldAnimate) | |
| } | |
| } else if case .scrollToTab(let tabUUID, let shouldAnimate) = scrollBehavior { | |
| if let tabIndex = state.tabs.firstIndex(where: { $0.tabUUID == tabUUID }) { | |
| return ScrollState(toIndex: tabIndex, withAnimation: shouldAnimate) | |
| } else { |
🤖 Prompt for AI Agents
In `@firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift` around
lines 130 - 148, The off-by-one occurs in createTabScrollBehavior when returning
ScrollState(toIndex: state.tabs.count, ...), which is out of bounds; change the
fallback to use the last valid index (state.tabs.count - 1) and ensure this
branch only executes when state.tabs is non-empty (the existing guard
!state.tabs.isEmpty already covers this), so replace the toIndex value with
state.tabs.count - 1 in the ScrollState returned by that else block to avoid
invalid IndexPath errors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#5
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.