Skip to content

fix(ui): forward refs through ui primitives and fail tests on swallowed refs - #32401

Merged
ryan-crabbe-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_ui_forwardref_react18
Jul 9, 2026
Merged

fix(ui): forward refs through ui primitives and fail tests on swallowed refs#32401
ryan-crabbe-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_ui_forwardref_react18

Conversation

@ryan-crabbe-berri

@ryan-crabbe-berri ryan-crabbe-berri commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

Screenshots / Proof of Fix

This fixes a latent composition bug with no user-visible surface on staging yet, so the proof is the failing-before/passing-after behavior of the new tests

Before (staging db24027): passing a ref to any of these primitives leaves ref.current null and React only logs a dev console warning; a Base UI <TooltipTrigger render={<Skeleton/>}> (or any render-prop trigger over them) silently never opens. Running the new src/components/ui/ref-forwarding.test.tsx at that commit fails 6 of 8 cases (expected null to be an instance of HTMLElement); only Button and Input pass since they already forwarded refs

After (762ddb1): cd ui/litellm-dashboard && npx vitest run src/components/ui passes 24/24, including one contract case per primitive asserting ref.current is the real DOM node, and a self-test proving the setupTests tripwire records the React warning. PR #32393 contains the sibling fix for ui/badge.tsx where this class of bug was first hit for real (the Virtual Keys blocked-key tooltip never opened until Badge forwarded refs); the hover-opens-tooltip proof lives there

Type

🐛 Bug Fix
✅ Test

Changes

Under React 18.3 a ref passed to a plain function component is dropped; React logs a dev-only console warning and the ref stays null. Our components/ui primitives were generated in the React 19 shadcn style (plain functions, ref-as-prop), so any ref-based composition over them silently breaks: Base UI render-prop triggers, antd's cloneElement children, or a plain useRef for focus or measurement. ui/badge.tsx hit this for real on the shared DataTable track; only button.tsx and input.tsx already forwarded refs

Three changes. Label, Separator, Skeleton, UiLoadingSpinner and the Table family (Table, TableHeader, TableBody, TableFooter, TableRow, TableHead, TableCell, TableCaption) now use React.forwardRef and deliver the ref to their root DOM node. A contract test (ref-forwarding.test.tsx) renders each primitive with a ref and asserts the DOM node arrives, so a regression to plain-function style fails immediately. tests/setupTests.ts records React's "Function components cannot be given refs" warning and fails the offending test in afterEach with the captured warning text appended, so the failure names the component that swallowed the ref instead of pointing at console output. This turns the whole class from silent-in-prod into loud-in-CI, covering primitives this PR did not touch and any future npx shadcn add output (which will keep arriving in React 19 style until the React 19 upgrade)

The tripwire fails in afterEach rather than throwing from the console.error hook because a mid-render throw is caught by React's error recovery and the warning deduplicates on the retry, which silently defeats the guard; a self-test pins the recording path so the setup block cannot be deleted without a test failure

The Base UI wrapper components (dialog, popover, select, tooltip and friends) are deliberately left alone; they are used as direct JSX rather than render targets, the tripwire now covers them if that changes, and AntDLoadingSpinner wraps an antd Spin that accepts no ref. Everything here is inert once React 19 lands (forwardRef keeps working; the tripwire simply never fires) and can be codemodded away then

…ed refs

Under React 18 a ref passed to a plain function component is dropped
with only a dev console warning, so Base UI render-prop triggers
composed over our shadcn-style primitives silently stop working (the
tooltip just never opens; ui/badge.tsx hit exactly this on the shared
DataTable branch). Label, Separator, Skeleton, UiLoadingSpinner and the
Table family now use React.forwardRef like Button and Input already
did, a contract test pins ref delivery for each, and setupTests turns
React's ref warning into a test failure so the next primitive that
swallows a ref fails CI instead of shipping a dead tooltip
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a latent ref-forwarding bug in UI primitive components: under React 18.3, refs passed to plain function components are silently dropped, breaking ref-based composition (Base UI render-prop triggers, focus management, measurement). Label, Separator, Skeleton, UiLoadingSpinner, and the full Table family are each converted to React.forwardRef with displayName set.

  • ref-forwarding.test.tsx adds one contract test per primitive asserting ref.current arrives as the correct DOM node type, immediately catching any future regression to plain-function style.
  • tests/setupTests.ts installs a module-level console.error spy that captures React's "Function components cannot be given refs" warning and throws in afterEach with the full warning text (including the component name) embedded in the error — turning silent-in-prod into loud-in-CI for any future npx shadcn add output not yet converted.

