PT-4532: Menu section headings and keyboard shortcut hints

[katherinejensen00]

Summary

Menus built from contributed menu data now show each column's label as a section heading, and a menu item shows the keyboard shortcut that runs its command.

RTL:

LTR:

Two chords get hints today — Find (Ctrl+F) and Insert comment (Ctrl+Shift+N). Insert footnote and Insert cross-reference deliberately get none: their chords only work in Standard view, and a hint that lies is worse than no hint.

62 files, but 14 are generated platform-bible-react/dist — skip those. The real change is about 20 source files; the rest are tests, stories and docs.

Where to look

Read these three first — they are the whole design:

File	What it does
src/extension-host/services/menu-data.service-host.ts	The join. Stamps shortcut onto every menu it serves, for the OS it is running on.
src/shared/data/keyboard-shortcuts.data.ts	The source of truth. Moved here from src/stories/; an entry's new optional command is what opts it into a hint.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx	The headings. Renders each non-empty column as a labeled section.

Then, if you want the rest:

File	What it does
src/shared/utils/keyboard-shortcut-hint.util.ts	Looks up a command's chord and picks the right OS + first alternative.
lib/platform-bible-utils/.../menus.model.ts	Adds MenuItemContainingCommand.shortcut. Display-only, @experimental, and the schema rejects it from menus.json on purpose.
lib/platform-bible-react/.../menus/menu.util.ts	getMenuSectionsWithItems / getSortedMenuColumns — which columns become sections, in what order.
src/shared/utils/menu-document-combiner.ts	Drive-by bug fix: web view menu localization was never awaited, so a heading could render as a raw %localize_key%. One-line spread fix + regression test.
shadcn-ui/{dropdown-menu,context-menu,menubar}.tsx	One class each so hints keep their key order in RTL.
src/shared/data/keyboard-shortcuts.data.test.ts	Pins each hint's text per OS and which menus show it, so reusing a command can't silently add a hint somewhere new.

Three decisions worth your scrutiny

1. Why the extension host does the join. The scripture editor's Project menu renders inside the editor's iframe, from an extension bundle that cannot import src/shared — so the lookup can never reach it. Anything the renderer knows and the iframe doesn't has to arrive as data, and menu data is the pipe it already travels down. It is also the only choke point that covers all four menu surfaces at once.

2. Section headings are opt-in (showSectionHeadings, default off). TabDropdownMenu is exported from platform-bible-react and its docs used to say column labels were ignored. TabToolbar / TabFloatingMenu turn it on, so the app gets headings while out-of-repo consumers are unaffected.

3. Two catalog entries are outside the ticket's scope. The structure-lock shortcuts were missing from the catalog; they are accurate and verified against the handler, but they do widen the diff.

Also in here

• The Reference → Keyboard shortcuts page now renders one Kbd per key in a KbdGroup instead of one Kbd per whole combination, with a tested helper for the catalog's per-OS spellings.
• That change follows the guidance in docs: reconcile Kbd/KbdGroup keyboard-shortcut rendering guidance #2583 (@merchako), which is unmerged and unlikely to be picked up. Rather than ship code contradicting our own guideline page, its MDX is included here with Alex credited as co-author — so docs: reconcile Kbd/KbdGroup keyboard-shortcut rendering guidance #2583 should be closed rather than merged. Two fixes were needed: the catalog's new path, and its relative .mdx links, which don't resolve in built Storybook.

Known limits (deliberate, documented in the ADRs)

• Hints are per-OS, never per-view — so view-gated chords get none.
• The unlocalized main menu (native macOS menubar) gets none; that needs an Electron accelerator, not a string.
• Key names are English, not localized — PT-4629 tracks that, with a TODO at the lookup.
• Third-party extensions can't declare hints; command is typed against this repo's CommandNames.

Testing

• platform-bible-react unit suite: 671 passing. Root suites touched: 108 passing.
• typecheck:e2e clean; build:types regenerates papi.d.ts byte-identical; build:pbr clean.
• Manually verified in-app on Windows: section headings, both hints, the empty-column case, the app menubar (no headings, as designed), and RTL (hints keep key order and move to the inline end).
• Not verified: macOS ⌘⌥M. If macOS claims it system-wide, the Insert comment entry should drop its command.

One honest caveat: the 'column' in group guard in menu.util.ts is a TypeScript narrowing, not a runtime gate — removing it produces identical output for every input, so no test can pin it. The compiler is what protects it. Its sibling guard is pinned.

Risk

Low-to-moderate. Additive: menus that have no matching catalog entry render exactly as before. The behavior change is visual (headings + hints) and gated behind an opt-in prop for library consumers.

AI Involvement

AI-assisted — session. Claude Code drafted the implementation, the review fixes and this description; reviewed by the author.

🤖 Generated with Claude Code

---

This change is 

[mattgetgen]

Review — PT-4532 menu section headings and shortcut hints

Reviewed 92d77b5b0d0...3ee579b5f44 (62 files), skipping the generated lib/**/dist/** and lib/papi-dts/papi.d.ts — I spot-checked those three to confirm they were regenerated consistently (showSectionHeadings, MenuItemContainingCommand.shortcut and the unicode-bidi class are all present).

The change is in good shape, and the menu-document-combiner.ts spread is a real latent-bug fix rather than tidy-up — web view menus were never awaited, so a column label could be served as a raw %localize_key%, which is exactly what the new headings would have put on screen.

Three findings, all low-to-medium and none blocking. Each has a proposed patch inline; I applied and tested all three locally, including the revert test, so the details below are measured rather than guessed.

Checked and found sound

• parseShortcutKeycaps against every string in the catalog: ⌘F8, ⌃Space, ⌘], Ctrl++, Ctrl+-, Alt+←, ↑ / ↓ / ← / →, \, — (no equivalent) all split correctly, and no combination or alternative produces duplicate React keys.
• The unicode-bidi: plaintext reasoning in the three shadcn components is right: it changes the bidi paragraph direction without changing direction, so ms-auto still resolves to the inline end in RTL.
• Both new command joins match their handlers (⌥⌘M / Ctrl+Shift+N at platform-scripture-editor.web-view.tsx:2279-2285; ⌃F / Ctrl+F via useOpenFindShortcut, mounted in all four editor/panel web views), and the two new structure-protection-button entries match the (ctrlKey||metaKey)+shift(±alt)+L handler.
• addShortcutHints never mutates the cached menu objects, so getUnlocalizedMainMenu genuinely stays hint-free — worth confirming, since the constructor aliases mainMenu and unlocalizedMainMenu to the same array.
• Every bundled renderer of MenuItemContainingCommand now shows shortcut (menubar, tab dropdown, tab-title context menu, overlay converter); a grep across lib/platform-bible-react/src, src/renderer and extensions/src found no missed site.
• ADR slugs are in correct LC_ALL=C byte order. @shared/* resolves in Storybook via TsconfigPathsPlugin. The only stale reference to the old src/stories/keyboard-shortcuts.data.ts path is in a historical doc under .context/designs/.

If you take the menu.util.ts patch

platform-bible-react ships through its committed dist/, and the root npm run build does not rebuild it — the fix needs npm run build:pbr and a committed dist/ to actually ship, and CI cannot see that skew.

Verification

With all three patches applied: menu.util / tab-dropdown-menu / platform-menubar 17 passing, and keyboard-shortcut-hint.util / keyboard-shortcuts.data / keyboard-shortcut-keycaps.util / keyboard-shortcuts-catalog.component / menu-data.service-host 72 passing. ESLint and tsc clean on every touched file.

Two things I could not verify, so you can weigh them yourself. My worktree resolves node_modules from a checkout of a different branch, so a repo-wide npm run typecheck reports pre-existing shortcut does not exist errors out of a stale platform-bible-utils/dist — unrelated to this PR, and the same artifact makes one assertion in platform-tab-title.move-menu.test.tsx fail there while passing against this branch's own dist. And the E2E specs are comment-only changes that I did not run.

[mattgetgen]

The comment says the (\s|$) anchor keeps this locator from selecting "Find and replace…", but it does not: /^find(\s|$)/i matches "Find and replace…", because \s matches the space right after Find. Playwright tests the regex against the whole accessible name, so if such an item is ever added to this hamburger menu the locator resolves to two elements and the click fails with a strict-mode violation — the precise case the comment claims is handled.

The anchor's real effect is narrower: it excludes a longer single word, e.g. "Finder". That is worth keeping, so the smallest honest fix is to say what it actually does. replace.spec.ts:104 carries the identical comment and pattern and needs the same edit.

Suggested change

	const findMenuItem = editorFrame.getByRole('menuitem', { name: /^find(\s|$)/i });
	// The item's accessible name is its label "Find" (%webView_platformScriptureEditor_openFind%)
	// followed by a keyboard shortcut hint, so the match cannot be anchored to the end of the name.
	// The trailing `(\s|$)` rules out only a longer single word, e.g. "Finder"; a sibling item whose
	// label begins "Find " — "Find and replace…" — matches too, and the click then fails with a
	// strict-mode violation rather than opening the wrong panel. That failure is the signal to narrow
	// this locator to the label element instead of the whole accessible name.
	const findMenuItem = editorFrame.getByRole('menuitem', { name: /^find(\s|$)/i });

The alternative is to tighten the pattern so it genuinely excludes the sibling — something like /^find(\s+\S*[⌃⌘⇧⌥+]\S*)?$/i. I would not: it encodes an assumption about the hint's spelling, which this PR has already changed once, and the strict-mode violation is a loud failure rather than a silent wrong-item click.

[katherinejensen00]

Done, in both find.spec.ts and replace.spec.ts, using your wording.

[mattgetgen]

getShortcutHintForCommand returns splitShortcutAlternatives(keys)[0] with no handling for the catalog's no-equivalent marker, so an entry whose keys for one OS read — (no equivalent) would render that literal string in the menu's shortcut slot on that OS.

Nothing triggers it today, but the catalog already carries such an entry (tab-context-menu, macOS), and keyboard-shortcuts.model.ts invites exactly this combination — it forbids a command only for focus-blind and view-gated chords, not for a chord one OS simply lacks. keyboard-shortcut-keycaps.util.ts special-cases the same marker via NO_EQUIVALENT_PREFIX; this function is the other reader of keys and should agree.

Suggested fix — move the marker here so there is one definition, and rule it out:

Suggested change

	return splitShortcutAlternatives(keys)[0];
	// An OS with no equivalent for the chord gets no hint, rather than a menu row reading
	// `— (no equivalent)` where a chord belongs
	if (startsWith(keys, NO_EQUIVALENT_PREFIX)) return undefined;
	return splitShortcutAlternatives(keys)[0];

That needs startsWith added to the platform-bible-utils import, and this exported alongside splitShortcutAlternatives:

/**
 * Marker an operating system's `keys` uses when that OS has no equivalent for the shortcut, e.g. `—
 * (no equivalent)`. It is prose for the catalog page, so a reader of `keys` must rule it out before
 * treating the string as a chord.
 */
export const NO_EQUIVALENT_PREFIX = '—';

with keyboard-shortcut-keycaps.util.ts importing it from here instead of declaring its own copy. The @returns line wants widening too: "or the entry has no chord on this platform".

A regression test that fails without the guard (expected '— (no equivalent)' to be undefined):

it('gives no hint on an operating system the catalog marks as having no equivalent', () => {
  expect(
    getShortcutHintForCommand('platform.openBookChapterControl', 'darwin', ENTRIES),
  ).toBeUndefined();
  expect(getShortcutHintForCommand('platform.openBookChapterControl', 'win32', ENTRIES)).toBe(
    'Shift+F10',
  );
});

[katherinejensen00]

Done, with the marker moved here and the keycaps util importing it. One change: the test uses platform.openUserSettings, since the entry's command has to be a real PAPI command. I confirmed it fails without the guard.

[mattgetgen]

This decides "does the column have items" with only the column half of the predicate, but the renderer uses both halves. getGroupContent (tab-dropdown-menu.component.tsx:47) also renders any group whose key equals the column key:

('column' in group && group.column === columnOrSubMenuKey) || key === columnOrSubMenuKey

So for menu data where a column key coincides with a group key — a column ext.tools alongside a group keyed ext.tools — the two disagree: getGroupContent would render that group's items under the column, but getMenuSectionsWithItems reports the column as empty, so the whole section is dropped and the items disappear with no heading and no separator. Before this PR that column rendered; after it, it does not.

