onboarding: data-plane picker in the signup form - #2045
SeanWhelan wants to merge 13 commits into
Conversation
⚪ Code HealthNo change to the dead-code surface. 48 Unused files
66 Unused exports
29 Unused exported types
14 Unused exported enum members
5 Unused dependencies
3 Unused devDependencies
|
c1f79e9 to
7020609
Compare
…onboarding The authenticated dataPlanes query returns nothing for a brand-new signup (no grants yet), so the onboarding plane picker needs the control-plane API's unauthenticated publicDataPlanes query instead.
…rd claims Adds a plane picker between the organization name field and the survey, backed by the unauthenticated publicDataPlanes query (a brand-new signup has no grants yet, so the authenticated dataPlanes query returns nothing). Auto-preselects aws-us-east-1-c1 (falling back to the first available plane) so submitting without touching the picker still records an explicit, valid choice; fails safe to no picker + an omitted claim if the plane list can't be loaded, letting the backend apply its own default. generateUserClaim now forwards requestedDataPlane only when set, never as an explicit null, so older backends that don't know the field are unaffected. Also registers PublicDataPlane in the urql cache's keys config (alongside the existing DataPlane entry) to silence a normalization warning for the new unkeyed type. Verified end-to-end against a real local stack: picker lists the seeded public plane, auto-selects it, and a completed registration produced a storage_mappings row with the picked plane in data_planes.
- Move PublicDataPlaneNode + a named toPublicDataPlaneNode transform into src/api/gql/dataPlanes.ts, alongside the existing DataPlaneNode / toDataPlaneNode pair the sibling useDataPlanes() hook uses. The hook previously defined its own type and inlined an identity transform, diverging from where this codebase already models data-plane nodes. - Destructure betaOnboard's generateUserClaim args by name instead of indexing (args[0], args[1], args[2]) — same positional-args signature (shared across all directive types), just clearer at the read site. - Rename DataPlaneSelector's PREFERRED_DEFAULT to DEFAULT_PUBLIC_DATA_PLANE, matching the backend's constant of the same meaning and making the value's type (a plane name) obvious from the name. No behavior change: tsc and lint both pass clean.
The Organization Name field overrides the 2px theme default to borderRadius: 3 (6px). The picker sat right below it inheriting the theme default, so the two adjacent inputs had visibly different corners. Apply the same override.
- Sort options by cloudProvider then region: groupBy only groups correctly when the list is ordered by group, which previously worked only because plane names embed the provider. - Extract the region/name label used by both getOptionLabel and renderOption so the two can't drift. - Drop isOptionEqualToValue; value is always drawn from options, so MUI's default reference comparison already matches. - Hoist the input sx to a module const, and reuse hasLength for the claim's non-empty check.
CI's Check Quality runs prettier, which I hadn't run locally. Formatting only, no functional change.
The helper text said 'processed and stored'. That's true for a tenant left on the trial bucket, but we encourage customers to bring their own storage, in which case the plane choice doesn't decide where their data lives. Say only what the choice always controls.
f3f213e to
47fbe96
Compare
47fbe96 to
fe27956
Compare
adrian-estuary
left a comment
adrian-estuary
left a comment
There was a problem hiding this comment.
Overall looks good. Just a couple of points about not using Zustand and react-intl.
| export const useOnboardingStore_requestedDataPlane = () => { | ||
| return useLocalZustandStore< | ||
| OnboardingState, | ||
| OnboardingState['requestedDataPlane'] | ||
| >(OnboardingStoreNames.GENERAL, (state) => state.requestedDataPlane); | ||
| }; |
There was a problem hiding this comment.
@SeanWhelan - I think the plan is to migrate away from Zustand. I would recommend just using react's useState to manage the selected data plane in the onboarding form.
cc: @GregorShear - Correct me if I'm wrong here. 🙏
There was a problem hiding this comment.
Generally yes, Zustand is overkill in a lot of places and should be removed. It's still appropriate when it shares state for some complex flow (setting up a new capture for example?) or a deep subtree. Not the case here.
(and in some future PR we could remove zustand entirely from Onboarding)
| 'tenant.dataPlane.label': `Data Plane`, | ||
| 'tenant.dataPlane.helper': `Where your data is processed. Pick the region closest to your data sources.`, |
There was a problem hiding this comment.
We're also migrating away from react-intl. So better to just put the strings in the component.
cc: @GregorShear
| {intl.formatMessage({ id: 'tenant.dataPlane.label' })} | ||
| </FormLabel> | ||
|
|
||
| <Autocomplete |
There was a problem hiding this comment.
Should add disableClearable to hide the x button to clear the selection. Seems like we don't want this to be unset. Also, the useEffect above will just trigger, setting it back to the default anyway.
| const optionLabel = (option: PublicDataPlaneNode) => | ||
| `${option.region} (${option.name})`; |
There was a problem hiding this comment.
Is it required to show the full plane name to the user? Seems a bit noisy. Perhaps just the region and suffix would do?
So us-east-1 - c1 (or something like that) versus us-east-1 (ops/dp/public/aws-us-east-1-c1).
| if (error || (!loading && options.length === 0)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Should failing API calls log somewhere? What do we use for frontend logging?
There was a problem hiding this comment.
Oh good call
postHog.capture('Entity:Action', {
someData,
});
| ); | ||
| } | ||
|
|
||
| export default DataPlaneSelector; |
There was a problem hiding this comment.
Pretty minor but would love help establishing a new pattern where we favor named exports
| export default DataPlaneSelector; |
| const optionLabel = (option: PublicDataPlaneNode) => | ||
| `${option.region} (${option.name})`; | ||
|
|
||
| function DataPlaneSelector() { |
There was a problem hiding this comment.
| function DataPlaneSelector() { | |
| export function DataPlaneSelector() { |
Keep the selected plane in BetaOnboard's local state instead of the onboarding store, drop react-intl for plain strings, disable clearing, shorten option labels to region + cluster, log query failures to PostHog, and switch to a named export.
|
Thanks both, all addressed in a1cde3e, 90c99d6, eae120d and 4681232:
I also trimmed the code comments down to the non-obvious ones in line with the feedback on the Bindings PR. |
Preselecting a specific plane name meant a UI PR every time a region's plane was succeeded (c1 -> c2 -> ...). publicDataPlanes only returns open planes, so defaulting to a region and taking its newest open plane follows a succession automatically: open the new plane, close the old one.
adrian-estuary
left a comment
adrian-estuary
left a comment
There was a problem hiding this comment.
LGTM! Thanks for addressing the feedback! 🙏
4 participants
Issues
estuary/sre#29 (Phase 2 — tracked in a roadmap doc, not an estuary/ui issue)
Changes
sre#29
DataPlaneSelectorto the signup form (BetaOnboard.tsx), between the organization name field and the survey. It's backed by the existing unauthenticatedpublicDataPlanesquery, which this PR consumes for the first time — the authenticateddataPlanesquery returns nothing for a brand-new signup, since there are no grants yet.publicDataPlanesonly returns open planes, so replacing a plane (open c2, close c1) needs no UI change. While c1 and c2 are both open, the picker lists both and preselects c2.requestedDataPlanerather than sending it as an explicit null, so the backend falls back to its own default instead of the signup form breaking. The failure is sent to PostHog as anOnboarding:DataPlanesevent, so we can see when it happens.betaOnboarddirective'sgenerateUserClaimto forwardrequestedDataPlaneonly when it's set, so backends that don't know the field yet aren't affected.usePublicDataPlanes()hook andPublicDataPlaneNodetype, following the existinguseDataPlanes()/DataPlaneNodepattern already in the codebase.useStateinBetaOnboardand passed down to the picker, rather than added to the onboarding Zustand store.Options are grouped by cloud provider and labelled with the region plus the cluster suffix (e.g.
us-east-1 c1). The suffix is what tells apart two public planes in the same region (already the case for GCP). If a name doesn't parse, the label falls back to the full catalog name. The full name is always what gets submitted. The picker can't be cleared, since a plane is always preselected.This depends on estuary/flow#3318 merging and deploying first. The agent rejects unknown claim fields, so if this ships before that one is live, every signup fails with
invalidClaims. Merging flow isn't sufficient on its own — an old agent still running will reject the claim.Tests
Manually tested
I ran a full signup through the real onboarding form against a local flow stack. The picker listed the seeded public planes grouped by cloud provider, auto-selected the platform default, and a completed registration produced the correct
storage_mappingsrow with the picked plane recorded as the tenant's default.I repeated this against a synthetic AWS-shaped plane with
COLOCATED_TRIAL_BUCKETSenabled on the backend. The resulting storage mapping showed the derived S3 bucket, confirming the picker's choice flows through correctly end to end.I also checked what happens against a real, older backend that predates this feature: the query fails with a clear GraphQL error, the picker just doesn't render, and the rest of the form stays fully usable. No crash.
Finally, with the companion PR's migration applied, a data plane marked
closeddisappears from the picker's options — the list and the backend agree on what's selectable.Automated tests
tsc --noEmitandnpm run lintboth pass clean. I didn't add new unit tests since this is UI wiring around an already-tested backend contract; flagging that for reviewer input rather than assuming it's fine.Playwright tests ran locally
Screenshots