fix: blockPublishLogic #973
Conversation
|
Warning Rate limit exceeded@chilingling has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces significant changes to block deployment and management across multiple components. The primary modifications involve replacing 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: 1
🧹 Nitpick comments (4)
packages/common/component/BlockDeployDialog.vue (1)
144-160: Incrementing version might need stricter validation.
getNextVersionincrements the patch number, but doesn't validate if the existing version follows the X.Y.Z pattern (e.g., it might break on unusual version strings like "1.0-alpha"). Consider a more robust parser if your workflow needs advanced versioning.+ // Example approach: parse the version with a library like semver, or manually split by dots.packages/toolbars/breadcrumb/src/Main.vue (1)
75-91:nextVersionlogic is commented out.
Leaving the old versioning approach commented may cause confusion. If no longer needed, consider removing it and relying onBlockDeployDialog.vue’s approach.packages/plugins/block/src/BlockSetting.vue (2)
158-173: Commented out version logic.
Similar toMain.vue, remove if it’s fully deprecated. Leaving large chunks of commented code can confuse future maintainers.
280-280:nextVersionstill commented out.
Reiterating: consider removing or documenting purpose if you plan to restore it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/common/component/BlockDeployDialog.vue(6 hunks)packages/plugins/block/src/BlockSetting.vue(4 hunks)packages/plugins/block/src/composable/useBlock.js(1 hunks)packages/toolbars/breadcrumb/src/Main.vue(2 hunks)packages/toolbars/breadcrumb/src/composable/useBreadcrumb.js(1 hunks)
🔇 Additional comments (17)
packages/common/component/BlockDeployDialog.vue (7)
3-3: Use of the visible prop is correct.
Good switch from $attrs.visible to a dedicated visible prop for clarity and maintainability.
83-83: Check if separate module import is necessary.
While importing useNotify is valid, confirm whether useNotify is used in production code or only for planned notifications. If unused, consider removing it to reduce bundle size.
99-105: Well-defined props for block and visible.
Declaring default values ensures component stability when these props are not passed.
108-109: New emits are consistent with usage.
Emitting 'update:visible' and 'changeSchema' aligns well with your new prop approach for toggling dialog visibility and broadcasting schema changes.
162-162: Watch handler for visible looks correct.
When the dialog is shown, the version is updated using getNextVersion. Ensure external calls do not also modify formState.version, to avoid overwriting.
Also applies to: 164-164, 167-167, 168-168
197-197: Guard against missing block or id.
changeCompare relies on block.id; consider graceful handling if block is empty or missing an id.
Also applies to: 199-199, 204-204, 213-213
236-238: Emitting changeSchema is efficient.
emit('changeSchema', { ...newSchema, componentName: COMPONENT_NAME.Block }) neatly decouples updating from the dialog’s own logic. Looks good.
packages/toolbars/breadcrumb/src/composable/useBreadcrumb.js (1)
26-27: Simplified breadcrumb logic.
Removing the histories param aligns with the updated data flow. Confirm that no other calls still expect the second argument.
packages/toolbars/breadcrumb/src/Main.vue (4)
27-31: New block-deploy-dialog props and listeners match refactored API.
Passing currentBlock and listening for @changeSchema integrate well with the new design.
39-39: Additional imports for useBlock and useCanvas.
Ensure that you only import what you use, to minimize overhead. If all are currently in use, this is fine.
98-103: currentBlock and handleChangeSchema are well-structured.
Computing the current block and applying schema changes signals a clean separation of data retrieval and update logic.
108-108: Return references are sensible for setup().
Exposing currentBlock and handleChangeSchema helps maintain a cohesive interface for the parent or sibling components.
Also applies to: 110-112
packages/plugins/block/src/BlockSetting.vue (4)
78-82: Dialog now uses block instead of nextVersion.
This refactor centralizes data handling in the dialog, streamlining version pick logic. Nice improvement.
88-88: Extended import for useCanvas.
This suggests new canvas interactions. Confirm you’re also removing any leftover references to older APIs if not needed.
270-275: handleChangeSchema method to optionally re-import.
Good check for matching block IDs before calling importSchema. Minimal overhead and reduces unintended overwrites.
292-293: Consistent show/hide guide toggling.
These changes keep state management consistent for UI toggles. Straightforward and clear.
packages/plugins/block/src/composable/useBlock.js (1)
290-290: Removed histories argument from setBreadcrumbBlock.
Ensure no references remain to the old signature. Otherwise, the breadcrumb data will be incomplete or cause runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/plugins/block/src/js/blockSetting.jsx (1)
423-432: Consider swappingblock.contentwithnewBlock.contentfor consistency.Currently,
useHistory().addHistory(block.content)on line 426 stores the old content rather than the freshly-fetched content fromnewBlock. If the goal is to preserve and track new changes, consider updating to:- useHistory().addHistory(block.content) + useHistory().addHistory(newBlock.content)packages/common/component/BlockDeployDialog.vue (2)
83-83: Confirm all imports fromtiny-engine-meta-registerare necessary.If any imports are unused after these changes, consider removing them to keep the codebase clean.
180-180: Encapsulateis_compilein a named constant or config if used frequently.This helps readability and maintainability, especially if more compile options are introduced later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/common/component/BlockDeployDialog.vue(6 hunks)packages/plugins/block/src/BlockSetting.vue(4 hunks)packages/plugins/block/src/js/blockSetting.jsx(1 hunks)packages/toolbars/breadcrumb/src/Main.vue(2 hunks)
🔇 Additional comments (15)
packages/toolbars/breadcrumb/src/Main.vue (3)
27-31: Block-deploy-dialog usage looks good.
Props and event handlers are well-defined, ensuring that the dialog receives the correct block data and emits changeSchema when needed.
39-39: Consolidate or verify imports if unused.
Double-check that all imported features from useBreadcrumb, useLayout, and useBlock are needed in this file, especially if partial usage is removed in the refactor.
80-85: Validate initBlock usage.
handleChangeSchema invokes initBlock directly. Ensure that downstream logic (e.g., watchers or history recording) is triggered as expected after the block is replaced.
packages/common/component/BlockDeployDialog.vue (8)
3-3: Visibility prop integration is correct.
Using :visible="visible" and @update:visible fosters clear, two-way binding. It aligns with typical Vue patterns.
99-105: Transition from nextVersion to block prop is clear.
This update improves clarity by using an explicit block reference rather than relying on an inferred next version.
108-109: New emits look consistent.
Emitting update:visible and changeSchema is aligned with your needs for external visibility control and schema updates.
144-159: Ensure fallback for missing or invalid date fields in getNextVersion.
If v.created_at is missing or has an invalid date, this logic may fail or produce an unexpected NaN comparison. Consider adding safety checks or fallback behavior.
162-168: Good approach to compute the next version upon dialog opening.
Recomputing formState.version in the watch ensures consistency when the dialog becomes visible. Confirm any complexity in the watch won't cause stale data if multiple re-openings occur.
176-177: Refactor suggestion about lacking error handling remains valid.
This line calls publishBlock(params) without awaiting or dedicated error handling. A past reviewer already noted this:
"Consider handling publish success or failure…"
197-204: (Optional) Validate remote result in changeCompare.
When getBlockById returns undefined, remote?.content may cause problems. Double-check for potential edge cases when the block ID is invalid.
227-228: Emitting changeSchema is direct and effective.
Emitting the new schema here is a clean way to let parent components decide how to integrate or import it. The code looks good.
packages/plugins/block/src/BlockSetting.vue (4)
78-82: Updated prop usage in <block-deploy-dialog> is aligned with the revised interface.
Replacing :nextVersion with :block="publishBlock" consistently builds on the new prop-based design.
88-88: Consolidated imports are fine.
Confirm that all imported APIs (useModal, getMergeMeta, useBlock) are necessary and used.
142-151: Publish block selection logic is sound.
publishBlock returns the canvas block if the edit block matches the current block; otherwise, it returns the edit block. This ensures that the updated data flows into the publish dialog.
262-268: Schema update logic is straightforward and consistent.
handleChangeSchema correctly reinitializes the block if it's the currently edited one. You might consider verifying that all references to the old schema have been replaced.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/toolbars/breadcrumb/src/Main.vue (1)
80-84: Add error handling for schema updates.While the implementation is functional, consider adding error handling to make it more robust:
- The schema update could potentially fail
- The current block might be undefined in some cases
Consider applying this improvement:
const currentBlock = computed(useBlock().getCurrentBlock) const handleChangeSchema = (newSchema) => { + try { + const block = useBlock().getCurrentBlock() + if (!block) { + console.warn('No block is currently selected') + return + } useBlock().initBlock({ ...useBlock().getCurrentBlock(), content: newSchema }) + } catch (error) { + console.error('Failed to update block schema:', error) + // Consider adding user feedback here + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/breadcrumb/src/Main.vue(2 hunks)
🔇 Additional comments (4)
packages/toolbars/breadcrumb/src/Main.vue (4)
27-31: LGTM! Block deployment dialog changes align with PR objectives.
The updated block-deploy-dialog component now correctly handles the current block state and schema changes, which addresses the PR's goal of fixing the block publishing logic.
39-39: LGTM! Required import added for block functionality.
The addition of useBlock import is necessary and consistent with the module's dependencies.
91-93: LGTM! Return statement properly exposes required properties.
The return statement correctly includes all necessary properties for the component's functionality.
27-31: Verify usage of block-deploy-dialog across the codebase.
Let's ensure that all instances of block-deploy-dialog have been updated to use the new props and handle the schema changes consistently.
Also applies to: 80-84
✅ Verification successful
block-deploy-dialog implementation is consistent across the codebase
The verification shows that:
- There are only two instances of
block-deploy-dialogusage in the codebase:packages/toolbars/breadcrumb/src/Main.vuepackages/plugins/block/src/BlockSetting.vue
- Both implementations use identical props and event handlers:
v-model:visiblefor dialog visibility:blockprop for passing block data@changeSchemaevent handler for schema updates
- The component's emits and event handling in
BlockDeployDialog.vuealign with both usages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of block-deploy-dialog to verify consistent usage
rg -l "block-deploy-dialog" --type vue
# Search for potential direct calls to initBlock that might need similar error handling
ast-grep --pattern 'useBlock().initBlock($$$)'
Length of output: 412
Script:
#!/bin/bash
# Search for block-deploy-dialog with correct file extension
rg -l "block-deploy-dialog" --type-add 'vue:*.vue' --type vue
# Get the context around block-deploy-dialog usage
rg "block-deploy-dialog" --type-add 'vue:*.vue' --type vue -B2 -A2
# Check for changeSchema event handlers
rg "changeSchema" --type-add 'vue:*.vue' --type vue -B2 -A2
Length of output: 3049
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: N/A
What is the new behavior?
【新改动】
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes