Skip to content

feat(scroller): DLT-2624 add before/after/empty slots and scroll-end events - #1385

Merged
Joan Gil (joandvgv) merged 16 commits into
stagingfrom
DLT-2624
Aug 7, 2026
Merged

feat(scroller): DLT-2624 add before/after/empty slots and scroll-end events#1385
Joan Gil (joandvgv) merged 16 commits into
stagingfrom
DLT-2624

Conversation

@joandvgv

@joandvgv Joan Gil (joandvgv) commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Fix
  • Feature
  • Performance Improvement
  • Refactor

📖 Jira Ticket

https://dialpad.atlassian.net/browse/DLT-2624

📖 Description

Adds the slots and events DtScroller was missing, so consumers can build infinite-scrolling lists on it.

  • before, after and empty slots. Positioned exactly as upstream vue-virtual-scroller places them: before and after render inside a .vue-recycle-scroller__slot wrapper as siblings of the item wrapper, empty renders inside the wrapper after the items. The CSS for those wrappers already shipped in dialtone-css, so there is no CSS change here. Every forward is guarded by v-if="$slots.<name>" at all three layers, so no empty wrapper element renders when a consumer passes nothing.
  • scroll-end and scroll-start events, restored from the vendored upstream code that had been commented out in core_scroller.vue. They fire when the last or first item enters the rendered view pool — one buffer length before the viewport edge — so a consumer can prefetch before the user runs out of content. user-position is unchanged.
  • buffer promoted to a declared prop. It previously reached CoreScroller only through attribute fallthrough: functional, but untyped, undocumented, and liable to break silently if anyone added inheritAttrs: false.
  • Docs and Storybook. New Infinite Scroll section on the docs page, argTypes entries for the new slots and events, and the buffer control is now actually bound in the Default story (it was previously declared but inert).

💡 Context

DtScroller is a vendored fork of vue-virtual-scroller that kept only the default slot. Any consumer needing a loading indicator pinned to the end of a virtualised list — the standard infinite-scroll pattern — had no way to render one inside the scroll viewport, and no event telling it when to fetch. That left applications on the third-party library instead of the design system.

The docs also now cover something that was previously undocumented and easy to trip over: DtScroller recomputes its rendered window only on mount and on scroll, so it does not react to items changing on its own. Consumers have to call updateItems() after appending a page, and should assign a new array rather than mutating in place. This has caught more than one consumer, so it is written down now with a worked example.

📝 Checklist

For all PRs:

  • I have ensured no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).
  • I have reviewed my changes.
  • I have added all relevant documentation.
  • I have considered the performance impact of my change.

For all Vue changes:

  • I have added / updated unit tests.
  • I have validated components with a screen reader.
  • I have validated components keyboard navigation.

The two accessibility boxes are deliberately unticked — see Next Steps.

🔮 Next Steps

  • Restoring the upstream items watcher in core_scroller.vue would remove the need for consumers to call updateItems() at all. It was deliberately left out of scope here: it is a behaviour change to a shared component and deserves its own ticket. The commented-out original shows why it was disabled — prepending and appending need different recompute calls to avoid the scroll position jumping, and only the caller knows which happened.

📷 Screenshots / GIFs

The Default story in Storybook covers this: it is fixed-height with 50 items, logs scroll-start / scroll-end to the actions panel, and renders a loading indicator in the after slot. Scrolling to the floor there exercises the whole path — event timing, slot placement, and that the indicator scrolls with the list rather than being pinned.

🔗 Sources

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 1fcca3fd-01f5-4626-bbc1-b998578e1ac8

📥 Commits

Reviewing files that changed from the base of the PR and between 31215e7 and 7a70f95.

📒 Files selected for processing (5)
  • apps/dialtone-documentation/docs/components/scroller.md
  • packages/dialtone-vue/components/scroller/modules/core_scroller.vue
  • packages/dialtone-vue/components/scroller/scroller.stories.js
  • packages/dialtone-vue/components/scroller/scroller.test.js
  • packages/dialtone-vue/components/scroller/scroller.vue
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • dialpad/ios (manual)
  • dialpad/firespotter (manual)
  • dialpad/semantic-release-changelog-json (auto-detected)
  • dialpad/conventional-changelog-angular (auto-detected)

Adds before, after, and empty slots, scroll-start and scroll-end events, and a declared buffer prop to DtScroller. Updates rendering logic, tests, documentation, and Storybook examples for infinite scrolling and updateItems() usage.

Overall Judgement: ✅ Ready to merge — The implementation and supporting tests and documentation cover the new infinite-scrolling API.

Walkthrough

Changes

The scroller now supports buffer-relative scroll-start and scroll-end events, before, empty, and after slots, dynamic-mode forwarding, loading indicators, tests, Storybook metadata, and infinite-scroll documentation.

Scroller infinite-scroll support

Layer / File(s) Summary
Boundary events and buffer contract
packages/dialtone-vue/components/scroller/modules/core_scroller.vue, packages/dialtone-vue/components/scroller/scroller.vue, packages/dialtone-vue/components/scroller/scroller.test.js
CoreScroller detects newly activated first and last views and emits boundary events. DtScroller exposes the events and forwards the configurable buffer, which defaults to 200. Tests cover event emission, cleanup, and buffer forwarding.
Slot composition and loading content
packages/dialtone-vue/components/scroller/modules/core_scroller.vue, packages/dialtone-vue/components/scroller/modules/dynamic_scroller.vue, packages/dialtone-vue/components/scroller/scroller.vue, packages/dialtone-vue/components/scroller/scroller.test.js, packages/dialtone-vue/components/scroller/scroller_default.story.vue
The scroller renders optional before, empty, and after slots in static and dynamic modes. Slot dimensions affect the rendered window. The default story renders a loading indicator in the after slot.
Storybook and infinite-scroll documentation
packages/dialtone-vue/components/scroller/scroller.stories.js, apps/dialtone-documentation/docs/components/scroller.md
Storybook documents the new events, slots, and buffer control. The component guide documents pagination timing, item updates, array replacement, and empty-state rendering.

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant CoreScroller
  participant DtScroller
  participant Parent
  User->>CoreScroller: Scroll within the configured buffer
  CoreScroller->>DtScroller: Emit scroll-start or scroll-end
  DtScroller->>Parent: Forward scroll-start or scroll-end
Loading

Possibly related PRs

Suggested reviewers: francisrupert, iropolo, ninarepetto

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DLT-2624

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

✔️ Deploy previews ready!
😎 Dialtone documentation preview: https://dialtone.dialpad.com/deploy-previews/pr-1385/
😎 Dialtone-vue preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-1385/

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/dialtone-documentation/docs/components/scroller.md`:
- Around line 237-244: Update fetchNextPage to return immediately when
isFetching.value is already true, preventing concurrent pagination requests.
Wrap the fetchMore and item-update flow in try/finally so isFetching.value is
reset to false on both success and rejection, while preserving the existing
nextTick and scroller.value.updateItems behavior after a successful fetch.
- Around line 200-206: Update the Infinite Scroll section in the Scroller
documentation to describe the before slot as content rendered before the items
that scrolls with the list, optionally including a concise usage example.

In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue`:
- Around line 54-56: Render the empty slot only when items is empty by adding
the !items.length guard in core_scroller.vue. Update
packages/dialtone-vue/components/scroller/scroller.test.js lines 211-216 to
mount with an empty item list and retain one assertion confirming the empty
content renders.
- Around line 12-18: The normal-flow before slot is not accounted for by the
virtual scroll calculations, causing incorrect visibility when it has height.
Update the scroller’s _getScroll() and _updateVisibleItems() logic to measure
and apply the before-slot offset, or render the slot outside the virtual
coordinate system, while preserving correct item visibility; add a regression
test using a nonzero-height before slot.

In `@packages/dialtone-vue/components/scroller/scroller_default.story.vue`:
- Around line 137-140: Update the loading indicator styling around the .loading
class to remove inline component CSS, replacing it with Dialtone utility classes
or a tokenized style defined in packages/dialtone-css. Replace the raw `#666`
color and hardcoded pixel padding with the appropriate Dialtone color and
spacing tokens while preserving the italic loading appearance.

In `@packages/dialtone-vue/components/scroller/scroller.test.js`:
- Around line 95-98: Split the multi-assertion tests in scroller.test.js so each
test contains exactly one assertion: lines 95-98 should separately verify the
public buffer prop and CoreScroller forwarding; lines 149-156 should separately
verify the precondition and event emission; lines 197-202 and 204-209 should
separately verify component existence and wrapper placement. Update the relevant
scroller test cases without changing their behaviors.

In `@packages/dialtone-vue/components/scroller/scroller.vue`:
- Around line 68-76: Update the buffer prop definition in Scroller to validate
finite, non-negative numbers, then normalize any invalid runtime value to the
default before passing it to CoreScroller. Ensure negative and non-finite values
cannot affect the computed render range, while valid buffer values retain their
existing behavior.
🪄 Autofix

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: Repository YAML (base), Central YAML (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 423bdaca-882b-4403-8158-9abc4c55ccfc

📥 Commits

Reviewing files that changed from the base of the PR and between bfc478c and 080c708.

📒 Files selected for processing (7)
  • apps/dialtone-documentation/docs/components/scroller.md
  • packages/dialtone-vue/components/scroller/modules/core_scroller.vue
  • packages/dialtone-vue/components/scroller/modules/dynamic_scroller.vue
  • packages/dialtone-vue/components/scroller/scroller.stories.js
  • packages/dialtone-vue/components/scroller/scroller.test.js
  • packages/dialtone-vue/components/scroller/scroller.vue
  • packages/dialtone-vue/components/scroller/scroller_default.story.vue
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • dialpad/ios (manual)
  • dialpad/firespotter (manual)
  • dialpad/semantic-release-changelog-json (auto-detected)
  • dialpad/conventional-changelog-angular (auto-detected)

Comment thread apps/dialtone-documentation/docs/components/scroller.md Outdated
Comment thread apps/dialtone-documentation/docs/components/scroller.md
Comment thread packages/dialtone-vue/components/scroller/modules/core_scroller.vue
Comment thread packages/dialtone-vue/components/scroller/modules/core_scroller.vue Outdated
Comment thread packages/dialtone-vue/components/scroller/scroller_default.story.vue Outdated
Comment on lines +95 to +98
it('declares buffer as its own prop and passes it to the underlying scroller', () => {
expect(wrapper.props('buffer')).toBe(500);
expect(wrapper.findComponent(CoreScroller).props('buffer')).toBe(500);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Split these tests to one assertion each.

  • packages/dialtone-vue/components/scroller/scroller.test.js#L95-L98: Separate public prop declaration from CoreScroller forwarding.
  • packages/dialtone-vue/components/scroller/scroller.test.js#L149-L156: Separate the precondition from event emission.
  • packages/dialtone-vue/components/scroller/scroller.test.js#L197-L202: Assert existence and wrapper placement in separate tests.
  • packages/dialtone-vue/components/scroller/scroller.test.js#L204-L209: Assert existence and wrapper placement in separate tests.

As per coding guidelines, **/*.test.{js,ts} requires one assertion per test.

📍 Affects 1 file
  • packages/dialtone-vue/components/scroller/scroller.test.js#L95-L98 (this comment)
  • packages/dialtone-vue/components/scroller/scroller.test.js#L149-L156
  • packages/dialtone-vue/components/scroller/scroller.test.js#L197-L202
  • packages/dialtone-vue/components/scroller/scroller.test.js#L204-L209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/scroller.test.js` around lines 95 -
98, Split the multi-assertion tests in scroller.test.js so each test contains
exactly one assertion: lines 95-98 should separately verify the public buffer
prop and CoreScroller forwarding; lines 149-156 should separately verify the
precondition and event emission; lines 197-202 and 204-209 should separately
verify component existence and wrapper placement. Update the relevant scroller
test cases without changing their behaviors.

Sources: Coding guidelines, Path instructions

Comment thread packages/dialtone-vue/components/scroller/scroller.vue
@braddialpad

Copy link
Copy Markdown
Contributor

Ignacio Ropolo (@iropolo) you probably want to take a look at this one since you wrote it initially

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/dialtone-vue/components/scroller/modules/core_scroller.vue (2)

12-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Account for the leading slot in scrollToItem()

DtScroller publicly exposes scrollToItem(), but the method uses the item-wrapper coordinate. A non-empty before slot shifts the wrapper, so the target is offset by the slot size. Add this offset for fixed and dynamic item sizes, and add a regression test.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue` around
lines 12 - 20, Update DtScroller’s scrollToItem() calculations to include the
rendered before slot’s height/offset when determining the target position, for
both fixed-size and dynamic-size item layouts. Preserve existing behavior when
the slot is absent or empty, and add a regression test covering a non-empty
before slot and scrollToItem().

544-547: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Finalize scroller state before emitting boundary events.

If a boundary handler changes items and calls updateItems() synchronously, re-entry occurs before view.position, startIndex, and endIndex are assigned. The outer loop can then resume with a changed item list and pool. Record boundary flags and emit after these assignments, or defer re-entry.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue` around
lines 544 - 547, Update the scroller logic around the boundary checks in the
newlyUsedView handling to record scroll-start and scroll-end flags instead of
emitting immediately. Assign view.position, startIndex, and endIndex first, then
emit the deferred boundary events so synchronous updateItems() calls cannot
re-enter with incomplete state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue`:
- Around line 361-366: Update the buffered bounds calculation in the scroller’s
scroll-window logic to subtract the measured leading-slot size from both
scroll.start and scroll.end. Remove the trailing-slot addition to scroll.end,
while preserving the existing direction-specific slotSize measurement.

In `@packages/dialtone-vue/components/scroller/scroller.test.js`:
- Around line 252-256: Update the scroller test around firstItem to add a stable
data-qa attribute to the recycled item and select it using that attribute
instead of the .vue-recycle-scroller__item-view class. Preserve the existing
User 0 selection and transform assertion.

---

Outside diff comments:
In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue`:
- Around line 12-20: Update DtScroller’s scrollToItem() calculations to include
the rendered before slot’s height/offset when determining the target position,
for both fixed-size and dynamic-size item layouts. Preserve existing behavior
when the slot is absent or empty, and add a regression test covering a non-empty
before slot and scrollToItem().
- Around line 544-547: Update the scroller logic around the boundary checks in
the newlyUsedView handling to record scroll-start and scroll-end flags instead
of emitting immediately. Assign view.position, startIndex, and endIndex first,
then emit the deferred boundary events so synchronous updateItems() calls cannot
re-enter with incomplete state.
🪄 Autofix

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: Repository YAML (base), Central YAML (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 94f57528-8799-4a21-89e8-eb329f8f5ff5

📥 Commits

Reviewing files that changed from the base of the PR and between 080c708 and f7ebf24.

📒 Files selected for processing (4)
  • apps/dialtone-documentation/docs/components/scroller.md
  • packages/dialtone-vue/components/scroller/modules/core_scroller.vue
  • packages/dialtone-vue/components/scroller/scroller.test.js
  • packages/dialtone-vue/components/scroller/scroller_default.story.vue
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • dialpad/ios (manual)
  • dialpad/firespotter (manual)
  • dialpad/semantic-release-changelog-json (auto-detected)
  • dialpad/conventional-changelog-angular (auto-detected)

Comment on lines +361 to +366
// The leading and trailing slots sit in normal flow inside the viewport, so the item
// wrapper does not start at offset 0. Widen the window by their measured size.
const slotSize = props.direction === 'vertical' ? 'scrollHeight' : 'scrollWidth';
if (before.value) scroll.start -= before.value[slotSize];
if (after.value) scroll.end += after.value[slotSize];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use the leading slot offset for both buffered bounds.

The item wrapper starts after before, so both scroll.start and scroll.end must subtract the before size. Adding after to scroll.end expands the item window beyond the viewport. A tall after slot can activate the final item too early, emit scroll-end before the documented threshold, and hit the 1000-view guard.

Proposed fix
-    if (before.value) scroll.start -= before.value[slotSize];
-    if (after.value) scroll.end += after.value[slotSize];
+    const beforeSize = before.value?.[slotSize] ?? 0;
+    scroll.start -= beforeSize;
+    scroll.end -= beforeSize;
📝 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
// The leading and trailing slots sit in normal flow inside the viewport, so the item
// wrapper does not start at offset 0. Widen the window by their measured size.
const slotSize = props.direction === 'vertical' ? 'scrollHeight' : 'scrollWidth';
if (before.value) scroll.start -= before.value[slotSize];
if (after.value) scroll.end += after.value[slotSize];
// The leading and trailing slots sit in normal flow inside the viewport, so the item
// wrapper does not start at offset 0. Widen the window by their measured size.
const slotSize = props.direction === 'vertical' ? 'scrollHeight' : 'scrollWidth';
const beforeSize = before.value?.[slotSize] ?? 0;
scroll.start -= beforeSize;
scroll.end -= beforeSize;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue` around
lines 361 - 366, Update the buffered bounds calculation in the scroller’s
scroll-window logic to subtract the measured leading-slot size from both
scroll.start and scroll.end. Remove the trailing-slot addition to scroll.end,
while preserving the existing direction-specific slotSize measurement.

Comment thread packages/dialtone-vue/components/scroller/scroller.test.js Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (4)
packages/dialtone-vue/components/scroller/scroller.test.js (1)

95-98: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use one assertion in each test.

Split each changed test that has multiple assertions. Keep setup or event preconditions in separate tests where needed.

Also applies to: 149-156, 197-216, 243-256

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/scroller.test.js` around lines 95 -
98, Split the affected tests in the scroller test suite, including the test
around buffer prop forwarding and the additional ranges noted, so each test
contains exactly one assertion. Preserve the existing setup, event
preconditions, and expected values, moving related assertions into separate
clearly named tests.

Sources: Coding guidelines, Path instructions

packages/dialtone-vue/components/scroller/modules/core_scroller.vue (2)

55-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Render empty only when items is empty.

The empty slot renders with populated item views. This displays empty-state content and list content together.

  • packages/dialtone-vue/components/scroller/modules/core_scroller.vue#L55-L57: Guard the slot with v-if="!items.length".
  • packages/dialtone-vue/components/scroller/scroller.test.js#L211-L216: Mount with an empty items array and assert the empty-state output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue` around
lines 55 - 57, Guard the empty slot in core_scroller.vue with !items.length so
it renders only when no items exist. Add or update the scroller test in
packages/dialtone-vue/components/scroller/scroller.test.js at lines 211-216 to
mount with an empty items array and assert the empty-state output.

361-365: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Map both virtual bounds into the item-wrapper coordinate system.

before shifts the item wrapper. Subtract its size from both scroll.start and scroll.end. Do not add the after size because it does not shift item coordinates.

  • packages/dialtone-vue/components/scroller/modules/core_scroller.vue#L361-L365: Subtract the leading-slot size from both buffered bounds and remove the trailing-slot adjustment.
  • packages/dialtone-vue/components/scroller/scroller.test.js#L225-L257: Mock a distinct leading-slot size and assert the expected item range after coordinate conversion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue` around
lines 361 - 365, Update the coordinate conversion in
packages/dialtone-vue/components/scroller/modules/core_scroller.vue#L361-L365 to
subtract the measured leading-slot size from both scroll.start and scroll.end,
and remove the trailing-slot adjustment. In
packages/dialtone-vue/components/scroller/scroller.test.js#L225-L257, mock a
distinct leading-slot size and assert the resulting item range uses both
converted bounds.
packages/dialtone-vue/components/scroller/scroller.vue (1)

68-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Normalize invalid buffer values before forwarding them.

buffer accepts negative and non-finite numbers. CoreScroller uses the value in render-range arithmetic. This can create invalid ranges or hit the 1,000-item guard.

A Vue prop validator only produces a development warning. It does not replace the supplied value. (vuejs.org) Add a finite, non-negative validator and forward a normalized fallback of 200.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/scroller.vue` around lines 68 - 76,
Update the buffer prop definition in Scroller to validate only finite,
non-negative numbers, then normalize invalid supplied values to 200 before
passing buffer into CoreScroller. Preserve the existing default of 200 for
omitted values and ensure CoreScroller never receives negative or non-finite
buffer values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/dialtone-vue/components/scroller/scroller.stories.js`:
- Around line 108-123: Update the descriptions for the scroll-start and
scroll-end event entries in the Scroller stories to use direction-neutral
“viewport start” and “viewport end” edge wording instead of “top” and “bottom,”
while preserving the existing buffer-length behavior and append-items guidance.

In `@packages/dialtone-vue/components/scroller/scroller.test.js`:
- Around line 149-172: Update both boundary-event tests around handleScroll to
be async, await each wrapper.trigger('scroll') call, and await one
animation-frame tick before asserting emitted scroll-end or scroll-start events.
Preserve the existing scroll positions and assertion logic.
- Around line 197-221: The scroller slot and empty-item tests currently rely on
styling classes; update the affected tests, including the referenced ranges, to
query stable data-qa markers for before/after slot wrappers and recycled item
wrappers. Replace classList assertions with data-qa-based checks while
preserving the existing rendering expectations and no-wrapper behavior.

---

Duplicate comments:
In `@packages/dialtone-vue/components/scroller/modules/core_scroller.vue`:
- Around line 55-57: Guard the empty slot in core_scroller.vue with
!items.length so it renders only when no items exist. Add or update the scroller
test in packages/dialtone-vue/components/scroller/scroller.test.js at lines
211-216 to mount with an empty items array and assert the empty-state output.
- Around line 361-365: Update the coordinate conversion in
packages/dialtone-vue/components/scroller/modules/core_scroller.vue#L361-L365 to
subtract the measured leading-slot size from both scroll.start and scroll.end,
and remove the trailing-slot adjustment. In
packages/dialtone-vue/components/scroller/scroller.test.js#L225-L257, mock a
distinct leading-slot size and assert the resulting item range uses both
converted bounds.

In `@packages/dialtone-vue/components/scroller/scroller.test.js`:
- Around line 95-98: Split the affected tests in the scroller test suite,
including the test around buffer prop forwarding and the additional ranges
noted, so each test contains exactly one assertion. Preserve the existing setup,
event preconditions, and expected values, moving related assertions into
separate clearly named tests.

In `@packages/dialtone-vue/components/scroller/scroller.vue`:
- Around line 68-76: Update the buffer prop definition in Scroller to validate
only finite, non-negative numbers, then normalize invalid supplied values to 200
before passing buffer into CoreScroller. Preserve the existing default of 200
for omitted values and ensure CoreScroller never receives negative or non-finite
buffer values.
🪄 Autofix

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: Repository YAML (base), Central YAML (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 94f57528-8799-4a21-89e8-eb329f8f5ff5

📥 Commits

Reviewing files that changed from the base of the PR and between bfc478c and f7ebf24.

📒 Files selected for processing (7)
  • apps/dialtone-documentation/docs/components/scroller.md
  • packages/dialtone-vue/components/scroller/modules/core_scroller.vue
  • packages/dialtone-vue/components/scroller/modules/dynamic_scroller.vue
  • packages/dialtone-vue/components/scroller/scroller.stories.js
  • packages/dialtone-vue/components/scroller/scroller.test.js
  • packages/dialtone-vue/components/scroller/scroller.vue
  • packages/dialtone-vue/components/scroller/scroller_default.story.vue
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • dialpad/ios (manual)
  • dialpad/firespotter (manual)
  • dialpad/semantic-release-changelog-json (auto-detected)
  • dialpad/conventional-changelog-angular (auto-detected)

Comment thread packages/dialtone-vue/components/scroller/scroller.stories.js
Comment on lines +149 to +172
it('`scroll-end` when the last item enters the render pool', () => {
expect(wrapper.emitted()['scroll-end']).toBeUndefined();

defaultContent.element.scrollTop =
defaultContent.element.scrollHeight - defaultContent.element.clientHeight;
wrapper.trigger('scroll');

expect(wrapper.emitted()['scroll-end']).toBeTruthy();
});

it('`scroll-start` when the first item re-enters the render pool', () => {
// 240 is chosen so the window starts at index 1 (240 - buffer 200 = 40, over
// itemSize 30) while still overlapping the mount window — the algorithm only
// releases off-window views on a continuous move, and a view must be released
// before it can count as newly used again.
defaultContent.element.scrollTop = 240;
wrapper.trigger('scroll');

const beforeReturn = wrapper.emitted()['scroll-start'].length;

defaultContent.element.scrollTop = 0;
wrapper.trigger('scroll');

expect(wrapper.emitted()['scroll-start'].length).toBeGreaterThan(beforeReturn);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Wait for the scheduled render update before asserting events.

handleScroll() updates the pool in requestAnimationFrame(). These assertions run before that callback can emit either boundary event. Make both tests async, await wrapper.trigger('scroll'), then await one animation frame before asserting.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/scroller/scroller.test.js` around lines 149
- 172, Update both boundary-event tests around handleScroll to be async, await
each wrapper.trigger('scroll') call, and await one animation-frame tick before
asserting emitted scroll-end or scroll-start events. Preserve the existing
scroll positions and assertion logic.

Comment thread packages/dialtone-vue/components/scroller/scroller.test.js Outdated
@iropolo Ignacio Ropolo (iropolo) added the no-visual-test Add this tag when the PR does not need visual testing label Aug 6, 2026

@braddialpad Brad Paugh (braddialpad) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Restoring the upstream items watcher in core_scroller.vue would remove the need for consumers to call updateItems() at all. It was deliberately left out of scope here: it is a behaviour change to a shared component and deserves its own ticket. The commented-out original shows why it was disabled — prepending and appending need different recompute calls to avoid the scroll position jumping, and only the caller knows which happened."

I've generated a ticket for this: https://dialpad.atlassian.net/browse/DLT-3563

Solid PR, works really well, clean code

Comment thread packages/dialtone-vue/components/scroller/scroller.test.js Outdated
@joandvgv Joan Gil (joandvgv) added visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests no-visual-test Add this tag when the PR does not need visual testing and removed no-visual-test Add this tag when the PR does not need visual testing visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests labels Aug 6, 2026
@joandvgv Joan Gil (joandvgv) self-assigned this Aug 6, 2026

@iropolo Ignacio Ropolo (iropolo) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!!

@joandvgv
Joan Gil (joandvgv) merged commit f2e1f86 into staging Aug 7, 2026
6 checks passed
@joandvgv
Joan Gil (joandvgv) deleted the DLT-2624 branch August 7, 2026 14:55
Brad Paugh (braddialpad) pushed a commit that referenced this pull request Aug 7, 2026
# [3.224.0](dialtone-vue/v3.223.2...dialtone-vue/v3.224.0) (2026-08-07)

### Features

* **Scroller:** DLT-2624 add before/after/empty slots and scroll-end events ([#1385](#1385)) ([f2e1f86](f2e1f86))
Brad Paugh (braddialpad) pushed a commit that referenced this pull request Aug 7, 2026
# [9.190.0](dialtone/v9.189.1...dialtone/v9.190.0) (2026-08-07)

### Features

* **Scroller:** DLT-2624 add before/after/empty slots and scroll-end events ([#1385](#1385)) ([f2e1f86](f2e1f86))
Brad Paugh (braddialpad) pushed a commit that referenced this pull request Aug 10, 2026
# [10.0.0-next.11](dialtone/v10.0.0-next.10...dialtone/v10.0.0-next.11) (2026-08-10)

### Bug Fixes

* **Avatar:** DP-200415 prevent presence indicators from clipping avatar content ([#1387](#1387)) ([e693f80](e693f80))
* **Message Input:** DP-200077 disable text-align when rich-text is false ([#1384](#1384)) ([6474c8e](6474c8e))

### Features

* **Icons:** DLT-3553 add fax-inbound and fax-outbound icons ([#1378](#1378)) ([46538f6](46538f6))
* **Scroller:** DLT-2624 add before/after/empty slots and scroll-end events ([#1385](#1385)) ([f2e1f86](f2e1f86))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-visual-test Add this tag when the PR does not need visual testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants