Skip to content

Avoid effect-driven renders in image preview dialog#2772

Draft
cursor[bot] wants to merge 5 commits into
mainfrom
cursor/react-component-health-fb9a
Draft

Avoid effect-driven renders in image preview dialog#2772
cursor[bot] wants to merge 5 commits into
mainfrom
cursor/react-component-health-fb9a

Conversation

@cursor
Copy link
Copy Markdown
Contributor

@cursor cursor Bot commented May 20, 2026

What Changed

  • Removed the ExpandedImageDialog effect that mirrored the incoming preview prop into local state.
  • Added a stable preview identity key so a newly selected parent preview remounts the dialog cleanly.
  • Kept in-dialog image navigation local by storing only a navigation offset and deriving the active index during render.
  • Moved the keyboard listener into a named hook that uses useEffectEvent.

Why

React Doctor flagged the dialog for prop-mirrored state and derived-state synchronization. The previous effect added an extra commit after the parent selected a different image preview. The new flow derives preview selection during render, avoids the sync effect, and keeps keyboard subscription logic isolated in a hook.

React Doctor verification:

  • Before: src/components/chat/ExpandedImageDialog.tsx reported useState "preview" is mirrored from prop "initialPreview" via this effect plus related derived-state warnings.
  • After: no ExpandedImageDialog.tsx diagnostics in the final React Doctor JSON scan.

UI Changes

React Scan before/after recordings are included in this PR:

  • Before: assets/react-scan/expanded-image-dialog-before.webm
  • After: assets/react-scan/expanded-image-dialog-after.webm

Both recordings were captured with React Scan enabled against the same Vite harness and interaction sequence.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Validation:

  • bun fmt
  • bun lint (passes with existing warnings)
  • bun typecheck
Open in Web View Automation 

Note

Avoid effect-driven renders in ExpandedImageDialog by replacing synced state with a navigation offset

  • Removes the local preview state and its syncing effect from ExpandedImageDialog, replacing them with a navigationOffset value computed relative to the incoming preview prop.
  • Extracts keyboard handling into a useExpandedImageDialogKeyboardShortcuts hook using useEffectEvent to avoid stale closures on Escape and Arrow key events.
  • Adds getExpandedImagePreviewIdentityKey in ExpandedImagePreview.tsx and applies it as a React key on ExpandedImageDialog in ChatView, so the dialog remounts when the selected image identity changes instead of syncing via effects.
📊 Macroscope summarized b89ba06. 3 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

cursoragent and others added 5 commits May 20, 2026 16:08
Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels May 20, 2026
Comment on lines +50 to +51
const activeIndex =
imageCount > 0 ? (preview.index + navigationOffset + imageCount) % imageCount : preview.index;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical chat/ExpandedImageDialog.tsx:50

When navigationOffset is negative and larger than imageCount, the modulo calculation (preview.index + navigationOffset + imageCount) % imageCount produces a negative activeIndex. For example, with imageCount = 3, preview.index = 0, and navigationOffset = -5, the result is -2, causing preview.images[-2] to be undefined and the component to return null unexpectedly.

-  const activeIndex =
-    imageCount > 0 ? (preview.index + navigationOffset + imageCount) % imageCount : preview.index;
+  const activeIndex =
+    imageCount > 0 ? (((preview.index + navigationOffset) % imageCount) + imageCount) % imageCount : preview.index;
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/chat/ExpandedImageDialog.tsx around lines 50-51:

When `navigationOffset` is negative and larger than `imageCount`, the modulo calculation `(preview.index + navigationOffset + imageCount) % imageCount` produces a negative `activeIndex`. For example, with `imageCount = 3`, `preview.index = 0`, and `navigationOffset = -5`, the result is `-2`, causing `preview.images[-2]` to be `undefined` and the component to return `null` unexpectedly.

Evidence trail:
apps/web/src/components/chat/ExpandedImageDialog.tsx lines 48-70 (REVIEWED_COMMIT): `navigationOffset` state starts at 0 and is incremented/decremented without clamping (lines 53-61). The activeIndex calculation at line 50-51 adds only one `imageCount` before applying JS `%` remainder. JavaScript `%` preserves sign of dividend: `(-2) % 3 === -2`. With sufficient left-arrow presses, activeIndex becomes negative, `preview.images[negativeIndex]` is undefined, and line 70 returns null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). 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.

1 participant