Conversation
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis PR updates multiple compiler/runtime components to support thunk-wrapped component nodes and optional-chained map transformations. It adjusts render/for/expression runtime handling, updates generators and Babel plugins, and broadens tests. Several package.json files switch workspace dependencies from workspace:* to workspace:^. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Runtime as runtime.render
participant DOM as Container
Caller->>Runtime: render(compNode | () => compNode, container)
alt compNode is function
Runtime->>Runtime: compNode = compNode()
end
Runtime->>DOM: validate/clear/insertNode(compNode)
Runtime-->>Caller: runDidMount()
sequenceDiagram
participant Src as Source Code
participant Babel as mapping2ForPlugin
participant Gen as ForGenerator
participant RT as ForNode (runtime)
Src->>Babel: array?.map?(...) in JSX
Babel->>Gen: transform to <for each=... map=...>
Gen->>Gen: keyMappingFn -> value?.map?.(...)
Gen-->>Src: emits createForNode(...)
Src->>RT: execute at runtime
RT-->>Src: iterates with optional-chained mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (5)
next-packages/runtime/src/Nodes/MutableNodes/for.ts (5)
401-404: Bug: updateItemFunc invoked with wrong arguments.You're passing (dirtyBits, item) but the signature is (item: Value, idx: number). This prevents per-item updates from receiving the correct item and index.
- newUpdateArr[prevIdx]?.(this.owner.dirtyBits, newData[idx]); + newUpdateArr[prevIdx]?.(newData[idx], idx);
256-261: Stale cached nodes risk: mark nodes as dirty when deleting.removeNodesInLifeCycle deletes from nodesMap but doesn’t flip nodesDirty. Any consumer of this.nodes may see stale cachedNodes.
Outside this hunk, update removeNodesInLifeCycle to set nodesDirty:
removeNodesInLifeCycle(nodes: InulaBaseNode[], key: Value) { this.runWillUnmount(key); super.removeNodes(nodes); this.runDidUnmount(key); this.nodesMap.delete(key); this.nodesDirty = true; // ensure nodes getter recomputes }
49-57: Guard against missing node groups in nodes getter.If keys/data fall out of sync, spreading undefined throws. Default to [] to be safe.
Outside this hunk, update nodes getter:
get nodes() { if (!this.nodesDirty) return this.cachedNodes; const nodes: InulaBaseNode[] = []; for (let idx = 0; idx < this.data.length; idx++) { const key = this.keys?.[idx] ?? idx; const group = this.nodesMap.get(key) ?? []; nodes.push(...group); } this.cachedNodes = nodes; this.nodesDirty = false; return nodes; }
92-99: Guard iteration in updateItem when group is missing.Iterating over undefined will throw.
Outside this hunk, update updateItem:
updateItem(idx: number, data: Value[]) { this.updateItemFuncArr[idx]?.(data[idx], idx); const key = this.keys?.[idx] ?? idx; const group = this.nodesMap.get(key) ?? []; for (const node of group) { update(node); } }
267-312: Handle non-array data in updateWithOutKey to support optional chaining transforms.Coerce non-arrays to [] to match arr?.map(...) semantics.
Outside this hunk, update updateWithOutKey:
updateWithOutKey() { const rawData = this.dataFunc(); const newData = Array.isArray(rawData) ? rawData : []; const preLength = this.data.length; const currLength = newData.length; // ...rest of function unchanged }
🧹 Nitpick comments (10)
next-packages/runtime/src/Nodes/MutableNodes/expression.ts (1)
66-68: Unconditionally invoking function-valued children: consider guarding against async thunks and unexpected return typesCalling thunks here is aligned with the new render contract. However, if a function returns a Promise (async thunk) or another unexpected value, it will flow into the node list and likely break later during mount. Suggest guarding against Promises or surfacing a clear error in dev.
Apply this diff to add a defensive check:
- if (typeof node === 'function') { - return node(); - } + if (typeof node === 'function') { + const out = node(); + // Guard against accidental async thunks that return a Promise + if (out && typeof (out as any).then === 'function') { + throw new Error( + 'Async thunks in children are not supported. Return a CompNode or an array of CompNodes, not a Promise.' + ); + } + return out; + }If you anticipate nested thunks or deeper arrays, we can also refactor to recursively unwrap and normalize nodes to avoid partial flattening. Happy to draft that if desired.
next-packages/runtime/test/e2e/jsx-rendering.test.tsx (1)
43-44: Rename test for clarity and grammarMinor naming nit: “multi times” → “multiple times” improves readability.
-it('should render JSX variable multi times', ({ container }) => { +it('should render the same JSX variable multiple times', ({ container }) => {next-packages/compiler/babel-inula-next-core/src/sugarPlugins/jsxSlicePlugin.ts (1)
90-95: Name the wrapper as a “thunk” for clarity and reuse it consistentlyThe new wrapper correctly defers
createCompNode(sliceId). A small readability nit: naming itsliceThunkcommunicates intent better and reads cleaner in both branches.Apply this diff:
- const jsxSliceAlternative = + const sliceThunk = t.arrowFunctionExpression( [], t.callExpression(t.identifier(importMap.createCompNode), [sliceId]) ); if (path.parentPath.isArrowFunctionExpression()) { // special case: returned by arrow function const block = t.blockStatement([ sliceComp, - t.returnStatement(jsxSliceAlternative), + t.returnStatement(sliceThunk), ]); path.replaceWith(block); } else { // insert into the previous statement const stmt = path.getStatementParent(); if (!stmt) { throw new Error('Cannot find the statement parent'); } stmt.insertBefore(sliceComp); - path.replaceWith(jsxSliceAlternative); + path.replaceWith(sliceThunk); }Also applies to: 100-101, 110-111
next-packages/runtime/test/e2e/for.test.tsx (2)
62-72: Prefer const for non-reassigned variables
extraisn’t reassigned.Apply this diff:
- let extra = 'extra'; + const extra = 'extra';
359-403: Fix inline snapshot escaping to satisfy no-useless-escapeThe inline snapshot uses unnecessary
\"escapes. Use a template literal to avoid escaping double quotes.Apply this diff:
- expect(container.innerHTML).toMatchInlineSnapshot( - '"<div style=\"margin-left: 0px;\"><div>Comment: Root comment</div><div style=\"margin-left: 20px;\"><div>Comment: First reply</div></div><div style=\"margin-left: 20px;\"><div>Comment: Second reply</div></div></div>"' - ); + expect(container.innerHTML).toMatchInlineSnapshot( + `"<div style="margin-left: 0px;"><div>Comment: Root comment</div><div style="margin-left: 20px;"><div>Comment: First reply</div></div><div style="margin-left: 20px;"><div>Comment: Second reply</div></div></div>"` + );next-packages/compiler/babel-inula-next-core/test/sugarPlugins/jsxSlice.test.ts (1)
85-97: Array elements as thunks are fine; consider a sparse-array case.Minor: add a test for sparse arrays or arrays containing non-JSX items to ensure the plugin doesn't over-transform mixed content.
Would you like me to add a test variant like: const arr = [
, null, 0, ] and assert only the JSX entries are wrapped?next-packages/runtime/src/Nodes/MutableNodes/for.ts (2)
75-75: Nit: spacing around assignment.- const data= dataFunc(); + const data = dataFunc();
107-112: Minor: avoid optional field for boolean sentinel.notInitialized? = true is odd; make it a plain boolean to reflect intent.
Outside this hunk:
- notInitialized? = true; + notInitialized = true;next-packages/compiler/babel-inula-next-core/test/sugarPlugins/mappingToFor.test.tsx (1)
109-133: Optional-chaining map → for transform looks correct; add a safety test.The transform drops optional chaining on the data source (). This is fine if ForNode treats non-array values as [] during updates. Given the runtime change in ForNode’s constructor only, please add a test ensuring comment.replies toggling between undefined and an array doesn’t crash and renders nothing for undefined.
I can add a follow-up plugin test for comment.replies?.map?.(…) and an e2e runtime test that mutates replies from undefined → [] → [x] and back, to validate stability. Want me to draft those?
next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts (1)
71-81: Callee extraction is solid; consider computed '["map"]' support and narrowing typesHandling both MemberExpression and OptionalMemberExpression here is correct. Two small enhancements to consider:
- Support computed property access where users write arr?.['map']?.(cb). Today, the plugin silently skips this valid case because property isn’t an Identifier.
- Narrow NodePath types to MemberExpression | OptionalMemberExpression after the type guard to improve maintainability.
Example adjustment for property checks (pseudo):
const isMapProp = map.isIdentifier({ name: 'map' }) || (callee.node.computed === true && map.isStringLiteral({ value: 'map' })); if (!isMapProp) return;This would allow both arr?.map?.(cb) and arr?.['map']?.(cb).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (22)
next-packages/compiler/babel-inula-next-core/package.json(1 hunks)next-packages/compiler/babel-inula-next-core/src/sugarPlugins/jsxSlicePlugin.ts(2 hunks)next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts(4 hunks)next-packages/compiler/babel-inula-next-core/test/e2e/mappingToFor.test.ts(1 hunks)next-packages/compiler/babel-inula-next-core/test/e2e/subComponent.test.tsx(1 hunks)next-packages/compiler/babel-inula-next-core/test/generator/view.test.tsx(1 hunks)next-packages/compiler/babel-inula-next-core/test/sugarPlugins/jsxSlice.test.ts(2 hunks)next-packages/compiler/babel-inula-next-core/test/sugarPlugins/mappingToFor.test.tsx(1 hunks)next-packages/compiler/error-handler/package.json(1 hunks)next-packages/compiler/jsx-parser/package.json(1 hunks)next-packages/compiler/reactivity-parser/package.json(1 hunks)next-packages/compiler/unplugin-inula-next/package.json(1 hunks)next-packages/compiler/view-generator/package.json(1 hunks)next-packages/compiler/view-generator/src/NodeGenerators/ForGenerator.ts(2 hunks)next-packages/compiler/view-generator/src/NodeGenerators/SuspenseGenerator.ts(1 hunks)next-packages/inula-bridge/package.json(1 hunks)next-packages/runtime/package.json(1 hunks)next-packages/runtime/src/Nodes/MutableNodes/expression.ts(1 hunks)next-packages/runtime/src/Nodes/MutableNodes/for.ts(4 hunks)next-packages/runtime/src/render.ts(1 hunks)next-packages/runtime/test/e2e/for.test.tsx(2 hunks)next-packages/runtime/test/e2e/jsx-rendering.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
next-packages/runtime/test/e2e/jsx-rendering.test.tsx (1)
next-packages/runtime/src/render.ts (1)
render(11-21)
next-packages/compiler/babel-inula-next-core/test/sugarPlugins/mappingToFor.test.tsx (1)
packages/inula-dev-tools/babel.config.js (1)
api(16-40)
next-packages/compiler/babel-inula-next-core/test/e2e/mappingToFor.test.ts (1)
next-packages/compiler/unplugin-inula-next/src/index.ts (1)
transform(38-59)
next-packages/compiler/babel-inula-next-core/test/sugarPlugins/jsxSlice.test.ts (1)
packages/inula-dev-tools/babel.config.js (1)
api(16-40)
🪛 GitHub Actions: ci
next-packages/compiler/error-handler/package.json
[error] 1-1: Step: pnpm install. ERR_PNPM_OUTDATED_LOCKFILE: Cannot install with 'frozen-lockfile' because pnpm-lock.yaml is not up to date with /next-packages/compiler/error-handler/package.json.
🪛 ast-grep (0.38.6)
next-packages/runtime/src/render.ts
[warning] 14-14: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 ESLint
next-packages/runtime/test/e2e/for.test.tsx
[error] 63-63: 'extra' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 401-401: Unnecessary escape character: ".
(no-useless-escape)
[error] 401-401: Unnecessary escape character: ".
(no-useless-escape)
[error] 401-401: Unnecessary escape character: ".
(no-useless-escape)
[error] 401-401: Unnecessary escape character: ".
(no-useless-escape)
[error] 401-401: Unnecessary escape character: ".
(no-useless-escape)
[error] 401-401: Unnecessary escape character: ".
(no-useless-escape)
🔇 Additional comments (27)
next-packages/inula-bridge/package.json (1)
33-41: Caret workspace ranges adoption: LGTMSwitching to "workspace:^" for internal packages is a good move for predictable semver alignment across the monorepo.
next-packages/runtime/package.json (1)
37-38: LGTM on caret workspace range updateThe devDependency switch to "workspace:^" aligns with the repo-wide dependency strategy.
next-packages/compiler/jsx-parser/package.json (1)
26-27: LGTM on devDependency range updateConsistent with the monorepo’s move to caret workspace ranges.
next-packages/compiler/reactivity-parser/package.json (1)
28-30: Switching to workspace:^ is appropriate for published rangesGood change. Using "workspace:^" communicates a compatible semver range to external consumers upon publish, while still linking locally in the workspace. For pre-1.0 versions, caret ranges remain conservative (patch-level for 0.0.x), which aligns with safety expectations.
next-packages/runtime/test/e2e/jsx-rendering.test.tsx (1)
43-51: Solid coverage for reusing the same JSX variableThis test is valuable. It verifies that reusing a JSX variable multiple times yields distinct rendered instances rather than moving a single node—preventing subtle DOM ownership bugs.
next-packages/compiler/unplugin-inula-next/package.json (1)
88-90: No "workspace:*" occurrences found — caret workspace ranges look consistentI ran the provided audit across the repository's package.json files; it returned no matches for "workspace:*". No further changes required.
next-packages/compiler/babel-inula-next-core/test/e2e/subComponent.test.tsx (1)
70-72: Snapshot updates align with thunk-based subcomponent creationSwitching
inputto a factory (() => $$createCompNode(...)) and emitting$$createExpNode(() => input, () => [], 0)is consistent with the thunk/wrapper approach. Removing deps ([]) and lowering the priority to 0 is appropriate sinceinputis stable and the inner JSX_input tracks its own reactive state.next-packages/runtime/src/render.ts (1)
16-18: Thunk support in render looks correctType-guarding the argument and executing function-valued nodes aligns with the new JSX slice/thunk strategy.
next-packages/compiler/babel-inula-next-core/src/sugarPlugins/jsxSlicePlugin.ts (1)
96-101: Replacing an expression-bodied arrow with a block body is safe hereTransforming the JSX-returning arrow body to a block with a local declaration plus a return is valid and avoids scope leakage. Good call.
next-packages/compiler/view-generator/src/NodeGenerators/ForGenerator.ts (1)
70-75: createForNode accepts null keys; ForNode falls back to indexVerified: createForNode's keysFunc is typed as
null | (() => Value[]), and ForNode defineskeys?: Value[]and usesthis.keys?.[idx] ?? idxin the nodes getter — returningt.nullLiteral()when no key is provided is correct and downstream logic will fall back to the item index.
- next-packages/runtime/src/Nodes/MutableNodes/for.ts —
createForNodesignature (lines ~477–484) andForNodeclass (lines ~28–52) showingkeys?: Value[]and fallback usage.- next-packages/compiler/view-generator/src/NodeGenerators/ForGenerator.ts — generator returns
t.nullLiteral()for absent keys and passes the key mapper intocreateForNode(calls around lines ~35–43 and the checked snippet at ~70–75).next-packages/compiler/babel-inula-next-core/test/generator/view.test.tsx (1)
168-174: Snapshot update matches new optional-chained key mappingThe expected code now emits a block-bodied arrow and guards mapping with optional chaining; this aligns with the generator change.
next-packages/runtime/test/e2e/for.test.tsx (2)
269-308: Solid coverage: dependency updates across list and external stateGood end-to-end assertions for pushing items and independent counter changes within the same render tree.
310-318: Nice breadth of for-loop scenariosEmpty arrays, removals, clear operations, and optional member expressions are all exercised. This materially reduces regression risk.
Also applies to: 320-339, 341-357, 405-436
next-packages/compiler/babel-inula-next-core/test/sugarPlugins/jsxSlice.test.ts (9)
34-36: Thunks for JSX slice assignment look correct.Wrapping the slice in a thunk aligns with the new runtime that accepts function-valued nodes.
41-66: Multi-level JSX slice transformation LGTM.Snapshot reflects correct thunk-wrapping of the component node.
69-81: Ternary branches correctly return thunks.This preserves lazy evaluation and prevents premature node creation.
101-112: Attribute value thunking relies on runtime invocation — please verify.Ensure the runtime attribute/expr handlers will invoke function-valued entries (e.g., ExpNode/props path). Without that, icon would receive a function instead of a node.
I can add an integration test asserting that <div icon={}/> renders the Icon in attributes after transformation to icon={() => $$createCompNode(JSX_Icon)}. Want me to draft it?
116-131: Fragment handling with thunks looks consistent.Approach is consistent for both direct assignment and conditional branches.
135-164: Correct: transform inside nested arrow but not the arrow-return of .This matches the documented behavior; inner JSX returned from fn is wrapped, outer
{fn(color)} remains intact.
168-186: Arrow-return JSX is wrapped correctly.Render prop output as a thunk is expected with the updated runtime.
204-206: Return-path JSX thunking is correct.Snapshot matches the new pattern.
212-224: No transform for arrow in is correct.Maintains expected semantics for the iterator callback.
next-packages/compiler/view-generator/package.json (1)
26-32: Workspace deps switched to caret range — good.This aligns with publishing-friendly semver ranges across the monorepo.
Double-check your package manager (pnpm/yarn) version supports "workspace:^" and that your release pipeline respects it during publish. Need a quick repo-wide scan to ensure consistency?
next-packages/compiler/babel-inula-next-core/package.json (1)
38-43: Consistent switch to "workspace:^" across compiler deps.Looks good and consistent with the PR objective.
next-packages/compiler/babel-inula-next-core/test/sugarPlugins/mappingToFor.test.tsx (1)
106-107: Snapshot literal formatting change is fine.No functional impact.
next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts (2)
39-44: OptionalCallExpression support added — good coverageRegistering a visitor for OptionalCallExpression and delegating to the same handler as CallExpression is the right approach to support both arr.map(...) and arr?.map?.(...).
115-120: Good: nested OptionalCallExpression traversal is coveredAdding OptionalCallExpression to the inner traversal ensures nested optional map calls inside the generated for are processed too. This keeps nested transformations consistent.
| * Convert map call (regular or optional) in JSXExpressionContainer to for visitor | ||
| * | ||
| * @param path Map call expression path | ||
| * @param path Map call expression path (CallExpression or OptionalCallExpression) | ||
| * @param inner is inside for tag | ||
| */ | ||
|
|
||
| function callExpressionVisitor(path: NodePath<t.CallExpression>, inner: boolean): void { | ||
| function mapCallVisitor(path: NodePath<t.CallExpression | t.OptionalCallExpression>, inner: boolean): void { |
There was a problem hiding this comment.
Potentially missed cases due to ChainExpression wrappers around optional chains
Optional chaining in Babel can introduce a ChainExpression wrapper around the entire chain. In such cases:
- For top-level usage in JSX, the OptionalCallExpression’s parent may be ChainExpression, not JSXExpressionContainer.
- For nested usage inside map callbacks, the immediate parent may be ChainExpression, not ArrowFunctionExpression.
Because the current checks require the immediate parent to be JSXExpressionContainer (outer) or ArrowFunctionExpression (inner), these cases may be skipped and not transformed.
Proposed fix: Loosen the parent checks to tolerate an intermediate ChainExpression, and replace the correct target when doing path.replaceWith(). Here is a robust version of mapCallVisitor illustrating the adjustments (focus: parent checks and replacement target):
function mapCallVisitor(path: NodePath<t.CallExpression | t.OptionalCallExpression>, inner: boolean): void {
// Helper: climb through a ChainExpression parent if present
const unwrapChainParent = <T extends Node>(p: NodePath<T>) =>
p.parentPath && p.parentPath.isChainExpression() ? p.parentPath : p;
const parent = path.parentPath;
const parentOrChain = parent && parent.isChainExpression() ? parent.parentPath : parent;
// Match arrow function map call inside for tag
if (inner) {
const isInsideArrow =
parent?.isArrowFunctionExpression() ||
(parent?.isChainExpression() && parent.parentPath?.isArrowFunctionExpression());
if (!isInsideArrow) return;
}
// Match map call in JSX expression container
if (!inner) {
const isInJsxExpr =
parent?.isJSXExpressionContainer() ||
(parent?.isChainExpression() && parent.parentPath?.isJSXExpressionContainer());
if (!isInJsxExpr) return;
}
// Don't convert map call inside any JSXOpeningElement (e.g. attributes)
if (path.parentPath?.parentPath?.parentPath?.isJSXOpeningElement()) return;
const callee = path.get('callee');
// … existing callee handling …
// Generate for tag ...
const forElement = /* as in current implementation */;
// Replace correct target: if callee node is wrapped in a ChainExpression parent, replace that
const replaceTarget = parent?.isChainExpression() ? parent : path;
if (replaceTarget.parentPath?.isArrowFunctionExpression()) {
replaceTarget.replaceWith(forElement);
} else {
replaceTarget.parentPath?.replaceWith(forElement);
}
// Traverse inner map calls inside the new for
if (!inner) {
(replaceTarget.parentPath ?? path.parentPath)?.traverse({
CallExpression(innerPath: NodePath<t.CallExpression>) {
mapCallVisitor(innerPath, true);
},
OptionalCallExpression(innerPath: NodePath<t.OptionalCallExpression>) {
mapCallVisitor(innerPath, true);
},
});
}
}This keeps your current logic but ensures wrapped chains aren’t accidentally skipped.
🤖 Prompt for AI Agents
In
next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts
around lines 51-56, optional chaining can wrap call expressions in a
ChainExpression so current checks that require the immediate parent to be
JSXExpressionContainer (outer) or ArrowFunctionExpression (inner) will miss
those cases; update the visitor to tolerate an intermediate ChainExpression by
treating parentOrChain = parentPath.isChainExpression() ? parentPath.parentPath
: parentPath (or a helper to unwrap one ChainExpression), use that unwrapped
parent for the JSX/ArrowFunction checks, and when replacing ensure you replace
the ChainExpression node (or its parent) rather than the raw call path (i.e.,
set replaceTarget = parentPath.isChainExpression() ? parentPath : path and then
replace either replaceTarget or replaceTarget.parentPath as appropriate), and
finally traverse inside the newly inserted for element to call mapCallVisitor
for nested CallExpression/OptionalCallExpression occurrences.
| it('should transform to optional key mapping ', () => { | ||
| const code = ` | ||
| function MyComp({ comment }) { | ||
| return ( | ||
| <div > | ||
| {comment.replies?.map(reply => ( | ||
| <CommentComponent | ||
| key={reply.id} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ) | ||
| } | ||
| `; | ||
| const transformedCode = transform(code); | ||
| expect(transformedCode).toMatchInlineSnapshot(` | ||
| "import { compBuilder as $$compBuilder, createForNode as $$createForNode, createCompNode as $$createCompNode, createHTMLNode as $$createHTMLNode } from "@openinula/next"; | ||
| function MyComp({ | ||
| comment | ||
| }) { | ||
| const $$self = $$compBuilder(); | ||
| $$self.addProp("comment", $$value => comment = $$value, 1); | ||
| return $$self.prepare().init($$createHTMLNode("div", null, $$createForNode(() => comment.replies, () => { | ||
| return comment.replies?.map?.(reply => reply.id); | ||
| }, ($$n, updateItemFuncArr, reply, $$key, $$i) => { | ||
| updateItemFuncArr[$$i] = (newItem, newIdx) => { | ||
| reply = newItem; | ||
| }; | ||
| return [$$createCompNode(CommentComponent, { | ||
| "key": reply.id | ||
| }, $$node => { | ||
| $$node.updateProp("key", () => reply.id, [reply?.id], 1); | ||
| })]; | ||
| }, 1))); | ||
| }" | ||
| `); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Great coverage for arr?.map with key mapping; add a couple of edge-case tests
This snapshot thoroughly validates the optional map transformation and key handling. To avoid regressions, consider adding:
- arr.map?.(cb) form (optional call on the method), e.g., comment.replies.map?.(reply => ).
- Nested optional mapping inside the callback, e.g., list.map(item => item.children?.map(child => )).
- Computed property access: comment.replies?.['map']?.(reply => ...).
These ensure both OptionalCallExpression variants and computed properties are covered.
You can check if these cases are already covered anywhere with:
🏁 Script executed:
#!/bin/bash
# 1) Look for existing tests using `map?.(`, nested optional mapping, or computed '["map"]'
rg -n --no-heading "map\?\.\(|\?\.\s*map\(|\?\.\s*\[\s*['\"]map['\"]\s*\]\s*\?\.\s*\(" -A 2 test
# 2) Confirm no handling of `ChainExpression` in the plugin (helps validate the earlier concern)
rg -n --no-heading "ChainExpression" next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.tsLength of output: 267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for literal 'map?.(' occurrences =="
rg -n -F 'map?.(' || true
echo
echo "== Search for literal '?.map(' occurrences =="
rg -n -F '?.map(' || true
echo
echo "== Search for literal '\"map\"' and \"'map'\" (to find computed property uses) =="
rg -n -F '"map"' || true
rg -n -F "'map'" || true
echo
echo "== Locate mapping2ForPlugin.ts =="
mapping_files=$(fd -HI --type f mapping2ForPlugin.ts 2>/dev/null || true)
if [ -z "$mapping_files" ]; then
mapping_files=$(find . -type f -name mapping2ForPlugin.ts 2>/dev/null || true)
fi
if [ -z "$mapping_files" ]; then
echo "No mapping2ForPlugin.ts found."
else
echo "Found files:"
printf '%s\n' "$mapping_files"
fi
echo
echo "== Show mapping2ForPlugin.ts (first 400 lines) and search for 'ChainExpression' =="
if [ -n "$mapping_files" ]; then
while IFS= read -r f; do
echo "---- $f ----"
sed -n '1,400p' "$f" || true
echo
echo "Search for 'ChainExpression' in $f:"
rg -n "ChainExpression" "$f" || true
echo
done <<< "$mapping_files"
fi
echo
echo "== Search for 'ChainExpression' anywhere in repo =="
rg -n "ChainExpression" || trueLength of output: 8138
Good coverage for arr?.map?. — computed-property + nested-callback cases need attention
Short summary: arr?.map?.(...) is already covered in tests, but the mapping plugin only accepts Identifier properties named "map", so computed-access forms (e.g. obj?.['map']?.(...)) will be ignored. Please add tests for the missing edge cases and decide whether to extend the plugin.
Files/locations found
- Tests already covering optional-call forms:
- next-packages/compiler/babel-inula-next-core/test/e2e/mappingToFor.test.ts — line ~96:
return comment.replies?.map?.(reply => reply.id); - next-packages/compiler/babel-inula-next-core/test/generator/view.test.tsx — line ~169:
return list?.map?.((item, index) => index);
- next-packages/compiler/babel-inula-next-core/test/e2e/mappingToFor.test.ts — line ~96:
- Plugin code that blocks computed-property handling:
- next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts — contains
if (!map.isIdentifier()) { return; }if (map.node.name !== 'map') { return; }
These checks cause computed property access (e.g.comment.replies?.['map']?.(...)) to be skipped.
- next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts — contains
Suggested actions
- Add a test for nested optional mapping inside the callback (e.g.
list.map(item => item.children?.map(child => <X key={child.id}/>))). - Add a test for computed-property access (
comment.replies?.['map']?.(reply => <CommentComponent key={reply.id} />)) to assert current behaviour. - If computed-access should be supported, update mapping2ForPlugin.ts to accept computed string-literal/property
'map'(and handle any ChainExpression variants), then add corresponding tests.
🤖 Prompt for AI Agents
In next-packages/compiler/babel-inula-next-core/test/e2e/mappingToFor.test.ts
around lines 73 to 109, add tests for two missing edge cases: (1) a nested
optional mapping inside the map callback (e.g. list.map(item =>
item.children?.map(child => <X key={child.id}/>))) to ensure inner optional-map
transforms, and (2) a computed-property access form (e.g.
comment.replies?.['map']?.(reply => <CommentComponent key={reply.id} />)) to
capture current behavior. Then decide whether to extend the plugin: if
supporting computed access, update
next-packages/compiler/babel-inula-next-core/src/sugarPlugins/mapping2ForPlugin.ts
to accept computed property access and ChainExpression variants by allowing
MemberExpression with computed string-literal 'map' (and ChainExpression
wrapping) instead of only Identifier named "map", handle extraction of the
property name safely, and add the new tests asserting the transformed output;
otherwise add the computed-access test asserting it remains untransformed.
| "@openinula/babel-api": "workspace:^" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
CI is failing due to an outdated pnpm-lock.yaml; update lockfile after changing workspace ranges
The change to "workspace:^" is fine, but the CI failure indicates the lockfile is out of sync. Please regenerate and commit pnpm-lock.yaml.
Recommended steps:
- Run: pnpm install -w to refresh the workspace lockfile.
- Commit pnpm-lock.yaml.
- If CI must remain frozen, ensure the updated lockfile is part of this PR. Alternatively (less ideal), adjust CI to not use --frozen-lockfile for this run.
Optionally verify no lingering "workspace:*" specifiers:
🏁 Script executed:
#!/bin/bash
# Find any remaining workspace:* specifiers
rg -n $'\"workspace:\\*\"' -S --glob '!**/pnpm-lock.yaml'Length of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Git branch: $(git rev-parse --abbrev-ref HEAD 2>/dev/null || true)"
echo "Git short HEAD: $(git rev-parse --short HEAD 2>/dev/null || true)"
echo
echo "1) Searching for any workspace:* or workspace:^ specifiers (excluding pnpm-lock.yaml)..."
rg -n --hidden -S '"workspace:[^"]*"' --glob '!**/pnpm-lock.yaml' || true
echo
echo "2) Searching for workspace specifiers inside pnpm-lock.yaml files (if present)..."
rg -n --hidden -S '"workspace:[^"]*"' || true
echo
echo "3) Show the package.json under review (next-packages/compiler/error-handler/package.json):"
if [ -f next-packages/compiler/error-handler/package.json ]; then
sed -n '1,240p' next-packages/compiler/error-handler/package.json || true
else
echo "File not found: next-packages/compiler/error-handler/package.json"
fi
echo
echo "4) List pnpm-lock.yaml files in the repo (if any):"
if command -v fd >/dev/null 2>&1; then
fd -HI --hidden --type f pnpm-lock.yaml || true
else
rg --files --hidden -g 'pnpm-lock.yaml' || true
fi
echo
echo "5) Show files changed in the last commit (HEAD):"
git show --name-only --pretty=format:'' HEAD || true
echo
echo "6) Git status (porcelain):"
git status --porcelain || true
echoLength of output: 6584
Regenerate and commit pnpm-lock.yaml — lockfile out of sync
Multiple package.json files were changed to use "workspace:^" but pnpm-lock.yaml was not updated in this PR, so CI (with --frozen-lockfile) will fail.
Files requiring the lockfile refresh:
- next-packages/compiler/babel-inula-next-core/package.json
- next-packages/compiler/error-handler/package.json
- next-packages/compiler/jsx-parser/package.json
- next-packages/compiler/reactivity-parser/package.json
- next-packages/compiler/unplugin-inula-next/package.json
- next-packages/compiler/view-generator/package.json
- next-packages/inula-bridge/package.json
- next-packages/runtime/package.json
Relevant snippet (example):
"@openinula/babel-api": "workspace:^"
Recommended fix:
- Run: pnpm install -w
- Commit the updated pnpm-lock.yaml to this branch/PR
- Ensure CI uses the updated lockfile (avoid temporarily disabling --frozen-lockfile unless absolutely necessary)
Note: I verified there are no remaining "workspace:*" specifiers.
🤖 Prompt for AI Agents
In next-packages/compiler/error-handler/package.json around lines 21 to 22, the
package.json was changed to use "workspace:^" but pnpm-lock.yaml was not
regenerated, causing CI --frozen-lockfile to fail; run pnpm install -w at the
monorepo root to refresh pnpm-lock.yaml (this will pick up the workspace:^
changes across the listed packages), add and commit the updated pnpm-lock.yaml
to this branch, and push so CI uses the updated lockfile (do not disable
--frozen-lockfile).
| const fallbacked = t.callExpression( | ||
| t.memberExpression(suspenseNode, t.identifier('fallback')), [ | ||
| fallbackNode, | ||
| ]); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm fallback API: passing raw expression may change laziness semantics
Previously the generator emitted .fallback(() => ${fallback}), which defers evaluation. Changing to pass fallbackNode directly risks eager evaluation if fallback is a JSX/comp node instead of a factory. Please confirm createSuspenseNode().fallback now expects a node rather than a thunk. If not, wrap fallbackNode in an arrow to preserve laziness:
-const fallbacked = t.callExpression(
- t.memberExpression(suspenseNode, t.identifier('fallback')), [
- fallbackNode,
-]);
+const fallbacked = t.callExpression(
+ t.memberExpression(suspenseNode, t.identifier('fallback')),
+ [t.arrowFunctionExpression([], fallbackNode)]
+);Also, update the JSDoc/example (Lines 10-11) if the new, non-thunk shape is intentional to avoid confusing consumers.
Run this to verify the expected parameter shape for fallback at its definition:
🏁 Script executed:
#!/bin/bash
# Locate createSuspenseNode and fallback signature/usages to confirm expected arg type.
rg -n -A 8 -B 4 'createSuspenseNode\('
rg -n -A 2 -B 2 '\.fallback\s*\('
rg -n -A 10 -B 5 'function\s+createSuspenseNode|const\s+createSuspenseNode|export\s+const\s+createSuspenseNode|export\s+function\s+createSuspenseNode'Length of output: 4537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show Suspense.ts ==="
sed -n '1,240p' next-packages/runtime/src/Nodes/MutableNodes/Suspense.ts || true
echo; echo "=== search for 'fallback(' across repo ==="
rg -n --hidden --no-ignore 'fallback\s*\(' || true
echo; echo "=== show surrounding area where createSuspenseNode is exported ==="
rg -n -A8 -B8 'export function createSuspenseNode' next-packages/runtime/src/Nodes/MutableNodes/Suspense.ts || trueLength of output: 279282
Pass a thunk to Suspense.fallback — runtime expects () => InulaBaseNode
Confirmed: next-packages/runtime/src/Nodes/MutableNodes/Suspense.ts defines
fallback(fallback: () => InulaBaseNode) { this.fallbackNode = fallback(); return this; }
so the runtime expects a thunk and calls it. Passing the raw node from the generator will cause eager evaluation and changes semantics.
Files to fix:
- next-packages/compiler/view-generator/src/NodeGenerators/SuspenseGenerator.ts
- Replace the current call that passes
fallbackNodedirectly with a thunk-wrapping arrow function.
- Replace the current call that passes
Suggested change (update generator):
-const fallbacked = t.callExpression(
- t.memberExpression(suspenseNode, t.identifier('fallback')), [
- fallbackNode,
-]);
+const fallbacked = t.callExpression(
+ t.memberExpression(suspenseNode, t.identifier('fallback')),
+ [t.arrowFunctionExpression([], fallbackNode)]
+);- next-packages/runtime/src/Nodes/MutableNodes/Suspense.ts
- Reference:
fallback(fallback: () => InulaBaseNode) { this.fallbackNode = fallback(); return this; }— keeps the expected thunk API.
- Reference:
Docs: the JSDoc/example already shows .fallback(() => ${fallback}); keep it as-is. Only update docs if you intentionally change the runtime API to accept non-thunks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fallbacked = t.callExpression( | |
| t.memberExpression(suspenseNode, t.identifier('fallback')), [ | |
| fallbackNode, | |
| ]); | |
| const fallbacked = t.callExpression( | |
| t.memberExpression(suspenseNode, t.identifier('fallback')), | |
| [t.arrowFunctionExpression([], fallbackNode)] | |
| ); |
🤖 Prompt for AI Agents
In next-packages/compiler/view-generator/src/NodeGenerators/SuspenseGenerator.ts
around lines 26 to 29, the generator currently passes the raw fallbackNode into
Suspense.fallback but the runtime expects a thunk (() => InulaBaseNode); change
the call so it passes an arrow-function thunk that returns the fallback node
(e.g. create an arrow function with no params whose body returns the existing
fallbackNode AST) and use that thunk as the argument to the
t.callExpression/t.memberExpression instead of the raw fallbackNode so the
runtime can lazily evaluate the fallback.
| const data= dataFunc(); | ||
| if (Array.isArray(data)) { | ||
| this.data = [...data]; | ||
| if (keysFunc) this.keys = [...keysFunc()]; | ||
| } | ||
| this.update(); |
There was a problem hiding this comment.
Initialize guards only in ctor; update paths still assume arrays.
Constructor now guards non-array returns, but updateWithOutKey/updateWithKey still assume array-like returns. With the new compiler behavior transforming arr?.map(...) to , dataFunc may return undefined/null at runtime, which will throw on .length. Treat non-arrays as [] in updates.
Apply this diff within updateWithKey and I can follow up with a patch for updateWithOutKey:
- updateWithKey() {
- const newData = this.dataFunc();
- const newKeys = this.keysFunc!();
+ updateWithKey() {
+ const rawData = this.dataFunc();
+ const newData = Array.isArray(rawData) ? rawData : [];
+ const rawKeys = this.keysFunc!();
+ const newKeys = Array.isArray(rawKeys) ? rawKeys : [];Additionally, updateWithOutKey should coerce non-arrays to [] as shown below (outside this hunk).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In next-packages/runtime/src/Nodes/MutableNodes/for.ts around lines 75 to 80,
updateWithKey and updateWithOutKey still assume dataFunc() returns an array and
call array properties/methods directly; change both update paths to coerce
non-array returns to an empty array (e.g. let data = dataFunc(); if
(!Array.isArray(data)) data = [];), and similarly coerce keysFunc() before using
it (or default to []). Ensure all uses of .length, indexing, and iteration
operate on the coerced local array so runtime undefined/null from
optional-chaining inputs won’t throw.
| if (this.dataReactBits & this.owner.dirtyBits!) { | ||
| this.updateArray(); | ||
| return; | ||
| } | ||
| this.updateItems(); | ||
| // Only update items if there are dirty bits not covered by our reactive data bits | ||
| // This optimization prevents unnecessary item updates when all changes are already | ||
| // handled by the current reactive scope | ||
| // | ||
| // Example 1: this.dataReactBits -> 1110, changed (dirtyBits) -> 1010 | ||
| // ~this.dataReactBits & changed -> ~1110 & 1010 -> 0001 & 1010 -> 0000 | ||
| // No update needed because dataReactBits contains all the changed bits | ||
| // | ||
| // Example 2: this.dataReactBits -> 1110, changed (dirtyBits) -> 1101 | ||
| // ~this.dataReactBits & changed -> ~1110 & 1101 -> 0001 & 1101 -> 0001 | ||
| // Update needed because dataReactBits doesn't contain all the changed bits | ||
| // (bit 0 is dirty but not covered by our reactive scope) | ||
| if (~this.dataReactBits & this.owner.dirtyBits!) { | ||
| this.updateItems(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make dirty-bit checks explicit to avoid accidental truthiness traps.
Using raw bitwise results as booleans works but is easy to misread and fragile if Bits changes type. Compare against 0 for clarity.
- if (this.dataReactBits & this.owner.dirtyBits!) {
+ if (((this.dataReactBits) & (this.owner.dirtyBits!)) !== 0) {
this.updateArray();
}
- if (~this.dataReactBits & this.owner.dirtyBits!) {
+ if (((~this.dataReactBits) & (this.owner.dirtyBits!)) !== 0) {
this.updateItems();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.dataReactBits & this.owner.dirtyBits!) { | |
| this.updateArray(); | |
| return; | |
| } | |
| this.updateItems(); | |
| // Only update items if there are dirty bits not covered by our reactive data bits | |
| // This optimization prevents unnecessary item updates when all changes are already | |
| // handled by the current reactive scope | |
| // | |
| // Example 1: this.dataReactBits -> 1110, changed (dirtyBits) -> 1010 | |
| // ~this.dataReactBits & changed -> ~1110 & 1010 -> 0001 & 1010 -> 0000 | |
| // No update needed because dataReactBits contains all the changed bits | |
| // | |
| // Example 2: this.dataReactBits -> 1110, changed (dirtyBits) -> 1101 | |
| // ~this.dataReactBits & changed -> ~1110 & 1101 -> 0001 & 1101 -> 0001 | |
| // Update needed because dataReactBits doesn't contain all the changed bits | |
| // (bit 0 is dirty but not covered by our reactive scope) | |
| if (~this.dataReactBits & this.owner.dirtyBits!) { | |
| this.updateItems(); | |
| } | |
| } | |
| if (((this.dataReactBits) & (this.owner.dirtyBits!)) !== 0) { | |
| this.updateArray(); | |
| } | |
| // Only update items if there are dirty bits not covered by our reactive data bits | |
| // This optimization prevents unnecessary item updates when all changes are already | |
| // handled by the current reactive scope | |
| // | |
| // Example 1: this.dataReactBits -> 1110, changed (dirtyBits) -> 1010 | |
| // ~this.dataReactBits & changed -> ~1110 & 1010 -> 0001 & 1010 -> 0000 | |
| // No update needed because dataReactBits contains all the changed bits | |
| // | |
| // Example 2: this.dataReactBits -> 1110, changed (dirtyBits) -> 1101 | |
| // ~this.dataReactBits & changed -> ~1110 & 1101 -> 0001 & 1101 -> 0001 | |
| // Update needed because dataReactBits doesn't contain all the changed bits | |
| // (bit 0 is dirty but not covered by our reactive scope) | |
| if (((~this.dataReactBits) & (this.owner.dirtyBits!)) !== 0) { | |
| this.updateItems(); | |
| } | |
| } |
| export const render = (compNode: CompNode | (() => CompNode), container: HTMLElement) => { | ||
| if (container == null) { | ||
| throw new Error('Render target is empty. Please provide a valid DOM element.'); | ||
| } | ||
| container.innerHTML = ''; | ||
| if (typeof compNode === 'function') { | ||
| compNode = compNode(); | ||
| } | ||
| insertNode(container as InulaHTMLNode, compNode, 0); | ||
| runDidMount(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid param reassignment; resolve thunk before clearing; prefer replaceChildren() over innerHTML=''
- Don’t reassign the function parameter; use a local
nodefor clarity and lint-compatibility. - Resolve the thunk before clearing to avoid wiping the container if resolution throws.
container.replaceChildren()is cleaner and avoids the static analysis innerHTML warning.
Apply this diff:
export const render = (compNode: CompNode | (() => CompNode), container: HTMLElement) => {
if (container == null) {
throw new Error('Render target is empty. Please provide a valid DOM element.');
}
- container.innerHTML = '';
- if (typeof compNode === 'function') {
- compNode = compNode();
- }
- insertNode(container as InulaHTMLNode, compNode, 0);
+ const node = typeof compNode === 'function' ? compNode() : compNode;
+ container.replaceChildren();
+ insertNode(container as InulaHTMLNode, node, 0);
runDidMount();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const render = (compNode: CompNode | (() => CompNode), container: HTMLElement) => { | |
| if (container == null) { | |
| throw new Error('Render target is empty. Please provide a valid DOM element.'); | |
| } | |
| container.innerHTML = ''; | |
| if (typeof compNode === 'function') { | |
| compNode = compNode(); | |
| } | |
| insertNode(container as InulaHTMLNode, compNode, 0); | |
| runDidMount(); | |
| }; | |
| export const render = (compNode: CompNode | (() => CompNode), container: HTMLElement) => { | |
| if (container == null) { | |
| throw new Error('Render target is empty. Please provide a valid DOM element.'); | |
| } | |
| const node = typeof compNode === 'function' ? compNode() : compNode; | |
| container.replaceChildren(); | |
| insertNode(container as InulaHTMLNode, node, 0); | |
| runDidMount(); | |
| }; |
🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 14-14: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
In next-packages/runtime/src/render.ts around lines 11 to 21, avoid reassigning
the compNode parameter and resolve thunks before mutating the container: create
a local const node = typeof compNode === 'function' ? compNode() : compNode;
then clear the container using container.replaceChildren() instead of
container.innerHTML = '' (so the container is only cleared after the thunk
successfully resolves), and call insertNode(container as InulaHTMLNode, node, 0)
followed by runDidMount(); this keeps the parameter immutable and prevents
wiping the DOM if thunk throws while addressing the innerHTML lint warning.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores