Skip to content

Default app name from .miren/app.toml for CLI commands#562

Merged
evanphx merged 2 commits intomainfrom
mir-643-any-cli-command-that-expects-an-app-should-default
Feb 3, 2026
Merged

Default app name from .miren/app.toml for CLI commands#562
evanphx merged 2 commits intomainfrom
mir-643-any-cli-command-that-expects-an-app-should-default

Conversation

@evanphx
Copy link
Contributor

@evanphx evanphx commented Jan 30, 2026

Summary

  • Make app delete, route set, and route set-default commands default to the app name from .miren/app.toml when not provided as a positional argument
  • Maintains backwards compatibility - positional arguments still work as before
  • Improves UX when running commands from within an app directory

Test plan

  • make lint passes
  • hack/it ./cli/commands/... tests pass
  • Manual testing in dev environment:
    • Create .miren/app.toml with name = "testapp"
    • Run m route set example.com (should use testapp)
    • Run m route set-default (should use testapp)
    • Run m app delete (should prompt for testapp)

Make app delete, route set, and route set-default commands default to
the app name from .miren/app.toml when not provided as a positional
argument. This maintains backwards compatibility while improving UX
when running commands from within an app directory.

Commands updated:
- app delete: app name is now optional
- route set: app name (position 1) is now optional
- route set-default: app name is now optional
@evanphx evanphx requested a review from a team as a code owner January 30, 2026 18:46
@coderabbitai
Copy link

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

Three CLI command files (app_delete.go, route_set.go, route_set_default.go) were changed to make the application name optional. The options structs now use AppName without the required:"true" tag. Each command attempts to derive the app name from AppName or by loading configuration via appconfig; if unresolved, they return an "app is required" error. Code paths, lookups, error messages, and final output were updated to use the resolved app name and appconfig was imported.


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

Copy link

@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: 3

🤖 Fix all issues with AI agents
In `@cli/commands/app_delete.go`:
- Around line 17-21: The current code calls appconfig.LoadAppConfig() and then
accesses ac.Name without ensuring ac is non-nil, which can panic because
LoadAppConfig may return (nil, nil); update the check in the app deletion logic
to verify ac != nil before referencing ac.Name (i.e., change the condition to
ensure err == nil && ac != nil && ac.Name != "" so appName is only read when ac
is not nil).

In `@cli/commands/route_set_default.go`:
- Around line 16-20: The code in route_set_default.go assumes
appconfig.LoadAppConfig() always returns a non-nil config and dereferences
ac.Name, causing a nil pointer panic when LoadAppConfig returns (nil, nil);
update the conditional that sets appName to first verify ac != nil and ac.Name
!= "" (e.g., if ac, err := appconfig.LoadAppConfig(); err == nil && ac != nil &&
ac.Name != "" { appName = ac.Name }) so you never access ac.Name when ac is nil;
keep the LoadAppConfig call and error handling as-is but add the ac != nil check
around the ac.Name access.

In `@cli/commands/route_set.go`:
- Around line 17-21: LoadAppConfig() can return (nil, nil) and the current code
dereferences ac.Name causing a panic; update the conditional that assigns
appName to ensure ac is not nil before accessing ac.Name (i.e., only set appName
when err == nil && ac != nil && ac.Name != ""), mirroring the nil-check used in
the app_delete logic and using the same symbols LoadAppConfig, ac, and appName
to locate and fix the check.
🧹 Nitpick comments (1)
cli/commands/app_delete.go (1)

11-24: Consider extracting common app name resolution logic.

The same fallback pattern (opts → appconfig → error) is duplicated in app_delete.go, route_set.go, and route_set_default.go. A shared helper function would reduce duplication and ensure consistent behavior.

♻️ Example helper function
// In a shared location, e.g., cli/commands/appname.go
func resolveAppName(explicit string) (string, error) {
	if explicit != "" {
		return explicit, nil
	}
	if ac, err := appconfig.LoadAppConfig(); err == nil && ac != nil && ac.Name != "" {
		return ac.Name, nil
	}
	return "", fmt.Errorf("app is required")
}

@phinze
Copy link
Contributor

phinze commented Jan 30, 2026

^^ Maybe LoadAppConfig() should return an empty config object instead of nil when nothing's there?

LoadAppConfig() returns (nil, nil) when no .miren/app.toml is found.
Add nil check before accessing ac.Name to prevent panic.
@evanphx
Copy link
Contributor Author

evanphx commented Feb 3, 2026

^^ Maybe LoadAppConfig() should return an empty config object instead of nil when nothing's there?

I'm thinking we do a pass at the app config api writ large.

@evanphx evanphx added this pull request to the merge queue Feb 3, 2026
Merged via the queue into main with commit 6f16a69 Feb 3, 2026
9 checks passed
@evanphx evanphx deleted the mir-643-any-cli-command-that-expects-an-app-should-default branch February 3, 2026 00:31
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