Only derive feeders where the halving rule actually holds
Review caught that the generic feeder rule was being applied to templates that route by their own logic. It was harmless as dead code; driving the renderer with it made several brackets worse than before. The rule pairs rounds by array order and assumes match n is fed by 2n-1 and 2n. That describes advanceWinnerTemplate, not every bracket: - afl_10's Wildcard Round feeds the Elimination Finals, skipping the round listed next to it, so array order fabricated the entire chain and drew ten wrong connectors contradicting advanceAFLWinner. - fifa_48's Third Place Game sits between the Semifinals and the Finals, so the Finals came out fed by the third place game. Once BracketTreeView filtered the consolation round out, the group had three roots and the whole World Cup bracket rendered with no connectors at all. - ncaa_68 labelled Round of 64 #1/#2 with First Four feeds that advanceFirstFourWinner doesn't use. - nba_20's play-in halves in size but pairs the 7v8 loser with the 9v10 winner. Follow each round's declared feedsInto, and derive edges only where the round halves exactly — the condition under which the generic ceil(n/2) mapping is true. Bespoke transitions that happen to halve are named explicitly. Slots left without a feeder read TBD, which is honest. Dropping those edges sends the group to the fallback, so the fallback now has to keep drawing what those brackets already drew: halving U-shapes by round size, and winner tracing through irregular shapes. Previously it drew nothing, which also silently removed every connector from brackets with no bracketTemplateId. Also from review: - clear-bracket deleted seasonParticipantResults for the entire sports season with no rebuild. That table is keyed by season, not event, so it wiped placements for every other event in the season — permanently zeroing standings on a finalized qualifying season. Delete only the matches and point the admin at Reprocess Bracket, which rebuilds placements correctly. - The clear-bracket form sent confirm=true from a hidden field, making the server's completed-match guard unreachable. It's a checkbox now, so the guard is real, including without JS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnzbrCHoM8ESbtbDamaqFb
This commit is contained in:
parent
89ceee432a
commit
569081fe29
5 changed files with 293 additions and 31 deletions
|
|
@ -12,6 +12,7 @@ import {
|
|||
LLWS_20,
|
||||
SIMPLE_16,
|
||||
NFL_14,
|
||||
BRACKET_TEMPLATES,
|
||||
getBracketTemplate,
|
||||
type BracketTemplate,
|
||||
type ConferenceGroup,
|
||||
|
|
@ -28,6 +29,10 @@ import { GAME_TO_MATCH, MATCH_TO_GAME } from "~/test/fixtures/llws-bracket";
|
|||
interface TestMatch {
|
||||
round: string;
|
||||
matchNumber: number;
|
||||
/** Only the fallback reads these, to trace edges through an unrecognised shape. */
|
||||
winnerId?: string | null;
|
||||
participant1Id?: string | null;
|
||||
participant2Id?: string | null;
|
||||
}
|
||||
|
||||
/** Every match a template defines, as the renderer would receive them. */
|
||||
|
|
@ -311,6 +316,157 @@ describe("computeGroupLayout — standard brackets are unchanged", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("buildFeederMap — templates with routing of their own", () => {
|
||||
// The halving rule describes advanceWinnerTemplate, not every bracket. Inventing it
|
||||
// where it doesn't hold draws confident, wrong connectors and mislabels slots, which
|
||||
// is worse than drawing nothing.
|
||||
|
||||
it("follows feedsInto rather than the order rounds are listed in", () => {
|
||||
// AFL's Wildcard Round feeds the Elimination Finals, skipping the round printed
|
||||
// next to it, so array order would fabricate the whole chain.
|
||||
const afl = BRACKET_TEMPLATES.afl_10;
|
||||
const feeders = buildFeederMap(afl);
|
||||
const fed = [...feeders.entries()].filter(([, pair]) =>
|
||||
pair.some((s) => s.kind === "match")
|
||||
);
|
||||
// Only Preliminary Finals → Grand Final actually halves.
|
||||
expect(fed.map(([key]) => key)).toEqual(["Grand Final#1"]);
|
||||
});
|
||||
|
||||
it("leaves a bye round's slots seeded rather than inventing feeds", () => {
|
||||
// CFP's First Round (4) feeds the Quarterfinals (4) — the top seeds have byes.
|
||||
const feeders = buildFeederMap(BRACKET_TEMPLATES.cfp_12);
|
||||
expect(feeders.get(matchKey("Quarterfinals", 1))).toEqual([
|
||||
{ kind: "seed" },
|
||||
{ kind: "seed" },
|
||||
]);
|
||||
});
|
||||
|
||||
it("leaves the First Four out of the Round of 64", () => {
|
||||
// advanceFirstFourWinner puts each winner in a specific seed slot, not games 1-2.
|
||||
const feeders = buildFeederMap(BRACKET_TEMPLATES.ncaa_68);
|
||||
expect(feeders.get(matchKey("Round of 64", 1))).toEqual([
|
||||
{ kind: "seed" },
|
||||
{ kind: "seed" },
|
||||
]);
|
||||
});
|
||||
|
||||
it("leaves the NBA play-in alone, where a loser feeds forward", () => {
|
||||
// Play-In Round 2 pairs the 7v8 loser with the 9v10 winner, so the round sizes
|
||||
// halve but the winners-only rule still doesn't describe it.
|
||||
const feeders = buildFeederMap(BRACKET_TEMPLATES.nba_20);
|
||||
expect(feeders.get(matchKey("Play-In Round 2", 1))).toEqual([
|
||||
{ kind: "seed" },
|
||||
{ kind: "seed" },
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not route the FIFA final through the third place game", () => {
|
||||
// Third Place Game sits between Semifinals and Finals in round order, so array
|
||||
// order made it the Finals' feeder and left the Finals' second slot empty.
|
||||
const feeders = buildFeederMap(BRACKET_TEMPLATES.fifa_48);
|
||||
expect(feeders.get(matchKey("Finals", 1))).toEqual([
|
||||
{ kind: "match", ref: { round: "Semifinals", matchNumber: 1 }, result: "winner" },
|
||||
{ kind: "match", ref: { round: "Semifinals", matchNumber: 2 }, result: "winner" },
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeGroupLayout — every template still draws connectors", () => {
|
||||
/** Lay a whole template out the way BracketTreeView would. */
|
||||
function layOut(template: BracketTemplate) {
|
||||
const byRound = allMatches(template);
|
||||
const order = template.rounds.map((r) => r.name);
|
||||
// BracketTreeView renders a third place game outside the tree.
|
||||
const rounds = order.filter((r) => r !== "Third Place Game");
|
||||
return computeGroupLayout(rounds, byRound, buildFeederMap(template), order);
|
||||
}
|
||||
|
||||
// A gutter joining a column to one exactly half its size is a plain bracket join and
|
||||
// must always be drawn. Where the sizes don't halve — a bye round, a play-in, the
|
||||
// First Four — the routing is bespoke and nothing is drawn until the games decide it,
|
||||
// which is what these brackets did before.
|
||||
it.each(Object.keys(BRACKET_TEMPLATES).filter((id) => id !== "llws_20"))(
|
||||
"%s draws every gutter that halves",
|
||||
(id) => {
|
||||
const layout = layOut(BRACKET_TEMPLATES[id]);
|
||||
const gutters = new Set(layout.edges.map((e) => e.fromColumn));
|
||||
let halvingGutters = 0;
|
||||
for (let ci = 0; ci < layout.columns.length - 1; ci++) {
|
||||
const from = layout.columns[ci].matches.length;
|
||||
const to = layout.columns[ci + 1].matches.length;
|
||||
if (from !== to * 2) continue;
|
||||
halvingGutters += 1;
|
||||
expect(gutters).toContain(ci);
|
||||
}
|
||||
// Every template has at least one, so a template that lost all its lines fails.
|
||||
expect(halvingGutters).toBeGreaterThan(0);
|
||||
}
|
||||
);
|
||||
|
||||
it("keeps the FIFA bracket a single tree once the third place game is set aside", () => {
|
||||
const layout = layOut(BRACKET_TEMPLATES.fifa_48);
|
||||
expect(layout.columns.map((c) => c.label)).toEqual([
|
||||
"Round of 32",
|
||||
"Round of 16",
|
||||
"Quarterfinals",
|
||||
"Semifinals",
|
||||
"Finals",
|
||||
]);
|
||||
expect(layout.edges).toHaveLength(30);
|
||||
});
|
||||
|
||||
// llws_20 is excluded above because both sides in one group is genuinely not a tree;
|
||||
// it renders per side, which the tests further up cover.
|
||||
});
|
||||
|
||||
describe("computeGroupLayout — fallback keeps the old connectors", () => {
|
||||
const rounds = ["Quarterfinals", "Semifinals", "Finals"];
|
||||
const byRound = new Map<string, TestMatch[]>([
|
||||
["Quarterfinals", [1, 2, 3, 4].map((n) => ({ round: "Quarterfinals", matchNumber: n }))],
|
||||
["Semifinals", [1, 2].map((n) => ({ round: "Semifinals", matchNumber: n }))],
|
||||
["Finals", [{ round: "Finals", matchNumber: 1 }]],
|
||||
]);
|
||||
|
||||
it("infers halving edges when there is no feeder map at all", () => {
|
||||
// A bracket with no template id, which SportSeasonDisplay renders.
|
||||
const layout = computeGroupLayout(rounds, byRound, new Map(), rounds);
|
||||
expect(layout.edges).toHaveLength(6);
|
||||
// Quarterfinals 1 and 2 both join Semifinal 1.
|
||||
const intoFirstSemi = layout.edges.filter((e) => e.toCenter === 1);
|
||||
expect(intoFirstSemi.map((e) => e.fromCenter)).toEqual([0.5, 1.5]);
|
||||
});
|
||||
|
||||
it("traces played winners when the shape is not a halving", () => {
|
||||
const irregular = new Map<string, TestMatch[]>([
|
||||
[
|
||||
"Wildcard",
|
||||
[
|
||||
{ round: "Wildcard", matchNumber: 1, winnerId: "a" },
|
||||
{ round: "Wildcard", matchNumber: 2, winnerId: "b" },
|
||||
],
|
||||
],
|
||||
[
|
||||
"Semifinals",
|
||||
[
|
||||
{ round: "Semifinals", matchNumber: 1, participant1Id: "seeded", participant2Id: "b" },
|
||||
{ round: "Semifinals", matchNumber: 2, participant1Id: "seeded2", participant2Id: "a" },
|
||||
],
|
||||
],
|
||||
]);
|
||||
const layout = computeGroupLayout(
|
||||
["Wildcard", "Semifinals"],
|
||||
irregular,
|
||||
new Map(),
|
||||
["Wildcard", "Semifinals"]
|
||||
);
|
||||
// b won Wildcard 2 (centre 1.5) and plays Semifinal 1 (centre 0.5) — a crossing
|
||||
// edge that only the actual result can reveal.
|
||||
expect(layout.edges).toContainEqual({ fromColumn: 0, fromCenter: 1.5, toCenter: 0.5 });
|
||||
expect(layout.edges).toContainEqual({ fromColumn: 0, fromCenter: 0.5, toCenter: 1.5 });
|
||||
});
|
||||
});
|
||||
|
||||
describe("describeSlotSource", () => {
|
||||
const feeders = buildFeederMap(LLWS_20);
|
||||
const sourcesFor = (game: number): [SlotSource, SlotSource] => {
|
||||
|
|
|
|||
|
|
@ -47,6 +47,16 @@ export type FeederMap = Map<string, [SlotSource, SlotSource]>;
|
|||
|
||||
const SEED: SlotSource = { kind: "seed" };
|
||||
|
||||
/**
|
||||
* `template.id:roundName` for transitions routed by a dedicated advancement function
|
||||
* rather than advanceWinnerTemplate's ceil(n/2) rule, and whose round sizes happen to
|
||||
* halve so the check in buildFeederMap can't rule them out on shape alone.
|
||||
*
|
||||
* The NBA play-in is the case: Play-In Round 2 pairs the 7v8 *loser* with the 9v10
|
||||
* winner (advanceNBAPlayInWinner), which no winners-only halving describes.
|
||||
*/
|
||||
const BESPOKE_TRANSITIONS = new Set(["nba_20:Play-In Round 1"]);
|
||||
|
||||
export function matchKey(round: string, matchNumber: number): string {
|
||||
return `${round}#${matchNumber}`;
|
||||
}
|
||||
|
|
@ -94,22 +104,34 @@ export function buildFeederMap(template: BracketTemplate | undefined): FeederMap
|
|||
return feeders;
|
||||
}
|
||||
|
||||
for (let ri = 1; ri < template.rounds.length; ri++) {
|
||||
const round = template.rounds[ri];
|
||||
const prev = template.rounds[ri - 1];
|
||||
// Follow each round's declared `feedsInto` rather than array order — AFL's Wildcard
|
||||
// Round feeds the Elimination Finals, skipping the round printed next to it.
|
||||
for (const prev of template.rounds) {
|
||||
if (!prev.feedsInto) continue;
|
||||
const round = template.rounds.find((r) => r.name === prev.feedsInto);
|
||||
if (!round) continue;
|
||||
|
||||
// advanceWinnerTemplate sends match n to ceil(n/2) in the next round, slot by
|
||||
// parity. That describes the bracket only where the round halves exactly; a
|
||||
// play-in, a bye round, or a First Four routes by rules of its own, and inventing
|
||||
// a halving there would draw connectors and slot labels that are simply wrong.
|
||||
// Leaving those edges out drops the group to computeGroupLayout's fallback, which
|
||||
// is the geometry these brackets already had.
|
||||
if (prev.matchCount !== round.matchCount * 2) continue;
|
||||
if (BESPOKE_TRANSITIONS.has(`${template.id}:${prev.name}`)) continue;
|
||||
|
||||
for (let n = 1; n <= round.matchCount; n++) {
|
||||
const pair = slots(matchKey(round.name, n));
|
||||
for (const [slotIdx, source] of [
|
||||
[0, 2 * n - 1],
|
||||
[1, 2 * n],
|
||||
] as const) {
|
||||
if (source > prev.matchCount) continue;
|
||||
pair[slotIdx] = {
|
||||
pair[0] = {
|
||||
kind: "match",
|
||||
ref: { round: prev.name, matchNumber: source },
|
||||
ref: { round: prev.name, matchNumber: 2 * n - 1 },
|
||||
result: "winner",
|
||||
};
|
||||
pair[1] = {
|
||||
kind: "match",
|
||||
ref: { round: prev.name, matchNumber: 2 * n },
|
||||
result: "winner",
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -195,6 +217,10 @@ export interface BracketLayout<M> {
|
|||
interface PositionedMatch {
|
||||
round: string;
|
||||
matchNumber: number;
|
||||
/** Only read by the fallback, to trace edges through an unrecognised shape. */
|
||||
winnerId?: string | null;
|
||||
participant1Id?: string | null;
|
||||
participant2Id?: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -328,8 +354,10 @@ export function computeGroupLayout<M extends PositionedMatch>(
|
|||
}
|
||||
|
||||
/**
|
||||
* The previous behaviour: one column per round, matches spread evenly, no edges.
|
||||
* Used when a group's shape can't be resolved into a single tree.
|
||||
* The previous behaviour, kept for groups whose shape can't be resolved into a single
|
||||
* tree: one column per round, matches spread evenly over it, and edges inferred from the
|
||||
* round sizes. Brackets with bespoke routing (AFL, CFP byes, a bracket with no template)
|
||||
* land here, so it has to keep drawing what they drew before rather than nothing.
|
||||
*/
|
||||
function fallbackLayout<M extends PositionedMatch>(
|
||||
visibleRounds: string[],
|
||||
|
|
@ -339,13 +367,54 @@ function fallbackLayout<M extends PositionedMatch>(
|
|||
...visibleRounds.map((r) => matchesByRound.get(r)?.length ?? 0),
|
||||
1
|
||||
);
|
||||
const centersFor = (matches: M[]) => {
|
||||
const span = leafCount / Math.max(matches.length, 1);
|
||||
return matches.map((_, i) => (i + 0.5) * span);
|
||||
};
|
||||
|
||||
const columns = visibleRounds.map((round) => {
|
||||
const matches = matchesByRound.get(round) ?? [];
|
||||
const span = leafCount / Math.max(matches.length, 1);
|
||||
const centers = centersFor(matches);
|
||||
return {
|
||||
label: round,
|
||||
matches: matches.map((match, i) => ({ match, center: (i + 0.5) * span })),
|
||||
matches: matches.map((match, i) => ({ match, center: centers[i] })),
|
||||
};
|
||||
});
|
||||
return { columns, leafCount, edges: [] };
|
||||
|
||||
const edges: BracketLayout<M>["edges"] = [];
|
||||
for (let ci = 0; ci < columns.length - 1; ci++) {
|
||||
const from = columns[ci].matches;
|
||||
const to = columns[ci + 1].matches;
|
||||
|
||||
if (to.length === Math.ceil(from.length / 2) && from.length > 1) {
|
||||
// A halving: matches 2k and 2k+1 feed match k.
|
||||
for (let k = 0; k < to.length; k++) {
|
||||
for (const idx of [2 * k, 2 * k + 1]) {
|
||||
if (idx >= from.length) continue;
|
||||
edges.push({
|
||||
fromColumn: ci,
|
||||
fromCenter: from[idx].center,
|
||||
toCenter: to[k].center,
|
||||
});
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
// Otherwise the only thing that can be known is where a winner actually went, so
|
||||
// nothing is drawn until the games are played.
|
||||
const winnerToCenter = new Map<string, number>();
|
||||
for (const { match, center } of from) {
|
||||
if (match.winnerId) winnerToCenter.set(match.winnerId, center);
|
||||
}
|
||||
for (const { match, center } of to) {
|
||||
for (const id of [match.participant1Id, match.participant2Id]) {
|
||||
const fromCenter = id ? winnerToCenter.get(id) : undefined;
|
||||
if (fromCenter === undefined) continue;
|
||||
edges.push({ fromColumn: ci, fromCenter, toCenter: center });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return { columns, leafCount, edges };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -68,7 +68,7 @@ describe("clear-bracket", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("deletes the matches and the placements derived from them", async () => {
|
||||
it("deletes the matches", async () => {
|
||||
vi.mocked(findPlayoffMatchesByEventId).mockResolvedValue([
|
||||
match(false),
|
||||
match(false),
|
||||
|
|
@ -78,7 +78,20 @@ describe("clear-bracket", () => {
|
|||
|
||||
expect(result.success).toContain("2 match(es) removed");
|
||||
expect(deletePlayoffMatchesByEventId).toHaveBeenCalledWith("event-1");
|
||||
expect(deleteParticipantResultsBySportsSeasonId).toHaveBeenCalledWith("season-1");
|
||||
});
|
||||
|
||||
it("leaves placements alone — they belong to the whole season, not this event", async () => {
|
||||
// seasonParticipantResults is keyed by sports season, so deleting here would wipe
|
||||
// every other event's placements with nothing to rebuild them. Reprocess Bracket is
|
||||
// the tool that rebuilds them correctly.
|
||||
vi.mocked(findPlayoffMatchesByEventId).mockResolvedValue([
|
||||
match(true),
|
||||
] as unknown as Awaited<ReturnType<typeof findPlayoffMatchesByEventId>>);
|
||||
|
||||
const result = await run(clearRequest("true"));
|
||||
|
||||
expect(deleteParticipantResultsBySportsSeasonId).not.toHaveBeenCalled();
|
||||
expect(result.success).toContain("Reprocess Bracket");
|
||||
});
|
||||
|
||||
it("refuses to discard completed matches without confirmation", async () => {
|
||||
|
|
@ -91,7 +104,6 @@ describe("clear-bracket", () => {
|
|||
|
||||
expect(result.error).toContain("1 completed match(es)");
|
||||
expect(deletePlayoffMatchesByEventId).not.toHaveBeenCalled();
|
||||
expect(deleteParticipantResultsBySportsSeasonId).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("discards completed matches once confirmed", async () => {
|
||||
|
|
|
|||
|
|
@ -310,13 +310,20 @@ export async function action({ request, params }: Route.ActionArgs) {
|
|||
}
|
||||
|
||||
await deletePlayoffMatchesByEventId(params.eventId);
|
||||
// Placements were derived from the matches just deleted; leaving them behind would
|
||||
// keep stale points on the standings.
|
||||
await deleteParticipantResultsBySportsSeasonId(event.sportsSeasonId);
|
||||
await recalculateAffectedLeagues(event.sportsSeasonId, database(), { skipDiscord: true });
|
||||
|
||||
// Placements are deliberately left alone. seasonParticipantResults is keyed by
|
||||
// sports season, not by event, so a season-wide delete here would wipe the
|
||||
// placements of every other event in the season with nothing to rebuild them —
|
||||
// and on a finalized qualifying season that means permanently zeroed standings.
|
||||
// Reprocess Bracket already rebuilds placements correctly, qualifying path
|
||||
// included, so point the admin at it once the new bracket is in place.
|
||||
const note =
|
||||
completed > 0
|
||||
? " Run Reprocess Bracket after rebuilding to clear the placements those results produced."
|
||||
: "";
|
||||
|
||||
return {
|
||||
success: `Bracket cleared (${existing.length} match(es) removed). Set it up again below.`,
|
||||
success: `Bracket cleared (${existing.length} match(es) removed). Set it up again below.${note}`,
|
||||
};
|
||||
} catch (error) {
|
||||
logger.error("Error clearing bracket:", error);
|
||||
|
|
|
|||
|
|
@ -622,17 +622,19 @@ export default function EventBracket({
|
|||
<CardTitle>Clear Bracket</CardTitle>
|
||||
<CardDescription>
|
||||
Delete every match in this bracket so it can be set up again from
|
||||
scratch. Use this when the wrong participants were seeded. This also
|
||||
clears the season's placements and the points derived from them.
|
||||
scratch. Use this when the wrong participants were seeded. Placements
|
||||
are left alone — run Reprocess Bracket after rebuilding to clear any
|
||||
that the discarded results produced.
|
||||
</CardDescription>
|
||||
</CardHeader>
|
||||
<CardContent>
|
||||
<Form
|
||||
method="post"
|
||||
className="space-y-3"
|
||||
onSubmit={(e) => {
|
||||
if (
|
||||
!confirm(
|
||||
`Delete all ${matches.length} match(es) in this bracket? Recorded results and placements will be lost.`
|
||||
`Delete all ${matches.length} match(es) in this bracket? Recorded results will be lost.`
|
||||
)
|
||||
) {
|
||||
e.preventDefault();
|
||||
|
|
@ -640,7 +642,23 @@ export default function EventBracket({
|
|||
}}
|
||||
>
|
||||
<input type="hidden" name="intent" value="clear-bracket" />
|
||||
<input type="hidden" name="confirm" value="true" />
|
||||
{/* The server refuses to discard completed matches unless this is
|
||||
checked. Sending it unconditionally from a hidden field would make
|
||||
that guard unreachable, including for a submit without JS. */}
|
||||
{matches.some((m: { isComplete: boolean }) => m.isComplete) && (
|
||||
<div className="flex items-center gap-2">
|
||||
<input
|
||||
type="checkbox"
|
||||
id="confirm-clear-bracket"
|
||||
name="confirm"
|
||||
value="true"
|
||||
className="h-4 w-4"
|
||||
/>
|
||||
<Label htmlFor="confirm-clear-bracket" className="font-normal">
|
||||
Yes, discard the results already recorded in this bracket
|
||||
</Label>
|
||||
</div>
|
||||
)}
|
||||
<Button type="submit" variant="destructive">
|
||||
Clear Bracket
|
||||
</Button>
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue