fix(ios): insert-row defaults and the missing table list search field - #2548
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. |
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.
Fixes #2543
Fixes #2544
Two iOS defects that turned out to share a shape: a three-valued choice modelled as a two-valued one.
#2543 Insert Row cannot leave a column on its database default
Root cause
InsertRowViewheld a per-column state that is really three-valued (use the database default, SQLNULL, a typed literal) as two parallel arrays,values: [String]andisNullFlags: [Bool]. Two booleans cannot express three states, so "untouched" and "empty string" collapsed into one andbuildInsertSQLappended every column to theINSERT. The only omission path was a heuristic on primary keys.ColumnInfo.isNullablewas never read, so the NULL badge was offered onNOT NULLcolumns where it can only fail.Measured against MySQL 8.4 with the default
sql_mode, on the reporter's table shape:INSERT ... VALUES ('', ...)ERROR 1366 Incorrect integer value: ''INSERT (created_at, ...) VALUES (NULL, ...)ERROR 1048 (23000) Column 'created_at' cannot be nullINSERT (amount) VALUES ('5'), omitting the restcreated_at= now,name= its defaultINSERT (amount, gen) VALUES ('5','10')ERROR 3105, a generated column rejects any valueThe second row is the issue's error text verbatim.
The fix
The three-valued model already existed next door.
PayloadValue(.null/.text, with an absent key meaning "leave it out") andRowInsertPlannerare what the App Intents path has always used, which is why a Shortcut could already leave a column on its default while the UI could not.InsertRowViewnow holds one[String: PayloadValue]keyed by column name, and both paths share the planner.PayloadRowandRowInsertPlannermoved fromIntents/toHelpers/now that two features depend on them.SQLStatementGenerator, which excludes only generated columns, and keeps the shippedincludesProvidedPrimaryKeycontract for the Add Rows Shortcut.INSERT INTO t () VALUES ()on MySQL and MariaDB,INSERT INTO t DEFAULT VALUESon PostgreSQL, Redshift, SQLite, SQL Server and DuckDB, and nothing on Oracle, which accepts neither. All four forms were measured, not read off a doc page.allowAllDefaultsanddropsEmptyPrimaryKeyboth default to what a payload has always meant, where absence is the only way to say "leave this out". The form turns both off, because it distinguishes DEFAULT from an explicitly empty value: picking Empty String on a text primary key now writes''rather than silently dropping the column while the badge readsVALUE.ColumnInfogainedisAutoIncrementandisGenerated, populated from metadata the drivers already fetch: MySQL'sExtracolumn (index 6 ofSHOW FULL COLUMNS, previously read past), SQL Server'sIsIdentity(already parsed intoMSSQLColumnRowand dropped on the floor), and PostgreSQL'sis_identity/is_generated. SQLite reports a loneINTEGER PRIMARY KEYas the rowid alias, and itsPRAGMA table_infoalready hides generated columns. Oracle and DuckDB keep the defaults rather than change a schema query the macOS app also links; they lose nothing, because a column left on DEFAULT is omitted regardless.ColumnInfolives inTableProModels, which only the iOS app and TableProCore's own targets import. The macOS app has its own separateColumnInfo, and the plugin boundary type isPluginColumnInfo, which already carries both fields. This is not a PluginKit ABI change and needs no version bump.MySQLDEFAULT_GENERATEDcontains the substringGENERATEDbut is a default, not a generated column. There is a test for that specifically.#2544 The table list search field never appears
Root cause
Two structural facts, both measured on an iOS 27.0 simulator with a probe app built for the iOS 18 deployment target:
.searchableapplied inside aTabreaches no navigation bar, because theTabViewis theNavigationStack's root and the tab content has no navigation container of its own. A.navigationTitleinside the tab is dropped identically. Moving.searchableonto theTabViewrenders nothing either.TabViewwhose tabs each own aNavigationStackrenders no navigation bar at all when thatTabViewis aNavigationSplitViewdetail. Verified with one binary and one variable: the identical connection screen renders its title and search field as a root, and neither when wrapped inNavigationSplitView { sidebar } detail: { it }. Broken on iPad too, where the split view does not collapse.So both suggestions in the issue fail.
Tab(role: .search)renders the iOS 26 search affordance but its own.searchablestill does not surface, and it is the wrong control anyway: a search-role tab owns search for the whole tab view rather than filtering one list. Inverting the nesting alone is defeated by (2).Nine structures were measured in total.
.searchable(placement: .navigationBarDrawer(displayMode: .always))does force a field into the current nesting, but displaces the tab bar;.searchToolbarBehavior(.minimize)and.searchPresentationToolbarBehavior(.avoidHidingContent)restore the tab bar and suppress the field, and both are iOS 26 only.The fix
The connection screen becomes a root:
ConnectionListViewpresents it in a.fullScreenCoverinstead of aNavigationSplitViewdetail, andConnectedViewputs theTabViewoutermost with oneNavigationStackper tab..tabViewStyle(.sidebarAdaptable)(iOS 18) gives iPad a sidebar for the four sections instead of a phone tab bar.TableListViewkeeps its.searchableexactly as written; it then renders.coordinator.tablesPathand theDataBrowserViewdestination moved into the Tables tab's stack, and the shared toolbar is applied per tab.NavigationSplitViewsupplied the back affordance for free, soConnectedViewgained an explicit Connections button. That is the visible cost of the change, along with the iPad connection list no longer sitting beside the connection.The two search fields also had one
@SceneStoragekey each, shared by every connection and every table, so a filter typed in one connection silently hid rows in another with no field on screen to reveal it. The table list filters client-side and applies immediately, so its text is now keyed per connection and still restored. The data browser searches on the server and applies on submit, so restoring its text would name a filter the rows on screen were never fetched under; it is plain@Stateand no longer persisted.Also fixed in the same files
columnDetails.countininit, and the + button was not gated on the column list having loaded. Opening the sheet early left the arrays empty while the form rendered every section, so every keystroke was discarded by the bounds guard and every column inserted as''. Keying by column name removes the failure mode; the button is gated as well.isPrimaryKey && typeName.contains("INT")pre-nulled every integer primary key, and the insert then dropped it. A compositePRIMARY KEY (order_id, line_no)over twoINT NOT NULLcolumns rendered both as the word NULL, labelled both auto-increment, and omitted both. The heuristic is gone, replaced by real metadata.NULLonNOT NULLcolumns too, the same defect on the sibling write path. It needed no driver work:ColumnInfo.isNullablewas already there.Verification
xcodebuild build, iPhone 17 Pro Max, iOS 26/27 simulator: BUILD SUCCEEDED.xcodebuild test -only-testing:TableProMobileTests: 250 passed, 0 failed.swiftlint: 0 violations acrossTableProMobileandPackages/TableProCore/Sources/TableProModels.sqlite3, and theduckdbCLI, not read off documentation.INSERT INTO t () VALUES ()is a syntax error on PostgreSQL, SQLite and DuckDB;DEFAULT VALUESis a syntax error on MySQL. Both forms are covered by tests.New tests:
ColumnMetadataRulesTests(includingDEFAULT_GENERATED, the composite-key rank, andWITHOUT ROWID),SQLBuilderDefaultValuesTests, nine cases added toRowInsertPlannerTestscovering omission, generated columns, an explicit auto-increment value, an explicitly empty primary key, and each dialect's all-defaults form, plusRowDetailViewModelTests.isNullableFollowsMetadata.Review
Two reviewers read this diff. A
security-reviewpass found nothing: the new SQL surfaces reuse the escaping the surrounding driver code already applies, and the value flow throughescapeStringLiteralis unchanged. Acode-reviewpass at high effort filed six findings; five were fixed here, and they are the reason the diff looks the way it does:canInsertRow. Only the toolbar button had been gated, so the empty-table placeholder still offered the sheet on Redis and before the column list arrived.dropsEmptyPrimaryKey, above.CancellationErrorrather than swallowing it and issuing a second query.The sixth is real and deliberately not fixed here:
MSSQLColumnRowcarries no computed-column flag, soisGeneratedstays false on SQL Server and a computed column is still offered as an editable field. Adding it means changingMSSQLSchemaQueriesin a package the macOS app also links, which is the blast radius this PR set out to avoid. It joins Oracle and DuckDB in the gap list below.An earlier Codex review was started and died before producing a verdict, so it is not counted among the reviewers. Its partial reasoning did surface three leads that were verified by measurement and fixed: the SQLite
pkrank,WITHOUT ROWID, and the Redshift query break.What could not be verified here
No before/after screenshots of the connected screens. The Xcode on this machine ships no
Simulator.app,simctlhas no touch-injection command, andsimctl openurlraises a SpringBoard confirmation that cannot be dismissed without a pointer. The connection list was captured; the Tables tab and the Insert Row sheet were not. The structure they depend on was measured directly instead, with a probe app built at the iOS 18 deployment target and screenshotted on both an iPhone and an iPad running iOS 27, in the exact shape this PR ships.No UI automation.
TableProMobilehas no UI test target at all (TableProMobile/project.ymldeclares onlyTableProMobileTests), so neither fix could get XCUITest coverage without adding one. Both flows also need a live database, which does not run deterministically on CI.Oracle, DuckDB and SQL Server do not report generated columns, and Oracle and DuckDB do not report identity either. Their metadata queries live in packages the macOS app also links. A column left on DEFAULT is omitted on every engine regardless, so the reported bug is fixed everywhere; what these three lose is the extra protection of refusing to write such a column when a value is typed into it.
SQLite composite primary keys were mis-read app-wide before this branch:
PRAGMA table_inforeportspkas a 1-based rank, and the driver tested it for== "1", so only the first column of a composite key was treated as a key at all. Fixed here because the auto-increment rule depends on the count.