feat: Add CommandRegistry and default command descriptions. by Le0C · Pull Request #217 · MetaCell/neuroglancer

Le0C · 2026-07-20T07:58:33Z

This PR makes the command set discoverable independent of keybinds, and makes it carry human readable names/descriptions.

CommandRegistry - a global registry of commands. Each command is declared once with a stable id, label, optional description/category/defaultBinding, and availability signal. CommandInfo is an explicit discriminated union on type rather than inferring behaviour from which fields are present. Registration returns a disposer, so features can register/unregister commands over their own lifetime
default_commands.ts - Seeds the built-in commands with labels + help test.
CommandCatalog - now enumerates the registry as the authoritative comamnd list.

seankmartin

Thank you very much for the PR! I have left some thoughts which hopefully help. The most awkward one to me is whether this registry may list commands or must list commands. But happy to kind of go with our best read on that for now and discuss more in the google PR

seankmartin · 2026-08-13T18:25:34Z

+export interface CommandEntry extends CommandPaletteEntryBase {
+  readonly kind: "command";
+  readonly id: ActionIdentifier;
+  readonly invoke: () => void;


Is there a reason for having invoke here instead of in the registry? In other words, I'd suggest we add a real Command class, either in its own file or bundled into the command registry file. That would also avoid this slightly awkward duplication here of kind which is very similar to type in the registry.

The Command class could have id, label and invoke as required, and description as optional.

I think overall this is probably the biggest structural change if we agree on it, and the rest of the changes would likely fall pretty naturally from this.

Perhaps something like (open to exacts):

// Interface instead of target only in case later // the mouse position etc is useful export interface CommandContext { readonly dispatchTarget: EventTarget; } class Command { // This is intended to bundle any changes together // e.g. if the command becomes enabled/disabled readonly changed = new NullarySignal(); constructor( readonly id: CommandId, readonly label: string, readonly description?: string, // properties excluded on purpose, see other comments ) {} // Could also be called execute etc, don't feel strongly on the name invoke(context: CommandContext): void {} } class ActionCommand extends Command { invoke({ dispatchTarget }: CommandContext) { dispatchTarget.dispatchEvent( new CustomEvent(`action:${this.id}`, { bubbles: true, cancelable: true, detail: {} })); } }

It would cause some extra refactoring in the catalog but I'm happy to take that, so the main goal I think for this to PR should be to align on the registry - I'll still bring up integration points, even if they get tackled separately

I quite like the Command class you have sketched here. There is no reason invoke had to be here instead of the registry. I am happy to take a pass at implementing this, as downstream changes of adding this are pretty clear to me.

seankmartin · 2026-08-13T18:54:56Z

  selectedLayer: SelectedLayerState;
  inputEventBindings: InputEventBindings;
+  /**
+   * Authoritative source of the flat command set. Its command-kind entries are


I think we could be more accurate than this, maybe a small separate command README is worth it about how we picture each piece and the responsibilities.

Something like:

Command registry - owns what commands exist, what their ID is and name in the UI, as well as how to run the command. Imo we could have Command here or in a separate Command file.

Command catalog - owns what commands could be presented, ordering the commands, providing a shortcut for the commands, etc. Essentially providing some nice wrapping around a registry with extra convience.

Command palette - owns the UI representation of a command catalog, alllowing a user to filter commands, search commands, and execute them.

That maybe glosses over the update flow a little, but I kind of imagine the flow being that adding/removing from the registry flows to the catalog which flows to the palette

I can make some docs for this. Would the right place for these docs be docs/concepts/ ?

I also think how you have imagined the flow is also how I see it working.

seankmartin · 2026-08-13T18:55:42Z

+      }
+      const shortcut =
+        shortcutByAction.get(command.id) ??
+        (command.defaultBinding !== undefined


I suggest we remove defaultBinding

seankmartin · 2026-08-13T20:21:24Z

+   */
+  readonly description?: string;
+  /** Optional flat category / section for grouping in a host surface. */
+  readonly category?: string;


I suggest we remove this and let the "host" do this grouping. Though I think we should be consistent and consider consumer as the term for this instead of host, like on L48.

As an example, the command palette and the help menu may want to group differently. And since both would likely want to use the same registry I'm not sure about us including the categories into the registry

seankmartin · 2026-08-13T20:22:02Z

+   * concern. A command with no `defaultBinding` and no live binding is still a
+   * first-class member of the registry.
+   */
+  readonly defaultBinding?: string;


I think we should drop this, it's likely to go out of sync. Just rely on the real bind instead (so bind not part of registry)

seankmartin · 2026-08-13T20:37:26Z

+  readonly label: string;
+  /**
+   * Optional longer help text describing what the command does. Surfaced by
+   * hosts that can afford more than a label (help panel, tooltips).


Kind of prefer consumer over host but open to the alternative

seankmartin · 2026-08-13T20:38:41Z

+
+/**
+ * A command that runs a callback directly, for commands with no corresponding
+ * DOM action (e.g. host-registered commands).


Host here sounds more reasonable, I used embedder also during the review. In this case I agree though on the broader term over consumer

seankmartin · 2026-08-13T20:42:08Z

  );

+  // Global, binding-independent registry of viewer commands. Populated with the
+  // built-in commands during default viewer setup; feature code and hosts may


I think "may" here is reasonable and is what leads to my comment in the other file about dynamic entries in a catalog. Since it is may, not must, we should support filling the catalog from the registry and binds imo, with registry as precedence. Or this has to become must, which could be a pretty large change and hard to enforce since other apps using nglancer may have just inserted binds.

We can also go with our best idea here and bring anything further over to the google PR for discussion. In which case I'd lean the mixed approach for simplicity, and we discuss if the mix isn't acceptable and the registry must be authorative.

This makes sense to me. Turning this feature from "may" to "must" would indeed be a large breaking change and I have no desire to force other consumers of the library to use these features. If at some point in the future the registry is widely adopted then perhaps we can revisit the topic.

seankmartin · 2026-08-13T20:42:50Z

+ *
+ *     this.registerDisposer(
+ *       viewer.commandRegistry.registerCallback({
+ *         id: "clip.addPlane",


Minor but real actions are abc-def style

seankmartin · 2026-08-13T20:43:59Z

+const CATEGORY_STATE = "State";
+const CATEGORY_TOOLS = "Tools";
+
+const AXES = ["X", "Y", "Z"] as const;


I think these are dynamic in the help panel? worth checking

There is AXES_NAMES in util/geom.ts so I can use that instead of making it again here.

Le0C · 2026-08-14T07:21:41Z

@seankmartin Thanks for the comments, I will take a look through this in more detail today. For the smaller points everything seemed agreeable, and for the larger points (like Command class) I will take some time to read things through and come back to you.

Grouping is a presentation concern: the command palette and the help panel would reasonably group the same commands in different ways, so the section a command belongs to belongs to whoever is presenting it. A suggested binding on the command was only ever informational, and would drift from whatever binding is actually installed. The shortcut a consumer shows now always comes from the live input event bindings.

RenderedDataPanel registers its per-axis action listeners by iterating AXES_NAMES, so declaring the matching commands from a second local list of axis names left two places to keep in step. Import the same constant and fold the move and rotate generators into one pass over it.

Behaviour had nowhere to live on a plain data record, so each consumer re-derived it: the palette built the `action:<id>` CustomEvent itself, and the catalog translated the registry's `type` discriminant into its own `kind` discriminant to decide which branch to take. A Command now owns its id, label, optional description and how it runs. ActionCommand dispatches the DOM action, CallbackCommand runs a callback, and both take a CommandContext rather than a bare target so that more context (mouse position, originating layer) can be added later without touching every implementation. The registry stores instances and forwards each command's `changed` signal, which replaces the per-command WatchableValue subscription that backed the old `isAvailable`; that property is now a settable `enabled` on the command itself. The catalog's ActionCommandEntry and CommandEntry collapse into a single entry carrying the Command.

The registry lists the commands it was told about, and there is no way to make that list complete: a viewer embedded in another application, or driven from the Python integration, can bind an action without ever registering a command for it. Enumerating only the registry dropped those from the palette, which the previous catalog did show. The catalog now enumerates the registry first, so a registered command keeps its curated label and description, then adds an ActionCommand for each keyboard-bound action the registry does not know, labelled from its action id as before. Tool slots and layer-index actions stay excluded; the catalog contributes its own entries for those.

The command ids in default_commands.ts have to match the action ids the default input event bindings dispatch, and nothing checked that. A typo in either direction is silent: a command whose id no action listens for does nothing when invoked, and a bound action with no command loses its label and description. Assert both directions against the real binding maps, with the tool slots and layer-index actions excluded as dynamic, and the three actions that have no default binding listed explicitly.

Describe what each piece owns: a Command holds identity, presentation and behaviour; the registry holds which commands exist; the catalog turns that plus viewer state into an ordered list with shortcuts attached; the palette renders it. Records why the registry cannot be treated as the complete list of commands, and how a change flows from a registration through to a re-render.

seankmartin

Looks great, thank you very much for this

feat: Add CommandRegistry and default command descriptions.

d923d18

seankmartin self-requested a review August 11, 2026 18:52

seankmartin reviewed Aug 13, 2026

View reviewed changes

Le0C added 6 commits August 14, 2026 11:11

seankmartin approved these changes Aug 14, 2026

View reviewed changes

seankmartin merged commit 311fdb6 into MetaCell:feat/command-palette Aug 14, 2026
1 check passed

Uh oh!

Conversation

Le0C commented Jul 20, 2026

Uh oh!

seankmartin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Le0C commented Aug 14, 2026

Uh oh!

seankmartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants