fix: channels blanking and weird virtua scroll bug#2227
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughBumped Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/block-channel/component/MessageList/MessageList.tsx (1)
511-516: 🧹 Nitpick | 🔵 TrivialConsider using
scrollTo(handle.scrollSize)for clearer intent.Using
scrollBy(Number.MAX_SAFE_INTEGER)works because virtua clamps the value, but it's semantically unclear. If the virtualizer provides ascrollSizeproperty, usingscrollTo(handle.scrollSize)would be more explicit about scrolling to the end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-channel/component/MessageList/MessageList.tsx` around lines 511 - 516, Replace the semantically-unclear use of virtualHandle()?.scrollBy(Number.MAX_SAFE_INTEGER) in MessageList.tsx with an explicit call that scrolls to the virtualizer's total scroll size: obtain the virtual handle via virtualHandle() and call its scrollTo method with the handle's scrollSize (i.e., use scrollTo(handle.scrollSize) instead of scrollBy(Number.MAX_SAFE_INTEGER)); update the branch in which forceBottom / isNearBottom() triggers the scroll so it uses virtualHandle().scrollTo(virtualHandle().scrollSize) and keep the early return behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/block-channel/component/MessageList/MessageList.tsx`:
- Around line 265-266: Remove the dead/commented-out call and the unused helper:
delete the single-line comment "// scrollElementIntoView(targetEl, align);" near
the call to targetEl.scrollIntoView({ block: align }); and remove the entire
scrollElementIntoView function definition (the function named
scrollElementIntoView) since it is no longer referenced anywhere; ensure no
other references to scrollElementIntoView remain after deletion.
---
Outside diff comments:
In `@js/app/packages/block-channel/component/MessageList/MessageList.tsx`:
- Around line 511-516: Replace the semantically-unclear use of
virtualHandle()?.scrollBy(Number.MAX_SAFE_INTEGER) in MessageList.tsx with an
explicit call that scrolls to the virtualizer's total scroll size: obtain the
virtual handle via virtualHandle() and call its scrollTo method with the
handle's scrollSize (i.e., use scrollTo(handle.scrollSize) instead of
scrollBy(Number.MAX_SAFE_INTEGER)); update the branch in which forceBottom /
isNearBottom() triggers the scroll so it uses
virtualHandle().scrollTo(virtualHandle().scrollSize) and keep the early return
behavior intact.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2eb8ce9e-9b3e-4c09-ba71-ec8f3a093a59
⛔ Files ignored due to path filters (1)
js/app/bun.lockis excluded by!**/*.lock,!**/bun.lock
📒 Files selected for processing (2)
js/app/package.jsonjs/app/packages/block-channel/component/MessageList/MessageList.tsx
js/app/packages/block-channel/component/MessageList/MessageList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/block-channel/component/MessageList/MessageList.tsx`:
- Around line 467-468: Replace the call to
virtualHandle()?.scrollBy(Number.MAX_SAFE_INTEGER) with an index-based scroll
using virtualHandle()?.scrollToIndex(...); specifically call
scrollToIndex(topLevelMessages().length - 1, { align: 'end' }) so the
virtualizer scrolls to the last message reliably; ensure you handle the
empty-list case (only call when topLevelMessages().length > 0) and keep using
the same virtualHandle() reference in the MessageList component/function.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 721973c6-4981-45c1-910b-de8a82beba1a
📒 Files selected for processing (1)
js/app/packages/block-channel/component/MessageList/MessageList.tsx
| virtualHandle()?.scrollBy(Number.MAX_SAFE_INTEGER); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
virtua scrollBy large value scroll to bottom
💡 Result:
In virtual scrolling implementations (Angular CDK, TanStack Virtual, react-window/react-virtualized), using scrollBy or scrollToOffset/scrollTo with a very large pixel value to reach the bottom often fails due to precision limitations in JavaScript numbers (especially for lists with millions of items where total height exceeds ~1e6-1e15 px depending on browser/engine). Instead, use index-based scrolling methods which internally compute the correct offset. Key solutions: 1. Angular CDK Virtual Scroll (most common context): - Use viewport.scrollToIndex(items.length - 1) to scroll to the last item. - For bottom: viewport.scrollTo({ bottom: 0 }) - call twice with setTimeout if needed due to async rendering. - Example: setTimeout( => { this.viewport.scrollTo({ bottom: 0, behavior: 'auto' }); }, 0); setTimeout( => { this.viewport.scrollTo({ bottom: 0, behavior: 'auto' }); }, 50); 2. TanStack Virtual / react-window: - virtualizer.scrollToIndex(items.length - 1, { align: 'end' }) - For custom scrollToFn, implement smooth scrolling but use index, not raw offset. 3. General pattern for large lists: - Avoid raw pixel offsets >1e6 px; use scrollToIndex(lastIndex) or scrollToItem({index: lastIndex, align: 'end'}). - For dynamic heights, may need iterative refinement: guess offset = index * avgHeight, scroll, check renderedRange, adjust. Issues with large scrollBy values: - JS number precision loss (float64 mantissa ~53 bits). - Browser scroll recalculation errors. - Seen in react-virtualized #1537 (scrollToRow inaccurate for 1B+ items), TanStack issues with dynamic heights. Prefer official docs/methods over manual scrollBy(totalHeight).
Citations:
- 1: https://stackoverflow.com/questions/64932671/scroll-to-bottom-with-cdk-virtual-scroll-angular-8
- 2: https://stackoverflow.com/questions/77638790/custom-scrolltoindex-for-mixed-height-items-in-a-virtual-scroll-viewport
- 3: https://pdanpdan.github.io/virtual-scroll/docs/
- 4: https://tanstack.com/virtual/latest/docs/framework/react/examples/smooth-scroll?from=reactVirtualV2
- 5: Grid.scrollToCell/scrollToRow does not scroll to a proper location on a very large list bvaughn/react-virtualized#1537
- 6: scrollToIndex doesn't work correctly with dynamic height TanStack/virtual#468
- 7: https://v12.material.angular.io/docs-content/overviews/cdk/scrolling/scrolling
- 8: https://angularindepth.com/posts/1091/writing-custom-virtual-scroll-strategy
🏁 Script executed:
# Find the file and examine the context around lines 467-468
find . -name "MessageList.tsx" -path "*/block-channel/component/MessageList/*" | head -5Repository: macro-inc/macro
Length of output: 128
🏁 Script executed:
# Check virtua package in the repo
fd -e json -e tsx -e ts | xargs rg -l "virtua" | head -10Repository: macro-inc/macro
Length of output: 661
🏁 Script executed:
# Look for package.json files that might define virtua version
fd -name "package.json" | xargs rg -l "virtua" 2>/dev/nullRepository: macro-inc/macro
Length of output: 3171
🏁 Script executed:
# Read the MessageList.tsx file, focusing on the relevant section
cat -n js/app/packages/block-channel/component/MessageList/MessageList.tsx | sed -n '450,480p'Repository: macro-inc/macro
Length of output: 1333
🏁 Script executed:
# Check imports at the top of the file to understand virtualHandle
cat -n js/app/packages/block-channel/component/MessageList/MessageList.tsx | head -60Repository: macro-inc/macro
Length of output: 2503
🏁 Script executed:
# Check the virtua patch to see what's modified
cat js/app/patches/virtua@0.48.1.patchRepository: macro-inc/macro
Length of output: 6132
🏁 Script executed:
# Look for scrollToIndex usage in the codebase
rg "scrollToIndex" --type ts --type tsxRepository: macro-inc/macro
Length of output: 86
🏁 Script executed:
# Search for scrollToIndex in the codebase (without -n flag)
rg "scrollToIndex" --type-listRepository: macro-inc/macro
Length of output: 448
🏁 Script executed:
# Try again with correct syntax
rg "scrollToIndex"Repository: macro-inc/macro
Length of output: 5041
🏁 Script executed:
# Look at what methods are called on virtualHandle in the file
rg "virtualHandle\(\)" js/app/packages/block-channel/component/MessageList/MessageList.tsx -A 2Repository: macro-inc/macro
Length of output: 1367
🏁 Script executed:
# Check if scrollToIndex is documented in virtua types
rg "scrollToIndex" js/app/node_modules/virtua 2>/dev/null | head -20Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# Get more context around the commented-out scrollToIndex line
cat -n js/app/packages/block-channel/component/MessageList/MessageList.tsx | sed -n '475,510p'Repository: macro-inc/macro
Length of output: 1340
🏁 Script executed:
# Check git log to see when/why scrollToIndex was commented out
cd js/app && git log --oneline -p -- packages/block-channel/component/MessageList/MessageList.tsx 2>/dev/null | grep -A 10 -B 10 "scrollToIndex.*commented\|align: 'end'" | head -50Repository: macro-inc/macro
Length of output: 569
🏁 Script executed:
# Search for any issues or TODOs related to scroll behavior
rg "TODO|FIXME|BUG|ISSUE" js/app/packages/block-channel/component/MessageList/MessageList.tsx -B 2 -A 2Repository: macro-inc/macro
Length of output: 356
Use scrollToIndex instead of scrollBy(Number.MAX_SAFE_INTEGER) for scrolling to bottom.
In virtua, index-based scrolling is the standard pattern and avoids precision issues with large pixel values. The codebase uses scrollToIndex consistently across 40+ other components. This exact pattern is even present in the same function (commented out with a "TODO"), confirming it's the intended approach:
Suggested replacement
virtualHandle()?.scrollToIndex(topLevelMessages().length - 1, { align: 'end' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-channel/component/MessageList/MessageList.tsx` around
lines 467 - 468, Replace the call to
virtualHandle()?.scrollBy(Number.MAX_SAFE_INTEGER) with an index-based scroll
using virtualHandle()?.scrollToIndex(...); specifically call
scrollToIndex(topLevelMessages().length - 1, { align: 'end' }) so the
virtualizer scrolls to the last message reliably; ensure you handle the
empty-list case (only call when topLevelMessages().length > 0) and keep using
the same virtualHandle() reference in the MessageList component/function.
2 participants
Uh oh!
There was an error while loading. Please reload this page.