Helpers: new fromStaticMarkup, fallback to react-dom/server if needed#613
Merged
Helpers: new fromStaticMarkup, fallback to react-dom/server if needed#613
fromStaticMarkup, fallback to react-dom/server if needed#613Conversation
🦋 Changeset detectedLatest commit: b44557b The changes in this PR will be included in the next version bump. 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 |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a static HTML/SVG-to-Takumi converter ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Docs preview is ready: https://feat-from-static-markup.preview.takumi.kane.tw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@takumi-helpers/src/jsx/jsx.ts`:
- Around line 211-244: containsReactContextProvider is incorrectly doing a raw
pre-scan that eagerly iterates generic iterables and descends through unresolved
wrappers (Promises, functions, memo, forwardRef), which consumes single-use
generators and misses providers introduced after rendering; stop doing that.
Change containsReactContextProvider to only check for a direct React context
provider node (inspect isValidElement and element.type.$$typeof ===
Symbol.for("react.context") and return false for iterables / Promises / non-HTML
components) and do NOT recursively iterate generic iterables or attempt to
resolve wrappers; then move the fallback/provider-detection that needs
rendered/awaited resolution into processReactElement where wrappers are actually
resolved (and where collectIterable can safely consume iterables), referencing
containsReactContextProvider, processReactElement, getElementChildren, and
collectIterable to implement the safer detection after rendering/awaiting.
- Around line 335-338: The current use of nullish coalescing turns an explicit
"no presets" into the default set; change the call to fromStaticMarkup to
preserve an explicitly provided presets value (including false) by using
presence-checking logic instead of ?? — e.g. set defaultStyles to
options.presets when the caller provided the presets property (so false stays
false), otherwise fall back to defaultStylePresets; update the object passed to
fromStaticMarkup (where defaultStyles is set) and keep references to
fromStaticMarkup, defaultStylePresets, and FromStaticMarkupOptions intact.
- Around line 234-239: The type check currently only detects React Context
objects by comparing element.type.$$typeof to Symbol.for("react.context"),
missing Context.Provider nodes; update the conditional that inspects
element.type and element.type.$$typeof to also accept
Symbol.for("react.provider") so Provider elements are routed to
renderWithReactDomServer instead of falling through to fromJsxInternal; locate
the check around element.type, $$typeof and adjust the comparison to include
both Symbol.for("react.context") and Symbol.for("react.provider") so Provider
detection is handled correctly.
In `@takumi-helpers/src/jsx/markup.ts`:
- Around line 271-273: The cssPropertyToJsProperty function currently
camel-cases CSS custom properties and breaks names like `--foo-bar`; update
cssPropertyToJsProperty to first detect properties that start with `--` and
return them unchanged, otherwise continue to perform the existing
hyphen-to-camelCase conversion (the existing regexp replace) so all non-custom
properties are converted as before.
- Around line 169-184: The current logic treats empty children arrays as
"onlyTextChildren" (because Array.every returns true for empty arrays), causing
empty elements to be emitted as empty text nodes; update the conditional that
decides between producing text(...) versus container(...) so it requires there
to be at least one child node (e.g., change the decision that uses
onlyTextChildren to also check children.nodes.length > 0) — modify the logic
around the onlyTextChildren variable and the return branch that calls
text({...}) / container({...}) so empty elements fall through to container(...)
instead of text("").
- Around line 108-143: The early isHtmlVoidElement({ type: element.name, props:
{} } as ReactElementLike) check is dropping <br> and <img> before the explicit
branches run; move or adjust that void-element check so <br> and <img> are
handled first: check element.name === "br" and element.name === "img" (and call
extractStaticNodeMetadata/parseDimension/text/image as shown) before calling
isHtmlVoidElement, or change the void check to exclude "br" and "img" so those
conversion branches execute.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 282f3db5-9bd4-45ec-80b1-181114a5c74f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.changeset/blue-tips-punch.md.changeset/weak-horses-fail.mdtakumi-helpers/package.jsontakumi-helpers/src/jsx/jsx.tstakumi-helpers/src/jsx/markup.tstakumi-helpers/src/jsx/react.tstakumi-helpers/src/jsx/utils.tstakumi-helpers/test/jsx/jsx-processing.test.tsxtakumi-helpers/tsconfig.jsontakumi-helpers/tsdown.config.ts
💤 Files with no reviewable changes (1)
- takumi-helpers/src/jsx/react.ts
kane50613
force-pushed
the
feat/from-static-markup
branch
from
a3b2acc to
36e3c0a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Tests