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 20 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>
# Conflicts: # packages/shared/src/components/sidebar/SidebarDesktopV2.tsx
1 participant
Teaches the v2 sidebar, behind the
sidebar_tourflag (default off; control renders the rail exactly as today).What it does
Seen/exposure flags are device-local (
usePersistentContext) for now; the cross-device home is auseActions/ActionTypefield on daily-api, and the swap is isolated to one file (see the why-note inuseSidebarTourState.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:
68 tests cover the module (state machine, overlay wiring incl. flag-off rail parity, anchors, both coaches, dock 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).
packages/storybook/stories/sidebar-tutorial/final/carries the signed-off design reference: the four stories the decision was made from, plus a README recording the decision and the three places the product deliberately resolved differently from the mock. The twelve exploration concepts it was chosen from are not in this PR.🤖 Generated with Claude Code
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.