Skip to content

refactor: replace Popover with Tooltip in HelpTooltip #19635

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

Merged
merged 36 commits into from
Sep 8, 2025
Merged

refactor: replace Popover with Tooltip in HelpTooltip #19635

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
fba9492
refactor: replace Popover with Tooltip in AgentLatency
aqandrew Aug 28, 2025
e230f38
refactor: replace PopoverTrigger with TooltipTrigger in AgentStatus
aqandrew Aug 28, 2025
75c9fdf
refactor: replace PopoverTrigger with TooltipTrigger in
aqandrew Aug 28, 2025
bea24d8
fix: remove HelpTooltipText from AgentLatency (div inside p)
aqandrew Aug 28, 2025
9e6fa0c
fix: add TooltipProvider to AuditHelpTooltip
aqandrew Aug 28, 2025
303222b
fix: add TooltipProvider to ConnectionLogHelpTooltip,
aqandrew Aug 28, 2025
f835cb1
fix: add TooltipProvider to PublishTemplateVersionDialog
aqandrew Aug 28, 2025
dd7e233
fix: add TooltipProvider to TemplateHelpTooltip
aqandrew Aug 29, 2025
d397fff
fix: add TooltipProvider to WorkspaceHelpTooltip
aqandrew Aug 29, 2025
6134bad
fix: add TooltipProvider to WorkspaceOurdatedTooltip
aqandrew Aug 29, 2025
ad562be
fix: add TooltipProvider to SubAgentOutdatedTooltip
aqandrew Aug 29, 2025
d30170c
fix: add TooltipProvider to EditRolesButton
aqandrew Aug 29, 2025
4438af8
fix: add TooltipProvider to AutoCreateMissingGroupsHelpTooltip and
aqandrew Aug 29, 2025
00b70ea
refactor: extract TooltipProvder to definition of HelpTooltip
aqandrew Aug 29, 2025
c59daf3
refactor: remove extract TooltipProvider from DevContainerStartError
aqandrew Aug 29, 2025
3aeaa49
test: add PublishDTemplateVersionDialog story
aqandrew Aug 29, 2025
eaf3baf
chore: remove unused import
aqandrew Aug 29, 2025
c93e092
refactor: rename Tooltip to MUITooltip
aqandrew Aug 29, 2025
ef6cceb
refactor: replace Popover in OwnerBreadcrumb, WorkspaceBreadcrumb,
aqandrew Aug 29, 2025
c142b5a
chore: check:fix
aqandrew Aug 29, 2025
143e33e
Merge branch 'main' into aqandrew/replace-popover-in-helptooltip
aqandrew Aug 29, 2025
d22a040
chore: lint:fix
aqandrew Aug 29, 2025
2ff9e66
test: expect elements + text content rendered by HelpTooltip
aqandrew Aug 29, 2025
ed5bf98
Revert "refactor: rename Tooltip to MUITooltip"
aqandrew Aug 29, 2025
8f939f9
chore: remove TODO
aqandrew Sep 3, 2025
5050a92
docs: add date to disablePortal comment
aqandrew Sep 3, 2025
ded8411
chore: remove 1rem comment
aqandrew Sep 3, 2025
3466e90
refactor: rename to WorkspaceOutdatedTooltipProps
aqandrew Sep 3, 2025
3a8a403
refactor: export TooltipProps from Tooltip
aqandrew Sep 3, 2025
5f79acc
Merge branch 'main' into aqandrew/replace-popover-in-helptooltip
aqandrew Sep 3, 2025
b0696dd
refactor: rename HelpTooltipTrigger to HelpTooltipIconTrigger
aqandrew Sep 4, 2025
71c1824
refactor: rename HelpTooltipTriggerProps to HelpTooltipIconTriggerProps
aqandrew Sep 4, 2025
70e0dbf
refactor: use HelpTooltipTrigger inside of HelpTooltip
aqandrew Sep 4, 2025
1df04ec
Merge branch 'main' into aqandrew/replace-popover-in-helptooltip
aqandrew Sep 4, 2025
6733d6a
chore: rm TODO
aqandrew Sep 4, 2025
bc12f84
chore: check:fix
aqandrew Sep 4, 2025
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
Prev Previous commit
Next Next commit
refactor: extract TooltipProvder to definition of HelpTooltip
  • Loading branch information
@aqandrew
aqandrew committed Aug 29, 2025
commit 00b70ea6f2382bcf7d8e503f048ce843fb47621e
7 changes: 6 additions & 1 deletion site/src/components/HelpTooltip/HelpTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
Tooltip,
TooltipContent,
type TooltipContentProps,
TooltipProvider,
TooltipTrigger,
} from "components/Tooltip/Tooltip";
import { CircleHelpIcon, ExternalLinkIcon } from "lucide-react";
Expand All @@ -30,7 +31,11 @@ type Size = "small" | "medium";
export const HelpTooltipIcon = CircleHelpIcon;

export const HelpTooltip: FC<TooltipProps> = (props) => {
return <Tooltip delayDuration={0} {...props} />;
return (
<TooltipProvider>
<Tooltip delayDuration={0} {...props} />
Copy link
Member

Choose a reason for hiding this comment

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

I think 0 is probably too fast. what's the default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 700 which feels very long.

Worth mentioning that the MUI popover, which deprecated/Popover uses, doesn't use any delay, and I don't think we've added any:

coder/site/src/components/deprecated/Popover/Popover.tsx

Lines 130 to 132 in 2030907

const hoverProps = {
onPointerEnter: (event: PointerEvent<HTMLElement>) => {
popover.setOpen(true);

so I used a delay of 0 to keep consistency with our current UX

Copy link
Member

Choose a reason for hiding this comment

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

could we set it to like, 200? I feel like 0 is intense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, 0 is exactly as intense as prod is rn:

Screen.Recording.2025-09-04.at.4.05.34.PM.mov

I'm so used to 0 delay that 200 feels sluggish to me:

Screen.Recording.2025-09-04.at.4.03.46.PM.mov

</TooltipProvider>
);
};

export const HelpTooltipContent: FC<TooltipContentProps> = ({
Expand Down
78 changes: 38 additions & 40 deletions site/src/modules/resources/AgentLatency.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
HelpTooltipTitle,
} from "components/HelpTooltip/HelpTooltip";
import { Stack } from "components/Stack/Stack";
import { TooltipProvider, TooltipTrigger } from "components/Tooltip/Tooltip";
import { TooltipTrigger } from "components/Tooltip/Tooltip";
Copy link
Member

Choose a reason for hiding this comment

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

Stuff like this is making me worry we're not closing the loop on the abstraction all the way

I think my preference would be to have HelpTooltip export a separate HelpTooltipTrigger component, so that in the future, people don't need to go rifling around for the right component to get the help tooltip wired up. We could even make it so that HelpTooltip just exports TooltipTrigger under a different name – but I think that co-locating everything you need to wire up a HelpTooltip in one file is really important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tricky thing is that the HelpTooltipTrigger component already exported by HelpTooltip renders a button with an SVG icon:

coder/site/src/components/HelpTooltip/HelpTooltip.tsx

Line 64 in 3a8a403

export const HelpTooltipTrigger = forwardRef<

which is currently instantiated in 16 places like this:

coder/site/src/components/ActiveUserChart/ActiveUserChart.tsx

Line 119 in 62c7430

<HelpTooltipTrigger size="small" />

So my thought with using TooltipTrigger here is that you can insert whatever elements you want to act as a trigger (like in AgentLatency we're using a span instead of a button). Looking at it again, it is clunky.

What do you think about renaming the existing HelpTooltipTrigger to something like HelpTooltipIcon or HelpTooltipIconTrigger, so that we can export Radix's TooltipTrigger as HelpTooltipTrigger like you suggested?

Copy link
Member

Choose a reason for hiding this comment

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

I think I like the renaming approach better. I'm not even fully sure about having a version of the trigger that has an icon pre-baked, but I don't think it's all that egregious, and I imagine it'd be pretty easy to change it down the line

import type { FC } from "react";
import { getLatencyColor } from "utils/latency";

Expand Down Expand Up @@ -43,45 +43,43 @@ export const AgentLatency: FC<AgentLatencyProps> = ({ agent }) => {
}

return (
<TooltipProvider>
<HelpTooltip>
<TooltipTrigger asChild>
<span
role="presentation"
aria-label="latency"
css={{ cursor: "pointer", color: latency.color }}
>
{Math.round(latency.latency_ms)}ms
</span>
</TooltipTrigger>
<HelpTooltipContent>
<HelpTooltipTitle>Latency</HelpTooltipTitle>
<HelpTooltipText>
This is the latency overhead on non peer to peer connections. The
first row is the preferred relay.
</HelpTooltipText>
<Stack direction="column" spacing={1} css={{ marginTop: 16 }}>
{Object.entries(agent.latency)
.sort(([, a], [, b]) => a.latency_ms - b.latency_ms)
.map(([regionName, region]) => (
<Stack
direction="row"
key={regionName}
spacing={0.5}
justifyContent="space-between"
css={
region.preferred && {
color: theme.palette.text.primary,
}
<HelpTooltip>
<TooltipTrigger asChild>
<span
role="presentation"
aria-label="latency"
css={{ cursor: "pointer", color: latency.color }}
>
{Math.round(latency.latency_ms)}ms
</span>
</TooltipTrigger>
<HelpTooltipContent>
<HelpTooltipTitle>Latency</HelpTooltipTitle>
<HelpTooltipText>
This is the latency overhead on non peer to peer connections. The
first row is the preferred relay.
</HelpTooltipText>
<Stack direction="column" spacing={1} css={{ marginTop: 16 }}>
{Object.entries(agent.latency)
.sort(([, a], [, b]) => a.latency_ms - b.latency_ms)
.map(([regionName, region]) => (
<Stack
direction="row"
key={regionName}
spacing={0.5}
justifyContent="space-between"
css={
region.preferred && {
color: theme.palette.text.primary,
}
>
<strong>{regionName}</strong>
{Math.round(region.latency_ms)}ms
</Stack>
))}
</Stack>
</HelpTooltipContent>
</HelpTooltip>
</TooltipProvider>
}
>
<strong>{regionName}</strong>
{Math.round(region.latency_ms)}ms
</Stack>
))}
</Stack>
</HelpTooltipContent>
</HelpTooltip>
);
};
84 changes: 41 additions & 43 deletions site/src/modules/resources/AgentOutdatedTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Stack } from "components/Stack/Stack";
import { RotateCcwIcon } from "lucide-react";
import { useState, type FC } from "react";
import { agentVersionStatus } from "../../utils/workspace";
import { TooltipProvider, TooltipTrigger } from "components/Tooltip/Tooltip";
import { TooltipTrigger } from "components/Tooltip/Tooltip";

type AgentOutdatedTooltipProps = {
agent: WorkspaceAgent;
Expand Down Expand Up @@ -40,50 +40,48 @@ export const AgentOutdatedTooltip: FC<AgentOutdatedTooltipProps> = ({
const text = `${opener} This can happen after you update Coder with running workspaces. To fix this, you can stop and start the workspace.`;

return (
<TooltipProvider>
<HelpTooltip open={isOpen} onOpenChange={setIsOpen}>
<TooltipTrigger asChild>
<span role="status" className="cursor-pointer">
{status === agentVersionStatus.Outdated ? "Outdated" : "Deprecated"}
</span>
</TooltipTrigger>
<HelpTooltipContent>
<Stack spacing={1}>
<div>
<HelpTooltipTitle>{title}</HelpTooltipTitle>
<HelpTooltipText>{text}</HelpTooltipText>
</div>
<HelpTooltip open={isOpen} onOpenChange={setIsOpen}>
<TooltipTrigger asChild>
<span role="status" className="cursor-pointer">
{status === agentVersionStatus.Outdated ? "Outdated" : "Deprecated"}
</span>
</TooltipTrigger>
<HelpTooltipContent>
<Stack spacing={1}>
<div>
<HelpTooltipTitle>{title}</HelpTooltipTitle>
<HelpTooltipText>{text}</HelpTooltipText>
</div>

<Stack spacing={0.5}>
<span className="font-semibold text-content-primary">
Agent version
</span>
<span>{agent.version}</span>
</Stack>

<Stack spacing={0.5}>
<span className="font-semibold text-content-primary">
Server version
</span>
<span>{serverVersion}</span>
</Stack>
<Stack spacing={0.5}>
<span className="font-semibold text-content-primary">
Agent version
</span>
<span>{agent.version}</span>
</Stack>

<HelpTooltipLinksGroup>
<HelpTooltipAction
icon={RotateCcwIcon}
onClick={() => {
onUpdate();
// TODO can we move this tooltip-closing logic to the definition of HelpTooltipAction?
setIsOpen(false);
}}
ariaLabel="Update workspace"
>
Update workspace
</HelpTooltipAction>
</HelpTooltipLinksGroup>
<Stack spacing={0.5}>
<span className="font-semibold text-content-primary">
Server version
</span>
<span>{serverVersion}</span>
</Stack>
</HelpTooltipContent>
</HelpTooltip>
</TooltipProvider>

<HelpTooltipLinksGroup>
<HelpTooltipAction
icon={RotateCcwIcon}
onClick={() => {
onUpdate();
// TODO can we move this tooltip-closing logic to the definition of HelpTooltipAction?
setIsOpen(false);
}}
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to get some clarity on this comment: was this meant more in the sense of "I'm not sure if it's a good idea to do this", or "I feel like this is a good idea, but I'm not 100% sure how to do it"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit of both, but more the latter. I think whenever a HelpTooltipAction is clicked, we want to close the tooltip. By having the tooltip-closing logic built into HelpTooltipAction by default, we could write handlers like onClick={onUpdate} here + wherever else we want to execute a function when closing a tooltip.

Since Radix doesn't export a component like <Popover.Close />, it seems the only way to do this is with a state hook. Is it a good idea to make every tooltip a controlled component? I'm not sure

Copy link
Member

@Parkreiner Parkreiner Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, looking at Radix's API docs, I don't see anything that gives direct access to the state that's being contained inside Popover.Root

I imagine that if we want to bake that behavior in, we'd basically need to make wrappers over all the Popover components, and then wire up our own custom context (on top of the one already used by Radix) to control the closing state. I don't think that's too bad, but I also feel like Radix could've easily given a way to trigger that behavior if they felt like it was a common enough usage pattern

Unless we have a bunch of other components wiring that behavior up already, I feel like we can leave things be and wait for more examples to emerge

ariaLabel="Update workspace"
>
Update workspace
</HelpTooltipAction>
</HelpTooltipLinksGroup>
</Stack>
</HelpTooltipContent>
</HelpTooltip>
);
};
30 changes: 14 additions & 16 deletions site/src/modules/resources/AgentStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,22 +266,20 @@ const TimeoutStatus: FC<AgentStatusProps> = ({ agent }) => {

export const AgentStatus: FC<AgentStatusProps> = ({ agent }) => {
return (
<TooltipProvider>
<ChooseOne>
<Cond condition={agent.status === "connected"}>
<ConnectedStatus agent={agent} />
</Cond>
<Cond condition={agent.status === "disconnected"}>
<DisconnectedStatus />
</Cond>
<Cond condition={agent.status === "timeout"}>
<TimeoutStatus agent={agent} />
</Cond>
<Cond>
<ConnectingStatus />
</Cond>
</ChooseOne>
</TooltipProvider>
<ChooseOne>
<Cond condition={agent.status === "connected"}>
<ConnectedStatus agent={agent} />
</Cond>
<Cond condition={agent.status === "disconnected"}>
<DisconnectedStatus />
</Cond>
<Cond condition={agent.status === "timeout"}>
<TimeoutStatus agent={agent} />
</Cond>
<Cond>
<ConnectingStatus />
</Cond>
</ChooseOne>
);
};

Expand Down
51 changes: 24 additions & 27 deletions site/src/modules/resources/SubAgentOutdatedTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
HelpTooltipTrigger,
} from "components/HelpTooltip/HelpTooltip";
import { Stack } from "components/Stack/Stack";
import { TooltipProvider } from "components/Tooltip/Tooltip";
import { RotateCcwIcon } from "lucide-react";
import type { FC } from "react";

Expand All @@ -39,32 +38,30 @@ export const SubAgentOutdatedTooltip: FC<SubAgentOutdatedTooltipProps> = ({
const text = `${opener} This can happen if you modify your devcontainer.json file after the Dev Container has been created. To fix this, you can rebuild the Dev Container.`;

return (
<TooltipProvider>
<HelpTooltip>
<HelpTooltipTrigger>
<span role="status" className="cursor-pointer">
Outdated
</span>
</HelpTooltipTrigger>
<HelpTooltipContent>
<Stack spacing={1}>
<div>
<HelpTooltipTitle>{title}</HelpTooltipTitle>
<HelpTooltipText>{text}</HelpTooltipText>
</div>
<HelpTooltip>
<HelpTooltipTrigger>
<span role="status" className="cursor-pointer">
Outdated
</span>
</HelpTooltipTrigger>
<HelpTooltipContent>
<Stack spacing={1}>
<div>
<HelpTooltipTitle>{title}</HelpTooltipTitle>
<HelpTooltipText>{text}</HelpTooltipText>
</div>

<HelpTooltipLinksGroup>
<HelpTooltipAction
icon={RotateCcwIcon}
onClick={onUpdate}
ariaLabel="Rebuild Dev Container"
>
Rebuild Dev Container
</HelpTooltipAction>
</HelpTooltipLinksGroup>
</Stack>
</HelpTooltipContent>
</HelpTooltip>
</TooltipProvider>
<HelpTooltipLinksGroup>
<HelpTooltipAction
icon={RotateCcwIcon}
onClick={onUpdate}
ariaLabel="Rebuild Dev Container"
>
Rebuild Dev Container
</HelpTooltipAction>
</HelpTooltipLinksGroup>
</Stack>
</HelpTooltipContent>
</HelpTooltip>
);
};
17 changes: 7 additions & 10 deletions site/src/modules/workspaces/WorkspaceOutdatedTooltip/WorkspaceOutdatedTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
useWorkspaceUpdate,
WorkspaceUpdateDialogs,
} from "../WorkspaceUpdateDialogs";
import { TooltipProvider } from "components/Tooltip/Tooltip";

interface TooltipProps {
workspace: Workspace;
Expand All @@ -32,15 +31,13 @@ export const WorkspaceOutdatedTooltip: FC<TooltipProps> = (props) => {
const [isOpen, setIsOpen] = useState(false);

return (
<TooltipProvider>
<HelpTooltip open={isOpen} onOpenChange={setIsOpen}>
<HelpTooltipTrigger size="small" hoverEffect={false}>
<InfoIcon css={styles.icon} />
<span className="sr-only">Outdated info</span>
</HelpTooltipTrigger>
<WorkspaceOutdatedTooltipContent isOpen={isOpen} {...props} />
</HelpTooltip>
</TooltipProvider>
<HelpTooltip open={isOpen} onOpenChange={setIsOpen}>
<HelpTooltipTrigger size="small" hoverEffect={false}>
<InfoIcon css={styles.icon} />
<span className="sr-only">Outdated info</span>
</HelpTooltipTrigger>
<WorkspaceOutdatedTooltipContent isOpen={isOpen} {...props} />
</HelpTooltip>
);
};

Expand Down
27 changes: 12 additions & 15 deletions site/src/pages/AuditPage/AuditHelpTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
HelpTooltipTitle,
HelpTooltipTrigger,
} from "components/HelpTooltip/HelpTooltip";
import { TooltipProvider } from "components/Tooltip/Tooltip";
import type { FC } from "react";
import { docs } from "utils/docs";

Expand All @@ -19,20 +18,18 @@ const Language = {

export const AuditHelpTooltip: FC = () => {
return (
<TooltipProvider>
<HelpTooltip>
<HelpTooltipTrigger />
<HelpTooltip>
<HelpTooltipTrigger />

<HelpTooltipContent>
<HelpTooltipTitle>{Language.title}</HelpTooltipTitle>
<HelpTooltipText>{Language.body}</HelpTooltipText>
<HelpTooltipLinksGroup>
<HelpTooltipLink href={docs("/admin/security/audit-logs")}>
{Language.docs}
</HelpTooltipLink>
</HelpTooltipLinksGroup>
</HelpTooltipContent>
</HelpTooltip>
</TooltipProvider>
<HelpTooltipContent>
<HelpTooltipTitle>{Language.title}</HelpTooltipTitle>
<HelpTooltipText>{Language.body}</HelpTooltipText>
<HelpTooltipLinksGroup>
<HelpTooltipLink href={docs("/admin/security/audit-logs")}>
{Language.docs}
</HelpTooltipLink>
</HelpTooltipLinksGroup>
</HelpTooltipContent>
</HelpTooltip>
);
};
Loading