Fix/bracket viewer #226
Fix/bracket viewer #226
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/api/brackets/route.jsOops! 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 WalkthroughThis pull request introduces significant changes to the bracket management system across multiple files. The modifications simplify the bracket creation and retrieval processes by removing complex validation and management layers. The data model for brackets has been restructured, with changes to how teams, tournament names, and bracket properties are handled. The front-end components have been updated to work with the new data structure, including modifications to the bracket creation, viewing, and listing components. Changes
Sequence DiagramsequenceDiagram
participant User
participant BracketCreatePage
participant BracketsAPI
participant Database
User->>BracketCreatePage: Enter tournament details
BracketCreatePage->>BracketsAPI: Submit bracket data
BracketsAPI->>Database: Save new bracket
Database-->>BracketsAPI: Confirmation
BracketsAPI-->>BracketCreatePage: Success response
BracketCreatePage->>User: Display bracket
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
app/api/brackets/route.js (2)
24-31: Enhance error response structure for better client-side handlingThe error response currently returns a generic message. For improved client-side error handling, consider adding an error code or more specific details that can help the client application respond appropriately.
40-40: Avoid logging sensitive data in productionThe
console.log(newBracket);statement may expose sensitive information in the server logs. It's advisable to remove this logging statement or ensure that it doesn't log sensitive data, especially in a production environment.Apply this diff to remove the console log:
- console.log(newBracket);app/bracket/create/page.jsx (4)
90-97: Avoid duplicate success messagesBoth
sendBracketsandonTeamSubmitdisplay a success toast upon bracket creation. This results in duplicate success messages to the user.Consider removing the success toast from one of the functions. For example, remove it from
onTeamSubmit:// toast.success("Bracket created successfully");
135-147: Correct 'aria-label' attribute for accessibilityThe attribute
arial-labelis incorrect. It should bearia-labelto comply with accessibility standards.Apply this diff to correct the attribute:
<Button variant="ghost" onClick={() => { bracketCreated ? setBracketCreated(false) : router.push("/bracket"); }} className="mr-auto" - arial-label="back-button" + aria-label="back-button" > <FaArrowLeftLong className="size-5 m-0" /> </Button>
263-276: Correct 'aria-label' attribute and ensure consistencyIn the submit button, the attribute
arial-labelis misspelled and inconsistent with the back button. Ensurearia-labelis correctly spelled and used consistently.Apply this diff to correct the attribute:
<Button type="submit" disabled={showBrackets} - arial-label="submit-team-btn" + aria-label="submit-team-btn" className="w-full md:w-1/6 lg:w-1/6" > Submit </Button>
123-123: Remove commented-out console log statementThere's a commented-out
console.logstatement that isn't necessary. Removing it will help keep the code clean and maintainable.Apply this diff to remove the commented code:
- // console.log("Bracket Info", JSON.stringify(info, null, 2));app/bracket/[id]/page.js (2)
51-52: Ensure 'bracket' data is available before destructuringWhen destructuring
bracket, there's a risk of an error ifbracketisnullorundefined. Even though loading and error states are handled, it's good practice to safeguard against unexpected null values.Ensure
bracketis not null before destructuring:if (!bracket) { + return <div>No bracket data available.</div>; } const { tournament_name, format, consolationFinal, grandFinalType, teams } = bracket;
59-61: Consider adding fallback text for missing tournament nameIf
tournament_nameis undefined, it might cause rendering issues. Adding a fallback can improve user experience.Apply this diff to add a fallback:
<h2 className="font-black text-3xl" id="tournament_heading"> - {tournament_name} + {tournament_name || "Tournament"} </h2>app/bracket/bracket.jsx (1)
58-60: Improve window object check.The window object check could be more robust by checking for
bracketsViewer.Enhance the check:
-if (typeof window === "undefined" || !stageData) return; +if (typeof window === "undefined" || !window.bracketsViewer || !stageData) return;model/Bracket.js (1)
9-13: Add format-specific validation.The format validation could include specific rules for each elimination type.
Consider adding custom validation:
format: { type: String, enum: ["single_elimination", "double_elimination"], required: true, + validate: { + validator: function(v) { + if (v === "double_elimination" && this.teams.length < 8) { + return false; + } + return true; + }, + message: "Double elimination requires at least 8 teams" + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
app/api/brackets/route.js(2 hunks)app/bracket/[id]/page.js(3 hunks)app/bracket/bracket.jsx(1 hunks)app/bracket/create/page.js(0 hunks)app/bracket/create/page.jsx(7 hunks)app/bracket/styles.css(1 hunks)app/createBracket/styles.css(0 hunks)components/BracketList.jsx(1 hunks)model/Bracket.js(1 hunks)
💤 Files with no reviewable changes (2)
- app/bracket/create/page.js
- app/createBracket/styles.css
✅ Files skipped from review due to trivial changes (1)
- app/bracket/styles.css
🔇 Additional comments (2)
app/bracket/create/page.jsx (1)
76-103:⚠️ Potential issueEnsure numeric values are correctly parsed and validated
In the
onTeamSubmitfunction, thevaluesparameter representing the number of teams is received as a string. This can lead to issues when performing numeric comparisons and operations.Apply this diff to parse and validate the number of teams:
function onTeamSubmit(values) { + const numTeams = parseInt(values, 10); + if (isNaN(numTeams) || numTeams < 4) { toast.error("Number of teams must be greater than 4"); return; } - const teams = Array.from({ length: values }, (_, i) => `Team${i + 1}`); + const teams = Array.from({ length: numTeams }, (_, i) => `Team${i + 1}`); const res = { ...bracketInfo, teams }; setInfo(res); sendBrackets({ tournament_name: res.tournament_name, format: res.format, consolationFinal: res.consolationFinal, grandFinalType: res.grandFinalType, teams: res.teams, }); if (!showBrackets) { setShowBrackets(true); } }Likely invalid or redundant comment.
components/BracketList.jsx (1)
71-71: LGTM! Property name aligned with schema.The change from
tournamentNametotournament_namecorrectly aligns with the updated schema inmodel/Bracket.js.Let's verify all tournament name references are consistently updated:
✅ Verification successful
Property name correctly aligned with Bracket model, but inconsistent naming conventions detected
The change from
tournamentNametotournament_namein BracketList.jsx is correct as it aligns with the Bracket model's schema. However, there are two different naming conventions being used across the codebase:
- Bracket-related components use snake_case (
tournament_name)- Tournament-related components use camelCase (
tournamentName)While this doesn't affect functionality as they're separate models, consider standardizing the naming convention in future refactoring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of 'tournamentName' rg -i "tournamentName" --type js --type jsxLength of output: 71
Script:
#!/bin/bash # Search for any remaining instances of both naming conventions rg -i "tournament[_]?name" --type-add 'jsx:*.{jsx,js}' --type jsx -C 2 # Specific pattern search for object property access ast-grep --pattern 'tournamentName' ast-grep --pattern 'tournament_name'Length of output: 20453
app/api/brackets/route.js
Outdated
| if ( | ||
| !tournament_name || | ||
| !format || | ||
| !grandFinalType || | ||
| !teams || | ||
| teams.length < 4 | ||
| ) { | ||
| return NextResponse.json( | ||
| { | ||
| error: | ||
| "Invalid input: Ensure all fields are filled and at least 4 teams are provided.", | ||
| }, | ||
| { status: 400 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider including 'consolationFinal' in input validation
Currently, the input validation checks for the presence of tournament_name, format, grandFinalType, and teams, but it overlooks consolationFinal. If consolationFinal is a required field or holds significance in bracket creation, it should be validated to prevent unexpected behavior.
Apply this diff to include consolationFinal in the validation:
if (
!tournament_name ||
!format ||
!grandFinalType ||
+ typeof consolationFinal !== 'boolean' ||
!teams ||
teams.length < 4
) {
return NextResponse.json(
{
error:
"Invalid input: Ensure all fields are filled and at least 4 teams are provided.",
},
{ status: 400 },
);
}📝 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 ( | |
| !tournament_name || | |
| !format || | |
| !grandFinalType || | |
| !teams || | |
| teams.length < 4 | |
| ) { | |
| return NextResponse.json( | |
| { | |
| error: | |
| "Invalid input: Ensure all fields are filled and at least 4 teams are provided.", | |
| }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if ( | |
| !tournament_name || | |
| !format || | |
| !grandFinalType || | |
| typeof consolationFinal !== 'boolean' || | |
| !teams || | |
| teams.length < 4 | |
| ) { | |
| return NextResponse.json( | |
| { | |
| error: | |
| "Invalid input: Ensure all fields are filled and at least 4 teams are provided.", | |
| }, | |
| { status: 400 }, | |
| ); | |
| } |
app/api/brackets/route.js
Outdated
| const brackets = await Bracket.find(); | ||
| return NextResponse.json(brackets, { status: 200 }); |
There was a problem hiding this comment.
Filter brackets based on user authorization
In the GET function, all brackets are fetched and returned regardless of the authenticated user. If brackets are user-specific or access should be restricted, consider modifying the query to filter brackets based on the user's session.
Apply this diff to filter brackets by the authenticated user:
const brackets = await Bracket.find({
+ userId: session.user.id,
});
return NextResponse.json(brackets, { status: 200 });Ensure that the Bracket model includes a userId field to associate brackets with users.
Committable suggestion skipped: line range outside the PR's diff.
app/bracket/bracket.jsx
Outdated
| // const bracketsViewerNode = document.querySelector(".brackets-viewer"); | ||
| // bracketsViewerNode?.replaceChildren(); | ||
|
|
||
| // window.bracketsViewer.onMatchClicked = async (match) => { | ||
| // console.log("Match clicked", match); | ||
| // try { | ||
| // await manager.update.match({ | ||
| // id: match.id, | ||
| // opponent1: { score: 5 }, | ||
| // opponent2: { score: 7, result: "win" }, | ||
| // }); | ||
| // } catch (error) { | ||
| // console.error("Error during match update:", error); | ||
| // } | ||
| // const tourneyData2 = await manager.get.currentMatches(0); | ||
| // const tourneyData = await manager.get.stageData(0); | ||
| // setStageData(tourneyData); | ||
| // }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or implement commented code.
The commented match click handler should either be implemented or removed.
If the match click functionality is needed, implement it. Otherwise, remove the commented code.
app/bracket/bracket.jsx
Outdated
| function removeBracket() { | ||
| storage.reset(); | ||
| manager.reset; | ||
| router.push("/bracket"); | ||
| } |
There was a problem hiding this comment.
Fix typo in reset function.
The manager.reset call is missing parentheses, making it ineffective.
Fix the typo:
function removeBracket() {
storage.reset();
- manager.reset;
+ manager.reset();
router.push("/bracket");
}📝 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.
| function removeBracket() { | |
| storage.reset(); | |
| manager.reset; | |
| router.push("/bracket"); | |
| } | |
| function removeBracket() { | |
| storage.reset(); | |
| manager.reset(); | |
| router.push("/bracket"); | |
| } |
app/bracket/bracket.jsx
Outdated
| useEffect(() => { | ||
| rendering(); | ||
| }, []); |
There was a problem hiding this comment.
Add cleanup to useEffect.
The useEffect hook should clean up resources on unmount to prevent memory leaks.
Add cleanup:
useEffect(() => {
rendering();
+ return () => {
+ storage.reset();
+ manager.reset();
+ };
}, []);Committable suggestion skipped: line range outside the PR's diff.
app/bracket/bracket.jsx
Outdated
| const storage = new InMemoryDatabase(); | ||
| const manager = new BracketsManager(storage); |
There was a problem hiding this comment.
Move storage and manager inside component.
Global instances of InMemoryDatabase and BracketsManager could cause memory leaks and state conflicts between different bracket instances.
Move these instances inside the component:
-const storage = new InMemoryDatabase();
-const manager = new BracketsManager(storage);
export default function Bracket({...}) {
+ const storage = new InMemoryDatabase();
+ const manager = new BracketsManager(storage);
...
}Committable suggestion skipped: line range outside the PR's diff.
model/Bracket.js
Outdated
| teams: { | ||
| type: [String], // Array of team names | ||
| required: true, | ||
| validate: { | ||
| validator: function (arr) { | ||
| return arr.length >= 4; // Ensure at least 4 teams | ||
| }, | ||
| message: "Number of teams must be at least 4.", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance teams validation.
The teams array validation could be more robust by ensuring non-empty and unique team names.
Add additional validations:
teams: {
type: [String],
required: true,
validate: [
{
validator: function (arr) {
- return arr.length >= 4;
+ return arr.length >= 4 &&
+ arr.every(team => team.trim().length > 0) &&
+ new Set(arr).size === arr.length;
},
- message: "Number of teams must be at least 4.",
+ message: "Must have at least 4 unique, non-empty team names.",
}
],
},📝 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.
| teams: { | |
| type: [String], // Array of team names | |
| required: true, | |
| validate: { | |
| validator: function (arr) { | |
| return arr.length >= 4; // Ensure at least 4 teams | |
| }, | |
| message: "Number of teams must be at least 4.", | |
| }, | |
| }, | |
| teams: { | |
| type: [String], // Array of team names | |
| required: true, | |
| validate: { | |
| validator: function (arr) { | |
| return arr.length >= 4 && | |
| arr.every(team => team.trim().length > 0) && | |
| new Set(arr).size === arr.length; | |
| }, | |
| message: "Must have at least 4 unique, non-empty team names.", | |
| }, | |
| }, |
model/Bracket.js
Outdated
| grandFinalType: { | ||
| type: String, | ||
| enum: ["simple", "double"], | ||
| required: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add conditional validation for grandFinalType.
The grandFinalType should only be required when using double elimination format.
Update the validation:
grandFinalType: {
type: String,
enum: ["simple", "double"],
- required: true,
+ required: function() {
+ return this.format === "double_elimination";
+ },
},Committable suggestion skipped: line range outside the PR's diff.
|
could you look at the suggestions by @CodeRabbit and make changes @thornxyz |
|
@dinxsh: i've previously provided several suggestions for improvements in this pr. here are the key areas that need attention:
once @thornxyz makes the suggested changes, i'll review them in the next iteration. 🔗 analysis chain 🏁 scripts executed the following scripts were executed for the analysis: script: length of output: 11237 ✅ Actions performedReview triggered.
|
thornxyz
force-pushed
the
fix/bracket_viewer
branch
from
f92d605 to
2837699
Compare
rewritten bracket routes, changed backend schema for brackets so that rendering works.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Refactor