fix(datagrid): restore FK jump arrows lost after sort, filter, or paginate#1663
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef3ff65a49
ℹ️ 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".
| if let schema, | ||
| parent.tabManager.tabs.contains(where: { | ||
| $0.id == tabId && $0.tableContext.tableName == tableName | ||
| }) { |
There was a problem hiding this comment.
Include database/schema in the stale metadata guard
When a Phase 2 schema fetch from an older generation finishes after the same tab has been reused for a table with the same name in a different database or schema, this guard still passes because it only compares tabId and tableName. QueryTabManager tracks databaseName and schemaName separately for table tabs, so navigating from e.g. db1.orders to db2.orders before the old metadata task completes can apply stale defaults/PK/FK metadata to the new table and show wrong FK arrows. The generation guard was what previously prevented this cross-context apply, so the replacement guard needs to also verify the captured database/schema context.
Useful? React with 👍 / 👎.
|
Added structured tracing across the whole FK pipeline (a907afc) since the first fix did not resolve the reporter's case. Every stage now logs with an
To capture, run this in Terminal while reproducing (open the affected table once): Reading the output:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a907afc6b3
ℹ️ 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".
| foreignKeys = try await DatabaseManager.shared.withMetadataDriver(connectionId: connectionId) { driver in | ||
| try await driver.fetchForeignKeys(table: tableName) | ||
| } |
There was a problem hiding this comment.
Keep schema metadata reads on the same context
When the user switches database/schema while the first metadata call is still running, this new second withMetadataDriver call re-reads the current session (see DatabaseManager.withMetadataDriver) and can fetch foreign keys from the new database/schema while returning columns/defaults from the old one. For same-named tables, the later table-name-only apply path can mark foreignKeysFetched and cache the wrong FK arrows. Fresh evidence versus the prior guard comment is this newly split second metadata-driver acquisition; keep the FK fetch on the same captured driver/context as the column fetch or pass the captured database/schema through both calls.
Useful? React with 👍 / 👎.
|
The trace found the real root cause for the reporter. Their log showed a healthy pipeline with the failure at the driver stage: The PostgreSQL plugin read FKs through Fixed in 784a405 by rewriting Verified empirically against PostgreSQL 18.4: with a probe role granted SELECT but owning nothing, the old query returns 0 rows for a table with an FK; the new query returns the FK with correct rules. CockroachDB's driver copies the old pattern but CRDB's information_schema does not implement PostgreSQL's ownership restriction, so it is unaffected. The earlier commits in this PR remain valid fixes for the two Phase-2 drop modes, and the |
|
Confirmed working by the reporter's scenario on a live database (
No functional changes beyond the enum-guard alignment; all suites pass (one pre-existing environment-dependent failure on clean main, noted earlier). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a05ef68f
ℹ️ 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".
| CROSS JOIN LATERAL unnest(con.conkey, con.confkey) | ||
| WITH ORDINALITY AS cols(src_attnum, ref_attnum, ord) |
There was a problem hiding this comment.
Keep foreign-key lookups working on PostgreSQL 9.3
On PostgreSQL servers older than 9.4, this new WITH ORDINALITY/multi-array unnest syntax is a parse error, so FK metadata fetches fail and the grid never marks foreign keys as fetched. The plugin still has explicit compatibility gates for older PostgreSQL versions (for example hasMaterializedViewsCatalog at 9.3 and the guarded array_position use), so this replaces a broadly compatible information_schema query with one that breaks those supported connections; add a version-gated fallback or avoid the 9.4-only syntax here and in the matching fetchAllForeignKeys query.
Useful? React with 👍 / 👎.
What
Fixes a user report (v0.50.0 release build): foreign key cells in the data grid stop showing the jump arrow, so clicking an id no longer navigates to the referenced table.
Root cause
No single commit removed the arrow; the render and hit-test code is unchanged since v0.46.0. The seam is 94ec469 (#1574, v0.49.0), which split table loading into two phases: rows render first, FK metadata arrives later via a deferred Phase 2 task. Phase 2 results can be dropped silently in two ways:
capturedGeneration == queryGeneration. Any re-query in the window while the.utilityschema task runs (sort, filter, paginate, the v0.49.x default-sort double query) bumps the generation and the valid FK result is thrown away. FK metadata is table-scoped, not query-scoped, so the guard was wrong for it.fetchColumnsandfetchForeignKeysran in one metadata-pool closure; an FK fetch error (for example missinginformation_schemaprivileges) lost the columns too, andtry? await schemaTask?.valueswallowed it with zero logging. A persistent error means the arrows silently fail on every load.Ruled out with evidence during investigation: metadata-pool schema replication, bundled plugin changes, PluginKit transfer types, asset packaging,
resolveTableEditability, andisMetadataCachedinputs (all byte-identical across the release tags).Fix
launchPhase2Work: the metadata apply is now guarded by the tab still existing and still showing the same table (tabId+tableName), instead of generation equality. Row count keeps the strict generation guard since it is query-scoped.fetchTableSchema: the FK fetch is isolated from the column fetch. A failure is logged through OSLog (QueryExecutorcategory) and degrades to a nil FK result; columns always survive.SchemaResult.fkInfois now optional so a failed fetch (nil) stays distinguishable from a table with zero FKs ([]).foreignKeysFetchedflag onTableRows, set only when a real FK result (including an empty one) was applied, carried across re-queries, and required byisMetadataCached. A transient FK failure self-heals on the next load; a persistent failure retries one cheap metadata query per load and keeps logging instead of failing silently forever. A zero-FK table warms the cache normally and does not re-fetch in a loop.Note: splitting the fetch is what makes the flag necessary. Before the split, defaults and FKs always populated together, so an FK failure left the cache cold by itself. After the split, columns can succeed while FKs fail; without the flag a single transient error would hide the arrows for the rest of the session.
Pre-existing behavior left unchanged: view tabs (
isEditable = false) never fetch FK metadata, so views do not show FK arrows. That predates this fix.Testing
TableRows - foreignKeysFetchedsuite (5 cases): default false, set by a real FK apply, set by an empty FK apply, untouched by other metadata updates, preserved by the factory.FKNavigationTests: a tab with populated defaults and primary keys but no fetched FK result must not be considered metadata-cached. This encodes the self-heal contract.parseSchemaMetadatacase: a failed FK fetch (fkInfo: nil) stays distinguishable from zero FKs.QueryExecutorTestsfor the optional FK type.parseSchemaMetadataExtractsEnumValuesfails identically on clean main in this environment (pre-existing, plugin-dependent).[fetchTableSchema] FK fetch failedunder thecom.TablePro/QueryExecutorlog category.