perf: Improve search performance in code editor #6920
Conversation
Its-treason
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a cache-backed search layer to CodeMirrorSearch: computes a cache key from search params, caches up to MAX_MATCHES, and updates doSearch to use cached matches (marking only previous/current/next on hits) or recompute and mark all on misses; clears cache on close/errors and syncs navigation with debounced input. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,200,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant UI as Search UI
end
rect rgba(255,200,200,0.5)
participant Search as CodeMirrorSearch
end
rect rgba(255,255,200,0.5)
participant Editor
end
rect rgba(220,220,220,0.5)
participant Cache
end
User->>UI: type search term (debounced)
UI->>Search: trigger doSearch(debouncedText, options)
Search->>Cache: createCacheKey(debouncedText, options)
Cache-->>Search: cache hit/miss
alt cache miss
Search->>Editor: findSearchMatches(...) (scan document)
Editor-->>Search: matches[]
Search->>Cache: store limited matches
Search->>Editor: mark all matches, highlight current, scroll/select
else cache hit
Cache-->>Search: cached matches subset
Search->>Editor: mark prev/current/next only, highlight current, scroll/select
end
UI->>Search: navigate Next/Prev
Search->>Editor: update selection/highlight accordingly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/components/CodeMirrorSearch/index.js`:
- Around line 147-150: The useEffect in CodeMirrorSearch currently contains a
debug console.log statement; remove the console.log('reset search!!') from the
effect that depends on debouncedSearchText and doSearch so the hook simply calls
doSearch(0) without emitting debug output (look for the useEffect referencing
debouncedSearchText and doSearch).
🧹 Nitpick comments (4)
packages/bruno-app/src/components/CodeMirrorSearch/index.js (4)
16-20: Silent swallow of regex parsing errors.The empty
catch {}block silently discards regex syntax errors. Consider logging the error for debugging purposes when users enter invalid regex patterns.♻️ Suggested improvement
try { query = new RegExp(searchText, caseSensitive ? 'g' : 'gi'); - } catch { + } catch (e) { + console.warn('Invalid regex pattern:', e.message); return []; }
100-101: Variable shadowing: localmatchIndexshadows state variable.The local constant
matchIndexshadows the state variable of the same name declared at line 53. This can cause confusion during maintenance.♻️ Rename local variable
- const matchIndex = Math.max(0, Math.min(newIndex, matches.length - 1)); - setMatchIndex(matchIndex); + const clampedIndex = Math.max(0, Math.min(newIndex, matches.length - 1)); + setMatchIndex(clampedIndex);Then update all subsequent references within
doSearchto useclampedIndexinstead ofmatchIndex.
173-189: RedundantdoSearch(0)calls in toggle handlers use stale closure values.When
setRegex,setCaseSensitive, orsetWholeWordis called, the state update is asynchronous. The immediately followingdoSearch(0)still captures the old values from the closure. TheuseEffectat line 147 will correctly trigger a search whendoSearchis recreated with new dependencies, making these explicit calls redundant and potentially causing a flicker.♻️ Remove redundant doSearch calls
const handleToggleRegex = () => { setRegex((prev) => !prev); setMatchIndex(0); - doSearch(0); }; const handleToggleCase = () => { setCaseSensitive((prev) => !prev); setMatchIndex(0); - doSearch(0); }; const handleToggleWholeWord = () => { setWholeWord((prev) => !prev); setMatchIndex(0); - doSearch(0); };
191-203: Minor redundancy: doublesetMatchIndexcalls.Both
handleNextandhandlePrevcallsetMatchIndexbeforedoSearch, butdoSearchalso callssetMatchIndexat line 101. This is harmless (React batches updates) but slightly redundant. Consider removing thesetMatchIndexcalls here sincedoSearchhandles it.
Its-treason
force-pushed
the
fix-search-perf
branch
from
81dc617 to
942f836
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/CodeMirrorSearch/index.js (1)
179-195: Toggle handlers calldoSearchbefore state updates take effect.When
handleToggleRegex(and similar handlers) calldoSearch(0)immediately aftersetRegex((prev) => !prev), thedoSearchcallback still captures the oldregexvalue from its closure. The cache key will be computed with stale state, potentially returning incorrect cached results.🐛 Suggested fix: let the useEffect handle re-search
Remove the explicit
doSearch(0)calls from toggle handlers. TheuseEffecton line 154-156 combined with thedoSearchdependency onregex/caseSensitive/wholeWordshould trigger a re-search automatically.const handleToggleRegex = () => { setRegex((prev) => !prev); setMatchIndex(0); - doSearch(0); }; const handleToggleCase = () => { setCaseSensitive((prev) => !prev); setMatchIndex(0); - doSearch(0); }; const handleToggleWholeWord = () => { setWholeWord((prev) => !prev); setMatchIndex(0); - doSearch(0); };However, note that the current
useEffectonly depends ondebouncedSearchTextanddoSearch. SincedoSearchchanges whenregex/caseSensitive/wholeWordchange (viauseCallbackdeps), this should trigger a re-search. Verify this works as expected.
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/components/CodeMirrorSearch/index.js`:
- Around line 91-94: When matches.length is 0 you return early after
setMatchIndex(0) but you must also clear any existing search highlights; update
the branch that checks matches.length to call the existing mark-clearing routine
(e.g., clearMarks(), removeSearchMarks(), or iterate stored marks and call
.clear()) before returning so stale highlights from a prior search are removed;
ensure you reference the same marks storage used elsewhere in this module when
clearing.
- Around line 44-46: The cache key created by createCacheKey(searchText, regex,
caseSensitive, wholeWord) doesn't include document identity so cached match
positions become stale after edits; update createCacheKey to accept a document
generation or content hash and incorporate it into the key, and in doSearch
obtain that value from the editor (e.g., editor.getDoc().changeGeneration() or a
lightweight content hash) and pass it when calling createCacheKey so the cache
is invalidated whenever the document changes.
- Around line 99-123: The isCacheHit branch currently only tries to clear the
old "current" mark by inspecting a non-existent className on TextMarker objects,
so marks accumulate; update the isCacheHit handling to mirror the non-cache-hit
path by clearing all existing markers in searchMarks.current (call .clear() on
each and reset the array) before creating the three new markers for
prev/current/next using editor.markText with className 'cm-search-current' or
'cm-search-match' and push them onto searchMarks.current (use matches and
matchIndex to compute prev/current/next indices).
🧹 Nitpick comments (1)
packages/bruno-app/src/components/CodeMirrorSearch/index.js (1)
96-97: Variable shadowing: localmatchIndexshadows state variable.Line 96 declares a local
const matchIndexthat shadows the state variable from line 53. This makes the code harder to reason about.♻️ Rename local variable
- const matchIndex = Math.max(0, Math.min(newIndex, matches.length - 1)); - setMatchIndex(matchIndex); + const clampedIndex = Math.max(0, Math.min(newIndex, matches.length - 1)); + setMatchIndex(clampedIndex);Then use
clampedIndexthroughout the rest of the function instead ofmatchIndex.
Its-treason
force-pushed
the
fix-search-perf
branch
2 times, most recently
from
12bfeff to
85bcb26
Compare
Added cache key to reduce duplicate searches over the complete text. Added incremental update logic, to not update every marked entry when clicking "next" or "previous". Fixes: usebruno#6913
Its-treason
force-pushed
the
fix-search-perf
branch
from
85bcb26 to
e3d7ccc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/CodeMirrorSearch/index.js (1)
185-201: Remove synchronousdoSearch(0)calls from toggle handlers.When you call
setRegex,setCaseSensitive, orsetWholeWord, React schedules the state update asynchronously. The synchronousdoSearch(0)calls immediately after read stale state values. However, thedoSearchuseCallback is itself a dependency of theuseEffecton line 160-162. SincedoSearchdepends onregex,caseSensitive, andwholeWord(line 158), changing any of these causes the callback to be recreated, which automatically triggers theuseEffectwith fresh state. The synchronous calls are redundant and cause redundant searches.Remove
doSearch(0)fromhandleToggleRegex,handleToggleCase, andhandleToggleWholeWord.Proposed fix
const handleToggleRegex = () => { setRegex((prev) => !prev); setMatchIndex(0); - doSearch(0); }; const handleToggleCase = () => { setCaseSensitive((prev) => !prev); setMatchIndex(0); - doSearch(0); }; const handleToggleWholeWord = () => { setWholeWord((prev) => !prev); setMatchIndex(0); - doSearch(0); };
♻️ Duplicate comments (1)
packages/bruno-app/src/components/CodeMirrorSearch/index.js (1)
104-129: Cache-hit mark cleanup still broken—marks will accumulate.CodeMirror 5's
TextMarkerdoes not expose theclassNameoption as a readable property. ThefindIndexcheck on line 106 always returns-1, so no marks are cleared. Each Next/Prev click adds 2-3 new marks, causing the same memory growth this PR aims to fix.Clear all marks before adding new ones, matching the non-cache path pattern.
🐛 Proposed fix
if (isCacheHit) { - // Clear only old current mark - const oldIndex = searchMarks.current.findIndex(mark => mark.className?.includes('cm-search-current')); - - if (oldIndex !== -1) { - searchMarks.current[oldIndex].clear(); - searchMarks.current.splice(oldIndex, 1); - } - - // Add mark to the new current and remark the previous and next + // Clear previous marks + searchMarks.current.forEach((mark) => mark.clear()); + searchMarks.current = []; + + // Mark only previous/current/next for performance const toMark = [ - // Previous matchIndex > 0 ? matchIndex - 1 : null, - // Current matchIndex, - // Next matchIndex < matches.length - 1 ? matchIndex + 1 : null ].filter((i) => i !== null); toMark.forEach((i) => { const mark = editor.markText(matches[i].from, matches[i].to, { className: i === matchIndex ? 'cm-search-current' : 'cm-search-match', clearOnEnter: true }); searchMarks.current.push(mark); });
🧹 Nitpick comments (3)
packages/bruno-app/src/components/CodeMirrorSearch/index.js (3)
44-46: Expensive cache key generation for large documents.
editor.getValue()copies the entire document content on everydoSearchcall. For the large JSON payloads this PR targets (per issue#6913), this defeats some of the performance gains.Use
editor.getDoc().changeGeneration()instead—it's O(1) and increments on each edit.♻️ Suggested refactor
-function createCacheKey(editor, searchText, regex, caseSensitive, wholeWord) { - return `${editor.getValue()}-${searchText}-${regex}-${caseSensitive}-${wholeWord}`; +function createCacheKey(editor, searchText, regex, caseSensitive, wholeWord) { + const docGen = editor.getDoc().changeGeneration(); + return `${docGen}-${searchText}-${regex}-${caseSensitive}-${wholeWord}`; }
101-102: Variable shadowing: localmatchIndexshadows state.Declaring
const matchIndexhere shadows the state variable from line 53. This compiles but is confusing—readers may not realize this is a different variable.♻️ Rename to avoid confusion
- const matchIndex = Math.max(0, Math.min(newIndex, matches.length - 1)); - setMatchIndex(matchIndex); + const clampedIndex = Math.max(0, Math.min(newIndex, matches.length - 1)); + setMatchIndex(clampedIndex);Then update all references below (lines 106, 116-120, 125, 138, 145, 149, 150) from
matchIndextoclampedIndex.
203-215: RedundantsetMatchIndexin navigation handlers.Both
handleNextandhandlePrevcallsetMatchIndex(next/prev)and then pass the same value todoSearch(next/prev). InsidedoSearch, line 102 already callssetMatchIndex. The state is set twice.Remove the redundant
setMatchIndexcalls here.♻️ Simplify navigation handlers
const handleNext = () => { if (!searchMatches.current || !searchMatches.current.length) return; let next = (matchIndex + 1) % searchMatches.current.length; - setMatchIndex(next); doSearch(next); }; const handlePrev = () => { if (!searchMatches.current || !searchMatches.current.length) return; let prev = (matchIndex - 1 + searchMatches.current.length) % searchMatches.current.length; - setMatchIndex(prev); doSearch(prev); };
1 participant
Added cache key to reduce duplicate searches over the complete text. Added incremental update logic, to not update every marked entry when clicking "next" or "previous". Limited the maximum results to 99,999 to prevent long loading times.
Fixes: #6913
Preview
Before
2026-01-26.00-11-56.mp4
After
2026-01-26.00-13-07.mp4
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.