Refactor Backend Services, Fix Hook Dependencies, and Clean Up Components#9
Merged
Conversation
…nd services Summary: Implemented singleton pattern and comprehensive testing for backend services Changed: - Refactored BettingService, GameService, and StatisticsService to export singleton instances - Added full unit test suites covering happy paths, error handling, and transaction rollbacks - Updated Jest configuration to support backend service module resolution Files: - jest.config.js - src/server/services/BettingService.js - src/server/services/GameService.js - src/server/services/StatisticsService.js - src/server/services/__tests__/BettingService.test.js - src/server/services/__tests__/GameService.test.js - src/server/services/__tests__/StatisticsService.test.js Why: To improve system stability and maintainability. Exporting singletons ensures consistent state management across the server. The new test coverage validates critical betting logic and database transactions, protecting against regressions in financial operations.
Summary: Fixed stale closures and dependency issues in React hooks Changed: - Wrapped fetchGames in useCallback to prevent infinite re-fetching loops in useGames hook - Corrected useEffect dependency arrays in LoadingContext, Home, and Tasks pages Files: - src/app/contexts/LoadingContext.jsx - src/app/hooks/useGames.js - src/app/page.jsx - src/app/tasks/page.jsx Why: To prevent performance issues and infinite API call loops caused by missing dependencies or unstable function references in useEffect hooks.
Summary: Removed redundant state management from MinimalBettingModal Changed: - Removed unused stateRef and associated useEffect - Cleaned up unused imports Files: - src/components/experimental/ui/MinimalBettingModal.jsx Why: To clean up dead code and improve component readability following previous refactors.
✅ Deploy Preview for huskybids ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors backend services to use the singleton pattern, adds comprehensive unit test coverage for BettingService, GameService, and StatisticsService, and fixes React hook dependency issues that could cause infinite loops or stale closures. It also includes minor code cleanup in components.
Key changes:
- Backend services now export singleton instances for consistent state management
- Added 1,100+ lines of unit tests covering happy paths, error handling, and transaction rollback scenarios
- Fixed missing dependencies in useEffect/useCallback hooks across multiple components
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/services/tests/StatisticsService.test.js | New comprehensive test suite for StatisticsService with 265 lines covering user stats, leaderboards, rankings, and global statistics |
| src/server/services/tests/GameService.test.js | New comprehensive test suite for GameService with 376 lines covering ESPN updates, game lifecycle, and betting automation |
| src/server/services/tests/BettingService.test.js | New comprehensive test suite for BettingService with 459 lines covering bet placement, settlement, refunds, and transaction handling |
| src/server/services/StatisticsService.js | Refactored to export singleton instance via intermediate variable |
| src/server/services/GameService.js | Updated imports to use path aliases and exported singleton instance |
| src/server/services/BettingService.js | Simplified date validation logic, exported singleton instance, but has indentation and documentation issues |
| src/components/experimental/ui/MinimalBettingModal.jsx | Fixed useCallback and useEffect dependency arrays to prevent stale closures |
| src/app/hooks/useGames.js | Fixed useEffect dependency by wrapping fetchGames in useCallback |
| src/app/contexts/LoadingContext.jsx | Added missing isLoading dependency to useEffect |
| src/app/page.jsx | Added missing router dependency to useEffect |
| src/app/tasks/page.jsx | Wrapped tasks in useMemo but missing the import |
| jest.config.js | Added transformIgnorePatterns for ESM node_modules (bson, mongoose, mongodb) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| const tasks = data?.tasks || []; | ||
| const tasks = useMemo(() => data?.tasks || [], [data?.tasks]); |
There was a problem hiding this comment.
The useMemo hook is used on line 42 but is not imported. Add useMemo to the React imports on line 3.
Comment on lines
62
to
71
| // Validation: Check if game has started | ||
| // Use a small buffer (e.g., 5 seconds) to account for slight server time differences | ||
| // But be strict - if game has started, no bets. | ||
| const now = new Date(); | ||
| const gameTime = new Date(game.gameDate || game.startTime); // Handle both field names | ||
|
|
||
| if (gameTime <= now) { | ||
| throw new Error(`Betting is closed - game has already started (gameDate: ${gameTime.toISOString()}, now: ${now.toISOString()})`); | ||
| } | ||
|
|
There was a problem hiding this comment.
The indentation for the game time validation block is inconsistent with the surrounding code. Lines 62-70 should be indented to align with the rest of the method body (lines 36-60 and 72+). The comment mentions a buffer of 5 seconds but the actual logic doesn't implement any buffer - it uses a strict less-than-or-equal comparison.
Suggested change
| // Validation: Check if game has started | |
| // Use a small buffer (e.g., 5 seconds) to account for slight server time differences | |
| // But be strict - if game has started, no bets. | |
| const now = new Date(); | |
| const gameTime = new Date(game.gameDate || game.startTime); // Handle both field names | |
| if (gameTime <= now) { | |
| throw new Error(`Betting is closed - game has already started (gameDate: ${gameTime.toISOString()}, now: ${now.toISOString()})`); | |
| } | |
| // Validation: Check if game has started | |
| // Use a small buffer (e.g., 5 seconds) to account for slight server time differences | |
| // But be strict - if game has started, no bets. | |
| const now = new Date(); | |
| const gameTime = new Date(game.gameDate || game.startTime); // Handle both field names | |
| // Close betting if the game is starting in 5 seconds or less (or already started) | |
| if (gameTime.getTime() - now.getTime() <= 5000) { | |
| throw new Error( | |
| `Betting is closed - game has already started (gameDate: ${gameTime.toISOString()}, now: ${now.toISOString()})` | |
| ); | |
| } |
Comment on lines
+67
to
+68
|
|
||
| if (gameTime <= now) { |
There was a problem hiding this comment.
The comment on line 63 states "Use a small buffer (e.g., 5 seconds) to account for slight server time differences" but the implementation on line 68 uses a strict comparison (<=) without any buffer. Either implement the buffer mentioned in the comment or update the comment to reflect the actual strict behavior.
Suggested change
| if (gameTime <= now) { | |
| const BUFFER_MS = 5000; // 5-second buffer for clock skew | |
| const cutoffTime = new Date(now.getTime() - BUFFER_MS); | |
| if (gameTime <= cutoffTime) { |
Summary: Implemented betting window buffer and corrected code formatting in BettingService Changed: - Introduced a 5-second grace period for placing bets after the official game start time - Aligned indentation of the game-start validation block with the method body - Updated unit tests to verify both strict expiration and the new buffer period Files: - src/server/services/BettingService.js - src/server/services/__tests__/BettingService.test.js Why: To improve user experience by accounting for minor clock skews or network latency between the client and server. A strict cutoff can frustrate users whose bets are received milliseconds after the official start. The indentation fix ensures codebase consistency and readability.
Summary: Resolved ReferenceError in TasksPage by importing missing React hook Changed: - Added useMemo to the React named imports Files: - src/app/tasks/page.jsx Why: The TasksPage component was utilizing useMemo to compute the task list but lacked the corresponding import, leading to a runtime ReferenceError.
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
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.
Summary
This PR implements significant improvements to the backend service architecture by adopting the singleton pattern and adding comprehensive unit tests. It also addresses stability issues in React hooks by resolving stale closures and dependency warnings, and performs minor code cleanup in UI components.
What problem does it solve?
useEffectanduseCallbackhooks had missing dependencies, causing potential infinite loops or stale data.MinimalBettingModalcontained unused state references.Context / Background
test/backend-coverage).Changes
BettingService,GameService, andStatisticsServiceto export singleton instances.useGames.js,LoadingContext.jsx,app/page.jsx, andtasks/page.jsx.stateRef) fromMinimalBettingModal.jsx.Implementation Details
Tests
How to test:
npm test src/server/services(Verify all pass).Impact / Risk
Checklist