From c8df2fbe5f38e96a3476bc861bb944b7e06a657c Mon Sep 17 00:00:00 2001 From: Thomas Camlong Date: Fri, 21 Aug 2026 17:01:29 +0200 Subject: [PATCH] fix(cli): resolve PR review feedback and improve command execution --- .gitignore | 2 + .../command-line/development/index.mdx | 4 +- package.json | 1 + tools/homarr-dev/README.md | 2 +- tools/homarr-dev/internal/cli/cli.go | 9 +++- .../internal/registry/probe_test.go | 5 +- .../internal/task/build_probe_test.go | 3 ++ tools/homarr-dev/internal/tui/actions.go | 25 ++++++++-- tools/homarr-dev/internal/tui/keys.go | 46 +++++++++++++++++-- tools/homarr-dev/internal/tui/model.go | 42 ++++++++++++----- tools/homarr-dev/internal/tui/rows.go | 4 +- tools/homarr-dev/internal/tui/view.go | 30 +++++++++++- tools/homarr-dev/internal/ui/badge.go | 5 +- tools/homarr-dev/internal/ui/theme.go | 13 ++---- 14 files changed, 149 insertions(+), 42 deletions(-) diff --git a/.gitignore b/.gitignore index 64749f817..be3e30235 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,8 @@ packages/db/migrations/*/migrate.cjs # production build +tools/homarr-dev/homarr +tools/homarr-dev/homarr-dev # misc .DS_Store diff --git a/apps/docs/docs/advanced/command-line/development/index.mdx b/apps/docs/docs/advanced/command-line/development/index.mdx index f17a754db..6f4701af8 100644 --- a/apps/docs/docs/advanced/command-line/development/index.mdx +++ b/apps/docs/docs/advanced/command-line/development/index.mdx @@ -21,7 +21,7 @@ homarr The main browser combines local `homarr:*` images with open pull requests and their GHCR preview status. Select an image to start or stop it, use `R` to build a remote pull request locally in the background or rebuild a local image, or use `p` to force a remote preview pull. Press `d` (or `1`/`2`) to toggle between the image browser and the instance dashboard. -A locally built PR image is folded into its remote PR row only when its recorded revision matches the current GitHub head SHA (marked `◆ local`). Starting it prefers that local image without a registry pull. Outdated local builds remain separate rows. +A locally built PR image is folded into its remote PR row only when its recorded revision matches the current GitHub head SHA (marked `◆ local`). When the matching GHCR image is also available, pressing `Enter` opens a local/remote selector with local preselected. Outdated local builds remain separate rows. ### Multi-Source Sidebar @@ -45,7 +45,7 @@ Pulls and builds run asynchronously in the background. Press `t` to open the tas | ---------------------- | --------------------------------------------------------------------------------------------- | | `/` | Filter rows by PR number, title, author, branch, CI, image, state, or port | | `↑` / `↓` or `j` / `k` | Navigate rows | -| `Enter` or `Space` | Start local build / pull remote PR image; stop if running | +| `Enter` or `Space` | Start the available image; choose local or remote when both exist; stop if running | | `R` | Build locally in background (or rebuild local image) | | `p` | Pull remote PR image in background and start container | | `m` | Manage mode (`b` rebuild, `i` delete image, `d` delete data, `c` remove container, `p` prune) | diff --git a/package.json b/package.json index 9c91f6cc6..347e76aa0 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ "dev": "pnpm -F nextjs dev", "dev:benchmark": "node --experimental-strip-types scripts/benchmarks/dev-board.mts", "dev:cli": "go -C tools/homarr-dev run -buildvcs=false .", + "dev:cli:build": "go -C tools/homarr-dev build -buildvcs=false .", "dev:cli:install": "go -C tools/homarr-dev install -buildvcs=false .", "dev:docs": "pnpm -F @homarr/docs dev", "docker:docs": "docker compose -f apps/workshop/docker-compose.yml up --build docs", diff --git a/tools/homarr-dev/README.md b/tools/homarr-dev/README.md index 5fc86c338..039c132cd 100644 --- a/tools/homarr-dev/README.md +++ b/tools/homarr-dev/README.md @@ -75,7 +75,7 @@ homarr doctor Check Docker, Engine API, and optional integrations ```text / Filter by PR number, title, author, branch, CI, image, state, or port ↑/↓ or j/k Select row -Enter or Space Start local build / pull remote; stop when running +Enter or Space Start the available image; choose local or remote when both exist; stop when running R Build locally in background (or rebuild local image) p Pull remote PR image in background and start container m Manage mode (chords: b rebuild, i delete image, d delete data, c remove container, p prune) diff --git a/tools/homarr-dev/internal/cli/cli.go b/tools/homarr-dev/internal/cli/cli.go index a1e895140..7866da327 100644 --- a/tools/homarr-dev/internal/cli/cli.go +++ b/tools/homarr-dev/internal/cli/cli.go @@ -5,6 +5,7 @@ package cli import ( "fmt" + "os" "github.com/spf13/cobra" @@ -36,7 +37,13 @@ func NewApp() *App { // Execute runs the CLI. func Execute() error { - return NewApp().Root().Execute() + args := os.Args[1:] + if len(args) > 0 && args[0] == "--" { + args = args[1:] + } + root := NewApp().Root() + root.SetArgs(args) + return root.Execute() } func (a *App) Root() *cobra.Command { diff --git a/tools/homarr-dev/internal/registry/probe_test.go b/tools/homarr-dev/internal/registry/probe_test.go index 02478d7d2..ce987e551 100644 --- a/tools/homarr-dev/internal/registry/probe_test.go +++ b/tools/homarr-dev/internal/registry/probe_test.go @@ -2,13 +2,14 @@ package registry import ( "context" + "os" "testing" "time" ) func TestPublishedTagsIntegration(t *testing.T) { - if testing.Short() { - t.Skip("network test") + if testing.Short() || os.Getenv("HOMARR_INTEGRATION_TEST") != "1" { + t.Skip("skipping GHCR integration test; set HOMARR_INTEGRATION_TEST=1 to run") } ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) defer cancel() diff --git a/tools/homarr-dev/internal/task/build_probe_test.go b/tools/homarr-dev/internal/task/build_probe_test.go index 634ea1437..359ceea23 100644 --- a/tools/homarr-dev/internal/task/build_probe_test.go +++ b/tools/homarr-dev/internal/task/build_probe_test.go @@ -12,6 +12,9 @@ import ( // Exercises the real BuildKit streaming and parsing path against a small // Dockerfile, so the step tracker is validated on genuine output. func TestBuildImageStreamsRealSteps(t *testing.T) { + if testing.Short() || os.Getenv("HOMARR_INTEGRATION_TEST") != "1" { + t.Skip("skipping docker build probe test; set HOMARR_INTEGRATION_TEST=1 to run") + } dir := t.TempDir() dockerfile := `FROM alpine:3.20 AS base RUN echo "hello from step two" && sleep 1 diff --git a/tools/homarr-dev/internal/tui/actions.go b/tools/homarr-dev/internal/tui/actions.go index 5fd483831..fb675383a 100644 --- a/tools/homarr-dev/internal/tui/actions.go +++ b/tools/homarr-dev/internal/tui/actions.go @@ -100,9 +100,9 @@ func (m Model) instanceTarget(row instanceRow) target { return result } -// primaryAction is Enter: stop what is running, start what is not. Starting -// prefers a local image built from the exact head commit, falls back to the -// registry image, and reports honestly when neither exists yet. +// primaryAction is Enter: stop what is running, start what is not. A matching +// local build and a published registry image open a small source selector; +// otherwise the only available source is started directly. func (m Model) primaryAction() (Model, tea.Cmd) { if m.screen == screenInstances { row, found := m.selectedInstance() @@ -126,6 +126,9 @@ func (m Model) primaryAction() (Model, tea.Cmd) { return m.startLocal(row) } if row.hasCurrentLocalImage() { + if m.remoteImageAvailable(row) { + return m.selectImage(row) + } return m.startLocalPR(row) } switch row.image { @@ -139,6 +142,18 @@ func (m Model) primaryAction() (Model, tea.Cmd) { return m.deployRemotePR(row) } +func (m Model) remoteImageAvailable(row devRow) bool { + return row.kind == rowRemote && m.tagsKnown && registry.HasPRImage(m.tags, row.pr.Number) +} + +func (m Model) selectImage(row devRow) (Model, tea.Cmd) { + m.imageSelection = imageSelection{rowKey: row.key(), choice: imageChoiceLocal} + m.mode = modeImageSelect + m.status, m.statusLevel = "choose an image source for "+row.label(), levelInfo + m.relayout() + return m, nil +} + func (m Model) startLocal(row devRow) (Model, tea.Cmd) { plan, err := run.BuildPlan(run.Options{Context: context.Background(), Tag: row.local.Tag, Demo: m.demo}) if err != nil { @@ -264,9 +279,9 @@ func (m Model) focusOn(id int, title string) (Model, tea.Cmd) { m.sidebar.follow = true m.status, m.statusLevel = title+" started in the background", levelInfo m.relayout() - m.refreshSidebar() + cmd := m.refreshSidebar() clocks := m.animate() - return m, clocks + return m, tea.Batch(cmd, clocks) } func (m Model) stopContainer(name string) (Model, tea.Cmd) { diff --git a/tools/homarr-dev/internal/tui/keys.go b/tools/homarr-dev/internal/tui/keys.go index 7bdcb3abe..61f91f8f5 100644 --- a/tools/homarr-dev/internal/tui/keys.go +++ b/tools/homarr-dev/internal/tui/keys.go @@ -19,6 +19,8 @@ func (m Model) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m.handleFilterKey(msg) case modeConfirm: return m.handleConfirmKey(msg) + case modeImageSelect: + return m.handleImageSelectKey(msg) case modeManage: return m.handleManageKey(msg) case modeHelp: @@ -30,6 +32,42 @@ func (m Model) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { } } +func (m Model) handleImageSelectKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { + switch { + case key.Matches(msg, m.keys.Cancel): + m.mode = modeNormal + m.imageSelection = imageSelection{} + m.status, m.statusLevel = "launch canceled", levelInfo + m.relayout() + return m, nil + + case key.Matches(msg, m.keys.Up), key.Matches(msg, m.keys.Down): + if m.imageSelection.choice == imageChoiceLocal { + m.imageSelection.choice = imageChoiceRemote + } else { + m.imageSelection.choice = imageChoiceLocal + } + return m, nil + + case key.Matches(msg, m.keys.Enter): + selection := m.imageSelection + row, found := m.selectedDev() + m.mode = modeNormal + m.imageSelection = imageSelection{} + if !found || row.key() != selection.rowKey { + m.status, m.statusLevel = "image selection expired; choose a row again", levelWarn + m.relayout() + return m, nil + } + m.relayout() + if selection.choice == imageChoiceRemote { + return m.deployRemotePR(row) + } + return m.startLocalPR(row) + } + return m, nil +} + func (m Model) handleNormalKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { switch { case key.Matches(msg, m.keys.Quit): @@ -245,8 +283,8 @@ func (m Model) handleFilterKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { m.filter.Reset() m.filter.Blur() m.relayout() - m.refreshSidebar() - return m, nil + cmd := m.refreshSidebar() + return m, cmd case tea.KeyEnter: m.mode = modeNormal m.filter.Blur() @@ -256,8 +294,8 @@ func (m Model) handleFilterKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { var command tea.Cmd m.filter, command = m.filter.Update(msg) m.applyFilter() - m.refreshSidebar() - return m, command + cmd := m.refreshSidebar() + return m, tea.Batch(command, cmd) } func (m Model) handleOverlayKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { diff --git a/tools/homarr-dev/internal/tui/model.go b/tools/homarr-dev/internal/tui/model.go index 2c3ba669a..1c7b59549 100644 --- a/tools/homarr-dev/internal/tui/model.go +++ b/tools/homarr-dev/internal/tui/model.go @@ -38,10 +38,23 @@ const ( modeFilter modeManage modeConfirm + modeImageSelect modeHelp modeTasks ) +type imageChoice int + +const ( + imageChoiceLocal imageChoice = iota + imageChoiceRemote +) + +type imageSelection struct { + rowKey string + choice imageChoice +} + type statusLevel int const ( @@ -96,7 +109,8 @@ type Model struct { taskCursor int focusTask int - confirm confirmation + confirm confirmation + imageSelection imageSelection includeBots bool demo bool @@ -243,8 +257,8 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.applyTasks() case logsChangedMsg: - m.refreshSidebar() - return m, waitForLogs(m.logs) + cmd := m.refreshSidebar() + return m, tea.Batch(cmd, waitForLogs(m.logs)) case ciChecksMsg: m.ciChecks[msg.pr] = ciCheckState{ @@ -253,16 +267,16 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { err: msg.err, updated: time.Now(), } - m.refreshSidebar() + cmd := m.refreshSidebar() for _, c := range msg.checks { if c.IsPending() { pollCmd := tea.Tick(6*time.Second, func(t time.Time) tea.Msg { return pollCIChecksMsg{pr: msg.pr} }) - return m, pollCmd + return m, tea.Batch(cmd, pollCmd) } } - return m, nil + return m, cmd case pollCIChecksMsg: if m.sidebar.source == sourceCI && m.selectedPRNumber() == msg.pr { @@ -274,12 +288,12 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Batch(tickRefresh(), loadContainers(m.generation)) case frameMsg: - m.refreshSidebar() + cmd := m.refreshSidebar() if !m.busy() { m.animating = false - return m, nil + return m, cmd } - return m, tickFrame() + return m, tea.Batch(cmd, tickFrame()) case spinner.TickMsg: if !m.busy() { @@ -337,8 +351,8 @@ func (m Model) applyLoaded(msg loadedMsg) (Model, tea.Cmd) { m.status, m.statusLevel = m.summary(), levelInfo } m.relayout() - m.refreshSidebar() - return m, m.syncLogStreams() + cmd := m.refreshSidebar() + return m, tea.Batch(cmd, m.syncLogStreams()) } func (m Model) summary() string { @@ -408,7 +422,9 @@ func (m Model) applyTasks() (Model, tea.Cmd) { commands = append(commands, clocks) } m.relayout() - m.refreshSidebar() + if cmd := m.refreshSidebar(); cmd != nil { + commands = append(commands, cmd) + } return m, tea.Batch(commands...) } @@ -592,6 +608,8 @@ func (m *Model) relayout() { reserved += 2 case modeConfirm: reserved += 3 + case modeImageSelect: + reserved += 5 } if m.status != "" { reserved += 2 diff --git a/tools/homarr-dev/internal/tui/rows.go b/tools/homarr-dev/internal/tui/rows.go index 916a87ab2..1c5d9e3f3 100644 --- a/tools/homarr-dev/internal/tui/rows.go +++ b/tools/homarr-dev/internal/tui/rows.go @@ -104,7 +104,7 @@ func (r devRow) searchTerms() string { // buildDevRows merges open pull requests with local images into a single list. // A local image folds into its pull request only when the revisions match, so -// the row that Enter starts is always the freshest thing available. +// the row exposes both sources when the registry image is available too. func buildDevRows(prs []gh.PR, images []docker.Image, tags map[string]bool, tagsKnown bool) []devRow { rows := make([]devRow, 0, len(prs)+len(images)) indexByPR := make(map[int]int, len(prs)) @@ -117,8 +117,8 @@ func buildDevRows(prs []gh.PR, images []docker.Image, tags map[string]bool, tags if matched && image.Revision != "" && image.Revision == rows[index].pr.HeadSHA { if rows[index].local.Tag == "" { rows[index].local = image + continue } - continue } rows = append(rows, devRow{kind: rowLocal, local: image, image: ui.ImageLocal}) } diff --git a/tools/homarr-dev/internal/tui/view.go b/tools/homarr-dev/internal/tui/view.go index bb69e671d..619ef9b24 100644 --- a/tools/homarr-dev/internal/tui/view.go +++ b/tools/homarr-dev/internal/tui/view.go @@ -28,6 +28,8 @@ func (m Model) View() tea.View { sections = append(sections, "", m.manageBar()) case modeConfirm: sections = append(sections, "", m.confirmBar()) + case modeImageSelect: + sections = append(sections, "", m.imageSelectBar()) } if m.status != "" { sections = append(sections, "", m.statusView()) @@ -208,8 +210,7 @@ func (m Model) taskOverlay(width int) string { } // detail describes the selected row in full, including the things that are too -// wide for a table cell: branch, provenance, and the exact image that Enter -// would run. +// wide for a table cell: branch, provenance, and the image source Enter can run. func (m Model) detail() string { if m.screen == screenInstances { row, found := m.selectedInstance() @@ -275,6 +276,8 @@ func (m Model) detail() string { switch { case row.running: meta = append(meta, ui.Help.Render("enter stops it")) + case row.hasCurrentLocalImage() && m.remoteImageAvailable(row): + meta = append(meta, ui.Help.Render("enter chooses local or remote")) case row.hasCurrentLocalImage(): meta = append(meta, ui.Local.Render("enter runs "+row.localReference())) case row.image == ui.ImageRemote: @@ -290,6 +293,29 @@ func (m Model) detail() string { clip(strings.Join(meta, ui.Dim.Render(" · ")), m.width) } +func (m Model) imageSelectBar() string { + row, found := m.selectedDev() + if !found { + return ui.Help.Render("image selection is no longer available") + } + + title := fmt.Sprintf("%s choose image for PR #%d", ui.IconDocker, row.pr.Number) + local := m.imageChoiceLine(imageChoiceLocal, ui.IconLocal+" local", row.localReference()) + remote := m.imageChoiceLine(imageChoiceRemote, ui.IconCloud+" remote", remoteImageFor(row)) + hint := ui.Help.Render("↑/↓ choose · enter start · esc cancel") + return strings.Join([]string{ui.Heading.Render(title), local, remote, hint}, "\n") +} + +func (m Model) imageChoiceLine(choice imageChoice, label, reference string) string { + marker := " " + style := ui.Help + if m.imageSelection.choice == choice { + marker = "▸ " + style = ui.Selected + } + return style.Render(ui.Truncate(marker+label+" "+reference, max(m.width-2, 20))) +} + // taskTray is the always-visible one-liner for the job being followed. It uses // the springy bubbles progress bar because a single headline bar reads better // when it eases towards its target. diff --git a/tools/homarr-dev/internal/ui/badge.go b/tools/homarr-dev/internal/ui/badge.go index 250aa79cb..3dd1c7831 100644 --- a/tools/homarr-dev/internal/ui/badge.go +++ b/tools/homarr-dev/internal/ui/badge.go @@ -98,8 +98,9 @@ var imageBadges = map[ImageState]badgeSpec{ ImageUnknown: {"?", "checking", Warning}, } -// ImageBadge renders image availability. Local builds win over registry builds -// because running one is instant and needs no network. +// ImageBadge renders the preferred image availability for a row. A matching +// local build is shown first; the start action can still offer the registry +// image when both sources are available. func ImageBadge(state ImageState, verbose bool) string { spec, found := imageBadges[state] if !found { diff --git a/tools/homarr-dev/internal/ui/theme.go b/tools/homarr-dev/internal/ui/theme.go index 452649287..c0c5f4979 100644 --- a/tools/homarr-dev/internal/ui/theme.go +++ b/tools/homarr-dev/internal/ui/theme.go @@ -7,6 +7,7 @@ import ( "strings" "charm.land/lipgloss/v2" + "github.com/charmbracelet/x/ansi" ) // The palette is intentionally small. Colour carries meaning here — green is @@ -69,17 +70,11 @@ func Rule(label string, width int) string { // occupy a row for no reason, which turns a log panel into double-spaced text. func Blank(line string) bool { return lipgloss.Width(strings.TrimSpace(line)) == 0 } -// Truncate shortens a string to width runes, marking the cut with an ellipsis. +// Truncate shortens a string to display width, marking the cut with an ellipsis. +// It handles ANSI escape sequences and multi-byte runes correctly. func Truncate(value string, width int) string { if width <= 0 { return "" } - runes := []rune(value) - if len(runes) <= width { - return value - } - if width == 1 { - return "…" - } - return string(runes[:width-1]) + "…" + return ansi.Truncate(value, width, "…") }