mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 16:51:28 +02:00
fix(connections): stop the sign-in screen from adding a phantom step (#13601)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Agents reach other products through connections, which people create
in the connector setup wizard
> - That wizard shows a stepper: a row of dots, a label under each, and
"Step 1 of 2" in the header
> - When a curated OAuth app hands off to the provider, a waiting screen
replaces the wizard while Paperclip prepares sign-in
> - That waiting screen drew its own stepper with three hardcoded
labels, so the stepper grew a dot at the exact moment the person pressed
Connect
> - A stepper that grows mid-flow tells the person they missed a screen
> - This pull request makes the waiting screen show the step model of
the flow that opened it
> - The benefit is that the stepper keeps the same shape from the first
screen to the handoff
## Linked Issues or Issue Description
**What happened?**
The connector setup wizard shows a two-step stepper for a curated OAuth
app: two dots, labelled "Access" and "Sign in", with "Step 1 of 2" and
then "Step 2 of 2" in the header. When you press Connect, a third dot
appears, labelled "Ready".
The extra dot comes from the OAuth waiting screen.
`OAuthConnectStateScreen` rendered its own header with
`labels={["Access", "Sign in", "Ready"]}`, a constant that did not
depend on the wizard that opened it.
"Ready" is also not a step the wizard can reach. The success screen
hides the step header, so no one ever sees a third step become active.
**Expected behavior**
The stepper keeps the same number of steps from the first screen through
the provider handoff. Waiting for browser sign-in is part of the last
step, not a step of its own.
**Steps to reproduce**
1. Open **Apps → Connect** and pick a curated app that signs in through
OAuth, for example Notion.
2. Look at the stepper. It shows two dots and reads "Step 1 of 2".
3. Complete the Access step and press the button that starts sign-in.
4. Look at the stepper on the "Preparing secure sign-in" screen. Before
this change it shows three dots and adds a "Ready" label.
**Paperclip version or commit**
`45586170e` on `master`.
**Deployment mode**
Local.
**Agent adapter(s) involved**
Not adapter-specific (core bug).
**Additional context**
The reporter suggested removing the stepper, on the condition that no
connector flow has three or more steps. One does.
`ConnectionSetupFlow.tsx` keeps `STEP_LABELS = ["Pick app", "Access",
"Add your key"]` for the generic path, which a person walks when they
paste an MCP endpoint instead of choosing a curated app. The two-step
counts apply only after an app is selected. The condition fails, so this
pull request keeps the stepper and repairs the phantom step alone.
## What Changed
- `OAuthConnectStateScreen` takes a `steps` prop: the labels and active
index of the flow that opened it.
- The curated OAuth path passes its own two-step model, so the count
does not change at the handoff.
- The generic pasted-endpoint path passes the three-step model it was
already walking, so its count does not change either.
- The default for hosts without a wizard of their own, such as the
paste-a-config tab, is `["Access", "Sign in"]`. The invented "Ready"
step is gone.
- `AppsConnect.test.tsx` gains a test that reads the stepper before and
after the handoff and fails if the two differ.
## Verification
- `npx vitest run ui/src/pages/apps/AppsConnect.test.tsx
ui/src/features/connections ui/src/pages/tools/PasteConfigTab.test.tsx`
— 185 tests pass.
- `npx vitest run ui/src/pages/apps/generic-mcp-connect.test.ts` — 27
tests pass.
- `npx tsc -b ui` — clean.
- The new test fails on the previous code. With the old three-label
constant restored it reports `["Access", "Sign in", "Ready"]` where it
expects two labels.
## Risks
Low. The change is limited to which labels the waiting screen draws. No
connection logic, no network call, and no navigation changes.
## Model Used
Claude Opus 5 (`claude-opus-5`), 1M context, extended thinking, with
tool use and code execution.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [ ] I will address all Greptile and reviewer comments before
requesting merge
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
352153b5ed
commit
378f11994b
2 files changed
+97
-7
No files matched your search
@@ -315,6 +315,11 @@ const SELECTED_APP_STEP_INDEX: Record<Exclude<Step, "gallery" | "success">, numb
|
||||
key: 1,
|
||||
};
|
||||
const ZAPIER_STEP_LABELS = ["Access", "Add MCP URL"];
|
||||
// Waiting for browser sign-in happens *inside* the flow's last step, so the
|
||||
// waiting screen reuses the step model of the flow that opened it. It must not
|
||||
// append a trailing step the flow never lands on: a stepper that grows from two
|
||||
// dots to three the moment you press Connect reads as a step you missed.
|
||||
const OAUTH_SIGN_IN_STEP_LABELS = ["Access", "Sign in"];
|
||||
|
||||
/**
|
||||
* Which identity a fresh connection should default to (PAP-17835).
|
||||
@@ -1987,6 +1992,9 @@ export function ConnectionSetupFlow({
|
||||
name: linkName.trim() || endpointHost(linkUrl) || "this server",
|
||||
unverifiedHost: endpointHost(linkUrl),
|
||||
}}
|
||||
// A pasted endpoint walks the generic three-step wizard, so keep that
|
||||
// model rather than dropping to the curated two-step one.
|
||||
steps={{ labels: STEP_LABELS, activeIndex: STEP_INDEX.key }}
|
||||
phase={oauthPhase}
|
||||
error={oauthError}
|
||||
authorizationHost={authorizationHost}
|
||||
@@ -2464,17 +2472,25 @@ function StepHeader({
|
||||
</Button>
|
||||
</div>
|
||||
{step !== "gallery" && (
|
||||
<div className="mt-4">
|
||||
<div className="flex gap-2">
|
||||
// A landmark with stable hooks, so the step model can be read without
|
||||
// guessing at Tailwind classes. The dots are decoration — the label
|
||||
// line below already says the same thing, so announcing both would
|
||||
// read every step name twice.
|
||||
<nav className="mt-4" aria-label="Setup progress" data-testid="wizard-stepper">
|
||||
<ol className="flex gap-2" aria-hidden="true">
|
||||
{labels.map((label, i) => (
|
||||
<div
|
||||
<li
|
||||
key={label}
|
||||
data-testid="wizard-step-dot"
|
||||
data-step-active={i === activeIndex ? "true" : undefined}
|
||||
className={cn("h-1 w-20 rounded-full", i <= activeIndex ? "bg-foreground" : "bg-border")}
|
||||
/>
|
||||
))}
|
||||
</ol>
|
||||
<div className="mt-2 text-xs text-muted-foreground" data-testid="wizard-step-labels">
|
||||
{labels.join(" · ")}
|
||||
</div>
|
||||
<div className="mt-2 text-xs text-muted-foreground">{labels.join(" · ")}</div>
|
||||
</div>
|
||||
</nav>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
@@ -2489,6 +2505,7 @@ export function OAuthConnectStateScreen({
|
||||
recoveryActions,
|
||||
authorizationHost,
|
||||
authorizationUrl,
|
||||
steps = { labels: OAUTH_SIGN_IN_STEP_LABELS, activeIndex: 1 },
|
||||
onRetry,
|
||||
onOpenAuthorization,
|
||||
onBack,
|
||||
@@ -2511,6 +2528,12 @@ export function OAuthConnectStateScreen({
|
||||
authorizationHost?: string | null;
|
||||
/** Already validated by prepareOAuthNavigation; used for a native browser link. */
|
||||
authorizationUrl?: string | null;
|
||||
/**
|
||||
* Step model of the flow that opened this screen, so the stepper keeps the
|
||||
* shape it had on the previous screen. Defaults to the two-step curated
|
||||
* sign-in model for hosts that have no wizard of their own.
|
||||
*/
|
||||
steps?: { labels: string[]; activeIndex: number };
|
||||
onOpenAuthorization?: () => void;
|
||||
onRetry: () => void;
|
||||
onBack: () => void;
|
||||
@@ -2549,8 +2572,8 @@ export function OAuthConnectStateScreen({
|
||||
<StepHeader
|
||||
subtitle="Secure MCP sign-in"
|
||||
step="key"
|
||||
activeIndex={1}
|
||||
labels={["Access", "Sign in", "Ready"]}
|
||||
activeIndex={steps.activeIndex}
|
||||
labels={steps.labels}
|
||||
appIdentity={entry ? { name: entry.name, logoUrl: entry.branding.logoUrl, darkLogoUrl: entry.branding.darkLogoUrl } : undefined}
|
||||
unverifiedHost={unverifiedHost}
|
||||
onCancel={onCancel}
|
||||
|
||||
@@ -150,6 +150,24 @@ function radioContaining(text: string): HTMLButtonElement | undefined {
|
||||
);
|
||||
}
|
||||
|
||||
/** The wizard stepper's progress dots, one per step. */
|
||||
function stepDots(): HTMLElement[] {
|
||||
return Array.from(
|
||||
document.body.querySelectorAll<HTMLElement>('[data-testid="wizard-step-dot"]'),
|
||||
);
|
||||
}
|
||||
|
||||
function stepDotCount(): number {
|
||||
return stepDots().length;
|
||||
}
|
||||
|
||||
/** The step names printed under the dots, e.g. "Access · Sign in". */
|
||||
function stepLabelsOnScreen(): string[] {
|
||||
const labelLine =
|
||||
document.body.querySelector('[data-testid="wizard-step-labels"]')?.textContent ?? "";
|
||||
return labelLine.split("·").map((label) => label.trim()).filter(Boolean);
|
||||
}
|
||||
|
||||
/**
|
||||
* Advance past the Access step (PAP-17835), which now sits between picking a
|
||||
* curated app and entering its credential. Picks "Any agent" so Continue is
|
||||
@@ -1837,6 +1855,25 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => {
|
||||
expect(connectAppMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("keeps the same steps when Notion sign-in takes over the wizard", async () => {
|
||||
mockSearch.value = "source=notion";
|
||||
listGalleryMock.mockResolvedValueOnce({ apps: [NOTION] });
|
||||
connectAppMock.mockReturnValueOnce(new Promise(() => {}));
|
||||
|
||||
await render();
|
||||
|
||||
const stepsOnAccess = stepLabelsOnScreen();
|
||||
expect(stepsOnAccess).toEqual(["Access", "Sign in"]);
|
||||
|
||||
await passAccessStep();
|
||||
await submitCuratedOAuthSetup();
|
||||
|
||||
expect(container.textContent).toContain("Preparing secure sign-in");
|
||||
// Connecting must not grow the stepper a step the flow never lands on.
|
||||
expect(stepLabelsOnScreen()).toEqual(stepsOnAccess);
|
||||
expect(stepDotCount()).toBe(stepsOnAccess.length);
|
||||
});
|
||||
|
||||
it("backs from the sign-in checkpoint to Access without exiting the wizard", async () => {
|
||||
mockSearch.value = "source=notion";
|
||||
listGalleryMock.mockResolvedValueOnce({ apps: [NOTION] });
|
||||
@@ -3292,6 +3329,36 @@ describe("AppsConnect — guided generic MCP flow (PAP-17087)", () => {
|
||||
expect(container.textContent).toContain("auth.example.test");
|
||||
});
|
||||
|
||||
// The curated flow has its own regression test for this. The generic flow is
|
||||
// a separate three-step model with a separate override, so it needs its own —
|
||||
// otherwise the waiting screen could quietly drop back to the curated
|
||||
// two-step labels here and nothing would catch it.
|
||||
it("keeps the generic three-step model when a pasted endpoint hands off to sign-in", async () => {
|
||||
connectAppMock.mockResolvedValue({
|
||||
connectionId: "conn-1",
|
||||
application: { id: "app-1", name: "mcp.example.test" },
|
||||
actions: { readOnly: [], canMakeChanges: [] },
|
||||
catalog: [],
|
||||
suggestedDefaults: {},
|
||||
auth: { kind: "oauth", startUrl: "https://auth.example.test/authorize?state=abc" },
|
||||
});
|
||||
await render();
|
||||
await gotoLinkFrame(container, "https://mcp.example.test/mcp");
|
||||
|
||||
const stepsBeforeHandoff = stepLabelsOnScreen();
|
||||
expect(stepsBeforeHandoff).toEqual(["Pick app", "Access", "Add your key"]);
|
||||
|
||||
await act(async () => {
|
||||
buttonByText("Check link")?.dispatchEvent(new MouseEvent("click", { bubbles: true }));
|
||||
});
|
||||
await flushReact();
|
||||
|
||||
// Signing in must neither grow the stepper nor shrink it to the curated
|
||||
// two-step set the pasted endpoint never walked.
|
||||
expect(stepLabelsOnScreen()).toEqual(stepsBeforeHandoff);
|
||||
expect(stepDotCount()).toBe(stepsBeforeHandoff.length);
|
||||
});
|
||||
|
||||
it("exchanges a managed connect response in the tenant without opening Cloud confirmation", async () => {
|
||||
const session = "managed_background_session_1234";
|
||||
const request = vi.spyOn(globalThis, "fetch").mockImplementation(async (input) =>
|
||||
|
||||
Reference in new issue
Block a user