refactor(editor): refactor search panel#759
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: a7fb9bfe13
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR refactors the editor search panel API and behavior, and adds in-editor match highlighting for search results (via overlay selection blocks styled with theme-derived colors).
Changes:
- Simplifies
TextDocument.search()/PieceTable.search()to a single-argument API returning all matches for givenSearchParams. - Refactors
SearchPanelWidgetto manage match navigation locally and integrate with the editor viascrollToMatch+onUpdate. - Adds match highlight rendering and theme/CSS support for find match vs. find match highlight.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/diffs/test/editorPieceTable.test.ts | Updates tests to match the new PieceTable.search(searchParams) API. |
| packages/diffs/src/editor/tokenzier.ts | Exposes theme colors as CSS vars for find match highlighting. |
| packages/diffs/src/editor/textDocument.ts | Simplifies search() wrapper to match new piece table API. |
| packages/diffs/src/editor/searchPanel.ts | Refactors search panel state, navigation, and editor integration callbacks. |
| packages/diffs/src/editor/pieceTable.ts | Simplifies search() to return all matches (removes kind/range logic). |
| packages/diffs/src/editor/editor.ts | Integrates search panel updates, scroll-to-match, and renders match highlights in the overlay. |
| packages/diffs/src/editor/css.ts | Adds styling for match highlights and updates overlay/selection-related rules. |
Comments suppressed due to low confidence (1)
packages/diffs/src/editor/editor.ts:789
#retainSearchPanelFocusis never reset after focusing the search panel. Once it becomestrue, subsequent renders will keep re-focusing the search input and also suppress focusing the editor (!this.#retainSearchPanelFocuscheck above), which can make it hard/impossible to interact with the editor while the panel is open.
if (this.#retainSearchPanelFocus) {
requestAnimationFrame(() => {
this.#searchPanel?.focus();
});
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
amadeus
left a comment
amadeus
left a comment
There was a problem hiding this comment.
My review here is mostly high level, definitely requires a lot of additional context that I don't have around how the editor works. But in general looks good.
Most of my notes are around future stuff.
Also i guess side question, does editor work with CodeView? Reason I ask is because there's potentially some integration stuff we might need to talk about
| @@ -7,9 +7,12 @@ export const editorCSS: string = /* CSS */ ` | |||
| 50% { opacity: 0; } | |||
| 100% { opacity: 1; } | |||
| } | |||
There was a problem hiding this comment.
Not for this PR, but I would love to get this file moved into a normal css file so we can run it through our various linters and stuff. I think it would also be nice to figure out a way to figure out how to make it a re-usable stylesheet like we do here:
https://github.com/pierrecomputer/pierre/blob/main/packages/diffs/src/components/web-components.ts
| this.#scrollingToLine = undefined; | ||
| this.#scrollingToLineChar = undefined; | ||
| } else if (this.#selections !== undefined && this.#selections.length > 0) { | ||
| this.#scrollingToLineNoFocus = false; |
There was a problem hiding this comment.
How will this scrollToLine stuff interface with CodeView implementations?
There was a problem hiding this comment.
I think they are totally different. The scrollToLine of editor is used in a diff/file component when editable, CodeView scroll api works multiple diff/file components
| <circle cx="7.5" cy="14" r="2.5" fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></circle> | ||
| <line x1="5" y1="14" x2="3" y2="14" fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></line> | ||
| </svg> | ||
| `, |
There was a problem hiding this comment.
We should make this icon part of our SVG sprite sheet, cc @mdo
There was a problem hiding this comment.
i didn't put these svg icons in the sprite to keep the diffs index module smaller. the user may not need the editing feature. maybe an standalone editor svg sprite?
SearchPanelWidgetclass