[FIX] Notion: New or Updated Page in Database is not emitting events #18127
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughVersion bumps applied to Notion components. The updated-page source refactors new-page detection to a page-level check, tracks property values for all pages, and gates emissions based on includeNewPages. Events now emit on property changes for existing pages and optionally for new pages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as Scheduler/Poller
participant N as Notion API
participant U as UpdatedPage Source
participant E as Event Stream
P->>U: Invoke run()
U->>N: Query database pages
N-->>U: Pages with properties
loop For each page
U->>U: pageExistsInDB = propertyValues[page.id] != null
alt Existing page
loop For each property
U->>U: Compare current vs stored
alt Changed
U->>E: Emit change event
else Not changed
U->>U: No emit
end
U->>U: Update stored value
end
else New page
U->>U: Ensure propertyValues[page.id] initialized
loop For each property
U->>U: Store current value
alt includeNewPages == true and Changed
U->>E: Emit change event
else Gate emission
U->>U: No emit
end
end
end
end
U-->>P: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/notion/sources/updated-page/updated-page.mjs (2)
79-84: Potential crash when state is absent or from older deployments (inflate undefined/legacy JSON).on cold starts or migrated sources,
this.db.get("propertyValues")can be null/undefined or contain legacy (uncompressed) JSON. CallingBuffer.from(compressed, "base64")andinflateSyncwill throw, preventing runs and emissions.Harden
_getPropertyValuesto:
- Return
{}if the key is missing.- Try deflate/base64 first, then fall back to direct JSON parse for backward compatibility.
Apply this diff:
_getPropertyValues() { - const compressed = this.db.get("propertyValues"); - const buffer = Buffer.from(compressed, "base64"); - const decompressed = zlib.inflateSync(buffer).toString(); - return JSON.parse(decompressed); + const compressed = this.db.get("propertyValues"); + if (!compressed) return {}; + try { + const buffer = Buffer.from(compressed, "base64"); + const decompressed = zlib.inflateSync(buffer).toString(); + return JSON.parse(decompressed); + } catch (err) { + // Backward compat or corrupted state: attempt direct JSON parse; otherwise, start fresh + try { + return typeof compressed === "string" ? JSON.parse(compressed) : {}; + } catch { + return {}; + } + } },
133-140: Early return should precede state load to avoid unnecessary (and potentially failing) reads.If
LAST_EDITED_TIMEis null (e.g., right after deactivation or before first activation completes), we should skip without touchingpropertyValues. Currently,_getPropertyValues()is called before the guard, which can still throw if state is missing.Apply this diff:
async run() { const lastCheckedTimestamp = this._getLastUpdatedTimestamp(); - const propertyValues = this._getPropertyValues(); - - if (!lastCheckedTimestamp) { + if (!lastCheckedTimestamp) { // recently updated (deactivated / activated), skip execution console.log("Awaiting restart completion: skipping execution"); return; } + const propertyValues = this._getPropertyValues();
🧹 Nitpick comments (1)
components/notion/sources/updated-page/updated-page.mjs (1)
67-69: Log says “clearing states” but only timestamp is cleared; consider clearing cached property snapshots too.Small consistency nit. Either update the message or clear
propertyValuesso state is fully reset on deactivate. Clearing avoids stale snapshots and reduces storage.Apply this diff to clear the cache:
async deactivate() { console.log("Deactivating: clearing states"); this._setLastUpdatedTimestamp(null); + // Clear cached property snapshots to force a re-sync on next activation + this.db.set("propertyValues", null); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
components/notion/package.json(1 hunks)components/notion/sources/updated-page/updated-page.mjs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#15376
File: components/monday/sources/name-updated/name-updated.mjs:6-6
Timestamp: 2025-01-23T03:55:15.166Z
Learning: Source names in Monday.com components don't need to start with "New" if they emit events for updated items (e.g., "Name Updated", "Column Value Updated") rather than new items. This follows the component guidelines exception where the "New" prefix is only required when emits are limited to new items.
🧬 Code Graph Analysis (1)
components/notion/sources/updated-page/updated-page.mjs (1)
components/notion/sources/updated-page-by-timestamp/updated-page-by-timestamp.mjs (1)
isNewPage(74-74)
🔇 Additional comments (6)
components/notion/package.json (1)
3-3: Version bump aligns with PR scope.Increment to 0.9.1 is appropriate to publish the updated source behavior.
components/notion/sources/updated-page/updated-page.mjs (5)
12-12: Version bump acknowledged.Increment to 0.1.11 reflects functional changes in the source.
169-172: New-page detection via presence in persisted state looks correct.Using
propertyValues[page.id] != nullfor existence is robust and fixes per-iteration re-declarations.
196-212: Tracking properties for new pages while gating emission viaincludeNewPagesis sound.This ensures we always seed state for new pages (preventing future false positives) and only emit when configured.
215-219: Emission gating is correct and matches expected behavior.Emits for existing pages on changes, and for new pages only when
includeNewPagesis true.
165-167: Confirmed descending sort; early break is safe.The
lastUpdatedSortParammethod (incomponents/notion/sources/common/base.mjs) setsdirection: constants.directions.DESCENDING. Pages are fetched in descending order oflast_edited_time, so once you encounter a timestamp older thanlastCheckedTimestamp, it’s correct tobreak.
WHY
Resolves #18102
Summary by CodeRabbit
New Features
Bug Fixes
Chores