Repository navigation
Bind on the Domains page sends only the domains still offered for binding - #1840
dawsontoth wants to merge 1 commit into
Conversation
The Bind selection was page state that outlived its rows. A ticked domain that was then removed, or bound to another cluster, was still counted and sent, and central manager refused every later Bind (404 not found, 400 already associated) until the page was left. Bind now counts and sends the selection intersected with the list's bindable rows, using the same predicate that renders the checkbox. A removal takes its row out of the cached list as soon as it succeeds, so the gap before the refetch lands cannot send it, and Bind waits while a removal is in flight, including a click queued before the controls disabled. Remove likewise refuses a click queued behind a Bind that is still sending its domain. Bind reads the list from the query client at click time, since a removal reaches the button a render later. Closes #1800 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the domain binding and removal flow in the cluster domains management feature by handling race conditions between concurrent bind and delete actions. It introduces state checks to ensure only bindable domains are processed, disables controls while mutations are in flight, and optimistically updates the React Query cache upon domain deletion. The review feedback correctly points out that the global queryClient is still being used for query invalidation in Management.tsx and tableDefinition.tsx instead of the newly introduced activeQueryClient hook, which could lead to issues in test environments or custom configurations.
| }, | ||
| }); | ||
| }, [cluster, selectedDomainIds, setDomainIds]); | ||
| }, [activeQueryClient, cluster, organizationId, selectedDomainIds, setDomainIds]); |
There was a problem hiding this comment.
In onBindClick, queryClient.invalidateQueries is called on line 128 using the global queryClient import instead of the context-provided activeQueryClient. This can cause query invalidation to fail or be ignored in environments with custom query clients (such as during testing). Please update it to use activeQueryClient.invalidateQueries on line 128.
| }, | ||
| }); | ||
| }, [deleteDomain, cluster.organizationId]); | ||
| }, [activeQueryClient, deleteDomain, cluster.organizationId]); |
There was a problem hiding this comment.
In deleteDomainById, queryClient.invalidateQueries is called on line 54 using the global queryClient import instead of the context-provided activeQueryClient. This can cause query invalidation to fail or be ignored in environments with custom query clients (such as during testing). Please update it to use activeQueryClient.invalidateQueries on line 54. Additionally, consider updating unbindDomainById (on line 68) to also use activeQueryClient and adding activeQueryClient to its dependency array.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1 participant
Closes #1800
⊙ Problem
On a cluster's Domains page, the domains ticked for binding are page state that outlived their rows. Tick a domain, then click Remove Domain on it (enabled for any unbound domain, ticked or not): the row disappears, but Bind 1 Domain stays, and clicking it sends the deleted id. Central manager answers
404 Domain <id> not found, and since the selection only clears when a bind succeeds, every later Bind on that page fails the same way until the user leaves it. A ticked domain that gets bound to another cluster meanwhile fails the same way with a 400 (#1800).💡 Solution
Bind now counts and sends only the ticked domains that the current list still offers for binding: the selection intersected with the rows
canBindDomainaccepts, which is the same predicate that renders the checkbox. A domain that was removed, or bound elsewhere, drops out of the count and the request on its own, with no extra state to keep in sync.Three gaps around a removal are closed as well:
⚖️ Alternatives
🔧 Changes
Management.tsx: the Bind button's count and visibility come fromdomainIdsToBind; the click handler refuses while a delete is in flight and recomputes the ids from the query client, and Bind is disabled meanwhile.canBindDomain.ts(new): the bindable-row predicate, now shared by the checkbox and Bind, andbindableDomainIds, the selection filtered by it.tableDefinition.tsx: the checkbox usescanBindDomain; Remove refuses while a domain change is in flight; the delete call passes the organization id the cache write needs.deleteDomain.ts: the mutation takes a mutation key, removes the deleted row from the cached list on success, and exportsisDeletingDomain/useIsDeletingDomain.setDomainIdsOnCluster.ts: exportsisSettingDomainIds, the synchronous checkmutateWhenIdlealready made, so Remove can share it.src/features/cluster/DESIGN.md,DESIGN.md: a note on the Bind selection and the index line.✅ Verification
End-to-end route: not observable end-to-end here — it needs a cluster, verified custom domains and a central manager, and a Bind changes real infrastructure. Verified at the request boundary instead:
Management.test.tsxrenders the realDomainsManagementand table against a mockedapiClientand asserts the serialized PUT body.Management.test.tsx, new "a ticked domain the list no longer offers" block:canBindDomain.test.ts(new): verified + unbound is offered; pending, bound (to any cluster) and self-managed are not;bindableDomainIdskeeps tick order and offers nothing before the list loads.Management.test.tsxtests fail against theorigin/claude/1630-challenge-cert-poll-backoffsources with the bug signatures (the label staysBind 2 Domainsafter a removal; the button stays for a domain bound elsewhere; Bind stays enabled during a removal; the queued Remove sends its DELETE; the render-lag click sends both ids). Each guard was mutation-checked: without the cache write, the intersection, the disabled state, either queued-click guard or the click-time read, its test goes red.npx vitest run397 files / 3,688 passed (exit 0),npx tsc -bexit 0,npx oxlint --format stylish .exit 0,npx dprint checkexit 0, and again by the pre-commit hook.Pre-push review: three full rounds (codex graded, gemini). The Harper-domain adjudicator failed on auth every round and the Cursor legs failed on the 1Password SSH agent, so no round converged. Round 1 found the render-lag click (fixed: Bind reads the list from the query client); round 2 found the reverse race, a Remove queued behind a Bind (fixed: Remove shares
isSettingDomainIds). Round 3 returned COMMENTS with no defects. I rejected:useDeleteDomaincallers break on the neworganizationId" (gemini):tableDefinition.tsxis the only caller, andDeleteDomainSchemais only a type;mutateWhenIdlealready refuses a second domain change while one is in flight;🤖 Generated with Claude Code (Claude Opus 5.5); posted via @dawsontoth.
Related PRs: #1801 overlaps (this PR stacks on it), #1802 overlaps (this PR's base)
Complexity: easy
Review-Coverage: authored=claude; ran=gemini,codex; blocked=cursor-composer(no-receipt),domain(auth),cursor-muse(no-receipt); declined=cursor-grok,cursor-kimi; rounds=3; full=1 @ 00c3b5d
Review-Attention: read ~5m (raised: degraded review) @ 00c3b5d