🧹 Refactor CrowdManager by extracting CrowdTextureManager#1364
Merged
Conversation
Extracted asset loading and texture storage into a dedicated CrowdTextureManager class to adhere to the Single Responsibility Principle and resolve the "overly long function/component" code smell in CrowdManager. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Contributor
|
👋 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. |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTexture management for crowd sprites is refactored from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
📝 PR Kommentar-ZusammenfassungWird automatisch aktualisiert, sobald sich Kommentare ändern. |
Lint Fix PreviewTarget roots:
Patch preview(Showing last 4000 lines if larger.) diff --git a/src/components/overworld/OverworldMap.tsx b/src/components/overworld/OverworldMap.tsx
index 58a87f0f..c0750543 100644
--- a/src/components/overworld/OverworldMap.tsx
+++ b/src/components/overworld/OverworldMap.tsx
@@ -23,7 +23,7 @@ export const OverworldMap = React.memo(
travelCompletedRef,
onTravelComplete,
activeStoryFlags
-}: OverworldMapProps) => {
+ }: OverworldMapProps) => {
// Memoized URL generators
const mapBgUrl = useMemo(
() => getGenImageUrl(IMG_PROMPTS.OVERWORLD_MAP),
@@ -83,41 +83,46 @@ export const OverworldMap = React.memo(
const renderedNodes = useMemo(() => {
if (!gameMap) return null
const nodes = gameMap.nodes
- return Object.values(nodes as Record<string, unknown>).map((node: unknown) => {
- const isCurrent = node.id === player.currentNodeId
- const visibility = getNodeVisibility(node.layer, currentLayer)
- const isReachable = isConnected(node.id) || node.type === 'START'
+ return Object.values(nodes as Record<string, unknown>).map(
+ (node: unknown) => {
+ const isCurrent = node.id === player.currentNodeId
+ const visibility = getNodeVisibility(node.layer, currentLayer)
+ const isReachable = isConnected(node.id) || node.type === 'START'
- let iconUrl = pinClubUrl
- if (node.type === 'FESTIVAL') iconUrl = pinFestivalUrl
- else if (node.type === 'START') iconUrl = pinHomeUrl
- else if (node.type === 'REST_STOP') iconUrl = pinRestUrl
- else if (node.type === 'SPECIAL') iconUrl = pinSpecialUrl
- else if (node.type === 'FINALE') iconUrl = pinFinaleUrl
+ let iconUrl = pinClubUrl
+ if (node.type === 'FESTIVAL') iconUrl = pinFestivalUrl
+ else if (node.type === 'START') iconUrl = pinHomeUrl
+ else if (node.type === 'REST_STOP') iconUrl = pinRestUrl
+ else if (node.type === 'SPECIAL') iconUrl = pinSpecialUrl
+ else if (node.type === 'FINALE') iconUrl = pinFinaleUrl
- const effectivePrice = calculateEffectiveTicketPrice(node.venue || {}, {
- discountedTickets: activeStoryFlags?.includes(
- 'discounted_tickets_active'
+ const effectivePrice = calculateEffectiveTicketPrice(
+ node.venue || {},
+ {
+ discountedTickets: activeStoryFlags?.includes(
+ 'discounted_tickets_active'
+ )
+ }
)
- })
- return (
- <MapNode
- key={node.id}
- node={node}
- isCurrent={isCurrent}
- isTraveling={isTraveling}
- visibility={visibility}
- isReachable={isReachable}
- isPendingConfirm={pendingTravelNode?.id === node.id}
- handleTravel={handleTravel}
- setHoveredNode={setHoveredNode}
- iconUrl={iconUrl}
- vanUrl={vanUrl}
- ticketPrice={effectivePrice}
- />
+ return (
+ <MapNode
+ key={node.id}
+ node={node}
+ isCurrent={isCurrent}
+ isTraveling={isTraveling}
+ visibility={visibility}
+ isReachable={isReachable}
+ isPendingConfirm={pendingTravelNode?.id === node.id}
+ handleTravel={handleTravel}
+ setHoveredNode={setHoveredNode}
+ iconUrl={iconUrl}
+ vanUrl={vanUrl}
+ ticketPrice={effectivePrice}
+ />
+ )
+ }
)
- })
}, [
gameMap,
player.currentNodeId,
diff --git a/src/components/stage/CrowdTextureManager.ts b/src/components/stage/CrowdTextureManager.ts
index f2a5b0cf..97d104e0 100644
--- a/src/components/stage/CrowdTextureManager.ts
+++ b/src/components/stage/CrowdTextureManager.ts
@@ -40,14 +40,22 @@ export class CrowdTextureManager {
}
getTargetTexture(shouldMosh: boolean): Texture | null {
- return shouldMosh && this.textures.mosh ? this.textures.mosh : this.textures.idle
+ return shouldMosh && this.textures.mosh
+ ? this.textures.mosh
+ : this.textures.idle
}
dispose(): void {
- if (this.textures.idle && typeof this.textures.idle.destroy === 'function') {
+ if (
+ this.textures.idle &&
+ typeof this.textures.idle.destroy === 'function'
+ ) {
this.textures.idle.destroy(true)
}
- if (this.textures.mosh && typeof this.textures.mosh.destroy === 'function') {
+ if (
+ this.textures.mosh &&
+ typeof this.textures.mosh.destroy === 'function'
+ ) {
this.textures.mosh.destroy(true)
}
this.textures = { idle: null, mosh: null }
diff --git a/src/hooks/usePreGigLogic.ts b/src/hooks/usePreGigLogic.ts
index fff7c887..be4ae62e 100644
--- a/src/hooks/usePreGigLogic.ts
+++ b/src/hooks/usePreGigLogic.ts
@@ -178,15 +178,18 @@ export const usePreGigLogic = (): PreGigLogicReturn => {
}
}, [activeEvent, isScreenshotMode, triggerEvent])
- const toggleSong = useCallback(song => {
- if (selectedSongIds.has(song.id)) {
- setSetlist(setlist.filter(s => getSongId(s) !== song.id))
- } else {
- if (setlist.length < 3) {
- setSetlist([...setlist, { id: song.id }])
+ const toggleSong = useCallback(
+ song => {
+ if (selectedSongIds.has(song.id)) {
+ setSetlist(setlist.filter(s => getSongId(s) !== song.id))
+ } else {
+ if (setlist.length < 3) {
+ setSetlist([...setlist, { id: song.id }])
+ }
}
- }
- }, [selectedSongIds, setSetlist, setlist])
+ },
+ [selectedSongIds, setSetlist, setlist]
+ )
const calculatedBudget = useMemo(() => {
let acc = 0
diff --git a/src/scenes/kabelsalat/hooks/useKabelsalatInteractions.ts b/src/scenes/kabelsalat/hooks/useKabelsalatInteractions.ts
index 605cd77b..a4dbecfe 100644
--- a/src/scenes/kabelsalat/hooks/useKabelsalatInteractions.ts
+++ b/src/scenes/kabelsalat/hooks/useKabelsalatInteractions.ts
@@ -1,4 +1,11 @@
-import { useEffect, useCallback, useRef, type MutableRefObject, type Dispatch, type SetStateAction } from 'react'
+import {
+ useEffect,
+ useCallback,
+ useRef,
+ type MutableRefObject,
+ type Dispatch,
+ type SetStateAction
+} from 'react'
import { SOCKET_DEFS, CABLE_MAP } from '../constants'
export const useKabelsalatInteractions = (
@@ -23,28 +30,33 @@ export const useKabelsalatInteractions = (
}
}, [])
- const triggerShock = useCallback((reason: string) => {
- setIsShocked(true)
- setFaultReason(reason)
- setSelectedCable(null)
- setConnections({})
+ const triggerShock = useCallback(
+ (reason: string) => {
+ setIsShocked(true)
+ setFaultReason(reason)
+ setSelectedCable(null)
+ setConnections({})
- if (shockTimeoutRef.current) {
- clearTimeout(shockTimeoutRef.current)
- }
+ if (shockTimeoutRef.current) {
+ clearTimeout(shockTimeoutRef.current)
+ }
- shockTimeoutRef.current = setTimeout(() => {
- setIsShocked(false)
- setFaultReason('')
- shockTimeoutRef.current = null
- }, 1200)
- }, [setIsShocked, setFaultReason, setSelectedCable, setConnections])
+ shockTimeoutRef.current = setTimeout(() => {
+ setIsShocked(false)
+ setFaultReason('')
+ shockTimeoutRef.current = null
+ }, 1200)
+ },
+ [setIsShocked, setFaultReason, setSelectedCable, setConnections]
+ )
const handleCableClick = useCallback(
(cableId: string) => {
if (isShocked || isPoweredOn || isGameOver || isWinningRef.current) return
- const connectionSocketId = Object.keys(connections).find(key => connections[key] === cableId)
+ const connectionSocketId = Object.keys(connections).find(
+ key => connections[key] === cableId
+ )
if (connectionSocketId) {
setConnections(prev => {
@@ -56,7 +68,15 @@ export const useKabelsalatInteractions = (
}
setSelectedCable(prev => (prev === cableId ? null : cableId))
},
- [isShocked, isPoweredOn, isGameOver, connections, isWinningRef, setConnections, setSelectedCable]
+ [
+ isShocked,
+ isPoweredOn,
+ isGameOver,
+ connections,
+ isWinningRef,
+ setConnections,
+ setSelectedCable
+ ]
)
const handleSocketClick = useCallback(
diff --git a/src/scenes/kabelsalat/hooks/useKabelsalatShuffle.ts b/src/scenes/kabelsalat/hooks/useKabelsalatShuffle.ts
index 218804c2..8d278d2a 100644
--- a/src/scenes/kabelsalat/hooks/useKabelsalatShuffle.ts
+++ b/src/scenes/kabelsalat/hooks/useKabelsalatShuffle.ts
@@ -1,4 +1,10 @@
-import { useEffect, type MutableRefObject, type Dispatch, type SetStateAction, useMemo } from 'react'
+import {
+ useEffect,
+ type MutableRefObject,
+ type Dispatch,
+ type SetStateAction,
+ useMemo
+} from 'react'
import { INITIAL_SOCKET_ORDER } from '../constants'
import { getSafeRandom } from '../../../utils/crypto'
@@ -52,5 +58,13 @@ export const useKabelsalatShuffle = (
}, 2000)
return () => clearInterval(interval)
- }, [isPoweredOn, isGameOver, isShocked, unconnectedIds, connections, isWinningRef, setSocketOrder])
+ }, [
+ isPoweredOn,
+ isGameOver,
+ isShocked,
+ unconnectedIds,
+ connections,
+ isWinningRef,
+ setSocketOrder
+ ])
}
diff --git a/src/scenes/kabelsalat/hooks/useKabelsalatTimer.ts b/src/scenes/kabelsalat/hooks/useKabelsalatTimer.ts
index b3fec8b9..ab36e6c3 100644
--- a/src/scenes/kabelsalat/hooks/useKabelsalatTimer.ts
+++ b/src/scenes/kabelsalat/hooks/useKabelsalatTimer.ts
@@ -1,4 +1,10 @@
-import { useEffect, useRef, type MutableRefObject, type Dispatch, type SetStateAction } from 'react'
+import {
+ useEffect,
+ useRef,
+ type MutableRefObject,
+ type Dispatch,
+ type SetStateAction
+} from 'react'
import { SOCKET_DEFS } from '../constants'
export const useKabelsalatTimer = (Duplicate codeNo significant duplicates found (per jscpd thresholds). |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac3ad5040
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| crowdMembers: CrowdMember[] | ||
| container: Container | null | ||
| textures: CrowdTextures | ||
| textureManager: CrowdTextureManager |
There was a problem hiding this comment.
This refactor replaces the textures field with textureManager but does not keep a compatibility alias, so any existing consumer that reads or writes crowdManager.textures.* now gets undefined and throws (e.g., setting crowdManager.textures.idle). In this repo that regression is immediately observable in the node CrowdManager tests, and it will also affect any runtime code using the old field contract; please either migrate all callers in the same change or expose textures as a passthrough accessor.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors texture management by extracting logic from CrowdManager into a new CrowdTextureManager class, which handles asset loading, retrieval, and disposal. The review feedback suggests several improvements: marking the textureManager property as readonly for better safety, using the getTargetTexture method consistently instead of direct property access, and improving encapsulation by making the textures property private within the manager class.
| crowdMembers: CrowdMember[] | ||
| container: Container | null | ||
| textures: CrowdTextures | ||
| textureManager: CrowdTextureManager |
Contributor
There was a problem hiding this comment.
The textureManager property is initialized in the constructor and is not intended to be reassigned during the lifecycle of the CrowdManager. Marking it as readonly improves code safety and clarity.
Suggested change
| textureManager: CrowdTextureManager | |
| readonly textureManager: CrowdTextureManager |
| _createCrowdMember(radius: number, fallbackColor: number): CrowdMember { | ||
| if (this.textures.idle) { | ||
| const crowd = new Sprite(this.textures.idle) as CrowdSpriteMember | ||
| const idleTexture = this.textureManager.textures.idle |
Contributor
There was a problem hiding this comment.
For better consistency and encapsulation, consider using the getTargetTexture method with false instead of accessing the textures property directly. This ensures that the logic for retrieving the base texture is centralized within the CrowdTextureManager.
Suggested change
| const idleTexture = this.textureManager.textures.idle | |
| const idleTexture = this.textureManager.getTargetTexture(false) |
| } | ||
|
|
||
| export class CrowdTextureManager { | ||
| textures: CrowdTextures |
Contributor
There was a problem hiding this comment.
The textures property is currently public, which allows external components to modify the manager's internal state directly. Consider making it private (e.g., _textures) and providing a public getter if needed, or relying solely on methods like getTargetTexture to access the textures. This improves encapsulation and prevents accidental side effects from other components.
❌ Node.js tests failedShowing relevant failure lines (full log is attached to the workflow run): See the CI logs for full output. |
Addressed CI review feedback: 1. Marked PIXI Application and Texture as type-only imports where appropriate. 2. Added `silent: true` to `handleError` during texture loading since there is a fallback graphic. 3. Added `destroy(true)` calls in `CrowdTextureManager.dispose()` to prevent GPU memory leaks. 4. Added backwards compatibility getter `textures` on `CrowdManager` to fix test regressions and preserve the public interface. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
| idle: Texture | null | ||
| mosh: Texture | null | ||
| } | ||
| import { CrowdTextureManager, CrowdTextures } from './CrowdTextureManager' |
Contributor
There was a problem hiding this comment.
🟡 Type-only import CrowdTextures missing type qualifier violates AGENTS.md isolatedModules rule
CrowdTextures is exported as export type in src/components/stage/CrowdTextureManager.ts:6, but it is imported as a regular value import in CrowdManager.ts:9: import { CrowdTextureManager, CrowdTextures }. Per AGENTS.md: "isolatedModules: true — type-only imports must use import type (mixed form: import { Foo, type Bar })." The correct form is import { CrowdTextureManager, type CrowdTextures }. Note the new CrowdTextureManager.ts itself correctly uses import type { Texture } on line 1, making this inconsistency within the same PR.
Suggested change
| import { CrowdTextureManager, CrowdTextures } from './CrowdTextureManager' | |
| import { CrowdTextureManager, type CrowdTextures } from './CrowdTextureManager' |
Was this helpful? React with 👍 or 👎 to provide feedback.
This was referenced Apr 25, 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.
🎯 What: Extracted the asset loading and texture storage logic from
CrowdManager.tsinto a new, dedicatedCrowdTextureManager.tsclass.💡 Why: To improve maintainability, reduce the file size of
CrowdManager, and adhere to the Single Responsibility Principle, fixing the "overly long function/component" code health issue. This matches the existing pattern seen inEffectTextureManager.✅ Verification: Verified by running both logic (
npm run test:vitest:logic) and UI (npm run test:vitest:ui) test suites, as well asnpm run lint. Checked that the extracted texture functionality and fallback behavior work identically.✨ Result: Cleaned up
CrowdManager, significantly reduced its size and responsibility, making the code much easier to read and maintain.PR created automatically by Jules for task 875482965929525904 started by @DaFum