Skip to content

fix(datagrid): restore FK jump arrows lost after sort, filter, or paginate#1663

Merged
datlechin merged 5 commits into
mainfrom
fix/fk-arrows-phase2-drop
Jun 12, 2026
Merged

fix(datagrid): restore FK jump arrows lost after sort, filter, or paginate#1663
datlechin merged 5 commits into
mainfrom
fix/fk-arrows-phase2-drop

Conversation

@datlechin

Copy link
Copy Markdown
Member

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:

  1. Generation race. Phase 2's apply was guarded by capturedGeneration == queryGeneration. Any re-query in the window while the .utility schema 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.
  2. Silent fetch failure. fetchColumns and fetchForeignKeys ran in one metadata-pool closure; an FK fetch error (for example missing information_schema privileges) lost the columns too, and try? await schemaTask?.value swallowed 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, and isMetadataCached inputs (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 (QueryExecutor category) and degrades to a nil FK result; columns always survive. SchemaResult.fkInfo is now optional so a failed fetch (nil) stays distinguishable from a table with zero FKs ([]).
  • New foreignKeysFetched flag on TableRows, set only when a real FK result (including an empty one) was applied, carried across re-queries, and required by isMetadataCached. 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

  • New TableRows - foreignKeysFetched suite (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.
  • New regression test in 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.
  • New parseSchemaMetadata case: a failed FK fetch (fkInfo: nil) stays distinguishable from zero FKs.
  • Updated QueryExecutorTests for the optional FK type. parseSchemaMetadataExtractsEnumValues fails identically on clean main in this environment (pre-existing, plugin-dependent).
  • The Phase 2 timing race itself needs a mock driver to test deterministically and the suite has none (existing TODO in QueryExecutorTests); covered by manual QA instead: open a table with FKs, then sort, filter, paginate, navigate away and back; arrows persist at every step. If the reporter's setup hits the fetch-failure mode, Console now shows [fetchTableSchema] FK fetch failed under the com.TablePro / QueryExecutor log category.

@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: 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".

Comment on lines +259 to +262
if let schema,
parent.tabManager.tabs.contains(where: {
$0.id == tabId && $0.tableContext.tableName == tableName
}) {

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 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 👍 / 👎.

@datlechin

Copy link
Copy Markdown
Member Author

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 [fk] prefix:

Stage Log line What it proves
Decision [fk] metadata decision table=... isEditable=... needsFetch=... Whether a schema fetch is even attempted
Cache [fk] cache check table=... defaults=N pks=N fkFetched=... fks=N enumsReady=... cached=... Why the cache considered metadata hot or cold
Fetch [fk] schema fetch start table=... db=... schema=... / schema fetch done ... fks=N|failed The scope the metadata connection used, and whether FK fetch succeeded
Driver [fk] mysql fetchForeignKeys db=... table=... rows=N parsed=N (same for postgres with schema=) What the database actually returned for the FK query
Phase 2 [fk] phase2 schema fetch failed or cancelled / phase2 apply skipped / phase2 applied ... fks=N activeTabRefreshed=... Whether the result was applied, dropped, or failed
Grid [fk] grid columns=N fkColumns=N fkMeta=N Whether the rendered grid received FK columns

To capture, run this in Terminal while reproducing (open the affected table once):

log stream --predicate 'eventMessage CONTAINS "[fk]"' --info

Reading the output:

  • driver rows=0 with the right db= means the server returned no FK rows for that scope: server-side grants or schema mismatch, not app code.
  • db= or schema= pointing at the wrong database means a metadata-connection scoping bug.
  • needsFetch=false with fkFetched=false in the cache line means a cache-gating bug.
  • phase2 applied fks=N with grid fkColumns=0 means a grid-side rebuild bug.

@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: 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".

Comment on lines +134 to +136
foreignKeys = try await DatabaseManager.shared.withMetadataDriver(connectionId: connectionId) { driver in
try await driver.fetchForeignKeys(table: tableName)
}

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 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 👍 / 👎.

@datlechin

Copy link
Copy Markdown
Member Author

The trace found the real root cause for the reporter. Their log showed a healthy pipeline with the failure at the driver stage:

[fk] postgres fetchForeignKeys schema=public table=order_items rows=0 parsed=0

The PostgreSQL plugin read FKs through information_schema, and that query joins constraint_column_usage. Per PostgreSQL's documentation, that view only returns columns of tables owned by a currently enabled role. Connect with a role that has privileges but no ownership (the standard app-role setup after a migration) and the join eliminates every row: zero FKs, no error. The query has been like this since the plugin extraction; the reporter most likely switched from connecting as the table owner to a granted role, which is why it "worked before".

Fixed in 784a405 by rewriting fetchForeignKeys and fetchAllForeignKeys on pg_catalog (pg_constraint + unnest(conkey, confkey) WITH ORDINALITY), the same source psql \d uses. pg_catalog visibility does not depend on ownership, composite FKs map column-by-column, and confdeltype/confupdtype map to the same rule strings as before, so the Swift row mapping is unchanged.

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 [fk] tracing is what made this diagnosis a five-minute read instead of another guess.

@datlechin

Copy link
Copy Markdown
Member Author

Confirmed working by the reporter's scenario on a live database (ses_email_events: rows=1 -> fks=1 -> phase2 applied -> arrow renders). Final commit 94a05ef cleans the pipeline up after the incremental fixes:

  • SchemaResult tuple typealias replaced by a FetchedTableSchema struct (columns, foreignKeys: [ForeignKeyInfo]? where nil means the fetch failed, approximateRowCount). Named type instead of a labeled tuple with an optional member.
  • fetchTableSchema split into the column/row-count fetch plus a private fetchForeignKeys helper that owns the error boundary and logging.
  • launchPhase2Work flattened: the apply paths moved into applySchemaMetadata and applyEnumValues, sharing tabShowsTable and isActiveTab helpers with applyPhase2Metadata.
  • One deliberate behavior alignment: enum-value application now uses the same table-scoped guard as FK metadata instead of the query-generation guard. Enum values are table-scoped like FKs; the old generation guard could drop them under the same race this PR fixes for arrows. Same bug class, same cure.
  • Earlier commit 6276f02 also fixed the last render gap found via the trace: refreshForeignKeyColumns now rebuilds the column kind sets from fresh data before reloading cells, so arrow rendering does not depend on structural-update ordering.

No functional changes beyond the enum-guard alignment; all suites pass (one pre-existing environment-dependent failure on clean main, noted earlier).

@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: 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".

Comment on lines +256 to +257
CROSS JOIN LATERAL unnest(con.conkey, con.confkey)
WITH ORDINALITY AS cols(src_attnum, ref_attnum, ord)

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 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 👍 / 👎.

@datlechin datlechin merged commit 7c2081b into main Jun 12, 2026
4 checks passed
@datlechin datlechin deleted the fix/fk-arrows-phase2-drop branch June 12, 2026 06:51
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.

1 participant