feat: persist scroll across tabs#7695
Open
shubh-bruno wants to merge 24 commits intousebruno:mainfrom
Open
Conversation
added 9 commits
April 1, 2026 14:05
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughReplaces Redux-driven tab/editor scroll persistence with localStorage-backed hooks ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component (UI)
participant HookC as usePersistedContainerScroll
participant HookE as usePersistedEditorScroll
participant Storage as localStorage
participant DOM as DOM / CodeMirror
UI->>HookC: mount with (wrapperRef, selector, key, enabled)
HookC->>Storage: read persisted value
Storage-->>HookC: parsed scrollTop
HookC->>DOM: set scrollTop
UI->>HookE: mount with (editorRef, key)
HookE->>Storage: read persisted editor scroll
Storage-->>HookE: parsed scroll
HookE-->>UI: return initialScroll
UI->>DOM: init editor with initialScroll
User->>DOM: scrolls
DOM->>HookC: scroll event
HookC->>HookC: debounce then write
HookC->>Storage: write scrollTop
DOM->>HookE: editor scroll event
HookE->>HookE: debounce then setScroll()
HookE->>Storage: write scroll
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (1)
3160-3202:⚠️ Potential issue | 🟠 MajorProper cleanup on tab close — but consider clearing scroll state on new request.
The
clearPersistedScope(tabUid)call correctly removes persisted localStorage entries when a tab closes.However, per the
initRunRequestEventreducer (Line 2934-2949 incollections/index.js), response state fields are reset when a new request is sent, but the persisted scroll keys (persisted::<tabUid>::response-scroll-*) are not cleared. This means when a new response arrives, the response pane may restore stale scroll position from the previous request instead of starting atscrollY: 0.Consider clearing the response scroll key in
sendRequestafterinitRunRequestEventdispatches:import { clearPersistedScope } from 'hooks/usePersistedState/PersistedScopeProvider'; // Inside sendRequest, after initRunRequestEvent dispatch: // Clear response pane scroll so new response starts at top const responseScrollKey = `persisted::${state.tabs.activeTabUid}::response-scroll-${itemUid}`; localStorage.removeItem(responseScrollKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js` around lines 3160 - 3202, The initRunRequestEvent reducer resets response fields but doesn't clear persisted response-scroll keys so new responses can restore previous scroll; in sendRequest (after dispatching initRunRequestEvent) remove the persisted response scroll entry by constructing the key `persisted::<activeTabUid>::response-scroll-<itemUid>` (use state.tabs.activeTabUid and the request's itemUid/tabUid) and call localStorage.removeItem(key) (or reuse clearPersistedScope if it supports that exact key) to ensure the response pane scroll is reset to top when a new request is sent.packages/bruno-app/src/components/RequestPane/Script/index.js (1)
44-56:⚠️ Potential issue | 🟡 MinorMissing dependencies in
useEffectmay cause stale scroll values.
preReqScrollandpostResScrollare used inside the effect but not listed in the dependency array. This could restore outdated scroll positions if those values change whileactiveTabremains the same.Proposed fix
return () => clearTimeout(timer); - }, [activeTab]); + }, [activeTab, preReqScroll, postResScroll]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Script/index.js` around lines 44 - 56, The effect in useEffect uses preReqScroll and postResScroll but only depends on activeTab, risking stale values; update the dependency array for the effect to include preReqScroll and postResScroll (and keep activeTab) so the refresh/scroll logic in the effect runs when those scroll values change, referencing the existing preRequestEditorRef and postResponseEditorRef editor accesses inside the effect; alternatively, if you want to avoid re-running for every scroll change, read the latest scroll values via refs inside the effect callback and keep the dependency array as [activeTab]—but ensure either approach is applied to eliminate stale scroll restoration.packages/bruno-app/src/components/FolderSettings/Script/index.js (1)
43-61:⚠️ Potential issue | 🟠 MajorFast tab switches can still restore an older editor position.
usePersistedEditorScroll()saves on a 200ms debounce, but this effect restores immediately frompreReqScroll/postResScrolland only reruns onactiveTab. If I scroll and flip tabs inside that debounce window,refresh()ends up reapplying the previous saved position instead of the one I just left at.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/FolderSettings/Script/index.js` around lines 43 - 61, The effect is restoring stale persisted scroll (preReqScroll/postResScroll) while the editor's latest position hasn't been flushed by usePersistedEditorScroll's 200ms debounce; update the effect to prefer the live editor scroll when available: when activeTab switches, if preRequestEditorRef.current?.editor (or postResponseEditorRef.current?.editor) exists, call the editor API to read its current scroll (e.g. editor.getScrollInfo().top or equivalent) and use that value for editor.scrollTo(null, ...) after refresh, falling back to preReqScroll/postResScroll only if the live read is unavailable; reference usePersistedEditorScroll, preRequestEditorRef, postResponseEditorRef, preReqScroll, postResScroll and activeTab to locate where to change the logic.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/RequestPane/GrpcBody/index.js (1)
77-77: Index-based persistence key may cause scroll misalignment after message deletion.When a message is removed, subsequent messages shift indices, causing their persisted scroll states to become misaligned. Consider using a stable message identifier if available, or accept this as a known limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/GrpcBody/index.js` at line 77, The persisted scroll key currently uses a volatile index which will misalign scroll state when messages are deleted; update the key passed to usePersistedEditorScroll (the grpcScroll declaration) to use a stable identifier (e.g., a message-level unique id like message.uid or a persistent id on item/messages) instead of the numeric index (replace `request-grpc-msg-scroll-${item.uid}-${index}` with a key that includes the stable message id), or if no stable id exists document/accept the limitation in the code comment where grpcScroll and usePersistedEditorScroll are used.packages/bruno-app/src/hooks/usePersistedState/PersistedScopeProvider.tsx (1)
2-2: Use single quotes for import path.Per coding guidelines, stick to single quotes for strings.
🔧 Suggested fix
-import * as React from "react" +import * as React from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/hooks/usePersistedState/PersistedScopeProvider.tsx` at line 2, Change the import string to use single quotes: update the module import line (the React import at the top of PersistedScopeProvider.tsx) so the path uses single quotes instead of double quotes (i.e., replace the double-quoted import string in the statement importing React).packages/bruno-app/src/hooks/usePersistedState/index.ts (1)
29-43: Missingoptions.defaultin useEffect dependency array.The effect depends on
options.defaultwhen resetting state (line 42), but it's not in the dependency array. If the consumer passes a different default while the key stays the same, the effect won't re-run. This is likely intentional (only react to key changes), but worth confirming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/hooks/usePersistedState/index.ts` around lines 29 - 43, The useEffect that loads/reset state (function using prevKeyRef, storageKey, setState) reads options.default when falling back but doesn't include it in the dependency array; update the effect dependencies to include options.default (i.e., [storageKey, options.default]) so the effect will re-run when the default value changes, or alternatively document/ensure the omission is intentional if you want the effect to only react to storageKey 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 `@packages/bruno-app/src/components/Documentation/index.js`:
- Around line 25-37: readScroll can return a stale value because
usePersistedContainerScroll only updates localStorage during effect cleanup; fix
by reading the live DOM scroll when available: inside readScroll, if
wrapperRef.current exists use wrapperRef.current.scrollTop (coerce to number) as
the source of truth, otherwise fallback to reading/parsing localStorage via the
existing logic; mention wrapperRef, readScroll, usePersistedContainerScroll and
CodeEditor so the change is applied where the editor mounts to avoid restoring a
stale position.
In `@packages/bruno-app/src/components/FolderSettings/Documentation/index.js`:
- Around line 21-32: The render-time read of localStorage in readScroll (used
when switching into edit mode) causes a stale-scroll race; move the read out of
render and instead obtain the initial scroll value when edit mode is entered
(e.g., in a useEffect that runs when isEditing becomes true) so the editor uses
the most recent persisted position from usePersistedContainerScroll's cleanup.
Locate usePersistedContainerScroll, wrapperRef and storageKey and change
readScroll so it is invoked lazily in an effect (or via a ref populated by the
preview hook’s cleanup callback) rather than during render; apply the same
change for the similar code around the other instance noted (lines 72-73).
In
`@packages/bruno-app/src/components/RequestPane/RequestBody/RequestBodyMode/index.js`:
- Around line 27-28: In RequestBodyMode update the label for the option with id
'formUrlEncoded' from "Form URL Encoded - from" to "Form URL Encoded - form";
locate the options array (used by the RequestBodyMode component) containing the
entries with id 'multipartForm' and 'formUrlEncoded' and correct the typo in the
label string for 'formUrlEncoded'.
In
`@packages/bruno-app/src/hooks/usePersistedState/usePersistedContainerScroll.ts`:
- Around line 55-57: Wrap the localStorage.setItem calls in
usePersistedContainerScroll with a try/catch to prevent runtime exceptions from
bubbling up during scroll/unmount; specifically, guard the two writes that use
storageKey and scrollPosRef.current (the setTimeout callback that assigns
JSON.stringify(scrollPosRef.current) and the other write at the later location)
and swallow or log errors via console.warn/processLogger instead of letting them
throw, keeping existing behavior (clearing saveTimeout.current etc.) unchanged.
- Line 27: The storage key construction currently falls back to raw key when
scope is falsy, allowing unscoped entries to collide across tabs; change the
storageKey logic in usePersistedContainerScroll (replace the ternary at
storageKey) to always include a stable prefix and an explicit scope token (e.g.,
`persisted::${scope ?? 'unscoped'}::${key}`) instead of returning key when scope
is empty so tab-level isolation and clearPersistedScope(tabUid) semantics are
preserved; update any references that rely on storageKey accordingly.
---
Outside diff comments:
In `@packages/bruno-app/src/components/FolderSettings/Script/index.js`:
- Around line 43-61: The effect is restoring stale persisted scroll
(preReqScroll/postResScroll) while the editor's latest position hasn't been
flushed by usePersistedEditorScroll's 200ms debounce; update the effect to
prefer the live editor scroll when available: when activeTab switches, if
preRequestEditorRef.current?.editor (or postResponseEditorRef.current?.editor)
exists, call the editor API to read its current scroll (e.g.
editor.getScrollInfo().top or equivalent) and use that value for
editor.scrollTo(null, ...) after refresh, falling back to
preReqScroll/postResScroll only if the live read is unavailable; reference
usePersistedEditorScroll, preRequestEditorRef, postResponseEditorRef,
preReqScroll, postResScroll and activeTab to locate where to change the logic.
In `@packages/bruno-app/src/components/RequestPane/Script/index.js`:
- Around line 44-56: The effect in useEffect uses preReqScroll and postResScroll
but only depends on activeTab, risking stale values; update the dependency array
for the effect to include preReqScroll and postResScroll (and keep activeTab) so
the refresh/scroll logic in the effect runs when those scroll values change,
referencing the existing preRequestEditorRef and postResponseEditorRef editor
accesses inside the effect; alternatively, if you want to avoid re-running for
every scroll change, read the latest scroll values via refs inside the effect
callback and keep the dependency array as [activeTab]—but ensure either approach
is applied to eliminate stale scroll restoration.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 3160-3202: The initRunRequestEvent reducer resets response fields
but doesn't clear persisted response-scroll keys so new responses can restore
previous scroll; in sendRequest (after dispatching initRunRequestEvent) remove
the persisted response scroll entry by constructing the key
`persisted::<activeTabUid>::response-scroll-<itemUid>` (use
state.tabs.activeTabUid and the request's itemUid/tabUid) and call
localStorage.removeItem(key) (or reuse clearPersistedScope if it supports that
exact key) to ensure the response pane scroll is reset to top when a new request
is sent.
---
Nitpick comments:
In `@packages/bruno-app/src/components/RequestPane/GrpcBody/index.js`:
- Line 77: The persisted scroll key currently uses a volatile index which will
misalign scroll state when messages are deleted; update the key passed to
usePersistedEditorScroll (the grpcScroll declaration) to use a stable identifier
(e.g., a message-level unique id like message.uid or a persistent id on
item/messages) instead of the numeric index (replace
`request-grpc-msg-scroll-${item.uid}-${index}` with a key that includes the
stable message id), or if no stable id exists document/accept the limitation in
the code comment where grpcScroll and usePersistedEditorScroll are used.
In `@packages/bruno-app/src/hooks/usePersistedState/index.ts`:
- Around line 29-43: The useEffect that loads/reset state (function using
prevKeyRef, storageKey, setState) reads options.default when falling back but
doesn't include it in the dependency array; update the effect dependencies to
include options.default (i.e., [storageKey, options.default]) so the effect will
re-run when the default value changes, or alternatively document/ensure the
omission is intentional if you want the effect to only react to storageKey
changes.
In `@packages/bruno-app/src/hooks/usePersistedState/PersistedScopeProvider.tsx`:
- Line 2: Change the import string to use single quotes: update the module
import line (the React import at the top of PersistedScopeProvider.tsx) so the
path uses single quotes instead of double quotes (i.e., replace the
double-quoted import string in the statement importing React).
🪄 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: bdb4c029-626f-4580-aca0-dc04f7e10e01
📒 Files selected for processing (37)
packages/bruno-app/src/components/BodyModeSelector/index.jspackages/bruno-app/src/components/Documentation/index.jspackages/bruno-app/src/components/EditableTable/StyledWrapper.jspackages/bruno-app/src/components/FolderSettings/Documentation/index.jspackages/bruno-app/src/components/FolderSettings/Headers/StyledWrapper.jspackages/bruno-app/src/components/FolderSettings/Headers/index.jspackages/bruno-app/src/components/FolderSettings/Script/index.jspackages/bruno-app/src/components/FolderSettings/Tests/index.jspackages/bruno-app/src/components/FolderSettings/Vars/index.jspackages/bruno-app/src/components/FolderSettings/index.jspackages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.jspackages/bruno-app/src/components/RequestPane/GrpcBody/index.jspackages/bruno-app/src/components/RequestPane/HttpRequestPane/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/RequestPane/QueryParams/index.jspackages/bruno-app/src/components/RequestPane/RequestBody/RequestBodyMode/index.jspackages/bruno-app/src/components/RequestPane/RequestBody/index.jspackages/bruno-app/src/components/RequestPane/RequestHeaders/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/RequestHeaders/index.jspackages/bruno-app/src/components/RequestPane/Script/index.jspackages/bruno-app/src/components/RequestPane/Tests/index.jspackages/bruno-app/src/components/RequestPane/Vars/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/ResponsePane/GrpcResponsePane/StyledWrapper.jspackages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.jspackages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/index.jspackages/bruno-app/src/components/ResponsePane/ResponseHeaders/index.jspackages/bruno-app/src/components/ResponsePane/StyledWrapper.jspackages/bruno-app/src/components/ResponsePane/TestResults/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/index.jspackages/bruno-app/src/components/ResponsePane/index.jspackages/bruno-app/src/hooks/usePersistedState/PersistedScopeProvider.tsxpackages/bruno-app/src/hooks/usePersistedState/index.tspackages/bruno-app/src/hooks/usePersistedState/usePersistedContainerScroll.tspackages/bruno-app/src/hooks/usePersistedState/usePersistedEditorScroll.tspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.js
💤 Files with no reviewable changes (4)
- packages/bruno-app/src/components/BodyModeSelector/index.js
- packages/bruno-app/src/components/EditableTable/StyledWrapper.js
- packages/bruno-app/src/components/RequestPane/RequestHeaders/StyledWrapper.js
- packages/bruno-app/src/providers/ReduxStore/slices/tabs.js
Comment on lines
+25
to
+37
| // Scroll persistence for both edit (CodeMirror) and preview (Markdown) modes using one shared key. | ||
| // Preview mode: hook tracks .flex-boundary scroll (enabled only when not editing). | ||
| // Edit mode: CodeEditor's onScroll/initialScroll props write/read the same localStorage key. | ||
| const wrapperRef = useRef(null); | ||
| // Pass null selector — wrapperRef itself is the scrollable container (has overflow-y: auto) | ||
| const storageKey = usePersistedContainerScroll(wrapperRef, null, `request-docs-scroll-${item.uid}`, !isEditing); | ||
|
|
||
| const readScroll = () => { | ||
| try { | ||
| const raw = localStorage.getItem(storageKey); | ||
| return raw !== null ? JSON.parse(raw) || 0 : 0; | ||
| } catch { return 0; } | ||
| }; |
Contributor
There was a problem hiding this comment.
Preview → Edit can mount the editor with a stale scroll.
readScroll() runs during the render that mounts CodeEditor, but usePersistedContainerScroll() only flushes the latest preview scrollTop in its effect cleanup. On a quick scroll-then-toggle, that cleanup happens after this render, so edit mode restores the previous position instead of the one the user just left.
Also applies to: 75-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Documentation/index.js` around lines 25 -
37, readScroll can return a stale value because usePersistedContainerScroll only
updates localStorage during effect cleanup; fix by reading the live DOM scroll
when available: inside readScroll, if wrapperRef.current exists use
wrapperRef.current.scrollTop (coerce to number) as the source of truth,
otherwise fallback to reading/parsing localStorage via the existing logic;
mention wrapperRef, readScroll, usePersistedContainerScroll and CodeEditor so
the change is applied where the editor mounts to avoid restoring a stale
position.
Comment on lines
+21
to
+32
| // Scroll persistence for both edit (CodeMirror) and preview (Markdown) modes using one shared key. | ||
| // Preview mode: hook tracks .folder-settings-content scroll (enabled only when not editing). | ||
| // Edit mode: CodeEditor's onScroll/initialScroll props write/read the same localStorage key. | ||
| const wrapperRef = useRef(null); | ||
| const storageKey = usePersistedContainerScroll(wrapperRef, '.folder-settings-content', `folder-docs-scroll-${folder.uid}`, !isEditing); | ||
|
|
||
| const readScroll = () => { | ||
| try { | ||
| const raw = localStorage.getItem(storageKey); | ||
| return raw !== null ? JSON.parse(raw) || 0 : 0; | ||
| } catch { return 0; } | ||
| }; |
Contributor
There was a problem hiding this comment.
Folder docs preview has the same stale-scroll race.
This path also reads from localStorage during the render that switches into edit mode, while the preview scroll hook persists its latest position in cleanup. If the user scrolls and immediately clicks Edit, the editor can reopen at the older saved offset.
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/FolderSettings/Documentation/index.js`
around lines 21 - 32, The render-time read of localStorage in readScroll (used
when switching into edit mode) causes a stale-scroll race; move the read out of
render and instead obtain the initial scroll value when edit mode is entered
(e.g., in a useEffect that runs when isEditing becomes true) so the editor uses
the most recent persisted position from usePersistedContainerScroll's cleanup.
Locate usePersistedContainerScroll, wrapperRef and storageKey and change
readScroll so it is invoked lazily in an effect (or via a ref populated by the
preview hook’s cleanup callback) rather than during render; apply the same
change for the similar code around the other instance noted (lines 72-73).
Comment on lines
+27
to
+28
| { id: 'multipartForm', label: 'Multipart Form - form', leftSection: IconForms }, | ||
| { id: 'formUrlEncoded', label: 'Form URL Encoded - from', leftSection: IconForms } |
Contributor
There was a problem hiding this comment.
Typo: "from" should be "form".
Line 28 has 'Form URL Encoded - from' — this appears to be a typo.
- { id: 'formUrlEncoded', label: 'Form URL Encoded - from', leftSection: IconForms }
+ { id: 'formUrlEncoded', label: 'Form URL Encoded - form', leftSection: IconForms }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { id: 'multipartForm', label: 'Multipart Form - form', leftSection: IconForms }, | |
| { id: 'formUrlEncoded', label: 'Form URL Encoded - from', leftSection: IconForms } | |
| { id: 'multipartForm', label: 'Multipart Form - form', leftSection: IconForms }, | |
| { id: 'formUrlEncoded', label: 'Form URL Encoded - form', leftSection: IconForms } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/components/RequestPane/RequestBody/RequestBodyMode/index.js`
around lines 27 - 28, In RequestBodyMode update the label for the option with id
'formUrlEncoded' from "Form URL Encoded - from" to "Form URL Encoded - form";
locate the options array (used by the RequestBodyMode component) containing the
entries with id 'multipartForm' and 'formUrlEncoded' and correct the typo in the
label string for 'formUrlEncoded'.
| enabled: boolean = true | ||
| ): string { | ||
| const scope = usePersistenceScope(); | ||
| const storageKey = scope ? `persisted::${scope}::${key}` : key; |
Contributor
There was a problem hiding this comment.
Scope fallback at Line 27 breaks tab-level isolation.
Falling back to raw key when scope is empty creates unscoped entries. In call sites outside ScopedPersistedContext (e.g., packages/bruno-app/src/components/ResponsePane/Timeline/index.js, Line 47-49), same-UID tabs can overwrite each other and bypass clearPersistedScope(tabUid) semantics.
Suggested fix
- const storageKey = scope ? `persisted::${scope}::${key}` : key;
+ const storageKey = scope ? `persisted::${scope}::${key}` : '';
...
useEffect(() => {
+ if (!storageKey) return;
if (!enabled) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/hooks/usePersistedState/usePersistedContainerScroll.ts`
at line 27, The storage key construction currently falls back to raw key when
scope is falsy, allowing unscoped entries to collide across tabs; change the
storageKey logic in usePersistedContainerScroll (replace the ternary at
storageKey) to always include a stable prefix and an explicit scope token (e.g.,
`persisted::${scope ?? 'unscoped'}::${key}`) instead of returning key when scope
is empty so tab-level isolation and clearPersistedScope(tabUid) semantics are
preserved; update any references that rely on storageKey accordingly.
Comment on lines
+55
to
+57
| saveTimeout.current = setTimeout(() => { | ||
| localStorage.setItem(storageKey, JSON.stringify(scrollPosRef.current)); | ||
| }, 200); |
Contributor
There was a problem hiding this comment.
Guard localStorage.setItem writes from runtime failures.
Writes at Line 56 and Line 64 are unguarded. setItem can throw (quota/security/storage-disabled), which can surface from scroll/unmount paths.
Suggested fix
+ const persistScroll = () => {
+ try {
+ localStorage.setItem(storageKey, JSON.stringify(scrollPosRef.current));
+ } catch {}
+ };
+
const onScroll = () => {
scrollPosRef.current = el.scrollTop;
if (saveTimeout.current) clearTimeout(saveTimeout.current);
saveTimeout.current = setTimeout(() => {
- localStorage.setItem(storageKey, JSON.stringify(scrollPosRef.current));
+ persistScroll();
}, 200);
};
...
return () => {
el.removeEventListener('scroll', onScroll);
if (saveTimeout.current) clearTimeout(saveTimeout.current);
- localStorage.setItem(storageKey, JSON.stringify(scrollPosRef.current));
+ persistScroll();
};Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/hooks/usePersistedState/usePersistedContainerScroll.ts`
around lines 55 - 57, Wrap the localStorage.setItem calls in
usePersistedContainerScroll with a try/catch to prevent runtime exceptions from
bubbling up during scroll/unmount; specifically, guard the two writes that use
storageKey and scrollPosRef.current (the setTimeout callback that assigns
JSON.stringify(scrollPosRef.current) and the other write at the later location)
and swallow or log errors via console.warn/processLogger instead of letting them
throw, keeping existing behavior (clearing saveTimeout.current etc.) unchanged.
Contributor
There was a problem hiding this comment.
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/CollectionSettings/Script/index.js (1)
48-60:⚠️ Potential issue | 🟡 MinorAdd missing dependencies to useEffect.
The
preReqScrollandpostResScrollvalues are used inside the useEffect but missing from the dependency array. These are dynamic values that update when the user scrolls. Add them to the dependency array to ensure the effect uses the latest scroll positions when the tab switches.}, [activeTab, preReqScroll, postResScroll]);Note: If this causes unnecessary re-runs due to scroll updates, consider extracting the refresh logic or using a ref to store the current scroll positions instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/CollectionSettings/Script/index.js` around lines 48 - 60, The effect watching activeTab uses preReqScroll and postResScroll but they are missing from the dependency array; update the useEffect dependencies to include preReqScroll and postResScroll so the effect sees latest scroll values when switching tabs (affecting preRequestEditorRef.current.editor.scrollTo and postResponseEditorRef.current.editor.scrollTo), or if that causes noisy reruns move the scroll values to refs and reference those inside the useEffect instead.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/CollectionSettings/Headers/index.js (1)
29-30: Minor: Scroll persistence inactive during bulk-edit mode.The
wrapperRefis only attached in the non-bulk-edit render path (line 126). WhenisBulkEditModeis true, the ref remainsnulland scroll won't be persisted. The hook handles this gracefully (returns early when element is null), but users switching to bulk-edit and back will lose their scroll position.If this is intentional, consider passing
enabled: !isBulkEditModeas the 4th argument to make the intent explicit:- usePersistedContainerScroll(wrapperRef, '.collection-settings-content', `collection-headers-scroll-${collection.uid}`); + usePersistedContainerScroll(wrapperRef, '.collection-settings-content', `collection-headers-scroll-${collection.uid}`, !isBulkEditMode);Also applies to: 109-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/CollectionSettings/Headers/index.js` around lines 29 - 30, The scroll persistence hook is not active during bulk-edit because wrapperRef is null in that render path; update the usePersistedContainerScroll call (referencing wrapperRef, usePersistedContainerScroll, isBulkEditMode, and collection.uid) to explicitly disable persistence when bulk-editing by passing an options/enabled fourth argument set to !isBulkEditMode (or alternatively ensure wrapperRef is attached in both render paths) so scroll position is preserved when toggling bulk-edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/bruno-app/src/components/CollectionSettings/Script/index.js`:
- Around line 48-60: The effect watching activeTab uses preReqScroll and
postResScroll but they are missing from the dependency array; update the
useEffect dependencies to include preReqScroll and postResScroll so the effect
sees latest scroll values when switching tabs (affecting
preRequestEditorRef.current.editor.scrollTo and
postResponseEditorRef.current.editor.scrollTo), or if that causes noisy reruns
move the scroll values to refs and reference those inside the useEffect instead.
---
Nitpick comments:
In `@packages/bruno-app/src/components/CollectionSettings/Headers/index.js`:
- Around line 29-30: The scroll persistence hook is not active during bulk-edit
because wrapperRef is null in that render path; update the
usePersistedContainerScroll call (referencing wrapperRef,
usePersistedContainerScroll, isBulkEditMode, and collection.uid) to explicitly
disable persistence when bulk-editing by passing an options/enabled fourth
argument set to !isBulkEditMode (or alternatively ensure wrapperRef is attached
in both render paths) so scroll position is preserved when toggling bulk-edit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 369deeea-5eee-4be2-87d7-64c0b99be328
📒 Files selected for processing (12)
packages/bruno-app/src/components/CollectionSettings/Docs/index.jspackages/bruno-app/src/components/CollectionSettings/Headers/index.jspackages/bruno-app/src/components/CollectionSettings/Script/index.jspackages/bruno-app/src/components/CollectionSettings/Tests/index.jspackages/bruno-app/src/components/CollectionSettings/Vars/index.jspackages/bruno-app/src/components/CollectionSettings/index.jspackages/bruno-app/src/components/RequestPane/Assertions/index.jspackages/bruno-app/src/components/RequestPane/HttpRequestPane/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.js
💤 Files with no reviewable changes (1)
- packages/bruno-app/src/providers/ReduxStore/slices/tabs.js
✅ Files skipped from review due to trivial changes (3)
- packages/bruno-app/src/components/CollectionSettings/index.js
- packages/bruno-app/src/components/RequestPane/HttpRequestPane/index.js
- packages/bruno-app/src/components/RequestTabPanel/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js
- packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js
added 2 commits
April 7, 2026 19:25
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/request/body-scroll/scroll-persistent.spec.ts (1)
46-77: Replace the fixed sleeps in the shared helpers with state-based waits.These helpers bake in multiple
waitForTimeout()calls, so the whole suite becomes slower and more brittle under Electron CI. Poll the observable state instead of sleeping for guessed durations.As per coding guidelines, "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
Also applies to: 80-100
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/request/body-scroll/scroll-persistent.spec.ts`:
- Around line 818-834: The test step "Add 50 headers via Bulk Edit" opens the
bulk editor but never switches back to key-value mode: after obtaining the
locator for 'key-value-edit-toggle' (the element referenced by
page.getByTestId('key-value-edit-toggle')), await and click that toggle to
switch back into the key-value table view (optionally wait for the table or a
specific key-value row to appear) so the rest of the test validates the long
headers table instead of staying in the bulk editor; update the test.step to
perform an awaited click on that locator (and a short wait/assert for the table
if flaky).
- Around line 108-112: The current expectScrollRestored helper
(expectScrollRestored(restored: number, original: number)) only asserts restored
> 0 which is too weak; change it to compare restored against original using the
existing SCROLL_TOLERANCE constant (i.e., assert Math.abs(restored - original) <
SCROLL_TOLERANCE) so the test fails when the restored position deviates beyond
the acceptable tolerance, and remove the console.log; keep the non-zero check
only if you think it's still needed but the tolerance comparison must be the
primary assertion.
🪄 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: 1970f8a8-53bc-4935-a451-a8d87e16a527
📒 Files selected for processing (12)
packages/bruno-app/src/components/BulkEditor/index.jspackages/bruno-app/src/components/CollectionSettings/Headers/index.jspackages/bruno-app/src/components/CollectionSettings/Script/index.jspackages/bruno-app/src/components/FolderSettings/Headers/index.jspackages/bruno-app/src/components/FolderSettings/Script/index.jspackages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/RequestPane/RequestBody/index.jspackages/bruno-app/src/components/RequestPane/RequestHeaders/index.jspackages/bruno-app/src/components/RequestPane/Script/index.jspackages/bruno-app/src/components/Tabs/index.jstests/request/body-scroll/scroll-persistent.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-app/src/components/BulkEditor/index.js
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js
- packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js
- packages/bruno-app/src/components/RequestPane/RequestBody/index.js
- packages/bruno-app/src/components/FolderSettings/Script/index.js
- packages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.js
- packages/bruno-app/src/components/FolderSettings/Headers/index.js
Comment on lines
+108
to
+112
| const expectScrollRestored = (restored: number, original: number) => { | ||
| console.log({ restored, original }); | ||
| expect(restored).toBeGreaterThan(0); | ||
| // expect(Math.abs(restored - original)).toBeLessThan(SCROLL_TOLERANCE); | ||
| }; |
Contributor
There was a problem hiding this comment.
The restore assertion is too weak to catch real regressions.
expectScrollRestored() currently passes any non-zero offset, so a tab can restore to the wrong position and the suite still goes green. This helper should compare against the captured value with a tolerance.
Proposed fix
-// const SCROLL_TOLERANCE = 50;
+const SCROLL_TOLERANCE = 50;
const expectScrollRestored = (restored: number, original: number) => {
- console.log({ restored, original });
expect(restored).toBeGreaterThan(0);
- // expect(Math.abs(restored - original)).toBeLessThan(SCROLL_TOLERANCE);
+ expect(Math.abs(restored - original)).toBeLessThanOrEqual(SCROLL_TOLERANCE);
};As per coding guidelines, "Write behaviour-driven tests, not implementation-driven ones. Tests should validate real expected output and observable behaviour, not internal details or mocked-out logic unless absolutely necessary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/request/body-scroll/scroll-persistent.spec.ts` around lines 108 - 112,
The current expectScrollRestored helper (expectScrollRestored(restored: number,
original: number)) only asserts restored > 0 which is too weak; change it to
compare restored against original using the existing SCROLL_TOLERANCE constant
(i.e., assert Math.abs(restored - original) < SCROLL_TOLERANCE) so the test
fails when the restored position deviates beyond the acceptable tolerance, and
remove the console.log; keep the non-zero check only if you think it's still
needed but the tolerance comparison must be the primary assertion.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/request/body-scroll/scroll-persistent.spec.ts (2)
547-554: Timeline population via rapid send/cancel may be slow and potentially flaky.This loop clicks send then immediately clicks again to cancel, 50 times. This adds significant test duration and depends on timing. Consider using a fixture with pre-populated timeline data or reducing iterations if full coverage isn't critical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/request/body-scroll/scroll-persistent.spec.ts` around lines 547 - 554, The rapid send/cancel loop in the test step 'Send and cancel requests to generate timeline entries' is slow and timing-flaky; replace it by either using a test fixture or helper that pre-populates timeline entries, or reduce the loop count (e.g., from 50 to a smaller deterministic number) to avoid timing-dependency. Locate the test.step block containing sendBtn (page.getByTestId('send-arrow-icon')) and update it to call the fixture/helper that injects timeline entries, or change the for loop iterations to a lower stable value and/or add explicit waits that assert timeline entry creation rather than relying on immediate double-click behavior.
60-63: Consider reducing fixed timeouts in scroll simulation loop.The 50ms delay per scroll step accumulates quickly. For a 1500px scroll with 200px steps, that's ~8 iterations × 50ms = 400ms minimum. Combined with other waits, each scroll operation takes nearly a second.
If flakiness isn't observed, consider reducing or removing the in-loop delay and relying on the final debounce wait.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/request/body-scroll/scroll-persistent.spec.ts` around lines 60 - 63, The in-loop fixed delay (await page.waitForTimeout(50)) inside the scroll simulation for-loop (which uses steps, scrollStep and page.mouse.wheel) is causing unnecessary test slowness; either remove the per-iteration wait entirely or reduce it to a much smaller value (e.g., 5–10ms) and rely on the existing final debounce/wait after the loop to stabilize the page instead—update the loop to eliminate or shrink the call to page.waitForTimeout so the final debounce covers synchronization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/request/body-scroll/scroll-persistent.spec.ts`:
- Around line 469-475: Remove the leftover broken commented line in the test
step: delete the "// await page.(200);" comment inside the Initialize hook where
selectResponsePaneTab(page, 'Response') and selectResponsePaneTab(page,
'Headers') are called; leave the surrounding calls to selectResponsePaneTab
intact (the function names to locate are selectResponsePaneTab and the page
variable) so the test step no longer contains the incomplete artifact.
- Line 1: The import statement bringing in rest from lodash-es is unused; remove
the unused import (the symbol rest) from the top of the test file (or replace it
with the correct imported symbol if a different utility was intended) so there
are no unused imports in scroll-persistent.spec.ts and linting will pass.
- Around line 838-841: The click on
locators.paneTabs.folderSettingsTab('headers') uses a 10000ms timeout while
other clicks (e.g., locators.paneTabs.folderSettingsTab('script').click) use
2000ms; change the timeout to 2000ms to normalize behavior or, if this
particular tab is genuinely slower, leave 10000ms but add a one-line comment
immediately above explaining why it needs the longer timeout (mentioning
locators.paneTabs.folderSettingsTab('headers') and any flakiness or known
delay).
- Line 19: The local constant modifier is defined but never used; remove the
declaration of modifier from the test (the const modifier = ... line) or, if you
intended to use it, replace its usage accordingly—locate the declaration of
modifier in scroll-persistent.spec.ts and either delete it or wire it into the
relevant test logic (e.g., key event helpers) so it is actually referenced.
- Around line 149-163: Remove the leftover debug console.log calls in the
scroll-persistent.spec.ts test: delete the console.log that prints the saved
object (console.log({ saved })), the console.log that prints '[localStorage
after scroll]' with lsKeys, and any other debug console.log in this test (the
one noted later), leaving only the assertions and page.evaluate code intact so
no debug logging remains.
---
Nitpick comments:
In `@tests/request/body-scroll/scroll-persistent.spec.ts`:
- Around line 547-554: The rapid send/cancel loop in the test step 'Send and
cancel requests to generate timeline entries' is slow and timing-flaky; replace
it by either using a test fixture or helper that pre-populates timeline entries,
or reduce the loop count (e.g., from 50 to a smaller deterministic number) to
avoid timing-dependency. Locate the test.step block containing sendBtn
(page.getByTestId('send-arrow-icon')) and update it to call the fixture/helper
that injects timeline entries, or change the for loop iterations to a lower
stable value and/or add explicit waits that assert timeline entry creation
rather than relying on immediate double-click behavior.
- Around line 60-63: The in-loop fixed delay (await page.waitForTimeout(50))
inside the scroll simulation for-loop (which uses steps, scrollStep and
page.mouse.wheel) is causing unnecessary test slowness; either remove the
per-iteration wait entirely or reduce it to a much smaller value (e.g., 5–10ms)
and rely on the existing final debounce/wait after the loop to stabilize the
page instead—update the loop to eliminate or shrink the call to
page.waitForTimeout so the final debounce covers synchronization.
🪄 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: 26e925e0-18a2-425b-b2b1-444f2d7f9c02
📒 Files selected for processing (1)
tests/request/body-scroll/scroll-persistent.spec.ts
added 10 commits
April 9, 2026 12:22
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
This PR helps to persists scroll positions across tab switches for all request pane, response pane, folder settings, and collection settings tabs
Problem
When switching between tabs (Headers → Body → Script, etc.) or between requests, scroll positions reset to the top. Users lose their place in long scripts, large header lists, and response bodies — requiring them to re-scroll every time they navigate.
Solution
usePersistedEditorScroll & usePersistedContainerScroll hooks for uses
persisted::<tabUid>::<type>-scroll-<entityUid>format, cleaned up automatically using clearPersistedScope(tabUid) on tab close and clearAllPersistedState() on app boot.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
New Features
Bug Fixes
UI/UX Improvements
Tests