Repository navigation
fix(dependency):SP-4443 scan dependencies with include all files type… - #926
Conversation
📝 WalkthroughWalkthroughDependencyTask now loads filter rules with graceful handling for missing files and passes them to BlackListDependencies via a new DependencyFilterRule contract, preventing crashes when Include all file types is enabled. ChangesDependency Filter Loading Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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
🧹 Nitpick comments (3)
src/main/task/scanner/dependency/DependencyTask.ts (2)
48-48: 💤 Low valueClarify the log message.
The message mentions "all extensions enabled," but the filters being skipped are folder-name exclusions (e.g.,
node_modules), not file-extension filters. While the message references the "Include all file types" setting that causedfilter.jsonto be absent, the wording could be clearer about what is actually being skipped.💬 Suggested rewording
- log.info('[ DependencyTask ]: filter.json not found, skipping dependency filters (all extensions enabled)'); + log.info('[ DependencyTask ]: filter.json not found, skipping folder exclusions (Include all file types enabled)');🤖 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 `@src/main/task/scanner/dependency/DependencyTask.ts` at line 48, Update the log message in DependencyTask (the log.info call that currently reads "[ DependencyTask ]: filter.json not found, skipping dependency filters (all extensions enabled)") to clearly state that folder-name exclusion filters are being skipped (e.g., node_modules) rather than file-extension filters; locate the log statement in the DependencyTask class/function and replace the wording to reference "folder-name exclusion filters" and the "Include all file types" setting so it's clear what is being skipped.
47-47: 💤 Low valuePrefer async file existence check for consistency.
The method uses
fs.existsSync()(blocking) followed byfs.promises.readFile()(async). For consistency and to avoid blocking the event loop, consider usingfs.promises.access()to check existence asynchronously, or simply attempt the read and catchENOENT.♻️ Async alternative
private async loadDependencyFilterRules(): Promise<DependencyFilterRule[]> { const filterPath = `${this.project.metadata.getMyPath()}/filter.json`; - if (!fs.existsSync(filterPath)) { - log.info('[ DependencyTask ]: filter.json not found, skipping dependency filters (all extensions enabled)'); - return []; - } - const raw = await fs.promises.readFile(filterPath, 'utf8'); - return JSON.parse(raw).filters ?? []; + try { + const raw = await fs.promises.readFile(filterPath, 'utf8'); + return JSON.parse(raw).filters ?? []; + } catch (err) { + if (err.code === 'ENOENT') { + log.info('[ DependencyTask ]: filter.json not found, skipping dependency filters (all extensions enabled)'); + return []; + } + throw err; + } }🤖 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 `@src/main/task/scanner/dependency/DependencyTask.ts` at line 47, Replace the blocking fs.existsSync(filterPath) check with an async existence check: either await fs.promises.access(filterPath, fs.constants.F_OK) inside the async method of DependencyTask (or the surrounding async function) and handle the thrown error, or simply remove the existence check and call await fs.promises.readFile(filterPath) inside a try/catch that specifically handles ENOENT. Update the control flow around the existing fs.promises.readFile() usage so you no longer block the event loop and correctly handle the missing-file error for filterPath.src/main/workspace/tree/blackList/BlackListDependencies.ts (1)
5-11: ⚡ Quick winMark
scopeas optional in the interface.From the context snippets (defaultFilter.ts shows rules like
{ condition: 'starts', value: '.', ftype: 'NAME' }without ascopeproperty), the persisted filter rules do not always include ascopefield. The interface should reflect this runtime reality. Line 29 handles undefined scope correctly by filtering it out, but the type definition should be accurate.📝 Suggested type refinement
export interface DependencyFilterRule { ftype: string; - scope: string; + scope?: string; condition: string; value: string; }🤖 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 `@src/main/workspace/tree/blackList/BlackListDependencies.ts` around lines 5 - 11, The DependencyFilterRule interface incorrectly requires scope even though persisted rules can omit it; update the interface declaration for DependencyFilterRule to make the scope property optional (e.g., scope?: string) so it matches runtime data (check references in BlackListDependencies and defaultFilter.ts to ensure callers already handle undefined scope, no further logic changes required).
🤖 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 `@src/main/task/scanner/dependency/DependencyTask.ts`:
- Around line 45-53: The loadDependencyFilterRules function reads and parses
filter.json but returns parsed.filters without runtime validation; update
loadDependencyFilterRules to validate the parsed JSON before returning: after
JSON.parse(raw) verify parsed.filters is an Array, then ensure each element
matches the DependencyFilterRule shape (e.g., has required keys like condition
and value and correct types), filter out or reject invalid entries (log a
warning or throw a descriptive error), and finally return the validated array
(or [] on validation failure) so downstream code that expects
rule.condition/rule.value is safe.
---
Nitpick comments:
In `@src/main/task/scanner/dependency/DependencyTask.ts`:
- Line 48: Update the log message in DependencyTask (the log.info call that
currently reads "[ DependencyTask ]: filter.json not found, skipping dependency
filters (all extensions enabled)") to clearly state that folder-name exclusion
filters are being skipped (e.g., node_modules) rather than file-extension
filters; locate the log statement in the DependencyTask class/function and
replace the wording to reference "folder-name exclusion filters" and the
"Include all file types" setting so it's clear what is being skipped.
- Line 47: Replace the blocking fs.existsSync(filterPath) check with an async
existence check: either await fs.promises.access(filterPath, fs.constants.F_OK)
inside the async method of DependencyTask (or the surrounding async function)
and handle the thrown error, or simply remove the existence check and call await
fs.promises.readFile(filterPath) inside a try/catch that specifically handles
ENOENT. Update the control flow around the existing fs.promises.readFile() usage
so you no longer block the event loop and correctly handle the missing-file
error for filterPath.
In `@src/main/workspace/tree/blackList/BlackListDependencies.ts`:
- Around line 5-11: The DependencyFilterRule interface incorrectly requires
scope even though persisted rules can omit it; update the interface declaration
for DependencyFilterRule to make the scope property optional (e.g., scope?:
string) so it matches runtime data (check references in BlackListDependencies
and defaultFilter.ts to ensure callers already handle undefined scope, no
further logic changes required).
🪄 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: 2f1dc833-2aed-4683-90f8-82b34a87ae6e
📒 Files selected for processing (3)
CHANGELOG.mdsrc/main/task/scanner/dependency/DependencyTask.tssrc/main/workspace/tree/blackList/BlackListDependencies.ts
1 participant
…s option enabled
Summary by CodeRabbit