Skip to content

Refresh (Cmd+R): fix self-cancelling reload, scope to focused window, coalesce rapid presses#1655

Merged
datlechin merged 5 commits into
mainfrom
fix-refresh-cmd-r
Jun 11, 2026
Merged

Refresh (Cmd+R): fix self-cancelling reload, scope to focused window, coalesce rapid presses#1655
datlechin merged 5 commits into
mainfrom
fix-refresh-cmd-r

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

Cmd+R refresh behaved incorrectly and laggily. Diagnosed with structured OSLog tracing, then refactored the refresh path onto native, key-window-scoped dispatch.

Two distinct bugs, both confirmed from logs:

  1. Refresh on a table failed silently. On an idle table tab, cancelCurrentQuery() always called driver.cancelQuery() even with no query in flight. That stray cancel (e.g. Postgres PQcancel) raced and aborted the reload query the same refresh had just issued, so the grid never updated.
  2. Holding Cmd+R piled up work. SwiftUI .commands auto-repeats the shortcut while held, and each fire spawned an un-coalesced full schema reload. A 2s hold queued ~16 refreshes; ~12 bulk schema reloads ran concurrently and kept draining for ~2s after release, saturating the main actor (a query whose data was ready took 1.5s just to post its result).

Changes

  • Cancel only when needed: cancelCurrentQuery() calls driver.cancelQuery() only when a query is actually in flight, so an idle refresh stops poisoning its own reload.
  • Native dispatch: user Cmd+R / toolbar refresh now calls MainContentCommandActions.refresh() on the key window via @FocusedValue / CommandActionsRegistry, like every other menu command. No more global broadcast or hand-rolled key-window filtering.
  • Targeted data-changed signal: AppCommands.refreshData is now PassthroughSubject<UUID, Never> (per-connection). Programmatic senders pass a connectionId; subscribers filter by it, so a refresh no longer reloads views or clears autocomplete caches for unrelated connections.
  • Dispatch by active view: structure mode reloads via structureActions.refresh(); table/data (and JSON) reload the grid; query tabs stay a no-op (Cmd+Enter runs them). ClickHouse Parts reloads through the same reloadAllTabs() path (it had no other refresh hook after the broadcast change).
  • Coalescing: rapid refreshes collapse to one leading + one trailing run; a held key never stacks work.
  • Ignore key auto-repeat: KeyRepeatFilter drops only isARepeat keydown events matching the Refresh shortcut, so holding Cmd+R fires once per physical press (matching Finder/Safari/Mail).

Tests

MainContentCoordinatorRefreshTests: idle refresh does not cancel the driver, in-flight refresh does, structure-mode refresh dispatches to the structure handler, plus the existing in-flight/idle/query-tab coverage. MockDatabaseDriver gained a cancelQuery counter.

Not in this PR

Loading a very large result set (e.g. 40k rows) is slow because the on-screen load uses the bulk PQexec path while the driver's streaming path (streamRows, single-row mode) is only used by exports. That is a separate data-load effort, tracked independently.

Verification

swiftlint lint --strict clean. Verified via log stream (category Refresh, since removed): a held Cmd+R now fires one refresh per press with nothing trailing after release, and table refresh reloads rows instead of erroring.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 855be15415

ℹ️ 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".

Comment on lines +898 to +901
coordinator.requestRefresh(
hasPendingTableOps: hasPendingTableOps,
onDiscard: { [weak self] in self?.clearPendingTableOps() }
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Invalidate autocomplete cache on manual refresh

When Cmd+R or the toolbar calls this new direct refresh path, it no longer publishes AppCommands.refreshData; the only place that clears autocomplete column metadata is SchemaProviderRegistry.subscribeToRefreshSignal() via that publisher. In the scenario where a table's columns changed outside this window and the user presses Refresh to pick up current schema, the table/sidebar reload runs but SQL autocomplete keeps serving the stale cached columns for this connection, whereas the previous refreshData.send(nil) path cleared the cache.

Useful? React with 👍 / 👎.

@datlechin datlechin merged commit d490404 into main Jun 11, 2026
2 of 3 checks passed
@datlechin datlechin deleted the fix-refresh-cmd-r branch June 11, 2026 15:36
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