Skip to content

Conversation

@nexxeln
Copy link
Member

@nexxeln nexxeln commented Dec 4, 2025

No description provided.

@nexxeln nexxeln marked this pull request as ready for review December 4, 2025 16:01
Copy link
Member Author

nexxeln commented Dec 4, 2025


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.

@graphite-app graphite-app bot requested a review from Dhravya December 4, 2025 16:01
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 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 failed
View logs
supermemory-app 7a2f2cb Dec 04 2025, 06:56 PM

@nexxeln nexxeln requested review from MaheshtheDev and removed request for Dhravya December 4, 2025 16:01
Copy link
Member Author

nexxeln commented Dec 4, 2025

@MaheshtheDev can you check now

@claude
Copy link

claude bot commented Dec 4, 2025

Code Review - PR #604: Use latest graph and remove old graph

Overview

This PR migrates from the internal @repo/ui/memory-graph package to a published @supermemory/memory-graph workspace package. The changes involve updating import statements across multiple files and adding the new package as a dependency.

✅ Positive Aspects

  1. Clean Import Migration: All imports have been consistently updated from @repo/ui/memory-graph/* to @supermemory/memory-graph
  2. Proper Package Structure: The new @supermemory/memory-graph package has a well-defined structure with proper exports, types, and build configuration
  3. Backward Compatibility: Changes are non-breaking - only import paths changed, not the API surface
  4. Complete Migration: All references to the old import paths have been updated (verified via grep)

🔍 Code Quality

Strong Points:

  • Consistent import path updates across all affected files
  • The new package has proper TypeScript types and dual CJS/ESM exports
  • Good separation of concerns by extracting colors.ts to @repo/ui/colors

⚠️ Considerations & Recommendations

1. Build & Type Checking

  • ✅ Recommendation: Run bun run check-types and bun run build to ensure no TypeScript errors
  • The new package is added as a workspace dependency (workspace:*), which should work correctly

2. Missing Information

  • Old Code Removal: The PR title mentions "remove old graph" but the diff doesn't show deletion of the old packages/ui/memory-graph/ directory. Consider verifying this was removed to avoid confusion.
  • Testing: No test updates or additions are shown. If there were tests for the memory graph functionality, they should be updated.

3. Package Version

  • The @supermemory/memory-graph package is at version 0.1.2
  • The @supermemory/tools package version was bumped from 1.3.11 to 1.3.12 (appears unrelated to this PR)

4. Dependency Management

The bun.lock shows significant changes including:

  • Removal of Raycast extension dependencies (appears to be a separate cleanup)
  • Addition of the memory-graph package to the web app dependencies
  • Some dependency version updates

🐛 Potential Issues

None identified - The changes appear safe and well-structured.

🚀 Performance Considerations

  • Bundle Size: The new package is now a separate module, which could improve code splitting
  • Build Process: The memory-graph package has its own build step, which is properly configured in the monorepo
  • No Performance Concerns: The changes are purely structural refactoring

🔒 Security Concerns

No security issues identified - This is a refactoring PR with no changes to functionality or data handling.

📋 Recommendations Before Merge

  1. Verify the old packages/ui/memory-graph/ directory was deleted (not shown in diff)
  2. ⚠️ Run CI checks: Ensure bun run check-types and bun run build pass
  3. ⚠️ Manual testing: Test the memory graph visualization in the web app to ensure it renders correctly
  4. ℹ️ Consider adding a migration note to the commit message or PR description explaining why this was extracted

Summary

This is a well-executed refactoring PR that extracts the memory graph component into a standalone package. The import migrations are complete and consistent. The main action item is ensuring the old code is removed and that builds pass successfully.

Approval Status: ✅ Approved with minor verification needed


Generated via Claude Code review

@graphite-app
Copy link

graphite-app bot commented Dec 4, 2025

Merge activity

@graphite-app
Copy link

graphite-app bot commented Dec 4, 2025

Waynes World Thumbs Up GIF by Hollywood Suite (Added via Giphy)

@graphite-app graphite-app bot force-pushed the use-memory-graph-package branch from 5cff73d to 7a2f2cb Compare December 4, 2025 18:54
@graphite-app graphite-app bot merged commit 7a2f2cb into main Dec 4, 2025
3 of 4 checks passed
@claude
Copy link

claude bot commented Dec 4, 2025

Pull Request Review: Use Latest Graph and Remove Old Graph

Summary

This PR migrates from the internal @repo/ui/memory-graph component to the new standalone @supermemory/memory-graph package. This is a significant refactoring that removes ~4,500 lines of code and consolidates the graph visualization into a dedicated, publishable package.

Code Quality ✅

Strengths

  1. Excellent consolidation: Moving the memory graph to a standalone package improves modularity and reusability
  2. Consistent refactoring: All imports have been systematically updated across 9 files
  3. Clean separation of concerns: The colors constant is now properly exported from @repo/ui/colors instead of being buried in memory-graph/constants
  4. Type safety maintained: The DocumentWithMemories type export has been properly migrated

Import Changes

All changes follow a consistent pattern:

  • @repo/ui/memory-graph@supermemory/memory-graph
  • @repo/ui/memory-graph/constants@repo/ui/colors
  • @ui/memory-graph/types@supermemory/memory-graph

Files updated:

  • apps/web/components/graph-dialog.tsx
  • apps/web/components/masonry-memory-list.tsx
  • apps/web/components/memory-list-view.tsx
  • apps/web/components/memories-utils/index.tsx
  • apps/web/lib/document-icon.tsx
  • apps/web/components/content-cards/*.tsx (4 files)

Dependency Management ✅

Package Changes

  1. New dependency: @supermemory/memory-graph added as workspace dependency in apps/web/package.json
  2. Package exists: Verified that packages/memory-graph/package.json exists with proper configuration
  3. Workspace setup: The package is properly configured in the turbo monorepo structure

Potential Issues & Recommendations ⚠️

1. Bun Lock File Changes

The bun.lock file shows significant changes including:

  • Added configVersion: 0 field
  • Removed apps/raycast-extension workspace
  • Dependency version updates (ajv, ansi-escapes, clean-stack, escape-string-regexp, etc.)

Recommendation: These seem to be auto-generated changes from running bun install. Ensure these don't introduce breaking changes by:

# Test the build
bun run build
# Test type checking
bun run check-types

2. Missing CSS Import Verification

The new package exports styles via ./styles.css according to its package.json:

"./styles.css": "./dist/memory-graph.css"

Question: Are the CSS imports properly handled in the consuming components? The PR diff doesn't show any changes to CSS imports. Please verify that:

  • The graph styles are loading correctly
  • No duplicate CSS is being imported
  • The build process includes the CSS from the new package

3. Type Exports

The PR changes the import:

// Before
import type { DocumentWithMemories } from "@ui/memory-graph/types"
// After  
import type { DocumentWithMemories } from "@supermemory/memory-graph"

Recommendation: Verify that all types from the old package are re-exported from the new package's main entry point.

4. Tools Package Version Bump

The diff shows packages/tools version changed from 1.3.11 to 1.3.12. This seems unrelated to the graph migration.

Question: Is this intentional or a side effect? If intentional, what changes were made to the tools package?

Testing Checklist 📋

Before merging, please verify:

  • Build succeeds for all packages: bun run build
  • Type checking passes: bun run check-types
  • Graph visualization renders correctly in the web app
  • All graph interactions work (hover, click, drag, etc.)
  • Colors are rendering correctly from the new @repo/ui/colors export
  • Memory list and memory cards display correctly
  • Document icons render with correct colors
  • No console errors related to missing exports
  • CSS styles are applied correctly (no styling regressions)

Security Considerations 🔒

No security concerns identified. The changes are purely internal refactoring with no:

  • External API changes
  • Authentication/authorization modifications
  • Data handling changes
  • External dependency additions (workspace package only)

Performance Considerations ⚡

Potential improvement: Consolidating into a standalone package may improve bundle splitting and tree-shaking, but this should be verified with:

  • Bundle size analysis before/after
  • Runtime performance testing of graph rendering

Best Practices Alignment ✅

According to the repository's CLAUDE.md:

  • ✅ Uses Biome for linting (should run bun run format-lint before merge)
  • ✅ TypeScript strict mode maintained
  • ✅ Turbo monorepo structure properly utilized
  • ✅ Package naming follows @supermemory/* convention for publishable packages

Conclusion

This is a well-executed refactoring that improves code organization and package structure. The changes are systematic and consistent. The main concerns are:

  1. Verifying CSS imports work correctly
  2. Ensuring all type exports are available
  3. Testing that no functionality is broken
  4. Understanding the unrelated lockfile and tools package changes

Overall Assessment: ✅ Approve with minor verification needed

Once the testing checklist is confirmed and any CSS import questions are resolved, this PR is ready to merge.

Great work on the consolidation! 🎉

MaheshtheDev added a commit that referenced this pull request Dec 7, 2025
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.

3 participants