Notion - updated-page source improvements #14045
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe pull request introduces updates to the 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)
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: 3
Outside diff range and nitpick comments (1)
components/notion/sources/updated-page/updated-page.mjs (1)
60-62: Limit event emission during deploymentEmitting events for the first 25 pages during deployment might be excessive. Consider reducing the number or making it configurable to prevent overwhelming the system with events.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (2)
- components/notion/package.json (2 hunks)
- components/notion/sources/updated-page/updated-page.mjs (5 hunks)
Additional comments not posted (6)
components/notion/package.json (2)
3-3: Version update looks good!The minor version increment aligns with the PR objective of adding improvements while maintaining backwards compatibility.
20-20: The addition of the "md5" dependency looks good.The "md5" package is a popular, lightweight, and stable choice for MD5 hashing in JavaScript. The version specification "^2.3.0" is reasonable.
Please verify that this dependency is being used somewhere in the codebase. If it's unused, consider removing it to keep the dependencies lean.
Run the following script to search for the import or require statements of this package:
components/notion/sources/updated-page/updated-page.mjs (4)
5-5: Ensure 'md5' package is added to dependenciesThe
md5module is imported, but please verify that it's included in your project'spackage.jsonunder dependencies to prevent runtime errors.Run the following script to check if
md5is listed inpackage.json:#!/bin/bash # Description: Check if 'md5' is listed under dependencies in package.json grep '"md5"' package.json
46-47: Confirm implementation oflastUpdatedSortParam()methodThe method
lastUpdatedSortParam()is used to obtain sorting parameters for fetching pages. Ensure that this method is correctly implemented and accessible within the scope of this component.Run the following script to verify the definition of
lastUpdatedSortParam():#!/bin/bash # Description: Search for the definition of 'lastUpdatedSortParam' method rg --type js --type mjs --pcre2 'lastUpdatedSortParam\s*\('Also applies to: 102-102
53-53: Consider potential issues with JSON serialization orderUsing
JSON.stringifyon objects may lead to inconsistent results due to non-deterministic property ordering, which can affect the md5 hash comparison.To ensure consistent hashing, consider using a library that performs deterministic serialization, such as
json-stable-stringify.Apply this diff to change the implementation:
-import md5 from "md5"; +import md5 from "md5"; +import stringify from "json-stable-stringify"; ... -propertyValues[page.id][property] = md5(JSON.stringify(page.properties[property])); +propertyValues[page.id][property] = md5(stringify(page.properties[property])); ... -const currentProperty = md5(JSON.stringify(page.properties[property])); +const currentProperty = md5(stringify(page.properties[property]));Don't forget to add
json-stable-stringifyto your dependencies.Run the following script to check if
json-stable-stringifyis listed inpackage.json:#!/bin/bash # Description: Check if 'json-stable-stringify' is listed under dependencies in package.json grep '"json-stable-stringify"' package.jsonAlso applies to: 115-115
132-132: Verify that all necessary events are emittedEnsure that the
emitEventmethod is called for all pages that meet the criteria, and that no pages are unintentionally skipped due to logic errors.Run the following script to check where
emitEventis called:Verification successful
Verification confirms proper event emission
After a thorough analysis of the codebase, particularly the
components/notion/sources/updated-page/updated-page.mjsfile, I can confirm that theemitEventmethod is correctly implemented and called in appropriate places. The logic in therunmethod appears to cover all necessary cases:
- It emits events for updated pages
- It can optionally include new pages based on the
includeNewPagesprop- It uses timestamps and property change detection to avoid unnecessary emissions
The implementation looks thorough and doesn't seem to skip any necessary events. The concern raised in the original review comment appears to be addressed by the current implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to 'emitEvent' in the codebase rg --type js --type mjs 'this\.emitEvent\('Length of output: 45
Script:
#!/bin/bash # Description: Search for variations of 'emit' method calls # Search for 'emit' with various prefixes echo "Searching for 'emit' method calls:" rg --type js --type mjs '\b\w+\.emit\w*\(' # Search for 'emitEvent' without 'this' prefix echo -e "\nSearching for 'emitEvent' without 'this' prefix:" rg --type js --type mjs '\bemitEvent\(' # Search for any method containing 'emit' in its name echo -e "\nSearching for methods containing 'emit':" rg --type js --type mjs '\b\w*emit\w*\('Length of output: 526
Script:
#!/bin/bash # Description: Search for emit-related functions and examine the specific file # Search for 'emit' related functions in .js files echo "Searching for 'emit' related functions in .js files:" rg --type js '\b\w*emit\w*\(' # Display the content of the specific file echo -e "\nContent of the specific file:" cat components/notion/sources/updated-page/updated-page.mjs # Search for 'emit' related functions in the specific file echo -e "\nSearching for 'emit' related functions in the specific file:" grep -E '\b\w*emit\w*\(' components/notion/sources/updated-page/updated-page.mjsLength of output: 6709
| this.$emit(page, meta); | ||
| this.emitEvent(page); | ||
|
|
||
| if (Date.parse(page?.last_edited_time) < lastCheckedTimestamp) { |
There was a problem hiding this comment.
Does it make sense to add back the HTTP request param to filter out pages where last_edited_time < lastCheckedTimestamp:
{
filter: {
timestamp: "last_edited_time",
last_edited_time: {
on_or_after: new Date(lastCheckedTimestamp).toISOString()
}
}
}
There was a problem hiding this comment.
@andrewjschuang Ah, I didn't notice there's an on-or-after filter in addition to after. I added the filter back in, but I'm still hesitant to be using last_edited_time (either for filtering or for exitng the loop) since that was the reason the source was skipping events to begin with. The last_edited_time is not necessarily updated when a page is updated. I guess in this case it's a choice between possibly skipping events and having to loop through a large amount of pages.
There was a problem hiding this comment.
Thanks, based on my tests the Notion API was reliably updating the last_edited_time. The issues the user was seeing were most likely due to performance issues due to a huge number of pages in the database. I tested with a fairly large database, and with the improvements from this PR, I did not have any issues
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/notion/sources/updated-page/updated-page.mjs (5 hunks)
Additional comments not posted (6)
components/notion/sources/updated-page/updated-page.mjs (6)
5-5: LGTM: Good addition of md5 for hashingThe addition of the md5 library is a good choice for hashing property values. This will improve the efficiency and reliability of property comparisons.
12-12: LGTM: Version updateThe version update from "0.0.16" to "0.0.17" is appropriate given the changes made to the component.
46-62: LGTM: Improved deploy method logicThe changes in the deploy method enhance the handling of page updates and timestamp management. The use of md5 for hashing property values is a good practice for efficient comparison. Limiting event emission to the first 25 pages is likely a performance optimization.
104-109: LGTM: Efficient filtering of pagesThe addition of the filter to fetch only pages edited on or after the last checked timestamp is a good optimization. It reduces the number of pages that need to be processed, potentially improving performance.
123-123: LGTM: Consistent use of md5 for property comparisonThe use of md5 for hashing property values in the comparison logic is consistent with earlier changes and improves the efficiency and reliability of property comparisons.
140-140: LGTM: Consistent use of emitEvent methodThe use of the newly added
emitEventmethod for event emission is consistent and improves code organization by encapsulating the event emission logic.
This reverts commit c0657fa.
|
Summarizing our conversation at Slack:
Solution:
|
Summary by CodeRabbit
New Features
notion-updated-pagecomponent to improve page update handling.md5library.Version Updates
@pipedream/notionfrom 0.1.21 to 0.1.22.