Refactor NO_TICKET Various improvements #24
Conversation
WalkthroughThe changes refactor the address toolbar's page action stack naming convention, add accessibility identifiers for UI testing across multiple browser view controllers, and fix constraint logic and keyboard handling bugs in toolbar layout code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift (1)
328-333: Fix animation stack list to include trailingPageActionStack.
trailingPageActionStackis missing fromstacks, andbrowserActionStackis duplicated. Newly added trailing actions can stay alpha=0 and never appear.🐛 Proposed fix
- let stacks = browserActionStack.arrangedSubviews + - navigationActionStack.arrangedSubviews + - leadingPageActionStack.arrangedSubviews + - browserActionStack.arrangedSubviews + let stacks = browserActionStack.arrangedSubviews + + navigationActionStack.arrangedSubviews + + leadingPageActionStack.arrangedSubviews + + trailingPageActionStack.arrangedSubviews
🤖 Fix all issues with AI agents
In `@BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift`:
- Around line 418-421: The divider visibility logic only checks
leadingPageActionStack, so trailing page actions can be ignored; update the
calculation of hasPageActions to consider both
leadingPageActionStack.arrangedSubviews and
trailingPageActionStack.arrangedSubviews (e.g., check that either is not empty)
and then set dividerWidthConstraint?.constant to
uxConfig.browserActionsAddressBarDividerWidth when either stack has actions,
otherwise 0; modify the code around hasPageActions, dividerWidthConstraint,
leadingPageActionStack, and trailingPageActionStack accordingly.
In `@firefox-ios/Client/Frontend/Browser/Toolbars/AddressToolbarContainer.swift`:
- Around line 448-449: The toolbar's horizontal constraints are inverted:
toolbar.leadingAnchor is incorrectly constrained to
rightSkeletonAddressBar.trailingAnchor, creating a negative/inconsistent width;
change the leading constraint to use leftSkeletonAddressBar.trailingAnchor so
the toolbar spans between leftSkeletonAddressBar.trailingAnchor and
rightSkeletonAddressBar.leadingAnchor (update the constraint involving
toolbar.leadingAnchor to reference leftSkeletonAddressBar.trailingAnchor while
keeping toolbar.trailingAnchor tied to rightSkeletonAddressBar.leadingAnchor).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swiftfirefox-ios/Client/Application/AccessibilityIdentifiers.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swiftfirefox-ios/Client/Frontend/Browser/Toolbars/AddressToolbarContainer.swift
🔇 Additional comments (4)
firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift (1)
137-139: Accessibility identifiers wired cleanly.Nice, this improves UI testing and accessibility tooling without changing behavior.
Also applies to: 149-151, 155-157, 179-188, 191-193
firefox-ios/Client/Application/AccessibilityIdentifiers.swift (1)
64-73: New accessibility identifiers look good.Clear, consistent naming for UI testing.
firefox-ios/Client/Frontend/Browser/Toolbars/AddressToolbarContainer.swift (2)
223-235: LGTM for keyboard accessory state gating.
The rename and guard logic read cleanly and preserve the dispatch throttling.
461-477: LGTM for the early-return guard in skeleton layout.
The guard-based structure keeps the constraints scoped to the swiping-tabs feature flag.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Page action spacing | ||
| let hasPageActions = !pageActionStack.arrangedSubviews.isEmpty | ||
| let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty | ||
| dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0 | ||
| } |
There was a problem hiding this comment.
Divider visibility ignores trailing page actions.
hasPageActions only checks the leading stack. If trailing actions exist but leading is empty, the divider stays hidden.
🐛 Proposed fix
- let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty
+ let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty ||
+ !trailingPageActionStack.arrangedSubviews.isEmpty📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Page action spacing | |
| let hasPageActions = !pageActionStack.arrangedSubviews.isEmpty | |
| let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty | |
| dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0 | |
| } | |
| // Page action spacing | |
| let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty || | |
| !trailingPageActionStack.arrangedSubviews.isEmpty | |
| dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0 | |
| } |
🤖 Prompt for AI Agents
In `@BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift`
around lines 418 - 421, The divider visibility logic only checks
leadingPageActionStack, so trailing page actions can be ignored; update the
calculation of hasPageActions to consider both
leadingPageActionStack.arrangedSubviews and
trailingPageActionStack.arrangedSubviews (e.g., check that either is not empty)
and then set dividerWidthConstraint?.constant to
uxConfig.browserActionsAddressBarDividerWidth when either stack has actions,
otherwise 0; modify the code around hasPageActions, dividerWidthConstraint,
leadingPageActionStack, and trailingPageActionStack accordingly.
| toolbar.leadingAnchor.constraint(equalTo: rightSkeletonAddressBar.trailingAnchor).isActive = true | ||
| toolbar.trailingAnchor.constraint(equalTo: rightSkeletonAddressBar.leadingAnchor).isActive = true |
There was a problem hiding this comment.
Fix inverted toolbar constraints (leading/trailing pinned to the same right skeleton).
toolbar.leadingAnchor is now tied to rightSkeletonAddressBar.trailingAnchor while toolbar.trailingAnchor is tied to rightSkeletonAddressBar.leadingAnchor, which creates an unsatisfiable/inverted width. The leading anchor should remain tied to the left skeleton’s trailing edge.
🐛 Proposed fix
- toolbar.leadingAnchor.constraint(equalTo: rightSkeletonAddressBar.trailingAnchor).isActive = true
+ toolbar.leadingAnchor.constraint(equalTo: leftSkeletonAddressBar.trailingAnchor).isActive = true🤖 Prompt for AI Agents
In `@firefox-ios/Client/Frontend/Browser/Toolbars/AddressToolbarContainer.swift`
around lines 448 - 449, The toolbar's horizontal constraints are inverted:
toolbar.leadingAnchor is incorrectly constrained to
rightSkeletonAddressBar.trailingAnchor, creating a negative/inconsistent width;
change the leading constraint to use leftSkeletonAddressBar.trailingAnchor so
the toolbar spans between leftSkeletonAddressBar.trailingAnchor and
rightSkeletonAddressBar.leadingAnchor (update the constraint involving
toolbar.leadingAnchor to reference leftSkeletonAddressBar.trailingAnchor while
keeping toolbar.trailingAnchor tied to rightSkeletonAddressBar.leadingAnchor).
Benchmark PR from qodo-benchmark#228
Summary by CodeRabbit
Release Notes
Bug Fixes
Accessibility
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.