Skip to content

Conversation

michelle0927
Copy link
Collaborator

@michelle0927 michelle0927 commented Sep 16, 2025

Resolves #18349
Follow-up to PR #18368

Summary by CodeRabbit

  • Bug Fixes

    • Added strict validation for Notion page properties (title, rich text, number, select, status, multi-select, URL, email, phone) to prevent invalid inputs.
    • Improved error messages to reference property names for clearer troubleshooting.
  • Refactor

    • Enhanced property handling with human-readable names used across messaging and downstream processing.
  • Chores

    • Version bumps across multiple Notion actions and the Notion package; no behavior changes.

Copy link

vercel bot commented Sep 16, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
pipedream-docs Ignored Ignored Sep 16, 2025 6:06pm
pipedream-docs-redirect-do-not-edit Ignored Ignored Sep 16, 2025 6:06pm

Copy link
Contributor

coderabbitai bot commented Sep 16, 2025

Walkthrough

Multiple Notion action modules received version metadata bumps. The Notion page property conversion gained runtime type validation for several property types. The base page builder now includes a name field in property descriptors and updates error messages to reference property.name. Package version was incremented.

Changes

Cohort / File(s) Summary of changes
Property pipeline updates
components/notion/actions/common/base-page-builder.mjs
Adds name to property descriptors from _filterProps; error messages in _convertPropertiesToNotion now reference property.name.
Property conversion validation
components/notion/common/notion-page-properties.mjs
Adds runtime type checks in convertToNotion for title, rich_text, number, status.name, select.name, multi_select, url, email, phone_number; throws on invalid types before building payloads.
Action metadata version bumps
components/notion/actions/append-block/append-block.mjs, .../complete-file-upload/complete-file-upload.mjs, .../create-database/create-database.mjs, .../create-file-upload/create-file-upload.mjs, .../create-page-from-database/create-page-from-database.mjs, .../create-page/create-page.mjs, .../delete-block/delete-block.mjs, .../duplicate-page/duplicate-page.mjs, .../list-file-uploads/list-file-uploads.mjs, .../retrieve-file-upload/retrieve-file-upload.mjs, .../send-file-upload/send-file-upload.mjs, .../update-block/update-block.mjs, .../update-database/update-database.mjs, .../update-page/update-page.mjs
Bumps version fields only; no logic, props, or runtime changes.
Package version bump
components/notion/package.json
Increments package version from 1.0.1 to 1.0.2.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Action as Notion Action
  participant Builder as base-page-builder
  participant Props as NOTION_PAGE_PROPERTIES

  User->>Action: Trigger with properties input
  Action->>Builder: _filterProps(properties)
  Builder-->>Action: descriptors [{type,label,value,name}]
  Action->>Builder: _convertPropertiesToNotion(descriptors, CONVERTER)
  loop For each descriptor
    Builder->>Props: convertToNotion(property)
    alt Valid type
      Props-->>Builder: Notion-formatted property
    else Invalid type
      Props-->>Builder: throw Error (type validation)
      Builder-->>Action: throw ConfigurationError mentioning property.name
    end
  end
  Action-->>User: Success or configuration error
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

User submitted

Suggested reviewers

  • luancazarine
  • danhsiung

Poem

A rabbit taps keys with a gentle thrum,
Properties named, validations come.
Titles and numbers, checked in line,
Errors now speak each property’s sign.
Versions hop up—no fuss, no fright—
Pages and blocks align just right.
Thump-thump: shipping through the night.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description does not follow the repository template: the required "## WHY" section is missing and the body only contains "Resolves #18349" and a follow-up note, so it lacks rationale, a concise summary of the substantive code changes, and any testing/impact information needed for reviewers. Please update the PR description to include the "## WHY" section with a brief rationale, a short summary of the files and behavioral changes (e.g., base-page-builder and notion-page-properties), any migration/compatibility impacts, and a brief testing/verifications checklist plus links to the related PR(s) and issue(s).
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Notion property building improvements" is concise, focused, and accurately reflects the primary change in this PR (improvements to Notion property construction and validation in shared Notion modules), so it is clear to reviewers and history scanners.
Linked Issues Check ✅ Passed The changes address issue #18349: _filterProps now includes property.name, NOTION_PAGE_PROPERTIES.convertToNotion adds runtime type validation for multiple property types (title, rich_text, number, status, select, multi_select, url, email, phone_number), and _convertPropertiesToNotion's error handling now uses property.name in the ConfigurationError message; together these validate inputs against expected schemas and produce clearer ConfigurationErrors when values have the wrong type.
Out of Scope Changes Check ✅ Passed The substantive functional changes are confined to components/notion/actions/common/base-page-builder.mjs (adds name to property descriptors and updates ConfigurationError usage) and components/notion/common/notion-page-properties.mjs (adds runtime type checks); the other modified files are metadata version bumps and package.json version updates, so there are no unrelated functional changes introduced by this PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch issue-1839-2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@michelle0927 michelle0927 marked this pull request as ready for review September 16, 2025 18:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
components/notion/actions/common/base-page-builder.mjs (1)

99-112: Guard missing converters and surface the root cause in ConfigurationError.
If a converter is absent, the code currently sets undefined silently, leading to opaque downstream errors. Also, the catch discards the original error message (e.g., allowed select options), reducing debuggability.

Apply this diff:

         } else {
           // Otherwise, convert using the appropriate converter
           const notionProperty = NOTION_CONVERTER[property.type];
-          try {
-            notionProperties[property.label] = notionProperty?.convertToNotion(property);
-          } catch {
-            throw new ConfigurationError(`Error converting property \`${property.name}\` to Notion format. Must be of type \`${NOTION_CONVERTER[property.type]?.type}\`.`);
-          }
+          if (!notionProperty || typeof notionProperty.convertToNotion !== "function") {
+            throw new ConfigurationError(`Unsupported property type \`${property.type}\` for property \`${property.name}\`.`);
+          }
+          try {
+            notionProperties[property.label] = notionProperty.convertToNotion(property);
+          } catch (err) {
+            const expected = NOTION_CONVERTER[property.type]?.type;
+            const details = err?.message ? ` Details: ${err.message}` : "";
+            throw new ConfigurationError(
+              `Error converting property \`${property.name}\` to Notion format. Must be of type \`${expected}\`.${details}`
+            );
+          }
         }
components/notion/common/notion-page-properties.mjs (1)

1-1: Use ConfigurationError with clear messages (per PR objective).

All new type guards throw a bare Error without a message. The objective requires a ConfigurationError with an explanatory message. Import and use it across all guards.

Add this import at the top (outside the changed ranges):

import { ConfigurationError } from "@pipedream/platform";
🧹 Nitpick comments (3)
components/notion/actions/common/base-page-builder.mjs (1)

83-88: Don’t drop valid falsy values from properties (use hasOwnProperty).
Current truthiness check omits values like empty strings, false, 0 when passed via this.properties. Use an own‑property check instead.

Apply this diff:

 return Object.keys(properties)
