Remove FXIOS-14327 [Clean up] Inactive tabs removal part 2 (final) #18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code Review by Qodo
1. MPL header removed in TabPanelAction.swift
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
1
to
3
| import Redux | ||
| import Common | ||
|
|
There was a problem hiding this comment.
1. Mpl header removed in tabpanelaction.swift 📘 Rule violation ⛨ Security
• TabPanelAction.swift no longer begins with the required Mozilla Public License v2.0 header. • This violates the repository licensing requirement for all Swift source files and can create legal/compliance risk if merged. • Re-add the exact MPL v2.0 header block at the very top of the file before any imports.
Agent prompt
## Issue description
`TabPanelAction.swift` is missing the required MPL v2.0 header at the very top of the file.
## Issue Context
Compliance requires every `.swift` file to begin with the exact MPL v2.0 header block.
## Fix Focus Areas
- firefox-ios/Client/Frontend/Browser/Tabs/Action/TabPanelAction.swift[1-3]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
171
to
175
| 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) | ||
| // Piping this into main thread let the collection view finish its layout process | ||
| DispatchQueue.main.async { | ||
| guard !self.collectionView.indexPathsForFullyVisibleItems.contains(indexPath) else { return } |
There was a problem hiding this comment.
2. scrolltotab uses wrong section 📘 Rule violation ⛯ Reliability
• TabDisplayView.scrollToTab(_:) builds an IndexPath with section: 1 even though the diffable data source snapshot now appends only a single section. • This creates an out-of-range section index (boundary value error), causing scroll behavior to silently fail and increasing risk of future crashes if validation guards change. • Update scrollToTab(_:) to target the correct section index for the .tabs section (0).
Agent prompt
## Issue description
`TabDisplayView.scrollToTab(_:)` uses `section: 1`, but the diffable data source now only appends one section (`.tabs`). This is a boundary/indexing bug and can cause scrolling to silently fail.
## Issue Context
`TabDisplayDiffableDataSource.updateSnapshot` appends only `[.tabs]`, so the valid section indices are only `0`.
## Fix Focus Areas
- firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift[171-180]
- firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayDiffableDataSource.swift[17-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+134
to
144
| guard !state.tabs.isEmpty else { return nil } | ||
|
|
||
| if case .scrollToSelectedTab(let shouldAnimate) = scrollBehavior { | ||
| if let selectedTabIndex = state.tabs.firstIndex(where: { $0.isSelected }) { | ||
| return ScrollState(toIndex: selectedTabIndex, isInactiveTabSection: false, withAnimation: shouldAnimate) | ||
| return ScrollState(toIndex: selectedTabIndex, withAnimation: shouldAnimate) | ||
| } else if !state.tabs.isEmpty { | ||
| // If the user switches between the normal and private tab panels, there's a chance this subset of tabs does | ||
| // 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.
3. createtabscrollbehavior off-by-one index 📘 Rule violation ⛯ Reliability
• TabsPanelState.createTabScrollBehavior returns toIndex: state.tabs.count when no selected tab exists, which is one past the last valid index (count - 1). • This is a boundary value error that can produce invalid index paths for scrolling and cause silent failures or crashes depending on downstream handling. • Use state.tabs.count - 1 (or equivalent safe clamping) when scrolling to the last tab.
Agent prompt
## Issue description
`TabsPanelState.createTabScrollBehavior` returns `toIndex: state.tabs.count` when no selected tab exists, which is out of bounds for the `tabs` array.
## Issue Context
The returned `toIndex` is later used to build an `IndexPath(row: toIndex, ...)` for scrolling. The valid last row index is `state.tabs.count - 1`.
## Fix Focus Areas
- firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift[134-151]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Benchmark PR from qodo-benchmark#230