Refine the connect modal's API-key credential UX (#1195)

- Merged credential field: the placement's lead + prefix is a fixed, non-editable addon inside the field, so it reads as the header value being built ("Authorization: Bearer <token>") and the separate preview line is dropped. autoComplete/ignore attrs stop the browser and password managers from offering to GENERATE a password here; the app's own 1Password picker still covers filling an existing secret. - Attached tabs: the auth-method tabs join the panel as one card (full-width rounded-top header, hairline divider) instead of a pill bar floating above a separate card. - Prefix trailing-space warning (amber): a non-empty prefix with no trailing space is sent joined to the value ("Bearer" + token -> "Bearertoken"). Advisory, not blocking.

Rhys Sullivan committed 4 months ago

Commit b1e73d21 parent 53bfa033 files · +193 −20Browse files

Unified · Split

3 files

e2e/selfhost/connect-modal-credential-ux.test.ts +86 −0
packages/react/src/components/add-account-modal.tsx +84 −19
packages/react/src/components/placement-editor.tsx +23 −1

e2e/selfhost/connect-modal-credential-ux.test.ts+86−0

@@ -0,0 +1,86 @@
	1	// Selfhost-only (browser): the connect modal's API-key credential UX.
	2	// - the credential field MERGES the placement's lead + prefix as an affix, so
	3	// it reads as the header value being built ("Authorization: Bearer ▏token");
	4	// - the "Add authentication method" editor offers placement presets;
	5	// - a prefix with no trailing space (sent joined to the value) warns, and the
	6	// warning clears once the space is restored.
	7	// Video is the artifact.
	8	import { randomBytes } from "node:crypto";
	9
	10	import { Effect } from "effect";
	11	import { composePluginApi } from "@executor-js/api/server";
	12	import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";
	13	import { IntegrationSlug } from "@executor-js/sdk/shared";
	14
	15	import { scenario } from "../src/scenario";
	16	import { Api, Browser, Target } from "../src/services";
	17
	18	const api = composePluginApi([openApiHttpPlugin()] as const);
	19
	20	const bearerSpec = (): string =>
	21	JSON.stringify({
	22	openapi: "3.0.3",
	23	info: { title: "Bearer Fixture", version: "1.0.0" },
	24	servers: [{ url: "https://api.bearerfix.test" }],
	25	security: [{ bearerAuth: [] }],
	26	components: { securitySchemes: { bearerAuth: { type: "http", scheme: "bearer" } } },
	27	paths: {
	28	"/ping": { get: { operationId: "ping", responses: { "200": { description: "ok" } } } },
	29	},
	30	});
	31
	32	scenario(
	33	"Connect modal · API key credential UX: merged affix, add-method, prefix warning",
	34	{},
	35	Effect.scoped(
	36	Effect.gen(function* () {
	37	const target = yield* Target;
	38	const { client: makeApiClient } = yield* Api;
	39	const browser = yield* Browser;
	40	const identity = yield* target.newIdentity();
	41	const apiClient = yield* makeApiClient(api, identity);
	42	const slug = `connect_ux_${randomBytes(4).toString("hex")}`;
	43
	44	yield* Effect.ensuring(
	45	Effect.gen(function* () {
	46	yield* apiClient.openapi.addSpec({
	47	payload: { spec: { kind: "blob", value: bearerSpec() }, slug },
	48	});
	49
	50	yield* browser.session(identity, async ({ page, step }) => {
	51	await step("Open the connect modal", async () => {
	52	await page.goto(`/integrations/${slug}?addAccount=1`, { waitUntil: "networkidle" });
	53	await page.getByRole("heading", { name: /Add connection/ }).waitFor();
	54	});
	55
	56	await step("The credential field merges the placement prefix", async () => {
	57	// The placement's lead + prefix renders as a non-editable affix
	58	// inside the field, so there is no separate preview line.
	59	await page.getByText("Authorization: Bearer").first().waitFor();
	60	});
	61
	62	await step("The add-method editor offers placement presets", async () => {
	63	await page.getByRole("button", { name: "Add authentication method" }).click();
	64	await page.getByRole("heading", { name: "Add authentication method" }).waitFor();
	65	await page.getByRole("button", { name: "Bearer header" }).waitFor();
	66	await page.getByRole("button", { name: "API key query" }).waitFor();
	67	});
	68
	69	await step("A prefix with no trailing space warns", async () => {
	70	await page.getByPlaceholder("Bearer ").first().fill("Bearer");
	71	await page.getByText("Prefix has no trailing space").waitFor();
	72	});
	73
	74	await step("Restoring the trailing space clears the warning", async () => {
	75	await page.getByPlaceholder("Bearer ").first().fill("Bearer ");
	76	await page.getByText("Prefix has no trailing space").waitFor({ state: "detached" });
	77	});
	78	});
	79	}),
	80	apiClient.openapi
	81	.removeSpec({ params: { slug: IntegrationSlug.make(slug) } })
	82	.pipe(Effect.ignore),
	83	);
	84	}),
	85	),
	86	);

packages/react/src/components/add-account-modal.tsx+84−19

   /** Force the per-input label even for a single input (env vars name their
    *  field rather than relying on a generic "value / token" placeholder). */
   readonly showLabels?: boolean;
+  /** Single-input only: the placement's lead + prefix (e.g. "Authorization:
+   *  Bearer "), rendered as a non-editable affix INSIDE the field so the input
+   *  reads as the header value being built. Replaces the separate preview line. */
+  readonly affix?: string | null;
   readonly values: Readonly<Record<string, string>>;
   readonly onChange: (values: Record<string, string>) => void;
 }) {
             <Input
               id={`credential-input-${input.variable}`}
               type="password"
-              autoComplete="new-password"
+              autoComplete="off"
+              data-1p-ignore
+              data-lpignore="true"
+              data-bwignore
+              data-form-type="other"
               placeholder={`paste ${input.label}`}
               value={props.values[input.variable] ?? ""}
               onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
           {labelled && (
             <Label className="font-mono text-xs text-muted-foreground">{input.label}</Label>
           )}
-          <Input
+          {props.affix ? (
-            type="password"
+            // Merged field: the placement's lead + prefix is a FIXED addon (its
-            autoComplete="new-password"
+            // own muted segment, divider, non-selectable), and the user types
-            placeholder={labelled ? "secret value" : "paste the value / token"}
+            // only the token after it — the field reads as the header value being
-            value={props.values[input.variable] ?? ""}
+            // built. Only the border reacts to focus (no full-field glow). The
-            onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
+            // autoComplete/ignore attrs stop the browser and password managers
-              props.onChange({
+            // from offering to GENERATE a password here; the app's own 1Password
-                ...props.values,
+            // picker covers filling an existing secret.
-                [input.variable]: e.target.value,
+            <div className="flex h-9 w-full min-w-0 items-stretch overflow-hidden rounded-md border border-input bg-transparent font-mono text-sm shadow-xs transition-colors focus-within:border-ring dark:bg-input/30">
-              })
+              <span className="flex shrink-0 select-none items-center whitespace-pre border-r border-input bg-muted/40 px-3 text-muted-foreground/70">
-            }
+                {props.affix}
-            className="font-mono"
+              </span>
-            data-ph-block
+              {/* oxlint-disable-next-line react/forbid-elements */}
-          />
+              <input
+                type="password"
+                autoComplete="off"
+                data-1p-ignore
+                data-lpignore="true"
+                data-bwignore
+                data-form-type="other"
+                placeholder="token"
+                value={props.values[input.variable] ?? ""}
+                onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
+                  props.onChange({ ...props.values, [input.variable]: e.target.value })
+                }
+                className="min-w-0 flex-1 bg-transparent px-3 outline-none placeholder:text-muted-foreground/60"
+                data-ph-block
+              />
+            </div>
+          ) : (
+            <Input
+              type="password"
+              autoComplete="off"
+              data-1p-ignore
+              data-lpignore="true"
+              data-bwignore
+              data-form-type="other"
+              placeholder={labelled ? "secret value" : "paste the value / token"}
+              value={props.values[input.variable] ?? ""}
+              onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
+                props.onChange({
+                  ...props.values,
+                  [input.variable]: e.target.value,
+                })
+              }
+              className="font-mono"
+              data-ph-block
+            />
+          )}
         </div>
       ))}
     </div>
   readonly inputs: readonly CredentialInput[];
   readonly singleInput: boolean;
   readonly showLabels?: boolean;
+  /** Single-input only: placement lead + prefix to merge into the field. */
+  readonly affix?: string | null;
   /** Allow an external provider (1Password) origin. Off for env methods: the
    *  wire's external `from` is keyed under the canonical `token` variable, but
    *  an env credential must be keyed under its env var name, so a 1Password
           inputs={props.inputs}
           singleInput={props.singleInput}
           showLabels={props.showLabels}
+          affix={props.affix}
           values={props.values}
           onChange={props.onValuesChange}
         />
   // registration entirely.
   const isCimd = isOAuth && method?.oauth?.supportsClientIdMetadataDocument === true;
   const cimdActive = isCimd;
+  // Single-input header/query methods: the placement's lead + prefix (e.g.
+  // "Authorization: Bearer ") merges INTO the credential field as a non-editable
+  // affix, so the input reads as the header value being built and the separate
+  // preview line is dropped. Env and multi-input methods keep the plain field.
+  const singleCredentialAffix = useMemo<string | null>(() => {
+    if (!method || !singleInput || isEnvMethod) return null;
+    const placement = method.placements.find((p) => p.carrier !== "env");
+    if (!placement) return null;
+    const lead =
+      placement.carrier === "header"
+        ? `${placement.name || "Authorization"}: `
+        : `?${placement.name || "api_key"}=`;
+    return `${lead}${placement.prefix ?? ""}`;
+  }, [method, singleInput, isEnvMethod]);
   // DCR-capable: the integration advertises dynamic registration (MCP oauth2),
   // OR carries a discovery URL we can probe at connect time. When DCR-capable
   // and not yet fallen back, we skip the app picker entirely (Option A).
                 <Tabs
                   value={methodId}
                   onValueChange={selectMethod}
-                  className="w-full min-w-0 max-w-full gap-3"
+                  className="w-full min-w-0 max-w-full gap-0"
                 >
-                  <TabsList className="flex h-10 w-fit min-w-0 max-w-full justify-start overflow-x-auto overflow-y-hidden p-1 [scrollbar-width:thin]">
+                  <TabsList className="flex h-10 w-full min-w-0 max-w-full justify-start overflow-x-auto overflow-y-hidden rounded-b-none rounded-t-md border border-b-0 border-border/60 bg-muted/30 p-1 [scrollbar-width:thin]">
                     <div className="flex w-max shrink-0 items-stretch gap-1">
                       {allMethods.map((m: AuthMethod) => (
                         <div
                         "mt-0 min-w-0 space-y-5",
                         // No-auth renders no fields, so skip the framed box that
                         // would otherwise show up as an empty bordered panel.
-                        isNoAuth ? null : "rounded-md border border-border/60 bg-muted/15 p-4",
+                        // Otherwise the panel attaches to the tab header above
+                        // (square top, rounded bottom).
+                        isNoAuth
+                          ? null
+                          : "rounded-b-md rounded-t-none border border-border/60 bg-muted/15 p-4",
                       )}
                     >
-                      {method?.placements && !isEnvMethod && singleInput
+                      {method?.placements && !isEnvMethod && singleInput && !singleCredentialAffix
                         ? (() => {
                             const shown = method.placements.filter((p) => p.carrier !== "env");
                             return shown.length > 0 ? (
                               inputs={credentialInputs}
                               singleInput={singleInput}
                               showLabels={isEnvMethod}
+                              affix={singleCredentialAffix}
                               allowExternalProvider={!isEnvMethod}
                               values={values}
                               onValuesChange={setValues}

packages/react/src/components/placement-editor.tsx+23−1

@@ -1,4 +1,4 @@
1	import { PlusIcon, XIcon } from "lucide-react";	1	import { PlusIcon, TriangleAlertIcon, XIcon } from "lucide-react";
2		2
3	import {	3	import {
4	emptyPlacement,	4	emptyPlacement,
@@ -47,6 +47,10 @@ export function PlacementEditor(props: { · 42 unmodified lines
47	const remove = (index: number): void =>	47	const remove = (index: number): void =>
48	onChange(placements.filter((_p: Placement, j: number) => j !== index));	48	onChange(placements.filter((_p: Placement, j: number) => j !== index));
49		49
		50	// A non-empty prefix with no trailing space is sent JOINED to the credential
		51	// (e.g. "Bearer" + token -> "Bearertoken"). Almost always a mistake: warn.
		52	const barePrefix = placements.some((p) => p.prefix.length > 0 && !p.prefix.endsWith(" "));
		53
50	return (	54	return (
51	<div className="flex flex-col gap-3">	55	<div className="flex flex-col gap-3">
52	<div className="flex flex-wrap gap-1.5">	56	<div className="flex flex-wrap gap-1.5">
@@ -147,6 +151,24 @@ export function PlacementEditor(props: { · 94 unmodified lines
147	) : null}	151	) : null}
148	</div>	152	</div>
149	))}	153	))}
		154	{barePrefix ? (
		155	<section className="grid grid-cols-[auto_1fr] gap-x-2.5 gap-y-1 rounded-md border border-l-[3px] border-amber-300/70 border-l-amber-500 bg-amber-50 px-3 py-2.5 text-[12px] leading-5 dark:border-amber-500/25 dark:border-l-amber-500/80 dark:bg-amber-500/10">
		156	<TriangleAlertIcon
		157	className="mt-0.5 size-4 shrink-0 text-amber-600 dark:text-amber-400"
		158	aria-hidden
		159	/>
		160	<div className="min-w-0 space-y-1">
		161	<p className="font-medium text-amber-900 dark:text-amber-100">
		162	Prefix has no trailing space
		163	</p>
		164	<p className="text-amber-900/80 dark:text-amber-100/80">
		165	It is sent joined to the value (<span className="font-mono">Bearer••••••</span>). Most
		166	APIs expect a space, like <span className="font-mono">"Bearer "</span>.
		167	</p>
		168	</div>
		169	</section>
		170	) : null}
		171
150	<Button	172	<Button
151	type="button"	173	type="button"
152	variant="outline"	174	variant="outline"

Refine the connect modal's API-key credential UX (#1195)

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code