Fix Tiger Mom buff delay and UpgradeOverlay refactor#134
Conversation
- Fixed a bug in `MainViewModel.kt` where enemy buffs were cleaned up using a stale lookup map, causing a delay in removing buffs when a source enemy (like Tiger Mom) died. - Deduplicated `availableStats` in `UpgradeOverlay.kt` by using `StallUpgradeManager.getAvailableUpgradeStats(stall)`. - Removed a redundant null check in `MainViewModel.onCellClick` to eliminate a compiler warning. - Updated `fixes.md` with FIX-004. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR addresses three related bugs and a refactoring: projectile impact buff cleanup now filters against surviving enemies to prevent stale buff relationships; upgrade stat selection is extracted from UpgradeOverlay to StallUpgradeManager; a redundant null check is removed from stall placement logic. ChangesBug Fixes and Upgrade Stats Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/main/java/com/messark/hawker/ui/components/UpgradeOverlay.kt (3)
70-70: ⚡ Quick winReplace hardcoded colors with MaterialTheme.colorScheme.
The hardcoded green color
Color(0xFF4CAF50)violates the coding guideline to avoid hardcoding colors in UI components. Use theme colors to support light and dark themes consistently.🎨 Proposed fix to use theme colors
For the free upgrade message (line 70):
Text( text = "Free specific upgrade active ($freeUpgradeCount remaining)! This upgrade will cost $0 and will not cause a shutdown.", - color = Color(0xFF4CAF50), + color = MaterialTheme.colorScheme.primary, fontSize = 12.sp,For the button background (line 144):
Surface( - color = if (enabled) Color(0xFF4CAF50) else MaterialTheme.colorScheme.surfaceVariant, + color = if (enabled) MaterialTheme.colorScheme.primary else MaterialTheme.colorScheme.surfaceVariant, shape = RoundedCornerShape(8.dp),As per coding guidelines, avoid hardcoding colors in UI components; always prefer MaterialTheme.colorScheme to support light and dark themes.
Also applies to: 144-144
155-156: ⚡ Quick winReplace hardcoded Color.White with theme colors.
Hardcoded
Color.Whiteshould be replaced withMaterialTheme.colorScheme.onPrimaryto ensure proper contrast and theme support.🎨 Proposed fix
Row( modifier = Modifier.padding(12.dp), horizontalArrangement = Arrangement.SpaceBetween, verticalAlignment = Alignment.CenterVertically ) { - Text(text = label, color = Color.White, fontWeight = FontWeight.Bold) - Text(text = "$$cost", color = Color.White, fontWeight = FontWeight.Bold) + Text(text = label, color = MaterialTheme.colorScheme.onPrimary, fontWeight = FontWeight.Bold) + Text(text = "$$cost", color = MaterialTheme.colorScheme.onPrimary, fontWeight = FontWeight.Bold) }As per coding guidelines, avoid hardcoding colors in UI components; always prefer MaterialTheme.colorScheme to support light and dark themes.
137-159: ⚖️ Poor tradeoffConsider using SpriteButton for consistency.
The coding guidelines specify using
SpriteButtonfor all action buttons and referencingbuttons.pngfor visual consistency. The customUpgradeButtoncomposable may lead to inconsistent styling across the app.As per coding guidelines, use SpriteButton for all action buttons and reference buttons.png for visual consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9be0297e-b659-47d9-9526-8988256dd0b4
📒 Files selected for processing (3)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/ui/components/UpgradeOverlay.ktfixes.md
This PR addresses several minor issues:
UpgradeOverlay.ktto useStallUpgradeManagerfor retrieving available upgrade categories, ensuring the UI is always in sync with the game logic and reducing duplication.tile.stall == nullcheck inMainViewModel.ktthat was triggering a 'Condition is always true' warning.fixes.md.PR created automatically by Jules for task 944813144424530300 started by @candour
Summary by CodeRabbit
Release Notes
Bug Fixes