Discover project favicons recursively for monorepos#690
Discover project favicons recursively for monorepos#690rexlManu wants to merge 1 commit intopingdotgg:mainfrom
Conversation
github-actions
bot
added
the
vouch:unvouched
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| return true; | ||
| } | ||
|
|
||
| const checkIgnore = await runProcess("git", ["check-ignore", "--no-index", "-z", "--stdin"], { |
There was a problem hiding this comment.
🟢 Low src/projectFiles.ts:97
The --no-index flag causes git check-ignore to ignore whether a file is tracked, so force-added tracked files matching .gitignore patterns are incorrectly filtered out. This drops valid files like git add -f config files from the result. Consider removing --no-index so tracked status is respected.
- const checkIgnore = await runProcess("git", ["check-ignore", "--no-index", "-z", "--stdin"], {
+ const checkIgnore = await runProcess("git", ["check-ignore", "-z", "--stdin"], {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/projectFiles.ts around line 97:
The `--no-index` flag causes `git check-ignore` to ignore whether a file is tracked, so force-added tracked files matching `.gitignore` patterns are incorrectly filtered out. This drops valid files like `git add -f` config files from the result. Consider removing `--no-index` so tracked status is respected.
Evidence trail:
1. apps/server/src/projectFiles.ts line 97 shows `git check-ignore --no-index -z --stdin`
2. Git documentation at https://git-scm.com/docs/git-check-ignore confirms: "By default, tracked files are not shown at all since they are not subject to exclude rules; but see '--no-index'." and "--no-index: Don't look in the index when undertaking the checks. This can be used to debug why a path became tracked by e.g. `git add .` and was not ignored by the rules as expected by the user or when developing patterns including negation to match a path previously added with `git add -f`."
3. Function name `filterGitIgnoredPaths` (line 80) indicates intent to filter git-ignored files, but tracked files are not considered ignored by git.
| function shouldIncludeDirent(relativePath: string, dirent: Dirent): boolean { | ||
| if (!dirent.name || dirent.name === "." || dirent.name === "..") { | ||
| return false; | ||
| } | ||
| if (!dirent.isDirectory() && !dirent.isFile()) { | ||
| return false; | ||
| } | ||
| if (isPathInIgnoredDirectory(relativePath)) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟠 High src/projectFiles.ts:264
listProjectFilePaths fails to ignore nested directories like packages/app/node_modules. shouldIncludeDirent passes the full relative path to isPathInIgnoredDirectory, which only checks the first segment against IGNORED_DIRECTORY_NAMES — so packages/app/node_modules is not recognized as ignored and the scanner traverses into dependency trees and build artifacts. Consider checking all path segments in isPathInIgnoredDirectory, or use a separate check in shouldIncludeDirent that tests individual directory names.
function shouldIncludeDirent(relativePath: string, dirent: Dirent): boolean {
if (!dirent.name || dirent.name === "." || dirent.name === "..") {
return false;
}
if (!dirent.isDirectory() && !dirent.isFile()) {
return false;
}
- if (isPathInIgnoredDirectory(relativePath)) {
+ if (
+ isPathInIgnoredDirectory(relativePath) ||
+ IGNORED_DIRECTORY_NAMES.has(dirent.name)
+ ) {
return false;
}
return true;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/projectFiles.ts around lines 264-275:
`listProjectFilePaths` fails to ignore nested directories like `packages/app/node_modules`. `shouldIncludeDirent` passes the full relative path to `isPathInIgnoredDirectory`, which only checks the first segment against `IGNORED_DIRECTORY_NAMES` — so `packages/app/node_modules` is not recognized as ignored and the scanner traverses into dependency trees and build artifacts. Consider checking all path segments in `isPathInIgnoredDirectory`, or use a separate check in `shouldIncludeDirent` that tests individual directory names.
Evidence trail:
apps/server/src/projectFiles.ts lines 37-41: `isPathInIgnoredDirectory` only checks `relativePath.split('/')[0]` (first segment); apps/server/src/projectFiles.ts lines 264-275: `shouldIncludeDirent` passes full `relativePath` to `isPathInIgnoredDirectory`; apps/server/src/projectFiles.ts lines 10-19: `IGNORED_DIRECTORY_NAMES` includes `node_modules`; apps/server/src/workspaceEntries.ts line 166: alternative implementation uses `IGNORED_DIRECTORY_NAMES.has(dirent.name)` which catches nested directories
1 participant
In monorepos where applications life in a own namespace called apps, the favicon search fails. I updated how the favicon discovering works.
I only added the
appsnamespace for now but maybe it makes sense to add other common patterns tooSummary
Testing