feat: implement find and replace functionality#7
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
editor | b198652 | Commit Preview URL Branch Preview URL |
Mar 22 2026, 10:10 PM |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds find-and-replace: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Toolbar
participant App
participant Modal as FindReplaceModal
participant Matches as getMatches
participant Editor
User->>Toolbar: click search
Toolbar->>App: onSearchClick()
App->>App: set isSearchOpen = true
App->>Modal: render (isOpen = true)
User->>Modal: type search term
Modal->>App: onSearchTermChange(term)
App->>Matches: getMatches(content, term, caseSensitive)
Matches-->>App: indices[]
App->>Editor: update props (searchTerm, currentMatchIndex, isSearchOpen)
Editor->>Editor: highlight matches & manage caret
User->>Modal: click "Replace" / "Replace All"
Modal->>App: onReplace(repl) / onReplaceAll(repl)
App->>App: update content (replace at indices)
App->>Editor: render updated content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/components/FindReplaceModal.tsx (2)
5-16: Remove unusedcontentprop.The
contentprop is defined inFindReplaceModalProps(line 8) but is never used within the component. This creates unnecessary coupling.♻️ Remove unused prop
interface FindReplaceModalProps { isOpen: boolean; onClose: () => void; - content: string; searchTerm: string; onSearchTermChange: (term: string) => void;Also update the call site in
App.tsxto removecontent={content()}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FindReplaceModal.tsx` around lines 5 - 16, The FindReplaceModalProps interface declares an unused prop named content; remove the content field from the interface and any corresponding prop usage (there should be none inside FindReplaceModal component) and update the component call site (e.g., in App.tsx) to stop passing content={content()} so the prop signature and callers remain consistent; reference the FindReplaceModalProps type and the FindReplaceModal component as the locations to change.
57-76: Add Escape key handler to close the modal.Users typically expect pressing Escape to close modal dialogs. Consider adding keyboard support for better UX.
⌨️ Add Escape handler
+ import { createSignal, type JSX, Show, onMount, onCleanup } from "solid-js"; const FindReplaceModal = (props: FindReplaceModalProps): JSX.Element => { const [replaceText, setReplaceText] = createSignal(""); + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape" && props.isOpen) { + handleClose(); + } + }; + + onMount(() => document.addEventListener("keydown", handleKeyDown)); + onCleanup(() => document.removeEventListener("keydown", handleKeyDown)); const handleFindKeyDown = (e: KeyboardEvent) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FindReplaceModal.tsx` around lines 57 - 76, Add Escape key handling so pressing Escape closes the modal: update the keyboard handlers (handleFindKeyDown and handleReplaceKeyDown) to detect e.key === "Escape" (or "Esc" for cross-browser) and call the existing handleClose (which clears replace text and calls props.onClose()). Alternatively attach a single keydown listener that calls handleClose on Escape; ensure you preventDefault if needed and clean up any global listener on unmount.tests/get-matches.lib.test.ts (1)
4-7: Expand test coverage for edge cases.The current test only covers the basic happy path. Consider adding tests for:
- Empty search term (should return
[])- No matches found
- Multiple matches
- Overlapping matches (e.g., "aaa" in "aaaa" should return
[0, 1, 2]based on current implementation)- Case-insensitive matching
🧪 Additional test cases
it("should return empty array for empty search term", () => { expect(getMatches("hello world", "")).toEqual([]); }); it("should return empty array when no matches", () => { expect(getMatches("hello world", "foo")).toEqual([]); }); it("should find multiple matches", () => { expect(getMatches("hello hello", "hello")).toEqual([0, 6]); }); it("should find overlapping matches", () => { expect(getMatches("aaaa", "aa")).toEqual([0, 1, 2]); }); it("should be case insensitive", () => { expect(getMatches("Hello HELLO", "hello")).toEqual([0, 6]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/get-matches.lib.test.ts` around lines 4 - 7, Add comprehensive unit tests for getMatches to cover edge cases beyond the happy path: add tests asserting that an empty search term returns [], a search with no matches returns [], multiple non-overlapping matches (e.g., "hello hello" vs "hello") returns [0,6], overlapping matches (e.g., "aaaa" vs "aa") returns [0,1,2], and case-insensitive matching (e.g., "Hello HELLO" vs "hello") returns [0,6]; update tests in tests/get-matches.lib.test.ts using the existing describe("getMatches", ...) block and the getMatches function name so CI validates these behaviors.src/components/Editor.tsx (1)
90-100: DuplicategetMatchesimplementation with incompatible return type.There's a local
getMatchesfunction (lines 132-145) returningMatch[]with{start, end}objects, whilesrc/lib/get-matches.tsexports a function returningnumber[]. ThecreateEffectat line 92 uses the local version since there's no import, but this creates maintenance burden and inconsistency.Consider importing and adapting the shared utility, or extracting this version to the shared module if the
Matchinterface is needed elsewhere.♻️ Suggested approach
Either update
src/lib/get-matches.tsto returnMatch[]objects, or import it and map the results:+import { getMatches as getMatchPositions } from "#lib/get-matches"; // Then in createEffect: - const matches = getMatches(props.content, props.searchTerm); + const positions = getMatchPositions(props.content, props.searchTerm); + const matches: Match[] = positions.map(pos => ({ + start: pos, + end: pos + props.searchTerm.length + }));And remove the local
getMatchesfunction (lines 132-145).Also applies to: 132-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Editor.tsx` around lines 90 - 100, The Editor component defines a local getMatches (returning Match[] with {start,end}) that conflicts with the shared src/lib/get-matches.ts (returns number[]), causing duplication and inconsistent types; fix by removing the local getMatches from Editor.tsx, import the shared getMatches from src/lib/get-matches.ts into Editor.tsx, and then either (A) update the shared getMatches to return Match[] (preferred if multiple callers need ranges) or (B) map the imported number[] to Match[] inside Editor (e.g., numbers -> {start: n, end: n + term.length}) so createEffect, setCaretPosition, and any use of matches/currentMatchIndex continue to work with the Match shape; ensure you update any types/usages (Match interface, createEffect usage) accordingly and delete the now-unused local function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/App.tsx`:
- Around line 78-81: The replace-all logic in handleReplaceAll creates a RegExp
with only the "g" flag, causing replacements to be case-sensitive while
getMatches is case-insensitive; update the RegExp creation for newContent (the
call to text.replace using new RegExp(RegExp.escape(searchTerm()), "g")) to
include the "i" flag (e.g., use "gi") so replacements mirror getMatches'
case-insensitive behavior while still escaping the searchTerm().
In `@src/components/Editor.tsx`:
- Around line 67-82: renderContent currently builds the regex from the raw
props.searchTerm but runs it against the HTML-escaped text, so terms with HTML
entities (e.g. "<" or "&") won't match; fix by HTML-escaping the search term
(via escapeHtml) first, then escape regex metacharacters on that
escapedSearchTerm before constructing the RegExp, and use that regex to replace
within the escaped text; update references in renderContent to use
escapedSearchTerm (and still handle empty/null searchTerm/text as before).
- Around line 104-121: The issue is that binding innerHTML={renderContent()} on
the contentEditable div resets the caret on every keystroke; stop setting
innerHTML on every render and only mutate the DOM when necessary (e.g., when
searchTerm or content-formatting changes). Replace the direct innerHTML prop
with a controlled update inside an effect: use editorRef to set innerHTML inside
a useEffect that depends on props.searchTerm (and initial mount/format changes)
rather than props.content on every keystroke, and ensure handleInput still calls
props.onChange without overriding the live DOM; also update setCaretPosition to
run when you intentionally update DOM for search highlights so selection
restoration occurs when you change innerHTML. Use the function names editorRef,
handleInput, renderContent, setCaretPosition, props.searchTerm, props.content,
and props.onChange to locate where to apply this change.
In `@src/components/Toolbar.tsx`:
- Around line 79-84: SearchSvgIcon currently draws using 24×24 coordinates but
inherits SvgIcon's default viewBox="0 0 240 240", causing it to render too
small; fix by adding an explicit viewBox="0 0 24 24" prop to the SvgIcon element
inside the SearchSvgIcon component (SearchSvgIcon and SvgIcon are the
identifiers to update) so the circle (cx=11) and path coordinates render at the
correct scale.
---
Nitpick comments:
In `@src/components/Editor.tsx`:
- Around line 90-100: The Editor component defines a local getMatches (returning
Match[] with {start,end}) that conflicts with the shared src/lib/get-matches.ts
(returns number[]), causing duplication and inconsistent types; fix by removing
the local getMatches from Editor.tsx, import the shared getMatches from
src/lib/get-matches.ts into Editor.tsx, and then either (A) update the shared
getMatches to return Match[] (preferred if multiple callers need ranges) or (B)
map the imported number[] to Match[] inside Editor (e.g., numbers -> {start: n,
end: n + term.length}) so createEffect, setCaretPosition, and any use of
matches/currentMatchIndex continue to work with the Match shape; ensure you
update any types/usages (Match interface, createEffect usage) accordingly and
delete the now-unused local function.
In `@src/components/FindReplaceModal.tsx`:
- Around line 5-16: The FindReplaceModalProps interface declares an unused prop
named content; remove the content field from the interface and any corresponding
prop usage (there should be none inside FindReplaceModal component) and update
the component call site (e.g., in App.tsx) to stop passing content={content()}
so the prop signature and callers remain consistent; reference the
FindReplaceModalProps type and the FindReplaceModal component as the locations
to change.
- Around line 57-76: Add Escape key handling so pressing Escape closes the
modal: update the keyboard handlers (handleFindKeyDown and handleReplaceKeyDown)
to detect e.key === "Escape" (or "Esc" for cross-browser) and call the existing
handleClose (which clears replace text and calls props.onClose()). Alternatively
attach a single keydown listener that calls handleClose on Escape; ensure you
preventDefault if needed and clean up any global listener on unmount.
In `@tests/get-matches.lib.test.ts`:
- Around line 4-7: Add comprehensive unit tests for getMatches to cover edge
cases beyond the happy path: add tests asserting that an empty search term
returns [], a search with no matches returns [], multiple non-overlapping
matches (e.g., "hello hello" vs "hello") returns [0,6], overlapping matches
(e.g., "aaaa" vs "aa") returns [0,1,2], and case-insensitive matching (e.g.,
"Hello HELLO" vs "hello") returns [0,6]; update tests in
tests/get-matches.lib.test.ts using the existing describe("getMatches", ...)
block and the getMatches function name so CI validates these behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4541fde7-464c-41a0-97d5-254352e994ac
📒 Files selected for processing (7)
package.jsonsrc/components/App.tsxsrc/components/Editor.tsxsrc/components/FindReplaceModal.tsxsrc/components/Toolbar.tsxsrc/lib/get-matches.tstests/get-matches.lib.test.ts
Comment on lines
+104
to
121
| <div | ||
| ref={editorRef} | ||
| contentEditable={true} | ||
| spellcheck={props.settings.spellcheck} | ||
| onInput={handleInput} | ||
| class="w-full absolute h-screen overflow-y-auto overflow-x-hidden | ||
| [word-break:break-word] text-black dark:text-white caret-blue-500 | ||
| bg-transparent delay-500 [scrollbar-width:thin] scroll-smooth | ||
| bg-transparent [scrollbar-width:thin] scroll-smooth | ||
| p-[calc(min(1em,20vh)+72px)_max(-372px+50vw,1em)_min(5em,15vh)] | ||
| scroll-pb-0 left-0 top-0" | ||
| scroll-pb-0 left-0 top-0 outline-none whitespace-pre-wrap" | ||
| style={{ | ||
| "font-size": `${props.settings.fontSize}px`, | ||
| "font-family": props.settings.fontFamily, | ||
| "text-align": props.settings.textAlign, | ||
| }} | ||
| spellcheck={props.settings.spellcheck} | ||
| onInput={(e) => props.onChange(e.currentTarget.value)} | ||
| value={props.content} | ||
| placeholder="Start writing..." | ||
| data-placeholder="Start writing..." | ||
| innerHTML={renderContent()} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any existing mechanism to preserve cursor position during typing
rg -n "selection|caret|cursor" src/components/Editor.tsxRepository: rmandotdev/editor
Length of output: 259
🏁 Script executed:
wc -l src/components/Editor.tsxRepository: rmandotdev/editor
Length of output: 90
🏁 Script executed:
cat -n src/components/Editor.tsxRepository: rmandotdev/editor
Length of output: 5045
Setting innerHTML on contentEditable resets cursor position on every keystroke.
When the user types, handleInput triggers props.onChange, which changes props.content. This causes renderContent() to re-evaluate and innerHTML is set, resetting the cursor. The setCaretPosition effect (lines 90-100) only executes its cursor restoration logic when props.searchTerm is truthy, so during normal typing without an active search, the cursor is never restored.
Consider using a diffing approach to update only the changed text nodes, or only update innerHTML when searchTerm changes (not on every keystroke).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Editor.tsx` around lines 104 - 121, The issue is that binding
innerHTML={renderContent()} on the contentEditable div resets the caret on every
keystroke; stop setting innerHTML on every render and only mutate the DOM when
necessary (e.g., when searchTerm or content-formatting changes). Replace the
direct innerHTML prop with a controlled update inside an effect: use editorRef
to set innerHTML inside a useEffect that depends on props.searchTerm (and
initial mount/format changes) rather than props.content on every keystroke, and
ensure handleInput still calls props.onChange without overriding the live DOM;
also update setCaretPosition to run when you intentionally update DOM for search
highlights so selection restoration occurs when you change innerHTML. Use the
function names editorRef, handleInput, renderContent, setCaretPosition,
props.searchTerm, props.content, and props.onChange to locate where to apply
this change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/App.tsx (1)
78-87:⚠️ Potential issue | 🟠 MajorReplace All should respect case sensitivity setting.
handleReplaceAllalways uses just the"g"flag, butgetMatches(which determines visible matches) respectscaseSensitive(). When case-insensitive matching is active, replacements won't match all highlighted occurrences.🔧 Proposed fix
const handleReplaceAll = (replacement: string) => { if (!searchTerm()) return; const text = content(); + const flags = caseSensitive() ? "g" : "gi"; const newContent = text.replace( - new RegExp(RegExp.escape(searchTerm()), "g"), + new RegExp(RegExp.escape(searchTerm()), flags), replacement, ); updatePageContent(newContent); setCurrentMatchIndex(0); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/App.tsx` around lines 78 - 87, handleReplaceAll currently always builds the replacement RegExp with only the "g" flag, so replacements don't honor the caseSensitive() setting used by getMatches; change the RegExp construction to include the "i" flag when caseSensitive() is false (e.g., compute flags = caseSensitive() ? "g" : "gi") and use new RegExp(RegExp.escape(searchTerm()), flags) so replacements match the same items getMatches highlights; keep the rest of the flow (updatePageContent and setCurrentMatchIndex).src/components/Editor.tsx (2)
89-111:⚠️ Potential issue | 🟠 MajorCursor resets during normal typing when search is inactive.
The effect updates
innerHTMLon every content change, but cursor restoration (line 108-110) only runs whensearchTermis truthy. During normal typing without an active search, the cursor will jump to the end after each keystroke.Consider either:
- Always restoring the cursor position, or
- Skipping the
innerHTMLupdate when there's no active search and content hasn't changed structurally🔧 Proposed fix — always restore cursor
editorRef.innerHTML = html; - if (searchTerm && hadSelection) { + if (hadSelection) { setCaretPosition(editorRef, savedOffset); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Editor.tsx` around lines 89 - 111, The effect replaces editorRef.innerHTML on every run but only restores the caret when props.searchTerm is set, causing the cursor to jump during normal typing; update the logic in createEffect so after computing html via renderContent() and assigning editorRef.innerHTML = html you always restore the caret when a prior selection was detected (use the savedOffset/hadSelection variables and call setCaretPosition(editorRef, savedOffset) regardless of props.searchTerm), and optionally avoid unnecessary DOM rewrites by comparing the new html with editorRef.innerHTML before assigning to skip innerHTML updates when unchanged.
68-81:⚠️ Potential issue | 🟡 MinorSearch term with HTML entities still won't match correctly.
The previous review flagged this issue: if
searchTermcontains<,>, or&, the regex is built from the raw term but applied against HTML-escaped content. For example, searching for<div>looks for<div>in text containing<div>.The fix should HTML-escape the search term before regex-escaping it.
🔧 Proposed fix
const renderContent = (): string => { const text = props.content; if (!props.searchTerm || !text) return escapeHtml(text); const escaped = escapeHtml(text); - const searchEscaped = RegExp.escape(props.searchTerm); + const searchEscaped = RegExp.escape(escapeHtml(props.searchTerm)); const flags = props.caseSensitive ? "" : "i"; const regex = new RegExp(`(${searchEscaped})`, `g${flags}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Editor.tsx` around lines 68 - 81, The current renderContent builds a regex from props.searchTerm and applies it to HTML-escaped content, so terms like "<div>" won't match; fix by HTML-escaping the search term before regex-escaping: call escapeHtml on props.searchTerm first, then pass that result to RegExp.escape (preserving caseSensitive flag logic) so the constructed regex (in renderContent) matches the escaped content; update the sequence around RegExp.escape(props.searchTerm) to use escapeHtml(props.searchTerm) and keep existing RegExp/String handling.
🧹 Nitpick comments (1)
src/components/FindReplaceModal.tsx (1)
20-36: Consider reducingstroke-widthfor the icons.The
stroke-width="20"on a24x24viewBox will render extremely thick strokes (nearly solid shapes). Icons at this scale typically use a stroke-width of 1.5–2.5. The toolbar'sSearchSvgIconusesstroke-width={20}on the same coordinates but with a240x240viewBox, so the relative thickness is 10× less.♻️ Suggested fix
const SearchIcon = (): JSX.Element => ( <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" - stroke-width="20" + stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-label="Search" >const CloseIcon = (): JSX.Element => ( <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" - stroke-width="20" + stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-label="Close" >Also applies to: 38-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FindReplaceModal.tsx` around lines 20 - 36, The SVG SearchIcon uses an excessively large stroke-width ("20") for a 24x24 viewBox; update the SVG props on SearchIcon (and the other icons at 38-54) to use a normal icon stroke thickness (e.g., 1.5–2) and use JSX prop names (strokeWidth, strokeLinecap, strokeLinejoin) instead of the HTML-style hyphenated attributes so the rendered stroke is correctly sized and the JSX compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/App.tsx`:
- Around line 72-76: The call to getMatches when recomputing after a replacement
is missing the caseSensitive argument, causing inconsistent match counts; update
the call in the block that defines newMatches (the code that currently does
const newMatches = getMatches(newContent, searchTerm()); and then
setCurrentMatchIndex(...)) to pass the same case sensitivity flag used elsewhere
(the caseSensitive state/selector used in the memo on lines ~42-44), i.e. call
getMatches(newContent, searchTerm(), caseSensitive) (or caseSensitive()) so
newMatches uses the same matching rules before calling setCurrentMatchIndex.
---
Duplicate comments:
In `@src/components/App.tsx`:
- Around line 78-87: handleReplaceAll currently always builds the replacement
RegExp with only the "g" flag, so replacements don't honor the caseSensitive()
setting used by getMatches; change the RegExp construction to include the "i"
flag when caseSensitive() is false (e.g., compute flags = caseSensitive() ? "g"
: "gi") and use new RegExp(RegExp.escape(searchTerm()), flags) so replacements
match the same items getMatches highlights; keep the rest of the flow
(updatePageContent and setCurrentMatchIndex).
In `@src/components/Editor.tsx`:
- Around line 89-111: The effect replaces editorRef.innerHTML on every run but
only restores the caret when props.searchTerm is set, causing the cursor to jump
during normal typing; update the logic in createEffect so after computing html
via renderContent() and assigning editorRef.innerHTML = html you always restore
the caret when a prior selection was detected (use the savedOffset/hadSelection
variables and call setCaretPosition(editorRef, savedOffset) regardless of
props.searchTerm), and optionally avoid unnecessary DOM rewrites by comparing
the new html with editorRef.innerHTML before assigning to skip innerHTML updates
when unchanged.
- Around line 68-81: The current renderContent builds a regex from
props.searchTerm and applies it to HTML-escaped content, so terms like "<div>"
won't match; fix by HTML-escaping the search term before regex-escaping: call
escapeHtml on props.searchTerm first, then pass that result to RegExp.escape
(preserving caseSensitive flag logic) so the constructed regex (in
renderContent) matches the escaped content; update the sequence around
RegExp.escape(props.searchTerm) to use escapeHtml(props.searchTerm) and keep
existing RegExp/String handling.
---
Nitpick comments:
In `@src/components/FindReplaceModal.tsx`:
- Around line 20-36: The SVG SearchIcon uses an excessively large stroke-width
("20") for a 24x24 viewBox; update the SVG props on SearchIcon (and the other
icons at 38-54) to use a normal icon stroke thickness (e.g., 1.5–2) and use JSX
prop names (strokeWidth, strokeLinecap, strokeLinejoin) instead of the
HTML-style hyphenated attributes so the rendered stroke is correctly sized and
the JSX compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6efae633-a790-4a81-a5f2-a3d3b4c90d5c
📒 Files selected for processing (8)
AGENTS.mdsrc/components/App.tsxsrc/components/Editor.tsxsrc/components/FindReplaceModal.tsxsrc/components/Toolbar.tsxsrc/components/ui/SvgIcon.tsxsrc/lib/get-matches.tstests/get-matches.lib.test.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/get-matches.ts
- tests/get-matches.lib.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Editor.tsx (1)
132-148: Consider adding accessibility attributes.The
contentEditablediv acts as a text input but lacks ARIA attributes to communicate its role to assistive technologies.♿ Suggested accessibility attributes
<div ref={editorRef} contentEditable={true} + role="textbox" + aria-multiline="true" + aria-label="Document editor" spellcheck={props.settings.spellcheck} onInput={handleInput}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Editor.tsx` around lines 132 - 148, The contentEditable div used with editorRef and handleInput should expose proper ARIA and focus attributes so assistive tech recognizes it as an editable text field—add role="textbox", aria-multiline="true", a descriptive aria-label or aria-labelledby (e.g., referencing a visible label), and tabIndex={0} so it’s keyboard-focusable; also include an accessible placeholder via aria-placeholder or ensure the placeholder text is exposed through aria-label/aria-labelledby when empty and keep spellcheck/font props from props.settings unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Editor.tsx`:
- Around line 114-128: The effect that calls getMatches and setCaretPosition
(createEffect using editorRef, props.searchTerm, props.content, getMatches,
props.currentMatchIndex) should only run when the search index actually changes
to avoid jumping caret while typing; implement a previous-index tracker (a local
mutable ref or a createSignal to hold prevIndex) and early-return unless
props.currentMatchIndex is defined and differs from prevIndex, then compute
matches (or reuse cached matches) and call setCaretPosition(editorRef,
match.start) only when the index changed, finally update prevIndex to
props.currentMatchIndex; also remove props.content from the reactive
dependencies for this navigation effect so typing doesn’t trigger it.
- Line 73: The usage of the ES2025 RegExp.escape in Editor.tsx (the line
creating searchEscaped via RegExp.escape(escapeHtml(props.searchTerm))) can
throw in older browsers; replace it with a safe fallback or polyfill: add a
small compatibility check at module scope (e.g., if (typeof RegExp.escape !==
"function") { RegExp.escape = /* fallback implementation */ }) or implement a
local helper function (e.g., safeRegExpEscape) and use that instead of
RegExp.escape when building searchEscaped; update the code that references
RegExp.escape (the line using escapeHtml(props.searchTerm)) to call the
fallback/helper to ensure runtime compatibility.
---
Nitpick comments:
In `@src/components/Editor.tsx`:
- Around line 132-148: The contentEditable div used with editorRef and
handleInput should expose proper ARIA and focus attributes so assistive tech
recognizes it as an editable text field—add role="textbox",
aria-multiline="true", a descriptive aria-label or aria-labelledby (e.g.,
referencing a visible label), and tabIndex={0} so it’s keyboard-focusable; also
include an accessible placeholder via aria-placeholder or ensure the placeholder
text is exposed through aria-label/aria-labelledby when empty and keep
spellcheck/font props from props.settings unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cbca455-6ede-4cb3-89f5-571a506381e9
📒 Files selected for processing (5)
biome.jsoncsrc/components/App.tsxsrc/components/Editor.tsxsrc/components/FindReplaceModal.tsxsrc/hooks/usePages.ts
✅ Files skipped from review due to trivial changes (1)
- biome.jsonc
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/FindReplaceModal.tsx
- src/components/App.tsx
| if (!props.searchTerm || !text) return escapeHtml(text); | ||
|
|
||
| const escaped = escapeHtml(text); | ||
| const searchEscaped = RegExp.escape(escapeHtml(props.searchTerm)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RegExp.escape browser support ES2024
💡 Result:
RegExp.escape() is not an ES2024 feature; it’s standardized in ECMAScript 2025 (ES2025) (added to the spec as RegExp.escape). [3]
Browser support (from “Can I use”): [1]
- Chrome: 136+
- Edge: 136+
- Firefox: 134+
- Safari (macOS): 18.2+
- iOS Safari: 18.2+
- Samsung Internet: 29+
MDN summarizes this as becoming broadly available across “latest” browsers since May 2025. [2]
Sources:
[1] Can I use: RegExp.escape() (caniuse.com)
[2] MDN: RegExp.escape() (Baseline 2025 / since May 2025) (developer.mozilla.org)
[3] ECMA-262 (ECMAScript 2025) PDF mentioning RegExp.escape (ecma-international.org)
Citations:
- 1: https://caniuse.com/wf-regexp-escape
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/escape
- 3: https://www.ecma-international.org/wp-content/uploads/ECMA-262.pdf?utm_source=openai
Add a polyfill or compatibility check for RegExp.escape.
RegExp.escape is an ES2025 feature with limited browser support (Chrome 136+, Firefox 134+, Safari 18.2+, and others as of May 2025). Browsers below these versions will throw a runtime error. Either add a polyfill or verify your target environments support it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Editor.tsx` at line 73, The usage of the ES2025 RegExp.escape
in Editor.tsx (the line creating searchEscaped via
RegExp.escape(escapeHtml(props.searchTerm))) can throw in older browsers;
replace it with a safe fallback or polyfill: add a small compatibility check at
module scope (e.g., if (typeof RegExp.escape !== "function") { RegExp.escape =
/* fallback implementation */ }) or implement a local helper function (e.g.,
safeRegExpEscape) and use that instead of RegExp.escape when building
searchEscaped; update the code that references RegExp.escape (the line using
escapeHtml(props.searchTerm)) to call the fallback/helper to ensure runtime
compatibility.
Comment on lines
+114
to
+128
| createEffect(() => { | ||
| if (editorRef && props.searchTerm) { | ||
| const matches = getMatches( | ||
| props.content, | ||
| props.searchTerm, | ||
| props.caseSensitive, | ||
| ); | ||
| if (matches.length > 0 && props.currentMatchIndex !== undefined) { | ||
| const match = matches[props.currentMatchIndex]; | ||
| if (match) { | ||
| setCaretPosition(editorRef, match.start); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Effect reacts to content changes, causing caret to jump during typing.
This effect tracks props.content (via getMatches). When the user types with an active search, both effects run: the first effect restores caret to typing position, then this effect immediately moves it to the current match position—disrupting typing.
Consider using a previous-value pattern to only navigate when currentMatchIndex actually changes:
🔧 Proposed fix using previous index tracking
+import { createEffect, createSignal, type JSX } from "solid-js";
...
function Editor(props: EditorProps): JSX.Element {
let editorRef: HTMLDivElement | undefined;
+ const [prevMatchIndex, setPrevMatchIndex] = createSignal<number | undefined>(undefined);
// ... existing code ...
createEffect(() => {
- if (editorRef && props.searchTerm) {
+ const currentIndex = props.currentMatchIndex;
+ const searchTerm = props.searchTerm;
+
+ if (editorRef && searchTerm && currentIndex !== undefined && currentIndex !== prevMatchIndex()) {
+ setPrevMatchIndex(currentIndex);
const matches = getMatches(
props.content,
- props.searchTerm,
+ searchTerm,
props.caseSensitive,
);
- if (matches.length > 0 && props.currentMatchIndex !== undefined) {
- const match = matches[props.currentMatchIndex];
+ if (matches.length > 0) {
+ const match = matches[currentIndex];
if (match) {
setCaretPosition(editorRef, match.start);
}
}
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Editor.tsx` around lines 114 - 128, The effect that calls
getMatches and setCaretPosition (createEffect using editorRef, props.searchTerm,
props.content, getMatches, props.currentMatchIndex) should only run when the
search index actually changes to avoid jumping caret while typing; implement a
previous-index tracker (a local mutable ref or a createSignal to hold prevIndex)
and early-return unless props.currentMatchIndex is defined and differs from
prevIndex, then compute matches (or reuse cached matches) and call
setCaretPosition(editorRef, match.start) only when the index changed, finally
update prevIndex to props.currentMatchIndex; also remove props.content from the
reactive dependencies for this navigation effect so typing doesn’t trigger it.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/Editor.tsx (2)
74-76:⚠️ Potential issue | 🟠 MajorAvoid hard-requiring
RegExp.escape()in the render path.Older runtimes will throw here before search highlighting even renders. A tiny local escape helper gives the same behavior without depending on that newer API.
What is the current browser support for `RegExp.escape()` across Chrome, Firefox, Safari, and Edge?🛡️ Compatibility-safe alternative
+function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + function Editor(props: EditorProps): JSX.Element { let editorRef: HTMLDivElement | undefined; @@ - const searchEscaped = RegExp.escape(escapeHtml(props.searchTerm)); + const searchEscaped = escapeRegExp(escapeHtml(props.searchTerm));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Editor.tsx` around lines 74 - 76, The render code calls RegExp.escape which can throw in older runtimes; create a small local helper (e.g., escapeRegExp) near the component and use it instead of RegExp.escape when computing searchEscaped (still wrap props.searchTerm with escapeHtml first), then build the regex the same way using the flags logic (props.caseSensitive ? "" : "i") and new RegExp(`(${searchEscaped})`, `g${flags}`); update any references to searchEscaped to use the helper so you avoid relying on the newer RegExp.escape API.
115-129:⚠️ Potential issue | 🟠 MajorOnly run match navigation when the selected match actually changes.
This effect still tracks
props.content. While the editor is focused, every keystroke with an active search can snap the caret back tocurrentMatchIndex; while the modal input has focus, thedocument.activeElement === editorRefguard means next/prev won't reveal anything at all. Gate this on index changes, and scroll the selected<mark>into view without depending on editor focus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Editor.tsx` around lines 115 - 129, The effect should only run when the selected match index (and search term) change instead of on every content change; update the createEffect to depend on props.currentMatchIndex and props.searchTerm, compute matches via getMatches(props.content, props.searchTerm, props.caseSensitive), locate the corresponding match by index, then find the rendered <mark> for that match (e.g. via a data-match-index attribute or querySelector) and call element.scrollIntoView({ block: 'nearest', inline: 'nearest', behavior: 'auto' }); only call setCaretPosition(editorRef, match.start) if document.activeElement === editorRef; this prevents caret snapping on unrelated edits while still scrolling the highlighted match into view when navigation index changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Editor.tsx`:
- Around line 69-82: renderContent() and getMatches() use different matching
rules causing mismatched highlights vs match counts (e.g., overlapping matches
like "ana" in "banana"); make them consistent by changing renderContent() to use
the same overlapping-match logic as getMatches() — use a lookahead-based regex
(e.g., (?=(...))) or iterate with regex.exec advancing by 1 char to find every
overlapping match, then build the escaped output by slicing and inserting the
<mark> wrappers at each match offset so the number and positions of rendered
marks match getMatches() and currentMatchIndex.
In `@src/components/FindReplaceModal.tsx`:
- Around line 88-95: Wrap the floating container div in dialog semantics by
adding role="dialog", aria-modal="true", and aria-labelledby pointing to the
Find & Replace title span (give that span a stable id) so assistive tech
recognizes it as a dialog; also ensure the dialog gets keyboard focus when
opened. For the case-sensitivity control (the "Aa" button) expose its pressed
state with aria-pressed (true/false) and include an accessible name (aria-label
like "Toggle case sensitivity") so screen readers announce its state; apply the
same changes to the duplicate popup instance in this component.
In `@src/components/Toolbar.tsx`:
- Around line 95-98: The icon-only search Button (the Button element with
variant="toolbar" and onClick={props.onSearchClick} in Toolbar.tsx) lacks an
accessible name; add an explicit aria-label (e.g., aria-label="Search" or a
localized label) to that Button so assistive tech can announce it, ensuring you
place the aria-label prop on the same Button element that renders <SearchSvgIcon
/>.
---
Duplicate comments:
In `@src/components/Editor.tsx`:
- Around line 74-76: The render code calls RegExp.escape which can throw in
older runtimes; create a small local helper (e.g., escapeRegExp) near the
component and use it instead of RegExp.escape when computing searchEscaped
(still wrap props.searchTerm with escapeHtml first), then build the regex the
same way using the flags logic (props.caseSensitive ? "" : "i") and new
RegExp(`(${searchEscaped})`, `g${flags}`); update any references to
searchEscaped to use the helper so you avoid relying on the newer RegExp.escape
API.
- Around line 115-129: The effect should only run when the selected match index
(and search term) change instead of on every content change; update the
createEffect to depend on props.currentMatchIndex and props.searchTerm, compute
matches via getMatches(props.content, props.searchTerm, props.caseSensitive),
locate the corresponding match by index, then find the rendered <mark> for that
match (e.g. via a data-match-index attribute or querySelector) and call
element.scrollIntoView({ block: 'nearest', inline: 'nearest', behavior: 'auto'
}); only call setCaretPosition(editorRef, match.start) if document.activeElement
=== editorRef; this prevents caret snapping on unrelated edits while still
scrolling the highlighted match into view when navigation index changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69610b12-9945-4c99-b89b-2a7fd32281c6
📒 Files selected for processing (5)
biome.jsoncsrc/components/App.tsxsrc/components/Editor.tsxsrc/components/FindReplaceModal.tsxsrc/components/Toolbar.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- biome.jsonc
- src/components/App.tsx
Comment on lines
+69
to
+82
| const renderContent = (): string => { | ||
| const text = props.content; | ||
| if (!props.searchTerm || !text) return escapeHtml(text); | ||
|
|
||
| const escaped = escapeHtml(text); | ||
| const searchEscaped = RegExp.escape(escapeHtml(props.searchTerm)); | ||
| const flags = props.caseSensitive ? "" : "i"; | ||
| const regex = new RegExp(`(${searchEscaped})`, `g${flags}`); | ||
|
|
||
| return escaped.replace( | ||
| regex, | ||
| '<mark class="bg-yellow-300 dark:bg-yellow-600 rounded px-0.5">$1</mark>', | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Unify the matching rules used for highlights and indexes.
renderContent() uses global regex replacement, so it only marks non-overlapping hits, but getMatches() advances by one character and counts overlaps. Searches like ana in banana can report two matches while only one <mark> is rendered, and currentMatchIndex can point to a match the user never sees.
🔧 Minimal fix if overlaps are not intended
while (found !== -1) {
matches.push({ start: found, end: found + searchTerm.length });
- pos = found + 1;
+ pos = found + searchTerm.length;
found = searchContent.indexOf(search, pos);
}Also applies to: 172-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Editor.tsx` around lines 69 - 82, renderContent() and
getMatches() use different matching rules causing mismatched highlights vs match
counts (e.g., overlapping matches like "ana" in "banana"); make them consistent
by changing renderContent() to use the same overlapping-match logic as
getMatches() — use a lookahead-based regex (e.g., (?=(...))) or iterate with
regex.exec advancing by 1 char to find every overlapping match, then build the
escaped output by slicing and inserting the <mark> wrappers at each match offset
so the number and positions of rendered marks match getMatches() and
currentMatchIndex.
Comment on lines
+88
to
+95
| <div | ||
| class="fixed top-14 right-4 z-50 bg-white dark:bg-[#1a1a1a] | ||
| border border-[#ccc] dark:border-[#444] rounded-lg shadow-xl p-4 min-w-80" | ||
| > | ||
| <div class="flex justify-between items-center mb-3"> | ||
| <span class="text-black dark:text-white font-medium"> | ||
| Find & Replace | ||
| </span> |
There was a problem hiding this comment.
Expose the popup as a dialog and the case control as a toggle.
Assistive tech currently just gets a floating <div> and an Aa button with no state. Adding dialog semantics on the container plus aria-pressed on the case-sensitivity control makes the feature understandable without sight.
♿ Suggested accessibility fix
<div
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="find-replace-title"
class="fixed top-14 right-4 z-50 bg-white dark:bg-[`#1a1a1a`]
border border-[`#ccc`] dark:border-[`#444`] rounded-lg shadow-xl p-4 min-w-80"
>
<div class="flex justify-between items-center mb-3">
- <span class="text-black dark:text-white font-medium">
+ <span
+ id="find-replace-title"
+ class="text-black dark:text-white font-medium"
+ >
Find & Replace
</span>
@@
<button
type="button"
onClick={() =>
props.onCaseSensitiveChange(!props.caseSensitive)
}
+ aria-label="Match case"
+ aria-pressed={props.caseSensitive}
class="px-2 py-1 text-xs font-mono bg-[`#f5f5f5`] dark:bg-[`#2a2a2a`] border border-[`#ddd`]
dark:border-[`#444`] rounded text-black dark:text-white cursor-pointer
hover:bg-[`#eee`] dark:hover:bg-[`#333`]"Also applies to: 151-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/FindReplaceModal.tsx` around lines 88 - 95, Wrap the floating
container div in dialog semantics by adding role="dialog", aria-modal="true",
and aria-labelledby pointing to the Find & Replace title span (give that span a
stable id) so assistive tech recognizes it as a dialog; also ensure the dialog
gets keyboard focus when opened. For the case-sensitivity control (the "Aa"
button) expose its pressed state with aria-pressed (true/false) and include an
accessible name (aria-label like "Toggle case sensitivity") so screen readers
announce its state; apply the same changes to the duplicate popup instance in
this component.
Comment on lines
+95
to
+98
| <div class="flex gap-2"> | ||
| <Button variant="toolbar" onClick={props.onSearchClick}> | ||
| <SearchSvgIcon /> | ||
| </Button> |
There was a problem hiding this comment.
Label the new icon-only search button.
Right now this control renders as SVG-only content, so assistive tech will likely announce an unnamed button. Please pass an explicit aria-label here.
♿ Suggested fix
- <Button variant="toolbar" onClick={props.onSearchClick}>
+ <Button
+ variant="toolbar"
+ onClick={props.onSearchClick}
+ aria-label="Find and replace"
+ >
<SearchSvgIcon />
</Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Toolbar.tsx` around lines 95 - 98, The icon-only search Button
(the Button element with variant="toolbar" and onClick={props.onSearchClick} in
Toolbar.tsx) lacks an accessible name; add an explicit aria-label (e.g.,
aria-label="Search" or a localized label) to that Button so assistive tech can
announce it, ensuring you place the aria-label prop on the same Button element
that renders <SearchSvgIcon />.
Merged
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
1 participant
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.
Closes #5
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores