chore: decoupled property pane registration #40851
Conversation
WalkthroughThis update shifts the registration of property control builders from a synchronous process during editor initialization to an asynchronous, dynamic import within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PropertyPane
participant PropertyControlRegistry
User->>PropertyPane: Mounts component
PropertyPane->>PropertyControlRegistry: Dynamically import with retry
PropertyControlRegistry-->>PropertyPane: Module loaded
PropertyPane->>PropertyControlRegistry: registerPropertyControlBuilders()
PropertyControlRegistry-->>PropertyPane: Registration complete
PropertyPane->>User: Render property pane
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
github-actions
bot
added
the
skip-changelog
![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 (1)
app/client/src/pages/Editor/PropertyPane/index.tsx (1)
54-54: Consider adding loading indicator for better UX.The early return logic correctly prevents rendering until registration completes. However, users might experience a blank state without feedback during the async loading process.
Consider showing a loading spinner or skeleton during the registration phase to improve user experience.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/PropertyPane/index.tsx(2 hunks)app/client/src/utils/editor/EditorUtils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/utils/editor/EditorUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/pages/Editor/PropertyPane/index.tsx (2)
1-1: Good addition of useState import for the new state management.The import change correctly adds useState to support the registration tracking state.
9-9: Appropriate utility import for retry mechanism.Adding retryPromise import supports the resilient async loading pattern.
| // Track registration state | ||
| const [controlsRegistered, setControlsRegistered] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| const loadPropertyControlBuilders = async () => { | ||
| const module = await retryPromise( | ||
| async () => | ||
| import( | ||
| /* webpackChunkName: "PropertyControlRegistry" */ "utils/PropertyControlRegistry" | ||
| ), | ||
| ); | ||
|
|
||
| module.default.registerPropertyControlBuilders(); | ||
|
|
||
| setControlsRegistered(true); | ||
| }; | ||
|
|
||
| loadPropertyControlBuilders(); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed property control registration.
The async loading implementation is well-structured with retry mechanism and webpack chunk naming. However, consider adding error handling for scenarios where registration fails completely.
useEffect(() => {
const loadPropertyControlBuilders = async () => {
- const module = await retryPromise(
- async () =>
- import(
- /* webpackChunkName: "PropertyControlRegistry" */ "utils/PropertyControlRegistry"
- ),
- );
-
- module.default.registerPropertyControlBuilders();
-
- setControlsRegistered(true);
+ try {
+ const module = await retryPromise(
+ async () =>
+ import(
+ /* webpackChunkName: "PropertyControlRegistry" */ "utils/PropertyControlRegistry"
+ ),
+ );
+
+ module.default.registerPropertyControlBuilders();
+ setControlsRegistered(true);
+ } catch (error) {
+ console.error("Failed to load property control builders:", error);
+ // Consider fallback behavior or user notification
+ }
};
loadPropertyControlBuilders();
}, []);📝 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.
| // Track registration state | |
| const [controlsRegistered, setControlsRegistered] = useState(false); | |
| useEffect(() => { | |
| const loadPropertyControlBuilders = async () => { | |
| const module = await retryPromise( | |
| async () => | |
| import( | |
| /* webpackChunkName: "PropertyControlRegistry" */ "utils/PropertyControlRegistry" | |
| ), | |
| ); | |
| module.default.registerPropertyControlBuilders(); | |
| setControlsRegistered(true); | |
| }; | |
| loadPropertyControlBuilders(); | |
| }, []); | |
| // Track registration state | |
| const [controlsRegistered, setControlsRegistered] = useState(false); | |
| useEffect(() => { | |
| const loadPropertyControlBuilders = async () => { | |
| try { | |
| const module = await retryPromise( | |
| async () => | |
| import( | |
| /* webpackChunkName: "PropertyControlRegistry" */ "utils/PropertyControlRegistry" | |
| ), | |
| ); | |
| module.default.registerPropertyControlBuilders(); | |
| setControlsRegistered(true); | |
| } catch (error) { | |
| console.error("Failed to load property control builders:", error); | |
| // Consider fallback behavior or user notification | |
| } | |
| }; | |
| loadPropertyControlBuilders(); | |
| }, []); |
🤖 Prompt for AI Agents
In app/client/src/pages/Editor/PropertyPane/index.tsx around lines 32 to 50, the
async function loading and registering property control builders lacks error
handling for failure scenarios. Wrap the await import and registration calls in
a try-catch block, and in the catch block, log or handle the error appropriately
to ensure failures are detected and managed. This will improve robustness by
handling cases where the module fails to load or registration fails.
2 participants
Description
Decoupled property registration code and lazily initialise it in the property pane component.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15461239247
Commit: 9c70387
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 05 Jun 2025 08:50:15 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes