fix: add _models/.cache/ to gitignore and HF cache comment#64
fix: add _models/.cache/ to gitignore and HF cache comment#64vlordier wants to merge 6 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
- 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%)
Address Copilot AI review comments from PR Liquid4All#54: - Add _models/.cache/ to .gitignore to prevent accidental cache commits - Add comment about HuggingFace cache location (HF_HOME, HF_HUB_CACHE, XDG_CACHE_HOME)
There was a problem hiding this comment.
Pull request overview
This PR is described as a small fix to prevent HuggingFace cache artifacts from being committed and to document HF cache env vars, but the diff also introduces substantial new frontend UI/state and Rust/Tauri backend behavior (settings store, config hot-reload polling, inference client/type updates, and new tests).
Changes:
- Add
_models/.cache/to the example app’s.gitignoreand document HuggingFace cache env vars inconfig.yaml. - Add new frontend settings/config-watch plumbing (Zustand store, UI toast, message input debouncing) and large global stylesheet.
- Add/modify Rust/Tauri commands and inference client/types, plus new/updated tests.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/styles.css | Adds a large global stylesheet including new UI elements and animations. |
| src/stores/settingsStore.ts | Introduces a Zustand settings store with config hot-reload polling. |
| src/components/Chat/MessageInput.tsx | Adds debounced send behavior and an input toolbar. |
| src/App.tsx | Adds a config-reload toast and starts/stops config watching. |
| src-tauri/src/lib.rs | Adds/updates app setup logic and a substantial set of unit tests. |
| src-tauri/src/inference/types.rs | Adds/updates shared inference request/response types and tests. |
| src-tauri/src/inference/client.rs | Adds/updates inference client logic (streaming, repair, status) and tests. |
| src-tauri/src/commands/settings.rs | Adds config hot-reload check logic and settings-related commands/tests. |
| examples/localcowork/_models/config.yaml | Adds documentation about HuggingFace cache defaults/env vars. |
| examples/localcowork/.gitignore | Ignores _models/.cache/ in the example project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.ts or src/types/ in this package, so the import will fail to resolve. Either add the missing types module or update the import path to the correct location (e.g., the existing types under examples/localcowork/src/types if that's intended).
| import type { | |
| McpServerStatus, | |
| ModelsOverview, | |
| PermissionGrant, | |
| SamplingConfig, | |
| } from "../types"; | |
| // Local type definitions for entities returned from the backend. | |
| // These are intentionally permissive to avoid coupling this UI layer | |
| // to a specific backend type shape while keeping types resolvable. | |
| interface McpServerStatus { | |
| [key: string]: unknown; | |
| } | |
| interface ModelsOverview { | |
| [key: string]: unknown; | |
| } | |
| interface PermissionGrant { | |
| [key: string]: unknown; | |
| } | |
| interface SamplingConfig { | |
| [key: string]: unknown; | |
| } |
| .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.
These rules reference CSS variables --color-text-primary / --color-text-secondary, but those variables are not defined in :root (only --color-text, --color-text-muted, etc.). This makes the color declarations invalid and can lead to unexpected inherited colors. Replace with existing variables or define the missing variables in :root.
| @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 file. The later definition will override the earlier one, so elements relying on the first pulse animation will silently change behavior. Rename one of the animations (e.g., pulseDots vs pulseOpacity) or consolidate into a single definition.
| let root = resolve_project_root(); | ||
| assert!(root.is_absolute()); | ||
| } | ||
|
|
There was a problem hiding this comment.
This test is missing the #[test] attribute, so it will never run and won't catch regressions. Add #[test] above the function.
| #[test] |
| fn test_resolve_db_path_returns_sqlite_path() { | ||
| let path = resolve_db_path(); | ||
| assert!(path.starts_with('/')); // Should be absolute path | ||
| assert!(path.ends_with(".db")); |
There was a problem hiding this comment.
This test asserts the resolved DB path starts with '/', which will fail on Windows. Use Path::new(&path).is_absolute() (or similar) instead of checking the leading character.
| let modified_secs = modified | ||
| .duration_since(SystemTime::UNIX_EPOCH) | ||
| .map_err(|e| format!("Time error: {}", e))? | ||
| .as_secs(); | ||
|
|
||
| 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.
check_config_reload will return true on the first call whenever the config file exists (since CONFIG_LAST_MODIFIED starts at 0). That will trigger an unnecessary reload/toast the first time the watcher runs. Consider treating the first observation as the baseline (store modified_secs and return false), or initialize CONFIG_LAST_MODIFIED during app startup.
| _models/*.gguf | ||
| _models/*.bin | ||
| _models/*.safetensors | ||
| _models/.cache/ |
There was a problem hiding this comment.
The PR description says this is only a .gitignore update and a comment about the HuggingFace cache, but this PR also includes substantial frontend and Rust/Tauri code changes (new UI, config watcher, inference changes, etc.). Please update the PR description/title to match the actual scope, or split these changes into separate PRs so review risk is manageable.
| if (interval) { | ||
| clearInterval(interval); |
There was a problem hiding this comment.
stopConfigWatch references interval, but only configWatchInterval is in scope. This will throw at runtime / fail type-checking and will prevent the polling interval from being cleared. Use the configWatchInterval value from get() (and clear that), then set it to null.
| if (interval) { | |
| clearInterval(interval); | |
| if (configWatchInterval) { | |
| clearInterval(configWatchInterval); |
refactor: document HuggingFace cache env var in environment configuration
Summary
Address Copilot AI review comments from PR #54:
_models/.cache/to.gitignoreto prevent accidental cache commitsChanges
Testing