Skip to content

feat(datagrid): find in results, with a match counter that names its scope - #2210

Merged
datlechin merged 2 commits into
mainfrom
feat/find-in-results
Aug 19, 2026
Merged

feat(datagrid): find in results, with a match counter that names its scope#2210
datlechin merged 2 commits into
mainfrom
feat/find-in-results

Conversation

@datlechin

Copy link
Copy Markdown
Member

Root cause

This was a mis-wiring, not a missing feature. MainSplitViewController+EditMenuActions.swift:65 routed Cmd+F on a table tab to toggleFilterPanel(). Because that is a toggle, pressing Cmd+F with the filter panel already open closed it, and nothing anywhere searched the rows on screen. Meanwhile findNext/findPrevious went straight to EditorEventRouter regardless of surface.

Cmd+F was also a redundant third path to the filter panel, which already has Cmd+Option+F and a funnel button in the status bar. So reassigning it removes nothing from the user. That matches Postico 2, which leaves Cmd+F free and uses Opt+Cmd+F for 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+F was 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:

  • TablePlus refused an in-grid find outright for exactly this reason: "it could cause confusion with the advanced filter - which searches all the table, not only the current page." (Allow searching the data grid like a browser or text editor. TablePlus/TablePlus#2279)
  • DataGrip puts the scope in the command name: "Find on Current Page".
  • DBeaver ships one with no counter at all, and only warns in its docs that "Search only checks rows that are already fetched".

So the counter always names its scope: 3 of 12 on this page while rows remain unfetched, 3 of 12 once everything is loaded, Not on this page instead of a bare No matches. The scope is keyed off hasMoreRows rather than comparing row counts, which is what makes it correct when totalRowCount is nil.

Design decisions worth reviewing

Not NSTextFinder. Its client contract needs contentViewAtIndex:effectiveCharacterRange: and rectsForCharacterRange:, and a view-based NSTableView has 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.h says findBarView "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 to FilterPanelView. That also gives per-tab ownership for free, since DataGridView is an NSViewRepresentable built per tab.

Whole-cell tint, not substring highlighting. DataGridCellView.cachedCTLine() caps at 300 NSString units and drawText swaps in CTLineCreateTruncatedLine at the column width, so a rect derived from a displayText index is wrong past either. The tint uses NSColor.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 over modifiedColumnTint and ignores onEmphasizedSelection, 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.filterLogicMode is 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. NativeSearchField consumes 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 indexing TableRows.rows with 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 new TabFindState on QueryTab beside filterState, session-only, and is not written to PersistedTab.

Cmd+G / Cmd+Shift+G now drive the grid when its find bar is open, gated through validateMenuItem(_:) via a new hasActiveGridFind, so they stay dimmed rather than silently acting on the SQL editor.

Verification

  • build PASS
  • test PASS, 46 executed / 46 passed, covering the three new suites plus DisplayRowMappingTests, TableViewCoordinatorValueFilterTests, DataGridCellViewDoubleClickTests and DataGridCellAccessoryAppearanceTests
  • lint TablePro 0 violations

No screenshots. scripts/export-screenshots.sh does 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 TableProUITests cannot set up deterministically today.

Found while investigating, not fixed here

PaginationCoordinator.showAllRows() (line 51) reads tab.pagination.totalRowCount and never consults isApproximateRowCount, so on a MySQL InnoDB table whose information_schema estimate reads 640,000 against a real 1,000,000 rows, "All rows" emits LIMIT 640000 and 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 through FilterCoordinator, not through showAllRows(), 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 read pagination.hasMoreRows. That flag is the query-tab truncation state and is never set for a table tab: syncLoadMoreState opens with guard tabType == .query else { return }, and the field's own comment says "Result truncation state (query tabs)". Table tabs page through currentPage/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 through canGoToNextPage(loadedRowCount:), which also handles the case where the row count is unknown, and FindScopeFromPaginationTests covers all five states so it cannot regress silently.

The rest:

Finding Fix
rowView.needsDisplay redraws only the row background, never the cells reloadData(forRowIndexes:columnIndexes:)
invalidateMatches() had no callers, so matches survived a page change re-run keyed on a rows revision through onChange
Escalation built LIKE against integer and timestamp columns, which errors the whole page on Postgres new isServerSearchable, text and enum and set only
White text on the yellow highlight, because jumping selects the row black text when the match tint is set, and cachedLine invalidated with it
Search field never took focus focusOnAppear
Hidden columns were searched and counted but had nothing to show filtered through visibleColumnDataIndices()
No horizontal scroll to an off-screen match scrollColumnToVisible
Shift+Return documented but never implemented claim removed from docs and CHANGELOG
filterLogicMode = .or written before the discard prompt was answered, so cancelling left the tab in OR mode now passed into applyFilters and written inside the confirmed branch
A full synchronous scan on every keystroke 120ms coalesce, cancelled on dismiss
.keyboardShortcut(.escape) pre-empted the field's own cancel handling removed; NativeSearchField keeps the two-step Escape from #1490

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@mintlify

mintlify Bot commented Aug 18, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 18, 2026, 7:57 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@datlechin
datlechin merged commit a612e99 into main Aug 19, 2026
9 checks passed
@datlechin
datlechin deleted the feat/find-in-results branch August 19, 2026 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant