fix: page or block save update breadcrumb incorrectly #820
Conversation
1. 修复页面名称更新时，面包屑页名称没有同步更新的 bug 2. 修复区块保存更新时，强行切换当前画布到正在编辑区块的 bug 3. 修复区块保存更新时，如果画布不是当前编辑的区块，仍取截图的 bug
WalkthroughThe changes introduced in this pull request primarily enhance the functionality of page and block management within the application. Key modifications include the addition of a new parameter to the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments not posted (1)
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: 1
🧹 Outside diff range and nitpick comments (5)
packages/controller/js/http.js (1)
64-64: Approve change and consider performance impact.Removing the condition for
pageSettingState.updateTreeData()ensures that the tree data is always updated, which aligns with the PR objectives to fix synchronization issues. This change should resolve the problem of the header bar page name not updating after a page name modification.However, consider the performance impact if
updateTreeData()is an expensive operation. If it is, you might want to add a comment explaining why it's necessary to call this function unconditionally.packages/plugins/page/src/PageSetting.vue (1)
198-200: LGTM! Consider adding a null check for robustness.The addition of the
isCurEditPageparameter tohandlePageUpdateaddresses the issue of synchronizing the breadcrumb page name when it's changed. This change aligns well with the PR objectives and should resolve the problem described in issue #782.For improved robustness, consider adding a null check:
-const isCurEditPage = pageState?.currentPage?.id === id +const isCurEditPage = pageState?.currentPage?.id ? pageState.currentPage.id === id : falseThis change ensures that
isCurEditPageis always a boolean, even ifpageStateorcurrentPageis undefined.packages/plugins/block/src/js/blockSetting.jsx (3)
662-669: Enhance block update logic to synchronize canvasThe changes improve the block update process by synchronizing the canvas when the current block is being edited. This addresses the issue mentioned in the PR objectives where the UI wasn't reflecting changes accurately.
However, there are a couple of points to consider:
- The variable
currentIdis declared but never used. It can be directly used in the condition.- The condition could be simplified for better readability.
Consider applying this refactor to improve code clarity:
setSaved(true) - const currentId = useBlock().getCurrentBlock()?.id - - // 如果是当前正在编辑的区块，需要同步更新画布 - if (currentId === id) { + // 如果是当前正在编辑的区块，需要同步更新画布 + if (useBlock().getCurrentBlock()?.id === id) { useBlock().initBlock(data, {}, true) }
Line range hint
636-681: Consider enhancing error handling and code organizationWhile the changes address the main issue, there are opportunities to improve the overall function:
Error Handling: The catch block only displays an error message. Consider adding more robust error handling, such as logging the error or providing a way to retry the operation.
Code Organization: The function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions for better maintainability.
Consistency: Ensure consistent use of template literals for string interpolation throughout the function.
Here's a suggestion for improved error handling:
.catch((error) => { console.error('Failed to update block:', error); message({ message: `Failed to save block: ${error.message}`, status: 'error' }); // Optionally, set a flag or update state to indicate failed save setSaved(false); })Consider extracting the logic for updating the canvas and generating the block service into separate functions to improve code organization.
Line range hint
1-681: Consider additional improvements to enhance block managementWhile the current changes address the immediate issue, there are opportunities to further improve the block management functionality:
Breadcrumb Update: The PR mentions a bug where the breadcrumb page name didn't update synchronously. Consider implementing a function to update the breadcrumb when the page name changes.
Screenshot Capture: The PR mentions an issue with screenshot capture. Review the
getBlockBase64function (lines 249-265) to ensure it only captures screenshots for the currently edited block.Block Switching: To address the issue of forced canvas switching, review the
initEditBlockfunction (lines 284-293) and ensure it doesn't unnecessarily switch the canvas when updating block data.Performance Optimization: Consider implementing debounce or throttle mechanisms for functions that may be called frequently, such as
saveBlockorupdateBlock.Unit Tests: Add unit tests for the new and modified functions to ensure reliability and prevent regressions.
To implement these improvements:
- Create a new function
updateBreadcrumb(blockName)and call it after successful block updates.- Modify
getBlockBase64to check if the block being captured is the currently edited one.- Review and refactor
initEditBlockto avoid unnecessary canvas switches.- Implement debounce/throttle using a utility library or custom implementation.
- Add unit tests for
updateBlock,saveBlock, and any new functions you create.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/controller/js/http.js (3 hunks)
- packages/plugins/block/src/BlockSetting.vue (2 hunks)
- packages/plugins/block/src/js/blockSetting.jsx (1 hunks)
- packages/plugins/page/src/PageSetting.vue (1 hunks)
🔇 Additional comments not posted (6)
packages/controller/js/http.js (3)
17-17: LGTM: New import added correctly.The import for
useBreadcrumbis properly added and aligns with the new functionality inhandlePageUpdate.
71-74: LGTM: New conditional block addresses breadcrumb update issue.The new conditional block correctly uses the
isCurEditPageparameter to update the breadcrumb only when the current page is being edited. This change directly addresses the issue of breadcrumbs not updating when the page name changes, as mentioned in the PR objectives.
Line range hint
17-75: Summary: Changes effectively address PR objectives.The modifications in this file successfully tackle the issues mentioned in the PR summary and linked issue #782. The changes to
handlePageUpdatefunction, including the new parameter and conditional logic, should resolve the synchronization problems with page names and breadcrumbs. The implementation looks solid, with no major issues detected.packages/plugins/block/src/BlockSetting.vue (2)
81-81: LGTM: New import added correctly.The addition of
useBlockto the import statement is consistent with the existing code style and is necessary for the changes made in theexecfunction.
215-219: LGTM: Improved screenshot capture logic.The changes effectively address the issue of unnecessary screenshot captures. By checking if the current block matches the block being processed, we ensure that screenshots are only taken for the active block. This optimization improves performance and aligns with the PR objectives.
Some benefits of this change:
- Prevents redundant screenshot captures.
- Improves overall performance of the block update process.
- Ensures that only relevant screenshots are saved.
packages/plugins/page/src/PageSetting.vue (1)
198-200: Verify the breadcrumb update functionalityThe changes introduced here are minimal and focused on addressing the synchronization issue between the page name and the breadcrumb. To ensure the fix works as intended:
- Test the page name update functionality, especially when editing the current page.
- Verify that the breadcrumb updates correctly in real-time when the page name is changed.
- Check for any potential side effects in related components that might rely on the
handlePageUpdatefunction.To assist with verification, you can run the following script to find all occurrences of
handlePageUpdatein the codebase:This will help identify any other components that might be affected by the change and ensure they're updated if necessary.
✅ Verification successful
Breadcrumb update functionality verified successfully
The changes in
PageSetting.vueare localized and do not affect other components. ThehandlePageUpdatefunction remains unchanged and its usage in other files is unaffected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all occurrences of handlePageUpdate rg --type vue --type js 'handlePageUpdate'Length of output: 70
Script:
#!/bin/bash # Search for all occurrences of handlePageUpdate in .vue and .js files rg --glob '*.vue' --glob '*.js' 'handlePageUpdate'Length of output: 621
chilingling
changed the title
* fix: page or block save update breadcrumb uncorrectly 1. 修复页面名称更新时，面包屑页名称没有同步更新的 bug 2. 修复区块保存更新时，强行切换当前画布到正在编辑区块的 bug 3. 修复区块保存更新时，如果画布不是当前编辑的区块，仍取截图的 bug * fix: add handlePageUpdate mehtod missing params
* fix: page or block save update breadcrumb uncorrectly 1. 修复页面名称更新时，面包屑页名称没有同步更新的 bug 2. 修复区块保存更新时，强行切换当前画布到正在编辑区块的 bug 3. 修复区块保存更新时，如果画布不是当前编辑的区块，仍取截图的 bug * fix: add handlePageUpdate mehtod missing params
2 participants
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
What is the current behavior?
Issue Number: close #782
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation