-
Notifications
You must be signed in to change notification settings - Fork 4.1k
chore: Add util methods in WidgetRefactorUtil class #40320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Add util methods in WidgetRefactorUtil class #40320
Conversation
WalkthroughA new utility method was introduced in the Changes
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/widgets/refactors/WidgetRefactorUtil.java (4)
217-221
: Handle null inputs or annotate parameter
The public API currently delegates to the recursive helper, which handlesnull
by early returning an empty set—but the public method itself doesn’t guard or document this behavior. Consider adding an explicit null check at the start or annotating thedsl
parameter with@NotNull
/@Nullable
to clarify the contract.
217-221
: Add JavaDoc to describe behavior and edge cases
As a new public utility, please include a concise JavaDoc covering:
- Purpose of the method
- Parameter expectations (e.g., null handling)
- Return value semantics (empty set if no widget names)
223-235
: Use constant and guard against non-array children
Rather than hard‑coding"children"
, useFieldName.CHILDREN
. Also, verify that the node is anArrayNode
before iterating to avoid potential NPEs if the JSON shape changes.
Apply this diff:- if (dsl.has("children")) { - dsl.get("children").forEach(child -> extractWidgetNamesRecursive(child, widgetNames)); - } + if (dsl.has(FieldName.CHILDREN) && + dsl.get(FieldName.CHILDREN).isArray()) { + ((ArrayNode) dsl.get(FieldName.CHILDREN)) + .forEach(child -> extractWidgetNamesRecursive(child, widgetNames)); + }
223-235
: Add unit tests for nested and edge cases
We need coverage for:
- Nested
children
structures- Missing or empty
widgetName
fields- Null or malformed DSL inputs
Would you like me to draft JUnit tests for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/widgets/refactors/WidgetRefactorUtil.java
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/widgets/refactors/WidgetRefactorUtil.java (2)
217-221
: New method looks solid
The recursion-based approach cleanly collects widget names without side effects on existing logic.
223-235
: Recursive helper is correct
TheextractWidgetNamesRecursive
logic is straightforward and efficient for typical DSL sizes.
Description
EE Counterpart: https://github.com/appsmithorg/appsmith-ee/pull/7176
Fixes #
Issue Number
or
Fixes
Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14562403965
Commit: 0f601bb
Cypress dashboard.
Tags:
@tag.Sanity
Spec:
Sun, 20 Apr 2025 19:32:18 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit