Conversation
Contributor
📝 WalkthroughWalkthroughThe PR implements deterministic image-proxy fallback: tracks unique image failures, rewrites failing image/src and background-image URLs to a fallback host, probes the primary host in background, activates a one-time global fallback persisted in sessionStorage, and emits a single Sentry warning on fallback activation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Rendered Image
participant Tracker as Image Failure Tracker
participant Config as SessionStorage/Config
participant Proxy as Proxy Handler
participant Sentry as Sentry Logger
UI->>Tracker: image error (capture phase)
Tracker->>Tracker: verify PRIMARY_HOST && not retried
Tracker->>UI: replace element src → FALLBACK_HOST
Tracker->>Tracker: add key to failedUrls
Tracker->>Config: check failedUrls size >= threshold?
alt Threshold reached
Tracker->>Config: sessionStorage[FALLBACK_SESSION_KEY] = "1"
Tracker->>Proxy: setProxyBase(IMAGE_PROXY_FALLBACK)
Tracker->>UI: rewrite CSS background-image URLs to FALLBACK_HOST
Tracker->>Sentry: captureMessage("fallback activated", extras)
else Threshold not reached
Note over Tracker: continue tracking
end
rect rgba(100,150,200,0.5)
Note over Tracker,Proxy: Background probe flow
Tracker->>Proxy: probe primary-host image (with timeout)
alt Probe fails/timeout
Tracker->>Config: sessionStorage[FALLBACK_SESSION_KEY] = "1"
Tracker->>Proxy: setProxyBase(IMAGE_PROXY_FALLBACK)
else Probe succeeds
Note over Tracker: no change
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/app/_components/image-failure-tracker.tsx (1)
7-11:⚠️ Potential issue | 🟠 MajorThe override-detection fix is still using a different resolver than
defaults.ts.The earlier hard-coded proxy concern is only partially addressed. Line 33 treats any parsed string that does not contain
images.ecency.comas a user override, butapps/web/src/defaults.ts:32-57only honors values inALLOWED_IMAGE_SERVERSthat differ fromdefaultImageServer. A stale/invalidimage_proxycan therefore leave the app on the primary host while this tracker skips both the probe and the cached-fallback path. Please reuse the shared proxy-resolution/helper/constants instead of duplicating the logic here. A regression test with an invalid stored value would help.Also applies to: 27-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/_components/image-failure-tracker.tsx` around lines 7 - 11, The override-detection in image-failure-tracker (using PRIMARY_HOST, FALLBACK_HOST, PROBE_TIMEOUT_MS, SESSION_KEY) incorrectly treats any parsed image_proxy string not containing "images.ecency.com" as a user override; replace this ad-hoc logic with the shared resolver/constant logic used in defaults.ts (specifically reuse ALLOWED_IMAGE_SERVERS and defaultImageServer or the shared resolveImageServer helper) so only allowed overrides are honored, ensure the probe/cached-fallback path runs when a stored image_proxy is invalid/stale, and add a regression test that stores an invalid image_proxy value to verify the tracker falls back to probing and caching the fallback host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/_components/image-failure-tracker.tsx`:
- Around line 109-127: The background probe created inside the useEffect can
outlive the component; add a cleanup function that cancels the timeout and
detaches the image handlers to prevent stale callbacks calling
switchToFallback() after unmount. Specifically, inside the useEffect that
references globalSwitched, hasNonDefaultProxy(), PROBE_TIMEOUT_MS and creates
const img and const timeout, return a cleanup that calls clearTimeout(timeout),
sets img.onload = null and img.onerror = null, and sets img.src = "" (or
otherwise aborts the request) only if the probe was created so you don't touch
undefined variables.
- Around line 13-24: The retry dedupe currently uses retriedUrls (Set<string>)
keyed by src which prevents later <img> elements with the same URL from
attempting a fallback; change the dedupe to be per DOM node: replace retriedUrls
with a WeakSet<HTMLImageElement> (e.g., retriedElements) or a Map<string,
WeakSet<HTMLImageElement>> if you must group by src, update all references that
check/add to retriedUrls to instead check/add the actual image element (the same
places around the retry logic and where retriedUrls is used at lines ~39-43 and
~137-142), and update _resetImageProxyFallback to clear the new retriedElements
structure so tests can reset state; keep failedUrls keyed by URL as-is.
---
Duplicate comments:
In `@apps/web/src/app/_components/image-failure-tracker.tsx`:
- Around line 7-11: The override-detection in image-failure-tracker (using
PRIMARY_HOST, FALLBACK_HOST, PROBE_TIMEOUT_MS, SESSION_KEY) incorrectly treats
any parsed image_proxy string not containing "images.ecency.com" as a user
override; replace this ad-hoc logic with the shared resolver/constant logic used
in defaults.ts (specifically reuse ALLOWED_IMAGE_SERVERS and defaultImageServer
or the shared resolveImageServer helper) so only allowed overrides are honored,
ensure the probe/cached-fallback path runs when a stored image_proxy is
invalid/stale, and add a regression test that stores an invalid image_proxy
value to verify the tracker falls back to probing and caching the fallback host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6182125-c3cb-4654-b4a8-892c6c018bef
📒 Files selected for processing (2)
apps/web/src/app/_components/image-failure-tracker.tsxapps/web/src/specs/features/shared/image-failure-tracker.spec.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx (3)
47-51: Nit: Redundant mock clear.Line 49 calls
mockSetProxyBase.mockClear(), butvi.clearAllMocks()on line 47 already clears all mocks includingmockSetProxyBase.♻️ Remove redundant call
beforeEach(() => { vi.clearAllMocks(); _resetImageProxyFallback(); - mockSetProxyBase.mockClear(); probeInstances = []; urlCounter = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx` around lines 47 - 51, The test teardown double-clears mocks: vi.clearAllMocks() already resets mockSetProxyBase, so remove the redundant mockSetProxyBase.mockClear() call; update the teardown in image-failure-tracker.spec.tsx (remove the mockSetProxyBase.mockClear() line) and ensure remaining cleanup still calls _resetImageProxyFallback(), clears probeInstances and resets urlCounter as before.
248-266: Consider more robust DOM cleanup for SSR test.The manually appended
<img>elements are only removed in the happy path. Ifrender()or the assertions throw, these elements remain in the DOM and could affect subsequent tests.♻️ Use try/finally for guaranteed cleanup
it("rewrites SSR-rendered img elements on hydration when fallback is cached", () => { sessionStorage.setItem("image_proxy_fallback_active", "1"); // Simulate SSR-rendered img elements already in the DOM const img1 = document.createElement("img"); img1.src = `https://${PRIMARY_HOST}/p/abc123?format=match`; const img2 = document.createElement("img"); img2.src = `https://${PRIMARY_HOST}/u/user1/avatar/medium`; document.body.appendChild(img1); document.body.appendChild(img2); + try { render(<ImageFailureTracker />); expect(img1.src).toContain(FALLBACK_HOST); expect(img2.src).toContain(FALLBACK_HOST); - - document.body.removeChild(img1); - document.body.removeChild(img2); + } finally { + img1.remove(); + img2.remove(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx` around lines 248 - 266, The test that simulates SSR-rendered <img> elements (in image-failure-tracker.spec.tsx) should guarantee DOM cleanup by wrapping the DOM mutation, render(<ImageFailureTracker />) call, and assertions in a try/finally block and removing img1 and img2 in the finally to ensure they are removed even if render or assertions throw; locate the img element creation variables (img1, img2), the render call, and the expect assertions and move the document.body.removeChild(img1)/removeChild(img2) into the finally so cleanup always runs.
10-14: Optional: Simplify the mock wrapper.The wrapper function adds indirection that isn't needed since
mockSetProxyBaseis already avi.fn().♻️ Proposed simplification
const mockSetProxyBase = vi.fn(); vi.mock("@ecency/render-helper", async () => ({ ...(await vi.importActual("@ecency/render-helper")), - setProxyBase: (...args: unknown[]) => mockSetProxyBase(...args) + setProxyBase: mockSetProxyBase }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx` around lines 10 - 14, The vi.mock wrapper for "@ecency/render-helper" adds an unnecessary layer by forwarding to mockSetProxyBase; replace the wrapper function with a direct reference so the mock exports setProxyBase: mockSetProxyBase instead of setProxyBase: (...args: unknown[]) => mockSetProxyBase(...args), keeping the vi.fn() mockSetProxyBase and the rest of the imported actual module unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx`:
- Around line 47-51: The test teardown double-clears mocks: vi.clearAllMocks()
already resets mockSetProxyBase, so remove the redundant
mockSetProxyBase.mockClear() call; update the teardown in
image-failure-tracker.spec.tsx (remove the mockSetProxyBase.mockClear() line)
and ensure remaining cleanup still calls _resetImageProxyFallback(), clears
probeInstances and resets urlCounter as before.
- Around line 248-266: The test that simulates SSR-rendered <img> elements (in
image-failure-tracker.spec.tsx) should guarantee DOM cleanup by wrapping the DOM
mutation, render(<ImageFailureTracker />) call, and assertions in a try/finally
block and removing img1 and img2 in the finally to ensure they are removed even
if render or assertions throw; locate the img element creation variables (img1,
img2), the render call, and the expect assertions and move the
document.body.removeChild(img1)/removeChild(img2) into the finally so cleanup
always runs.
- Around line 10-14: The vi.mock wrapper for "@ecency/render-helper" adds an
unnecessary layer by forwarding to mockSetProxyBase; replace the wrapper
function with a direct reference so the mock exports setProxyBase:
mockSetProxyBase instead of setProxyBase: (...args: unknown[]) =>
mockSetProxyBase(...args), keeping the vi.fn() mockSetProxyBase and the rest of
the imported actual module unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1168ed5-4e09-4ca3-8ba8-c00b154af0a0
📒 Files selected for processing (2)
apps/web/src/app/_components/image-failure-tracker.tsxapps/web/src/specs/features/shared/image-failure-tracker.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/_components/image-failure-tracker.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Tests