fix: preserve worker group tag override on 'Run again'#10004
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying windmill with
|
| Latest commit: |
f770136
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5764999f.windmill.pages.dev |
| Branch Preview URL: | https://hugo-win-2142-preserve-worke.windmill.pages.dev |
|
Claude finished @hugocasa's task in 4m 15s —— View job cc @hugocasa Mergeable, but should ideally address nits: one behavioral edge with ReviewI traced the tag through all four touched files plus the backend push path. The frontend wiring is clean and correct:
One edge worth a look (P2)
The gap: for a tag template like Your PR note already documents the "tag config changes between runs" tradeoff; this AGENTS.md compliance
Test coverageFrontend-only change touching Svelte pages + one exported util ( Manual verification already covered in your test plan (DB-verified tag on both paths, refresh survival). The one scenario not in the plan and worth a quick check is the |
Pi ReviewGood to merge. Summary: This PR is a clean, well-scoped fix that correctly threads the worker group tag override through the "Run again" path. All four touched files are internally consistent, the new Test coverage
|
Codex Reviewcc @hugocasa Found 1 issue:
Test coverage Frontend-only diff. The PR reports |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review findings in 8e73c8f:
|
|
I've now read the full diff and all relevant context. The PR has already been through multiple reviews (Claude, Codex, Pi), and the author addressed both prior findings in commit 8e73c8f. Let me produce the final review. Pi ReviewGood to merge. The latest commit (8e73c8f) resolves the AGENTS.md compliance
Test coverageNew unit tests in No further manual verification needed beyond what's already in the test plan. |
Codex Reviewcc @hugocasa Found 2 issues:
Test coverage Frontend-only diff. The new Manual verification before merge: create or use a custom worker tag named Manual verification before merge: land on a “Run again” page with an override tag, change or reset the worker group from Advanced without changing args, refresh the page, and confirm the refreshed form matches the latest worker group selection rather than the original hash value. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the second Codex round in 95b9c19:
|
Pi ReviewGood to merge. This PR has been through three prior review rounds (Claude, Codex, Pi), with two sets of findings addressed in commits What changed in the latest commit (95b9c19)The Verification
Test coverageNew unit tests in No further manual verification needed beyond what's already in the test plan. |
Codex Reviewcc @hugocasa Found 1 issue:
Test coverage Frontend-only diff. The new Manual verification notes: the PR test plan covers normal script/flow reruns, refresh survival, and DB-verified routing. The remaining scenario is a script or flow with an input named |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pi ReviewGood to merge. This PR has been through four prior review rounds (Claude, Pi, Codex, Pi, Codex), with findings addressed in commits What changed since my last review (95b9c19)The Verification
Test coverageNew unit tests in No further manual verification needed beyond what's in the test plan. |
Codex Reviewcc @hugocasa Found 2 issues:
Test coverage Frontend-only diff. The new Manual verification still needed: run a script or flow with a dynamic configured tag and an explicit Advanced worker group override, click “Run again”, and confirm the override remains applied. Also verify a script or flow with an input named |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Now I have a thorough understanding of the full diff and the prior review history. This PR has been through 5 prior review rounds, and the two edge-case issues that were raised (dynamic tags discarding explicit overrides, and Pi ReviewGood to merge. This PR went through five prior review rounds (Claude, Pi, Codex ×3). The two edge cases raised — (1) dynamic-tag scripts lose a manual override on "Run again" because What the diff does
AGENTS.md compliance
Test coverageNew unit tests in No further manual verification needed beyond what's already in the test plan. |
Codex Reviewcc @hugocasa Found 1 issue:
Test coverage Frontend-only diff. The new Manual verification still needed: run a script or flow with an input named |
…keys Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the remaining Codex finding:
|
Pi ReviewGood to merge. This PR has been through seven prior review rounds (Claude, Pi, Codex ×5, Pi, Codex, and repliers), with all substantive findings addressed across commits What the final diff does
Verification of resolved edge cases
AGENTS.md compliance
Test coverageNew unit tests in No further manual verification needed beyond what's already in the test plan. |
Codex ReviewGood to merge No issues found. The latest duplicate-key approach preserves both the carried worker tag and a real Test coverage Frontend-only change. The new I attempted the focused Vitest file locally, but this checkout is missing frontend dependencies ( |
Fixes WIN-2142
Summary
When clicking 'Run again' on a job, the worker group tag override from the original run was lost: the main button only encoded args in the URL hash, and the 'Run immediately with same args' dropdown did not pass a tag to the run APIs. Both paths now carry the original job's tag forward, with dynamic (templated) tags re-resolved instead of pinned.
Changes
computeSharableHash(utils.ts) accepts an optional tag and encodes it in the URL hash under the reserved key__tag, with at:value prefix. No JSON-encoded arg value can start witht:(JSON strings start with", numbers with a digit or-, etc.), so the newextractTagFromSharableHashcounterpart can unambiguously tell a carried tag apart from a genuine arg named__tagfor every possible tag value, including JSON-parseable tags like123. A carried tag and an arg named__tagcoexist as duplicate keys, so both are preserved.job?.taginto the hash, andrunImmediately()passestag: job?.tagtorunScriptByHash/runScriptByPath/runFlowByPath.overrideTagviaextractTagFromSharableHashand removed before the remaining params are parsed as args, so it never leaks into the input form.$args[...]/$workspaceat push time), so pinning it would go stale if the user edits args before re-running. When the loaded script/flow's configured tag is$args[...]-templated (isDynamicTag), the carried tag is dropped so the backend re-resolves it from the submitted args, andRunFormshows a note in place of the tag override line explaining that the previous run's tag was not applied.RunForm's debounced hash rewrite includesoverrideTagand reacts to tag changes as well as arg changes, so the URL stays in sync when the worker group is changed or reset from the Advanced popup, and the tag survives a page refresh or a copied link.utils.test.tscover the round-trip, the__tagarg collision cases, JSON-parseable tags, a tag that itself starts with the value prefix, andisDynamicTag.Notes
Test plan
npm run check:fastpasses;npx vitest run src/lib/utils.test.tspasses (73 tests)?tag=my-custom-tag→ 'Run again' navigates to/scripts/get/...#__tag=t%3Amy-custom-tag&name=..., the form showstag override: my-custom-tag, the arg is prefilled, and submitting creates a job with tagmy-custom-tag(verified in DB)my-custom-tag(verified in DB)/flows/get/...with the tag override captured__tagthrough the debounced rewrite and the override survives a browser refresh__tag(verified in browser)worker-$args[env]run withenv=prod(job tagworker-prod) → 'Run again' shows the dynamic-tag note instead of a tag override; editingenvtodevand running creates a job with tagworker-dev, re-resolved from the edited args (verified in DB)__taground-trip as args, including alongside a carried tag (both preserved); JSON-parseable tags like123are preserved (unit tests + browser check)Screenshots
Script 'Run again' with the original run's tag override prefilled:
Flow 'Run again' with the tag override prefilled:
'Run again' on a job whose script has a dynamic tag (
worker-$args[env]): the resolved tag is not pinned and a note explains why:🤖 Generated with Claude Code