Stop StoneHandler.getFoilStones() from mutating shared level data - #2009
Conversation
getFoilStones() rewrote currentPuzzleData.foilStones in place, but that array is a reference into the shared level data. Because it also caps the pool to eight stones, repeated calls permanently dropped foil stones and reshuffled the set; the puzzle_completed analytics event compounded this by reading the foils more than once per puzzle. Build the result on a copy so the level data is never mutated, and read the foils once when logging.
📝 WalkthroughWalkthrough
ChangesFoil stone integrity
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/stone-handler/stone-handler.spec.ts (1)
157-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the returned collection as well as source immutability.
This test only checks that
levelData.puzzles[0].foilStonesremains unchanged. It passes even ifgetFoilStones()stops enforcing the eight-stone limit.Store the result and assert that it contains eight stones and retains the target stone.
As per coding guidelines, “Relevant automated tests are added or updated for the changes.”
Proposed assertions
- handler.getFoilStones(); + const result = handler.getFoilStones(); + expect(result).toHaveLength(8); + expect(result).toEqual(expect.arrayContaining(['A'])); expect(levelData.puzzles[0].foilStones).toEqual(originalFoilStones);🤖 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 `@src/components/stone-handler/stone-handler.spec.ts` around lines 157 - 166, Update the test around StoneHandler.getFoilStones to store its returned collection, assert that it contains exactly eight stones, and verify the target stone is retained, while preserving the existing assertion that levelData.puzzles[0].foilStones remains unchanged.Source: Coding guidelines
src/scenes/gameplay-scene/gameplay-flow-manager.ts (1)
495-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the cached analytics value.
The added tests do not execute
GameplayFlowManager.logPuzzleEndFirebaseEvent. A regression could callgetFoilStones()more than once again while the current tests still pass.Add a focused test that returns a known foil array, invokes the completion logger, verifies one
getFoilStones()call, and checks thatfoilsequalsfoilStones.join(',').As per coding guidelines, “Relevant automated tests are added or updated for the changes.”
Also applies to: 507-507
🤖 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 `@src/scenes/gameplay-scene/gameplay-flow-manager.ts` at line 495, Add a focused regression test for GameplayFlowManager.logPuzzleEndFirebaseEvent that mocks getFoilStones() to return a known array, invokes the completion logger, verifies getFoilStones() is called exactly once, and asserts the emitted foils value equals foilStones.join(',').Source: Coding guidelines
src/components/stone-handler/stone-handler.ts (1)
255-278: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse
shuffleArrayfor the final shuffle.
StoneHandler.shuffleArrayalready provides a Fisher-Yates shuffle and returns a copy.sort(() => Math.random() - 0.5)produces biased permutations and duplicates the shuffle implementation.Proposed fix
- return foilStones.sort(() => Math.random() - 0.5); + return this.shuffleArray(foilStones);🤖 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 `@src/components/stone-handler/stone-handler.ts` around lines 255 - 278, Replace the biased sort-based shuffle in the method building the final foil-stone list with the existing StoneHandler.shuffleArray helper, preserving the returned shuffled copy and all preceding stone-selection logic.
🤖 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.
Nitpick comments:
In `@src/components/stone-handler/stone-handler.spec.ts`:
- Around line 157-166: Update the test around StoneHandler.getFoilStones to
store its returned collection, assert that it contains exactly eight stones, and
verify the target stone is retained, while preserving the existing assertion
that levelData.puzzles[0].foilStones remains unchanged.
In `@src/components/stone-handler/stone-handler.ts`:
- Around line 255-278: Replace the biased sort-based shuffle in the method
building the final foil-stone list with the existing StoneHandler.shuffleArray
helper, preserving the returned shuffled copy and all preceding stone-selection
logic.
In `@src/scenes/gameplay-scene/gameplay-flow-manager.ts`:
- Line 495: Add a focused regression test for
GameplayFlowManager.logPuzzleEndFirebaseEvent that mocks getFoilStones() to
return a known array, invokes the completion logger, verifies getFoilStones() is
called exactly once, and asserts the emitted foils value equals
foilStones.join(',').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99cfa997-cc08-4f51-a5c5-dbe51f209a5d
📒 Files selected for processing (3)
src/components/stone-handler/stone-handler.spec.tssrc/components/stone-handler/stone-handler.tssrc/scenes/gameplay-scene/gameplay-flow-manager.ts
1 participant
This PR proposes a fix that stops
StoneHandler.getFoilStones()from mutating the shared level data it reads. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/230. You can sign in with your GitHub ID to claim ownership of the project.What this fixes
getFoilStones()insrc/components/stone-handler/stone-handler.tsassembles the stones to display by editingthis.currentPuzzleData.foilStonesin place — it removes the target stones, caps the pool to eight by splicing foil stones out, pushes the targets back, then shuffles. The catch is thatcurrentPuzzleDatais a live reference intolevelData.puzzles[...], so each call permanently rewrites your level data instead of computing a fresh list. That makes the method non-idempotent: a second call sees the already-mutated array, and for any puzzle with more than eight stones the spliced-out foil stones are gone for good.It bites hardest in analytics —
logPuzzleEndFirebaseEvent()insrc/scenes/gameplay-scene/gameplay-flow-manager.tscallsgetFoilStones()two or three times while assembling a singlepuzzle_completedevent, so it re-shuffles and re-mutates the pool mid-event. Reproduced onmainat HEAD with a focused test: for a puzzle withtargetStones: ['A']andfoilStones: ['B'..'J'](nine), a singlegetFoilStones()call leaveslevelData.puzzles[0].foilStonesas a shuffled eight-item array withBandCdropped andAinjected, instead of the original nine.npx jest src/components/stone-handler/stone-handler.spec.tsshows the two added assertions failing against the current code.The fix builds the result on a local copy (
const foilStones = [...this.currentPuzzleData.foilStones]) and applies the exact same de-duplication, eight-stone cap and shuffle to that copy, so the value handed tocreateStones()is unchanged while your level data is left untouched; thepuzzle_completedlogging now reads the foils once. The full suite was run before and after the change with no new failures —npx jestreports 44 suites and 302 tests passing (the 300 that already passed, plus the two new regression tests that fail without this change).How this was managed
We mirrored this repository onto a live board and used it to manage this fix. The story tracking the work is at https://eastagiletracker.com/projects/230/stories/100558, on the board at https://eastagiletracker.com/projects/230 — which imported this repository's issues and pull requests as 1,986 stories.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com
Summary by CodeRabbit
Bug Fixes
Tests