fix(desktop): eliminate image-hover layout jump in messages

[thomaspblock]

Summary

• Hovering an inline image in a message caused the body text to shift down ~1-2px relative to the username header after a short pause, leaving a visibly larger gap.
• Root cause: the inline image was wrapped in a <DialogPrimitive.Trigger asChild> cloned onto a wrapper <div> around the <img>, itself wrapped in an ImageContextMenu div. The nested wrappers + Radix attribute cloning caused a layout reflow in the surrounding inline flow on :hover state changes.
• Replaced the wrappers with a new ImageBlock component that renders a bare <img> and drives the lightbox via controlled open state instead of Trigger asChild. Right-click-to-download and click-to-zoom still work.
• Added a header comment to ImageBlock explaining why the bare <img> matters, so this isn't accidentally reintroduced.

Diagnostic journey

The cause wasn't obvious from reading the code — the wrapper had no hover:border / hover:ring styles, and the message action bar (the other hover-reactive element) is absolutely positioned. We narrowed it down by:

1. Removing transition-opacity hover:opacity-90 on the image wrapper → bug persisted.
2. Forcing the action bar always-visible (no fade-in transition) → bug persisted.
3. Stripping the entire wrapper stack (Dialog, ContextMenu, inner div) and rendering a bare <img> → bug fixed on hover-enter, partial fix on hover-leave; then fully fixed once tested in isolation.

Step 3 isolated the cause to the wrapper stack (specifically Trigger asChild + nested divs around the img). The fix keeps the <img> bare and moves the dialog/menu wiring into React state.

Test plan

• Hover an inline image in a channel message → no vertical shift between the username header and body text.
• Click an inline image → full-size lightbox opens. Esc closes; click outside the image closes.
• Right-click an inline image → "Download image" context menu appears at the cursor; click downloads the image.
• Right-click an image while the lightbox is open → same menu appears.
• Hover other parts of the message (avatar, username, body text) → no regression, action bar fades in as before.
• Run just ci locally.

Made with Cursor

[wesbillman]

Fix is sound. The diagnosis (nested wrapper divs + Radix Trigger asChild attribute cloning repainting inline flow on :hover) is well-reasoned, the controlled-open-state rewrite is the right call, and the header comment guards against reintroduction. Single-image hover/zoom/right-click/download parity is preserved (incl. right-click on the open lightbox).

One regression to address before merge (multi-image gallery), plus a small asymmetry to confirm — both inline.

Note: I couldn't run pnpm test in a fresh review checkout (no node_modules), but the existing markdown unit tests cover classifyChildren/isImageOnlyParagraph (unchanged here) and wouldn't catch a CSS/markup grid regression anyway — the gallery case needs a manual multi-image check regardless.

[wesbillman]

🔴 Multi-image gallery layout regression. This bakes mt-1 max-w-sm directly onto the bare <img>. That breaks the 2-up gallery grid path in the p handler (line 514):

grid ... grid-cols-2 ... [&_div]:mt-0 [&_div]:max-w-none

That [&_div]:mt-0 [&_div]:max-w-none selector exists specifically to strip the per-image wrapper div's mt-3/max-w-sm inside the grid (added in #316, when each tile was a <div className="mt-3 ... max-w-sm">). This PR removed that wrapper <div>, so the selector now targets div but the styles live on img — it no longer neutralizes anything. In a 2-up gallery each image keeps its max-w-sm cap (won't fill the cell) and gains an unwanted mt-1. The single-inline-image test plan doesn't exercise this path.

Fix options:

• change the grid selector (line 514) to also hit images: [&_img]:mt-0 [&_img]:max-w-none, or
• have ImageBlock accept a layout variant that drops mt-1 max-w-sm in the grid context.

[wesbillman]

From my bots, btw ^

[wesbillman]

🤖 Addressed in 6bd5a44f: the gallery selector now resets the new [data-block-media] wrapper and inner img styles (mt, max-width, and width), so multi-image grid cells still fill correctly after removing the old wrapper div.

[wesbillman]

🟡 Nit — className="block" asymmetry. The image branch adds <span data-block-media="" className="block">, but the sibling video branch above keeps <span data-block-media=""> with no block. Probably intentional (the bare <img> needs block context here), but worth a one-line confirm it's deliberate and not also needed for video.

[wesbillman]

🤖 Confirmed deliberate. The image branch needs className="block" on the media marker because ImageBlock intentionally renders a bare img with no wrapper. The video branch can keep its existing span because VideoPlayer owns its block layout.