fix: generate unique widgetIds when cloning page#41762
fix: generate unique widgetIds when cloning page#41762amelia-c0n wants to merge 6 commits intoreleasefrom
Conversation
WalkthroughAdds widget-id regeneration: new secure 10-character id generator and a DSL-wide Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant PageService as ApplicationPageService
participant DslUtils as DslUtils
participant JsonObj as JSONObject (DSL)
participant DB as Database
Client->>PageService: clonePage(sourcePageId)
PageService->>DB: fetch source page layouts
DB-->>PageService: source layouts
loop for each layout
PageService->>JsonObj: parse layout DSL
PageService->>DslUtils: regenerateWidgetIds(dsl)
activate DslUtils
DslUtils->>JsonObj: deep-copy DSL
DslUtils->>DslUtils: collect widgetIds (exclude "0") and generate new IDs
DslUtils->>JsonObj: recursively rewrite id references (keys ending with "Id")
DslUtils-->>PageService: regenerated DSL
deactivate DslUtils
PageService->>PageService: set regenerated DSL on clone
end
PageService->>DB: save cloned page with new widget IDs
DB-->>PageService: success
PageService-->>Client: return cloned page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (1)
944-953: Optional: tighten the clone assertions to also check id format.Currently the test only ensures the cloned ids are
!= "firstWidget"/!= "Table1". A stricter check (e.g.,matches("[0-9a-z]{10}")) would catch regressions where clone leaves the id untouched after a future refactor but still happens to differ by some unrelated mutation. Unit tests inDslUtilsTestalready cover format, so this is purely belt-and-braces.♻️ Suggested tightening
- assertThat(clonedDsl.get("widgetId")).isNotEqualTo("firstWidget"); + assertThat(clonedDsl.get("widgetId")) + .isNotEqualTo("firstWidget") + .asString() + .matches("[0-9a-z]{10}"); @@ - assertThat(clonedChild.get("widgetId")).isNotEqualTo("Table1"); + assertThat(clonedChild.get("widgetId")) + .isNotEqualTo("Table1") + .asString() + .matches("[0-9a-z]{10}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java` around lines 944 - 953, Tighten the clone assertions in PageServiceTest by validating the regenerated widgetId formats (not just inequality): after retrieving unpublishedPage.getLayouts().get(0).getDsl() as clonedDsl and its child clonedChild, assert that clonedDsl.get("widgetId") and clonedChild.get("widgetId") match the expected id pattern (e.g., a regex like "[0-9a-z]{10}" used elsewhere in DslUtilsTest) rather than merely checking != "firstWidget"/"Table1", so the test ensures both uniqueness and correct id format on clone.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java (1)
124-132: Minor: graceful-fallback returns the source reference, not a copy.If
JSONValue.parse(...)produces something other than aJSONObject,deepCopyreturns the input directly.regenerateWidgetIdswould then mutate the caller's DSL viarewriteWidgetIdReferences. In practice this path is unreachable for valid DSL (so this is a nitpick), but you could make the fallback safer by returningsourceDslat the caller site (i.e., bail out beforerewriteWidgetIdReferences) or by cloning the map manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java` around lines 124 - 132, The fallback in deepCopy currently returns the original JSONObject reference which allows callers like regenerateWidgetIds -> rewriteWidgetIdReferences to mutate the caller's DSL; change the behavior so deepCopy never returns the original reference: when parsed is not a JSONObject, create and return a new JSONObject clone (e.g., construct a new JSONObject from the original content or its string form) instead of returning source, or alternatively have regenerateWidgetIds bail out early when deepCopy fails; update deepCopy (and call sites such as regenerateWidgetIds/rewriteWidgetIdReferences if you choose the bail-out approach) so the caller always works with a copy, not the original reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java`:
- Around line 74-91: collectWidgetIds only recurses into FieldName.CHILDREN and
therefore misses widgets under list templates (FieldName.LIST_WIDGET_TEMPLATE),
causing IDs there not to be regenerated; update collectWidgetIds to also
traverse the LIST_WIDGET_TEMPLATE field (and any List/Map nested structures
similar to rewriteWidgetIdReferences) by checking for
FieldName.LIST_WIDGET_TEMPLATE (and/or iterating all Map/List entries) and
recursing into JSONObject entries so template widgets get their IDs added to
oldToNewWidgetIdMap (use the same guards as existing code: skip empty IDs,
MAIN_CONTAINER_WIDGET_ID, and already-mapped IDs, and call generateWidgetId()
when adding).
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java`:
- Around line 124-132: The fallback in deepCopy currently returns the original
JSONObject reference which allows callers like regenerateWidgetIds ->
rewriteWidgetIdReferences to mutate the caller's DSL; change the behavior so
deepCopy never returns the original reference: when parsed is not a JSONObject,
create and return a new JSONObject clone (e.g., construct a new JSONObject from
the original content or its string form) instead of returning source, or
alternatively have regenerateWidgetIds bail out early when deepCopy fails;
update deepCopy (and call sites such as
regenerateWidgetIds/rewriteWidgetIdReferences if you choose the bail-out
approach) so the caller always works with a copy, not the original reference.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java`:
- Around line 944-953: Tighten the clone assertions in PageServiceTest by
validating the regenerated widgetId formats (not just inequality): after
retrieving unpublishedPage.getLayouts().get(0).getDsl() as clonedDsl and its
child clonedChild, assert that clonedDsl.get("widgetId") and
clonedChild.get("widgetId") match the expected id pattern (e.g., a regex like
"[0-9a-z]{10}" used elsewhere in DslUtilsTest) rather than merely checking !=
"firstWidget"/"Table1", so the test ensures both uniqueness and correct id
format on clone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4e29f1e-3414-4961-9b16-464e91b1adc1
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/DslUtilsTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java
| private static void collectWidgetIds(JSONObject widget, Map<String, String> oldToNewWidgetIdMap) { | ||
| Object widgetIdValue = widget.get(FieldName.WIDGET_ID); | ||
| if (widgetIdValue instanceof String widgetId | ||
| && !widgetId.isEmpty() | ||
| && !MAIN_CONTAINER_WIDGET_ID.equals(widgetId) | ||
| && !oldToNewWidgetIdMap.containsKey(widgetId)) { | ||
| oldToNewWidgetIdMap.put(widgetId, generateWidgetId()); | ||
| } | ||
|
|
||
| Object children = widget.get(FieldName.CHILDREN); | ||
| if (children instanceof List<?> childrenList) { | ||
| for (Object child : childrenList) { | ||
| if (child instanceof JSONObject childObject) { | ||
| collectWidgetIds(childObject, oldToNewWidgetIdMap); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Hunt for widget DSL shapes where widget nodes live outside "children".
rg -nP --type=ts -C2 '\btemplate\s*:\s*\{' -g '!**/node_modules/**' -g '**/widgets/**'
rg -nP --type=ts -C2 '"widgetId"\s*:' -g '!**/node_modules/**' -g '**/widgets/**/*.json'
rg -nP --type=ts -C2 '\b(tabsObj|template|mainCanvas)\b' -g '!**/node_modules/**' -g '**/widgets/ListWidget*/**'Repository: appsmithorg/appsmith
Length of output: 50376
🏁 Script executed:
cat -n app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java | head -120Repository: appsmithorg/appsmith
Length of output: 6048
🏁 Script executed:
# Look at the test JSON files to see if they contain "template" field with widgets
grep -l "template" app/server/appsmith-server/src/test/resources/com/appsmith/server/widgets/refactors/*.json | head -3Repository: appsmithorg/appsmith
Length of output: 434
🏁 Script executed:
# Check if test cases use or validate the collectWidgetIds function
rg "collectWidgetIds" app/server/appsmith-server/src/test/java --type=java -A 5Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Examine one of the test JSON files for template structure
head -300 app/server/appsmith-server/src/test/resources/com/appsmith/server/widgets/refactors/refactorDslWithOnlyWidgets.json | tail -150Repository: appsmithorg/appsmith
Length of output: 5052
🏁 Script executed:
# Search for tests of DslUtils in test directory
find app/server/appsmith-server/src/test -name "*DslUtils*" -type fRepository: appsmithorg/appsmith
Length of output: 151
🏁 Script executed:
# Look for tests that verify regenerateWidgetIds
rg -l "regenerateWidgetIds" app/server/appsmith-server/src/test --type=javaRepository: appsmithorg/appsmith
Length of output: 151
🏁 Script executed:
cat app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/DslUtilsTest.java | head -250Repository: appsmithorg/appsmith
Length of output: 11395
🏁 Script executed:
# Search for "template" in server-side test resources/code
rg -i "template" app/server/appsmith-server/src/test --type=java | head -20Repository: appsmithorg/appsmith
Length of output: 3662
🏁 Script executed:
# Check if ListWidget uses template on server-side
rg -i "template" app/server/appsmith-server/src/main --type=java | head -20Repository: appsmithorg/appsmith
Length of output: 3570
🏁 Script executed:
# Find the FieldName class and check for LIST_WIDGET_TEMPLATE
rg -A 2 "LIST_WIDGET_TEMPLATE" app/server/appsmith-server/src/main --type=javaRepository: appsmithorg/appsmith
Length of output: 1000
🏁 Script executed:
# Check how template widgets are handled in regenerateWidgetIds or if there's a separate method
rg -B 5 -A 10 "LIST_WIDGET_TEMPLATE" app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.javaRepository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check if regenerateWidgetIds is called with template handling elsewhere
rg "regenerateWidgetIds" app/server/appsmith-server/src/main --type=java -B 3 -A 3Repository: appsmithorg/appsmith
Length of output: 2164
🏁 Script executed:
# Check what calls regenerateWidgetIds to understand the flow
rg -B 5 -A 8 "regenerateWidgetIds" app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.javaRepository: appsmithorg/appsmith
Length of output: 893
🏁 Script executed:
# Look for any test that validates template widget handling in DSL
rg -A 20 "regenerateWidgetIds" app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/DslUtilsTest.javaRepository: appsmithorg/appsmith
Length of output: 6416
collectWidgetIds misses nested widget trees under non-children keys — List widget template field contains widgets with IDs that won't be collected.
The traversal asymmetry causes data integrity issues: collectWidgetIds only recurses through FieldName.CHILDREN and skips the template field (defined as "template" in FieldName.LIST_WIDGET_TEMPLATE). This means widgets nested under List1.template.{WidgetName} won't have their IDs collected or regenerated during page cloning. However, rewriteWidgetIdReferences traverses all nested structures (Maps and Lists), so it will still attempt to rewrite any references to those uncollected template widget IDs elsewhere in the DSL. The result is an inconsistent state where template widgets retain source IDs while references to them are updated, causing potential collisions or dangling pointers.
The test suite lacks coverage for this scenario — all DslUtilsTest cases use only the children structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java`
around lines 74 - 91, collectWidgetIds only recurses into FieldName.CHILDREN and
therefore misses widgets under list templates (FieldName.LIST_WIDGET_TEMPLATE),
causing IDs there not to be regenerated; update collectWidgetIds to also
traverse the LIST_WIDGET_TEMPLATE field (and any List/Map nested structures
similar to rewriteWidgetIdReferences) by checking for
FieldName.LIST_WIDGET_TEMPLATE (and/or iterating all Map/List entries) and
recursing into JSONObject entries so template widgets get their IDs added to
oldToNewWidgetIdMap (use the same guards as existing code: skip empty IDs,
MAIN_CONTAINER_WIDGET_ID, and already-mapped IDs, and call generateWidgetId()
when adding).
Failed server tests
|
1 similar comment
Failed server tests
|
amelia-c0n
changed the title
Failed server tests
|
1 similar comment
Failed server tests
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java (1)
74-91:⚠️ Potential issue | 🟠 MajorTraversal still skips non-
childrenwidget containers (e.g. List widgettemplate, TabstabsObj).
collectWidgetIdsonly descends throughFieldName.CHILDREN, so widgets nested undertemplate(ListWidgetV2) ortabsObj(Tabs) never make it intooldToNewWidgetIdMap. However,rewriteWidgetIdReferenceshappily walks those subtrees and will rewrite any*Idfields it finds — producing an inconsistent state where template/tab-internal widgets keep the sourcewidgetIdwhileparentId/mainCanvasIdreferences to them may get rewritten (or dangle against the parent page).At minimum, consider mirroring
rewriteWidgetIdReferences' generic traversal insidecollectWidgetIds(walk any nestedMap/ListforwidgetIdfields), or explicitly recurse intoFieldName.LIST_WIDGET_TEMPLATEand other known widget-bearing keys. Please also add a dedicated test covering a List widget with populatedtemplate.🛠️ Suggested generalised traversal
private static void collectWidgetIds(JSONObject widget, Map<String, String> oldToNewWidgetIdMap) { Object widgetIdValue = widget.get(FieldName.WIDGET_ID); if (widgetIdValue instanceof String widgetId && !widgetId.isEmpty() && !MAIN_CONTAINER_WIDGET_ID.equals(widgetId) && !oldToNewWidgetIdMap.containsKey(widgetId)) { oldToNewWidgetIdMap.put(widgetId, generateWidgetId()); } - Object children = widget.get(FieldName.CHILDREN); - if (children instanceof List<?> childrenList) { - for (Object child : childrenList) { - if (child instanceof JSONObject childObject) { - collectWidgetIds(childObject, oldToNewWidgetIdMap); - } - } - } + // Recurse into every nested object/array so widget trees nested under + // non-"children" keys (e.g. List widget template, Tabs tabsObj) are covered. + for (Object value : widget.values()) { + collectNestedWidgetIds(value, oldToNewWidgetIdMap); + } + } + + private static void collectNestedWidgetIds(Object node, Map<String, String> oldToNewWidgetIdMap) { + if (node instanceof JSONObject obj) { + collectWidgetIds(obj, oldToNewWidgetIdMap); + } else if (node instanceof List<?> list) { + for (Object element : list) { + collectNestedWidgetIds(element, oldToNewWidgetIdMap); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java` around lines 74 - 91, collectWidgetIds currently only recurses into FieldName.CHILDREN, missing widgets nested under other keys (e.g. List widget FieldName.LIST_WIDGET_TEMPLATE or Tabs FieldName.TABS_OBJ) and causing inconsistent ID rewrites; update collectWidgetIds to mirror rewriteWidgetIdReferences' traversal by descending into any nested Map/JSONObject and List/JSONArray it encounters and collecting widget.get(FieldName.WIDGET_ID) values (skipping MAIN_CONTAINER_WIDGET_ID and empty IDs) so all embedded widgets are mapped, explicitly include known container keys like FieldName.LIST_WIDGET_TEMPLATE and FieldName.TABS_OBJ if you prefer explicit recursion, and add a unit test that creates a List widget with a populated template to assert IDs in the template are included in oldToNewWidgetIdMap.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java (1)
130-138: Optional:JSONObject.toJSONString()+JSONValue.parse()is a pricey deep-copy on large DSLs.Works fine, but on sizeable pages this is O(N) serialization + O(N) parse + full object re-allocation. If clone-page latency ever shows up in traces, a structural copy that walks
Map/Listdirectly would be materially cheaper. Leaving as-is for now is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java` around lines 130 - 138, The current deepCopy method in deepCopy(JSONObject) uses JSONObject.toJSONString() + JSONValue.parse(), which is expensive for large DSLs; replace this serialization round-trip with a structural recursive copy that walks JSONObject/JSONArray (and Map/List primitives) and clones entries and elements to new JSONObject/JSONArray objects to avoid O(N) serialize/parse and extra allocation overhead. Update the deepCopy implementation to detect JSONValue types and recursively copy Maps (JSONObject), Lists/JSONArray and primitive values, returning a newly constructed JSONObject copy (or the original fallback only on irrecoverable types), keeping the method name deepCopy and using the same return type so callers are unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java`:
- Around line 74-91: collectWidgetIds currently only recurses into
FieldName.CHILDREN, missing widgets nested under other keys (e.g. List widget
FieldName.LIST_WIDGET_TEMPLATE or Tabs FieldName.TABS_OBJ) and causing
inconsistent ID rewrites; update collectWidgetIds to mirror
rewriteWidgetIdReferences' traversal by descending into any nested
Map/JSONObject and List/JSONArray it encounters and collecting
widget.get(FieldName.WIDGET_ID) values (skipping MAIN_CONTAINER_WIDGET_ID and
empty IDs) so all embedded widgets are mapped, explicitly include known
container keys like FieldName.LIST_WIDGET_TEMPLATE and FieldName.TABS_OBJ if you
prefer explicit recursion, and add a unit test that creates a List widget with a
populated template to assert IDs in the template are included in
oldToNewWidgetIdMap.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java`:
- Around line 130-138: The current deepCopy method in deepCopy(JSONObject) uses
JSONObject.toJSONString() + JSONValue.parse(), which is expensive for large
DSLs; replace this serialization round-trip with a structural recursive copy
that walks JSONObject/JSONArray (and Map/List primitives) and clones entries and
elements to new JSONObject/JSONArray objects to avoid O(N) serialize/parse and
extra allocation overhead. Update the deepCopy implementation to detect
JSONValue types and recursively copy Maps (JSONObject), Lists/JSONArray and
primitive values, returning a newly constructed JSONObject copy (or the original
fallback only on irrecoverable types), keeping the method name deepCopy and
using the same return type so callers are unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ad95f6d-e6d7-447c-a53f-3a95afb95d9a
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/DslUtilsTest.java
Failed server tests
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (1)
944-958: Consider tightening the regenerated-id assertions.Right now the test only proves the cloned ids are not the literal sentinels
"firstWidget"/"Table1". That bar is fairly low — a regression that, say, blanks out the id, returnsnull, or accidentally reuses the same regenerated value for parent and child would still slip past. Since the whole point of this PR is "unique widget ids in the documented client format", it would be worth asserting:
- both ids are non-blank and match the 10-char lowercase-alphanumeric format produced by
DslUtils.generateWidgetId(), and- the parent's
widgetIdis not equal to the child'swidgetId(uniqueness within the cloned DSL).♻️ Suggested extra assertions
Object clonedChildren = clonedDsl.get("children"); assertThat(clonedChildren).isInstanceOf(List.class); // After persisting the cloned page and reading it back, nested DSL nodes // come back as LinkedHashMap rather than net.minidev.json.JSONObject, so we // assert against the generic Map contract instead of casting to JSONObject. Object firstChild = ((List<?>) clonedChildren).get(0); assertThat(firstChild).isInstanceOf(Map.class); Map<?, ?> clonedChild = (Map<?, ?>) firstChild; assertThat(clonedChild.get("widgetName")).isEqualTo("Table1"); assertThat(clonedChild.get("widgetId")).isNotEqualTo("Table1"); + // Regenerated ids should match the client id format (10 lowercase alphanumerics) + // and be unique within the cloned DSL. + assertThat((String) clonedDsl.get("widgetId")).matches("[a-z0-9]{10}"); + assertThat((String) clonedChild.get("widgetId")).matches("[a-z0-9]{10}"); + assertThat(clonedDsl.get("widgetId")).isNotEqualTo(clonedChild.get("widgetId"));Bonus points if you also flip the source DSL's seed
widgetIds (lines 815, 822) to actual 10-char lookalikes — keeps the fixture honest with the production format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java` around lines 944 - 958, Tighten the cloned-id assertions: instead of only asserting clonedDsl.widgetId != "firstWidget" and clonedChild.widgetId != "Table1", assert both cloned IDs are non-blank, match the 10-char lowercase-alphanumeric pattern produced by DslUtils.generateWidgetId(), and assert the parent id (clonedDsl.get("widgetId")) is not equal to the child id (clonedChild.get("widgetId")) to guarantee uniqueness within the cloned DSL; also consider updating the source fixture widgetId seeds to realistic 10-char lookalikes so tests better reflect production format.
🤖 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/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java`:
- Around line 944-958: Tighten the cloned-id assertions: instead of only
asserting clonedDsl.widgetId != "firstWidget" and clonedChild.widgetId !=
"Table1", assert both cloned IDs are non-blank, match the 10-char
lowercase-alphanumeric pattern produced by DslUtils.generateWidgetId(), and
assert the parent id (clonedDsl.get("widgetId")) is not equal to the child id
(clonedChild.get("widgetId")) to guarantee uniqueness within the cloned DSL;
also consider updating the source fixture widgetId seeds to realistic 10-char
lookalikes so tests better reflect production format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85949881-b18f-44db-9abc-1baa4a641ab2
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/24983989304. |
|
Deploy-Preview-URL: https://ce-41762.dp.appsmith.com |
1 participant
Description
Problem
The Clone Page feature copied the source page's DSL verbatim, so every widget in the cloned page kept the exact same widgetId as its counterpart on the source page. This produced duplicate widget ids at the application level, causing some small bugs.
Fix
Regenerate all widget IDs in the cloned page's DSL as part of the server-side clone flow, while preserving internal references so the cloned page remains structurally identical to the source.
Changes
DslUtils.regenerateWidgetIds(JSONObject)(app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/DslUtils.java)Deep-copies the source DSL (source is never mutated).
Phase 1 — walks the widget tree via children, building an oldId → newId map for every widget.
Phase 2 — id-blind rewrite: recursively visits every map entry and list element and replaces any string equal to an old id with its new id, so references anywhere in the DSL stay consistent (including non-children locations like Tabs' tabsObj and List V2's mainCanvasId).
Leaves the root MainContainer (widgetId: "0") untouched — it is a well-known fixed id that children reference.
DslUtils.generateWidgetId()Produces a 10-char lowercase alphanumeric id using RandomStringUtils.secure() (SecureRandom-backed), matching the format and id space of the client's generateReactKey (nanoid) so cloned widgets are indistinguishable from client-created ones.
ApplicationPageServiceCEImpl.clonePageGivenApplicationIdCalls
DslUtils.regenerateWidgetIds(layout.getDsl())when building each new Layout for the cloned page. Single-line change at the clone site; no other flows touched.Tests
generateWidgetId format (length, alphabet) and distinctness across 1000 calls.
regenerateWidgetIds for null/empty DSL, MainContainer preservation, deep reference rewriting (nested parentId chains + non-children id references), source non-mutation, no duplicate ids across a 20-widget tree, and empty-children tree.
Scope and safety
How to verify
{{Container1.widgetId}}. Note the displayed id.Fixes https://github.com/appsmithorg/appsmith-ee/issues/8933
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 30118ce yet
Thu, 30 Apr 2026 07:12:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests