Refs #39253 - perf 2/4 - Replace interval polling with setTimeout chains

[adamruzicka]

Follows #1047

TODOs:

• pull in missing tests from the "big PR"

[adamlazik1]

Mostly LGTM.

[adamruzicka]

@coderabbitai fullreview

[coderabbitai]

@adamruzicka I will perform a complete review of pull request #1048.

✅ Action performed

Full review finished.

[coderabbitai]

Walkthrough

Job invocation and template invocation polling now use recursive five-second timeouts with cleanup on terminal responses, errors, unmounts, and expansion changes. Task cancellability is returned by the API and consumed by selectors. Obsolete task-fetching actions were removed. Recurring actions no longer receive a job ID. Tests cover polling, cleanup, expansion, cancellation, and recurring-action updates.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: lukshio

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: replacing interval polling with setTimeout chains.
Description check	✅ Passed	The description relates to the changeset by referencing the preceding pull request and the inclusion of missing tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

webpack/JobInvocationDetail/JobInvocationActions.js (1)

14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Export isJobFinished to avoid duplicating the terminal-status list.

index.js recomputes the same STATUS.FAILED/STATUS.SUCCEEDED/STATUS.CANCELLED check inline for its finished variable. If the set of terminal statuses ever changes here, the UI's "finished" flag can silently drift out of sync with when polling actually stops.

♻️ Proposed fix

-const isJobFinished = statusLabel =>
+export const isJobFinished = statusLabel =>
   statusLabel === STATUS.FAILED ||
   statusLabel === STATUS.SUCCEEDED ||
   statusLabel === STATUS.CANCELLED;

Then in index.js:

-  const finished =
-    statusLabel === STATUS.FAILED ||
-    statusLabel === STATUS.SUCCEEDED ||
-    statusLabel === STATUS.CANCELLED;
+  const finished = isJobFinished(statusLabel);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@webpack/JobInvocationDetail/JobInvocationActions.js` around lines 14 - 17,
Export the isJobFinished helper from JobInvocationActions.js and update index.js
to use it for the finished value instead of duplicating the STATUS.FAILED,
STATUS.SUCCEEDED, and STATUS.CANCELLED checks. Keep the terminal-status logic
centralized in isJobFinished.

webpack/JobInvocationDetail/TemplateInvocation.js (1)

15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import the shared refresh interval instead of redefining it.

AUTO_REFRESH_INTERVAL_MS is already exported from webpack/JobInvocationDetail/JobInvocationConstants.js with the same value (5000). Redefining it locally here creates two independent sources of truth for the same timing value. If the interval changes in one place, job and template polling will drift out of sync silently.

♻️ Proposed fix

-const AUTO_REFRESH_INTERVAL_MS = 5000;
+import { AUTO_REFRESH_INTERVAL_MS } from './JobInvocationConstants';

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@webpack/JobInvocationDetail/TemplateInvocation.js` around lines 15 - 16,
Remove the local AUTO_REFRESH_INTERVAL_MS definition in TemplateInvocation.js
and import the shared AUTO_REFRESH_INTERVAL_MS export from
JobInvocationConstants.js, ensuring the existing refresh logic uses that shared
constant.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@webpack/JobInvocationDetail/JobInvocationActions.js`:
- Around line 19-23: Update the fallback returned by extractErrorMessage to pass
'Unknown error.' through the existing __() translation helper, while preserving
the current response error-message precedence.
- Around line 25-55: Update getJobInvocation and stopJobInvocationPolling to
invalidate prior poll cycles, not just clear their timers. Create a per-cycle
request/token guard tied to pollTimeoutRef so handleSuccess and handleError from
an older URL or cleanup cannot reschedule polling or mutate shared state; ensure
the active cycle still polls and clears normally. Add coverage for resolving an
earlier request after a new getJobInvocation call starts.

In `@webpack/JobInvocationDetail/TemplateInvocation.js`:
- Around line 88-125: Guard schedulePoll against cancelled effect runs before
dispatching, and make handleError ignore responses from cancelled runs so they
cannot clear a newer poll’s timeout. Update the schedulePoll and handleError
callbacks while preserving the existing cancellation cleanup and active-poll
behavior.

---

Nitpick comments:
In `@webpack/JobInvocationDetail/JobInvocationActions.js`:
- Around line 14-17: Export the isJobFinished helper from
JobInvocationActions.js and update index.js to use it for the finished value
instead of duplicating the STATUS.FAILED, STATUS.SUCCEEDED, and STATUS.CANCELLED
checks. Keep the terminal-status logic centralized in isJobFinished.

In `@webpack/JobInvocationDetail/TemplateInvocation.js`:
- Around line 15-16: Remove the local AUTO_REFRESH_INTERVAL_MS definition in
TemplateInvocation.js and import the shared AUTO_REFRESH_INTERVAL_MS export from
JobInvocationConstants.js, ensuring the existing refresh logic uses that shared
constant.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 79743ae0-a143-4270-a79c-483bd376e92a

📥 Commits

Reviewing files that changed from the base of the PR and between b278a1d and d98e8be.

📒 Files selected for processing (11)

• app/views/api/v2/job_invocations/main.json.rabl
• webpack/JobInvocationDetail/JobInvocationActions.js
• webpack/JobInvocationDetail/JobInvocationConstants.js
• webpack/JobInvocationDetail/JobInvocationSelectors.js
• webpack/JobInvocationDetail/JobInvocationToolbarButtons.js
• webpack/JobInvocationDetail/TemplateInvocation.js
• webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js
• webpack/JobInvocationDetail/__tests__/MainInformation.test.js
• webpack/JobInvocationDetail/__tests__/TemplateInvocationPolling.test.js
• webpack/JobInvocationDetail/__tests__/fixtures.js
• webpack/JobInvocationDetail/index.js

💤 Files with no reviewable changes (1)

• webpack/JobInvocationDetail/JobInvocationConstants.js

[adamlazik1]

Suggested change

[adamlazik1]

Don't know what's up with this preview. Anyway, STATUS should be removed.

[adamruzicka]

removed

[adamlazik1]

Claude review, seems like a valid suggestion to me but let me know your thoughts:

The cancelPreviousCycle() canceller was only invoked when a new getJobInvocation call started, so a request still in flight when the component unmounted (no new call ever follows) could resolve afterward and revive a zombie polling loop via setTimeout. The fix moves that cancellation into stopJobInvocationPolling itself, since both retirement paths (new cycle and unmount) already call it, so the active cycle is always invalidated regardless of which path retires it.

diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js
index 978a6acc..dcedea06 100644
--- a/webpack/JobInvocationDetail/JobInvocationActions.js
+++ b/webpack/JobInvocationDetail/JobInvocationActions.js
@@ -52,10 +52,6 @@ export const getJobInvocation = (url, pollTimeoutRef) => dispatch => {
 
   stopJobInvocationPolling(pollTimeoutRef);
 
-  // Cancel previous poll cycle's in-flight requests
-  if (pollTimeoutRef.cancelPreviousCycle) {
-    pollTimeoutRef.cancelPreviousCycle();
-  }
   pollTimeoutRef.cancelPreviousCycle = () => {
     cancelled = true;
   };
@@ -66,6 +62,13 @@ export const getJobInvocation = (url, pollTimeoutRef) => dispatch => {
 export const stopJobInvocationPolling = pollTimeoutRef => {
   clearTimeout(pollTimeoutRef.current);
   pollTimeoutRef.current = null;
+  // Invalidate any request still in flight from a previous poll cycle
+  // (e.g. the job id changed, or the component unmounted) so its
+  // response can no longer reschedule polling or mutate shared state.
+  if (pollTimeoutRef.cancelPreviousCycle) {
+    pollTimeoutRef.cancelPreviousCycle();
+    pollTimeoutRef.cancelPreviousCycle = null;
+  }
 };
 
 export const cancelJob = (jobId, force) => dispatch => {
diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js
index e00520fe..b423de03 100644
--- a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js
+++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js
@@ -197,4 +197,27 @@ describe('job invocation polling', () => {
     // Timeout should not be set by the stale callback
     expect(ref.current).toBeNull();
   });
+
+  it('ignores a stale response resolving after stopJobInvocationPolling (e.g. unmount)', () => {
+    let capturedHandleSuccess;
+    apiGetSpy = jest
+      .spyOn(APIActions, 'get')
+      .mockImplementation(({ handleSuccess }) => dispatch => {
+        capturedHandleSuccess = handleSuccess;
+        return dispatch({ type: 'MOCK_GET' });
+      });
+
+    const store = makeStore();
+    store.dispatch(getJobInvocation(url, ref));
+    expect(apiGetSpy).toHaveBeenCalledTimes(1);
+
+    // Simulate unmount: stop polling while the request is still in flight.
+    stopJobInvocationPolling(ref);
+
+    // The in-flight request resolves after the "unmount".
+    capturedHandleSuccess({ data: runningData });
+
+    // The stale response must not revive polling.
+    expect(ref.current).toBeNull();
+  });
 });

[adamlazik1]

The comments can be plucked out ^

[adamruzicka]

Claude review, seems like a valid suggestion to me but let me know your thoughts:

Seems valid, added.

[adamlazik1]

Actually, sorry to bother but let's drop it. It breaks the UI and fixing it correctly seems non-trivial. I'd rather not inflate this PR more than it is, we can mark it down for future.