feat: send user agent header Foresight PR for review and unit tests#4
feat: send user agent header Foresight PR for review and unit tests#4
Conversation
gazreese
changed the title
WalkthroughThe pull request introduces SDK version tracking via a public Changes
Sequence DiagramsequenceDiagram
participant Client
participant Flagsmith
participant Router
participant Network
rect rgb(220, 240, 255)
Note over Client,Network: User-Agent Header Addition Flow
Client->>Flagsmith: request data
Flagsmith->>Flagsmith: compute userAgent<br/>(via getSDKVersion)
Flagsmith->>Router: pass userAgent
Router->>Router: setValue("User-Agent",<br/>Flagsmith.userAgent)
Router->>Network: HTTP request with<br/>User-Agent header
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/pull_request_template.md(1 hunks)FlagsmithClient/Classes/Flagsmith.swift(1 hunks)FlagsmithClient/Classes/Internal/Router.swift(1 hunks)FlagsmithClient/Classes/Internal/SSEManager.swift(1 hunks)FlagsmithClient/Tests/RouterTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
FlagsmithClient/Classes/Internal/SSEManager.swift (2)
FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-108)FlagsmithClient/Classes/Internal/APIManager.swift (3)
request(131-167)request(174-183)request(191-213)
FlagsmithClient/Classes/Internal/Router.swift (1)
FlagsmithClient/Classes/Internal/APIManager.swift (3)
request(131-167)request(174-183)request(191-213)
FlagsmithClient/Tests/RouterTests.swift (1)
FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-108)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/RouterTests.swift
[warning] 43-43:
Line Length Violation: Line should be 140 characters or less; currently it has 156 characters (line_length)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
🔇 Additional comments (4)
FlagsmithClient/Classes/Flagsmith.swift (1)
20-27: LGTM!The computed property correctly constructs the User-Agent header using the SDK version with proper formatting.
FlagsmithClient/Classes/Internal/Router.swift (1)
105-105: LGTM!The User-Agent header is correctly added to all requests using the centralized
Flagsmith.userAgentproperty.FlagsmithClient/Classes/Internal/SSEManager.swift (1)
160-160: LGTM!The User-Agent header is correctly added to the SSE request, maintaining consistency with the standard HTTP requests in Router.swift.
FlagsmithClient/Tests/RouterTests.swift (1)
46-55: LGTM!The test correctly validates the User-Agent format and ensures it ends with either "unknown" or a semantic version number.
gazreese
left a comment
gazreese
left a comment
There was a problem hiding this comment.
File: FlagsmithClient/Classes/Flagsmith.swift
Lines: 32-35
🚨 CRITICAL BUG: This returns the host app's version, not the SDK version!
// Try to get version from the main bundle first
if let version = Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String, !version.isEmpty {
return version
}Problem: Bundle.main refers to the app using the SDK, not the SDK itself. This is why tests show "16.0" (macOS version) instead of "3.8.4" (SDK version).
Solution: Remove this check entirely, or move it to LAST as a fallback (though even then it's incorrect).
Recommended Fix:
private static func getSDKVersion() -> String {
// Try CocoaPods bundle first
if let bundle = Bundle(identifier: "org.cocoapods.FlagsmithClient"),
let version = bundle.infoDictionary?["CFBundleShortVersionString"] as? String,
!version.isEmpty,
version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil {
return version
}
// Try SPM bundle
if let version = Bundle(for: Flagsmith.self).infoDictionary?["CFBundleShortVersionString"] as? String,
!version.isEmpty,
version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil {
return version
}
// DO NOT use Bundle.main - it returns host app version
return "unknown"
}Alternative: Hardcode the version and update it via release-please:
private static func getSDKVersion() -> String {
return "3.8.4" // x-release-please-version
}
gazreese
left a comment
gazreese
left a comment
There was a problem hiding this comment.
File: FlagsmithClient/Classes/Flagsmith.swift
Lines: 21-27
Optimization: Consider making this a static constant instead of computed property:
internal static let userAgent: String = {
let version = getSDKVersion()
return "flagsmith-swift-ios-sdk/\(version)"
}()This avoids recomputing the User-Agent on every access (though current impact is minimal since it's only called once per request).
gazreese
left a comment
gazreese
left a comment
There was a problem hiding this comment.
File: FlagsmithClient/Classes/Internal/Router.swift
Line: 105
Minor: Consider using setValue instead of addValue for User-Agent:
request.setValue(Flagsmith.userAgent, forHTTPHeaderField: "User-Agent")setValue replaces any existing value, while addValue appends. For User-Agent, replacing is typically more appropriate to ensure only one value is set.
gazreese
left a comment
gazreese
left a comment
There was a problem hiding this comment.
File: FlagsmithClient/Tests/RouterTests.swift
Lines: 38-43
Issue: Test regex is too strict - it won't accept pre-release versions.
Current pattern requires exactly X.Y.Z format, but semantic versioning allows:
3.8.4-beta.1(pre-release)3.8.4+build.123(build metadata)4.0.0-rc.1(release candidate)
Recommended Fix:
// Support full semantic versioning
let expectedPattern = "^flagsmith-swift-ios-sdk/([0-9]+\\.[0-9]+\\.[0-9]+(-[a-zA-Z0-9.]+)?(\\+[a-zA-Z0-9.]+)?|unknown)$"Or simplify:
let userAgent = request.value(forHTTPHeaderField: "User-Agent")
XCTAssertTrue(userAgent?.hasPrefix("flagsmith-swift-ios-sdk/") ?? false)
let version = userAgent?.components(separatedBy: "/").last ?? ""
XCTAssertTrue(version == "unknown" || version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil,
"Version should be 'unknown' or semantic version, got: \(version)")
gazreese
left a comment
gazreese
left a comment
There was a problem hiding this comment.
File: FlagsmithClient/Tests/ (Missing tests)
Missing Coverage: No tests for User-Agent in SSE connections.
While the implementation in SSEManager.swift:160 looks correct, there are no corresponding tests in SSEManagerTests.swift.
Recommended Addition:
// In SSEManagerTests.swift
func testSSERequestIncludesUserAgentHeader() {
let userAgent = Flagsmith.userAgent
XCTAssertTrue(userAgent.hasPrefix("flagsmith-swift-ios-sdk/"),
"SSE connections should include User-Agent header")
XCTAssertFalse(userAgent.hasSuffix("/"),
"User-Agent should not end with slash")
}
func testUserAgentNeverContainsHostAppVersion() {
let userAgent = Flagsmith.userAgent
let mainVersion = Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String
if let mainVersion = mainVersion {
XCTAssertFalse(userAgent.contains(mainVersion),
"User-Agent should not contain host app version (\(mainVersion))")
}
}
gazreese
left a comment
gazreese
left a comment
There was a problem hiding this comment.
File: FlagsmithClient/Classes/Flagsmith.swift (Documentation)
Enhancement: Improve documentation for the userAgent property:
/// User-Agent header value sent with all HTTP and SSE requests.
///
/// Format: `flagsmith-swift-ios-sdk/<version>`
///
/// The version is determined at runtime from bundle metadata. If the version
/// cannot be determined (e.g., in test environments or certain integration scenarios),
/// this will return `flagsmith-swift-ios-sdk/unknown`.
///
/// This property is `internal` to allow testing of the User-Agent format.
internal static var userAgent: String {
let version = getSDKVersion()
return "flagsmith-swift-ios-sdk/\(version)"
}This clarifies when "unknown" is returned and why the property is internal.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
FlagsmithClient/Tests/RouterTests.swift (1)
25-44: Regex pattern doesn't match assertion message.The regex pattern on line 39 is
^flagsmith-swift-ios-sdk/[0-9]+\.[0-9]+\.[0-9]+$, which only accepts semantic versions like "3.8.4". However, the assertion message on line 43 states the pattern should match eitherflagsmith-swift-ios-sdk/<version>orflagsmith-swift-ios-sdk/unknown, implying "unknown" is a valid value.If "unknown" is indeed a valid fallback (as suggested by the fallback mechanism in
Flagsmith.getSDKVersion), update the regex pattern to accommodate it.Apply this diff to allow "unknown":
- let expectedPattern = "^flagsmith-swift-ios-sdk/[0-9]+\\.[0-9]+\\.[0-9]+$" + let expectedPattern = "^flagsmith-swift-ios-sdk/([0-9]+\\.[0-9]+\\.[0-9]+|unknown)$"
Additionally, line 43 exceeds the 140-character limit as previously noted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FlagsmithClient/Classes/Flagsmith.swift(1 hunks)FlagsmithClient/Tests/RouterTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/Classes/Flagsmith.swift
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/RouterTests.swift (1)
FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
| func testUserAgentHeaderFormat() { | ||
| // Test that the User-Agent format is correct | ||
| let userAgent = Flagsmith.userAgent | ||
| XCTAssertTrue(userAgent.hasPrefix("flagsmith-swift-ios-sdk/")) | ||
|
|
||
| // Should have a semantic version number (e.g., 3.8.4) | ||
| let versionPart = String(userAgent.dropFirst("flagsmith-swift-ios-sdk/".count)) | ||
| XCTAssertTrue(versionPart.range(of: #"^\d+\.\d+\.\d+$"#, options: NSString.CompareOptions.regularExpression) != nil, | ||
| "Version part should be a semantic version number (e.g., 3.8.4), got: \(versionPart)") | ||
|
|
||
| // Should be the expected SDK version | ||
| XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)") | ||
| } |
There was a problem hiding this comment.
Remove hardcoded version assertion.
Line 57 asserts that the version must equal "3.8.4". This makes the test brittle—it will fail every time the SDK version is updated, requiring manual test maintenance. The purpose of having a dynamic version mechanism is to avoid such brittleness.
Remove the hardcoded version check and rely on the semantic version format validation already performed on lines 52-54.
Apply this diff:
-
- // Should be the expected SDK version
- XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)")Minor inconsistency: Line 53 uses Swift's native regex syntax (#"..."# with NSString.CompareOptions.regularExpression), while line 40 in testUserAgentHeader uses NSRegularExpression. Consider standardizing on one approach across both tests for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testUserAgentHeaderFormat() { | |
| // Test that the User-Agent format is correct | |
| let userAgent = Flagsmith.userAgent | |
| XCTAssertTrue(userAgent.hasPrefix("flagsmith-swift-ios-sdk/")) | |
| // Should have a semantic version number (e.g., 3.8.4) | |
| let versionPart = String(userAgent.dropFirst("flagsmith-swift-ios-sdk/".count)) | |
| XCTAssertTrue(versionPart.range(of: #"^\d+\.\d+\.\d+$"#, options: NSString.CompareOptions.regularExpression) != nil, | |
| "Version part should be a semantic version number (e.g., 3.8.4), got: \(versionPart)") | |
| // Should be the expected SDK version | |
| XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)") | |
| } | |
| func testUserAgentHeaderFormat() { | |
| // Test that the User-Agent format is correct | |
| let userAgent = Flagsmith.userAgent | |
| XCTAssertTrue(userAgent.hasPrefix("flagsmith-swift-ios-sdk/")) | |
| // Should have a semantic version number (e.g., 3.8.4) | |
| let versionPart = String(userAgent.dropFirst("flagsmith-swift-ios-sdk/".count)) | |
| XCTAssertTrue(versionPart.range(of: #"^\d+\.\d+\.\d+$"#, options: NSString.CompareOptions.regularExpression) != nil, | |
| "Version part should be a semantic version number (e.g., 3.8.4), got: \(versionPart)") | |
| } |
🤖 Prompt for AI Agents
FlagsmithClient/Tests/RouterTests.swift around lines 46 to 58: the test contains
a brittle hardcoded version assertion ( XCTAssertEqual(versionPart, "3.8.4") )
that will break on SDK updates; remove that assertion so the test only validates
the User-Agent prefix and that the version part matches the semantic-version
regex already checked, and optionally standardize regex usage across tests
(either switch this test to use NSRegularExpression like the other test or
change the other to use Swift raw-string regex) for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
FlagsmithClient/Tests/RouterTests.swift (1)
47-59: Remove hardcoded version assertion to prevent test brittleness.Line 58 asserts a hardcoded version "3.8.4", which will break on every SDK version update. The semantic version format validation on lines 54-55 already ensures the version follows the correct pattern—there's no need for an exact match.
Apply this diff to remove the brittle assertion:
let versionPart = String(userAgent.dropFirst("flagsmith-swift-ios-sdk/".count)) XCTAssertTrue(versionPart.range(of: #"^\d+\.\d+\.\d+$"#, options: NSString.CompareOptions.regularExpression) != nil, "Version part should be a semantic version number (e.g., 3.8.4), got: \(versionPart)") - - // Should be the expected SDK version - XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)")
🧹 Nitpick comments (3)
.github/pull_request_template.md (1)
19-25: Consider more formal verb choice for consistency.Line 20 uses "fixes," while a more formal alternative like "resolves" would align with professional documentation standards.
Apply this diff for slightly more formal wording:
- [ ] 🛠️ Bug fix (non-breaking change which fixes an issue) + [ ] 🛠️ Bug fix (non-breaking change which resolves an issue)FlagsmithClient/Classes/Flagsmith.swift (2)
20-31: Remove or utilize the unusedsdkVersionConstant.The
sdkVersionConstanton line 23 is declared but never used. The comment states it's "used as a fallback when bundle version detection fails," butgetSDKVersion()actually returns"unknown"as the fallback, not this constant.Choose one of these options:
Option 1: Remove the unused constant (if "unknown" is the desired fallback):
- - /// SDK version constant - should match the podspec version - /// This is used as a fallback when bundle version detection fails - private static let sdkVersionConstant = "3.8.4"Option 2: Use the constant as the fallback (if you want to report a known version instead of "unknown"):
In
getSDKVersion()at line 51, change:- return "unknown" + return sdkVersionConstant
35-52: Consider anchoring regex pattern for stricter semantic version validation.Lines 40 and 47 use the pattern
#"^\d+\.\d+\.\d+"#to validate version strings. Without an end anchor ($), this matches versions like "1.2.3.4.5" or "1.2.3-beta" because it only requires the prefix to match.Apply this diff for stricter validation:
if let bundle = Bundle(identifier: "org.cocoapods.FlagsmithClient"), let version = bundle.infoDictionary?["CFBundleShortVersionString"] as? String, !version.isEmpty, - version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil { + version.range(of: #"^\d+\.\d+\.\d+$"#, options: .regularExpression) != nil { return version } // Try SPM bundle if let version = Bundle(for: Flagsmith.self).infoDictionary?["CFBundleShortVersionString"] as? String, !version.isEmpty, - version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil { + version.range(of: #"^\d+\.\d+\.\d+$"#, options: .regularExpression) != nil { return version }This ensures only strict semantic versions (e.g., "3.8.4") are accepted, rejecting malformed versions with extra segments or suffixes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/pull_request_template.md(1 hunks)FlagsmithClient/Classes/Flagsmith.swift(1 hunks)FlagsmithClient/Classes/Internal/Router.swift(1 hunks)FlagsmithClient/Tests/RouterTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
FlagsmithClient/Tests/RouterTests.swift (1)
FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-108)
FlagsmithClient/Classes/Internal/Router.swift (1)
FlagsmithClient/Classes/Internal/APIManager.swift (3)
request(131-167)request(174-183)request(191-213)
🪛 LanguageTool
.github/pull_request_template.md
[style] ~20-~20: Consider using a different verb for a more formal wording.
Context: ... 🛠️ Bug fix (non-breaking change which fixes an issue) - [ ] ❌ Breaking change (fix ...
(FIX_RESOLVE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
🔇 Additional comments (4)
.github/pull_request_template.md (2)
27-27: Typo appears to have been resolved.The past review flagged "refernced" → "referenced"; the current version shows the correct spelling. No action needed.
1-29: Well-structured PR template.The template provides clear, actionable guidance across description, regression testing, change classification, and effort estimation. The emoji annotations enhance visual scanability, and the HTML comments effectively guide contributors without cluttering the rendered output. This complements the User-Agent header feature being introduced in the PR by establishing a repeatable template for documenting similar changes.
FlagsmithClient/Classes/Internal/Router.swift (1)
103-105: LGTM! Header handling improved.The changes correctly:
- Use
setValueinstead ofaddValuefor headers that should have single values (X-Environment-Key, Content-Type)- Add the User-Agent header using
Flagsmith.userAgentThis ensures headers are replaced rather than appended, which is the appropriate behavior for these header types.
FlagsmithClient/Tests/RouterTests.swift (1)
25-45: LGTM! Comprehensive User-Agent validation.The test thoroughly validates:
- Header presence in the request
- Correct prefix format ("flagsmith-swift-ios-sdk/")
- Semantic version pattern (major.minor.patch)
The line length issue from the previous review has been properly addressed.
… latest Swift as wasn't picking up the test-config.json
…ion out of the bundle etc
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores