⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0.58140% with 171 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.10%. Comparing base (556c518) to head (2a3b733).

@@            Coverage Diff             @@
##             main     #701      +/-   ##
==========================================
- Coverage   69.43%   69.10%   -0.33%     
==========================================
  Files          46       47       +1     
  Lines       31838    31990     +152     
==========================================
+ Hits        22104    22105       +1     
- Misses       9734     9885     +151     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

REV Code Review — PR #701: feat/history-picker

• PR: feat(repl): interactive history picker for \s #701 — Interactive history picker for \s
• Branch: feat/history-picker → main
• Diff size: ~4280 lines (additions + deletions across 15 files)

Note: This diff is larger than just the history picker feature. The branch diverged from an older base and contains substantial unrelated rollbacks (connection handling, SSL, SIGWINCH, CI). Review covers the full diff as submitted.

BLOCKING ISSUES (3)

HIGH src/update.rs:146 — Wrong GitHub API URL (stale internal repo name)

const GITHUB_API_URL: &str = "https://api.github.com/repos/NikolayS/project-alpha/releases/latest";

The update checker now points to NikolayS/project-alpha (the old internal repo name) instead of NikolayS/rpg. Every user on this branch who runs the update check will either get a 404 or, worse, pick up releases from the wrong repository. This is a correctness regression introduced by this branch.

Fix: Restore to "https://api.github.com/repos/NikolayS/rpg/releases/latest".

HIGH src/repl/mod.rs — SIGWINCH handler removed, terminal resize no longer redraws status bar in real time

The branch removes the SIGWINCH tokio signal handler that called on_resize() + render() when the terminal was resized. It also drops the Arc<Mutex<StatusLine>> wrapper in favor of a bare StatusLine in ReplSettings. While this simplification is clean for single-threaded access, the consequence is that the status bar will only resize at the next REPL prompt iteration (poll-based via on_resize() call before each readline), not immediately during a long-running query. This is a UX regression from main.

If the simplification is intentional, the PR description and CHANGELOG must document it explicitly as a known limitation. As submitted, there is no mention of this trade-off.

Fix: Either reintroduce the SIGWINCH handler (adapted for the non-Arc ownership) or document the behavior change in the PR.

MEDIUM src/history_picker.rs:42-50 — Terminal cleanup in Drop writes to stdout then sends an escape to stderr; cursor restore may corrupt non-alternate-screen terminals on error

impl Drop for TerminalGuard {
    fn drop(&mut self) {
        let _ = terminal::disable_raw_mode();
        let _ = execute!(io::stdout(), LeaveAlternateScreen);
        let mut stdout = io::stdout();
        let _ = stdout.write_all(b"\x1b[H\x1b[2J\x1b[H");
        let _ = stdout.flush();
        let _ = io::stderr().write_all(b"\x1b[r");   // ← scroll region reset on stderr
        let _ = io::stderr().flush();
    }
}

Two problems:

1. After LeaveAlternateScreen, \x1b[H\x1b[2J\x1b[H (home + erase + home) is sent to the main screen buffer — this will blank the user's scrollback content that was visible before the picker opened. The alternate screen exit already restores the prior content; the extra clear is not only redundant but destructive.
2. \x1b[r (scroll region reset) is written to stderr instead of stdout. Mixing terminal control sequences between stderr and stdout is undefined behavior depending on whether they both point to the same TTY. In the statusline path, the caller re-renders the scroll region after the picker returns; sending a half-baked reset on stderr can interleave with that in unpredictable ways.

Fix: Remove the manual \x1b[H\x1b[2J\x1b[H write after LeaveAlternateScreen. Send \x1b[r to stdout, not stderr.

NON-BLOCKING (4)

MEDIUM src/repl/mod.rs:3671-3680 — Empty history not handled in the interactive picker path

None => {
    match crate::history_picker::run(entries) {
        Ok(Some(query)) => {
            settings.initial_input = Some(query);
        }
        Ok(None) => {} // user cancelled
        Err(e) => eprintln!("\\s: {e}"),
    }
}

entries may be empty (no history yet). history_picker::run is called unconditionally, then PickerState::new is given an empty vec. The UI will open showing zero entries with no explanation. The old \s (no-arg) path showed "\\s: history is empty" for the equivalent case. The interactive picker should show that same message (or refuse to open) when history is empty.

Fix: Add a guard before calling run:

if entries.is_empty() {
    eprintln!("\\s: history is empty");
    return;
}

MEDIUM src/history_picker.rs:195-228 — Scroll window calculation is off by one when selected == list_height - 1

let visible_start = if state.selected >= list_height {
    state.selected - list_height + 1
} else {
    0
};

When selected equals exactly list_height - 1 (last visible row), visible_start stays at 0 — correct. But when the user navigates to selected == list_height, visible_start jumps to 1, and the selected row is rendered at position list_height - 1 in the window. The selected row is always at the very bottom of the viewport. This means the user cannot see any context below the selected item (there is none) but also cannot see any context above (only one row). A conventional picker scrolls to keep the selected item centered when possible.

This is not a crash — it works — but it is surprising UX for a list taller than the viewport.

Suggestion: Consider a centered scroll: visible_start = selected.saturating_sub(list_height / 2).

LOW src/repl/mod.rs:4339-4347 — Env-var tests lose their serialization mutex without explanation

-    static ENV_MUTEX: std::sync::LazyLock<std::sync::Mutex<()>> =
-        std::sync::LazyLock::new(|| std::sync::Mutex::new(()));

Four tests that mutate std::env (startup_file_returns_psqlrc_env_when_set, resolve_api_key_*, etc.) had their ENV_MUTEX guard removed. std::env::set_var / remove_var are not thread-safe in Rust's test harness when tests run in parallel. The removal reintroduces a data race that the mutex was explicitly protecting against. This may be flaky in CI depending on scheduler timing.

Fix: Restore the mutex guards, or switch to std::env::set_var + #[serial_test::serial] if moving away from the manual lock is desired.

LOW tests/compat/test-connections.sh — test_wrong_password now tests against the trust-auth instance

# Before: required TEST_PG_SCRAM_PORT — skipped if not set (correct: trust auth ignores passwords)
# After: always runs against TEST_PGHOST/TEST_PGPORT

The original code had an explicit comment: trust-auth instances ignore the password and always return exit 0. The new code removes the SCRAM requirement and runs the wrong-password test against the same trust-auth postgres used by all other tests. This means the test will pass for the wrong reason (exit non-zero because the user TEST_PGUSER with PGPASSWORD=wrongpassword fails only if trust-auth is not configured, which it normally is). On a trust-auth server, PGPASSWORD=wrongpassword succeeds — the test will fail.

Fix: Keep the skip condition (TEST_PG_SCRAM_PORT not set → skip) or document that the CI postgres is configured with scram-sha-256.

POTENTIAL ISSUES (2)

MEDIUM src/connection.rs — parse_uri does not handle ?host= / ?port= query parameters (confidence: 6/10)

The new parse_uri implementation parses the authority section for host/port but does not handle host= or port= as query parameters. The original tokio-postgres delegation handled ?host=/tmp/pg&port=5437 correctly for Unix socket URIs. The new custom parser only reads sslmode, sslrootcert, sslcert, sslkey, application_name, connect_timeout, options, and target_session_attrs from the query string.

A URI like postgres:///dbname?host=/var/run/postgresql&port=5432&user=alice — which is the standard libpq URI format for Unix sockets — will be mishandled (empty authority → no host/port set). This affects tests C4 and C7/C8 from the deleted connection_paths.rs.

Confidence 6/10: the behavior may be intentional if the branch is rolling back this feature, but the deletion of those tests without explanation makes it ambiguous.

MEDIUM src/history_picker.rs:160-165 — Terminal::new receives a CrosstermBackend but there is no terminal.clear() before the first draw (confidence: 5/10)

let _guard = TerminalGuard::new()?;
let backend = CrosstermBackend::new(io::stdout());
let mut terminal = Terminal::new(backend)?;

ratatui 0.30 Terminal::new does not automatically clear the alternate screen. Without an explicit terminal.clear()? call, the first frame may render on top of whatever was in the alternate screen buffer from a previous use (e.g., if the picker is opened twice in the same session). This is unlikely to cause a crash but can produce visual artifacts on the first render.

Suggestion: Add terminal.clear()?; after Terminal::new(backend)?.

Summary

Notes:

• Findings: High-confidence issues (8-10/10) — blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) — review manually
• Filtered: Low-confidence issues (0-3/10) — excluded as likely false positives

Overall assessment

The history picker itself (src/history_picker.rs) is well-structured: RAII terminal guard, clean state machine, case-insensitive filter, Ctrl-P/N navigation — solid fundamentals. The initial_input → readline_with_initial handoff is an elegant way to return a selection to the REPL without a channel.

However, the PR includes a large unrelated rollback of connection handling, SSL, SIGWINCH, and CI that introduces several regressions. The src/update.rs API URL bug is a hard blocker. The SIGWINCH removal and the empty-history edge case need resolution before merge.

Verdict: CHANGES REQUESTED — 1 blocker (wrong update URL), 2 high/medium blocking items (SIGWINCH regression, Drop cleanup bug), plus the empty-history guard before the blocking items are addressed.

REV-assisted review (AI analysis by postgres-ai/rev)

REV Review — PR #701: feat(repl): interactive history picker for \s

Reviewed against current main (c1b21d2, v0.8.3). Previous automated review posted incorrect findings based on a stale base — disregard it.

PR merges cleanly into current main. 1702/1702 unit tests pass post-merge.

What this PR does

Adds src/history_picker.rs — a ratatui TUI picker that opens when \s is invoked with no arguments. User can type to filter, navigate with ↑↓ or Ctrl-P/N, confirm with Enter, cancel with Esc/Ctrl-C. Selected entry is pre-filled into the next readline prompt via initial_input: Option<String> on ReplSettings.

PRAISE — TerminalGuard RAII is correct

Raw mode + alternate screen cleanup happens unconditionally on drop, even through panics. The \x1b[H\x1b[2J\x1b[H hard reset and \x1b[r scroll region reset are belt-and-suspenders but appropriate for a TUI that takes over the full screen.

PRAISE — Scroll window implementation is solid

Manual visible-window calculation (visible_start = selected - list_height + 1) keeps the selected row in view without clamping issues. The saturating_sub(2) for the prefix accounts for ▶ width cleanly.

MINOR — is_terminal() guard missing before opening picker

run() in history_picker.rs does not check io::stdout().is_terminal() before entering raw mode. If rpg is run with stdout redirected (e.g., rpg ... | tee log), calling terminal::enable_raw_mode() will return an error, but the caller silently ignores Err(e) with eprintln!("\\s: {e}"). That's safe but could confuse users. Better to check upfront:

if !io::stdout().is_terminal() {
    eprintln!("\\s: interactive picker requires a terminal");
    return Ok(None);
}

MINOR — No unit tests for PickerState

PickerState::update_matches, move_up, move_down, selected_entry are pure logic with no I/O — they're straightforwardly testable. The PR adds no tests for the picker module. Not blocking for a new feature PR, but a follow-up issue would help.

NIT — event::poll timeout of 50ms

50ms polling means up to 50ms input latency between keystrokes and filter updates. 16ms (≈60fps) would be more responsive with no meaningful overhead.

NIT — truncate helper cuts on byte boundary

truncate slices at width bytes. Multi-byte UTF-8 history entries (e.g. queries with Japanese identifiers) will panic or produce invalid UTF-8 if a boundary falls mid-character. Use char_indices or unicode_truncate crate.

Testing evidence required before merge

Please post a screenshot or terminal recording showing:

1. \s opens the picker
2. Typing filters the list
3. Enter pre-fills the selected query into the prompt
4. Esc returns to prompt without pre-fill

CI is green (all 16 checks pass). Post-merge unit test run: 1702/1702 pass.

Overall verdict: REQUEST_CHANGES

Two items before merge:

1. Add is_terminal() guard (MINOR — prevents confusing error on non-TTY)
2. Fix truncate for multi-byte UTF-8 (NIT but can cause panic in production)

Everything else is polish — good implementation overall.

Follow-up review — commit 088d43c addresses both REQUEST_CHANGES items:

• is_terminal() guard added at top of run() — non-TTY stdout returns Ok(None) cleanly; non-TTY stdin returns an explicit error. Stricter than required, which is fine.
• truncate() is now char-boundary-safe: chars().take(max_chars).collect() — no more potential panic on multi-byte UTF-8.
• Bonus: pre-existing unused_self clippy issue in lua_commands.rs suppressed with #[allow].

CI green (all 16 checks pass). Verdict: APPROVE — ready to merge.