BTraitsObserver: protocol-driven unified trait observation#4
BTraitsObserver: protocol-driven unified trait observation#4
Conversation
Show only directories instead of enumerating individual files. Add missing BroadwayTestHost and BroadwayTesting to README. Made-with: Cursor
Add BTraitsValueObserver protocol and associated type to BTraitsValue so each trait declares its own observer. BTraitsObserver coordinates them behind a single start/stop interface, simplifying BRootViewController. Remove BAccessibility.observe(_:) factory in favor of direct Observer init. Made-with: Cursor
BTraits now owns the set of registered trait types via register<V>() and a .system factory. BTraitsObserver reads registrations and the UIViewController hierarchy from the BTraits instance instead of hardcoding types. initialValue replaced with currentValue(from:) for future injectability. EquatableIgnored gains Hashable conformance. Made-with: Cursor
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b306ccf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public func start() { | ||
| for observer in observers { | ||
| observer.start() |
There was a problem hiding this comment.
BTraitsObserver documents start() as safe to call multiple times, but this implementation always forwards start() to every underlying observer with no guard at the coordinator level. Custom BTraitsValueObserver types are public and may not be idempotent, so repeated start() calls can create duplicate subscriptions/callbacks; add an internal started-state check so the documented behavior is guaranteed.
Useful? React with 👍 / 👎.
Defer child VC creation, trait observation, and context setup until the controller enters a valid view hierarchy (viewIsAppearing). Replace the XCTestCase extension with standalone free functions compatible with Swift Testing. Made-with: Cursor
…e BAccessibility trait conformance into the BAccessibility file.
…ewController Move BTraits, BTraitsObserver, and BAccessibility into BroadwayCore/Sources/Traits/. Extract trait value conformances into Values/. Add BTraits+Overrides for override support and BTraitOverridesViewController for propagating trait overrides through the view hierarchy. Made-with: Cursor
…gnored for caches Flatten BStylesheets to store traits/themes directly instead of through a Config struct. Mark cache and creating arrays with @EquatableIgnored so equality is based solely on traits and themes. Add @unchecked Sendable to EquatableIgnored and mark stylesheets as @EquatableIgnored on BContext. Made-with: Cursor
…troller fix Pass UIViewController through to BTraitsValue.makeObserver so VC-scoped traits can use registerForTraitChanges. Add UIViewControllerTraitObserver as a reusable observer for UIKit trait-backed values. Wire BMode and BContentSizeCategory to read from UITraitCollection and observe changes. Fix BTraitOverridesViewController with proper child containment and lazy setup. Bridge BContext into SwiftUI via UITraitBridgedEnvironmentKey and add bTraitOverrides/bMode/ bContentSizeCategory view modifiers. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f0a2f072a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private func applyOverrides() { | ||
| var current = traitCollection.bContext.traitOverrides | ||
| overrides(.init(traits: traitCollection.bContext.traits), ¤t) | ||
| traitOverrides.bContext.traitOverrides = current |
There was a problem hiding this comment.
applyOverrides() derives new override values from the resolved traitCollection.bContext, but then writes only traitOverrides onto traitOverrides.bContext. On first use, traitOverrides.bContext is the local default context, so this replaces the subtree's inherited baseTraits/themes with defaults and keeps only explicit overrides, which can reset accessibility/theme data for descendants. Build from the inherited context and assign the full context back to traitOverrides.bContext.
Useful? React with 👍 / 👎.
BroadwayCore/Sources/BContext.swift
Outdated
| traitOverrides = overrides | ||
| self.themes = themes | ||
| stylesheets = BStylesheets(config: .init(traits: traits, themes: themes)) | ||
| stylesheets = BStylesheets(traits: traits, themes: themes) |
There was a problem hiding this comment.
The initializer accepts overrides, but initializes stylesheets using only traits. When overrides is non-empty, context.traits (merged) and context.stylesheets.traits (base-only) diverge until another mutation triggers a didSet, so immediate stylesheet lookups can be computed under stale traits.
Useful? React with 👍 / 👎.
| init( | ||
| notificationCenter: NotificationCenter, | ||
| settingsProvider: SettingsProvider, | ||
| notificationCenter: NotificationCenter = .default, | ||
| settingsProvider: any SettingsProvider = BAccessibility.systemSettings, | ||
| onChange: @MainActor @escaping @Sendable (BAccessibility, BAccessibility) -> Void, | ||
| ) { |
There was a problem hiding this comment.
After removing the BAccessibility.observe(...) factory, the nested Observer initializer is now the only construction path, but it is still internal. External modules importing BroadwayCore cannot instantiate BAccessibility.Observer, so they can no longer start accessibility observation through public API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is fine, let's keep it internal until we need to make it public.
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 7f0a2f0. Configure here.
| var current = traitCollection.bContext.traitOverrides | ||
| overrides(.init(traits: traitCollection.bContext.traits), ¤t) | ||
| traitOverrides.bContext.traitOverrides = current | ||
| } |
There was a problem hiding this comment.
Overrides VC loses inherited base traits and themes
High Severity
applyOverrides() reads traitOverrides.bContext (this VC's own override slot, which starts as BContextTrait.defaultValue = BContext()) and only mutates its .traitOverrides field before writing it back. This discards the inherited baseTraits, themes, and stylesheets from the parent. Children of this VC will always see a default-constructed BContext with empty base traits instead of the live system traits propagated by BRootViewController. The method needs to start from traitCollection.bContext (the resolved inherited context), apply overrides to that, and write the full context back.
Reviewed by Cursor Bugbot for commit 7f0a2f0. Configure here.
BroadwayCore/Sources/BContext.swift
Outdated
| traitOverrides = overrides | ||
| self.themes = themes | ||
| stylesheets = BStylesheets(config: .init(traits: traits, themes: themes)) | ||
| stylesheets = BStylesheets(traits: traits, themes: themes) |
There was a problem hiding this comment.
Stylesheets initialized with un-merged traits in init
Low Severity
BContext.init initializes stylesheets with the raw traits parameter (base traits only), not traits.merging(with: overrides). Since didSet doesn't fire during init, when non-empty overrides are passed, stylesheets.traits won't reflect the overrides. This is inconsistent with the didSet on baseTraits and traitOverrides, which both set stylesheets.traits = traits using the merged computed property.
Reviewed by Cursor Bugbot for commit 7f0a2f0. Configure here.
- Merge main agent tooling (sync-agents, nested AGENTS.md, external skills) - Resolve AGENTS.md and README.md conflicts with flat layout trees - Keep README Targets table; document AI skills via sync-agents Made-with: Cursor
- Seed BStylesheets with merged traits when BContext init takes overrides. - Apply trait overrides from inherited parent bContext and republish full context. - Prefer parent traitCollection when resolving inherited BContext during viewIsAppearing. - Make BTraitsObserver.start/stop idempotent at the coordinator level. - Add BContext and show()-based BroadwayUI tests for the above. Made-with: Cursor
| extension BMode: BTraitsValue { | ||
| public static let defaultValue: BMode = .light | ||
|
|
||
| @MainActor public static func currentValue(from viewController: UIViewController) -> BMode { |
There was a problem hiding this comment.
This isn't consistent with below, which is just `from1?
| } | ||
| } | ||
|
|
||
| extension BTraitsValue where Observer == NeverObserver { |
There was a problem hiding this comment.
I think we should remove this default, every trait should have an observable observer? Maybe we default it to a system trait where you can provide a key path? Thoughts?
| var isVoiceOverRunning: Bool | ||
|
|
||
| init(context: SlicingContext) { | ||
| isVoiceOverRunning = context.stylesheets.traits.accessibility.isVoiceOverRunning |
There was a problem hiding this comment.
Can we make traits on stylesheets private? I'd rather we access this from the context directly.
|
|
||
| let context = BContext(traits: base, overrides: overrides) | ||
|
|
||
| #expect(context.stylesheets.traits.accessibility == BAccessibility(isVoiceOverRunning: true)) |
There was a problem hiding this comment.
Same comment, no one should really be accessing this outside of internals
| private func makeObserver( | ||
| onChange: @MainActor @escaping @Sendable (BTraits) -> Void = { _ in }, | ||
| ) -> BTraitsObserver { | ||
| BTraitsObserver(traits: .system, from: UIViewController(), onChange: onChange) |
There was a problem hiding this comment.
UIViewController() is weak here so it's not preserved long enough to matter, right? Let's
- Make the view controller in its own line
- present it via
show - etc...
| observer.stop() | ||
| } | ||
|
|
||
| @Test("Calling start twice is safe") |
There was a problem hiding this comment.
Nothing actually verifies this is safe? Eg are we actually not listening to notifications or callbacks anymore?
| private let makeContent: () -> Content | ||
| private let overrides: (Context, inout BTraits.Overrides) -> Void | ||
|
|
||
| private func setUpIfNeeded() { |
There was a problem hiding this comment.
I don't love this being always lazy; can we maybe move some of it to init or didMoveToParentViewController or something?
| sources: ["BroadwayTesting/Sources/**"], | ||
| dependencies: [ | ||
| .target(name: "BroadwayCore"), | ||
| .xctest, |
- Align BMode.currentValue with BContentSizeCategory via BMode.from(_: UIUserInterfaceStyle). - Require explicit BTraitsValue.makeObserver (drop NeverObserver default extension); update ScaleFactor tests. - Make BStylesheets traits storage private; sync via updateTraits from BContext only; stop reading traits from tests. - BTraitsObserverTests: use show() with a retained anchor VC; assert coordinator double-start only calls underlying start once (CountingStartSpy). - BTraitOverridesViewController: apply overrides from didMove(toParent:) when embedded. - Restore BroadwayTesting dependency on .xctest (matches main). Made-with: Cursor
- Make themes storage private; add updateThemes like updateTraits. - Theme propagation test resolves theme via lazy TestStylesheet get(). Made-with: Cursor
- Mark updateTraits/updateThemes as package; document Tuist -package-name. - Apply OTHER_SWIFT_FLAGS -package-name Broadway to all first-party targets so package symbols compile outside a SwiftPM Package.swift layout. Made-with: Cursor
- Add Package.swift with BroadwayCore, BroadwayUI, BroadwayTesting (iOS 26 + Mac Catalyst); target paths use */Sources so Tests stay out of libraries. - Tuist Project.swift: local package at repo root; Catalog/TestHost/CoreTests/UITests/CatalogTests depend on package products; drop generated framework targets and -package-name flags. - Sendable: BMode/BContentSizeCategory; SettingsProvider + @unchecked Sendable system/mock providers for SPM concurrency checks. - Update AGENTS.md and README for hybrid SPM + Tuist workflows and scheme names. Made-with: Cursor
| let context = BContext(traits: base, overrides: overrides) | ||
|
|
||
| #expect(context.traits.accessibility == BAccessibility(isVoiceOverRunning: true)) | ||
| _ = try context.stylesheets.get(TestStylesheet.self) |
There was a problem hiding this comment.
Not sure this line adds anything
1 participant
Summary
BTraitsValueObserverprotocol andObserverassociated type onBTraitsValue, so each trait type declares its own observer. ANeverObserverdefault means non-observed traits need no extra work.BTraitsObserveras a unified coordinator that reads registered trait types fromBTraits, initializes values from the view controller hierarchy viacurrentValue(from:), and manages per-type observers behind a singlestart()/stop()lifecycle.BTraitsgains a registration system (.systemfactory,register<V>(),readCurrentValues(from:)) so trait type configuration is centralized and extensible.BRootViewControlleris simplified to a singleBTraitsObserver(traits: .system, from: self)call, removing directBAccessibility.Observermanagement.BAccessibility.observe(on:with:_:)factory in favor of directObserverinit with defaults.AGENTS.mdandREADME.md.Test plan
BTraitsObserverTestscover initial value population and lifecycle safetyBAccessibilityObserverTestsupdated to useObserverinit directlyBRootViewControllerTestspass unchanged (behavioral parity)Made with Cursor
Note
Medium Risk
Touches core environment/trait propagation and introduces new observation/bridging paths, so regressions could affect UI appearance and trait-driven styling across the app, though changes are mostly additive and covered by updated tests.
Overview
Introduces a unified, protocol-driven trait observation system:
BTraitsnow supports registration (.system,register,readCurrentValues) and eachBTraitsValueprovidescurrentValue+ aBTraitsValueObserver, coordinated by the newBTraitsObserver.Updates
BContextto separatebaseTraitsfrom per-subtreetraitOverrides(resolved viatraits), and adjusts stylesheet caching/equality to ignore internal caches while still invalidating on trait/theme changes.Refactors
BRootViewControllerto lazily create its child and initialize/maintain context only once it’s in a real hierarchy, driven byBTraitsObserver(replacing direct accessibility observation). Adds SwiftUI and UIKit APIs to override traits (EnvironmentValues.bContext,View.bTraitOverrides/bMode/bContentSizeCategory, andBTraitOverridesViewController).Cleans up test utilities to use Swift Testing-style helpers (
show,waitFornow throwing) and updates/adds tests accordingly; docs/project manifest are adjusted and some placeholder tests/entry files removed.Reviewed by Cursor Bugbot for commit 7f0a2f0. Bugbot is set up for automated code reviews on this repo. Configure here.