Schema - Fix Add HTTP Header button losing rows on double-click

[geodem127]

Summary

The root cause was a stale closure on headersLocal inside the click handler: the handler captured a snapshot of headers at render time, so a second click in the same render cycle spread {...staleHeaders, [uuid2]: ...}, silently overwriting the row added by the first click.

Two complementary fixes:

• Functional state updater — uses (prev) => ({...prev, [id]: ...}) so each click composes from the latest state rather than a stale closure capture. This is the primary fix.
• uuidv4() for row keys — replaces Date.now().toString() as defense-in-depth: two Date.now() calls in the same millisecond produce the same key, masking the stale-closure bug as a silent overwrite. Unique UUIDs surface any future key collision as a visible problem.

Test plan

• Open Schema app, navigate to an integration field, and open the API configuration
• Double-click "Add HTTP Header" rapidly — confirm both rows appear instead of one being lost
• Verify headers still save and persist correctly

🤖 Generated with Claude Code

NOTE: This PR is required to resolve a failing test #4153

[github-actions]

Review summary

The refactor from Record<string, {key, value}> to a plain {key, value}[] is a clean simplification and the right call — switching Add and Remove to functional updaters (setHeadersLocal((prev) => …)) does fix the original double-click race.

A few follow-ups worth addressing before merge:

1. key={i} on row 244 is an anti-pattern for deletable lists. Deleting an earlier row will cause React to reuse DOM elements for later rows — losing focus position and cursor offset mid-edit. Inline comment with details.
2. The key/value onChange handlers (lines 259–286) still read headersLocal from closure instead of using the functional updater. Inconsistent with the rest of the fix; suggestion posted inline.
3. focusRef.current = headersLocal.length (line 320) is an index, so it drifts after deletions. Mostly cosmetic given autoFocus only fires on mount, but a stable per-row id would fix (1), (3), and avoid bringing back the historical staleness of Date.now() keys.

Test coverage

The existing tests in cypress/e2e/schema/integration.spec.js:302–309 add 3 headers with three sequential .click() calls — Cypress inserts delays between commands, so this does not exercise the double-click-in-same-ms race the PR claims to fix. Consider adding a spec that simulates rapid clicks (e.g. dispatching two click events synchronously, or using cy.get(...).click().click() while asserting both rows render) so the regression is locked down.

Nits

• onChange={(e: any) => …} — the explicit any annotation is unnecessary; ChangeEvent<HTMLInputElement> would be more honest. Pre-existing in this file, not new in the PR, but worth tidying since the surrounding code is being touched.

[github-actions]

Review

The core fix — switching Add HTTP Header from setHeadersLocal({ ...headersLocal, [keyId]: ... }) to the functional form setHeadersLocal((prev) => ({ ...prev, [keyId]: ... })) — correctly resolves the double-click race. Swapping Date.now() for uuidv4() is also the right call as a belt-and-suspenders measure: even with requestAnimationFrame-batched clicks, two Date.now() calls in the same ms would collide, and keyId is also used as the row's React key, so collisions would have caused React reconciliation oddities too.

A few notes worth addressing before merge:

1. PR description doesn't match the implementation

The description's "Note" claims the final implementation switched headersLocal to { key: string; value: string }[] and "eliminated the need for UUID keys entirely." The actual diff still uses Record<string, { key: string; value: string }> keyed by uuidv4(). Either the description is stale (likely — appears to describe an intermediate revision that was reverted) or the code is. The current object-map approach works fine; just update the description so reviewers and the merge commit reflect reality.

2. Test coverage gap

The existing assertions in cypress/e2e/schema/integration.spec.js:302-309 don't actually exercise the double-click race — Cypress retries between commands, so three sequential .click() calls land on three different render ticks. They never sit inside the same render commit, which is exactly the situation that produced the original bug. The current test would have passed against the buggy code as long as Date.now() advanced ~1ms between clicks (which it always does in Cypress's CI environment). Consider adding a regression assertion that dispatches two click events synchronously (e.g. .then($btn => { $btn[0].click(); $btn[0].click(); })) and verifies both rows appear.

3. Minor inline nits

Posted as inline comments.

🤖 Generated with Claude Code

[github-actions]

Re-review on commit 7cc3e8f

The code-level concerns from the previous review pass are resolved in this commit:

• key= no longer uses array index — now key={header-${entryId}} (line 247), which is the uuid. Deleting an earlier row will no longer cause React to reuse DOM nodes for later rows.
• All setHeadersLocal callers use the functional updater — the key/value onChange handlers (lines 262–267, 282–287) now read prev instead of closing over headersLocal, matching the Add/Remove handlers. This means even within a single render tick, typing + add/remove cannot race against each other.
• (e: any) on the new handlers is gone — replaced with React.ChangeEvent<HTMLInputElement>. (The pre-existing onInput={(e: any) => …} on the URL field at line 205 still uses any, but that's not part of this PR's scope.)

Still outstanding

1. PR description still claims an array refactor that isn't in the diff. The "Note" in the description says headersLocal was switched to { key: string; value: string }[] and "UUID keys are no longer needed" — but the code at lines 105–117 still uses Record<string, { key: string; value: string }> keyed by uuidv4(). The object-map approach works (uuid insertion order is stable in modern JS engines, and the functional updaters close the race), so no code change is needed — just sync the description so the merge commit and future readers aren't misled.

2. No regression test for the actual race. cypress/e2e/schema/integration.spec.js:302–309 issues three sequential cy.get(...).click() commands. Cypress's command queue inserts a render tick between them, so each click lands on a different React commit — the very thing that prevented the bug from reproducing. A targeted assertion that fires two clicks inside one tick (.then($btn => { $btn[0].click(); $btn[0].click(); })) and verifies both rows render would lock down the regression. Without it, reverting either the uuid swap or the functional updater would not fail CI.

Minor observation (not blocking)

The component still always renders at least one row (lines 108–111) and the first row's delete button is hidden via {i > 0 && …} (line 291). Combined, this means a user can never get to "zero headers" through the UI — only blank-key rows that handleNext filters out. Pre-existing behavior, not changed here, but worth confirming with design that it's intentional.

🤖 Generated with Claude Code

[geodem127]

Regarding the minor observation: always rendering at least one row and hiding the first row's delete button is intentional per design — the field should never present an empty state with no header rows visible.

[github-actions]

Review

Tight, focused fix — the diff is mostly the right cleanup, and both changes are needed together rather than redundant. A few notes:

Root cause framing

The PR description leads with Date.now() collisions, but the dominant root cause is actually the stale closure on headersLocal in the click handler. Even with unique UUIDs, the old code spread a closure-captured headersLocal snapshot — so a second click in the same render cycle would still {...staleHeaders, [uuid2]: ...} and lose the row added by the first click. The two fixes are complementary:

• Functional updater — necessary to compose multiple state transitions inside a single render cycle.
• uuidv4() — defense-in-depth for the small chance two Date.now() calls land in the same ms (and avoids the silent-overwrite mode the bug exposed).

Worth noting in the description so future readers don't mistakenly think swapping just the key generator would have fixed it.

Missing automated coverage

The Cypress suite has selectors for addHeaderButton / integrationHeadersContainerRow-* in cypress/e2e/schema/integration.spec.js, but the relevant assertions live inside connectToEndpoint(...) which is itself called from an it() block — meaning the nested it(\"Add HTTP Headers\", ...) (line 302) and it(\"Remove HTTP Headers\", ...) (line 312) never register or run under Mocha. That's a pre-existing structural problem, not something this PR introduced, but it's the reason this regression made it to production. Adding a top-level spec that exercises rapid "Add HTTP Header" clicks (and the row-removal flow) would both validate this fix and start covering a flow that currently has zero working assertions.

Nits (non-blocking)

• The URL TextField at L205 still uses onInput={(e: any) => ...} while the headers' new handlers are properly typed as React.ChangeEvent<HTMLInputElement> — minor inconsistency you could clean up while you're here.
• handleNext / handleApiConnect still type their reduce accumulator as any — could be Record<string, string> now that the input shape is well-known. Not regression territory, just an easy win.

LGTM on the fix itself.

[geodem127]

PR description updated to lead with the stale closure as the primary root cause and frame uuidv4 as defense-in-depth.

Nits applied:

• URL TextField onInput/(e: any) → onChange/(e: ChangeEvent<HTMLInputElement>)
• Both reduce accumulators in handleNext/handleApiConnect typed as Record<string, string>

Cypress: acknowledged as a pre-existing structural issue (nested it() calls inside connectToEndpoint() never register under Mocha). Fixing that requires restructuring the spec file — tracking as a separate follow-up.

[github-actions]

Review

Verdict: Looks good — the diagnosis is correct and the fix is well-targeted.

Strengths

• Functional state updater is the right call. The previous setHeadersLocal({...headersLocal, ...}) captures headersLocal at render time, so two clicks dispatched in the same render cycle (rapid double-click before React flushes) both build off the same stale snapshot and the second overwrites the first. Switching to (prev) => ({...prev, [keyId]: ...}) composes from the latest queued state and is the canonical fix for this class of bug.
• uuidv4() for row keys is solid defense-in-depth. Date.now().toString() can legitimately collide on a same-millisecond double-click, which was masking the stale-closure bug as a silent overwrite rather than surfacing it as a visible duplicate-key React warning. With UUIDs, even if the closure bug regressed, the duplicate would render visibly.
• The opportunistic moves to functional updaters in the key/value onChange handlers and the delete IconButton are consistent and worth keeping — they harden the same class of race in adjacent code paths.
• TypeScript tightening (ChangeEvent<HTMLInputElement>, ReactNode imports, reduce<Record<string, string>>) is a nice cleanup, and removing the unnecessary ?. chains on header.key/focusRef.current is safe since the iterated Object.entries values are always defined and useRef's ref object is non-null by construction.

Minor

• onInput → onChange on the URL TextField isn't called out in the PR description and isn't related to the headers bug. In React these are functionally equivalent for text inputs (React's onChange is wired to the native input event), so this is a no-op behavior change — fine to keep, but worth a one-liner in the PR body so reviewers don't have to verify equivalence themselves.
• No regression test added. Acknowledging the bug is hard to reproduce deterministically in Cypress (the true stale-closure race needs two clicks in the same render cycle, not sequential user-style clicks), but a rapid cy.get('[data-cy="addHeaderButton"]').click().click().click() followed by an assertion on row count would at least catch the Date.now() collision side of this in the unlikely event it regresses. Worth considering, not a blocker.

Pre-existing, not in scope

Noting these only so they're on the radar — none are introduced by this PR:

• headersLocal is initialized once from the headers prop and never resyncs if the parent updates headers; fine in the current usage but a latent footgun.
• The delete IconButton is gated on array index i > 0, so when the dialog opens with multiple pre-existing headers from props, the first one is undeletable.
• cypress/e2e/schema/integration.spec.js declares the Add HTTP Headers / Remove HTTP Headers it() blocks inside the connectToEndpoint helper, which is itself called from inside an it(). Nested it() calls don't register in Mocha — those tests don't actually run today. Worth a separate cleanup PR.

Nice clean fix overall.

[github-actions]

Review

Clean, targeted fix. The root-cause diagnosis is accurate and both countermeasures are appropriate.

What's good

• Functional state updaters on every setHeadersLocal call site — this is the correct fix. All three sites (add-header click, key-field onChange, value-field onChange) were updated, so the stale-closure window is fully closed.
• uuidv4() over Date.now().toString() in the add-header click handler — same-millisecond collisions on rapid clicks would silently overwrite the just-added row; UUID eliminates that class of problem.
• onChange replacing onInput on the URL TextField — onInput is a native DOM event; React's onChange already maps to the native input event for text fields, so this is both semantically correct and necessary for the ChangeEvent<HTMLInputElement> type.
• Type improvements — replacing acc: any, obj: any with reduce<Record<string, string>> + destructuring, and pulling ReactNode/ChangeEvent as named imports instead of via React., are real quality gains.
• JSX unwrap — removing the extra return (...) wrapper reduces nesting with no behaviour change.

Concerns

Cypress test coverage gap — The existing integration.spec.js test clicks addHeaderButton three times in sequence and checks for 5 children. Cypress queues each .click() with DOM-retry cycles between them, so this exercises the sequential case but not the same-render-cycle race that triggered the original bug. A dedicated test using { force: true } with back-to-back clicks or a programmatic double-click would confirm the fix actually prevents the regression. Not a blocker, but the test plan in the PR description was manual-only.

setApiData missing from handleApiConnect deps — Pre-existing issue, not introduced here, but setApiData is called inside the useCallback without appearing in [endpointLocal, headersLocal]. Since setApiData is a prop (not a useState setter), a parent re-render could pass a new reference and leave the callback holding a stale one. Fine for now given how the parent likely works, but worth a lint rule (react-hooks/exhaustive-deps) catching it.

See inline comments for two additional minor items.

[geodem127]

PR #4153 fixes failing and flaky tests