-        .filter((property) => this[property] != null
-          || (this.properties && this.properties[property]))
+        .filter((property) =>
+          this[property] != null
+          || (this.properties
+            && Object.prototype.hasOwnProperty.call(this.properties, property))
+        )
         .map((property) => ({
components/notion/common/notion-page-properties.mjs (2)

79-89: Extend validation to multi_select for consistency (type + options).

Follow the same pattern: require a string (to parse), and validate each value against allowed options when present.

Suggested replacement:

-    convertToNotion: (property) => ({
-      multi_select: utils.parseArray(property.value).map((name) => ({
-        name,
-      })),
-    }),
+    convertToNotion: (property) => {
+      if (typeof property.value !== "string") {
+        throw new ConfigurationError(`Invalid value for property "${property.name ?? "multi_select"}": expected a string (CSV or JSON array), received ${typeof property.value}.`);
+      }
+      const values = utils.parseArray(property.value);
+      const allowed = property.multi_select?.options?.map(({ name }) => name) ?? [];
+      if (allowed.length) {
+        const invalid = values.filter((v) => !allowed.includes(v));
+        if (invalid.length) {
+          throw new ConfigurationError(`Invalid options for "${property.name ?? "multi_select"}": ${invalid.map((v) => `"${v}"`).join(", ")}. Allowed: ${allowed.map((v) => `"${v}"`).join(", ")}.`);
+        }
+      }
+      return { multi_select: values.map((name) => ({ name })) };
+    },

11-186: Reduce duplication with small guard helpers (optional).

Consider extracting helpers in this module or utils (e.g., ensureString(property, kind), ensureNumber(property)) to standardize messages and keep convertors concise.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3e3821 and d9c216f.

📒 Files selected for processing (17)
  • components/notion/actions/append-block/append-block.mjs (1 hunks)
  • components/notion/actions/common/base-page-builder.mjs (2 hunks)
  • components/notion/actions/complete-file-upload/complete-file-upload.mjs (1 hunks)
  • components/notion/actions/create-database/create-database.mjs (1 hunks)
  • components/notion/actions/create-file-upload/create-file-upload.mjs (1 hunks)
  • components/notion/actions/create-page-from-database/create-page-from-database.mjs (1 hunks)
  • components/notion/actions/create-page/create-page.mjs (1 hunks)
  • components/notion/actions/delete-block/delete-block.mjs (1 hunks)
  • components/notion/actions/duplicate-page/duplicate-page.mjs (1 hunks)
  • components/notion/actions/list-file-uploads/list-file-uploads.mjs (1 hunks)
  • components/notion/actions/retrieve-file-upload/retrieve-file-upload.mjs (1 hunks)
  • components/notion/actions/send-file-upload/send-file-upload.mjs (1 hunks)
  • components/notion/actions/update-block/update-block.mjs (1 hunks)
  • components/notion/actions/update-database/update-database.mjs (1 hunks)
  • components/notion/actions/update-page/update-page.mjs (1 hunks)
  • components/notion/common/notion-page-properties.mjs (2 hunks)
  • components/notion/package.json (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T09:02:18.222Z
Learnt from: nurul3101
PR: PipedreamHQ/pipedream#18092
File: components/prisma_management_api/README.md:24-31
Timestamp: 2025-08-19T09:02:18.222Z
Learning: In Prisma Management API documentation, the "Create Database" section title is intentional and should not be changed to "Create Project", even though the action technically creates a complete project with database. This naming choice follows user-facing terminology preferences.

Applied to files:

  • components/notion/actions/create-database/create-database.mjs
🧬 Code graph analysis (2)
components/notion/actions/common/base-page-builder.mjs (2)
components/notion/actions/update-page/update-page.mjs (1)
  • properties (89-89)
components/notion/actions/create-page-from-database/create-page-from-database.mjs (1)
  • properties (70-70)
components/notion/common/notion-page-properties.mjs (1)
components/notion/actions/common/base-page-builder.mjs (1)
  • property (57-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Verify TypeScript components
  • GitHub Check: Publish TypeScript components
  • GitHub Check: Lint Code Base
🔇 Additional comments (16)
components/notion/actions/update-block/update-block.mjs (1)

10-10: Patch version bump acknowledged.

Metadata-only change; safe to merge.

components/notion/actions/create-database/create-database.mjs (1)

10-10: Version bump only — fine.

No functional changes detected.

components/notion/actions/delete-block/delete-block.mjs (1)

9-9: Version metadata update LGTM.

No runtime impact here.

components/notion/actions/list-file-uploads/list-file-uploads.mjs (1)

9-9: Version increment approved.

No code path changes.

components/notion/actions/duplicate-page/duplicate-page.mjs (1)

10-10: Version bump acknowledged.

No behavior change in this diff.

components/notion/package.json (1)

3-3: Approve — Notion package and actions bumped to 1.0.2.
components/notion/package.json is "version": "1.0.2" and no occurrences of version: "1.0.1" were found under components/notion/actions.

components/notion/actions/create-page-from-database/create-page-from-database.mjs (1)

11-11: Version bump OK — validation present; verify unmatched-type handling

base-page-builder._convertPropertiesToNotion wraps NOTION converters in a try/catch and rethrows a ConfigurationError on conversion failures; NOTION_PAGE_PROPERTIES' convertToNotion implementations throw on invalid values, so those mismatches surface as ConfigurationError. Caveat: if a property.type has no matching entry in NOTION_CONVERTER, the optional chaining prevents an error (the conversion is skipped and no ConfigurationError is thrown). See components/notion/actions/common/base-page-builder.mjs and components/notion/common/notion-page-properties.mjs.

components/notion/actions/update-database/update-database.mjs (1)

10-10: Version bump only — looks good.

No behavior change in this file; version syncs with package release. Notion’s docs do not use the slug "update-a-data-source" — the page is titled "Update data source" and the API endpoints are under /v1/data_sources; update any repo links that reference the incorrect slug.

Likely an incorrect or invalid review comment.

components/notion/actions/send-file-upload/send-file-upload.mjs (1)

11-11: Version bump only — no functional risk detected.

components/notion/actions/retrieve-file-upload/retrieve-file-upload.mjs (1)

9-9: Version bump only — no functional risk detected.

components/notion/actions/create-file-upload/create-file-upload.mjs (1)

9-9: Version bump only — no functional risk detected.

components/notion/actions/complete-file-upload/complete-file-upload.mjs (1)

9-9: Version bump only — no functional risk detected.

components/notion/actions/create-page/create-page.mjs (1)

10-10: Version bump only — no functional risk detected.

components/notion/actions/update-page/update-page.mjs (1)

10-10: Version bump only — no functional risk detected.

components/notion/actions/append-block/append-block.mjs (1)

10-10: Version bump only — no functional risk detected.

components/notion/actions/common/base-page-builder.mjs (1)

90-91: Nice UX improvement: include property.name for clearer errors.
This will surface human-friendly names in error messages.

Copy link
Collaborator

@GTFalcao GTFalcao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @michelle0927 , I left one comment regarding the errors with no message, let me know if it makes sense

}),
convertToNotion: (property) => {
if (typeof property.value !== "string") {
throw new Error;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a message to these errors, in case they do get thrown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're caught in /common/base-page-builder.mjs on line 111:

throw new ConfigurationError(Error converting property \${property.name}` to Notion format. Must be of type `${NOTION_CONVERTER[property.type]?.type}`.`);

@michelle0927 michelle0927 merged commit 4636f3e into master Sep 18, 2025
10 checks passed
@michelle0927 michelle0927 deleted the issue-1839-2 branch September 18, 2025 14:38
sergio-eliot-rodriguez pushed a commit to sergio-eliot-rodriguez/sergio_wong_does_pipedream that referenced this pull request Sep 21, 2025
vunguyenhung added a commit that referenced this pull request Sep 24, 2025
* Leonardo AI components

* added unzoom image action

* fixing link errors

* more lint fixes

* Merging pull request #18359

* fix: pagination prop and params struct

* fix: no need for paginate here

* chore: update version

* chore: cleanup

* chore: update package

* feat: allow raw response

* chore: bump package

* fix: buffer response instead

* Update components/google_drive/actions/download-file/download-file.mjs

Co-authored-by: Jorge Cortes <jacortesmahmud@gmail.com>

* versions

* pnpm-lock.yaml

* pnpm-lock.yaml

* pnpm-lock.yaml

* feat: add content selector

* chore: bump package

* fix: comments

* chore: bump versions

* chore: fix versions

* fixes: QA fixes

* feat: add cursor to req

* package.json

---------

Co-authored-by: joao <joao@coform.com>
Co-authored-by: joaocoform <joao@usecoform.com>
Co-authored-by: Jorge Cortes <jacortesmahmud@gmail.com>
Co-authored-by: Michelle Bergeron <michelle.bergeron@gmail.com>
Co-authored-by: Luan Cazarine <luanhc@gmail.com>

* Merging pull request #18361

* update siteId prop

* pnpm-lock.yaml

* package.json version

* Google Sheets - update row refresh fields  (#18369)

* change prop order and refresh fields

* bump package.json

* Pipedrive - fix app name (#18370)

* use pipedriveApp instead of app

* bump package.json

* Pipedrive - pipelineId integer (#18372)

* pipelineId - integer

* bump versions

* Adding app scaffolding for lightspeed_ecom_c_series

* Adding app scaffolding for financial_data

* Adding app scaffolding for microsoft_authenticator

* Merging pull request #18345

* updates

* versions

* versions

* Merging pull request #18368

* updates

* remove console.log

* versions

* Coinbase Developer Platform - New Wallet Event (#18342)

* new component

* pnpm-lock.yaml

* updates

* updates

* Hubspot - update search-crm (#18360)

* update search-crm

* limit results to one page

* update version

* package.json version

* Merging pull request #18347

* widget props

* fix version

* Adding app scaffolding for rundeck

* Merging pull request #18378

* Update Taiga component with new actions and sources

- Bump version to 0.1.0 in package.json and add dependency on @pipedream/platform.
- Introduce new actions for creating, updating, and deleting issues, tasks, and user stories.
- Add sources for tracking changes and deletions of issues and tasks.
- Implement utility functions for parsing and cleaning objects in common/utils.mjs.
- Enhance prop definitions for better integration with Taiga API.

* pnpm update

* Refactor Taiga actions to utilize parseObject utility

- Added parseObject utility for tags, watchers, and points in update-issue, update-task, and update-userstory actions.
- Removed the update-project action as it is no longer needed.
- Enhanced base source to include secret key validation for webhook security.

* Merging pull request #18382

* add testSources prop

* pnpm-lock.yaml

* fix

* Merging pull request #18323

* Added actions

* Added actions

* Added actions

* Merging pull request #18377

* Added actions

* Update components/weaviate/actions/create-class/create-class.mjs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

---------

Co-authored-by: Luan Cazarine <luanhc@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Merging pull request #18376

* Adding app scaffolding for etrusted

* Adding app scaffolding for intelliflo_office

* Adding app scaffolding for thoughtspot

* Adding app scaffolding for kordiam

* Adding app scaffolding for ticketsauce

* trustpilot fixes (#18152)

* trustpilot fixes

* more fixes

* update versions

* more version updates

* fixes

* Bump all Trustpilot actions to version 0.1.0

Major improvements and API updates across all actions:
- Enhanced private API support with proper authentication
- Improved parameter handling and validation
- Better error handling and response structures
- Added new conversation flow for product reviews
- Fixed endpoint URLs to match latest API documentation
- Streamlined request/response processing

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* up version and clean up sources

* merge

* fix business ID

* delete temp action

* Update components/trustpilot/sources/new-product-reviews/new-product-reviews.mjs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Update components/trustpilot/sources/new-product-reviews/new-product-reviews.mjs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Update components/trustpilot/sources/new-product-reviews/new-product-reviews.mjs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Update components/trustpilot/sources/new-service-reviews/new-service-reviews.mjs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* comments

* Pagination

* fixes

* comments

* missed some `$`'s

* unduplicated

* more fixes

* final comments

* more comments

* .

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Adding app scaffolding for peekalink

* 18314 twilio (#18350)

* Update Twilio component versions and dependencies

- Update Twilio Send Message action adding detailed description for 'from' prop and refactoring phone number validation logic.
- Incremented action versions for several Twilio actions.

* pnpm update

* Updating LinkedIn API version (#18399)

* Merging pull request #18394

* Databricks API - Jobs action components (#18371)

* Notion property building improvements (#18381)

* validate property types

* versions

* Google Business - add debug log (#18407)

* add debug log

* bump versions

* Notion API Key - update @pipedream/notion version (#18409)

* update @pipedream/notion dependency version

* pnpm-lock.yaml

* Adding app scaffolding for reduct_video

* Adding app scaffolding for shopware

* Adding app scaffolding for instamojo

* Hubspot - bug fix to sources w/ property changes (#18379)

* updates

* versions

* Google sheets type fix (#18411)

* Fixing worksheetId prop type from string to integer

* Version bumps

---------

Co-authored-by: Leo Vu <vunguyenhung@outlook.com>

* Merging pull request #18393

* new components

* remove console.log

* versions

* update

* Merging pull request #18408

* 403 error message

* versions

* update

* Merging pull request #18419

* Changes per PR Review

* Removes leonardo_ai_actions.mdc not indented for merging

* synced lockfile after install

* fully lock form-data for leonardo_ai

* conflict solving

* lint fixes

* Chipped down Readme, implemented async options in gen motion

---------

Co-authored-by: jocarino <45713006+jocarino@users.noreply.github.com>
Co-authored-by: joao <joao@coform.com>
Co-authored-by: joaocoform <joao@usecoform.com>
Co-authored-by: Jorge Cortes <jacortesmahmud@gmail.com>
Co-authored-by: Michelle Bergeron <michelle.bergeron@gmail.com>
Co-authored-by: Luan Cazarine <luanhc@gmail.com>
Co-authored-by: michelle0927 <michelle0927@users.noreply.github.com>
Co-authored-by: Andrew Chuang <andrewjschuang@gmail.com>
Co-authored-by: danhsiung <35384182+danhsiung@users.noreply.github.com>
Co-authored-by: Lucas Caresia <lucascarezia@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Job <9075380+Afstkla@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Guilherme Falcão <48412907+GTFalcao@users.noreply.github.com>
Co-authored-by: Leo Vu <vunguyenhung@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notion property building improvements
2 participants