Fix inconsistent auto-scroll behavior in Actions log viewer

[bdruth]

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

[wxiaoguang]

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]

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]

* **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]

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]

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]

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.

[wxiaoguang]

Your tests pass this simple change (your test line 338 should be vm.getJobStepLogsContainer = vi.fn(() => mockLastLogElement); because the container is empty):

I don't understand why AI makes the problem that complex (this change is just a draft demo, still not quite right)

diff --git a/web_src/js/components/RepoActionView.vue b/web_src/js/components/RepoActionView.vue
index 2eb2211269..f717e88711 100644
--- a/web_src/js/components/RepoActionView.vue
+++ b/web_src/js/components/RepoActionView.vue
@@ -56,7 +56,7 @@ function parseLineCommand(line: LogLine): LogLineCommand | null {
 
 function isLogElementInViewport(el: Element): boolean {
   const rect = el.getBoundingClientRect();
-  return rect.top >= 0 && rect.bottom <= window.innerHeight; // only check height but not width
+  return rect.bottom >= 0 && rect.bottom <= window.innerHeight + rect.height / 2; // only check height but not width
 }
 
 type LocaleStorageOptions = {
@@ -290,9 +290,13 @@ export default defineComponent({
 
     shouldAutoScroll(stepIndex: number): boolean {
       if (!this.optionAlwaysAutoScroll) return false;
+      if (!this.currentJobStepsStates[stepIndex]?.expanded) return false;
+      const step = this.currentJob?.steps?.[stepIndex];
+      if (!step || step.status !== 'running') return false;
+
       const el = this.getJobStepLogsContainer(stepIndex);
       // if the logs container is empty, then auto-scroll if the step is expanded
-      if (!el.lastChild) return this.currentJobStepsStates[stepIndex].expanded;
+      if (!el.lastChild) return this.currentJobStepsStates[stepIndex].expanded && isLogElementInViewport(el);
       return isLogElementInViewport(el.lastChild as Element);
     },

[bdruth]

I'll give this a shot and test it - some of the changes in the PR weren't strictly to pass the test but were additional because of odd behavior that I saw (like the mentioned snapping back up to earlier expanded steps) when I was manually testing it.

[wxiaoguang]

I can see many problems in the test code, it doesn't test the real cases. AI Hallucination.

[bdruth]

Can you point out the hallucinations? I went through the tests manually and verified what was being tested. Maybe there's hallucinations in what's being mocked? I just trusted the mock setups, for the most part, as there's very little I can do to determine how accurate they are without a vastly deeper understanding of the frontend. Maybe it's just easier to remove the test?

[wxiaoguang]

At least, the elements are wrong. See this comment: #34957 (comment) , for example, around line 338, why an empty container can have a "last element" with "bounding client rect" ?

I think I won't spend more time on AI code. Please understand the code first, feel free to ask questions directly (but don't post AI contents)

[bdruth]

I'm not sure I understand what you're referring to on line 338 of the test - it's necessary to avoid an undefined TypeError - it doesn't need to contain anything for the purpose of the test, it just needs to not be undefined.

 FAIL  web_src/js/components/RepoActionView.test.ts > RepoActionView auto-scroll logic (shouldAutoScroll method) > should auto-scroll when step is running and user following logs
TypeError: Cannot read properties of undefined (reading '0')
 ❯ Proxy.getJobStepLogsContainer web_src/js/components/RepoActionView.vue:220:26
    218|     // get the job step logs container ('.job-step-logs')
    219|     getJobStepLogsContainer(stepIndex: number): HTMLElement {
    220|       return (this.$refs.logs as any)[stepIndex];
       |                          ^
    221|     },
    222| 
 ❯ Proxy.shouldAutoScroll web_src/js/components/RepoActionView.vue:340:30
 ❯ web_src/js/components/RepoActionView.test.ts:341:29

I'll close this and take another run at it without any tests. Thanks for your time.

[wxiaoguang]

why an empty container can have a "last element" with "bounding client rect" ?

The problem is: the test code uses an empty container to test the behavior which requires a "last line element". The test itself is not right, then it introduces more hacks to workaround such wrong test case.