feat: live CLI scan UX + dashboard refresh improvements#23
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds real-time scan progress reporting from the engine up to the CLI, and improves the browser dashboard’s scan refresh UX for better visibility into scan state.
Changes:
- Introduces
model.StageEventand anOptions.Progresscallback to emit stage lifecycle/progress events during scans. - Adds a live CLI renderer with a spinner and colored stage/status output wired through
cmd/macaron. - Improves the dashboard header/detail view with refresh status, a manual refresh button, and periodic auto-refresh.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ui/assets/index.html | Adds refresh status pill, manual “refresh now”, and periodic auto-refresh logic. |
| internal/model/model.go | Adds StageEventType/StageEvent models for scan progress events. |
| internal/engine/engine.go | Emits stage/target lifecycle events via a new Options.Progress callback. |
| internal/cliui/live.go | New live CLI renderer for stage lifecycle output + spinner UX. |
| internal/app/app.go | Plumbs Progress callback from app-level args into engine options. |
| cmd/macaron/main.go | Instantiates the renderer and hooks progress events into it when not --quiet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| close(r.spinStop) | ||
| r.spinnerOn = false | ||
| fmt.Fprint(r.out, "\r\033[2K") | ||
| } |
There was a problem hiding this comment.
ANSI clear-line sequences ("\r\033[2K") are written even when output is not a TTY or when NO_COLOR is set, which can pollute piped logs / redirected output (and may behave poorly on some terminals). Consider detecting terminal capability (e.g., using go-isatty which is already in go.mod) and disabling the spinner/clear-line behavior when not interactive.
| scanStart time.Time | ||
| lastPrinted time.Time | ||
| } |
There was a problem hiding this comment.
lastPrinted is written to but never read, which adds noise and makes it unclear whether throttling/printing logic is incomplete. Either remove lastPrinted or use it (e.g., to rate-limit spinner updates / avoid flicker).
| loadScans(); | ||
| setInterval(async () => { | ||
| await loadScans(); | ||
| if (activeId) await loadResult(activeId); | ||
| }, refreshMS); |
There was a problem hiding this comment.
The periodic refresh uses setInterval with an async callback and no in-flight guard. If /api/scans or /api/results is slow, calls can overlap (including with the manual refresh button) and race the UI state. Consider using a single refresh loop with an "inFlight" flag (or chaining setTimeout after completion) to ensure only one refresh runs at a time.
| loadScans(); | |
| setInterval(async () => { | |
| await loadScans(); | |
| if (activeId) await loadResult(activeId); | |
| }, refreshMS); | |
| let refreshInFlight = false; | |
| async function refreshOnce() { | |
| if (refreshInFlight) { | |
| return; | |
| } | |
| refreshInFlight = true; | |
| try { | |
| await loadScans(); | |
| if (activeId) { | |
| await loadResult(activeId); | |
| } | |
| } finally { | |
| refreshInFlight = false; | |
| } | |
| } | |
| function startRefreshLoop() { | |
| const loop = async () => { | |
| await refreshOnce(); | |
| setTimeout(loop, refreshMS); | |
| }; | |
| setTimeout(loop, refreshMS); | |
| } | |
| loadScans(); | |
| startRefreshLoop(); |
| async function loadScans() { | ||
| const res = await fetch('/api/scans'); | ||
| if (!res.ok) { | ||
| document.getElementById('pulse').textContent = 'scan index unavailable'; | ||
| return; | ||
| } | ||
| scans = await res.json(); | ||
| document.getElementById('pulse').textContent = `${scans.length} indexed scans`; | ||
| document.getElementById('lastRefresh').textContent = `updated ${new Date().toLocaleTimeString()}`; | ||
| renderScanList(scans); |
There was a problem hiding this comment.
loadScans() does not handle fetch() exceptions (e.g., network errors). With the new async setInterval refresh, this can surface as unhandled promise rejections and stop subsequent refresh logic. Wrap the fetch/json path in try/catch (similar to loadHeat) and update both "pulse" and "lastRefresh" to reflect the failure state.
| func (r *LiveRenderer) startSpinnerLocked() { | ||
| if r.spinnerOn { | ||
| return | ||
| } | ||
| r.spinnerOn = true | ||
| r.spinStop = make(chan struct{}) | ||
| go r.spin() | ||
| } |
There was a problem hiding this comment.
Spinner lifecycle has a concurrency bug: startSpinnerLocked overwrites r.spinStop and launches r.spin() which reads r.spinStop from the struct. If a new scan starts soon after a previous stop, the old goroutine can end up selecting on the new channel and never exit, resulting in multiple spinner goroutines writing to the same output. Fix by capturing the stop channel in a local variable and passing it into spin(stopCh) (and selecting on that channel), so each goroutine listens to the channel it was created with.
Summary
Validation