Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Aug 11, 2025

PR Type

Enhancement


Description

  • Add time range filtering to conversation and instruction log pages

  • Implement time range dropdown with predefined options (15 minutes to 1 year)

  • Update search functionality to include start/end time parameters

  • Minor UI improvements and code cleanup


Diagram Walkthrough

flowchart LR
  A["Time Range Enum"] --> B["Time Range Options"]
  B --> C["Convert Time Range Function"]
  C --> D["Conversation Page Filter"]
  C --> E["Instruction Log Filter"]
  D --> F["Search API Calls"]
  E --> F
Loading

File Walkthrough

Relevant files
Enhancement
+page.svelte
Add time range filtering to conversation page                       

src/routes/page/conversation/+page.svelte

  • Add time range dropdown with default "Last 3 Hours" selection
  • Integrate time range conversion into search filters
  • Update search API calls to include startTime/endTime parameters
  • Comment out task selection column temporarily
+49/-19 
+page.svelte
Add time range filtering to instruction log page                 

src/routes/page/instruction/log/+page.svelte

  • Add time range dropdown with default "Today" selection
  • Implement time range filtering in instruction log search
  • Update placeholder text for better consistency
  • Adjust column layout to accommodate new time range filter
+45/-8   
enums.js
Add TimeRange enum definition                                                       

src/lib/helpers/enums.js

  • Add TimeRange enum with 13 time range values
  • Include options from 15 minutes to 1 year
  • Freeze enum object for immutability
+18/-1   
conversationTypes.js
Update conversation types for time filtering                         

src/lib/helpers/types/conversationTypes.js

  • Add startTime and endTime fields to ConversationFilter type
  • Add startTime and endTime fields to StateSearchQuery type
  • Add timeRange field to ConversationSearchOption type
+5/-0     
instructTypes.js
Update instruction types for time filtering                           

src/lib/helpers/types/instructTypes.js

  • Add startTime and endTime fields to InstructLogFilter type
  • Add startTime and endTime fields to StateSearchQuery type
+4/-0     
common.js
Implement time range conversion utility                                   

src/lib/helpers/utils/common.js

  • Add convertTimeRange function to convert time range enums to date
    strings
  • Import moment.js for date manipulation
  • Handle various time range cases including relative and absolute ranges
+60/-0   
Miscellaneous
vector-item.svelte
Simplify payload key display formatting                                   

src/routes/page/knowledge-base/common/vector-table/vector-item.svelte

  • Remove text case splitting for payload key display
  • Simplify key display to show original key names
+1/-1     
Configuration changes
constants.js
Define time range options constants                                           

src/lib/helpers/constants.js

  • Add TIME_RANGE_OPTIONS array with 13 predefined time ranges
  • Include quantity and unit metadata for each time range option
  • Import TimeRange enum for option definitions
+18/-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

Possible Issue

The convertTimeRange function never sets endTime for most ranges, leading to an open-ended range (now) that may not match backend expectations or cause inconsistent filtering. Confirm API requires explicit endTime and align behavior (e.g., set endTime to current UTC for non-Yesterday ranges).

