-
Notifications
You must be signed in to change notification settings - Fork 0
Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #18
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: greptile_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?
Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #18
Conversation
Greptile SummaryThis PR completes the removal of the inactive tabs feature from Firefox iOS by deleting UI components, middleware logic, state management, and associated tests. Key Changes
Critical Issue
Confidence Score: 0/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabDisplayView
participant Store
participant TabManagerMiddleware
participant TabsPanelState
participant DataSource
Note over User,DataSource: Inactive Tabs Feature Removal
User->>TabDisplayView: View tabs panel
TabDisplayView->>Store: Dispatch TabPanelMiddlewareAction.didLoadTabPanel
Store->>TabManagerMiddleware: Handle didLoadTabPanel
TabManagerMiddleware->>TabManagerMiddleware: getTabsDisplayModel()
Note over TabManagerMiddleware: No longer calls refreshInactiveTabs()
TabManagerMiddleware->>Store: Dispatch refreshTab action
Store->>TabsPanelState: Reduce TabPanelMiddlewareAction
Note over TabsPanelState: Removed inactiveTabs & isInactiveTabsExpanded state
TabsPanelState-->>Store: Updated state (tabs only)
Store-->>TabDisplayView: State update
TabDisplayView->>DataSource: updateSnapshot(state)
Note over DataSource: Only creates .tabs section (no .inactiveTabs)
DataSource-->>TabDisplayView: Updated UI
User->>TabDisplayView: Switch between panels
TabDisplayView->>Store: Dispatch willAppearTabPanel
Store->>TabManagerMiddleware: Handle willAppearTabPanel
TabManagerMiddleware->>TabsPanelState: Create scroll behavior
Note over TabsPanelState: Bug: Returns state.tabs.count instead of count-1
TabsPanelState-->>Store: ScrollState with toIndex
Store-->>TabDisplayView: Updated scroll state
TabDisplayView->>TabDisplayView: scrollToTab(scrollState)
Note over TabDisplayView: IndexPath created with scrollState.toIndex
Note over TabDisplayView: May crash if index out of bounds
|
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.
22 files reviewed, 1 comment
| // 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.
logic: index out of bounds - changed from state.tabs.count - 1 to state.tabs.count
This will cause a crash when scrolling to a non-existent index. For an array with 3 items (indices 0, 1, 2), state.tabs.count is 3, which is out of bounds. The test testCreateTabScrollBehavior_forScrollToSelectedTab_noSelectedTab_returnsLastTab_ifTabsNotEmpty in TabsPanelStateTests.swift:152 still expects tabCount - 1.
| return ScrollState(toIndex: state.tabs.count, withAnimation: shouldAnimate) | |
| return ScrollState(toIndex: state.tabs.count - 1, withAnimation: shouldAnimate) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift
Line: 143:143
Comment:
**logic:** index out of bounds - changed from `state.tabs.count - 1` to `state.tabs.count`
This will cause a crash when scrolling to a non-existent index. For an array with 3 items (indices 0, 1, 2), `state.tabs.count` is 3, which is out of bounds. The test `testCreateTabScrollBehavior_forScrollToSelectedTab_noSelectedTab_returnsLastTab_ifTabsNotEmpty` in `TabsPanelStateTests.swift:152` still expects `tabCount - 1`.
```suggestion
return ScrollState(toIndex: state.tabs.count - 1, withAnimation: shouldAnimate)
```
How can I resolve this? If you propose a fix, please make it concise.
Benchmark PR from qodo-benchmark#230