Skip to content

feat: Add version check #310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rubiin wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Add version check #310

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ae60ef9
feat: Add version check and upgrade notification to root command
rubiin Dec 8, 2025
854e088
fix: Remove debug print statement for version check in root command
rubiin Dec 8, 2025
63265da
Merge branch 'main' into main
rubiin Dec 8, 2025
cc905db
Update seshcli/root_command.go
rubiin Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions seshcli/root_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package seshcli

import (
stdjson "encoding/json"
"errors"
"fmt"
"log/slog"
"net/http"
"strconv"
"strings"

"github.com/spf13/cobra"

Expand Down Expand Up @@ -31,6 +35,38 @@ import (
"github.com/joshmedeski/sesh/v2/zoxide"
)

func getLatestVersion() (string, error) {
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this feature to live in the root_command. Perhaps a "updater" package? Then we can add unit tests to ensure the isUpgradeAvailable has unit tests and mock the http response when testing other functionality

Copy link
Contributor Author

@rubiin rubiin Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to get this wrapped up in the coming weekend

resp, err := http.Get("https://api.github.com/repos/joshmedeski/sesh/releases/latest")
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use graphql or a query param to only return the tag_name instead of the entire payload?

if err != nil {
return "", err
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming if there's no internet it just silently fails?

}
defer resp.Body.Close()
var release struct {
TagName string `json:"tag_name"`
}
if err := stdjson.NewDecoder(resp.Body).Decode(&release); err != nil {
return "", err
}
Comment on lines +38 to +49
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub API has rate limits (60 requests/hour for unauthenticated requests). When users hit this limit, they'll see no update notification and won't know why the check failed. Additionally, if many users run sesh frequently, they could exhaust their rate limit quickly.

Consider:

  1. Adding a User-Agent header to identify the client: req.Header.Set("User-Agent", "sesh/"+version)
  2. Logging rate limit errors to help users understand why updates aren't being checked
  3. Implementing caching to reduce API calls
Suggested change
func getLatestVersion() (string, error) {
resp, err := http.Get("https://api.github.com/repos/joshmedeski/sesh/releases/latest")
if err != nil {
return "", err
}
defer resp.Body.Close()
var release struct {
TagName string `json:"tag_name"`
}
if err := stdjson.NewDecoder(resp.Body).Decode(&release); err != nil {
return "", err
}
// cache for latest version
var latestVersionCache struct {
version string
timestamp int64
}
func getLatestVersion(version string) (string, error) {
// cache for 1 hour
now := runtimewrap.Now().Unix()
if latestVersionCache.version != "" && now-latestVersionCache.timestamp < 3600 {
return latestVersionCache.version, nil
}
req, err := http.NewRequest("GET", "https://api.github.com/repos/joshmedeski/sesh/releases/latest", nil)
if err != nil {
return "", err
}
req.Header.Set("User-Agent", "sesh/"+version)
client := &http.Client{}
resp, err := client.Do(req)
if err != nil {
return "", err
}
defer resp.Body.Close()
if resp.StatusCode == http.StatusForbidden {
slog.Error("GitHub API rate limit exceeded. Update checks are temporarily disabled.")
return "", errors.New("GitHub API rate limit exceeded")
}
if resp.StatusCode != http.StatusOK {
return "", fmt.Errorf("unexpected status code: %d", resp.StatusCode)
}
var release struct {
TagName string `json:"tag_name"`
}
if err := stdjson.NewDecoder(resp.Body).Decode(&release); err != nil {
return "", err
}
latestVersionCache.version = release.TagName
latestVersionCache.timestamp = now

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this further supports my suggestion to limit how often we check for this.

return release.TagName, nil
Comment on lines +43 to +50
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTP response status code is not checked before attempting to decode the body. A non-200 response (e.g., 404, 403 from rate limiting, or 500) will attempt to decode an error response as a release, leading to silent failures or incorrect behavior.

Add status code validation:

defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
    return "", fmt.Errorf("unexpected status code: %d", resp.StatusCode)
}

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably something simple like "Unable to determine if upgrade is available"

}
Comment on lines +38 to +51
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTP request to GitHub API lacks a timeout, which could cause the CLI to hang indefinitely if the network is slow or GitHub is unresponsive. This blocks the application startup for all users on every invocation.

Consider using an HTTP client with a reasonable timeout (e.g., 2-3 seconds):

client := &http.Client{Timeout: 2 * time.Second}
resp, err := client.Get("https://api.github.com/repos/joshmedeski/sesh/releases/latest")

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I highly recommend this, since sesh is always meant to be fast.

I think it's less likely users will go out of their way to explicitly check for updates (which is why you built this feature), but I'm thinking 1 second would be the longest we'd want to wait.

Copy link
Contributor Author

@rubiin rubiin Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback


func isUpgradeAvailable(current, latest string) bool {
c := strings.TrimPrefix(current, "v")
l := strings.TrimPrefix(latest, "v")
cParts := strings.Split(c, ".")
lParts := strings.Split(l, ".")
for i := 0; i < len(cParts) && i < len(lParts); i++ {
cNum, _ := strconv.Atoi(cParts[i])
lNum, _ := strconv.Atoi(lParts[i])
Comment on lines +59 to +60
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version comparison silently ignores errors from strconv.Atoi, which means non-numeric version components (e.g., "2.0.0-beta", "1.2.3-rc1") will be treated as 0. This can lead to incorrect version comparison results.

Consider handling the error or using a proper semver library like github.com/Masterminds/semver/v3:

cNum, err := strconv.Atoi(cParts[i])
if err != nil {
    return false // or handle pre-release versions properly
}
lNum, err := strconv.Atoi(lParts[i])
if err != nil {
    return false
}

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done pre-release versions before, but if I choose to start doing that I'll make sure to update this area of the code.

if lNum > cNum {
return true
} else if lNum < cNum {
return false
}
}
return len(lParts) > len(cParts)
}
Comment on lines 53 to 68
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version check functions (getLatestVersion and isUpgradeAvailable) lack test coverage. Given that the codebase has comprehensive test coverage for other components (e.g., lister, namer, tmux packages all have corresponding test files), these new functions should also be tested.

Consider adding a test file seshcli/root_command_test.go with tests for:

  • isUpgradeAvailable with various version formats (e.g., "v1.2.3" vs "v1.2.4", "v2.0.0" vs "v1.9.9", equal versions, etc.)
  • Edge cases like empty strings, malformed versions, versions with different lengths

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I'm recommending we move this to it's own package :)


func NewRootCommand(version string) *cobra.Command {
// wrapper dependencies
exec := execwrap.NewExec()
Expand Down Expand Up @@ -66,6 +102,11 @@ func NewRootCommand(version string) *cobra.Command {

slog.Debug("seshcli/root_command.go: NewRootCommand", "version", version, "config", config)

latest, err := getLatestVersion()
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is uncommon to see sesh's output as many commands switch the user to a different session.

What would be a good way to present this to the user? I'm thinking when a user calls sesh without an argument and/or calls the --version flag.

And I agree, during development we'll need a way to skip this all-together.

if err == nil && isUpgradeAvailable(version, latest) {
fmt.Printf("A new version of sesh is available: %s (current: %s)\n", latest, version)
}
Comment on lines +105 to +108
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version check is performed synchronously during CLI initialization, blocking the startup of every command for every user. This adds network latency (potentially 100ms-2000ms+) to all CLI invocations, even when users just want to run quick commands.

Consider one of these approaches:

  1. Run the version check in a goroutine to avoid blocking startup
  2. Cache the result with a TTL (e.g., check once per day)
  3. Only check on specific commands or with a --check-updates flag
  4. Move this to a background service or scheduled task

Example with goroutine (non-blocking):

go func() {
    latest, err := getLatestVersion()
    if err == nil && isUpgradeAvailable(version, latest) {
        fmt.Fprintf(os.Stderr, "A new version of sesh is available: %s (current: %s)\n", latest, version)
    }
}()

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some interesting ideas here, let me know what you think


// core dependencies
ls := ls.NewLs(config, shell)
lister := lister.NewLister(config, home, tmux, zoxide, tmuxinator)
Expand Down