Find in page (⌘F) - #480
Conversation
Chrome-style find bar: live debounced search with a match counter (n/m), Enter/Shift+Enter and chevron buttons for next/prev, Esc closes. The bar takes a DOM strip at the top of the content column so the native view's mirrored bounds shrink below it — no OS-view overlap. Closes (and clears highlights) on tab switch. Main side: findInPage/ stopFindInPage IPC resolving through the tab's webContents with the found-in-page result.
The find-bar close callback referenced activeTabId in its useCallback deps before the const below it was initialized, so mounting the Browser route threw a temporal-dead-zone ReferenceError and the whole route failed to render. Moved below the declaration.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ebe025d0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| // Skip the round-trip for an identical query (arrow-key re-renders). | ||
| const key = `${q}|${findNext}|${forward}`; | ||
| if (key === lastSentRef.current) return; |
There was a problem hiding this comment.
When the user presses Enter or clicks Next more than once for the same query, the first navigation stores a key like term|true|true, and every subsequent identical next action returns here before sending another findInPage request. That leaves the active match stuck after one advance instead of continuing through the page; the de-dupe needs to skip only passive re-renders/live-search repeats, not explicit findNext actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — explicit Enter/Next always sends now; only passive repeats are de-duped (dc3b978).
| if (activeTabId) host.browserStopFind?.(activeTabId); | ||
| }, [activeTabId]); | ||
| // Switching tabs closes the bar (and clears the old page's highlights). | ||
| useEffect(() => { if (findOpen) closeFind(); }, [activeTabId]); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
When switching tabs while the find bar is open, this effect runs after activeTabId has already changed, so closeFind() sends browserStopFind to the destination tab rather than the tab that currently has find highlights. The source tab's WebContents keeps its highlighted matches, and they reappear when the user switches back even though the find bar was closed; keep the previous tab id or clear before activating the new tab.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the tab-switch effect stops find on the tab being left (tracked via a ref), so the old page's highlights are actually cleared (dc3b978).
Two valid Codex findings on #480: - The de-dupe key swallowed explicit repeated Enter/Next actions, so the active match advanced once and then appeared stuck. Explicit findNext actions now always send; only passive repeats are skipped. - Switching tabs closed the bar AFTER activeTabId changed, sending stopFind to the destination tab and leaving the old page's matches highlighted. The switch effect now stops find on the tab being left.
1 participant
What
⌘F opens a Chrome-style find bar over the current tab: live match count, Enter / Shift+Enter to step through matches, Esc to close.
The bar is a DOM strip above the native WebContentsView, so the page's bounds simply shrink below it — no fighting an OS-level view for z-order. Find state resets when you switch tabs (we keep it simple rather than mirroring Chrome's per-tab find memory).
Also carries a one-commit fix for a crash found in testing:
closeFindwas declared before theactiveTabIdit closes over, so mounting the route hit a temporal-dead-zone error. It's now declared after.Stacked on
pr/agent-apps.npm run typecheck+ tests green.For QA