Skip to content

Commit 5743047

Browse files
authored
fix: prevent ASI in wrapCode causing refactoring to fail (#41727)
## Description When a UI module containing a custom widget is instantiated in an app, the `inputs` references inside the custom widget's `defaultModel` property are not refactored. For example, `inputs.title` should become `Commons1.inputs.title` but remains unchanged. **Root cause:** The `entityRefactorFromCode` function in the AST package passes raw binding content (which may have leading whitespace/newlines) directly to `wrapCode` without sanitization. `wrapCode` wraps code as `(function() { return ${code} })`. When the binding content starts with a newline followed by `{` (common in custom widget `defaultModel` which contains multi-line object literals), JavaScript's Automatic Semicolon Insertion (ASI) rule inserts a semicolon after `return`. This causes `{` to be parsed as a block statement instead of an object literal, resulting in a `SyntaxError`. The error is silently swallowed in `AstServiceCEImpl.refactorNameInDynamicBindings`, leaving the binding unrefactored. **Fix:** Trim leading whitespace from the script in `entityRefactorFromCode` before passing it to `wrapCode`, so `{` lands on the same line as `return` (preventing ASI). The leading whitespace is restored after unwrapping to preserve the original content. This fix is localized to `entityRefactorFromCode` — `wrapCode` itself 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 in `unwrapCode` (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:** | Phase | Result | |-------|--------| | Red: multi-line object literal test | Failed as expected (`isSuccess: false`) | | Green: applied localized fix in `entityRefactorFromCode` | All 73 tests across 4 AST suites pass | | No regression to `wrapCode` consumers | `wrapCode` output is identical to original | ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/24177924705> > Commit: 76a1a67 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=24177924705&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 09 Apr 2026 09:10:21 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Tests** * Added comprehensive test coverage for code entity refactoring functionality. * **Refactor** * Improved code wrapping logic with named constants for better maintainability. * Enhanced preservation of whitespace formatting in code transformations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 306f25f commit 5743047

2 files changed

Lines changed: 95 additions & 13 deletions

File tree

app/client/packages/ast/src/index.test.ts

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
import { parseJSObject } from "../index";
1+
import { parseJSObject, entityRefactorFromCode } from "../index";
22
import {
33
extractIdentifierInfoFromCode,
44
getMemberExpressionObjectFromProperty,
55
isFunctionPresent,
6+
wrapCode,
67
} from "../src/index";
78

89
describe("getAllIdentifiers", () => {
@@ -795,3 +796,80 @@ describe("getMemberExpressionObjectFromProperty", () => {
795796
}
796797
});
797798
});
799+
800+
describe("entityRefactorFromCode", () => {
801+
it("should refactor a simple identifier reference", () => {
802+
const script = "inputs.title";
803+
const result = entityRefactorFromCode(
804+
script,
805+
"inputs",
806+
"Commons1.inputs",
807+
false,
808+
2,
809+
);
810+
811+
expect(result.isSuccess).toBe(true);
812+
const body = result.body as { script: string; refactorCount: number };
813+
814+
expect(body.script).toBe("Commons1.inputs.title");
815+
expect(body.refactorCount).toBe(1);
816+
});
817+
818+
it("should refactor inputs in a multi-line object literal (custom widget defaultModel)", () => {
819+
// This is the exact pattern from a custom widget's defaultModel inside a UI module.
820+
// The binding content starts with a newline followed by {, which triggers
821+
// JavaScript's ASI after 'return' in wrapCode, causing a SyntaxError.
822+
const script = `\n {\n title: inputs.title,\n fields: inputs.fields,\n }\n`;
823+
const result = entityRefactorFromCode(
824+
script,
825+
"inputs",
826+
"Commons1.inputs",
827+
false,
828+
2,
829+
);
830+
831+
expect(result.isSuccess).toBe(true);
832+
const body = result.body as { script: string; refactorCount: number };
833+
834+
expect(body.script).toContain("Commons1.inputs.title");
835+
expect(body.script).toContain("Commons1.inputs.fields");
836+
expect(body.refactorCount).toBe(2);
837+
});
838+
839+
it("should refactor inputs in an inline object literal", () => {
840+
const script = "{ title: inputs.title, fields: inputs.fields }";
841+
const result = entityRefactorFromCode(
842+
script,
843+
"inputs",
844+
"Commons1.inputs",
845+
false,
846+
2,
847+
);
848+
849+
expect(result.isSuccess).toBe(true);
850+
const body = result.body as { script: string; refactorCount: number };
851+
852+
expect(body.script).toContain("Commons1.inputs.title");
853+
expect(body.script).toContain("Commons1.inputs.fields");
854+
expect(body.refactorCount).toBe(2);
855+
});
856+
857+
it("should refactor entity names in standard widget bindings", () => {
858+
const script = "Api1.data";
859+
const result = entityRefactorFromCode(script, "Api1", "NewApi1", false, 2);
860+
861+
expect(result.isSuccess).toBe(true);
862+
const body = result.body as { script: string; refactorCount: number };
863+
864+
expect(body.script).toBe("NewApi1.data");
865+
expect(body.refactorCount).toBe(1);
866+
});
867+
868+
it("should handle wrapCode/unwrapCode roundtrip correctly", () => {
869+
const code = "inputs.title";
870+
const wrapped = wrapCode(code);
871+
872+
expect(wrapped).toContain("return");
873+
expect(wrapped).toContain(code);
874+
});
875+
});

app/client/packages/ast/src/index.ts

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -350,20 +350,15 @@ const isArrayAccessorNode = (node: Node): node is MemberExpressionNode => {
350350
);
351351
};
352352

353+
const WRAP_CODE_PREFIX = "\n (function() {\n return ";
354+
const WRAP_CODE_SUFFIX = "\n })\n ";
355+
353356
export const wrapCode = (code: string) => {
354-
return `
355-
(function() {
356-
return ${code}
357-
})
358-
`;
357+
return `${WRAP_CODE_PREFIX}${code}${WRAP_CODE_SUFFIX}`;
359358
};
360359

361-
//Tech-debt: should upgrade this to better logic
362-
//Used slice for a quick resolve of critical bug
363360
const unwrapCode = (code: string) => {
364-
const unwrapedCode = code.slice(32);
365-
366-
return unwrapedCode.slice(0, -10);
361+
return code.slice(WRAP_CODE_PREFIX.length, -WRAP_CODE_SUFFIX.length);
367362
};
368363

369364
const getFunctionalParamNamesFromNode = (
@@ -486,8 +481,17 @@ export const entityRefactorFromCode = (
486481
//Sanitizing leads to removal of special charater.
487482
//Hence we are not sanatizing the script. Fix(#18492)
488483
//If script is a JSObject then replace export default to decalartion.
484+
// Trim leading whitespace before wrapping to prevent ASI after 'return'.
485+
// When code starts with a newline before '{' (e.g., multi-line object literals
486+
// in custom widget defaultModel), JS inserts a semicolon after 'return',
487+
// making '{' parse as a block statement instead of an object literal.
488+
let leadingWhitespace = "";
489+
489490
if (isJSObject) script = jsObjectToCode(script);
490-
else script = wrapCode(script);
491+
else {
492+
leadingWhitespace = script.match(/^(\s*)/)?.[0] ?? "";
493+
script = wrapCode(script.trimStart());
494+
}
491495

492496
let ast: Node = { end: 0, start: 0, type: "" };
493497
//Copy of script to refactor
@@ -572,7 +576,7 @@ export const entityRefactorFromCode = (
572576

573577
//If script is a JSObject then revert decalartion to export default.
574578
if (isJSObject) refactorScript = jsCodeToObject(refactorScript);
575-
else refactorScript = unwrapCode(refactorScript);
579+
else refactorScript = leadingWhitespace + unwrapCode(refactorScript);
576580

577581
return {
578582
isSuccess: true,

0 commit comments

Comments
 (0)