Fix "Always expand running logs" behavior on subsequent updates

[bdruth]

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.

[wxiaoguang]

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.

[wxiaoguang]

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

[bdruth]

Thanks for the feedback!

So, we can, sort of, but look at how many internal references the body of the loop has ... currentJob, currentJobStepsStates, optionAlwaysExpandRunning, etc. It's not impossible to set that up and mock it, but it's also not as trivial as it seems? The question is also - what's the goal of the test? You're looking at this after-the-fact, I wrote the test before I fixed the bug, so I wanted a test that reproduced the bug behavior itself as a failing test. Doing what you suggest requires knowing in advance what the fix is, which also means that it's impossible to write a test before you know what the fix is. It's a different approach, that's all.

[wxiaoguang]

It's not impossible to set that up and mock it, but it's also not as trivial as it seems?

IMO it should be easier than the vue mock. And we have clear data definition for various cases.

what's the goal of the test?

Make the logic correct.

You're looking at this after-the-fact .... it's impossible to write a test before you know what the fix is. It's a different approach

I also wrote a lot of code to test something, and if the test code is not necessary, I won't leave them in the code base. We can only keep the necessary tests to test what we want. We can still make the tests fail with old approach, and pass with new approach.

[bdruth]

I see, so you want just logic tests, not behavior tests. That's fine - I didn't realize that was the project's convention. I can try to do that. When I test, I tend to test behavior, not logic - logic, once written, doesn't usually "break" - but behavior often does, so in my experience it's been a better safety net to test behaviors. Apologies for the misstep, I will match your conventions.

[wxiaoguang]

Tests are always good and welcome, and I agree that many components lack tests (well, crowd-contributed open-source project, I am not able to convince everyone to write tests properly, not everyone has that much time).

IMO:

• The best: we can have proper tests, including behavior tests.
• Good: cover most core logic.
• Acceptable: fully manually tested.

Meanwhile, the most important: the code should be overall right and maintainable, that's the first rule.

---

Disclaimer: that's just my personal opinion, I am not native speaker so sometimes my expression might not be accurate. Thank you for understanding.

[bdruth]

@wxiaoguang are you referring to the test code? I don't think the fix code itself is that difficult to understand.

The test code has to setup a bunch of stuff because of the Vue component, otherwise mounting the view just fails and makes any test impossible.

Edit: had AI generate a code review below for you.

[bdruth]

Code Review

This PR successfully addresses the "Always expand running logs" bug by fixing the auto-expansion behavior on subsequent job updates. Here's my analysis:

✅ Strengths

1. Clear Problem Resolution: The fix correctly addresses the core issue - auto-expansion now works on all job updates, not just initial page loads.

2. Minimal, Focused Changes: The solution adds only 6 lines of logic to the Vue component, making it easy to understand and maintain.

3. Comprehensive Test Coverage: The test file demonstrates both scenarios:

   • Auto-expansion on subsequent loads when steps are already running
   • Auto-expansion when steps transition from "waiting" to "running"

4. Good Testing Architecture: The dependency injection pattern for fetchJobDataFn is a clean approach that makes the component more testable without breaking existing functionality.

⚠️ Considerations

1. AbortController Check Removal: The removal of the abort controller check needs careful consideration. While the PR author's reasoning about MDN docs is sound, this change affects race condition handling. Consider:

   • Adding a comment explaining why this check was removed
   • Ensuring comprehensive testing of concurrent requests

2. Minor Code Style: The type annotation could be simplified for consistency with the existing codebase.

🔧 Technical Implementation

The core fix is elegant - it correctly handles both the "collapsed running step" and "step status transition" scenarios mentioned in the issue by checking if auto-expand is enabled, if the step is running, and if it's not already expanded.

📝 Overall Assessment

This is a well-implemented fix that:

• Solves the reported problem effectively
• Maintains backward compatibility
• Includes good test coverage
• Uses minimal, focused changes

The only significant concern is the AbortController check removal, but the reasoning provided seems sound. Consider adding documentation about this change.

Recommendation: ✅ Approve with the suggestion to document the AbortController change reasoning in a code comment.

[wxiaoguang]

Please post the understanding from you but not from an AI robot .........

AbortController Check Removal: ...

AbortController: it is necessary to avoid duplicate requests, and cancel the previous request before sending a new one.

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

What's the difference?

[bdruth]

To be clear - I'm trying as much as possible to fix a behavior I'm seeing without disrupting what's there ... I can try removing the isFirstLoad check - but my question would be - what's the purpose of that check in the current code?

Re: the abort controller - I spent a disproportionate amount of time around that code - the AI couldn't make heads or tails of it. I hadn't used AbortController in the past, so I looked at the docs on MDN to familiarize myself with what it's supposed to be doing. As near as I could tell, the way that it's being used in this code wasn't necessary, but similar question as before - what's the current purpose of the abortcontroller check that I removed -- it doesn't seem to match what you wrote -- " to avoid duplicate requests, and cancel the previous request before sending a new one"?

No tests were written, so it's nearly impossible for an outsider to understand the intent of the current code, especially when it seems like it isn't working correctly. This is why I try to leave things as they are as much as possible, since I can't know what might break if I make changes without understanding the intent.

I'll see what I can do to simplify this and maybe just remove any testing since that seems to be problematic since the frontend hasn't been developed with this in mind.

[wxiaoguang]

To be clear - I'm trying as much as possible to fix a behavior I'm seeing without disrupting what's there ... I can try removing the isFirstLoad check - but my question would be - what's the purpose of that check in the current code?

If you don't understand how the current code works, why you fully trust the AI's code? I believe the developers themselves should fully understand the code when making changes, right?

[wxiaoguang]

it doesn't seem to match what you wrote -- " to avoid duplicate requests, and cancel the previous request before sending a new one"?

Why it doesn't match? Could you show a real example?

[bdruth]

There's a few examples in the previously linked MDN docs, is that what you mean?

[wxiaoguang]

There's a few examples in the previously linked MDN docs, is that what you mean?

I also read MDN docs. I don't see any problem.

If you think there is a problem, please show the details, based on our code: what are the steps to prove that the problem exists? For example, you can show the real example: do step1, then step2, then step3, expect to see A, but we get B, then mismatch. Otherwise, I am not able to know what is the "mismatch" mentioned by your review.

[bdruth]

What's your impression of what this line of code is intended to do?

if (this.loadingAbortController !== abortController) return;

[bdruth]

I checked if just removing the isFirstLoad fixes the issue - it does not.

[wxiaoguang]

if (this.loadingAbortController !== abortController) return;

fetchJobData is "async", and it may contain other "await" after the fetch (not now, but possible in the future)

So we must make sure that after await fetchJobData, we are still in the same fetch request as the one started by loadJob.

There won't cause difference in current code base, but to make the whole design stable, it's good to keep. At least, it isn't wrong and doesn't cause any problem.

[bdruth]

It makes it impossible to test for some reason - even using loadJobForce from the test, this check never succeeds under the test conditions.

I'll close this for now and maybe try again without any tests. Sorry for the bother.