Skip to content

Fix inconsistent auto-scroll behavior in Actions log viewer #34957

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bdruth
Copy link

@bdruth bdruth commented Jul 4, 2025

Problem

Auto-scroll in the Actions log viewer becomes unreliable after manual scrolling, requiring repeated manual intervention to re-enable auto-scroll when following long-running actions.

Root Cause

The original auto-scroll logic had several issues:

  • Too strict viewport detection that couldn't handle slight scroll variations
  • Inability to properly detect the last log line in nested log groups
  • Poor timing of auto-scroll decisions relative to DOM updates

Solution

  • Improved viewport detection: Replaced strict boundary checks with more lenient threshold-based detection (10% overflow tolerance)
  • Enhanced log line detection: Added proper handling of nested log groups to find the actual last visible log line
  • Better timing: Check auto-scroll decisions before appending logs, then scroll after DOM updates
  • Smoother scrolling: Use requestAnimationFrame, throttling, and scrollIntoView with behavior: 'instant'
  • Status-aware scrolling: Only auto-scroll for running steps to avoid jumping back to completed steps (that are expanded) when the updating step isn't expanded.

Testing

  • Added some unit tests
  • All frontend tests pass
  • Manual testing - seems much improved for me

Files Changed

  • web_src/js/components/RepoActionView.vue - Enhanced auto-scroll logic and viewport detection
  • web_src/js/components/RepoActionView.test.ts - Added behavioral tests for auto-scroll scenarios

- Resolves auto-scroll becoming unreliable after manual scrolling
- Improves auto-scroll reliability when following logs of running actions
- Added comprehensive unit tests covering auto-scroll scenarios

Eliminates the need for repeated manual intervention to re-enable auto-scroll
when following long-running actions.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 4, 2025
@wxiaoguang
Copy link
Contributor

Written by AI? I can see duplicate code and the tests are not easy to read.

Auto-scroll in the Actions log viewer becomes unreliable after manual scrolling, requiring repeated manual intervention to re-enable auto-scroll when following long-running actions.

Can you show a screenshot (picture or video) to elaborate the problem?

@bdruth
Copy link
Author

bdruth commented Jul 5, 2025

Written by AI?

With a lot of help from AI, yep. The tests are not easy to read because the Vue code is very difficult to test (there weren't any for this view previously). We could just continue that trend and I can delete the tests ...

I'll see if I can record a quick video - the behavior isn't subtle, though - the auto scroll just basically doesn't work for more than a few log updates, at least from my experience. I've checked Safari and Chrome both, no difference.

@wxiaoguang
Copy link
Contributor

* **Improved viewport detection**: Replaced strict boundary checks with more lenient threshold-based detection (10% overflow tolerance)

I can understand this part.

* **Enhanced log line detection**: Added proper handling of nested log groups to find the actual last visible log line

I can understand this part, but I think we can simplify the code.

Ideally, we only need to check "if the last element's bottom (the last group's bottom) is in the viewport", no need to use recursive traversal to find the "last line".

* **Better timing**: Check auto-scroll decisions before appending logs, then scroll after DOM updates

understand

* **Smoother scrolling**: Use `requestAnimationFrame`, throttling, and `scrollIntoView` with `behavior: 'instant'`

Do we really need the animation and use "requestAnimationFrame"?

* **Status-aware scrolling**: Only auto-scroll for running steps to avoid jumping back to completed steps (that are expanded) when the updating step isn't expanded.

Don't quite understand

@wxiaoguang
Copy link
Contributor

Some more thoughts: can we only maintain a "autoscroll-able" flag and avoid more "if" checks?

The brief idea is:

  1. on init, the flag = "autoscroll option"
  2. on window's scroll event, check whether the running job's last line's bottom is in the view port
    • if yes, set flag=true
    • else, set flag=false
  3. when new logs come, scroll if flag===true

@bdruth
Copy link
Author

bdruth commented Jul 5, 2025

I'll see what I can do with your ideas, thanks for the feedback! The "status-aware scrolling" is more of a side-effect fix of the other parts - I was noticing while testing that if an earlier group was expanded and completed, with no subsequent groups expanded, the logic kept scrolling me back to the last line of the previous group when I was trying to get to a subsequent group to expand it. So, that was the simplest approach I could think of - don't scroll to the bottom of a group that's already completed.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 6, 2025

Reviewed the "Root Causes" (again):

  • Too strict viewport detection that couldn't handle slight scroll variations
  • Inability to properly detect the last log line in nested log groups

We can relax the check (as implemented in this PR, relax about 10%), and check the last item's bottom line.

Maybe we can have a separate function and have a simple test for it, then no need these vue mock tricks.

  • Poor timing of auto-scroll decisions relative to DOM updates

Although I proposed to handle "scroll" event, after I asked GitHub Copilot, it says that it's almost impossible to correctly distinguish the "scroll" events triggered by scrollIntoView or user's input.

So maybe we should use scrollIntoView({behavior: 'instant'}) to avoid the animation and then no need to use the 100ms magic timing or requestAnimationFrame trick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants