[BUG] Spotify Get Playlist Action doesn't work with custom expressions or variables #17863
Conversation
|
@tkalejaiye is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
WalkthroughThis change updates the version numbers of multiple Spotify action modules and the package manifest. Additionally, it modifies the "Get Playlist" action to accept a direct playlist ID value, addressing an input issue. The Spotify app component also removes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~6 minutes Assessment against linked issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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. 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: 1
🧹 Nitpick comments (1)
components/spotify/actions/get-track/get-track.mjs (1)
9-9: Version bump looks good – consider updating the component changelog as wellThe patch-level increment to
0.1.2is appropriate for a non-functional change. Ensure the repository’s global CHANGELOG (or per-package release notes) captures this bump so downstream consumers understand that no behavioral changes were introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
components/spotify/actions/add-items-to-playlist/add-items-to-playlist.mjs(1 hunks)components/spotify/actions/create-playlist/create-playlist.mjs(1 hunks)components/spotify/actions/get-album-tracks/get-album-tracks.mjs(1 hunks)components/spotify/actions/get-all-tracks-by-artist/get-all-tracks-by-artist.mjs(1 hunks)components/spotify/actions/get-artist-top-tracks/get-artist-top-tracks.mjs(1 hunks)components/spotify/actions/get-audio-features-for-a-track/get-audio-features-for-a-track.mjs(1 hunks)components/spotify/actions/get-categorys-playlist/get-categorys-playlist.mjs(1 hunks)components/spotify/actions/get-currently-playing-track/get-currently-playing-track.mjs(1 hunks)components/spotify/actions/get-playlist-items/get-playlist-items.mjs(1 hunks)components/spotify/actions/get-playlist/get-playlist.mjs(2 hunks)components/spotify/actions/get-recommendations/get-recommendations.mjs(1 hunks)components/spotify/actions/get-track/get-track.mjs(1 hunks)components/spotify/actions/remove-items-from-playlist/remove-items-from-playlist.mjs(1 hunks)components/spotify/actions/remove-user-saved-tracks/remove-user-saved-tracks.mjs(1 hunks)components/spotify/actions/save-track/save-track.mjs(1 hunks)components/spotify/package.json(1 hunks)components/spotify/spotify.app.mjs(0 hunks)
💤 Files with no reviewable changes (1)
- components/spotify/spotify.app.mjs
🧰 Additional context used
🧠 Learnings (3)
components/spotify/actions/create-playlist/create-playlist.mjs (1)
Learnt from: jcortes
PR: #14467
File: components/gainsight_px/actions/create-account/create-account.mjs:4-6
Timestamp: 2024-10-30T15:24:39.294Z
Learning: In components/gainsight_px/actions/create-account/create-account.mjs, the action name should be "Create Account" instead of "Create Memory".
components/spotify/actions/save-track/save-track.mjs (2)
Learnt from: jcortes
PR: #14467
File: components/gainsight_px/actions/create-account/create-account.mjs:4-6
Timestamp: 2024-10-30T15:24:39.294Z
Learning: In components/gainsight_px/actions/create-account/create-account.mjs, the action name should be "Create Account" instead of "Create Memory".
Learnt from: GTFalcao
PR: #14265
File: components/the_magic_drip/sources/common.mjs:35-43
Timestamp: 2024-10-10T19:18:27.998Z
Learning: In components/the_magic_drip/sources/common.mjs, when processing items in getAndProcessData, savedIds is intentionally updated with IDs of both emitted and non-emitted items to avoid emitting retroactive events upon first deployment and ensure only new events are emitted as they occur.
components/spotify/package.json (1)
Learnt from: jcortes
PR: #14935
File: components/sailpoint/package.json:15-18
Timestamp: 2024-12-12T19:23:09.039Z
Learning: When developing Pipedream components, do not add built-in Node.js modules like fs to package.json dependencies, as they are native modules provided by the Node.js runtime.
⏰ 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). (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (15)
components/spotify/actions/remove-items-from-playlist/remove-items-from-playlist.mjs (1)
9-9: Patch version increment acknowledgedNo logic changes detected;
0.1.2correctly signals an internal update. Just double-check that any automated publishing pipeline will pick up this new version.components/spotify/actions/save-track/save-track.mjs (1)
9-9: Consistent versioning maintainedThe file follows the same patch bump pattern as the other actions – good for keeping the bundle in sync.
components/spotify/actions/get-album-tracks/get-album-tracks.mjs (1)
10-10: Minor version bump observedSince this action was still at
0.0.x, moving to0.0.2is fine for a metadata-only edit. No further action needed.components/spotify/actions/create-playlist/create-playlist.mjs (1)
8-8: Version bump aligns with the rest of the suiteEverything stays in lockstep at
0.1.2. 👍components/spotify/actions/get-audio-features-for-a-track/get-audio-features-for-a-track.mjs (1)
9-9: Patch-level version bump looks fine
No functional code touched; patch increment is appropriate.components/spotify/actions/get-currently-playing-track/get-currently-playing-track.mjs (1)
10-10: Version updated to 0.0.2 – OK
Consistent with other modules; no further action required.components/spotify/actions/add-items-to-playlist/add-items-to-playlist.mjs (1)
9-9: Version bump acknowledged
Patch release matches the coordinated package update.components/spotify/actions/get-categorys-playlist/get-categorys-playlist.mjs (1)
9-9: Patch version increment confirmed
Change is metadata-only; implementation untouched.components/spotify/actions/get-all-tracks-by-artist/get-all-tracks-by-artist.mjs (1)
8-8: Minor version bump accepted
Keeps action aligned with the rest of the Spotify package.components/spotify/actions/get-artist-top-tracks/get-artist-top-tracks.mjs (1)
9-9: LGTM - Version bump is appropriateThe version increment from "0.1.1" to "0.1.2" is consistent with the package-wide version updates across all Spotify action components.
components/spotify/actions/get-playlist/get-playlist.mjs (1)
26-26: LGTM - Core bug fix addresses the issuePassing
playlistIddirectly instead ofplaylistId.valuecorrectly allows the action to handle both async options (objects with value/label) and custom expressions/variables (primitive values). This should resolve the 400 Bad Request error described in issue #17862.components/spotify/package.json (1)
3-3: LGTM - Package version bump follows semantic versioningThe version increment from "0.7.2" to "0.7.3" is appropriate for a patch release containing bug fixes and is consistent with the coordinated version updates across all Spotify action components.
components/spotify/actions/get-recommendations/get-recommendations.mjs (1)
8-8: LGTM - Version bump is consistent with package updateThe version increment from "0.1.1" to "0.1.2" aligns with the coordinated version updates across all Spotify action components in this release.
components/spotify/actions/get-playlist-items/get-playlist-items.mjs (1)
9-9: LGTM - Version bump is appropriateThe version increment from "0.1.1" to "0.1.2" is consistent with the package-wide updates. Note that this file already correctly handles
playlistIdusing theget(playlistId, "value", playlistId)pattern on lines 60 and 70, which is the same approach that should be applied to fix the summary export issue inget-playlist.mjs.components/spotify/actions/remove-user-saved-tracks/remove-user-saved-tracks.mjs (1)
8-8: ✅ Version bump acknowledged and consistentThe increment to
0.1.2aligns with the coordinated package update and follows semantic-versioning expectations. No further action required.
| playlistId, | ||
| }); | ||
|
|
||
| $.export("$summary", `The playlist with Id: ${playlistId.value} was successfully fetched!`); |
There was a problem hiding this comment.
Fix inconsistent property access in summary export
The summary export still references playlistId.value, but since playlistId can now be a primitive value (when using custom expressions), this will cause a runtime error when .value is undefined.
Apply this diff to handle both object and primitive playlist ID values:
- $.export("$summary", `The playlist with Id: ${playlistId.value} was successfully fetched!`);
+ $.export("$summary", `The playlist with Id: ${playlistId.value || playlistId} was successfully fetched!`);Alternatively, use the same pattern as other Spotify actions with lodash:
+import get from "lodash/get.js";
+
import spotify from "../../spotify.app.mjs";- $.export("$summary", `The playlist with Id: ${playlistId.value} was successfully fetched!`);
+ $.export("$summary", `The playlist with Id: ${get(playlistId, "value", playlistId)} was successfully fetched!`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $.export("$summary", `The playlist with Id: ${playlistId.value} was successfully fetched!`); | |
| $.export("$summary", `The playlist with Id: ${playlistId.value || playlistId} was successfully fetched!`); |
🤖 Prompt for AI Agents
In components/spotify/actions/get-playlist/get-playlist.mjs at line 29, the
summary export incorrectly accesses playlistId.value, which causes errors when
playlistId is a primitive. Update the code to safely handle both object and
primitive playlistId by checking if playlistId has a value property before
accessing it, or use lodash's get method as done in other Spotify actions to
retrieve the ID safely for the summary string.
|
Hi, thank you for your contribution. There's another PR here https://github.com/PipedreamHQ/pipedream/pull/17895/files, which contains less changes. I would like to close this one in favor of the other one. |
5 participants
WHY
Resolves #17862
Summary by CodeRabbit