Performance by adamruzicka · Pull Request #1035 · theforeman/foreman_remote_execution

adamruzicka · 2026-05-26T12:36:49Z

Includes #1034

Opening as a draft until I actually read through it. Whether the individual fixes should be included is up to debate.

I'd expect these two to yield the biggest benefits:

The rest are sort of nice to haves

adamruzicka · 2026-05-26T14:09:13Z

As a non-admin user, looking at a job with 5000 hosts, this fix brings the time to first full draw from ~43 seconds to ~15 seconds

Lukshio

Hi, I've finished first round of the review, and found some things to discuss.

Lukshio · 2026-05-26T15:27:20Z

-  dispatch(fetchData);
+export const getJobInvocation = url => dispatch => {
+  stopJobInvocationPolling();
+  fetchJobInvocation(dispatch, url, { include_permissions: true });


I would be very careful about calling with inlcude_permissions only once. We can use it in each request, the difference between response with and without this information should not be more significant. After that we can simplify calls by removing this getJobInvocation. All informations should be already in the repeating calls.

One of the goals here was to reduce the number of times we re-load rarely changing information we already have.

We can use it in each request, the difference between response with and without this information should not be more significant.

The assumption was that the permissions shouldn't really change as the job runs, even though technically they can and recalculating those every second is a waste in majority of cases.

the difference between response with and without this information should not be more significant.

Yes, at the same time, it is yet another thing the backend has to compute.

With that being said, I don't feel strongly about this. We may find out that the savings coming from this are negligible and then we can very well favor the code simplicity over this.

On the other hand, I assume that when user privileges changes, or user logs out, server won't respond for this recurring api request, so there still will be permissions check on server side. So we can omit it on frontend.

server won't respond for this recurring api request, so there still will be permissions check on server side

Backend always checks permissions, no matter what.

So we can omit it on frontend.

I was under the impression we load these so that we know which action elements can be shown as active. Yes, we could skip that, make everything appear to be clickable and let the backend reject what the user can't do, but then the user experience wouldn't be all that great.

If we will check it in first request, it will render buttons correctly, so there won't be any issue, and if something changes (user log out, change permissions etc.) request will fail and user will be redirected or will have to reload, that will send new first request and receive updated privileges. So this should work fine.

Lukshio · 2026-05-26T15:31:04Z

+    statusLabel === STATUS.FAILED ||
+    statusLabel === STATUS.SUCCEEDED ||
+    statusLabel === STATUS.CANCELLED;
+  const autoRefresh = task?.state === STATUS.PENDING || false;


As this is bigger refactor. We have some inconsistency across the states like this one. Auto refresh is in status pending, but it should be also in status running which we are not checking. Same issue with the Create Report button, where user can create report when the job is not finished yet. Could we introduce new state RUNNING and adjust this check for it? Or convert pending state to array and then check it like that.

If you consider it as another task, I'm okay with that.

Create Report bug: https://redhat.atlassian.net/browse/SAT-44552

I would agree that the frontend should cover all the possible values that it might get from the backend one way or another. Filed https://projects.theforeman.org/issues/39360

I would prefer to solve the task after this refactor, so we don't change each others part, wdym?

adamruzicka · 2026-07-07T07:38:48Z

+        return unless @include_permissions
+        @task_by_host = template_invocations.to_h do |ti|
+          task = ti.run_host_job_task
+          [ti.host_id, task]
+        end
+        @permissions_by_host = hosts.to_h do |host|
+          template_invocation = template_invocations_by_host_id[host.id]
+          task = template_invocation.try(:run_host_job_task)
+          [host.id, {
+            :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => task),
+            :cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation),
+            :execute_jobs => authorized_for(controller: :job_invocations, action: :create) && (!host.infrastructure_host? || User.current.can?(:execute_jobs_on_infrastructure_hosts)),
+          }]
+        end


Not happy about this at all.

Alternatives would be:

Having a ui-specific api-like controller

as a completely separate class

as a subclass of pi::V2::JobInvocationsController,only adding bits on top

graphql

Querying two different controllers on every poll is out of the question.

adamruzicka · 2026-07-07T07:39:09Z

-  Date.prototype.toLocaleString = function (locale, options) {
-    return originalToLocaleString.call(this, locale, { ...options, timeZone: 'UTC' });
+  Date.prototype.toLocaleString = function(locale, options) {
+    return originalToLocaleString.call(this, locale, {
+      ...options,
+      timeZone: 'UTC',
+    });


adamruzicka · 2026-07-07T07:40:12Z

+          if (!jobFinishedRef.current) {
+            pollTimeoutId.current = setTimeout(pollHostTable, 5000);
+          } else {
+            pollTimeoutId.current = null;
+          }


If we wanted to go the extra mile, we could stop refreshing the page once all the hosts on the current page reach a terminal state.

MariaAga

Shallow-ish review for JS:

Host table stale on job id change (JobInvocationHostTable.js:261): When React Router reuses the component for a different job (via breadcrumbs), the effect guard checks filterChanged || statusChanged but not id change, so it never re-fetches.
TemplateInvocation re-fetches finished invocations (TemplateInvocation.js:108): The isEmpty(responseRef.current) guard was removed, so every expand/collapse cycle triggers an extra HTTP request even for completed invocations.
run recurring job -> be in the page when it goes from pending to done -> "disable recurring" button is disabled
Go to a done recurring job -> click "disable recurring" -> see success alert, but the button is still showing disable instead of enable (fixes on refresh)

MariaAga · 2026-07-09T15:10:18Z

@@ -0,0 +1,158 @@
+import configureMockStore from 'redux-mock-store';


https://github.com/reduxjs/redux-mock-store

The Redux team does not recommend testing using this library. Instead, see our docs for recommended practices, using a real store.

should use renderWithStore with real store

MariaAga · 2026-07-09T15:15:45Z

Host table polling has no test coverage — no tests for makeApiCall's setTimeout chain, the jobFinishedRef pattern, cleanup on unmount, or behavior on id change

TemplateInvocation polling has zero test coverage — the cancelled flag pattern and isExpanded lifecycle aren't tested

Lukshio

I found same issues as Maria, adding just one thing + nitpick

Lukshio · 2026-07-09T18:19:28Z

-          handleError: () => setStatus(STATUS_UPPERCASE.ERROR),
+          key: JOB_INVOCATION_HOSTS,
+          url: `/api/job_invocations/${id}/hosts`,
+          params: { include_permissions: true, ...requestParams },


This always calls with include_permissions we probably should also check only once because this will trigger backend check every X seconds.

Eh, yes, but we can't really load all the permissions for all the hosts up front. And while we could keep track whether we already fetched permissions for the current displayed page, it felt like I'd be overcomplicating things with that.

I took a stab at it. It seems calculating all those permissions is a more expensive operation than I even expected so it is probably worth the complication.

Lukshio · 2026-07-09T18:24:33Z

Out of scope but since we are doing changes :D

Suggested change

? sprintf(__('Could not abort the job %s: %s'), jobId, response)

: sprintf(__('Could not cancel the job %s: %s'), jobId, response);

ofedoren

Review of PR #1035

Good work overall — the setInterval → setTimeout chain migration is the right pattern, removing list_jobs_hosts and consolidating into the API endpoint cleans up a real problem, and the dead code removal (updateJob, GET_TASK, ALL_JOB_HOSTS) is welcome. The test coverage additions are solid.

Four things that should be addressed before merge:

1. `cancelRecurringLogic` doesn't re-fetch the job (bug)

enableRecurringLogic correctly calls fetchJobInvocation in its handleSuccess, but cancelRecurringLogic had its handleSuccess: () => dispatch(updateJob(jobId)) removed with no replacement. The jobId parameter was also dropped from the signature.

This is the root cause of the reported issue: "click 'disable recurring' → success alert, but the button still shows 'disable' instead of 'enable'".

Fix: restore jobId in the signature and add the same fetchJobInvocation call that enableRecurringLogic uses. Update the call site in JobInvocationToolbarButtons.js to pass jobId back.

2. Redundant per-host permission checks in the controller

In set_statuses_and_smart_proxies, two of the three permission checks inside the hosts.to_h loop don't depend on the host:

:cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation),
:execute_jobs => authorized_for(controller: :job_invocations, action: :create) && (...)

cancel_job_invocations checks the same @job_invocation for every row. authorized_for(controller: :job_invocations, action: :create) is also host-independent. Only host.infrastructure_host? varies. These should be hoisted before the loop — on a page of 20 hosts that's ~40 redundant authorization calls per request, which directly contradicts the performance intent of this PR.

3. No Ruby tests for the new controller behavior

The old list_jobs_hosts action is deleted and its replacement is a modified API endpoint with conditional response fields. There's no backend test coverage for:

Response shape with/without include_permissions=true
task and permissions nodes in the hosts response
The cancellable field on the task child in main.json.rabl
Behavior when run_host_job_task returns nil (the rabl template guards with task &&, but the controller passes nil straight into authorized_for)

4. Module-level `pollTimeoutId` in `JobInvocationActions.js`

let pollTimeoutId = null is a module-scope singleton. The host table and TemplateInvocation both correctly use useRef for their timeout IDs. This one should match that pattern. It's not a bug in today's single-detail-page flow, but it's architecturally inconsistent with the rest of the PR and will silently break if React Router ever renders two detail pages concurrently during transitions.

Suggested PR split

This PR has 33 commits across 18 files. Extracting the independent pieces would make the core change easier to review and unblock merging the simpler parts sooner.

Extractable PR 1: TemplateInvocation polling rewrite

Commits (TemplateInvocation.js portions of):

aace775b Replace setInterval with setTimeout chain in TemplateInvocation polling
7bdf7a02 Apply simplifications from code review
26ca0320 Fix TemplateInvocation poll race and drop dead jobId params
0ea21766 Fix host table staleness on id change and redundant fetches for finished invocations
cb43e25a Fix polling cleanup inconsistencies

Touches only TemplateInvocation.js — it doesn't import or reference anything changed elsewhere in the PR. Cherry-picks cleanly onto master. This alone removes ~300 lines of test code from the main PR's review surface.

Extractable PR 2: Standalone bug fixes and cleanup

These three cherry-pick cleanly onto master individually and don't depend on each other or on the polling rewrite:

6733116a Remove updateJob — its response was never read
bbcbbbe8 Fix host table firing duplicate API calls on mount
3fc0b7ba Use sprintf placeholders for error response in cancelJob toast

One file each, self-contained, easy to review and merge first.

What must stay together: the core

Everything else forms one interdependent chain and can't be split further:

Main job invocation polling rewrite (ece91808, 8061dce1, 6c431762, b39d4adf, 9f735332) — setInterval → setTimeout, stop-on-finish logic, autoRefresh cleanup
Drop getTask, inline cancellable (861ceb2f, 9677991d) — depends on (1) via stopJobInvocationPolling import in index.js
Host table polling (7779ceba, c5963e43, 96e0aae4, a6d19779) — depends on isJobFinished from (1)
Backend consolidation (41ed2d32, cd207a6b, 9bf9f059, 2421ab32, 9f45cd36) — task+permissions in hosts API, delete list_jobs_hosts; frontend consumer changes depend on (3)
Tests and lint (b3201918, 151ad413, 21c1d12b, 50d49af0, 8d139475, f46dde28, 1258caa8, f52f7bb9)

After extracting PRs 1 and 2, the core drops from 33 to ~25 commits and from 18 to ~12 files.

This review was generated with the assistance of Claude Code (claude-opus-4-6).

ofedoren · 2026-07-13T13:58:23Z

The first 4 points seem to make sense, but needs another human/tool to check.

The split suggestion is just a suggestion, although if it's doable, I'd vote for that.

Also, this needs a rebase :/

adamruzicka · 2026-07-13T14:02:04Z

cancelRecurringLogic doesn't re-fetch the job (bug)

Well, except it didn't do that before either. It used updateJob, which put the response into a place in the redux store from where it was never read. That's the reason why I could just drop it.

Edit:

Also, this needs a rebase :/

Oh neptune

Replace setInterval-based polling (via withInterval) with a setTimeout chain that only schedules the next request after the previous one completes or fails, preventing request pileup when the server is slow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add a cancelled flag inside the TemplateInvocation useEffect closure so that handleSuccess/handleError callbacks from in-flight fetches are ignored after the effect tears down. Without this, a handleError arriving after a fast isExpanded toggle could null out the new live timeoutRef, silently breaking polling. Also remove the now-unused jobId parameter from enableRecurringLogic and cancelRecurringLogic — it was only ever passed to the removed updateJob dispatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Adds a self-scheduling setTimeout chain in JobInvocationHostTable that refreshes the host list every 5s while the job is not finished. Uses a ref for statusLabel so the poll callback doesn't need to be recreated on every job status update. filterApiCall cancels any pending poll before issuing a manual fetch to avoid races. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

RowActions reads permissions from selectTemplateInvocationList which is keyed by LIST_TEMPLATE_INVOCATIONS. It was only fetched once on mount (page 1 hosts), so switching pages left it stale — RowActions returned null for any host not in the initial result, hiding the actions column. Now filterApiCall and pollHostTable both refresh LIST_TEMPLATE_INVOCATIONS alongside the main hosts fetch, keeping it in sync with whatever page is currently visible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The RowActions component needs per-host task and permissions data to render the actions column. Previously this came from a separate non-API endpoint (list_jobs_hosts) fetched once on mount, causing the actions column to disappear on page switches since the stale data had no entries for hosts on pages 2+. Now set_statuses_and_smart_proxies builds @task_by_host and @permissions_by_host from the already-loaded template invocations and exposes them in the hosts.json.rabl response. RowActions reads directly from the paginated JOB_INVOCATION_HOSTS Redux key instead of a separate LIST_TEMPLATE_INVOCATIONS key. Drops ALL_JOB_HOSTS, LIST_TEMPLATE_INVOCATIONS, handleResponse, selectTemplateInvocationList, and the non-API /job_invocations/:id/hosts fetch entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Task and permissions data is now included in /api/job_invocations/:id/hosts, so the separate non-API list_jobs_hosts action is no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…s param The per-host authorization checks are expensive — skip them unless the caller explicitly requests them with include_permissions=true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The parent already knows whether the job is done — move isJobFinished out of the table and into JobInvocationActions where the STATUS constants live, export it, and let index.js pass a plain boolean prop down. The table no longer needs to know what constitutes "finished"; it just checks the prop via a ref (same pattern, cleaner responsibility split). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Inline scheduleNextPoll (single-line wrapper with no benefit) and extract updateHostsState to deduplicate the repeated state-update block shared between pollHostTable and makeApiCall. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

cancellable? is only defined on DynflowTask; the :some_task factory uses the base class, causing test failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The frontend only needs task id (for links) and cancellable (to gate cancel actions). Shape the response in the rabl template rather than serializing all task attributes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

pollHostTable was a no-args wrapper that always called makeApiCall with currentPollParams.current, so it can be replaced by an inline lambda. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Extract a shared extractErrorMessage helper to deduplicate the error toast fallback chain across fetchJobInvocation, cancelJob, enableRecurringLogic, and cancelRecurringLogic. Also normalize the inconsistent final fallback ('Error' vs 'Unknown error.') to always use 'Unknown error.'. Replace the redundant run_host_job_task re-lookup in set_permissions_by_host with a direct lookup from @task_by_host, which was already built from the same association. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tTable Tests the setTimeout chain, cancelled flag, isExpanded lifecycle, cleanup on unmount, and jobFinishedRef behaviour that were previously untested. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Auto-fix prettier formatting in JobInvocationActions.js, JobInvocationToolbarButtons.js, and three test files - Split TemplateInvocation.test.js: move polling describe block to new TemplateInvocationPolling.test.js to stay under max-lines limit and eliminate no-shadow violations (store variable) - Name anonymous function assigned to Date.prototype.toLocaleString in MainInformation.test.js to satisfy func-names rule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…hed invocations When React Router reuses JobInvocationDetailPage for a different job via breadcrumb navigation, the host table guard effect was not checking for id changes, leaving the table stale. Add a prevId ref so an id change triggers a re-fetch alongside the existing filter/status guards. TemplateInvocation was unconditionally calling dispatchFetch() on every expand, even for invocations that were already finished and had a full response in the Redux store. Guard dispatchFetch() behind response?.finished !== true to skip the extra HTTP request on collapse→expand cycles for completed invocations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The host table polls the API every 5 seconds while a job is running. Previously every poll included include_permissions=true, causing a backend permission check on each request. Now permissions are only fetched on user-initiated calls (initial load, page change, search/filter). Poll calls omit include_permissions and instead merge the cached permissions back into each result so RowActions continues to render correctly. RowActions previously read permissions directly from the Redux store, which was overwritten by each poll response (without permissions), causing actions to disappear. It now receives permissions as a prop from the table row where the merged data lives.

authorized_for(:cancel_job_invocations) and authorized_for(create job_invocations) return the same value for every host, so calling them inside the hosts loop was redundant. Likewise User.current.can? for execute_jobs_on_infrastructure_hosts is user-scoped, not host-scoped. Pre-compute all three before the loop and reuse the cached booleans, keeping only host.infrastructure_host? inside the iteration.

The module-level singleton is architecturally inconsistent with how JobInvocationHostTable and TemplateInvocation manage their timeout IDs, and would silently break if two detail pages were ever rendered concurrently during React Router transitions. Move ownership of the timeout ID into the component via useRef, threading the ref through getJobInvocation and stopJobInvocationPolling. Callers that do not own the polling lifecycle (enableRecurringLogic) omit the argument and rely on the default { current: null } in fetchJobInvocation.

…ermissions - Add test coverage for the #hosts API endpoint: - Response shape without include_permissions (task/permissions absent) - Response shape with include_permissions=true (task/permissions present, task includes id and cancellable fields, permissions includes all three keys) - Nil run_host_job_task when include_permissions=true (no crash, task is nil) - Add test for cancellable field in the task node of the #show response - Fix: guard view_foreman_tasks permission check with 'task &&' so that authorized_for is not called with a nil auth_object when a host has no run_host_job_task yet; previously this returned true for admin users even though there is no task to view

adamruzicka · 2026-07-13T15:02:09Z

Rebased and addressed point 2, 3 and 4. I'll see what could be done about squashing (or splitting) this into a more sane shape. It sort of grew organically into the tangled mess it is now as I kept uncovering things :/

adamruzicka · 2026-07-14T11:28:53Z

I'll see what could be done about squashing (or splitting) this into a more sane shape.

Would this be better https://github.com/theforeman/foreman_remote_execution/compare/master...adamruzicka:perf-clean?expand=1 ? It is a bit of an approximation, but probably still better than this.

ofedoren

Re-review of PR #1035 (post-rebase)

Thanks for addressing findings 2, 3, and 4 from the previous review — the permission hoisting (5ee41fe7), Ruby tests (5bd334d6), and useRef migration (022c4fa0) all look correct. The nil task guard fix is good too.

Two remaining issues:

1. `cancelRecurringLogic` still doesn't re-fetch the job (bug — unfixed from previous review)

cancelRecurringLogic still has no handleSuccess callback and still drops jobId from its signature. enableRecurringLogic was fixed (it calls fetchJobInvocation on success), but cancelRecurringLogic was not.

This is still the root cause of Maria's finding #4: "click 'disable recurring' → success alert, but the button still shows 'disable' instead of 'enable'".

2. `enableRecurringLogic` call site drops `jobId` — broken re-fetch (new bug)

In JobInvocationToolbarButtons.js:139, enableRecurringLogic is called with only two arguments:

enableRecurringLogic(recurrence?.id, recurringEnabled)

The function signature is (recurrenceId, enabled, jobId), and the handleSuccess calls fetchJobInvocation(dispatch, /api/job_invocations/${jobId}). With jobId missing, this fetches /api/job_invocations/undefined.

The original code (on master) passed all three: enableRecurringLogic(recurrence?.id, recurringEnabled, jobId). The third argument was dropped during the PR.

Same issue applies to cancelRecurringLogic — the original call was cancelRecurringLogic(recurrence?.id, jobId), now it's cancelRecurringLogic(recurrence?.id).

Bonus: orphaned polling from `enableRecurringLogic` after the `useRef` migration

fetchJobInvocation now takes an optional pollTimeoutRef defaulting to { current: null }. When enableRecurringLogic calls it without passing the component's ref, the timeout ID is stored in a throwaway object. If the job is still running, this starts a polling loop that the component's cleanup (stopJobInvocationPolling(pollTimeoutRef)) cannot cancel — it uses a different ref. The loop will self-terminate when the job finishes, but if the user navigates away before that, it becomes an orphaned poll.

Fix: either pass pollTimeoutRef through (requires threading it from the component), or have enableRecurringLogic do a single fetch without scheduling follow-up polls (e.g. by passing a flag or not passing handleSuccess's poll logic).

This review was generated with the assistance of Claude Code (claude-opus-4-6).

pr-processor Bot added Not yet reviewed Waiting on contributor labels May 26, 2026

adamruzicka force-pushed the perf branch 2 times, most recently from 7adeb14 to 671573e Compare May 26, 2026 13:46

Lukshio reviewed May 26, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label May 26, 2026

Lukshio self-assigned this May 26, 2026

adamruzicka force-pushed the perf branch from 671573e to 613e793 Compare May 27, 2026 12:19

pr-processor Bot added the Needs re-review label May 27, 2026

adamruzicka force-pushed the perf branch from 613e793 to 25c50ae Compare May 27, 2026 12:32

pr-processor Bot removed the Needs re-review label May 27, 2026

sbernhard mentioned this pull request Jun 11, 2026

Fixes #39336 - Batch taxonomy subtree lookups to avoid N+1 queries theforeman/foreman#10990

Merged

adamruzicka force-pushed the perf branch from 25c50ae to 43583cf Compare June 25, 2026 12:39

pr-processor Bot added the Needs re-review label Jun 25, 2026

adamruzicka force-pushed the perf branch from 43583cf to 8061dce Compare July 2, 2026 13:11

adamruzicka marked this pull request as ready for review July 2, 2026 14:55

adamruzicka commented Jul 7, 2026

View reviewed changes

pr-processor Bot added Needs re-review and removed Needs re-review labels Jul 7, 2026

MariaAga requested changes Jul 9, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Jul 9, 2026

Lukshio reviewed Jul 9, 2026

View reviewed changes

pr-processor Bot added the Needs re-review label Jul 10, 2026

adamruzicka force-pushed the perf branch from 7d06a8c to 9f45cd3 Compare July 10, 2026 10:06

pr-processor Bot removed the Needs re-review label Jul 10, 2026

ofedoren reviewed Jul 13, 2026

View reviewed changes

adamruzicka and others added 24 commits July 13, 2026 16:27

Make lints happy

4cc5429

Drop list_jobs_hosts — replaced by API hosts endpoint

491fc93

Task and permissions data is now included in /api/job_invocations/:id/hosts, so the separate non-API list_jobs_hosts action is no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Gate task and permissions in hosts response behind include_permission…

fa93cc0

…s param The per-host authorization checks are expensive — skip them unless the caller explicitly requests them with include_permissions=true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix polling cleanup inconsistencies and error toast fallbacks

7f18a92

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Make lints happy

48a1892

Use try(:cancellable?) to handle base ForemanTasks::Task in tests

945c0f0

cancellable? is only defined on DynflowTask; the :some_task factory uses the base class, causing test failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Trim task data in hosts response to id and cancellable

e7ffafc

The frontend only needs task id (for links) and cancellable (to gate cancel actions). Shape the response in the rabl template rather than serializing all task attributes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Unify pollHostTable into makeApiCall

0b692d9

pollHostTable was a no-args wrapper that always called makeApiCall with currentPollParams.current, so it can be replaced by an inline lambda. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace redux-mock-store with a real Redux store in polling tests

43842bf

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add polling test coverage for TemplateInvocation and JobInvocationHos…

9a5008c

…tTable Tests the setTimeout chain, cancelled flag, isExpanded lifecycle, cleanup on unmount, and jobFinishedRef behaviour that were previously untested. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Use sprintf placeholders for error response in cancelJob toast

9b0664d

adamruzicka force-pushed the perf branch from a6d1977 to 5bd334d Compare July 13, 2026 15:00

pr-processor Bot added the Needs re-review label Jul 13, 2026

ofedoren reviewed Jul 14, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Jul 14, 2026

		@@ -0,0 +1,158 @@
		import configureMockStore from 'redux-mock-store';


	? sprintf(__('Could not abort the job %s: %s'), jobId, response)
	: sprintf(__('Could not cancel the job %s: %s'), jobId, response);

Uh oh!

Conversation

adamruzicka commented May 26, 2026

Uh oh!

adamruzicka commented May 26, 2026

Uh oh!

Lukshio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukshio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Review of PR #1035

1. cancelRecurringLogic doesn't re-fetch the job (bug)

2. Redundant per-host permission checks in the controller

3. No Ruby tests for the new controller behavior

4. Module-level pollTimeoutId in JobInvocationActions.js

Suggested PR split

Extractable PR 1: TemplateInvocation polling rewrite

Extractable PR 2: Standalone bug fixes and cleanup

What must stay together: the core

Uh oh!

ofedoren commented Jul 13, 2026

Uh oh!

adamruzicka commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamruzicka commented Jul 13, 2026

Uh oh!

adamruzicka commented Jul 14, 2026

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Re-review of PR #1035 (post-rebase)

1. cancelRecurringLogic still doesn't re-fetch the job (bug — unfixed from previous review)

2. enableRecurringLogic call site drops jobId — broken re-fetch (new bug)

Bonus: orphaned polling from enableRecurringLogic after the useRef migration

Uh oh!

Reviewers

Assignees

Labels

1. `cancelRecurringLogic` doesn't re-fetch the job (bug)

4. Module-level `pollTimeoutId` in `JobInvocationActions.js`

adamruzicka commented Jul 13, 2026 •

edited

Loading

1. `cancelRecurringLogic` still doesn't re-fetch the job (bug — unfixed from previous review)

2. `enableRecurringLogic` call site drops `jobId` — broken re-fetch (new bug)

Bonus: orphaned polling from `enableRecurringLogic` after the `useRef` migration