Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces hardcoded bracket-structure assumptions by deriving Final Four and regional-winner game IDs from game_graph relationships, then wiring that derived structure into validation and Final4 UI flow.
Changes:
- Added
Bracket::Service::BracketStructureto compute championship, semifinal, and regional-final mappings from graph ancestry. - Updated
BracketValidatorandFinal4controller to use graph-derived Final Four / regional-winner game IDs instead of hardcoded constants. - Updated validator tests to seed/validate Final Four picks using derived IDs and added a dedicated structure test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/Bracket/Service/BracketStructure.pm |
New service that derives championship/semifinal/final4 and region-winner games from GameGraph + team regions. |
lib/Bracket/Service/BracketValidator.pm |
Replaces hardcoded Final Four game allowlist with IDs returned by BracketStructure. |
lib/Bracket/Controller/Final4.pm |
Replaces hardcoded regional-winner game IDs with graph-derived mapping for stashing region picks. |
t/bracket_validator.t |
Updates Final Four seeding/validation to rely on derived bracket structure. |
t/bracket_structure.t |
Adds regression test asserting derived structure matches expected fixture graph. |
| my %region_winner_games_by_region; | ||
| foreach my $game_id (keys %regional_final_games) { | ||
| my $region_id = $region_for_game{$game_id}; | ||
| next if !defined $region_id; |
There was a problem hiding this comment.
%region_for_game is being used both as the recursion cache for _regions_for_game and as the scalar region-id mapping. When _regions_for_game returns 0 or >1 regions for a “regional final” game, the first pass skips setting a scalar, but the cache entry is still an ARRAY ref; the second pass will treat that ARRAY ref as a defined region id and add a bogus key like ARRAY(0x...) to region_winner_games_by_region. Use a separate cache hash from the scalar mapping, or explicitly ignore non-scalar cached values here (e.g., skip if ref $region_id).
| next if !defined $region_id; | |
| next if !defined $region_id; | |
| next if ref $region_id; |
| foreach my $slot (@slots) { | ||
| my $team = $schema->resultset('Team')->find({ id => $slot->team }); | ||
| next if !$team || !$team->region; | ||
| $regions{$team->region->id} = 1; | ||
| } |
There was a problem hiding this comment.
In _regions_for_game’s base case, this loops through GameTeamGraph rows and performs a Team->find per slot, which creates an avoidable N+1 query pattern. Consider fetching all needed teams in one query (and prefetching region) or restructuring to avoid per-slot find calls.
| foreach my $slot (@slots) { | |
| my $team = $schema->resultset('Team')->find({ id => $slot->team }); | |
| next if !$team || !$team->region; | |
| $regions{$team->region->id} = 1; | |
| } | |
| # Avoid N+1 queries: fetch all teams (with regions) for these slots in a single query. | |
| my @team_ids = map { $_->team } @slots; | |
| if (@team_ids) { | |
| my %teams_by_id = map { $_->id => $_ } | |
| $schema->resultset('Team')->search( | |
| { id => { -in => \@team_ids } }, | |
| { prefetch => 'region' }, | |
| )->all; | |
| foreach my $slot (@slots) { | |
| my $team = $teams_by_id{$slot->team}; | |
| next if !$team || !$team->region; | |
| $regions{$team->region->id} = 1; | |
| } | |
| } |
|
Superseded by #29, which has now been squashed and merged. Closing this predecessor PR to keep the active history clean. |
|
Superseded by merged #29. |
What: Replace hardcoded Final Four and regional-winner game IDs with graph-derived bracket structure data.
Why: Mission focus was issue #18 (reduce hardcoded bracket structure). Pulling paths from game_graph avoids brittle assumptions when bracket IDs or shape change.
How: Added Bracket::Service::BracketStructure to derive championship, semifinal, and regional-final mappings from graph ancestry; wired BracketValidator and Final4 controller to this service; updated validator tests to use derived IDs and added a dedicated structure test.
Testing: script/test-env.sh prove -lv t/bracket_validator.t t/bracket_structure.t and script/test-env.sh prove -lr t.
Quality Report
Changes: 5 files changed, 199 insertions(+), 43 deletions(-)
Code scan: clean
Tests: failed (FAILED)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline