Replay archived games from older builds via versioned CDN shells - #4959
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughArchived games can use commit-specific replay shells. The client detects replay-shell URLs, redirects archived games when a matching CDN shell exists, and skips Turnstile for replay flows. Deployment renders, validates, and uploads versioned shells. ChangesVersioned replay support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant JoinLobbyModal
participant CDN
participant Browser
participant Main
JoinLobbyModal->>JoinLobbyModal: Check archived game commit
JoinLobbyModal->>CDN: Probe versioned replay shell
CDN-->>JoinLobbyModal: Return successful text/html response
JoinLobbyModal->>Browser: Navigate to versioned shell
Browser->>Main: Load replay-shell game URL
Main->>JoinLobbyModal: Open join modal
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
73a06bc to
3212574
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 1196-1207: Update the CDN probe in checkArchivedGame to use an
AbortController with a short timeout, pass its signal to the HEAD fetch, and
clean up the timer afterward. Treat timeout-triggered aborts like other probe
failures by returning false so the existing version-mismatch flow continues.
In `@src/client/Main.ts`:
- Around line 823-832: Update the hash-based join flow around the joinMatch
handling and its downstream handleJoinLobby/game-start URL updates so versioned
replay pages detected by isVersionedReplayPage(window.location.pathname)
preserve the existing versioned shell pathname and `#join`=<gameId> hash instead
of replacing the URL with /game/<gameId>; retain the current /game/<gameId>
behavior for non-versioned pages.
In `@src/server/RenderStaticIndex.ts`:
- Around line 12-17: Update the error handling callback in RenderStaticIndex to
pass the user-visible “Failed to render static index:” message through the
repository’s translateText() helper, adding the required import and preserving
the existing error output and exit behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfad9503-5b16-4c0b-999d-43adde56a969
📒 Files selected for processing (6)
src/client/JoinLobbyModal.tssrc/client/Main.tssrc/client/VersionedReplay.tssrc/server/RenderStaticIndex.tstests/VersionedReplay.test.tsupdate.sh
| try { | ||
| const probe = await fetch(url.split("#")[0], { method: "HEAD" }); | ||
| if (!probe.ok) { | ||
| return false; | ||
| } | ||
| const contentType = probe.headers.get("content-type") ?? ""; | ||
| if (!contentType.includes("text/html")) { | ||
| return false; | ||
| } | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the CDN shell probe.
fetch() has no default timeout. If the CDN connection stalls, checkArchivedGame() never returns and the join modal stays in its connecting state.
Abort the HEAD request after a short timeout. Return false after the abort so the existing version-mismatch flow can continue.
Proposed fix
+ const controller = new AbortController();
+ const timeoutId = window.setTimeout(() => controller.abort(), 5000);
try {
- const probe = await fetch(url.split("#")[0], { method: "HEAD" });
+ const probe = await fetch(url.split("#")[0], {
+ method: "HEAD",
+ signal: controller.signal,
+ });
if (!probe.ok) {
return false;
}
const contentType = probe.headers.get("content-type") ?? "";
if (!contentType.includes("text/html")) {
return false;
}
} catch {
return false;
+ } finally {
+ clearTimeout(timeoutId);
}📝 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.
| try { | |
| const probe = await fetch(url.split("#")[0], { method: "HEAD" }); | |
| if (!probe.ok) { | |
| return false; | |
| } | |
| const contentType = probe.headers.get("content-type") ?? ""; | |
| if (!contentType.includes("text/html")) { | |
| return false; | |
| } | |
| } catch { | |
| return false; | |
| } | |
| const controller = new AbortController(); | |
| const timeoutId = window.setTimeout(() => controller.abort(), 5000); | |
| try { | |
| const probe = await fetch(url.split("#")[0], { | |
| method: "HEAD", | |
| signal: controller.signal, | |
| }); | |
| if (!probe.ok) { | |
| return false; | |
| } | |
| const contentType = probe.headers.get("content-type") ?? ""; | |
| if (!contentType.includes("text/html")) { | |
| return false; | |
| } | |
| } catch { | |
| return false; | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/JoinLobbyModal.ts` around lines 1196 - 1207, Update the CDN probe
in checkArchivedGame to use an AbortController with a short timeout, pass its
signal to the HEAD fetch, and clean up the timer afterward. Treat
timeout-triggered aborts like other probe failures by returning false so the
existing version-mismatch flow continues.
| renderHtmlContent(path.join(__dirname, "../../static/index.html")).then( | ||
| (html) => process.stdout.write(html), | ||
| (error: unknown) => { | ||
| console.error("Failed to render static index:", error); | ||
| process.exit(1); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use translateText() for the error message.
Line 15 shows operator-visible text without translateText(). Import and use the repository translation helper for "Failed to render static index:".
As per coding guidelines: **/*.{ts,tsx}: All user-visible text must go through translateText().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/RenderStaticIndex.ts` around lines 12 - 17, Update the error
handling callback in RenderStaticIndex to pass the user-visible “Failed to
render static index:” message through the repository’s translateText() helper,
adding the required import and preserving the existing error output and exit
behavior.
Source: Coding guidelines
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/Main.ts`:
- Around line 1077-1090: Update the history-replacement comparison following the
targetUrl selection to compare the complete relative URL, including the hash,
rather than only window.location.pathname. Ensure the versioned replay case in
isVersionedReplayPage removes an existing `#join` fragment when lobbyIdHidden is
true, while preserving unchanged URLs and the existing behavior for other
targetUrl branches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83edb46b-1515-46b1-8a5f-ba66103059d5
📒 Files selected for processing (1)
src/client/Main.ts
Every deploy now uploads a fully-rendered app shell to R2 as index-<short-commit>.html, rendered inside the image with the deploy's own env so the baked BOOTSTRAP_CONFIG matches what the server would serve. When a replay's record was produced by a different build, the client probes the CDN for the matching shell and navigates to it (#join=<id> hash, since static hosting has no path routing); the shell's build then simulates the game under the rules it was played with. Replays no longer request a Turnstile token (they simulate locally from the record), and the shells skip the prefetch entirely - the CDN origin is not on the site key's allowlist. Fixes #4934 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On a versioned replay shell, updateJoinUrlForShare and the prestart handler rewrote the address to /game/<id> (and the #refresh trampoline could rewrite it to the bare origin) - URLs that only exist on the game-server origin and 404 on the CDN when reloaded or shared. Keep the shell pathname and #join= hash there; non-shell pages keep the existing behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f579ae4 to
c45c5b4
Compare
The versioned shells are now served on replay.openfront.io/.dev by the API worker (infra#514), so the redirect derives the host from the JWT audience - exactly like getApiBase() derives api.<audience> - instead of building a cdn.ofedge.* URL from cdnBase. No client config needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/VersionedReplay.ts`:
- Around line 33-34: Update isVersionedReplayPage to match only the host-root
shell path /index-<SHORT_COMMIT_LENGTH>-hex.html, reusing the existing
SHORT_COMMIT_LENGTH constant rather than allowing arbitrary nesting or hash
lengths. Add negative tests covering nested paths and hashes with incorrect
lengths while preserving valid root-path matches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff7b9aa1-b50d-4bcd-ae5f-32631fb2c3fc
📒 Files selected for processing (3)
src/client/JoinLobbyModal.tssrc/client/VersionedReplay.tstests/VersionedReplay.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/JoinLobbyModal.ts
| export function isVersionedReplayPage(pathname: string): boolean { | ||
| return /\/index-[0-9a-f]+\.html$/.test(pathname); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the exact generated shell path.
Line 34 accepts nested paths and any hexadecimal length. The deployment generates only /index-<7-hex>.html at the host root. A path such as /nested/index-0847945.html can therefore be treated as a versioned shell, causing redirectToVersionedShell() to skip the redirect. Match the root path and SHORT_COMMIT_LENGTH, then add negative tests for nested and wrong-length paths.
Proposed fix
export function isVersionedReplayPage(pathname: string): boolean {
- return /\/index-[0-9a-f]+\.html$/.test(pathname);
+ return new RegExp(
+ `^/index-[0-9a-f]{${SHORT_COMMIT_LENGTH}}\\.html$`,
+ ).test(pathname);
}📝 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.
| export function isVersionedReplayPage(pathname: string): boolean { | |
| return /\/index-[0-9a-f]+\.html$/.test(pathname); | |
| export function isVersionedReplayPage(pathname: string): boolean { | |
| return new RegExp( | |
| `^/index-[0-9a-f]{${SHORT_COMMIT_LENGTH}}\\.html$`, | |
| ).test(pathname); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/VersionedReplay.ts` around lines 33 - 34, Update
isVersionedReplayPage to match only the host-root shell path
/index-<SHORT_COMMIT_LENGTH>-hex.html, reusing the existing SHORT_COMMIT_LENGTH
constant rather than allowing arbitrary nesting or hash lengths. Add negative
tests covering nested paths and hashes with incorrect lengths while preserving
valid root-path matches.
The replay worker now resolves the record's commit itself and serves the matching shell at replay.<domain>/<gameId> (infra#514), so the client redirect carries only the game id - no commit hash in the URL. On the shell the pathname IS the game id, replacing the #join= hash mechanism, and shell detection is host-based (replay.*) instead of pathname-based. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/VersionedReplay.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a type-only import for
GameID.
GameIDis used only as a type. Useimport typeto make this dependency explicit and prevent a runtime import if import preservation is enabled later.Suggested change
-import { GameID } from "../core/Schemas"; +import type { GameID } from "../core/Schemas";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/VersionedReplay.ts` at line 10, Change the GameID import in VersionedReplay.ts to a type-only import, preserving its existing type usage and avoiding a runtime dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/client/VersionedReplay.ts`:
- Line 10: Change the GameID import in VersionedReplay.ts to a type-only import,
preserving its existing type usage and avoiding a runtime dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17b7a90b-94a3-471f-823e-66e38fa9e4d1
📒 Files selected for processing (4)
src/client/JoinLobbyModal.tssrc/client/Main.tssrc/client/VersionedReplay.tstests/VersionedReplay.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/VersionedReplay.test.ts
- src/client/JoinLobbyModal.ts
- src/client/Main.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Main.ts (1)
1048-1054: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProtect replay rollback from a pending
hashchange.If a replay URL contains a hash, this pathname-only
history.pushStatecreates the replay entry without that hash. When the player presses Back,onPopStaterestoresthis.currentUrlbefore the browser dispatcheshashchange.onHashUpdatecan then callonJoinChanged. The replay leave path can redirect to the main site even when the player cancels the exit confirmation.Set a
preventHashUpdateflag only when the traversed hash differs from the restored URL. Clear the flag at the start ofonHashUpdate. Add a regression test for browser Back navigation followed by cancel on a hash-bearing replay URL.Based on learnings: when
popstatefires beforehashchangeduring rollback, use apreventHashUpdate-like flag and document its set/reset conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/Main.ts` around lines 1048 - 1054, Protect replay rollback from a pending hashchange by updating the relevant onPopState/onHashUpdate flow: set preventHashUpdate only when the traversed hash differs from the restored currentUrl, clear it at the start of onHashUpdate, and document these set/reset conditions. Add a regression test covering Back navigation on a hash-bearing replay URL followed by cancelling the exit confirmation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/Main.ts`:
- Around line 1048-1054: Protect replay rollback from a pending hashchange by
updating the relevant onPopState/onHashUpdate flow: set preventHashUpdate only
when the traversed hash differs from the restored currentUrl, clear it at the
start of onHashUpdate, and document these set/reset conditions. Add a regression
test covering Back navigation on a hash-bearing replay URL followed by
cancelling the exit confirmation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aec5b835-4c3e-49c1-8865-3c36a53d74c7
📒 Files selected for processing (1)
src/client/Main.ts
8ff19d3 to
58ed56c
Compare
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
1 participant
Fixes #4934
Archived games can only be replayed by the exact build that produced them - the client refuses mismatched
gitCommits because the deterministic sim would desync. Once a deployment is replaced, its games become unwatchable. This PR keeps every build replayable by publishing a static, self-contained copy of the app shell per deploy and redirecting mismatched replays to it.Upload side (
update.sh+RenderStaticIndex.ts)After the existing asset upload, the deploy renders the app shell inside the freshly built image (
docker run --entrypoint npx <image> tsx src/server/RenderStaticIndex.ts, a thin wrapper around the server's ownrenderHtmlContent) using the same env file the live container gets, so the bakedBOOTSTRAP_CONFIG(gitCommit, cdnBase, jwtAudience, ...) matches what the server would have served. The result is PUT to R2 asindex-<short-commit>.htmlnext to the hashed assets it references. Since hashed assets are immutable and never deleted, the shell + CDN + API are enough to replay - no game server involved.Redirect side
The canonical replay URL is
https://replay.<domain>/<gameId>(openfrontio/infra#514): the API worker reads the archived record'sgitCommitand serves the matching shell at that URL, so the address players see and share carries only the game id. The client derives the host from the JWT audience, exactly likegetApiBase()derivesapi.<audience>- no new config.JoinLobbyModal.checkArchivedGame: on commit mismatch, HEAD-probereplay.<audience>/<gameId>and navigate there. The probe requires 200 andtext/html; games from builds that predate this feature 404 (no shell uploaded) and fall back to the existing version-mismatch message.Main.ts: on a replay shell host the pathname IS the game id - the shell parses it and starts the join flow. The active-lobby check fails harmlessly (relative URL), the archive fetch succeeds (replay.<aud>is covered by the API's subdomain CORS rule), the commit matches, and the replay runs locally./game/<id>URL behavior.VersionedReplay.ts: pure URL helpers, unit-tested. Loop guard: pages on areplay.*host never redirect again, so a bad record cannot loop.lobby.gameRecordset ->null, mirroring the singleplayer exemption - replays run throughLocalServer, nothing consumes a token), and shell hosts skip the prefetch entirely: the replay host may not be on the Turnstile site key's allowlist, where the widget would alert and reject.Deploy order
Requires openfrontio/infra#514 (serve shells on
replay.<domain>; supersedes the serving half of infra#513, whose special grants it reverts). Safe to merge in either order: until infra deploys, the client probe fails and behavior is unchanged (version-mismatch message).Testing
tests/VersionedReplay.test.tscovers URL building (audience-derived host, localhost -> null) and the host-based loop-guard invariant.RenderStaticIndex.tswith staging-like env produced a self-contained shell with the full commit, cdnBase, and absolute CDN asset URLs baked in.mcKawtSyarchived at one commit, opened on a deployment of another -> redirect -> shell replayed it (sim ticking, map rendered, URL preserved). Re-verification with the canonical URLs planned after infra#514 deploys.🤖 Generated with Claude Code