fix: resetCanvasState indirectly called in useHistory, resets the isBlock status #1009
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Finishing Touches
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 (
|
github-actions
bot
added
bug
![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/toolbars/redoundo/src/composable/useHistory.js (1)
64-64: LGTM! Consider adding documentation.The addition of
trueas the second argument toimportSchemacorrectly preserves the canvas state during history navigation, fixing the issue withisBlockstatus being reset. However, the purpose of this parameter isn't immediately clear from the code.Consider adding a comment explaining why
trueis passed here, for example:- useCanvas().importSchema(string2Schema(list[historyState.index]), true) + // Pass true to preserve canvas state properties (e.g., isBlock) during history navigation + useCanvas().importSchema(string2Schema(list[historyState.index]), true)packages/canvas/DesignCanvas/src/api/useCanvas.js (1)
Line range hint
513-525: Enhance error handling in importSchema.The current error handling only logs to console. Consider providing more context and proper error propagation.
try { importData = JSON.parse(data) } catch (error) { - // eslint-disable-next-line no-console - console.error('[useCanvas.importSchema] Invalid data') + const message = '[useCanvas.importSchema] Failed to parse schema data' + console.error(message, error) + throw new Error(message) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/DesignCanvas/src/api/useCanvas.js(3 hunks)packages/toolbars/redoundo/src/composable/useHistory.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/canvas/DesignCanvas/src/api/useCanvas.js (2)
114-126: LGTM! Well-documented implementation.The addition of the
partialparameter toresetCanvasStateis well-implemented and properly documented. The conditional state reset logic effectively addresses the issue withisBlockstatus being reset during history navigation.
525-526: Add JSON schema validation.The comment "JSON 格式校验" suggests that JSON validation should be performed, but the implementation is missing.
Let's check if there's a schema validation utility in the codebase:
Would you like me to help implement the JSON schema validation for the canvas state?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/canvas/DesignCanvas/src/api/useCanvas.js (2)
563-563: Add unit tests for the new parameters.The changes to
resetCanvasStateandimportSchemaneed test coverage to ensure they maintain theisBlockstatus correctly.Would you like me to generate unit tests that verify:
resetCanvasStatepreservesisBlockwhenpartialis trueimportSchemacorrectly uses theresetPartialparameter- History navigation maintains the
isBlockstatus
563-563: Document the new parameters in JSDoc.The new parameters need to be documented to explain their purpose and behavior.
Add JSDoc comments for the modified functions:
+/** + * Resets the canvas state + * @param {Object} state - The new state to apply + * @param {boolean} partial - If true, preserves existing state values not present in the new state + */ const resetCanvasState = async (state = {}, partial = false) => { +/** + * Imports schema data into the canvas + * @param {Object|string} data - The schema data to import + * @param {boolean} resetPartial - If true, preserves existing state values during reset + */ const importSchema = (data, resetPartial = false) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/DesignCanvas/src/api/useCanvas.js(1 hunks)packages/toolbars/redoundo/src/composable/useHistory.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/toolbars/redoundo/src/composable/useHistory.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/canvas/DesignCanvas/src/api/useCanvas.js (1)
563-563: Update function calls to use the new parameters.Functions that call
resetCanvasStateorimportSchemaneed to be updated to use the new parameters correctly.Let's verify the usage of these functions:
gene9831
force-pushed
the
fix/history-reset-canvas-state
branch
from
adf6551 to
28a6508
Compare
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?
历史记录前进或者后退时，会重置全部画布数据，导致部分数据丢失。比如区块中历史记录前进或者后退，画布的
isBlock被重置成了false。下面例子中，画布顶部加了一个路由bar，只有在isBlock为false时才显示，但是回退后，出现了路由barWhat is the new behavior?
修改后
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Improvements