feat(tui): add remaining connection manager features#39
Conversation
Defer saved-connection password resolution until after the first TUI frame and add safe mode flags for bypassing startup reconnect and update-check side effects. Add connection manager actions for copying sanitized URLs, copying `tsql` commands, and testing saved connections while preserving the existing duplicate flow on a distinct key. Co-authored-by: nikita42 <13642481+rekurt@users.noreply.github.com>
Bring over the larger connection manager feature set from the original PR: metadata fields, search, persisted sort modes, manual reorder, usage stats, connection import/export, and richer form/detail UI. Preserve the current PR's startup fixes and AI-era changes while leaving repository rename churn out of this branch. Co-authored-by: nikita42 <13642481+rekurt@users.noreply.github.com>
fcoury
changed the base branch from
fix/pr37-stage1-startup-manager-actions
to
master
There was a problem hiding this comment.
Pull request overview
This PR completes the “connection manager” feature set for the TUI by extending the saved-connection schema (metadata + usage stats + persisted sort mode), enriching the manager UI (filter/sort/details/reorder), and adding import/export commands with conflict handling.
Changes:
- Expand
ConnectionEntry/ConnectionsFilewith metadata fields, usage stats (last_used_at,use_count), manual order, and persistedSortMode; add atomic writes plus import/export helpers. - Enhance connection manager and form UX: filtering (
/), sort cycling (s), detail pane, manual reorder, richer form fields + improved dirty checking, and inline “password persistence” guard. - Add
:export-connections/:import-connectionscommands, URL validation, and toast/status improvements.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tsql/src/util.rs | Adds URL sanitization helper and Postgres error hints; integrates hints into formatted PG errors. |
| crates/tsql/src/ui/sidebar.rs | Improves empty-state messaging and makes schema tree rendering resilient to tree build errors. |
| crates/tsql/src/ui/help_popup.rs | Updates help text for new manager features and new import/export commands. |
| crates/tsql/src/ui/connection_manager.rs | Adds filtering, sort cycling, detail pane, reorder actions, and toast footer behavior; adjusts key handling and layout. |
| crates/tsql/src/ui/connection_form.rs | Adds metadata fields (folder/tags/notes/app/timeout), Ctrl-W delete-word, improved dirty checking, and duplication flow support via mark_as_new(). |
| crates/tsql/src/ui/confirm_prompt.rs | Changes SwitchConnection context payload to Box<ConnectionEntry> to reduce cloning overhead. |
| crates/tsql/src/config/mod.rs | Re-exports new connection import/export, atomic write, conflict, and sort-mode APIs. |
| crates/tsql/src/config/connections.rs | Implements schema expansion, sort modes, filtering helpers, atomic writes, import/export with conflict handling, and credential-safe Mongo display/yank behavior. |
| crates/tsql/src/app/app.rs | Wires new manager actions (sort persistence, reorder), usage-stat recording + debounced saves, URL validation, and import/export command handlers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Strip the password component from a connection URL, preserving the | ||
| /// rest of the URL exactly. Used before displaying a URL to the user or | ||
| /// writing it to a log line. Leaves the input unchanged if it isn't a | ||
| /// parseable URL. | ||
| pub fn sanitize_url(url: &str) -> String { | ||
| if let Ok(mut parsed) = url::Url::parse(url) { | ||
| if parsed.password().is_some() { | ||
| let _ = parsed.set_password(None); | ||
| return parsed.to_string(); |
There was a problem hiding this comment.
sanitize_url’s docstring claims it preserves the rest of the URL “exactly”, but url::Url::parse + to_string() can normalize/percent-encode and otherwise change the textual representation. Either adjust the docstring to say it preserves the URL semantically (best-effort) or implement a string-based redaction that guarantees the original formatting is retained.
| pub fn mark_as_new(&mut self, title: impl Into<String>) { | ||
| self.original_name = None; | ||
| self.original_values = None; | ||
| self.title = title.into(); |
There was a problem hiding this comment.
mark_as_new() converts an edit-form into a new-entry form for duplication, but it doesn’t reset password_persist_acknowledged. Since edit-mode initializes this flag to true, duplicated entries won’t show the Issue #16 “password won’t be remembered” guard if the user types a password without selecting a persistence mechanism. Consider resetting password_persist_acknowledged to false (and/or re-deriving it from current persistence state) when switching into new-entry mode.
| self.title = title.into(); | |
| self.title = title.into(); | |
| self.password_persist_acknowledged = | |
| self.save_password || !self.op_ref.is_empty(); |
| if !s.contains("://") { | ||
| if s.contains('=') { | ||
| return Ok(()); | ||
| } | ||
| return Err(format!( | ||
| "Not a recognised connection URL. Expected postgres://user:pass@host:port/db, mongodb://..., or libpq `host=... user=...`. Got: {}", | ||
| truncate_for_error(s) | ||
| )); |
There was a problem hiding this comment.
validate_connection_url includes the user’s raw input in the “Not a recognised connection URL … Got: …” error. If a user pastes a credential-bearing string that doesn’t contain :// (or other malformed input), this can echo secrets back into the UI. Consider redacting likely password material (e.g. password=...) and/or using an existing URL-sanitizer before embedding user input in error messages.
| // Search / filter | ||
| (KeyCode::Char('/'), KeyModifiers::NONE) => { | ||
| self.search_active = true; | ||
| ConnectionManagerAction::Continue | ||
| } | ||
|
|
||
| // Cycle sort mode | ||
| (KeyCode::Char('s'), KeyModifiers::NONE) => { | ||
| self.sort_mode = self.sort_mode.next(); | ||
| self.resort_by_current_mode(); | ||
| self.toast = Some(format!("Sort: {}", self.sort_mode.label())); | ||
| ConnectionManagerAction::SortModeChanged { | ||
| mode: self.sort_mode, | ||
| } | ||
| } | ||
|
|
||
| // Duplicate selected (shift-d) | ||
| (KeyCode::Char('D'), _) => { | ||
| if let Some(entry) = self.selected_connection() { | ||
| ConnectionManagerAction::Duplicate { | ||
| entry: entry.clone(), | ||
| } | ||
| } else { | ||
| ConnectionManagerAction::Continue | ||
| } | ||
| } | ||
|
|
||
| // Yank URL (no password) | ||
| (KeyCode::Char('y'), KeyModifiers::NONE) => { | ||
| if let Some(entry) = self.selected_connection() { | ||
| ConnectionManagerAction::YankUrl { | ||
| url: entry.to_url(None), | ||
| } | ||
| } else { | ||
| ConnectionManagerAction::Continue | ||
| } | ||
| } | ||
|
|
||
| // Copy-as-CLI | ||
| (KeyCode::Char('c'), KeyModifiers::NONE) => { | ||
| if let Some(entry) = self.selected_connection() { | ||
| ConnectionManagerAction::YankCli { | ||
| command: entry.to_cli_command(), | ||
| } | ||
| } else { | ||
| ConnectionManagerAction::Continue | ||
| } | ||
| } | ||
|
|
||
| // Test connection | ||
| (KeyCode::Char('t'), KeyModifiers::NONE) => { | ||
| if let Some(entry) = self.selected_connection() { | ||
| ConnectionManagerAction::TestConnection { | ||
| entry: entry.clone(), | ||
| } | ||
| } else { | ||
| ConnectionManagerAction::Continue | ||
| } | ||
| } | ||
|
|
||
| // Reorder up / down (Ctrl-K / Ctrl-J) | ||
| (KeyCode::Char('K'), KeyModifiers::CONTROL) | ||
| | (KeyCode::Char('k'), KeyModifiers::CONTROL) => self.try_reorder(-1), | ||
| (KeyCode::Char('J'), KeyModifiers::CONTROL) | ||
| | (KeyCode::Char('j'), KeyModifiers::CONTROL) => self.try_reorder(1), |
There was a problem hiding this comment.
The connection manager key handling gained/changed several behaviors here (filter mode /, sort cycling s with SortModeChanged, yank URL/CLI, test, duplicate). The file already has unit tests but no longer asserts these key→action mappings (some prior tests were removed). Adding targeted tests for these new/updated bindings would help prevent regressions in input handling and toast behavior.
| let tokens: Vec<&str> = trimmed.split_whitespace().collect(); | ||
| let last = *tokens.last().unwrap_or(&""); | ||
| if last.starts_with("--") && tokens.len() >= 2 { | ||
| let strategy = parse_import_flag(last); | ||
| let path = tokens[..tokens.len() - 1].join(" "); | ||
| (Some(path), strategy) | ||
| } else { | ||
| ( | ||
| Some(trimmed.to_string()), | ||
| Ok(crate::config::ImportConflict::Rename), | ||
| ) | ||
| } |
There was a problem hiding this comment.
parse_import_args() treats a single token like --overwrite as a path (because it only interprets a trailing --flag when there are ≥2 tokens). That means :import-connections --overwrite yields a confusing “failed to read import file: --overwrite” instead of a usage error. Consider detecting the “flag-only” case (trimmed starts with -- and has no path) and returning None so the caller can show proper usage.
Stop applying the cursor-line underline style to the command prompt so entered commands render as plain text inside the prompt. Co-authored-by: nikita42 <13642481+rekurt@users.noreply.github.com>
Stop emitting unsupported `sslrootcert`, `sslcert`, and `sslkey` URL parameters until the rustls connector can consume those saved fields directly. Keep explicit saved-name connects working in `--safe-mode`, and make duplicated passworded connections prompt instead of connecting without credentials.
Reset duplicate forms so newly typed passwords still hit the persistence warning before save. Keep credential fields from the duplicate form when users add a password or 1Password reference, while preserving donor prompt behavior for passworded entries.
Suppress config-driven startup connects before app construction in safe mode, preserve supported URL query options on import, and block reorder actions while the manager list is filtered. These fixes keep startup side effects honest and prevent imported or reordered connection settings from being silently lost.
Resolve the manager feature branch against the merged startup manager changes while keeping async credential generation guards intact.
Summary
This PR brings over the scoped startup/action work from #38 plus the larger connection-manager pieces from the original #37 branch while leaving out the repository rename/doc churn.
:export-connectionsand:import-connectionswith conflict handlingAttribution: this ports the remaining connection-manager direction from @rekurt's original PR work. Both commits include a
Co-authored-by:trailer for @rekurt.Verification
cargo fmt --allcargo test --lib --binscargo clippy --all --all-targets -- -D warnings