Conversation
📝 WalkthroughWalkthroughAdds a document-level image load failure tracker that reports sampled failures to Sentry, strengthens floating-ui auto-update with lifecycle checks and guarded disposal, supplies a DOM context element for a virtual selection reference, and adjusts test environment/polyfills and related tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/src/app/_components/image-failure-tracker.tsx (2)
21-27: Sampling before deduplication may report duplicates.The sampling check on line 21 occurs before the deduplication check on line 25. This means the same failing image could be reported multiple times if it fails repeatedly and passes the random sampling each time. Consider reordering to deduplicate first:
♻️ Suggested reorder
- if (Math.random() > REPORT_SAMPLE_RATE) return; - // Deduplicate rapid failures for same URL const key = src.slice(0, 200); if (recentlyReported.has(key)) return; + + if (Math.random() > REPORT_SAMPLE_RATE) return; + recentlyReported.add(key); setTimeout(() => recentlyReported.delete(key), DEBOUNCE_MS);🤖 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 21 - 27, The sampling check currently runs before deduplication which can allow the same failing image to be reported multiple times; change the order in the failure-reporting logic so that you compute the dedupe key (key = src.slice(0, 200)) and check recentlyReported.has(key) first, add the key and schedule its deletion via setTimeout using DEBOUNCE_MS if not present, and only then apply the Math.random() > REPORT_SAMPLE_RATE sampling check; update the block around REPORT_SAMPLE_RATE, recentlyReported, DEBOUNCE_MS and the key creation to implement this reordered flow.
36-36: Consider typingnavigator.connectionproperly.Instead of casting to
any, you can use the Network Information API type for better type safety:♻️ Suggested type improvement
+interface NavigatorWithConnection extends Navigator { + connection?: { effectiveType?: string }; +} + // Inside the handler: -connection_type: (navigator as any).connection?.effectiveType, +connection_type: (navigator as NavigatorWithConnection).connection?.effectiveType,🤖 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` at line 36, The code currently casts navigator to any when reading connection_type; replace that unsafe cast by using the Network Information API type: treat navigator as Navigator & { connection?: NetworkInformation } (or narrow navigator.connection as NetworkInformation | undefined) and read connection?.effectiveType so you get proper typing for navigator.connection in the image-failure-tracker component (the line assigning connection_type and any related references to navigator.connection).
🤖 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 12-48: Add unit tests for the ImageFailureTracker feature by
creating a new spec under src/specs/ that mounts ImageFailureTracker and
simulates error events to assert behavior: fire an error event with an
HTMLImageElement whose src includes IMAGE_HOST and confirm Sentry.captureMessage
is called with tags "failure_type":"image_load" and "image_host":IMAGE_HOST and
extras include image_src, page_url, connection_type, and online; send an error
event from a non-HTMLImageElement and an image with a non-matching host and
assert Sentry is not called; also test debounce/dedup by triggering two rapid
errors for the same src and asserting only one Sentry call (references:
ImageFailureTracker, handler, recentlyReported, REPORT_SAMPLE_RATE,
DEBOUNCE_MS).
In `@apps/web/src/features/ui/util/safe-auto-update.ts`:
- Around line 80-84: The catch block currently disconnects observer but never
invokes the disposer returned by autoUpdate, leaking listeners; change the setup
so the return value from autoUpdate(...) is captured in an outer-scoped variable
(e.g., disposeAutoUpdate or dispose) and then call that disposer in the catch
block before disconnecting observer and returning, ensuring autoUpdate's cleanup
runs if subsequent setup throws; update any existing variable name (dispose,
disposeAutoUpdate, cleanup) used around the autoUpdate call so the catch can
reference and invoke it.
---
Nitpick comments:
In `@apps/web/src/app/_components/image-failure-tracker.tsx`:
- Around line 21-27: The sampling check currently runs before deduplication
which can allow the same failing image to be reported multiple times; change the
order in the failure-reporting logic so that you compute the dedupe key (key =
src.slice(0, 200)) and check recentlyReported.has(key) first, add the key and
schedule its deletion via setTimeout using DEBOUNCE_MS if not present, and only
then apply the Math.random() > REPORT_SAMPLE_RATE sampling check; update the
block around REPORT_SAMPLE_RATE, recentlyReported, DEBOUNCE_MS and the key
creation to implement this reordered flow.
- Line 36: The code currently casts navigator to any when reading
connection_type; replace that unsafe cast by using the Network Information API
type: treat navigator as Navigator & { connection?: NetworkInformation } (or
narrow navigator.connection as NetworkInformation | undefined) and read
connection?.effectiveType so you get proper typing for navigator.connection in
the image-failure-tracker component (the line assigning connection_type and any
related references to navigator.connection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 445adc69-ca73-4751-a097-37adfdfebc84
📒 Files selected for processing (7)
apps/web/src/app/(dynamicPages)/entry/[category]/[author]/[permlink]/_components/selection-popover/index.tsxapps/web/src/app/_components/image-failure-tracker.tsxapps/web/src/app/_components/index.tsapps/web/src/app/layout.tsxapps/web/src/features/ui/util/safe-auto-update.tsapps/web/src/specs/api/import-route.spec.tsapps/web/src/specs/setup-any-spec.ts
| export function ImageFailureTracker() { | ||
| useEffect(() => { | ||
| const handler = (event: Event) => { | ||
| const el = event.target; | ||
| if (!(el instanceof HTMLImageElement)) return; | ||
|
|
||
| const src = el.src || el.currentSrc; | ||
| if (!src || !src.includes(IMAGE_HOST)) return; | ||
|
|
||
| if (Math.random() > REPORT_SAMPLE_RATE) return; | ||
|
|
||
| // Deduplicate rapid failures for same URL | ||
| const key = src.slice(0, 200); | ||
| if (recentlyReported.has(key)) return; | ||
| recentlyReported.add(key); | ||
| setTimeout(() => recentlyReported.delete(key), DEBOUNCE_MS); | ||
|
|
||
| Sentry.withScope((scope) => { | ||
| scope.setTag("failure_type", "image_load"); | ||
| scope.setTag("image_host", IMAGE_HOST); | ||
| scope.setLevel("warning"); | ||
| scope.setExtras({ | ||
| image_src: src, | ||
| page_url: window.location.href, | ||
| connection_type: (navigator as any).connection?.effectiveType, | ||
| online: navigator.onLine | ||
| }); | ||
| Sentry.captureMessage("Client image load failure: " + IMAGE_HOST); | ||
| }); | ||
| }; | ||
|
|
||
| // Capture phase to catch errors on <img> elements (they don't bubble) | ||
| document.addEventListener("error", handler, true); | ||
| return () => document.removeEventListener("error", handler, true); | ||
| }, []); | ||
|
|
||
| return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if any test file exists for ImageFailureTracker
fd -e spec.ts -e spec.tsx | xargs grep -l "ImageFailureTracker" 2>/dev/null || echo "No test found for ImageFailureTracker"Repository: ecency/vision-next
Length of output: 99
🏁 Script executed:
# Check if ImageFailureTracker is a new file or has git history
git log --oneline --follow -- "apps/web/src/app/_components/image-failure-tracker.tsx" 2>/dev/null | head -5 || echo "File may be new or untracked"Repository: ecency/vision-next
Length of output: 92
🏁 Script executed:
# Check existing test examples in src/specs directory to understand conventions
find apps/web/src/specs -name "*.spec.tsx" -type f | head -3 | xargs wc -lRepository: ecency/vision-next
Length of output: 286
🏁 Script executed:
# Verify the test-utils location mentioned in learnings
test -f "apps/web/src/specs/test-utils.tsx" && echo "test-utils.tsx found" || echo "test-utils.tsx not found"Repository: ecency/vision-next
Length of output: 82
🏁 Script executed:
# Get details of the commit that added ImageFailureTracker
git show d7a385f07 --stat | head -20Repository: ecency/vision-next
Length of output: 1076
🏁 Script executed:
# Check if there's a test file in the same commit
git show d7a385f07 --name-only | grep -i "image-failure"Repository: ecency/vision-next
Length of output: 116
Missing test coverage for new feature.
All new features in @ecency/web require tests. Add a test file in src/specs/ that verifies:
- The error handler filters for
images.ecency.comURLs - Non-image element errors are ignored
- Sentry is called with expected tags and extras
🤖 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 12 - 48,
Add unit tests for the ImageFailureTracker feature by creating a new spec under
src/specs/ that mounts ImageFailureTracker and simulates error events to assert
behavior: fire an error event with an HTMLImageElement whose src includes
IMAGE_HOST and confirm Sentry.captureMessage is called with tags
"failure_type":"image_load" and "image_host":IMAGE_HOST and extras include
image_src, page_url, connection_type, and online; send an error event from a
non-HTMLImageElement and an image with a non-matching host and assert Sentry is
not called; also test debounce/dedup by triggering two rapid errors for the same
src and asserting only one Sentry call (references: ImageFailureTracker,
handler, recentlyReported, REPORT_SAMPLE_RATE, DEBOUNCE_MS).
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/web/src/features/ui/util/safe-auto-update.ts (1)
7-88:⚠️ Potential issue | 🟠 MajorAdd tests for this utility per coding guidelines.
All new features in
@ecency/webrequire tests. This utility contains non-trivial logic around DOM connectivity checks, cleanup coordination, and race condition handling that should be covered by tests. Place tests inapps/web/src/specs/following the*.spec.tsxpattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/ui/util/safe-auto-update.ts` around lines 7 - 88, Add unit tests for the safeAutoUpdate utility covering its DOM-connectivity guard, cleanup/dispose behavior, MutationObserver teardown, and error-swallowing paths; specifically write tests that import safeAutoUpdate and verify: when reference or floating is null it returns a no-op; when elements are detached before or during initialization it returns a no-op and does not call autoUpdate (or its cleanup); safeUpdate calls update when connected and calls dispose (and stops future updates) when either reference.isConnected or floating.isConnected becomes false; dispose disconnects the MutationObserver and calls the cleanup returned by autoUpdate; and initialization errors during autoUpdate setup swallow exceptions and perform cleanup. Use DOM test fixtures (real or jsdom), spy/mocks for the update callback and autoUpdate cleanup, and place the tests as apps/web/src/specs/safe-auto-update.spec.tsx following the project’s *.spec.tsx pattern.
🧹 Nitpick comments (3)
apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx (2)
13-17: Reset shared URL state inbeforeEachfor full test isolation
urlCounteris file-global and accumulates across tests. Resetting it inbeforeEachavoids hidden cross-test coupling.Proposed tweak
beforeEach(() => { vi.clearAllMocks(); vi.spyOn(Math, "random").mockReturnValue(0); // always below sample rate vi.useFakeTimers(); + urlCounter = 0; });Also applies to: 35-39
🤖 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 13 - 17, Reset the file-global counter to ensure test isolation: add a beforeEach that sets urlCounter = 0 so the shared state used by uniqueImageUrl() is reset before each test; locate the top-level urlCounter variable and the uniqueImageUrl() helper and add the beforeEach reset near other test setup to avoid cross-test coupling.
60-67: Removeanyand type the mocked scope callback explicitly
Line 66usesmockScope as any, which bypasses strict typing. Prefer a typedinterfacefor the scope shape and a typed callback signature.Proposed refactor
+interface MockSentryScope { + setTag: ReturnType<typeof vi.fn>; + setLevel: ReturnType<typeof vi.fn>; + setExtras: ReturnType<typeof vi.fn>; +} + -const scopeCb = vi.mocked(Sentry.withScope).mock.calls[0][0]; -const mockScope = { +const scopeCb = vi.mocked(Sentry.withScope).mock.calls[0][0] as ( + scope: MockSentryScope +) => void; +const mockScope: MockSentryScope = { setTag: vi.fn(), setLevel: vi.fn(), setExtras: vi.fn() }; -scopeCb(mockScope as any); +scopeCb(mockScope);As per coding guidelines, “TypeScript strict mode is enabled; all new code should include proper types” and “Prefer
interfacefor defining object shapes in TypeScript.”🤖 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 60 - 67, Define a proper interface for the mocked Sentry scope (with methods setTag(key: string, value: string), setLevel(level: Sentry.Severity | string) and setExtras(extras: Record<string, any>)) and use it to type both the extracted callback and the mock scope instead of casting to any; specifically, type scopeCb as (scope: MockScope) => void when pulling the callback from vi.mocked(Sentry.withScope).mock.calls[0][0] and declare mockScope: MockScope = { setTag: vi.fn(), setLevel: vi.fn(), setExtras: vi.fn() } so you can call scopeCb(mockScope) with strict typing while keeping references to Sentry.withScope, scopeCb, mockScope, and the setTag/setLevel/setExtras methods.apps/web/src/features/ui/util/safe-auto-update.ts (1)
80-84: Consider usingdispose()for consistency.The catch block manually cleans up resources, which works correctly. However, using the centralized
dispose()function would ensure consistent cleanup behavior (same order, setsdisposed = true, nulls references), providing additional defense against edge cases where a pendingsafeUpdatecallback might execute.Optional refactor for consistency
} catch { // Handle initialization errors (e.g., elements removed during autoUpdate setup) - cleanup?.(); - observer?.disconnect(); + dispose(); return () => {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/ui/util/safe-auto-update.ts` around lines 80 - 84, The catch block currently duplicates cleanup logic; replace the manual calls (cleanup?.(); observer?.disconnect(); return () => {};) with a call to the existing dispose() helper so the standardized teardown runs (sets disposed = true, nulls references and disconnects observer) and returns dispose; update the catch to call dispose() and return its result to ensure consistent cleanup ordering and guards against pending safeUpdate callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/features/ui/util/safe-auto-update.ts`:
- Around line 7-88: Add unit tests for the safeAutoUpdate utility covering its
DOM-connectivity guard, cleanup/dispose behavior, MutationObserver teardown, and
error-swallowing paths; specifically write tests that import safeAutoUpdate and
verify: when reference or floating is null it returns a no-op; when elements are
detached before or during initialization it returns a no-op and does not call
autoUpdate (or its cleanup); safeUpdate calls update when connected and calls
dispose (and stops future updates) when either reference.isConnected or
floating.isConnected becomes false; dispose disconnects the MutationObserver and
calls the cleanup returned by autoUpdate; and initialization errors during
autoUpdate setup swallow exceptions and perform cleanup. Use DOM test fixtures
(real or jsdom), spy/mocks for the update callback and autoUpdate cleanup, and
place the tests as apps/web/src/specs/safe-auto-update.spec.tsx following the
project’s *.spec.tsx pattern.
---
Nitpick comments:
In `@apps/web/src/features/ui/util/safe-auto-update.ts`:
- Around line 80-84: The catch block currently duplicates cleanup logic; replace
the manual calls (cleanup?.(); observer?.disconnect(); return () => {};) with a
call to the existing dispose() helper so the standardized teardown runs (sets
disposed = true, nulls references and disconnects observer) and returns dispose;
update the catch to call dispose() and return its result to ensure consistent
cleanup ordering and guards against pending safeUpdate callbacks.
In `@apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx`:
- Around line 13-17: Reset the file-global counter to ensure test isolation: add
a beforeEach that sets urlCounter = 0 so the shared state used by
uniqueImageUrl() is reset before each test; locate the top-level urlCounter
variable and the uniqueImageUrl() helper and add the beforeEach reset near other
test setup to avoid cross-test coupling.
- Around line 60-67: Define a proper interface for the mocked Sentry scope (with
methods setTag(key: string, value: string), setLevel(level: Sentry.Severity |
string) and setExtras(extras: Record<string, any>)) and use it to type both the
extracted callback and the mock scope instead of casting to any; specifically,
type scopeCb as (scope: MockScope) => void when pulling the callback from
vi.mocked(Sentry.withScope).mock.calls[0][0] and declare mockScope: MockScope =
{ setTag: vi.fn(), setLevel: vi.fn(), setExtras: vi.fn() } so you can call
scopeCb(mockScope) with strict typing while keeping references to
Sentry.withScope, scopeCb, mockScope, and the setTag/setLevel/setExtras methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20897d4d-e02b-475a-8053-e6ddda02653b
📒 Files selected for processing (3)
apps/web/src/app/_components/image-failure-tracker.tsxapps/web/src/features/ui/util/safe-auto-update.tsapps/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
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/features/ui/util/safe-auto-update.ts (1)
33-45: Consider checkingcontextElementconnectivity for virtual references.When
referenceis a virtual element (non-Node withgetBoundingClientRect), the code assumes it's always connected (line 36). However, per the relevant context snippet,VirtualSelectionReferenceincludes acontextElementderived fromselection.anchorNode. If that DOM node becomes detached, the virtual reference is effectively stale, butsafeUpdatewould still invokeupdate().You could optionally strengthen this check:
Suggested enhancement
const safeUpdate = () => { if (disposed) return; try { - const refConnected = reference instanceof Node ? reference.isConnected : true; + const refConnected = reference instanceof Node + ? reference.isConnected + : (reference as { contextElement?: Element }).contextElement?.isConnected ?? true; if (!refConnected || !floating.isConnected) { dispose(); return; } update(); } catch { // Swallow errors from computePosition on detached elements } };This would catch the case where a virtual reference's
contextElementis removed from the DOM between selection and positioning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/ui/util/safe-auto-update.ts` around lines 33 - 45, safeUpdate currently treats non-Node references as always connected, so virtual references (e.g., VirtualSelectionReference) whose contextElement (derived from selection.anchorNode) may be detached can still call update(); modify safeUpdate to also check virtual reference connectivity by detecting virtual references (reference not instanceof Node but exposing contextElement or similar) and verify that reference.contextElement is a connected DOM node before calling update(); if contextElement is missing or not connected, call dispose() and return. Ensure you reference the safeUpdate function, the reference variable, VirtualSelectionReference/contextElement, floating.isConnected, update(), and dispose() while implementing this check.apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx (1)
115-117: Consider importingDEBOUNCE_MSconstant to avoid coupling to magic number.The test hardcodes
5000for the debounce window. If the implementation constant changes, this test may silently pass incorrectly or fail for the wrong reason. Consider exportingDEBOUNCE_MSfrom the component module and importing it here.Same applies to
REPORT_SAMPLE_RATE(0.2) referenced in the sampling test comment at line 130.♻️ Suggested approach
Export the constants from the implementation:
// In image-failure-tracker.tsx export const DEBOUNCE_MS = 5000; export const REPORT_SAMPLE_RATE = 0.2;Then import in the test:
-import { ImageFailureTracker } from "@/app/_components/image-failure-tracker"; +import { ImageFailureTracker, DEBOUNCE_MS } from "@/app/_components/image-failure-tracker";// After DEBOUNCE_MS, the same URL can report again act(() => { - vi.advanceTimersByTime(5000); + vi.advanceTimersByTime(DEBOUNCE_MS); });🤖 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 115 - 117, Export the debounce and sampling constants from the implementation (e.g., add exported DEBOUNCE_MS and REPORT_SAMPLE_RATE in the image-failure-tracker module) and update the spec to import those constants instead of using hardcoded values; replace vi.advanceTimersByTime(5000) with vi.advanceTimersByTime(DEBOUNCE_MS) and any hardcoded 0.2 references in the sampling test with REPORT_SAMPLE_RATE so the tests stay in sync with the ImageFailureTracker implementation.
🤖 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/features/ui/util/safe-auto-update.ts`:
- Around line 33-45: safeUpdate currently treats non-Node references as always
connected, so virtual references (e.g., VirtualSelectionReference) whose
contextElement (derived from selection.anchorNode) may be detached can still
call update(); modify safeUpdate to also check virtual reference connectivity by
detecting virtual references (reference not instanceof Node but exposing
contextElement or similar) and verify that reference.contextElement is a
connected DOM node before calling update(); if contextElement is missing or not
connected, call dispose() and return. Ensure you reference the safeUpdate
function, the reference variable, VirtualSelectionReference/contextElement,
floating.isConnected, update(), and dispose() while implementing this check.
In `@apps/web/src/specs/features/shared/image-failure-tracker.spec.tsx`:
- Around line 115-117: Export the debounce and sampling constants from the
implementation (e.g., add exported DEBOUNCE_MS and REPORT_SAMPLE_RATE in the
image-failure-tracker module) and update the spec to import those constants
instead of using hardcoded values; replace vi.advanceTimersByTime(5000) with
vi.advanceTimersByTime(DEBOUNCE_MS) and any hardcoded 0.2 references in the
sampling test with REPORT_SAMPLE_RATE so the tests stay in sync with the
ImageFailureTracker implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f8daa50-58cf-486e-8907-f569378a0fb5
📒 Files selected for processing (3)
apps/web/src/features/ui/util/safe-auto-update.tsapps/web/src/specs/features/shared/image-failure-tracker.spec.tsxapps/web/src/specs/safe-auto-update.spec.tsx
Summary by CodeRabbit
New Features
Bug Fixes
Tests