Refactor/component helper struct#9
Open
agustingroh wants to merge 4 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughIntroduces an instance-based API: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Helper as ComponentHelper
participant WorkerPool
participant ScanOSS_DB as "scanoss / DB"
Caller->>Helper: NewHelper(cfg)
Caller->>Helper: GetComponentsVersion(ctx, logger, []ComponentDTO)
activate Helper
Helper->>WorkerPool: start workers (maxWorkers)
activate WorkerPool
loop per component (concurrent)
WorkerPool->>ScanOSS_DB: scan component (uses Helper.db)
ScanOSS_DB-->>WorkerPool: scan results
end
WorkerPool-->>Helper: aggregate results
deactivate WorkerPool
Helper-->>Caller: []Component (resolved versions, status)
deactivate Helper
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
agustingroh
force-pushed
the
refactor/component-helper-struct
branch
from
d659c83 to
84f353e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
componenthelper/component.go (2)
88-90:⚠️ Potential issue | 🟡 MinorVariable
cshadows method receiver.The loop variable
cshadows the method receiverc, which could lead to bugs if the receiver is accessed inside the loop body.✏️ Proposed fix
- for _, c := range sanitisedComponents { - jobs <- c + for _, comp := range sanitisedComponents { + jobs <- comp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component.go` around lines 88 - 90, The for-loop in the method on receiver named `c` uses a loop variable `c` which shadows the receiver; rename the loop variable (e.g., to `comp` or `sanitisedComp`) in the loop that iterates over `sanitisedComponents` and update any uses inside the loop body (including the send to `jobs <- ...`) so the receiver `c` is not shadowed and the code remains clear and correct.
48-49: 🛠️ Refactor suggestion | 🟠 MajorRemove unused
Inputfield fromCfg.The
Inputfield is documented but never used—input is now passed directly toGetComponentsVersion(ctx, componentDTOs). This dead field adds confusion to the API.🧹 Proposed fix
type Cfg struct { // MaxWorkers is the maximum number of concurrent goroutines used to resolve versions. // If <= 0, defaults to MaxWorkers (5). MaxWorkers int // Ctx is the context used for cancellation and deadline propagation. Ctx context.Context // S is the sugared logger for structured logging. S *zap.SugaredLogger // DB is the database connection used to query component data. DB *sqlx.DB - // Input is the list of components whose versions need to be resolved. - Input []ComponentDTO }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component.go` around lines 48 - 49, Remove the dead Input field from the Cfg struct: delete the `Input []ComponentDTO` field and its docs in the `Cfg` type (used for componenthelper/component.go), and ensure no remaining code references it (search for `Cfg.Input`); confirm callers use the existing GetComponentsVersion(ctx, componentDTOs) path (e.g., where `componentDTOs` is passed) and update any comments or tests that mention `Cfg.Input`.
🧹 Nitpick comments (3)
componenthelper/component_test.go (1)
70-78: Fix typo in variable name:componenentHelper→componentHelper.The variable name has a typo ("componenent" instead of "component").
✏️ Proposed fix
- componenentHelper := NewComponentHelper(Cfg{ + componentHelper := NewComponentHelper(Cfg{ MaxWorkers: 2, Ctx: ctx, S: s, }) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := componenentHelper.GetComponentsVersion(ctx, tt.input) + result := componentHelper.GetComponentsVersion(ctx, tt.input)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component_test.go` around lines 70 - 78, Rename the misspelled variable componenentHelper to componentHelper where it's declared and referenced: change the instantiation using NewComponentHelper(Cfg{...}) and the subsequent call to GetComponentsVersion(ctx, tt.input) to use componentHelper so the variable name is consistent and typo-free.CHANGELOG.md (1)
69-70: Add a trailing newline at end of file.The file appears to be missing a trailing newline after line 70. Most linters and version control systems expect files to end with a newline character.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 69 - 70, The file ends without a trailing newline after the final link line "[1.0.0]: https://github.com/scanoss/go-component-helper/compare/v0.6.0...v1.0.0"; add a single newline character at EOF so the last line terminates with '\n' (ensure the file ends with a blank line or newline byte).componenthelper/component.go (1)
78-80: Avoid mutating receiver state for default value logic.
c.maxWorkers = MaxWorkerspermanently modifies the helper instance. IfGetComponentsVersionis called multiple times, the first call's mutation persists. Use a local variable instead:♻️ Proposed fix
- if c.maxWorkers <= 0 { - c.maxWorkers = MaxWorkers + workers := c.maxWorkers + if workers <= 0 { + workers = MaxWorkers } - numWorkers := min(c.maxWorkers, numJobs) + numWorkers := min(workers, numJobs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component.go` around lines 78 - 80, In GetComponentsVersion avoid mutating the receiver by not assigning to c.maxWorkers; instead compute a local variable (e.g., maxWorkers := c.maxWorkers; if maxWorkers <= 0 { maxWorkers = MaxWorkers }) and use that local value for subsequent logic; update any references in GetComponentsVersion that currently read c.maxWorkers to use the local maxWorkers so the helper instance state is not changed across calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@componenthelper/component.go`:
- Around line 88-90: The for-loop in the method on receiver named `c` uses a
loop variable `c` which shadows the receiver; rename the loop variable (e.g., to
`comp` or `sanitisedComp`) in the loop that iterates over `sanitisedComponents`
and update any uses inside the loop body (including the send to `jobs <- ...`)
so the receiver `c` is not shadowed and the code remains clear and correct.
- Around line 48-49: Remove the dead Input field from the Cfg struct: delete the
`Input []ComponentDTO` field and its docs in the `Cfg` type (used for
componenthelper/component.go), and ensure no remaining code references it
(search for `Cfg.Input`); confirm callers use the existing
GetComponentsVersion(ctx, componentDTOs) path (e.g., where `componentDTOs` is
passed) and update any comments or tests that mention `Cfg.Input`.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 69-70: The file ends without a trailing newline after the final
link line "[1.0.0]:
https://github.com/scanoss/go-component-helper/compare/v0.6.0...v1.0.0"; add a
single newline character at EOF so the last line terminates with '\n' (ensure
the file ends with a blank line or newline byte).
In `@componenthelper/component_test.go`:
- Around line 70-78: Rename the misspelled variable componenentHelper to
componentHelper where it's declared and referenced: change the instantiation
using NewComponentHelper(Cfg{...}) and the subsequent call to
GetComponentsVersion(ctx, tt.input) to use componentHelper so the variable name
is consistent and typo-free.
In `@componenthelper/component.go`:
- Around line 78-80: In GetComponentsVersion avoid mutating the receiver by not
assigning to c.maxWorkers; instead compute a local variable (e.g., maxWorkers :=
c.maxWorkers; if maxWorkers <= 0 { maxWorkers = MaxWorkers }) and use that local
value for subsequent logic; update any references in GetComponentsVersion that
currently read c.maxWorkers to use the local maxWorkers so the helper instance
state is not changed across calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71bc67f8-e064-4909-a8da-c140d033f20d
📒 Files selected for processing (7)
CHANGELOG.mdcomponenthelper/component.gocomponenthelper/component_entity.gocomponenthelper/component_test.gocomponenthelper/semver.gocomponenthelper/semver_test.gocomponenthelper/utils/semver_test.go
💤 Files with no reviewable changes (1)
- componenthelper/utils/semver_test.go
agustingroh
force-pushed
the
refactor/component-helper-struct
branch
2 times, most recently
from
afddd93 to
dc4cc10
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
componenthelper/component.go (2)
37-50: Remove unused fields fromCfgstruct.The
Ctx(line 43) andInput(line 49) fields are defined inCfgbut are never used byNewHelper. The refactored API passes these as method parameters toGetComponentsVersion(ctx, componentDTOs)instead. Keeping unused fields in the config struct can confuse users about the intended API.Proposed fix to remove unused fields
// Cfg holds the configuration for the component helper. type Cfg struct { // MaxWorkers is the maximum number of concurrent goroutines used to resolve versions. // If <= 0, defaults to MaxWorkers (5). MaxWorkers int - // Ctx is the context used for cancellation and deadline propagation. - Ctx context.Context // S is the sugared logger for structured logging. S *zap.SugaredLogger // DB is the database connection used to query component data. DB *sqlx.DB - // Input is the list of components whose versions need to be resolved. - Input []ComponentDTO }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component.go` around lines 37 - 50, The Cfg struct contains unused fields Ctx and Input that are no longer used by NewHelper; remove the Ctx and Input fields from the Cfg definition and update any comments to reflect the reduced configuration surface. Verify NewHelper and callers rely on passing context and component list into GetComponentsVersion(ctx, componentDTOs) and adjust any construction sites of Cfg to stop setting Ctx/Input so there are no unused assignments or imports.
88-90: Variable shadowing:cshadows the receiver.The loop variable
cshadows the method receiverc *ComponentHelper. This doesn't cause a bug here since the receiver isn't used inside the loop, but it's confusing and error-prone for future maintenance.Proposed fix to rename the loop variable
- for _, c := range sanitisedComponents { - jobs <- c + for _, comp := range sanitisedComponents { + jobs <- comp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component.go` around lines 88 - 90, The for-loop variable name `c` shadows the method receiver `c *ComponentHelper`; rename the loop variable to a non-conflicting identifier (e.g., `comp` or `sc`) in the loop that sends elements of `sanitisedComponents` into `jobs` so the receiver `c` is not shadowed—update the loop `for _, c := range sanitisedComponents { jobs <- c }` to use the new name (and send that variable into `jobs`) to avoid shadowing and improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@componenthelper/component.go`:
- Around line 78-80: GetComponentsVersion currently mutates the ComponentHelper
field c.maxWorkers when it's <= 0; instead preserve instance state by
introducing a local variable (e.g., maxWorkers) initialized from c.maxWorkers,
set it to MaxWorkers if <= 0, and use that local variable throughout the
function (rather than assigning back to c.maxWorkers). Update any references in
GetComponentsVersion to use the local maxWorkers so ComponentHelper.maxWorkers
is not modified.
In `@README.md`:
- Around line 26-46: Update the Component Output JSON example to match Go's
default JSON marshaling for domain.ComponentStatus by changing the lowercase
status fields to their exported names: replace "status" with "Status" and inside
it replace "status_code" and "message" with "StatusCode" and "Message"
respectively so the example reflects the actual serialized output from the
ComponentStatus type.
---
Nitpick comments:
In `@componenthelper/component.go`:
- Around line 37-50: The Cfg struct contains unused fields Ctx and Input that
are no longer used by NewHelper; remove the Ctx and Input fields from the Cfg
definition and update any comments to reflect the reduced configuration surface.
Verify NewHelper and callers rely on passing context and component list into
GetComponentsVersion(ctx, componentDTOs) and adjust any construction sites of
Cfg to stop setting Ctx/Input so there are no unused assignments or imports.
- Around line 88-90: The for-loop variable name `c` shadows the method receiver
`c *ComponentHelper`; rename the loop variable to a non-conflicting identifier
(e.g., `comp` or `sc`) in the loop that sends elements of `sanitisedComponents`
into `jobs` so the receiver `c` is not shadowed—update the loop `for _, c :=
range sanitisedComponents { jobs <- c }` to use the new name (and send that
variable into `jobs`) to avoid shadowing and improve clarity.
🪄 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: 03df3836-945c-4462-9a56-bf39d4107bd7
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdcomponenthelper/component.gocomponenthelper/component_entity.gocomponenthelper/component_test.gocomponenthelper/semver.gocomponenthelper/semver_test.gocomponenthelper/utils/semver_test.go
💤 Files with no reviewable changes (1)
- componenthelper/utils/semver_test.go
✅ Files skipped from review due to trivial changes (2)
- componenthelper/component_test.go
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- componenthelper/semver.go
BREAKING CHANGE: GetComponentsVersion is now a method on ComponentHelper instead of a standalone function. ComponentVersionCfg has been renamed to Cfg. Context and input are now passed as method parameters.
agustingroh
force-pushed
the
refactor/component-helper-struct
branch
from
dc4cc10 to
e2b8fc6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
componenthelper/component.go (1)
70-73:⚠️ Potential issue | 🟠 MajorAvoid mutating
ComponentHelper.maxWorkersinside request execution path.This concern is still present: mutating
c.maxWorkershere creates persistent side effects across calls and can race when the same helper is used concurrently. Use a local variable instead.Proposed fix
- if c.maxWorkers <= 0 { - c.maxWorkers = MaxWorkers - } - numWorkers := min(c.maxWorkers, numJobs) + maxWorkers := c.maxWorkers + if maxWorkers <= 0 { + maxWorkers = MaxWorkers + } + numWorkers := min(maxWorkers, numJobs)#!/bin/bash # Verify whether GetComponentsVersion mutates helper state. rg -n -C2 'c\.maxWorkers\s*=' componenthelper/component.go rg -n -C2 'numWorkers\s*:=\s*min\(c\.maxWorkers,\s*numJobs\)' componenthelper/component.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/component.go` around lines 70 - 73, The code mutates c.maxWorkers during request handling (c.maxWorkers <= 0 { c.maxWorkers = MaxWorkers }) which creates persistent state and races; instead, inside the request path (e.g., in GetComponentsVersion or the function using c), capture into a local variable like maxWorkers := c.maxWorkers, then if maxWorkers <= 0 set the local to MaxWorkers, and compute numWorkers := min(maxWorkers, numJobs) using that local; keep all changes local and do not assign back to c.maxWorkers to avoid side effects and races.
🧹 Nitpick comments (1)
componenthelper/semver_test.go (1)
169-210: Consider adding a<=operator case for completeness.
HasSemverOperatoris well covered; adding<=would close the remaining common-operator gap and protect against regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@componenthelper/semver_test.go` around lines 169 - 210, Test suite TestHasSemverOperator is missing a case for the "<=" operator; add a test case inside the testCases slice (in TestHasSemverOperator) that uses input "<=1.2.3" and expected true so HasSemverOperator is exercised for the less-than-or-equal operator and the common-operator gap is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@componenthelper/component.go`:
- Around line 70-73: The code mutates c.maxWorkers during request handling
(c.maxWorkers <= 0 { c.maxWorkers = MaxWorkers }) which creates persistent state
and races; instead, inside the request path (e.g., in GetComponentsVersion or
the function using c), capture into a local variable like maxWorkers :=
c.maxWorkers, then if maxWorkers <= 0 set the local to MaxWorkers, and compute
numWorkers := min(maxWorkers, numJobs) using that local; keep all changes local
and do not assign back to c.maxWorkers to avoid side effects and races.
---
Nitpick comments:
In `@componenthelper/semver_test.go`:
- Around line 169-210: Test suite TestHasSemverOperator is missing a case for
the "<=" operator; add a test case inside the testCases slice (in
TestHasSemverOperator) that uses input "<=1.2.3" and expected true so
HasSemverOperator is exercised for the less-than-or-equal operator and the
common-operator gap is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f09144da-4709-4bce-9c4e-f636c2949edd
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdcomponenthelper/component.gocomponenthelper/component_entity.gocomponenthelper/component_test.gocomponenthelper/semver.gocomponenthelper/semver_test.gocomponenthelper/utils/semver_test.go
💤 Files with no reviewable changes (1)
- componenthelper/utils/semver_test.go
✅ Files skipped from review due to trivial changes (2)
- componenthelper/semver.go
- componenthelper/component_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests