fix: CodeRabbit auto-fixes for PR #78 - #79
Merged
Conversation
Fixed 8 file(s) based on 13 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Merged
Contributor
Reviewer's GuideRefines Shimeji download management, safety, caching, and animation behavior, adjusts related settings UI flow, and makes theme clouds respect the active color scheme. Sequence diagram for the updated Shimeji download and cancellation flowsequenceDiagram
actor User
participant SettingsView
participant ShimejiSettingsView
participant ShimejiResourceManager
participant URLSession
participant ShimejiZipReader
participant ShimejiSpriteView
User->>SettingsView: toggleShimeji.onChange(newValue=true)
SettingsView->>ShimejiResourceManager: download()
ShimejiResourceManager->>URLSession: downloadTask(with: packURL)
URLSession-->>ShimejiResourceManager: handleDownloadCompletion(tempURL,response,error)
ShimejiResourceManager->>ShimejiZipReader: extractAll(from: tempURL,to: packDirectory)
ShimejiZipReader-->>ShimejiResourceManager: manifest
ShimejiResourceManager->>ShimejiSpriteView: invalidateImageCache()
alt User cancels while downloading
User->>SettingsView: toggleShimeji.onChange(newValue=false)
SettingsView->>ShimejiResourceManager: cancelDownload()
ShimejiResourceManager->>URLSession: downloadTask.cancel()
ShimejiResourceManager->>ShimejiResourceManager: state = notDownloaded
end
alt User manually starts download in ShimejiSettingsView
User->>ShimejiSettingsView: tap Download button
ShimejiSettingsView->>ShimejiResourceManager: download()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift" line_range="15" />
<code_context>
+ init() {
+ cache = NSCache<NSString, UIImage>()
+ cache.countLimit = 200
+ cache.totalCostLimit = 50 * 1024 * 1024 // 50 MB
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Total cost limit on NSCache is ineffective without providing per-object costs.
`image(at:)` uses `cache.setObject(image, forKey: key)` without a `cost`, so `totalCostLimit` won’t affect eviction. To make the 50 MB cap meaningful, estimate each `UIImage`’s memory usage (e.g. pixels * bytes per pixel) and pass that to `setObject(_:forKey:cost:)` so NSCache can evict based on actual cost.
Suggested implementation:
```
/// re-read the same handful of small PNGs from disk every cycle.
private final class ShimejiImageCache {
static let shared = ShimejiImageCache()
private let cache: NSCache<NSString, UIImage>
init() {
cache = NSCache<NSString, UIImage>()
cache.countLimit = 200
cache.totalCostLimit = 50 * 1024 * 1024 // 50 MB
}
```
```
func image(at url: URL) -> UIImage? {
let key = url.path as NSString
// Assuming `image` is created/loaded earlier in this method.
// Compute an approximate memory cost so `totalCostLimit` is effective.
if let image = image {
let scale = image.scale
let pixelsWide = image.size.width * scale
let pixelsHigh = image.size.height * scale
let bytesPerPixel: CGFloat = 4 // RGBA8
let cost = Int(pixelsWide * pixelsHigh * bytesPerPixel)
cache.setObject(image, forKey: key, cost: cost)
return image
}
return nil
```
1. Ensure that within `image(at:)` you have logic that actually loads or retrieves a `UIImage` into a local variable named `image` before the `if let image = image` block (e.g. from disk or another cache).
2. If the method previously returned a cached image early (e.g. `if let cached = cache.object(forKey: key) { return cached }`), keep that logic intact above this new block so you avoid reloading images unnecessarily.
3. If you prefer different cost semantics (e.g. using `cgImage.bytesPerRow * cgImage.height` when available), you can adjust the cost calculation accordingly but still pass it as the `cost:` parameter to `setObject(_:forKey:cost:)`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| init() { | ||
| cache = NSCache<NSString, UIImage>() | ||
| cache.countLimit = 200 | ||
| cache.totalCostLimit = 50 * 1024 * 1024 // 50 MB |
Contributor
There was a problem hiding this comment.
suggestion (performance): Total cost limit on NSCache is ineffective without providing per-object costs.
image(at:) uses cache.setObject(image, forKey: key) without a cost, so totalCostLimit won’t affect eviction. To make the 50 MB cap meaningful, estimate each UIImage’s memory usage (e.g. pixels * bytes per pixel) and pass that to setObject(_:forKey:cost:) so NSCache can evict based on actual cost.
Suggested implementation:
/// re-read the same handful of small PNGs from disk every cycle.
private final class ShimejiImageCache {
static let shared = ShimejiImageCache()
private let cache: NSCache<NSString, UIImage>
init() {
cache = NSCache<NSString, UIImage>()
cache.countLimit = 200
cache.totalCostLimit = 50 * 1024 * 1024 // 50 MB
}
func image(at url: URL) -> UIImage? {
let key = url.path as NSString
// Assuming `image` is created/loaded earlier in this method.
// Compute an approximate memory cost so `totalCostLimit` is effective.
if let image = image {
let scale = image.scale
let pixelsWide = image.size.width * scale
let pixelsHigh = image.size.height * scale
let bytesPerPixel: CGFloat = 4 // RGBA8
let cost = Int(pixelsWide * pixelsHigh * bytesPerPixel)
cache.setObject(image, forKey: key, cost: cost)
return image
}
return nil
- Ensure that within
image(at:)you have logic that actually loads or retrieves aUIImageinto a local variable namedimagebefore theif let image = imageblock (e.g. from disk or another cache). - If the method previously returned a cached image early (e.g.
if let cached = cache.object(forKey: key) { return cached }), keep that logic intact above this new block so you avoid reloading images unnecessarily. - If you prefer different cost semantics (e.g. using
cgImage.bytesPerRow * cgImage.heightwhen available), you can adjust the cost calculation accordingly but still pass it as thecost:parameter tosetObject(_:forKey:cost:).
Collaborator
@coderabbitai please rework your PR to fix issues found by sourcery-ai |
Contributor
Author
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift --items all --view expanded
printf '\n--- relevant cache implementation ---\n'
rg -n -C 12 'ShimejiImageCache|func image\s*\(|setObject\s*\(' Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift
printf '\n--- cache references and tests ---\n'
rg -n -C 4 'ShimejiImageCache|ShimejiSpriteView' -g '*.swift' .Length of output: 11724
You are interacting with an AI system. |
Contributor
Author
|
✅ The requested changes have been implemented and a pull request has been created: View PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This stacked PR contains CodeRabbit auto-fixes for #78.
Files modified:
Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swiftTwinskaraoke/Features/Account/SettingsView.swiftTwinskaraoke/Features/Account/ShimejiSettingsView.swiftTwinskaraoke/Services/Shimeji/ShimejiEngine.swiftTwinskaraoke/Services/Shimeji/ShimejiResourceManager.swiftTwinskaraoke/Services/Shimeji/ShimejiZipReader.swiftTwinskaraoke/Theme/AuroraBackgroundViews.swiftTwinskaraokeShared/Localization/Localizable.xcstringsSummary by Sourcery
Improve robustness and safety of Shimeji asset downloading, extraction, and rendering while refining related UI behavior.
New Features:
Bug Fixes:
Enhancements: