fix: sector select never hydrates the saved value in the completion pane - #893
Merged
Merged
Conversation
…nge during hydration
The sector select never showed a saved value on load: Radix's Select
mirrors its controlled value onto a hidden native <select> for native
form semantics, and re-syncs that mirror with a dispatched native
"change" event whenever the controlled value changes -- including the
mount-time change from "" to a hydrated value. When that sync runs
before the mirror's own matching <option> has committed, the native
element silently falls back to "", and the bubbled event fires
onValueChange("") -- overwriting the value reset() just hydrated in.
There is no real SelectItem for an empty value in this field (no
"clear" affordance), so an empty-string callback can only be that
phantom event, never a genuine user selection. Ignoring it fixes
hydration for both canonical and legacy sector values.
Confirmed by an isolated repro against the real (unmocked) Radix
Select: the phantom revert only occurs when the Select is inside a
<form> element (Radix's isFormControl gate), which is why it wasn't
caught by the suite's Radix mock -- the mock doesn't model the native
mirror at all.
… for sector guard Addresses nanaki/vincent review notes on 99463a9: verify a genuine Radix Select user selection still reaches field.onChange (the fix's premise), add an explicit regression test that a phantom empty onValueChange never clears an already-hydrated value, and document the "" sentinel at the sectors export so a future clear affordance finds the guard.
…re lifetime
Real-browser evidence (Playwright, both against a rebuilt local docker
stack and a host-run production build) showed the previous fix's guard
was sound but incomplete: the Select's `value` prop still flipped from
`undefined` (while the case-information record was loading) to a
string (once hydrated), which React treats as an uncontrolled-to-
controlled switch on Radix's hidden native <select> mirror and warns
about — a mount-time race the mirror can lose, silently dropping the
hydrated value back to empty.
Pass `value={currentValue}` (always a string, including "") instead of
`currentValue || undefined`. An empty string matches no SelectItem, so
Radix still shows the placeholder pre-hydration — but the prop's type
never changes, so there is no controlled/uncontrolled switch to race.
Adds a regression test (run first in its file, since React dedupes the
warning process-wide after its first occurrence) asserting zero
controlled/uncontrolled console warnings across the loading-to-
hydrated transition.
Root cause of the original failed verification: the docker dev
container's app_next volume was serving a production build compiled
before the previous fix's commits landed, silently masking real
behaviour. Confirmed clean via the diag spec against a freshly rebuilt
container.
…est comment Nanaki (review of d9e454e) traced the controlled/uncontrolled warning this test captures to Radix's react-use-controllable-state hook, which tracks state in a per-instance useRef with no module-scoped dedup — it fires fresh on every mount, unlike React DOM's own didWarnUncontrolledToControlled for native elements. Verified by moving the test to second position: it still fails correctly. Comment no longer claims running first is load-bearing.
Cid's throwaway real-browser diagnostics for the sector-select hydration investigation; both marked delete-after-use. The lasting regression coverage lives in the component test suite.
Fallow combined reportGitHub PR summary, scope: project Important Quality gates need attention. Found 3 findings. Checks
Top fixes
Generated by fallow. |
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
1 participant
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.
Summary
The publish completion pane's sector select never displayed the saved sector on a fresh load: the closed trigger showed the placeholder and no option was selected, even though the value was correctly persisted and returned by the API. Found during the 2026-08-18 walkthrough of the PR #891 fixes.
Two client-side mechanisms, both fixed:
99463a92): Radix Select's hidden native<select>mirror can dispatch an empty-string change during mount-time re-sync, overwriting the freshly hydrated form value. The list has no empty-valued item, so an empty callback can only be that phantom event — it is now ignored.d9e454e3):value={currentValue || undefined}mounted the select uncontrolled while the record loaded, then flipped it controlled on hydration — a race that can silently drop the hydrated value back to empty. The select now always receives a string (""pre-hydration shows the placeholder), so it is controlled for its entire lifetime.Legacy free-text sector values (pre-dating the canonical list) still display, marked "(legacy value)", and remain replaceable.
Tests
case-information-section.sector-hydration.test.tsx: canonical and legacy hydration into the closed trigger, genuine-selection propagation, phantom-event never clears, and zero controlled/uncontrolled warnings across the loading-to-hydrated transition. Each verified to fail with the fixes reverted.slug-service.test.tsfile-level setup failure is the known pre-existingDATABASE_URLenvironment issue, reproduced identically on the base tree.Review
Implemented by the frontend specialist; QA and code review both approved the final state (including an adversarial revert-check of the regression tests and a structural-quality audit with no new findings attributable to this change). Post-review delta was comment-only plus removal of temporary diagnostic files.
Two local-dev infrastructure defects discovered during verification (stale-build-serving container entrypoint; build memory limit) are tracked separately and not addressed here.