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 Oct 17, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2025

🦋 Changeset detected

Latest commit: 7c33bfe

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

@vercel
Copy link

vercel bot commented Oct 17, 2025

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

Project Deployment Preview Comments Updated (UTC)
plate Ready Ready Preview Comment Oct 17, 2025 2:51am

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. core labels Oct 17, 2025
@claude
Copy link
Contributor

claude bot commented Oct 17, 2025

Claude finished @felixfeng33's task —— View job


Pull Request Review

Summary

This PR extracts the keyToDataAttribute function and related logic from getNodeDataAttributes.ts into a new utility file getNodeDataAttributeKeys.ts. The refactoring appears to be part of decoupling React-specific code, although the changes don't directly remove React dependencies but rather reorganize utility functions.

Code Quality ✅

The code changes follow good software engineering principles:

  • Single Responsibility: The extracted getNodeDataAttributeKeys function has a clear, focused purpose
  • Reusability: Both static and React-based code can now use these utilities
  • Naming: Function names clearly describe their purpose
  • Type Safety: Proper TypeScript types are maintained throughout

Changes Analysis

New File: getNodeDataAttributeKeys.ts

export const getNodeDataAttributeKeys = (node: TElement | TText) => {
  return Object.keys(node)
    .filter(
      (key) =>
        typeof node[key] !== 'object' &&
        (!TextApi.isText(node) || key !== 'text')
    )
    .map((key) => keyToDataAttribute(key));
};

Good extraction of reusable logic that filters node keys and transforms them to data attributes.

Modified Files

  1. HTML Deserialization Chain: Added isSlate parameter that flows through:

    • deserializeHtmlNode.tshtmlElementToElement.tsdeserializeHtmlNodeChildren.ts

    This allows proper handling of slate-specific nodes during deserialization.

  2. Data Attribute Handling:

    • getDataNodeProps.ts: Added type parameter to getDefaultNodeProps
    • getPluginNodeProps.ts: Now uses the extracted getNodeDataAttributeKeys
    • getNodeDataAttributes.ts: Simplified by using the extracted utility

Issues Found

🚨 Missing Changeset (Required)

Despite the checklist showing changeset as complete, the changeset bot indicates no changeset was found. This is a breaking requirement for package updates.

Action Required: Add a changeset by running:

yarn changeset add

Select @platejs/core package with patch version bump.

⚠️ Potential Test Coverage Gap

While existing tests may cover the refactored code paths, there are no direct unit tests for:

  • getNodeDataAttributeKeys function
  • keyToDataAttribute function

Consider adding unit tests in packages/core/src/lib/utils/getNodeDataAttributeKeys.spec.ts.

Recommendations

  1. Add the missing changeset - This is blocking the PR
  2. Consider adding unit tests for the newly extracted functions
  3. Update the PR description to explain:
    • What specifically was decoupled from React
    • The motivation for this refactoring
    • Any breaking changes or migration requirements

Security & Error Handling ✅

  • No security concerns identified
  • Error handling remains consistent with existing patterns
  • Type safety is maintained throughout

Documentation

No documentation updates appear necessary for this internal refactoring.

Conclusion

The code quality is good and the refactoring improves code organization. However, the missing changeset is a blocker that must be addressed before merging.

@claude
Copy link
Contributor

claude bot commented Oct 17, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR.

Comment on lines +1 to +8
import type { TElement, TText } from '@platejs/slate';

import type { SlateEditor } from '../../editor';

import { type AnyEditorPlugin, getEditorPlugin } from '../../plugin';
import {
keyToDataAttribute,
} from '../../utils/getNodeDataAttributeKeys';

Choose a reason for hiding this comment

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

P1 Badge Maintain static export for data attribute helpers

Moving getNodeDataAttributeKeys and keyToDataAttribute into utils stopped packages/core/static from exporting them. Any consumers that imported these helpers from the static barrel (e.g. import { getNodeDataAttributeKeys } from '@udecode/plate-core/static') will now receive an undefined export even though no breaking-change note or changeset accompanied the commit. If the intent was only to refactor the implementation to avoid React dependencies, the static entry point should continue re‑exporting these helpers from their new location to avoid an inadvertent breaking API change.

Useful? React with 👍 / 👎.

@felixfeng33
Copy link
Collaborator Author

@claude /changeset

@claude
Copy link
Contributor

claude bot commented Oct 17, 2025

Claude finished @felixfeng33's task —— View job


✅ Changeset created successfully!

Todo List

  • Analyze PR changes
  • Read changed files to understand the scope
  • Determine version bump type (patch/minor/major)
  • Create changeset file
  • Commit and push changeset

Summary

Created changeset for decoupling React utilities from static exports. The changeset:

  • Marks this as a patch version bump for @platejs/core
  • Documents the breaking change where getNodeDataAttributeKeys and keyToDataAttribute are no longer exported from @platejs/core/static
  • Provides migration instructions for users to import from @platejs/core instead

The changeset has been committed to the branch.

@claude
Copy link
Contributor

claude bot commented Oct 17, 2025

Claude finished @felixfeng33's task —— View job


Tasks

  • Read changed files to understand the refactoring
  • Analyze API impact and determine change type (major/minor/patch)
  • Create appropriate changeset file(s)
  • Provide recommendations if needed

Summary

I've successfully created a changeset for your PR that moves getNodeDataAttributeKeys and keyToDataAttribute functions from static utilities to regular utilities.

Changeset Details:

  • Package: @platejs/core
  • Version Bump: patch
  • Reason: This is an internal refactoring with no API changes. Both functions remain accessible from the main @platejs/core export.

The changeset has been committed to your branch: .changeset/react-decouple-patch.md

Recommendation

The refactoring looks good. While the code reviewer mentioned that these functions are no longer exported from the static barrel, since @platejs/core doesn't have a separate /static export path in package.json, this change shouldn't break any consumers. Users importing from @platejs/core will still have access to these functions.

claude bot and others added 2 commits October 17, 2025 02:22
@felixfeng33 felixfeng33 merged commit ff915b0 into main Oct 17, 2025
6 of 7 checks passed
@felixfeng33 felixfeng33 deleted the fix/react-decouple branch October 17, 2025 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants