Conversation
Code Review SummaryThis PR adds tracking support for display ad media impressions (view and click events) to the Swift SDK. It introduces two new data structs, a new Detailed Feedback[File: Both new data structs are declared as [File: Both structs implement [File:
[File: The existing worker-level tracking tests (e.g.,
Consider using a hardcoded stub [File: Other worker-level tracking test files include tests for 400, 500, and no-connectivity cases that verify the correct [File:
[File: The minimum deployment target is being raised from iOS 11.0 to iOS 15.6 across all targets. This is a potentially breaking change for existing SDK consumers who support iOS 11.x–15.5. Is this intentional? If so, it should be clearly documented in the changelog and PR description. If it is an incidental side effect of Xcode regenerating the project file, it should be reverted. ConclusionThe feature implementation is clean and consistent with the existing codebase patterns. The key items to address before merging:
|
There was a problem hiding this comment.
Pull request overview
Adds support to emit tracking events for display ads (media impressions) view and click within the Constructor.io Swift client, along with generated documentation updates.
Changes:
- Add
trackMediaImpressionView/trackMediaImpressionClickAPIs and supporting request-data structures/constants. - Extend request building/configuration to support a separate
baseMediaURLfor media tracking requests. - Regenerate Jazzy docs and update README with media impression examples.
Reviewed changes
Copilot reviewed 129 out of 158 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| AutocompleteClient/FW/Logic/Worker/ConstructorIO.swift | Adds public media impression tracking methods and a dedicated media request builder path. |
| AutocompleteClient/FW/Logic/Request/CIOTrackMediaImpressionViewData.swift | Introduces request payload/url builder for “display_ad_view” tracking. |
| AutocompleteClient/FW/Logic/Request/CIOTrackMediaImpressionClickData.swift | Introduces request payload/url builder for “display_ad_click” tracking. |
| AutocompleteClient/FW/Logic/Request/Builder/RequestBuilder.swift | Adds baseMediaURL support and getMediaRequest() to construct media tracking requests. |
| AutocompleteClient/FW/Config/ConstructorIOConfig.swift | Adds baseMediaURL to configuration and initializer. |
| AutocompleteClient/Constants/Constants.swift | Adds default media base URL and tracking endpoint formats. |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOTrackMediaImpressionTests.swift | Adds tests for new media tracking APIs (currently implemented with live network dependency). |
| AutocompleteClientTests/FW/Logic/Request/TrackMediaImpressionViewRequestBuilderTests.swift | Adds unit tests for media impression view request building (URL/body/custom base URL). |
| AutocompleteClientTests/FW/Logic/Request/TrackMediaImpressionClickRequestBuilderTests.swift | Adds unit tests for media impression click request building (URL/body/custom base URL). |
| README.md | Documents media impression tracking usage examples. |
| .jazzy.yaml | Includes media tracking request structures and adds a “Tracking Data Structures” doc category. |
| docs/index.html | Regenerated docs; adds “Media Impression Events” section and updates footer metadata. |
| docs/js/jazzy.js | Fixes a typo in a comment (“targetted” → “targeted”) in generated docs assets. |
| docs/undocumented.json | Regenerated docs artifact; updates source paths/source_directory (currently absolute). |
| docs/Client.html | Regenerated docs footer metadata. |
| docs/Classes/ConstructorIO.html | Regenerated docs; includes new media tracking methods and updated footer metadata. |
| docs/Classes/CIOSearchQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOResultSources.html | Regenerated docs footer metadata. |
| docs/Classes/CIOResultSourceData.html | Regenerated docs footer metadata. |
| docs/Classes/CIOResult.html | Regenerated docs footer metadata. |
| docs/Classes/CIORecommendationsQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOGroup.html | Regenerated docs footer metadata. |
| docs/Classes/CIOFilterGroup.html | Regenerated docs footer metadata. |
| docs/Classes/CIOCollectionData.html | Regenerated docs footer metadata. |
| docs/Classes/CIOBrowseQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOBrowseItemsQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOBrowseGroupsQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOBrowseFacetsQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOBrowseFacetOptionsQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Classes/CIOAutocompleteResult.html | Regenerated docs footer metadata. |
| docs/Classes/CIOAutocompleteQueryBuilder.html | Regenerated docs footer metadata. |
| docs/Structs/ConstructorIOConfig.html | Regenerated docs; documents baseMediaURL and updated initializer signature. |
| docs/Structs/CIOSortOption.html | Regenerated docs footer metadata. |
| docs/Structs/CIOSearchResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOSearchRedirectInfo.html | Regenerated docs footer metadata. |
| docs/Structs/CIOSearchQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOResultFacet.html | Regenerated docs footer metadata. |
| docs/Structs/CIOResultData.html | Regenerated docs footer metadata. |
| docs/Structs/CIORefinedContent.html | Regenerated docs footer metadata. |
| docs/Structs/CIORecommendationsStrategy.html | Regenerated docs footer metadata. |
| docs/Structs/CIORecommendationsResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIORecommendationsQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIORecommendationsPod.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizResultsResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizResult.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizQuestionResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizQuestion.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizOptionAttribute.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizOption.html | Regenerated docs footer metadata. |
| docs/Structs/CIOQuizImages.html | Regenerated docs footer metadata. |
| docs/Structs/CIOFilterFacetOption.html | Regenerated docs footer metadata. |
| docs/Structs/CIOFilterFacet.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseItemsQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseGroupsQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseFacetsResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseFacetsQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseFacetOptionsResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOBrowseFacetOptionsQuery.html | Regenerated docs footer metadata. |
| docs/Structs/CIOAutocompleteResponse.html | Regenerated docs footer metadata. |
| docs/Structs/CIOAutocompleteQuery.html | Regenerated docs footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/index.html | Regenerated docset index; adds “Media Impression Events” section and updates footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/js/jazzy.js | Regenerated docset JS; typo fix in comment. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/undocumented.json | Regenerated docset artifact; updates source paths/source_directory (currently absolute). |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Client.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/ConstructorIO.html | Regenerated docset docs; includes new media tracking methods and updated footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOSearchQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOResultSources.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOResultSourceData.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOResult.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIORecommendationsQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOGroup.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOFilterGroup.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOCollectionData.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOBrowseQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOBrowseItemsQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOBrowseGroupsQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOBrowseFacetsQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOBrowseFacetOptionsQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOAutocompleteResult.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Classes/CIOAutocompleteQueryBuilder.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/ConstructorIOConfig.html | Regenerated docset docs; documents baseMediaURL and updated initializer signature. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOSortOption.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOSearchResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOSearchRedirectInfo.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOSearchQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOResultFacet.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOResultData.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIORefinedContent.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIORecommendationsStrategy.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIORecommendationsResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIORecommendationsQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIORecommendationsPod.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizResultsResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizResult.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizQuestionResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizQuestion.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizOptionAttribute.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizOption.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOQuizImages.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOFilterFacetOption.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOFilterFacet.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseItemsQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseGroupsQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseFacetsResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseFacetsQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseFacetOptionsResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOBrowseFacetOptionsQuery.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOAutocompleteResponse.html | Regenerated docset footer metadata. |
| docs/docsets/ConstructorAutocomplete.docset/Contents/Resources/Documents/Structs/CIOAutocompleteQuery.html | Regenerated docset footer metadata. |
| Podfile.lock | Updates lockfile metadata (specs repo URL casing, CocoaPods version, checksum). |
| Pods/Manifest.lock | Updates manifest lockfile metadata (specs repo URL casing, CocoaPods version, checksum). |
| AutocompleteClient.xcodeproj/project.pbxproj | Adds new sources/tests to the Xcode project; also updates CocoaPods integration metadata and raises deployment targets. |
Files not reviewed (22)
- Pods/Target Support Files/Kingfisher/Kingfisher-Info.plist: Language not supported
- Pods/Target Support Files/Kingfisher/Kingfisher.debug.xcconfig: Language not supported
- Pods/Target Support Files/Kingfisher/Kingfisher.release.xcconfig: Language not supported
- Pods/Target Support Files/OHHTTPStubs/OHHTTPStubs-Info.plist: Language not supported
- Pods/Target Support Files/OHHTTPStubs/OHHTTPStubs.debug.xcconfig: Language not supported
- Pods/Target Support Files/OHHTTPStubs/OHHTTPStubs.release.xcconfig: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests-Info.plist: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests-frameworks-Debug-input-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests-frameworks-Debug-output-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests-frameworks-Release-input-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests-frameworks-Release-output-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests-frameworks.sh: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests.debug.xcconfig: Language not supported
- Pods/Target Support Files/Pods-AutocompleteClientTests/Pods-AutocompleteClientTests.release.xcconfig: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication-Info.plist: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication-frameworks-Debug-input-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication-frameworks-Debug-output-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication-frameworks-Release-input-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication-frameworks-Release-output-files.xcfilelist: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication-frameworks.sh: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication.debug.xcconfig: Language not supported
- Pods/Target Support Files/Pods-UserApplication/Pods-UserApplication.release.xcconfig: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| public func trackMediaImpressionView(bannerAdId: String, placementId: String, completionHandler: TrackingCompletionHandler? = nil) { | ||
| let data = CIOTrackMediaImpressionViewData(bannerAdId: bannerAdId, placementId: placementId) | ||
| let request = self.buildMediaRequest(data: data) | ||
| executeTracking(request, completionHandler: completionHandler) | ||
| } | ||
|
|
||
| /** | ||
| Track when a user clicks a media impression (display ad) | ||
|
|
||
| - Parameters: | ||
| - bannerAdId: The banner ad ID | ||
| - placementId: The placement ID | ||
| - completionHandler: The callback to execute on completion. | ||
|
|
||
| ### Usage Example: ### | ||
| ``` | ||
| constructorIO.trackMediaImpressionClick(bannerAdId: "abc123", placementId: "home") | ||
| ``` | ||
| */ | ||
| public func trackMediaImpressionClick(bannerAdId: String, placementId: String, completionHandler: TrackingCompletionHandler? = nil) { | ||
| let data = CIOTrackMediaImpressionClickData(bannerAdId: bannerAdId, placementId: placementId) |
There was a problem hiding this comment.
Public API naming: new methods use bannerAdId / placementId, but the existing public API consistently uses ...ID (e.g., quizID, resultID, itemID). Consider renaming these parameters to bannerAdID / placementID (and aligning related types/docs/tests) now to keep the API consistent before release.
| let config = ConstructorIOConfig(apiKey: TestConstants.testApiKeyWithAdPlacements) | ||
| self.constructor = ConstructorIO(config: config) | ||
|
|
||
| // Fetch a valid banner_ad_id from the display ads endpoint | ||
| let fetchExpectation = XCTestExpectation(description: "Fetch display ads to get banner_ad_id") | ||
| let urlString = "https://display.media-cnstrc.com/display-ads?key=\(TestConstants.testApiKeyWithAdPlacements)&placement_ids=\(TestConstants.testPlacementId)" | ||
| let url = URL(string: urlString)! | ||
|
|
||
| let task = URLSession.shared.dataTask(with: url) { data, _, error in | ||
| guard let data = data, error == nil else { | ||
| XCTFail("Failed to fetch display ads: \(error?.localizedDescription ?? "unknown error")") | ||
| fetchExpectation.fulfill() | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| if let json = try JSONSerialization.jsonObject(with: data, options: []) as? [String: Any], | ||
| let displayAds = json["display_ads"] as? [String: Any], | ||
| let ad = displayAds[TestConstants.testPlacementId] as? [String: Any], | ||
| let bannerAdId = ad["banner_ad_id"] as? String { | ||
| self.bannerAdId = bannerAdId | ||
| } else { | ||
| XCTFail("Failed to parse banner_ad_id from display ads response") | ||
| } | ||
| } catch { | ||
| XCTFail("Failed to parse display ads JSON: \(error.localizedDescription)") | ||
| } | ||
|
|
||
| fetchExpectation.fulfill() | ||
| } | ||
| task.resume() | ||
|
|
||
| wait(for: [fetchExpectation], timeout: 10.0) |
There was a problem hiding this comment.
This test performs a real network call in setUp() to fetch banner_ad_id, which makes the unit test suite flaky (depends on network availability and external service state) and slows CI. The rest of the tracking tests in this repo use OHHTTPStubs to deterministically stub requests/responses; this test should follow that pattern by stubbing the display-ads fetch (or avoiding it entirely) and stubbing the media tracking endpoints instead of calling live URLs.
| func testTrackMediaImpressionView() { | ||
| let expectation = XCTestExpectation(description: "Tracking media impression view") | ||
| self.constructor.trackMediaImpressionView(bannerAdId: self.bannerAdId, placementId: TestConstants.testPlacementId, completionHandler: { response in | ||
| let cioError = response.error as? CIOError | ||
| XCTAssertNil(cioError) | ||
| expectation.fulfill() | ||
| }) | ||
| self.wait(for: [expectation], timeout: 10.0) | ||
| } | ||
|
|
||
| func testTrackMediaImpressionClick() { | ||
| let expectation = XCTestExpectation(description: "Tracking media impression click") | ||
| self.constructor.trackMediaImpressionClick(bannerAdId: self.bannerAdId, placementId: TestConstants.testPlacementId, completionHandler: { response in | ||
| let cioError = response.error as? CIOError | ||
| XCTAssertNil(cioError) | ||
| expectation.fulfill() | ||
| }) | ||
| self.wait(for: [expectation], timeout: 10.0) |
There was a problem hiding this comment.
bannerAdId is an implicitly-unwrapped optional that may remain nil if the fetch/parsing fails; the subsequent calls to trackMediaImpressionView/Click will then crash when passing it into a non-optional parameter. If you keep the fetch, fail the setup early (or skip the tests) when bannerAdId cannot be obtained, or make the tests fully stubbed so bannerAdId is always defined.
| { | ||
| "file": "/Users/stanleypeng/Documents/Work/constructorio-client-swift/AutocompleteClient/FW/Logic/Result/CIORecommendationsPod.swift", | ||
| "file": "/Users/viktor.zavala/projects/constructorio-client-swift/AutocompleteClient/FW/Logic/Result/CIORecommendationsPod.swift", | ||
| "line": 15, | ||
| "symbol": "CIORecommendationsPod.displayName", |
There was a problem hiding this comment.
This file contains absolute paths to a developer machine (e.g., /Users/.../projects/...) which are environment-specific and can leak local usernames/paths into the repo. Consider excluding this generated file from version control, or configuring the docs generation step to emit paths relative to source_directory so the output is reproducible across machines.
| { | ||
| "file": "/Users/evan.yan/Constructor/constructorio-client-swift/AutocompleteClient/FW/Logic/Result/CIORecommendationsPod.swift", | ||
| "file": "/Users/viktor.zavala/projects/constructorio-client-swift/AutocompleteClient/FW/Logic/Result/CIORecommendationsPod.swift", | ||
| "line": 15, | ||
| "symbol": "CIORecommendationsPod.displayName", | ||
| "symbol_kind": "source.lang.swift.decl.var.instance", | ||
| "warning": "undocumented" | ||
| }, | ||
| { | ||
| "file": "/Users/evan.yan/Constructor/constructorio-client-swift/AutocompleteClient/FW/Logic/Result/CIORecommendationsPod.swift", | ||
| "file": "/Users/viktor.zavala/projects/constructorio-client-swift/AutocompleteClient/FW/Logic/Result/CIORecommendationsPod.swift", | ||
| "line": 16, |
There was a problem hiding this comment.
This generated undocumented.json includes absolute local filesystem paths (and in this diff even mixes multiple different /Users/... roots). This makes the docs artifacts non-reproducible and may leak developer machine details; prefer generating relative paths or excluding this file from commits.
ZSnake
force-pushed
the
REM-3041/display-ads-tracking
branch
from
25444a5 to
d6e73c0
Compare
Code Review SummaryThis PR adds display ad tracking functionality (media impression view and click events) to the Constructor.io Swift client. The implementation follows the existing patterns in the codebase (quiz tracking, etc.) well overall, and is accompanied by unit and integration tests. I have a few issues to flag — one significant correctness concern, a few minor issues, and some observations. Detailed Feedback[File:
[File: let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString)
self.baseMediaURL = baseMediaURL ?? Constants.Query.baseMediaURLStringSo the [File:
This duplication could be a source of future drift bugs. A simple refactor — extracting a shared private helper that takes the base URL string and a flag for the [File:
[File: The
This is noted as a design choice — integration tests often require live data — but it's worth making sure CI is configured to handle/skip these gracefully when network is unavailable, and ideally to add a [File: The other request builder tests in this codebase (e.g. XCTAssertTrue(url.contains("_dt="), "URL should contain the timestamp parameter.")[File: The minimum deployment target across all targets was raised from [File: Both new data structs implement ConclusionThe implementation is well-structured and follows established patterns in the codebase. The main issue to resolve before merging is the |
Code Review SummaryThis PR adds two new public tracking methods — Detailed Feedback[File: The new Consider refactoring to a shared private helper, for example: private func buildRequest(baseURL: String, alwaysIncludeDate: Bool = true) -> URLRequest {
let urlString = self.trackData!.url(with: baseURL)
var urlComponents = URLComponents(string: urlString)!
var allQueryItems = self.queryItems
allQueryItems.add(URLQueryItem(name: "c", value: Constants.versionString()))
if alwaysIncludeDate {
self.addDateQueryItem(queryItems: &allQueryItems)
}
// ... rest of shared logic
}[File:
[File: In let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString)However, self.baseMediaURL = baseMediaURL ?? Constants.Query.baseMediaURLStringThis mirrors the same redundancy present in [File: Both
These explicit overrides are dead code and should be removed to keep the structs clean. [File: The
[File: Both [File: Other request builder tests in the suite (e.g. for autocomplete select) assert that the ConclusionThe feature is well-structured and consistent with the existing codebase patterns. The most important items to address before merging are:
The redundant default in |
.jazzy.yaml
Outdated
| - "/**/Result/Responses/*.swift" | ||
| - "/**/Result/*.swift" | ||
| - "/**/Config/ConstructorIOConfig.swift" | ||
| - "/**/Request/CIOTrackMediaImpressionViewData.swift" |
There was a problem hiding this comment.
I just wanted to double-check something following the automated CC review: it seems like these CIOTrackMediaImpressionViewData / CIOTrackMediaImpressionClickData are internal, since they don't have the public keyword. Do you think it makes sense to include them here?
| func testTrackMediaImpressionView() { | ||
| let expectation = XCTestExpectation(description: "Tracking media impression view") | ||
| self.constructor.trackMediaImpressionView(bannerAdId: self.bannerAdId, placementId: TestConstants.testPlacementId, completionHandler: { response in |
There was a problem hiding this comment.
I know we're not using it in this project, but since we're making a real HTTP call in setUp() to fetch a banner_ad_id, does it makes sense to add some guard with a message?
| func testTrackMediaImpressionView() { | |
| let expectation = XCTestExpectation(description: "Tracking media impression view") | |
| self.constructor.trackMediaImpressionView(bannerAdId: self.bannerAdId, placementId: TestConstants.testPlacementId, completionHandler: { response in | |
| func testTrackMediaImpressionView() throws { | |
| let bannerAdId = try XCTUnwrap(self.bannerAdId, "banner_ad_id was not fetched during setUp") | |
| let expectation = XCTestExpectation(description: "Tracking media impression view") | |
| self.constructor.trackMediaImpressionView(bannerAdId: bannerAdId, placementId: TestConstants.testPlacementId, completionHandler: { response in |
Code Review SummaryThis PR adds display ads tracking support by introducing two new public API methods — Detailed Feedback[File: 1. Structs should be Both // Before
struct CIOTrackMediaImpressionViewData: CIORequestData {
// After
public struct CIOTrackMediaImpressionViewData: CIORequestData {
2. Duplicate Both new structs override 3. All other POST tracking structs (e.g., let analyticsTags: [String: String]?
// In httpBody:
if let analyticsTags = self.analyticsTags {
dict["analytics_tags"] = analyticsTags
}[File: 4. Force-unwrap (
// getMediaRequest() must only be called after build(trackData:)[File: 5. Redundant fallback in private func buildMediaRequest(data: CIORequestData) -> URLRequest {
let requestBuilder = RequestBuilder(apiKey: self.config.apiKey,
baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString)
let requestBuilder = RequestBuilder(apiKey: self.config.apiKey,
baseMediaURL: self.config.baseMediaURL)The same pattern inconsistency exists in [File: 6. Integration test makes a live network call in The
Look at the other integration tests in the project — they use
// Simpler, deterministic approach:
override func setUp() {
super.setUp()
let config = ConstructorIOConfig(apiKey: TestConstants.testApiKey)
self.constructor = ConstructorIO(config: config)
self.bannerAdId = "banner-ad-test-123" // fixed test value
}7.
guard self.bannerAdId != nil else {
XCTSkip("Could not fetch bannerAdId from live API, skipping test")
return
}[File: 8. Hardcoded API key in test constants
[File: 9. Minor: The local variable ConclusionThe overall approach is clean, idiomatic Swift, and well-aligned with existing patterns in the repository. The most important issues to address before merging are:
The optional |
Code Review SummaryThis PR adds two new public tracking methods — Detailed Feedback[File: In // ConstructorIO.swift (current)
let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString)
// RequestBuilder.swift (existing)
self.baseMediaURL = baseMediaURL ?? Constants.Query.baseMediaURLStringThis is inconsistent with how // buildQuizRequest — passes nil and lets RequestBuilder apply the default
let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseQuizURL: self.config.baseQuizURL ?? Constants.Query.baseQuizURLString)Wait — actually private func buildMediaRequest(data: CIORequestData) -> URLRequest {
let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL)
...
}[File: Both new data structs override // This function is never called for media impression data — safe to remove
func urlWithFormat(baseURL: String, format: String) -> String {
return String(format: format, baseURL)
}If the intent was to future-proof the implementation, a comment would help. Otherwise removing it keeps the struct lean. [File: Both data structs are declared without an explicit access modifier ( If these structs are intended to be part of the public API (so consumers can construct them directly), they should be declared Compare with [File: The integration test fetches a live
Consider using a hardcoded func testTrackMediaImpressionView() {
guard let bannerAdId = self.bannerAdId else {
XCTFail("bannerAdId was not set in setUp — skipping test")
return
}
...
}[File: All other worker-level integration tests in this suite (e.g. [File:
Consider extracting the shared logic into a private helper: private func buildURLRequest(urlString: String, addDate: Bool = true) -> URLRequest {
var urlComponents = URLComponents(string: urlString)!
var allQueryItems = self.queryItems
allQueryItems.add(URLQueryItem(name: "c", value: Constants.versionString()))
if addDate { self.addDateQueryItem(queryItems: &allQueryItems) }
if let existingItem = urlComponents.queryItems?.first {
allQueryItems.add(existingItem)
}
urlComponents.queryItems = self.trackData!.queryItems(baseItems: allQueryItems.all())
urlComponents.percentEncodedQuery = urlComponents.percentEncodedQuery?
.replacingOccurrences(of: "+", with: "%2B")
let url = urlComponents.url!
let httpBody = self.trackData!.httpBody(baseParams: allQueryItems.allAsDictionary())
var request = URLRequest(url: url)
if httpBody != nil {
request.addValue("application/json", forHTTPHeaderField: "Content-Type")
request.addValue("application/json", forHTTPHeaderField: "Accept")
request.httpBody = httpBody
}
request.httpMethod = self.trackData!.httpMethod()
return request
}[File:
ConclusionThe core implementation is solid and follows the established conventions well. The main items worth addressing before merging are:
The Pods-related file changes appear to be a CocoaPods lock file regeneration and are not a concern. |
Code Review SummaryThis PR adds display ad tracking support (media impression view and click) to the iOS client. The implementation follows existing patterns closely (quiz tracking via buildQuizRequest), introduces two new data structs, a new RequestBuilder method, and integration/unit tests. Overall the approach is sound and well-structured. A few issues are worth addressing before merging. Detailed Feedback[File: CIOTrackMediaImpressionViewData.swift L23-L25 and CIOTrackMediaImpressionClickData.swift L23-L25] Unnecessary urlWithFormat override Both new structs explicitly implement urlWithFormat(baseURL:format:) even though CIORequestData already provides a default implementation returning an empty string. Since getMediaRequest() in RequestBuilder only ever calls url(with:), these overrides are dead code. They should be removed to keep the structs lean and consistent with other non-quiz data structs such as CIOTrackRecommendationResultClickData which do not override this method. [File: RequestBuilder.swift L119-L148] getMediaRequest() is nearly identical to getRequest() — DRY concern The two methods share roughly 90% of their body. The only meaningful differences are: (1) getMediaRequest uses self.baseMediaURL instead of self.baseURL, and (2) getMediaRequest always adds the dt query item whereas getRequest has a special-case exclusion for certain browse paths. Consider a shared private helper parameterised on the base URL to eliminate this duplication. At minimum, add a comment explaining why a separate method was needed rather than reusing getRequest. [File: ConstructorIO.swift L858] Redundant nil-coalescing in buildMediaRequest RequestBuilder.init already applies the identical fallback internally (baseMediaURL ?? Constants.Query.baseMediaURLString), so passing it explicitly in buildMediaRequest is redundant. The same pattern exists in buildQuizRequest. For consistency, either remove the fallback in both places and let RequestBuilder handle the default, or keep it everywhere — just make the choice intentional. [File: ConstructorIOTrackMediaImpressionTests.swift L17-L52] Live network call in setUp is fragile and can crash on failure The setUp method makes a real HTTP request to display.media-cnstrc.com to obtain a banner_ad_id. If the external service is unavailable or the key has no ad placements, the fetch silently fails and self.bannerAdId remains nil. Since bannerAdId is declared as String! (implicitly unwrapped), the subsequent tracking call will crash the test runner rather than produce a meaningful XCTFail. Suggestions:
[Files: CIOTrackMediaImpressionViewData.swift and CIOTrackMediaImpressionClickData.swift] Structs are internal but listed in .jazzy.yaml as public API Both structs are declared without public, making them internal. Yet .jazzy.yaml adds them under a new "Tracking Data Structures" category, implying they are part of the public API surface. If consumers are expected to see or construct these types, they should be public struct. If they are only internal implementation details, they should be removed from .jazzy.yaml. [File: TestConstants.swift L24] Second API key committed in plaintext static let testApiKeyWithAdPlacements = "key_x6UnCVRZaJgIHFQD" follows the existing pattern of testApiKey, but please confirm this key has appropriately restricted permissions and can be safely rotated, since it is now permanently in git history. [File: RequestBuilder.swift L120-L145] Force-unwraps carried over from getRequest() getMediaRequest() retains the same ! force-unwraps already present in getRequest() (which has an open TODO comment about this). Since this is new code, it is a good opportunity to address the debt here even if not in the older method. A guard let with preconditionFailure would communicate the programming contract more clearly and avoid a hard crash in production if build(trackData:) is accidentally omitted. ConclusionThe feature is well-scoped and closely follows existing conventions. The two most important issues to resolve before merging are:
The remaining points are lower-severity consistency and robustness improvements. Good work overall — the pattern-matching with quiz tracking is clean and sensible. |
…ession tests, remove Media impression data files from jazzy
Code Review SummaryThis PR adds display ads tracking support by introducing two new public tracking methods — Detailed Feedback[File: The new let urlString = self.trackData!.url(with: self.baseMediaURL) // crash if trackData is nil
var urlComponents = URLComponents(string: urlString)! // crash if URL string is malformed
let url = urlComponents.url! // crash if percent-encoding failsWhile [File: private func buildMediaRequest(data: CIORequestData) -> URLRequest {
let requestBuilder = RequestBuilder(
apiKey: self.config.apiKey,
baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString // ← manual fallback
)
...
}
self.baseMediaURL = baseMediaURL ?? Constants.Query.baseMediaURLStringCompare with let requestBuilder = RequestBuilder(
apiKey: self.config.apiKey,
baseQuizURL: self.config.baseQuizURL ?? Constants.Query.baseQuizURLString // same duplication
)Both [File: Both new data structs override func urlWithFormat(baseURL: String, format: String) -> String {
return String(format: format, baseURL)
}
[File: Both data structs are declared without an access modifier (defaulting to struct CIOTrackMediaImpressionClickData: CIORequestData { ... }
struct CIOTrackMediaImpressionViewData: CIORequestData { ... }This matches the pattern for other tracking data structs in the codebase (e.g., [File: The guard let data = data, error == nil else {
XCTFail("Failed to fetch display ads: ...")
fetchExpectation.fulfill()
return
}If let bannerAdId = try XCTUnwrap(self.bannerAdId, "banner_ad_id was not fetched during setUp")This is a reasonable pattern for integration tests, but:
Consider either: (a) mocking the display ads fetch with OHHTTPStubs and hardcoding a valid [File: The builder tests verify XCTAssertTrue(url.contains("_dt="), "URL should contain the timestamp.")Adding this assertion would improve test completeness. [File: In the This is pre-existing, but since the doc block is being edited in this PR it's a good opportunity to fix. ConclusionThe implementation is well-structured and follows the existing conventions. The main items to address are: (1) the redundant |
AutocompleteClientTests/FW/Logic/Worker/ConstructorIOTrackMediaImpressionTests.swift
Outdated
Show resolved
Hide resolved
Code Review SummaryThis PR adds display ads tracking support by introducing two new public API methods — Detailed Feedback[File: Both new data structs implement // Remove these from both CIOTrackMediaImpressionClickData and CIOTrackMediaImpressionViewData:
func urlWithFormat(baseURL: String, format: String) -> String {
return String(format: format, baseURL)
}[File:
Consider extracting the shared logic into a private helper, or passing the base URL string as a parameter: private func buildRequest(using urlString: String) -> URLRequest {
var urlComponents = URLComponents(string: urlString)!
var allQueryItems = self.queryItems
allQueryItems.add(URLQueryItem(name: "c", value: Constants.versionString()))
self.addDateQueryItem(queryItems: &allQueryItems)
urlComponents.queryItems = self.trackData!.queryItems(baseItems: allQueryItems.all())
urlComponents.percentEncodedQuery = urlComponents.percentEncodedQuery?
.replacingOccurrences(of: "+", with: "%2B")
let url = urlComponents.url!
let httpBody = self.trackData!.httpBody(baseParams: allQueryItems.allAsDictionary())
var request = URLRequest(url: url)
if httpBody != nil {
request.addValue("application/json", forHTTPHeaderField: "Content-Type")
request.addValue("application/json", forHTTPHeaderField: "Accept")
request.httpBody = httpBody
}
request.httpMethod = self.trackData!.httpMethod()
return request
}[File: let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString)
self.baseMediaURL = baseMediaURL ?? Constants.Query.baseMediaURLStringSo passing let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL)This also matches how the [File: The
Consider using a hardcoded or fixture-based [File: override func tearDown() {
super.tearDown()
}This override does nothing and can be removed to keep the file clean. [File: let tracker: CIORequestData = CIOTrackMediaImpressionClickData(bannerAdId: bannerAdId, placementId: placementId)The explicit let tracker = CIOTrackMediaImpressionClickData(bannerAdId: bannerAdId, placementId: placementId)[File: Both A possible improvement would be to use a shared base type or a factory approach (passing the endpoint constant), though this may be acceptable given the rest of the codebase follows similar patterns (e.g., [File: static let testApiKeyWithAdPlacements = "key_x6UnCVRZaJgIHFQD"
static let testPlacementId = "home"A real-looking API key is being committed to source control. While test API keys are generally lower risk, confirm this key is intended to be public and has appropriate scope restrictions. If it is a key for a test/sandbox account, it should be clearly commented as such. ConclusionThe feature is well-structured and follows the patterns established by the existing codebase. The main concerns are:
Once these are addressed, the PR will be in great shape. |
Code Review SummaryThis PR adds two new public tracking methods to Detailed Feedback[File:
This duplication will become a maintenance burden. Consider refactoring to a shared private helper: private func buildURLRequest(urlString: String) -> URLRequest {
var urlComponents = URLComponents(string: urlString)!
var allQueryItems = self.queryItems
allQueryItems.add(URLQueryItem(name: "c", value: Constants.versionString()))
self.addDateQueryItem(queryItems: &allQueryItems)
// ...attach action from base URL if present, percent-encode, etc.
urlComponents.queryItems = self.trackData!.queryItems(baseItems: allQueryItems.all())
urlComponents.percentEncodedQuery = urlComponents.percentEncodedQuery?
.replacingOccurrences(of: "+", with: "%2B")
let url = urlComponents.url!
let httpBody = self.trackData!.httpBody(baseParams: allQueryItems.allAsDictionary())
var request = URLRequest(url: url)
if httpBody != nil {
request.addValue("application/json", forHTTPHeaderField: "Content-Type")
request.addValue("application/json", forHTTPHeaderField: "Accept")
request.httpBody = httpBody
}
request.httpMethod = self.trackData!.httpMethod()
return request
}Then both [File: let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL ?? Constants.Query.baseMediaURLString)
let requestBuilder = RequestBuilder(apiKey: self.config.apiKey, baseMediaURL: self.config.baseMediaURL)This also matches how [File: Both new structs override func urlWithFormat(baseURL: String, format: String) -> String {
return String(format: format, baseURL)
}This is the same as the default implementation provided by the [File: Both enum MediaImpressionEventType {
case view
case click
}
struct CIOTrackMediaImpressionData: CIORequestData {
let bannerAdId: String
let placementId: String
let eventType: MediaImpressionEventType
func url(with baseURL: String) -> String {
switch eventType {
case .view: return String(format: Constants.TrackMediaImpressionView.format, baseURL)
case .click: return String(format: Constants.TrackMediaImpressionClick.format, baseURL)
}
}
// ...rest unchanged
}This reduces duplication and makes future additions (e.g. a third event type) a one-line change. [File: static let testApiKeyWithAdPlacements = "key_x6UnCVRZaJgIHFQD"This constant is declared but never referenced anywhere in the test files added in this PR. If it's not needed, it should be removed to keep the test utilities clean. [File: The integration tests stub by URL regex and verify that a request is sent, but they don't assert that the HTTP body contains the correct [General] Missing test for payload sent with custom The request builder tests cover the custom base URL case. However, there's no integration test in ConclusionThe feature is well-scoped and follows the existing codebase patterns. The main actionable items are: eliminating the redundant |
Add functions to emit tracking events for display ads media impressions view and click