Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Aug 26, 2025

PR Type

Enhancement


Description

  • Optimize Select component reactive updates and performance

  • Add debounced search functionality with 500ms delay

  • Improve agent options initialization and state management

  • Fix reactive statement ordering and variable assignments


Diagram Walkthrough

flowchart LR
  A["Search Input"] -- "debounce 500ms" --> B["Filter Options"]
  C["Option Selection"] --> D["Update State"]
  D --> E["Sync References"]
  F["Agent Init"] --> G["Clear Previous State"]
  G --> H["Set New Options"]
Loading

File Walkthrough

Relevant files
Enhancement
Select.svelte
Optimize Select component reactivity and search                   

src/lib/common/Select.svelte

  • Add timer variable for debounced search functionality
  • Reorganize reactive statements for better performance
  • Optimize variable assignments using temporary variables
  • Remove redundant function calls in reactive blocks
+30/-18 
instruction-agent.svelte
Improve agent options initialization                                         

src/routes/page/instruction/instruction-components/instruction-agent.svelte

  • Rename collectAgentOptions to initAgentOptions
  • Clear all state variables before initialization
  • Reset selected agent and template on options change
+7/-2     

@qodo-merge-pro
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Timer Cleanup

The debounced search uses a module-level timer but never clears it on component destroy, which may trigger callbacks after unmount or leak timers. Ensure cleanup in onDestroy and guard callbacks against stale state.

    /**
     * @type {number | undefined}
     */
    let timer;

    onMount(() => {
        initOptions();
    });

    $: {
        innerOptions = verifySelectedOptions(innerOptions, selectedValues);
        refOptions = verifySelectedOptions(innerOptions, selectedValues);
    }

    $: {
        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
                ];

            }
        } else {
            innerOptions = verifySelectedOptions(options, selectedValues);
            refOptions = verifySelectedOptions(options, selectedValues);
        }
    }

    $: {
        if (innerOptions && refOptions) {
            applySearchFilter();
            changeDisplayText();
        }
    }

    function initOptions() {
        const newInnerOptions = options.map(x => {
            return {
                label: x.label,
                value: x.value,
                checked: false
            }
        });

        const newRefOptions = options.map(x => {
            return {
                label: x.label,
                value: x.value,
                checked: false
            }
        });

        innerOptions = newInnerOptions;
        refOptions = newRefOptions;
    }

    /**
	 * @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;
        }) || [];
    }

    async function toggleOptionList() {
        if (disabled) return;

        showOptionList = !showOptionList;
        if (showOptionList) {
            await tick();
            adjustDropdownPosition();
        }
    }


    /** @param {any} e */
    function changeSearchValue(e) {
        searchValue = e.target.value || '';
        clearTimeout(timer);
        timer = setTimeout(() => {
            applySearchFilter();
            verifySelectAll();
        }, 500);
    }
Reactive Ordering

The split of reactive blocks moves applySearchFilter/changeDisplayText into a separate block gated by innerOptions/refOptions. Verify that display text and filtering still update correctly when loadMore and selectedValues change to avoid stale UI.

    }
}

$: {
    if (innerOptions && refOptions) {
        applySearchFilter();
        changeDisplayText();
    }
}
State Reset

initAgentOptions clears selectedAgent and selectedTemplate whenever agents changes; confirm this does not undesirably reset user selections on minor updates. Consider preserving selection if still present in new options.

    $: {
        initAgentOptions(agents);
    }

    /**
	 * @param {import('$agentTypes').AgentModel[]} agents
	 */
    function initAgentOptions(agents) {
        agentOptions = [];
        templateOptions = [];
        selectedAgent = null;
        selectedTemplate = null;

        agentOptions = agents?.map(x => ({
            label: x.name,
            value: x.id
        }))?.sort((a, b) => a.label.localeCompare(b.label)) || [];

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 26, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Preserve selection on reinit

initAgentOptions unconditionally clears selectedAgent and selectedTemplate,
which will wipe the user’s current selection whenever agents updates, causing a
disruptive UX. Instead, rebuild options and only reset selections if the
previously selected ids are no longer present; otherwise, carry them forward.
This avoids unexpected deselection during routine data refreshes.

Examples:

src/routes/page/instruction/instruction-components/instruction-agent.svelte [31-39]
    function initAgentOptions(agents) {
        agentOptions = [];
        templateOptions = [];
        selectedAgent = null;
        selectedTemplate = null;
        
        agentOptions = agents?.map(x => ({
            label: x.name,
            value: x.id

Solution Walkthrough:

Before:

function initAgentOptions(agents) {
    // These lines unconditionally clear all state,
    // including user selections.
    agentOptions = [];
    templateOptions = [];
    selectedAgent = null;
    selectedTemplate = null;
    
    agentOptions = agents?.map(x => ({
        label: x.name,
        value: x.id
    }));
}

After:

function initAgentOptions(agents) {
    // Preserve current selections to check against new options.
    const prevSelectedAgentId = selectedAgent?.value;

    const newAgentOptions = agents?.map(x => ({
        label: x.name,
        value: x.id
    }));
    agentOptions = newAgentOptions;

    // Only reset selection if the previously selected agent is no longer valid.
    if (!agentOptions.some(opt => opt.value === prevSelectedAgentId)) {
        selectedAgent = null;
        selectedTemplate = null; // Template depends on agent
    }
    // Template options would be rebuilt based on the potentially preserved agent.
}
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a significant UX regression where user selections are cleared on data refresh, proposing a robust fix to preserve state.

High
General
Harden debounce timer handling
Suggestion Impact:The commit added a guard around clearTimeout, checking if timer exists before calling it. The part about resetting the timer to undefined after the callback was not implemented.

code diff:

+        if (timer) {
+            clearTimeout(timer);
+        }

Guard clearTimeout to avoid calling it with undefined, and reset timer after the
callback runs. This prevents stale timer references and reduces the chance of
double-execution or leaks.

src/lib/common/Select.svelte [174-181]

 function changeSearchValue(e) {
     searchValue = e.target.value || '';
-    clearTimeout(timer);
+    if (timer != null) clearTimeout(timer);
     timer = setTimeout(() => {
         applySearchFilter();
         verifySelectAll();
+        timer = undefined;
     }, 500);
 }

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential issue with the timer, but calling clearTimeout with undefined is safe in JavaScript, making the guard unnecessary, and resetting the timer to undefined inside the callback is a minor improvement.

Low
  • Update

@iceljc iceljc merged commit 943a56f into SciSharp:main Aug 26, 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