Skip to content

Replace react-tooltip with AppTooltip in PullRequestItem#17613

Open
bohdansolovie wants to merge 2 commits intotwentyhq:mainfrom
bohdansolovie:replace-react-tooltip-with-apptooltip
Open

Replace react-tooltip with AppTooltip in PullRequestItem#17613
bohdansolovie wants to merge 2 commits intotwentyhq:mainfrom
bohdansolovie:replace-react-tooltip-with-apptooltip

Conversation

@bohdansolovie
Copy link

Summary

Replaces react-tooltip with AppTooltip from twenty-ui/display in the PullRequestItem component.

Changes

  • ✅ Removed react-tooltip import
  • ✅ Removed unused StyledTooltip styled component
  • ✅ Added AppTooltip import from twenty-ui/display
  • ✅ Updated component to use AppTooltip

Motivation

This addresses the TODO comment in the file and ensures consistency by using the shared UI component from twenty-ui instead of directly importing from react-tooltip.

AppTooltip is a styled wrapper around react-tooltip that provides consistent theming and styling across the application.

- Remove react-tooltip import and unused StyledTooltip component
- Use AppTooltip from twenty-ui/display for consistency
- Addresses TODO comment in PullRequestItem.tsx
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2026

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

TODOs/FIXMEs:

  • // TODO: use twenty-ui Tooltip: packages/twenty-website/src/app/_components/contributors/PullRequestItem.tsx

Generated by 🚫 dangerJS against 8eac08c

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 2, 2026

Greptile Overview

Greptile Summary

This PR updates packages/twenty-website/src/app/_components/contributors/PullRequestItem.tsx to stop importing react-tooltip directly and instead use the shared AppTooltip component from twenty-ui/display for the date tooltip.

The change centralizes tooltip styling/theming via twenty-ui, but the tooltip anchor is still selected via a DOM id derived only from prNumber (parsed from the PR URL). If duplicate PR numbers can appear in the same rendered page, the anchor id/anchorSelect pairing will collide and the tooltip may attach to the wrong element.

Confidence Score: 4/5

  • Generally safe to merge once the tooltip anchor uniqueness concern is addressed/confirmed.
  • The change is small and localized (tooltip wrapper swap), but the current anchorSelect targets an id derived only from prNumber, which can break tooltip targeting if duplicate values render on the same page.
  • packages/twenty-website/src/app/_components/contributors/PullRequestItem.tsx

Important Files Changed

Filename Overview
packages/twenty-website/src/app/_components/contributors/PullRequestItem.tsx Replaces react-tooltip Tooltip/styled wrapper with AppTooltip from twenty-ui/display for date tooltip rendering; need to verify anchorSelect id uniqueness in list contexts and ensure AppTooltip is compatible with Next.js rendering for this component.

Sequence Diagram

sequenceDiagram
  participant PRList as PullRequests (list)
  participant Item as PullRequestItem
  participant Anchor as span#date-<key>
  participant Tooltip as AppTooltip

  PRList->>Item: render(props: {id,url,createdAt,mergedAt,...})
  Item->>Item: prNumber = last segment of url
  Item->>Anchor: render id="date-${prNumber}" with relative date
  Item->>Tooltip: render AppTooltip(anchorSelect="#date-${prNumber}")
  Tooltip->>Anchor: querySelector(anchorSelect)
  Tooltip-->>PRList: tooltip anchored to matched element
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bohdansolovie
Copy link
Author

Hello, @charlesBochet
Can you review this PR please?

Thanks

@charlesBochet
Copy link
Member

@bohdansolovie Have you tried it?

image

Closing...

@bohdansolovie
Copy link
Author

bohdansolovie commented Feb 9, 2026

Hi, @charlesBochet
I tested it. I think this is an edge case.
Can you reopen this PR so that I can fix it?

@charlesBochet charlesBochet reopened this Feb 10, 2026
@charlesBochet
Copy link
Member

charlesBochet commented Feb 10, 2026

Ok, please add a screenshot in this case :)
To give you some context, we are getting a lot if IA PRs these days without people actually taking the time to think about it and to test it.
As a result, the core team is losing time to help people that are not really trying to help, which is not helping

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 94 to 98
<span id={`date-${prNumber}`}>
{formatIntoRelativeDate(mergedAt ? mergedAt : createdAt)}
</span>
<StyledTooltip
<AppTooltip
anchorSelect={`#date-${prNumber}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tooltip anchor id collision

id={date-${prNumber}} is only derived from the PR number, so if this component is ever rendered with duplicate prNumber values on the same page, the HTML id will be duplicated and anchorSelect={#date-${prNumber}} will bind to the first matching element (wrong tooltip target). Consider including a guaranteed-unique value (e.g. id prop) in both the id and anchorSelect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants