Conversation
WalkthroughThis change updates GutterAds to set Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 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 76-89: The close() method leaves this.isVisible true so the
component can reload ads on the next resize; update close() (in GutterAds) to
set this.isVisible = false (or otherwise mark the component as closed) and
ensure any flags that allow handleViewportChange() to call loadAds() are
disabled; specifically modify close() to set this.isVisible = false and stop any
probes so handleViewportChange() will no longer call loadAds() after close().
- Around line 175-194: The interval started in startNoContentCheck() never gets
cleared when ads load successfully; update startNoContentCheck() so that when
noContentCheckCount >= 10 you always call stopNoContentCheck() (move the
stopNoContentCheck() call out of the !this.adLoaded branch), then set
this.isProbingAds = false and this.adLoaded = this.leftAdVisible ||
this.rightAdVisible and, if !this.adLoaded, set this.isVisible = false; ensure
references to stopNoContentCheck(), noContentCheckCount, isProbingAds, and
adLoaded in the startNoContentCheck() function are used accordingly.
🧹 Nitpick comments (2)
src/client/GutterAds.ts (2)
39-53: Missing cleanup foruserMeResponselistenerThe
"userMeResponse"event listener added todocumentis never removed indisconnectedCallback. If the element is ever removed and re-added, this stacks up duplicate listeners. Store the handler as a class field and remove it on disconnect, similar to what you do withhandleViewportChange.Proposed fix
Add a field for the handler and wire up removal:
+ private handleUserMeResponse = () => { + if (window.adsEnabled) { + console.log("showing gutter ads"); + this.show(); + } else { + console.log("not showing gutter ads"); + } + }; + connectedCallback() { super.connectedCallback(); this.xlMediaQuery = window.matchMedia("(min-width: 1280px)"); this.isXlViewport = this.xlMediaQuery.matches; this.xlMediaQuery.addEventListener("change", this.handleViewportChange); - - document.addEventListener("userMeResponse", () => { - if (window.adsEnabled) { - console.log("showing gutter ads"); - this.show(); - } else { - console.log("not showing gutter ads"); - } - }); + document.addEventListener("userMeResponse", this.handleUserMeResponse); }disconnectedCallback() { this.xlMediaQuery?.removeEventListener("change", this.handleViewportChange); this.xlMediaQuery = null; this.stopNoContentCheck(); + document.removeEventListener("userMeResponse", this.handleUserMeResponse); super.disconnectedCallback(); }
215-224: Duplicated visible-class string — extract to a shared constant
leftClassandrightClassuse the same long class string when the ad is visible. Pull it into a local constant to keep things tidy and avoid drift.Proposed refactor
+ const visibleClass = + "fixed flex transform -translate-y-1/2 w-[160px] min-h-[600px] z-[100] pointer-events-auto items-center justify-center"; const probeClass = "absolute w-px h-px overflow-hidden pointer-events-none opacity-0"; - const leftClass = this.leftAdVisible - ? "fixed flex transform -translate-y-1/2 w-[160px] min-h-[600px] z-[100] pointer-events-auto items-center justify-center" - : probeClass; - const rightClass = this.rightAdVisible - ? "fixed flex transform -translate-y-1/2 w-[160px] min-h-[600px] z-[100] pointer-events-auto items-center justify-center" - : probeClass; + const leftClass = this.leftAdVisible ? visibleClass : probeClass; + const rightClass = this.rightAdVisible ? visibleClass : probeClass;
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
ryanbarlow97
added
UI/UX
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/GutterAds.ts (1)
39-53:⚠️ Potential issue | 🟡 MinorEvent listener for "userMeResponse" is never removed
The
"userMeResponse"listener is added inconnectedCallbackbut not cleaned up indisconnectedCallback. If this element is ever removed and re-added, a new listener stacks on top of the old one.Store the handler as a class field and remove it on disconnect, same as you do for
handleViewportChange.Proposed fix
+ private handleUserMeResponse = () => { + if (window.adsEnabled) { + console.log("showing gutter ads"); + this.show(); + } else { + console.log("not showing gutter ads"); + } + }; + connectedCallback() { super.connectedCallback(); this.xlMediaQuery = window.matchMedia("(min-width: 1280px)"); this.isXlViewport = this.xlMediaQuery.matches; this.xlMediaQuery.addEventListener("change", this.handleViewportChange); - document.addEventListener("userMeResponse", () => { - if (window.adsEnabled) { - console.log("showing gutter ads"); - this.show(); - } else { - console.log("not showing gutter ads"); - } - }); + document.addEventListener("userMeResponse", this.handleUserMeResponse); }disconnectedCallback() { this.xlMediaQuery?.removeEventListener("change", this.handleViewportChange); this.xlMediaQuery = null; this.stopNoContentCheck(); + document.removeEventListener("userMeResponse", this.handleUserMeResponse); super.disconnectedCallback(); }
🤖 Fix all issues with AI agents
In `@src/client/GutterAds.ts`:
- Around line 93-107: The viewport-change handler (handleViewportChange)
currently only calls close() when !isXlViewport && adLoaded, which lets a
pending probe (isProbingAds true) complete and start the no-content timer
against removed containers; update the guard to also close when a probe is
active (e.g., if (!this.isXlViewport && (this.adLoaded || this.isProbingAds))
this.close()), and ensure close() also clears any probing/no-content timers or
cancels pending ramp.que callbacks so no timers run after the container has been
removed.
🧹 Nitpick comments (2)
src/client/GutterAds.ts (2)
130-157:adLoadedis used for two different thingsAt line 143,
adLoadedis set totrueas a guard against double-loading. Then at line 190, probing may override it tofalse. Meanwhile,handleViewportChange(line 96) reads it as "ads are confirmed loaded."Consider splitting this into two flags — one for "load requested" (guard) and one for "content confirmed" — so the meaning is clear and you avoid relying on the override.
219-226: Visible class string is duplicated for left and right
leftClassandrightClassshare the exact same visible-state string. Extract it once to keep things tidy.Proposed fix
const probeClass = "absolute w-px h-px overflow-hidden pointer-events-none opacity-0"; + const visibleClass = + "fixed flex transform -translate-y-1/2 w-[160px] min-h-[600px] z-[100] pointer-events-auto items-center justify-center"; - const leftClass = this.leftAdVisible - ? "fixed flex transform -translate-y-1/2 w-[160px] min-h-[600px] z-[100] pointer-events-auto items-center justify-center" - : probeClass; - const rightClass = this.rightAdVisible - ? "fixed flex transform -translate-y-1/2 w-[160px] min-h-[600px] z-[100] pointer-events-auto items-center justify-center" - : probeClass; + const leftClass = this.leftAdVisible ? visibleClass : probeClass; + const rightClass = this.rightAdVisible ? visibleClass : probeClass;
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-80: show() currently resets state and starts a new load cycle
but doesn't tear down existing ramp units, causing duplicate/orphaned units;
modify show() to call the existing close() (or invoke window.ramp.destroyUnits
when available) at the top whenever adLoaded or adLoadRequested is true so any
previously created units are destroyed before resetting state, then proceed with
incrementing adWorkToken, resetting flags (adLoaded, adLoadRequested,
leftAdVisible, rightAdVisible, isProbingAds) and requesting an update; ensure
you reference the show() and close() methods and the window.ramp.destroyUnits
path so the old units are reliably cleaned up before loadAds() is scheduled.
- Around line 228-235: disconnectedCallback currently increments adWorkToken,
removes listeners, nulls xlMediaQuery and stops timers but doesn't destroy Ramp
ad units; update disconnectedCallback to call this.close() (which already wraps
ramp.destroyUnits calls) so Ramp can clean up its unit references when the
element is removed, keeping the existing adWorkToken increment, xlMediaQuery
removal, handleViewportChange/handleUserMeResponse listener removals and
stopNoContentCheck call intact.
🧹 Nitpick comments (1)
src/client/GutterAds.ts (1)
208-216:adLoadRequested = falseon line 214 is redundantLine 212 already sets
adLoadRequested = this.adLoaded. WhenadLoadedisfalse,adLoadRequestedis alreadyfalseby the time line 214 runs. You can drop line 214 for clarity.Proposed fix
if (this.noContentCheckCount >= 10) { this.stopNoContentCheck(); this.isProbingAds = false; this.adLoaded = this.leftAdVisible || this.rightAdVisible; this.adLoadRequested = this.adLoaded; if (!this.adLoaded) { - this.adLoadRequested = false; this.isVisible = false; } }
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
isn't there a simpler way? just like:
ifNoAdd { return html``}
is what i'm imagining
2 participants
Description:
Ads were only using tailwind "hidden" but were still being rendered on the left/right:
This PR hides ads without rendering the component - which fixes the bug of not being able to close modals by clicking where the ad would have been.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n