Conversation
WalkthroughAdds a VideoAd LitElement and Ramp/Bolt ad integration; matchmaking now coordinates ad midpoint/completion and includes a Changes
Sequence DiagramsequenceDiagram
participant User as User
participant MM as Matchmaking UI
participant VideoAd as VideoAd Component
participant RampBolt as Ramp/Bolt API
participant ClientGR as ClientGameRunner
User->>MM: Click "Join Ranked"
activate MM
MM->>VideoAd: render & showVideoAd()
activate VideoAd
VideoAd->>RampBolt: wait for Ramp, addUnits(), displayUnits()
RampBolt-->>VideoAd: ad events (started, midpoint, complete) or error
alt Ad Blocked / Error
VideoAd->>MM: onAdBlocked()
MM->>MM: show ad-blocked UI
VideoAd-->>MM: resolve waitBeforeJoin (blocked path)
else Ad Plays
RampBolt-->>VideoAd: midpoint
VideoAd->>MM: onMidpoint()
RampBolt-->>VideoAd: complete
VideoAd->>MM: onComplete()
end
MM->>ClientGR: dispatch join-lobby (detail.waitBeforeJoin = waitForAdComplete())
activate ClientGR
ClientGR->>ClientGR: await waitBeforeJoin
ClientGR->>ClientGR: call onPrestart() and onJoin() after wait resolves
deactivate ClientGR
deactivate MM
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🤖 Fix all issues with AI agents
In `@src/client/VideoAd.ts`:
- Around line 60-88: The component removes the video container when isVisible is
false so subsequent showVideoAd() calls have no mount point; change
showVideoAd() to set this.isVisible = true before queuing/adding units and
modify render() to always render the <div id="precontent-video-location"> but
toggle its visibility via a CSS class or inline style (e.g.,
hidden/opacity/visibility) instead of returning html``; update any hide logic to
hide the container (set a hidden flag or class) rather than removing it so
window.ramp can mount the player on subsequent calls.
- Around line 8-33: The component currently shows the overlay immediately and
polls ramp indefinitely; change the initial state of isVisible to false, update
connectedCallback to wait for the adsEnabled decision (listen for the
"userMeResponse" event or check an existing adsEnabled flag) before calling
showVideoAd, and modify showVideoAd to add a max poll count (~100 attempts at
100ms = ~10s) so the interval clears and aborts if window.ramp or
window.ramp.que never appears; ensure loadVideoAd is still called when ramp
becomes available and also ensure isVisible is reset to true when starting an ad
so the overlay can show again after completion.
- Around line 35-58: Add a guard to prevent registering Bolt listeners more than
once in loadVideoAd(): introduce a private boolean flag (e.g.,
boltListenersAttached) on the VideoAd class, check it at the start of
loadVideoAd()/inside window.ramp.onPlayerReady and return early if true, and set
it to true immediately after successfully calling the three window.Bolt.on(...)
registrations for VIDEO_AD_UNIT_TYPE so listeners are only bound once across
disconnect/reconnect cycles.
🧹 Nitpick comments (2)
startup.sh (1)
88-90: Simplify the redundant DOMAIN/SUBDOMAIN check.Both branches execute the same command now, so the
ifcan be removed.♻️ Suggested cleanup
-if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -else - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -fi +exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.confsrc/client/Main.ts (1)
182-200: MakerampandBoltglobals optional to match the runtime guards already in code.These window globals are loaded asynchronously and may not exist (ad-free users, CrazyGames, script timing). The type definitions currently mark them as required, but the codebase already guards against their absence with checks like
if (!window.ramp)andif (window.Bolt)throughout VideoAd.ts, GutterAds.ts, and InGameHeaderAd.ts. Making them optional in the type definition will align TypeScript with the actual runtime behavior and ensure safe access patterns.Suggested type tweaks
ramp: { que: Array<() => void>; passiveMode: boolean; spaAddAds: (ads: Array<{ type: string; selectorId: string }>) => void; destroyUnits: (adType: string) => void; settings?: { slots?: any; }; spaNewPage: (url?: string) => void; // Video ad methods - onPlayerReady: (() => void) | null; - addUnits: (units: Array<{ type: string }>) => Promise<void>; - displayUnits: () => void; + onPlayerReady?: (() => void) | null; + addUnits?: (units: Array<{ type: string }>) => Promise<void>; + displayUnits?: () => void; }; - Bolt: { + Bolt?: {
| @state() | ||
| private isVisible: boolean = true; | ||
|
|
||
| createRenderRoot() { | ||
| return this; | ||
| } | ||
|
|
||
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| this.showVideoAd(); | ||
| } | ||
|
|
||
| public showVideoAd(): void { | ||
| if (!window.ramp) { | ||
| // Wait for ramp to be available | ||
| const checkRamp = setInterval(() => { | ||
| if (window.ramp && window.ramp.que) { | ||
| clearInterval(checkRamp); | ||
| this.loadVideoAd(); | ||
| } | ||
| }, 100); | ||
| return; | ||
| } | ||
|
|
||
| this.loadVideoAd(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "VideoAd.ts" -type f | head -5Repository: openfrontio/OpenFrontIO
Length of output: 90
🏁 Script executed:
git ls-files "**/*.ts" | grep -i video | head -20Repository: openfrontio/OpenFrontIO
Length of output: 88
🏁 Script executed:
rg "window\.ramp" --type ts -A 3 -B 3Repository: openfrontio/OpenFrontIO
Length of output: 3489
🏁 Script executed:
rg "adsEnabled" --type ts -A 2 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 1425
🏁 Script executed:
cat -n src/client/VideoAd.tsRepository: openfrontio/OpenFrontIO
Length of output: 2941
🏁 Script executed:
rg "onPlayerReady" --type ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 712
Gate video ads and add a ramp timeout to prevent stuck full-screen overlay.
The overlay displays immediately on component mount, and the ramp availability check polls indefinitely with no timeout. If Ramp never loads or ads are disabled in the app, users see a black screen they cannot close. Additionally, VideoAd lacks the adsEnabled gate that GutterAds and InGameHeaderAd already use, and it doesn't listen for the userMeResponse event to wait for the ads decision.
Also, isVisible is set to false on ad completion but never reset to true, so the component cannot display ads a second time if needed.
Please apply these fixes:
- Wait for
adsEnabledto be determined before callingshowVideoAd() - Add a maximum poll count to
showVideoAd()so the check stops after ~10 seconds - Initialize
isVisibletofalseto avoid showing the overlay before ads are allowed
Suggested implementation
`@state`()
-private isVisible: boolean = true;
+private isVisible = false;
+private rampCheckAttempts = 0;
+private readonly maxRampChecks = 100;
connectedCallback() {
super.connectedCallback();
- this.showVideoAd();
+ if (window.adsEnabled === undefined) {
+ const onUserMe = () => {
+ document.removeEventListener("userMeResponse", onUserMe as EventListener);
+ if (window.adsEnabled) this.showVideoAd();
+ };
+ document.addEventListener("userMeResponse", onUserMe as EventListener);
+ return;
+ }
+ if (window.adsEnabled) this.showVideoAd();
}
public showVideoAd(): void {
+ if (window.adsEnabled === false) return;
if (!window.ramp) {
const checkRamp = setInterval(() => {
+ this.rampCheckAttempts += 1;
if (window.ramp && window.ramp.que) {
clearInterval(checkRamp);
this.loadVideoAd();
+ } else if (this.rampCheckAttempts >= this.maxRampChecks) {
+ clearInterval(checkRamp);
+ console.warn("[VideoAd] ramp not available, skipping ad");
+ this.isVisible = false;
}
}, 100);
return;
}🤖 Prompt for AI Agents
In `@src/client/VideoAd.ts` around lines 8 - 33, The component currently shows the
overlay immediately and polls ramp indefinitely; change the initial state of
isVisible to false, update connectedCallback to wait for the adsEnabled decision
(listen for the "userMeResponse" event or check an existing adsEnabled flag)
before calling showVideoAd, and modify showVideoAd to add a max poll count (~100
attempts at 100ms = ~10s) so the interval clears and aborts if window.ramp or
window.ramp.que never appears; ensure loadVideoAd is still called when ramp
becomes available and also ensure isVisible is reset to true when starting an ad
so the overlay can show again after completion.
| private loadVideoAd(): void { | ||
| // Set up event listeners when player is ready | ||
| window.ramp.onPlayerReady = () => { | ||
| if (window.Bolt) { | ||
| window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_COMPLETE, () => { | ||
| console.log("[VideoAd] Ad completed"); | ||
| this.isVisible = false; | ||
| }); | ||
|
|
||
| window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_ERROR, () => { | ||
| console.log("[VideoAd] Ad error/no fill"); | ||
| this.isVisible = false; | ||
| }); | ||
|
|
||
| window.Bolt.on( | ||
| VIDEO_AD_UNIT_TYPE, | ||
| window.Bolt.SHOW_HIDDEN_CONTAINER ?? "showHiddenContainer", | ||
| () => { | ||
| console.log("[VideoAd] Ad finished"); | ||
| this.isVisible = false; | ||
| }, | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Playwire RAMP onPlayerReady API documentation multiple listeners
💡 Result:
Playwire’s RAMP “onPlayerReady” API docs don’t appear to be publicly accessible (the RAMP portal is gated behind a login), so there isn’t a public reference that confirms whether onPlayerReady supports multiple listeners out of the box. [1]
What is publicly documented is Playwire’s React RAMP wrapper, which shows the supported integration surface (loading <Ramp /> once, and placing <RampUnit /> components), but it does not document a player-event API like onPlayerReady or listener semantics. [2]
Practical guidance (when you need “multiple listeners” anyway)
If your integration exposes onPlayerReady as a single callback property (common pattern: player.onPlayerReady = fn), you typically can’t attach multiple independent listeners without overwriting. The usual solution is to register one onPlayerReady handler and fan out yourself:
- keep an array of callbacks
- call them all from the single
onPlayerReadyyou wire to RAMP
If you can share the snippet you’re using (or the exact object where onPlayerReady lives—e.g., ramp, ramp.player, window.ramp, etc.), I can tell you which pattern applies and the safest way to attach multiple handlers.
Sources: [1] [2]
🏁 Script executed:
# Search for onPlayerReady usage in the codebase
rg "onPlayerReady" -t ts -t js
# Check VideoAd.ts structure to understand initialization
fd VideoAd.ts -x wc -l {}Repository: openfrontio/OpenFrontIO
Length of output: 216
🏁 Script executed:
# Search for window.ramp usage patterns
rg "window\.ramp" -t ts -t js -A 2 -B 2
# Check if loadVideoAd is called multiple times
rg "loadVideoAd" -t ts -t jsRepository: openfrontio/OpenFrontIO
Length of output: 3072
🏁 Script executed:
# Read VideoAd.ts around the showVideoAd method and full context
sed -n '20,75p' src/client/VideoAd.tsRepository: openfrontio/OpenFrontIO
Length of output: 1562
🏁 Script executed:
# Also check if there are other places that might call loadVideoAd indirectly
rg "showVideoAd" -t ts -t js -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 420
Prevent duplicate Bolt listener registration on reconnection.
If the component is disconnected and reconnected, loadVideoAd() executes again, registering the same three Bolt.on() listeners multiple times. Add a guard flag to ensure listeners are bound only once.
🛠️ Suggested fix
+ private boltListenersBound = false;
+
private loadVideoAd(): void {
// Set up event listeners when player is ready
window.ramp.onPlayerReady = () => {
+ if (this.boltListenersBound) return;
+ this.boltListenersBound = true;
if (window.Bolt) {
window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_COMPLETE, () => {
console.log("[VideoAd] Ad completed");
this.isVisible = false;
});📝 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.
| private loadVideoAd(): void { | |
| // Set up event listeners when player is ready | |
| window.ramp.onPlayerReady = () => { | |
| if (window.Bolt) { | |
| window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_COMPLETE, () => { | |
| console.log("[VideoAd] Ad completed"); | |
| this.isVisible = false; | |
| }); | |
| window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_ERROR, () => { | |
| console.log("[VideoAd] Ad error/no fill"); | |
| this.isVisible = false; | |
| }); | |
| window.Bolt.on( | |
| VIDEO_AD_UNIT_TYPE, | |
| window.Bolt.SHOW_HIDDEN_CONTAINER ?? "showHiddenContainer", | |
| () => { | |
| console.log("[VideoAd] Ad finished"); | |
| this.isVisible = false; | |
| }, | |
| ); | |
| } | |
| }; | |
| private boltListenersBound = false; | |
| private loadVideoAd(): void { | |
| // Set up event listeners when player is ready | |
| window.ramp.onPlayerReady = () => { | |
| if (this.boltListenersBound) return; | |
| this.boltListenersBound = true; | |
| if (window.Bolt) { | |
| window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_COMPLETE, () => { | |
| console.log("[VideoAd] Ad completed"); | |
| this.isVisible = false; | |
| }); | |
| window.Bolt.on(VIDEO_AD_UNIT_TYPE, window.Bolt.BOLT_AD_ERROR, () => { | |
| console.log("[VideoAd] Ad error/no fill"); | |
| this.isVisible = false; | |
| }); | |
| window.Bolt.on( | |
| VIDEO_AD_UNIT_TYPE, | |
| window.Bolt.SHOW_HIDDEN_CONTAINER ?? "showHiddenContainer", | |
| () => { | |
| console.log("[VideoAd] Ad finished"); | |
| this.isVisible = false; | |
| }, | |
| ); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/client/VideoAd.ts` around lines 35 - 58, Add a guard to prevent
registering Bolt listeners more than once in loadVideoAd(): introduce a private
boolean flag (e.g., boltListenersAttached) on the VideoAd class, check it at the
start of loadVideoAd()/inside window.ramp.onPlayerReady and return early if
true, and set it to true immediately after successfully calling the three
window.Bolt.on(...) registrations for VIDEO_AD_UNIT_TYPE so listeners are only
bound once across disconnect/reconnect cycles.
| // Queue the video ad initialization | ||
| window.ramp.que.push(() => { | ||
| const pwUnits = [{ type: VIDEO_AD_UNIT_TYPE }]; | ||
|
|
||
| window.ramp | ||
| .addUnits(pwUnits) | ||
| .then(() => { | ||
| window.ramp.displayUnits(); | ||
| }) | ||
| .catch((e: Error) => { | ||
| console.error("[VideoAd] Error adding units:", e); | ||
| window.ramp.displayUnits(); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| render() { | ||
| if (!this.isVisible) { | ||
| return html``; | ||
| } | ||
|
|
||
| // Provide a container for the Playwire video player to render into | ||
| return html` | ||
| <div | ||
| id="precontent-video-location" | ||
| class="fixed inset-0 z-[99999] flex items-center justify-center" | ||
| style="background: rgba(0,0,0,0.9);" | ||
| ></div> | ||
| `; |
There was a problem hiding this comment.
showVideoAd() can’t show a second ad after it hides itself.
Once isVisible becomes false, the container is removed and never shown again. If showVideoAd() is called later, there is no mount point. Set isVisible = true before loading, and keep the container in the DOM while hidden.
🛠️ Possible fix
private loadVideoAd(): void {
+ this.isVisible = true;
// Queue the video ad initialization
window.ramp.que.push(() => {
const pwUnits = [{ type: VIDEO_AD_UNIT_TYPE }];
@@
- render() {
- if (!this.isVisible) {
- return html``;
- }
-
- // Provide a container for the Playwire video player to render into
- return html`
- <div
- id="precontent-video-location"
- class="fixed inset-0 z-[99999] flex items-center justify-center"
- style="background: rgba(0,0,0,0.9);"
- ></div>
- `;
- }
+ render() {
+ return html`
+ <div
+ id="precontent-video-location"
+ class="fixed inset-0 z-[99999] items-center justify-center"
+ style="background: rgba(0,0,0,0.9); display: ${this.isVisible
+ ? "flex"
+ : "none"};"
+ aria-hidden=${this.isVisible ? "false" : "true"}
+ ></div>
+ `;
+ }🤖 Prompt for AI Agents
In `@src/client/VideoAd.ts` around lines 60 - 88, The component removes the video
container when isVisible is false so subsequent showVideoAd() calls have no
mount point; change showVideoAd() to set this.isVisible = true before
queuing/adding units and modify render() to always render the <div
id="precontent-video-location"> but toggle its visibility via a CSS class or
inline style (e.g., hidden/opacity/visibility) instead of returning html``;
update any hide logic to hide the container (set a hidden flag or class) rather
than removing it so window.ramp can mount the player on subsequent calls.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/VideoAd.ts`:
- Around line 20-29: The polling interval started in showVideoAd (currently
using const checkRamp = setInterval(...)) isn't cleared if the component
unmounts; change it to store the interval ID on the instance (e.g.,
this.checkRampInterval) and use that stored ID to clearInterval when ramp is
found, and also clearInterval in the component's disconnectedCallback method to
prevent the interval from running after unmount; update references to
clearInterval(checkRamp) to clearInterval(this.checkRampInterval) and ensure
loadVideoAd is only called after clearing the stored interval.
- Around line 69-72: The catch block for addUnits in VideoAd currently calls
window.ramp.displayUnits() even when addUnits fails; remove the call to
displayUnits from the error handler so that only the error is logged (keep the
console.error/ logging) and ensure displayUnits is invoked only from the
successful resolution path of addUnits (look for the promise chain around
addUnits and the surrounding code in VideoAd to place displayUnits after the
successful .then).
| public showVideoAd(): void { | ||
| if (!window.ramp) { | ||
| // Wait for ramp to be available | ||
| const checkRamp = setInterval(() => { | ||
| if (window.ramp && window.ramp.que) { | ||
| clearInterval(checkRamp); | ||
| this.loadVideoAd(); | ||
| } | ||
| }, 100); | ||
| return; |
There was a problem hiding this comment.
Missing cleanup of polling interval if component unmounts.
If the component disconnects before window.ramp becomes available, the setInterval continues running and may call methods on a detached component.
Store the interval ID and clear it in disconnectedCallback.
Suggested fix
+ private rampCheckInterval?: ReturnType<typeof setInterval>;
+
+ disconnectedCallback() {
+ super.disconnectedCallback();
+ if (this.rampCheckInterval) {
+ clearInterval(this.rampCheckInterval);
+ this.rampCheckInterval = undefined;
+ }
+ }
+
public showVideoAd(): void {
if (!window.ramp) {
// Wait for ramp to be available
- const checkRamp = setInterval(() => {
+ this.rampCheckInterval = setInterval(() => {
if (window.ramp && window.ramp.que) {
- clearInterval(checkRamp);
+ clearInterval(this.rampCheckInterval);
+ this.rampCheckInterval = undefined;
this.loadVideoAd();
}
}, 100);
return;
}🤖 Prompt for AI Agents
In `@src/client/VideoAd.ts` around lines 20 - 29, The polling interval started in
showVideoAd (currently using const checkRamp = setInterval(...)) isn't cleared
if the component unmounts; change it to store the interval ID on the instance
(e.g., this.checkRampInterval) and use that stored ID to clearInterval when ramp
is found, and also clearInterval in the component's disconnectedCallback method
to prevent the interval from running after unmount; update references to
clearInterval(checkRamp) to clearInterval(this.checkRampInterval) and ensure
loadVideoAd is only called after clearing the stored interval.
| .catch((e: Error) => { | ||
| console.error("[VideoAd] Error adding units:", e); | ||
| window.ramp.displayUnits(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the VideoAd.ts file to understand the full context
cd src/client && wc -l VideoAd.tsRepository: openfrontio/OpenFrontIO
Length of output: 80
🏁 Script executed:
# Get the context around lines 60-91 in VideoAd.ts
cat -n src/client/VideoAd.ts | sed -n '55,95p'Repository: openfrontio/OpenFrontIO
Length of output: 1211
🏁 Script executed:
# Search for other usages of displayUnits() in the codebase
rg "displayUnits" --type tsRepository: openfrontio/OpenFrontIO
Length of output: 237
🌐 Web query:
Playwire RAMP queue addUnits displayUnits error handling best practice
💡 Result:
What to do (best-practice pattern)
-
Always gate RAMP calls behind the command queue (
window.ramp.que)- Initialize
window.ramp = window.ramp || {}; window.ramp.que = window.ramp.que || []; - Push functions onto the queue; RAMP later drains the queue and then replaces
que.pushso queued functions execute immediately. (This is how RAMP avoids “API not ready yet” races in async loading.) [2]
- Initialize
-
Call order:
addUnits()first, thendisplayUnits()- Treat
addUnits()as “declare/register what units should exist on this page/view” - Treat
displayUnits()as “render/fill now that the DOM is ready” - In SPAs, do this per route/view: update page context →
addUnits()→displayUnits()(and optionally refresh specific slots, if your integration supports it). [2]
- Treat
-
Only
displayUnits()after the containers exist- If your containers are injected later (SPA hydration, infinite scroll), delay
displayUnits()until:- the target element exists, and
- it’s attached to the document (not a detached node)
- Common approaches:
MutationObserver, a “waitForElement” promise, or your framework’s “after render” hook.
- If your containers are injected later (SPA hydration, infinite scroll), delay
-
Error handling: fail “closed” (don’t spam) and keep the page usable
- Wrap each queued function body with
try/catch - Add idempotency guards so you don’t re-add/re-display the same units repeatedly on re-renders (a very common SPA bug):
- e.g., keep a
Setof unit IDs/types already registered for the current view.
- e.g., keep a
- Add light telemetry (your logger /
console.warn) so you can debug without breaking UX.
- Wrap each queued function body with
-
Timeout protection for “never becomes ready”
- Because RAMP loads additional scripts asynchronously (the core loader injects the main PageOS script async) you should assume “might never finish” (blocked script, adblock, network). [1]
- Put a max wait (e.g., 3–5s) around your own “wait for element then display” flow, and then just give up silently.
Reference implementation (robust queue + try/catch + SPA-safe guards)
// 1) Safe bootstrap
window.ramp = window.ramp || {};
window.ramp.que = window.ramp.que || [];
const rampSafe = (fn, label) => () => {
try { fn(); }
catch (e) { console.warn(`[RAMP] ${label} failed`, e); }
};
// Optional: per-view dedupe
const registered = new Set();
function registerAndDisplay(units) {
window.ramp.que.push(rampSafe(() => {
// 2) addUnits first (deduped)
const toAdd = units.filter(u => {
const key = u.id || u.type; // adapt to your unit shape
if (!key || registered.has(key)) return false;
registered.add(key);
return true;
});
if (toAdd.length) window.ramp.addUnits(toAdd);
// 3) then displayUnits (only after containers exist in your code)
window.ramp.displayUnits();
}, "registerAndDisplay"));
}
// SPA route change:
// - clear dedupe set if your “view” is truly new
// registered.clear();
// - wait for DOM containers, then:
registerAndDisplay([{ type: "in_content" }, { type: "sidebar" }]);Why this works: it matches how RAMP expects calls to be queued and later executed when the library is ready, and it prevents the most common SPA failure modes (double-registration, calling display before DOM exists, and uncaught exceptions breaking future queued work). [2]
Sources: RAMP core loads its main runtime asynchronously by injecting PageOS (pageos.js) [1], and RAMP processes a command queue (window.ramp.que) by executing queued functions and then making subsequent que.push execute immediately; it also exposes/uses methods like window.ramp.addUnits(...) in that flow. [2]
Remove displayUnits() call from the error handler
When addUnits() fails, the units aren't registered. Calling displayUnits() afterward tries to display units that don't exist. The recommended pattern is: call displayUnits() only after successful addUnits().
Current problematic pattern:
.catch((e: Error) => {
console.error("[VideoAd] Error adding units:", e);
window.ramp.displayUnits(); // ← Remove this
});Just log the error and skip displaying when units fail to add.
🤖 Prompt for AI Agents
In `@src/client/VideoAd.ts` around lines 69 - 72, The catch block for addUnits in
VideoAd currently calls window.ramp.displayUnits() even when addUnits fails;
remove the call to displayUnits from the error handler so that only the error is
logged (keep the console.error/ logging) and ensure displayUnits is invoked only
from the successful resolution path of addUnits (look for the promise chain
around addUnits and the surrounding code in VideoAd to place displayUnits after
the successful .then).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
67-116: Await or safely fire-and-forgetonPrestart.
onPrestartis now async but is called withoutawaitor error handling. If it rejects (or waits onwaitUntil), you can get unhandled rejections and racey UI state. Sinceonmessageis async already, it’s safer to await it (or explicitly catch/log).💡 Possible fix
- onPrestart(); + await onPrestart(); ... - onPrestart(); + await onPrestart();
🤖 Fix all issues with AI agents
In `@src/client/Matchmaking.ts`:
- Around line 54-78: waitForAdComplete currently treats any falsy
window.adsEnabled as "ads disabled" and resolves early; change it so it only
resolves immediately when window.adsEnabled === false or this.videoAdComplete
=== true, and otherwise wait for either the ad to finish (via
adCompleteResolve/handleAdComplete) or for a definitive ads-ready signal such as
the existing userMeResponse (or a dedicated ads-ready event/promise). Update
waitForAdComplete to listen for userMeResponse (or an ads-ready flag) when
window.adsEnabled is undefined, and only resolve when that response indicates
ads are disabled or when handleAdComplete triggers adCompleteResolve; also
ensure renderVideoAd still uses window.adsEnabled === true to render the
<video-ad>.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Matchmaking.ts (1)
265-272: Resolve pending ad waits when the modal closes.If the modal closes while waiting for midpoint/complete, the promises can hang forever. Resolve and clear them on close so the awaiting code can exit cleanly.
♻️ Proposed fix
protected onClose(): void { this.connected = false; this.socket?.close(); + if (this.adMidpointResolve) { + this.adMidpointResolve(); + this.adMidpointResolve = null; + } + if (this.adCompleteResolve) { + this.adCompleteResolve(); + this.adCompleteResolve = null; + } if (this.gameCheckInterval) { clearInterval(this.gameCheckInterval); this.gameCheckInterval = null; } }
🧹 Nitpick comments (1)
src/client/Matchmaking.ts (1)
84-91: Avoid console logging in render.
renderVideoAdlogs every render; this is noisy in prod. Prefer a debug flag or remove the log.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/ClientGameRunner.ts`:
- Around line 109-117: The code currently attaches
waitBeforeJoin.then(onPrestart) in both the "prestart" and "start" message
handlers which can call onPrestart twice and also swallows rejections; fix by
attaching the handler only once (e.g., add a boolean flag like prestartAttached
and set it when you first call waitBeforeJoin.then(...)) and change the
attachment to waitBeforeJoin.then(onPrestart).catch(err => {
console.error("waitBeforeJoin failed", err); onPrestart(); }) so any rejection
is logged and onPrestart still runs; apply these changes in the message handling
for message.type "prestart" and "start" where waitBeforeJoin and onPrestart are
used.
In `@src/client/Matchmaking.ts`:
- Around line 250-251: Fix the typo in the console log inside Matchmaking.ts:
update the console.log call that prints "Matchmaking server closed connection"
(the on-close/connection handler) to use a single space so the message reads
"Matchmaking server closed connection"; locate the console.log in the connection
close handler and correct the string.
🧹 Nitpick comments (1)
src/client/Main.ts (1)
791-791: Unnecessaryasynckeyword.This callback is marked
asyncbut contains noawait. Either removeasyncor add a comment if this is intentional for future use.
| waitBeforeJoin.then(onPrestart); | ||
| } | ||
| if (message.type === "start") { | ||
| // Trigger prestart for singleplayer games | ||
| onPrestart(); | ||
| waitBeforeJoin.then(onPrestart); | ||
| console.log( | ||
| `lobby: game started: ${JSON.stringify(message, replacer, 2)}`, | ||
| ); | ||
| await waitBeforeJoin; |
There was a problem hiding this comment.
Potential double onPrestart call and missing rejection handling.
When a prestart message arrives (line 109), you fire waitBeforeJoin.then(onPrestart). Then when start arrives (line 113), you fire it again. If both messages arrive in the same session, onPrestart runs twice.
Also, .then(onPrestart) with no .catch() silently swallows any rejection from waitBeforeJoin. If the ad flow fails, onPrestart never fires and the user sees a stuck loading screen.
Suggested fix
if (message.type === "prestart") {
console.log(
`lobby: game prestarting: ${JSON.stringify(message, replacer)}`,
);
terrainLoad = loadTerrainMap(
message.gameMap,
message.gameMapSize,
terrainMapFileLoader,
);
- waitBeforeJoin.then(onPrestart);
+ waitBeforeJoin.then(onPrestart).catch((e) => {
+ console.error("waitBeforeJoin failed during prestart:", e);
+ onPrestart(); // Proceed anyway so game doesn't hang
+ });
}
if (message.type === "start") {
// Trigger prestart for singleplayer games
- waitBeforeJoin.then(onPrestart);
+ // Only call onPrestart here if prestart message was never received
+ // (singleplayer case). Consider tracking whether onPrestart was already queued.
+ waitBeforeJoin.then(onPrestart).catch((e) => {
+ console.error("waitBeforeJoin failed during start:", e);
+ onPrestart();
+ });🤖 Prompt for AI Agents
In `@src/client/ClientGameRunner.ts` around lines 109 - 117, The code currently
attaches waitBeforeJoin.then(onPrestart) in both the "prestart" and "start"
message handlers which can call onPrestart twice and also swallows rejections;
fix by attaching the handler only once (e.g., add a boolean flag like
prestartAttached and set it when you first call waitBeforeJoin.then(...)) and
change the attachment to waitBeforeJoin.then(onPrestart).catch(err => {
console.error("waitBeforeJoin failed", err); onPrestart(); }) so any rejection
is logged and onPrestart still runs; apply these changes in the message handling
for message.type "prestart" and "start" where waitBeforeJoin and onPrestart are
used.
src/client/Matchmaking.ts
Outdated
| console.log("Matchmaking server closed connection"); | ||
| }; |
There was a problem hiding this comment.
Typo: double space in log message.
Minor: "Matchmaking server closed" has two spaces.
- console.log("Matchmaking server closed connection");
+ console.log("Matchmaking server closed connection");📝 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.
| console.log("Matchmaking server closed connection"); | |
| }; | |
| console.log("Matchmaking server closed connection"); | |
| }; |
🤖 Prompt for AI Agents
In `@src/client/Matchmaking.ts` around lines 250 - 251, Fix the typo in the
console log inside Matchmaking.ts: update the console.log call that prints
"Matchmaking server closed connection" (the on-close/connection handler) to use
a single space so the message reads "Matchmaking server closed connection";
locate the console.log in the connection close handler and correct the string.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/ClientGameRunner.ts`:
- Around line 117-118: The await of waitBeforeJoin inside the async onmessage
handler can reject and leave unhandled rejections; wrap the await waitBeforeJoin
in a try-catch (or append .catch()) inside the onmessage handler in
ClientGameRunner so that onJoin() is only called on success and the catch
handles the error (e.g., log via process/logger, call any existing error
handler, and/or close the transport/session) to prevent bubbling unhandled
rejections; update the block containing waitBeforeJoin and onJoin to perform
this protected await and error handling.
In `@src/client/components/VideoAd.ts`:
- Line 78: Currently the code assigns a new callback to
window.ramp.onPlayerReady, which clobbers any existing listener; change this to
preserve and call the previous callback by capturing the existing handler (e.g.,
const prev = window.ramp.onPlayerReady) and then assign a new function to
window.ramp.onPlayerReady that first invokes prev (if present) and then runs the
VideoAd-specific logic in this component (look for window.ramp.onPlayerReady in
VideoAd.ts to update the assignment); ensure the wrapper correctly binds context
and handles errors from the previous handler.
🧹 Nitpick comments (1)
src/client/components/VideoAd.ts (1)
4-4: Remove debug console.log statements before release.Multiple
console.logcalls exist for debugging purposes. These should be removed or gated behind a debug flag for production.Also applies to: 34-34, 169-169
| await waitBeforeJoin; | ||
| onJoin(); |
There was a problem hiding this comment.
Unhandled rejection if waitBeforeJoin fails at game start.
If waitBeforeJoin rejects at line 117, the error bubbles up from this async onmessage handler. The transport callback may not handle async rejections, leaving the user stuck without feedback.
Wrap in try-catch or use .catch():
Suggested fix
if (message.type === "start") {
// Trigger prestart for singleplayer games
waitBeforeJoin.then(onPrestart);
console.log(
`lobby: game started: ${JSON.stringify(message, replacer, 2)}`,
);
- await waitBeforeJoin;
- onJoin();
+ try {
+ await waitBeforeJoin;
+ } catch (e) {
+ console.error("waitBeforeJoin failed, proceeding anyway:", e);
+ }
+ onJoin();📝 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.
| await waitBeforeJoin; | |
| onJoin(); | |
| try { | |
| await waitBeforeJoin; | |
| } catch (e) { | |
| console.error("waitBeforeJoin failed, proceeding anyway:", e); | |
| } | |
| onJoin(); |
🤖 Prompt for AI Agents
In `@src/client/ClientGameRunner.ts` around lines 117 - 118, The await of
waitBeforeJoin inside the async onmessage handler can reject and leave unhandled
rejections; wrap the await waitBeforeJoin in a try-catch (or append .catch())
inside the onmessage handler in ClientGameRunner so that onJoin() is only called
on success and the catch handles the error (e.g., log via process/logger, call
any existing error handler, and/or close the transport/session) to prevent
bubbling unhandled rejections; update the block containing waitBeforeJoin and
onJoin to perform this protected await and error handling.
| }, VideoAd.AD_LOAD_TIMEOUT_MS); | ||
|
|
||
| // Set up event listeners when player is ready | ||
| window.ramp.onPlayerReady = () => { |
There was a problem hiding this comment.
Overwriting onPlayerReady may break other listeners.
Setting window.ramp.onPlayerReady = () => {...} replaces any existing callback. If other code in the app relies on this callback, it will be lost. Consider preserving the existing callback:
Suggested fix
- window.ramp.onPlayerReady = () => {
+ const existingCallback = window.ramp.onPlayerReady;
+ window.ramp.onPlayerReady = () => {
+ if (existingCallback) existingCallback();
if (window.Bolt) {
// ... listener registrations
}
};🤖 Prompt for AI Agents
In `@src/client/components/VideoAd.ts` at line 78, Currently the code assigns a
new callback to window.ramp.onPlayerReady, which clobbers any existing listener;
change this to preserve and call the previous callback by capturing the existing
handler (e.g., const prev = window.ramp.onPlayerReady) and then assign a new
function to window.ramp.onPlayerReady that first invokes prev (if present) and
then runs the VideoAd-specific logic in this component (look for
window.ramp.onPlayerReady in VideoAd.ts to update the assignment); ensure the
wrapper correctly binds context and handles errors from the previous handler.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Matchmaking.ts (1)
218-237:⚠️ Potential issue | 🔴 CriticalFix indentation - this is causing the Prettier pipeline failure.
Lines 218-229 have broken indentation. The
await this.waitForAdMidpoint()block andthis.socket.send(...)appear to be at the wrong nesting level relative to thesetTimeoutcallback.Suggested fix
this.connectTimeout = setTimeout(async () => { if (this.socket?.readyState !== WebSocket.OPEN) { console.warn("[Matchmaking] socket not ready"); return; } // Set a delay so the user can see the "connecting" message, // otherwise the "searching" message will be shown immediately. // Also wait so people who back out immediately aren't added // to the matchmaking queue. - - // Wait for ad midpoint before sending join request - // This is so the ad doesn't get delay game start too long. - await this.waitForAdMidpoint(); - - // Early return if modal was closed while waiting for ad - if (!this.isModalOpen) { - this.socket?.close(); - return; - } - - this.socket.send( + + // Wait for ad midpoint before sending join request + // This is so the ad doesn't delay game start too long. + await this.waitForAdMidpoint(); + + // Early return if modal was closed while waiting for ad + if (!this.isModalOpen) { + this.socket?.close(); + return; + } + + this.socket.send( JSON.stringify({ type: "join", jwt: await getPlayToken(), }), ); this.connected = true; this.requestUpdate(); }, 2000);
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Matchmaking.ts (1)
336-346:⚠️ Potential issue | 🟡 MinorVerify rejection handling for
waitBeforeJoinin the message handler.The
waitBeforeJoinpromise is correctly passed from the event handler inMain.ts(line 873) tojoinLobby()inClientGameRunner.ts. However, inClientGameRunner.tslines 109, 113, and 117, the promise is used with.then()andawaitwithout explicit rejection handling (no.catch()ortry-catch). If the promise rejects, the rejection will propagate unhandled in the message handler. Ensure rejection is caught and handled appropriately.
🤖 Fix all issues with AI agents
In `@src/client/Matchmaking.ts`:
- Around line 62-80: The resolvers stored by waitForAdComplete and
waitForAdMidpoint (adCompleteResolve and adMidpointResolve) are never cleared if
the modal closes, causing callers like joinLobby to hang; update the modal's
onClose() to call these stored resolve functions (or reject them) if present and
then null out adCompleteResolve and adMidpointResolve so pending Promises are
settled and references cleaned up; locate the instance fields and methods by
name (waitForAdComplete, waitForAdMidpoint, adCompleteResolve,
adMidpointResolve, onClose) and ensure onClose always settles and clears both
resolvers.
- Around line 94-106: The adBlocked branch renders a message but no way to exit;
update the JSX in Matchmaking.ts where this.adBlocked is handled to include a
dismiss control (e.g., a "Close" or "Dismiss" button) next to the message that
invokes the component's existing modal close/dismiss logic (call the existing
close handler such as this.close() or this.closeModal(), or dispatch a 'close'
event if that's the pattern used elsewhere), ensure the button text uses
translateText("matchmaking_modal.close_button") (or add that key) and has
appropriate styling and accessibility attributes so users can exit the modal
when ads are blocked.
| private waitForAdComplete = (): Promise<void> => { | ||
| // If ad is already complete or ads are disabled, resolve immediately | ||
| if (this.videoAdComplete || !window.adsEnabled) { | ||
| return Promise.resolve(); | ||
| } | ||
| return new Promise((resolve) => { | ||
| this.adCompleteResolve = resolve; | ||
| }); | ||
| }; | ||
|
|
||
| private waitForAdMidpoint = (): Promise<void> => { | ||
| // If midpoint already reached or ads are disabled, resolve immediately | ||
| if (this.adMidpointReached || !window.adsEnabled) { | ||
| return Promise.resolve(); | ||
| } | ||
| return new Promise((resolve) => { | ||
| this.adMidpointResolve = resolve; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Promise resolvers not cleaned up when modal closes.
waitForAdComplete() and waitForAdMidpoint() store resolve functions in instance fields. If the user closes the modal before the ad finishes, these promises never resolve. The caller (e.g., joinLobby in ClientGameRunner) will wait forever on await waitBeforeJoin.
Clean up the resolvers in onClose():
Suggested fix in onClose()
protected onClose(): void {
this.connected = false;
this.socket?.close();
+ // Resolve any pending ad promises so callers don't hang
+ if (this.adCompleteResolve) {
+ this.adCompleteResolve();
+ this.adCompleteResolve = null;
+ }
+ if (this.adMidpointResolve) {
+ this.adMidpointResolve();
+ this.adMidpointResolve = null;
+ }
if (this.connectTimeout) {
clearTimeout(this.connectTimeout);
this.connectTimeout = null;
}🤖 Prompt for AI Agents
In `@src/client/Matchmaking.ts` around lines 62 - 80, The resolvers stored by
waitForAdComplete and waitForAdMidpoint (adCompleteResolve and
adMidpointResolve) are never cleared if the modal closes, causing callers like
joinLobby to hang; update the modal's onClose() to call these stored resolve
functions (or reject them) if present and then null out adCompleteResolve and
adMidpointResolve so pending Promises are settled and references cleaned up;
locate the instance fields and methods by name (waitForAdComplete,
waitForAdMidpoint, adCompleteResolve, adMidpointResolve, onClose) and ensure
onClose always settles and clears both resolvers.
| if (this.adBlocked) { | ||
| return html` | ||
| <div | ||
| class="w-full flex flex-col items-center justify-center mb-4 px-6 py-8 bg-red-900/30 border border-red-500/50 rounded-lg" | ||
| > | ||
| <p class="text-red-400 text-lg font-semibold text-center mb-2"> | ||
| ${translateText("matchmaking_modal.ad_blocked_title")} | ||
| </p> | ||
| <p class="text-white/70 text-sm text-center"> | ||
| ${translateText("matchmaking_modal.ad_blocked_message")} | ||
| </p> | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
Ad blocked state shows message but user has no way to proceed.
When adBlocked is true, the user sees "Ad blocked" text but there is no button to close the modal, dismiss the message, or navigate elsewhere. The user is stuck.
Add a close button or link so the user can exit:
Suggested fix
if (this.adBlocked) {
return html`
<div
class="w-full flex flex-col items-center justify-center mb-4 px-6 py-8 bg-red-900/30 border border-red-500/50 rounded-lg"
>
<p class="text-red-400 text-lg font-semibold text-center mb-2">
${translateText("matchmaking_modal.ad_blocked_title")}
</p>
<p class="text-white/70 text-sm text-center">
${translateText("matchmaking_modal.ad_blocked_message")}
</p>
+ <button
+ `@click`="${this.close}"
+ class="mt-4 px-4 py-2 bg-white/10 hover:bg-white/20 text-white rounded-lg transition"
+ >
+ ${translateText("common.back")}
+ </button>
</div>
`;
}🤖 Prompt for AI Agents
In `@src/client/Matchmaking.ts` around lines 94 - 106, The adBlocked branch
renders a message but no way to exit; update the JSX in Matchmaking.ts where
this.adBlocked is handled to include a dismiss control (e.g., a "Close" or
"Dismiss" button) next to the message that invokes the component's existing
modal close/dismiss logic (call the existing close handler such as this.close()
or this.closeModal(), or dispatch a 'close' event if that's the pattern used
elsewhere), ensure the button text uses
translateText("matchmaking_modal.close_button") (or add that key) and has
appropriate styling and accessibility attributes so users can exit the modal
when ads are blocked.
1 participant
Description:
If the user has not made a purchase, then display a video ad while they wait for matchmaking. If the video ad does not load, assume they have adblock and request they disable adblock or make a purchase to play ranked.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan