reduce TooltipProviders components on pages#2801
Conversation
🦋 Changeset detectedLatest commit: a44fb8c The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Removes redundant per-tooltip Key changes
Summary | 11 files | 3 commits | base: Deduplicate
|
There was a problem hiding this comment.
Clean refactor — the approach is correct since SidebarProvider (sidebar.tsx:122) already wraps all tenant-scoped page content in a <TooltipProvider delayDuration={0}>, making the per-component wrappers redundant. The tooltip.tsx change to remove the self-wrapping TooltipProvider from Tooltip is also good since it was double-nesting providers.
One issue: all 8 consumer files still import TooltipProvider despite no longer using it in JSX. This will fail the knip unused-exports check in CI.
| > | ||
| <X className="size-3" /> | ||
| </button> | ||
| {selectedMembers.map((member) => ( |
There was a problem hiding this comment.
Unused import: TooltipProvider is still in the import statement (line 8) but no longer used in JSX. Same issue in all 7 other changed files:
error-indicator.tsx:4nango-providers-grid.tsx:14project-scheduled-triggers-table.tsx:29project-triggers-table.tsx:20chart-card.tsx:13inheritance-indicator.tsx:5slack-dashboard.tsx:9
Remove TooltipProvider from the import in each file.
| <TooltipPrimitive.Root data-slot="tooltip" {...props} /> | ||
| </TooltipProvider> | ||
| ); | ||
| return <TooltipPrimitive.Root data-slot="tooltip" {...props} />; |
There was a problem hiding this comment.
With this change, Tooltip no longer self-provides a TooltipProvider. This works because all tenant-scoped pages are wrapped by the TooltipProvider inside SidebarProvider (sidebar.tsx:122). Worth noting that any future tooltip usage outside the [tenantId] layout (e.g. login, error pages) would need its own provider — but currently none of those pages use tooltips, so this is fine.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR correctly deduplicates TooltipProvider wrappers across the codebase. The approach is sound:
-
Root cause addressed: The
Tooltipcomponent was self-wrapping withTooltipProvider, and consumer components were also wrapping with their ownTooltipProvider— creating unnecessary nesting (up to 3 providers per tooltip). -
Single source of truth: All tenant-scoped pages are now served by the
TooltipProviderinsidebar.tsx:122withdelayDuration={0}. -
Prior feedback resolved: The unused
TooltipProviderimports flagged in the first commit were cleaned up in thelint fixcommit.
💭 Consider (1) 💭
💭 1) scope Document the TooltipProvider requirement for non-sidebar pages
Issue: Pages outside the [tenantId] layout (e.g., /login, /forgot-password, /device) are not wrapped by SidebarProvider and therefore have no TooltipProvider. Currently none of these pages use tooltips, but future additions would silently fail.
Why: This is a latent footgun — tooltips added to public pages would render without content and no error would surface.
Fix: Consider adding a brief comment in tooltip.tsx near the Tooltip export noting that consumers must be within a TooltipProvider (typically via SidebarProvider), or add a TooltipProvider to the root app/layout.tsx to cover all pages.
Refs:
✅ APPROVE
Summary: Clean refactor that eliminates redundant React context nesting. The approach is correct and the implementation is complete. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 1 | 0 | 0 | 0 | 0 |
Note: Lightweight refactor — no sub-reviewers dispatched. Prior run's inline comments (8 Critical: unused imports) are now outdated after the lint fix commit.
agents-manage-ui/src/components/credentials/views/nango-providers-grid.tsx
Show resolved
Hide resolved
agents-manage-ui/src/components/project-triggers/project-scheduled-triggers-table.tsx
Show resolved
Hide resolved
agents-manage-ui/src/components/project-triggers/project-triggers-table.tsx
Show resolved
Hide resolved
agents-manage-ui/src/features/work-apps/slack/components/slack-dashboard.tsx
Show resolved
Hide resolved
|
@claude add a changeset based on changes in this PR. do we have similar performance issues with other components maybe? |
|
Claude finished @dimaMachina's task in 2m 21s —— View job Tasks
Changeset addedPushed Similar provider patterns analysisI searched the entire No other redundant provider issues exist. The other Radix primitives used in this codebase (Dialog, Select, DropdownMenu, Popover, etc.) don't have a "Provider" concept — they work per-instance by design, so the same class of problem doesn't apply to them. One thing worth noting about the remaining | |
Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ito Test Report ❌15 test cases ran. 3 failed, 12 passed. Overall, 15 QA test cases were executed with 12 passing and 3 failing, indicating broad stability across profile, members, triggers, providers, Slack dashboard query handling, agent-editor tooltip semantics, mobile viewport behavior, and injection-resilience checks. The most important issues were one high-severity authorization defect where non-admin users can access trigger-creation entry points, plus two minor but real regressions where expected info tooltips are missing on the stats and traces “Conversations per day” chart cards because the required tooltip prop is not passed. ❌ Failed (3)
|
















closes #2786 (comment)