export function convertTimeRange(timeRange) {
    let ret = { startTime: null, endTime: null };

    if (!timeRange) {
        return ret;
    }

    const found = TIME_RANGE_OPTIONS.find(x => x.value === timeRange);
    if (!found) {
        return ret;
    }

    switch (found.value) {
        case TimeRange.Last15Minutes:
        case TimeRange.Last30Minutes:
        case TimeRange.Last1Hour:
        case TimeRange.Last3Hours:
        case TimeRange.Last12Hours:
        case TimeRange.Last3Days:
        case TimeRange.Last7Days:
        case TimeRange.Last30Days:
        case TimeRange.Last90Days:
        case TimeRange.Last180Days:
        case TimeRange.LastYear:
            ret = {
                ...ret,
                // @ts-ignore
                startTime: moment().subtract(found.qty, found.unit).utc().format()
            };
            break;
        case TimeRange.Today:
            ret = {
                ...ret,
                // @ts-ignore
                startTime: moment().startOf('day').utc().format()
            };
            break;
        case TimeRange.Yesterday:
            ret = {
                ...ret,
                // @ts-ignore
                startTime: moment().subtract(1, 'days').startOf('day').utc().format(),
                // @ts-ignore
                endTime: moment().subtract(1, 'days').endOf('day').utc().format()
            };
            break;
        default:
            break;
    }

    return ret;
i18n/Value Coupling

TimeRange enum uses human-readable labels as values. These values are sent around as identifiers and matched in TIME_RANGE_OPTIONS. This couples UI text with logic and localization, risking breakage if labels are translated. Consider stable keys (e.g., LAST_15_MINUTES) and separate display labels.

const timeRange = {
    Last15Minutes: "Last 15 minutes",
    Last30Minutes: "Last 30 minutes",
    Last1Hour: "Last 1 hour",
    Last3Hours: "Last 3 hours",
    Last12Hours: "Last 12 hours",
    Today: "Today",
    Yesterday: "Yesterday",
    Last3Days: "Last 3 days",
    Last7Days: "Last 7 days",
    Last30Days: "Last 30 days",
    Last90Days: "Last 90 days",
    Last180Days: "Last 180 days",
    LastYear: "Last year"
};
export const TimeRange = Object.freeze(timeRange);
State Sync

timeRange is added to searchOption and used to compute innerTimeRange, but URL/query param sync and initial default handling might be inconsistent. Ensure timeRange is included in setUrlQueryParams/getPagingQueryParams flow and preserved on navigation to avoid surprising resets.

	/** @type {import('$conversationTypes').ConversationSearchOption} */
	let searchOption = {
		agentId: null,
		agentIds: [],
		channel: null,
		status: null,
		taskId: null,
		timeRange: TimeRange.Last3Hours,
		states: [],
		tags: []
	};

	/** @type {{key: string, value: string | null}[]} */
    let states = [
        { key: '', value: ''}
    ];

    onMount(async () => {
		const { pageNum, pageSizeNum } = getPagingQueryParams({
			page: $page.url.searchParams.get("page"),
			pageSize: $page.url.searchParams.get("pageSize")
		}, { defaultPageSize: pageSize });
		innerTimeRange = convertTimeRange(searchOption.timeRange || '');

		filter = {
			...filter,
			startTime: innerTimeRange.startTime,
			endTime: innerTimeRange.endTime,
			pager: {
				...filter.pager,
				page: pageNum,
				size: pageSizeNum
			}
		};

		isLoading = true;
		try {
			await Promise.all([
				loadAgentOptions(),
				loadSearchOption(),
				loadConversations()
			]);
		} catch (error) {
			console.error('Error loading data:', error);

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 11, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Always set end time for ranges

For relative ranges you only set startTime and omit endTime, which can cause
drifting results and inconsistent server filtering. Set endTime explicitly to
current time in UTC for all relative ranges, and for Today set endTime to end of
day for consistency. This avoids open-ended queries and ensures predictable
ranges.

src/lib/helpers/utils/common.js [167-218]

 export function convertTimeRange(timeRange) {
     let ret = { startTime: null, endTime: null };
-
-    if (!timeRange) {
-        return ret;
-    }
+    if (!timeRange) return ret;
 
     const found = TIME_RANGE_OPTIONS.find(x => x.value === timeRange);
-    if (!found) {
-        return ret;
-    }
+    if (!found) return ret;
 
     switch (found.value) {
         case TimeRange.Last15Minutes:
         case TimeRange.Last30Minutes:
         case TimeRange.Last1Hour:
         case TimeRange.Last3Hours:
         case TimeRange.Last12Hours:
         case TimeRange.Last3Days:
         case TimeRange.Last7Days:
         case TimeRange.Last30Days:
         case TimeRange.Last90Days:
         case TimeRange.Last180Days:
         case TimeRange.LastYear:
             ret = {
-                ...ret,
-                // @ts-ignore
-                startTime: moment().subtract(found.qty, found.unit).utc().format()
+                startTime: moment().subtract(found.qty, found.unit).utc().format(),
+                endTime: moment().utc().format()
             };
             break;
         case TimeRange.Today:
             ret = {
-                ...ret,
-                // @ts-ignore
-                startTime: moment().startOf('day').utc().format()
+                startTime: moment().startOf('day').utc().format(),
+                endTime: moment().endOf('day').utc().format()
             };
             break;
         case TimeRange.Yesterday:
             ret = {
-                ...ret,
-                // @ts-ignore
                 startTime: moment().subtract(1, 'days').startOf('day').utc().format(),
-                // @ts-ignore
                 endTime: moment().subtract(1, 'days').endOf('day').utc().format()
             };
             break;
         default:
             break;
     }
-
     return ret;
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that not setting endTime for relative time ranges can lead to inconsistent filtering results, and the proposed change makes the time ranges explicit and predictable.

Medium
General
Remove misleading unused fields

Including qty and unit for Today and Yesterday is misleading since they are not
used as relative offsets in the switch. Remove unused fields for clarity or
ensure consumer logic doesn’t incorrectly rely on them. This reduces risk of
misapplication elsewhere.

src/lib/helpers/constants.js [50-64]

 export const TIME_RANGE_OPTIONS = [
     { label: TimeRange.Last15Minutes, value: TimeRange.Last15Minutes, qty: 15, unit: 'minutes' },
     { label: TimeRange.Last30Minutes, value: TimeRange.Last30Minutes, qty: 30, unit: 'minutes' },
     { label: TimeRange.Last1Hour, value: TimeRange.Last1Hour, qty: 1, unit: 'hours' },
     { label: TimeRange.Last3Hours, value: TimeRange.Last3Hours, qty: 3, unit: 'hours' },
     { label: TimeRange.Last12Hours, value: TimeRange.Last12Hours, qty: 12, unit: 'hours' },
-    { label: TimeRange.Today, value: TimeRange.Today, qty: 1, unit: 'days' },
-    { label: TimeRange.Yesterday, value: TimeRange.Yesterday, qty: 1, unit: 'days' },
+    { label: TimeRange.Today, value: TimeRange.Today },
+    { label: TimeRange.Yesterday, value: TimeRange.Yesterday },
     { label: TimeRange.Last3Days, value: TimeRange.Last3Days, qty: 3, unit: 'days' },
     { label: TimeRange.Last7Days, value: TimeRange.Last7Days, qty: 7, unit: 'days' },
     { label: TimeRange.Last30Days, value: TimeRange.Last30Days, qty: 30, unit: 'days' },
     { label: TimeRange.Last90Days, value: TimeRange.Last90Days, qty: 90, unit: 'days' },
     { label: TimeRange.Last180Days, value: TimeRange.Last180Days, qty: 180, unit: 'days' },
     { label: TimeRange.LastYear, value: TimeRange.LastYear, qty: 365, unit: 'days' }
 ];
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that the qty and unit fields for Today and Yesterday are unused in the convertTimeRange function, and removing them improves code clarity and maintainability.

Low
  • Update

@iceljc iceljc marked this pull request as draft August 12, 2025 02:21
@iceljc iceljc marked this pull request as ready for review August 12, 2025 14:25
@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

Possible Issue

The convertTimeRange function never sets an endTime for most ranges (e.g., Last7Days), implicitly meaning "until now". Confirm backend expects null vs explicit current time, and ensure timezone/format (UTC ISO) matches API requirements to avoid filtering mismatches.

/**
 * @param {string} timeRange
 * @returns {{ startTime: string | null, endTime: string | null }}
 */
export function convertTimeRange(timeRange) {
    let ret = { startTime: null, endTime: null };

    if (!timeRange) {
        return ret;
    }

    const found = TIME_RANGE_OPTIONS.find(x => x.value === timeRange);
    if (!found) {
        return ret;
    }

    switch (found.value) {
        case TimeRange.Last15Minutes:
        case TimeRange.Last30Minutes:
        case TimeRange.Last1Hour:
        case TimeRange.Last3Hours:
        case TimeRange.Last12Hours:
        case TimeRange.Last3Days:
        case TimeRange.Last7Days:
        case TimeRange.Last30Days:
        case TimeRange.Last90Days:
        case TimeRange.Last180Days:
        case TimeRange.LastYear:
            ret = {
                ...ret,
                // @ts-ignore
                startTime: moment().subtract(found.qty, found.unit).utc().format()
            };
            break;
        case TimeRange.Today:
            ret = {
                ...ret,
                // @ts-ignore
                startTime: moment().startOf('day').utc().format()
            };
            break;
        case TimeRange.Yesterday:
            ret = {
                ...ret,
                // @ts-ignore
                startTime: moment().subtract(1, 'days').startOf('day').utc().format(),
                // @ts-ignore
                endTime: moment().subtract(1, 'days').endOf('day').utc().format()
            };
            break;
        default:
            break;
    }

    return ret;
State Sync

innerTimeRange is derived from searchOption.timeRange, but refreshFilter is only called in certain flows. Changing the time range updates searchOption yet won’t affect results until a manual search; verify UX and consider triggering refresh/search on selection or disabling stale state usage in handleStateSearch.

	} 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] : []
		};
	} else if (type === 'timeRange') {
		searchOption = {
			...searchOption,
			timeRange: selectedValues.length > 0 ? selectedValues[0] : null
		};
	}
}
I18n Consistency

TIME_RANGE_OPTIONS labels use English strings from TimeRange enum. If UI uses i18n, labels may bypass translation. Consider mapping to translation keys or using $_() at render sites to keep localization consistent.

export const TIME_RANGE_OPTIONS = [
    { label: TimeRange.Last15Minutes, value: TimeRange.Last15Minutes, qty: 15, unit: 'minutes' },
    { label: TimeRange.Last30Minutes, value: TimeRange.Last30Minutes, qty: 30, unit: 'minutes' },
    { label: TimeRange.Last1Hour, value: TimeRange.Last1Hour, qty: 1, unit: 'hours' },
    { label: TimeRange.Last3Hours, value: TimeRange.Last3Hours, qty: 3, unit: 'hours' },
    { label: TimeRange.Last12Hours, value: TimeRange.Last12Hours, qty: 12, unit: 'hours' },
    { label: TimeRange.Today, value: TimeRange.Today, qty: 1, unit: 'days' },
    { label: TimeRange.Yesterday, value: TimeRange.Yesterday, qty: 1, unit: 'days' },
    { label: TimeRange.Last3Days, value: TimeRange.Last3Days, qty: 3, unit: 'days' },
    { label: TimeRange.Last7Days, value: TimeRange.Last7Days, qty: 7, unit: 'days' },
    { label: TimeRange.Last30Days, value: TimeRange.Last30Days, qty: 30, unit: 'days' },
    { label: TimeRange.Last90Days, value: TimeRange.Last90Days, qty: 90, unit: 'days' },
    { label: TimeRange.Last180Days, value: TimeRange.Last180Days, qty: 180, unit: 'days' },
    { label: TimeRange.LastYear, value: TimeRange.LastYear, qty: 365, unit: 'days' }
];

@qodo-merge-pro
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Timezone handling risks

The convertTimeRange function uses moment().utc().format(), but the UI defaults
and API filters may expect local time or ISO with explicit Z; without clear
backend contract this can mis-filter records around day boundaries (e.g.,
Today/Yesterday). Confirm backend expects UTC ISO strings and align semantics
for "Today"/"Yesterday" (local vs UTC), otherwise compute start/end in the
server’s expected timezone or include timezone in requests.

Examples:

src/lib/helpers/utils/common.js [197-212]
case TimeRange.Today:
    ret = {
        ...ret,
        // @ts-ignore
        startTime: moment().startOf('day').utc().format()
    };
    break;
case TimeRange.Yesterday:
    ret = {
        ...ret,

 ... (clipped 6 lines)

Solution Walkthrough:

Before:

// In convertTimeRange function
// 'Today' is based on the user's local start of day
case TimeRange.Today:
    startTime = moment().startOf('day').utc().format();
    break;
// 'Yesterday' is also based on the user's local timezone
case TimeRange.Yesterday:
    startTime = moment().subtract(1, 'days').startOf('day').utc().format();
    endTime = moment().subtract(1, 'days').endOf('day').utc().format();
    break;

After:

// In convertTimeRange function
// 'Today' should be based on UTC start of day
case TimeRange.Today:
    startTime = moment.utc().startOf('day').format();
    break;
// 'Yesterday' should also be based on UTC timezone
case TimeRange.Yesterday:
    startTime = moment.utc().subtract(1, 'days').startOf('day').format();
    endTime = moment.utc().subtract(1, 'days').endOf('day').format();
    break;
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical and subtle bug in timezone handling for date-boundary filters ('Today', 'Yesterday'), which could lead to incorrect data filtering based on the user's local timezone.

High
Possible issue
Normalize and validate time outputs

Ensure the returned date strings are in a stable ISO format without timezone
ambiguity. Explicitly use ISO 8601 with 'Z' to avoid parsing issues server-side,
and handle invalid units defensively. Also, remove the ts-ignore comments by
guarding types at runtime.

src/lib/helpers/utils/common.js [167-218]

 export function convertTimeRange(timeRange) {
     let ret = { startTime: null, endTime: null };
-
-    if (!timeRange) {
-        return ret;
-    }
+    if (!timeRange) return ret;
 
     const found = TIME_RANGE_OPTIONS.find(x => x.value === timeRange);
-    if (!found) {
-        return ret;
-    }
+    if (!found) return ret;
+
+    const toIsoUtc = (m) => m.utc().toDate().toISOString();
+    const isValidUnit = (u) => ['minutes','hours','days'].includes(u);
 
     switch (found.value) {
         case TimeRange.Last15Minutes:
         case TimeRange.Last30Minutes:
         case TimeRange.Last1Hour:
         case TimeRange.Last3Hours:
         case TimeRange.Last12Hours:
         case TimeRange.Last3Days:
         case TimeRange.Last7Days:
         case TimeRange.Last30Days:
         case TimeRange.Last90Days:
         case TimeRange.Last180Days:
         case TimeRange.LastYear:
-            ret = {
-                ...ret,
-                // @ts-ignore
-                startTime: moment().subtract(found.qty, found.unit).utc().format()
-            };
+            if (typeof found.qty === 'number' && isValidUnit(found.unit)) {
+                ret = {
+                    ...ret,
+                    startTime: toIsoUtc(moment().subtract(found.qty, found.unit))
+                };
+            }
             break;
         case TimeRange.Today:
             ret = {
                 ...ret,
-                // @ts-ignore
-                startTime: moment().startOf('day').utc().format()
+                startTime: toIsoUtc(moment().startOf('day'))
             };
             break;
         case TimeRange.Yesterday:
             ret = {
                 ...ret,
-                // @ts-ignore
-                startTime: moment().subtract(1, 'days').startOf('day').utc().format(),
-                // @ts-ignore
-                endTime: moment().subtract(1, 'days').endOf('day').utc().format()
+                startTime: toIsoUtc(moment().subtract(1, 'days').startOf('day')),
+                endTime: toIsoUtc(moment().subtract(1, 'days').endOf('day'))
             };
             break;
         default:
             break;
     }
-
     return ret;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly improves the robustness of the convertTimeRange function by adding runtime validation for qty and unit, and makes the date format more explicit, which are good practices.

Low
General
Remove misleading metadata

Prevent misuse by removing qty/unit metadata for non-relative ranges like
"Today" and "Yesterday", which are handled specially. This avoids accidental
subtraction logic if the switch is extended and reduces ambiguity.

src/lib/helpers/constants.js [50-64]

 export const TIME_RANGE_OPTIONS = [
     { label: TimeRange.Last15Minutes, value: TimeRange.Last15Minutes, qty: 15, unit: 'minutes' },
     { label: TimeRange.Last30Minutes, value: TimeRange.Last30Minutes, qty: 30, unit: 'minutes' },
     { label: TimeRange.Last1Hour, value: TimeRange.Last1Hour, qty: 1, unit: 'hours' },
     { label: TimeRange.Last3Hours, value: TimeRange.Last3Hours, qty: 3, unit: 'hours' },
     { label: TimeRange.Last12Hours, value: TimeRange.Last12Hours, qty: 12, unit: 'hours' },
-    { label: TimeRange.Today, value: TimeRange.Today, qty: 1, unit: 'days' },
-    { label: TimeRange.Yesterday, value: TimeRange.Yesterday, qty: 1, unit: 'days' },
+    { label: TimeRange.Today, value: TimeRange.Today },
+    { label: TimeRange.Yesterday, value: TimeRange.Yesterday },
     { label: TimeRange.Last3Days, value: TimeRange.Last3Days, qty: 3, unit: 'days' },
     { label: TimeRange.Last7Days, value: TimeRange.Last7Days, qty: 7, unit: 'days' },
     { label: TimeRange.Last30Days, value: TimeRange.Last30Days, qty: 30, unit: 'days' },
     { label: TimeRange.Last90Days, value: TimeRange.Last90Days, qty: 90, unit: 'days' },
     { label: TimeRange.Last180Days, value: TimeRange.Last180Days, qty: 180, unit: 'days' },
     { label: TimeRange.LastYear, value: TimeRange.LastYear, qty: 365, unit: 'days' }
 ];
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: This is a valid suggestion that improves code clarity and maintainability by removing unused and potentially misleading qty and unit properties for specific time range options.

Low
  • More

@iceljc iceljc merged commit 93624e5 into SciSharp:main Aug 12, 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