Delete the legacy iOS pixel-firing layer (iOS pixel migration, phase 3) - #6599
Open
federicocappelli wants to merge 8 commits into
Open
Conversation
Pixel, DailyPixel, UniquePixel, PersistentPixel, PersistentPixelStoring, and their PixelFiring/PixelFiringAsync/DailyPixelFiring protocols are gone, along with PixelFiringMock and the other legacy test mocks. Pixel.Event and PixelParameters survive - the catalogue of already- defined iOS pixel names and parameter keys every migrated pixel still reads from - trimmed into Pixel.swift alongside Pixel.BuildTarget, which is a load-bearing associated-value type on two existing Pixel.Event cases. Four production consumers hadn't moved off the legacy API yet, so this finishes that first: - OnboardingPixelReporter + its 1578-line test file: dual Pixel.self/ UniquePixel.self metatypes collapse into a single (any PixelKitFiring)?. Onboarding's "unique" pixels end in `_unique`, not `_u`, so `.legacyInitial` is the correct frequency - `.uniqueByName` asserts on `_u` and would drop them. - The VPN's PersistentPixelFiring seams (NetworkProtectionTunnelController, NetworkProtectionPacketTunnelProvider) convert to `PixelKit.fire(..., options: .withRetry)`. PixelKit's retry queue drains automatically after any successful fire, so the explicit sendQueuedPixels() calls in ReportingService and the tunnel provider are deleted rather than replaced. - WaitlistExtensions.swift picked up a PixelKit call, then was reverted: the surrounding package builds in no target, so migrating it was wasted work. Its removal is a separate PR (#6593). PixelTests.swift split three ways: legacy-mechanism tests deleted with the mechanism, two pixel-name spot-checks with no PixelKit dependency moved into PixelEventPixelKitTests.swift, and PixelKitLegacyNamingParityTests (the naming-contract suite pinning PixelKit's default naming to what legacy Pixel produced) kept as-is. .cursor/rules/pixels.mdc rewritten to describe PixelKit as the sole pixel API, PixelEvent.swift as closed to new cases, and the (any PixelKitFiring)?/PixelKitMock idiom as the pattern to follow. Verified: app build, test-target build, and the full local suite - 7180 XCTest + 144 swift-testing, 0 failures (down from 7220+143 pre-cleanup, accounted for by the deleted legacy test files). Known open items from review, not yet addressed: - originalPixelTimestamp is no longer added to the initial (non-retry) send of the 16 converted VPN pixels - PixelRetryQueue only adds it on replay, where legacy PersistentPixel added it on every send. - Those same 16 pixels are opted into options: .withRetry although neither retry parameter is declared in any iOS pixel definition (inherited from PersistentPixel, not newly introduced, but now explicit via the opt-in). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lsites' into fcappelli/pixelkit.migration.cleanup
Contributor
|
…pixelkit.migration.cleanup
…pixelkit.migration.cleanup
…pixelkit.migration.cleanup
Left over from the adapter-branch fix forward-merged in the previous commit; this branch already renamed the function and removed DailyPixel/ UniquePixel entirely in Phase 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5f0be47. Configure here.
| private func fire(event: Pixel.Event, unique: Bool, additionalParameters: [String: String] = [:]) { | ||
| // `.legacyInitial`: fires once ever, like legacy UniquePixel.fire, but without UniquePixel's | ||
| // `_u`/`_unique` name-suffix requirement (every onboarding "unique" case ends in `Unique`, | ||
| // whose name ends in `_unique` - `.uniqueByName` would assert-fail and drop these). |
Contributor
There was a problem hiding this comment.
Migration comments over-explain intent
Low Severity
New comments narrate this migration (UniquePixel vs .legacyInitial, sendQueuedPixels vs PixelKit's drain, why Pixel remains) rather than a lasting invariant. Situational notes like these are usually dropped in review.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 5f0be47. Configure here.
federicocappelli
changed the title
federicocappelli
changed the title
…pixelkit.migration.cleanup
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1205842942115003/task/1217858720586294
Stacked on #6550 (the call-site migration). Base branch is
fcappelli/pixelkit.migration.callsites, notmain, so this diff is scoped to this phase only.Description
Deletes the legacy iOS pixel-firing layer now that every call site fires through PixelKit. Gone:
Pixel,DailyPixel,UniquePixel,PersistentPixel,PersistentPixelStoring, theirPixelFiring/PixelFiringAsync/DailyPixelFiringprotocols, andPixelFiringMockplus the other legacy test mocks.Pixel.EventandPixelParameterssurvive trimmed into a 12-linePixel.swiftImportant:
PersistentPixelFiringseams (NetworkProtectionTunnelController,NetworkProtectionPacketTunnelProvider) convert toPixelKit.fire(..., options: .withRetry). PixelKit's retry queue drains automatically after any successful fire, so the explicitsendQueuedPixels()calls inReportingServiceand the tunnel provider are deleted.PersistentPixelstampedoriginalPixelTimestampon every send, including the first attempt; PixelKit's retry queue only adds it on replay. The initial send of the 16 converted VPN pixels no longer carries that parameter. Inherited behavior difference from the retry-queue design in PixelKit adapter for Pixel.Event (iOS pixel migration, phase 1) #6540, surfaced here because these are the first call sites to actually opt into.withRetry.Testing Steps
Impact and Risks
What could go wrong?
.legacyInitialdrops the_u/_uniquesuffix requirementUniquePixel.fireenforced. Checked every onboarding "unique" case (28 total, including the three call sites that pass a variable rather than a literal) against its wire name - all end in_unique, so nothing that was previously silently dropped starts firing..withRetrywithout either retry parameter (originalPixelTimestamp,retriedPixel) declared in an iOS pixel definition - carried over fromPersistentPixel, which had the same gap.Rollback: revert the PR. Nothing downstream depends on this phase.
Internal references: DoD | Eng Expectations | Tech Design Template
Note
Medium Risk
Large deletion in core telemetry and VPN paths; VPN pixels now use PixelKit retry (no
originalPixelTimestampon first send) and onboarding unique firing semantics changed to.legacyInitial.Overview
Removes the deprecated iOS pixel-firing stack (
Pixel,DailyPixel,UniquePixel,PersistentPixel, storage, and legacyPixelFiringprotocols/mocks) now that production code fires through PixelKit.Pixel.swiftis reduced toPixel.Event,Pixel.BuildTarget, andPixelParameters; firing stays onPixelEvent+PixelKit.swift.VPN and app lifecycle no longer depend on
PersistentPixelFiring:NetworkProtectionTunnelControllerinjects(any PixelKitFiring)?and fires controller start pixels withoptions: .withRetry.ReportingServicedropssendQueuedPixels()—PixelKit’s retry queue drains after successful fires.AppDependencyProviderremoves thepersistentPixeldependency.Onboarding telemetry drops the
OnboardingPixelFiringshim;OnboardingPixelReporterusesPixelKitFiringwith.legacyInitialfor once-ever onboarding events (replacingUniquePixelwithout requiring_u/_uniquesuffixes on names). Docs in.cursor/rules/pixels.mdcand PixelKit comments are updated to describe PixelKit as the sole API; the localPixelKitFiringtypealias file is removed in favor of PixelKit’s export.Tests for
DailyPixel,PersistentPixel, and related mocks are deleted with the implementations.Reviewed by Cursor Bugbot for commit 32880bf. Bugbot is set up for automated code reviews on this repo. Configure here.