Skip to content

perf(core): memoize hot leaf components and lazy-render Tooltip content #3365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
orrgottlieb wants to merge 2 commits into
base: master
Choose a base branch
from
Open

perf(core): memoize hot leaf components and lazy-render Tooltip content #3365

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bdbe96
perf(core): memoize hot leaf components and lazy-render Tooltip content
orrgottlieb May 21, 2026
80b030a
fix(tooltip): revert lazy content render that broke Dialog setup
orrgottlieb May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/components/button/src/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable react/button-has-type */
import React, { type AriaAttributes, forwardRef, useCallback, useEffect, useMemo, useRef } from "react";
import React, { type AriaAttributes, forwardRef, memo, useCallback, useEffect, useMemo, useRef } from "react";
import { camelCase } from "es-toolkit";
import cx from "classnames";
import { useMergeRef, NOOP } from "@vibe/shared";
Expand Down Expand Up @@ -93,7 +93,9 @@ export interface ButtonProps extends VibeComponentProps {
tabIndex?: number;
}

const Button = forwardRef<HTMLButtonElement, ButtonProps>(
// TODO(perf): callers commonly pass inline `onClick`/`children` closures which defeat memoization.
// Memoization here still helps when parent props are stable; consumer call sites are out of scope for this PR.
const ButtonComponent = forwardRef<HTMLButtonElement, ButtonProps>(
(
{
className,
Expand Down Expand Up @@ -379,4 +381,9 @@ const Button = forwardRef<HTMLButtonElement, ButtonProps>(
}
);

ButtonComponent.displayName = "Button";

const Button = memo(ButtonComponent);
Button.displayName = "Button";

export default Button;
11 changes: 9 additions & 2 deletions packages/components/dialog/src/Dialog/Dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import cx from "classnames";
import React, { useState, useEffect, useRef, useContext, useCallback, useMemo } from "react";
import React, { memo, useState, useEffect, useRef, useContext, useCallback, useMemo } from "react";
import { createPortal } from "react-dom";
import {
useFloating,
Expand Down Expand Up @@ -29,7 +29,9 @@ import styles from "./Dialog.module.scss";
import { type DialogTriggerEvent, type DialogEvent, type DialogProps } from "./Dialog.types";
import { LayerContext, LayerProvider } from "@vibe/layer";

function Dialog({
// TODO(perf): consumers commonly pass inline `content` functions/`children`/`middleware` arrays which defeat
// memoization. Fixing those is out of scope for this PR; memo still avoids re-renders when parents pass stable props.
function DialogComponent({
// Core props
id,
"data-testid": dataTestId,
Expand Down Expand Up @@ -545,4 +547,9 @@ function Dialog({
);
}

DialogComponent.displayName = "Dialog";

const Dialog = memo(DialogComponent);
Dialog.displayName = "Dialog";

export default Dialog;
18 changes: 16 additions & 2 deletions packages/components/tooltip/src/Tooltip/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,13 @@ const globalState: { lastTooltipHideTS: number; openTooltipsCount: number } = {
openTooltipsCount: 0
};

export default class Tooltip extends PureComponent<TooltipProps> {
interface TooltipState {
shown: boolean;
}

// TODO(perf): consumers commonly pass inline `content` (functions/elements) which can't be deduped here.
// Lazy content rendering below avoids the work when the tooltip isn't visible.
export default class Tooltip extends PureComponent<TooltipProps, TooltipState> {
wasShown: boolean;
/* eslint-disable react/default-props-match-prop-types -- props inherited from DialogProps via Omit<> */
static defaultProps = {
Expand Down Expand Up @@ -143,10 +149,16 @@ export default class Tooltip extends PureComponent<TooltipProps> {
this.onTooltipHide = this.onTooltipHide.bind(this);

this.wasShown = false;
this.state = { shown: !!props.shouldShowOnMount };
}

renderTooltipContent() {
const { theme, content, className, style, maxWidth, title, image, icon, dir } = this.props;
const { theme, content, className, style, maxWidth, title, image, icon, dir, withoutDialog, open } = this.props;
// Lazy content rendering: skip the (potentially expensive) tooltip body unless the tooltip is
// actually visible. Dialog still calls this on every render, but we short-circuit early when hidden.
if (!withoutDialog && !this.state.shown && !open) {
return null;
}
Comment thread
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
Outdated
if (!content) {
// don't render empty tooltip
return null;
Expand Down Expand Up @@ -192,6 +204,7 @@ export default class Tooltip extends PureComponent<TooltipProps> {
const { onTooltipShow } = this.props;
globalState.openTooltipsCount++;
this.wasShown = true;
this.setState({ shown: true });
onTooltipShow && onTooltipShow();
}
}
Expand All @@ -202,6 +215,7 @@ export default class Tooltip extends PureComponent<TooltipProps> {
globalState.lastTooltipHideTS = Date.now();
globalState.openTooltipsCount--;
this.wasShown = false;
this.setState({ shown: false });
onTooltipHide && onTooltipHide();
}
}
Expand Down
11 changes: 9 additions & 2 deletions packages/core/src/components/AttentionBox/AttentionBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { forwardRef } from "react";
import React, { forwardRef, memo } from "react";
import cx from "classnames";
import type { AttentionBoxProps, AttentionBoxRole } from "./AttentionBox.types";
import AttentionBoxDefault from "./layouts/AttentionBoxDefault/AttentionBoxDefault";
Expand All @@ -8,7 +8,9 @@ import { ComponentDefaultTestId, ComponentVibeId } from "../../tests/constants";
import { getTestId } from "../../tests/test-ids-utils";
import styles from "./AttentionBox.module.scss";

const AttentionBox = forwardRef(
// TODO(perf): callers may pass inline `action`/`link` objects which defeat memoization.
// Memoization helps when parents pass stable props; fixing call sites is out of scope.
const AttentionBoxComponent = forwardRef(
(
{
compact = false,
Expand Down Expand Up @@ -71,4 +73,9 @@ const AttentionBox = forwardRef(
}
);

AttentionBoxComponent.displayName = "AttentionBox";

const AttentionBox = memo(AttentionBoxComponent);
AttentionBox.displayName = "AttentionBox";

export default AttentionBox;
11 changes: 9 additions & 2 deletions packages/core/src/components/Avatar/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { camelCase } from "es-toolkit";
import { getStyle } from "@vibe/shared";
import { ComponentDefaultTestId, getTestId } from "../../tests/test-ids-utils";
import cx from "classnames";
import React, { type AriaRole, useCallback, useMemo } from "react";
import React, { type AriaRole, memo, useCallback, useMemo } from "react";
import { isNil } from "es-toolkit";
import { type ElementAllowedColor, getElementColor } from "../../types/Colors";
import { type AvatarSize, type AvatarType } from "./Avatar.types";
Expand Down Expand Up @@ -114,7 +114,9 @@ export interface AvatarProps extends VibeComponentProps {
onClick?: (event: React.MouseEvent | React.KeyboardEvent, avatarId: string) => void;
}

const Avatar = ({
// TODO(perf): consumers may pass inline `tooltipProps`/badge prop objects, defeating memoization at call sites.
// Fixing those is out of scope; memo still avoids re-renders when parents pass stable props.
const AvatarComponent = ({
id,
type = "text",
className,
Comment on lines +119 to 122

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. avatarcomponent missing forwardref 📘 Rule violation ⚙ Maintainability

AvatarComponent is a function component wrapped with memo but is not implemented with
React.forwardRef, so consumers cannot pass refs to the Avatar’s root DOM element. This violates
the library’s required ref-forwarding pattern for components rendering DOM.
Agent Prompt 
## Issue description
`AvatarComponent` renders a root DOM element but does not use `React.forwardRef`, preventing callers from attaching a ref to the component in the standardized way.

## Issue Context
This PR changed the component definition to `AvatarComponent` and wrapped it with `memo`, but did not implement the required `forwardRef` pattern.

## Fix Focus Areas
- packages/core/src/components/Avatar/Avatar.tsx[119-147]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down Expand Up @@ -264,4 +266,9 @@ const Avatar = ({
);
};

AvatarComponent.displayName = "Avatar";

const Avatar = memo(AvatarComponent);
Avatar.displayName = "Avatar";

export default Avatar;
Loading