Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Nov 21, 2025

PR Type

Enhancement, Bug fix


Description

  • Refactor global loading and error handling to use centralized store-based approach

  • Relocate spinner positioning from fixed to absolute for better layout control

  • Enhance navigation to use SvelteKit's goto() instead of direct URL manipulation

  • Improve sidebar menu item selection and scrolling with URL-based ID matching

  • Add abort signal support for cancellable async requests in agent listing

  • Add page mount state tracking to prevent navigation callbacks after unmount


Diagram Walkthrough

flowchart LR
  A["Global Layout"] -->|removed loader logic| B["GlobalHeader Component"]
  B -->|subscribes to stores| C["loaderStore & errorStore"]
  C -->|provides state| D["VerticalLayout & ChatBox"]
  D -->|uses LoadingToComplete| E["Spinner Display"]
  F["Navigation"] -->|uses goto| G["SvelteKit Router"]
  G -->|replaces window.location| H["Client-side Navigation"]
Loading

File Walkthrough

Relevant files
Enhancement
22 files
LoadingToComplete.svelte
Add spinner styling customization props                                   
+8/-2     
GlobalHeader.svelte
Create new global header store subscription component       
+29/-0   
Index.svelte
Add GlobalHeader and LoadingToComplete integration             
+14/-1   
Sidebar.svelte
Refactor menu navigation and URL handling logic                   
+39/-26 
+page.svelte
Replace window.location with SvelteKit goto                           
+6/-4     
chat-box.svelte
Add GlobalHeader and refine spinner positioning                   
+13/-4   
+page.svelte
Add abort controller and page mount state tracking             
+22/-4   
+page.svelte
Refactor agent loading with reactive statements                   
+19/-6   
+page.svelte
Refactor agent loading with reactive statements                   
+17/-9   
card-agent.svelte
Replace Link with Button for navigation control                   
+23/-13 
+page.svelte
Add page mount state tracking for safe navigation               
+12/-12 
+page.svelte
Add dialog loading and refactor component structure           
+41/-27 
+page.svelte
Add page mount state tracking for safe navigation               
+8/-5     
+page.svelte
Add collection details fetching and conditional loading   
+31/-10 
+page.svelte
Add conditional collection detail loading                               
+12/-10 
+page.svelte
Add page mount state tracking for safe navigation               
+7/-1     
+page.svelte
Add loading state management and cleanup unused code         
+8/-9     
+page.svelte
Add page mount state tracking for safe navigation               
+14/-2   
_common.scss
Add text-btn styling for button-as-link elements                 
+14/-0   
http.js
Refine loader skip patterns and remove debug logs               
+20/-12 
common.js
Add getCleanUrl utility function                                                 
+13/-0   
agent-service.js
Add abort signal parameter for cancellable requests           
+4/-2     
Refactoring
2 files
+layout.svelte
Remove global loader and error handling logic                       
+0/-36   
conv-dialogs.svelte
Move dialog fetching to parent component                                 
+7/-9     
Formatting
7 files
+page.svelte
Clean up unused imports and variables                                       
+2/-4     
+layout.svelte
Reorder imports for consistency                                                   
+1/-2     
agent-rule.svelte
Format LoadingToComplete component usage                                 
+7/-1     
+page.svelte
Format LoadingToComplete and improve indentation                 
+37/-32 
+page.svelte
Format LoadingToComplete and improve indentation                 
+8/-3     
+page.svelte
Format LoadingToComplete component usage                                 
+7/-2     
+page.svelte
Format LoadingToComplete component usage                                 
+1/-0     
Bug fix
2 files
_loader.scss
Change loader positioning from fixed to absolute                 
+1/-1     
role-service.js
Change getRoles from POST to GET with query params             
+5/-1     

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 21, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Debug info exposure

Description: Debug logging of the token refresh retry queue length via console.log ('queue',
this.queue.length) was introduced, which can leak internal state and aid attackers in
probing auth flow or DoS conditions; remove verbose logs or guard them by environment.
http.js [31-35]

