refactor(datagrid): stack the inspector's data fields so the name and the value stop competing - #2666
Open
datlechin wants to merge 2 commits into
Open
refactor(datagrid): stack the inspector's data fields so the name and the value stop competing#2666datlechin wants to merge 2 commits into
datlechin wants to merge 2 commits into
Conversation
… the value stop competing Claude-Session: https://claude.ai/code/session_012uFmqwYUhaBACJ6fzMVmT3
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…stops inheriting another's layout Claude-Session: https://claude.ai/code/session_012uFmqwYUhaBACJ6fzMVmT3
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
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.
The problem
At the inspector's 270pt minimum, a field row asked one 256pt line to hold four things that each want to grow: the column name, the type badge, the value editor and the value menu. It settled that contest with
.layoutPriority(1)on the name andmaxWidth: 150with nominWidthon the editor, so the name always won and the value took what was left.On
public.analytics_eventsthat meantconnection_succeeded_atrendered its value asNU…,machine_idas78ad8172e…,created_atas2026-02-13…. The type badge, having nolineLimit, wrapped inside its own capsule todatovereand grew those rows by 11pt. And because every row negotiated inside its ownHStack, no value column existed: nine consecutive fields started their values at nine different x positions.Root cause
InspectorFieldLayout.resolve(for:)chose the row's arrangement from the editor's size class. Those are different questions. The arrangement has to follow whose labels these are:StructureColumnField.displayNameplus the index, foreign-key and check-constraint headers), whose widest member, "Ref Columns", is 66.4pt.One geometry cannot serve both, which is why the enum could not express the fix. This is a refactor, not a patch.
What changed
A data field now puts its name and type on one line and its value at full width beneath, so every value in the pane starts at the same x and gets the whole content width. A schema field keeps a one-line row, with its label in a fixed 96pt trailing-aligned lane, which is the shape Apple's own inspectors use and which is achievable there because the vocabulary is closed.
TypeBadgegains.lineLimit(1)and.fixedSize(horizontal:vertical:), which fixes the wrap at its cause and also coversJSONTreeViewandPhpTreeView, which share the component.The status glyphs no longer reserve width on every row. A row with no key and no foreign key takes none, so its name sits at the same leading edge as its own value; only a key column is indented, where the indent means something. The unsaved-edit dot moved to the trailing end so recording an edit cannot shift the name it belongs to.
Both switches in
InspectorFieldLayoutstay exhaustive overFieldEditorKind, so a new editor kind must be classified for each provenance rather than inheriting whichever arm was written first. ThreadingisSchemaFieldis not optional:.enumPickeris the kind for a data row's ENUM column and for a structure row's On Delete dropdown, so the kind alone cannot say which shape a row takes.Why stacked rather than an aligned two-lane row
The two-lane inline row is the shape Apple's inspectors use, and it was the leading alternative. Rendered at the 270pt minimum with the same data, it truncates both halves: labels come out as
connec…eded_atandfirst_qu…uted_atwhile values still read2026-02-13 08:41…. Truncating both is strictly worse than truncating neither. Apple's labels are authored strings sized for their lane; a column name is not.Measured against Apple's own inspectors for the underlying principle: Finder's Get Info at 265pt wraps its long
Size:value to a second line rather than eliding it, and Xcode's File Inspector gives Full Path three lines. More vertical space, not an ellipsis, is the native answer to a value that does not fit. The label-line-above shape itself follows TablePlus, whoseRowDetailTextCellputs the name and type on one line and the value in anNSTextViewof the full width beneath, and Postico, which does the same.Density was the real cost and the one thing three reviewers pushed back on. A stacked row is 44pt against an inline row's 25pt, but across a realistic column set a mixed layout only saves 16-19%, because the long rows have to stack in either design. What mixing costs instead is the thing this PR exists to fix: two label columns and two value columns alternating down the pane, the type badge dropped from exactly the rows too narrow to carry it, and a row that reflows from one shape to the other while the user is typing into it, since its shape would depend on the value's current length.
Also in this PR
Both are in the code this change rewrites, and both were found while investigating it.
A hex dump rendered in the proportional system font.
InspectorFieldRow.valueFont(for:)returnsnilfor.image, andImageFieldView's Hex branch rendersBlobHexEditorView, which named no font. So the identical dump drew monospaced under.blobHexand proportional under.image, and its offset, hex and ASCII columns stopped lining up.BlobHexEditorViewnow namesThemeEngine.shared.valueFontSwiftUIitself, which covers both routes, and is added to the path list inValueFontTests.standaloneValueViewsResolveTheValueFont.A BLOB over 10 KB lost the rest of itself on save.
formattedAsEditableHexstops at 10,240 bytes and marks the cut with a trailing ellipsis. The inline editor never checked for that marker, so it read it as a syntax error: an untouched value showed a permanent "Invalid hex", every edit was silently reverted on blur, and deleting the ellipsis let the commit write the prefix over the whole column. Measured on a 50,000-byte value: 10,240 bytes committed, 39,760 lost, with the save reporting success.Past the cap the field now shows its dump read-only and says so. It renders a selectable view rather than a disabled
TextField, because a disabled text field on macOS can neither take first responder nor have its text selected, which would have put the value out of reach of the keyboard and the pasteboard whileInspectorFieldListView.moveFocuswent on stopping at the row.The pop-out editor turned out to have the same bug, found by the code review:
validateHexrecomputed itsisTruncatedfrom the text view on every keystroke, so deleting the marker cleared the flag, re-enabled Save, and committed the prefix. Both editors now anchor truncation to the value they opened on rather than to the draft the user is typing into, which is the only version of the guard that cannot be talked out of.Verified
Run in a dedicated worktree, because another session was mid-refactor in the shared checkout.
verify.sh build: PASSverify.sh testover the eight suites that own the changed types: PASS, 109 executed, 109 passed, 0 failedverify.sh uitest InspectorFieldAffordanceUITests: PASS, 10 executed, 10 passed, 0 failedswiftlint lint --stricton the app, and on the four changed test files by explicit path, sinceincluded: [TablePro]never reaches the test targets: cleandocs/scripts/check-writing-style.shandcheck-docs-against-source.py: both passThe two new UI tests assert the shape as geometry rather than as text, because whether a given value is elided depends on the pane's width and on which row the runner selected, while "the editor begins below the name" holds at every width for every value.
Both were checked against the old layout to prove they guard something. Reverted to the shipped row they fail, and the failure message is the report restated as a measurement:
Nine editors, nine different leading edges. After the change there is one. The three pre-existing tests in that suite pass on both sides, so the two new ones are the only thing that moved.
The measurements quoted above come from compiled
swiftc+ImageRendererharnesses rather than from inspection. The harness reproduces the reported bug pixel for pixel,dat/ewrap included, which is what makes its verdict on the alternatives worth anything.The CI failure this branch exposed, and the leak behind it
QueryHistoryActionsUITests/testLoadInEditorFillsTheTabInFrontInsteadOfOpeningAnotherwent red on this branch and stayed red on a rerun, while passing on the merge base. It is not the inspector:TypeBadgereaches only the inspector and the JSON and PHP trees, neverHistoryDetailPane, and the editor a field builds is chosen byFieldEditorContentfrom the editor kind, which this change never touches. Two independent review lanes reached the same conclusion.The element tree XCTest captured at the failure says what actually happened. The window is
{{0,31},{1024,674}}, so its bottom edge is y=705. The wholequery-history-detailpane ran from y=545 to y=712, and all three of its actions sat at y 692 to 712:The drawer was laid out 7pt taller than the window, so the click on a button straddling the edge did nothing. The two TextViews present confirm it: the editor carried no value at all, and only the drawer's preview held the marker, so the count was 1 rather than 2.
The drawer got that geometry from a previous test in its shard.
NSSplitView.autosaveNameandNSWindowframe autosave both write into the app process's standard defaults domain, and the UI test sandbox does not redirect that one: it redirectsapplicationSupportRootand hands out its ownUserDefaults(suiteName:), and these records land in neither. So a divider position, a pane width or a window frame set by one case is inherited by every case after it in the same shard. CI shards by list position (runnable[index::count]), so the two UI cases this PR adds re-dealt every later case: this one moved from shard 2 to shard 1 and woke up with a different case's geometry.SplitViewAutosaveNamenamespaces every autosave record per sandbox, at the three places that assign one: the window split,AutosavingSplitView(which is what the history drawer's own list/detail divider uses) andNSWindow.applyAutosaveName. Production keeps the bare names, so no saved width, divider or frame a real user has is touched. A test asserts the namespacing and that all three call sites go through it, so a fourth autosave name added later cannot quietly reintroduce the leak.Review
Reviewed by Codex, twice. It found the truncation flag being derived from user-editable text (an ordinary blob could be locked read-only for good by pasting an ellipsis into it), the same defeatable guard in the pop-out editor, a docs sentence that promised full visibility at the minimum width when the Data Grid Font setting goes to 18pt, an
isTruncatedcomputed property that re-materialized a multi-megabyte blob on every body pass, and the disabled-field accessibility problem. All five are fixed here; none was dismissed.Not in this PR
The docs screenshots at
docs/images/row-details-inspector.pngand-dark.pngstill show the old layout and need retaking. I could not capture them here: this grid draws its cells rather than mounting views, and synthetic clicks fromosascriptdo not reach it, which is why the UI tests click a point offset from the grid element instead.https://claude.ai/code/session_012uFmqwYUhaBACJ6fzMVmT3