Skip to content

feat(site): add organizationId prop to OrganizationAutocomplete #23906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
johnstcn wants to merge 6 commits into from
Closed

feat(site): add organizationId prop to OrganizationAutocomplete #23906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c879fed
feat(site): add organizationId prop to OrganizationAutocomplete
johnstcn Apr 1, 2026
e0c6e5a
fix(site): exorcise useEffect demon from OrganizationAutocomplete
johnstcn Apr 1, 2026
5933b94
fix(site): address self-review findings for OrganizationAutocomplete
johnstcn Apr 1, 2026
6f2ef59
fix(site): add display-revert assertion and JSDoc warning for auth ed…
johnstcn Apr 1, 2026
778b9bf
fix(site): link follow-up issue in organizationId JSDoc
johnstcn Apr 1, 2026
03191e0
fix(site): use ID comparison in auto-select to survive react-query re…
johnstcn Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 143 additions & 1 deletion site/src/components/OrganizationAutocomplete/OrganizationAutocomplete.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import type { Meta, StoryObj } from "@storybook/react-vite";
import { action } from "storybook/actions";
import { userEvent, within } from "storybook/test";
import { expect, fn, screen, userEvent, waitFor, within } from "storybook/test";
import type { Organization } from "#/api/typesGenerated";
import {
MockOrganization,
MockOrganization2,
MockUserOwner,
} from "#/testHelpers/entities";
import { OrganizationAutocomplete } from "./OrganizationAutocomplete";

type OnChangeFn = (org: Organization | null) => void;

const meta: Meta<typeof OrganizationAutocomplete> = {
title: "components/OrganizationAutocomplete",
component: OrganizationAutocomplete,
Expand Down Expand Up @@ -53,3 +56,142 @@ export const OneOrg: Story = {
],
},
};

export const PreselectedOrg: Story = {
args: {
organizationId: MockOrganization2.id,
onChange: fn<OnChangeFn>(),
},
parameters: {
showOrganizations: true,
user: MockUserOwner,
features: ["multiple_organizations"],
permissions: { viewDeploymentConfig: true },
queries: [
{
key: ["organizations"],
data: [MockOrganization, MockOrganization2],
},
],
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const button = canvas.getByRole("button");
await waitFor(() =>
expect(button).toHaveTextContent(MockOrganization2.display_name),
);
const onChangeSpy = args.onChange as ReturnType<typeof fn<OnChangeFn>>;
expect(onChangeSpy).not.toHaveBeenCalled();
},
};

export const PreselectedOrgNotFound: Story = {
args: {
organizationId: "nonexistent-id",
},
parameters: {
showOrganizations: true,
user: MockUserOwner,
features: ["multiple_organizations"],
permissions: { viewDeploymentConfig: true },
queries: [
{
key: ["organizations"],
data: [MockOrganization, MockOrganization2],
},
],
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const button = canvas.getByRole("button");
// Open the dropdown to verify data has loaded.
await userEvent.click(button);
await waitFor(() =>
expect(
screen.getByText(MockOrganization.display_name),
).toBeInTheDocument(),
);
// Close and verify the button still shows placeholder
// (the org ID doesn't match any loaded option).
await userEvent.keyboard("{Escape}");
await waitFor(() =>
expect(button).toHaveTextContent("Select an organization"),
);
},
};

export const PreselectedOrgUserSelects: Story = {
args: {
organizationId: MockOrganization2.id,
onChange: fn<OnChangeFn>(),
},
parameters: {
showOrganizations: true,
user: MockUserOwner,
features: ["multiple_organizations"],
permissions: { viewDeploymentConfig: true },
queries: [
{
key: ["organizations"],
data: [MockOrganization, MockOrganization2],
},
],
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const button = canvas.getByRole("button");
// Wait for the preselected org to appear.
await waitFor(() =>
expect(button).toHaveTextContent(MockOrganization2.display_name),
);
const onChangeSpy = args.onChange as ReturnType<typeof fn<OnChangeFn>>;
onChangeSpy.mockClear();
// Open dropdown and select a different org.
await userEvent.click(button);
await waitFor(() =>
expect(
screen.getByText(MockOrganization.display_name),
).toBeInTheDocument(),
);
await userEvent.click(screen.getByText(MockOrganization.display_name));
// Verify onChange was called with the new org.
await waitFor(() =>
expect(onChangeSpy).toHaveBeenCalledWith(
expect.objectContaining({ id: MockOrganization.id }),
),
);
// Button should still show the prop-controlled value since
// the parent hasn't updated organizationId.
await waitFor(() =>
expect(button).toHaveTextContent(MockOrganization2.display_name),
);
},
};

export const OneOrgWithControlledId: Story = {
args: {
organizationId: MockOrganization.id,
onChange: fn<OnChangeFn>(),
},
parameters: {
showOrganizations: true,
user: MockUserOwner,
features: ["multiple_organizations"],
permissions: { viewDeploymentConfig: true },
queries: [
{
key: ["organizations"],
data: [MockOrganization],
},
],
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const button = canvas.getByRole("button");
await waitFor(() =>
expect(button).toHaveTextContent(MockOrganization.display_name),
);
const onChangeSpy = args.onChange as ReturnType<typeof fn<OnChangeFn>>;
expect(onChangeSpy).not.toHaveBeenCalled();
},
};
57 changes: 46 additions & 11 deletions site/src/components/OrganizationAutocomplete/OrganizationAutocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,28 @@ type OrganizationAutocompleteProps = {
id?: string;
required?: boolean;
check?: AuthorizationCheck;
/**
* Pre-selects an organization by ID. When provided, the

@mafredri mafredri Apr 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

🤖

* displayed selection is derived from this prop. The parent
* is responsible for updating this prop in response to user
* selections via onChange.
*
* When combined with `check`, the ID must reference an org
* the user is authorized for — if the org fails the check,

@mafredri mafredri Apr 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

🤖

* the button silently shows placeholder text without firing
* onChange(null). The follow-up full-object refactor
* (https://github.com/coder/internal/issues/1440) will
* address this.
*/
organizationId?: string;
Comment thread
johnstcn marked this conversation as resolved.
};

export const OrganizationAutocomplete: FC<OrganizationAutocompleteProps> = ({
onChange,
id,
required,
check,
organizationId,
}) => {
const [open, setOpen] = useState(false);
const [selected, setSelected] = useState<Organization | null>(null);
Expand Down Expand Up @@ -66,18 +81,31 @@ export const OrganizationAutocomplete: FC<OrganizationAutocompleteProps> = ({
: [];
}

// Unfortunate: this useEffect sets a default org value
// if only one is available and is necessary as the autocomplete loads
// its own data. Until we refactor, proceed cautiously!
// In controlled mode, derive the displayed selection from the
// prop so we never need to sync prop → state via an effect.
// Note: when `check` is also provided, options may be empty
// 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

@mafredri mafredri Apr 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

🤖

@mafredri mafredri Apr 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

🤖

? (options.find((o) => o.id === organizationId) ?? null)
: selected;

// Auto-select when only one option exists. Only active in
// uncontrolled mode — when the parent controls the value via
// organizationId it is responsible for the initial selection.
useEffect(() => {
if (organizationId) {
return;
}
const org = options[0];
if (options.length !== 1 || org === selected) {
if (options.length !== 1 || org.id === selected?.id) {
return;
}

setSelected(org);
onChange(org);
}, [options, selected, onChange]);
}, [options, selected, onChange, organizationId]);

return (
<Popover open={open} onOpenChange={setOpen}>
Expand All @@ -90,14 +118,16 @@ export const OrganizationAutocomplete: FC<OrganizationAutocompleteProps> = ({
data-testid="organization-autocomplete"
className="w-full justify-start gap-2 font-normal"
>
{selected ? (
{displayedSelection ? (
<>
<Avatar
size="sm"
src={selected.icon}
fallback={selected.display_name}
src={displayedSelection.icon}
fallback={displayedSelection.display_name}
/>
<span className="truncate">{selected.display_name}</span>
<span className="truncate">
{displayedSelection.display_name}
</span>
</>
) : (
<span className="text-content-secondary">
Expand All @@ -121,7 +151,12 @@ export const OrganizationAutocomplete: FC<OrganizationAutocompleteProps> = ({
key={org.id}
value={`${org.display_name} ${org.name}`}
onSelect={() => {
setSelected(org);
// Only update internal state in uncontrolled mode.
// In controlled mode, displayedSelection is derived
// from the organizationId prop.
if (!organizationId) {
setSelected(org);
}
onChange(org);
setOpen(false);
}}
Expand All @@ -134,7 +169,7 @@ export const OrganizationAutocomplete: FC<OrganizationAutocompleteProps> = ({
<span className="truncate">
{org.display_name || org.name}
</span>
{selected?.id === org.id && (
{displayedSelection?.id === org.id && (
<Check className="ml-auto size-icon-sm shrink-0" />
)}
</CommandItem>
Expand Down
Loading