fix(ui): prevent transaction mismatch when linking blueprint - WF-558 #973
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (BuilderSettingsHandlersBlueprint.vue)
participant Actions as useComponentActions
participant Core as Core/Store
UI->>Actions: createAndInsertComponentsTree(parentId, componentsTreeData)
loop For each component in componentsTreeData
Actions->>Core: createComponent(type, initProperties, parent)
end
Actions->>Core: commit mutation transaction
Actions->>UI: return [componentIds]
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ui/src/renderer/useFieldsErrors.spec.ts (1)
162-171: Good defensive test with a minor typo.This test correctly verifies that the hook handles missing component definitions gracefully. However, there's a typo in the test name.
- it("should handle unknow component", () => { + it("should handle unknown component", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/ui/src/builder/settings/BuilderSettingsHandlersBlueprint.vue(2 hunks)src/ui/src/builder/useComponentAction.spec.ts(4 hunks)src/ui/src/builder/useComponentActions.ts(4 hunks)src/ui/src/renderer/useFieldsErrors.spec.ts(1 hunks)src/ui/src/renderer/useFieldsErrors.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: tests (firefox)
- GitHub Check: tests (chromium)
- GitHub Check: tests (webkit)
- GitHub Check: build (3.13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
🔇 Additional comments (9)
src/ui/src/builder/useComponentActions.ts (4)
193-195: Clean type alias definition.The
CreateComponentPropstype alias improves readability and maintainability by clearly defining what properties can be provided when creating components.
209-209: Good use of the new type alias.Using
CreateComponentPropsfor theinitPropertiesparameter makes the function signature cleaner and more maintainable.
245-263: Verify the parent chaining logic for component trees.The logic that chains components together (making each component the parent of the next) appears correct, but ensure this creates the intended tree structure for your use case.
The implementation assumes a linear parent-child chain where:
- Component 0 parent = provided
parentId- Component 1 parent = Component 0's ID
- Component 2 parent = Component 1's ID
- etc.
Is this the intended tree structure, or should there be support for more complex branching?
1110-1110: Correctly added to exports.The new function is properly exported, making it available to consumers of the
useComponentActionshook.src/ui/src/renderer/useFieldsErrors.ts (1)
37-37: Excellent defensive programming improvement.Adding optional chaining prevents runtime errors when
getComponentDefinitionreturnsundefinedfor unknown components. This change aligns perfectly with the new test case and improves application robustness.src/ui/src/builder/settings/BuilderSettingsHandlersBlueprint.vue (2)
27-28: Updated import to use the new tree creation function.Correctly imports the new
createAndInsertComponentsTreefunction alongside the existingremoveComponentsSubtree.
73-97: Excellent usage of the new tree creation function.This change demonstrates the primary benefit of the new function - creating both the blueprint and trigger components in a single transaction, which should resolve the transaction mismatch issue mentioned in the PR title. The code is much cleaner and more maintainable than the previous approach.
src/ui/src/builder/useComponentAction.spec.ts (2)
8-14: Clean refactoring of test variables.The renaming from
core/ssbmtomockCore/wfbmimproves consistency and clarity in the test setup.
218-241: Comprehensive test for the new tree creation function.This test properly verifies that:
- Components are created in the correct parent-child relationship
- The transaction can be undone, confirming transactional integrity
- All created components are properly cleaned up during undo
The test coverage for the new functionality is excellent.
| const transactionId = `create-${componentIds.join("-")}`; | ||
| ssbm.openMutationTransaction(transactionId, `Create`); | ||
| for (const component of components) { | ||
| wf.addComponent(component); | ||
| repositionHigherSiblings(component.id, 1); | ||
| ssbm.registerPostMutation(component); | ||
| tracking?.track( | ||
| component.type.startsWith("blueprints_") | ||
| ? "blueprints_block_added" | ||
| : "ui_block_added", | ||
| { componentId: component.id }, | ||
| ); | ||
| } | ||
| ssbm.closeMutationTransaction(transactionId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for component creation failures.
The function doesn't handle potential failures when adding components or during transaction operations. Consider adding try-catch blocks to ensure transaction integrity.
const transactionId = `create-${componentIds.join("-")}`;
ssbm.openMutationTransaction(transactionId, `Create`);
+try {
for (const component of components) {
wf.addComponent(component);
repositionHigherSiblings(component.id, 1);
ssbm.registerPostMutation(component);
tracking?.track(
component.type.startsWith("blueprints_")
? "blueprints_block_added"
: "ui_block_added",
{ componentId: component.id },
);
}
ssbm.closeMutationTransaction(transactionId);
+} catch (error) {
+ ssbm.closeMutationTransaction(transactionId);
+ throw error;
+}📝 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.
| const transactionId = `create-${componentIds.join("-")}`; | |
| ssbm.openMutationTransaction(transactionId, `Create`); | |
| for (const component of components) { | |
| wf.addComponent(component); | |
| repositionHigherSiblings(component.id, 1); | |
| ssbm.registerPostMutation(component); | |
| tracking?.track( | |
| component.type.startsWith("blueprints_") | |
| ? "blueprints_block_added" | |
| : "ui_block_added", | |
| { componentId: component.id }, | |
| ); | |
| } | |
| ssbm.closeMutationTransaction(transactionId); | |
| const transactionId = `create-${componentIds.join("-")}`; | |
| ssbm.openMutationTransaction(transactionId, `Create`); | |
| try { | |
| for (const component of components) { | |
| wf.addComponent(component); | |
| repositionHigherSiblings(component.id, 1); | |
| ssbm.registerPostMutation(component); | |
| tracking?.track( | |
| component.type.startsWith("blueprints_") | |
| ? "blueprints_block_added" | |
| : "ui_block_added", | |
| { componentId: component.id }, | |
| ); | |
| } | |
| ssbm.closeMutationTransaction(transactionId); | |
| } catch (error) { | |
| // Ensure the transaction is closed on error | |
| ssbm.closeMutationTransaction(transactionId); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In src/ui/src/builder/useComponentActions.ts around lines 267 to 280, the code
lacks error handling for failures during component addition and transaction
operations. Wrap the entire mutation transaction block in a try-catch statement
to catch any errors thrown during wf.addComponent, repositionHigherSiblings,
ssbm.registerPostMutation, or tracking calls. In the catch block, ensure to call
ssbm.closeMutationTransaction to properly close the transaction and maintain
integrity, then rethrow or handle the error appropriately.
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 9d8f9c5 (latest)
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 3e129de (latest)
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 3f70fa7 (latest)
Summary by CodeRabbit