Skip to content

Commit d131feb

Browse files
committed
fix(tables): drop find's selection restore, commit the term on Enter
1 parent b9bd619 commit d131feb

3 files changed

Lines changed: 51 additions & 46 deletions

File tree

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-find.test.tsx

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ function render(overrides: Partial<TableFindProps> = {}) {
5959
onQueryChange: vi.fn(),
6060
onNext: vi.fn(),
6161
onPrev: vi.fn(),
62+
onSubmit: vi.fn(),
6263
onClose: vi.fn(),
64+
isStale: false,
6365
count: 0,
6466
currentIndex: 0,
6567
truncated: false,
@@ -155,6 +157,19 @@ describe('TableFind keyboard', () => {
155157
press('Escape')
156158
expect(props.onClose).toHaveBeenCalledTimes(1)
157159
})
160+
161+
// Mid-debounce the visible matches still belong to the previous term, so
162+
// stepping through them would land on a cell the box no longer describes.
163+
it('commits instead of stepping while the results are stale', () => {
164+
const props = render({ query: 'abcd', count: 3, isStale: true })
165+
press('Enter')
166+
expect(props.onSubmit).toHaveBeenCalledTimes(1)
167+
expect(props.onNext).not.toHaveBeenCalled()
168+
169+
press('Enter', { shiftKey: true })
170+
expect(props.onSubmit).toHaveBeenCalledTimes(2)
171+
expect(props.onPrev).not.toHaveBeenCalled()
172+
})
158173
})
159174

