Skip to content

fix(datagrid): keep the execution indicator through a run's result, report runs in Output mode and under a plan, and keep the offscreen fixture off the app's own state - #3138

Draft
datlechin wants to merge 4 commits into
mainfrom
fix/status-bar-indicator-followups
Draft

datlechin wants to merge 4 commits into
mainfrom
fix/status-bar-indicator-followups

Conversation

@datlechin

Copy link
Copy Markdown
Member

Follow-ups to #3130 from its review. Every finding was checked against the code first, and all six held.

Cause

The standalone indicator flashed. #3130 drew the execution indicator on its own when a query tab had no result grid, in the else if showsExecutionWithoutReadout arm of the bar's row. That flag was showsResultScope && !describesAResult && pagination.isLoading, and isLoading is the tab's busy bit, so the arm went false in the same render the run ended. The indicator keeps its reveal state (showsExecution, and the gate's revealedAt) in its own @State, so removing the view dropped the state:

  • a run that ended with no columns left a Spacer, and the spinner vanished at once;
  • a run that landed columns mounted a second ExecutionIndicatorView inside readoutCluster, a different branch of the same if, which starts with showsExecution == false and shows the duration straight away.

LoadingRevealPolicy is a 500 ms grace plus a 500 ms dwell, so a first run of 600 ms showed Executing and Stop from 500 ms to 600 ms.

Output mode and a plan got nothing. The flag also required viewMode.showsResultScope, which is false for .output, and it was set after guard !snapshot.isQueryPlan else { return controls }. A PL/SQL block re-run in Output mode, or any re-run with an EXPLAIN plan on screen, had no spinner and no Stop for the whole run.

The fixture wrote the developer's own state. OffscreenConnectionWindow injected the session before it installed its private toolbar. refreshFromActiveSessions() then reached adoptSession, then pointToolbar(at:) with no owner, which built MainWindowToolbar() with the production identifier and autosave on and attached it, and AppKit writes NSToolbar Configuration com.TablePro.main.toolbar.v4 on attach.

The recovery file was wider than the fixture. Measured in the unit host, the fixture's coordinator is activated offscreen (MainContentView.onAppear calls markActivated(), which calls SessionRecoveryTracker.sync()), and its teardown() calls sync() again. The same holds for every coordinator a unit test builds: 60 test files construct one and about 300 call teardown(). AppStorageEnvironment resolves to production storage in the unit host, so each of those rewrote the developer's LastOpenConnections.json, or deleted it when the list came out empty. Reordering the fixture could not fix that, because its own teardown writes too.

Change

  • ResultStatusBar always draws the readout zone. The execution indicator has one slot in it, mounted whenever the mode reports execution, whether or not the result readout is showing. The same view therefore lives from the start of a run until its dwell has passed, across the result landing, and hideAfterDwell runs.
  • ResultStatusControls.showsExecution replaces showsExecutionWithoutReadout. It is ResultsViewMode.reportsExecution (every mode but Structure) and is set before the plan guard, so Output mode and a plan get the indicator.
  • The · before the indicator moved into ExecutionIndicatorView (leadsWithSeparator), so it is drawn only when the indicator has something to show. It used to stand alone through the grace. StatusBarSeparator is the one spelling of it.
  • SessionRecoveryTracker.sync() writes nothing when XCTest is loaded, the way TableLoadTracer picks a discarding sink.
  • The fixture installs its own toolbar owner before it injects the session, so adoptSession repoints that toolbar at the live coordinator instead of building the app's. The measured toolbar now has a subject.
  • ConnectionWindowChromeUITests loses the two cases that skipped on all 58 and 54 CI runs (the runner is 1024x768), along with skipUnlessTheScreenFitsThePinnedWindow and shownToolbar. ConnectionWindowToolbarTests asserts the same things on the real window and toolbar at 1512x861, off screen.
  • CHANGELOG: the fix(datagrid): report a query tab's first run in the status bar, and put the six quarantined UI cases back on the gate #3130 entry is reworded in place to name the bug.
  • InspectorToolbarPlacementTests.swift:55 is wrapped to 120 columns.

Tests, and the edit that turns each red

Each edit was applied to this branch, run, and reverted.

Test Edit Result
ResultStatusBarLayoutTests "The indicator a first run revealed is the one still on screen when the result lands" put the indicator back in its own else arm beside readoutZone(readoutCluster) red: "The result landing replaced the revealed indicator instead of letting it serve its dwell"
ResultStatusModelTests "Output mode reports the execution of a query that has no result yet" showsExecution = viewMode.showsResultScope red: showsExecution → false
ResultStatusModelTests "A query plan on screen gives up the readout and still reports the execution" set showsExecution after the plan guard red: showsExecution → false
ResultStatusModelTests "A query tab that never ran reads the fetch the registry reports as loading" delete pagination.isLoading = pagination.isLoading || isFetching red: isLoading → false, readout → .noRows
OffscreenConnectionWindowTests "Only the fixture's own toolbar reaches the window..." inject the session before installing the toolbar, as #3130 did red: the toolbars that reached the window are com.TablePro.main.toolbar.v4 and the fixture's own
OffscreenConnectionWindowTests "Building, driving and tearing down the fixture writes neither the app's toolbar nor its recovery list" drop the XCTest check from SessionRecoveryTracker red: the fixture's connection is in LastOpenConnectionsStorage.shared.load()
ConnectionWindowToolbarTests "The default toolbar draws its controls in the titlebar and carries no mode control" stop hiding the container capsule for a file-based engine red: database.isHidden → false, hidden set []
ConnectionWindowToolbarTests "Refresh leaves the toolbar on a Create Table tab..." .createTable hides nothing red: Refresh never left the titlebar

The first test in that table is a hosting test. It reveals the spinner past the real 500 ms grace, lands the result, and checks for the same NSProgressIndicator instance before yielding the main actor. The dwell task cannot run in between, so the check does not race the clock. It then waits for the spinner to go.

The toolbar-record half of the isolation test only catches a write that changes the record. The production toolbar rewrites the same configuration it loaded, and other suites in this host attach it too. So the ordering is guarded by the arrival probe in the first fixture test. It records every toolbar that posts willAddItemNotification while the fixture is built, and on a clean fixture that is the fixture's own toolbar alone.

Local, Xcode 27: ResultStatusModelTests, ResultStatusBarLayoutTests, OffscreenConnectionWindowTests, ConnectionWindowToolbarTests, InspectorToolbarPlacementTests and EditorTabStripWindowDragTests, plus RecoveryConnectionListTests, WindowBusyStateGuardTests and TabExecutionSettleGuardTests: 122 executed, 122 passed. swiftlint lint --strict on every changed file: 0 violations. No UI tests were run locally. CI is the UI proof.

Not changed

  • Other suites still attach the app's toolbar to windows they order front (ConnectionWindowChromeTests' harness, for one), which writes the same toolbar record. MainWindowToolbarNativeContractTests pins autosave on for the production toolbar, so turning it off in the test host would break that contract test. Moving those harnesses onto the fixture is separate work.
  • A ViewThatFits tier change while the result lands can still rebuild the row. That happens only in a pane narrow enough that the new controls push the bar down a tier.

CI

macOS Tests is running on this branch. This section is updated when it finishes.

This branch has not been deployed

No deployments
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