fix(clients): persist project icons across reloads and reconnects - #10138

1. browse_code — apps/mobile/src/lib/projectFaviconCache.ts

2. browse_code — packages/client-runtime/src/projectFaviconCache.ts

💭 I'm checking whether the earlier concern about runDatabase re-entering the runtime was actually resolved—the author claims the cache no longer re-enters ManagedRuntime since MobileEnvironmentCacheStore now hands over an already-yielded MobileDatabase instance, but I still see runPromise being called directly in a persistence module at the current head, so I want to verify that claim against the actual code. Since I already flagged this exact issue and the author's fix appears partial, I'm weighing whether to re-flag it given the guidance that duplicate comments should only be posted if the code changed in direct response to my original concern. Now I'm scanning the diff for genuinely new issues in the latest commit rather than the same finding, checking whether other in-scope files introduce fresh convention violations beyond what I already flagged. I notice the new code throws plain new Error(...) in several places rather than using Schema.TaggedErrorClass, but since this favicon cache code is Promise-based rather than Effect-service code, it's unclear whether that convention really applies here — it feels like a stretch to flag it strongly. Now I want to check environment-cache-store.ts and client-cache-state.ts for similar convention issues.

3. browse_code — apps/mobile/src/connection/environment-cache-store.ts

4. browse_code — apps/mobile/src/state/client-cache-state.ts

💭 I'm checking whether the author's fix to the favicon cache actually resolves the runtime re-entry concern—looking at whether running the effect this way still bypasses the managed runtime as before, since the claim that it "no longer re-enters" needs verification against the actual code change. But the deeper issue remains: the module-global mutable database and singleton projectFaviconCache still hide a dependency, which is exactly the pattern the convention warns against. The author gave a design rationale for this tradeoff, so I'm weighing whether re-flagging the same concern after the code changed counts as noise or as a legitimately new violation worth raising. I'm now scanning the rest of the diff for other issues I haven't already flagged, checking things like key decoding, import patterns, and whether raw new Error throws in the favicon cache module violate the tagged-error conventions. I decide that module isn't an Effect service, so raw errors there aren't a real violation, and the status-derived message is fine. I also check the mobile database's new listCache for consistency with existing error handling, and note that the environment-cache-store.ts hydrate call ties back to the same module-singleton cache concern I raised earlier, so I treat it as related rather than a new finding. Similarly, I flag apps/web/src/connection/storage.ts as another instance of Effect.promise wrapping a singleton inside a layer — same pattern family. I also note that Effect.promise turns rejections into defects, and since projectFaviconCache.clearAll() can reject when unattached, that becomes an untyped defect rather than a typed Effect failure, which matters for callers needing fallback behavior. Checking further though, the persist errors inside hydrate, clear, and remove are already caught internally, so clearAll likely won't actually reject — mostly safe in practice. I also glance at environment-cache-store.ts's clear logic, which chains a promise into clearEnvironmentCache with mapped errors, and that looks fine, as does the mobile-database.ts unavailable stub. Let me check mobile-database.ts's new code region for any lint suppressions.

5. browse_code — apps/mobile/src/persistence/mobile-database.ts

6. browse_code — apps/mobile/src/state/assets.ts

💭 I'm checking the assets.ts file for the new atom family, wondering whether the createUrl/preparedConnection parameter is actually a service dependency being injected into an atom factory rather than an Effect service itself, per the convention on flagging such factories. The imageCache parameter is a Promise-based cache singleton, not an Effect service, and createUrl/preparedConnection look like atom accessors, similar to the existing connection atom pattern used elsewhere—so this doesn't seem like a clear violation. I want to look at similar atom families in state to confirm this is consistent with existing conventions.

7. browse_code — packages/client-runtime/src/state/assets.ts

💭 The missing labels on new atoms are stylistic, not part of the conventions list, so I'll skip flagging that. The decodeKey/JSON.parse issue was already raised and rejected by the author, so that thread's resolved too — now I need to check if there are any genuinely new findings left to surface. Looking more closely at Effect.runPromise in projectFaviconCache.ts: the conventions say these runtime calls belong at boundaries and shouldn't become dependencies of other Effect services, but runDatabase is exactly that — an imperative adapter that gets pulled into MobileEnvironmentCacheStore.make as a dependency, plus it hides state in a module global. This is the same violation I flagged before, just persisting in a reshaped form after the author's fix attempt. I'm weighing whether re-flagging is warranted given the author's rationale that atoms need synchronous access outside any runtime, but that doesn't actually resolve the module-global-mutable-service problem. Checking whether there's a cleaner pattern the author missed — I notice Atom.runtime is used elsewhere (client-cache-state.ts, assets.ts) to let atoms access services through a runtime-scoped mechanism, suggesting the favicon cache could similarly be wired through an atom runtime instead of a bare module-level adapter. I already raised this architectural point once though, so I decide not to repeat it and instead scan for anything genuinely new — checking for lint suppressions (none found) and verifying the mobile database import stays a correct namespace import at the service boundary, with the pre-existing client-cache-state.ts import left unchanged. Now I want to look at the storage.ts changes around those specific line ranges.

8. browse_code — apps/web/src/connection/storage.ts

9. browse_code — apps/web/src/connection/storage.ts

💭 The web version's use of the module-singleton cache is fine since it's a plain IndexedDB wrapper calling a genuine external browser API through Effect.promise, with no Effect service dependency. The mobile version is trickier though, since its singleton is backed by an actual Effect service (MobileDatabase) injected through a mutable global setter and relies on Effect.runPromise in persistence code — that's the concerning pattern. The author already responded to my earlier comment, defending the module-singleton approach by noting a full Context.Service would still need a runtime-bound accessor for the atom family, so this smaller model is intentional. Reposting the same architectural concern would just be duplicate noise since the author explicitly addressed it — but I'm wondering if there's a genuinely new, distinct issue introduced by the fix itself, specifically around the new attachProjectFaviconDatabase mechanism that mutates module state and can fail at runtime if ordering is wrong, which I haven't raised before. I traced through whether Effect.runPromise on the database effect discarding the app runtime's context (loggers, tracing, fiber refs) is a real problem, but callers wrap it with Effect.promise, converting failures into defects that get caught internally by the cache's hydrate/clear methods, so it doesn't actually surface as an issue. Now I want to verify this line was genuinely changed in commit 3069ab5 versus what it looked like at 8090a47, since the earlier version likely used Runtime.runtime or a ManagedRuntime instead of a bare runPromise call.

10. browse_code — apps/mobile/src/lib/projectFaviconCache.ts

💭 The fix eliminated the ManagedRuntime re-entry, the most serious issue, but Effect.runPromise with a module-global service handle remains. I'm now weighing whether this residual pattern still violates the convention that imperative adapters shouldn't become dependencies of other Effect services, checking it against projectFaviconCache specifically. projectFaviconCache still feeds MobileEnvironmentCacheStore through the same pattern, but that was already flagged and the author's rationale about the atom family needing a singleton stands, so no new comment there. Scanning further, projectFavicon.ts is just a pure helper, fine as-is. The browser IndexedDB cache is plain JS with no Effect involvement, but I notice it opens a second, independent IndexedDB connection rather than going through the existing layer-based approach used in storage.ts, which is worth flagging. That module-global handling doesn't cleanly match the "don't create per-feature runtimes for a shared resource" convention since it's a distinct database, so it's not a clear violation, though the lack of scoped release is still notable. I also want to flag the raw rejected promise for a missing storage attachment—that error escapes into Effect.promise as an untyped defect rather than being represented as a typed failure in the service layer. Checking whether it actually surfaces as a problem, I trace the call sites and see the failure is always caught during hydrate and persist operations, so its impact is minimal. I'm scanning the rest of the diff for other Effect-service touches: the new listCache query in the mobile database doesn't filter by environment, but that's not a convention violation, and the new cache-kind literal is fine. Checking the other convention areas—lint-disabling comments and test coverage for backend changes—everything looks properly handled, so overall I don't find any new clear violations beyond what I've already reported. Given the instructions, when there are nothing findings, I should just state "All clear" as my final summary rather than adding more detail. But I want to reconsider whether the mobile module-level runtime issue deserves a report, since the flagged line is a changed line introduced in this PR and matches the categories I was told to flag in persistence code. Though this issue was already raised and the author responded and resolved it, so re-posting the same concern on the same file would just be duplicate noise rather than a new finding.