brackt/plans/phase2-bugs-review.md

Phase 2 Code Review - Bug Report

🔴 CRITICAL BUGS

Bug #1: Winner Advancement Logic Is Incorrect

File: app/models/playoff-match.ts:205-247

Issue: The advanceWinner() function uses "fill participant1 first, then participant2" logic, which doesn't guarantee proper bracket structure.

Current Code:

// Determine which participant slot to fill
// For simplicity, fill participant1 first, then participant2
if (!nextMatch.participant1Id) {
  await updatePlayoffMatch(nextMatch.id, { participant1Id: winnerId });
} else if (!nextMatch.participant2Id) {
  await updatePlayoffMatch(nextMatch.id, { participant2Id: winnerId });
}

Problem: For Quarterfinals → Semifinals:

• QF Match 1 winner should → SF Match 1, participant1
• QF Match 2 winner should → SF Match 1, participant2
• QF Match 3 winner should → SF Match 2, participant1
• QF Match 4 winner should → SF Match 2, participant2

But with current logic:

• QF Match 1 winner → SF Match 1, participant1 ✅
• QF Match 2 winner → SF Match 1, participant2 ✅
• QF Match 3 winner → SF Match 2, participant1 ✅
• QF Match 4 winner → SF Match 2, participant2 ✅

Actually, this WORKS for simple cases, but breaks if matches complete out of order:

• If QF Match 2 completes before Match 1, winner goes to SF1 participant1
• Then QF Match 1 winner goes to SF1 participant2
• This reverses the bracket structure!

Fix:

// Determine which participant slot to fill based on match number
if (match.round === "Quarterfinals") {
  nextRound = "Semifinals";
  nextMatchNumber = match.matchNumber <= 2 ? 1 : 2;

  // Odd matches (1, 3) → participant1, Even matches (2, 4) → participant2
  const participantSlot = match.matchNumber % 2 === 1 ? 'participant1Id' : 'participant2Id';

  await updatePlayoffMatch(nextMatch.id, { [participantSlot]: winnerId });
} else if (match.round === "Semifinals") {
  nextRound = "Finals";
  nextMatchNumber = 1;

  // SF Match 1 → Finals participant1, SF Match 2 → Finals participant2
  const participantSlot = match.matchNumber === 1 ? 'participant1Id' : 'participant2Id';

  await updatePlayoffMatch(nextMatch.id, { [participantSlot]: winnerId });
}

Impact: Bracket structure breaks if matches complete out of order. Semifinals and Finals may have wrong matchups.

Covered in Future Plans? Yes, bracket-expansion.md will rewrite this entirely with template system.

---

Bug #2: Multiple Fantasy Leagues with Different Scoring Rules

File: app/models/scoring-calculator.ts:66-74

Issue: When processing playoff events, we only use the FIRST fantasy season's scoring rules, even if multiple fantasy leagues with different rules use the same sports season.

Current Code:

const seasonSport = event.sportsSeason.seasonSports[0];
if (!seasonSport) {
  throw new Error(`No fantasy seasons found for sports season ${event.sportsSeasonId}`);
}

const scoringRules = await getScoringRules(seasonSport.seasonId, db);

Problem: Scenario:

• League A uses "2024 NFL Playoffs" with scoring: 1st=100, 2nd=70
• League B uses "2024 NFL Playoffs" with scoring: 1st=200, 2nd=150

When Super Bowl completes:

• We calculate points using League A's rules (100 pts)
• Store pointsAwarded: 100 in participantResults
• League B also sees 100 pts instead of 200 pts

Actually Not a Bug (Misleading Design): Upon further review, pointsAwarded in participantResults is stored but never used!

In calculateTeamScore() (line 294):

const points = calculateFantasyPoints(result.finalPosition, scoringRules);

It recalculates points from finalPosition using each fantasy season's own scoring rules.

The Real Issues:

1. Confusing Design: We store pointsAwarded but never use it
2. Wasted Storage: Storing incorrect values that are never read
3. Potential Future Bug: Someone might assume pointsAwarded is correct and use it

Recommendation: Either:

• Option A: Stop storing pointsAwarded entirely (per plans/scoring-system.md line 341)
• Option B: Document clearly that pointsAwarded is for admin display only, not calculations

Impact: Currently none (not used), but high risk of future bugs.

Covered in Future Plans? Not explicitly mentioned. This is a design cleanup issue.

---

🟡 MODERATE BUGS

Bug #3: processPlayoffEvent Assumes Only One Round Per Event

File: app/models/scoring-calculator.ts:25-136

Issue: The function processes matches based on event.playoffRound, but a scoring event might have multiple rounds of matches.

Current Design:

• One scoring event per round (Quarterfinals event, Semifinals event, Finals event)
• Admin must "Complete Round" separately for each

Problem: If admin creates one event for entire tournament and adds all matches to it, the scoring won't work correctly. We'd need to know which round we're processing.

Actually Not a Bug (By Design): Looking at the UI flow in admin.sports-seasons.$id.events.$eventId.bracket.server.ts:178-180:

// We need to temporarily update the event's playoffRound to the one being completed
await updateScoringEvent(params.eventId, { playoffRound: round });

The system updates the event's playoffRound field when completing each round, so this works.

Potential Issue: What if admin completes rounds out of order? (Completes Finals before Semifinals?)

Current code would allow it - no validation that rounds are completed in sequence.

Impact: Low - admin would have to intentionally do something wrong.

Covered in Future Plans? bracket-expansion.md will address this with template system.

---

Bug #4: No Validation for Completing Rounds Out of Order

File: app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts:144-206

Issue: Admin can complete rounds in any order (Finals before Quarterfinals, etc.)

Current Code:

// Verify all matches in this round are complete
const roundMatches = matches.filter((m) => m.round === round);
const allComplete = roundMatches.every((m) => m.isComplete);
if (!allComplete) {
  return { error: `Not all matches in ${round} are complete` };
}

Only checks if matches in the selected round are complete, not whether previous rounds are complete.

Problem:

1. Admin completes Finals first (assigns 1st and 2nd place)
2. Later completes Semifinals (assigns 3rd and 4th place)
3. Participants now have multiple placement records!

Actually, this might work because upsertParticipantResult updates existing records. But it's confusing and could lead to data inconsistency.

Fix: Add validation to ensure rounds are completed in order (QF → SF → Finals).

Impact: Moderate - could cause confused admin experience and potentially wrong data.

Covered in Future Plans? Not explicitly, but template system might make this clearer.

---

Bug #5: Decimal Point Handling May Cause Precision Issues

File: app/models/playoff-match.ts:125-126, database/schema.ts:participant1Score

Issue: Scores are stored as decimal in database but converted to/from string.

Current Code:

participant1Score: participant1Score?.toString(),
participant2Score: participant2Score?.toString(),

Database Schema:

participant1Score: decimal("participant1_score", { precision: 10, scale: 2 })

Problem: When reading scores back, we get strings: "27.50" not numbers: 27.5

The UI displays: parseFloat(match.participant1Score) but this could show 27.5 instead of 27.50 (minor cosmetic issue).

Impact: Very low - cosmetic only for score display.

Fix: Parse to float when displaying, or use proper decimal type handling.

---

🟢 MINOR ISSUES / CODE QUALITY

Issue #1: Unused Import in complete-round Action

File: app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts:183-191

Code:

const { findSeasonSportsBySportsSeasonId } = await import("~/models/season-sport");
const seasonSports = await findSeasonSportsBySportsSeasonId(params.id);
if (seasonSports.length === 0) {
  return { error: "No fantasy seasons found for this sports season" };
}

Issue: We check if seasonSports exist but never use them. The check is good validation, but we could simplify.

Impact: None - just unnecessary code.

---

Issue #2: Error Swallowing in advanceWinner

File: app/routes/admin.sports-seasons.$id.events.$eventId.bracket.server.ts:124-132

Code:

try {
  await advanceWinner(matchId, winnerId);
} catch (error) {
  // It's ok if advancement fails (e.g., next match already filled)
  console.warn("Could not advance winner:", error);
}

Issue: Silently swallows all advancement errors, including real bugs.

Better Approach:

try {
  await advanceWinner(matchId, winnerId);
} catch (error) {
  // Only ignore "already filled" errors
  if (error instanceof Error && error.message.includes("already has both participants")) {
    console.warn("Match already filled, skipping advancement");
  } else {
    throw error; // Re-throw unexpected errors
  }
}

Impact: Bugs in advancement logic might be hidden.

---

Issue #3: Magic Numbers in Bracket Generation

File: app/models/playoff-match.ts:220-226

Code:

if (match.round === "Quarterfinals") {
  nextRound = "Semifinals";
  // Match 1,2 -> SF1, Match 3,4 -> SF2
  nextMatchNumber = match.matchNumber <= 2 ? 1 : 2;
}

Issue: Hard-coded bracket structure won't work for larger brackets.

Impact: Will be rewritten in bracket-expansion.md anyway.

---

📊 SUMMARY

Severity	Count	Critical?
🔴 Critical	2	Bug #1 (advancement), Bug #2 (scoring rules - design issue)
🟡 Moderate	3	Bugs #3, #4, #5
🟢 Minor	3	Issues #1, #2, #3

Recommendations

Immediate Fixes Needed:

1. ✅ Fix Bug #1 (Winner advancement) - High priority, breaks brackets
2. ✅ Fix Bug #4 (Round order validation) - Prevents data corruption

Design Decisions Needed: 3. Bug #2 (pointsAwarded) - Decide: Remove field or document it's unused? 4. Issue #2 (Error swallowing) - Better error handling

Can Wait for Phase 2.6+ (Bracket Expansion): 5. Bug #3 (multiple rounds) - Will be fixed by template system 6. Bug #5 (decimals) - Cosmetic only 7. Issue #1 (unused validation) - Minor cleanup 8. Issue #3 (magic numbers) - Will be rewritten

Comparison with Plans

From scoring-system.md:

"pointsAwarded: Calculated based on league scoring rules (NOT stored here - calculated on demand)"

Current Implementation: We ARE storing it but not using it. ❌ Not following plan.

From bracket-expansion.md:

"Phase 2.6: Template System Foundation"

Impact on Bugs: Bugs #1 and #3 will be completely rewritten, so fixing them now is optional if Phase 2.6 is coming soon.

Decision Point

Should we fix bugs now or wait for bracket-expansion?

Fix Now:

• Bug #1 (advancement) - 30 min fix
• Bug #4 (round order) - 15 min fix
• Bug #2 decision (remove pointsAwarded field) - Requires migration

Total Time: ~1 hour + migration

Wait for Phase 2.6:

• Bracket expansion rewrites most of this code anyway
• Could waste time fixing code that's about to be replaced

My Recommendation:

1. Fix Bug #4 (round order validation) now - prevents data corruption
2. Document Bug #1 and #2, wait for bracket-expansion to fix properly
3. Make decision on pointsAwarded field (keep but document, or remove entirely)