fix(coordinator): stop an unknown row count blanking the estimate already on screen - #2072
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 the row count appearing under the grid and then vanishing about a second later. Regression from #2068, which revived a guard in
resolveRowCountthat had been dead since #2055, so this code ran for the first time and brought a latent bug with it.Cause
RowCountOutcome.appliedTotalcollapsed two different answers into one value:(nil, false)meant both "the count is genuinely gone" and "I could not work it out". The caller wrote it either way.Those are not the same thing, and the difference is visible. Phase 1 puts an estimate on screen synchronously from inline metadata. Phase 2's driver round trip lands a few hundred milliseconds later. When it comes back with an unusable estimate, which the doc comment on that very function already explains is normal (an un-analyzed table reports 0 or -1 while holding millions of rows), it blanked the number the user was already looking at.
The path needs no filters, which is worth stating because I twice told the user it did.
rowCountPlanreturns.approximatefor a non-SQL source with no filters, that callsfetchApproximateRowCount, and a driver returning 0 or less lands straight on the nil branch.Fix
appliedTotalbecomes optional.nilmeans "no answer", and the caller skips the write entirely, leaving whatever is on screen alone..clearstill returns(nil, false)and still wipes the count, because a filter change genuinely invalidates it.That is the whole behavioural change: an absent answer stops overwriting a present one.
Tests
The six
RowCountOutcomeTestscases are updated to the new semantics in the same commit, including the two that now assert nothing is applied rather than asserting nil is applied. 36 tests pass acrossRowCountOutcomeTests,RowCountPlanTests,Phase2RowCountGuardTests,RowCountTaskLifecycleTestsandMainContentCoordinatorRefreshTests.swiftlint lint --strictreports 0 violations in 1322 files.Still open, not in this PR
Phase 2 has no tracer coverage, which is why this needed a round trip through the user to diagnose. Adding stages is not as cheap as it looks: the trace finishes right after
mainRunLoopIdle, which is before the phase 2 row count lands, so a stage on a finished token is a no-op. Covering it properly means extending the trace lifetime to wait for phase 2, which risks leaving traces open, and is its own change.Correction to an earlier claim of mine: I said
StatusBarSnapshot+RowInfo.swift:23hides the total for a genuinely empty table. It does not. A table with zero rows falls through to"No rows", which is better than"0-0 of 0 rows". There is no bug there.