From 97483c8cc39bc8d664b15850042d09a357ee8fc9 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 07:35:02 -0400 Subject: [PATCH] lint cleanup of code base --- .golangci.yml | 239 +++++++++++++++++++++++++++++++++++ .yamllint | 16 +++ Makefile | 82 ++++++++++-- cache.go | 35 ++--- github.go | 158 ++++++++++------------- loginitem_darwin.go | 64 +++++++--- loginitem_other.go | 4 +- main.go | 302 ++++++++++++++++++++++---------------------- main_test.go | 75 +++++++++++ ui.go | 285 ++++++++++++++++++++--------------------- 10 files changed, 819 insertions(+), 441 deletions(-) create mode 100644 .golangci.yml create mode 100644 .yamllint create mode 100644 main_test.go diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..74e86ed --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,239 @@ +## Golden config for golangci-lint - strict, but within the realm of what Go authors might use. +# +# This is tied to the version of golangci-lint listed in the Makefile, usage with other +# versions of golangci-lint will yield errors and/or false positives. +# +# Docs: https://github.com/golangci/golangci-lint/blob/HEAD/.golangci.reference.yml +# Based heavily on https://gist.github.com/maratori/47a4d00457a92aa426dbd48a18776322 + +version: "2" + +issues: + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. + max-issues-per-linter: 0 + # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. + max-same-issues: 0 + +formatters: + enable: + # - gci + # - gofmt + - gofumpt + # - goimports + # - golines + - swaggo + + settings: + golines: + # Default: 100 + max-len: 120 + +linters: + default: all + disable: + # linters that give advice contrary to what the Go authors advise + - decorder # checks declaration order and count of types, constants, variables and functions + - dupword # [useless without config] checks for duplicate words in the source code + - exhaustruct # [highly recommend to enable] checks if all structure fields are initialized + - forcetypeassert # [replaced by errcheck] finds forced type assertions + - ginkgolinter # [if you use ginkgo/gomega] enforces standards of using ginkgo and gomega + - gochecknoglobals # checks that no global variables exist + - cyclop # replaced by revive + - gocyclo # replaced by revive + - funlen # replaced by revive + - godox # TODO's are OK + - ireturn # It's OK + - musttag + - nonamedreturns + - goconst # finds repeated strings that could be replaced by a constant + - goheader # checks is file header matches to pattern + - gomodguard # [use more powerful depguard] allow and block lists linter for direct Go module dependencies + - gomoddirectives + - err113 # bad advice about dynamic errors + - lll # [replaced by golines] reports long lines + - mnd # detects magic numbers, duplicated by revive + - nlreturn # [too strict and mostly code is not more readable] checks for a new line before return and branch statements to increase code clarity + - noinlineerr # disallows inline error handling `if err := ...; err != nil {` + - prealloc # [premature optimization, but can be used in some cases] finds slice declarations that could potentially be preallocated + - tagliatelle # needs configuration + - testableexamples # checks if examples are testable (have an expected output) + - testpackage # makes you use a separate _test package + - paralleltest # not every test should be in parallel + - wrapcheck # not required + - wsl # [too strict and mostly code is not more readable] whitespace linter forces you to use empty lines + - wsl_v5 # [too strict and mostly code is not more readable] add or remove empty lines + - zerologlint # detects the wrong usage of zerolog that a user forgets to dispatch zerolog.Event + + # All settings can be found here https://github.com/golangci/golangci-lint/blob/HEAD/.golangci.reference.yml + settings: + depguard: + rules: + "deprecated": + files: + - "$all" + deny: + - pkg: github.com/golang/protobuf + desc: Use google.golang.org/protobuf instead, see https://developers.google.com/protocol-buffers/docs/reference/go/faq#modules + - pkg: github.com/satori/go.uuid + desc: Use github.com/google/uuid instead, satori's package is not maintained + - pkg: github.com/gofrs/uuid$ + desc: Use github.com/gofrs/uuid/v5 or later, it was not a go module before v5 + "non-test files": + files: + - "!$test" + deny: + - pkg: math/rand$ + desc: Use math/rand/v2 instead, see https://go.dev/blog/randv2 + - pkg: "github.com/sirupsen/logrus" + desc: not allowed + - pkg: "github.com/pkg/errors" + desc: Should be replaced by standard lib errors package + + dupl: + # token count (default: 150) + threshold: 300 + + embeddedstructfieldcheck: + # Checks that sync.Mutex and sync.RWMutex are not used as embedded fields. + forbid-mutex: true + + errcheck: + # Report about not checking of errors in type assertions: `a := b.(MyStruct)`. + check-type-assertions: true + check-blank: true + + exhaustive: + # Program elements to check for exhaustiveness. + # Default: [ switch ] + check: + - switch + - map + default-signifies-exhaustive: true + + fatcontext: + # Check for potential fat contexts in struct pointers. + # May generate false positives. + # Default: false + check-struct-pointers: true + + funcorder: + # Checks if the exported methods of a structure are placed before the non-exported ones. + struct-method: false + + gocognit: + min-complexity: 55 + + gocritic: + enable-all: true + disabled-checks: + - paramTypeCombine + # The list of supported checkers can be found at https://go-critic.com/overview. + settings: + captLocal: + # Whether to restrict checker to params only. + paramsOnly: false + underef: + # Whether to skip (*x).method() calls where x is a pointer receiver. + skipRecvDeref: false + hugeParam: + # Default: 80 + sizeThreshold: 200 + + govet: + enable-all: true + + godot: + scope: toplevel + + inamedparam: + # Skips check for interface methods with only a single parameter. + skip-single-param: true + + nakedret: + # Default: 30 + max-func-lines: 4 + + nestif: + min-complexity: 12 + + nolintlint: + # Exclude following linters from requiring an explanation. + # Default: [] + allow-no-explanation: [funlen, gocognit, golines] + # Enable to require an explanation of nonzero length after each nolint directive. + require-explanation: true + # Enable to require nolint directives to mention the specific linter being suppressed. + require-specific: true + + revive: + enable-all-rules: true + rules: + - name: add-constant + severity: warning + disabled: false + exclude: [""] + arguments: + - max-lit-count: "5" + allow-strs: '"","\n"' + allow-ints: "0,1,2,3,24,30,365,1024,0o600,0o700,0o750,0o755" + allow-floats: "0.0,0.,1.0,1.,2.0,2." + - name: cognitive-complexity + arguments: [55] + - name: cyclomatic + arguments: [60] + - name: function-length + arguments: [150, 225] + - name: line-length-limit + arguments: [150] + - name: nested-structs + disabled: true + - name: max-public-structs + arguments: [10] + - name: flag-parameter # fixes are difficult + disabled: true + + rowserrcheck: + # database/sql is always checked. + # Default: [] + packages: + - github.com/jmoiron/sqlx + + perfsprint: + # optimize fmt.Sprintf("x: %s", y) into "x: " + y + strconcat: false + + staticcheck: + checks: + - all + + usetesting: + # Enable/disable `os.TempDir()` detections. + # Default: false + os-temp-dir: true + + varnamelen: + max-distance: 40 + min-name-length: 2 + + exclusions: + # Default: [] + presets: + - common-false-positives + rules: + # Allow "err" and "ok" vars to shadow existing declarations, otherwise we get too many false positives. + - text: '^shadow: declaration of "(err|ok)" shadows declaration' + linters: + - govet + - text: "parameter 'ctx' seems to be unused, consider removing or renaming it as _" + linters: + - revive + - path: _test\.go + linters: + - dupl + - gosec + - godot + - govet # alignment + - noctx + - perfsprint + - revive + - varnamelen diff --git a/.yamllint b/.yamllint new file mode 100644 index 0000000..9a08ad1 --- /dev/null +++ b/.yamllint @@ -0,0 +1,16 @@ +--- +extends: default + +rules: + braces: + max-spaces-inside: 1 + brackets: + max-spaces-inside: 1 + comments: disable + comments-indentation: disable + document-start: disable + line-length: + level: warning + max: 160 + allow-non-breakable-inline-mappings: true + truthy: disable diff --git a/Makefile b/Makefile index 6164740..4f0e327 100644 --- a/Makefile +++ b/Makefile @@ -4,6 +4,12 @@ VERSION = 1.0.0 BUNDLE_VERSION = 1 BUNDLE_ID = dev.codegroove.r2r +# Version information for builds +GIT_VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") +GIT_COMMIT := $(shell git rev-parse --short HEAD 2>/dev/null || echo "unknown") +BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ") +LDFLAGS := -X main.version=$(GIT_VERSION) -X main.commit=$(GIT_COMMIT) -X main.date=$(BUILD_DATE) + .PHONY: build clean deps run app-bundle install install-darwin install-unix install-windows # Install dependencies @@ -24,9 +30,9 @@ endif # Build for current platform build: ifeq ($(OS),Windows_NT) - CGO_ENABLED=1 go build -ldflags -H=windowsgui -o $(APP_NAME).exe . + CGO_ENABLED=1 go build -ldflags "-H=windowsgui $(LDFLAGS)" -o $(APP_NAME).exe . else - CGO_ENABLED=1 go build -o $(APP_NAME) . + CGO_ENABLED=1 go build -ldflags "$(LDFLAGS)" -o $(APP_NAME) . endif # Build for all platforms @@ -34,18 +40,18 @@ build-all: build-darwin build-linux build-windows # Build for macOS build-darwin: - CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 go build -o out/$(APP_NAME)-darwin-amd64 . - CGO_ENABLED=1 GOOS=darwin GOARCH=arm64 go build -o out/$(APP_NAME)-darwin-arm64 . + CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 go build -ldflags "$(LDFLAGS)" -o out/$(APP_NAME)-darwin-amd64 . + CGO_ENABLED=1 GOOS=darwin GOARCH=arm64 go build -ldflags "$(LDFLAGS)" -o out/$(APP_NAME)-darwin-arm64 . # Build for Linux build-linux: - CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -o out/$(APP_NAME)-linux-amd64 . - CGO_ENABLED=1 GOOS=linux GOARCH=arm64 go build -o out/$(APP_NAME)-linux-arm64 . + CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -ldflags "$(LDFLAGS)" -o out/$(APP_NAME)-linux-amd64 . + CGO_ENABLED=1 GOOS=linux GOARCH=arm64 go build -ldflags "$(LDFLAGS)" -o out/$(APP_NAME)-linux-arm64 . # Build for Windows build-windows: - CGO_ENABLED=1 GOOS=windows GOARCH=amd64 go build -ldflags -H=windowsgui -o out/$(APP_NAME)-windows-amd64.exe . - CGO_ENABLED=1 GOOS=windows GOARCH=arm64 go build -ldflags -H=windowsgui -o out/$(APP_NAME)-windows-arm64.exe . + CGO_ENABLED=1 GOOS=windows GOARCH=amd64 go build -ldflags "-H=windowsgui $(LDFLAGS)" -o out/$(APP_NAME)-windows-amd64.exe . + CGO_ENABLED=1 GOOS=windows GOARCH=arm64 go build -ldflags "-H=windowsgui $(LDFLAGS)" -o out/$(APP_NAME)-windows-arm64.exe . # Clean build artifacts clean: @@ -166,4 +172,62 @@ install-windows: build @echo "Copying executable..." @copy /Y "$(APP_NAME).exe" "%LOCALAPPDATA%\Programs\$(APP_NAME)\" @echo "Installation complete! $(APP_NAME) has been installed to %LOCALAPPDATA%\Programs\$(APP_NAME)" - @echo "You may want to add %LOCALAPPDATA%\Programs\$(APP_NAME) to your PATH environment variable." \ No newline at end of file + @echo "You may want to add %LOCALAPPDATA%\Programs\$(APP_NAME) to your PATH environment variable." +# BEGIN: lint-install . +# http://github.com/codeGROOVE-dev/lint-install + +.PHONY: lint +lint: _lint + +LINT_ARCH := $(shell uname -m) +LINT_OS := $(shell uname) +LINT_OS_LOWER := $(shell echo $(LINT_OS) | tr '[:upper:]' '[:lower:]') +LINT_ROOT := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) + +# shellcheck and hadolint lack arm64 native binaries: rely on x86-64 emulation +ifeq ($(LINT_OS),Darwin) + ifeq ($(LINT_ARCH),arm64) + LINT_ARCH=x86_64 + endif +endif + +LINTERS := +FIXERS := + +GOLANGCI_LINT_CONFIG := $(LINT_ROOT)/.golangci.yml +GOLANGCI_LINT_VERSION ?= v2.3.1 +GOLANGCI_LINT_BIN := $(LINT_ROOT)/out/linters/golangci-lint-$(GOLANGCI_LINT_VERSION)-$(LINT_ARCH) +$(GOLANGCI_LINT_BIN): + mkdir -p $(LINT_ROOT)/out/linters + rm -rf $(LINT_ROOT)/out/linters/golangci-lint-* + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(LINT_ROOT)/out/linters $(GOLANGCI_LINT_VERSION) + mv $(LINT_ROOT)/out/linters/golangci-lint $@ + +LINTERS += golangci-lint-lint +golangci-lint-lint: $(GOLANGCI_LINT_BIN) + find . -name go.mod -execdir "$(GOLANGCI_LINT_BIN)" run -c "$(GOLANGCI_LINT_CONFIG)" \; + +FIXERS += golangci-lint-fix +golangci-lint-fix: $(GOLANGCI_LINT_BIN) + find . -name go.mod -execdir "$(GOLANGCI_LINT_BIN)" run -c "$(GOLANGCI_LINT_CONFIG)" --fix \; + +YAMLLINT_VERSION ?= 1.37.1 +YAMLLINT_ROOT := $(LINT_ROOT)/out/linters/yamllint-$(YAMLLINT_VERSION) +YAMLLINT_BIN := $(YAMLLINT_ROOT)/dist/bin/yamllint +$(YAMLLINT_BIN): + mkdir -p $(LINT_ROOT)/out/linters + rm -rf $(LINT_ROOT)/out/linters/yamllint-* + curl -sSfL https://github.com/adrienverge/yamllint/archive/refs/tags/v$(YAMLLINT_VERSION).tar.gz | tar -C $(LINT_ROOT)/out/linters -zxf - + cd $(YAMLLINT_ROOT) && pip3 install --target dist . || pip install --target dist . + +LINTERS += yamllint-lint +yamllint-lint: $(YAMLLINT_BIN) + PYTHONPATH=$(YAMLLINT_ROOT)/dist $(YAMLLINT_ROOT)/dist/bin/yamllint . + +.PHONY: _lint $(LINTERS) +_lint: $(LINTERS) + +.PHONY: fix $(FIXERS) +fix: $(FIXERS) + +# END: lint-install . diff --git a/cache.go b/cache.go index fe23bf1..6eda802 100644 --- a/cache.go +++ b/cache.go @@ -2,6 +2,7 @@ package main import ( + "context" "crypto/sha256" "encoding/hex" "encoding/json" @@ -21,8 +22,8 @@ type cacheEntry struct { UpdatedAt time.Time `json:"updated_at"` } -// getTurnData fetches Turn API data with caching -func (app *App) getTurnData(url string, updatedAt time.Time) (*turn.CheckResponse, error) { +// turnData fetches Turn API data with caching. +func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (*turn.CheckResponse, error) { // Create cache key from URL and updated timestamp key := fmt.Sprintf("%s-%s", url, updatedAt.Format(time.RFC3339)) hash := sha256.Sum256([]byte(key)) @@ -41,8 +42,14 @@ func (app *App) getTurnData(url string, updatedAt time.Time) (*turn.CheckRespons // Cache miss, fetch from API log.Printf("Cache miss for %s, fetching from Turn API", url) - data, err := app.turnClient.Check(app.ctx, url, app.currentUser.GetLogin(), updatedAt) + + // Just try once with timeout - if Turn API fails, it's not critical + turnCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + data, err := app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), updatedAt) if err != nil { + log.Printf("Turn API error (will use PR without metadata): %v", err) return nil, err } @@ -62,7 +69,7 @@ func (app *App) getTurnData(url string, updatedAt time.Time) (*turn.CheckRespons return data, nil } -// cleanupOldCache removes cache files older than 5 days +// cleanupOldCache removes cache files older than 5 days. func (app *App) cleanupOldCache() { entries, err := os.ReadDir(app.cacheDir) if err != nil { @@ -74,7 +81,6 @@ func (app *App) cleanupOldCache() { continue } - filePath := filepath.Join(app.cacheDir, entry.Name()) info, err := entry.Info() if err != nil { continue @@ -82,22 +88,9 @@ func (app *App) cleanupOldCache() { // Remove cache files older than 5 days if time.Since(info.ModTime()) > cacheCleanupInterval { - log.Printf("Removing old cache file: %s", entry.Name()) - os.Remove(filePath) + if err := os.Remove(filepath.Join(app.cacheDir, entry.Name())); err != nil { + log.Printf("Failed to remove old cache: %v", err) + } } } } - -// startCacheCleanup starts periodic cache cleanup -func (app *App) startCacheCleanup() { - // Initial cleanup - app.cleanupOldCache() - - // Schedule periodic cleanup - ticker := time.NewTicker(24 * time.Hour) - go func() { - for range ticker.C { - app.cleanupOldCache() - } - }() -} diff --git a/github.go b/github.go index e69d8c5..9f3b6f0 100644 --- a/github.go +++ b/github.go @@ -2,6 +2,8 @@ package main import ( + "context" + "errors" "fmt" "log" "os" @@ -16,9 +18,9 @@ import ( "golang.org/x/oauth2" ) -// initClients initializes GitHub and Turn API clients -func (app *App) initClients() error { - token, err := app.githubToken() +// initClients initializes GitHub and Turn API clients. +func (app *App) initClients(ctx context.Context) error { + token, err := app.githubToken(ctx) if err != nil { return fmt.Errorf("get github token: %w", err) } @@ -26,7 +28,7 @@ func (app *App) initClients() error { ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: token}, ) - tc := oauth2.NewClient(app.ctx, ts) + tc := oauth2.NewClient(ctx, ts) app.client = github.NewClient(tc) // Initialize Turn client with base URL @@ -40,43 +42,26 @@ func (app *App) initClients() error { return nil } -// findGHCommand locates the gh CLI tool -func findGHCommand() (string, error) { - log.Println("Looking for gh command...") - - // Log current PATH - log.Printf("Current PATH: %s", os.Getenv("PATH")) - - // Check if gh is in PATH first - ghCmd := "gh" - if runtime.GOOS == "windows" { - ghCmd = "gh.exe" - } - if path, err := exec.LookPath(ghCmd); err == nil { - log.Printf("Found gh in PATH: %s", path) - return path, nil - } - - log.Println("gh not found in PATH, checking common locations...") - - // Common installation paths for gh based on OS - var commonPaths []string +// githubToken retrieves the GitHub token using gh CLI. +func (*App) githubToken(ctx context.Context) (string, error) { + // Only check absolute paths for security - never use PATH + var trustedPaths []string switch runtime.GOOS { case "windows": - commonPaths = []string{ + trustedPaths = []string{ `C:\Program Files\GitHub CLI\gh.exe`, `C:\Program Files (x86)\GitHub CLI\gh.exe`, filepath.Join(os.Getenv("LOCALAPPDATA"), "Programs", "gh", "gh.exe"), filepath.Join(os.Getenv("LOCALAPPDATA"), "GitHub CLI", "gh.exe"), } case "darwin": - commonPaths = []string{ + trustedPaths = []string{ "/opt/homebrew/bin/gh", // Homebrew on Apple Silicon "/usr/local/bin/gh", // Homebrew on Intel / manual install "/usr/bin/gh", // System package managers } case "linux": - commonPaths = []string{ + trustedPaths = []string{ "/usr/local/bin/gh", // Manual install "/usr/bin/gh", // System package managers "/home/linuxbrew/.linuxbrew/bin/gh", // Linuxbrew @@ -84,32 +69,37 @@ func findGHCommand() (string, error) { } default: // BSD and other Unix-like systems - commonPaths = []string{ + trustedPaths = []string{ "/usr/local/bin/gh", "/usr/bin/gh", } } - for _, path := range commonPaths { - log.Printf("Checking: %s", path) - if _, err := os.Stat(path); err == nil { - log.Printf("Found gh at: %s", path) - return path, nil + var ghPath string + for _, path := range trustedPaths { + // Verify the file exists and is executable + if info, err := os.Stat(path); err == nil { + // Check if it's a regular file and executable + const executableMask = 0o111 + if info.Mode().IsRegular() && info.Mode()&executableMask != 0 { + // Verify it's actually the gh binary by running version command + cmd := exec.Command(path, "version") //nolint:noctx // Quick version check doesn't need context + output, err := cmd.Output() + if err == nil && strings.Contains(string(output), "gh version") { + log.Printf("Found and verified gh at: %s", path) + ghPath = path + break + } + } } } - return "", fmt.Errorf("gh cli not found, please install from https://cli.github.com") -} - -// githubToken retrieves the GitHub token using gh CLI -func (app *App) githubToken() (string, error) { - ghPath, err := findGHCommand() - if err != nil { - return "", err + if ghPath == "" { + return "", errors.New("gh cli not found in trusted locations, please install from https://cli.github.com") } - log.Printf("Executing: %s auth token", ghPath) - cmd := exec.Command(ghPath, "auth", "token") + log.Printf("Executing command: %s auth token", ghPath) + cmd := exec.CommandContext(ctx, ghPath, "auth", "token") output, err := cmd.CombinedOutput() if err != nil { log.Printf("gh command failed with output: %s", string(output)) @@ -117,34 +107,26 @@ func (app *App) githubToken() (string, error) { } token := strings.TrimSpace(string(output)) if token == "" { - return "", fmt.Errorf("empty github token") + return "", errors.New("empty github token") } - if len(token) < 20 { + const minTokenLength = 20 + if len(token) < minTokenLength { return "", fmt.Errorf("invalid github token length: %d", len(token)) } - log.Printf("Successfully obtained GitHub token (length: %d)", len(token)) + log.Println("Successfully obtained GitHub token") return token, nil } -// loadCurrentUser loads the authenticated GitHub user -func (app *App) loadCurrentUser() error { - user, _, err := app.client.Users.Get(app.ctx, "") - if err != nil { - return fmt.Errorf("get current user: %w", err) - } - app.currentUser = user - return nil -} - -// fetchPRs retrieves all PRs involving the current user -func (app *App) fetchPRs() ([]PR, []PR, error) { +// fetchPRs retrieves all PRs involving the current user. +func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err error) { user := app.currentUser.GetLogin() // Single query to get all PRs involving the user query := fmt.Sprintf("is:open is:pr involves:%s archived:false", user) + const perPage = 100 opts := &github.SearchOptions{ - ListOptions: github.ListOptions{PerPage: 100}, + ListOptions: github.ListOptions{PerPage: perPage}, Sort: "updated", Order: "desc", } @@ -152,74 +134,64 @@ func (app *App) fetchPRs() ([]PR, []PR, error) { log.Printf("Searching for PRs with query: %s", query) searchStart := time.Now() - result, _, err := app.client.Search.Issues(app.ctx, query, opts) + // Just try once - GitHub API is reliable enough + result, resp, err := app.client.Search.Issues(ctx, query, opts) if err != nil { + // Check for rate limit + const httpStatusForbidden = 403 + if resp != nil && resp.StatusCode == httpStatusForbidden { + log.Print("GitHub API rate limited") + } return nil, nil, fmt.Errorf("search PRs: %w", err) } log.Printf("GitHub search completed in %v, found %d PRs", time.Since(searchStart), len(result.Issues)) - // Process results - prMap := make(map[int64]*github.Issue, len(result.Issues)) - for _, issue := range result.Issues { - if issue.IsPullRequest() { - prMap[issue.GetID()] = issue - } + // Limit PRs for performance + if len(result.Issues) > maxPRsToProcess { + log.Printf("Limiting to %d PRs for performance (total: %d)", maxPRsToProcess, len(result.Issues)) + result.Issues = result.Issues[:maxPRsToProcess] } - var incoming, outgoing []PR - turnStart := time.Now() + // Process results turnSuccesses := 0 turnFailures := 0 - log.Printf("Processing %d PRs for Turn API status", len(prMap)) - - for _, issue := range prMap { + for _, issue := range result.Issues { + if !issue.IsPullRequest() { + continue + } repo := strings.TrimPrefix(issue.GetRepositoryURL(), "https://api.github.com/repos/") pr := PR{ - ID: issue.GetID(), - Number: issue.GetNumber(), Title: issue.GetTitle(), URL: issue.GetHTMLURL(), - User: issue.GetUser(), Repository: repo, + Number: issue.GetNumber(), UpdatedAt: issue.GetUpdatedAt().Time, } // Get Turn API data with caching - turnData, err := app.getTurnData(issue.GetHTMLURL(), issue.GetUpdatedAt().Time) - if err == nil && turnData != nil { + turnData, err := app.turnData(ctx, issue.GetHTMLURL(), issue.GetUpdatedAt().Time) + if err == nil && turnData != nil && turnData.PRState.UnblockAction != nil { turnSuccesses++ - pr.Tags = turnData.PRState.Tags - pr.Size = turnData.PRState.Size - - // Check if user is in UnblockAction - if turnData.PRState.UnblockAction != nil { - if _, exists := turnData.PRState.UnblockAction[user]; exists { - pr.NeedsReview = true - log.Printf("PR %s #%d needs review from %s", repo, issue.GetNumber(), user) - } + if _, exists := turnData.PRState.UnblockAction[user]; exists { + pr.NeedsReview = true } } else if err != nil { turnFailures++ - log.Printf("Turn API error for %s #%d: %v", repo, issue.GetNumber(), err) } // Categorize as incoming or outgoing if issue.GetUser().GetLogin() == user { pr.IsBlocked = pr.NeedsReview outgoing = append(outgoing, pr) - log.Printf("Outgoing PR: %s #%d (blocked: %v)", repo, issue.GetNumber(), pr.IsBlocked) } else { incoming = append(incoming, pr) - log.Printf("Incoming PR: %s #%d (needs review: %v)", repo, issue.GetNumber(), pr.NeedsReview) } } - log.Printf("Turn API calls completed in %v (successes: %d, failures: %d)", - time.Since(turnStart), turnSuccesses, turnFailures) - log.Printf("Final count: %d incoming, %d outgoing PRs", len(incoming), len(outgoing)) - + log.Printf("Found %d incoming, %d outgoing PRs (Turn API: %d/%d succeeded)", + len(incoming), len(outgoing), turnSuccesses, turnSuccesses+turnFailures) return incoming, outgoing, nil } diff --git a/loginitem_darwin.go b/loginitem_darwin.go index c1a2722..3ab6c82 100644 --- a/loginitem_darwin.go +++ b/loginitem_darwin.go @@ -1,10 +1,11 @@ //go:build darwin -// +build darwin // Package main - loginitem_darwin.go provides macOS-specific login item management. package main import ( + "context" + "errors" "fmt" "log" "os" @@ -15,8 +16,18 @@ import ( "github.com/energye/systray" ) -// isLoginItem checks if the app is set to start at login -func isLoginItem() bool { +// escapeAppleScriptString safely escapes a string for use in AppleScript. +// This prevents injection attacks by escaping quotes and backslashes. +func escapeAppleScriptString(s string) string { + // Escape backslashes first + s = strings.ReplaceAll(s, `\`, `\\`) + // Then escape quotes + s = strings.ReplaceAll(s, `"`, `\"`) + return s +} + +// isLoginItem checks if the app is set to start at login. +func isLoginItem(ctx context.Context) bool { appPath, err := getAppPath() if err != nil { log.Printf("Failed to get app path: %v", err) @@ -24,8 +35,14 @@ func isLoginItem() bool { } // Use osascript to check login items - script := fmt.Sprintf(`tell application "System Events" to get the name of every login item where path is "%s"`, appPath) - cmd := exec.Command("osascript", "-e", script) + escapedPath := escapeAppleScriptString(appPath) + // We use %s here because the string is already escaped by escapeAppleScriptString + //nolint:gocritic // already escaped + script := fmt.Sprintf( + `tell application "System Events" to get the name of every login item where path is "%s"`, + escapedPath) + log.Printf("Executing command: osascript -e %q", script) + cmd := exec.CommandContext(ctx, "osascript", "-e", script) output, err := cmd.CombinedOutput() if err != nil { log.Printf("Failed to check login items: %v", err) @@ -36,8 +53,8 @@ func isLoginItem() bool { return result != "" } -// setLoginItem adds or removes the app from login items -func setLoginItem(enable bool) error { +// setLoginItem adds or removes the app from login items. +func setLoginItem(ctx context.Context, enable bool) error { appPath, err := getAppPath() if err != nil { return fmt.Errorf("get app path: %w", err) @@ -45,8 +62,14 @@ func setLoginItem(enable bool) error { if enable { // Add to login items - script := fmt.Sprintf(`tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`, appPath) - cmd := exec.Command("osascript", "-e", script) + escapedPath := escapeAppleScriptString(appPath) + // We use %s here because the string is already escaped by escapeAppleScriptString + //nolint:gocritic // already escaped + script := fmt.Sprintf( + `tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`, + escapedPath) + log.Printf("Executing command: osascript -e %q", script) + cmd := exec.CommandContext(ctx, "osascript", "-e", script) if output, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("add login item: %w (output: %s)", err, string(output)) } @@ -55,8 +78,11 @@ func setLoginItem(enable bool) error { // Remove from login items appName := filepath.Base(appPath) appName = strings.TrimSuffix(appName, ".app") - script := fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, appName) - cmd := exec.Command("osascript", "-e", script) + escapedName := escapeAppleScriptString(appName) + // We use %s here because the string is already escaped by escapeAppleScriptString + script := fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, escapedName) //nolint:gocritic // already escaped + log.Printf("Executing command: osascript -e %q", script) + cmd := exec.CommandContext(ctx, "osascript", "-e", script) if output, err := cmd.CombinedOutput(); err != nil { // Ignore error if item doesn't exist if !strings.Contains(string(output), "Can't get login item") { @@ -69,7 +95,7 @@ func setLoginItem(enable bool) error { return nil } -// isRunningFromAppBundle checks if the app is running from a .app bundle +// isRunningFromAppBundle checks if the app is running from a .app bundle. func isRunningFromAppBundle() bool { execPath, err := os.Executable() if err != nil { @@ -87,7 +113,7 @@ func isRunningFromAppBundle() bool { return strings.Contains(execPath, ".app/Contents/MacOS/") } -// getAppPath returns the path to the application bundle +// getAppPath returns the path to the application bundle. func getAppPath() (string, error) { // Get the executable path execPath, err := os.Executable() @@ -112,11 +138,11 @@ func getAppPath() (string, error) { } // Not running from an app bundle, return empty string to indicate this - return "", fmt.Errorf("not running from app bundle") + return "", errors.New("not running from app bundle") } -// addLoginItemUI adds the login item menu option (macOS only) -func addLoginItemUI(app *App) { +// addLoginItemUI adds the login item menu option (macOS only). +func addLoginItemUI(ctx context.Context, app *App) { // Only show login item menu if running from an app bundle if !isRunningFromAppBundle() { log.Println("Hiding 'Start at Login' menu item - not running from app bundle") @@ -127,15 +153,15 @@ func addLoginItemUI(app *App) { app.menuItems = append(app.menuItems, loginItem) // Set initial state - if isLoginItem() { + if isLoginItem(ctx) { loginItem.Check() } loginItem.Click(func() { - isEnabled := isLoginItem() + isEnabled := isLoginItem(ctx) newState := !isEnabled - if err := setLoginItem(newState); err != nil { + if err := setLoginItem(ctx, newState); err != nil { log.Printf("Failed to set login item: %v", err) // Revert the UI state on error if isEnabled { diff --git a/loginitem_other.go b/loginitem_other.go index 7428795..0ddb8a2 100644 --- a/loginitem_other.go +++ b/loginitem_other.go @@ -4,7 +4,9 @@ // Package main - loginitem_other.go provides stub functions for non-macOS platforms. package main +import "context" + // addLoginItemUI is a no-op on non-macOS platforms -func addLoginItemUI(app *App) { +func addLoginItemUI(ctx context.Context, app *App) { // Login items are only supported on macOS } diff --git a/main.go b/main.go index af391fb..9bd74fb 100644 --- a/main.go +++ b/main.go @@ -6,13 +6,11 @@ package main import ( "context" - "crypto/sha256" _ "embed" "fmt" "log" "os" "path/filepath" - "strings" "sync" "time" @@ -25,86 +23,102 @@ import ( //go:embed menubar-icon.png var embeddedIcon []byte +// Version information - set during build with -ldflags. +var ( + version = "dev" + commit = "unknown" + date = "unknown" +) + const ( - // Cache settings + // Cache settings. cacheTTL = 2 * time.Hour cacheCleanupInterval = 5 * 24 * time.Hour - // PR settings + // PR settings. stalePRThreshold = 90 * 24 * time.Hour + maxPRsToProcess = 200 // Limit for performance - // Update intervals - updateInterval = 2 * time.Minute + // Update intervals. + updateInterval = 5 * time.Minute // Reduced frequency to avoid rate limits ) -// PR represents a pull request with metadata +// PR represents a pull request with metadata. type PR struct { - ID int64 `json:"id"` - Number int `json:"number"` - Title string `json:"title"` - URL string `json:"url"` - User *github.User - Repository string UpdatedAt time.Time + Title string + URL string + Repository string + Number int IsBlocked bool NeedsReview bool - Size string - Tags []string } -// App holds the application state +// App holds the application state. type App struct { - client *github.Client - turnClient *turn.Client - currentUser *github.User - ctx context.Context - incoming []PR - outgoing []PR - menuItems []*systray.MenuItem - cacheDir string - hideStaleIncoming bool - previousBlockedPRs map[string]bool // Track previously blocked PRs by URL - lastMenuHash string // Hash of last menu state to detect changes - mu sync.RWMutex // Protects concurrent access to PR data + lastSuccessfulFetch time.Time + turnClient *turn.Client + currentUser *github.User + previousBlockedPRs map[string]bool + client *github.Client + cacheDir string + lastMenuHash string + incoming []PR + menuItems []*systray.MenuItem + outgoing []PR + consecutiveFailures int + mu sync.RWMutex + hideStaleIncoming bool } func main() { log.SetFlags(log.LstdFlags | log.Lshortfile) - log.Println("Starting GitHub PR Monitor") + log.Printf("Starting GitHub PR Monitor (version=%s, commit=%s, date=%s)", version, commit, date) + + ctx := context.Background() cacheDir, err := os.UserCacheDir() if err != nil { log.Fatalf("Failed to get cache directory: %v", err) } cacheDir = filepath.Join(cacheDir, "ready-to-review") - if err := os.MkdirAll(cacheDir, 0o755); err != nil { + const dirPerm = 0o700 // Only owner can access cache directory + if err := os.MkdirAll(cacheDir, dirPerm); err != nil { log.Fatalf("Failed to create cache directory: %v", err) } app := &App{ - ctx: context.Background(), cacheDir: cacheDir, hideStaleIncoming: true, previousBlockedPRs: make(map[string]bool), } log.Println("Initializing GitHub clients...") - err = app.initClients() + err = app.initClients(ctx) if err != nil { log.Fatalf("Failed to initialize clients: %v", err) } log.Println("Loading current user...") - err = app.loadCurrentUser() + user, _, err := app.client.Users.Get(ctx, "") if err != nil { log.Fatalf("Failed to load current user: %v", err) } + app.currentUser = user log.Println("Starting systray...") - systray.Run(app.onReady, app.onExit) + // Create a cancellable context for the application + appCtx, cancel := context.WithCancel(ctx) + defer cancel() + + systray.Run(func() { app.onReady(appCtx) }, func() { + log.Println("Shutting down application") + cancel() // Cancel the context to stop goroutines + app.cleanupOldCache() + }) } -func (app *App) onReady() { +func (app *App) onReady(ctx context.Context) { log.Println("System tray ready") systray.SetIcon(embeddedIcon) systray.SetTitle("Loading PRs...") @@ -114,185 +128,169 @@ func (app *App) onReady() { systray.SetOnClick(func(menu systray.IMenu) { log.Println("Icon clicked") if menu != nil { - menu.ShowMenu() + if err := menu.ShowMenu(); err != nil { + log.Printf("Failed to show menu: %v", err) + } } }) systray.SetOnRClick(func(menu systray.IMenu) { log.Println("Right click detected") if menu != nil { - menu.ShowMenu() + if err := menu.ShowMenu(); err != nil { + log.Printf("Failed to show menu: %v", err) + } } }) // Create initial menu log.Println("Creating initial menu") - app.updateMenu() + app.updateMenu(ctx) - // Start cache cleanup - app.startCacheCleanup() + // Clean old cache on startup + app.cleanupOldCache() // Start update loop - go app.updateLoop() + go app.updateLoop(ctx) } -func (app *App) onExit() { - log.Println("Shutting down application") - app.cleanupOldCache() -} +func (app *App) updateLoop(ctx context.Context) { + // Recover from panics to keep the update loop running + defer func() { + if r := recover(); r != nil { + log.Printf("PANIC in update loop: %v", r) + // Restart the update loop after a delay + time.Sleep(10 * time.Second) + go app.updateLoop(ctx) + } + }() -func (app *App) updateLoop() { ticker := time.NewTicker(updateInterval) defer ticker.Stop() // Initial update - app.updatePRs() - - for range ticker.C { - log.Println("Running scheduled PR update") - app.updatePRs() + app.updatePRs(ctx) + + for { + select { + case <-ticker.C: + log.Println("Running scheduled PR update") + app.updatePRs(ctx) + case <-ctx.Done(): + log.Println("Update loop stopping due to context cancellation") + return + } } } -func (app *App) updatePRs() { - incoming, outgoing, err := app.fetchPRs() +func (app *App) updatePRs(ctx context.Context) { + incoming, outgoing, err := app.fetchPRs(ctx) if err != nil { log.Printf("Error fetching PRs: %v", err) - systray.SetTitle("Error") + app.mu.Lock() + app.consecutiveFailures++ + app.mu.Unlock() + + // Show different icon based on failure count + if app.consecutiveFailures > 3 { + systray.SetTitle("❌") // Complete failure + } else { + systray.SetTitle("⚠️") // Warning + } + + // Include time since last success in tooltip + timeSinceSuccess := "never" + if !app.lastSuccessfulFetch.IsZero() { + timeSinceSuccess = time.Since(app.lastSuccessfulFetch).Round(time.Minute).String() + } + systray.SetTooltip(fmt.Sprintf("GitHub PR Monitor - Error: %v\nLast success: %s ago", err, timeSinceSuccess)) return } - // Check for newly blocked PRs - var newlyBlockedPRs []PR - currentBlockedPRs := make(map[string]bool) + // Update health status on success + app.mu.Lock() + app.lastSuccessfulFetch = time.Now() + app.consecutiveFailures = 0 + app.mu.Unlock() - // Check incoming PRs - for _, pr := range incoming { - if pr.NeedsReview { - currentBlockedPRs[pr.URL] = true - // Check if this is newly blocked - app.mu.RLock() - wasBlocked := app.previousBlockedPRs[pr.URL] - app.mu.RUnlock() - if !wasBlocked { - newlyBlockedPRs = append(newlyBlockedPRs, pr) - } - } - } + // Check for newly blocked PRs and send notifications + app.mu.RLock() + oldBlockedPRs := app.previousBlockedPRs + app.mu.RUnlock() - // Check outgoing PRs - for _, pr := range outgoing { - if pr.IsBlocked { - currentBlockedPRs[pr.URL] = true - // Check if this is newly blocked - app.mu.RLock() - wasBlocked := app.previousBlockedPRs[pr.URL] - app.mu.RUnlock() - if !wasBlocked { - newlyBlockedPRs = append(newlyBlockedPRs, pr) + currentBlockedPRs := make(map[string]bool) + var incomingBlocked, outgoingBlocked int + + // Count blocked PRs and send notifications + for i := range incoming { + if incoming[i].NeedsReview { + currentBlockedPRs[incoming[i].URL] = true + if !app.hideStaleIncoming || !isStale(incoming[i].UpdatedAt) { + incomingBlocked++ + } + // Send notification for newly blocked + if !oldBlockedPRs[incoming[i].URL] { + if err := beeep.Notify("PR Blocked on You", + fmt.Sprintf("%s #%d: %s", incoming[i].Repository, incoming[i].Number, incoming[i].Title), ""); err != nil { + log.Printf("Failed to send notification: %v", err) + } } } } - // Send notifications for newly blocked PRs - for _, pr := range newlyBlockedPRs { - title := "PR Blocked on You" - message := fmt.Sprintf("%s #%d: %s", pr.Repository, pr.Number, pr.Title) - if err := beeep.Notify(title, message, ""); err != nil { - log.Printf("Failed to send notification: %v", err) + for i := range outgoing { + if outgoing[i].IsBlocked { + currentBlockedPRs[outgoing[i].URL] = true + if !app.hideStaleIncoming || !isStale(outgoing[i].UpdatedAt) { + outgoingBlocked++ + } + // Send notification for newly blocked + if !oldBlockedPRs[outgoing[i].URL] { + if err := beeep.Notify("PR Blocked on You", + fmt.Sprintf("%s #%d: %s", outgoing[i].Repository, outgoing[i].Number, outgoing[i].Title), ""); err != nil { + log.Printf("Failed to send notification: %v", err) + } + } } } - // Update the previous blocked PRs map + // Update state app.mu.Lock() app.previousBlockedPRs = currentBlockedPRs app.incoming = incoming app.outgoing = outgoing app.mu.Unlock() - incomingBlocked := 0 - outgoingBlocked := 0 - - app.mu.RLock() - for _, pr := range app.incoming { - // Skip stale PRs if hiding them - if app.hideStaleIncoming && isStale(pr.UpdatedAt) { - continue - } - if pr.NeedsReview { - incomingBlocked++ - } - } - - for _, pr := range app.outgoing { - // Skip stale PRs if hiding them - if app.hideStaleIncoming && isStale(pr.UpdatedAt) { - continue - } - if pr.IsBlocked { - outgoingBlocked++ - } - } - app.mu.RUnlock() - // Set title based on PR state systray.SetIcon(embeddedIcon) - if incomingBlocked == 0 && outgoingBlocked == 0 { + switch { + case incomingBlocked == 0 && outgoingBlocked == 0: systray.SetTitle("") - } else if incomingBlocked > 0 { + case incomingBlocked > 0: systray.SetTitle(fmt.Sprintf("%d/%d 🔴", incomingBlocked, outgoingBlocked)) - } else { + default: systray.SetTitle(fmt.Sprintf("0/%d 🚀", outgoingBlocked)) } - app.updateMenuIfChanged() + app.updateMenuIfChanged(ctx) } -// isStale returns true if the PR hasn't been updated in over 90 days +// isStale returns true if the PR hasn't been updated in over 90 days. func isStale(updatedAt time.Time) bool { return time.Since(updatedAt) > stalePRThreshold } -// generateMenuHash creates a hash of the current menu state to detect changes -func (app *App) generateMenuHash() string { - var builder strings.Builder - +// updateMenuIfChanged only rebuilds the menu if the PR data has actually changed. +func (app *App) updateMenuIfChanged(ctx context.Context) { app.mu.RLock() - defer app.mu.RUnlock() - - // Include hideStaleIncoming setting - builder.WriteString(fmt.Sprintf("hide:%v|", app.hideStaleIncoming)) - - // Hash incoming PRs (filtered by stale setting) - builder.WriteString("incoming:") - for _, pr := range app.incoming { - if !app.hideStaleIncoming || !isStale(pr.UpdatedAt) { - builder.WriteString(fmt.Sprintf("%d:%s:%v:%s|", pr.ID, pr.Repository, pr.NeedsReview, pr.Size)) - } - } - - // Hash outgoing PRs (filtered by stale setting) - builder.WriteString("outgoing:") - for _, pr := range app.outgoing { - if !app.hideStaleIncoming || !isStale(pr.UpdatedAt) { - builder.WriteString(fmt.Sprintf("%d:%s:%v|", pr.ID, pr.Repository, pr.IsBlocked)) - } - } - - // Generate SHA256 hash - hash := sha256.Sum256([]byte(builder.String())) - return fmt.Sprintf("%x", hash) -} + // Simple hash: just count of PRs and hideStale setting + currentHash := fmt.Sprintf("%d-%d-%t", len(app.incoming), len(app.outgoing), app.hideStaleIncoming) + app.mu.RUnlock() -// updateMenuIfChanged only rebuilds the menu if the PR data has actually changed -func (app *App) updateMenuIfChanged() { - currentHash := app.generateMenuHash() if currentHash == app.lastMenuHash { - log.Println("Menu data unchanged, skipping menu rebuild") return } - log.Println("Menu data changed, rebuilding menu") app.lastMenuHash = currentHash - app.updateMenu() + app.updateMenu(ctx) } diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..ee05a9e --- /dev/null +++ b/main_test.go @@ -0,0 +1,75 @@ +package main + +import ( + "testing" + "time" +) + +func TestIsStale(t *testing.T) { + tests := []struct { + name string + time time.Time + expected bool + }{ + { + name: "recent PR", + time: time.Now().Add(-24 * time.Hour), + expected: false, + }, + { + name: "stale PR", + time: time.Now().Add(-91 * 24 * time.Hour), + expected: true, + }, + { + name: "exactly at threshold", + time: time.Now().Add(-90 * 24 * time.Hour), + expected: true, // >= 90 days is stale + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isStale(tt.time); got != tt.expected { + t.Errorf("isStale() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestEscapeAppleScriptString(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "simple string", + input: "hello world", + expected: "hello world", + }, + { + name: "string with quotes", + input: `hello "world"`, + expected: `hello \"world\"`, + }, + { + name: "string with backslashes", + input: `hello\world`, + expected: `hello\\world`, + }, + { + name: "complex string", + input: `path\to\"file"`, + expected: `path\\to\\\"file\"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := escapeAppleScriptString(tt.input); got != tt.expected { + t.Errorf("escapeAppleScriptString() = %v, want %v", got, tt.expected) + } + }) + } +} diff --git a/ui.go b/ui.go index e1660a3..74d5fc4 100644 --- a/ui.go +++ b/ui.go @@ -2,6 +2,8 @@ package main import ( + "context" + "errors" "fmt" "log" "net/url" @@ -12,135 +14,187 @@ import ( "github.com/energye/systray" ) -// formatAge formats a duration in human-readable form +// formatAge formats a duration in human-readable form. func formatAge(updatedAt time.Time) string { duration := time.Since(updatedAt) - if duration < time.Minute { - seconds := int(duration.Seconds()) - if seconds == 1 { - return "1 second" - } - return fmt.Sprintf("%d seconds", seconds) - } else if duration < time.Hour { - minutes := int(duration.Minutes()) - if minutes == 1 { - return "1 minute" - } - return fmt.Sprintf("%d minutes", minutes) - } else if duration < 24*time.Hour { - hours := int(duration.Hours()) - if hours == 1 { - return "1 hour" - } - return fmt.Sprintf("%d hours", hours) - } else if duration < 30*24*time.Hour { - days := int(duration.Hours() / 24) - if days == 1 { - return "1 day" - } - return fmt.Sprintf("%d days", days) - } else if duration < 365*24*time.Hour { - months := int(duration.Hours() / (24 * 30)) - if months == 1 { - return "1 month" - } - return fmt.Sprintf("%d months", months) - } else { - // For PRs older than a year, show the year + switch { + case duration < time.Hour: + return fmt.Sprintf("%dm", int(duration.Minutes())) + case duration < 24*time.Hour: + return fmt.Sprintf("%dh", int(duration.Hours())) + case duration < 30*24*time.Hour: + return fmt.Sprintf("%dd", int(duration.Hours()/24)) + case duration < 365*24*time.Hour: + return fmt.Sprintf("%dmo", int(duration.Hours()/(24*30))) + default: return updatedAt.Format("2006") } } -// openURL safely opens a URL in the default browser after validation -func openURL(rawURL string) error { +// openURL safely opens a URL in the default browser after validation. +func openURL(ctx context.Context, rawURL string) error { // Parse and validate the URL u, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("parse url: %w", err) } - // Only allow https URLs for security + // Security validation: scheme, host whitelist, no userinfo if u.Scheme != "https" { return fmt.Errorf("invalid url scheme: %s (only https allowed)", u.Scheme) } - // Whitelist of allowed hosts allowedHosts := map[string]bool{ "github.com": true, "www.github.com": true, "dash.ready-to-review.dev": true, } - if !allowedHosts[u.Host] { return fmt.Errorf("invalid host: %s", u.Host) } + if u.User != nil { + return errors.New("URLs with user info are not allowed") + } + // Execute the open command based on OS + // Use safer methods that don't invoke shell interpretation var cmd *exec.Cmd switch runtime.GOOS { case "darwin": - cmd = exec.Command("open", rawURL) + // Use open command with explicit arguments to prevent injection + log.Printf("Executing command: /usr/bin/open -u %q", rawURL) + cmd = exec.CommandContext(ctx, "/usr/bin/open", "-u", rawURL) case "windows": - cmd = exec.Command("cmd", "/c", "start", rawURL) + // Use rundll32 to open URL safely without cmd shell + log.Printf("Executing command: rundll32.exe url.dll,FileProtocolHandler %q", rawURL) + cmd = exec.CommandContext(ctx, "rundll32.exe", "url.dll,FileProtocolHandler", rawURL) case "linux": - cmd = exec.Command("xdg-open", rawURL) + // Use xdg-open with full path + log.Printf("Executing command: /usr/bin/xdg-open %q", rawURL) + cmd = exec.CommandContext(ctx, "/usr/bin/xdg-open", rawURL) default: // Try xdg-open for other Unix-like systems - cmd = exec.Command("xdg-open", rawURL) + log.Printf("Executing command: /usr/bin/xdg-open %q", rawURL) + cmd = exec.CommandContext(ctx, "/usr/bin/xdg-open", rawURL) } if err := cmd.Start(); err != nil { return fmt.Errorf("open url: %w", err) } - // Don't wait for the command to finish + // Don't wait for the command to finish, but add timeout to prevent goroutine leak go func() { - cmd.Wait() // Prevent zombie processes + // Kill the process after 30 seconds if it hasn't finished + timer := time.AfterFunc(30*time.Second, func() { + if cmd.Process != nil { + if err := cmd.Process.Kill(); err != nil { + log.Printf("Failed to kill process: %v", err) + } + } + }) + defer timer.Stop() + + if err := cmd.Wait(); err != nil { + log.Printf("Failed to wait for command: %v", err) + } }() return nil } -// updateMenu rebuilds the system tray menu -func (app *App) updateMenu() { +// countPRs counts the number of PRs that need review/are blocked. +// Returns: incomingCount, incomingBlocked, outgoingCount, outgoingBlocked +// +//nolint:revive,gocritic // 4 return values is clearer than a struct here +func (app *App) countPRs() (int, int, int, int) { app.mu.RLock() - incomingLen := len(app.incoming) - outgoingLen := len(app.outgoing) - app.mu.RUnlock() - - log.Printf("Updating menu with %d incoming and %d outgoing PRs", incomingLen, outgoingLen) - - // Store the current menu items to clean up later - oldMenuItems := app.menuItems - app.menuItems = nil + defer app.mu.RUnlock() - // Calculate counts first - incomingBlocked := 0 - incomingCount := 0 + var incomingCount, incomingBlocked, outgoingCount, outgoingBlocked int - app.mu.RLock() - for _, pr := range app.incoming { - if !app.hideStaleIncoming || !isStale(pr.UpdatedAt) { + for i := range app.incoming { + if !app.hideStaleIncoming || !isStale(app.incoming[i].UpdatedAt) { incomingCount++ - if pr.NeedsReview { + if app.incoming[i].NeedsReview { incomingBlocked++ } } } - outgoingBlocked := 0 - outgoingCount := 0 - for _, pr := range app.outgoing { - if !app.hideStaleIncoming || !isStale(pr.UpdatedAt) { + for i := range app.outgoing { + if !app.hideStaleIncoming || !isStale(app.outgoing[i].UpdatedAt) { outgoingCount++ - if pr.IsBlocked { + if app.outgoing[i].IsBlocked { outgoingBlocked++ } } } + return incomingCount, incomingBlocked, outgoingCount, outgoingBlocked +} + +// addPRMenuItem adds a menu item for a pull request. +func (app *App) addPRMenuItem(ctx context.Context, pr PR, isOutgoing bool) { + title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) + if (!isOutgoing && pr.NeedsReview) || (isOutgoing && pr.IsBlocked) { + if isOutgoing { + title = fmt.Sprintf("%s 🚀", title) + } else { + title = fmt.Sprintf("%s 🔴", title) + } + } + tooltip := fmt.Sprintf("%s (%s)", pr.Title, formatAge(pr.UpdatedAt)) + item := systray.AddMenuItem(title, tooltip) + app.menuItems = append(app.menuItems, item) + + // Capture URL and context properly to avoid loop variable capture bug + item.Click(func(capturedCtx context.Context, url string) func() { + return func() { + if err := openURL(capturedCtx, url); err != nil { + log.Printf("failed to open url: %v", err) + } + } + }(ctx, pr.URL)) +} + +// addPRSection adds a section of PRs to the menu. +func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, blockedCount int, isOutgoing bool) { + if len(prs) == 0 { + return + } + + // Add header + header := systray.AddMenuItem(fmt.Sprintf("%s — %d blocked on you", sectionTitle, blockedCount), "") + header.Disable() + app.menuItems = append(app.menuItems, header) + + // Add PR items (already protected by mutex in caller) + for i := range prs { + // Apply filters + if app.hideStaleIncoming && isStale(prs[i].UpdatedAt) { + continue + } + app.addPRMenuItem(ctx, prs[i], isOutgoing) + } +} + +// updateMenu rebuilds the system tray menu. +func (app *App) updateMenu(ctx context.Context) { + app.mu.RLock() + incomingLen := len(app.incoming) + outgoingLen := len(app.outgoing) app.mu.RUnlock() + log.Printf("Updating menu with %d incoming and %d outgoing PRs", incomingLen, outgoingLen) + + // Store the current menu items to clean up later + oldMenuItems := app.menuItems + app.menuItems = nil + + // Calculate counts first + incomingCount, incomingBlocked, outgoingCount, outgoingBlocked := app.countPRs() + // Show "No pull requests" if both lists are empty if incomingLen == 0 && outgoingLen == 0 { noPRs := systray.AddMenuItem("No pull requests", "") @@ -149,85 +203,30 @@ func (app *App) updateMenu() { systray.AddSeparator() } - // Incoming section - clean header + // Incoming section if incomingCount > 0 { - incomingHeader := systray.AddMenuItem(fmt.Sprintf("Incoming — %d blocked on you", incomingBlocked), "") - incomingHeader.Disable() - app.menuItems = append(app.menuItems, incomingHeader) - } - - if incomingCount > 0 { - app.mu.RLock() - prs := make([]PR, len(app.incoming)) - copy(prs, app.incoming) - app.mu.RUnlock() - - for _, pr := range prs { - // Apply filters - if app.hideStaleIncoming && isStale(pr.UpdatedAt) { - continue - } - title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) - if pr.Size != "" { - title = fmt.Sprintf("%s – %s", title, pr.Size) - } - if pr.NeedsReview { - title = fmt.Sprintf("%s 🔴", title) - } - tooltip := fmt.Sprintf("%s by %s (%s)", pr.Title, pr.User.GetLogin(), formatAge(pr.UpdatedAt)) - item := systray.AddMenuItem(title, tooltip) - app.menuItems = append(app.menuItems, item) - - // Capture URL properly to avoid loop variable capture bug - item.Click(func(url string) func() { - return func() { - if err := openURL(url); err != nil { - log.Printf("failed to open url: %v", err) - } - } - }(pr.URL)) - } + app.addPRSection(ctx, app.incoming, "Incoming", incomingBlocked, false) } systray.AddSeparator() - // Outgoing section - clean header + // Outgoing section if outgoingCount > 0 { - outgoingHeader := systray.AddMenuItem(fmt.Sprintf("Outgoing — %d blocked on you", outgoingBlocked), "") - outgoingHeader.Disable() - app.menuItems = append(app.menuItems, outgoingHeader) + app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked, true) } - if outgoingCount > 0 { - app.mu.RLock() - prs := make([]PR, len(app.outgoing)) - copy(prs, app.outgoing) - app.mu.RUnlock() - - for _, pr := range prs { - // Apply stale filter to outgoing PRs - if app.hideStaleIncoming && isStale(pr.UpdatedAt) { - continue - } - title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) - if pr.IsBlocked { - title = fmt.Sprintf("%s 🚀", title) - } - tooltip := fmt.Sprintf("%s by %s (%s)", pr.Title, pr.User.GetLogin(), formatAge(pr.UpdatedAt)) - item := systray.AddMenuItem(title, tooltip) - app.menuItems = append(app.menuItems, item) - - // Capture URL properly to avoid loop variable capture bug - item.Click(func(url string) func() { - return func() { - if err := openURL(url); err != nil { - log.Printf("failed to open url: %v", err) - } - } - }(pr.URL)) - } + // Add static menu items + app.addStaticMenuItems(ctx) + + // Now hide old menu items after new ones are created + // This prevents the flicker by ensuring new items exist before old ones disappear + for _, item := range oldMenuItems { + item.Hide() } +} +// addStaticMenuItems adds the static menu items (hide stale, dashboard, about, quit). +func (app *App) addStaticMenuItems(ctx context.Context) { systray.AddSeparator() // Hide stale PRs @@ -246,7 +245,7 @@ func (app *App) updateMenu() { log.Printf("Hide stale PRs: %v", app.hideStaleIncoming) // Force menu rebuild since hideStaleIncoming changed app.lastMenuHash = "" - app.updateMenu() + app.updateMenu(ctx) }) systray.AddSeparator() @@ -255,7 +254,7 @@ func (app *App) updateMenu() { dashboardItem := systray.AddMenuItem("Dashboard", "") app.menuItems = append(app.menuItems, dashboardItem) dashboardItem.Click(func() { - if err := openURL("https://dash.ready-to-review.dev/"); err != nil { + if err := openURL(ctx, "https://dash.ready-to-review.dev/"); err != nil { log.Printf("failed to open dashboard: %v", err) } }) @@ -265,7 +264,7 @@ func (app *App) updateMenu() { app.menuItems = append(app.menuItems, aboutItem) aboutItem.Click(func() { log.Println("GitHub PR Monitor - A system tray app for tracking PR reviews") - if err := openURL("https://github.com/ready-to-review/pr-menubar"); err != nil { + if err := openURL(ctx, "https://github.com/ready-to-review/pr-menubar"); err != nil { log.Printf("failed to open about page: %v", err) } }) @@ -273,7 +272,7 @@ func (app *App) updateMenu() { systray.AddSeparator() // Add login item option (macOS only) - addLoginItemUI(app) + addLoginItemUI(ctx, app) // Quit quitItem := systray.AddMenuItem("Quit", "") @@ -282,10 +281,4 @@ func (app *App) updateMenu() { log.Println("Quit requested by user") systray.Quit() }) - - // Now hide old menu items after new ones are created - // This prevents the flicker by ensuring new items exist before old ones disappear - for _, item := range oldMenuItems { - item.Hide() - } }