refine scrolling by iceljc · Pull Request #468 · SciSharp/BotSharp-UI

iceljc · 2026-07-09T19:15:38Z

No description provided.

qodo-code-review · 2026-07-09T19:16:55Z

PR Summary by Qodo

Refine chat/log scrolling and prevent modal backdrop closes

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Prevent DialogModal from closing on backdrop click when explicitly disabled.
• Rework chat thread scrolling to use OverlayScrollbars + reliable bottom pinning during initial
 layout.
• Hide chat scrollbars consistently and remove legacy reverse-column scroll styling.

Diagram

graph TD
A["ChatBox.svelte"] --> B["Scroll container"] --> C["OverlayScrollbars"]
D["persist-log.svelte"] --> C --> E["Pin-to-bottom helper"] --> F["ResizeObserver"]
A --> E
A --> G["DialogModal.svelte"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Rely on native scroll anchoring (CSS overflow-anchor) + scrollIntoView

➕ Less custom logic than ResizeObserver + manual scrollTop writes
➕ Avoids dependency on OverlayScrollbars internals (viewport element)
➖ Anchoring can be inconsistent with complex DOM updates and virtualization-like patterns
➖ Doesn't address initial async height growth as deterministically across browsers

2. Use OverlayScrollbars events/plugins (e.g., updated callbacks) to re-pin

➕ Keeps scrolling logic inside the scrollbar library lifecycle
➕ Potentially fewer DOM observers and event listeners
➖ Requires deeper coupling to OverlayScrollbars API/events
➖ May still need explicit “stop on user interaction” behavior

3. Extract shared pin-to-bottom utility (shared helper module)

➕ Removes duplicated logic between chat-box and persist-log
➕ Single place to tune stop conditions/timeouts and add tests
➖ Small upfront refactor cost
➖ Requires deciding on a shared API surface (elements vs scrollbar instances)

Recommendation: Current approach (ResizeObserver-based pinning with a user-interaction stop + timeout) is a pragmatic fix for async content height changes (images/mermaid/code blocks) that commonly break “scroll to bottom” behavior. The main improvement to consider is extracting the pinning logic into a shared helper to avoid divergence between chat and log implementations; the rest of the design is reasonable for the problem.

Files changed (4) +127 / -40

Enhancement (1) +4 / -0

DialogModal.svelteAdd option to disable backdrop click-to-close +4/-0

Add option to disable backdrop click-to-close

• Introduces a new 'disableBackdropClick' prop and short-circuits the backdrop click handler when enabled. This allows consumers to keep modals open unless explicitly closed via buttons/actions.

src/lib/common/modals/DialogModal.svelte

Bug fix (3) +123 / -40

_chat.scssSwitch chat thread to standard scrolling and hide scrollbar cross-browser +5/-13

Switch chat thread to standard scrolling and hide scrollbar cross-browser
• Removes the legacy reversed flex-column scrolling approach and uses 'overflow-y: scroll' for the message thread container. Adds WebKit scrollbar hiding and deletes the now-unused '.cb-msgs-scroll-rev' alias class.
src/lib/styles/pages/_chat.scss

chat-box.svelteStabilize initial chat autoscroll with OverlayScrollbars + ResizeObserver pinning +79/-25

Stabilize initial chat autoscroll with OverlayScrollbars + ResizeObserver pinning

• Initializes OverlayScrollbars explicitly for the message thread, destroys instances on component teardown, and updates auto-scroll to target the OverlayScrollbars viewport element. Adds 'pinToBottomWhileSettling()' to keep the thread pinned during initial async layout growth, and updates refresh() to optionally skip autoscroll during setup. Also wires 'disableBackdropClick' into the big-message DialogModal usage and removes the legacy reverse-scroll class from markup.

src/routes/chat/[agentId]/[conversationId]/chat-box.svelte

persist-log.sveltePin log panels to bottom while content height settles +39/-2

Pin log panels to bottom while content height settles
• Replaces the initial one-time smooth scroll-to-bottom with a ResizeObserver-based pinning helper that re-pins on size changes. Pinning stops on user interaction or after a timeout to avoid fighting manual scrolling.
src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte

qodo-code-review · 2026-07-09T19:20:05Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Pin observer lacks teardown 🐞 Bug ☼ Reliability

Description

In chat-box.svelte, pinToBottomWhileSettling() registers a ResizeObserver, timeout, and event
listeners but nothing calls its stop()/disconnect logic during component teardown, so callbacks can
run against detached DOM until the timeout fires. onDestroy() only destroys OverlayScrollbars
instances and does not stop the pinning observer/listeners.

Code

src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[R501-526]

+	function pinToBottomWhileSettling(timeoutMs = 3000) {
+		const scrollbar = scrollbars[0];
+		if (!scrollbar) return;
+
+		const { viewport } = scrollbar.elements();
+		const content = viewport.firstElementChild || viewport;
+		const pin = () => { viewport.scrollTop = viewport.scrollHeight; };
+		pin();
+
+		const observer = new ResizeObserver(pin);
+		observer.observe(content);
+
+		/** @type {ReturnType<typeof setTimeout>} */
+		let timer;
+		const stop = () => {
+			observer.disconnect();
+			clearTimeout(timer);
+			viewport.removeEventListener('wheel', stop);
+			viewport.removeEventListener('pointerdown', stop);
+			viewport.removeEventListener('keydown', stop);
+		};
+		viewport.addEventListener('wheel', stop, { passive: true });
+		viewport.addEventListener('pointerdown', stop);
+		viewport.addEventListener('keydown', stop);
+		timer = setTimeout(stop, timeoutMs);
+	}

Evidence
The pinning function explicitly creates a ResizeObserver, timer, and event listeners, but the
component teardown only destroys OverlayScrollbars instances and never calls the pinning cleanup;
therefore those callbacks/listeners can outlive the component until the timeout/user interaction
triggers stop().
src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[235-237]
src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[492-526]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pinToBottomWhileSettling()` creates a `ResizeObserver`, a timeout, and several event listeners, but its cleanup (`observer.disconnect()`, `clearTimeout`, `removeEventListener`) is only executed when `stop()` runs. If the route/component unmounts before that, these resources can remain active briefly and execute against DOM that’s no longer owned by the component.

### Issue Context
`onDestroy()` currently destroys OverlayScrollbars instances, but the pinning observer/listeners created by `pinToBottomWhileSettling()` are not referenced there.

### Fix Focus Areas
- src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[235-237]
- src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[492-526]

### Suggested fix
- Make `pinToBottomWhileSettling()` return a disposer (e.g., `return stop;`) or store `stop` in an outer-scope variable.
- Invoke that disposer from `onDestroy()` (and optionally before re-running pinning) to ensure the `ResizeObserver`, timer, and listeners are always cleaned up on unmount.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Persist-log leaks scroll resources 🐞 Bug ☼ Reliability

Description

In persist-log.svelte, pinToBottomWhileSettling() adds ResizeObserver/listeners per panel but the
onMount cleanup only calls cleanLogs(), so these resources may remain active after unmount until the
timeout fires. Additionally, initScrollbars() creates OverlayScrollbars instances but the component
never destroys them on teardown.

Code

src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[R123-148]

+    function pinToBottomWhileSettling(timeoutMs = 3000) {
+        scrollbars.forEach(scrollbar => {
+            if (!scrollbar) return;
+
+            const { viewport } = scrollbar.elements();
+            const content = viewport.firstElementChild || viewport;
+            const pin = () => { viewport.scrollTop = viewport.scrollHeight; };
+            pin();
+
+            const observer = new ResizeObserver(pin);
+            observer.observe(content);
+
+            /** @type {ReturnType<typeof setTimeout>} */
+            let timer;
+            const stop = () => {
+                observer.disconnect();
+                clearTimeout(timer);
+                viewport.removeEventListener('wheel', stop);
+                viewport.removeEventListener('pointerdown', stop);
+                viewport.removeEventListener('keydown', stop);
+            };
+            viewport.addEventListener('wheel', stop, { passive: true });
+            viewport.addEventListener('pointerdown', stop);
+            viewport.addEventListener('keydown', stop);
+            timer = setTimeout(stop, timeoutMs);
+        });

Evidence
The component mounts and initializes OverlayScrollbars, then calls the new pinning function which
registers ResizeObservers/listeners/timers. The only cleanup currently performed on teardown is
clearing the logs array, so the scroll-related resources are not explicitly released (and scrollbars
are never destroyed).
src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[69-84]
src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[114-149]
src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[151-160]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pinToBottomWhileSettling()` attaches `ResizeObserver` + event listeners + a timer for each OverlayScrollbars viewport, but the component’s teardown path (the `onMount` return cleanup) doesn’t call any disposer, so resources can outlive the component briefly. Separately, OverlayScrollbars instances created in `initScrollbars()` are never `.destroy()`’d.

### Issue Context
PersistLog is conditionally mounted/unmounted from chat-box; when it unmounts, it should release all observers/listeners and destroy OverlayScrollbars instances.

### Fix Focus Areas
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[69-84]
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[114-149]
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[151-160]

### Suggested fix
- Refactor `pinToBottomWhileSettling()` to return an array of disposer functions (one per scrollbar) or store them in component scope.
- In the `onMount` cleanup (or `onDestroy`), call those disposers to `disconnect()` observers, remove listeners, and clear timers immediately.
- Also destroy OverlayScrollbars instances in teardown (e.g., `scrollbars.forEach(sb => sb?.destroy?.()); scrollbars = [];`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-07-09T19:20:06Z

+	function pinToBottomWhileSettling(timeoutMs = 3000) {
+		const scrollbar = scrollbars[0];
+		if (!scrollbar) return;
+
+		const { viewport } = scrollbar.elements();
+		const content = viewport.firstElementChild || viewport;
+		const pin = () => { viewport.scrollTop = viewport.scrollHeight; };
+		pin();
+
+		const observer = new ResizeObserver(pin);
+		observer.observe(content);
+
+		/** @type {ReturnType<typeof setTimeout>} */
+		let timer;
+		const stop = () => {
+			observer.disconnect();
+			clearTimeout(timer);
+			viewport.removeEventListener('wheel', stop);
+			viewport.removeEventListener('pointerdown', stop);
+			viewport.removeEventListener('keydown', stop);
+		};
+		viewport.addEventListener('wheel', stop, { passive: true });
+		viewport.addEventListener('pointerdown', stop);
+		viewport.addEventListener('keydown', stop);
+		timer = setTimeout(stop, timeoutMs);
+	}


1. Pin observer lacks teardown 🐞 Bug ☼ Reliability

In chat-box.svelte, pinToBottomWhileSettling() registers a ResizeObserver, timeout, and event listeners but nothing calls its stop()/disconnect logic during component teardown, so callbacks can run against detached DOM until the timeout fires. onDestroy() only destroys OverlayScrollbars instances and does not stop the pinning observer/listeners.

Agent Prompt

### Issue description `pinToBottomWhileSettling()` creates a `ResizeObserver`, a timeout, and several event listeners, but its cleanup (`observer.disconnect()`, `clearTimeout`, `removeEventListener`) is only executed when `stop()` runs. If the route/component unmounts before that, these resources can remain active briefly and execute against DOM that’s no longer owned by the component. ### Issue Context `onDestroy()` currently destroys OverlayScrollbars instances, but the pinning observer/listeners created by `pinToBottomWhileSettling()` are not referenced there. ### Fix Focus Areas - src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[235-237] - src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[492-526] ### Suggested fix - Make `pinToBottomWhileSettling()` return a disposer (e.g., `return stop;`) or store `stop` in an outer-scope variable. - Invoke that disposer from `onDestroy()` (and optionally before re-running pinning) to ensure the `ResizeObserver`, timer, and listeners are always cleaned up on unmount.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-07-09T19:20:06Z

+    function pinToBottomWhileSettling(timeoutMs = 3000) {
+        scrollbars.forEach(scrollbar => {
+            if (!scrollbar) return;
+
+            const { viewport } = scrollbar.elements();
+            const content = viewport.firstElementChild || viewport;
+            const pin = () => { viewport.scrollTop = viewport.scrollHeight; };
+            pin();
+
+            const observer = new ResizeObserver(pin);
+            observer.observe(content);
+
+            /** @type {ReturnType<typeof setTimeout>} */
+            let timer;
+            const stop = () => {
+                observer.disconnect();
+                clearTimeout(timer);
+                viewport.removeEventListener('wheel', stop);
+                viewport.removeEventListener('pointerdown', stop);
+                viewport.removeEventListener('keydown', stop);
+            };
+            viewport.addEventListener('wheel', stop, { passive: true });
+            viewport.addEventListener('pointerdown', stop);
+            viewport.addEventListener('keydown', stop);
+            timer = setTimeout(stop, timeoutMs);
+        });


2. Persist-log leaks scroll resources 🐞 Bug ☼ Reliability

In persist-log.svelte, pinToBottomWhileSettling() adds ResizeObserver/listeners per panel but the onMount cleanup only calls cleanLogs(), so these resources may remain active after unmount until the timeout fires. Additionally, initScrollbars() creates OverlayScrollbars instances but the component never destroys them on teardown.

Agent Prompt

### Issue description `pinToBottomWhileSettling()` attaches `ResizeObserver` + event listeners + a timer for each OverlayScrollbars viewport, but the component’s teardown path (the `onMount` return cleanup) doesn’t call any disposer, so resources can outlive the component briefly. Separately, OverlayScrollbars instances created in `initScrollbars()` are never `.destroy()`’d. ### Issue Context PersistLog is conditionally mounted/unmounted from chat-box; when it unmounts, it should release all observers/listeners and destroy OverlayScrollbars instances. ### Fix Focus Areas - src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[69-84] - src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[114-149] - src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[151-160] ### Suggested fix - Refactor `pinToBottomWhileSettling()` to return an array of disposer functions (one per scrollbar) or store them in component scope. - In the `onMount` cleanup (or `onDestroy`), call those disposers to `disconnect()` observers, remove listeners, and clear timers immediately. - Also destroy OverlayScrollbars instances in teardown (e.g., `scrollbars.forEach(sb => sb?.destroy?.()); scrollbars = [];`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

refine scrolling

4d55f46

iceljc merged commit 3c0df85 into SciSharp:main Jul 9, 2026
1 of 2 checks passed

qodo-code-review Bot reviewed Jul 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refine scrolling#468

refine scrolling#468
iceljc merged 1 commit into
SciSharp:mainfrom
iceljc:features/refine-response-format

iceljc commented Jul 9, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 9, 2026

Uh oh!

qodo-code-review Bot commented Jul 9, 2026

Uh oh!

qodo-code-review Bot Jul 9, 2026

Uh oh!

qodo-code-review Bot Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

iceljc commented Jul 9, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 9, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 9, 2026

Code Review by Qodo

Uh oh!

qodo-code-review Bot Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant