fix: cache project favicons across web and mobile - #4767
Conversation
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces a new content-based caching mechanism for project favicons across web and mobile, including SHA-256 versioning on the server, new cache key generation, and request lifecycle tracking. The scope of new logic and behavioral changes across multiple platforms warrants human review. You can customize Macroscope's approvability policy. Learn more. |
📝 WalkthroughWalkthroughProject favicon asset URLs now include content revisions, with shared cache-key derivation and updated web and mobile loading behavior. Web rendering preserves valid images across refresh failures, while mobile image caching uses project-specific keys. ChangesProject favicon refresh flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AssetAccess
participant ProjectFavicon
participant ProjectFaviconImage
participant ImageLoader
AssetAccess->>ProjectFavicon: return content-versioned favicon URL
ProjectFavicon->>ProjectFaviconImage: provide cacheKey and favicon URL
ProjectFaviconImage->>ImageLoader: begin active image request
ImageLoader-->>ProjectFaviconImage: report load or failure
ProjectFaviconImage->>ProjectFaviconImage: record or invalidate cache entry
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
- Version favicon asset URLs when the file changes - Reuse stable project-scoped cache keys across signed URLs - Configure mobile image caching and add coverage
417da35 to
f77304e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/shared/src/projectFavicon.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the inferred return type.
The return annotation is redundant here.
Proposed change
-): string { +) {As per coding guidelines, “Prefer inferred types over explicit annotations and do not use
any.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/projectFavicon.ts` at line 7, Remove the explicit string return annotation from the function in projectFavicon.ts and let TypeScript infer its return type, while leaving the function implementation unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/assets/AssetAccess.test.ts`:
- Around line 223-228: Update the test around issueAssetUrl to provide the
repository’s controllable Clock test layer and freeze the timestamp before both
project-favicon URL calls. Ensure both invocations run under the same fixed
clock value so the existing equality assertion remains deterministic.
In `@apps/server/src/assets/AssetAccess.ts`:
- Around line 344-346: Update the favicon revision construction in
AssetAccess.ts around modifiedAt/revision to use a content digest or durable
content version, ensuring every content change—including equal-size
updates—produces a new filename. In AssetAccess.test.ts lines 238-244, add a
controlled equal-size content replacement and assert that the generated revision
filename changes.
---
Nitpick comments:
In `@packages/shared/src/projectFavicon.ts`:
- Line 7: Remove the explicit string return annotation from the function in
projectFavicon.ts and let TypeScript infer its return type, while leaving the
function implementation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e38efc06-abce-43f3-bbf0-a91f691d3cae
📒 Files selected for processing (7)
apps/mobile/src/components/ProjectFavicon.tsxapps/server/src/assets/AssetAccess.test.tsapps/server/src/assets/AssetAccess.tsapps/web/src/components/ProjectFavicon.test.tsxapps/web/src/components/ProjectFavicon.tsxpackages/shared/src/projectFavicon.test.tspackages/shared/src/projectFavicon.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/src/assets/AssetAccess.ts (1)
173-173: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the
Cryptoservice dependency to favicon issuance.Line 173 resolves
Crypto.Cryptobefore the resource switch, so workspace-file and attachment URL issuance now requires a Crypto layer even though those branches never hash bytes. Move the lookup into theproject-faviconbranch, after confirming a canonical favicon exists.Proposed adjustment
- const crypto = yield* Crypto.Crypto; ... if (relativePath && canonicalFaviconPath) { + const crypto = yield* Crypto.Crypto;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/assets/AssetAccess.ts` at line 173, Move the Crypto.Crypto service lookup from the shared resource-issuance setup into the project-favicon branch in AssetAccess, resolving it only after confirming a canonical favicon exists. Keep workspace-file and attachment URL issuance independent of the Crypto service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/src/components/ProjectFavicon.tsx`:
- Around line 104-107: Update the ProjectFavicon error handling around the image
URL/request generation so onError deletes the shared loadedFaviconKeys entry
only when the failed request still matches the current generation; ignore stale
errors after a newer revision has loaded. Add a regression test covering a stale
error arriving after the same revision succeeds and verify it does not evict the
shared key or trigger fallback/reload flicker.
---
Nitpick comments:
In `@apps/server/src/assets/AssetAccess.ts`:
- Line 173: Move the Crypto.Crypto service lookup from the shared
resource-issuance setup into the project-favicon branch in AssetAccess,
resolving it only after confirming a canonical favicon exists. Keep
workspace-file and attachment URL issuance independent of the Crypto service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d67bccd0-2fd2-470f-849f-a8baa9c88d9b
📒 Files selected for processing (4)
apps/mobile/src/components/ProjectFavicon.tsxapps/server/src/assets/AssetAccess.test.tsapps/server/src/assets/AssetAccess.tspackages/shared/src/projectFavicon.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/projectFavicon.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/assets/AssetAccess.ts (1)
177-181: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCompute the favicon expiry after the hashing work.
The expiry is bucketed from the clock before path resolution, file I/O, SHA-256 hashing, secret loading, and signing. A request started in the final milliseconds of a bucket can therefore return an already-expired token. Recompute the bucket from a fresh clock reading immediately before building/signing the claims, and add a bucket-boundary regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/assets/AssetAccess.ts` around lines 177 - 181, Move the project-favicon expiresAt calculation out of the initial timestamp section and recompute it from a fresh clock reading immediately before constructing and signing the claims, after path resolution, I/O, hashing, and secret loading complete. Preserve the existing non-favicon TTL behavior, and add a regression test covering a request that crosses a favicon token-bucket boundary.
♻️ Duplicate comments (1)
apps/mobile/src/components/ProjectFavicon.tsx (1)
104-110: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIgnore callbacks from superseded favicon requests.
key={props.faviconUrl}remounts the native image, but an old native callback may still arrive after the URL changes. If that old request errors before the refreshed URL loads,markProjectFaviconFailedevicts the still-valid entry and this component shows the fallback. A late oldonLoadcan also overwrite the recorded URL. Track the latest requested URL/generation and guard both callbacks; add a regression covering an old callback arriving before the new URL loads.Proposed guard
+ const latestFaviconUrl = useRef(props.faviconUrl); + latestFaviconUrl.current = props.faviconUrl; + onLoad={() => { + if (latestFaviconUrl.current !== props.faviconUrl) return; markProjectFaviconLoaded(props.cacheKey, props.faviconUrl); setStatus("loaded"); }} onError={() => { + if (latestFaviconUrl.current !== props.faviconUrl) return; if (!markProjectFaviconFailed(props.cacheKey, props.faviconUrl)) return; setStatus("error"); }}Import
useRefand add a component-level regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/src/components/ProjectFavicon.tsx` around lines 104 - 110, Update ProjectFavicon’s load and error callbacks to track the latest requested favicon URL or request generation via useRef, and ignore callbacks from superseded native image requests before mutating cache state or component status. Ensure only callbacks for the current props.faviconUrl can call markProjectFaviconLoaded, markProjectFaviconFailed, or setStatus, and add a regression test covering an old callback arriving before the new URL loads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/server/src/assets/AssetAccess.ts`:
- Around line 177-181: Move the project-favicon expiresAt calculation out of the
initial timestamp section and recompute it from a fresh clock reading
immediately before constructing and signing the claims, after path resolution,
I/O, hashing, and secret loading complete. Preserve the existing non-favicon TTL
behavior, and add a regression test covering a request that crosses a favicon
token-bucket boundary.
---
Duplicate comments:
In `@apps/mobile/src/components/ProjectFavicon.tsx`:
- Around line 104-110: Update ProjectFavicon’s load and error callbacks to track
the latest requested favicon URL or request generation via useRef, and ignore
callbacks from superseded native image requests before mutating cache state or
component status. Ensure only callbacks for the current props.faviconUrl can
call markProjectFaviconLoaded, markProjectFaviconFailed, or setStatus, and add a
regression test covering an old callback arriving before the new URL loads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d0b1fa4-5e10-4096-aaae-1092add5aef9
📒 Files selected for processing (4)
apps/mobile/src/components/ProjectFavicon.tsxapps/mobile/src/components/projectFaviconCache.test.tsapps/mobile/src/components/projectFaviconCache.tsapps/server/src/assets/AssetAccess.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Reviewed by Cursor Bugbot for commit 6ada303. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/src/components/projectFaviconCache.ts`:
- Around line 6-7: Bound the module-level bookkeeping in currentFaviconUrls and
loadedFaviconKeys so favicon revision updates do not accumulate indefinitely.
Evict superseded revision entries only when they are no longer active or needed
by concurrent requests, preserving in-flight request tracking and current
favicon behavior.
- Around line 13-17: Update beginProjectFaviconRequest and the corresponding
onLoad/onError stale-callback handling so URL-bearing requests cannot bypass
validation when cacheKey is null. Prefer enforcing a non-null cacheKey for
requests with faviconUrl; otherwise add per-request URL/session tracking for
uncached images, and add a regression test covering a null-key A→B remount where
the stale A callback cannot update B’s status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c81f8034-3eb9-41ed-a49b-06c1c4655b4b
📒 Files selected for processing (3)
apps/mobile/src/components/ProjectFavicon.tsxapps/mobile/src/components/projectFaviconCache.test.tsapps/mobile/src/components/projectFaviconCache.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
## What's Changed * fix(mobile): support pre-Liquid-Glass iOS bottom toolbar by @gabrielelpidio in pingdotgg/t3code#4984 * fix(server): restore PR detection without HOME by @StiensWout in pingdotgg/t3code#4985 * fix(web): fill fast mode icon by @maria-rcks in pingdotgg/t3code#5004 * fix: cache project favicons across web and mobile by @gabrielelpidio in pingdotgg/t3code#4767 * perf(ci): cut stale runs and redundant setup by @t3dotgg in pingdotgg/t3code#4802 **Full Changelog**: pingdotgg/t3code@v0.0.32-nightly.20260730.957...v0.0.32-nightly.20260730.958 Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.32-nightly.20260730.958
Brings in 30 upstream commits since abc409c, including the libghostty-vt web terminal (pingdotgg#4860), `npx t3 pair` (pingdotgg#4955), project favicon caching (pingdotgg#4767), and forward-compatible config union decoding (pingdotgg#5055). Three files needed manual resolution; the rest auto-merged. - modelOptions.test.ts / AssetAccess.test.ts: both sides added tests and imports at the same spot, so both sides are kept. - ThreadTerminalDrawer: upstream replaced xterm.js with the libghostty-vt surface, so the fork's terminal typography feature was reimplemented on the new API. Font family and size now flow through GhosttyTerminalSurface `font` options and `setFont`, which owns loading, cell remeasurement, refit, and re-render, replacing the manual FitAddon dance. The appearance default font stack ends in `monospace`, which would swallow the Nerd Font glyph fallbacks the renderer appends to any face it is given, so the default now defers to the renderer's own stack and only a genuinely custom face is forwarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
What Changed
Why
Signed asset URLs change as tokens rotate, which caused browsers and mobile clients to reload unchanged project favicons. Versioning the asset by the favicon's file metadata provides a stable cache identity while still invalidating the cache when the favicon changes.
Checklist
Note
Medium Risk
Changes signed asset URL shape and expiry for project favicons plus client caching behavior; scope is limited to favicon assets and is covered by new tests, but incorrect hashing or cache logic could show stale or missing icons.
Overview
Signed project favicon URLs change whenever tokens rotate, which forced web and mobile to reload unchanged icons. This PR makes asset filenames content-addressed (SHA-256 hash in the path) and aligns favicon token expiry to 30-minute buckets, so the same file keeps a stable URL identity across re-issues.
Shared
getProjectFaviconCacheKeyties cache identity to environment, workspace root, and the versioned filename—not the signed token.Web keeps showing the last successfully loaded favicon while a new URL loads in a hidden
<img>, and only drops to the folder icon when the displayed image errors.Mobile adds a small request lifecycle cache so stale
onLoad/onErrorfrom superseded URLs are ignored, and wires expo-image withcacheKey,recyclingKey, andmemory-diskpolicy.Tests cover bucketed expiry, hash-based filenames, cache-key stability, refresh/fallback behavior, and mobile supersession.
Reviewed by Cursor Bugbot for commit 0d0779c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Cache project favicons by content-hash key across web and mobile
getProjectFaviconCacheKeyinpackages/shared/src/projectFavicon.tsto derive a stable cache identity fromenvironmentId, workspace root, and a versioned filename, independent of transient signed tokens.ProjectFaviconImageholds the previous image on screen while preloading a refreshed URL, only swapping on success and guarding error handlers by src match.ProjectFaviconImagereplaces a local Set-based cache with coordinated per-cacheKeyrequest tracking, ignoring load/error callbacks from superseded URLs.Macroscope summarized 0d0779c.
Summary by CodeRabbit