layout change ui fix

src/components/ui-editor/UiEditor.tsx

@@ -1,88 +1,61 @@
 import React from 'react';
-import { RotateCcw, SlidersHorizontal, Undo2 } from 'lucide-react';
+import { RotateCcw, Undo2 } from 'lucide-react';
 import { Button } from '@/components/ds';
 import { useUiEditing } from '@/components/ui-tree/UiEditingProvider';
-import { inspectTree } from '@/lib/ui/inspect';
-import { nodeRegistry } from '@/lib/ui/registry';
-import { TreePanel } from './TreePanel';
-import { NodeInspector } from './NodeInspector';
-import { NodePicker } from './NodePicker';
 
 /**
- * The visual editor.
+ * The preview bar for a layout change.
  *
  * It is a *client* of the UI system, not a second implementation of it. Every
- * control it draws ends in one call — `propose(op)` on the editing session —
- * with an operation object of exactly the shape Owliver produces for the same
- * change. From there the two are indistinguishable: same validation, same
- * preview merge, same Apply, same `preferences.uiLayouts`.
+ * control it draws ends in one call on the editing session, with an operation
+ * of exactly the shape Owliver produces for the same change. From there the two
+ * are indistinguishable: same validation, same preview merge, same Apply, same
+ * `preferences.uiLayouts`.
  *
- * That is the whole architecture:
+ *     Owliver  →  operation  →  propose  →  validate  →  preview
+ *                                              ↓ Apply
+ *                                      preferences.uiLayouts
  *
- *     editor / Owliver  →  operation  →  propose  →  validate  →  preview
- *                                                      ↓ Apply
- *                                              preferences.uiLayouts
+ * WHAT THIS USED TO BE, AND WHY IT IS NOT
  *
- * There is no page in this file, no component name, and no branch on what a
- * node is. What can be done to the selected node comes from its registration;
- * what can be added comes from the registry and the closed data vocabulary;
- * what it is showing comes from the tree. A page that migrates tomorrow is
- * editable tomorrow with nothing here changed.
+ * There was a `Customise layout` button here, and behind it a tree panel, a
+ * node inspector and a picker — a visual editor sitting beside Owliver, both
+ * producing the same operations. It was removed on request: two ways to
+ * rearrange a page is two things to explain, and the panel was on every admin
+ * screen whether or not anybody was arranging anything.
+ *
+ * Owliver is now the only way to propose a layout change. This component keeps
+ * the other half of that conversation — a person still has to SEE the change
+ * and decide. `TreePanel`, `NodeInspector` and `NodePicker` are left in the
+ * directory, unimported: they are the editor, intact, if it is ever wanted
+ * back.
+ *
+ * Nothing about applying moved. `useAssistant` calls `apply` and `discard` on
+ * the session directly, so "Apply" typed into the panel works exactly as the
+ * button does — see the `ui-apply` branch there.
  */
-export function UiEditor({ registry = nodeRegistry }) {
+export function UiEditor() {
   const editing = useUiEditing();
-  const [open, setOpen] = React.useState(false);
-  const [selectedId, setSelectedId] = React.useState(null);
 
   /* Rendered only where a page has opted into composition. A page that has not
      is not broken; it simply has nothing to arrange. */
   if (!editing) return null;
 
   const {
-    tree, propose, discard, apply, undo, reset,
+    discard, apply, undo, reset,
     previewing, customised, saving, problems, skipped,
-    /* The page this session belongs to. Handed to the picker so what can be
-       added here is decided by the registry rather than by the picker being
-       shown everything that exists. */
-    page,
   } = editing;
 
-  const nodes = inspectTree(tree, { registry });
-  const selected = nodes.find((node) => node.id === selectedId) || null;
-  /* A node's own container, for the reorder buttons and for the picker. */
-  const siblings = selected ? nodes.filter((node) => node.parent === selected.parent) : [];
-  const parent = selected?.container ? selected : nodes.find((n) => n.id === selected?.parent) || null;
-
-  /**
-   * The one door out of this component.
-   *
-   * Everything the panels do arrives here as an operation and goes straight to
-   * the session. Nothing is applied, nothing is stored, and nothing is
-   * validated locally — `propose` refuses what cannot be kept and the refusal
-   * is shown below.
-   */
-  const operate = (op) => {
-    const result = propose(op);
-    /* A removed node cannot stay selected; a replaced one keeps its id. */
-    if (result.ok && op.op === 'remove') setSelectedId(null);
-  };
+  /* Nothing proposed and nothing saved means nothing to say. Without this the
+     component drew an empty bar on every page, which is most of what the button
+     was being blamed for. */
+  if (!previewing && !customised && !problems.length && !skipped.length) return null;
 
   return (
     <div data-ui-controls="editor" className="space-y-2">
-      <div className="flex flex-wrap items-center justify-between gap-2">
-        <Button
-          size="xs"
-          variant={open ? 'default' : 'outline'}
-          shape="rounded"
-          onClick={() => setOpen((v) => !v)}
-        >
-          <SlidersHorizontal className="mr-1.5 h-3.5 w-3.5" aria-hidden="true" />
-          Customise layout
-        </Button>
-
-        {/* The preview bar. Unsaved and saved have to be told apart at a
-            glance, because the whole promise is that nothing is kept until
-            somebody says so. */}
+      <div className="flex flex-wrap items-center justify-end gap-2">
+        {/* Unsaved and saved have to be told apart at a glance, because the
+            whole promise is that nothing is kept until somebody says so. */}
         {(previewing || customised) && (
           <div className="flex flex-wrap items-center gap-2">
             {previewing && (
@@ -122,25 +95,6 @@ export function UiEditor({ registry = nodeRegistry }) {
           {skipped.length} saved change{skipped.length === 1 ? '' : 's'} no longer apply to this page.
         </p>
       )}
-
-      {open && (
-        <div className="grid gap-3 rounded-xl border border-border bg-surface-subtle p-3 lg:grid-cols-2">
-          <div className="space-y-3">
-            <TreePanel tree={tree} selectedId={selectedId} onSelect={setSelectedId} registry={registry} />
-            <NodePicker tree={tree} parent={parent} page={page} onAdd={operate} registry={registry} />
-          </div>
-
-          <div className="rounded-xl border border-border bg-surface p-3">
-            {selected
-              ? <NodeInspector node={selected} siblings={siblings} page={page} onOperate={operate} registry={registry} />
-              : (
-                <p className="text-caption text-ink-3">
-                  Choose a section on the left to see what can be changed about it.
-                </p>
-              )}
-          </div>
-        </div>
-      )}
     </div>
   );
 }

src/components/ui-tree/UiNodeBoundary.tsx

@@ -88,7 +88,7 @@ export class UiNodeBoundary extends React.Component<UiNodeBoundaryProps, UiNodeB
           {node?.props?.title || 'This section could not be shown'}
         </p>
         <p className="mt-0.5 text-caption text-ink-4">
-          It is still on the page and can be hidden or removed from Customise layout.
+          It is still on the page. Ask Owliver to hide it.
         </p>
       </section>
     );

src/lib/authReturnTo.ts

@@ -0,0 +1,133 @@
+/**
+ * Where an interrupted flow resumes after sign-in.
+ *
+ * Kept in one module because it is security-sensitive and easy to drift: this is
+ * the only place that decides whether a URL somebody else supplied may be
+ * navigated to.
+ *
+ * WHO SETS ?returnTo=, AND WHY THIS EXISTS
+ *
+ * The API does. `GET /oauth/authorize` is the browser leg of the MCP OAuth
+ * flow, and it needs a signed-in person to show a consent screen to. When
+ * nobody is signed in it redirects to the configured login path, carrying its
+ * own path and query so the authorization request survives the round trip:
+ *
+ *     /admin/login?returnTo=%2Foauth%2Fauthorize%3Fclient_id%3D...%26state%3D...
+ *
+ * Without this module the person signs in, lands on the dashboard, and the
+ * authorization request is gone — the connector can never finish.
+ *
+ * WHY AN ALLOWLIST RATHER THAN "ANY SAME-ORIGIN PATH"
+ *
+ * Only two kinds of destination are reachable this way, and they are reached
+ * differently, so naming them is both safer and necessary:
+ *
+ *   /oauth/authorize   a BACKEND route. React Router has no such path, so
+ *                      routing to it client-side renders the not-found page and
+ *                      the request never reaches the server. It needs a real
+ *                      browser navigation.
+ *   /admin/...         an in-app route, which is what the existing
+ *                      `location.state.from` mechanism already carries. Router
+ *                      navigation, exactly as before.
+ *
+ * Anything else has no business arriving in this parameter, and the narrower
+ * rule means a future backend route cannot be reached through here by accident.
+ *
+ * THE SAME-ORIGIN CHECK IS NOT ENOUGH ON ITS OWN
+ *
+ * Values like `/.//evil.example` and `/\evil.example` resolve same-origin and
+ * then normalise to a protocol-relative `//evil.example` when assigned to
+ * location — an open redirect through a check that appeared to pass. So the
+ * resolved path must also begin with exactly one slash and contain no
+ * backslash. The allowlist below would catch these anyway; both are kept
+ * because each is load-bearing on its own, and a later edit that loosens the
+ * allowlist must not silently lose the other.
+ *
+ * WHY THE QUERY STRING IS PASSED THROUGH UNTOUCHED
+ *
+ * `url.pathname + url.search` is the original text, byte for byte. Nothing here
+ * reads, rewrites or re-serialises the parameters, and that is deliberate:
+ * touching `url.searchParams` at all makes the browser re-encode the whole
+ * query, which can turn `%20` into `+` inside `scope`, or re-spell the
+ * percent-encoding of `redirect_uri` and `resource`. Every one of those is a
+ * value the authorization server compares EXACTLY — `code_challenge` against
+ * the verifier, `redirect_uri` against the registered URI, `state` against what
+ * the client sent. A re-encoded query is a different query, and the failure
+ * would surface much later as a mismatched PKCE challenge.
+ */
+
+/** How the destination has to be reached. */
+export type ReturnVia = 'browser' | 'router';
+
+export interface ReturnTarget {
+  /** A path on this origin, with its query preserved exactly. */
+  path: string;
+  /**
+   * 'browser' for a backend route, which must bypass React Router.
+   * 'router' for an in-app route, which must not reload the page.
+   */
+  via: ReturnVia;
+}
+
+/** The backend route the OAuth browser leg returns to. */
+const OAUTH_AUTHORIZE_PATH = '/oauth/authorize';
+
+/** The login itself, which would sign a person in and show them the login. */
+const LOGIN_PATH = '/admin/login';
+
+/** The in-app console. Everything under it is a React Router destination. */
+const ADMIN_PREFIX = '/admin';
+
+/**
+ * Resolve a `returnTo` query parameter to a safe destination.
+ *
+ * @param search The query string to read, defaulting to the document's. The
+ *   login page passes the router's value explicitly so this never reaches for a
+ *   global it does not need.
+ * @returns The destination and how to reach it, or null when there is nothing
+ *   safe to return to — in which case the caller keeps its existing behaviour.
+ */
+export function safeReturnTo(search?: string): ReturnTarget | null {
+  if (typeof window === 'undefined') return null;
+
+  const raw = new URLSearchParams(search ?? window.location.search).get('returnTo');
+  if (!raw) return null;
+
+  let url: URL;
+  try {
+    url = new URL(raw, window.location.origin);
+  } catch {
+    // Malformed. Nothing safe to do with it.
+    return null;
+  }
+
+  // Cross-origin, and anything carrying a scheme of its own — javascript:,
+  // data:, https://evil.example — fails here: the resolved origin is not ours.
+  if (url.origin !== window.location.origin) return null;
+
+  const path = url.pathname + url.search;
+
+  // Exactly one leading slash, no backslash. See the note above on why the
+  // origin check does not cover this. The backslash test also reaches values
+  // the allowlist below would have accepted — a backslash survives unencoded
+  // into `search`, so `/admin/x?next=/\evil.example` is on the allowlist and
+  // refused only here. The cost is a legitimate `state` containing a raw
+  // backslash being refused too; that fails closed, landing the person on the
+  // dashboard rather than anywhere an attacker chose.
+  if (!path.startsWith('/') || path.startsWith('//') || path.includes('\\')) return null;
+
+  if (url.pathname === OAUTH_AUTHORIZE_PATH) {
+    return { path, via: 'browser' };
+  }
+
+  // The login is refused before the admin prefix is considered, because it sits
+  // underneath it.
+  if (url.pathname === LOGIN_PATH) return null;
+
+  if (url.pathname === ADMIN_PREFIX || url.pathname.startsWith(ADMIN_PREFIX + '/')) {
+    return { path, via: 'router' };
+  }
+
+  // Everything else: not a destination this parameter is for.
+  return null;
+}

src/lib/ui/intent.ts

@@ -37,6 +37,22 @@ const canon = (value) => String(value ?? '')
 
 const has = (text, ...words) => words.some((w) => text.includes(canon(w)));
 
+/**
+ * Whole-word match, for words that live inside longer ones.
+ *
+ * `has` is a substring test. That is right for phrases and wrong for short
+ * words: `over` is inside `coverage` and `overtime`, `end` is inside `trends`
+ * and `calendar`, `up` is inside `group`. Read as substrings, "move coverage to
+ * the bottom" matched both a top reading and a bottom one — and the top is
+ * tested first, so it moved the opposite way from the one asked for.
+ *
+ * Single words only. A phrase is unambiguous as a substring and stays on `has`.
+ */
+const hasWord = (text, ...words) => {
+  const tokens = new Set(String(text ?? '').split(' '));
+  return words.some((w) => tokens.has(canon(w)));
+};
+
 /**
  * The verbs, as data.
  *
@@ -443,8 +459,27 @@ function planUnhide(text, tree, registry) {
 function planMove(text, tree, registry) {
   const roots = tree.map((node) => describeNode(node, { registry }));
 
-  const before = has(text, 'above', 'before', 'over', 'on top of', 'to the top', 'first');
-  const after = has(text, 'below', 'under', 'beneath', 'after', 'to the bottom', 'last', 'end');
+  /**
+   * Where it should end up.
+   *
+   * The phrase list is long because people say this a dozen ways and the ones
+   * that were missing were the short ones: "move it to top" was refused while
+   * "move it to the top" worked, which reads as the feature being broken rather
+   * than as a phrasing it does not know. A destination is the whole of what
+   * this function needs, so failing to recognise one costs the entire request.
+   *
+   * Short words go through `hasWord` — see its note. `top` and `bottom` alone
+   * are enough on their own here: by the time a sentence has reached planMove
+   * it is already a move, and a bare "top" in a move is a destination.
+   */
+  const before = has(text, 'on top of', 'to the top', 'to top', 'to the very top',
+    'at the top', 'to the start', 'to the front', 'right to the top')
+    || hasWord(text, 'above', 'before', 'over', 'top', 'topmost', 'first', 'up', 'upward', 'upwards');
+
+  const after = has(text, 'to the bottom', 'to bottom', 'to the very bottom',
+    'at the bottom', 'to the end', 'to the back', 'right to the bottom')
+    || hasWord(text, 'below', 'under', 'underneath', 'beneath', 'after', 'bottom',
+      'last', 'end', 'down', 'downward', 'downwards');
 
   /* Split on the positional word so the two halves name two different nodes:
      "move timeline above the notice" is a subject and a reference. */

src/pages/admin/Login.tsx

@@ -5,6 +5,7 @@ import { cn } from '@/lib/utils';
 import { KROW_LOGO_URL } from '@/assets/brand';
 import { Checkbox } from '@/components/ds';
 import { useAuth } from '@/lib/AuthContext';
+import { safeReturnTo } from '@/lib/authReturnTo';
 
 const DEMO_EMAIL = 'demo@krow.app';
 
@@ -26,6 +27,24 @@ export default function AdminLogin() {
   const location = useLocation();
   const { login } = useAuth();
 
+  /* Two kinds of "where was I going", and they are not interchangeable.
+
+     A ?returnTo= in the QUERY is put there by the API, not by this app. The
+     OAuth authorization endpoint redirects here when nobody is signed in,
+     carrying its own path and query so the authorization request survives the
+     round trip. It names a route on the BACKEND (/oauth/authorize), which React
+     Router does not have and must not be given — routing to it client-side
+     renders the not-found page and the connector never finishes. So it is
+     followed with a real navigation, which safeReturnTo reports as `browser`.
+
+     `location.state.from` is the in-app case: a guard bounced someone off a
+     page in this bundle (ProtectedRoute and AdminRoute both set it). That is a
+     router destination and stays one, unchanged.
+
+     The query wins when both exist. It is the more specific instruction, and it
+     is the one the person is actually in the middle of. */
+  const fromQuery = safeReturnTo(location.search);
+
   const from = location.state?.from;
   const returnTo = typeof from === 'string' && from.startsWith('/admin') && from !== '/admin/login'
     ? from
@@ -79,7 +98,13 @@ export default function AdminLogin() {
     }
 
     setStatus('success');
-    setTimeout(() => navigate(returnTo, { replace: true }), 320);
+    setTimeout(() => {
+      // `replace`, not `assign`: the login should not sit in history between
+      // the authorization request and the consent screen, or Back from consent
+      // returns to a login the person has already completed.
+      if (fromQuery?.via === 'browser') window.location.replace(fromQuery.path);
+      else navigate(fromQuery?.path ?? returnTo, { replace: true });
+    }, 320);
   };
 
   const busy = status !== 'idle';