feat(datagrid): pick a foreign key value from the rows it references - #2610
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
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.
What
Editing a foreign key cell meant knowing the target row's key by heart. The grid gave
Track.AlbumIdthe same plain text overlay it gives any other integer column: no list of valid keys, no name for the row a key points at, and the mistake only surfaced when the server rejected the save.A writable foreign key cell now opens a picker instead of that editor. Double-click,
Return, or Choose … Row… on the cell's right-click menu.Before / After
Before,
ReturnonTrack.AlbumIdopened the plain text overlay over the integer5, with nothing to say which album that is or which other keys exist.After, the same keystroke lists the rows it points at, with
Album.Titlebeside each key, a check on the one the cell already holds, and Set NULL because the column takes it. This shot ships as the docs page's image, so it renders from the branch:The dark pair is
docs/images/fk-value-picker-dark.png. The before shot is not committed, since nothing indocs/uses it; it is atscratchpad/before-light.pnglocally if it is wanted inline.How
The grid already had the reference.
TableRows.columnForeignKeyscarries aForeignKeyInfoper column, prefetched for the whole schema bySchemaForeignKeyStore, and the navigation arrow, Preview Referenced Row and the JSON inspector's key expansion all read it. Nothing used it to help write a key.CellInteractionResolvergains.editForeignKey, returned from the plain-text branch when the cell is writable, so a foreign key column holding a blob, JSON or PHP-serialized value keeps the editor its content needs and a read-only cell keeps every viewer it has.DataGridView+Popoverspresents the picker through the existingactiveCellEditorPopoverslot andcommitPopoverEdit, which is the same anchoring, dismissal and commit path the JSON, hex, date and array editors already use, and it falls back to the plain editor whenever the picker cannot be built.Both reads go through
DatabaseManager.withMetadataDriver: the referenced table's columns because that is the rule for a metadata read, and the rows because a search runs on every keystroke and the session driver is the one carrying the user's own query.ForeignKeyRowFetcherreads its single row on the session driver; a picker cannot afford to. The scope comes from the grid's owndatabaseName/schemaName, not frombrowseScope, so a tab that stays on the database it opened is not read against whatever the sidebar moved to.The search predicate is a type question
FilterSQLGeneratorowns the dialect, the case folding and the escaping, soForeignKeyLookupQueryreuses it and decides only which columns may carry a predicate at all:containspredicate goes only on a column whose raw type name is a known character type.ColumnTypecannot answer this:ColumnTypeClassifierfilesUUID,UNIQUEIDENTIFIERand every type it does not recognise under.text, and PostgreSQL has no~~foruuid, for an enum or for an array. The whitelist fails closed, so an unfamiliar type costs a search rather than an error on every search.containsfor a character key,equalfor a numeric key when the term reads as a number,equalfor auuidkey when the term parses as one.renderLiteralquotes anything else, andAlbumId = 'rock'on an integer, or a malformed literal on auuid, is an error rather than a query that returns nothing.<key> IS NOT NULLalways, ahead of the ordering and the limit. A referenced column may be a nullableUNIQUEone, and ascending order puts its NULLs first, so the page could be fifty rows the picker then discards.ORDER BYthe key, withLIMIT 50orOFFSET … FETCH NEXTper the dialect's pagination style.The label column
Resolved from the stored choice, then a short preferred-name list (
name,title,label,username,email,code,description), then the first text-like column that is not the key. The stored choice is honoured only when the table still carries that column, because the name reaches the query as a quoted identifier.Remembered per referenced table rather than per source column:
orders.user_idandcomments.user_idboth wantusers.name, so setting it once forusersis what remembering it should mean. Device-localUserDefaultsthrough the injectableKeyValueStore, the same shape asValueDisplayFormatStorage, so it needs no CloudKit record type.What review changed
Eight defects, each now with a test where one can reach it. The first two came from driving the built app against the Chinook sample, the rest from the Codex pass.
Use "…"led the list and was preselected whatever the user typed, so searching Album by title withBigput the search term underReturnand would have writtenBiginto an integer column. The term is offered as a value only when it could be a key.With nothing typed the head of the list was selected, so opening the picker and pressing
Returnwrote the referenced table's first key over the one already in the cell. The cell's own value is selected instead, and nothing at all when the first page does not carry it.A column of a composite key no longer opens the picker at all. The picker writes one column, so it would have offered a list of keys of which only some pair with the values the row already holds in the constraint's other columns, and the save is rejected on a reference the picker presented as valid.
ForeignKeyConstraintSpanreads the span from the constraint name the columns share; an unnamed constraint is read as single-column, which costs a picker rather than a refused write.A row is identified by its position, not by its key. A foreign key may reference one column of a composite unique key, which is not unique on its own, so two rows can carry the same key and two entries sharing an id is undefined behaviour in the
Listthat renders them.An empty column read no longer leaves the spinner up for good. The search waited on a non-empty column list, so a referenced table that answered with none left the picker loading with no way out. It waits on the read having finished instead, and the key guard reports what is wrong.
Every search on a PostgreSQL UUID foreign key was an error.
uuidclassifies as.text, so the key took acontainspredicate and the query came outuuid_column ILIKE '%…%', which PostgreSQL rejects. UUID keys are common enough that this alone would have made the feature useless on a large share of PostgreSQL schemas. The predicate rules above are the fix, and the automatic label pick narrowed with them.A nullable referenced key could show an empty list over a full table. Ascending order puts NULLs first, the limit took fifty of them, and
rows(from:)discarded all fifty because a NULL key references nothing.A generated column with foreign key metadata opened a picker whose commit went nowhere.
CellInteractionResolverknows only the columns the plugin declares immutable, so the picker opened, the choice was dropped byrecordCellEdit, and the popover closed over an unchanged cell.showForeignKeyPickeraskscanStartInlineEditagain.A
Returnduring an in-flight search committed the previous row's key. Rows and selection survived the debounce, so typing a new key and pressingReturnbefore the lookup landed found the old row id in the old entries. The selection is dropped the moment the search changes.Verified
verify.sh buildverify.sh test(17 suites)verify.sh lint TablePro TableProTests TableProUITestsverify.sh docsSet NULLand the current-value check all driven by handNew suites:
ForeignKeyLookupQueryTests(both pagination styles, numeric, character anduuidkeys, a type the classifier only guessed at, an enum label, a term no column can carry, the NOT NULL guard, quote and wildcard escaping on both LIKE conventions),ForeignKeyPickerEntryTests,ForeignKeyConstraintSpanTests,ForeignKeyLabelColumnTests,ForeignKeyLabelColumnStoreTests, plus a foreign key suite inCellInteractionResolverTests.ForeignKeyPickerUITestsdrives the flow against the Chinook sample. It did not run locally: XCUITest cannot start while another TablePro is running, and one was, so CI is the first run. No PluginKit change, so the ABI check does not apply; nothing underPlugins/, so the plugin aggregate does not either.Left for a follow-up
Two findings from the same review are real and pre-existing rather than introduced here, so they are not in this diff:
REFERENCED_TABLE_SCHEMA, andtargetScopeputs it inschemawhile the connection stays on the current database. MySQL advertises no schema switching and itsfetchColumnsignores the schema argument, so the read lands on the wrong database.ForeignKeyRowFetcherhas resolved it this way since Preview Referenced Row shipped, and fixing it means teaching the scope that a.byDatabaseengine's referenced namespace is a database..task, but pluginexecuteruns its C query through non-cooperativepluginDispatchAsync, so the metadata pool serialises the replacement behind a%term%scan that no longer has a reader. Every metadata read in the app has this property; cancelling one needs a handle on the pooled driver's running query.Not in this change
FieldEditorContextcarries no foreign key information, so a picker there means plumbing it throughRightSidebarViewandFieldEditorResolver. Worth doing separately.Fixes #2511
https://claude.ai/code/session_01HrHeVBX1ud3gtERNhc8LXn