refactor: Apple-pattern storage DI plus 3 product bug fixes#955
refactor: Apple-pattern storage DI plus 3 product bug fixes#955
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be69ff2b04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| MainContentView.lifecycleLogger.error( | ||
| "[open] buildBaseTableQuery failed for restored tab table=\(tableName, privacy: .public): \(error.localizedDescription, privacy: .public)" | ||
| ) |
There was a problem hiding this comment.
Reset table query after restore rebuild failure
When buildBaseTableQuery throws here (for example while plugin metadata/dialect is still unavailable), the catch block only logs and leaves the persisted content.query untouched. Later in handleRestoreOrDefault, table tabs are auto-executed whenever content.query is non-empty, so this path can still run stale persisted SQL instead of failing closed. That reintroduces malformed/outdated query execution during restore, which this change set is otherwise trying to prevent.
Useful? React with 👍 / 👎.
…torage default URL
…ge refactor entries
Summary
Three product bug fixes plus a storage singleton refactor to Apple's
URLSession.shared-style dependency injection pattern, enabling parallel test execution without races. Surfaced during a deep audit of pre-existing test failures and an end-to-end review of the External API code path.Product fixes
LibSSH2TunnelFactory.buildAuthenticatornow expands~inconfig.agentSocketPathandconfigEntry?.identityAgentbefore handing them toAgentAuthenticator. libssh2 does not expand~, so users on.sshAgentauth with a custom socket path (1Password and similar) silently failed to connect. HIGH severity.deleteGroupnow persists before firing the sync notification, mirroring the precedent fixed inConnectionStorage. Previously,markDeletedcould trigger an iCloud sync that read stale data and re-uploaded the deleted group. MEDIUM severity.SQLStatementGenerator,SQLRowToStatementConverter,QueryTabManager,QueryTab.buildBaseTableQuery, andDataChangeManagernow throwSQLDialectError.dialectUnavailablewhenPluginMetadataRegistry.snapshot(forTypeId:)returns nil. Previously fell back to identity quoting, silently emitting unquoted identifiers for tables/columns with reserved words. Throw is absorbed at UI boundaries viado/catchwithos.Logger.Singleton refactor (Apple pattern)
Nine classes refactored to Apple's documented pattern (WWDC 2018 #417, mirrors
URLSession.shared+URLSession(configuration:)):GroupStorage,AppSettingsStorage,ConnectionStorage,SyncMetadataStorage,QueryHistoryStorage,SQLFavoriteStorage,SQLFavoriteManagerDatabaseManager,PluginManagerSyncChangeTracker(now injectsSyncMetadataStorageso test-isolated chains stay isolated end-to-end)Each class keeps
static let sharedfor production callers (435+ references unchanged). Adds public init accepting dependencies via parameters with defaults. Tests construct isolated instances usingUserDefaults(suiteName: UUID().uuidString)or temp file URLs.Result: parallel test execution re-enabled (
parallelizable="YES"restored on the scheme). Inherently shared-state tests marked with@Suite(.serialized)per Apple's published guidance; tests previously serialized for race protection now run in parallel because their dependencies are isolated.Removed:
init(isolatedForTesting: Bool)andinit(isolatedStorage:)flavors onQueryHistoryStorage,SQLFavoriteStorage,SQLFavoriteManager. Replaced by Apple-pattern inits taking the actual dependency. No backward-compat shim.Known regression risk to verify:
SQLFavoriteStorage.initnow waits synchronously on database setup at first.sharedaccess. Prior init was async. Smoke-test app launch time before merging — if cold-launch slows visibly, defer database open via lazy property.Test cleanup
SQLStatementGenerator*Testsfiles (feature was removed from product code).cassandra-iconand safe-mode lock symbol tolock.open.fill(renamed in product).FakeMSSQLPluginstub to test target soTableQueryBuilderMSSQLTestscan resolve MSSQL dialect/driver.Test plan
~-prefixed socket path connects successfully (1Password).shared(default args preserve the singleton path)Related
SyncCoordinatorandAppSettingsManager(private inits, not constructed in tests, would need CloudKit-hook re-validation). Separate followup if needed.