Fetch package versions concurrently - #29
Conversation
14-make-fetching-packages-version-concurrent
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR adds concurrency to npm package version checking. The code introduces a new dependency on ChangesConcurrent NPM Dependency Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
npm.go (2)
169-173: 💤 Low valueMinor cleanups: misleading comment and unnecessary loop variable shadow.
- Line 169: "Rate limits hihi" —
SetLimit(8)is a concurrency limit, not a rate limit. They're different things (concurrent in-flight vs. requests-per-time). Worth clarifying or removing.- Line 173:
i := iis no longer required.go.moddeclaresgo 1.25.0, and per-iteration loop variables have been the default since Go 1.22, so the shadow is dead code.Suggested cleanup
- // Rate limits hihi - g.SetLimit(8) + // Cap concurrent npm registry requests to avoid overwhelming the host. + g.SetLimit(8) for i := range deps { - i := i - g.Go(func() error {🤖 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 `@npm.go` around lines 169 - 173, The comment "Rate limits hihi" is misleading for G.SetLimit(8) because SetLimit controls concurrency (in-flight goroutines) not request rate; update or remove the comment to accurately say it sets a concurrency limit (e.g., "Limit concurrency to 8"). Also remove the unnecessary per-iteration shadow `i := i` inside the loop `for i := range deps { ... }` since Go 1.22+ fixes loop variable capture; keep using `i` directly in the goroutine or closure.
167-167: ⚡ Quick winDrop
WithContextsince the context is unused and provides no cancellation benefit.
errgroup.WithContextreturns a context that cancels on the first error, but you discard it with_. NeithergetNPMPackageLatestVersionnorgetOtherNPMPackageVersionsaccept a context parameter, and both useclient.Get()without context propagation. This means when one fetch fails, the remaining in-flight HTTP requests continue to completion beforeg.Wait()returns.Replace with a plain
errgroup.Groupand remove the"context"import:Suggested change
- g, _ := errgroup.WithContext(context.Background()) - - // Rate limits hihi - g.SetLimit(8) + var g errgroup.Group + g.SetLimit(8) // bound concurrent registry requestsRemove the
"context"import from the imports section.If proper cancellation on error is desired in the future, thread the context into the HTTP calls via
http.NewRequestWithContextinstead.🤖 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 `@npm.go` at line 167, The code currently calls errgroup.WithContext and discards the returned context (g, _ := errgroup.WithContext(...)) while the helper functions getNPMPackageLatestVersion and getOtherNPMPackageVersions do not accept contexts or propagate them into client.Get(), so replace the WithContext usage with a plain errgroup.Group (use var g errgroup.Group or errgroup.Group{} as appropriate) and remove the unused "context" import; if you later want cancellation on first error, thread a context through getNPMPackageLatestVersion/getOtherNPMPackageVersions and use http.NewRequestWithContext when calling client.Get() so g.Wait() can cancel in-flight requests.go.mod (1)
21-21: ⚡ Quick win
golang.org/x/syncshould be a direct dependency, not indirect.
npm.gonow importsgolang.org/x/sync/errgroupdirectly, so this dependency is used by your own code rather than transitively pulled in. Runninggo mod tidywill likely move it out of the indirect group; consider running it now to keepgo.modaccurate.Proposed change
require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect ... golang.org/x/exp v0.0.0-20231006140011-7918f672742d // indirect - golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.30.0 // indirect ) + +require golang.org/x/sync v0.20.0Or simply run
go mod tidyand let the toolchain place it correctly.🤖 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 `@go.mod` at line 21, go.mod lists golang.org/x/sync as indirect but npm.go now imports golang.org/x/sync/errgroup directly; update module dependencies by running "go mod tidy" (or add golang.org/x/sync at the correct version as a direct require) so the dependency is recorded as direct in go.mod; check npm.go (the import of golang.org/x/sync/errgroup) to confirm and then commit the updated go.mod/go.sum.
🤖 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 `@npm.go`:
- Around line 162-245: The concurrent loop appends to updates in completion
order, breaking determinism; instead preallocate a results slice of length
len(deps) (e.g. results := make([]DependencyUpdate, len(deps))) and in the
goroutine write the computed update into results[i] (using the same i capture)
rather than appending; after g.Wait, build the final updates slice by iterating
results in index order and filtering out empty/zero entries. Keep the existing
deps[i] assignment and its mutex around it (or use a separate mutex only if you
choose to make writes to results concurrent), and leave calls like
getNPMPackageLatestVersion, classifyDependencyUpdate,
getLatestPatchNPMPackageVersion and mergeDependencyVersion unchanged.
---
Nitpick comments:
In `@go.mod`:
- Line 21: go.mod lists golang.org/x/sync as indirect but npm.go now imports
golang.org/x/sync/errgroup directly; update module dependencies by running "go
mod tidy" (or add golang.org/x/sync at the correct version as a direct require)
so the dependency is recorded as direct in go.mod; check npm.go (the import of
golang.org/x/sync/errgroup) to confirm and then commit the updated
go.mod/go.sum.
In `@npm.go`:
- Around line 169-173: The comment "Rate limits hihi" is misleading for
G.SetLimit(8) because SetLimit controls concurrency (in-flight goroutines) not
request rate; update or remove the comment to accurately say it sets a
concurrency limit (e.g., "Limit concurrency to 8"). Also remove the unnecessary
per-iteration shadow `i := i` inside the loop `for i := range deps { ... }`
since Go 1.22+ fixes loop variable capture; keep using `i` directly in the
goroutine or closure.
- Line 167: The code currently calls errgroup.WithContext and discards the
returned context (g, _ := errgroup.WithContext(...)) while the helper functions
getNPMPackageLatestVersion and getOtherNPMPackageVersions do not accept contexts
or propagate them into client.Get(), so replace the WithContext usage with a
plain errgroup.Group (use var g errgroup.Group or errgroup.Group{} as
appropriate) and remove the unused "context" import; if you later want
cancellation on first error, thread a context through
getNPMPackageLatestVersion/getOtherNPMPackageVersions and use
http.NewRequestWithContext when calling client.Get() so g.Wait() can cancel
in-flight requests.
🪄 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
Run ID: 2c98fc20-1129-4df7-a44a-1c087900e2ed
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modnpm.go
Cap npm registry requests with errgroup while avoiding the extra context wrapper.
1 participant
Closes #14
Summary by CodeRabbit