feat(site): add organizationId prop to OrganizationAutocomplete#23906
feat(site): add organizationId prop to OrganizationAutocomplete#23906johnstcn wants to merge 6 commits into
Conversation
The component was fully uncontrolled — it managed its own selected state internally. Callers that pre-selected an org (like AgentCreateForm for single-org users) had no way to display that selection in the picker, resulting in a visual desync where the button shows 'Select an organization…' while the form already knows the answer. Adding organizationId as an optional controlled prop lets the parent set which org is visually selected. When provided, the component syncs from options on load. The existing auto-select-on-single-option behavior is preserved when the prop is omitted.
Derive the displayed selection during render instead of syncing via useEffect. This eliminates the selection-revert loop, the missing onChange notification, and the unnecessary extra render cycle. - Replace first useEffect with render-time derivation - Guard auto-select effect to only run in uncontrolled mode - Handle empty string organizationId as 'not provided' - Add three new Storybook stories for controlled pre-selection
- Guard setSelected in onSelect to prevent shadow state in controlled mode - Fix PreselectedOrgNotFound test to prove data loaded before asserting - Add PreselectedOrgUserSelects story for controlled-mode interaction - Extract OnChangeFn type alias to deduplicate spy casts - Document placeholder flash edge case when check + organizationId combine
…ge case - Assert button reverts to controlled value after user selection in PreselectedOrgUserSelects story - Warn in JSDoc that organizationId must reference an authorized org when combined with check prop
There was a problem hiding this comment.
Pull request overview
Adds a controlled pre-selection capability to OrganizationAutocomplete so callers can provide an organizationId and have the displayed selection derived at render time, avoiding prior prop→state synchronization pitfalls.
Changes:
- Introduce an
organizationId?: stringprop and derive the displayed selection from it during render. - Guard the “auto-select single org” effect so it only runs in uncontrolled mode.
- Add Storybook coverage for controlled preselection, unmatched ID fallback, and single-org suppression in controlled mode.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| site/src/components/OrganizationAutocomplete/OrganizationAutocomplete.tsx | Adds organizationId prop, render-time selection derivation, and adjusts auto-select/onSelect behavior for controlled vs uncontrolled usage. |
| site/src/components/OrganizationAutocomplete/OrganizationAutocomplete.stories.tsx | Adds Storybook stories validating controlled preselection behavior and ensuring no unintended onChange calls. |
Comments suppressed due to low confidence (1)
site/src/components/OrganizationAutocomplete/OrganizationAutocomplete.tsx:104
- The auto-select effect uses
org === selected(reference equality) to decide whether to fire. If the organizations query refetches and returns a new object instance for the same org (common with react-query), this will re-triggersetSelected+onChange(org)even though the selected org ID hasn’t changed. Prefer comparing stable identity (e.g.,selected?.id === org.id) soonChangeisn’t fired repeatedly on background refetches/window-focus refetches when there is only one org.
const org = options[0];
if (options.length !== 1 || org === selected) {
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fetches Reference equality (org === selected) breaks on background refetch since react-query returns new object instances. Compare by ID instead to prevent spurious onChange calls for single-org users.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Solid work. The render-time derivation replacing useEffect prop-to-state sync is the correct React pattern, and the controlled/uncontrolled guards are clean. The ID-equality fix in auto-select (org.id === selected?.id) prevents duplicate onChange fires on react-query refetches. Existing callers are unaffected. Stories are genuine: PreselectedOrgUserSelects is the standout, proving display-revert and controlled behavior.
Severity summary: 0 P0, 0 P1, 1 P2, 3 P3, 5 notes.
The top finding (P2) is a convergent result from 5 of 6 reviewers: when check filters out the controlled org, parent and component silently disagree. No story pins this documented behavior. Meruem's sharpest observation: the deferred fix (coder/internal#1440, full-object refactor) changes the input type but does not add a notification mechanism, so the deferral doesn't fix this specific problem.
P3 (file-level, OrganizationAutocomplete.stories.tsx) Empty string organizationId is claimed as handled (PR changelog: "Handle empty string organizationId as 'not provided' (falsy check)") but has no test. The code relies on JavaScript truthiness at three sites (lines 90, 98, 157 of the component). This works today, but it's one organizationId !== undefined refactor away from breaking all three guards simultaneously. A one-story test locks it down: pass organizationId: "" with a single-org query mock, assert auto-select fires onChange (proving uncontrolled fallthrough). (Bisky)
Two additional inline findings on comment accuracy and convention. Plus notes on displayedSelection naming, options reference instability in useEffect deps, and PreselectedOrgNotFound using action() instead of fn() (cannot spy on onChange).
Mafuuu on the PreselectedOrgNotFound story: "Falls through to the meta default action('Selected organization'), which cannot be spied on. This means the story cannot assert that onChange was NOT called."
🤖 This review was automatically generated with Coder Agents.
| // until permissions load, causing a brief placeholder flash. | ||
| // The follow-up refactor (passing the full Organization object | ||
| // instead of just an ID) will eliminate this. | ||
| const displayedSelection = organizationId |
There was a problem hiding this comment.
P2 When check filters out the controlled org, parent and component silently disagree about the selected organization.
Trace: parent passes organizationId = "org-2-id" with a check prop. Permissions load. Org-2 fails the check and is excluded from options (line 80). displayedSelection becomes null here. Button shows placeholder. onChange(null) never fires. The parent continues to believe org-2 is selected.
If the consuming form (#23827) submits with the uncorrected organizationId, it sends an ID the user isn't authorized for. Backend rejects it. The user never explicitly chose anything, so they have no idea what went wrong.
The JSDoc (lines 35-39) documents this and defers to coder/internal#1440 ("full-object refactor"). That refactor changes the input type but doesn't add a notification mechanism. If the refactored component receives a full Organization object that fails the check, it still needs to tell the parent. The deferral points at a fix that does not fix this specific problem.
No story covers check + organizationId together. The PreselectedOrgNotFound story tests a nonexistent ID (different failure mode). The check interaction is structurally distinct because it depends on an async permission query filtering options after they load.
Suggestion: add a story with organizationId pointing to an org that exists in the query data but is filtered out by check. Assert placeholder text after both queries resolve. Consider whether onChange(null) should fire once options settle. (Meruem P2, Nami P2, Hisoka P2, Mafuuu P3, Bisky P3)
🤖
| required?: boolean; | ||
| check?: AuthorizationCheck; | ||
| /** | ||
| * Pre-selects an organization by ID. When provided, the |
There was a problem hiding this comment.
P3 JSDoc opens with "Pre-selects" but the prop does continuous control, not one-time initialization.
A caller reading "Pre-selects an organization by ID" will think this is a defaultValue that can be overridden by user interaction. The PreselectedOrgUserSelects story proves otherwise: the display reverts to the prop value after user selection. "Controls the selected organization by ID" matches the actual semantics. (Gon)
🤖
| * selections via onChange. | ||
| * | ||
| * When combined with `check`, the ID must reference an org | ||
| * the user is authorized for — if the org fails the check, |
There was a problem hiding this comment.
P3 Em-dash character (U+2014) in new comments. Also on line 95. Project convention prohibits em-dashes. Replace with a comma, period, or restructure the sentence.
🤖
| // until permissions load, causing a brief placeholder flash. | ||
| // The follow-up refactor (passing the full Organization object | ||
| // instead of just an ID) will eliminate this. | ||
| const displayedSelection = organizationId |
There was a problem hiding this comment.
Note displayedSelection is a name invented for this component with no precedent in the codebase. The sibling Autocomplete.tsx uses displayValue for the same concept. Other components use value. Consider selection for consistency: it contrasts with the internal state selected without inventing vocabulary. (Gon)
PS. The comment on line 154 references displayedSelection by name, so if renamed, update the comment too.
🤖
|
I decided that it wasn't worth working around the hybrid state issues and elected instead to proceed with migration to a fully-controlled component instead (#24211). |
Enabling change for #23827 (organization-scoped chats), which reuses
OrganizationAutocompletein the chat creation form. This PR adds the controlled pre-selection capability so callers can pre-fill the org picker instead of requiring the user to select each time.setSelectedinonSelectto prevent shadow state in controlled modeorganizationIdas "not provided" (falsy check)Follow-up for full controlled refactor: coder/internal#1440
Implementation plan & decision log
Decisions:
MultiSelectComboboxprecedent.onChangeon controlled initialization — parent already knows the ID, parent handles side-effects.Deep review findings addressed:
selected?.idin deps) → eliminated by removing the useEffectonChangein controlled sync → eliminated by removing the useEffect"" === undefinedistrue) → falsy guard catches itPreselectedOrgNotFoundvacuously true → fixed to prove data loaded firstsetSelectedin controlled mode → guarded behindif (!organizationId)PreselectedOrgUserSelectsadded with display-revert assertioncheck+organizationId→ JSDoc warning added