Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #18
Conversation
WalkthroughThis pull request removes the entire inactive tabs feature from the Firefox iOS codebase, including UI components, state management, business logic, middleware handlers, data source configuration, and related test coverage. Approximately 22 files are modified to eliminate all inactive tabs functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (2)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (1)
138-153: Implementation has off-by-one error: returns out-of-bounds index.Line 143 of
TabsPanelState.createTabScrollBehaviorreturnsstate.tabs.countwhen no selected tab exists, but this is out of bounds. For an array with 3 elements (indices 0–2), returning index 3 will cause a crash when used with collection views.The test correctly expects
tabCount - 1. The implementation should returnstate.tabs.count - 1instead.firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
171-181: Change hardcoded section index from 1 to 0 inscrollToTabfunction.After inactive tabs removal,
TabDisplayDiffableDataSourcenow contains only a single section (.tabs) at index 0. Line 172 incorrectly hardcodessection: 1, which references a non-existent section. This will causescrollToItemto fail with an invalid index path.All tests confirm tabs are expected at section 0:
testNumberOfSections_ForRegularTabs:numberOfItems(inSection: 0) = 2testNumberOfSections_PrivateTabs:numberOfItems(inSection: 0) = 9Suggested fix
private func scrollToTab(_ scrollState: TabsPanelState.ScrollState) { - 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 139-144: In TabsPanelState, the fallback ScrollState currently
uses toIndex: state.tabs.count which is off-by-one (points past the last item);
update the return to use the last valid index (state.tabs.count - 1) when
state.tabs is non-empty so scrollToTab receives a valid index; reference
TabsPanelState, the ScrollState construction there, and
TabDisplayView.scrollToTab/isValid(indexPath:) to ensure the chosen index stays
within 0..<state.tabs.count.
📜 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/Client/Application/AccessibilityIdentifiers.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Models/InactiveTabsModel.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Action/TabPanelAction.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsHeaderView.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Models/TabDisplayModel.swift
- firefox-ios/Client/Frontend/Browser/Tabs/LayoutManager/InactiveTabsSectionManager.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsFooterView.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/Animation/TabAnimation.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Views/InactiveTabs/InactiveTabsCell.swift
- firefox-ios/firefox-ios-tests/Tests/XCUITests/FxScreenGraph.swift
- firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.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/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)
firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift (1)
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
scrollToTab(171-181)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayPanelTests.swift (2)
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayDiffableDataSourceTests.swift (1)
createTabs(76-85)firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (1)
createTabs(240-244)
🔇 Additional comments (15)
firefox-ios/firefox-ios-tests/Tests/XCUITests/registerSettingsNavigation.swift (1)
187-190: LGTM!The
TabsSettingsscreen state correctly reflects the removal of the inactive tabs toggle. With the inactive tabs feature removed, the first switch (boundBy: 0) now controls Tab Groups, and the navigation back to Settings remains intact.firefox-ios/firefox-ios-tests/Tests/XCUITests/TabsTests.swift (4)
49-52: LGTM!The tab cell identifier correctly updated from section
1to section0, reflecting the removal of the inactive tabs section. The active tabs section is now the first (and only) section.
96-98: LGTM!Consistent update of the tab cell identifier to use section
0instead of1.
353-354: LGTM!Section index correctly updated to
0for the tab cell identifier.
529-546: Update tab cell identifiers to match other tests in the file.This test uses
_1_2for tab cell identifiers (6 occurrences) while all other tests in the same file use section0indices (e.g., lines 49, 96, 353). If the inactive tabs removal changed section numbering from1to0for active tabs, these references should be updated to_0_2for consistency.Occurrences in testCloseOneTabUndo
app.cells[AccessibilityIdentifiers.TabTray.tabCell+"_1_2"].buttons[StandardImageIdentifiers.Large.cross].tap() mozWaitForElementToNotExist(app.cells[AccessibilityIdentifiers.TabTray.tabCell+"_1_2"]) app.cells[AccessibilityIdentifiers.TabTray.tabCell+"_1_2"].press(forDuration: 2) mozWaitForElementToNotExist(app.cells[AccessibilityIdentifiers.TabTray.tabCell+"_1_2"]) app.cells[AccessibilityIdentifiers.TabTray.tabCell+"_1_2"].swipeLeft() mozWaitForElementToNotExist(app.cells[AccessibilityIdentifiers.TabTray.tabCell+"_1_2"])firefox-ios/Shared/Strings.swift (1)
8461-8482: LGTM — legacy inactive-tabs strings are correctly isolated.firefox-ios/Client/Frontend/Browser/ToastType.swift (1)
9-72: LGTM!The
ToastTypeenum has been cleanly simplified by removing the inactive tab cases. The switch statements remain exhaustive, and the redux action mappings for undo functionality are correctly preserved for the active tab cases.firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift (1)
84-128: LGTM on the reducer simplification.The middleware action handling and state construction have been cleanly updated to remove inactive tabs state. The reducer logic remains sound and properly handles all action types.
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayDiffableDataSourceTests.swift (2)
25-47: LGTM!Test assertions correctly updated to expect a single section after removing inactive tabs. Coverage maintained for regular tabs, private tabs, and empty state scenarios.
50-85: LGTM on helper methods.The
createSubjectandcreateTabshelpers are cleanly simplified to work with the single-section model.firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayDiffableDataSource.swift (1)
8-25: LGTM!The diffable data source has been cleanly simplified to a single section and item type. The snapshot construction is straightforward and correct.
firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabDisplayPanelTests.swift (1)
20-71: LGTM!Test setup and assertions have been cleanly updated to work without inactive tabs state. The tests for
isPrivateTabsEmptyremain valid and the helper methods are appropriately simplified.firefox-ios/firefox-ios-tests/Tests/ClientTests/TabTray/TabsPanelStateTests.swift (2)
22-100: LGTM on middleware action tests.The tests for
didLoadTabPanel,didChangeTabPanel,willAppearTabPanel, andrefreshTabsare correctly updated and properly verify the reducer behavior without inactive tabs state.
104-117: LGTM on remaining scroll behavior tests.The tests for edge cases (no tabs, tab doesn't exist) and the
scrollToTabbehavior are correctly implemented and verify the expected scroll state outcomes.Also applies to: 155-217
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift (1)
224-236: LGTM!The layout creation logic is correctly simplified after removing inactive tabs. The branching now only depends on the experiment flag, delegating to the appropriate
TabsSectionManagermethod.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } 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) | ||
| } |
There was a problem hiding this comment.
Potential off-by-one: toIndex: state.tabs.count may exceed valid bounds.
When no selected tab is found, returning state.tabs.count as the index points past the last valid item (valid indices are 0..<count). The comment says "scroll to the bottom," which typically means the last item at index count - 1.
The TabDisplayView.scrollToTab method does guard with isValid(indexPath:), so this won't crash, but the scroll may silently fail or behave unexpectedly.
Suggested fix
} 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, withAnimation: shouldAnimate)
+ return ScrollState(toIndex: state.tabs.count - 1, withAnimation: shouldAnimate)
}🤖 Prompt for AI Agents
In `@firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift` around
lines 139 - 144, In TabsPanelState, the fallback ScrollState currently uses
toIndex: state.tabs.count which is off-by-one (points past the last item);
update the return to use the last valid index (state.tabs.count - 1) when
state.tabs is non-empty so scrollToTab receives a valid index; reference
TabsPanelState, the ScrollState construction there, and
TabDisplayView.scrollToTab/isValid(indexPath:) to ensure the chosen index stays
within 0..<state.tabs.count.
Benchmark PR from qodo-benchmark#230
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.