Conversation
|
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:
WalkthroughAdds temporary "Try me" flares: backend grant API and client API call, expiration-aware cosmetics validation using userMe.flareExpiration, trial countdowns and Try Me flow with rewarded-video integration, UI updates to pattern modal/button, and three new English translation keys. Changes
Sequence DiagramsequenceDiagram
actor User
participant PatternButton
participant RewardedAd
participant Api
participant Backend
participant Main
participant Cosmetics
User->>PatternButton: Click "Try Me"
PatternButton->>RewardedAd: showRewardedAd()
RewardedAd->>User: present ad UI
RewardedAd-->>PatternButton: onRewardGranted
PatternButton->>Api: grantTemporaryFlare(flare)
Api->>Backend: POST /flares_granted/temporary
Backend-->>Api: 200 OK
Api-->>PatternButton: true
PatternButton->>PatternButton: select flare, reload page
User->>Main: Page load / join
Main->>Cosmetics: validateAndGetCosmetics(userSettings)
Cosmetics->>Api: getUserMe()
Api->>Backend: GET /user/me
Backend-->>Api: UserMeResponse (includes player.flareExpiration)
Api-->>Cosmetics: user data
Cosmetics-->>Main: { color, patternName, patternColorPaletteName, expirations }
Main->>PatternButton: render with trialTimeRemaining
PatternButton->>PatternButton: setupCountdown()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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: 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/Cosmetics.ts (1)
84-137:⚠️ Potential issue | 🟠 MajorFix WinModal.ts to handle numeric return value from trial expirations.
The comparison at line 172 only checks for
"purchasable"and will ignore numeric trial values. Update the condition to handle both cases:Current code
if ( patternRelationship(pattern, colorPalette, me, null) === "purchasable" ) {The TerritoryPatternsModal.ts call site already handles this correctly (checking
typeof rel === "number"), but WinModal.ts needs the same treatment.
🤖 Fix all issues with AI agents
In `@src/client/components/PatternButton.ts`:
- Around line 100-117: handleTryMe currently calls grantTemporaryFlare but
proceeds to call handleClick and reload even when grantTemporaryFlare returns
false; change handleTryMe to await grantTemporaryFlare(flare) into a boolean
result and only call this.handleClick() and window.location.reload() when the
result is true, otherwise abort the flow and surface a user-facing error (e.g.,
show a toast or set an error state) so users aren’t redirected without the
trial; update references in PatternButton to use the result check around
grantTemporaryFlare, handleClick, and window.location.reload.
| private async handleTryMe(e: Event) { | ||
| e.stopPropagation(); | ||
| if (this.pattern === null) return; | ||
| const flare = | ||
| this.colorPalette?.name === undefined | ||
| ? `pattern:${this.pattern.name}` | ||
| : `pattern:${this.pattern.name}:${this.colorPalette.name}`; | ||
| await grantTemporaryFlare(flare); | ||
| this.handleClick(); | ||
| window.location.reload(); | ||
| } | ||
|
|
||
| private formatTimeRemaining(seconds: number): string { | ||
| const m = Math.floor(seconds / 60); | ||
| const s = seconds % 60; | ||
| if (m > 0) return `${m}m ${s}s`; | ||
| return `${s}s`; | ||
| } |
There was a problem hiding this comment.
Stop the flow when the trial grant fails.
grantTemporaryFlare returns false on errors, but the code still selects and reloads. That can leave users without the trial and still refresh the page. Gate the rest of the flow on success (and ideally show a user-facing error).
💡 Suggested fix
- await grantTemporaryFlare(flare);
- this.handleClick();
- window.location.reload();
+ const granted = await grantTemporaryFlare(flare);
+ if (!granted) {
+ return;
+ }
+ this.handleClick();
+ window.location.reload();📝 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 async handleTryMe(e: Event) { | |
| e.stopPropagation(); | |
| if (this.pattern === null) return; | |
| const flare = | |
| this.colorPalette?.name === undefined | |
| ? `pattern:${this.pattern.name}` | |
| : `pattern:${this.pattern.name}:${this.colorPalette.name}`; | |
| await grantTemporaryFlare(flare); | |
| this.handleClick(); | |
| window.location.reload(); | |
| } | |
| private formatTimeRemaining(seconds: number): string { | |
| const m = Math.floor(seconds / 60); | |
| const s = seconds % 60; | |
| if (m > 0) return `${m}m ${s}s`; | |
| return `${s}s`; | |
| } | |
| private async handleTryMe(e: Event) { | |
| e.stopPropagation(); | |
| if (this.pattern === null) return; | |
| const flare = | |
| this.colorPalette?.name === undefined | |
| ? `pattern:${this.pattern.name}` | |
| : `pattern:${this.pattern.name}:${this.colorPalette.name}`; | |
| const granted = await grantTemporaryFlare(flare); | |
| if (!granted) { | |
| return; | |
| } | |
| this.handleClick(); | |
| window.location.reload(); | |
| } | |
| private formatTimeRemaining(seconds: number): string { | |
| const m = Math.floor(seconds / 60); | |
| const s = seconds % 60; | |
| if (m > 0) return `${m}m ${s}s`; | |
| return `${s}s`; | |
| } |
🤖 Prompt for AI Agents
In `@src/client/components/PatternButton.ts` around lines 100 - 117, handleTryMe
currently calls grantTemporaryFlare but proceeds to call handleClick and reload
even when grantTemporaryFlare returns false; change handleTryMe to await
grantTemporaryFlare(flare) into a boolean result and only call
this.handleClick() and window.location.reload() when the result is true,
otherwise abort the flow and surface a user-facing error (e.g., show a toast or
set an error state) so users aren’t redirected without the trial; update
references in PatternButton to use the result check around grantTemporaryFlare,
handleClick, and window.location.reload.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/client/components/RewardedVideoPromo.ts`:
- Around line 106-110: In RewardedVideoPromo, handle the SDK-returning-void case
from manuallyCreateRewardUi by invoking the appropriate callback instead of
silently logging: when result is null/undefined set this.adPlaying = false and
call either this.onAdNotAvailable?.() if void implies no ad or
this.onAdError?.(new Error("manuallyCreateRewardUi returned no result")) to
surface an error to the parent; ensure you only call one callback and guard
calls with optional chaining/checks so you don't throw if the handler is absent.
- Around line 118-124: The button label in RewardedVideoPromo is hardcoded;
update the render to pass the user-facing string through translateText (e.g.,
translateText('rewarded_video.watch_for_reward')) instead of the literal "Watch
Ad for Reward", and add the corresponding key "rewarded_video.watch_for_reward":
"Watch Ad for Reward" to en.json so the text is localizable; locate this change
in the RewardedVideoPromo component's render method and ensure any required
import or usage pattern for translateText matches the project convention.
In `@src/client/Main.ts`:
- Around line 478-496: The code appends a new rewarded-video-ad every time the
onUserMe/getUserMe path runs, causing duplicates and losing type safety with "as
any"; fix by first querying document.body for an existing 'rewarded-video-ad'
(or give it a stable id) and only create/append a new element if none exists,
and replace "as any" with a narrow typed element (e.g., declare an interface
with onRewardGranted/onAdNotAvailable/onAdError and cast to that type or use
HTMLElement & { onRewardGranted?:()=>void; onAdNotAvailable?:()=>void;
onAdError?:(e:unknown)=>void }) when assigning event handlers on the rewardedAd
so duplicates are prevented and typing is restored (apply this change around the
rewardedAd creation/append logic and where onUserMe/getUserMe are invoked).
| } else { | ||
| // SDK returned void/undefined - it doesn't return a promise | ||
| console.log("[RewardedVideoAd] manuallyCreateRewardUi returned:", result); | ||
| this.adPlaying = false; | ||
| } |
There was a problem hiding this comment.
Silent failure when SDK returns void - no user feedback.
When manuallyCreateRewardUi returns void/undefined (not a Promise), the code only logs and sets adPlaying = false. The user clicked the button expecting a reward, but no callback fires - no reward granted, no error shown.
Consider either:
- Triggering
onAdErrorto let the parent handle this edge case - Triggering
onAdNotAvailableif this means no ad was available
Proposed fix
} else {
// SDK returned void/undefined - it doesn't return a promise
console.log("[RewardedVideoAd] manuallyCreateRewardUi returned:", result);
this.adPlaying = false;
+ this.onAdNotAvailable?.();
}📝 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.
| } else { | |
| // SDK returned void/undefined - it doesn't return a promise | |
| console.log("[RewardedVideoAd] manuallyCreateRewardUi returned:", result); | |
| this.adPlaying = false; | |
| } | |
| } else { | |
| // SDK returned void/undefined - it doesn't return a promise | |
| console.log("[RewardedVideoAd] manuallyCreateRewardUi returned:", result); | |
| this.adPlaying = false; | |
| this.onAdNotAvailable?.(); | |
| } |
🤖 Prompt for AI Agents
In `@src/client/components/RewardedVideoPromo.ts` around lines 106 - 110, In
RewardedVideoPromo, handle the SDK-returning-void case from
manuallyCreateRewardUi by invoking the appropriate callback instead of silently
logging: when result is null/undefined set this.adPlaying = false and call
either this.onAdNotAvailable?.() if void implies no ad or this.onAdError?.(new
Error("manuallyCreateRewardUi returned no result")) to surface an error to the
parent; ensure you only call one callback and guard calls with optional
chaining/checks so you don't throw if the handler is absent.
| return html` | ||
| <button | ||
| @click="${this.handleWatchAd}" | ||
| class="px-4 py-2 bg-blue-600 hover:bg-blue-700 text-white font-bold rounded shadow-lg transition-colors" | ||
| > | ||
| Watch Ad for Reward | ||
| </button> |
There was a problem hiding this comment.
Hardcoded button text needs i18n.
The text "Watch Ad for Reward" is hardcoded in English. Per PR checklist requirements, user-facing text should go through translateText() and be added to en.json.
Proposed fix
+import { translateText } from "../Utils";Then in render:
<button
`@click`="${this.handleWatchAd}"
class="px-4 py-2 bg-blue-600 hover:bg-blue-700 text-white font-bold rounded shadow-lg transition-colors"
>
- Watch Ad for Reward
+ ${translateText("rewarded_video.watch_ad_button")}
</button>🤖 Prompt for AI Agents
In `@src/client/components/RewardedVideoPromo.ts` around lines 118 - 124, The
button label in RewardedVideoPromo is hardcoded; update the render to pass the
user-facing string through translateText (e.g.,
translateText('rewarded_video.watch_for_reward')) instead of the literal "Watch
Ad for Reward", and add the corresponding key "rewarded_video.watch_for_reward":
"Watch Ad for Reward" to en.json so the text is localizable; locate this change
in the RewardedVideoPromo component's render method and ensure any required
import or usage pattern for translateText matches the project convention.
|
|
||
| // Test rewarded video ad | ||
| if (window.adsEnabled) { | ||
| const rewardedAd = document.createElement("rewarded-video-ad") as any; | ||
| rewardedAd.onRewardGranted = () => { | ||
| console.log("[Main] Reward granted to user!"); | ||
| }; | ||
| rewardedAd.onAdNotAvailable = () => { | ||
| console.log("[Main] Rewarded ad not available"); | ||
| }; | ||
| rewardedAd.onAdError = (error: unknown) => { | ||
| console.error("[Main] Rewarded ad error:", error); | ||
| }; | ||
| rewardedAd.style.position = "fixed"; | ||
| rewardedAd.style.bottom = "20px"; | ||
| rewardedAd.style.right = "20px"; | ||
| rewardedAd.style.zIndex = "9999"; | ||
| document.body.appendChild(rewardedAd); | ||
| } |
There was a problem hiding this comment.
Potential duplicate ad elements on repeated calls.
The onUserMe callback can run multiple times (initial check at line 501 and after getUserMe() resolves at line 505). Each call appends a new rewarded-video-ad element to document.body without checking if one already exists.
Also, using as any loses type safety.
Proposed fix - prevent duplicates and improve typing
// Test rewarded video ad
if (window.adsEnabled) {
+ // Prevent duplicate ad elements
+ if (document.querySelector("rewarded-video-ad")) {
+ return;
+ }
+ const rewardedAd = document.createElement(
+ "rewarded-video-ad",
+ ) as HTMLElement & {
+ onRewardGranted?: () => void;
+ onAdNotAvailable?: () => void;
+ onAdError?: (error: unknown) => void;
+ };
- const rewardedAd = document.createElement("rewarded-video-ad") as any;
rewardedAd.onRewardGranted = () => {
console.log("[Main] Reward granted to user!");
};📝 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.
| // Test rewarded video ad | |
| if (window.adsEnabled) { | |
| const rewardedAd = document.createElement("rewarded-video-ad") as any; | |
| rewardedAd.onRewardGranted = () => { | |
| console.log("[Main] Reward granted to user!"); | |
| }; | |
| rewardedAd.onAdNotAvailable = () => { | |
| console.log("[Main] Rewarded ad not available"); | |
| }; | |
| rewardedAd.onAdError = (error: unknown) => { | |
| console.error("[Main] Rewarded ad error:", error); | |
| }; | |
| rewardedAd.style.position = "fixed"; | |
| rewardedAd.style.bottom = "20px"; | |
| rewardedAd.style.right = "20px"; | |
| rewardedAd.style.zIndex = "9999"; | |
| document.body.appendChild(rewardedAd); | |
| } | |
| // Test rewarded video ad | |
| if (window.adsEnabled) { | |
| // Prevent duplicate ad elements | |
| if (document.querySelector("rewarded-video-ad")) { | |
| return; | |
| } | |
| const rewardedAd = document.createElement( | |
| "rewarded-video-ad", | |
| ) as HTMLElement & { | |
| onRewardGranted?: () => void; | |
| onAdNotAvailable?: () => void; | |
| onAdError?: (error: unknown) => void; | |
| }; | |
| rewardedAd.onRewardGranted = () => { | |
| console.log("[Main] Reward granted to user!"); | |
| }; | |
| rewardedAd.onAdNotAvailable = () => { | |
| console.log("[Main] Rewarded ad not available"); | |
| }; | |
| rewardedAd.onAdError = (error: unknown) => { | |
| console.error("[Main] Rewarded ad error:", error); | |
| }; | |
| rewardedAd.style.position = "fixed"; | |
| rewardedAd.style.bottom = "20px"; | |
| rewardedAd.style.right = "20px"; | |
| rewardedAd.style.zIndex = "9999"; | |
| document.body.appendChild(rewardedAd); | |
| } |
🤖 Prompt for AI Agents
In `@src/client/Main.ts` around lines 478 - 496, The code appends a new
rewarded-video-ad every time the onUserMe/getUserMe path runs, causing
duplicates and losing type safety with "as any"; fix by first querying
document.body for an existing 'rewarded-video-ad' (or give it a stable id) and
only create/append a new element if none exists, and replace "as any" with a
narrow typed element (e.g., declare an interface with
onRewardGranted/onAdNotAvailable/onAdError and cast to that type or use
HTMLElement & { onRewardGranted?:()=>void; onAdNotAvailable?:()=>void;
onAdError?:(e:unknown)=>void }) when assigning event handlers on the rewardedAd
so duplicates are prevented and typing is restored (apply this change around the
rewardedAd creation/append logic and where onUserMe/getUserMe are invoked).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/components/PatternButton.ts`:
- Around line 46-68: The countdown is being torn down and recreated every second
because writing to the reactive property trialTimeRemaining triggers updated() →
setupCountdown(); fix by guarding setup: in setupCountdown() return early if an
active interval already exists (check this._countdownInterval) so you don't
create a new setInterval on each tick, and ensure clearCountdown() clears the
interval and resets this._countdownInterval to undefined; keep the existing
logic that clears when trialTimeRemaining <= 0.
🧹 Nitpick comments (3)
src/client/components/PatternButton.ts (1)
119-124:formatTimeRemainingdoes not handle hours.If the trial duration is longer than 59 minutes, users see something like
"75m 30s". If that is acceptable, no change needed. Otherwise, add hours support.src/client/Cosmetics.ts (2)
89-89: Return typenumberfor a timestamp is easy to misread.The union
"owned" | "purchasable" | "blocked" | numberworks, but a rawnumberamong string literals can confuse callers about what it represents. A small type alias would make intent clear:/** Unix-ms timestamp when the trial flare expires */ type FlareExpiration = number; export type PatternRelationship = "owned" | "purchasable" | "blocked" | FlareExpiration;
111-116: Duplicated expiration-check logic in two places.Lines 111–116 (
patternRelationship) and lines 163–166 (validateAndGetCosmetics) both perform the same "is this flare expired withinTEMP_FLARE_OFFSET?" check. Extract a small helper to keep them in sync:Example helper
function isFlareExpiringSoon( expirations: Record<string, number>, flareName: string, ): boolean { const exp = expirations[flareName]; return !!exp && exp - Date.now() <= TEMP_FLARE_OFFSET; }Also applies to: 163-166
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/client/components/RewardedVideoPromo.ts`:
- Around line 60-108: ensureRewardedUnitRegistered currently resolves after
blind timeouts (100ms/500ms) which can let showRewardedAd run before an ad is
ready; change it to wait for the actual readiness signal by subscribing to the
rewardedAdVideoRewardReady event or by polling the rewardedAdReady flag with a
max-wait fallback. Specifically, in ensureRewardedUnitRegistered replace the
fixed setTimeout resolutions with logic that: if rewardedUnitRegistered &&
!rewardedAdReady, attach a one-time listener for the global/event emitter event
(e.g., "rewardedAdVideoRewardReady") or poll window.ramp.rewardedAdReady until
true, resolve immediately when rewardedAdReady becomes true, and reject after a
configurable timeout (e.g., 5s); do the same after calling window.ramp.spaAddAds
so you only resolve once rewardedAdReady/event fires, and ensure you clean up
any listeners/timers and still handle rampSdkFailed and spaAddAds-absent cases.
- Around line 19-58: The module-level initRewardedAds and the component method
RewardedVideoAd.registerRewardedUnit both call window.ramp.spaAddAds and
maintain separate readiness flags (module globals
rewardedUnitRegistered/rewardedAdReady vs this.rampReady), causing double
registration and visibility bugs for showRewardedAd; fix by centralizing
registration in one place: remove spaAddAds from
RewardedVideoAd.registerRewardedUnit and instead have registerRewardedUnit call
initRewardedAds(), then read the module-level flags (rewardedUnitRegistered and
rewardedAdReady) for readiness checks (and update showRewardedAd to consult
those module flags instead of this.rampReady), ensuring only initRewardedAds
performs spaAddAds and sets the shared state.
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 169-176: The modal is recalculating trialTimeRemaining from
Date.now() on every render (using isTrial, rel and TEMP_FLARE_OFFSET) which
resets PatternButton's internal countdown; fix by computing the initial
remaining time once and passing a stable prop (e.g., store the computed value in
a local state or ref when the modal mounts or when rel changes) so PatternButton
receives a consistent starting value instead of a new Date.now()-based value on
each render; ensure the change surrounds the code that sets trialTimeRemaining
(the expression using isTrial, rel, TEMP_FLARE_OFFSET) and keep PatternButton's
internal interval logic unchanged.
🧹 Nitpick comments (5)
src/client/components/RewardedVideoPromo.ts (1)
110-206: Promise constructor anti-pattern — use async/await instead.Wrapping
ensureRewardedUnitRegistered().then(...)insidenew Promise((resolve, reject) => ...)is hard to follow and easy to get wrong. Converting toasyncmakes the flow flat and readable.Sketch of the simplification
-export function showRewardedAd(): Promise<void> { - ... - return new Promise((resolve, reject) => { - if (rampSdkFailed) { - reject(new Error("Ad blocker detected")); - return; - } - ensureRewardedUnitRegistered() - .then(() => { - ... - }) - .catch((err) => { - reject(err); - }); - }); -} +export async function showRewardedAd(): Promise<void> { + if (rampSdkFailed) { + throw new Error("Ad blocker detected"); + } + + await ensureRewardedUnitRegistered(); + + if (!window.ramp?.manuallyCreateRewardUi) { + throw new Error("Ramp SDK manuallyCreateRewardUi not available"); + } + + return new Promise((resolve, reject) => { + // only the event-listener part needs a raw Promise + let timeoutId: ReturnType<typeof setTimeout> | null = null; + const cleanup = () => { /* same cleanup */ }; + // ... event wiring + manuallyCreateRewardUi call ... + }); +}src/client/Cosmetics.ts (2)
84-89: Return type uses a barenumberin a union with string literals — consider a tagged type for clarity.
"owned" | "purchasable" | "blocked" | numberworks, but callers needtypeof rel === "number"checks which are easy to miss. A small tagged object like{ status: "trial"; expiresAt: number }would make intent clearer, but this is a style preference — fine to keep if you prefer the compact form.
139-180:validateAndGetCosmeticssilently clears the user's pattern whengetUserMereturns false.When
getUserMe()returnsfalse(unauthenticated / network error), the code enters theif (userMe)block as false, sopatternis never nulled, and the selected pattern is returned as-is. That's actually fine for the unauthenticated case — the pattern persists.However, there is duplicated expiration logic between this function (lines 160-167) and
patternRelationship(lines 110-116). If the TEMP_FLARE_OFFSET threshold or expiration semantics change, both spots must be updated in sync.Consider extracting a small helper like
isFlareExpired(flareName, expirations)to keep the logic in one place.Example helper
function isFlareExpiredOrExpiring( flareName: string, expirations: Record<string, number>, ): boolean { const exp = expirations[flareName]; return !!exp && exp - Date.now() <= TEMP_FLARE_OFFSET; }Then use it in both
patternRelationshipandvalidateAndGetCosmetics.src/client/components/PatternButton.ts (1)
31-32: Mutating a parent-owned@propertyfrom inside the component is a Lit anti-pattern.
trialTimeRemainingis set by the parent (TerritoryPatternsModal) via.trialTimeRemaining=, but the countdown decrements it internally withthis.trialTimeRemaining--. This breaks the one-way data flow that Lit@propertyexpects.Consider changing
trialTimeRemainingto an initial-value prop (e.g.initialTrialSeconds) and tracking the live countdown in a private@state()field instead.src/client/TerritoryPatternsModal.ts (1)
134-134: Type annotationstring | numberis broad — mirror the function return type.
patternRelationshipreturns"owned" | "purchasable" | "blocked" | number. Usingstring | numberhere loses the literal type info. Consider:-let rel: string | number = "owned"; +let rel: ReturnType<typeof patternRelationship> = "owned";This keeps IDE autocomplete and narrows the type automatically if the return type changes.
| export function initRewardedAds(): void { | ||
| if (rewardedUnitRegistered || rampSdkFailed) return; | ||
|
|
||
| const startTime = Date.now(); | ||
|
|
||
| // Set a hard timeout - if registration doesn't complete, mark SDK as failed | ||
| setTimeout(() => { | ||
| if (!rewardedUnitRegistered && !rampSdkFailed) { | ||
| console.log("[RewardedVideoPromo] Ramp SDK timeout - likely blocked"); | ||
| rampSdkFailed = true; | ||
| } | ||
| }, RAMP_TIMEOUT_MS); | ||
|
|
||
| const tryRegister = () => { | ||
| // Check for timeout (adblocker likely blocking the SDK) | ||
| if (Date.now() - startTime > RAMP_TIMEOUT_MS) { | ||
| console.log("[RewardedVideoPromo] Ramp SDK timeout - likely blocked"); | ||
| rampSdkFailed = true; | ||
| return; | ||
| } | ||
|
|
||
| // Check if spaAddAds exists (real SDK loaded, not just the stub from index.html) | ||
| if (!window.ramp?.spaAddAds) { | ||
| // Retry in 100ms if ramp not ready yet | ||
| setTimeout(tryRegister, 100); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| window.ramp.spaAddAds([{ type: "rewarded_ad_video", selectorId: "" }]); | ||
| rewardedUnitRegistered = true; | ||
| console.log("[RewardedVideoPromo] Rewarded unit pre-registered"); | ||
| } catch (e) { | ||
| console.error("[RewardedVideoPromo] Failed to pre-register:", e); | ||
| rampSdkFailed = true; | ||
| } | ||
| }; | ||
|
|
||
| tryRegister(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Two separate registration paths create confusing shared state.
initRewardedAds() (module-level) and RewardedVideoAd.registerRewardedUnit() (component) both call spaAddAds to register the rewarded unit, but they track readiness differently — one uses the module globals (rewardedUnitRegistered, rewardedAdReady), the other uses this.rampReady. If both run, the unit gets registered twice. If only the component runs, showRewardedAd() won't see it as registered.
Consider picking one registration path and sharing state. For example, have the component call initRewardedAds() and read from the module-level flags, or remove the component's own registration entirely.
🤖 Prompt for AI Agents
In `@src/client/components/RewardedVideoPromo.ts` around lines 19 - 58, The
module-level initRewardedAds and the component method
RewardedVideoAd.registerRewardedUnit both call window.ramp.spaAddAds and
maintain separate readiness flags (module globals
rewardedUnitRegistered/rewardedAdReady vs this.rampReady), causing double
registration and visibility bugs for showRewardedAd; fix by centralizing
registration in one place: remove spaAddAds from
RewardedVideoAd.registerRewardedUnit and instead have registerRewardedUnit call
initRewardedAds(), then read the module-level flags (rewardedUnitRegistered and
rewardedAdReady) for readiness checks (and update showRewardedAd to consult
those module flags instead of this.rampReady), ensuring only initRewardedAds
performs spaAddAds and sets the shared state.
| function ensureRewardedUnitRegistered(): Promise<void> { | ||
| console.log("[ensureRewardedUnitRegistered] Called", { | ||
| rampSdkFailed, | ||
| rewardedUnitRegistered, | ||
| rewardedAdReady, | ||
| hasSpaAddAds: !!window.ramp?.spaAddAds, | ||
| }); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| // If SDK failed to load (adblocker), reject immediately | ||
| if (rampSdkFailed) { | ||
| console.log("[ensureRewardedUnitRegistered] Rejecting: SDK failed"); | ||
| reject(new Error("Ad blocker detected")); | ||
| return; | ||
| } | ||
|
|
||
| // Check for real SDK (not just stub from index.html) | ||
| if (!window.ramp?.spaAddAds) { | ||
| console.log( | ||
| "[ensureRewardedUnitRegistered] Rejecting: spaAddAds not available", | ||
| ); | ||
| reject(new Error("Ramp SDK not available")); | ||
| return; | ||
| } | ||
|
|
||
| // If already registered and ready, resolve immediately | ||
| if (rewardedUnitRegistered && rewardedAdReady) { | ||
| resolve(); | ||
| return; | ||
| } | ||
|
|
||
| // If already registered but not ready, wait briefly then resolve | ||
| if (rewardedUnitRegistered) { | ||
| setTimeout(() => resolve(), 100); | ||
| return; | ||
| } | ||
|
|
||
| // Register the unit directly (not via queue) | ||
| try { | ||
| window.ramp.spaAddAds([{ type: "rewarded_ad_video", selectorId: "" }]); | ||
| rewardedUnitRegistered = true; | ||
| console.log("[RewardedVideoPromo] Rewarded unit registered"); | ||
| // Wait a bit for the ad to load, then resolve | ||
| setTimeout(() => resolve(), 500); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
ensureRewardedUnitRegistered resolves optimistically on a fixed delay.
Lines 92-95: when the unit is registered but not yet ready, it resolves after a blind 100ms wait. Lines 102-103: after a fresh registration, it resolves after 500ms. Neither actually checks if the ad became ready — the caller (showRewardedAd) then immediately tries to show the ad, which may fail silently if the ad hasn't loaded.
Consider waiting for the rewardedAdVideoRewardReady event (or the rewardedAdReady flag) instead of a fixed timeout, with a max-wait fallback.
🤖 Prompt for AI Agents
In `@src/client/components/RewardedVideoPromo.ts` around lines 60 - 108,
ensureRewardedUnitRegistered currently resolves after blind timeouts
(100ms/500ms) which can let showRewardedAd run before an ad is ready; change it
to wait for the actual readiness signal by subscribing to the
rewardedAdVideoRewardReady event or by polling the rewardedAdReady flag with a
max-wait fallback. Specifically, in ensureRewardedUnitRegistered replace the
fixed setTimeout resolutions with logic that: if rewardedUnitRegistered &&
!rewardedAdReady, attach a one-time listener for the global/event emitter event
(e.g., "rewardedAdVideoRewardReady") or poll window.ramp.rewardedAdReady until
true, resolve immediately when rewardedAdReady becomes true, and reject after a
configurable timeout (e.g., 5s); do the same after calling window.ramp.spaAddAds
so you only resolve once rewardedAdReady/event fires, and ensure you clean up
any listeners/timers and still handle rampSdkFailed and spaAddAds-absent cases.
| .trialTimeRemaining=${isTrial | ||
| ? Math.max( | ||
| 0, | ||
| Math.floor( | ||
| ((rel as number) - TEMP_FLARE_OFFSET - Date.now()) / 1000, | ||
| ), | ||
| ) | ||
| : 0} |
There was a problem hiding this comment.
Trial countdown is recalculated on every render, which can conflict with PatternButton's internal countdown.
Each time the modal re-renders (e.g. another pattern's state changes), trialTimeRemaining is recomputed from Date.now(), resetting the prop on PatternButton. Combined with PatternButton's own setInterval decrement, this can cause visible jumps in the displayed time.
This is low-risk since modal re-renders are infrequent during normal use, but worth noting if you add more reactive state to this view.
🤖 Prompt for AI Agents
In `@src/client/TerritoryPatternsModal.ts` around lines 169 - 176, The modal is
recalculating trialTimeRemaining from Date.now() on every render (using isTrial,
rel and TEMP_FLARE_OFFSET) which resets PatternButton's internal countdown; fix
by computing the initial remaining time once and passing a stable prop (e.g.,
store the computed value in a local state or ref when the modal mounts or when
rel changes) so PatternButton receives a consistent starting value instead of a
new Date.now()-based value on each render; ensure the change surrounds the code
that sets trialTimeRemaining (the expression using isTrial, rel,
TEMP_FLARE_OFFSET) and keep PatternButton's internal interval logic unchanged.
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
1 participant
Description:
Added rewarded video ads for skin trials via Playwire's manuallyCreateRewardUi API. Users can now click "Try me" to watch a video ad and receive a temporary skin trial. Upon completion a temporary flare is granted to the player so they have ~5 minutes to use the skin.
added getPlayerCosmeticsRefs and getPlayerCosmetics to Cosmetics.ts to centralize cosmetic retrieval & validation.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan