ProductImageStatusStorage class for managing stored images statuses in User Defaults
#15256
Conversation
…in User Defaults. This file has been created in another branch, but given the size of the branch/PR, I'm splitting them, and I renamed the class from `ProductImagesUserDefaultsStatuses` to `ProductImageStatusStorage`.
pmusolino
added
the
feature: add/edit products
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a new feature flag for background product image uploads by adding a corresponding case in both the feature flag enum and the default feature flag service. It also enhances the product image status functionality by adding computed properties and implementing a new storage class that manages statuses reactively using Combine and User Defaults. Additionally, unused test file references for the FeatureFlag have been removed from the project configuration and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultFeatureFlagService
participant BuildConfig
Client->>DefaultFeatureFlagService: isFeatureFlagEnabled(.backgroundProductImageUpload)
DefaultFeatureFlagService->>BuildConfig: Check buildConfig (localDeveloper or alpha)
BuildConfig-->>DefaultFeatureFlagService: Return boolean result
DefaultFeatureFlagService-->>Client: Return true/false
sequenceDiagram
participant User
participant ProductImageStatusStorage
participant UserDefaults
participant Observer
User->>ProductImageStatusStorage: addStatus(newStatus)
ProductImageStatusStorage->>UserDefaults: saveStatuses(updatedStatuses)
UserDefaults-->>ProductImageStatusStorage: Confirm save
ProductImageStatusStorage->>Observer: statusesPublisher emits update
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Networking/Networking/ProductImageInBackground/ProductImageStatusStorage.swift (6)
9-19: Potentially unused private static instance.
private static let internalInstancemight be dead code if you never expose or use it elsewhere. Consider removing to keep the codebase clean.
23-28: Main-thread dispatch for status publication might block.
Publishing onDispatchQueue.mainis convenient for UI updates, but be mindful of potential performance impact if large data sets are processed frequently.
49-61: Usage ofUserDefaults.synchronize()is discouraged.
Apple generally recommends against callingsynchronize()as it’s automatically managed. Consider removing to avoid potential performance overhead.
109-113: Consider consistency in clearing statuses.
You reset to[]but you callremoveObjectinstead of encoding an empty array. Both are valid; consider uniform usage ofsaveStatuses([]).
133-142: Consider a logging approach instead of
Printing can be overlooked in production environments. A logging framework or error-handling flow might be beneficial.
144-151: Timely decoding error messages, but same logging note as above.
Again, consider a centralized or structured logging method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Experiments/Experiments/DefaultFeatureFlagService.swift(1 hunks)Experiments/Experiments/FeatureFlag.swift(1 hunks)Networking/Networking.xcodeproj/project.pbxproj(4 hunks)Networking/Networking/ProductImageInBackground/ProductImageStatus.swift(1 hunks)Networking/Networking/ProductImageInBackground/ProductImageStatusStorage.swift(1 hunks)WooCommerce/WooCommerce.xcodeproj/project.pbxproj(0 hunks)WooCommerce/WooCommerceTests/System/FeatureFlagTests.swift(0 hunks)
💤 Files with no reviewable changes (2)
- WooCommerce/WooCommerce.xcodeproj/project.pbxproj
- WooCommerce/WooCommerceTests/System/FeatureFlagTests.swift
🔇 Additional comments (23)
Experiments/Experiments/FeatureFlag.swift (1)
199-202: Feature flag addition looks goodThe new feature flag for background product image uploads is well-documented and follows the same pattern as other feature flags in the enum. The placement at the end of the enum is appropriate.
Experiments/Experiments/DefaultFeatureFlagService.swift (1)
94-95: Implementation is consistent with other development featuresThe implementation for the new feature flag follows the same pattern as other development features by enabling it only for local development and alpha builds, which is appropriate for a feature under development.
Networking/Networking.xcodeproj/project.pbxproj (1)
462-462: Project file updated correctly for the new ProductImageStatusStorage.swiftThe project file has been properly updated to include the new file:
- Added to PBXBuildFile section for compilation
- Added to PBXFileReference section with correct file type
- Placed in the appropriate ProductImageInBackground group
- Included in the Sources build phase
The changes are consistent with the PR objectives to introduce a new storage class for managing product image upload statuses.
Also applies to: 1680-1680, 2715-2715, 5507-5507
Networking/Networking/ProductImageInBackground/ProductImageStatus.swift (7)
89-94: Straightforward computed property for checking upload status.
The code cleanly checks if the enum case is.uploading.
96-101: Clear computed property for checking upload failure status.
This mirrors the structure ofisUploadingand is equally well implemented.
103-110: Site ID extraction appears correct.
All enum cases are handled. This is straightforward and accurate.
112-119: Concise retrieval of productOrVariationID.
Clean pattern matching to retrieve the associated value from all relevant cases.
121-129: Conditional assetType property.
ReturnsProductImageAssetTypefor.uploadingand.uploadFailureornilotherwise. This is logically sound.
131-137: Error property is well-scoped.
It correctly returns the error associated with.uploadFailure, simplifying checks in caller code.
138-143: isLocalID logic is clear.
It appropriately checks for an ID of 0, indicating a local product or variation.Networking/Networking/ProductImageInBackground/ProductImageStatusStorage.swift (13)
1-3: Imports are straightforward.
No issues here.
4-8: Class documentation is clear and informative.
Helps future maintainers understand the rationale behind Combine usage and the data flow.
20-22: Encoder/decoder property definitions appear correct.
No issues, as explicit control over JSON encoding is often necessary.
29-47: Only first failure is emitted inerrorsPublisher.
If multiple statuses experience upload failures simultaneously, this logic captures only the first. Confirm this is the intended behavior.
63-67: Add status method is succinct and functional.
Appends to the current list, then persists.
69-72: Remove status method is straightforward.
No apparent issues.
74-77: removeStatus(where:) provides a flexible filter removal.
This meets typical removal needs elegantly.
79-87: updateStatus method merges add and replace flows.
The approach is neat and aligns with typical “upsert” logic.
89-91: Simple findStatus method for retrieving the first match.
Looks good and is consistent with the rest of the API.
93-95: getAllStatuses method is minimal and clear.
No concerns here.
97-107: Filtering statuses by site and product ID.
Handles the cases for all statuses thoroughly.
115-131: setAllStatuses methods are intuitive.
They replace or merge statuses as expected.
155-184: Reactive user defaults observation.
The approach is valid. However, if writes happen on a background thread, concurrency needs careful handling. Confirm that the main-thread checks suffice.
Part 3 of the changes needed for product image upload in the background.
Before merging this PR, make sure to merge this one.
Description
This PR includes:
ProductImageStatusStoragewill facilitate the storage of product image upload statuses in User Defaults. This class utilizes the Combine framework to manage notifications and monitor actual data changes. Currently, this class is not yet implemented in the code but will be incorporated in future pull requests.backgroundProductImageUpload. Every functionality for image upload in background will be hidden under this feature flag.ProductImageStatusthat will be useful to quickly check the statuses of product images.All these changes are part of the larger project for uploading product images while the app is in the background state.
In the next PR, expect the unit tests for
ProductImageStatusStorage. Otherwise, the PR would have been too big.Testing information
This code is still not used in the app, so just review the code for the moment.
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: