fix(app-shell): arm the create deep link on the create action, not on any toolbar action (#4123)

.changeset/environment-deeplink-arm-on-create-action-4123.md

@@ -0,0 +1,13 @@
+---
+'@object-ui/app-shell': patch
+---
+
+`?runAction=create_environment` is no longer consumed when the environments toolbar has no create action to run it on.
+
+`EnvironmentListToolbar` armed the deep link on `toolbarActions.length > 0` — the presence of *any* toolbar action. Consumption of this deep link is modelled as stripping the param from the URL, so arming is destructive: a toolbar carrying some other action stripped `?runAction=create_environment` and triggered nothing (measured with the real `action:bar` + `action:button` runner: `urlParam=null execute=0`). Because the strip *is* the consumption, the user's intent was unrecoverable — reloading could not retry it, and the welcome page's "Create your environment" CTA (#844) simply landed on the list with no dialog and no way to ask again.
+
+Arming now keys on the create action actually being present. When it is not, the URL is left alone, so the next mount that can act on the deep link still does — a reload, or the action arriving with fresh metadata.
+
+The two lists that used to disagree are now one. The toolbar filtered placement only (`locations`), while `action:bar` additionally applies the ADR-0066 D4 capability gate (`requiredPermissions`) to what it renders — so a create action the caller may not invoke was counted here and dropped there, which is the divergence that made the bug reachable without any change to cloud metadata. The toolbar now builds its list with both of the bar's predicates (`actionRendersAt` + `useCapabilityGate`, the shared hook that exists precisely to keep self-filtering surfaces from drifting), and every affordance derived from that list follows: the loading skeleton is held only for a create button that can actually arrive, and the plan-locked "Add environment" upgrade CTA — which stands in for the create action — is not offered when there is no create action to stand in for.
+
+The `#3803`-verified consumption ordering is untouched and re-pinned: the runner still consumes `autoTrigger` before the parent strips the param.

packages/app-shell/src/environment/EnvironmentListToolbar.tsx

@@ -59,7 +59,8 @@
  */
 
 import { useEffect, useRef, useState } from 'react';
-import { SchemaRenderer } from '@object-ui/react';
+import { SchemaRenderer, useCapabilityGate } from '@object-ui/react';
+import { actionRendersAt } from '@object-ui/types';
 import { Button, Skeleton } from '@object-ui/components';
 import { Plus } from 'lucide-react';
 import { useObjectTranslation } from '@object-ui/i18n';
@@ -79,6 +80,17 @@ const CREATE_ACTION = 'create_environment';
  * user doesn't land on the list and have to find the create button again.
  * The param is consumed exactly once (stripped from the URL on consumption)
  * so refresh / back don't re-open the dialog.
+ *
+ * ## Arming is destructive — only arm when there is something to trigger (#4123)
+ *
+ * Consumption is modelled as "strip the param", so arming SPENDS a one-shot
+ * user intent. Arm it when nothing can act on it and the intent is not merely
+ * lost, it is unrecoverable: the URL no longer carries it, so a reload cannot
+ * retry. That is why the caller passes a non-null `ctaKind` only once the
+ * create action is actually on the bar — see `hasCreateAction` below. When it
+ * is not, the honest degradation is to leave the URL alone: the next mount
+ * that CAN act on the deep link still does (a reload, or the action arriving
+ * with fresh metadata).
  */
 function useAutoRunCreate(ctaKind: string | null): boolean {
   // Deliberately router-free (window.location + history.replaceState): the
@@ -141,9 +153,28 @@ interface Props {
 
 export function EnvironmentListToolbar({ actions, entitlements, onUpgrade }: Props) {
   const { t } = useObjectTranslation();
-  const toolbarActions = (actions || []).filter((a: any) => a?.locations?.includes('list_toolbar'));
+  // [ADR-0066 D4] The same capability gate `action:bar` applies to the actions
+  // it RENDERS. This toolbar is one more surface that filters its own action
+  // list instead of routing through `ActionEngine.getActionsForLocation`, which
+  // is precisely the case `useCapabilityGate` exists to keep from drifting.
+  const mayInvoke = useCapabilityGate();
+  // ONE list, built from both predicates the bar applies downstream
+  // (`actionRendersAt` + the capability gate), so what this component decides
+  // and what the bar renders cannot disagree by construction (#4123). The bar
+  // re-applies both to the list it receives — idempotent on an already-filtered
+  // list. Before this, placement was filtered here and capability only there,
+  // so a create action the caller may not invoke was counted here and dropped
+  // there.
+  const toolbarActions = (actions || []).filter(
+    (a: any) => actionRendersAt(a, 'list_toolbar') && mayInvoke(a?.requiredPermissions),
+  );
+  const hasCreateAction = toolbarActions.some((a: any) => a?.name === CREATE_ACTION);
   const ctaKind = entitlements?.ready ? decideEnvironmentCta(entitlements) : null;
-  const autoRunCreate = useAutoRunCreate(toolbarActions.length > 0 ? ctaKind : null);
+  // Arm the deep link on the CREATE ACTION's presence — never on "the toolbar
+  // has actions" (#4123). The old predicate armed on `toolbarActions.length > 0`,
+  // so a toolbar carrying any other action consumed `?runAction=create_environment`
+  // and triggered nothing (measured: `urlParam=null execute=0`), unrecoverably.
+  const autoRunCreate = useAutoRunCreate(hasCreateAction ? ctaKind : null);
 
   // Deep-linked "create" while in the upgrade state opens the SAME upgrade
   // prompt — the honest answer to "create" here. In an effect, not render:
@@ -164,11 +195,10 @@ export function EnvironmentListToolbar({ actions, entitlements, onUpgrade }: Pro
   // no create action at all, nothing can jump and no skeleton is rendered.
   if (entitlements === null) {
     const others = otherActions(toolbarActions);
-    const hasCreate = others.length !== toolbarActions.length;
     return (
       <>
         <OtherActionsBar actions={others} />
-        {hasCreate && (
+        {hasCreateAction && (
           // Sized like the `size="sm"` bar button it stands in for (h-9,
           // rounded-md), mirroring CloudOnboardingNext's loading placeholder.
           // The width is an approximation of the resolved labels — the point is
@@ -186,15 +216,24 @@ export function EnvironmentListToolbar({ actions, entitlements, onUpgrade }: Pro
     return (
       <>
         <OtherActionsBar actions={otherActions(toolbarActions)} />
-        <Button
-          size="sm"
-          onClick={() => onUpgrade(upgradeDialogSpec(entitlements!, t))}
-          className="shadow-none gap-1.5 sm:gap-2 h-8 sm:h-9"
-          data-testid="environment-add-upgrade"
-        >
-          <Plus className="h-4 w-4" />
-          <span>{t('environment.addEnvironment')}</span>
-        </Button>
+        {/*
+          This button IS the create affordance for a plan-locked org — it stands
+          in for the create action so the click opens the upgrade prompt instead
+          of POSTing into a 403. With no create action on the bar there is
+          nothing to stand in for, so it must not appear either (#4123): same
+          one-list rule as the arming predicate above.
+        */}
+        {hasCreateAction && (
+          <Button
+            size="sm"
+            onClick={() => onUpgrade(upgradeDialogSpec(entitlements!, t))}
+            className="shadow-none gap-1.5 sm:gap-2 h-8 sm:h-9"
+            data-testid="environment-add-upgrade"
+          >
+            <Plus className="h-4 w-4" />
+            <span>{t('environment.addEnvironment')}</span>
+          </Button>
+        )}
       </>
     );
   }

packages/app-shell/src/environment/__tests__/EnvironmentListToolbar.deepLinkArming.test.tsx

@@ -0,0 +1,269 @@
+/**
+ * #4123 — what ARMS the `?runAction=create_environment` deep link, and what
+ * happens when the thing it names is not on the bar.
+ *
+ * Consumption of this deep link is modelled as "strip the param from the URL",
+ * so arming is destructive: whatever arms it spends a one-shot user intent that
+ * a reload cannot recreate. The arming predicate must therefore agree with what
+ * actually renders — if it can arm while the create action is absent, the
+ * intent is spent on nothing and is gone for good (`urlParam=null execute=0`,
+ * the measurement on the card).
+ *
+ * ## Why this file drives the REAL bar
+ *
+ * The sibling `EnvironmentListToolbar.test.tsx` stubs `SchemaRenderer` to
+ * inspect the schema the toolbar hands down. That stub cannot see this defect
+ * class at all, because the divergence lives INSIDE `action:bar`: the ADR-0066
+ * D4 capability gate (`useCapabilityGate` over `requiredPermissions`) filters
+ * the actions the bar RENDERS, and the toolbar used to arm off a list computed
+ * before that gate ran. Two filters over one list is the bug; only the real bar
+ * can demonstrate it. Same reason #3803's investigation had to swap the stub
+ * out before it could measure the runner's consumption order.
+ *
+ * The registry import is at module scope (not in a `beforeAll`) per AGENTS.md
+ * §测试纪律 — `action:bar` resolves its members through the ComponentRegistry at
+ * render time, and the cost belongs in the import phase where no hook timeout
+ * applies.
+ */
+
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { render, screen, waitFor } from '@testing-library/react';
+import React from 'react';
+// Side-effect import: registers `action:bar` / `action:button` in the
+// ComponentRegistry that the real `SchemaRenderer` resolves against.
+import '@object-ui/components';
+import { ActionProvider } from '@object-ui/react';
+import { I18nProvider } from '@object-ui/i18n';
+import { EnvironmentListToolbar } from '../EnvironmentListToolbar';
+import type { EnvironmentEntitlementsState } from '../entitlements';
+
+const CREATE = {
+  name: 'create_environment',
+  label: 'Create Environment',
+  type: 'api',
+  variant: 'primary',
+  locations: ['list_toolbar'],
+};
+/** A perfectly ordinary second toolbar action — the one the old predicate counted. */
+const REFRESH = {
+  name: 'refresh_environments',
+  label: 'Refresh',
+  type: 'api',
+  locations: ['list_toolbar'],
+};
+
+const st = (o: Partial<EnvironmentEntitlementsState>): EnvironmentEntitlementsState =>
+  ({ ready: true, hasProductionEnv: true, upgradeUrl: '/settings/billing', source: 'summary', ...o });
+
+/** The state that makes the create action a live "set up production" CTA. */
+const SETUP_PRODUCTION = st({ hasProductionEnv: false });
+
+const runActionParam = () => new URL(window.location.href).searchParams.get('runAction');
+
+let events: string[] = [];
+let origReplaceState: typeof window.history.replaceState;
+
+function deepLink() {
+  const url = new URL(window.location.href);
+  url.searchParams.set('runAction', 'create_environment');
+  // Set the param BEFORE the instrumentation goes on, so the toolbar's own
+  // consume-and-strip is the only `replaceState` the events log can see.
+  origReplaceState.call(window.history, null, '', url);
+}
+
+beforeEach(() => {
+  events = [];
+  origReplaceState = window.history.replaceState.bind(window.history);
+  vi.spyOn(window.history, 'replaceState').mockImplementation(((...args: unknown[]) => {
+    events.push('parent:consumed+strip');
+    return (origReplaceState as any)(...args);
+  }) as any);
+});
+
+afterEach(() => {
+  vi.restoreAllMocks();
+  origReplaceState.call(window.history, null, '', '/');
+});
+
+/**
+ * Mounts the toolbar over the real action runtime. `held` is the caller's
+ * `user.systemPermissions` — `undefined` means the host never supplied any,
+ * which `useCapabilityGate` treats as unknown and fails OPEN on.
+ */
+function mountStack(opts: {
+  actions: any[];
+  entitlements: EnvironmentEntitlementsState | null;
+  held?: string[];
+  onUpgrade?: (spec: any) => void;
+}) {
+  const execute = vi.fn(async () => {
+    events.push('runner:execute');
+    return { success: true };
+  });
+  const view = render(
+    <I18nProvider config={{ defaultLanguage: 'en', detectBrowserLanguage: false }}>
+      <ActionProvider
+        context={{ user: { id: 'u1', ...(opts.held ? { systemPermissions: opts.held } : {}) } } as any}
+        handlers={{ api: execute as any }}
+      >
+        <EnvironmentListToolbar
+          actions={opts.actions}
+          entitlements={opts.entitlements}
+          onUpgrade={opts.onUpgrade ?? vi.fn()}
+        />
+      </ActionProvider>
+    </I18nProvider>,
+  );
+  return { execute, ...view };
+}
+
+describe('EnvironmentListToolbar — deep-link arming keys on the create action (#4123)', () => {
+  it('happy path is unchanged: create present → arms, triggers exactly once, then strips', async () => {
+    deepLink();
+    const { execute } = mountStack({ actions: [CREATE], entitlements: SETUP_PRODUCTION });
+
+    await waitFor(() => expect(execute).toHaveBeenCalledTimes(1));
+    await waitFor(() => expect(runActionParam()).toBeNull());
+    expect(execute.mock.calls[0][0]).toMatchObject({ name: 'create_environment' });
+  });
+
+  it('keeps the #3803 ordering: the runner consumes autoTrigger BEFORE the parent strips', async () => {
+    // Measured and ruled correct on #3803 (`["runner:execute","parent:consumed+strip"]`)
+    // — React flushes passive effects child-before-parent, so the button's
+    // auto-trigger effect runs first. Re-pinned here because this card moves the
+    // predicate that feeds `autoTrigger`; the ordering machinery must not move.
+    deepLink();
+    const { execute } = mountStack({ actions: [CREATE], entitlements: SETUP_PRODUCTION });
+
+    await waitFor(() => expect(execute).toHaveBeenCalledTimes(1));
+    await waitFor(() => expect(runActionParam()).toBeNull());
+    expect(events).toEqual(['runner:execute', 'parent:consumed+strip']);
+  });
+
+  it('toolbar has actions but NO create action → the param is not swallowed, nothing runs', async () => {
+    // The card's measurement, as the red→green pin: before the fix this was
+    // `urlParam=null execute=0` — the intent spent on an action that does not
+    // exist, and unrecoverable because consumption IS the strip.
+    deepLink();
+    const { execute } = mountStack({ actions: [REFRESH], entitlements: SETUP_PRODUCTION });
+
+    expect(await screen.findByText('Refresh')).toBeTruthy();
+    await waitFor(() => expect(events).toEqual([]));
+    expect(execute).not.toHaveBeenCalled();
+    expect(runActionParam()).toBe('create_environment');
+  });
+
+  it('create action filtered out by the ADR-0066 D4 capability gate → same: not swallowed', async () => {
+    // The card's candidate reachability path, measured. `toolbarActions` used to
+    // be computed from `locations` alone — before the gate — so the toolbar
+    // counted an action the bar was about to drop, and armed on it.
+    deepLink();
+    const gatedCreate = { ...CREATE, requiredPermissions: ['manage_environments'] };
+    const { execute } = mountStack({
+      actions: [REFRESH, gatedCreate],
+      entitlements: SETUP_PRODUCTION,
+      held: ['view_environments'],
+    });
+
+    expect(await screen.findByText('Refresh')).toBeTruthy();
+    // The gate really did drop it: neither the metadata label nor the
+    // state-aware override reaches the DOM.
+    expect(screen.queryByText('Set up your production environment')).toBeNull();
+    expect(screen.queryByText('Create Environment')).toBeNull();
+    await waitFor(() => expect(events).toEqual([]));
+    expect(execute).not.toHaveBeenCalled();
+    expect(runActionParam()).toBe('create_environment');
+  });
+
+  it('a capability the caller HOLDS still arms — the gate is the only thing that stops it', async () => {
+    // The mirror of the case above: same declaration, caller holds it, so the
+    // bar renders the create action and arming must go through as normal.
+    deepLink();
+    const gatedCreate = { ...CREATE, requiredPermissions: ['manage_environments'] };
+    const { execute } = mountStack({
+      actions: [REFRESH, gatedCreate],
+      entitlements: SETUP_PRODUCTION,
+      held: ['view_environments', 'manage_environments'],
+    });
+
+    await waitFor(() => expect(execute).toHaveBeenCalledTimes(1));
+    await waitFor(() => expect(runActionParam()).toBeNull());
+  });
+
+  it('unsatisfiable arming is RECOVERABLE: a later mount with the create action still runs it', async () => {
+    // Why "leave the param alone" is the honest degradation rather than a
+    // silent strip: the intent survives, so the next mount that CAN act on it
+    // does. A reload of the same URL is exactly this.
+    deepLink();
+    const first = mountStack({ actions: [REFRESH], entitlements: SETUP_PRODUCTION });
+    expect(await screen.findByText('Refresh')).toBeTruthy();
+    expect(first.execute).not.toHaveBeenCalled();
+    expect(runActionParam()).toBe('create_environment');
+    first.unmount();
+
+    const second = mountStack({ actions: [REFRESH, CREATE], entitlements: SETUP_PRODUCTION });
+    await waitFor(() => expect(second.execute).toHaveBeenCalledTimes(1));
+    await waitFor(() => expect(runActionParam()).toBeNull());
+  });
+
+  it('no toolbar action is capability-permitted → renders nothing, and still no strip', async () => {
+    deepLink();
+    const gatedCreate = { ...CREATE, requiredPermissions: ['manage_environments'] };
+    const gatedRefresh = { ...REFRESH, requiredPermissions: ['manage_environments'] };
+    const { execute, container } = mountStack({
+      actions: [gatedRefresh, gatedCreate],
+      entitlements: SETUP_PRODUCTION,
+      held: [],
+    });
+
+    expect(container.textContent).toBe('');
+    await waitFor(() => expect(events).toEqual([]));
+    expect(execute).not.toHaveBeenCalled();
+    expect(runActionParam()).toBe('create_environment');
+  });
+});
+
+describe('EnvironmentListToolbar — the non-arming affordances read the same list (#4123)', () => {
+  it('loading skeleton is held only for a create action the bar will actually render', () => {
+    // The skeleton stands in for the create button. A capability-gated create
+    // action never becomes that button, so holding its slot promises a button
+    // that can never arrive.
+    const gatedCreate = { ...CREATE, requiredPermissions: ['manage_environments'] };
+    mountStack({
+      actions: [REFRESH, gatedCreate],
+      entitlements: null,
+      held: ['view_environments'],
+    });
+
+    expect(screen.getByText('Refresh')).toBeTruthy();
+    expect(screen.queryByTestId('environment-create-cta-loading')).toBeNull();
+  });
+
+  it('upgrade CTA stands in for the create action, so a gated-out create shows none', () => {
+    // `environment-add-upgrade` is the create affordance for a plan-locked org.
+    // With no create action on the bar there is nothing for it to stand in for.
+    const gatedCreate = { ...CREATE, requiredPermissions: ['manage_environments'] };
+    const onUpgrade = vi.fn();
+    mountStack({
+      actions: [REFRESH, gatedCreate],
+      entitlements: st({ canCreateDevelopmentEnv: false, plan: 'free' }),
+      held: ['view_environments'],
+      onUpgrade,
+    });
+
+    expect(screen.getByText('Refresh')).toBeTruthy();
+    expect(screen.queryByTestId('environment-add-upgrade')).toBeNull();
+    expect(onUpgrade).not.toHaveBeenCalled();
+  });
+
+  it('…and still shows it when the caller holds the capability', () => {
+    const gatedCreate = { ...CREATE, requiredPermissions: ['manage_environments'] };
+    mountStack({
+      actions: [REFRESH, gatedCreate],
+      entitlements: st({ canCreateDevelopmentEnv: false, plan: 'free' }),
+      held: ['manage_environments'],
+    });
+
+    expect(screen.getByTestId('environment-add-upgrade')).toBeTruthy();
+  });
+});