[Fix][Zeta] Refresh job logs when job id changes

[zhaoysg]

Purpose

Refresh job log data when the selected job changes.

Changes

• Watch jobId and reload logs immediately and on later prop changes.
• Remove the unused NSpace import.

Tests

• git diff --check
• npm.cmd run type-check was attempted locally, but vue-tsc is unavailable because frontend dependencies are not installed in seatunnel-engine-ui/node_modules.

[DanielLeens]

Thanks for working on this. I re-reviewed the latest head f7767c105fbd from scratch.

What this PR fixes

• User pain: on the Zeta job detail page, the JobLog panel can miss the real job logs because the component tries to load logs before the parent page has finished populating job.jobId.
• Fix approach: replace the one-shot getJobLogs(props.jobId) call with a watch(() => props.jobId, ..., { immediate: true }), so the log request runs again once the prop becomes available.
• One-line summary: this is a small but accurate lifecycle fix for the job log panel, and I do not see a new source-side blocker in the current head.

Full call path I checked

Route /jobs/:jobId
  -> views/jobs/detail.tsx
      -> getJobInfo(jobId) [detail.tsx:61-63]
      -> Object.assign(job, res)
      -> <JobLog jobId={job.jobId}> [detail.tsx:574]

JobLog component
  -> components/job-log/index.tsx
      -> old code: getJobLogs(props.jobId) once during setup
      -> new code: watch(() => props.jobId, ...) with immediate=true
      -> getJobLogs(`/logs/${jobId}?format=json`)
      -> render NCollapse items + iframe log links

I also checked the route lifecycle: the main layout renders <router-view key={route.fullPath}> in layouts/main/index.tsx:66, so a full path change remounts the page. That means the real bug here is the async "prop becomes valid after mount" path inside the same detail page, and this patch addresses exactly that path.

Findings

I did not find a new code-side blocker in the current version.

The only follow-up I would suggest is adding a small frontend regression test for the case where JobLog mounts with an empty/placeholder jobId and then receives the real value later, so this lifecycle bug stays covered in the future. I would treat that as a non-blocking improvement rather than a merge blocker for this patch.

Review conclusion

Conclusion: source blocker cleared, waiting for CI

1. Blocking items

• None from my side in the current source diff.

2. Non-blocking suggestion

• Add a focused regression test for the delayed-jobId update path in JobLog.

Overall, the fix looks correct to me on the actual parent/child lifecycle path. The remaining gate right now is CI: the required Build check is still queued at https://github.com/apache/seatunnel/runs/86071083921, so I am not turning this into an approval yet. Once the required checks are green, this looks good from the source side.

[DanielLeens]

Rechecked the current unchanged head f7767c105fbd again.

My earlier source-side blocker on this same revision still stands from Daniel's side. Separately, this branch is also behind the latest dev (compare status: diverged, behind_by=3), and the current red Build signal does not look actionable on top of this stale head yet.

So at this point I would treat the next steps as two separate gates:

1. the existing source-side blocker on the current head still needs to be fixed
2. please sync the latest dev and rerun CI first using the current failing run as the reference point here:
   https://github.com/apache/seatunnel/actions/runs/29003968170/job/86070979674

If the updated head is still red after the sync, please paste the new failing check link and I can help narrow it down against the refreshed revision.

[SEZ9]

Thanks for working on this. I re-reviewed the latest head f7767c105fbd from scratch.

What this PR fixes

• User pain: The job log panel fetched logs only once on component setup, so switching to a different job left stale logs from the previously selected job on screen.
• Fix approach: Replace the one-shot getJobLogs(props.jobId) call with a watch on props.jobId (with immediate: true and a falsy-guard) so logs reload both on mount and on every subsequent jobId change; also drops the now-unused NSpace import.
• One-line summary: The fix is correct and idiomatic Vue, but the async fetch inside the watcher has no stale-response guard, so rapid jobId switches can race and render the wrong job's logs, and the change ships without any test or completed type-check.

Runtime chain I rechecked

JobLog component setup(props)  seatunnel-engine-ui/src/components/job-log/index.tsx:30
  watch(() => props.jobId, handler, { immediate: true })  index.tsx:32-39
    handler fires immediately on mount and again on each jobId prop change  index.tsx:34
      guard: if (!jobId) return  index.tsx:35
      getJobLogs(jobId)  index.tsx:36 -> seatunnel-engine-ui/src/service/job-log/index.ts (HTTP fetch of job logs)
        .then((res) => (logList.value = res))  index.tsx:36  <-- assignment is unconditional; a slow response for an old jobId can overwrite logs for the currently selected job
  render: logList.value mapped into NCollapse/NCollapseItem  index.tsx:41-48

Findings

Issue 1: Race condition: out-of-order responses when jobId changes quickly can display the wrong job's logs

• Location: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:36
• Why it matters: - Root cause: the watcher fires getJobLogs(jobId) on every jobId change but never cancels or guards the previous in-flight request. If the user switches from job A to job B while A's request is still pending, and A's response arrives after B's, logList.value = res at seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:36 overwrites the correct data with stale logs.
• Impact: operators on the Zeta engine UI can see logs labeled for job B that actually belong to job A — a misleading result that is worse than an empty view, especially on slow REST endpoints or large log payloads.
• Fix: capture the current jobId in the closure and compare before assigning, e.g. getJobLogs(jobId).then((res) => { if (jobId === props.jobId) logList.value = res }), or use an incrementing request token / AbortController to invalidate superseded requests.
• Evidence: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:36 getJobLogs(jobId).then((res) => (logList.value = res))
• Suggested fix: There is a stale-response race here: if jobId changes while a previous getJobLogs call is still in flight, the older response can resolve last and overwrite logList with the wrong job's logs. Please guard the assignment, e.g. getJobLogs(jobId).then((res) => { if (jobId === props.jobId) logList.value = res }), or track a request token so only the latest request wins.
• Severity: Medium

Issue 2: No test or verified type-check for the reworked watcher logic; the fixed behavior is unguarded against regression

• Location: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:32
• Why it matters: - The PR description states vue-tsc could not be run locally, so the only changed file has not even been type-checked, let alone covered by a component test.
• The exact bug being fixed (logs not refreshing on jobId change) is behavioral and easy to regress if someone later refactors the setup function back to a one-shot fetch.
• Fix: add a Vitest component test (the seatunnel-engine-ui module already uses Vue 3 tooling) that mounts the component, updates the jobId prop, and asserts getJobLogs is called again with the new id and that { immediate: true } triggers the initial load; at minimum confirm npm run type-check and npm run build pass in CI before merge.
• Suggested fix: Since the PR notes vue-tsc could not be run locally, please make sure npm run type-check/build pass in CI, and ideally add a small Vitest test that mounts this component, changes the jobId prop, and asserts getJobLogs is re-invoked — that would lock in the refresh behavior this PR fixes.
• Severity: Low

Review conclusion

Conclusion: can merge; the notes below are non-blocking

2. Suggested (non-blocking) follow-ups

• Issue 1: Race condition: out-of-order responses when jobId changes quickly can display the wrong job's logs
• Issue 2: No test or verified type-check for the reworked watcher logic; the fixed behavior is unguarded against regression

Nice work — thanks again for the contribution. Happy to discuss any of the above.

[DanielLeens]

Thanks @SEZ9 for the fresh review. I agree the stale-response race in the job-log watcher is the main code-side risk to address before this can move forward; it is aligned with the concern that the UI must not render logs from an older jobId after a fast switch.

No new commit has landed after that review yet, so I am not doing another full Daniel re-review in this round. Once the branch is updated, I will recheck the latest head.

[SEZ9]

Thanks for working on this. I re-reviewed the latest head 01945d6ef326 from scratch.

What this PR fixes

• User pain: In the Zeta engine UI, the job log panel fetched logs only once at component setup, so switching to a different job kept showing the previous job's stale logs.
• Fix approach: Replace the one-shot getJobLogs(props.jobId) call in setup() with a watch on () => props.jobId (with { immediate: true } and a falsy-id guard) that refetches and reassigns logList, plus removal of the now-unused NSpace import.
• One-line summary: The fix is small, correct for the reported symptom, and keeps the initial-load behavior via immediate: true, but the watcher has no request cancellation/staleness guard, so rapid jobId switches can race and render the wrong job's logs, and neither a component test nor a successful vue-tsc run backs the change.

Findings

Issue 1: Out-of-order responses can display the wrong job's logs when jobId changes rapidly

• Location: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:36
• Why it matters: - Root cause: the watch callback fires a new getJobLogs(jobId) request on every jobId change, but the .then unconditionally writes into logList.value. If the user switches from job A to job B and A's (slower) response resolves after B's, the UI will show job A's logs under job B's view.
• Impact: operators inspecting Zeta job logs through the engine UI can be shown log content belonging to a different job than the one selected, which is misleading during incident triage and is a mild cross-job information-exposure in shared-console scenarios.
• Fix: guard the assignment with a staleness check, e.g. capture the requested id and only assign when it still matches the current prop: getJobLogs(jobId).then((res) => { if (jobId === props.jobId) logList.value = res }), or use an incrementing request token / AbortController to cancel superseded requests.
• Suggested fix: This .then assigns to logList.value unconditionally, so if the user switches jobs quickly, a slower response for the previous job can overwrite the logs of the currently selected job. Please add a staleness guard, e.g. getJobLogs(jobId).then((res) => { if (jobId === props.jobId) logList.value = res }), or track a request token/AbortController so only the latest request wins.
• Severity: Medium

Issue 2: No component test for the new watch behavior; type-check was not actually run

• Location: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:32
• Why it matters: - Root cause: the PR adds reactive re-fetch logic but ships no Vitest/@vue/test-utils coverage, and the description states vue-tsc could not be executed locally because seatunnel-engine-ui/node_modules was not installed, so even static type verification of the new watch signature is unconfirmed.
• Impact: regressions in this component (e.g. the race and stale-state issues flagged above, or a future refactor of getJobLogs) would go undetected; the engine-ui module already has a test setup that this change bypasses.
• Fix: add a component test that mocks getJobLogs, mounts the component, updates the jobId prop, and asserts a second fetch occurs and logList is replaced; also run npm run type-check in CI-equivalent conditions before merge.
• Evidence: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:33 () => props.jobId,
• Suggested fix: Please add a small component test (mock getJobLogs, mount, change the jobId prop, assert a second fetch and list refresh) — this new watch is exactly the kind of reactive logic that regresses silently. Also please run npm run type-check with dependencies installed before merge, since it was skipped per the PR description.
• Severity: Low

Issue 3: Consider watchEffect or explicit oldValue check to keep the watcher minimal

• Location: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:32 watch(
• Why it matters: - Root cause: the watcher only depends on props.jobId and immediately uses it, which is exactly the pattern watchEffect covers with less boilerplate; alternatively the current watch is fine but should stay a single-source watcher.
• Impact: purely stylistic — no functional difference — but consolidating on one idiom keeps the seatunnel-engine-ui components consistent.
• Fix: either keep the current watch(() => props.jobId, ..., { immediate: true }) (acceptable) or switch to watchEffect(() => { const id = props.jobId; if (!id) return; ... }); whichever is chosen, pair it with the staleness guard from the other comment.
• Evidence: seatunnel-engine/seatunnel-engine-ui/src/components/job-log/index.tsx:38 { immediate: true }
• Severity: Low

Review conclusion

Conclusion: can merge; the notes below are non-blocking

2. Suggested (non-blocking) follow-ups

• Issue 1: Out-of-order responses can display the wrong job's logs when jobId changes rapidly
• Issue 2: No component test for the new watch behavior; type-check was not actually run
• Issue 3: Consider watchEffect or explicit oldValue check to keep the watcher minimal

Nice work — thanks again for the contribution. Happy to discuss any of the above.

[DanielLeens]

Thanks for the update. I re-reviewed the latest head 01945d6ef326 against the actual Zeta UI job-detail lifecycle.

What this PR fixes

• User pain: the JobLog component previously loaded logs only once in setup(), so when the parent detail page populated or changed job.jobId after mount, the log panel could keep the old or empty result.
• Fix approach: replace the one-shot getJobLogs(props.jobId) call with a watch(() => props.jobId, ..., { immediate: true }), so initial load and later jobId updates both trigger a log fetch.
• One-line summary: this is the correct Vue lifecycle fix for the delayed-jobId path.

Runtime chain I rechecked

Job detail route
  -> layouts/main/index.tsx renders router-view with key={route.fullPath}
      -> navigating to another job detail remounts the detail page
  -> views/jobs/detail.tsx
      -> getJobInfo(route job id)
      -> Object.assign(job, res)
      -> <JobLog jobId={job.jobId}>

JobLog component
  -> setup(props)
      -> watch(() => props.jobId, handler, { immediate: true })
      -> handler ignores empty jobId
      -> getJobLogs(jobId)
      -> logList.value = response
      -> render NCollapseItem / iframe log links

Findings

I do not see a remaining source-level blocker on the latest head.

+1 to @SEZ9's point that guarding stale async responses would make this component more robust. I rechecked the current parent/route lifecycle, though, and I would not keep that as a merge blocker for this PR: the normal cross-job navigation path remounts the page because router-view is keyed by route.fullPath, while this PR's real bug is the same component receiving the real jobId after initial mount. The new watcher handles that path, and the falsy guard avoids the initial empty request.

I would still keep the stale-response guard as a nice follow-up, for example:

getJobLogs(jobId).then((res) => {
  if (jobId === props.jobId) logList.value = res
})

I did not run local frontend tests in this round. This review is based on:

• git diff --stat upstream/dev...01945d6ef326: 1 file changed, 10 insertions and 3 deletions.
• git diff upstream/dev...01945d6ef326 for components/job-log/index.tsx.
• Parent lifecycle lookup in layouts/main/index.tsx and views/jobs/detail.tsx.
• GitHub check metadata for the current Build run.

Current CI / merge gate

The aggregate Build check is red: https://github.com/apache/seatunnel/runs/86718101675

The linked fork workflow run is accessible and does not show a source failure from this UI diff: it has 10 successful jobs, 19 cancelled jobs, and 1 skipped job. I do not see a failing job that points back to components/job-log/index.tsx.

So from Daniel's side, the source blocker is cleared. The remaining gate is to rerun/retrigger the Build workflow so the cancelled aggregate result can be replaced with a completed green run.

Merge conclusion

Conclusion: source blocker cleared, waiting for CI

Blocking items:

• None from Daniel's source review on the latest head.
• Remaining gate: rerun Build; the current aggregate result is red because jobs were cancelled/skipped.

Suggested follow-ups, non-blocking:

• Add the stale-response guard above for extra robustness.
• Add a small component test for the delayed-jobId update path when the UI test setup is available.

Overall, this is a small and correct Zeta UI lifecycle fix. I am not approving yet only because the required Build gate is not green.