feat: updater + version display - #9
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds an auto-update workflow and version reporting to the dbtree CLI: a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as dbtree CLI
participant GitHub as GitHub API
participant FS as File System
User->>CLI: dbtree update
activate CLI
CLI->>GitHub: GET /repos/.../releases/latest
activate GitHub
GitHub-->>CLI: {tag_name: "vX.Y.Z", ...}
deactivate GitHub
Note over CLI: Compare currentVersion vs latestVersion
alt Newer version available
CLI->>GitHub: GET /archive/.../dbtree_<ver>_<os>_<arch>.(tar.gz|zip)
activate GitHub
GitHub-->>CLI: Archive bytes
deactivate GitHub
CLI->>FS: Save archive to temp dir
FS-->>CLI: Archive saved
CLI->>FS: Extract archive → locate `dbtree` binary
FS-->>CLI: Extracted binary path
CLI->>FS: Atomically replace running executable (win/unix flows)
FS-->>CLI: Replacement complete
CLI->>User: Update successful
else Already latest
CLI->>User: Already up-to-date
end
deactivate CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
updater/updater.go (3)
76-97: HTTP requests lack timeout and User-Agent header.
http.Getuses the default client with no timeout, which could hang indefinitely. Additionally, GitHub API may rate-limit requests without aUser-Agentheader more aggressively.♻️ Proposed fix with timeout and User-Agent
func getLatestVersion() (string, error) { - resp, err := http.Get("https://api.github.com/repos/viveknathani/dbtree/releases/latest") + client := &http.Client{Timeout: 30 * time.Second} + req, err := http.NewRequest("GET", "https://api.github.com/repos/viveknathani/dbtree/releases/latest", nil) + if err != nil { + return "", err + } + req.Header.Set("User-Agent", "dbtree-updater") + resp, err := client.Do(req) if err != nil { return "", err }Don't forget to add
"time"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/updater.go` around lines 76 - 97, The getLatestVersion function uses http.Get with no timeout or headers; replace it by creating an http.Client with a reasonable Timeout (e.g., a few seconds), build the request via http.NewRequest("GET", "...", nil), set a User-Agent header (e.g., "dbtree-updater" or similar) on the request, and call client.Do(req) instead of http.Get; keep existing resp.Body.Close and JSON decoding/error checks, and add the "time" import for the timeout configuration.
29-32: Version comparison uses string equality — may fail for semantic versions.String comparison works for exact matches but doesn't handle semantic versioning edge cases (e.g.,
1.10.0vs1.9.0would incorrectly compare as strings). Consider using a semver library likegolang.org/x/mod/semverorgithub.com/Masterminds/semverfor robust comparison.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/updater.go` around lines 29 - 32, The code currently compares versions using string equality between currentVersion and latestClean (strings.TrimPrefix(latest, "v")), which fails for semantic-version ordering; replace the string equality check with a semantic-version comparison using a semver library (e.g., import "golang.org/x/mod/semver" or "github.com/Masterminds/semver"), validate/normalize both currentVersion and latestClean (add leading "v" if required by the chosen lib), and use the library's Compare/Equal functions to determine if currentVersion == latestClean or if an update is needed before printing "Already up to date" in the updater logic.
99-118: Same timeout concern applies to download requests.Large file downloads without timeout could hang indefinitely. Consider using the same client pattern with timeout as suggested for
getLatestVersion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/updater.go` around lines 99 - 118, The downloadFile function uses http.Get which can hang for large downloads; replace it with an http.Client configured with a timeout (the same pattern used in getLatestVersion) and use client.Do on a GET request (or accept a context) so the request is bounded; update downloadFile to create an http.Client{Timeout: <appropriate duration>}, build an http.NewRequest("GET", url, nil), call client.Do(req), handle resp.StatusCode as before, and ensure resp.Body and file are properly closed and errors propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/updater.go`:
- Line 40: The archive name currently always uses ".tar.gz" (archiveName :=
fmt.Sprintf("dbtree_%s_%s_%s.tar.gz", ...)), which breaks Windows updates;
change the extension to ".zip" when runtime.GOOS == "windows" and build
archiveName accordingly, add a new extractBinaryFromZip(archivePath, destDir
string) (returning the extracted binary path and error) that opens the zip,
finds "dbtree.exe", copies it with executable perms into destDir, and then,
where extraction is performed (the existing tar.gz extraction call/site),
conditionally call extractBinaryFromZip for Windows and the existing tar.gz
extractor for other platforms so Windows downloads and extraction succeed.
- Line 143: The check for the dbtree binary only matches "dbtree" and misses
Windows "dbtree.exe"; update the condition that uses header and
filepath.Base(header.Name) so it normalizes the filename by trimming a possible
".exe" suffix before comparing to "dbtree" (e.g. name :=
filepath.Base(header.Name); if header.Typeflag == tar.TypeReg &&
strings.TrimSuffix(name, ".exe") == "dbtree" { ... }), and add the required
import for the strings package.
- Around line 161-195: replaceBinary currently fails on Windows because
executables are locked; implement a Windows-specific two-process swap or use a
library that does (e.g., inconshreveable/go-update) to perform a safe replace
while the app is running. Update the replaceBinary flow: detect runtime.GOOS ==
"windows" in replaceBinary, spawn a short-lived helper process (or call
MoveFileEx via syscall with MOVEFILE_REPLACE_EXISTING after the main process
exits) that waits for the parent to exit, then atomically moves the new binary
into place and removes the old one; ensure the helper returns exit codes on
failure and that file permissions are preserved similarly to the non-Windows
path. Also add a brief comment in replaceBinary explaining the Windows lock
behavior and the chosen approach.
---
Nitpick comments:
In `@updater/updater.go`:
- Around line 76-97: The getLatestVersion function uses http.Get with no timeout
or headers; replace it by creating an http.Client with a reasonable Timeout
(e.g., a few seconds), build the request via http.NewRequest("GET", "...", nil),
set a User-Agent header (e.g., "dbtree-updater" or similar) on the request, and
call client.Do(req) instead of http.Get; keep existing resp.Body.Close and JSON
decoding/error checks, and add the "time" import for the timeout configuration.
- Around line 29-32: The code currently compares versions using string equality
between currentVersion and latestClean (strings.TrimPrefix(latest, "v")), which
fails for semantic-version ordering; replace the string equality check with a
semantic-version comparison using a semver library (e.g., import
"golang.org/x/mod/semver" or "github.com/Masterminds/semver"),
validate/normalize both currentVersion and latestClean (add leading "v" if
required by the chosen lib), and use the library's Compare/Equal functions to
determine if currentVersion == latestClean or if an update is needed before
printing "Already up to date" in the updater logic.
- Around line 99-118: The downloadFile function uses http.Get which can hang for
large downloads; replace it with an http.Client configured with a timeout (the
same pattern used in getLatestVersion) and use client.Do on a GET request (or
accept a context) so the request is bounded; update downloadFile to create an
http.Client{Timeout: <appropriate duration>}, build an http.NewRequest("GET",
url, nil), call client.Do(req), handle resp.StatusCode as before, and ensure
resp.Body and file are properly closed and errors propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9279926c-f7b2-480a-b26f-35a402077a35
📒 Files selected for processing (2)
cmd/dbtree/main.goupdater/updater.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
updater/updater.go (1)
217-228:⚠️ Potential issue | 🟠 MajorWindows in-process binary swap can still fail on a running
.exe.Line 220 renames the currently running executable in-process. On Windows, this path is brittle and can fail with file-lock/access errors. Prefer a helper-process swap (main exits, helper replaces, then relaunches), or a proven self-update library flow.
In Go on Windows, can a running executable reliably be renamed/replaced by the same process, and what self-update pattern is recommended?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/updater.go` around lines 217 - 228, The Windows branch in updater.go currently attempts an in-process os.Rename of the running executable (using target + ".old" and moveFile), which can fail due to Windows file locks; replace this with a helper-process swap flow: write the new binary to a temporary path, spawn a short-lived updater helper (or use a proven self-update library) passing the current target, temp-path and old-path, then have the main process exit immediately so the helper can wait for the process to terminate, rename/move files atomically (rename target→old, temp→target) and finally relaunch the app; update the code paths around the existing moveFile call and the Windows-specific block in updater.go to invoke the helper (or library API) and handle its error/exit codes instead of attempting an in-process os.Rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/updater.go`:
- Around line 54-57: After downloadFile writes archivePath (using tmpDir,
archiveName, downloadURL) you must verify the artifact integrity before any
extraction/replacement: compute the SHA-256 (or configured hash) of archivePath
and compare it to the expected checksum retrieved from the signed checksums
asset (download or load the checksums file and verify its signature) and fail
fast if they differ; add a helper like verifyArchiveChecksum(archivePath,
expectedChecksum) and a routine verifyChecksumsSignature(checksumsBlob,
signature) to validate the signed checksums, and invoke these checks immediately
after the downloadFile call and before any extraction/replacement logic,
returning an error if verification fails.
- Around line 87-117: The two http.Get calls (the one fetching the GitHub latest
release and the one in downloadFile) use the default client without timeouts;
replace them with an http.Client that has a sensible Timeout (e.g., 10-30s) and
use client.Get instead, or create a package-level client with a configured
Timeout and reuse it in both the release-checking function and downloadFile to
prevent indefinite hangs; ensure you close resp.Body as before and propagate
errors the same way.
---
Duplicate comments:
In `@updater/updater.go`:
- Around line 217-228: The Windows branch in updater.go currently attempts an
in-process os.Rename of the running executable (using target + ".old" and
moveFile), which can fail due to Windows file locks; replace this with a
helper-process swap flow: write the new binary to a temporary path, spawn a
short-lived updater helper (or use a proven self-update library) passing the
current target, temp-path and old-path, then have the main process exit
immediately so the helper can wait for the process to terminate, rename/move
files atomically (rename target→old, temp→target) and finally relaunch the app;
update the code paths around the existing moveFile call and the Windows-specific
block in updater.go to invoke the helper (or library API) and handle its
error/exit codes instead of attempting an in-process os.Rename.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
updater/updater.go (2)
33-37: Version comparison doesn't handle semantic versioning or edge cases.The simple string equality comparison works for normal version checks, but has edge cases:
- Pre-release or development builds (e.g.,
1.1.0-dev) will always be considered different from release versions- If a user has a newer local build than the latest release, it would attempt a "downgrade"
Consider using semantic version comparison (e.g.,
github.com/Masterminds/semver) or at minimum document that dev builds should not use the updater.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/updater.go` around lines 33 - 37, The current comparison using latestClean and currentVersion only checks string equality and fails for semver edge cases (pre-releases, build metadata, newer local builds); update the logic in updater.go to parse and compare semantic versions instead of string equality: use a semver library (e.g., github.com/Masterminds/semver) to parse currentVersion and latest (after TrimPrefix) into Version objects, handle parse errors (fall back to logging and skipping automatic update), and compare with Version.Compare/Compare to detect older/newer/pre-release cases so you avoid downgrades and correctly treat dev builds; include a clear log message when versions are incomparable and document that dev builds are not supported by the updater.
123-130: Consider explicit sync before close for robustness.While typically fine, adding
f.Sync()before close ensures the downloaded content is flushed to disk, making the operation more robust against unexpected system interruptions during large downloads.♻️ Proposed fix
_, err = io.Copy(f, resp.Body) + if err != nil { + return err + } + if err := f.Sync(); err != nil { + return err + } return err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/updater.go` around lines 123 - 130, The file currently creates a file with os.Create (variable f), copies resp.Body into it with io.Copy, and defers f.Close() without flushing; modify the write sequence in the function containing this snippet so that after io.Copy returns you call f.Sync() and check its error (return it if non-nil) before closing the file, and keep the deferred f.Close() for cleanup; reference the os.Create / f variable, the io.Copy call, and the deferred f.Close() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@updater/updater.go`:
- Around line 33-37: The current comparison using latestClean and currentVersion
only checks string equality and fails for semver edge cases (pre-releases, build
metadata, newer local builds); update the logic in updater.go to parse and
compare semantic versions instead of string equality: use a semver library
(e.g., github.com/Masterminds/semver) to parse currentVersion and latest (after
TrimPrefix) into Version objects, handle parse errors (fall back to logging and
skipping automatic update), and compare with Version.Compare/Compare to detect
older/newer/pre-release cases so you avoid downgrades and correctly treat dev
builds; include a clear log message when versions are incomparable and document
that dev builds are not supported by the updater.
- Around line 123-130: The file currently creates a file with os.Create
(variable f), copies resp.Body into it with io.Copy, and defers f.Close()
without flushing; modify the write sequence in the function containing this
snippet so that after io.Copy returns you call f.Sync() and check its error
(return it if non-nil) before closing the file, and keep the deferred f.Close()
for cleanup; reference the os.Create / f variable, the io.Copy call, and the
deferred f.Close() when making the change.
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.
closes #6, closes #7