Skip to content

Conversation

MaheshtheDev
Copy link
Member

@MaheshtheDev MaheshtheDev commented Oct 4, 2025

Improved memory detail UI with better document icons and layout and Added dark mode support to the Chrome extension button

What changed?

  • Enhanced the WebsiteCard component with an onOpenDetails prop
  • Improved the MemoryDetail component with:
    • Better layout for document headers and source links
    • Enhanced content scrolling with proper height constraints
    • Improved spacing and padding in various sections
  • Added favicon support to document icons, showing website favicons for webpage documents
  • Fixed document type detection for webpages and URLs
  • Added dark mode support to the Chrome extension button component with appropriate color classes

Screen Recording 2025-10-03 at 11.34.01 PM.mov (uploaded via Graphite)

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label Main to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

cloudflare-workers-and-pages bot commented Oct 4, 2025

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
supermemory-app 583c265 Oct 04 2025, 09:46 PM

@MaheshtheDev MaheshtheDev marked this pull request as ready for review October 4, 2025 06:37
@graphite-app graphite-app bot requested a review from Dhravya October 4, 2025 06:37
Copy link

claude bot commented Oct 4, 2025

Code Review - PR #452: URL Cards with Rich Details

Thank you for this contribution! Overall this PR improves the user experience with dark mode support and enhanced UI elements.

Strengths

  1. Dark Mode Support - Comprehensive dark mode classes added with proper design tokens
  2. Code Quality - Fixed typo primay-foreground to primary-foreground and clean component structure
  3. UX Improvements - Favicon support enhances visual recognition with better layout and scrolling

Issues and Suggestions

1. Security Concern - External Image Loading (apps/web/lib/document-icon.tsx:46-56)

The FaviconIcon component loads favicons from Google service which could have privacy implications. Consider implementing a server-side proxy cache favicons or use a self-hosted service.

2. Missing Error Handling (apps/web/lib/document-icon.tsx:19-26)

The getFaviconUrl function silently returns empty string on error. Consider logging errors using Sentry and add error tracking to monitor malformed URLs.

3. Performance Consideration (apps/web/lib/document-icon.tsx:28-57)

The FaviconIcon uses useState which could cause unnecessary re-renders in list views. Consider memoizing with React.memo() and use useCallback for the onError handler.

4. Accessibility Issue (apps/web/lib/document-icon.tsx:44-56)

The favicon img has generic alt text. Use more descriptive alt text including the domain name.

5. Inconsistent Spacing (apps/web/components/memories-utils/memory-detail.tsx:286)

Line 286 adds both padding AND margin which is unusual. Please verify this is intentional.

6. Type Safety (apps/web/lib/document-icon.tsx:59-64)

Consider using union types for type and source parameters instead of generic strings.

7. Button Capitalization (apps/web/components/memories-utils/memory-detail.tsx:174)

The button text visit source is lowercase. Consider changing to Visit Source for consistency.

Critical Issues

Missing Test Coverage

No tests were found for any modified components. Please add:

  1. Unit tests for getFaviconUrl function
  2. Component tests for FaviconIcon error states
  3. Integration tests for WebsiteCard with onOpenDetails prop
  4. Dark mode rendering tests

Conclusion

This PR delivers valuable UI improvements but needs:

  1. Test coverage (critical)
  2. Security/privacy review for external image loading
  3. Performance optimizations for list rendering

Recommendation: Request changes for test coverage and favicon security review before merging.

Review generated by Claude Code

Copy link

graphite-app bot commented Oct 4, 2025

Merge activity

Copy link

graphite-app bot commented Oct 4, 2025

Movie gif. Actor Chow Yun-fat leans out of a doorway casually chewing while giving a cool, confident thumbs-up of approval. (Added via Giphy)

Improved memory detail UI with better document icons and layout and Added dark mode support to the Chrome extension button

### What changed?

- Enhanced the WebsiteCard component with an `onOpenDetails` prop
- Improved the MemoryDetail component with:
  - Better layout for document headers and source links
  - Enhanced content scrolling with proper height constraints
  - Improved spacing and padding in various sections
- Added favicon support to document icons, showing website favicons for webpage documents
- Fixed document type detection for webpages and URLs
- Added dark mode support to the Chrome extension button component with appropriate color classes

[Screen Recording 2025-10-03 at 11.34.01 PM.mov <span class="graphite__hidden">(uploaded via Graphite)</span> <img class="graphite__hidden" src="https://app.graphite.dev/user-attachments/thumbnails/2f6ceef2-43a2-4e11-bafa-0f3ce614696b.mov" />](https://app.graphite.dev/user-attachments/video/2f6ceef2-43a2-4e11-bafa-0f3ce614696b.mov)
@graphite-app graphite-app bot force-pushed the 10-03-feat_url_cards_as_content_preview_with_rich_details branch from a063f1e to 583c265 Compare October 4, 2025 21:38
@graphite-app graphite-app bot merged commit 583c265 into main Oct 4, 2025
3 of 4 checks passed
Copy link

claude bot commented Oct 4, 2025

Pull Request Review

This PR improves the memory detail UI with better document icons, layout improvements, and dark mode support for the Chrome extension button. Overall, the changes are well-structured and improve the user experience. Here are my findings:

Strengths

  1. Dark Mode Support: Comprehensive dark mode implementation using semantic color tokens (foreground, muted-foreground, background, etc.)
  2. Improved UX: Better layout hierarchy with the "visit source" button and favicon support enhances usability
  3. Graceful Error Handling: FaviconIcon component properly handles image load failures with fallback to Globe icon
  4. Type Safety: Proper TypeScript usage with clear prop interfaces

🐛 Potential Issues

  1. React Hook in Non-Component Function (document-icon.tsx:17)

    • useState is used inside FaviconIcon, but this component is returned from getDocumentIcon function
    • Each call to getDocumentIcon creates a new component instance, which could cause React to lose state between renders
    • Recommendation: Extract FaviconIcon as a separate exported component or memoize it
  2. Inconsistent Inline Styles (document-icon.tsx:49-52)

    • Mixing className with inline width/height styles using em units
    • The className may contain width/height that conflicts with inline styles
    • Recommendation: Use consistent styling approach, preferably CSS classes
  3. Missing Alt Text Context (document-icon.tsx:47)

    • Generic "Website favicon" alt text doesn't help users understand which site it represents
    • Recommendation: alt={new URL(url).hostname + " favicon"} or pass domain as alt text
  4. Potential XSS Risk (memory-detail.tsx:200)

    • HTMLContentRenderer renders document.content which may contain user-generated HTML
    • Ensure HTMLContentRenderer properly sanitizes content (couldn't verify implementation)
    • Recommendation: Verify sanitization is in place or use DOMPurify
  5. Accessibility Issues

    • Button at chrome-extension-button.tsx:175 uses border-primary which may have insufficient contrast in some themes
    • Missing aria-label on favicon images would be helpful for screen readers

Performance Considerations

  1. Favicon Loading - External Google favicon API calls could be slow

    • Consider caching favicon URLs or using a CDN
    • Add loading state to prevent layout shift
  2. Memory Component - max-h-[80vh] with overflow on memory-detail.tsx:235 could cause performance issues with many memories

    • Consider virtualization for long lists (e.g., react-window)

🔒 Security Concerns

  1. External Image Loading - Favicon URLs from untrusted domains could leak information

    • Consider using a proxy service or CSP restrictions
  2. window.open Calls - Multiple instances without additional security attributes

    • Already using noopener,noreferrer which is good ✅

📝 Code Quality

  1. Typo Fixes - Good catch on text-primay-foregroundtext-primary-foreground in header.tsx

  2. Inconsistent Padding (memory-detail.tsx:286)

    • <div className="p-3 md:p-4 m-4"> has both padding and margin which seems unusual
    • Verify this doesn't create excessive spacing
  3. Magic Numbers - Several hardcoded values like w-96, max-h-[80vh] could be constants

🧪 Test Coverage

  • No test files found for these components
  • Recommendation: Add tests for:
    • FaviconIcon error handling
    • WebsiteCard click behavior with different prop combinations
    • Dark mode rendering

📋 Suggested Improvements

  1. Extract FaviconIcon as a proper component to fix React hooks issue
  2. Add proper image loading states and error boundaries
  3. Consider adding unit tests for the new functionality
  4. Document the HTMLContentRenderer security measures
  5. Use CSS custom properties for repeated spacing values

Summary

The PR improves UX significantly with favicon support and better dark mode. The main concern is the React hooks usage pattern in getDocumentIcon. I recommend extracting FaviconIcon as a standalone component before merging.

Recommendation: Request changes for the React hooks issue, otherwise looks good to merge after addressing the hook concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants