Repository navigation
feat(replay): schedule capture from a main run-loop observer instead of swizzling UIView layout - #896
Conversation
|
[Medium risk] Replaces UIView swizzling with run-loop observer for replay and surveys. This should not merge until replay resets capture backoff when an active recording enters a new session. Reviews (1) · Last reviewed commit: "chore: add changeset for the run-loop ca..." |
posthog-ios Compliance ReportDate: 2026-11-06T11:08:58.424448+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 4377ms |
| Format Validation.Event Has Uuid | ✅ | 2723ms |
| Format Validation.Event Has Lib Properties | ✅ | 531ms |
| Format Validation.Distinct Id Is String | ✅ | 2762ms |
| Format Validation.Token Is Present | ✅ | 2733ms |
| Format Validation.Custom Properties Preserved | ✅ | 2868ms |
| Format Validation.Event Has Timestamp | ✅ | 2799ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 336ms |
| Retry Behavior.Retries On 503 | ✅ | 11501ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 4853ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2484ms |
| Retry Behavior.Respects Retry After Header | ✅ | 5439ms |
| Retry Behavior.Implements Backoff | ✅ | 19190ms |
| Retry Behavior.Retries On 500 | ✅ | 9453ms |
| Retry Behavior.Retries On 502 | ✅ | 9332ms |
| Retry Behavior.Retries On 504 | ✅ | 5516ms |
| Retry Behavior.Max Retries Respected | ❌ | 21815ms |
| Deduplication.Generates Unique Uuids | ✅ | 2967ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 9345ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 10362ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 8011ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 3240ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 2944ms |
| Compression.Sends Gzip When Enabled | ✅ | 537ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 382ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 368ms |
| Batch Format.Multiple Events Batched Together | ✅ | 3010ms |
| Error Handling.Does Not Retry On 403 | ✅ | 4874ms |
| Error Handling.Does Not Retry On 413 | ✅ | 4964ms |
| Error Handling.Retries On 408 | ✅ | 9670ms |
Failures
retry_behavior.max_retries_respected
Expected 4 requests, got 6
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 2889ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 3202ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 2982ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 2895ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 2793ms |
| Request Payload.Groups Round Trip | ✅ | 2847ms |
| Request Payload.Groups Default To Empty Object | ✅ | 2827ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 2804ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 2760ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 2875ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 97ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 392ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 5638ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 2897ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 2871ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 2831ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 3097ms |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed db68cde: 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
|
@PostHog review --deep |
|
@ioannisj PostHog Review is reviewing this pull request. |
PostHog Review · Deep · reviewed
|
| Step | Status | Result |
|---|---|---|
| Prepare the diff | done | 21 files, 5 chunks |
| Pick perspectives | done | 3 perspectives |
| Run review passes | done | 6 issues |
| Merge overlapping findings | done | 6 → 4 |
| Validate findings | done | 1 kept, 3 dismissed |
| Publish | done | 1 posted (view review) |
ioannisj
left a comment
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Pushed fixes for the open threads. 98ec4ca clamps the replay capture interval, so a zero or non-finite throttleDelay can't keep the run loop awake on an idle screen. 66f8e4a lets a backed-off screen notice touchless changes like toasts through a cheap view-tree fingerprint, which catches 5/5 toasts on a simulator again, same as main.
turnipdabeets
left a comment
There was a problem hiding this comment.
I ran the five touched suites at 66f8e4ae0 on the iPhone 17 Pro simulator, and all 116 tests pass. A few things before I approve:
- CI hasn't run on
98ec4ca13or66f8e4ae0. The last Build/Tests run is ondb68cde8. Could you re-trigger it, or rebase? The branch is 55 behind main but merges cleanly. - [suggestion] The
enableSwizzlingdoc inPostHogConfig.swift:169still lists surveys among the features that need swizzling. That isn't true after this PR. - I'll rerun my toast test on device against the fingerprint change and report back here.
dustinbyrne
left a comment
There was a problem hiding this comment.
Shared observer ownership and replay/Survey isolation look sound, and the session-reset and zero-interval concerns are addressed in source.
A paired real-UIKit scheduler test still loses a 2.2-second color state after idle: base captures it, head never admits a render during it, and a same-timing text-change control succeeds. This is scheduler-level evidence, not full SDK delivery. Consider correcting the narrow case or explicitly accepting the fidelity tradeoff before approval. There is also a small public-option documentation contradiction.
AI-assisted review.
…eplay-scheduler # Conflicts: # PostHog/Replay/PostHogReplayIntegration.swift # PostHogTests/ApplicationViewLayoutPublisherTest.swift
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
…eplay-scheduler # Conflicts: # PostHog/Replay/PostHogReplayIntegration.swift # PostHogTests/ApplicationViewLayoutPublisherTest.swift
dustinbyrne
left a comment
There was a problem hiding this comment.
The earlier color-fingerprint and swizzling-documentation concerns are addressed. No remaining actionable findings at this snapshot.
AI-assisted review.
|
@turnipdabeets wanna have a final closer look on the concerns above before I get this merged? |
turnipdabeets
left a comment
There was a problem hiding this comment.
Had a final look, all good from my side. My earlier points are addressed, and I checked the new layer fingerprint on a device: it stays stable on idle UIKit and SwiftUI screens, so the backoff holds. 0.08 on energy is fine for now; driving the wake from the backoff interval could be a nice follow-up whenever. Feel free to merge.
Brings in #896 (run-loop replay/survey capture scheduler) and the 3.94.0 release. Conflicts: - ApplicationViewLayoutPublisherTest.swift: kept main's deletion. - PostHogSurveysTest.swift, PostHogSurveyEventsTest.swift: took main's changes on top of 4.0's Surveys trait gate. - PostHog.podspec: kept deleted (4.0 is SPM-only). - PostHogVersion.swift: kept the SPI declaration, version 3.94.0. 4.0 adaptations: - Replay frameHash hashes only the screenshot; dropped the wireframe tree hash, since 4.0 has no wireframe mode. - PostHogReplayScreenshotDedupTest: dropped wireframe-tree tests and moved the backoff/window-switch tests to RRScreenshotFrame. - Gated new replay/surveys code and tests behind the SessionReplay and Surveys package traits; added the PostHogInternal SPI import where the tests use the session manager.
Conflict resolutions: - UIView+Util toImage: keep the renderer switch and record drawHierarchy's result in both drawHierarchy branches, so #999's failed-draw skip applies to the fidelity and bridge renders. - Replay integration: becoming active wakes the capture backoff (#896), then resumes plugins and prewarms GPU capture; collectScreenshotMetadata keeps the renderer API and skips a tick whose attempted render failed (#999). - Tests: touch tests use the shared ReplayTestSupport helpers with main's new cases, PresentingController, makeWindow and the maskAllTextInputs parameter; the screenshot size test draws only the presentation-tree renderer, since failed drawHierarchy renders are now dropped in the hostless bundle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019xLynxqQ97w5iqqo4Dxyi5
💡 Motivation and Context
Closes #813.
Session replay and surveys both scheduled their work by exchanging the implementation of
UIView.layoutSublayers(of:).ApplicationViewLayoutPublisherinstalled that swizzle whenever either product had a subscriber, which put SDK code on one of UIKit's hottest methods and made PostHog part of every host app's layout call chain.Capture now comes from a
CFRunLoopObserveron the main run loop instead. Nothing in the SDK subscribes to the layout publisher any more, so the swizzle is never installed.Changes:
ApplicationRunLoopPublisher, shaped like the otherApplication*Publishertypes. It observes the main run loop incommonModes, hands out at most one opportunity per cycle atbeforeWaiting/exit, installs its observer only while something is subscribed, and pauses while the app is backgrounded.subscribe(throttle:trailing:)call it used on the layout publisher, so the delivered cadence is unchanged.PostHogReplayCaptureBackoff, owned by replay, because a run-loop observer fires whether or not the screen changed. After three consecutive frames that the existing dedup rejects, the interval doubles fromthrottleDelayup to an 8s cap. Any frame that differs restores the full rate, and a touch leaves backoff immediately. It never stops permanently.ApplicationViewLayoutPublisher, its DI entry and its tests.requiresSwizzling, since it subscribes only to the run-loop publisher and the lifecycle publisher, neither of which swizzles. Apps that setenableSwizzlingto false will now get surveys instead of having the integration skipped. Replay still reports it, becauseApplicationEventPublisherswizzlesUIApplication.sendEventfor touch capture.The renderer, masking, deduplication, throttling and in-flight protections are untouched. This changes when capture is considered, not what is captured. No public API change.
Relates to #843, which keeps the swizzle and adds a timer next to it. I measured both on a device before picking this one.
Three decisions worth calling out.
Backing off rather than stopping. An earlier revision stopped capturing after a few unchanged frames and waited for a touch. That is safe while the layout swizzle exists, because a layout wakes it again. With the swizzle gone the only remaining external signal is a touch, so a screen that starts changing on its own is never captured again. On a device a colour change 12s into an idle screen was missed entirely.
Hashing the wireframe tree, not just the screenshot.
screenshotModedefaults to false, so wireframe is the default. The dedup hash came from the screenshot'sbase64, which wireframe never produces, so the backoff could never engage in the default configuration. It now hashes the wireframe payload too, excludingidat every depth because it comes fromUIView.hashand would pin the verdict at changed forever.Fingerprinting layers as well as views. A backed-off screen leaves backoff early only on a touch or a changed structure fingerprint, and SwiftUI updates reach the screen through layers without touching any
UIViewproperty. The fingerprint now also reads each view's layer content and any sublayers with no view behind them, within the existing 1000-node budget and a depth limit of 64.animationKeys()is left out so a running animation alone doesn't leave backoff.💚 How did you test it?
Unit tests for the publisher's lifecycle and the backoff, including a regression test that drives it into deep backoff, never touches the screen and asserts opportunities keep arriving.
PostHogSurveysRunLoopTestcaptures thelayoutSublayers(of:)implementation beforesetupand asserts it is unchanged afterwards.910 tests pass on SPM and 1700 in the full simulator bundle.
Measured on a physical iPhone 17 Pro, iOS 26.7, Release, masking on,
throttleDelay1s, surveys and replay both enabled, againstmainand against #843. Frame timing from aCADisplayLink, CPU sampled in process, ten scenarios per build.The swizzle is gone, sampled every second for every run, in both capture modes:
mainIt costs nothing in smoothness. Slow-frame percentage scales with capture count, so the comparable figure is slow frames per capture:
mainAbout two dropped frames per capture either way, and p99 frame time sits at 66 to 73ms for every build, which is the render itself.
A quiet screen stays cheap, and nothing is given up to get there:
mainWireframe mode was measured separately in
PostHogExampleStoryboard, since the SwiftUI example app'sUIHostingControllerroot makes replay refuse wireframe capture entirely. The backoff engages there too: a static screen attempts 10 captures in 40s against 40 on a changing screen, with 9 of the 10 discarded as duplicates.I also compared uploaded frames against
mainon an iPhone 17 Pro and an iOS 26.5 simulator. For a 2s SwiftUI change on an idle screen,maincaught 20 of 20 and this PR catches 20 of 20, up from 5 of 21 with the view-only fingerprint.One known difference from
main: a looping Core Animation on an idle screen stays at about 1 frame per second, because each frame differs and resets the backoff.maincaptures none of it. In a 150s simulator run that was 140 frames against 2. A busy screen already runs at that rate, so I think this is a cost rather than a correctness gap. Worth a follow-up to stop animation-only frames resetting the backoff.Not covered: React Native, and I didn't measure energy impact.
One caveat. A survey test failed intermittently twice during this work and passes on
main. I fixed a real leak behind it, test integrations staying subscribed to the shared publisher for the rest of the process, but the pre-fix baseline was 0 failures in 6 full-bundle runs, so the fix is unproven rather than verified. Worth watching in CI.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
DRI: @ioannisj
Autonomy: Human-driven (agent-assisted)