Repository navigation
chore(layout): graduate layout v2, drop the flag and the v1 chrome - #7017
Conversation
The dual-sidebar layout won its experiment, so the `layout_v2_2` flag goes and the laptop layout becomes the only laptop layout. `useLaptopLayout` replaces `useLayoutVariant().isV2`: it is the laptop breakpoint, held at the server's answer (the mirrored `/layout-v2` shell, or none) until the client has measured the viewport, so the server HTML and the first client render agree and the chrome never flips because a flag resolved late. Every `isLoading` gate that waited for GrowthBook (the sidebar hold, the top hero, the CV banner, the extension hijacking strip) is gone with it, and the content padding no longer animates its first settle on any page. The `/layout-v2` post-page mirror and its `dl_v` cookie stay: prerendered pages are cached per path, so the only way a logged-in laptop session gets the rail and the floating card in its initial HTML is a path that carries the shell. The cookie now records "logged in on a laptop" rather than the flag's answer. Removed with the control arm: `SidebarDesktop` (v1) and its spec, the v1 expanded-sidebar padding (`screenCentered` is accepted but no longer read), the laptop-only v1 height branches in the agent shell, the `!isV2` gates in `DiscoverSection` (which only the laptop sidebar renders now), and the storybook history page comparing the two navigations. Branches that still read the hook pick between the laptop and tablet chrome. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Reviewed the full diff (67 files) against main, plus the code it leaves behind. Most of it is a safe mechanical swap from isV2 to isLaptopLayout. The flag and the v1 SidebarDesktop are gone, and the mirror route, cookie and hydration agreement still hold on the post page.
Blocking
useLaptopLayoutreturnsfalseon the first render of every mount, not only during hydration. Laptop client navigations and every extension new tab now render the tablet chrome for one commit, which also causes double or phantom impressions (CV banner, hijacking strip). Suggested fix: use the existinguseIsHydrated. Details are inline.
Control/v1 leftovers not covered inline (these files are not in the diff)
sidebar/sections/CustomFeedSection.tsxis now dead. Its only renderer was the deletedSidebarDesktop, and V2 only mentions it in a comment.sidebar/SidebarMenuIcon.tsxis now dead (onlySidebarDesktopimported it).NetworkSectionandCustomFeedSectionkeep!isV2Panelbranches that can no longer run, because their only renderer passesisV2Panel: true.BookmarkSectionstill needs the false branch for the tablet bookmarks page.useFeedLayout().screenCenteredOnMobileLayoutno longer has a consumer.- Storybook
stories/sources/shell.tsx(SignedOutRail: "SidebarDesktop (v1)… production still serves the legacy 44px sidebar to visitors",laptop:pl-11) andstories/sources/states.tsx(~381) still draw the old signed-out laptop chrome. This PR changes exactly that surface for anonymous laptop visitors. - Already dead on
main, but in the same area:sidebar/SidebarProfileCompletion.tsxhas no importers.
Coverage
Sidebar.spec.tsxwas deleted rather than ported. Some of its cases cover behaviour that is not specific to v1, such as the login requirement before an anonymous user opens Following, and section collapse and dividers. Please move the cases that still apply to the rail intoSidebarDesktopV2.spec.tsx.
Product checks worth stating in the PR
- Users from the control arm become eligible for the sidebar tour (
useSidebarTourState) and the Agents entry in Discover (showAgentis now evaluated for every logged-in user). This is presumably intended, but please confirm. - Anonymous laptop visitors now get the rail under the legacy header. Please add screenshots for anonymous and logged-in laptop at 1024 and 1440 on the final head, plus one client-side navigation (feed to wallet to settings) with a performance recording, to confirm nothing flips after paint.
CI: typecheck, lint, the webapp and extension tests and the extension build are green. test_shared was still running when I checked.
Reviewed by AI.
| // `useViewSize(ViewSize.Laptop)` check is needed at the call site. | ||
| export const useLaptopLayout = (): boolean => { | ||
| const serverVariant = useContext(LayoutVariantContext); | ||
| const isLaptop = useMediaClient(laptopQueries, laptopValues, false); |
There was a problem hiding this comment.
Blocking: every component that mounts reads false on its first render, including after client-side navigation, not only on hydration.
useMediaClient starts as undefined on every mount and only settles in an effect. Outside the mirror, the hook therefore returns false for the first commit of every newly mounted consumer. The old useLayoutVariant used the synchronous useViewSize and a cached flag, so after the first load the v2 arm got the right value on its first render.
Effect on laptop, for everyone after this merges:
- Client navigation. Going to
/wallet,/notifications,/analytics, settings, bookmarks, jobs and so on renders the tablet branch first (desktopHeader,LayoutHeader,ProfileSettingsMenuDesktop, Float buttons,max-w-5xl) and then swaps inPageHeaderand the compact chrome. Route changes are not discrete updates, so the passive effect most likely flushes after paint. That makes it a visible flash plus layout shift on most laptop page changes. - Extension new tab. There is no SSR there (
createRoot), so nothing needs to be held at all. Every new tab still pays the same extra commit: the hijacking strip, the shortcuts row in the feed slot, and the inline reading reminder. - Impressions. Components that log on mount now log in an arm the user never sees (see the
FeedContainerandHijackingLoginStripthreads).
The repo already has the primitive for this: useIsHydrated (useSyncExternalStore). It is false only on hydration renders, true on every later mount, and true from the first render under createRoot:
export const useLaptopLayout = (): boolean => {
const serverVariant = useContext(LayoutVariantContext);
const isHydrated = useIsHydrated();
const isLaptop = useViewSize(ViewSize.Laptop);
return isHydrated ? isLaptop : serverVariant === 'v2';
};This keeps the server and hydration renders in agreement, which is what the hook is meant to do, and it removes the post-mount flip everywhere else. Could you add a spec that mounts a consumer after hydration (or under a plain render) and asserts true on its first render at laptop?
Reviewed by AI.
| // evaluated, wait for it: mounting in the wrong arm first logs a second | ||
| // impression. | ||
| const isLayoutVariantPending = isLaptop && isLayoutVariantLoading; | ||
| const shouldEvaluateBanner = |
There was a problem hiding this comment.
Blocking: CV banner impression is logged twice. The guard removed here had a written reason: "mounting in the wrong arm first logs a second impression". That reason still holds, because useLaptopLayout is false on the first render of any FeedContainer mount (see the useLaptopLayout thread).
Scenario: a laptop user with the CV marketing CTA navigates client-side to My Feed. Settings are already loaded, so the first render mounts the inline ProfileUploadBanner, and its mount effect logs Impression. The next commit unmounts it and mounts CvTopHero in the portal, which logs Impression again. The same thing makes the reading reminder in Feed.tsx render inline for one commit before it moves to TopHeroPortal.
Fixing the hook as suggested resolves both. If the hook stays as it is, the hold has to stay.
Reviewed by AI.
| return null; | ||
| } | ||
|
|
||
| export default function HijackingLoginStrip(): ReactElement { |
There was a problem hiding this comment.
Blocking (extension): the strip flashes on laptop and can log an impression nobody saw. MainFeedPage drops the shortcuts slot when isLaptopLayout is true. Under createRoot, though, useLaptopLayout is false on the first render, so HijackingHeroStrip mounts for one commit and then unmounts. If user is set, isReadyToLogImpression is already true, so the hijacking impression fires for a strip that was never seen. That skews the running hijacking experiment. The two deleted specs covered exactly this case.
With the useIsHydrated fix on the hook, the extension gets the right value on its first render and this wrapper can go. When it does, please inline HijackingHeroStrip as the default export rather than keeping a pass-through.
Reviewed by AI.
|
|
||
| useEffect(() => { | ||
| if (isExtension || isLoading) { | ||
| if (isExtension) { |
There was a problem hiding this comment.
Non-blocking: removing the isLoading guard means the effect now runs on mount with isLoggedIn === false until boot resolves. Every hard load therefore expires dl_v (max-age=0) and writes it again later. If boot fails or the tab closes first, the next post open serves the plain page and flips. The deleted spec "writes nothing before the flag resolves" protected against this. Gating on isAuthReady (return early until it is true) keeps that behaviour without the flag.
Reviewed by AI.
| */ | ||
| export const featureSponsorStrip = new Feature('sponsor_strip', isDevelopment); | ||
|
|
||
| export const featureLayoutV2 = new Feature('layout_v2_2', false); |
There was a problem hiding this comment.
Rollout ordering: please don't archive layout_v2_2 when this deploys. Extension builds already published keep evaluating this flag until the store update reaches them. If the feature is archived, they fall back to the false default and return those users to the v1 sidebar and header. Keep the flag forced on at 100% until the extension release containing this PR has propagated, then archive it. The PR description should state this order.
Reviewed by AI.
| activePage?: string; | ||
| isNavItemsButton?: boolean; | ||
| /** | ||
| * No longer read: the laptop layout pads the content for the sidebar the |
There was a problem hiding this comment.
Cleanup: you asked for the control path to be removed completely, and this keeps a prop that is no longer read and is still set in about 60 layoutProps objects plus MainFeedPage (screenCentered={false}). Its only other input, useFeedLayout().screenCenteredOnMobileLayout, has also lost its last consumer. Removing all of it is mechanical (git grep -l screenCentered) and keeps a future reader from tuning a prop that does nothing. Non-blocking if you'd rather do it as a follow-up, but then please link the follow-up.
Reviewed by AI.
| // card border rules. No bottom margin from `tablet` up, where the grid | ||
| // already opens with that inset; mobile keeps one as the only separator. | ||
| const isV2Grid = isV2 && !shouldUseListFeedLayout; | ||
| const isV2Grid = isLaptopLayout && !shouldUseListFeedLayout; |
There was a problem hiding this comment.
Cleanup (non-blocking): v2 naming is still left now that there is only one laptop layout. In this file: isV2Grid, showExploreV2PageHeader, showFeedV2PageHeader, v2ActionButtons. In layout/common.tsx: hasV2Chips, shouldUseCompactV2Actions. Also isV2Grid in BookmarkFeedLayout, the isV2Panel prop on Section/SidebarSectionProps, SidebarDesktopV2 itself (now the only desktop sidebar), and the html.layout-v2 root class. Comments that still describe a v1/v2 split include BookmarkFeedLayout ("v2 hoists…", "inside the v2 floating card, so v2 is unchanged"), ToggleClickbaitShield ("v2 dual-sidebar laptop"), Section.tsx / sections/common.tsx ("so the v1 sidebar is unchanged"), ReadTopLeaderboard ("4rem for the v1 header") and SidebarDesktopV2 (around line 1224, "CustomFeedSection is v1's sidebar only"). The renames could go in a separate PR so this one stays reviewable, but they belong in the graduation.
Reviewed by AI.
…ntrol sweep `useLaptopLayout` now holds the server's shell only across hydration (`useIsHydrated`) and reads the viewport synchronously everywhere else, so a client-side navigation and the extension's new tab get the laptop chrome on their first render. The previous `useMediaClient` version answered false on the first render of every mount, which re-introduced the tablet-then-laptop flip the flag removal was meant to end and logged impressions for a CV banner and a hijacking strip nobody saw. The cookie hook waits for boot again, so a hard load no longer expires `dl_v` before writing it back. Control-only remnants removed: the install-extension CTA in the header strip (rendered only on laptop under the header layout), the laptop breadcrumbs on the leaderboard pages, the explore header's breadcrumbs and laptop branch, the heading wrapper and `feedHeading` memo in `FeedContainer`, `Float` button variants that only applied on laptop without the rail, dead `laptop:` classes inside tablet-only branches, `CustomFeedSection`, `SidebarMenuIcon`, `SidebarProfileCompletion`, the `!isV2Panel` branch in `NetworkSection`, the `screenCentered` layout prop and `useFeedLayout().screenCenteredOnMobileLayout`. The four pages whose only change is the dropped prop carry pre-existing strict errors and join the strict skip list. The storybook sources shell draws the collapsed rail for signed-out laptops instead of the v1 sidebar. The rail panel cases from the deleted v1 sidebar spec that still apply (Happening Now link, Explore hub pages) move to the rail spec; the anonymous Following login case has no rail equivalent, since signed-out visitors get the rail without panels. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Addressed in 65b7e12. Blocking
Cookie: Rollout: the PR description now says to keep Control leftovers: Coverage: the rail spec now covers the Happening Now link and the Explore hub pages. The anonymous "login before Following" case has no rail equivalent, since signed-out visitors get the rail without panels. Naming: the Screenshots / recording: not attached; the description states what the specs pin down instead. |
…ore/layout-v2-cleanup
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Second pass over the full main...head diff (now 127 files, +705/−2129), not only the new commit. I traced every removed !isV2 branch against main, swept the repo for control/v1 remnants, and ran the related webapp suites locally.
What the new commit fixed (those threads can be resolved)
useLaptopLayoutnow usesuseIsHydrated+useViewSize. Client navigation and the extension get the right value on the first render, so the CV-banner and hijacking-strip phantom impressions are gone.- The cookie waits for
isAuthReadyagain. - The rollout order for
layout_v2_2is now in the PR body. screenCenteredandscreenCenteredOnMobileLayout,CustomFeedSection,SidebarMenuIconandSidebarProfileCompletionare fully removed.- The storybook sources shell no longer draws v1.
Branch audit: no tablet, phone or extension regression found. Every deleted branch I traced was reachable only at laptop width on the control arm, or on the single hydration render. That covers:
- the install-extension CTA, the
Floatvariants and the feed-heading wrapper - the explore,
usersandusers/[id]breadcrumbs - the
sources/index, briefing, squad-directory andAccountPageContainerclasses - the
interests/shellheights,DiscoverSectionandSidebar.tsx
On hard loads, loadedSettings and the cached user arrive via the BootProvider effect, after the useIsHydrated re-render. So the CV banner and reading reminder can't mount in the tablet slot first. A future synchronous boot-cache read would reintroduce the double impression, so a spec pinning that order would be cheap.
Blocking
- CI is red.
test_webappfails in 9 cases (PostPage.tsx×5,ExplorePage.tsx×4). They pass onmain. The cause is a router-mock gap that the laptop path now exposes, not a product bug; details and the fix are in theMainLayout.tsxthread. The PR body says the webapp suites are green, so please re-run on the final head.
Should fix before merge
2. styles/utilities.css (~lines 53-68), "Sidebar overlay mode on profile pages below 1360px". This hack from profile 4.0 was written for the old sidebar. Its nav[role='navigation'] selector matches nothing now. The second rule, body:has(.profile-page) main { padding-left: 0 !important; margin-left: 20px }, still applies from 1020 to 1359px. It overrides MainLayout's laptop:pl-20 rail padding on every profile page (ProfileLayout adds .profile-page), so profile content very likely sits under the fixed 80px rail. Please check /<username> at 1024–1359 on the preview and delete or replace the rule. It is not in the diff, so I can't comment on the line.
3. Dead control code still in the head: see the inline threads for sidebar/sections/MainSection.tsx (whole file plus spec), the ExploreChipsBar compact prop, and NetworkSection asPin.
Verification still missing
- Playwright. Login success is detected via a
/profile/ilink or the "Profile settings" button (playwright/tests/helpers.ts~61,login.spec.ts~24). At 1280px, logged-in users now always get the rail, where the avatar tab isaria-label="You"andProfileButtonisn't rendered once the sidebar owns the header. e2e only runs onmaindeploys, so please dispatche2e-tests.ymlagainst the preview before merging. - Hard-load layout shift on non-post routes. A logged-in laptop session hard-loading
/tags,/sources,/users, squads, settings or a profile gets SSR HTML withMainLayoutHeaderandlaptop:pt-16, which the rail then replaces after auth. The v2 arm already had this; now every laptop session does. "Nothing shifts because a flag resolved after paint" is true for the flag but not for this swap. Please add CLS numbers for one hard-loaded non-feed page, logged in on laptop. Feed pages are fine, since they hold until paint-ready. - Screenshots for anonymous and logged-in laptop at 1024 and 1440, plus an anonymous click-through of Tags, Sources, Leaderboard and Discussions (the rail without panels is now everyone's logged-out navigation).
Coverage (non-blocking)
- The laptop branch is now the main production path, but
AccountPageContainer.spec,AppearanceSettingsPage.spec,SettingsFeedbackPage.specand__tests__/MainLayout.tsxall forceuseLaptopLayouttofalse. Please add one laptop case for the settings container and one for MainLayout. - The
requiresLogin→showLogingate inSidebarItem.tsx(~99) lost its only test withSidebar.spec.tsx. It is still live through the tablet bookmarks section, so aSidebarItem.speccase would cover it. - Nothing asserts that the extension
MainFeedPagenever mounts the hijacking strip on the first render at laptop. The two deleted cases pinned that.
Product (already listed in the PR body, flagging the side effects)
- Former control users will see the sidebar tour auto-start (it is built for "existing users who were moved"). Expect a wave of tour starts on deploy.
SidebarDensitySectionin Appearance now shows for every laptop user.- The install-extension CTA in the header strip existed only for the control arm, so this removes a growth surface. That is intended, but worth stating for whoever owns extension installs.
Cleanup follow-up (noted in the PR body)
About 90 layout v1/v2 comment lines remain in the touched areas, beyond the identifiers already listed. Some examples:
BookmarkSection.tsx:101,CustomFeedEmptyScreen.tsx:43,FeedContainer.tsx~315-343MainFeedLayout.tsx~797-837,header/FeedExploreTabs.tsx:64,RouteProgressBar.tsx~12sidebar/common.tsx,NotificationsBell.tsx~33-72,ExploreChipsBar.tsx~24/125- the extension's
CompanionPopupButton.tsx:59 v2FeedSideInsetClassandv2FeedInsetClassinFeedContainer
Please include these in the rename PR.
Reviewed by AI.
| const { isEnabled: isLfgEnabled } = useLfgEnabled(); | ||
| // The v2 rail has its own Looking for group tab. | ||
| const showLfg = isLfgEnabled && !isV2; | ||
| const showLfg = isLfgEnabled && !isLaptopLayout; |
There was a problem hiding this comment.
This file is now dead, so please delete it rather than edit it. On main its only importer was the deleted SidebarDesktop. ProfileMenu imports a different component (ProfileMenu/sections/MainSection), and nothing else imports sidebar/sections/MainSection except MainSection.spec.tsx.
That makes every !isLaptopLayout branch edited here (Game Center, LFG, Marketplace and their alert dots, the agent dot, and the avatar "For You" icon) leftover control-arm code. The spec's "game center link" cases only exercise those branches. Once the file is gone, check whether ActionType.InterestAgentSidebarClick and ActionType.MarketplaceSidebarClick still have a client caller.
Reviewed by AI.
| const { rest: shellBlockRest } = useShellBlockPlan(); | ||
| useLayoutVariantCookie(); | ||
| useRecordRecentPages(isV2); | ||
| useRecordRecentPages(isLaptopLayout); |
There was a problem hiding this comment.
Two things follow from this gate now being the viewport alone.
- It is why
test_webappis red.PostPage.tsx(5 cases) andExplorePage.tsx(4 cases) pass onmainand fail here. At laptop width,useRecordRecentPagesnow runscapture(router.asPath), which hitsurl.splitonundefined.RouteProgressBarnow mounts inside the floating card and callsrouter.events.onwith no guard. Both suites' router mocks lackasPath(and ExplorePage's lacksevents). AddingasPathto the PostPagemockRouter, plusasPathandevents: { on, off, emit }to ExplorePage'sbeforeEachmock, makes both suites pass (70/70), including the laptop-only assertions (signup banner, pinned card, organic ad units, sponsor dock, hero suppression). Optionally, guardrouter?.eventsinRouteProgressBarthe wayuseRecordRecentPagesalready does. - Recording now covers every laptop visitor, anonymous and pre-auth included. On
mainit neededisAuthReadyOrCachedplus the flag. The store is a device-wide localStorage key (dailydev:recentPages), so pages browsed while logged out appear in the next logged-in user's "Recent" on that device. ConsideruseRecordRecentPages(isLaptopLayout && isLoggedIn).
Reviewed by AI.
| const shouldUseCompactV2Actions = isV2Strip && (!!chips || !isTablet); | ||
| const dropdownIconSize = isLaptopLayout ? IconSize.XSmall : IconSize.Medium; | ||
| const hasV2Chips = isLaptopLayout && !!chips; | ||
| const shouldUseCompactV2Actions = isLaptopLayout && (!!chips || !isTablet); |
There was a problem hiding this comment.
Nit: ViewSize.Tablet is a min-width query, so isTablet is always true wherever isLaptopLayout is true after hydration, and !isTablet is dead here. This simplifies to isLaptopLayout && !!chips. Separately, the isMobile || in buttonSize above can't be reached because of the early if (isMobile) return null (that one was already true on main).
Reviewed by AI.
| categories={exploreCategories} | ||
| isPending={!feeds} | ||
| compact={isV2} | ||
| compact={isLaptopLayout} |
There was a problem hiding this comment.
Leftover control styling: chips are only built when !!user && isLaptop && isChipStripPage. The user is never known on the hydration render, and after hydration isLaptop implies isLaptopLayout, so compact is always true whenever this renders. The non-compact h-10/rounded-12 branches in ExploreChipsBar (around lines 106, 131, 189 and 200) were control-only. So was the chips slot in the tablet return of SearchControlHeader. Please drop the prop and those branches.
Reviewed by AI.
| // this shared section keeps the v1 star when the flag is absent. | ||
| // Squad "favorite" is framed as "pin" in the rail panel; the star stays for | ||
| // other surfaces of `SquadFavoriteButton`. | ||
| asPin = false, |
There was a problem hiding this comment.
asPin is now always true: the only renderer is SidebarDesktopV2 (around line 1728), which passes it. The false default, and the SquadFavoriteButton branch it selects in createSquadMenuItem (squadMenuItem.tsx), were the v1 star. The new comment ("the star stays for other surfaces") is misleading, because those surfaces use SquadFavoriteButton directly, not this path. Please drop the param and that branch.
Reviewed by AI.
|
|
||
| // Below laptop the header gives way to SidebarTablet, so the shell shows | ||
| // through the floating card rather than the banner. | ||
| it('drops the painted shell once the viewport turns out narrower', () => { |
There was a problem hiding this comment.
These cases no longer test what the suite describes ("reproduces a server render"). render uses createRoot, so useIsHydrated is true from the first render, and every case is decided by the mocked useViewSize. layoutVariant doesn't affect the outcome. The mirror's real contract is: server markup equals the hydration render, then a flip to tablet chrome on a narrow window. That is now pinned only by useLaptopLayout.spec mocking useIsHydrated. A renderToString + hydrateRoot case for the mirror (laptop and tablet width), asserting no hydration mismatch plus the value after hydration, would cover the only place this hook can still be wrong.
Reviewed by AI.
| 'packages/extension/src/newtab/App.tsx', | ||
| // Layout v2 graduation: touched only to drop the dead `screenCentered` | ||
| // layout prop; the strict violations are pre-existing and unrelated. | ||
| 'packages/webapp/components/layouts/OrganizationLayout/index.tsx', |
There was a problem hiding this comment.
Two notes on this file:
- Four files go permanently onto the strict skip list because one
screenCentered: trueline was deleted from each. The list only ever grows. Either fix their strict errors or open a tracked follow-up. - The Prettier reflow further down (
resolveBaseRef,spawnSync) is unrelated to the layout change. Please revert it so this file only carries the skip-list entries.
Reviewed by AI.
…ntrol code The router mocks in the PostPage and ExplorePage suites predate the laptop path: it reads `router.asPath` to record recent pages and subscribes to `router.events` for the route progress bar, and both suites lacked them. The progress bar now also tolerates a router without an emitter. Recent pages are recorded for signed-in laptop sessions only; the store is device-wide, so anonymous browsing used to seed the next account's list. Control-only code that the second review pass found: the sidebar's `MainSection` (its only importer was the deleted v1 sidebar; the profile menu has its own), `ExploreChipsBar`'s pill rendering (`compact` was always true where the strip renders), the chips slot on the tablet header strip and the tablet empty screen, `createSquadMenuItem`'s favorite-star branch (the rail pins), and the profile-page sidebar overlay rule in `utilities.css` that targeted the v1 sidebar and still stripped the rail padding from profile pages between 1020 and 1359px. The strict-typecheck script carries only the four skip-list entries now. Coverage: `useLaptopLayout.spec` renders the mirrored shell to a string and hydrates it, asserting no mismatch and the viewport taking over afterwards; `SidebarItem.spec` covers the login gate the v1 sidebar spec used to; the settings container and MainLayout each gain a laptop case. The Playwright login helpers also accept the rail's "You" tab, which is where the account entry lives for every signed-in laptop session now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The banner pins to the window's left edge with 40px of padding, which the 44px v1 sidebar barely covered. Under the 80px rail the headline, the email link and the Google button ran under the rail on every signed-out laptop page below 1668px, where the banner is left-aligned. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The helper clicked "Log in" as soon as the password was typed, so on a cold preview the form rejected the submit with "Please complete the security check" and every retry failed the same way. Turnstile writes its token into a hidden field in the form; wait for that field to fill first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Second pass addressed in 65b7e12…4e969fa50 plus the e2e helper commit. Blocking: Should fix: the Script: only the four skip-list lines remain; the reflow is reverted. Tracked follow-up for their strict errors: ENG-2243. Coverage: Playwright: helpers and Screenshots: taken at 1024 and 1440 for the anonymous Explore hub pages. They surfaced one real defect: the bottom signup banner ran under the rail at 1024, fixed in 4e969fa and re-verified on the preview. I can't attach images from this session; they're with the author. Product side effects (tour wave, density section, install CTA) and the hard-load header swap on non-post routes are now stated in the PR body. |
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Final pass on head e98d919. I checked the delta since my last review (5004b87…e98d919: 3 commits, 23 files) line by line, and re-checked the full main...head diff (138 files, +881/−2811) for anything the earlier rounds missed.
Everything raised in rounds 1 and 2 is resolved:
- CircleCI is fully green on the head: typecheck, lint, the shared, webapp and extension tests, and the extension build.
- The PostPage and ExplorePage mocks are fixed, and
RouteProgressBarnow guardsrouter?.events. - The sidebar
MainSectionand its spec are deleted.ExploreChipsBarlostcompactand the pill branch, thechipsslot is gone from the tablet header andCustomFeedEmptyScreen, andcreateSquadMenuItemlostasPinand the star branch. - The profile-page overlay rule in
utilities.cssis deleted. - Recent pages are recorded only for signed-in users.
- The script change is down to the four skip-list entries, with ENG-2243 linked.
- New tests:
- a real
renderToString+hydrateRootmirror test with a check for no mismatch - the
SidebarItemlogin gate - laptop cases for
AccountPageContainerandMainLayout
- a real
- The PR body now states the flag-archive order, the product side effects, the remaining header swap on hard load, and the rename follow-up.
I found no remaining control or v1 code paths. What is left is naming and comments, and those are tracked for the rename PR.
One new issue, in the banner fix (inline): laptop:left-20 is sized for the 80px rail, but logged-out visitors, who are the only audience of this banner, get the compact 64px rail. That leaves a 16px gap.
Non-blocking
- e2e is still unverified for this branch, and the PR body is out of date on it. Dispatched run 39933000566 did reach Playwright. The new Turnstile wait passed, then the preview rejected the test account's credentials ("doesn't match our records"), so the "You"-tab assertion never ran. That looks like the test account not existing on the backend the preview talks to, rather than anything in this PR. On
main, login passes, but the profile-social-links spec fails later on its "Add link" field, which predates this branch. The body still says the re-dispatches never reached Playwright. Please correct that and note that laptop login detection is untested end-to-end. utilities.cssstill has two unrelated Prettier reflows, around lines 419 and 1059. In the second one, the/* The top edge catching the light. */comment moved onto the same line as the shadow value. Both are harmless, but they are churn in a file this PR otherwise only deletes from.- Scope: the Playwright Turnstile wait (
e98d919) fixes test infrastructure that already exists onmain. It is fine to keep here, but it is not part of the layout change.
Still not measured (stated in the PR body): CLS on a logged-in hard load of a non-post route, and screenshots attached to the PR. Both rest on the author's description, not on evidence a reviewer can check.
Verdict: there are no blocking issues apart from the banner offset. Because I'm reviewing on the author's account, the approval has to come from another maintainer. I'd recommend approval once the banner offset is fixed and the e2e note in the body is corrected.
Reviewed by AI.
| 'min-h-[17.75rem] items-center border-t border-accent-cabbage-default px-10 py-4 pe-[388px] shadow-3 laptopXL:pe-10', | ||
| // The banner pins to the window, but on laptop the rail sits fixed | ||
| // along the left edge; start beside it or the headline runs under it. | ||
| 'laptop:left-20 laptop:w-[calc(100%-5rem)]', |
There was a problem hiding this comment.
This offset assumes an 80px rail, but this banner's audience sees a 64px one.
PublicPageSignupBanner shows only to logged-out laptop visitors. Their rail width comes from useSidebarCompact(), which is useSettingsBooleanFlag('sidebarCompact', true). A logged-out visitor has no stored flag, so it resolves to compact, and the rail is laptop:w-16 (SidebarDesktopV2.tsx:814). MainLayout pads the content laptop:pl-16 to match.
With left-20 hardcoded, every visitor this banner targets gets a 16px strip of page between the rail and the banner. Please read the same value MainLayout uses, so the two can't drift:
const { value: isSidebarCompact } = useSidebarCompact();
// …
isSidebarCompact
? 'laptop:left-16 laptop:w-[calc(100%-4rem)]'
: 'laptop:left-20 laptop:w-[calc(100%-5rem)]',If logged-out visitors can never be non-compact, laptop:left-16 laptop:w-[calc(100%-4rem)] alone is enough. Either way, the 1024 screenshot should show the banner flush with the rail.
Reviewed by AI.
Logged-out visitors, the banner's only audience, have no stored sidebar density and so get the compact 64px rail; a fixed 80px offset left a 16px strip of page between the two. Read the same flag MainLayout pads with. Also drops the unrelated Prettier reflows in utilities.css so the file only loses the overlay rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Final-pass item fixed in 48dc835: the banner offset now reads |
layout_v2 graduated in #7017 and useLayoutVariant is gone, so the laptop chrome is now gated on useLaptopLayout() like every other page. The remaining non-laptop branch is the tablet chrome, so its laptop: variants were dead and are dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…over strip The header's Log in / Sign up opened the in-extension auth modal, where social OAuth from the extension origin is rejected with a 403; the cover strip on the same screen already hands off to the webapp onboarding URL. Both now share one onboardingHref module. Drop the ExtensionSignInStrip wrapper: App.tsx routes every logged-out or onboarding-incomplete user to HijackingPage, which always passes loginStrip, so the fallback could only ever render null. MainFeedPage gates the top-banner strip on laptop itself instead of leaning on MainLayout hiding the slot. Drop feedStyles.cards from the hijacking strip: its desktopL cap reads --num-cards, which is only set inside the feed, and below laptop the breakpoint never applies, so the class was inert in both placements. Add a MainFeedPage spec for the laptop/below-laptop strip and header wiring, the path #7017 dropped without any test noticing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Since #7017 graduated layout v2, an anonymous laptop visitor got the v2 rail and the legacy global header at the same time: two logos, two searches, the rail toggle buried under the header, no floating card, and a Streak tab and an Invite button that both need an account. The rail now owns the header for every laptop session, signed in or not. - MainLayout: sidebarOwnsHeader = isLaptopLayout && showSidebar. The login audience and the cookie/server-shell guesswork that only existed to predict it are gone; useLaptopLayout already answers for the server. Anonymous laptop sessions in the webapp get SignInStrip as the card's top banner, where the extension already mounts it through topBanner. - ExtensionSignInStrip moves to shared as auth/SignInStrip. Its Log in / Sign up keep target_id 'header' so the login funnel dashboards hold; it carries Get the app from laptopL. - SidebarDesktopV2: a pinned Log in tab (UserIcon) in the avatar's slot for visitors, Get the app (the QR popover, icon-only, opening right) instead of Invite, the Streak tab gated on login, and the overflow budget reserving the account row for everyone. - useLayoutVariantCookie records "laptop" rather than "logged in on a laptop", written once hydrated, so the mirrored post page serves the rail to returning visitors too. hasLayoutVariantCookie is unused and removed. - Tests: the anonymous laptop case (no banner, floating card, strip), the hydration settle, and the visitor rail (Log in tab, no Streak, Invite or New post, Get the app present). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Why
The dual-sidebar layout won its experiment. This removes the
layout_v2_2flag and everything that existed only to wait for it or to serve the control arm, so nothing in the layout shifts because a flag resolved after paint.What changes
useLaptopLayoutreplacesuseLayoutVariant().isV2. It is the laptop breakpoint. Across hydration it answers with the shell the server painted (the rail shell on the mirrored/layout-v2post page, the header shell everywhere else), so server HTML and the hydration render always agree. Every later mount, including client-side navigation and the extension's new tab, reads the viewport synchronously and is right on its first render.isLoadinggate is gone: the sidebar hold inMainLayout, the top-hero hold inFeed, the CV-banner hold inFeedContainer, and the extension'sHijackingLoginStripwrapper. The content padding transition enables one commit after the settle on every page, so the first paint never slides.SidebarDesktop(v1) and its spec, the sidebar'sMainSection,CustomFeedSection,SidebarMenuIcon,SidebarProfileCompletion, the v1 expanded-sidebar padding and thescreenCenteredlayout prop (plususeFeedLayout().screenCenteredOnMobileLayout), the install-extension CTA in the header strip, the laptop breadcrumbs on the leaderboard pages and in the explore header,ExploreChipsBar's pill rendering, the chips slot on the tablet header strip and empty screen, the favorite-star branch of the squad row, the!isV2Panelbranch inNetworkSection,Floatbutton variants that only applied on laptop without the rail, deadlaptop:classes inside tablet-only branches, the profile-page sidebar overlay rule inutilities.css, and the storybook history page comparing the two navigations. The storybook sources shell now draws the collapsed rail for signed-out laptops./layout-v2post-page mirror anddl_vcookie stay. Prerendered pages are cached per path, so a logged-in laptop session can only get the rail and the floating card in its initial HTML from a path that carries the shell. The cookie now records "logged in on a laptop" and waits for boot before writing.isLaptopLayoutbranches pick between the laptop chrome and the tablet chrome. Tablet and phone are unchanged.Rollout order
Do not archive
layout_v2_2when this deploys. Published extension builds keep evaluating the flag until the store update carrying this PR reaches them, and an archived feature falls back tofalse, which would return those users to the v1 sidebar. Keep the flag forced on at 100% until that extension release has propagated, then archive it.Product changes to confirm
sidebarOwnsHeaderstill requires a user.useSidebarTourState, built for "existing users who were moved", so expect a wave of tour starts on deploy), sees the Agents entry in Discover (showAgentis evaluated for every logged-in user), and getsSidebarDensitySectionin Appearance.Known behaviour, unchanged by this PR
A logged-in laptop session hard-loading a non-post, non-feed route (
/tags,/sources,/users, squads, settings, a profile) gets SSR HTML with the header, which the rail replaces once the cached session is read on the client. The v2 arm already had this; the post page is the only mirrored route because every mirrored route doubles its ISR entries. Feed pages hold until paint-ready and are not affected. Extending the mirror to more routes is a separate decision.Follow-ups
screenCenteredprop.v2identifiers (SidebarDesktopV2,isV2Panel,isV2Grid,showExploreV2PageHeader,showFeedV2PageHeader,hasV2Chips,v2FeedSideInsetClass,v2FeedInsetClass, thehtml.layout-v2root class) and the ~90 remaining v1/v2 comment lines (BookmarkSection,CustomFeedEmptyScreen,FeedContainer,MainFeedLayout,FeedExploreTabs,sidebar/common.tsx,NotificationsBell,ExploreChipsBar, the extension'sCompanionPopupButton).Verification
node ./scripts/typecheck-strict-changed.jspasses; fulltscfor webapp, extension and playwright passes.useLaptopLayout.spec(first render of a client mount reads the viewport; the mirrored shell renders to a string and hydrates without a mismatch, then the viewport takes over),SidebarItem.spec(login gate),AccountPageContainer.specandMainLayout.tsx(laptop cases),SidebarDesktopV2.spec(Happening Now, Explore hub pages).mainruns today fail later in the same spec. Laptop login detection is therefore untested end-to-end on this branch; the "You" tab is pinned only by the rail's DOM (role="tab",aria-label="You")./posts,/tags,/sources,/userson the preview were taken and reviewed (rail under the header, page-header strip, banner beside the rail after the fix). They could not be attached from this session.🤖 Generated with Claude Code
Preview domain
https://chore-layout-v2-cleanup.preview.app.daily.dev