Skip to content

Conversation

shoummu1
Copy link
Collaborator

📌 Summary
This PR refactors how tool descriptions are rendered in the admin UI table. It improves UI safety and consistency by escaping problematic characters and removing HTML artifacts from tool descriptions. Previously, unescaped characters like quotes or newlines caused Alpine expression errors when used in tooltip attributes.

🔗 Related Issues
Closes #557

✅ Changes Made

  1. Removed use of x-tooltip attribute to prevent Alpine.js parsing errors
  2. Normalized tool descriptions by stripping HTML tags and escaping special characters
  3. Replaced newline characters (\n, \r) with spaces for consistent formatting
  4. Truncated long descriptions (over 120 characters) with a ... suffix
  5. Added null-safe fallback for missing descriptions

🧪 Test Plan

  1. Verified rendering of long tool descriptions in the UI
  2. Confirmed that special characters (e.g., ', ", &, %) are rendered safely
  3. Ensured there are no tooltip-related JavaScript errors in browser console
  4. Checked that empty or null descriptions do not break the UI

Before
before

After
after

🚧 Future Enhancements
Support for hover-based tooltips can be added in the future to improve usability. This would allow users to view the full tool description on hover without modifying the table layout.

@shoummu1 shoummu1 marked this pull request as ready for review August 11, 2025 11:41
@shoummu1 shoummu1 requested a review from crivetimihai as a code owner August 11, 2025 11:41
@crivetimihai
Copy link
Member

PR #717 Review: Truncating Long Tool Descriptions

Can be merged, suggestions below can be addressed after.

Summary

This PR addresses UI layout issues caused by long tool descriptions in the admin panel by implementing description truncation at 120 characters.

Changes Reviewed

  • File: mcpgateway/templates/admin.html
  • Lines Modified: 768-774
  • Type: UI/UX improvement

What Works Well ✅

  1. Prevents Layout Breakage: Successfully addresses the core issue of long descriptions disrupting table layout
  2. Security Conscious: Properly sanitizes content with striptags, trim, and escape filters
  3. Clean Implementation: Uses Jinja2 templating features appropriately
  4. Handles Edge Cases: Safely handles null/empty descriptions with (tool.description or "")

Suggested Improvements 🔧

1. Add Tooltip for Full Description

Users lose access to the complete description. Consider adding a tooltip:

{% if refactor_desc | length is greaterthan 120 %}
  <span title="{{ refactor_desc }}" class="cursor-help">
    {{ refactor_desc[:120] }}...
  </span>
{% else %}
  {{ refactor_desc }}
{% endif %}

2. Make Truncation Length Configurable

Instead of hardcoding 120, consider using a configuration variable:

{% set max_desc_length = config.UI_MAX_DESCRIPTION_LENGTH | default(120) %}
{% if refactor_desc | length is greaterthan max_desc_length %}
  {{ refactor_desc[:max_desc_length] }}...

3. Improve Variable Naming

The variable name refactor_desc doesn't clearly convey its purpose. Consider:

  • sanitized_desc - emphasizes the security processing
  • processed_desc - indicates transformation applied
  • safe_desc - highlights it's safe for display

4. Consider Progressive Disclosure Pattern

For better UX, implement an expand/collapse mechanism:

<div x-data="{ expanded: false }">
  <span x-show="!expanded">
    {{ refactor_desc[:120] }}{% if refactor_desc | length > 120 %}...
    <button @click="expanded = true" class="text-blue-500 hover:underline">
      Show more
    </button>
    {% endif %}
  </span>
  <span x-show="expanded">
    {{ refactor_desc }}
    <button @click="expanded = false" class="text-blue-500 hover:underline">
      Show less
    </button>
  </span>
</div>

5. Add Visual Indicator for Truncation

Consider adding an icon or different styling to indicate truncated content:

{% if refactor_desc | length is greaterthan 120 %}
  <span class="truncated-description">
    {{ refactor_desc[:120] }}...
    <svg class="inline w-4 h-4 text-gray-400" fill="currentColor" viewBox="0 0 20 20">
      <!-- Info icon SVG path -->
    </svg>
  </span>
{% else %}

Additional Considerations 💭

  1. Accessibility: Ensure truncated descriptions don't lose critical information for screen readers
  2. Mobile Responsiveness: Consider different truncation lengths for mobile views
  3. Consistency: Apply similar truncation to other long text fields in the admin UI
  4. Documentation: Update admin UI documentation to mention the 120-character limit

Testing Recommendations 🧪

  1. Test with descriptions of various lengths (0, 119, 120, 121, 1000+ characters)
  2. Test with descriptions containing special characters and HTML entities
  3. Test with multi-line descriptions containing various newline characters
  4. Verify no XSS vulnerabilities with malicious description content
  5. Check responsive behavior on different screen sizes

Overall Assessment ✔️

Approved with suggestions - The change effectively solves the immediate problem and is implemented safely. The suggested improvements would enhance user experience but aren't blocking issues.

Priority of Suggestions

  1. High: Add tooltip (minimal effort, high UX value)
  2. Medium: Improve variable naming (code clarity)
  3. Low: Make configurable, progressive disclosure (nice-to-have features)

@crivetimihai crivetimihai force-pushed the update-tool-description branch from 00f8e0c to 9f7c295 Compare August 11, 2025 13:56
@crivetimihai crivetimihai merged commit b0c2582 into main Aug 11, 2025
37 checks passed
@crivetimihai crivetimihai deleted the update-tool-description branch August 11, 2025 14:03
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.

[BUG] Cleanup tool descriptions to remove newlines and truncate text
2 participants