fix(block): fix block group info missing when edit block & fix filed block_id is undefined when change bock version; #1056
Conversation
WalkthroughThis pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
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: 0
🧹 Nitpick comments (2)
packages/plugins/block/src/js/blockSetting.jsx (2)
401-413: Add error handling for the API call.While the function correctly handles the category/group logic and sorting, it should include error handling for the API call to ensure robustness and provide feedback to users when the refresh fails.
export const updateBlockList = () => { let params = useBlock().shouldReplaceCategoryWithGroup() ? { groupId: currentCategory.value } : { categoryId: currentCategory.value } if (!currentCategory.value) { params = {} } const appId = getAppId() - fetchBlockList({ appId, ...params }).then((data) => { - const blockListDescByUpdateAt = data.sort((a, b) => new Date(b.updated_at) - new Date(a.updated_at)) - useBlock().setBlockList(blockListDescByUpdateAt) - }) + fetchBlockList({ appId, ...params }) + .then((data) => { + const blockListDescByUpdateAt = data.sort((a, b) => new Date(b.updated_at) - new Date(a.updated_at)) + useBlock().setBlockList(blockListDescByUpdateAt) + }) + .catch((error) => { + useNotify({ + type: 'error', + message: '刷新区块列表失败', + description: error.message + }) + }) }
535-537: Consider removing redundant refresh mechanism.The function uses both
updateBlockList()andisRefresh.value = truefor refresh. SinceupdateBlockList()already triggers a reactive update, theisRefreshflag might be redundant.- updateBlockList() - useBlock().isRefresh.value = true + updateBlockList()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/block/src/Main.vue(2 hunks)packages/plugins/block/src/js/blockSetting.jsx(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/plugins/block/src/js/blockSetting.jsx (2)
56-56: LGTM! Well-structured state management implementation.The reactive variable and its setter function are implemented following Vue's best practices.
Also applies to: 279-281
Line range hint
414-433: LGTM! Proper list refresh implementation after deletion.The function correctly refreshes the block list after successful deletion and includes proper error handling and user feedback.
packages/plugins/block/src/Main.vue (1)
351-352: LGTM! Clean and focused implementation.The changeCategory method is well-refactored to be more focused and maintainable, properly utilizing the new state management functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
169-169: Consolidate repeated refresh triggers.
isRefresh.valueis already set totrueearlier in the same flow. Using it twice in quick succession may be redundant and could potentially cause confusion. Consider removing or clarifying one of the triggers if it’s not needed.packages/plugins/block/src/BlockConfig.vue (1)
170-175: Handle empty groups or categories gracefully.If
block.groupsorblock.categoriesis empty, the fallback logic may result in an unexpected empty array or undefined identifier. Consider adding a default or a guard condition to prevent potential assignment errors.packages/plugins/block/src/js/blockSetting.jsx (3)
279-281: Check for invalid or empty category IDs.
setCurrentCategorysetscurrentCategory.valuewithout validation. IfcategoryIdis null or invalid, subsequent calls toupdateBlockListmay yield unexpected results. Consider adding a guard if needed.
537-538: Unify post-publish refresh logic.Similarly calling
updateBlockList()andisRefresh.value = trueensures the UI is in sync. Confirm all relevant create/update flows mirror this logic for uniform user experience.
724-724: Invoke “isRefresh” for uniformity.
updateBlockList()is called after new or updated blocks are saved. If other flows also triggerisRefresh.value = true, consider whether to apply the same pattern here to avoid partial states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/common/component/BlockHistoryList.vue(1 hunks)packages/plugins/block/src/BlockConfig.vue(4 hunks)packages/plugins/block/src/js/blockSetting.jsx(9 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (9)
packages/common/component/BlockHistoryList.vue (1)
2-2: Ensure that the “row-id” is indeed unique.Adding
row-id="id"helps the grid track rows by their unique identifier, which is important for row-level operations. However, please verify thatidvalues are truly unique across the data set to avoid potential rendering or state mismatch issues.packages/plugins/block/src/BlockConfig.vue (3)
41-41: Confirm that multiple selection is desired.By dynamically setting
:multiple="shouldReplaceCategoryWithGroup()", you switch between single and multiple category/group selection. Ensure that users will not be confused when the dropdown behavior changes based on this condition.
246-250: Use descriptive property keys for clarity.Using
'id'or'category_id'depending onshouldReplaceCategoryWithGroup()is correct. However, ensure that developers easily recognize which key is active and handle potential edge cases (e.g., an empty selection).
291-291: Propagating the condition is consistent.Exposing
shouldReplaceCategoryWithGroupin the returned setup object is logical for reusability across templates. The addition looks clean.packages/plugins/block/src/js/blockSetting.jsx (5)
13-13: Importing “ref” is necessary.Good catch integrating
reffrom Vue. This aligns with the newly introduced reactive variables below.
56-56: Initialize the reactive category to a known state.Defining
currentCategoryas an empty string by default is acceptable. Be mindful of how your watchers handle this initial empty state and whether any early calls might cause unintended requests.
401-413: Ensure robust error handling for “updateBlockList.”This function properly sets parameters based on
shouldReplaceCategoryWithGroup(). However, ifupdated_atis missing or invalid, the sort might fail. Consider verifying the field or adding a fallback before sorting.
427-428: Maintain consistency with other refresh mechanisms.Calling
updateBlockList()followed byisRefresh.value = trueis consistent with the approach in other methods like publishBlock. This ensures the UI remains in sync after block deletion.
659-659: Consider matching other operations’ patterns.While updating a block sets
isRefresh.value = true, other operations also callupdateBlockList(). Decide if an immediate block list refresh is needed here for consistency (or rely solely on watchers).
…iny-engine into fix/block-refresh
…meter should be a array when create a new block;
…hange of block version
betterdancing
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (1)
81-81: LGTM! Consider adding a comment explaining the fallback logic.The use of the nullish coalescing operator to determine
blockIdimproves robustness by providing a fallback torefId. This change helps ensure consistent block management.Consider adding a comment explaining why we fall back to
refId:+ // Fall back to refId if block_id is undefined to handle legacy or migrated blocks blockId: selectedRow.block_id ?? selectedRow.refId,Let's verify that this change properly integrates with the block refresh functionality:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other usages of block_id and refId to ensure consistent handling rg -A 3 'block_id|refId' --type vue --type jsxLength of output: 76
I'll re-run the check without restricting to file types (since “vue” isn’t recognized) to see if there are any other usages of
block_idorrefIdthat might affect consistency with the block refresh functionality:Let's review the output from this command before finalizing our analysis.
🏁 Script executed:
#!/bin/bash # Search across the repository for usages of block_id and refId rg -A 3 'block_id|refId'Length of output: 695
LGTM! The use of the nullish coalescing operator is appropriate and the fallback to
refIdis consistent with other usages found.The search across the repository confirmed that this pattern is only applied in the Vue component (and does not conflict with other contexts such as the JSON data), so the change properly integrates with block refresh functionality. For clarity, consider adding an inline comment:
+ // Fall back to refId if block_id is not available (ensuring compatibility with legacy/migrated blocks) blockId: selectedRow.block_id ?? selectedRow.refId,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/plugins/block/src/SaveNewBlock.vue (1)
64-69: 🛠️ Refactor suggestionInitialize group as an array when multiple selections are enabled.
When
shouldReplaceCategoryWithGroup()is true and multiple selections are enabled, initializinggroupas an empty string could lead to type mismatches.Update the initialization:
const formData = reactive({ label: '', path: 'default', name_cn: '', - group: '' + group: shouldReplaceCategoryWithGroup() ? [] : '' })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/block/src/SaveNewBlock.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/plugins/block/src/SaveNewBlock.vue (1)
90-94:❓ Verification inconclusive
Verify block creation with multiple groups.
Please verify that block creation works correctly when multiple groups are selected, especially focusing on:
- The block_id generation
- The persistence of all selected groups
- The proper handling of the path parameter
🏁 Script executed:
#!/bin/bash # Description: Check block creation implementation and group handling # Search for block creation and update logic echo "Searching for block creation implementation..." rg -A 10 "createBlock|updateBlock" --type js # Search for block_id handling echo "Searching for block_id usage..." rg "block_id" --type jsLength of output: 25152
Verify multi-group block creation and plugin activation behavior.
Please ensure the following during verification:
- Verify that block creation correctly generates and propagates the block_id.
- Confirm that when multiple groups are selected, all groups persist in the block's configuration.
- Check that the path parameter is properly handled during block creation.
- Revisit the rationale for activating the Materials plugin (via
activePlugin(PLUGIN_NAME.Materials)) after creating a block to ensure this behavior is intentional and does not introduce side-effects.
… delete;
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
修改仅针对java版本，nodejs版本无影响，对于nodejs版本而言区块分类字段是categories且是单选，而java版本改成了区块分组，使用的字段是groups且是多选，无论创建还是修改都要求传递参数groups为数组格式，因此在这个文件里做一个转换
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit