feat(quests): offer partner gifts when the day's quests are done - #6568
Conversation
The streak_milestone_offers experiment failed, so the streak milestone popup goes back to the classic NewStreak modal and everything the experiment added around it is removed. quest_offers replaces it at a different moment: once every unlocked daily quest is claimed, the same Encore offers are presented as the reward. Eligibility is a state check rather than a transition watch, so the moment survives a reload, and a persisted day stamp keeps it to one showing per day. Locked Plus quests are excluded from the "day is done" test, otherwise free users would sit permanently one quest short. Neither the control group nor a treatment with no inventory stamps the day: there is no classic popup to fall back to here, so a later visit can still catch offers once they exist. The offer primitives move from streak to quest and lose the flame ladder for a level ring, XP earned, and a quest step strip. Needs QUEST_COMPLETION on the API's OfferPlacement enum (daily-api#4183). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Adding a key to PersistentContextKeys pulled the file into the strict migration guard, which surfaced two pre-existing errors: an empty cache with no fallback resolves to null, but the hook's tuple claimed T. The public signature is unchanged and `||` is kept over `??`, so no caller's inferred types or runtime behaviour move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Waiting for every daily quest to be claimed put the offer behind a moment too rare to measure, and it was rarer still for Plus users, whose unlocked Plus quests counted toward the total they had to clear. The persisted day stamp already caps this at one showing, so the threshold drops to the first claim of the day. Locked quests stay out of the total so the progress denominator is one a free user can close, and the panel only claims the day is complete once it actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The popup never fired: getDailyQuestSummary keyed off `status === claimed`, but the API stamps `claimedAt` without reliably flipping status, so every claim counted as zero. QuestCard and CompactQuestList already guarded against this with `status === claimed || Boolean(claimedAt)`. That predicate is now isQuestClaimed in graphql/quests.ts and all three sites share it, rather than the summary carrying a third, subtly weaker copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The total dropped locked quests, so a free user with two regular and three Plus dailies read "1/2" while the quest panel plainly showed five. A denominator that disagrees with the screen is worse than one a free user cannot close, and showing all five frames the locked Plus track as something still to do today. The trigger is unchanged: locked quests are never claimed, so the claimed count this fires on does not move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two blind spots in the experiment's tracking. Control renders nothing, and so does a treatment that finds no Encore inventory, so both arms were invisible in analytics — a dud experiment looked exactly like missing stock, which is the confusion that cost us several rounds of debugging. A `quest offers eligible` event now fires when the moment is reached, carrying the arm and the offer count, on its own day stamp so every arm logs once per user per day. That stamp is deliberately separate from the modal's: an eligibility event logged by control, or by a treatment with no stock, must not stop the modal showing later that day once offers do turn up. The split layout renders every offer at once while the carousel renders one card per swipe, so impression counts are only comparable when segmented. Every offer event now carries `variant`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Reviewed the quest-offers experiment end to end (cleanup of streak_milestone_offers, the new trigger, the modal/celebration retheme, the shared isQuestClaimed predicate and the usePersistentContext typing). The cleanup is complete — no references to streakTiers, StreakOffer*, LazyModal.StreakOffers or STREAK_MILESTONE survive — CI is green, and the experiment is correctly conditional on eligibility with a control default of off. Test coverage on the trigger is genuinely good.
Two findings I'd like resolved before this goes to enrollment, both about the popup trigger rather than the UI, plus a few non-blocking ones inline.
Cross-cutting (no single line):
- The Events table in the description is missing the new
quest offers eligibleevent, and thevariant/offersextras added to the twoimpressionevents and todismiss quest offersin the last commit. Since that table is what gets reviewed for taxonomy, please bring it up to date with what the code now emits. - Deploy ordering:
OfferPlacement.STREAK_MILESTONEis replaced rather than extended, so a webapp deploy ahead of dailydotdev/daily-api#4183 would send an enum the API does not know. In practice the flag defaults to off so nothing queries it — worth stating explicitly in the merge plan rather than leaving it implicit. - The day stamp is a global idb key (
quest_offers_last_seen), not namespaced by user. The description calls out per-device, but the sharper consequence is account switching: on a shared browser the second account is suppressed for the rest of the day.ReadingReminderLastSeenhas the same shape so this is consistent with precedent — flagging it as a known limitation rather than asking for a change.
Reviewed by AI.
| } | ||
|
|
||
| hasOpened.current = true; | ||
| setLastSeen(new Date().toISOString()); |
There was a problem hiding this comment.
Blocking: the day is stamped before we know the modal actually stuck, and !!modal cannot prevent a same-commit collision.
useLazyModal writes to a react-query key; setQueryData does not update modal synchronously, and effects in sibling components all run in the same commit. MainLayout now mounts three independent writers to MODAL_KEY — BootPopups (which does not check modal at all), StreakMilestonePopup, and this trigger, in that order. A logged-in user who is due a streak-milestone popup and has claimed a quest today hits this on first paint: both effects read modal === undefined, both call openModal, and the last write wins.
Whichever loses is silently dropped, but its side effects already ran. In the case where this one loses, hasOpened.current is set and setLastSeen(now) has burned the day stamp, so the user sees no offers today, no impression fires, and confirmOffersDelivered never runs for offers we already fetched from Encore — which is exactly the outcome the "neither control nor an empty inventory stamps the day" design is trying to avoid.
Two directions: read the live value (queryClient.getQueryData(MODAL_KEY)) inside the effect immediately before opening rather than relying on the render snapshot, and move the setLastSeen stamp to the point the modal actually renders (alongside the existing useLogEventOnce impression in QuestOffersModal) so it can only be written for a showing that happened. Longer term the cleaner answer is an immediate boot popup via addImmediatePopup, which is the repo's existing arbiter for exactly this.
Reviewed by AI.
| ]); | ||
|
|
||
| useEffect(() => { | ||
| if (hasOpened.current || !shouldShow || isOffersFeatureLoading) { |
There was a problem hiding this comment.
Blocking (analytics): this effect does not wait for isEligibleLogFetched, but the eligibility effect above does — and opening the modal flips shouldShow to false permanently for the session.
lastSeen and eligibleLoggedAt are two independent idb-keyval reads resolving in parallel. If isLastSeenFetched lands first, shouldShow becomes true and this effect can open the modal in that commit, while the eligibility effect is still short-circuiting on !isEligibleLogFetched. On the next render shouldShow is false, so quest offers eligible never fires for that user that day.
The bias is one-directional and lands on the arm you most need: control and no-inventory treatment never open a modal, so they keep re-evaluating until their stamp read resolves and always log. Only treatment-with-inventory can drop the event — so the denominator is under-counted precisely where the numerator lives.
Either add !isEligibleLogFetched to the shouldShow predicate, or log the eligibility event from this same effect immediately before openModal so the two can't be separated. Worth a spec case with the eligibility stamp resolving after the last-seen stamp — the current tests mock usePersistentContext synchronously, so this ordering isn't exercised.
Reviewed by AI.
| target_id: summary.claimed.toString(), | ||
| extra: JSON.stringify({ | ||
| enabled: offersEnabled, | ||
| offers: offers?.length ?? 0, |
There was a problem hiding this comment.
Non-blocking: the event's stated purpose is telling a dud experiment apart from missing Encore stock, but a failed fetch is indistinguishable from empty inventory here.
userOffersQueryOptions sets retry: false, so a 5xx or a network blip resolves areOffersPending to false with offers still undefined, and this logs offers: 0 — the same payload as a healthy response with no inventory. That is the third state that cost the debugging rounds the commit message refers to.
Pulling isError (or status) off the query and adding it to extra makes the three cases separable at query time.
Reviewed by AI.
| } from '../../../graphql/offers'; | ||
| import { featureQuestOffers } from '../../../lib/featureManagement'; | ||
|
|
||
| const hasSeenToday = (lastSeen: string | null): boolean => { |
There was a problem hiding this comment.
Non-blocking: this already exists as getHasSeenToday in hooks/notifications/useReadingReminderHero.ts:34 — same null guard, same Number.isNaN(getTime()) guard, same isToday, reading the same kind of ISO day stamp out of usePersistentContext.
That makes two copies of the once-per-day stamp idiom, and this file calls it against two separate keys. Worth lifting a single helper (a lib/dateFormat export, or a small useDayStamp-style hook that pairs the read with the write) and pointing both surfaces at it, rather than leaving the semantics of "seen today" defined in two places.
Reviewed by AI.
| return [data ?? fallbackValue, updateValue, isFetched, isLoading]; | ||
| // An empty cache with no fallback resolves to null, so callers that can hit | ||
| // that case pass a T which admits it (`usePersistentContext<string | null>`). | ||
| return [(data ?? fallbackValue) as T, updateValue, isFetched, isLoading]; |
There was a problem hiding this comment.
Non-blocking: the cast makes the signature less honest than it was, in the opposite direction from the commit's intent.
getAsyncCache now correctly returns T | null, but as T re-hides that at the boundary: any caller that passes neither valueWhenCacheEmpty nor fallbackValue still receives null typed as T, and the compiler will not tell them. The comment carries the contract instead of the types, so the next caller has to read the hook body to learn the rule.
Since the tuple type is already exported, encoding it — UserPersistentContextType<T | null> when no fallback is supplied, via an overload or a conditional on the argument — keeps the fix without the cast. Happy for this to be a follow-up given the PR's scope, but the cast plus comment is the thing this commit was trying to remove.
Reviewed by AI.
| claimed: number; | ||
| className?: string; | ||
| }): ReactElement => ( | ||
| <div className={classNames('flex items-center gap-1.5', className)}> |
There was a problem hiding this comment.
Non-blocking: the strip has no wrap and no cap, and total grew in the last commit.
getDailyQuestSummary now counts locked Plus quests toward total on purpose, so this renders one 28px circle per daily quest, regular and Plus. In the split layout the panel is w-[19rem] with p-6, leaving roughly 256px of usable width — with gap-1.5 that fits about seven steps before the row overflows the panel it sits in. QuestOfferSplit only renders on desktop so a phone won't hit it, but the count is API-driven and nothing in the component defends against it.
flex-wrap on this container, or capping at N with a +n chip, keeps it safe whatever the daily set grows to.
Reviewed by AI.
…nominator Addresses PR review. `useLazyModal` reads MODAL_KEY through react-query, so `modal` is a render snapshot, and MainLayout mounts three independent writers to that key whose effects run in the same commit. A user due a streak milestone who had also claimed a quest could have both write, last one wins, and the loser was dropped silently with its side effects already run — in our case the day stamp burned for offers that were never shown, never logged an impression, and never confirmed back to Encore. The trigger now reads the live value immediately before opening, and delegates the day stamp to the modal's mount so it can only be written for a popup that reached the screen. The eligibility effect waited on its own stamp read while the modal effect did not, so whichever idb read landed first decided the outcome: opening the modal flips `shouldShow` false for the day, silently dropping the event. The bias was one-directional and landed on treatment-with-inventory, under-counting the denominator exactly where the numerator lives. Both stamps are now gated in `shouldShow`. Also from review: `retry: false` makes a failed offers fetch settle with no offers, indistinguishable from empty inventory, so the eligibility event carries `failed`; the step strip wraps now that locked quests count toward a total the API controls; and the once-per-day stamp idiom is a single `isTodayStamp` in lib/dateFormat rather than a copy here and another in useReadingReminderHero. usePersistentContext trades the `as T` cast for overloads, so an omitted `valueWhenCacheEmpty` is typed `T | null` at the call site instead of having the rule live in a comment. Every current caller passes one, so nothing changes for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Changes
The
streak_milestone_offersexperiment failed. This removes it and replaces it withquest_offers, which shows the same Encore partner offers at a different moment: the day's first claimed daily quest.Cleanup
StreakOffersModal, and the wholecomponents/streak/offers/directory (including thestreakTiersflame ladder)StreakMilestonePopupis back to opening the classicNewStreakmodal — its strict-TS fixes are keptLazyModal.StreakOffers,DismissStreakOffers,TargetType.StreakOfferNew experiment
QuestOffersPopupis mounted inMainLayoutand gated likeQuestUpdatesListener(logged in, settings loaded, not opted out of quests)PersistentContextKeys.QuestOffersLastSeenkeeps it to one showing per dayisQuestClaimedpredicate (status === claimed || Boolean(claimedAt)). The API stampsclaimedAtwithout reliably flippingstatus;QuestCardandCompactQuestListalready guarded against this and now share one copyAPI: needs
QUEST_COMPLETIONonOfferPlacement— merged in dailydotdev/daily-api#4183.Merge plan
OfferPlacement.STREAK_MILESTONEwas replaced rather than extended, so a webapp deploy ahead of the API would have sent an enum it did not know. In practice the flag defaults to control, so nothing queriesuserOffersuntil enrolment — but the ordering is now moot since the API shipped first.quest_offersat control.quest_completionand thatremoteConfig.vars.encoreOffers.enabledis on withallowedCountriescovering the target markets. That remote config is a single global gate shared with the deprecated streak placement — if it was switched off when the streak experiment was called a failure, this ships as a silent no-op. The newquest offers eligibleevent makes exactly that case visible.streak_milestone_offerscan be archived in GrowthBook; it has no readers left.Events
Did you introduce any new tracking events? Yes.
quest offers eligibletarget_type: 'quest offer',target_id: <claimed count>,extra: { enabled, offers, failed }impressiontarget_type: 'quests completed',target_id: <claimed count>,extra: { variant, offers }impressiontarget_type: 'quest offer',target_id: <impressionUid>,extra: { brand, questsCompleted, variant }clicktarget_type: 'quest offer',target_id: <impressionUid>,extra: { brand, questsCompleted, variant }dismiss quest offerstarget_type: 'quest offer',target_id: <claimed count>,extra: { method: 'close' | 'decline', claimed, variant }dismiss streak offersquest offers eligibleis the experiment's denominator. Control renders nothing and so does a treatment that found no inventory, so without it both arms are invisible and a dud experiment cannot be told apart from missing Encore stock. It carries its own day stamp so every arm logs once per user per day, andextraseparates the three outcomes:enabled:false(control),enabled:true, offers:0, failed:false(no inventory),enabled:true, offers:0, failed:true(query error —retry: falsemeans a 5xx settles with no offers).variant(split|carousel) rides every offer event because the two layouts deliver differently — the split renders all offers at once, the carousel one card per swipe — so impression counts are only comparable when segmented by it.Every dismissal path funnels through one handler (desktop X, backdrop, escape; the drawer's own button, outside click and escape on mobile; and "No thanks"), guarded by a ref so it logs exactly once per modal.
Experiment
Did you introduce any new experiments? Yes —
quest_offers, boolean, defaults to control.Important
Please do not merge the PR until the experiment enrolment is approved.
Known limitations
ReadingReminderLastSeenhas the same shape, so this is consistent with existing precedent rather than new behaviour — flagging it rather than changing it.claimed >= totalnow that locked quests count, so the "Daily quests complete" headline is Plus-only in practice. Free users always see "Quest complete", which is accurate and fits the still-to-do framing.Manual Testing
On those affected packages:
Did you test the modified components media queries?
The drawer/carousel engages below 656px and the split layout above it, so both need a look.
Automated: shared 2688 / webapp 626 / extension 52 passing. The
numberFormatandworld/*failures also fail onmain.typecheck-strict-changedand eslint clean.🤖 Generated with Claude Code
Preview domain
https://quest-offers-experiment.preview.app.daily.dev