Repository navigation
refactor(up): inject the navigation menu instead of importing cmd/formatter - #14324
Conversation
…matter pkg/compose imported cmd/formatter for the interactive keyboard menu of a foreground up, so the library depended on the CLI. Define a NavigationMenu interface (and a factory) in pkg/compose, satisfied by formatter.LogKeyboard, and have the CLI inject it with the new WithNavigationMenu option. The terminal code stays in cmd/formatter. Without the option no menu is displayed. formatter.Feature moves to api.Feature so that both sides share the type, and a depguard rule now forbids pkg/ and internal/ from importing cmd/. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
1 medium-severity finding in the new navigation menu injection introduced by this PR.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
pkg/compose still imported the keyboard library: it grabbed the terminal, forwarded raw key events to the menu and released the terminal on exit. The NavigationMenu now takes care of all of it through Open, Run and Close, so the library no longer knows about keys or the terminal, and the menu interface no longer exposes a third-party event type. As a side effect, key handling no longer shares a goroutine with the signal handling loop of `up`. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since f66220634a42 (the navigation-menu inversion base); earlier commits were already reviewed.
Assessment: 🟡 NEEDS ATTENTION
Two moderate-confidence findings in the new Open/Run/Close keyboard lifecycle added to cmd/formatter/shortcut.go. Both are conditional on unverifiable eiannone/keyboard library behaviour (the module source is not in the local cache), but the defensive fixes cost nothing and make the documented intent code-level explicit.
keyboard.Close closes the channel handed out by GetKeys. Run kept reading zero-value events from it until its context was done. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 51c8bbaa5f0c.
Assessment: 🟢 APPROVE
The single change in this incremental diff — switching the channel receive in Run() to the two-return form and returning when ok is false — is the correct, idiomatic Go fix for the closed-channel spin loop identified in the prior review. No new issues introduced.
3 participants
What this PR does, in one sentence
Stops the Compose library (
pkg/compose) from importing the CLI's terminal code: the interactive menu ofcompose upbecomes an interface the library drives and the CLI injects.Context
pkg/composeimportedcmd/formatterto build the keyboard menu shown during a foregroundcompose up. So the SDK depended on the CLI package, and nothing prevented more such imports from creeping in. The menu is terminal rendering, so it belongs with the CLI, but the library has to drive it duringup.What the PR brings
NavigationMenuinterface and a factory inpkg/compose, implemented byformatter.LogKeyboard, and aWithNavigationMenuoption onNewComposeService.cmd/main.goinjects the formatter implementation, so the terminal code stays incmd/formatterandcompose upbehaves exactly as before.pkg/composeandinternal/no longer importcmd/, and a lint rule keeps it that way.NavigationMenu: truemust now provide an implementation through the option; without it no menu is displayed and a warning says so.The injected menu also owns the terminal: it grabs the keyboard, reads the keys and gives the terminal back, so the library neither imports the keyboard library nor sees raw key events. The interface is just what
upneeds from a menu: decorate the logs, enable watch and detach, then run until the session ends.🤖 Generated with Claude Code