[MOB-3028] Ecosia Framework

[lucaschifino]

MOB-3028

Context

As a first step towards moving Core into Client, we want to create an Ecosia framework and move Braze into it.

This was jump started while pairing with @d4r1091 🙂

Approach

• Create Ecosia Framework
• Move Braze into it
• Move Analytics and dependencies
• Move tests and make sure they still work
• Add new tests target to CI

Other

Before merging

• Wait for [MOB-2959] Braze IAM for Newsletter NTP card #812 to be merged

Checklist

• I performed some relevant testing on a real device and/or simulator
• I wrote Unit Tests that confirm the expected behaviour
• I made sure that any change to the Analytics events included in PR won't alter current analytics (e.g. new users, upgrading users)
• I included documentation updates to the coding standards or Confluence doc, when needed

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 9d779a9)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Bug The fetchFromSingularServerAndUpdate function does not handle the case where singularService.getConversionValue might return a nil response. This could lead to unexpected crashes or undefined behavior. Performance Issue The save method in the User struct writes to disk on every property change. This could lead to performance degradation due to frequent I/O operations. Code Smell The testIncognitoValuesNoPersonalData test uses a complex dictionary reduction logic multiple times. This could be refactored into a reusable helper function for better readability and maintainability. Code Smell The persistUpdatedValues function has a deeply nested structure and repetitive code for setting user defaults. This could be refactored for better readability and maintainability. Test Coverage The testPatchWithAnalyticsId test does not cover edge cases where the URL might already contain query parameters other than _sp. This could lead to incorrect URL generation.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9d779a9
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add error handling for potential exceptions when parsing the document in BookmarkParser Add error handling for cases where try self.document.getLeadingDL() might throw an exception, to ensure the continuation is properly resumed. Ecosia/Core/Bookmarks/BookmarkParser.swift [35-36] -let result = try self.parse(element: try self.document.getLeadingDL()) -continuation.resume(with: .success(result)) +do { + let result = try self.parse(element: try self.document.getLeadingDL()) + continuation.resume(with: .success(result)) +} catch { + continuation.resume(throwing: error) +} Suggestion importance[1-10]: 10 Why: Adding error handling ensures that the continuation is properly resumed even when an exception occurs. This is critical for maintaining the stability and correctness of asynchronous operations.	10
	Add a safeguard for cases where domain.bundleDomains is empty or nil to prevent crashes Ensure that the EcosiaURLProvider.getBundleDomain(for:) method handles cases where domain.bundleDomains is empty or nil to avoid potential crashes. BrowserKit/Sources/SiteImageView/ImageProcessing/BundleImageFetcher/BundleImageFetcher.swift [83-87] if let ecosiaBundleDomain = EcosiaURLProvider.getBundleDomain(for: domain) { return ecosiaBundleDomain -} else { - return domain.bundleDomains.first(where: { bundledImages[$0] != nil }) +} else if let firstDomain = domain.bundleDomains?.first(where: { bundledImages[$0] != nil }) { + return firstDomain } +return nil Suggestion importance[1-10]: 9 Why: The suggestion addresses a potential crash scenario by adding a safeguard for cases where domain.bundleDomains is empty or nil. This is a critical improvement for robustness and prevents runtime errors.	9
	Add error handling for cases where the nib file is not found to avoid crashes Add a safeguard to handle cases where the nib file "EcosiaLaunchScreen" is not found to prevent runtime crashes. Client/Ecosia/UI/LaunchScreen/EcosiaLaunchScreenView.swift [11-14] -public class func fromNib() -> UIView { - return Bundle.main.loadNibNamed(EcosiaLaunchScreenView.viewName, - owner: nil, - options: nil)![0] as! UIView +public class func fromNib() -> UIView? { + guard let view = Bundle.main.loadNibNamed(EcosiaLaunchScreenView.viewName, + owner: nil, + options: nil)?.first as? UIView else { + return nil + } + return view } Suggestion importance[1-10]: 9 Why: The suggestion introduces error handling for missing nib files, which is a critical improvement to prevent runtime crashes. This ensures the application gracefully handles such scenarios.	9
	Add a nil check for the tracker instance in the track method to prevent potential crashes Ensure that the track method in the Analytics class properly handles cases where the tracker instance might be nil to avoid runtime crashes. Ecosia/Analytics/Analytics.swift [41-43] private func track(_ event: SnowplowTracker.Event) { - guard User.shared.sendAnonymousUsageData else { return } + guard User.shared.sendAnonymousUsageData, let tracker = tracker else { return } _ = tracker.track(event) } Suggestion importance[1-10]: 9 Why: Adding a nil check for the tracker instance is crucial to prevent runtime crashes when the tracker is unexpectedly nil. This improves the robustness of the track method and ensures the application does not fail unexpectedly.	9
	Ensure thread-safe access to the rules array to avoid race conditions Add thread-safety mechanisms to the rules array to prevent potential race conditions when accessing or modifying it from multiple threads. Ecosia/Core/FeatureManagement/Unleash/Unleash+RefreshComponent.swift [15-17] static func addRule(_ rule: RefreshingRule) { - if !rules.contains(where: { type(of: $0) == type(of: rule) }) { - rules.append(rule) + queue.sync { + if !rules.contains(where: { type(of: $0) == type(of: rule) }) { + rules.append(rule) + } } } Suggestion importance[1-10]: 9 Why: The suggestion introduces thread-safety to the rules array, which is critical since it is accessed and modified from multiple threads. This change prevents potential race conditions, ensuring the integrity of the rules array.	9
	Add validation for URL creation to prevent crashes due to malformed URLs Validate the URLs in EcosiaURLProvider to ensure they are correctly formed and handle potential failures in URL creation. BrowserKit/Sources/SiteImageView/ImageProcessing/BundleImageFetcher/BundleImageFetcher.swift [170-181] -static let privacyURL = URL(string: "https://www.ecosia.org/privacy")! +static let privacyURL = URL(string: "https://www.ecosia.org/privacy") ?? URL(string: "https://fallback.url")! static var financialReportsURL: URL { - let blog: URL! + guard let blog = URL(string: "https://blog.ecosia.org/") else { + return URL(string: "https://fallback.url")! + } switch Language.current { case .de: - blog = URL(string: "https://de.blog.ecosia.org/")! + return blog.appendingPathComponent("ecosia-finanzberichte-baumplanzbelege/") case .fr: - blog = URL(string: "https://fr.blog.ecosia.org/")! + return blog.appendingPathComponent("rapports-financiers-recus-de-plantations-arbres/") default: - blog = URL(string: "https://blog.ecosia.org/")! + return blog.appendingPathComponent("ecosia-financial-reports-tree-planting-receipts/") } +} Suggestion importance[1-10]: 8 Why: The suggestion improves the reliability of URL creation by adding fallback mechanisms for malformed URLs. This is a significant enhancement to ensure the application does not crash due to invalid URLs.	8
	Add validation for HTTPCookie creation to prevent force-unwrapping and potential crashes Validate the creation of HTTPCookie objects in makeIncognitoCookie and makeStandardCookie methods to ensure they are not force-unwrapped, which could lead to runtime crashes. Ecosia/Core/Cookie.swift [83-86] -HTTPCookie(properties: [.name: Cookie.main.name, - .domain: ".\(urlProvider.domain ?? "")", - .path: "/", - .value: Cookie.incognitoValues.map { $0.0 + "=" + $0.1 }.joined(separator: ":")])! +guard let cookie = HTTPCookie(properties: [.name: Cookie.main.name, + .domain: ".\(urlProvider.domain ?? "")", + .path: "/", + .value: Cookie.incognitoValues.map { $0.0 + "=" + $0.1 }.joined(separator: ":")]) else { + fatalError("Failed to create HTTPCookie") +} +return cookie Suggestion importance[1-10]: 8 Why: Validating the creation of HTTPCookie objects and avoiding force-unwrapping is important to prevent runtime crashes. This suggestion enhances the safety and reliability of the code.	8
	Adjust the dictionary property to handle non-string values in the encoded object Ensure that the dictionary property correctly handles cases where the encoded object contains non-string values, as the current implementation assumes all values are strings. Ecosia/Core/Encodable+Dictionary.swift [8-10] -public var dictionary: [String: String] { - (try? JSONSerialization.jsonObject(with: JSONEncoder().encode(self))).flatMap { $0 as? [String: String] } ?? [String: String]() +public var dictionary: [String: Any] { + (try? JSONSerialization.jsonObject(with: JSONEncoder().encode(self))).flatMap { $0 as? [String: Any] } ?? [String: Any]() } Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential issue where the current implementation assumes all values in the encoded object are strings, which may not always be the case. By changing the type to [String: Any], the code becomes more robust and flexible, reducing the risk of runtime errors.	8
	Add a nil check for data before attempting JSON decoding to prevent crashes Handle cases where the data returned from the network request is nil to avoid potential crashes during JSON decoding. Ecosia/Core/FeatureManagement/Unleash/UnleashFeatureManagementSessionInitializer.swift [49-51] -guard let remoteToggles = try? JSONDecoder().decode(Unleash.FeatureResponse.self, from: data) else { +guard let data = data, let remoteToggles = try? JSONDecoder().decode(Unleash.FeatureResponse.self, from: data) else { return model as? T } Suggestion importance[1-10]: 8 Why: The suggestion adds a necessary nil check for the data variable before JSON decoding, which is a common source of crashes. This improves the robustness of the code by handling unexpected nil values gracefully.	8
	Add fallback values to prevent crashes if legacyTheme.ecosia properties are nil Ensure that the update method in EcosiaPrimaryButton handles cases where legacyTheme.ecosia properties are nil to avoid runtime crashes. Client/Ecosia/UI/EcosiaPrimaryButton.swift [28-30] private func update() { - backgroundColor = (isSelected || isHighlighted) ? .legacyTheme.ecosia.primaryButtonActive : .legacyTheme.ecosia.primaryButton + backgroundColor = (isSelected || isHighlighted) ? (.legacyTheme.ecosia?.primaryButtonActive ?? .clear) : (.legacyTheme.ecosia?.primaryButton ?? .clear) } Suggestion importance[1-10]: 7 Why: The suggestion adds fallback values to handle potential nil values in legacyTheme.ecosia properties, improving the robustness of the update method. However, the likelihood of legacyTheme.ecosia being nil might be low, slightly reducing the impact.	7
Security	Escape all user-provided strings in the bookmarkBody method to prevent injection vulnerabilities Ensure that the bookmarkBody method properly escapes all user-provided strings to avoid potential injection vulnerabilities. Ecosia/Core/Bookmarks/BookmarkSerializer.swift [58] -\(String.indent(by: indentation))<DT><A\(metadata.stringValue) HREF="\(url)">\(Entities.escape(title))</A> +\(String.indent(by: indentation))<DT><A\(metadata.stringValue) HREF="\(Entities.escape(url))">\(Entities.escape(title))</A> Suggestion importance[1-10]: 10 Why: Escaping all user-provided strings is essential to prevent injection vulnerabilities. This suggestion addresses a significant security concern and ensures the generated HTML is safe from malicious input.	10
General	Add error handling to the download method to manage potential failures during network requests Ensure that the download method properly handles errors from the dataTask completion handler to avoid potential silent failures. Ecosia/Core/Images.swift [38-47] -session.dataTask(with: url) { data, _, _ in +session.dataTask(with: url) { data, response, error in DispatchQueue.main.async { [weak self] in + if let error = error { + print("Download error: \(error.localizedDescription)") + return + } data.map { let item = Item(url, $0) self?.send(item) self?.items.insert(item) } } }.resume() Suggestion importance[1-10]: 7 Why: The suggestion enhances the download method by adding error handling for the dataTask completion handler. This ensures that network errors are logged and handled appropriately, improving the reliability and debuggability of the code.	7

---

Previous suggestions

Suggestions up to commit e5bca0a

Category	Suggestion	Score
Possible bug	Enhance the robustness of version string parsing by checking for non-integer values Consider adding a check for non-integer values in the version string components to prevent runtime errors when casting to Int. Ecosia/Helpers/Version/Version.swift [19-22] let components = versionString.split(separator: ".") guard components.count == 3, let major = Int(components[0]), let minor = Int(components[1]), let patch = Int(components[2]) else { return nil } +if components.contains(where: { Int($0) == nil }) { + return nil +} Suggestion importance[1-10]: 7 Why: This suggestion adds a check for non-integer values in the version string, which prevents potential runtime errors and enhances the robustness of the version parsing logic.	7
Enhancement	Improve the documentation for the public initializer to clarify expected input format and behavior Add documentation to the public initializer to explain the format of the version string expected and the conditions under which it returns nil. Ecosia/Helpers/Version/Version.swift [19-22] +/// Initializes a new `Version` from a string representation. +/// The string should be in the format "major.minor.patch" where all parts are integers. +/// If the format does not meet these criteria, the initialization will fail and return nil. public init?(_ versionString: String) { let components = versionString.split(separator: ".") guard components.count == 3, let major = Int(components[0]), let minor = Int(components[1]), let patch = Int(components[2]) else { return nil } } Suggestion importance[1-10]: 5 Why: Adding detailed documentation for the public initializer clarifies the expected input format and behavior, which improves code readability and usability.	5
	Add a debugDescription to provide more detailed debugging output for the Version struct Implement a custom debugDescription for the Version struct to provide more detailed debug information. Ecosia/Helpers/Version/Version.swift [34-35] public var description: String { return "\(major).\(minor).\(patch)" } +public var debugDescription: String { + return "Version(major: \(major), minor: \(minor), patch: \(patch))" +} Suggestion importance[1-10]: 3 Why: Implementing a custom debugDescription provides more detailed debug information, which can be helpful during development, but it's a minor enhancement.	3
	Include the description property in the hash(into:) method to ensure consistent hashing Ensure that the hash(into:) method also includes the description property to maintain consistency in hash calculation. Ecosia/Helpers/Version/Version.swift [75-78] public func hash(into hasher: inout Hasher) { hasher.combine(major) hasher.combine(minor) hasher.combine(patch) + hasher.combine(description) } Suggestion importance[1-10]: 2 Why: Including the description property in the hash calculation could potentially improve the uniqueness of the hash, but it might not be necessary as major, minor, and patch already provide sufficient uniqueness. This suggestion could introduce unnecessary complexity.	2

[d4r1091]

Many updates were part of this series of commits.
I wanted to take the opportunity to go the extra mile, carrying the entire Core project into the current Project.

The goals of it were essentially these:

• Move Core in
• Remove the dependencies from the Swift Package
• Make it work
• Move as many swift files and resources as possible into the Ecosia framework
• Move all tests into the Ecosia Tests targets
• Review dependencies
• Review the CI to make all tests (including Snapshots) work
• Swiftlint it ✨
• Make it solid so it doesn't have any breaking changes between Xcode versions
• Note down the next steps iterations.

I tried to follow each of the steps, always taking into account a smoother integration as part of the upcoming upgrade 🚀 .

Let's highlight the major changes worth noticing.

Dependency in BrowserKit

There was an annoying dependency on Core in BrowserKit as part of the BundleImageFetcher.swift that would lead to a flaky build step.
This is noticeable especially when checking out between branches without cleaning up your DerivedData.
With the advent of our EcosiaFramework, this flakiness became even more noticeable. To get a Build Successful, you had to first // comment out the Ecosia-related code as part of that file, let it build, uncomment, and build again.
This process is far from being ideal and we want as less dependencies/code changes in BrowserKit as possible.

After spending a considerable amount of time looking for a robust solution, I realized that the best approach in this very niche scenario would be to directly implement a part of our Ecosia.Environment.swift directly in the file.
I'm conscious this is not ideal, but that's the only approach so far that guarantees the expected Favicon's workaround to work solidly.

Some files are still in the Client/Ecosia folder

Not everything could be moved as part of this first iteration. The UI-related code, its tightly coupled dependencies, and resources will be part of a migration dedicated step that will take place at another time. There are no tickets for it yet as we may want to discuss as a team what the best approach could be.

Info.plist updates

After the EcosiaFramework creation, we began seeing a lot of different issue either at built time or runtime. The EcosiaFramework carried some further specification info to be added to both the framework itself and the App's Info.plist to make it package (creating the .app file and compiling all the frameworks) successfully.
Those additions are separated in the property list files with the Ecosia's appropriate comment.

EcosiaTests dependencies from ClientTests

The Ecosia Tests belonging to our main Client weren't written with the isolation in mind. That's totally understandable, as we would not have thought about this merge to happen anytime soon.
In order to separate the tests, some of the EcosiaTests files have a reference to our ClientTests target. This target is not added as a dependency (otherwise would have not made sense to move those tests ultimately), but some files have simply the reference. This is common pattern for a multi-target project but it was also good to mention.

Bookmarks migration tests

When assessing the tests, I realized that the bookmarks have a precise saving pattern which took Core as a path reference. As we now have a different pattern, would be good to take into account when performing the tests and, eventually, think of a migration (I will add that as part of the ticket's ACs).

---

These are the main resources to let you better review the changes.
Hope it clarified a bit the overall work and foresight behind certain choices.

[lucaschifino]

Great job! It runs very smoothly and feels great to have it all in one repo ❤️ 👏

I'm only halfway through so far (so many files 🙈) but already added some comments we can talk about, nothing major, let me know what you think.

[d4r1091]

@lucaschifino tackled all the comments 💪 - feel free to move forward with the review

[lucaschifino]

All good from my side 👏

Raised a small internal point on this ticket comment.

[github-actions]

Persistent review updated to latest commit 9d779a9

[github-actions]

Add a check to ensure that the response from singularService.getConversionValue is not nil before proceeding. This can prevent potential crashes. [important]

[github-actions]

Implement a debounce mechanism or batch updates to reduce the frequency of disk writes in the save method. This will improve performance. [important]

[github-actions]

Refactor the dictionary reduction logic into a helper function to improve readability and reduce code duplication. [medium]

[github-actions]

Refactor the persistUpdatedValues function to eliminate repetitive calls to setUserDefaults by using a loop or a mapping structure. [medium]

[github-actions]

Extend the testPatchWithAnalyticsId test to include cases where the URL already contains query parameters other than _sp. This will ensure robust URL handling. [medium]

[d4r1091]

@lucaschifino, just to keep everything tracked and as transparent as possible, I'll also comment here on the decision not to implement those Codium suggestions for now.