From 378f11994bd95f29dd9dac804f70d8d94023659a Mon Sep 17 00:00:00 2001 From: scotttong Date: Fri, 18 Sep 2026 14:11:35 -0700 Subject: [PATCH] fix(connections): stop the sign-in screen from adding a phantom step (#13601) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .../connections/ConnectionSetupFlow.tsx | 37 ++++++++-- ui/src/pages/apps/AppsConnect.test.tsx | 67 +++++++++++++++++++ 2 files changed, 97 insertions(+), 7 deletions(-) diff --git a/ui/src/features/connections/ConnectionSetupFlow.tsx b/ui/src/features/connections/ConnectionSetupFlow.tsx index bc126a9cfb4..ef94a0ba81b 100644 --- a/ui/src/features/connections/ConnectionSetupFlow.tsx +++ b/ui/src/features/connections/ConnectionSetupFlow.tsx @@ -315,6 +315,11 @@ const SELECTED_APP_STEP_INDEX: Record, 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({ {step !== "gallery" && ( -
-
+ // 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. +
+ )}
); @@ -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({ ('[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) =>