Skip to content

fix(device): read UIKit traits on the main thread and cache them - #497

Open
yusuftor wants to merge 11 commits into
developfrom
fix/device-helper-main-thread-traits
Open

fix(device): read UIKit traits on the main thread and cache them#497
yusuftor wants to merge 11 commits into
developfrom
fix/device-helper-main-thread-traits

Conversation

@yusuftor

@yusuftor yusuftor commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Changes in this pull request

  • interfaceStyle, fontSize, fontScale and preferredContentSizeCategory read UIScreen.traitCollection, UIFontMetrics and UIApplication.preferredContentSizeCategory directly. Those APIs are main-thread only, but the getters are called from background contexts such as getTemplateDevice(), which trips the Main Thread Checker.
  • Reads the four values once on the main thread into a cached UITraits, refreshed on UIContentSizeCategory.didChangeNotification and UIApplication.didBecomeActiveNotification so they still track the user changing their text size or appearance. Observers are removed in deinit.
  • Extracts interfaceStyleToken(for:) next to the existing contentSizeCategoryToken(for:), so both dashboard token mappings are directly testable — these strings are a backend audience-filter contract and must not drift.
  • Adds three tests: exhaustive interfaceStyleToken mapping, reading all four traits off the main thread, and asserting the cache holds the device's real values rather than the visionOS placeholders.

Checklist

  • All unit tests pass.
  • All UI tests pass.
  • Demo project builds and runs on iOS.
  • Demo project builds and runs on Mac Catalyst.
  • Demo project builds and runs on visionOS.
  • I added/updated tests or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have run swiftlint in the main directory and fixed any issues.
  • I have updated the SDK documentation as well as the online docs.
  • I have reviewed the contributing guide

Unchecked boxes are not verified rather than not applicable — only the unit tests (896 passing on iPhone 17 Pro / iOS 26.5), the added tests, the changelog entry and swiftlint were actually run. The demo projects and UI tests weren't exercised. No public API changed, so no doc updates were needed.

🤖 Generated with Claude Code

Greptile Summary

This PR moves UIKit trait reads onto the main thread and caches the resulting device metadata.

  • Refreshes the cache for content-size, activation, and iOS 17 trait changes.
  • Adds active-scene selection and lifecycle cleanup for observers.
  • Extracts interface-style token mapping and adds background-read, cache, and override tests.
  • Documents the Main Thread Checker fix in the changelog.

Confidence Score: 4/5

The PR is not yet safe to merge because appearance tracking can remain attached to an inactive scene, while the previously reported window-override path also remains unresolved.

DeviceHelper treats any still-existing observed scene as current, so activation cannot move trait registration to a newly active scene and later appearance changes there leave backend-facing traits stale. The earlier window-override issue also remains because the implementation intentionally observes a scene even though window overrides do not propagate to it.

Files Needing Attention: Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift; Sources/SuperwallKit/Misc/Extensions/UIApplication+ActiveWindow.swift

Important Files Changed

Filename Overview
Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift Adds synchronized UIKit trait caching and refresh registration, but the registration can remain bound to an old scene and window-level appearance changes remain unobserved.
Sources/SuperwallKit/Misc/Extensions/UIApplication+ActiveWindow.swift Adds foreground-prioritized UIWindowScene selection used by trait registration.
Tests/SuperwallKitTests/Network/DeviceHelperTests.swift Adds coverage for token mapping, off-main-thread access, cached values, repeated refreshes, and interface-style overrides.
CHANGELOG.md Documents the Main Thread Checker warning fix.
Prompt To Fix All With AI
### Issue 1
Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:309-311
**Registration stays on old scene**

When a multi-window host keeps the original `UIWindowScene` alive while another scene becomes foreground-active, this guard treats the old registration as current. Appearance changes in the new active scene then leave `uiTraits` stale, causing device attributes and request headers to report the previous Light or Dark token and select the wrong audience behavior.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (4): Last reviewed commit: "test(device): name the template trait te..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

`interfaceStyle`, `fontSize`, `fontScale` and `preferredContentSizeCategory`
read `UIScreen.traitCollection`, `UIFontMetrics` and
`UIApplication.preferredContentSizeCategory` directly. Those are main-thread
only, but the getters are called from background contexts such as
`getTemplateDevice()`, which trips the Main Thread Checker.

Reads them once on the main thread into a cached `UITraits` value, refreshed on
`UIContentSizeCategory.didChangeNotification` and
`UIApplication.didBecomeActiveNotification` so the values still track the user
changing their text size or appearance. Observers are torn down in `deinit`.

Extracts `interfaceStyleToken(for:)` alongside the existing
`contentSizeCategoryToken(for:)` so both dashboard token mappings are directly
testable — these strings are a backend audience-filter contract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift
…t stick

`UIContentSizeCategory.didChangeNotification` and `didBecomeActiveNotification`
cover a change made via Settings or Control Center, since both cycle the app
through resign/become-active. They miss a system appearance change that lands
while the app stays active — the automatic light/dark switch at sunset — leaving
the cache reporting the previous token until the next foreground, which feeds
the wrong value into request headers and audience filters.

Reads now schedule a main-thread refresh, so the cache trails a change by one
read rather than indefinitely. A test-and-set guard keeps a burst of reads to a
single main-thread hop, and the read itself stays non-blocking.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Moving these reads off the hot path is the right call, but swapping a live read for a cache narrows when interfaceStyle is correct. The two observed notifications don't cover a system appearance flip that happens while the app stays active, so X-Device-Interface-Style and the interfaceStyle device attribute can report the previous value until the next activation. Worth resolving before merge.

Reviewed changes — full review of the single commit afd9c05 (3 files) against develop.

  • Four trait reads replaced with a cached UITraitsinterfaceStyle, fontSize, fontScale and preferredContentSizeCategory now return values captured from a @DispatchQueueBacked cache instead of touching UIScreen.traitCollection / UIFontMetrics / UIApplication.preferredContentSizeCategory on whatever thread the getter runs on.
  • Cache populated in init on the main threadmakeUITraits() uses the same Thread.isMainThread ? read() : DispatchQueue.main.sync(...) shape as the pre-existing makeScreenMetrics() in the same initializer, and the wrapper assignment in init correctly lowers to direct backing-storage construction rather than the wrapper's queue.async setter, so the initial value can't be lost.
  • Cache refreshed via two notification observersUIContentSizeCategory.didChangeNotification and UIApplication.didBecomeActiveNotification, both delivered on .main, removed in deinit. The observer blocks capture self weakly, so no retain cycle.
  • interfaceStyleToken(for:) extracted as an internal static alongside the existing contentSizeCategoryToken(for:), making both backend-contract token maps directly testable.
  • Three new tests — exhaustive interfaceStyleToken mapping, an off-main read of all four values, and a main-thread check that the cache holds the device's real values rather than the visionOS placeholders.
  • Numeric behaviour preservedfontSize and fontScale compute identically to the code they replace, from a single scaledValue(for: 16.0) call.

ℹ️ Nothing exercises the cache-invalidation path

The new tests cover the token maps and the freshly-populated cache, but observeUITraitChanges() — the part that can silently stop working, and the reason the cached values stay correct at all — has no coverage. It also isn't reachable from a test as written: makeUITraits() is private static and the only refresh entry point is a NotificationCenter block.

Technical details
# `observeUITraitChanges()` is untested and untestable as written

## Affected sites
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:259-275``observeUITraitChanges()`; the refresh is only reachable by posting a real notification.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:234``makeUITraits()` is `private static`, so a test can't drive a refresh directly.
- `Tests/SuperwallKitTests/Network/DeviceHelperTests.swift:83-131` — the three new tests only touch the token maps and the post-`init` cache.

## Required outcome
- A regression that breaks the refresh (observer never registered, wrong notification name, refresh reading from the wrong source) fails a test rather than shipping silently.

## Suggested approach (optional)
- Extract an internal `refreshUITraits()` that assigns `uiTraits = Self.makeUITraits()`, have `observeUITraitChanges()` call it, and assert in a test that after calling it the four values still equal the live UIKit values (the assertions `traits_matchTheCurrentTraitValues` already makes). That pins the refresh source without depending on notification delivery.
- Avoid asserting only "the value is unchanged after posting the notification" — that passes even if the refresh is a no-op.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift
Comment thread Tests/SuperwallKitTests/Network/DeviceHelperTests.swift Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Two rough edges in the new refresh-on-read mechanism — neither blocking.

Reviewed changes — incremental review of 815dc10 only, the response to the interface-style staleness finding on afd9c05.

  • Reads now trigger a refresh — the four getters go through a new private currentUITraits, which calls refreshUITraits() and returns the previously cached value; the notification observers are kept alongside it.
  • Refreshes are coalescedguard !$isRefreshingUITraits.testAndSetTrue() gates a single DispatchQueue.main.async hop per burst of reads. The latch can't be lost: testAndSetTrue() is queue.sync and the reset is queue.async on the same private serial queue, so FIFO ordering guarantees a later testAndSetTrue() observes the reset. The UIKit read still happens on the main thread, so the original Main Thread Checker fix is intact.
  • One new testtraits_stayCorrectAfterRepeatedReadsTriggerRefreshes drives five off-main read/refresh cycles and re-checks the four values against live UIKit values.
  • Doc comments updated to describe the new "refreshed on notifications and on read" behaviour.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift
Comment thread Tests/SuperwallKitTests/Network/DeviceHelperTests.swift Outdated
…t they happen

Refresh-on-read only narrowed the staleness window: the first read after an
unobserved appearance flip still reported the old token, and with no traffic
between the flip and a paywall presentation that read is the `getTemplateDevice()`
call feeding audience filters.

There is no notification for `userInterfaceStyle`, so this registers for trait
changes on the active window (iOS 17+) and updates the cache at the moment of the
flip. Registration needs a window, which may not exist when `DeviceHelper` is
built, so it retries on activation and no-ops once registered.

Refresh-on-read stays as the backstop for iOS 13-16 and for the window between
launch and registration. It now takes an inline fast path on the main thread —
the wrapper's setter and getter share one serial queue, so the write is ordered
ahead of the read and main-thread callers see the live value.

Also strengthens `traits_areReadableOffTheMainThread`, whose assertions passed
against the `UITraits.unavailable` placeholders and so only proved the off-main
read didn't hang.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The trait hook is the right mechanism for closing the staleness gap, but it observes a UIWindow — an object whose lifetime it doesn't control, and whose userInterfaceStyle the host app can pin. In both of those cases it silently never fires for the sunset appearance flip it was added to catch, and the one-shot guard means it can't recover.

Reviewed changes — incremental review of 2c3ee93, the response to the two rough edges raised on 815dc10.

  • Main-thread reads refresh inlinerefreshUITraits() takes a Thread.isMainThread fast path that assigns uiTraits directly and returns, so main-thread callers (TestModeDeviceAttributesViewController, a main-thread Superwall.getDeviceAttributes()) see the live value instead of trailing by one read. The latch is bypassed on that path and can't be left stuck: any block that would clear it is already enqueued on the main queue.
  • Added an iOS 17+ trait-change hookregisterForTraitChanges() hops through Task { @MainActor } to performTraitChangeRegistration(), which registers [UITraitUserInterfaceStyle.self, UITraitPreferredContentSizeCategory.self] on UIApplication.sharedApplication?.activeWindow and refreshes the cache from the handler. This is what updates the cache at the moment of a flip rather than on the next read.
  • Registration is retried from the notification handlers — a window may not exist when DeviceHelper is constructed, so the observer block calls registerForTraitChanges() again; traitChangeRegistration == nil makes it a no-op once registered.
  • Off-main read test tightenedtraits_areReadableOffTheMainThread now compares all four detached-task values against freshly read UIKit values instead of isEmpty == false / > 0, so the visionOS placeholders no longer satisfy it.
  • Doc comments rewritten to describe the inline-vs-scheduled refresh split and to name the trait hook as the primary path with refresh-on-read as the backstop.

ℹ️ One cached value now has four refresh paths, two of them untested

init, the two notification observers, refresh-on-read, and the trait hook all write uiTraits, with overlapping coverage — UITraitPreferredContentSizeCategory in the hook duplicates UIContentSizeCategory.didChangeNotification, and refresh-on-read partly duplicates both. Nothing asserts that either the observers or the registration are wired up at all, so the two paths that can silently stop working are exactly the two with no test.

Technical details
# Refresh-path redundancy and coverage

## Affected sites
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:338-353``observeUITraitChanges()`; `UIContentSizeCategory.didChangeNotification` is now also covered by `UITraitPreferredContentSizeCategory` in the hook. `didBecomeActive` still earns its place (pre-iOS-17 refresh + registration retry).
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:267-292` — the registration path has no test.
- `Tests/SuperwallKitTests/Network/DeviceHelperTests.swift:83-175` — the four tests cover the token maps, the post-`init` cache and the read-triggered refresh only.

## Required outcome
- A regression that breaks a refresh path (observer never registered, registration never armed, refresh reading from the wrong source) fails a test rather than shipping silently.
- Each retained refresh path is retained deliberately, not accumulated.

## Suggested approach (optional)
- Make the refresh entry point internal and assert that after calling it the four values still equal the live UIKit values — that pins the refresh source without depending on UIKit delivery. Injecting the trait environment (default `activeWindow`) would additionally make the registration assertable.
- If the notification observers are being kept purely as the pre-iOS-17 path, say so in the doc comment so a later reader doesn't delete one as redundant.

ℹ️ Nitpicks

  • registerForTraitChanges()'s doc comment says the retry runs "on activation", but the same handler is installed for UIContentSizeCategory.didChangeNotification too, so a text-size change also retries. Both notification handlers already run on .main, so the extra Task { @MainActor } hop there only defers a call that could be made directly.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift Outdated
yusuftor and others added 3 commits July 30, 2026 16:47
Observing the active window was wrong in two ways.

The window is a different trait environment from the one the cache reads. A
window pinned via `overrideUserInterfaceStyle` reports no change in that trait
when the system flips, so the handler never fired even though `UIScreen.main` —
what `makeUITraits()` actually reads — had changed. Overrides propagate down the
trait hierarchy, not up, so the scene's `userInterfaceStyle` follows the system
the way the screen does. Adds `UIApplication.activeWindowScene` for it, mirroring
`activeWindow`'s activation-state priority.

`activeWindow` is also computed per call with no identity guarantee, and UIKit
removes a registration when the object that created it deallocates. The
`traitChangeRegistration == nil` guard then read a dead registration as a live
one and never re-armed. The observed scene is now held weakly, and a non-nil
token with a nil scene counts as needing re-arming, which the existing activation
retry picks up. Scenes also outlive the windows they host, so this arises less
often to begin with.

No `deinit` unregister: UIKit cleans registrations up at end of lifecycle.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The refresh is enqueued on the main queue before the hop, so hopping to the main
actor drains it without a sleep. Removes five 50ms sleeps and the timing
dependency they carried.

Also scopes the docstring to what the test actually pins: nothing here can move
`UIScreen.main`'s trait collection, so it proves the refresh keeps the cache
correct, not that a flip is picked up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℓ No new issues of substance — one minor suggestion inline on the new scene helper.

Reviewed changes — incremental review of dad046b, the response to the [!IMPORTANT] trait-hook finding on 2c3ee93.

  • The trait hook now observes the UIWindowScene, not a UIWindowperformTraitChangeRegistration() registers [UITraitUserInterfaceStyle.self, UITraitPreferredContentSizeCategory.self] on activeWindowScene. This is the right target: window-level overrideUserInterfaceStyle doesn't propagate up to the scene, so the scene's userInterfaceStyle tracks the system the way UIScreen.main.traitCollection — the value makeUITraits() actually reads — does. Verified UIWindowScene conforms to UITraitChangeObservable (conforming-types list), so the registration is valid.
  • Registration is re-armableprivate weak var observedTraitScene plus the traitChangeRegistration != nil && observedTraitScene != nil guard replaces the one-shot == nil check, so losing the observed scene re-registers instead of disabling the hook for the process lifetime. The re-arm is reachable in the case that motivated it: Apple's sceneDidBecomeActive(_:) docs state "UIKit posts a UIScene.didActivateNotification and a UIApplication.didBecomeActiveNotification", so a multi-window app whose observed scene disconnects while the app stays active still drives the existing observer.
  • New UIApplication.activeWindowScene helper — mirrors activeWindow's three-tier activation-state priority at the scene level. Written standalone rather than as activeWindow?.windowScene, which reads like duplication but isn't: activeWindow returns nil outright when the foreground-active scene has no windows yet, which is exactly the early-startup state DeviceHelper.init runs in.
  • No behavioural change to the cache itself — the four getters, makeUITraits(), the refresh-on-read latch and the notification observers are untouched, as is the test file.

ℓ Nitpicks

  • observeUITraitChanges()'s doc comment (DeviceHelper.swift:344-345) still says the activation retry helps because "a window is more likely to exist by then" — the registration target is a scene now.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

…le cache

Caching these values regressed freshness on versions with no trait hook. Before
this branch they were read live on every access, so `X-Device-Interface-Style`
and the audience filters keyed on it always saw the current appearance. On iOS
13-16 nothing invalidates the cache when the appearance flips while the app stays
active, so the first read after a flip reported the previous style — a regression
against develop, not a pre-existing gap.

Those versions now read live. `makeUITraits()` already hops to the main thread
itself, so this keeps the threading fix that motivated the branch: the read is on
the main thread rather than the undefined-behaviour background access it replaced.

iOS 17+ keeps the cache, kept current by the scene registration, with the
scheduled refresh covering the gap between launch and registration.

Trade-off: a blocking main-queue hop per read before iOS 17, and with it a
deadlock surface if the main thread is ever blocked waiting on the thread doing
the read. Judged the better side of reporting a wrong appearance to audience
filters.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Reading live on iOS 13–16 does close the staleness gap, but it turns every access into a blocking DispatchQueue.main.sync — four per getTemplateDevice() and one per network request, from async code on the cooperative thread pool. Worth collapsing before merge.

Reviewed changes — incremental review of ae03842, b805a17 and b6350d9, the three commits since the dad046b review.

  • Pre-iOS-17 reads bypass the cache entirelycurrentUITraits now returns DeviceHelper.makeUITraits() under guard #available(iOS 17.0, *) else, so iOS 13–16 gets a live read on every access instead of the cached value plus a scheduled refresh. This does close the "one stale read after a sunset appearance flip" gap on exactly the versions that have no trait hook, and the Main Thread Checker fix is intact — makeUITraits() still performs the UIKit reads on the main thread.
  • iOS 17+ path unchanged — still refreshUITraits() followed by the cached uiTraits, with the scene trait registration keeping the cache current.
  • Doc comments record the iOS 13–16 rationale (ae03842) — why a traitCollectionDidChange view isn't injected into the host's window to reach earlier releases.
  • Test determinism (b805a17) — traits_stayCorrectAfterRepeatedReadsTriggerRefreshes swaps five 50 ms Task.sleeps for await MainActor.run {}. The refresh is a DispatchQueue.main.async block enqueued before the hop, so the main-actor job drains it in FIFO order; the docstring now states plainly what the test can and can't pin, and the suite drops 250 ms.

ℹ️ On iOS 13–16 the trait cache is now write-only

Once currentUITraits short-circuits to a live read, nothing below iOS 17 ever reads uiTraits. init populates it, the two notification observers keep rewriting it, refreshUITraits() and isRefreshingUITraits are unreachable, and registerForTraitChanges() spawns a Task { @MainActor } per notification only to find its #available check false. The runtime cost is negligible; the comprehension cost of five write paths, four of them inert on the older half of the supported range, is not.

Technical details
# The trait cache and three of its refresh paths are inert below iOS 17

## Affected sites
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:214-215``@DispatchQueueBacked uiTraits` has no reader below iOS 17; the `init` write at `:755` and the observer writes are discarded there.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:219-220` and `:252-268``isRefreshingUITraits` and `refreshUITraits()` are only reachable from the iOS 17+ branch of `currentUITraits`, so the coalescing latch is never exercised below 17.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:358-373``observeUITraitChanges()` installs two main-queue observers that recompute and store `uiTraits` on every `didBecomeActive` and every content-size change. Below iOS 17 both the write and the `registerForTraitChanges()` retry are no-ops.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:208-213` — the `uiTraits` doc comment still presents the cache as *the* read path ("read on the main thread and cached, then refreshed on trait-change notifications and on read"), which has been true only from iOS 17 since `b6350d9`.

## Required outcome
- A reader can tell which mechanism is load-bearing on which OS range without deriving it from the `#available` branch in `currentUITraits`.
- Code that provably cannot affect behaviour on a supported OS version is either gated out on that version or documented as deliberately inert.

## Open questions for the human
- Is the cache meant to stay as a single code path that happens to be dead below 17, or should the two modes be separated explicitly? The answer decides whether the observers stay unconditional.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift
Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift Outdated
yusuftor and others added 2 commits July 31, 2026 14:08
The note still described the behaviour from before traits were read live
below iOS 17: those releases don't fall back to the notifications or to
refresh-on-read, and `refreshUITraits()` is unreachable there, so they're
correct on every read rather than after one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`getTemplateDevice()` read `interfaceStyle`, `fontSize`, `fontScale` and
`preferredContentSizeCategory` as four separate accesses. Before iOS 17
each goes live through `makeUITraits()`, which blocks off-main callers on
the main queue, so one template cost four round trips — and the callers
are async functions on the cooperative pool, which doesn't grow when its
threads block.

Read the snapshot once and take all four fields from it. That inlines the
`interfaceStyleOverride` short-circuit the `interfaceStyle` property
applied, so a test pins that the override still wins and the other three
still come from the device.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ The [!IMPORTANT] finding on b6350d9 is properly closed — two naming/duplication nits inline, neither blocking.

Reviewed changes — incremental review of 2b8ddd5 and 10aee46, the two commits since the b6350d9 review.

  • getTemplateDevice() takes one trait snapshotlet traits = currentUITraits (DeviceHelper.swift:800-803) replaces four independent property accesses. Below iOS 17 that collapses four blocking DispatchQueue.main.sync round trips per template into one, which was the ask on b6350d9. It also makes the four fields mutually consistent: previously a main-thread refresh landing mid-construction could mix values from two different trait snapshots into one template.
  • The interfaceStyleOverride short-circuit is inlined at the template's interfaceStyle fieldinterfaceStyleOverride?.description ?? traits.interfaceStyle (:826) reproduces var interfaceStyle exactly, and interfaceStyleMode (:858) is still derived independently from interfaceStyleOverride == nil.
  • currentUITraits' doc comment now says the hop blocks (:240-242) — "off the main thread that hop blocks the caller until the main queue drains. Read this once and reuse the snapshot rather than touching it per field." That was the second half of the same ask.
  • registerForTraitChanges()'s doc comment corrected (2b8ddd5, :284-285) — earlier releases read live on every access rather than falling back to the notifications, which is what the code has done since b6350d9.
  • New test templateDevice_usesOneTraitSnapshotAndKeepsTheInterfaceStyleOverride — pins that the override still wins for interfaceStyle while the other three fields come from the device, in both the override and no-override cases. The Double cast on fontScale is load-bearing: DeviceTemplate.toDictionary() round-trips through JSONSerialization, so the raw value is an NSNumber.

