feat(datagrid): show the selected row as JSON, with foreign keys that expand into the rows they reference - #2593
Conversation
… expand into the rows they reference
…f once per body pass
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fb074ec7f
ℹ️ 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".
| var contentHasher = Hasher() | ||
| contentHasher.combine(row.values) | ||
| contentHasher.combine(tableRows.columns) |
There was a problem hiding this comment.
Rebuild the JSON tree when foreign-key metadata changes
On a cold schema fetch, tableRows.columnForeignKeys is populated after a result row may already be selected, but this token only includes the cells and column names. The subsequent inspector update therefore takes the same-row early return and keeps the original tree without foreign-key nodes, so Expand controls never appear until the selection changes. Include foreign-key metadata (and column types) in change detection or compare the full snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and column types had the same hole. Fixed in d3e5e32.
The token is gone rather than extended. JSONRowSnapshot is Equatable, so the view model holds the last snapshot and compares the whole value; rowIdentity stays, but only to answer "is this a different row" for the reader's expansions. A hand-written token over the parts that seemed to matter was the defect, so nothing hand-written replaced it.
JSONRowSnapshotChangeTests covers late columnForeignKeys, late columnTypes, moved values on a stable identity, and the unchanged case.
| if !isSameRow { | ||
| cancelFetches() | ||
| states = JSONForeignKeyStates() | ||
| chains = [:] | ||
| } |
There was a problem hiding this comment.
Clear foreign-key expansions when reloading the same row
When a query is re-executed or refreshed, rows retain identities based on their tab and offset while their values can change. For that same identity this branch leaves states.fetched and active fetches intact; because the renderer substitutes a fetched expansion solely by its path, a new FK value such as artist_id = 2 can still show the referenced row previously fetched for artist_id = 1. Cancel and clear FK state, or version it by the content token, before rebuilding a changed row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and it is the worse of the two: rowIdentity is the tab plus the row's position, so a rerun keeps it while the values move underneath, and states.fetched is keyed by the node's path alone. ArtistId going 1 to 2 kept Artist 1 printed under it, presented as this row's own data.
Fixed in d3e5e32: any content change now cancels the in-flight fetches and clears states and chains before the rebuild, not just a change of row.
The same commit also replaces the fetch's own guard, which compared snapshotIdentity — exactly the value that survives a rerun, so a late-returning query could still write into a rebuilt tree. It is a monotonic generation counter now, bumped by every rebuild and every reset, matching the connection-attempt generation guard in DatabaseManager.
| for (index, column) in columns.enumerated() { | ||
| let value = index < values.count ? values[index] : .null | ||
| let type = index < columnTypes.count ? columnTypes[index] : nil | ||
| let childPath = path.appending(column) |
There was a problem hiding this comment.
Make JSON paths unique for duplicate result-column labels
Queries can return duplicate labels, such as an unaliased join selecting two id columns, but both nodes receive this same path. That produces duplicate JSONDisplayRow.id values in the SwiftUI ForEach and also merges collapse and fetched-FK state for the two columns, so duplicate-labeled JSON/container columns cannot be rendered or expanded independently. Add the top-level column occurrence/index to the path while retaining the original label as the JSON key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. SELECT a.id, b.id gives columns == ["id", "id"], so both nodes shared a path: one id in the ForEach, and one entry in both the expanded set and the fetched-key map.
Fixed in d3e5e32 the way you describe: the path component leads with the column's position (0.id, 2.id) while JSONNodeKey keeps the plain label, so the printed JSON is unchanged. It is the same shape the document members already used for duplicate keys inside one object.
separatesDuplicateColumnLabels in JSONRowNodeBuilderTests pins it. Four existing tests spelled child paths out by hand and now ask the tree for them instead.
…hanges, and give each column its own path
Signed-off-by: Nana Kwesi Ofosu-Aikins <61139144+nanaaikinson@users.noreply.github.com>
… settings withdrew
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review pass over the branchA second review of the whole diff against What the JSON tab printed, filtered and copied
Foreign key fetches
The model took its lookup from The comment on The panel
Docs and changelog
The page said "keys in the grid's column order"; the tab reads the result's own columns and shows the hidden ones, exactly as Details does. It said "nothing is truncated" over a blob display cap. It said "A foreign key key". The section had no screenshot, which every other feature page carries, so there is a light and dark pair now.
Verified
One failure is inherited, not from this branch: CI has never run on this PR. All eight workflow runs are The screenshots are 1512x861 rather than the usual 3024x1722, because the machine that took them has a 1080p display with no backing scale. They are real captures of this branch, not placeholder cards, and worth re-taking on a retina machine before release. |
…validate a scalar document
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adversarial pass over the fixes aboveA second reviewer went at the approach rather than the lines, and returned a no-ship. Three of its findings were mine to answer, and one of them reversed a call I had made. A value match was keeping the whole referenced row
Key matching and value matching are separate now. A key keeps its subtree; a value keeps its own line and the keys that lead to it. The filter walked the same subtree once per matching ancestorThe old shape scanned every descendant, then re-walked those same descendants through It is one DFS now: whether the key matches is decided before descending and carried down, so every node is visited once. A scalar document is validated before it is retypedLetting a JSON column keep a top-level scalar was right, but A scalar is now admitted only if And one I had wrongI had made Copy Value carry a blob whole, on the grounds that the pasteboard should hold the value rather than the rendering of it. The reviewer called it an unbounded main-actor encode, which sent me to look at what the rest of the app does: Copy Value is capped at the same 64 bytes now. The quotes were the actual bug; the cap was never it. The docs line changed with it. The hex itself still goes through a byte buffer rather than a character-at-a-time append, and the Not fixed, and why
Verified again
|
The inspector gains a third tab that prints the selected row as JSON and lets a foreign key be followed without leaving the row. Modelled on Beekeeper Studio's details view, which is what the request asked for.
Two commits: the tab, then a performance fix to shared inspector code that the tab exposed.
The JSON tab
Show Row as JSON on a row's right-click menu opens the inspector on the tab; the tab is also reachable from the segmented control, which now appears whether or not the AI assistant is enabled (AI Chat drops out of the list when disabled rather than sitting there unselectable).
The row prints as one JSON object, keys in the grid's column order. Whether a value is quoted comes from the column's own type, never from the text:
INTandDECIMALprint bare throughJsonNumberNormalizer, so aBIGINTholding 9007199254740993 keeps all 16 digits;BOOLreads the spellings the drivers emit; a blob prints as hex; NULL prints asnull. A JSON column, and a text column whose value is shaped like a document, expand as nested keys. That parse isJsonSyntaxParser, the one the JSON cell viewer already reads with, so a document cannot render one way in a cell and another in the row.Following a key
A foreign key column carries a disclosure control. Opening it fetches the referenced row, and a foreign key inside that row opens the same way, five levels deep. The fetch is
ForeignKeyRowFetcher, extracted from the fetch that used to live insideForeignKeyPreviewView, so Preview Referenced Row and the tab now share one query path, one set of identifier quoting, and one dialect-aware limit clause. Metadata for a nested key answers fromSchemaForeignKeyStorewhen the schema prefetch covers the table, and otherwise goes throughwithMetadataDriver, so the embedded-engine pooling rule holds.employee.manager_id → employeeis ordinary schema design, so the chain is checked rather than trusted:JSONForeignKeyExpansionPolicyrefuses a key already followed with the same table, schema, column and value, and refuses anything past the fifth level. Both report on the row with the reason. A NULL key offers no control at all.Always Expand Foreign Keys fetches the first level on every row selected from then on. It is session state, off at every start, because each key it follows is a query.
Filtering
The filter field takes text, matched case- and diacritic-insensitively, or a regular expression wrapped in slashes. It keeps every match plus the keys that lead to one, opens what was collapsed, and searches inside foreign key rows already fetched. An invalid expression outlines the field and filters nothing rather than falling back to substring matching, which would silently answer a different question.
Copy Visible prints exactly the lines on screen, collapsed containers as
{…}and filtered-out keys not at all, through the same renderer the view draws from.Where the state lives
JSONRowInspectorViewModelis owned byRightPanelState, not by the tab, so switching to Details and back keeps the reader's expansions and the rows already fetched for them.The row reaches it from
RightPanelState.inspectorContext'sdidSetrather than from the view'sonChange.onChangeruns after the render that already observed the new value, so the first draft drew one frame of the previous record's tree on every row change: the tab visibly flickered. Writing the context and the model in the same turn means every render sees one consistent row.JSONRowSnapshotcarries its ownconnectionIdanddatabaseTypefor that, which also makes it impossible to hand the model a row from one connection with another connection's id.The tab is read-only. Editing stays on Details, which owns the whole write path.
The performance fix
FieldEditorResolver.resolverunslooksLikeJson, a fullJSONSerializationparse of the value, and then a PHP-serialized parse after it. It was reached from two view bodies per field,RightSidebarView.fieldDetailRowandFieldDetailView.body, so a row with long text columns re-parsed every value twice per body pass: on every inspector tab switch, on every hover (isHoveredis@State), and on every keystroke in a pending edit.FieldEditState.resolvedEditornow holds the answer, filled once whenMultiRowEditStatebuilds the fields, which is once per selection. The view passes the resolved kind down throughFieldEditorContext.editorso the second resolve short-circuits.Separately, the Details tab stays mounted and is hidden rather than rebuilt when another tab is selected. Its field list is a
List, so leaving the tab tore down anNSTableViewand a field editor per column and returning built them again, with a cost that grows with the row's width. It is hidden withopacity(0),allowsHitTesting(false),disabled(true)so its text fields leave the key view loop, andaccessibilityHidden(true). JSON and AI Chat stay conditional: JSON is aLazyVStackand cheap to rebuild with its state held elsewhere, and mounting AI Chat eagerly would create its view model and load conversations on windows that never open it.Tests
TableProTests/Models/JSON/holds 37 cases over the pure layer: the builder's typing rules and its document parsing, the filter's substring, regex, ancestor and fetched-subtree behaviour, the flattener's braces, commas, collapsed tokens, disclosure state and per-key status, the expansion policy's cycle and depth decisions, and the Copy Visible renderer.JSONRowInspectorUITestsdrives the real flow on Chinook: the context menu item opens the tab, and expandingAlbum.ArtistIdfetches the Artist row.Verified
swiftlint lint --strictoverTablePro,TableProTests,TableProUITestsdocs/scripts/check-writing-style.shmodifier glyphfailure asmain, no new onedocs/scripts/check-docs-against-source.pyxcodebuild build(Debug)TableProTestsxcodebuild testJSONRowInspectorUITestsThe machine this was written on runs Xcode 26.2; CI pins 26.4.1. Under 26.2 five files that this branch does not touch fail to compile:
CompareRowService(its closure parameters cross an isolation boundary),AIChatViewModel+PersistenceandInspectorViewController(sending 'self' risks causing data races),PluginMetadataRegistry+RegistryDefaults(type-check timeout), andRedisReplyin the test target. The successful build above needed temporary local edits to four of them plus a raised-solver-expression-time-threshold; every edit was reverted and none of it is in either commit. The test target's failure is why the suites and the UI test have not run here. CI is the first place they will.The Details↔JSON switch cost is likewise unmeasured. The two causes above are real and are fixed at their source, but nobody has timed the result.
Docs and changelog
features/json-viewer.mdxgains a Row as JSON section,features/data-grid.mdxpoints at it from its foreign key section.[Unreleased]gains three Added entries for the tab, the expansion and the filter, and one Fixed entry for the inspector lag, which predates this branch.