no-vacation support for tournaments

[GreenAsJade]

Fixes complaining about vacation effects on tournaments

Proposed Changes

• Allow TDs to set "no vacation" on their correspondence tournament
• Alert users in sensible places that this is the case
• Test for these

Needs: https://github.com/online-go/ogs/pull/2345

[github-actions]

In VacationSettings.tsx, the parameterized plural string uses .replace() instead of interpolate():

// current
ngettext(
    "You have {{count}} active correspondence game that will not be paused by vacation:",
    "You have {{count}} active correspondence games that will not be paused by vacation:",
    no_vacation_games.length,
).replace("{{count}}", String(no_vacation_games.length))

// should be
interpolate(
    ngettext(
        "You have {{count}} active correspondence game that will not be paused by vacation:",
        "You have {{count}} active correspondence games that will not be paused by vacation:",
        no_vacation_games.length,
    ),
    { count: no_vacation_games.length },
)

The rest of the codebase uses interpolate(ngettext(...), ...) for this (e.g. src/views/Prizes/SponsorshipRequestTypes.ts), and the project guidelines require interpolate() for all parameterized strings so that i18n tooling handles substitution consistently.

[github-actions]

Review notes

Tournament.tsx — checkbox may be uncontrolled for existing tournaments

When an existing tournament is loaded and the API response does not include a no_vacation field (e.g. pre-existing tournaments created before this feature), tournament.no_vacation will be undefined at runtime. The checkbox then receives checked={undefined}, making it an uncontrolled input. React will warn about the uncontrolled-to-controlled transition if the user later edits and checks the box.

Fix: checked={tournament.no_vacation ?? false}

---

VacationSettings.tsx — ui/overview is a heavyweight call for a narrow query

The component fetches ui/overview on every mount solely to find games with no_vacation set. This endpoint returns all active game data for the user — potentially a large payload — when only a small subset of fields on a subset of games is needed. If this endpoint is already called and cached elsewhere on the page, that data could be reused; otherwise a more targeted query would be more efficient.

[github-actions]

Review

Two issues worth addressing before merge:

---

1. Missing fetch cleanup in VacationSettings.tsx

The new useEffect that calls get("ui/overview") doesn't return a cleanup function. If the user navigates away from the Vacation settings tab while the request is in-flight, set_no_vacation_games will fire on an unmounted component. Home.tsx and Settings.tsx both follow the pattern of aborting this request on cleanup:

React.useEffect(() => {
    get("ui/overview")
        .then(...)
        .catch(errorAlerter);
    return () => {
        abort_requests_in_flight("ui/overview");
    };
}, []);

---

2. no_vacation and time_per_move missing from rest_api.players.full.Game

Home.tsx types ui/overview's active_games as rest_api.players.full.Game[], and that interface only declares { black, white, width, height, json, id, name } — it has no no_vacation or time_per_move fields. VacationSettings.tsx works around this with an inline type override, but the fields should be added to the model in src/models/games.d.ts so TypeScript can catch future regressions:

interface Game {
    // existing fields...
    no_vacation?: boolean;
    time_per_move?: number;
}

Without this, TypeScript won't warn if these fields are renamed or removed from the API response.

[github-actions]

Bug: GameInfoModal.css — warning text invisible in dark mode

src/views/Game/GameInfoModal.css:

.no-vacation-warning {
    color: var(--warning-bg);  /* bug */
    font-weight: bold;
}

--warning-bg in dark mode is #925a00 — a dark brownish-orange that is nearly invisible against a dark modal background. The Settings.css version of this class correctly pairs background-color: var(--warning-bg) with color: var(--warning-fg). For the inline text-only variant used in GameInfoModal, a separate text-appropriate color (e.g. an orange/amber from the palette, or a dedicated CSS variable) should be used instead of reusing the background-color variable.

---

Type issue: TournamentInterface.no_vacation should be optional

src/views/Tournament/Tournament.tsx:

no_vacation: boolean;   // required — but field may be absent in API response for existing tournaments

Existing tournaments loaded from the backend will not have no_vacation in their API response, making the runtime value undefined while the type says boolean. Making it no_vacation?: boolean (matching how no_vacation is declared in src/models/games.d.ts) accurately reflects the API contract and prevents any code down the line from incorrectly assuming the value is always a concrete boolean.

[github-actions]

Code Review

GameInfoModal no-vacation warning may never display

In GameDock.tsx::showGameInfo() (line 172-181), certain fields are explicitly copied from goban.engine.config to goban.config before the modal is opened -- komi, rules, handicap, handicap_rank_difference, rengo, rengo_teams -- but no_vacation is not in this list.

If the server sends no_vacation on goban.engine.config (via the socket game data) but not directly on goban.config, the check in GameInfoModal.tsx:

(config as GobanConfig & { no_vacation?: boolean }).no_vacation

will always be undefined and the warning will silently never appear. The type cast is required precisely because no_vacation is not in the GobanConfig type, which makes this easy to overlook.

Worth explicitly adding no_vacation to the copy list in GameDock.tsx to be safe, mirroring the established pattern already used for rengo and handicap.

[github-actions]

Code Review Notes

Inconsistency between GameInfoModal and VacationSettings no-vacation guards

VacationSettings correctly filters games with time_per_move >= 3600 so the warning only appears for correspondence games:

// VacationSettings.tsx
const games = result.active_games.filter(
    (g) => g.no_vacation && (g.time_per_move ?? 0) >= 3600,
);

But GameInfoModal has no such guard — it shows "Vacation is disabled for this game" for any game where no_vacation is truthy, regardless of time control:

// GameInfoModal.tsx
{(config as GobanConfig & { no_vacation?: boolean }).no_vacation && (
    <dd className="no-vacation-warning">
        Vacation is disabled for this game. The clock will not pause for vacation.
    </dd>
)}

If a non-correspondence game ever has no_vacation: true on its config (e.g., because a TD checked the box before realising it only applies to correspondence tournaments — see below), this modal will display a warning that is technically incorrect: vacation never pauses live/blitz clocks anyway.

---

no_vacation checkbox is shown unconditionally in the tournament form

The new checkbox is added to the settings table without any condition on the tournament's speed. Since vacation has no effect on live or blitz game clocks, enabling no_vacation on a non-correspondence tournament would silently do nothing from the player's perspective (correctly suppressed by the time_per_move filter in VacationSettings), but would still cause GameInfoModal to display the misleading "Vacation is disabled" warning on those games (per the issue above).

Hiding or disabling the checkbox when the selected speed is not correspondence would prevent TDs from enabling it unintentionally and would eliminate the inconsistency with GameInfoModal.

[github-actions]

Code Review

Bug: Spurious error alert on navigation in VacationSettings.tsx

In the new useEffect that fetches ui/overview, when the user navigates away from the Settings page while the request is still in-flight, the cleanup calls abort_requests_in_flight which aborts the fetch and causes the Promise to reject with an AbortError. That rejection flows into .catch(errorAlerter), showing a spurious 'An error has occurred' alert during normal navigation.

requests.ts line 174 confirms AbortErrors are not suppressed — they are re-rejected to callers. errorAlerter has no special handling for AbortError either.

The pattern used in Settings.tsx (a canceled flag checked before calling setState/errorAlerter) avoids this. The simplest fix is to filter the abort in the catch handler, e.g. checking err?.name !== 'AbortError' before calling errorAlerter.

[github-actions]

Code Review

VacationSettings.tsx — abort_requests_in_flight in cleanup may cancel unrelated requests

The cleanup function calls abort_requests_in_flight("ui/overview"), which globally cancels all in-flight requests to that endpoint — including any initiated by other components (Home.tsx also fetches ui/overview). The canceled = true flag is already sufficient to prevent stale state updates on unmount; the abort_requests_in_flight call is redundant and potentially harmful if both pages happen to be in flight simultaneously.

---

Tournament.tsx — no_vacation not cleared when time control speed changes

setTimeControl only updates time_control_parameters without resetting no_vacation. If a user enables no-vacation on a correspondence tournament and then switches the speed to live (hiding the checkbox), submitting the form will still send no_vacation: true for a non-correspondence tournament.

Fixing it would be a one-liner in setTimeControl:

const setTimeControl = (tc: TimeControl) => {
    setTournament({
        ...tournament,
        time_control_parameters: tc,
        no_vacation: tc.speed === "correspondence" ? tournament.no_vacation : false,
    });
};

[github-actions]

Review

One issue worth addressing:

VacationSettings.tsx — global abort for a component-scoped request

The cleanup for the new useEffect calls abort_requests_in_flight("ui/overview"), which cancels all in-flight requests to that URL globally, including any made by Home.tsx. The canceled boolean flag already correctly prevents stale state updates after unmount, so the abort is both redundant and a latent hazard — if routing ever renders both components simultaneously, the Settings unmount would silently cancel the Home page's overview fetch.

The safest minimal fix is to drop the abort_requests_in_flight call entirely and rely solely on the canceled flag, which already handles the unmount case correctly. If request cancellation is desired for bandwidth reasons, the correct scoping is via an AbortController instance rather than the URL-keyed global mechanism.

[GreenAsJade]

Claude reckons that abort is consistent with how the rest of Settings does it. Plus Home and Vacation pane can't be mounted at the same time.

[github-actions]

Code Review

Found one real bug:

Bug: wrong CSS variable for warning text color in GameInfoModal.css

The new .disable-vacation-warning rule in src/views/Game/GameInfoModal.css sets color: var(--warning-bg). The --warning-bg variable is #ffae2c in light mode and #925a00 (dark brown) in dark mode, so in dark mode the warning text is dark brown on a dark background — effectively invisible.

The Settings.css version of the same class correctly uses background-color: var(--warning-bg) and color: var(--warning-fg). The GameInfoModal.css fix is either:

• change color: var(--warning-bg) → color: var(--warning-fg), or
• add background-color: var(--warning-bg) and change the text to var(--warning-fg) to match the Settings panel style.

[github-actions]

Code Review

VacationSettings.tsx — abort_requests_in_flight("ui/overview") is too broad

The cleanup function in the new useEffect calls abort_requests_in_flight("ui/overview"), which cancels all in-flight requests to that URL globally — not just the one initiated by this component. Home.tsx also fetches ui/overview and could have a request in flight when VacationSettings unmounts (e.g. the user navigates from /user/settings to /). This could silently abort Home's refresh, causing it to show stale data or stay in a loading state.

The canceled = true flag already correctly prevents the post-unmount state update, which is the important invariant to enforce. The abort_requests_in_flight call is redundant for correctness and only serves as a network optimization — but at the cost of potentially aborting an unrelated component's request. Consider removing it, or switching to a per-request AbortController so only this component's request is cancelled.

---

GameDock.tsx — any cast added for goban.engine.config

(goban.config as any)[k] = (goban.engine.config as any)[k];

disable_vacation was correctly added to rest_api.players.full.Game in games.d.ts, but GobanConfig (or the engine config type) was not updated, requiring the any cast here. The project rules prohibit any types. The GameInfoModal.tsx handles this more explicitly with a type intersection (config as GobanConfig & { disable_vacation?: boolean }), which is the safer pattern. Applying the same intersection to the engine config read in GameDock would avoid the unchecked any.

[github-actions]

Code Review

One issue found in the new code:

VacationSettings.tsx — redundant abort_requests_in_flight call

In the new useEffect cleanup:

return () => {
    canceled = true;
    abort_requests_in_flight("ui/overview");
};

The canceled = true flag already prevents any stale state update after unmount, making the abort_requests_in_flight call redundant. More importantly, this function globally cancels all in-flight requests matching that URL — not just this component's request. If another component (e.g. Home.tsx, which also fetches ui/overview) has a concurrent request when this panel unmounts, it will be silently aborted.

Since canceled already guards correctness here, the abort_requests_in_flight line can simply be removed.

[github-actions]

Reviewed for bugs and performance issues — nothing significant found.

The implementation looks correct:

• abort_requests_in_flight("ui/overview") in the VacationSettings cleanup is consistent with the existing pattern in Home.tsx, and since these are on separate routes there is no cross-component interference.
• The time_per_move >= 3600 client-side filter is the correct threshold for correspondence speed.
• The disable_vacation state reset in setTimeControl (when speed changes away from correspondence) is handled correctly.
• CSS scoping via PostCSS nesting means the shared .disable-vacation-warning class name in both Settings.css and GameInfoModal.css does not conflict.
• Optional field types with appropriate fallbacks (?? 0, g.name || '#' + g.id) handle missing server data gracefully.