Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 15, 2025

PR Type

Enhancement


Description

  • Replace MultiSelect component with unified Select component

  • Add multi-select and single-select modes support

  • Enhance component with disabled state and search functionality

  • Update all component usages across application pages


Changes diagram

flowchart LR
  A["MultiSelect component"] --> B["Unified Select component"]
  B --> C["Single-select mode"]
  B --> D["Multi-select mode"]
  B --> E["Disabled state support"]
  B --> F["Enhanced search functionality"]
  G["Agent pages"] --> B
  H["Conversation pages"] --> B
  I["Instruction pages"] --> B
Loading

Changes walkthrough 📝

Relevant files
Configuration changes
1 files
app.scss
Update SCSS import path for select component                         
+1/-1     
Enhancement
8 files
_select.scss
Add disabled state styling for select component                   
+18/-0   
Select.svelte
Major refactor to support single/multi-select modes           
+149/-62
+page.svelte
Replace MultiSelect with Select component usage                   
+7/-5     
+page.svelte
Convert dropdowns to Select component with multi-select   
+52/-33 
instruction-llm.svelte
Replace dropdowns with Select component for LLM selection
+38/-15 
instruction-template.svelte
Update agent and template selection to Select component   
+38/-15 
+page.svelte
Convert filter dropdowns to Select component usage             
+64/-46 
conversationTypes.js
Add agentIds array support to conversation types                 
+2/-0     
Bug fix
1 files
+page.svelte
Fix instruction text assignment in agent selection             
+4/-3     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Logic Complexity

    The component has complex reactive statements and multiple interdependent functions that handle both single and multi-select modes. The logic for verifying selected options, handling search, and managing state updates needs careful validation to ensure it works correctly in all scenarios.

        onMount(() => {
            initOptions();
        });
    
        $: {
            innerOptions = verifySelectedOptions(innerOptions, selectedValues);
            refOptions = verifySelectedOptions(innerOptions, selectedValues);
            changeDisplayText();
        }
    
        $: {
            if (loadMore) {
                if (options.length > refOptions.length) {
                    const curKeys = refOptions.map(x => x.value);
                    const newOptions = options.filter(x => !curKeys.includes(x.value)).map(x => {
                        return {
                            label: x.label,
                            value: x.value,
                            checked: false
                        };
                    });
    
                    innerOptions = [
                        ...innerOptions,
                        ...newOptions
                    ];
    
                    refOptions = [
                        ...refOptions,
                        ...newOptions
                    ];
    
                    changeDisplayText();
                }
            } else {
                innerOptions = verifySelectedOptions(options, selectedValues);
                refOptions = verifySelectedOptions(options, selectedValues);
                changeDisplayText();
            }
        }
    
    
        function initOptions() {
            innerOptions = options.map(x => {
                return {
                    label: x.label,
                    value: x.value,
                    checked: false
                }
            });
    
            refOptions = options.map(x => {
                return {
                    label: x.label,
                    value: x.value,
                    checked: false
                }
            });
        }
    
        /**
    	 * @param {any[]} list
    	 * @param {string[]} selecteds
    	 */
        function verifySelectedOptions(list, selecteds) {
            return list?.map(x => {
                const item = { ...x, checked: false };
                if (multiSelect) {
                    item.checked = !!selecteds?.includes(item.value);
                } else {
                    item.checked = selecteds.length > 0 && selecteds[0] === item.value;
                }
                return item;
            }) || [];
        }
    Event Handling

    The component uses pointer-events: none on checkboxes and relies on click handlers on parent elements. This approach may cause accessibility issues and unexpected behavior, especially with keyboard navigation and screen readers.

                    style="pointer-events: none;"
                    checked={selectAllChecked}
                    readonly
                />
            </div>
            <div class="line-align-center select-name fw-bold">
                {'Select all'}
            </div>
        </li>
    {/if}
    {#if !multiSelect}
        <!-- svelte-ignore a11y-click-events-have-key-events -->
        <!-- svelte-ignore a11y-no-noninteractive-element-interactions -->
        <li
            class="option-item clickable"
            on:click|preventDefault|stopPropagation={() => {
                clearSelection();
            }}
        >
            <div class="line-align-center text-secondary">
                {`Clear selection`}
            </div>
        </li>
    {/if}
    {#each innerOptions as option, idx (idx)}
        <!-- svelte-ignore a11y-click-events-have-key-events -->
        <!-- svelte-ignore a11y-no-noninteractive-element-interactions -->
        <li
            class="option-item clickable"
            on:click|preventDefault|stopPropagation={() => {
                checkOption(null, option);
            }}
        >
            <div class="line-align-center select-box">
                <Input
                    type="checkbox"
                    style="pointer-events: none;"
                    checked={option.checked}
                    readonly
                />
    Data Mapping

    The conversion from old data structure (id/name) to new structure (value/label) and the handling of selectedValues arrays needs validation to ensure data consistency and proper filtering functionality.

    const selectedValues = e?.detail?.selecteds?.map(x => x.value) || [];
    
    if (type === 'agent') {
    	searchOption = {
    		...searchOption,
    		agentIds: selectedValues
    	};
    } else if (type === 'task') {
    	searchOption = {
    		...searchOption,
    		taskId: selectedValues.length > 0 ? selectedValues[0] : null
    	};
    } else if (type === 'channel') {
    	searchOption = {
    		...searchOption,
    		channel: selectedValues.length > 0 ? selectedValues[0] : null
    	};
    } else if (type === 'status') {
    	searchOption = {
    		...searchOption,
    		status: selectedValues.length > 0 ? selectedValues[0] : null
    	};
    } else if (type === 'tags') {
    	searchOption = {
    		...searchOption,
    		tags: e.target.value?.length > 0 ? [e.target.value] : []

    @qodo-merge-pro
    Copy link

    qodo-merge-pro bot commented Jul 15, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Include selected values in event payload

    The event should include the selected values array for easier consumption by
    parent components. This provides a more convenient API for handling selection
    changes.

    src/lib/common/Select.svelte [302-305]

     function sendEvent() {
         const selecteds = refOptions.filter(x => x.checked);
    -    dispatch('select', { selecteds });
    +    const selectedValues = selecteds.map(x => x.value);
    +    dispatch('select', { selecteds, selectedValues });
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: Adding selectedValues to the event payload is a significant improvement, as it simplifies the logic in parent components by providing the raw values directly, which is what they primarily use.

    Medium
    Possible issue
    Add null safety for selecteds parameter

    The function should handle cases where selecteds is null or undefined to prevent
    potential runtime errors when checking array length or calling includes().

    src/lib/common/Select.svelte [139-149]

     function verifySelectedOptions(list, selecteds) {
         return list?.map(x => {
             const item = { ...x, checked: false };
             if (multiSelect) {
    -            item.checked = !!selecteds?.includes(item.value);
    +            item.checked = !!(selecteds?.includes(item.value));
             } else {
    -            item.checked = selecteds.length > 0 && selecteds[0] === item.value;
    +            item.checked = (selecteds?.length > 0) && selecteds[0] === item.value;
             }
             return item;
         }) || [];
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: This suggestion correctly identifies a potential runtime error if the selecteds prop is null or undefined and improves the component's robustness with a simple fix.

    Low
    Add null safety for label property

    The search function should handle null/undefined labels gracefully to prevent
    runtime errors. Add null checks before calling toLowerCase() on the label
    property.

    src/lib/common/Select.svelte [163-171]

     function changeSearchValue(e) {
         searchValue = e.target.value || '';
         const innerValue = searchValue.toLowerCase();
     
         if (searchValue) {
    -        innerOptions = [...refOptions.filter(x => x.label.toLowerCase().includes(innerValue))];
    +        innerOptions = [...refOptions.filter(x => x.label?.toLowerCase().includes(innerValue))];
         } else {
             innerOptions = [...refOptions];
         }
     }
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: This is a good defensive programming practice that adds robustness by preventing a potential runtime error if an option with a null or undefined label is provided.

    Low
    • Update

    @iceljc iceljc merged commit 93bc01c into SciSharp:main Jul 15, 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.

    1 participant