fix(datagrid): stop a header click made during a run from replacing the next Run's statement - #3118
Merged
Merged
Conversation
…he next Run's statement
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.
What was wrong
A header click on a query result handed its re-run to
runQuerythrough a field on the tab,pagination.sortExecutionOverride, andrunQueryread that field before it looked at the caret. The field was only cleared whenrunQueryconsumed it, or byresetLoadMore().While a run is in flight, the grid keeps showing the previous result, and its headers stay clickable. So a click during a run did this:
handleSortStateChangedwrote the re-run to the field and changed the tab's sort state.runQueryreturned at itsisExecutingguard and left the field set.The field is older than #3114. On main before #3114, that stale re-run was the old editor text plus
ORDER BY. #3114 made a parameterized re-run carry the values its result ran with and skip the parameter panel. So after #3114 the leftover also froze the old values: with:mset tobyein the panel, Run sent... WHERE msg = ? ORDER BY msg ASCbound tohello, with nothing on screen to say so. A reviewer found this on #3114 after it merged.What changed
sortExecutionOverrideis gone.handleSortStateChangedhands the re-run straight toexecuteResolvedSQL, which is the same path it reached throughrunQuerybefore: Safe Mode gate, missing-value check, driver placeholders, history.runQuerynow only runs the selection or the statement at the caret.The first test below fails on main without these changes.
Tests
MainContentCoordinatorSortTests.headerClickDuringRunLeavesNothingForTheNextRun, with two arguments: the in-flight run is stopped (stopExecution), or its claim settles without a result (how a failure or a truncated result ends). It uses the recording-driver harness from #3114:SELECT id, msg FROM audit WHERE msg = :mwithm = hello.beginTabExecution, then click themsgheader.bye, then press Run.ORDER BY, bound to["bye"].On main (ea0caa3), both cases fail. The sort state becomes
msg ASC, and Run sendsSELECT id, msg FROM audit WHERE msg = ? ORDER BY `msg` ASCbound to["hello"]. I got that by running the new test against main's source before changing it.PaginationStateTests: the test forresetLoadMore()clearing the override now covers the query and its values, since the override no longer exists. Three in-place sort tests lose theirsortExecutionOverride == nilline. Their rows-sorted-in-place assertions are what show that no re-run was sent.Verification
verify.sh test MainContentCoordinatorSortTests PaginationStateTests TabQueryIsolationTests FetchAllQueryTaskTests PaginationCoordinatorTests: PASS, 93/93 (before the rebase)verify.sh generatePASS,verify.sh buildPASS, and the same suites plusTabQueryContentEqualityTests: PASS, 100/100verify.sh linton the four changed Swift files: 0 violations. The step reports FAIL only for a stale path in.claude/skills/fix-issue/references/verification.md(/Applications/Xcode-beta.app), which is older than this change.Not covered
runQuerybails. The load in flight returns rows in the old order under the new chevron, until the next load runs the rebuilt query. Nothing stale is replayed there, because the rebuilt query matches the sort state on screen. It is older than fix(datagrid): sort a query result by re-running its own statement with the values it ran with #3114.Found while fixing #3078 (#3105). Follow-up to #3114.