Skip to content

Add CommandGroup metadata for grouped help rendering#14

Merged
phinze merged 3 commits into
mainfrom
phinze/mir-845-command-group-provider
May 18, 2026
Merged

Add CommandGroup metadata for grouped help rendering#14
phinze merged 3 commits into
mainfrom
phinze/mir-845-command-group-provider

Conversation

@phinze
Copy link
Copy Markdown
Contributor

@phinze phinze commented May 18, 2026

Working on MIR-845 (de-emphasizing server commands in the miren CLI's help) we wanted to organize the top-level command listing into named groups like "Getting started", "Monitoring your app", "Server operations", etc. The plan we settled on with Evan is that mflags owns parsing and command-tree data while the calling CLI owns presentation, so this PR just wires up the data side.

Adds a CommandGroupProvider interface that commands can implement to declare their group, a WithCommandGroup option for the built-in funcCommand, and a Group field on ChildEntry and CommandDoc so iterating the tree returns the group along with the rest of the metadata. getDirectChildren becomes exported (GetDirectChildren) since callers will use it to walk the tree themselves, and Dispatcher.Name() is exposed so callers rendering their own help text don't need to re-plumb the program name.

mflags' built-in help rendering doesn't use any of this. It still produces the same flat listing it always did. The runtime side of MIR-845 (in mirendev/runtime) is what actually consumes these fields and renders the grouped output with lipgloss.

Callers want to organize commands into named groups in their own help
output (for example, separating "Getting started" commands from
"Server operations" in a CLI's top-level listing). mflags had no way
to carry that information through.

Adds a CommandGroupProvider interface, a WithCommandGroup option, a
Group field on ChildEntry and CommandDoc, and exports GetDirectChildren
so callers can iterate the command tree and read group metadata. Also
exposes Dispatcher.Name() so callers rendering their own help can use
the same program name without re-plumbing it.

Strictly additive plumbing. mflags' own built-in help rendering still
ignores Group; rendering decisions stay with the caller.
@phinze phinze requested a review from a team as a code owner May 18, 2026 20:42
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 10aadde9-e9fc-4d4c-a068-4c982d36c2d1

📥 Commits

Reviewing files that changed from the base of the PR and between 626d988 and 50a3de1.

📒 Files selected for processing (5)
  • fromstruct.go
  • help.go
  • help_doc.go
  • mflags.go
  • mflags_test.go

📝 Walkthrough

Walkthrough

This PR adds command grouping metadata and hidden-flag support. Commands can declare a group via CommandGroupProvider and WithCommandGroup; groups are stored on funcCommand and exposed in ChildEntry via the exported GetDirectChildren. Help rendering and command docs (HelpDoc/HelpJSON) include command group info. FromStruct gains a hidden tag parsed into Flag.Hidden; hidden flags are skipped by WriteFlagHelp and included as Hidden in FlagDoc. Dispatcher.Name() was also added and tests cover group and hidden-flag behaviors.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
dispatcher.go (1)

697-710: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Normalize parentPath in exported GetDirectChildren.

Line 697 exposes this as a public API, but unlike other public path-based methods it doesn’t normalize whitespace. External callers can get inconsistent/empty results for logically equivalent paths (e.g., trailing or repeated spaces).

Suggested fix
 func (d *Dispatcher) GetDirectChildren(parentPath string) []ChildEntry {
+	parentPath = normalizeCommandPath(parentPath)
 	children := make(map[string]*ChildEntry)

 	for path, entry := range d.commands {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dispatcher.go` around lines 697 - 710, GetDirectChildren is a public API that
currently uses parentPath as-is, causing inconsistent results for logically
equivalent inputs with extra/trailing/repeated spaces; normalize parentPath at
the start of GetDirectChildren by trimming whitespace and collapsing consecutive
internal spaces (e.g., via strings.Fields + strings.Join) so " a  b " becomes "a
b", then use that normalized parentPath for the existing prefix checks against
d.commands and remainder calculation; ensure the empty or whitespace-only
parentPath becomes "" to preserve the existing empty-root behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dispatcher_test.go`:
- Around line 1716-1718: The tests currently assert values from the map "groups"
using direct indexing (e.g., groups["deploy"]) which will succeed with zero
value even if the key is missing; change each such assertion to first check key
existence (use the comma-ok pattern to get value,ok := groups["deploy"]) and
assert that ok is true, then assert.Equal the expected value (e.g., "" or
"Operator") against value; update all occurrences that use groups["deploy"],
groups["server"], and groups["debug"] (and the similar lines around 1756-1758)
to this two-step presence-then-value pattern to ensure the tests validate both
presence and content.
- Around line 1768-1769: The test currently does a whitespace-sensitive
substring check on the JSON by converting data to json and calling
assert.Contains with `"group": "Operator"`; instead, unmarshal data (the byte
slice named data) into a concrete struct or map (e.g., var obj
map[string]interface{} or a struct with Group string) and assert the group field
directly (e.g., assert.Equal(t, "Operator", obj["group"] or obj.Group) to avoid
formatting/whitespace fragility and ensure a robust assertion.

---

Outside diff comments:
In `@dispatcher.go`:
- Around line 697-710: GetDirectChildren is a public API that currently uses
parentPath as-is, causing inconsistent results for logically equivalent inputs
with extra/trailing/repeated spaces; normalize parentPath at the start of
GetDirectChildren by trimming whitespace and collapsing consecutive internal
spaces (e.g., via strings.Fields + strings.Join) so " a  b " becomes "a b", then
use that normalized parentPath for the existing prefix checks against d.commands
and remainder calculation; ensure the empty or whitespace-only parentPath
becomes "" to preserve the existing empty-root behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 24dfc3e5-0c7a-438b-a2a2-7b4b6919e305

📥 Commits

Reviewing files that changed from the base of the PR and between 9b4f3df and 391790b.

📒 Files selected for processing (3)
  • dispatcher.go
  • dispatcher_test.go
  • help_doc.go

Comment thread dispatcher_test.go
Comment thread dispatcher_test.go
phinze added 2 commits May 18, 2026 16:04
Normalize parentPath in GetDirectChildren to match the convention of
other public path-based methods. Tests for group propagation now check
key presence explicitly instead of relying on map zero values.
Discovered while bumping mflags in mirendev/runtime: a deprecated
server flag had been tagged hidden:"yes" with the intent of keeping
it parseable but out of --help output. mflags didn't recognize the
tag, so it had been silently doing nothing on main, and our newer
strict tag validation made the latent omission visible.

Adds a Hidden bool to Flag, threads it through FromStruct (accepting
"yes", "true", or "1"), drops hidden flags from WriteFlagHelp, and
surfaces it in FlagDoc/JSON output so callers rendering their own
help can honor it too.
@phinze phinze merged commit 0ca7adc into main May 18, 2026
2 checks passed
phinze added a commit to mirendev/runtime that referenced this pull request May 18, 2026
Prospect feedback flagged that miren's top-level help dumped a flat
list of 30+ commands, with server-administration commands sitting
right next to deploy and rollback. For someone who only ever deploys
an app, that's a lot of noise to navigate around.

Reorganizes top-level help into five named groups (Getting started,
Monitoring your app, Configuring your app, Client operations, Server
operations) so deployers see what they need first and the operator
surface is still discoverable but visually separated. Headers render
in bold yellow via lipgloss; sub-command counts dim.

The rendering moved into the runtime layer. mflags now just exposes
group metadata via the new CommandGroupProvider interface (see the
companion PR mirendev/mflags#14), and the runtime walks the command
tree and styles output itself. Same model means future help-rendering
polish doesn't keep pulling presentation logic back into mflags.

A drift-prevention test catches new top-level commands that forget to
declare a group, so the grouping stays complete as the CLI grows.

Closes MIR-845
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants