-
-
Notifications
You must be signed in to change notification settings - Fork 298
fix(sidebar): prevent stuck spinner after table list loads #1460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a764ef6
bc530fd
97c7f11
a8382fb
354769a
8a85e25
7e337dc
11d6168
723f422
886a058
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,8 @@ final class SchemaService { | |
| let schema: String | ||
| } | ||
| @ObservationIgnored private var schemaChangeCancellable: AnyCancellable? | ||
| @ObservationIgnored private var loadGenerations: [UUID: Int] = [:] | ||
| @ObservationIgnored private var nextLoadGeneration = 0 | ||
| @ObservationIgnored private static let logger = Logger(subsystem: "com.TablePro", category: "SchemaService") | ||
|
|
||
| init() { | ||
|
|
@@ -181,6 +183,7 @@ final class SchemaService { | |
| await perSchemaDedup.cancel(key: SchemaKey(connectionId: connectionId, schema: schema)) | ||
| } | ||
| } | ||
| loadGenerations.removeValue(forKey: connectionId) | ||
| states.removeValue(forKey: connectionId) | ||
| procedures.removeValue(forKey: connectionId) | ||
| functions.removeValue(forKey: connectionId) | ||
|
|
@@ -201,6 +204,7 @@ final class SchemaService { | |
| driver: DatabaseDriver, | ||
| connection: DatabaseConnection | ||
| ) async { | ||
| let generation = beginLoadGeneration(for: connectionId) | ||
| states[connectionId] = .loading | ||
| bumpGeneration(connectionId) | ||
|
|
||
|
|
@@ -211,7 +215,7 @@ final class SchemaService { | |
|
|
||
| let grouping = PluginManager.shared.databaseGroupingStrategy(for: connection.type) | ||
| if grouping == .hierarchicalSchema { | ||
| await runHierarchicalLoad(connectionId: connectionId, driver: driver) | ||
| await runHierarchicalLoad(connectionId: connectionId, driver: driver, generation: generation) | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -230,22 +234,49 @@ final class SchemaService { | |
| dedup: functionDedup, | ||
| fetch: { try await driver.fetchFunctions(schema: nil) } | ||
| ) | ||
|
|
||
| let loadedProcedures = await proceduresTask | ||
| let loadedFunctions = await functionsTask | ||
| if supportsSchemas { | ||
| await loadSchemaList(connectionId: connectionId, driver: driver) | ||
| } | ||
| async let schemasTask: [String]? = supportsSchemas | ||
| ? Self.fetchSchemasSafely( | ||
| connectionId: connectionId, | ||
| dedup: schemasDedup, | ||
| fetch: { try await driver.fetchSchemas() } | ||
| ) | ||
| : nil | ||
|
|
||
| do { | ||
| let tables = try await tablesTask | ||
| guard isCurrentLoadGeneration(generation, for: connectionId, phase: "tables-loaded") else { | ||
| return | ||
| } | ||
| states[connectionId] = .loaded(tables) | ||
|
|
||
| let loadedProcedures = await proceduresTask | ||
| guard isCurrentLoadGeneration(generation, for: connectionId, phase: "procedures-loaded") else { | ||
| return | ||
| } | ||
| procedures[connectionId] = loadedProcedures | ||
|
|
||
| let loadedFunctions = await functionsTask | ||
| guard isCurrentLoadGeneration(generation, for: connectionId, phase: "functions-loaded") else { | ||
| return | ||
| } | ||
| functions[connectionId] = loadedFunctions | ||
|
|
||
| if let loadedSchemas = await schemasTask { | ||
| guard isCurrentLoadGeneration(generation, for: connectionId, phase: "schemas-loaded") else { | ||
| return | ||
| } | ||
| schemasInOrder[connectionId] = loadedSchemas | ||
| } | ||
| bumpGeneration(connectionId) | ||
| } catch is CancellationError { | ||
| return | ||
| } catch { | ||
| guard isCurrentLoadGeneration(generation, for: connectionId, phase: "tables-failed") else { | ||
| if loadGenerations[connectionId] == nil, case .loading = states[connectionId] { | ||
| states[connectionId] = .idle | ||
|
Comment on lines
+274
to
+276
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an older table fetch fails after a newer load has already started (for example the schema/database switch paths invalidate and then reload), this stale branch observes the newer load's Useful? React with 👍 / 👎. |
||
| } | ||
| return | ||
| } | ||
| Self.logger.warning( | ||
| "[schema] load failed connId=\(connectionId, privacy: .public) error=\(error.localizedDescription, privacy: .public)" | ||
| ) | ||
|
|
@@ -254,7 +285,7 @@ final class SchemaService { | |
| } | ||
| } | ||
|
|
||
| private func runHierarchicalLoad(connectionId: UUID, driver: DatabaseDriver) async { | ||
| private func runHierarchicalLoad(connectionId: UUID, driver: DatabaseDriver, generation: Int) async { | ||
| async let proceduresTask: [RoutineInfo] = Self.fetchRoutinesSafely( | ||
| connectionId: connectionId, | ||
| kind: .procedure, | ||
|
|
@@ -270,27 +301,66 @@ final class SchemaService { | |
|
|
||
| let loadedProcedures = await proceduresTask | ||
| let loadedFunctions = await functionsTask | ||
| await loadSchemaList(connectionId: connectionId, driver: driver) | ||
| let loadedSchemas = await Self.fetchSchemasSafely( | ||
| connectionId: connectionId, | ||
| dedup: schemasDedup, | ||
| fetch: { try await driver.fetchSchemas() } | ||
| ) | ||
|
|
||
| guard isCurrentLoadGeneration(generation, for: connectionId, phase: "hierarchical-loaded") else { | ||
| return | ||
| } | ||
| if let loadedSchemas { | ||
| schemasInOrder[connectionId] = loadedSchemas | ||
| } | ||
| procedures[connectionId] = loadedProcedures | ||
| functions[connectionId] = loadedFunctions | ||
| states[connectionId] = .loaded([]) | ||
| bumpGeneration(connectionId) | ||
| } | ||
|
|
||
| private func loadSchemaList(connectionId: UUID, driver: DatabaseDriver) async { | ||
| private func beginLoadGeneration(for connectionId: UUID) -> Int { | ||
| nextLoadGeneration += 1 | ||
| let generation = nextLoadGeneration | ||
| if case .loading? = states[connectionId] { | ||
| let previousGeneration = loadGenerations[connectionId] ?? 0 | ||
| Self.logger.debug( | ||
| "[schema] superseding in-flight load connId=\(connectionId, privacy: .public) previousGeneration=\(previousGeneration) newGeneration=\(generation)" | ||
| ) | ||
| } | ||
| loadGenerations[connectionId] = generation | ||
| return generation | ||
| } | ||
|
|
||
| private func isCurrentLoadGeneration( | ||
| _ generation: Int, | ||
| for connectionId: UUID, | ||
| phase: String | ||
| ) -> Bool { | ||
| guard loadGenerations[connectionId] == generation else { | ||
| let currentGeneration = loadGenerations[connectionId] ?? 0 | ||
| Self.logger.debug( | ||
| "[schema] stale load transition ignored connId=\(connectionId, privacy: .public) phase=\(phase, privacy: .public) generation=\(generation) currentGeneration=\(currentGeneration)" | ||
| ) | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| private static func fetchSchemasSafely( | ||
| connectionId: UUID, | ||
| dedup: OnceTask<UUID, [String]>, | ||
| fetch: @Sendable @escaping () async throws -> [String] | ||
| ) async -> [String]? { | ||
| do { | ||
| let allSchemas = try await schemasDedup.execute(key: connectionId) { | ||
| try await driver.fetchSchemas() | ||
| } | ||
| schemasInOrder[connectionId] = allSchemas | ||
| bumpGeneration(connectionId) | ||
| return try await dedup.execute(key: connectionId, work: fetch) | ||
| } catch is CancellationError { | ||
| return | ||
| return nil | ||
| } catch { | ||
| Self.logger.warning( | ||
| "[schema] fetchSchemas failed connId=\(connectionId, privacy: .public) error=\(error.localizedDescription, privacy: .public)" | ||
| ) | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a reload replaces the table list with a different list of the same count while routine/schema fetches are still pending, this assignment makes the new tables observable without changing
generationToken;SidebarViewModel.filteredTables(from:)and kind caches key only on count, generation, and query, so after the.loadingbump they can keep returning the old table rows until auxiliary metadata completes (or indefinitely if it hangs). Bump the generation at the point the loaded table state is published, before awaiting procedures/functions/schemas.Useful? React with 👍 / 👎.