Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #16
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| private func scrollToTab(_ scrollState: TabsPanelState.ScrollState) { | ||
| let section: Int = scrollState.isInactiveTabSection ? 0 : 1 | ||
| let indexPath = IndexPath(row: scrollState.toIndex, section: section) | ||
| let indexPath = IndexPath(row: scrollState.toIndex, section: 1) |
There was a problem hiding this comment.
Bug: The scrollToTab() method uses a hardcoded section: 1 for an IndexPath, but the collection view now only has one section at index 0, causing scrolling to silently fail.
Severity: MEDIUM
Suggested Fix
Update the IndexPath creation in scrollToTab() to use the correct section index. Change let indexPath = IndexPath(row: scrollState.toIndex, section: 1) to let indexPath = IndexPath(row: scrollState.toIndex, section: 0).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift#L172
Potential issue: In `TabDisplayView.swift`, the `scrollToTab()` method hardcodes the
collection view section to `1` when creating an `IndexPath`. However, due to recent
refactoring, the collection view now only has a single section at index `0`. A
subsequent `isValid(indexPath:)` check prevents a crash by detecting the out-of-bounds
section index, but this causes the guard clause to return early. As a result, the
intended scrolling action to a specific tab will silently fail, negatively impacting
user experience when switching tabs or using the undo/redo feature.
Did we get this right? 👍 / 👎 to inform future reviews.
| // 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.
Bug: An off-by-one error sets the scroll toIndex to state.tabs.count instead of state.tabs.count - 1, causing the scroll operation to silently fail.
Severity: MEDIUM
Suggested Fix
Correct the off-by-one error by subtracting 1 from the count to get the correct index for the last element. Change return ScrollState(toIndex: state.tabs.count, ...) back to return ScrollState(toIndex: state.tabs.count - 1, ...). This will ensure scrolling to the last tab works as expected.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift#L143
Potential issue: An off-by-one error was introduced when calculating the scroll
destination index. The code sets `toIndex` to `state.tabs.count`, but valid array
indices range from `0` to `count - 1`. This occurs when switching between tab panels if
the destination panel has no currently selected tab. While a downstream `isValid` check
will prevent a crash by catching the out-of-bounds index, it causes the scrolling
functionality to silently fail. The user will not be scrolled to the last tab as
intended, leading to a broken user experience.
Did we get this right? 👍 / 👎 to inform future reviews.
| private func scrollToTab(_ scrollState: TabsPanelState.ScrollState) { | ||
| let section: Int = scrollState.isInactiveTabSection ? 0 : 1 | ||
| let indexPath = IndexPath(row: scrollState.toIndex, section: section) | ||
| let indexPath = IndexPath(row: scrollState.toIndex, section: 1) |
There was a problem hiding this comment.
Bug: The code attempts to scroll to an IndexPath using a hardcoded section: 1, but the collection view now only has one section at index 0, causing the scroll to fail.
Severity: HIGH
Suggested Fix
Update the IndexPath creation in TabDisplayView.swift to use section: 0 instead of the hardcoded section: 1 to correctly target the only available section in the collection view.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift#L172
Potential issue: In `TabDisplayView.swift`, the code attempts to scroll to a new tab
using a hardcoded section index of 1. However, due to changes in
`TabDisplayDiffableDataSource.swift`, the collection view now only has a single section
at index 0. This causes the `self.collectionView.isValid(indexPath: indexPath)` check to
fail because the section index is out of bounds (`0..<1 ~= 1` is false). As a result,
the guard statement is triggered, and the scroll operation silently fails, breaking
auto-scrolling functionality when switching panels or undoing a closed tab.
Did we get this right? 👍 / 👎 to inform future reviews.
| // 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.
Bug: When no tab is selected, the code calculates an out-of-bounds scroll index using state.tabs.count instead of state.tabs.count - 1, causing the scroll to fail.
Severity: MEDIUM
Suggested Fix
In TabsPanelState.swift, change the toIndex parameter from state.tabs.count back to state.tabs.count - 1 to provide a valid index for the last item in the collection, as expected by the existing tests.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift#L143
Potential issue: In `TabsPanelState.swift`, when there is no selected tab, the scroll
behavior is calculated to use an index of `state.tabs.count`. This is an out-of-bounds
index, as the valid indices for a collection are from 0 to `count - 1`. This incorrect
index is passed to the scroll view, where the `isValid(indexPath:)` check fails. This is
confirmed by the test
`testCreateTabScrollBehavior_forScrollToSelectedTab_noSelectedTab_returnsLastTab_ifTabsNotEmpty`,
which explicitly expects an index of `tabCount - 1`. This bug causes auto-scrolling to
the last tab to fail silently.
Did we get this right? 👍 / 👎 to inform future reviews.
Benchmark PR from qodo-benchmark#230