feat: add click-to-open functionality for images in markdown content

[nimrodkra]

Summary

• Images in comments and other markdown content can now be clicked to open in a new tab
• Matches the existing behavior for post thumbnail images
• Added cursor pointer styling to indicate images are clickable

Test plan

• Navigate to a post with comments containing images
• Click on an image in a comment
• Verify the image opens in a new tab
• Verify cursor changes to pointer when hovering over images

Preview domain

https://feat-clickable-comment-images.preview.app.daily.dev

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
daily-webapp	Ready	Preview	Jan 15, 2026 7:47am

1 Skipped Deployment

Project	Deployment	Review	Updated (UTC)
storybook	Ignored		Jan 15, 2026 7:47am

[nimrodkra]

@claude please review this pr

[claude]

Claude finished @nimrodkra's task in 1m 17s —— View job

---

Code Review

• Read changed files
• Analyze implementation
• Provide review feedback

---

Summary

The implementation is clean and straightforward, adding click-to-open functionality for images in markdown content. The changes are minimal and focused.

✅ What's Good

1. Type Safety: Proper type guard isImageElement() ensures type-safe handling of the click target
2. Security: Uses noopener,noreferrer with window.open() to prevent security vulnerabilities
3. User Experience: Added cursor-pointer to indicate interactivity
4. Performance: useCallback with empty dependency array prevents unnecessary re-renders
5. Code Quality: Follows existing patterns in the component (similar structure to onHoverHandler)

🔍 Observations & Considerations

1. Event Propagation

The onClick handler doesn't call e.stopPropagation(), which means the click event will bubble up. This is likely intentional, but worth verifying that:

• Clicking images doesn't trigger any unwanted parent handlers
• The hover card doesn't interfere with the click behavior

Location: packages/shared/src/components/Markdown.tsx:88-95

2. Missing preventDefault() Check

The code calls e.preventDefault() inside the image check, but this might be unnecessary since clicking an <img> element typically doesn't have default browser behavior to prevent. This line could potentially be removed for clarity:

if (isImageElement(element) && element.src) {
  e.preventDefault(); // ← Is this needed?
  window.open(element.src, '_blank', 'noopener,noreferrer');
}

Location: packages/shared/src/components/Markdown.tsx:92

3. Accessibility Consideration

Images in markdown now act as clickable elements but don't have:

• Keyboard support (Enter/Space key handling)
• ARIA attributes to indicate they're interactive
• Focus states

For better accessibility, consider:

• Adding role="button" and tabindex="0" to clickable images
• Adding keyboard event handlers
• Or wrapping images in <a> tags during markdown rendering

This isn't blocking for the PR, but worth considering for a follow-up. Fix accessibility →

4. Test Coverage

The PR description includes a manual test plan, but there are no automated tests for this functionality. Consider adding:

• Unit test for isImageElement type guard
• Integration test for image click behavior
• Test to verify window.open is called with correct parameters

🎯 Recommendation

Approve with minor suggestions. The implementation is solid and ready to merge. The observations above are minor improvements that could be addressed in follow-up work if needed.

Testing Notes

Based on the test plan, please verify:

• ✅ Images in comments are clickable
• ✅ Images open in new tab with proper security attributes
• ✅ Cursor changes to pointer on hover
• ⚠️ Additional: Verify no conflicts with mention hover cards
• ⚠️ Additional: Test on mobile devices (touch events)

--- | Branch: feat/clickable-comment-images

[nimrodkra]

@claude do a 2nd round of review for this pr

[claude]

Claude finished @nimrodkra's task in 1m 24s —— View job

---

2nd Round Code Review

• Read changed files and commit history
• Analyze implementation details
• Check for security, accessibility, and edge cases
• Provide comprehensive review feedback

---

Changes Since First Review

Good news - the author addressed one of the concerns from the first review! The code was updated to use e.stopPropagation() instead of e.preventDefault() (commit 629cfc7), which is the correct approach since:

• Images don't have default click behavior that needs preventing
• We do want to stop the event from bubbling to parent handlers
• This prevents potential conflicts with the HoverCard wrapper

2nd Round Findings

✅ Strengths

1. Type Safety: The isImageElement() type guard is well-implemented and provides proper type narrowing
2. Security: Proper use of 'noopener,noreferrer' prevents security vulnerabilities (tabnabbing)
3. Performance: useCallback with empty deps is appropriate here
4. Code Quality: Clean, readable implementation following existing patterns
5. Fix Applied: Correctly changed from preventDefault() to stopPropagation()

🔍 Issues & Considerations

1. Missing Accessibility (Critical) 🚨

Images are now interactive but lack keyboard and screen reader support:

• No keyboard navigation (Tab key won't focus images)
• No keyboard activation (Enter/Space won't work)
• No ARIA attributes to indicate interactivity
• No visual focus indicator

Impact: Users relying on keyboards or screen readers cannot open images.

Location: packages/shared/src/components/Markdown.tsx:88-95

Fix accessibility →%20Adding%20onKeyDown%20handler%20to%20check%20for%20Enter%2FSpace%20keys%202)%20Adding%20tabindex%3D%220%22%20to%20images%20via%20CSS%20or%20runtime%203)%20Adding%20role%3D%22button%22%20to%20images%204)%20Adding%20focus-visible%20styles%20in%20markdown.module.css&repo=dailydotdev/apps)

2. Event Delegation Concern

The onClick handler is attached to the parent <div> and relies on event bubbling. While this works, there's a potential issue:

onClick={onImageClick}  // On the wrapper div

If stopPropagation() is called elsewhere in the event chain (e.g., by mention links or other interactive elements), image clicks could be blocked.

Test: Verify that clicking images inside nested elements (like a <blockquote> containing an image) still works.

Location: packages/shared/src/components/Markdown.tsx:114

3. Missing Null Check on window.open

window.open() can return null in some cases (popup blockers, browser settings). While not critical, it's good practice to handle this:

const newWindow = window.open(element.src, '_blank', 'noopener,noreferrer');
if (!newWindow) {
  // Fallback: could show a toast or copy link to clipboard
}

Location: packages/shared/src/components/Markdown.tsx:93

4. No Test Coverage

There are no tests for the Markdown component at all (Markdown.spec.tsx doesn't exist). This makes it harder to verify:

• Image clicks work correctly
• Type guard functions properly
• No regressions with mention hover cards
• Accessibility features (if added)

Suggestion: Consider adding at least basic tests for this new functionality.

5. Cursor Pointer on Non-Images

The CSS applies cursor-pointer to all images, but the JavaScript only opens images with a valid src. Edge case: if an image fails to load but still exists in the DOM without a src, it will show a pointer cursor but do nothing on click.

Location: packages/shared/src/components/markdown.module.css:87

6. No Loading State or Error Handling

When window.open is called, there's no:

• Visual feedback that the action occurred
• Error handling if the window fails to open
• Loading indicator for slow-loading images

This is minor but could improve UX.

Edge Cases to Test

1. Click on broken/loading images - Images without src attribute
2. Click on SVG images - Does instanceof HTMLImageElement work?
3. Click on background images - CSS background images vs <img> tags
4. Rapid clicking - Can users spam-click and open multiple tabs?
5. Right-click/middle-click - Should these behaviors be affected?
6. Mobile long-press - Does this interfere with long-press to save images?
7. Nested interactive elements - Images inside links or buttons

Recommendations

Priority:

1. 🔴 High: Add keyboard accessibility (critical for a11y compliance)
2. 🟡 Medium: Add basic test coverage
3. 🟢 Low: Handle window.open null return
4. 🟢 Low: Add user feedback (toast notification)

Comparison with First Review

The first review mentioned:

• ✅ Event propagation - Fixed (now using stopPropagation)
• ⚠️ Accessibility - Still missing (should be prioritized)
• ℹ️ Test coverage - Still missing (not blocking but recommended)

Final Verdict

Conditional Approval - The implementation is functional and the propagation fix was good, but the missing accessibility support is a significant gap that should be addressed before merging. Images that act as buttons need keyboard support and proper ARIA attributes to meet accessibility standards.

The code quality is solid, security is handled properly, and the feature works as intended for mouse users. Once accessibility is added, this would be ready to ship.

---

| Branch: feat/clickable-comment-images

[nimrodkra]

@claude are we good to go?

[claude]

Claude finished @nimrodkra's task in 43s —— View job

---

Final PR Assessment

• Review current state of changed files
• Check for accessibility improvements since 2nd review
• Provide final go/no-go recommendation

---

✅ Yes, you're good to go!

What Changed Since the 2nd Review

Great news! The critical accessibility issue from the 2nd review has been fully addressed in commit 1fdbebf. The implementation now includes:

✅ Full Accessibility Support Added

1. Keyboard Support (Markdown.tsx:124-136)

   • onKeyDown handler responds to Enter and Space keys
   • Properly calls preventDefault() and stopPropagation()
   • Opens images in new tab just like mouse clicks

2. ARIA Attributes (Markdown.tsx:80-93)

   • useEffect adds tabindex="0" to make images focusable
   • role="button" indicates images are interactive
   • aria-label="Open image in new tab" provides screen reader context

3. Focus Styles (markdown.module.css:89-91)

   • focus-visible styling with ring effect
   • Proper contrast with cabbage-themed accent color
   • Offset to prevent visual clipping

Code Quality

The implementation is now production-ready:

• ✅ Type-safe with proper type guards
• ✅ Secure with noopener,noreferrer
• ✅ Accessible with keyboard and screen reader support
• ✅ Good UX with cursor pointer and focus styles
• ✅ Clean code following existing patterns
• ✅ Event delegation handled correctly
• ✅ Helpful code comments explaining the approach

What Was Fixed

2nd Review Concern	Status
🔴 Missing keyboard accessibility	✅ Fixed - Full keyboard support added
🔴 Missing ARIA attributes	✅ Fixed - role, tabindex, aria-label added
🔴 Missing focus styles	✅ Fixed - focus-visible styles added
🟡 Test coverage	⚠️ Still missing (optional)
🟢 Null check on window.open	ℹ️ Not added (acceptable trade-off)

Minor Notes

The only items not addressed from the 2nd review are:

• Test coverage - Still no tests, but this is consistent with the team's pragmatic testing philosophy ("write tests that validate functionality, not hit metrics")
• window.open null check - Not added, but this is a reasonable trade-off for simplicity

These are not blockers for merging.

Recommendation

✅ APPROVED - This PR is ready to merge. All critical issues have been resolved, and the feature now meets accessibility standards while maintaining clean, secure code.

---

| | Branch: feat/clickable-comment-images

[idoshamun]

I think it's possible to bake this functionality to our markdown renderer rather than adding it here. Something I'd check before merging