Skip to content

feat(coordinator): notify when long-running work finishes out of sight - #2272

Merged
datlechin merged 4 commits into
mainfrom
feat/long-running-work-notifications
Aug 20, 2026
Merged

feat(coordinator): notify when long-running work finishes out of sight#2272
datlechin merged 4 commits into
mainfrom
feat/long-running-work-notifications

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #2265.

Problem

Long-running work finishes silently. Switch to another app during a five minute query and the only way to learn the outcome is to switch back and look.

Root cause, and why this is a refactor

There was nowhere to ask "did something the user started just finish, and did they see it?" Around fifteen terminal functions across eight subsystems each wrote their own outcome state, and cancellation was expressed as an absence rather than an outcome: TabExecutionRegistry.invalidate erased the epoch, the cancelled task's completion then found settle false and returned without writing anything, and nothing anywhere learned the work had stopped.

The first draft of this change added a completion ledger plus a hand-written list of three cancel sites to hook. That was wrong twice over. There are four invalidation sites plus a .cancelled settle in the multi-statement path, so the list was already incomplete on the day it was written, and a parallel ledger leaks (a one-statement Execute All routes to executeQueryInternal and never reaches the batch's finish).

So the outcome is now part of the registry's own vocabulary instead:

  • invalidate(_:reason:) and invalidateAll(reason:) take a required ExecutionEndReason. Adding a new way to end an execution is now a compile error at the new call site rather than a silent gap. Updating the existing tests for it was not busywork, it was the enforcement working.
  • TabExecutionClaim carries a ContinuousClock start, because no terminal point can recover the elapsed time afterwards. The driver-reported time excludes queueing and tunnel setup, and the failure path records zero.
  • Work with no tab (export, import, backup, MCP) reports directly. Those already own clean three-way outcomes; PostgresDumpService.handleTermination was the model.

One pure OperationCompletionPolicy holds every rule, with no AppKit and no notification centre, so all of it is testable on CI.

What it does

Notifies when work ran longer than a threshold (default 20 seconds) and the result is not on screen: the app is not frontmost, or the window is occluded or minimized, or the owning tab is not selected. Success and failure both notify. Cancelled work never does, and neither does work the app started for itself.

Covered: queries and Explain, Execute All (once per batch, never per statement), row saves, structure changes, imports, exports, backups, fetch all rows, and MCP queries.

Clicking a notification selects the tab that ran the work. A retargeted or closed tab still reports its outcome but carries no click target rather than focusing whatever tab reused the id. Failures offer Copy Error, exports and backups offer Show in Finder. Notifications group by connection, and re-running a tab replaces its previous notification.

A tab whose work finished while you were elsewhere keeps a dot until you select it. That does not depend on notification permission, on the per-kind toggles, or on catching a banner.

Where this deviates from the issue text, and why

  1. Threshold 20s, not 10s. DataGrip is 20s and is the only database client shipping this enabled by default; DBeaver is 30s and off on macOS, Warp 30s, VS Code 60s. Ten seconds is the chattiest number in the corpus. The sample of database clients is small (n=2), so the honest argument is banner-spam avoidance, not the timeout interaction.
  2. No "play sound" toggle. Authorization options freeze at the first grant ("Subsequent authorization requests don't prompt the person"), and the plugin service already requested [.alert, .badge] with no sound, so the toggle would silently do nothing for every existing user. macOS owns per-app notification sound in System Settings. The request now includes .sound for new grants and drops .badge, which nothing ever set.
  3. No "only when in the background" toggle. Its off position produces behaviour the HIG rules out and that no client which got this right offers. The visibility gate is unconditional.
  4. Dock fallback keys on denied permission, not Focus. No API reports that Focus swallowed a notification.
  5. No "View Results" action. The HIG says to avoid an action that merely opens your app; the default click already does.
  6. No "timed out" label. No timeout classifier exists and the timeout is applied driver-side at connect, so the driver's own message is relayed.

Measured, not assumed

NSWindow.occlusionState on macOS 27, from a compiled probe:

onscreen  : raw=8194  == .visible=false  contains(.visible)=true   isMiniaturized=false
minimized : raw=8192  == .visible=false  contains(.visible)=false  isMiniaturized=true
app hidden: raw=8192  == .visible=false  contains(.visible)=false  appActive=false

.visible is 1 << 1, and the live value carries an undocumented high bit, so occlusionState == .visible is always false. A gate written that way notifies for everything, including the result you are looking at. Membership is the only correct test, and it already covers minimize and app-hide, so neither needs a separate check. A test asserts both the raw values and that the resolver never uses equality.

Two bugs fixed alongside

Both were found while investigating and both were verified before being touched.

  • Settings categories could be marked dirty but never sync. The category name was spelled out in four lists that nothing forced to agree. mcp and sync were marked dirty while appearing in none of the encode, decode or seed lists, so they were written to the dirty set on every change, never encoded, never cleared, and re-walked on every push forever. Measured on a real install: both sat in the dirty set with no corresponding record in the cache. Not syncing them is the documented contract, so the stray markDirty calls are gone, and every category now flows through one AppSettingsCategory list that the seed, encoder and decoder all read. A test fails if a category is marked dirty without being encodable. Without this, the new notification settings would have compiled, run, marked dirty and never reached a second Mac, with nothing reporting a problem.
  • A cancelled fetch-all stranded its tab. PaginationCoordinator bare-returned on the cancelled exit while every other exit cleared isLoadingMore, leaving a permanent "Loading…" with Fetch All hidden, healed only by re-running the query. Deterministic on any driver whose cancelQuery() is the PluginKit no-op default, because the fetch always runs to completion and returns straight into that guard.

Verification

  • Build: PASS.
  • Lint: 0 violations across all new code.
  • Tests: six new suites covering the threshold boundary, each visibility axis, cancellation, per-kind and master switches, identifier scoping by owner and kind, notification content, permission requested in context, category unioning, end reasons, and duration formatting. The notification centre is behind a protocol with a fake, because the real one has no authorization on CI and drops everything silently, which would make a test pass whether or not the code works.

No CloudKit schema work: the new settings category rides inside the existing settingsJson blob as a new value, and both SyncRecordType.settings and every AppSettingsSyncField are already verified in production.

Not covered by automation

No UI test for the tab dot. The flow needs a query slow enough to outlast a deliberate tab switch, which is not deterministic in the UI suite. The state that drives the dot and its accessibility value are unit tested instead, and the dot was checked by hand.

No screenshots. The settings page section this extends carries none, so the page stays consistent, and the capture script the contributor docs point at does not exist in this repo.

Privacy note for review

A failure notification includes the driver's error text, truncated to 120 characters. On a Mac showing notification previews on the lock screen, a constraint violation could put a column value there. Every comparable client does the same, and the alternative is a notification that does not say what went wrong, but it is worth a deliberate decision rather than an accident.

@mintlify

mintlify Bot commented Aug 20, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 20, 2026, 4:14 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

…k-notifications

# Conflicts:
#	TablePro/Core/Coordinators/PaginationCoordinator.swift
…k-notifications

# Conflicts:
#	TablePro/Views/Settings/SettingsView.swift
@datlechin
datlechin merged commit 5dfe5a9 into main Aug 20, 2026
7 of 8 checks passed
@datlechin
datlechin deleted the feat/long-running-work-notifications branch August 20, 2026 06:32
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.

Notifications for long-running work

1 participant