I scanned src/extension-host/data/menu.data.json and every extensions/src/*/contributions/menus.json and found zero column/group key overlaps, so this is latent, not live. It is cheap to remove though — the two sites just need one predicate:

/**
 * Whether a group's items render under `columnOrSubMenuKey`: either the group names it as its
 * `column`, or the group is the one keyed by it, which is how a submenu addresses its own group.
 *
 * `TabDropdownMenu` picks a column's groups with this and {@link getMenuSectionsWithItems} decides
 * which columns have something to show with it, so "this column renders nothing" can never mean two
 * different things.
 */
export function isGroupUnderColumnOrSubMenu(
  groupKey: string,
  group: Localized<MenuGroupDetailsInColumn | MenuGroupDetailsInSubMenu>,
  columnOrSubMenuKey: string,
): boolean {
  return (
    ('column' in group && group.column === columnOrSubMenuKey) || groupKey === columnOrSubMenuKey
  );
}

with this function becoming:

Suggested change

	'column' in group && groupKeysWithItems.has(groupKey) ? [group.column] : [],
	export function getMenuSectionsWithItems(menuData: Localized<MultiColumnMenu>): MenuSection[] {
	const groupKeysWithItems = new Set(menuData.items.map((item) => item.group));
	const groupsWithItems = Object.entries(menuData.groups).filter(([groupKey]) =>
	groupKeysWithItems.has(groupKey),
	);
	return getSortedMenuColumns(menuData.columns)
	.filter(({ columnKey }) =>
	groupsWithItems.some(([groupKey, group]) =>
	isGroupUnderColumnOrSubMenu(groupKey, group, columnKey),
	),
	)
	.map(({ columnKey, column }) => ({ columnKey, label: column.label }));
	}

and getGroupContent's filter collapsing to .filter(([key, group]) => isGroupUnderColumnOrSubMenu(key, group, columnOrSubMenuKey)).

Your existing "gives no section of its own to a submenu group" test still passes — a submenu group key is not a column key, so that case is unaffected. Two tests pin the new behavior, and both go red against the current predicate; the component one fails with Unable to find an accessible element with the role "group" and name "Tools", which is the user-visible symptom:

it('gives a section to a column that a group of the same key fills, as the menu renders it', () => {
  // `TabDropdownMenu` renders a group whose KEY is the column key under that column, so a column
  // filled only that way still has something to show
  const sections = getMenuSectionsWithItems({
    ...MENU_WITH_SUBMENU,
    groups: {
      ...MENU_WITH_SUBMENU.groups,
      'platformScriptureEditor.tools': {
        order: 2,
        menuItem: 'platformScriptureEditor.unusedMenuItem',
      },
    },
    items: [
      ...MENU_WITH_SUBMENU.items.filter(
        (item) => item.group !== 'platformScriptureEditor.inventories',
      ),
      {
        label: 'Open Markers Inventory…',
        localizeNotes: '',
        group: 'platformScriptureEditor.tools',
        order: 1,
        command: 'platformScripture.openMarkersInventory',
      },
    ],
  });

  expect(sections.map(({ columnKey }) => columnKey)).toEqual([
    'platform.app',
    'platformScriptureEditor.tools',
  ]);
});

[katherinejensen00]

Done. getMenuSectionsWithItems and TabDropdownMenu now both use isGroupUnderColumnOrSubMenu, and I rebuilt dist. I confirmed the new test fails against the old check.