Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
86 changes: 86 additions & 0 deletions e2e/selfhost/connect-modal-credential-ux.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Selfhost-only (browser): the connect modal's API-key credential UX.
// - the credential field MERGES the placement's lead + prefix as an affix, so
// it reads as the header value being built ("Authorization: Bearer ▏token");
// - the "Add authentication method" editor offers placement presets;
// - a prefix with no trailing space (sent joined to the value) warns, and the
// warning clears once the space is restored.
// Video is the artifact.
import { randomBytes } from "node:crypto";

import { Effect } from "effect";
import { composePluginApi } from "@executor-js/api/server";
import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";
import { IntegrationSlug } from "@executor-js/sdk/shared";

import { scenario } from "../src/scenario";
import { Api, Browser, Target } from "../src/services";

const api = composePluginApi([openApiHttpPlugin()] as const);

const bearerSpec = (): string =>
JSON.stringify({
openapi: "3.0.3",
info: { title: "Bearer Fixture", version: "1.0.0" },
servers: [{ url: "https://api.bearerfix.test" }],
security: [{ bearerAuth: [] }],
components: { securitySchemes: { bearerAuth: { type: "http", scheme: "bearer" } } },
paths: {
"/ping": { get: { operationId: "ping", responses: { "200": { description: "ok" } } } },
},
});

scenario(
"Connect modal · API key credential UX: merged affix, add-method, prefix warning",
{},
Effect.scoped(
Effect.gen(function* () {
const target = yield* Target;
const { client: makeApiClient } = yield* Api;
const browser = yield* Browser;
const identity = yield* target.newIdentity();
const apiClient = yield* makeApiClient(api, identity);
const slug = `connect_ux_${randomBytes(4).toString("hex")}`;

yield* Effect.ensuring(
Effect.gen(function* () {
yield* apiClient.openapi.addSpec({
payload: { spec: { kind: "blob", value: bearerSpec() }, slug },
});

yield* browser.session(identity, async ({ page, step }) => {
await step("Open the connect modal", async () => {
await page.goto(`/integrations/${slug}?addAccount=1`, { waitUntil: "networkidle" });
await page.getByRole("heading", { name: /Add connection/ }).waitFor();
});

Comment thread
greptile-apps[bot] marked this conversation as resolved.
await step("The credential field merges the placement prefix", async () => {
// The placement's lead + prefix renders as a non-editable affix
// inside the field, so there is no separate preview line.
await page.getByText("Authorization: Bearer").first().waitFor();
});

await step("The add-method editor offers placement presets", async () => {
await page.getByRole("button", { name: "Add authentication method" }).click();
await page.getByRole("heading", { name: "Add authentication method" }).waitFor();
await page.getByRole("button", { name: "Bearer header" }).waitFor();
await page.getByRole("button", { name: "API key query" }).waitFor();
});

await step("A prefix with no trailing space warns", async () => {
await page.getByPlaceholder("Bearer ").first().fill("Bearer");
await page.getByText("Prefix has no trailing space").waitFor();
});

await step("Restoring the trailing space clears the warning", async () => {
await page.getByPlaceholder("Bearer ").first().fill("Bearer ");
await page.getByText("Prefix has no trailing space").waitFor({ state: "detached" });
});
});
}),
apiClient.openapi
.removeSpec({ params: { slug: IntegrationSlug.make(slug) } })
.pipe(Effect.ignore),
);
}),
),
);
99 changes: 80 additions & 19 deletions packages/react/src/components/add-account-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ function PasteCredentialInputs(props: {
/** 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;
}) {
Expand All @@ -173,7 +177,11 @@ function PasteCredentialInputs(props: {
<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>) =>
Expand Down Expand Up @@ -201,20 +209,55 @@ function PasteCredentialInputs(props: {
{labelled && (
<Label className="font-mono text-xs text-muted-foreground">{input.label}</Label>
)}
<Input
type="password"
autoComplete="new-password"
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
/>
{props.affix ? (
// Merged field: the placement's lead + prefix is a FIXED addon (its
// own muted segment, divider, non-selectable), and the user types
// only the token after it — the field reads as the header value being
// built. Only the border reacts to focus (no full-field glow). The
// autoComplete/ignore attrs stop the browser and password managers
// from offering to GENERATE a password here; the app's own 1Password
// picker covers filling an existing secret.
<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}
</span>
{/* 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>
Comment thread
greptile-apps[bot] marked this conversation as resolved.
) : (
<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>
Expand Down Expand Up @@ -292,6 +335,8 @@ function CredentialValueFields(props: {
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
Expand Down Expand Up @@ -346,6 +391,7 @@ function CredentialValueFields(props: {
inputs={props.inputs}
singleInput={props.singleInput}
showLabels={props.showLabels}
affix={props.affix}
values={props.values}
onChange={props.onValuesChange}
/>
Expand Down Expand Up @@ -882,6 +928,20 @@ function AddAccountModalView(props: AddAccountModalProps) {
method != null &&
method.placements.length > 0 &&
method.placements.every((p) => p.carrier === "env");
// 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).
Expand Down Expand Up @@ -1394,9 +1454,9 @@ function AddAccountModalView(props: AddAccountModalProps) {
<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
Expand Down Expand Up @@ -1452,9 +1512,9 @@ function AddAccountModalView(props: AddAccountModalProps) {
{dcrActive ? null : (
<TabsContent
value={methodId}
className="mt-0 min-w-0 space-y-5 rounded-md border border-border/60 bg-muted/15 p-4"
className="mt-0 min-w-0 space-y-5 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 ? (
Expand Down Expand Up @@ -1573,6 +1633,7 @@ function AddAccountModalView(props: AddAccountModalProps) {
inputs={credentialInputs}
singleInput={singleInput}
showLabels={isEnvMethod}
affix={singleCredentialAffix}
allowExternalProvider={!isEnvMethod}
values={values}
onValuesChange={setValues}
Expand Down
24 changes: 23 additions & 1 deletion packages/react/src/components/placement-editor.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { PlusIcon, XIcon } from "lucide-react";
import { PlusIcon, TriangleAlertIcon, XIcon } from "lucide-react";

import {
emptyPlacement,
Expand Down Expand Up @@ -47,6 +47,10 @@ export function PlacementEditor(props: {
const remove = (index: number): void =>
onChange(placements.filter((_p: Placement, j: number) => j !== index));

// A non-empty prefix with no trailing space is sent JOINED to the credential
// (e.g. "Bearer" + token -> "Bearertoken"). Almost always a mistake: warn.
const barePrefix = placements.some((p) => p.prefix.length > 0 && !p.prefix.endsWith(" "));

return (
<div className="flex flex-col gap-3">
<div className="flex flex-wrap gap-1.5">
Expand Down Expand Up @@ -147,6 +151,24 @@ export function PlacementEditor(props: {
) : null}
</div>
))}
{barePrefix ? (
<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">
<TriangleAlertIcon
className="mt-0.5 size-4 shrink-0 text-amber-600 dark:text-amber-400"
aria-hidden
/>
<div className="min-w-0 space-y-1">
<p className="font-medium text-amber-900 dark:text-amber-100">
Prefix has no trailing space
</p>
<p className="text-amber-900/80 dark:text-amber-100/80">
It is sent joined to the value (<span className="font-mono">Bearer••••••</span>). Most
APIs expect a space, like <span className="font-mono">"Bearer "</span>.
</p>
</div>
</section>
) : null}

<Button
type="button"
variant="outline"
Expand Down
Loading