160175
describe('TableFind controls', () => {

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-find.tsx

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ export interface TableFindProps {
1010
onQueryChange: (query: string) => void
1111
onNext: () => void
1212
onPrev: () => void
13+
/** Adopts the typed term immediately, skipping the debounce. */
14+
onSubmit: () => void
1315
onClose: () => void
16+
/** Whether the results on screen still describe an older term. */
17+
isStale: boolean
1418
/** Number of matches after dropping columns not in the current view. */
1519
count: number
1620
/** 0-based index of the active match. Ignored when `count` is 0. */
@@ -31,7 +35,9 @@ export const TableFind = memo(function TableFind({
3135
onQueryChange,
3236
onNext,
3337
onPrev,
38+
onSubmit,
3439
onClose,
40+
isStale,
3541
count,
3642
currentIndex,
3743
truncated,
@@ -41,7 +47,10 @@ export const TableFind = memo(function TableFind({
4147
const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => {
4248
if (e.key === 'Enter') {
4349
e.preventDefault()
44-
if (e.shiftKey) onPrev()
50+
// Committing beats stepping while the matches on screen belong to an
51+
// older term — stepping there navigates results the box no longer shows.
52+
if (isStale) onSubmit()
53+
else if (e.shiftKey) onPrev()
4554
else onNext()
4655
return
4756
}

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx

Lines changed: 26 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -493,11 +493,6 @@ export function TableGrid({
493493
const [pendingMatchTick, setPendingMatchTick] = useState(0)
494494
const findInputRef = useRef<HTMLInputElement>(null)
495495
const pendingMatchRef = useRef<TableFindMatch | null>(null)
496-
/** Cell selected when find was opened, restored on close. */
497-
const preFindAnchorRef = useRef<CellCoord | null>(null)
498-
/** Last cell find itself moved the selection to, so close can tell a match
499-
* cursor apart from a selection the user made while the bar was open. */
500-
const lastRevealedAnchorRef = useRef<CellCoord | null>(null)
501496
/** Monotonic id for the in-flight match jump; see `goToMatch`. */
502497
const goToMatchSeqRef = useRef(0)
503498
/** Term the auto-reveal has already run for, so a background refetch of the
@@ -1132,6 +1127,21 @@ export function TableGrid({
11321127
return () => clearTimeout(timer)
11331128
}, [findOpen, trimmedFindQuery])
11341129

1130+
const trimmedFindQueryRef = useRef(trimmedFindQuery)
1131+
trimmedFindQueryRef.current = trimmedFindQuery
1132+
1133+
/**
1134+
* Adopt the typed term now instead of waiting out the debounce. Enter uses
1135+
* this while the two disagree: navigating there would step through the
1136+
* PREVIOUS term's matches — `keepPreviousData` still holds them — and land on
1137+
* a cell that doesn't match the box. Pressing Enter means "search this now",
1138+
* so it commits rather than navigates, and the auto-reveal takes it from
1139+
* there. The pending timer is harmless: it later sets the same string.
1140+
*/
1141+
const handleFindSubmit = useCallback(() => {
1142+
setSubmittedQuery(trimmedFindQueryRef.current)
1143+
}, [])
1144+
11351145
const {
11361146
data: findData,
11371147
isFetching: isFindFetching,
@@ -1260,7 +1270,6 @@ export function TableGrid({
12601270
setIsColumnSelection(false)
12611271
setRowSelection((prev) => (prev.kind === 'none' ? prev : ROW_SELECTION_NONE))
12621272
setSelectionFocus(null)
1263-
lastRevealedAnchorRef.current = { rowIndex, colIndex }
12641273
cursorIsOnMatchRef.current = true
12651274
setSelectionAnchor({ rowIndex, colIndex })
12661275
}, [rows, displayColumns, pendingMatchTick])
@@ -1328,11 +1337,15 @@ export function TableGrid({
13281337
* Closes the bar and leaves no trace of the search: the term, the highlights
13291338
* (via the emptied term), and the match cursor all go.
13301339
*
1331-
* The cell the user was on before opening find is restored, so an abandoned
1332-
* search does not relocate them — Sheets parks the cursor on the last match
1333-
* instead, which is a standing complaint there. Restoring is skipped once the
1334-
* user has selected a cell themselves: at that point the selection is their
1335-
* own work, not find's, and yanking it back would lose their place.
1340+
* The cell selection is deliberately left where it is. Restoring the cell the
1341+
* user was on before opening find reads nicely, but deciding whether the
1342+
* current selection belongs to find or to the user is not answerable here —
1343+
* the grid has ~15 places that move the selection and no notion of who owns
1344+
* it, so every heuristic (compare the anchor, also check the focus, clear on
1345+
* click, clear on keydown) mis-fires on some ordinary gesture: extending a
1346+
* range from a match, clicking the match cell itself, arrowing away and back,
1347+
* Cmd+Z, or Cmd+F to refocus the bar. Leaving the cursor on the last match is
1348+
* what Sheets does and what this grid already did before find was reworked.
13361349
*/
13371350
const handleFindClose = useCallback(() => {
13381351
setFindOpen(false)
@@ -1345,25 +1358,6 @@ export function TableGrid({
13451358
autoRevealedTermRef.current = ''
13461359
cursorIsOnMatchRef.current = false
13471360
setIsJumping(false)
1348-
const origin = preFindAnchorRef.current
1349-
const lastRevealed = lastRevealedAnchorRef.current
1350-
preFindAnchorRef.current = null
1351-
lastRevealedAnchorRef.current = null
1352-
const anchor = selectionAnchorRef.current
1353-
// A revealed match is a single cell: find sets the anchor and clears the
1354-
// focus. A non-null focus means the user extended a range from it
1355-
// (Shift+Arrow, Shift+click, drag), which makes the selection theirs even
1356-
// though the anchor still sits on the match — restoring would delete it.
1357-
const stillOnMatch =
1358-
lastRevealed !== null &&
1359-
anchor !== null &&
1360-
selectionFocusRef.current === null &&
1361-
anchor.rowIndex === lastRevealed.rowIndex &&
1362-
anchor.colIndex === lastRevealed.colIndex
1363-
if (stillOnMatch) {
1364-
setSelectionFocus(null)
1365-
setSelectionAnchor(origin)
1366-
}
13671361
scrollRef.current?.focus({ preventScroll: true })
13681362
}, [])
13691363

@@ -1679,10 +1673,6 @@ export function TableGrid({
16791673
setRowSelection((prev) => (prev.kind === 'none' ? prev : ROW_SELECTION_NONE))
16801674
setIsColumnSelection(false)
16811675
lastCheckboxRowRef.current = null
1682-
// Any deliberate click hands the selection back to the user, so closing
1683-
// find must not restore over it — including a click on the very cell find
1684-
// had revealed, which leaves the anchor and focus looking find-owned.
1685-
lastRevealedAnchorRef.current = null
16861676
if (shiftKey && selectionAnchorRef.current) {
16871677
setSelectionFocus({ rowIndex, colIndex })
16881678
} else {
@@ -2644,13 +2634,6 @@ export function TableGrid({
26442634
const tag = (e.target as HTMLElement).tagName
26452635
if (tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT') return
26462636

2647-
// Any key that reaches the GRID while find is open is the user driving
2648-
// the grid — the find input swallows its own keys via the guard above —
2649-
// so the selection is theirs from here on and close must not restore over
2650-
// it. Escape is excluded: it IS the close, and must still restore.
2651-
// One choke point rather than a hook at each of the ~15 anchor writers.
2652-
if (e.key !== 'Escape') lastRevealedAnchorRef.current = null
2653-
26542637
if ((e.metaKey || e.ctrlKey) && (e.key === 'z' || e.key === 'y')) {
26552638
e.preventDefault()
26562639
if (e.key === 'y' || e.shiftKey) {
@@ -3555,10 +3538,6 @@ export function TableGrid({
35553538
if (!(e.metaKey || e.ctrlKey) || e.key !== 'f') return
35563539
if (!containerRef.current) return
35573540
e.preventDefault()
3558-
// Remember where the user was, but only on the transition into find —
3559-
// Cmd+F pressed again while the bar is open (to refocus it) must not
3560-
// overwrite the origin cell with the match they are currently on.
3561-
if (!findOpenRef.current) preFindAnchorRef.current = selectionAnchorRef.current
35623541
setFindOpen(true)
35633542
requestAnimationFrame(() => {
35643543
findInputRef.current?.focus()
@@ -4419,7 +4398,9 @@ export function TableGrid({
44194398
onQueryChange={setFindQuery}
44204399
onNext={handleFindNext}
44214400
onPrev={handleFindPrev}
4401+
onSubmit={handleFindSubmit}
44224402
onClose={handleFindClose}
4403+
isStale={trimmedFindQuery !== submittedQuery}
44234404
count={findMatches.length}
44244405
// Clamped, not stored: a background refetch of the same term can
44254406
// shrink the match set under a cursor the user already paged, and

0 commit comments

Comments
 (0)