Add remote download progress#371
Merged
Merged
Conversation
Contributor
Reviewer's GuideImplements a configurable download progress observation system for remote configs and mixins, threads it through config loading and CLI wiring, and adds a Bubble Tea/bubbles-based animated progress UI plus tests and docs. Sequence diagram for remote download with progress reportingsequenceDiagram
actor User
participant CLI as Main
participant Loader as LoadRemote
participant Cfg as Config
participant Fetch as Download
participant Obs as Observer
participant Trk as ProgressTracker
User->>CLI: run lets -c https://url
CLI->>Loader: LoadRemote(ctx, url, noCache, version, WithProgress(...))
Loader->>Loader: ensureRemoteConfig(ctx, url, noCache, progress)
Loader->>Fetch: Download(ctx, url, WithProgress(SourceRemoteConfig, progress))
Fetch->>Obs: Start(ProgressInfo)
Obs-->>Fetch: ProgressTracker
Fetch->>Fetch: readAll(resp.Body, tracker)
loop while body.Read
Fetch->>Trk: Add(n)
end
Fetch->>Trk: Done(err)
Fetch-->>Loader: []byte, error
Loader-->>CLI: *config.Config, error
CLI->>Cfg: SetDownloadOptions(ctx, progress)
Cfg->>RemoteMixin: download(context(), downloadProgress)
RemoteMixin->>Fetch: Download(ctx, url, WithProgress(SourceRemoteMixin, progress))
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
recordingProgresshelper type is duplicated in bothinternal/fetch/fetch_test.goandinternal/config/load_test.go; consider moving a shared test helper into a common test file/package to avoid repetition and keep progress test utilities in one place. - In
readAll, you always use a plainbytes.Buffer; sinceDownloadalready knowsresp.ContentLength, you could pre-allocate the buffer capacity (e.g., viabuf.Grow(int(resp.ContentLength))) to avoid repeated allocations for larger downloads, including when aProgressTrackeris used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `recordingProgress` helper type is duplicated in both `internal/fetch/fetch_test.go` and `internal/config/load_test.go`; consider moving a shared test helper into a common test file/package to avoid repetition and keep progress test utilities in one place.
- In `readAll`, you always use a plain `bytes.Buffer`; since `Download` already knows `resp.ContentLength`, you could pre-allocate the buffer capacity (e.g., via `buf.Grow(int(resp.ContentLength))`) to avoid repeated allocations for larger downloads, including when a `ProgressTracker` is used.
## Individual Comments
### Comment 1
<location path="internal/fetch/fetch.go" line_range="25-26" />
<code_context>
- Timeout: 5 * 60 * time.Second,
+var httpClient = newHTTPClient()
+
+func newHTTPClient() *http.Client {
+ transport := http.DefaultTransport.(*http.Transport).Clone()
+ transport.DisableCompression = true
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard the type assertion on http.DefaultTransport to avoid unexpected panics if the default transport is overridden.
`http.DefaultTransport.(*http.Transport).Clone()` will panic if `DefaultTransport` has been replaced with a non-`*http.Transport` `RoundTripper`. Please guard the type assertion (e.g., with an `ok` check or type switch) and fall back to using `http.DefaultTransport` unchanged when it isn’t a `*http.Transport`, while preserving the current behavior when it is.
</issue_to_address>
### Comment 2
<location path="internal/downloadprogress/progress.go" line_range="506-515" />
<code_context>
+ return rawURL[:idx]
+}
+
+func truncateMiddle(s string, width int) string {
+ if width <= 0 {
+ return ""
+ }
+
+ if lipgloss.Width(s) <= width {
+ return s
+ }
+
+ if width <= 3 {
+ return strings.Repeat(".", width)
+ }
+
+ runes := []rune(s)
+ keep := width - 3
+ left := keep / 2
+ right := keep - left
+
+ if left+right >= len(runes) {
+ return s
+ }
+
+ return string(runes[:left]) + "..." + string(runes[len(runes)-right:])
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** truncateMiddle mixes rune-based slicing with lipgloss.Width, which can mis-handle wide or combining characters.
This logic decides whether to truncate using `lipgloss.Width(s)` but then truncates by indexing `[]rune(s)`. For strings with wide/combining characters where `lipgloss.Width` ≠ `len([]rune(s))`, you can end up with off-by-one visual widths or split grapheme clusters.
Consider either:
- Using a lipgloss-provided truncation helper if one exists, or
- Making the measurement and slicing use the same width metric (even plain rune length), so the truncation decision and the actual slice are consistent.
Also, clamping `keep` to `len(runes)` before computing `left/right` would reduce the risk of awkward splits.
Suggested implementation:
```golang
func truncateMiddle(s string, width int) string {
if width <= 0 {
return ""
}
runes := []rune(s)
if len(runes) <= width {
return s
}
if width <= 3 {
return strings.Repeat(".", width)
}
keep := width - 3
if keep > len(runes) {
keep = len(runes)
}
left := keep / 2
right := keep - left
if left+right >= len(runes) {
return s
}
return string(runes[:left]) + "..." + string(runes[len(runes)-right:])
}
```
If `lipgloss` is only used in `truncateMiddle`, you should also remove its import from this file to avoid an unused import error. If it's used elsewhere, no import changes are needed.
</issue_to_address>
### Comment 3
<location path="docs/docs/config.md" line_range="328-332" />
<code_context>
It is possible to specify mixin as url. Lets will download it and load it as a mixin.
File will be stored in `.lets/mixins` directory.
+When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress.
</code_context>
<issue_to_address>
**suggestion (typo):** Capitalize URL and consider adding missing articles in the mixin URL sentence.
For example, you could write: “It is possible to specify a mixin as a URL. lets will download it and load it as a mixin.”
```suggestion
It is possible to specify a mixin as a URL. lets will download it and load it as a mixin.
The file will be stored in the `.lets/mixins` directory.
When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress.
By default, the mixin filename will be the SHA256 hash of the URL.
```
</issue_to_address>
### Comment 4
<location path="docs/docs/config.md" line_range="332" />
<code_context>
File will be stored in `.lets/mixins` directory.
+When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress.
By default mixin filename will be sha256 hash of url.
</code_context>
<issue_to_address>
**suggestion (typo):** Capitalize URL and add article for readability.
Rephrase to: "By default, the mixin filename will be the SHA256 hash of the URL."
```suggestion
By default, the mixin filename will be the SHA256 hash of the URL.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+25
to
+26
| func newHTTPClient() *http.Client { | ||
| transport := http.DefaultTransport.(*http.Transport).Clone() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard the type assertion on http.DefaultTransport to avoid unexpected panics if the default transport is overridden.
http.DefaultTransport.(*http.Transport).Clone() will panic if DefaultTransport has been replaced with a non-*http.Transport RoundTripper. Please guard the type assertion (e.g., with an ok check or type switch) and fall back to using http.DefaultTransport unchanged when it isn’t a *http.Transport, while preserving the current behavior when it is.
Comment on lines
+506
to
+515
| func truncateMiddle(s string, width int) string { | ||
| if width <= 0 { | ||
| return "" | ||
| } | ||
|
|
||
| if lipgloss.Width(s) <= width { | ||
| return s | ||
| } | ||
|
|
||
| if width <= 3 { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): truncateMiddle mixes rune-based slicing with lipgloss.Width, which can mis-handle wide or combining characters.
This logic decides whether to truncate using lipgloss.Width(s) but then truncates by indexing []rune(s). For strings with wide/combining characters where lipgloss.Width ≠ len([]rune(s)), you can end up with off-by-one visual widths or split grapheme clusters.
Consider either:
- Using a lipgloss-provided truncation helper if one exists, or
- Making the measurement and slicing use the same width metric (even plain rune length), so the truncation decision and the actual slice are consistent.
Also, clamping keep to len(runes) before computing left/right would reduce the risk of awkward splits.
Suggested implementation:
func truncateMiddle(s string, width int) string {
if width <= 0 {
return ""
}
runes := []rune(s)
if len(runes) <= width {
return s
}
if width <= 3 {
return strings.Repeat(".", width)
}
keep := width - 3
if keep > len(runes) {
keep = len(runes)
}
left := keep / 2
right := keep - left
if left+right >= len(runes) {
return s
}
return string(runes[:left]) + "..." + string(runes[len(runes)-right:])
}If lipgloss is only used in truncateMiddle, you should also remove its import from this file to avoid an unused import error. If it's used elsewhere, no import changes are needed.
Comment on lines
328
to
332
| It is possible to specify mixin as url. Lets will download it and load it as a mixin. | ||
| File will be stored in `.lets/mixins` directory. | ||
| When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress. | ||
|
|
||
| By default mixin filename will be sha256 hash of url. |
Contributor
There was a problem hiding this comment.
suggestion (typo): Capitalize URL and consider adding missing articles in the mixin URL sentence.
For example, you could write: “It is possible to specify a mixin as a URL. lets will download it and load it as a mixin.”
Suggested change
| It is possible to specify mixin as url. Lets will download it and load it as a mixin. | |
| File will be stored in `.lets/mixins` directory. | |
| When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress. | |
| By default mixin filename will be sha256 hash of url. | |
| It is possible to specify a mixin as a URL. lets will download it and load it as a mixin. | |
| The file will be stored in the `.lets/mixins` directory. | |
| When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress. | |
| By default, the mixin filename will be the SHA256 hash of the URL. |
| File will be stored in `.lets/mixins` directory. | ||
| When stderr is an interactive terminal, lets shows download progress for remote mixin downloads. Cache hits do not show progress. | ||
|
|
||
| By default mixin filename will be sha256 hash of url. |
Contributor
There was a problem hiding this comment.
suggestion (typo): Capitalize URL and add article for readability.
Rephrase to: "By default, the mixin filename will be the SHA256 hash of the URL."
Suggested change
| By default mixin filename will be sha256 hash of url. | |
| By default, the mixin filename will be the SHA256 hash of the URL. |
kindermax
force-pushed
the
add-download-progress
branch
from
6c93616 to
096d449
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #360
Verification
Summary by Sourcery
Add a user-facing download progress indicator for remote configs and mixins and thread progress tracking through config loading and fetching.
New Features:
Enhancements:
Build:
Documentation:
Tests: