feat(mobile-ios): Section renderer – MediaCollection (#104)#118
feat(mobile-ios): Section renderer – MediaCollection (#104)#118Ur-imazing wants to merge 1 commit intomainfrom
Conversation
Made-with: Cursor
WalkthroughIntroduces a new SwiftUI view component Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/ios/Sources/ForgeMobile/Views/MediaCollectionView.swift`:
- Around line 12-21: The switch in body correctly selects
heroLayout/playerLayout/standardLayout but those layout views (heroLayout,
playerLayout, standardLayout — and the code around lines referenced where
section.items is not used) never iterate or render section.items; update each
layout builder to consume section.items (e.g., map/ForEach over section.items)
and pass item-specific views/data into the renderer used by those layouts so
media content is actually displayed, ensuring the layouts accept the item
collection or access section.items directly and render item views for
carousel/grid/collection/hero/player variants.
- Around line 105-117: The CTA Button currently has an empty action so taps do
nothing; update the Button created where section.ctaLink is unwrapped (the
Button block referencing section.ctaLink and the "Learn more" Text) to perform
navigation: safely create a URL from section.ctaLink and open it (e.g. via
UIApplication.shared.open(_:options:completionHandler:) on iOS) or replace the
Button with a SwiftUI Link to that URL; ensure you guard/unwrap the URL before
opening and keep accessibilityLabel/accessibilityHint as-is.
- Around line 23-25: The Group-level accessibility modifier
`.accessibilityElement(children: .combine)` is hiding the nested ctaButton from
VoiceOver; remove that container-level `.accessibilityElement(children:
.combine)` applied to the Group (the view that also sets
`.accessibilityLabel(accessibilityLabelText)`) so the Button remains an
independent, focusable element, and if you need a combined label for the CTA,
apply `.accessibilityElement(children: .combine)` only to the Button's label
(inside the Button closure) instead of the container.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mobile/ios/Sources/ForgeMobile/Views/MediaCollectionView.swift
| public var body: some View { | ||
| Group { | ||
| switch section.variant { | ||
| case .hero: | ||
| heroLayout | ||
| case .player: | ||
| playerLayout | ||
| case .carousel, .collection, .grid: | ||
| standardLayout | ||
| } |
There was a problem hiding this comment.
Variant routing exists, but media items are never rendered.
Line 19 routes variants, but Lines 74-101 (and other layouts) never use section.items. This misses the core renderer requirement for carousel/grid/collection/hero/player content.
Also applies to: 74-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/ios/Sources/ForgeMobile/Views/MediaCollectionView.swift` around lines
12 - 21, The switch in body correctly selects
heroLayout/playerLayout/standardLayout but those layout views (heroLayout,
playerLayout, standardLayout — and the code around lines referenced where
section.items is not used) never iterate or render section.items; update each
layout builder to consume section.items (e.g., map/ForEach over section.items)
and pass item-specific views/data into the renderer used by those layouts so
media content is actually displayed, ensuring the layouts accept the item
collection or access section.items directly and render item views for
carousel/grid/collection/hero/player variants.
| .accessibilityElement(children: .combine) | ||
| .accessibilityLabel(accessibilityLabelText) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n mobile/ios/Sources/ForgeMobile/Views/MediaCollectionView.swiftRepository: JesusFilm/forge
Length of output: 5981
🌐 Web query:
SwiftUI accessibilityElement children combine nested Button VoiceOver focus activation
💡 Result:
accessibilityElement(children: .combine) turns the container into a single accessibility element and hides its children from VoiceOver. So if that container contains (or wraps) a nested Button, the nested button generally won’t be focusable/activatable, because it’s no longer exposed as its own accessibility element. [1][2][3]
What to do instead (common fixes)
- Only combine inside a single control’s label (recommended)
Button(action: primary) {
HStack {
Image(systemName: "star")
Text("Favorite")
}
.accessibilityElement(children: .combine) // combine *within the button label*
}This keeps the Button as the element VoiceOver focuses, while reading the label as one phrase. [2][3]
- Don’t combine a container that holds multiple interactive elements
If you have, e.g., a row tap + an info button, keep the parent as.contain(or default), so each control remains focusable:
HStack {
Button("Open", action: open)
Button("Info", action: info)
}
// .accessibilityElement(children: .contain) // explicit if neededBecause .combine would collapse these into one element and hide the inner buttons. [1][3]
- If you intentionally make a container act like one control, use
.ignore+ an accessibility action
This is for “tappable row” patterns where you want exactly one VoiceOver target:
HStack { /* visuals */ }
.accessibilityElement(children: .ignore)
.accessibilityLabel("…")
.accessibilityAddTraits(.isButton)
.accessibilityAction { primary() }Without an accessibility action, it may not perform anything meaningful when activated. [1][4]
Rule of thumb: never use .combine on a view hierarchy that contains more than one independently tappable control; VoiceOver can’t focus what you’ve told SwiftUI to hide. [1][3]
Sources: [1] Rob Whitaker (semantic views / .combine hides children), [2] SwiftLee (what .combine does), [3] Kodeco tutorial (combining hides individual children), [4] Swift with Majid (container-as-element + actions).
Remove .combine from container level—it hides the nested CTA button from VoiceOver.
Applying .accessibilityElement(children: .combine) at the Group level makes the nested Button (ctaButton) non-focusable and non-activatable for VoiceOver users. The button is hidden as an independent accessible element, breaking interaction with the primary call-to-action.
If a combined label is needed, move .combine inside the button's label only:
Button(action: {}, label: {
Text("Learn more")
.font(.subheadline)
.fontWeight(.medium)
.accessibilityElement(children: .combine)
})Otherwise, remove .combine from the Group and let the button remain independently focusable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/ios/Sources/ForgeMobile/Views/MediaCollectionView.swift` around lines
23 - 25, The Group-level accessibility modifier `.accessibilityElement(children:
.combine)` is hiding the nested ctaButton from VoiceOver; remove that
container-level `.accessibilityElement(children: .combine)` applied to the Group
(the view that also sets `.accessibilityLabel(accessibilityLabelText)`) so the
Button remains an independent, focusable element, and if you need a combined
label for the CTA, apply `.accessibilityElement(children: .combine)` only to the
Button's label (inside the Button closure) instead of the container.
| if let link = section.ctaLink, !link.isEmpty { | ||
| Button( | ||
| action: {}, | ||
| label: { | ||
| Text("Learn more") | ||
| .font(.subheadline) | ||
| .fontWeight(.medium) | ||
| } | ||
| ) | ||
| .buttonStyle(.borderedProminent) | ||
| .accessibilityLabel("Learn more") | ||
| .accessibilityHint("Opens link") | ||
| } |
There was a problem hiding this comment.
CTA is non-functional because the button action is empty.
Line 107 uses action: {} while a valid ctaLink is present, so the CTA appears tappable but does nothing.
Proposed fix
public struct MediaCollectionView: View {
+ `@Environment`(\.openURL) private var openURL
private let section: MediaCollectionSection
@@
private var ctaButton: some View {
if let link = section.ctaLink, !link.isEmpty {
Button(
- action: {},
+ action: {
+ guard let url = URL(string: link) else { return }
+ openURL(url)
+ },
label: {
Text("Learn more")
.font(.subheadline)
.fontWeight(.medium)
}
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/ios/Sources/ForgeMobile/Views/MediaCollectionView.swift` around lines
105 - 117, The CTA Button currently has an empty action so taps do nothing;
update the Button created where section.ctaLink is unwrapped (the Button block
referencing section.ctaLink and the "Learn more" Text) to perform navigation:
safely create a URL from section.ctaLink and open it (e.g. via
UIApplication.shared.open(_:options:completionHandler:) on iOS) or replace the
Button with a SwiftUI Link to that URL; ensure you guard/unwrap the URL before
opening and keep accessibilityLabel/accessibilityHint as-is.
1 participant
Summary
Adds MediaCollectionView section renderer for Epic #100 (native iOS watch app). Renders
MediaCollectionSectionfrom the data layer with variant-based layout (hero, player, carousel/collection/grid), title, subtitle, description, categoryLabel, and CTA. SwiftLint and build pass.Contracts changed
None.
Regeneration required
No.
Validation
swiftlint lint --strictin mobile/ios: 0 violationsxcodebuild -project App/ForgeApp.xcodeproj -scheme ForgeApp -destination 'generic/platform=iOS Simulator' build: successResolves #104
Made with Cursor
Summary by CodeRabbit