Conversation
📝 WalkthroughWalkthroughThis pull request introduces an ignore-list feature for diagnostic warnings across three categories: deprecations, replacements, and vulnerabilities. New configuration properties ( Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/code-actions/quick-fix.test.ts (2)
35-58: Consider verifying command arguments for ignore actions.The tests verify action titles and counts, but do not assert that the command arguments are correct. Consider adding assertions to verify the
action.command.argumentscontain the expected values (code,ignoreTarget,configTarget). This would catch regressions if the argument order or values change.Example addition for the "vulnerability with fix" test:
expect(actions[1]!.command?.arguments).toEqual([ 'vulnerability', 'lodash@4.17.20', ConfigurationTarget.Workspace, ])
71-84: Add tests for deprecation and replacement ignore actions.The test suite only covers upgrade and vulnerability scenarios. The
addIgnoreRulesfunction insrc/providers/code-actions/quick-fix.tsdefines handlers fordeprecationandreplacementdiagnostic types, but no tests exercise these code paths. Adding test cases for both would improve coverage of the ignore-rule functionality.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.mdpackage.jsonsrc/providers/code-actions/index.tssrc/providers/code-actions/quick-fix.tssrc/providers/diagnostics/rules/deprecation.tssrc/providers/diagnostics/rules/replacement.tssrc/providers/diagnostics/rules/vulnerability.tstests/__mocks__/vscode.tstests/__setup__/index.tstests/code-actions/quick-fix.test.ts
| useCommand('npmx.addToIgnore', async (scope: string, name: string, target: ConfigurationTarget) => { | ||
| scope = `ignore.${scope}` | ||
| const config = workspace.getConfiguration(scopedConfigs.scope) | ||
| const current = config.get<string[]>(scope, []) | ||
| if (current.includes(name)) | ||
| return | ||
| await config.update(scope, [...current, name], target) | ||
| }) |
There was a problem hiding this comment.
Validate command arguments before mutating configuration.
npmx.addToIgnore currently trusts runtime arguments. A malformed invocation can write unexpected keys or values into settings.
Suggested patch
useCommand('npmx.addToIgnore', async (scope: string, name: string, target: ConfigurationTarget) => {
+ if (!scope || !name)
+ return
+ if (!['deprecation', 'replacement', 'vulnerability'].includes(scope))
+ return
+
scope = `ignore.${scope}`
const config = workspace.getConfiguration(scopedConfigs.scope)
const current = config.get<string[]>(scope, [])
if (current.includes(name))
return
await config.update(scope, [...current, name], target)
})| config: { | ||
| ignore: { | ||
| deprecation: [], | ||
| replacement: [], | ||
| vulnerability: [], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Mocked config shape is incomplete for modules that read diagnostics flags.
Only config.ignore is mocked here, but consumers like src/providers/code-actions/index.ts access config.diagnostics.*. Please include a minimal diagnostics object in the mock to keep test wiring realistic.
Suggested patch
vi.mock('#state', () => ({
logger: { info: vi.fn(), warn: vi.fn() },
config: {
+ diagnostics: {
+ upgrade: true,
+ deprecation: true,
+ replacement: true,
+ vulnerability: true,
+ },
ignore: {
deprecation: [],
replacement: [],
vulnerability: [],
},
},
}))📝 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.
| config: { | |
| ignore: { | |
| deprecation: [], | |
| replacement: [], | |
| vulnerability: [], | |
| }, | |
| }, | |
| config: { | |
| diagnostics: { | |
| upgrade: true, | |
| deprecation: true, | |
| replacement: true, | |
| vulnerability: true, | |
| }, | |
| ignore: { | |
| deprecation: [], | |
| replacement: [], | |
| vulnerability: [], | |
| }, | |
| }, |
Resolve #52