-
Notifications
You must be signed in to change notification settings - Fork 0
Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: cursor_combined_20260121_qodo_grep_cursor_copilot_1_base_remove_fxios-14327_clean_up_inactive_tabs_removal_part_2_final_pr230
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded section index is wrong after removing inactive tabs
High Severity
The scrollToTab method hardcodes section: 1 when creating the IndexPath, but since the inactive tabs section was removed, there's now only one section (.tabs) at index 0. This causes the scroll functionality to silently fail because the guard on line 176 will return early when isValid(indexPath:) returns false. The test files were correctly updated to use section 0 (e.g., _0_1 instead of _1_1), but this production code was not.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-by-one error when scrolling to last tab
Medium Severity
The scroll-to-last-tab fallback uses state.tabs.count as the index, but array indices are 0-based, so this produces an out-of-bounds index. For example, with 3 tabs (indices 0, 1, 2), count returns 3 which is invalid. The original code used state.tabs.count - 1 which was correct. This causes the scroll to fail silently when switching tab panels with no selected tab.
Benchmark PR from qodo-benchmark#230
Note
Cursor Bugbot is generating a summary for commit f7001a1. Configure here.