feat(datagrid): find in results, with a match counter that names its scope - #2210
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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.
Root cause
This was a mis-wiring, not a missing feature.
MainSplitViewController+EditMenuActions.swift:65routedCmd+Fon a table tab totoggleFilterPanel(). Because that is a toggle, pressingCmd+Fwith the filter panel already open closed it, and nothing anywhere searched the rows on screen. MeanwhilefindNext/findPreviouswent straight toEditorEventRouterregardless of surface.Cmd+Fwas also a redundant third path to the filter panel, which already hasCmd+Option+Fand a funnel button in the status bar. So reassigning it removes nothing from the user. That matches Postico 2, which leavesCmd+Ffree and usesOpt+Cmd+Ffor its filter bar.The honesty problem, and why the counter is worded the way it is
Results are paginated at the database, default page size 1000. A find that scans only the loaded page will confidently report "No results" for a row sitting on page 7 of a 4M-row table. The old
Cmd+Fwas clunky but correct, so replacing it with something fast and wrong would have been the worst possible trade for an app whose pitch is that it is safe to point at production.Three competitors settled the design:
So the counter always names its scope:
3 of 12 on this pagewhile rows remain unfetched,3 of 12once everything is loaded,Not on this pageinstead of a bareNo matches. The scope is keyed offhasMoreRowsrather than comparing row counts, which is what makes it correct whentotalRowCountis nil.Design decisions worth reviewing
Not NSTextFinder. Its client contract needs
contentViewAtIndex:effectiveCharacterRange:andrectsForCharacterRange:, and a view-basedNSTableViewhas no honest answer: cell views are recycled and off-screen rows have no view at all. It also only ever searches the string the client hands it, so it can never cross the page boundary.Not the NSScrollView find-bar slot either.
NSTextFinder.hsaysfindBarView"is managed by NSTextFinder. You should not set this property", and we are not using NSTextFinder. The bar goes where this codebase already puts one: the SwiftUI VStack above the grid, next toFilterPanelView. That also gives per-tab ownership for free, sinceDataGridViewis anNSViewRepresentablebuilt per tab.Whole-cell tint, not substring highlighting.
DataGridCellView.cachedCTLine()caps at 300 NSString units anddrawTextswaps inCTLineCreateTruncatedLineat the column width, so a rect derived from adisplayTextindex is wrong past either. The tint usesNSColor.findHighlightColor, the system colour documented as "Background color of find indicators", so it adapts to light and dark without widening the theme schema for one value. It deliberately takes precedence overmodifiedColumnTintand ignoresonEmphasizedSelection, because jumping to a match selects the row and the highlight has to survive that.Search All Rows only appears when no filters are applied.
TabFilterState.filterLogicModeis one mode for the whole filter array, so escalating to a cross-column OR search would silently loosen AND filters the user wrote. Offering the button on top of existing filters would either discard their work or lie about what it searched, so it is hidden and the docs say to use the filter panel instead. This is a deliberate limit, not an oversight.Escape composes with the existing two-step.
NativeSearchFieldconsumes Escape when the field has text and clears it, and returns false when empty. So the first Escape clears the term and the second closes the bar, matching #1490.Scope
Every match resolves through
displayRow(at:in:), never by indexingTableRows.rowswith a display position, per the CLAUDE.md invariant that shipped as #1837. Binary and spatial columns are excluded from matching since they render as hex. Find state is a newTabFindStateonQueryTabbesidefilterState, session-only, and is not written toPersistedTab.Cmd+G/Cmd+Shift+Gnow drive the grid when its find bar is open, gated throughvalidateMenuItem(_:)via a newhasActiveGridFind, so they stay dimmed rather than silently acting on the SQL editor.Verification
buildPASStestPASS, 46 executed / 46 passed, covering the three new suites plusDisplayRowMappingTests,TableViewCoordinatorValueFilterTests,DataGridCellViewDoubleClickTestsandDataGridCellAccessoryAppearanceTestslint TablePro0 violationsNo screenshots.
scripts/export-screenshots.shdoes not exist in the repo, so the docs page for this feature ships without the<Frame>pair the other feature pages have. That needs a follow-up capture.No UI automation. The find bar is reachable only with a live connection and loaded rows, which
TableProUITestscannot set up deterministically today.Found while investigating, not fixed here
PaginationCoordinator.showAllRows()(line 51) readstab.pagination.totalRowCountand never consultsisApproximateRowCount, so on a MySQL InnoDB table whoseinformation_schemaestimate reads 640,000 against a real 1,000,000 rows, "All rows" emitsLIMIT 640000and the grid holds a subset while the UI says everything is loaded. A verifier tried to refute this and could not. It is not bundled here because this feature's escalation goes throughFilterCoordinator, not throughshowAllRows(), so the find bar does not inherit it. Worth its own PR.Code review pass
A high-effort review found 12 findings and all 12 are addressed in this branch. The first one is worth calling out because it broke the exact property this feature exists to provide:
scope(for:)originally readpagination.hasMoreRows. That flag is the query-tab truncation state and is never set for a table tab:syncLoadMoreStateopens withguard tabType == .query else { return }, and the field's own comment says "Result truncation state (query tabs)". Table tabs page throughcurrentPage/totalPages. Since the find bar only renders on table tabs, every counter would have read "3 of 12" on page 1 of 12, claiming the page was the whole table, and "Search All Rows" would have been unreachable dead code. It now goes throughcanGoToNextPage(loadedRowCount:), which also handles the case where the row count is unknown, andFindScopeFromPaginationTestscovers all five states so it cannot regress silently.The rest:
rowView.needsDisplayredraws only the row background, never the cellsreloadData(forRowIndexes:columnIndexes:)invalidateMatches()had no callers, so matches survived a page changeonChangeLIKEagainst integer and timestamp columns, which errors the whole page on PostgresisServerSearchable, text and enum and set onlycachedLineinvalidated with itfocusOnAppearvisibleColumnDataIndices()scrollColumnToVisibleShift+Returndocumented but never implementedfilterLogicMode = .orwritten before the discard prompt was answered, so cancelling left the tab in ORapplyFiltersand written inside the confirmed branch.keyboardShortcut(.escape)pre-empted the field's own cancel handlingNativeSearchFieldkeeps the two-step Escape from #1490