Repository navigation
[JENKINS-73556] Disable Build while reactive parameters are updating - #1039
Conversation
kinow
left a comment
There was a problem hiding this comment.
@Tomer9000 I only allowed the CICD workflows to run here in GitHub, and had a brief look at the code, and it looks great. Thanks a ton for including tests. This looks easy to review and probably include in the next release!
I will have a better look at another time with IDE + some test jobs. Thank you!
|
I tested the build from this PR and it works perfectly. |
|
@kinow Thanks for the quick look and the kind words! Do you have a rough ETA for the next release? I'm deciding whether to run a build from this branch in the meantime or wait for the release, so even a ballpark estimate would help. No pressure at all, I know this is volunteer time. |
|
Hi @Tomer9000 , I could try to have a look at this PR this weekend, but not sure if that'd be enough for a release. After I released the last version I got two new issues created, almost immediately after the release. I haven't had time to confirm, but I suspect it'd be better to fix those issues before cutting a new release.
Next week I have to travel to another city in Spain for some days, and return the other week. I think it'd be quite optimistic to try a release this weekend. But maybe by the end of October, that could be easier to achieve. |
|
(and thanks for the understanding!) |
|
It's bank holiday in Spain today. Got a bit of spare time, so reviewed an issue that seemed serious but cannot be reproduced with the latest version. I'm reviewing an issue with recursive parameters, and once the PR for that one is done and reviewed/merged, I will try to test this one. Hopefully I will manage to complete everything tonight, and then maybe I'll be able to cut a new release with what we have on |
kinow
left a comment
There was a problem hiding this comment.
Works as expected! I couldn't find any bug reading the code nor playing with a simple job, so I will approve it. First, I will just amend the commit to fix conflicts after rebasing, and change the format of JS comments.
The Groovy I used is basically
sleep(4000)
return ['1', '2', '3', '4']That works fine after the script is approved to run 🙂
a734510 to
d20c8a3
Compare
Users could submit the form while cascade/dynamic reference parameters were still recalculating, building with empty or stale values (jenkinsciGH-868). A LoadingGuard counts pending updates: every update is scheduled through scheduleUpdate(), and proxy calls count until their callback has rendered. While anything is pending, the form's submit buttons are disabled and submit is blocked; a failed update keeps them blocked with a reload message. The proxy now also returns the response promise, so awaiting a call waits for its callback (JENKINS-75793, jenkinsciGH-902, same fix as jenkinsci#960). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d20c8a3 to
92ef807
Compare
|
Fixed conflicts after rebasing, added license headers, reformatted docs to match existing code comments (some duplication in types in TS & jsdoc-style, but we can sort that out later). Waiting for GH Actions, if it passes, will approve and merge. |
kinow
left a comment
There was a problem hiding this comment.
LGTM! I think I have only one more issue to review that seems important for 2.8.11.
Thanks for the PR, @Tomer9000 !
Fixes #868 (JENKINS-73556). Also fixes #902 (JENKINS-75793), with the same one-line fix as #960.
Problem
Users can click Build while reactive parameters are still recalculating. The build then gets empty or stale values.
Change
LoadingGuard(src/main/js/LoadingGuard.ts): counts pending parameter updates.submitin the capture phase, which also covers Enter.UnoChoice.loadingGuard, following the "global variable" idea from fix: workaround for the cascading parameters bug #357.scheduleUpdate()runs every update: the initial render, referenced-parameter changes and dependent chaining. The counter goes up when an update is scheduled, so it never reaches zero between a parent finishing and its dependents starting.makeStaplerProxy2tracks calls of cascade and dynamic-reference proxies until their callback has rendered.Testing done
src/test/js/LoadingGuard.test.ts: unit tests covering pending and overlapping updates, idempotent release, failure, leaving other scripts' disabled state alone, and blocking submit.src/test/js/UnoChoice.test.ts: an integration test that loads the realUnoChoice.es6.package.jsonto transform.es6.yarn testpasses 38/38, 8 runs in a row, andyarn buildis OK.mvn verifylocally (no JDK/Maven in this environment), so I'm relying on CI for the Java, HtmlUnit and Selenium tests.Notes for review
makeStaplerProxy2is now logged withconsole.error.inputstep page, Abort is gated together with Proceed while parameters update.Submitter checklist
🤖 Generated with Claude Code