fix: improve project favicon detection for monorepos#1024
fix: improve project favicon detection for monorepos#1024ponbac wants to merge 1 commit intopingdotgg:mainfrom
Conversation
|
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)
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 |
30cac2c to
b936f17
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4a8d330eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c396c88 to
551d9f2
Compare
|
My 2c
I would add unit tests for this file rather than relying on integration tests. Since this is a new file, I would also add some solid docblocks to provide good human and AI context for the decisions being made. Hopefully the reviewers will pick that up and think it's a good idea for their code standards. |
Not sure about separating them right now, but I also feel like I agree with the unit tests, added now. Also added an explanatory comment to the |
a079d93 to
470ab3d
Compare
9f02bfb to
8cc9e19
Compare
47c7166 to
b41ebd3
Compare
ea10d7f to
49c8309
Compare
| // and nested scans only hit `git check-ignore` once per candidate. | ||
| const allowedRelativePaths = yield* Effect.promise(() => | ||
| filterGitIgnoredPaths(projectRoot, uncachedRelativePaths), | ||
| ).pipe(Effect.orElseSucceed(() => uncachedRelativePaths)); |
There was a problem hiding this comment.
orElseSucceed cannot catch Effect.promise defects
Low Severity
Effect.promise converts promise rejections into defects (unchecked exceptions), but Effect.orElseSucceed only catches expected (typed) errors. The fallback () => uncachedRelativePaths is unreachable — if filterGitIgnoredPaths ever rejects, the defect propagates instead of gracefully falling back. Compare with the isInsideGitWorkTree call at line 392–393, which correctly uses .catch(() => false) on the promise itself before wrapping with Effect.promise. Using Effect.tryPromise here instead of Effect.promise would make orElseSucceed actually effective.
49c8309 to
09da607
Compare
09da607 to
7404ccc
Compare
| return nextPromise; | ||
| } | ||
|
|
||
| export function clearWorkspaceIndexCache(cwd: string): void { |
There was a problem hiding this comment.
🟢 Low src/workspaceEntries.ts:425
clearWorkspaceIndexCache deletes entries from workspaceIndexCache and inFlightWorkspaceIndexBuilds, but any in-flight buildWorkspaceIndex promise (already spawned before the clear) will still execute its .then() handler and repopulate workspaceIndexCache after the function returns. This means a caller who clears the cache to force a fresh rebuild may unexpectedly receive stale data from the completing in-flight build instead of a new index.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/workspaceEntries.ts around line 425:
`clearWorkspaceIndexCache` deletes entries from `workspaceIndexCache` and `inFlightWorkspaceIndexBuilds`, but any in-flight `buildWorkspaceIndex` promise (already spawned before the clear) will still execute its `.then()` handler and repopulate `workspaceIndexCache` after the function returns. This means a caller who clears the cache to force a fresh rebuild may unexpectedly receive stale data from the completing in-flight build instead of a new index.
Evidence trail:
- apps/server/src/workspaceEntries.ts lines 408-421 (REVIEWED_COMMIT): Promise creation with `.then()` handler that unconditionally sets `workspaceIndexCache.set(cwd, next)`
- apps/server/src/workspaceEntries.ts lines 425-428 (REVIEWED_COMMIT): `clearWorkspaceIndexCache` deletes from maps but cannot cancel the running promise
- apps/server/src/workspaceEntries.ts lines 34-35 (REVIEWED_COMMIT): Cache definitions as module-level Maps
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| entries: rankedEntries.map((candidate) => candidate.entry), | ||
| truncated: index.truncated || matchedEntryCount > limit, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Entire new file is dead code, never imported
Medium Severity
The entire 453-line workspaceEntries.ts file is dead code. Its exports clearWorkspaceIndexCache and searchWorkspaceEntries are not imported anywhere in the codebase. The file duplicates nearly all logic from apps/server/src/workspace/Layers/WorkspaceEntries.ts (the Effect-based service version) using raw async functions instead. All existing consumers continue to use WorkspaceEntriesLive from workspace/Layers/WorkspaceEntries.ts.
| lookup.fileSystem.readFile(resolvedPath), | ||
| ); | ||
| if (Option.isSome(fileOption)) { | ||
| return Option.some(fileOption.value.path); |
There was a problem hiding this comment.
Favicon probe reads entire file content unnecessarily
Low Severity
findFirstReadableFaviconPath calls readFileIfExists with readFile (full binary read) for each favicon candidate, but only uses the resulting path — the file content is discarded. Since resolveExistingPath already confirms the file exists via realPath and stat, the full content read is redundant. The route handler in projectFaviconRoute.ts then reads the same file again to serve it, doubling I/O for potentially large PNG/ICO files.


The
XXLtag seems a little weird for this PR? It is a net +288 LoC, excluding test files.What Changed
Improve project favicon detection without introducing a broad recursive search.
Closes #1020.
Rules, in order:
Example:
/repo/favicon.svgor/repo/public/favicon.ico.<link rel="icon" ... href="...">tags or object-style{ href, rel: "icon" }declarations in a small set of common source files, then resolve that target.Example:
/repo/index.htmlpoints to/brand/logo.svg, so the route first tries/repo/public/brand/logo.svgand then/repo/brand/logo.svg.apps/andpackages/, plus other top-level directories in the requested root, using the same non-git directory ignore rules asworkspaceEntries.Example:
/repo/apps/web/public/favicon.pngor/repo/frontend/public/favicon.pngis found when the cwd is/repo.Example:
/repo/favicon.svgwins over/repo/apps/web/public/favicon.ico.I also extracted the shared gitignore probing into
apps/server/src/gitIgnore.tsso bothworkspaceEntriesandprojectFaviconRouteuse the same chunkedgit check-ignore --no-index -z --stdinfiltering. I also extracted the shared workspace directory ignore policy intoapps/server/src/workspaceIgnore.ts, soprojectFaviconRoutenow uses the same non-git skip rules asworkspaceEntrieswhen scanning nested roots instead of keeping a separate ignore list.Why
The current behavior misses common monorepo layouts where the favicon lives under an app directory instead of the repo root.
There was already an earlier PR for this in the upstream repo: #690. That version was larger and messier. This keeps the rules simple on purpose, leaving more extensive changes to project icons for trusted maintainers.
UI Changes
Not applicable.
Checklist
Note
Improve project favicon detection to support monorepos with gitignore filtering
ProjectFaviconResolver.resolvePathto search nestedapps/,packages/, and top-level child directories when no root favicon is found, enabling monorepo support.gitIgnore.tsutilities (filterGitIgnoredPaths,isInsideGitWorkTree) that stream candidates togit check-ignorein 256 KiB chunks.workspaceIgnore.tsto skip well-known directories (.git,node_modules,.next,dist, etc.) during directory traversal.workspaceEntries.tsfor a cacheable, git-backed or filesystem-fallback workspace index with fuzzy-ranked search.FileSystemservice injection in the static file handler inwsServer.tsthat previously caused runtime errors.📊 Macroscope summarized 7404ccc. 7 files reviewed, 2 issues evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues
Note
Medium Risk
Modifies server-side favicon resolution and adds git-driven ignore filtering/search across workspace roots, which could affect file discovery performance and edge cases around path/permission handling. Git/process failures are handled fail-open, reducing risk of hiding files but still changing behavior.
Overview
Project favicon resolution is expanded to better support monorepos. The resolver now checks well-known favicon paths and icon metadata in common source files in the requested root, then repeats the same checks across first-level
apps/,packages/, and other top-level directories (skipping ignored workspace dirs), preferring root matches before falling back to the generated SVG.Gitignore-aware filtering is added and shared. New
gitIgnore.tsprovidesisInsideGitWorkTreeand chunkedfilterGitIgnoredPaths(git check-ignore --no-index -z --stdin) with fail-open behavior; both the favicon resolver and the newworkspaceEntries.tsuse this to avoid considering gitignored candidates.Testing and wiring updates. New unit tests cover gitignore chunking/fail-open and expanded favicon route scenarios (nested roots, unreadable files, gitignored root/app/source cases), and
wsServer.tsnow providesFileSystemto the static-file handler Effect pipeline.Written by Cursor Bugbot for commit 7404ccc. This will update automatically on new commits. Configure here.