Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final)#5
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 function uses a hardcoded section index of 1, but the collection view now only has a section 0, causing scrolling to silently fail.
Severity: HIGH
Suggested Fix
In TabDisplayView.swift, change the hardcoded section index from 1 to 0 when creating the IndexPath inside the scrollToTab function. The line should be 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` function creates an
`IndexPath` with a hardcoded section index of `1`. However, a recent refactoring has
changed the collection view's data source to only have a single section at index `0`. As
a result, the `self.collectionView.isValid(indexPath: indexPath)` guard check will
always fail when this function is called. This prevents a crash but causes the scroll
functionality to silently fail, meaning users will not be able to scroll to tabs when
switching panels, closing/undoing tabs, or opening a tab in the tray.
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 index to tabs.count instead of tabs.count - 1 when no tab is selected, causing the scroll action to fail silently.
Severity: MEDIUM
Suggested Fix
In TabsPanelState.swift, when handling the case with no selected tab, return a ScrollState with toIndex set to state.tabs.count - 1 to correctly target the last item in the array.
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: When creating a `ScrollState` for a tab panel with no selected tab, the
code calculates the index to scroll to as `state.tabs.count`. This is an off-by-one
error, as the last valid index is `state.tabs.count - 1`. This incorrect index is then
used to create an `IndexPath`. A guard check, `isValid(indexPath:)`, prevents an
out-of-bounds crash, but it causes the scroll operation to be silently skipped. The
intended behavior of scrolling to the last tab will not occur.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
Benchmark PR from agentic-review-benchmarks#5