fix: resolve the automatic save state invalid after refresh #991
Conversation
github-actions
bot
added
bug
WalkthroughThe pull request introduces enhancements to the save toolbar component's auto-save functionality. A new constant Changes
Possibly related PRs
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: 2
🧹 Nitpick comments (1)
packages/utils/src/constants/index.js (1)
97-98: Consider using a storage key prefix to avoid collisions.While defining
AUTO_SAVEDis a good approach, it’s often safer to prefix storage keys for multi-app environments (e.g.,'tiny-auto-saved'). Additionally, consider centralizing all local storage keys to maintain better consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/toolbars/save/src/Main.vue(2 hunks)packages/utils/src/constants/index.js(1 hunks)
🔇 Additional comments (1)
packages/toolbars/save/src/Main.vue (1)
58-67: Good use of constants and lifecycle hooks.Importing
AUTO_SAVEDfrom the constants helps avoid magic strings. LeveragingonMountedis a solid approach to initialize your state upon component load.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/toolbars/save/src/Main.vue (1)
141-146: Consider persisting the auto-save interval alongside the auto-save state.Currently, only the auto-save enabled/disabled state is persisted. Consider also saving the user's preferred auto-save interval to provide a complete persistence of user preferences.
Example implementation:
const autoSave = () => { try { - localStorage.setItem(AUTO_SAVED, JSON.stringify(state.checked)) + localStorage.setItem(AUTO_SAVED, JSON.stringify({ + enabled: state.checked, + interval: state.timeValue + })) } catch (error) { return } // ... rest of the function } const initSaveStatus = () => { const autoSaveValue = localStorage.getItem(AUTO_SAVED) try { - state.checked = JSON.parse(autoSaveValue) ?? false + const savedState = JSON.parse(autoSaveValue) ?? { enabled: false, interval: 5 } + state.checked = savedState.enabled + state.timeValue = savedState.interval } catch (e) { state.checked = false + state.timeValue = 5 } }Also applies to: 154-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/toolbars/save/src/Main.vue(2 hunks)packages/utils/src/constants/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/constants/index.js
🔇 Additional comments (4)
packages/toolbars/save/src/Main.vue (4)
58-58: LGTM! Import changes are well-structured.The addition of
onMountedand constants import aligns with the implementation needs.Also applies to: 65-67
141-146: Consider adding user feedback for storage failures.While the try-catch block handles storage errors gracefully, silent failure might confuse users when their settings don't persist.
Consider adding user feedback:
try { localStorage.setItem(AUTO_SAVED, JSON.stringify(state.checked)) } catch (error) { + console.warn('Failed to save auto-save preference:', error) + // Using the existing tiny-engine UI components for consistency + TinyMessage.warning('Failed to save your auto-save preference') return }
154-162: LGTM! Robust implementation of save status initialization.The implementation includes proper error handling and default value fallback using nullish coalescing, which aligns with best practices.
164-166: LGTM! Appropriate use of lifecycle hook.The initialization of save status during component mount ensures proper state restoration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/toolbars/save/src/Main.vue (3)
141-146: Enhance error handling for better user feedback.While the try-catch block is good, silently failing when localStorage operations fail could lead to a confusing user experience. Consider:
- Notifying users when their settings fail to save
- Logging errors for debugging
- Providing fallback behavior
try { localStorage.setItem(AUTO_SAVED, JSON.stringify(state.checked)) } catch (error) { - return + console.error('Failed to save auto-save settings:', error) + // Using your UI notification system (assuming it exists) + notify.error('Failed to save your auto-save preferences') + state.checked = false // Reset to default state }
164-167: Consider adding storage event handling.While the initialization is well-implemented, consider adding a storage event listener to sync state across tabs/windows, and clean it up in onUnmounted:
onMounted(() => { initSaveStatus() + // Sync state across tabs + const handleStorageChange = (e) => { + if (e.key === AUTO_SAVED) { + initSaveStatus() + } + } + window.addEventListener('storage', handleStorageChange) + + onUnmounted(() => { + window.removeEventListener('storage', handleStorageChange) + }) })
141-162: Consider extracting storage logic to a composable.The current implementation mixes UI and storage concerns. Consider extracting the localStorage operations into a dedicated composable for better maintainability and reusability:
// useAutoSave.ts export function useAutoSave() { const getAutoSaveStatus = () => { try { const value = localStorage.getItem(AUTO_SAVED) return JSON.parse(value) ?? false } catch { return false } } const setAutoSaveStatus = (status: boolean) => { try { localStorage.setItem(AUTO_SAVED, JSON.stringify(status)) return true } catch (error) { console.error('Failed to save auto-save settings:', error) return false } } return { getAutoSaveStatus, setAutoSaveStatus } }This would simplify the component code and make the storage logic more testable and reusable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/toolbars/save/src/Main.vue(2 hunks)packages/utils/src/constants/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/constants/index.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/toolbars/save/src/Main.vue (2)
58-58: LGTM! Clean import organization.The imports are well-organized, and the constant destructuring follows best practices.
Also applies to: 66-67
154-162: Well-implemented error handling with fallback!The implementation includes:
- Proper error handling with try-catch
- Default fallback using null coalescing
- Clear single responsibility
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
New Features
Improvements