Sidebar 2.0: sync with next, implement sticky selected-node ancestors, deep review fixes#35399
Open
Sidnioulz wants to merge 1892 commits into
Open
Sidebar 2.0: sync with next, implement sticky selected-node ancestors, deep review fixes#35399Sidnioulz wants to merge 1892 commits into
Sidnioulz wants to merge 1892 commits into
Conversation
Defer superseding review updates, sync notification state to the store, and clear arrival notifications on dismiss or pending accept.
Surface new reviews via the manager notification API instead of auto-navigating away from the user's current canvas.
Replace auto-navigation on review push with the notification layer.
…pdates Use the shared review store for the widget title and navigate via the canonical review summary entry helper.
Reset the review store between stories, stabilize createdAt fixtures, align assertions with Update navigation and router-based widget opens, and wait for notification effects before asserting on mocks.
Resolve conflicts from the query rename (drop redundant `get` prefix) while keeping per-service registrars and typed getService. Update query/command wiring to docgen, storyDocs, and latestStoryChanges. Co-authored-by: Cursor <cursoragent@cursor.com>
The story mocked addNotification without rendering NotificationList, so the canvas showed no notification despite the play test passing. Use a harness that mirrors sidebar notification state and assert the visible headline.
Thumbnail previews on the review summary emitted STORY_SPECIFIED, which the manager treated as canvas navigation and replaced the URL. Treat review routes as non-canvas and keep review viewMode in the main layout so the summary stays mounted.
Yarn expects posix-path separated paths for --cwd, not os separated. Switch DIR=$PWD to DIR=$(pwd) for powershell compatibility
When embed=true, measure story content and post iframe.resize messages to the parent frame. Wire a final measurement into the story freezer via onFrozen for stable thumbnail dimensions after freeze completes.
Review thumbnails opt into content-dimension broadcast via embed=true, emitted before freeze=finished when both params are requested.
Listen for embed preview postMessages in CollectionGrid and derive per-cell scale and aspect ratio from measured content via computeThumbnailLayout.
Keeping it mounted left the previously selected story's iframe alive underneath the summary, so it flashed through for a frame when navigating to a curated story. Unmounting boots a fresh preview per navigation instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The overlay's inert state was gated on a suppression flag set when navigating to a curated story and released only by an effect keyed on the summary route becoming visible. A click that never left the summary route left the flag stuck, so the overlay stayed inert on return. The main preview now unmounts on the summary route, so the suppression mechanism is no longer needed to prevent a stale-story flash. Drop it and gate the overlay purely on the summary route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: ghengeveld <321738+ghengeveld@users.noreply.github.com>
parsePath now maps /review/ and /review to viewMode review so manager state matches the layout policy. Rename previewsPaused to summaryHidden across the thumbnail pipeline for clearer intent.
Remove REVIEW_MODE_SESSION_KEY when clearing review state so a reload during navigateOutOfReview cannot rehydrate dismissed review mode.
…5.0-alpha.9 Release: Prerelease 10.5.0-alpha.10
Manager: Restructure review feature state, layout integration, and thumbnail pipeline
…5.0-alpha.10 Release: Prerelease 10.5.0-alpha.11
…5.0-alpha.11 Release: Prerelease beta 10.5.0-beta.0
…re flags Extracts the inline `features` object type on `StorybookConfigRaw` into an exported `StorybookFeatures` interface. Addons can now declare their own feature flags via module augmentation of 'storybook/internal/types' instead of requiring users to @ts-expect-error unknown flags in main.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The review feature (manager UI layer + server review channel) previously mounted unconditionally; the only gate lived in the external addon-mcp. Core now declares `features.experimentalReview` (default false) and gates both sides on it (plus `changeDetection`, mirroring the addon's availability check) via a shared isReviewFeatureEnabled helper. The internal Storybook enables the flag to keep its review workflow active. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI: Update package versions and changelog
…type Core: Add experimentalReview feature flag and make the features type augmentable
…5.0-beta.0 Release: Prerelease 10.5.0-beta.1
Conflict resolutions (feature branch preferred; new features from next preserved): - manager-api/tests/stories.test.ts: took next's test structure (length check inside vi.waitFor, snapshot outside the retry loop) with the feature branch's snapshot content (includes renderAriaLabel). - MobileNavigation.tsx: kept next's MobileBottomBarContent extraction (fixes a useLandmark stale-landmark crash) and its new showMenu flag; threaded the feature branch's fullStoryAriaLabel through it. - preview/Toolbar.tsx: kept the feature branch's FullscreenTool component (Escape-key exit handler); preserved next's new isNavUnavailable guard. Also moved an early return below useEffect to respect rules of hooks. - sidebar/ContextMenu.tsx: kept the feature branch's ContextMenu component architecture (replaces next's useContextMenu hook); adopted next's optional-chaining on shortcutKeys?.openInEditor. next's visibleStatus param applies to the removed hook and was dropped. - sidebar/IconSymbols.tsx: kept feature branch's change-* symbol names; added next's new 'reviewing' symbol and next's plus-style 'new' glyph. - sidebar/ReviewChangesButton.tsx: took next's deliberate icon change (new -> modified), mapped to the feature branch's change-modified name. - sidebar/StatusContext.tsx: kept feature branch's dual-status hasStatusWorthReporting; added next's 'reviewing' as report-worthy. - utils/status.tsx: kept feature branch's O(n) getGroupStatus and Status-object getGroupDualStatus; preserved next's review-status semantics (statusesExcludingReview in group aggregation, isAggregatedTestStatus for test aggregation) and the new status-value:reviewing mapping (adapted to the icon/textColor shape). - sidebar/Tree.tsx: feature branch version taken wholesale (full react-aria rewrite). next's tweaks (REVIEW_STATUS_TYPE_ID filter on context-menu status links, getSidebarVisibleStatus for the menu button) target structures the rewrite removed - flagged in PR notes. - TreeNode.tsx: added 'status-value:reviewing' aria label for the StatusValue union extension from next. - yarn.lock: regenerated via yarn install (Yarn Berry auto-merge). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGgBpgK3t9Z8W1jQZLXSL
Sticky tree (VSCode-style sticky scroll, scoped to the selection): - Tree.tsx computes the selected story's ancestor chain (root-most first, plus the selected node itself) and passes a stickyIndex to chain rows. - TreeNode pins chain rows with position: sticky at stacked 28px offsets, using the sidebar background so rows scrolling beneath are covered. - A rAF-throttled scroll effect suspends a row's stickiness once its subtree has fully scrolled past its pinned slot, so the stack retracts instead of covering unrelated sections. Leaf rows never effectively pin because their subtree ends with themselves. - New Tree stories (light/dark) assert pinned offsets, non-sticky rows, and suspension via play functions. Post-merge repairs: - addon-docs: import useId from react-aria/useId (react-aria 3.48 no longer ships @react-aria/utils as a separate package) and declare the react-aria devDependency. - types/modules/api.ts: restore channels/index.ts and manager-api/index.ts import paths (feature-branch WIP had broken paths). - FilterPanel/SearchResults: adapt next's iconColor usage to the feature branch StatusMapping (icons self-color; labels use textColor). - utils/tree.ts: type-level cast for hoisted top-level stories that legitimately have no parent. - Refs.tsx: render error/auth/loading/empty blocks even when the ref has no index (regression that hid ErrorBlock and friends). - TreeNode: gate change-status icons through shouldShowChangeStatus (affected never shown; modified only while its filter is active), wire isModifiedFilterActive through StatusContext, add per-slot testids, add the reviewing aria label, and guard against missing shortcuts. - ContextMenu: optional-chain getShortcutKeys(). - Stories: rewrite TreeNode.stories for the react-aria Tree (the old file targeted the pre-rewrite API and did not compile), fix stale props in Tree/Explorer/Refs stories, adapt IconSymbols gallery to change-* symbol names, drop Tree's SkipToCanvasLinkFocused story (the link lives in Heading and is covered by Heading.stories). - Tests: escape now renders as ESCAPE (feature-branch behavior); update getGroupStatus snapshots for the sparse O(n) aggregation. - yarn.lock: re-resolve webpack to 5.108 (next's 5.105+ typing is required for core-webpack's devtool array form). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGgBpgK3t9Z8W1jQZLXSL
…sition react-aria 3.48's isInert (and calculatePosition's scale check) reads node.ownerDocument.defaultView.HTMLElement without a null check, which crashes with 'Cannot read properties of null' when a node lives in a detached document (e.g. story teardown in the vitest browser runner, or composed storybooks tearing down iframes). Null-guard defaultView in the same way react-aria's own getOwnerWindow helper does. Fixes the Panel and manager index story suites, which failed on mount with this crash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGgBpgK3t9Z8W1jQZLXSL
Correctness (all confirmed by failing repros before the fix): - utils/tree.ts: delete the replaced component entry when hoisting a single-story component; indexToTree resolves rows from parent pointers, so the surviving entry rendered as a phantom row next to the hoisted story (unit tests added). - utils/status.tsx: rewrite getGroupDualStatus as a single bottom-up pass that includes each story/docs entry's OWN statuses. Previously the aggregation was descendants-only, so a failing leaf story showed no status icon on its own row - only its ancestors did (unit tests added). Also O(total statuses x depth) instead of O(n*m). - Tree.tsx: selecting a story above the viewport never scrolled to it - its sticky-pinned row reported an in-view rect, so scrollIntoView no-oped and the suspension pass then unpinned it offscreen. Scrolling now measures natural positions (data-sticky-measuring disables pinning during measurement) and rows carry scroll-margin-top equal to the pinned stack height so targets land below the stack. - Tree.tsx: the sticky suspension state went stale on layout shifts that fire no scroll event (a sibling ref loading or collapsing, resizes); a ResizeObserver on the scroller and its content wrapper now re-syncs it. - Tree.tsx/TreeNode.tsx: pinned slots were hardcoded to 28px offsets while rows only have minHeight 28; the suspension pass now assigns measured cumulative tops so taller rows (custom labels, zoom) never overlap. - Tree.tsx: getScrollParent now matches overflow 'overlay' and falls back to the document scroller, so suspension still runs in embeddings without an auto/scroll ancestor. - preview/Toolbar.tsx: only attach the global Escape handler when the fullscreen tool is actually rendered (single-story mode without panels previously had Escape hijacked by an invisible tool). - Modal.tsx: honor e.preventDefault() from the deprecated onEscapeKeyDown handler (Radix contract) instead of closing unconditionally; this fixed the failing OnEscapeKeyDownPreventDefault story. - Search.tsx: stories whose only statuses are review-typed no longer short-circuit the group-status fallback with a truthy 'unknown'. Restored features that next had and the sidebar rewrite dropped: - Per-status context-menu entries (navigate + select the status via fullStatusStore) rendered as their own link group. - Context menus on group/component rows when test provider addons are registered (e.g. run tests for a whole group), threaded through hasContextMenu(context, hasTestProviders). - Hover preloading of a branch row's first child story (PRELOAD_ENTRIES), implemented as one delegated listener instead of a handler per row. Performance and hygiene: - Tree no longer subscribes to the whole manager state; the modified filter arrives as an includedStatusFilters prop from Refs. - StatusContext value is memoized, so unrelated Tree re-renders (focus tracking, context-menu state) no longer re-render every TreeNode. - One shared Collection dependencies array across nesting levels (the inner copy had already drifted and omitted groupDualStatus). - The suspension pass batches all layout reads before attribute writes and uses an id->index map instead of repeated indexOf scans. - Sticky rows' z-index is isolated to the tree, so the overlay scrollbar and the focus tooltip paint above pinned rows. - Removed the dead getSidebarVisibleStatus helper (zero consumers and a latent Status-vs-StatusValue contract mismatch), the unused isOrphan/isSelected TreeNode props, and duplicated row-height constants (stories now import TREE_ROW_HEIGHT). - MobileNavigation memoizes the combined index instead of rebuilding an index-sized object per render. - ecosystem-ci EXISTING_RESOLUTIONS covers the react-aria patch keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGgBpgK3t9Z8W1jQZLXSL
- TreeNode/Tree: suspended or measuring sticky rows now reset `top` (position:relative kept the pinned offset, displacing rows over their neighbors during stack retraction and skewing scroll measurements). The measured pin offset moved to a --sticky-top custom property so the suspended/measuring CSS can still override it. - Tree: the suspension effect now runs before the scroll effects and its cleanup keeps --sticky-stack-height, so a selection change scrolls with the correct scroll margin instead of landing under the pinned stack. - Tree: the selection scroll retries when the target row isn't rendered yet (ancestors expand one commit later than the selection change), and never re-scrolls for a selection that already succeeded. - utils/tree.ts: sibling single-story hoists compose — the parent's children rewrite now starts from the accumulator, not the stale source data, so two hoists under one parent no longer drop the first story and leave a dangling child id (unit test added). - TreeNode/Tree: group/component rows gate the context-menu button on the test providers actually contributing content for that item, instead of a blanket "some provider is registered" flag that opened empty menus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGgBpgK3t9Z8W1jQZLXSL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - if: ${{ steps.is-next-release-branch.outputs.result == 'true' }} | ||
| run: | | ||
| echo 'WARNING: Do not push directly to the `${{ needs.branch-checks.outputs.branch }}` branch. This branch is created and force-pushed over after pushing to the `${{ env.relevant-base-branch }}` branch and the changes you just pushed will be lost.' | ||
| echo 'WARNING: Do not push directly to the `${{ needs.branch-checks.outputs.branch }}` branch. This branch is created and force-pushed over after pushing to the `${{ steps.relevant-base-branch.outputs.relevant-base-branch }}` branch and the changes you just pushed will be lost.' |
| - if: ${{ steps.is-next-release-branch.outputs.result == 'true' }} | ||
| run: | | ||
| echo 'WARNING: Do not push directly to the `${{ needs.branch-checks.outputs.branch }}` branch. This branch is created and force-pushed over after pushing to the `${{ env.relevant-base-branch }}` branch and the changes you just pushed will be lost.' | ||
| echo 'WARNING: Do not push directly to the `${{ needs.branch-checks.outputs.branch }}` branch. This branch is created and force-pushed over after pushing to the `${{ steps.relevant-base-branch.outputs.relevant-base-branch }}` branch and the changes you just pushed will be lost.' |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #
What I did
Three workstreams on top of
issue-31267-sidebar-2:1. Branch health — merged
origin/next(1890 commits)All 10 conflicts resolved, preferring feature-branch logic while preserving every new feature from next. Decision log (full detail in the merge commit message,
59833bea):manager-api/tests/stories.test.tswaitFor, snapshot outside) + our snapshot content (renderAriaLabel)MobileNavigation.tsxMobileBottomBarContentextraction (useLandmark crash fix) +showMenuflag, with ourfullStoryAriaLabelthreaded throughpreview/Toolbar.tsxFullscreenTool(Escape-to-exit) + next'sisNavUnavailableguardsidebar/ContextMenu.tsxshortcutKeys?.openInEditoroptional chaining. next'svisibleStatus/status-links applied to the removed hook — restored equivalents in the review pass (see below)sidebar/IconSymbols.tsxchange-*symbol names + next's newreviewingsymbol and plus-stylenewglyphsidebar/ReviewChangesButton.tsxsidebar/StatusContext.tsxhasStatusWorthReporting+ next'sreviewingas report-worthyutils/status.tsxgetGroupStatus+ Status-object dual status, preserving next's review-status semantics (statusesExcludingReview,isAggregatedTestStatus) and the newreviewingmappingsidebar/Tree.tsxyarn.lockdevtoolarray form)Post-merge repairs: restored
channels/index.tsimport paths, adaptediconColorconsumers to ourStatusMapping, fixed addon-docs'@react-aria/utilsimport (react-aria 3.48 vendors it; nowreact-aria/useId+ devDependency), rewrote the staleTreeNode.stories.tsxfor the react-aria Tree, fixed stale props across Tree/Explorer/Refs stories, and patched react-aria 3.48'sisInert/calculatePositionnull-deref on detached documents (.yarn/patches/react-aria-npm-3.48.0-*.patch) which crashed the Panel and manager story suites.2. Sticky tree (VSCode-style sticky scroll, keyed to the selection)
The selected story's ancestor chain (plus the selected node itself, when it's a branch) pins to the top of the sidebar scroller while its subtree scrolls, stacked in order — not all top-level nodes:
position: stickywith measured cumulative offsets (--sticky-top), so rows taller than 28px never overlap.ResizeObserverre-syncs on layout shifts that fire no scroll event (sibling refs loading/collapsing, resizes).data-sticky-measuring) and applies the live stack height asscroll-margin-top, so selecting a story above the viewport scrolls correctly and never lands under the stack. Selection scrolls retry once collapsed ancestors have expanded and mounted.3. Deep review — 10-angle adversarial pass + gap sweep, 15 confirmed findings fixed
Highest-severity fixes (each was reproduced before fixing; regression tests added):
getGroupDualStatusaggregated descendants only; rewritten as a single bottom-up pass including own statuses (O(total statuses × depth) instead of O(n·m)).scrollIntoView).e.preventDefault()from the deprecatedonEscapeKeyDown(Radix veto contract).ErrorBlockwas gated behind a truthy index).fullStatusStore.selectStatuses), context menus on group/component rows when test providers contribute content, and hover preloading (PRELOAD_ENTRIES, now one delegated listener).StatusContextvalue is memoized (no more full-tree re-renders per status tick), the suspension pass batches layout reads before writes, and dead code (getSidebarVisibleStatus, unused props/constants) was removed.Flags for human review (not fixed here)
Figma could not be inspected from this environment — the network policy blocks figma.com (403 via proxy, tunnel refused headless), and the org's Figma MCP connector isn't enabled for this session. The sticky tree was implemented from the task description. Enabling the connector in a future session allows a true design-alignment pass. Flags from code inspection only (token naming/value drift deliberately not flagged):
scroll-margin-top, but RAC's scroller may not honor it) — partial residual of a fixed bug.useStatusSummaryinStatusContext.tsxwas orphaned before the merge (zero consumers) — WIP surface to keep or delete.Upstream (next-authored) issues observed during review — out of scope for this PR, listed for follow-up: mobile nav becomes unreachable when a persisted desktop "sidebar hidden" state sets
showMenu=false;docs/blocks/Source.tsxconditional hook;docs/blocks/ArgTypes.tsxnon-nullgetComponentIdassertion;ReviewWidgetdismiss drops the return-path payload;Layout.tsxfull-state subscription; convention drift (extensionless TS imports, rawconsole.*, two new*.test.tsxrendering React components).Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Full suite is green: 8032 unit/story tests passed (769 files), typecheck green across all 44 projects. New coverage: sticky pin/retract play tests (
Sidebar/TreeStickySelectedAncestors light/dark), rewrittenSidebar/TreeNodestories (types, expand/select, statuses, long names), hoisting unit tests (utils/tree.test.js), dual-status unit tests (utils/status.test.ts), leaf-status and no-phantom play assertions.Manual testing
cd code && yarn storybook:uiManager/Sidebar/TreeNode).Sidebar/Tree → Sticky Selected Ancestorsfor an isolated scrollable harness (light + dark).Documentation
No docs updated: the sticky behavior is UI-internal to the in-progress Sidebar 2.0 branch; docs should land with the feature branch itself.
Checklist for Maintainers
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsqa:neededorqa:skipfeature request🤖 Generated with Claude Code
https://claude.ai/code/session_01DNGgBpgK3t9Z8W1jQZLXSL
Generated by Claude Code