hardcode timer (TO REVERT) #2425
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications to three components related to the countdown timer functionality. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
landing/src/components/modules/season-start-timer.tsx (1)
13-13: Consider accessibility for the timer displayWhile the "<" character addition matches the CountdownTimer component, consider:
- Adding ARIA labels for screen readers
- Using semantic HTML for better accessibility
Consider this improvement:
- {"<"} {String(hours).padStart(2, "0")}:{String(minutes).padStart(2, "0")}:{String(seconds).padStart(2, "0")} + <span aria-hidden="true">{"<"}</span> + <span aria-label={`${hours} hours ${minutes} minutes ${seconds} seconds remaining`}> + {String(hours).padStart(2, "0")}:{String(minutes).padStart(2, "0")}:{String(seconds).padStart(2, "0")} + </span>landing/src/components/ui/elements/CountdownTimer.tsx (1)
25-25: Enhance accessibility and maintain visual consistencyThe added "<" character matches the SeasonStartTimer component, but consider:
- Adding ARIA labels for screen readers
- Using semantic HTML for better accessibility
- Extracting the "<" character to a shared constant for consistency
Consider these improvements:
+const TIMER_PREFIX = "<"; // ... in the component - <span className="text-4xl font-bold">{"<"}</span> + <span className="text-4xl font-bold" aria-hidden="true">{TIMER_PREFIX}</span>Also, consider wrapping the entire timer in a more semantic structure:
- <div className="flex gap-4 text-center mt-4 mx-auto"> + <div className="flex gap-4 text-center mt-4 mx-auto" role="timer" aria-label="Season start countdown">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
landing/src/components/modules/season-start-timer.tsx(1 hunks)landing/src/components/ui/elements/CountdownTimer.tsx(1 hunks)landing/src/hooks/use-season-start.tsx(1 hunks)
🔇 Additional comments (1)
landing/src/components/ui/elements/CountdownTimer.tsx (1)
Line range hint 13-31: Verify the overlay's accessibility impact
The blurred overlay might affect the content's readability. Ensure sufficient contrast ratio is maintained.
✅ Verification successful
Based on the search results, I can now generate the final response:
The overlay's contrast appears to be consistent with established patterns
The blurred overlay in the CountdownTimer uses bg-black/30 with backdrop-blur-sm, which aligns with similar overlay implementations across the codebase. This opacity level is used in other loading/overlay scenarios (e.g., LoadingScreen component) and provides sufficient visibility while maintaining readability of overlaid content. The opacity value of 0.3 (30%) with blur is a common pattern that balances visibility and contrast.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other overlay usages and their opacity values
rg -A 2 'bg-black/[0-9]+'
Length of output: 4056
| const seasonStart = BigInt(new Date("2024-12-11T15:35:00Z").getTime() / 1000); | ||
|
|
||
| /*BigInt(configManager.getSeasonConfig().startAt || 0);*/ |
There was a problem hiding this comment.
Critical: Reconsider hardcoding the season start date
The change from a configuration-based approach to a hardcoded date raises several concerns:
- Hardcoding dates makes the code less maintainable and requires code changes for future seasons
- The date is set to December 2024, which seems unusually far in the future
- The commented-out code should be removed rather than left in the codebase
Consider these alternatives:
- Keep using the configuration-based approach for flexibility
- If a fixed date is required, move it to a configuration file
- Add error handling for the date conversion
- const seasonStart = BigInt(new Date("2024-12-11T15:35:00Z").getTime() / 1000);
-
- /*BigInt(configManager.getSeasonConfig().startAt || 0);*/
+ const seasonStart = BigInt(configManager.getSeasonConfig().startAt || 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const seasonStart = BigInt(new Date("2024-12-11T15:35:00Z").getTime() / 1000); | |
| /*BigInt(configManager.getSeasonConfig().startAt || 0);*/ | |
| const seasonStart = BigInt(configManager.getSeasonConfig().startAt || 0); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
client/src/hooks/useSeasonStart.tsx (3)
1-1: Remove commented import instead of commenting it outCommented-out code can lead to confusion and maintenance issues. If the
configManageris no longer needed, it should be removed entirely rather than commented out.-//import { configManager } from "@/dojo/setup";
Line range hint
8-21: Enhance hook robustness with additional validations and optimizationsThe current implementation could benefit from several improvements:
- Add a condition to stop the interval when countdown reaches zero
- Include error handling for invalid dates
- Consider performance optimizations for the interval
Here's a suggested improvement:
useMemo(() => { if (nextBlockTimestamp === 0n || seasonStart === 0n) return; const initialCountdown = seasonStart - nextBlockTimestamp; if (initialCountdown <= 0n) { setCountdown(0n); return; } setCountdown(initialCountdown); const timer = setInterval(() => { setCountdown((prev) => { const next = prev - 1n; if (next <= 0n) { clearInterval(timer); return 0n; } return next; }); }, 1000); return () => clearInterval(timer); }, [nextBlockTimestamp, seasonStart]);
Based on the search results, I can now provide a final response:
Hardcoded season start date requires coordinated updates
The hardcoded season start date is used in both client and landing applications:
client/src/hooks/useSeasonStart.tsxlanding/src/hooks/use-season-start.tsxBoth implementations need to be kept in sync to maintain consistency across the applications. Currently:
- Client:
2024-12-11T15:35:00Z- Landing: Using commented out config manager code
Please ensure both implementations use the same season start date to avoid confusion for users viewing different parts of the application.
🔗 Analysis chain
Line range hint
1-24: Verify impact on dependent componentsThe switch from dynamic configuration to a hardcoded date might affect other components that rely on this hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components using this hook echo "Components using useSeasonStart:" rg "useSeasonStart" --type tsx -l # Search for any remaining references to seasonConfig echo -e "\nReferences to season configuration:" rg "getSeasonConfig|seasonConfig" --type tsxLength of output: 298
Script:
#!/bin/bash # Search for components using this hook without file type restriction echo "Components using useSeasonStart:" rg "useSeasonStart" # Search for any remaining references to season configuration echo -e "\nReferences to season configuration:" rg "getSeasonConfig|seasonConfig" # List all TypeScript/React files to understand the extension pattern echo -e "\nTypeScript/React files in the repository:" fd "\.(tsx?|jsx?)$"Length of output: 25711
| import { useMemo, useState } from "react"; | ||
|
|
||
| export const useSeasonStart = () => { | ||
| const seasonStart = useMemo(() => BigInt(configManager.getSeasonConfig().startAt || 0), []); | ||
| const seasonStart = BigInt(new Date("2024-12-11T15:35:00Z").getTime() / 1000);/*useMemo(() => BigInt(configManager.getSeasonConfig().startAt || 0), []);*/ |
There was a problem hiding this comment.
Avoid hardcoding future dates and consider configuration-based approach
Several concerns with the current implementation:
- Hardcoding dates makes the code less maintainable and requires code changes for future seasons
- The date is set to December 2024, which means this code will need to be updated
- The inline comment with the old implementation should be removed
Consider these alternatives:
- Move the date to an environment variable or configuration file
- Implement a more flexible system that can handle multiple seasons without code changes
- Add validation for the date to ensure it's not in the past
Example implementation:
const seasonStart = useMemo(() => {
const configuredDate = process.env.SEASON_START_DATE || "2024-12-11T15:35:00Z";
const timestamp = new Date(configuredDate).getTime() / 1000;
if (timestamp < Date.now() / 1000) {
console.warn("Season start date is in the past");
}
return BigInt(timestamp);
}, []);
Summary by CodeRabbit
New Features
Bug Fixes