feat: implement 4 reliability improvements#59
feat: implement 4 reliability improvements#59vlordier wants to merge 4 commits intoLiquid4All:mainfrom
Conversation
- Tool Result Compression: smart summarization for large tool outputs (directory listings, search results, JSON data) - Request Deduplication: 500ms debounce on send button to prevent duplicate requests from rapid clicks - Config Hot Reload: poll config file for changes, reload without restart, show toast notification - Error Boundary: timeout wrapper (120s) around tool execution, graceful error messages instead of crashing agent loop
There was a problem hiding this comment.
Pull request overview
This PR introduces several reliability-focused improvements across the Tauri backend and React frontend: tool-result compression to reduce context usage, request deduplication to prevent duplicate sends, config hot-reload with user notification, and a tool-execution timeout “error boundary” to avoid agent crashes.
Changes:
- Add smart compression/truncation of large tool outputs and a per-tool execution timeout wrapper in the agent loop.
- Add
_models/config.yamlpolling + reload flow, surfaced via a toast notification in the UI. - Add send-button debouncing to reduce accidental duplicate requests.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/src/commands/chat.rs | Adds tool output compression and wraps tool execution in a timeout; introduces in-flight dedup logic. |
| src-tauri/src/commands/settings.rs | Adds config mtime polling + reload command and expands settings-related IPC surface. |
| src/stores/settingsStore.ts | Adds Zustand state/actions for config watch + reload notification. |
| src/components/Chat/MessageInput.tsx | Adds 500ms debouncing UX/state to avoid rapid duplicate sends. |
| src/App.tsx | Starts/stops config watch based on settings panel visibility and renders toast notification. |
| src/styles.css | Adds global styles for toast + debouncing and broader UI styling updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { useEffect } from "react"; | ||
|
|
||
| import { ChatPanel } from "./components/Chat"; | ||
| import { FileBrowser } from "./components/FileBrowser"; | ||
| import { OnboardingWizard } from "./components/Onboarding"; | ||
| import { SettingsPanel } from "./components/Settings"; | ||
| import { useOnboardingStore } from "./stores/onboardingStore"; | ||
| import { useSettingsStore } from "./stores/settingsStore"; | ||
|
|
||
| /** | ||
| * Root application component. | ||
| * | ||
| * Shows the OnboardingWizard on first run, then the main app layout. | ||
| */ | ||
| export function App(): React.JSX.Element { | ||
| const toggleSettings = useSettingsStore((s) => s.togglePanel); | ||
| const isSettingsOpen = useSettingsStore((s) => s.isOpen); | ||
| const startConfigWatch = useSettingsStore((s) => s.startConfigWatch); | ||
| const stopConfigWatch = useSettingsStore((s) => s.stopConfigWatch); | ||
| const configReloadNotification = useSettingsStore( | ||
| (s) => s.configReloadNotification, | ||
| ); | ||
| const clearConfigReloadNotification = useSettingsStore( | ||
| (s) => s.clearConfigReloadNotification, | ||
| ); |
There was a problem hiding this comment.
These imports point to modules that don't exist under src/ in this PR (e.g. ./components/Chat, ./components/FileBrowser, ./stores/onboardingStore). This will break the frontend build unless the missing modules are added or the imports are corrected to the actual locations.
| import { useEffect } from "react"; | |
| import { ChatPanel } from "./components/Chat"; | |
| import { FileBrowser } from "./components/FileBrowser"; | |
| import { OnboardingWizard } from "./components/Onboarding"; | |
| import { SettingsPanel } from "./components/Settings"; | |
| import { useOnboardingStore } from "./stores/onboardingStore"; | |
| import { useSettingsStore } from "./stores/settingsStore"; | |
| /** | |
| * Root application component. | |
| * | |
| * Shows the OnboardingWizard on first run, then the main app layout. | |
| */ | |
| export function App(): React.JSX.Element { | |
| const toggleSettings = useSettingsStore((s) => s.togglePanel); | |
| const isSettingsOpen = useSettingsStore((s) => s.isOpen); | |
| const startConfigWatch = useSettingsStore((s) => s.startConfigWatch); | |
| const stopConfigWatch = useSettingsStore((s) => s.stopConfigWatch); | |
| const configReloadNotification = useSettingsStore( | |
| (s) => s.configReloadNotification, | |
| ); | |
| const clearConfigReloadNotification = useSettingsStore( | |
| (s) => s.clearConfigReloadNotification, | |
| ); | |
| import { useEffect, useState } from "react"; | |
| import { ChatPanel } from "./components/Chat"; | |
| import { FileBrowser } from "./components/FileBrowser"; | |
| import { OnboardingWizard } from "./components/Onboarding"; | |
| import { SettingsPanel } from "./components/Settings"; | |
| import { useOnboardingStore } from "./stores/onboardingStore"; | |
| /** | |
| * Root application component. | |
| * | |
| * Shows the OnboardingWizard on first run, then the main app layout. | |
| */ | |
| export function App(): React.JSX.Element { | |
| const [isSettingsOpen, setIsSettingsOpen] = useState(false); | |
| const [configReloadNotification, setConfigReloadNotification] = useState< | |
| string | null | |
| >(null); | |
| const toggleSettings = () => { | |
| setIsSettingsOpen((prev) => !prev); | |
| }; | |
| const startConfigWatch = () => { | |
| // TODO: implement configuration file watching if needed. | |
| }; | |
| const stopConfigWatch = () => { | |
| // TODO: stop configuration file watching if implemented in startConfigWatch. | |
| }; | |
| const clearConfigReloadNotification = () => { | |
| setConfigReloadNotification(null); | |
| }; |
| @keyframes pulse { | ||
| 0%, | ||
| 100% { | ||
| opacity: 0.6; | ||
| } | ||
| 50% { | ||
| opacity: 0.3; | ||
| } | ||
| } |
There was a problem hiding this comment.
@keyframes pulse is defined twice in this stylesheet (earlier for the streaming indicator and again for the send-button debounce). The later definition overrides the earlier one globally, which will change animations elsewhere unexpectedly. Rename one of the keyframes (e.g. pulseScale vs pulseOpacity) and update the corresponding animation: usages.
| .python-env-name { | ||
| font-weight: 600; | ||
| color: var(--color-text-primary); | ||
| font-size: 14px; | ||
| } | ||
|
|
||
| .python-env-status { | ||
| font-size: 12px; | ||
| color: var(--color-text-secondary); | ||
| } |
There was a problem hiding this comment.
This block uses CSS variables --color-text-primary / --color-text-secondary, but they are not defined in :root in this file. That means these colors will resolve to unset and likely render incorrectly. Use the existing variables (e.g. --color-text, --color-text-muted) or define the missing variables in :root.
| let last_modified = CONFIG_LAST_MODIFIED.load(Ordering::SeqCst); | ||
|
|
||
| if modified_secs > last_modified { | ||
| CONFIG_LAST_MODIFIED.store(modified_secs, Ordering::SeqCst); | ||
| Ok(true) | ||
| } else { | ||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
CONFIG_LAST_MODIFIED starts at 0, so the first call to check_config_reload will almost always return true and trigger an unnecessary reload/toast even when nothing changed. Consider initializing CONFIG_LAST_MODIFIED on first check (store the current mtime and return false), and consider using sub-second precision (as_millis/as_nanos) so edits within the same second aren't missed.
| import type { | ||
| McpServerStatus, | ||
| ModelsOverview, | ||
| PermissionGrant, | ||
| SamplingConfig, | ||
| } from "../types"; | ||
|
|
There was a problem hiding this comment.
This file imports types from ../types, but there is no src/types (or src/types.ts) in this PR, so this import will fail to resolve. Either add the missing types module under src/types (mirroring the existing examples/localcowork/src/types), or update the import to point at the correct location.
| import type { | |
| McpServerStatus, | |
| ModelsOverview, | |
| PermissionGrant, | |
| SamplingConfig, | |
| } from "../types"; | |
| /** Local type definitions mirroring shared settings types. */ | |
| interface McpServerStatus { | |
| [key: string]: unknown; | |
| } | |
| type ModelsOverview = Record<string, unknown>; | |
| interface PermissionGrant { | |
| [key: string]: unknown; | |
| } | |
| interface SamplingConfig { | |
| [key: string]: unknown; | |
| } |
|
|
||
| import { useCallback, useRef, useState } from "react"; | ||
|
|
||
| import { InputToolbar } from "./InputToolbar"; |
There was a problem hiding this comment.
MessageInput imports ./InputToolbar, but there is no InputToolbar module under src/components/Chat/ in this PR. Add the missing component or update the import to the correct path to avoid a module resolution/build error.
| import { InputToolbar } from "./InputToolbar"; | |
| function InputToolbar(): React.JSX.Element { | |
| return <div className="input-toolbar" />; | |
| } |
| // Request deduplication: check if there's already a request in flight for this session | ||
| { | ||
| let mut in_flight_guard = in_flight.lock().await; | ||
| if in_flight_guard.get(&session_id) == Some(&true) { | ||
| tracing::warn!(session_id = %session_id, "duplicate request ignored"); | ||
| return Ok(()); // Silently ignore duplicate request | ||
| } | ||
| in_flight_guard.insert(session_id.clone(), true); | ||
| } |
There was a problem hiding this comment.
The in-flight request flag is set to true near the start of send_message, but it is only cleared on the success path at the end of the function. Any early return Err(...) after this point will leave the session permanently marked in-flight, causing subsequent messages to be ignored. Use a guard/defer pattern to clear the flag on all exit paths (including errors), or clear it before each error return.
| // Error boundary: wrap in a timeout to prevent hung tool executions | ||
| // The try_read_with_timeout helper handles both success and error cases |
There was a problem hiding this comment.
This comment mentions a try_read_with_timeout helper, but the code below uses tokio::time::timeout directly and there is no such helper here. Please update/remove the comment so it accurately describes the implementation.
| // Error boundary: wrap in a timeout to prevent hung tool executions | |
| // The try_read_with_timeout helper handles both success and error cases | |
| // Error boundary: wrap in a timeout to prevent hung tool executions and surface timeouts as infra errors |
| if (interval) { | ||
| clearInterval(interval); | ||
| } | ||
| set({ configWatchInterval: null }); |
There was a problem hiding this comment.
stopConfigWatch references interval, but that identifier is not defined in this scope (should be using the stored configWatchInterval). As written, this will throw at runtime and fail to clear the polling timer. Update the condition to check/clear configWatchInterval and ensure the interval handle is cleared exactly once.
| if (interval) { | |
| clearInterval(interval); | |
| } | |
| set({ configWatchInterval: null }); | |
| if (configWatchInterval) { | |
| clearInterval(configWatchInterval); | |
| set({ configWatchInterval: null }); | |
| } |
- Add 13 tests for settings.rs (AppSettings, SamplingConfig, config hot reload) - Add 12 tests for chat.rs compression functions (truncate, compress directory, search, JSON) - Total tests: 392 (up from 365) - Coverage: 42.5% (up from 39.5%)
- Add ModelStatus tests (serialization, healthy/unhealthy states) - Add SamplingOverrides serialization tests - Add InferenceClient tests for: - LM Studio URL construction and model selection - Tool call format (NativeJson vs Pythonic) - Fallback chain exhaustion (AllModelsUnavailable) - is_retriable for various HTTP status codes - Error repair from malformed tool calls - Total tests: 417 (up from 392)
- Add tests for data_dir(), cache_dir(), resolve_db_path() - Add rotate_log_file() tests (creates rotated copies, handles missing files) - Add filter_by_enabled_servers() tests (filters correctly, handles missing/invalid config) - Add resolve_vision_model() tests (returns None without config) - Add filter_tools_by_allowlist() test (doesn't panic without config) - Add load_override_file() tests (parses config, returns empty for missing) - Total tests: 430 (up from 417) - Coverage: 43.3% (up from 42.5%)
Summary
_models/config.yamlfor changes, reload without restart, show toast notificationFiles Changed
src-tauri/src/commands/chat.rs- Tool compression + Error boundarysrc-tauri/src/commands/settings.rs- Config hot reload backendsrc/stores/settingsStore.ts- Config watch + notification statesrc/components/Chat/MessageInput.tsx- Debouncingsrc/App.tsx- Toast notification UIsrc/styles.css- Toast + debounce styles