-
Notifications
You must be signed in to change notification settings - Fork 77
fix(provider): added auto-scroll fix #2127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe ActivityLogDetails component now manages scroll-follow state per task. This allows the component to track whether users are at the bottom of each task's log and automatically maintain or release the auto-follow behavior accordingly. Changes
Sequence DiagramsequenceDiagram
actor User
participant ActivityLogDetails
participant LazyLog
participant ScrollFollow
User->>LazyLog: Scroll in log
LazyLog->>ActivityLogDetails: onScroll event
ActivityLogDetails->>ActivityLogDetails: Detect bottom reach
alt At bottom
ActivityLogDetails->>ActivityLogDetails: Set followScroll = true
else Not at bottom
ActivityLogDetails->>ActivityLogDetails: Set followScroll = false
end
ActivityLogDetails->>ScrollFollow: Pass followScroll as startFollowing
ScrollFollow->>User: Auto-follow or release scroll behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2127 +/- ##
==========================================
- Coverage 46.42% 46.06% -0.36%
==========================================
Files 1016 1006 -10
Lines 28692 28343 -349
Branches 7462 7421 -41
==========================================
- Hits 13319 13056 -263
+ Misses 15060 14983 -77
+ Partials 313 304 -9
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/provider-console/src/components/shared/ActivityLogDetails.tsx (2)
188-188: Consider clearer default handling.The logic is correct but the double negative pattern (
!== false) is less readable. Consider using nullish coalescing for clarity.Apply this diff for improved readability:
- const shouldFollow = followScroll[taskId] !== false; // Default to true, but allow user to disable + const shouldFollow = followScroll[taskId] ?? true; // Default to true unless explicitly disabled
20-20: Consider cleanup of followScroll state entries.The
followScrollstate accumulates entries for all tasks but never removes them when tasks complete or accordions close. While not immediately critical, this could lead to memory growth in long-running sessions with many tasks.Consider clearing entries when accordions close:
const handleAccordionToggle = (index: number, task: Task) => { setOpenAccordions(prev => { const newState = [...prev]; newState[index] = !newState[index]; if (newState[index]) { if (task.status === "in_progress") { setupLogStream(task.id); } else if (!taskLogs[task.id]) { fetchTaskLogs(task.id); } } else { logStreams.current[task.id]?.close(); logStreams.current[task.id] = null; // Clean up followScroll state when closing accordion setFollowScroll(prev => { const updated = { ...prev }; delete updated[task.id]; return updated; }); } return newState; }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/net/src/generated/netConfigData.tsis excluded by!**/generated/**
📒 Files selected for processing (1)
apps/provider-console/src/components/shared/ActivityLogDetails.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/provider-console/src/components/shared/ActivityLogDetails.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/provider-console/src/components/shared/ActivityLogDetails.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: validate (packages) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (1)
apps/provider-console/src/components/shared/ActivityLogDetails.tsx (1)
20-20: Good addition of per-task scroll state.The
followScrollstate properly tracks scroll-follow behavior for each task individually, which aligns with the PR objective.
| onScroll={({ scrollTop, scrollHeight, clientHeight }) => { | ||
| // Check if user has scrolled to the bottom | ||
| const isAtBottom = scrollTop + clientHeight >= scrollHeight - 10; // 10px threshold | ||
|
|
||
| // Update follow state based on scroll position | ||
| setFollowScroll(prev => ({ | ||
| ...prev, | ||
| [taskId]: isAtBottom | ||
| })); | ||
|
|
||
| // Call the original onScroll handler | ||
| onScroll({ scrollTop, scrollHeight, clientHeight }); | ||
| }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Optimize scroll handler performance and verify ScrollFollow behavior.
The onScroll handler updates state on every scroll event, which fires continuously during scrolling and may cause performance issues.
Additionally, verify that the ScrollFollow component respects the startFollowing prop on re-renders. If it only uses startFollowing during initial mount and maintains internal state thereafter, updating followScroll state may not affect the auto-follow behavior as intended.
Performance optimization: Consider throttling or debouncing the state updates:
onScroll={({ scrollTop, scrollHeight, clientHeight }) => {
const isAtBottom = scrollTop + clientHeight >= scrollHeight - 10;
// Only update state if the value actually changes
setFollowScroll(prev => {
if (prev[taskId] !== isAtBottom) {
return { ...prev, [taskId]: isAtBottom };
}
return prev;
});
onScroll({ scrollTop, scrollHeight, clientHeight });
}}Verification: Please test that toggling auto-follow works correctly by:
- Scrolling up in an active log stream (should disable auto-follow)
- Scrolling back to bottom (should re-enable auto-follow)
- Verifying new logs appear automatically when at bottom
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine ActivityLogDetails.tsx
fd --type f "ActivityLogDetails.tsx" --exec cat {} \;
Length of output: 11611
🏁 Script executed:
#!/bin/bash
# Find ScrollFollow component
fd --type f -e "ts" -e "tsx" -e "js" -e "jsx" | xargs rg -l "ScrollFollow" -g '!node_modules' | head -20Length of output: 66
🏁 Script executed:
#!/bin/bash
# Search for ScrollFollow component definition
rg -nP --type=ts --type=tsx "(?:export\s+)?(const|function|class)\s+ScrollFollow\b" -A 15Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check for throttle/debounce imports or usage in the ActivityLogDetails file
rg -nP "(throttle|debounce)" apps/provider-console/src/components/shared/ActivityLogDetails.tsxLength of output: 0
---
🌐 Web query:
```web
@melloware/react-logviewer ScrollFollow startFollowing prop behavior
💡 Result:
Short answer: startFollowing is the initial boolean that determines whether ScrollFollow begins in "follow" mode (i.e., child LazyLog receives follow=true). It only sets the initial state — user scrolling will turn follow off, and to re-enable following you must call the startFollowing helper returned by the render/child function (the prop itself is not a runtime restart callback). [1][2]
Sources:
- @melloware/react-logviewer ScrollFollow docs (usage, startFollowing prop example). [1]
- react-lazylog ScrollFollow docs (explains startFollowing is initial value and exposes startFollowing/stopFollowing functions from the child render). [2]
Address both performance and architectural issues with ScrollFollow integration.
The performance concern is valid: the onScroll handler updates state on every scroll event without checking if the value changes, causing unnecessary re-renders.
More critically, the current approach is architecturally problematic. According to the library documentation, startFollowing is an initial prop only—it doesn't control follow behavior on subsequent renders. The code recreates the entire ScrollFollow component on each render attempt to work around this, which is inefficient. The proper approach is to use the startFollowing() and stopFollowing() functions from the render prop to control follow state dynamically.
Required fixes:
- Performance: Only update state when the value actually changes:
onScroll={({ scrollTop, scrollHeight, clientHeight }) => {
const isAtBottom = scrollTop + clientHeight >= scrollHeight - 10;
setFollowScroll(prev => {
if (prev[taskId] !== isAtBottom) {
return { ...prev, [taskId]: isAtBottom };
}
return prev;
});
onScroll({ scrollTop, scrollHeight, clientHeight });
}}- Architecture: Refactor to use the
startFollowingandstopFollowingcallback functions instead of relying on prop changes:
render={({ follow, onScroll, startFollowing, stopFollowing }) => (
<LazyLog
text={sanitizedLogs}
follow={follow}
onScroll={({ scrollTop, scrollHeight, clientHeight }) => {
const isAtBottom = scrollTop + clientHeight >= scrollHeight - 10;
if (isAtBottom) {
startFollowing();
} else {
stopFollowing();
}
onScroll({ scrollTop, scrollHeight, clientHeight });
}}
// ... rest of props
/>
)}🤖 Prompt for AI Agents
In apps/provider-console/src/components/shared/ActivityLogDetails.tsx around
lines 198-210, the current onScroll handler updates follow state on every scroll
and relies on re-rendering via the startFollowing prop; change it to (1) only
call setFollowScroll when the computed isAtBottom value actually differs from
the existing state for taskId to avoid needless re-renders, and (2) refactor the
ScrollFollow render prop to use the provided startFollowing and stopFollowing
functions to control follow behavior dynamically (call startFollowing when at
bottom, stopFollowing when not) while still invoking the original onScroll({
scrollTop, scrollHeight, clientHeight }) after handling follow state.
Summary by CodeRabbit