feat: added a new field in settings screen, regarding maxConnection c…#72
feat: added a new field in settings screen, regarding maxConnection c…#72ealvesss wants to merge 2 commits intodebba:mainfrom
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| keychain_utils::delete_ai_key(&provider) | ||
| } | ||
|
|
||
| pub const DEFAULT_MAX_CONNECTIONS: u32 = 1; |
There was a problem hiding this comment.
WARNING: Default pool size of 1 is too conservative and will likely cause performance degradation
The original code used max_connections(10) for MySQL/PostgreSQL and max_connections(5) for SQLite. A default of 1 means all queries will be serialized — only one query can execute at a time per connection pool. This will cause noticeable slowdowns for any workload with concurrent queries (e.g., opening multiple tabs, running queries while the sidebar loads schema info).
Consider a higher default such as 5 or 10 to match the previous behavior, while still allowing users to tune it down if needed.
| /// Get the configured maximum pool connections, or DEFAULT_MAX_CONNECTIONS if not set. | ||
| /// Does not require AppHandle — reads config.json directly via the paths module. | ||
| pub fn get_pool_max_connections() -> u32 { | ||
| let config_path = get_app_config_dir().join("config.json"); |
There was a problem hiding this comment.
WARNING: get_pool_max_connections() may resolve a different config path than the rest of the app
get_app_config_dir() uses directories::ProjectDirs::from("", "", "tabularis"), while get_config_dir() uses app.path().app_config_dir() (Tauri's path resolver). On macOS, Tauri uses the bundle identifier to construct the config path (e.g., ~/Library/Application Support/com.tabularis.app/), while the directories crate uses the app name directly (e.g., ~/Library/Application Support/tabularis/). These can diverge, meaning get_pool_max_connections() may silently read from a different (possibly non-existent) config file and always return DEFAULT_MAX_CONNECTIONS.
Consider passing AppHandle to this function (like get_max_blob_size does) to ensure consistent path resolution, or at minimum add a test/log to verify the paths match.
| let url = build_sqlite_url(params); | ||
| let pool = sqlx::sqlite::SqlitePoolOptions::new() | ||
| .max_connections(5) // SQLite has lower concurrency needs | ||
| .max_connections(crate::config::get_pool_max_connections()) |
There was a problem hiding this comment.
WARNING: SQLite now uses the same pool size as MySQL/PostgreSQL, which can cause database is locked errors
SQLite is a file-based database with limited write concurrency — only one writer can hold the lock at a time. The original code intentionally used a lower max_connections(5) with the comment // SQLite has lower concurrency needs. If a user sets maxConnections to a high value (e.g., 50 or 100), SQLite will create that many connections, and concurrent writes will result in SQLITE_BUSY / database is locked errors.
Consider capping SQLite pool size independently, e.g.:
let sqlite_max = crate::config::get_pool_max_connections().min(5);
.max_connections(sqlite_max)Or document clearly that high values are not recommended for SQLite connections.
| type="number" | ||
| value={settings.maxConnections ?? DEFAULT_SETTINGS.maxConnections} | ||
| onChange={(e) => | ||
| updateSetting( |
There was a problem hiding this comment.
SUGGESTION: parseInt fallback silently resets to default when the field is cleared
parseInt(e.target.value) || DEFAULT_SETTINGS.maxConnections! will fall back to 1 whenever the user clears the input field (empty string → NaN → falsy) or types 0. While min="1" prevents submitting 0 via the browser's native validation, the onChange handler fires on every keystroke, so clearing the field mid-edit will immediately reset the value to 1, making it impossible to type a number starting by clearing the field first.
Consider using Math.max(1, parseInt(e.target.value) || 1) or only updating the setting on onBlur to avoid this UX issue. The existing resultPageSize input has the same pattern, so this is consistent with the codebase — but worth noting.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
|
|
@ealvesss Thanks for the contribution! The feature is well-structured overall, but there are a few issues to address before merging. I have 2 points I would change:
Design suggestion Rather than a global setting in the Settings page, consider moving
This approach is more flexible, more explicit, and makes the global What do you think? |
2 participants
PR Description
feat: make connection pool size configurable via Settings
Problem
The maximum number of database connections per pool was hardcoded in pool_manager.rs (10 for MySQL/PostgreSQL, 5 for SQLite), with no way for users to adjust it.
Changes
Backend (src-tauri/src/)
Frontend (src/)
Behavior