fix: sidebar not refreshing after creating or dropping tables#354
Merged
fix: sidebar not refreshing after creating or dropping tables#354
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces a cache-invalidation mechanism for database tables and improves tab management. It adds methods to SQLSchemaProvider for managing cached tables, introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Coordinator as MainContentCoordinator
participant ViewModel as SidebarViewModel
participant Fetcher as LiveTableFetcher
participant Provider as SQLSchemaProvider
participant Driver as Database Driver
Coordinator->>Coordinator: reloadSidebar()
Coordinator->>Provider: invalidateTables()
Provider->>Provider: clear cached tables
Coordinator->>ViewModel: forceLoadTables()
ViewModel->>ViewModel: loadTables(force: true)
ViewModel->>Fetcher: fetchTables(force: true)
Fetcher->>Provider: fetchFreshTables()
Provider->>Driver: fetch fresh tables
Driver-->>Provider: return tables
Provider->>Provider: update cache
Provider-->>Fetcher: return tables
Fetcher->>Provider: updateTables(newTables)
Fetcher-->>ViewModel: return tables
ViewModel->>ViewModel: update sidebar state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
TablePro/Views/Main/MainContentCoordinator.swift (1)
315-319: Move this added coordinator behavior to a dedicated extension file.Please place
reloadSidebar()changes in something likeMainContentCoordinator+SidebarRefresh.swiftinstead of expanding the main coordinator file.Based on learnings: Applies to TablePro/Views/Main/**/*.swift : When adding coordinator functionality, add a new extension file (e.g., +Alerts, +Filtering, +Pagination) rather than growing the main MainContentCoordinator file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/MainContentCoordinator.swift` around lines 315 - 319, The reloadSidebar() method added to MainContentCoordinator should be moved into a dedicated extension file to avoid growing the main coordinator; create a new file named something like MainContentCoordinator+SidebarRefresh.swift, add an extension on MainContentCoordinator containing reloadSidebar(), and ensure it calls schemaProvider.invalidateTables() and sidebarViewModel?.forceLoadTables() with the same access level and any required imports so behavior and visibility remain unchanged.TableProTests/ViewModels/LiveTableFetcherTests.swift (1)
142-194: Add a force-refresh test for the empty-schema edge case.Current new tests cover non-empty fresh results only. Add a case where forced fetch returns
[]to lock down single-fetch behavior and prevent accidental duplicate DB calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/ViewModels/LiveTableFetcherTests.swift` around lines 142 - 194, Add a new test function (e.g., forceRefreshEmptySchemaEdgeCase) that mirrors the existing force tests but covers the empty-schema edge case: create a MockDatabaseDriver, pre-load SQLSchemaProvider with some initial table(s) via provider.loadSchema(using:), then call await provider.invalidateTables(), set mockDriver.tablesToReturn = [] (empty array), instantiate LiveTableFetcher(connectionId: UUID(), schemaProvider: provider) and call try await fetcher.fetchTables(force: true), assert the returned result is empty, assert mockDriver.fetchTablesCallCount incremented by 1, and finally assert await provider.getTables() returns an empty array; reference symbols: LiveTableFetcher.fetchTables(force:), SQLSchemaProvider.loadSchema(using:), SQLSchemaProvider.invalidateTables(), SQLSchemaProvider.getTables(), MockDatabaseDriver.tablesToReturn and MockDatabaseDriver.fetchTablesCallCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro.xcodeproj/project.pbxproj`:
- Line 1761: The pbxproj currently hardcodes DEVELOPMENT_TEAM = D7HJ5TFYCU which
prevents others from building; replace the literal value with a neutral setting
(e.g. remove the hardcoded value or set DEVELOPMENT_TEAM = $(DEVELOPMENT_TEAM)
or an empty string) so team ID is not locked into the repo; update any matching
entries that set DEVELOPMENT_TEAM (the duplicated occurrences) so the project
uses a variable or no-team default and developers/CI can override it locally or
via environment.
In `@TablePro/Core/Autocomplete/SQLSchemaProvider.swift`:
- Around line 112-117: fetchFreshTables() currently returns [] both when
cachedDriver is nil and when the DB returned an empty schema, conflating “no
driver” with “empty result”; change fetchFreshTables() to return an optional
([TableInfo]?) and return nil when cachedDriver is nil (otherwise return the
fetched array, possibly empty), then update LiveTableFetcher.fetchTables(force:)
to treat any non-nil return (including []) as the definitive fresh result and
only trigger alternative logic when fetchFreshTables() returns nil; ensure all
call sites of fetchFreshTables() are updated to handle the optional result.
---
Nitpick comments:
In `@TablePro/Views/Main/MainContentCoordinator.swift`:
- Around line 315-319: The reloadSidebar() method added to
MainContentCoordinator should be moved into a dedicated extension file to avoid
growing the main coordinator; create a new file named something like
MainContentCoordinator+SidebarRefresh.swift, add an extension on
MainContentCoordinator containing reloadSidebar(), and ensure it calls
schemaProvider.invalidateTables() and sidebarViewModel?.forceLoadTables() with
the same access level and any required imports so behavior and visibility remain
unchanged.
In `@TableProTests/ViewModels/LiveTableFetcherTests.swift`:
- Around line 142-194: Add a new test function (e.g.,
forceRefreshEmptySchemaEdgeCase) that mirrors the existing force tests but
covers the empty-schema edge case: create a MockDatabaseDriver, pre-load
SQLSchemaProvider with some initial table(s) via provider.loadSchema(using:),
then call await provider.invalidateTables(), set mockDriver.tablesToReturn = []
(empty array), instantiate LiveTableFetcher(connectionId: UUID(),
schemaProvider: provider) and call try await fetcher.fetchTables(force: true),
assert the returned result is empty, assert mockDriver.fetchTablesCallCount
incremented by 1, and finally assert await provider.getTables() returns an empty
array; reference symbols: LiveTableFetcher.fetchTables(force:),
SQLSchemaProvider.loadSchema(using:), SQLSchemaProvider.invalidateTables(),
SQLSchemaProvider.getTables(), MockDatabaseDriver.tablesToReturn and
MockDatabaseDriver.fetchTablesCallCount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26dd2956-166c-478e-af1e-6ab23f4460ed
📒 Files selected for processing (9)
CHANGELOG.mdTablePro.xcodeproj/project.pbxprojTablePro/Core/Autocomplete/SQLSchemaProvider.swiftTablePro/ViewModels/SidebarViewModel.swiftTablePro/Views/Main/MainContentCoordinator.swiftTableProTests/ViewModels/LiveTableFetcherTests.swiftTableProTests/ViewModels/SidebarViewModelTests.swiftTableProTests/Views/Main/CoordinatorReloadSidebarTests.swiftTableProTests/Views/SwitchDatabaseTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/ViewModels/SidebarViewModel.swift`:
- Around line 163-168: The cancel flow in forceLoadTables cancels loadTask which
can cause the underlying loadTables/loadTablesAsync error handling to treat Task
cancellations as real errors and set errorMessage; update the error handling in
loadTables (and any await of loadTablesAsync) to explicitly ignore
CancellationError/Task.isCancelled (or check
Task.currentPriority/Task.isCancelled) and return without setting isLoading or
errorMessage, and ensure forceLoadTables still clears loadTask and resets
isLoading before calling loadTables(force: true) so cancellations do not surface
spurious user-facing errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32245f26-3d21-43be-ac82-fbd257146e31
📒 Files selected for processing (4)
TablePro/Core/Autocomplete/SQLSchemaProvider.swiftTablePro/ViewModels/SidebarViewModel.swiftTablePro/Views/Main/MainContentCoordinator.swiftTableProTests/ViewModels/LiveTableFetcherTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- TablePro/Views/Main/MainContentCoordinator.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+SaveChanges.swift:
- Around line 236-242: The current logic always resets tabManager.selectedTabId
to the first remaining tab after removing tabs, which changes focus even when
the previous selection still exists; update the removal block around
tabIdsToRemove so it only picks a new selection when the existing
tabManager.selectedTabId is nil or was among the removed IDs (i.e. check whether
tabIdsToRemove.contains(tabManager.selectedTabId) or tabManager.selectedTabId ==
nil), otherwise leave tabManager.selectedTabId unchanged; reference
tabIdsToRemove, tabManager.tabs and tabManager.selectedTabId when applying this
conditional selection change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b429e91-91fa-4d17-9aa6-14a621f34879
📒 Files selected for processing (2)
CHANGELOG.mdTablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift (1)
12-12: Missing explicit access control on extension.As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
Suggested fix
-extension MainContentCoordinator { +internal extension MainContentCoordinator {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+SaveChanges.swift at line 12, The extension declaration "extension MainContentCoordinator" lacks an explicit access modifier; update the declaration to include the correct access control (for example "internal", "public" or "private") based on the coordinator's intended visibility so it matches project guidelines—e.g. change the line declaring the extension of MainContentCoordinator to explicitly specify access control; ensure any types or members inside that extension do not conflict with the chosen modifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+SaveChanges.swift:
- Line 12: The extension declaration "extension MainContentCoordinator" lacks an
explicit access modifier; update the declaration to include the correct access
control (for example "internal", "public" or "private") based on the
coordinator's intended visibility so it matches project guidelines—e.g. change
the line declaring the extension of MainContentCoordinator to explicitly specify
access control; ensure any types or members inside that extension do not
conflict with the chosen modifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b639901d-f2ff-4c7a-a789-a4aaf2ff55b6
📒 Files selected for processing (2)
TablePro.xcodeproj/project.pbxprojTablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- TablePro.xcodeproj/project.pbxproj
datlechin
force-pushed
the
fix/sidebar-refresh-stale-cache
branch
from
d47fec0 to
df7bf14
Compare
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
1 participant
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.
Summary
LiveTableFetcher.fetchTables()always returned cached data fromSQLSchemaProvider— refresh never bypassed the cacheforce: Boolparameter toTableFetcherprotocol; whenforce: true, bypass cache and fetch fresh data from the driverinvalidateTables()toSQLSchemaProviderthat clears only the table list (preserves column cache for autocomplete)reloadSidebar()now invalidates table cache before callingforceLoadTables()PROVISIONING_PROFILE_SPECIFIERthat conflicted with automatic signing for local developmentTest plan
fetchTables(force:)signatureSummary by CodeRabbit