Skip to content

fix: stop the schema refresh from blanking the sidebar and the structure tab counts (#1916)#1952

Merged
datlechin merged 2 commits into
mainfrom
fix/1916-schema-refresh-flicker
Jul 24, 2026
Merged

fix: stop the schema refresh from blanking the sidebar and the structure tab counts (#1916)#1952
datlechin merged 2 commits into
mainfrom
fix/1916-schema-refresh-flicker

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #1916.

Frame-by-frame on the reporter's recording, a structure save produces two separate flickers. Deleting an index at t=9.25s:

t Sidebar Structure picker
8.83s populated Columns (24) │ Indexes (4) │ Foreign Keys (3)
9.25s blank + spinner counts stripped, every segment resizes
9.75s repopulated Columns (24) │ Indexes │ Foreign Keys (3)
10.0s settled Indexes (3), third re-layout

Both saves in the video do it, and at 41.7s the sidebar is still blank after the index list has already refreshed.

Root cause

One modeling error in two places: state that conflates do we have data with is a fetch in flight, so refreshing destroys what it is refreshing.

Sidebar. SchemaState.loading carries no payload and tables(for:) returns [] for anything but .loaded, while runLoad wrote states[id] = .loading before the network call. So SidebarView.flatContent's case .loading where tables.isEmpty matched on every refresh and the case .loaded, .loading: tableList fallback below it was unreachable. The view could not tell a cold load from a warm refresh because the data was already gone.

Structure counts. loadedTabs.removeAll() cleared the loaded flag for every tab at once, but tabLabel(for:) gates each count on loadedTabs.contains(...). That flag was only being cleared so DDL and triggers would refetch later; the counts were collateral damage.

Safe Mode is not a second trigger. It is a pure gate with no reload path, so it only makes users watch the screen during its confirm prompt.

Fix

SchemaService now fetches first and commits over the old value:

  • .loading is entered only when there is no loaded content; an in-flight refresh is signalled separately through isRefreshing, guarded by load generation so a superseded load cannot clear the current one's flag.
  • Both failure paths route through markLoadFailed, which already had the "keep loaded data" guard, so a transient refresh error no longer swaps the sidebar for an error screen.
  • invalidate (real teardown: disconnect, database switch) is split from prepareForReload (cancel in-flight fetches, keep content). SchemaRefreshService was calling invalidate on every hierarchical refresh, which wiped the tree to .idle.
  • Per-schema state had the identical bug and shares the same shape now. refreshLoadedSchemaTables re-fetches expanded schemas in place, so keeping content does not turn into serving stale content.

StructureTabDataState splits "has data" (drives the count) from "needs refetch" (drives the reload), so marking everything stale never blanks a count. Both reload sites fetch columns, indexes and foreign keys in one withMetadataDriver block and commit all three in a single synchronous write: one render instead of three. .monospacedDigit() keeps a 43 change from reflowing segments too.

Removing the blank would have silently removed all feedback from a slow manual refresh, so DelayedProgressIndicator shows a small spinner in the sidebar footer only past ~500ms. It sits before the Spacer(), so nothing shifts when it appears.

Why this shape

  • HIG Loading: "Show something as soon as possible... consider showing placeholder text, graphics, or animations as content loads, replacing these elements as content becomes available."
  • HIG Progress indicators (macOS): a spinner is for "a background operation or when space is constrained... within a small area, such as within a text field or next to a specific control". A centered spinner replacing the whole source list is not that shape.
  • HIG Segmented controls: "In general, keep segment size consistent."
  • Apple documents no numeric spinner threshold and ships no delayed-progress mechanism in AppKit, SwiftUI or Foundation, so the 500ms delay is explicitly app-level.
  • DatabaseTreeMetadataService.reloadTablesInPlace already had the correct pattern and markLoadFailed already had the guard. SchemaService was the outlier.

DataGrip shipped a fix for this exact behaviour in 2024.3, describing the old one as "a full introspection of the entire schema... time-consuming and often disrupted the workflow". Postico fixed the tree-collapse half in v1.2.

Tests

14 new tests. The key one gates the driver's fetchTables on a continuation and asserts mid-flight that tables(for:) still returns the old list and the state is not .loading, the assertion whose absence let this ship. Also covers cold load still reporting .loading, a failed refresh keeping loaded tables, prepareForReload vs invalidate, and per-schema refresh.

42/42 pass, including every pre-existing suite in this area (SchemaServiceTests, SchemaServiceHierarchicalTests, SchemaServiceRoutinesTests, DatabaseTreeMetadataServiceTests). SwiftLint --strict clean.

Not verified: the visual result. That needs the app running against a real PostgreSQL connection, so it is worth reproducing the issue's steps before merge.

Follow-ups, deliberately not in this PR

  • Setting a new index to Unique silently does nothing on PostgreSQL and every other .truefalse database. Visible in the same recording (green optimistic row says YES at 41.3s, reloaded row says NO at 41.7s). StructureRowProvider.customDropdownOptions registers options for the Indexes Type column but not Unique, so the generic boolean fallback offers "true"/"false" while StructureEditingSupport.updateIndex only accepts "YES"/"1". Separate data-correctness bug, deserves its own fix and changelog line.
  • Scoped refresh. Dropping one index still reloads the whole connection, it is just invisible now. Narrowing that is a performance change with real stale-UI risk, better on its own.

https://claude.ai/code/session_01WKxR4tvL8bPU4NfsULyYNA

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 570e0495fe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +112 to +115
await schemaService.refreshLoadedSchemaTables(
connectionId: connectionId,
driver: driver
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Prune tables from schemas removed during refresh

When a hierarchical-schema refresh drops or renames a schema that had previously been expanded, this reloads every entry in perSchemaStates, including schemas absent from the newly fetched schema list. A missing-schema fetchTables normally fails, and runSchemaLoad deliberately preserves the prior loaded state on failure; allLoadedTables then continues to include those stale tables and feeds them to autocomplete. Filter/prune per-schema state against the refreshed schemasInOrder before retaining it (and only reload schemas that still exist).

Useful? React with 👍 / 👎.

@datlechin
datlechin merged commit bdd6124 into main Jul 24, 2026
2 checks passed
@datlechin
datlechin deleted the fix/1916-schema-refresh-flicker branch July 24, 2026 03:35
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.

Multiple UI Flicker

1 participant