Skip to content

Conversation

@felixfeng33
Copy link
Collaborator

Checklist

  • yarn typecheck
  • yarn lint:fix
  • yarn test
  • yarn brl
  • yarn changeset
  • ui changelog

@codesandbox
Copy link

codesandbox bot commented Jul 23, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@vercel
Copy link

vercel bot commented Jul 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 23, 2025 2:27am

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 23, 2025
@claude
Copy link
Contributor

claude bot commented Jul 23, 2025

Claude finished @felixfeng33's task —— View job


Code Review for: Add addOnContextMenu

Summary

This PR introduces a new addOnContextMenu function to the BlockSelectionPlugin API, providing a cleaner and more consistent way to handle context menu behavior for block selection. The implementation consolidates previously inline logic into a dedicated function.

Code Quality ✅

The code follows the project's patterns and conventions well:

  • Proper TypeScript typing with clear parameter interfaces
  • Consistent use of plugin API patterns
  • Clean separation of concerns

Specific Feedback

1. packages/selection/src/react/hooks/useBlockSelectable.ts

Strengths:

  • Well-documented function with clear JSDoc comment explaining the purpose
  • Good parameter structure with sensible defaults
  • Proper use of TypeScript for type safety

Minor improvements:

  • Line 48-52: Consider extracting the isSelected check logic for better readability:
const isBlockSelected = editor.getOption(
  BlockSelectionPlugin,
  'isSelected',  
  id
);

2. packages/selection/src/react/BlockSelectionPlugin.tsx

Good: The API exposure follows the established pattern using bindFirst for proper context binding.

3. apps/www/src/registry/ui/block-draggable.tsx

Good: Simplified implementation using the new API method. The refactoring from inline logic to API call improves maintainability.

Missing Elements 🚨

1. Unit Tests

No tests were added for the new addOnContextMenu function. Given that the selection package has existing test infrastructure, this functionality should be tested. Consider adding tests for:

  • Basic context menu selection behavior
  • Shift-click multi-selection
  • The disabledWhenFocused option
  • Void element handling
  • The data-plate-open-context-menu attribute behavior

2. Documentation

The new API method should be documented. Consider:

  • Adding the method to the BlockSelectionPlugin API documentation
  • Including usage examples
  • Documenting the disabledWhenFocused parameter and when to use it

Security & Error Handling ✅

The implementation handles edge cases appropriately:

  • Null checks for enableContextMenu option
  • Proper handling of undefined IDs
  • Safe access to DOM elements via optional chaining

Suggestions for Improvement

  1. Consider adding a return value to indicate whether the context menu was handled:
export const addOnContextMenu = (...): boolean => {
  // ... existing logic ...
  return true; // when handled
};
  1. Extract magic strings like 'data-plate-open-context-menu' to constants for maintainability:
const CONTEXT_MENU_ATTR = 'data-plate-open-context-menu';
  1. Type safety for dataset access: Consider adding type guards or interfaces for the dataset properties.

Conclusion

The implementation is solid and follows project conventions well. However, the PR would benefit from:

  1. Adding unit tests for the new functionality (Required for package changes per guidelines)
  2. Adding documentation for the new API method
  3. Consider the minor improvements suggested above

Once these items are addressed, this will be a great addition to the BlockSelectionPlugin API.

@claude
Copy link
Contributor

claude bot commented Jul 23, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@felixfeng33
Copy link
Collaborator Author

@claude /changset changelog

@claude
Copy link
Contributor

claude bot commented Jul 23, 2025

Claude finished @felixfeng33's task —— View job


Changeset and Changelog Review

Tasks

  • Check for changeset file in .changeset directory
  • Verify docs/components/changelog.mdx has been updated
  • Review changeset and changelog entries for accuracy
  • Create missing changeset file
  • Update changelog.mdx for registry change

Issues Found

Missing Changeset: No changeset file found in .changeset/ directory

  • The selection package (packages/selection) was modified and requires a changeset

Missing Changelog Entry: docs/components/changelog.mdx has not been updated

  • The block-draggable component in registry was modified and requires a changelog entry

Required Actions

1. Create a changeset file

Run the following command and select the @platejs/selection package with a minor version bump:

yarn changeset

Suggested changeset content:

---
"@platejs/selection": minor
---

Add `addOnContextMenu` API method to BlockSelectionPlugin for cleaner context menu handling

2. Update docs/components/changelog.mdx

Add an entry after July 18 #24.6 (around line 16):

### July 23 #24.7
- `block-draggable`: Updated to use new `addOnContextMenu` API from BlockSelectionPlugin for cleaner context menu handling

These changes are required because:

  • Package changes require changesets for proper versioning
  • Registry UI changes require changelog entries per the contribution guidelines

@changeset-bot
Copy link

changeset-bot bot commented Jul 23, 2025

🦋 Changeset detected

Latest commit: 4d53c04

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@platejs/selection Patch
@platejs/ai Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 23, 2025
@felixfeng33 felixfeng33 merged commit 0d8c874 into main Jul 24, 2025
9 checks passed
@felixfeng33 felixfeng33 deleted the fix/ai-insert branch July 24, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer plugin:block-selection size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants