Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Jun 4, 2025

PR Type

Enhancement


Description

  • Add custom tooltip component for utility descriptions

    • Introduce BotSharpTooltip Svelte component
    • Integrate tooltip for function descriptions in agent utilities
  • Enhance agent utility UI and UX

    • Show function descriptions as markdown tooltips
    • Adjust icon sizes and add scroll-to-top/bottom in logs
    • Improve utility item reset and deletion logic
  • Update and document utility types and helpers

    • Add description to UtilityItem type
    • Add classnames utility function
  • Update dependencies and styles

    • Add @popperjs/core for tooltip positioning
    • Refine SCSS for tooltip and markdown display

Changes walkthrough 📝

Relevant files
Enhancement
6 files
BotsharpTooltip.svelte
Add custom tooltip Svelte component with Popper.js             
+233/-0 
agent-utility.svelte
Integrate tooltip for function descriptions and refine utility UI
+67/-8   
_agent.scss
Add styles for custom tooltip and markdown display             
+17/-0   
common.js
Add classnames utility function                                                   
+6/-1     
Markdown.svelte
Add custom scrollbar and container id for markdown             
+31/-0   
persist-log.svelte
Add scroll-to-top/bottom buttons and refine log scrolling
+46/-22 
Documentation
1 files
agentTypes.js
Add description field to UtilityItem typedef                         
+1/-0     
Dependencies
1 files
package.json
Add @popperjs/core dependency for tooltips                             
+1/-0     
Additional files
1 files
signalr-service.js +1/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @iceljc iceljc marked this pull request as draft June 4, 2025 22:11
    @qodo-merge-pro
    Copy link

    qodo-merge-pro bot commented Jun 4, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit b2c0f6e)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Parameter Name Change

    The URL parameter name was changed from 'conversationId' to 'conversation-id'. This change might cause compatibility issues if other parts of the application still use the old parameter name.

    .withUrl(endpoints.chatHubUrl + `?conversation-id=${conversationId}&access_token=${user.token}`) // the hub URL, change it according to your server
    .withAutomaticReconnect() // enable automatic reconnection
    Event Listener Cleanup

    The tooltip component adds event listeners but doesn't always properly remove them in the unregisterEventListeners function, particularly for the tooltipEl element.

    $: if (persist && tooltipEl) {
        tooltipEl.addEventListener("mouseleave", close);
    }

    @qodo-merge-pro
    Copy link

    qodo-merge-pro bot commented Jun 4, 2025

    PR Code Suggestions ✨

    Latest suggestions up to b2c0f6e
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Restore infinite scrolling functionality

    The infinite scrolling functionality for loading older logs was removed when the
    scroll event listener was deleted. This means users can no longer load
    historical logs by scrolling to the top. Consider restoring this functionality
    or implementing an alternative way to load older logs.

    src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte [107-111]

     function initScrollbars() {
         scrollbarElements.forEach(item => {
             const elem = document.querySelector(item.id);
             if (!elem) return;
     
             // @ts-ignore
             const scrollbar = OverlayScrollbars(elem, options);
    +        scrollbar.on("scroll", async (e) => {
    +            const curScrollTop = e.elements().scrollOffsetElement.scrollTop;
    +            if (curScrollTop <= 3 && curScrollTop > 0) {
    +                if (item.type === contentLogTab) {
    +                    await getChatContentLogs();
    +                } else if (item.type === conversationStateLogTab) {
    +                    await getChatStateLogs();
    +                }
    +            }
    +        });
             scrollbars = [ ...scrollbars, scrollbar];
         });
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The removal of scroll event listeners eliminates the ability to load historical logs through infinite scrolling, which is a significant loss of functionality that users likely depend on.

    Medium
    Fix API parameter naming

    The query parameter name has been changed from conversationId to
    conversation-id. This change might cause compatibility issues with the backend
    service if it's expecting the original parameter name. Verify that the backend
    API accepts this new parameter name format.

    src/lib/services/signalr-service.js [51]

    -.withUrl(endpoints.chatHubUrl + `?conversation-id=${conversationId}&access_token=${user.token}`)
    +.withUrl(endpoints.chatHubUrl + `?conversationId=${conversationId}&access_token=${user.token}`)

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: The parameter name change from conversationId to conversation-id could break backend compatibility if the API expects the original parameter format. This could cause connection failures.

    Medium
    General
    Fix event handling

    The e.preventDefault() call in the click outside handler may interfere with
    normal click behavior on other elements. This can prevent users from interacting
    with elements outside the tooltip. Remove this call since it's not necessary for
    the click outside functionality.

    src/lib/common/tooltip/BotsharpTooltip.svelte [149-160]

     function handleClickOutside(e) {
    -    e.preventDefault();
    -
         if (!persist) return;
     
         const curNode = e.detail.currentNode;
         const targetNode = e.detail.targetNode;
     
         if (!curNode?.contains(targetNode)) {
             isOpen = false;
         }
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: Removing e.preventDefault() from the click outside handler improves user experience by not interfering with normal click interactions on other page elements.

    Low
    • Update

    Previous suggestions

    Suggestions up to commit 63e3973
    CategorySuggestion                                                                                                                                    Impact
    General
    Reset description on deletion

    When deleting a utility item, the description field should also be reset to null
    to maintain consistency with the data model. This prevents orphaned descriptions
    from persisting when items are deleted.

    src/routes/page/agent/[agentId]/agent-components/agent-utility.svelte [162-175]

     function deleteUtilityItem(uid, fid, type) {
         const foundUtility = innerUtilities.find((_, index) => index === uid);
         const foundItem = foundUtility?.items?.find((_, index) => index === fid);
         if (!foundUtility || !foundItem) return;
     
         if (type === 'function') {
             foundItem.function_name = null;
             foundItem.function_display_name = null;
             foundItem.template_name = null;
             foundItem.template_display_name = null;
    +        foundItem.description = null;
         } else if (type === 'template') {
             foundItem.template_name = null;
             foundItem.template_display_name = null;
         }
    Suggestion importance[1-10]: 6

    __

    Why: Good suggestion for data consistency. Since the PR adds the description field and resets it to null in innerRefresh, it should also be reset when deleting utility items to prevent orphaned descriptions.

    Low
    Possible issue
    Add error handling

    The code calls init() only after the utility options are loaded, but doesn't
    handle potential errors in the Promise. Add error handling to ensure the
    application doesn't hang if the utility options fetch fails.

    src/routes/page/agent/[agentId]/agent-components/agent-utility.svelte [41-62]

     onMount(async () =>{
         getAgentUtilityOptions().then(data => {
             const list = data || [];
             list.forEach(utility => {
                 const content = {
                     name: utility.name,
                     items: utility.items.map(it => {
                         return {
                             ...it,
                             function_display_name: truncateByPrefix(it.function_name, prefix),
                             template_display_name: truncateByPrefix(it.template_name, prefix)
                         };
                     })
                 };
     
                 const contents = utilityMapper[utility.category] || [];
                 contents.push(content);
                 utilityMapper[utility.category] = contents;
             });
             init();
    +    }).catch(error => {
    +        console.error("Failed to load utility options:", error);
    +        init(); // Still initialize with empty data
         });
     });
    Suggestion importance[1-10]: 4

    __

    Why: Valid suggestion for defensive programming, but has low impact since the application will still function if the service call fails. Error handling is good practice but not critical here.

    Low

    @iceljc iceljc requested a review from Oceania2018 June 6, 2025 18:19
    @iceljc iceljc marked this pull request as ready for review June 6, 2025 18:19
    @Oceania2018 Oceania2018 merged commit b60ca93 into SciSharp:main Jun 11, 2025
    1 of 2 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants