Skip to content

Fix "Always expand running logs" behavior on subsequent updates #34964

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 5, 2025

Summary

Fixes the "Always expand running logs" setting behavior so that it properly auto-expands running log steps on subsequent job updates, not just on the initial page load.

Problem

Currently, the "Always expand running logs" setting only works when the Actions page is first loaded (isFirstLoad = true). If a a running step is collapsed, or if a step transitions from "waiting" to "running" status during subsequent job updates, the logs do not auto-expand even though the setting is enabled.

Solution

Modified RepoActionView.vue to:

  1. Check for running steps and auto-expand them on all job updates when the setting is enabled
  2. Handle step status transitions (e.g., waiting → running) by expanding logs when steps become running
  3. Preserve existing behavior for initial page loads

Test Coverage

Added basic tests (RepoActionView.autoExpand.test.ts) to reproduce buggy behavior. This was challenging because of how the Vue component is currently written - introduced a (hopefully) minor refactoring to simplify mocking the loadJobData function by optionally allowing injection of it. Also removed a check of the abortController that I think is unnecessary - if (this.loadingAbortController !== abortController) return -- this was really making the testing difficult and I'm not 100% sure what was going on behind the scenes to make it so, but according to the MDN docs, this line shouldn't be necessary - the line to read from the response, await resp.json() will fail with an AbortError if the request was aborted at any point before the response is read. There's already try/catch handling of the AbortError further down, so this should still be handled. Apologies if I'm parsing this wrong, it just seems impossible to get a component test past this line otherwise ☹️.

Changes Made

  • web_src/js/components/RepoActionView.vue: Enhanced the loadJob method to handle auto-expansion on all updates
  • web_src/js/components/RepoActionView.autoExpand.test.ts: Added focused tests for the auto-expand functionality
  • package.json / package-lock.json: Added @vue/test-utils dependency for testing

p.s. interestingly when I was testing this "live", I found that with the auto-expand behavior working, the current auto-scroll logs (see #34957) worked better ... until it got to a long step that was updating a lot ... then it broke again, so I think #34957 is still needed.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 5, 2025
@wxiaoguang
Copy link
Contributor

Sorry I am not able to review the code written by AI, it looks more complex than it should be and really difficult to understand.

To be simple, why not just remove the isFirstLoad check? What's the difference from your change.

I would suggest you to use AI as reference and rewrite the code to make sure every line is clear and maintainable.

this.currentJobStepsStates[i] = {cursor: null, expanded: shouldExpand};
} else if (this.optionAlwaysExpandRunning && isRunning && !this.currentJobStepsStates[i].expanded) {
// auto-expand running steps on subsequent loads (handles step transitions)
this.currentJobStepsStates[i].expanded = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can extract the loop's body to a separate function, then we only need to test that function's behavior.

For example:

for (let i = 0; i < this.currentJob.steps.length; i++) {
    syncCurrentJobStepsStates(i);
}

Then we only need to mock some related variables to test syncCurrentJobStepsStates

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/dependencies modifies/frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants