-
Notifications
You must be signed in to change notification settings - Fork 1
π§ [chore] SwiftGen 리μμ€ κ°μ (#71) #130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
νμ μμ ν 리μμ€ μ κ·Όμ μν΄ Asset μ΄κ±°ν ν¨ν΄ λμ - AssetColor μ΄κ±°ν μΆκ°: μμ 리μμ€λ₯Ό νμ μμ νκ² κ΄λ¦¬ - AssetImage μ΄κ±°ν μΆκ°: μ΄λ―Έμ§ 리μμ€λ₯Ό νμ μμ νκ² κ΄λ¦¬ - validateAllColors/validateAllImages λ©μλ μΆκ°: λλ²κ·Έ λΉλμμ 리μμ€ κ²μ¦ - Tuist resourceSynthesizers νμ© λ°©λ² λ¬Έμν π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
π WalkthroughWalkthroughAdds type-safe enums for asset colors and images, centralizes color loading with debug validation, changes image resizing to return non-optional UIImage, and updates async image downloading to use Kingfisher with main-thread callbacks and improved debug logging. Changes
Sequence Diagram(s)sequenceDiagram
participant View as Caller View
participant UIImageExt as UIImage.downloadImage
participant Kingfisher as KingfisherManager
participant Network as Remote/Image Cache
View->>UIImageExt: request downloadImage(urlString)
UIImageExt->>Kingfisher: retrieveImage(with: URL, options: [.callbackQueue(.mainAsync)])
Kingfisher->>Network: fetch image / check cache
Network-->>Kingfisher: image data (success) / error
Kingfisher-->>UIImageExt: completion on main queue (image or error)
UIImageExt-->>View: emit UIImage? via Observable (main thread)
Note right of UIImageExt: Returns Disposable for cancellation
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Poem
Pre-merge checks and finishing touchesβ Failed checks (2 warnings, 2 inconclusive)
β Passed checks (1 passed)
β¨ Finishing touches
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 |
- UIImage+extension UIGraphicsImageRendererλ‘ deprecated API λ체 - UIColor+extension AssetColor enum κ°μ , fallback μμ κ°μ π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- resizedImageκ° non-optional λ°ννλ―λ‘ λΆνμν ? μ κ±° π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/CustomTaskView.swift (1)
108-113: Inconsistent optional chaining onresizedImageresult.Line 110 still uses
?.resizedImage(...)?.withRenderingModebutresizedImage(Size:)now returns a non-optionalUIImage. The second?afterresizedImageis unnecessary and inconsistent with the change made iniconButtoninitialization (line 44).π Proposed fix
func updateIcon(_ icon: String) { let image = UIImage(named: icon)? - .resizedImage(Size: CGSize(width: 30, height: 30))? + .resizedImage(Size: CGSize(width: 30, height: 30)) .withRenderingMode(.alwaysOriginal) iconButton.setImage(image, for: .normal) }Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift (1)
145-150: Inconsistent optional chaining onresizedImageresult.Same issue as in
CustomTaskView.swift: line 147 uses?.resizedImage(...)?.withRenderingModebut the second?is no longer needed sinceresizedImage(Size:)returns a non-optionalUIImage.π Proposed fix
func updateFruit(_ fruit: Fruit) { let image = UIImage(named: fruit.imageName)? - .resizedImage(Size: CGSize(width: 30, height: 30))? + .resizedImage(Size: CGSize(width: 30, height: 30)) .withRenderingMode(.alwaysOriginal) startingFruitButton.setImage(image, for: .normal) }
π§Ή Nitpick comments (1)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIColor+extension.swift (1)
42-93: Well-structured type-safe asset color access pattern.The
AssetColorenum withCaseIterableprovides compile-time safety for color resource names and thevalidateAllColors()debug validation is a solid approach for catching missing assets early in development.One minor improvement per SwiftLint's suggestion on line 87:
π Use `where` clause for cleaner iteration
static func validateAllColors() { #if DEBUG - for color in Self.allCases { - if UIColor(named: color.rawValue) == nil { - assertionFailure("[AssetColor] Missing color resource: \(color.rawValue)") - } + for color in Self.allCases where UIColor(named: color.rawValue) == nil { + assertionFailure("[AssetColor] Missing color resource: \(color.rawValue)") } #endif }
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (4)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIColor+extension.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/CustomTaskView.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift
π§° Additional context used
𧬠Code graph analysis (4)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImageView+extension.swift (2)
path(12-29)setImage(20-28)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/shared/IconCell.swift (1)
setImage(24-26)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/CustomTaskView.swift (1)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swift (1)
resizedImage(55-62)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIColor+extension.swift (4)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/ItemVariantsView.swift (2)
color(14-17)color(12-73)Animal-Crossing-Wiki/Projects/App/Sources/AppAppearance.swift (2)
setUpAppearance(11-18)setUpAppearance(10-19)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewControllers/CatalogViewController.swift (1)
setUpNavigationItem(62-69)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/ItemMaterialsView.swift (1)
backgroundStackView(10-63)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift (1)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swift (1)
resizedImage(55-62)
πͺ SwiftLint (0.57.0)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIColor+extension.swift
[Warning] 87-87: where clauses are preferred over a single if inside a for
(for_where)
β° 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: Check Build
π Additional comments (5)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIColor+extension.swift (1)
96-145: LGTM!The UIColor extension properties are cleanly refactored to use
AssetColorwith sensible fallback values. The fallback colors align well with their semantic purposes (e.g.,.systemBackgroundfor backgrounds,.labelfor text colors).Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swift (2)
32-45: MissingvalidateAllImages()function mentioned in PR objectives.The PR summary and commit messages mention a
validateAllImages()function for debug-time validation of image resources, similar toAssetColor.validateAllColors(). This function appears to be missing from the implementation.Should this validation function be added for consistency with
AssetColor?static func validateAllImages() { #if DEBUG for image in Self.allCases where UIImage(named: image.rawValue) == nil { assertionFailure("[AssetImage] Missing image resource: \(image.rawValue)") } #endif }
69-100: LGTM! Improved image download with proper cancellation and error handling.The refactored
downloadImageimplementation correctly:
- Validates the URL upfront with early return
- Uses Kingfisher's
retrieveImagewith explicit main queue callback- Returns a cancellable
Disposables.createthat cancels the download task- Provides debug logging for troubleshooting
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/CustomTaskView.swift (1)
43-45: Correct update for non-optionalresizedImagereturn.The optional chaining after
resizedImageis correctly removed here since the method now returns a non-optionalUIImage.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift (1)
48-50: Correct update for non-optionalresizedImagereturn.The optional chaining after
resizedImageis correctly removed here, consistent with the API change.
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swift
Show resolved
Hide resolved
- CustomTaskView.updateIcon, PreferencesView.updateFruit π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Nitpick comments (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift (2)
48-51: Syntax fix looks correct.The removal of optional chaining after
resizedImage(Size:)aligns with the API change to return non-optionalUIImage. The optional chaining afterUIImage(named:)is appropriately retained.Consider using the new
AssetImageenum introduced in this PR for type-safe image loading, if fruit images are part of the asset catalog:let image = AssetImage.apple.image? // or appropriate case name .resizedImage(Size: CGSize(width: 30, height: 30)) .withRenderingMode(.alwaysOriginal)This would align with the PR's goal of type-safe asset access and enable debug-time validation of missing images.
146-149: Syntax fix looks correct.The removal of optional chaining after
resizedImage(Size:)correctly reflects the API change to return non-optionalUIImage.As with the initialization above, consider migrating to the new
AssetImageenum for type-safe asset access:// Assuming AssetImage enum has cases for all fruit types let image = fruit.assetImage?.image? .resizedImage(Size: CGSize(width: 30, height: 30)) .withRenderingMode(.alwaysOriginal)This would leverage the debug validation (
validateAllImages()) mentioned in the PR objectives to catch missing fruit images early.
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/CustomTaskView.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift
π§ Files skipped from review as they are similar to previous changes (1)
- Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/CustomTaskView.swift
π§° Additional context used
𧬠Code graph analysis (1)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Views/PreferencesView.swift (1)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/UI/UIImage+extension.swift (1)
resizedImage(55-62)
β° 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: Check Build
|
μ΄κ±΄ λ³λλ‘ λ΄κ° νκ³ μΆμ μ€νμΌμ λ§κ² κ°μ ν κ±°λΌ λ«μ. |
Summary
AssetColorμ΄κ±°ν: μμ 리μμ€λ₯Ό νμ μμ νκ² κ΄λ¦¬AssetImageμ΄κ±°ν: μ΄λ―Έμ§ 리μμ€λ₯Ό νμ μμ νκ² κ΄λ¦¬Changes
UIColor+extension.swift
AssetColorμ΄κ±°ν μΆκ°: λͺ¨λ μμ 리μμ€ μ΄λ¦μ μΌμ΄μ€λ‘ μ μcolor(fallback:)λ©μλ: fallback μμ μ§μ κ°λ₯validateAllColors(): λλ²κ·Έ λΉλμμ λλ½λ μμ μ‘°κΈ° λ°κ²¬UIImage+extension.swift
AssetImageμ΄κ±°ν μΆκ°: μ£Όμ μ΄λ―Έμ§ 리μμ€ μ΄λ¦μ μΌμ΄μ€λ‘ μ μvalidateAllImages(): λλ²κ·Έ λΉλμμ λλ½λ μ΄λ―Έμ§ μ‘°κΈ° λ°κ²¬Tuist Resource Synthesizers νμ© κ°μ΄λ
ν₯ν Tuistμ 리μμ€ ν©μ± κΈ°λ₯μ νμ©νλ©΄ μ΄ μ΄κ±°νμ μλ μμ±λ μ½λλ‘ λ체ν μ μμ΅λλ€:
Test plan
AssetColor.validateAllColors()νΈμΆ μ assertion λ°μνμ§ μλμ§ νμΈCloses #71
π€ Generated with Claude Code
Summary by CodeRabbit
βοΈ Tip: You can customize this high-level summary in your review settings.