Draft
Conversation
Contributor
|
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:
WalkthroughReplaced single ad slot with three-config AD_TYPES in in-game promos; updated ad enqueue/teardown and render for multiple slots; added bottom-rail load/destroy methods to homepage promos and wired them into client lobby join/leave; removed footer ad element; increased Docker timeout for a dev domain and lengthened dev game creation interval. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Homepage as HomepagePromos
participant InGame as InGamePromo
participant RAMP as window.ramp
participant DOM as Browser DOM
Client->>Homepage: find homepage-promos
Client->>Homepage: call loadBottomRail()
Homepage->>RAMP: spaAddAds([{type:"bottom_rail"}])
RAMP-->>DOM: insert bottom_rail units into selectors
Note over InGame,RAMP: In-game multi-slot flow
InGame->>RAMP: spaAddAds(AD_TYPES array)
RAMP-->>DOM: insert multiple slot units into selectorIds
DOM-->>InGame: slots available in DOM
Client->>Homepage: on leave -> call destroyBottomRail()
Homepage->>RAMP: destroyUnits("bottom_rail")
InGame->>RAMP: destroyUnits(type) for each AD_TYPES entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/graphics/layers/InGamePromo.ts (2)
162-167: Consider single destroyUnits call with array.The API accepts
string | string[]per the type declaration inMain.ts. A single call with all types is cleaner than looping. However, the current pattern matchesGutterAds.ts, so this is optional.♻️ Optional: Use array parameter for single call
try { - for (const { type } of AD_TYPES) { - window.ramp.destroyUnits(type); - } + window.ramp.destroyUnits(AD_TYPES.map(({ type }) => type)); console.log("successfully destroyed in-game bottom-left ads");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/InGamePromo.ts` around lines 162 - 167, AD_TYPES is being iterated to call window.ramp.destroyUnits(type) multiple times; instead call window.ramp.destroyUnits with an array of types in one call. Replace the loop over AD_TYPES with a single call passing AD_TYPES.map(item => item.type) (or otherwise extract the string[] of types) to window.ramp.destroyUnits, keeping the same try/catch and console logging; reference window.ramp.destroyUnits and AD_TYPES (and the type signature in Main.ts that accepts string | string[]).
130-136: Redundant map - AD_TYPES already has the right shape.
AD_TYPESalready contains objects with onlytypeandselectorIdproperties. The map creates identical copies with no transformation. Pass the array directly.♻️ Simplify by passing AD_TYPES directly
window.ramp.que.push(() => { try { - window.ramp.spaAddAds( - AD_TYPES.map(({ type, selectorId }) => ({ type, selectorId })), - ); + window.ramp.spaAddAds(AD_TYPES); console.log( "In-game bottom-left ads loaded:", AD_TYPES.map((a) => a.type), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/InGamePromo.ts` around lines 130 - 136, In InGamePromo, the call to window.ramp.spaAddAds unnecessarily maps AD_TYPES into identical objects; remove the redundant map and pass AD_TYPES directly to window.ramp.spaAddAds. Locate the call in the InGamePromo file (look for window.ramp.spaAddAds and AD_TYPES) and change the argument from AD_TYPES.map(({ type, selectorId }) => ({ type, selectorId })) to AD_TYPES while leaving the console.log that prints AD_TYPES.map(a => a.type) intact if you still want the type list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/InGamePromo.ts`:
- Around line 162-167: AD_TYPES is being iterated to call
window.ramp.destroyUnits(type) multiple times; instead call
window.ramp.destroyUnits with an array of types in one call. Replace the loop
over AD_TYPES with a single call passing AD_TYPES.map(item => item.type) (or
otherwise extract the string[] of types) to window.ramp.destroyUnits, keeping
the same try/catch and console logging; reference window.ramp.destroyUnits and
AD_TYPES (and the type signature in Main.ts that accepts string | string[]).
- Around line 130-136: In InGamePromo, the call to window.ramp.spaAddAds
unnecessarily maps AD_TYPES into identical objects; remove the redundant map and
pass AD_TYPES directly to window.ramp.spaAddAds. Locate the call in the
InGamePromo file (look for window.ramp.spaAddAds and AD_TYPES) and change the
argument from AD_TYPES.map(({ type, selectorId }) => ({ type, selectorId })) to
AD_TYPES while leaving the console.log that prints AD_TYPES.map(a => a.type)
intact if you still want the type list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2b8dab5-ed2d-4865-8fe3-84e07256e006
📒 Files selected for processing (1)
src/client/graphics/layers/InGamePromo.ts
github-project-automation
bot
moved this from Triage
to Final Review
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 79-80: Replace the hard-coded "100h" timeout in the conditional
that checks DOMAIN and SUBDOMAIN with a configurable PREVIEW_TTL environment
variable: read PREVIEW_TTL (defaulting to "100h" via shell parameter expansion,
e.g. ${PREVIEW_TTL:-100h}) and if PREVIEW_TTL is "0" start supervisord directly
(exec /usr/bin/supervisord ...) otherwise run exec timeout "${PREVIEW_TTL}"
/usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf; keep the DOMAIN
and SUBDOMAIN checks intact so only preview containers use PREVIEW_TTL.
In `@src/client/graphics/layers/InGamePromo.ts`:
- Around line 143-167: hideAd currently only tears down bottom_rail and
left-side units but never removes the corner_ad_video added by
loadCornerAdVideo, so update hideAd to also remove/destroy that unit: when
tearing down, check for window.ramp and push a task to window.ramp.que that
calls the appropriate teardown API (e.g., window.ramp.removeUnits([{ type:
"corner_ad_video" }]) or the corresponding destroy/remove method provided by
RAMP), and wrap it in try/catch to log errors; apply the same removal logic to
the other similar load block (the second corner_ad_video addition) so all corner
videos are properly removed when hiding the layer.
- Around line 17-18: Reset the per-match ad flags inside the InGamePromo.init()
method so each match starts fresh: specifically set bottomRailDestroyed = false
and cornerAdShown = false (and shouldShow if intended) at the beginning of
init(). Locate the InGamePromo class fields (bottomRailDestroyed, cornerAdShown)
and update the init() method to explicitly reinitialize these flags to false to
ensure spawn-phase teardown and post-spawn ad loading run for each reused
instance.
In `@src/client/HomepagePromos.ts`:
- Around line 62-80: The loadBottomRail method currently queues the bottom_rail
ad regardless of the global ad gate; update loadBottomRail to first check
window.adsEnabled (treat undefined as false) and return early without setting
this.bottomRailActive or calling window.ramp.que.push if ads are disabled,
mirroring the gating logic used by onUserMeResponse() and
InGamePromo.showBottomRail(); keep the existing ramp existence checks and error
handling but ensure this.bottomRailActive is only set when ads are enabled and
the ad is actually queued.
- Around line 51-60: close() currently turns off bottomRailActive and adLoaded
but does not unset the visibility gate used by show(), so the fixed gutter
wrappers controlled by isVisible remain mounted and block clicks; update the
close() method to also set the visibility flag (flip whatever isVisible/state
variable the show() method uses) to false when destroying ads (e.g., set
isVisible = false or call the same visibility toggle used by show()), ensuring
handleJoinLobby() calling homepage-promos.close() will fully hide the promo
shell and remove the pointer-active side containers.
🪄 Autofix (Beta)
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
Run ID: 820b3a1f-3f30-40aa-94c4-57ce5e3a1524
📒 Files selected for processing (6)
Dockerfileindex.htmlsrc/client/HomepagePromos.tssrc/client/Main.tssrc/client/graphics/layers/InGamePromo.tssrc/core/configuration/DevConfig.ts
💤 Files with no reviewable changes (1)
- index.html
✅ Files skipped from review due to trivial changes (1)
- src/core/configuration/DevConfig.ts
Comment on lines
79
to
+80
| if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then | ||
| exec timeout 18h /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf | ||
| exec timeout 100h /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find where DOMAIN/SUBDOMAIN and timeout are configured =="
rg -n -C2 'openfront\.dev|DOMAIN|SUBDOMAIN|timeout [0-9]+h|start\.sh|ENTRYPOINT'
echo
echo "== Find runtime restart/health settings =="
rg -n -C2 'restartPolicy|restart:|healthcheck|livenessProbe|readinessProbe|terminationGracePeriodSeconds|backoffLimit|ttlSecondsAfterFinished'
echo
echo "== List likely deployment/config files for manual follow-up =="
fd -i 'dockerfile|compose|deployment|kustomization|helm|values|procfile|\.ya?ml$|\.tf$'Repository: openfrontio/OpenFrontIO
Length of output: 8110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check restart policy assignment in update.sh ==="
sed -n '54,65p' update.sh
echo
echo "=== Search for any existing TTL, timeout, or duration environment variables ==="
rg -n 'TTL|TIMEOUT|DURATION|_TIME' --type sh --type dockerfile
echo
echo "=== Check if supervisord behavior is documented or tested ==="
rg -n 'supervisord' -A 3 -B 1 --type dockerfile
echo
echo "=== Check example.env to understand intended configuration ==="
cat example.envRepository: openfrontio/OpenFrontIO
Length of output: 522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify full restart policy context in update.sh ==="
sed -n '50,75p' update.sh
echo
echo "=== Search for TTL, timeout, duration patterns across all files ==="
rg -i 'TTL|TIMEOUT|DURATION|_TIME' -l
echo
echo "=== Check what supervisord manages in conf.d ==="
rg 'supervisord' -A 5 -B 1
echo
echo "=== Check if any environment-based configuration already exists ==="
rg '\$\{[A-Z_]+:-' | head -20Repository: openfrontio/OpenFrontIO
Length of output: 14030
Make preview TTL configurable instead of hard-coding 100h.
At line 80, preview containers (non-main openfront.dev) force shutdown after 100 hours even if healthy. Combined with RESTART=no in update.sh (line 54–60), this terminates the container and keeps it stopped—interrupting active QA sessions and tests.
A simple environment variable lets you tune or disable this timeout per deployment without rebuilding the image.
Suggested change
if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then
- exec timeout 100h /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf
+ PREVIEW_TTL="${PREVIEW_TTL:-100h}"
+ exec timeout "$PREVIEW_TTL" /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf
else
exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf
fiPass PREVIEW_TTL=0 to disable timeout, or any duration like PREVIEW_TTL=200h.
📝 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.
Suggested change
| if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then | |
| exec timeout 18h /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf | |
| exec timeout 100h /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf | |
| if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then | |
| PREVIEW_TTL="${PREVIEW_TTL:-100h}" | |
| exec timeout "$PREVIEW_TTL" /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 79 - 80, Replace the hard-coded "100h" timeout in
the conditional that checks DOMAIN and SUBDOMAIN with a configurable PREVIEW_TTL
environment variable: read PREVIEW_TTL (defaulting to "100h" via shell parameter
expansion, e.g. ${PREVIEW_TTL:-100h}) and if PREVIEW_TTL is "0" start
supervisord directly (exec /usr/bin/supervisord ...) otherwise run exec timeout
"${PREVIEW_TTL}" /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf; keep the DOMAIN and SUBDOMAIN checks
intact so only preview containers use PREVIEW_TTL.
Comment on lines
17
to
+18
| private shouldShow: boolean = false; | ||
| private bottomRailActive: boolean = false; | ||
| private bottomRailDestroyed: boolean = false; |
Contributor
There was a problem hiding this comment.
Reset the per-match ad flags in init().
bottomRailDestroyed and cornerAdShown are only initialized once. After one completed game they stay true, so a reused in-game-promo instance will skip the next match's spawn-phase teardown and post-spawn ad load.
Proposed fix
init() {
+ this.shouldShow = false;
+ this.bottomRailDestroyed = false;
+ this.cornerAdShown = false;
this.showBottomRail();
}Also applies to: 25-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/InGamePromo.ts` around lines 17 - 18, Reset the
per-match ad flags inside the InGamePromo.init() method so each match starts
fresh: specifically set bottomRailDestroyed = false and cornerAdShown = false
(and shouldShow if intended) at the beginning of init(). Locate the InGamePromo
class fields (bottomRailDestroyed, cornerAdShown) and update the init() method
to explicitly reinitialize these flags to false to ensure spawn-phase teardown
and post-spawn ad loading run for each reused instance.
Comment on lines
143
to
167
| private loadCornerAdVideo(): void { | ||
| if (window.innerWidth < 1280) return; | ||
| if (!window.ramp) { | ||
| console.warn("Playwire RAMP not available for corner_ad_video"); | ||
| return; | ||
| } | ||
| try { | ||
| window.ramp.que.push(() => { | ||
| try { | ||
| window.ramp | ||
| .addUnits([{ type: "corner_ad_video" }]) | ||
| .then(() => { | ||
| window.ramp.displayUnits(); | ||
| console.log("In-game corner_ad_video loaded"); | ||
| }) | ||
| .catch((e: unknown) => { | ||
| console.error("Failed to display in-game corner_ad_video:", e); | ||
| }); | ||
| } catch (e) { | ||
| console.error("Failed to add in-game corner_ad_video:", e); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to load in-game corner_ad_video:", error); | ||
| } |
Contributor
There was a problem hiding this comment.
Destroy corner_ad_video when the layer hides.
The new video unit is added in loadCornerAdVideo(), but hideAd() only tears down bottom_rail and the left-side units. That can leave the corner video alive over the next screen or next match.
Proposed fix
public hideAd(): void {
this.destroyBottomRail();
+ this.cornerAdShown = false;
if (crazyGamesSDK.isOnCrazyGames()) {
crazyGamesSDK.clearBottomLeftAd();
this.shouldShow = false;
this.requestUpdate();
@@
try {
for (const { type } of AD_TYPES) {
window.ramp.destroyUnits(type);
}
+ window.ramp.destroyUnits("corner_ad_video");
console.log("successfully destroyed in-game bottom-left ads");
} catch (e) {
console.error("error destroying in-game ads:", e);
}Also applies to: 170-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/InGamePromo.ts` around lines 143 - 167, hideAd
currently only tears down bottom_rail and left-side units but never removes the
corner_ad_video added by loadCornerAdVideo, so update hideAd to also
remove/destroy that unit: when tearing down, check for window.ramp and push a
task to window.ramp.que that calls the appropriate teardown API (e.g.,
window.ramp.removeUnits([{ type: "corner_ad_video" }]) or the corresponding
destroy/remove method provided by RAMP), and wrap it in try/catch to log errors;
apply the same removal logic to the other similar load block (the second
corner_ad_video addition) so all corner videos are properly removed when hiding
the layer.
Comment on lines
51
to
+60
| public close(): void { | ||
| this.bottomRailActive = false; | ||
| this.adLoaded = false; | ||
| try { | ||
| window.ramp.spaAds({ ads: [], countPageview: false }); | ||
| console.log("successfully destroyed all homepage ads"); | ||
| } catch (e) { | ||
| console.error("error destroying homepage ads", e); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Hide the promo shell inside close().
show() gates the fixed gutter wrappers behind isVisible, but close() never flips that flag back. handleJoinLobby() still calls homepage-promos.close(), so the empty, pointer-active side containers stay mounted after the ads are destroyed and can keep blocking clicks near the page edges.
Proposed fix
public close(): void {
+ this.isVisible = false;
this.bottomRailActive = false;
this.adLoaded = false;
+ this.cornerAdLoaded = false;
+ this.requestUpdate();
try {
window.ramp.spaAds({ ads: [], countPageview: false });
console.log("successfully destroyed all homepage ads");📝 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.
Suggested change
| public close(): void { | |
| this.bottomRailActive = false; | |
| this.adLoaded = false; | |
| try { | |
| window.ramp.spaAds({ ads: [], countPageview: false }); | |
| console.log("successfully destroyed all homepage ads"); | |
| } catch (e) { | |
| console.error("error destroying homepage ads", e); | |
| } | |
| } | |
| public close(): void { | |
| this.isVisible = false; | |
| this.bottomRailActive = false; | |
| this.adLoaded = false; | |
| this.cornerAdLoaded = false; | |
| this.requestUpdate(); | |
| try { | |
| window.ramp.spaAds({ ads: [], countPageview: false }); | |
| console.log("successfully destroyed all homepage ads"); | |
| } catch (e) { | |
| console.error("error destroying homepage ads", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/HomepagePromos.ts` around lines 51 - 60, close() currently turns
off bottomRailActive and adLoaded but does not unset the visibility gate used by
show(), so the fixed gutter wrappers controlled by isVisible remain mounted and
block clicks; update the close() method to also set the visibility flag (flip
whatever isVisible/state variable the show() method uses) to false when
destroying ads (e.g., set isVisible = false or call the same visibility toggle
used by show()), ensuring handleJoinLobby() calling homepage-promos.close() will
fully hide the promo shell and remove the pointer-active side containers.
Comment on lines
+62
to
+80
| public loadBottomRail(): void { | ||
| if (!window.ramp) { | ||
| console.warn("Playwire RAMP not available for bottom_rail ad"); | ||
| return; | ||
| } | ||
|
|
||
| this.bottomRailActive = true; | ||
| try { | ||
| window.ramp.que.push(() => { | ||
| try { | ||
| window.ramp.spaAddAds([{ type: "bottom_rail" }]); | ||
| console.log("Bottom rail ad loaded"); | ||
| } catch (e) { | ||
| console.error("Failed to add bottom_rail ad:", e); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to load bottom_rail ad:", error); | ||
| } |
Contributor
There was a problem hiding this comment.
Respect window.adsEnabled before queuing bottom_rail.
This path skips the same ad gate used by onUserMeResponse() and InGamePromo.showBottomRail(). Right now a lobby join can still queue bottom_rail for sessions that are supposed to stay ad-free.
Proposed fix
public loadBottomRail(): void {
+ if (!window.adsEnabled) {
+ return;
+ }
+
if (!window.ramp) {
console.warn("Playwire RAMP not available for bottom_rail ad");
return;
}📝 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.
Suggested change
| public loadBottomRail(): void { | |
| if (!window.ramp) { | |
| console.warn("Playwire RAMP not available for bottom_rail ad"); | |
| return; | |
| } | |
| this.bottomRailActive = true; | |
| try { | |
| window.ramp.que.push(() => { | |
| try { | |
| window.ramp.spaAddAds([{ type: "bottom_rail" }]); | |
| console.log("Bottom rail ad loaded"); | |
| } catch (e) { | |
| console.error("Failed to add bottom_rail ad:", e); | |
| } | |
| }); | |
| } catch (error) { | |
| console.error("Failed to load bottom_rail ad:", error); | |
| } | |
| public loadBottomRail(): void { | |
| if (!window.adsEnabled) { | |
| return; | |
| } | |
| if (!window.ramp) { | |
| console.warn("Playwire RAMP not available for bottom_rail ad"); | |
| return; | |
| } | |
| this.bottomRailActive = true; | |
| try { | |
| window.ramp.que.push(() => { | |
| try { | |
| window.ramp.spaAddAds([{ type: "bottom_rail" }]); | |
| console.log("Bottom rail ad loaded"); | |
| } catch (e) { | |
| console.error("Failed to add bottom_rail ad:", e); | |
| } | |
| }); | |
| } catch (error) { | |
| console.error("Failed to load bottom_rail ad:", error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/HomepagePromos.ts` around lines 62 - 80, The loadBottomRail method
currently queues the bottom_rail ad regardless of the global ad gate; update
loadBottomRail to first check window.adsEnabled (treat undefined as false) and
return early without setting this.bottomRailActive or calling
window.ramp.que.push if ads are disabled, mirroring the gating logic used by
onUserMeResponse() and InGamePromo.showBottomRail(); keep the existing ramp
existence checks and error handling but ensure this.bottomRailActive is only set
when ads are enabled and the ad is actually queued.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/client/HomepagePromos.ts (2)
63-70:⚠️ Potential issue | 🟠 MajorRespect
window.adsEnabledbefore queuingbottom_rail.Right now this queues ads even for sessions that should be ad-free.
Proposed fix
public loadBottomRail(): void { + if (!window.adsEnabled || this.bottomRailActive) { + return; + } + if (!window.ramp) { console.warn("Playwire RAMP not available for bottom_rail ad"); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HomepagePromos.ts` around lines 63 - 70, The loadBottomRail method currently queues the bottom_rail ad regardless of ad-free sessions; update loadBottomRail to first check window.adsEnabled (and treat undefined as true only if your app policy allows) and return early without setting this.bottomRailActive or calling window.ramp.queue if ads are disabled; specifically, add a guard at the top of loadBottomRail that verifies window.adsEnabled before the existing window.ramp and bottomRailActive logic and only proceed to queue "bottom_rail" when ads are enabled.
51-57:⚠️ Potential issue | 🟠 Major
close()should also hide the promo shell state.
show()usesisVisibleto render the wrappers, butclose()never resets it. Empty gutter wrappers can remain mounted and block clicks near edges.Proposed fix
public close(): void { + this.isVisible = false; this.adLoaded = false; + this.cornerAdLoaded = false; + this.requestUpdate(); try { // Only destroy gutter ads; bottom_rail persists into spawn phase. window.ramp.destroyUnits(this.leftAdType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HomepagePromos.ts` around lines 51 - 57, The close() method currently only flips adLoaded and destroys gutter units but never resets the promo shell render flag; update close() (alongside its existing adLoaded=false and destroyUnits calls for leftAdType/rightAdType) to also set this.isVisible = false so the wrappers rendered by show() are unmounted and stop blocking clicks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/HomepagePromos.ts`:
- Around line 90-93: The try/catch around the async call
window.ramp.destroyUnits("bottom_rail") will not catch Promise rejections;
update the call to handle its Promise (either await the call by making the
enclosing function async, or attach a .catch() handler) and log the error via
process/logger/console inside the rejection handler; do the same fix for the
analogous call in InGamePromo (where destroyUnits is called) so both Promise
rejections are properly handled and errors are logged.
In `@src/client/Main.ts`:
- Around line 872-875: The bottom-rail cleanup (calling
HomepagePromos.destroyBottomRail on the homepagePromos instance) must run even
if this.lobbyHandle is null; move or copy the block that queries
document.querySelector("homepage-promos") and calls
homepagePromos.destroyBottomRail() to execute before the early-return/null-guard
around this.lobbyHandle in the leave-lobby handler (or ensure the handler always
invokes that cleanup regardless of this.lobbyHandle). Locate the code around the
leave-lobby handler where this.lobbyHandle is checked and adjust so
HomepagePromos.destroyBottomRail is invoked unconditionally prior to returning.
---
Duplicate comments:
In `@src/client/HomepagePromos.ts`:
- Around line 63-70: The loadBottomRail method currently queues the bottom_rail
ad regardless of ad-free sessions; update loadBottomRail to first check
window.adsEnabled (and treat undefined as true only if your app policy allows)
and return early without setting this.bottomRailActive or calling
window.ramp.queue if ads are disabled; specifically, add a guard at the top of
loadBottomRail that verifies window.adsEnabled before the existing window.ramp
and bottomRailActive logic and only proceed to queue "bottom_rail" when ads are
enabled.
- Around line 51-57: The close() method currently only flips adLoaded and
destroys gutter units but never resets the promo shell render flag; update
close() (alongside its existing adLoaded=false and destroyUnits calls for
leftAdType/rightAdType) to also set this.isVisible = false so the wrappers
rendered by show() are unmounted and stop blocking clicks.
🪄 Autofix (Beta)
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
Run ID: 454f7c2c-b6f5-4f36-b3d9-0dbe7697631b
📒 Files selected for processing (6)
Dockerfileindex.htmlsrc/client/HomepagePromos.tssrc/client/Main.tssrc/client/graphics/layers/InGamePromo.tssrc/core/configuration/DevConfig.ts
💤 Files with no reviewable changes (1)
- index.html
✅ Files skipped from review due to trivial changes (2)
- Dockerfile
- src/core/configuration/DevConfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/InGamePromo.ts
Comment on lines
+90
to
+93
| try { | ||
| window.ramp.destroyUnits("bottom_rail"); | ||
| console.log("Bottom rail ad destroyed"); | ||
| } catch (e) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Promise-returning signature and current call handling.
rg -n -C2 'destroyUnits\s*:\s*\(adType:\s*string\s*\|\s*string\[\]\)\s*=>\s*Promise<void>' src/client/Main.ts
rg -n -C3 'destroyUnits\("bottom_rail"\)' src/client/HomepagePromos.ts src/client/graphics/layers/InGamePromo.tsRepository: openfrontio/OpenFrontIO
Length of output: 1284
🏁 Script executed:
#!/bin/bash
# Check the function context around the destroyUnits calls
echo "=== HomepagePromos.ts context ==="
sed -n '80,100p' src/client/HomepagePromos.ts
echo -e "\n=== InGamePromo.ts context ==="
sed -n '35,50p' src/client/graphics/layers/InGamePromo.ts
# Check if there are any .catch() chains after these calls
echo -e "\n=== Check for .catch chains ==="
rg -n 'destroyUnits.*\.catch|\.then.*catch' src/client/Repository: openfrontio/OpenFrontIO
Length of output: 1123
Handle async destroyUnits failures with .catch() or await.
window.ramp.destroyUnits("bottom_rail") returns a Promise. A sync try/catch will not catch Promise rejection, so failures become unhandled. Either add .catch() to handle the rejection inline, or make the parent function async and await the call.
This issue appears in:
src/client/HomepagePromos.ts:91src/client/graphics/layers/InGamePromo.ts:45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/HomepagePromos.ts` around lines 90 - 93, The try/catch around the
async call window.ramp.destroyUnits("bottom_rail") will not catch Promise
rejections; update the call to handle its Promise (either await the call by
making the enclosing function async, or attach a .catch() handler) and log the
error via process/logger/console inside the rejection handler; do the same fix
for the analogous call in InGamePromo (where destroyUnits is called) so both
Promise rejections are properly handled and errors are logged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/client/HomepagePromos.ts (3)
53-56:⚠️ Potential issue | 🔴 CriticalHandle
destroyUnits()as async.The calls here are still wrapped in sync
try/catch. IfdestroyUnits()rejects, the failure is unhandled whileadLoadedandbottomRailActiveare already set tofalse, so the component can think the slots are gone when teardown actually failed.#!/bin/bash echo "=== destroyUnits type ===" rg -n -C2 'destroyUnits\s*:\s*\(adType:\s*string\s*\|\s*string\[\]\)\s*=>\s*Promise<void>' src echo echo "=== HomepagePromos destroyUnits call sites ===" rg -n -C2 'destroyUnits\((this\.leftAdType|this\.rightAdType|"bottom_rail")\)' src/client/HomepagePromos.tsExpected result: the first search should show the Promise-returning signature, and the second should show these un-awaited call sites. If that matches, these paths need
awaitor.catch()before logging success.Also applies to: 90-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HomepagePromos.ts` around lines 53 - 56, The calls to window.ramp.destroyUnits in HomepagePromos are async but currently invoked without awaiting or handling rejections, so failures can be unobserved while state (adLoaded, bottomRailActive) is flipped; update the teardown in the HomepagePromos method that calls window.ramp.destroyUnits(this.leftAdType) and window.ramp.destroyUnits(this.rightAdType) (and the similar calls at lines ~90-93) to await each Promise or attach .catch() handlers and only set adLoaded/bottomRailActive to false after successful completion (or revert state on error), ensuring errors are logged via processLogger or console.error and not left unhandled.
63-70:⚠️ Potential issue | 🟠 MajorRespect
window.adsEnabledbefore loadingbottom_rail.This path still skips the same ad gate used in
onUserMeResponse(). That means a lobby join can queuebottom_railfor ad-free sessions, and it setsbottomRailActiveeven when ads should stay off.Suggested fix
public loadBottomRail(): void { + if (!window.adsEnabled) { + return; + } + if (!window.ramp) { console.warn("Playwire RAMP not available for bottom_rail ad"); return; } this.bottomRailActive = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HomepagePromos.ts` around lines 63 - 70, loadBottomRail currently activates and queues bottom_rail without checking the global ad gate; before touching window.ramp or setting this.bottomRailActive, mirror the same ad gating used in onUserMeResponse by checking window.adsEnabled (or the project's canonical ads flag) and return early if ads are disabled, ensuring you do not set this.bottomRailActive or call ramp.load for "bottom_rail" when window.adsEnabled is false.
51-52:⚠️ Potential issue | 🟠 MajorHide the promo shell in
close().Line 51 only resets
adLoaded.show()mounts the fixed gutter wrappers behindisVisible, butclose()never clears that flag or triggers a re-render, so the empty wrappers can stay on screen and block clicks after join starts.Suggested fix
public close(): void { + this.isVisible = false; this.adLoaded = false; + this.requestUpdate(); try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HomepagePromos.ts` around lines 51 - 52, The close() method currently only clears adLoaded and leaves the mounted fixed gutter wrappers visible; update close() to also clear the visibility flag and trigger the same re-render/unmount path that show() uses so the wrappers are removed — i.e., set this.isVisible = false and then invoke the component's UI-update routine (for React components use this.setState({isVisible: false}) or this.forceUpdate(), otherwise call the existing DOM update/unmount method used by show()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/HomepagePromos.ts`:
- Around line 51-56: In the HomepagePromos.close() method, add teardown for the
corner video by calling window.ramp.destroyUnits("corner_ad_video") alongside
the existing gutter destroys and reset the state by setting this.cornerAdLoaded
= false; ensure the destroy call is inside the existing try block (with the
other window.ramp.destroyUnits calls) so the corner unit is removed and the
cornerAdLoaded flag is cleared when close() is invoked.
---
Duplicate comments:
In `@src/client/HomepagePromos.ts`:
- Around line 53-56: The calls to window.ramp.destroyUnits in HomepagePromos are
async but currently invoked without awaiting or handling rejections, so failures
can be unobserved while state (adLoaded, bottomRailActive) is flipped; update
the teardown in the HomepagePromos method that calls
window.ramp.destroyUnits(this.leftAdType) and
window.ramp.destroyUnits(this.rightAdType) (and the similar calls at lines
~90-93) to await each Promise or attach .catch() handlers and only set
adLoaded/bottomRailActive to false after successful completion (or revert state
on error), ensuring errors are logged via processLogger or console.error and not
left unhandled.
- Around line 63-70: loadBottomRail currently activates and queues bottom_rail
without checking the global ad gate; before touching window.ramp or setting
this.bottomRailActive, mirror the same ad gating used in onUserMeResponse by
checking window.adsEnabled (or the project's canonical ads flag) and return
early if ads are disabled, ensuring you do not set this.bottomRailActive or call
ramp.load for "bottom_rail" when window.adsEnabled is false.
- Around line 51-52: The close() method currently only clears adLoaded and
leaves the mounted fixed gutter wrappers visible; update close() to also clear
the visibility flag and trigger the same re-render/unmount path that show() uses
so the wrappers are removed — i.e., set this.isVisible = false and then invoke
the component's UI-update routine (for React components use
this.setState({isVisible: false}) or this.forceUpdate(), otherwise call the
existing DOM update/unmount method used by show()).
🪄 Autofix (Beta)
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
Run ID: a56c36d9-3d89-46f7-932c-1bbf575e9255
📒 Files selected for processing (6)
Dockerfileindex.htmlsrc/client/HomepagePromos.tssrc/client/Main.tssrc/client/graphics/layers/InGamePromo.tssrc/core/configuration/DevConfig.ts
💤 Files with no reviewable changes (1)
- index.html
✅ Files skipped from review due to trivial changes (3)
- Dockerfile
- src/core/configuration/DevConfig.ts
- src/client/Main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/InGamePromo.ts
Comment on lines
51
to
56
| public close(): void { | ||
| this.adLoaded = false; | ||
| try { | ||
| // Keep corner video ad alive. | ||
| // Only destroy gutter ads; bottom_rail persists into spawn phase. | ||
| window.ramp.destroyUnits(this.leftAdType); | ||
| window.ramp.destroyUnits(this.rightAdType); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== corner_ad_video lifecycle in HomepagePromos ==="
rg -n -C2 'corner_ad_video|cornerAdLoaded|loadCornerAdVideo|public close\(' src/client/HomepagePromos.ts
echo
echo "=== close() call during join flow ==="
sed -n '773,789p' src/client/Main.ts
echo
echo "=== repo-wide teardown references for corner_ad_video ==="
rg -n -C2 'destroyUnits\("corner_ad_video"\)|destroyUnits\(.*corner_ad_video|removeUnits\(.*corner_ad_video' srcRepository: openfrontio/OpenFrontIO
Length of output: 2098
Destroy corner_ad_video and reset its flag when close() is called.
loadCornerAdVideo() adds the corner video unit via window.ramp.addUnits() and sets cornerAdLoaded = true, but close() only destroys the gutter ads and never resets cornerAdLoaded or removes the corner unit. Since Main.ts calls close() when the game starts, the corner video remains active with no teardown path.
Add cleanup logic to close():
- Call
window.ramp.destroyUnits("corner_ad_video") - Reset
this.cornerAdLoaded = false
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/HomepagePromos.ts` around lines 51 - 56, In the
HomepagePromos.close() method, add teardown for the corner video by calling
window.ramp.destroyUnits("corner_ad_video") alongside the existing gutter
destroys and reset the state by setting this.cornerAdLoaded = false; ensure the
destroy call is inside the existing try block (with the other
window.ramp.destroyUnits calls) so the corner unit is removed and the
cornerAdLoaded flag is cleared when close() is invoked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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