fix(quests): tie the offer popup to the claim that earned it

[rebelchris]

Changes

Follow-up to #6568. Reported after merge: the offers popup appeared on app entry with no apparent trigger.

That was the design working as specified rather than a bug. Eligibility was the standing fact of having claimed a quest today, so it stayed true on any later visit and the reward turned up detached from the action that earned it.

The trigger now reacts to the claim itself. useClaimQuestReward — the one mutation every claim surface goes through — publishes the claim to the query cache under RequestKey.QuestClaim, and the popup observes it. That is how useLazyModal already carries the open modal between components that never meet, so it needs no new module, no provider and no global.

A plain mutation callback cannot do this: the hook is instantiated in four claim surfaces (header panel, sidebar CompactQuestList, /daily-quests, game center) and the listener is a fifth in MainLayout. Folding the offers logic into the mutation instead would evaluate the experiment flag whenever any claim surface mounts, rather than only at the eligible moment, which would dilute enrolment.

The claim is a one-shot, not a flag. It is spent once the offers fetch settles, whichever way it decided, so an empty result cannot be revived by a later refetch. A sibling popup holding MODAL_KEY defers rather than spending, so the claim can still land when that closes — and a 30s window expires anything never acted on, which also covers a fetch that never settles.

Any real claim qualifies — daily, weekly and milestone. Intro quests are excluded: they are onboarding, they already have their own celebration in IntroQuestModal, and because that is a LazyModal this trigger would defer behind it and land a sponsored offer as someone's first-run experience.

Experiment

quest_offers is not enrolled — still 0%. That matters for reading this PR: the review raised that broadening the trigger and changing / 3 quests to / 3 daily quests would move the enrolment population and the treatment copy mid-flight. With no traffic there is no data to be discontinuous with and no arm has seen the old copy, so both are free changes today. Recording it here so nobody later hunts for a discontinuity that never existed.

The trigger broadening does change who ever enrolls once it is switched on — weekly and milestone claimers who never finish a daily quest are now in scope. That is the intended product behaviour, not a side effect.

Trade-offs

• The moment no longer survives a reload. Claim while Encore is down or has no stock and there is no second chance that day. The old state check would have retried on the next visit — and that retry is precisely what produced the reported behaviour, so it goes with it.
• The 30s claim window makes sibling-deferral a coin flip. A boot popup or streak modal the user actually reads is often on screen longer than that, and the claim then expires unspent. The day stays unstamped so a later claim still shows, and the alternative — waiting indefinitely — is what we just removed. Worth revisiting with data: if quest offers eligible fires materially more often than the modal is shown, this number is the first thing to look at.
• The progress count is derived from the daily set, so a weekly or milestone claim can legitimately show none of it done. Both layouts label it "daily quests" so 0 / 3 reads as what is left today rather than as a bug.

Events

No new events. quest offers eligible now fires on the claim moment rather than on eligibility-by-state, which makes it a tighter denominator, and gains questType so it stays segmentable now that three cadences feed it. Historical rows have no such marker, but since the experiment has never run there are none that matter.

Testing

Several of these tests needed more than one attempt to be worth anything, which is worth knowing when reviewing them:

• A negative assertion inside waitFor resolves at t=0, before the offers query can settle, so it passes against the unfixed code. This bit the entry test, the claim-spend test and the expiry test.
• isFetchingOffers is dependable only as a negative: the fetch can settle before waitFor next polls, so positives are anchored on the modal instead.
• Fake timers stall React Query's notify queue, so the expiry test passed for the wrong reason until the flush was added.

Every behavioural test here was checked against the unfixed build: entry, claim-spend, expiry and the carousel label each fail without their fix.

shared 2716 / webapp 634 / extension 52 passing. The numberFormat and world/* failures also fail on main. typecheck-strict-changed and eslint clean. Rebased onto current main.

Manual Testing

• Claim a daily quest with the app open — popup appears
• Reload with that quest already claimed — no popup
• Claim a second quest in the same session — no second popup
• Claim a weekly quest — popup appears, count reads "0 / N daily quests"
• Below 656px (carousel) as well as above (split)

🤖 Generated with Claude Code

Preview domain

https://fix-quest-offers-claim-trigger.preview.app.daily.dev

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
daily-webapp	Ready	Preview	Sep 2, 2026 1:42pm UTC

1 Skipped Deployment

Project	Deployment	Actions	Updated
storybook	Ignored		Sep 2, 2026 1:42pm UTC

[rebelchris]

Approach is right — moving the trigger onto the mutation that every claim surface already goes through, with questClaimed.ts mirroring the existing questRewardAnimation.ts event precedent, is a smaller and more honest model than the count-rise baseline in the first commit. One blocking correctness gap (the session flag is set once and never cleared, so the reported "popup with no apparent trigger" can still happen inside a long session), plus two non-blocking notes, left inline.

Reviewed by AI.

[rebelchris]

Blocking: hasClaimedThisSession latches to true and is never cleared, so it stops being "the claim moment" and becomes a session-scoped standing fact — the same shape of bug that was reported, just bounded by the tab's lifetime instead of by a visit.

Two concrete scenarios where the claim does not produce a popup but the flag stays set:

1. No inventory / failed offers query. That path deliberately does not stamp the day (onShown never runs), and the flag stays true. Any later refetch of the offers query in the same session that comes back with inventory now satisfies shouldShow and opens the popup at an arbitrary moment, with no claim behind it.
2. Long-lived session. The webapp tab and the extension new tab routinely stay open past midnight. isTodayStamp(lastSeen) flips back to false on the new day while hasClaimedThisSession is still true from yesterday's claim, so the popup can open the next day on no action at all.

A sibling popup owning MODAL_KEY at claim time is a milder version of the same thing: the trigger silently defers until that modal closes, which may be much later than the claim.

Suggested direction: treat the claim as a one-shot rather than a flag — consume it (clear it once the effect has decided, whether it opened, found no offers, or bailed), or use the short-lived "claimed just now" marker the description contemplates, so the popup can only open in a window that genuinely belongs to the claim.

Reviewed by AI.

[rebelchris]

Non-blocking: the negative assertion now depends on a real 150ms wall-clock sleep, twice in this file. It is understood why a bare waitFor was not discriminating, but a fixed sleep is both slow and only probabilistically correct on a loaded CI box — if the entry-phase fetch ever takes longer than 150ms this test passes for the wrong reason and stops guarding the regression.

Can the settle be anchored on something deterministic instead — e.g. asserting the nock interceptor for the offers query was consumed (or explicitly not consumed, which is the actual claim being made on entry) — rather than on elapsed time?

Reviewed by AI.

[rebelchris]

Non-blocking (reviewability): the PR description still describes the first commit's approach — "the trigger now watches the claimed count rise", "whatever was already claimed when the dashboard first loaded is treated as history", "the baseline tracks downwards as well". 84b03dd replaced all of that with a QUEST_CLAIMED_EVENT listener and a session boolean; there is no baseline, no high-water mark and no rotation-reset case in the shipped code. Worth rewriting the body to the event-based design so the merge commit and any future bisect describe what actually landed.

Reviewed by AI.

[rebelchris]

Re-reviewed 14fc596. The blocking finding is resolved, and resolved in the right place: the token is spent inside the open effect once the offers fetch has settled, whichever way it decided, so an empty treatment cannot be revived by a later refetch, and the 30s window closes the tab-open-past-midnight and never-settles cases. The eligibility effect is declared before the open effect and gates on the same offersEnabled && areOffersPending condition, so it still logs in the commit where the fetch settles — the denominator is not lost to the spend. Deferring rather than spending while a sibling holds MODAL_KEY keeps the earlier behaviour intact. The test rewrite is a genuine improvement: fetchStatus during the mount commit is a deterministic anchor, and both new tests were verified failing against the latching build.

Two non-blocking notes below. CI is green on this sha (only the storybook preview deploy is outstanding).

Reviewed by AI.

[rebelchris]

Non-blocking: worth being explicit that 30s makes the sibling-deferral path a coin flip in practice — a boot popup or streak modal the user actually reads is often on screen longer than that, and the claim then expires unspent. The day stays unstamped so a later claim can still show, and the alternative (waiting indefinitely) is what we just removed, so this is the right trade for now. Just flag it as a number to revisit once there is data: if quest offers eligible fires materially more often than the modal is shown, this window is the first thing to look at.

Reviewed by AI.

[rebelchris]

Non-blocking (nit): !dashboard here is now unreachable — shouldShow already includes !dashboard, and the top of the effect returns on !shouldShow. Fine to leave, but dropping it would make the remaining condition read as purely "control, or no inventory".

Reviewed by AI.

[rebelchris]

Re-reviewed 984dac8. The cache-as-bus refactor is the right call and lands cleanly: it mirrors useLazyModal exactly (disabled query + queryFn reading the cache), the writer and reader both derive the key from questClaimQueryKey, and there is no query-cache persistence in the repo, so the claim cannot survive a reload and revive the original bug. Dropping lib/questClaimed.ts, the CustomEvent typing and the listener effect is a net simplification, and the commit message answers the two alternatives (mutation callback, folding offers into the mutation) before I could ask.

My remaining concern is not the mechanism but abadd89, which is a product change riding inside a fix PR. Details inline. CI note: test_shared, lint_shared and test_webapp are still pending on this sha — they were green on 14fc596, and the spec harness changed materially since (every renderAndClaim is now awaited), so worth confirming before merge.

Reviewed by AI.

[rebelchris]

Blocking (scope + experiment integrity): broadening the trigger from daily to daily/weekly/milestone is a deliberate product change, and featureQuestOffers is a live experiment. Landing it inside a PR whose stated job is fixing the trigger has three consequences worth deciding on explicitly rather than inheriting:

1. Enrolment population changes mid-flight. shouldEvaluate: shouldShow means the set of users who ever enroll is now larger and differently composed — weekly and milestone claimers who never finish a daily quest are in for the first time. Data before and after this merge is not comparable, and the split itself shifts.
2. Treatment copy changes mid-flight. / 3 quests → / 3 daily quests in QuestOfferCelebration alters what the treatment arm sees. Copy fidelity inside a running arm is something we have been bitten on before; the change is defensible, but it should be a decision by whoever owns the experiment, not a side effect.
3. The eligibility denominator changes shape. Adding questType to extra makes it segmentable going forward, but the historical rows have no such marker, so any analysis has to be cut at this deploy.

Suggested direction: land the trigger fix (0c5821c–14fc596) on its own, and take the broadening as its own PR with the experiment owner's sign-off on whether the current run is restarted or annotated. If the call is to keep them together, please say so in the description with that sign-off recorded, so the analysis is not read across a discontinuity nobody remembers.

Reviewed by AI.

[rebelchris]

Blocking (mobile): the label fix only landed on the split panel. QuestOfferCelebrationCompact further down renders `${summary.claimed}/${summary.total}` with no noun at all, so on the compact surface a weekly or milestone claim shows an unlabelled 0/3 directly above Quest complete — exactly the "reads as a bug" case this comment describes, on the narrower breakpoint where there is least context to infer it from. Worth giving both variants the same treatment, or deriving the label once so they cannot drift again.

Reviewed by AI.

[rebelchris]

Non-blocking (nit): an intro claim is written to the claim key by the mutation and then filtered out here, so pendingClaim is null and the expiry effect never runs — the intro claim sits in the cache until the next claim overwrites it. Harmless today, but it means the cache entry is "last claim" while the rest of the file reads it as "the pending moment". Filtering intro at the write side, or clearing the key when a claim is ignored, would keep the invariant true.

Reviewed by AI.

[rebelchris]

Re-reviewed 1d99a5a. All three findings from the last pass are addressed, and the shared ProgressCount is the better fix than labelling both call sites — the noun can no longer go missing on one layout, and the it.each over split/carousel guards it. Clearing an ignored claim at the read side keeps the cache key honest as "the pending moment".

On the experiment concern: verified independently rather than taken on trust. quest_offers in production is defaultValue: false with a single team-targeted force rule and no experiment rule, so there is genuinely no traffic and no arm has seen the old copy — the broadening and the copy change are free today, as the description now says. The rewritten description also matches the shipped design, and the trade-offs section records the 30s window and the reload behaviour, which is what I asked for.

One non-blocking layout question inline. No blocking findings remain from me.

Reviewed by AI.

[rebelchris]

Non-blocking (layout): the compact header previously rendered a single typo-mega2 token (0/3) in a left-aligned column; it now renders a typo-mega2 number plus a typo-title3 / 3 daily quests in a justify-center row, next to the level ring, on the narrowest breakpoint. The new test asserts the text is present but not that it fits. Worth a quick look at the carousel at ~320-360px to confirm it does not wrap awkwardly or push the ring — and if justify-center inside the items-start column shifts the alignment, that is the place it would show.

Reviewed by AI.