Skip to content

set x-slate-fragment in plateStatic #4431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

set x-slate-fragment in plateStatic #4431

wants to merge 11 commits into from

Conversation

felixfeng33
Copy link
Collaborator

Checklist

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

Copy link

codesandbox bot commented Jun 30, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

changeset-bot bot commented Jun 30, 2025

🦋 Changeset detected

Latest commit: 13dbdcd

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

This PR includes changesets to release 3 packages
Name Type
@platejs/core Patch
platejs Patch
@platejs/utils 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

Copy link

vercel bot commented Jun 30, 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 1, 2025 11:00am

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

claude bot commented Jun 30, 2025

Claude finished @felixfeng33's task —— View job


Code Review for "set x-slate-fragment in plateStatic"

Review Checklist

  • Read and analyze all changed files
  • Review code quality and adherence to project standards
  • Check error handling implementation
  • Verify test coverage for new functionality
  • Assess if changeset is required
  • Check documentation requirements
  • Provide comprehensive feedback

Summary

This PR adds copy functionality to PlateStatic by setting the x-slate-fragment data format when content is copied. This enables proper Slate-compatible copy operations in the static renderer, aligning its behavior with the interactive editor.

Code Review

✅ Strengths

  1. Clean implementation: The code follows existing patterns and is well-structured
  2. Proper data encoding: Uses the same encoding method (btoa(encodeURIComponent())) as other parts of the codebase
  3. Minimal changes: Only adds necessary functionality without breaking existing behavior
  4. Browser safety: Includes proper typeof window === 'undefined' check for SSR compatibility

🔍 Issues Found

1. Critical: Missing data-slate-id Implementation 🚨

The copy functionality relies on elements having data-slate-id attributes, but these attributes are never set in PlateStatic:

// In getSelectedDomBlocks.ts
const domBlocks = fragment.querySelectorAll(
  '[data-slate-node="element"][data-slate-id]'
);

This means the copy functionality will not work as the query selector will find no elements. You need to:

  1. Add data-slate-id attributes to elements in BaseElementStatic
  2. Ensure elements have an id property to use

Example fix in PlateStatic.tsx:

// In BaseElementStatic
const attributes: SlateRenderElementProps['attributes'] = {
  'data-slate-node': 'element',
  'data-slate-id': element.id, // Add this line
  ref: null,
};

2. Missing Changeset ⚠️

The changeset bot correctly identified that this PR needs a changeset. This is a new feature for the @platejs/core package:

# Run this command or use the link provided by the bot:
yarn changeset

Suggested changeset:

"@platejs/core": minor
---

feat: Add x-slate-fragment support to PlateStatic for copy operations

3. Missing Tests 🚨

The new copy functionality lacks test coverage. Consider adding tests to verify:

  • Elements have data-slate-id attributes
  • Copy event sets the correct clipboard data
  • Fragment encoding works correctly

4. Potential Runtime Error 🐛

In setFragmentDataStatic.ts line 24-26:

const blockId = node.dataset.slateId;
const block = editor.api.node({ id: blockId, at: [] })!;
fragment.push(block[0]);

Issues:

  • Non-null assertion could cause runtime errors
  • If blockId is undefined (which it currently will be), this will fail

Consider safer implementation:

const blockId = node.dataset.slateId;
if (!blockId) continue;

const block = editor.api.node({ id: blockId, at: [] });
if (block) {
  fragment.push(block[0]);
}

5. Documentation Update Needed 📚

The static rendering guide should mention the new copy functionality.

💡 Additional Suggestions

  1. Consider element path fallback: If elements don't have IDs, you might need a different approach to identify them (e.g., using data attributes with paths)
  2. Add integration test: Test the full copy flow with actual DOM selection
  3. File extension: getPlainText.tsx should be .ts (no JSX content)

Conclusion

While the implementation is clean, this feature currently won't work because data-slate-id attributes are never set on elements. This must be fixed before merging.

Required actions:

  1. Fix data-slate-id implementation (critical)
  2. Add changeset (required)
  3. Add tests (strongly recommended)
  4. Improve error handling (recommended)

Copy link
Contributor

claude bot commented Jun 30, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@dosubot dosubot bot added the core label Jun 30, 2025
@zbeyens
Copy link
Member

zbeyens commented Jun 30, 2025

/changeset

Copy link
Contributor

claude bot commented Jun 30, 2025

Claude finished @zbeyens's task —— View job


Todo List:

  • Read PR changes to understand the feature
  • Create changeset file for @platejs/core
  • Write proper changeset description
  • Commit the changeset file

Summary

Created a changeset file for the new copy functionality in PlateStatic:

  • File: .changeset/platejs-core-minor.md
  • Change type: minor (new feature)
  • Package: @platejs/core

The changeset documents the addition of copy functionality to the PlateStatic component, which enables proper serialization of selected content with the x-slate-fragment data format for seamless paste operations.

@zbeyens
Copy link
Member

zbeyens commented Jun 30, 2025

/test

Copy link
Contributor

claude bot commented Jun 30, 2025

Claude finished @zbeyens's task —— View job


Todo List:

  • Read testing guidelines from .cursor/rules/testing.mdc
  • Analyze PR changes to understand what needs testing
  • Create test file for setFragmentDataStatic
  • Create test file for getSelectedDomBlocks
  • Create test file for getPlainText
  • Add integration test for PlateStatic copy functionality
  • Run build and tests for affected packages
  • Commit test files to the PR branch

Summary

Successfully created comprehensive test coverage for the new copy functionality in PlateStatic. The tests include:

  1. Unit tests for each new utility function:

    • setFragmentDataStatic.spec.ts - Tests fragment data encoding and clipboard operations
    • getSelectedDomBlocks.spec.ts - Tests DOM selection and element querying
    • getPlainText.spec.ts - Tests text extraction from DOM nodes
  2. Integration tests in PlateStatic.copy.spec.tsx that demonstrate:

    • How the copy functionality should work when elements have data-slate-id attributes
    • The current bug: Elements in PlateStatic don't have data-slate-id attributes, causing copy to fail
    • Edge cases like empty selection and inline elements

Key Finding

The tests confirm the issue identified in the previous review: data-slate-id attributes are never set on elements in PlateStatic, which prevents the copy functionality from working. This needs to be fixed by adding data-slate-id to the attributes in BaseElementStatic.
|

- Add tests for setFragmentDataStatic
- Add tests for getSelectedDomBlocks
- Add tests for getPlainText
- Add integration tests for PlateStatic copy functionality
- Tests demonstrate that data-slate-id attributes are missing

Co-authored-by: Ziad Beyens <[email protected]>
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 30, 2025
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Jul 1, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jul 1, 2025
@felixfeng33
Copy link
Collaborator Author

felixfeng33 commented Jul 1, 2025

/changeset

@felixfeng33
Copy link
Collaborator Author

/test

Copy link
Contributor

claude bot commented Jul 1, 2025

Claude finished @felixfeng33's task —— View job


Todo List:

  • Read testing guidelines from .cursor/rules/testing.mdc
  • Analyze PR changes to understand what needs testing
  • Create tests for createStaticEditor
  • Create tests for CopyPlugin
  • Create tests for new utility functions (getSelectedDomBlocks, getPlainText, etc.)
  • Create tests for PlateView component
  • Create tests for usePlateViewEditor hook
  • Build affected packages using turbo
  • Run tests to ensure they pass
  • Fix failing tests
  • Commit test files to the PR branch

Summary

Successfully created comprehensive test coverage for the new PlateStatic copy functionality. The tests include:

  1. Unit tests for each new utility function:

    • createStaticEditor.spec.tsx - Tests editor creation with copy plugin configuration
    • CopyPlugin.spec.ts - Tests fragment data encoding and clipboard operations
    • getSelectedDomBlocks.spec.ts - Tests DOM selection and element querying
    • getSelectedDomNode.spec.ts - Tests DOM node extraction from selection
    • isSelectOutside.spec.ts - Tests editor boundary detection
    • getPlainText.spec.ts - Tests text extraction from DOM nodes
  2. Component tests:

    • PlateView.spec.tsx - Tests the copy event handling in the view component
    • usePlateViewEditor.spec.tsx - Tests the hook for creating memoized static editors

Known Issues

Some existing tests in the project are failing due to missing dependencies (@platejs/callout, @platejs/resizable), but the new tests for the copy functionality are working correctly. The test suite covers all the new code paths and edge cases.

variant,
...props
}: PlateViewProps & VariantProps<typeof editorVariants>) {
return <PlateView {...props} />;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing variants?

}).overrideEditor(({ editor }) => ({
transforms: {
setFragmentData(fragment) {
return setFragmentDataStatic(editor, fragment);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to export this, you can move the logic here

}
};

export const CopyPlugin = createTSlatePlugin({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can override dom plugin like ReactPlugin:

export const ViewPlugin = DOMPlugin.overrideEditor...


export type GetStaticPluginsOptions = {
/** Enable copy plugin. */
copyPlugin?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options not needed, we'll always enable that plugin

* @param deps - Additional dependencies for the useMemo hook (default: [])
* @see {@link createStaticEditor} for detailed information on static editor creation and configuration.
*/
export function usePlateViewEditor<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude do you prefer usePlateViewEditor or useViewEditor? to match createStaticEditor, createViewEditor, usePlateEditor, createSlateEditor, createPlateEditor

Copy link
Contributor

@claude claude bot Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @zbeyens's task —— View job


Todo List:

  • Analyze the naming patterns in the codebase
  • Provide recommendation for hook naming

Naming Recommendation

I prefer usePlateViewEditor for the following reasons:

  1. Consistency with existing patterns: The codebase already has usePlateEditor as the primary hook for creating React editors. Using usePlateViewEditor maintains this naming convention.

  2. Clear namespace distinction: The "Plate" prefix distinguishes these React-enhanced hooks from base Slate functionality. This is important because:

    • usePlateEditor creates a Plate editor (React-enhanced)
    • usePlateViewEditor creates a Plate view editor (React-enhanced, static)
    • Without the prefix, useViewEditor could be confused with a base Slate hook
  3. Parallel structure: The existing create functions follow this pattern:

    • createPlateEditorusePlateEditor
    • createStaticEditorusePlateViewEditor

    Note: There's no createViewEditor in the codebase - the static editor is created with createStaticEditor.

  4. Semantic clarity: The name clearly indicates this is a Plate-specific hook for view/static editors, not a generic editor hook.

The pattern useEditor* (without "Plate") is reserved for hooks that interact with existing editors (like useEditorSelector, useEditorPlugin), not for creating new editor instances.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dosu do you prefer usePlateViewEditor or useViewEditor? to match createStaticEditor, createViewEditor, usePlateEditor, createSlateEditor, createPlateEditor

- Add tests for createStaticEditor with copy plugin configuration
- Add tests for CopyPlugin fragment data serialization
- Add tests for DOM selection utilities (getSelectedDomBlocks, getSelectedDomNode)
- Add tests for isSelectOutside editor detection
- Add tests for getPlainText DOM text extraction
- Add tests for PlateView component copy event handling
- Add tests for usePlateViewEditor hook memoization

Co-authored-by: Felix Feng <[email protected]>
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants