fix: prevent ASI in wrapCode causing refactoring to fail#41727
fix: prevent ASI in wrapCode causing refactoring to fail#41727
Conversation
WalkthroughThe pull request fixes an AST refactoring bug triggered by moustache bindings starting with newlines. Changes refactor wrap/unwrap logic to use constants instead of magic numbers, capture and preserve leading whitespace, and address automatic semicolon insertion (ASI) issues when wrapped code begins with a newline before object literals. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/client/packages/ast/src/index.test.ts (1)
868-875: Test name is misleading—it doesn't test the roundtrip.The test is named "wrapCode/unwrapCode roundtrip" but only asserts on
wrapCodeoutput. Either rename to"should wrap code with return statement"or actually test the roundtrip by verifyingunwrapCode(wrapCode(code)) === code.🔧 Suggested fix to test actual roundtrip
it("should handle wrapCode/unwrapCode roundtrip correctly", () => { const code = "inputs.title"; const wrapped = wrapCode(code); expect(wrapped).toContain("return"); expect(wrapped).toContain(code); + + // Import unwrapCode or replicate the unwrap logic to test roundtrip + const prefix = "\n (function() {\n return "; + const suffix = "\n })\n "; + const unwrapped = wrapped.slice(prefix.length, -suffix.length); + expect(unwrapped).toBe(code); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/packages/ast/src/index.test.ts` around lines 868 - 875, The test name claims a wrapCode/unwrapCode roundtrip but only checks wrapCode; update the test to actually assert the roundtrip by calling unwrapCode(wrapCode(code)) and expecting it to equal the original code (use wrapCode and unwrapCode symbols in the same it block), or if you prefer to keep the simpler check, rename the test to "should wrap code with return statement" to reflect it only verifies wrapCode; make the change inside the existing test that references wrapCode/unwrapCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/client/packages/ast/src/index.test.ts`:
- Around line 868-875: The test name claims a wrapCode/unwrapCode roundtrip but
only checks wrapCode; update the test to actually assert the roundtrip by
calling unwrapCode(wrapCode(code)) and expecting it to equal the original code
(use wrapCode and unwrapCode symbols in the same it block), or if you prefer to
keep the simpler check, rename the test to "should wrap code with return
statement" to reflect it only verifies wrapCode; make the change inside the
existing test that references wrapCode/unwrapCode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 453c42c6-492f-44e0-a43e-b61d86e9fcc2
📒 Files selected for processing (2)
app/client/packages/ast/src/index.test.tsapp/client/packages/ast/src/index.ts
Description
When a UI module containing a custom widget is instantiated in an app, the
inputsreferences inside the custom widget'sdefaultModelproperty are not refactored. For example,inputs.titleshould becomeCommons1.inputs.titlebut remains unchanged.Root cause: The
entityRefactorFromCodefunction in the AST package passes raw binding content (which may have leading whitespace/newlines) directly towrapCodewithout sanitization.wrapCodewraps code as(function() { return ${code} }). When the binding content starts with a newline followed by{(common in custom widgetdefaultModelwhich contains multi-line object literals), JavaScript's Automatic Semicolon Insertion (ASI) rule inserts a semicolon afterreturn. This causes{to be parsed as a block statement instead of an object literal, resulting in aSyntaxError. The error is silently swallowed inAstServiceCEImpl.refactorNameInDynamicBindings, leaving the binding unrefactored.Fix: Trim leading whitespace from the script in
entityRefactorFromCodebefore passing it towrapCode, so{lands on the same line asreturn(preventing ASI). The leading whitespace is restored after unwrapping to preserve the original content. This fix is localized toentityRefactorFromCode—wrapCodeitself is unchanged because other consumers (linting, ActionCreator, etc.) pass multi-statement evaluation scripts through it, and those would break if the return expression were parenthesized. Also replaces the magic-number slicing inunwrapCode(flagged as tech-debt) with named constants.Fixes #32841
Fixes https://linear.app/appsmith/issue/APP-15087/bug-ast-fails-to-refactor-if-moustache-binding-starts-with-a-new-line
TDD verification:
isSuccess: false)entityRefactorFromCodewrapCodeconsumerswrapCodeoutput is identical to originalAutomation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/24177924705
Commit: 76a1a67
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 09 Apr 2026 09:10:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
Tests
Refactor