Core: Onboarding guide #32874
Core: Onboarding guide #32874
Conversation
…first task for each section rather than first 3 open tasks
…lar set of (known) tasks
…e ChecklistModule
This reverts commit a55985b.
…o allow muting the widget
… in the settings page navigation
📝 WalkthroughWalkthroughExtended the checklist user settings schema with optional muted field, introduced new focus-ring component system for highlight management, refactored collapsible and button components for interactive state control, replaced the sidebar checklist display with a dynamic interactive module featuring task progress tracking and item-level actions, and restructured the settings/guide page with location-hash-based navigation and mute controls. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Sidebar/Guide
participant Store as ChecklistStore
participant Hooks as useChecklist Hook
participant Focus as FocusTarget
User->>App: Navigate to Guide
App->>Hooks: Call useChecklist()
Hooks->>Store: Read checklist state (completed, skipped, muted)
Store-->>Hooks: Return state
Hooks->>Hooks: Compute progress, nextItems, isReady items
Hooks-->>App: Return computed checklist data
App->>App: Render task list with FocusTarget
User->>App: Click task action
App->>Store: Call complete/skip/mute
Store->>Store: Update state (completed/skipped/muted)
Store-->>App: Emit state change
App->>Hooks: Re-compute derived state
Hooks-->>App: Return updated progress & nextItems
App->>App: Re-render with new progress
User->>Focus: Navigate via location.hash
Focus->>Focus: useLocationHash detects hash change
Focus->>Focus: Focus target element
Focus->>Focus: Apply highlight (FocusOutline active)
Focus->>Focus: Set timeout for highlight duration
Focus->>Focus: Clear highlight after duration
sequenceDiagram
participant Menu as useMenu Hook
participant State as Redux State
participant MenuUI as SidebarMenuList
participant ListboxItem as ListboxItem
Note over Menu: Updated hook signature<br/>(api, flags object)
MenuUI->>Menu: Call useMenu with {api, showToolbar, isPanelShown, ...}
Menu->>State: Access state (isNavShown, isPanelShown, etc.)
Menu->>Menu: Build menu rows with guide item
Menu->>Menu: Compute guide progress from useChecklist
Menu-->>MenuUI: Return structured menu: NormalLink[][]
MenuUI->>MenuUI: Map menu to Listbox items
MenuUI->>ListboxItem: Render each link as ListboxAction
ListboxItem->>ListboxItem: Show ListboxIcon for progress/status
ListboxItem->>MenuUI: onClick (link id, active, href)
MenuUI->>MenuUI: Navigate or toggle based on link type
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit fd39890
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
code/core/src/components/components/Button/Button.tsx (4)
42-51: Guard clicks when disabled/readOnly (also foras="a").Currently clicks still propagate, animate, and call
onClick. Block interaction at the handler level.Apply this diff:
- const handleClick = (event: SyntheticEvent) => { - if (onClick) { - onClick(event); - } - - if (animation === 'none') { - return; - } - setIsAnimating(true); - }; + const handleClick = (event: SyntheticEvent) => { + if (disabled || readOnly) { + event.preventDefault?.(); + event.stopPropagation?.(); + return; + } + onClick?.(event); + if (animation !== 'none') setIsAnimating(true); + };
63-76: Wire a11y and interactivity props (aria-disabled, tabIndex, role) and drop handler when not interactive.Make non-interactive elements non-focusable; add semantics for div/label.
Apply this diff:
- <StyledButton + {/* + Treat readOnly as non-interactive UI: aria-disabled + no handler. + For div/label, expose role=button only when interactive. + */} + <StyledButton as={Comp} ref={ref} variant={variant} size={size} padding={padding} disabled={disabled} - readOnly={readOnly} + readOnly={readOnly} active={active} animating={isAnimating} animation={animation} - onClick={handleClick} + role={as === 'div' || as === 'label' ? 'button' : undefined} + tabIndex={disabled || readOnly ? -1 : undefined} + aria-disabled={disabled || readOnly || undefined} + onClick={disabled || readOnly ? undefined : handleClick} {...props} />
83-90: Prevent leaking styling props (incl. readOnly) to the DOM.
isPropValiddoesn’t filter tag-specific props;readOnlyandsizemay end up on a<button>/<div>. Explicitly exclude styling-only props.Apply this diff:
-const StyledButton = styled('button', { - shouldForwardProp: (prop) => isPropValid(prop), -})< +const FILTER_PROPS = new Set([ + 'variant', + 'size', + 'padding', + 'active', + 'animating', + 'animation', + 'readOnly', +]); + +const StyledButton = styled('button', { + shouldForwardProp: (prop) => isPropValid(prop) && !FILTER_PROPS.has(String(prop)), +})<
62-77: Stories and RTL tests for readOnly behavior not yet implemented—required before merge.The review requirements have not been met:
Missing readOnly stories: Button.stories.tsx lacks stories demonstrating disabled vs readOnly behavior across
asvariants (existing stories are Base, Variants, PseudoStates, Active, WithIcon, IconOnly, Sizes, Disabled, WithHref, Animated—none cover readOnly).Missing RTL tests: No test files exist for the Button component. RTL tests asserting onClick is prevented when readOnly or disabled are required.
onClick handler issue: The handleClick function (lines 42–45) does not check the readOnly or disabled state before invoking the callback, allowing onClick to fire even when readOnly is true. This must be guarded.
No tabIndex implementation: No tabIndex handling found in the component for focus behavior tests.
Add the missing stories and tests, and prevent onClick execution when
readOnlyordisabledis true.code/core/src/manager/components/sidebar/Tree.tsx (2)
321-331: Aria label inverted (Expand/Collapse) on root toggleLabel reads “Expand” when fully expanded and “Collapse” when not, which is reversed.
Apply:
- aria-label={isFullyExpanded ? 'Expand' : 'Collapse'} + aria-label={isFullyExpanded ? 'Collapse' : 'Expand'}
398-405: Guard PRELOAD_ENTRIES against empty childrenWhen a component/story has no children,
children[0]is undefined and will be emitted.Apply:
- onMouseEnter={() => { - if (item.type === 'component' || item.type === 'story') { - api.emit(PRELOAD_ENTRIES, { - ids: [children[0]], - options: { target: refId }, - }); - } - }} + onMouseEnter={() => { + if ((item.type === 'component' || item.type === 'story') && children.length > 0) { + api.emit(PRELOAD_ENTRIES, { + ids: [children[0]], + options: { target: refId }, + }); + } + }}code/core/src/components/components/Card/Card.stories.tsx (1)
8-10: Add React type-only import forReactNodeAvoid relying on a global
Reactnamespace in types.Apply:
-import preview from '../../../../../.storybook/preview'; -import { Card } from './Card'; +import preview from '../../../../../.storybook/preview'; +import { Card } from './Card'; +import type { ReactNode } from 'react'; -const Contents = ({ children }: { children: React.ReactNode }) => ( +const Contents = ({ children }: { children: ReactNode }) => ( <div style={{ padding: 16 }}>{children}</div> );
🧹 Nitpick comments (22)
code/core/src/components/components/Button/Button.tsx (5)
21-21: Ref type is too narrow for polymorphic “as”.When
as="a" | "div" | "label", the ref isn’t anHTMLButtonElement. Widen toHTMLElement(or adopt a proper polymorphic typing later).Apply this diff:
-export const Button = forwardRef<HTMLButtonElement, ButtonProps>( +export const Button = forwardRef<HTMLElement, ButtonProps>(
103-103: Cursor for readOnly should be ‘default’, not ‘inherit’.‘inherit’ can pick up odd parents. Use ‘default’ to signal non-interactive.
Apply this diff:
- cursor: disabled ? 'not-allowed' : readOnly ? 'inherit' : 'pointer', + cursor: disabled ? 'not-allowed' : readOnly ? 'default' : 'pointer',
184-187: Keep focus-visible even when readOnly, or remove from tab order.You gate
&:focusunder!readOnly, yet the element may still be focusable. Either always render a focus ring or settabIndex={-1}(proposed above) so it’s not focusable.Apply this diff to always allow the focus ring:
- ...(!readOnly && { - '&:hover': { + ...(!readOnly && { + '&:hover': { color: theme.barHoverColor, background: transparentize(0.86, theme.barHoverColor), }, - - '&:active': { + '&:active': { color: theme.barSelectedColor, background: transparentize(0.9, theme.barSelectedColor), }, - - '&:focus': { - boxShadow: `${rgba(theme.barHoverColor, 1)} 0 0 0 1px inset`, - outline: 'none', - }, }), + '&:focus': { + boxShadow: `${rgba(theme.barHoverColor, 1)} 0 0 0 1px inset`, + outline: 'none', + },- ...(!readOnly && { - '&:hover': { + ...(!readOnly && { + '&:hover': { /* ... */ }, - '&:active': { + '&:active': { /* ... */ }, - - '&:focus': { - boxShadow: `${rgba(theme.color.secondary, 1)} 0 0 0 1px inset`, - outline: 'none', - }, }), + '&:focus': { + boxShadow: `${rgba(theme.color.secondary, 1)} 0 0 0 1px inset`, + outline: 'none', + },Also applies to: 256-259
262-265: Don’t animate icons on readOnly.Skip SVG animation when non-interactive.
Apply this diff:
- animation: - animating && animation !== 'none' ? `${theme.animation[animation]} 1000ms ease-out` : '', + animation: + !readOnly && animating && animation !== 'none' + ? `${theme.animation[animation]} 1000ms ease-out` + : '',
8-19: Consider proper polymorphic typing foras+ element-specific props.With
as="a", consumers can’t passhref(not inButtonHTMLAttributes<HTMLButtonElement>). A discriminated union or a polymorphic helper type would improve DX and safety.If you want, I can provide a minimal
PolymorphicComponentProps<As, OwnProps>pattern.code/core/src/manager/components/TextFlip.stories.tsx (1)
10-10: Consider removing debug styling.The red border appears to be for development/debugging purposes. Consider removing it or making it conditional for Chromatic visual testing only.
Apply this diff if the border is not needed:
- <div style={{ display: 'inline-block', border: '1px solid red' }}> + <div style={{ display: 'inline-block' }}> <TextFlip {...args} /> </div>Or keep it only for visual testing:
- <div style={{ display: 'inline-block', border: '1px solid red' }}> + <div style={{ display: 'inline-block' }}> <TextFlip {...args} /> </div>code/core/src/manager/components/sidebar/Sidebar.tsx (1)
161-175: Use isDevelopment prop consistently for gatingYou compute
isDevelopmentalready; use it instead of referencingglobal.CONFIG_TYPEdirectly.Apply:
- {!isLoading && global.CONFIG_TYPE === 'DEVELOPMENT' && <ChecklistModule />} + {!isLoading && isDevelopment && <ChecklistModule />}code/core/src/manager/container/Menu.stories.tsx (1)
9-37: Ensure mock store init actually runs in stories
beforeEachon CSF meta may not execute in Storybook runtime. Prefer a decorator that seeds the store per render.Apply:
export default { component: TooltipLinkList, decorators: [ - (storyFn) => ( + (storyFn) => { + // Seed mock store before each story render + mockStore.setState({ + loaded: true, + muted: false, + completed: ['add-component'], + skipped: ['add-5-10-components'], + }); + return ( <div style={{ height: '300px', }} > <WithTooltip placement="top" startOpen tooltip={storyFn()}> <div>Tooltip</div> </WithTooltip> </div> - ), + ); + }, ], - beforeEach: async () => { - mockStore.setState({ - loaded: true, - muted: false, - completed: ['add-component'], - skipped: ['add-5-10-components'], - }); - }, excludeStories: ['links'], } satisfies Meta<typeof TooltipLinkList>;code/core/src/manager/settings/GuidePage.tsx (1)
; keep semantic action
50-63: Replace obsoleteUse CSS centering instead of
<center>. KeepLinkif you want inline style, but avoid obsolete tags.Apply:
+const FooterNote = styled.div({ + textAlign: 'center', +}); @@ - {checklist.muted ? ( - <center> + {checklist.muted ? ( + <FooterNote> Want to see this in the sidebar?{' '} <Link onClick={() => checklist.mute(false)}>Show in sidebar</Link> - </center> + </FooterNote> ) : ( - <center> + <FooterNote> Don't want to see this in the sidebar?{' '} <Link onClick={() => checklist.mute(checklist.allItems.map(({ id }) => id))}> Remove from sidebar </Link> - </center> + </FooterNote> )}code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
35-46: Consider renaming "Proxy" story to avoid shadowing global.The export name
Proxyshadows the JavaScript globalProxyobject. While this is a named export and less problematic than a local variable, it could still cause confusion.Consider renaming:
-export const Proxy = meta.story({ +export const ProxyExample = meta.story({ render: () => (code/core/src/manager/settings/index.tsx (2)
46-53: Remove meaningless key prop on TabButton.
key="id"is a literal string and not needed outside list rendering.Apply:
- key="id"
122-124: Gate the route with enableWhatsNew to avoid dangling navigation.The tab is hidden when
enableWhatsNewis false, but the route remains active. Keep them consistent.- <Route path="whats-new"> - <WhatsNewPage key="whats-new" /> - </Route> + {enableWhatsNew && ( + <Route path="whats-new"> + <WhatsNewPage key="whats-new" /> + </Route> + )}code/core/src/components/components/Card/Card.tsx (1)
52-56: Avoid leakingcolorprop to the DOM; use a non-DOM prop name.Using
coloras a custom prop risks forwarding it to the DOM. The diff above renames it tooutlineColorKey. Keep non-DOM prop names (prefixed or renamed) to avoid React warnings.Also applies to: 63-66
code/core/src/manager/container/Menu.tsx (1)
47-50: AvoidreadOnlyon a button; use a non-interactive container.Buttons without actions are confusing for a11y and keyboard users. The diff above swaps it for a non-interactive div with status semantics.
Also applies to: 91-96
code/core/src/components/components/FocusRing/FocusRing.tsx (1)
55-70: Use isomorphic timeout type
NodeJS.Timeoutcan conflict in DOM builds. PreferReturnType<typeof setTimeout>.Apply:
- let timeout: NodeJS.Timeout; + let timeout: ReturnType<typeof setTimeout>;code/core/src/manager/settings/Checklist/Checklist.tsx (2)
269-285: Remove unusednodeRefto reduce allocations
nodeRefis created but never attached or read. Drop it and the related import.Apply:
-import React, { createRef, useMemo } from 'react'; +import React, { useEffect, useMemo } from 'react'; @@ type ChecklistItem = ChecklistData['sections'][number]['items'][number] & { isCompleted: boolean; isLockedBy: string[]; isSkipped: boolean; - nodeRef?: React.RefObject<HTMLLIElement>; }; @@ - const nodeRef = createRef<HTMLLIElement>(); - return [item.id, { ...item, isCompleted, isLockedBy, isSkipped, nodeRef }]; + return [item.id, { ...item, isCompleted, isLockedBy, isSkipped }];
333-336: Optional: reflect hash-target state in ListboxItemactivestylingPassing
activeimproves affordance when an item is hash-targeted.Apply:
- <ListboxItem key={item.id}> + <ListboxItem key={item.id} active={itemId === locationHash}>code/core/src/components/components/Collapsible/Collapsible.tsx (4)
25-26: Avoid requiringcollapsedvia type intersection.Intersecting with
ComponentProps<typeof CollapsibleContent>can accidentally makecollapsedrequired. UsingOmit<..., 'id' | 'collapsed' | 'aria-hidden'>(as in the diff above) prevents this and keepscollapsedoptional.Please build TS to ensure
Collapsedstory (which omitscollapsed) type-checks.
29-41: Remove duplicatearia-hidden.
aria-hiddenis set in bothCollapsibleandCollapsibleContent. Keep it in one place to avoid confusion (the diff above keeps it inCollapsibleContentonly).
78-84: Enhance a11y and flexibility oftoggleProps.Spreading
disabledon non-button elements is ineffective. Addaria-disabledalongsidedisabledso it’s useful for non-native controls.- const toggleProps = { - disabled, - onClick: toggleCollapsed, + const toggleProps = { + disabled, + 'aria-disabled': !!disabled, + onClick: toggleCollapsed, 'aria-controls': contentId, 'aria-expanded': !isCollapsed, } as const;
67-75: Reconsider defaultstopPropagation().Always stopping propagation can interfere with parent click handlers. Either make this behavior opt-in (e.g., a parameter) or document it clearly.
Confirm no parent components rely on click bubbling from summary controls.
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
39-41: Interaction sanity-check.
Toggledclicks “Close” (default open) which collapses content; good smoke test. Consider adding an assertion onaria-expandedfor robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
code/core/src/cli/globalSettings.ts(1 hunks)code/core/src/components/components/Button/Button.tsx(4 hunks)code/core/src/components/components/Card/Card.stories.tsx(1 hunks)code/core/src/components/components/Card/Card.tsx(4 hunks)code/core/src/components/components/Collapsible/Collapsible.stories.tsx(3 hunks)code/core/src/components/components/Collapsible/Collapsible.tsx(2 hunks)code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx(0 hunks)code/core/src/components/components/FocusProxy/FocusProxy.tsx(0 hunks)code/core/src/components/components/FocusRing/FocusRing.stories.tsx(1 hunks)code/core/src/components/components/FocusRing/FocusRing.tsx(1 hunks)code/core/src/components/components/Listbox/Listbox.stories.tsx(3 hunks)code/core/src/components/components/Listbox/Listbox.tsx(4 hunks)code/core/src/components/components/shared/useLocationHash.ts(1 hunks)code/core/src/components/index.ts(2 hunks)code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager/components/TextFlip.stories.tsx(1 hunks)code/core/src/manager/components/TextFlip.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Explorer.tsx(0 hunks)code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Menu.tsx(2 hunks)code/core/src/manager/components/sidebar/SearchResults.tsx(0 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(3 hunks)code/core/src/manager/components/sidebar/Sidebar.tsx(5 hunks)code/core/src/manager/components/sidebar/TestingModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Tree.tsx(1 hunks)code/core/src/manager/components/sidebar/TreeNode.tsx(1 hunks)code/core/src/manager/components/sidebar/useChecklist.ts(1 hunks)code/core/src/manager/container/Menu.stories.tsx(2 hunks)code/core/src/manager/container/Menu.tsx(12 hunks)code/core/src/manager/container/Sidebar.tsx(2 hunks)code/core/src/manager/globals/exports.ts(3 hunks)code/core/src/manager/settings/Checklist/Checklist.stories.tsx(2 hunks)code/core/src/manager/settings/Checklist/Checklist.tsx(5 hunks)code/core/src/manager/settings/Checklist/checklistData.tsx(7 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)code/core/src/manager/settings/GuidePage.tsx(2 hunks)code/core/src/manager/settings/index.tsx(3 hunks)code/core/src/shared/checklist-store/index.ts(2 hunks)
💤 Files with no reviewable changes (4)
- code/core/src/components/components/FocusProxy/FocusProxy.tsx
- code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx
- code/core/src/manager/components/sidebar/Explorer.tsx
- code/core/src/manager/components/sidebar/SearchResults.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/core/src/manager/settings/GuidePage.stories.tsx
🧬 Code graph analysis (23)
code/core/src/manager/components/sidebar/Sidebar.tsx (1)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (1)
ChecklistModule(67-205)
code/core/src/manager/components/TextFlip.stories.tsx (1)
code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(57-104)
code/core/src/components/components/shared/useLocationHash.ts (2)
code/renderers/web-components/template/components/Html.js (1)
globalThis(5-5)code/core/src/components/index.ts (1)
useLocationHash(96-96)
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (3)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-243)code/core/src/manager/manager-stores.mock.ts (1)
checklistStore(57-57)code/core/src/manager/manager-stores.ts (1)
checklistStore(25-25)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
code/core/src/components/components/FocusRing/FocusRing.tsx (3)
FocusRing(23-41)FocusProxy(17-21)FocusTarget(43-73)
code/core/src/manager/settings/GuidePage.tsx (2)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(12-57)code/core/src/manager/settings/Checklist/Checklist.tsx (1)
Checklist(243-446)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-243)
code/core/src/components/components/FocusRing/FocusRing.tsx (1)
code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
code/core/src/components/components/Card/Card.tsx (1)
Card(35-44)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (6)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
Collapsible(12-37)code/core/src/components/components/Listbox/Listbox.tsx (6)
ListboxButton(61-68)Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/manager-api/root.tsx (1)
useStorybookApi(294-297)code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(57-104)code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)code/core/src/manager/manager-stores.ts (1)
checklistStore(25-25)
code/core/src/manager/components/sidebar/Menu.stories.tsx (1)
code/core/src/manager/container/Menu.tsx (1)
useMenu(59-257)
code/core/src/manager/components/sidebar/Menu.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (5)
Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/components/index.ts (5)
Listbox(48-48)ListboxItem(52-52)ListboxAction(49-49)ListboxIcon(51-51)ListboxText(53-53)
code/core/src/manager/components/TextFlip.tsx (1)
code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
code/core/src/components/index.ts (1)
Collapsible(44-44)
code/core/src/manager/settings/GuidePage.stories.tsx (3)
code/core/src/manager/settings/GuidePage.tsx (1)
GuidePage(37-65)code/core/src/manager-api/root.tsx (1)
ManagerContext(78-78)code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
Default(36-45)
code/core/src/manager/container/Menu.tsx (3)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(12-57)code/core/src/components/components/Listbox/Listbox.tsx (2)
ListboxButton(61-68)ListboxIcon(96-103)code/core/src/manager-api/modules/shortcuts.ts (1)
getAddonsShortcuts(187-189)
code/core/src/manager/settings/index.tsx (1)
code/core/src/manager/settings/GuidePage.tsx (1)
GuidePage(37-65)
code/core/src/manager/settings/Checklist/Checklist.tsx (6)
code/core/src/manager/components/sidebar/useChecklist.ts (2)
ChecklistItem(8-8)useChecklist(12-57)code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)code/core/src/components/components/FocusRing/FocusRing.tsx (2)
FocusProxy(17-21)FocusTarget(43-73)code/core/src/components/components/Listbox/Listbox.tsx (1)
ListboxItem(17-59)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Actions(112-116)code/core/src/components/components/Button/Button.tsx (1)
Button(21-79)
code/core/src/components/components/Card/Card.stories.tsx (1)
code/core/src/components/components/Card/Card.tsx (1)
Card(35-44)
code/core/src/manager/container/Sidebar.tsx (2)
code/core/src/manager/container/Menu.tsx (1)
useMenu(59-257)code/core/src/manager/manager-stores.ts (1)
experimental_useStatusStore(13-13)
code/core/src/components/components/Listbox/Listbox.tsx (1)
code/core/src/components/index.ts (5)
ListboxItem(52-52)ListboxAction(49-49)ListboxButton(50-50)ListboxText(53-53)ListboxIcon(51-51)
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
useCollapsible(58-93)
code/core/src/components/components/Listbox/Listbox.stories.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (4)
ListboxButton(61-68)ListboxItem(17-59)ListboxAction(70-79)ListboxText(81-94)code/core/src/components/index.ts (7)
ListboxButton(50-50)ListboxItem(52-52)ListboxAction(49-49)ProgressSpinner(92-92)Badge(28-28)Form(66-66)ListboxText(53-53)
🪛 Biome (2.1.2)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx
[error] 35-35: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
code/core/src/manager/components/sidebar/Menu.tsx
[error] 80-80: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
code/core/src/components/components/Collapsible/Collapsible.tsx
[error] 17-17: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 59-59: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 61-61: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 67-67: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 77-77: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
code/core/src/manager/container/Sidebar.tsx
[error] 59-59: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (36)
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
21-29: LGTM! Theme-aware styling improvement.The refactoring to use a function-based style with the theme parameter enables dynamic shadow rendering that adapts to the theme. The CSS variable
--card-box-shadowallows child components to access the computed shadow value.code/core/src/cli/globalSettings.ts (1)
18-24: Union type formutedfield is correctly handled with proper type narrowing.The schema extension is sound. The
mutedfield's union type (boolean | string[]) is being properly consumed throughout the codebase:
- The store's
mutefunction usesArray.isArray(value)to distinguish between boolean (blanket muting) and array (selective item muting), properly merging array values with existing state- The UI component uses
Array.isArray(muted)before accessing array methods likeincludes()- Initialization defaults to
falsewhen the value is undefinedThe dual-use pattern (boolean for whole-checklist muting, array for selective item muting) is intentional and correctly implemented with type guards.
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
5-6: LGTM: Checklist store integration follows established patterns.The addition of
universalChecklistStoreinitialization in the globalbeforeEachhook and the import ofglobalfor production-mode testing are both consistent with the broader checklist integration across the codebase.Also applies to: 13-13, 107-112
158-164: LGTM: Proper cleanup of global state modification.The production-mode simulation correctly mutates
global.CONFIG_TYPEand restores the original value via the cleanup function returned frombeforeEach. This ensures test isolation.code/core/src/manager/components/TextFlip.tsx (1)
1-104: LGTM: Well-implemented text flip animation component.The component correctly uses
react-transition-groupto animate between text values, with proper support for:
- Direction detection via
localeComparewith numeric sorting- Accessibility via
aria-hiddenon stale content- Reduced motion preferences
- Ref forwarding to avoid findDOMNode warnings
The dual-Transition pattern (one for entering, one for exiting) is the correct approach for simultaneous enter/exit animations.
code/core/src/manager/components/sidebar/TreeNode.tsx (1)
91-93: LGTM: Styling adjustment for first tree node.The CSS rule removes the top margin from the first
RootNode, which aligns with the layout changes for the onboarding guide integration.code/core/src/manager/settings/GuidePage.stories.tsx (1)
1-36: LGTM: Story setup follows established patterns.The GuidePage story properly mocks the ManagerContext and initializes the checklist store state, consistent with other checklist-related story files in the codebase.
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (1)
24-31: LGTM: Complete checklist store initialization.The addition of
loadedandmutedfields aligns with the expanded checklist store schema and is consistent with other story files in this PR.code/core/src/core-server/utils/checklist.ts (1)
17-22: LGTM: Complete checklist store initialization with new fields.The addition of
loaded: trueandmuted(with proper fallback) extends the store state to match the updated schema. The immediate setting ofloaded: trueis appropriate since settings are loaded synchronously from the settings file.code/core/src/manager/globals/exports.ts (1)
1-3: LGTM: Public API expanded with new components and utilities.The additions of
FocusOutline,FocusRing,FocusTarget,ListboxIcon, anduseLocationHashto the public exports are correctly present in the generated file. These exports are automatically discovered since they are exported fromcode/core/src/components/index.ts, which is bundled as part of the auto-generation process incode/core/scripts/generate-source-files.ts. No manual generator configuration changes are required—the mechanism is fully automatic.code/core/src/manager/settings/GuidePage.tsx (1)
37-50: Nice: unified state via useChecklistSwitching to
useChecklistkeeps Guide and Sidebar in sync. Looks good.code/core/src/components/index.ts (1)
46-54: Public API expansion looks goodExporting Focus components, ListboxIcon, and useLocationHash aligns with usage across manager UI. No issues spotted.
Also applies to: 96-97
code/core/src/manager/components/sidebar/Tree.tsx (1)
716-720: Stale hasOrphans references resolved; spacing regression requires manual verification
- ✓ No stale
hasOrphansprop references found in codebase- ✓ Orphan hoisting logic and
isOrphanprop remain intact- ⚠ Cannot verify whether styled Container had conditional orphan margin or if visual regression occurs — the original Container definition is not accessible. Manual testing of sidebar spacing with hoisted orphans is required to confirm no regression.
code/core/src/components/components/shared/useLocationHash.ts (1)
37-41: LGTM!The hook correctly subscribes to the hash monitor, handles cleanup, and returns the hash without the leading
#character.code/core/src/manager/components/sidebar/Menu.tsx (1)
4-12: LGTM!The new Listbox-related imports are correctly added and align with the refactored menu rendering.
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (2)
3-3: LGTM!The checklistStore import is correctly added and used in the story configuration.
36-44: LGTM!The story args correctly merge checklist data and store, with explicit state fields for testing different scenarios.
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (3)
20-33: LGTM!The Active and Temporary stories correctly demonstrate the FocusRing component with different duration behaviors.
48-64: LGTM!The Target story correctly demonstrates hash-based focus behavior with appropriate timing for the play function.
65-67: LGTM!The beforeEach hook correctly resets the hash state between story runs to ensure consistent test behavior.
code/core/src/manager/components/sidebar/Menu.stories.tsx (2)
13-13: LGTM!The mock store setup correctly initializes checklist state for consistent story behavior across all tests.
Also applies to: 32-39
66-79: LGTM!All three stories correctly updated to use the new useMenu object-based signature, consistently passing the required properties.
Also applies to: 108-134, 157-170
code/core/src/manager/container/Sidebar.tsx (2)
36-53: LGTM!The mapper refactor correctly exposes the necessary properties for menu construction while maintaining clean separation of concerns.
57-73: LGTM!The Consumer render correctly computes the menu using the new useMenu signature and passes all required props to SidebarComponent. The Biome warning about hook usage is a false positive—the Consumer render prop is effectively the component body, and hook usage here is valid.
code/core/src/manager/components/sidebar/useChecklist.ts (3)
8-10: LGTM!The type extraction and allItems computation are clean and efficient.
12-27: LGTM!The hook setup correctly handles checklist state with proper memoization. The
Array.isArray(muted)check on line 24 defensively handles cases wheremutedmight have different types.
29-56: LGTM with consideration for property spreading.The computed values correctly implement the round-robin selection logic for next items and progress calculation. The return statement spreads multiple objects (
checklistData,checklistStore,state) which could potentially cause property name collisions, but this appears intentional for the consumer API.code/core/src/components/components/Listbox/Listbox.stories.tsx (4)
3-3: LGTM!The Badge and ProgressSpinner imports are correctly added and used in the story examples.
20-22: LGTM!The aria-label addition improves accessibility for the icon-only button.
35-49: LGTM!The new story items effectively demonstrate integration with Badge and ProgressSpinner components.
50-62: LGTM!The active prop correctly moved to ListboxItem and the checkbox label text is properly wrapped in ListboxText for consistent styling.
code/core/src/components/components/Listbox/Listbox.tsx (1)
12-15: Widened sibling selector may cause unintended bordersChanging to '& + *' affects any following sibling, not just another Listbox. If the intent was only stacked Listboxes, revert to a narrower selector.
Apply if appropriate:
- '& + *': { + '& + &': { borderTop: `1px solid ${theme.appBorderColor}`, },code/core/src/components/components/FocusRing/FocusRing.tsx (1)
17-21: Confirm :has() support or add a non-:has fallback
&:has(#id:focus-visible)is great but not universally reliable in older engines. Consider a fallback (e.g., also styling when the proxy itself is focus-visible).code/core/src/manager/settings/Checklist/checklistData.tsx (1)
14-20: Structured, content‑rich checklist with sequencing looks goodAction objects,
afterdependencies, and JSX content improve UX and clarity. Ensure the consumer invokespredicatefunctions; otherwise these items won’t auto-complete. See my Checklist.tsx comment for a concrete fix.Also applies to: 24-41, 45-75, 83-104, 106-125, 127-148, 150-172, 174-195, 203-214, 216-227, 229-239
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (2)
7-14: Stories align with new API; type-only import is correct.Use of
togglePropsandReturnType<typeof useCollapsible>is tidy and keeps runtime bundle clean.
43-54: Controlled story looks good.Local state toggles and play action validate the controlled path.
Run:
…es, update Listbox components to use div elements, and adjust related logic in ChecklistModule and useChecklist hook.
code/core/src/components/components/Collapsible/Collapsible.tsx
Outdated
Show resolved
Hide resolved
…h current prop, preventing broken animations during rapid changes.
See #32644
Telescopes on #32795
What I did
This PR handles all the UI improvements made for the onboarding guide / checklist since #32795.
Checklist data and completion behavior goes in #32799.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
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.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements