perf(launch): hold a loading state back until the work behind it outlasts a grace period - #2606
Merged
Merged
Conversation
…asts a grace period Claude-Session: https://claude.ai/code/session_01J6xU4Zx4DRJ5JaxMP437uT
|
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.
Mitchell Hashimoto, watching TablePro open the SQLite sample: "Final frame didn't settle until the icon nearly hit the dock. A full bounce. I recommend getting rid of these three loading states unless the delay exceeds a certain amount. Frame reconciliation on view hierarchies is slow as hell and you do NOT want it in the startup path."
He is right, and instrumenting the launch found a fourth state he could not see.
What the window actually does today
refreshPanesand every phase transition, Debug build, opening the bundled Chinook sample from a restored session. Three runs, within 2ms of each other:Three pane hierarchies built in the first 103ms, for a connect that takes 39ms. Between builds 2 and 3,
applySidebarChromeModecollapses both split items, dropssplitView.autosaveName, callsrecalculateKeyViewLoop(), and then puts all of it back. That is AppKit layout, not only SwiftUI reconciliation.The fourth state is build 1: a "Not connected" pane, built 38ms before the connecting card replaces it. It resolves that way because
.idleanswers both for a window that finished dialling and for one that has not begun, and only the caller knows which.Root cause
The window's content is a pure function of
ConnectionWindowPhaseandSchemaState. Neither carries how long the state has lasted, so every state paints the instant it is entered however briefly it lives. One missing input, not five separate spinner bugs.What the HIG says
The change
LoadingRevealPolicyholds both halves of the rule: a 500ms grace before any progress UI appears, and a 500ms minimum dwell once it has. The dwell is the half that gets left out, andDelayedProgressIndicatorhad left it out: an indicator revealed at 500ms over work that ends at 510ms is a 10ms flash, worse than either state it sits between. The grace reuses the 500ms that component already used rather than inventing a second number.ConnectionWindowPaneResolvergains.preparing, the sub-grace pane. It draws nothing, and it is the one contentless pane thathidesChromereturns false for. The existing argument that "an object browser and an inspector with nothing to put in them are two empty columns that promise a session the window does not have yet" holds for a wait the user can see and not for one they cannot. On the happy path the window opens in its final shape and stays there..idlewith an auto-connect pending resolves to.preparingtoo, which is what removes build 1. The grace expiring is the exit from.preparingin both directions, and.idlereads it for that reason:startActivationConnectIfNeededreturns without dialling when the phase disallows it or the connection record has gone, and a window that never dials has to reach the not-connected pane rather than sit blank.The grace timer belongs to
ConnectionWorkspace, not to the window, for the same reasonattemptTokendoes: a window hosts several connections and each dials on its own clock, so a window-wide flag would let one connection's slow server put a progress screen over another's finished one. Its reveal repaints throughsyncPanes, so it costs nothing on the path where the connect landed first and the flag never flipped.SidebarObjectListPresentationgains.preparingon the same input. Past the grace nothing changes: the card, its Cancel button and the spinners are exactly what they are today, which is the case the connecting screen was designed for.The other two spinners from the screenshots go through the same policy at the view: the toolbar's "Executing…" cluster and the result readout. The toolbar gates the state rather than the content, so a query too fast to report leaves the previous duration standing instead of emptying the item and changing the row's width twice.
After
Same three runs, same build:
Two pane builds instead of three, and the first draws nothing. One chrome application instead of a collapse and a reveal. The connecting card is never built. The sidebar spinner never appears, because 194ms is inside the grace. Total time is unchanged, as expected: this does not make the connect faster, it stops the window rearranging itself around one.
Not done
installToolbarneeds a coordinator and the coordinator needs a session, and moving that is an ownership refactor the measured win does not need. Not collapsing the chrome is what mattered.Loading insights…,Loading users and roles…,Loading dashboard…,Loading schema…) and the sheet-based export, import and backup indicators are untouched. Those are user-initiated operations, not the startup path, and the HIG note about vague labels applies to them separately.Tests
48 cases across four suites, all passing. The two resolvers are pure and are tested in both directions, including that the grace never delays a settled outcome: a server that refuses in 20ms is an answer, not a wait, and a failed schema read has to keep its message and its Retry.
ConnectionWorkspaceGraceTestscovers the timer itself: that a connect landing first leaves no reveal behind for the next one, that re-arming does not restart a wait already running (or a redialling connection would never report anything), that teardown takes a pending reveal with it, and that a window which never dials falls back to the not-connected pane.No UI test. The behaviour this changes is entirely inside 500ms, and an XCUITest assertion that a card never appeared is a race rather than a check. The logic is in the two pure resolvers and the workspace timer, which is what the unit tests cover.
Also here
Two SwiftLint errors on
mainthat would have made this branch red: a missing thousands separator inSSHPublicKeyFilefrom #2604, and a blank line before a closing brace inCompareSyncProfileStoragefrom #2605. Neither file is otherwise touched by this branch.Verification
verify.sh build: PASSverify.sh testover ConnectionWorkspaceGraceTests, ConnectionWindowPaneResolverTests, SidebarObjectListPresentationTests, LoadingRevealPolicyTests: 48 executed, 48 passed, 0 failedverify.sh lintoverTableProandTableProTests: 0 violations. The reported doc-symbol miss (AXCell, CLAUDE.md:220) is on main and untouched here.TABLEPRO_UI_TEST_SANDBOXso it never read the connections on this machine.https://claude.ai/code/session_01J6xU4Zx4DRJ5JaxMP437uT