fix(app-shell): Studio's nav save leaves out entries with no target, and its banner clears on the next landed save (objectui#11776)

.changeset/11776-nav-placeholder-save.md

@@ -0,0 +1,20 @@
+---
+'@object-ui/app-shell': patch
+---
+
+Studio's navigation editor no longer fails every autosave while an "Add nav item" entry is still unbound, and its error banner goes once a nav save lands (objectui#11776).
+
+"Add nav item" adds an entry before its target is picked, and the spec refuses an entry that names no
+target for its type: an `object` entry needs an `objectName`. The save sent it anyway, so each autosave
+failed with "navigation.N.objectName — Invalid input" until the entry was bound or removed. The save now
+leaves out every entry that names no target for its type, at every depth of the tree, and sends the rest
+in the editor's order with their ids unchanged. A `group` is always sent. The unbound entry stays on the
+editor's canvas, in its place, and is sent there once its target is picked; until then the editor counts
+it as an unsaved edit, so leaving the page warns before it is dropped.
+
+The banner a failed nav save raises now clears when a later nav save lands. It is held apart from a
+failure the open page or dashboard is showing, which a nav save leaves in place.
+
+A nav entry's remove control is now reachable from the keyboard: it is in the tab order after its card,
+shows on keyboard focus as it does on hover, and Enter or Space removes the entry. It was mounted only
+while a pointer hovered the card.

packages/app-shell/src/views/metadata-admin/previews/AppNavCanvas.navPayload-11776.test.tsx

@@ -0,0 +1,215 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * objectui#11776 — two halves of the nav canvas.
+ *
+ * 1. `navPayloadOf`: the navigation a save sends. It leaves out every entry
+ *    that names no target for its `type`, at every depth, and nothing else.
+ *    "No target" is read per branch from the spec: the parity pin below walks
+ *    every member of `NavigationItemSchema`'s discriminator and holds that the
+ *    save leaves an entry out exactly when the spec refuses it for want of a
+ *    target, and keeps it once the target is set. A `group` is never left out.
+ *
+ * 2. The row's remove control is reachable by keyboard. It was mounted only
+ *    while a mouse hovered the card, so Tab never reached it. Now it is always
+ *    in the tab order, shown on hover and on focus, and Enter or Space removes
+ *    the entry. Whether it is VISIBLE when focused is a stylesheet question no
+ *    DOM double answers: the classes are pinned here, and the rendered result
+ *    was read once in Chromium for the pull request — nothing here re-reads it.
+ */
+import '@testing-library/jest-dom/vitest';
+import * as React from 'react';
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { render, screen, cleanup, within } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { NavigationItemSchema } from '@objectstack/spec/ui';
+
+import { AppNavCanvas, navPayloadOf } from './AppNavCanvas';
+
+afterEach(cleanup);
+
+/**
+ * Every `type` the spec's navigation union discriminates on, read off the
+ * schema itself, so a branch the spec adds is a branch this suite judges.
+ */
+function specNavTypes(): string[] {
+  const union = (NavigationItemSchema as unknown as { _zod: { def: { getter: () => { options: Array<{ shape: { type: { _zod: { def: { values: string[] } } } } }> } } } })._zod.def.getter();
+  return union.options.flatMap((o) => o.shape.type._zod.def.values);
+}
+
+/** The entry the spec takes for each branch: its target set, and nothing else that the branch needs. */
+const BOUND: Record<string, Record<string, unknown>> = {
+  object: { objectName: 'account' },
+  page: { pageName: 'home' },
+  dashboard: { dashboardName: 'pipeline' },
+  report: { reportName: 'won_deals' },
+  url: { url: 'https://example.com' },
+  action: { actionDef: { actionName: 'export_all' } },
+  component: { componentRef: 'metadata:directory' },
+  doc: { book: 'crm_manual' },
+  separator: {},
+  group: { children: [] },
+};
+
+const bare = (type: string): Record<string, unknown> =>
+  type === 'group' ? { id: 'nav_probe', type, children: [] } : { id: 'nav_probe', type };
+
+describe('navPayloadOf — what a nav save sends (objectui#11776)', () => {
+  it('judges every branch the spec declares', () => {
+    expect(Object.keys(BOUND).sort()).toEqual(specNavTypes().sort());
+  });
+
+  it.each(Object.keys(BOUND))('`%s`: left out exactly when the spec refuses it for want of a target', (type) => {
+    const entry = bare(type);
+    const specTakes = NavigationItemSchema.safeParse(entry).success;
+    expect(navPayloadOf([entry])).toEqual(specTakes ? [entry] : []);
+  });
+
+  it.each(Object.keys(BOUND))('`%s`: kept once its target is set, which the spec then takes', (type) => {
+    const entry = { ...bare(type), ...BOUND[type] };
+    expect(NavigationItemSchema.safeParse(entry).success).toBe(true);
+    expect(navPayloadOf([entry])).toEqual([entry]);
+  });
+
+  it('only the separator and the group are taken bare; every other branch names a target', () => {
+    const takenBare = specNavTypes().filter((type) => NavigationItemSchema.safeParse(bare(type)).success);
+    expect(takenBare.sort()).toEqual(['group', 'separator']);
+  });
+
+  it('a `doc` entry names its target by `doc` OR `book`', () => {
+    const byDoc = { id: 'nav_guide', type: 'doc', doc: 'crm_lead_guide' };
+    const byBook = { id: 'nav_help', type: 'doc', book: 'crm_manual' };
+    expect(navPayloadOf([byDoc, byBook])).toEqual([byDoc, byBook]);
+  });
+
+  it('an `action` entry names its target by `actionDef.actionName`', () => {
+    expect(navPayloadOf([{ id: 'nav_run', type: 'action', actionDef: {} }])).toEqual([]);
+    expect(navPayloadOf([{ id: 'nav_run', type: 'action', actionDef: { actionName: 'run_it' } }])).toHaveLength(1);
+  });
+
+  it('an entry with no `type` — what an unbind leaves — is left out, as it was before', () => {
+    expect(navPayloadOf([{ id: 'nav_item_3', label: 'Orders' }, { id: 'nav_item_4', type: undefined }])).toEqual([]);
+  });
+
+  it('the boundary is the spec’s: a target key holding any string is sent, and the server judges it', () => {
+    const empty = { id: 'nav_blank', type: 'object', objectName: '' };
+    expect(NavigationItemSchema.safeParse(empty).success).toBe(true);
+    expect(navPayloadOf([empty])).toEqual([empty]);
+  });
+
+  it('a `type` the spec does not declare is sent: its refusal is the server’s to make, by name', () => {
+    const view = { id: 'nav_view', type: 'view', objectName: 'account' };
+    expect(NavigationItemSchema.safeParse(view).success).toBe(false);
+    expect(navPayloadOf([view])).toEqual([view]);
+  });
+
+  it('an unbound entry inside a group is left out, and the group is kept', () => {
+    const bound = { id: 'nav_accounts', type: 'object', objectName: 'account' };
+    const group = {
+      id: 'nav_sales',
+      type: 'group',
+      label: 'Sales',
+      children: [{ id: 'nav_item_5', type: 'object' }, bound, { id: 'nav_item_6' }],
+    };
+    expect(navPayloadOf([group])).toEqual([{ ...group, children: [bound] }]);
+    // The group's own document is untouched: only what is sent loses the child.
+    expect(group.children).toHaveLength(3);
+  });
+
+  it('a group that loses every child is still sent, with no children', () => {
+    const group = { id: 'nav_sales', type: 'group', label: 'Sales', children: [{ id: 'nav_item_5', type: 'object' }] };
+    const [sent] = navPayloadOf([group]);
+    expect(sent).toEqual({ ...group, children: [] });
+    expect(NavigationItemSchema.safeParse(sent).success).toBe(true);
+  });
+
+  it('nesting is judged at every depth, an `object` entry’s nested views included', () => {
+    const view = { id: 'nav_open_tasks', type: 'object', objectName: 'task', viewName: 'open' };
+    const object = { id: 'nav_tasks', type: 'object', objectName: 'task', children: [view, { id: 'nav_item_7', type: 'page' }] };
+    const group = { id: 'nav_work', type: 'group', label: 'Work', children: [object] };
+    expect(navPayloadOf([group])).toEqual([{ ...group, children: [{ ...object, children: [view] }] }]);
+  });
+
+  it('the sent array is the editor’s, less the entries with no target, in order and with every id kept', () => {
+    const home = { id: 'nav_home', type: 'page', label: 'Home', pageName: 'home' };
+    const sep = { type: 'separator' };
+    const accounts = { id: 'nav_accounts', type: 'object', objectName: 'account' };
+    const editor = [home, { id: 'nav_item_2', type: 'object' }, sep, accounts, { id: 'nav_item_5', type: 'report' }];
+    const sent = navPayloadOf(editor);
+    expect(sent).toEqual([home, sep, accounts]);
+    // The entries sent ARE the editor's: nothing kept is copied or rewritten.
+    expect(sent[0]).toBe(home);
+    expect(sent[1]).toBe(sep);
+    expect(sent[2]).toBe(accounts);
+  });
+
+  it('the control: a navigation whose every entry names a target is sent as it is, entry for entry', () => {
+    const nav = [
+      { id: 'nav_home', type: 'page', label: 'Home', pageName: 'home' },
+      { id: 'nav_sales', type: 'group', label: 'Sales', children: [{ id: 'nav_accounts', type: 'object', objectName: 'account' }] },
+      { id: 'nav_help', type: 'doc', book: 'crm_manual' },
+    ];
+    const sent = navPayloadOf(nav);
+    expect(sent).toEqual(nav);
+    sent.forEach((entry, i) => expect(entry).toBe(nav[i]));
+  });
+});
+
+describe('AppNavCanvas — the remove control is reachable by keyboard (objectui#11776)', () => {
+  const NAV = [
+    { id: 'nav_home', type: 'page', label: 'Home', pageName: 'home' },
+    { id: 'nav_item_2', type: 'object' },
+  ];
+
+  function renderCanvas() {
+    const onPatch = vi.fn();
+    render(<AppNavCanvas draft={{ navigation: NAV }} rootKey="navigation" onPatch={onPatch} selection={null} />);
+    return onPatch;
+  }
+
+  /** The card for `label`: the row button whose text starts with it. */
+  const card = (label: string) =>
+    screen.getAllByRole('button').find((b) => b.getAttribute('aria-pressed') !== null && b.textContent?.startsWith(label))!;
+
+  it('every row carries it with no pointer over the card, and it has an accessible name', () => {
+    renderCanvas();
+    expect(screen.getAllByRole('button', { name: 'Remove nav item' })).toHaveLength(NAV.length);
+    expect(within(card('Home')).getByRole('button', { name: 'Remove nav item' })).toHaveAttribute('tabindex', '0');
+  });
+
+  it('Tab reaches it straight after its card, and it is shown on focus as on hover', async () => {
+    const user = userEvent.setup();
+    renderCanvas();
+    const home = card('Home');
+    const remove = within(home).getByRole('button', { name: 'Remove nav item' });
+    home.focus();
+    await user.tab();
+    expect(remove).toHaveFocus();
+    // Hidden it takes no room; the card's hover, the card's keyboard focus and
+    // its own keyboard focus each show it at full size.
+    expect(remove.className.split(/\s+/)).toEqual(
+      expect.arrayContaining([
+        'opacity-0',
+        'w-0',
+        'group-hover:opacity-100',
+        'group-hover:w-6',
+        'group-focus-visible:opacity-100',
+        'group-focus-visible:w-6',
+        'focus-visible:opacity-100',
+        'focus-visible:w-6',
+      ]),
+    );
+    // The card is the `group` those variants read.
+    expect(home.className.split(/\s+/)).toContain('group');
+  });
+
+  it.each([['Enter', '{Enter}'], ['Space', ' ']])('%s on it removes that entry, and only that one', async (_name, keys) => {
+    const user = userEvent.setup();
+    const onPatch = renderCanvas();
+    const placeholder = card('nav_item_2');
+    within(placeholder).getByRole('button', { name: 'Remove nav item' }).focus();
+    await user.keyboard(keys);
+    expect(onPatch).toHaveBeenCalledTimes(1);
+    expect(onPatch).toHaveBeenCalledWith({ navigation: [NAV[0]] });
+  });
+});

