playwire ads #2898
playwire ads #2898
Conversation
WalkthroughThe pull request migrates the ad serving integration from Fuse to Playwire's RAMP API. Changes include updating the HTML script tag, refactoring the GutterAds component to use RAMP methods, removing FuseTag from the Window interface, and cleaning up related lifecycle code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 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: 2
🤖 Fix all issues with AI agents
In `@src/client/GutterAds.ts`:
- Around line 64-83: The adLoaded guard is racy because the flag is only set
inside the queued callback; update the show() logic in GutterAds (the method
using window.ramp.que.push and this.adLoaded) to set a guard before pushing to
the queue (e.g., set this.adLoaded or a new this.adQueued = true immediately
when deciding to queue) so subsequent calls return early; if you need to
distinguish queued vs successfully loaded, consider using two flags (adQueued
and adLoaded) and only clear/reset them on explicit failure inside the queued
callback.
🧹 Nitpick comments (2)
src/client/GutterAds.ts (2)
12-16: Consider making ad configuration readonly.These private members act as constants. Marking them
readonlymakes the intent clear and prevents accidental mutation.♻️ Suggested improvement
- private leftAdType: string = "left_rail"; - private rightAdType: string = "right_rail"; - private leftContainerId: string = "gutter-ad-container-left"; - private rightContainerId: string = "gutter-ad-container-right"; - private margin: string = "10px"; + private readonly leftAdType = "left_rail"; + private readonly rightAdType = "right_rail"; + private readonly leftContainerId = "gutter-ad-container-left"; + private readonly rightContainerId = "gutter-ad-container-right"; + private readonly margin = "10px";
23-23: Emptystatic stylescan be removed.Since shadow DOM is disabled via
createRenderRoot(), this empty styles declaration serves no purpose.♻️ Suggested removal
- static styles = css``;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.htmlsrc/client/GutterAds.tssrc/client/Main.tssrc/client/graphics/layers/AdTimer.ts
💤 Files with no reviewable changes (1)
- src/client/graphics/layers/AdTimer.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-29T23:33:17.920Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:17.920Z
Learning: In Tailwind CSS v4, update blur-related utilities in HTML templates. The mappings are: backdrop-blur-sm (v3) -> backdrop-blur-xs (v4); backdrop-blur (bare) -> backdrop-blur-sm; blur-sm -> blur-xs. Apply these changes to all HTML files (e.g., index.html and other templates) to reflect the v4 naming. Consider updating a project-wide search/replace or using a codemod to ensure consistency across the codebase.
Applied to files:
index.html
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/GutterAds.tssrc/client/Main.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
📚 Learning: 2026-01-08T13:52:00.939Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:00.939Z
Learning: In src/client/Main.ts, ensure the browser back/forward navigation ordering is correctly handled: when popstate fires before hashchange during a navigation rollback (e.g., user cancels leaving an active game), use a preventHashUpdate-like flag to suppress the hashchange listener (onHashUpdate) from running in that rollback scenario. This avoids applying an unintended hash update after a rollback and keeps the UI/game state consistent. Document the flag’s usage and the exact conditions under which it is set/reset to prevent subtle regressions.
Applied to files:
src/client/Main.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (5)
index.html (1)
78-83: LGTM! Playwire RAMP integration looks correct.The initialization pattern with
window.ramp.queensures queued operations run after the script loads. Usingdata-cfasync="false"is the right call to prevent Cloudflare Rocket Loader from deferring the ad script.src/client/Main.ts (2)
172-181: LGTM! Window.ramp interface declaration is well-typed.The interface captures the essential RAMP API methods. Clean and minimal.
278-282: Callingshow()immediately may load ads before the user reaches gameplay.Calling
this.gutterAds.show()right after element validation means ads load on page load. If the intent is to show gutter ads only during certain states (like in-game or lobby), this might show ads prematurely.Please confirm this is the intended behavior—showing gutter ads immediately on page load. If ads should only appear in specific contexts, consider deferring the
show()call.src/client/GutterAds.ts (2)
31-39: LGTM! Clean async pattern for ad loading.Using
updateComplete.then()ensures DOM containers exist before callingspaAddAds. Good approach.
105-144: LGTM! Clean render with loading state.The conditional "Loading ad..." text provides good feedback. Using template literals for dynamic IDs and margins is straightforward.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (this.adLoaded) { | ||
| console.log("Ads already loaded, skipping"); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| console.log("registering zones"); | ||
| window.fusetag.que.push(() => { | ||
| window.fusetag.registerZone(LEFT_FUSE); | ||
| window.fusetag.registerZone(RIGHT_FUSE); | ||
| window.ramp.que.push(() => { | ||
| window.ramp.spaAddAds([ | ||
| { | ||
| type: this.leftAdType, | ||
| selectorId: this.leftContainerId, | ||
| }, | ||
| { | ||
| type: this.rightAdType, | ||
| selectorId: this.rightContainerId, | ||
| }, | ||
| ]); | ||
| this.adLoaded = true; | ||
| console.log("Playwire ads loaded:", this.leftAdType, this.rightAdType); | ||
| }); |
There was a problem hiding this comment.
Race condition: adLoaded guard does not prevent double-queueing.
The adLoaded flag is set inside the queued callback (line 81), not before pushing to the queue. If show() is called twice in quick succession, both calls pass the guard at line 64 and queue duplicate spaAddAds calls.
🐛 Suggested fix: set a flag before queueing
+ // Guard before queueing to prevent duplicate ad requests
+ this.adLoaded = true;
+
try {
window.ramp.que.push(() => {
window.ramp.spaAddAds([
{
type: this.leftAdType,
selectorId: this.leftContainerId,
},
{
type: this.rightAdType,
selectorId: this.rightContainerId,
},
]);
- this.adLoaded = true;
console.log("Playwire ads loaded:", this.leftAdType, this.rightAdType);
});
} catch (error) {
+ this.adLoaded = false;
console.error("Failed to load Playwire ads:", error);
}🤖 Prompt for AI Agents
In `@src/client/GutterAds.ts` around lines 64 - 83, The adLoaded guard is racy
because the flag is only set inside the queued callback; update the show() logic
in GutterAds (the method using window.ramp.que.push and this.adLoaded) to set a
guard before pushing to the queue (e.g., set this.adLoaded or a new
this.adQueued = true immediately when deciding to queue) so subsequent calls
return early; if you need to distinguish queued vs successfully loaded, consider
using two flags (adQueued and adLoaded) and only clear/reset them on explicit
failure inside the queued callback.
| private destroyAds(): void { | ||
| if (!window.fusetag) { | ||
| if (!window.ramp || !this.adLoaded) { | ||
| return; | ||
| } | ||
| window.fusetag.que.push(() => { | ||
| window.fusetag.destroyZone(LEFT_FUSE); | ||
| window.fusetag.destroyZone(RIGHT_FUSE); | ||
| }); | ||
| this.requestUpdate(); | ||
| try { | ||
| window.ramp.destroyUnits("all"); | ||
| } catch (error) { | ||
| console.error("Failed to destroy Playwire ad:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the GutterAds.ts file
find . -name "GutterAds.ts" -type fRepository: openfrontio/OpenFrontIO
Length of output: 92
🏁 Script executed:
# Also search for related ad files
fd -e ts -e tsx | grep -i ad | head -20Repository: openfrontio/OpenFrontIO
Length of output: 916
🏁 Script executed:
# Check if there are any references to Playwire or RAMP in the codebase
rg -i "playwire|ramp" --type ts --type tsx -lRepository: openfrontio/OpenFrontIO
Length of output: 94
🏁 Script executed:
# Read the GutterAds.ts file to see the full context
cat -n src/client/GutterAds.tsRepository: openfrontio/OpenFrontIO
Length of output: 5032
🏁 Script executed:
# Search for RAMP and Playwire references properly
rg -i "playwire|ramp" -A 3 -B 3Repository: openfrontio/OpenFrontIO
Length of output: 50380
🏁 Script executed:
# Check if other ad units exist and how they interact
grep -r "window.ramp" --include="*.ts" --include="*.tsx" -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 1000
🌐 Web query:
Playwire RAMP API destroyUnits method documentation
💡 Result:
I couldn't find any public documentation for a RAMP API method named destroyUnits. The RAMP site and docs appear to require sign‑in, so API references are likely behind the Playwire/RAMP account portal. See RAMP sign‑in/portal and product page. [1][2]
If you want, I can:
- try again and search more widely for private/archived docs or SDK code mentioning destroyUnits, or
- walk through how to inspect the RAMP client SDK on your site (if you can share a snippet) to locate a destroyUnits call, or
- draft a support message you can send to Playwire asking for destroyUnits docs.
Which would you prefer?
Sources:
- RAMP portal (requires sign‑in). [turn0search4].
- Playwire RAMP product page. [turn0search1].
🏁 Script executed:
# Check GoogleAdElement for Playwire usage
rg -A 10 -B 5 "class GoogleAdElement|window|ramp" src/client/GoogleAdElement.ts | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1532
🏁 Script executed:
# Search for all ad-related instantiations or methods
rg "spaAddAds|destroyUnits" --type tsRepository: openfrontio/OpenFrontIO
Length of output: 346
🏁 Script executed:
# Check if there are any other components that might load ads
rg "new.*Ad|AdElement" --type ts -A 2 | head -40Repository: openfrontio/OpenFrontIO
Length of output: 1186
Use specific ad type strings instead of "all" when destroying ads.
The destroyUnits("all") call destroys all Playwire ad units on the page. While only GutterAds currently uses RAMP in the codebase, this approach could break future ad units. Instead, destroy only the ad types that GutterAds added:
🐛 Suggested fix: destroy only gutter ad types
private destroyAds(): void {
if (!window.ramp || !this.adLoaded) {
return;
}
try {
- window.ramp.destroyUnits("all");
+ window.ramp.destroyUnits(this.leftAdType);
+ window.ramp.destroyUnits(this.rightAdType);
} catch (error) {
console.error("Failed to destroy Playwire ad:", error);
}
}
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME