v0.1.5: enriched JSON, refresh indicator, slower sampling, bits label#24
Conversation
Reviewer's GuideAdds enriched JSON output for one-shot runs, exposes non-default refresh intervals and bits mode in the UI footer, extends available sampling intervals up to 5 minutes, and bumps the version and changelog for v0.1.5. 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)
📝 WalkthroughWalkthroughEnriches ChangesFlow 0.1.5 feature updates
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 enriches the JSON snapshot output with status, timestamp, and human-readable download/upload speeds, extends the sampling range for refresh intervals up to 5 minutes, and displays the active refresh interval and bits mode indicator in the footer. The feedback suggests using integer division instead of floating-point conversion for formatting intervals, resolving a potential inconsistency in the JSON output where raw numeric values remain in bytes/sec while the display unit is in bits, and replacing a hardcoded default refresh interval with a constant to improve maintainability.
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.
| if d >= time.Second { | ||
| return fmt.Sprintf("every %ds", int(d.Seconds())) | ||
| } |
There was a problem hiding this comment.
Using int(d.Seconds()) involves floating-point conversion, which is less idiomatic in Go and can theoretically lead to precision issues. It is more idiomatic and precise to use integer division: int(d / time.Second).
| if d >= time.Second { | |
| return fmt.Sprintf("every %ds", int(d.Seconds())) | |
| } | |
| if d >= time.Second { | |
| return fmt.Sprintf("every %ds", int(d/time.Second)) | |
| } |
| "download_bps": s.DownBps, | ||
| "upload_bps": s.UpBps, | ||
| "download_human": down, | ||
| "upload_human": up, | ||
| "peak_down_bps": s.DownBps, | ||
| "peak_up_bps": s.UpBps, | ||
| "unit_display": autoUnitExt(s.DownBps, bits), |
There was a problem hiding this comment.
When bits mode is active, unit_display returns a bits-based unit (e.g., Gb/s), and download_human displays the speed in bits/sec. However, download_bps, upload_bps, peak_down_bps, and peak_up_bps still contain the raw bytes/sec values (s.DownBps and s.UpBps). This creates an inconsistency in the JSON output where the raw numeric values do not match the unit specified in unit_display. Consider scaling these numeric values by 8 when bits is true, or renaming/adding separate fields to clarify the unit.
| if m.refreshInterval != 100*time.Millisecond { | ||
| ifaceStr += theme.Muted().Render(" " + formatInterval(m.refreshInterval)) | ||
| } |
There was a problem hiding this comment.
The default refresh interval of 100 * time.Millisecond is hardcoded here. If the default refresh interval is changed in the configuration or another part of the codebase in the future, this check will become out of sync. Consider defining a constant for the default refresh interval (e.g., in the config package or as a package-level constant) and referencing it here to improve maintainability.
![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, and left some high level feedback:
- The JSON snapshot timestamp is generated with a fresh
time.Now()rather than being coupled to the sample, which could drift if the sampling ever moves; consider sourcing or passing through the actual sample time so the JSON reflects when the measurement was taken. - The default refresh interval
100*time.Millisecondis now hard-coded indashboardContentLineswhile the set of intervals lives inadjustRefreshInterval; consider centralizing the default interval value (e.g., a shared constant) to avoid divergence if the default changes in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The JSON snapshot timestamp is generated with a fresh `time.Now()` rather than being coupled to the sample, which could drift if the sampling ever moves; consider sourcing or passing through the actual sample time so the JSON reflects when the measurement was taken.
- The default refresh interval `100*time.Millisecond` is now hard-coded in `dashboardContentLines` while the set of intervals lives in `adjustRefreshInterval`; consider centralizing the default interval value (e.g., a shared constant) to avoid divergence if the default changes in the future.
## Individual Comments
### Comment 1
<location path="internal/ui/views.go" line_range="452-453" />
<code_context>
+ if m.bitsMode {
+ ifaceStr += theme.Muted().Render(" [bits]")
+ }
+ if m.refreshInterval != 100*time.Millisecond {
+ ifaceStr += theme.Muted().Render(" " + formatInterval(m.refreshInterval))
+ }
</code_context>
<issue_to_address>
**suggestion:** Avoid hard-coding the 100ms default refresh interval in the view layer
Using `100*time.Millisecond` here couples the UI to a specific default. If that default changes, this condition will drift out of sync. Please reference a shared constant (or a model field) instead so changes to the default automatically stay consistent with the display logic.
Suggested implementation:
```golang
if m.refreshInterval != defaultRefreshInterval {
ifaceStr += theme.Muted().Render(" " + formatInterval(m.refreshInterval))
}
```
```golang
const defaultRefreshInterval = 100 * time.Millisecond
func formatInterval(d time.Duration) string {
```
If your project already defines a canonical default refresh interval (e.g. in a config or model package), you should:
1. Remove the newly added `defaultRefreshInterval` constant from `views.go`.
2. Import and reference the existing shared default (e.g. `model.DefaultRefreshInterval`) in the view condition instead of the local constant.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if m.refreshInterval != 100*time.Millisecond { | ||
| ifaceStr += theme.Muted().Render(" " + formatInterval(m.refreshInterval)) |
There was a problem hiding this comment.
suggestion: Avoid hard-coding the 100ms default refresh interval in the view layer
Using 100*time.Millisecond here couples the UI to a specific default. If that default changes, this condition will drift out of sync. Please reference a shared constant (or a model field) instead so changes to the default automatically stay consistent with the display logic.
Suggested implementation:
if m.refreshInterval != defaultRefreshInterval {
ifaceStr += theme.Muted().Render(" " + formatInterval(m.refreshInterval))
}const defaultRefreshInterval = 100 * time.Millisecond
func formatInterval(d time.Duration) string {If your project already defines a canonical default refresh interval (e.g. in a config or model package), you should:
- Remove the newly added
defaultRefreshIntervalconstant fromviews.go. - Import and reference the existing shared default (e.g.
model.DefaultRefreshInterval) in the view condition instead of the local constant.
1 participant
Closes #19, #20, #21, #22
Summary by Sourcery
Extend one-shot JSON output and the TUI footer to expose more context (status, timing, units, and refresh interval) and support slower sampling for long-running monitoring.
New Features:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Chores