fix(datagrid): repair three broken Preview Referenced Row paths - #2324
Merged
Conversation
|
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.
Three defects found while investigating #2316 and reported on #2323. All three break Preview Referenced Row, none of them was needed for that fix, and each fails in a way that looks like a foreign key pointing at nothing rather than like a bug.
1. The menu command read the wrong column
MainContentCoordinator.toggleFKPreviewForFocusedCellturnedfocusedColumninto a data index by subtracting 1.focusedColumnis a position intableView.tableColumns, which carries the row-number column and a hidden spacer ahead of the data, so data index N sits at table column N+2. The offset was wrong before the reader touched anything, and reordering a column moved it again.Query > Preview FK Reference therefore previewed a different column than the focused one, or failed its bounds guard and did nothing at all, while
Spaceon the same cell worked: the key-equivalent path inKeyHandlingTableViewalready resolves by column identity throughDataGridView.dataColumnIndex(for:in:schema:). The menu path now uses the same two calls.2. The lookup was invalid SQL on SQL Server
ForeignKeyPreviewViewemittedOFFSET 0 ROWS FETCH NEXT 1 ROWS ONLYwith noORDER BY. T-SQL parses OFFSET/FETCH as part of an ORDER BY clause, so SQL Server rejects the statement with Msg 102. Every other.offsetFetchconsumer readsdialect.offsetFetchOrderByfirst (FilterSQLGenerator.swift:485,TableQueryBuilder), and MSSQL keeps the defaultORDER BY (SELECT NULL).The
catchreplaced the server's message with the generic "Failed to load referenced row", and becausevaluesstayed empty the popover's Open button was disabled too, so the whole popover was inert on every SQL Server connection.The clause now comes from the dialect. The statement moved into
ForeignKeyPreviewQuery, a pure builder, so the clause order can be pinned by a test instead of living inside a SwiftUI view's async method.3. The iOS fork hardcoded
LIMIT 1TableProMobile'sFKPreviewViewbuilt... LIMIT 1directly, which both SQL Server and Oracle reject, and both ship inIOSDriverFactory.supportedTypes.SQLBuilder.paginationClausealready handles those two engines and was simply not called; it is now.The same view swallowed the failure into
row = nil, which rendersContentUnavailableView("No Referenced Row"), indistinguishable from a key with no matching row. A preview that fails now says so and shows the reason, and the "no row" state means only that.What I built and tested
verify.sh buildPASS,verify.sh testoverForeignKeyPreviewQueryTests,FocusedColumnResolutionTests,FKNavigationTests,DataGridColumnPoolTestsandColumnIdentitySchemaTestsPASS at 79/79,verify.sh lint TablePro TableProTests TableProMobile0 violations.xcodebuild -scheme TableProMobile -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' ARCHS=arm64BUILD SUCCEEDED, andSQLBuilderPaginationTests+SQLDialectParityTestspass on the simulator.New tests:
ForeignKeyPreviewQueryTests: the OFFSET/FETCH clause carries its ORDER BY filler, a dialect that declares no filler emits the clause alone, a LIMIT dialect and an absent dialect both getLIMIT 1, the statement ends with the clause, and an already-escaped value is not escaped twice.FocusedColumnResolutionTests: builds a real grid throughDataGridColumnPool, then asserts a data column does not sit one place after its data index, that every column round-trips, that reordering moves a column without changing what it resolves to, and that the row-number column is not a data column. The first of those fails against the old- 1mapping.SQLBuilderPaginationTests(iOS): SQL Server and Oracle each get OFFSET/FETCH behind their own filler ORDER BY and neverLIMIT, a caller's own order replaces the filler instead of stacking with it, the LIMIT engines getLIMIT n OFFSET n, and every typeIOSDriverFactoryships produces a clause its engine can parse.Note
None of these needed #2316's fix and none is needed by it; this is separate work on the same subsystem, opened after that PR merged.