fix(connection-form): a new edit target withdraws the dialog's transients (#1180)

[methakon]

Problem

ConnectionModal is published (src/exports/components.ts, and package.json exports ./components), so a host can replace editConnection while isOpen stays true. The standalone app cannot reach this: Studio.tsx clears editingConnection together with isOpen in both onClose and onConnect, and the dialog is modal.

The degraded-save offer in handleConnect saves only on the second click and remembers the first one in degradedSaveAcknowledged. The comment above that branch said the acknowledgement "is withdrawn when the dialog closes", and the close reset block was indeed the only place that withdrew it. The block that applies a new edit target (guarded by appliedEdit.conn !== editConnection) cleared none of the four transient values, so a target that arrived without a close inherited all of them.

The consequence is a silent first save: Y's health read is refused, Y is handed to onConnect on the first click with nothing reported, and X's "click again" banner is shown over it.

Change

The close path's four set* calls and the new call in the edit-load block are now one function, withdrawTransientState, called from both. That is the shape the issue asks for and the reason for it: two copies of that list had already drifted once, which is this bug, and would be free to drift again.

The trigger is the block's existing one, unchanged. A rerender carrying the same target is not a new connection and must not withdraw anything, or the second click becomes unreachable for any host that re-renders while the dialog is open.

The comment above the degraded branch now names both triggers this hook owns and points at #1167 for the third (withdrawal after a successful save), which is that issue's work, not this one's.

No comparison by id or any other new condition was added, and the four fields, the build, and the save path are untouched.

Tests

Two tests in tests/hooks/use-connection-form.test.ts, beside the existing "closing the dialog withdraws the acknowledgement" and in the same shape:

• the new-target side: X open, first handleConnect() warns, rerender with Y while still open. Asserts testResult is null, the name is Y, Y's first handleConnect() does not call onConnect and leaves a warning, and the second saves Y.
• the same-target side: after X's warning, rerender with the same X while open; the next handleConnect() saves X with no second warning. This one passes before the fix as well, and it is here because a fix that withdrew on every render would pass the first test alone.

The first test fails on main and passes with the fix. Proven by sabotage rather than asserted: removing the single new call from the edit-load block and leaving the helper in place turns that test red while the same-target test stays green, and restoring the file returns both to passing.

tests/hooks/use-connection-form.test.ts is 115/115, including the existing degraded-save tests unchanged. use-connection-form.ts is at 100% line coverage.

Verification

• bun run format, lint, typecheck, knip, chart:check, channels:showcase:check, readme:check, security:check all pass
• full suite 690/692 files; the two failures (inspect-schema, run-read-query) are the pre-existing MCP pair that also fails on main and on the openGauss branch

Note on the base

#1157 and #1167 both change this file and the blocks next to this one, and the issue asks for a branch from main after both land. Both are still open, so this is branched from current main and will need a rebase if either lands first. The fix is deliberately confined to the two blocks the issue names, to keep that rebase small.

Fixes #1180

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[cevheri]

Thanks @methakon, this is what #1180 asked for. On main the new-target test fails on testResult and passes here, and the same-target test holds on both.

I pushed 8b101cf, a comment-only change: the note above the degraded branch said a successful save also withdraws the acknowledgement, but no code path does that yet; that is #1167's work. It now names only the two triggers this hook has.

[methakon]

@cevheri this one is ready too. 22 passed, 2 skipped, E2E included.

One thing to flag rather than have you find: the issue asked for a branch from main after #1157 and #1167 merge, and both are still open, so this is branched from current main and will need a rebase if either lands first. I kept the change to the two blocks the issue names for exactly that reason, so the rebase should be small.

The part worth a second look is the trigger. I kept the block's existing appliedEdit.conn !== editConnection condition and added no new comparison, because a rerender carrying the same target is not a new connection and must not withdraw the acknowledgement - otherwise the second click becomes unreachable for any host that re-renders while the dialog is open. There is a test for that side too, and it passes both before and after the fix, which is deliberate: it is there so a fix that withdrew on every render would not pass on the strength of the first test alone.

[methakon]

Thank you, and you are right on 8b101cff - that line was mine and it was wrong.

I wrote "A successful save withdraws it as well (#1167)" on the assumption #1167 had already landed. It has not: it is still open, so no code path does that, and a comment asserting a behaviour the hook does not have is worse than no comment. Naming only the two triggers the hook actually owns is the correct version, and I should have written that from the code rather than from what I expected to be true elsewhere in the tree.

Thank you also for confirming the two tests behave as intended - the new-target one failing on main and passing here, and the same-target one holding on both. That second test is the one I would have been most tempted to drop as redundant, and it is exactly what stopped a fix that withdrew on every render.

My ping landed after you had already merged, which wasted your time on a status update you did not need. Noted for next time: check whether the PR has moved before reporting on it.