Merge 955aa06eb3d8575072eee6d8f5f80f298e091bb4 into 70685a948979469a8086b2e8d1784a8f27c40f33

web_src/js/components/RepoActionView.test.ts

@@ -0,0 +1,349 @@
+/**
+ * Unit test for RepoActionView's auto-scroll *logic* (the shouldAutoScroll method).
+ * This test focuses on the predicate that determines if a scroll *should* occur,
+ * rather than verifying the full end-to-end auto-scroll behavior (DOM interaction or actual scrolling).
+ *
+ * This test should FAIL with the original buggy code and PASS with our fix,
+ * specifically for the 'slightly below viewport' scenario.
+ */
+
+import {createApp} from 'vue';
+import RepoActionView from './RepoActionView.vue';
+
+// Mock dependencies to isolate the shouldAutoScroll logic
+vi.mock('../svg.ts', () => ({
+  SvgIcon: {template: '<span></span>'},
+}));
+
+vi.mock('./ActionRunStatus.vue', () => ({
+  default: {template: '<span></span>'},
+}));
+
+vi.mock('../utils/dom.ts', () => ({
+  createElementFromAttrs: vi.fn(),
+  toggleElem: vi.fn(),
+}));
+
+vi.mock('../utils/time.ts', () => ({
+  formatDatetime: vi.fn(() => '2023-01-01'),
+}));
+
+vi.mock('../render/ansi.ts', () => ({
+  renderAnsi: vi.fn((text) => text),
+}));
+
+vi.mock('../modules/fetch.ts', () => ({
+  POST: vi.fn(() => Promise.resolve()),
+  DELETE: vi.fn(() => Promise.resolve()),
+}));
+
+vi.mock('../utils.ts', () => ({
+  toggleFullScreen: vi.fn(),
+}));
+
+describe('RepoActionView auto-scroll logic (shouldAutoScroll method)', () => {
+  beforeEach(() => {
+    // Mock window properties for controlled environment
+    Object.defineProperty(window, 'innerHeight', {
+      writable: true,
+      configurable: true,
+      value: 600,
+    });
+
+    Object.defineProperty(window, 'localStorage', {
+      value: {
+        getItem: vi.fn(() => null),
+        setItem: vi.fn(),
+      },
+      writable: true,
+    });
+
+    // Mock clearInterval and setInterval to prevent actual timer execution
+    globalThis.clearInterval = vi.fn();
+    globalThis.setInterval = vi.fn(() => 1 as any);
+  });
+
+  test('should auto-scroll when log element is slightly below viewport (following logs)', () => {
+    // This test verifies the core behavioral change in the `shouldAutoScroll` method:
+    // Original code: STRICT check (element must be entirely in viewport)
+    // Fixed code: LENIENT check (element can be slightly below if user is following logs)
+
+    // Mock the last child element's getBoundingClientRect to simulate its position.
+    // NOTE: This test *mocks* the DOM interaction (getLastLogLineElement and getBoundingClientRect)
+    // and does not verify the component's ability to correctly find the element or
+    // that the real DOM element would produce these exact coordinates.
+    const mockLastChildElement = {
+      getBoundingClientRect: () => ({
+        top: 590,      // Starts at bottom of 600px viewport
+        bottom: 610,   // Extends 10px below viewport
+        left: 0,
+        right: 800,
+        width: 800,
+        height: 20,
+      }),
+    };
+
+    // Create container and mount component for context and state setup
+    const container = document.createElement('div');
+    document.body.append(container);
+
+    const app = createApp(RepoActionView, {
+      runIndex: '1',
+      jobIndex: '0',
+      actionsURL: '/test',
+      locale: {
+        status: {
+          unknown: 'Unknown', waiting: 'Waiting', running: 'Running',
+          success: 'Success', failure: 'Failure', cancelled: 'Cancelled',
+          skipped: 'Skipped', blocked: 'Blocked',
+        },
+        approvals_text: 'Approvals', commit: 'Commit', pushedBy: 'Pushed by',
+      },
+    });
+
+    const vm = app.mount(container) as any;
+
+    // Set up component state to enable auto-scroll conditions
+    vm.optionAlwaysAutoScroll = true;
+    vm.$data.currentJobStepsStates = [{expanded: true, cursor: null}];
+
+    // Mock a running step (required for auto-scroll)
+    vm.$data.currentJob = {
+      steps: [{status: 'running'}],
+    };
+
+    // Mock internal methods that interact with the DOM to control inputs to shouldAutoScroll.
+    // This allows us to precisely test the `shouldAutoScroll` method's logic.
+    const mockContainer = {
+      getBoundingClientRect: () => ({
+        top: 100,      // Container is visible in viewport
+        bottom: 500,   // Container extends into viewport
+        left: 0,
+        right: 800,
+        width: 800,
+        height: 400,   // Large container, clearly visible
+      }),
+    };
+    vm.getJobStepLogsContainer = vi.fn(() => mockContainer); // Return mock container
+    vm.getLastLogLineElement = vi.fn(() => mockLastChildElement); // Return the test element
+
+    // Test the actual component's shouldAutoScroll method
+    const shouldScroll = vm.shouldAutoScroll(0);
+
+    // CRITICAL BEHAVIORAL TEST (for the predicate logic):
+    // When element is slightly below viewport (simulating user following logs), should auto-scroll?
+    // Original buggy code: FALSE (too strict - requires element entirely in viewport)
+    // Fixed code: TRUE (lenient - allows slight overflow for better UX)
+    expect(shouldScroll).toBe(true);
+
+    // Cleanup
+    app.unmount();
+    container.remove();
+  });
+
+  test('should NOT auto-scroll when element is far below viewport (user scrolled up)', () => {
+    // Both original and fixed code should agree on this case.
+    // This scenario simulates a user having scrolled up significantly.
+
+    // Mock the last child element's getBoundingClientRect to simulate its position.
+    // As with other tests, this directly feeds values to `shouldAutoScroll` without
+    // verifying actual DOM rendering or element finding.
+    const mockLastChildElement = {
+      getBoundingClientRect: () => ({
+        top: 800,      // Way below 600px viewport
+        bottom: 820,
+        left: 0,
+        right: 800,
+        width: 800,
+        height: 20,
+      }),
+    };
+
+    const container = document.createElement('div');
+    document.body.append(container);
+
+    const app = createApp(RepoActionView, {
+      runIndex: '1',
+      jobIndex: '0',
+      actionsURL: '/test',
+      locale: {
+        status: {
+          unknown: 'Unknown', waiting: 'Waiting', running: 'Running',
+          success: 'Success', failure: 'Failure', cancelled: 'Cancelled',
+          skipped: 'Skipped', blocked: 'Blocked',
+        },
+        approvals_text: 'Approvals', commit: 'Commit', pushedBy: 'Pushed by',
+      },
+    });
+
+    const vm = app.mount(container) as any;
+    vm.optionAlwaysAutoScroll = true;
+    vm.$data.currentJobStepsStates = [{expanded: true, cursor: null}];
+
+    // Mock a running step (so the failure is due to scroll position, not step status)
+    vm.$data.currentJob = {
+      steps: [{status: 'running'}],
+    };
+
+    // Mock a container that's far above viewport (user scrolled past it)
+    const mockContainer = {
+      getBoundingClientRect: () => ({
+        top: -300,     // Container is above viewport
+        bottom: -100,  // Container ends above viewport
+        left: 0,
+        right: 800,
+        width: 800,
+        height: 200,
+      }),
+    };
+    vm.getJobStepLogsContainer = vi.fn(() => mockContainer);
+    vm.getLastLogLineElement = vi.fn(() => mockLastChildElement);
+
+    const shouldScroll = vm.shouldAutoScroll(0);
+
+    // The `shouldAutoScroll` logic should return false here.
+    expect(shouldScroll).toBe(false);
+
+    app.unmount();
+    container.remove();
+  });
+
+  test('should NOT auto-scroll when step is not expanded', () => {
+    // This test verifies that auto-scroll is prevented when the job step is not expanded,
+    // regardless of the log element's position.
+    const container = document.createElement('div');
+    document.body.append(container);
+
+    const app = createApp(RepoActionView, {
+      runIndex: '1',
+      jobIndex: '0',
+      actionsURL: '/test',
+      locale: {
+        status: {
+          unknown: 'Unknown', waiting: 'Waiting', running: 'Running',
+          success: 'Success', failure: 'Failure', cancelled: 'Cancelled',
+          skipped: 'Skipped', blocked: 'Blocked',
+        },
+        approvals_text: 'Approvals', commit: 'Commit', pushedBy: 'Pushed by',
+      },
+    });
+
+    const vm = app.mount(container) as any;
+    vm.optionAlwaysAutoScroll = true;
+    vm.$data.currentJobStepsStates = [{expanded: false, cursor: null}]; // Not expanded
+
+    const shouldScroll = vm.shouldAutoScroll(0);
+
+    // The `shouldAutoScroll` logic should return false.
+    expect(shouldScroll).toBe(false);
+
+    app.unmount();
+    container.remove();
+  });
+
+  test('should NOT auto-scroll when step is finished (not running)', () => {
+    // Auto-scroll should only happen for currently executing steps, not finished ones
+
+    // Mock log element that would normally trigger auto-scroll
+    const mockLastLogElement = {
+      getBoundingClientRect: () => ({
+        top: 590,      // Near bottom of viewport (would normally auto-scroll)
+        bottom: 610,
+        left: 0,
+        right: 800,
+        width: 800,
+        height: 20,
+      }),
+    };
+
+    const container = document.createElement('div');
+    document.body.append(container);
+
+    const app = createApp(RepoActionView, {
+      runIndex: '1',
+      jobIndex: '0',
+      actionsURL: '/test',
+      locale: {
+        status: {
+          unknown: 'Unknown', waiting: 'Waiting', running: 'Running',
+          success: 'Success', failure: 'Failure', cancelled: 'Cancelled',
+          skipped: 'Skipped', blocked: 'Blocked',
+        },
+        approvals_text: 'Approvals', commit: 'Commit', pushedBy: 'Pushed by',
+      },
+    });
+
+    const vm = app.mount(container) as any;
+    vm.optionAlwaysAutoScroll = true;
+    vm.$data.currentJobStepsStates = [{expanded: true, cursor: null}];
+
+    // Mock a finished step (success status)
+    vm.$data.currentJob = {
+      steps: [{status: 'success'}],
+    };
+
+    vm.getJobStepLogsContainer = vi.fn(() => ({}));
+    vm.getLastLogLineElement = vi.fn(() => mockLastLogElement);
+
+    const shouldScroll = vm.shouldAutoScroll(0);
+
+    // Should NOT auto-scroll for finished steps, even if logs are following-friendly
+    expect(shouldScroll).toBe(false);
+
+    app.unmount();
+    container.remove();
+  });
+
+  test('should auto-scroll when step is running and user following logs', () => {
+    // This ensures we still auto-scroll when the step is actively running and user is following
+
+    // Mock log element that suggests user is following
+    const mockLastLogElement = {
+      getBoundingClientRect: () => ({
+        top: 590,
+        bottom: 610,   // Slightly below viewport (normal following behavior)
+        left: 0,
+        right: 800,
+        width: 800,
+        height: 20,
+      }),
+    };
+
+    const container = document.createElement('div');
+    document.body.append(container);
+
+    const app = createApp(RepoActionView, {
+      runIndex: '1',
+      jobIndex: '0',
+      actionsURL: '/test',
+      locale: {
+        status: {
+          unknown: 'Unknown', waiting: 'Waiting', running: 'Running',
+          success: 'Success', failure: 'Failure', cancelled: 'Cancelled',
+          skipped: 'Skipped', blocked: 'Blocked',
+        },
+        approvals_text: 'Approvals', commit: 'Commit', pushedBy: 'Pushed by',
+      },
+    });
+
+    const vm = app.mount(container) as any;
+    vm.optionAlwaysAutoScroll = true;
+    vm.$data.currentJobStepsStates = [{expanded: true, cursor: null}];
+
+    // Mock a running step
+    vm.$data.currentJob = {
+      steps: [{status: 'running'}],
+    };
+
+    vm.getJobStepLogsContainer = vi.fn(() => ({}));
+    vm.getLastLogLineElement = vi.fn(() => mockLastLogElement);
+
+    const shouldScroll = vm.shouldAutoScroll(0);
+
+    // SHOULD auto-scroll when step is running and user is following logs
+    expect(shouldScroll).toBe(true);
+
+    app.unmount();
+    container.remove();
+  });
+});

web_src/js/components/RepoActionView.vue

@@ -54,9 +54,16 @@ function parseLineCommand(line: LogLine): LogLineCommand | null {
   return null;
 }
 
-function isLogElementInViewport(el: Element): boolean {
+function isUserFollowingLogs(el: Element): boolean {
   const rect = el.getBoundingClientRect();
-  return rect.top >= 0 && rect.bottom <= window.innerHeight; // only check height but not width
+  const windowHeight = window.innerHeight;
+
+  // Check if the user is "following" the logs by seeing if the element is near the bottom
+  // We're more lenient than the original strict check, but still reasonable:
+  // - Element's top should be at or before the viewport bottom (user hasn't scrolled far up)
+  // - Element's bottom should be within reasonable range of viewport (not too far below)
+  const threshold = windowHeight * 0.1; // Allow element to extend 10% of viewport height below
+  return rect.top <= windowHeight && rect.bottom <= windowHeight + threshold;
 }
 
 type LocaleStorageOptions = {
@@ -114,6 +121,7 @@ export default defineComponent({
       },
       optionAlwaysAutoScroll: autoScroll ?? false,
       optionAlwaysExpandRunning: expandRunning ?? false,
+      lastAutoScrollTime: 0,
 
       // provided by backend
       run: {
@@ -218,6 +226,38 @@ export default defineComponent({
       // @ts-expect-error - _stepLogsActiveContainer is a custom property
       return el._stepLogsActiveContainer ?? el;
     },
+
+    // get the actual last log line element in a step, accounting for nested groups
+    getLastLogLineElement(stepIndex: number): Element | null {
+      const container = this.getJobStepLogsContainer(stepIndex);
+      if (!container) return null;
+
+      // Find the actual last log line, which might be nested in groups
+      // We need to check if groups are expanded (open) to find the truly visible last line
+      const findLastLogLine = (element: Element): Element | null => {
+        let lastChild = element.lastElementChild;
+        while (lastChild) {
+          if (lastChild.classList.contains('job-log-line')) {
+            return lastChild;
+          }
+          if (lastChild.classList.contains('job-log-group')) {
+            // Only look inside groups that are open (expanded)
+            const detailsElement = lastChild as HTMLDetailsElement;
+            if (detailsElement.open) {
+              const nestedLast = findLastLogLine(lastChild);
+              if (nestedLast) return nestedLast;
+            }
+            // If group is closed, the summary line is the visible "last" element
+            const summary = lastChild.querySelector('.job-log-group-summary .job-log-line');
+            if (summary) return summary;
+          }
+          lastChild = lastChild.previousElementSibling;
+        }
+        return null;
+      };
+
+      return findLastLogLine(container);
+    },
     // begin a log group
     beginLogGroup(stepIndex: number, startTime: number, line: LogLine, cmd: LogLineCommand) {
       const el = (this.$refs.logs as any)[stepIndex];
@@ -290,10 +330,21 @@ export default defineComponent({
 
     shouldAutoScroll(stepIndex: number): boolean {
       if (!this.optionAlwaysAutoScroll) 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;
-      return isLogElementInViewport(el.lastChild as Element);
+      if (!this.currentJobStepsStates[stepIndex]?.expanded) return false;
+
+      // Only auto-scroll for currently executing (active) groups, not finished ones
+      const step = this.currentJob?.steps?.[stepIndex];
+      if (!step || step.status !== 'running') return false;
+
+      // Get the step logs container to check scroll position
+      const container = this.getJobStepLogsContainer(stepIndex);
+      if (!container) return true; // If no container yet, auto-scroll when it appears
+
+      const lastLogLine = this.getLastLogLineElement(stepIndex);
+      if (!lastLogLine) return true; // If no logs yet, auto-scroll when logs appear
+
+      // Check if user is following the logs within this step
+      return isUserFollowingLogs(lastLogLine);
     },
 
     appendLogs(stepIndex: number, startTime: number, logLines: LogLine[]) {
@@ -360,11 +411,12 @@ export default defineComponent({
           }
         }
 
-        // find the step indexes that need to auto-scroll
-        const autoScrollStepIndexes = new Map<number, boolean>();
+        // Check which steps should auto-scroll BEFORE appending new logs
+        const shouldAutoScrollSteps = new Set<number>();
         for (const logs of job.logs.stepsLog ?? []) {
-          if (autoScrollStepIndexes.has(logs.step)) continue;
-          autoScrollStepIndexes.set(logs.step, this.shouldAutoScroll(logs.step));
+          if (this.shouldAutoScroll(logs.step)) {
+            shouldAutoScrollSteps.add(logs.step);
+          }
         }
 
         // append logs to the UI
@@ -374,13 +426,26 @@ export default defineComponent({
           this.appendLogs(logs.step, logs.started, logs.lines);
         }
 
-        // auto-scroll to the last log line of the last step
-        let autoScrollJobStepElement: HTMLElement;
-        for (let stepIndex = 0; stepIndex < this.currentJob.steps.length; stepIndex++) {
-          if (!autoScrollStepIndexes.get(stepIndex)) continue;
-          autoScrollJobStepElement = this.getJobStepLogsContainer(stepIndex);
+        // Auto-scroll to the last log line for steps that should auto-scroll
+        // Do this AFTER appending logs so the DOM is up to date
+        // Use requestAnimationFrame to ensure DOM is fully updated
+        if (shouldAutoScrollSteps.size > 0) {
+          const now = Date.now();
+          // Throttle auto-scroll to prevent rapid corrections (max once per 100ms)
+          if (now - this.lastAutoScrollTime > 100) {
+            this.lastAutoScrollTime = now;
+            requestAnimationFrame(() => {
+              for (const stepIndex of shouldAutoScrollSteps) {
+                const lastLogLine = this.getLastLogLineElement(stepIndex);
+                if (lastLogLine) {
+                  // Use instant scrolling to avoid conflicts with user scrolling
+                  // and scroll to the bottom of the element to ensure it's fully visible
+                  lastLogLine.scrollIntoView({behavior: 'instant', block: 'end'});
+                }
+              }
+            });
+          }
         }
-        autoScrollJobStepElement?.lastElementChild.scrollIntoView({behavior: 'smooth', block: 'nearest'});
 
         // clear the interval timer if the job is done
         if (this.run.done && this.intervalID) {