Repository navigation
fix(cp): cancel in-flight work when a modal is dismissed - #302
Merged
Merged
Conversation
netravnen
force-pushed
the
fix/cp-modal-cancellation
branch
from
August 19, 2026 22:27
118b972 to
c8525a0
Compare
netravnen
force-pushed
the
fix/cp-modal-cancellation
branch
from
August 19, 2026 22:28
c8525a0 to
d4e9803
Compare
netravnen
force-pushed
the
fix/cp-modal-cancellation
branch
from
August 19, 2026 22:29
d4e9803 to
3123c59
Compare
netravnen
force-pushed
the
fix/cp-modal-cancellation
branch
from
August 19, 2026 22:31
3123c59 to
5b13248
Compare
netravnen
force-pushed
the
fix/cp-modal-cancellation
branch
2 times, most recently
from
August 19, 2026 23:06
28a06a5 to
0c47a18
Compare
Dismissing a CP modal hid the work without stopping it. close() was literally backdrop.remove(), so a backdrop click or Close during an apply tore down the progress display while the loop kept issuing PUT and DELETE against live rows -- with no progress shown and no remaining way to abort, since the Cancel button that sets cancelSignal.cancelled went away with the modal. The explicit Cancel path had always been correct; close() simply did not use it. Two related safeguards were inert for the same reason -- present in the UI, absent in effect. Each opener resolved at its first render, so the toolbar handler's action lock was released while the modal was still open and a second click stacked a second modal, with its own cancelSignal, over the same rows. And the conflict resolver pre-filled its type-to-confirm input from first render, so the banner's "type the ixlan id to enable Apply" gate was satisfied before the admin touched anything. Changes: - close() now sets cancelSignal.cancelled = true before removing the backdrop, in the IXLAN renumber, IX-F member audit and conflict resolver modals. - The conflict resolver and recent-IP-changes modals gain a real close() and a backdrop-click path; both previously had only an inline () => backdrop.remove() on the Close button. - Each of the four openers now resolves on dismissal rather than on first render, so the caller's action lock spans the modal's lifetime. For the read-only recent-changes report this is what makes its existing "report is already open" message true. - Drop the conflict resolver's confirmInput.value pre-fill and stop showing the expected id as the placeholder -- the placeholder is what made an empty field look filled, which is the "false lockout" the pre-fill was originally added to work around. The input listeners that enable Apply were already wired, so the gate now works as the banner describes. - Give the two fire-and-forget modal launches their own .catch(), per the convention added in the pdbFetch commit. Security: - Removes a path where an admin who dismissed a modal mid-apply believed they had stopped a bulk renumber or merge while PUT/DELETE continued against production rows. - Restores the type-to-confirm gate on the destructive conflict resolver, and prevents two concurrent modals from writing to the same ixlan. Testing: - node --test: 518 tests, 517 pass, 1 skipped (live tests are opt-in). - New tests/cp-modal-safeguards.test.js pins all three safeguards. These are source assertions, not behavioral ones, and the file says so at length: the modals are DOM wiring this repo does not unit test, and the shim's FakeElement has no addEventListener, so a modal cannot be constructed or clicked through loadScript. They cannot prove the wiring works -- the AGENTS.md smoke test covers that -- but they do catch the failure that actually happened, which was a safeguard going missing with nothing red. - Each safeguard reverted individually and confirmed red: 4, 10 and 1 failures respectively. Backwards Compatibility: - The four modal openers now settle on dismissal instead of first render. All seven call sites are updated here; three already awaited them inside the action lock, which is the behavior being corrected. - No storage, API or audit-log format changes. Assisted-by: Claude:claude-opus-5
netravnen
force-pushed
the
fix/cp-modal-cancellation
branch
from
August 19, 2026 23:07
0c47a18 to
bf3b975
Compare
netravnen
marked this pull request as ready for review
August 19, 2026 23:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dismissing a CP modal hid the work without stopping it. close() was
literally backdrop.remove(), so a backdrop click or Close during an apply
tore down the progress display while the loop kept issuing PUT and DELETE
against live rows -- with no progress shown and no remaining way to
abort, since the Cancel button that sets cancelSignal.cancelled went away
with the modal. The explicit Cancel path had always been correct; close()
simply did not use it.
Two related safeguards were inert for the same reason -- present in the
UI, absent in effect. Each opener resolved at its first render, so the
toolbar handler's action lock was released while the modal was still
open and a second click stacked a second modal, with its own
cancelSignal, over the same rows. And the conflict resolver pre-filled
its type-to-confirm input from first render, so the banner's "type the
ixlan id to enable Apply" gate was satisfied before the admin touched
anything.
Changes:
backdrop, in the IXLAN renumber, IX-F member audit and conflict
resolver modals.
and a backdrop-click path; both previously had only an inline
() => backdrop.remove() on the Close button.
render, so the caller's action lock spans the modal's lifetime. For the
read-only recent-changes report this is what makes its existing
"report is already open" message true.
showing the expected id as the placeholder -- the placeholder is what
made an empty field look filled, which is the "false lockout" the
pre-fill was originally added to work around. The input listeners that
enable Apply were already wired, so the gate now works as the banner
describes.
convention added in the pdbFetch commit.
Security:
they had stopped a bulk renumber or merge while PUT/DELETE continued
against production rows.
and prevents two concurrent modals from writing to the same ixlan.
Testing:
are source assertions, not behavioral ones, and the file says so at
length: the modals are DOM wiring this repo does not unit test, and the
shim's FakeElement has no addEventListener, so a modal cannot be
constructed or clicked through loadScript. They cannot prove the wiring
works -- the AGENTS.md smoke test covers that -- but they do catch the
failure that actually happened, which was a safeguard going missing
with nothing red.
failures respectively.
Backwards Compatibility:
All seven call sites are updated here; three already awaited them
inside the action lock, which is the behavior being corrected.
Assisted-by: Claude:claude-opus-5
Stack created with GitHub Stacks CLI • Give Feedback 💬