Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #18
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // 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.
🔴 Off-by-one error: scroll index uses tabs.count instead of tabs.count - 1
When there's no selected tab and the user switches between tab panels, the code attempts to scroll to an invalid index.
Click to expand
Bug mechanism
In TabsPanelState.swift:143, the code was changed from state.tabs.count - 1 to state.tabs.count. This is incorrect because:
- If there are 3 tabs, valid indices are 0, 1, 2
state.tabs.countreturns 3, which is out of bounds- The correct index for the last tab is
state.tabs.count - 1(i.e., 2)
Actual behavior
return ScrollState(toIndex: state.tabs.count, withAnimation: shouldAnimate)Returns an out-of-bounds index.
Expected behavior
return ScrollState(toIndex: state.tabs.count - 1, withAnimation: shouldAnimate)Returns the last valid tab index.
Evidence
The unit test at TabsPanelStateTests.swift:152 expects tabCount - 1:
XCTAssertEqual(scrollState?.toIndex, tabCount - 1, "Should return the last tab if there is no selected tab")Impact
This could cause scroll operations to fail silently or potentially crash when trying to scroll to an invalid index path.
Recommendation: Change state.tabs.count back to state.tabs.count - 1 to correctly reference the last tab index.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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.
🔴 Invalid section index: scrollToTab uses section 1 but only section 0 exists
After removing inactive tabs, the scrollToTab function references a non-existent section.
Click to expand
Bug mechanism
In TabDisplayView.swift:172, the code was changed from section: 0 to section: 1. However, after removing the inactive tabs section in this PR, there is now only one section (.tabs) at index 0.
Looking at TabDisplayDiffableDataSource.swift:19:
snapshot.appendSections([.tabs])Only one section is added, which will be at index 0.
Actual behavior
let indexPath = IndexPath(row: scrollState.toIndex, section: 1)Creates an index path with section 1, which doesn't exist.
Expected behavior
let indexPath = IndexPath(row: scrollState.toIndex, section: 0)Creates an index path with section 0, which is where the tabs are.
Impact
The isValid(indexPath:) guard will fail, preventing scrolling to tabs entirely. This affects:
- Scrolling to selected tab when tab tray appears
- Scrolling to restored tab after undo close
- Any scroll-to-tab functionality
Recommendation: Change section: 1 back to section: 0 since the tabs section is now at index 0 after removing the inactive tabs section.
Was this helpful? React with 👍 or 👎 to provide feedback.
Benchmark PR from qodo-benchmark#230