fix: correct PR number string conversion and add pagination to GetIssues - #506
fix: correct PR number string conversion and add pagination to GetIssues#506saurabhhhcodes wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe analyze command now handles cache initialization failures, while the trends command uses Unicode-aware capitalization for forecast labels. ChangesCache initialization handling
Trend label formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/github/issues.go`:
- Around line 49-52: Update the pagination flow around the `c.get` call in the
issues-fetching method so errors on later pages do not discard previously
accumulated issues. Define and consistently handle an explicit partial-results
contract, or propagate the pagination failure so `cmd/analyze.go` marks the
analysis incomplete instead of replacing the result with an empty slice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fa173f3-5009-419e-b1e4-03a400bb90a6
📒 Files selected for processing (3)
internal/analyzer/collaboration.gointernal/config/settings.gointernal/github/issues.go
| var issues []Issue | ||
| if err := c.get(url, &issues); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not discard previously fetched issues on later-page errors.
If page 2 or later fails, return nil, err drops all successfully fetched pages. cmd/analyze.go, Lines 229-237, then replaces the result with an empty slice, so analysis can silently report no issues after a transient pagination failure. Make the caller fail/mark the analysis incomplete, or define and handle an explicit partial-results contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/github/issues.go` around lines 49 - 52, Update the pagination flow
around the `c.get` call in the issues-fetching method so errors on later pages
do not discard previously accumulated issues. Define and consistently handle an
explicit partial-results contract, or propagate the pagination failure so
`cmd/analyze.go` marks the analysis incomplete instead of replacing the result
with an empty slice.
71566e6 to
06becb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/trends.go (1)
256-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for Unicode first-rune capitalization.
Please cover multi-word ASCII text, non-ASCII leading letters, non-letter leading runes, and the empty-trend fallback (
Stable) incmd/trends_test.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/trends.go` around lines 256 - 258, Add regression tests in cmd/trends_test.go for the capitalization logic around forecast trend labels: cover multi-word ASCII input, a non-ASCII leading letter, a non-letter leading rune, and an empty trend that falls back to Stable. Use the existing test conventions and verify the complete resulting label for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/analyze.go`:
- Around line 256-259: Update the cache initialization error path around
cache.NewCache so it calls overallProgress.Finish() before returning the wrapped
cache error, matching the neighboring fatal-error paths and ensuring progress
tracking is completed.
- Around line 256-259: Move the cache.NewCache call and its error handling into
the existing incremental-analysis block in the analyze flow, using cacheInstance
only when incremental is enabled. Keep non-incremental analysis independent of
cache initialization and preserve the existing initialization error propagation
for incremental runs.
---
Nitpick comments:
In `@cmd/trends.go`:
- Around line 256-258: Add regression tests in cmd/trends_test.go for the
capitalization logic around forecast trend labels: cover multi-word ASCII input,
a non-ASCII leading letter, a non-letter leading rune, and an empty trend that
falls back to Stable. Use the existing test conventions and verify the complete
resulting label for each case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6cd39eb-225d-474b-b8f3-4aed4b3ddc9e
📒 Files selected for processing (2)
cmd/analyze.gocmd/trends.go
| cacheInstance, cacheErr := cache.NewCache() | ||
| if cacheErr != nil { | ||
| return fmt.Errorf("failed to initialize cache: %w", cacheErr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Finish the progress tracker before returning the cache error.
Neighboring fatal-error paths call overallProgress.Finish(). This new early return does not, so a cache initialization failure can leave the progress UI active or incomplete.
cacheInstance, cacheErr := cache.NewCache()
if cacheErr != nil {
+ overallProgress.Finish()
return fmt.Errorf("failed to initialize cache: %w", cacheErr)
}📝 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.
| cacheInstance, cacheErr := cache.NewCache() | |
| if cacheErr != nil { | |
| return fmt.Errorf("failed to initialize cache: %w", cacheErr) | |
| } | |
| cacheInstance, cacheErr := cache.NewCache() | |
| if cacheErr != nil { | |
| overallProgress.Finish() | |
| return fmt.Errorf("failed to initialize cache: %w", cacheErr) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/analyze.go` around lines 256 - 259, Update the cache initialization error
path around cache.NewCache so it calls overallProgress.Finish() before returning
the wrapped cache error, matching the neighboring fatal-error paths and ensuring
progress tracking is completed.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initialize the cache only for incremental analysis.
cacheInstance is unused unless incremental is enabled, but this now makes every analysis fail when the cache directory cannot be created. Move cache creation inside the if incremental block so non-incremental runs remain independent of cache availability.
Proposed fix
- cacheInstance, cacheErr := cache.NewCache()
- if cacheErr != nil {
- return fmt.Errorf("failed to initialize cache: %w", cacheErr)
- }
// Incremental analysis support
if incremental {
+ cacheInstance, cacheErr := cache.NewCache()
+ if cacheErr != nil {
+ return fmt.Errorf("failed to initialize cache: %w", cacheErr)
+ }📝 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.
| cacheInstance, cacheErr := cache.NewCache() | |
| if cacheErr != nil { | |
| return fmt.Errorf("failed to initialize cache: %w", cacheErr) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/analyze.go` around lines 256 - 259, Move the cache.NewCache call and its
error handling into the existing incremental-analysis block in the analyze flow,
using cacheInstance only when incremental is enabled. Keep non-incremental
analysis independent of cache initialization and preserve the existing
initialization error propagation for incremental runs.
1 participant
Two runtime bugs fixed:
Wrong string conversion for PR numbers in
collaboration.go(string(rune(pr.Number+'0'))produces incorrect Unicode characters for numbers > 9, e.g., PR ci.yml update #42 → "PR #Z"). Fixed withfmt.Sprintf.Missing pagination in GetIssues - only fetched the first 100 issues, silently dropping all others. Added full pagination loop.
Summary by CodeRabbit