Skip to content

refactor(storage): hide QueryHistoryStorage and SQLFavoriteStorage behind their manager facades#1176

Merged
datlechin merged 1 commit intomainfrom
refactor/fold-storage-behind-manager-facades
May 10, 2026
Merged

refactor(storage): hide QueryHistoryStorage and SQLFavoriteStorage behind their manager facades#1176
datlechin merged 1 commit intomainfrom
refactor/fold-storage-behind-manager-facades

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Audit finding 3.3 — Manager-on-Storage facade duplication. Two singleton pairs (Manager + Storage) had a public Storage static let shared that callers could (and did) bypass the Manager with. After this PR, the public surface for each domain is just the Manager.

Changes

  • QueryHistoryStorage and SQLFavoriteStorage lose their static let shared. They are constructed by their owning Manager and not reachable directly from elsewhere in the codebase.
  • QueryHistoryManager.init(storage:) defaults to QueryHistoryStorage(). Same for SQLFavoriteManager.init(storage:) -> SQLFavoriteStorage().
  • QueryHistoryManager gains a small addHistory(_:) method that wraps storage.addHistory(entry) and posts queryHistoryDidUpdate(entry.connectionId). recordQuery(...) (which builds an entry from raw arguments) now delegates to addHistory(_:) so the event-posting logic lives in one place.
  • The 5 sites that were calling QueryHistoryStorage.shared.X directly are migrated to QueryHistoryManager.shared.X:
    • AppDelegate (background warm-up access)
    • MCPAuthPolicy.recordAuditQuery (addHistory)
    • MCPConnectionBridge.queryHistory (fetchHistory)
    • SearchQueryHistoryTool (fetchHistory)
    • HistoryDataProvider.deleteEntry and clearAll (deleteHistory, clearAllHistory)
  • AppServices.queryHistoryStorage field is removed; consumers thread services.queryHistoryManager instead. (No callers in main today read services.queryHistoryStorage; it was a vestigial leak from the early DI rollout.)

Side benefit (latent bug fix)

HistoryDataProvider.deleteEntry and clearAll were calling QueryHistoryStorage.shared directly, bypassing the Manager's queryHistoryDidUpdate event post. So deleting a history entry from the panel UI didn't refresh other windows' history panels. After this PR, all writes go through the Manager and post the event.

ValueDisplayFormatService (also flagged by 3.3) is intentionally not folded: it carries its own per-process state (autoDetectedFormats, overridesVersion) and isn't a thin wrapper. The audit's framing of it as a Manager/Storage duplication is loose; folding it would be a different refactor.

Test plan

  • Execute a query and verify it lands in the history panel (covers recordQuery -> addHistory).
  • Delete a history entry from the panel UI; confirm the panel refreshes in the same window AND in another window opened on the same connection (the latent bug fix).
  • Clear all history; confirm both windows refresh.
  • MCP search_query_history tool returns recent entries (covers fetchHistory migration).
  • Add / edit / delete SQL favorites; sidebar refreshes (covers SQLFavoriteStorage shared removal — the only construction path now is via SQLFavoriteManager).
  • swiftlint --strict clean.

@datlechin datlechin merged commit 3539d63 into main May 10, 2026
2 checks passed
@datlechin datlechin deleted the refactor/fold-storage-behind-manager-facades branch May 10, 2026 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant