feat(coordinator): notify when long-running work finishes out of sight - #2272
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
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
This was referenced Aug 20, 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
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.
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.invalidateerased the epoch, the cancelled task's completion then foundsettlefalse 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
.cancelledsettle 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 toexecuteQueryInternaland never reaches the batch's finish).So the outcome is now part of the registry's own vocabulary instead:
invalidate(_:reason:)andinvalidateAll(reason:)take a requiredExecutionEndReason. 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.TabExecutionClaimcarries aContinuousClockstart, 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.PostgresDumpService.handleTerminationwas the model.One pure
OperationCompletionPolicyholds 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
[.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.soundfor new grants and drops.badge, which nothing ever set.Measured, not assumed
NSWindow.occlusionStateon macOS 27, from a compiled probe:.visibleis1 << 1, and the live value carries an undocumented high bit, soocclusionState == .visibleis 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.
mcpandsyncwere 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 straymarkDirtycalls are gone, and every category now flows through oneAppSettingsCategorylist 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.PaginationCoordinatorbare-returned on the cancelled exit while every other exit clearedisLoadingMore, leaving a permanent "Loading…" with Fetch All hidden, healed only by re-running the query. Deterministic on any driver whosecancelQuery()is the PluginKit no-op default, because the fetch always runs to completion and returns straight into that guard.Verification
No CloudKit schema work: the new settings category rides inside the existing
settingsJsonblob as a new value, and bothSyncRecordType.settingsand everyAppSettingsSyncFieldare 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.