Austinamoruso/update #37
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @austinamorusoyardstick, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a new update-checking feature for the application, which includes a dedicated UpdateChecker component, its integration into the user interface, and a new keybinding for manual checks. Alongside this new functionality, I've also focused on improving overall code quality by refactoring existing structures and performing extensive cleanup of formatting and unused code.
Highlights
- New Update Checking Feature: I've added a new
UpdateCheckercomponent that handles both background and on-demand checks for application updates. This component is integrated into the main application logic. - UI Integration for Updates: The application's
Menucomponent now visually indicates when an update is available, providing a clear signal to users. - New Keybinding for Updates: Users can now trigger an immediate update check using a new 'U' keybinding, enhancing control over the update process.
- Code Structure Refinements: I've refactored anonymous
Userstructs insession/git/pr_comments.goto named structs, which improves code readability and maintainability. - Whitespace and Import Cleanup: I've performed a comprehensive cleanup across numerous files, removing unused imports and fixing minor formatting inconsistencies, particularly extraneous tab characters.
- Markdown Rendering Improvements: I've significantly enhanced the markdown rendering capabilities in
ui/markdown.go, leading to a more accurate and visually appealing display of headers, lists, code blocks, tables, and inline formatting.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
austinamorusoyardstick
force-pushed
the
austinamoruso/update
branch
from
286a130 to
63a1749
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new update-checking feature, which is a great addition for keeping users informed. The implementation is well-integrated into the existing application structure. The PR also includes a significant number of code quality improvements across many files, mainly by removing extra whitespace and improving formatting, which enhances readability. My review focuses on a couple of areas in the new app/update_checker.go file to improve its robustness and adherence to Go best practices.
| count := 0 | ||
| if _, err := strings.NewReader(strings.TrimSpace(string(output))).Read([]byte{}); err == nil { | ||
| // Parse the count | ||
| if n, _ := strings.CutSuffix(strings.TrimSpace(string(output)), "\n"); n != "" { | ||
| // Simple atoi implementation | ||
| for _, c := range n { | ||
| if c >= '0' && c <= '9' { | ||
| count = count*10 + int(c-'0') | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return count, nil |
There was a problem hiding this comment.
This custom implementation for parsing an integer from the command output is unnecessarily complex and less robust than the standard library's strconv.Atoi. The check for a non-empty string is also convoluted. Using strconv.Atoi simplifies the code, improves readability, and provides proper error handling. You'll need to add "strconv" to your imports.
countStr := strings.TrimSpace(string(output))
if countStr == "" {
return 0, nil
}
count, err := strconv.Atoi(countStr)
if err != nil {
log.WarningLog.Printf("could not parse commit count from output '%s': %v", countStr, err)
return 0, nil
}
return count, nil| func getMainBranch(gitRoot string) string { | ||
| // Try to get the default branch from remote | ||
| cmd := exec.Command("sh", "-c", "git -C "+gitRoot+" remote show origin | sed -n '/HEAD branch/s/.*: //p'") | ||
| output, err := cmd.Output() | ||
| if err == nil && len(output) > 0 { | ||
| return strings.TrimSpace(string(output)) | ||
| } | ||
|
|
||
| // Fallback to common defaults | ||
| for _, branch := range []string{"main", "master"} { | ||
| cmd := exec.Command("git", "-C", gitRoot, "rev-parse", "--verify", "origin/"+branch) | ||
| if err := cmd.Run(); err == nil { | ||
| return branch | ||
| } | ||
| } | ||
|
|
||
| return "main" | ||
| } |
There was a problem hiding this comment.
The use of sh -c to determine the main branch name is not robust and can be platform-dependent. It also introduces a potential for shell injection if gitRoot were ever to come from an untrusted source. A more idiomatic and safer approach in Go is to execute commands directly without a shell wrapper. You could use git symbolic-ref refs/remotes/origin/HEAD to get the default branch name more reliably.
func getMainBranch(gitRoot string) string {
// Try to get the default branch from remote using symbolic-ref
cmd := exec.Command("git", "-C", gitRoot, "symbolic-ref", "refs/remotes/origin/HEAD")
output, err := cmd.Output()
if err == nil {
// Output is typically 'refs/remotes/origin/main', we need to extract 'main'
if parts := strings.Split(strings.TrimSpace(string(output)), "/"); len(parts) > 0 {
return parts[len(parts)-1]
}
}
// Fallback to common defaults
for _, branch := range []string{"main", "master"} {
cmd := exec.Command("git", "-C", gitRoot, "rev-parse", "--verify", "origin/"+branch)
if err := cmd.Run(); err == nil {
return branch
}
}
return "main"
}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable update-checking feature and includes several code quality improvements. The core logic for the new feature in app/update_checker.go is a good start, but it relies heavily on shelling out to external commands, which can be fragile. I've provided specific suggestions to make these shell commands more robust and portable.
The most critical issue is in the new getCommitCount function, which contains a buggy parsing implementation that can fail silently. I've provided a much simpler and more correct version using the standard library.
The other changes are mostly stylistic cleanups and are well-executed, improving the overall readability of the codebase.
| func getCommitCount(gitRoot, ref string) (int, error) { | ||
| cmd := exec.Command("git", "-C", gitRoot, "rev-list", "--count", ref) | ||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| count := 0 | ||
| if _, err := strings.NewReader(strings.TrimSpace(string(output))).Read([]byte{}); err == nil { | ||
| // Parse the count | ||
| if n, _ := strings.CutSuffix(strings.TrimSpace(string(output)), "\n"); n != "" { | ||
| // Simple atoi implementation | ||
| for _, c := range n { | ||
| if c >= '0' && c <= '9' { | ||
| count = count*10 + int(c-'0') | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return count, nil |
There was a problem hiding this comment.
The integer parsing logic in getCommitCount is overly complex and buggy. It re-implements atoi and uses a confusing method to check for an empty string. This can lead to silent failures if the git command returns non-numeric output. This can be simplified and made more robust using the standard library's strconv.Atoi.
func getCommitCount(gitRoot, ref string) (int, error) {
cmd := exec.Command("git", "-C", gitRoot, "rev-list", "--count", ref)
output, err := cmd.Output()
if err != nil {
return 0, err
}
countStr := strings.TrimSpace(string(output))
if countStr == "" {
return 0, nil
}
count, err := strconv.Atoi(countStr)
if err != nil {
return 0, fmt.Errorf("failed to parse commit count '%s': %w", countStr, err)
}
return count, nil
}| func getMainBranch(gitRoot string) string { | ||
| // Try to get the default branch from remote | ||
| cmd := exec.Command("sh", "-c", "git -C "+gitRoot+" remote show origin | sed -n '/HEAD branch/s/.*: //p'") | ||
| output, err := cmd.Output() | ||
| if err == nil && len(output) > 0 { | ||
| return strings.TrimSpace(string(output)) | ||
| } |
There was a problem hiding this comment.
The current implementation for getting the main branch name by shelling out to sh and using sed is fragile and not portable across different operating systems. It depends on the user having sh and sed available and on the specific output format of git remote show origin.
A more robust and portable approach is to use git symbolic-ref refs/remotes/origin/HEAD, which directly provides the default remote branch.
| func getMainBranch(gitRoot string) string { | |
| // Try to get the default branch from remote | |
| cmd := exec.Command("sh", "-c", "git -C "+gitRoot+" remote show origin | sed -n '/HEAD branch/s/.*: //p'") | |
| output, err := cmd.Output() | |
| if err == nil && len(output) > 0 { | |
| return strings.TrimSpace(string(output)) | |
| } | |
| func getMainBranch(gitRoot string) string { | |
| cmd := exec.Command("git", "-C", gitRoot, "symbolic-ref", "refs/remotes/origin/HEAD") | |
| output, err := cmd.Output() | |
| if err == nil { | |
| refName := strings.TrimSpace(string(output)) | |
| if parts := strings.Split(refName, "/"); len(parts) > 0 { | |
| return parts[len(parts)-1] | |
| } | |
| } |
| func (uc *UpdateChecker) StartBackgroundCheck() { | ||
| go func() { | ||
| // Do initial check | ||
| uc.checkForUpdates() | ||
|
|
||
| // Set up periodic checking | ||
| ticker := time.NewTicker(uc.checkInterval) | ||
| defer ticker.Stop() | ||
|
|
||
| for range ticker.C { | ||
| uc.checkForUpdates() | ||
| } | ||
| }() |
There was a problem hiding this comment.
StartBackgroundCheck starts a goroutine with a time.NewTicker that runs indefinitely. However, there is no corresponding Stop method to halt this goroutine. If an UpdateChecker instance is ever replaced or no longer needed, this goroutine will leak.
It's a good practice to provide a way to clean up long-running goroutines. Consider adding a done channel to the UpdateChecker struct and a Stop() method to signal the goroutine to exit.
| func findGitRoot(startPath string) string { | ||
| current := startPath | ||
| for current != "/" && current != "" { | ||
| cmd := exec.Command("git", "-C", current, "rev-parse", "--show-toplevel") | ||
| output, err := cmd.Output() | ||
| if err == nil { | ||
| return strings.TrimSpace(string(output)) | ||
| } | ||
|
|
||
| // Go up one directory | ||
| if idx := strings.LastIndex(current, "/"); idx > 0 { | ||
| current = current[:idx] | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
| return "" |
There was a problem hiding this comment.
The findGitRoot function manually walks up the directory tree using string manipulation on paths, which is not portable (e.g., current != "/" and strings.LastIndex(current, "/") assume Unix-style paths). This can be made more robust and cross-platform by using the path/filepath package.
| func findGitRoot(startPath string) string { | |
| current := startPath | |
| for current != "/" && current != "" { | |
| cmd := exec.Command("git", "-C", current, "rev-parse", "--show-toplevel") | |
| output, err := cmd.Output() | |
| if err == nil { | |
| return strings.TrimSpace(string(output)) | |
| } | |
| // Go up one directory | |
| if idx := strings.LastIndex(current, "/"); idx > 0 { | |
| current = current[:idx] | |
| } else { | |
| break | |
| } | |
| } | |
| return "" | |
| func findGitRoot(startPath string) string { | |
| current, err := filepath.Abs(startPath) | |
| if err != nil { | |
| return "" | |
| } | |
| for { | |
| cmd := exec.Command("git", "-C", current, "rev-parse", "--show-toplevel") | |
| if output, err := cmd.Output(); err == nil { | |
| return strings.TrimSpace(string(output)) | |
| } | |
| parent := filepath.Dir(current) | |
| if parent == current { | |
| return "" | |
| } | |
| current = parent | |
| } | |
| } |
1 participant
This pull request introduces a new update-checking feature to the application, alongside various code quality improvements and minor fixes. The most notable changes include the addition of an
UpdateCheckercomponent, integration of this feature into the UI, and the introduction of a new keybinding for triggering update checks. Additionally, there are minor refactorings and fixes for consistency and readability.New Feature: Update Checking
UpdateCheckercomponent to manage background update checks, including methods for checking updates, determining commits behind, and triggering immediate checks (app/update_checker.go).UpdateCheckerinto thehomestruct and initialized it in thenewHomefunction (app/app.go) [1] [2] [3].U) to trigger immediate update checks and updated the keybinding configuration (keys/keys.go) [1] [2] [3] [4] [5] [6].Menucomponent to display an update indicator when updates are available (ui/menu.go) [1] [2] [3].Code Quality Improvements
Userstructs insession/git/pr_comments.gofor better readability and maintainability [1] [2] [3].ui/jest.goandui/markdown.go[1] [2] [3] [4] [5] [6] [7].Minor Fixes
session/storage.go,session/instance.go, and other files [1] [2].test_suggestion.gofor clarity.These changes collectively improve the application's functionality, maintainability, and user experience.