packages/app-shell/src/views/metadata-admin/previews/AppNavCanvas.tsx

@@ -4,8 +4,8 @@
  * AppNavCanvas — form-canvas-style editor for an App's top-level
  * navigation tree. Each nav entry becomes a card with a drag handle,
  * a kind icon, an inline-rename label, the metadata record it targets,
- * and a remove affordance on hover. Drag-drop reorders within the root
- * list.
+ * and a remove affordance shown on hover or keyboard focus. Drag-drop
+ * reorders within the root list.
  *
  * Kind and target are read from the spec's discriminated union — `type`
  * plus that branch's own target key — never inferred from off-spec keys
@@ -210,34 +210,87 @@ function navLabel(it: RawNav, i: number, locale: string, targetLabel: NavTargetL
   return tFormat('engine.appNav.item', locale, { n: i + 1 });
 }
 
+/**
+ * Where each target-bearing branch of the spec's `NavigationItemSchema` names
+ * its target: the branch's own key, or keys in the order they are read.
+ * `group` and `separator` declare none — they open nothing, so neither is
+ * ever unbound. One table, two readers: what a card shows (`navTarget`) and
+ * what a save leaves out (`navPayloadOf`), so the two cannot disagree about
+ * which key is the target.
+ */
+const NAV_TARGET_READS = new Map<string, ReadonlyArray<(it: RawNav) => unknown>>([
+  ['object', [(it) => it.objectName]],
+  ['page', [(it) => it.pageName]],
+  ['dashboard', [(it) => it.dashboardName]],
+  ['report', [(it) => it.reportName]],
+  ['url', [(it) => it.url]],
+  ['component', [(it) => it.componentRef]],
+  ['action', [(it) => it.actionDef?.actionName]],
+  // The page it opens, else the book (objectui#11197).
+  ['doc', [(it) => it.doc, (it) => it.book]],
+]);
+
 /**
  * The metadata record this entry names, read from the key its own branch
  * declares. Replaces a `path ?? href ?? route ?? url` chain in which only
  * `url` was ever a real key — and only on `type: 'url'`.
  */
 function navTarget(it: RawNav): string | undefined {
-  const pick = (v: unknown) => (typeof v === 'string' && v ? v : undefined);
-  switch (it.type) {
-    case 'object':
-      return pick(it.objectName);
-    case 'page':
-      return pick(it.pageName);
-    case 'dashboard':
-      return pick(it.dashboardName);
-    case 'report':
-      return pick(it.reportName);
-    case 'url':
-      return pick(it.url);
-    case 'component':
-      return pick(it.componentRef);
-    case 'action':
-      return pick(it.actionDef?.actionName);
-    case 'doc':
-      // The page it opens, else the book (objectui#11197).
-      return pick(it.doc) ?? pick(it.book);
-    default:
-      return undefined;
+  const reads = typeof it.type === 'string' ? NAV_TARGET_READS.get(it.type) : undefined;
+  for (const read of reads ?? []) {
+    const v = read(it);
+    if (typeof v === 'string' && v) return v;
+  }
+  return undefined;
+}
+
+/**
+ * Whether a save leaves this entry out (objectui#11776): it has no `type` (the
+ * placeholder an unbind leaves behind), or its `type` is a target-bearing
+ * branch and none of that branch's target keys holds a string — the entry the
+ * spec refuses for want of a target ("navigation.N.objectName — expected
+ * string, received undefined"). A key that holds a string is a target the
+ * spec takes, so the save sends it and the server judges it.
+ */
+function namesNoTarget(it: RawNav): boolean {
+  if (typeof it.type !== 'string') return true;
+  const reads = NAV_TARGET_READS.get(it.type);
+  return !!reads && !reads.some((read) => typeof read(it) === 'string');
+}
+
+/**
+ * The navigation a save sends (objectui#11776): the editor's entries, in their
+ * order, less every entry that names no target for its `type`, at every depth.
+ * A `group` is kept whatever its children are.
+ *
+ * *Add nav item* births an entry before its target is picked, and the editor
+ * keeps showing it so that it can be bound where it was added; only what is
+ * sent leaves it out. An entry that is kept, and a `children` list that loses
+ * nothing, is returned as the same object, so a caller can tell by reference
+ * whether anything was left out.
+ *
+ * Exported for the Studio nav save (`StudioDesignSurface`'s `doNavSave`) and
+ * its pins: it lives here so that it reads the card's own target table. It is
+ * a pure function, not a component, and it never reaches the package entry
+ * (`index.ts` re-exports a named list, and `package.json` exports only `.`).
+ */
+// eslint-disable-next-line react-refresh/only-export-components -- see above
+export function navPayloadOf(entries: readonly unknown[]): Array<Record<string, unknown>> {
+  const sent: Array<Record<string, unknown>> = [];
+  for (const entry of entries) {
+    if (!entry || typeof entry !== 'object' || Array.isArray(entry)) continue;
+    const it = entry as RawNav;
+    if (namesNoTarget(it)) continue;
+    const kids = it.children;
+    if (!Array.isArray(kids)) {
+      sent.push(it);
+      continue;
+    }
+    const children = navPayloadOf(kids);
+    const kept = children.length === kids.length && children.every((c, i) => c === kids[i]);
+    sent.push(kept ? it : { ...it, children });
   }
+  return sent;
 }
 
 export interface AppNavCanvasProps {
@@ -532,7 +585,6 @@ function NavCard({
   const authored = navItemLabelText(item.label, locale).trim();
   const [editing, setEditing] = React.useState(false);
   const [draft, setDraft] = React.useState(authored);
-  const [hover, setHover] = React.useState(false);
   const [dropPos, setDropPos] = React.useState<'before' | null>(null);
 
   React.useEffect(() => {
@@ -571,8 +623,6 @@ function NavCard({
           onDropBefore();
         }}
         onClick={onClick}
-        onMouseEnter={() => setHover(true)}
-        onMouseLeave={() => setHover(false)}
         aria-pressed={isSelected}
         className={`group flex w-full items-center gap-2 rounded-md border bg-card px-2.5 py-2 text-left text-xs transition-colors hover:border-primary/40 ${
           isSelected ? 'border-primary ring-1 ring-primary' : 'border-border'
@@ -629,7 +679,12 @@ function NavCard({
             {target}
           </code>
         )}
-        {canEdit && hover && !editing && (
+        {/* objectui#11776 — always in the tab order, shown on hover AND on
+            keyboard focus (the card's, or its own). It was mounted only while
+            a mouse hovered the card, so no keyboard path could reach it.
+            Hidden, it takes no room (no width, and its margin gives back the
+            row's gap), so the card lays out as it did when it was absent. */}
+        {canEdit && !editing && (
           <span
             role="button"
             tabIndex={0}
@@ -644,7 +699,7 @@ function NavCard({
                 onRemove();
               }
             }}
-            className="inline-flex h-6 w-6 items-center justify-center rounded text-muted-foreground hover:bg-destructive/10 hover:text-destructive"
+            className="-ml-2 inline-flex h-6 w-0 items-center justify-center overflow-hidden rounded text-muted-foreground opacity-0 transition-opacity hover:bg-destructive/10 hover:text-destructive group-hover:ml-0 group-hover:w-6 group-hover:opacity-100 group-focus-visible:ml-0 group-focus-visible:w-6 group-focus-visible:opacity-100 focus-visible:ml-0 focus-visible:w-6 focus-visible:opacity-100 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring"
             aria-label={t('engine.appNav.removeItem', locale)}
           >
             <Trash2 className="h-3 w-3" />