-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: resolve the automatic save state invalid after refresh #1004
fix: resolve the automatic save state invalid after refresh #1004
Conversation
WalkthroughThis pull request introduces modifications to implement auto-save functionality within the application. Changes are made across three files, adding methods to manage auto-save status using local storage. The user can toggle auto-save preferences, which are persistently stored and retrieved between sessions. These updates enhance the save toolbar's functionality by allowing for a more configurable saving experience. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/toolbars/save/src/js/index.js (2)
179-186
: Consider adding error logging.The error handling is appropriate, but consider logging errors to help with debugging issues in production.
export const getAutoSaveStatus = () => { try { const value = localStorage.getItem(AUTO_SAVED) return JSON.parse(value) ?? false - } catch { + } catch (error) { + console.error('Failed to get auto-save status:', error) return false } }
188-195
: Add JSDoc comments for better documentation.The function implementation is correct, but adding JSDoc comments would improve maintainability.
+/** + * Sets the auto-save status in localStorage + * @param {boolean} status - The auto-save status to store + * @returns {boolean} - True if the status was successfully stored, false otherwise + */ export const setAutoSaveStatus = (status) => { try { localStorage.setItem(AUTO_SAVED, JSON.stringify(status)) return true - } catch { + } catch (error) { + console.error('Failed to set auto-save status:', error) return false } }packages/toolbars/save/src/Main.vue (2)
148-151
: Add error handling and loading state for initialization.Consider adding error handling and a loading state during initialization to improve user experience.
+ const isInitializing = ref(true) onMounted(() => { + try { state.checked = getAutoSaveStatus() + if (state.checked) { + saveSetTimeout() + } + } catch (error) { + console.error('Failed to initialize auto-save:', error) + useNotify({ + type: 'error', + message: 'Failed to load auto-save settings' + }) + } finally { + isInitializing.value = false + } })
140-147
: Add user feedback for save settings.Consider providing feedback when save settings are updated or when errors occur.
const autoSave = () => { - setAutoSaveStatus(state.checked) + if (setAutoSaveStatus(state.checked)) { + useNotify({ + type: 'success', + message: `Auto-save ${state.checked ? 'enabled' : 'disabled'}` + }) + } else { + useNotify({ + type: 'error', + message: 'Failed to update auto-save settings' + }) + // Revert the checkbox state on error + state.checked = getAutoSaveStatus() + return + } if (state.checked) { saveSetTimeout() } else { clearTimeout(state.preservationTime) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/toolbars/save/src/Main.vue
(2 hunks)packages/toolbars/save/src/js/index.js
(2 hunks)packages/utils/src/constants/index.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/utils/src/constants/index.js (1)
101-102
: LGTM! The constant follows the established pattern.The new
AUTO_SAVED
constant follows the project's naming convention and is appropriately placed among other storage-related constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/toolbars/save/src/Main.vue (1)
139-145
: Consider triggering an immediate save when auto-save is enabled.The current implementation sets up auto-save for future intervals but doesn't perform an immediate save when auto-save is enabled. This might not meet user expectations, as they might expect their current changes to be saved immediately when enabling auto-save.
const saveConfig = () => { setAutoSaveStatus(state.checked) if (state.checked) { + openApi() // Trigger immediate save saveSetTimeout() } else { clearTimeout(state.preservationTime) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/save/src/Main.vue
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/toolbars/save/src/Main.vue (3)
23-23
: LGTM! Method name change improves clarity.The rename from
autoSave
tosaveConfig
better reflects the method's purpose and follows the verb+noun naming convention.
58-58
: LGTM! Required imports added for auto-save persistence.The new imports support the auto-save state persistence functionality, which aligns with the PR objective.
Also applies to: 64-64
166-166
: LGTM! Method correctly exposed to template.The
saveConfig
method is properly exposed in the component's return statement.
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