fix(server): find project icons inside monorepo workspace packages - #5347
fix(server): find project icons inside monorepo workspace packages#5347mackinleysmith wants to merge 1 commit into
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f794a2d. Configure here.
| for (const directory of packages) { | ||
| if (excluded.has(directory)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Negated globs block pnpm re-inclusions
Medium Severity
In findWorkspacePackages, negated workspace globs add paths to excluded, and the final pass drops any package in that set. pnpm allows a later positive glob to re-include a package after a broad negation (for example !packages/* then packages/web). Those re-included packages stay in excluded, so they are never scanned for icons and favicon resolution can miss the only valid icon.
Reviewed by Cursor Bugbot for commit f794a2d. Configure here.
There was a problem hiding this comment.
Fixed by removal in a6229a9 — the analysis was correct.
pnpm re-inclusion after a broad negation (!packages/* then packages/web) was genuinely mishandled: excluded was a one-way set with no re-inclusion path.
Rather than fix the ordering, the PR no longer parses workspace globs at all. It scans the apps/* and packages/* convention directly, so negation semantics are out of scope. Projects with a custom layout can still point at an icon with iconPath in t3.json.
ApprovabilityVerdict: Needs human review This PR adds new monorepo favicon discovery capability, searching workspace packages when icons aren't found at the root. While well-tested and relatively low-risk, this introduces new runtime behavior beyond a simple bug fix. The open review comment references code not present in this PR. You can customize Macroscope's approvability policy. Learn more. |
f794a2d to
83d35d9
Compare
Monorepo roots almost never carry a favicon of their own, so projects like a Turborepo with its app at apps/frontend fell back to the generic project icon even though apps/frontend/app/favicon.ico was right there. When the root scan comes up empty, the resolver now re-checks the same well-known icon locations inside each apps/* and packages/* directory. The root is still checked first, so nothing changes for single-package projects. Model: Claude Opus 5. Harness: T3 Code.
83d35d9 to
a6229a9
Compare


Problem
Project favicons are resolved by checking a fixed list of paths relative to the project root (
favicon.ico,public/favicon.*,app/favicon.ico,src/app/icon.*, …). A monorepo root almost never has any of those — the icon lives inside a workspace package. A Turborepo whose web app sits atapps/frontendshows the generic fallback icon even thoughapps/frontend/app/favicon.icoexists.Setting
iconPathint3.jsonworks around it, but it shouldn't be needed for the standard layout.Fix
When the root scan comes up empty,
ProjectFaviconResolverre-checks the same well-known icon locations and<link rel="icon">source files inside eachapps/*andpackages/*directory.Most of the diff is moving the existing root scan into a
findIconWithin(projectCwd, packageDir)helper that takes a package directory prefix; the new logic is one directory listing and the loop that calls it.t3.jsoniconPathstill wins over everything, so single-package projects are unchanged.apps/*is scanned beforepackages/*, children sorted, so the result is deterministic.apps/orpackages/directory is skipped. Other filesystem failures surface asProjectFaviconResolutionError(list-packages), matching how candidate stats are already handled rather than silently reporting "no icon".This deliberately keys off the
apps//packages/convention rather than parsingpnpm-workspace.yamlorpackage.jsonworkspace globs. That covers the layout Turborepo scaffolds and nearly everyone uses, without a glob engine in the resolver. A project with a custom layout still hast3.jsoniconPath.No UI change beyond the icon itself now resolving, so there's nothing meaningful to screenshot.
Testing
vp test run src/project/ProjectFaviconResolver.test.ts src/assets/AssetAccess.test.ts— 25 passed. Five new cases cover a package favicon file, package-level<link rel="icon">resolution, root precedence, apps-before-packages ordering, and a propagated listing failure.Also ran the resolver against a real Turborepo: it now returns
apps/frontend/app/favicon.icoinstead ofnull, and this repo still resolves via its ownt3.jsoniconPath.Model: Claude Opus 5. Harness: T3 Code.
Note
Fix project icon resolution to search inside monorepo workspace packages
ProjectFaviconResolver.resolvePathnow searchesapps/*andpackages/*workspace directories for icons when no icon is found at the repo root.findIconWithinto scope icon discovery (favicon candidates and<link rel="icon">hrefs in source files) to a single directory, andfindPackageDirectoriesto enumerate workspace package dirs.apps/is checked beforepackages/, with subdirectories sorted alphabetically.NotFoundfilesystem errors during package directory listing are surfaced asProjectFaviconResolutionErrorwith the newlist-packagesoperation.Macroscope summarized a6229a9.