feat(new-channels): improved scrolling stability#2433
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors channel tab state management using Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
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/notifications/notification-navigation.ts (1)
67-79:⚠️ Potential issue | 🟡 Minor
goToLocationFromParamsis called unconditionally, causing redundant navigation for new splits.When
openSplitIfNotOpencreates a new split, it already passesparamsas initial props (line 69). The subsequentgoToLocationFromParamscall (line 79) then triggers a second navigation to the same location. The comment at lines 74-75 indicates this was intended only for "already-open channels."Consider guarding the call:
Proposed fix
- openSplitIfNotOpen(layoutManager, 'channel', channelId, { + const existing = layoutManager.getSplitByContent('channel', channelId); + openSplitIfNotOpen(layoutManager, 'channel', channelId, { newSplit, params, }); if (!messageId) return; // Also call goToLocationFromParams for already-open channels where // the split existed before and params weren't applied as initial props. + if (!existing) return; + const orchestrator = layoutManager.getOrchestrator(); const handle = await orchestrator.getBlockHandle(channelId, 'channel'); handle?.goToLocationFromParams(getChannelParams(messageId, threadId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/notifications/notification-navigation.ts` around lines 67 - 79, The code always calls handle?.goToLocationFromParams even when openSplitIfNotOpen just created a new split and already applied params; change this so goToLocationFromParams is only invoked for channels that were already open. Specifically, after calling openSplitIfNotOpen(layoutManager, 'channel', channelId, { newSplit, params }), guard the subsequent orchestrator.getBlockHandle(...) and handle?.goToLocationFromParams(getChannelParams(messageId, threadId)) behind a check that the split was not newly created (i.e., only when newSplit is false/undefined), keeping the early return for !messageId.
🤖 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/channel/Channel/ThreadList.tsx`:
- Around line 162-163: The 'top' position check in ThreadList.tsx currently
reuses NEAR_BOTTOM_THRESHOLD for handle.scrollOffset which is confusing;
introduce a dedicated constant NEAR_TOP_POSITION_THRESHOLD (or a shared
NEAR_EDGE_THRESHOLD if you prefer a single name) and replace the 'top' case
return to use NEAR_TOP_POSITION_THRESHOLD; update any nearby comments and
imports/usages (e.g., the switch case handling 'top' that returns
handle.scrollOffset <= NEAR_BOTTOM_THRESHOLD) so the check is clear and
maintainable.
- Around line 313-320: The RAF callback can run after unmount and operate on a
stale virtualizer handle (captured `handle`), so make the callback defensive:
when scheduling the requestAnimationFrame in ThreadList (the block that calls
`requestAnimationFrame(() => { const retryScrolled = scrollToTarget(handle,
initialScrollTarget); ... })`) store the RAF id and either cancel it on unmount
or check a mounted/valid flag before calling `scrollToTarget` and
`completeInitialScroll`; specifically, add a guard that verifies the component
is still mounted and `handle` is still valid (or use a boolean ref like
`isMountedRef.current`) inside the RAF callback and cancel the RAF in the
cleanup to avoid touching a stale `handle` after unmount.
In `@js/app/packages/queries/channel/thread-replies.ts`:
- Line 35: The placeholderData callback currently types its parameter as (prev:
ApiThreadReply[]) but TanStack Query v5 passes previousValue which can be
undefined; update the annotation on placeholderData’s parameter to accept
ApiThreadReply[] | undefined (or use an optional param prev?: ApiThreadReply[])
and handle the undefined case inside the callback so code does not assume an
array when previousValue is missing.
---
Outside diff comments:
In `@js/app/packages/notifications/notification-navigation.ts`:
- Around line 67-79: The code always calls handle?.goToLocationFromParams even
when openSplitIfNotOpen just created a new split and already applied params;
change this so goToLocationFromParams is only invoked for channels that were
already open. Specifically, after calling openSplitIfNotOpen(layoutManager,
'channel', channelId, { newSplit, params }), guard the subsequent
orchestrator.getBlockHandle(...) and
handle?.goToLocationFromParams(getChannelParams(messageId, threadId)) behind a
check that the split was not newly created (i.e., only when newSplit is
false/undefined), keeping the early return for !messageId.
🪄 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: 7e060cb1-b162-4d12-880b-aef46819b2ae
📒 Files selected for processing (6)
js/app/packages/block-channel/component/NewChannelBlockAdapter.tsxjs/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Channel/ThreadList.tsxjs/app/packages/channel/Channel/create-target-message-controller.tsjs/app/packages/notifications/notification-navigation.tsjs/app/packages/queries/channel/thread-replies.ts
| case 'top': | ||
| return handle.scrollOffset <= NEAR_BOTTOM_THRESHOLD; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a dedicated threshold for the 'top' position check.
NEAR_BOTTOM_THRESHOLD (50px) is reused for the 'top' case, which may be confusing and could need different tolerances. Consider extracting a shared constant or creating NEAR_TOP_POSITION_THRESHOLD for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Channel/ThreadList.tsx` around lines 162 - 163, The
'top' position check in ThreadList.tsx currently reuses NEAR_BOTTOM_THRESHOLD
for handle.scrollOffset which is confusing; introduce a dedicated constant
NEAR_TOP_POSITION_THRESHOLD (or a shared NEAR_EDGE_THRESHOLD if you prefer a
single name) and replace the 'top' case return to use
NEAR_TOP_POSITION_THRESHOLD; update any nearby comments and imports/usages
(e.g., the switch case handling 'top' that returns handle.scrollOffset <=
NEAR_BOTTOM_THRESHOLD) so the check is clear and maintainable.
| requestAnimationFrame(() => { | ||
| const retryScrolled = scrollToTarget(handle, initialScrollTarget); | ||
| if (!retryScrolled) { | ||
| // Target disappeared between mount and retry — finalize now since | ||
| // no scroll events will fire to trigger another onScrollEnd. | ||
| completeInitialScroll(handle); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
RAF callback may execute after component unmount.
The requestAnimationFrame callback captures handle from line 292. If the component unmounts before the RAF fires, this could attempt operations on a stale virtualizer handle. Consider adding a guard:
Proposed defensive check
requestAnimationFrame(() => {
+ if (didInitialScroll()) return;
const retryScrolled = scrollToTarget(handle, initialScrollTarget);
if (!retryScrolled) {
// Target disappeared between mount and retry — finalize now since
// no scroll events will fire to trigger another onScrollEnd.
completeInitialScroll(handle);
}
});📝 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.
| requestAnimationFrame(() => { | |
| const retryScrolled = scrollToTarget(handle, initialScrollTarget); | |
| if (!retryScrolled) { | |
| // Target disappeared between mount and retry — finalize now since | |
| // no scroll events will fire to trigger another onScrollEnd. | |
| completeInitialScroll(handle); | |
| } | |
| }); | |
| requestAnimationFrame(() => { | |
| if (didInitialScroll()) return; | |
| const retryScrolled = scrollToTarget(handle, initialScrollTarget); | |
| if (!retryScrolled) { | |
| // Target disappeared between mount and retry — finalize now since | |
| // no scroll events will fire to trigger another onScrollEnd. | |
| completeInitialScroll(handle); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Channel/ThreadList.tsx` around lines 313 - 320, The
RAF callback can run after unmount and operate on a stale virtualizer handle
(captured `handle`), so make the callback defensive: when scheduling the
requestAnimationFrame in ThreadList (the block that calls
`requestAnimationFrame(() => { const retryScrolled = scrollToTarget(handle,
initialScrollTarget); ... })`) store the RAF id and either cancel it on unmount
or check a mounted/valid flag before calling `scrollToTarget` and
`completeInitialScroll`; specifically, add a guard that verifies the component
is still mounted and `handle` is still valid (or use a boolean ref like
`isMountedRef.current`) inside the RAF callback and cancel the RAF in the
cleanup to avoid touching a stale `handle` after unmount.
1 participant
onScrollEndand does one retry if neccesaryload_around_message_idcache not clearing