diff --git a/e2e/scenarios/integration-detail-loading-surface.test.ts b/e2e/scenarios/integration-detail-loading-surface.test.ts new file mode 100644 index 0000000000..75ba21d9f8 --- /dev/null +++ b/e2e/scenarios/integration-detail-loading-surface.test.ts @@ -0,0 +1,322 @@ +// Opening an integration from the list shows ONE loading surface, not four. +// +// ## What went wrong +// +// Clicking an integration in the list walked the detail page through a run of +// visually distinct placeholders, each alive for a few hundred milliseconds: +// +// 1. the header printed the raw URL slug (`postman-echo-9f2a`) and then +// swapped it for the real name ("Postman Echo"); +// 2. the Accounts pane rendered the generic section with ZERO auth methods, +// which is the same render as "this integration has no way to connect" — +// so the dashed empty card briefly said "Ask a workspace admin to +// configure an authentication method"; +// 3. a pulsing dot and the words "Loading accounts…" — a loading vocabulary +// used nowhere else on the page; +// 4. the real accounts content, in a taller box than any of the above. +// +// Every one of those is an internal boundary of ours — the catalog row request, +// the plugin lookup that depends on it, the connections request — and none is a +// fact the person clicking an integration has any use for. What they produced +// was churn: several different things flashing in several different places, and +// one of them stating something false about the integration. +// +// ## What is asserted, and why it is asserted this way +// +// Same method as the artifact loading-surface scenario, for the same reason: +// the states in question were only ever on screen for a few hundred +// milliseconds, so a screenshot at one moment would miss them and pass against +// the old code too. The page is SAMPLED CONTINUOUSLY from before the click +// until the detail page is live, and the assertion is over everything that was +// ever on screen: +// +// - none of the placeholder texts ever appeared, and the raw slug was never +// shown as the page title; +// - the detail body's box never changed size, so nothing was laid out twice. +// +// Both directions are covered, because they fail differently: warm (a click +// from the list, catalog already in the client's atom cache) and cold (a direct +// URL in a fresh context, where nothing is cached and the window is widest). +import { randomBytes } from "node:crypto"; +import { createServer } from "node:http"; + +import { expect } from "@effect/vitest"; +import { Effect, Predicate } from "effect"; +import type { Page } from "playwright"; +import { composePluginApi } from "@executor-js/api/server"; +import { AccountHttpApi } from "@executor-js/api"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const api = composePluginApi([openApiHttpPlugin()] as const); + +const TEMPLATE = AuthTemplateSlug.make("apiKey"); + +/** A display name that shares no substring with the slug, so "the header showed + * the slug" and "the header showed the name" are impossible to confuse. */ +const DISPLAY_NAME = "Postman Echo"; + +/** A two-operation spec — enough that the Tools tab has real content to lay out + * once the page settles, which is what the steady-state box is measured at. */ +const echoSpec = (baseUrl: string): string => + JSON.stringify({ + openapi: "3.0.3", + info: { title: DISPLAY_NAME, version: "1.0.0" }, + servers: [{ url: baseUrl }], + paths: { + "/me": { + get: { + operationId: "getMe", + summary: "The current account", + responses: { "200": { description: "ok" } }, + }, + }, + "/echo": { + get: { + operationId: "getEcho", + summary: "Echo the request back", + responses: { "200": { description: "ok" } }, + }, + }, + }, + }); + +/** A real node:http upstream on 127.0.0.1 so discovery and health probes have + * something that answers. Closed by the scope's finalizer. */ +const serveEchoApi = Effect.acquireRelease( + Effect.callback<{ readonly url: string; readonly close: () => void }>((resume) => { + const server = createServer((request, response) => { + response.writeHead(200, { "content-type": "application/json" }); + response.end(JSON.stringify({ ok: true, url: request.url })); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resume( + Effect.succeed({ + url: `http://127.0.0.1:${port}`, + close: () => { + server.close(); + server.closeAllConnections(); + }, + }), + ); + }); + }), + (server) => Effect.sync(server.close), +); + +type Sample = { + readonly text: string; + readonly title: string | null; + readonly body: { + readonly w: number; + readonly h: number; + } | null; +}; + +declare global { + // eslint-disable-next-line no-var + var __detailSamples: Array | undefined; +} + +/** + * Watch the console document continuously for the whole open. + * + * Installed as an init script so it is running before the first byte of the + * page, and polls on an animation frame — fast enough that a placeholder + * visible for even one paint is recorded. The header title and the detail + * body's box are sampled alongside the text, because a placeholder that came + * and went without changing any WORDS would still have moved the layout, and a + * layout that jumps is the same defect wearing a different hat. + */ +const startSampling = async (page: Page): Promise => { + await page.addInitScript(() => { + globalThis.__detailSamples = []; + const sample = () => { + const body = document.querySelector('[data-testid="integration-detail-body"]'); + const box = body?.getBoundingClientRect(); + const title = document.querySelector('[data-testid="integration-detail-title"]'); + globalThis.__detailSamples?.push({ + text: document.body?.innerText ?? "", + title: title?.innerText ?? null, + body: box ? { w: Math.round(box.width), h: Math.round(box.height) } : null, + }); + requestAnimationFrame(sample); + }; + sample(); + }); +}; + +const readSamples = (page: Page): Promise> => + page.evaluate(() => globalThis.__detailSamples ?? []); + +const resetSamples = (page: Page): Promise => + page.evaluate(() => { + globalThis.__detailSamples = []; + }); + +/** + * Assert the whole open was one surface. + * + * Three independent properties over the same recording — no placeholder words, + * no raw slug in the title, and a body box that never changed — because any one + * alone would let the churn back in through another door. + */ +const expectSingleLoadingSurface = ( + samples: ReadonlyArray, + slug: string, + label: string, +): void => { + expect(samples.length, `${label}: the sampler actually ran`).toBeGreaterThan(3); + + // The exact strings the superseded placeholders rendered. Named literally + // rather than by testid: the point is that these WORDS are gone from the + // experience, and a rename that kept the churn should not pass. + const forbidden = [ + "Loading accounts", + // The empty-state copy for "this integration declares no auth method". It + // is a true sentence for such an integration and a false one here, so it + // must never appear for an integration that has connections. + "Ask a workspace admin to configure an authentication method", + "No connections yet", + ] as const; + + for (const text of forbidden) { + const hit = samples.findIndex((entry) => entry.text.includes(text)); + expect( + hit, + `${label}: "${text}" was on screen at sample ${hit} of ${samples.length} — the open still walks through more than one loading state`, + ).toBe(-1); + } + + // The title is the integration's name or nothing at all — never the raw slug + // from the URL, which is a machine identifier the reader did not ask to see. + const slugTitle = samples.findIndex((entry) => entry.title?.includes(slug) === true); + expect( + slugTitle, + `${label}: the header printed the raw slug "${slug}" at sample ${slugTitle} of ${samples.length} before the name arrived`, + ).toBe(-1); + + // The body's geometry, over every frame in which a body existed at all. The + // skeleton and the settled content share one box by construction, so a change + // here means the content was laid out differently from the skeleton that held + // its place. + const boxes = samples.map((entry) => entry.body).filter(Predicate.isNotNull); + expect(boxes.length, `${label}: the detail body was on screen at some point`).toBeGreaterThan(0); + + const first = boxes[0]; + if (!first) return; + for (const [index, box] of boxes.entries()) { + // A pixel of tolerance for sub-pixel rounding as the scrollbar settles. + expect( + Math.abs(box.w - first.w) <= 1 && Math.abs(box.h - first.h) <= 1, + `${label}: the detail body changed size mid-load at sample ${index} (${JSON.stringify(box)} vs ${JSON.stringify(first)}) — the content appeared in a different box than the skeleton held`, + ).toBe(true); + } +}; + +scenario( + "Integrations · opening an integration shows one loading surface", + { timeout: 240_000 }, + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: apiClient } = yield* Api; + + const upstream = yield* serveEchoApi; + const identity = yield* target.newIdentity(); + const client = yield* apiClient(api, identity); + + const slug = IntegrationSlug.make(`postman-echo-${randomBytes(4).toString("hex")}`); + + yield* client.openapi.addSpec({ + payload: { + spec: { kind: "blob", value: echoSpec(upstream.url) }, + slug, + baseUrl: upstream.url, + authenticationTemplate: [ + { + slug: "apiKey", + type: "apiKey", + headers: { authorization: ["Bearer ", { type: "variable", name: "token" }] }, + }, + ], + }, + }); + + // A real connection, so the settled Accounts pane is the POPULATED one — + // the state whose height the loading surface has to hold open. + yield* client.connections.create({ + payload: { + owner: "org", + name: ConnectionName.make("primary"), + integration: slug, + template: TEMPLATE, + value: "tok_echo", + }, + }); + + const accountClient = yield* apiClient(AccountHttpApi, identity); + const me = yield* accountClient.account.me(); + const orgSlug = me.organization?.slug; + const listPath = orgSlug ? `/${orgSlug}` : "/"; + const detailPath = orgSlug + ? `/${orgSlug}/integrations/${String(slug)}` + : `/integrations/${String(slug)}`; + + // ------------------------------------------------------------------ + // WARM: the journey a user actually takes — the list, then a click. + // ------------------------------------------------------------------ + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the integrations list", async () => { + await startSampling(page); + await visit(page, `${target.baseUrl}${listPath}`); + await page.getByTestId(`integration-entry-${String(slug)}`).waitFor({ timeout: 30_000 }); + // Discard everything from the list's own load: this scenario is about + // the OPEN, and the list has a loading state of its own. + await resetSamples(page); + }); + + await step("Click through to the integration", async () => { + await page.getByTestId(`integration-entry-${String(slug)}`).click(); + await page.getByTestId("connection-row-primary").waitFor({ timeout: 60_000 }); + }); + + await step("The whole open was one surface", async () => { + expectSingleLoadingSurface(await readSamples(page), String(slug), "warm open"); + }); + + await step("The settled page is the real integration", async () => { + await expect + .poll(async () => await page.getByTestId("integration-detail-title").innerText(), { + timeout: 10_000, + message: "the header carries the integration's display name", + }) + .toContain(DISPLAY_NAME); + }); + }); + + // ------------------------------------------------------------------ + // COLD: the deep link, in a context that has never loaded the console. + // + // The harder case: nothing is cached, so the catalog row and the + // connections both start from zero and the loading window is at its widest. + // ------------------------------------------------------------------ + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the integration by URL, cold", async () => { + await startSampling(page); + await page.goto(`${target.baseUrl}${detailPath}`, { waitUntil: "commit" }); + await page.getByTestId("connection-row-primary").waitFor({ timeout: 60_000 }); + }); + + await step("The cold open was one surface too", async () => { + expectSingleLoadingSurface(await readSamples(page), String(slug), "cold open"); + }); + }); + }), +); diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index b87b5d8229..308a5689a9 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -1,6 +1,5 @@ import { useEffect, useMemo, useState } from "react"; import { useAtomValue, useAtomRefresh, useAtomSet } from "@effect/atom-react"; -import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; import * as Exit from "effect/Exit"; import { IntegrationSlug, @@ -27,6 +26,7 @@ import { ownerLabel, useOwnerDisplay } from "../api/owner-display"; import { trackEvent } from "../api/analytics"; import { useCanCreateWorkspaceConnections } from "../multiplayer/use-admin-nav"; import type { AuthMethod } from "../lib/auth-placements"; +import { asyncResultValue, isAsyncResultLoading } from "../lib/async-result"; import { connectionNeedsReconsent, oauthReconnectPayload, @@ -52,6 +52,7 @@ import { AlertDialogTitle, } from "./alert-dialog"; import { Badge } from "./badge"; +import { Skeleton } from "./skeleton"; import { Button } from "./button"; import { CardStack, @@ -209,7 +210,10 @@ function AccountRow(props: { }; return ( - + { @@ -644,12 +649,15 @@ export function AccountsSection(props: { useAtomSet(addConnectionOptimistic("user")); const totalCount = useMemo(() => { - const orgRows = AsyncResult.isSuccess(orgConnections) ? orgConnections.value.length : 0; - const userRows = AsyncResult.isSuccess(userConnections) ? userConnections.value.length : 0; + const orgRows = asyncResultValue(orgConnections)?.length ?? 0; + const userRows = asyncResultValue(userConnections)?.length ?? 0; return orgRows + userRows; }, [orgConnections, userConnections]); - const loading = !AsyncResult.isSuccess(orgConnections) && !AsyncResult.isSuccess(userConnections); + // Loading means "nothing to show yet" — NOT "a request is in flight". Once + // either owner has answered, a later refresh keeps rendering what it knows + // instead of collapsing the list back into a placeholder. + const loading = isAsyncResultLoading(orgConnections) && isAsyncResultLoading(userConnections); // When there are zero connections the dashed empty-state card below carries // its own "Add connection" CTA, so the header button would be a redundant @@ -697,9 +705,12 @@ export function AccountsSection(props: { {loading ? ( -
-
-

Loading accounts…

+ // A placeholder in the shape of the row it is holding space for — not a + // pulsing dot and a sentence, which is a different thing in a different + // place from everything it replaces. +
+ +
) : showEmptyState ? (
diff --git a/packages/react/src/lib/async-result.ts b/packages/react/src/lib/async-result.ts index 94f0d333b7..9e13fb394f 100644 --- a/packages/react/src/lib/async-result.ts +++ b/packages/react/src/lib/async-result.ts @@ -7,3 +7,19 @@ export function isAsyncResultLoading(result: AsyncResult.AsyncResult (AsyncResult.isWaiting(result) && Option.isNone(AsyncResult.value(result))) ); } + +/** + * The result's value when it has one — INCLUDING the value retained while a + * revalidation is in flight. + * + * `AsyncResult.isSuccess` is false for a waiting result even when that result + * is still carrying the data it loaded a moment ago. Reading only `isSuccess` + * therefore turns an ordinary background refresh — an atom's time-to-live + * lapsing, a write firing a reactivity key — into something that looks exactly + * like a cold load, and replaces live content with a placeholder that has + * nothing to say. Read through this instead wherever a placeholder would + * otherwise cover data the client already has. + */ +export function asyncResultValue(result: AsyncResult.AsyncResult): A | undefined { + return Option.getOrUndefined(AsyncResult.value(result)); +} diff --git a/packages/react/src/pages/integration-detail.tsx b/packages/react/src/pages/integration-detail.tsx index a30d8da9d6..a98d57e533 100644 --- a/packages/react/src/pages/integration-detail.tsx +++ b/packages/react/src/pages/integration-detail.tsx @@ -18,6 +18,7 @@ import { import { checkConnectionHealth, connectionsAllAtom, + connectionsForIntegrationAtom, integrationToolsAllAtom, integrationsOptimisticAtom, integrationAtom, @@ -45,7 +46,7 @@ import { Button } from "../components/button"; import { Skeleton } from "../components/skeleton"; import { useExecutorDocumentTitle } from "../lib/document-title"; import { ErrorState } from "../components/error-state"; -import { isAsyncResultLoading } from "../lib/async-result"; +import { asyncResultValue, isAsyncResultLoading } from "../lib/async-result"; import { useConnectionsHealth } from "../lib/use-connection-health"; import { accountPolicyPattern } from "../lib/policy-pattern"; import { @@ -137,10 +138,38 @@ export function IntegrationDetailPage(props: { setActiveTab(integrationDetailInternalTabFromSearch(props.tab)); }, [namespace, props.tab]); - const integrationData = AsyncResult.isSuccess(integration) ? integration.value : null; + // Everything the page renders about the integration hangs off this. Read + // with the retained value so a background refresh cannot blank the page's + // own identity — its name, its kind, the plugin that owns its Accounts pane. + const integrationData = asyncResultValue(integration) ?? null; useExecutorDocumentTitle(integrationData?.name || namespace); const isBuiltInIntegration = namespace === "executor" || integrationData?.kind === "built-in"; const currentTab = isBuiltInIntegration ? "tools" : activeTab; + + // The Accounts pane's own readiness, read from the SAME atoms it reads (the + // registry dedupes, so this costs no extra request). The page holds one + // skeleton until these have answered, which is why the pane can never paint + // its own second placeholder underneath this one. + const orgConnections = useAtomValue( + connectionsForIntegrationAtom({ integration: slug, owner: "org" }), + ); + const userConnections = useAtomValue( + connectionsForIntegrationAtom({ integration: slug, owner: "user" }), + ); + // Only the Accounts tab is waiting on connections; the Tools tab is not, and + // making it wait would hold a skeleton over content that is already there. + const willShowAccounts = namespace !== "executor" && activeTab === "accounts"; + const accountsPending = + willShowAccounts && + isAsyncResultLoading(orgConnections) && + isAsyncResultLoading(userConnections); + // Everything on this page is derived from the catalog row: the name, whether + // the integration is built-in (which tabs exist), which plugin owns the + // Accounts pane, and the declared auth methods. Rendering before it lands + // does not show less — it shows a DIFFERENT page, stating things that are + // not true of this integration, and then replaces it. So hold one surface + // until the page can be itself. + const pending = isAsyncResultLoading(integration) || accountsPending; // Integrations are workspace-owned; the server refuses catalog mutations // (update/remove) from non-admin members, so disable the controls for them. const canMutateIntegration = useCanCreateWorkspaceConnections(); @@ -501,9 +530,20 @@ export function IntegrationDetailPage(props: { {/* Header bar */}
-

- {integrationData?.name || namespace} -

+ {/* The slug is a machine identifier out of the URL, not this + integration's name. Printing it while the catalog row is in + flight puts a word on screen that the reader then watches get + replaced — so hold the space instead and say nothing. */} + {pending ? ( + + ) : ( +

+ {integrationData?.name || namespace} +

+ )} {AsyncResult.isSuccess(tools) && ( {distinctToolCount} {distinctToolCount === 1 ? "tool" : "tools"} @@ -572,134 +612,143 @@ export function IntegrationDetailPage(props: {
- -
- - {!isBuiltInIntegration && Accounts} - Tools - -
- - {/* Hub: integration-level auth methods + accounts. Plugins that - declare auth methods fill the `accounts` slot (real methods from - the plugin's config); otherwise we render the generic fallback. */} - {!isBuiltInIntegration && ( - - {editPlugin?.accounts ? ( - }> - - - ) : ( -
- -
- )} - - )} - - {/* Tools -- split pane (unchanged behavior) */} - - {isAsyncResultLoading(tools) ? ( - - ) : ( - AsyncResult.match(tools, { - onInitial: () => , - onFailure: () => ( -
- -
- ), - onSuccess: () => ( -
- {/* Left: tool tree */} -
- + ) : ( + +
+ + {!isBuiltInIntegration && Accounts} + Tools + +
+ + {/* Hub: integration-level auth methods + accounts. Plugins that + declare auth methods fill the `accounts` slot (real methods from + the plugin's config); otherwise we render the generic fallback. */} + {!isBuiltInIntegration && ( + + {editPlugin?.accounts ? ( + }> + + + ) : ( +
+
+ )} + + )} - {/* Right: tool detail with Schema · TypeScript · Run tabs */} -
- {selectedTool && selectedAddress && selectedBareName ? ( - + {isAsyncResultLoading(tools) ? ( + + ) : ( + AsyncResult.match(tools, { + onInitial: () => , + onFailure: () => ( +
+ +
+ ), + onSuccess: () => ( +
+ {/* Left: tool tree */} +
+ +
+ + {/* Right: tool detail with Schema · TypeScript · Run tabs */} +
+ {selectedTool && selectedAddress && selectedBareName ? ( + - ) : !isBuiltInIntegration && integrationConnections.length === 0 ? ( - 0} - /> - ) : hasToolSyncIssue ? ( - void handleRetryTools()} - disabled={retryingTools} - > - {retryingTools ? "Checking…" : "Check and sync tools"} - - } - /> - ) : ( - 0} /> - )} -
-
- ), - }) - )} - - + {...(!selection?.static && selectedBareName + ? { + integration: slug, + runToolName: selectedBareName, + connections: integrationConnections, + initialConnectionName: selection?.connection ?? null, + } + : {})} + /> + ) : !isBuiltInIntegration && integrationConnections.length === 0 ? ( + 0} + /> + ) : hasToolSyncIssue ? ( + void handleRetryTools()} + disabled={retryingTools} + > + {retryingTools ? "Checking…" : "Check and sync tools"} + + } + /> + ) : ( + 0} /> + )} +
+
+ ), + }) + )} + + + )} +
+
+
+ + +
+
+ {props.tab === "accounts" ? : } + + ); +} + function IntegrationDetailSkeleton() { return (