fix(tui): defer startup reconnect and add manager actions#38
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves tsql TUI startup resilience by deferring session auto-reconnect credential resolution until after the first frame, adds a startup safe mode to skip auto-connect/update checks, and expands connection-manager actions for copying safe connection strings and testing saved connections.
Changes:
- Defer session reconnect/password resolution to post-first-draw and add
--safe-mode/--no-auto-connectto skip startup side effects. - Prevent stale connect task completions from overwriting newer connection state via a connection generation id.
- Add connection-manager actions (yank sanitized URL, yank CLI command, test connection) and update keybindings/help.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tsql/tests/startup_nonblocking.rs | Adds regression tests to ensure startup doesn’t do blocking password resolution in main.rs and that reconnect is deferred to App. |
| crates/tsql/src/ui/help_popup.rs | Adds a new help section documenting connection manager keybindings. |
| crates/tsql/src/ui/connection_manager.rs | Adds new manager actions (YankUrl/YankCli/TestConnection), updates keybindings (D for duplicate), and adds tests. |
| crates/tsql/src/main.rs | Introduces safe-mode flags, defers session reconnect by queuing it in App, and adjusts startup warning handling. |
| crates/tsql/src/config/connections.rs | Adds URL sanitization and CLI-command helpers on ConnectionEntry plus tests. |
| crates/tsql/src/app/app.rs | Adds deferred reconnect dispatch after first draw, background password resolution events, connect-generation gating, and background connection testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match rx.recv_timeout(Duration::from_millis(750)) { | ||
| Ok(true) => None, | ||
| Ok(false) => Some( | ||
| "1Password support is enabled, but `op` was not found on PATH. Install/sign in via \ | ||
| 1Password CLI or disable `connection.enable_onepassword`." | ||
| .to_string(), | ||
| ); | ||
| ), | ||
| Err(_) => Some( | ||
| "1Password CLI probe timed out; use `tsql --safe-mode` to bypass startup side effects." | ||
| .to_string(), | ||
| ), | ||
| } |
There was a problem hiding this comment.
onepassword_startup_warning_nonblocking still blocks the main thread (and therefore first draw) for up to 750ms via recv_timeout. That undermines the goal of avoiding startup black-screen delays. Consider deferring this probe until after the first frame (similar to the pending reconnect), or make it fully async/non-blocking by sending the result to the UI thread without waiting during startup.
| let Ok(rt) = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| else { | ||
| let _ = tx.send(DbEvent::TestConnectionResult { | ||
| success: false, | ||
| message: format!( | ||
| "Connection failed: could not start test runtime for {}", | ||
| entry.name | ||
| ), | ||
| }); | ||
| return; | ||
| }; | ||
|
|
||
| rt.block_on(async move { | ||
| let result = probe_connection(&url, entry.kind).await; | ||
| let event = match result { | ||
| Ok(()) => DbEvent::TestConnectionResult { |
There was a problem hiding this comment.
test_entry_in_background creates a brand-new Tokio runtime inside a spawn_blocking task for every test action. This is relatively expensive and can cause extra thread/IO driver setup per invocation. Consider resolving the password in spawn_blocking, then using the existing app runtime (self.rt.spawn(async { ... })) to run probe_connection, or reusing the existing PasswordResolved/event mechanism for this flow to avoid nested runtimes.
|
Attribution note: this PR is a scoped rebase/refactor of the first mergeable pieces from @rekurt's original #37. The original PR introduced the startup nonblocking direction and the connection-manager action ideas. This branch narrows that work to the pieces we can safely merge now, resolves the conflicts against current Thanks to @rekurt for the original work and direction. |
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>
fcoury
force-pushed
the
fix/pr37-stage1-startup-manager-actions
branch
from
775029a to
3b0e0de
Compare
Track saved-connection password lookups with a generation token so late credential results cannot override a newer connect or disconnect. Copy `tsql <connection-name>` commands from the connection manager so startup keeps using saved entry credential resolution.
Clear config `connection.default_url` before building the app in safe mode so startup cannot auto-connect from configuration. Emit and parse `--` for saved connection names that look like options so copied CLI commands still resolve the selected saved entry.
Skip `DATABASE_URL` and libpq environment fallbacks in safe mode so `--no-auto-connect` only connects when a positional target is provided. Track pending saved connection sources so safe mode suppresses automatic reconnects while still honoring explicit CLI saved names, and preserve clipboard failure statuses for connection manager copy actions.
Treat saved connection names passed on the CLI like user-picked connects when credential lookup cannot return a password. Automatic reconnects still use the startup fallback, but explicit saved names now open the password prompt instead of the connection picker.
Let saved-connection password resolves complete even if the old active connection reports `ConnectionLost` while credentials are still loading. The pending password generation remains the source of truth for whether a resolve result is current, and successful resolves restore the target connection name before starting the new connection.
Capture the saved connection name when a connect attempt starts and use that captured value when the matching async connect completes. This prevents an older connect from being recorded under a newer pending saved connection while credentials for the new target are still loading.
Summary
This is a scoped recovery of the mergeable pieces from #37.
--safe-mode/--no-auto-connectto skip startup reconnect and startup update checkstsqlcommands, and testing saved connectionsytoDsoycan yank the safe URLIntentionally left out the broader #37 changes around search/sort/import/export, metadata schema changes, and repository renames.
Verification
cargo fmt --allcargo test --lib --binscargo test -p tsql --test startup_nonblockingcargo clippy --all --all-targets -- -D warningscargo test -p tsql --test integration_tests