fix: Tooltip Component Update and Tooltips added to Chat UI

[JasonToups]

What this Pull Request (PR) does

The original issue was the tooltips were rendering, but not being shown, because they were constrained to a box around the UI buttons.

This PR fixes the Tooltip Component, since the tooltips were rendering, but not being shown.

Adds the fixed Tooltip to the Chat UI to key action buttons (Revert Last Message, Clear Chat, Copy Chat to Clipboard,

Load Session from File, Save Session to File) in the chat session manager.

Updated Tooltip Features:

• Delayed tooltip display (appears after 0.5 seconds of hover/focus).
• Smart positioning: tooltips automatically flip to avoid viewport overflow (top, bottom, left, right).
• Improved visual style and accessibility.
• Tooltip now renders outside the normal flow using fixed positioning for better placement.

Related issues

closes #1790

Screenshots

Here's what the tooltips look like in the current layout.

Model Controls UI

Chat UI

Here's an example of the different positions that can be used; top, bottom, left, right.

If the trigger button is too close to the edge of the viewport, and the original position of the tooltip won't fit within the viewport, we use the opposite value.

In the gif, the 4th UI button should have a tooltip positioned to the right, but since it's too close to the right part of the screen, it's automatically positioned to the left.

[ksylvan]

Thanks @JasonToups for this fix! I appreciate that you also added the changelog entry. Nice job.

I ran the PR diff through the review_code pattern and I'd like to know what you think about the recommendations as I am thinking about adding an automated review process to another repo. I'd appreciate your feedback here.

[ksylvan]

Please look this over and let me know if it makes sense to make these changes.

Thanks Jason!

Overall Assessment

This PR addresses tooltip rendering issues and adds tooltips to the chat UI. The code shows improvement in user experience by adding informative tooltips and implementing smart positioning logic. However, there are several concerns: removed error logging without replacement, potential memory leaks from timer cleanup, accessibility issues, and overly complex positioning logic. The core functionality appears sound, but needs refinement for production readiness.

Prioritized Recommendations

1. Fix potential memory leak by ensuring cleanup of timeouts in component lifecycle
2. Restore error logging with proper error handling instead of silent failures
3. Improve accessibility by using proper ARIA attributes and keyboard navigation
4. Simplify tooltip positioning logic and consider using a well-tested library
5. Add transition animations for better UX when showing/hiding tooltips
6. Handle window resize and scroll events to reposition tooltips dynamically

Detailed Feedback

---

[CORRECTNESS] - Potential Memory Leak from Uncleared Timeout

Original Code:

let showTimeout: ReturnType<typeof setTimeout> | null = null;

function showTooltip() {    
  if (showTimeout) {
    clearTimeout(showTimeout);
  }
  
  showTimeout = setTimeout(() => {
    tooltipVisible = true;
  }, 500);
}

Suggested Improvement:

import { onDestroy } from 'svelte';

let showTimeout: ReturnType<typeof setTimeout> | null = null;

onDestroy(() => {
  if (showTimeout) {
    clearTimeout(showTimeout);
    showTimeout = null;
  }
});

function showTooltip() {    
  if (showTimeout) {
    clearTimeout(showTimeout);
  }
  
  showTimeout = setTimeout(() => {
    tooltipVisible = true;
    showTimeout = null; // Clear reference after execution
  }, 500);
}

Rationale:
If the component is destroyed while a timeout is pending, the timeout will still fire and potentially cause issues. Using Svelte's onDestroy lifecycle hook ensures cleanup when the component unmounts. Also, clearing the timeout reference after execution prevents holding onto stale timeout IDs.

---

[SECURITY/MAINTAINABILITY] - Removed Error Logging Without Alternative

Original Code:

try {
  await sessionAPI.loadSessions();
} catch (error) {
  // Error silently caught - no logging or user feedback
}

Suggested Improvement:

try {
  await sessionAPI.loadSessions();
} catch (error) {
  console.error('Failed to load sessions:', error);
  toastService.error('Unable to load sessions. Please try again.');
}

Rationale:
Silently catching errors makes debugging extremely difficult in production and provides no feedback to users when operations fail. While removing console.error calls can be appropriate for production builds (use a proper logging service), completely removing error handling is dangerous. At minimum, users should be notified via the existing toast service, and errors should be logged to a monitoring service. This change affects three locations in SessionManager.svelte.

---

[ACCESSIBILITY] - Improper ARIA and Keyboard Navigation

Original Code:

<div 
  class="tooltip-trigger cursor-pointer" 
  bind:this={triggerElement}
  on:mouseenter={showTooltip}
  on:mouseleave={hideTooltip}
  on:focusin={showTooltip}
  on:focusout={hideTooltip}
  role="tooltip"
  aria-label="Tooltip trigger"
>
  <slot />
</div>

Suggested Improvement:

<div 
  class="tooltip-trigger" 
  bind:this={triggerElement}
  on:mouseenter={showTooltip}
  on:mouseleave={hideTooltip}
  on:focus={showTooltip}
  on:blur={hideTooltip}
  aria-describedby={tooltipVisible ? 'tooltip-' + tooltipId : undefined}
>
  <slot />
</div>

{#if tooltipVisible}
  <div
    bind:this={tooltipElement}
    id="tooltip-{tooltipId}"
    class="tooltip fixed z-[9999] px-3 py-2 text-sm rounded-lg bg-gray-500 text-white whitespace-nowrap shadow-xl border-2 border-gray-600"
    role="tooltip"
  >
    {text}
  </div>
{/if}

<script>
  let tooltipId = Math.random().toString(36).substr(2, 9);
</script>

Rationale:
The trigger element should not have role="tooltip" - that role belongs to the tooltip itself. The trigger should use aria-describedby to reference the tooltip's ID. Using focusin/focusout instead of focus/blur can cause issues with event bubbling. The tooltip needs a unique ID for proper ARIA association. Additionally, the slotted content (likely buttons) already has its own accessibility attributes, so the wrapper shouldn't interfere.

---

[PERFORMANCE] - Complex Positioning Logic Without Optimization

Original Code:

function updateTooltipPosition() {
  if (!tooltipVisible || !triggerElement || !tooltipElement) {
    return;
  }
  
  const triggerRect = triggerElement.getBoundingClientRect();
  const tooltipRect = tooltipElement.getBoundingClientRect();
  // ... 80+ lines of manual positioning logic
}

$: if (tooltipVisible) {
  setTimeout(updateTooltipPosition, 0);
}

Suggested Improvement:

import { onMount } from 'svelte';

let resizeObserver: ResizeObserver | null = null;

onMount(() => {
  // Consider using a library like Floating UI (formerly Popper.js)
  // Or at minimum, throttle position updates
  
  resizeObserver = new ResizeObserver(() => {
    if (tooltipVisible) {
      updateTooltipPosition();
    }
  });
  
  if (triggerElement) {
    resizeObserver.observe(triggerElement);
  }
  
  return () => {
    if (resizeObserver) {
      resizeObserver.disconnect();
    }
  };
});

$: if (tooltipVisible && tooltipElement) {
  // Use requestAnimationFrame for smoother updates
  requestAnimationFrame(updateTooltipPosition);
}

Rationale:
The manual positioning logic is complex and error-prone. Libraries like Floating UI handle edge cases, viewport boundaries, and collision detection much more reliably. If you must implement it manually, use requestAnimationFrame instead of setTimeout(fn, 0) for better performance. Additionally, tooltips should reposition on scroll/resize events, which the current implementation doesn't handle. The complexity of this code suggests it would be better to use a battle-tested library.

---

[USER EXPERIENCE] - Missing Transitions and Visual Polish

Original Code:

<div
  bind:this={tooltipElement}
  class="tooltip fixed z-[9999] px-3 py-2 text-sm rounded-lg bg-gray-500 text-white whitespace-nowrap shadow-xl border-2 border-gray-600"
  role="tooltip"
  aria-label={text}
>
  {text}
</div>

Suggested Improvement:

<script>
  import { fade, scale } from 'svelte/transition';
</script>

{#if tooltipVisible}
  <div
    bind:this={tooltipElement}
    transition:fade={{ duration: 150 }}
    class="tooltip fixed z-[9999] px-3 py-2 text-sm rounded-lg bg-gray-800 text-white whitespace-nowrap shadow-xl border border-gray-700"
    role="tooltip"
  >
    {text}
  </div>
{/if}

Rationale:
Tooltips should have smooth fade-in/fade-out transitions for better UX. The current implementation appears/disappears abruptly. Using Svelte's built-in transitions provides smooth animations with minimal code. Also, aria-label should not be used on the tooltip itself - it's redundant since the text content is already accessible. The color scheme could be improved (gray-500 background with gray-600 border has poor contrast; gray-800 with gray-700 is better).

---

[MAINTAINABILITY] - Magic Numbers Without Constants

Original Code:

showTimeout = setTimeout(() => {
  tooltipVisible = true;
}, 500);

// ... later in positioning logic
if (triggerRect.top - tooltipRect.height - 8 < 8) {
  actualPosition = 'bottom';
  top = triggerRect.bottom + 8;
}

Suggested Improvement:

const TOOLTIP_DELAY_MS = 500;
const TOOLTIP_OFFSET_PX = 8;
const VIEWPORT_PADDING_PX = 8;

showTimeout = setTimeout(() => {
  tooltipVisible = true;
}, TOOLTIP_DELAY_MS);

// ... later in positioning logic
if (triggerRect.top - tooltipRect.height - TOOLTIP_OFFSET_PX < VIEWPORT_PADDING_PX) {
  actualPosition = 'bottom';
  top = triggerRect.bottom + TOOLTIP_OFFSET_PX;
}

Rationale:
Magic numbers scattered throughout the code make it difficult to maintain and adjust. By extracting these values into named constants at the top of the script, the code becomes more readable and easier to modify. If you want to change the tooltip delay or spacing, you only need to update it in one place. This also makes the intent of each number clear.

---

[CORRECTNESS] - Tooltip Position Update Timing Issue

Original Code:

// Update position when tooltip becomes visible
$: if (tooltipVisible) {
  setTimeout(updateTooltipPosition, 0);
}

Suggested Improvement:

import { tick } from 'svelte';

$: if (tooltipVisible && tooltipElement) {
  tick().then(() => {
    updateTooltipPosition();
  });
}

Rationale:
Using setTimeout(fn, 0) is a common pattern but not the most appropriate for Svelte. The tick() function is Svelte's way of waiting until pending state changes have been applied to the DOM. This is more semantically correct and ensures the tooltip element has been fully rendered with its final dimensions before calculating position. Also, add a check for tooltipElement to prevent calling the positioning function before the element is bound.

[ksylvan]

@JasonToups Please look the review over and make the requested changes.

[ksylvan]

@JasonToups Please look the review over and make the requested changes.

Any updates here @JasonToups

[ksylvan]

@JasonToups While the work is essentially good, I need you to address the suggested improvements.

I am closing this due to lack of response. Please feel free to re-create and submit another version.