ℹ️ Nitpicks

  • DeviceHelperTests.swift:83-178 — the four pre-existing trait tests each construct their own DependencyContainer(), and this delta adds a fifth. Not worth changing here, but if a future commit adds another, a shared fixture would be cheaper than a sixth container.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift
Comment thread Tests/SuperwallKitTests/Network/DeviceHelperTests.swift Outdated
yusuftor and others added 2 commits July 31, 2026 14:26
`getTemplateDevice()` resolves `interfaceStyleOverride` by hand against
its trait snapshot, so the precedence rule now lives there as well as in
`interfaceStyle` — the property backing `X-Device-Interface-Style`. Note
the pairing at both ends, and record why folding them into a shared
`interfaceStyle(from:)` would be a regression: the argument evaluates
eagerly, adding a blocking main-queue hop per network request below
iOS 17 whenever an override is set.

Also assert the template and the property agree in both states, so the
duplication can't drift silently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The old name claimed the test pinned the one-snapshot read, but reverting
`getTemplateDevice()` to four separate property reads left every assertion
green. Name it after the override precedence and the device-derived
fields, which is what actually fails, and say outright that the snapshot
isn't pinned: `makeUITraits()` is private, so its hop count can't be
counted from a test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues — the duplication finding on 10aee46 is closed; one wording nit below.

Reviewed changes — incremental review of 477aa1c, the response to the duplicated override-precedence finding on 10aee46.

  • The interfaceStyleOverride precedence rule is documented at both sitesvar interfaceStyle (DeviceHelper.swift:189-196) now names X-Device-Interface-Style as what it backs, points at getTemplateDevice()'s parallel resolution, and records why the obvious dedupe is a trap (interfaceStyle(from: currentUITraits) would evaluate the snapshot eagerly, restoring a blocking main-queue hop per network request below iOS 17 whenever an override is set). The snapshot comment in getTemplateDevice() (:808-813) carries the same pairing note. Verified the claim: DependencyContainer.swift:441 is the sole X-Device-Interface-Style reader and it reads that property.

  • The two resolutions are now pinned against each other#expect(overridden["interfaceStyle"] as? String == deviceHelper.interfaceStyle) (DeviceHelperTests.swift:219) and its no-override counterpart (:207) add a failure mode the surrounding assertions don't cover: dropping the override short-circuit from the property leaves overridden["interfaceStyle"] == "Dark" green but fails the new comparison. Both sides read the same unchanged device traits, so there's no flake window.

  • No behavioural changecurrentUITraits, refreshUITraits(), makeUITraits(), the scene registration and the notification observers are untouched.

ℹ️ Nitpicks

  • DeviceHelper.swift:811-812 — the sentence uses interfaceStyle for both referents (the DeviceTemplate field and the property), distinguished only by single vs double backticks, and DocC symbol links don't render inside a // comment so interfaceStyle appears literally. Naming them ("the template's interfaceStyle field … rather than the interfaceStyle property") would read cleaner in a comment whose whole job is preventing drift.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

Comment on lines +309 to +311
if traitChangeRegistration != nil && observedTraitScene != nil {
return
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Registration stays on old scene

When a multi-window host keeps the original UIWindowScene alive while another scene becomes foreground-active, this guard treats the old registration as current. Appearance changes in the new active scene then leave uiTraits stale, causing device attributes and request headers to report the previous Light or Dark token and select the wrong audience behavior.

Knowledge Base Used: Networking Layer

Prompt To Fix With AI
This is a comment left during a code review.
Path: Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift
Line: 309-311

Comment:
**Registration stays on old scene**

When a multi-window host keeps the original `UIWindowScene` alive while another scene becomes foreground-active, this guard treats the old registration as current. Appearance changes in the new active scene then leave `uiTraits` stale, causing device attributes and request headers to report the previous Light or Dark token and select the wrong audience behavior.

**Knowledge Base Used:** [Networking Layer](https://app.greptile.com/superwall/-/custom-context/knowledge-base/superwall/superwall-ios/-/docs/networking-layer.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant