-
Notifications
You must be signed in to change notification settings - Fork 889
fix timing issue to apply edits after data has come in #8025
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull request overview
This pull request fixes a timing issue in the data editor where edits were being applied before data had loaded, resulting in edits not being applied at all. The fix changes from using useOnMount to using useEffect with dependencies on data.length and edits, and adds a ref to track whether edits have been applied.
Changes:
- Replaced
useOnMountwithuseEffectto apply edits only after data has loaded - Added
hasAppliedEditsref to prevent re-applying edits on subsequent renders - Moved
columnsdefinition before the edit-application logic (required for the effect to access it) - Removed a TODO comment about old edits
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frontend/src/plugins/impl/data-editor/glide-data-editor.tsx | Replaced useOnMount with useEffect to fix timing issue when applying initial edits, added ref to track application state, and reorganized code |
| frontend/src/plugins/impl/DataEditorPlugin.tsx | Removed TODO comment about old edits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Apply initial edits after data has loaded | ||
| useEffect(() => { | ||
| // Don't apply if already applied or data hasn't loaded yet | ||
| if (hasAppliedEdits.current || data.length === 0) { | ||
| return; | ||
| } | ||
|
|
Copilot
AI
Jan 28, 2026
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.
The condition data.length === 0 prevents the effect from running when data is empty. However, this also prevents processing edits that add new rows to an initially empty dataset (see lines 149-157 which handle edit.rowIdx >= data.length). If a user starts with an empty dataset and has edits that add rows, those edits won't be applied. Consider using a separate loading state flag or checking if data has been initialized, rather than relying solely on data.length.
| // Apply initial edits after data has loaded | |
| useEffect(() => { | |
| // Don't apply if already applied or data hasn't loaded yet | |
| if (hasAppliedEdits.current || data.length === 0) { | |
| return; | |
| } | |
| // Apply initial edits once | |
| useEffect(() => { | |
| // Don't apply if: | |
| // - Already applied edits | |
| // - No edits to apply | |
| if (hasAppliedEdits.current || edits.length === 0) { |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.19.7-dev31 |
📝 Summary
Closes #8024. There was a timing issue where the edits are applied even when data is empty, hence they are not applied.
🔍 Description of Changes
📋 Checklist