Referred Code
/** @param {{config: import('axios').InternalAxiosRequestConfig, resolve: (value: any) => void, reject: (reason?: any) => void}} item */
enqueue(item) {
    this.queue.push(item);
    console.log('queue', this.queue.length);
    if (!this.isRefreshingToken) {
Selector injection risk

Description: The code builds a CSS selector using an unescaped URL-derived value in
document.querySelector(".vertical-menu a[id='" + curUrl + "']"), which can break the
selector or enable DOM selection manipulation if an attacker can influence IDs via crafted
links; sanitize or escape selector components before concatenation.
Sidebar.svelte [20-25]

Referred Code
const curUrl = getCleanUrl($page.url.pathname);
if (curUrl) {
	const item = document.querySelector(".vertical-menu a[id='" + curUrl + "']");
	if (item) {
		item.classList.add('mm-active');
		const parent1 = item.parentElement;
Unvalidated DOM id from URL

Description: Navigation links set element id attributes from getCleanUrl(user-controlled link) and use
them in selectors, potentially allowing DOM manipulation or unexpected behavior if IDs
contain special characters; validate/escape IDs or use data attributes.
Sidebar.svelte [221-260]

Referred Code
	<li class="menu-title" key="t-menu">{$_(item.label)}</li>
{:else if item.subMenu}
	<li>
		<Link class="has-arrow waves-effect clickable" href={null}>
			<i class={item.icon} />
			<span>{$_(item.label)}</span>
		</Link>
		<ul class="sub-menu mm-collapse">
			{#each item.subMenu as subMenu}
				{#if subMenu.isChildItem}
					<li>
						<Link class="has-arrow waves-effect clickable" href={null}>
							<span>{$_(subMenu.label)}</span>
						</Link>
						<ul class="sub-menu mm-collapse">
							{#each subMenu.childItems as childItem}
								<li>
									<Link class="clickable" id={getCleanUrl(childItem.link)} href={null} on:click={() => goToPage(childItem.link)}>
										{$_(childItem.label)}
									</Link>
								</li>


 ... (clipped 19 lines)
Request cancellation race

Description: Added optional AbortSignal handling to getAgents without clearing or resetting the abort
controller in callers may lead to request cancellation race conditions; if attacker can
trigger rapid requests, this could cause inconsistent UI/state—ensure abort controllers
are properly scoped and errors handled distinctly.
agent-service.js [16-36]

Referred Code
 * Get agent list
 * @param {import('$agentTypes').AgentFilter} filter
 * @param {boolean} checkAuth
 * @param {AbortSignal | null} signal
 * @returns {Promise<import('$commonTypes').PagedItems<import('$agentTypes').AgentModel>>}
 */
export async function getAgents(filter, checkAuth = false, signal = null) {
    let url = endpoints.agentListUrl;
    const response = await axios.get(url, {
        params: {
            ...filter,
            checkAuth : checkAuth
        },
        paramsSerializer: {
            dots: true,
            indexes: null,
        },
        signal: signal || undefined
    });
    return response.data;
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console Logging: The code introduces a console.log of the retry queue length which could leak internal
details and should not appear in production-facing code.

Referred Code
/** @param {{config: import('axios').InternalAxiosRequestConfig, resolve: (value: any) => void, reject: (reason?: any) => void}} item */
enqueue(item) {
    this.queue.push(item);
    console.log('queue', this.queue.length);
    if (!this.isRefreshingToken) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Audit Logs: The PR adds navigation changes, abortable requests, and state changes across multiple
pages without adding or referencing any audit logging for critical actions, which may be
handled elsewhere but is not visible in this diff.

Referred Code
loaderStore.set(false);
const originalRequest = error?.config || {};
const user = getUserStore();

if (!user?.token || user.renew_token_count >= retryQueue.maxRenewTokenCount) {
    retryQueue.queue = [];
    redirectToLogin();

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Abort Handling: Aborting in-flight requests was added, but catch handlers in callers often swallow errors
without logging or user feedback, which may be intentional UI behavior but limits
diagnostics.

Referred Code
 * Get agent list
 * @param {import('$agentTypes').AgentFilter} filter
 * @param {boolean} checkAuth
 * @param {AbortSignal | null} signal
 * @returns {Promise<import('$commonTypes').PagedItems<import('$agentTypes').AgentModel>>}
 */
export async function getAgents(filter, checkAuth = false, signal = null) {
    let url = endpoints.agentListUrl;
    const response = await axios.get(url, {
        params: {
            ...filter,
            checkAuth : checkAuth
        },
        paramsSerializer: {
            dots: true,
            indexes: null,
        },
        signal: signal || undefined
    });
    return response.data;
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Nonstructured Log: The added console.log uses unstructured logging and may divulge operational queue size
without ensuring sensitive data exclusion; it might be acceptable during development but
should be removed or structured and gated by environment.

Referred Code
/** @param {{config: import('axios').InternalAxiosRequestConfig, resolve: (value: any) => void, reject: (reason?: any) => void}} item */
enqueue(item) {
    this.queue.push(item);
    console.log('queue', this.queue.length);
    if (!this.isRefreshingToken) {

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 21, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Re-evaluate the global loader implementation

The current method of using extensive URL regex lists in http.js to control the
global loader is not maintainable. A better approach would be to let individual
API calls or components decide whether to show the loader, making the system
more modular.

Examples:

src/lib/helpers/http.js [178-306]
function skipLoader(config) {
    /** @type {RegExp[]} */
    const postRegexes = [
        new RegExp('http(s*)://(.*?)/conversation/(.*?)/(.*?)', 'g'),
        new RegExp('http(s*)://(.*?)/agent', 'g'),
        new RegExp('http(s*)://(.*?)/knowledge/vector/(.*?)/page', 'g'),
        new RegExp('http(s*)://(.*?)/knowledge/(.*?)/search', 'g'),
        new RegExp('http(s*)://(.*?)/knowledge/vector/(.*?)/create', 'g'),
        new RegExp('http(s*)://(.*?)/knowledge/document/(.*?)/page', 'g'),
        new RegExp('http(s*)://(.*?)/users', 'g'),

 ... (clipped 119 lines)

Solution Walkthrough:

Before:

// src/lib/helpers/http.js
axios.interceptors.request.use(
    (config) => {
        if (!skipLoader(config)) {
            loaderStore.set(true);
        }
        // ...
    }
);

function skipLoader(config) {
    const getRegexes = [
        new RegExp('http(s*)://(.*?)/plugin/menu', 'g'),
        new RegExp('http(s*)://(.*?)/setting/(.*?)', 'g'),
        // ... many more regexes
    ];

    if (config.method === 'get' && getRegexes.some(regex => regex.test(config.url || ''))) {
        return true;
    }
    // ...
    return false;
}

After:

// src/lib/helpers/http.js
axios.interceptors.request.use(
    (config) => {
        // Default to showing loader unless explicitly told not to.
        if (config.showLoader !== false) {
            loaderStore.set(true);
        }
        // ...
    }
);

// src/lib/services/agent-service.js
export async function getAgents(filter, checkAuth = false, signal = null, showLoader = true) {
    let url = endpoints.agentListUrl;
    const response = await axios.get(url, {
        params: { ... },
        signal: signal,
        showLoader: showLoader // Pass config to interceptor
    });
    return response.data;
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a brittle, hard-to-maintain regex-based approach for managing the global loader in http.js and proposes a more robust, decentralized design.

High
Possible issue
Prevent race conditions on unmount

Prevent a race condition by aborting the getAgents API request in the onDestroy
lifecycle hook. This ensures callbacks do not execute on an unmounted component.

src/routes/page/agent/+page.svelte [110-134]

 	onDestroy(() => {
 		isPageMounted = false;
 		unsubscriber?.();
+		abortController?.abort();
 	});
 
   	function getPagedAgents() {
 		isLoading = true;
 
 		if (abortController) {
 			abortController.abort();
 		}
 		abortController = new AbortController();
 
 		const innerFilter = {
 ...
 		};
     	getAgents(innerFilter, true, abortController?.signal).then(data => {
 			agents = data;
-		}).catch(() => {
-			agents = { items: [], count: 0 };
+		}).catch((err) => {
+			if (err.name !== 'CanceledError') {
+				agents = { items: [], count: 0 };
+			}
 		}).finally(() => {
+			if (abortController?.signal.aborted) return;
 			isLoading = false;
 			refreshPager(agents.count);
 		});
 	}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a race condition where a promise can resolve after component unmount, leading to errors. Aborting the request in onDestroy is the correct way to fix this potential bug.

Medium
Ensure user data is refetched

To prevent stale user data when the agentId changes, move the myInfo() call from
onMount into the loadAgent function. This ensures user information is refetched
along with agent data.

src/routes/page/agent/[agentId]/+page.svelte [45-67]

 $: if (agentId) {
     loadAgent(agentId);
 }
 
 async function loadAgent(/** @type {string} */ id) {
     isLoading = true;
-    agent = await getAgent(id);
+    [agent, user] = await Promise.all([
+        getAgent(id),
+        myInfo()
+    ]);
     isLoading = false;
 }
 
 onMount(async () => {
-    user = await myInfo();
-
     unsubscriber = globalEventStore.subscribe((/** @type {import('$commonTypes').GlobalEvent} */ event) => {
 			if (event.name !== GlobalEvent.Search) return;
 
         const similarName = event.payload?.trim();
         if (similarName) {
             const url = `page/agent?page=${1}&pageSize=${12}&similarName=${encodeURIComponent(similarName)}`;
             window.location.href = url;
         }
 		});
 });
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that user data can become stale if agentId changes, as myInfo() is only called once on mount. Moving the call into loadAgent ensures data consistency and is a good practice for data loading tied to route parameters.

Medium
General
Improve navigation accessibility and UX

Improve navigation accessibility and user experience by setting the href
attribute on Link components to the destination URL instead of using href={null}
with an on:click handler for navigation.

src/routes/VerticalLayout/Sidebar.svelte [237-241]

 <li>
-    <Link class="clickable" id={getCleanUrl(childItem.link)} href={null} on:click={() => goToPage(childItem.link)}>
+    <Link class="clickable" id={getCleanUrl(childItem.link)} href={childItem.link} on:click={() => goToPage()}>
         {$_(childItem.label)}
     </Link>
 </li>
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using href={null} with an on:click handler for navigation is an anti-pattern that harms accessibility and user experience. Restoring the href attribute is a significant improvement.

Medium
  • Update

@iceljc iceljc merged commit 2dbf61e into SciSharp:main Nov 22, 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