-
-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/bracket viewer #224
Fix/bracket viewer #224
Conversation
@thornxyz is attempting to deploy a commit to the blaze's projects Team on Vercel. A member of the Team first needs to authorize it. |
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
app/createBracket/page.jsxOops! Something went wrong! :( ESLint: 9.17.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant User
participant CreateBracketPage
participant BracketSystem
User->>CreateBracketPage: Enter number of teams
CreateBracketPage->>CreateBracketPage: Validate input
alt Valid Input
CreateBracketPage->>BracketSystem: Submit team count
BracketSystem->>BracketSystem: Generate team names
BracketSystem-->>CreateBracketPage: Prepare bracket
else Invalid Input
CreateBracketPage->>User: Display error message
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/createBracket/page.jsx (2)
49-50
: Consider using number type for teams stateUsing string type for numeric input can lead to type coercion issues. Consider using number type and handling the conversion explicitly.
- const [teams, setTeams] = useState(""); + const [teams, setTeams] = useState(0);
106-115
: Optimize power of 2 calculation and improve image handlingConsider these optimizations:
- Use bitwise operations for power of 2 calculations
- Make participant image URL configurable
- Add error handling for image loading
- const nearestPowerOf2 = Math.pow(2, Math.ceil(Math.log2(len))); + const nearestPowerOf2 = 1 << Math.ceil(Math.log2(len));Consider extracting the participant image URL to a configuration file and adding error handling for image loading failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/createBracket/page.jsx
(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/createBracket/page.jsx
[error] 60-60: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
<form onSubmit={handleSubmit} className="lg:w-1/2 mx-auto"> | ||
<h2 className="text-2xl font-bold mb-2">Enter Number of Teams</h2> | ||
<Input | ||
type="text" | ||
id="number" | ||
value={teams} | ||
onChange={handleInputChange} | ||
placeholder="Enter Number of Teams" | ||
/> | ||
{error && <p style={{ color: "red" }}>{error}</p>} | ||
<div className="flex justify-center mt-4"> | ||
<Button | ||
type="button" | ||
variant="outline" | ||
onClick={() => | ||
teamForm.setValue("teams", [...teamForm.watch("teams"), ""]) | ||
} | ||
arial-label="add-another-team-btn" | ||
type="submit" | ||
disabled={showBrackets} | ||
arial-label="submit-team-btn" | ||
> | ||
Add Another Team | ||
Submit | ||
</Button> | ||
<div className="flex justify-center"> | ||
<Button | ||
type="submit" | ||
disabled={teamForm.formState.isSubmitting || showBrackets} | ||
arial-label="submit-team-btn" | ||
> | ||
Submit Teams | ||
</Button> | ||
</div> | ||
</form> | ||
</Form> | ||
</div> | ||
</form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix input semantics and accessibility issues
Several UI improvements are needed:
- Use number input type for semantic HTML
- Fix misspelled aria-label attribute
- Move inline styles to CSS
<Input
- type="text"
+ type="number"
+ min="4"
+ max="64"
id="number"
value={teams}
onChange={handleInputChange}
placeholder="Enter Number of Teams"
/>
- {error && <p style={{ color: "red" }}>{error}</p>}
+ {error && <p className="text-red-500 mt-2">{error}</p>}
<div className="flex justify-center mt-4">
<Button
type="submit"
disabled={showBrackets}
- arial-label="submit-team-btn"
+ aria-label="submit-team-btn"
>
Submit
</Button>
</div>
📝 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.
<form onSubmit={handleSubmit} className="lg:w-1/2 mx-auto"> | |
<h2 className="text-2xl font-bold mb-2">Enter Number of Teams</h2> | |
<Input | |
type="text" | |
id="number" | |
value={teams} | |
onChange={handleInputChange} | |
placeholder="Enter Number of Teams" | |
/> | |
{error && <p style={{ color: "red" }}>{error}</p>} | |
<div className="flex justify-center mt-4"> | |
<Button | |
type="button" | |
variant="outline" | |
onClick={() => | |
teamForm.setValue("teams", [...teamForm.watch("teams"), ""]) | |
} | |
arial-label="add-another-team-btn" | |
type="submit" | |
disabled={showBrackets} | |
arial-label="submit-team-btn" | |
> | |
Add Another Team | |
Submit | |
</Button> | |
<div className="flex justify-center"> | |
<Button | |
type="submit" | |
disabled={teamForm.formState.isSubmitting || showBrackets} | |
arial-label="submit-team-btn" | |
> | |
Submit Teams | |
</Button> | |
</div> | |
</form> | |
</Form> | |
</div> | |
</form> | |
<form onSubmit={handleSubmit} className="lg:w-1/2 mx-auto"> | |
<h2 className="text-2xl font-bold mb-2">Enter Number of Teams</h2> | |
<Input | |
type="number" | |
min="4" | |
max="64" | |
id="number" | |
value={teams} | |
onChange={handleInputChange} | |
placeholder="Enter Number of Teams" | |
/> | |
{error && <p className="text-red-500 mt-2">{error}</p>} | |
<div className="flex justify-center mt-4"> | |
<Button | |
type="submit" | |
disabled={showBrackets} | |
aria-label="submit-team-btn" | |
> | |
Submit | |
</Button> | |
</div> | |
</form> |
if (values < 4) { | ||
toast.error("Number of teams must be greater than 4"); | ||
return; | ||
} | ||
|
||
const uniqueTeams = new Set(values.teams); | ||
if (uniqueTeams.size !== values.teams.length) { | ||
toast.error("Team names must be unique."); | ||
return; | ||
} | ||
const teams = Array.from({ length: values }, (_, i) => `Team${i + 1}`); | ||
|
||
const res = { ...bracketInfo, ...values }; | ||
const res = { ...bracketInfo, teams }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance team generation and validation
The team submission logic could be improved in several ways:
- Add maximum team validation
- Improve team name generation
- Move success toast after successful bracket creation
function onTeamSubmit(values) {
- if (values < 4) {
+ if (values < 4 || values > 64) {
- toast.error("Number of teams must be greater than 4");
+ toast.error("Number of teams must be between 4 and 64");
return;
}
- const teams = Array.from({ length: values }, (_, i) => `Team${i + 1}`);
+ const teams = Array.from(
+ { length: values },
+ (_, i) => `Team ${String(i + 1).padStart(2, '0')}`
+ );
const res = { ...bracketInfo, teams };
setInfo(res);
console.log("Bracket Info", JSON.stringify(info, null, 2));
if (!showBrackets) {
setShowBrackets(true);
}
- toast.success("Bracket created successfully");
}
📝 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.
if (values < 4) { | |
toast.error("Number of teams must be greater than 4"); | |
return; | |
} | |
const uniqueTeams = new Set(values.teams); | |
if (uniqueTeams.size !== values.teams.length) { | |
toast.error("Team names must be unique."); | |
return; | |
} | |
const teams = Array.from({ length: values }, (_, i) => `Team${i + 1}`); | |
const res = { ...bracketInfo, ...values }; | |
const res = { ...bracketInfo, teams }; | |
if (values < 4 || values > 64) { | |
toast.error("Number of teams must be between 4 and 64"); | |
return; | |
} | |
const teams = Array.from( | |
{ length: values }, | |
(_, i) => `Team ${String(i + 1).padStart(2, '0')}` | |
); | |
const res = { ...bracketInfo, teams }; |
const handleInputChange = (e) => { | ||
setTeams(e.target.value); | ||
setError(""); | ||
}; | ||
|
||
const handleSubmit = (e) => { | ||
e.preventDefault(); | ||
|
||
if (isNaN(teams) || teams === "") { | ||
setError("Please enter a valid number."); | ||
} else { | ||
onTeamSubmit(teams); | ||
setTeams(""); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve input validation and safety
The current validation has several issues:
- Using
isNaN
is unsafe due to type coercion - Missing validation for negative numbers and decimals
- No maximum limit for number of teams
Apply these improvements:
const handleInputChange = (e) => {
- setTeams(e.target.value);
+ const value = e.target.value.trim();
+ setTeams(value);
setError("");
};
const handleSubmit = (e) => {
e.preventDefault();
+ const numTeams = Number(teams);
- if (isNaN(teams) || teams === "") {
+ if (Number.isNaN(numTeams) || teams === "" || !Number.isInteger(numTeams) || numTeams <= 0) {
setError("Please enter a valid number.");
+ } else if (numTeams > 64) {
+ setError("Maximum 64 teams allowed.");
} else {
- onTeamSubmit(teams);
+ onTeamSubmit(numTeams);
setTeams("");
}
};
📝 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 handleInputChange = (e) => { | |
setTeams(e.target.value); | |
setError(""); | |
}; | |
const handleSubmit = (e) => { | |
e.preventDefault(); | |
if (isNaN(teams) || teams === "") { | |
setError("Please enter a valid number."); | |
} else { | |
onTeamSubmit(teams); | |
setTeams(""); | |
} | |
}; | |
const handleInputChange = (e) => { | |
const value = e.target.value.trim(); | |
setTeams(value); | |
setError(""); | |
}; | |
const handleSubmit = (e) => { | |
e.preventDefault(); | |
const numTeams = Number(teams); | |
if (Number.isNaN(numTeams) || teams === "" || !Number.isInteger(numTeams) || numTeams <= 0) { | |
setError("Please enter a valid number."); | |
} else if (numTeams > 64) { | |
setError("Maximum 64 teams allowed."); | |
} else { | |
onTeamSubmit(numTeams); | |
setTeams(""); | |
} | |
}; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
added bye logic, now can generate bracket for any number of teams
Summary by CodeRabbit
New Features
Bug Fixes
User Interface