feat: Standardize betting validation, improve loading states, and add service tests#10
Merged
Merged
Conversation
Changed:
- Wrapped SignIn/SignUp with ClerkLoading/ClerkLoaded components
- Added HuskyBidsLoader with subtitle support during Clerk initialization
- Extracted duplicate Clerk appearance config to shared utility
- Standardized auth page styling across login and signup
- Updated test mocks to include ClerkLoading/ClerkLoaded components
Files:
- src/app/login/[[...sign-in]]/page.js
- src/app/sign-up/[[...sign-up]]/page.jsx
- src/components/experimental/ui/HuskyBidsLoader.jsx
- src/shared/utils/clerk-appearance.js
- src/app/__tests__/auth-pages.test.jsx
- src/app/sign-up/__tests__/redirect.test.jsx
Why:
Clerk components can take 500-2000ms to initialize, causing a blank screen flash that makes the app feel unresponsive. The new ClerkLoading state shows users a branded loader ("Loading your account...") during initialization. Also reduces code duplication by centralizing the 50+ lines of appearance config that was copied between login and signup pages.
Changed: - LoadingContext: Removed isLoading from useEffect dependencies - BettingService: Removed fallback to non-existent game.startTime field Files: - src/app/contexts/LoadingContext.jsx - src/server/services/BettingService.js Why: The LoadingContext bug caused loading states to immediately cancel (infinite loop: setLoading(true) → effect fires → setLoading(false)). Only pathname should trigger the cleanup effect. The BettingService cleanup removes a confusing fallback to a field that doesn't exist in the Game schema (only gameDate is defined).
…robustness Changed: - Migrated useBetValidation hook to use gameDate exclusively for start-time checks - Added isNaN check to server-side BettingService to prevent betting on invalid dates - Updated betting service tests to include gameDate in mock objects Why: The application was inconsistently using startTime and gameDate for betting availability checks. Standardizing on gameDate ensures alignment with the database schema and simplifies validation logic. Adding the isNaN check on the server prevents a fail-open scenario where invalid dates could bypass the start-time validation.
Changed: - Removed conditional check before resetting isLoading and message in LoadingProvider Why: Ensures that any global loading state is always cleared immediately when a user navigates to a new route, regardless of its current state. This prevents edge cases where a loading spinner might persist into a new page due to race conditions in state updates.
Changed: - Added useMemo for the tasks array derived from SWR data Why: SWR data updates can trigger frequent re-renders. Memoizing the tasks array prevents child components that depend on it from re-rendering unnecessarily when other parts of the data object change but the tasks themselves remain identical.
✅ 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 standardizes betting validation logic to consistently use gameDate instead of the dual startTime/gameDate approach, hardens validation against invalid dates, improves loading state management during navigation, and centralizes Clerk appearance configuration. Additionally, it introduces comprehensive unit tests for core backend services.
- Migrated betting validation (client and server) to exclusively use
gameDate, aligning with the database schema - Fixed race condition in
LoadingContextby unconditionally clearing state on route changes - Centralized Clerk authentication UI styling and added loading states to login/signup flows
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/shared/utils/clerk-appearance.js |
Standardized Clerk appearance configuration with updated color values and simplified font stack |
src/server/services/__tests__/BettingService.test.js |
Added gameDate field to test mock to align with validation changes |
src/server/services/BettingService.js |
Updated validation to use only gameDate and added isNaN check for invalid dates |
src/components/experimental/ui/HuskyBidsLoader.jsx |
Added optional subtitle prop for contextual loading messages |
src/app/tasks/page.jsx |
Added memoization comment to clarify optimization |
src/app/sign-up/__tests__/redirect.test.jsx |
Added mocks for ClerkLoading and ClerkLoaded components |
src/app/sign-up/[[...sign-up]]/page.jsx |
Refactored to use centralized appearance config and added loading states |
src/app/login/[[...sign-in]]/page.js |
Refactored to use centralized appearance config and added loading states |
src/app/hooks/useBetValidation.js |
Simplified to use only gameDate and removed startTime references |
src/app/hooks/__tests__/useBetValidation.test.js |
Updated tests to use gameDate instead of startTime |
src/app/contexts/LoadingContext.jsx |
Simplified route change effect to always clear loading state |
src/app/__tests__/auth-pages.test.jsx |
Added mocks for ClerkLoading and ClerkLoaded components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+10
to
+55
| function LoginContent() { | ||
| const searchParams = useSearchParams(); | ||
| const redirectUrl = searchParams.get('redirect') || '/dashboard'; | ||
|
|
||
| return ( | ||
| <div className="w-full max-w-md space-y-8"> | ||
| {/* Minimal header */} | ||
| <header className="text-center mb-8"> | ||
| <h1 className="text-sm tracking-[0.3em] uppercase text-zinc-400 mb-2"> | ||
| HuskyBids | ||
| </h1> | ||
| <p className="text-xs text-zinc-600"> | ||
| Log in to your account | ||
| </p> | ||
| </header> | ||
| return ( | ||
| <div className="w-full max-w-md space-y-8"> | ||
| <ClerkLoading> | ||
| <HuskyBidsLoader | ||
| centered | ||
| subtitle="Loading your account..." | ||
| /> | ||
| </ClerkLoading> | ||
|
|
||
| {/* Clerk SignIn Component with minimal styling */} | ||
| <div> | ||
| <SignIn | ||
| appearance={{ | ||
| variables: { | ||
| colorPrimary: '#71717a', | ||
| colorText: '#d4d4d8', | ||
| colorTextSecondary: '#71717a', | ||
| colorBackground: '#18181b', | ||
| colorInputBackground: '#27272a', | ||
| colorInputText: '#d4d4d8', | ||
| fontFamily: 'ui-monospace, monospace', | ||
| fontSize: '14px', | ||
| borderRadius: '0px', | ||
| }, | ||
| elements: { | ||
| rootBox: 'mx-auto', | ||
| card: 'bg-zinc-900 border border-dotted border-zinc-800 shadow-none', | ||
| headerTitle: 'text-zinc-300 text-sm font-mono', | ||
| headerSubtitle: 'text-zinc-600 text-xs font-mono', | ||
| formFieldInput: 'bg-zinc-800 border border-dotted border-zinc-700 text-zinc-300 font-mono text-sm focus:border-zinc-500 rounded-none', | ||
| formFieldLabel: 'text-zinc-500 text-xs font-mono uppercase tracking-wider', | ||
| formButtonPrimary: 'bg-zinc-800 hover:bg-zinc-700 text-zinc-300 border border-dotted border-zinc-700 font-mono text-sm shadow-none rounded-none', | ||
| footerActionLink: 'text-zinc-500 hover:text-zinc-400 font-mono text-xs', | ||
| socialButtonsBlockButton: 'border border-dotted border-zinc-700 bg-zinc-800 hover:bg-zinc-700 text-zinc-400 font-mono text-sm rounded-none shadow-none', | ||
| dividerLine: 'bg-zinc-800', | ||
| dividerText: 'text-zinc-600 font-mono text-xs', | ||
| }, | ||
| layout: { | ||
| socialButtonsPlacement: 'bottom', | ||
| socialButtonsVariant: 'iconButton', | ||
| } | ||
| }} | ||
| path="/login" | ||
| afterSignInUrl={redirectUrl} | ||
| signUpUrl="/sign-up" | ||
| /> | ||
| </div> | ||
| <ClerkLoaded> | ||
| {/* Header */} | ||
| <header className="text-center mb-8"> | ||
| <h1 className="text-sm tracking-[0.3em] uppercase text-zinc-400 mb-2"> | ||
| HuskyBids | ||
| </h1> | ||
| <p className="text-xs text-zinc-600"> | ||
| Log in to your account | ||
| </p> | ||
| </header> | ||
|
|
||
| {/* Clerk SignIn Component */} | ||
| <div> | ||
| <SignIn | ||
| appearance={minimalClerkAppearance} | ||
| path="/login" | ||
| afterSignInUrl={redirectUrl} | ||
| signUpUrl="/sign-up" | ||
| /> | ||
| </div> | ||
|
|
||
| {/* Footer link */} | ||
| <p className="text-center text-xs text-zinc-600 font-mono mt-6"> | ||
| No account?{' '} | ||
| <Link href="/sign-up" className="text-zinc-500 hover:text-zinc-400 underline"> | ||
| Sign up | ||
| </Link> | ||
| </p> | ||
| </div> | ||
| ); | ||
| } | ||
| {/* Footer link */} | ||
| <p className="text-center text-xs text-zinc-600 font-mono mt-6"> | ||
| No account?{' '} | ||
| <Link href="/sign-up" className="text-zinc-500 hover:text-zinc-400 underline"> | ||
| Sign up | ||
| </Link> | ||
| </p> | ||
|
|
||
| /** | ||
| * Login Page | ||
| * Uses Clerk's pre-built SignIn component for authentication | ||
| * Clerk handles redirects natively via afterSignInUrl | ||
| */ | ||
| export default function LoginPage() { | ||
| return ( | ||
| <div className="min-h-screen bg-zinc-950 flex items-center justify-center px-4 py-12 font-mono"> | ||
| <Suspense fallback={<div className="text-zinc-500 text-sm">Loading...</div>}> | ||
| <LoginContent /> | ||
| </Suspense> | ||
| </div> | ||
| ); | ||
| } | ||
| </ClerkLoaded> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation detected. The LoginContent function has an extra indentation level (2 spaces) compared to the rest of the file. The function declaration should start at column 0 like the export default function LoginPage() below it. This appears to be a formatting error introduced during refactoring.
| fontFamily: 'ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace', | ||
| fontSize: '0.875rem', // 14px | ||
| borderRadius: '0', // No rounded corners | ||
| fontFamily: 'ui-monospace, monospace', |
There was a problem hiding this comment.
The fontFamily fallback list was significantly reduced from a comprehensive CSS font stack to just 'ui-monospace, monospace'. While this may work, the original included system monospace fonts (SFMono-Regular, Menlo, Monaco, Consolas, Liberation Mono, Courier New) as fallbacks for better cross-platform compatibility. The generic monospace keyword may not render consistently across all browsers and operating systems. Consider keeping the more comprehensive fallback list unless there's a specific reason to simplify it.
Suggested change
| fontFamily: 'ui-monospace, monospace', | |
| fontFamily: 'ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace', |
|
|
||
| if (gameTime <= cutoffTime) { | ||
| if (isNaN(gameTime.getTime()) || gameTime <= cutoffTime) { | ||
| throw new Error(`Betting is closed - game has already started (gameDate: ${gameTime.toISOString()}, now: ${now.toISOString()})`); |
There was a problem hiding this comment.
When gameTime is invalid (NaN), calling gameTime.toISOString() will throw a RangeError. The error message construction will fail before the error can be thrown. Consider using a conditional check to safely format the date in the error message, or use a fallback string like "Invalid Date" when the date is NaN.
Suggested change
| throw new Error(`Betting is closed - game has already started (gameDate: ${gameTime.toISOString()}, now: ${now.toISOString()})`); | |
| const gameTimeStr = isNaN(gameTime.getTime()) ? 'Invalid Date' : gameTime.toISOString(); | |
| throw new Error(`Betting is closed - game has already started (gameDate: ${gameTimeStr}, now: ${now.toISOString()})`); |
Remove redundant comment. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 refactors the betting validation logic to use
gameDateconsistently across the client and server. It also resolves race conditions in the global loading state during navigation, optimizes the Tasks page rendering, and introduces comprehensive unit tests for core server services.What problem does it solve?
gameDatefor betting availability checks, aligning with the database schema.BettingServicewhere invalid or missing dates could bypass start-time checks.Context / Background
gameDate.Changes
useBetValidation(client) andBettingService(server) to usegameDateexclusively.LoadingContextto unconditionally reset state on route changes.BettingService,GameService, andStatisticsService.Implementation Details
BettingService.js: Validation logic now explicitly checksisNaN(gameTime.getTime())to ensure corrupted dates default to a "closed" state.LoadingContext.jsx: The route change effect now clears loading state without checking the currentisLoadingflag, ensuring a fresh state on every navigation.clerk-appearance.js: Centralized styling for Clerk components to match the project's minimal design system.Tests
How to test:
npm testand confirm all 84 tests pass across 7 suites.Impact / Risk
Checklist