Open
Conversation
2ca0941 to
78ba166
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
IstanbulCoveragePlugin, the duplicate-loader guard only checksrule.useas an object or string; if any existing rules use an array foruse, they won’t be detected and could result in duplicate instrumentation—consider handling the array form as well. - The new
build:coveragescript hard-codesCOVERAGE=truewhile the workflow also passesCOVERAGEviaenv; it may be clearer to rely on a single source of truth (either the script or the workflow) to avoid confusing or conflicting configurations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `IstanbulCoveragePlugin`, the duplicate-loader guard only checks `rule.use` as an object or string; if any existing rules use an array for `use`, they won’t be detected and could result in duplicate instrumentation—consider handling the array form as well.
- The new `build:coverage` script hard-codes `COVERAGE=true` while the workflow also passes `COVERAGE` via `env`; it may be clearer to rely on a single source of truth (either the script or the workflow) to avoid confusing or conflicting configurations.
## Individual Comments
### Comment 1
<location> `fec.config.js:20-25` </location>
<code_context>
+ options.module.rules = options.module.rules || [];
+
+ // Guard against duplicate rules on incremental multi-run builds
+ const hasIstanbulLoader = options.module.rules.some(
+ (rule) =>
+ rule.use?.loader === '@jsdevtools/coverage-istanbul-loader' ||
+ (typeof rule.use === 'string' && rule.use.includes('istanbul'))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Broaden the duplicate-loader guard to handle `rule.use` arrays and non-object shapes.
This guard only handles `rule.use?.loader` and the case where `rule.use` is a string. In many Webpack configs `use` is an array (e.g. `use: [{ loader: '...', options: {} }, 'style-loader']`), so an existing Istanbul loader in that array would be missed and a duplicate rule could be added on later builds. Consider normalizing `rule.use` to an array and scanning each entry (string or object) for the Istanbul loader name.
```suggestion
// Guard against duplicate rules on incremental multi-run builds
const hasIstanbulLoader = options.module.rules.some((rule) => {
if (!rule) {
return false;
}
// Handle the simple case where the loader is attached directly
if (typeof rule.loader === 'string' && rule.loader.includes('istanbul')) {
return true;
}
const useEntries = Array.isArray(rule.use)
? rule.use
: rule.use
? [rule.use]
: [];
return useEntries.some((entry) => {
if (!entry) {
return false;
}
if (typeof entry === 'string') {
return entry.includes('istanbul');
}
if (typeof entry === 'object') {
const loaderName = typeof entry.loader === 'string' ? entry.loader : undefined;
return !!loaderName && loaderName.includes('istanbul');
}
return false;
});
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Import fixtures for coverage upload to Currents
All tests should import this way.
78ba166 to
e01f658
Compare
Member
Author
|
@sourcery-ai dismiss |
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
IstanbulCoveragePlugin, the duplicate-loader guard only checksrule.usewhen it is an object or string; ifuseis an array (a common Webpack pattern) the check will miss it and may add duplicate Istanbul loaders—consider handling the array case as well. - The new
COVERAGE-driven build path in the GitHub Actions workflow assumesvars.COVERAGEis explicitly set to"true"or"false"; it might be safer to give it a default (e.g. viaenvat the job level or${{ vars.COVERAGE || 'false' }}) to avoid ambiguous behavior when the variable is unset.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `IstanbulCoveragePlugin`, the duplicate-loader guard only checks `rule.use` when it is an object or string; if `use` is an array (a common Webpack pattern) the check will miss it and may add duplicate Istanbul loaders—consider handling the array case as well.
- The new `COVERAGE`-driven build path in the GitHub Actions workflow assumes `vars.COVERAGE` is explicitly set to `"true"` or `"false"`; it might be safer to give it a default (e.g. via `env` at the job level or `${{ vars.COVERAGE || 'false' }}`) to avoid ambiguous behavior when the variable is unset.
## Individual Comments
### Comment 1
<location> `fec.config.js:21-24` </location>
<code_context>
+ options.module.rules = options.module.rules || [];
+
+ // Guard against duplicate rules on incremental multi-run builds
+ const hasIstanbulLoader = options.module.rules.some(
+ (rule) =>
+ rule.use?.loader === '@jsdevtools/coverage-istanbul-loader' ||
+ (typeof rule.use === 'string' && rule.use.includes('istanbul'))
+ );
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Istanbul loader duplicate detection misses common `use: [ { loader } ]` patterns.
This only detects `rule.use?.loader` and string `use`, so it will miss common patterns like `use: [{ loader: '@jsdevtools/coverage-istanbul-loader' }]` and `use: ['@jsdevtools/coverage-istanbul-loader']`. As a result, incremental builds can still accumulate duplicate Istanbul rules. To make this robust, normalize `rule.use` to an array and check each entry (both objects and strings) for the Istanbul loader.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
e01f658 to
9b15b71
Compare
Member
Author
|
@sourcery-ai review |
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
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
Testing #858 in main
Testing steps