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:
✨ 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 |
… directory scan Adds PWA manifest.json/site.webmanifest icon parsing with size-aware selection and a directory-scan fallback over common asset folders. Includes tests for manifest and directory-scan detection paths.
197f718 to
12e6e2b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ 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 12e6e2b. Configure here.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. This PR introduces significant new capability (PWA manifest parsing and directory scanning for favicon resolution) with 396 lines of new code. New features that add user-facing behavior warrant human review. Additionally, there are unresolved review comments identifying potential bugs in the new logic. You can customize Macroscope's approvability policy. Learn more. |
- parseIconSize now scans every size token and returns the largest - candidatePriority keys include the directory so directory order wins - resolveManifestIcon tolerates valid JSON null/non-object manifests - directory scan skips unreadable directories instead of aborting
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together. This adds PWA manifest parsing and a broad directory scan with many filename, extension, folder, and priority rules for favicon discovery. #4424 provides a narrower monorepo favicon repair without the general repository scan. If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. |

Closes #4636
What Changed
Two new resolution stages in
ProjectFaviconResolver, both framework-agnostic and both tried only after every existing check fails:manifest.json,public/manifest.json,site.webmanifestandpublic/site.webmanifest, resolving eachicons[].srcon disk and preferring the largest declaredsizes.public,app,src,src/app,assets,src/assets,assets/icons,assets/icon,static,resources,images,img,media,app-icon,.idea) for well-known basenames (favicon,icon,logo,apple-touch-icon,app-icon,brand,app), picking by directory → name → extension priority.ProjectFaviconResolutionErrorgainsread-manifestandscan-directoryoperations. Paths stay guarded inside the workspace root.Why
The resolver is good at React-shaped repos and falls back to the generic folder glyph for almost everything else. That's the complaint in #4304 ("works well for React projects and poorly for others"), #1020 and #4935, and it's what the reporter on #2561 pushed back with after it was closed: "auto detect doesn't work for most projects, for example BE, cli databases or some specific monorepos."
The two stages here are deliberately not per-framework. A Flutter app is the motivating case: no
package.json, noindex.html, icon atassets/icons/icon.pngreferenced only fromflutter_launcher_icons.yaml. Nothing in the current resolver — or in any of the open favicon PRs — finds it, but a prioritized scan of asset directories does, and the same scan covers Android, Go, Rust, CLI and backend repos without teaching the resolver about any of them.How this differs from the other open favicon PRs
These are complementary, not competing — each works a different axis:
apps/*,packages/*,services/*. This PR searches the selected root more thoroughly. Orthogonal; both can land, and this one does not close monorepo projects do not resolve nested app favicons #1020.Note on custom icons
t3.jsonalready supportsiconPathand it is checked before everything here, so a project that wants an explicit icon has an escape hatch today. This PR only changes what happens when nothing is declared.Verification
npx vitest run src/project/ProjectFaviconResolver.test.ts— 20/20 pass, covering manifest selection, multi-size strings, scan priority ordering, unreadable directories, permission failures, and manifest→scan fallback.npx tsc --noEmitclean.Additive behind the same
resolvePathAPI, and the added filesystem reads only happen on the path that currently returnsnull. Cursor Bugbot reviewed 709dbde as Low Risk.