Feat: Add interactive review flow for commit messages #74
Conversation
WalkthroughIntroduces an interactive commit-message review loop in the CLI, adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (createMsg)
participant Types as types.BuildCommitPrompt
participant Provider as LLM Provider
User->>CLI: Request generate commit message
CLI->>Types: BuildCommitPrompt(changes, opts{Attempt=1})
Types-->>CLI: Prompt
CLI->>Provider: GenerateCommitMessage(config, changes, key/url/model, opts)
Provider-->>CLI: Message or error
loop Interactive review
CLI->>User: Show message + actions [Accept | Regenerate | Edit | Exit]
alt Accept
CLI->>User: Copy to clipboard and finish
else Regenerate
CLI->>CLI: opts.Attempt++, set opts.StyleInstruction
CLI->>Types: BuildCommitPrompt(changes, opts)
Types-->>CLI: Prompt
CLI->>Provider: GenerateCommitMessage(..., opts)
Provider-->>CLI: New message or error
else Edit
CLI->>CLI: Open editor with message, return edited content
else Exit
CLI-->>User: Abort without copying
end
end
sequenceDiagram
autonumber
participant CLI
participant Providers as ChatGPT/Claude/Gemini/Grok/Groq/Ollama
participant Types as BuildCommitPrompt
CLI->>Types: BuildCommitPrompt(changes, opts)
Types-->>CLI: Prompt
CLI->>Providers: GenerateCommitMessage(..., opts)
Providers-->>CLI: Commit message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.gitignore(1 hunks)README.md(6 hunks)cmd/cli/createMsg.go(4 hunks)go.mod(1 hunks)internal/chatgpt/chatgpt.go(1 hunks)internal/claude/claude.go(1 hunks)internal/gemini/gemini.go(1 hunks)internal/grok/grok.go(1 hunks)internal/groq/groq.go(1 hunks)internal/groq/groq_test.go(4 hunks)internal/ollama/ollama.go(1 hunks)pkg/types/options.go(1 hunks)pkg/types/prompt.go(2 hunks)pkg/types/types_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
pkg/types/prompt.go (1)
pkg/types/options.go (1)
GenerationOptions(4-10)
internal/ollama/ollama.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)
internal/gemini/gemini.go (3)
internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(13-32)pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)
internal/groq/groq.go (6)
internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(13-32)internal/claude/claude.go (1)
GenerateCommitMessage(33-85)internal/gemini/gemini.go (1)
GenerateCommitMessage(13-44)internal/ollama/ollama.go (1)
GenerateCommitMessage(23-74)pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)
internal/chatgpt/chatgpt.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)
internal/claude/claude.go (7)
internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(13-32)internal/gemini/gemini.go (1)
GenerateCommitMessage(13-44)internal/grok/grok.go (1)
GenerateCommitMessage(15-87)internal/groq/groq.go (1)
GenerateCommitMessage(46-111)internal/ollama/ollama.go (1)
GenerateCommitMessage(23-74)pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)
internal/grok/grok.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)
internal/groq/groq_test.go (2)
internal/groq/groq.go (1)
GenerateCommitMessage(46-111)pkg/types/options.go (1)
GenerationOptions(4-10)
cmd/cli/createMsg.go (8)
pkg/types/types.go (9)
RepoConfig(59-62)LLMProvider(3-3)Config(53-56)ProviderGemini(8-8)ProviderOpenAI(6-6)ProviderClaude(7-7)ProviderGroq(10-10)ProviderOllama(11-11)ProviderGrok(9-9)internal/display/display.go (2)
ShowCommitMessage(99-115)ShowChangesPreview(118-133)pkg/types/options.go (1)
GenerationOptions(4-10)internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(13-32)internal/claude/claude.go (1)
GenerateCommitMessage(33-85)internal/gemini/gemini.go (1)
GenerateCommitMessage(13-44)internal/groq/groq.go (1)
GenerateCommitMessage(46-111)internal/ollama/ollama.go (1)
GenerateCommitMessage(23-74)
pkg/types/types_test.go (2)
pkg/types/prompt.go (1)
BuildCommitPrompt(28-49)pkg/types/options.go (1)
GenerationOptions(4-10)
🪛 markdownlint-cli2 (0.18.1)
README.md
183-183: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
189-189: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (13)
pkg/types/prompt.go (1)
26-49: LGTM! Well-structured prompt builder with proper null safety.The
BuildCommitPromptfunction is well-implemented:
- Uses efficient
strings.Builderfor concatenation- Properly handles nil
optsparameter- Trims whitespace from
StyleInstructionbefore checking if it's empty- Provides clear regeneration context when
Attempt > 1The implementation correctly constructs prompts that will help the LLM generate varied, context-aware commit messages.
pkg/types/options.go (1)
3-10: LGTM! Clean and well-documented options struct.The
GenerationOptionsstruct is well-designed:
- Clear, concise field names
- Comprehensive documentation explaining the purpose of each field
- The 1-indexed
Attemptconvention is explicitly documentedREADME.md (2)
34-34: LGTM! Clear feature description.The new interactive review flow feature is well-documented and aligns with the PR objectives.
141-150: LGTM! Helpful workflow documentation.The interactive commit workflow section provides clear, user-friendly guidance on the new features:
- Accept & copy
- Regenerate with presets
- Edit in editor
- Exit
This documentation will help users understand and adopt the new interactive flow.
internal/claude/claude.go (1)
33-35: LGTM! Consistent integration of GenerationOptions.The changes properly integrate the new
GenerationOptionsparameter:
- Function signature updated to accept
opts *types.GenerationOptions- Prompt construction delegated to
types.BuildCommitPrompt(changes, opts)- Implementation is consistent with other providers (Groq, ChatGPT, Gemini)
internal/chatgpt/chatgpt.go (1)
13-17: LGTM! Consistent integration of GenerationOptions.The changes properly integrate the new
GenerationOptionsparameter:
- Function signature updated to accept
opts *types.GenerationOptions- Prompt construction delegated to
types.BuildCommitPrompt(changes, opts)- Implementation is consistent with other providers
internal/groq/groq.go (1)
46-51: LGTM! Consistent integration of GenerationOptions.The changes properly integrate the new
GenerationOptionsparameter:
- Function signature updated to accept
opts *types.GenerationOptions- Prompt construction delegated to
types.BuildCommitPrompt(changes, opts)- Implementation is consistent with other providers
The validation for empty
changesstring (line 47-49) remains in place, which is good defensive programming.go.mod (1)
9-9: No action needed:github.com/google/shlexusage confirmed. The package is imported andshlex.Splitis called in cmd/cli/createMsg.go.internal/ollama/ollama.go (1)
23-31: Prompt builder integration looks solid.Switching to
BuildCommitPromptcleanly threads regeneration context and style guidance without altering the rest of the flow.internal/gemini/gemini.go (1)
13-31: Gemini now respects generation options.Passing
optsthroughBuildCommitPromptkeeps the provider aligned with the shared prompt strategy. Looks good.internal/grok/grok.go (1)
15-87: Grok request updates align with the new prompt builder.The change cleanly injects regeneration context/style instructions while leaving transport logic intact.
pkg/types/types_test.go (1)
52-105: Great coverage on the new prompt builder paths.These tests pin the default, style, and regeneration behaviors, making future regressions unlikely.
internal/groq/groq_test.go (1)
7-145: Test updates thoroughly exercise the new options plumbing.The additional assertions confirm request payloads include style instructions and regeneration context—exactly what we need.
| # Test Files | ||
| test_* | ||
|
|
There was a problem hiding this comment.
Reconsider the test file ignore pattern.
The pattern test_* is overly broad and doesn't follow Go conventions:
- Standard Go test files are named
*_test.go, nottest_* - This pattern could accidentally ignore legitimate files like
test_data/,test_config.json, ortest_helpers.go - Go test binaries created with
go test -care already covered by the*.testpattern on line 12
If the intent is to ignore temporary test files or test artifacts, consider either:
- Removing this pattern entirely (Go conventions already covered)
- Using a more specific pattern like
test_output/ortest_artifacts/ - Documenting what specific files this is meant to ignore
Apply this diff to remove the overly broad pattern:
-# Test Files
-test_*
-Or if specific test artifacts need to be ignored, use a more targeted pattern:
# Test Files
-test_*
+test_output/
+test_artifacts/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Test Files | |
| test_* | |
| # Test Files | |
| test_output/ | |
| test_artifacts/ |
🤖 Prompt for AI Agents
In .gitignore around lines 14 to 16, the broad pattern `test_*` will erroneously
ignore valid files and folders (e.g. test_data/, test_config.json,
test_helpers.go) and doesn't match Go test naming; remove the `test_*` line and,
if specific test artifacts should be ignored, replace it with targeted patterns
such as `test_output/`, `test_artifacts/`, or any explicit filenames/directories
you intend to exclude (do not add a blanket `test_*` pattern).
|
Hey @Muneer320 did u check the code coderabbit review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/cli/store/store.go (2)
220-255: Prevent setting default to a non-existent providerChangeDefault doesn’t verify that the target exists in LLMProviders. Add a check and error out if missing.
func ChangeDefault(Model types.LLMProvider) error { @@ - cfg.Default = Model + // Ensure the model exists before setting default + found := false + for _, p := range cfg.LLMProviders { + if p.LLM == Model { + found = true + break + } + } + if !found { + return fmt.Errorf("cannot set default to %s: no saved entry", Model.String()) + } + cfg.Default = Model @@ return os.WriteFile(configPath, data, 0600) }
338-349: UpdateAPIKey should error when provider not foundCurrently it writes the file even if nothing was updated. Return an error when the model doesn’t exist.
- for i, p := range cfg.LLMProviders { + updated := false + for i, p := range cfg.LLMProviders { if p.LLM == Model { cfg.LLMProviders[i].APIKey = APIKey + updated = true } } + if !updated { + return fmt.Errorf("no saved entry for %s to update", Model.String()) + }
♻️ Duplicate comments (1)
cmd/cli/createMsg.go (1)
147-176: Custom style is dropped on subsequent regenerations; preserve and reselect itThe selector doesn’t include the active custom label; defaulting to it silently falls back to the first preset, losing instructions. Keep the active custom label in options and reuse stored GenerationOptions when reselected.
Apply these diffs:
- Track current style options and pass them to the selector
- currentStyleLabel := stylePresets[0].Label + currentStyleLabel := stylePresets[0].Label + var currentStyleOpts *types.GenerationOptions @@ - opts, styleLabel, err := promptStyleSelection(currentStyleLabel) + opts, styleLabel, err := promptStyleSelection(currentStyleLabel, currentStyleOpts) @@ - if styleLabel != "" { + if styleLabel != "" { currentStyleLabel = styleLabel } + // Persist selected style options for future regenerations + if opts != nil { + currentStyleOpts = opts + }
- Include the active custom label in the selector and return stored opts when chosen
-func promptStyleSelection(currentLabel string) (*types.GenerationOptions, string, error) { - options := make([]string, 0, len(stylePresets)+2) - for _, preset := range stylePresets { - options = append(options, preset.Label) - } - options = append(options, customStyleOption, styleBackOption) +func promptStyleSelection(currentLabel string, currentOpts *types.GenerationOptions) (*types.GenerationOptions, string, error) { + options := make([]string, 0, len(stylePresets)+3) + foundCurrent := false + for _, preset := range stylePresets { + options = append(options, preset.Label) + if preset.Label == currentLabel { + foundCurrent = true + } + } + if currentOpts != nil && currentLabel != "" && !foundCurrent { + options = append(options, currentLabel) + } + options = append(options, customStyleOption, styleBackOption) @@ choice, err := selector.Show() @@ switch choice { @@ case customStyleOption: @@ return &types.GenerationOptions{StyleInstruction: text}, formatCustomStyleLabel(text), nil default: for _, preset := range stylePresets { if choice == preset.Label { if strings.TrimSpace(preset.Instruction) == "" { return nil, preset.Label, nil } return &types.GenerationOptions{StyleInstruction: preset.Instruction}, preset.Label, nil } } + // If the user reselected the current custom label, reuse previous options + if currentOpts != nil && choice == currentLabel { + clone := *currentOpts + return &clone, currentLabel, nil + } } - return nil, currentLabel, nil + // Default: keep current selection and options + if currentOpts != nil && currentLabel != "" { + clone := *currentOpts + return &clone, currentLabel, nil + } + return nil, currentLabel, nilAlso applies to: 263-307
🧹 Nitpick comments (7)
pkg/types/types.go (3)
22-23: Misplaced inline comment inside switchDoc belongs near the type, not inside IsValid(). Please remove here and attach to LLMProvider type.
Apply this diff to remove the inline comment:
case ProviderOpenAI, ProviderClaude, ProviderGemini, ProviderGrok, ProviderGroq, ProviderOllama: return true - // LLMProvider identifies the large language model backend used to author - // commit messages.Add this doc where the type is declared (outside this hunk):
// LLMProvider identifies the large language model backend used to author commit messages. type LLMProvider string
35-37: Stray/misleading comment in provider listThis comment describes String(), but it sits next to ProviderGroq in GetSupportedProviders. Remove it.
ProviderGrok, - // String returns the string form of the provider identifier. ProviderGroq,
41-42: Doc comments attached to wrong declarationsSeveral doc comments are placed above unrelated items. Please relocate for correct go doc generation.
Apply this diff to remove the misplaced comments:
-// IsValid reports whether the provider is part of the supported set. @@ -// GetSupportedProviders returns all available provider enums. @@ -// GetSupportedProviderStrings returns the human-friendly names for providers. @@ - // ParseLLMProvider converts a string into an LLMProvider enum when supported.Then add properly placed docs (outside this hunk):
// GetSupportedProviders returns all available provider enums. func GetSupportedProviders() []LLMProvider { ... } // IsValid reports whether the provider is part of the supported set. func (p LLMProvider) IsValid() bool { ... } // GetSupportedProviderStrings returns the human‑friendly names for providers. func GetSupportedProviderStrings() []string { ... } // ParseLLMProvider converts a string into an LLMProvider enum when supported. func ParseLLMProvider(s string) (LLMProvider, bool) { ... }Also applies to: 52-53, 65-66, 75-76
cmd/cli/store/store.go (1)
21-25: Naming collisions increase confusionLocal Config and LLMProvider types shadow pkg/types.Config and types.LLMProvider. Consider renaming to StoreConfig and ProviderEntry for clarity.
cmd/cli/createMsg.go (1)
239-251: Overloading APIKey as Ollama URL is leaky and confusingTreating the stored APIKey as the Ollama URL complicates UX and storage semantics. Prefer a provider-specific config (e.g., URL, Model) or read Ollama URL/model from env/config only.
internal/grok/grok.go (1)
50-64: Consider reusing HTTP client instances.The HTTP client is created on every
GenerateCommitMessagecall, which incurs TLS handshake overhead. For better performance, consider creating the client once (e.g., as a package-level or struct-level variable) and reusing it across calls.Example refactor:
var ( grokClientOnce sync.Once grokClient *http.Client ) func getGrokClient() *http.Client { grokClientOnce.Do(func() { transport := &http.Transport{ TLSHandshakeTimeout: 10 * time.Second, MaxIdleConns: 10, IdleConnTimeout: 30 * time.Second, DisableCompression: true, TLSClientConfig: &tls.Config{ InsecureSkipVerify: false, }, } grokClient = &http.Client{ Timeout: 30 * time.Second, Transport: transport, } }) return grokClient }Then use
client := getGrokClient()instead of creating a new client.internal/claude/claude.go (1)
20-24: Consolidate duplicate Message type.The
Messagetype defined here is identical totypes.Messageinpkg/types/types.go(lines 81-84). To reduce duplication and ensure consistency, use the shared type from the types package.Apply this diff to use the shared type:
-// Message represents a single role/content pair exchanged with Claude. -type Message struct { - Role string `json:"role"` - Content string `json:"content"` -} - // ClaudeResponse captures the subset of fields used from Anthropic responses. type ClaudeResponse struct { ID string `json:"id"` @@ -42,7 +37,7 @@ func GenerateCommitMessage(config *types.Config, changes string, apiKey string, reqBody := ClaudeRequest{ Model: "claude-3-5-sonnet-20241022", MaxTokens: 200, - Messages: []Message{ + Messages: []types.Message{ { Role: "user", Content: prompt, @@ -13,7 +13,7 @@ // ClaudeRequest describes the payload sent to Anthropic's Claude messages API. type ClaudeRequest struct { Model string `json:"model"` - Messages []Message `json:"messages"` + Messages []types.Message `json:"messages"` MaxTokens int `json:"max_tokens"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/cli/createMsg.go(4 hunks)cmd/cli/llmSetup.go(2 hunks)cmd/cli/store/store.go(7 hunks)internal/chatgpt/chatgpt.go(1 hunks)internal/claude/claude.go(2 hunks)internal/gemini/gemini.go(1 hunks)internal/grok/grok.go(1 hunks)internal/ollama/ollama.go(1 hunks)pkg/types/prompt.go(2 hunks)pkg/types/types.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/cli/llmSetup.go
🧰 Additional context used
🧬 Code graph analysis (8)
internal/chatgpt/chatgpt.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
cmd/cli/store/store.go (1)
pkg/types/types.go (1)
LLMProvider(3-3)
internal/grok/grok.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
internal/ollama/ollama.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
pkg/types/prompt.go (1)
pkg/types/options.go (1)
GenerationOptions(4-10)
internal/gemini/gemini.go (3)
internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(15-34)pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
internal/claude/claude.go (3)
pkg/types/types.go (2)
Message(82-85)Config(60-63)pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
cmd/cli/createMsg.go (9)
pkg/types/types.go (2)
RepoConfig(68-71)Config(60-63)internal/display/display.go (2)
ShowCommitMessage(99-115)ShowChangesPreview(118-133)pkg/types/options.go (1)
GenerationOptions(4-10)internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(15-34)internal/claude/claude.go (1)
GenerateCommitMessage(37-89)internal/gemini/gemini.go (1)
GenerateCommitMessage(15-46)internal/grok/grok.go (1)
GenerateCommitMessage(17-89)internal/ollama/ollama.go (1)
GenerateCommitMessage(27-78)internal/groq/groq.go (1)
GenerateCommitMessage(46-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Package Binaries
🔇 Additional comments (7)
cmd/cli/createMsg.go (1)
395-410: Helpful provider-specific errors — LGTMClear, actionable guidance per provider. Good UX.
pkg/types/prompt.go (1)
28-51: Prompt builder logic looks solidCorrectly appends regeneration context when Attempt > 1 and optional style guidance, then changes. Centralizing this across providers is good.
If not already covered, ensure tests assert:
- Attempt == 1 omits regeneration section.
- Attempt > 1 includes both lines of regeneration context.
- StyleInstruction with surrounding whitespace is trimmed.
internal/gemini/gemini.go (1)
13-18: Switched to centralized prompt builder — goodSignature and prompt sourcing updated consistently; response checks remain intact.
Also applies to: 31-46
internal/grok/grok.go (1)
15-19: LGTM! Function signature and prompt construction updated correctly.The addition of the
optsparameter and the use oftypes.BuildCommitPrompt(changes, opts)align with the interactive regeneration feature. The prompt builder handles nil opts safely.internal/chatgpt/chatgpt.go (1)
13-19: LGTM! Function signature and prompt construction updated correctly.The addition of the
optsparameter and the use oftypes.BuildCommitPrompt(changes, opts)are consistent with the broader refactoring across all LLM providers. The prompt builder handles nil opts safely.internal/claude/claude.go (1)
36-39: LGTM! Function signature and prompt construction updated correctly.The addition of the
optsparameter and the use oftypes.BuildCommitPrompt(changes, opts)align with the interactive regeneration feature across all providers.internal/ollama/ollama.go (1)
25-34: LGTM! Function signature and prompt construction updated correctly.The addition of the
optsparameter and the use oftypes.BuildCommitPrompt(changes, opts)are consistent with the broader refactoring. The prompt builder handles nil opts safely.
| type LLMProvider struct { | ||
| LLM types.LLMProvider `json:"model"` | ||
| APIKey string `json:"api_key"` | ||
| } |
There was a problem hiding this comment.
Plain-text storage of API keys to disk
Storing API keys unencrypted in config.json is a security risk (even with 0600). Prefer OS keychains or secret stores; at minimum, allow env-var override and document the risk.
Options:
- Windows: DPAPI/Credential Manager; macOS: Keychain; Linux: libsecret/gnome-keyring.
- Or let users opt out of persistence and read from env only; if persisting, encrypt at rest with OS facilities.
Also applies to: 82-83
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
README.md (1)
178-194: Use proper markdown headings for better document structure.Lines 178, 183, and 189 use bold text for section labels. These should be proper headings (e.g.,
### Set LLM as default) for better document hierarchy, accessibility, and consistency with markdown best practices.Apply this diff to fix the markdown structure:
-**Set LLM as default** - +### Set LLM as default + ```bash Select: Set Default-Change API Key
+### Change API Key
+Select: Change API Key-Delete LLM
+### Delete LLM
+Select: Delete
Based on static analysis hints. </blockquote></details> <details> <summary>cmd/cli/createMsg.go (1)</summary><blockquote> `412-427`: **LGTM!** Provider-specific error messages are helpful and guide users to the correct remediation steps. **Note:** There's no specific case for `types.ProviderOllama`, so Ollama errors will use the generic message. This is acceptable since Ollama is local and doesn't use API keys, but you could add a specific message mentioning Ollama URL and model configuration if desired. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ec3d2bc9b29d3a2180de9cc8d79cb85622f637f7 and 5f65d558d17b4d8c763d6c32df30351434b1384d. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `README.md` (6 hunks) * `cmd/cli/createMsg.go` (4 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>cmd/cli/createMsg.go (9)</summary><blockquote> <details> <summary>pkg/types/types.go (9)</summary> * `RepoConfig` (68-71) * `LLMProvider` (3-3) * `Config` (60-63) * `ProviderGemini` (8-8) * `ProviderOpenAI` (6-6) * `ProviderClaude` (7-7) * `ProviderGroq` (10-10) * `ProviderOllama` (11-11) * `ProviderGrok` (9-9) </details> <details> <summary>pkg/types/options.go (1)</summary> * `GenerationOptions` (4-10) </details> <details> <summary>internal/display/display.go (2)</summary> * `ShowCommitMessage` (99-115) * `ShowChangesPreview` (118-133) </details> <details> <summary>internal/chatgpt/chatgpt.go (1)</summary> * `GenerateCommitMessage` (15-34) </details> <details> <summary>internal/gemini/gemini.go (1)</summary> * `GenerateCommitMessage` (15-46) </details> <details> <summary>internal/grok/grok.go (1)</summary> * `GenerateCommitMessage` (17-89) </details> <details> <summary>internal/ollama/ollama.go (1)</summary> * `GenerateCommitMessage` (27-78) </details> <details> <summary>internal/groq/groq.go (1)</summary> * `GenerateCommitMessage` (46-111) </details> <details> <summary>internal/claude/claude.go (1)</summary> * `GenerateCommitMessage` (37-89) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>README.md</summary> 183-183: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 189-189: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Build Go Binary (windows-latest) </details> <details> <summary>🔇 Additional comments (10)</summary><blockquote> <details> <summary>README.md (2)</summary><blockquote> `34-34`: **LGTM!** The feature description accurately reflects the new interactive review flow introduced in this PR. --- `141-151`: **LGTM!** The interactive workflow documentation is clear, comprehensive, and provides helpful context for users about the new review loop. </blockquote></details> <details> <summary>cmd/cli/createMsg.go (8)</summary><blockquote> `4-25`: **LGTM!** All new imports are necessary for the interactive review flow functionality: `errors` for custom errors, `exec` for editor spawning, `runtime` for OS detection, `strings` for text processing, and `shlex` for shell command parsing. --- `106-203`: **LGTM!** The interactive loop is well-structured and implements all the actions specified in the PR objectives: Accept, Regenerate, Edit, and Exit. Error handling and flow control are appropriate. --- `205-228`: **LGTM!** The style preset system is well-designed with clear labels and appropriate instructions. The use of `errSelectionCancelled` as a sentinel error is a good pattern for flow control. --- `265-324`: **Custom style persistence is now correctly implemented.** The logic now properly persists custom style instructions across regenerations: - Lines 274-276 ensure the custom label appears in the selector options - Lines 280-282 set it as the default when present - Lines 313-316 return the stored options when the custom label is reselected - Lines 319-323 provide a fallback This addresses the concern raised in the previous review about custom instructions being silently dropped. --- `326-363`: **LGTM!** The editor integration is well-implemented with proper temp file management, error handling at each step, and cleanup via `defer os.Remove` (line 336). --- `365-392`: **LGTM!** The editor resolution logic follows standard conventions by checking `GIT_EDITOR`, `VISUAL`, and `EDITOR` in order. The use of `shlex.Split` (line 377) properly handles editor commands that may include arguments, and platform-specific fallbacks (notepad on Windows, nano elsewhere) are appropriate. Based on learnings about shlex usage. --- `394-401`: **LGTM!** The label formatting correctly uses runes for proper Unicode handling and provides a reasonable 40-character display limit with ellipsis for truncated text. --- `403-410`: **LGTM!** The function correctly clones the `styleOpts` to avoid mutation (line 407) and properly sets the attempt number. This ensures the original options are not modified. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/cli/store/store.go (2)
27-83: Add file locking to prevent concurrent modification issues.Multiple processes or goroutines could simultaneously read/write the config file, leading to race conditions and data corruption. Consider using
syscall.Flock(Unix) or equivalent locking mechanisms to ensure atomic operations.Additionally, the logic at lines 48-53 catches
os.ErrNotExistand setsdata = []byte("{}"), butcreateConfigFile(lines 40-46) already ensures the file exists. This fallback is redundant. Also, lines 55-60 unmarshal intocfgthat was initialized at lines 30-33, overwriting the initial setup—consider moving the initialization after the read.
146-185: Inconsistent empty-config check and error message formatting.Line 167 uses
len(data) > 2, which is brittle (assumes{}with spaces or content), whereasListSavedModels(line 207) useslen(data) > 0. These checks should be consistent. Also, line 159's error message "config file Not exists" has inconsistent capitalization compared to other functions (lines 199, 232, 281, 334 use lowercase "not exists").Consider extracting the repeated config-loading pattern (lines 148-174, 189-214, 225-245, 274-294, 327-347) into a helper function like
loadConfig() (*Config, error)to reduce duplication and ensure consistency.
🧹 Nitpick comments (3)
internal/grok/grok.go (1)
23-35: Set a TLS minimum version on the Grok client transport.The custom HTTP client doesn’t set
MinVersionontls.Config, so it can silently negotiate TLS 1.0/1.1 if the server offers it. Please pin the minimum to TLS 1.2 (or 1.3 if the API supports it) to avoid downgrades.Suggested patch:
- TLSClientConfig: &tls.Config{ - InsecureSkipVerify: false, - }, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: false, + MinVersion: tls.VersionTLS12, + },cmd/cli/store/store.go (2)
85-93: Simplify redundant error check.Line 88 checks
err != nil || os.IsNotExist(err). Iferrisos.ErrNotExist, the first condition already evaluates to true, making the second check redundant.Apply this diff:
- if err != nil || os.IsNotExist(err) { + if err != nil { return false }
95-104: Function name is misleading.
createConfigFileonly creates the parent directory structure but not the file itself. Consider renaming toensureConfigDiror adding file creation logic to match the name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(6 hunks)cmd/cli/createMsg.go(4 hunks)cmd/cli/store/store.go(9 hunks)internal/claude/claude.go(2 hunks)internal/grok/grok.go(3 hunks)pkg/types/types.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/cli/store/store.go (1)
pkg/types/types.go (1)
LLMProvider(5-5)
pkg/types/types.go (1)
cmd/cli/store/store.go (1)
LLMProvider(16-19)
internal/claude/claude.go (3)
pkg/types/types.go (1)
Message(80-83)pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
internal/grok/grok.go (2)
pkg/types/options.go (1)
GenerationOptions(4-10)pkg/types/prompt.go (1)
BuildCommitPrompt(30-51)
cmd/cli/createMsg.go (9)
pkg/types/types.go (9)
RepoConfig(66-69)LLMProvider(5-5)Config(60-63)ProviderGemini(10-10)ProviderOpenAI(8-8)ProviderClaude(9-9)ProviderGroq(12-12)ProviderOllama(13-13)ProviderGrok(11-11)pkg/types/options.go (1)
GenerationOptions(4-10)internal/display/display.go (2)
ShowCommitMessage(99-115)ShowChangesPreview(118-133)internal/claude/claude.go (1)
GenerateCommitMessage(31-83)internal/grok/grok.go (1)
GenerateCommitMessage(42-99)internal/chatgpt/chatgpt.go (1)
GenerateCommitMessage(15-34)internal/gemini/gemini.go (1)
GenerateCommitMessage(15-46)internal/ollama/ollama.go (1)
GenerateCommitMessage(27-78)internal/groq/groq.go (1)
GenerateCommitMessage(46-111)
🪛 ast-grep (0.39.5)
internal/grok/grok.go
[warning] 27-29: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: false,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Package Binaries
🔇 Additional comments (3)
cmd/cli/store/store.go (3)
106-144: LGTM!Cross-platform path resolution correctly handles Windows, macOS, and Linux with appropriate fallbacks for missing environment variables.
247-256: LGTM!Validation ensures the target provider exists before updating the default, preventing orphaned references.
268-320: LGTM!The delete logic correctly prevents removing the default provider when alternatives exist and appropriately resets the config when deleting the last provider.
| updated := false | ||
| for i, p := range cfg.LLMProviders { | ||
| if p.LLM == Model { | ||
| cfg.LLMProviders[i].APIKey = APIKey | ||
| updated = true | ||
| } | ||
| } | ||
|
|
||
| if !updated { | ||
| return fmt.Errorf("no saved entry for %s to update", Model.String()) | ||
| } |
There was a problem hiding this comment.
Add break after updating API key.
Line 353 sets updated = true but the loop continues unnecessarily. Add a break statement for efficiency.
Apply this diff:
for i, p := range cfg.LLMProviders {
if p.LLM == Model {
cfg.LLMProviders[i].APIKey = APIKey
updated = true
+ break
}
}The validation at lines 357-359 correctly ensures the provider exists before updating.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updated := false | |
| for i, p := range cfg.LLMProviders { | |
| if p.LLM == Model { | |
| cfg.LLMProviders[i].APIKey = APIKey | |
| updated = true | |
| } | |
| } | |
| if !updated { | |
| return fmt.Errorf("no saved entry for %s to update", Model.String()) | |
| } | |
| updated := false | |
| for i, p := range cfg.LLMProviders { | |
| if p.LLM == Model { | |
| cfg.LLMProviders[i].APIKey = APIKey | |
| updated = true | |
| break | |
| } | |
| } | |
| if !updated { | |
| return fmt.Errorf("no saved entry for %s to update", Model.String()) | |
| } |
🤖 Prompt for AI Agents
In cmd/cli/store/store.go around lines 349 to 359, the loop that updates a
provider's APIKey sets updated = true but keeps iterating; add a break
immediately after setting cfg.LLMProviders[i].APIKey = APIKey (right after
updated = true) so the loop exits once the matching provider is updated,
improving efficiency.
DFanso
added
enhancement
|
Wow, this rabbit really have nitpicks. 😭 Also on an unrelated note, there's a mismatch corners of the commit message panel, 
|
|
Sometime it also shows fixed parts 🤣. Btw can u raise a issue I will assign u if u like to work on that issue 🎉 |
2 participants
Description
Introduces an interactive commit review loop so contributors can accept, regenerate, or edit AI-generated commit messages. Regeneration attempts now send incremental context to the LLM, encouraging varied outputs and honoring tone presets or custom instructions.
Type of Change
Related Issue
Fixes #58
Changes Made
Testing
Checklist
Screenshots (if applicable)
N/A
Additional Notes
Regeneration prompts now encourage distinct alternatives, helping users pick the best-fitting commit summary without manual tweaking.
For Hacktoberfest Participants
Thank you for your contribution! 🎉
Summary by CodeRabbit
New Features
Documentation
Tests
Chores