Skip to content

fix: use client-side fallbacks for missing project favicons#152

Merged
aaditagrawal merged 1 commit into
mainfrom
sync/20260715-favicon
Jul 14, 2026
Merged

fix: use client-side fallbacks for missing project favicons#152
aaditagrawal merged 1 commit into
mainfrom
sync/20260715-favicon

Conversation

@aaditagrawal

@aaditagrawal aaditagrawal commented Jul 14, 2026

Copy link
Copy Markdown
Owner

What changed

  • Ports upstream #3959: shared projectFavicon helper and client-side fallbacks when project favicon assets are missing.
  • Updates web/mobile ProjectFavicon components and simplifies server asset/HTTP handling for missing favicons.

Validation

  • vp check (0 errors)
  • Tip stack includes focused shared/server coverage for this slice

Stack: 1 of 3.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of missing project favicons across web and mobile.
    • Missing or invalid favicon images now display the appropriate fallback instead of attempting to load unavailable assets.
    • Prevented repeated loading states for project icons that have already loaded successfully.
    • Updated asset requests to return a not-found response when no favicon is available.

Copy link
Copy Markdown
Owner Author

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 effective changed lines (test files excluded in mixed PRs). labels Jul 14, 2026
@aaditagrawal aaditagrawal changed the title Use client-side fallbacks for missing project favicons (#3959) fix: use client-side fallbacks for missing project favicons Jul 14, 2026
@aaditagrawal

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 31625d88-2f79-4697-95e6-4936513d993e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Project favicon fallback handling is centralized around a shared marker and URL predicate. Server asset resolution now returns missing fallbacks as null, while web and mobile components detect marked URLs and render fallback states.

Changes

Project favicon fallback handling

Layer / File(s) Summary
Shared fallback marker and URL predicate
packages/shared/src/projectFavicon.ts, packages/shared/src/projectFavicon.test.ts, packages/shared/package.json
Adds and exports the fallback marker and URL detector, with tests for valid, invalid, query-only, and null inputs.
Server fallback asset flow
apps/server/src/assets/AssetAccess.ts, apps/server/src/assets/AssetAccess.test.ts, apps/server/src/http.ts
Uses the shared marker for issued fallback URLs, resolves fallback claims to null, removes the fallback resolved-asset variant, and removes inline fallback SVG serving from the asset route.
Client favicon rendering
apps/web/src/components/ProjectFavicon.tsx, apps/mobile/src/components/ProjectFavicon.tsx
Detects fallback URLs before rendering images; web tracks loaded favicon sources, and mobile passes marked URLs as non-renderable values.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: juliusmarminge

Sequence Diagram(s)

sequenceDiagram
  participant Server
  participant Client
  participant FaviconPredicate
  participant FaviconFallback
  Server->>Client: issue project-favicon URL with fallback marker
  Client->>FaviconPredicate: inspect favicon URL
  FaviconPredicate-->>Client: identify fallback URL
  Client->>FaviconFallback: render fallback component
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers what changed, but it omits the required Why, UI Changes, and Checklist sections from the template. Add a Why section, include UI Changes if applicable or delete it if not, and complete the checklist items required by the template.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change: moving missing project favicon handling to client-side fallbacks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync/20260715-favicon

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

aaditagrawal commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Merge activity

  • Jul 14, 7:50 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 14, 7:50 PM UTC: @aaditagrawal merged this pull request with Graphite.

@aaditagrawal
aaditagrawal merged commit eb18072 into main Jul 14, 2026
12 checks passed
aaditagrawal added a commit that referenced this pull request Jul 14, 2026
## What changed

- Ports upstream `pingdotgg#3961`: skip APNs delivery for stale working-task activity.
- Extends relay agent-activity row/delivery logic and regression tests.

## Validation

- `vp check` (0 errors)
- Cherry-picked cleanly onto the favicon stack branch

Stack: 2 of 3; depends on #152.
aaditagrawal pushed a commit that referenced this pull request Jul 14, 2026
, pingdotgg#3967)

Content was already adapted via #152#154; keep fork review-diff fixes
and record upstream/main ancestry so the tip is zero commits behind.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 effective changed lines (test files excluded in mixed PRs). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants