Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Aug 8, 2025

PR Type

Enhancement


Description

  • Refine knowledge base search with advanced filtering

  • Add reasoning effort level to agent LLM config

  • Enhance vector knowledge editing with payload support

  • Improve UI scrolling and styling


Diagram Walkthrough

flowchart LR
  A["Advanced Search"] --> B["Filter Groups"]
  B --> C["Knowledge Base"]
  D["Agent Config"] --> E["Reasoning Level"]
  F["Vector Edit"] --> G["Payload Support"]
  C --> H["Enhanced Search"]
Loading

File Walkthrough

Relevant files
Formatting
1 files
_knowledgebase.scss
Add styling for tooltips and scrollable containers             
+29/-0   
Enhancement
8 files
agent-llm-config.svelte
Add reasoning effort level configuration option                   
+30/-0   
advanced-search.svelte
Refactor advanced search with dynamic filters                       
+169/-64
vector-item-edit-modal.svelte
Add payload editing functionality to modal                             
+154/-6 
+page.svelte
Integrate advanced search with filter groups                         
+84/-75 
+page.svelte
Add advanced search to Q&A knowledge                                         
+101/-59
enums.js
Add reasoning effort level enum values                                     
+8/-1     
agentTypes.js
Add reasoning effort level to type definition                       
+1/-0     
knowledgeTypes.js
Update knowledge types for filter groups                                 
+10/-2   
Bug fix
2 files
vector-item.svelte
Update vector dimension display logic                                       
+2/-2     
common.js
Fix text splitting logic condition                                             
+1/-1     

@iceljc iceljc marked this pull request as draft August 8, 2025 20:29
@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 8, 2025

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

Possible Issue

The local showAdvSearch, items, and operator props are mutated internally (e.g., reset() sets items = [...]) without syncing to a parent unless bound; ensure parent bindings are always present or emit changes to avoid stale state when this component is used elsewhere.

    function reset() {
        items = [{ uuid: uuidv4(), key: '', value: '', checked: true }];
    }


    /**
     * @param {any} e
     * @param {number} idx
	 */
    function toggleItem(e, idx) {
        const found = items.find((_, index) => index === idx);
        if (found) {
            found.checked = e.target.checked;
            items = items.map((x, index) => {
                return index === idx ? { ...found } : x;
            });
        }
    }

    async function addItem() {
        items = [
            ...items,
            { uuid: uuidv4(), key: '', value: '', checked: true }
        ];

        // Wait for DOM to update, then scroll to bottom
        await tick();
        if (scrollContainer) {
            scrollContainer.scrollTop = scrollContainer.scrollHeight;
        }
    }

    /**
     * @param {number} idx
	 */
    function removeItem(idx) {
        if (disabled) return;

        items = items.filter((_, index) => idx !== index);
    }

    /**
     * @param {any} e
	 * @param {number} idx
     * @param {string} key
	 */
    function changeItem(e, idx, key) {
        const found = items.find((_, index) => index === idx);
        if (found) {
            if (key === 'key') {
                found.key = e.target.value;
            } else if (key === 'value') {
                found.value = e.target.value;
            }
            items = items.map((x, index) => {
                return index === idx ? { ...found } : x;
            });
        }
    }

    /**
     * @param {any} e
     */
    function changeLogicalOperator(e) {
Data Consistency

handleConfirm sends payload separately while also spreading text/answer into data. The caller updates only found.data with newItem.payload merged, but existing data keys not in payload are replaced; verify server/consumer expectations to avoid losing unrelated fields.

/** @param {any} e */
function handleConfirm(e) {
    e.preventDefault();

    if (!allowPayload) {
        innerPayloads = [];
    }

    const validPayloads = innerPayloads.map(x => ({ key: util.trim(x.key), value: util.trim(x.value) }))
                                    .filter(x => !!x.key && !!x.value && !excludedPayloads.includes(x.key));

    const obj = validPayloads.reduce((acc, cur) => {
        // @ts-ignore
        acc[cur.key] = cur.value;
        return acc;
    }, {});

    const newItem = {
        ...item,
        data: {
            ...item?.data,
            text: question.text,
            answer: answer.text
        },
        payload: obj || {}
    };
    confirm?.(newItem);
}
API Contract Change

Replaced included_payloads/search_pairs with fields/filter_groups. Ensure backend supports these fields across both paging and search endpoints; otherwise calls may fail or silently ignore filters.

/**
 * @param {{
 * startId: string | null,
 * isReset: boolean,
 * filterGroups: any[] }} params
 */
function getKnowledgeListData(params = {
	startId: null,
	isReset: false,
	filterGroups: []
}) {
	return new Promise((resolve, reject) => {
		const filter = {
			size: pageSize,
			start_id: params.startId,
			with_vector: enableVector,
			fields: [],
			filter_groups: params.filterGroups
		};

		getVectorKnowledgePageList(

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Validate filter schema contract

The new client API switches from search_pairs to filter_groups with
filter_operator and filters, but there’s no visible normalization/validation to
ensure operators and keys map to what the backend expects (e.g., enum values,
required fields, empty groups). Add a central validator/mapper that guarantees
only supported operators are sent, trims/removes empty filters, and converts UI
state into the exact backend contract to prevent silent no-op searches or 400s
across both pages.

Examples:

src/routes/page/knowledge-base/documents/+page.svelte [709-723]
function buildSearchFilterGroups(items) {
	/** @type {any[]} */
	let groups = [];
	if (textSearch && !!text) {
		groups = [ ...groups, { filter_operator: 'or', filters: [{ key: KnowledgePayloadName.Text, value: text }] } ];
	}

	if (isAdvSearchOn && items?.length > 0) {
		const validItems = items.filter(x => x.checked && !!util.trim(x.key) && !!util.trim(x.value))
									.map(x => ({ key: util.trim(x.key), value: util.trim(x.value) }));

 ... (clipped 5 lines)
src/routes/page/knowledge-base/question-answer/+page.svelte [656-673]
function buildSearchFilterGroups(items) {
	/** @type {any[]} */
	let groups = [];
	if (textSearch && !!text) {
		groups = [ ...groups, { filter_operator: 'or', filters: [
			{ key: KnowledgePayloadName.Text, value: text },
			{ key: KnowledgePayloadName.Answer, value: text }
		] } ];
	}


 ... (clipped 8 lines)

Solution Walkthrough:

Before:

function buildSearchFilterGroups(items) {
    let groups = [];
    if (textSearch && !!text) {
        // ... adds a group for text search
    }

    if (isAdvSearchOn && items?.length > 0) {
        const validItems = items.filter(x => x.checked && !!util.trim(x.key) && !!util.trim(x.value))
                                .map(x => ({ key: util.trim(x.key), value: util.trim(x.value) }));
        // This can add a group with an empty `filters` array if `validItems` is empty.
        groups = [ ...groups, { filter_operator: selectedOperator, filters: validItems } ];
    }

    return groups;
}

After:

// In a shared utility file
function createFilterGroups(textSearch, text, isAdvSearchOn, items, selectedOperator) {
    const groups = [];
    if (textSearch && text) {
        // ... adds a group for text search
    }

    if (isAdvSearchOn) {
        const validFilters = items
            .filter(item => item.checked && item.key && item.value)
            .map(item => ({ key: item.key.trim(), value: item.value.trim() }));

        if (validFilters.length > 0) {
            groups.push({ filter_operator: selectedOperator, filters: validFilters });
        }
    }
    return groups;
}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the buildSearchFilterGroups function, duplicated in two files, lacks validation for empty filter sets, which could lead to incorrect API requests and search results.

Medium
Possible issue
Use unified data source
Suggestion Impact:The commit removed reliance on dataSource from newItem.data in refreshItems and shifted dataSource handling to e.payload elsewhere, aligning with the suggestion’s goal to use a unified source and avoid mixing data and payload. It also updated confirmEdit to consistently use e.payload.dataSource, reinforcing the unified approach.

code diff:

@@ -571,7 +569,6 @@
 		if (found) {
 			const newData = {
 				text: newItem.data?.text || '',
-				dataSource: newItem.data?.dataSource,
 				...newItem.payload
 			};

refreshItems reads newItem.payload while the edit modal now merges payload into
data. This causes updated fields to be dropped from the UI. Read from
newItem.data only to keep behavior consistent regardless of where payload lives.

src/routes/page/knowledge-base/documents/+page.svelte [569-581]

 function refreshItems(newItem) {
     const found = items?.find(x => x.id == newItem?.id);
     if (found) {
         const newData = {
-            text: newItem.data?.text || '',
-            dataSource: newItem.data?.dataSource,
-            ...newItem.payload
+            ...(found.data || {}),
+            ...(newItem.data || {})
         };
-
         found.data = { ...newData };
         items = [ ...items ];
     }
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that refreshItems merges newItem.data and newItem.payload, but the updateVectorKnowledgeData call in confirmEdit uses a confusing mix of properties from e.data and e.payload. The proposed change simplifies refreshItems to use a single data source, which would be consistent if the data structure from the modal were also simplified, thus preventing potential bugs.

Medium
Consolidate item update source

As with documents, mixing data and payload breaks updates if the modal merges
payload into data. Consolidate by deriving from newItem.data only to ensure the
UI reflects all fields, including added payload entries.

src/routes/page/knowledge-base/question-answer/+page.svelte [558-571]

 function refreshItems(newItem) {
     const found = items?.find(x => x.id == newItem?.id);
     if (found) {
         const newData = {
-            text: newItem.data?.text || '',
-            answer: newItem.data?.answer || '',
-            dataSource: newItem.data?.dataSource,
-            ...newItem.payload
+            ...(found.data || {}),
+            ...(newItem.data || {})
         };
-
         found.data = { ...newData };
         items = [ ...items ];
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: Similar to the previous suggestion, this one correctly identifies an inconsistent data handling pattern. The refreshItems function merges newItem.data and newItem.payload, while the confirmEdit function also manually combines properties. The suggestion to consolidate the update source in refreshItems is a good step toward simplifying the logic and making it more robust.

Medium
Merge payload into data

Merging payload into a separate payload property may be ignored downstream where
data is expected to contain all fields. Either integrate the payload into data
or ensure all consumers use payload. To prevent data loss, merge the computed
payload back into data consistently with readers.

src/routes/page/knowledge-base/common/vector-table/vector-item-edit-modal.svelte [158-166]

 const newItem = {
     ...item,
     data: {
-        ...item?.data,
+        ...(item?.data || {}),
         text: question.text,
-        answer: answer.text
-    },
-    payload: obj || {}
+        answer: answer.text,
+        ...(obj || {})
+    }
 };

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that passing data and payload separately from the modal leads to more complex handling in the parent components, which currently have to merge them back together. Unifying them into a single data object in the modal improves code clarity and maintainability.

Medium
  • More

@iceljc iceljc marked this pull request as ready for review August 9, 2025 02:55
@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 9, 2025

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

Possible Issue

The edit modal constructs a new payload object and assigns it to e.payload, but the UI also displays data from item.data. This may cause divergence between data and payload fields after update (e.g., some non-text fields moved to payload and removed from data). Verify backend contract and ensure consistent rendering after edits.

/** @param {any} e */
function handleConfirm(e) {
    e.preventDefault();

    if (!allowPayload) {
        innerPayloads = [];
    }

    const validPayloads = innerPayloads.map(x => ({ key: util.trim(x.key), value: util.trim(x.value) }))
                                       .filter(x => !!x.key && !!x.value && !excludedPayloads.includes(x.key));

    const obj = validPayloads.reduce((acc, cur) => {
        // @ts-ignore
        acc[cur.key] = cur.value;
        return acc;
    }, {});

    const newItem = {
        ...item,
        data: {
            ...item?.data,
            text: question.text,
            answer: answer.text
        },
        payload: obj || {}
    };
    confirm?.(newItem);
UX/Accessibility

Clickable icons for remove actions use suppressed a11y rules and lack keyboard handlers. Consider adding key handlers and aria labels for better accessibility.

<div class="search-item-cb line-align-center" style="flex: 0 0 12px;">
    <div class="line-align-center">
        <!-- svelte-ignore a11y-click-events-have-key-events -->
        <!-- svelte-ignore a11y-no-static-element-interactions -->
        <i
            class="bx bx-no-entry text-danger clickable"
            class:hide={items.length === 1}
            on:click={() => removeItem(idx)}
        />
    </div>
</div>
Filtering Logic

Advanced filter groups are always passed even when empty, and selectedOperator defaults to 'or'. Confirm backend handling of empty groups and operator casing; ensure that adding text search also sets proper payload keys and that limit/fields are aligned with API expectations.

                isError = false;
            }, duration);
        }
    }

	/**
	 *  @param {any[]} items
	 */
	function buildSearchFilterGroups(items) {
		/** @type {any[]} */
		let groups = [];
		if (textSearch && !!text) {
			groups = [ ...groups, { filter_operator: 'or', filters: [{ key: KnowledgePayloadName.Text, value: text }] } ];
		}

		if (isAdvSearchOn && items?.length > 0) {
			const validItems = items.filter(x => x.checked && !!util.trim(x.key) && !!util.trim(x.value))
									.map(x => ({ key: util.trim(x.key), value: util.trim(x.value) }));
			groups = [ ...groups, { filter_operator: selectedOperator, filters: validItems } ];
		}

		return groups;
	}
</script>

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Inconsistent payload/schema changes

The PR introduces new request/response shapes (filter_groups, fields,
vector_dimension) and shifts edited data into payload, but updates appear only
in frontend types and UI. Ensure backend APIs, persistence, and existing
consumers are aligned to avoid runtime failures (e.g., searches ignoring
filters, edits dropping fields). Provide migration/compat layer so older data
with vector length but no vector_dimension and existing
search_pairs/included_payloads paths still work.

Examples:

src/routes/page/knowledge-base/documents/+page.svelte [327-333]
const filter = {
	size: pageSize,
	start_id: params.startId,
	with_vector: enableVector,
	fields: [],
	filter_groups: params.filterGroups
};
src/routes/page/knowledge-base/documents/+page.svelte [500-512]
e.payload = {
	...e.payload || {},
	dataSource: e.payload?.dataSource || VectorDataSource.User
};

if (!!editItem) {
	updateVectorKnowledgeData(
		e.id,
		editCollection,
		e.data?.text,

 ... (clipped 3 lines)

Solution Walkthrough:

Before:

// Old search logic
function getKnowledgeListData(params) {
    const searchPairs = buildSearchPairs(params);
    const filter = {
        // ...
        search_pairs: searchPairs
    };
    callApi(filter);
}

// Old update logic
function confirmEdit(event) {
    const { text, ...payload } = event.data;
    updateApi(event.id, text, payload);
}

After:

// New search logic
function getKnowledgeListData(params) {
    const filter = {
        // ...
        filter_groups: params.filterGroups
    };
    callApi(filter);
}

// New update logic
function confirmEdit(event) {
    const { data, payload } = event;
    updateApi(event.id, data.text, payload);
}
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical architectural flaw where frontend data contracts have been significantly altered without any corresponding backend changes in the PR, which will likely break core search and edit functionality.

High
Possible issue
Avoid invalid/duplicate payload fields

Ensure that excluded payload keys are not duplicated between data and payload.
Currently answer is added to data for all collection types, which is invalid for
document collections and may override or duplicate fields. Conditionally include
answer only for Q&A collections and keep other extra fields strictly in payload.

src/routes/page/knowledge-base/common/vector-table/vector-item-edit-modal.svelte [167-176]

+const baseData = {
+    ...item?.data,
+    text: question.text
+};
+
+const dataForCollection = isQuestionAnswerCollection
+    ? { ...baseData, answer: answer.text }
+    : baseData;
+
 const newItem = {
     ...item,
-    data: {
-        ...item?.data,
-        text: question.text,
-        answer: answer.text
-    },
+    data: dataForCollection,
     payload: obj || {}
 };
 confirm?.(newItem);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that unconditionally adding answer to data is problematic for document collections and proposes a valid conditional logic to prevent this potential issue.

Medium
General
Remove redundant change handler

Using both bind:group and an explicit on:change handler can cause double updates
or race conditions. Since you already bind operator, remove the manual on:change
handler and rely on binding to keep state consistent. This avoids redundant
state updates.

src/routes/page/knowledge-base/common/search/advanced-search.svelte [250-258]

 <Input
     type="radio"
     id={op.id}
     name="searchOperator"
     value={op.value}
     disabled={disabled}
     bind:group={operator}
-    on:change={e => changeLogicalOperator(e)}
 />
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that using both bind:group and on:change is redundant, and removing the on:change handler is a valid simplification for better code clarity.

Low
  • More

@Oceania2018 Oceania2018 merged commit e90c9ad into SciSharp:main Aug 9, 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