feat(sidebar): v2 sidebar tour and ambient coaches behind sidebar_tour - #6429
feat(sidebar): v2 sidebar tour and ambient coaches behind sidebar_tour#6429tsahimatsliah wants to merge 34 commits into
Conversation
Three-step spotlight tour for users who already learned the old sidebar, plus the pin and ••• coaches for everyone who did not. Steps stay data, targets resolve from the live rail, and the seen flags are device-local until daily-api grows an ActionType for them.
Renders the overlay and both coaches once from SidebarDesktopV2, adds the Learn the sidebar replay row to the support popup, holds the real Streak panel open for step three, and lifts the rail over the spotlight scrim. With the flag off the rail is unchanged, which the spec asserts.
The shortcut store fills in from storage after the first render, so a plain count increase is not a pin. Require an open panel or a drag that just ran.
The pin coach no longer reads storage hydration as a pin: the shortcuts hook now reports isFetched and the first reading after that is a baseline. The tour cannot auto-restart if the seen-flag write fails, and the dock step force-paints the ••• button, which is hover-only on an empty dock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ••• coach fired its retire path even when it was inactive, so a control user on a flag-off build wrote to IndexedDB and pre-retired a lesson they had never been shown. The guard is hoisted so an inactive coach does nothing at all. "Got it" now retires the coach: an explicit dismissal is the strongest comprehension signal available, and closing without retiring meant the next ••• hover re-opened the card and burned another exposure. An outside click still only closes, since brushing past is not comprehension and the exposure was already counted. That outside click is new too: the card previously had no exit besides "Got it", opening the tray or the coach dropping, so a pointer that wandered off pinned it for the rest of the session. The pin coach charged an exposure the instant the card became visible, which meant a panel flashing past under the pointer cost a third of the budget; it now dwells 700ms first. Its drag attribution came from a boolean that was never cleared unless a success fired, so dragging a shortcut out to remove it and hitting Undo minutes later logged a false drag success. That is a timestamp now, valid for one second. The dock's ••• button also reported a 'hover' from onFocus. Browsers focus a button on mousedown, so every click was a 'hover' immediately followed by an 'open': a visible flash of the card and an inflated view event. The handler is gone; the coaches are pointer-directed teaching, and a keyboard-focus-opened card whose dismiss button sits at the end of the document is worse than no card. The dock and the More menu both hardcoded the rail popup group string instead of importing RAIL_POPUP_GROUP, with a comment pointing at the wrong file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coach card never measured its own height. The layout effect that set it is keyed on the card's content, and the card only mounts on the render AFTER the anchor resolves, so the effect never fired again for that mount: the card hung below its target with the pointer parked at mid-card aiming at nothing. A callback ref measures on mount instead. The anchor's geometry went stale on every layout change that was not a window resize. Toggling Compact mode on step one, the panel opening or closing, and the rail hover-expanding all move the rail, the panel and the target, and the two timed measures have long since run by then. A ResizeObserver on all three keeps it current, and the first measure moved to a layout effect so a step change never paints a frame at the previous step's position. measureLeft deliberately still clears whichever panel is open: hugging the rail with a pinned panel would park the card on top of panel content. The rail stays clickable above the scrim, which is the point of a spotlight, but that meant a tab or shortcut click navigated while the scrim and card rode along to the new page around a stale ring. The tour now ends on routeChangeStart. Relatedly, the scrim rendered on isRunning alone while the card needed a resolved anchor, so a target that vanished mid-step left a full-screen scrim whose only exit was Escape; a step that cannot resolve for one settle window is now dropped. Radix portals every popper at z-index 1000 !important, and the rail tooltips open into exactly the gap the card occupies (the ••• tooltip opens side=right straight into the dock step's card). Suppressing them would have meant threading a tour flag through nine Tooltip call sites across two files, several of them module-level helpers, so the card and its highlight take z-[1001] instead and win the tie outright. Also: Escape now ignores an event a modal already consumed, the compact switch no longer leaves an unhandled rejection when the settings mutation fails, losing eligibility mid-tour clears the parked run without claiming the user saw it, and a tour step that forces the Game Center panel open is no longer masked by a hovered or pinned create panel. For a11y the card carries aria-live on the shell that survives step changes (a live region that remounts announces nothing), the tour card alone becomes a labelled dialog, and a fresh run lands focus on the primary action so Next and Skip are reachable without a pointer marathon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ReviewFull-diff review done (every product file read end to end, storybook checked as design reference only), on top of a three-lens agent review (state/logic, integration/regression, UX/DS/a11y/copy) whose 17 confirmed findings are already fixed in the last two commits. Summary of what was verified and what a human reviewer should still weigh in on: Verified
Deliberate calls a reviewer may want to challenge
Known gap: not yet exercised in a live build (flag defaults off; previews build as production, so a review there needs a temporary default flip that must not merge). |
The twelve exploration concepts, their catalog README and the mock rail they shared were scaffolding for choosing a direction. That choice is made and built, so they are noise in a PR that ships the thing: dropped here, recoverable from 9a43994. The surviving final/ README had drifted from what actually shipped. Its step two still carried the "drag any page" copy that was corrected to "drag anything from the sidebar" (the rows are sidebar options, not pages), and its productionization map, seen-flag plan and log event names were all written before the implementation existed and disagreed with it. Those are now a short account of where the code lives and the three places the product deliberately resolved differently from the mock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tsahimatsliah
left a comment
tsahimatsliah
left a comment
There was a problem hiding this comment.
Review
Read the whole module plus the rail wiring, and traced the tour against the rest of the app rather than only against itself. The engineering inside the feature is genuinely careful — the hydration-vs-pin baseline, the drag attribution window, the callback-ref height measure, the ResizeObserver on all three moving boxes, the layout-effect first measure, reduced-motion on the new keyframes, and the flag-off parity assertion are all the right calls, and the commit messages explain why rather than what. RAIL_POPUP_GROUP finally having one home is a nice incidental cleanup. CI is fully green including typecheck_strict_changed.
What I found is mostly at the seams: what happens when something else on the page is also trying to be modal, what the end-of-tour paths write, and where the design reference lives.
Blocking
1. Nothing stops the tour auto-starting on top of an open modal, and the card is built to win that fight. See the inline notes on the auto-start timer and z-[1001]. zIndex in tailwind.config.ts tops out at max: 1000, with modal: 100 and sidebarOverlay: 77. So with a modal open when the 800ms timer fires: the scrim paints under the modal, the card and its highlight ring paint above everything in the scale, and the rail they point at only lifts to z-tooltip (80) — behind the modal. The user gets a floating sentence about a rail they cannot see, and the modal's own Escape handling and the tour's fight over the same key. LazyModal state is already in scope in SidebarDesktopV2 (modal?.type === LazyModal.SmartComposer a few hundred lines down); gating the auto-start on it, and parking a running tour when a modal opens, closes this.
2. Three endings the user never chose write the seen flag and log user intent. routeChangeStart → skip, another rail popup taking the group → skip, and a lost target on the last step → next() → CompleteSidebarTour. Two consequences, both bad: a stray tab click during step one retires the tour permanently on that device (the flags are device-local, so "permanently" is real), and skip sidebar tour / complete sidebar tour — the two metrics the ramp decision will read — are contaminated by runs the user never ended. Please separate the reasons: end without writing seen when the cause is navigation or an interrupting popup, and don't log a completion for a step that was dropped. A reason in extra would make all of it legible.
3. packages/storybook/stories/sidebar-tutorial/final/ doesn't belong in this PR. packages/storybook/AGENTS.md is explicit on both counts: stories are grouped atoms/ components/ tokens/ experiments/ extension/, and "skip one-off page components". This adds a new top-level group (also absent from preview.tsx's storySort order, so it sorts outside it) whose centrepiece is finalRail.tsx — 615 lines re-implementing the rail as a mock. That is ~1.9k of the diff, it duplicates a component this same PR is modifying, and it will drift the first time the real rail changes with nothing to catch it. The README already says the folder is recoverable from 9a439940d, which is the right answer for design history. Drop it, or move it under experiments/ if the team wants it browsable.
Non-blocking
Six inline notes: the pin coach's success without exposure, the ••• coach charging an exposure on a bare hover, the hardcoded rollout date, the arbitrary z-index, raw pixel inline styles, and the anchor's redundant state churn.
One more that doesn't attach to a line: enrollment happens for every signed-in v2 user. shouldEvaluate is isV2 && isAuthReady && !!user, so getFeatureValue — and the exposure with it — fires for people who have already seen the tour and retired both coaches, who then sit in the treatment arm with nothing left to be treated by. Gating on the persisted flags would tighten the arm, but it also means a user stops being counted after they learn, which may be exactly what you don't want for a retention read. Worth a deliberate call rather than an accident; a line in the PR body either way.
Verification
- Read root and
packages/storybookAGENTS.md, plus the tailwind design-system guide - Read every new file in
features/sidebarTour/and the full rail/dock/support diff - Traced z-index against the token scale for scrim, rail lift, card and modals
- Traced all four end paths (finish, skip, navigation, lost target) to what they write and log
- Checked
useConditionalFeatureenrollment semantics anduseInteractivePopupgroup behaviour - Confirmed
SidebarDesktopV2is laptop-only, so no mobile scrim path exists - Confirmed control parity: every new hook is inert with the flag off, and the dock's props default to no-ops
- CI green (
test_shared,test_webapp,test_extension,lint_shared,typecheck_strict_changed) - Not verified live — the PR notes the flag needs flipping in GrowthBook, and I'd want blockers 1 and 2 settled before that pass
Verdict
Comment, with three blockers. Two are small and mechanical (a modal guard, and splitting the end reasons); the third is deleting or relocating a folder. The feature itself I'm happy with.
Reviewed by AI.
A navigation, another rail popup taking the group and a modal opening all ended the tour through skip(), and a lost target on the last step ended it through next() as a completion. Both retired the tour permanently on that device and contaminated the two events the ramp decision reads. They now go through interrupt(reason), which clears the run without writing the seen flag and logs `end sidebar tour`, and dropStep(), which advances when steps remain and otherwise ends with reason `target_lost`. The tour also refuses to auto-start while a modal is open, and steps aside when one opens over a running tour. The v2 rollout cutoff keeps its constant, with a note that it stands in for the layout ramp and belongs in the flag payload if that ramp goes gradual. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An active coach only means its exposure budget is unspent, so a pin made inside the dwell window, or one the user was always going to make, logged `sidebar pin coach success` and retired a card that never appeared. Success now needs a counted exposure behind it. The ••• coach charged its exposure on a bare hover, and the dock sits under the rail's scroll path, so a pointer travelling to Support could spend a third of the budget. It now waits out the same dwell the pin coach uses, sharing one constant, and still opens the moment the pointer arrives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
z-[1001] was an arbitrary value outside a closed semantic scale; it is now a named `coach` token sitting one above `max` for the documented reason. The skip and primary buttons drop their raw pixel inline styles for `!px-1.5` and `min-w-[5.5rem]`; the important modifier is what the trim needs, since the size's own px-2 is emitted after px-1.5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…etry update() built a fresh anchor on every call, and it is called immediately, next frame, after the settle timeout, on resize and on three ResizeObservers, so one step change repositioned the card at least three times with the same numbers while it was mid animation. Every trigger stays; only unchanged measurements now end in the hook. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stories are grouped atoms/components/tokens/experiments/extension, and the folder added a top-level group that preview.tsx's storySort does not list, so it sorted outside the intended sequence. Moved under experiments/ and retitled to `Experiments/Sidebar Tutorial/...`, with the README's story names and the log event list brought up to date. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keeps main's jobs feature gate and its removal of the giveback and daily homepage log events, alongside this branch's sidebar tour flag, tour log events, and the isFetched flag the coaches wait on.
Closing the giveback campaign deleted GivebackGiftDock, which defined RAIL_ANCHOR_ATTRIBUTE and put it on the rail. The tour's coach cards measure the rail through that attribute, so without it measureLeft found no rail and every card would sit against the viewport edge, under the rail it is supposed to clear. Move the constant to useCoachAnchor, which is now its only consumer, and set it on the rail nav again.
…change Two comments described how the code got here rather than the constraint it has to hold: one dated the flag-off parity note to "today", the other credited the Storybook mock for a ring the product paints itself.
Step two says "or add it from the ••• menu", and opening that menu took the rail's popup group away from the tour, which read the eviction as the user reaching past it and ended the run. Following the instruction killed the lesson giving it. The tour now leaves the popup group for that one step, so the tray opens beside the card instead of evicting it and the run continues to step three. Every other step keeps the old behaviour: opening Support or Settings mid-tour really is reaching past it. The eviction check is gated on wanting the group rather than on the tour running, so stepping onto the dock step releases it deliberately without reading as an eviction.
The card and the shortcuts tray are both fixed at the rail's right edge, and the tray is the wider of the two, so z-order alone can only decide which one hides the other. The card won, and covered the menu the step had just told the user to open. The card now measures around an open rail dropdown the way it already measures around the context panel, so both are readable: the tray, and the tour's progress and controls beside it. Closing the tray brings the card back to the rail. Hiding the card instead would leave a full-screen scrim with no card and no visible way out, which is the state dropStep exists to avoid.
The deploy for fd23661 failed on Vercel's side while every other check passed, so the preview is still serving the commit before the coach card learned to clear the shortcuts tray.
Both conflicts were additive: main appended the comment_first_action flag where sidebar_tour sits, and added a layoutVariant import into the block the tour's imports occupy. Kept both sides of each.
1 participant
Teaches the v2 sidebar, behind the
sidebar_tourflag (default off; control renders the rail exactly as it does on main).What it does
The single invariant to hold while reading: every hook, prop and branch added here is inert unless the flag is on, so the control arm renders the rail's unmodified markup and behaviour.
What needs review, and what is reference
The diff is 32 files, and about 40% of its lines need no line-by-line review.
Review this (roughly 1,600 lines):
packages/shared/src/features/sidebarTour/: the feature itself plus its tests.SidebarDesktopV2.tsx,SidebarShortcutsDock.tsx,RailMoreMenu.tsx,common.tsx: the rail integration. The dock's two new props are optional and gate all new DOM, so the dock is unchanged for every existing caller.featureManagement.ts,log.ts,utilities.css,tailwind.config.ts: the flag, the eight log events, the card animation, and az-coachtier one abovemax(Radix portals its poppers with an inlinez-index: 1000 !important).Reference, not shipping code (2,072 lines):
packages/storybook/stories/experiments/sidebar-tutorial/final/is the signed-off design record: four stories, a mock rail, and a README. Nothing inshared,webapporextensionimports it. The README is the part worth reading if you read one file here: it records the decision, the three places the product deliberately resolved differently from the mock, and the eight alternatives that were built and rejected, with reasons. The twelve exploration concepts it was chosen from are not in this PR.Two things worth a second opinion
The rollout cutoff.
SIDEBAR_V2_ROLLOUT_DATEinuseSidebarTourState.tsis a hardcoded2026-08-01, and it is what decides which of the two entirely different treatments a user gets. It stands in for "when this user actually got the v2 rail", which the layout rollout is what really decides. If that ramp ever runs gradually, the cutoff belongs in the GrowthBook flag payload so it can be tuned mid-ramp instead of waiting on a release.Device-local seen-flags. They ride
usePersistentContext(idb-keyval) rather thanSettingsFlags, because the API rejects flags it does not declare and one undeclared key fails the wholeupdateUserSettingsmutation. The cross-device home is auseActions/ActionTypefield once daily-api declares it, and the swap is isolated touseSidebarTourState.ts.Review hardening
The branch went through a three-lens review (state/logic, integration/regression, UX/DS/a11y/copy) plus an adversarial test pass; 17 confirmed findings were fixed, notably:
79 tests cover the change: 75 across the feature module (state machine, overlay wiring including flag-off rail parity, anchors, both coaches, step resolution) and 4 on the dock's new customize-button props. Key guards were proven by breaking them and watching the test fail.
Verification
typecheck-strict-changedand eslint cleanNot yet verified live in the product (needs the flag flipped in GrowthBook; previews build as production, so reviewing there needs a TEMP default flip).
Preview domain
https://claude-sidebar-tour.preview.app.daily.dev
Known: the experiment arm is wider than the treatable population
Enrollment is gated on
isV2 && isAuthReady && !!user, sogetFeatureValue, and the GrowthBook exposure with it, fires for every signed-in v2 user. That includes people who have already seen the tour and retired both ambient coaches, who then sit in the treatment arm with nothing left to treat them, diluting any per-exposure read.This is deliberate. Gating enrollment on the persisted seen-flags would tighten the arm, but it would also stop counting a user the moment they learn, and the retention question worth answering is whether people who were taught the rail behave differently weeks later. Dropping them from the arm as soon as the lesson lands is the wrong cut for that. If a per-exposure read is wanted later, the move is to segment on the
start sidebar tourandview sidebar pin coachevents rather than to narrowshouldEvaluate.🤖 Generated with Claude Code