fix: fix materials panel #909
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (1)
packages/plugins/materials/src/meta/layout/src/Main.vue (1)
4-8: LGTM! Consider moving event handler to methods section.The implementation correctly adds the fix-panel functionality. However, for better maintainability, consider moving the event handler to the methods section.
<template> <component :is="headerComponent" :fixedPanels="fixedPanels" - @fix-panel="(id) => $emit('fix-panel', id)" + @fix-panel="handleFixPanel" ></component> </template> <script> export default { // ... existing code ... methods: { handleFixPanel(id) { this.$emit('fix-panel', id) } } } </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/plugins/materials/src/meta/layout/src/Main.vue(1 hunks)
🔇 Additional comments (1)
packages/plugins/materials/src/meta/layout/src/Main.vue (1)
4-8: Verify headerComponent implementation.
Let's verify that the headerComponent properly implements the fix-panel event.
✅ Verification successful
Header component implementation is correct
The header component properly implements the fix-panel event. The component:
- Correctly emits the 'fix-panel' event with the Materials plugin name as payload
- Accepts the required
fixedPanelsprop - Uses the event to toggle panel fixing functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for headerComponent implementation and its fix-panel event
ast-grep --pattern 'export default {
$$$
emits: [$$$"fix-panel"$$$]
$$$
}'
# Search for any fix-panel event emissions
rg "\\$emit\\(['\"]fix-panel['\"]" -g '!Main.vue'
Length of output: 205
Script:
#!/bin/bash
# Search for headerComponent definition/import
rg "headerComponent" -A 5 -B 5
# Search for fix-panel event handling with proper regex escaping
rg "\$emit\('fix-panel'" -g '!Main.vue'
rg "\$emit\(\"fix-panel\"" -g '!Main.vue'
# Find all Vue components that might be used as headerComponent
fd -e vue --exec grep -l "export default" {}
Length of output: 17026
Script:
#!/bin/bash
# Let's check the header component implementation
cat packages/plugins/materials/src/components/header/Main.vue
# Also check for any fix-panel event handling in the header component
rg -g "packages/plugins/materials/src/components/header/Main.vue" "fix-panel"
Length of output: 794
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/plugins/materials/src/components/header/Main.vue (2)
4-6: Consider internationalizing button tooltipsThe button tooltips are currently hardcoded in Chinese. Consider using i18n for better internationalization support.
- :tips="panelFixed ? '解除固定面板' : '固定面板'" + :tips="panelFixed ? t('unfixPanel') : t('fixPanel')"
Line range hint
20-23: Consider adding default value and type validation for fixedPanels propThe prop definition could be more robust with a default value and validation.
fixedPanels: { - type: Array + type: Array, + default: () => [], + validator: (value) => Array.isArray(value) }packages/plugins/tree/src/Main.vue (1)
78-83: LGTM! Consider adding TypeScript type safety.The implementation correctly handles panel state management and event emissions. The computed property is well-implemented with null-safe checks.
Consider adding TypeScript type safety to improve maintainability:
- props: { - fixedPanels: { - type: Array - } - }, + props: { + fixedPanels: { + type: Array as PropType<string[]>, + default: () => [] + } + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/design-core/assets/fixed-solid.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
packages/plugins/materials/src/components/header/Main.vue(2 hunks)packages/plugins/tree/src/Main.vue(3 hunks)
🔇 Additional comments (2)
packages/plugins/materials/src/components/header/Main.vue (1)
25-32: LGTM! Clean implementation of panel state logic
The implementation is well-structured and follows Vue 3 composition API best practices:
- Proper use of computed properties for reactive state
- Clean separation of concerns
- Correct handling of optional chaining for fixedPanels
packages/plugins/tree/src/Main.vue (1)
6-7: LGTM! Verify button state changes.
The button implementation correctly uses the computed panelFixed property to toggle between fixed/unfixed states, with appropriate visual feedback through icons and tooltips.
Let's verify the button state changes are working as expected:
✅ Verification successful
Button state changes are properly implemented and consistent
The implementation is verified to be working correctly:
- The
panelFixedcomputed property correctly derives its state fromfixedPanelsprop using the proper plugin identifier - The button click correctly emits the
fixPanelevent with the correct plugin name - The pattern is consistent with other plugins (e.g., Tutorial plugin) that use the same panel fixing mechanism
- The visual feedback (icon and tooltip) properly reflects the panel's fixed state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent panel state handling across components
# Look for similar panel state implementations
rg -A 3 'panelFixed.*\?' --type vue
# Check for proper event handling
rg -A 3 '@fix-panel|fixPanel' --type vue
Length of output: 136
Script:
#!/bin/bash
# Description: Check for consistent panel state handling across components using different file extensions
# Look for similar panel state implementations
rg -A 3 'panelFixed.*\?' -t js -t jsx -t ts -t tsx -t html
# Check for proper event handling
rg -A 3 '@fix-panel|fixPanel' -t js -t jsx -t ts -t tsx -t html
# Find all Vue-like files to determine correct extension
fd -e vue
Length of output: 16957
Script:
#!/bin/bash
# Now that we know the Vue files exist, let's search with the correct pattern
# Search for panel fixed state implementations
rg -U 'panelFixed.*\?' packages/plugins/tree/src/Main.vue
# Search for fix panel event handling
rg -U '@fix-panel|fixPanel|emits.*fixPanel' packages/plugins/tree/src/Main.vue
# Look for similar implementations in other plugin panels
rg -U 'panelFixed|fixPanel' packages/plugins/*/src/Main.vue
Length of output: 1407
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/layout/src/DesignPlugins.vue (1)
157-162: Consider extracting panel state persistence logic into a dedicated composableThe current implementation mixes panel state persistence directly in the component. This could be improved by:
- Creating a dedicated composable for panel state management
- Implementing proper state persistence patterns
- Adding migration capabilities for future state structure changes
Consider creating a new composable like this:
// useFixedPanelsState.ts import { ref, watch } from 'vue' const STORAGE_KEY = 'tiny-engine-fixed-panels' export function useFixedPanelsState() { const fixedPanels = ref<string[]>([]) const loadState = () => { try { const stored = localStorage.getItem(STORAGE_KEY) const parsed = stored ? JSON.parse(stored) : [] fixedPanels.value = Array.isArray(parsed) ? parsed : [] } catch (error) { console.warn('Failed to restore fixed panels state:', error) fixedPanels.value = [] } } const saveState = () => { try { localStorage.setItem(STORAGE_KEY, JSON.stringify(fixedPanels.value)) } catch (error) { console.warn('Failed to persist fixed panels state:', error) } } // Auto-save on changes watch(fixedPanels, saveState, { deep: true }) // Load initial state loadState() return { fixedPanels, togglePanel: (pluginName: string) => { const index = fixedPanels.value.indexOf(pluginName) if (index === -1) { fixedPanels.value.push(pluginName) } else { fixedPanels.value.splice(index, 1) } } } }This would simplify the component code and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/layout/src/DesignPlugins.vue (1)
160-165: Consider handling concurrent localStorage updatesWhile the current implementation works well for single-instance scenarios, consider implementing a pub/sub mechanism or using a state management solution (like Vuex/Pinia) to handle potential race conditions when multiple instances of the application try to update the fixed panels state simultaneously.
Also applies to: 168-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/layout/src/DesignPlugins.vue(3 hunks)
🔇 Additional comments (6)
packages/layout/src/DesignPlugins.vue (6)
77-77: LGTM: Import statement is correctly added
The addition of useModal import is appropriate for the new error handling functionality.
80-81: LGTM: Well-defined storage key constant
The constant is appropriately namespaced and follows naming conventions.
103-103: LGTM: Proper hook usage
Correctly destructured message from useModal hook following Vue Composition API practices.
182-182: LGTM: Proper initialization timing
The restoreFixedPanels method is correctly called during component setup.
160-165:
Fix template literal syntax in error message
The error message has incorrect quote usage that could lead to malformed output.
Apply this fix:
- message({ message: `'Failed to persist fixed panels state:'${error}`, status: 'error' })
+ message({ message: `Failed to persist fixed panels state: ${error}`, status: 'error' })Note: The implementation correctly follows the recommended approach for localStorage operations with proper serialization and error handling.
168-180:
Fix template literal syntax in error message
The error message has incorrect quote usage that could lead to malformed output.
Apply this fix:
- message({ message: `'Failed to restore fixed panels state:'${error}`, status: 'error' })
+ message({ message: `Failed to restore fixed panels state: ${error}`, status: 'error' })Note: The implementation correctly follows the recommended approach for localStorage initialization with proper parsing, type validation, and error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/utils/src/constants/index.js (1)
96-97: Add JSDoc documentation for the new constantConsider adding documentation to explain the purpose and usage of this constant.
+/** + * Storage key for persisting fixed panel states + * Used in conjunction with localStorage to maintain panel configurations across sessions + */ export const STORAGE_KEY_FIXED_PANELS = `tiny-engine-fixed-panels-${CHANNEL_UID}`packages/layout/src/DesignPlugins.vue (3)
161-166: Fix error message string formattingThe error message contains unnecessary quotes that will appear in the UI.
- message({ message: `'存储固定面板数据失败:'${error}`, status: 'error' }) + message({ message: `存储固定面板数据失败: ${error}`, status: 'error' })
169-181: Fix error message string formattingThe error message contains unnecessary quotes that will appear in the UI.
- message({ message: `'读取固定面板数据失败:'${error}`, status: 'error' }) + message({ message: `读取固定面板数据失败: ${error}`, status: 'error' })
183-183: Move localStorage initialization to onMounted hookFor better alignment with Vue's lifecycle hooks and to ensure proper handling of browser APIs, consider moving the initialization to onMounted:
+ import { reactive, ref, watch, onMounted } from 'vue' // ... other code ... - restoreFixedPanels() + onMounted(() => { + restoreFixedPanels() + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/design-core/assets/fixed-solid.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
packages/layout/src/DesignPlugins.vue(3 hunks)packages/utils/src/constants/index.js(1 hunks)
🔇 Additional comments (2)
packages/utils/src/constants/index.js (1)
96-97: LGTM! Verify usage in DesignPlugins.vue
The constant follows the established pattern of using CHANNEL_UID for unique storage keys.
Let's verify its usage in DesignPlugins.vue:
✅ Verification successful
LGTM! The constant is properly used in DesignPlugins.vue
The constant STORAGE_KEY_FIXED_PANELS is correctly imported and used in DesignPlugins.vue for:
- Storing fixed panel states via
localStorage.setItem - Retrieving stored panel states via
localStorage.getItem - Proper error handling is implemented for storage operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of STORAGE_KEY_FIXED_PANELS constant
# Expected: Should find usage in DesignPlugins.vue for localStorage operations
# Search for the constant usage
rg "STORAGE_KEY_FIXED_PANELS" -A 3
# Search for related localStorage operations in DesignPlugins.vue
fd -e vue -x rg "localStorage" -l {} \; | rg "DesignPlugins"
Length of output: 1372
packages/layout/src/DesignPlugins.vue (1)
77-81: LGTM: Import changes are well-structured
The new imports are properly organized and follow Vue best practices.
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: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
fix-panelaction, allowing better communication with parent components.panelFixed, to streamline the logic for determining the fixed state of panels in the header and tree components.