Confidence Score: 5/5

Safe to merge — all changes are additive ref-forwarding wrappers over existing components with no behavioral change at runtime, and every modified primitive is covered by a new contract test.

Each conversion is mechanical and correct: hooks inside UiLoadingSpinner remain inside the render callback, ref types match the root DOM element for every component, and the Base UI Separator correctly uses ComponentRef to derive the ref type from the wrapped primitive. The tripwire in setupTests drains warnings before afterEach could double-fire, and the self-test pins that path so the setup block cannot be silently removed. No existing test coverage is weakened and no runtime behavior changes.

No files require special attention.

Important Files Changed

Filename Overview
ui/litellm-dashboard/src/components/ui/label.tsx Converts Label from a plain function to React.forwardRef, forwarding the ref to the root element. Uses ComponentPropsWithoutRef, adds displayName. Mechanically correct.
ui/litellm-dashboard/src/components/ui/separator.tsx Wraps Separator in React.forwardRef using ComponentRef for correct ref typing. Ref is forwarded to the Base UI SeparatorPrimitive which handles its own DOM delegation. Correct.
ui/litellm-dashboard/src/components/ui/skeleton.tsx Converts Skeleton to React.forwardRef forwarding to the root
. Clean, minimal change with displayName added.
ui/litellm-dashboard/src/components/ui/table.tsx All 8 table components converted to React.forwardRef with correct element types and displayName. Each ref is threaded to the correct DOM element.
ui/litellm-dashboard/src/components/ui/ui-loading-spinner.tsx Wraps UiLoadingSpinner in React.forwardRef; hooks (useId, useSafeLayoutEffect) are correctly placed inside the render function body. Ref forwarded to the root . displayName added.
ui/litellm-dashboard/src/components/ui/ref-forwarding.test.tsx New contract tests asserting each primitive delivers its ref to the correct DOM node. Includes a self-test that verifies the setupTests tripwire correctly records a warning from a plain function component.
ui/litellm-dashboard/tests/setupTests.ts Adds a module-level console.error spy that captures React's 'Function components cannot be given refs' warning into pendingRefWarnings. afterEach drains and throws with the full warning text included, surfacing the offending component name in CI output.

Reviews (2): Last reviewed commit: "fix(ui): include captured ref warnings i..." | Re-trigger Greptile

Comment thread ui/litellm-dashboard/tests/setupTests.ts Outdated
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed-hq

codspeed-hq Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_ui_forwardref_react18 (c307f4a) with litellm_internal_staging (131aa05)1

Open in CodSpeed

Footnotes

  1. No successful run was found on litellm_internal_staging (0099c6b) during the generation of this report, so 131aa05 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

The afterEach tripwire threw a fixed message and discarded the collected
React warnings, so a failure never said which component swallowed the ref.
Append the captured warnings (component name + stack) to the thrown error.
@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

@ryan-crabbe-berri
ryan-crabbe-berri merged commit 7d63b86 into litellm_internal_staging Jul 9, 2026
126 checks passed
@ryan-crabbe-berri
ryan-crabbe-berri deleted the litellm_ui_forwardref_react18 branch July 9, 2026 18:59
edelauna pushed a commit to edelauna/litellm that referenced this pull request Jul 22, 2026
…ed refs (BerriAI#32401)

* fix(ui): forward refs through ui primitives and fail tests on swallowed refs

Under React 18 a ref passed to a plain function component is dropped
with only a dev console warning, so Base UI render-prop triggers
composed over our shadcn-style primitives silently stop working (the
tooltip just never opens; ui/badge.tsx hit exactly this on the shared
DataTable branch). Label, Separator, Skeleton, UiLoadingSpinner and the
Table family now use React.forwardRef like Button and Input already
did, a contract test pins ref delivery for each, and setupTests turns
React's ref warning into a test failure so the next primitive that
swallows a ref fails CI instead of shipping a dead tooltip

* fix(ui): include captured ref warnings in the tripwire error

The afterEach tripwire threw a fixed message and discarded the collected
React warnings, so a failure never said which component swallowed the ref.
Append the captured warnings (component name + stack) to the thrown error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants