poc: codeeditor shrink state persist#7797
Open
shubh-bruno wants to merge 1 commit intousebruno:mainfrom
Open
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughIntroduced per-tab document caching in CodeEditor using a module-level Map keyed by Changes
Sequence DiagramsequenceDiagram
actor User
participant React as React Component
participant CM as CodeEditor Instance
participant Cache as docCache Map
participant Editor as CodeMirror Editor
rect rgba(0, 150, 255, 0.5)
Note over User,Editor: Initial Mount or Tab Switch
User->>React: Mount component / Switch tab
React->>CM: componentDidMount / componentDidUpdate
CM->>CM: _getDocKey() → derive cache key
CM->>Cache: getOrCreateDoc(key, mode, readOnly)
alt Doc exists in cache
Cache-->>CM: return cached Doc
else Doc not in cache
Cache->>Editor: new CodeMirror.Doc(value, mode)
Editor-->>Cache: Doc instance
Cache-->>CM: return new Doc
end
CM->>Editor: editor.swapDoc(cachedDoc)
Editor-->>CM: old doc detached, new doc active
CM->>Editor: reapply overlay & lint options
end
rect rgba(0, 200, 100, 0.5)
Note over User,Editor: Value Update (Same Tab)
User->>React: props.value changes
React->>CM: componentDidUpdate (same docKey)
CM->>Editor: editor.setValue(newValue)
Editor-->>CM: doc content updated
end
rect rgba(200, 100, 0, 0.5)
Note over User,Editor: Unmount
User->>React: Unmount component
React->>CM: componentWillUnmount
CM->>Editor: editor.swapDoc(new empty Doc)
Editor-->>CM: cached doc detached safely
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/CodeEditor/index.js`:
- Line 44: docCache currently holds CodeMirror.Doc objects forever (const
docCache = new Map()), leaking content, markers, and undo history; add an
explicit eviction/invalidation strategy: either implement a bounded LRU or TTL
for entries in docCache, and expose a removal call that your tab/collection
lifecycle can call when a tab is closed (e.g., add a removeDoc(key) or
clearDocsForCollection(collectionId) function and call it from the
tab-close/unmount handler). Ensure eviction cleans up the underlying
CodeMirror.Doc (call its .toTextArea() / .clearHistory() or appropriate dispose
API) before deleting the Map entry so markers/undo stacks are released.
Reference docCache and the tab/collection unmount/close handlers when wiring
this up.
- Around line 337-338: The code is converting null/undefined into the literal
strings "null"/"undefined" when setting this.cachedValue and calling
this.editor.setValue; change both uses to coalesce this.props.value to an empty
string before stringifying (i.e., use this.props.value ?? '' or
this?.props?.value ?? '' as the input to String) so cachedValue and
editor.setValue never receive the text "null" or "undefined" — update the
references to cachedValue and the call to this.editor.setValue accordingly.
- Around line 46-55: getOrCreateDoc mutates a cached Doc (doc.setValue(content))
before checking whether that Doc is attached to another editor, which can
trigger change handlers on the wrong editor; change getOrCreateDoc signature to
accept the current editor instance, and inside getOrCreateDoc check doc.cm (or
doc.cm !== undefined) before calling doc.setValue — if doc.cm is set (attached
elsewhere) create a fresh Doc instead of mutating the cached one and update
docCache accordingly; update all call sites (including the other occurrences
mentioned) to pass the current editor so the defensive attachment check happens
before any mutation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7aa22db5-759e-45e2-b071-1779a4040c92
📒 Files selected for processing (4)
packages/bruno-app/src/components/CodeEditor/index.jspackages/bruno-app/src/components/CollectionSettings/Script/index.jspackages/bruno-app/src/components/FolderSettings/Script/index.jspackages/bruno-app/src/components/RequestPane/Script/index.js
| * Constraint: a Doc can be attached to only one editor at a time (CM5 enforces | ||
| * this via `doc.cm`). See componentWillUnmount for how we release the Doc. | ||
| */ | ||
| const docCache = new Map(); |
Contributor
There was a problem hiding this comment.
Add an eviction path for cached docs.
docCache keeps every CodeMirror.Doc indefinitely, and unmount intentionally leaves content, markers, and undo history cached. Long sessions that open many requests or large responses can retain a lot of memory. Please add an explicit invalidation path tied to tab/collection close, or a bounded LRU/TTL policy.
Also applies to: 395-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/CodeEditor/index.js` at line 44, docCache
currently holds CodeMirror.Doc objects forever (const docCache = new Map()),
leaking content, markers, and undo history; add an explicit
eviction/invalidation strategy: either implement a bounded LRU or TTL for
entries in docCache, and expose a removal call that your tab/collection
lifecycle can call when a tab is closed (e.g., add a removeDoc(key) or
clearDocsForCollection(collectionId) function and call it from the
tab-close/unmount handler). Ensure eviction cleans up the underlying
CodeMirror.Doc (call its .toTextArea() / .clearHistory() or appropriate dispose
API) before deleting the Map entry so markers/undo stacks are released.
Reference docCache and the tab/collection unmount/close handlers when wiring
this up.
Comment on lines
+46
to
+55
| const getOrCreateDoc = (key, content, mode) => { | ||
| let doc = docCache.get(key); | ||
| if (doc) { | ||
| // The cached Doc may have stale content if props.value changed while this | ||
| // tab was inactive (e.g. a new response arrived). Sync the content so the | ||
| // user sees the latest value. This does reset fold state on this Doc, but | ||
| // that's correct — fold positions for old content are meaningless. | ||
| if (doc.getValue() !== content) { | ||
| doc.setValue(content); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "index.js" | grep -i "codeeditor"Repository: usebruno/bruno
Length of output: 286
🏁 Script executed:
cat -n packages/bruno-app/src/components/CodeEditor/index.js | head -80Repository: usebruno/bruno
Length of output: 3651
🏁 Script executed:
cat -n packages/bruno-app/src/components/CodeEditor/index.js | sed -n '40,60p'Repository: usebruno/bruno
Length of output: 974
🏁 Script executed:
cat -n packages/bruno-app/src/components/CodeEditor/index.js | sed -n '230,260p'Repository: usebruno/bruno
Length of output: 1650
🏁 Script executed:
cat -n packages/bruno-app/src/components/CodeEditor/index.js | sed -n '310,330p'Repository: usebruno/bruno
Length of output: 1237
🏁 Script executed:
wc -l packages/bruno-app/src/components/CodeEditor/index.jsRepository: usebruno/bruno
Length of output: 115
Move the doc.cm attachment check into getOrCreateDoc before mutating cached content.
The function calls doc.setValue(content) before checking whether that Doc is still attached to another editor. If the cached Doc is in use elsewhere, this mutation fires change handlers on the wrong editor and corrupts its state. The defensive fallback check only happens after getOrCreateDoc returns — too late to prevent the damage.
Pass the current editor to getOrCreateDoc and check doc.cm before calling setValue. If the Doc is attached elsewhere, create a fresh Doc instead of mutating the cached one.
Proposed fix
-const getOrCreateDoc = (key, content, mode) => {
+const getOrCreateDoc = (key, content, mode, editor) => {
let doc = docCache.get(key);
if (doc) {
+ if (doc.cm && doc.cm !== editor) {
+ doc = new CodeMirror.Doc(content, mode);
+ docCache.set(key, doc);
+ return doc;
+ }
+
// The cached Doc may have stale content if props.value changed while this
// tab was inactive (e.g. a new response arrived). Sync the content so the
// user sees the latest value. This does reset fold state on this Doc, but
// that's correct — fold positions for old content are meaningless.
if (doc.getValue() !== content) {
@@
let doc = getOrCreateDoc(
docKey,
this.props.value || '',
- this.props.mode || 'application/ld+json'
+ this.props.mode || 'application/ld+json',
+ editor
);
- // Defensive fallback: a CM5 Doc can only be attached to one editor at a
- // time. If the cached Doc is still attached to a previous (dead) editor —
- // e.g. React StrictMode double-mounting, or an unmount that skipped our
- // release logic — swapDoc would throw "document already in use". Replace
- // the cache entry with a fresh Doc in that case.
- if (doc.cm && doc.cm !== editor) {
- doc = new CodeMirror.Doc(this.props.value || '', this.props.mode || 'application/ld+json');
- docCache.set(docKey, doc);
- }
@@
let doc = getOrCreateDoc(
newDocKey,
this.props.value || '',
- this.props.mode || 'application/ld+json'
+ this.props.mode || 'application/ld+json',
+ this.editor
);
- // Same defensive fallback as componentDidMount — see there for why.
- if (doc.cm && doc.cm !== this.editor) {
- doc = new CodeMirror.Doc(this.props.value || '', this.props.mode || 'application/ld+json');
- docCache.set(newDocKey, doc);
- }Also applies to: 246-249, 317-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/CodeEditor/index.js` around lines 46 - 55,
getOrCreateDoc mutates a cached Doc (doc.setValue(content)) before checking
whether that Doc is attached to another editor, which can trigger change
handlers on the wrong editor; change getOrCreateDoc signature to accept the
current editor instance, and inside getOrCreateDoc check doc.cm (or doc.cm !==
undefined) before calling doc.setValue — if doc.cm is set (attached elsewhere)
create a fresh Doc instead of mutating the cached one and update docCache
accordingly; update all call sites (including the other occurrences mentioned)
to pass the current editor so the defensive attachment check happens before any
mutation.
Comment on lines
+337
to
+338
| this.cachedValue = String(this?.props?.value ?? ''); | ||
| this.editor.setValue(String(this.props.value) || ''); |
Contributor
There was a problem hiding this comment.
Avoid rendering nullish values as text.
Line 338 uses String(this.props.value) || '', so undefined becomes 'undefined' and null becomes 'null'. Coalesce before stringifying.
Proposed fix
- this.cachedValue = String(this?.props?.value ?? '');
- this.editor.setValue(String(this.props.value) || '');
+ const nextValue = String(this.props.value ?? '');
+ this.cachedValue = nextValue;
+ this.editor.setValue(nextValue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/CodeEditor/index.js` around lines 337 -
338, The code is converting null/undefined into the literal strings
"null"/"undefined" when setting this.cachedValue and calling
this.editor.setValue; change both uses to coalesce this.props.value to an empty
string before stringifying (i.e., use this.props.value ?? '' or
this?.props?.value ?? '' as the input to String) so cachedValue and
editor.setValue never receive the text "null" or "undefined" — update the
references to cachedValue and the call to this.editor.setValue accordingly.
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.
Description
Problem
Every tab switch called
editor.setValue(newContent), which mutates the underlying CodeMirrorDocand destroys all of its attached state — folds expand back, cursor resets, selection clears, undo history is wiped, and scroll jumps to the top. For anyone working with large responses or long scripts, re-folding and re-scrolling on every tab switch was a constant papercut.Solution
Switch to CM5's native multi-document API,
editor.swapDoc(doc). Each tab now owns its ownDocin a module-level cache:swapDocto that tab's cachedDoc→ folds, cursor, selection, undo history, and scroll position are all restored automatically.setValueon the currentDoc(fold state resets, which is correct — positions no longer mean anything).Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit