-
Notifications
You must be signed in to change notification settings - Fork 46
Notion: optimize syncing files and images #416
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
kaloyanvi
left a comment
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.
Looks mostly good, left a comment about using simpler data structure for storing.
kaloyanvi
left a comment
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.
Pre-approving, nice work! We should address this comment though.
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.
Comment @cursor review or bugbot run to trigger another review on this PR
plugins/notion/src/data.ts
Outdated
| } | ||
|
|
||
| if (fieldsById.has(pageCoverProperty.id)) { | ||
| if (fieldsById.has(pageCoverProperty.id) && !isUnchanged) { |
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.
Bug: Field Type Changes Missed in Sync
The syncCollection function's optimization for unchanged items doesn't fully apply to page content and cover fields. It skips processing these fields if the item hasn't changed, even if their field type has been modified or they are newly added. This can lead to missing data for these fields.
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.
Great catch here. The field type of content and cover can't be changed, but was broken when you would sync with content or cover disabled then enable them and sync again. Fixed that.
|
Fixed another bug and added the optimization you suggested @kaloyanvi. Ready for QA now. |
|
✅ QA, nice one! |
|
Btw, for things like this we should start adding tests. We should be able to simulate all cases to update / not update based on item / field with some mock data. |
Description
This pull request optimizes re-syncing collections in the Notion plugin by only including field values if the item has been changed since the last sync or the field type has changed. This eliminates one API call per image or file on unchanged items.
When re-syncing a database with 50 images and no changes in Notion, it now takes about 1.5-2 seconds to sync compared to 5-8 seconds before.
Screen.Recording.2025-09-04.at.8.27.32.PM.mov
Testing
Here's a database with 50 images:
https://www.notion.so/framer/264adf6e8c968083994bc1de22c55953?v=264adf6e8c96808aa97d000c2f134e07&source=copy_link