Skip to content

feat(hijacking): cover-art strip arms for the next experiment - #6492

Open
tsahimatsliah wants to merge 20 commits into
mainfrom
feat/hijacking-cover-strip
Open

feat(hijacking): cover-art strip arms for the next experiment#6492
tsahimatsliah wants to merge 20 commits into
mainfrom
feat/hijacking-cover-strip

Conversation

@tsahimatsliah

@tsahimatsliah tsahimatsliah commented Aug 18, 2026

Copy link
Copy Markdown
Member

Changes

Two new arms on the extension new-tab hijacking strip, built from the daily.dev homepage's closing artwork with centered copy ("Start discovering what's next.") and a Get started → / Log in pair that hands off to the webapp onboarding flow (same OAuth-origin constraint as the cta arm):

  • cover — pinned to the top of the feed; cards scroll behind it.
  • cover_bottom — the same card pinned to the bottom of the viewport.

Where each arm renders

Arm Slot Why
default / cta / auth shortcuts (unchanged) in-flow strip, no pinning
cover feedTopContentFeedContainer's topContent inside the feed column, so it has the feed's width and a full-height container for sticky travel
cover_bottom after the feed (MainFeedLayout children) sticky bottom can only pull an element up into view, so it must render after the feed

MainLayout's topBanner was tried first and is not usable: it renders only inside the sidebarOwnsHeader branch, which requires isV2.

Pinning

The pinned arm measures whatever chrome is parked at the top of the viewport and clears it, plus an 8px gap so it reads as floating. The offset can't be a constant: the chrome is a one-row <header> on laptop, a two-row sticky <div> (FeedNav) on phone and tablet, and absent under v2. Verified against simulated chrome — 95px pins the card at 103px, 56px at 64px, none at 8px.

Width and height

Both arms span the feed column edge to edge, measured identical to the cards grid at 375 / 656 / 900 / 1280.

Banner height is a controlled variable in the header-ad-impression experiment (#6315). From tablet up, an invisible in-flow block reproduces the control's row geometry — its copy plus a real Button, and a media panel reserved by the control artwork's intrinsic ratio so no image is fetched to hold space:

Viewport Control Cover arms Δ
656 193 194 +1
900 176.59 177.59 +1
1020 176.59 177.59 +1
1280 176.59 177.59 +1
1440 194.09 195.09 +1

The +1px is structural: the control is flush with the feed and carries a top border only, the cover card is bordered on all four sides.

Below tablet the arms are deliberately shorter and size to their own copy (even 20px padding all round) — stacked, the control's geometry puts a band of empty artwork under the copy, and laying the copy over it clipped the heading. So mobile height is not held constant and should be read that way.

Extras:

  • New cover-hero-dome utility (pepper-90 radial via color-mix) for copy legibility over the artwork.
  • The artwork's subject anchor is measured, not fixed: object-cover scales by width, so the correct anchor runs from 49.5% at 958px to 61.5% at 1900px. It holds the dog and the person just under the CTA at any width.
  • Storybook page Extension/HijackingStrip: one story per arm over a scrollable mock feed, each rendering on the side of the feed it uses in production, booting anonymous through a story beforeEach.

pnpm --filter extension test (lint + 54 tests) ✅ · @dailydotdev/shared (2196 tests) ✅ · typecheck-strict-changed

Known gaps

  • Pinning and geometry are verified in Storybook and in a side-loaded build of this branch; jsdom has no layout, so the tests assert the positioning contract only.
  • The cover artwork is served at its 2072px source. Its Cloudinary URL is signed, so a width cap can't be appended — it needs a re-upload or re-sign to serve smaller.
  • The chrome lookup matches on the z-header layer. If that class is renamed, the offset silently falls back to the bare 8px gap.

Events

No new events — the arms reuse the existing hijacking impression/click events (SignupButton/LoginButton target types), exactly as the current arms do.

Experiment

Two new values (cover, cover_bottom) on the existing hijacking_variants3 flag. The committed default stays default (control); the arms only serve once the experiment is configured in GrowthBook.

Enrollment is scoped so every arm renders for the same population. The control reaches the page through the shortcuts slot, which MainFeedPage drops under the v2 layout, while the cover arms render from the feed column, which every layout keeps. Enrolling v2 users would have compared "no strip" against "a strip" and flattered the new arms, so the flag is not evaluated for them and no arm renders there. A test pins this.

Consequently no layout_v2 targeting rule is needed — those users are excluded in code, and excluded from enrollment rather than merely from the UI, so they do not dilute the split.

Important

Please do not merge the PR until the experiment enrolment is approved (message pending in #experiments).

🤖 Generated with Claude Code

Preview domain

https://feat-hijacking-cover-strip.preview.app.daily.dev

Two new arms on hijacking_variants3, both reusing the daily.dev
homepage's closing artwork with centered copy and a Get started /
Log in pair that hands off to the webapp onboarding flow:

- cover: sticky at the top of the feed, cards scroll behind it
- cover_bottom: the same card floating fixed at the viewport bottom

Banner height is a controlled variable in the header-ad-impression
experiment, so the card keeps two invisible in-flow sizers that
replicate the control strip's exact geometry (its cat artwork and its
text block); all visible content overlays them absolutely. Verified
pixel-identical to the control at mobile/desktop/wide widths.

Also adds a Storybook page (Extension/HijackingStrip) rendering every
arm of the flag over a mock feed, with an anonymous boot mode for
ExtensionProviders so logged-out arms render.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
daily-webapp Ready Ready Preview Aug 20, 2026 9:21am
storybook Building Building Preview Aug 20, 2026 9:21am

Request Review

Stacked, the control's 12.5rem media panel sat below the overlaid copy as
dead space. The vertical layout now keeps only the text sizer (206px, down
from 447px); tablet and up still mirror the control's full row geometry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review of the first pass turned up problems that would have invalidated
the experiment:

- The arms rendered through the `shortcuts` slot, which FeedContainer puts
  inside its search <header>. A sticky child there has almost no travel, so
  the pinning never happened outside Storybook. The cover arms now render
  through `topBanner`, whose column spans the feed — the same slot
  ExtensionSignInStrip already pins from.
- The height sizer replicated the CTA arm rather than the control, leaving
  the strip 44px taller than the control on mobile. It now reproduces the
  control's own text block (its copy and a real Button) and reserves the
  media panel with the control artwork's intrinsic ratio, landing within
  the same 1px the CTA arm already carries.
- Sign-back and onboarding states dropped the arms back into the top slot,
  so a remembered visitor watched the strip jump after storage resolved.
  Placement is now shared across every state of an arm.
- The bottom arm sat at z-3, under the sidebar (z-70) and over the
  scroll-to-top button (z-2). It now uses the shared bottom-banner
  primitive: z-modal, centered, clear of the button's corner.
- The sizer fetched the control's cat artwork (~88KB) only to hide it.
- The storybook boot mock was mutated during render and never restored,
  leaking an anonymous user into later stories; it moves to a story
  `beforeEach` with a per-mount query client.
- The arm dispatch is derived from the hero map, so a future arm can't be
  added to the enum and map yet silently fall through to the control.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The z-index fix had capped the pinned card at 40rem and centered it, so it
no longer lined up with the feed. It now spans the viewport gutters up to
the feed's own max width, measuring identical to the cards field
(960px at 160-1120 on a 1280 viewport).

The cap is an inline style: an arbitrary `max-w-*` class from this package
is not guaranteed to reach the compiled bundle, and a silently dropped one
had already collapsed the card once.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both arms now span the feed column edge to edge, matching the cards
exactly at every breakpoint (measured identical width and both edges at
375/656/900/1280).

The top arm dropped the control's 16px horizontal inset. The bottom arm
stopped being `fixed`: a fixed element sizes against the viewport, so it
was 16px wider than the column below 1280 and could not track it at all.
It now renders after the feed via MainFeedLayout's children, where
`sticky bottom` pins it to the viewport while it inherits the column's
width — and where it no longer needs z-modal to clear the sidebar or a
carve-out for the scroll-to-top button.

`sticky bottom` can only pull an element up into view, which is why the
arm has to render after the feed rather than above it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MainFeedPage's MainFeedLayout lost its showDailyHome branch upstream;
the feedTrailing children carry over onto the unconditional render.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MainLayout renders `topBanner` only inside its `sidebarOwnsHeader` branch,
and that requires `isV2` — so the slot exists only under the layout_v2
experiment. The top cover arm rendered there, which meant it would have
shown to layout_v2 users only and silently to nobody else, confounding
the two experiments.

It now goes through a new `feedTopContent` pass-through on MainFeedLayout,
landing in FeedContainer's `topContent` inside the feed column: present in
every layout branch, correct width, and a full-height container for the
sticky travel the arm needs. The bottom arm was never affected — it
renders through MainFeedLayout's children, which both branches render.

`topContent` still falls back to the reading-reminder hero when neither
this nor the chips row is set.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tsahimatsliah and others added 2 commits August 19, 2026 01:13
…k's subject

Two problems visible only in the real extension:

- The global header is `fixed top-0 h-14 laptop:h-16` at z-header, so a
  `sticky top-0` strip pinned underneath it and had its headline clipped.
  The top arm now offsets by the header's height, and only when that header
  is on screen — the v2 layout hides it and gives the sidebar the job.
- The cover artwork was anchored at 35% vertically, which reads fine in a
  tall frame but crops the dog and the person out entirely once the strip
  is as wide and short as the real feed column. Anchored low instead so the
  subject stays in frame.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The arbitrary `object-[50%_54%]` class was not emitted for this package, so
the crop silently fell back to centre and dropped the artwork's subject.
Same failure mode as the width cap: inline it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e the feed

Two problems the real extension surfaced:

- A feed card's avatar painted over the strip. The strip is a deliberate
  interrupt, so it now sits at z-rank rather than z-3 — above card content,
  still below the sidebar, header and modals.
- The dog and the person kept disappearing. `object-cover` scales the art by
  the card's width, so where the pair lands depends on how wide the card is:
  the anchor that frames them at 958px is 49.5%, at 1900px it is 61.5%. Any
  fixed percentage loses them at some width, which is what happened. The crop
  is now measured from the rendered box and re-measured on resize, holding the
  pair 4px under the CTA row at every width.

The pair is taller than the space under the buttons, so their lower half stays
cropped by the card edge — the strip would have to grow to fit them whole, and
its height is fixed by the experiment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The pinned arm sat flush against the fixed header. It now clears it by the
  header's height plus a small gap, so the card reads as floating over the
  feed instead of welded to the chrome. The offset is inline because it
  depends on the breakpoint (3.5rem header on mobile, 4rem on laptop) and on
  whether the header is on screen at all — the v2 layout hides it.
- The centred copy is lifted so the band under the CTA belongs to the
  artwork. The dog and the person now show 54px rather than 36px at the
  common width; they were being squeezed out by copy sitting too low.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The offset was a constant matching the laptop header (4rem). On phones and
tablets the header stacks a nav row under the logo — roughly 95px — so the
strip pinned underneath it and the headline was cut off, which is what the
constant could never handle.

The offset is now measured from whatever is actually parked at the top of the
viewport, and re-measured on resize and scroll. Verified against simulated
headers: 95px chrome pins the card at 103px, 56px chrome at 64px, and with no
chrome it keeps just the 8px floating gap.

Measured synchronously rather than on an animation frame so the first paint
lands on the right offset; React drops the update when the value is unchanged,
so the scroll listener stays cheap.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On phones and tablets MainLayoutHeader returns <FeedNav />, whose root is a
sticky <div> rather than a <header>. The offset lookup only matched header,
nav and role=banner, so it measured nothing there: the strip pinned 8px from
the top and FeedNav, sitting on the z-header layer, covered it — the strip
appeared to have a sliver of padding and then slid behind the header on
scroll.

What both chromes share is that layer, so the lookup now matches it too.
Verified against a simulated FeedNav: 95px of sticky chrome pins the card at
103px, and it returns to the bare 8px gap once that chrome is gone.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stacked, the copy is taller than the control's row geometry, so laying it out
absolutely over that geometry made it overflow the card and clip its own
heading against the top edge.

The copy now sits in flow below tablet, giving the card its height with an
even 20px on every side, and goes back to overlaying the control's geometry
from tablet up — which is the width range where banner height is the
experiment's controlled variable. Desktop is unchanged at 177.59px against
the control's 176.59px.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The control reaches the page through the `shortcuts` slot, which MainFeedPage
drops under the v2 layout, while the cover arms render from the feed column,
which every layout keeps. A v2 user in the control therefore saw no strip at
all while one in a cover arm saw the new card — the comparison would have been
"nothing" against "a strip" rather than one design against another, flattering
the new arms.

The flag is no longer evaluated for those users, so they are not enrolled and
no arm renders for them. A test pins this: enrollment must not happen where
the control cannot show.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five throwaway extension builds were swept into earlier commits by `git add
-A`. They are local review artifacts, not part of the change, and ignoring
them keeps the next one out.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@tsahimatsliah tsahimatsliah left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Two new arms (cover, cover_bottom) on the existing hijacking_variants3 flag, plus a feedTopContent/feedTrailing slot pair so the pinned arms can render from the feed column. The slot analysis is right (topBanner really is v2-only, and sticky bottom really does need to render after the feed), the committed default stays default, the map-derived arm dispatch is a nice touch, and the height-parity work is unusually thorough for an experiment PR.

Two blockers though — one is an enrollment leak that defeats the v2 exclusion the PR is built around, the other is the height-parity sizer, which already disagrees with the control it is copying. Details inline.

Blocking

  • HijackingLoginStrip.tsx:843useLayoutVariant().isLoading is discarded, so v2 users do enroll during the pre-auth window and then see no strip at all. This is exactly the contamination the PR says it prevents.
  • HijackingLoginStrip.tsx:436 — the invisible sizer hand-copies DefaultHijackingStrip's text block from ~100 lines below in the same file, and copies the wrong branch of its conditional paragraph.

Question (potentially blocking)

  • HijackingLoginStrip.tsx:313 — unthrottled querySelectorAll + getComputedStyle + getBoundingClientRect on every scroll event, on the extension new tab. The header is fixed h-14 laptop:h-16; what does measuring buy over a static sticky offset?

Non-blocking

  • Duplicate useTopChromeOffset mount (:598)
  • 2072px artwork on every new tab (image.ts:487)
  • Imperative objectPosition measurement (:329)
  • Unrelated .gitignore entry
  • Impression re-fires when the laptop breakpoint is crossed
  • Ad-viewability question for the pinned arms

Verification

  • Root + package AGENTS.md read; scope reviewed
  • Enrollment path traced: BootProviderGrowthBookProvider (cachedBootData?.exp) → useConditionalFeaturetrackingCallback/e/x
  • MainFeedLayout blast radius checked — feedTopContent is additive and chipsTopContent preserves the previous undefined behaviour for every webapp consumer
  • topContent render position in FeedContainer checked (.cards is a max-width, not the grid, so the full-width claim holds)
  • Sizer markup diffed against DefaultHijackingStrip
  • Not verified by me: real pinning/geometry (jsdom has no layout, as the PR notes), and the side-loaded extension build

Verdict

Comment — the enrollment gate and the sizer should be fixed before this is enrolled, which lines up with the do-not-merge note pending #experiments approval.

Reviewed by AI.

const { value, isLoading } = useConditionalFeature({
feature: featureHijackingVariants,
shouldEvaluate: true,
shouldEvaluate: !isV2,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (enrollment leak): useLayoutVariant returns { isV2, isLoading } and only isV2 is used here (and again at :866). isV2 is shouldEvaluate && value === true where shouldEvaluate = isAuthReady && isLaptop, so it is false while auth is still resolving — indistinguishable from "resolved, not v2".

That window is real in the extension, not theoretical. BootProvider passes experimentation={cachedBootData?.exp} — the localStorage-cached boot — into GrowthBookProvider, while isAuthReady is fed from isBootReady = isFetched && !isError, i.e. the network boot. So on a new tab GrowthBook flips ready from cache first, and for that whole round trip:

  • useLayoutVariant: shouldEvaluate false → isV2 = false, isLoading = true (discarded)
  • here: shouldEvaluate = !isV2 = true, ready = true → the flag is evaluated → trackingCallback → allocation POST /e/x

A laptop v2 user is therefore enrolled, and then isV2 resolves to true and HijackingLoginStrip returns null. Those users are in the split with no strip in any arm — precisely the "no strip vs a strip" comparison the PR is designed to avoid, just arriving through the loading state instead of the settled one.

The existing test can't catch it because it pins { isV2: true, isLoading: false }; the leak only exists while isLoading is true.

Suggested fix — consume the flag you already have:

const { isV2, isLoading: isLayoutLoading } = useLayoutVariant();
const { value, isLoading } = useConditionalFeature({
  feature: featureHijackingVariants,
  shouldEvaluate: !isLayoutLoading && !isV2,
});

if (isLayoutLoading || isV2 || isLoading) {
  return null;
}

Same in useHijackingPlacement. Worth a test asserting shouldEvaluate: false while { isV2: false, isLoading: true }, since that is the state that actually leaks.

Reviewed by AI.

<div className="flex flex-1 flex-col items-center p-5 tablet:items-start tablet:p-6">
<div className="flex flex-col items-center gap-1 tablet:items-start">
<h3 className="font-bold typo-title2">
Unlock the full daily.dev experience

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (manual sync the experiment depends on, and it has already drifted): this invisible block hand-copies DefaultHijackingStrip's text block — heading, paragraph, Button label, paddings — from ~100 lines below in this same file. Nothing keeps the two in step, and the copy is already wrong: the control's paragraph is conditional,

{isLoggedOut
  ? 'Log in to pick up where you left off.'
  : 'You still have a few onboarding steps left. Finish them to unlock the full experience.'}

while the sizer always uses the short logged-out string. The long branch wraps to more lines at every width in your table, so the control is taller for users with onboarding incomplete and the height-parity claim only holds for the logged-out state. (Those users hit the chrome() branch on the cover arms, which has no sizer at all — a third height.)

The deeper problem is that this is the experiment's controlled variable resting on a copy-paste no reviewer or CI job can verify. If someone changes the control's heading, this silently stops reserving the right height and #6315's banner-height control is quietly broken with a green build.

Suggestion: extract the control's text block (heading + paragraph + Button + the media panel's ratio box) into one small component and render that here inside the aria-hidden/invisible wrapper, so the geometry is the control's by construction rather than by resemblance. Failing that, a test asserting the sizer's rendered strings equal the control's would at least make drift loud.

Reviewed by AI.


update();
window.addEventListener('resize', update);
window.addEventListener('scroll', update, { passive: true });

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question (potentially blocking, perf): update runs measureTopChrome() on every scroll event with no throttle and no rAF. Each call does a querySelectorAll over 'header, nav, [role="banner"], [class*="z-header"]', then getComputedStyle and getBoundingClientRect for every match — a forced style/layout recalc per scroll event, on the extension's new tab, which is the hottest surface we ship. passive: true avoids blocking the gesture but does nothing about the layout thrash.

And the value being computed is static: the global header is fixed top-0 h-14 laptop:h-16, FeedNav is the same two-row sticky at phone/tablet, and v2 is excluded from this experiment entirely. A plain sticky top-[3.875rem] laptop:top-[4.5rem] (or a small calc off the header tokens) reproduces your 103px/64px/8px numbers with no JS, no listener, and no [class*="z-header"] substring match — which you already flag as silently degrading to a bare 8px gap if the class is renamed.

What does measuring achieve here that the static offset doesn't? If there is a case I'm missing, could it at least be rAF-coalesced and scoped to a known container rather than a document-wide selector?

Reviewed by AI.

const { showLogin, user } = useAuthContext();
const { logEvent } = useLogContext();
const { signBack, provider, isLoaded: isSignBackLoaded } = useSignBack();
const chromeTopOffset = useTopChromeOffset(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (duplicate work): for the cover arm this hook is mounted twice — here in HijackingHeroStrip, and again inside CoverSignupHero — so two scroll/resize listener pairs and two independent state updates compute the same number on every scroll event. Only one of the two consumers can render at a time (chrome() for the sign-back/onboarding states, SigninHero otherwise), so one of them is always doing the work for nothing.

Measuring once here and passing the offset down (or moving it entirely into the shared chrome/card wrapper) would halve it. If the static-offset suggestion above lands, this disappears with it.

Reviewed by AI.

// extension hijacking strip's cover arms. The signature covers the
// transformation string, so a width cap can't be appended here — the asset
// has to be re-uploaded or re-signed to serve smaller than its 2072px source.
export const cloudinaryHijackingCoverArt =

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (perf), and the known-gaps note is the right call: a 2072px source with no width cap is heavier here than in the webapp equivalent, because this decodes on every new tab for users in the treatment arms — the highest-frequency surface in the product. It's cached after the first fetch, but the decode cost recurs per tab, and the arms it lands on are the ones being measured.

Could the re-upload/re-sign happen before this ramps past a small percentage? Otherwise the arm carries a per-tab cost the control doesn't, which is a confound rather than just a perf nit.

Reviewed by AI.

// the art up and crops most of its height — where that band lands depends on
// the card's width, and no single percentage keeps the pair in frame across
// the range. The crop is therefore measured rather than guessed.
const SUBJECT_TOP_FRACTION = 0.67;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: useCoverArtAnchor wires a ResizeObserver plus a load listener to imperatively write style.objectPosition on every resize. Given the inputs are the card width and a fixed SUBJECT_TOP_FRACTION, and the PR already documents the measured answers (49.5% at 958px → 61.5% at 1900px), a handful of breakpoint-keyed object-position classes would get the same result declaratively — no observer, no imperative style mutation, no first-paint jump between the initial object-cover centre and the measured anchor.

Also worth confirming: the load listener is attached in useLayoutEffect, so a cached image that has already fired load before the effect runs relies on the synchronous update() seeing a non-zero naturalWidth. That holds for a warm cache but not obviously for a decode still in flight — is there a state where neither path fires?

Reviewed by AI.

Comment thread .gitignore Outdated
.claude/drafts/ No newline at end of file
.claude/drafts/
# Local review builds of the extension, never committed
dailydev-cover-strip-REVIEW-*.zip

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (scope): dailydev-cover-strip-REVIEW-*.zip is an artifact of one author's local review build, not something the repo produces. Repo-wide ignores for personal build output tend to accumulate; a global gitignore or a *.zip entry (if we want one at all) would keep this branch to the feature.

Reviewed by AI.

const { isV2 } = useLayoutVariant();
const { value, isLoading } = useConditionalFeature({
feature: featureHijackingVariants,
shouldEvaluate: !isV2,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (analytics): two smaller things about the new v2 gate, separate from the enrollment blocker above.

  1. isV2 depends on useViewSize(ViewSize.Laptop), so resizing across the laptop breakpoint (or opening a new tab in a small window and maximising it) unmounts and remounts the strip. hasLoggedImpression is a useRef, so it resets on remount and a second hijacking impression fires for the same session — inflating impressions in whichever arm the user is in. Cheap to make this idempotent per session.
  2. Both useHijackingPlacement and HijackingLoginStrip call useConditionalFeature for the same flag from different components. Harmless today, but the placement decision and the render decision can only stay consistent by accident. Lifting the resolved variant into HijackingPage and passing it down would make them one source of truth.

Also a product question rather than a code one: with cover pinned over the feed and cover_bottom pinned to the viewport bottom, both permanently occlude part of the feed. Since banner height is a controlled variable in #6315 specifically because of ad impressions — do these arms reduce in-feed ad viewability, and is that being watched alongside the signup metric?

Reviewed by AI.

… cost

Blocking:

- The layout hook's `isLoading` was discarded, and `isV2` reads false while
  auth resolves. The extension boots GrowthBook from its localStorage cache
  before that settles, so laptop v2 users were enrolled during the window and
  then shown nothing — the same contamination the v2 gate exists to prevent,
  arriving through the loading state. Both call sites now wait for the layout
  to resolve before evaluating, with a test pinning the leaking state.
- The height sizer hand-copied the control's text block and copied the wrong
  branch of its conditional paragraph, so parity only held for logged-out
  users. The control's text column and media panel are now one component that
  the control renders visibly and the cover arms render invisibly, so the
  geometry is the control's by construction. The shared sign-back/onboarding
  card reserves it too, which it never did — that was a third height.

Perf and analytics:

- The chrome offset no longer measures on scroll. Its height cannot change
  there (`useScrollTopClassName` swaps background colours, not geometry), so
  the listener was forcing a style and layout recalc per scroll event on the
  new tab to recompute a constant. Mount and resize only.
- The offset is measured once and passed down instead of being mounted twice.
- The impression is now once per tab rather than once per mount: crossing the
  laptop breakpoint remounts the strip and was logging a second one.
- Dropped the unrelated .gitignore entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tsahimatsliah

Copy link
Copy Markdown
Member Author

Review addressed

Both blockers fixed, the perf question answered with a change, and all six non-blocking notes handled or answered. 794b4541.

Blocking

Enrollment leak while the layout resolves (HijackingLoginStrip.tsx:843) — correct, and the diagnosis of why was the useful part: isV2 is false during the pre-auth window and indistinguishable from a settled "not v2", while GrowthBook is already ready off the localStorage boot. Both call sites now wait for the layout hook to resolve before evaluating:

const { isV2, isLoading: isLayoutLoading } = useLayoutVariant();
const { value, isLoading } = useConditionalFeature({
  feature: featureHijackingVariants,
  shouldEvaluate: !isLayoutLoading && !isV2,
});

Test added for { isV2: false, isLoading: true } — the state that actually leaked, which the old test could not reach.

Sizer drift (:436) — also correct, including the detail that it copied the wrong branch. Extracted ControlTextColumn + ControlMediaPanel: the control renders them visibly, the cover arms render the same components invisibly, so the geometry is the control's by construction rather than by resemblance. The paragraph is conditional in both now.

Your parenthetical about the chrome() states being a third height was right too, and the parity test caught it the moment I wrote it — the shared sign-back/onboarding card reserved nothing. It now reserves the control's geometry for the cover arms (cta/auth untouched). Parity is covered by a test across both auth states.

Perf question

Fair challenge, and you were right that the value is static — but for a reason worth recording: useScrollTopClassName only swaps background colours, so the chrome's height cannot change on scroll. The scroll listener was forcing a style and layout recalc per event to recompute a constant. Removed — mount and resize only.

I kept the measurement rather than a static offset because the chrome isn't one height: it's h-14/laptop:h-16 on laptop but FeedNav's two-row sticky (~95px measured) on phone and tablet, and a static value that clears one tucks under the other — that was the bug reported from the real extension two rounds ago. Verified against simulated chrome: 95px → 103px, 56px → 64px, none → 8px. The [class*="z-header"] fragility stands and is in Known gaps; a data- attribute on both chrome roots would fix it properly if you'd prefer that as a follow-up.

Non-blocking

  • Duplicate hook mount (:598) — fixed, measured once in HijackingHeroStrip and passed down.
  • Impression re-fires on breakpoint cross — fixed, now once per tab via module scope rather than a per-instance ref, with a remount test.
  • Two useConditionalFeature call sites — still two, deliberately: lifting the variant into HijackingPage means the page must resolve the flag to decide the slot before the strip renders, which reintroduces a second consumer at a different level. GrowthBook dedupes exposure per experiment, so this is a readability point rather than a correctness one. Happy to do it if you disagree.
  • .gitignore — reverted, the file now matches main. It was mine and it was noise.
  • 2072px artwork — agreed this is a confound, not a nit: the treatment arms carry a per-tab decode the control doesn't. It needs a re-upload or re-sign (the URL is signed, so a width cap 404s — I tried). Worth doing before ramping past a small percentage, and I'd treat that as a gate rather than a follow-up.
  • Imperative objectPosition — kept. The anchor is continuous in card width (49.5% at 958px → 61.5% at 1900px), so breakpoint classes approximate it and lose the pair at the widths between. On the race you asked about: update() runs synchronously in the layout effect and reads naturalWidth, so a warm cache is covered; a decode in flight is covered by the load listener. The only uncovered state is a broken image, where there's nothing to anchor.

Ad viewability

Not currently watched, and it should be. Both arms occlude feed area permanently — cover while pinned, cover_bottom over the fold — so in-feed ad viewability is the metric most likely to move against us while signups improve. Flagging for the #experiments message so it's tracked alongside the signup metric rather than discovered afterwards.

Verification

pnpm --filter extension test (lint + 58 tests, 5 new) ✅ · @dailydotdev/shared (2196) ✅ · typecheck-strict-changed

Still not verified by me, unchanged from the PR body: real pinning and geometry beyond Storybook and a side-loaded build of this branch.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant