Feature/tooltip system clean#2973
Conversation
📝 WalkthroughWalkthroughAdds tooltip metadata to widgets, manages hover and focus tooltip state in ChangesTooltip system
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant TooltipManager
participant TooltipOverlay
participant Text
App->>TooltipManager: open resolved tooltip text and position
TooltipManager-->>TooltipOverlay: emit tooltip state change
TooltipOverlay->>Text: create or update tooltip text
TooltipOverlay-->>Text: render clamped rectangle
App->>TooltipManager: close tooltip on widget change or blur
TooltipManager-->>TooltipOverlay: emit null state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/ui/src/TooltipOverlay.ts (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyon fallback colors.
this.style.fg/bgare typed asColor | undefined, so this can assignstringtoText/styleToCellAttrs. Use the sharedColorfallback type (for example{ type: 'named', name: 'black' }/{ type: 'named', name: 'yellow' }) or normalize the fallback toColorconsistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/TooltipOverlay.ts` around lines 34 - 38, Update the Text construction in TooltipOverlay’s _textWidget initialization to remove the any casts and ensure both fg and bg fallbacks are valid Color values. Use the shared named-color fallback representation consistently while preserving the existing black and yellow defaults.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/app/App.ts`:
- Around line 711-728: The focused-widget tooltip logic in the event handler
currently runs before _widgetById is available and is lost during pending-state
replay. Update the changed === null replay path to resolve the focused widget
from _widgetById, walk its parent chain for tooltip text, and open the tooltip
using the widget rectangle once the map is built, while preserving the existing
focus styling behavior.
- Around line 311-313: Update the hover timer handling in App so a pending
tooltip hover cannot overwrite a tooltip opened by focus: clear
_tooltipHoverTimer before focus-triggered opens, clear it during teardown, and
set the handle to null inside the timer callback before opening. Apply the same
behavior to the related logic around the additional referenced section,
preserving existing tooltip behavior otherwise.
In `@packages/ui/src/TooltipOverlay.ts`:
- Around line 63-66: Update the text-widget cleanup block in TooltipOverlay to
call removeChild(this._textWidget) instead of filtering _children directly, then
clear _textWidget as before so Text.destroy() and parent cleanup run through the
lifecycle API.
- Around line 22-28: Store the unsubscribe callback returned by the tooltip
manager’s events.on listener in TooltipOverlay, then invoke it during unmount()
or destroy() before the overlay is disposed. Ensure teardown removes the
listener so later tooltip events cannot call _open or _close on the destroyed
overlay.
- Around line 52-56: Update the tooltip positioning and sizing logic in
TooltipOverlay so widths exceeding screenW never produce a negative _tooltipX or
an off-screen rectangle: clamp the horizontal position to a non-negative value,
constrain the rendered width to the available screen width, and truncate or wrap
the tooltip content accordingly. Add coverage for text wider than the terminal,
preserving normal positioning for content that fits.
---
Nitpick comments:
In `@packages/ui/src/TooltipOverlay.ts`:
- Around line 34-38: Update the Text construction in TooltipOverlay’s
_textWidget initialization to remove the any casts and ensure both fg and bg
fallbacks are valid Color values. Use the shared named-color fallback
representation consistently while preserving the existing black and yellow
defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e39c5df2-e446-470e-9857-b7ef0e114023
📒 Files selected for processing (7)
packages/core/src/app/App.tspackages/core/src/app/TooltipManager.tspackages/core/src/index.tspackages/ui/src/TooltipOverlay.test.tspackages/ui/src/TooltipOverlay.tspackages/ui/src/index.tspackages/widgets/src/base/Widget.ts
Description
This PR implements a global Tooltip System to provide extended descriptions when users hover over or focus on terminal UI elements. It introduces a
TooltipManagerin@termuijs/core, extends the baseWidgetwith an optionaltooltipproperty, and provides aTooltipOverlaycomponent in@termuijs/uithat correctly measures wide characters and emoji usingstringWidth().Related Issue
Closes #2861
Which package(s)?
@termuijs/core, @termuijs/widgets, @termuijs/ui
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
neverrules are respected.bun run buildpasses locally.bun run lintandbun run typecheckpass locally.Summary by CodeRabbit