17967 bug look into async options prop for business unit id for trustpilot returning bad info #18031
Conversation
bump versions
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
WalkthroughTrustpilot component versions were incremented across actions and sources. Several source and action files had imports reordered. The Trustpilot app now uses HTTP_STATUS for 429 retry logic and formatQueryParams for request params, and updates business unit options labeling to prefer name.identifying over displayName. Package version bumped. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TrustpilotApp
participant Axios
participant TrustpilotAPI
Client->>TrustpilotApp: call action/source
TrustpilotApp->>TrustpilotApp: formatQueryParams(params)
TrustpilotApp->>Axios: request(params)
Axios->>TrustpilotAPI: HTTP request
TrustpilotAPI-->>Axios: 429 Too Many Requests
Axios-->>TrustpilotApp: 429 response
TrustpilotApp->>TrustpilotApp: detect HTTP_STATUS.TOO_MANY_REQUESTS + sleep/retry
TrustpilotApp->>Axios: retry request
Axios->>TrustpilotAPI: HTTP request
TrustpilotAPI-->>Axios: 200 OK
Axios-->>TrustpilotApp: response
TrustpilotApp-->>Client: result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
✨ 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: 1
🔭 Outside diff range comments (9)
components/trustpilot/sources/new-product-review-replies/new-product-review-replies.mjs (1)
64-67: Bug: reply preview never appends ellipsis
replyPreviewis already truncated to 50 chars, soreplyPreview.length > 50is never true. The ellipsis will never be appended.- const replyPreview = item.text?.substring(0, 50) || ""; - const preview = replyPreview.length > 50 - ? `${replyPreview}...` - : replyPreview; + const fullText = item.text ?? ""; + const preview = fullText.length > 50 + ? `${fullText.substring(0, 50)}...` + : fullText;components/trustpilot/sources/new-conversations/new-conversations.mjs (1)
31-39: Align Business Unit label with new app-level label preferenceTo match the updated labeling (prefer name.identifying over displayName), update the summary fallback chain.
Apply:
const businessUnit = item.businessUnit?.displayName || this.businessUnitId || "Unknown"; + const businessUnit = item.businessUnit?.name?.identifying + || item.businessUnit?.displayName + || this.businessUnitId + || "Unknown";components/trustpilot/sources/new-service-review-replies/new-service-review-replies.mjs (2)
41-47: Ensure reply event IDs are globally unique and timestamps are accurateIf replies can be edited or re-posted, using only review.id may cause ID collisions. Also, prefer updatedAt when provided.
Apply:
- repliesWithReviews.push({ - id: `reply_${review.id}`, + repliesWithReviews.push({ + id: `reply_${review.id}_${review.company.reply.createdAt}`, reviewId: review.id, text: review.company.reply.text, createdAt: review.company.reply.createdAt, - updatedAt: review.company.reply.createdAt, // Replies don't get updated + updatedAt: review.company.reply.updatedAt || review.company.reply.createdAt, review: { id: review.id, title: review.title, stars: review.stars, consumer: review.consumer, }, });
61-66: Fix truncation logic so ellipsis actually appears when neededCurrently you take substring(0, 50) and then check replyPreview.length > 50, which can never be true. Check the original text length instead.
Apply:
- const replyPreview = item.text?.substring(0, 50) || ""; - const preview = replyPreview.length > 50 - ? `${replyPreview}...` - : replyPreview; + const replyPreview = item.text?.substring(0, 50) || ""; + const preview = (item.text?.length > 50) + ? `${replyPreview}...` + : replyPreview;components/trustpilot/sources/new-product-reviews/new-product-reviews.mjs (1)
31-38: Prefer name.identifying over displayName in Business Unit labelAlign with app-level label preference so summaries match the UI.
Apply:
- const businessUnit = item.businessUnit?.displayName || this.businessUnitId || "Unknown"; + const businessUnit = item.businessUnit?.name?.identifying + || item.businessUnit?.displayName + || this.businessUnitId + || "Unknown";components/trustpilot/sources/updated-conversations/updated-conversations.mjs (1)
31-39: Prefer name.identifying for Business Unit label in summaryKeep the summary consistent with the updated async option labels and app logic.
Apply:
- const businessUnit = item.businessUnit?.displayName || this.businessUnitId || "Unknown"; + const businessUnit = item.businessUnit?.name?.identifying + || item.businessUnit?.displayName + || this.businessUnitId + || "Unknown";components/trustpilot/sources/new-service-reviews/new-service-reviews.mjs (2)
32-38: Prefer name.identifying for Business Unit labelMirror the app-level preference so source summaries align with the new async option labels.
Apply:
- const businessUnit = item.businessUnit?.displayName || this.businessUnitId || "Unknown"; + const businessUnit = item.businessUnit?.name?.identifying + || item.businessUnit?.displayName + || this.businessUnitId + || "Unknown";
20-23: Private-only polling; no public fallback detected
ThegetPollingMethodcomment in components/trustpilot/sources/new-service-reviews/new-service-reviews.mjs (around line 21) claims “fallback to public if needed,” but the client’s_getReviewsalways uses the private endpoint and never invokesgetPublicServiceReviewson error.Action items:
- Either remove or update the comment in
new-service-reviews.mjsto reflect that only the private endpoint is used.- Or implement true fallback by catching failures from
getServiceReviewsand callingthis.trustpilot.getPublicServiceReviews(...)with the same params.Location:
- File: components/trustpilot/sources/new-service-reviews/new-service-reviews.mjs
- Lines: 20–23
Suggested diff:
getPollingMethod() { - // Use private endpoint first as it has more data, fallback to public if needed - return "getServiceReviews"; + // Polls the private endpoint only; remove fallback reference or implement public fallback + return "getServiceReviews"; },components/trustpilot/trustpilot.app.mjs (1)
34-39: Honor user search input in async options to avoid irrelevant results.Right now,
queryis hardcoded to an empty string (Line 37), ignoring any user-entered search. This can surface seemingly “incorrect” options. Pass the UI-provided search term tosearchBusinessUnits.Suggested change (outside the selected lines for diff application; adapt in place):
// Change signature to accept context with search async options({ search } = {}) { try { const businessUnits = await this.searchBusinessUnits({ query: (search ?? "").trim(), limit: 20, }); // mapping remains as updated in the previous diff } catch (error) { ... } }This aligns the options prop with how Pipedream surfaces filtered lists in the UI and should address the “bad info” complaint when users search.
♻️ Duplicate comments (1)
components/trustpilot/actions/fetch-service-review-by-id/fetch-service-review-by-id.mjs (1)
7-7: Version bump only — also ensure businessUnitId options return IDs.Same verification as noted for fetch-service-reviews applies here, since this action depends on
businessUnitId’s async options.If you didn’t already, run the script from the fetch-service-reviews comment to confirm label/value mapping for business units.
🧹 Nitpick comments (6)
components/trustpilot/actions/reply-to-service-review/reply-to-service-review.mjs (1)
37-45: Avoid repeated trim and null-guard message onceMinor cleanup: compute the trimmed message once and reuse it for validation, API call, and metadata.
- if (!message || message.trim().length === 0) { + const trimmedMessage = (message ?? "").trim(); + if (!trimmedMessage) { throw new ConfigurationError("Reply message cannot be empty"); } const result = await this.trustpilot.replyToServiceReview({ businessUnitId, reviewId, - message: message.trim(), + message: trimmedMessage, }); @@ - messageLength: message.trim().length, + messageLength: trimmedMessage.length,Also applies to: 55-55
components/trustpilot/actions/reply-to-product-review/reply-to-product-review.mjs (1)
30-37: Trim once and reuse for validation and payloadSame minor cleanup as the service reply action to simplify and avoid repeated work.
- if (!message || message.trim().length === 0) { + const trimmedMessage = (message ?? "").trim(); + if (!trimmedMessage) { throw new ConfigurationError("Reply message cannot be empty"); } const result = await this.trustpilot.replyToProductReview({ reviewId, - message: message.trim(), + message: trimmedMessage, }); @@ - messageLength: message.trim().length, + messageLength: trimmedMessage.length,Also applies to: 46-46
components/trustpilot/sources/updated-service-reviews/updated-service-reviews.mjs (1)
11-11: Naming nit: avoid “New Updated”The name reads awkwardly. Consider simplifying to “Updated Service Reviews”.
- name: "New Updated Service Reviews", + name: "Updated Service Reviews",components/trustpilot/sources/updated-product-reviews/updated-product-reviews.mjs (1)
11-11: Naming nit: avoid “New Updated”Suggest aligning the display name to “Updated Product Reviews”.
- name: "New Updated Product Reviews", + name: "Updated Product Reviews",components/trustpilot/sources/updated-conversations/updated-conversations.mjs (1)
10-11: Rename for clarity: “Updated Conversations”“New Updated Conversations” reads awkwardly. Recommend “Updated Conversations” to match the source key and intent.
Apply:
- name: "New Updated Conversations", + name: "Updated Conversations",components/trustpilot/trustpilot.app.mjs (1)
157-165: Leverage Retry-After header for rate limiting backoff (optional).The retry logic works, but many rate-limited APIs (including Trustpilot) send a
Retry-Afterheader that should be preferred over a fixed/backoff delay when present. This improves compliance and reduces unnecessary retries.Consider this refinement:
- if (retries > 0 && error.response?.status === HTTP_STATUS.TOO_MANY_REQUESTS) { - const delay = Math.min( - RETRY_CONFIG.INITIAL_DELAY * (RETRY_CONFIG.MAX_RETRIES - retries + 1), - RETRY_CONFIG.MAX_DELAY, - ); - await sleep(delay); + if (retries > 0 && error.response?.status === HTTP_STATUS.TOO_MANY_REQUESTS) { + const retryAfterHeader = error.response?.headers?.["retry-after"]; + const retryAfterMs = Number.isFinite(Number(retryAfterHeader)) + ? Number(retryAfterHeader) * 1000 + : null; + const backoffMs = RETRY_CONFIG.INITIAL_DELAY * (RETRY_CONFIG.MAX_RETRIES - retries + 1); + const delay = Math.min(retryAfterMs ?? backoffMs, RETRY_CONFIG.MAX_DELAY); + await sleep(delay); return this._makeRequestWithRetry(config, retries - 1); }
📜 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 (16)
components/trustpilot/actions/fetch-product-review-by-id/fetch-product-review-by-id.mjs(1 hunks)components/trustpilot/actions/fetch-product-reviews/fetch-product-reviews.mjs(1 hunks)components/trustpilot/actions/fetch-service-review-by-id/fetch-service-review-by-id.mjs(1 hunks)components/trustpilot/actions/fetch-service-reviews/fetch-service-reviews.mjs(1 hunks)components/trustpilot/actions/reply-to-product-review/reply-to-product-review.mjs(1 hunks)components/trustpilot/actions/reply-to-service-review/reply-to-service-review.mjs(1 hunks)components/trustpilot/package.json(1 hunks)components/trustpilot/sources/new-conversations/new-conversations.mjs(1 hunks)components/trustpilot/sources/new-product-review-replies/new-product-review-replies.mjs(1 hunks)components/trustpilot/sources/new-product-reviews/new-product-reviews.mjs(1 hunks)components/trustpilot/sources/new-service-review-replies/new-service-review-replies.mjs(1 hunks)components/trustpilot/sources/new-service-reviews/new-service-reviews.mjs(1 hunks)components/trustpilot/sources/updated-conversations/updated-conversations.mjs(1 hunks)components/trustpilot/sources/updated-product-reviews/updated-product-reviews.mjs(1 hunks)components/trustpilot/sources/updated-service-reviews/updated-service-reviews.mjs(1 hunks)components/trustpilot/trustpilot.app.mjs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
components/trustpilot/trustpilot.app.mjs (1)
components/reddit/reddit.app.mjs (1)
displayName(461-464)
⏰ 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 (19)
components/trustpilot/package.json (1)
3-3: Version bump looks good.Package version increment to 0.1.1 aligns with the component action/source version bumps in this PR.
components/trustpilot/actions/fetch-service-reviews/fetch-service-reviews.mjs (1)
7-7: Verified businessUnitId async options mappingThe
async options()in components/trustpilot/trustpilot.app.mjs correctly returns objects as:
- label:
identifying || displayName- value:
idNo changes required.
components/trustpilot/actions/fetch-product-review-by-id/fetch-product-review-by-id.mjs (1)
7-7: Version bump only — OK.No behavioral changes introduced. This action doesn’t rely on businessUnitId, so the app-level fix shouldn’t affect it.
components/trustpilot/actions/fetch-product-reviews/fetch-product-reviews.mjs (1)
7-7: Version bump only — OK; confirm businessUnitId async options.This action uses
businessUnitIdas well. Ensure the async options mapping now returns the correct ID in the value field after the app fix.Re-run the verification script from the fetch-service-reviews comment to validate the mapping across all consumers.
components/trustpilot/actions/reply-to-service-review/reply-to-service-review.mjs (1)
2-2: LGTM: import reordering and version bumpImport order change and version bump to 0.0.2 look good and are consistent with the broader PR.
Also applies to: 8-8
components/trustpilot/actions/reply-to-product-review/reply-to-product-review.mjs (1)
2-2: LGTM: import reordering and version bumpMatches the pattern across other actions. No functional impact.
Also applies to: 8-8
components/trustpilot/sources/new-product-review-replies/new-product-review-replies.mjs (2)
2-3: LGTM: import order normalization and version bumpImport ordering and version bump align with the rest of the Trustpilot components.
Also applies to: 5-5, 12-12
23-30: Business Unit ID async options mapping is correctVerified in components/trustpilot/trustpilot.app.mjs (lines 34–45) that
async options()returns an array of{ label: identifying || displayName, value: id }. All polling sources passthis.businessUnitId(the selectedvalue) into their API calls viagetPollingParams(). No changes required.components/trustpilot/sources/updated-service-reviews/updated-service-reviews.mjs (1)
2-3: LGTM: import order normalization and version bumpConsistent with other source modules; no functional change.
Also applies to: 5-5, 12-12
components/trustpilot/sources/updated-product-reviews/updated-product-reviews.mjs (1)
2-3: LGTM: import order normalization and version bumpMatches the broader consistency changes.
Also applies to: 5-5, 12-12
components/trustpilot/sources/new-conversations/new-conversations.mjs (2)
2-6: Version bump and import reorder look goodNo functional changes introduced here; safe metadata update.
Also applies to: 12-12
25-29: businessUnitId correctly uses the async option value (BU ID) end-to-endI’ve verified that in
trustpilot.app.mjsthebusinessUnitIdprop’sasync options()mapsvalueto each unit’sid, and that every source/action (includingnew-conversations.mjs) passesthis.businessUnitId(the raw ID) into API requests. No labels are used for request parameters—everything is wired to the ID. No changes needed.components/trustpilot/sources/new-service-review-replies/new-service-review-replies.mjs (1)
2-6: Version bump and import reorder look goodNo runtime logic changes here; safe to ship.
Also applies to: 12-12
components/trustpilot/sources/new-product-reviews/new-product-reviews.mjs (1)
2-6: Version bump and import reorder look goodNo behavior change; consistent with other Trustpilot sources.
Also applies to: 12-12
components/trustpilot/sources/updated-conversations/updated-conversations.mjs (1)
2-6: Version bump and import reorder look goodNo functional changes; matches the PR pattern.
Also applies to: 12-12
components/trustpilot/sources/new-service-reviews/new-service-reviews.mjs (1)
2-6: Version bump and import reorder look goodNo functional changes in this module; all good.
Also applies to: 12-12
components/trustpilot/trustpilot.app.mjs (3)
2-13: Good move to typed imports and HTTP status constant.Switching to defineApp and using HTTP_STATUS.TOO_MANY_REQUESTS improves readability and reduces magic numbers.
16-24: Centralizing param handling via formatQueryParams is sound.Routing all requests through formatQueryParams is a solid improvement for consistency and to avoid sending null/undefined params.
140-143: LGTM on param normalization in requests.Using
formatQueryParams(params)before sending requests avoids sending undefined/null values and keeps the query string clean.
| return businessUnits.map(({ | ||
| id, displayName, name: { identifying }, | ||
| }) => ({ | ||
| label: `${identifying || displayName}`, | ||
| value: id, | ||
| })); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix potential runtime TypeError in options() mapping and strengthen label fallback.
Destructuring name: { identifying } (Line 41) will throw if name is undefined for any business unit. Also, ensure a robust label fallback when identifying or displayName are missing. This can cause the async options UI to break or show blank labels, which aligns with the reported “bad info” symptom.
Apply this diff to make the mapping defensive and improve fallback:
- return businessUnits.map(({
- id, displayName, name: { identifying },
- }) => ({
- label: `${identifying || displayName}`,
- value: id,
- }));
+ return businessUnits.map((bu) => {
+ const { id, displayName, name } = bu || {};
+ const identifying = name?.identifying;
+ return {
+ label: `${identifying || displayName || id}`,
+ value: id,
+ };
+ });To ensure the mapping aligns with the normalized shape, please verify parseBusinessUnit returns { id, displayName, name } where name.identifying is present when available:
🏁 Script executed:
#!/bin/bash
# Locate and display the parseBusinessUnit implementation and its usage.
fd -a utils.mjs
rg -n -A 12 -B 6 'parseBusinessUnit'Length of output: 35623
Fix mapping to use identifyingName and strengthen label fallback.
Destructuring name: { identifying } will always crash because parseBusinessUnit returns a top-level identifyingName (no nested name object). Also guard against missing labels by falling back to id:
• File: components/trustpilot/trustpilot.app.mjs (around lines 40–45)
- return businessUnits.map(({
- id, displayName, name: { identifying },
- }) => ({
- label: `${identifying || displayName}`,
- value: id,
- }));
+ return businessUnits.map((unit) => {
+ const { id, displayName, identifyingName } = unit || {};
+ return {
+ label: identifyingName || displayName || id,
+ value: id,
+ };
+ });📝 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.
| return businessUnits.map(({ | |
| id, displayName, name: { identifying }, | |
| }) => ({ | |
| label: `${identifying || displayName}`, | |
| value: id, | |
| })); | |
| return businessUnits.map((unit) => { | |
| const { id, displayName, identifyingName } = unit || {}; | |
| return { | |
| label: identifyingName || displayName || id, | |
| value: id, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
components/trustpilot/trustpilot.app.mjs around lines 40-45: the current mapping
destructures a nested name.identifying which doesn't exist and will crash;
change the destructure to read identifyingName and displayName from the
top-level business unit and build the label with a robust fallback (use
identifyingName || displayName || id) so missing names fall back to id; update
the returned object to use value: id and label: `${identifyingName ||
displayName || id}`.
Resolves #17967
Summary by CodeRabbit
New Features
Chores