Add Shimejees - #78
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds an experimental Shimeji feature: downloadable character packs rendered in an overlay window with physics/interaction, controlled via new settings and lifecycle hooks, and integrated with the mini player/tab bar layout. Sequence diagram for enabling a Shimeji sessionsequenceDiagram
actor User
participant SettingsView
participant ShimejiResourceManager
participant ShimejiSessionModifier
participant ShimejiOverlayController
participant ShimejiEngine
User->>SettingsView: toggle shimejiToggleBinding
SettingsView->>ShimejiResourceManager: download
ShimejiResourceManager-->>ShimejiResourceManager: extractAndLoad
ShimejiResourceManager-->>ShimejiSessionModifier: manifest updated
ShimejiSessionModifier->>ShimejiSessionModifier: updateSession(active)
ShimejiSessionModifier->>ShimejiOverlayController: show
ShimejiSessionModifier->>ShimejiEngine: start(manifest)
ShimejiEngine-->>ShimejiEngine: respawn(manifest)
ShimejiEngine-->>ShimejiEngine: tick
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a UIKit-gated Shimeji system with downloadable character resources, physics-driven sprites, a touch-through overlay window, experimental settings, session lifecycle wiring, and localization updates. Theme cloud colors now follow the active color scheme. ChangesShimeji resource pipeline
Runtime overlay and configuration
Presentation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant ShimejiResourceManager
participant ShimejiSessionModifier
participant ShimejiOverlayController
participant ShimejiEngine
SettingsView->>ShimejiResourceManager: download Shimeji pack
ShimejiResourceManager-->>ShimejiSessionModifier: publish ready manifest
ShimejiSessionModifier->>ShimejiOverlayController: show overlay
ShimejiSessionModifier->>ShimejiEngine: start engine with manifest
ShimejiOverlayController->>ShimejiEngine: update bounds and floor positions
ShimejiEngine-->>ShimejiOverlayController: provide sprite instances
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
![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, and left some high level feedback:
- The Shimeji pack URL is hard-coded to a personal domain; consider making
ShimejiResourceManager.packURLconfigurable (e.g., via build config or an app-level constant) so it can be swapped without code changes and is clearly marked as temporary. - ShimejiZipReader currently loads the entire ZIP into memory with
Data(contentsOf:options:); if the pack size grows this may become costly, so you may want to switch to a streaming approach or enforce a max size to guard against large archives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Shimeji pack URL is hard-coded to a personal domain; consider making `ShimejiResourceManager.packURL` configurable (e.g., via build config or an app-level constant) so it can be swapped without code changes and is clearly marked as temporary.
- ShimejiZipReader currently loads the entire ZIP into memory with `Data(contentsOf:options:)`; if the pack size grows this may become costly, so you may want to switch to a streaming approach or enforce a max size to guard against large archives.
## Individual Comments
### Comment 1
<location path="Twinskaraoke/Services/Shimeji/ShimejiZipReader.swift" line_range="48-57" />
<code_context>
+ private static func readCentralDirectory(_ data: Data) throws -> [Entry] {
</code_context>
<issue_to_address>
**issue (bug_risk):** Add bounds checks before reading EOCD and central directory fields to avoid out-of-range memory access on corrupt archives.
`u16`/`u32` currently assume `eocdOffset + 10/12/16` and `centralDirOffset + 46 + nameLength + extraLength + commentLength` are always within `data`, but there’s no check that the EOCD and central directory headers are fully present before reading. On truncated or malformed ZIPs this can cause out-of-bounds reads. Please add size checks like `data.count >= eocdOffset + 22` (minimum EOCD size) and similar bounds validation before each `u16`/`u32` access, returning `corruptArchive` when the data is too short.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
✅ Merge conflicts resolved successfully! Resolved 1 conflict file(s). Commit: 15 file operation(s)
View agent analysis |
Resolved conflicts in: - TwinskaraokeShared/Localization/Localizable.xcstrings (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
Twinskaraoke/Services/Shimeji/ShimejiModels.swift (1)
36-40: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding against non-positive
frameDurationfrom a remote manifest.Since the pack is remote-hosted and unversioned within
formatVersion, aframeDurationof0or a negative value would flow straight into the animation timer. A validating initializer (or clamping at use site) makes the engine resilient to a bad pack.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Services/Shimeji/ShimejiModels.swift` around lines 36 - 40, Update ShimejiActionDefinition to validate frameDuration when decoding remote manifests, rejecting or safely normalizing zero and negative values before they reach the animation timer; preserve valid positive durations and the existing Codable behavior.Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift (1)
129-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate pack-directory derivation.
base/packDirectoryare recomputed here because thelazy varis main-actor isolated. Anonisolated statichelper used by both the property and this method would keep the two paths from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift` around lines 129 - 138, Extract the application-support and ShimejiPack path construction from extractAndLoad into a nonisolated static helper, then reuse that helper for both the lazy pack-directory property and extractAndLoad. Remove the duplicate base/packDirectory derivation while preserving the existing directory path.Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift (1)
54-67: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
show(in:)silently ignores a different scene.The
guard window == nilearly-return means that if the overlay is already up in scene A andshow(in: sceneB)arrives (multi-window iPad, or a scene swap), the overlay stays attached to the stale scene and bounds tracking keeps polling the old one. Consider re-hosting when the incoming scene differs from the currentwindow?.windowScene.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift` around lines 54 - 67, Update show(in:) to handle an incoming scene different from the current window?.windowScene instead of returning whenever a window exists. Reuse the existing overlay setup for the new scene, remove or replace the stale window and bounds tracking as needed, and ensure tracking follows the incoming scene while preserving the no-op behavior when the window is already hosted in that same scene.Twinskaraoke/Components/Shimeji/ShimejiFloorRegistry.swift (2)
55-70: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFull window-hierarchy walk every 0.5 s.
firstTabBarrecurses through every subview of every scene window on each poll tick for as long as the overlay is visible. Caching the discoveredUITabBarin aweak varand only re-walking when it'snilor detached would cut this to a single traversal in the common case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Components/Shimeji/ShimejiFloorRegistry.swift` around lines 55 - 70, The tab-bar lookup currently traverses every scene window hierarchy on each polling tick. Add a weak cached UITabBar property near the floor-registry state, reuse it in the window-floor lookup when it remains attached and valid, and call firstTabBar(in:) only when the cache is nil or detached; update the cache when a tab bar is discovered while preserving the existing visibility and frame checks.
17-22: 🩺 Stability & Availability | 🔵 TrivialMake the documented main-thread invariant compiler-enforced.
SWIFT_VERSION = 5.0, so Swift 6 isolation is not enabled here; this is a future-proofing refactor rather than a current build error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Components/Shimeji/ShimejiFloorRegistry.swift` around lines 17 - 22, Update the ShimejiMiniPlayerTracker declaration to enforce its documented main-thread invariant at compile time, using the appropriate Swift concurrency isolation annotation while remaining compatible with the project’s Swift 5.0 setting. Keep the singleton, weak barView reference, and private initializer unchanged.Twinskaraoke/Features/Account/ShimejiSettingsView.swift (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two manifest checks into one
if let.
resources.manifest != nilfollowed immediately byif let manifest = resources.manifestevaluates the same condition twice for adjacent sections.♻️ Proposed simplification
statusSection - if resources.manifest != nil { - behaviorSection - } if let manifest = resources.manifest { + behaviorSection spawnCountSection charactersSection(manifest: manifest) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Features/Account/ShimejiSettingsView.swift` around lines 11 - 17, Update the view’s manifest handling to use a single if-let binding that conditionally renders behaviorSection, spawnCountSection, and charactersSection(manifest:) together. Remove the separate resources.manifest != nil check and reuse the bound manifest for all sections.Twinskaraoke/Components/Shimeji/ShimejiSessionModifier.swift (1)
19-31: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPause Shimeji lifecycle when the app backgrounds or becomes inactive.
ShimejiSessionModifier.updateSession(active:)currently resumes only when aforegroundActivescene is found and otherwise callsstop()/hide()only from the session toggle or pack readiness. Add.onChange(\.scenePhase)or app lifecycle handling so the engine’s physics tick andShimejiOverlayController.boundsTrackingTimerdo not keep scheduling work while the app is background/inactive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Components/Shimeji/ShimejiSessionModifier.swift` around lines 19 - 31, The Shimeji lifecycle must react to app scene activity, not only the session toggle and manifest changes. Update ShimejiSessionModifier.body to observe scenePhase, and use the existing updateSession(active:) lifecycle path to stop or hide the engine when the scene is inactive/backgrounded and resume only when both the session is enabled and the scene is foreground active, ensuring physics and bounds tracking stop scheduling work offscreen.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift`:
- Around line 26-33: The sprite view duplicates the sprite size, allowing
rendering and hit testing to diverge. In
Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift lines 26-33, remove the
local displaySize property and use ShimejiEngine.displaySize for both the frame
dimensions and position offset; in
Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift lines 22-41, make no
direct change because it already uses ShimejiEngine.displaySize.
In `@Twinskaraoke/Features/Account/SettingsView.swift`:
- Around line 295-306: Update shimejiToggleBinding to call
ShimejiResourceManager.shared.cancelDownload() when the toggle is turned off,
ensuring any in-flight transfer is stopped and state is reset. Also update the
Shimeji settings footer to display the pack size before download, reusing the
existing resource metadata rather than starting or duplicating download logic.
In `@Twinskaraoke/Features/Account/ShimejiSettingsView.swift`:
- Around line 24-28: Update the resources status-row UI in ShimejiSettingsView
so the .notDownloaded state displays an explicit Download button instead of an
indeterminate ProgressView. Have the button invoke resources.download(), while
preserving automatic download triggering in the existing onAppear flow.
In `@Twinskaraoke/Services/Shimeji/ShimejiEngine.swift`:
- Around line 77-82: Update the ShimejiEngine properties bounds and navBarY so
changes invoke handleFloorTargetChange(), using guarded observers to avoid
unnecessary handling during initialization or unchanged values. Ensure layout,
rotation, and tab-bar updates recalculate existing sprites’ floor transitions
while preserving the existing miniPlayerY and isNowPlayingOpen behavior.
- Around line 232-241: Update the action-switching logic associated with
currentFrames and frameIndex to immediately normalize frameIndex to a valid
index when the new frame collection is empty or shorter than the previous one.
Reset frameIndex and frameTimer when there are zero or one frames, before any
renderer can access currentFrames[frameIndex]; preserve normal multi-frame
advancement in advanceFrame.
- Around line 174-175: Update the count calculation in the Shimeji instance
creation flow to clamp the persisted settings.maxCount to a nonnegative value
before constructing 0..<count, while retaining the upper limit of 12. Ensure
negative decoded values produce zero instances instead of trapping.
- Around line 151-157: The start(manifest:) flow must not call respawn or create
the display link while Shimeji bounds are zero. Retain the active manifest and
defer spawning/link creation until the overlay bounds become non-zero, then
respawn that manifest; ensure later bounds updates are not blocked by the
displayLink guard.
In `@Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift`:
- Line 16: Replace the contributor-hosted packURL in ShimejiResourceManager with
a project-controlled distribution URL, and add a pinned SHA-256 or
signed-manifest verification step before ShimejiZipReader processes or writes
the archive. Ensure downloads are rejected when the source or integrity
verification fails, preserving extraction only for verified packs.
- Around line 165-172: Invalidate cached sprite images when downloaded packs are
removed or re-extracted. In ShimejiResourceManager.deleteDownloadedPack and
after successful extraction in extractAndLoad, call
ShimejiImageCache.removeAll(); in ShimejiSpriteView’s ShimejiImageCache, add
removeAll() wrapping cache.removeAllObjects() and configure the NSCache with
explicit countLimit and totalCostLimit values.
- Around line 66-83: Update download() and handleDownloadCompletion to retain
and validate the URLResponse HTTP status before parsing the temporary file,
treating non-2xx responses as network failures and surfacing the corresponding
error instead of passing the body to the zip parser. Configure the URLSession
request with an explicit resource timeout suitable for the large pack download,
while preserving progress reporting and existing success handling.
In `@Twinskaraoke/Services/Shimeji/ShimejiZipReader.swift`:
- Around line 53-106: Make all ZIP offset and length reads in ShimejiZipReader
bounds-safe so malformed archives throw ZipError.corruptArchive instead of
trapping. Update the local u16/u32 helpers to validate their requested ranges
and propagate failure, validate the EOCD’s complete 22-byte layout before
reading its fields, and check each central-directory record and name range
before indexing or advancing cursor. In extractFileData, validate localOffset
and all local-header/data ranges using the same safe helpers before reading.
- Around line 26-34: Update the archive extraction loop in ShimejiZipReader to
validate each entry.path before creating directories or writing files: reject
absolute paths and any path containing a ".." component, then resolve the
destination and verify it remains contained under destinationDirectory. Only
perform createDirectory, extractFileData, and write after this validation
succeeds.
In `@Twinskaraoke/Theme/AuroraBackgroundViews.swift`:
- Around line 58-61: Update FloatingClouds to read the active
`@Environment`(\.colorScheme) value and pass it as the scheme argument to the
Theme.ellipsesTopLeading, ellipsesBottomTrailing, ellipsesTopTrailing, and
ellipsesBottomLeading helpers instead of using the always-dark
FloatingClouds.scheme.
In `@TwinskaraokeShared/Localization/Localizable.xcstrings`:
- Around line 166-177: Update the localization entry for "%@ ready — %lld
characters" to support plural variation on the %lld argument, including singular
and plural forms while preserving the resource-pack name placeholder. Ensure the
English localization renders “1 character” and uses the plural form for other
counts, and structure the entry so languages with richer plural rules can
provide appropriate translations.
---
Nitpick comments:
In `@Twinskaraoke/Components/Shimeji/ShimejiFloorRegistry.swift`:
- Around line 55-70: The tab-bar lookup currently traverses every scene window
hierarchy on each polling tick. Add a weak cached UITabBar property near the
floor-registry state, reuse it in the window-floor lookup when it remains
attached and valid, and call firstTabBar(in:) only when the cache is nil or
detached; update the cache when a tab bar is discovered while preserving the
existing visibility and frame checks.
- Around line 17-22: Update the ShimejiMiniPlayerTracker declaration to enforce
its documented main-thread invariant at compile time, using the appropriate
Swift concurrency isolation annotation while remaining compatible with the
project’s Swift 5.0 setting. Keep the singleton, weak barView reference, and
private initializer unchanged.
In `@Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift`:
- Around line 54-67: Update show(in:) to handle an incoming scene different from
the current window?.windowScene instead of returning whenever a window exists.
Reuse the existing overlay setup for the new scene, remove or replace the stale
window and bounds tracking as needed, and ensure tracking follows the incoming
scene while preserving the no-op behavior when the window is already hosted in
that same scene.
In `@Twinskaraoke/Components/Shimeji/ShimejiSessionModifier.swift`:
- Around line 19-31: The Shimeji lifecycle must react to app scene activity, not
only the session toggle and manifest changes. Update ShimejiSessionModifier.body
to observe scenePhase, and use the existing updateSession(active:) lifecycle
path to stop or hide the engine when the scene is inactive/backgrounded and
resume only when both the session is enabled and the scene is foreground active,
ensuring physics and bounds tracking stop scheduling work offscreen.
In `@Twinskaraoke/Features/Account/ShimejiSettingsView.swift`:
- Around line 11-17: Update the view’s manifest handling to use a single if-let
binding that conditionally renders behaviorSection, spawnCountSection, and
charactersSection(manifest:) together. Remove the separate resources.manifest !=
nil check and reuse the bound manifest for all sections.
In `@Twinskaraoke/Services/Shimeji/ShimejiModels.swift`:
- Around line 36-40: Update ShimejiActionDefinition to validate frameDuration
when decoding remote manifests, rejecting or safely normalizing zero and
negative values before they reach the animation timer; preserve valid positive
durations and the existing Codable behavior.
In `@Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift`:
- Around line 129-138: Extract the application-support and ShimejiPack path
construction from extractAndLoad into a nonisolated static helper, then reuse
that helper for both the lazy pack-directory property and extractAndLoad. Remove
the duplicate base/packDirectory derivation while preserving the existing
directory path.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a88916a4-a2cc-4db6-987f-85e8cec9ffeb
📒 Files selected for processing (15)
Twinskaraoke/App/ContentView.swiftTwinskaraoke/App/TwinskaraokeApp.swiftTwinskaraoke/Components/Shimeji/ShimejiFloorRegistry.swiftTwinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swiftTwinskaraoke/Components/Shimeji/ShimejiSessionModifier.swiftTwinskaraoke/Components/Shimeji/ShimejiSpriteView.swiftTwinskaraoke/Features/Account/SettingsView.swiftTwinskaraoke/Features/Account/ShimejiSettingsView.swiftTwinskaraoke/Features/Home/HomeView.swiftTwinskaraoke/Services/Shimeji/ShimejiEngine.swiftTwinskaraoke/Services/Shimeji/ShimejiModels.swiftTwinskaraoke/Services/Shimeji/ShimejiResourceManager.swiftTwinskaraoke/Services/Shimeji/ShimejiZipReader.swiftTwinskaraoke/Theme/AuroraBackgroundViews.swiftTwinskaraokeShared/Localization/Localizable.xcstrings
Comment on lines
+26
to
+33
| private let displaySize: CGFloat = 84 | ||
|
|
||
| var body: some View { | ||
| currentImage | ||
| .resizable() | ||
| .interpolation(.none) | ||
| .scaledToFit() | ||
| .frame(width: displaySize, height: displaySize) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Single sprite size, declared twice. The rendered frame size and the touch-target size are independent constants, so any change to one silently breaks hit testing against the other.
Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift#L26-L33: deleteprivate let displaySize: CGFloat = 84and useShimejiEngine.displaySizefor both.frame(...)and the.positionoffset.Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift#L22-L41: keep readingShimejiEngine.displaySize— no change needed once the view stops redeclaring it.
📍 Affects 2 files
Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift#L26-L33(this comment)Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift#L22-L41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift` around lines 26 -
33, The sprite view duplicates the sprite size, allowing rendering and hit
testing to diverge. In Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift
lines 26-33, remove the local displaySize property and use
ShimejiEngine.displaySize for both the frame dimensions and position offset; in
Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift lines 22-41, make no
direct change because it already uses ShimejiEngine.displaySize.
Comment on lines
+295
to
+306
| private var shimejiToggleBinding: Binding<Bool> { | ||
| Binding( | ||
| get: { shimejiEnabled }, | ||
| set: { newValue in | ||
| shimejiEnabled = newValue | ||
| newValue ? AppHaptic.success.play() : AppHaptic.light.play() | ||
| if newValue, case .notDownloaded = ShimejiResourceManager.shared.state { | ||
| ShimejiResourceManager.shared.download() | ||
| } | ||
| } | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Flipping the toggle starts an unbounded network download with no consent or cancellation.
Enabling Shimeji immediately fetches the pack (and ShimejiSettingsView.onAppear does the same), with no size shown to the user and no way to stop it — turning the toggle back off leaves the transfer running because download() isn't cancelled. On cellular this silently consumes data.
Minimum: cancel the in-flight download when the toggle goes off, and surface the pack size in the footer.
♻️ Proposed change
set: { newValue in
shimejiEnabled = newValue
newValue ? AppHaptic.success.play() : AppHaptic.light.play()
if newValue, case .notDownloaded = ShimejiResourceManager.shared.state {
ShimejiResourceManager.shared.download()
+ } else if !newValue, case .downloading = ShimejiResourceManager.shared.state {
+ ShimejiResourceManager.shared.cancelDownload()
}
}(cancelDownload() would cancel downloadTask, clear progressObservation, and reset state to .notDownloaded.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Features/Account/SettingsView.swift` around lines 295 - 306,
Update shimejiToggleBinding to call
ShimejiResourceManager.shared.cancelDownload() when the toggle is turned off,
ensuring any in-flight transfer is stopped and state is reset. Also update the
Shimeji settings footer to display the pack size before download, reusing the
existing resource metadata rather than starting or duplicating download logic.
Comment on lines
+24
to
+28
| .onAppear { | ||
| if case .notDownloaded = resources.state { | ||
| resources.download() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
After "Remove Downloaded Pack", the status row spins forever.
deleteDownloadedPack() resets state to .notDownloaded, but the auto-download in onAppear has already fired for this view instance, so nothing re-triggers it. The user is left staring at "Waiting to download…" plus an indeterminate ProgressView with no affordance. Give .notDownloaded an explicit "Download" button rather than an implied-in-progress spinner.
Also applies to: 35-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Features/Account/ShimejiSettingsView.swift` around lines 24 -
28, Update the resources status-row UI in ShimejiSettingsView so the
.notDownloaded state displays an explicit Download button instead of an
indeterminate ProgressView. Have the button invoke resources.download(), while
preserving automatic download triggering in the existing onAppear flow.
Comment on lines
+77
to
+82
| @Published var bounds: CGRect = .zero | ||
|
|
||
| /// The app's actual tab bar top edge, in the same coordinate space as | ||
| /// `bounds`, kept fresh by `ShimejiOverlayController`'s periodic poll of | ||
| /// the live `UITabBar`. Nil when there's no tab bar to find. | ||
| var navBarY: CGFloat? |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle bounds and navBarY changes as floor changes.
Unlike miniPlayerY and isNowPlayingOpen, these floor inputs do not invoke handleFloorTargetChange(). When layout, rotation, or the tab-bar poll changes them, existing sprites can remain at the old floor or miss the intended hop/fall transition. Add guarded property observers or notify the engine from the overlay controller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Services/Shimeji/ShimejiEngine.swift` around lines 77 - 82,
Update the ShimejiEngine properties bounds and navBarY so changes invoke
handleFloorTargetChange(), using guarded observers to avoid unnecessary handling
during initialization or unchanged values. Ensure layout, rotation, and tab-bar
updates recalculate existing sprites’ floor transitions while preserving the
existing miniPlayerY and isNowPlayingOpen behavior.
Comment on lines
+151
to
+157
| func start(manifest: ShimejiManifest) { | ||
| guard displayLink == nil else { return } | ||
| respawn(manifest: manifest) | ||
| let link = CADisplayLink(target: self, selector: #selector(tick)) | ||
| link.add(to: .main, forMode: .common) | ||
| displayLink = link | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'ShimejiEngine\.swift$' . || true
echo "== outline =="
file="$(fd 'ShimejiEngine\.swift$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
ast-grep outline "$file" || true
echo "== relevant lines 120-190 =="
sed -n '120,190p' "$file" | cat -n -v
echo "== respawn/start references =="
rg -n "func start|var displayLink|func respawn|bounds|instances|active" "$file"
fiRepository: Evil-Project/Twinskaraoke
Length of output: 5934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ShimejiEngine.swift path =="
for f in $(fd 'ShimejiEngine\.swift$' .); do
echo "--- $f"
wc -l "$f"
done
file="$(fd 'ShimejiEngine\.swift$' . | head -n1 || true)"
if [ -z "${file:-}" ]; then
echo "No ShimejiEnginge.swift found"
exit 0
fi
echo "== line 1-230 =="
sed -n '1,230p' "$file" | cat -n
echo "== broader references to engine APIs =="
rg -n "ShimejiEngine|start\\(|respawn\\(|bounds|displayLink|instances" -S .Repository: Evil-Project/Twinskaraoke
Length of output: 24321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ShimejiSessionModifier.swift =="
sed -n '1,90p' Twinskaraoke/Components/Shimeji/ShimejiSessionModifier.swift | cat -n
echo "== ShimejiOverlayWindow.swift =="
sed -n '1,140p' Twinskaraoke/Components/Shimeji/ShimejiOverlayWindow.swift | cat -n
echo "== ShimejiManifest references =="
rg -n "struct ShimejiManifest|class ShimejiManifest|typealias ShimejiManifest|type ShimejiManifest|active manifest|manifest" Twinskaraoke/Services/Shimeji Twinskaraoke/Components/Shimeji Twinskaraoke/Features/Account/ShimejiSettingsView.swift Twinskaraoke/Components/Shimeji/ShimejiFloorRegistry.swift -SRepository: Evil-Project/Twinskaraoke
Length of output: 14607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Twinskaraoke/Services/Shimeji/ShimejiEngine.swift"
python3 - <<'PY'
from pathlib import Path
import re
p = Path("Twinskaraoke/Services/Shimeji/ShimejiEngine.swift")
s = p.read_text()
print("bounds_init", bool(re.search(r'var bounds:\s*CGRect\s*=\s*\.zero\Z', s, flags=re.M)))
print("has_active_manifest_state", bool(re.search(r'private\s+var\s+activeManifest|`@Published`\s+.*manifest|manifest\s*:', s)))
calls = [(m.group(0).strip(), m.start()) for m in re.finditer(r'func\s+start\s*\([^)]*\)\s*\{', s)]
respawns = [(m.group(0).strip(), m.start()) for m in re.finditer(r'func\s+respawn\s*\([^)]*\)\s*\{', s)]
print("start_defines", calls)
print("respawn_defines", respawns)
start_re = r'func\s+start\s*\([^)]*\)\s*\{.*?\n \}'
for m in re.finditer(start_re, s, flags=re.S):
block = m.group(0)
print("start_uses_respawn_before_link", "respawn(" in block and "CADisplayLink" in block and block.find("respawn(") < block.find("CADisplayLink"))
print("start_after_guard_early_return", "guard displayLink == nil else { return }" in s)
print("respawn_clears_on_empty_bounds", "guard !pool.isEmpty, bounds.width > 0" in s and "instances = []" in re.search(r'func\s+respawn\s*\(.*?\)\s*\{.*?\n \}', s, flags=re.S).group(0))
PY
echo "== deterministic path probe from source state (read-only structural check) =="
if python3 - <<'PY'
from pathlib import Path
import re
s = Path("Twinskaraoke/Services/Shimeji/ShimejiEngine.swift").read_text()
start = re.search(r'func\s+start\s*\([^)]*\)\s*\{.*?\n \}', s, flags=re.S)
respawn = re.search(r'func\s+respawn\s*\([^)]*\)\s*\{.*?\n \}', s, flags=re.S)
assert start and respawn and "guard displayLink == nil else { return }" in start.group(0)
assert "respawn(" in start.group(0)
assert "CADisplayLink" in start.group(0)
assert start.group(0).find("respawn(") < start.group(0).find("CADisplayLink")
assert "guard !pool.isEmpty, bounds.width > 0" in respawn.group(0)
assert "instances = []" in respawn.group(0)
print("TRUE")
PY
then
:
else
exit 1
fiRepository: Evil-Project/Twinskaraoke
Length of output: 539
Defer spawning until Shimeji bounds are non-zero.
ShimejiSessionModifier can call ShimejiEngine.shared.start(manifest:) before ShimejiOverlayController assigns non-zero bounds, so respawn() sets instances = [] and installs the display link. Later calls exit at the displayLink guard, and existing resource-bound respawn paths do not restore the active session. Retain the active manifest and respawn it when bounds become valid, or defer link creation until bounds are usable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Services/Shimeji/ShimejiEngine.swift` around lines 151 - 157,
The start(manifest:) flow must not call respawn or create the display link while
Shimeji bounds are zero. Retain the active manifest and defer spawning/link
creation until the overlay bounds become non-zero, then respawn that manifest;
ensure later bounds updates are not blocked by the displayLink guard.
Comment on lines
+165
to
+172
| func deleteDownloadedPack() { | ||
| downloadTask?.cancel() | ||
| downloadTask = nil | ||
| progressObservation = nil | ||
| try? FileManager.default.removeItem(at: packDirectory) | ||
| manifest = nil | ||
| state = .notDownloaded | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Frame images are cached by path and never invalidated when the pack changes. Because every pack extracts to the same ShimejiPack/Characters/<folder>/<frame> paths, removing or replacing the pack leaves stale UIImages served from memory for the process lifetime.
Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift#L165-L172: purge the sprite image cache indeleteDownloadedPack()(and after a successful re-extraction inextractAndLoad).Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift#L8-L19: expose aremoveAll()method onShimejiImageCachewrappingcache.removeAllObjects(), and give theNSCacheexplicitcountLimit/totalCostLimitvalues.
📍 Affects 2 files
Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift#L165-L172(this comment)Twinskaraoke/Components/Shimeji/ShimejiSpriteView.swift#L8-L19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Services/Shimeji/ShimejiResourceManager.swift` around lines 165
- 172, Invalidate cached sprite images when downloaded packs are removed or
re-extracted. In ShimejiResourceManager.deleteDownloadedPack and after
successful extraction in extractAndLoad, call ShimejiImageCache.removeAll(); in
ShimejiSpriteView’s ShimejiImageCache, add removeAll() wrapping
cache.removeAllObjects() and configure the NSCache with explicit countLimit and
totalCostLimit values.
Comment on lines
+26
to
+34
| for entry in entries where !entry.isDirectory { | ||
| let destination = destinationDirectory.appendingPathComponent(entry.path) | ||
| try fm.createDirectory( | ||
| at: destination.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true | ||
| ) | ||
| let fileData = try extractFileData(data, entry: entry) | ||
| try fileData.write(to: destination, options: .atomic) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Zip Slip: entry paths are written without containment checks.
entry.path comes straight from the archive's central directory and is joined to destinationDirectory unvalidated. An entry named ../../Library/Preferences/foo.plist escapes the pack directory and overwrites arbitrary files in the app container. Since the archive is fetched over the network (and, per the PR description, currently from a personal domain), this is remotely triggerable.
Reject absolute paths, any .. component, and verify the resolved destination stays under the destination root.
🔒️ Proposed containment check
let fm = FileManager.default
+ let root = destinationDirectory.standardizedFileURL
for entry in entries where !entry.isDirectory {
- let destination = destinationDirectory.appendingPathComponent(entry.path)
+ guard !entry.path.hasPrefix("/"),
+ !entry.path.split(separator: "/").contains("..")
+ else { throw ZipError.corruptArchive }
+ let destination = root.appendingPathComponent(entry.path).standardizedFileURL
+ guard destination.path.hasPrefix(root.path + "/") else {
+ throw ZipError.corruptArchive
+ }
try fm.createDirectory(
at: destination.deletingLastPathComponent(),
withIntermediateDirectories: true
)📝 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.
Suggested change
| for entry in entries where !entry.isDirectory { | |
| let destination = destinationDirectory.appendingPathComponent(entry.path) | |
| try fm.createDirectory( | |
| at: destination.deletingLastPathComponent(), | |
| withIntermediateDirectories: true | |
| ) | |
| let fileData = try extractFileData(data, entry: entry) | |
| try fileData.write(to: destination, options: .atomic) | |
| } | |
| let fm = FileManager.default | |
| let root = destinationDirectory.standardizedFileURL | |
| for entry in entries where !entry.isDirectory { | |
| guard !entry.path.hasPrefix("/"), | |
| !entry.path.split(separator: "/").contains("..") | |
| else { throw ZipError.corruptArchive } | |
| let destination = root.appendingPathComponent(entry.path).standardizedFileURL | |
| guard destination.path.hasPrefix(root.path + "/") else { | |
| throw ZipError.corruptArchive | |
| } | |
| try fm.createDirectory( | |
| at: destination.deletingLastPathComponent(), | |
| withIntermediateDirectories: true | |
| ) | |
| let fileData = try extractFileData(data, entry: entry) | |
| try fileData.write(to: destination, options: .atomic) | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 26-26: An archive entry's path/name (e.g. ZIPFoundation Entry.path) is joined to a destination directory with appendingPathComponent / appending(path:) without a containment check. A malicious archive entry such as ../../etc/passwd can escape the extraction directory and overwrite arbitrary files (Zip Slip / path traversal). Before writing, canonicalize the joined URL and verify it is still inside the destination root (e.g. compare standardizedFileURL.path against the destination's path with a trailing separator, and reject entries that resolve outside it).
Context: destinationDirectory.appendingPathComponent(entry.path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(zip-slip-entry-path-append-swift)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Services/Shimeji/ShimejiZipReader.swift` around lines 26 - 34,
Update the archive extraction loop in ShimejiZipReader to validate each
entry.path before creating directories or writing files: reject absolute paths
and any path containing a ".." component, then resolve the destination and
verify it remains contained under destinationDirectory. Only perform
createDirectory, extractFileData, and write after this validation succeeds.
Source: Linters/SAST tools
Comment on lines
+58
to
+61
| Cloud(proxy: proxy, color: Theme.ellipsesTopLeading(forScheme: scheme), rotationStart: 0, duration: 60, alignment: .topLeading) | ||
| Cloud(proxy: proxy, color: Theme.ellipsesBottomTrailing(forScheme: scheme), rotationStart: 90, duration: 90, alignment: .bottomTrailing) | ||
| Cloud(proxy: proxy, color: Theme.ellipsesTopTrailing(forScheme: scheme), rotationStart: 180, duration: 75, alignment: .topTrailing) | ||
| Cloud(proxy: proxy, color: Theme.ellipsesBottomLeading(forScheme: scheme), rotationStart: 270, duration: 105, alignment: .bottomLeading) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching AuroraBackgroundViews.swift:"
fd -a 'AuroraBackgroundViews\.swift$' . || true
file=$(fd 'AuroraBackgroundViews\.swift$' . | head -n 1 || true)
if [ -n "${file:-}" ]; then
echo
echo "Line count:"
wc -l "$file"
echo
echo "Relevant source:"
cat -n "$file"
fi
echo
echo "Search for FloatingClouds and ellipses helpers:"
rg -n "FloatingClouds|ellipses(Bottom|Top)|(rotationStart|duration|align)" .Repository: Evil-Project/Twinskaraoke
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Theme.swift ellipses helpers:"
sed -n '1,55p' Twinskaraoke/Theme/Theme.swift | cat -n
echo
echo "AppAppearance/ColorScheme helpers:"
rg -n "struct AppearanceMode|enum AppearanceMode|let dark|case dark|lightMode|colorScheme|\.appBackground|Color\.appBackground" Twinskaraoke TwinskaraokeTVApp -g '*.swift' | head -n 200Repository: Evil-Project/Twinskaraoke
Length of output: 5567
Use the active color scheme instead of hard-coding .dark.
FloatingClouds.scheme is always .dark, so the ambient aurora backgrounds choose dark cloud colors even in light mode. Fetch @Environment(\.colorScheme) in FloatingClouds and pass it into the theme helpers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Twinskaraoke/Theme/AuroraBackgroundViews.swift` around lines 58 - 61, Update
FloatingClouds to read the active `@Environment`(\.colorScheme) value and pass it
as the scheme argument to the Theme.ellipsesTopLeading, ellipsesBottomTrailing,
ellipsesTopTrailing, and ellipsesBottomLeading helpers instead of using the
always-dark FloatingClouds.scheme.
Comment on lines
+166
to
+177
| "%@ ready — %lld characters" : { | ||
| "comment" : "A label that displays the name of the resource pack and a checkmark.", | ||
| "isCommentAutoGenerated" : true, | ||
| "localizations" : { | ||
| "en" : { | ||
| "stringUnit" : { | ||
| "state" : "new", | ||
| "value" : "%1$@ ready — %2$lld characters" | ||
| } | ||
| } | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No plural variation for the character count.
"%1$@ ready — %2$lld characters" renders "1 characters" for a single-character pack, and languages with richer plural rules can't be translated correctly from a single form. Add a variations.plural block for the %lld argument (or use AttributedString/inflect-style automatic grammar agreement).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TwinskaraokeShared/Localization/Localizable.xcstrings` around lines 166 -
177, Update the localization entry for "%@ ready — %lld characters" to support
plural variation on the %lld argument, including singular and plural forms while
preserving the resource-pack name placeholder. Ensure the English localization
renders “1 character” and uses the plural form for other counts, and structure
the entry so languages with richer plural rules can provide appropriate
translations.
Collaborator
Author
|
It’ll be fine, right? I mean it’s experimental… |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 8 file(s) based on 13 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
This was referenced Jul 30, 2026
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.
Added experimental Shimejees, uses a temp downlaod link on my domain until out of testing, idk why it cant merge
Summary by Sourcery
Introduce an experimental, remotely updatable Shimeji system that downloads a character pack, renders draggable animated sprites in an overlay window across the app, and exposes user controls for enabling, configuring, and spawning characters.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Documentation