Enrich JSON, extend sampling intervals, show refresh hint#23
Conversation
Reviewer's GuideThis PR enriches one-shot JSON output for scripting, extends the available sampling intervals, and adds a transient UI hint that shows the active refresh rate when the user changes it. Sequence diagram for refresh hint when adjusting sampling intervalsequenceDiagram
actor User
participant Model
participant Sampler
participant Runtime
User->>Model: handleKey(Faster)
activate Model
Model->>Model: adjustRefreshInterval(true)
Note over Model: updates m.refreshInterval
Model->>Sampler: sampler.New(col, m.refreshInterval)
Sampler-->>Model: smp (started via smp.Run)
Model->>Model: set m.refreshHint
Model-->>Runtime: tea.Tick(2s, func(t) refreshHintClearMsg)
deactivate Model
Runtime-->>Model: refreshHintClearMsg
activate Model
Model->>Model: Update(refreshHintClearMsg)
Model->>Model: m.refreshHint = ""
deactivate Model
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR enriches ChangesJSON output and refresh hint updates
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant View as dashboardContentLines
User->>Model: press + or - key
Model->>Model: adjustRefreshInterval returns tea.Cmd
Model->>Model: set refreshHint to "every <interval>"
Model->>Model: schedule tea.Tick 2s
View->>View: render refreshHint in footer
Model->>Model: refreshHintClearMsg received after tick
Model->>Model: clear refreshHint
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/ui/model.go" line_range="393" />
<code_context>
+ if m.ifaceName != "" {
</code_context>
<issue_to_address>
**issue (bug_risk):** Sampler restart on refresh interval changes may leak goroutines or duplicate sampling without cancelling the previous one.
Adjusting the refresh interval currently creates a new sampler with a new background context but never cancels the old one. If the interval is changed multiple times, you can end up with several samplers running in parallel and leaked goroutines. Please ensure the previous sampler/context is cleanly stopped before starting a new one, or update the existing sampler’s configuration instead of recreating it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| m.smp = sampler.New(col, m.refreshInterval) | ||
| go m.smp.Run(ctx) | ||
|
|
||
| m.refreshHint = fmt.Sprintf("every %s", m.refreshInterval) |
There was a problem hiding this comment.
issue (bug_risk): Sampler restart on refresh interval changes may leak goroutines or duplicate sampling without cancelling the previous one.
Adjusting the refresh interval currently creates a new sampler with a new background context but never cancels the old one. If the interval is changed multiple times, you can end up with several samplers running in parallel and leaked goroutines. Please ensure the previous sampler/context is cleanly stopped before starting a new one, or update the existing sampler’s configuration instead of recreating it.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the application to version 0.1.5, introducing an enriched JSON snapshot output, extended sampling intervals (3s, 5s, and 10s), and a temporary footer notification showing the current refresh rate when adjusted. Feedback on these changes highlights a critical goroutine leak when adjusting the refresh interval due to unclosed channels, a recommendation to use the sample's actual timestamp rather than the current system time in the JSON output, and a race condition in the refresh hint timer that can be resolved by tracking and verifying timestamps on clear messages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| case key.Matches(msg, m.keys.Faster): | ||
| m.adjustRefreshInterval(true) | ||
| return m, waitForSample(m.smp.Out) | ||
| cmd := m.adjustRefreshInterval(true) | ||
| return m, tea.Batch(waitForSample(m.smp.Out), cmd) | ||
|
|
||
| case key.Matches(msg, m.keys.Slower): | ||
| m.adjustRefreshInterval(false) | ||
| return m, waitForSample(m.smp.Out) | ||
| cmd := m.adjustRefreshInterval(false) | ||
| return m, tea.Batch(waitForSample(m.smp.Out), cmd) |
There was a problem hiding this comment.
Goroutine Leak on Refresh Interval Adjustment
Every time the refresh interval is adjusted (via Faster or Slower keys), a new Sampler is created with a new Out channel, and a new waitForSample goroutine is started to read from it via tea.Batch(waitForSample(m.smp.Out), cmd).
However, the previous waitForSample goroutine remains blocked indefinitely on the old Out channel because:
- The old channel is never closed.
- The old sampler's
Runloop has exited (due tom.samplerCtx()cancellation) and will never write to it again.
This results in a goroutine leak every time the user adjusts the refresh rate (or cycles the interface).
Recommended Fix:
- Close the old channel in
adjustRefreshInterval(and similarly in the interface cycling logic) to unblock the old reader:m.samplerCtx() close(m.smp.Out) // Unblock the old waitForSample goroutine
- Update
waitForSample(around line 512) to handle channel closure gracefully:func waitForSample(ch <-chan sampler.Sample) tea.Cmd { return func() tea.Msg { s, ok := <-ch if !ok { return nil } return sampleMsg(s) } }
| "interface": s.Interface, | ||
| "unit_display": autoUnitExt(s.DownBps, bits), | ||
| "status": "ok", | ||
| "timestamp": time.Now().Format(time.RFC3339), |
There was a problem hiding this comment.
Use the sample's actual timestamp (s.At) instead of time.Now() to ensure the JSON output reflects the exact time the network metrics were captured.
| "timestamp": time.Now().Format(time.RFC3339), | |
| "timestamp": s.At.Format(time.RFC3339), |
|
|
||
| type pingMsg time.Duration | ||
|
|
||
| type refreshHintClearMsg struct{} |
There was a problem hiding this comment.
Using an empty struct for refreshHintClearMsg can lead to a race/overlapping timer issue. If the user presses + or - multiple times quickly, multiple clear messages are scheduled. When the first one fires, it will prematurely clear the hint for the subsequent presses. Changing refreshHintClearMsg to carry a timestamp allows us to ensure we only clear the hint if it matches the most recent update.
| type refreshHintClearMsg struct{} | |
| type refreshHintClearMsg time.Time |
| downPulse float64 | ||
| upPulse float64 | ||
| pingLatency time.Duration | ||
| refreshHint string |
There was a problem hiding this comment.
Add a refreshHintTime field to track when the last refresh hint was set, allowing us to prevent older scheduled timers from clearing newer hints.
| refreshHint string | |
| refreshHint string | |
| refreshHintTime time.Time |
| case refreshHintClearMsg: | ||
| m.refreshHint = "" | ||
| return m, nil |
There was a problem hiding this comment.
Only clear the refresh hint if the message's timestamp matches the most recently set hint's timestamp. This prevents older scheduled clear messages from clearing newer hints.
| case refreshHintClearMsg: | |
| m.refreshHint = "" | |
| return m, nil | |
| case refreshHintClearMsg: | |
| if time.Time(msg).Equal(m.refreshHintTime) { | |
| m.refreshHint = "" | |
| } | |
| return m, nil |
| m.refreshHint = fmt.Sprintf("every %s", m.refreshInterval) | ||
| return tea.Tick(2*time.Second, func(t time.Time) tea.Msg { return refreshHintClearMsg{} }) |
There was a problem hiding this comment.
Capture the current timestamp when setting the refresh hint, and pass it to the scheduled refreshHintClearMsg so we can uniquely identify this timer event.
now := time.Now()
m.refreshHint = fmt.Sprintf("every %s", m.refreshInterval)
m.refreshHintTime = now
return tea.Tick(2*time.Second, func(t time.Time) tea.Msg { return refreshHintClearMsg(now) })1 participant
Closes #19, #20, #21
Summary by Sourcery
Enrich JSON snapshot output and improve sampling interval controls and feedback for both short-term and long-term monitoring use cases.
New Features:
Summary by CodeRabbit
--once --jsonoutput now includes status, timestamp, and human-readable download/upload rates.