Refactor home page ads into a single file, add corner video ad#3601
Merged
Refactor home page ads into a single file, add corner video ad#3601
Conversation
Contributor
WalkthroughThe PR consolidates two separate ad-handling components ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/client/Main.ts (1)
247-247: RenamegutterAdstohomepageAdsfor clearer intent.The current name reflects the old component and can confuse future changes.
Proposed refactor
- private gutterAds: HomepageAds; + private homepageAds: HomepageAds; ... - const gutterAds = document.querySelector("homepage-ads"); - if (!(gutterAds instanceof HomepageAds)) + const homepageAds = document.querySelector("homepage-ads"); + if (!(homepageAds instanceof HomepageAds)) throw new Error("Missing homepage-ads"); - this.gutterAds = gutterAds; + this.homepageAds = homepageAds;Also applies to: 307-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Main.ts` at line 247, Rename the private field gutterAds to homepageAds to reflect the new component name: update the declaration "private gutterAds: HomepageAds" to "private homepageAds: HomepageAds" and then rename all references/usages of gutterAds (constructor assignments, methods, event handlers, and any access at lines around the later usage block) to homepageAds; ensure imports/types still reference HomepageAds and update any tests or view bindings that reference the old name so compilation and runtime references are consistent.
🤖 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/HomepageAds.ts`:
- Around line 61-69: close() currently only destroys gutter ads; update it to
also destroy the corner video and fully reset state so ads can be reloaded: call
the appropriate destroy on the corner_ad_video created in loadCornerAdVideo()
(and/or window.ramp.destroyUnits for its type), set this.corner_ad_video to
null, set this.adLoaded = false so loadGutterAds() can run again, and set
this.isVisible = false to mark the component hidden; keep destroying leftAdType
and rightAdType as before (leftAdType/rightAdType).
- Around line 195-207: Add a guard boolean to prevent duplicate footer ads: add
a class-level flag (e.g., footerAdLoaded or footerAdShown) and in
onUserMeResponse check that flag before setting this.shouldShow and queuing
this.loadAd() via this.updateComplete; if the flag is false, set it true when
you schedule/load the ad so subsequent userMeResponse events do nothing. Use the
same guard pattern as HomeFooterAd and reference onUserMeResponse,
this.updateComplete, this.loadAd, and this.shouldShow when making the change.
- Around line 33-45: The inline document.addEventListener("userMeResponse", ...)
in connectedCallback adds a handler that cannot be removed on disconnect;
extract that listener into a class property (e.g., this._onUserMeResponse) and
use it when adding the event in connectedCallback and removing it in
disconnectedCallback so handlers don't accumulate on reconnects; keep the
handler body calling this.show() and this.loadCornerAdVideo() conditionally on
window.adsEnabled (mirror HomeFooterAd's pattern) and ensure you reference the
same function identity for both addEventListener and removeEventListener.
---
Nitpick comments:
In `@src/client/Main.ts`:
- Line 247: Rename the private field gutterAds to homepageAds to reflect the new
component name: update the declaration "private gutterAds: HomepageAds" to
"private homepageAds: HomepageAds" and then rename all references/usages of
gutterAds (constructor assignments, methods, event handlers, and any access at
lines around the later usage block) to homepageAds; ensure imports/types still
reference HomepageAds and update any tests or view bindings that reference the
old name so compilation and runtime references are consistent.
🪄 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: b09e7a6c-9951-4824-b016-0f56c8585197
📒 Files selected for processing (5)
index.htmlsrc/client/GutterAds.tssrc/client/HomepageAds.tssrc/client/Main.tssrc/client/components/HomeFooterAd.ts
💤 Files with no reviewable changes (2)
- src/client/components/HomeFooterAd.ts
- src/client/GutterAds.ts
Comment on lines
+33
to
+45
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| this.onResize(); | ||
| window.addEventListener("resize", this.onResize); | ||
| document.addEventListener("userMeResponse", () => { | ||
| if (window.adsEnabled) { | ||
| console.log("showing homepage ads"); | ||
| this.show(); | ||
| this.loadCornerAdVideo(); | ||
| } else { | ||
| console.log("not showing homepage ads"); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l src/client/HomepageAds.tsRepository: openfrontio/OpenFrontIO
Length of output: 96
🏁 Script executed:
# Read the entire file to understand the structure
cat -n src/client/HomepageAds.tsRepository: openfrontio/OpenFrontIO
Length of output: 8882
Extract userMeResponse handler to a class property so it can be removed in disconnectedCallback.
The current inline callback cannot be removed, so if the component reconnects, new handlers stack on top of old ones. Each reconnect re-runs show() and loadCornerAdVideo(), causing duplicate ad operations. See how HomeFooterAd (line 195) implements this correctly.
Proposed fix
export class HomepageAds extends LitElement {
`@state`() private isVisible: boolean = false;
`@state`() private adLoaded: boolean = false;
`@state`() private hasFooterAd: boolean = false;
private onResize = () => {
const isDesktop = window.innerWidth >= 640;
this.hasFooterAd = isDesktop && window.innerHeight >= FOOTER_AD_MIN_HEIGHT;
};
+ private onUserMeResponse = () => {
+ if (!window.adsEnabled) return;
+ console.log("showing homepage ads");
+ this.show();
+ this.loadCornerAdVideo();
+ };
connectedCallback() {
super.connectedCallback();
this.onResize();
window.addEventListener("resize", this.onResize);
- document.addEventListener("userMeResponse", () => {
- if (window.adsEnabled) {
- console.log("showing homepage ads");
- this.show();
- this.loadCornerAdVideo();
- } else {
- console.log("not showing homepage ads");
- }
- });
+ document.addEventListener("userMeResponse", this.onUserMeResponse);
}
disconnectedCallback() {
super.disconnectedCallback();
window.removeEventListener("resize", this.onResize);
+ document.removeEventListener("userMeResponse", this.onUserMeResponse);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/HomepageAds.ts` around lines 33 - 45, The inline
document.addEventListener("userMeResponse", ...) in connectedCallback adds a
handler that cannot be removed on disconnect; extract that listener into a class
property (e.g., this._onUserMeResponse) and use it when adding the event in
connectedCallback and removing it in disconnectedCallback so handlers don't
accumulate on reconnects; keep the handler body calling this.show() and
this.loadCornerAdVideo() conditionally on window.adsEnabled (mirror
HomeFooterAd's pattern) and ensure you reference the same function identity for
both addEventListener and removeEventListener.
Comment on lines
+195
to
+207
| private onUserMeResponse = () => { | ||
| const isDesktop = window.innerWidth >= 640; | ||
| if ( | ||
| !window.adsEnabled || | ||
| (isDesktop && window.innerHeight < FOOTER_AD_MIN_HEIGHT) | ||
| ) { | ||
| return; | ||
| } | ||
| this.shouldShow = true; | ||
| this.updateComplete.then(() => { | ||
| this.loadAd(); | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the HomepageAds.ts file to understand the class structure
cat -n src/client/HomepageAds.ts | head -250Repository: openfrontio/OpenFrontIO
Length of output: 8862
🏁 Script executed:
# Search for userMeResponse binding/registration to understand when it fires
rg "userMeResponse|addEventListener" src/client/HomepageAds.ts -n -A 3Repository: openfrontio/OpenFrontIO
Length of output: 532
🏁 Script executed:
# Search for spaAddAds calls to see all paths that might load ads
rg "spaAddAds" src/client/HomepageAds.ts -n -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 614
Add guard to prevent duplicate footer ad loads on repeated auth events.
The userMeResponse event can fire multiple times while the component is connected. Each fire currently triggers spaAddAds without protection, creating duplicate ad units. The HomepageAds class already has this guard pattern—HomeFooterAd should use the same approach.
Proposed fix
export class HomeFooterAd extends LitElement {
`@state`() private shouldShow: boolean = false;
+ private adLoaded = false;
private onUserMeResponse = () => {
+ if (this.adLoaded) return;
const isDesktop = window.innerWidth >= 640;
if (
!window.adsEnabled ||
(isDesktop && window.innerHeight < FOOTER_AD_MIN_HEIGHT)
) {
return;
}
this.shouldShow = true;
this.updateComplete.then(() => {
this.loadAd();
});
};
private loadAd(): void {
+ if (this.adLoaded) return;
if (!window.ramp) {
console.warn("Playwire RAMP not available for footer ad");
return;
}
try {
window.ramp.que.push(() => {
try {
window.ramp.spaAddAds([
{ type: FOOTER_AD_TYPE, selectorId: FOOTER_AD_CONTAINER_ID },
]);
+ this.adLoaded = true;
console.log("Footer ad loaded:", FOOTER_AD_TYPE);
} catch (e) {
console.error("Failed to add footer ad:", e);
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/HomepageAds.ts` around lines 195 - 207, Add a guard boolean to
prevent duplicate footer ads: add a class-level flag (e.g., footerAdLoaded or
footerAdShown) and in onUserMeResponse check that flag before setting
this.shouldShow and queuing this.loadAd() via this.updateComplete; if the flag
is false, set it true when you schedule/load the ad so subsequent userMeResponse
events do nothing. Use the same guard pattern as HomeFooterAd and reference
onUserMeResponse, this.updateComplete, this.loadAd, and this.shouldShow when
making the change.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/HomepagePromos.ts (1)
27-31: Remove unusedstatic stylesdeclaration.Since
createRenderRoot()returnsthis(disabling shadow DOM), thestatic stylesproperty is never applied. This is dead code.Proposed fix
createRenderRoot() { return this; } - - static styles = css``;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HomepagePromos.ts` around lines 27 - 31, Remove the dead static styles declaration from the HomepagePromos component: since createRenderRoot() returns this (disabling shadow DOM), the static styles property on the HomepagePromos class is never applied, so delete the static styles = css`` declaration (or alternatively move the styles into global CSS or apply them manually in render) and ensure no other code references HomepagePromos.static styles.
🤖 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 109-134: loadCornerAdVideo lacks the duplicate-load guard used in
loadGutterAds, so repeated userMeResponse events can enqueue multiple
"corner_ad_video" units; add a boolean flag (e.g., adLoadedCorner or reuse
adLoaded with a separate key) checked at the top of loadCornerAdVideo to return
early if already loaded, set that flag to true once
window.ramp.addUnits(...).then(...) succeeds (or immediately after addUnits
resolves), and leave the rest of the existing try/catch logic intact to avoid
enqueuing duplicate corner_ad_video units.
---
Nitpick comments:
In `@src/client/HomepagePromos.ts`:
- Around line 27-31: Remove the dead static styles declaration from the
HomepagePromos component: since createRenderRoot() returns this (disabling
shadow DOM), the static styles property on the HomepagePromos class is never
applied, so delete the static styles = css`` declaration (or alternatively move
the styles into global CSS or apply them manually in render) and ensure no other
code references HomepagePromos.static styles.
🪄 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: fc90a5a0-3274-460e-a317-47e5b1489e2f
📒 Files selected for processing (5)
index.htmlsrc/client/GutterAds.tssrc/client/HomepagePromos.tssrc/client/Main.tssrc/client/components/HomeFooterAd.ts
💤 Files with no reviewable changes (2)
- src/client/components/HomeFooterAd.ts
- src/client/GutterAds.ts
✅ Files skipped from review due to trivial changes (1)
- index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Main.ts
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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.
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan