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
Conversation
…result, and report runs in Output mode and under a query plan
…r record and the unit host off the recovery list
…r on the offscreen window instead of two UI cases that always skip
This branch has not been deployed
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.
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 showsExecutionWithoutReadoutarm of the bar's row. That flag wasshowsResultScope && !describesAResult && pagination.isLoading, andisLoadingis 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'srevealedAt) in its own@State, so removing the view dropped the state:Spacer, and the spinner vanished at once;ExecutionIndicatorViewinsidereadoutCluster, a different branch of the sameif, which starts withshowsExecution == falseand shows the duration straight away.LoadingRevealPolicyis 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 afterguard !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.
OffscreenConnectionWindowinjected the session before it installed its private toolbar.refreshFromActiveSessions()then reachedadoptSession, thenpointToolbar(at:)with no owner, which builtMainWindowToolbar()with the production identifier and autosave on and attached it, and AppKit writesNSToolbar Configuration com.TablePro.main.toolbar.v4on attach.The recovery file was wider than the fixture. Measured in the unit host, the fixture's coordinator is activated offscreen (
MainContentView.onAppearcallsmarkActivated(), which callsSessionRecoveryTracker.sync()), and itsteardown()callssync()again. The same holds for every coordinator a unit test builds: 60 test files construct one and about 300 callteardown().AppStorageEnvironmentresolves to production storage in the unit host, so each of those rewrote the developer'sLastOpenConnections.json, or deleted it when the list came out empty. Reordering the fixture could not fix that, because its own teardown writes too.Change
ResultStatusBaralways 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, andhideAfterDwellruns.ResultStatusControls.showsExecutionreplacesshowsExecutionWithoutReadout. It isResultsViewMode.reportsExecution(every mode but Structure) and is set before the plan guard, so Output mode and a plan get the indicator.·before the indicator moved intoExecutionIndicatorView(leadsWithSeparator), so it is drawn only when the indicator has something to show. It used to stand alone through the grace.StatusBarSeparatoris the one spelling of it.SessionRecoveryTracker.sync()writes nothing when XCTest is loaded, the wayTableLoadTracerpicks a discarding sink.adoptSessionrepoints that toolbar at the live coordinator instead of building the app's. The measured toolbar now has a subject.ConnectionWindowChromeUITestsloses the two cases that skipped on all 58 and 54 CI runs (the runner is 1024x768), along withskipUnlessTheScreenFitsThePinnedWindowandshownToolbar.ConnectionWindowToolbarTestsasserts the same things on the real window and toolbar at 1512x861, off screen.InspectorToolbarPlacementTests.swift:55is wrapped to 120 columns.Tests, and the edit that turns each red
Each edit was applied to this branch, run, and reverted.
ResultStatusBarLayoutTests"The indicator a first run revealed is the one still on screen when the result lands"elsearm besidereadoutZone(readoutCluster)ResultStatusModelTests"Output mode reports the execution of a query that has no result yet"showsExecution = viewMode.showsResultScopeshowsExecution → falseResultStatusModelTests"A query plan on screen gives up the readout and still reports the execution"showsExecutionafter the plan guardshowsExecution → falseResultStatusModelTests"A query tab that never ran reads the fetch the registry reports as loading"pagination.isLoading = pagination.isLoading || isFetchingisLoading → false,readout → .noRowsOffscreenConnectionWindowTests"Only the fixture's own toolbar reaches the window..."com.TablePro.main.toolbar.v4and the fixture's ownOffscreenConnectionWindowTests"Building, driving and tearing down the fixture writes neither the app's toolbar nor its recovery list"SessionRecoveryTrackerLastOpenConnectionsStorage.shared.load()ConnectionWindowToolbarTests"The default toolbar draws its controls in the titlebar and carries no mode control"database.isHidden → false, hidden set[]ConnectionWindowToolbarTests"Refresh leaves the toolbar on a Create Table tab...".createTablehides nothingThe 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
NSProgressIndicatorinstance 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
willAddItemNotificationwhile the fixture is built, and on a clean fixture that is the fixture's own toolbar alone.Local, Xcode 27:
ResultStatusModelTests,ResultStatusBarLayoutTests,OffscreenConnectionWindowTests,ConnectionWindowToolbarTests,InspectorToolbarPlacementTestsandEditorTabStripWindowDragTests, plusRecoveryConnectionListTests,WindowBusyStateGuardTestsandTabExecutionSettleGuardTests: 122 executed, 122 passed.swiftlint lint --stricton every changed file: 0 violations. No UI tests were run locally. CI is the UI proof.Not changed
ConnectionWindowChromeTests' harness, for one), which writes the same toolbar record.MainWindowToolbarNativeContractTestspins 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.ViewThatFitstier 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.