Skip to content

fix(scroller): don't throw from scroll operations before an element is set - #324

Merged
dadajam4 merged 3 commits into
mainfrom
feat/FABRIC_DESIGN-1746
Oct 19, 2026
Merged

dadajam4 merged 3 commits into
mainfrom
feat/FABRIC_DESIGN-1746

Conversation

@schwarz9791

@schwarz9791 schwarz9791 commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Scroller threw missing element when it was operated before an element had been set. @fastkit/vue-scroller reaches this: useScrollerControl({ el: 'self' }) (and VScroller) only sets the element in onMounted, so while a <Suspense> boundary is still pending, the control exists but its Scroller has no element yet.

Scroll operations

by(), to(), toElement(), toSide(), toTop() and the other side/corner methods built their options through _checkElement(), which threw. All of them go through one private method, which now returns a result that does nothing and resolves immediately when there is no element (no onCancel / onDone). The options are only built after that check.

Scroll stoppers

pushScrollStopper() / removeScrollStopper() threw the same way via _updateScrollable(). It now skips when there is no element; the stopper stays recorded and _setup() applies it once the element is set. On a destroyed instance pushScrollStopper() ignores the stopper, so scrollEnabled stays as it is.

Since destroy() drops the element, these calls after destroy() also do nothing instead of throwing. This matches the existing setters (scrollTop etc.), which already return when there is no element.

Tests

  • packages/scroller/src/__tests__/scroll-without-element.client.spec.ts
    • every scroll operation on a Scroller without an element: does not throw, promise resolves, plans is { x: 0, y: 0 }
    • a stopper pushed before the element is set locks scrolling once the element is set, and removing it unlocks it
    • a scroll operation after destroy(): promise resolves, plans is { x: 0, y: 0 }
    • pushScrollStopper() after destroy(): scrollEnabled stays true

All of them fail before the fix and pass after it.

Release

  • @fastkit/scroller patch
  • @fastkit/vue-scroller patch, so its published range becomes ^1.0.2 and updating it brings the fix along (see "Delivering a fix in a dependency to its dependents" in docs/release.md)

🤖 Generated with Claude Code

https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6

…s set

Scroll operations (`by()`, `to()`, `toElement()`, `toSide()`, `toTop()`
etc.) built their options through `_checkElement()`, which threw `missing
element` when no element had been set. `useScrollerControl({ el: 'self' })`
only sets the element in `onMounted`, so calling them while a `<Suspense>`
boundary was still pending threw. They now return a result that does
nothing and resolves immediately.

`pushScrollStopper()` / `removeScrollStopper()` threw the same way. A
stopper pushed before the element is set is now kept and applied once the
element is set.

Release @fastkit/vue-scroller as well so its published range requires the
fixed @fastkit/scroller.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6
@changeset-bot

changeset-bot Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 946da15

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@schwarz9791 schwarz9791 self-assigned this Oct 16, 2026

@schwarz9791 schwarz9791 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

コードレビューで見つかった指摘を検証し、残ったものをインラインでまとめました。ブロッカーはありません。

Comment thread packages/scroller/src/scroller.ts
Comment thread packages/scroller/src/scroller.ts Outdated
…ing it

`_checkElement()` could no longer throw: every caller ran only after
`_runScroll()`, `_updateScrollable()` or `destroy()` had made sure an
element exists. Pass that element to the option builders as an argument
and inline `_scrollEnable()` / `_scrollDisable()`, so code that needs the
element has to get it from one of those guards.

Also cover scroll operations after `destroy()` in the tests.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6

@schwarz9791 schwarz9791 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

76a6dc9 を再レビューしました。指摘への対応はどれも妥当で、追加の指摘はありません。scroller の typecheck / lint / test(16 件)が通ることも手元で確認しました。

@schwarz9791
schwarz9791 marked this pull request as ready for review October 16, 2026 05:02
Comment thread packages/scroller/src/scroller.ts
Comment thread packages/scroller/src/scroller.ts
Comment thread packages/scroller/src/scroller.ts
Comment thread packages/scroller/src/scroller.ts
Comment thread packages/scroller/src/scroller.ts

@nkenji09 nkenji09 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

この PR が入れた不具合は見つかりませんでした。PR の前からある不具合を 5 件、参考としてインラインに添えています(別対応で構いません)。

`pushScrollStopper()` on a destroyed instance no longer throws since the
element check was relaxed, but it still recorded the stopper and turned
`scrollEnabled` false. Ignore it instead, like the other calls after
`destroy()`. `removeScrollStopper()` already does nothing then, as
`destroy()` empties the stoppers.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6
@dadajam4
dadajam4 merged commit fa293c4 into main Oct 19, 2026
1 check passed
@dadajam4
dadajam4 deleted the feat/FABRIC_DESIGN-1746 branch October 19, 2026 00:25
@github-actions github-actions Bot mentioned this pull request Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants