feat: add ~/.pgpass file support and pre-connect script#279
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ~/.pgpass support and a pre-connect shell-hook for database connections, including pgpass parsing, UI status, connection-model flags, driver password resolution, pre-connect execution with timeout, and documentation/localization updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as ConnectionFormView
participant Manager as DatabaseManager
participant Driver as DatabaseDriverFactory
participant Pgpass as PgpassReader
participant Hook as PreConnectHookRunner
participant Storage as ConnectionStorage
participant DB as Database
User->>Form: connect request
Form->>Manager: connectToSession(connection)
Manager->>Driver: createDriver(connection)
Driver->>Driver: resolvePassword(connection)
alt usePgpass & Postgres/Redshift
Driver->>Pgpass: resolve(host, port, db, user)
Pgpass-->>Driver: password or nil
else
Driver->>Storage: loadPassword(connection.id)
Storage-->>Driver: password
end
Driver-->>Manager: driver ready
opt preConnectScript present
Manager->>Hook: run(script, env)
alt hook fails (timeout/exit != 0)
Hook-->>Manager: HookError
Manager->>Manager: cleanup session & rethrow
else
Hook-->>Manager: success
end
end
Manager->>DB: establish connection
DB-->>Manager: connected
Manager-->>Form: connection ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
TablePro/Resources/Localizable.xcstrings (1)
12048-12068: Consolidate stale pre-connect error keys to a single placeholder format.Both
%dand stale%lldvariants now exist for the same error messages. Keep one canonical key format and remove stale duplicates once call sites are aligned, to avoid translator drift and key mismatch risk.Also applies to: 12072-12082
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Resources/Localizable.xcstrings` around lines 12048 - 12068, Consolidate the two localized keys for the pre-connect error into a single canonical placeholder format by keeping the "%d" variant ("Pre-connect script failed (exit %d): %@") and removing the stale "%lld" key ("Pre-connect script failed (exit %lld): %@") and its extractionState entry; update any call sites that still use the %lld placeholder to use the %d placeholder so they match the canonical key before deletion, and ensure the Vietnamese localization value is migrated to the retained key's localizations so translators are not lost.docs/databases/postgresql.mdx (3)
422-422: Use sentence case for heading.📝 Suggested fix
-## Pre-Connect Script +## Pre-connect script🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/databases/postgresql.mdx` at line 422, Heading "## Pre-Connect Script" uses title case; change it to sentence case ("## Pre-connect script") in the docs/databases/postgresql.mdx file by updating the heading text (the line starting with "## Pre-Connect Script") so only the first word is capitalized and subsequent words use lowercase.
390-392: Add language identifiers to code blocks.Per documentation guidelines, include language identifiers in fenced code blocks. These appear to be plain text configuration examples.
📝 Suggested fix
-``` +```text hostname:port:database:username:passwordAnd similarly for lines 396-402. </details> Also applies to: 396-402 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/databases/postgresql.mdxaround lines 390 - 392, The fenced code blocks
showing configuration examples (e.g., the block containing
"hostname:port:database:username:password" and the other blocks around lines
396-402) are missing language identifiers; update each fenced block to include
the "text" language tag (```text) so the examples are explicitly marked as plain
text per docs guidelines.</details> --- `375-375`: **Use sentence case for heading.** Per documentation guidelines, headings should use sentence case. "File" and "Support" should be lowercase. <details> <summary>📝 Suggested fix</summary> ```diff -## ~/.pgpass File Support +## ~/.pgpass file support ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/databases/postgresql.mdx` at line 375, Change the heading "## ~/.pgpass File Support" to sentence case by updating it to "## ~/.pgpass file support" in the docs where the heading string appears; ensure any table of contents or internal references use the same sentence-case heading text to keep links and anchors consistent. ``` </details> </blockquote></details> <details> <summary>TablePro/Core/Services/Infrastructure/PreConnectHookRunner.swift (2)</summary><blockquote> `52-63`: **Consider wrapping the blocking `waitUntilExit()` for better async behavior.** `process.waitUntilExit()` blocks the current thread. While this works within the 10-second timeout window, wrapping it in a detached task or using `terminationHandler` would be more idiomatic for async Swift. However, given the short timeout, this is acceptable. The current implementation is functional. If you want to improve it later: ```swift // Alternative using terminationHandler (optional future improvement) await withCheckedContinuation { continuation in process.terminationHandler = { _ in continuation.resume() } } ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Infrastructure/PreConnectHookRunner.swift` around lines 52 - 63, The code currently calls process.waitUntilExit(), which blocks the thread; replace this blocking call with an asynchronous wait by using a detached Task or process.terminationHandler combined with await withCheckedContinuation to resume when the process exits (refer to process.waitUntilExit(), timeoutTask, Task, and process.terminationHandler), and ensure timeoutTask still terminates the process after 10s and is cancelled once the continuation resumes to avoid leaks. ``` </details> --- `11-12`: **Add explicit access control.** Per coding guidelines, use explicit access control on the enum and its members. <details> <summary>♻️ Suggested fix</summary> ```diff -enum PreConnectHookRunner { +internal enum PreConnectHookRunner { private static let logger = Logger(subsystem: "com.TablePro", category: "PreConnectHookRunner") - enum HookError: LocalizedError { + internal enum HookError: LocalizedError { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Infrastructure/PreConnectHookRunner.swift` around lines 11 - 12, The enum PreConnectHookRunner currently lacks an explicit access modifier—add an explicit access level (e.g., internal or public per your module API) to the enum declaration and ensure all members have explicit modifiers as well (for example keep private static let logger as private, and mark any functions/props inside PreConnectHookRunner with internal/private/public as appropriate) so the enum and its members conform to the codebase access-control guideline. ``` </details> </blockquote></details> <details> <summary>TablePro/Core/Utilities/Connection/PgpassReader.swift (2)</summary><blockquote> `10-11`: **Add explicit access control to the enum and its members.** Per coding guidelines, always use explicit access control. The `enum` and its `static` methods should be marked with appropriate visibility. <details> <summary>♻️ Suggested fix</summary> ```diff -enum PgpassReader { - private static let logger = Logger(subsystem: "com.TablePro", category: "PgpassReader") +internal enum PgpassReader { + private static let logger = Logger(subsystem: "com.TablePro", category: "PgpassReader") ``` And for each public-facing static method (lines 14, 20, 34), add `internal` explicitly: ```diff - static func fileExists() -> Bool { + internal static func fileExists() -> Bool { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Utilities/Connection/PgpassReader.swift` around lines 10 - 11, The enum PgpassReader and its members lack explicit access control; mark the enum as internal and explicitly annotate its stored property logger and each static method (the three static functions referenced in the review) with internal visibility (e.g., change "enum PgpassReader" to "internal enum PgpassReader" and add "internal" before "private static let logger" where appropriate or make logger "internal" if used externally, and prefix each static function declaration with "internal") so all enum, property, and static method declarations have explicit access control. ``` </details> --- `35-39`: **Consider using URL-based file reading API.** `String(contentsOfFile:encoding:)` works but the URL-based variant is preferred in modern Swift. This is a minor refinement. <details> <summary>♻️ Suggested change</summary> ```diff - let path = NSHomeDirectory() + "/.pgpass" - guard let contents = try? String(contentsOfFile: path, encoding: .utf8) else { + let url = URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent(".pgpass") + guard let contents = try? String(contentsOf: url, encoding: .utf8) else { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Utilities/Connection/PgpassReader.swift` around lines 35 - 39, Replace the file-read call to use the URL-based API: build a URL for the home directory entry (e.g., using FileManager.homeDirectoryForCurrentUser.appendingPathComponent(".pgpass") or URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent(".pgpass")), then call String(contentsOf: url, encoding: .utf8) instead of String(contentsOfFile:encoding:); update the guard in the PgpassReader read logic (the variable currently named path/contents and the guard that logs "Could not read ~/.pgpass") to use the URL and preserve the existing error handling and logger.debug behavior. ``` </details> </blockquote></details> <details> <summary>docs/vi/databases/postgresql.mdx (1)</summary><blockquote> `390-392`: **Add language identifiers to code blocks.** Same as the English version - add language identifiers to the configuration examples for consistency. <details> <summary>📝 Suggested fix</summary> ```diff -``` +```text hostname:port:database:username:password ``` ``` </details> Also applies to: 396-402 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/vi/databases/postgresql.mdxaround lines 390 - 392, Update the fenced
code blocks that show the PostgreSQL connection format so they include a
language identifier; specifically change the triple-backtick blocks containing
the text "hostname:port:database:username:password" (and the other similar
example further down) to use "text" instead of "" so the examples match
the English version and render consistently.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@TablePro/Resources/Localizable.xcstrings:
- Around line 9386-9388: The new localization entries (for example the
"Loading..." key in Localizable.xcstrings and other newly added keys listed) are
missing zh-Hans translations causing fallback to source; update each of these
keys to include a zh-Hans translation entry alongside existing locales (at
minimum add a "zh-Hans" string for "Loading..." and for the other ranges
referenced) so every new key has a zh-Hans value; locate the keys by their exact
string identifiers (e.g., "Loading...") in Localizable.xcstrings and add the
corresponding zh-Hans translations in the same object structure as the vi
entries.In
@TablePro/Views/Connection/ConnectionFormView.swift:
- Around line 932-934: The code computes trimmedScript but stores the original
preConnectScript into finalAdditionalFields, so replace the stored value with
the trimmed string; set finalAdditionalFields["preConnectScript"] =
trimmedScript instead of preConnectScript, and apply the same change at the
other location where preConnectScript is stored later in this file (the second
assignment that mirrors this logic).- Around line 379-384: The pgpass lookup is using the wrong defaults: change the
username fallback from "root" to "postgres" and use the selected database type's
default port instead of always DatabaseType.postgresql.defaultPort; update the
call to PgpassReader.resolve (the block that builds host/port/database/username)
to compute portInt = Int(port) ?? currentDatabaseType.defaultPort (or
DatabaseType.redshift.defaultPort when the connection type is Redshift) and set
username to username.isEmpty ? "postgres" : username so Redshift connections use
port 5439 and PostgreSQL uses 5432.
Nitpick comments:
In@docs/databases/postgresql.mdx:
- Line 422: Heading "## Pre-Connect Script" uses title case; change it to
sentence case ("## Pre-connect script") in the docs/databases/postgresql.mdx
file by updating the heading text (the line starting with "## Pre-Connect
Script") so only the first word is capitalized and subsequent words use
lowercase.- Around line 390-392: The fenced code blocks showing configuration examples
(e.g., the block containing "hostname:port:database:username:password" and the
other blocks around lines 396-402) are missing language identifiers; update each
fenced block to include the "text" language tag (```text) so the examples are
explicitly marked as plain text per docs guidelines.- Line 375: Change the heading "## ~/.pgpass File Support" to sentence case by
updating it to "## ~/.pgpass file support" in the docs where the heading string
appears; ensure any table of contents or internal references use the same
sentence-case heading text to keep links and anchors consistent.In
@docs/vi/databases/postgresql.mdx:
- Around line 390-392: Update the fenced code blocks that show the PostgreSQL
connection format so they include a language identifier; specifically change the
triple-backtick blocks containing the text
"hostname:port:database:username:password" (and the other similar example
further down) to use "text" instead of "" so the examples match the
English version and render consistently.In
@TablePro/Core/Services/Infrastructure/PreConnectHookRunner.swift:
- Around line 52-63: The code currently calls process.waitUntilExit(), which
blocks the thread; replace this blocking call with an asynchronous wait by using
a detached Task or process.terminationHandler combined with await
withCheckedContinuation to resume when the process exits (refer to
process.waitUntilExit(), timeoutTask, Task, and process.terminationHandler), and
ensure timeoutTask still terminates the process after 10s and is cancelled once
the continuation resumes to avoid leaks.- Around line 11-12: The enum PreConnectHookRunner currently lacks an explicit
access modifier—add an explicit access level (e.g., internal or public per your
module API) to the enum declaration and ensure all members have explicit
modifiers as well (for example keep private static let logger as private, and
mark any functions/props inside PreConnectHookRunner with
internal/private/public as appropriate) so the enum and its members conform to
the codebase access-control guideline.In
@TablePro/Core/Utilities/Connection/PgpassReader.swift:
- Around line 10-11: The enum PgpassReader and its members lack explicit access
control; mark the enum as internal and explicitly annotate its stored property
logger and each static method (the three static functions referenced in the
review) with internal visibility (e.g., change "enum PgpassReader" to "internal
enum PgpassReader" and add "internal" before "private static let logger" where
appropriate or make logger "internal" if used externally, and prefix each static
function declaration with "internal") so all enum, property, and static method
declarations have explicit access control.- Around line 35-39: Replace the file-read call to use the URL-based API: build
a URL for the home directory entry (e.g., using
FileManager.homeDirectoryForCurrentUser.appendingPathComponent(".pgpass") or
URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent(".pgpass")), then
call String(contentsOf: url, encoding: .utf8) instead of
String(contentsOfFile:encoding:); update the guard in the PgpassReader read
logic (the variable currently named path/contents and the guard that logs "Could
not read ~/.pgpass") to use the URL and preserve the existing error handling and
logger.debug behavior.In
@TablePro/Resources/Localizable.xcstrings:
- Around line 12048-12068: Consolidate the two localized keys for the
pre-connect error into a single canonical placeholder format by keeping the "%d"
variant ("Pre-connect script failed (exit %d): %@") and removing the stale
"%lld" key ("Pre-connect script failed (exit %lld): %@") and its extractionState
entry; update any call sites that still use the %lld placeholder to use the %d
placeholder so they match the canonical key before deletion, and ensure the
Vietnamese localization value is migrated to the retained key's localizations so
translators are not lost.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `44f925c5-809f-4f1f-bab5-34159c2d936f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f24289fec760161d981670340c1fead266122677 and 3b7e2935d44779592bcf7a6c924a7f4391a44114. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `CHANGELOG.md` * `TablePro/Core/Database/DatabaseDriver.swift` * `TablePro/Core/Database/DatabaseManager.swift` * `TablePro/Core/Services/Infrastructure/PreConnectHookRunner.swift` * `TablePro/Core/Utilities/Connection/PgpassReader.swift` * `TablePro/Models/Connection/DatabaseConnection.swift` * `TablePro/Resources/Localizable.xcstrings` * `TablePro/Views/Connection/ConnectionFormView.swift` * `docs/databases/postgresql.mdx` * `docs/vi/databases/postgresql.mdx` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- PreConnectHookRunner: wrap Process execution in Task.detached to avoid blocking MainActor; use StderrCollector (Sendable, NSLock) to drain stderr concurrently and prevent pipe deadlock - DatabaseManager: forward additionalFields in buildEffectiveConnection so usePgpass is preserved through SSH tunnels - ConnectionFormView: cache pgpass status in @State via onChange handlers instead of doing file I/O in view body on every keystroke
This was referenced Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
~/.pgpassfile support for PostgreSQL/Redshift connections: toggle in the Authentication section disables the password field and lets libpq read credentials from~/.pgpass, with live validation (file exists, permissions are 0600, matching entry found)/bin/bash -cbefore each explicit connection attempt (10s timeout, non-zero exit aborts connection), useful for refreshing credentials or updating~/.pgpassfrom secrets managersadditionalFieldsstorage onDatabaseConnection, requiring no data migrationTest plan
~/.pgpasswithchmod 0600, verify toggle shows green "matching entry exists" status~/.pgpass, verify yellow "not found" warningecho testscript — verify it runs before connectexit 1— verify connection is aborted with error messagesleep 15— verify 10s timeout terminates itSummary by CodeRabbit
New Features
Documentation