tooltips for tab bar buttons (AI and workspace switcher)#2914
Conversation
WalkthroughAdds two public Tooltip props: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
frontend/app/element/tooltip.tsx
Outdated
| ref={(node) => { | ||
| refs.setReference(node); | ||
| if (divRef) { | ||
| (divRef as React.RefObject<HTMLDivElement>).current = node; |
There was a problem hiding this comment.
CRITICAL: Invalid ref assignment - RefObject.current is readonly
The code attempts to assign to divRef.current, but React.RefObject<T>.current is a readonly property. This violates TypeScript's type system and React's ref contract.
| (divRef as React.RefObject<HTMLDivElement>).current = node; | |
| if (divRef && 'current' in divRef) { | |
| (divRef as React.MutableRefObject<HTMLDivElement>).current = node; | |
| } |
Alternatively, consider using a callback ref pattern or checking if the ref is mutable before assignment.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (2 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/element/tooltip.tsx`:
- Around line 140-151: The JSX currently spreads getReferenceProps() and then
separately supplies onClick and onPointerEnter, which overrides Floating UI's
handlers; update the reference element to call getReferenceProps with the custom
handlers (e.g., getReferenceProps({ onClick: handleClick, onPointerEnter:
handlePointerEnter, ref: ... })) so Floating UI can merge them instead of being
replaced; keep the existing refs logic (refs.setReference and divRef assignment)
but pass any event handlers and ref through the argument to getReferenceProps so
handlers are combined correctly for the reference element.
Deploying waveterm with
|
| Latest commit: |
a6f0fca
|
| Status: | 🚫 Build failed. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
frontend/app/element/tooltip.tsx (2)
147-151:⚠️ Potential issue | 🔴 Critical
onClick/onPointerEnterplaced after the spread override Floating UI's handlers — hover is completely broken.In JSX, later props shadow earlier ones.
onPointerEnter={handlePointerEnter}replacesuseHover'sonPointerEnterentirely, so the tooltip never opens on hover. Both handlers must be merged throughgetReferenceProps(...).🛠️ Proposed fix
- {...getReferenceProps()} - className={divClassName} - style={divStyle} - onClick={handleClick} - onPointerEnter={handlePointerEnter} + {...getReferenceProps({ + onClick: handleClick, + onPointerEnter: handlePointerEnter, + })} + className={divClassName} + style={divStyle}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/element/tooltip.tsx` around lines 147 - 151, The custom onClick and onPointerEnter are currently shadowing Floating UI's handlers because they are set after the spread of getReferenceProps(); fix by passing these handlers into getReferenceProps so they are merged with Floating UI's handlers (e.g., call getReferenceProps({ onClick: handleClick, onPointerEnter: handlePointerEnter }) and remove the standalone onClick/onPointerEnter props), keeping divClassName and divStyle as regular props so the reference element still receives merged event handlers from useHover/getReferenceProps.
141-146:⚠️ Potential issue | 🔴 Critical
divRef.currentassignment violates TypeScript'sRefObjectreadonly constraint.
React.RefObject<T>.currentis readonly; writingdivRef.current = nodedirectly is a TypeScript type error. Cast toMutableRefObjector rely on React's built-in ref merging.🛠️ Proposed fix
- ref={(node) => { - refs.setReference(node); - if (divRef) { - divRef.current = node; - } - }} + ref={(node) => { + refs.setReference(node); + if (divRef) { + (divRef as React.MutableRefObject<HTMLDivElement | null>).current = node; + } + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/element/tooltip.tsx` around lines 141 - 146, The assignment divRef.current = node violates React.RefObject's readonly type; update the ref handling in the ref callback used with refs.setReference by either casting divRef to React.MutableRefObject before assigning or by using React's ref callback pattern (invoke divRef as a function if it's a function ref) instead of direct write; locate the ref callback where refs.setReference(node) is called and change the divRef handling to (divRef as MutableRefObject).current = node or conditional-call divRef(node) to satisfy TypeScript while preserving the existing refs.setReference usage.
🧹 Nitpick comments (1)
frontend/app/element/tooltip.tsx (1)
132-136:clickDisabledinhandlePointerEnter's dependency array is unnecessary.
setClickDisabled(false)is idempotent when the state is alreadyfalse, so the guardif (hideOnClick && clickDisabled)adds no value and forces the callback to be recreated on every state flip. DroppingclickDisabledfrom the deps simplifies the logic and avoids a stale-closure risk.♻️ Proposed refactor
- const handlePointerEnter = useCallback(() => { - if (hideOnClick && clickDisabled) { - setClickDisabled(false); - } - }, [hideOnClick, clickDisabled]); + const handlePointerEnter = useCallback(() => { + if (hideOnClick) { + setClickDisabled(false); + } + }, [hideOnClick]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/element/tooltip.tsx` around lines 132 - 136, The handlePointerEnter callback currently includes clickDisabled in its dependency array and guards setClickDisabled(false) with clickDisabled; remove clickDisabled from the deps to avoid needless re-creations and stale-closure risk and simplify the logic in handlePointerEnter to only check hideOnClick before calling setClickDisabled(false). Locate the handlePointerEnter useCallback and the setClickDisabled state updater (and the clickDisabled state variable) and update the dependency array to [hideOnClick] and the body to only call setClickDisabled(false) when hideOnClick is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/app/element/tooltip.tsx`:
- Around line 147-151: The custom onClick and onPointerEnter are currently
shadowing Floating UI's handlers because they are set after the spread of
getReferenceProps(); fix by passing these handlers into getReferenceProps so
they are merged with Floating UI's handlers (e.g., call getReferenceProps({
onClick: handleClick, onPointerEnter: handlePointerEnter }) and remove the
standalone onClick/onPointerEnter props), keeping divClassName and divStyle as
regular props so the reference element still receives merged event handlers from
useHover/getReferenceProps.
- Around line 141-146: The assignment divRef.current = node violates
React.RefObject's readonly type; update the ref handling in the ref callback
used with refs.setReference by either casting divRef to React.MutableRefObject
before assigning or by using React's ref callback pattern (invoke divRef as a
function if it's a function ref) instead of direct write; locate the ref
callback where refs.setReference(node) is called and change the divRef handling
to (divRef as MutableRefObject).current = node or conditional-call divRef(node)
to satisfy TypeScript while preserving the existing refs.setReference usage.
---
Nitpick comments:
In `@frontend/app/element/tooltip.tsx`:
- Around line 132-136: The handlePointerEnter callback currently includes
clickDisabled in its dependency array and guards setClickDisabled(false) with
clickDisabled; remove clickDisabled from the deps to avoid needless re-creations
and stale-closure risk and simplify the logic in handlePointerEnter to only
check hideOnClick before calling setClickDisabled(false). Locate the
handlePointerEnter useCallback and the setClickDisabled state updater (and the
clickDisabled state variable) and update the dependency array to [hideOnClick]
and the body to only call setClickDisabled(false) when hideOnClick is true.
| divRef={workspaceSwitcherRef} | ||
| divClassName="flex items-center h-full" | ||
| > | ||
| <WorkspaceSwitcher /> |
There was a problem hiding this comment.
WARNING: Ref forwarding issue with WorkspaceSwitcher wrapper
The WorkspaceSwitcher component uses forwardRef to forward refs to its internal Popover component (line 98 in workspaceswitcher.tsx). However, wrapping it in a Tooltip creates an extra div wrapper, so workspaceSwitcherRef will point to the Tooltip's wrapper div instead of the WorkspaceSwitcher's root Popover element.
This breaks the width calculation on line 259: workspaceSwitcherRef.current?.getBoundingClientRect().width will measure the Tooltip wrapper instead of the actual WorkspaceSwitcher button.
Recommendation: Either:
- Don't wrap WorkspaceSwitcher in a Tooltip (add tooltip directly to WorkspaceSwitcher component)
- Or pass the ref directly to WorkspaceSwitcher and let it handle the ref forwarding:
<WorkspaceSwitcher ref={workspaceSwitcherRef} />
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/app/element/tooltip.tsx (1)
27-28:⚠️ Potential issue | 🔴 CriticalFix readonly
RefObject.currentassignment.Line 141-145 assigns to
divRef.current, butReact.RefObject<T>.currentis readonly in TypeScript. This will fail type-checking and is a build blocker.🛠️ Suggested fix
- divRef?: React.RefObject<HTMLDivElement>; + divRef?: React.Ref<HTMLDivElement>; ... - ref={(node) => { - refs.setReference(node); - if (divRef) { - divRef.current = node; - } - }} + ref={(node) => { + refs.setReference(node); + if (typeof divRef === "function") { + divRef(node); + } else if (divRef) { + (divRef as React.MutableRefObject<HTMLDivElement | null>).current = node; + } + }}Please confirm with a local TypeScript typecheck (e.g.,
tsc --noEmit) that there are no readonly ref assignment errors after the change.Also applies to: 141-147, 190-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/element/tooltip.tsx` around lines 27 - 28, The component assigns to divRef.current but RefObject.current is readonly; change the prop type for divRef to a mutable ref (e.g., React.MutableRefObject<HTMLDivElement | null>) or accept a callback ref and update all usages in the component (places referencing divRef/current around the divRef assignment blocks and the other occurrences noted) so you can safely set .current after null-checking, update any callers to pass a mutable ref or callback ref, and run a local TypeScript typecheck (tsc --noEmit) to confirm no readonly ref assignment errors remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/app/element/tooltip.tsx`:
- Around line 27-28: The component assigns to divRef.current but
RefObject.current is readonly; change the prop type for divRef to a mutable ref
(e.g., React.MutableRefObject<HTMLDivElement | null>) or accept a callback ref
and update all usages in the component (places referencing divRef/current around
the divRef assignment blocks and the other occurrences noted) so you can safely
set .current after null-checking, update any callers to pass a mutable ref or
callback ref, and run a local TypeScript typecheck (tsc --noEmit) to confirm no
readonly ref assignment errors remain.
1 participant
No description provided.