Skip to content

Commit 205355d

Browse files
djalmaaraujodjalmaaraujo
committed
[Bug Fix] Message Scroller: address review feedback
- Gate anchored-turn scrolling behind autoScroll/following, so a new turn never yanks a reader who scrolled up to older content (P1). - Scroll button honors data-direction: a start-direction button now jumps to the start instead of the end (renamed action jumpToEnd → jump) (P2). - Guard last-anchor opening position with hasContentTarget; the Stimulus target getter throws rather than returning undefined (P2). - Include the flex row gap in prepend preservation so the visible row no longer drifts down by one gap per history insertion (P2). - Only treat direct content children as transcript rows; markup mutated inside a message (subtree) is handled as streaming, not history (P2). Rebuilt MCP registry.
1 parent 2cf848c commit 205355d

5 files changed

Lines changed: 90 additions & 26 deletions

File tree

‎docs/app/javascript/controllers/ruby_ui/message_scroller_controller.js‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,29 @@ export default class extends Controller {
117117
onMutations(records) {
118118
let appended = null;
119119
let prependedHeight = 0;
120+
let streamed = false;
121+
const gap = this.rowGap();
120122

121123
for (const record of records) {
124+
// Text streamed into an existing row (e.g. tokens) — not a new turn.
125+
if (record.type === "characterData") {
126+
streamed = true;
127+
continue;
128+
}
122129
if (record.type !== "childList") continue;
130+
// Only direct children of the content element are transcript rows.
131+
// Markup inserted *inside* a message must not be mistaken for history.
132+
if (record.target !== this.contentTarget) {
133+
streamed = true;
134+
continue;
135+
}
123136
for (const node of record.addedNodes) {
124137
if (node.nodeType !== Node.ELEMENT_NODE) continue;
125138
if (record.previousSibling === null && record.nextSibling !== null) {
126-
// Inserted above existing rows → history prepend.
127-
prependedHeight += node.offsetHeight || 0;
139+
// Inserted above existing rows → history prepend. Account for the
140+
// flex row gap each prepended row introduces, or the preserved row
141+
// drifts down by one gap per insertion.
142+
prependedHeight += (node.offsetHeight || 0) + gap;
128143
} else {
129144
// Inserted at (or after) the end → new turn.
130145
appended = node;
@@ -137,23 +152,32 @@ export default class extends Controller {
137152
this.viewportTarget.scrollTop += prependedHeight;
138153
}
139154

140-
if (appended) {
155+
// Only move for new/streamed content while the reader is at the live edge.
156+
// If they scrolled away, leave them there and let the button surface it.
157+
const follow = this.autoScrollValue && this.following;
158+
if (appended && follow) {
141159
const anchor = appended.matches?.("[data-scroll-anchor]")
142160
? appended
143161
: appended.querySelector?.("[data-scroll-anchor]");
144162
if (anchor) {
145163
this.scrollToAnchor(anchor);
146-
} else if (this.autoScrollValue && this.following) {
164+
} else {
147165
this.scrollToEnd();
148166
}
149-
} else if (this.autoScrollValue && this.following) {
150-
// No new row — text streamed into the last row. Stay pinned.
167+
} else if (!appended && streamed && follow) {
168+
// Text streamed into the last row. Stay pinned.
151169
this.scrollToEnd("auto");
152170
}
153171

154172
this.updateButton();
155173
}
156174

175+
rowGap() {
176+
if (!this.hasContentTarget) return 0;
177+
const value = parseFloat(getComputedStyle(this.contentTarget).rowGap);
178+
return Number.isFinite(value) ? value : 0;
179+
}
180+
157181
// --- Public scroll commands ---------------------------------------------
158182

159183
scrollToEnd(behavior = "smooth") {
@@ -184,9 +208,14 @@ export default class extends Controller {
184208
this.scrollTo(top, behavior);
185209
}
186210

187-
// Bound to the scroll button's click action.
188-
jumpToEnd() {
189-
this.scrollToEnd();
211+
// Bound to the scroll button's click action. Honors the button's
212+
// data-direction so a start-direction button jumps to the start.
213+
jump(event) {
214+
if (event?.currentTarget?.dataset.direction === "start") {
215+
this.scrollToStart();
216+
} else {
217+
this.scrollToEnd();
218+
}
190219
}
191220

192221
// --- Internals -----------------------------------------------------------
@@ -254,8 +283,11 @@ export default class extends Controller {
254283
}
255284

256285
if (position === "last-anchor") {
257-
const anchors = this.contentTarget?.querySelectorAll("[data-scroll-anchor]");
258-
const last = anchors && anchors[anchors.length - 1];
286+
// Stimulus' contentTarget getter throws when missing — guard explicitly.
287+
const anchors = this.hasContentTarget
288+
? this.contentTarget.querySelectorAll("[data-scroll-anchor]")
289+
: [];
290+
const last = anchors[anchors.length - 1];
259291
// Fall back to the end when there's no anchor, or the last turn already
260292
// fits in the viewport.
261293
if (last && last.offsetTop - this.previousItemPeekValue > 0) {

‎gem/lib/ruby_ui/message_scroller/message_scroller_button.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ def default_attrs
2929
direction: @direction,
3030
active: "false",
3131
ruby_ui__message_scroller_target: "button",
32-
action: "click->ruby-ui--message-scroller#jumpToEnd"
32+
action: "click->ruby-ui--message-scroller#jump"
3333
},
3434
class: "absolute left-1/2 z-10 -translate-x-1/2 inline-flex size-8 items-center justify-center rounded-full border border-border bg-background text-foreground shadow-sm transition-[translate,scale,opacity] duration-200 hover:bg-muted hover:text-foreground data-[active=false]:pointer-events-none data-[active=false]:scale-95 data-[active=false]:opacity-0 data-[active=true]:scale-100 data-[active=true]:opacity-100 data-[direction=end]:bottom-4 data-[direction=end]:data-[active=false]:translate-y-full data-[direction=start]:top-4 data-[direction=start]:data-[active=false]:-translate-y-full data-[direction=start]:[&_svg]:rotate-180"
3535
}

‎gem/lib/ruby_ui/message_scroller/message_scroller_controller.js‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,29 @@ export default class extends Controller {
117117
onMutations(records) {
118118
let appended = null;
119119
let prependedHeight = 0;
120+
let streamed = false;
121+
const gap = this.rowGap();
120122

121123
for (const record of records) {
124+
// Text streamed into an existing row (e.g. tokens) — not a new turn.
125+
if (record.type === "characterData") {
126+
streamed = true;
127+
continue;
128+
}
122129
if (record.type !== "childList") continue;
130+
// Only direct children of the content element are transcript rows.
131+
// Markup inserted *inside* a message must not be mistaken for history.
132+
if (record.target !== this.contentTarget) {
133+
streamed = true;
134+
continue;
135+
}
123136
for (const node of record.addedNodes) {
124137
if (node.nodeType !== Node.ELEMENT_NODE) continue;
125138
if (record.previousSibling === null && record.nextSibling !== null) {
126-
// Inserted above existing rows → history prepend.
127-
prependedHeight += node.offsetHeight || 0;
139+
// Inserted above existing rows → history prepend. Account for the
140+
// flex row gap each prepended row introduces, or the preserved row
141+
// drifts down by one gap per insertion.
142+
prependedHeight += (node.offsetHeight || 0) + gap;
128143
} else {
129144
// Inserted at (or after) the end → new turn.
130145
appended = node;
@@ -137,23 +152,32 @@ export default class extends Controller {
137152
this.viewportTarget.scrollTop += prependedHeight;
138153
}
139154

140-
if (appended) {
155+
// Only move for new/streamed content while the reader is at the live edge.
156+
// If they scrolled away, leave them there and let the button surface it.
157+
const follow = this.autoScrollValue && this.following;
158+
if (appended && follow) {
141159
const anchor = appended.matches?.("[data-scroll-anchor]")
142160
? appended
143161
: appended.querySelector?.("[data-scroll-anchor]");
144162
if (anchor) {
145163
this.scrollToAnchor(anchor);
146-
} else if (this.autoScrollValue && this.following) {
164+
} else {
147165
this.scrollToEnd();
148166
}
149-
} else if (this.autoScrollValue && this.following) {
150-
// No new row — text streamed into the last row. Stay pinned.
167+
} else if (!appended && streamed && follow) {
168+
// Text streamed into the last row. Stay pinned.
151169
this.scrollToEnd("auto");
152170
}
153171

154172
this.updateButton();
155173
}
156174

175+
rowGap() {
176+
if (!this.hasContentTarget) return 0;
177+
const value = parseFloat(getComputedStyle(this.contentTarget).rowGap);
178+
return Number.isFinite(value) ? value : 0;
179+
}
180+
157181
// --- Public scroll commands ---------------------------------------------
158182

159183
scrollToEnd(behavior = "smooth") {
@@ -184,9 +208,14 @@ export default class extends Controller {
184208
this.scrollTo(top, behavior);
185209
}
186210

187-
// Bound to the scroll button's click action.
188-
jumpToEnd() {
189-
this.scrollToEnd();
211+
// Bound to the scroll button's click action. Honors the button's
212+
// data-direction so a start-direction button jumps to the start.
213+
jump(event) {
214+
if (event?.currentTarget?.dataset.direction === "start") {
215+
this.scrollToStart();
216+
} else {
217+
this.scrollToEnd();
218+
}
190219
}
191220

192221
// --- Internals -----------------------------------------------------------
@@ -254,8 +283,11 @@ export default class extends Controller {
254283
}
255284

256285
if (position === "last-anchor") {
257-
const anchors = this.contentTarget?.querySelectorAll("[data-scroll-anchor]");
258-
const last = anchors && anchors[anchors.length - 1];
286+
// Stimulus' contentTarget getter throws when missing — guard explicitly.
287+
const anchors = this.hasContentTarget
288+
? this.contentTarget.querySelectorAll("[data-scroll-anchor]")
289+
: [];
290+
const last = anchors[anchors.length - 1];
259291
// Fall back to the end when there's no anchor, or the last turn already
260292
// fits in the viewport.
261293
if (last && last.offsetTop - this.previousItemPeekValue > 0) {

‎gem/test/ruby_ui/message_scroller_test.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ def test_button_targets_and_action
7070
output = phlex { RubyUI.MessageScrollerButton }
7171

7272
assert_match(/message-scroller-target="button"/, output)
73-
assert_match(/click->ruby-ui--message-scroller#jumpToEnd/, output)
73+
assert_match(/click->ruby-ui--message-scroller#jump/, output)
7474
assert_match(/data-direction="end"/, output)
7575
assert_match(/Scroll to end/, output)
7676
end

0 commit comments

Comments
 (0)