feat(get-app): smart-link QR + ship to logged-out desktop visitors (no flag) - #6399
feat(get-app): smart-link QR + ship to logged-out desktop visitors (no flag)#6399tsahimatsliah wants to merge 7 commits into
Conversation
The QR encoded daily.dev/apps, a landing page - one more tap between the scan and the store. It now encodes r.daily.dev/get, the same smart link the onboarding signup panel uses, which redirects by user agent straight to the App Store on iOS or Google Play on Android. utm_source=header_qr rides along because the redirect chain preserves query params end to end onto the final store URL (verified on both hops for both stores), so header scans stay distinguishable from onboarding scans - and on Google Play the utm param feeds install-referrer attribution. Same error-correction level H as before; scannability re-verified by decoding the rendered pixels at 96-160px. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
tsahimatsliah
left a comment
There was a problem hiding this comment.
Summary
Single-file asset swap: the header QR now encodes the OS-aware smart link. I verified the claim independently — parsed the SVG path into a 37×37 matrix and it matches a freshly generated QR for https://r.daily.dev/get?utm_source=header_qr exactly (version 5, ECC H, mask 6, 4-module quiet zone), so scannability and the logo knockout budget are unchanged. Scope is clean, CI is green, no code/analytics/flag changes.
No blocking findings. Two non-blocking comments inline: one factual correction about how far utm_source actually survives the redirect chain, and one about the URL now being duplicated between this asset and APP_URL with no automated consistency check.
Reviewed by AI.
Addresses both review findings on the QR smart-link change: The asset comment over-claimed attribution. Following the chain end to end, Apple's locale redirect strips query params (and App Store attribution only honours pt/ct campaign tokens), and Play install referrer reads a referrer param, not a bare utm_source. What the param reliably does is reach our own redirector, which is what distinguishes header scans from onboarding scans. The comment now claims exactly that, and points store-side attribution at the redirector as the right place to map tokens per store. The destination URL previously lived only inside an opaque path string, in duplicate with the onboarding panel's APP_URL, with nothing in CI noticing if one moved. New specs parse each committed matrix back into modules and decode it with jsQR the way a scanner would, asserting it still says what the code claims - decoding rather than re-encoding keeps the assertion valid regardless of which generator or mask produced the asset (the onboarding matrix uses a different mask than a fresh encode picks, so strict matrix equality would false-positive there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tsahimatsliah
left a comment
There was a problem hiding this comment.
Re-review of 4e36c9b
Both comments from the first pass are properly addressed, and the fixes are better than what I asked for.
Attribution claims (comment 1) — the asset comment and the PR body now state only what holds: the param reaches our own redirector (the part that's actually useful), survives onto the Play and Chrome Web Store URLs but is consumed by neither, and Apple strips it. Store-side attribution is correctly pushed to the redirector as a daily-api follow-up rather than being faked here. Matches what I measured per-UA.
Drift protection (comment 2) — decoding rather than re-encoding is the right call, and the reasoning holds: the onboarding matrix uses mask 3 while a fresh qrcode encode picks a different one, so matrix equality would have false-positived. I verified the new helper independently — parsing QR_PATH with the same rect grammar reproduces a 29x29 matrix that matches a freshly generated QR for APP_URL exactly at version 3 / ECC H / mask 3. So the parser is correct and the onboarding assertion is real, not vacuous. Covering both hand-committed matrices, not just the one this PR touched, closes the drift class rather than the instance.
jsqr as a devDependency checks out: cozmo/jsQR, Apache-2.0, ~7.4M downloads/month, test-only, no runtime bundle impact.
CI is fully green, including test_shared, so both new specs pass.
Findings
One nit inline (the asset comment describes re-encoding while the spec decodes), plus a residual gap worth noting: the specs pin the URL but not the error-correction level, and the logo knockout depends on level H.
Verdict
Comment only - no blocking findings. Good to merge from my side.
Reviewed by AI.
This comment has been minimized.
This comment has been minimized.
The asset note claimed the spec re-encodes the URL and compares matrices; it decodes the committed matrix with jsQR instead, which is why it survives a different generator or mask. Both QR specs now also assert the module count (37 header, 29 onboarding), so a regeneration at a weaker error-correction level fails CI rather than silently shrinking the logo knockout budget. Co-authored-by: Tsahi Matsliah <41912669+tsahimatsliah@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Product call: the header entry point goes to everyone logged out (anonymous visitors, post-page traffic) with no GrowthBook rollout, and the logged-in action rail drops its icon-only instance. The logged-out gate lives in the component itself so every call site inherits it, and the icon-only variant survives only as a Storybook reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tsahimatsliah
left a comment
There was a problem hiding this comment.
Re-review of 372549d + 9d9e46b
Two separate things landed since the last pass.
The nit and the ECC gap are both fixed (372549d). The asset comment now describes decoding rather than re-encoding, and both specs pin the module count (37 here, 29 for the onboarding asset). I re-derived both independently: 37 is version 5 at H for this URL, and the onboarding matrix is version 3 at H — so the pins are correct, and dropping to M would shrink the matrix and fail. That closes the level-H gap about as cheaply as it can be closed.
9d9e46b is a different change than the PR set out to make, and it deserves separate scrutiny: it deletes the header_get_app flag, ships the entry point to 100% of logged-out desktop visitors on merge, and removes the logged-in instance. The body documents the team decision, which is the right thing to have done. Three findings inline, one of which I'd fix before merge.
Findings
- Orphaned flag comment at the end of
featureManagement.ts— the constant is gone, its four-line comment (still saying "Keep the defaultfalse— GrowthBook ramps it") is not. Trivial, but please clear it before merge. - The one-time attention dot is now unreachable —
showDotrequires!showLabel, and the sole shipped call site passesshowLabel. Either the nudge was meant to survive, or the dot plushasSeen/isSeenFetched/GET_APP_SEEN_KEYare dead code that should go with it. InHeaderLoggedInstory now renders an anonymous context under a logged-in name, documenting a variant that can't ship.
Question on the rollout
Deleting the flag leaves no way to turn this off without a deploy — and this header renders in the extension too (the flag's own comment named "desktop webapp and extension visitors"), where turning something off means a store release rather than a deploy. Did the team decision account for the extension surface, or is it worth keeping the flag as a kill switch for one release and removing it after? Not a code defect either way; I'd just rather it be a decision than a side effect.
Verification
- Re-derived both pinned module counts against freshly generated QR codes at ECC H
- Traced the new render gate and every remaining
GetAppButtoncall site - Confirmed no remaining references to
header_get_app/featureHeaderGetApp - CI green: build, extension build, all package tests, strict changed-file typecheck
- Not verified by me: the preview deploy (logged-out header layout with the label button next to Log in / Sign up)
Verdict
Comment only. The QR work is done and well covered. One leftover to clear and one intent question on the dot; the rollout question is for the author, not a blocker.
Reviewed by AI.
Review follow-ups: the one-time dot only rendered on the icon-only variant, which no longer ships, so the dot and its persistent seen-flag go (and with them the wrapper span that only existed to host the dot). The flag's stranded comment in featureManagement.ts goes with its constant, and the misnamed InHeaderLoggedIn story is dropped in favour of the compact logged-out story as the icon-only reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tsahimatsliah
left a comment
There was a problem hiding this comment.
Re-review of 678ba7f
The three findings from the last pass are all properly closed, and I verified the cleanup rather than taking the commit message for it:
- Orphaned flag comment - gone with its constant; no references to
header_get_app/featureHeaderGetAppremain anywhere in the repo. - Dead attention dot - option 2, and it's complete: the dot branch,
hasSeen/setHasSeen/isSeenFetched,GET_APP_SEEN_KEY, theusePersistentContextimport and the wrapper<span>that only existed to host the dot are all removed, with no dangling reads ofgetAppHeaderSeenleft. Dropping the nudge for a labelled pill is a reasonable call and it's now recorded in the body rather than being a side effect. InHeaderLoggedInstory - deleted;InHeaderLoggedOutCompactis the single icon-only reference, in the anonymous context it would actually ship to, marked NOT shipped.
Also traced the shipped path end to end: HeaderButtons renders <GetAppButton showLabel /> only in the !isLoggedIn branch and only after isAuthReady && loadedSettings, so there's no logged-in flash and no second call site. The component-level gate is redundant with the container's, which is fine as the deliberate belt-and-braces the comment describes. Specs match the new gate (anonymous renders, logged-in renders nothing, below-laptop renders nothing, Android app vetoes the viewport), and CI is green on this commit including test_shared, test_extension, build_extension and strict changed-file typecheck.
Findings
- One inline, non-blocking: the viewport gate is JS-only, so the pill appears at hydration and reflows
Log in/Sign upon first paint for every anonymous desktop visitor. Both siblings in that row gate the breakpoint in CSS instead. - Nit: the spec covers the Android veto but not
isIOSNative(), which is the other half of the same reasoning and is a one-line mock. - Still open from the last pass: the extension surface. This header renders in the extension too, and with the flag deleted there is no way to turn the entry point off there without a store release. Not a code defect, and fine if the team weighed it - just worth an explicit answer rather than leaving it implied.
Verification
- Confirmed all removed flag/dot/persistent-key references are fully gone
- Traced the render gate and every remaining call site, including auth-readiness ordering
- Re-read the drift specs and helpers; both matrices still pinned by URL + module count
- CI green on
678ba7f(build, extension build, all package tests, strict typecheck) - Not verified by me: preview deploy of the logged-out header layout across breakpoints
Verdict
Comment only - no blocking findings. Good to merge from my side once the rollout question has an answer.
Reviewed by AI.
…n reflow useViewSize resolves only on the client, so the server HTML omitted the pill and hydration inserted it ahead of Log in / Sign up, reflowing the header on first paint for exactly the anonymous desktop traffic this ships to. The trigger now carries hidden laptop:flex like its LoginButton / ProfileButton siblings and the JS gate keeps only the checks CSS cannot express (logged-in, iOS/Android wrappers). Also covers the isIOSNative veto in the spec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Answering the open rollout question (extension surface): intentional, team-decided. The extension header is part of the audience — this entry point was scoped to "desktop webapp and extension visitors" from #6384, and the extension is exactly a desktop surface whose users may not know the mobile app exists. On the no-kill-switch trade-off: the button's failure surface is small and static (a pre-rendered QR asset and two store links — no network calls, no experiment logic), the webapp side can be changed same-day by deploy, and anonymous extension sessions are a small slice of extension traffic. If it ever needs to come off the extension specifically, that ships in a regular store release like any other extension change. We accept that latency rather than keeping a permanent flag alive for a static button. |
Changes
Follow-up to #6384 (now merged). Two things: the QR now points at the OS-aware smart link, and — per the team decision — the entry point ships to all logged-out desktop visitors with no feature flag.
1. Logged-out only, no flag
header_get_appis removed fromfeatureManagement.ts. No GrowthBook setup needed; merging this ships the button.GetAppButtonitself (isLaptop && !isLoggedIn && !isIOSNative() && !isAndroidApp), so every call site inherits the product decision.HeaderButtons. The icon-only variant survives only as a Storybook reference story (marked NOT shipped) in case it comes back.getAppHeaderSeenpersistent key, and the wrapper span that hosted it are all deleted rather than left as dead code.TestBootProviderdefaults to a logged-in session, so the specs opt into anonymous explicitly.2. QR smart link (earlier commits)
The QR asset encodes
https://r.daily.dev/get?utm_source=header_qrinstead ofhttps://daily.dev/apps?utm_source=header_qr.r.daily.dev/getis the same smart link the onboarding signup panel uses (LandingAppInstall.APP_URL): it redirects by user agent, so a phone scan lands directly on the App Store (iOS) or Google Play (Android) instead of a landing page with one more tap in the way.utm_source=header_qrreliably reaches our own redirector (api.daily.dev/get), which is what lets header scans be distinguished from onboarding scans (which use the bare link). The param also survives onto the Play / Chrome Web Store URLs, but nothing store-side consumes it there: Play install-referrer reads areferrer=param, and Apple strips query params entirely (its attribution only honourspt/ctcampaign tokens). Store-side attribution therefore belongs on the redirector — mapping the incomingutm_sourceinto Play'sreferrerand Apple'spt/ct— which is a daily-api change, not one this repo can make. Until then, the QR encoding is already the right shape for it.Drift protection (added after review)
The destination URL used to live only inside an opaque path string, duplicated against the onboarding panel's
APP_URL, with nothing in CI noticing if one moved. Two new specs now parse each committed matrix back into modules and decode it with jsQR the way a scanner would:getAppQr.spec.ts— asserts the header asset decodes to the expected URL and the viewBox matches the matrix.LandingAppInstall.spec.tsx— asserts the onboardingQR_PATHdecodes toAPP_URL.Decoding rather than re-encoding keeps the assertion valid regardless of which generator or mask produced an asset (the onboarding matrix uses a different mask than a fresh encode picks, so strict matrix-equality would false-positive). Adds
jsqras a shared devDependency.Events
No event changes. Store-button clicks keep logging
download appwithtarget_id: ios|android; scan volume is measured at the short link / redirector.Experiment
No — and no rollout flag either. Team decision (2026-07-30): visible to every logged-out desktop visitor on merge;
header_get_appdeleted.Manual Testing
[Get the app] [Log in] [Sign up], popover + QR + store buttons work with no flagshared(306 suites) andwebapp(45 suites) test runs green locally; changed-file strict typecheck passes🤖 Generated with Claude Code
Preview domain
https://claude-get-app-qr-smart-link.preview.app.daily.dev